Repository navigation
fix(prompt): drop the unused rewind record, pin dialog reader order and remove dead registry paths - #1890
Merged
Conversation
added 3 commits
October 24, 2026 23:10
…e rewind list's send guard (#1582) Chat's own rewind (#1500, never released) kept claude-rewinds.json in the state directory; nothing reads it since #1577, so the server removes it when it starts. One recorded fixture of the rewind's message list comes back, so a committed test holds Chat sends on the screen the dialog opens on. The banner comment names Claude Code beside grok and agy, and chatOwnedInput, which only returned the last chat send, is inlined.
…egistry's dead paths (#1437) - registry.test.ts pins, per agent, the order its screen is offered to the readers; swapping two priorities now fails a test. Its snapshot comment cites #1435's base, 3df0824. - A picker's final key comes from the chosen answer's own key sequence, not the first answer's. - Removed what nothing read: InteractionPlan.identity, ReaderContext.fallback (the three fallback readers only lend their answer behaviour; parseFallbackPrompt draws those cards) and modelListWaits; the fallback card's model-list check now asks registry.waits. - parsePrompt stops at the first reader that reads the screen, and a parser several readers share (Claude's approval, Hermes' clarify, pi's dialog, OmO's forms) runs once per read pass. - interaction-plan.test.ts makes a move before the asking ends.
Review follow-up: onePerPass keys a shared parser's result by the screen and the pass's context object, so a caller that changed the context between calls would read the previous pass's result. ReaderContext's fields are now readonly, so such a change fails to compile.
|
No actionable comments were generated in the recent review.
 |
| Layer / File(s) | Summary |
|---|---|
Startup cleanup server/index.ts, server/claude-rewinds-cleanup.test.ts |
Server creation attempts to remove claude-rewinds.json from the selected state directory. Removal errors are logged, and startup continues. Tests check removal, preservation of another state file, and startup when no rewind record exists. |
Rewind screen and chat input handling server/fixtures/claude-rewind-2.1.304-select-banana-40.txt, server/dialogs/claude.test.ts, server/core/claude-rewind-send.test.ts, server/core/composer.ts, src/components/PaneTerminal.css |
Tests cover the select-banana rewind screen and refusal to send while it is open. The composer uses lastChatSubmitted directly, and the rewind-hint comment includes Claude Code. |
Dialog reader behavior
| Layer / File(s) | Summary |
|---|---|
Per-pass parser reuse server/dialogs/shared.ts, server/dialogs/claude.ts, server/dialogs/hermes.ts, server/dialogs/omo.ts, server/dialogs/pi.ts, server/dialogs/registry.ts, server/dialogs/registry.test.ts |
onePerPass reuses a parser result for the same screen and context. Claude, Hermes, OMO, and Pi readers use the wrapper. Tests cover reuse, and ReaderContext properties are readonly. |
Reader selection and wait detection server/dialogs/live.ts, server/dialogs/fallback.ts, server/dialogs/registry.ts, server/dialogs/registry.test.ts, server/prompt.ts, server/prompt.test.ts |
Prompt parsing returns the first active non-result reader. Fallback readers return null, and prompt wait detection uses registry waits. Registry tests pin reader order. The prompt code removes chatOwnedInput and modelListWaits. |
Answer-specific interaction plans server/dialogs/interaction-plan.ts, server/dialogs/interaction-plan.test.ts |
Commit steps and terminal fallback checks now use the selected answer index. Tests cover interrupted moves and answers with different commit sequences. |
Priority:
Estimated code review effort: 3 (Moderate) | ~25 minutes
Change: Bug fix · Severity of issue fixed: Low
Merge Risk: 
c707e
-
No identified behavior change blocks merging after normal checks.
Pre-merge checks: 
-
Check name Status Explanation Resolution Docstring Coverage Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 18 files. (2 skipped:… Write docstrings for the functions missing them to satisfy the coverage threshold. -
Check name Status Explanation Title check The title clearly summarizes the main changes: removing the unused rewind record, pinning dialog reader order, and removing dead registry paths. Description check The description explains the problems, resulting behavior, issue links, implementation details, and validation results. It provides complete Change and Validation information, although it uses "Verifi… Linked Issues check The changes meet the coding requirements in [ #1582] and [#1437]. For [#1582],server/index.tsremovesclaude-rewinds.jsonwith logged, non-blocking error handling. The restored fixture has tests f…Out of Scope Changes check The changes stay within [ #1582] and [#1437]. The fixture, startup test, interaction-plan tests, parser wrappers, readonlyReaderContext, and registry wait integration directly support the linked req…Full details: Description check
-
Explanation
The description explains the problems, resulting behavior, issue links, implementation details, and validation results. It provides complete Change and Validation information, although it uses "Verification" instead of the template heading.
Full details: Linked Issues check
-
Explanation
The changes meet the coding requirements in [
#1582] and [#1437]. For [#1582],server/index.tsremovesclaude-rewinds.jsonwith logged, non-blocking error handling. The restored fixture has tests for rewind-screen detection and the send guard. ThePaneTerminal.csscomment includes Claude Code.chatOwnedInputis inlined throughlastChatSubmitted. For [#1437], registry tests pin reader order and parser memoization.pickerPlanandexecuteInteractionPlanuse the selected answer's key sequence. The change removesInteractionPlan.identity, the unused fallback parsing path, andmodelListWaits.parsePromptstops after the first active reader, and shared parsers useonePerPass. The snapshot provenance comment is updated.
Full details: Out of Scope Changes check
-
Explanation
The changes stay within [
#1582] and [#1437]. The fixture, startup test, interaction-plan tests, parser wrappers, readonlyReaderContext, and registry wait integration directly support the linked requirements or verify them. The reviewed diff shows no unrelated product behavior or unrelated files.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 68.75% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 16 functions across 18 files. (2 skipped: 2 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 2
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- Commit to this branch
- Create a new PR
🛠️ Fix failing CI checks: 💡
-
- Commit to this branch
- Create a new PR
-
- Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts
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 @coderabbitai help to get the list of available commands.
Owner
Author
|
Path check for the merge pass: every path this PR changes is plain ASCII. |
Owner
Author
|
Merge pass review-work report. Overall: PASSED.
What was checked
All required checks are green and no review thread is open. |
This was referenced Oct 24, 2026
Merged
devswha
added a commit
that referenced
this pull request
Oct 25, 2026
## Change Follow-ups from #1399's review (#1445), for Command Code (`cmd`) panes. Per item: - **2, the footer check was strict** (`server/dialogs/cmd.ts`, `server/dialogs/live.ts`): a permission prompt now gets its card when its footer wraps (split hint, word or character wrap) or uses the ctrl+e / ctrl+y variants. These are the shapes the published `command-code` 1.88.0 renderer draws (read statically from the npm package; the CLI was never run or logged into). A stale or incomplete footer, or anything drawn below it, still gives no card. Normal status chrome is hidden while a permission dialog is open, so it cannot sit under the footer. - **LOW, an ordinary numbered menu was labelled an approval card**: a menu is an approval card only when its rows are Command Code's own Yes … No choices, so a theme picker with the same footer is not one. - **4, herdr's model stuck in the transcript cache** (`server/conversation.ts`, cmd metadata branch only): the served model follows the pane's current model, and the ETag changes with it. The response is a new object, so the cached page is never mutated. herdr's live model now also replaces a model recorded in the transcript, which is what the item asks for. This branch is only reached where herdr reports the pane's Command Code session, which no released herdr does yet (#1647), so the changelog entry says so. - **5, store validation copied from `server/pi.ts`**: both now use `server/transcript-store.ts`. Behaviour is identical, and the old names are re-exported. - **6, test gaps**: tests for the cmd metadata branch, for the keys each answer sends from every cursor position, and for refusing a stale card over HTTP (409 `prompt_changed`, no keys sent). Session resolution and the skills source were already covered. - **LOW**: the unused `CmdTranscriptOptions.home` is removed. **Not fixed** - **1, the card id covers everything above the rows**, so a card goes stale when output above an open dialog redraws, as before. Narrowing the id to the dialog was tried and reverted: two review rounds showed that command text (a heredoc) can imitate the dialog's frame, including its real layout (rule, blank line, heading, subtitle). Then two different commands got the same id, and a stale card could be answered. Doing this safely needs Command Code to expose the dialog, or a capture proving a boundary that command text cannot imitate. A test pins the safe behaviour. - **LOW, a multi-line command's title**: left as main draws it, for the same reason (it would need the same boundary guess). - **3, the shell's cwd won over the agent's**: already fixed on main by #1745. - **LOW, the comment saying the body is part of the id**: it is accurate with main's id, which covers everything above the rows, the body included. Fixes #1445 ## Verification - New tests fail on origin/main and pass here: footer shapes, the Yes/No menu check, the model and ETag refresh without a transcript append, and the answer keys. Tests from the reviews are kept as regressions. Heredoc and indentation changes give different ids, a quoted dialog frame in the real layout gives different ids, and file-permission card bodies equal main's. The stale-card route test gets 409 with no keys sent. - Single-file runs after the rebase onto main: `server/cmd.test.ts` 56 pass, `server/prompt.test.ts` 357 pass, `server/cmd-conversation.test.ts` 2 pass, `server/pi.test.ts` 31 pass, `server/dialogs/registry.test.ts` 55 pass, `server/dialogs/interaction-plan.test.ts` 23 pass, `server/dialogs/claude.test.ts` 13 pass. - Live check over HTTP, against a real headless herdr session of its own (not a fake socket) and the server's `/api/pane/prompt` routes. A pane reported as `cmd` drew Command Code's shell permission dialog at 80 columns, where the terminal wraps the footer mid-word (`… bypass all perm` / `issions (Docs ↗)`): - this branch: an approval card with Yes / Yes, don't ask again / No. origin/main: only the generic fallback card (↑ ↓ Enter Esc). - a theme picker with the same footer: this branch gives the generic menu card; origin/main labels it an approval card titled "Theme". - a line redrawn above the open dialog gives a new id; answering the old id gets 409 `prompt_changed` and no byte reaches the pane; answering the new id sends only Enter. - Reviews: - Round 1: omo-native-code-reviewer and Codex (gpt-6.1-sol, xhigh) each found a HIGH in the narrowed card id (a heredoc hiding the command prefix). - Round 2: both found more ways around the narrowing, and file cards showing the whole screen. That led to restoring main's id. - Round 3: both APPROVE with no HIGH or MEDIUM. - After the rebase (one import conflict in `server/pi.ts`, where main's #1899 added `fileURLToPath`): Codex (gpt-6.1-sol, xhigh) reviewed the rebase against the new main (#1899's store and agent directory code, #1893's cache ordering, #1890's dialog reader order) and APPROVED with no findings. - `bun run check fast` on this head: workflow syntax, generated types and themes, typecheck, build and unit tests ok (6800 pass, 0 fail, 13 skip). --------- Co-authored-by: mp <mp@local>
This was referenced Oct 27, 2026
This was referenced Oct 28, 2026
devswha
added a commit
that referenced
this pull request
Oct 28, 2026
…d drop #1548's duplicate paths (#2200) ## Change The three items from #1548 that were still open on main: - **OpenCode cleanup on stop (item 3).** A stopped server now also releases its OpenCode event stream, the panes it verified, and their send watches, as it already did for Codex's app-server connections. That state is shared by every server in the process, so `createServer` holds it (`holdOpenCodeNative` in `server/opencode-native.ts`) and `stop()` releases it. Only the last server to stop resets it, so a server that is still running keeps its stream and committed watches. After that reset, a verification still on its way (a request that was mid-flight at stop) opens no stream until a server holds the state again or an explicit reset starts over. - **SessionHistory (item 1).** `src/components/SessionHistory.tsx` calls the tested `recentlyResumedPane` instead of repeating its lookup. Behaviour is unchanged. - **Test-only export (item 2).** `hermesResults` and its `HermesResult` type had no production caller and are gone. `server/dialogs/hermes.test.ts` checks those screens through the registered result readers' `parse`, the path the server uses. The rest of #1548 was already fixed: items 4 and 5 by #1725, and the other two exports in item 2 (`claudeRewindOf`, `claudeRewindOnScreen`) by #1890. Fixes #1548 - **Contract test.** `server/opencode.contract.test.ts` (from #1489) stopped the only server and expected the next one to inherit the pane the first one verified. That is the state item 3 releases, so its replacement server now starts before the old one stops. Its collector still learns the pane verified before it started, and ends it. Production runs one server per process, so nothing else depends on the state surviving a stop. ## Validation - `server/opencode-native-stop.test.ts` (new, `createServer` on port 0 with a mkdtemp `stateDir`, a stand-in OpenCode v2 service): - A stopped server closes the stream at the service. - A second server's stream and panes survive the first one's stop, and a second `stop()` of the same server releases nothing more. - A verification after the last stop opens no stream; a new server verifies again. - The first case fails on main. Removing the holder count fails the second case, and removing the late check fails the third. - `bun test server/opencode-native-stop.test.ts server/opencode-native.test.ts server/dialogs/hermes.test.ts src/lib/sessionHistory.test.ts`: 72 pass. `bun run typecheck` is clean. - The Hermes help-cut assertion now goes through the reader: dropping the help layout's frame check makes it fail. - `bun scripts/session-history-browser-qa.ts`: all 15 checks pass, including Open (not Resume) for a pane resumed a moment ago. - Every unit file in one bun process (`UNIT_TEST_WORKERS=1`, as native Windows runs them), so that state left by one file reaches the next: - 7957 pass and 1 fail. The failure is `server/core/pending.test.ts` "pending cap" (Pi, no OpenCode), which timed out at 5.1 s under load and passes alone in 1.3 s. - `bun run check fast`: green. - `server/opencode.contract.test.ts` on an isolated herdr test session: 24 pass. Server code the remote bundle carries changed; `REMOTE_BUNDLE_VERSION` is left to the release PR, as docs/development.md says. ## Review - **omo-native-code-reviewer** on 411ef56: PASS. Its LOW-1 and LOW-3 were the shared-state and late-stream issues below. - **Codex Sol 6.1 Fast xhigh** on 411ef56 found two MEDIUMs: stopping one server reset state that another running server shares, and a request mid-flight at stop could open a new stream afterwards. Both are fixed in ee883e8. Codex's re-review of that delta: PASS, no findings, with its harness covering 9 lifecycle cases.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #1582
Fixes #1437
Two follow-ups grouped because both touch
server/prompt.ts: one commit per issue.#1582 (rewind follow-ups)
claude-rewinds.jsonfrom its state directory on start. Only the unreleased Chat rewind (feat(chat): rewind to an earlier message, and the chat follows #1500) wrote it, and nothing has read it since chat: rewind through the terminal for every agent #1577. A failure is logged, and the server still starts.claude-rewind-2.1.304-select-banana-40.txt, deleted in chat: rewind through the terminal for every agent #1577) is back.dialogs/claude.test.tschecks it holds Chat sends without offering a card.core/claude-rewind-send.test.tsrefuses a send on it and types nothing.PaneTerminal.css's banner comment now names Claude Code too.chatOwnedInputonly returnedlastChatSubmitted, so its two callers now call that directly.#1437 (dialog reader registry)
registry.test.tslists, per agent, the order its screen is offered to the readers. Swapping two priorities now fails a test (mutation-checked:claude-questionpriority 10 to 25 fails). The snapshot comment now cites3df0824e, refactor(prompt): register dialog readers per agent and answer pickers through interaction plans #1435's base.InteractionPlan.commit(answer)andterminalFallback(answer)read the chosen answer's own key sequence, so the loop executes the plan for the answer it validated. Mutating back tosequences[0]fails the new test.InteractionPlan.identity.ReaderContext.fallback: nothing set it, so the three fallback readers never parsed anything. They now only lend their answer behaviour (reread), andparseFallbackPromptstill draws those cards.modelListWaits: the fallback card's model-list guard callsregistry.waits, the same Claude-model, Claude-effort and Codex-model check. Tests call it directly.parsePromptreturns at the first reader that reads the screen instead of running every reader.onePerPasskeyed by the screen and that pass's context object: Claude's approval, Hermes' clarify, pi's dialog and OmO's forms. The other parsers run at most once each, because the pass stops at the first reader that reads the screen.ReaderContextis readonly, so a caller cannot change the context mid-pass and get a cached result.picker-asking-before-commit.mdfragment from the issue comment was already folded into 0.4.13 (chore(release): 0.4.13 #1640), so there is no fragment left to drop.Verification
bun testof the five changed test files: 96 pass.server/prompt.test.ts,server/dialogs,server/core,outcome,pane-routesandconversation: 828 pass, 0 fail.sequences[0];bun run typecheck: passes on each commit.omo-native-code-reviewerapproved with no HIGH or MEDIUM findings; its readonly-context LOW is the third commit. Codex (gpt-6.1-sol, xhigh) found no HIGH, MEDIUM or LOW: the early return matched the old eager evaluation in 1,760 comparisons over 80 fixtures, andregistry.waitsmatched the removed condition.bun run check fast(throughdori heavy) on c707e82:status: success, with a clean tree and every step ok (typecheck, build, unit tests, generated types and themes, workflow syntax).