Repository navigation
Implement robust orchestration primitives and connected-server workers - #9925
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe pull request adds Run-scoped orchestration, durable inbox delivery, question threads, mutation receipts, worker lifecycle management, federation relay and recovery, structured worker reports, and capability-aware RPC transport. It updates CLI handlers, command specifications, help text, skill guidance, development wrappers, runtime behavior, database migrations, and extensive unit/integration coverage. Documentation and implementation ledgers describe the orchestration phases, contracts, acceptance criteria, and progress history. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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: 9
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/cli/help.ts (1)
88-103: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winExpose the worker lifecycle commands in root help.
The new Run commands are listed, but the normal supervised workflow’s
worker-startand relatedworker-show/worker-read/worker-stop/worker-abandoncommands remain undiscoverable throughorca --help. Add them beside the other orchestration commands.src/main/runtime/orca-runtime.ts (1)
17126-17200: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
setupReceipt.stateis inconsistent between the local and remote worktree-create paths, and doesn't distinguish a policy-declined run from a real spawn failure.The local path (
createManagedWorktree, ~17191-17197) explicitly mapseffectiveDecision === 'skip'to'skipped', but even there a hook that exists yet wasn't run only becauseshouldRunSetupForCreatedeclined it (reposetupRunPolicy, decision'inherit') falls through todidSpawnSetup ? 'running' : 'spawn_failed'— reporting'spawn_failed'for a setup that was never attempted. The remote path (createManagedRemoteWorktree, ~17531-17535) is missing the'skip'branch entirely, so an explicitsetupDecision: 'skip'on a remote worktree create can report'not_configured'or'spawn_failed'instead of'skipped'. Given the PR's stated goal of "truthful worker outcomes," this receipt should report the same semantics for both paths.🩹 Proposed fix: mirror the local `'skip'` branch in the remote path
const setupReceipt = { requested: (args.runHooks ? 'run' : (args.setupDecision ?? 'inherit')) as | 'run' | 'skip' | 'inherit', hookFound: Boolean(result.setup), startupPolicy: result.setup?.waitForAgentStartup ? ('wait-for-setup' as const) : ('start-immediately' as const), state: !result.setup ? ('not_configured' as const) + : (args.runHooks ? 'run' : (args.setupDecision ?? 'inherit')) === 'skip' + ? ('skipped' as const) : didSpawnSetup ? ('running' as const) : ('spawn_failed' as const) }Also applies to: 17522-17540
🧹 Nitpick comments (6)
src/cli/runtime/client.ts (2)
249-263: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a "why" comment for the check/dispatch special-casing.
The rationale for treating
orchestration.checkas a mutation only whenackis present, andorchestration.dispatchas a mutation unlessdryRun, isn't obvious from the code alone.As per path instructions: "Add concise, preferably one-line comments only for non-obvious code, explaining why rather than how."
📝 Suggested comments
function isOrchestrationMutation(method: string, params: unknown): boolean { + // Why: `check` only mutates state when it acknowledges a delivery (--ack); + // a plain read/poll needs no recovery id. if (method === 'orchestration.check') { return Boolean( params && typeof params === 'object' && typeof (params as { ack?: unknown }).ack === 'string' ) } + // Why: `--dry-run` dispatch never mutates state, so it's excluded. if (method === 'orchestration.dispatch') {
54-58: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winOrchestration envelope type is duplicated inline across three files. The
{ orchestrationCapability?: string; orchestrationRequestId?: string }shape is repeated independently instead of sharing one type, risking drift as fields are added (this shape also appears insrc/shared/remote-runtime-client.ts'ssendRemoteRuntimeRequest).
src/cli/runtime/client.ts#L54-L58: import/use a sharedOrchestrationEnvelopetype for theoptions/envelopeshape instead of the inline literal.src/cli/runtime/transport.ts#L11-L12: use the same shared type forsendRequest'senvelopeparameter.src/cli/runtime/websocket-transport.ts#L12-L13: use the same shared type forsendWebSocketRequest'senvelopeparameter.src/cli/specs/orchestration.ts (1)
40-73: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
allowedFlagsincludes flags that theusagehelp string never documents. Users relying on--help/usage text won't discover these accepted flags.
src/cli/specs/orchestration.ts#L40-L73:send'sallowedFlagsincludes'dispatch-capability', but theusagestring omits--dispatch-capability; add it to the usage text.src/cli/specs/orchestration.ts#L180-L200:ask'sallowedFlagsincludes'dispatch-capability', but theusagestring omits it; add it to the usage text.src/cli/specs/orchestration-worker-specs.ts#L5-L35:worker-start'sallowedFlagsincludes'display-name'and'comment', but theusagestring only shows--name; add--display-name/--commentto the usage text.src/main/runtime/rpc/methods/orchestration-worker-control.ts (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
WorkerDispatchParamsis defined identically in two files. Both independently declarez.object({ dispatch: requiredString('Missing --dispatch') }), risking divergence (e.g. inconsistent error messages) as the schema evolves.
src/main/runtime/rpc/methods/orchestration-worker-control.ts#L13-L13: import a sharedWorkerDispatchParamsfrom a common schemas module instead of redeclaring it.src/main/runtime/rpc/methods/orchestration-worker-stop.ts#L11-L11: import the same shared schema.src/main/ipc/runtime-environment-transport-routing.ts (1)
109-120: 🚀 Performance & Scalability | 🔵 TrivialEnvelope-bearing calls always pay for a fresh one-shot WebSocket handshake.
When
envelopeis set, this branch always usessendRemoteRuntimeRequest(new handshake per call) rather than the cached-connection or shared-control paths used by other methods. If federation lifecycle/question relay traffic is frequent, this adds per-call connection setup latency. Worth confirming this is an accepted trade-off (envelope auth isn't threaded through the persistent shared-control channel) rather than an oversight, and considering connection reuse for envelope calls later if call volume grows.src/main/runtime/orchestration/db.ts (1)
194-198: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
messagesCHECK omits'question'increateTables().The canonical table definition allows
heartbeatbut notquestion, even thoughcreateQuestion()insertstype: 'question'. It currently works only because the omission forces the v9 rebuild (Line 552!messagesTypeCheckAllowsQuestion()) to fire on every fresh database, doing an unnecessary DROP/recreate. Add'question'here so the authoritative schema matches the intended state and fresh DBs skip the rebuild.♻️ Add 'question' to the canonical CHECK
CHECK(type IN ( 'status', 'dispatch', 'worker_done', 'merge_ready', - 'escalation', 'handoff', 'decision_gate', 'heartbeat' + 'escalation', 'handoff', 'decision_gate', 'question', 'heartbeat' )),
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: ce27697d-882f-43d0-95b6-ecb6eb598488
⛔ Files ignored due to path filters (1)
src/main/runtime/orchestration/__snapshots__/preamble.test.ts.snapis excluded by!**/*.snap
📒 Files selected for processing (76)
ORCHESTRATION_IMPLEMENTATION_CHECKLIST.mdconfig/scripts/orchestration-skill-guidance.test.mjsresources/skills/current-manifest.jsonresources/skills/release-mapping.jsonresources/skills/snapshot-registry.jsonskill-guides/orchestration.mdskills/orchestration/SKILL.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-lifecycle-rejection.test.tssrc/cli/handlers/orchestration-run-cli.test.tssrc/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration.tssrc/cli/help.tssrc/cli/index.test.tssrc/cli/runtime-client.test.tssrc/cli/runtime/client.tssrc/cli/runtime/transport.tssrc/cli/runtime/websocket-transport.tssrc/cli/specs/orchestration-worker-specs.tssrc/cli/specs/orchestration.tssrc/main/index.tssrc/main/ipc/runtime-environment-shared-control-support.tssrc/main/ipc/runtime-environment-transport-routing.tssrc/main/runtime/orca-runtime.test.tssrc/main/runtime/orca-runtime.tssrc/main/runtime/orchestration-cli-subprocess.test.tssrc/main/runtime/orchestration/coordinator.test.tssrc/main/runtime/orchestration/coordinator.tssrc/main/runtime/orchestration/db.test.tssrc/main/runtime/orchestration/db.tssrc/main/runtime/orchestration/environment-transport.tssrc/main/runtime/orchestration/federation-sync.tssrc/main/runtime/orchestration/formatter.test.tssrc/main/runtime/orchestration/lifecycle-reconciliation.test.tssrc/main/runtime/orchestration/lifecycle-reconciliation.tssrc/main/runtime/orchestration/orchestration-db-permissions.test.tssrc/main/runtime/orchestration/orchestration-error.tssrc/main/runtime/orchestration/orchestration-mutation-question-db.test.tssrc/main/runtime/orchestration/orchestration-run-delivery-db.test.tssrc/main/runtime/orchestration/orchestration-worker-dispatch-db.test.tssrc/main/runtime/orchestration/preamble.test.tssrc/main/runtime/orchestration/preamble.tssrc/main/runtime/orchestration/types.tssrc/main/runtime/rpc/core.tssrc/main/runtime/rpc/dispatcher.tssrc/main/runtime/rpc/errors.tssrc/main/runtime/rpc/methods/orchestration-federated-worker-start.tssrc/main/runtime/rpc/methods/orchestration-federation-control.tssrc/main/runtime/rpc/methods/orchestration-federation-effects.tssrc/main/runtime/rpc/methods/orchestration-federation-methods.tssrc/main/runtime/rpc/methods/orchestration-federation-relay.tssrc/main/runtime/rpc/methods/orchestration-federation.test.tssrc/main/runtime/rpc/methods/orchestration-federation.tssrc/main/runtime/rpc/methods/orchestration-runs.tssrc/main/runtime/rpc/methods/orchestration-worker-control.tssrc/main/runtime/rpc/methods/orchestration-worker-methods.tssrc/main/runtime/rpc/methods/orchestration-worker-observation.tssrc/main/runtime/rpc/methods/orchestration-worker-start-schema.tssrc/main/runtime/rpc/methods/orchestration-worker-stop.tssrc/main/runtime/rpc/methods/orchestration-worker-topology.tssrc/main/runtime/rpc/methods/orchestration-workers-new-worktree.test.tssrc/main/runtime/rpc/methods/orchestration-workers-recovery.test.tssrc/main/runtime/rpc/methods/orchestration-workers.tssrc/main/runtime/rpc/methods/orchestration.test.tssrc/main/runtime/rpc/methods/orchestration.tssrc/main/runtime/rpc/orchestration-mutation-executor.tssrc/main/runtime/rpc/orchestration-mutation-ledger.test.tssrc/main/runtime/rpc/runtime-feature-interaction.tssrc/main/ssh/ssh-remote-cli-error-response.tssrc/main/ssh/ssh-remote-orca-cli.test.tssrc/main/ssh/ssh-remote-orca-cli.tssrc/main/ssh/ssh-remote-orchestration-send.test.tssrc/main/ssh/ssh-remote-orchestration-send.tssrc/shared/protocol-version.tssrc/shared/remote-runtime-client.tssrc/shared/types.ts
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ORCHESTRATION_IMPLEMENTATION_CHECKLIST.md (1)
71-73: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winUpdate the stale acknowledgment guidance.
Line [73] says the consume-before-response crash window remains until explicit acknowledgment ships, but Lines [119-123] document that acknowledgment is already implemented. Reword this item to describe the current replay/acknowledgment behavior rather than a future feature.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 2b31e63b-5507-4545-9e6f-bf76b24f8f65
📒 Files selected for processing (7)
ORCHESTRATION_IMPLEMENTATION_CHECKLIST.mdconfig/scripts/orca-dev-bin.test.mjsconfig/scripts/orca-dev.mjssrc/cli/handlers/orchestration.tssrc/cli/index.test.tssrc/shared/agent-prompt-injection.test.tssrc/shared/agent-prompt-injection.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
config/scripts/dev-cli-terminal-wrapper.test.mjs (1)
8-22: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the
out/bincopies too.These tests only validate
userDataPath/cli/bin, butconfig/scripts/run-electron-vite-dev.mjsputs the returnedbinDirfirst onPATH. Assert that eachout/binwrapper matches the profile copy so the Windows routing regression cannot recur unnoticed.Proposed test addition
- prepareDevCliTerminalWrappers({ + const { binDir } = prepareDevCliTerminalWrappers({ repoRoot: root, userDataPath, electronExecutable: path.join(root, 'electron.exe'), platform: 'win32' }) @@ expect(readFileSync(path.join(userDataPath, 'cli', 'bin', 'orca.cmd'), 'utf8')).toBe(wrapper) + expect(readFileSync(path.join(binDir, 'orca-dev.cmd'), 'utf8')).toBe(wrapper) + expect(readFileSync(path.join(binDir, 'orca.cmd'), 'utf8')).toBe(wrapper)Also applies to: 25-41
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro
Run ID: 75261024-4062-41a4-be32-21e008d55861
📒 Files selected for processing (5)
ORCHESTRATION_IMPLEMENTATION_CHECKLIST.mdconfig/scripts/dev-cli-terminal-wrapper.mjsconfig/scripts/dev-cli-terminal-wrapper.test.mjsconfig/scripts/orca-dev.mjsconfig/scripts/run-electron-vite-dev.mjs
AmethystLiang
added
the
orca-remote-server
8dd0d1b to
58c0365
Compare
|
@coderabbitai review |
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 21239489-ea24-4938-89bf-113269e7cfff
📒 Files selected for processing (25)
ORCHESTRATION_IMPLEMENTATION_CHECKLIST.mdORCHESTRATION_STRUCTURED_OUTPUT_DESIGN.mdconfig/scripts/dev-cli-terminal-wrapper.mjsconfig/scripts/dev-cli-terminal-wrapper.test.mjsconfig/scripts/orca-dev-bin.test.mjsconfig/scripts/orca-dev.mjsconfig/scripts/orchestration-skill-guidance.test.mjsconfig/scripts/run-electron-vite-dev.mjsdocs/orchestration-primitives.htmlskill-guides/orchestration.mdsrc/cli/bundled-skill-guides.tssrc/cli/handlers/orchestration-lifecycle-rejection.test.tssrc/cli/handlers/orchestration-run-cli.test.tssrc/cli/handlers/orchestration-timeout-cli.test.tssrc/cli/handlers/orchestration-worker-cli.test.tssrc/cli/handlers/orchestration.test.tssrc/cli/handlers/orchestration.tssrc/cli/help.tssrc/cli/index.test.tssrc/cli/runtime-client.test.tssrc/cli/runtime/client.tssrc/cli/runtime/transport.tssrc/cli/runtime/websocket-transport.tssrc/cli/specs/orchestration-worker-specs.tssrc/cli/specs/orchestration.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- src/cli/runtime/websocket-transport.ts
- src/cli/handlers/orchestration-lifecycle-rejection.test.ts
- src/cli/specs/orchestration-worker-specs.ts
- config/scripts/orca-dev-bin.test.mjs
- config/scripts/dev-cli-terminal-wrapper.mjs
- src/cli/help.ts
- src/cli/handlers/orchestration-run-cli.test.ts
- config/scripts/orca-dev.mjs
- src/cli/runtime/transport.ts
- config/scripts/run-electron-vite-dev.mjs
- config/scripts/orchestration-skill-guidance.test.mjs
- src/cli/handlers/orchestration-timeout-cli.test.ts
- src/cli/bundled-skill-guides.ts
- src/cli/runtime/client.ts
- skill-guides/orchestration.md
- src/cli/handlers/orchestration.test.ts
- src/cli/specs/orchestration.ts
- src/cli/handlers/orchestration.ts
- config/scripts/dev-cli-terminal-wrapper.test.mjs
e8b0257 to
6a6189e
Compare
b69477d to
817261f
Compare
817261f to
68ba7c4
Compare
Resolve conflicts from main's lazy CLI dispatch refactor (#? handler-group manifest) and orchestration-envelope client changes (stablyai#9925): - dispatch.ts: adopt main's manifest-driven routing; register the updater commands by adding a `{ name: 'updater', keys: ['version','update'] }` group to handler-group-manifest.ts (verified by handler-group-manifest.test.ts). - client.ts: keep both the remote-server-update imports and main's new orchestration-envelope imports. Extract the remote status→CliStatus mapping to remote-cli-status.ts to stay under the max-lines budget (no baseline/disable). Updater reconciliation (revision long-poll + install-mode/SSH gate) is intact. Verified: tc:cli, tc:node, oxlint, and the scoped CLI/updater/manifest vitest suites pass (312 CLI tests green). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…r workers (stablyai#9925)" This reverts commit cd05f2f for the v1.4.160-rc.4 cut. Three files needed manual resolution because later commits touched the same regions. Their post-9925 work is preserved: - src/main/runtime/rpc/core.ts and dispatcher.ts keep clientCapabilities and the RuntimeCapability import from stablyai#10129 (close-intent negotiation for paired runtimes). - src/cli/runtime/client.ts keeps the deferred websocket-transport import from stablyai#10919.
v1.4.161's release branch carried upstream's revert of stablyai#9925; main inherited it via that sync. v1.4.162 ships orchestration again, but git resolved the files upstream did not also modify as plain deletions, so no conflict flagged them and the subsystem merged back half-restored. The fork owns no orchestration code, so all 87 take v1.4.162 verbatim.
Summary
worker-start/show/read/stop/abandonflows for current, existing, child, and top-level worktrees while preserving low-level worktree and terminal commandsworker-start --on <saved-environment>, keeping Run/Task truth on the home server while authenticated lifecycle, questions, control mail, recovery, and output cross the server boundaryORCHESTRATION_IMPLEMENTATION_CHECKLIST.mdDesign boundaries
This intentionally does not add a dashboard, scheduler, automatic placement or retry, capacity allocator, commit/integration tracking, filesystem access enforcement, generalized ACLs, Run replication/failover, dead letters, or a universal provider-session abstraction. Agents still choose decomposition, placement, concurrency, conflict avoidance, and recovery.
For federation, the Run home owns orchestration state. A connected worker server owns exact resources, an authenticated Dispatch attachment, and durable relay state. Runtime UUIDs are epochs; saved-server public-key fingerprints pin peer identity.
Screenshots
Not applicable. This PR adds runtime, CLI, and skill primitives and intentionally makes no product UI changes.
Testing
git diff --checkorigin/main, including a wire-level regression for bounded encrypted orchestration requestsAI review report
Multiple independent read-only reviews covered agent ergonomics, federation/recovery, skill guidance, and the final branch. All actionable review threads are resolved. The final reviewer reported no remaining correctness issue.
Cross-platform review explicitly covered macOS, Linux, and Windows path handling, shell behavior, connected-server routing, native/WSL/SSH execution boundaries, and Electron runtime differences. This PR changes no UI or keyboard shortcuts.
Security audit
Notes
All planned implementation phases are complete. Exact structured worker output is no longer deferred: Orca selects a proven Codex/Claude transcript when available and otherwise returns bounded terminal output with a typed fallback reason.
Local CLI installation can warn when the current user cannot replace
/usr/local/bin/orca-dev; that does not affect the CLI build artifact or CI.