fix(dashboard): warn when redaction rewrites a credential in chat text

[chenmingwei23]

1. What is the problem?

Credential redaction rewrites a scheme://user:pass@host connection string to
[REDACTED: credential] in the assistant text the user copies out of the
dashboard chat, and tells the user nothing about it.

Reproduced byte-identical to the report, by calling the shipped redactor on the
reporter's exact string:

in:  echo 'DATABASE_URL=postgresql://user:pass@host:5432/db' >> .env
out: echo 'DATABASE_URL=[REDACTED: credential]host:5432/db' >> .env

The matching branch is one alternative inside _CREDENTIAL_PATTERNS
(security.py:11527-11537): (?:postgres(?:ql)?|mysql|mongodb(?:\+srv)?|...)://[^\s:/@]*:[^\s/]+@.
The password class [^\s/]+ is greedy and excludes / but allows @, so the
match ends at the final @ of the authority and stops before the host. That is
why host:5432/db survives and the result is a syntactically plausible but
broken URL rather than an obviously empty one.

Two things are wrong, and only one of them is the substitution:

• the pasted command does not work, and
• nothing says the text was altered.

The second is what turns a visible redaction into a debugging problem. Every
call site on this path discards the signal it already has. redact_credentials
returns (text, warnings), and the chat path spells it
safe_chunk, _ = redact_credentials(safe_chunk) (chat_runner.py:6603).
_flush_segment does bind the list, then routes it only to
logger.warning (chat_runner.py:3036-3038). Slack is identical
(slack/handler.py:3824-3828). No surface has ever told a chat user that their
message was rewritten.

2. Why this issue matters to the user

The user copies a command the assistant wrote, runs it, and it silently writes a
broken value. The failure then surfaces far from its cause: the reporter got
getaddrinfo EAI_AGAIN out of a container at boot, because the .env line was
74 chars instead of ~130 and had no @. Nothing in that error points back at
the chat message, so the time goes on debugging DNS and Prisma instead.

It reproduces 100% of the time whenever the assistant emits a full connection
string, and the assistant emits one whenever it is asked to write a
DATABASE_URL.

3. How our fix solves it

The redactor is not weakened, and that is a deliberate choice, not an
omission. The chat bubble IS the egress this control guards: the same text is
broadcast to WS/SSE clients (chat_runner.py:6631) and persisted into the
transcript (chat_runner.py:3044). The issue suggests keeping the value
copyable by redacting only at the display layer; on this path that means
transmitting the real credential to the browser and storing it in the session
transcript. Today the raw string exists only as the in-flight chunk and is never
persisted. Making the command paste-able means undoing that. That trades a
usability bug for a credential leak, so this PR does not do it.

So this PR fixes the half that can be fixed without that trade: the silence. A
segment whose persisted text carries redaction placeholders now appends a
notice row stating that a credential was replaced and, explicitly, that the
command will not run as pasted:

Security notice: A credential in this message was replaced with a redaction
placeholder before it reached this page. Any command shown above will not work
if you paste it as-is; supply the secret yourself on the machine where you run
it.

That second clause is the point. A notice that only said "a credential was
removed" would still leave the user pasting text that cannot run.

Two implementation notes, both load-bearing:

The count is read from the tag in the persisted text, not from
cred_warnings. On the streaming path that list is essentially always empty
at the flush: the run loop redacts every chunk before it enters assistant_text
(chat_runner.py:6602-6605), so the flush-time call re-redacts already-clean
text and reports nothing. cred_warnings only fires for a credential split
across chunk boundaries. Counting the tag also makes the notice correct
regardless of WHICH of the three sites did the substitution - per-chunk, the
StreamRedactor wire pass, or the flush - because all three write the same
REDACTED_CREDENTIAL_TAG. Hooking the event would have required hooking all
three; reading the artifact hooks none, and answers the question the user
actually has: is what I am about to copy still what the assistant wrote?

The notice broadcasts even under quiet_persist. That flag exists to
suppress a duplicate of pre-steer assistant text clients already rendered; a
notice row has no streamed counterpart to duplicate, and suppressing it would
drop the warning on one of the paths this fix exists to cover.

The count sums every tag the redactor can emit, and security.py owns the
list. This was a real gap in the first revision, found by the Opus lane. Pass 2
replaces a base64-encoded credential with [REDACTED: encoded credential], which
is NOT a substring of the plaintext tag, so counting one tag left an
encoded-credential-only segment silently rewritten - the same #6189 failure
reached through a different pass - and undercounted a mixed segment.

The first fix for that enumerated both tags at the call site, which the First
Principles lane correctly called out as the very mechanism that produced the
undercount: a third tag added in security.py would escape the notice again. So
the list is now CREDENTIAL_REDACTION_TAGS, owned by security.py beside the
passes that write the tags, and chat_runner asks for it instead of naming tags
itself - a net subtraction, since the two-constant import and the local tuple both
go away. test_every_redaction_tag_constant_is_registered fails if a
_REDACTED_*_TAG constant is not registered in it, so the drift cannot recur
silently rather than merely being relocated. A second test pins the non-overlap
invariant that makes summing per-tag counts safe.

The encoded tag stays PRIVATE. An earlier revision gave it a public alias mirroring
REDACTED_CREDENTIAL_TAG, but once callers ask the registry that alias had zero
non-test consumers, so it was unjustified API surface and the First Principles lane
was right to ask for it back. Tests import the private constant, which
test_credential_prefilter.py already does for _REDACTED_CREDENTIAL_TAG.

No frontend change is needed: the notice role already renders as a
NoticeCard (website/src/pages/ChatPage.tsx:6856).

4. What tests we did

Three new tests in TestFlushSegment, plus two added after the Opus finding, plus
one existing test corrected.

New:

• test_a_redacted_connection_string_warns_the_user - the reporter's exact
  command; asserts the assistant row still has the credential removed (redaction
  intact), that a notice follows it, that the notice says the command will not
  work as pasted, and that the notice itself carries no secret.
• test_the_notice_counts_multiple_credentials - two connection strings report
  "2 credentials".
• test_a_clean_segment_gets_no_notice - no placeholders, no notice row.
• test_an_encoded_credential_also_warns - the issue's own connection string,
  base64-encoded so only pass 2 fires; asserts the encoded tag is present, the
  plaintext tag is not, and a notice is still raised.
• test_the_two_credential_tags_do_not_overlap - pins the invariant that makes
  summing per-tag counts safe.
• test_every_redaction_tag_constant_is_registered (in test_security.py) - the
  drift ratchet: a _REDACTED_*_TAG constant that is not in
  CREDENTIAL_REDACTION_TAGS fails here, naming the offender.
• test_pass_two_emits_a_registered_tag (in test_security.py) - the encoded pass
  substitutes a tag consumers actually look for.

Corrected: test_credentials_in_the_segment_are_redacted asserted on
slot.messages[-1], which is now the notice row. Left alone it would have kept
passing while no longer checking the assistant text at all - a security test
passing vacuously. It now selects the assistant row by role.

Mutation-verified against the revision this body is published with, each mutation
applied to the committed tree and reverted with git checkout (never stash).
Keyed by test NAME, not position, and every red reported with the error TYPE it
arrived as - because a red that arrives as an EXCEPTION proves nothing about the
predicate: it shows the machinery stopped running, not that the value changed. Each
mutation below changes what the count ANSWERS while leaving every name bound.

1. count forced to 0 (notice never appends) -> 3 red, all assertions:

   • test_a_redacted_connection_string_warns_the_user - AssertionError: assert ['assistant'] == ['assistant', 'notice']
   • test_the_notice_counts_multiple_credentials - AssertionError: expected exactly one notice row, got []
   • test_an_encoded_credential_also_warns - AssertionError: unexpected rows: ['assistant']

   The ['assistant'] in those messages is load-bearing: it witnesses that
   _flush_segment ran to completion and only the notice was absent.

2. zero-guard dropped (if True:) -> 4 red. Three are assertions on exact role
   lists (test_a_clean_segment_gets_no_notice,
   test_trailing_stop_event_is_replaced_below_the_segment,
   test_unparseable_cls_is_not_a_stop_event) - an extra row genuinely changes
   them. The fourth, test_pending_variants_are_attached_and_broadcast, fails with
   KeyError: 'variant_idx' because it reads slot.messages[-1], which the notice
   displaces. That red is EXCLUDED from the evidence: it arrived as an exception, so
   it says nothing about the predicate. It is also not a production defect -
   _flush_segment binds last_msg before appending the notice, and the load-path
   analogue _attach_variants runs immediately after its own append, so its
   messages[-1] is always its own row.

3. plurality hardcoded singular -> 1 red, assertion:
   test_the_notice_counts_multiple_credentials - AssertionError: assert '2 credentials' in 'Security notice: A credential in...'

4. tag set narrowed to CREDENTIAL_REDACTION_TAGS[:1] -> 1 red, assertion:
   test_an_encoded_credential_also_warns - AssertionError: unexpected rows: ['assistant']

5. an unregistered _REDACTED_MUTANT5_TAG added to security.py -> 1 red,
   assertion: test_every_redaction_tag_constant_is_registered names the offender.

No test in this file locates a row with a bare next(...), deliberately: next
over a generator raises StopIteration when the row is missing, and that raise
cannot distinguish "no notice was appended" from "the flush threw before appending
one". Every lookup is a list plus an assertion that prints what WAS there, so no red
here can arrive as an exception.

Harness provenance, so a reviewer can tell which tree these numbers came from
without re-running anything. pytest resolves rootdir to this worktree and reads
its setup.cfg, whose [tool:pytest] sets pythonpath = src; that PREPENDS to
sys.path, so every number above was measured against
kirocrew-fix-6189/src/kiro_crew, not against the main checkout. The worktree's
own venv agrees independently - its editable install resolves kiro_crew to the
same path.

This matters here specifically because main already performs the redaction these
tests assert, so a run against the wrong module would show the baseline rows
passing and only the new rows failing - indistinguishable from a genuinely weak
test. Two things rule that out: the provenance above, and the fact that
_redaction_notice and CREDENTIAL_REDACTION_TAGS do not exist on main at all,
so the notice tests could not have passed against it under any conditions.

Suites run (each file its own invocation, -p no:randomly -n0):
test_chat_runner_coverage.py 272 passed; test_security.py 1265 passed, 1
skipped; test_dashboard_chat.py 735 passed; test_post_compaction_continuation.py
55 passed (this one asserts on the _flush_segment call-site source text, and
stays green because this change adds no call-site argument);
test_slot_chunk_retention.py 16, test_chat_steer.py 19,
test_inline_tool_cards_props.py 7. isort, flake8 and mypy clean on all four
changed files, and the project's own scripts/check_black_formatting.py gate
passes - security.py is in the black baseline on main, so it is deliberately NOT
reformatted here; the diff to it is additive.

5. Any other suggestions on the work

This warns; it does not make the command paste-able. That residue is
deliberate, so the trailer is Refs, not Closes. Of the four fixes the issue
proposes, only "warn explicitly" is implementable without changing the security
posture. The other three - skip redaction inside execution-intended fences,
display-layer-only redaction with the clipboard intact, and distinguishing an
assistant-GENERATED value from an echoed user secret - all require either
shipping the credential to the client or adding provenance the redactor does not
have. Those remain a maintainer call, which is what the earlier needs-human
routing was right about; this PR is the part that did not need that decision.

The reporter also asked for the copy action itself to be blocked. Not done here:
that is frontend behaviour on a control this diff does not touch, and the notice
is the backend half it would need either way.

Two adjacent gaps found while tracing this, deliberately NOT fixed to keep the
diff narrow:

• The non-dashboard channels are explicitly DEFERRED, not fixed. They keep the
  exact silence this PR ends for the dashboard, and they are filed as
  #8123: Slack final text
  (slack/handler.py:3826), Slack thinking (slack/handler.py:3961), iMessage
  (imessage/renderer.py:56), and the shared messaging scrubbers
  (messaging/renderer.py:230, messaging/driver.py:266). The First Principles
  lane asked for this deferral to have an owner, and it was right to - the sharpest
  case is slack/handler.py:3935, where the branch
  if _stream_had_redaction or _render_redacted or exfil_warnings or cred_warnings:
  already reads the signal and rewrites the posted message because of it, and still
  tells the user nothing. Each surface needs its own delivery (the notice row is a
  dashboard mechanism), which is what makes the general fix larger than this diff
  rather than a line of it.
• Exfiltration-URL redaction has the identical silence on this same path
  (exfil_warnings logged only, chat_runner.py:3034-3035). It rewrites text the
  user copies just as credential redaction does. Not filed separately: it is the
  same structural point as Credential redaction silently rewrites pasteable text on Slack and iMessage #8123 and worth deciding once, rather than opening a
  second issue that the same change would close.

Checked against the sibling issue #8042 / PR #8055:
no overlap. That PR lives entirely in dashboard/handlers/files.py plus
test/test_project_tree.py with zero lines in security.py, and it concerns
pass 3 (the bare-secret run amplification) whereas this is pass 1
(_CREDENTIAL_PATTERNS). This PR's only security.py change is additive and
nowhere near that: a new tag constant beside the existing REDACTED_CREDENTIAL_TAG
alias, and pass 2's inline literal swapped for it (byte-identical value). No
regex, no matching behaviour, and no line either issue's mechanism depends on is
touched.

Pattern harvest

Rule candidate: semgrep (or a targeted lint)
Pattern: a redactor's warnings channel discarded at a user-facing surface

redact_credentials returns (text, warnings). That warnings list is the
subsystem's only way to say it acted, and on this path every caller threw it
away: safe_chunk, _ = redact_credentials(safe_chunk) at the chunk site
(chat_runner.py:6603), and a logger.warning-only consumer at the flush
(chat_runner.py:3036-3038). The effect is a security control that silently
rewrites text the user is about to copy.

Not a one-off: the same discard shape appears at roughly 550 call sites in the
tree, and two are the adjacent gaps named above - exfiltration-URL redaction on
this very path, and the Slack final-text path. The bug is structural, not local.

The generalizable rule: in a module that renders to a human surface, binding a
redactor's warnings element to _, or consuming it only through a logger, should
be flagged, because it turns a visible mutation into a silent one.

The honest caveat is scope, and it is why this PR does not ship that lint. Most
of those ~550 sites redact log lines, tool titles and internal metadata, where a
user-facing notice would be pure noise. A useful rule has to key on "this string
is rendered to a person", which the call site alone does not say. So this PR
fixes the one path where that is provably true and leaves the rule as a
candidate rather than asserting a tree-wide invariant it cannot yet enforce.

Refs #6189

[github-actions]

Design Review (Fable 5) — ✅ PASS

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

Design-Verdict: PASS

Real harm, right half fixed: warns instead of weakening a persisted-egress redaction, with tag ownership moved to the emitter and channel gaps explicitly filed (#8123).

[DESIGN-REVIEWED] ae91e2e

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No findings.

The sole candidate requires the assistant's own output to contain the literal string [REDACTED: credential] without any credential actually having been redacted this pass. That input is speculative — it depends on the LLM emitting the exact marker verbatim, which is a "could happen," not a concrete input that occurs in practice — and its worst outcome is a harmless advisory row, not a crash, data loss, or security hole. The candidate's own confidence is low, and (a) does not clear the 80+ bar. Dropped.

[OPUS-REVIEWED] ae91e2e

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

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

[github-actions]

First Principles Review (Fable 5) — 🟡 CONCERNS

Premise-level review of ae91e2e993c6454bfca9c0dc9369051cfe5354f5 — 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.

I have what I need. Verified: the notice mechanism reuses the existing slot.append("notice", ...) channel (15+ existing call sites, so no frontend change needed); CREDENTIAL_REDACTION_TAGS has exactly one real consumer (chat_runner.py:3121); and the same silent-rewrite root cause has unfixed siblings on other human-facing chat surfaces (Slack final text and thinking, iMessage, generic channel), of which the diff declares only the exfiltration-URL one and the description names only Slack.

First-Principles-Verdict: CONCERNS

The dashboard notice earns its place against issue #6189, but it is one surface out of at least four that silently rewrite chat text.

What this change ships

Intent: stop a user from unknowingly copying a chat command whose credential was silently replaced — a FIX (of the silence, deliberately not the substitution).

1. Dashboard chat shows a "Security notice" row after a message whose credential was redacted — justified (reported defect Credential redaction corrupts commands meant to be pasted into a terminal (silent, no warning) #6189)
2. The notice says the command above will not run as pasted — justified; this is the harm the reporter hit
3. The notice appears even on the quiet post-steer re-persist path — declared, justified (no streamed duplicate exists)
4. New public registry CREDENTIAL_REDACTION_TAGS in security.py — one consumer (chat_runner.py:3121), derived from a counted undercount defect, ratchet-tested
5. The encoded-credential tag becomes a named private constant — rides along, required by item 4
6. Existing redaction test retargeted from messages[-1] to the assistant row — rides along, necessitated by item 1
7. Slack, iMessage, and generic channel surfaces still rewrite silently — point patch, siblings counted below

Watch

• Point patch with counted unfixed siblings. Grepped redact_credentials across src/: the same log-only silent rewrite of human-facing assistant text remains at slack/handler.py:3826 and :3961, imessage/renderer.py:56, channel.py:833, and crew_chat.py:980 — plus redact_exfiltration_urls in the very function patched (chat_runner.py:3063). The diff comment declares only the exfil sibling "tracked separately"; the description names Slack in the problem statement but the change enumerates neither iMessage nor the generic channel as left behind. Accepted-and-deferred is fine — but the deferral should name all the surfaces, not two of five.
• CREDENTIAL_REDACTION_TAGS is permanent public surface with exactly one consumer today; acceptable only because its cause (the call-site enumeration undercount) is real and the ratchet test pins it — do not grow it beyond credential tags.

[FIRST-PRINCIPLES-REVIEWED] ae91e2e

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] ae91e2e

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