Repository navigation
Conversation
#19689 shipped because no lane paired two builds over `orchestration.*`. The cross-version harness only covered the terminal stream and `agentSession.*`, and every federation "old peer" test wrote a protocol_version integer into a row and read it back with the same build, so a coordinator that omits a field and a host that starts requiring it were both invisible. Pairing against the newest tag alone would not have caught it either: the newest tag already contains the fix for anything broken and repaired inside the last release cycle. A worker host and the desktop that dispatches to it update on their own schedules, so N-1 is an ordinary peer. The suite therefore extracts the two newest stable tags and runs both skew directions against each. Each pairing drives one journey through the real coordinator modules — `startFederatedWorker` composes the attach params, `syncFederatedDispatch` drives the relay — against the peer build's real dispatcher over its real store, with a JSON round trip so an `undefined` field drops exactly as on the wire. What the old side has is read from its checkout; nothing about a release is written down. `pr-code-change-scope.mjs` did not route orchestration source at this job, so the lane would not have run on the PR that broke it; the federation source prefixes are added with cases in its test. The job's file list is now derived from the directory, so a new suite that is never named in the workflow fails instead of covering nothing.
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes — cross-version coverage for the orchestration federation surface, pairing current code against the two newest releases, plus the harness, lane wiring, scope classification, contract test, and docs that keep it honest.
cross-version-orchestration-wire.unit.test.ts— new suite (311 lines, 35 tests) pairingfederationAttachStart,federationFleetSnapshot,federationShow,federationRead, and thefederationPull/Ack/Importrelay loop against two baselines.orchestration-federation-peers.ts,orchestration-skew-journey.ts,versioned-orchestration-wire.ts— the peer/journey/loader harness that drives each build's realstartFederatedWorker,RpcDispatcher, andOrchestrationDbover a JSON round trip, so it fails on what a release actually sends.release-checkout.ts—selectStableReleaseTags(tags, count)/resolveBaselineReleaseRefs(count)generalize single-baseline selection; the single-baseline paths delegate so the two cannot disagree about "newest".config/scripts/cross-version-wire-lane-contract.test.mjs— new contract test deriving the lane's file list from the directory so a silently-uncovered suite fails CI.config/scripts/pr-code-change-scope.mjs+ test — new orchestration federation prefixes so host-side-only changes (the #19542 miss) still select the lane..github/workflows/pr.yml— adds the two suites to the cross-version lane.docs/reference/remote-wire-compatibility.md— documents the two-baseline rationale and the rules.
ℹ️ Federation module surface becomes a versioned contract for the harness
The MODULE_ALIASES table plus the pick/required calls in versioned-orchestration-wire.ts turn the orchestration federation module layout and export names into an implicit contract. Any future reorganization of those modules — moving a file, renaming one of the twelve picked exports, or folding a new method into the surface — must be updated in the same PR, or the cross-version lane goes red on unrelated PRs the moment that change ships as a release baseline. This is the intended "fail loudly instead of silently covering less" trade-off and it is documented, but it is a recurring maintenance cadence to be deliberate about when reorganizing orchestration RPC files.
Technical details
# Federation module surface is now a versioned contract for the harness
## Affected sites
- tests/e2e/cross-version-wire/versioned-orchestration-wire.ts — MODULE_ALIASES (dispatcher, methodRegistry,
protocol, db, orchestrationError, federationSync, controlMessage, federatedWorkerStart, workerObservation,
legacyFederatedRead, attachStartSchema, fleetSnapshot) + pick/required over RpcDispatcher, OrchestrationDb,
OrchestrationError, FederationAttachStartParams, ALL_RPC_METHODS, startFederatedWorker,
callFederatedWorkerShow, readLegacyFederatedTerminal, syncFederatedDispatch, encodeFederatedControlMessage,
readFederatedFleetSnapshots.
## Required outcome
- None for this PR; informational. The hazard is a future PR that renames/moves one of these modules or
exports and lands without updating the table, reddening the lane for everyone else when that release is
next picked as a baseline.
## Suggested approach (optional)
- None. The error messages ("add its path to MODULE_ALIASES rather than skipping the pairing") already direct
the fix; no change requested here.DeepSeek Pro (free via Pullfrog for OSS) ｜ 𝕏
📝 WalkthroughWalkthroughThe change adds release-based orchestration wire testing across two stable baselines and both skew directions. It adds loaders for working-tree and released builds, federation peer fixtures, orchestration journeys, and compatibility assertions for methods, schemas, fields, capabilities, and relay behavior. It updates release-tag selection to support multiple baselines. CI now runs the new suites, lane contracts verify workflow coverage, source-path classification includes federation surfaces, and documentation describes the coverage. Priority: ➖ Normal Merge Risk: 🟡 Moderate · up to The new compatibility lane can either fail before exercising its intended baselines or miss important federation and nested-suite regressions. These coverage gaps should be fixed before relying on the lane for release-skew safety. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 37.50% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 24 functions across 9 files. (2 skipped: 2 unsupported.)
✨ 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: 4
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: ce0a6ded-6bdb-41fb-930a-553db4012299
📒 Files selected for processing (11)
.github/workflows/pr.ymlconfig/scripts/cross-version-wire-lane-contract.test.mjsconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-code-change-scope.test.mjsdocs/reference/remote-wire-compatibility.mdtests/e2e/cross-version-wire/cross-version-orchestration-wire.unit.test.tstests/e2e/cross-version-wire/orchestration-federation-peers.tstests/e2e/cross-version-wire/orchestration-skew-journey.tstests/e2e/cross-version-wire/release-checkout.tstests/e2e/cross-version-wire/release-checkout.unit.test.tstests/e2e/cross-version-wire/versioned-orchestration-wire.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
| const onDisk = readdirSync(join(projectDir, crossVersionDir)) | ||
| .filter((name) => name.endsWith('.unit.test.ts')) | ||
| .map((name) => `${crossVersionDir}/${name}`) | ||
| .sort() |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Discover nested cross-version suites recursively.
config/vitest.config.ts supports nested tests/e2e/**/*.unit.test.ts files, but the workflow passes suite paths explicitly. A nested suite can therefore remain unlisted and untested while this nonrecursive scan still passes. Recurse while keeping the .unit.test.ts convention.
♻️ Proposed change
- import { join, resolve } from 'node:path'
+ import { join, resolve, sep } from 'node:path'
...
- const onDisk = readdirSync(join(projectDir, crossVersionDir))
+ const onDisk = readdirSync(join(projectDir, crossVersionDir), { recursive: true })
+ .map((name) => name.split(sep).join('/'))
.filter((name) => name.endsWith('.unit.test.ts'))
.map((name) => `${crossVersionDir}/${name}`)
.sort()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const onDisk = readdirSync(join(projectDir, crossVersionDir)) | |
| .filter((name) => name.endsWith('.unit.test.ts')) | |
| .map((name) => `${crossVersionDir}/${name}`) | |
| .sort() | |
| import { join, resolve, sep } from 'node:path' | |
| const onDisk = readdirSync(join(projectDir, crossVersionDir), { recursive: true }) | |
| .map((name) => name.split(sep).join('/')) | |
| .filter((name) => name.endsWith('.unit.test.ts')) | |
| .map((name) => `${crossVersionDir}/${name}`) | |
| .sort() |
| callOrchestrationWorkerServer: async ( | ||
| _environmentId: string, | ||
| method: string, | ||
| params: unknown, | ||
| _timeoutMs?: number, | ||
| meta?: { orchestrationRequestId?: string } | ||
| ) => { |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
#!/bin/bash
# Description: Resolve the callOrchestrationWorkerServer contract and its federation call sites.
set -euo pipefail
# Declaration(s)
rg -nP --type=ts -C6 'callOrchestrationWorkerServer\s*[:(]' src | head -200
# Call sites with argument lists
ast-grep run --pattern '$RUNTIME.callOrchestrationWorkerServer($$$)' --lang typescript srcRepository: stablyai/orca
Length of output: 40211
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- changed test stub ---'
sed -n '170,235p' tests/e2e/cross-version-wire/orchestration-federation-peers.ts
printf '%s\n' '--- runtime contract ---'
sed -n '1,80p' src/main/runtime/orca-runtime-get-runtime-id.ts
printf '%s\n' '--- relevant type definitions ---'
rg -n -P -C4 'type RuntimeOrchestrationEnvelope|interface RuntimeOrchestrationEnvelope|type OrchestrationEnvironmentCallOptions|interface OrchestrationEnvironmentCallOptions' src
printf '%s\n' '--- all test references to the stub ---'
rg -n -P -C5 'callOrchestrationWorkerServer|orchestrationRequestId|expectedEnvironmentPairingRevision' tests/e2e/cross-version-wire/orchestration-federation-peers.tsRepository: stablyai/orca
Length of output: 9671
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- federation start call and envelope handling ---'
sed -n '150,205p' src/main/runtime/rpc/methods/orchestration/federation/federated-worker-start.ts
printf '%s\n' '--- implementation of callOrchestrationWorkerServer ---'
rg -n -P -C12 'callOrchestrationWorkerServer\s*\(' src/main/runtime | head -160
printf '%s\n' '--- test assertions around recorded calls ---'
rg -n -P -C8 'attach-start|attachStart|requestId|pairing|calls|missing|recorded' tests/e2e/cross-version-wire/orchestration-federation-peers.tsRepository: stablyai/orca
Length of output: 24200
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- transport implementation and pairing fence use ---'
rg -n -P -C12 'callWorkerServer\s*\(|expectedEnvironmentPairingRevision|pairingRevision' src/main/runtime/orchestration/environment-transport.ts src/main/runtime/orchestration
printf '%s\n' '--- cross-version test entry points and assertions ---'
rg -n -P -C8 'createFederationHomePeer|createFederationHostPeer|calls|federationAttachStart|pairingRevision' tests/e2e/cross-version-wireRepository: stablyai/orca
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- federation transport call implementation ---'
rg -l -P 'callWorkerServer\s*\(' src/main/runtime/orchestration src/main/runtime | head -20Repository: stablyai/orca
Length of output: 301
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -P -C18 'callWorkerServer\s*\(' src/main/runtime/orchestration --glob '*.ts' | head -220Repository: stablyai/orca
Length of output: 151
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n -P -C22 'callWorkerServer\s*\(' src/main/runtime/runtime-orchestration-federation.tsRepository: stablyai/orca
Length of output: 1618
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '66,125p' src/main/runtime/runtime-orchestration-federation.tsRepository: stablyai/orca
Length of output: 2182
Preserve and validate the sixth internal argument in the stub.
callOrchestrationWorkerServer accepts the request envelope at position 5 and OrchestrationEnvironmentCallOptions at position 6. runtime-orchestration-federation.ts forwards expectedEnvironmentPairingRevision to both transport calls for mutation methods. The stub drops that argument and calls the host directly, so the cross-version harness bypasses the pairing-revision fence. Capture the sixth argument and apply the same revision check before dispatching the host call.
| const override = process.env[BASELINE_REF_ENV]?.trim() | ||
| if (override) { | ||
| return override | ||
| return [override] |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Keep the override contract compatible with two-baseline callers.
When ORCA_CROSS_VERSION_BASELINE_REF is set, this returns one ref even for resolveBaselineReleaseRefs(2). The new orchestration suite then requires two refs at cross-version-orchestration-wire.unit.test.ts Line 117 and fails before it runs a pairing. Its repository-backed unit test also assumes every override is a release tag, although this function accepts any ref.
Define a multi-ref override, or make the suite and its documentation handle an explicit one-ref pin consistently.
🧰 Tools
🪛 ast-grep (0.45.3)
[warning] Importing child_process exposes a command-execution surface; ensure any command/argument built from input is validated, and prefer execFile/spawn with an argument array over exec.
Context: import { execFileSync } from 'node:child_process'
Note: [CWE-78] Improper Neutralization of Special Elements used in an OS Command ('OS Command Injection').
(detect-child-process-typescript)
| const methods = methodRegistry.ALL_RPC_METHODS as unknown[] | ||
|
|
||
| return { | ||
| label, | ||
| revision, | ||
| capabilities: capabilities as readonly string[], | ||
| protocolVersion: protocol.RUNTIME_PROTOCOL_VERSION as number, | ||
| orchestrationContractVersion: protocol.ORCHESTRATION_CONTRACT_VERSION as number, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Validate the build exports before constructing the wire snapshot.
The working-tree and release loaders read separate module sources. A missing export in one build fails the comparison. If both builds omit ORCHESTRATION_CONTRACT_VERSION, the contract check compares undefined values and passes. Both protocol values must be validated before they enter the build snapshot and peer status.
A missing ALL_RPC_METHODS currently reaches registeredMethodNames, which calls .flatMap and throws an unlabelled TypeError. It does not make method coverage pass, but the guard gives the build-specific failure used by the other surface checks.
🛡️ Proposed guards
- const methods = methodRegistry.ALL_RPC_METHODS as unknown[]
+ const methods = methodRegistry.ALL_RPC_METHODS
+ if (!Array.isArray(methods)) {
+ throw new Error(`Build ${label} publishes no ALL_RPC_METHODS registry`)
+ }
+ const versionOf = (name: 'RUNTIME_PROTOCOL_VERSION' | 'ORCHESTRATION_CONTRACT_VERSION'): number => {
+ const value = protocol[name]
+ if (typeof value !== 'number') {
+ throw new Error(`Build ${label} publishes no numeric ${name}`)
+ }
+ return value
+ }- protocolVersion: protocol.RUNTIME_PROTOCOL_VERSION as number,
- orchestrationContractVersion: protocol.ORCHESTRATION_CONTRACT_VERSION as number,
+ protocolVersion: versionOf('RUNTIME_PROTOCOL_VERSION'),
+ orchestrationContractVersion: versionOf('ORCHESTRATION_CONTRACT_VERSION'),📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const methods = methodRegistry.ALL_RPC_METHODS as unknown[] | |
| return { | |
| label, | |
| revision, | |
| capabilities: capabilities as readonly string[], | |
| protocolVersion: protocol.RUNTIME_PROTOCOL_VERSION as number, | |
| orchestrationContractVersion: protocol.ORCHESTRATION_CONTRACT_VERSION as number, | |
| const methods = methodRegistry.ALL_RPC_METHODS | |
| if (!Array.isArray(methods)) { | |
| throw new Error(`Build ${label} publishes no ALL_RPC_METHODS registry`) | |
| } | |
| const versionOf = (name: 'RUNTIME_PROTOCOL_VERSION' | 'ORCHESTRATION_CONTRACT_VERSION'): number => { | |
| const value = protocol[name] | |
| if (typeof value !== 'number') { | |
| throw new Error(`Build ${label} publishes no numeric ${name}`) | |
| } | |
| return value | |
| } | |
| return { | |
| label, | |
| revision, | |
| capabilities: capabilities as readonly string[], | |
| protocolVersion: versionOf('RUNTIME_PROTOCOL_VERSION'), | |
| orchestrationContractVersion: versionOf('ORCHESTRATION_CONTRACT_VERSION'), |
This branch has not been deployed
1 participant
What the user notices
Before: A user updates their desktop while their worker host is still on the previous
release (or the reverse — the common case, because the two update on their own schedules).
They run
orca orchestration worker-start --on <server> .... The host refuses the attach withMissing Run ID, the worker never starts, and nothing in CI saw it coming: this is #19689,shipped 2026-09-08. Every federation "old peer" test in the tree writes a
protocol_versioninteger into a database row and reads it back with the same build, so a coordinator that
stops sending a field and a host that starts requiring one are both invisible.
git grep -i orchestration tests/e2e/cross-version-wire/*returned nothing.After: The same skew runs on every PR that touches federation source. A released
coordinator's real
startFederatedWorkercomposes the attach params, a released host's realdispatcher validates them, and the PR that makes
runIdrequired goes red naming the exactpairing and the exact error. The user's worker-start keeps working across the release boundary.
The delete question
Deleted: nothing was removed from
src/. Three things were removed from the shape of thechange rather than added beside it:
selectLatestStableReleaseTagis now a one-line delegation to the newselectStableReleaseTags(tags, count)instead of a second sort/filter/.at(-1)chain, andresolveBaselineReleaseRefa one-line delegation toresolveBaselineReleaseRefs(count).There is one tag-selection path, not two, so the single-baseline suites and the two-baseline
suite cannot disagree about what "newest" means.
readdirSync(tests/e2e/cross-version-wire)rather than ahand-maintained list of six paths. A future suite that nobody adds to the job fails the
contract test instead of quietly covering nothing — which is the class of miss this whole
workstream exists to close. The one file that was in the directory but not in the job
(
published-field-shape.unit.test.ts) is now in the job, so the derivation is exact.comparePublishedFieldsfrompublished-field-shape.ts. The only addition ispublishedFieldPaths, which flattens apayload to dotted names so a field removed from
setup/launch/attachment/observationcannot hide behind an intact top level.
What could not be deleted: the two-baseline rule cannot collapse to one. v1.4.199 (cut
2026-09-09) already contains #19689's fix, so a suite pairing only against the newest tag
cannot see the bug it was written for. That is the point, and it is stated in the doc.
Proof
Both scratch edits were local-only and reverted with
git checkout -- <that file>.Proof 1 —
runIdrequired again onfederationAttachStart(#19689's exact change)Scratch edit in
src/main/runtime/rpc/methods/orchestration/federation/federation-start-schema.ts:runId: OptionalString→runId: requiredString('Missing Run ID').Note the v1.4.199 pairing stayed green through all five failures: v1.4.199 already sends
runId. Pairing against the newest tag alone would have shipped this a second time.Proof 2 — a field removed from what the host publishes in the attach receipt
Scratch edit in
.../federation/federation.ts: dropsetup,from the ready-receipt return.Green on the branch, twice
The whole directory, to prove the
release-checkout.tschange did not break the existing foursuites:
Wire / SSH / folder workspace
capability. It pins three invariants: a released coordinator's attach params stay acceptable
to the current host's zod schema (Rule 2), every field a released peer's receipt parser reads
is still published (Rule 3, compared as dotted paths), and the
orchestration.*method-nameset only grows (Rule 3 for an
orcaCLI or coordinator calling an older verb name). It alsopins that
ORCHESTRATION_CONTRACT_VERSIONis equal across all three builds, because a bumpfences every unupdated peer out of every orchestration mutation at once.
wire, not the transport: an SSH-hosted worker host exchanges the same params and receipts, but
ssh-channel-multiplexerre-encodes them and that hop is not in this suite. Liveness stayslive/unverifiable/exitedthroughout — the fleet degradation asserted here producescapability_unsupportedon the host-error channel, never a fabricated verdict.selector (
id:...) and an existing agent terminal, so it never reachesassertOrchestrationWorktreeCreationSupported. Folder-vs-git placement is covered byfederation-folder-placement.test.ts(same-build) and is orthogonal to version skew.Testing
pnpm tcpnpm exec tsc --noEmit -p config/tsconfig.e2e.json, filtered to the new filescross-version-agent-session-wire.unit.test.ts:610andrelease-checkout.ts:9, both onmain)pnpm exec vitest run --config config/vitest.config.ts tests/e2e/cross-version-wire/release-checkout.unit.test.ts, run 1pnpm run check:code-quality:changedpnpm run check:react-doctor:changedpnpm exec oxlint <changed paths>max-lines; largest new file is 311 lines)git diff --checkgit statusProof commands and their red output are in Proof above.