fix(tui): a null message_id on the first hook fire must not disarm the F1 guard (#160)

Residual found by the independent reviewer's second pass, by PROBING the fixed code rather
than reading it — the F1 fix was correct but its ARMING could be skipped entirely.

TuiDeltaAssembler.messageId was initialized to `null`. A first MessageDisplay payload carrying
message_id:null therefore compared EQUAL to the sentinel, registered no boundary, and left
`messages` at 0. When the real boundary then arrived, `else if (this.messages > 1)` evaluated
1 > 1 === false, so restartedAfterEmit never armed, and the `released` branch forwarded the
auth banner to the client — the exact leak F1 closed, reachable again through a single null
field. parseDeltaChunk does not validate message_id, so such a payload does reach push().

Two changes, both narrowing:
  - messageId now initializes to a Symbol sentinel, which is === to nothing a JSON payload can
    produce, so the first fire ALWAYS registers as message 1 whatever its message_id is.
  - the boundary branch drops the `this.messages > 1` sub-condition. It bought nothing and was
    the sole cause. The real invariant is "a boundary occurred while emitted !== ''" — which is
    unrecoverable regardless of how many messages have been seen — and that is now what the
    code says.

Whether claude ever emits message_id:null is unverified (the observed contract has it present),
so this is defense-in-depth, not a live bug. But the guard is the mechanism this feature
nominates as its primary safety property; it should not be disarmable by a field's absence.

Mutation-tested: restore the null sentinel + the messages>1 condition and the new test fails;
with the fix, 317 passed / 0 failed (was 316).

Class B (ADR 0007, OCP-owned TUI spawn) — cli.js does NOT perform this operation.


Claude-Session: https://claude.ai/code/session_01VqgWJcjxrjjL9L9SkpZyXR

Co-authored-by: dtzp555 <dtzp555@gmail.com>
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
This commit is contained in:
dtzp555-max
2026-07-14 08:21:30 +10:00
committed by GitHub
co-authored by taodeng Claude Opus 4.8
parent 1b324968f4
commit a90f830b5d
2 changed files with 35 additions and 2 deletions
+15 -2
View File
@@ -173,6 +173,9 @@ export function parseDeltaChunk(text, consumed = 0) {
// fails the turn loudly (SSE error frame, no cache, counted on /health). Fail-loud is
// the correct posture — a proxy that silently serves text the transcript disagrees with
// is the exact class of bug ALIGNMENT.md exists to prevent.
// Sentinel for "no message seen yet". Deliberately not null/undefined — see the constructor.
const NO_MESSAGE_YET = Symbol("no-message-yet");
export class TuiDeltaAssembler {
constructor({ holdbackChars = DEFAULT_HOLDBACK_CHARS, detectError = detectTuiUpstreamError } = {}) {
this.holdbackChars = holdbackChars;
@@ -180,7 +183,12 @@ export class TuiDeltaAssembler {
this.emitted = ""; // bytes ALREADY written to the client — unretractable
this.pending = ""; // held back, not yet written
this.released = false;
this.messageId = null;
// NOT null: a payload may legitimately carry message_id === null, and if the sentinel were
// also null the FIRST such payload would compare equal to it, register no boundary, and
// leave `messages` at 0 — which used to disarm the restartedAfterEmit guard below entirely.
// A unique object is === to nothing a JSON payload can produce, so the first fire ALWAYS
// registers as message 1, whatever its message_id is (or isn't).
this.messageId = NO_MESSAGE_YET;
this.deltas = 0; // hook fires seen
this.messages = 0; // distinct message_ids seen
this.restartedAfterEmit = false;
@@ -198,7 +206,12 @@ export class TuiDeltaAssembler {
this.messages++;
if (this.emitted === "") {
this.pending = ""; // safe: the transcript will drop this message too
} else if (this.messages > 1) {
} else {
// A boundary while bytes are ALREADY out is unrecoverable, full stop — the count of
// messages seen so far is irrelevant. The old `else if (this.messages > 1)` guard was
// the sole reason a null-message_id first payload could disarm F1: it left `messages`
// at 0, so the real boundary evaluated 1 > 1 === false and never armed. The invariant
// is "a boundary occurred while emitted !== ''", and that is exactly what this says.
this.restartedAfterEmit = true; // unrecoverable — finalize() will refuse the turn
}
}