fix(ci): route pip stderr logs through the context redactor - #7554
fix(ci): route pip stderr logs through the context redactor#7554chenmingwei23 wants to merge 1 commit into
Conversation
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
Design Review (Fable 5) — ✅ PASSDesign-level review of 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
[DESIGN-REVIEWED] ed4602c |
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: |
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: the census guard exists with 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 shipsIntent: make the red
No item rides along, none duplicates an existing mechanism (no context-spelling redact-then-bound helper exists; grep [FIRST-PRINCIPLES-REVIEWED] ed4602c |
|
Dispositions for this revision ( Design Review suggestion - "a scheduled or merge-triggered GPT 5.6, Opus 4.8, First Principles, Design: all four clean on this head, markers confirmed at 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 |
|
Superseded by #7572, which landed the same fix on Closing rather than resolving the conflict: #7572 took the identical branch of the On my test-file delta, which #7572 did not touch: it is also redundant, and I
Verified the guard is green against One thing #7572 left behind, flagged rather than fixed here: the |
Problem / Motivation
test_security_posture.py::TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactorfails on
main, which reds theBackend Testsshard carrying it on every openpull request.
2e627dc10- "fix(memory): redact pip stderr before logging install failures(#7279) (#7283)" - added two logging sites in
src/kiro_crew/dashboard/handlers/memory.pythat redact throughredact_and_truncate, the OSS baseline, while_BASELINE_LOG_SITE_CENSUSstill 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 thebaseline 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.pyruns in the dashboard process, which doescompose a platform context; the one process documented as deliberately not
composing is
mcp_gateway.gatewayd(seeredact_log_via_context's owndocstring). 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 establishedconvention for exactly this payload:
platform/update_provider.py'sCommandProvider.applydecodes a subprocessstderrfrom an install/updatecommand, 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 onethat cannot raise, which a boot-time install step needs.
One module-local helper,
_redacted_pip_stderr, owns both decisions for the twocall 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.pyis not added totest_platform_context.CONVERGED_LOG_SITES. Thatlist's second assertion requires a module to have no direct
redact_credentials/redact_exfiltration_urlscall at all, and this modulestill 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_postureis what covers this module,and it now passes.
live count is zero, which is what the table already records, so
test_the_census_holds_no_slackstays satisfied without an entry.Tests
Existing coverage: the four behaviour tests #7283 added
(
TestPipStderrRedaction- userinfo credential masked on both sites, non-UTF-8stderr 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_contextdegrades to the same OSS pass - whichis precisely why this drift was silent, and why they are not sufficient on their
own.
New:
TestPipStderrReadsTheContextRedactor, three tests, all threemutation-verified red against the base source:
redact_log_via_context(patched to a marker, asserted present in the record)POST /api/memory/enable-embeddingsit 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(26passed), 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),flake8andisortclean on both files,mypyreports nothing in the touchedmodule, 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
mainand is green here. The changed lines onlyrun 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-mdPattern: 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_slackreporteddashboard/handlers/files.py: 1 sites, census says 3earlier and has sincecleared - 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
mainto catch it before the nextauthor 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.