diff --git a/doctor.mjs b/doctor.mjs index 5e784aa39f..ed68f008be 100644 --- a/doctor.mjs +++ b/doctor.mjs @@ -54,8 +54,16 @@ CLIs: ${VALID_CLIS.join(', ')}`; validateFlags(argv, KNOWN_FLAGS, USAGE, { valueFlags: VALUE_FLAGS, requireOperand: true }); const targetIdx = argv.indexOf('--target'); -const projectRoot = - targetIdx !== -1 && argv[targetIdx + 1] ? argv[targetIdx + 1] : getCareerOpsRoot(); +const explicitTarget = targetIdx !== -1 && argv[targetIdx + 1] ? argv[targetIdx + 1] : null; +const projectRoot = explicitTarget || getCareerOpsRoot(); +// node_modules and .git belong to the CODE checkout, not the resolved data +// root — under a split checkout (CAREER_OPS_ROOT/CAREER_OPS_DATA_DIR or the +// .career-ops-data marker) those are two different directories, and neither +// ever holds the other's artifacts (career-ops#3867 finding 6). --target is +// the one case that means "diagnose this whole other checkout" — code layer +// included — so it keeps pointing both roots at the same place, matching how +// tests/doctor-tracked-bak-files.test.mjs already exercises it. +const codeRoot = explicitTarget || __dirname; const JSON_OUT = argv.includes('--json'); // --strict adds a live reachability probe of every portals.yml entry (network). // Opt-in so the default `npm run doctor` stays fast and fully offline. @@ -145,7 +153,7 @@ function checkBillingSource() { } function checkDependencies() { - if (existsSync(join(projectRoot, 'node_modules'))) { + if (existsSync(join(codeRoot, 'node_modules'))) { return { pass: true, label: 'Dependencies installed' }; } return { @@ -653,7 +661,7 @@ async function main() { geminiNodeFloor(activeCli, process.versions.node), checkBillingSource(), checkDependencies(), - checkTrackedBakFiles(projectRoot), + checkTrackedBakFiles(codeRoot), await checkPlaywright(), checkPlaywrightMcp(projectRoot, activeCli), checkScanExtractor(projectRoot), @@ -821,7 +829,12 @@ function onboardingState(root) { const mcpCheck = checkPlaywrightMcp(root, activeCli); const unpersonalized = unpersonalizedFiles(root); - const bakCheck = checkTrackedBakFiles(root); + // Every other check in this function is data-layer and correctly uses this + // function's own `root` parameter. The tracked-.bak check is the one + // code-layer exception (#3867 finding 6) — it must read the module-level + // codeRoot (the code checkout), which only differs from `root` when a real + // split-checkout data root is in play and no --target was given. + const bakCheck = checkTrackedBakFiles(codeRoot); const warnings = [ ...(cliWarning ? [cliWarning] : []), ...(mcpCheck?.warn ? [`${mcpCheck.label}\n→ ${[].concat(mcpCheck.fix || []).join('\n ')}`] : []), diff --git a/tests/doctor-code-root-checks.test.mjs b/tests/doctor-code-root-checks.test.mjs new file mode 100644 index 0000000000..e643074b55 --- /dev/null +++ b/tests/doctor-code-root-checks.test.mjs @@ -0,0 +1,101 @@ +// tests/doctor-code-root-checks.test.mjs — checkDependencies() and +// checkTrackedBakFiles() must read the CODE checkout, not the resolved data +// root (career-ops#3867 finding 6). +// +// node_modules and .git belong to wherever doctor.mjs itself lives. Under a +// split checkout (CAREER_OPS_ROOT/CAREER_OPS_DATA_DIR, or the .career-ops-data +// marker) getCareerOpsRoot() resolves to a separate data-only directory that +// never holds either — jday013/maxmilian's proof on #3867: an empty +// node_modules/ created inside the data root flips "Dependencies not +// installed" to "installed" even though the real code checkout's own +// node_modules never moved. The inverse held for the tracked-.bak check, and +// in two places: the human-readable checks array in main() AND the separate +// onboardingState() used by `--json` (the form AGENTS.md has every agent run +// on the first message of every session) each ran their own +// checkTrackedBakFiles(root) against the data root. +// +// This does not use --target: that flag means "diagnose this whole other +// checkout" (code layer included) and tests/doctor-tracked-bak-files.test.mjs +// already pins that it keeps checking the targeted directory. Here the split +// is the ambient one a real installation hits — CAREER_OPS_ROOT set, no +// --target — so codeRoot must fall back to doctor.mjs's own directory. +import { pass, fail, NODE, ROOT } from './helpers.mjs'; +import { execFileSync } from 'child_process'; +import { mkdtempSync, writeFileSync, rmSync } from 'fs'; +import { tmpdir } from 'os'; +import { join } from 'path'; + +console.log('\ndoctor.mjs — code-root checks stay on the code checkout under a split root (#3867)'); + +const DOCTOR = join(ROOT, 'doctor.mjs'); + +function git(cwd, ...args) { + execFileSync('git', args, { cwd, stdio: 'ignore' }); +} + +function runDoctor(dataRoot, extraArgs = []) { + try { + const out = execFileSync(NODE, [DOCTOR, ...extraArgs], { + cwd: ROOT, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'pipe'], + env: { ...process.env, CAREER_OPS_ROOT: dataRoot }, + }); + return { out, code: 0 }; + } catch (e) { + // doctor.mjs exits 1 on a real failing check (e.g. missing cv.md in the + // throwaway data root) — that is expected here and not itself a test + // failure, so stdout is still read from the caught error. + return { out: e.stdout ? String(e.stdout) : '', stderr: e.stderr ? String(e.stderr) : '', code: e.status ?? 1 }; + } +} + +const dataRoot = mkdtempSync(join(tmpdir(), 'co-doctor-coderoot-')); +try { + // The data root: no node_modules (the real, always-true shape of a + // data-only install), but IS its own git checkout with a tracked .bak file + // — the strongest proof that the checks stopped reading it, not merely + // that they happen to also pass elsewhere. + git(dataRoot, 'init', '-q'); + git(dataRoot, 'config', 'user.email', 'test@example.com'); + git(dataRoot, 'config', 'user.name', 'Test'); + writeFileSync(join(dataRoot, 'stray.md.bak'), 'a data-root backup that is not the code checkout\n', 'utf-8'); + git(dataRoot, 'add', 'stray.md.bak'); + git(dataRoot, 'commit', '-q', '-m', 'tracked backup living in the data root only'); + + // 1. Plain-text mode drives main()'s checks array: checkDependencies() and + // checkTrackedBakFiles(codeRoot). + const plain = runDoctor(dataRoot); + if (plain.stderr) { + fail(`doctor crashed under a split CAREER_OPS_ROOT (plain mode): ${plain.stderr}`); + } else { + if (!/Dependencies not installed/.test(plain.out)) { + pass("checkDependencies() reads the code checkout's own node_modules, not the split data root"); + } else { + fail(`checkDependencies() reported the code checkout's real node_modules as missing:\n${plain.out}`); + } + if (!/stray\.md\.bak/.test(plain.out)) { + pass("main()'s checkTrackedBakFiles() does not surface a .bak file tracked only in the split data root"); + } else { + fail(`main()'s checkTrackedBakFiles() leaked the data root's tracked .bak file into the report:\n${plain.out}`); + } + } + + // 2. --json mode drives the separate onboardingState() path (what AGENTS.md + // has every agent run on session start) — its own, independent + // checkTrackedBakFiles(root) call had the same bug. + const jsonRun = runDoctor(dataRoot, ['--json']); + try { + const state = JSON.parse(jsonRun.out); + const warningText = (state.warnings || []).join('\n'); + if (!/stray\.md\.bak/.test(warningText)) { + pass("onboardingState()'s checkTrackedBakFiles() does not surface the data root's tracked .bak file either"); + } else { + fail(`onboardingState()'s checkTrackedBakFiles() leaked the data root's tracked .bak file: ${JSON.stringify(state.warnings)}`); + } + } catch { + fail(`doctor --json did not produce parseable JSON under a split CAREER_OPS_ROOT: ${jsonRun.out}\n${jsonRun.stderr || ''}`); + } +} finally { + rmSync(dataRoot, { recursive: true, force: true }); +}