fix(chat): fire redaction notice on the 7 exception-path assistant persists

[bolichen97]

Fixes #8311.

Summary

The credential-redaction notice (#6189 / PR #8109) previously fired only on the _flush_segment path. Seven exception/teardown branches in src/kiro_crew/dashboard/chat_runner.py persist the pasteable assistant body directly via slot.append("assistant", …, "msg msg-a"), bypassing _flush_segment — so a segment finalized through one of those branches got its credentials redacted correctly but never told the user.

This change extracts the count-tags-and-append-notice tail of _flush_segment into a module-level helper _append_redaction_notice(slot, redacted) and calls it from all 8 assistant-body persist sites so they share one notice contract.

Changes

• New module-level helper _append_redaction_notice(slot, redacted): sums occurrences of CREDENTIAL_REDACTION_TAGS in the exact persisted bytes and appends a single msg msg-info notice row when non-zero. Counting is artifact-based (reads tags in the persisted text, not the redactor's warning list), so it composes with the per-chunk redaction already done on the streaming paths.
• _flush_segment tail replaced with a call to the helper (behavior unchanged; notice still lands after the assistant row and before the trailing stop card).
• The 7 exception/teardown branches (CancelledError, AcpAuthRequired, AcpProcessDied, PromptBusyExhausted, AcpError transient, post-token transient recovery, terminal else) each call the helper immediately after their assistant persist and before any subsequent error/retry/stop card. The 4 inline-redact sites hoist the redacted value into a _redacted local so the helper sees the exact persisted bytes; the 3 _safe sites reuse the existing local.

Scope / divergence from the issue

At the current repo HEAD only the credential notice (#8109) has landed. The exfiltration-URL notice (#8291) referenced in the issue is not present in the file, so this fix propagates the existing credential-only notice and does not invent a URL notice. The issue's line numbers (from bbbf77567) are stale; sites were located by pattern, confirming the count of 7. The URL-exfiltration notice remains the separately-tracked #8291 concern.

Testing

• python -m pytest test/test_chat_runner_coverage.py → 277 passed (Python 3.12.13).
• black --check / isort --check-only / flake8 clean on both changed files.
• New tests in test/test_chat_runner_coverage.py: TestAppendRedactionNotice (helper units: clean → no notice, single/multiple credentials, encoded-credential tag counted, no secret leak) and TestExceptionPathRedactionNotice (drives a real AcpProcessDied turn, asserts ordering assistant → notice → error card).
• Mutation guard confirmed: removing the helper call from the AcpProcessDied branch makes TestExceptionPathRedactionNotice fail with "expected a redaction notice row", then restored.

Sequencing note

The issue triage suggested sequencing this after open PR #8291 (URL notice, same file) merges to avoid a conflict. This change is self-contained (adds a helper + call sites) and should compose cleanly with a later URL-notice landing, but reviewers may want to reconcile ordering with #8291 before merging.

Pattern harvest

Rule candidate: semgrep
Pattern: a user-visible side effect is implemented inline in one persist path while sibling persist paths write the same artifact directly. Concretely: an slot.append("assistant", ..., "msg msg-a") call in chat_runner.py that is not followed by the shared _append_redaction_notice(...) call. The defect class is a cross-cutting notice/audit tail coupled to one function body instead of to the artifact it describes, so every later branch that persists that artifact silently opts out. Extracting the tail into a named helper is what makes the rule expressible at all -- before this change there was no single symbol a linter could require.

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Root-cause fix: the notice tail is now coupled to the persisted artifact via one helper, not to _flush_segment's body — the right shape.

Suggestions

• Three slash-command persists (chat_runner.py:4186, 4285, 5883) run the same redact_credentials and write msg msg-a rows silently — the PR's own proposed semgrep rule ("slot.append(\"assistant\"…) not followed by _append_redaction_notice") flags them on day one; either add the call there or scope the rule to model-output persists before adopting it.

[DESIGN-REVIEWED] 607351b

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

[OPUS-REVIEWED] 607351b

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of 607351b04475e0ff0f33bd2950e52de99f8e2e06 — 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 verification is done. The 7 exception-path sites and the _flush_segment extraction check out; CREDENTIAL_REDACTION_TAGS is the redactor-owned constant; the helper has 8 real consumers. I counted the remaining redact-then-persist assistant sites in the file and found 3 the fix leaves without the notice (/workflow at 4186, /goal at 4285 via _redact_for_display, /prompts at 5883), which the PR's own harvested pattern would flag. Final review:

First-Principles-Verdict: CONCERNS

The fix is real and rightly shaped, but by its own harvested rule three redact-then-persist sites in the same file still opt out of the notice.

What this change ships

Intent: a user whose turn ends in an error/cancel is now told when a credential in the salvaged reply was replaced — a FIX.

1. Redaction notice now fires when a turn ends via cancel, auth loss, process death, busy-exhaustion, transient retry, or terminal error — justified (issue Redaction notice does not fire on the 7 exception-path assistant persists #8311/Credential redaction corrupts commands meant to be pasted into a terminal (silent, no warning) #6189; 7 sites verified in the diff).
2. Happy-path notice unchanged, its tail moved into shared helper _append_redaction_notice — justified; 8 counted consumers, all in chat_runner.py.
3. Notice lands between the assistant body and the following error/retry card — declared, pinned by test.
4. New module-level symbol other code may call — declared; it is what makes the author's lint rule expressible.

Watch

• Counted unfixed siblings: grepping chat_runner.py for redact_credentials/_redact_for_display feeding slot.append("assistant", …, "msg msg-a") without the helper leaves 3 sites — /workflow (src/kiro_crew/dashboard/chat_runner.py:4186), /goal (src/kiro_crew/dashboard/chat_runner.py:4285), /prompts list (src/kiro_crew/dashboard/chat_runner.py:5883). Each redacts a body that can echo user- or prompt-file content, so a tag written there is silent — the exact defect class the description names ("every later branch that persists that artifact silently opts out"). The description's "all 8 assistant-body persist sites" is true only for LLM assistant_text; the fix is one helper call per site.
• The helper still relies on every future persist site remembering to call it — the coupling moved up a level rather than being removed; the author's semgrep candidate is the acknowledgment. Acceptable as deferred, worth a human's eye on whether fix(dashboard): warn when redaction rewrites a URL in chat text #8291 lands on the same seam.

[FIRST-PRINCIPLES-REVIEWED] 607351b

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 607351b

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

[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 #7626 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 #7626: REBASE. Different goals (banner suppression vs redaction-notice parity) but a hard mechanical collision on the same seven persist sites, plus an accidental overlap in the user-visible outcome. Sequence them and let the loser rebase onto the winner's shape. Files: src/kiro_crew/dashboard/chat_runner.py.
• PR #8291 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 #8291: MERGE_DISCUSSION. Complementary halves of one fix that collide textually. Landing PR #8318 first and then re-applying PR #8291's by-kind wording inside the shared _append_redaction_notice helper gives URL notices on all eight persists at once; landing PR #8291 first forces PR #8318 to rebase and leaves the URL kind confined to _flush_segment until someone widens it again. Same author, so the sequencing is decidable rather than contested. Files: src/kiro_crew/dashboard/chat_runner.py.

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

[bolichen97]

• _append_redaction_notice docstring attributes the tag to a pass that cannot write it — fixed in 607351b (span=8023cfe9506b)

"StreamRedactor wire pass" cannot modify the independently accumulated persisted body, contradicting the data flow -> Fix: remove that attribution.

Legitimate, and the evidence is in this same file. The docstring listed three passes that could have put a redaction tag into the persisted body, one of them the StreamRedactor wire pass. But the comment above _wsred = StreamRedactor() states the opposite outright: "assistant_text (the source for the final _flush_segment redaction) is accumulated independently and is unaffected." Both wire redactors (_wsred on chat_chunk, _thinkred on chat_thinking) rewrite only what crosses WS/SSE, so neither can contribute a tag to the string this helper counts. Two comments in one file disagreed about the data flow, and mine was the wrong one.

Root cause of the wrong clause: the sentence is the dashboard analogue of the same enumeration in slack/handler.py, where the attribution IS accurate because that path really does accumulate the posted body through its StreamRedactor. Carried across to the dashboard path it names a mechanism that is not in the flow.

The fix drops the wire pass from the list and says why it is excluded, rather than silently shortening the enumeration — an unexplained omission would read as an oversight to the next reader and invite the same clause back. Comment-only: no behaviour change, no call-site change, and the counting contract (read the tag in the persisted artifact, not the redactor's warning list) is untouched. Re-verified after the edit: black gate, isort, flake8, mypy --platform linux (1281 files) clean, and 1039 targeted tests pass across test_chat_runner_coverage.py, test_flush_exception_safety.py, test_dashboard_chat.py.

[bolichen97]

• Three redact-then-persist slash-command sites still opt out of the notice — accepted-and-deferred to fix(chat): extend the credential-redaction notice to slash-command assistant persists #8435

Counted unfixed siblings: grepping chat_runner.py for redact_credentials/_redact_for_display feeding slot.append("assistant", …, "msg msg-a") without the helper leaves 3 sites — /workflow (4186), /goal (4285), /prompts list (5883). Each redacts a body that can echo user- or prompt-file content, so a tag written there is silent — the exact defect class the description names.

Verified rather than taken on trust, and the count is right. I ran my own census of every slot.append("assistant", …, "msg msg-a") in the file: 12 sites, 8 covered (_flush_segment at 3265 plus the seven exception paths), and the named three uncovered — /workflow redacts via redact_credentials + redact_exfiltration_urls, /goal via _redact_for_display, /prompts list via the same pair as /workflow. Each then persists to the assistant row with no notice. The defect class is the one this PR exists to close.

Deferred rather than fixed here, for three reasons. First, it is a different surface: these are synchronous slash-command outputs, not the LLM assistant_text turn that issue #8311 and this PR's title scope. This review's own reading concedes that — the description's "all 8 assistant-body persist sites" is accurate for assistant_text, and the gap is that the harvested rule is broader than the fix, not that the fix under-delivers on its stated scope. Second, it is not deferral-barred: redaction already runs correctly at all three sites, so no credential reaches the page and nothing is lost or corrupted. What is missing is the user-facing warning, which makes this a UX-completeness gap rather than a security finding. Third, folding three sites in would change this PR's stated purpose and title and re-arm all five review lanes on a broader diff, for a gap that predates the notice entirely.

The census also turned up something this finding does not cover and a mechanical sweep would have papered over. A fourth uncovered site, /prompts not-found at 5839, persists f"❌ Prompt \{name}` not found."with **no redaction pass at all** — a different shape, not a missing helper call. Whether a user's own echoed input needs a redaction pass before it lands inslot.messages` is a real question, and answering it inside a PR about exception paths would have been the wrong place. #8435 carries both shapes and separates them.

One thing #8435 must decide before any code, which is why it is a task and not a chore: the notice text says "Any command shown above will not work if you paste it as-is". That reads correctly under a salvaged assistant reply. Whether it reads correctly under a /prompts listing or a /goal confirmation is a wording judgment, so the follow-up either confirms the existing sentence is acceptable on command output or adds a shorter variant for it.

[bolichen97]

• The helper still relies on every future persist site remembering to call it, and fix(dashboard): warn when redaction rewrites a URL in chat text #8291 may land on the same seam — needs-a-decision

The helper still relies on every future persist site remembering to call it — the coupling moved up a level rather than being removed; the author's semgrep candidate is the acknowledgment. Acceptable as deferred, worth a human's eye on whether #8291 lands on the same seam.

Agreed on the diagnosis, and it is stated accurately: extracting the tail makes the contract nameable in one place, but it does not make forgetting it impossible. That is why the ## Pattern harvest section proposes the rule rather than claiming the coupling is gone — an slot.append("assistant", …, "msg msg-a") with no following helper call is now a grep-able shape, which it was not before this change. Landing that rule is the durable fix and it is tracked with the sites in #8435.

The half I am putting to a maintainer rather than answering myself is the seam question, because it is a sequencing call and not a code judgment. Open PR #8291 adds the exfiltration-URL notice to this same file. Two orderings are available and they are not equivalent:

Land this PR first and #8291 rebases onto _append_redaction_notice, extending it to a second tag family — the helper becomes the one place both notices are counted and appended, and the semgrep rule in #8435 then covers both. Or land #8291 first and this PR rebases onto whatever shape it introduces, in which case the helper may need to be a two-tag-family shape from the start rather than the credentials-only one it is now.

I have deliberately not picked. This PR is self-contained and composes either way — it adds a helper and eight call sites and touches no URL-notice code, and the current head is rebased onto 434ff7c70cc9 with no conflict against main. But which shape the shared seam should end up with is a maintainer's call about the two PRs together, and #8435's semgrep rule inherits the answer, so it is worth deciding rather than discovering at merge time.

Nothing here blocks readiness: this review's verdict is advisory CONCERNS, and both halves are answered — the coupling is acknowledged and tracked, and the seam question is on the record for whoever merges.

[chenmingwei23]

Approving: PR Readiness green (the repo's only required check), no failing lanes, MERGEABLE.