golem15/msd-core
4
0
Fork 0
Code Issues 1 Pull Requests Actions 1 Packages Projects Releases Wiki Activity

refactor(#3183): route live-plan counting through scanPhasePlans (#3199)

Browse Source 
* refactor(#3183): route live-plan counting through scanPhasePlans

scanPhasePlans becomes the sole owner of the live-plan derivation. Twenty-one
independent re-derivations across seven modules now route through it, and
scripts/lint-plan-count-drift.cjs reports zero, scanning the whole repo rather
than an allowlist (ADR-3180 Decision 4a).

The epic scoped this at three copies. A whole-repo guard found twenty-six sites
across nine files, so Phase 1 absorbs every live-plan re-derivation and Phase 3
narrows to window plus sentinel enumeration.

Two sites are exempt with a documented reason rather than a bare allowlist:
audit.cts scans one quick task's own directory for a single completion record,
and gsd2-import.cts reads a foreign GSD-2 tasks/ layout during a one-time
import. Neither is a phase directory.

scanPhasePlans gains allPlanFiles (pre-supersession) alongside planFiles so one
owner answers both questions: verify.cts's numbering-gap check wants every plan
on disk, its pairing check wants the live set. Both fields are additive.

Highest-severity fix: cmdPhasePlanIndex, which feeds execute-phase wave
scheduling, was scheduling status:superseded plans into waves and reporting zero
plans for the post-#3139 nested layout.

filterPlanFiles and filterSummaryFiles are deleted; getPhaseFileStats orphaned
them and only their own tests still called them.

New leaf module src/planning-scope.cts carries the frozen SCOPE discriminator,
with its six-gate ripple closed: gitignore, inventory manifest, INVENTORY.md and
the CONTEXT.md glossary.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma

* docs(#3183): amend ADR-3180 for the Phase 1/3 boundary re-slice

The contract held; the phase boundary did not. The whole-repo drift guard found
26 re-derivations across 9 files against the epic's estimate of 3, and
cmdProgressRender re-derives both enumeration and plan counting on adjacent
lines, so DW4 was unsatisfiable within Phase 1's original file scope.

Records the amended scope, scanPhasePlans's new allPlanFiles field,
findOrphanSummaries, the two documented exemptions, the re-derived Tier-2
table, and the describeNonCanonicalPlans trap for later phases.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma

* fix(#3183): complete the canonical pairing rule and gate the naming diagnostic

The remote runner went red with 13 deterministic failures on both lanes,
and they were right: replacing verify.cts's canonicalPlanStem pairing with
summaryCandidates dropped a case the bespoke rule covered. A plan carrying a
descriptive slug after its id (68-01-scaffolding-PLAN.md) pairs with its
canonical-stem summary (68-01-SUMMARY.md), and summaryCandidates generated no
such candidate, so the plan read unsummarized.

The fix is to complete the one rule rather than restore a second:
summaryCandidates gains a canonical-id candidate, narrowed to fire only when an
id pair was actually extracted. countMatchedSummaries, findUnsummarizedPlans
and findOrphanSummaries all inherit it. The two-plans-one-summary collision
behaviour of the original rule is preserved deliberately and documented in
place.

Second defect, independently root-caused while verifying: routing the #2893
naming diagnostic through scanPhasePlans exposed it to the loose /PLAN/i
fallback, which is correct for counting and wrong for a naming check — a
non-canonically-named file was accepted as a valid plan and the diagnostic
went silent. cmdPhasesList, cmdFindPhase and cmdPhasePlanIndex now intersect
with a strict isCanonicalPlanFile predicate before reporting names.

Same class as the describeNonCanonicalPlans trap already recorded in ADR-3180:
a question about file naming wants the physical, strictly-matched set; only a
question about outstanding work wants the live set.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma

* chore(#3183): register planning-scope.cjs in the eslint migration list

tests/repo-invariants.test.cjs asserts every bin/lib/*.cjs is linted xor
ignored per its ADR-457 migration state. The new planning-scope module closed
five of the six .cts ripple gates - gitignore, inventory manifest, INVENTORY.md
and the CONTEXT.md glossary - but not eslint, because that one is enforced by a
test rather than by lint:ci, so the local pipeline stayed green while it was
missing.

Generated from src/planning-scope.cts, so the .cjs is ignored and the .cts is
linted, matching every other migrated module.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma

* fix(#3183): replace the plan-count drift detector with a literal tokenizer

CodeQL reported 4 high-severity js/redos alerts on REGEX_LITERAL_MD_RE, the
backtracking regex that finds "a regex literal mentioning PLAN/SUMMARY and an
escaped \.md". Five review rounds found it had two defects, not one:

  - EXPONENTIAL, then CUBIC. Its "any char" atom `(?:\\.|[^/\r\n])` let a `\.`
    pair be consumed either as one escape or as two class characters, which is
    exponential backtracking: 27,464ms on `"/\.mdplan" + "\.".repeat(28) + "X"`.
    Excluding `\` from the class killed that but left a cubic path — 23ms at
    N=200, 172ms at N=400, 1362ms at N=800 on `"/" + "PLAN\.md".repeat(N)` with
    no closing `/`. This guard is the last stage of `npm run lint:ci`, which CI
    runs on fork pull requests, so a crafted src/*.cts could stall the job.
  - A DETECTION HOLE. A character class holding a bare, unescaped `/` — e.g.
    `/SUMMARY[^/]*\.md$/`, an ordinary path-excluding filter — terminated the
    literal at that `/`, so the scan never reached `\.md` and the guard missed
    it entirely. (Classes holding an ESCAPED `\/` were already matched; the
    tests cover those separately as parity, not as regressions.)

Both defects have one root cause: regex-literal grammar — `\x` escapes, and
`/` inside `[...]` not terminating — is not expressible in a backtracking
regex. So the detector is now a tokenizer, not a regex.

readRegexLiteralAt reads the literal at a given `/` in a single left-to-right
pass with no backtracking, treating escapes as two-character units and
suppressing the `/` terminator inside a character class. findRegexLiteralMdMatch
restarts it at every `/` on the line, preserving the old "find anywhere"
behaviour; MAX_REGEX_LITERAL_LEN (400) bounds each read — including the
trailing-flag scan — which keeps the whole-line cost linear.

Results: cubic shape flat at 0.06-0.39ms out to N=3200 (25KB), exponential
shape 0.01ms at 28 reps and 0.00ms at 64, and the bare-`/` class shapes are now
caught. Differential against the old regex over 28,474 lines (those matching
FILENAME_TEST_RE but not PLAN_SUMMARY_LITERAL_RE, across src/tests/scripts/
gsd-core/bin/eslint-rules, excluding 265 lines with >6 backslashes on which the
old regex hangs): 6 differences, all the tokenizer returning the fuller or
newly-correct literal, 0 old-only misses. The `\.md` token stays
case-insensitive, matching the `/i` the old regex carried.

Also closes three holes in the same new file:

  - walk() tested entry.isFile(), false for a symlink, so a symlinked
    src/*.cts was silently unscanned — an evasion of a guard whose stated
    principle (ADR-3180 Decision 4a) is whole-repo discovery with no allowlist.
    It now resolves symlinks, but confined: file links must resolve inside the
    repo root, directory links inside the scanned dir itself. Every sibling
    drift guard in scripts/ uses the Dirent classification and never follows
    links, so following them unconfined would have made this the only linter
    able to read outside the tree — on fork PRs an arbitrary out-of-repo read
    whose matched fragments reach a public CI log. The narrower directory rule
    additionally stops `src/up -> ..` from sweeping the whole repo, and the
    skip list is now checked against resolved paths so `src/g -> ../.git`
    cannot reach .git/** or node_modules/**. Real paths are de-duplicated and
    files reported canonically, so a symlink alias cannot shift which
    FUNCTION_SCOPED_EXEMPTIONS key applies.
  - Both the reported fragment and the reported FILE PATH are attacker-
    controlled source text written straight to a CI log, and git permits
    control bytes in a filename. Both are now escaped — C0/C1/DEL plus the
    bidi and zero-width controls — so a crafted literal or filename cannot
    recolour the log, overwrite a line with CR, or fabricate a line that looks
    like this guard's own success output.

Regression coverage in tests/plan-count-single-owner.test.cjs: a child-process
probe over both pathological shapes (catastrophic backtracking is synchronous
and would freeze the suite rather than fail one test), the bare-`/` class
shapes verified to fail against the parent-commit blob, root-confinement tests
covering the outside-file, outside-directory, cycle, broken-link and duplicate
cases, direct isInsideRoot coverage including the sibling-prefix case that a
bare startsWith would let through, sanitizeForReport coverage, and
limit-1/limit/limit+1 coverage of MAX_REGEX_LITERAL_LEN derived from the
exported constant. The earlier structural assertion was dropped — it checked
for the substring `[^/`, which respelling the class as `[^\r\n/]` defeats
while staying exponential.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma

* chore(#3183): backfill changeset PR number

Restores b77931869, which a force-push during the ReDoS remediation dropped.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_012qYy4ZWif3sscQyMsup6Ma

---------

Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-08-08 01:20:55 -04:00
committed by GitHub
parent 409da3223c
commit 343835facc
24 changed files with 2084 additions and 202 deletions
Download Patch File Download Diff File Expand all files Collapse all files

120
src/phase.cts
View File

@@ -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<string, unknown> = {
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) {