Skip to content

fix(ci): use context redactor for Slack log - #7758

Closed
RohanK6 wants to merge 1 commit into
kirodotdev:mainfrom
RohanK6:fix/slack-log-redactor-census
Closed

fix(ci): use context redactor for Slack log#7758
RohanK6 wants to merge 1 commit into
kirodotdev:mainfrom
RohanK6:fix/slack-log-redactor-census

Conversation

@RohanK6

@RohanK6 RohanK6 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

Current main is deterministically red in a required backend shard because
src/kiro_crew/slack/gateway.py has seven gate-side log sites that use the
baseline OSS redactor while the security census allows six.

Why it matters

Every pull request built against current main can fail the same Linux/Windows/
Python shard before its own changes are evaluated. This blocks PR Readiness and
prevents Stage-2 review dispatch across the repository.

What changed (motivation → approach → change)

PR #7424 correctly changed the heartbeat incomplete-task log to redact the full
text before bounding it, but selected redact_and_truncate. Slack gateway can
compose a platform companion, so gate-side logs must use the context-aware
redactor rather than expanding the baseline census.

This change routes only that logger argument through
redact_log_via_context(task_text)[:80]. Redaction still runs over the full text
before the display bound, while composed deployments receive the stronger
platform scan. The separate delivered-task preview remains on
redact_and_truncate because it is not a gate-side log argument.

Tests

  • test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor: passed
  • test/test_core_path_redact_before_bound.py: passed
  • Combined targeted set: 7 passed
  • Black, isort, flake8, mypy, and git diff --check: passed

Manual verification

N/A — the existing AST census and redact-before-bound tests exercise the exact
source contract.

Related Issues

Fixes #7757

Pattern harvest

Rule candidate: review-prompt
Pattern: A new logger/audit argument in a companion-capable process must use redact_log_via_context before any output bound, not the baseline redactor.

Checklist

  • At most two commits (one is the norm), with a Conventional Commits title (feat|fix|docs|refactor|perf|test|chore|ci|build|revert: ...)
  • Existing tests pass and new tests added for new functionality
  • Self-review completed; code follows project style guidelines
  • Documentation updated (N/A — no user-facing contract changed)
  • No secrets, credentials, or internal references in the diff

Contribution License Agreement

N/A — the repository template does not currently provide CLA attestation wording.

@RohanK6
RohanK6 requested a review from a team as a code owner September 1, 2026 23:07
@RohanK6
RohanK6 requested a review from pepmach September 1, 2026 23:07
@github-actions github-actions Bot added fork Pull request from a fork (external contributor) readiness: checking Automated validation is still running labels Sep 1, 2026
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
Route the heartbeat incomplete-task log through the composed context redactor before applying its display bound. This clears the required gate-side redactor census without weakening the redact-before-bound invariant.

sim: kirodotdev#7757
@RohanK6
RohanK6 force-pushed the fix/slack-log-redactor-census branch from 2aceb71 to 3b4c672 Compare September 2, 2026 00:09
@github-actions github-actions Bot added readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor

This clears a fleet-wide breach, and I can pre-confirm it: I arrived at a byte-equivalent change independently and measured it before your shard 3 finished. Evidence, in case it saves you the wait.

The failure is main-owned and hits every open PR. A detached worktree at origin/main containing no local changes reproduces it byte-for-byte:

test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor
AssertionError: ... slack/gateway.py: 7 sites, census says 6.

Offender is 7d05970c7 (#7424), which added the seventh gate-side baseline-redactor log site without raising _BASELINE_LOG_SITE_CENSUS.

It is platform-independent, despite surfacing first on the Windows shard. The assertion is a static census of source files; shard 3 is just where the test is scheduled. On my branch carrying the same change, both Backend Tests (3.10, 3) and Backend Tests (Windows) (3) went green, so your three pending shard-3 runs should follow.

Conversion is the right remedy over raising the census, which is the tempting one-character alternative. redact_log_via_context's docstring grants the baseline exemption only to a process that deliberately does not compose, and names mcp_gateway.gatewayd as that case. slack/gateway.py composes -- it calls boot_platform itself -- so a companion's patterns are exactly what should apply to its log lines. Raising the number to 7 would also have turned the shard green while recording a companion-blind log line as accepted in the process most likely to have a companion loaded, and test_the_census_holds_no_slack exists to keep that number falling. Your PR picks the same side of that.

Also verified locally on the identical change: all 47 tests in test/test_security_posture.py pass, including test_the_census_holds_no_slack, plus 206 passed / 1 skipped across the heartbeat suites.

I opened #7762 for the same fix about 15 minutes after yours before I noticed it. Yours is first and complete, so I am closing mine and deferring to this one -- one review burden rather than two. The only thing mine carried that this does not is a short comment above the call recording why the context-aware redactor is required there and why the slice comes after it; worth folding in only if you think a future reader would otherwise reach for the nearest redact_* helper again.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 2, 2026
@RohanK6

RohanK6 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor Author

Superseded by merged PR #7761 (74ffc2890), which applies the same context-aware redactor fix. Verified the exact gate-side census regression passes on current main. Closing without merge.

@RohanK6 RohanK6 closed this Sep 2, 2026
@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 2, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fork Pull request from a fork (external contributor)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

main is red: Slack log-site census breach reds required shard

2 participants