diff --git a/.changeset/agile-rams-run.md b/.changeset/agile-rams-run.md new file mode 100644 index 000000000..a4e327713 --- /dev/null +++ b/.changeset/agile-rams-run.md @@ -0,0 +1,5 @@ +--- +type: Changed +pr: 3199 +--- +**Live-plan counting now has one owner, so `superseded` plans stop being scheduled and nested-layout phases stop reporting zero** — `scanPhasePlans` is the sole source of which plans exist and which are outstanding. Twenty-one call sites that re-derived it from filenames now route through it, so a plan marked `status: superseded` is no longer scheduled into an execute-phase wave, phases using the nested `plans/` layout no longer report zero plans, and stray summaries no longer inflate completion. (#3183) diff --git a/.gitignore b/.gitignore index d02c33c4b..5918e6829 100644 --- a/.gitignore +++ b/.gitignore @@ -192,6 +192,7 @@ build/ /gsd-core/bin/lib/worktree-base-ref.cjs /gsd-core/bin/lib/worktree-safety.cjs /gsd-core/bin/lib/planning-workspace.cjs +/gsd-core/bin/lib/planning-scope.cjs /gsd-core/bin/lib/command-roster.cjs /gsd-core/bin/lib/runtime-artifact-conversion.cjs /gsd-core/bin/lib/runtime-artifact-layout.cjs diff --git a/CONTEXT.md b/CONTEXT.md index d9abc3d6d..2dae55b02 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -100,6 +100,9 @@ Module policy that defines query-time behavior when `.planning/config.json` is a ### Configuration Module Module owning legacy-key normalization, defaults merge, and explicit on-disk migration for `.planning/config.json`. Interface: `normalizeLegacyKeys(parsed) → { parsed, normalizations[] }` (idempotent, pure, returns the list of normalizations applied), `mergeDefaults(parsed) → MergedConfig` (deep-merge of parsed config over canonical defaults), `migrateOnDisk(cwd) → MigrationReport` (explicit, opt-in, called by the installer and by `gsd-tools migrate-config`). Invariants: legacy top-level keys (`branching_strategy`, `sub_repos`, `multiRepo`, `depth`) are normalized into their canonical nested locations in the returned value; defaults come from the shared `gsd-core/bin/shared/config-defaults.manifest.json`; schema (`VALID_CONFIG_KEYS`, `RUNTIME_STATE_KEYS`, `DYNAMIC_KEY_PATTERNS`) comes from `gsd-core/bin/shared/config-schema.manifest.json`. Note: `loadConfig` (project config read + merge) was extracted to the Config Loader Module (`config-loader.cjs`) per ADR-857 phase 2e (#885); `configuration.cjs` now provides only the pure normalization and defaults primitives that `config-loader.cjs` depends on. Source of truth: `gsd-core/bin/lib/configuration.cjs`, consumed via `bin/lib/config-loader.cjs` and `bin/lib/config-schema.cjs`. Eliminates the recurring #3523-class drift bug structurally. +### Planning Scope Module +Leaf module owning the frozen `SCOPE` discriminator (`COMPLETE` / `TRUNCATED` / `UNSCOPED` / `UNREADABLE`) that every consolidated `.planning/` semantic derivation returns alongside its payload, per ADR-3180 Decision 2. It exists to make one distinction representable: `COMPLETE` with zero items is a REAL answer (a phase genuinely has no plans; a milestone genuinely has no phases yet), while the other three with zero items are NON-answers — the derivation could not see all of its input. Before it, those two cases were output-identical, which is the failure class epic #3180 removes: a truncated milestone window returned `phase_count: 0` with no error, indistinguishable from a freshly-declared milestone. It is a frozen enum rather than a message string because `CONTRIBUTING.md` bans raw-text matching on outputs and requires a typed IR, so callers branch on `result.scope === SCOPE.TRUNCATED`. Pure and import-free — the bottom of the dependency graph, so any consumer can depend on it without a cycle (mirrors the Phase Id Module's leaf position). Source of truth: `gsd-core/bin/lib/planning-scope.cjs` (generated from `src/planning-scope.cts`). The contract is PROVISIONAL: #3183 is its first real implementation, and ADR-3180 requires the ADR be amended before Phase 2 rather than the contract worked around, if it does not fit. + ### Planning Workspace Module Module owning `.planning` path resolution, active workstream pointer policy (`session-scoped > shared`), pointer self-heal behavior, and planning lock semantics for workstream-aware execution. @@ -181,7 +184,7 @@ Shared fail-loud `Result` and per-surface write-set contracts (`gsd-core/bin/ Module owning ROADMAP.md parsing: shipped-milestone slicing, current-milestone extraction, milestone/phase lookups, and milestone-phase filtering (`stripShippedMilestones`, `extractCurrentMilestone`, `replaceInCurrentMilestone`, `getRoadmapPhaseInternal`, `getMilestoneInfo`, `getMilestonePhaseFilter`, `isMilestoneShippedInRoadmap`, `withPhaseSection`). Milestone shipped/active heading classification is owned here (#2562): `isMilestoneShippedInRoadmap(content, version)` answers "does the ROADMAP mark THIS milestone shipped" from heading and `` lines only — never a bullet that merely names the version — with the version token boundary-matched so `v2.0` does not match inside `v2.0.1`. `extractCurrentMilestone` and `getMilestonePhaseFilter` take an optional trailing workstream name so their `planningDir` resolution targets `.planning/workstreams//`; omitted, it resolves exactly as before (including the `GSD_WORKSTREAM` fallback). `getMilestonePhaseFilter` exposes `versionScoped`, true only when the returned phase set really is one milestone's — consumers must not read `phaseCount` as a current-milestone denominator otherwise — and `versionSectionFound`, true whenever the requested version's section was located at all. The two differ precisely for a located-but-EMPTY section: it falls through to the zero-count pass-all degrade, which resets `versionScoped` to false, leaving `versionSectionFound` the only surviving evidence that the milestone exists rather than being absent. `missingExplicitVersion` covers the complementary shape (versioned roadmap, no section for this version). `withPhaseSection(content, phaseId, edit)` resolves a phase's `### Phase N` detail-section heading via the #2121 phase-id source (`phaseMarkdownRegexSource`) and delegates to the markdown-sectionizer seam's `withSection`, so a per-phase ROADMAP edit is bounded to that phase's own section (ADR-2143 §4). Depends only on leaf modules (`phase-id`, `planning-workspace`, `shell-command-projection`, `markdown-sectionizer`, and — since #1881 — `unusable-input` for the out-of-band diagnostic) — no `loadConfig`, no other core dependency. An unreadable ROADMAP.md is reported rather than collapsed into the same sentinel as a genuinely absent one; absence itself stays silent, and neither lookup gains a throw (the #2245 audit records that `src/state.cts` removed its defensive try/catch on the strength of `getMilestoneInfo` never throwing). Extracted from the Core module per ADR-857 rollout phase 2b (#870), resolving the ROADMAP.md parse/write straddle so the Roadmap module (`roadmap.cjs`, which owns ROADMAP.md mutation) imports parsing directly instead of through Core; the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/roadmap-parser.cjs` (generated from `src/roadmap-parser.cts`). ### Core Utilities Module -Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `filterPlanFiles`, `filterSummaryFiles`, `extractCanonicalPlanId`, `timeAgo`). Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). +Module owning the shared low-level utility primitives extracted from Core: POSIX path normalization (`toPosixPath`), filesystem scanning (`detectSubRepos`, `readSubdirectories`, `getPhaseFileStats`, `pathExistsInternal`), plan/summary pairing helpers (`countMatchedSummaries`, `findUnsummarizedPlans`, `findOrphanSummaries`), and small pure helpers (`generateSlugInternal`, `extractOneLinerFromBody`, `extractCanonicalPlanId`, `timeAgo`). `filterPlanFiles`/`filterSummaryFiles` were retired by #3183 (ADR-3180 Decision 2) — `getPhaseFileStats` no longer re-derives plan/summary filename matching locally; it now sources `plans`/`summaries` (plus a `scope` field, `COMPLETE`/`TRUNCATED`/`UNREADABLE`) directly from `scanPhasePlans` (`src/plan-scan.cts`), the single owner of live-plan counting. Depends only on Node built-ins and already-leafed modules (`phase-id` for `comparePhaseNum`, `planning-workspace` for `findContextMdIn`) — no `loadConfig`, no other core dependency. Extracted from the Core module per ADR-857 rollout phase 2c (#877) as the shared leaf that unblocks the phase-locator fs-search extraction (2d); the `core.cjs` re-export spine was retired in epic #1267, so callers import this leaf directly. Source of truth: `gsd-core/bin/lib/core-utils.cjs` (generated from `src/core-utils.cts`). ### Agent Install Check Module Module owning agent-presence resolution and verification, extracted from the Core module as the cleanup step that retired the `core.cjs` re-export spine (the final ADR-857 decomposition, epic #1267). Interface: `getAgentsDir(runtime?, projectRoot?)` — env-var-aware, runtime-aware agents-directory resolution; Claude resolves `__dirname`-relative, while other runtimes prefer a manifest-backed project-local agents directory before their global configuration home. The manifest gate is intentional: runtime-native project agents must not shadow a working global GSD install. `checkAgentsInstalled(...)` validates `gsd-file-manifest.json` completeness and confirms the declared agents exist on disk. Pure read/verify — no install-write side effects (writes remain the Installer Module's). Consumed by the Init Command Module, the verify workflow, and the docs workflow. Source of truth: `gsd-core/bin/lib/agent-install-check.cjs` (generated from `src/agent-install-check.cts`); replaced the two functions that squatted in `core.cts`. See Installer Module and ADR-857. diff --git a/docs/INVENTORY-MANIFEST.json b/docs/INVENTORY-MANIFEST.json index 05f07a741..f01eceb31 100644 --- a/docs/INVENTORY-MANIFEST.json +++ b/docs/INVENTORY-MANIFEST.json @@ -416,6 +416,7 @@ "plan-dependency-graph.cjs", "plan-drift-guard.cjs", "plan-scan.cjs", + "planning-scope.cjs", "planning-workspace.cjs", "probe-core.cjs", "profile-output.cjs", diff --git a/docs/INVENTORY.md b/docs/INVENTORY.md index 669bbfd93..e51d56e59 100644 --- a/docs/INVENTORY.md +++ b/docs/INVENTORY.md @@ -523,6 +523,7 @@ Full listing: `gsd-core/bin/lib/*.cjs`. | `phases-command-router.cjs` | Thin CJS subcommand router adapter for `gsd-tools phases` | | `plan-dependency-graph.cjs` | Shared halt-propagation over a plan's `depends_on` DAG — the single topological-order + halt-propagation engine used by both `phase.cjs`'s wave-grouping and `phase-locator.cjs`'s phase-location primitive, so the two can never diverge on which plans a halted plan blocks (#2830) | | `plan-scan.cjs` | Canonical phase-plan scanner for detecting plan and summary files in flat and nested layouts (k014) | +| `planning-scope.cjs` | Frozen `SCOPE` discriminator (`COMPLETE`/`TRUNCATED`/`UNSCOPED`/`UNREADABLE`) distinguishing a genuinely-empty derivation from one computed over a truncated or unscoped input, so callers can branch on the difference instead of reading a plausible zero (ADR-3180) | | `planning-workspace.cjs` | Planning path/workstream seam (`planningDir`, `planningPaths`, active-workstream routing, `.planning/.lock` orchestration) | | `project-root.cjs` | Resolves a project root from a starting directory using four heuristics (own `.planning/` guard, `sub_repos` config, `multiRepo` flag, `.git` heuristic) | | `profile-output.cjs` | Profile rendering, USER-PROFILE.md and dev-preferences.md generation | diff --git a/docs/adr/3180-planning-semantic-model-single-owner.md b/docs/adr/3180-planning-semantic-model-single-owner.md index 831af044d..f7f3a38cd 100644 --- a/docs/adr/3180-planning-semantic-model-single-owner.md +++ b/docs/adr/3180-planning-semantic-model-single-owner.md @@ -244,4 +244,54 @@ Considered and not applicable: `choose-boring-technology` (no new dependency; fi ## Amendments -None yet. Decision 2's contract is provisional; any amendment arising from Phase 1's validation is recorded here before Phase 2 begins. +### Amendment 1 — Phase 1 (#3183) validation: the boundary between Phases 1 and 3 was mis-cut + +Decision 2 marked the contract provisional and required amendment before Phase 2 rather than a +workaround in code. Phase 1 exercised it and the contract itself **held** — `SCOPE` needed no +change. What did not hold was the **phase boundary**. + +**What Phase 1 found.** Building the Decision 4(a) whole-repo guard — the one that may not use a +file allowlist — turned up **26 live-plan re-derivations across 9 files**. The epic scoped this +derivation at **3 copies**. Per-site triage classified them 21 true re-derivations, 2 asking a +genuinely different question, 2 dead. + +**Why the boundary was wrong.** `commands.cts`'s `cmdProgressRender` re-derives *both* enumeration +(assigned to Phase 3) *and* plan counting (Phase 1), on adjacent lines. So DW4's "no caller +re-derives it from filenames" was **unsatisfiable within Phase 1's original file scope** — Phase 1 +would have shipped failing its own acceptance criterion while Phase 3 inherited half a derivation. + +**Amended scope (maintainer decision).** Phase 1 owns **every** live-plan-counting re-derivation +repo-wide. **Phase 3 narrows** to milestone-window + sentinel-filter enumeration only; its files are +already plan-count-clean when it starts, and its own drift guard inherits a green baseline. + +**Two consequential changes to Decision 1's owner surface:** + +1. **`scanPhasePlans` gains `allPlanFiles`** (every plan on disk, *pre*-supersession) alongside + `planFiles` (the live set). `verify.cts` conflated two questions in one loop — numbering-gap + detection legitimately wants every file on disk, pairing wants the live set. The fix is for the + owner to answer both explicitly, not to exempt the caller. Single ownership is preserved; the + owner simply stopped under-serving. Additive — no existing field changed. +2. **`findOrphanSummaries` joins `findUnsummarizedPlans`** in core-utils, sharing the same + `summaryCandidates` rule. `verify.cts` needed the inverse question (summaries with no plan) and + had no canonical primitive, so it had hand-rolled one — a third pairing rule of exactly the kind + Decision 1 exists to prevent. + +**Exemptions are by documented reason, never by file allowlist** (Decision 4(a)). Two sites are +exempt, each carrying an inline comment stating the question it actually asks: `audit.cts` +`scanQuickTasks` checks one quick task's own directory for a single completion record, and +`gsd2-import.cts` `readTasksDir` reads a foreign GSD-2 `tasks/` layout during a one-time import. +Neither is a `.planning/` phase directory. + +**Decision 3's Tier-2 table is re-derived for Phase 1**, per its own contingency clause. Beyond the +superseded-plan change, the migration also corrects: phases on the post-#3139 **nested `plans/` +layout** (previously counted as zero by every migrated site), **loosely-named plan files**, and +**stray summaries** that inflated completion. The sharpest is `phase.cts`'s `cmdPhasePlanIndex`, +which feeds execute-phase **wave scheduling** — it was scheduling `status: superseded` plans into +waves and reporting zero plans for nested-layout phases. + +**Regression caught during migration, recorded because it is a trap for Phases 2–5:** passing the +superseded-filtered `planFiles` into `describeNonCanonicalPlans` made a superseded-but-correctly-named +plan report as a naming violation — the diagnostic reads non-membership as a defect. It takes +`allPlanFiles`. The general rule: a **diagnostic about file naming** wants the physical set; only a +question about outstanding *work* wants the live set. Later phases must make that choice explicitly +per call site rather than swapping in `planFiles` mechanically. diff --git a/eslint.config.mjs b/eslint.config.mjs index 1db9ea096..232e6f3cd 100644 --- a/eslint.config.mjs +++ b/eslint.config.mjs @@ -160,6 +160,7 @@ export default tseslint.config( 'gsd-core/bin/lib/worktree-safety.cjs', 'gsd-core/bin/lib/worktree-base-ref.cjs', 'gsd-core/bin/lib/planning-workspace.cjs', + 'gsd-core/bin/lib/planning-scope.cjs', 'gsd-core/bin/lib/command-roster.cjs', 'gsd-core/bin/lib/runtime-artifact-conversion.cjs', 'gsd-core/bin/lib/runtime-artifact-install-plan.cjs', diff --git a/package.json b/package.json index acfbf2356..88d2694b7 100644 --- a/package.json +++ b/package.json @@ -108,7 +108,7 @@ "lint": "eslint . --cache --cache-location node_modules/.cache/eslint/", "lint:fix": "eslint . --fix", "lint:table-schema-drift": "node scripts/lint-table-schema-drift.cjs", - "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs", + "lint:ci": "npm run lint && npm run lint:skill-deps && npm run lint:generated-sync && node scripts/lint-test-file-count.cjs && node scripts/lint-command-contract.cjs && node scripts/lint-pr-check-project-dir.cjs && npm run lint:legacy-name && node scripts/lint-regression-test-names.cjs && node scripts/lint-allow-test-rule-refs.cjs && node scripts/lint-resolution-provenance.cjs && node scripts/lint-emitted-drift-ack.cjs && node scripts/lint-portable-timeout.cjs && node scripts/validate-registry.cjs && node scripts/lint-table-schema-drift.cjs && node scripts/lint-fix-has-regression-test.cjs && node scripts/lint-example-parser-parity.cjs && node scripts/lint-docs-command-form.cjs && node scripts/lint-plan-count-drift.cjs", "lint:allow-test-rule-refs": "node scripts/lint-allow-test-rule-refs.cjs", "lint:regression-names": "node scripts/lint-regression-test-names.cjs", "lint:descriptions": "node scripts/lint-descriptions.cjs", diff --git a/scripts/lint-plan-count-drift.cjs b/scripts/lint-plan-count-drift.cjs new file mode 100644 index 000000000..6314ef04e --- /dev/null +++ b/scripts/lint-plan-count-drift.cjs @@ -0,0 +1,499 @@ +#!/usr/bin/env node +'use strict'; + +/** + * Anti-divergence drift guard for the live-plan-counting seam + * (epic #3180, issue #3183, ADR-3180 "Planning Semantic Model Single Owner"). + * + * `src/plan-scan.cts`'s `scanPhasePlans` is the SINGLE canonical owner of + * live-plan/summary counting: which files on disk are a "plan", which are a + * "summary", and how the two pair up. Every other module that reads a phase + * directory and re-derives that filename grammar itself — `readdirSync(...)` + * filtered by an inline `-PLAN.md` / `PLAN.md` / `-SUMMARY.md` / `SUMMARY.md` + * pattern — is a re-derivation that can silently drift from the owner (the + * exact failure class this epic removes; see #2349, #1988). + * + * Per ADR-3180 Decision 4(a) this guard discovers call sites by SCANNING THE + * WHOLE `src/` TREE, not by consulting an allowlist of known files — an + * allowlist only measures re-derivations in files someone remembered to + * list, and a new call site added anywhere else would sail through silently. + * + * Detection is intentionally NARROW and mirrors the existing + * `lint-phase-id-drift.cjs` precedent: a small, readable per-line regex pair + * over authored TypeScript source, with a short, explicitly-named exemption + * list — not a general-purpose AST/control-flow analysis. A line counts as a + * re-derivation when it contains BOTH: + * (a) a filename-TEST operation — `.filter(`, `.test(`, `.match(`, + * `.exec(`, `.endsWith(`, `.startsWith(`, `.includes(`, `.some(`, + * `.every(`, or `===` — and + * (b) a plan/summary filename-suffix pattern, either a quoted literal + * ('-PLAN.md', 'PLAN.md', '-SUMMARY.md', 'SUMMARY.md') OR an unquoted + * regex literal that mentions PLAN or SUMMARY and `\.md` together + * (`/-PLAN\.md$/`, `/^PLAN-\d+.*\.md$/i`) + * on the same source line. #3183 originally required (a) to be specifically + * `.filter(` on the SAME line as the literal — that missed a regex-literal + * predicate (`files.filter(f => /-PLAN\.md$/.test(f))`, no quotes) and a + * predicate defined on one line and consumed by `.filter(` on another + * (`const isPlan = f => f.endsWith('-PLAN.md'); … files.filter(isPlan)`). + * Widening (a) to any filename-test operator — not just `.filter(` itself — + * catches both: the predicate's OWN line already carries a qualifying test + * operation (`.test(`/`.endsWith(`) alongside the literal, independent of + * where `.filter(` ends up. + * + * KNOWN, ACCEPTED limits of a per-line textual scan (same tradeoff the + * phase-id-drift guard documents): a re-derivation that filters via a + * hand-rolled loop with none of the listed test operators (e.g. a manual + * character-index scan), or one whose literal and test operator are split + * across two DIFFERENT lines with no single line carrying both, is not + * caught by this narrow shape. That is left to code review, not this regex. + * + * `isInsideRoot`'s root-confinement check (used by the symlink-following + * `walk`, below) is an EXACT string comparison, deliberately not + * case-normalized. On a case-insensitive filesystem (macOS default; not CI, + * which is ubuntu) a symlink whose target is a case-VARIANT of an in-root + * path — a path the OS itself would still resolve to the same file — is + * REJECTED by this exact comparison and silently left unscanned. This is a + * fail-CLOSED miss (a re-derivation goes unreported), never an escape (never + * a wrongly-admitted outside-root read), so it is left as-is: making the + * comparison case-insensitive would WEAKEN confinement (a resolved path + * merely case-differing from a sibling-of-root name, `/repo-Evil` vs + * `/repo-evil`, could then be wrongly admitted) to fix a gap that only ever + * under-reports on a platform this guard is not gated on. + * + * A regex literal longer than MAX_REGEX_LITERAL_LEN (400) characters is not + * read, and is therefore not caught. That bound is what keeps the scan + * linear; no real plan/summary filename filter approaches it. The scan is + * scoped to SCAN_DIRS (`src`) with SCAN_EXT (.cts/.ts/.mts) — 186 files and + * 4,299 lines matching FILENAME_TEST_RE within SCAN_DIRS/SCAN_EXT as of this + * commit, 43 of them holding 7 or more backslashes and one (`src/milestone.cts`) + * holding 16. (Definition used, so this is reproducible: walk SCAN_DIRS + * filtering by SCAN_EXT exactly as `walk` does, split each file on `\n`, and + * count every line for which the exported `FILENAME_TEST_RE.test(line)` is + * true — independent of whether a PLAN/SUMMARY literal is also present on + * that line.) Those are the lines the old backtracking detector had to + * survive, and the reason the detector is now a tokenizer. + */ + +const fs = require('node:fs'); +const path = require('node:path'); + +// A `.filter(` call on the line — the shape every current re-derivation uses +// to turn a directory listing into a plan-or-summary subset. Kept as its own +// export for back-compat / documentation; FILENAME_TEST_RE below is the +// broadened detector actually used (any filename-test operator, not just +// `.filter(`). +const FILTER_CALL_RE = /\.filter\(/; + +// A filename-TEST operation: `.filter(`, `.test(`, `.match(`, `.exec(`, +// `.endsWith(`, `.startsWith(`, `.includes(`, `.some(`, `.every(`, or a +// strict-equality comparison. Any one of these on a line asking "is this +// filename a plan/summary" is a re-derivation, independent of whether the +// literal shows up as a `.filter(...)` predicate specifically. +const FILENAME_TEST_RE = /\.(?:filter|test|match|exec|endsWith|startsWith|includes|some|every)\(|===/; + +// A quoted plan/summary filename-suffix literal: 'PLAN.md', '-PLAN.md', +// 'SUMMARY.md', or '-SUMMARY.md', single- or double-quoted (opening and +// closing quote must match). +const PLAN_SUMMARY_LITERAL_RE = /(['"])-?(?:PLAN|SUMMARY)\.md\1/; + +// Longest regex literal this scanner will consider, in characters. Real +// plan/summary filename filters are far shorter; the bound is what keeps the +// scan linear. The tokenizer restarts at every `/` on the line (so that a +// literal preceded by a stray unpaired `/` is still found, matching the +// previous regex's "find anywhere" behaviour), which without a per-literal +// bound would be quadratic on a pathological line. With it the whole-line +// cost is O(n * MAX_REGEX_LITERAL_LEN) with no backtracking at all. +const MAX_REGEX_LITERAL_LEN = 400; + +// The two tokens that, appearing together INSIDE one regex literal, make it a +// plan/summary filename filter. `\.md` is matched as literal source text, not +// as a pattern, so there is nothing here to backtrack. +const PLAN_SUMMARY_TOKEN_RE = /PLAN|SUMMARY/i; +const ESCAPED_MD_TOKEN = '\\.md'; + +// Authored TypeScript source only (the generated bin/lib/*.cjs mirror it). +const SCAN_DIRS = ['src']; +const SCAN_EXT = new Set(['.cts', '.ts', '.mts']); + +// Directory names this scanner never descends into or reports out of — +// `.git` (repo internals, e.g. a persisted CI token in `.git/config`), +// `node_modules` (thousands of third-party files, none of them authored +// source), `dist` (build output). Named once and used at BOTH skip sites +// below: the cheap `entry.name` fast path in `walk`, and the resolved-path +// component check in `isUnderSkippedDir` — a symlink whose OWN name is not +// in this set but whose target resolves through a directory that IS (e.g. +// `src/g -> ../.git`, `src/nm -> ../node_modules`) must still be skipped, or +// the name-only check is a trivial bypass. +const SKIP_DIR_NAMES = new Set(['node_modules', 'dist', '.git']); + +// The canonical owner defines the grammar; it is exempt by construction. +const OWNER_FILE = path.join('src', 'plan-scan.cts'); + +// core-utils.cts's canonical pairing rule (#1988/#2648): these three +// functions build/match `*-SUMMARY.md` CANDIDATE strings for a given plan — +// that IS the single pairing rule, not a re-derivation of it. Scoped to just +// these functions (not the whole file) so an unrelated re-derivation added +// elsewhere in core-utils.cts is still caught. +const CORE_UTILS_FILE = path.join('src', 'core-utils.cts'); +const CORE_UTILS_EXEMPT_FUNCTIONS = new Set([ + 'summaryCandidates', + 'countMatchedSummaries', + 'findUnsummarizedPlans', + 'findOrphanSummaries', +]); + +// Per ADR-3180 Decision 4(a): NOT a bare file allowlist — each entry below is +// scoped to the SPECIFIC function asking a documented, different question +// (see the inline comment at each site), so an unrelated re-derivation added +// anywhere else in these same files is still caught. Mirrors the +// CORE_UTILS_EXEMPT_FUNCTIONS mechanism above, generalized per-file. +// +// - audit.cts scanQuickTasks: scans a quick task's OWN directory +// (`.planning/quick//`) for that ONE task's completion record — +// not a phase directory's live-plan/summary counting question. +// - gsd2-import.cts readTasksDir: reads a FOREIGN GSD-2 legacy project's +// `tasks/` dir convention during a one-time import, not this project's +// `.planning/phases/` layout at all. +// - estimate-cli.cts collectCalibrationSamples: pairs a PLAN.md and a +// SUMMARY.md by their identical `` to build an estimation +// CALIBRATION sample (projected vs. actual token counts) — a stem-keyed +// join for a statistics question, not a live-plan/completion count. +// It intentionally does NOT use the canonical three-candidate pairing +// rule (marker-swap / `-SUMMARY.md` / extended) or exclude superseded +// plans — an unmatched or superseded plan simply yields no sample, +// which is correct for calibration, not a live-completion determination. +// - roadmap.cts cmdRoadmapAnnotateDependencies: matches a plan-ID token +// out of an ALREADY-RENDERED ROADMAP.md checklist LINE OF TEXT +// (`- [ ] 01-01-PLAN.md — …`), not a filesystem directory listing — it +// can never diverge from scanPhasePlans's file-existence rule because it +// never tests file existence at all. +// - worktree-safety.cts defaultFindSummaryFiles: a recursive walk of the +// ENTIRE `.planning/` tree (not a single phase directory) for a +// pre-merge rescue of any `*SUMMARY.md` artifact, deliberately mirroring +// the shell fallback's own `find … -name "*SUMMARY.md"` glob (quick.md, +// #2296/#2070/#2838) byte-for-behaviour rather than the phase-scoped +// plan-scan owner's root+nested rule — "rescue every summary before a +// merge blows it away" is not a live-plan/completion count. +// - verify.cts cmdValidateConsistency: the strict `-(\d{2})-PLAN\.md$` +// match extracts a zero-padded SEQUENCE NUMBER from filenames the owner +// (`allPlanFiles`) already classified as plans — it does not re-derive +// "is this a plan", it answers a different, narrower question (does the +// canonical 2-digit numbering sequence have a gap) that the owner's +// boolean plan/summary classification cannot answer. See the extended +// inline comment at that call site for the full Question 1/2/3 split. +const FUNCTION_SCOPED_EXEMPTIONS = new Map([ + [CORE_UTILS_FILE, CORE_UTILS_EXEMPT_FUNCTIONS], + [path.join('src', 'audit.cts'), new Set(['scanQuickTasks'])], + [path.join('src', 'gsd2-import.cts'), new Set(['readTasksDir'])], + [path.join('src', 'estimate-cli.cts'), new Set(['collectCalibrationSamples'])], + [path.join('src', 'roadmap.cts'), new Set(['cmdRoadmapAnnotateDependencies'])], + [path.join('src', 'worktree-safety.cts'), new Set(['defaultFindSummaryFiles'])], + [path.join('src', 'verify.cts'), new Set(['cmdValidateConsistency'])], +]); + +// Optional `export ` modifier: `collectCalibrationSamples` (estimate-cli.cts) +// is declared `export function …` rather than a bare `function …`, and the +// function-boundary tracker below must still recognize it for its +// FUNCTION_SCOPED_EXEMPTIONS entry above to take effect. +const TOP_LEVEL_FUNCTION_RE = /^(?:export\s+)?function\s+([A-Za-z0-9_]+)\s*\(/; + +/** + * Read the JS regex literal starting at `line[start]` (which must be `/`). + * Returns `{ text, end }` — `text` includes the delimiters and any trailing + * flags, `end` is the index one past the literal — or null if no literal + * closes within MAX_REGEX_LITERAL_LEN characters. + * + * Single left-to-right pass, no backtracking. It models the two constructs a + * backtracking pattern gets wrong, which is why this is a tokenizer and not a + * regex: + * - `\x` escapes consume BOTH characters, so an escaped `\/` never + * terminates the literal; + * - inside a `[...]` character class a bare `/` does NOT terminate, so + * `/PLAN[\\/].*\.md$/` is one literal rather than two fragments. The + * previous regex silently MISSED every re-derivation using a + * cross-platform path-separator class for exactly this reason. + */ +function readRegexLiteralAt(line, start) { + if (line[start] !== '/') return null; + const limit = Math.min(line.length, start + MAX_REGEX_LITERAL_LEN); + let inClass = false; + for (let i = start + 1; i < limit; i++) { + const ch = line[i]; + if (ch === '\\') { + i++; // escape consumes the next character, whatever it is + continue; + } + if (ch === '\r' || ch === '\n') return null; // a literal cannot span lines + if (ch === '[') { + inClass = true; + } else if (ch === ']') { + inClass = false; + } else if (ch === '/' && !inClass) { + // Trailing flags are bounded by the SAME `limit` as the literal body + // itself (not `line.length`) — a literal followed by an unbounded run + // of lowercase letters must not make `text` grow past + // MAX_REGEX_LITERAL_LEN either. + let end = i + 1; + while (end < limit && line[end] >= 'a' && line[end] <= 'z') end++; + return { text: line.slice(start, end), end }; + } + } + return null; +} + +/** + * The regex literal on `line` that mentions PLAN or SUMMARY together with an + * escaped `.md` suffix — e.g. `/-PLAN\.md$/`, `/^PLAN-\d+.*\.md$/i`, + * `/-SUMMARY-\d+.*\.md$/i` — or null if there is none. Replaces the former + * `REGEX_LITERAL_MD_RE`, which was both exponentially/cubically backtracking + * (CodeQL js/redos; this guard runs in `lint:ci` on fork PRs) and unable to + * see a `[\\/]` character class. + */ +function findRegexLiteralMdMatch(line) { + for (let i = 0; i < line.length; i++) { + if (line[i] !== '/') continue; + const literal = readRegexLiteralAt(line, i); + if (!literal) continue; + // Case-insensitive `\.md` test — the regex literal this replaced carried + // the `i` flag, so `\.MD`/`\.Md` must still match. A lowercased-copy + // `.includes()` preserves that behaviour without reintroducing a + // backtracking regex. + if (PLAN_SUMMARY_TOKEN_RE.test(literal.text) && literal.text.toLowerCase().includes(ESCAPED_MD_TOKEN)) { + return literal.text; + } + } + return null; +} + +// Symlinks report `isDirectory()`/`isFile()` as false on the Dirent from +// `readdirSync`, so a symlinked `src/*.cts` (or a symlinked directory +// containing one) was previously invisible to this scanner — an evasion of a +// guard whose stated design principle (ADR-3180 Decision 4a) is whole-repo +// discovery with no allowlist. Resolve each entry with `fs.statSync` (which +// follows symlinks) to classify it, skipping broken links. `ctx.visitedRealDirs` +// guards against a symlink cycle sending `walk` into infinite recursion. +// +// Every sibling drift guard in `scripts/` (`lint-phase-id-drift.cjs`, +// `lint-package-identity-drift.cjs`, `lint-portable-timeout.cjs`, +// `lint-test-file-count.cjs`, `lint-allow-test-rule-refs.cjs`) uses the +// `Dirent` classification straight off `readdirSync` and does NOT follow +// symlinks at all. This guard follows them so a symlinked `src/*.cts` cannot +// evade ADR-3180 Decision 4a's whole-repo discovery — root confinement +// (`isInsideRoot` below) is the price of doing so: without it, a symlink +// planted anywhere under `src/` could walk this scanner out to read and +// report arbitrary files elsewhere on disk. +// +// DIRECTORY vs FILE symlinks are confined to two DIFFERENT roots, tracked as +// `ctx.scanDirRoot` (the realpath of the current top-level SCAN_DIRS entry, +// e.g. `/src`) vs `ctx.realRoot` (the whole repo): +// - a DIRECTORY symlink is descended ONLY if its resolved realpath is +// inside `ctx.scanDirRoot` — NOT merely inside `ctx.realRoot`. Without +// this, `src/up -> ..` (or `-> `) resolves inside the repo +// root and `walk` descends the ENTIRE repo, reporting violations under +// paths like `tests/not-src.cts` or `docs/other.cts` — files the header +// above says the scan is scoped OUT of (`SCAN_DIRS`). This is a +// deliberate, fail-CLOSED trade-off: a directory symlink pointing +// elsewhere INSIDE the repo (but outside the scan directory) is simply +// not followed. The alternative — descending it — is exactly the +// whole-repo sweep this rule exists to prevent, and a fork PR could use +// that sweep to redden `lint:ci` on files this guard was never meant to +// read. The narrower rule is worth more than the missed edge case. +// - a FILE symlink is still scanned if its resolved realpath is inside +// `ctx.realRoot` (the whole repo, not just the scan directory) — this is +// what keeps `src/alias.cts -> vendor/real.cts` covered (test (f)): an +// aliased file genuinely is part of the compiled surface even when its +// real target lives outside `src/`, and it is still reported under its +// canonical (real) path. +// +// A resolved path is inside a root only if it IS that root or begins with +// root + separator — a plain `startsWith(root)` would also accept a sibling +// directory whose name merely starts with the root's name (`/repo-evil`). +function isInsideRoot(realPath, realRoot) { + return realPath === realRoot || realPath.startsWith(realRoot + path.sep); +} + +// True when `realPath` (already confirmed inside `realRoot` by `isInsideRoot`) +// resolves THROUGH a skip-list directory anywhere along its path relative to +// the root — not just when `realPath` itself IS one. This is what closes the +// symlink bypass the `entry.name` fast path alone cannot: `walk` tests +// `entry.name` (the symlink's OWN name in its parent directory), but a +// symlink named something innocuous can still RESOLVE into `.git` / +// `node_modules` / `dist` (`src/g -> ../.git`, `src/leak.cts -> +// ../.git/config`, `src/nm -> ../node_modules`) — `isInsideRoot` alone admits +// all three, because every one of those real paths is still under the root. +function isUnderSkippedDir(realPath, realRoot) { + const rel = path.relative(realRoot, realPath); + return rel.split(path.sep).some((segment) => SKIP_DIR_NAMES.has(segment)); +} + +function walk(dir, acc, ctx) { + let entries; + try { + entries = fs.readdirSync(dir, { withFileTypes: true }); + } catch { + return acc; + } + for (const entry of entries) { + const full = path.join(dir, entry.name); + if (SKIP_DIR_NAMES.has(entry.name)) continue; // cheap fast path + let stat; + try { + stat = entry.isSymbolicLink() ? fs.statSync(full) : entry; + } catch { + continue; // broken symlink target + } + let realPath; + try { + realPath = fs.realpathSync(full); + } catch { + continue; // broken symlink target (race, or a link stat() followed but realpath cannot) + } + if (stat.isDirectory()) { + // Directories (symlinked or real) are confined to the CURRENT scan + // directory root, not merely the repo root — see the comment above + // `isInsideRoot` for why (`src/up -> ..` whole-repo sweep). + if (!isInsideRoot(realPath, ctx.scanDirRoot)) continue; + if (isUnderSkippedDir(realPath, ctx.realRoot)) continue; // symlink resolves through a skipped dir + if (ctx.visitedRealDirs.has(realPath)) continue; // symlink cycle guard + ctx.visitedRealDirs.add(realPath); + walk(full, acc, ctx); + } else if (stat.isFile() && SCAN_EXT.has(path.extname(entry.name))) { + // Files are confined to the whole repo root — a symlinked FILE whose + // real target lives outside the scan directory but inside the repo + // (e.g. `src/alias.cts -> vendor/real.cts`) is still part of the + // compiled surface and must be scanned. + if (!isInsideRoot(realPath, ctx.realRoot)) continue; + if (isUnderSkippedDir(realPath, ctx.realRoot)) continue; // symlink resolves through a skipped dir + if (ctx.visitedRealFiles.has(realPath)) continue; // two symlinks, same real file + ctx.visitedRealFiles.add(realPath); + acc.push(realPath); + } + } + return acc; +} + +/** + * Pure: find every unsanctioned plan/summary-filter re-derivation in `text`. + * `relPath` is the repo-relative path, used both to report file:line and to + * apply the narrow, function-scoped core-utils.cts exemption. + * Returns [{ line, found }]. + */ +function findPlanCountDrift(text, relPath) { + const out = []; + const lines = text.split('\n'); + const exemptFunctions = FUNCTION_SCOPED_EXEMPTIONS.get(relPath) || null; + let currentFunction = null; + for (let i = 0; i < lines.length; i++) { + const line = lines[i]; + const fnMatch = TOP_LEVEL_FUNCTION_RE.exec(line); + if (fnMatch) currentFunction = fnMatch[1]; + + if (!FILENAME_TEST_RE.test(line)) continue; + const quoted = PLAN_SUMMARY_LITERAL_RE.exec(line); + const found = quoted ? quoted[0] : findRegexLiteralMdMatch(line); + if (!found) continue; + + if (exemptFunctions && exemptFunctions.has(currentFunction)) continue; + + out.push({ line: i + 1, found }); + } + return out; +} + +/** + * Scan the authored source tree and return every unsanctioned re-derivation, + * each annotated with the repo-relative file path. + */ +function scanRepo(root) { + const violations = []; + let realRoot; + try { + realRoot = fs.realpathSync(root); + } catch { + return violations; // root itself does not exist / is unreadable + } + for (const dir of SCAN_DIRS) { + const scanDirPath = path.join(root, dir); + let scanDirRoot; + try { + scanDirRoot = fs.realpathSync(scanDirPath); + } catch { + continue; // scan directory itself does not exist / is unreadable + } + const ctx = { realRoot, scanDirRoot, visitedRealDirs: new Set(), visitedRealFiles: new Set() }; + for (const file of walk(scanDirPath, [], ctx)) { + // `file` is already the REAL path (walk pushes realPath, not the + // symlink path), so `rel` is the file's single canonical location + // regardless of which symlink reached it — this is what makes + // FUNCTION_SCOPED_EXEMPTIONS/OWNER_FILE, which are keyed on the + // repo-relative path, match consistently. + const rel = path.relative(realRoot, file); + if (rel === OWNER_FILE) continue; + let text; + try { + text = fs.readFileSync(file, 'utf8'); + } catch { + continue; + } + for (const d of findPlanCountDrift(text, rel)) { + violations.push({ file: rel, ...d }); + } + } + } + return violations; +} + +// Both a `found` fragment AND a reported file path are attacker-controlled +// source text on a fork PR (a repo can legally track a filename containing +// control bytes, so the path is exactly as attacker-controlled as the +// fragment — see the call sites in main() below), and both are written +// straight to a CI log. Replace C0/C1 control bytes (ANSI escapes included) +// with a visible \xNN, AND the non-Latin-1 formatting/bidi/line-separator +// codepoints below with \uNNNN, so a crafted literal or filename cannot +// rewrite the terminal rendering of the report or hide/reorder its own text: +// - U+200B-U+200F: zero-width space/joiners and directional marks +// - U+2028/U+2029: Unicode LINE SEPARATOR / PARAGRAPH SEPARATOR (line +// breaks a `\n`-only log scan would not catch) +// - U+202A-U+202E: bidi embedding/override controls (RLO etc.) +// - U+2066-U+2069: bidi isolate controls +function sanitizeForReport(text) { + return text + // eslint-disable-next-line no-control-regex -- the control range IS the target + .replace(/[\x00-\x1f\x7f-\x9f]/g, (c) => '\\x' + c.charCodeAt(0).toString(16).padStart(2, '0')) + .replace(/[\u200B-\u200F\u2028\u2029\u202A-\u202E\u2066-\u2069]/g, (c) => '\\u' + c.charCodeAt(0).toString(16).padStart(4, '0')); +} + +function main() { + const root = path.join(__dirname, '..'); + const violations = scanRepo(root); + if (violations.length === 0) { + process.stdout.write('ok plan-count-drift: no unsanctioned plan/summary re-derivations outside plan-scan.cts\n'); + return; + } + process.stderr.write('plan-count-drift: independent re-derivation(s) of plan/summary filename filtering found.\n'); + process.stderr.write('Use src/plan-scan.cjs `scanPhasePlans` (or core-utils.cjs `getPhaseFileStats`, which now\n'); + process.stderr.write('sources plans/summaries from it) instead of re-deriving the -PLAN.md/-SUMMARY.md filter:\n'); + for (const d of violations) { + // `d.file` is exactly as attacker-controlled as `d.found`: a repo can + // legally track a filename containing control bytes / bidi overrides, + // and it is a fork-PR-authored value reaching a CI log the same way the + // matched literal does — sanitize it at the same reporting boundary. + process.stderr.write(` ${sanitizeForReport(d.file)}:${d.line} ${sanitizeForReport(d.found)}\n`); + } + process.exitCode = 1; +} + +if (require.main === module) main(); + +module.exports = { + findPlanCountDrift, + scanRepo, + FILTER_CALL_RE, + FILENAME_TEST_RE, + PLAN_SUMMARY_LITERAL_RE, + findRegexLiteralMdMatch, + readRegexLiteralAt, + MAX_REGEX_LITERAL_LEN, + isInsideRoot, + sanitizeForReport, +}; diff --git a/src/audit.cts b/src/audit.cts index c1007d292..8d51fb092 100644 --- a/src/audit.cts +++ b/src/audit.cts @@ -239,6 +239,14 @@ function scanQuickTasks(planDir: string): QuickTaskItem[] { // workflows/quick.md mandates `${quick_id}-SUMMARY.md`; older flows used // bare `SUMMARY.md`. Accept either to avoid false-positive "missing". + // + // #3183 (ADR-3180 Decision 4(a) — bucket B, out of scope for the + // scanPhasePlans migration): this scans a quick task's OWN directory + // (`.planning/quick//`) for THAT task's single completion record — + // "does this one quick task have a SUMMARY.md" — not a phase directory's + // live-plan/summary counting question. scanPhasePlans is the wrong tool + // here; there is no plan/summary PAIRING to derive, only a single + // filename presence check local to a non-phase directory. let summaryPath: string | null = null; try { const summaryFiles = fs.readdirSync(safeTaskDir, { withFileTypes: true }) diff --git a/src/check-command-router.cts b/src/check-command-router.cts index c74c66ad9..0da5acddb 100644 --- a/src/check-command-router.cts +++ b/src/check-command-router.cts @@ -43,6 +43,12 @@ const { evaluatePredicate } = gatePredicateEval; import apiCoverageMod = require('./api-coverage.cjs'); const { detectApiIntegration, validateCoverageMatrix } = apiCoverageMod; import { execTool, platformReadSync, posixNormalize } from './shell-command-projection.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planScanMod = require('./plan-scan.cjs'); +const { scanPhasePlans } = planScanMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; // ─── Helpers ────────────────────────────────────────────────────────────────── @@ -133,13 +139,13 @@ function gateEnabled(projectDir: string): boolean { function loadPlanContents(phaseDir: string): string[] { if (!fs.existsSync(phaseDir)) return []; - try { - return fs.readdirSync(phaseDir) - .filter((entry) => /-PLAN\.md$/.test(entry)) - .map((entry) => readIfExists(path.join(phaseDir, entry))); - } catch { - return []; - } + // #3183 (lint-plan-count-drift): source live plan files from the single + // owner (scanPhasePlans) instead of a local `-PLAN.md` readdirSync filter + // — picks up bare PLAN.md and nested plans/, and excludes plans marked + // `status: superseded`, which the prior root-only exact-suffix filter did + // neither for. + return scanPhasePlans(phaseDir).planFiles + .map((entry) => readIfExists(path.join(phaseDir, entry))); } const DESIGNATED_HEADINGS_RE = /^#{1,6}\s+(?:must[_ ]haves?|truths?|tasks?|objective)\b/i; @@ -434,8 +440,11 @@ function cmdDecisionCoverageVerify(projectDir: string, args: string[], raw: bool } const planContents = loadPlanContents(phaseDir); + // #3183 (lint-plan-count-drift): same single-owner sourcing as + // loadPlanContents above — scanPhasePlans's summaryFiles instead of a + // local `-SUMMARY.md` readdirSync filter. const summaryParts = fs.existsSync(phaseDir) - ? fs.readdirSync(phaseDir).filter((entry) => /-SUMMARY\.md$/.test(entry)).map((entry) => readIfExists(path.join(phaseDir, entry))) + ? scanPhasePlans(phaseDir).summaryFiles.map((entry) => readIfExists(path.join(phaseDir, entry))) : []; const haystack = [ planContents.join('\n\n'), @@ -757,7 +766,9 @@ function cmdTddReviewCheckpoint(projectDir: string, args: string[], raw: boolean const tddPlanFiles: string[] = []; if (phaseDir) { try { - const files = fs.readdirSync(phaseDir).filter(f => f.endsWith('-PLAN.md')); + // #3183: canonical plan set (root+nested, superseded-excluded) from the + // single owner, rather than a root-only hand-rolled readdirSync filter. + const files = scanPhasePlans(phaseDir).planFiles; for (const file of files) { const planPath = path.join(phaseDir, file); const content = readIfExists(planPath); @@ -1394,12 +1405,26 @@ function isRealReadFailure(err: unknown): boolean { function readPhaseScope(projectDir: string, phaseDir: string, phaseNumber: string): PhaseScopeRead { const chunks: string[] = []; let readError: string | null = null; - try { - const entries = fs.readdirSync(phaseDir, { withFileTypes: true }); - const plans = entries - .filter((e) => e.isFile() && /-PLAN\.md$/i.test(e.name)) - .map((e) => e.name) - .sort(); + // A MISSING phase directory is fine (no plans yet → fall through to the + // roadmap). Checked up front (rather than via a readdirSync catch) because + // #3183 (lint-plan-count-drift) now sources the plan-file list from the + // single owner (scanPhasePlans) instead of a local `-PLAN\.md$` readdirSync + // filter — picks up bare PLAN.md and nested plans/, and excludes + // superseded plans, none of which the prior root-only exact-suffix filter + // did. + if (fs.existsSync(phaseDir)) { + const scan = scanPhasePlans(phaseDir); + if (scan.scope === SCOPE.UNREADABLE) { + // Directory exists but scanPhasePlans's own readdirSync(phaseDir) call + // failed (EACCES/EIO race) — a real read failure the gate must not + // silently pass (#2365 review), mirroring the prior isRealReadFailure + // branch below for the readdirSync-throws case. + return { + text: '', + readError: 'could not read the phase directory: scanPhasePlans reported scope UNREADABLE', + }; + } + const plans = [...scan.planFiles].sort(); for (const p of plans) { try { chunks.push(fs.readFileSync(path.join(phaseDir, p), 'utf8')); @@ -1411,16 +1436,6 @@ function readPhaseScope(projectDir: string, phaseDir: string, phaseNumber: strin } } } - } catch (err) { - // A MISSING phase directory is fine (no plans yet → fall through to the - // roadmap). A directory that exists but cannot be enumerated (EACCES/EIO) - // is a real read failure the gate must not silently pass (#2365 review). - if (isRealReadFailure(err)) { - return { - text: '', - readError: `could not read the phase directory: ${err instanceof Error ? err.message : String(err)}`, - }; - } } if (readError) return { text: chunks.join('\n\n'), readError }; if (chunks.join('').trim().length > 0) return { text: chunks.join('\n\n'), readError: null }; diff --git a/src/commands.cts b/src/commands.cts index ff59cd0bb..2cde54eb1 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -48,6 +48,9 @@ import modelProfiles = require('./model-profiles.cjs'); const { MODEL_PROFILES, VALID_PHASE_TYPES } = modelProfiles; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; import { realClock } from './clock.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planScanMod = require('./plan-scan.cjs'); +const { scanPhasePlans } = planScanMod; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -421,7 +424,14 @@ function cmdHistoryDigest(cwd: string, raw: boolean): void { try { for (const { name: dir, fullPath: dirPath } of allPhaseDirs) { - const summaries = fs.readdirSync(dirPath).filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); + // #3183: canonical summary set (root+nested) from the single owner. + // This call also opens every plan file's frontmatter to check + // superseded status even though cmdHistoryDigest never uses planFiles + // or the superseded distinction — that per-phase-dir cost is accepted + // deliberately (correctness/single-ownership over micro-optimization; + // summaryFiles itself is not superseded-filtered either way). Do not + // "optimize" this back into a second hand-rolled summary derivation. + const summaries = scanPhasePlans(dirPath).summaryFiles; for (const summary of summaries) { const summaryFilePath = path.join(dirPath, summary); @@ -1564,9 +1574,11 @@ function cmdProgressRender(cwd: string, format: string | undefined, raw: boolean const dm = dir.match(/^(\d+(?:\.\d+)*)-?(.*)/); const phaseNum = dm ? dm[1] : dir; const phaseName = dm && dm[2] ? dm[2].replace(/-/g, ' ') : ''; - const phaseFiles = fs.readdirSync(path.join(phasesDir, dir)); - const plans = phaseFiles.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').length; - const summaries = phaseFiles.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md').length; + // #3183: canonical plan/summary counts (root+nested, superseded-excluded, + // canonical pairing) from the single owner. + const phaseScan = scanPhasePlans(path.join(phasesDir, dir)); + const plans = phaseScan.planCount; + const summaries = phaseScan.summaryCount; totalPlans += plans; totalSummaries += summaries; @@ -1675,7 +1687,9 @@ function cmdTodoMatchPhase(cwd: string, phase: string | undefined, raw: boolean) if (phaseInfoDisk && phaseInfoDisk['found']) { try { const phaseDir = path.join(cwd, phaseInfoDisk['directory'] as string); - const planFiles = fs.readdirSync(phaseDir).filter(f => f.endsWith('-PLAN.md')); + // #3183: canonical plan set (root+nested, superseded-excluded) from the + // single owner, rather than a root-only hand-rolled readdirSync filter. + const planFiles = scanPhasePlans(phaseDir).planFiles; for (const pf of planFiles) { const planContent = platformReadSync(path.join(phaseDir, pf)); if (planContent === null) continue; @@ -1891,9 +1905,11 @@ function cmdStats(cwd: string, format: string | undefined, raw: boolean): void { // phaseName is everything after the token (strip leading '-') const afterToken = dir.slice(phaseToken ? phaseToken.length : 0).replace(/^-/, ''); const phaseName = afterToken ? afterToken.replace(/-/g, ' ') : ''; - const phaseFiles = fs.readdirSync(path.join(phasesDir, dir)); - const plans = phaseFiles.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').length; - const summaries = phaseFiles.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md').length; + // #3183: canonical plan/summary counts (root+nested, superseded-excluded, + // canonical pairing) from the single owner. + const phaseScan = scanPhasePlans(path.join(phasesDir, dir)); + const plans = phaseScan.planCount; + const summaries = phaseScan.summaryCount; totalPlans += plans; totalSummaries += summaries; diff --git a/src/core-utils.cts b/src/core-utils.cts index ae5a64c05..5f142ffc4 100644 --- a/src/core-utils.cts +++ b/src/core-utils.cts @@ -154,16 +154,6 @@ function transliterateForSlug(text: string): string { // ─── Phase file helpers ────────────────────────────────────────────────────── -/** Filter a file list to just PLAN.md / *-PLAN.md entries. */ -function filterPlanFiles(files: string[]): string[] { - return files.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md'); -} - -/** Filter a file list to just SUMMARY.md / *-SUMMARY.md entries. */ -function filterSummaryFiles(files: string[]): string[] { - return files.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); -} - interface PhaseFileStats { plans: string[]; summaries: string[]; @@ -171,20 +161,65 @@ interface PhaseFileStats { hasContext: boolean; hasVerification: boolean; hasReviews: boolean; + scope: string; +} + +// Minimal shape this module needs from plan-scan.cjs's scanPhasePlans result. +interface PlanScanResultShape { + planFiles: string[]; + summaryFiles: string[]; + scope: string; } /** * Read a phase directory and return counts/flags for common file types. + * + * #3183 (ADR-3180 Decision 2): `plans`/`summaries` are derived from the + * canonical `scanPhasePlans` rather than a local re-derivation, so this + * primitive can no longer diverge from the single owner of live-plan + * counting. `scanPhasePlans` + * lives in plan-scan.cjs, which itself imports `countMatchedSummaries` from + * THIS module — a top-level import here would be circular, so the require + * is deferred (lazy, inside the function body) to break the cycle at load + * time. This mirrors the lazy-require seam already used elsewhere in this + * repo (see src/audit-command-router.cts) for the same "module A needs + * module B which needs module A" shape. + * + * `hasResearch`/`hasContext`/`hasVerification`/`hasReviews` stay on the raw + * `readdirSync` listing — they are not plan-scan concerns. + * + * Degrades on an unreadable directory instead of throwing: empty arrays, + * every flag false, scope UNREADABLE (mirroring scanPhasePlans's own + * degrade path). */ function getPhaseFileStats(phaseDir: string): PhaseFileStats { - const files = fs.readdirSync(phaseDir); + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/no-unsafe-assignment + const scanPhasePlans: (dir: string) => PlanScanResultShape = require('./plan-scan.cjs'); + const scan = scanPhasePlans(phaseDir); + + let files: string[]; + try { + files = fs.readdirSync(phaseDir); + } catch { + return { + plans: scan.planFiles, + summaries: scan.summaryFiles, + hasResearch: false, + hasContext: false, + hasVerification: false, + hasReviews: false, + scope: scan.scope, + }; + } + return { - plans: filterPlanFiles(files), - summaries: filterSummaryFiles(files), + plans: scan.planFiles, + summaries: scan.summaryFiles, hasResearch: files.some(f => f.endsWith('-RESEARCH.md') || f === 'RESEARCH.md'), hasContext: findContextMdIn(files) !== null, hasVerification: files.some(f => f.endsWith('-VERIFICATION.md') || f === 'VERIFICATION.md'), hasReviews: files.some(f => f.endsWith('-REVIEWS.md') || f === 'REVIEWS.md'), + scope: scan.scope, }; } @@ -305,6 +340,34 @@ function summaryCandidates(plan: string): string[] { ]; const extended = base.match(/^(\d+)-PLAN-(\d+)/i); if (extended) candidates.push(dir + extended[1] + '-' + extended[2] + '-SUMMARY.md'); + // #3183: canonical-id form. Restores the coverage of the pre-migration + // bespoke I001 rule (verify.cts, pre-#3183, via validate.cjs's now-unused + // `canonicalPlanStem` — behaviourally identical to `extractCanonicalPlanId`, + // confirmed empirically), which matched a plan carrying a descriptive slug + // after its - id — e.g. `68-01-scaffolding-PLAN.md` — against + // a summary named only by the bare id — `68-01-SUMMARY.md`. None of the + // three candidates above produce that filename. + // + // Narrowed to the case `extractCanonicalPlanId` actually extracted an + // - pair (its result differs from the plan's own PLAN-stripped + // base). When no pair is found it falls back to returning that same base + // unchanged, which would otherwise push a redundant candidate identical to + // the `-SUMMARY.md` form above (e.g. `setup-PLAN.md` -> canonical + // 'setup' -> 'setup-SUMMARY.md', already candidate #2) rather than the + // original rule's actual behavior of matching only real id pairs. + // + // Collision, matching the original rule byte-for-behaviour: two plans that + // share the same - id but differ only in their descriptive + // slug (`68-01-alpha-PLAN.md` + `68-01-beta-PLAN.md`) both generate the + // SAME candidate `68-01-SUMMARY.md` and therefore BOTH read as summarized + // off one shared summary file. This is not a new regression: the + // pre-migration bespoke rule collapsed the same way (it populated one + // `summaryBases` Set keyed by canonical stem, so any plan whose canonical + // stem hit the set counted as matched, with no cardinality check against + // how many plans shared that stem). + const planStem = base.replace(/-PLAN$/i, ''); + const canonicalId = extractCanonicalPlanId(base + '.md'); + if (canonicalId !== planStem) candidates.push(dir + canonicalId + '-SUMMARY.md'); return candidates; } @@ -324,6 +387,24 @@ function findUnsummarizedPlans(planFiles: string[], summaryFiles: string[]): str return planFiles.filter((plan) => !summaryCandidates(plan).some((c) => summarySet.has(c))); } +/** + * #3183: the mirror image of `findUnsummarizedPlans` — the summary files in + * `summaryFiles` that do NOT pair with ANY plan in `planFiles`, using the + * identical `summaryCandidates` matching rule as `countMatchedSummaries` / + * `findUnsummarizedPlans`. Callers that must name orphaned summaries (a + * stray non-plan summary, or a summary whose plan was renamed/removed) need + * this instead of a bespoke exact-suffix Set-diff, which cannot recognize + * the nested or extended naming forms `summaryCandidates` already handles — + * a divergence that produced false "orphan summary" warnings. + */ +function findOrphanSummaries(planFiles: string[], summaryFiles: string[]): string[] { + const claimed = new Set(); + for (const plan of planFiles) { + for (const candidate of summaryCandidates(plan)) claimed.add(candidate); + } + return summaryFiles.filter((s) => !claimed.has(s)); +} + export = { toPosixPath, detectSubRepos, @@ -331,12 +412,11 @@ export = { pathExistsInternal, generateSlugInternal, transliterateForSlug, - filterPlanFiles, - filterSummaryFiles, getPhaseFileStats, readSubdirectories, timeAgo, extractCanonicalPlanId, countMatchedSummaries, findUnsummarizedPlans, + findOrphanSummaries, }; diff --git a/src/gap-checker.cts b/src/gap-checker.cts index c68a48083..0cedde9e2 100644 --- a/src/gap-checker.cts +++ b/src/gap-checker.cts @@ -29,6 +29,9 @@ import planningWorkspace = require('./planning-workspace.cjs'); const { planningPaths, planningDir, findContextMdIn } = planningWorkspace; import { parseDecisions, extractDecisions } from './decisions.cjs'; import { iterateBullets } from './markdown-sectionizer.cjs'; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planScanMod = require('./plan-scan.cjs'); +const { scanPhasePlans } = planScanMod; // ─── Types ──────────────────────────────────────────────────────────────────── @@ -318,7 +321,12 @@ function runGapAnalysis(cwd: string, phaseDir: string, options: RunGapAnalysisOp let planText = ''; try { if (phaseDirFiles.length > 0) { - const files = phaseDirFiles.filter(f => /-PLAN\.md$/.test(f)); + // #3183 (lint-plan-count-drift): source the live plan-file list from + // the single owner (scanPhasePlans) instead of a local `-PLAN\.md$` + // filter on the already-read listing — picks up bare PLAN.md, nested + // plans/, and excludes superseded plans, none of which the prior + // root-only exact-suffix filter did. + const files = scanPhasePlans(absPhaseDir).planFiles; planText = files.map(f => { try { return fs.readFileSync(path.join(absPhaseDir, f), 'utf-8'); } catch { return ''; } diff --git a/src/gsd2-import.cts b/src/gsd2-import.cts index dec1224de..b54646659 100644 --- a/src/gsd2-import.cts +++ b/src/gsd2-import.cts @@ -177,6 +177,12 @@ function parseTaskMustHaves(content: string): string[] { /** * Read all task plan files from a GSD-2 tasks/ directory. */ +// #3183 (ADR-3180 Decision 4(a) — bucket B, out of scope for the +// scanPhasePlans migration): this reads a FOREIGN GSD-2 legacy project's own +// `tasks/` directory convention (`T##-PLAN.md`) during a one-time import — +// it is not this project's `.planning/phases//` layout at all, has no +// nested-plans/superseded-status concept, and scanPhasePlans's grammar +// (which is scoped to GSD's OWN phase directories) does not apply here. function readTasksDir(tasksDir: string): TaskInfo[] { if (!fs.existsSync(tasksDir)) return []; diff --git a/src/milestone.cts b/src/milestone.cts index 14e77aa25..922c17979 100644 --- a/src/milestone.cts +++ b/src/milestone.cts @@ -32,7 +32,10 @@ import roadmapParserMod = require('./roadmap-parser.cjs'); const { getMilestonePhaseFilter, extractCurrentMilestone, getMilestoneInfo } = roadmapParserMod; // eslint-disable-next-line @typescript-eslint/no-require-imports import coreUtilsMod = require('./core-utils.cjs'); -const { extractOneLinerFromBody } = coreUtilsMod; +const { extractOneLinerFromBody, countMatchedSummaries } = coreUtilsMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module +import planScanMod = require('./plan-scan.cjs'); +const { scanPhasePlans } = planScanMod; const { planningPaths } = planningWorkspace; const { extractFrontmatter } = frontmatterMod; const { writeStateMd } = stateMod; @@ -333,17 +336,21 @@ function cmdRequirementsReadyIds(cwd: string, args: string[], raw: boolean): voi } const planAbsPath = path.resolve(cwd, planPathArg); - const phaseDir = path.dirname(planAbsPath); - const currentBasename = path.basename(planAbsPath); + // #3183: `planPathArg` may point at a root plan (`/-PLAN.md`) + // or a nested plan (`/plans/PLAN-.md`, #3139 layout) — + // scanPhasePlans always operates on the PHASE dir, so a nested plan needs + // one extra `dirname` to reach it, and its planFiles-relative identity + // carries the `plans/` prefix scanPhasePlans itself applies. + const isNestedPlanPath = path.basename(path.dirname(planAbsPath)) === 'plans'; + const phaseDir = isNestedPlanPath ? path.dirname(path.dirname(planAbsPath)) : path.dirname(planAbsPath); + const currentRelative = isNestedPlanPath ? `plans/${path.basename(planAbsPath)}` : path.basename(planAbsPath); - let siblingPlanFiles: string[] = []; - try { - siblingPlanFiles = fs - .readdirSync(phaseDir) - .filter((f) => f.endsWith('-PLAN.md') && f !== currentBasename); - } catch { - siblingPlanFiles = []; - } + // #3183: canonical plan/summary sets (root+nested, superseded-excluded) + // from the single owner, rather than a root-only hand-rolled readdirSync + // filter — a superseded sibling that still declares reqId with no SUMMARY + // used to block the ID forever (false-block); it is now excluded upstream. + const phaseScan = scanPhasePlans(phaseDir); + const siblingPlanFiles = phaseScan.planFiles.filter((f) => f !== currentRelative); const parseFrontmatterReqIds = (content: string, sourcePath?: string): string[] => { const fm = extractFrontmatter(content, sourcePath); @@ -379,9 +386,11 @@ function cmdRequirementsReadyIds(cwd: string, args: string[], raw: boolean): voi if (!siblingDeclaresId) continue; // Sibling declares the SAME ID — it must have finished (produced a - // SUMMARY) before this ID is ready to mark Complete. - const siblingSummaryPath = siblingPath.replace(/-PLAN\.md$/, '-SUMMARY.md'); - if (!fs.existsSync(siblingSummaryPath)) { + // SUMMARY) before this ID is ready to mark Complete. Canonical pairing + // via countMatchedSummaries (root+nested, all three naming forms) + // instead of a bespoke -PLAN.md→-SUMMARY.md regex swap. + const siblingHasSummary = countMatchedSummaries([siblingFile], phaseScan.summaryFiles) > 0; + if (!siblingHasSummary) { blockedBySibling = true; break; } @@ -643,10 +652,12 @@ function cmdMilestoneComplete(cwd: string, version: string, options: MilestoneCo if (!isDirInMilestone(dir)) continue; phaseCount++; - const phaseFiles = fs.readdirSync(path.join(phasesDir, dir)); - const plans = phaseFiles.filter((f) => f.endsWith('-PLAN.md') || f === 'PLAN.md'); - const summaries = phaseFiles.filter((f) => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); - totalPlans += plans.length; + // #3183: canonical plan/summary sets (root+nested, superseded-excluded) + // from the single owner, rather than a root-only hand-rolled readdirSync + // filter. + const phaseScan = scanPhasePlans(path.join(phasesDir, dir)); + const summaries = phaseScan.summaryFiles; + totalPlans += phaseScan.planCount; // Extract one-liners from summaries for (const s of summaries) { diff --git a/src/phase-locator.cts b/src/phase-locator.cts index d2c4db1ea..92f4b0d47 100644 --- a/src/phase-locator.cts +++ b/src/phase-locator.cts @@ -23,7 +23,7 @@ import phaseIdModule = require('./phase-id.cjs'); const { normalizePhaseName, phaseTokenMatches, extractPhaseToken } = phaseIdModule; // eslint-disable-next-line @typescript-eslint/no-require-imports import coreUtilsModule = require('./core-utils.cjs'); -const { readSubdirectories, getPhaseFileStats, extractCanonicalPlanId, toPosixPath } = coreUtilsModule; +const { readSubdirectories, getPhaseFileStats, extractCanonicalPlanId, toPosixPath, findUnsummarizedPlans } = coreUtilsModule; // eslint-disable-next-line @typescript-eslint/no-require-imports import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir } = planningWorkspace; @@ -178,18 +178,13 @@ function searchPhaseInDir(baseDir: string, relBase: string, normalized: string): const plans = unsortedPlans.sort(); const summaries = unsortedSummaries.sort(); - const completedPlanIds = new Set( - summaries.flatMap(s => { - const exact = s.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); - const canonical = extractCanonicalPlanId(s); - return canonical === exact ? [exact] : [exact, canonical]; - }) - ); - const incompletePlans = plans.filter(p => { - const planId = p.replace('-PLAN.md', '').replace('PLAN.md', ''); - const canonical = extractCanonicalPlanId(p); - return !completedPlanIds.has(planId) && !completedPlanIds.has(canonical); - }); + // #3183 (ADR-3180 Decision 2): the summary→plan pairing used to be a + // bespoke rule local to this function (a third pairing rule alongside + // scanPhasePlans's completion check and countMatchedSummaries). Routed + // through the canonical core-utils.findUnsummarizedPlans instead, which + // shares its `summaryCandidates` matching rule with countMatchedSummaries + // so the count and this named list can never disagree. + const incompletePlans = findUnsummarizedPlans(plans, summaries); // #2830: reverse lookup from a completed plan's id (exact or canonical) to // its actual summary filename. Shared builder (also used by phase.cts's diff --git a/src/phase.cts b/src/phase.cts index be149bcab..ed2cf23fd 100644 --- a/src/phase.cts +++ b/src/phase.cts @@ -85,9 +85,6 @@ const { updatePerformanceMetricsSection, } = stateMod; -// #2893 — strict canonical filter: `{padded_phase}-{NN}-PLAN.md` or `PLAN.md`. -const isCanonicalPlanFile = (f: string): boolean => f.endsWith('-PLAN.md') || f === 'PLAN.md'; - // Any .md file with PLAN anywhere in the basename — diagnostic net const PLAN_OUTLINE_RE = /-PLAN-OUTLINE\.md$/i; const PLAN_PRE_BOUNCE_RE = /-PLAN.*\.pre-bounce\.md$/i; @@ -243,11 +240,29 @@ function cmdPhasesList(cwd: string, options: PhaseListOptions, raw: boolean): vo let filtered: string[]; if (type === 'plans') { - filtered = dirFiles.filter(isCanonicalPlanFile); + // #3183: this is a "what plan files physically exist" query (this + // IS the file-listing command), not a live-completion question, so + // it uses the single owner's allPlanFiles (root+nested, INCLUDING + // status: superseded) rather than a root-only readdirSync filter + // that also missed nested plans. + // + // #2893 (regression fix): `allPlanFiles` also carries + // `isRootPlanFile`'s loose `/PLAN/i` fallback (deliberately + // permissive for live-plan COUNTING elsewhere — see + // plan-count-single-owner.test.cjs). That fallback silently + // recognized a non-canonically-named file (e.g. + // `01-PLAN-01-foundation.md`) as "matched", which defeated this + // command's #2893 naming-convention diagnostic entirely (no + // warning, file listed as if valid). Intersect with the STRICT + // `isCanonicalPlanFile` predicate so this diagnostic — and the + // `files` list this command actually returns — only ever + // recognizes the canonical root/nested forms, exactly like the + // pre-#3183 behavior this feature was built and tested against. + filtered = scanPhasePlans(dirPath).allPlanFiles.filter(isCanonicalPlanFile); const w = describeNonCanonicalPlans(dirFiles, filtered); if (w) warnings.push(`${dir}: ${w}`); } else if (type === 'summaries') { - filtered = dirFiles.filter((f) => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); + filtered = scanPhasePlans(dirPath).summaryFiles; } else { filtered = dirFiles; } @@ -471,9 +486,31 @@ function cmdFindPhase(cwd: string, phase: string, raw: boolean): void { const phaseDir = path.join(searchDir, match); const phaseFiles = fs.readdirSync(phaseDir); - const plans = phaseFiles.filter(isCanonicalPlanFile).sort(); - const summaries = phaseFiles.filter((f) => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md').sort(); - const planNamingWarning = describeNonCanonicalPlans(phaseFiles, plans); + // #3183: canonical, live (superseded-excluded) plan/summary sets + // (root+nested) from the single owner, rather than a root-only + // isCanonicalPlanFile filter + hand-rolled summary filter. + // + // #2893 (regression fix): both `plans` and the naming-diagnostic + // "matched" set are further intersected with the STRICT + // `isCanonicalPlanFile` predicate — scanPhasePlans's own + // planFiles/allPlanFiles carry `isRootPlanFile`'s loose `/PLAN/i` + // fallback (deliberately permissive for live-plan COUNTING elsewhere), + // which silently recognized a non-canonically-named file (e.g. + // `01-PLAN-01-foundation.md`) as a valid plan here and defeated this + // command's #2893 naming-convention diagnostic (no warning, offender + // listed in `plans` as if valid). + const phaseScan = scanPhasePlans(phaseDir); + const plans = phaseScan.planFiles.filter(isCanonicalPlanFile).sort(); + const summaries = phaseScan.summaryFiles.slice().sort(); + // describeNonCanonicalPlans is a NAMING-CONVENTION diagnostic, unrelated + // to supersession — compare against allPlanFiles (every plan-shaped file + // the owner recognizes, canonical or not) rather than the live-only + // `plans`, so a superseded-but-canonically-named plan is not misreported + // as a naming violation. + const planNamingWarning = describeNonCanonicalPlans( + phaseFiles, + phaseScan.allPlanFiles.filter(isCanonicalPlanFile), + ); const result: Record = { found: true, @@ -630,23 +667,52 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { } void phaseDirName; // used only to set phaseDir above + // phaseFiles stays root-only readdirSync — it feeds only + // describeNonCanonicalPlans's near-miss naming diagnostic below, which is + // advisory text, not a counted/scheduled file set. const phaseFiles = fs.readdirSync(phaseDir); - const planFiles = phaseFiles.filter(isCanonicalPlanFile).sort(); - const summaryFiles = phaseFiles.filter((f) => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); - const planNamingWarning = describeNonCanonicalPlans(phaseFiles, planFiles); - - const completedPlanIds = new Set( - summaryFiles.flatMap((s) => { - const exact = s.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); - const canonical = extractCanonicalPlanId(s); - return canonical === exact ? [exact] : [exact, canonical]; - }), + // #3183 (highest-severity site, ADR-3180 Decision 2): canonical LIVE + // plan/summary sets (root+nested, status: superseded EXCLUDED) from the + // single owner. This fixes two real bugs in the wave/dependency index this + // function builds: (1) a superseded plan used to still get scheduled into + // an execution wave, and (2) a phase using the #3139 nested `plans/` + // layout used to report ZERO plans (root-only readdirSync, no `plans/` + // join). + // #2893 (regression fix): intersected with the STRICT `isCanonicalPlanFile` + // predicate — scanPhasePlans's own planFiles/allPlanFiles carry + // `isRootPlanFile`'s loose `/PLAN/i` fallback (deliberately permissive for + // live-plan COUNTING elsewhere), which silently scheduled a + // non-canonically-named file (e.g. `01-PLAN-01-foundation.md`) into a wave + // here and defeated this command's #2893 naming-convention diagnostic (no + // warning). Restores the pre-#3183, tested behavior: only canonical + // root/nested filenames are ever counted or scheduled by this command. + const phaseScan = scanPhasePlans(phaseDir); + const planFiles = phaseScan.planFiles.filter(isCanonicalPlanFile).sort(); + const summaryFiles = phaseScan.summaryFiles; + // describeNonCanonicalPlans is a NAMING-CONVENTION diagnostic, unrelated to + // supersession — compare against allPlanFiles (every plan-shaped file the + // owner recognizes, canonical or not) rather than the live-only planFiles, + // so a superseded-but-canonically-named plan is not misreported as a + // naming violation. + const planNamingWarning = describeNonCanonicalPlans( + phaseFiles, + phaseScan.allPlanFiles.filter(isCanonicalPlanFile), ); + + // #3183: completion pairing via the canonical findUnsummarizedPlans + // (shares its `summaryCandidates` matching rule with countMatchedSummaries, + // and is layout-agnostic — it pairs a nested `plans/PLAN-01.md` with + // `plans/SUMMARY-01.md` correctly) instead of a bespoke ID-Set built from + // extractCanonicalPlanId, which only ever handled the root-canonical + // `-PLAN.md`/`-SUMMARY.md` naming form. + const unsummarizedPlanFiles = new Set(findUnsummarizedPlans(planFiles, summaryFiles)); // #2830: reverse lookup from a completed plan's id (exact or canonical) to // the actual summary filename, so a plan's own SUMMARY frontmatter can be // read for its `status`. Shared builder (also used by phase-locator.cts's // searchPhaseInDir) so the two can never disagree about which summary - // belongs to which plan. + // belongs to which plan. This is a FILE resolution for reading halted + // status, not a completion-count pairing rule, so it is unaffected by the + // #3183 pairing migration above. const summaryFileByPlanId = buildSummaryFileIndex(summaryFiles, extractCanonicalPlanId); // ── Pass 1: parse each plan file ───────────────────────────────────────── @@ -688,8 +754,7 @@ function cmdPhasePlanIndex(cwd: string, phase: string, raw: boolean): void { filesModified = Array.isArray(fmFiles) ? fmFiles.map(String) : [String(fmFiles)]; } - const hasSummary = - completedPlanIds.has(planId) || completedPlanIds.has(extractCanonicalPlanId(planFile)); + const hasSummary = !unsummarizedPlanFiles.has(planFile); // #2830: a plan can have a SUMMARY (hasSummary=true) and still be halted — // a designed stop still writes a completion record, just one whose status @@ -1629,11 +1694,14 @@ function cmdPhaseRemove( const targetDir = subdirs.find((d) => phaseTokenMatches(d, normalized)) || null; if (targetDir && !force) { - const files = fs.readdirSync(path.join(phasesDir, targetDir)); - const summaries = files.filter((f) => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); - if (summaries.length > 0) { + // #3183: canonical summary set (root+nested) from the single owner — + // a root-only readdirSync filter left nested (#3139 layout) summaries + // invisible, letting a phase with completed nested work be deleted + // without --force. + const summaryCount = scanPhasePlans(path.join(phasesDir, targetDir)).summaryFiles.length; + if (summaryCount > 0) { error( - `Phase ${targetPhase} has ${summaries.length} executed plan(s). Use --force to remove anyway.`, + `Phase ${targetPhase} has ${summaryCount} executed plan(s). Use --force to remove anyway.`, ); } } @@ -2810,7 +2878,7 @@ function cmdPhaseUatPassed( // paths without re-discovering the phase directory themselves. // eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module import planScanMod = require('./plan-scan.cjs'); -const { scanPhasePlans } = planScanMod; +const { scanPhasePlans, isCanonicalPlanFile } = planScanMod; function cmdPhaseListPlans(cwd: string, phaseNum: string | undefined, raw: boolean): void { if (!phaseNum) { diff --git a/src/plan-scan.cts b/src/plan-scan.cts index bd57f90d0..ae05e8153 100644 --- a/src/plan-scan.cts +++ b/src/plan-scan.cts @@ -15,6 +15,9 @@ const { countMatchedSummaries } = coreUtils; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatterMod = require('./frontmatter.cjs'); const { extractFrontmatter } = frontmatterMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; // Excluded derivative files const PLAN_OUTLINE_RE = /-OUTLINE\.md$/i; @@ -94,13 +97,58 @@ function isNestedSummaryFile(fileName: string): boolean { return /^SUMMARY-\d+.*\.md$/i.test(fileName) || /-SUMMARY-\d+.*\.md$/i.test(fileName); } +/** + * Strict canonical-naming predicate over a `scanPhasePlans` `planFiles`/ + * `allPlanFiles` ENTRY (root form bare, nested form `plans/`-prefixed, exactly + * as those arrays store them) — root `--PLAN.md`/bare `PLAN.md`, + * or nested `plans/PLAN-....md`/`plans/-PLAN-....md` — WITHOUT + * `isRootPlanFile`'s loose `/\.md$/i && /PLAN/i` fallback. + * + * The `plans/` prefix check is load-bearing, not cosmetic: `isNestedPlanFile` + * matches ANY basename containing `-PLAN-...md` with no anchor + * requiring an actual `plans/` directory — that shape is exactly the #2893 + * reporter's non-canonical example, `01-PLAN-01-foundation.md`. Applying + * `isNestedPlanFile` directly to a bare root-level name would therefore + * misclassify that exact offender as canonical. Only entries scanPhasePlans + * itself produced with the `plans/` prefix (i.e. read from the real nested + * subdirectory) are eligible for the nested check. + * + * #2893/#3183: `isRootPlanFile`'s loose fallback is deliberately permissive + * for live-plan COUNTING (a lowercase `plan.md` still counts toward + * completion — see plan-count-single-owner.test.cjs's pinned case-sensitivity + * asymmetry). But the #2893 "non-canonical filename" diagnostic (phase.cts's + * `describeNonCanonicalPlans`, used by find-phase/phase-plan-index/phases + * list --type plans) exists specifically to CATCH a plan-shaped file that + * does NOT match the canonical contract and warn instead of silently + * scheduling it. Feeding that diagnostic (and the `plans`/`files` lists those + * commands return) the loose `allPlanFiles`/`planFiles` set defeats the + * diagnostic entirely, since the loose fallback already recognizes the + * non-canonical file as "matched". This predicate is the STRICT filter those + * three call sites intersect against so the diagnostic (and what counts as a + * schedulable plan for those commands specifically) stays canonical-only, + * while scanPhasePlans's own planCount/summaryCount/completed stay on the + * loose, permissive rule. + */ +function isCanonicalPlanFile(fileEntry: string): boolean { + if (fileEntry.startsWith('plans/')) return isNestedPlanFile(fileEntry.slice('plans/'.length)); + return fileEntry.endsWith('-PLAN.md') || fileEntry === 'PLAN.md'; +} + interface PhaseScanResult { planCount: number; summaryCount: number; completed: boolean; hasNestedPlans: boolean; + // Callers asking "which plans are OUTSTANDING" (live-completion tracking, + // pairing, wave scheduling) use planFiles — it is post status:superseded + // exclusion. Callers asking "what plan files physically exist on disk" + // (e.g. numbering-gap detection) use allPlanFiles — it is EVERY plan file + // found, root + nested, BEFORE the superseded exclusion. One owner, two + // questions. planFiles: string[]; + allPlanFiles: string[]; summaryFiles: string[]; + scope: planningScopeMod.Scope; } function scanPhasePlans(phaseDir: string): PhaseScanResult { @@ -114,7 +162,9 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { completed: false, hasNestedPlans: false, planFiles: [], + allPlanFiles: [], summaryFiles: [], + scope: SCOPE.UNREADABLE, }; } @@ -123,6 +173,7 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { let nestedPlanFiles: string[] = []; let nestedSummaryFiles: string[] = []; let hasNestedPlans = false; + let scope: planningScopeMod.Scope = SCOPE.COMPLETE; const nestedDir = join(phaseDir, 'plans'); if (existsSync(nestedDir)) { @@ -131,7 +182,12 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { nestedPlanFiles = nestedFiles.filter(isNestedPlanFile).map((file) => `plans/${file}`); nestedSummaryFiles = nestedFiles.filter(isNestedSummaryFile).map((file) => `plans/${file}`); hasNestedPlans = nestedPlanFiles.length > 0; - } catch { /* ignore unreadable nested layout */ } + } catch { + // #3183 (ADR-3180 Decision 2): the nested plans/ dir exists but could not + // be read — this scan cannot see plans it knows are there, so zero is + // NOT a reliable answer; mark TRUNCATED rather than COMPLETE. + scope = SCOPE.TRUNCATED; + } } const allPlanFiles = rootPlanFiles.concat(nestedPlanFiles); @@ -166,7 +222,9 @@ function scanPhasePlans(phaseDir: string): PhaseScanResult { completed: allPlanFiles.length > 0 && summaryCount >= planCount, hasNestedPlans, planFiles, + allPlanFiles, summaryFiles, + scope, }; } @@ -179,4 +237,5 @@ export = Object.assign(scanPhasePlans, { isNestedPlanFile, isRootSummaryFile, isNestedSummaryFile, + isCanonicalPlanFile, }); diff --git a/src/planning-scope.cts b/src/planning-scope.cts new file mode 100644 index 000000000..a97edf4a6 --- /dev/null +++ b/src/planning-scope.cts @@ -0,0 +1,49 @@ +/** + * Planning Scope — shared result discriminator for live-plan scanning. + * + * ADR-3180 Decision 2 (docs/adr/3180-planning-semantic-model-single-owner.md): + * `scanPhasePlans` is the single owner of live-plan counting, and every + * consumer of that count needs to distinguish a REAL answer from a + * NON-answer. `SCOPE.COMPLETE` with zero items is a real answer — a phase + * genuinely has no plans yet. `SCOPE.TRUNCATED`, `SCOPE.UNSCOPED`, and + * `SCOPE.UNREADABLE` with zero items are NOT — they mean the scan could not + * see (part of) the phase directory, so a caller must not treat that zero as + * "this phase has no plans." + * + * This is a frozen enum, not a message string: CONTRIBUTING.md bans raw-text + * matching on outputs and requires a typed IR, so callers branch on the + * `SCOPE` value rather than pattern-matching prose. + * + * PROVISIONAL: this contract is pending this phase's validation and may be + * revised before the epic (#3180) ships. + * + * Dependencies: none — this is a leaf module (mirrors src/phase-id.cts). It + * imports nothing, so any consumer can depend on it without risking a cycle. + */ + +const SCOPE = Object.freeze({ + COMPLETE: 'complete', + TRUNCATED: 'truncated', + UNSCOPED: 'unscoped', + UNREADABLE: 'unreadable', +}); + +type Scope = (typeof SCOPE)[keyof typeof SCOPE]; + +const planningScope = { SCOPE }; +// Namespace merge (same binding name as the value above) is how a CommonJS +// `export =` module exposes a type alongside its runtime export — `export +// type` is rejected by TS2309 ("An export assignment cannot be used in a +// module with other exported elements") when combined with `export =`, so +// the `Scope` type rides along on the exported object via declaration +// merging instead. Consumers doing `import x = require('./planning-scope.cjs')` +// can reference the type as `x.Scope`. +// Required to merge a compile-time-only type onto the `export =` runtime +// value; there is no ES-module-syntax way to export a type alongside a CJS +// `export =`. +// eslint-disable-next-line @typescript-eslint/no-namespace +declare namespace planningScope { + export { Scope }; +} + +export = planningScope; diff --git a/src/state.cts b/src/state.cts index 803db8439..dbc24e6da 100644 --- a/src/state.cts +++ b/src/state.cts @@ -31,6 +31,9 @@ const { extractFrontmatter, reconstructFrontmatter, stripFrontmatter } = frontma // eslint-disable-next-line @typescript-eslint/no-require-imports import scanPhasePlans = require('./plan-scan.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports import stateTransitionMod = require('./state-transition.cjs'); const { transitionCore, applyStatePreservation, sliceCurrentPositionSection } = stateTransitionMod; type StateTransitionIntent = stateTransitionMod.StateTransitionIntent; @@ -3179,9 +3182,21 @@ function cmdStateRebuild(cwd: string, options: StateRebuildOptions, raw: boolean // Directory-name convention: `-` (e.g. `03-test-phase`). const m = entry.match(/^(\d+)-(.+)$/); if (!m) continue; - const files = fs.readdirSync(full); - const planCount = files.filter(f => /-PLAN\.md$/i.test(f)).length; - const summaryCount = files.filter(f => /-SUMMARY\.md$/i.test(f)).length; + // #3183 (lint-plan-count-drift / ADR-3180 Decision 2): source + // planCount/summaryCount from the single owner (scanPhasePlans) + // instead of a local root-only `-PLAN.md`/`-SUMMARY.md` readdirSync + // filter — picks up bare PLAN.md/SUMMARY.md and nested plans/. A + // non-COMPLETE scope (TRUNCATED: nested plans/ unreadable; + // UNREADABLE: `full` itself unreadable) is not a trustworthy count — + // throw so it surfaces via the outer catch as a real scan failure + // (`ok:false`), mirroring the #3057 B1 contract documented above for + // the sibling `fs.readdirSync(phasesDir)` failure mode, rather than + // silently reporting an undercount. + const scan = scanPhasePlans(full); + if (scan.scope !== SCOPE.COMPLETE) { + throw new Error(`could not fully scan plan directory (scope ${scan.scope}): ${full}`); + } + const { planCount, summaryCount } = scan; records.push({ number: m[1], name: m[2], planCount, summaryCount }); } return { ok: true, phases: records }; diff --git a/src/verify.cts b/src/verify.cts index f1e1d81db..fd3b27eb0 100644 --- a/src/verify.cts +++ b/src/verify.cts @@ -11,7 +11,7 @@ import path from 'node:path'; import os from 'node:os'; import { phaseVariants, buildRoadmapPhaseVariants, buildNotStartedPhaseVariants } from './validate.cjs'; import { realClock } from './clock.cjs'; -import { phaseDirNameRe, PHASE_TOKEN_FROM_DIR_RE, MILESTONE_ARCHIVE_DIR_RE, canonicalPlanStem, textEncodingError } from './validate.cjs'; +import { phaseDirNameRe, PHASE_TOKEN_FROM_DIR_RE, MILESTONE_ARCHIVE_DIR_RE, textEncodingError } from './validate.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-workspace.cjs is an export= CommonJS module import planningWorkspace = require('./planning-workspace.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- frontmatter.cjs is an export= CommonJS module @@ -22,6 +22,12 @@ import stateMod = require('./state.cjs'); import modelProfilesMod = require('./model-profiles.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports -- plan-scan.cjs is an export= CommonJS module import planScanMod = require('./plan-scan.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- core-utils.cjs is an export= CommonJS module +import coreUtilsMod = require('./core-utils.cjs'); +const { findOrphanSummaries, findUnsummarizedPlans } = coreUtilsMod; +// eslint-disable-next-line @typescript-eslint/no-require-imports -- planning-scope.cjs is an export= CommonJS module +import planningScopeMod = require('./planning-scope.cjs'); +const { SCOPE } = planningScopeMod; import { execGit, platformReadSync as safeReadFile, platformWriteSync, posixNormalize } from './shell-command-projection.cjs'; import { PACKAGE_NAME } from './package-identity.cjs'; import { formatGsdSlash, resolveRuntime } from './runtime-slash.cjs'; @@ -940,26 +946,28 @@ function cmdVerifyPhaseCompleteness(cwd: string, phase: string, raw: boolean): v const warnings: string[] = []; const phaseDir = path.join(cwd, phaseInfo['directory'] as string); - let files: string[]; - try { - files = fs.readdirSync(phaseDir); - } catch { + // #3183 (lint-plan-count-drift / ADR-3180 Decision 2): source plans/ + // summaries and their pairing from the single owner (scanPhasePlans + + // findUnsummarizedPlans/findOrphanSummaries) instead of a bespoke + // root-only `-PLAN.md`/`-SUMMARY.md` filter and exact-suffix-stem + // Set-diff. The prior pairing missed bare PLAN.md/SUMMARY.md, nested + // (#3139 layout) plans, and any of the canonical pairing's other + // recognized naming forms — producing false "Plans without summaries" / + // "Summaries without plans" for names it could not recognize as paired + // (the same failure class #1988/#2648 fixed for the owner's own callers). + const scan = planScanMod.scanPhasePlans(phaseDir); + if (scan.scope === SCOPE.UNREADABLE) { output({ error: 'Cannot read phase directory' }, raw); return; } + const { planFiles, summaryFiles } = scan; - const plans = files.filter((f) => f.match(/-PLAN\.md$/i)); - const summaries = files.filter((f) => f.match(/-SUMMARY\.md$/i)); - - const planIds = new Set(plans.map((p) => p.replace(/-PLAN\.md$/i, ''))); - const summaryIds = new Set(summaries.map((s) => s.replace(/-SUMMARY\.md$/i, ''))); - - const incompletePlans = [...planIds].filter((id) => !summaryIds.has(id)); + const incompletePlans = findUnsummarizedPlans(planFiles, summaryFiles); if (incompletePlans.length > 0) { errors.push(`Plans without summaries: ${incompletePlans.join(', ')}`); } - const orphanSummaries = [...summaryIds].filter((id) => !planIds.has(id)); + const orphanSummaries = findOrphanSummaries(planFiles, summaryFiles); if (orphanSummaries.length > 0) { warnings.push(`Summaries without plans: ${orphanSummaries.join(', ')}`); } @@ -968,8 +976,8 @@ function cmdVerifyPhaseCompleteness(cwd: string, phase: string, raw: boolean): v { complete: errors.length === 0, phase: phaseInfo['phase_number'], - plan_count: plans.length, - summary_count: summaries.length, + plan_count: planFiles.length, + summary_count: summaryFiles.length, incomplete_plans: incompletePlans, orphan_summaries: orphanSummaries, errors, @@ -1461,15 +1469,42 @@ function cmdValidateConsistency(cwd: string, raw: boolean): void { for (const dir of dirs) { const phasePath = path.join(phaseRoot, dir); const phaseLabel = posixNormalize(path.relative(planBase, phasePath)); - const phaseFiles = fs.readdirSync(phasePath); - const plans = phaseFiles.filter((f) => f.endsWith('-PLAN.md')).sort(); - const planNums = plans + // #3183: this loop mixes two DIFFERENT questions — split explicitly + // rather than migrating it as one blind swap-in. + + // QUESTION 1 — physical numbering-gap detection: wants EVERY plan + // file that physically exists, superseded or not (a retired plan + // still occupied a number in the sequence), root+nested. Uses the + // single owner's allPlanFiles rather than a root-only readdirSync + // filter. The strict `-NN-PLAN.md` suffix regex below already + // ignores any entry (nested, loose-named, bare PLAN.md) that isn't + // in the root canonical numbered form, so widening the input set is + // a pure visibility fix with no change to which files feed a number. + // + // One scan serves both questions below: `allPlanFiles` answers + // Question 1 (numbering-gap), `planFiles`/`summaryFiles` answer + // Question 2 (pairing) a few lines down. + const { allPlanFiles, planFiles, summaryFiles } = planScanMod.scanPhasePlans(phasePath); + + // Root-canonical numbered plans only (`-NN-PLAN.md`) — the + // shape the numbering-gap sequence check operates on. Matched via + // the numbering regex itself rather than a separate suffix filter, + // so this stays a single derivation from the owner's output, not a + // second independent re-derivation of its filename grammar. + const numberedPlans = allPlanFiles .map((p) => { const pm = p.match(/-(\d{2})-PLAN\.md$/); - return pm ? parseInt(pm[1], 10) : null; + return pm ? { file: p, num: parseInt(pm[1], 10) } : null; }) - .filter((n): n is number => n !== null); + .filter((e): e is { file: string; num: number } => e !== null) + .sort((a, b) => a.num - b.num); + // numberedPlans (and planNums below) answers Question 1 ONLY — the + // numbering-gap sequence check. It is a strict `-NN-PLAN.md` subset + // and must NOT be reused as a general "all live plans" set: a plan + // whose filename isn't in that canonical 2-digit form (a 3-digit + // continuation, a bare PLAN.md, etc.) is silently absent from it. + const planNums = numberedPlans.map((e) => e.num); for (let i = 1; i < planNums.length; i++) { if (planNums[i] !== planNums[i - 1] + 1) { @@ -1479,17 +1514,25 @@ function cmdValidateConsistency(cwd: string, raw: boolean): void { } } - const summaries = phaseFiles.filter((f) => f.endsWith('-SUMMARY.md')); - const planIds = new Set(plans.map((p) => p.replace('-PLAN.md', ''))); - const summaryIds = new Set(summaries.map((s) => s.replace('-SUMMARY.md', ''))); - - for (const sid of summaryIds) { - if (!planIds.has(sid)) { - warnings.push(`Summary ${sid}-SUMMARY.md in ${phaseLabel} has no matching PLAN.md`); - } + // QUESTION 2 — plan↔summary pairing: "does this summary have a + // matching LIVE plan" wants the single owner's superseded-excluded + // planFiles and the canonical summaryCandidates-based pairing + // (findOrphanSummaries) instead of an exact-suffix Set-diff, which + // produced false "orphan summary" warnings for legacy/nested naming + // forms it could not recognize as paired. + const orphanSummaries = findOrphanSummaries(planFiles, summaryFiles); + for (const orphan of orphanSummaries) { + warnings.push(`Summary ${orphan} in ${phaseLabel} has no matching PLAN.md`); } - for (const plan of plans) { + // QUESTION 3 — wave-frontmatter presence: "does every LIVE plan + // declare a wave" wants the same live (superseded-excluded) set as + // Question 2's pairing check — planFiles, NOT numberedPlans/Question + // 1's strict 2-digit subset. A superseded plan legitimately carries + // no wave, and a live plan whose filename isn't in canonical 2-digit + // form (a 3-digit continuation, a bare PLAN.md, etc.) must still be + // checked here even though it is invisible to the numbering-gap scan. + for (const plan of planFiles) { const planFilePath = path.join(phasePath, plan); const content = fs.readFileSync(planFilePath, 'utf-8'); const fmData = extractFrontmatter(content, planFilePath); @@ -1737,6 +1780,14 @@ function cmdValidateHealth( let phaseDirEntries: fs.Dirent[] = []; const phaseDirFiles = new Map(); + // #3183: companion map of the single owner's scan per phase dir + // (root+nested, superseded-excluded plan/summary sets + canonical + // pairing), computed alongside the raw readdirSync listing above. The + // W023 duplicate-dir describer and the I001 unsummarized-plan detector + // below use THIS map for plan/summary counts and pairing; phaseDirFiles + // stays raw for the RESEARCH/VALIDATION and phase-dir-naming checks that + // are not plan-count questions. + const phaseDirScans = new Map>(); try { phaseDirEntries = fs .readdirSync(phasesDir, { withFileTypes: true }) @@ -1747,6 +1798,7 @@ function cmdValidateHealth( } catch { phaseDirFiles.set(e.name, []); } + phaseDirScans.set(e.name, planScanMod.scanPhasePlans(path.join(phasesDir, e.name))); } } catch { /* intentionally empty */ @@ -1793,9 +1845,11 @@ function cmdValidateHealth( .slice() .sort((a, b) => comparePhaseNum(a, b) || String(a).localeCompare(String(b))) .map((d) => { - const files = phaseDirFiles.get(d) || []; - const plans = files.filter(f => f.endsWith('-PLAN.md') || f === 'PLAN.md').length; - const summaries = files.filter(f => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md').length; + // #3183: canonical plan/summary counts (root+nested, + // superseded-excluded, canonical pairing) from the single owner. + const scan = phaseDirScans.get(d); + const plans = scan ? scan.planCount : 0; + const summaries = scan ? scan.summaryCount : 0; const status = determinePhaseStatus(plans, summaries, path.join(phasesDir, d), 'Not Started'); return `${d} (${status})`; }) @@ -1809,23 +1863,18 @@ function cmdValidateHealth( } } + // I001 (#3183): this IS findUnsummarizedPlans's exact question — routed + // through the single owner's scan (root+nested, superseded-excluded plan + // set) and the canonical summaryCandidates-based pairing, instead of a + // bespoke canonicalPlanStem reimplementation. Fixes: a superseded plan is + // no longer permanently flagged "may be in progress" (false noise + // forever), and nested (#3139 layout) plans are no longer invisible. for (const e of phaseDirEntries) { - const phaseFiles = phaseDirFiles.get(e.name) || []; - const plans = phaseFiles.filter((f) => f.endsWith('-PLAN.md') || f === 'PLAN.md'); - const summaries = phaseFiles.filter((f) => f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'); - const summaryBases = new Set(); - for (const s of summaries) { - const summaryBase = s.replace('-SUMMARY.md', '').replace('SUMMARY.md', ''); - summaryBases.add(summaryBase); - summaryBases.add(canonicalPlanStem(summaryBase)); - } - - for (const plan of plans) { - const planBase = plan.replace('-PLAN.md', '').replace('PLAN.md', ''); - const canonicalBase = canonicalPlanStem(planBase); - if (!summaryBases.has(planBase) && !summaryBases.has(canonicalBase)) { - addIssue('info', 'I001', `${e.name}/${plan} has no SUMMARY.md`, 'May be in progress'); - } + const scan = phaseDirScans.get(e.name); + const planFiles = scan ? scan.planFiles : []; + const summaryFiles = scan ? scan.summaryFiles : []; + for (const plan of findUnsummarizedPlans(planFiles, summaryFiles)) { + addIssue('info', 'I001', `${e.name}/${plan} has no SUMMARY.md`, 'May be in progress'); } } @@ -2473,8 +2522,15 @@ function cmdVerifySchemaDrift( return; } + // #3183: canonical LIVE plan/summary sets (root+nested, + // status: superseded EXCLUDED) from the single owner, rather than a + // root-only readdirSync filter — a superseded plan's claimed + // files_modified is no longer treated as an expected drift target, and + // nested (#3139 layout) plans/summaries are no longer invisible to the + // drift check. + const { planFiles, summaryFiles } = planScanMod.scanPhasePlans(phaseDir); + const allFiles: string[] = []; - const planFiles = fs.readdirSync(phaseDir).filter((f) => f.endsWith('-PLAN.md')); for (const pf of planFiles) { const content = fs.readFileSync(path.join(phaseDir, pf), 'utf-8'); const fmMatch = content.match(/files_modified:\s*\[([^\]]{0,8000})\]/); @@ -2485,7 +2541,6 @@ function cmdVerifySchemaDrift( } let executionLog = ''; - const summaryFiles = fs.readdirSync(phaseDir).filter((f) => f.endsWith('-SUMMARY.md')); for (const sf of summaryFiles) { executionLog += fs.readFileSync(path.join(phaseDir, sf), 'utf-8') + '\n'; } diff --git a/tests/core-utils.test.cjs b/tests/core-utils.test.cjs index 4a0753f3d..67075d410 100644 --- a/tests/core-utils.test.cjs +++ b/tests/core-utils.test.cjs @@ -8,8 +8,6 @@ * - extractOneLinerFromBody * - pathExistsInternal * - generateSlugInternal - * - filterPlanFiles - * - filterSummaryFiles * - getPhaseFileStats * - readSubdirectories * - timeAgo @@ -30,6 +28,7 @@ const path = require('node:path'); const os = require('node:os'); const coreUtils = require('../gsd-core/bin/lib/core-utils.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); const { cleanup } = require('./helpers.cjs'); // ─── toPosixPath ───────────────────────────────────────────────────────────── @@ -338,43 +337,6 @@ describe('generateSlugInternal', () => { }); }); -// ─── filterPlanFiles ────────────────────────────────────────────────────────── - -describe('filterPlanFiles', () => { - test('returns only PLAN.md and *-PLAN.md files', () => { - const files = ['PLAN.md', '01-PLAN.md', 'SUMMARY.md', 'README.md', 'foo-PLAN.md']; - assert.deepEqual(coreUtils.filterPlanFiles(files), ['PLAN.md', '01-PLAN.md', 'foo-PLAN.md']); - }); - - test('empty array → empty array', () => { - assert.deepEqual(coreUtils.filterPlanFiles([]), []); - }); - - test('no matching files → empty array', () => { - assert.deepEqual(coreUtils.filterPlanFiles(['SUMMARY.md', 'CONTEXT.md']), []); - }); - - test('case-sensitive: plan.md is not matched', () => { - assert.deepEqual(coreUtils.filterPlanFiles(['plan.md', 'Plan.md']), []); - }); -}); - -// ─── filterSummaryFiles ─────────────────────────────────────────────────────── - -describe('filterSummaryFiles', () => { - test('returns only SUMMARY.md and *-SUMMARY.md files', () => { - const files = ['SUMMARY.md', '01-SUMMARY.md', 'PLAN.md', 'foo-SUMMARY.md']; - assert.deepEqual(coreUtils.filterSummaryFiles(files), ['SUMMARY.md', '01-SUMMARY.md', 'foo-SUMMARY.md']); - }); - - test('empty array → empty array', () => { - assert.deepEqual(coreUtils.filterSummaryFiles([]), []); - }); - - test('no matching files → empty array', () => { - assert.deepEqual(coreUtils.filterSummaryFiles(['PLAN.md', 'CONTEXT.md']), []); - }); -}); // ─── readSubdirectories ─────────────────────────────────────────────────────── @@ -493,6 +455,36 @@ describe('getPhaseFileStats', () => { const stats = coreUtils.getPhaseFileStats(tmpDir); assert.strictEqual(stats.hasContext, true); }); + + // ─── #3183 (ADR-3180 Decision 2): scope field + degrade-not-throw ───────── + + test('#3183 row 17: scope is COMPLETE for a readable, empty phase dir', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cu-test-')); + const stats = coreUtils.getPhaseFileStats(tmpDir); + assert.strictEqual(stats.scope, SCOPE.COMPLETE); + }); + + test('#3183 row 15: scope is UNREADABLE and getPhaseFileStats does not throw for a nonexistent dir', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cu-test-')); + const missing = path.join(tmpDir, 'does-not-exist'); + assert.doesNotThrow(() => coreUtils.getPhaseFileStats(missing)); + const stats = coreUtils.getPhaseFileStats(missing); + assert.strictEqual(stats.scope, SCOPE.UNREADABLE); + assert.deepEqual(stats.plans, []); + assert.deepEqual(stats.summaries, []); + assert.strictEqual(stats.hasResearch, false); + assert.strictEqual(stats.hasContext, false); + assert.strictEqual(stats.hasVerification, false); + assert.strictEqual(stats.hasReviews, false); + }); + + test('#3183 row 7 regression: nested plans/PLAN-01.md ONLY is reported (used to report 0)', () => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-cu-test-')); + fs.mkdirSync(path.join(tmpDir, 'plans')); + fs.writeFileSync(path.join(tmpDir, 'plans', 'PLAN-01.md'), '# Plan\n'); + const stats = coreUtils.getPhaseFileStats(tmpDir); + assert.deepEqual(stats.plans, ['plans/PLAN-01.md']); + }); }); // ─── extractOneLinerFromBody ────────────────────────────────────────────────── diff --git a/tests/plan-count-single-owner.test.cjs b/tests/plan-count-single-owner.test.cjs new file mode 100644 index 000000000..277ae24f7 --- /dev/null +++ b/tests/plan-count-single-owner.test.cjs @@ -0,0 +1,944 @@ +/** + * Tests for the live-plan-counting single-owner contract (#3183, ADR-3180). + * + * Covers: + * - src/plan-scan.cts `scanPhasePlans` — plan/summary counting matrix, + * superseded-plan exclusion (#2349), root+nested layouts, exclusion + * filters (-OUTLINE.md, .pre-bounce.md, -PLAN-REVIEW.md), the additive + * `scope` field (COMPLETE/TRUNCATED/UNREADABLE). + * - src/planning-scope.cts `SCOPE` — frozen enum contract. + * - IDENTITY GUARD (ADR-3180 Decision 4c): core-utils.cts's + * `getPhaseFileStats` must return the EXACT `planFiles`/`summaryFiles` + * scanPhasePlans produced — asserted at the consumer's output, not the + * owner's return value, so a future local post-filter at the call site + * fails it. Also asserts `findUnsummarizedPlans` never disagrees with + * `summaryCount` for the same inputs. + * + * Uses helpers.cjs createTempDir/cleanup per CONTRIBUTING.md — never inline + * mkdtemp. IO failure injection uses mock.method(fs, 'readdirSync', ...) + * restored via t.after(), never fs.chmodSync (root bypasses 000 in Docker/CI). + */ + +'use strict'; + +const { test, describe, mock } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); + +const planScan = require('../gsd-core/bin/lib/plan-scan.cjs'); +const { SCOPE } = require('../gsd-core/bin/lib/planning-scope.cjs'); +const coreUtils = require('../gsd-core/bin/lib/core-utils.cjs'); +const { createTempDir, cleanup } = require('./helpers.cjs'); +const { PROBE_TIMEOUT_MS } = require('./helpers/timeouts.cjs'); +const drift = require('../scripts/lint-plan-count-drift.cjs'); + +function writeFile(dir, relName, content) { + const full = path.join(dir, relName); + fs.mkdirSync(path.dirname(full), { recursive: true }); + fs.writeFileSync(full, content); +} + +function planBody() { + return ['# Plan', ''].join('\n'); +} + +function summaryBody() { + return ['# Summary', ''].join('\n'); +} + +function frontmatterBlock(fields) { + const lines = ['---']; + for (const [k, v] of Object.entries(fields)) lines.push(`${k}: ${v}`); + lines.push('---', '', '# Plan', ''); + return lines.join('\n'); +} + +// ─── Scenario matrix (rows 1-14) ────────────────────────────────────────── +// Reused by both the scanPhasePlans matrix tests below AND the identity-guard +// tests (rows 22-23), so the two suites can never see different fixtures. + +const SCENARIOS = [ + { + id: 'row1', + label: 'root layout: 3 plans/3 summaries, none superseded', + build(dir) { + for (const n of ['01', '02', '03']) { + writeFile(dir, `${n}-PLAN.md`, planBody()); + writeFile(dir, `${n}-SUMMARY.md`, summaryBody()); + } + }, + check(scan) { + assert.strictEqual(scan.planCount, 3); + assert.strictEqual(scan.summaryCount, 3); + assert.strictEqual(scan.scope, SCOPE.COMPLETE); + }, + }, + { + id: 'row2', + label: '1 of 3 plans has frontmatter status: superseded -> planCount 2', + build(dir) { + writeFile(dir, '01-PLAN.md', frontmatterBlock({ status: 'superseded' })); + writeFile(dir, '02-PLAN.md', planBody()); + writeFile(dir, '03-PLAN.md', planBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 2); + }, + }, + { + id: 'row3', + label: 'ALL plans superseded -> planCount 0 AND completed TRUE (#2349 invariant)', + build(dir) { + for (const n of ['01', '02', '03']) { + writeFile(dir, `${n}-PLAN.md`, frontmatterBlock({ status: 'superseded' })); + } + }, + check(scan) { + assert.strictEqual(scan.planCount, 0); + assert.strictEqual(scan.completed, true); + }, + }, + { + id: 'row4', + label: 'zero plans authored -> planCount 0, completed FALSE, scope COMPLETE', + build() { /* empty phase dir */ }, + check(scan) { + assert.strictEqual(scan.planCount, 0); + assert.strictEqual(scan.completed, false); + assert.strictEqual(scan.scope, SCOPE.COMPLETE); + }, + }, + { + id: 'row5', + label: 'exactly 1 plan (boundary limit)', + build(dir) { writeFile(dir, '01-PLAN.md', planBody()); }, + check(scan) { assert.strictEqual(scan.planCount, 1); }, + }, + { + id: 'row6', + label: 'exactly 2 plans (boundary limit+1)', + build(dir) { + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, '02-PLAN.md', planBody()); + }, + check(scan) { assert.strictEqual(scan.planCount, 2); }, + }, + { + id: 'row7', + label: 'nested plans/PLAN-01.md ONLY -> planCount 1 (regression: used to report 0)', + build(dir) { writeFile(dir, 'plans/PLAN-01.md', planBody()); }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.ok(scan.planFiles.includes('plans/PLAN-01.md')); + }, + }, + { + id: 'row8', + label: 'mixed root + nested plans -> both counted, no double count', + build(dir) { + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, 'plans/PLAN-02.md', planBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 2); + assert.deepEqual([...scan.planFiles].sort(), ['01-PLAN.md', 'plans/PLAN-02.md']); + }, + }, + { + id: 'row9', + label: '-OUTLINE.md present -> NOT counted', + build(dir) { + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, '01-OUTLINE.md', planBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.ok(!scan.planFiles.includes('01-OUTLINE.md')); + }, + }, + { + id: 'row10', + label: '.pre-bounce.md present -> NOT counted', + build(dir) { + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, '01-PLAN.pre-bounce.md', planBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.ok(!scan.planFiles.includes('01-PLAN.pre-bounce.md')); + }, + }, + { + id: 'row11', + label: '-PLAN-REVIEW.md present -> NOT counted', + build(dir) { + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, '01-PLAN-REVIEW.md', planBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.ok(!scan.planFiles.includes('01-PLAN-REVIEW.md')); + }, + }, + { + id: 'row12', + label: 'stray summary with no matching plan -> summaryCount excludes it', + build(dir) { + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, '01-SUMMARY.md', summaryBody()); + writeFile(dir, '99-GAPCLOSURE-SUMMARY.md', summaryBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.strictEqual(scan.summaryCount, 1); + }, + }, + { + id: 'row13', + label: 'bare PLAN.md <-> SUMMARY.md pairing', + build(dir) { + writeFile(dir, 'PLAN.md', planBody()); + writeFile(dir, 'SUMMARY.md', summaryBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.strictEqual(scan.summaryCount, 1); + }, + }, + { + id: 'row14', + label: 'nested PLAN-01.md <-> SUMMARY-01.md pairing', + build(dir) { + writeFile(dir, 'plans/PLAN-01.md', planBody()); + writeFile(dir, 'plans/SUMMARY-01.md', summaryBody()); + }, + check(scan) { + assert.strictEqual(scan.planCount, 1); + assert.strictEqual(scan.summaryCount, 1); + }, + }, +]; + +// ─── scanPhasePlans matrix (rows 1-14) ──────────────────────────────────── + +describe('scanPhasePlans — counting matrix (#3183 rows 1-14)', () => { + for (const scenario of SCENARIOS) { + test(`${scenario.id}: ${scenario.label}`, (t) => { + const dir = createTempDir('gsd-plan-scan-'); + t.after(() => cleanup(dir)); + scenario.build(dir); + const scan = planScan(dir); + scenario.check(scan); + }); + } +}); + +// ─── IDENTITY GUARD (rows 22-23, ADR-3180 Decision 4c) ──────────────────── + +describe('identity guard: getPhaseFileStats output === scanPhasePlans output (row 22)', () => { + for (const scenario of SCENARIOS) { + test(`${scenario.id}: getPhaseFileStats.plans/.summaries deep-equal scanPhasePlans.planFiles/.summaryFiles`, (t) => { + const dir = createTempDir('gsd-plan-scan-identity-'); + t.after(() => cleanup(dir)); + scenario.build(dir); + const scan = planScan(dir); + const stats = coreUtils.getPhaseFileStats(dir); + assert.deepEqual(stats.plans, scan.planFiles); + assert.deepEqual(stats.summaries, scan.summaryFiles); + }); + } +}); + +describe('identity guard: findUnsummarizedPlans length never disagrees with summaryCount (row 23)', () => { + for (const scenario of SCENARIOS) { + test(`${scenario.id}: findUnsummarizedPlans(...).length === planFiles.length - summaryCount`, (t) => { + const dir = createTempDir('gsd-plan-scan-unsummarized-'); + t.after(() => cleanup(dir)); + scenario.build(dir); + const scan = planScan(dir); + const unsummarized = coreUtils.findUnsummarizedPlans(scan.planFiles, scan.summaryFiles); + assert.strictEqual(unsummarized.length, scan.planFiles.length - scan.summaryCount); + }); + } +}); + +// ─── Superseded frontmatter detection (rows 18-20) ──────────────────────── + +describe('isPlanSuperseded — frontmatter detection edge cases', () => { + test('row18: uppercase SUPERSEDED with surrounding whitespace still detected', (t) => { + const dir = createTempDir('gsd-plan-scan-'); + t.after(() => cleanup(dir)); + writeFile(dir, '01-PLAN.md', ['---', 'status: SUPERSEDED ', '---', '', '# Plan', ''].join('\n')); + writeFile(dir, '02-PLAN.md', planBody()); + const scan = planScan(dir); + assert.strictEqual(scan.planCount, 1); + assert.ok(!scan.planFiles.includes('01-PLAN.md')); + }); + + test('row19: status: supersededX is NOT superseded (no prefix matching)', (t) => { + const dir = createTempDir('gsd-plan-scan-'); + t.after(() => cleanup(dir)); + writeFile(dir, '01-PLAN.md', ['---', 'status: supersededX', '---', '', '# Plan', ''].join('\n')); + writeFile(dir, '02-PLAN.md', planBody()); + const scan = planScan(dir); + assert.strictEqual(scan.planCount, 2); + assert.ok(scan.planFiles.includes('01-PLAN.md')); + }); + + test('row20: CRLF line endings in a superseded plan frontmatter still detected', (t) => { + const dir = createTempDir('gsd-plan-scan-'); + t.after(() => cleanup(dir)); + writeFile(dir, '01-PLAN.md', ['---', 'status: superseded', '---', '', '# Plan', ''].join('\r\n')); + writeFile(dir, '02-PLAN.md', planBody()); + const scan = planScan(dir); + assert.strictEqual(scan.planCount, 1); + assert.ok(!scan.planFiles.includes('01-PLAN.md')); + }); +}); + +// ─── scope field: UNREADABLE / TRUNCATED / COMPLETE independence (rows 15-17) ── + +describe('scope field — UNREADABLE / TRUNCATED / COMPLETE independence', () => { + test('row15: nonexistent phase dir -> scope UNREADABLE, planCount 0, getPhaseFileStats does not throw', () => { + const base = createTempDir('gsd-plan-scan-'); + const missing = path.join(base, 'does-not-exist'); + try { + const scan = planScan(missing); + assert.strictEqual(scan.scope, SCOPE.UNREADABLE); + assert.strictEqual(scan.planFiles.length, 0); + assert.strictEqual(scan.planCount, 0); + + assert.doesNotThrow(() => coreUtils.getPhaseFileStats(missing)); + const stats = coreUtils.getPhaseFileStats(missing); + assert.strictEqual(stats.scope, SCOPE.UNREADABLE); + assert.deepEqual(stats.plans, []); + } finally { + cleanup(base); + } + }); + + test('row16: plans/ exists but readdirSync on it throws -> scope TRUNCATED, root plans still returned', (t) => { + const dir = createTempDir('gsd-plan-scan-'); + writeFile(dir, '01-PLAN.md', planBody()); + const nestedDir = path.join(dir, 'plans'); + fs.mkdirSync(nestedDir); + const originalReaddirSync = fs.readdirSync; + mock.method(fs, 'readdirSync', (p, ...rest) => { + if (p === nestedDir) throw new Error('EACCES: permission denied, scandir plans/'); + return originalReaddirSync.call(fs, p, ...rest); + }); + t.after(() => { + mock.restoreAll(); + cleanup(dir); + }); + + const scan = planScan(dir); + assert.strictEqual(scan.scope, SCOPE.TRUNCATED); + assert.deepEqual(scan.planFiles, ['01-PLAN.md']); + }); + + test('row17: zero plans AND dir readable -> scope COMPLETE, assertably different from row16 TRUNCATED', (t) => { + // Same zero planFiles.length as row16, but readable — must diverge in scope. + const readableEmptyDir = createTempDir('gsd-plan-scan-'); + const truncatedDir = createTempDir('gsd-plan-scan-'); + const nestedDir = path.join(truncatedDir, 'plans'); + fs.mkdirSync(nestedDir); + const originalReaddirSync = fs.readdirSync; + mock.method(fs, 'readdirSync', (p, ...rest) => { + if (p === nestedDir) throw new Error('EACCES: permission denied, scandir plans/'); + return originalReaddirSync.call(fs, p, ...rest); + }); + t.after(() => { + mock.restoreAll(); + cleanup(readableEmptyDir); + cleanup(truncatedDir); + }); + + const completeScan = planScan(readableEmptyDir); + const truncatedScan = planScan(truncatedDir); + + assert.strictEqual(completeScan.planFiles.length, 0); + assert.strictEqual(truncatedScan.planFiles.length, 0); + assert.strictEqual(completeScan.scope, SCOPE.COMPLETE); + assert.strictEqual(truncatedScan.scope, SCOPE.TRUNCATED); + assert.notStrictEqual(completeScan.scope, truncatedScan.scope); + }); +}); + +// ─── Case sensitivity: plan.md/Plan.md vs summary.md/Summary.md (#3183) ─── +// +// isRootPlanFile (src/plan-scan.cts) has a loose fallback — +// `/\.md$/i.test(f) && /PLAN/i.test(f)` — that is case-INSENSITIVE, so +// `plan.md`/`Plan.md` count as plans even though neither matches the +// canonical `-PLAN.md`/`PLAN.md` suffix exactly. isRootSummaryFile has no +// such fallback — `f.endsWith('-SUMMARY.md') || f === 'SUMMARY.md'` is +// case-SENSITIVE — so `summary.md`/`Summary.md` do NOT count as summaries. +// This asymmetry is INTENTIONAL (the loose plan fallback is the point of +// consolidating onto the single owner; summary detection was never given +// the same fallback) — these tests pin the current behavior at both +// altitudes (scanPhasePlans and getPhaseFileStats) so a future change to +// either rule is caught rather than silently drifting. +describe('case sensitivity: plan.md/Plan.md counted, summary.md/Summary.md NOT (#3183 asymmetry)', () => { + test('lowercase plan.md is counted as a plan (loose /PLAN/i fallback is case-insensitive)', (t) => { + const dir = createTempDir('gsd-plan-scan-case-'); + t.after(() => cleanup(dir)); + writeFile(dir, 'plan.md', planBody()); + const scan = planScan(dir); + assert.strictEqual(scan.planCount, 1); + assert.ok(scan.planFiles.includes('plan.md')); + + const stats = coreUtils.getPhaseFileStats(dir); + assert.ok(stats.plans.includes('plan.md')); + }); + + test('mixed-case Plan.md is counted as a plan (loose /PLAN/i fallback is case-insensitive)', (t) => { + const dir = createTempDir('gsd-plan-scan-case-'); + t.after(() => cleanup(dir)); + writeFile(dir, 'Plan.md', planBody()); + const scan = planScan(dir); + assert.strictEqual(scan.planCount, 1); + assert.ok(scan.planFiles.includes('Plan.md')); + + const stats = coreUtils.getPhaseFileStats(dir); + assert.ok(stats.plans.includes('Plan.md')); + }); + + test('lowercase summary.md is NOT counted as a summary (isRootSummaryFile is case-sensitive)', (t) => { + const dir = createTempDir('gsd-plan-scan-case-'); + t.after(() => cleanup(dir)); + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, 'summary.md', summaryBody()); + const scan = planScan(dir); + assert.ok(!scan.summaryFiles.includes('summary.md')); + // Not swept in as a plan either — no "PLAN" substring. + assert.ok(!scan.planFiles.includes('summary.md')); + + const stats = coreUtils.getPhaseFileStats(dir); + assert.ok(!stats.summaries.includes('summary.md')); + }); + + test('mixed-case Summary.md is NOT counted as a summary (isRootSummaryFile is case-sensitive)', (t) => { + const dir = createTempDir('gsd-plan-scan-case-'); + t.after(() => cleanup(dir)); + writeFile(dir, '01-PLAN.md', planBody()); + writeFile(dir, 'Summary.md', summaryBody()); + const scan = planScan(dir); + assert.ok(!scan.summaryFiles.includes('Summary.md')); + assert.ok(!scan.planFiles.includes('Summary.md')); + + const stats = coreUtils.getPhaseFileStats(dir); + assert.ok(!stats.summaries.includes('Summary.md')); + }); +}); + +// ─── SCOPE frozen enum contract (row 21) ────────────────────────────────── + +describe('SCOPE — frozen enum contract', () => { + test('row21: Object.isFrozen(SCOPE) is true, and assigning to a member does not change it', () => { + assert.strictEqual(Object.isFrozen(SCOPE), true); + const before = SCOPE.COMPLETE; + const assigned = Reflect.set(SCOPE, 'COMPLETE', 'mutated-value'); + assert.strictEqual(assigned, false); + assert.strictEqual(SCOPE.COMPLETE, before); + }); +}); + +// ─── summaryCandidates canonical-id coverage (#3183 I001 regression) ────── +// +// The pre-migration bespoke I001 rule (verify.cts, pre-#3183) matched a plan +// carrying a descriptive slug after its - id — e.g. +// `68-01-scaffolding-PLAN.md` — against a summary named only by the bare id +// — `68-01-SUMMARY.md` — via `canonicalPlanStem` (validate.cjs). The +// consolidation onto the single `summaryCandidates` rule in core-utils.cts +// (used by countMatchedSummaries / findUnsummarizedPlans / findOrphanSummaries) +// dropped that candidate, causing 13 remote-runner failures (health-validation +// and phase test suites) — a live-scaffolding-style plan with a matching +// bare-id SUMMARY was misreported as unsummarized. These tests pin the +// restored candidate at both altitudes: the pure core-utils functions AND +// scanPhasePlans's real-directory integration. +describe('summaryCandidates canonical-id form — long PLAN stem matches short SUMMARY stem (#3183)', () => { + test('68-01-scaffolding-PLAN.md pairs with 68-01-SUMMARY.md: countMatchedSummaries/findUnsummarizedPlans agree', () => { + const plans = ['68-01-scaffolding-PLAN.md']; + const summaries = ['68-01-SUMMARY.md']; + assert.strictEqual(coreUtils.countMatchedSummaries(plans, summaries), 1); + assert.deepEqual(coreUtils.findUnsummarizedPlans(plans, summaries), []); + assert.deepEqual(coreUtils.findOrphanSummaries(plans, summaries), []); + }); + + test('same pairing, real directory: scanPhasePlans + validate health emit zero I001', (t) => { + const dir = createTempDir('gsd-plan-count-i001-'); + t.after(() => cleanup(dir)); + writeFile(dir, '68-01-scaffolding-PLAN.md', frontmatterBlock({ wave: 1 })); + writeFile(dir, '68-01-SUMMARY.md', summaryBody()); + const scan = planScan(dir); + assert.strictEqual(scan.planCount, 1); + assert.strictEqual(scan.summaryCount, 1); + assert.strictEqual(scan.completed, true); + assert.deepEqual(coreUtils.findUnsummarizedPlans(scan.planFiles, scan.summaryFiles), []); + }); + + test('COLLISION: two plans sharing one canonical id (differing only by slug) both pair to the ' + + 'SAME single summary — reproduces the pre-migration bespoke rule\'s own collapsing behavior, ' + + 'not a new regression (the old rule populated one Set keyed by canonical stem with no ' + + 'cardinality check)', () => { + const plans = ['68-01-alpha-PLAN.md', '68-01-beta-PLAN.md']; + const summaries = ['68-01-SUMMARY.md']; + // Both plans read as summarized off the one shared summary. + assert.deepEqual(coreUtils.findUnsummarizedPlans(plans, summaries), []); + // countMatchedSummaries counts per-plan matches, so it double-counts the + // single summary here (2), not the number of distinct summary files (1) — + // same modeling limit as the pre-migration rule, preserved intentionally. + assert.strictEqual(coreUtils.countMatchedSummaries(plans, summaries), 2); + assert.deepEqual(coreUtils.findOrphanSummaries(plans, summaries), []); + }); + + test('NEGATIVE: a plan whose canonical stem has no matching summary is still reported unsummarized', () => { + const plans = ['68-02-other-PLAN.md']; + const summaries = ['68-01-SUMMARY.md']; + assert.deepEqual(coreUtils.findUnsummarizedPlans(plans, summaries), ['68-02-other-PLAN.md']); + assert.strictEqual(coreUtils.countMatchedSummaries(plans, summaries), 0); + }); + + test('narrowing: a plan whose base has no extractable - pair does not gain a redundant ' + + 'candidate (canonicalId falls back to the plain base, already covered by the -SUMMARY.md ' + + 'candidate)', () => { + const plans = ['setup-PLAN.md']; + const summaries = ['setup-SUMMARY.md']; + assert.strictEqual(coreUtils.countMatchedSummaries(plans, summaries), 1); + assert.deepEqual(coreUtils.findUnsummarizedPlans(plans, summaries), []); + }); +}); + +// ─── #2893 regression: non-canonical plan filenames must stay non-canonical +// when routed through scanPhasePlans's live-plan set (find-phase / +// phase-plan-index / phases list --type plans naming diagnostic) ────── +// +// scanPhasePlans's `isRootPlanFile` loose `/PLAN/i` fallback is deliberately +// permissive for live-plan COUNTING (see the case-sensitivity describe block +// above). Routing phase.cts's #2893 naming-convention diagnostic through +// `allPlanFiles`/`planFiles` directly (the #3183 migration's first pass) let +// that loose fallback silently recognize a non-canonically-named file (e.g. +// the reporter's own `01-PLAN-01-foundation.md`) as a valid, already-matched +// plan — defeating the diagnostic (no warning, offender listed as if valid). +// `isCanonicalPlanFile` is the strict predicate those three call sites now +// intersect against. Pinned here at the predicate level; the CLI-level +// behavior is covered by tests/phase.test.cjs's `(#2893 parity)` suite. +describe('isCanonicalPlanFile — strict predicate excludes the loose /PLAN/i fallback (#2893 regression)', () => { + test('root canonical forms match', () => { + assert.strictEqual(planScan.isCanonicalPlanFile('03-01-PLAN.md'), true); + assert.strictEqual(planScan.isCanonicalPlanFile('PLAN.md'), true); + }); + + test('nested canonical forms match only when plans/-prefixed', () => { + assert.strictEqual(planScan.isCanonicalPlanFile('plans/PLAN-01.md'), true); + assert.strictEqual(planScan.isCanonicalPlanFile('plans/03-PLAN-01-foo.md'), true); + }); + + test('the #2893 reporter\'s exact non-canonical example does NOT match at root level, even though ' + + 'its basename shape collides with the nested-form regex', () => { + assert.strictEqual(planScan.isCanonicalPlanFile('01-PLAN-01-foundation.md'), false); + assert.strictEqual(planScan.isCanonicalPlanFile('01-PLAN-02-api.md'), false); + }); + + test('loose-fallback-only root matches (lowercase plan.md) do NOT satisfy the strict predicate', () => { + assert.strictEqual(planScan.isCanonicalPlanFile('plan.md'), false); + assert.strictEqual(planScan.isCanonicalPlanFile('Plan.md'), false); + }); +}); + +// ─── findRegexLiteralMdMatch — literal tokenizer regression +// (scripts/lint-plan-count-drift.cjs) +// +// The scanner's "unquoted regex literal that mentions PLAN/SUMMARY and \.md" +// detector used to be a single backtracking regex (REGEX_LITERAL_MD_RE). An +// independent security review found it was STILL defective after a prior +// backslash-exclusion fix: it was cubic (not just exponential) on +// `"/" + "PLAN\\.md".repeat(N)` with no closing `/`, and it structurally +// could not see a character class containing a BARE, unescaped `/` between +// the PLAN/SUMMARY token and `\.md` (e.g. `/SUMMARY[^/]*\.md$/`) — the old +// regex treated that `/` as the literal's terminator and stopped scanning +// before ever reaching `\.md`, silently missing real re-derivation shapes. +// A class holding an ESCAPED `\/` (e.g. `/PLAN[\\/].*\.md$/`) was already +// matched by the old regex's `\\.` alternative, so those shapes are parity +// coverage, not regressions. Both genuine defects share one root cause: +// regex-literal grammar (escapes, and `/` inside `[...]` not terminating) is +// not expressible in a backtracking regex. The fix replaces it with +// `readRegexLiteralAt`/`findRegexLiteralMdMatch`, a deterministic +// single-pass tokenizer with no backtracking at all. +describe('findRegexLiteralMdMatch — literal tokenizer (ReDoS + character-class regression)', () => { + test('findPlanCountDrift completes on backslash-dense and repetition-dense lines (ReDoS regression)', (t) => { + // Catastrophic backtracking is synchronous and cannot be interrupted + // in-process — a hang here would freeze the whole suite instead of + // failing this one test. Spawn a child process with a hard timeout. + // Exercises BOTH pathological shapes the old regex blew up on: + // - exponential: `/\.mdplan` + `\.`.repeat(reps) + `X` (no closing `/`) + // at reps 24 / 28 / 32; + // - cubic: `/` + `PLAN\.md`.repeat(reps) + ` ` (no closing `/`) at reps + // 400 / 800 / 1600. + // These are GROWTH-RATE samples, deliberately doubling, not limit-1/limit/ + // limit+1 boundary coverage — there is no limit here to sit either side + // of, and under the old regex each step multiplied the runtime (~2x per + // rep exponential, ~8x per doubling cubic) so a tokenizer that had + // silently regressed to backtracking blows the child's timeout at the top + // of either ladder. The one real numeric limit in this module, + // MAX_REGEX_LITERAL_LEN, gets true limit-1/limit/limit+1 coverage in the + // readRegexLiteralAt test below. + const dir = createTempDir('gsd-plan-count-drift-redos-'); + t.after(() => cleanup(dir)); + + const guardPath = path.join(__dirname, '..', 'scripts', 'lint-plan-count-drift.cjs'); + const childPath = path.join(dir, 'probe.cjs'); + const childSource = [ + "'use strict';", + "const { findPlanCountDrift } = require(process.argv[2]);", + "for (const reps of [24, 28, 32]) {", + " const attack = '\\\\.'.repeat(reps);", + " const line = `const RE = /\\\\.mdplan${attack}X; names.filter(n => RE.test(n));`;", + " findPlanCountDrift(line, 'src/probe.cts');", + "}", + "for (const reps of [400, 800, 1600]) {", + " const attack = 'PLAN\\\\.md'.repeat(reps);", + " const line = `const RE = /${attack} ; names.filter(n => RE.test(n));`;", + " findPlanCountDrift(line, 'src/probe.cts');", + "}", + "process.stdout.write('done');", + ].join('\n'); + fs.writeFileSync(childPath, childSource); + + try { + const output = execFileSync(process.execPath, [childPath, guardPath], { + encoding: 'utf8', + timeout: PROBE_TIMEOUT_MS, + }); + assert.strictEqual(output, 'done'); + } catch (err) { + // `code === 'ETIMEDOUT'` is the canonical execFileSync timeout signal; + // on darwin the kill yields signal 'SIGTERM' with `killed` undefined, + // so all three are checked rather than relying on any one platform's + // spelling. + if (err.code === 'ETIMEDOUT' || err.signal === 'SIGTERM' || err.killed) { + assert.fail( + 'findPlanCountDrift did not complete within PROBE_TIMEOUT_MS: the child probe ' + + 'process was killed instead of exiting. Catastrophic backtracking (ReDoS) in ' + + 'the literal tokenizer is the expected cause; an externally killed child would ' + + 'also land here.', + ); + } + throw err; + } + }); + + test('findRegexLiteralMdMatch semantics: matches genuine plan/summary literals, ' + + 'catches path-separator character-class shapes, rejects near-misses and attack lines', () => { + const matchOf = (line) => drift.findRegexLiteralMdMatch(line); + + // Positive: genuine plan/summary regex literals must still match. + assert.strictEqual( + matchOf("if (/-PLAN\\.md$/.test(name)) return true;"), + '/-PLAN\\.md$/', + ); + assert.strictEqual( + matchOf('names.filter((n) => /^PLAN-\\d+.*\\.md$/i.test(n));'), + '/^PLAN-\\d+.*\\.md$/i', + ); + assert.strictEqual( + matchOf('names.filter((n) => /-SUMMARY-\\d+.*\\.md$/i.test(n));'), + '/-SUMMARY-\\d+.*\\.md$/i', + ); + assert.strictEqual( + matchOf('names.filter((n) => /\\.md.*SUMMARY/.test(n));'), + '/\\.md.*SUMMARY/', + ); + + // Character classes containing an ESCAPED `\/` pair. The old backtracking + // regex also matched these (its `\\.` alternative consumed the `\/`), so + // they are NOT regression cases — they are parity coverage proving the + // tokenizer did not LOSE behaviour when it replaced the regex. + assert.strictEqual( + matchOf(String.raw`files.filter(f => /PLAN[\\/].*\.md$/.test(f));`), + String.raw`/PLAN[\\/].*\.md$/`, + ); + assert.strictEqual( + matchOf(String.raw`files.filter(f => /PLAN[\\/]\d+\.md$/.test(f));`), + String.raw`/PLAN[\\/]\d+\.md$/`, + ); + assert.strictEqual( + matchOf(String.raw`files.filter(f => /SUMMARY[\\/][^x]*\.md$/.test(f));`), + String.raw`/SUMMARY[\\/][^x]*\.md$/`, + ); + assert.strictEqual( + matchOf(String.raw`files.filter(f => /\.md[\\/]SUMMARY/.test(f));`), + String.raw`/\.md[\\/]SUMMARY/`, + ); + + // Character classes containing a BARE, UNESCAPED `/`. These are the real + // regression cases: the old regex treated that `/` as the literal's + // terminator, so it stopped scanning before reaching `\.md` and MISSED + // every one of them — verified against the parent-commit blob, where all + // four return null. A guard that cannot see `/SUMMARY[^/]*\.md$/` is + // blind to an ordinary path-excluding filter. + assert.strictEqual( + matchOf(String.raw`files.filter(f => /PLAN[/\\].*\.md$/.test(f));`), + String.raw`/PLAN[/\\].*\.md$/`, + ); + assert.strictEqual( + matchOf(String.raw`files.filter(f => /PLAN[a/b]\.md$/.test(f));`), + String.raw`/PLAN[a/b]\.md$/`, + ); + assert.strictEqual( + matchOf(String.raw`files.filter(f => /SUMMARY[^/]*\.md$/.test(f));`), + String.raw`/SUMMARY[^/]*\.md$/`, + ); + assert.strictEqual( + matchOf(String.raw`files.filter(f => /PLAN[/]\.md$/.test(f));`), + String.raw`/PLAN[/]\.md$/`, + ); + + // Positive: an escaped slash (`\/`) inside the literal must not + // terminate it early — the whole literal is returned, not a truncated + // fragment up to the escaped `/`. + assert.strictEqual( + matchOf(String.raw`names.filter(n => /PLAN\/\d+\.md$/.test(n));`), + String.raw`/PLAN\/\d+\.md$/`, + ); + + // Negative: \.md with no PLAN/SUMMARY token in the literal. + assert.strictEqual(matchOf('names.filter((n) => /\\.md$/.test(n));'), null); + // Negative: PLAN with no escaped \.md token in the literal. + assert.strictEqual(matchOf('names.filter((n) => /PLAN/.test(n));'), null); + // Negative: the backslash-dense ReDoS attack construction (no closing + // `/`, so the literal never resolves). + const attack = '\\.'.repeat(8); + const attackLine = `const RE = /\\.mdplan${attack}X; names.filter(n => RE.test(n));`; + assert.strictEqual(matchOf(attackLine), null); + }); + + // Direct coverage of readRegexLiteralAt — the tokenizer's single-pass + // scan primitive. Replaces the deleted `!source.includes('[^/')` + // structural check, which was a gameable substring test (respelling the + // class as `[^\r\n/]` would still pass it while remaining exponential) + // inspecting a constant (REGEX_LITERAL_MD_RE) that no longer exists. + describe('readRegexLiteralAt', () => { + test('an unterminated literal (no closing `/`) returns null', () => { + assert.strictEqual( + drift.readRegexLiteralAt('/PLAN and no closing slash at all', 0), + null, + ); + }); + + test('MAX_REGEX_LITERAL_LEN boundary: limit-1 reads, limit reads, limit+1 returns null', () => { + // Derived from the exported constant so the boundary cannot silently + // drift from it (a missing/undefined export must fail loudly, not + // vacuously pass a boundary computed from `undefined`). + const MAX = drift.MAX_REGEX_LITERAL_LEN; + assert.ok(Number.isInteger(MAX) && MAX > 10, `MAX_REGEX_LITERAL_LEN must be exported as an integer > 10, got ${MAX}`); + + // Total literal length (both delimiters included) = contentLen + 2. + const build = (contentLen) => '/' + 'a'.repeat(contentLen) + '/'; + const underBound = build(MAX - 3); // total length MAX-1 — limit-1 + const atBound = build(MAX - 2); // total length MAX — limit (MAX_REGEX_LITERAL_LEN) + const overBound = build(MAX - 1); // total length MAX+1 — limit+1 + + assert.strictEqual(drift.readRegexLiteralAt(underBound, 0)?.text, underBound); + assert.strictEqual(drift.readRegexLiteralAt(atBound, 0)?.text, atBound); + assert.strictEqual(drift.readRegexLiteralAt(overBound, 0), null); + }); + + test('a `/` inside a `[...]` character class does not terminate the literal', () => { + const line = '/a[/]b/'; + assert.strictEqual(drift.readRegexLiteralAt(line, 0)?.text, line); + }); + + test('an escaped `\\/` does not terminate the literal', () => { + const line = String.raw`/a\/b/`; + assert.strictEqual(drift.readRegexLiteralAt(line, 0)?.text, line); + }); + + test('trailing flags are included in the returned literal text', () => { + const line = '/foo/gi'; + assert.strictEqual(drift.readRegexLiteralAt(line, 0)?.text, line); + }); + }); + + // scanRepo's `walk` follows symlinks (via fs.statSync) to close the + // Dirent-classification evasion documented at `walk`'s definition, which + // means it must also confine itself to the repo root — an unconfined + // symlink follow would let a fork PR read and leak arbitrary files outside + // the repo into the CI-log report. `{ skip }` mirrors the win32 symlink + // guard used elsewhere in this repo (tests/phase.test.cjs) — symlink + // creation needs elevated privilege on Windows CI runners. + describe('walk — symlink handling and root confinement', () => { + const skip = process.platform === 'win32' ? 'symlink creation needs privilege on Windows' : false; + + function violatingLine() { + return "module.exports.isPlan = (f) => f.endsWith('-PLAN.md');\n"; + } + + test('(a) a symlinked .cts INSIDE the root IS scanned, reported under its canonical real path', { skip }, (t) => { + const root = createTempDir('gsd-plan-count-drift-root-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.mkdirSync(path.join(root, 'vendor'), { recursive: true }); + const realFile = path.join(root, 'vendor', 'real-target.cts'); + fs.writeFileSync(realFile, violatingLine()); + fs.symlinkSync(realFile, path.join(root, 'src', 'linked.cts')); + + const violations = drift.scanRepo(root); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].file, path.join('vendor', 'real-target.cts')); + }); + + test('(b) a symlinked .cts pointing OUTSIDE the root is NOT scanned: zero violations reported', { skip }, (t) => { + const root = createTempDir('gsd-plan-count-drift-root-'); + const outside = createTempDir('gsd-plan-count-drift-outside-'); + t.after(() => cleanup(root)); + t.after(() => cleanup(outside)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + const outsideFile = path.join(outside, 'evil.cts'); + fs.writeFileSync(outsideFile, violatingLine()); + fs.symlinkSync(outsideFile, path.join(root, 'src', 'evil.cts')); + + const violations = drift.scanRepo(root); + assert.strictEqual(violations.length, 0); + }); + + test('(c) a directory symlink pointing OUTSIDE the root is not descended into', { skip }, (t) => { + const root = createTempDir('gsd-plan-count-drift-root-'); + const outside = createTempDir('gsd-plan-count-drift-outside-'); + t.after(() => cleanup(root)); + t.after(() => cleanup(outside)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + const outsideDir = path.join(outside, 'dir'); + fs.mkdirSync(outsideDir, { recursive: true }); + fs.writeFileSync(path.join(outsideDir, 'evil.cts'), violatingLine()); + fs.symlinkSync(outsideDir, path.join(root, 'src', 'outdir'), 'dir'); + + const violations = drift.scanRepo(root); + assert.strictEqual(violations.length, 0); + }); + + test('(d) a symlink CYCLE terminates rather than recursing forever, and dedupes the real file', { skip }, (t) => { + const root = createTempDir('gsd-plan-count-drift-root-'); + t.after(() => cleanup(root)); + const srcDir = path.join(root, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.writeFileSync(path.join(srcDir, 'real.cts'), violatingLine()); + fs.symlinkSync(srcDir, path.join(srcDir, 'loop'), 'dir'); + + const violations = drift.scanRepo(root); + assert.strictEqual(violations.filter((v) => v.file === path.join('src', 'real.cts')).length, 1); + }); + + test('(e) a BROKEN symlink is skipped without throwing', { skip }, (t) => { + const root = createTempDir('gsd-plan-count-drift-root-'); + t.after(() => cleanup(root)); + const srcDir = path.join(root, 'src'); + fs.mkdirSync(srcDir, { recursive: true }); + fs.symlinkSync(path.join(srcDir, '.does-not-exist.cts'), path.join(srcDir, 'broken.cts')); + + assert.doesNotThrow(() => drift.scanRepo(root)); + assert.strictEqual(drift.scanRepo(root).length, 0); + }); + + test('(f) two symlinks inside the root to the SAME real file yield ONE entry, not two', { skip }, (t) => { + const root = createTempDir('gsd-plan-count-drift-root-'); + t.after(() => cleanup(root)); + fs.mkdirSync(path.join(root, 'src'), { recursive: true }); + fs.mkdirSync(path.join(root, 'vendor'), { recursive: true }); + const realFile = path.join(root, 'vendor', 'shared-target.cts'); + fs.writeFileSync(realFile, violatingLine()); + fs.symlinkSync(realFile, path.join(root, 'src', 'link1.cts')); + fs.symlinkSync(realFile, path.join(root, 'src', 'link2.cts')); + + const violations = drift.scanRepo(root); + assert.strictEqual(violations.length, 1); + assert.strictEqual(violations[0].file, path.join('vendor', 'shared-target.cts')); + }); + }); +}); + +// Direct coverage of isInsideRoot — a reviewer mutating its body to a bare +// `realPath.startsWith(realRoot)` left every existing test above still +// passing, because none of those temp fixtures ever produce a SIBLING +// directory sharing the root's name as a strict prefix. This describe closes +// that gap so the mutant is actually killed (relevant to the repo's 80% +// Stryker mutation gate). +describe('isInsideRoot', () => { + test('the root itself is inside', () => { + const root = path.join(path.sep, 'tmp', 'repo'); + assert.strictEqual(drift.isInsideRoot(root, root), true); + }); + + test('a child path is inside', () => { + const root = path.join(path.sep, 'tmp', 'repo'); + const child = path.join(root, 'src', 'file.cts'); + assert.strictEqual(drift.isInsideRoot(child, root), true); + }); + + test('a SIBLING whose name is the root plus a suffix is NOT inside', () => { + // This is the case that kills the `startsWith(realRoot)` mutant: a bare + // prefix check wrongly admits `/tmp/repo-evil/x.cts` as "inside" + // `/tmp/repo`, because the string "/tmp/repo-evil/x.cts" does start with + // the string "/tmp/repo". The real implementation requires an exact + // match or a root+separator prefix, so this must be rejected. + const root = path.join(path.sep, 'tmp', 'repo'); + const sibling = path.join(path.sep, 'tmp', 'repo-evil', 'x.cts'); + assert.strictEqual(drift.isInsideRoot(sibling, root), false); + }); + + test('a parent path is not inside', () => { + const root = path.join(path.sep, 'tmp', 'repo', 'src'); + const parent = path.join(path.sep, 'tmp', 'repo'); + assert.strictEqual(drift.isInsideRoot(parent, root), false); + }); + + test('an unrelated absolute path is not inside', () => { + const root = path.join(path.sep, 'tmp', 'repo'); + const unrelated = path.join(path.sep, 'var', 'other', 'x.cts'); + assert.strictEqual(drift.isInsideRoot(unrelated, root), false); + }); +}); + +// Direct coverage of sanitizeForReport — both a matched `found` fragment and +// a reported file path are attacker-controlled source text on a fork PR, and +// both are written straight to a CI log; this asserts the escaping contract +// documented at the function's definition (C0/C1 control bytes plus the +// listed bidi/line-separator/zero-width codepoints), and that ordinary +// printable text — including the regex-literal punctuation this module +// itself tokenizes — passes through completely unchanged. +describe('sanitizeForReport', () => { + test('escapes a C0 control byte (ESC 0x1b)', () => { + assert.strictEqual(drift.sanitizeForReport(String.fromCharCode(0x1b)), '\\x1b'); + }); + + test('escapes DEL (0x7f)', () => { + assert.strictEqual(drift.sanitizeForReport(String.fromCharCode(0x7f)), '\\x7f'); + }); + + test('escapes a C1 byte (0x9b)', () => { + assert.strictEqual(drift.sanitizeForReport(String.fromCharCode(0x9b)), '\\x9b'); + }); + + test('escapes ZERO WIDTH SPACE (U+200B)', () => { + assert.strictEqual(drift.sanitizeForReport('\u200B'), '\\u200b'); + }); + + test('escapes LINE SEPARATOR (U+2028)', () => { + assert.strictEqual(drift.sanitizeForReport('\u2028'), '\\u2028'); + }); + + test('escapes PARAGRAPH SEPARATOR (U+2029)', () => { + assert.strictEqual(drift.sanitizeForReport('\u2029'), '\\u2029'); + }); + + test('escapes RIGHT-TO-LEFT OVERRIDE (U+202E)', () => { + assert.strictEqual(drift.sanitizeForReport('\u202E'), '\\u202e'); + }); + + test('ordinary printable text, including regex-literal punctuation, is unchanged', () => { + const text = String.raw`/-PLAN\.md$/i names.filter([].$^*+ )`; + assert.strictEqual(drift.sanitizeForReport(text), text); + }); +});