Repository navigation
Conversation
|
Important Review skippedWe couldn't safely recover the incremental review. No full review was started, and the last reviewed checkpoint was preserved. Retry later, or explicitly request a full review by commenting You can disable this status message by setting the Use the checkbox below for a quick retry:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info
📝 Walkthrough
Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to
|
| Check name | Status | Explanation | Resolution |
|---|---|---|---|
| Docstring Coverage |  |
Docstring coverage is 2.00% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 50 functions across 40 files. | Write docstrings for the functions missing them to satisfy the coverage threshold. |
-
Check name Status Explanation Linked Issues check The PR implements the subscription option in issue #18206. Verified terminal identities can subscribe, inspect status, and unsubscribe. Live, idle terminal mailboxes can receive a short pointer while …Out of Scope Changes check The changed production files implement terminal mailbox subscriptions, pointer delivery, capability negotiation, RPC and CLI exposure, identity validation, replay handling, and delivery-state safety f… Title check The title is concise, specific, and accurately describes the primary change: receiver-initiated subscriptions for terminal mailbox pointers. Description check The description includes all required sections, links issue #18206, explains the change and rationale, documents testing and limitations, and records AI use. It also clearly states that full repositor…Full details: Linked Issues check
-
Explanation
The PR implements the subscription option in issue
#18206. Verified terminal identities can subscribe, inspect status, and unsubscribe. Live, idle terminal mailboxes can receive a short pointer while the message remains in the durable inbox. The implementation preserves opt-in behavior and adds authority checks, replay handling, generation fences, deferred outcomes, and automated unit, RPC, CLI, and end-to-end tests. The separateterminal send --enterstalled-payload addendum is not required for this subscription objective.
Full details: Out of Scope Changes check
-
Explanation
The changed production files implement terminal mailbox subscriptions, pointer delivery, capability negotiation, RPC and CLI exposure, identity validation, replay handling, and delivery-state safety for issue
#18206. Documentation and test helpers support the feature. The supplied whole-PR summary shows no demonstrated unrelated production change.
Full details: Description check
-
Explanation
The description includes all required sections, links issue
#18206, explains the change and rationale, documents testing and limitations, and records AI use. It also clearly states that full repository and fresh platform coverage were not completed.
-
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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 1667cf5e-352c-44a4-a9ea-d5e37b1405ce
⛔ Files ignored due to path filters (1)
src/shared/rpc-contract/rpc-params-catalog.generated.tsis excluded by!**/*.generated.*
📒 Files selected for processing (35)
docs/reference/terminal-mailbox-subscriptions.mdsrc/cli/handler-group-manifest.tssrc/cli/handlers/orchestration.tssrc/cli/handlers/orchestration/subscription-handlers.test.tssrc/cli/handlers/orchestration/subscription-handlers.tssrc/cli/specs/orchestration.tssrc/main/runtime/orca-runtime-stop-requested-pty-ids.tssrc/main/runtime/orca-runtime-write-orchestration-pointer-pty.tssrc/main/runtime/orchestration/formatter.tssrc/main/runtime/orchestration/mailbox-pointer-delivery-contract.tssrc/main/runtime/orchestration/mailbox-pointer-delivery.tssrc/main/runtime/orchestration/mailbox-pointer-resume.tssrc/main/runtime/orchestration/mailbox-pointer-stage.tssrc/main/runtime/orchestration/mailbox-pointer-submit.test.tssrc/main/runtime/orchestration/mailbox-pointer-submit.tssrc/main/runtime/orchestration/terminal-mailbox-subscriptions.test.tssrc/main/runtime/orchestration/terminal-mailbox-subscriptions.tssrc/main/runtime/rpc/methods/orchestration.tssrc/main/runtime/rpc/methods/orchestration/messaging/subscription-methods.test.tssrc/main/runtime/rpc/methods/orchestration/messaging/subscription-methods.tssrc/main/runtime/rpc/orchestration-mutation-executor.tssrc/main/runtime/rpc/orchestration-mutation-receipt.tssrc/main/runtime/terminal-mailbox-subscription-authority.test.tssrc/shared/orchestration-rpc-contract.test.tssrc/shared/orchestration-rpc-contract.tssrc/shared/protocol-version.tssrc/shared/rpc-contract/terminal-subscription-params.tssrc/shared/terminal-mailbox-subscription.tstests/e2e/helpers/orchestration-mail-pane-agent.tstests/e2e/helpers/orchestration-mail-pane-python-agent.tstests/e2e/helpers/ssh-test-target-connection.tstests/e2e/orchestration-terminal-mailbox-subscription-live-agents.spec.tstests/e2e/orchestration-terminal-mailbox-subscription-ssh.spec.tstests/e2e/orchestration-terminal-mailbox-subscription-wsl.spec.tstests/e2e/orchestration-terminal-mailbox-subscription.spec.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 9 remain after this review.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
This PR's documented replay contract — "a completed replay does not reapply the ephemeral mutation" — holds on the unary dispatcher but not on the streaming/WebSocket dispatcher, which never forwards the replay marker the new subscription handlers read. One-line fix below, with a reachability caveat.
Reviewed changes
- Receiver-initiated subscription verbs —
orchestration subscribe|unsubscribe|subscription statusCLI handlers and RPC methods, capability-gated onorchestration.terminal-subscription.v1. - Authority binding —
TerminalMailboxSubscriptionsbinds host scope, handle, pane key, PTY, and process incarnation;getTerminalMailboxSubscriptionBindingre-verifies the caller and current dispatch authority before every read/write and fails closed on any drift. - Bare-terminal pointer delivery —
isTerminalMailboxgates threaded through delivery/stage/submit/resume; existingrun:/dispatch:behavior is unchanged. - Generation fence — async pointer/Enter completions carry the registration generation and are dropped once a re-registration moves it.
- Durable replay binding — subscribe/unsubscribe payload hashes include the verified receiver binding; completed replays return current status plus
historicalReplay. - Formatter, docs, and E2E — bare-terminal inbox pointer text, a reference doc, and local/SSH/WSL/live-agent E2E coverage.
⚠️ Streaming/WebSocket dispatch drops the replay marker, so a replayed subscription mutation re-applies
A completed replay of orchestration.subscribe/orchestration.unsubscribe that arrives over the streaming dispatcher re-enters the handler's fresh branch: it re-runs register/remove and deliverPendingMessagesForHandle instead of returning the current-status-plus-historicalReplay split. This is the exact invariant the PR documents and asks reviewers to focus on. Reachability requires a streaming/WebSocket caller that can supply valid orchestrationCompatibilityEvidence — I could not confirm such a caller exists in production (mobile is allowlisted away from these methods and I did not verify the CLI's remote transport), so treat this as a conformance gap to confirm or guard rather than a proven outage.
Technical details
# Subscription replays re-apply over the streaming dispatcher
## Affected sites
- `src/main/runtime/rpc/rpc-streaming-dispatcher.ts:125-157` — the `invoke` context passes `recordMutationReceipt` but not `replayedMutationReceipt`.
- `src/main/runtime/rpc/dispatcher-unary-method-invocation.ts:72` — the unary path does pass `replayedMutationReceipt: mutation?.replayedReceipt`, which is why the unary replay tests pass.
- `src/main/runtime/rpc/methods/orchestration/messaging/subscription-methods.ts:17-27` / `43-53` — the handler only takes the "current status + historicalReplay" path when `replayedMutationReceipt !== undefined`.
- `src/main/runtime/rpc/orchestration-mutation-receipt.ts` (`shouldObserveCompletedMutation`) returns `true` for subscription methods, which is what forces the handler to run on replay; the streaming dispatcher then re-applies because the marker is missing.
- `src/main/runtime/runtime-rpc/runtime-rpc-websocket-dispatch.ts:135` routes every paired WebSocket request through `dispatchStreaming`.
## Required outcome
A completed replay of `orchestration.subscribe`/`orchestration.unsubscribe` must not re-apply the ephemeral mutation and must return the historical/current split on every dispatch path, not just the unary one.
## Suggested approach
Pass `replayedMutationReceipt: mutation?.replayedReceipt` in the streaming `invoke` context so both dispatchers build the same handler context.
## Open questions for the human
- Is there any production caller that reaches `orchestration.subscribe`/`unsubscribe` via `dispatchStreaming` (paired web/desktop scope, remote CLI transport) while able to present a valid launch token? If not, a short comment stating the unary-only assumption would also resolve it.ℹ️ Nitpicks
src/main/runtime/orchestration/formatter.ts:117re-implements theterm_*pattern inline instead of importingisTerminalMailbox, so the handle grammar now lives in two places and can drift.TerminalMailboxSubscriptions.hostScopesEqual(terminal-mailbox-subscriptions.ts:35-40) compares withJSON.stringifywhile the runtime already exposesorchestrationCompatibilityHostScopesEqual; both sides are built from one shape today, so this is only a key-order/drift nit.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
ℹ️ No new issues in this delta. The replay-consistency gap raised against
4cc16deeis unchanged by8f457cf6faand remains open.
Reviewed changes
Since the prior review (4cc16dee), one commit landed: 8f457cf6fa — fix(orchestration): preserve subscriptions until certified exit.
- Certified-exit gate —
onPtyExitnow retires mailbox subscriptions only when process death is certified (exitCode >= 0, orhostExitConfirmed/providerExitObserved); an unverifiable exit leaves the subscription inhost_unverifiablerather thanproven_exited. - Delivery-uncertainty preservation —
refreshCurrentonly restoreshost_unverifiable→activewhen the reason iscurrent_authority_unverifiable, so delivery-derived uncertainty (current_target_unverifiable,pty_not_writable,live_idle_not_observed) is no longer silently cleared. - Tests — new
pty-exit-agent-status-reconciliation.test.tspins the retire-only-on-certified-exit matrix; the authority tests pin preservation and authority-restore. - E2E hardening — the SSH lane no longer defaults to a hardcoded host and skips when none is configured; WSL cleanup moved into
finallyso a pre-launch failure still tears down.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior pullfrog review (8f457cf6fa), one commit landed: 03088de5f9 — fix(orchestration): preserve subscription replay over streaming RPC. It closes the replay-consistency gap raised against 4cc16dee.
- Streaming replay marker restored —
RpcStreamingDispatchernow forwardsreplayedMutationReceipt: mutation?.replayedReceiptinto the invoke context, so a replayedorchestration.subscribe/orchestration.unsubscribeover the streaming/WebSocket path returns live status plushistoricalReplayinstead of re-applying the ephemeral mutation. The parameterized test proves this: removing the line makes thestreamingcase fail (historicalReplayabsent, stalesubscribedvalue). - Host-scope comparison unified — the inline
JSON.stringifycheck and the runtime verifier's copy were replaced by a sharedorchestrationCompatibilityHostScopesEqual(src/main/runtime/orchestration-compatibility-host-scope.ts), now used by the verifier and byTerminalMailboxSubscriptions.matches/canManage, eliminating key-order drift. - Formatter reuses the canonical predicate —
formatMessagePointercallsisTerminalMailboxrather than re-inlining theterm_*regex. - Tests — new
orchestration-compatibility-host-scope.test.ts;subscription-methods.test.tsnow exercises both transports viait.each(['unary', 'streaming']).
All four CodeRabbit threads and both prior pullfrog nitpicks are addressed in this delta.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
ELI5
An agent can receive durable Orca mail today, but an idle bare terminal has no safe signal that mail is waiting. This adds an explicit receiver subscription: after the agent opts in, Orca may type a short inbox pointer into that same terminal when it is live and idle. The message body remains in the inbox.
What Changed
orchestration subscribe,orchestration subscription status, andorchestration unsubscribefor the current Orca-launched terminal.term_*recipients.Why
orchestration senddurably stores mail, but an idle Claude Code or Codex terminal may not inspect its inbox until a person gives it another turn. Sender-driven raw PTY input cannot prove receiver intent and can duplicate or misroute input. Receiver opt-in preserves durable mail as the source of truth while reusing Orca's existing pointer delivery and safety gates.Linked Issue
Fixes #18206
Visual Proof
N/A — this adds CLI/RPC behavior and hidden terminal delivery; it does not change rendered UI.
Testing
Current-main conflict refresh (2026-10-18, main baseline
33e6785fe2c3):Previous integration validation (2026-10-14, main baseline
efbe3210d68d):pnpm test:node, covering subscription authority, native PTYs without a renderer, pointer staging/submission, ambiguous-write reservations, mutation replay, CLI/RPC contracts, and PTY exit handling.pnpm tc:node,pnpm tc:cli, RPC catalog verification, changed-code quality, and diff checks passed.The SSH fixture now uses the current managed-server architecture. The execution host can resolve an explicitly subscribed native PTY without an attached renderer; the existing identity, ownership, settled-idle, and write-reservation checks still apply. The portable CI fixture uses Docker, while this validation ran over SSH against a native Ubuntu ARM64 host.
Historical validation before this main merge included 14 macOS Electron/PTY cases, live Codex 0.155.0 and Claude Code 2.1.278, macOS/Linux SSH, Windows SSH/ConPTY, and WSL. Those broader platform/provider cases were not all rerun on the updated architecture and are not claimed as fresh results. The old relay-specific #22112 dependency no longer describes the current managed-server path.
Antigravity CLI 1.2.7 remains outside the v1 automatic-wake scope: earlier live runs lost current terminal authority after subscription, so mail stayed unread and no pointer or Enter was written. The reference document directs those sessions to inspect the inbox explicitly.
At the previous integration, the full repository E2E TypeScript check reported 264 diagnostics outside this feature's changed files; no fresh clean-base comparison or full-repository pass was established. That full E2E typecheck was not rerun in this conflict refresh. Fresh live platform/vendor-TUI coverage remains for follow-up/CI.
AI Disclosure
OpenAI Codex assisted with implementation, test construction, review, and validation. Independent Codex sessions reviewed the authority, lifecycle, mutation-replay, and ambiguous-write boundaries through five read-only review passes.
Review
Please focus on the receiver authority binding, same-incarnation ambiguous-write fence, completed mutation replay semantics, and the distinction between durable enqueue and best-effort wake status.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Notes
Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)