Seven findings from the two-axis review, all fixed in place.
THE ONE THAT MATTERS: cmdTodoComplete validated sourcePath and targetPath and
then ran every fs call against the RAW strings — existsSync, statSync,
readFileSync, platformWriteSync, unlinkSync, and the dry-run path payload —
never sourceCheck.resolved / targetCheck.resolved. That is the exact
"validate one path, use another" shape ADR-4650 names as the defect this epic
exists to prevent, and it is the same bug this phase had just fixed in
check-command-router. Committed inside the fix for it. All I/O now uses the
resolved paths; user-facing messages still echo the raw filename, never a
resolved absolute path.
A VACUOUS TEST, and the false doc claim it was propping up. The test
"[RED #4327] an absolute path outside the project is rejected" would have
passed with ZERO containment logic: path.join(pendingDir, '/abs/outside/x')
yields <pendingDir>/abs/outside/x — Node does not let a later absolute segment
escape — so the name is FOLDED under the root, passes containment, and simply
404s. The test only ever observed "Todo not found". It now asserts what is
actually true and actually valuable: an absolute name is neutralized, and the
real outside file is not read, not moved, and still present afterward.
docs/CLI-TOOLS.md claimed such a path "is rejected as a usage error", which
was false; it now describes the fold-under-root behavior. Traversal and
embedded separators ARE rejected, and those claims stand.
DUPLICATION THIS EPIC EXISTS TO REMOVE. resolvePath already did
isAbsolute-or-join + validatePath + reject; cmdGapAnalysisPlanPost and
cmdCheckPredicate each re-inlined the identical triplet in the same file. Both
now call resolvePath. Cost, stated rather than hidden: its generic message
replaces the two sites' distinct "phase-dir escapes…" wording. The message
still names the offending input, and one predicate with one message is the
point.
SYMLINK COVERAGE was required by #4652's "Done when" and was missing. Added
for both the todos root and --phase-dir, skipping cleanly on EPERM so the
Windows lanes do not fail where unprivileged symlink creation is disallowed.
Both fast-check properties were UNSEEDED. Seeded now.
The changeset named "check decision-coverage-plan" as a boundary; that is a
caller of the shared resolvePath, which the body never mentioned. Corrected.
DISCLOSED, not hidden: ctx.phaseDir is now always the resolved ABSOLUTE path,
so ${PHASE_DIR} interpolation and the "not found in <targetDir>" message show
an absolute value where a relative --phase-dir previously produced a relative
one. That is an observable output change. A test pins it and
docs/reference/gate-predicates.md states it.
Also regenerated scripts/lib/platform-conformance-tier.generated.cjs and its
macos twin — the new tests changed check-predicate.test.cjs's tier
classification. Caught by npm run lint:ci locally rather than by a bench run.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 2 of epic #4636, absorbing #4327 and #4354. Implements ADR-4650
decision 3: containment is a boundary concern — the predicate runs where
external input enters, not at whichever interior call site remembered.
Four boundaries now validate against their managed root and reject with a
USAGE-shaped error before touching the filesystem:
todo complete <name> -> todosDir(cwd)
check predicate --phase-dir <dir> -> projectDir
check decision-coverage-plan <dir> -> projectDir (via resolvePath)
check gap-analysis.plan-post <dir> -> projectDir
#4327 understated its own severity. It reports that a traversal name
"resolves outside the todos root", which reads as an information leak.
Measured, it was destructive: the command exited 0, MOVED the outside file
into completed/, and unlinked the original. cmdTodoComplete ends in
fs.unlinkSync(sourcePath), so an unconfined name consumed across the
boundary rather than merely reading across it. Validation now precedes every
fs call — existsSync, readFileSync, ensureDir, writeSync, unlinkSync — and
both halves of the move are confined, so neither source nor destination can
land outside the root. --dry-run is rejected on the same terms; a preview
must not leak a resolved outside path either.
#4354 reproduces exactly: a BLOCKING gate returned block:false sourced
entirely from a SECURITY.md in a caller-chosen directory outside the project.
THE HARDER HALF, found by the isolated adversarial review of the first
attempt: validating a path and then using a DIFFERENT one closes nothing.
The first fix validated `--phase-dir` joined against `--cwd`, then passed the
RAW unjoined value into the predicate context. gate-predicate-evaluator uses
it as-is and findPhaseArtifact resolves a relative path against the REAL
process cwd — so validation and the read used two different roots whenever
process.cwd() differed from --cwd. Reproduced: running from a directory
holding a plan with `secret_field: LEAKED_VALUE`, a predicate declared
against an empty --cwd project exited 0 and returned "actual":"LEAKED_VALUE".
The rule now applied at all three router sites: **use the validated resolved
path, never the raw input.** Independently re-verified after the fix — the
lookup resolves in the --cwd project and no value leaks.
gate-predicate-evaluator.cts is untouched and still imports no fs. Confining
in the router is what keeps that pure-leaf contract intact AND covers
${PHASE_DIR} interpolation into command-exit-zero, which an evaluator-local
fix would have missed entirely.
Also fixed, same review: `todo complete .` and `..` passed containment
(they resolve to the pending dir, which IS inside the root) and then threw an
uncaught EISDIR with an absolute-path stack trace. Now a clean USAGE
rejection naming the real reason — "todo name is not a file" — rather than
borrowing the escape message, which would have stated something false.
Ripples discharged BEFORE the verification checkpoint rather than after, per
the Phase 1 retrospective: docs/reference/gate-predicates.md and
docs/CLI-TOOLS.md document the new constraints, CONTEXT.md records why
containment lives at the router rather than the evaluator, the changeset is
written, and the install-tree goldens were regenerated to confirm unchanged
(no new shipped file) rather than assumed.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
