Repository navigation
fix(prompt): keep an omp Ask card above omp's status row - #1812
Conversation
|
Important Review skippedAuto incremental reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the
|
| Layer / File(s) | Summary |
|---|---|
Recognize and filter OMP status rows server/dialogs/live.ts, changelog.d/omp-ask-status-row.md |
The parser removes the final visible row for omp-question and omp-submit prompts only when the row matches the OMP status pattern and the preceding nonempty row matches the panel border. The changelog describes the Ask card and status-row placement. |
Validate OMP Ask panel parsing server/dialogs/omp.test.ts, server/fixtures/omp-ask-18.12.6-* |
Tests and terminal fixtures cover active and settled Ask panels. Tests also check that intervening output or a missing panel border prevents parsing the card. |
Priority:
Estimated code review effort: 2 (Simple) | ~10 minutes
Change: Bug fix
Possibly related issues
- Issue 1644 identifies the OMP Ask card failure addressed by this change: the parser now keeps the card when the status row follows the panel border.
Possibly related PRs
- devswha/ovis#1761: This PR moved
promptTailIsActiveintoserver/dialogs/live.ts, where this change adds OMP-specific filtering.
Merge Risk: 
Minimal · up to 4b1ea
-
The parser change and production-path tests align with the stated behavior and are ready to merge subject to normal checks.
Pre-merge checks: 
4 · 
1
-
Failed checks: 1 warning Passed checks: 4 passed-
Full details: Description check
-
Explanation
The description explains the problem, resulting behavior, scope, fixtures, regression cases, and validation results. It uses Summary and Verification headings instead of the template's Change and Validation headings, but it provides the required information.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 2 functions across 2 files. (6 skipped: 6 unsupported.)
Resolution
Write docstrings for the functions missing them to satisfy the coverage threshold.
-
Finishing Touches: 💡 1
-
📝 Generate docstrings: 💡
-
- Commit to this branch
- Create a new PR
🧪 Generate unit tests (beta)
-
- 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.
|
Merge pass review-work report for 4b1ea28. Overall: FAILED. There is one blocker: a stale Ask card can come back.
Blocking issueA different, empty box under an old Ask panel revives the stale card. Reproduction: take a recorded waiting screen and remove its status row. Append a separate empty box ( With the resting Fix: tie the accepted bottom border to the detected Ask panel in the unfiltered screen, so it must be that panel's own What passed
Push the fix and the merge pass will re-run the affected rows with a fresh reviewer. |
|
Re: merge-pass report. Fixed in 4ed3223.
|
|
Confirmed the #1812 blocker is fixed on current head |
|
Merge pass: review-work report. Overall: PASSED.
B1 and its fix. An old Ask panel, then a separate empty box, then omp's status row brought the stale card back with its original id. The merge pass reproduced it. 4ed3223 requires the border to sit directly under the panel's last shown row ( Re-check at 4ed3223.
Notes (follow-ups).
All required checks are green and no review thread is open. |
…ips (#1814) Refs #1644 (the clipped multi-select hint noted there after #1812). ## Summary - `server/dialogs/omp.ts`: at 60 columns, omp 18.12.6 clips the multi-select Ask hint inside its cancel label: `␣ toggle · ⏎ next · ↑/↓ move · ⇥/←/→ question · ⎋ cance…`. `OMP_MULTI_HINT_RE` needs the whole `cancel`, so the question gave no card. - The new `OMP_MULTI_CLIPPED_HINT_RE` accepts that hint: toggle, then submit or next, then move, then Esc, with the start of "cancel" or nothing, ending in `…` or `...`. - `parseOmpQuestion` uses it to find the hint and to mark the card multi-select. - `server/dialogs/live.ts`: `promptTailIsActive` accepts the same clipped hint for `omp-question`, so the card stays while the question waits. - `server/fixtures/omp-ask-18.12.6-multi-60.txt`: the real 60-column multi-select screen. It comes from the same recording as #1812's fixtures (omp 18.12.6 with `composer.shape: pi`, isolated herdr test session, fake model provider). - `server/dialogs/omp.test.ts`: that screen gives the multi-select card (question, three options, `multi_select: true`), the same card it gives without its status row. - `changelog.d/omp-ask-clipped-multi-hint.md`. Part 2 of #1644 (Markdown in the question and options) is still a separate follow-up. It needs `shared/protocol.ts`, the remote PC bundle and a `REMOTE_BUNDLE_VERSION` bump. `server/dialogs/*.ts` is part of the remote PC bundle; the release process refreshes it (`docs/development.md`). ## Verification - On main the new test fails: the 60-column multi-select screen gives no card. It passes with this change. - All 24 recorded screens (omp 18.8.7 and 18.12.6, 120 and 60 columns, text and ANSI) replayed: - every multi-select screen gives the same card (`multi_select: true`); - answered screens give none; - single-select and review screens are unchanged. - `server/dialogs/`, `server/prompt.test.ts`, `server/core/omp-submit.test.ts`, `server/conventions.test.ts`: 507/507. Typecheck. `bun run check fast`.
…#2220) Closes #1644 (part 2; part 1, the card under omp's status row, was #1812). Thanks @CharleoYang, whose #1528 found both parts and first matched the live menu to the open `ask` call. ## What it does for the user An omp Ask card now shows the question and its options in the agent's own Markdown: bold, italics, code, strikethrough, lists, a fenced code block, and links in the question. A question that omp cuts at four rows with `…` shows in full on the card. When the card cannot prove which call is on screen, it shows the screen's text exactly as before, and the terminal is still one click away. ## How - `server/omp-ask.ts` `ompMarkdownPrompt`: the session's one open `ask` call (`ompAsksAfter` in `server/omo-ask.ts`) gives the card its text only when the call is provably the menu on screen. One of its questions, with every option's label and description in order, must be drawn exactly as the screen shows it. - The call's Markdown is read by a CommonMark parser, as omp's own is: `Bun.markdown.render`, built into the Bun the app pins (1.4.2). Its callbacks follow omp's renderer (pi-tui `ask-dialog.ts` and `components/markdown.ts`, omp 18.12.6 to 18.13.2): - emphasis, strikethrough and code show their text, and an unpaired or escaped mark (`*.log`, `_cache/`, `\*`) stays; - a link shows its words, and in the question its address after them; - list items take omp's `- ` or `N. `, and a heading of level 3 or more keeps its `#`s. A table, quote, rule, HTML block or task list, or a block inside a label, leaves the card as drawn. So do an HTML entity and a pair of single tildes (`~x~`): Bun decodes and strikes them, but omp draws them as written. - The card draws the same characters: the chat's own Markdown scanner (now `shared/markdown.ts`, which `PromptCard` renders with) is not CommonMark. It reads `Keep *.log and *.tmp` as emphasis and keeps the backslash of an escaped mark. So a field is Markdown only when that scanner draws exactly the text omp drew; otherwise the card stays as drawn. A picture always keeps the screen's card: omp draws its alt text, the card the picture or its address. - The screen's text keeps every character, and every run of spaces counts as one (omp wraps and pads its rows). So a difference of one `/`, `*`, `_`, a space, letter case or a punctuation mark leaves the card as drawn. A question may match by its start only when omp says it cut it: the question fills its four rows and omp's footer offers `<key> expand` (pi-tui `#expandHint`). At a width where the footer itself is cut, the card stays as drawn. - It also fails closed for: - no open call, or more than one; - no matching question, or more than one; - an answered call or a later turn; - an option with a preview (omp draws it into the option's rows); - a description omp cut at its two rows. - The card keeps the screen's id and answer keys, so POST `/api/pane/prompt/answer` still answers the menu on screen. Only the chat's own read (`readPrompt`) asks the session; an answer's re-reads do not. - New optional field `InteractivePrompt.text_format: "markdown"` (`shared/protocol.ts`). An older client shows the raw Markdown text. - `src/components/PromptCard.tsx` draws the question with the chat's Markdown renderer. Labels and descriptions use a new `InlineMarkdown`, which has no links or pictures, since a row answers when pressed. - `src/lib/promptText.ts` `optionName` is the one reading of an option's name. An omp Markdown card shows its label whole (a link to `recommended` is never the Recommended tag), and its words are `shownText` (`shared/markdown.ts`), which the server's check against omp's screen uses too. - `src/lib/promptAnswer.ts`: a message typed in the chat names an option of a Markdown card only by the words the card shows, with spacing collapsed as the page does (``Use `local` socket`` is "use local socket"). One option's raw Markdown can be the words another option shows. Words that name two options pick neither, on any card. - `src/lib/promptText.ts` `polledPrompt` (used by ChatView's poll): a card with the same id takes Markdown that arrives after its first read, and lets withdrawn Markdown go, keeping its edits. - `shared/askQuestions.ts`, `shared/markdown.ts` and `shared/markdownHtml.ts` moved unchanged from `src/lib`, and `fileUriPath` moved to `shared/fileUri.ts`. The server now reads the call, and checks the card's rendering, with the same code the chat uses, and the remote PC bundle (which carries `server/` and `shared/`, not `src/`) can load it. A new server convention fails on any import of `src/` from code the bundle carries. - The design follows the Claude and Codex apps, which render a question's Markdown in their question cards (DESIGN.md, Prompt card). ## Fixtures Real omp 18.12.6 and 18.13.2 screens at 120 and 60 columns: a single-select question, a four-row cut multi-select question, and an option with a preview. Each comes with its session file while the dialog waits and after it was answered. They were recorded with a fake model provider and a disposable omp home in the isolated herdr test session. Personal paths are replaced. ## Verification - `server/omp-ask.test.ts` (new): on main the module does not exist, so the test fails. With the change it passes: - Every fixture gives the Markdown card. The exceptions are the 60-column cut question, whose footer is cut before `expand`, and the preview screens; both stay as drawn. - Fail-closed cases: 22 variants of the open call, including every Codex reproduction (``local_socket`` in code, `/gui_de`, `Keep *.log`, `_cache/`, an HTML entity, `~x~`); a question that ends in its own `…`; a four-row question without omp's `expand`; and omp's review step. - Mutations that drop omp's cut proof, stop requiring the `expand` hint, drop the entity or tilde guard, or match raw labels first each make it fail. - `server/omp.contract.test.ts` (new case, waiting on herdr's events, `pane.output_matched` and `pane.agent_status_changed`, rather than polling): a stand-in omp draws the recorded 18.13.2 screen, and its session file is the recorded open call. GET `/api/pane/prompt` returns the Markdown card. Settling the call in the session gives the plain card. POST `/api/pane/prompt/answer` sends Down then Enter. Removing the wiring in `readKnownPrompt` makes it fail. - `src/lib/promptAnswer.test.ts`, `src/lib/promptText.test.ts` and `src/components/PromptCard.test.tsx`: new cases, each failing under its mutation or on the code before its fix. - Merge-pass round: the glob pair (in a label and in the question), an escape and a two-backtick code span now keep the screen card. A typed label naming two options picks neither, and a same-id card upgrades to its Markdown. Each test failed before its fix. - Live: an Ovis server on the isolated test session with a real omp 18.13.2 asking these questions. - `curl /api/pane/prompt` returns `text_format: "markdown"` with the raw Markdown. - In the browser at 1280 and 390 px wide, the card renders bold, code, the question's link and the list with its fenced block, with no links in the options and no overflow. - A pick pressed on the card reached omp (`prompt_matched=true outcome=ok`), and omp moved to its next question. - `bun run check fast`: green. ## Reviews Commit hashes before `5b01e9fa` name the branch before its rebase onto main `62662793`; the same commits are on the branch under new hashes. - omo-native-code-reviewer: APPROVE. Its three MEDIUMs are fixed in this PR: typed labels, server-path test coverage, and no session lookup during answers. - Codex Sol (`gpt-6.1-sol`, xhigh), three rounds: - Round 1 blocked on a HIGH (marks stripped inside code and URLs) and a MEDIUM (an authored `…` taken as omp's cut). - Round 2 blocked on two HIGHs (an unpaired `*`/`_` outside code, a typed raw label naming another option) and a MEDIUM (four rows alone taken as a cut). - Round 3 confirmed those fixed and blocked on a HIGH: Bun decodes HTML entities and reads `~x~` as strikethrough, where omp draws both as written. That is fixed by a guard with its two cases. - No fourth round on the matcher: under the project's rule for heuristic matchers, the last round's finding is fixed, and further mismatches need exotic inputs. - Merge-pass round (Codex BLOCK and omo MEDIUMs on `ca6bf3ba`), fixed in `663a835d`: the chat's scanner could show other characters than omp drew and name the wrong option; a same-id card never took late Markdown; the contract scenario polled. - Re-reviews of `663a835d`: Codex Sol found a picture in the question showing its address (MEDIUM), fixed in `c3545004`. omo found that `server/omp-ask.ts` imported `src/`, which the remote bundle does not carry (CRITICAL), fixed in `5f18f761`, with the convention in `c75f9e72`. - Final reviews of `c3545004` and `5f18f761`: Codex Sol APPROVE (it reproduced the bundle load itself) and omo APPROVE. omo's LOWs (stale path comments, the `fileUriPath` re-export) are fixed in `c75f9e72`. - Merge-pass round on `c75f9e72` (Codex HIGH: a link to `recommended` read as the Recommended tag; MEDIUM: spacing-only twins) and CodeRabbit's inline point (spaces removed in the comparison) are fixed in `5b01e9fa`. The branch is rebased onto main `62662793` (#2217). Re-review of `5b01e9fa`: Codex Sol APPROVE and omo APPROVE. ## Known limit A word omp breaks mid-way on a narrow pane (a long address) keeps the plain card, since the screen then has a space the call does not. Bun's CommonMark parser and omp's `marked` can still part on rare constructs not found yet. Almost every such case makes the call differ from the screen, and the card stays as drawn. For a call to lend Markdown that omp draws otherwise, the open call would have to differ from the menu on screen only in such a construct. Even then, the card keeps the screen's id and answer keys. ## Remote bundle `server/` and `shared/` changes are carried by the remote PC bundle. A bundle-shaped copy (`server`, `shared`, `integrations`, `node_modules`) of the final head loads `server/prompt.ts`. The same copy of `c3545004` failed with `Cannot find module '../src/lib/markdown.ts'`. `REMOTE_BUNDLE_VERSION` is not raised here: only a release PR raises it (docs/development.md, Releasing).
…) (#2245) Follow-up to #2106 (#2059). Found while preparing the 0.4.20 test guide: on live omp 18.13.2 the tool approval ("Allow tool: bash") still showed the generic "Waiting for input" card. ## What it does for the user omp's tool approval card now shows whatever composer omp draws under the panel, not only the default status band: `composer.shape: box` (the config that hit it), and the Claude Code, Pi, borderless, rule, field and rail shapes, with any status-line preset and the ascii symbols. omp's Ask card now also shows under the rule, field and rail shapes. Answers still go through `POST /api/pane/prompt/answer` with the keys the panel names. ## Cause omp 18.13 mounts its composer under the approval panel (pi-tui `extension-ui-controller.ts` `showCollabAwareApproval`). #2106 read only the band shape's two rows (`⠹ 0s …` then `╰─`), and the status row had to start with a spinner and a time. With `composer.shape: box` omp draws `╭── … ──╮` / `╰─ … ─╯` instead, so the panel was not at the bottom and no reader matched. The Ask card's status-row check (#1812) also wanted that row right under the panel's border, but rule, field and rail keep one empty row over their status bar (`bottomBarGap`). ## How - `server/dialogs/omp.ts`: `OMP_COMPOSERS` lists the rows each of omp 18.13.2's eight built-in shapes draws when empty (`pi-tui/src/components/composer/*.ts`), in the unicode and ascii box glyphs, including the box and field rows of omp's IME-safe cursor layout. `ompAboveComposer` takes one of them off the bottom of the screen; the panel must then end right above it. The status row is read by its place only, since the user's status-line settings choose its text (the minimal preset has no spinner). - The status row is read by its place and by how omp starts it (a segment group's pad space, the band's powerline cap, or with no left group the band's gauge rule), never by its words. omp's own cursor in the empty prompt (`showHardwareCursor: false`) and a status line with no segments (omp then leaves out the row and its gap) read too. - Anything else under the panel matches no shape, so the terminal answers: a draft in any shape's prompt (omp then also redraws the hint and the Deny row), later output, a hook's status line, an extension's own composer shape, a box border omp does not draw, and a bare borderless `❯` with no status under it (a shell draws that too). - `server/dialogs/live.ts`: the Ask card's status row may sit under one empty row. ## Verification - **Fixtures:** omp 18.13.2 recorded in the isolated test herdr with a fake provider, `--approval-mode always-ask`: one approval screen per shape, the band and box shapes with the minimal status-line preset, box and claude with the ascii presets, box in a 59-column pane, and the Ask panel over rule, field and rail. Plus the live boxed screen from the test guide (a user's own config). Run root paths are replaced by ones of the same length. - **Tests:** `server/dialogs/omp.test.ts` gains 8 cases. On main's `omp.ts`/`live.ts`, 3 fail (every shape, the live box screen, the Ask over a gap). The fail-closed cases (a draft in each shape's prompt, output after the composer, another composer shape or box border, a panel left over lines shaped like a composer, output in the Ask's gap) pass on both. Each review fix was mutation-checked: dropping the IME-safe rows, the software cursor, the status-row start or the box border's corners, or letting `❯` take a draft, fails a case. - The reviewers' rendering probes (omp's own `Editor` and composer styles, both symbol presets: software cursor, IME-safe layout, no status segments, right group only, hook status) give the intended verdict on all of them. - **Live check** (on the final commit, and before the review fixes): real omp 18.13.2 in the isolated test herdr, an Ovis server on port 0, `GET /api/pane/prompt`, then `POST /api/pane/prompt/answer` (200 each time); the fake provider recorded omp's tool result. | shape | answer | card | tool result | |---|---|---|---| | box | Approve | approval, Allow tool: bash | ran | | box | Approve bash for this session | same | ran | | box | Deny | same | denied | | claude | Approve | same | ran | | rail | Deny | same | denied | | pi | Deny | same | denied | - Every recorded capture (8 shapes × default, minimal and ascii presets, and a 59-column pane) gives the approval card. - `bun run check fast` passes on the final commit (typecheck, build, 8145 unit tests). - The omp, prompt, submit, pending-input, conversation-push and api contract files: 388 pass. The 4 failures are api.contract conversation tests whose fixture Grok, Agy, Muse and Codex processes never start on this machine, and they fail the same way on 9f85caf, before this change. ## Only while omp holds the input (merge-pass review) The merge pass reproduced the HIGH declined below with a real omp: herdr 0.9.5 keeps naming a pane omp while its omp is **stopped behind the shell** (job control). The screen still showed omp's approval panel under the shell's lines, the card was offered, and `POST /api/pane/prompt/answer` sent Ctrl+Q to bash. - `server/prompt.ts` `ompOwnsInput`: an omp pane gets a card (any omp reader's, or the fallback card) only while one of omp's own processes is in the pane's foreground (`pane.process_info`, `agentProcesses(…, "omp")`, which picks out omp itself, whether its own binary or bun running its script), not herdr's retained agent label. Live-verified: a running omp is in front; stopped, the foreground is `/bin/bash` alone while herdr still says omp. - The answer checks it again right before each key it sends (`sendStep`); otherwise it stops with 409 `prompt_changed`. herdr not answering counts as not omp. - Windows is read as before: it has no job control and no foreground process group (`process_info` names only the shell there). - This covers #2106's 18.10 panel and every other omp reader (Ask, review, model picker, /handoff), which shared the same guarantee. Verification: - `server/omp.contract.test.ts`, event-driven on real herdr: a stand-in omp draws 18.13's panel and 18.10's, is stopped behind its shell, and the shell draws the dialog it left and reads one raw byte. No card, a 409, and the first byte the shell reads is the test's own. On 9f85caf both panels give the card, and the answer's key reaches the shell. - `server/prompt.test.ts`: omp stopped after the first key of a custom answer (whose text and Enter go unseen) sends nothing more and answers 409. Removing the check before each key fails it. The fake herdrs now report omp in front for a live omp. - Live, with real omp 18.13.2, an isolated herdr and Ovis on port 0: running, Approve ran and Deny was denied. Stopped behind bash over the stale box panel: `GET /api/pane/prompt` returned no card, the POST returned 409, and bash's first byte was the test's. - Codex Sol 6.1 Fast xhigh reviewed the fix: - MEDIUM, fixed: the unit fakes had no omp in front. - HIGH, residual, documented: a stop between the last check and herdr writing the key is not seen. herdr's `pane.send_keys` takes no condition on the foreground, so the check runs immediately before each key. A guarded send in herdr would close this. - HIGH, declined: a foreground group holding omp *and* another process passes. herdr lists every process in the foreground group, and omp's own children (LSP servers, tools) share omp's group. Rejecting mixed groups would drop live cards, and herdr cannot say which process reads the terminal. A shell in front of a stopped omp is in its own group, which is the case reproduced. ## Reviews - omo-native-code-reviewer reviewed the first commit: approve, no blockers. - MEDIUM, fixed: the IME-safe box and field rows (from omp's source, not recorded) had no test. - LOW, fixed: a draft was tested in three shapes only; now in all eight. - LOWs noted, unchanged: the status row is read by place; a hook's status line or an empty claude/rule left group gives no card (as before for band); the Ask's one-row gap is accepted in every shape (the panel's hint row still anchors it). - Codex Sol 6.1 Fast xhigh (adversarial) reviewed the first commit: - HIGH, fixed: any line in the status row's place kept a panel left over another program's lines live. The status row must now start as omp draws it, and the box's top border needs a rule out of each corner. - MEDIUM, fixed: omp's software cursor broke every shape. - MEDIUM, fixed: a status line with no segments (no row, no gap) gave no card. - Codex's re-review of the fixes: - MEDIUM, fixed: the ascii band with only a right group (`-…< [M] …`) no longer read. - HIGH, declined at first, then reproduced by the merge pass through a stopped omp. Fixed by the ownership gate above. ## Known limits (not changed here) - omp's Ask card under a status bar without omp's spinner (the minimal, compact and ascii status-line presets) with a shape that puts the bar under the panel (pi, claude, borderless, rule, field, rail) still shows the generic card: under the Ask panel, that row is the only evidence it is not later output, and its text is the user's choice. Fails closed. - omp's Ask panel in the ascii symbol preset is not read in any shape (the Ask reader looks for `╭`); it was not before either. - A hook or extension status line under omp's status bar shows no approval card, as for the band shape before. So does a bare borderless composer with no status segments. No `REMOTE_BUNDLE_VERSION` change.
Refs #1644 (part 1: the card disappears under omp's status row).
Summary
server/dialogs/live.tspromptTailIsActive: withcomposer.shape: pi, omp 18.12 prints its status row right under the Ask panel's bottom border while the question waits, e.g.⠋ 0s · ⬢ Ask fixture · 🗑 …/home/fixture · ◫ 5.7%/131K ⟲. The row was taken for later output, so the card disappeared.omp-questionandomp-submit, a last line that matches omp's status row (<spinner> <elapsed> ·orπ ·) is now passed over when the line right above it is the panel's bottom border (╰─…─╯).server/fixtures/omp-ask-18.12.6-*.txt: real omp 18.12.6 screens, recorded in the isolated herdr test session with a disposable omp home and a fake model provider, at 120 and 60 columns:server/dialogs/omp.test.ts:changelog.d/omp-ask-status-row.md.Not in this PR (follow-ups on #1644)
shared/protocol.ts), changes to the remote PC bundle and aREMOTE_BUNDLE_VERSIONbump.⎋ cance…, whichOMP_MULTI_HINT_REdoes not accept.server/dialogs/live.tsis also part of the remote PC bundle. It is not bumped here: the release process refreshes the remote bundle (docs/development.md).Verification
server/dialogs/,server/prompt.test.ts,server/core/omp-submit.test.ts,server/conventions.test.ts: 505/505. Typecheck.bun run check fast.