Repository navigation
ci(mobile): fail CI when the RPC recording pin leaves main's history or the corpus does not reproduce - #21156
Merged
Merged
Conversation
`mobile/rpc-foundation/pilot-scenarios.json` carries the commit every golden claims it was recorded from, and `--record` refuses on any other tree. A behaviour-change branch pins its own last fenced commit, which stops being reachable the moment the branch squash-merges: nobody can record on main again until a hand-made repin lands, and until now only a human noticed. #21123 was that, and so was the repin after #20954. `scripts/rpc-recording-pin-guard.mts ancestry` fails when the pin is not an ancestor of the commit under test, and prints the repin recipe. It refuses to answer on a shallow clone rather than trusting grafted history, so the job checks out with `fetch-depth: 0`. Ordinary product drift past a reachable pin is not a failure. `reproduce` makes the other claim the corpus header makes, which the recording suites do not: they replay the goldens against the CURRENT tree, so a golden recorded somewhere other than the pin -- a merge that auto-merged golden JSON, a refresh copied back from a scratch directory -- passes them and is what the header exists to deny. It checks the pin out detached, lays this tree's recorder and manifest over it, and lets the same suites compare in place, so the comparison is `compareGolden` with lockfile and platform masked as ever. It runs unconditionally on a push to main, which has no `verify` job and is where a squash lands a spliced corpus. On a pull request it runs only when the corpus, the manifest or the recorder moved: nothing else can move the verdict away from the one the base commit published, and `verify` replays the corpus against the branch tree meanwhile. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
Contributor
📝 WalkthroughWalkthroughThe mobile workflow now runs recording pin validation on selected pushes to Priority: ⬇️ Low Merge Risk: 🔵 Low · up to A malformed recording-suite path can pass the new validation despite the required test file being absent. Tighten the check before merging so the CI guard reliably enforces its suite contract. 🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
Full details: Description checkExplanation The description gives detailed scope, rationale, implementation details, evidence, runtime data, and test results. It does not follow the repository template because it omits the required ELI5, Why, Linked Issue, Visual Proof, Testing, Review, Agent skill upstream boundary, Notes, and Checklist sections. It also does not provide the required issue link in the Resolution Add the missing template sections. Include a linked issue using
✨ 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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: 7994dcbb-bed1-49e0-bfb8-d5a6d0afc696
📒 Files selected for processing (4)
.github/workflows/mobile.ymlmobile/scripts/rpc-recording-pin-guard.mtsmobile/scripts/rpc-recording-pin-guard.test.tsmobile/vitest.config.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review.
Round-1 review of the pin guard. The pull_request ancestry check read the pin out of the merge preview and judged it against the branch head. Those differ whenever main repins after the branch point, so ordinary stale branches failed, and the instruction told the author to repin to their own head -- which creates the unreachable pin the guard exists to catch. Judge the checked-out tree instead. `git worktree prune` in the reproduce teardown was repository-wide. This git directory is shared by every worktree on the machine (611 registered here), so it could deregister an unrelated one whose directory was momentarily missing. `worktree remove --force` alone is enough; a failure to remove is now reported rather than papered over. Also: the concurrency group is per commit on main, because GitHub cancels a pending run in a group whatever `cancel-in-progress` says; the skip gate fails closed when a provenance path stops matching instead of skipping forever; the census-boundary comment states the rule the code uses; and five exports with no consumer are now module-private. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
️ No critical issues — minor suggestions inline.
Reviewed changes
- Pin guard script (
mobile/scripts/rpc-recording-pin-guard.mts): newancestryandreproducesubcommands.ancestryrequiresbaselineto be an ancestor of the ref and refuses to answer on a shallow clone;reproducechecks the pin out detached, lays the candidate recorder over it, points the golden/scenario env at the candidate tree, and replays the drivers plus the two census suites throughcompareGolden. - CI job (
.github/workflows/mobile.yml): mainpushtrigger,verifygated to pull requests, PR-onlycancel-in-progress, and arecording-pinjob withfetch-depth: 0that always checks ancestry and runs reproduction unconditionally onmainwhile gating it behind--if-changed-sinceon pull requests. - Unit tests (
mobile/scripts/rpc-recording-pin-guard.test.ts): 12 cases over throwaway repositories covering the ancestry verdicts (drift, branch-pinned, squash, missing commit, shallow) and the provenance gate. - Test collection (
mobile/vitest.config.ts):scripts/**/*.test.tsis now collected, so the guard test actually runs underpnpm test.
The reproduction's input provenance is sound: the run reads product sources from the pinned tree and only mobile/rpc-foundation plus mobile/src/test-support/rpc-recording from the candidate, and the PR-side verify job replays the candidate corpus against the candidate tree and dependencies, so a skipped reproduction cannot hide a dependency-driven divergence.
️ Nitpicks
mobile/scripts/rpc-recording-pin-guard.mts:29-33— the boundary comment says every suite that resolves its directory fromRPC_FOUNDATION_GOLDENSruns inCORPUS_CENSUS_SUITES, butmutants/pilot-mutants.test.tsandmutants/probe-hole-witness.test.tsalso do. They are correctly excluded (they exercise mutated source, not corpus provenance), so just narrow the wording to the census suites.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…oduction Two bot findings on the skip gate. `git diff` sees tracked paths only, but the reproduction's overlay copy and its census both read the corpus directory as it sits on disk, so an untracked golden or manifest is input to the verdict and used to skip the run that would judge it. Enumerate untracked entries under the provenance paths the way the recorder already does, and run rather than skip: an unjudged local addition is the case the reproduction exists for. The guard script is now a provenance path of its own, so a change to it re-runs the reproduction it implements. Left alone deliberately: run-process.ts and the workflow's `paths:` scope over src/shared, which is a pre-existing gap for the whole mobile workflow rather than this job's. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review, the two open threads were addressed and the guard hardened:
- Restored a per-commit concurrency group —
group: mobile-${{ github.event.pull_request.number || github.sha }}withcancel-in-progress: true, so a burst of main merges no longer drops an intermediate commit's verdict. - Judge the pin against the checked-out tree — dropped
--ref head.sha; on a PR the ancestry check now reads both thebaselineand the ref from the merge preview, which avoids false failures for branches cut before a main repin. - Closed the untracked/rename holes in the provenance gate —
corpusProvenanceChangednow throws when a provenance path no longer exists (a rename can't silently skip the run) and forces a reproduction on any untracked entry under those paths. - Counted the guard itself as provenance — moving
rpc-recording-pin-guard.mtsnow forces a reproduction, since it decides the skip. - Per-tree worktree teardown — replaced
git worktree remove --forceplus repo-widegit worktree prunewithremoveScratchWorktree, so an unrelated worktree whose directory is momentarily missing stays registered. - New unit coverage — merge-preview ancestry, guard/recorder provenance, untracked goldens, rename refusal, and scratch-worktree teardown.
Both prior threads were replied to and resolved. The ancestry-vs-merge-preview change is the right call: the old head-sha judgment would have failed ordinary branches cut before a repin, and the squash case is still caught by the push-to-main run.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
…m the files Round-2 review. The suite names reach vitest as positional filename filters, and vitest exits 0 when only some of them match. A renamed census suite therefore dropped out of the reproduction silently and the guard still printed that the corpus reproduces: three files and 761 tests instead of four and 762, exit 0. Resolve every name under the recorder overlay before spawning, and throw naming the drifted entry. The unit case walks the list and omits each name in turn, so no single rename can slip past it. This is the same fail-open shape as the renamed-pathspec finding. Also: pass an explicit directory type to `symlink`, since Windows needs one and a junction needs no privilege where a real symlink does; and build the throwaway test repositories with `symbolic-ref` rather than `--initial-branch`, which needs git 2.28 against a declared baseline of 2.25. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes
Since the prior Pullfrog review (90c56da), the reproduction was hardened against a silently drifting suite list and made friendlier on Windows:
- Fail closed on a drifted suite list —
assertReproductionSuitesExistresolves every name in the newly exportedREPRODUCTION_SUITES(drivers plus census suites) against the candidate overlay before the scratch tree is created, so a renamed suite can no longer drop out of the positional vitest run while the run still exits 0. - Windows directory links — the two
node_modulessymlinks now pass'junction'on Windows (no privilege needed) and'dir'elsewhere. - Older-git test setup — the throwaway-repository helper swaps
git init --initial-branch=main(git ≥ 2.28) forgit initplussymbolic-ref HEAD refs/heads/main, matching the repo's 2.25 baseline. - New unit coverage — one case proves every
REPRODUCTION_SUITESname resolves in this repository; a per-suite loop drops exactly one file at a time and proves the assertion throws naming the drifted suite, so it fails if the check is ever neutered.
Checked against the candidate tree is the right root: reproduceFromPin removes the pinned overlay and copies the candidate one over it before running, so the candidate overlay is exactly the set of files vitest will see.
DeepSeek Flash (free via Pullfrog for OSS) ｜ 𝕏
Contributor
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: 3e90537d-57ce-49f9-b320-4d4ba20fdcf6
📒 Files selected for processing (2)
mobile/scripts/rpc-recording-pin-guard.mtsmobile/scripts/rpc-recording-pin-guard.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 7 remain after this review.
Comment on lines
+59
to
+61
| export function assertReproductionSuitesExist(root: string): void { | ||
| for (const suite of REPRODUCTION_SUITES) { | ||
| if (!existsSync(resolve(root, RECORDER_OVERLAY, suite))) { |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,85p' mobile/scripts/rpc-recording-pin-guard.mts
sed -n '190,235p' mobile/scripts/rpc-recording-pin-guard.mts
sed -n '235,275p' mobile/scripts/rpc-recording-pin-guard.test.ts
rg -n "REPRODUCTION_SUITES|assertReproductionSuitesExist|vitest.*run|\\.test\\.ts" mobile/vitest.config.ts mobile/scripts/rpc-recording-pin-guard.mts mobile/scripts/rpc-recording-pin-guard.test.tsRepository: stablyai/orca
Length of output: 9477
🏁 Script executed:
set -eu
printf '%s\n' '--- mobile package metadata ---'
rg -n -C 3 '"vitest"|"test"' mobile/package.json package.json pnpm-lock.yaml yarn.lock package-lock.json 2>/dev/null | head -120 || true
printf '%s\n' '--- focused test imports and suite-presence tests ---'
sed -n '1,30p' mobile/scripts/rpc-recording-pin-guard.test.ts
sed -n '238,268p' mobile/scripts/rpc-recording-pin-guard.test.ts
printf '%s\n' '--- configured suite files ---'
find mobile/src/test-support/rpc-recording -maxdepth 1 -printf '%y %f\n' | sortRepository: stablyai/orca
Length of output: 6117
🌐 Web query:
Vitest 4.1.11 CLI run positional file path filter directory no test files exits 0 official documentation
💡 Result:
<search_synthesis>
In Vitest 4.1.11, if you use a positional file path filter in the CLI and no test files are found, Vitest will exit with a non-zero exit code (typically code 1) by default [1][2]. To prevent this and have the process exit with code 0 even when no tests are matched, you must use the --passWithNoTests flag [3][4][5]. According to official Vitest documentation, the --passWithNoTests option is specifically designed to allow the process to pass when no tests are found [3][4]. Without this flag, Vitest enforces that at least one test file must match the provided filter; otherwise, it treats the absence of tests as a failure state, resulting in a non-zero exit code [1][2]. Summary of behavior: - Default: Vitest exits with code 1 if no tests match your positional filter [1][2]. - With --passWithNoTests: Vitest exits with code 0 if no tests match your positional filter [3][4]. Note that this option is global and specifically meant to handle scenarios where the filter might not match any files during a particular run [3][5]. Ensure that your CLI command includes this flag to achieve the desired exit behavior [3].
</search_synthesis>
<source_evidence>
Citations:
- 1: https://github.com/vitest-dev/vitest/blob/c84113f410d76e15fed3cebd83491f628396b5ab/packages/vitest/src/node/core.ts
- 2: https://tangled.org/vitest.dev/vitest/commit/003c0bef48dcd13f660162578eb914fe1482c801
- 3: https://vitest.dev/guide/cli
- 4: https://vitest.dev/guide/cli.html
- 5: https://v3.vitest.dev/guide/cli
Require a regular file for each suite.
When the suite path is a directory, existsSync returns true, so assertReproductionSuitesExist accepts it even though the configured suite file is absent. Check statSync(...).isFile() and add a directory fixture to this test group.
Proposed fix
-import { existsSync } from 'node:fs'
+import { existsSync, statSync } from 'node:fs'
for (const suite of REPRODUCTION_SUITES) {
- if (!existsSync(resolve(root, RECORDER_OVERLAY, suite))) {
+ const suitePath = resolve(root, RECORDER_OVERLAY, suite)
+ if (!existsSync(suitePath) || !statSync(suitePath).isFile()) {📝 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.
Suggested change
| export function assertReproductionSuitesExist(root: string): void { | |
| for (const suite of REPRODUCTION_SUITES) { | |
| if (!existsSync(resolve(root, RECORDER_OVERLAY, suite))) { | |
| import { existsSync, statSync } from 'node:fs' | |
| export function assertReproductionSuitesExist(root: string): void { | |
| for (const suite of REPRODUCTION_SUITES) { | |
| const suitePath = resolve(root, RECORDER_OVERLAY, suite) | |
| if (!existsSync(suitePath) || !statSync(suitePath).isFile()) { |
ivanvykopal
pushed a commit
to ivanvykopal/orca
that referenced
this pull request
Sep 20, 2026
…or the corpus does not reproduce (stablyai#21156) * test(mobile): fail CI when the RPC recording pin leaves main's history `mobile/rpc-foundation/pilot-scenarios.json` carries the commit every golden claims it was recorded from, and `--record` refuses on any other tree. A behaviour-change branch pins its own last fenced commit, which stops being reachable the moment the branch squash-merges: nobody can record on main again until a hand-made repin lands, and until now only a human noticed. stablyai#21123 was that, and so was the repin after stablyai#20954. `scripts/rpc-recording-pin-guard.mts ancestry` fails when the pin is not an ancestor of the commit under test, and prints the repin recipe. It refuses to answer on a shallow clone rather than trusting grafted history, so the job checks out with `fetch-depth: 0`. Ordinary product drift past a reachable pin is not a failure. `reproduce` makes the other claim the corpus header makes, which the recording suites do not: they replay the goldens against the CURRENT tree, so a golden recorded somewhere other than the pin -- a merge that auto-merged golden JSON, a refresh copied back from a scratch directory -- passes them and is what the header exists to deny. It checks the pin out detached, lays this tree's recorder and manifest over it, and lets the same suites compare in place, so the comparison is `compareGolden` with lockfile and platform masked as ever. It runs unconditionally on a push to main, which has no `verify` job and is where a squash lands a spliced corpus. On a pull request it runs only when the corpus, the manifest or the recorder moved: nothing else can move the verdict away from the one the base commit published, and `verify` replays the corpus against the branch tree meanwhile. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): judge the recording pin against the tree it was read from Round-1 review of the pin guard. The pull_request ancestry check read the pin out of the merge preview and judged it against the branch head. Those differ whenever main repins after the branch point, so ordinary stale branches failed, and the instruction told the author to repin to their own head -- which creates the unreachable pin the guard exists to catch. Judge the checked-out tree instead. `git worktree prune` in the reproduce teardown was repository-wide. This git directory is shared by every worktree on the machine (611 registered here), so it could deregister an unrelated one whose directory was momentarily missing. `worktree remove --force` alone is enough; a failure to remove is now reported rather than papered over. Also: the concurrency group is per commit on main, because GitHub cancels a pending run in a group whatever `cancel-in-progress` says; the skip gate fails closed when a provenance path stops matching instead of skipping forever; the census-boundary comment states the rule the code uses; and five exports with no consumer are now module-private. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): let an untracked golden and the guard itself buy a reproduction Two bot findings on the skip gate. `git diff` sees tracked paths only, but the reproduction's overlay copy and its census both read the corpus directory as it sits on disk, so an untracked golden or manifest is input to the verdict and used to skip the run that would judge it. Enumerate untracked entries under the provenance paths the way the recorder already does, and run rather than skip: an unjudged local addition is the case the reproduction exists for. The guard script is now a provenance path of its own, so a change to it re-runs the reproduction it implements. Left alone deliberately: run-process.ts and the workflow's `paths:` scope over src/shared, which is a pre-existing gap for the whole mobile workflow rather than this job's. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb * test(mobile): refuse to reproduce when the suite list has drifted from the files Round-2 review. The suite names reach vitest as positional filename filters, and vitest exits 0 when only some of them match. A renamed census suite therefore dropped out of the reproduction silently and the guard still printed that the corpus reproduces: three files and 761 tests instead of four and 762, exit 0. Resolve every name under the recorder overlay before spawning, and throw naming the drifted entry. The unit case walks the list and omits each name in turn, so no single rename can slip past it. This is the same fail-open shape as the renamed-pathspec finding. Also: pass an explicit directory type to `symlink`, since Windows needs one and a junction needs no privilege where a real symlink does; and build the throwaway test repositories with `symbolic-ref` rather than `--initial-branch`, which needs git 2.28 against a declared baseline of 2.25. Claude-Session: https://claude.ai/code/session_01JNnE9qzUZMMnqpZWCqM3nb
8 of 9 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What this is
A CI guard for the two claims mobile's RPC recording corpus makes about its pin, so the next broken pin is a red job instead of a hand-made repin PR.
mobile/rpc-foundation/pilot-scenarios.jsoncarriesbaseline, the commit every golden undermobile/rpc-foundation/goldens/was recorded against, and--recordrefuses on any other fenced tree. Two things break that only a person notices today:main, and nobody can record onmainuntil a repin lands. test(mobile): repin the RPC recording corpus to main after #21089 #21123 was that repin after refactor(mobile): checked reply readers for the session domain (step 7) #21089; test(mobile): repin the recording baseline to main after #20950 #21065 was that repin after refactor(mobile): checked reply readers for the source-control domain (step 7 pilot) #20950.What it adds
mobile/scripts/rpc-recording-pin-guard.mts, runnable locally, with two subcommands:ancestry [--ref <commit>]:baselinemust be an ancestor of the ref. Three failures: not an ancestor, not a commit, or a shallow clone (a refusal, not a pass, becausemerge-base --is-ancestoranswers from grafted parents). The failure message prints the exact repin recipe.reproduce [--if-changed-since <commit>]: checks the pin out detached undermkdtemp, lays this tree's recorder and manifest over it, points the golden and scenario paths at this tree, and runs the recording drivers plus the two corpus census suites there. The comparison iscompareGoldenitself with the same masks as every other run. Worktree and temp dir are removed in afinally..github/workflows/mobile.ymlgains arecording-pinjob withfetch-depth: 0:merge-basewith the basemaincancel-in-progressbecomes pull-request-only, so a supersededmaincommit keeps its verdict. The existingverifyjob is gated to pull requests so the new push trigger does not double the workflow's cost.Evidence
main(pin4b876758d3), FAIL with the repin instruction on4b876758d3~1, both reproduced by the lead.baselineuntouched passespilot-recordings.test.tsagainst the current tree and fails reproduction atsender[3].args[1].value.limit.4b876758d3(the squash of refactor(mobile): checked reply readers for the session domain (step 7) #21089) and on refactor(mobile): checked reply readers for the source-control domain (step 7 pilot) #20950's squash, and stays quiet on the test(mobile): repin the recording baseline to main after #19850 #21092 and test(mobile): repin the recording baseline to main after #21088 #21105 repins, whose pins were still ancestors. That matches the plan's rule that ordinary drift past a reachable pin needs nothing.--depth 1clone where the pin is reachable in the full repo and the guard refuses anyway, and a corpus change the base branch made without the PR not counting toward--if-changed-since.Runtime
Gates
pnpm typechecktscover the new.mtsoxfmt --check .frommobile/Known gap, not fixed here
mobile/tsconfig.jsonincludes**/*.ts, which does not match.mts, so neitherscripts/rpc-recording.mtsnor this guard is covered bypnpm typecheck. The new file was checked with an isolatedtscrun. Adding**/*.mtssurfaces pre-existing errors in the existing script and is its own follow-up.