fix: redact git stderr before bounding in auto_improvement (#7333)

[CrysisDeu]

Summary

Closes #7333.

Subprocess stderr was sliced to a fixed character bound BEFORE redaction at 20 sites across the auto_improvement app. The downstream redaction pass needs a secret's full shape to match, so a credential straddling the bound leaked as an unrecognizable fragment. Reachability is by construction at the primary site: backend/commit.py passes an authenticated remote URL as git argv, and git echoes the full userinfo URL on an auth failure.

Every site now redacts the full text first and bounds after, keeping each site's existing bound:

• Head slices ((proc.stderr or '')[:N]) → redact_and_truncate(proc.stderr or '', N): backend/commit.py (6), backend/pr_watchers.py (2), spine/gate.py (1), spine/driver.py (9), profiles/github_repo/pr_recipe.py (2).
• Last-line alias slices (tail[0][:N] / err[0][:N] after splitlines()[-1:]) → redact_and_truncate(tail[0], N): backend/clone_setup.py (4), backend/pr_watchers.py:1340, profiles/github_repo/profile.py:974. Found by the pre-push review lanes — the issue's grep sweep cannot see the alias form. (backend/deps.py:114, the same alias site, was fixed upstream by PR #7316, merged 2026-09-01 — this PR's rebase dropped its duplicate and keeps main's version.)
• Tail slices (stderr[-400:]) → redact(text)[-400:]: spine/agent_runner.py:897 and :1015. Redact-then-tail rather than redact_and_truncate because the END of stderr carries the actionable error; a tail cut of already-redacted text can at worst split a redaction marker, never a secret.

Posture registry: clone_setup.py, profile.py, and gate.py are new redactor call sites classified in NON_EGRESS_REDACTION_MODULES (source-side pre-passes; their strings reach output only through this app's registered sinks); deps.py's entry landed with PR #7316. commit.py, driver.py, pr_watchers.py, pr_recipe.py, agent_runner.py were already registered sinks. Stale prose describing the old raw-slice behavior was updated in routes.py, security_posture.py, and a test docstring. Per main's gate-side log-redactor ratchet (TestGateSideLogRedactorSpelling), the 10 converted sites that are LOG lines (driver.py ×8, pr_recipe.py ×2) use the companion-aware redact_log_via_context(text)[:N] spelling — same redact-before-bound order; non-log error payloads and the persisted ledger note keep redact_and_truncate.

Deliberately out of scope: spine/proposer.py:83's unbounded r.stderr.strip() — with no slice, a credential keeps its full shape, which downstream pattern-based redaction can still match (reason recorded in the sweep test).

Overlap with PR #7316 (issue #7307): resolved — #7316 merged 2026-09-01 with the identical redact-then-bound fix for deps.py:114, and this PR's rebase onto post-#7316 main dropped its duplicate hunk. The app-wide structural pin below still covers deps.py (it scans every non-test module, and main's version passes).

Tests

New tests/test_stderr_redact_before_bound.py:

• Straddle regression at the highest-reachability site (materialize_queued_diff fetch failure): a fake secret is laid out so the 160-char bound falls INSIDE it, premise-guarded (start < 160 < start + len(secret), @ past the bound). Mutation-verified: reintroducing the raw slice fails 3 tests; a slice-then-redact reorder fails the straddle test (the cut drops the @host tail the userinfo regex needs).
• Structural class pin sweeping every non-test module of the app for head slices, multi-digit tail slices, and the tail[0][:N]/err[0][:N] alias forms, with the sanctioned redact-then-tail form excluded. The defect recurred file by file, so the sweep is what keeps the class closed.

Local gates all green: black/subprocess-encoding/isort/flake8 gates, mypy (1218 files clean), brand + harness-parity diff gates, full app test dir (1061 passed), test/test_security_posture.py (registry drift guards), and the six test/test_ai_*_coverage.py files for every touched module (1006 passed). Full suite: 77,502 passed; the 92 failures + 2 errors reproduce identically on a pristine main checkout on this host (environment-inherited, verified via a detached main worktree).

Pre-push review: GPT (gpt-5.6-sol) and Opus (claude-opus-5) lanes, one round each; all verified findings fixed (the alias and tail sites above), advisories addressed (stale posture prose, gate.py classification reason names the GateResult/ledger travel path, sweep limits documented in-test).

No UI change — backend only, no screenshots required.

Pattern harvest

Rule candidate: a fixed-character bound applied to subprocess output (stderr especially) BEFORE redaction defeats pattern-based redaction — a mid-value cut leaves a fragment no credential regex can match, in head-slice ([:N]), tail-slice ([-N:]), and last-line-alias (splitlines()[-1:] then tail[0][:N]) forms. Flag any changed line that bounds subprocess output without a preceding redact pass; the sanctioned forms are redact_and_truncate(text, N) and redact(text)[-N:]. This PR ships the app-scoped structural pin (tests/test_stderr_redact_before_bound.py); the harvest candidate is promoting the same scan repo-wide (AUTOSDE recurring-defect-patterns or a semgrep rule), since the class already recurred across two issues (#7307, #7333) and eight modules.

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] d86ef88

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

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

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Redact-then-bound at each build site, plus a mutation-verified straddle test and a structural sweep, closes the leak class rather than one instance.

Suggestions

• The regex sweep has self-documented evasions (multi-line sites, renamed aliases); a follow-up that redacts stderr once inside the app's _git/_gh capture wrappers would close the class by construction and shrink the sweep to a backstop.

[DESIGN-REVIEWED] d86ef88

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of d86ef88ea2af391ab102c3b81ae1a0acebe5c84c — 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 evidence gathered. Composing the review.

First-Principles-Verdict: CONCERNS

Every item is derived from issue #7333 and closed as a class in-app — but two counted same-defect siblings outside the app and a mechanism the description misnames deserve human eyes.

What this change ships

Intent: stop a credential in git/gh stderr from escaping redaction when the error text is cut to a fixed length — a FIX.

1. Served error payloads redact git stderr before the char bound (10 sites) — justified
2. Log lines quoting git stderr use the companion-aware log redactor first (10 sites) — justified, mandated by the TestGateSideLogRedactorSpelling ratchet
3. Straddle regression test at the fetch-failure site — justified
4. App-wide structural sweep test pinning the class — justified (defect counted in 8 modules)
5. Three modules classified in NON_EGRESS_REDACTION_MODULES — justified (posture drift guard mandates classification)
6. Stale prose updated in routes.py, security_posture.py, one test docstring — rides along, mandated by the present-tense comment convention

Watch

• The description says head slices became redact_and_truncate(proc.stderr or '', N) and "the persisted ledger note keep[s] redact_and_truncate", but the diff ships redact_via_context(...)[:N] at all 10 payload sites including the ledger note (driver.py:1500). Same redact-then-bound order and the spec-canonical shim, so behavior is right — the description names a helper the diff never calls.
• Grepping stderr[^\n]*\[:\d+\] repo-wide counts ~18 raw bounded slices in ~13 modules outside this app, two of which are the exact slice-then-redact defect this PR fixes: dashboard/handlers/themes.py:306 (redact_credentials(proc.stderr.strip()[:200]) on a clone of an operator-supplied URL — token-bearing by the same construction as commit.py) and apps/builtins/dev_fleet/worktree_ops.py:1239 (runtime._redact((stderr or stdout).strip()[:300]), the mirror-image form the new sweep refuses in-app). The description's harvest section defers the repo-wide scan — accepted-and-deferred, but themes.py:306 is reachable today and worth fixing before the harvest lands.

[FIRST-PRINCIPLES-REVIEWED] d86ef88

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] d86ef88

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

[CrysisDeu]

🤖 Kiro Crew Auto-Pipeline [operator: CrysisDeu#0c98c3a2]

Disposition for the First Principles CONCERNS on bf3a34a5f:

Finding: 3 same-class sites outside the app (dashboard/handlers/themes.py:306, apps/builtins/dev_fleet/server.py:4598, :5336) that the app-scoped sweep cannot see. Accepted — verified against the code, all three are real (redaction fed pre-cut text; the :5336 tail cut keeps a straddling secret's right half). Routed to issue #7374 rather than folded in: this PR's scope is issue #7333's app (the branch, tests, and posture entries are all app-scoped), the three sites live in two unrelated subsystems with their own local _redact wrappers and test surfaces, and the sweep-promotion path (repo-wide scan) is already proposed in this PR's Pattern harvest section — #7374 references it as the follow-up vehicle. The issue carries the exact sites, the reorder recipe, and the straddle-test contract so it is dispatchable as-is.

[bolichen97]

Audit note — part of this has already landed; the rest has not

This PR is not a duplicate and is not finished by anything on main. The audit checked it part by part against main, and some of what it does is already there. Flagging it so a reviewer does not have to rediscover the overlap, and so the PR is not mistaken for fully-covered work.

Already landed

• ref: fix(auto_improvement): redact pip stderr before bounding in install_deps (#7307) #7316; kind: PR; state: closed; merged_at: 2026-09-01T01:07:27Z; main_commit: 7eea2be; subject: fix(auto_improvement): redact pip stderr before bounding in install_deps (Unredacted pip stderr returned to the dashboard in auto_improvement deps.py (bound-before-redact) #7307) (fix(auto_improvement): redact pip stderr before bounding in install_deps (#7307) #7316); landed_confirmed: True; confirmed_from: the issue/PR reference check line 540 (PR, closed, merged_at non-'-') AND the landed-commit index for main line 195; covers: 1 of 20 code sites (backend/deps.py pip-failure tail), 1 of 4 NON_EGRESS_REDACTION_MODULES entries, and the deps.py straddle regression test; is_ancestor_of_pr_head: False

Which parts main already has

3 of 16 parts: backend/deps.py's pip-failure redact-before-bound reorder (origin/main:.../backend/deps.py:123), its NON_EGRESS_REDACTION_MODULES registration (security_posture.py:1371), and its straddle regression test (tests/test_backend_deps_cov80.py:249) — all landed together as #7316 / 7eea2be.

What is still genuinely yours

13 of 16 parts: 19 of the 20 code sites (commit.py x6, driver.py x9 -- wait, counted per module: commit.py 6, clone_setup.py 4, pr_watchers.py 3, driver.py 9 [note the PR body counts 9 driver edits], gate.py 1, pr_recipe.py 2, profile.py 1, agent_runner.py 2 tail cuts) are all still raw on origin/main; 3 of the 4 posture registrations (clone_setup.py, profile.py, gate.py) plus the rationale comment; both prose refreshes (routes.py, security_posture.py:~445); the test docstring refresh; and the entire new test file including the commit.py straddle pair and the app-wide structural class pin.

Suggested action: REBASE — the remainder is real work; rebase onto the landed part rather than closing.

---

_{From a repository-wide duplicate/overlap audit of every pull request open against main (2026-09-02, 330 PRs, one reviewer per PR). Each PR was read as its full merge-base diff plus its description and every comment and review, then compared against each candidate PR's own diff and against origin/main at 1a765b88ceb7. This PR is not being closed — the note is informational. If the reading is wrong, please correct the reasoning rather than just the conclusion.}

[CrysisDeu]

🤖 Kiro Crew Auto-Pipeline [operator: CrysisDeu#0c98c3a2]

Disposition for the GPT BLOCKING finding on a696f6665:

• Baseline redaction misses companion credentials (commit.py:142 and equivalent non-log sites) — fixed in b17220927. All 24 non-log payload sites (commit.py ×6, clone_setup.py ×4, pr_watchers.py ×3, gate.py ×1, driver.py ledger note ×1, profile.py ×1, agent_runner.py tail cuts ×2, plus their imports) now route through the canonical companion-aware shim redact_via_context(text)[:N] exactly as the finding prescribed — full-text redaction through the active PlatformContext (companion regexes apply; fail-closed on a composition-failed non-standalone host) before each bound. Log lines keep redact_log_via_context per main's gate-side ratchet; the structural sweep test's sanctioned-form pattern covers both shims. Verified locally: 1090 app + posture tests green, all lint/type gates green.

[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

• This PR is OVERLAPPING with PR #5081. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7350: KEEP. Real merge-order interaction on the same two lines and the same posture tuple. Sequencing needs a decision: land PR #7350 first and PR #5081 must carry the reorder into pr_recipe_base.py to pass the new sweep, or land PR #5081 first and PR #7350 must retarget those hunks. Files: src/kiro_crew/apps/builtins/auto_improvement/profiles/github_repo/pr_recipe.py, src/kiro_crew/security_posture.py.
• This PR is OVERLAPPING with PR #5603. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7350: KEEP. Same invariant, different subsystem, no shared file; both are needed to close the class. Files: src/kiro_crew/cron_script.py.
• This PR is OVERLAPPING with PR #7383. The goals differ or the implementations can complement each other; this is not a duplicate claim. Recommended action for PR #7350: KEEP. Deliberate scope split recorded in this PR's own disposition; no file or line contention. Files: src/kiro_crew/dashboard/handlers/themes.py.

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

[bolichen97]

Auto-triage Tier 1: fix:, 12 files — applies redact_via_context before character bounds on all git/gh subprocess stderr paths in the auto_improvement backend, so a credential in an echoed remote URL cannot be sliced mid-match and escape downstream redaction passes.

Qualifies: fix: category, 12 files (<=20 cap), all 5 AI reviewer checks success on head d86ef88 with 0 annotations, Semgrep 0 annotations, no CodeQL open alerts, security checklist all-NO (adds redaction, does not modify auth/trust-boundary/sandbox/secret handling logic).