Plan-mode review: approve/reject/revise goals & to-dos before execute#4105
Conversation
Auto-park thread-scoped plan cards for review on interactive (WebChat) turns: the todo tool now defaults approval_mode=Required when the turn origin is WebChat, so the dispatcher holds the plan at awaiting_approval until the user decides. Background origins keep the config-driven default. Add openhuman.todos_revise_plan: rejects every awaiting card so the orchestrator can re-plan from user feedback (sent back as a chat message). Nudge the orchestrator prompt to lay out the plan and stop for review before executing. Document the feature in about_app. Unit + JSON-RPC E2E coverage.
Surface a parked thread plan above the composer as a single review panel: Approve & run (all awaiting cards -> ready), Reject (-> rejected), or send free-text feedback (clears the plan via revisePlan, then bounces the feedback into the thread so the agent re-plans). The todo strip stays read-only progress while the review card owns the decision. Adds threadApi.revisePlan, runDecidePlanAll/runRevisePlan actions, i18n keys across all locales, and Vitest coverage.
The parked-plan Approve control moved from the todo strip to the new PlanReviewCard; the RPC path (threadApi.decidePlan) is unchanged.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (19)
✅ Files skipped from review due to trivial changes (7)
🚧 Files skipped from review as they are similar to previous changes (12)
📝 WalkthroughWalkthroughThe PR adds a plan-review gate and revise-plan RPC path, wires chat runtime events into a pending review card, and updates orchestrator policy, translations, capability metadata, and coverage docs. ChangesPlan review flow
Sequence Diagram(s)sequenceDiagram
participant RequestPlanReviewTool
participant PlanReviewGate
participant ApprovalSurfaceSubscriber
participant ChatRuntimeProvider
participant PlanReviewCard
participant openhuman_plan_review_decide as openhuman.plan_review_decide
RequestPlanReviewTool->>PlanReviewGate: request_review(summary, steps)
PlanReviewGate->>ApprovalSurfaceSubscriber: publish PlanReviewRequested
ApprovalSurfaceSubscriber->>ChatRuntimeProvider: plan_review_request socket event
ChatRuntimeProvider->>PlanReviewCard: pending review state
PlanReviewCard->>openhuman_plan_review_decide: approve / reject / revise
openhuman_plan_review_decide->>PlanReviewGate: decide(request_id, resolution)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fd8898bab1
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
coderabbitai
Bot
added
agent
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
app/src/pages/conversations/components/PlanReviewCard.tsx (2)
85-89: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueOptional: avoid recomputing
cardLabel(card).
cardLabel(card)runs up to three times per card (line 85 and twice on line 86). Hoisting it to aconst label = cardLabel(card)inside the.mapkeeps the objective dedupe check clearer and avoids the repeated calls.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/conversations/components/PlanReviewCard.tsx` around lines 85 - 89, Hoist the repeated card label computation in PlanReviewCard so it is evaluated once per card instead of multiple times. Inside the .map for the cards, store cardLabel(card) in a local const (for example, label) and use that value both for rendering the title and for the objective dedupe comparison in the objective conditional. This keeps the logic in PlanReviewCard clearer and avoids repeated calls while preserving the existing behavior.
60-64: 🔒 Security & Privacy | 🔵 TrivialUse a non-modal role here.
alertdialogimplies a modal dialog with focus management, but this panel is rendered inline above the composer and doesn’t trap or move focus.role="region"orrole="group"with the existing label fits better.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/conversations/components/PlanReviewCard.tsx` around lines 60 - 64, The PlanReviewCard container is using a modal-only role even though it renders inline, so update the root element in PlanReviewCard to use a non-modal role such as region or group while keeping the existing aria-label from t('conversations.planReview.title'). Make the change on the main wrapper that currently declares role="alertdialog" so the accessibility semantics match the actual behavior.app/src/pages/Conversations.tsx (1)
2612-2648: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winOptional: guard against duplicate plan decisions;
disabledprop is effectively dead here.Two small things on the wiring:
- This block is already gated by
selectedThreadId && awaitingPlanCards.length > 0, sodisabled={!selectedThreadId}is alwaysfalse. It's harmless but misleading — there's no actual disabled state while a decision is in flight.- Because
disablednever flips true, the Approve / Reject / Send-feedback buttons stay clickable whilerunDecidePlanAll/runRevisePlanare awaiting. A rapid double-click (or Approve-then-Reject) can fire two concurrentdecidePlanbatches, or send the feedback message twice, before the board refresh unmounts the card. Consider tracking a local in-flight flag and passing it asdisabledso the card locks until the action settles.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@app/src/pages/Conversations.tsx` around lines 2612 - 2648, The PlanReviewCard wiring in Conversations.tsx has no real disabled state, so Approve/Reject/Send-feedback can be triggered multiple times while runDecidePlanAll or runRevisePlan is still in flight. Add a local in-flight guard around this block, set it before calling runDecidePlanAll/runRevisePlan and clear it when the promise settles, then pass that flag into PlanReviewCard as disabled. Use the existing selectedThreadId, awaitingPlanCards, runDecidePlanAll, and runRevisePlan callbacks to keep the card locked until the action finishes.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@app/src/pages/Conversations.tsx`:
- Around line 2612-2648: The PlanReviewCard wiring in Conversations.tsx has no
real disabled state, so Approve/Reject/Send-feedback can be triggered multiple
times while runDecidePlanAll or runRevisePlan is still in flight. Add a local
in-flight guard around this block, set it before calling
runDecidePlanAll/runRevisePlan and clear it when the promise settles, then pass
that flag into PlanReviewCard as disabled. Use the existing selectedThreadId,
awaitingPlanCards, runDecidePlanAll, and runRevisePlan callbacks to keep the
card locked until the action finishes.
In `@app/src/pages/conversations/components/PlanReviewCard.tsx`:
- Around line 85-89: Hoist the repeated card label computation in PlanReviewCard
so it is evaluated once per card instead of multiple times. Inside the .map for
the cards, store cardLabel(card) in a local const (for example, label) and use
that value both for rendering the title and for the objective dedupe comparison
in the objective conditional. This keeps the logic in PlanReviewCard clearer and
avoids repeated calls while preserving the existing behavior.
- Around line 60-64: The PlanReviewCard container is using a modal-only role
even though it renders inline, so update the root element in PlanReviewCard to
use a non-modal role such as region or group while keeping the existing
aria-label from t('conversations.planReview.title'). Make the change on the main
wrapper that currently declares role="alertdialog" so the accessibility
semantics match the actual behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d9d8455b-951b-4b43-88a8-c4c9bc32f5ce
📒 Files selected for processing (28)
app/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsxapp/src/pages/conversations/components/PlanReviewCard.test.tsxapp/src/pages/conversations/components/PlanReviewCard.tsxapp/src/pages/conversations/taskPlanActions.test.tsapp/src/pages/conversations/taskPlanActions.tsapp/src/services/api/threadApi.tsdocs/TEST-COVERAGE-MATRIX.mdsrc/openhuman/about_app/catalog_data.rssrc/openhuman/agent/tools/todo.rssrc/openhuman/agent_registry/agents/orchestrator/prompt.mdsrc/openhuman/todos/ops.rssrc/openhuman/todos/schemas.rstests/json_rpc_e2e.rs
# Conflicts: # app/src/lib/i18n/ar.ts # app/src/lib/i18n/bn.ts # app/src/lib/i18n/de.ts # app/src/lib/i18n/en.ts # app/src/lib/i18n/es.ts # app/src/lib/i18n/fr.ts # app/src/lib/i18n/hi.ts # app/src/lib/i18n/id.ts # app/src/lib/i18n/it.ts # app/src/lib/i18n/ko.ts # app/src/lib/i18n/pl.ts # app/src/lib/i18n/pt.ts # app/src/lib/i18n/ru.ts # app/src/lib/i18n/zh-CN.ts # tests/json_rpc_e2e.rs
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4d77bb6a4d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
Three correctness fixes from PR review: - Orchestrator allowlist listed the legacy `todowrite` alias, which resolves to no registered tool (named scope matches `TodoTool::name() == "todo"` exactly). Switch to `todo` so interactive turns can create the cards the plan-review gate hooks. - `requires_plan_approval` ignored explicit per-card `Required` when the global task-plan-approval flag was off. Make explicit `Required` always park (and `NotRequired` always opt out); unset still follows the global default. - Plan-review feedback reused handleSendMessage/handleSendFollowup, which pulled in the composer's pending attachments and cleared the user's draft. Add an `isolateComposer` option so feedback sends only its own text and preserves the composer state.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 85fc17a37a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
The task-board approach didn't work for live chat: the dispatcher only sweeps the user-tasks / task-sources boards, never conversation threads, so a chat plan was never parked to awaiting_approval and an approved plan never ran. Replace it with a parked-future gate modelled on ApprovalGate: - New src/openhuman/plan_review/: in-memory PlanReviewGate parks the LIVE turn on a oneshot; the request_plan_review tool blocks the interactive turn until the user decides; openhuman.plan_review_decide resolves it (approve/reject/revise+feedback). Non-interactive origins auto-approve. - DomainEvent::PlanReviewRequested/Decided bridged to a plan_review_request socket event (reuses the web approval subscriber + APPROVAL_CHAT_CONTEXT). - Orchestrator: prompt calls request_plan_review after laying out the plan; allowlist gains request_plan_review (and todo, replacing the dead todowrite alias). - Frontend: PlanReviewCard is now event-driven (pendingPlanReviewByThread) and resolves via plan_review_decide; retired the task-board awaiting_approval rendering, runDecidePlanAll/runRevisePlan, threadApi.revisePlan, and the todo WebChat auto-gating. Keeps the requires_plan_approval fix (explicit Required overrides the global off-switch) for the dispatched boards. Rust gate/tool/schema unit tests + plan_review_decide JSON-RPC E2E + frontend card/provider tests.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b4358a5c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…post-approval Address two Codex P2s on the gate: - ChatRuntimeProvider now clears pendingPlanReviewByThread on chat_done / chat_error / disconnect (alongside the approval clear), so an expired/cancelled review card can't linger with a stale requestId. - Orchestrator prompt: call request_plan_review with the steps BEFORE creating todo cards, and only create cards after approval — a rejected/revised plan no longer leaves todo cards pinned on the thread board. Adds provider tests for the set + clear-on-end paths.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ac9a941dc0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
src/openhuman/plan_review/schemas.rs (1)
90-102: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd debug/trace logging on the error branches.
handle_decidelogs the success entry but therevise-without-feedback and invalid-decision branches return errors silently. Adding atracing::debug!/warn!on these rejection paths aids diagnosing malformed RPC calls.Proposed logging on error branches
"revise" => { let feedback = p.feedback.unwrap_or_default(); if feedback.trim().is_empty() { + tracing::debug!( + request_id = %p.request_id, + "[rpc][plan_review] revise rejected: empty feedback" + ); return Err("revise requires non-empty `feedback`".to_string()); } PlanReviewResolution::Revise { feedback } } other => { + tracing::debug!( + request_id = %p.request_id, + decision = %other, + "[rpc][plan_review] invalid decision verb" + ); return Err(format!( "invalid decision '{other}' (expected approve|reject|revise)" )) }As per coding guidelines: "Add debug logging to entry/exit, branches, external calls, retries/timeouts, state transitions, and errors using
log/tracingatdebug/tracelevel in Rust".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/plan_review/schemas.rs` around lines 90 - 102, The `handle_decide` error paths in `schemas.rs` return silently for `revise` with empty `feedback` and for invalid `decision` values; add `tracing::debug!` or `tracing::warn!` logging on both branches before returning the `Err`, including the rejected decision/feedback context so malformed RPC calls are visible. Keep the existing success logging intact and place the new logs near the `revise` arm and the `other` match arm in `PlanReviewResolution::from`/`handle_decide`-related code.Source: Coding guidelines
src/openhuman/plan_review/gate.rs (1)
59-101: 🩺 Stability & Availability | 🔵 TrivialShort-circuit unroutable plan reviews in
src/openhuman/plan_review/gate.rs
Whenthread_idisNone, this still registers a waiter and waits for the full TTL even though nothing can surface or calldecide_by_thread. ReturnRejectimmediately for unroutable requests instead of parking them.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/openhuman/plan_review/gate.rs` around lines 59 - 101, In request_review, handle unroutable plan reviews up front: when thread_id is None, do not create a oneshot waiter, insert into waiters/thread_to_request, or await the TTL. Short-circuit immediately with PlanReviewResolution::Reject before the publish_global(DomainEvent::PlanReviewRequested) path, and keep the existing request_id/tracing flow intact for routable requests.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/src/pages/Conversations.tsx`:
- Around line 2623-2625: `PlanReviewCard` is being reused across different
parked/re-parked reviews, so its local `useState` can retain stale decision and
feedback state. Update the `PlanReviewCard` usage in `Conversations.tsx` to add
a stable `key` based on the review/request id (for example, the same identifier
exposed by `pendingPlanReview`), matching the `ApprovalRequestCard` and
`IntegrationConnectCard` pattern above. This will force a remount when a new
review is shown and reset the component state correctly.
In `@src/openhuman/plan_review/tool.rs`:
- Around line 170-188: The plan-review waiter cleanup in request_review can be
skipped if the future is cancelled before rx.await completes, leaving stale
waiters and thread_to_request entries behind. Update the request_review flow in
gate.rs so cleanup happens before the await via a drop guard or similar
cancellation-safe mechanism, and ensure the existing
interactive_turn_parks_until_resolved test still passes by verifying parked
requests are removed even when the future is dropped early.
---
Nitpick comments:
In `@src/openhuman/plan_review/gate.rs`:
- Around line 59-101: In request_review, handle unroutable plan reviews up
front: when thread_id is None, do not create a oneshot waiter, insert into
waiters/thread_to_request, or await the TTL. Short-circuit immediately with
PlanReviewResolution::Reject before the
publish_global(DomainEvent::PlanReviewRequested) path, and keep the existing
request_id/tracing flow intact for routable requests.
In `@src/openhuman/plan_review/schemas.rs`:
- Around line 90-102: The `handle_decide` error paths in `schemas.rs` return
silently for `revise` with empty `feedback` and for invalid `decision` values;
add `tracing::debug!` or `tracing::warn!` logging on both branches before
returning the `Err`, including the rejected decision/feedback context so
malformed RPC calls are visible. Keep the existing success logging intact and
place the new logs near the `revise` arm and the `other` match arm in
`PlanReviewResolution::from`/`handle_decide`-related code.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 7dcd43b1-cc1b-48f9-aa5e-b1f1ee72ae58
📒 Files selected for processing (41)
app/src/lib/i18n/ar.tsapp/src/lib/i18n/bn.tsapp/src/lib/i18n/de.tsapp/src/lib/i18n/en.tsapp/src/lib/i18n/es.tsapp/src/lib/i18n/fr.tsapp/src/lib/i18n/hi.tsapp/src/lib/i18n/id.tsapp/src/lib/i18n/it.tsapp/src/lib/i18n/ko.tsapp/src/lib/i18n/pl.tsapp/src/lib/i18n/pt.tsapp/src/lib/i18n/ru.tsapp/src/lib/i18n/zh-CN.tsapp/src/pages/Conversations.tsxapp/src/pages/__tests__/Conversations.render.test.tsxapp/src/pages/conversations/components/PlanReviewCard.test.tsxapp/src/pages/conversations/components/PlanReviewCard.tsxapp/src/pages/conversations/taskPlanActions.test.tsapp/src/providers/ChatRuntimeProvider.tsxapp/src/providers/__tests__/ChatRuntimeProvider.test.tsxapp/src/services/chatService.tsapp/src/store/chatRuntimeSlice.tsdocs/TEST-COVERAGE-MATRIX.mdsrc/core/all.rssrc/core/event_bus/events.rssrc/openhuman/about_app/catalog_data.rssrc/openhuman/agent/task_dispatcher/poller.rssrc/openhuman/agent/task_dispatcher/tests.rssrc/openhuman/agent/tools/todo.rssrc/openhuman/agent_registry/agents/orchestrator/agent.tomlsrc/openhuman/agent_registry/agents/orchestrator/prompt.mdsrc/openhuman/channels/providers/web/event_bus.rssrc/openhuman/mod.rssrc/openhuman/plan_review/gate.rssrc/openhuman/plan_review/mod.rssrc/openhuman/plan_review/schemas.rssrc/openhuman/plan_review/tool.rssrc/openhuman/plan_review/types.rssrc/openhuman/tools/ops.rstests/json_rpc_e2e.rs
✅ Files skipped from review due to trivial changes (10)
- app/src/lib/i18n/de.ts
- src/openhuman/agent/tools/todo.rs
- app/src/lib/i18n/pt.ts
- app/src/lib/i18n/ko.ts
- app/src/lib/i18n/ar.ts
- src/openhuman/plan_review/mod.rs
- app/src/lib/i18n/pl.ts
- app/src/lib/i18n/ru.ts
- docs/TEST-COVERAGE-MATRIX.md
- src/openhuman/about_app/catalog_data.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- app/src/lib/i18n/es.ts
- src/openhuman/agent_registry/agents/orchestrator/prompt.md
- app/src/lib/i18n/fr.ts
- app/src/lib/i18n/en.ts
- app/src/lib/i18n/zh-CN.ts
- app/src/lib/i18n/hi.ts
- app/src/lib/i18n/it.ts
- app/src/lib/i18n/bn.ts
- tests/json_rpc_e2e.rs
…keyed card Address review on the gate: - request_plan_review now returns ToolTimeout::Unbounded so the harness's global tool timeout (~120s) can't drop the parked future before the gate's own 10-min TTL — approving the card always resumes the turn. - PlanReviewGate cleans up the waiter/thread mapping via a Drop guard, so a cancelled/dropped parked future can't leak a re-decidable entry. - PlanReviewCard is keyed by requestId so a re-parked (revised) plan or a thread switch remounts it and resets local decision/feedback state. Adds a gate cancellation-cleanup test.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 961319cf3a
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…gate The plan-review events reuse the approval-surface subscriber, which was only registered inside the `install_gate` branch. A standalone/CLI/Docker core run with OPENHUMAN_APPROVAL_GATE=0 (and no listening integrations, so start_channels is skipped) would then park a plan review and publish PlanReviewRequested with no subscriber to bridge it — the card never shows and the turn dies at the gate TTL. Move the subscriber registration to the always-run serve boot, independent of the approval gate.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78303a8e84
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
# Conflicts: # app/src/providers/ChatRuntimeProvider.tsx
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a2cf42563f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // onDone/onError would otherwise leave the card stuck for a turn that | ||
| // can't complete. | ||
| dispatch(clearPendingApprovalForThread({ threadId })); | ||
| dispatch(clearPendingPlanReviewForThread({ threadId })); |
There was a problem hiding this comment.
When the Socket.IO connection drops transiently while a turn is parked on request_plan_review, this clears the only stored requestId even though the core turn and PlanReviewGate are not cancelled by a socket disconnect; the socket bridge explicitly supports reconnecting to the thread room to continue receiving an in-flight stream. After reconnect the review card is gone, so the user cannot approve/revise and the parked turn waits until the 10-minute TTL rejects the plan.
Useful? React with 👍 / 👎.
1 participant
Summary
ApprovalGate. The orchestrator calls a newrequest_plan_reviewtool that blocks the turn on an in-memoryPlanReviewGate(oneshot) until the user decides via theopenhuman.plan_review_decideRPC. Non-interactive turns (cron/subconscious/CLI) auto-approve, so automation is never blocked.Problem
OpenHuman could set goals / lay out a multi-step plan and start executing immediately — no "review once before we execute" step like Claude Code / Codex plan mode.
An initial attempt reused the task-board
awaiting_approvallifecycle, but review (thanks @chatgpt-codex-connector) surfaced that this cannot work for live chat: the background dispatcher (task_dispatcher::poll_once) only sweeps theuser-tasks/task-sourcesboards, never conversation threads. So a chat plan was never parked toawaiting_approval(the card staystodo) and an "approved" card was never executed. This PR replaces that approach.Solution
Gate the live turn, not a board status:
src/openhuman/plan_review/— in-memoryPlanReviewGateparks the turn on aoneshot(fail-closed to reject on TTL/drop);request_plan_reviewtool blocks the interactive turn and maps the resolution to a proceed / stop / revise instruction;openhuman.plan_review_decideresolves it (approve/reject/revise+feedback).DomainEvent::PlanReviewRequested/PlanReviewDecided, bridged to aplan_review_requestsocket event by the existing web approval subscriber (reusesAPPROVAL_CHAT_CONTEXTfor routing).request_plan_reviewafter laying out the plan; allowlist gainsrequest_plan_review(andtodo, replacing the deadtodowritealias that matched no registered tool).PlanReviewCardis event-driven (pendingPlanReviewByThread) and resolves viaplan_review_decide; retired the task-boardawaiting_approvalrendering,runDecidePlanAll/runRevisePlan,threadApi.revisePlan, and thetodoWebChat auto-gating.requires_plan_approvalfix (explicit per-cardRequiredoverrides the global off-switch) for the dispatched boards.Submission Checklist
plan_review_decideJSON-RPC E2E, and frontend card + provider/render tests cover the changed lines; gate enforced by.github/workflows/pr-ci.yml.4.2.7 Plan-mode reviewindocs/TEST-COVERAGE-MATRIX.mdreflects the parked-future gate.## RelatedImpact
Desktop/web chat only. Interactive multi-step turns now pause for review; trivial one-step replies are unaffected. Background/cron/subconscious runs auto-approve. No migrations; one new in-memory gate + one read-only RPC + two domain events. No persistence (a parked interactive turn can't resume across a restart). i18n reuses existing keys.
Related
4.2.7.AI Authored PR Metadata
Linear Issue
Commit & Branch
feat/plan-mode-review0b4358a5c90cValidation Run
pnpm --filter openhuman-app format:checkpnpm typecheckcargo test --lib plan_review todos::ops agent::tools::todo task_dispatcher,pnpm vitest run(PlanReviewCard, taskPlanActions, Conversations.render)cargo fmt --check,cargo check --manifest-path Cargo.tomlapp/src-tauri) unchanged.Validation Blocked
command:N/Aerror:N/Aimpact:N/ABehavior Changes
Parity Contract
WebChatorigins never park.Duplicate / Superseded PR Handling
Summary by CodeRabbit
New Features
Bug Fixes
Tests