Skip to content

fix(dashboard): stop reporting a written steer as one the turn consumed - #7997

Merged
iamwhatever merged 1 commit into
mainfrom
fix/steer-midturn-consume-7246
Sep 4, 2026
Merged

fix(dashboard): stop reporting a written steer as one the turn consumed#7997
iamwhatever merged 1 commit into
mainfrom
fix/steer-midturn-consume-7246

Conversation

@chenmingwei23

@chenmingwei23 chenmingwei23 commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Problem / Motivation

A mid-turn Steer is reported in the dashboard as "Steered into the running turn"
while the streaming generation continues unchanged. When the turn ends, the same
text is requeued and answered as a separate turn. The user was told their
correction had been applied to work that in fact kept going.

Why it matters

Steering is how an operator course-corrects a long turn. The failure is silent
and the wrong way round: the operator believes the redirect landed, stops
watching, and only later sees the original answer complete plus their correction
re-asked as its own turn. Anyone who steers regularly hits it, and the more
useful the steer, the more the false confirmation costs.

What changed (motivation -> approach -> change)

steer_into_running_turn persisted the transcript row and broadcast steer_push
as soon as client.steer() returned. That return proves only that the bytes
reached the backend process; the backend's own proof of injection is a
steering_consumed echo, which _settle_consumed_steers already parses. A steer
is injected at a model-inference boundary, and a turn streaming text without
dispatching a tool can end before reaching one -- so no echo arrives,
_requeue_unconsumed_steers moves the message to the queue, and the row keeps
asserting an injection that never happened.

The backend already distinguishes these states on the wire and Crew already
parses both discriminants (steering_queued / steering_consumed, plus the KAS
steering_injected spelling). What was missing was recording WHICH state a row
is in, so the change records it as meta.steerState:

  • written at persist time -- the bytes were accepted, nothing more;
  • consumed when the echo confirms the running turn took them;
  • requeued when the turn ended without one and the teardown queued the message.

Both transitions go through one writer (_mark_steer_row_state) and reuse the
existing ts-keyed chat_message_update patch, so a live client and a page reload
agree without a new WS event. The row is resolved by its sanitized content plus a
still-written state, because the successful-steer path is terminal for the
delivery id and pops it; the one-in-flight-steer-per-text guard is what makes
that pair unambiguous, and the scan runs newest-first so a hard kill's stale
written row cannot capture a later identical steer.

Front end: the badge asserts the message reached the running turn, so it now
renders only for consumed. written and requeued render as an ordinary user
message. A row with no steerState predates this and keeps the original
rendering, so persisted history renders unchanged -- with one deliberate
exception the First Principles lane was right to make me state outright: the
client's own optimistic bubble, minted as { steer: true, optimistic: true }
with no state before the server has answered at all, is excluded from that
legacy case. It is the least confirmed a steer can be, so letting it inherit
the legacy rendering would show the success badge at the exact moment nothing
is known -- the claim this change exists to stop.

This does NOT make a steer redirect a boundary-free streaming turn -- that half
is the backend's and is filed separately (see Related Issues). This PR stops the
product claiming it did.

Round 8: a KAS-confirmed steer was requeued and re-run (GPT blocking finding)

The review lane blocked 4357b4c35 on chat_runner.py's requeue marking, and the
finding is correct, so this states the mechanism and the scope precisely.

steer_settle.settle_consumed_steers parses the echo for
<user_message>-wrapped blocks. That is kiro-cli's shape. KAS does not wrap:
its steering_injected frame is routed in session_handle and yields the
content field verbatim, so a KAS-backed turn produces a bare echo with no
envelope to find. settle_all_on_empty=True does not cover it either, because
that flag guards only the not snapshot.strip() branch and a bare echo is
non-empty. Nothing settled, the entry stayed pending, and the turn-end requeue
then re-ran a question the backend had already injected while this PR's new
requeued row state asserted it never applied.

Fix: when no wrapped block is found, treat the whole snapshot as one bare block.
Settling stays EQUALITY-based, so this recognises a second echo SHAPE without
relaxing the rule that guards against silent loss -- prose that merely mentions a
steer still matches nothing, which
test_an_echo_without_recognisable_blocks_keeps_entries_pending pins and which
still passes unchanged. The bare block is stripped for the same parity reason the
pending side is: a wrapped block is already stripped by its producer (the RPC
wraps message.strip()), a bare content field is not.

Scope, stated rather than discovered in review: the settlement gap is
PRE-EXISTING on main -- steer_settle.py is byte-identical between main and
this branch, and main's _requeue_unconsumed_steers (same name here; an earlier draft of this
description claimed a rename, which was wrong) already re-ran bare-echo steers. What this PR added was
the requeued row state, which turns that silent duplicate into a false claim on
the transcript. So the fix repairs main and widens this diff by one file. It is
taken here rather than split out because this PR's own change is what makes the
pre-existing gap user-visible, and because leaving it would ship a row state that
is wrong on every KAS turn.

settle_all_on_empty is deliberately NOT touched: chat_runner.py documents that
the /side sidecar chose the opposite policy and that aligning the main chat is a
separate change. This fix is orthogonal to that choice, and because the function
is shared it closes the same blindness for the /side caller
(side_state.py) at no extra cost.

Round 9: an empty echo became a confirmed steer (second GPT blocking finding)

The next review round blocked on chat_delivery.py, and it is the same defect class
as #7246 reached by a different route, so it is in scope by construction rather than
by argument.

The row's INITIAL state was STEER_STATE_WRITTEN if still_registered else STEER_STATE_CONSUMED, resting on a stated premise: "the only consumer left is the
running turn CONSUMING it -- every other remover returned above". That sentence is
not a trade someone chose, it is untrue.
_settle_consumed_steers passes
settle_all_on_empty=True, so it sweeps the pending list with no evidence at all and
returns nothing above. So this change does not reverse a deliberate decision; it
corrects one made on a premise its author did not know was false. _settle_consumed_steers passes settle_all_on_empty=True, so an
EMPTY frame clears the pending list without matching anything; if it lands while
client.steer() is still suspended, the entry is gone, still_registered is False,
and the row persisted consumed -- a success badge for a frame that proved nothing,
and terminal, so nothing ever corrected it.

Note the settle path was already right: it promotes only under snapshot.strip().
The gap was the initial persist inferring consumption from ABSENCE, where the two
removals are indistinguishable after the fact.

Fix: the settle path records the delivery ids a non-empty echo actually accounted
for (slot._steer_confirmed), and chat_delivery writes consumed only for a
delivery id in that set, falling back to written -- which is what is actually
known. Keyed on the delivery id rather than the text so a later identical steer
cannot inherit an earlier one's evidence. settle_all_on_empty is still untouched.

Note why blind written was NOT the fix, since it is the obvious cheaper path: the
comment at that site records that writing written unconditionally "would
permanently understate a CONFIRMED injection, since nothing runs the promotion
twice". That trades a permanent false consumed for a permanent false written --
still gating on absence of information rather than on evidence, which is the thing
this PR exists to stop. written is the ELSE-BRANCH of the gate, not a substitute
for it, which is why the gate needs to know whether the echo matched.

The marker FAILS CLOSED, and that is enforced rather than asserted. Absent, None,
empty, or not a set all take the written branch: "no marker" and "no evidence" are
the same branch by construction. The isinstance test is load-bearing rather than
defensive, because in raises TypeError on a non-container and .discard raises
AttributeError on a non-set -- so an unreadable marker would otherwise crash the
steer path instead of degrading to the honest state. New state whose ABSENCE yielded
the confirming value would reintroduce this defect through another door, invisibly,
because the row is terminal.

One existing test had to be corrected rather than relaxed, and it is worth naming:
test_a_steer_consumed_during_the_rpc_persists_as_consumed simulated the mid-RPC
echo with a bare slot._pending_steers.clear(). That reproduces the EVIDENCE-FREE
empty sweep, not a matched echo -- an injection narrower than the fault the test
names, which is why it passed while the defect was live. It now drives the real
_settle_consumed_steers with a real wrapped echo, so it pins the case it claims to.

Tests

test/test_steer_requeue.py::TestSteerLifecycleState, four cases:

  • a written steer's row and its steer_push both report written, not consumed;

  • a steering_consumed echo promotes the row and emits the ts-keyed patch;

  • the reported case: acked, never consumed, requeued -- the row must stop reading
    as an injection, and the message must still reach the queue;

  • two identical pending steers against a single echo block promote exactly one
    row, so a duplicate is not settled by its twin.

  • a steer whose steering_consumed echo lands while steer() is still
    suspended: the settle removes the pending entry before any row exists, so
    the row that follows must persist as consumed rather than understating a
    confirmed injection (found by the GPT lane on the first head).

  • an ambiguous row match patches NOTHING: two steers differing only in
    credential material sanitize to the same content, so which row is which is
    unknowable and both keep written rather than one being mislabelled.

The assertions use the literal wire values rather than importing the new state
constants. Importing them would make every case fail on an unfixed tree with an
ImportError, which proves only that the names are new; with literals the red is
the behaviour. Verified red on 556f08f939 before the fix -- the written and
requeued cases fail as assert None == 'written' and assert None == 'requeued'. Mutation-verified: neutralizing the requeue correction turns the
requeued case into assert 'written' == 'requeued'.

website/src/test/UserMessage.test.tsx, three cases: no badge for written, no
badge for requeued, badge for consumed. Mutation-verified -- reverting the
gate reddens exactly the two suppression cases.

Also run green: test_steer_settle.py, test_chat_steer.py,
test_chat_runner_coverage.py, test_kas_display_mapping.py,
test_session_control.py (460 passed -- the sweep for the
_settle_consumed_steers signature, which gained an optional trailing state
so existing callers are unaffected), plus UseWebSocketCoverage.test.tsx and
ChatPage.steerKeyIdentity.test.tsx (101 passed). black, isort, flake8, mypy,
tsc -b, and eslint clean on the touched files (eslint reports two pre-existing
warnings in useWebSocket.ts at lines 869 and 1950, neither from this diff).

Manual verification

Not yet performed end to end: reproducing the state this fixes needs a backend
turn that streams long enough to be steered without dispatching a tool, which
the unit tests model directly by driving the settle and teardown paths. The
visual delta is the badge's presence, which the three front-end cases pin.

Screenshots / video

The visual delta is the badge. It asserts the message reached the running turn,
so it now renders only for consumed; written and requeued render as an
ordinary user message, and a row with no steerState keeps the old rendering.

Steer rows in the written, consumed, requeued and legacy states

Captured from the real UserMessage component in a real browser (the project's
own Playwright against a Vite dev server), with the four metas supplied directly
rather than produced by a live backend turn -- reproducing the requeued state
end to end needs a generation long enough to steer that never dispatches a tool,
which the backend regressions drive directly instead.

Related Issues

Refs #7246

The consumption half of #7246 stays open: whether a supported backend can honor
_session/steer during a boundary-free generation is a backend contract
question, not something Crew can close, so #7246 is not closed here.

Pattern harvest

Rule candidate: review-prompt
Pattern: a write acknowledgement reported as a completion. The RPC returning
means the peer received the request; only the peer's own completion signal means
it acted on it. Where a protocol emits both (steering_queued vs
steering_consumed), consuming only the second and rendering success off the
first is the defect -- and a signal the product parses but no production code
reads is the smell that it is happening.

Rule candidate: review-prompt
Pattern: a fix for "asserts more than it knows" re-committing the same error one
layer down. Three of this PR's rounds were exactly that, in different cells of the
same lifecycle: a steer consumed during the RPC was persisted as merely written
(understating a confirmed injection); the state patch was keyed on an id the
steer_push that created the row never sent (so the update silently matched
nothing); and the promotion loop wrote consumed for an EMPTY echo, which
steer_settle's own docstring already called no evidence -- putting row,
transcript and persisted history back to asserting an injection nothing confirmed.
The lesson is not "be careful": when a change adds a state, every transition into
and out of it needs enumerating BEFORE the code, or each fix discovers the next
cell by review.

Rule candidate: agents-md
Pattern: an ASCII/secret gate that diffs against a MOVING ref reports other
people's lines as yours. Checking git diff origin/main HEAD produced 37 phantom
non-ASCII hits here once origin/main advanced past the commit's base -- the
em-dashes belonged to unrelated files main had changed. The only correct check is
the commit's own diff (git show HEAD -- <paths>), restricted to ADDED lines,
excluding committed binaries.

Round 8 (bare KAS echo), test/test_steer_settle.py -- 5 new cases, 16 passing:
a bare echo settles its steer; settles by equality not containment (a second
enforcement site for that rule, so it gets its own case); stays count-aware;
settles a redacted steer; tolerates surrounding whitespace.

Mutation-verified at two enforcement sites, each reddening a DIFFERENT set, which
is what rules out a harness artefact: neutering the bare fallback
(blocks = []) reddens 4 of the 5 on AssertionError; removing the .strip()
reddens exactly 1. The containment case correctly does NOT redden under either --
it asserts nothing settles, which holds with or without the fallback -- so its
guard is the shared equality site, not this one.

Suites run individually at -n0: test_steer_settle.py 16, test_steer_requeue.py
37, test_side_steer_queue.py 34, test_kas_display_mapping.py 32,
test_chat_runner_coverage.py 272 -- all passing. flake8 clean on both changed
files. The 2 mypy errors surfaced are in src/kiro_crew/transcribe.py, untouched
here, and reproduce identically on an unmodified checkout.

Round 9: test_an_empty_echo_during_the_rpc_persists_as_written (new) and the
corrected ..._persists_as_consumed; plus a sidecar pin,
test_a_bare_kas_echo_lets_the_sidecar_render_its_steer, because the sidecar's
RENDER is a distinct observable -- steer_settle marks rather than removes and its
caller renders exactly what it returns, so before the fix a bare frame made the
steer silently absent from the transcript, the INVERSE of the main chat's replay on
identical input.

Five mutations across five enforcement sites, each reddening a DIFFERENT set, which
is what rules out a harness artefact: bare fallback -> 4; .strip() -> 1; the
chat_delivery evidence gate -> 1; the chat_runner evidence recording -> 1; the
shared fallback against the sidecar pin -> 1. Suites at -n0:
test_steer_settle.py 16, test_steer_requeue.py 38, test_side_steer_queue.py 35,
test_chat_runner_coverage.py 267 -- all passing. flake8 clean on all seven
changed files.

Also test_an_unreadable_evidence_marker_fails_closed_to_written, which drives real
evidence and then makes the marker unreadable, so it pins the FALLBACK rather than
merely the absence of a match. Six mutations now, each reddening a different set:
bare fallback -> 4; .strip() -> 1; the chat_delivery evidence gate -> 1; the
marker's own WRITE -> 1 (assert 'written' == 'consumed', i.e. the fallback is
written specifically); the fail-closed isinstance guard -> 1 (a TypeError, not
an assertion -- the correct observable, since that guard exists to stop a crash); the
shared fallback against the sidecar pin -> 1.

Round 3: a fully-settled redaction collision left both rows understated

GPT's second blocking finding, and it is real. find_written_steer_row refuses to
patch when two LIVE steers share the sanitized content, and _settle_consumed_steers
called it before assigning _pending_steers[:] = remaining -- so the whole echo's
entries still counted as live. Two steers differing only in credential material are
admitted as distinct (the in-flight guard keys on RAW text) but persist byte-identical
rows, so an echo confirming BOTH left both rows reading written for the slot's life.

That understates a state the backend positively reported, which is the mirror of the
defect this PR exists to fix, and the two layers already disagreed: steer_settle
settles a fully-echoed collision on the stated ground that "ambiguity is about
attribution, not about redaction" and there is nothing left to attribute when the echo
accounts for the whole group. The row resolver never got that refinement.

The fix passes remaining + [_msg] as the live-steer list: the steers that can still
CLAIM the row are the ones still pending, plus the one being settled. Consequences:

  • Fully-echoed collision: no member remains, so the count is 1 and each row transitions.
    Both end consumed, and because they take the same state, which row is which is not
    observable -- so this needs no real steer identity, which stays Session control: cross-session message delivery needs resolved-binding authorization #4333's job.
  • Partially-echoed collision: a member is still pending, the count is 2, and the refusal
    stands. test_two_steers_with_identical_sanitized_content_are_left_alone pins that case
    and passes unchanged -- the two scenarios are disjoint, so nothing was relaxed.
  • Identical duplicate steer still pending: also 2, also refused, consistent with the
    multiset difference the evidence recording already uses one line above.

Two files, and they are easy to conflate: the refusal lives in find_written_steer_row
at chat_delivery.py:198, and the call site changed is in _settle_consumed_steers
at chat_runner.py:4146, one line above _pending_steers[:] = remaining.

The discrimination fails toward the PARTIAL case: "cannot prove the group fully settled"
and "partial" are the same branch, because erring permissive would confirm a steer whose
attribution is unknown -- the defect this change exists to prevent. Measured, not assumed:
settle_consumed_steers is all-or-nothing for a collision of DISTINCT raw texts (echoed
once, both stay pending; echoed twice, both settle), so the only shape that partially
settles is a group of IDENTICAL raw texts, and that is the state the second test builds.

Two mutations, because "the narrowing works" and "it fires on the right groups" are
different observables, and they fail in OPPOSITE directions:

  • Drop the siblings argument: the fully-settled test reds with
    assert ['written', 'written'] == ['consumed', 'consumed'] -- understating. Its
    _pending_steers == [] assertion still passes, proving the settlement layer did settle
    both and the failure is the row patch specifically, not a fixture that missed the case.
  • Report every group as fully settled (pass [_msg] alone): the partial test reds with
    assert 'consumed' == 'written' -- overstating, which is the Steer is acknowledged but not consumed mid-turn and falls back to a queued turn #7246 direction.

Where the risk now lives, stated plainly because it moved: there is no new "is this group
fully settled" boolean to get wrong. remaining + [_msg] DERIVES the answer -- fully
echoed gives a count of 1 by construction, partially echoed gives 2 -- so the fail-closed
property is structural rather than enforced. The consequence is that correctness now rests
entirely on remaining being right, which means it rests on steer_settle, the layer that
already had the ambiguity guard and its own tests. If remaining ever under-reports what
stayed pending, a row is confirmed wrongly; that is the single assumption this fix adds.

@chenmingwei23

Copy link
Copy Markdown
Contributor Author

The half this PR deliberately does not attempt -- a backend honoring _session/steer during a boundary-free streaming generation -- is filed as #7998, with the emitter and test references for the unconsumed EVENT_STEER_QUEUED signal. Noting it here rather than editing the body, so the review lanes are not re-rolled.

@github-actions github-actions Bot added the readiness: checking Automated validation is still running label Sep 2, 2026
@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Design Review (Fable 5) — 🟡 CONCERNS

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

Design-Verdict: CONCERNS

Sound evidence-gated fix that fails toward understatement, but row identity by sanitized-content matching is a workaround with a known permanent-wrong residual.

Watch

  • The lifecycle transition resolves its row by sanitized content plus three heuristics (find_written_steer_row: registered-entry exclusion, live-sibling count, newest-first), and the PR's own docstring concedes the residual: a partially-echoed redaction collision leaves confirmed rows written "for the slot's life", deferred to Session control: cross-session message delivery needs resolved-binding authorization #4333. The persist path already mints and broadcasts the row's mid — a delivery_id → mid map popped at transition would make resolution exact and delete all three heuristics plus the collision refusal. Safe as shipped (errs toward understating), but this resolver is where the next steer bug lands.
  • The state machine has no single owner: the state is decided at two independent sites (chat_delivery's initial-persist gate, chat_runner's settle promotion) coupled through the slot-attribute _steer_confirmed evidence set. Any future code that removes a _pending_steers entry must know to record evidence or its steers silently stay written — fail-closed, so damage is bounded to understatement, but rounds 3/8/9 of this PR are each a cell of this machine discovered by review rather than enumerated up front.

Suggestions

[DESIGN-REVIEWED] 883f0e1

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

UX Review (Fable 5) — ✅ PASS

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

UX-Verdict: PASS

The badge now renders only when the backend has confirmed injection — the UI stops claiming a steer the turn never took, and legacy rows are untouched.

Suggestions

  • While steerState is written (and on the optimistic bubble), the steer renders identically to an ordinary message, so a user who deliberately interjected gets no signal their steer is pending until the badge pops in seconds later — a muted, non-claiming "Steering…" indicator on the written state in UserMessage.tsx would give honest interim feedback the client already has the state for.

[UX-REVIEWED] 883f0e1

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

First Principles Review (Fable 5) — ✅ PASS

Premise-level review of 883f0e11fb0c8c7b08c8395cf10556f647984c88 — 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 counts run and claims verified: the KAS wire shapes exist as described (kas_wire.py:27, session_handle.py:2495), temp-screenshots/ is a documented CI-gated convention (~250 sibling dirs), steerState has a real renderer consumer, and both production callers of the new row-marker pass an explicit siblings list — the None default branch runs only in tests.

First-Principles-Verdict: PASS

Every item traces to a confirmed false claim on the transcript, sits at cause level (record evidence, render only what's proven), and the one rider is declared and load-bearing.

What this change ships

Intent: stop the dashboard telling an operator their mid-turn correction landed when the turn was never redirected — a FIX.

  1. Steer badge renders only after the backend confirms consumption — justified (the fix).
  2. Rows record written/consumed/requeued in meta.steerState, patched via the existing chat_message_update — justified, reuses existing event.
  3. steer_push carries steerState and the row mid so live clients track the promotion — justified.
  4. Optimistic bubble no longer claims success before any server answer — justified, declared.
  5. Entrance animation now plays when confirmation arrives, not at mount — justified consequence of 4.
  6. A bare KAS echo now settles its steer, ending duplicate re-runs of already-injected questions — rides along, declared, and required: without it the new requeued state is wrong on every KAS turn.
  7. An empty echo no longer yields a consumed row (_steer_confirmed evidence set) — justified (same defect, second route).
  8. Redaction-collision rows are left written rather than guessed — justified fail-closed choice, residual tracked.
  9. Review screenshot under temp-screenshots/ — derived (documented convention, ~250 sibling dirs, UX-review CI gates on it).

Subtractions

  • Drop the siblings=None default on find_written_steer_row / _mark_steer_row_state — both production callers pass an explicit list (chat_runner.py:4391, 4429; count: 2 of 2); the fallback to slot._pending_steers has 0 production consumers and hides a caller forgetting the requeue's cleared-list trap the comments warn about.

[FIRST-PRINCIPLES-REVIEWED] 883f0e1

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

Opus 4.8 Review — ✅ no blocking findings

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

Review details

No blocking issues; one advisory.

FINDING — temp-screenshots/steer-state/steer-lifecycle-states.png:0 — a stray dev artifact under temp-screenshots/ (new file mode 100644) is committed into the repo tree; nothing references it and the directory name marks it as local scratch → Fix: drop the file from the PR and add temp-screenshots/ to .gitignore.

[OPUS-REVIEWED] 883f0e1

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

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

@github-actions

github-actions Bot commented Sep 2, 2026

Copy link
Copy Markdown
Contributor

GPT 5.6 Review — ✅ no blocking findings

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

This comment is updated in place on each push.

Review details

No findings.
[GPT-REVIEWED] 883f0e1

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

@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch 2 times, most recently from e4eb58d to f3f6080 Compare September 2, 2026 23:18
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT's BLOCKING finding on e4eb58d was correct and is fixed in f3f6080.

The tail that persists the steer row is reached by two routes and I had hardcoded written on both. If the steering_consumed echo lands while steer() is still suspended, the settle clears the pending entry before any row exists, so it has nothing to promote -- and nothing runs the promotion twice. The row would have read written forever for an injection the backend actually confirmed. That is the mirror image of the defect this PR exists to fix: overstating and understating are both the row disagreeing with the backend.

The state is now derived from still_registered rather than assumed, for the row meta and the steer_push payload alike, with a comment naming both routes. Took GPT's prescribed fix as written.

New regression test_a_steer_consumed_during_the_rpc_persists_as_consumed drives a steer() whose side effect clears the pending entry mid-await. Mutation-verified: forcing the state back to a constant reddens that test alone (assert 'written' == 'consumed') and leaves the other 31 in the file green.

The screenshot URL in the body is re-pinned to f3f6080 -- the amend changed the sha, and a stale pin would 404.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch from f3f6080 to a40c36d Compare September 3, 2026 00:47
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT's round-2 BLOCKING finding on f3f6080 was real. Addressed in a40c36d, but not by the prescribed remedy -- that remedy does not fit, and the reason is worth recording.

The finding: resolving a steer's row by sanitized content can target the wrong row. Correct, and it is the same injectivity loss steer_settle already documents for its own keys -- the in-flight guard admits one steer per RAW text while the row stores the SANITIZED text, so two steers differing only in credential material are both admitted and their rows carry byte-identical content.

The prescribed fix was to resolve by delivery id. I implemented exactly that and it broke two pre-existing tests that pin the opposite invariant: test_a_successful_steer_leaves_no_entry and test_many_successful_steers_do_not_accumulate require the _steer_delivery_ids entry to be GONE once a delivery persists its own row, because the map is keyed by message TEXT and a retained entry holds a full message string for the slot's lifetime. Keeping the id to resolve the row trades this bug for a memory leak those tests exist to prevent, and _pending_steers is a list[str] whose consumers all match by content, so there is no other identity to hand the transition. Giving a pending steer a real identity is the refactor already tracked in #4333.

So the ambiguity is faced instead: an ambiguous match now patches NOTHING and logs. Both rows keep written. Understating a state is recoverable; claiming the wrong message was the one the turn consumed is not -- the same trade settle_consumed_steers makes for its ambiguous groups. GPT's concern ("the dashboard falsely labels which message was consumed") is closed: mislabelling is now impossible.

The ts half of the finding IS fixed as prescribed. slot_buffers.update_message patches the FIRST row with a given ts, so the local write now goes to the resolved row object directly, and the chat_message_update payload carries the row's mid with the client reducer preferring it over ts.

New regressions: test_two_steers_with_identical_sanitized_content_are_left_alone (mutation-verified -- removing the refusal reddens it) and test_a_duplicate_pending_steer_settles_one_entry_only, refocused onto the multiset accounting it actually guards now that its old row-patch premise is wrong. 63 python + 330 frontend tests green; black, flake8, mypy, tsc, eslint clean.

Residual, stated plainly: two steers whose raw texts differ only in credential material leave both rows at written even if one was consumed. Narrow, non-corrupting, and its proper fix is #4333.

@github-actions github-actions Bot added merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running and removed readiness: action required A blocking check or review needs attention labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch from a40c36d to 90cdc88 Compare September 3, 2026 01:32
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

Rebased onto current main (39 commits) to clear a real CONFLICTING state; head is now 90cdc88.

The conflict was in chatSlice.ts and it resolved by DELETING my change: main has independently shipped the same mid-preference fix in sseChatMessagePatchByTs, and its version is better -- it accepts mid alone (!ts && !mid) where mine still required a ts. Took main's side verbatim, so this PR no longer touches that file (7 files now, down from 8).

main also added slot_buffers.update_message(..., mid=...) in the same window, for exactly the hazard the GPT lane raised. My previous round had worked around its absence by writing row["meta"] directly and setting slot._dirty by hand; that is now replaced by the supported API, so the patch is keyed on the row's mid through main's own primitive rather than around it. Nice convergence -- the reviewer, main and this PR all landed on the same conclusion that ts is not a row identity.

Re-verified on the new base rather than trusting the clean automerge: 63 python tests (test_steer_requeue, test_steer_settle, test_chat_steer), 222 frontend tests (UserMessage, UseWebSocketCoverage, ChatSliceCoverage), black, flake8, mypy, tsc -b all clean. Still ONE commit. Screenshot URL re-pinned to 90cdc88.

Note on the push: the scrubgate override in the audit log for this push covers main's 39 REPLAYED commit messages, which carry non-ASCII and internal addresses from their original authors. My own commit is ASCII-clean -- verified as 0 non-ASCII hits across both git diff origin/main HEAD -- src test website and the commit message.

@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed merge conflict Branch has merge conflicts with its base — author must resolve before merge readiness: checking Automated validation is still running labels Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch from 90cdc88 to bc3d170 Compare September 3, 2026 01:52
@chenmingwei23

Copy link
Copy Markdown
Contributor Author

GPT's two BLOCKING findings on 90cdc88 were both real and both mine. Fixed as prescribed in bc3d170.

1. Early settlement could relabel a stale row. A hard kill clears the pending bookkeeping without reaching either transition, so that steer's row truthfully keeps written forever. Send the same text again and, while steer() is suspended, the new steer has no row yet -- so a settle arriving in that window resolved by content to the OLDER row and marked a steer consumed that never was. Took the prescribed guard: find_written_steer_row returns None while the message still has a _steer_delivery_ids entry, because that entry lives from registration until the persisting tail pops it, so its presence proves this steer owns no row yet.

2. The mid-keyed patch targeted an id the client never received. This is a regression I introduced myself in round 3. I added mid to the chat_message_update payload, and main's reducer resolves by mid when present with no ts fallback -- but the steer_push that CREATES the client row did not carry mid, so the stored row had none, the patch matched nothing, and a consumed steer's badge stayed hidden until reload. Fixed exactly as prescribed: steer_push now carries the appended row's meta.mid (slot.append returns the row), and the client stores it.

Worth noting the shape of that second one: adding an identity to the patch without adding it to the create is the same class of mistake as the original #7246 defect -- two halves of one lifecycle disagreeing about what identifies a thing.

New regressions, both mutation-verified: test_a_settle_during_the_rpc_does_not_relabel_an_earlier_stale_row (disabling the guard reddens it) and test_the_steer_push_carries_the_row_id_the_state_patch_uses (dropping the mid reddens it with assert None == 'm-...'). 65 python + 134 frontend tests green; black, flake8, mypy, tsc -b clean. Still ONE commit. Screenshot re-pinned to bc3d170.

@github-actions github-actions Bot removed the readiness: action required A blocking check or review needs attention label Sep 3, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch from 4bfe624 to 639ba36 Compare September 3, 2026 23:15
@github-actions github-actions Bot added readiness: action required A blocking check or review needs attention and removed readiness: checking Automated validation is still running labels Sep 4, 2026
@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch from 639ba36 to b50b54f Compare September 4, 2026 01:05
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention readiness: passed Eligible automated validation passed for the current revision and removed readiness: action required A blocking check or review needs attention readiness: checking Automated validation is still running labels Sep 4, 2026
A mid-turn steer was reported as injected the moment the RPC write returned.
The backend only proves injection with a `steering_consumed` echo, and a turn
streaming text without dispatching a tool can end before reaching any
model-inference boundary, so no echo arrives, the teardown requeues the
message, and it runs as its own turn -- while the transcript still read
"Steered into the running turn".

Record the three states the backend already distinguishes on the persisted
row as `meta.steerState`: written when the bytes were accepted, consumed when
the echo confirms the running turn took them, requeued when the turn ended
without one. The settle path promotes the row and the teardown corrects it,
both through the existing ts-keyed `chat_message_update` patch, so a live
client and a reload agree. The badge renders only for consumed; written and
requeued render as an ordinary user message. Rows with no `steerState`
predate this and keep the original rendering.

Refs #7246
@chenmingwei23
chenmingwei23 force-pushed the fix/steer-midturn-consume-7246 branch from b50b54f to 883f0e1 Compare September 4, 2026 02:35
@github-actions github-actions Bot added readiness: checking Automated validation is still running readiness: action required A blocking check or review needs attention and removed readiness: passed Eligible automated validation passed for the current revision readiness: checking Automated validation is still running labels Sep 4, 2026
@bolichen97

Copy link
Copy Markdown
Collaborator

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 #6825 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 #6825: MERGE_DISCUSSION. Complementary halves of the same honesty principle on different flags; no code collision. Files: website/src/pages/chat/UserMessage.tsx.
  • This PR is PARTIALLY_COVERED with PR #7658. Coverage is explicitly incomplete; this finding is not a completion or closure claim. Recommended action for PR #7997: CONTINUE_DEVELOPMENT. Main covers the sibling routes only. The steered-then-never-consumed route this PR exists for is not implemented anywhere in current origin/main, so closure as completed is not available; the PR should land (after the open premise decision below) rather than be superseded. Files: website/src/pages/ChatPage.tsx.

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants