Skip to content

fix(ci): route pip stderr logs through the context redactor - #7554

Closed
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/census-memory-py-7549
Closed

fix(ci): route pip stderr logs through the context redactor#7554
chenmingwei23 wants to merge 1 commit into
mainfrom
fix/census-memory-py-7549

Conversation

@chenmingwei23

Copy link
Copy Markdown
Contributor

Problem / Motivation

test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor
fails on main, which reds the Backend Tests shard carrying it on every open
pull request.

AssertionError: New gate-side log/audit line(s) reading the BASELINE redactor
dashboard/handlers/memory.py: 2 sites, census says 0.

2e627dc10 - "fix(memory): redact pip stderr before logging install failures
(#7279) (#7283)" - added two logging sites in
src/kiro_crew/dashboard/handlers/memory.py that redact through
redact_and_truncate, the OSS baseline, while
_BASELINE_LOG_SITE_CENSUS still records zero sites for that module.

Why it matters

Two effects. The shard is a required check, so every open PR carries a red lane
it did not cause and authors chase it into their own diffs first. And while the
guard sits red on a known breach it cannot flag the next one - so the class of
defect it exists to catch is unpoliced for as long as this lasts.

What changed (motivation -> approach -> change)

The guard's assertion names two ways out, and the choice between them is a
security question, not bookkeeping: route the sites through
redact_log_via_context, or raise the census and record which process the
baseline is correct for.

Raising the census is the wrong one here. Its precondition, stated in the
assertion message, is that "this PROCESS never composes a companion".
dashboard/handlers/memory.py runs in the dashboard process, which does
compose a platform context; the one process documented as deliberately not
composing is mcp_gateway.gatewayd (see redact_log_via_context's own
docstring). Raising the number would have recorded a reason that is not true.

So both sites move to redact_log_via_context. This is also the established
convention for exactly this payload: platform/update_provider.py's
CommandProvider.apply decodes a subprocess stderr from an install/update
command, routes it through the same helper for the same reason ("prime
territory for a host-specific credential shape, an internal registry cookie, an
SSO token in a fetch URL"), and then slices. The _log_ spelling is the one
that cannot raise, which a boot-time install step needs.

One module-local helper, _redacted_pip_stderr, owns both decisions for the two
call sites - which redactor, and that the length bound is applied to the
redactor's OUTPUT. That order is the invariant #7283 was built around and it is
preserved exactly: redacting the full text first is what stops a credential
straddling the 500-char boundary from surviving as an unmatchable fragment.

Two things deliberately NOT changed:

  • memory.py is not added to test_platform_context.CONVERGED_LOG_SITES. That
    list's second assertion requires a module to have no direct
    redact_credentials / redact_exfiltration_urls call at all, and this module
    still has one legitimately - for an API error payload, not a gate-side log
    line, which is why the census scanner counted 2 sites and not 3. The
    property-scanning guard in test_security_posture is what covers this module,
    and it now passes.
  • The census table itself is untouched. With both sites converted the module's
    live count is zero, which is what the table already records, so
    test_the_census_holds_no_slack stays satisfied without an entry.

Tests

Existing coverage: the four behaviour tests #7283 added
(TestPipStderrRedaction - userinfo credential masked on both sites, non-UTF-8
stderr does not raise, credential straddling the log bound leaks no fragment)
all pass unchanged. They pass under either redactor, because with no platform
context installed redact_log_via_context degrades to the same OSS pass - which
is precisely why this drift was silent, and why they are not sufficient on their
own.

New: TestPipStderrReadsTheContextRedactor, three tests, all three
mutation-verified red against the base source:

  • the ensurepip failure log reaches its text through
    redact_log_via_context (patched to a marker, asserted present in the record)
  • the faiss-cpu install failure log does the same, driven end to end through
    POST /api/memory/enable-embeddings
  • the bound is applied after redaction, not before: a fake redactor records what
    it was handed, so a bound applied to the INPUT shows up as a short call

Gates run locally: pytest test/test_enable_embeddings_faiss.py -n 4 (26
passed), the census and converged-sites guards
(test_security_posture.py + test_platform_context.py -k "census or baseline_redactor or GateSideLogRedactorSpelling or converged", 6 passed),
flake8 and isort clean on both files, mypy reports nothing in the touched
module, and the repo's baselined black gate passes in scope.

Manual verification

N/A - the failure and the fix are both fully expressed by the guard, which was
reproduced red at a pristine main and is green here. The changed lines only
run on a pip/ensurepip install failure, which the new end-to-end test drives
through the real handler.

Related Issues

Closes #7549

Pattern harvest

Rule candidate: agents-md
Pattern: a scan-based ratchet has no owner after merge, so it drifts in whichever
direction the last merge pushed it. This is the second census drift in this same
guard family within a day - test_the_census_holds_no_slack reported
dashboard/handlers/files.py: 1 sites, census says 3 earlier and has since
cleared - and both landed because the ratchet's inputs are computed from the
whole package while a PR's CI only ever ran against its own merge ref. Any PR
green at merge time can red the ratchet for everyone the moment a second PR
lands beside it, and nothing re-runs on main to catch it before the next
author does.

Worth noting the shape of this one: the breaching change was itself a redaction
fix, and it was the redaction census it breached. A contributor doing the right
thing at the call site had no signal that the helper they reached for was the
wrong spelling for that process, because the two spellings are behaviourally
identical on a host with no companion loaded. The issue's own suggestion is the
cheap half of the remedy - have the guard print the exact census edit it wants -
and the durable half is a note in the contributor guidance that a redaction
call in a gate-side log line picks its spelling by which process it runs in, not
by which import is nearest.

The gate-side log census fails on main: two logging sites added to
dashboard/handlers/memory.py read the OSS baseline redactor while the
census records zero for that module, so the Backend Tests shard carrying
the guard is red on every open pull request.

Route both pip/ensurepip stderr warnings through redact_log_via_context
instead of raising the census. The dashboard composes a platform context,
so an installer's stderr must be scanned with a loaded companion's
credential regexes; raising the census would assert the opposite. One
module-local helper owns the redactor choice and the redact-before-bound
order for both sites.

Closes #7549
@chenmingwei23
chenmingwei23 requested a review from a team as a code owner September 1, 2026 07:29
@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

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Right branch of the guard's fork taken — converting the sites to the context redactor fixes the cause, where raising the census would have recorded a falsehood.

Suggestions

  • The harvest note is right that the census ratchet re-reds on main after concurrent merges; a scheduled or merge-triggered main run of that guard is the follow-up worth filing.

[DESIGN-REVIEWED] ed4602c

@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 ed4602c8dae68c2889e3d4a7efdbc5f305b4211b and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ed4602c

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

@github-actions

github-actions Bot commented Sep 1, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] ed4602c

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

False positive or not applicable? A repository writer can comment:
/ai-review override fable ed4602c8dae68c2889e3d4a7efdbc5f305b4211b: <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 ed4602c8dae68c2889e3d4a7efdbc5f305b4211b — 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: the census guard exists with memory.py absent (expected 0), update_provider.py:459-467 is the established convention for the same payload, the module's remaining baseline call at memory.py:394-395 is an API payload (not a log line, so CONVERGED_LOG_SITES genuinely can't cover it), and the census table is untouched by the diff. The remaining ~19 redact_and_truncate( call sites package-wide are all policed by the same census guard, so none is an unfixed sibling of this defect.

First-Principles-Verdict: PASS

Unwedges a red required check by taking the correct of the guard's own two named exits, with the reason verifiable in the repo.

What this change ships

Intent: make the red test_no_new_gate_side_log_line_reads_the_baseline_redactor guard on main green by redacting pip stderr with the process-correct spelling. FIX.

  1. Both pip/ensurepip failure logs now redact via the context-aware helper — justified; the guard's assertion names exactly this exit, and update_provider.py:459 is the precedent for this payload.
  2. New module-local _redacted_pip_stderr owning redactor choice and redact-before-bound order — declared; 2 consumers (grep: both call sites), carries the invariant fix(memory): redact pip stderr before logging install failures (#7279) #7283 pinned.
  3. Three new tests pinning the redactor spelling and bound order — justified; the existing behaviour tests provably pass under either spelling, so they cannot pin this.
  4. Census table and CONVERGED_LOG_SITES deliberately untouched — justified; memory.py:394 still legitimately uses baseline components for an API payload, so the module-wide list cannot take it, and the live count now matches the recorded 0.

No item rides along, none duplicates an existing mechanism (no context-spelling redact-then-bound helper exists; grep def redact_log_via_context — one definition, no bounded variant), and the fix sits at the cause level the guard itself defines. Sibling count: 0 — every other baseline-redactor log site is either census-recorded or not gate-side, per the same passing guard.

[FIRST-PRINCIPLES-REVIEWED] ed4602c

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Dispositions for this revision (ed4602c8dae68c2889e3d4a7efdbc5f305b4211b), so nothing raised is left silent.

Design Review suggestion - "a scheduled or merge-triggered main run of that guard is the follow-up worth filing": accepted, and already tracked as #7511, "ci: main's runs are cancelled before the ratchet gates report, so drift surfaces on unrelated PRs". That issue is the general form of the same mechanism, so no duplicate is being filed. Deliberately not folded into this PR: it is a workflow-scheduling change with a different blast radius than a two-file redactor fix, and this PR exists to unwedge a required check now.

GPT 5.6, Opus 4.8, First Principles, Design: all four clean on this head, markers confirmed at ed4602c8d. No blocking or advisory findings left open.

Duplicate note: #7563 ("CI: gate-side redactor census fails on main (dashboard/handlers/memory.py, 2 sites vs census 0)") reports the same breach as #7549 and is fixed by this change. Flagging it here rather than adding a second Closes trailer, because editing the body re-triggers the review lanes on an already-clean head; #7563 can be closed as a duplicate of #7549 when this merges.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Superseded by #7572, which landed the same fix on main as ebc0936f2.

Closing rather than resolving the conflict: #7572 took the identical branch of the
guard's fork - both pip/ensurepip stderr logs now go through
redact_log_via_context, via a helper (_redact_pip_stderr) carrying the same
redact-before-bound order and the same rationale about the dashboard process
composing a companion. There is nothing left for this PR to add on the source side.

On my test-file delta, which #7572 did not touch: it is also redundant, and I
checked each of the three tests against what main now has rather than assuming.

  • Both call-site pins (each log reaches its text through the context redactor):
    a drift back to the baseline redactor re-reds the census scan in
    test_security_posture.py, and removing redaction outright is caught by the
    two existing masking tests in TestPipStderrRedaction.
  • The bound-after-redaction pin: already covered by
    test_credential_straddling_log_bound_does_not_leak_fragment, which plants the
    credential across the 500-character bound and asserts no prefix survives - so a
    reorder to truncate-then-redact fails on main today.

Verified the guard is green against main's content at ebc0936f2 before closing
(test_security_posture.py -k "census or baseline_redactor or GateSideLogRedactorSpelling", 5 passed).

One thing #7572 left behind, flagged rather than fixed here: the
TestPipStderrRedaction docstring still says both sites "must route the decoded
stderr through redact_and_truncate", which is now the wrong function - it names
the exact spelling the change moved away from, inside the class that guards the
behaviour. Worth a one-line correction on its own, not a reason to keep this PR open.

@github-actions github-actions Bot removed the readiness: passed Eligible automated validation passed for the current revision label Sep 1, 2026
@bolichen97
bolichen97 deleted the fix/census-memory-py-7549 branch September 6, 2026 03:55
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.

main is red: gate-side log census breach in dashboard/handlers/memory.py reds a required shard on every PR

1 participant