* chore(#2992): widen the when= grammar and key the section manifest per workflow Epic #1671 Phase 6.1. Two blockers stopped the fragment model reaching any file beyond execute-phase.md: the when= vocabulary was frozen at 4 atoms (3 execute-phase-specific), and the section manifest was single-workflow by construction with 'execute-phase' hardcoded into buildSectionManifestField. - widen WHEN_VOCABULARY 4 -> 14 via a coordinated ADR-1671 amendment; the grammar stays CLOSED (one atom, no operators, negation or nesting) and WHEN_PREDICATES stays a hand-written literal map, never deriving a predicate from its atom string - InvocationFacts gains flags: ReadonlySet<string> plus three computed state booleans; add the missing reverse vocabulary/predicate parity guard - key the manifest artifact per workflow; a stale flat {sections:[...]} artifact now fails shape validation instead of being misattributed - wire the field into six init entry points and parse the flags each needs An atom ships only with both a real consuming section and a fact the init seam actually computes. Six surveyed atoms are withheld because their workflows have no dedicated init entry point; an atom without a computed fact evaluates false forever and silently disables its own section. Fixes a defect found while wiring: parseNamedArgs always materializes a boolean flag key, so folding its false into the absent sentinel is required or every flag reads as present and gating is silently always-on. Also resolves ADR-1671:194 by measurement: --mvp stays unmarkable, because its interleaved sites are always-run flag resolution and a ~340 byte block that already delegates lazily. Refs #2992 * fix(#2992): treat any falsy option value as an absent flag and reject unsafe manifest read paths Findings from two orthogonal reviews (Claude /code-review + an isolated adversarial pass); both independently reproduced the first one. - MAJOR: the flags-builder treated only `undefined` as absent, but parseNamedArgs yields `null` for an absent value-flag and `false` for an absent boolean-flag, so `--granularity` read as present on every plan-phase invocation. Fixed at the root: a flag is present iff its option value is truthy. The six per-handler `|| undefined` folds are now redundant and removed, which also closes the duplicate-translation and missed-onboard-handler findings. - MAJOR: state:needs-codebase-map had zero coverage. Added unit, property and real-CLI integration tests. - MINOR: reject absolute, UNC/drive and `..`-traversing `read` paths in the manifest, degrading the whole load to null like every other shape violation. Verified: `/etc/passwd` previously reached section_manifest.read. - MINOR: corrected a stale "4 to 20" doc comment; the vocabulary is 14. Refs #2992 * test(#2992): update the generator suite for the per-workflow manifest shape The remote matrix went red with 5 unique failures, identical on linux-node22 and linux-node24, all in tests/gen-section-manifest.test.cjs. Re-keying the artifact to {workflows:{...}} left this suite asserting the old flat {sections:[...]} shape; nothing else in the tree still does. - three tests read manifest.sections.length, now undefined; retargeted at workflows.<name> with their original intent preserved (a fenced or loop-host marker still asserts NO section is produced, not merely a changed count) - the stale-manifest test wrote its fixture in the OLD shape, so it tripped shape validation and stopped exercising staleness at all. Its fixture is now valid-but-mismatched so FAIL_STALE is genuinely reached again. - added the coverage that exposed: a pre-6.1 flat artifact must report FAIL_MANIFEST_MALFORMED_SHAPE. That is the real upgrade path for an installed tree and nothing covered it. Refs #2992 * chore(#2992): backfill changeset pr number to 3013 --------- Co-authored-by: sim <sim@local>
This commit is contained in:
Tom Boucher
189
src/init.cts
189
src/init.cts
@@ -343,25 +343,58 @@ interface ManifestSection extends sectionManifest.SelectableSection {
|
||||
}
|
||||
|
||||
/**
|
||||
* Loads and shape-validates the generated section manifest's `sections` array.
|
||||
* Returns `null` — never throws — when the artifact is missing, unreadable,
|
||||
* malformed JSON, or valid JSON of the wrong shape (matrix rows 13/14/57/58:
|
||||
* a missing derived artifact must not break dispatch).
|
||||
* Defense-in-depth shape check for a manifest entry's `read` field, which is
|
||||
* documented as a POSIX-normalized, repo-root-RELATIVE path (never a
|
||||
* filesystem escape). Rejects any absolute path (POSIX leading `/`, a
|
||||
* Windows drive prefix like `C:\`/`C:/`, or a Windows UNC/rooted path
|
||||
* starting with `\`) and any path containing a `..` segment (checked on
|
||||
* BOTH separators — the artifact is generated as POSIX-normalized, but this
|
||||
* validates the raw field defensively rather than trusting that invariant).
|
||||
* `false` here is the only accept path in {@link loadSectionManifestSections};
|
||||
* a `true` degrades the WHOLE load to `null`, same as every other shape
|
||||
* violation — never throws, never partially loads.
|
||||
*/
|
||||
function loadSectionManifestSections(): ManifestSection[] | null {
|
||||
function isUnsafeManifestReadPath(readPath: string): boolean {
|
||||
if (readPath.startsWith('/') || readPath.startsWith('\\')) return true;
|
||||
if (/^[a-zA-Z]:[\\/]/.test(readPath)) return true;
|
||||
return readPath.split(/[\\/]/).includes('..');
|
||||
}
|
||||
|
||||
/**
|
||||
* Loads and shape-validates the generated section manifest, then returns the
|
||||
* document-order section array for exactly one named `workflow` (#2992 Phase
|
||||
* 6.1: the artifact is now `{ workflows: { <name>: [...] } }`, keyed by
|
||||
* `.md` basename — see `scripts/gen-section-manifest.cjs`). Returns `null`
|
||||
* — never throws — when the artifact is missing, unreadable, malformed
|
||||
* JSON, valid JSON of the wrong shape (INCLUDING the pre-6.1 flat
|
||||
* `{sections:[...]}` shape, which must never be mis-attributed to any
|
||||
* workflow — design row C4), or when `workflow` has no key in `workflows`.
|
||||
* `Object.hasOwn` guards the key lookup so a hostile workflow name
|
||||
* (`constructor`, `toString`, `__proto__`) can never resolve via the
|
||||
* prototype chain instead of a genuine own key. Each entry's `read` field is
|
||||
* additionally validated by {@link isUnsafeManifestReadPath} (rejects an
|
||||
* absolute path or a `..` segment) — a single unsafe entry degrades the
|
||||
* WHOLE load to `null`, all-or-nothing like every other shape violation.
|
||||
*/
|
||||
function loadSectionManifestSections(workflow: string): ManifestSection[] | null {
|
||||
try {
|
||||
const raw = fs.readFileSync(_sectionManifestCandidatePath(), 'utf8');
|
||||
const parsed: unknown = JSON.parse(raw);
|
||||
if (parsed === null || typeof parsed !== 'object') return null;
|
||||
const sections = (parsed as Record<string, unknown>)['sections'];
|
||||
if (parsed === null || typeof parsed !== 'object' || Array.isArray(parsed)) return null;
|
||||
const workflows = (parsed as Record<string, unknown>)['workflows'];
|
||||
if (workflows === null || typeof workflows !== 'object' || Array.isArray(workflows)) return null;
|
||||
if (!Object.hasOwn(workflows, workflow)) return null;
|
||||
const sections = (workflows as Record<string, unknown>)[workflow];
|
||||
if (!Array.isArray(sections)) return null;
|
||||
for (const section of sections) {
|
||||
const readValue = (section as Record<string, unknown> | null)?.['read'];
|
||||
if (
|
||||
!section ||
|
||||
typeof section !== 'object' ||
|
||||
typeof (section as Record<string, unknown>)['id'] !== 'string' ||
|
||||
typeof (section as Record<string, unknown>)['when'] !== 'string' ||
|
||||
typeof (section as Record<string, unknown>)['read'] !== 'string'
|
||||
typeof readValue !== 'string' ||
|
||||
isUnsafeManifestReadPath(readValue)
|
||||
) {
|
||||
return null;
|
||||
}
|
||||
@@ -405,6 +438,70 @@ function detectHasPriorPhases(cwd: string, phaseInfo: Record<string, unknown> |
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Strict-boolean, bounded, non-throwing read of a dotted key path from
|
||||
* `.planning/config.json` (design rows D7-D10): absent file, unreadable
|
||||
* file (fs error), malformed JSON, a non-object intermediate segment, or a
|
||||
* present-but-non-boolean value (e.g. the string `"true"`) all degrade to
|
||||
* `false` — strict `=== true`, never coerced, mirrors `detectHasPriorPhases`'s
|
||||
* degrade-to-false discipline. `keyPath` is always a fixed literal supplied
|
||||
* by this module, never attacker/user input, so a plain bracket traversal
|
||||
* carries no prototype hazard here.
|
||||
*/
|
||||
function readConfigJsonBoolean(cwd: string, keyPath: readonly string[]): boolean {
|
||||
try {
|
||||
const raw = fs.readFileSync(path.join(planningDir(cwd), 'config.json'), 'utf8');
|
||||
let cursor: unknown = JSON.parse(raw);
|
||||
for (const segment of keyPath) {
|
||||
if (cursor === null || typeof cursor !== 'object' || Array.isArray(cursor)) return false;
|
||||
cursor = (cursor as Record<string, unknown>)[segment];
|
||||
}
|
||||
return cursor === true;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* `state:phase-mvp-mode` ground truth (design doc §Behavior table: ROADMAP.md
|
||||
* `**Mode:** mvp` for the CURRENT phase). Bounded, non-throwing — an absent
|
||||
* `phaseNumber`, an absent ROADMAP.md, an absent phase heading, or a phase
|
||||
* section with no `**Mode:**` line (or a `**Mode:**` value other than the
|
||||
* literal `mvp` token, case-insensitively) all degrade to `false` (D11; "a
|
||||
* phase with no `**Mode:**` line and an absent ROADMAP are both false, but
|
||||
* neither may throw"). Self-contained rather than reusing `phase.cts`'s
|
||||
* private `getRoadmapModeForPhase` (unexported, and importing it here would
|
||||
* be a cross-module surface change outside this task's scope) — but derived
|
||||
* from the SAME extraction primitives (`extractCurrentMilestone`,
|
||||
* `PHASE_NUMBER_TOKEN_SOURCE`-adjacent `escapeRegex`) already used by this
|
||||
* file's own `cmdInitProgress` MVP-heading scan, so it is not a second
|
||||
* ROADMAP-heading parser invented from scratch.
|
||||
*/
|
||||
function detectPhaseMvpMode(cwd: string, phaseNumber: string | null): boolean {
|
||||
if (!phaseNumber) return false;
|
||||
try {
|
||||
const roadmapPath = path.join(planningDir(cwd), 'ROADMAP.md');
|
||||
if (!fs.existsSync(roadmapPath)) return false;
|
||||
const rawContent = fs.readFileSync(roadmapPath, 'utf-8');
|
||||
const content = extractCurrentMilestone(rawContent, cwd);
|
||||
const escapedPhase = escapeRegex(phaseNumber);
|
||||
const phaseHeader = new RegExp(`#{2,4}\\s*Phase\\s+${escapedPhase}(?:\\s*\\([^)\\n]{0,200}\\))?\\s*:`, 'i');
|
||||
const headerMatch = content.match(phaseHeader);
|
||||
if (!headerMatch || headerMatch.index === undefined) return false;
|
||||
const sectionStart = headerMatch.index;
|
||||
const rest = content.slice(sectionStart + headerMatch[0].length);
|
||||
const nextHeaderMatch = rest.match(/\n#{2,4}\s+Phase\s+\S/i);
|
||||
const sectionEnd = nextHeaderMatch
|
||||
? sectionStart + headerMatch[0].length + (nextHeaderMatch.index as number)
|
||||
: content.length;
|
||||
const section = content.slice(sectionStart, sectionEnd);
|
||||
const modeMatch = section.match(/\*\*Mode:\*\*\s*([^\n]+)/i);
|
||||
return modeMatch ? modeMatch[1].trim().toLowerCase() === 'mvp' : false;
|
||||
} catch {
|
||||
return false;
|
||||
}
|
||||
}
|
||||
|
||||
/**
|
||||
* Builds the `section_manifest` init-bundle field (#2932 Deliverable 2): resolves
|
||||
* {@link sectionManifest.InvocationFacts} from this invocation, loads the generated
|
||||
@@ -413,13 +510,37 @@ function detectHasPriorPhases(cwd: string, phaseInfo: Record<string, unknown> |
|
||||
* or an unexpected throw from the evaluator itself) — this field is additive and
|
||||
* optional, never load-bearing for dispatch (Hyrum's Law: 22 direct init-bundle
|
||||
* dependents must be unaffected by its absence).
|
||||
*
|
||||
* `flags` (D1-D5): built from `options`'s OWN keys, gated on VALUE TRUTHINESS
|
||||
* — not merely `!== undefined`. `parseNamedArgs` (src/command-arg-projection.cts)
|
||||
* never yields `undefined` for an absent flag of either kind: a value-flag's
|
||||
* absence is `null`, a booleanFlag's absence is `false`. An `undefined`-only
|
||||
* absence check therefore lets BOTH kinds of absent flag leak into `flags` as
|
||||
* present. A present value-flag is always a non-empty string, and a present
|
||||
* booleanFlag is always `true` — so skipping any falsy value (`undefined`,
|
||||
* `null`, `false`, `''`, `0`) is a safe, single-rule absence test for both
|
||||
* flag kinds; `--wave 0` still resolves to `true` via `booleanFlags`, so
|
||||
* truthiness never misclassifies a real invocation as absent. `Object.keys`
|
||||
* + a plain `new Set()` so a hostile option key (e.g. `constructor`) can
|
||||
* never leak via the prototype chain.
|
||||
*
|
||||
* `needsCodebaseMap` is not computed in this shared facts-assembly scope —
|
||||
* `isBrownfield && !hasCodebaseMap` is only meaningful for `new-project`
|
||||
* (`cmdInitNewProject` already computes both operands for its own result
|
||||
* object). Rather than recomputing it here (a second, divergence-prone
|
||||
* codebase-map scan) or widening every call site's positional signature,
|
||||
* callers that HAVE the fact pass it via the optional `overrides` param;
|
||||
* every other caller passes nothing and gets `undefined` (falsy per
|
||||
* `WHEN_PREDICATES`, never invented, never throws).
|
||||
*/
|
||||
function buildSectionManifestField(
|
||||
cwd: string,
|
||||
phaseInfo: Record<string, unknown> | null,
|
||||
options: Record<string, unknown>,
|
||||
workflow: string,
|
||||
overrides: { needsCodebaseMap?: boolean } = {},
|
||||
): Record<string, unknown> | null {
|
||||
const sections = loadSectionManifestSections();
|
||||
const sections = loadSectionManifestSections(workflow);
|
||||
if (!sections) return null;
|
||||
|
||||
const rawPhaseNumber = phaseInfo?.['phase_number'];
|
||||
@@ -430,17 +551,26 @@ function buildSectionManifestField(
|
||||
? String(rawPhaseNumber)
|
||||
: null;
|
||||
|
||||
const flags = new Set<string>();
|
||||
for (const key of Object.keys(options)) {
|
||||
if (!options[key]) continue;
|
||||
flags.add(`--${key}`);
|
||||
}
|
||||
|
||||
const facts: sectionManifest.InvocationFacts = {
|
||||
waveFlag: options['wave'] === true,
|
||||
flags,
|
||||
phaseNumber,
|
||||
hasPriorPhases: detectHasPriorPhases(cwd, phaseInfo),
|
||||
worktreesEnabled: readConfigJsonBoolean(cwd, ['workflow', 'use_worktrees']),
|
||||
phaseMvpMode: detectPhaseMvpMode(cwd, phaseNumber),
|
||||
needsCodebaseMap: overrides.needsCodebaseMap,
|
||||
};
|
||||
|
||||
try {
|
||||
const selection = sectionManifest.selectSections(sections, facts);
|
||||
const readById = new Map(sections.map((s) => [s.id, s.read]));
|
||||
return {
|
||||
workflow: 'execute-phase',
|
||||
workflow,
|
||||
included: selection.included,
|
||||
excluded: selection.excluded,
|
||||
read: selection.included
|
||||
@@ -585,8 +715,8 @@ function cmdInitExecutePhase(
|
||||
}
|
||||
}
|
||||
|
||||
// #2932 (Phase 5): additive, optional field — degrades to null, never throws.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, phaseInfo, options);
|
||||
// #2932/#2992 (Phase 5/6.1): additive, optional field — degrades to null, never throws.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, phaseInfo, options, 'execute-phase');
|
||||
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
@@ -778,10 +908,13 @@ function cmdInitPlanPhase(
|
||||
}
|
||||
}
|
||||
|
||||
// #2992 (Phase 6.1): additive, optional field — degrades to null, never throws.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, phaseInfo, options, 'plan-phase');
|
||||
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
|
||||
function cmdInitNewProject(cwd: string, raw: boolean): void {
|
||||
function cmdInitNewProject(cwd: string, raw: boolean, options: Record<string, unknown> = {}): void {
|
||||
const config = loadConfig(cwd);
|
||||
|
||||
const homedir = os.homedir();
|
||||
@@ -832,10 +965,18 @@ function cmdInitNewProject(cwd: string, raw: boolean): void {
|
||||
research_dir: toPosixPath(path.join(planningRoot(cwd), 'research')),
|
||||
};
|
||||
|
||||
// #2992 (Phase 6.1): additive, optional field — degrades to null, never throws.
|
||||
// needsCodebaseMap is threaded from this scope's own isBrownfield/hasCodebaseMap
|
||||
// computation (see `needs_codebase_map` above) so `state:needs-codebase-map` is
|
||||
// genuinely computed for this workflow, not left permanently false.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, null, options, 'new-project', {
|
||||
needsCodebaseMap: isBrownfield && !hasCodebaseMap,
|
||||
});
|
||||
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
|
||||
function cmdInitNewMilestone(cwd: string, raw: boolean): void {
|
||||
function cmdInitNewMilestone(cwd: string, raw: boolean, options: Record<string, unknown> = {}): void {
|
||||
const config = loadConfig(cwd);
|
||||
const milestone = getMilestoneInfo(cwd) as unknown as Record<string, unknown>;
|
||||
const latestCompleted = getLatestCompletedMilestone(cwd);
|
||||
@@ -891,10 +1032,18 @@ function cmdInitNewMilestone(cwd: string, raw: boolean): void {
|
||||
milestones_path: toPosixPath(path.join(planningDir(cwd), 'MILESTONES.md')),
|
||||
};
|
||||
|
||||
// #2992 (Phase 6.1): additive, optional field — degrades to null, never throws.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, null, options, 'new-milestone');
|
||||
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
|
||||
function cmdInitQuick(cwd: string, description: string | undefined, raw: boolean): void {
|
||||
function cmdInitQuick(
|
||||
cwd: string,
|
||||
description: string | undefined,
|
||||
raw: boolean,
|
||||
options: Record<string, unknown> = {},
|
||||
): void {
|
||||
const config = loadConfig(cwd);
|
||||
const now = new Date();
|
||||
const slug = description ? generateSlugInternal(description)?.substring(0, 40) : null;
|
||||
@@ -945,6 +1094,9 @@ function cmdInitQuick(cwd: string, description: string | undefined, raw: boolean
|
||||
planning_exists: fs.existsSync(planningRoot(cwd)),
|
||||
};
|
||||
|
||||
// #2992 (Phase 6.1): additive, optional field — degrades to null, never throws.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, null, options, 'quick');
|
||||
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
|
||||
@@ -1827,7 +1979,7 @@ function cmdInitManager(cwd: string, raw: boolean): void {
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
|
||||
function cmdInitProgress(cwd: string, raw: boolean): void {
|
||||
function cmdInitProgress(cwd: string, raw: boolean, options: Record<string, unknown> = {}): void {
|
||||
try {
|
||||
(pruneOrphanedWorktrees as (cwd: string) => void)(cwd);
|
||||
} catch {
|
||||
@@ -2036,6 +2188,9 @@ function cmdInitProgress(cwd: string, raw: boolean): void {
|
||||
config_path: toPosixPath(path.join(planningDir(cwd), 'config.json')),
|
||||
};
|
||||
|
||||
// #2992 (Phase 6.1): additive, optional field — degrades to null, never throws.
|
||||
result['section_manifest'] = buildSectionManifestField(cwd, null, options, 'progress');
|
||||
|
||||
output(withProjectRoot(cwd, result), raw);
|
||||
}
|
||||
|
||||
|
||||
Reference in New Issue
Block a user