github / github/app

Plan approval races between the human and the coordinating parent session, and the human's losing decision is silently discarded

未关闭
#2,353 0 条评论 0 个 reaction 已指派 0 人 在 GitHub 查看
Bugs
主要语言
没有语言数据
星标
2.1k
派生
153
PR 合并指标
30 天内没有已合并 PR

描述

### Short summary

When a child session pauses in plan mode, both the human and the coordinating parent session are told they are the approver, with no arbitration — whoever calls first wins, and the loser's approval silently does nothing (or worse, silently changes the child's mode).

### Affected version or release

Desktop app, main (app version 1.0.73).

### Installation context

macOS desktop app. Parent chat orchestrating child sessions via `create_session` with `kickoff.mode: "plan"` + `notify_on_idle`.

### What happened?

When a child hits `exit_plan_mode`, `TauriSessionHandler::handle` (`src-tauri/src/session/handler.rs`) does three things for the *same* plan:

1. Broadcasts `exit_plan_mode_request` over WS → the human sees the Approve/Reject card in the child session.
2. Records a `PendingPlanSnapshot` so a parent can read it via `get_session`.
3. Calls `notify_creator_plan_ready` → pushes a system notification to the parent that says *"To approve or reject it without waiting for a human, use the `respond_to_session_plan` tool."*

Both audiences are told they own the decision. The `orchestrate` skill reinforces it: *"you are the approver, so a human doesn't have to be."* Nothing tells the human that a parent is also about to approve, and nothing tells the parent that a human is looking at the same prompt. Both resolve the same in-process oneshot keyed by `request_id`, so it's a plain race.

**Parent wins (the common case).** `respond_to_session_plan` → `SessionManager::respond_to_session_plan` → `resolve_exit_plan_mode` fires the oneshot and clears the snapshot, but broadcasts nothing. The frontend only drops its exit-plan request when the human responds, or on the next `session.idle` / `user.message` / `session.task_complete` event (`useWebSocketMessageDispatcher.ts:1940-1960`). So the human keeps staring at a live-looking Approve/Reject card for the entire duration of the child's post-approval turn. (This is the visible symptom already filed as #2136.)

**Human then clicks the stale card — this is the part that isn't just cosmetic.** `respondToExitPlanMode` (`useWebSocketUtilities.ts:644`) optimistically removes the card, sends `set_session_mode` if they picked an autopilot action, and calls `markSessionWorkResumed` — *before* the backend replies. The backend's `resolve_exit_plan_mode` then finds no pending request and returns `AppError::RequestNotFound`, which `handlers::session::respond_to_exit_plan_mode` only logs with `warn!`. Nothing is surfaced to the user. Net effect: the human's *plan decision* is discarded, but their *mode selection* is applied. If the parent approved with `interactive` and the human clicks "Approve and implement" (autopilot), the app flips the child to autopilot while the CLI is running under the parent's `interactive` action. UI state and CLI behavior diverge, with no error shown.

**The two approval paths also aren't equivalent.** The frontend path uploads the approved plan as a GitHub issue artifact (`upload_approved_plan_artifact`) and syncs session mode; the parent path (`SessionManager::respond_to_session_plan`) does neither. The same approval produces different side effects depending on who won the race.

**Human wins** is the only clean case: the parent's later `respond_to_session_plan` fails with "has no plan awaiting a decision," which the parent can report sensibly.

### Steps to reproduce

1. From a parent chat, `create_session` with `kickoff.mode: "plan"` and `notify_on_idle: "once"`.
2. Wait for the child to produce a plan. The parent receives the `plan_ready` notification; the human simultaneously sees the Approve/Reject card in the child session.
3. Let the parent call `respond_to_session_plan` with `{ approved: true, selected_action: "interactive" }`.
4. Observe the child's Approve/Reject card is still rendered and still actionable.
5. Click "Approve and implement" (autopilot) on the stale card.
6. Observe: no error is shown, the plan decision is dropped, but the child's mode flips to autopilot in the UI while the CLI continues under `interactive`.

### Expected behavior

There should be one unambiguous owner of a pending plan decision, and losing the race should be visible rather than silent.

- Resolving a plan from *any* path should broadcast a "plan resolved" message so every client clears the prompt immediately, instead of waiting for the next `session.idle`.
- A late/duplicate response should be rejected before the frontend applies optimistic side effects — in particular, `set_session_mode` and `markSessionWorkResumed` should not fire when the underlying request is already gone.
- If a human's decision is discarded because a parent already resolved it, tell them (toast or an inline "already approved by parent session — continuing in interactive mode" state on the card).
- Both approval paths should have identical side effects (plan artifact upload, session-mode sync).
- Worth deciding explicitly: should the human prompt even be shown for a child whose parent is coordinating it? Today `notify_on_idle` implicitly opts the parent into approving, but the human prompt is unconditional. An explicit "who approves" signal on `create_session` would remove the ambiguity entirely.

### Additional context

Related: #2136 covers the stale card/indicator as a visual symptom. This issue is about the underlying ownership ambiguity and the silent data loss when the human loses the race.

Relevant code:
- `src-tauri/src/session/handler.rs` — `ExitPlanModeHandler::handle` (broadcast + snapshot + parent notification)
- `src-tauri/src/session/manager/mod.rs:1303` — `respond_to_session_plan`
- `src-tauri/src/session/manager/pending_requests.rs:161` — `resolve_exit_plan_mode` (no broadcast on resolve)
- `src-tauri/src/handlers/session.rs:3738` — frontend path (artifact upload; `RequestNotFound` swallowed)
- `src/hooks/useWebSocketUtilities.ts:644` — optimistic `set_session_mode` before backend ack
- `src-tauri/app-skills/orchestrate/SKILL.md` — "you are the approver, so a human doesn't have to be"

  Generated via Copilot (Claude Opus 5) on behalf of @tclem

贡献指南

打开贡献指南

调研方向

首先跟踪 src-tauri/src/session/handler.rs 中的 ExitPlanModeHandler::handle,经过 src-tauri/src/session/manager/pending_requests.rs 中的 resolve_exit_plan_mode,然后比较 manager/mod.rs、handlers/session.rs 和 useWebSocketUtilities.ts 中的 parent 路径与 frontend 路径。查看 app-skills/orchestrate/SKILL.md 中的编排指导。完成的标准是:审批归属明确无歧义,两条路径具有等效的效果,客户端能够获知计划何时已解决,并且延迟响应可见,同时不会应用过时的副作用。

由索引模型根据 Issue 内容生成。

评估

技术栈
rust, tauri, typescript
领域
backend, desktop, frontend
Issue 类型
缺陷
难度
5/5
预计耗时
一周以上
活跃度
冷清
描述清晰度
基本清楚
新手友好度
38/100

把新 issue 发到你的邮箱

精选适合新手参与的 GitHub issue 摘要。