fix(chat): follow-ups from the agent command forms review by devswha · Pull Request #1893 · devswha/ovis

devswha · 2026-10-24T15:06:53Z

Change

Follow-ups from #1441's review (#1444). Per item:

Fixed

M1, a failed command-list read left the menu and the terminal handoff empty (src/lib/usePaneCommands.ts): a failed read now keeps the list the pane already had and asks again after 2, 5 and 15 s, then every 30 s while the pane is open. Before, the list stayed empty until the pane, agent or PC changed. When a retry and a refresh overlap, only the read started last writes the shared cache, so an older answer cannot replace a newer list (found by the merge pass).
M3, pi's argument forms (server/skills/pi.ts, server/outcome.ts): { sub: "*" } is not a form the protocol defines. The client read it as the literal subcommand * and fell back to the bare form, so /name Release 1 was taken for a terminal screen. These forms now use "other", which the protocol and the client already define for any other argument. The server's outcome matcher no longer reads "*", and no catalog emits it any more.
M4, unused contract pieces: CommandOutcome, outcomes and invalidatePaneCommands are in use since feat(chat): show what became of a sent command, from evidence only #1450 (GET /api/pane/prompt returns outcomes, and ChatView invalidates the pane's commands on catalog_changed). Only PendingMessage.outcome_id has no producer and no reader. Its removal goes with the Native catalogs: Tab race after the fresh read, after-Tab check misses, cold scan outside the budget (#1489 follow-ups) #1493 PR, which also edits shared/protocol.ts, so two open PRs of mine never touch the same file.
M6, the first omo-looking process won (server/conversation.ts paneProcessRoots): an omo pane now binds to the omo process that holds a session. Before, an unheld one listed earlier won, such as omo's bundled Claude SDK binary under /omo-ai/ or a helper script. With no held process the first one still answers, unheld, so nothing new skips Claude's draft refusal.
LOW, held and the start time (server/omo.ts): omoProcessIdentity now reports held (a holder record and a readable start), so the start is read once per lookup, not twice (one ps fewer on macOS). There is a test for held: false when the start cannot be read.
LOW, the access check after a Hermes pane's process lookup now has a test (removing that check fails it). The one case in server/core/omo-pane-send.test.ts that needs no /proc now runs on every platform instead of Linux only.
LOW, OpenCode v2 pin (server/skills/opencode.ts, server/commands.ts): a source may name one pin per TUI. OpenCode names 1.19.3 and 2.0.32, so on a v2 pane the builtins both TUIs share are verified too.
LOW, Claude skill echoes (server/skill-invocations/claude.ts): only Claude's own commands are left out now. A bundled skill (code-review, debug, loop, simplify...), or a user's skill named like one, is recorded as skill activity and shows in Chat with its skill chip. A user's skill named like one of Claude's commands (review, init) is still not recorded: the echo alone cannot tell it from the command.
LOW, server/pane-routes.test.ts compares /quit's description across the omo and pi catalogs instead of pinning "Quit omo".
LOW, O_NOCTTY lands in the Command Code discovery: bound the work per folder, and follow-ups from #1449's review #1452 PR, which owns server/skills/walk.ts.

Not changed, and why

M2, draft confirmations went back through the terminal guard: that guard is gone since feat(chat): hand terminal commands to the terminal lens #1580. A terminal command now goes out through the normal send, and the terminal peek opens on its receipt. send intercepts only /resume, and "Clear it and send" and "Send anyway" call send directly.
M5, the convention test: terminalBound in server/skills/claude.ts is where the catalog design keeps each agent's terminal screens (AgentCommandSource.terminalBound). It is not a second list beside a source of truth. A semantic duplicate check would flag src/lib/terminalPeek.ts's per-agent screen-return lists (feat(chat): open terminal commands in a peek over the chat #1630, feat(chat): return the terminal peek to Chat from each agent's input row, and hold sends while a screen is open #1637), which are intentional.
M7, fallback refusals for real omo panes without a holder (--no-session, another project's folder, a deleted cwd, Windows): without a holder record an omo process cannot be told from its own Claude SDK child, so those panes keep Claude's draft checks. That fails safe.
LOW, agentPrompt's access check: the guard is passed into the herdr RPC and checked once connected, right before agent.prompt is written (server/herdr/client.ts).
LOW, the FIFO test reads /etc/codex/skills: read-only, and the assertion is toContain, so a host's own machine-wide skills cannot fail it.
LOW, Codex prompts hidden when the version is unknown: an unreadable version means a fresh session or a Codex whose rollout has no cli_version. Showing removed prompts on a modern Codex would offer rows that do nothing.
LOW, terminal-only commands before the catalog loads and through quick replies: since feat(chat): hand terminal commands to the terminal lens #1580 these go through the normal send by design. The peek reads the catalog when the receipt arrives, so a list that loads after the send still opens it.
LOW, a cached old client: the app shell revalidates on load (server/AGENTS.md), so the client catches up on the next load.

Fixes #1444

Verification

Each new test fails on origin/main (production code from main, tests from this branch) and passes here:
- src/lib/usePaneCommands.hook.test.ts (React test renderer, fake timers)
- server/skills/pi.test.ts "tells pi's bare commands from the ones given an argument"
- server/conversation.test.ts "binds an omo pane to the omo process holding a session..."
- server/omo.test.ts "counts an omo's holder as held only while the process's start is readable"
- server/skills/opencode.test.ts "verifies the builtins both TUIs share on a v2 pane as on a v1 one"
- server/skill-invocations/claude.test.ts "records a bundled skill's echo..."
The Hermes access test fails when the inTime() after the Hermes lookup is removed.
Browser, on the demo app with /api/pane/commands failing and then recovering, at 1280 and 390 px: this branch asks again and the / menu lists /config with its TERMINAL badge; main never asks again and the menu stays empty.
Reviews: omo-native-code-reviewer APPROVE, 5 LOW. I removed the unused claudeBuiltinNames and strengthened the hook test; the rest are covered above. Codex (gpt-6.1-sol, xhigh) APPROVE, 2 LOW. I made the held test independent of a readable host start (Windows); the /review alias is the limit stated under Claude skill echoes.
bun run check fast on 452f3f9: workflow syntax, generated files, typecheck, build and unit tests (6413 pass, 0 fail) all ok.

A failed read of the pane's command list left it empty until the pane, agent or PC changed, so the menu showed nothing and a terminal command sent from Chat got no terminal handoff. A failed read now keeps the list the pane had and asks again after 2, 5, 15 and then every 30 seconds. Refs #1444

pi's /model, /thinking, /name, /login, /import, /export and /bug forms for an argument used { sub: "*" }, which the protocol does not define: the client read it as the literal subcommand "*", fell back to the bare form, and so took /name Release 1 for a terminal screen. They now use "other", the form the protocol defines for any other argument, and the server's outcome matcher drops its own reading of "*". Refs #1444

paneProcessRoots took the first omo-looking foreground process, held or not, so an unheld one listed before the real omo (omo's bundled Claude SDK binary under /omo-ai/, a helper script) made typing fall back to herdr's label. It now prefers the process that holds a session and keeps the first one only when none does. omoProcessIdentity says whether its holder counts (held: a holder and a readable start), so the lookup no longer reads the start a second time (a second ps on macOS). Refs #1444

… too OpenCode's source had one pin, 1.19.3, so on a v2 pane the builtins both TUIs share were always listed as unverified while the v2-only extras verified against 2.0.32. A source may now name one pin per TUI, and OpenCode names both.\n\nRefs #1444

Claude's command echoes were dropped for every name in Claude's reference, its bundled skills (code-review, debug, loop, simplify...) included, so neither those skills nor a skill of the user's own named like one showed as skill activity. Only Claude's own commands are left out now.\n\nRefs #1444

…d of pinning its words Refs #1444

…lookup on every platform The Linux-only omo block also held a case that needs no /proc, so it never ran on macOS; it runs everywhere now, beside a new case for the access check right after a Hermes pane's process lookup (removing that check fails it). Refs #1444

Refs #1444

coderabbitai · 2026-10-24T15:07:03Z

Warning

Review limit reached

You'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 28 minutes.

Check out review usage here.

View limit details

Limit details: You’ve used all 4 included reviews currently available.

Learn how review limits work.

Review configuration:

Run configuration

Source mode: Auto
Repository: devswha/ovis
Branch: fix/command-forms-followups
File: .coderabbit.yaml
Commit: 452f3f9
Layers: Repository YAML
Review profile: CHILL
Plan: Advanced
Run ID: dc24d620-2147-47bc-b49e-69d3b553541d

Commits

Reviewing files that changed from the base of the PR and between 4c8ce30 and 452f3f9.

Files selected for processing: 25

changelog.d/command-forms-followups.md
server/catalog.conventions.test.ts
server/commands.ts
server/conversation.test.ts
server/conversation.ts
server/core/omo-pane-send.test.ts
server/core/outcome-delivery.test.ts
server/omo-identity.test.ts
server/omo.test.ts
server/omo.ts
server/outcome.test.ts
server/outcome.ts
server/pane-routes.test.ts
server/skill-invocations/claude.test.ts
server/skill-invocations/claude.ts
server/skills/claude.test.ts
server/skills/claude.ts
server/skills/index.ts
server/skills/opencode.test.ts
server/skills/opencode.ts
server/skills/pi.test.ts
server/skills/pi.ts
src/lib/paneCommandCache.ts
src/lib/usePaneCommands.hook.test.ts
src/lib/usePaneCommands.ts

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.

❤️ Share

_{Comment @coderabbitai help to get the list of available commands.}

devswha · 2026-10-24T15:12:15Z

CodeRabbit hit its review limit on this PR, so here are the agent reviews and how each point was resolved:

omo-native-code-reviewer: APPROVE, 5 LOW notes:

After the fourth failed read, the command-list retry keeps asking every 30 s while the pane is open, even for an error that will not change. Kept: it fails safe, and the read is the same cheap one the menu already makes.
claudeBuiltinNames became unused. It is removed, and its test now covers claudeCommandNames.
A user's skill named like one of Claude's own commands (review, init) is still not recorded as skill activity. The PR body states this limit: the echo alone cannot tell it from the command.
O_NOCTTY has no disposition here. It lands in the Command Code discovery: bound the work per folder, and follow-ups from #1449's review #1452 PR, which owns server/skills/walk.ts.
The second hook test was thin. It now also checks that a failed read after a successful one is retried.

Codex (gpt-6.1-sol, xhigh reasoning): APPROVE, 2 LOW notes:

Same as LOW 3 above.
The new held test assumed a readable process start, which Windows does not have. It now mocks the start from the beginning, so it reads the same on every platform.

Both reviewers checked the security role of the held-omo choice. Only typingRoots skips Claude's draft refusal, and it needs held, which requires a holder record plus a readable start. So an unheld omo-looking process still gets Claude's checks.

devswha · 2026-10-24T15:18:07Z

Merge pass gate review of 914065a: one blocker (MEDIUM). An older retry's answer can overwrite the newer list in the shared command cache.

Reproduced with fake timers and controlled responses

The first read fails and schedules a retry.
An invalidation (catalog_changed) starts another read.
The retry timer then fires and starts a second, overlapping read.
The newer read answers with new-list, and after it the older one answers with old-list.
The mounted hook keeps new-list, but a remount returns the cached old-list without asking again.

Code: src/lib/usePaneCommands.ts:21-37 and src/lib/paneCommandCache.ts:15-25.

Fix: clear the pending retry when an invalidation or any new read starts, or give each read a sequence number and let only the newest one write to the cache. Please add a regression test that remounts after the out-of-order answers and expects the newer list.

Holds

Recovery after a failed read works, and the retries are cancelled when the pane, agent or PC changes, when the pane closes and on unmount.
A successful read resets the backoff, and the retries stay GET-only.
pi's "other" forms round-trip as commands. Putting pi's old "*" back makes the regression fail.
The hook tests and the pi round-trip test pass.

The merge pass re-checks the fix commit, looking only at the changed range.

…wer out of order The merge pass found that after a failed read, a catalog change and the pending retry can start two overlapping reads of a pane's commands. When the later read answered first, the earlier one's answer then overwrote the shared cache, so a remount showed the older list without asking again. Each read now has a sequence number, and only the read started last for that pane and agent writes the cache. Refs #1444

devswha · 2026-10-24T15:26:00Z

Fixed in 452f3f9: the out-of-order retry and refresh.

src/lib/paneCommandCache.ts: each read of a pane's commands gets a sequence number, and only the read started last for that pane and agent writes the shared cache. The other answers are still returned to the caller that asked. Invalidation keeps its generation check as before.
New regression test in src/lib/usePaneCommands.hook.test.ts, "keeps the newer list when a retry and a refresh answer out of order". It follows your steps: a failed read, then an invalidation read, then the retry's read; the later read answers new-list first and the earlier one old-list after it. After a remount it expects new-list with no new fetch. Without the fix it fails, receiving old-list; with it, it passes.
bun run check fast on 452f3f9: workflow syntax, generated files, typecheck, build and unit tests (6413 pass, 0 fail) all ok.

The earlier Integration tests failure was infrastructure: "The self-hosted runner lost communication with the server" on herdr-ci-...-P620-3. This push reruns it.

devswha · 2026-10-24T20:32:26Z

Merge pass review-work report. Overall: PASSED.

Lane	Verdict	Confidence
Gate review of `914065a` (a different model; programming + remove-ai-slops applied directly)	REJECT: a stale retry answer could overwrite the shared command cache	HIGH
Merge pass re-check of `452f3f9` (the fix)	PASS	-

The fix in 452f3f9

cachedPaneCommands numbers each read for its pane and agent. Only the read started last writes to the cache. An older read that answers after it is returned, but never cached. This covers a retry running beside a refresh.
Mutant: with the latest-read guard removed, the new test "keeps the newer list when a retry and a refresh answer out of order" fails.
The hook and conventions tests pass (11), and so does the typecheck.

Held from the gate

Recovery after a failed read works, with backoff 2 s, 5 s, 15 s, then 30 s.
Retries are cancelled when the pane, agent or PC changes, on close and on unmount.
A successful read resets the backoff.
pi's "other" forms round-trip as commands, and the old "*" is gone from catalogs and from the matcher.

All required checks are green and no review thread is open.

CodeRabbit did not review this PR (rate-limited or skipped). The reviews above stand in for it.

## 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>

mp added 8 commits October 24, 2026 23:51

test(pane-routes): compare /quit's description across catalogs instea…

3e6af9a

…d of pinning its words Refs #1444

docs(changelog): note the command forms follow-ups

914065a

Refs #1444

devswha mentioned this pull request Oct 24, 2026

fix(chat): follow claude attach panes with options, an id prefix or a job name #1897

Merged

devswha merged commit aa0d182 into main Oct 24, 2026
34 of 36 checks passed

devswha deleted the fix/command-forms-followups branch October 24, 2026 20:32

devswha mentioned this pull request Oct 25, 2026

fix(cmd): follow-ups from the Command Code support review #1934

Merged

Navigation Menu

Repository navigation

fix(chat): follow-ups from the agent command forms review - #1893

fix(chat): follow-ups from the agent command forms review#1893
devswha merged 9 commits into
mainfrom
fix/command-forms-followups

devswha commented Oct 24, 2026 •

edited

Loading

Uh oh!

coderabbitai Bot commented Oct 24, 2026 •

edited

Loading

Review limit reached

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

devswha commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Change

Verification

Uh oh!

coderabbitai Bot commented Oct 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review limit reached

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

devswha commented Oct 24, 2026

Uh oh!

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

devswha commented Oct 24, 2026 •

edited

Loading

coderabbitai Bot commented Oct 24, 2026 •

edited

Loading