github / github/app

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

Đang mở
#2,353 0 bình luận 0 reaction 0 người được giao Xem trên GitHub
Bugs
Ngôn ngữ chính
Không có dữ liệu ngôn ngữ
Star
2.1k
Fork
153
Chỉ số merge pull request
Không có pull request nào được merge trong 30 ngày

Mô tả

### 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

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Hướng nghiên cứu

Start by tracing ExitPlanModeHandler::handle in src-tauri/src/session/handler.rs through resolve_exit_plan_mode in src-tauri/src/session/manager/pending_requests.rs, then compare the parent and frontend paths in manager/mod.rs, handlers/session.rs, and useWebSocketUtilities.ts. Review the orchestration guidance in app-skills/orchestrate/SKILL.md. Done means approval ownership is unambiguous, both paths have equivalent effects, clients learn when a plan is resolved, and late responses are visible without applying stale side effects.

Do mô hình lập chỉ mục viết ra từ nội dung của issue.

Đánh giá

Công nghệ
rust, tauri, typescript
Lĩnh vực
backend, desktop, frontend
Loại issue
Lỗi
Độ khó
5/5
Thời gian dự kiến
Hơn một tuần
Mức độ hoạt động
Ít trao đổi
Độ rõ ràng
Khá rõ ràng
Mức phù hợp với người mới
38/100

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.