Repository navigation
ci: run the cross-version tests when the chat send builders or the RPC request path change - #25061
Conversation
…hange The selector skipped the cross-version wire job for #24901, which changed the shared agent-send path, the structured send envelope builders, and orchestration RPC code. Route the send payload/fingerprint builders, src/main/runtime/orchestration/, and the orchestration RPC methods to the job, and pin each rule in a test.
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. Important Review skippedReview was skipped as selected files did not have any reviewable changes. ⚙️ Run configuration
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⚙️ Run configuration
📒 Files selected for processing (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review. 📝 WalkthroughWalkthroughThe cross-version wire path list now includes agent-session mutation, outbox, RPC dispatch, and orchestration paths. Classification tests check which paths enable the job. The wire harness loads mutation-plan and admission modules for working-tree and baseline-release builds. A new test checks plain and queue-if-active sends, successful replies, host delivery values, admission refusal with Priority: ➖ Normal Merge Risk: ⚪ Minimal · up to This change expands CI selection and cross-version send/admission coverage without changing application behavior; no actionable merge-blocking risk remains. Architecture SummaryArchitecture risk: 🔵 Low · up to The change affects 2 systems. Changed systems: Architecture concerns Review detailsSystems and components
Before / after behavior
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 2📝 Generate docstrings 💡
🛠️ Fix failing CI checks 💡
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 |
![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
- Broadened
CROSS_VERSION_WIRE_PREFIXES(config/scripts/pr-code-change-scope.mjs:168-203) — adds the structured send builders (structured-agent-session-mutation,…-send-mutation,…-outbox), replaces the three narrow orchestration entries with the wholesrc/main/runtime/orchestration/directory, and adds thesrc/main/runtime/rpc/methods/orchestrationmethod tree and itsorchestration*.tssiblings. - New routing test (
config/scripts/pr-code-change-scope-cross-version-send.test.mjs) — pins 13 paths that should trigger the cross-version-wire job and 4 that should not, viaclassifyPrJobs.
I verified the added prefixes are a strict superset of the three removed entries, so no prior coverage is lost; every file path named in the test exists; and the three selector suites (62 tests), oxlint, and check:code-quality:changed all pass. The new suite is auto-discovered through config/scripts/**/*.test.mjs in ci-unit-files.mjs.
One thing the PR body states plainly and I confirmed: no cross-version suite calls an orchestration RPC method or send-agent-turn.ts directly today, so these rules make the existing suites run on such PRs rather than add a new skew test for those paths. That is forward-looking coverage, not a defect.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
The agent-session suites now build their send through the shared outbox builder and check it against the release host's schema and fingerprint, so the send builder and outbox are selected exactly. Load-only orchestration rules are dropped; the dispatcher-path files every suite request runs through are selected instead.
…-version-selector
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Rewrote the cross-version selector rules.
CROSS_VERSION_WIRE_PREFIXESnow selects the send builders by exact file (structured-agent-session-mutation.ts,…-send-mutation.ts,…-outbox.ts) and the dispatcher-path files every dispatched request runs through (rpc/core.ts,rpc-streaming-dispatcher.ts,orchestration-contract-fence.ts,orchestration-session-caller.ts,orchestration-legacy-compatibility.ts,orchestration-mutation-executor.ts,shared/orchestration-rpc-contract.ts), dropping the prior whole-orchestration/-dir andmethods/orchestrationrules. - Added
cross-version-agent-session-send.unit.test.ts. It builds a current client'sagentSession.sendthrough the shared outbox, dispatches it to both the current and newest-release dispatchers, and asserts it reaches each host with a declared fingerprint equal to that build's own re-derivation. - Switched the manifest's
sendParamsto the outbox builder (createStructuredAgentSessionOutboxEntry+structuredAgentSessionSendRequest), so the existing agent-session tests now exercise the real client path. - Added
hostSendFingerprintper build inversioned-agent-session-wire.ts, reconstructing admission's digest from each build's ownsendPlanandcomputeAgentSessionPayloadFingerprint.
I verified the hostSendFingerprint reconstruction mirrors admitAndRunAgentSessionMutation (structured-agent-session-mutation-admission.ts:86) exactly; that the new sendParams output is shape- and value-identical to the old hand-built envelope for whitespace-free text (so the restart-cursor tests are unaffected); that all 12 newly selected paths exist; and that the selector suites (63 tests) and oxlint on the five changed files pass.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
![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-agent-session-send.unit.test.ts (1)
56-56: 📐 Maintainability & Code Quality | 🔵 Trivial | 🏗️ Heavy liftExercise release-host admission, or narrow the stated contract.
The test states that release admission must re-derive the send fingerprint. Instead,
structuredHostStub.sendreturns success directly, andhostSendFingerprintseparately recomputes the fingerprint. The test can pass even when real release admission would reject the send. Route the request through the release build’s actual mutation host to cover admission. If the intended scope is only dispatcher-to-host wire output, remove the admission guarantee from the test’s contract.
ℹ️ Review info
⚙️ Run configuration
- Configuration used: Repository UI
- Review profile: CHILL
- Plan: Advanced
- Run ID:
da5e2c34-b0c2-4484-adc3-c7ab7d31e581
📒 Files selected for processing (5)
config/scripts/pr-code-change-scope-cross-version-send-path.test.mjsconfig/scripts/pr-code-change-scope.mjstests/e2e/cross-version-wire/cross-version-agent-session-send.unit.test.tstests/e2e/cross-version-wire/structured-agent-session-surface-manifest.tstests/e2e/cross-version-wire/versioned-agent-session-wire.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 7 remain after this review.
…RPC reply builder
brennanb2025
changed the title
…agreed one is not vacuous
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Exercised the release's own send admission —
versioned-agent-session-wire.tsreplaces the fingerprfint re-derivation (hostSendFingerprint) withadmitSend, which loads each build's ownadmitAndRunAgentSessionMutationand runs it with no journal. A fingerprint the build derives differently now surfaces asfingerprintMismatch; an agreed one stops atsessionNotAttached, which the test asserts. This is stronger than comparing digests: it is the release's real admission deciding. - Covered a queued send — the send test is parameterised over a plain message and
delivery: 'queue-if-active', which joins the operation fingerprint, so a builder that dropsdeliveryis caught. - Added
src/main/runtime/rpc/errors.tsto the selector (pr-code-change-scope.mjs:184) and pinned it in the routing test. It is imported eagerly bydispatcher.ts:12andrpc-streaming-dispatcher.ts:9, so it is genuinely on every dispatched request's path.
I confirmed the new white-box assumption against the pinned baseline: the v1.4.219 checkout (commit 482a2438d5) exports admitAndRunAgentSessionMutation from the same module path with the same ordering — fingerprint check, then the absent-prepareSession skip, then journal() → sessionNotAttached — so the test is meaningful against the real release, not just current source. The now-unused computeAgentSessionPayloadFingerprint import was dropped cleanly.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Review statusProblem. CI skipped the cross-version wire compatibility tests for #24901, even though that PR changed the shared builders that produce a structured chat send and its fingerprint. Those tests run current code against the latest release, so an older and a newer Orca can't silently disagree. The job selector ( What changes for you. No app behavior changes. For contributors: a PR that changes the chat send builders, or the code that handles every RPC request and builds every reply, now runs the cross-version job. That job now catches a send an older host would refuse. Over the last 400 commits on main, the job would have run on 78 instead of 74. What review changed (relative to the first version of this PR):
Verified:
Not verified / known:
Deferred follow-ups:
This PR has not been merged. |
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
- Added a negative control to the cross-version send test (
cross-version-agent-session-send.unit.test.ts:74-78) — after the positive assertion that an agreed fingerprint stops atsessionNotAttached, it overwritessent.envelope.payloadFingerprintand asserts the sameadmitSendnow answersfingerprintMismatch. This proves the fingerprint check ran, so the positive case can no longer pass vacuously. I traced it throughadmitAndRunAgentSessionMutation: the plan fields are re-digested and compared against the overwrittenpayloadFingerprintbefore the journal is read, so theagent_session_operation_conflict/fingerprintMismatchrefusal is the only possible answer.
The change is test-only and matches the existing style; no runtime surface is touched.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
CI on the final head d77851b has finished. Every job passes except two: |
…-version-selector
|
Merged main (with the typecheck fix) into the branch; head is now cf6bd44 and CI is fully green. That includes |
1 participant
Problem
CI skipped the cross-version wire compatibility tests for #24901, a PR that changed what a client sends to a host. Those tests (
tests/e2e/cross-version-wire/) run current code against the newest release, to catch an older client or host breaking on what a newer one sends.#24901 changed the shared builder that produces the structured chat send request and its fingerprint (
src/shared/structured-agent-session-send-mutation.ts, called throughstructured-agent-session-outbox.ts), plus orchestration code. The desktop app uses that builder to send chat messages to remote Orca servers, which can run a different version. So a mistake there could get a user's message refused by an older server. The job was marked skipped, and the PR's compatibility evidence rested on running the suites by hand.The cause: the PR job selector (
config/scripts/pr-code-change-scope.mjs) runs that job only for files listed inCROSS_VERSION_WIRE_PREFIXES, and the send builders were not listed. There was also a deeper gap: even when the job ran, no suite built a send with the shared builder. The agent-session suite computed its own fingerprint, so a builder change an older host rejects would still have passed.What this changes for you
src/main/runtime/rpc/)send-agent-turn.tsCI cost: over the last 400 commits on main the job would have run on 78 instead of 74.
The change
New
tests/e2e/cross-version-wire/cross-version-agent-session-send.unit.test.ts. The suite's send params now come from the same outbox functions the desktop app sends from (createStructuredAgentSessionOutboxEntryandstructuredAgentSessionSendRequest), not a hand-built envelope. The test sends that request to both the current build and the newest release (v1.4.219 today) through each build's real dispatcher. It runs twice: once for a plain message, and once for a message queued behind a running turn (delivery: 'queue-if-active', which joins the fingerprint). Per build it checks two things. First, the build's own request schema accepts the send and the call reaches the host. Second, the build's own admission code (sendPlanandadmitAndRunAgentSessionMutation, loaded from that build) accepts the fingerprint the client declared. A mismatch there is what an older host refuses as an operation conflict.The host in this test is the suite's existing stub, so the release's admission runs on the params its dispatcher handed that stub, with no journal open. Admission checks the fingerprint first and then stops at the missing journal, so the test expects "session not attached" and fails on "fingerprint mismatch".
versioned-agent-session-wire.tsgainsadmitSendper build for this.Because
sendParamsin the shared manifest now uses the builder, the existing agent-session tests that send through a real current host (the restart cursor tests) also go through the builder.Selector (
CROSS_VERSION_WIRE_PREFIXES), choosing only code a suite executes:src/shared/structured-agent-session-mutation.ts,structured-agent-session-send-mutation.ts,structured-agent-session-outbox.ts: exact files. The rest of the outbox family (-outbox-admission,-outbox-delivery,-outbox-stop-withdrawal) is client-side queueing that no suite runs, so it stays off.src/main/runtime/rpc/core.ts,errors.ts,rpc-streaming-dispatcher.ts,orchestration-contract-fence.ts,orchestration-session-caller.ts,orchestration-legacy-compatibility.ts,orchestration-mutation-executor.ts, andsrc/shared/orchestration-rpc-contract.ts: every request the suites dispatch runs through these, whatever its method (the registry, the reply and error builders the suites compare across versions, the migration fence, the caller-resolution check, the old-client compatibility hook, and the mutation wrapper). Otherorchestration-legacy-*files do work only fororchestration.*methods and stay off.db.ts,db/,orchestration-schema-version-skew).config/scripts/pr-code-change-scope-cross-version-send-path.test.mjspins each rule, and pins that the outbox siblings,send-agent-turn.ts, the orchestration RPC methods, and renderer-only structured chat modules stay off.Why this approach
Selecting all of
src/main/runtime/orchestration/andsrc/main/runtime/rpc/methods/orchestration*would make the job run without testing anything there. The suites only load those modules because the dispatcher registers every RPC method; none of them calls an orchestration method orsend-agent-turn.ts, so a break would only fail if a module stopped loading.Computing the selection from the suites' import graph has the same problem at a larger scale. That graph reaches 2353 source files through the method registry. Selecting on it would run the job on about 142 of the last 400 commits, mostly git, GitHub and SSH code the suites never run. It would also need a source checkout and a bundler in the small job that decides which jobs run.
Making the send builder covered for real costs one small test, and it tests the producer #24901 changed: a request a current client really sends to a host of another version.
What is still not covered
orchestration.*RPC method. A PR that changes only those methods still skips the job. Adding a skew test for them is separate work.send-agent-turn.tsbuilds a send inside the host process and hands it to that same host, so its builder and host are always the same build. Version skew does not reach that path, and it stays off.agentSession.sendrequest and fingerprint (mobile/src/session/mobile-structured-agent-session-send.ts). No suite runs it, and a change there does not select the job.Testing
tests/e2e/cross-version-wire/directory passes locally: 16 files, 96 tests.deliveryfrom the fingerprint: only the queued-message case fails, on v1.4.219 only, with "fingerprint mismatch".oxlint(full), the anti-slop lint on the changed files,oxfmt --check, andpnpm run check:code-quality:changedpass. The e2e typecheck project (config/tsconfig.e2e.json, not yet enforced in CI) reports no errors in the changed files.