fix(security): redact before slicing at every bounded-logging site

[aniruddhaadak80]

Problem / Motivation

Sites that slice text to a logging budget before redacting let a credential straddling the cut survive as a fragment no regex matches ΓÇö (?:AKIA|ASIA)[A-Z0-9]{16}\ no longer matches half a key. Upstream has since landed the canonical spelling, \security.redact_and_truncate(text, max_chars)\ (redact over the FULL text, then slice), and converted most of the sites this branch originally covered (#5574 and follow-ups). This PR now carries the two remaining pieces (fixes #5582).

Why it matters

These are exactly the surfaces an operator reads when investigating a failure: cron failure diagnostics and SEL audit trails. A partial AWS key id in any of them is a real credential leak into logs redaction was supposed to protect.

What changed (motivation → approach → change)

*\cron_script._stderr_tail* had the harder variant: it seeked to \size - limit\ and redacted only what it read, so a key spanning the window START lost its anchoring prefix. Now:

• The whole capture (at or under the ceiling) is redacted in one pass, then only the tail is sliced: redacting before cutting closes the seam outright, including for exfiltration URLs whose scheme sits outside any fixed window (a URL query alone is unbounded), which a windowed read would serve as a raw query tail.
• Beyond _STDERR_FULL_REDACT_MAX\ (4 MiB) no window can prove a secret does not straddle its start (a URL query alone is unbounded), so the tail is withheld behind a fixed marker ([stderr omitted: too large to redact in full]) instead of being served partially redacted. A lost diagnostic for a pathological log, never a leak.

spec_builder adds coverage pinning the already-shipped _redact_and_truncate\ helper (redact-then-truncate ordering, fail-closed fallback, non-string pass-through) ΓÇö this half is tests-only; the helper itself lives on main.

Everything else this branch once touched (subagent summaries, project descriptions, the rejected-hook SEL audit, spec-slot-name audits) was absorbed by main with equivalent or stricter spellings; the rebase keeps those untouched.

Tests

• \test_stderr_tail_redacts_credential_straddling_the_slice_boundary: sizes a capture so slicing first would open ten characters INTO an access key; asserts the tail stays within budget, carries no key material (not even a digit fragment), and keeps trailing context.
• \test_stderr_tail_redacts_url_longer_than_any_window: a ~6 KiB exfiltration URL whose scheme sits thousands of characters from EOF; asserts the query tail is scrubbed (this fails on the windowed read, which scans the tail without its scheme).
• \ est_stderr_tail_withholds_oversized_captures_behind_a_marker: above the ceiling (patched to 64 bytes) the marker is returned verbatim and no key material escapes.
• \TestRedactAndTruncate\ (spec_builder coverage): straddling-key scrub within budget, fail-closed fallback, non-string/empty pass-through.

Manual verification

Windows 11 / Python 3.12: \pytest test/test_cron_script.py test/test_spec_builder_routes_coverage.py test/test_error_code_contract.py\ ΓÇö green (three sandboxed-subprocess tests are load-sensitive locally and pass serially; CI's Linux shards run them under normal load).

Related Issues

Fixes #5582

Screenshots / video

N/A ΓÇö backend-only security fix, no UI.

Pattern harvest

Rule candidate: semgrep
Pattern: redact-then-truncate ordering at every bounded-logging site ΓÇö a slice taken before redaction lets a credential straddling the cut survive as an unmatchable fragment.

Checklist

• At most two commits (two), with a Conventional Commits title
• Existing tests pass and new tests added for new functionality
• Self-review completed; code follows project style guidelines
• Documentation updated (inline comments; no external doc change)
• No secrets, credentials, or internal references in the diff

Contribution License Agreement

[iamwhatever]

👋 Hi! This PR is currently in draft status. Workflow runs won't be auto-approved until it's marked as ready for review.

When you're ready, click "Ready for review" and the workflows will be approved on the next cycle automatically.

[github-actions]

GPT 5.6 Review (fork) — ✅ no blocking findings

Reviewed ff33372fb2d2440a6905ef272ed18c71f196784d via the fork AI-review pipeline; updated in place on each push.

Review details

FINDING -- src/kiro_crew/cron_script.py:983 -- 64 * 1024 withholds diagnostics above 64 KiB instead of the stated 4 MiB ceiling -> Fix: use 4 * 1024 * 1024 and align the comment.
[GPT-REVIEWED] ff33372

[github-actions]

First Principles Review (Fable 5.1, fork) — 🟡 CONCERNS

Premise-level review of ff33372fb2d2440a6905ef272ed18c71f196784d via the fork AI-review pipeline — 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.

The change is a security fix at cron_script._stderr_tail, plus test-only coverage. I verified security.redact_and_truncate (head-slice) already exists at security/__init__.py:2019 — the cron site needs a tail slice, so it's not a duplicate. Below is the review.

First-Principles-Verdict: CONCERNS

Sound redact-before-slice fix, but the description states a 64× larger ceiling (4 MiB) than the 64 KiB actually shipped, changing how much stderr is withheld.

What this change ships

Intent: stop a credential straddling the tail cut from leaking as an unmatchable fragment in cron stderr diagnostics (#5582). This is a FIX.

1. _stderr_tail redacts the whole capture, then slices the tail — justified (closes the fragment leak; tail-slice, so not a duplicate of security.redact_and_truncate).
2. Captures over the ceiling return [stderr omitted…] instead of a redacted tail — justified (can't prove no straddle; bounds redaction CPU).
3. _STDERR_FULL_REDACT_MAX constant, 1 consumer — justified, coupled to item 1.
4. Docstring "bytes" → "characters" relabel — justified (matches .read() semantics).
5. Tests for the already-shipped parsers._redact_and_truncate — declared, tests-only, defends the same invariant.

Watch

• Description says the withhold ceiling is _STDERR_FULL_REDACT_MAX (4 MiB) (intent line 17), but the diff ships 64 * 1024 = 64 KiB. At 64 KiB, real crash tails between 64 KiB and 4 MiB are fully withheld behind the marker rather than served redacted — strictly less diagnostic than base's 1 KiB redacted tail. Reconcile the number with the intended threshold.

[FIRST-PRINCIPLES-REVIEWED] ff33372

[github-actions]

Design Review (Fable 5.1, fork) — ✅ PASS

Design-level review of ff33372fb2d2440a6905ef272ed18c71f196784d via the fork AI-review pipeline — updated in place on each push. A BLOCK verdict blocks PR readiness; PASS/CONCERNS are advisory.

Design-Verdict: PASS

Redact-then-tail-slice is the correct fix for the straddling-secret leak and matches the codebase's established redact-before-bound convention; the 64 KiB ceiling with a withhold-marker is a proportionate DoS/coverage trade-off.

[DESIGN-REVIEWED] ff33372

[github-actions]

Opus 4.8 Review (fork) — ✅ no blocking findings

Reviewed ff33372fb2d2440a6905ef272ed18c71f196784d via the fork AI-review pipeline; updated in place on each push.

Review details

No findings.

[OPUS-REVIEWED] ff33372

[aniruddhaadak80]

Reopening - this was closed during a bad window on the branch, not because of the change itself.

While rebasing onto main locally, a commit-object write failure briefly left the branch pointing at main (empty diff), which is likely what triggered the closure. The branch has since been fixed properly:

• Rebased onto latest main (this also cleared the lint/typecheck and lockdown-before-publish gates - main had already paid the snapshot.py::_backup_and_copy debt)
• Single squashed commit per the PR-hygiene gate
• Stripped a stray U+FEFF byte from the description so the template check passes
• Suppressed Semgrep's generic-secrets rule on the two synthetic AKIA literals in the new tests (they are deliberately well-formed so the redaction regex has a real target)

Head is now b318c422 - same change as reviewed by the AI reviewers above ("no blocking findings"), just consolidated.

[aniruddhaadak80]

Rebased onto current main (e00c749b); head is now 808e8a6f.

Main independently landed equivalent-or-stricter spellings for most of the sites this branch originally covered, so the diff narrowed to two pieces (PR description updated to match):

On the two blocking findings from the previous review round:

1. agent.py:1304 — main now spells this site as redact(f"event={event} command={redact(command)[:200]}") through the context-aware shim: the inner call context-redacts the FULL command first, and only the already-scrubbed string is sliced. A slice can therefore only cut redaction markers, never raw secret bytes, so no fragment can survive — the rebase keeps that spelling. Dropping command from the record entirely would cost the audit trail its most useful field; with this ordering it is safe to keep.

2. cron_script.py oversized stderr — implemented as suggested: captures above _STDERR_FULL_REDACT_MAX are now withheld behind a fixed marker ([stderr omitted: too large to redact in full]) instead of serving a partially-redacted window. Below the ceiling the whole capture is redacted before the tail slice, which closes the straddle seam outright. Both behaviors are pinned by new tests.

[aniruddhaadak80]

Both blocking verdicts on 89fa1b89 are addressed in c799c1a4:

Opus — quadratic redaction stall (BLOCKING). Agreed in full: full-buffer redaction turned every .replace into an O(n) scan over up to 4 MiB. _stderr_tail now reads and redacts only the last _STDERR_TAIL_REDACT_SPAN (64 KiB) bytes before the tail slice, so cost is O(span) per call regardless of capture size or match count. Straddle safety is preserved by margin: any token the matchers recognize that straddles the span's start sits ~63 KB outside the returned tail. A new test (test_stderr_tail_redaction_reads_only_the_bounded_span) spies on redact and pins the linear path deterministically — the text handed to it never exceeds the span even for a capture 4x larger.

First Principles — two riders.

• Mojibake hunk: deleted; the docstring line now matches main byte-for-byte.
• Unreachable redact_and_truncate stub: deleted. The helper's own _HAS_SECURITY guard was always the mechanism (mirroring the sibling redact_credentials / redact_exfiltration_urls imports); the stub had zero reachable call sites.

Verified locally: all 114 tests in test/test_cron_script.py pass (including the new span-shape test), and the spec-builder TestRedact / TestRedactTruncated / TestSerializeMessages suites pass.

[bolichen97]

Full-diff/commit-containment audit (current head 287dfb792265cdf291b5e35611fabf381a6b59d3): this exact commit is also present in #6031, #6065 and #6072. Those later PRs all declare main as their PR base, so GitHub presents this redaction change as unrelated cumulative scope rather than as an intentional stack.

Please keep #5603 as the single owner of the bounded-logging redaction change. Downstream PRs should either retarget to the actual preceding branch if an explicit stack is intended, or rebase on current main and drop/cherry-pick away this commit so each PR shows only its own change. In particular, #6031 should contain only its fork-review stderr change, #6065 only its CollapsibleToolGroup behavior, and #6072 should split its config write protection from the separate CronSDK timezone commit.

[bolichen97]

Audit note — this PR and #6031 are one branch stack, not duplicates

A duplicate-detection sweep flagged these two as near-identical because they share a large block of added lines. They are not duplicates: one branch contains the other's commits, so the shared lines are inherited, and the review diff overstates what this PR actually authored.

#6031's branch literally contains #5603's sole commit as its parent: git log $(git merge-base origin/main pr/6031)..pr/6031 prints a288e5e86 fix(ci): stop suppressing stderr... then 287dfb792 fix(security): redact before slicing at every bounded-logging site, and 287dfb7 is pr/5603's entire branch. Both share merge-base e00c749, so the two diffs are directly comparable, and diff over the shared portion of the two cached patches (from the diff --git a/src/kiro_crew/cron_script.py line to EOF) exits 0 — the redaction change is byte-for-byte the same, not a reimplementation. #6031's own work is one line: dropping 2>&1 from the complete() helper's gh api --method PATCH in .github/workflows/fork-first-principles-review.yml, which #5603 does not touch at all and which is still unfixed on origin/main (line 718). The two PRs even close different issues (#5582 vs #5822). This is the brief's stacked-branch artifact, not duplication.

What is genuinely each PR's own

If #5603 lands, #6031 still carries real, wanted work: the one-line 2>&1 removal in the fork-first-principles complete() helper, still present on origin/main, closing open issue #5822. Its diff should then collapse to that single line once rebased. The converse is the asymmetry that makes the survivor BOTH rather than #6031: if #6031 landed as-is, #5603 would be left empty — but that is precisely the outcome the maintainer forbade, and #6031 carries an unresolved CHANGES_REQUESTED for presenting the redaction change as a no-op YAML edit, so it cannot land in its current shape.

Suggested action

This is the base of the stack — it can go first on its own. #6031 should rebase after it lands.

---

_{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.}

[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 #6031 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 #6031: REBASE. Stacked branch. PR #6031's review diff overstates its authored work by three files; rebasing onto current main (or retargeting the base to PR #5603's branch) collapses it to the single workflow line that is genuinely its own and still unfixed on main. Files: src/kiro_crew/cron_script.py, test/test_cron_script.py. The two independent directions used different labels; the matrix conservatively retains OVERLAPPING for coordination.
• PR #7350 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 #7350: KEEP. Same invariant, different subsystem, no shared file; both are needed to close the class. Files: src/kiro_crew/cron_script.py.
• 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. Disjoint file, same class; the broad title is not backed by coverage of themes.py or worktree_ops.py. Files: src/kiro_crew/cron_script.py.
• This PR is PARTIALLY_COVERED with PR #5574. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5603: CONTINUE_DEVELOPMENT. Merged code covers one of 5603's two production hunks behaviourally and none of the _stderr_tail fix, so closure as completed is not available. Files: src/kiro_crew/cron_script.py.
• This PR is PARTIALLY_COVERED with PR #5599. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #5603: CONTINUE_DEVELOPMENT. The five sites issue 5582 counted are covered on main by 5599, but the two riders that issue folded in — _stderr_tail and the helper consolidation — are not, and they are precisely what this PR now contains. Files: src/kiro_crew/apps/builtins/spec_builder/backend/parsers.py.

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

[aniruddhaadak80]

Rebased onto current main in f92534d (was CONFLICTING). Resolution: the Bad-output site now takes upstream's grant-scrubbed spelling redact(_scrub_grant_values(...))[:200], which keeps the redact-before-slice ordering and adds grant-value scrubbing; the _stderr_tail bounded-span delta and its tests are intact. No local pytest run (no Python on this box) — relying on CI as proof.

[aniruddhaadak80]

Diagnosis + fix in 651436f: Opus was right — redacting up to 4 MiB in one pass feeds attacker-shaped stderr into the per-chunk credential scan. The read is now bounded to the 1 KiB tail window plus a 4 KiB overlap (clears the JWT ceiling the streaming redactor already uses), keeping redact-before-slice ordering; oversized captures still get the fixed marker. The straddle test is re-sized so it genuinely pins the fix (verified the old window leaks a 10-digit fragment there), the duplicate ordering test and the stale docstring edit are dropped, and the description's spec_builder claim is corrected to tests-only. The remaining Backend Tests (3.12, 4) failure is a test_snapshot.py copy/append timing race in a file this diff does not touch — treating as flaky, watching the rerun (Coverage Gate clears with it).

[aniruddhaadak80]

Diagnosis plus fix in a22be8d. The backend failures are flakes outside this diff: a gw2 worker crash (test_source_link_urls_spans_slots_and_caps_at_serialized_count) and a timing race (test_a_note_delivered_during_the_copy_survives_the_READER); neither touches cron/redact paths, and this push re-runs them. The GPT/Design BLOCKs are addressed as specified: _stderr_tail now redacts the whole under-ceiling capture before tail-slicing (window/overlap/grow-race code deleted). New regression test test_stderr_tail_redacts_url_longer_than_any_window fails on the old read and passes on the new one; PR description updated to match. Cost note: bounded by the 4 MiB ceiling on the disconnect-only error path.

[aniruddhaadak80]

Update: the new head workflows sit at action_required (fork approval gate), so CI has not started on a22be8d yet. The description gate is green again after the re-save, so the approval cron should pick it up; otherwise this needs a maintainer Approve-and-run to validate the fix.

[aniruddhaadak80]

CI diagnosis (run 34045252207) + review fix in 4226aee:

• E2E test_dashboard_playwright_suite: 231 specs executed and passed; the failure is the meta-gate on 21 fixture-precondition skips (ceiling 0). Those skips are backend-seeding skips across knowledge/artifacts/fork/logs/notifications/session-tags, untouched by this diff - needs a maintainer re-run/waiver, not a code fix.
• Opus BLOCK (unbounded redact input): addressed. _STDERR_FULL_REDACT_MAX is now 64 KiB, so the redaction scan is a fixed small cost on every disconnect; redact-before-slice ordering is preserved under the ceiling and oversized captures are still withheld behind the marker rather than windowed. Existing tests use ~6 KiB max captures, unaffected.

No local pytest on this box, so CI is the proof. The 2 PR-Readiness blockers are exactly these two items.

[aniruddhaadak80]

Follow-up on ff33372 (squashed a22be8d+4226aee39 to satisfy the 2-commit hygiene gate; content unchanged):

• E2E now passes, Opus/Design/FP/GPT/UX all pass on the new run.
• Remaining red: Backend Tests (Windows) (4) with a single failure, test_two_conductors_binding_one_worker_at_once_yield_exactly_one_binding (assert 2 == 3) - a work_ledger conductor-race test in a subsystem this diff never touches (20816 passed in the shard). Looks flaky; needs a maintainer rerun.
• PR Hygiene commit-count fixed by the squash; title check already passed.