Skip to content

fix(slack): read the heartbeat-suppression log line through the composed context - #7762

Closed
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/gate-side-log-census-slack-gateway
Closed

fix(slack): read the heartbeat-suppression log line through the composed context#7762
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/gate-side-log-census-slack-gateway

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

What is the problem?

test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor fails on main:

New gate-side log/audit line(s) reading the BASELINE redactor
slack/gateway.py: 7 sites, census says 6.

The offender is main commit 7d05970c7 ("fix: redact full text before bounding at core-path slice sites (#7390) (#7424)"). Its change to slack/gateway.py was a genuine improvement -- the heartbeat-suppression log line previously interpolated task_text[:80] with no redaction at all -- but it reached its text through redact_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 names mcp_gateway.gatewayd as that case. slack/gateway.py is not that process -- it calls boot_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:

logger.info(
    "Heartbeat task incomplete, suppressing delivery: %s",
    redact_log_via_context(task_text)[:80],
)

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 7d05970c7 set 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_slack exists to keep that number falling rather than climbing.

What tests we did

  • The failing test now passes; all 47 tests in test/test_security_posture.py pass, including test_the_census_holds_no_slack, which would fail had the count moved away from the census in either direction.
  • 206 passed, 1 skipped across the heartbeat-related suites, covering the delivery path this line sits in.
  • Gates clean on the changed file: flake8, isort, the baselined black gate, and mypy.
  • Confirmed the failure is main-owned before changing anything: a detached worktree at origin/main containing 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.py still 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 other redact_and_truncate calls 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 Closes trailer: 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 calls boot_platform composes, so its log lines must go through redact_log_via_context; only a process that deliberately does not compose (mcp_gateway.gatewayd is 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_CENSUS gate 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.

…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.
@chenmingwei23
chenmingwei23 force-pushed the fix/gate-side-log-census-slack-gateway branch from 37f5f50 to 53af662 Compare September 1, 2026 23:24
@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 1, 2026
@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 53af6627e971e5b287c0b7fe71f9ebbe43694715 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 53af662

False positive or not applicable? A repository writer can comment:
/ai-review override gpt 53af6627e971e5b287c0b7fe71f9ebbe43694715: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — ✅ PASS

Design-level review of 53af6627e971e5b287c0b7fe71f9ebbe43694715 — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

The conversion checks out statically: the site at slack/gateway.py:5301-5306 now calls redact_log_via_context (not in _BASELINE_REDACTORS), the remaining two redact_and_truncate calls (lines 5310, 7771) are delivery-feeding assignments the census scanner's _is_log_write doesn't count, so the module's gate-side baseline count returns to the census value of 6. The helper's docstring in platform/context.py:737-770 confirms the PR's rationale verbatim — this process composes via boot_platform, so the composed-context spelling is the designed remedy, and the redact-then-slice ordering matches the contract. The PR deliberately converts rather than bumping the census, which is the gate's intended direction, and scopes to the one main-owned regression.

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

Reviewed 53af6627e971e5b287c0b7fe71f9ebbe43694715 — this comment is updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] 53af662

Verdict parsed from the review's SHA-scoped output markers for commit 53af6627e971e5b287c0b7fe71f9ebbe43694715.

False positive or not applicable? A repository writer can comment:
/ai-review override fable 53af6627e971e5b287c0b7fe71f9ebbe43694715: <one-sentence reason>

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 53af6627e971e5b287c0b7fe71f9ebbe43694715 — why this exists and whether the shipped surface is the smallest honest version. Updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

All claims verified: redact_and_truncate is baseline-redact-then-slice (security.py:12376), so the new spelling changes only which redactor runs, not the order or length; slack/gateway.py:10843 calls boot_platform(cfg), so the docstring's "process that deliberately does not compose" exception does not apply; the ratchet test fires on count > census (test_security_posture.py:1039), the census pins slack/gateway.py: 6 (test_security_posture.py:255), and redact_log_via_context is deliberately excluded from _BASELINE_REDACTORS (test_security_posture.py:57). The helper already exists with 5 other consuming modules — this PR consumes the existing mechanism rather than adding one. The alternative green path (bumping the census to 7) would be the symptom-level fix; the conversion is the cause-level one the ratchet's own design demands. The 6 remaining grandfathered sites are declared and pinned by the census in both directions.

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 ships

Intent: unblock the red CI shard by making the heartbeat-suppression log line run the composed context's redaction — a FIX.

  1. Heartbeat-suppression log text now gets companion redaction patterns, not only OSS baseline — justified (uses existing redact_log_via_context, 5 other consuming modules).
  2. If a loaded companion's policy fails to compose, that line's text is withheld behind a placeholder instead of baseline-redacted — justified, inherent to the named helper's documented fail-closed contract.
  3. Backend Tests (Windows) shard 3 goes green for every open PR — the fix; census stays at 6, held in both directions by test_the_census_holds_no_slack.

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 slack/gateway.py are counted, declared, and pinned by the census, so their deferral is already recorded by the repo's own mechanism.

[FIRST-PRINCIPLES-REVIEWED] 53af662

@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 1, 2026
@github-actions github-actions Bot added readiness: checking Automated validation is still running merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 2, 2026
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

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 redact_log_via_context to the same import block and both replace redact_and_truncate(task_text, 80) with redact_log_via_context(task_text)[:80] at the same call site. The only difference is that this PR also carried a four-line comment above the call explaining why the context-aware redactor is required there and why the slice comes second. Nothing here covers a case #7758 misses.

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 origin/main worktree, that it is platform-independent despite surfacing on the Windows shard, and that this exact change turns both Backend Tests (3.10, 3) and Backend Tests (Windows) (3) green -- which pre-confirms #7758's own shard-3 runs while they are still pending. See #7758 for the detail.

The shard-3 census breach is fleet-wide, so whichever of the two lands heals every open PR.

@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 2, 2026
@bolichen97
bolichen97 deleted the fix/gate-side-log-census-slack-gateway branch September 6, 2026 03:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

merge conflict Branch has merge conflicts with its base — author must resolve before merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant