From e34b763a0b9ecebad936f3e5b366be7ae7224d93 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 11:24:14 +0000 Subject: [PATCH] =?UTF-8?q?fix(cli):=20withhold=20doctor's=20=E2=9C=93=20o?= =?UTF-8?q?ver=20a=20tree=20it=20never=20examined=20(#10679)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `findMissingTests()` and `findDeprecatedUsages()` both walk `/packages/spec/src`, a path present in this monorepo and in no application. Both returned `[]` for "that directory is not here" — the same value they return for "I walked it and found nothing wrong" — so every stock scaffold printed `✓ Test coverage` and `✓ Deprecations` about files doctor never opened, exiting 0 either way. Whether the tree was examined is now a fact in the return type (`MonorepoTreeScan`), the shape #5413 used for the installed-package ledger, so the print site cannot reach the `✓` from the unexamined arm. An unexamined tree prints an informational skip naming the reason, with the resolved directory under `--verbose`. It is deliberately not a warning: withholding a false ✓ must not manufacture a false ⚠. The adjacent `⚠ @objectstack/spec Not built` probe is gated on the workspace existing — outside the monorepo it warned about an absent package and prescribed a command that cannot succeed there. Inside it, unchanged. Two control assertions that pinned `Environment is functional but has some warnings` held only because of that phantom warning; they now assert the claim they actually make (doctor reached its summary and did not call the environment broken). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019bmVFqoQPq63zhKrxdYG1r --- ...r-withholds-checks-over-unexamined-tree.md | 54 +++ .../commands/doctor-env-provenance.test.ts | 13 +- .../doctor-tenancy-posture-report.test.ts | 13 +- .../doctor-unexamined-spec-tree.test.ts | 365 ++++++++++++++++++ packages/cli/src/commands/doctor.ts | 183 +++++++-- 5 files changed, 589 insertions(+), 39 deletions(-) create mode 100644 .changeset/doctor-withholds-checks-over-unexamined-tree.md create mode 100644 packages/cli/src/commands/doctor-unexamined-spec-tree.test.ts diff --git a/.changeset/doctor-withholds-checks-over-unexamined-tree.md b/.changeset/doctor-withholds-checks-over-unexamined-tree.md new file mode 100644 index 0000000000..a774efc685 --- /dev/null +++ b/.changeset/doctor-withholds-checks-over-unexamined-tree.md @@ -0,0 +1,54 @@ +--- +"@objectstack/cli": patch +--- + +`os doctor` no longer prints `✓ Test coverage` / `✓ Deprecations` about a tree it +never examined, and no longer warns `@objectstack/spec Not built` about a +workspace that is not part of the tree (#10679). + +`findMissingTests()` and `findDeprecatedUsages()` both walk +`/packages/spec/src` — a path that exists in this monorepo and in no +application built with the framework. Both answered "that directory is not here" +with the same value they return for "I walked it and found nothing wrong" (an +empty array), so in a stock `create-objectstack -t blank` scaffold every run +printed, verbatim: + +``` + ✓ Test coverage All *.zod.ts files have matching tests + ✓ Deprecations No @deprecated tags found +``` + +about files doctor never opened. The command exits 0 either way, so "no problems +found" and "I never looked" were byte-identical to every downstream reader. + +Doctor already refuses to do this one screen down: the ADR-0120 D5e advisory's +`✓ Unique scope` is withheld unless `ledgerReadingIsComplete()` says the ledger +half was read in full. These two checks escaped that discipline; this restores +it, in the same shape #5413 used for the ledger — whether the tree was examined +is now a fact in the return type rather than an absence, so the print site +cannot reach the `✓` from the unexamined arm. Where the tree is absent doctor +prints an informational, named-reason skip instead: + +``` + ℹ Test coverage Skipped — no packages/spec/src in this directory (monorepo-only check) + ℹ Deprecations Skipped — no packages/spec/src in this directory (monorepo-only check) +``` + +`--verbose` adds the resolved directory it looked for. The skip is deliberately +not a warning: nothing is wrong in an application that has no +`packages/spec/src`, and withholding a false `✓` must not manufacture a false +`⚠`. + +The adjacent `⚠ @objectstack/spec Not built` probe read `/packages/spec/dist` +with no check that the workspace it names exists, so in an application it warned +about an absent package and prescribed `pnpm --filter @objectstack/spec build`, a +command that cannot succeed there. It is now gated on `packages/spec/package.json` +being present. Inside the monorepo the row is unchanged; outside it there is no +row, and an application's spec dependency stays covered by the `Dependencies` +check and by the spec-version-gap advisory. + +Exit codes are untouched — 1 exactly when an error row exists, warnings never +flip it. One visible consequence: a stock scaffold with no other findings now +ends on `✅ Environment is healthy and ready for development!` instead of +`⚠️ Environment is functional but has some warnings`, because the warning it +used to carry was about a workspace that was never there. diff --git a/packages/cli/src/commands/doctor-env-provenance.test.ts b/packages/cli/src/commands/doctor-env-provenance.test.ts index e59cdfe6e1..c44afaa9d1 100644 --- a/packages/cli/src/commands/doctor-env-provenance.test.ts +++ b/packages/cli/src/commands/doctor-env-provenance.test.ts @@ -447,7 +447,18 @@ describe('os doctor, end to end, against a posture that only exists in .env', () const healthy = await runDoctor(); expect(healthy.exitCode).toBeUndefined(); - expect(healthy.out).toContain('Environment is functional'); + // #10679 — this used to read `toContain('Environment is functional')`, and + // it passed for a reason that had nothing to do with #5387: the temp cwd + // has no `packages/spec`, and doctor warned `@objectstack/spec Not built` + // about that absent workspace on every run. Removing that phantom warning + // leaves this cwd with no findings at all, so the summary is now the + // healthy one. What the control actually claims — doctor reached its + // summary and did NOT refuse to call this environment usable — is what the + // matcher says instead, and it still cannot pass for the broken leg below + // (that one prints `Some critical issues found`). + expect(healthy.out).toMatch( + /Environment is (healthy and ready for development|functional but has some warnings)/, + ); expect(healthy.out).not.toContain('Tenancy posture'); // The report says what it read even when everything is fine — that is the // "not a silent merge" half, and it is only observable on a healthy run. diff --git a/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts b/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts index 348bcef7ab..2466fcf656 100644 --- a/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts +++ b/packages/cli/src/commands/doctor-tenancy-posture-report.test.ts @@ -295,8 +295,19 @@ describe('os doctor reports an unrecognized posture and exits non-zero', () => { // Doctor completes normally. This is the sentence #5382 quoted, and here it // is CORRECT: this environment really can start. + // + // #10679 — the matcher accepts either non-error summary. The control used + // to pin `Environment is functional but has some warnings` literally, and + // it held only because the temp cwd has no `packages/spec` and doctor + // warned `@objectstack/spec Not built` about that absent workspace every + // time. With that phantom warning gone this cwd has no findings, so the + // summary is the healthy one. Either sentence proves the control's actual + // claim; neither can be produced by the broken leg below, which prints + // `Some critical issues found` and exits 1. expect(healthy.exitCode).toBeUndefined(); - expect(healthy.out).toContain('Environment is functional'); + expect(healthy.out).toMatch( + /Environment is (healthy and ready for development|functional but has some warnings)/, + ); expect(healthy.out).not.toContain('Tenancy posture'); // ── The case: one character changed ────────────────────────────────── diff --git a/packages/cli/src/commands/doctor-unexamined-spec-tree.test.ts b/packages/cli/src/commands/doctor-unexamined-spec-tree.test.ts new file mode 100644 index 0000000000..661758f2de --- /dev/null +++ b/packages/cli/src/commands/doctor-unexamined-spec-tree.test.ts @@ -0,0 +1,365 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * `os doctor` does not print `✓ Test coverage` / `✓ Deprecations` about a tree + * it never examined (#10679). + * + * ── The defect ─────────────────────────────────────────────────────────── + * + * `findMissingTests()` and `findDeprecatedUsages()` both walk + * `/packages/spec/src`, a path that exists in this monorepo and in no + * application built with the framework. Both opened with: + * + * const specSrcDir = path.join(cwd, 'packages/spec/src'); + * if (!fs.existsSync(specSrcDir)) return []; + * + * so "that directory is not here" and "I walked it and found nothing wrong" + * arrived at the caller as the same value — an empty array — and the caller's + * `else` branch printed, in a stock `create-objectstack -t blank` scaffold: + * + * ✓ Test coverage All *.zod.ts files have matching tests + * ✓ Deprecations No @deprecated tags found + * + * about files doctor never opened. The command exits 0 either way, so "no + * problems found" and "I never looked" are byte-identical to every downstream + * reader — a CI step, a scripted preflight, an operator scanning the report. + * + * ── Why this is doctor's own rule, not a new one ───────────────────────── + * + * One screen down, the ADR-0120 D5e advisory already refuses to do this: its + * `✓ Unique scope` is printed only when `ledgerReadingIsComplete()` says the + * ledger half was read in full (#5412 / #5413 / #5644, pinned next door in + * `doctor-ledger-read-failure.test.ts`), because a `✓` over an unread half is + * a false PASS and a false PASS is the one thing that stops an operator + * looking further. These two checks escaped that discipline. Restoring it is + * what this file pins. + * + * The fix takes #5413's shape as well as its rule: whether the tree was + * examined is now a FACT IN THE TYPE (`MonorepoTreeScan`'s `scanned` arm) + * rather than an absence, so the print site cannot reach the `✓` from the + * unexamined arm even by accident. + * + * ── The adjacent row, same class (same card) ───────────────────────────── + * + * `⚠ @objectstack/spec Not built` probed `/packages/spec/dist` with no + * check that the workspace it names is part of the tree at all. In an + * application it warned about a package that is not there, flipped the run's + * summary to "functional but has some warnings", and prescribed + * `pnpm --filter @objectstack/spec build` — a command that cannot succeed + * where there is no such workspace. It is now gated on the workspace + * existing; inside the monorepo the warning is unchanged, and the last two + * describes below pin both halves of that. + * + * ── What this file deliberately does NOT do ────────────────────────────── + * + * It does not assert that the `✓` still appears when the tree WAS walked and + * call that a fix — that assertion passes against the defect. Every case + * below is anchored on the user-app cwd where the tree is absent, with the + * monorepo-shaped cwd present only as the no-regression half. + * + * Nor does it change any exit code. The skip is informational (`ℹ`), never a + * warning: nothing is wrong in an application that has no `packages/spec/src`, + * and withholding a false `✓` must not manufacture a false `⚠`. + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; + +import Doctor, { monorepoTreeSkipNotice } from './doctor.js'; + +/** `packages/cli` — the oclif root the real command is loaded against below. */ +const CLI_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), '..', '..'); + +/** + * `chalk` may or may not emit SGR codes depending on TTY detection. + * + * The escape is written as `\x1b`, never as the byte itself: one raw control + * character makes `grep` treat the whole file as binary, and a test file no + * `git grep` can find stops being maintained (#4890 / #5157). + */ +const SGR = /\x1b\[[0-9;]*m/g; +const plain = (s: string) => s.replace(SGR, ''); + +/** The two clean bills of health that must NOT appear over an unwalked tree. */ +const TEST_COVERAGE_CLAIM = 'All *.zod.ts files have matching tests'; +const DEPRECATIONS_CLAIM = 'No @deprecated tags found'; + +/** The head of the row that replaces each of them. */ +const SKIP_HEADLINE = 'Skipped — no packages/spec/src in this directory'; + +/** The monorepo-anchored build probe's warning and its unusable prescription. */ +const NOT_BUILT = 'Not built'; +const NOT_BUILT_FIX = 'pnpm --filter @objectstack/spec build'; + +describe('monorepoTreeSkipNotice — the line an unexamined tree prints instead of a ✓', () => { + const DIR = '/srv/app/packages/spec/src'; + + it('names the reason, not merely that something was skipped', () => { + const notice = monorepoTreeSkipNotice('Test coverage', DIR); + + // "Skipped" alone is the same silence in a different hat: the operator + // still cannot tell WHICH tree went unread or why. + expect(notice.message).toContain('Skipped'); + expect(notice.message).toContain('packages/spec/src'); + expect(notice.message).toContain('monorepo-only check'); + }); + + it('keeps the check’s own name in the column an operator scans', () => { + // Load-bearing, not cosmetic — the same reason the ledger rows kept a name + // column rather than vanishing (#5429). A row that disappears is + // indistinguishable from a check that was silently dropped. + const notice = monorepoTreeSkipNotice('Test coverage', DIR); + + expect(notice.message.startsWith('Test coverage')).toBe(true); + // 22-column name field, matching the sibling `✓` lines of this report. + expect(notice.message.indexOf('Skipped')).toBe(22); + }); + + it('names the directory doctor actually resolved, in the verbose detail', () => { + // A relative literal restated in prose is a claim the reader has to trust. + // The resolved path is one they can check. + const notice = monorepoTreeSkipNotice('Deprecations', DIR); + + expect(notice.detail).toContain(DIR); + }); + + it('never carries either clean bill of health', () => { + for (const name of ['Test coverage', 'Deprecations']) { + const notice = monorepoTreeSkipNotice(name, DIR); + expect(notice.message).not.toContain(TEST_COVERAGE_CLAIM); + expect(notice.message).not.toContain(DEPRECATIONS_CLAIM); + expect(notice.detail).not.toContain(TEST_COVERAGE_CLAIM); + expect(notice.detail).not.toContain(DEPRECATIONS_CLAIM); + } + }); +}); + +/** + * `node_modules/` exists in every temp cwd below on purpose — without it + * doctor's `Dependencies` check is itself an `error` and exits 1 on its own, + * which would make an assertion pass for a reason having nothing to do with + * this change (the trap PR #5390 wrote down, inherited via #5402 / #5410). + */ +function makeTempCwd(tag: string): string { + const dir = fs.mkdtempSync(path.join(os.tmpdir(), `os-doctor-10679-${tag}-`)); + fs.mkdirSync(path.join(dir, 'node_modules')); + return dir; +} + +describe('os doctor, end to end, in a user-app cwd with no packages/spec', () => { + let tmp: string; + let cwdSpy: ReturnType; + + beforeEach(() => { + tmp = makeTempCwd('userapp'); + cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); + }); + + afterEach(() => { + cwdSpy.mockRestore(); + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + async function runDoctor(argv: string[] = []): Promise<{ out: string; exitCode: number | undefined }> { + const logs: string[] = []; + const logSpy = vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { + logs.push(a.join(' ')); + }); + let exitCode: number | undefined; + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`__PROCESS_EXIT__:${code}`); + }) as never); + + try { + await Doctor.run(argv, { root: CLI_ROOT }); + } catch (err) { + if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; + } finally { + logSpy.mockRestore(); + exitSpy.mockRestore(); + } + return { out: plain(logs.join('\n')), exitCode }; + } + + // A real `Doctor.run()` takes seconds (it shells out to `git --version`, + // walks the workspace and reads the ledger), so every case here carries an + // explicit timeout instead of racing vitest's 5s default. + const E2E_TIMEOUT = 60_000; + + it('withholds BOTH clean bills of health, and says why instead', async () => { + const run = await runDoctor(); + + // ① THE assertion of this issue: the two false PASSes are gone. + expect(run.out).not.toContain(TEST_COVERAGE_CLAIM); + expect(run.out).not.toContain(DEPRECATIONS_CLAIM); + + // ② …replaced by rows that name what was not examined and why. Present, + // not merely absent — see the name-column case above. + expect(run.out).toContain('Test coverage'); + expect(run.out).toContain('Deprecations'); + const skipRows = run.out.split('\n').filter((line) => line.includes(SKIP_HEADLINE)); + expect(skipRows).toHaveLength(2); + }, E2E_TIMEOUT); + + it('does not announce a scan that never started', async () => { + // The step lines pair with a result. "Checking for missing test files..." + // followed by a skip row is the same overclaim one line earlier. + const run = await runDoctor(); + + expect(run.out).not.toContain('Checking for missing test files'); + expect(run.out).not.toContain('Scanning for @deprecated usage'); + }, E2E_TIMEOUT); + + it('marks the skip as informational — a withheld ✓ must not become a ⚠', async () => { + const run = await runDoctor(); + + for (const name of ['Test coverage', 'Deprecations']) { + const row = run.out.split('\n').find((line) => line.includes(name)); + expect(row, `no row named ${name}`).toBeDefined(); + // The glyph IS the verdict for a reader scanning the column. + expect(row).toContain('ℹ'); + expect(row).not.toContain('✓'); + expect(row).not.toContain('⚠'); + } + }, E2E_TIMEOUT); + + it('names the directory it did not walk, and only when asked', async () => { + const unwalked = path.join(tmp, 'packages', 'spec', 'src'); + + // #5403's rule: a detail is optional reading until the operator asks. + const quiet = await runDoctor(); + expect(quiet.out).not.toContain(unwalked); + + const verbose = await runDoctor(['--verbose']); + expect(verbose.out).toContain(unwalked); + }, E2E_TIMEOUT); + + it('drops the monorepo-anchored `@objectstack/spec Not built` warning entirely', async () => { + // Same class, same card: a verdict about a workspace that is not part of + // this tree — prescribing a command that cannot succeed here. + const quiet = await runDoctor(); + const verbose = await runDoctor(['--verbose']); + + expect(quiet.out).not.toContain(NOT_BUILT); + expect(quiet.out).not.toContain(NOT_BUILT_FIX); + expect(verbose.out).not.toContain(NOT_BUILT); + expect(verbose.out).not.toContain(NOT_BUILT_FIX); + }, E2E_TIMEOUT); + + it('changes nothing about the exit contract — still 0, still no error rows', async () => { + // The fence on this card: withholding a ✓ is a reporting change, not a + // severity change. `process.exit` is never reached. + const run = await runDoctor(); + + expect(run.exitCode).toBeUndefined(); + expect(run.out).not.toContain('Some critical issues found'); + }, E2E_TIMEOUT); +}); + +describe('os doctor, end to end, in a monorepo-shaped cwd — the no-regression half', () => { + let tmp: string; + let cwdSpy: ReturnType; + let specSrc: string; + + beforeEach(() => { + tmp = makeTempCwd('monorepo'); + specSrc = path.join(tmp, 'packages', 'spec', 'src'); + fs.mkdirSync(specSrc, { recursive: true }); + cwdSpy = vi.spyOn(process, 'cwd').mockReturnValue(tmp); + }); + + afterEach(() => { + cwdSpy.mockRestore(); + fs.rmSync(tmp, { recursive: true, force: true }); + }); + + async function runDoctor(argv: string[] = []): Promise<{ out: string; exitCode: number | undefined }> { + const logs: string[] = []; + const logSpy = vi.spyOn(console, 'log').mockImplementation((...a: unknown[]) => { + logs.push(a.join(' ')); + }); + let exitCode: number | undefined; + const exitSpy = vi.spyOn(process, 'exit').mockImplementation(((code?: number) => { + exitCode = code; + throw new Error(`__PROCESS_EXIT__:${code}`); + }) as never); + + try { + await Doctor.run(argv, { root: CLI_ROOT }); + } catch (err) { + if (!(err instanceof Error) || !err.message.startsWith('__PROCESS_EXIT__')) throw err; + } finally { + logSpy.mockRestore(); + exitSpy.mockRestore(); + } + return { out: plain(logs.join('\n')), exitCode }; + } + + const E2E_TIMEOUT = 60_000; + + it('prints both ✓ lines when the tree really was walked and is clean', async () => { + fs.writeFileSync(path.join(specSrc, 'account.zod.ts'), 'export const A = 1;\n'); + fs.writeFileSync(path.join(specSrc, 'account.test.ts'), 'export const T = 1;\n'); + + const run = await runDoctor(); + + expect(run.out).toContain(TEST_COVERAGE_CLAIM); + expect(run.out).toContain(DEPRECATIONS_CLAIM); + // The skip is unreachable from this arm. + expect(run.out).not.toContain('Skipped'); + // And the step lines are back, because a scan really did start. + expect(run.out).toContain('Checking for missing test files'); + expect(run.out).toContain('Scanning for @deprecated usage'); + }, E2E_TIMEOUT); + + it('still reports the real findings when the walked tree is not clean', async () => { + // No sibling `.test.ts`, and a `@deprecated` tag in the file itself: one + // finding for each check, from a tree that genuinely was examined. + fs.writeFileSync( + path.join(specSrc, 'invoice.zod.ts'), + '/** @deprecated use billing */\nexport const I = 1;\n', + ); + + const run = await runDoctor(); + + expect(run.out).toContain('Missing test: invoice.test.ts'); + expect(run.out).toContain('@deprecated tag found'); + expect(run.out).not.toContain(TEST_COVERAGE_CLAIM); + expect(run.out).not.toContain(DEPRECATIONS_CLAIM); + expect(run.out).not.toContain('Skipped'); + // Warnings never flip the exit code. + expect(run.exitCode).toBeUndefined(); + }, E2E_TIMEOUT); + + it('keeps ⚠ Not built where the spec workspace really exists and is unbuilt', async () => { + fs.writeFileSync( + path.join(tmp, 'packages', 'spec', 'package.json'), + JSON.stringify({ name: '@objectstack/spec', version: '1.0.0' }), + ); + + const run = await runDoctor(['--verbose']); + + // Gating this probe on the workspace must not silence it where the + // workspace is real — that would be trading a false warning for no warning. + expect(run.out).toContain(NOT_BUILT); + expect(run.out).toContain(NOT_BUILT_FIX); + }, E2E_TIMEOUT); + + it('prints ✓ Built once that workspace has a dist/', async () => { + fs.writeFileSync( + path.join(tmp, 'packages', 'spec', 'package.json'), + JSON.stringify({ name: '@objectstack/spec', version: '1.0.0' }), + ); + fs.mkdirSync(path.join(tmp, 'packages', 'spec', 'dist')); + + const run = await runDoctor(); + + expect(run.out).toContain('@objectstack/spec'); + expect(run.out).not.toContain(NOT_BUILT); + expect(run.out).toMatch(/@objectstack\/spec\s+Built/); + }, E2E_TIMEOUT); +}); diff --git a/packages/cli/src/commands/doctor.ts b/packages/cli/src/commands/doctor.ts index 5b6405337b..f16540ff48 100644 --- a/packages/cli/src/commands/doctor.ts +++ b/packages/cli/src/commands/doctor.ts @@ -1138,9 +1138,85 @@ function walkDir(dir: string, ext: string): string[] { return results; } -function findMissingTests(cwd: string): string[] { +// ─── Monorepo-anchored scans (#10679) ─────────────────────────────── +// +// The two checks below walk `/packages/spec/src` — a path that exists in +// THIS monorepo and in no application built with the framework. Both used to +// answer "that directory is not here" with `[]`, the same value they return +// for "I walked it and found nothing wrong", and the caller printed a `✓` over +// the second reading of an empty list. In a stock `create-objectstack` +// scaffold that produced, verbatim: +// +// ✓ Test coverage All *.zod.ts files have matching tests +// ✓ Deprecations No @deprecated tags found +// +// about files doctor never opened. Doctor already refuses to do this one +// screen down: the D5e advisory's `✓` is withheld unless +// `ledgerReadingIsComplete()` says the ledger half was read in full, because +// "no problems found" and "I never looked" are byte-identical to a reader — +// and the run exits 0 either way, so nothing downstream can tell them apart. +// A green check over unread state is worse than a crash; a crash reports +// itself. +// +// The fix is the same one #5413 applied to the ledger, in the same shape: +// whether the tree was examined becomes a FACT IN THE TYPE rather than an +// absence, so the print site cannot help but branch on it. + +/** + * What a walk of a monorepo-internal tree actually examined. + * + * `scanned: false` is the entire reason this type exists — it is the state a + * bare `string[]` could not express. `dir` is carried on BOTH arms so the skip + * notice can name the directory doctor resolved and looked for, rather than + * restating a relative literal the reader has to trust. + */ +type MonorepoTreeScan = + | { scanned: false; dir: string } + | { scanned: true; dir: string; findings: string[] }; + +/** + * The line an unexamined tree prints INSTEAD of a `✓`, and its verbose detail. + * + * Triage named two admissible shapes for this defect — skip with a named + * reason, or scope the line to what was actually walked — and this is the + * first. Keeping the check's own name in the left column matters: an operator + * scans the report by that column, so the row has to be PRESENT and say it did + * not run, not quietly disappear (the `Installed packages` rows were moved for + * the same reason). + * + * It is deliberately NOT a warning. Nothing is wrong in an application that + * has no `packages/spec/src` — this check simply has no subject there — and + * routing it through `printWarning` would flip `hasWarnings` and end every + * healthy user app's report on "Environment is functional but has some + * warnings". Withholding a false `✓` must not manufacture a false `⚠`. + */ +export function monorepoTreeSkipNotice( + name: string, + dir: string, +): { message: string; detail: string } { + return { + // 21 + the joining space = the 22-column name field the sibling `✓` lines + // in this report use, so a skipped row lines up with the ones that ran. + message: `${name.padEnd(21)} Skipped — no packages/spec/src in this directory (monorepo-only check)`, + detail: + `Looked for ${dir} and found nothing to walk. This check reads the monorepo's own ` + + 'spec sources, so outside that checkout it has no subject — and a ✓ here would be a ' + + 'claim about files doctor never opened.', + }; +} + +/** Print a skipped monorepo-anchored check, with its reason under `--verbose`. */ +function printMonorepoTreeSkip(name: string, dir: string, verbose: boolean): void { + const notice = monorepoTreeSkipNotice(name, dir); + printInfo(notice.message); + if (verbose) { + console.log(chalk.dim(` → ${notice.detail}`)); + } +} + +function findMissingTests(cwd: string): MonorepoTreeScan { const specSrcDir = path.join(cwd, 'packages/spec/src'); - if (!fs.existsSync(specSrcDir)) return []; + if (!fs.existsSync(specSrcDir)) return { scanned: false, dir: specSrcDir }; const missing: string[] = []; const zodFiles = walkDir(specSrcDir, '.zod.ts'); @@ -1153,12 +1229,12 @@ function findMissingTests(cwd: string): string[] { missing.push(`Missing test: ${relTest} (for ${relZod})`); } } - return missing; + return { scanned: true, dir: specSrcDir, findings: missing }; } -function findDeprecatedUsages(cwd: string): string[] { +function findDeprecatedUsages(cwd: string): MonorepoTreeScan { const specSrcDir = path.join(cwd, 'packages/spec/src'); - if (!fs.existsSync(specSrcDir)) return []; + if (!fs.existsSync(specSrcDir)) return { scanned: false, dir: specSrcDir }; const deprecated: string[] = []; const tsFiles = walkDir(specSrcDir, '.ts') @@ -1178,7 +1254,7 @@ function findDeprecatedUsages(cwd: string): string[] { // Skip unreadable files } } - return deprecated; + return { scanned: true, dir: specSrcDir, findings: deprecated }; } // ─── Deprecated Pattern Detection ─────────────────────────────────── @@ -1833,22 +1909,42 @@ export default class Doctor extends Command { }); } - // Check if spec package is built - const specDistPath = path.join(cwd, 'packages/spec/dist'); - - if (fs.existsSync(specDistPath)) { - results.push({ - name: '@objectstack/spec', - status: 'ok', - message: 'Built', - }); - } else { - results.push({ - name: '@objectstack/spec', - status: 'warning', - message: 'Not built', - fix: 'Run: pnpm --filter @objectstack/spec build', - }); + // Check if the monorepo's spec WORKSPACE is built. + // + // #10679 — gated on that workspace existing, which is the same defect + // class as the two `✓`s further down and the reason this probe is on the + // same card. `packages/spec` is a directory in this repo and in no + // application: outside the checkout the probe had no subject, yet it + // reported `⚠ @objectstack/spec Not built` about it anyway — a warning + // that flipped every stock scaffold's report to "functional but has some + // warnings", prescribing `pnpm --filter @objectstack/spec build`, a + // command that cannot succeed where there is no such workspace. + // + // Here the honest report is no row at all. An application consumes + // `@objectstack/spec` from `node_modules`, where "built" is not a state it + // can be in — that dependency is covered by the `Dependencies` row above + // and by `checkSpecVersionGap()`. Inside the monorepo nothing changes: the + // workspace is present, and an unbuilt `dist/` is still the real warning + // it always was. + const specWorkspaceDir = path.join(cwd, 'packages/spec'); + + if (fs.existsSync(path.join(specWorkspaceDir, 'package.json'))) { + const specDistPath = path.join(specWorkspaceDir, 'dist'); + + if (fs.existsSync(specDistPath)) { + results.push({ + name: '@objectstack/spec', + status: 'ok', + message: 'Built', + }); + } else { + results.push({ + name: '@objectstack/spec', + status: 'warning', + message: 'Not built', + fix: 'Run: pnpm --filter @objectstack/spec build', + }); + } } // Check Git @@ -1928,27 +2024,40 @@ export default class Doctor extends Command { // ── Extended Checks ────────────────────────────────────────────── // Missing test files - printStep('Checking for missing test files...'); + // + // #10679 — the scan reports whether it ran, and the `✓` is reachable only + // from the arm where it did. The step line moved inside that arm too: + // "Checking for missing test files..." followed by a skip row announces + // work that never started. const missingTests = findMissingTests(cwd); - if (missingTests.length > 0) { - hasWarnings = true; - for (const msg of missingTests) { - printWarning(msg); - } + if (!missingTests.scanned) { + printMonorepoTreeSkip('Test coverage', missingTests.dir, flags.verbose); } else { - printSuccess('Test coverage All *.zod.ts files have matching tests'); + printStep('Checking for missing test files...'); + if (missingTests.findings.length > 0) { + hasWarnings = true; + for (const msg of missingTests.findings) { + printWarning(msg); + } + } else { + printSuccess('Test coverage All *.zod.ts files have matching tests'); + } } - // Deprecated usage detection - printStep('Scanning for @deprecated usage...'); + // Deprecated usage detection (#10679 — same shape as above) const deprecatedUsages = findDeprecatedUsages(cwd); - if (deprecatedUsages.length > 0) { - hasWarnings = true; - for (const msg of deprecatedUsages) { - printWarning(`Deprecated: ${msg}`); - } + if (!deprecatedUsages.scanned) { + printMonorepoTreeSkip('Deprecations', deprecatedUsages.dir, flags.verbose); } else { - printSuccess('Deprecations No @deprecated tags found'); + printStep('Scanning for @deprecated usage...'); + if (deprecatedUsages.findings.length > 0) { + hasWarnings = true; + for (const msg of deprecatedUsages.findings) { + printWarning(`Deprecated: ${msg}`); + } + } else { + printSuccess('Deprecations No @deprecated tags found'); + } } // Config-aware checks (only if config exists)