fix(auto_improvement): redact pip stderr before bounding in install_deps (#7307)

[bolichen97]

Closes #7307.

Problem

install_deps() in src/kiro_crew/apps/builtins/auto_improvement/backend/deps.py returned pip's stderr tail line in the handler's error payload with two defects:

• No redaction at the source: the stderr text never passed a scrubber in this module. The child pip inherits the gateway environment, so an authenticated private index (userinfo credentials in PIP_INDEX_URL) echoes its request URL — token and all — to stderr on an auth failure.
• Bound before redact: tail[0][:200] sliced first. This is the defect that stays reachable end to end: the serving route (_handle_deps_install in routes.py) does redact what it sends, but its credential regexes need the full credential shape to match. A token straddling the 200-char boundary is cut mid-match (e.g. https://ci-bot:TOKENedg, no trailing @host), so the fragment sails through the route's redact() and renders in the dashboard. Verified empirically: the sliced fragment survives redact() on the old code; the issue's stronger claim that the payload reaches the UI with no redaction pass at all does not hold (the route is fail-closed), but the partial-fragment leak does.

This is the same mechanism fixed for handlers/memory.py in #7283 (issue #7279); deps.py was the only remaining pip-stderr site with the slice-first shape (it evaded the stderr.decode grep because it uses text=True).

Fix

• Redact the full tail line, then let redact_and_truncate(tail[0], 200) apply the bound — the redact-before-bound invariant documented on redact_and_truncate itself (security.py). No manual slice remains anywhere in the expression.
• Import follows the existing from kiro_crew.security import … convention used by sibling backend modules (commit.py, mcp_server.py, routes.py). No import cycle: the module imports cleanly at module scope.
• Registered apps/builtins/auto_improvement/backend/deps.py in NON_EGRESS_REDACTION_MODULES (security_posture.py): it is a source-side pre-pass, not an egress boundary — the payload reaches the dashboard only through routes.py, the registered sink for this app. This is what the posture drift-guard (test_every_redactor_call_site_is_a_registered_sink_or_allowlisted) requires of every new redactor call site, and it is the test that failed on the first revision of this PR.

Scope note (exception branch): the install failed: {exc} branch two lines earlier is left unchanged deliberately. An OSError/SubprocessError string is a spawn failure (strerror, argv); pip credentials ride the environment (PIP_INDEX_URL), never argv, so that string cannot carry the index token, and the route's fail-closed redact() already covers it as defense in depth. Wrapping it would widen the diff without a reachable defect behind it.

Tests

Two regression tests in TestInstallDeps (test_backend_deps_cov80.py), stubbing subprocess.run (no real network/pip):

1. test_a_pip_failure_does_not_leak_index_credentials_to_the_payload — a credential-bearing URL in stderr must not survive into error (pins defect 1 at the source).
2. test_a_pip_failure_credential_is_redacted_before_it_is_bounded — lays the token out to start at index 190 so the 200-char bound cuts ten characters into it, asserts the exact prefix fragment a bound-before-redact implementation would leak (token[:10]) is absent, and carries a premise guard (assert start < 200 < start + len(token)) so the test can never silently stop straddling the boundary. The first revision's version of this test padded the token to start at index 202 — entirely past the bound — so it passed even on the unfixed code; this revision repairs the layout and locks the premise into the test itself.

Mutation-verified both ways: reverting the fix to the raw slice (tail[0][:200]) fails both tests; reordering to slice-then-redact (redact_and_truncate(tail[0][:200], 200)) fails the straddle test. Restored code passes.

Verification

• Full backend suite: python -m pytest 77379 passed / 339 skipped; 96 failures + 2 errors, all in the known host-environment baseline classes (AF_UNIX path length, sandbox contention), zero in the touched areas — verified by failure-file histogram.
• CI-pinned lint quad green: isort 6.0.0 / flake8 7.1.0 (0 findings) / mypy 1.14.1 (no issues in 1216 files) / scripts/check_black_formatting.py (CI-identical two-direction gate) all pass.
• Posture suite: test/test_security_posture.py 42 passed (including the drift-guard that failed on the first revision).

Pattern harvest

Rule candidate: semgrep — flag a subscript slice (x[:N]) applied to subprocess stderr/stdout that is formatted into a returned or served payload string unless the expression is wrapped by redact_and_truncate (redact-before-bound invariant). The security_posture drift-guard already harvests the sibling half of this class: every new redactor call site must be classified as sink or non-egress, so an unregistered fix like this one cannot land silently.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Verified: redact_and_truncate (security.py:11600) documents exactly the redact-before-bound invariant this fix leans on, routes.py is the fail-closed sink as described, and the posture allowlist entry matches the drift-guard's requirement. The straddle test pins the boundary with a premise guard. Description and diff match bidirectionally; the deliberately-unchanged exception branch is defensibly scoped (spawn errors carry argv/strerror, not the env-borne index credential, and the route still redacts it).

Design-Verdict: PASS

Root-cause fix at the source using the existing documented invariant helper, pinned by a boundary-straddling regression test — nothing to redesign.

[DESIGN-REVIEWED] 202e978

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 202e978a559241e5f463fde86619f4699a2dfe27 — 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 fix reuses the existing redact_and_truncate (whose docstring documents exactly this invariant), the posture registration is required by a real drift-guard test, and the framing is honest. The one depth finding: the root cause — slicing subprocess stderr before any redaction pass in a served payload — has counted unfixed siblings in this same app (the description scopes its "only remaining site" claim to pip stderr specifically, and defers the general fix to a proposed semgrep rule).

First-Principles-Verdict: CONCERNS

Mechanism-level fix reusing the right helper — but it's the second point patch in a series with 12 counted slice-first stderr siblings left in this app.

What this change ships

Intent: stop a pip index credential leaking (whole or as a boundary-cut fragment) into the dashboard's install-failure message — a FIX.

1. pip failure text is scrubbed of credentials before it is shortened — justified
2. deps.py registered as a non-egress redactor in security_posture.py — justified (drift-guard test requires it)
3. Two regression tests pin the leak and the redact-before-bound order — justified

Watch

• Point patch with counted unfixed siblings. The description's "deps.py was the only remaining pip-stderr site" is true only for the pip modifier; the root cause — a slice applied to subprocess stderr before any redaction pass, in a payload served through routes' fail-closed scan whose regexes need the full credential shape — has 12 sibling sites in this same app (grep stderr.*\[:\d+\]|tail\[0\]\[:): backend/commit.py ×6 ((proc.stderr or '')[:160], [:200]), backend/pr_watchers.py ×3, backend/clone_setup.py ×2, profiles/github_repo/profile.py:974 ×1. A git/gh credential straddling those bounds leaks the same fragment shape this PR fixes. The author names the general fix (the semgrep rule candidate) but ships neither it nor the siblings — accepted-and-deferred, and after fix(memory): redact pip stderr before logging install failures (#7279) #7283 this is already the second one-site pass at the same cause.

Subtractions

• Shrink the 8-line comment above safe_tail in deps.py to one line — it restates the invariant already documented on redact_and_truncate's own docstring (security.py:11601).

[FIRST-PRINCIPLES-REVIEWED] 202e978

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 202e978

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

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

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

GPT 5.6 completed its review of 202e978a559241e5f463fde86619f4699a2dfe27 and found no blocking issues.

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 202e978

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

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#bb3ad1ca]: This issue was claimed by this pipeline instance before the PR appeared, so rather than opening a duplicate I adopted this PR and pushed a repair commit (9c962f7df) on top — thank you, kiro-agent, for the correct core fix. What the repair addresses: (1) both red backend shards — the posture drift-guard requires every new redactor call site to be classified, so deps.py is now registered in NON_EGRESS_REDACTION_MODULES (source-side pre-pass; routes.py is the egress sink); (2) the straddle regression test was vacuous — its token started at index 202, past the 200-char bound, so it passed even on the unfixed code; it now starts at index 190 with a premise guard and is mutation-verified in both directions; (3) the black line-length gate on the new return line. PR body updated with the required Pattern harvest section and a precise reachability statement (the serving route does redact; the reachable defect is the mid-token cut that breaks the credential-regex match).

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision 202e978a559241e5f463fde86619f4699a2dfe27 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.

[bolichen97]

🤖 Kiro Crew Auto-Pipeline [operator: bolichen97#bb3ad1ca]: Answering the First Principles 🟡 CONCERNS (advisory) on 202e978a5:

Watch item (12 unfixed siblings, second point patch after #7283): Accepted — and acted on beyond deferral: the census is verified and tracked in #7333, now updated with every site FP counted plus one more both reviews missed (commit.py:330, push failed: [:200]). Verified census: commit.py ×6, clone_setup.py ×4, pr_watchers.py ×2, profile.py:974, spine/gate.py:105, pr_recipe.py:420. Why not folded in here: this PR adopts and repairs another author's single-site fix that CI had already mostly validated; widening it 13× would re-litigate the whole diff and delay closing the live, highest-reachability defect. #7333 proposes one class-wide pass with a parameterized straddle test instead of a third one-site PR — which is the same subtraction FP is arguing for, applied at the class level.

Subtraction (shrink the 8-line comment): Declined with rationale — the comment carries the one fact that is site-specific and NOT in redact_and_truncate's docstring: the serving route DOES redact downstream, and the mid-token cut is precisely what defeats its regexes. That is the reachability story a future editor needs before 'simplifying' the call back to a slice. Shrinking it now would also re-arm a full GPT re-review round for a cosmetic edit; if maintainers prefer the one-liner I'll fold it into any future push on this branch.

[bolichen97]

Open PR relationship audit

This is a consolidated, point-in-time code-level audit note. It compares complete merge-base diffs and current/merged code; it does not treat a shared topic as duplication or partial coverage as completion.

Relationship findings

• PR #7383 is OVERLAPPING relative to this PR. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7383: KEEP. Prior art that supplied the helper; it leaves this PR's three sites untouched. Files: src/kiro_crew/security.py.

No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit.