Repository navigation
fix(chat): an answered form's reply no longer doubles on reload; the card's text reaches the durable transcript - #4403
Conversation
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 12 minutes. View limit details
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
…ger doubles on reload
Submitting a request_user_input form posted the raw resume JSON as the operator's
bubble ({"plan":"C"}). The bubble is now one `Label: answer` line per answered field
(choice labels, Yes/No, humanized keys); the agent still receives the JSON (`sendAs`),
and the readable text rides as the send's `display`, so a server-rebuilt chat draws
the same bubble.
A HITL answer continues the parked task as a NEW rendered turn, so the question bubble
and the reply bubble share one taskId with no splitOf link. Boot hydration flagged the
prose-less question bubble as needing a #3340 repair and wrote the task's recovered
answer onto it — the reply rendered under both turns after a reload. Only a task's LAST
local turn is now repair-eligible (both needsDurableHydration and the repair itself).
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
![protoreview[bot]](https://avatars.githubusercontent.com/in/4233969?s=60&v=4){kind=small}
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 90d7158e2d47 · formal
No findings were raised, so there was nothing to verify. The panel reviewed the change across all angles and found no defects. The verifier confirmed there was nothing to verify. No prior requests to disposition.
No findings — the review came back clean.
findings JSON (machine-readable)
[]… rebase onto main The labelled card from #4397 now owns the live bubble; this branch keeps the resume JSON as content and sends the card's text as the durable `display`, plus the reload de-duplication. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
90d7158 to
2b784f4
Compare
mabry1985
changed the title
There was a problem hiding this comment.
QA panel review — WARN
code-review-structural · head 2b784f4ffd5d · formal
Low-risk PR: narrows durable hydration to the last turn per task and adds labelled card text for form answers in the chat bubble. One minor conventions finding (dead turnReads array in the e2e spec) was confirmed by the verifier against the file at head. No panel disagreements, no verification changes, no coverage gaps.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟡 | minor | apps/web/e2e/hitl-answer-bubble.spec.ts:113 |
Dead variable turnReads is collected but never asserted on — a leftover debug probe that PROTO.md's 'no unused variables' rule (F841) forbids. |
confirmed |
findings JSON (machine-readable)
[
{
"file": "apps/web/e2e/hitl-answer-bubble.spec.ts",
"line": 113,
"severity": "minor",
"category": "conventions",
"claim": "Dead variable `turnReads` is collected but never asserted on \u2014 a leftover debug probe that PROTO.md's 'no unused variables' rule (F841) forbids.",
"evidence": "const turnReads: string[] = [];\n await page.route(`**/api/chat/sessions/${SESSION}/turns*`, (route) => {\n turnReads.push(route.request().url());\n return route.fulfill({ json: { turns: [row], live_task_id: null } });\n });",
"verdict": "confirmed",
"note": "Read the full file at head 2b784f4: `turnReads` is declared and pushed to but never read or asserted anywhere in the spec. The quoted evidence matches the file verbatim."
}
]
There was a problem hiding this comment.
Promoting the WARN verdict for head 2b784f4ffd5d: all checks terminal-green, zero unresolved review threads. (approve-on-green)
Open findings carried by this approval — non-blocking, but they did not go away:
- minor
apps/web/e2e/hitl-answer-bubble.spec.ts:113— Dead variableturnReadsis collected but never asserted on — a leftover debug probe that PROTO.md's 'no unused variables' rule (F841) forbids.
Approving a WARN does not resolve its findings (issue #22).
… boot Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
There was a problem hiding this comment.
QA panel review — PASS
code-review-structural · head 2321d94a48f5 · formal
PR #4403 narrows durable hydration to the last turn and adds form-response card text to the transcript. Verification confirmed the single major finding: createParkTracker's pluginForm flag never resets, so a plugin-form callback early in a turn permanently suppresses any subsequent real input-required park. The verifier notes the doc comment asserts the plugin form "completes the task server-side," making the scenario arguably unreachable, but the code does not enforce that invariant. Fix first: either reset pluginForm when the task transitions to a non-parked state, or add a guard that only suppresses the first park after a plugin form. No panel disagreement; the finding was unanimous and confirmed. Coverage gap: no LLM finder independently reviewed turnReducers.ts logic beyond protoPatch's structural pass, so that file went under-read on the behavioral side.
Findings
| Severity | Location | Finding | Verified | |
|---|---|---|---|---|
| 🟠 | major | apps/web/src/chat/turnReducers.ts:275 |
createParkTracker's pluginForm flag never resets, masking subsequent real input-required parks in the same turn. | confirmed |
findings JSON (machine-readable)
[
{
"file": "apps/web/src/chat/turnReducers.ts",
"line": 275,
"severity": "major",
"category": "correctness",
"claim": "createParkTracker's pluginForm flag never resets, masking subsequent real input-required parks in the same turn.",
"evidence": "Once `pluginForm` is set to true (line 283), it is never reset for the lifetime of the tracker. The `taskState` method computes `next = isParkedState(state) && !pluginForm`, so any subsequent real input-required park in the same turn is permanently suppressed.",
"source": "protopatch",
"verdict": "confirmed",
"note": "Re-read at head: line 283 `if (payload.plugin_callback_id) pluginForm = true;` has no corresponding reset; line 288 `const next = isParkedState(state) && !pluginForm;` suppresses all subsequent parks once set. No test in turnReducers.test.ts covers plugin-form-then-real-park in one turn. The doc comment asserts the plugin form 'completes the task server-side' (making the scenario unreachable), but the code does not enforce that invariant \u2014 it silently drops the park if it ever occurs."
}
]1 finding(s) excluded from the verdict by in-diff confinement (file not among this PR's changed paths):
apps/web/src/chat/turnReducers.ts(major) — createParkTracker's pluginForm flag never resets, masking subsequent real input-required parks in the same turn.
Addressed since the last round (1 minor/nit): apps/web/e2e/hitl-answer-bubble.spec.ts:113 — no longer raised, and the PR changed the code around them since they were raised.
There was a problem hiding this comment.
Promoting the PASS verdict for head 2321d94a48f5: all checks terminal-green, zero unresolved review threads. (approve-on-green)
Open findings carried by this approval — non-blocking, but they did not go away:
- major
apps/web/src/chat/turnReducers.ts:275— createParkTracker's pluginForm flag never resets, masking subsequent real input-required parks in the same turn.
Approving a WARN does not resolve its findings (issue #22).
1 participant
What broke (B4)
request_user_inputform showed raw JSON ({"plan":"C"}) as the user bubble. feat: Show persistent labeled HITL form-response cards without changing resume JSON (#4259) #4397 (HITL form answers render as raw JSON in the user bubble — show a structured response card instead #4259) landed the live fix while this PR was open (a labelled form-response card on the local message). It still showed JSON in any chat rebuilt from the server (another browser, cleared cache), because nothing readable reached the durable transcript.Root cause of the duplicate
A HITL answer continues the task that parked (#3930) as a NEW rendered turn: question bubble → operator answer → reply bubble, all on one
taskId, with nosplitOflink. On boot,needsDurableHydrationtreated the prose-less question bubble (tool card, no text) as a #3340 turn missing its answer. It fetched the durable turns, andrepairHydratedMessageswrote the task's recovered answer onto that bubble. The reply was then on both turns, and the duplicate was persisted.Fix
chat-store.ts: only a task's LAST local turn can be repaired (lastTurnOfTask). This applies in bothneedsDurableHydrationandrepairHydratedMessages. The Switching agents in the console loses the prior turn's rendered reply, keeping only its tool cards #3340 repair of the reply bubble itself still works (tested).content. The resume send now carries the card's text asdisplay(formResponseText:Label: valuelines, with step titles for wizards). The server already storesdisplayas the durable bubble text, so a server-rebuilt chat readsPlan: Plan C.Tests
chat-storeHITL-answered turn: no repair flagged, nothing written onto the question bubble, and the reply-bubble repair still works. These fail on main.formResponseTextcases.hitl-answer-bubble.spec.ts:{), and the A2A send carries{"plan":"C"}+display: "Plan: Plan C".npm run test:unit3982 passed. Full chromium e2e was green before the rebase; the hitl specs are green after it.Verified in a real browser
Setup: an isolated dev server built from this branch (own box root, :7893, real model). Playwright prompted the agent to call
request_user_input(plan A/B/C), picked Plan C, submitted, reloaded, then cleared the local cache and reloaded again (server hydration).{"plan":"C"}, and after reload "You chose Plan C." appeared under both turns.Plan: Plan C, and the agent still answered from the JSON.🤖 Generated with Claude Code