fix(dashboard): stop reporting a written steer as one the turn consumed - #7997
Conversation
|
The half this PR deliberately does not attempt -- a backend honoring |
Design Review (Fable 5) — 🟡 CONCERNSDesign-level review of 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
Suggestions
[DESIGN-REVIEWED] 883f0e1 |
UX Review (Fable 5) — ✅ PASSUX-level review of 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
[UX-REVIEWED] 883f0e1 |
First Principles Review (Fable 5) — ✅ PASSPremise-level review of All counts run and claims verified: the KAS wire shapes exist as described ( 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 shipsIntent: stop the dashboard telling an operator their mid-turn correction landed when the turn was never redirected — a FIX.
Subtractions
[FIRST-PRINCIPLES-REVIEWED] 883f0e1 |
Opus 4.8 Review — ✅ no blocking findingsReviewed Review detailsNo blocking issues; one advisory. FINDING — temp-screenshots/steer-state/steer-lifecycle-states.png:0 — a stray dev artifact under [OPUS-REVIEWED] 883f0e1 Verdict parsed from the review's SHA-scoped output markers for commit False positive or not applicable? A repository writer can comment: |
GPT 5.6 Review — ✅ no blocking findingsGPT 5.6 completed its review of This comment is updated in place on each push. Review detailsNo findings. False positive or not applicable? A repository writer can comment: |
e4eb58d to
f3f6080
Compare
|
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 The state is now derived from New regression The screenshot URL in the body is re-pinned to f3f6080 -- the amend changed the sha, and a stale pin would 404. |
f3f6080 to
a40c36d
Compare
|
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 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: So the ambiguity is faced instead: an ambiguous match now patches NOTHING and logs. Both rows keep The ts half of the finding IS fixed as prescribed. New regressions: Residual, stated plainly: two steers whose raw texts differ only in credential material leave both rows at |
a40c36d to
90cdc88
Compare
|
Rebased onto current main (39 commits) to clear a real CONFLICTING state; head is now 90cdc88. The conflict was in main also added 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, 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 |
90cdc88 to
bc3d170
Compare
|
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 2. The mid-keyed patch targeted an id the client never received. This is a regression I introduced myself in round 3. I added 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: |
4bfe624 to
639ba36
Compare
639ba36 to
b50b54f
Compare
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
b50b54f to
883f0e1
Compare
Open PR relationship auditThis 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
No PR, Issue, label, branch, or review state was changed by the relationship-note portion of this audit. |
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_turnpersisted the transcript row and broadcaststeer_pushas soon as
client.steer()returned. That return proves only that the bytesreached the backend process; the backend's own proof of injection is a
steering_consumedecho, which_settle_consumed_steersalready parses. A steeris 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_steersmoves the message to the queue, and the row keepsasserting 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 KASsteering_injectedspelling). What was missing was recording WHICH state a rowis in, so the change records it as
meta.steerState:writtenat persist time -- the bytes were accepted, nothing more;consumedwhen the echo confirms the running turn took them;requeuedwhen the turn ended without one and the teardown queued the message.Both transitions go through one writer (
_mark_steer_row_state) and reuse theexisting ts-keyed
chat_message_updatepatch, so a live client and a page reloadagree without a new WS event. The row is resolved by its sanitized content plus a
still-
writtenstate, because the successful-steer path is terminal for thedelivery 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
writtenrow cannot capture a later identical steer.Front end: the badge asserts the message reached the running turn, so it now
renders only for
consumed.writtenandrequeuedrender as an ordinary usermessage. A row with no
steerStatepredates this and keeps the originalrendering, 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
4357b4c35onchat_runner.py's requeue marking, and thefinding is correct, so this states the mechanism and the scope precisely.
steer_settle.settle_consumed_steersparses the echo for<user_message>-wrapped blocks. That is kiro-cli's shape. KAS does not wrap:its
steering_injectedframe is routed insession_handleand yields thecontentfield verbatim, so a KAS-backed turn produces a bare echo with noenvelope to find.
settle_all_on_empty=Truedoes not cover it either, becausethat flag guards only the
not snapshot.strip()branch and a bare echo isnon-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
requeuedrow 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_pendingpins and whichstill 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 barecontentfield is not.Scope, stated rather than discovered in review: the settlement gap is
PRE-EXISTING on
main--steer_settle.pyis byte-identical betweenmainandthis branch, and
main's_requeue_unconsumed_steers(same name here; an earlier draft of thisdescription claimed a rename, which was wrong) already re-ran bare-echo steers. What this PR added was
the
requeuedrow state, which turns that silent duplicate into a false claim onthe transcript. So the fix repairs
mainand widens this diff by one file. It istaken 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_emptyis deliberately NOT touched:chat_runner.pydocuments thatthe
/sidesidecar chose the opposite policy and that aligning the main chat is aseparate change. This fix is orthogonal to that choice, and because the function
is shared it closes the same blindness for the
/sidecaller(
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 classas #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 therunning turn CONSUMING it -- every other remover returned above". That sentence is
not a trade someone chose, it is untrue.
_settle_consumed_steerspassessettle_all_on_empty=True, so it sweeps the pending list with no evidence at all andreturns 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_steerspassessettle_all_on_empty=True, so anEMPTY frame clears the pending list without matching anything; if it lands while
client.steer()is still suspended, the entry is gone,still_registeredis 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), andchat_deliverywritesconsumedonly for adelivery id in that set, falling back to
written-- which is what is actuallyknown. Keyed on the delivery id rather than the text so a later identical steer
cannot inherit an earlier one's evidence.
settle_all_on_emptyis still untouched.Note why blind
writtenwas NOT the fix, since it is the obvious cheaper path: thecomment at that site records that writing
writtenunconditionally "wouldpermanently understate a CONFIRMED injection, since nothing runs the promotion
twice". That trades a permanent false
consumedfor a permanent falsewritten--still gating on absence of information rather than on evidence, which is the thing
this PR exists to stop.
writtenis the ELSE-BRANCH of the gate, not a substitutefor 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
writtenbranch: "no marker" and "no evidence" arethe same branch by construction. The
isinstancetest is load-bearing rather thandefensive, because
inraisesTypeErroron a non-container and.discardraisesAttributeErroron a non-set -- so an unreadable marker would otherwise crash thesteer 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_consumedsimulated the mid-RPCecho with a bare
slot._pending_steers.clear(). That reproduces the EVIDENCE-FREEempty 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_steerswith 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_pushboth reportwritten, not consumed;a
steering_consumedecho 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_consumedecho lands whilesteer()is stillsuspended: the settle removes the pending entry before any row exists, so
the row that follows must persist as
consumedrather than understating aconfirmed 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
writtenrather 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 isthe behaviour. Verified red on
556f08f939before the fix -- the written andrequeued cases fail as
assert None == 'written'andassert None == 'requeued'. Mutation-verified: neutralizing the requeue correction turns therequeued case into
assert 'written' == 'requeued'.website/src/test/UserMessage.test.tsx, three cases: no badge forwritten, nobadge for
requeued, badge forconsumed. Mutation-verified -- reverting thegate 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_steerssignature, which gained an optional trailingstateso existing callers are unaffected), plus
UseWebSocketCoverage.test.tsxandChatPage.steerKeyIdentity.test.tsx(101 passed). black, isort, flake8, mypy,tsc -b, and eslint clean on the touched files (eslint reports two pre-existingwarnings in
useWebSocket.tsat 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;writtenandrequeuedrender as anordinary user message, and a row with no
steerStatekeeps the old rendering.Captured from the real
UserMessagecomponent in a real browser (the project'sown Playwright against a Vite dev server), with the four metas supplied directly
rather than produced by a live backend turn -- reproducing the
requeuedstateend 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/steerduring a boundary-free generation is a backend contractquestion, 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_queuedvssteering_consumed), consuming only the second and rendering success off thefirst 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_pushthat created the row never sent (so the update silently matchednothing); and the promotion loop wrote
consumedfor an EMPTY echo, whichsteer_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 HEADproduced 37 phantomnon-ASCII hits here once
origin/mainadvanced past the commit's base -- theem-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 onAssertionError; 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.py16,test_steer_requeue.py37,
test_side_steer_queue.py34,test_kas_display_mapping.py32,test_chat_runner_coverage.py272 -- all passing.flake8clean on both changedfiles. The 2
mypyerrors surfaced are insrc/kiro_crew/transcribe.py, untouchedhere, and reproduce identically on an unmodified checkout.
Round 9:
test_an_empty_echo_during_the_rpc_persists_as_written(new) and thecorrected
..._persists_as_consumed; plus a sidecar pin,test_a_bare_kas_echo_lets_the_sidecar_render_its_steer, because the sidecar'sRENDER is a distinct observable --
steer_settlemarks rather than removes and itscaller 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; thechat_deliveryevidence gate -> 1; thechat_runnerevidence recording -> 1; theshared fallback against the sidecar pin -> 1. Suites at
-n0:test_steer_settle.py16,test_steer_requeue.py38,test_side_steer_queue.py35,test_chat_runner_coverage.py267 -- all passing.flake8clean on all sevenchanged files.
Also
test_an_unreadable_evidence_marker_fails_closed_to_written, which drives realevidence 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; thechat_deliveryevidence gate -> 1; themarker's own WRITE -> 1 (
assert 'written' == 'consumed', i.e. the fallback iswrittenspecifically); the fail-closedisinstanceguard -> 1 (aTypeError, notan 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_rowrefuses topatch when two LIVE steers share the sanitized content, and
_settle_consumed_steerscalled it before assigning
_pending_steers[:] = remaining-- so the whole echo'sentries 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
writtenfor 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_settlesettles 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 stillCLAIM the row are the ones still pending, plus the one being settled. Consequences:
Both end
consumed, and because they take the same state, which row is which is notobservable -- so this needs no real steer identity, which stays Session control: cross-session message delivery needs resolved-binding authorization #4333's job.
stands.
test_two_steers_with_identical_sanitized_content_are_left_alonepins that caseand passes unchanged -- the two scenarios are disjoint, so nothing was relaxed.
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_rowat
chat_delivery.py:198, and the call site changed is in_settle_consumed_steersat
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_steersis all-or-nothing for a collision of DISTINCT raw texts (echoedonce, 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:
assert ['written', 'written'] == ['consumed', 'consumed']-- understating. Its_pending_steers == []assertion still passes, proving the settlement layer did settleboth and the failure is the row patch specifically, not a fixture that missed the case.
[_msg]alone): the partial test reds withassert '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 -- fullyechoed 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
remainingbeing right, which means it rests onsteer_settle, the layer thatalready had the ambiguity guard and its own tests. If
remainingever under-reports whatstayed pending, a row is confirmed wrongly; that is the single assumption this fix adds.