fix: redact full text before bounding at core-path slice sites (#7390)

[CrysisDeu]

Summary

Closes #7390.

A bounded slice applied to text BEFORE it is passed to a redactor can cut a credential at the slice boundary into fragments no redaction regex matches, so the raw fragment escapes redaction and reaches the sink (log line, error payload, external display surface). PRs #7316/#7350/#7383 fixed this class at subprocess-stderr sites; this PR applies the same mechanical reorder — feed the redactor the FULL text, bound AFTER (security.redact_and_truncate(x, N) where both redactors apply, redact(x)[:N] otherwise) — at the core-path sites #7390 names.

Per site, the current head/tail anchoring is preserved exactly: every fixed site was a head cut and stays a head cut. Head-vs-tail anchoring judgment remains #5555's scope.

Sites fixed

All sites from #7390's list, re-located by pattern on current main:

• channel.py (approval-prompt tool input, 500)
• dashboard/chat_runner.py × 4: the cross-surface mirror echo (500), the PostToolUse hook output (2000), the model-refusal log head (600), plus a multi-line sibling the issue's single-line grep missed — the native sub-agent card task label (2000) and tool purpose meta (200)
• acp/_dispatch.py (tool-result join, 8000 — see below)
• dashboard/handlers/artifacts.py: prefix snippet, match-context snippet lines (a multi-line same-class sibling in the same function family), comment anchor strings (2000), comment/reply author (256 × 2), delete reason (500)
• dashboard/handlers/discover.py (skill preview content, 64 KiB)
• mcp_tools/control.py (task label spec fallback, 80; the task_name branch is used whole, exactly as before)
• mcp_tools/spawn.py (sub-agent error labels, 60 × 2)
• vector_memory.py (episodic reject audit snippet, 200)
• knowledge/agent_fetch.py (fetch error snippet, 300)
• slack/gateway.py (heartbeat task label, 100; plus the adjacent unredacted task_text[:80] log line, found in review)
• telegram/renderer.py (inline-keyboard button labels, 64 — bound now applied after redaction, upstream label size is schema-capped so work stays bounded)
• subagent_manager/continuation.py (follow-up label messages, per-element 120 — each message is redacted whole, then bounded; messages are separate strings so per-element redaction sees every secret intact)
• apps/builtins/auto_research/handlers.py (Knowledge Library source name, 60)

acp/_dispatch.py needed more than the outer reorder: _build_tool_result_event also cut each output PART at 4000 chars before the join, so a credential straddling a part's own cut was severed before any redaction ran. Each part is now redacted whole before its per-part bound; parts join on a newline and no redaction pattern matches across whitespace, so the join needs no second pass and the outer cut applies to already-redacted text.

Not touched (scope)

• The five sites in security: five slice-before-redact sites survive #5574's fix; route them through redact_and_truncate #5582 (claimed by another operator): subagent.py, handlers_project.py, agent.py, spec_builder/backend/routes.py × 2, plus its cron_script.py fold-ins.
• The hex-verified dashboard/handlers/files.py redact(raw[:7]) site (security: five slice-before-redact sites survive #5574's fix; route them through redact_and_truncate #5582's documented exemption — regex-verified short hex cannot carry a credential).
• chat_utils._redact_tool_field's 1 MB byte-level safety cap runs before its redaction passes; it is a pre-existing site outside Slice-before-redact: ~15 core-path sibling sites beyond subprocess stderr (channel, chat_runner, artifacts, mcp_tools, memory) #7390's list, the cut is marked with an explicit truncation notice, and a 1 MB head window is a different risk class. Left for a follow-up rather than folded in here.
• Repo-wide promotion of the AST scan is deliberately left out, matching the calls made in fix: redact git stderr before bounding in auto_improvement (#7333) #7350 and fix: redact stderr before the bound in themes and dev_fleet (#7374) #7383: a remaining-offender baseline still exists outside the files this PR owns (security: five slice-before-redact sites survive #5574's fix; route them through redact_and_truncate #5582's sites among them), so the scan is pinned to the 13 named modules instead.

Event-loop safety (review finding, fixed in this PR)

Removing a pre-slice can turn a bounded redaction into a full-text pass over size-uncapped input. Where that input is externally scalable and the call ran on the asyncio event loop, the pass now runs off-loop (asyncio.to_thread), with the bound applied to the redacted text:

• discover.py skill preview content (registry-publisher-controlled, size-uncapped)
• channel.py approval tool input (model-authored, size-unbounded)
• artifacts.py comment anchor/author/reason fields (request-sized)

_scan_artifacts' snippet path already ran in an executor; its full-text pass is additionally memoized per artifact version (_cached_snippet) so a listing pays it once per version, not once per request.

Two sites also swap redaction pass order as a side effect of adopting redact_and_truncate (credentials-then-exfiltration became exfiltration-then-credentials): this is the stronger order — credential scrubbing first can shorten a URL query below the exfiltration matcher's minimum and suppress whole-URL redaction — and matches the canonical helper's composition.

Tests

test/test_core_path_redact_before_bound.py (new), reusing the shape of PR #7383's test_theme_clone_stderr_redact_before_bound.py:

• Behavioral straddle tests laying a fake AWS key across the exact bound: the Telegram button-label cut, the artifact prefix snippet (straddle + redaction-shrink slide layout, with premise guards), the artifact context-snippet line cut, and the tool-result per-part cut in _dispatch.
• A structural AST scan over all 13 named modules: a char slice with a bound ≥ 10 (or any non-constant bound) may never appear inside a redact* call's argument expression. Slicing the redact call's RESULT is the sanctioned composition.

test/test_cross_surface_mirror.py::test_truncates_and_redacts updated: it pinned the old truncate-first order at the mirror-echo site, which #7390 names as a defect site; the assertion now pins redact-then-bound.

Verification

• Backend gates green: black baseline gate, subprocess-encoding gate, isort, flake8, mypy src/kiro_crew/ (0 issues), brand gate, harness-parity gate.
• Full python -m pytest: 77 735 passed; the only failures (96 + 2 errors) reproduce identically on a pristine main worktree on the same host (host-environment classes: AF_UNIX path length, home-dir project-root classification, xdist budget) — zero net-new failures from this branch.
• Targeted re-runs green after every review fix: the new test file plus channel, artifacts handlers, telegram, mcp tools, dispatch/render, redaction-parity, and cross-surface mirror suites.
• Pre-push review: two concurrent model-pinned reviewer lanes (GPT + Opus mirrors of the CI review contracts). Both initial BLOCK verdicts' findings were fixed in this PR: the _dispatch per-part cuts, the event-loop offloads, the adjacent raw log line, the snippet memoization, and a duplicated redaction pass now shared. Five root-level scratch files that an earlier head accidentally shipped (caught again by the First Principles lane) are removed from the diff as of the current head.

No frontend changes; no UI changes (no screenshots required).

Pattern harvest

Rule candidate: AST scan (landed in this PR, module-pinned)
Pattern: a bounded char slice inside a redact* call's argument expression (slice-before-redact) — the recurring class behind #7316, #7350, #7383, and this PR. test/test_core_path_redact_before_bound.py::_find_slice_inside_redact_call pins all 13 modules this issue names and goes red on any new slice with a bound ≥ 10 (or any non-constant bound) nested in a redactor's arguments. Repo-wide promotion is deliberately deferred (same call as #7350/#7383) while an offender baseline exists outside this PR's files — #5582's claimed sites plus the shape-identical subagent_manager/run.py, subagent_manager/terminal.py, and dev_fleet/server.py hits the First Principles lane enumerated. A second candidate the AST shape cannot express — a bounded slice on a value that only later flows into a redactor (the _dispatch per-part cuts fixed here) — needs data-flow awareness and is better suited to a semgrep taint rule than to this scan.

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of a3312481ea29dcd769aad2b7e7ccbad4b1e69963 — 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 checks complete: the canonical helper exists (security.py:11837), no pre-existing AST scanner duplicates the new one (grepped ast.Slice/ast.Subscript across test/ — only the new file), and I counted the remaining sibling sites in src/. Final review:

First-Principles-Verdict: CONCERNS

Solid cause-level fix of a real leak class, but the follow-up-label rewrite ships semantics the description misstates, and the sibling baseline deserves the count.

What this change ships

Intent: stop credentials cut in half by a truncation bound from escaping redaction at the sites #7390 names — a FIX.

1. Redact-then-bound reorder at 13 modules' display/log/label sites — justified (reported defect, matches redact_and_truncate's own docstring)
2. Tool-result parts lose the per-part 4000 cut; one redaction over the join — justified, declared
3. Full-text redaction moved off the event loop at 3 handlers — rides along, declared, derived from the fix
4. Artifact list snippets memoized per version (new cache + cap) — rides along, declared, derived (fix's cost)
5. Raw Slack heartbeat log line now redacted — rides along, declared, justified
6. Follow-up labels: per-message 120 cap becomes pooled 120×N on the join — declared inaccurately (see Watch)
7. Two sites swap pass order to exfil-then-credentials — declared side effect, justified
8. Native card + PostToolUse hook share one redaction pass — rides along, declared
9. New straddle tests + AST scan pinning the 13 modules — justified; no existing scanner (grep: ast.Slice in test/ hits only this file)
10. Mirror test re-pinned to redact-then-bound — justified (old test pinned the defect)

Watch

• Description says the continuation labels are "per-element 120 — each message is redacted whole, then bounded", but the diff redacts the JOIN and applies one pooled [:120 * len(label_msgs)] (continuation.py:524) — one long first message can now crowd later messages out of the label entirely. Behavior may be the right call (cross-message PEM), but the description describes a different implementation.
• Counted siblings of the exact class outside the 13 pinned modules: grep redact*( … [:NN finds 11 hits in 8 files — agent.py:1503, subagent_manager/run.py:1478, subagent_manager/terminal.py:617, apps/builtins/dev_fleet/server.py:4598, dashboard/handlers/themes.py:306, dashboard/handlers/agents.py:1488, acp/runtime.py:1752,1838, acp/session_handle.py:1006,1204,2427. Deferral is declared (security: five slice-before-redact sites survive #5574's fix; route them through redact_and_truncate #5582 claimed elsewhere), but themes.py:306 is a subprocess-stderr site — the class fix(auto_improvement): redact pip stderr before bounding in install_deps (#7307) #7316/fix: redact git stderr before bounding in auto_improvement (#7333) #7350/fix: redact stderr before the bound in themes and dev_fleet (#7374) #7383 supposedly closed.

[FIRST-PRINCIPLES-REVIEWED] a331248

[github-actions]

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound mechanical continuation of the established redact-then-bound fix class, but the hottest site — tool results in the ACP read loop — skipped the PR's own cost mitigation.

Watch

• The PR's stated criterion — "where that input is externally scalable and the call ran on the asyncio event loop, the pass now runs off-loop" — is not applied to _build_tool_result_event: it is sync in the ACP dispatch path, and the diff removes every per-part 4000 cut and redacts the unbounded join (final_output = _redact(joined)[:8000]) on every mid-stream tool_call_update. A tool that emits megabytes (cat a large file) now pays a full multi-pattern regex pass per update on the event loop, stalling all sessions on the shared read loop — the exact failure mode the PR fixed at discover.py/channel.py/artifacts.py.
• The snippet cache's drop-all-at-4096 valve plus per-version keying means a workspace with many large artifacts near the cap re-pays full-text redaction for the whole listing after each eviction; acceptable, but it is the second unbounded-cost consequence of the same reorder and worth a load sanity check.

Suggestions

• Move the _dispatch join redaction off-loop (the builder's caller is async), or cap the join at a bound far past 8000 with the shrink-slide analysis the artifact test already formalizes — either preserves the fix while restoring a cost ceiling on the streaming path.

[DESIGN-REVIEWED] a331248

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

FINDING -- src/kiro_crew/acp/_dispatch.py:1026 -- output_parts.append(str(text)) lets an oversized first block consume the 8,000-character budget and discard later blocks despite the stated per-part cap -> Fix: redact and cap each part at 4,000, then redact the join before its 8,000-character cap.
[GPT-REVIEWED] a331248

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] a331248

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

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

[jingchaodev]

Heads-up: this change added a 7th gate-side log site to src/kiro_crew/slack/gateway.py but _BASELINE_LOG_SITE_CENSUS in test/test_security_posture.py still says 6, so TestGateSideLogRedactorSpelling::test_no_new_gate_side_log_line_reads_the_baseline_redactor now fails on every PR's merge ref (first seen on #7436). Per the test's guidance the census needs raising (or the new site routed through redact_log_via_context).

[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. The merged sibling swept a disjoint module set; this PR's three sites remain broken on main. Post-merge consolidation of the two AST scanners is a cleanup, not a blocker. Files: test/test_core_path_redact_before_bound.py.

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