Repository navigation
fix(orchestration): stop the sender-terminal refusal recommending another pane's handle - #21097
Conversation
…ther pane's handle The structured-session guard told callers to pass `--from <terminal-handle>`, but the explicit-flag branch returns before that guard runs — so following the advice succeeds, against a handle that necessarily belongs to a different pane, and the next `check` consumes that pane's unread mail. Both refusals now say what is actually true: no handle names a structured chat session, and a caller that does have one should pass its own. Also pins ORCA_STRUCTURED_SESSION in the gate CLI test, which until now decided which refusal it exercised from ambient environment.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ No critical issues — the copy change is correct and well-reasoned. One follow-up: the same refusal message survives unqualified on the SSH fallback path.
Reviewed changes
- Structured-session refusal copy (
terminal-identity.ts) — replaces the "pass--from <handle>" remediation with the real constraint (no handle names the session) plus a direct-drive workaround. throwNoActiveSenderTerminalcopy — qualifies--fromas your own terminal's handle and warns that another pane's handle is not a substitute.- Test env pinning (
orchestration-gate-cli.test.ts) — pinsORCA_STRUCTURED_SESSIONso the unboundgate-createcase deterministically hitsthrowNoActiveSenderTerminal, and updates the assertion to the new string.
️ The SSH fallback still ships the old, unqualified advice
src/main/ssh/ssh-remote-orchestration-send.ts:29-31 carries a byte-identical copy of the message this PR just fixed: Pass --from <terminal-handle> or run the command inside a live Orca terminal with ORCA_TERMINAL_HANDLE set. It fires on the same lifecycle-send path (worker_done / heartbeat with no explicit or env handle) when the request is served through the legacy SSH bridge, so the exact misleading advice this PR removes from the local CLI survives on the SSH execution path. AGENTS.md asks that every change consider the SSH use case, and here the two strings are twins that should move together.
Technical details
# SSH fallback keeps the pre-fix refusal copy
## Affected sites
- `src/main/ssh/ssh-remote-orchestration-send.ts:29-31` — `resolveRemoteOrchestrationSender` throws `RemoteCliArgumentError('no_active_sender_terminal', 'Could not determine the sender terminal for this orchestration command. Pass --from <terminal-handle> or run the command inside a live Orca terminal with ORCA_TERMINAL_HANDLE set.')`. Reached via `ssh-remote-orca-cli.ts:211` for `orchestration send`.
- `src/cli/handlers/orchestration/terminal-identity.ts:198-200` — the local-CLI twin, updated by this PR.
## Required outcome
- An identity-less lifecycle send over the SSH bridge must not suggest `--from <terminal-handle>` without the same qualifier the local CLI now carries (or points at the same direct-drive remediation).
## Suggested approach (optional)
- Mirror the updated `throwNoActiveSenderTerminal` wording in `resolveRemoteOrchestrationSender`, or extract the string into one shared constant so the two copies cannot drift again. Its test (`ssh-remote-orchestration-send.test.ts:26-30`) asserts only the error code, so either way stays green.ℹ️ Nitpicks
- The sentence
every live handle belongs to a different pane, whose mailbox passing it would consume(terminal-identity.ts:44-45) is hard to parse because the relative clause splitswhose mailboxfrompassing it would consume. Something like— every live handle names a different pane, and passing it would consume that pane's mailboxsays the same thing in one pass.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (3)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe CLI now provides specific guidance for missing terminal identity and for sender handles that belong to another pane. Structured-session errors now direct workers to launch through a worktree with Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The revised refusal guidance remains appropriate for CLI users and introduces no actionable merge risk. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description clearly explains the problem, user impact, implementation, testing, and visual-proof status. However, it omits the required Linked Issue entry and does not include the required checklist sections.
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
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[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: 05015c51-b910-4cdd-9c07-99ca78d8e303
📒 Files selected for processing (2)
src/cli/handlers/orchestration-gate-cli.test.tssrc/cli/handlers/orchestration/terminal-identity.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
…d message `orchestration send --type worker_done|heartbeat` refuses in the send handler before `resolveOrchestrationTerminalHandle` runs, so the structured guard never saw the case a structured session hits most: the canonical worker lifecycle report. That caller was still told to pass `--from` with "your own terminal's handle" — which it does not have, so any handle it picked would belong to another pane. `throwNoActiveSenderTerminal` now derives which refusal fits instead of each call site deciding: marker set AND no handle means no identity exists, so the structured refusal applies. A stale `ORCA_TERMINAL_HANDLE` is deliberately excluded — that caller does have an identity, it just went stale, and keeps the advice to re-run under a live one. Also corrects the guidance itself (`--agent` is a `worktree create` flag; `terminal create` has no such flag), aligns the SSH fallback wording with its local twin, and pins ORCA_STRUCTURED_SESSION in the send tests, which until now decided which refusal they exercised from ambient environment.
|
Follow-up commit The gap this closesThe first commit fixed the refusal inside That is the same defect this PR set out to remove, on the path a worker takes most often. The change
A stale Three smaller corrections
TestsAdded a test pinning the property this PR is about: a structured session's refusal must not name a handle flag. Nothing covered that before, so a regression could have restored the old text with the suite still green. Removing the fix fails that test and only that test. Also pinned Validation
Known gap, not addressed hereThe SSH path has no structured-session check at all, because |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
The follow-up commit f70ed8c addresses the prior review and adds structured-session routing for lifecycle sends.
- SSH twin updated (
ssh-remote-orchestration-send.ts) — the legacy bridge now emits the same qualified "your own terminal's handle" wording as the local CLI, closing the drift the prior review flagged. - Structured refusal extracted and routed (
terminal-identity.ts) —structuredSessionRefusal()centralizes the "no handle names this session" copy, andthrowNoActiveSenderTerminal()now returns it for an identity-less structured session with noORCA_TERMINAL_HANDLEinstead of advising--from. - Regression coverage added — a new case pins that a structured lifecycle send does not recommend
Pass --from, andORCA_STRUCTURED_SESSIONis now pinned in the affected suites so the branch they exercise no longer depends on the runner's ambient environment. - Prior nitpick resolved — the hard-to-parse relative clause was reworded ("every live handle belongs to a different pane, and passing one would consume that pane's mailbox").
ℹ️ The PR description predates the second commit
The body's "What changed" still says throwNoActiveSenderTerminal "keeps suggesting --from", and neither it nor the "No behavior change" line mentions f70ed8c's env-conditional routing or the SSH twin update. The code is correct; the description just undersells the delta, and a reader could mistakenly believe the SSH path still ships the old advice. Worth a quick refresh.
Technical details
# Stale PR description
## Affected sites
- PR body, "## What changed" / "## The bug" — written against `ce4f4da`, before `f70ed8c` updated
`src/main/ssh/ssh-remote-orchestration-send.ts:29-31` and added the structured branch in
`throwNoActiveSenderTerminal` (`src/cli/handlers/orchestration/terminal-identity.ts:207-209`).
## Required outcome
- Description should mention the SSH copy fix and that the lifecycle-send refusal now routes to
the structured message for identity-less structured sessions.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
When a native chat session tries to coordinate orchestration, Orca correctly refuses — then tells you to do the one thing that actually breaks something. This fixes the advice, not the refusal.
The bug
orchestration run-createfrom a structured chat session failsno_active_sender_terminaland says:So you run
orca terminal list, pick a handle, and pass it. It works —resolveOrchestrationTerminalHandlereturns on the explicit-flag branch before ever reaching the structured guard.But a structured chat has no pane, so every handle you could have picked belongs to a different pane. The Run binds to that pane's mailbox, and because
orchestration checkis destructive by default, the next check consumes that worker's oldest unread batch and marks it read. The rightful worker never sees its mail.The message warns about acting on another pane's mailbox while handing you the way to do it.
Where it came from
The guard and its wording were introduced together in #18827 (
feat(orchestration): orchestrate native-born structured chat sessions, merged 2026-09-07). The guard itself is right and stays: a structured session genuinely cannot infer its identity, and refusing beats guessing onto a sibling. Only the remediation advice was wrong — it was written as though a handle existed that names the caller, and none does.What changed
The structured refusal now states the real constraint — no handle names this session — and points at the path that works: create a worktree, create a terminal in it with an agent, then drive it with terminal send/read.
throwNoActiveSenderTerminalkeeps suggesting--from, because its callers may genuinely have a handle, but now qualifies it as your own terminal's handle and says why another pane's is not a substitute.No behavior change: same code, same refusal, same exit path. Message text only.
Testing
src/cli/handlers/orchestration-gate-cli.test.tsasserted on the old string. Updated — and while there, pinnedORCA_STRUCTURED_SESSIONalongside the handle/pane-key vars the suite already controls.That test's outcome depended on ambient environment: run from a structured session it took the structured refusal, run from CI it took
throwNoActiveSenderTerminal. It passed either way only because both messages happened to containPass --from <terminal-handle>. Removing that coincidence exposed it, so the env is now pinned rather than inherited.28 files / 275 tests pass across
src/cli/handlers/orchestration,src/cli/orchestration, and the SSH orchestration suites.Verified in both arms — with
ORCA_STRUCTURED_SESSIONset and withenv -u ORCA_STRUCTURED_SESSION— 18/18 in the three directly affected suites either way.pnpm tc:cli,pnpm tc:node,pnpm run check:code-quality:changed(0 findings across 2 files), formatted.pnpm-lock.yamlabsent from every commit and from the full branch range.Visual proof
N/A — CLI error text; covered by the assertions above.