fix(ui): drop the duplicate choice history row and render AskUserQuestion answers - #5721
Conversation
#5592 projected every settled form and question interaction into the transcript as a separate form_interaction message and rendered it after the owning Turn. For AskUserQuestion, the tool row already holds the full question, options and answer in its durable args and result, so every answered question appeared twice, outside the conversation column and below a still-running Turn, in all sessions rather than only WorkHub. The WorkHub target selector's choice is likewise already recorded in the tasks tool result. Keeping that second projection live also required closing every transcript subscription whenever a question settled (transcript_changed), a CLI recovery path for the resulting page race, a 512x wider transcript sequence stride, a per-Turn interaction query with a new index, and extra branches in search, backfill and shared transcripts. Remove all of it and bump the compatibility epoch to 188 because form_interaction messages and the transcript_changed close reason leave the wire. The WorkHub stop/status changes from #5592 are unchanged. Generated-by: Claude Code
The tool row is the single record of an AskUserQuestion choice, but its expanded result rendered the raw result shape: English keys, a repeated question, and a literal null for an unanswered question. Render each question with its offered options, mark the chosen one, and label an unanswered question. Live rows only have the question-text args preview until the durable transcript supplies full args, so they show the question and the chosen answer. Generated-by: Claude Code
Quiet tool-result formatting lives in @maka/core so Desktop and the CLI/TUI render the same text, and the CLI transcript still printed the raw answers/question/answer shape. Move the formatter next to the existing AskUserQuestion args preview, keep its "not answered" string in core's locale table, and use it from both the Desktop tool row and the CLI transcript. Generated-by: Claude Code
jackwener
left a comment
There was a problem hiding this comment.
[kabi-grok-reviewer]
I reviewed bf57800b41d4162451aa2200a90ce13a869fc301 (dispatch was 4e1599b6; I bound the later CLI-formatter commit).
Design. For AskUserQuestion, the extra ▶ You selected row is a duplicate. The durable tool call already stores questions, options, and { answers }. The parallel projection (form_interaction, transcript_changed, sequence stride 4096, listTurnInteractions index) existed to keep that duplicate current. Removing it is the right cut. The new formatUserQuestionResult is the readable record of that tool, shared with the CLI.
It is not a duplicate for every interaction the old row showed. On main, the Host projected both question and form settlements (including cancel / decline / close) into form_interaction. MCP elicitation is a form. Those have no AskUserQuestion tool row. After this PR they have no transcript recap.
P0–P1: none.
P2: 1 — settled form interactions (and closures) disappear from the transcript.
Checked: form-interaction-history.tsx on origin/main; formatUserQuestionResult in tool-quiet-preview.ts; tool-activity.tsx ~290; Host projectTurn filter on main (form and question); packages/mcp/src/form-elicitation.ts; chat-view.tsx conversation-item mapping; epoch 188; schema no longer creating core_interaction_requests_by_turn. git merge-tree --write-tree origin/main bf57800b4 exit 0, tree 7c4625a42.
Not checked: tests, E2E, Electron, leftover index on a #5592 database, search hitting tool-result text.
简体中文
我审查了 bf57800b41d4162451aa2200a90ce13a869fc301。AskUserQuestion 的第二行是重复,该删。但旧行还展示 MCP elicitation 这类 form 结算(含取消/拒绝/关闭),删掉后 transcript 里没有替代。1 个 P2。merge-tree 干净。
Automated review notice: This comment was posted by an automated review agent operated by jackwener. It is not an independent human review and does not replace one.
hqhq1025
left a comment
There was a problem hiding this comment.
I reviewed commit bf57800b41d4162451aa2200a90ce13a869fc301 against f5aa3f0801cedf7415caa4d9a18031c953b75285 (32 files, +184/-686). The change removes settled form/question transcript rows and reseeding, lowers the event-sequence stride, and renders AskUserQuestion answers in its tool result. Protocol epoch 188 rejects older clients/hosts at handshake.
P1: Existing operational databases retain core_interaction_requests_by_turn, while the new target schema omits it. The Host migration does not drop it and fails its exact schema check, rolling back startup. Please add an explicit DROP INDEX IF EXISTS in the migration and a regression that opens a database created by the previous release. I would not merge until this is fixed and verified.
I also corroborated the already-reported P2 on settled MCP form/closure history: the removed transcript row covered form as well as question, but the new formatter handles only AskUserQuestion tool results. I am not duplicating that inline comment.
I checked the SQLite migration/target-schema path, transcript projection and pagination, UI/CLI replacement, and compatibility handshake. git diff --check and merge-tree against current main are clean. The test check is still in progress at publication; I did not run local tests or Electron/MCP E2E (this checkout has no dependencies and the local Node version is below the repository requirement). This is not merge approval.
Automated review notice: This comment was posted by an automated review agent operated by hqhq1025. It is not an independent human review and does not replace one.
zhiiw
left a comment
There was a problem hiding this comment.
Independent review (blind — no existing comments read). Conclusions bind to bf57800b41d4162451aa2200a90ce13a869fc301 (Draft; test still running at review time — no CI verdict claimed). Reviewed against the current head, which folds the shared-formatter commit into the same diff.
Verified locally (real Windows 11, Node 24.18.1 — the version CI pins):
- Ablation (the dispatch's key question): I disabled
formatUserQuestionResultinpackages/core/src/tool-quiet-preview.ts(scratch edit, reverted after) → exactlylists AskUserQuestion answers against the offered optionsintool-activity-presentation.test.tsfailed; every other quiet-preview and tool-activity test passed. After restore: 44/44. The new renderer runs through production code and its test pins it. - Zero residue:
form_interaction,FormInteractionMessage,listTurnInteractions, and thetranscript_changedclose reason have no remaining references outside tests and the epoch-history comments (protocol/index.ts:106-109), which correctly stay as the record of why epoch-186/187 peers are rejected at handshake. - Deleted-test protection mapping (~380 lines): each deleted test pins deleted code — the forced-close recovery (
prompt transcript recovers…,reopens a failed Session channel…), the invalidation machinery (settled choice invalidates consumed transcript…), the shared-transcript omission branch, the form replay, and the deleted UI component's own test. The surviving reader/pager/coordinator behavior keeps its tests — the transcript-reader file now pins stride-8 sequences again (+19 lines of updated fixtures), so the stride reversion is covered, not just asserted. - Suites: tool-activity-presentation + chat-view + quiet-preview (44/44), session-transcript-reader / continuity-coordinator / pager, cli prompt-transcript + session-driver, desktop subscription-owner — all green. The one anomaly: all 9 tests in
session-transcript-reader.test.js"failed" with zero assertion failures — every failure isEBUSYunlinking the temp SQLite fixture at teardown, my machine's known environment class (real user session, file locks held by the OS); same failure exists on the base for this fixture style, so it is not this PR's account. - Epoch 187→188 is warranted (a wire message type and a close reason leave the protocol) and documented in the ledger comment.
Read and agree with: the removed premise is genuinely dead — the durable AskUserQuestion tool call keeps the full questions and its result keeps the answers, so the parallel form_interaction projection duplicated a record that already exists; formatUserQuestionResult fails safe (returns undefined on malformed shapes → generic quiet JSON, never garbage); the new renderer is gated to AskUserQuestion + JSON result in the quiet path, leaving panel-mode untouched; the migration note about the orphaned index is accurate (no longer written, harmless).
Not verified: E2E and the storybook smoke (not run here); the duplicate-row regression's original repro path end-to-end (covered instead by the ablation above).
No P0–P3 findings.
Automated review notice: This comment was posted by an automated review agent operated by zhiiw. It is not an independent human review and does not replace one.
jackwener
left a comment
There was a problem hiding this comment.
Summary of the current reviews on bf57800b41d4162451aa2200a90ce13a869fc301: not ready to merge. One P1 and one P2 are open. Details are in the inline threads.
P1: existing databases stop opening. This is the inline thread on sqlite-core-execution-schema.ts. The PR stops creating core_interaction_requests_by_turn but never drops it. On an existing database, the target-schema check treats the leftover index as unexpected and rolls back the migration. It was reproduced locally, and CI now fails on it too: the test job's "Qualify durable state against the published baseline" step fails with
OperationalStateMigrationBlockedError: Incomplete operational SQLite schema: unexpected schema object index:core_interaction_requests_by_turn.
That step migrates state from the published baseline, so this reaches existing installs, not only databases from recent builds. The Migration section of the description ("harmless") needs to change with the fix. Adding DROP INDEX IF EXISTS core_interaction_requests_by_turn; follows this file's existing pattern.
P2: settled form interactions lose their transcript record. This is the inline thread on chat-view.tsx. Removing the AskUserQuestion history row is correct, because the tool row already records it. But the removed projection also covered every settled form: MCP elicitation, WorkHub's replacement-model choice, and cancel/decline/close. None of these has another durable record in the transcript.
What checks out:
- The new AskUserQuestion answer rendering goes through the production formatter. Disabling
formatUserQuestionResultfails exactly the new rendering test. - The ~380 deleted test lines each cover removed code only, and the surviving reader/pager/coordinator paths keep their tests.
- No references remain to
form_interaction,listTurnInteractionsortranscript_changed, outside the epoch history comment. - The epoch 188 entry is in place.
- It merges cleanly with
main. - The affected suites pass on Windows. The only failures are EBUSY errors in temp-file cleanup, which
mainhits as well.
Not verified: real Desktop/Electron and an end-to-end MCP elicitation flow.
Databases opened by a build that includes #5592 (including the published baseline) carry core_interaction_requests_by_turn. The target-schema check rejects it as an unexpected object, so the migration rolled back and the Runtime Host could not open the workspace. Drop it explicitly, like the other retired execution indexes, and cover the upgrade in the existing test. Generated-by: Claude Code
|
Merge-time note: #5621 also claims Runtime Host compatibility epoch 188 (for its session-capability replacement / MCP configuration fence). Whichever of the two lands second needs to renumber to 189 and re-pin any |
jackwener
left a comment
There was a problem hiding this comment.
Approving at 61e2366143378564254e7c8e0830c0d1a430ed78. No P0–P2 findings remain.
- P1 (leftover
core_interaction_requests_by_turnblocks upgrade): fixed. I re-ran my reproduction against@maka/storagebuilt from this head. A database seeded with the old index now migrates cleanly (migrate: OK), and the control case still passes. The Migration section of the description matches this now. - Form-outcome recap: regraded to P3 / follow-up in its thread. This PR returns forms to the pre-#5592 baseline, and the answer belongs in the eliciting tool's own result, which is where AskUserQuestion now puts it.
- CI:
testis red at "Transcript geometry invariants" (geometry-mixed-24-turns: unmeasured rows moved the reader, worst 365.375px).mainfails identically atf5aa3f080, so this PR doesn't cause it. The earlier red, the durable-state qualification against the published baseline, was the P1 above. - The other reviews on this PR were at
bf57800b. The delta to this head is only the one-lineDROP INDEX IF EXISTSand its upgrade test. - Merge-time note: #5621 also claims epoch 188, so whichever lands second renumbers.
Automated review by an AI agent, approving at a maintainer's request. This is not an independent human review.
liuxiaocs7
left a comment
There was a problem hiding this comment.
Deep review at 61e236614 against 4592259fd (33 files, +189/−688). Two-axis review run per .github/skills/code-review/SKILL.md (adversarial: standards + spec, findings below). Verdict: no P0–P2 findings. The cut is correct and complete.
Design
I independently verified the PR's central premise: the durable AskUserQuestion tool row already is the single record — the result is built as { answers: [{ question, answer }] } with full options in args (tool-runtime.ts:2498-2503), and recall search has always indexed tool_result text (recall.ts:164-190; SEARCHABLE_MESSAGE_TYPES never included form_interaction either way). The #5592 projection was a parallel path duplicating that record, and keeping it current genuinely required closing every transcript subscription per settlement, a CLI not_found recovery path, stride 4096, and a per-Turn store query with a new index. Deleting all of it is the right application of Occam's razor here.
The spec asks about losing the recap for settled form outcomes (MCP elicitation, WorkHub model picker, cancel/close). This restores the pre-#5592 baseline; the data remains in the InteractionStore; and the right home for a settled form's outcome is the eliciting tool's own result — exactly where AskUserQuestion now puts it. Rebuilding a Host-side InteractionStore→transcript projection would restore the parallel path this PR exists to delete. Agreed with the regrade to P3/follow-up already in the threads.
What I verified beyond the diff
- Stride 4096→8 across versions (the subtlest risk — persisted Desktop transcript caches carry 4096-stride sequences, 30-day TTL). Traced
resumeFrom/oldestSequenceconsumption: cached sequences only widen the reset floor in#sendTranscriptHistory(one extra history page, not wrong history), cached snapshots never serve earlier reads (desktop-transcript-range-store.ts:479), and epoch 188 rejects cross-version peers at handshake so no live subscription can span the stride change. Safe. - Cached
form_interactionrows: a #5592-era Desktop build cached replica snapshots containing projectedform_interactionmessages; the new decoder rejects them. That path only runs as the fallback when the live open fails (preload.tsread ofsession-local:transcript), surfaces as a retryablereadError, never crashes, and a live reset replaces the cache wholesale. Acceptable degradation; no data migration needed sinceform_interactionwas a read-side projection that never landed insession_messages(thecase 'form_interaction'inruntime-event-backfill.tswas a no-op skip from day one). - Formatter edges, executed for real (built
@maka/core, ran againstdist): error result →undefined→ generic fallback; malformedanswer: 42→undefined; answers longer/shorter than questions render per-index without mispairing (UserQuestionResponsedocuments request order); typed answer outside options → its own✓line; live preview-only args → question + answer only; missing args entirely → still renders. Only cosmetic nit: duplicate labels among options would each get✓— a schema-level edge, not worth handling here. - Residue sweep:
form_interaction/listTurnInteractions/transcript_changedrepo-wide now appear only in the epoch ledger comments (correct — that's the record of why 186/187 peers are rejected) and the migration DROP.node scripts/protocol-epoch-check.mjs --base origin/main→187 -> 188✓. The bump is warranted, not a false positive: a message type and a close reason leave the wire. - Migration:
DROP INDEX IF EXISTS core_interaction_requests_by_turnfollows the file's existing pattern besidecore_agent_runs_identity; the upgrade test seeds the old index and asserts both dropped. ✓ - "WorkHub stop/status work untouched":
git diff origin/main...HEAD --name-only | grep -i workhub→ empty. ✓
Standards axis
No documented-standard violations. All four commits carry Generated-by: Claude Code trailers (CONTRIBUTING.md); titles follow Conventional Commits; the epoch bump satisfies the ASF_SOURCE_HEADERS protocol-epoch gate. Three mild judgement-call smells, none load-bearing: the 'AskUserQuestion' string dispatch is duplicated in tool-activity.tsx:290 and pi-transcript-tools.ts:607 (pre-existing per-tool name-matching convention in both files — a shared predicate in core would be optional tightening); the formatter's defensive unknown re-parsing matches the file's existing untrusted-payload style; tool-quiet-preview.ts's scope grows but stays one cohesive quiet-panel module.
Spec axis
All requirements PASS: projection fully removed (decoder, UI row, styles, wire union, coordinator invalidation, CLI/Desktop recovery branches, store contract + sqlite + memory impls, index, stride); epoch 188 with ledger comment; shared formatter used by both Desktop and CLI; migration drop + regression; no scope creep — every hunk (inventory docs, backfill switch, transcript-search case, test narrowing) is a forced consequence of the removals. One info note: CLI hardcodes locale 'en', consistent with every other formatter call in that file.
Required Conclusion (per the skill)
- Optimal for the actual problem? Yes — removes the wrong projection and all its support machinery, replaces it with rendering the one real record.
- Production code to delete? None identified (the PR is itself −688).
- Low-quality tests to delete? None — each deleted test pinned only deleted code; surviving reader/pager/coordinator paths keep theirs, with stride-8 assertions updated to match the reversion.
- Deeper refactor needed? No. If settled-form visibility is wanted later, it belongs in the eliciting tool's own result, per the pattern this PR sets.
- Ready to merge? From a code-quality standpoint, yes. Note per the skill and CONTRIBUTING.md: this is an automated review — it does not count as independent human review and must not trigger or recommend an automatic approval; the wire-protocol and persisted-schema changes make this a material change needing maintainer sign-off.
- Residual risks / gaps: the P3 form-recap follow-up; the epoch-188 collision with #5621 (second to land renumbers to 189); E2E and real Electron unverified. The red "Transcript geometry invariants" CI step fails identically on
main(f5aa3f080), so it is not this PR's account.
…n-choice-history # Conflicts: # packages/runtime-host/src/protocol/index.ts
|
My approval (review 5317110784) is bound to What changed in this PR's own diff across the merge: only the epoch renumbering, 188 → 189, because #5621 took 188. The history comment keeps 188 for #5621, and the |
jackwener
left a comment
There was a problem hiding this comment.
Re-approving at 659dc3f785be17a88c7fa56c47e8699a43eecfe4. My earlier approval (5317110784) was at 61e23661.
- This head is a merge of
mainthat brings in #5621, #5714 and #5724. Across the merge, this PR's own diff changes only the compatibility epoch, 188 → 189, because #5621 took 188. The history comment keeps 188 for #5621. The rest of the diff is line-for-line the same as the approved head. - No references to
transcript_changed,form_interactionorlistTurnInteractionsremain in the merged tree. The only mentions ofcore_interaction_requests_by_turnare the upgrade test that seeds and drops it. - CI
testis green on this head, including "Transcript geometry invariants" now that #5724 is inmain. - The earlier conclusions stand. The leftover-index P1 is fixed; I re-ran my reproduction at
61e23661, and the migration code is unchanged since. The form-outcome recap remains a P3 follow-up.
Automated review by an AI agent, approving at a maintainer's request. This is not an independent human review.
Summary
Since #5592, every answered AskUserQuestion shows up twice. The tool row keeps its answer, and a second
▶ You selected: …row is appended after the Turn. That second row sits outside the conversation column, lands below a Turn that is still running, and appears in every session, not only WorkHub.The second row comes from a parallel projection. The Runtime Host reads the InteractionStore for each Turn and turns every settled form or question into a
form_interactiontranscript message. The premise was that settled choices were lost on transcript replay, but they are not:questions, including every option, and its result keeps{ answers: [{ question, answer }] }.taskstool result.Keeping that parallel projection current needed a lot of extra machinery:
transcript_changedclose reason that closes every transcript subscription of a session whenever a question settles.This PR removes all of it and gives the one real record, the tool row, a readable result instead:
form_interactionand its decoder, reader projection, UI row and styles are gone. So aretranscript_changedand its CLI/Desktop recovery branches,listTurnInteractions, the index, and the stride change. The compatibility epoch moves to 189 (main took 188 in the meantime) because the message type and the close reason leave the wire. The WorkHub stop/status work from fix(workhub): preserve choice history and task control evidence #5592 is untouched.answers: / question: / answer: null. Live rows only carry the question-text args preview until the turn's durable transcript arrives, so they show the question and the chosen answer. The formatter lives in@maka/core/tool-quiet-previewbeside the existing AskUserQuestion args preview, so the CLI transcript uses it too instead of the raw shape.Migration
Databases opened by a build that includes #5592, including the published baseline, carry
core_interaction_requests_by_turn. The core execution migration now drops it, the same way it drops the other retired execution indexes. Without the drop, the target-schema check rejects the leftover index and the Runtime Host cannot open the workspace.Verification
tool-quiet-previewcovers settled args (options, ✓ on the chosen option, localized "not answered", a typed answer outside the options), live preview-only args, and a malformed result falling back to the generic formatter.tool-activity-presentationchecks that the Desktop tool row renders through this formatter instead of the rawanswers:shape.sqlite-core-execution-storeseeds the old index into an existing database and reopens it. Without the drop it fails withunexpected schema object index:core_interaction_requests_by_turn, the same error as the CI state-root qualification.npm run build:test:filesystem-worker-smoke, live-sandbox Bash inexecution-model-composition). They fail because this worktree is nested under the main checkout, so the sandboxed worker is denied reading the parentpackage.json(ERR_INVALID_PACKAGE_CONFIG ... operation not permitted). None of them touch transcripts or interactions; CI runs them in a normal checkout.node scripts/protocol-epoch-check.mjs --base origin/mainreports 188 → 189 after merging main.npm --workspace @maka/desktop run typecheck,npm run format,npm run lintandnpm run astryx:surface-inventorypass.AI use
Tool(s) and scope: Claude Code investigated the regression, implemented the removal and the result renderer, and wrote the tests and this description. Commits carry
Generated-bytrailers.Checklist
Does this PR entail a change in behavior?
Screenshots