Repository navigation
refactor(test): use __fixtures__ for wrapper snapshots and teach the LoC bot - #15365
Conversation
…LoC bot #15363 parked the generated wrapper fixtures under __tests__ because that was the only name the PR LoC classifier recognized. __fixtures__ is the convention this repo already uses for non-TS test data (src/shared, src/main/startup, terminal-pane), so fix the classifier instead of bending the layout around it. isTestPath now also counts __fixtures__ and __snapshots__ segments. Both are __-wrapped, so a directory cannot carry the name by accident. Bare fixtures/ is deliberately left out: it reads as a plausible prod module name, and this classifier bills every PR in the repo. That also un-bills five dirs that were already misclassified as prod today: 3 __fixtures__ and 2 __snapshots__. All 18 files are pure renames, verified byte-identical to main by sha256.
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (22)
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review. 📝 WalkthroughWalkthroughThe change moves shell-wrapper snapshots from Merge Risk: ⚪ Minimal · up to This PR only relocates byte-identical test fixtures and updates test-path classification with focused tests; it does not change production behavior or deployment configuration. No actionable merge-blocking risk remains after normal checks. 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 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: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b7cdb05-306b-4802-940b-77f9cd90c090
📒 Files selected for processing (22)
.gitattributes.github/scripts/pr-test-loc-table.mjsconfig/scripts/pr-test-loc-summary.test.mjssrc/main/__fixtures__/shell-wrapper-snapshots/README.mdsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-fish-init.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zlogin.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zprofile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshrc.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-fish-init.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zlogin.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zprofile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshrc.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zlogin.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zprofile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshrc.txtsrc/main/shell-wrapper-generated-file-snapshot.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txt (1)
53-60: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftSet
ZDOTDIRtoORCA_ORIG_ZDOTDIRbefore sourcing the relay user.zshenv.
ZDOTDIRstill points to the Orca wrapper when"$ORCA_ORIG_ZDOTDIR/.zshenv"runs. User startup files that resolve paths through$ZDOTDIRcan load the wrong files. Apply this change only torelay-zsh-zshenv.txt; the discovery wrappers intentionally unsetZDOTDIRbefore sourcing.src/main/__fixtures__/shell-wrapper-snapshots/README.md (1)
34-36: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the local snapshot-update warning.
The command does not pass
--update. Vitest’s local default isupdate: 'new', which writes missing snapshots but fails on mismatches. Use-u/--updateto rewrite changed snapshots.src/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txt (1)
53-55: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winDefine
omponly when the extension file exists.A stale
ORCA_OMP_STATUS_EXTENSIONreplaces the user'sompfunction, then__orca_ompbypasses it withcommand omp. Use[[ -f "${ORCA_OMP_STATUS_EXTENSION:-}" ]]for this guard.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 7b7cdb05-306b-4802-940b-77f9cd90c090
📒 Files selected for processing (22)
.gitattributes.github/scripts/pr-test-loc-table.mjsconfig/scripts/pr-test-loc-summary.test.mjssrc/main/__fixtures__/shell-wrapper-snapshots/README.mdsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-fish-init.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zlogin.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zprofile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/daemon-zsh-zshrc.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-fish-init.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zlogin.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zprofile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/local-zsh-zshrc.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-bash-rcfile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zlogin.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zprofile.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshenv.txtsrc/main/__fixtures__/shell-wrapper-snapshots/relay-zsh-zshrc.txtsrc/main/shell-wrapper-generated-file-snapshot.test.ts
Included review availability: Your plan provides up to 10 included reviews per hour; 1 remains after this review.
ELI5
Last PR put the generated shell-wrapper fixtures in a folder called
__tests__. That was picked to satisfy the bot that counts test-vs-prod lines, not because it was the right name — this repo already keeps non-TS test data in__fixtures__. So: rename the folder to the one we actually use, and fix the bot instead of bending the layout around it.What Changed
src/main/__tests__/shell-wrapper-snapshots/→src/main/__fixtures__/shell-wrapper-snapshots/(18 pure renames,src/main/__tests__/removed).github/scripts/pr-test-loc-table.mjs:isTestPathnow also counts__fixtures__and__snapshots__path segments.gitattributesreferences updated; new classifier cases inconfig/scripts/pr-test-loc-summary.test.mjsWhy
__fixtures__is this repo's existing convention for non-TS test data —src/shared/__fixtures__,src/main/startup/__fixtures__,src/renderer/src/components/terminal-pane/__fixtures__.src/main/providers/__tests__is the lone__tests__dir; everything else co-locates*.test.ts. #15363 used__tests__purely because it was the only nameisTestPathmatched, which is the layout following the tool rather than the other way round.Fixing the classifier also corrects five directories that are misbilled as prod today, independent of this move:
src/shared/__fixtures__PRODTESTsrc/main/startup/__fixtures__PRODTESTsrc/renderer/src/components/terminal-pane/__fixtures__PRODTESTsrc/main/runtime/orchestration/__snapshots__PRODTESTsrc/renderer/src/components/settings/__snapshots__PRODTESTWhy only the
__-wrapped namesBoth added names are
__-wrapped, so a directory cannot carry one by accident. Barefixtures/is deliberately not added: it reads as a plausible production module name, and this classifier bills every PR in the repo, so a false positive there is worse than a missed test dir. The two barefixtures/dirs outsidetests/(config/scripts/fixtures/,src/main/daemon/fixtures/) stay prod, and there is an explicit test pinning that.This PR's own LoC table will be wrong — expected
pr-test-loc.ymlfetches the classifier from the default branch, neverpull/<n>/head, because the job holds a write-scopedGITHUB_TOKENand may only execute reviewed code. So the table on this PR is still computed with the old regex; the fix applies from the next PR onward. Low stakes here — the fixtures move as renames, which carry 0 added/deleted lines either way.Testing
Platform: macOS 26.3.1 (arm64).
origin/main— all identical.git show --stat -Mshows every fixture as| 0.shell-wrapper-generated-file-snapshot.test.ts— 7 passed, andgit diffon the fixture dir is empty afterwards. That second check is the one that counts:toMatchFileSnapshotrewrites fixtures on mismatch outside CI, so a green run alone proves nothing.config/scripts/pr-test-loc-summary.test.mjs— 14 passed, including the new case asserting__fixtures__/__snapshots__are test and barefixtures/plusmy__fixtures__helper.tsare not.npx vitest runoversrc/main/providers/,src/main/daemon/,src/relay/,config/scripts/— 4540 passed. The 3 failures inpty-subprocess-env-inheritance.test.tsare pre-existing onmain(12550fc): verified by stashing this branch's changes and re-running against a clean tree, where they fail identically.npx tsc --noEmit -p config/tsconfig.node.json,npx oxlint,npx oxfmt --check— all clean.check-max-lines-ratchet— OK, no new bypasses.git grep '__tests__/shell-wrapper'returns nothing. The README's../../link still resolves (depth unchanged).Cross-platform: no runtime code touched. The
text eol=lfpin on the fixtures carries over to the new path, so byte-for-byte comparison stays safe against a CRLF checkout.Visual Proof
N/A— no UI surface. Directory rename, a regex, and its tests.Linked Issue
N/A — review follow-up to #15363 / #15245.
Notes
Security:
pr-test-loc.ymlis unchanged — the trusted-ref fetch that keeps the write-scoped token away from PR-authored code stays exactly as it is. This PR only edits the script that action pulls from the default branch. Performance and backwards compatibility: none, test-only.Still not in this PR: the three bash rcfile generators remain three near-identical copies (345 / 339 / 319 fixture lines each) — #15245 unified zsh only.
Agent skill upstream boundary
docs/reference/agent-skill-sharing-upstream-boundary.mdand copies or mechanically translates no upstream skill-installer source, tests, fixtures, registry entries, path tables, comments, or documentation.Checklist
N/Awith reasonpnpm lint,pnpm typecheck,pnpm test, andpnpm buildpass (or CI will cover; local preferred)