fix(terminal): stop scanning PTY output on the way to the browser

[buluoray]

Problem

The web terminal scanned every 4096-byte PTY read for credentials on its way to
the browser. That scan protected nothing, corrupted output, and hid secrets the
user had deliberately printed.

It protected nothing. The panel renders into the authenticated operator's own
browser, showing bytes their own shell just wrote a moment earlier. Any threat
that can read it can read the terminal application sitting beside it — and the
scrollback ring buffer is replayed only to that same browser. The one path by
which terminal output reaches a model is the selection hand-off, and
POST /api/terminal/redact already scans that.

It corrupted output. Scanning required decoding, and a PTY read ends wherever
the kernel had bytes, so a multi-byte character is routinely split across two
reads. Decoding each read independently turned both halves into U+FFFD,
permanently destroying CJK, emoji and a TUI's box-drawing glyphs — the client
cannot recover the original bytes from a replacement character.

It hid secrets printed on purpose. Observed false positives: the output of
gh auth token, an entire device-code login URL, a presigned S3 URL, and npm
integrity sha512-… lines.

Why it matters

A terminal that silently rewrites what your shell printed is not a terminal. The
failure is worst exactly where the user needs the output most — copying a token
they just minted, following a device-code URL, reading a TUI in a non-Latin
locale — and it is invisible: [REDACTED: …] looks deliberate, and a U+FFFD
looks like a font problem rather than data loss.

Fix

Forward each read byte-for-byte and delete the scan.

• The read loop sends data unchanged. Reassembly moves to xterm.js, which runs
  its own incremental decoder, so no read boundary can corrupt a character and
  the server holds no decoder state that could desynchronize from the client's.
• Reconnect replay is the same byte copy (bytes(sess.scrollback)). A truncated
  ring buffer can begin mid-character; the client renders that head as it finds
  it, the lead byte being genuinely gone.
• POST /api/terminal/redact is unchanged and remains unconditional. It is now
  documented as the whole credential boundary for the web terminal, which it
  already was in practice — and a contiguous selection is the only input the
  regex redactors can be accurate on, since a secret split across two reads is
  invisible to a per-chunk scan by construction.
• Separately: the read loop now captures the WebSocket into a local and
  revalidates it after taking the send lock. sess.ws is set to None by the WS
  handler on disconnect, and AttributeError is not caught by the loop's
  except OSError, so dereferencing it after a suspension point killed the
  reader task and stopped PTY draining and scrollback capture for a session the
  client could still reconnect to.

README.md's claim that Kiro Crew "redacts credential patterns from output
before it reaches a chat surface" stays accurate: that is the selection hand-off,
which is untouched.

Tests

Each test was verified to fail with the per-chunk scan restored (a temporary
patch reinstating main's _redact_terminal call in the read loop), then to
pass once removed.

• test_multibyte_output_survives_read_boundaries — drives the real PTY loop
  with an unbroken 9 KB run of 3-byte characters and asserts no U+FFFD arrives.
  Falsification: with the scan restored it fails on
  a read boundary corrupted a character. The payload is deliberately one line
  with no newline in it: an earlier version of this test used many short lines
  and passed against the corrupting code, because a shell that writes line by
  line hands the reader whole lines and every read then lands on a character
  boundary by accident. A single run of 3-byte characters longer than one
  4096-byte read cannot be split cleanly, since 4096 is not a multiple of 3.
• test_stream_and_replay_forward_output_verbatim — an AKIA key printed by the
  shell arrives intact, and so does the scrollback replayed on reconnect. The
  literal is split in the typed line ('AKIAIOSFODNN7''EXAMPLE') so the shell's
  echo of the command cannot satisfy the assertion; only the printf's real output
  joins the halves. Falsification: with the scan restored the needle never
  arrives and the assertion shows the redacted echo.
• test_redactors_are_called_only_by_the_selection_handler — source guard: each
  redactor has exactly one call site, inside api_terminal_redact's _scan.
  Falsification: with the scan restored, assert 2 == 1.
• test_no_send_through_the_session_field_after_an_await — source guard for the
  capture-and-revalidate fix; sess.ws.send must not appear in the module.
• test_session_reservation_has_no_await — source guard pinning the
  max-sessions-check → placeholder-assignment region await-free, so two
  concurrent opens for one session id cannot both spawn a PTY.

test_terminal_handler.py: 217 passed, 1 skipped. Full backend suite: 40679
passed; the 93 failures are pre-existing on this host (120 s timeouts in
test_worktree_create, test_cli_manifest_signature, the ops-mission-control
git suites, and sandbox-unavailable failures in test_terminal_commands's probe
tier and auto_improvement) — none of the 13 affected files import
dashboard.handlers.terminal. flake8, isort, mypy (866 files) and
scripts/docs-lint.sh are clean.

Note for reviewers who saw the earlier revision

This PR previously kept the scan on by default and added a
dashboard.terminal.redact_output opt-out. That was the wrong shape: the switch
was undiscoverable (there is no Settings → Terminal surface, and
kirocrew config set cannot even create a key that does not already exist), and
it required latching the mode per session because the two modes disagree about
who holds a partial character — complexity that existed only to preserve a scan
with no threat model behind it. Removing the scan removes the configuration, the
latch, and that entire class of bug.

[github-actions]

Design Review (Fable 5) — ✅ PASS

Advisory design-level review of a5c91a9695915af8a1f7ea2cfaefa786580b70f4 — updated in place on each push; does not block merge.

Design-Verdict: PASS

Removes a scan with no threat behind it at the root cause, and correctly relocates the credential boundary to the one path output reaches a model.

The corruption (per-read decode of split multi-byte characters) and false positives are inherent to per-chunk scanning — the description's own point that a straddled secret is "invisible to a per-chunk scan by construction" means the deleted layer was never a reliable backstop, so consolidating on the unconditional selection hand-off loses nothing real. The rejected earlier shape (opt-out config + per-session mode latch) was the right thing to kill; this version is simpler, reversible, and one-way-door-free. The bundled sess.ws capture-and-revalidate fix touches the same send site and is disclosed, so it belongs here. Docs and spec move in the same commit as required.

[DESIGN-REVIEWED] a5c91a9

[github-actions]

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] a5c91a9

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

[github-actions]

Opus 4.8 Review — ✅ no blocking findings

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

Review details

I've verified the diff, the base-branch backend-security-controls rule (blocking: true, matches src/kiro_crew/**/*.py), and the candidate's evidence directly.

Falsifying Candidate 1:

• The deleted helper and its former call sites match the diff (evidence grounded).
• (a) input: a credential/exfil URL written to the PTY — occurs in practice.
• (b) path: read_pty → live.send_bytes(data) and reconnect → ws.send_bytes(bytes(existing.scrollback)), both unscanned — holds.
• (c) observable wrong outcome: this is where it dies. The per-session WebSocket reaches only the one authenticated operator's own browser (not a broadcast, not a multi-party surface), rendering bytes that operator's own shell just wrote — bytes they already see in their real terminal. Under the repo's stated single-user, same-UID trust boundary there is no privilege crossing and thus no re-derivable wrong outcome. The rule is anchored to "Never trust LLM output"; terminal output is the operator's own shell, and the one path where that output actually crosses into a model (/api/terminal/redact) retains both redact_credentials() and redact_exfiltration_urls() — a compensating guard present in the same diff and pinned by a source test. The candidate's own confidence is "low" and concedes the removal is defensible under this trust model.

No observable wrong outcome survives; the model-facing boundary is still scanned. The candidate does not reach 80.

No findings.

[OPUS-REVIEWED] a5c91a9

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

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

[github-actions]

UX Review (Fable 5) — ⏭️ skipped

Revision a5c91a9695915af8a1f7ea2cfaefa786580b70f4 touches no user-facing surface (no changes under website/ or committed screenshots), so the UX review was skipped. Advisory — does not block merge.