fix(doctor): checkDependencies and checkTrackedBakFiles read the code root - #4138
fix(doctor): checkDependencies and checkTrackedBakFiles read the code root#4138Zoubeir23 wants to merge 1 commit into
Conversation
… root career-ops#3867 finding 6: both checks used projectRoot (getCareerOpsRoot()), but node_modules and .git only ever live where doctor.mjs itself sits. Under a split checkout (CAREER_OPS_ROOT/CAREER_OPS_DATA_DIR, or the .career-ops-data marker) that data root is a separate directory that never holds either — jday013/maxmilian's proof on the issue: an empty node_modules/ created inside the data root flips 'Dependencies not installed' to 'installed', and a data root with no .git silently skips a tracked-.bak warning that should have fired against the real code checkout. Introduces codeRoot alongside projectRoot: --target keeps meaning 'diagnose this whole other checkout' (both roots follow it, matching the existing tests/doctor-tracked-bak-files.test.mjs coverage), otherwise codeRoot falls back to __dirname. Two call sites needed it, not one — main()'s checks array AND the separate onboardingState() used by `doctor.mjs --json` (the form AGENTS.md has every agent run on session start) each ran their own, independently-broken checkTrackedBakFiles(root). Adds tests/doctor-code-root-checks.test.mjs, mutation-verified: reverting either call site reddens its assertion. Credit: jday013 and maxmilian for the split-checkout reproduction on career-ops-hq#3867.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Advanced Run ID: 📒 Files selected for processing (2)
🔗 Linked repositories identifiedCodeRabbit considers these linked repositories for cross-repo context during reviews:
Included review availability: Your plan provides up to 10 included reviews per hour; 8 remain after this review. 📝 WalkthroughWalkthroughChangesDoctor now separates the code checkout from the resolved data root. Dependency and tracked Code-root checks
Priority: ⬇️ Low Estimated code review effort: 3 (Moderate) | ~20 minutes Change: Bug fix Suggested reviewers: Merge Risk: ⚪ Minimal · up to The split-checkout doctor behavior is covered for plain and JSON output, with no actionable merge risk identified. 🚥 Pre-merge checks | ✅ 9✅ Passed checks (9 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
🚀 Post-Merge Actions
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 |
Scott-Emberson
left a comment
Scott-Emberson
left a comment
There was a problem hiding this comment.
I reviewed the owned test tests/doctor-code-root-checks.test.mjs. It is sound and mutation-sensitive, and it drives the real code.
The test spawns the real doctor.mjs (execFileSync from the code root, with CAREER_OPS_ROOT pointed at a throwaway data root), so it exercises both driver paths: the plain-text main() checks and the separate onboardingState() path behind --json. No import of the functions, no reconstructed logic. It sets up the split scenario deliberately: the throwaway data root is its own git checkout carrying a tracked stray.md.bak and no node_modules, while the code root has node_modules and no such tracked file. Then it asserts, with literal expecteds, that the output does not say "Dependencies not installed" (so checkDependencies read the code root's node_modules), and that stray.md.bak appears in neither the plain output nor the parsed --json warnings[] (so checkTrackedBakFiles read the code root, not the data root). That discriminates code-root from data-root rather than just confirming the command ran.
I mutation-checked all three. Pointing checkDependencies at the project root reddens assertion 1, pointing main()'s checkTrackedBakFiles at the project root reddens assertion 2, and pointing onboardingState()'s call at the data root reddens assertion 3, each independently, green again on full revert. So both checks are covered in both callers and none of the three is a false-pass. The suite also has no process.exit( in it, so test-all runs it rather than refusing it, and it is hermetic (temp data root, isolated git config, cleaned in finally).
Owned test sound, not a false-pass. The substance is in doctor.mjs, so the routing and merge decision is the maintainer's, not mine to gate.
Fixes finding 6 of #3867.
checkDependencies()andcheckTrackedBakFiles()both readprojectRoot(
getCareerOpsRoot()), butnode_modulesand.gitonly ever live wheredoctor.mjsitself sits. Under a split checkout (CAREER_OPS_ROOT/CAREER_OPS_DATA_DIR, or the.career-ops-datamarker) that data root is aseparate directory that never holds either.
jday013/maxmilian's proof on the issue: creating an empty
node_modules/inside the data root flips "Dependencies not installed" to "installed" even
though the real code checkout's own
node_modulesnever moved. The inverseholds for the
.bakcheck — a data root with no.gitsilently skips awarning that should have fired against the real code checkout.
Fix
Introduces
codeRootalongsideprojectRoot:--target <path>keeps meaning "diagnose this whole other checkout" — bothroots follow it, matching the existing coverage in
tests/doctor-tracked-bak-files.test.mjs.codeRootfalls back to__dirname(wheredoctor.mjslives).Two call sites needed it, not one:
main()'s human-readable checks array.onboardingState()used bydoctor.mjs --json— the formAGENTS.mdhas every agent run on the first message of every session — hadits own, independently broken
checkTrackedBakFiles(root)call. Foundthis while writing the test; it's the same bug hiding in a second place.
Tests
tests/doctor-code-root-checks.test.mjs(new): seeds a throwawayCAREER_OPS_ROOTdata root that is its own git repo with a tracked.bakfile and no
node_modules, then asserts bothdoctor.mjs(plain) anddoctor.mjs --jsonreport on the real code checkout instead. Mutation-checked— reverting either call site independently reddens its assertion.
node test-all.mjs: 8729 passed, 0 failed.Summary
doctor.mjsnow separatescodeRootfromprojectRoot.codeRoot/node_modules..bakchecks inspect the code checkout.tests/doctor-code-root-checks.test.mjs:1-101adds coverage for both output modes. Reported result: 8,729 passed and 0 failed.Touched system files:
doctor.mjsandtests/doctor-code-root-checks.test.mjs. No changes were reported forAGENTS.md,modes/,update-system.mjs,DATA_CONTRACT.md,providers/, or.github/.