Repository navigation
Run every cross-version wire suite, and start the job for every module they pair - #22041
brennanb2025 wants to merge 10 commits into
Conversation
…elay The gate that decides which PRs run the cross-version wire job listed 23 prefixes and none of them reached the agent-status store, the worktree/mobile row contract, or the relay transport. A change to any of those could break a released peer with nothing in CI to say so. Add those prefixes, and give them something that can fail: two new pairings against the newest release, one over the agent-status snapshot, mutation envelope and status payload, one over the relay handshake and JSON-RPC framing. Reaching a frozen relay daemon needs src/relay in the extracted tree, so the harness now archives it. Also fix the lane itself. The unit shards exclude this directory, so the job's vitest argv was the only thing deciding whether a suite ran, and it was a curated list of seven paths while the directory held nine. The two unlisted suites executed nowhere; one could not have passed if it had, because it extracts a checkout no sibling shares and had no timeout to cover it. The step now names the directory, and a new guard keeps the two in step.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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:
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 (4)
🚧 Files skipped from review as they are similar to previous changes (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 1 remain after this review. 📝 WalkthroughWalkthroughThe PR workflow now runs all tests in the cross-version wire directory. Job classification covers additional runtime, persistence, orchestration, and mobile worktree paths. A registration test checks suite selection, paired-module coverage, and unit-shard exclusion. Two downgrade tests and the checkout process use longer timeout limits. The compatibility documentation describes harness coverage and job selection. Priority: ⬇️ Low Merge Risk: 🟡 Moderate · up to The lane still does not test the session-tab status payload against an older reader, and a relay test failure may be misleading. Resolve or explicitly accept these gaps before merging. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change broadens compatibility testing without an apparent change to production permissions or shipped wire behavior. The newly reached tests run with read-only repository access, but runner-level controls and test outcomes were not established. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 45.45% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 22 functions across 11 files. (1 skipped: 1 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.
🧹 Nitpick comments (1)
tests/e2e/cross-version-wire/cross-version-relay-transport-wire.unit.test.ts (1)
156-162: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the verdict frame decoded before parsing it.
This test parses
frame?.payload ?? Buffer.alloc(0). IfdecodeAllreturns no frame,parseHandshakeMessagethrows on the empty buffer while evaluating theexpectargument. The custom message at Line 159 never runs, so the reported failure is a JSON parse error rather than a missing handshake verdict frame. The two neighbouring tests assertframe?.typefirst; apply the same guard here.♻️ Proposed guard
const [frame] = decodeAll(bridge, reply) + expect(frame?.type, `${bridge.label} did not read a handshake verdict frame`).toBe( + bridge.host.MessageType.Handshake + ) expect( bridge.host.parseHandshakeMessage(frame?.payload ?? Buffer.alloc(0)), `${bridge.label} could not read ${daemon.label}'s handshake verdict` ).toMatchObject({ type: agreed ? 'orca-relay-handshake-ok' : 'orca-relay-handshake-mismatch' })
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 0305f6a6-01a2-4ec5-8e02-fc5e52ece810
📒 Files selected for processing (11)
.github/workflows/pr.ymlconfig/scripts/cross-version-wire-lane-registration.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-agent-status-wire.unit.test.tstests/e2e/cross-version-wire/cross-version-relay-transport-wire.unit.test.tstests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.tstests/e2e/cross-version-wire/release-checkout-tree.tstests/e2e/cross-version-wire/release-checkout.tstests/e2e/cross-version-wire/versioned-agent-status-wire.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 4 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.
ℹ️ Minor suggestions only — two of the seven newly gated prefixes cannot fail any suite in the lane.
Reviewed changes
- Wire gate widened —
CROSS_VERSION_WIRE_PREFIXESgains seven agent-status, worktree-row and relay prefixes, so thecross-version-wirejob now starts for those diffs. - Lane runs the whole directory — the
cross-version-wirevitest argv drops the curated seven-file list fortests/e2e/cross-version-wire/, soorchestration-delivery-downgradeandpublished-field-shapeexecute again; the latter gets the missing180sbudget. - Lane registration guard (new) — asserts every
.unit.test.tsin the directory is selected, the argv names only on-disk suites, the shard exclusion stays, and the classifier gates the job for each suite, with a population floor. - Agent-status pairing (new) — skews the store snapshot, mutation envelope and OSC status payload in both directions and checks the arm set is a superset of the release's.
- Relay pairing (new) — skews the handshake frame and a desktop→daemon JSON-RPC frame, and pins the two
RELAY_VERSIONcopies together within a build. - Checkout support —
ARCHIVE_PATHSgainssrc/relay,CHECKOUT_FORMAT3→4, extraction timeout 45s→120s so two concurrent fills fit. - Docs + classifier tests —
remote-wire-compatibility.mddocuments the new surfaces;pr-code-change-scope.test.mjsasserts each new prefix exists on disk and classifies true.
The new agent-status suite is real coverage: the snapshot/mutation decoders use strict hasOnlyKeys, so an added nested field fails hydration, and the top-level key-parity assertion fails on an added/removed snapshot key. I re-ran the two config/scripts suites here (44 passed) and confirmed every path named in pr-code-change-scope.test.mjs exists.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Mobile Checks dispatches on its own paths filter, and it does not list the shared types mobile imports verbatim: runtime-types (26 mobile importers), agent-status-types (15) and runtime-worktree-contracts (2). Of 103 host-side modules named *mobile* outside mobile/, 85 are outside that filter, including the projection that builds the tab rows a phone reads. Two of those shared types are gated for cross-version skew by this change. That is a different failure from mobile's typecheck breaking, so the page now says so rather than letting a reader infer the phone is covered. The fix belongs in mobile.yml.
Resolve the lane argv to the directory (main had grown its curated list to ten files, still missing three suites), union ARCHIVE_PATHS with main's phone row reader and bump CHECKOUT_FORMAT to 5 since main already took 4, keep main's orchestration downgrade test and re-apply only its timeout, give the death evidence downgrade suite the same budget, drop the duplicate runtime-worktree-contracts prefix main already added, and point the shard exclusion guard at ci-unit-files.mjs where main moved it.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| expect( | ||
| store.applyMutation({ parent: { subject, firstObservedAt: 10 } }), | ||
| build.label | ||
| ).not.toBeNull() | ||
| const childWorkId = 'cross-version-child-1' | ||
| expect( | ||
| build.announceChild(store, childWorkId, { | ||
| parent: subject, | ||
| provider: 'claude', | ||
| aliases: [{ segmentId: 'segment-1', aliasKind: 'task_id', alias: 'task-1' }], | ||
| fence: { invocationId: 'invocation-1', generation: 1 }, | ||
| lifetime: 'current', | ||
| kind: 'agent', | ||
| state: 'working', | ||
| membership: 'live', | ||
| observedAt: 20, | ||
| stoppable: true, | ||
| provenance: { source: 'transport', producerId: 'cross-version-fixture' } | ||
| }), | ||
| build.label | ||
| ).toMatchObject({ accepted: true, childWorkId }) | ||
| const snapshot = store.getSnapshot() | ||
| const envelope = store.applyMutation({ | ||
| facts: [{ subject, key: 'cross-version-fact', value: 'published' }] |
There was a problem hiding this comment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| const parsed = daemon.host.parseHandshakeMessage(frame?.payload ?? Buffer.alloc(0)) | ||
| expect(parsed).toMatchObject({ | ||
| type: 'orca-relay-handshake', | ||
| version: bridge.host.RELAY_VERSION | ||
| }) | ||
| }) | ||
|
|
||
| it('answers the peer handshake with a verdict the peer can read', () => { | ||
| const { bridge, daemon } = pair() | ||
| const agreed = daemon.host.RELAY_VERSION === bridge.host.RELAY_VERSION | ||
| const reply = daemon.host.encodeHandshakeFrame( | ||
| agreed | ||
| ? { type: 'orca-relay-handshake-ok', version: daemon.host.RELAY_VERSION } | ||
| : { | ||
| type: 'orca-relay-handshake-mismatch', | ||
| expected: daemon.host.RELAY_VERSION, | ||
| got: bridge.host.RELAY_VERSION | ||
| } | ||
| ) | ||
| const [frame] = decodeAll(bridge, reply) |
There was a problem hiding this comment.
relay-handshake.ts start this job, but the pairing chooses and encodes the daemon’s reply itself. It never exercises the daemon’s version and credential checks or the bridge’s refusal handling. Those paths could break a reconnect while the job stays green; pair the actual handlers before treating that file as covered.
Knowledge Base Used: Relay protocol and transport
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| .announce(request), | ||
| normalizeStatusPayload: (payload) => modules.types.normalizeAgentStatusPayload(payload) |
There was a problem hiding this comment.
parseAgentStatusPayload entry point, so a change confined to that parser can break CLI status ingestion while this suite stays green. Send JSON text from one build through the other build’s OSC parser.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
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: f729e081-98a7-4ea2-9b5c-4095a65bab97
📒 Files selected for processing (9)
.github/workflows/pr.ymlconfig/scripts/cross-version-wire-lane-registration.test.mjsconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-code-change-scope.test.mjsdocs/reference/remote-wire-compatibility.mdtests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.tstests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.tstests/e2e/cross-version-wire/release-checkout-tree.tstests/e2e/cross-version-wire/release-checkout.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 3 remain after this review.
There was a problem hiding this comment.
ℹ️ No new issues in the merge delta — one informational note below.
Reviewed changes
Re-reviewed the commits that landed since the prior pullfrog review (adccab001a): the origin/main merge 8381e66.
- Re-based the lane guard onto the sharded unit workflow —
cross-version-wire-lane-registration.test.mjsnow finds the shard step by--shard=, assertsORCA_BALANCE_UNIT_SHARDSis1, and checksUNIT_EXCLUDEcarriestests/e2e/cross-version-wire/**, becauseunit-tests.yml/vitest.config.tsnow apply the exclusion through that shared constant behind the env flag instead of inline in the step'srun. - Dropped the duplicate worktree-contracts prefix — the merge removed the
src/shared/runtime-worktree-contractsentry this PR had added besidesrc/shared/agent-status;origin/mainalready carries that prefix, so the gate's coverage is unchanged and the prior thread on it is retired. - Reconciled the checkout config —
ARCHIVE_PATHSkeepssrc/relayand the mobile row reader, andCHECKOUT_FORMATlands at5(base4plus this PR's bump).
Verified in-run: config/scripts/cross-version-wire-lane-registration.test.mjs + pr-code-change-scope.test.mjs → 46 passed; the lane argv is the whole directory, matching the shard exclusion.
ℹ️ PR description still counts the pre-existing prefix as newly added
The body's "Added to CROSS_VERSION_WIRE_PREFIXES" list names seven paths, including src/shared/runtime-worktree-contracts, but that prefix ships on origin/main and the merge dropped this PR's duplicate — the PR adds six. A reviewer comparing the description to the diff is left to reconcile the extra entry.
Technical details
# PR body overstates the newly gated prefix set
## Affected sites
- PR description "The gate runs for these paths" list — names `src/shared/runtime-worktree-contracts` as added by this PR.
- `config/scripts/pr-code-change-scope.mjs:178` — `src/shared/runtime-worktree-contracts` exists on `origin/main`; the merge dropped this PR's duplicate entry next to `src/shared/agent-status`.
- HEAD adds six prefixes: `src/shared/agent-status`, `src/main/ssh/relay-protocol`, `src/shared/relay-frame-decoder`, `src/relay/protocol`, `src/relay/relay-frame-decoder`, `src/relay/relay-handshake`.
## Required outcome
- The description's prefix list matches the diff, so a reader is not left reconciling the extra entry.
## Open questions for the human
- None.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…sses versions The agent-status store's snapshot and mutation envelope never leave the process that owns the store, and the SSH relay is always the desktop's own build (content- hashed install directory, per-version socket, exact-version handshake). Pairing them gated changes no older peer can observe. What does cross versions is the hook server's status file: after an update or a downgrade the other build re-admits each persisted row with its own normalizeAgentStatusPayload. Pair that in both directions over every baseline arm, and check current still writes every arm and field the baseline writes. Narrow the lane prefixes to the normalizer's modules, drop the relay prefixes and src/relay from the extracted tree (CHECKOUT_FORMAT back to main's 4), fold the classification cases into main's existing list so the scope test fits max-lines, and correct the wire-compatibility doc.
… suites minimal A named timeout after the test body makes the formatter re-indent the whole test; a numeric literal keeps the test-call layout, so each suite diff is two lines.
There was a problem hiding this comment.
✅ No new issues found — the narrower gate and the new payload pairing are sound, and the prior relay thread is addressed.
Reviewed changes
Re-reviewed the commit that landed since the prior pullfrog review (8381e66): 26321e3, which narrows the PR to the agent-status payload surface.
- Gate narrowed to three payload prefixes —
CROSS_VERSION_WIRE_PREFIXESnow addssrc/shared/agent-status-types,src/shared/agent-status-field-normalizationandsrc/shared/agent-turn-outcome; the store, relay and worktree-row prefixes from earlier commits are gone. The three are the inputsnormalizeAgentStatusPayloaduses, so each can change what the hook server re-admits from a persisted status row after an update or downgrade. - Single payload pairing replaces three files —
cross-version-agent-status-payload.unit.test.tssupersedes the deletedcross-version-agent-status-wire.unit.test.ts,cross-version-relay-transport-wire.unit.test.tsandversioned-agent-status-wire.ts, pairingnormalizeAgentStatusPayloadagainst the newest release in both directions. - Checkout config reverted —
CHECKOUT_FORMATis back to4andsrc/relayis dropped fromARCHIVE_PATHS, leaving the 45s→120s extraction timeout as the only net change torelease-checkout-tree.ts. - Lane guard trimmed — the now-redundant "argv names no path that is not a suite" assertion is removed; the directory-argv selection and shard-exclusion checks remain.
- Docs rewritten — the canonical store and the relay are documented as deliberately ungated with their reasons; the earlier mobile-gap section is gone.
I re-ran pr-code-change-scope.test.mjs and cross-version-wire-lane-registration.test.mjs (44 passed), traced each gated prefix to an assertion that can actually fail on its change, and checked the docs' "the relay is always the desktop's own build" claim against src/main/ssh/ssh-relay-superseded-endpoints.ts (per-version relay directory, exact content-hash handshake) — it holds. The store snapshot/envelope serializers have no production caller, so pairing them "when it ships" is the right call.
ℹ️ PR description and title still describe the pre-26321e3 scope
The title ("agent status, worktree rows and the relay") and the body still promise the relay gate, the src/shared/agent-status store gate, the two deleted suites, src/relay in ARCHIVE_PATHS, the CHECKOUT_FORMAT bump and the mobile-gap analysis. None of those are in the diff at HEAD, so a reviewer reading the description is left to reconcile roughly twice the change that shipped. The code is mergeable as-is; this is a description refresh only.
Technical details
# PR body/title no longer match the diff
## Affected sites
- PR title — names "worktree rows and the relay", neither gated by this PR at HEAD.
- Body "The gate runs for these paths" — lists `src/shared/agent-status`, `src/main/ssh/relay-protocol`, the three `src/relay/*` prefixes; HEAD adds only `src/shared/agent-status-types`, `src/shared/agent-status-field-normalization`, `src/shared/agent-turn-outcome`.
- Body "What is now covered" — describes `cross-version-agent-status-wire.unit.test.ts` and `cross-version-relay-transport-wire.unit.test.ts`, both deleted; the surviving suite is `cross-version-agent-status-payload.unit.test.ts`.
- Body checkout paragraph — says `ARCHIVE_PATHS` gains `src/relay` and `CHECKOUT_FORMAT` goes 3→4; HEAD has neither (format is `4`, same as `main`).
- Body "A neighbouring gap I did not fold in" — the mobile-gap section was removed from `docs/reference/remote-wire-compatibility.md` in this commit.
## Required outcome
- Title and body describe the three-prefix / one-suite payload change that HEAD actually ships, so the description is not read as cover for the dropped relay and store surfaces.
## Open questions for the human
- Was dropping the relay/store gates deliberate (the commit and docs read as intentional) — if so, the body's "Blast radius" figures also need recomputing for the narrower prefix set.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| it('re-admits every row the writer persisted and reads each shared field as its own', () => { | ||
| const { writer, reader } = pair() | ||
| // Every arm the release can write; an arm current drops fails the old-writer direction. | ||
| for (const state of baseline.states) { |
There was a problem hiding this comment.
Knowledge Base Used: Agent integrations and hooks
…us's only version boundary Session-tab sync also carries each terminal tab's agent status to paired clients of any version; say so where the suite and the reference name what is and is not paired.
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: 237c9f1f-5037-4bc7-bf86-723573a4dfeb
📒 Files selected for processing (8)
config/scripts/cross-version-wire-lane-registration.test.mjsconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-code-change-scope.test.mjsdocs/reference/remote-wire-compatibility.mdtests/e2e/cross-version-wire/agent-session-death-evidence-downgrade.unit.test.tstests/e2e/cross-version-wire/cross-version-agent-status-payload.unit.test.tstests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.tstests/e2e/cross-version-wire/release-checkout-tree.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/reference/remote-wire-compatibility.md
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 0 remain after this review.
| it('re-admits every row the writer persisted and reads each shared field as its own', () => { | ||
| const { writer, reader } = pair() | ||
| // Every arm the release can write; an arm current drops fails the old-writer direction. | ||
| for (const state of baseline.states) { |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Test every state that the selected writer can persist.
Both pairings iterate baseline.states. If the current build adds a state, the new-writer pairing never sends that state to the baseline reader. Line 137 still passes because it permits additional current states. The baseline normalizer would reject the new state, and persisted-row hydration would drop the row. Iterate writer.states in each pairing so the lane detects this downgrade incompatibility. (raw.githubusercontent.com)
The registration test now scans each suite (and the wire helpers suites load through) for the working-tree modules and release-checkout paths it pairs, and fails when one does not start the cross-version-wire job. Nine paired modules did not; they are added as exact paths. The phone's worktree-ps row reader is a mobile-only diff, so the job is lifted outside should_run like mobile_web_app. Drops the agent-status payload pairing: the hook server's status file is a minor path, the suite skipped the new-arm-on-downgrade case it claimed, and it could not see the file's own version gate. Session-tab sync's agentStatus is now listed as unpaired instead.
This comment has been minimized.
This comment has been minimized.
…e-agent-status-coverage
There was a problem hiding this comment.
ℹ️ No code issues in the delta — one description refresh below.
Reviewed changes
Re-reviewed the commit that landed since the last pullfrog review (f4f6f76): e8fdd62, which starts the lane for every module its suites pair.
- Paired-module scanner added (new) —
cross-version-wire-lane-registration.test.mjswalks every.tsfile in the lane directory for static/dynamic working-tree imports and quoted checkout paths, and fails when a paired module is not inCROSS_VERSION_WIRE_PREFIXES, with a 25-module floor and an existence check so a broken scan cannot pass empty. - Nine paired modules gated —
rpc/methods/index.ts,runtime-subscription-registry.ts,terminal-retirement-proof-ledger.ts,mobile-session-terminal-retirement(-proof).ts,worktree-identity-migration.ts,orchestration/db.ts,orchestration/db/contract-constants.tsandmobile/src/worktree/agent-row-display.ts. Each is genuinely loaded by a suite, so the prefix can fail for a change. - Lane lifted outside
should_run— the phone'sworktree psrow reader is a mobile-only diff, so the job is set likemobile_web_appinstead of staying gated on desktop-relevant changes. - Agent-status payload pairing dropped — the suite, its three prefixes and
versioned-agent-status-wire.tsare removed; the docs now list session-tab-syncagentStatusas unpaired. - Docs tightened —
remote-wire-compatibility.mdstates every paired module must be on the prefix list and namescross-version-worktree-ps-verdict.unit.test.tsas the one paired agent-status path.
Verified in-run: cross-version-wire-lane-registration.test.mjs + pr-code-change-scope.test.mjs → 47 passed; the scanner resolves 30 modules, all on disk and classifying cross-version-wire=true.
ℹ️ Title and body still describe the gates this PR dropped
The title promises "agent status, worktree rows and the relay" and the body's "What is now covered" section lists src/shared/agent-status, the src/relay/* prefixes, two deleted suites and a CHECKOUT_FORMAT bump — none of which are in the diff at HEAD. With e8fdd62 also dropping the agent-status payload pairing, the description now reads as roughly the opposite of what ships.
Technical details
# PR title/body no longer match HEAD
## Affected sites
- PR title — names "agent status" and "the relay"; neither is in `CROSS_VERSION_WIRE_PREFIXES` at HEAD.
- Body "The gate runs for these paths" — lists `src/shared/agent-status`, `src/main/ssh/relay-protocol`, `src/shared/relay-frame-decoder`, `src/relay/protocol`, `src/relay/relay-frame-decoder`, `src/relay/relay-handshake`; none are added by HEAD.
- Body "What is now covered" — describes `cross-version-agent-status-wire.unit.test.ts` and `cross-version-relay-transport-wire.unit.test.ts`, both deleted; `cross-version-agent-status-payload.unit.test.ts` is also deleted in `e8fdd62`.
- Body checkout paragraph — says `ARCHIVE_PATHS` gains `src/relay` and `CHECKOUT_FORMAT` goes 3→4; HEAD has neither.
## Required outcome
- The title and body describe the surface HEAD actually gates (the worktree-row reader plus the terminal / agent-session suites already there), so a reviewer is not left reconciling a different PR.DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…e-agent-status-coverage
The orchestration downgrade suite opens a current-schema database with old code, so a migration under db/schema/ is what it guards. The registration guard only sees modules a suite loads directly and db.ts reaches the schema transitively, so gate it by hand and say so where the guard is described.
There was a problem hiding this comment.
ℹ️ No critical issues — one minor gap in the newly hand-gated transitive set.
Reviewed changes
Re-reviewed the commits since the prior pullfrog review (e8fdd62): the two origin/main merges and 7a75ff2.
- Orchestration schema gated by hand —
CROSS_VERSION_WIRE_PREFIXESgainssrc/main/runtime/orchestration/db/schema/, so a change to the schema migrations behinddb.tsstarts the lane; the orchestration downgrade suite reaches them transitively. - Scanner limit documented — the
readPairedModulesJSDoc andremote-wire-compatibility.mdnow state that only a suite's direct loads are auto-gated and a module reached through them has to be added to the prefix list by hand. - Classifier case added —
pr-code-change-scope.test.mjsassertsdb/schema/migrate-v42.tsclassifiescross-version-wire=true.
Verified in-run: the transitive claim holds (db.ts → orchestration-db.ts → schema/migrate.ts → schema/migrate-v42.ts), and cross-version-wire-lane-registration.test.mjs + pr-code-change-scope.test.mjs pass (47).
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
| 'src/main/runtime/orchestration/db.ts', | ||
| 'src/main/runtime/orchestration/db/contract-constants.ts', | ||
| // Why: the orchestration downgrade suite opens a current-schema database with old code; db.ts reaches this transitively. | ||
| 'src/main/runtime/orchestration/db/schema/', |
There was a problem hiding this comment.
orchestration-schema-version-skew.ts sits one directory up from this new prefix and is imported by the now-gated schema/migrate.ts; it owns resolveOrchestrationMigrationStartVersion, the exact cross-version decision the orchestration downgrade suite exercises. A change to it does not start the lane — classifyPrJobs(['src/main/runtime/orchestration/orchestration-schema-version-skew.ts'])['cross-version-wire'] is false. Consider adding it alongside db/schema/.
Technical details
# The orchestration version-skew resolver is ungated
## Affected sites
- `config/scripts/pr-code-change-scope.mjs:177` — `src/main/runtime/orchestration/db/schema/` was just added; the sibling resolver one directory up is not on the list.
- `src/main/runtime/orchestration/orchestration-schema-version-skew.ts:184` — `resolveOrchestrationMigrationStartVersion`, the function that decides whether a stored DB migrates or keeps its stamp.
- `src/main/runtime/orchestration/db/schema/migrate.ts:1` — imports the resolver; `migrate.ts` lives inside the newly gated directory.
- `tests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.ts:5,25,43` — drives `OrchestrationDb` (which runs the resolver through `migrate`) against a pre-v41 checkout, so the lane can fail for a change here.
## Required outcome
- A change to `orchestration-schema-version-skew.ts` starts the `cross-version-wire` job, the same standard the `db/schema/` prefix was just added for.
- Add a representative case to the `pr-code-change-scope.test.mjs` classifier list so the on-disk path is exercised too.
## Open questions for the human
- The hand-gated transitive list also has no existence guard, and `db/orchestration-db.ts` / `db/attach-orchestration-db-methods.ts` are likewise ungated. Is `db/schema/` the intended coverage boundary, or the transitive closure around `db.ts`?| 'src/main/runtime/orchestration/db/schema/', | |
| 'src/main/runtime/orchestration/db/schema/', | |
| 'src/main/runtime/orchestration/orchestration-schema-version-skew', |
brennanb2025
changed the title
Review status: ready for your review (not merged)The problem this now fixes: the cross-version wire job could stay green while seeing nothing. Three suites in What is different for developers: every suite in the directory runs whenever the job does; a change to any module a suite pairs (including a mobile-only change to the phone's The PR was rescoped during review, and the title and body are rewritten to match. The original version added agent-status and SSH relay pairings. Tracing every path by which one build's data reaches a different build showed those pairings guarded nothing real: the agent-status store's snapshot and change messages never leave the process that owns them, and the SSH relay is always the desktop's own build (versioned install directory, handshake refuses any other build). They would have failed CI on changes no older peer can see, so they were removed. What replaced them fixes the underlying failure class: the trigger list is now checked against what the suites actually load, so it cannot silently drift again. Still not covered, on purpose: session-tab sync sends each terminal tab's agent status to paired desktop and mobile clients of any version, and nothing pairs it. That needs a designed suite and belongs in its own PR; the reference doc lists it as unpaired. The guard also only sees modules a suite loads directly, so transitive dependencies (the orchestration database writers behind Evidence
Related open work: #19861 changes how the relay negotiates its protocol version. If that lands and a new client can talk to an older relay, the relay becomes a real cross-version surface and should get a pairing then. Not verified: the combined result against the single newest |
This branch has not been deployed
1 participant
The problem
The
cross-version wire compatibilityPR job is Orca's check that a change still works against the last released build: it extracts the newest release tag and runs old code against new code (a paired desktop and remote host, or a database written by one build and opened by another). Two gaps meant it could stay green while seeing nothing.Three suites in
tests/e2e/cross-version-wire/ran on no machine. The unit-test shards exclude that directory (a shallow clone has no release tags to extract), so the job's own file list is the only thing that runs it. That list was hand-maintained and had fallen behind:orchestration-delivery-downgrade,agent-session-death-evidence-downgradeandpublished-field-shapewere in the directory but in no list, so they never executed and never reported.Changes to modules the suites test did not start the job. Which PRs run the job is decided by a separate list of path prefixes (
CROSS_VERSION_WIRE_PREFIXES). Nine modules that suites load and pair against a release were missing from it, for examplesrc/main/runtime/orchestration/db/contract-constants.ts(the orchestration schema version the downgrade suite checks) andmobile/src/worktree/agent-row-display.ts(the phone'sworktree psrow reader). A PR changing only those files skipped the one job that could catch a break in them.What changes for developers
src/main/runtime/orchestration/db/schema/.No app behaviour changes; this PR only touches CI configuration, tests and one reference doc.
How
.github/workflows/pr.yml: the job runs the directory, not a file list. It is the same boundary the shard exclusion uses, so a file cannot fall between them.config/scripts/cross-version-wire-lane-registration.test.mjs(new) holds that shut: every suite is selected by the job, the shards still exclude the directory, and every module a suite (or theversioned-*helper it loads through) directly loads for pairing exists and starts the job. A population floor stops a broken scan from passing by finding nothing.config/scripts/pr-code-change-scope.mjs: the ten missing prefixes, and the job's trigger is computed outside the general "should anything run" check, the same waymobile_web_appalready is, because a change to the phone's row reader is a mobile-only diff that would otherwise skip every job.Why this design
The earlier version of this PR added hand-written pairings for agent status and the SSH relay. Review traced every path by which one build's data reaches a different build and found those pairings guarded nothing real: the agent-status store's snapshot and change messages never leave the process that owns them, and the SSH relay is always the desktop's own build (each build installs it under its own versioned directory and the handshake refuses any other build). A gate that fails on changes no older peer can see blocks legitimate work, so they were removed. The recurring failure underneath was that the trigger list and the suites drifted apart silently. This PR fixes that class instead: the list is checked against what the suites actually load, so it cannot drift again.
What stays uncovered, stated rather than implied: session-tab sync publishes each terminal tab's agent status to paired desktop and mobile clients of any version, and nothing pairs it yet. Pairing it needs a designed suite and is left to a separate PR;
docs/reference/remote-wire-compatibility.mdlists it as unpaired. The guard sees modules a suite loads directly, not everything those modules import, so a transitive dependency (the orchestration schema directory is the known one) still has to be added by hand; the doc says so.Cost
main, 37 of 6,470 commits (0.57%) would additionally have started the job, about 5% more runs of it, most from edits to the orchestration database facade. In a sample of the 300 newest open PRs, none newly start it.Verified
main's prefix list, it names exactly the modules this PR adds. Each edit was temporary and reverted.orchestration/db/schema/starts the job with this PR and did not before.