Repository navigation
fix(omp): OMP's plan review shows as a plan card you can answer from desktop and mobile chat - #31635
Conversation
4ce13b8 to
a54145a
Compare
📝 Walkthrough
Priority: ⚪ Pending latest changes Merge Risk: 🔵 Low · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 12 files. (1 skipped:… | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Title check The title clearly identifies the main change: OMP plan reviews are presented as answerable plan cards in desktop and mobile chat. Description check The description explains the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility, and known verification gaps. It omits several template checklist sections, but … Linked Issues check Issue #24415requires mobile interaction with OMP plan reviews instead of a stalled conversation. The PR adds a mobileplanReviewcard, gives it precedence over approval text, exposes enabled review…Out of Scope Changes check The shared prompt envelope, desktop and mobile cards, OMP event listener, blocked-row state, OSC routing, localization, and tests directly support issue #24415. The PR preserves the ACP route and comp…Full details: Description check
-
Explanation
The description explains the user impact, implementation, rationale, linked issue, visual proof, testing, compatibility, and known verification gaps. It omits several template checklist sections, but the required change details are substantially complete.
Full details: Linked Issues check
-
Explanation
Issue
#24415requires mobile interaction with OMP plan reviews instead of a stalled conversation. The PR adds a mobileplanReviewcard, gives it precedence over approval text, exposes enabled review choices, and routes answers through review-specific OSC input. The listener publishes the blocked review and handles resolution. Focused tests cover mobile precedence, forwarding, answer routing, and review state.
Full details: Out of Scope Changes check
-
Explanation
The shared prompt envelope, desktop and mobile cards, OMP event listener, blocked-row state, OSC routing, localization, and tests directly support issue
#24415. The PR preserves the ACP route and compatibility behavior. No unrelated change is demonstrated by the reviewed summary.
-
Full details: Docstring Coverage
-
Explanation
Docstring coverage is 52.38% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 21 functions across 12 files. (1 skipped: 1 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
-
- 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.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fd0f6c64-b545-4251-a8dd-a7300a70894b
📥 Commits
Reviewing files that changed from the base of the PR and between 7a06b65 and 4ce13b899cce2762c315fca9b3702589080e6f28.
📒 Files selected for processing: 13
mobile/src/session/mobile-native-chat-permission.tsmobile/src/session/use-mobile-native-chat-prompts.test.tsmobile/src/session/use-mobile-native-chat-prompts.tssrc/main/pi/agent-status-extension-test-harness.tssrc/main/pi/agent-status-handler-source.tssrc/main/pi/agent-status-omp-plan-review.test.tssrc/main/pi/omp-plan-review-source.tssrc/renderer/src/components/native-chat/native-chat-interactive-prompt.tssrc/renderer/src/i18n/locales/en.jsonsrc/shared/agent-hook-listener-omp-plan-review.test.tssrc/shared/agent-hook-listener/providers/pi-family-events.tssrc/shared/agent-hook-listener/providers/pi-family-tool-fields.tssrc/shared/omp-plan-review-prompt.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| " if (typeof data !== 'string' || !data.startsWith(OMP_PLAN_REVIEW_ANSWER_PREFIX)) return undefined", | ||
| " const [answerId, choice] = data.slice(OMP_PLAN_REVIEW_ANSWER_PREFIX.length).replace(/\\x07$/, '').split(';')", | ||
| // Why: a card drawn for a replaced review must not answer or dismiss this one. | ||
| ' if (answerId !== reviewId) return { consume: true }', |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
Validate the answer choice and the event.options shape in the generated handler.
The handler trusts two values without a runtime check:
- Line 22 parses the answer with
split(';'). If the data is…;<reviewId>with no;choice,choiceisundefined. The handler then callsctx.resolvePlanReview(reviewId, undefined)and consumes the input. - Line 34 calls
.filteronevent.options ?? []and readsoption.disabled. Ifoptionsis not an array, or an entry isnull, the handler throws. This happens afterompPlanReviewand the input listener are already registered. The review then never posts asblocked, and the listener stays active.
OMP 18.12.2 sends well-formed data, so this only matters for other OMP builds or malformed input. The fix is cheap.
🛡️ Proposed fix
- ' if (answerId !== reviewId) return { consume: true }',
+ ' if (answerId !== reviewId || typeof choice !== \'string\' || choice.length === 0) return { consume: true }',- ' options: (event.options ?? []).filter((option) => !option.disabled).map((option) => ({ id: option.id, label: option.label })),',
+ ' options: (Array.isArray(event.options) ? event.options : []).filter((option) => option && !option.disabled).map((option) => ({ id: option.id, label: option.label })),',Based on learnings: "check whether boundary data is used with shape or type assumptions not guaranteed by the boundary API or earlier runtime validation."
Also applies to: 34-34
Source: Learnings
a54145a to
c5f7194
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
d33c5bce-ed1c-4957-9dc6-9075c78fbd17
📥 Commits
Reviewing files that changed from the base of the PR and between 4ce13b899cce2762c315fca9b3702589080e6f28 and c5f7194.
📒 Files selected for processing: 1
src/shared/omp-plan-review-prompt.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review.
| while (json.length > AGENT_STATUS_INTERACTIVE_PROMPT_MAX_LENGTH && text.length > 0) { | ||
| const excess = json.length - AGENT_STATUS_INTERACTIVE_PROMPT_MAX_LENGTH | ||
| text = text.slice(0, Math.max(0, text.length - excess - TRUNCATION_NOTE.length)) | ||
| json = serialize(`${text}${TRUNCATION_NOTE}`) | ||
| } | ||
| return json |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Enforce the prompt cap when non-plan fields are large.
If the serialized options or reviewId exceed the cap, the loop stops when text is empty. It then returns an oversized prompt. Bound those fields or reject an envelope that still exceeds AGENT_STATUS_INTERACTIVE_PROMPT_MAX_LENGTH after clipping the plan. This keeps a large option label from breaking the status field’s length contract.
c5f7194 to
6bf419e
Compare
OMP 18.12 reports its plan review overlay to extensions and lets them answer it with ctx.resolvePlanReview. Orca's status extension now posts the open review as a blocked row with a plan envelope; desktop and mobile draw it with the existing plan approval card. Each choice writes a private OSC naming the review, which the extension reads through ctx.ui.onTerminalInput before the overlay sees it: approvals and save go to ctx.resolvePlanReview, refine closes the overlay so the user can send the change, and a card for a replaced review is ignored. The review stays blocked until it resolves, even if an earlier turn's async child finishes under it.
6bf419e to
8da0aac
Compare
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to GitHub limitations.
🟡 Minor · Preserve the OMP review ID through resolution handling. · pi-family-events.ts:71-79
src/shared/agent-hook-listener/providers/pi-family-events.ts:71-79
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve the OMP review ID through resolution handling.
/hook/ompacceptsplan_review_resolveddirectly, outside the generated handler that rejects stale IDs. The listener clearsinteractivePromptfor every such event. Because the forwarded event omitsreview_id, an older resolution can clear the replacement review and remove its card.Suggested fix
--- a/src/main/pi/omp-plan-review-source.ts +++ b/src/main/pi/omp-plan-review-source.ts @@ - post('plan_review_resolved', { is_idle: isIdle }) + post('plan_review_resolved', { review_id: event.reviewId, is_idle: isIdle })--- a/src/shared/agent-hook-listener/providers/pi-family-events.ts +++ b/src/shared/agent-hook-listener/providers/pi-family-events.ts @@ if ( !stateName || (stateName === 'working' && holdsOmpApproval(state, agentType, eventName, paneKey)) || (eventName === 'agent_end' && holdsOmpPlanReview(state, agentType, hookPayload, paneKey)) ) { return null } + if (isOmpPlanReviewEnd) { + const activeReviewId = parseOmpPlanReviewPrompt( + state.lastToolByPaneKey.get(paneKey)?.interactivePrompt + )?.reviewId + if (readString(hookPayload, 'review_id') !== activeReviewId) return null + } const snapshot = resolveToolState(
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
fe257841-712a-43f1-b1af-41bfb956623f
📥 Commits
Reviewing files that changed from the base of the PR and between c5f7194 and 6bf419ea1f81f5cfa2ccd628d5e90a4f6519e209.
📒 Files selected for processing: 3
mobile/src/session/mobile-native-chat-permission.tssrc/renderer/src/components/native-chat/native-chat-interactive-prompt.tssrc/renderer/src/i18n/locales/en.json
🚧 Files skipped from review as they are similar to previous changes: 1
- src/renderer/src/i18n/locales/en.json
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
Fixes #24415
What users saw
When terminal OMP opened its plan review, the chat on the phone and desktop showed no plan and no buttons. The row read "done" because OMP aborts the turn to open the overlay. The only way out was the raw terminal and arrow keys.
Now
The row goes to Blocked and the chat shows the plan in the existing plan approval card (the one Claude's ExitPlanMode uses), with OMP's own choices: Approve and execute, Approve and compact context, Approve and keep context, Refine plan, Save and quit. Disabled choices are left out.
ctx.resolvePlanReview.Root cause and fix
OMP 18.12 fires
plan_review_requested/plan_review_resolvedand offersctx.resolvePlanReview. Orca's OMP status extension never listened for them.src/main/pi/omp-plan-review-source.ts) posts the open review, and the hook listener makes it a blocked row with a{ planReview }prompt envelope. The plan is clipped to the field's 16k cap so the JSON stays valid.ESC ] 7503 ; orca-plan-review ; <reviewId> ; <choice> BEL) through the existing raw approval send path, the same one approvals already use locally, over SSH, on orcad and from the phone'sterminal.send. The extension reads it withctx.ui.onTerminalInputbefore the overlay does and callsresolvePlanReview. A card for a replaced review is consumed and ignored.plan_review_resolved: done if OMP is idle, working if it started executing. Anagent_endfrom an earlier turn's async child does not clear the card.yinto the overlay.Compatibility:
planReviewkey. They see the Blocked row but no card.Verification
/plan-review:MobileNativeChatPermissionthrough a web-bundle harness. Tapping Approve sends the exact OSC. No paired device run.Visual Proof
Before (origin/main): OMP's plan review is open in the terminal; chat shows nothing and the row says Done.
before.mp4
After: the plan card shows; Approve and execute starts the plan in OMP.
after.mp4
Mobile card (same envelope, mobile component):