Repository navigation
ci: run every cross-version wire test, picked up by folder so new ones can't be skipped - #24499
Conversation
…est is left out Three cross-version tests ran in no CI job because the job named its files by hand. Run the directory instead, ratchet that every file kept out of the unit shards runs in some PR job, and re-run the job when the modules the newly running tests guard change.
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. 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 (1)
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 6 remain after this review. 📝 WalkthroughWalkthroughThe cross-version wire job now runs Vitest against the full test directory. New CI tests check that excluded unit tests are covered by PR Vitest steps and that the directory’s tests match Priority: ⬇️ Low Merge Risk: ⚪ Minimal · up to The PR broadens CI test coverage, and the previously reported shell-test routing gap is addressed at the reviewed head. No unresolved current impact prevents merging, subject to normal CI checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The change broadens test coverage without showing additional repository-write permissions or production access. No introduced security concern was established. Complete dependency-to-job coverage and externally enforced cache and cancellation behavior remain unverified. 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)
✨ 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: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Advanced
Run ID: e4b2e907-d967-4b1a-bedd-ffb9ce40842d
📒 Files selected for processing (6)
.github/workflows/pr.ymlconfig/scripts/ci-unit-exclusion-coverage.test.mjsconfig/scripts/ci-unit-files.mjsconfig/scripts/pr-code-change-scope.mjsconfig/scripts/pr-code-change-scope.test.mjstests/e2e/cross-version-wire/orchestration-delivery-downgrade.unit.test.ts
Included review availability: This review used your included allowance. Your plan provides up to 10 included reviews per hour; 4 remain after this review.
| describe('unit files kept out of the sharded test job', () => { | ||
| it('each still runs in some PR job', () => { | ||
| const sharded = new Set(discoverUnitFiles()) | ||
| const excluded = posixPaths(globSync(UNIT_INCLUDE, { exclude: defaultExclude })).filter( | ||
| (file) => !sharded.has(file) | ||
| ) | ||
| const pathArguments = vitestPathArguments() | ||
|
|
||
| expect(excluded.length).toBeGreaterThan(0) | ||
| // Why: an excluded file no step names guards nothing, and nothing else reports it. | ||
| expect(excluded.filter((file) => !runByPrVitestStep(file, pathArguments))).toEqual([]) | ||
| }) | ||
|
|
||
| it('runs the whole cross-version-wire directory, not a list of its files', () => { | ||
| expect(vitestPathArguments()).toContain(CROSS_VERSION_WIRE_DIR) | ||
| }) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🔎 Supported by static analysis
🏁 Script executed:
sed -n '1,105p' config/scripts/ci-unit-exclusion-coverage.test.mjs
sed -n '1,95p' config/scripts/ci-unit-files.mjsRepository: stablyai/orca
Length of output: 4838
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- workflow files ---'
find .github/workflows -maxdepth 1 -type f -print | sort
printf '%s\n' '--- pr.yml ---'
cat -n .github/workflows/pr.yml
printf '%s\n' '--- locally called workflows referenced by pr.yml ---'
rg -n -C 3 'uses:\s*\./\.github/workflows/' .github/workflows/pr.yml
for f in $(rg -o 'uses:\s*\./\.github/workflows/[^[:space:]]+' .github/workflows/pr.yml | sed 's/^uses:[[:space:]]*//' | sort -u); do
printf '%s\n' "--- ${f#./} ---"
cat -n "${f#./}"
done
printf '%s\n' '--- vitest invocations and conditions across workflows ---'
rg -n -C 4 'vitest|^\s*(if|paths|paths-ignore|on):' .github/workflowsRepository: stablyai/orca
Length of output: 45665
🏁 Script executed:
set -eu
find .github/workflows -maxdepth 1 -type f -print | sort
printf '%s\n' '--- pr.yml ---'
cat -n .github/workflows/pr.yml
printf '%s\n' '--- locally called workflows ---'
rg -n -o 'uses:\s*\./\.github/workflows/[^\s]+' .github/workflows/pr.yml | sed 's/.*uses:[[:space:]]*//' | sort -u | while read -r path; do
printf '%s\n' "--- ${path#./} ---"
cat -n "${path#./}"
done
printf '%s\n' '--- workflow Vitest and condition context ---'
rg -n -C 4 'vitest|^\s*(if|paths|paths-ignore|on):' .github/workflowsRepository: stablyai/orca
Length of output: 45665
🏁 Script executed:
set -eu
find .github/workflows -maxdepth 1 -type f -print | sort
cat -n .github/workflows/pr.yml
rg -n -C 5 'uses:\s*\./\.github/workflows/|vitest|^\s*(if|paths|paths-ignore|on):' .github/workflowsRepository: stablyai/orca
Length of output: 45595
🏁 Script executed:
set -eu
printf '%s\n' '--- pr.yml relevant structure ---'
rg -n -C 3 'uses: \./\.github/workflows/|vitest|^\s{2}[A-Za-z0-9_.-]+:|^\s{4}if:|^\s{6}- name:' .github/workflows/pr.yml
printf '%s\n' '--- exact local workflow references ---'
rg -n 'uses: \./\.github/workflows/' .github/workflows/pr.yml
printf '%s\n' '--- Vitest commands in pr.yml and likely called workflows ---'
for f in .github/workflows/pr.yml .github/workflows/unit-tests.yml .github/workflows/unit-plan.yml .github/workflows/unit-selection-evidence.yml .github/workflows/pr-test-loc.yml; do
if test -f "$f"; then
printf '%s\n' "--- $f ---"
rg -n -C 8 'vitest|^\s{4}if:|^\s{2}[A-Za-z0-9_.-]+:|uses: \./\.github/workflows/' "$f"
fi
doneRepository: stablyai/orca
Length of output: 42578
🏁 Script executed:
set -eu
rg -n -C 3 'uses: \./\.github/workflows/|vitest|^\s{4}if:|^\s{2}[A-Za-z0-9_.-]+:' .github/workflows/pr.yml
rg -n 'uses: \./\.github/workflows/' .github/workflows/pr.yml
for f in .github/workflows/pr.yml .github/workflows/unit-tests.yml .github/workflows/unit-plan.yml .github/workflows/unit-selection-evidence.yml .github/workflows/pr-test-loc.yml; do
if test -f "$f"; then
printf '%s\n' "--- $f ---"
rg -n -C 8 'vitest|^\s{4}if:|^\s{2}[A-Za-z0-9_.-]+:|uses: \./\.github/workflows/' "$f"
fi
doneRepository: stablyai/orca
Length of output: 44429
🏁 Script executed:
set -eu
printf '%s\n' '--- classifier outline and relevant rules ---'
rg -n -C 8 'shell_contracts|cross-version-wire|test|package|git_compatibility|function|export|return|src/main/daemon|node-pty|live-shell|shell-ready' config/scripts/pr-code-change-scope.mjs
printf '%s\n' '--- complete current exclusion and discovery declarations ---'
cat -n config/scripts/ci-unit-files.mjs
printf '%s\n' '--- current diff for the changed assertion and related CI files ---'
git diff --stat 757736628f0dc0dfa972350b22524df0ca9d6f62d5 7bb42714a20e5e1c7a5264e4f140d9076a8e62d5 -- config/scripts/ci-unit-exclusion-coverage.test.mjs config/scripts/ci-unit-files.mjs .github/workflows/pr.yml config/scripts/pr-code-change-scope.mjsRepository: stablyai/orca
Length of output: 22555
🏁 Script executed:
set -eu
rg -n -C 8 'shell_contracts|cross-version-wire|test|package|git_compatibility|src/main/daemon|node-pty|live-shell|shell-ready' config/scripts/pr-code-change-scope.mjs
cat -n config/scripts/ci-unit-files.mjs
git diff --stat 757736628f0dc0dfa972350b22524df0ca9d6f62d5 7bb42714a20e5e1c7a5264e4f140d9076a8e62d5 -- config/scripts/ci-unit-exclusion-coverage.test.mjs config/scripts/ci-unit-files.mjs .github/workflows/pr.yml config/scripts/pr-code-change-scope.mjsRepository: stablyai/orca
Length of output: 17294
🏁 Script executed:
set -eu
target='src/main/runtime/structured-session-cli-login-shell.live-shell.test.ts'
printf '%s\n' '--- exact path references in PR workflows and configs ---'
rg -n -C 4 -F "$target" .github/workflows config/scripts config/vitest.config.ts package.json
printf '%s\n' '--- Vitest config and unit exclusion references ---'
rg -n -C 8 'UNIT_EXCLUDE|unit-files|defaultExclude|exclude:|include:|structured-session|live-shell|vitest' config/vitest.config.ts config/scripts package.json
printf '%s\n' '--- classifier shell detector and changed-path behavior ---'
sed -n '80,110p' config/scripts/pr-code-change-scope.mjs
sed -n '418,472p' config/scripts/pr-code-change-scope.mjsRepository: stablyai/orca
Length of output: 45663
🏁 Script executed:
set -eu
target='src/main/runtime/structured-session-cli-login-shell.live-shell.test.ts'
rg -n -C 4 -F "$target" .github/workflows config/scripts config/vitest.config.ts package.json
rg -n -C 8 'UNIT_EXCLUDE|unit-files|defaultExclude|exclude:|include:|structured-session|live-shell|vitest' config/vitest.config.ts config/scripts package.json
sed -n '80,110p' config/scripts/pr-code-change-scope.mjs
sed -n '418,472p' config/scripts/pr-code-change-scope.mjsRepository: stablyai/orca
Length of output: 45655
Route the structured-session shell test to shell_contracts.
src/main/runtime/structured-session-cli-login-shell.live-shell.test.ts is excluded from the sharded unit job and listed in the shell Vitest command. However, its path matches no SHELL_PREFIXES entry. A PR that changes only this test therefore skips shell_contracts. The new assertion still passes because it checks the textual Vitest path, while the test runs in no PR Vitest job.
Suggested fix
'src/main/terminal-history-fish',
'src/main/zsh-',
+ 'src/main/runtime/structured-session-cli-login-shell',
'src/renderer/src/components/terminal-pane/fish-color-scheme',![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
ℹ️ Minor suggestions only.
Reviewed changes
- Cross-version job runs the folder:
.github/workflows/pr.ymlnow passestests/e2e/cross-version-wire/to vitest instead of a hand-typed list, so a new*.unit.test.tsthere runs with no workflow edit; an empty match still exits non-zero. - One folder constant:
ci-unit-files.mjsexportsCROSS_VERSION_WIRE_DIRand reuses it inUNIT_EXCLUDE, so the exclude pattern and the workflow argument share a source. - New coverage guard:
ci-unit-exclusion-coverage.test.mjsparsespr.ymlplus its directly-called reusable workflows and fails when a unit file the sharded job excludes is named by no PR vitest step, when the directory argument is absent, or when a cross-version test is named soUNIT_INCLUDEmisses it. - Trigger expansion:
pr-code-change-scope.mjsadds the agent-status/turn modules and orchestration DB paths so the newly-running release tests start the job on their production dependencies. - Timeout budget:
orchestration-delivery-downgrade.unit.test.tsmoves from the 30 s default to120_000, matching its downgrade siblings.
I verified the central claim independently: vitest list --config config/vitest.config.ts tests/e2e/cross-version-wire/ selects all 13 *.unit.test.ts files (including the 3 previously unlisted), the new guard passes on this head, all added prefix entries map to real files, and every extra UNIT_EXCLUDE entry is named by a PR vitest step.
Technical details
# Guard models Vitest's positional args more strictly than Vitest does
## Affected sites
- `config/scripts/ci-unit-exclusion-coverage.test.mjs:37` — `path === file || (path.endsWith('/') && file.startsWith(path))`.
## Required outcome
- The coverage guard should consider a file run when Vitest would run it. Today it only accepts an exact file path or a root-anchored directory prefix, while Vitest matches positional arguments as substrings.
## Suggested approach (optional)
- `pathArguments.some((path) => file.includes(path))` mirrors Vitest for both the file and directory forms.
## Open questions for the human (optional)
- If the intent is deliberately to accept only root-anchored paths, a one-line comment saying so would make the stricter contract explicit.deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
|
|
||
| function runByPrVitestStep(file, pathArguments) { | ||
| return pathArguments.some( | ||
| (path) => path === file || (path.endsWith('/') && file.startsWith(path)) |
There was a problem hiding this comment.
This treats a directory argument as a root-anchored prefix (file.startsWith(path)), but Vitest matches positional arguments as substrings — confirmed in this repo: vitest list --config config/vitest.config.ts cross-version-wire/ selects the same files, and the pr.yml comment at the mobile-web-app step documents the substring semantics. A future workflow edit using a shorter but valid filter such as cross-version-wire/ would run the tests yet fail this guard. Matching with file.includes(path) mirrors Vitest; if the stricter root-anchored contract is intentional, a one-line comment would make that explicit.
…cluded file's job to fire on it The coverage check counted any pr.yml job, including e2e, terminal IME and Windows WSL, which are left out of verify.needs and so cannot block a merge. It now reads verify.needs and the reusable workflows those jobs call. It also only proved that some step names each excluded file, not that the job runs when the file changes. The structured-session zsh login-shell test runs only in shell_contracts, whose path trigger matched neither it, its harness nor its subject, so a PR touching only those ran it nowhere. The check now asserts a change to each excluded file fires a gating job that names it, and the shell trigger gains those three paths.
…e schema version-skew resolver A change confined to src/shared/agent-turn-outcome (the arms a newer host publishes) or to orchestration-schema-version-skew (how current code reopens a downgraded database) skipped the job whose tests guard exactly those contracts. Also corrects the publish/read direction in the turn-end comment.
There was a problem hiding this comment.
ℹ️ No new issues found in this delta. The guard rework is sound and the new assertions are load-bearing; the one prior suggestion on
runByVitestStep's matching is unchanged, so this stays non-approving while that thread remains open.
Reviewed changes
- Guard now counts only merge-gating jobs:
gatingJobs()iteratesverify.needs, parses each job'sneeds.code_paths.outputs.<name> == 'true'condition into a gate, and inherits a reusable workflow caller's gate into its jobs — so a file run only by a non-gating job (e2e, ime, wsl) no longer counts as covered. - New trigger assertion: a second test requires that a change to each excluded unit file fires the gate of the gating job that runs it, via
classifyPrJobs([file])[gate]. - Trigger-list additions:
SHELL_PREFIXESgains the structured-session login-shell test, its harness and its subject;CROSS_VERSION_WIRE_PREFIXESgainsagent-turn-outcomeandorchestration-schema-version-skew, each pinned bypr-code-change-scope.test.mjs. - Timeout rationale: the orchestration downgrade test now records why it needs the 120 s budget.
I ran both changed CI-script tests (45 passing), enumerated every verify.needs job to confirm each vitest-running gating job parses a gate, and confirmed every added prefix maps to a real file and is load-bearing for classifyPrJobs.
deepseek-v4.1-flash (free via Pullfrog for OSS) ｜ 𝕏
Review status: ready for a merge decisionProblem. The cross-version wire job ran a hand-typed list of test files. The main unit job skips that whole folder on purpose, so any file missing from the list ran in no CI job. Three files had been left out: What changes for you. Nothing in the app. In CI:
Fixed during review (on top of the original two commits):
Deferred (low severity, P3):
Verified:
Not verified: no app or live QA, because this changes only CI and touches nothing in the app. The three newly running tests were not cold-started locally; CI covers the cold start. |
1 participant
ELI5
Orca has "cross-version" tests: they load an older Orca release next to the current code and check the two still work together, because a phone, a desktop and a remote server update at different times. Three of these tests were never run by CI. They passed or failed with nobody watching, so they protected nothing. This PR makes CI run every test in that folder, including ones added later, and adds a check that fails if a test is ever left out again.
What Changed
The problem. The
cross-version wire compatibilityjob in.github/workflows/pr.ymlran a list of test files typed out by hand. The main unit test job skips that whole folder on purpose: these tests need the full git history to find old releases, and the main job checks out without it. So a file in the folder ran only if someone remembered to add it to the list. Three files were never added:cross-version-host-observed-turn-end.unit.test.tsorchestration-delivery-downgrade.unit.test.tspublished-field-shape.unit.test.tsNothing reported this. CI stayed green whether these tests passed or not.
The fix.
The job now runs the folder,
tests/e2e/cross-version-wire/, instead of a list. A new test file there runs in CI as soon as it is added, with no list to update. If the folder ever matches no tests, vitest exits with an error, so the job still cannot pass while running nothing.A new check,
config/scripts/ci-unit-exclusion-coverage.test.mjs, runs with the normal unit tests. It finds every test file the main unit job deliberately skips and fails unless:verifygate (e2e, real IME, real WSL terminal) don't count, because they can be red or skipped without stopping a merge.This covers this folder and every other skipped file, such as the real-shell tests and the relay-region tests. It also fails if a test in the folder is named so that vitest would not pick it up (anything not ending in
.unit.test.ts).The cross-version job only starts when a PR touches certain paths, and that path list is also kept by hand. I added the modules the two newly running release tests check, so a change to that code now starts the job: the agent status and turn-outcome modules for the turn-end test, and the orchestration database and its schema-version handling for the downgrade test.
The second rule in step 2 found one more test that already ran nowhere when edited:
src/main/runtime/structured-session-cli-login-shell.live-shell.test.tsruns only in theshell contractsjob, but a change to it, its test harness, or the module it tests (structured-session-child-identity-env) didn't start that job. Those three paths now do.Nothing changes in the app. Only CI is affected.
What each newly running test checks
interruption), that it could not confirm the end (unconfirmed), or that a newer request replaced it (superseded). An older desktop reading those values must keep the row and show it as done, not drop it or treat it as a user stop. Covers live status, sleeping-agent records and journal turns.Why
Running the folder takes the hand-kept list out of the loop entirely. The alternative was to keep the list and add a check that it matches the folder. That still makes every author edit the workflow, and it is the step that was already forgotten three times. The coverage check goes further than this folder because the real bug is general: any file the main unit job skips must run somewhere, and before this nothing checked that.
Testing
pr-code-change-scope,pr-workflow-parallelism,ci-unit-planandci-unit-selection.vitest list --config config/vitest.config.ts tests/e2e/cross-version-wire/selects all 13 test files in the folder: the 10 the list named plus the 3 above.vitest run --config config/vitest.config.ts <file>), on a heavily loaded Mac:published-field-shape: 8/8 pass.cross-version-host-observed-turn-end: 5/5 pass.orchestration-delivery-downgrade: passes (1/1) once the old release is unpacked. The first run on this machine hit the 30-second test timeout while unpacking that release, so I passed a longer--testTimeoutto get one clean run. With the release already unpacked, the test takes about 3 seconds. In CI, tests built the same way unpack their release in 6 to 14 seconds.Notes
orchestration-delivery-downgradeunpacks an old release inside its test body. That unpacking has its own 45 s deadline, which alone is longer than vitest's 30 s default test timeout, so the test now has a 120 s budget. In CI the file took about 8 s cold. No assertion changed. Running the three extra files adds about 5 to 10 s to the cross-version job (now about 58 s for its test step, 13 files, 82 tests).config/scripts/pr-code-change-scope.mjs. Deriving them from the import graph isn't a clear win: the path-detection job checks out only a handful of files and installs no dependencies, and the full import closure of these tests is around 3,000 source files, so a graph-based trigger would start the job on most PRs. Some modules existing tests check are still not on the list (for example the session-tab retirement proof and the worktree identity migration); adding those is a separate change.