fix(slack): read the heartbeat-suppression log line through the composed context - #7762
fix(slack): read the heartbeat-suppression log line through the composed context#7762chenmingwei23 wants to merge 1 commit into
Conversation
…sed context The gate-side log line added by #7424 reached its text through `redact_and_truncate`, a BASELINE redactor, so a host with a companion loaded had that line scanned by the weaker OSS pass. This module's process composes -- `slack/gateway.py` calls `boot_platform` itself -- so the no-companion exemption that `redact_log_via_context` documents for `mcp_gateway.gatewayd` does not apply to it. Routed through `redact_log_via_context` and sliced afterwards, which is the contract that function states: truncating second is what keeps a credential from surviving as an unmatchable fragment. The census entry for the module therefore stays at 6 rather than being raised to 7.
37f5f50 to
53af662
Compare
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
Design Review (Fable 5) — ✅ PASSDesign-level review of The conversion checks out statically: the site at Design-Verdict: PASS Root-cause conversion at the designed seam — restores the census honestly instead of grandfathering a companion-blind log line, minimal and correctly scoped. [DESIGN-REVIEWED] 53af662 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All claims verified: First-Principles-Verdict: PASS One log line converted to the existing context-aware redactor, fixing the cause the ratchet fired on — nothing rides along. What this change shipsIntent: unblock the red CI shard by making the heartbeat-suppression log line run the composed context's redaction — a FIX.
The zero-cost alternative (census bump 6→7) was considered and rejected on the ratchet's own documented purpose — that rejection is derived, not preference. The 6 remaining baseline sites in [FIRST-PRINCIPLES-REVIEWED] 53af662 |
|
Closing in favour of #7758, which is the same fix and landed first. RohanK6 opened #7758 at 23:07Z, about 15 minutes before this one, and I did not see it before pushing. The two diffs are semantically identical: both add One review burden is better than two, so the evidence gathered here has been moved to #7758 rather than duplicated: that the failure is main-owned and reproduces on a clean The shard-3 census breach is fleet-wide, so whichever of the two lands heals every open PR. |
What is the problem?
test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactorfails onmain:The offender is main commit
7d05970c7("fix: redact full text before bounding at core-path slice sites (#7390) (#7424)"). Its change toslack/gateway.pywas a genuine improvement -- the heartbeat-suppression log line previously interpolatedtask_text[:80]with no redaction at all -- but it reached its text throughredact_and_truncate, which is a BASELINE redactor. That grew the module's gate-side baseline-log count from 6 to 7 without the census being raised, so the gate fires.This test runs on Backend Tests (Windows) shard 3, so every open PR is red on that shard regardless of its own contents.
Why it matters to the user
Two separate costs.
The CI cost is immediate: a failing required shard on every open PR, which also reds Coverage Gate and PR Readiness downstream. Nobody can tell their own breakage from this one.
The security cost is the reason this is a conversion rather than a census bump. A gate-side log line that reads a baseline redactor is scanned by the OSS pass even on a host that has a companion loaded, so the companion's extra credential and cookie patterns never run on that text. The line logs heartbeat task text, which is model-authored and can carry whatever the task was handling.
How our fix solves it
redact_log_via_context's docstring sets the standard for when a baseline redactor is acceptable at a log site: a process that deliberately does not compose keeps its logs, and it namesmcp_gateway.gatewaydas that case.slack/gateway.pyis not that process -- it callsboot_platform(cfg)itself, so it composes, and a companion's policy is exactly what should apply to its log lines.So the site is converted rather than grandfathered:
The slice comes AFTER the redaction, which is the contract that same docstring states -- truncating second is what keeps a credential from surviving as an unmatchable fragment. That is also precisely the bug class
7d05970c7set out to fix, so this keeps its intent and only corrects which redactor runs.The census entry for the module therefore stays at 6. Raising it to 7 would also have turned the shard green, but it would have recorded a companion-blind log line as accepted in the one process most likely to have a companion loaded, and
test_the_census_holds_no_slackexists to keep that number falling rather than climbing.What tests we did
test/test_security_posture.pypass, includingtest_the_census_holds_no_slack, which would fail had the count moved away from the census in either direction.origin/maincontaining none of my work reproduces the assertion byte-for-byte, and it passes at an older merge-base. It is platform-independent despite surfacing only on the Windows shard, because the assertion is a static census of source files and that shard is simply where the test is scheduled.Any other suggestions on the work
slack/gateway.pystill carries 6 grandfathered baseline log sites, and this PR deliberately does not touch them -- converting them is a wider change that deserves its own review. Two otherredact_and_truncatecalls in the same commit's hunk (task_safe,task_preview) are assignments feeding delivery rather than log writes, so the census does not count them and they are left alone.No
Closestrailer: no issue tracks this. It unblocks Backend Tests (Windows) shard 3 for every open PR.Pattern harvest
Rule candidate: when you add or change redaction on a gate-side LOG line, pick the redactor by asking whether that module's process composes a platform context, not by reaching for the nearest
redact_*helper. A process that callsboot_platformcomposes, so its log lines must go throughredact_log_via_context; only a process that deliberately does not compose (mcp_gateway.gatewaydis the one the docstring names) is entitled to a baseline redactor. Apply the truncation after the redactor returns, never inside it.Worth harvesting because the defect arrived inside a fix for the adjacent bug. #7424 was correcting redact-then-truncate ordering across many call sites, got that ordering right here, and still picked the companion-blind redactor -- so a reviewer scanning for the bug being fixed would see a correct diff. The
_BASELINE_LOG_SITE_CENSUSgate is what caught it, and the tempting way to clear that gate is to raise the number, which converts a caught defect into an accepted one. The census entry is the accounting, not the remedy.