Repository navigation
Conversation
|
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 11 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. |
Review of #2888. Suspended with Ctrl+Z, Hermes leaves its shell in front while herdr still names the pane hermes: a program there that showed the panel's text got a card whose keys went to that program. The card is now offered only while Hermes is the pane's foreground process, checked again on every read the answer makes before a key. The generic card was still offered over an approval in another language or a pane too narrow for the hint, since the check needed the English hint. It now goes by the `⚠` Hermes puts on its input line while any of its modals holds it, in every language and with a draft typed there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review findings and how each was resolvedAn adversarial Codex review (gpt-6.1-terra, xhigh, read-only) ran on head 1. HIGH: a program can show the panel's text while it owns the pane. Fixed.
Fix:
Verified:
Not reproducible:
2. HIGH: the generic card could still reach an approval the reader leaves out. Fixed.
Reproduced with unit variants built from Fix: the check now goes by the 3. HIGH (as reported): what is drawn is lossy (tabs expanded, wraps shown as line breaks, trailing spaces gone), so two commands can draw the same and share a card id. Not changed. I'd rate it LOW for this PR.
A user reading the terminal can't tell such commands apart either. The card's id changes whenever the drawn command or warning changes, and every key is checked against it. A lossless source would need Hermes to expose the pending approval itself. That is the kind of agent-specific endpoint #857 rules out; it could be a later Hermes-side request. Also found in self-review and fixed in Not run here: the omo review. Its CLI isn't on this machine, so the lead is running it. |
…card Hermes's classic CLI (0.21.5, its default interface) draws a boxed approval panel over its input. No reader knew it, so Chat showed the generic "Waiting for input" card: the screen dumped into its body, raw arrow, Enter and Esc keys (Enter takes the highlighted Allow once), and an id that changed with every tick of the panel's countdown. A reader now turns the panel into an approval card: the command as the panel wraps it, Hermes's warning, and its own choices, answered by moving to the row and pressing Enter (never a digit, which picks and confirms at once). It reads only a complete, English panel that holds the input: a command or description cut or dropped, a box out of line, other labels, or anything under it but Hermes's own lines is no card, and registry.waits keeps the generic card off it too, so the terminal answers it. Fixes #857 Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Review of #2888. Suspended with Ctrl+Z, Hermes leaves its shell in front while herdr still names the pane hermes: a program there that showed the panel's text got a card whose keys went to that program. The card is now offered only while Hermes is the pane's foreground process, checked again on every read the answer makes before a key. The generic card was still offered over an approval in another language or a pane too narrow for the hint, since the check needed the English hint. It now goes by the `⚠` Hermes puts on its input line while any of its modals holds it, in every language and with a draft typed there. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
ca50c38 to
2e359ce
Compare
Deploying ovis with
|
| Latest commit: |
64f4092
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://59d6ca82.ovis.pages.dev |
| Branch Preview URL: | https://lane-issue-857.ovis.pages.dev |
Merge noteMerged origin/main ( Validation: CodeRabbit did not review this PR (rate limit, see its "Review limit reached" comment); the Codex review and its fix round above stand in for it. No review threads are open. |
…oval key's settling read Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Final-head review roundMerged origin/main ( Adversarial Codex review (gpt-6.1-terra, xhigh, read-only) at 1. The foreground look runs before the read that settles each key. Reproduced; the attempted fix was dropped; the order is documented (
2. On Windows,
3.
Validation at No review threads were open. CodeRabbit ran no review on this PR (its "Review limit reached" comment). |
|
Parking as a draft. One HIGH is reproduced: if Hermes is suspended (Ctrl+Z) at just the wrong moment, one answer key can reach a fresh shell prompt. The attempted fix made it worse (a moved cursor could put the Enter on a different row, Allow instead of Deny), so it was dropped. An approval answer must never be able to land on the wrong row or process; this comes back with a design that confirms the Hermes prompt is still in front at the moment of sending (or answers through a Hermes API, if one exists). |
Summary
Chat now answers Hermes's dangerous-command approval with the existing approval card. Before this, Chat showed the generic "Waiting for input" card.
Hermes's classic CLI is its default interface (0.21.5). It draws the approval as a boxed panel over its input. No reader recognized that panel, so a blocked Hermes pane fell through to the generic fallback card:
↑,↓,Enter,Esc(Enter takes whichever row is highlighted, which is "Allow once" when the panel opens);A new reader in
server/dialogs/hermes.tsturns the panel into an approval card:↑/↓, then pressesEnter. A digit is never sent, because in Hermes a digit selects and confirms at once. The existing answer path re-reads the screen before the Enter and goes on only when the card id is unchanged and the cursor is on the chosen row; otherwise it returns 409prompt_changed. The card id doesn't include the cursor or the ticking countdown.The card is offered only while Hermes is the pane's foreground process (
pane.process_info). When Hermes is suspended with Ctrl+Z, herdr still names the panehermeswhile a shell holds it. Every read the answer makes before a key repeats this check. On Windows, which lists no foreground process, it reads as before, the same as omp and pi.It fails closed. These get no card, so the Terminal answers:
… (choose Show full command),… (command truncated — …));… (description truncated)), or dropped without a marker on a short pane;⚠input line it holds. That covers text typed into the input, an extra widget, and output under the input.None of them get the generic card either.
registry.waitsis true whenever Hermes's input line starts with the⚠Hermes puts there while one of its modals holds the input. That mark is hard-coded, so this works in every language, with a draft typed in, with a hint cut by a pane narrower than 33 columns, and for a command's own confirmation (/clearand the like). That card was unusable anyway: the countdown on the hint changes its id every second. For the same reason, a Chat send is refused withagent_blockedwhile such a modal holds the input; herdr'sblockedstatus mostly did that already.Scope, for the lead to confirm (the issue asks that the supported layouts be agreed):
hermes --tui) draws a different approval box and has no⚠input line, so it still gets the generic card.kind: "approval"InteractivePrompt; only a server-internal responder id is added.Thanks to @RikiSanjayaa for the request and the clear safety requirements in #857.
Fixes #857
Repro (origin/main 50c348b)
Real Hermes 0.21.5 ran in an isolated setup:
lane857under its ownXDG_CONFIG_HOME;mkdtempHERMES_HOMEwithapprovals.mode: manual;terminal→rm -rf ./scratch;mkdtempstate dir on127.0.0.1:0.herdr reported the pane
blocked.GET /api/pane/promptreturnedkind: "menu",fallback: true, options↑ ↓ Enter Esc. Three polls 1.5 s apart gave three different ids (a35c3cc0446f,ce8a5c2da848,f055f34631ab). The server loggedis blocked on a screen no reader knows; fallback card.After (same setup, this branch)
fb03ee811b40three times).POST /api/pane/prompt/answer:→ denied.→ allowed once, and./scratchwas removed.→ allowed for session.→ denied.All shots come from the isolated test session.
Tests
server/dialogs/hermes-approval.test.tsruns on nine screens recorded from the real panes, inserver/fixtures/hermes-approval/, plus variants derived from them for each refusal above and for the shorter menus. These tests failed on main (7 of 10 at the time; the other 3 assert "no card", which main already met).server/prompt.test.tsadds a route-level test withwithPane:down down down enter, each move seen before the Enter;It failed on main, which returned
kind: "menu". Each later case also failed before its fix.server/dialogs/registry.test.tssnapshot: the new reader added.Validation
bun run check fastpassed on the pushed head2e359ce1, rebased on maine6977348: workflow syntax, generated types and themes, typecheck, build, and unit tests (11623 pass, 0 fail, 46 skip).src/lib/i18npart files over their line cap (main then, fixed by fix(i18n): keep settings part files under the line cap #2886), andserver/pending-input.contract.test.ts"checks a new blocked state after the typed ESC's settling gap", a Claude-agent timing test this change doesn't reach. After the rebase, that test passes locally on the isolated test herdr (bun run check run bun test ./server/pending-input.contract.test.ts -t "settling gap", 2 pass).agent: hermes), withcat <panel>; sleepin its shell, got no card. On main it got a full approval card. The real pending approval in another pane still got its card.64f40926(origin/main0c414311merged in as707926d3, plus one comment-only commit):bun run check fastpassed (unit tests 11922 pass, 0 fail, 46 skip; workflow syntax, generated types and themes, typecheck, build ok).bun test server/dialogs/ server/hermes*.test.ts server/prompt.test.ts server/question-steps.test.ts. All pass except one test:server/hermes.contract.test.ts"selects the pane process's store…" (Hermes stand-in did not become the pane's foreground process). It fails the same way with this change stashed, so it's an environment issue on this host and not part ofcheck fast.bun run check full, and the Windows and Mac checks. Nothing here is platform-specific: one server-side parser and the fallback gate.Merge danger
Door: two-way. No state, setting or contract change.
Blast radius: Hermes panes whose input a modal holds (an approval or a command's confirmation). A screen the reader doesn't fully recognize now gets no card, where it used to get the generic card, which flickered and couldn't be answered. One more herdr
pane.process_infocall is made per read of a Hermes approval card. Ink-TUI Hermes and every other agent are unchanged.🤖 Generated with Claude Code