0763326cedd2c7310dfb16094fcace3d4fe6f317
5848 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
0763326ced |
fix(#3780): serialize WINDOWS.md ledger mutations on a cross-process lock (#4681)
* test(#3780): regression tests for parallel ledger-writer loss * fix(#3780): serialize WINDOWS.md mutations on a cross-process ledger lock * fix(#3780): keep the ledger-unavailable degrade contract intact under the lock wrapper * chore(#3780): backfill changeset PR number (4681) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ccb39aec15 |
test(#4528): migrate final seam-dispatch batch and retire the timeout-literal allowlist (#4684)
Batch 17 of 17 — the terminal batch — in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/cjs-command-router-adapter.test.cjs, tests/dispatcher.test.cjs, tests/run-tests-temp-root.test.cjs, and tests/shell-command-projection-dispatch.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. No src/bin file touched, no numeric value changed anywhere. Eslint ground truth (9 sites) matches the issue's own stated count exactly for the first time in this epic — no drift to disclose. Reuses PROBE_TIMEOUT_MS (1 site) and QUICK_SPAWN_TIMEOUT_MS (1 site). Adds four file-local constants for shapes with no existing match: RUN_TESTS_ISOLATED_PROBE_TIMEOUT_MS and RUN_TESTS_HARNESS_SPAWN_TIMEOUT_MS (run-tests-temp-root.test.cjs, distinguishing a `node -e` isolated function call from a real end-to-end spawn of the test runner itself, despite each coinciding numerically with an unrelated existing constant), and EXEC_TOOL_OPTION_PASSTHROUGH_TIMEOUT_MS and DISPATCH_FORCED_TIMEOUT_MS (shell-command-projection-dispatch.test.cjs — a mocked-spawnSync pass-through fixture and a deliberately-forced real timeout, neither a real subprocess bound in the usual sense). Terminal-batch cleanup: deletes eslint-rules/no-adhoc-timeout-literal.allowlist.json entirely, drops its require and the allowlist option from eslint.config.mjs's local/no-adhoc-timeout-literal registration (now a bare 'error', mirroring local/no-unbounded-spawn's own already-terminal configuration in the same file), and updates TESTING-STANDARDS.md's enforcement note to match — a stale pointer to the deleted file caught by review, fixed inline. A full-repo eslint run with no cache confirms zero violations anywhere in the tree under the now allowlist-free rule. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a841575037 |
test(#4527): migrate planning/review-lane batch to named timeout constants (#4680)
Batch 16 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in 9 test files with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. No src/bin file touched, no numeric value changed anywhere. Ground truth via eslint found 48 sites, not the issue's stated 38 — tests/code-review.test.cjs alone has 11, not 1 (a 10-site undercount, the largest single-file drift in this epic). All 11 are migrated. Reuses PROBE_TIMEOUT_MS (17 sites across assumption-delta.test.cjs and code-review.test.cjs), LOOP_HOOK_POINT_CLI_TIMEOUT_MS (1 site), QUICK_SPAWN_TIMEOUT_MS (1 site). Adds a new shared constant, HTTP_REACHABLE_PROBE_TIMEOUT_FIXTURE_MS, promoted because two files (reviewer-manifest-body.test.cjs, reviewer-trust-disclosure.test.cjs) independently arrived at the same probe-fixture value across 3 sites. File-local constants elsewhere for values not shared across files: FALLOW_AUDIT_TIMEOUT_MS (code-review-pipeline-regression.test.cjs); a 5-constant set covering runBashScript's own override/forced-timeout/ boundary-triple tests (plan-phase-stall-detection.test.cjs); 9 lane-specific NATIVE_TIMEOUT_MS constants, one per shipped reviewer CLI tool, even where 4 lanes coincidentally share a value (review-lane-invocation.test.cjs); and a 6-constant set covering reviewer-manifest-body.test.cjs's own probe-kind fixtures and its separate, unrelated boundary/invalid set that coincidentally overlaps in shape (not value) with plan-phase-stall-detection.test.cjs's triple. Fixes applied inline from review: GENERATOR_SCRIPT_TIMEOUT_MS was misapplied to 3 sites in plan-review-convergence.test.cjs whose actual shape (nested bash -> node -> gsd-tools.cjs chain) doesn't match that constant's documented direct-spawn class, confirmed against its own cited precedent files — replaced with a new file-local constant naming the correct class, same value. Two stray un-renamed literals in review-lane-invocation.test.cjs (missed by eslint's object-literal-only detection) were investigated and left as disclosed literals with an explanatory comment rather than force-fit onto an unrelated lane's constant, since they belong to a distinct config-override code path. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
2a5d919a03 |
test(#4526): migrate gate/predicate evaluators batch to named timeout constants (#4679)
Batch 15 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/check-predicate.test.cjs, tests/gate-predicate-evaluator.test.cjs, tests/policy-160-route0-resume.test.cjs, tests/phase6-capstone-conformance.test.cjs, and tests/prohibition-enforcement.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 5 files from the rule's allowlist. Ground truth via eslint found 17 sites, not the issue's stated 16 -- prohibition-enforcement.test.cjs has 2 sites, not 1 -- disclosed in the PR body. Reuses QUICK_SPAWN_TIMEOUT_MS and LOOP_HOOK_POINT_CLI_TIMEOUT_MS across 1 file; no new shared constants needed. Adds file-local constants for a real bounded-shell subprocess class (including one deliberately non-generous value to force a timeout), a predicate's own declarative timeout FIELD as fixture/validation data (including a deliberately-invalid zero/negative pair proving rejection), a heavier gsd-tools.cjs check CLI dispatch tier, and two distinct deliberately-short enforcement bounds forcing a fast hang-timeout path. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
0cee0eee47 |
test(#4525): migrate statusline/teams batch to named timeout constants (#4677)
Batch 14 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/gsd-statusline.test.cjs and tests/teams-status.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 2 files from the rule's allowlist. Issue #4525 cautions against forcing either file onto PROBE_TIMEOUT_MS without checking, since both invoke a "long-lived status renderer." Checking each file's actual production handler separately found the caution applies to one file and not the other: gsd-statusline.test.cjs's 7 sites all spawn hooks/gsd-statusline.js, which does real rendering work (context-window, git, teams state) -- two new file-local constants (STATUSLINE_HOOK_TIMEOUT_MS=4000, 6 sites; STATUSLINE_HOOK_GIT_SHIM_TIMEOUT_MS =5000, 1 site for a heavier git-shim test). teams-status.test.cjs's 5 sites spawn `gsd-tools.cjs query teams-status`, whose handler (gsd-core/bin/lib/teams-status.cjs's cmdTeamsStatus) is a lightweight env-truthiness check with no rendering or fan-out -- genuinely matching PROBE_TIMEOUT_MS's class, confirmed by reading the handler directly rather than assumed from the matching value. No new shared-helper constant is needed this batch. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
56b706a07a |
test(#4524): migrate task-content resolution batch to named timeout constants (#4675)
Batch 13 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeoutMs object-literal property in tests/task-content-resolution.test.cjs, tests/task-command-router-resolve-content.test.cjs, and tests/task-content-resolver-grammar-parity.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 3 files from the rule's allowlist. Every one of the 13 sites describes the same field -- a task-content- resolver manifest's invoke.timeoutMs -- as fixture/validation data; none is a real subprocess spawn timeout, verified by tracing each site to a pure function, a garbage-shape rejection path, or a fully-injected fake exec function. Adds one new shared constant, TASK_RESOLVER_INVOKE_TIMEOUT_MS, used by 2 files in this batch (crossing the promotion bar). Adds file-local constants for a value used by only 1 file, plus three deliberately-invalid values (zero, negative, non-integer) inside one findResolver garbage-shapes test proving the validator rejects a malformed manifest regardless of which way its timeout is invalid. Also fixes a review-caught defect outside the mechanical rule's own scope: a bare-literal duplicate of the new shared constant inside a ResolverTimeoutError assertion (a call argument, not an object-literal property, so the lint rule never flagged it) -- renamed in this same PR per the no-deferrals rule. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
bbdf7e8e84 |
chore(#4654): add local/no-unconfined-path-join and drain it to zero — Phase 4 of #4636 (#4674)
* chore(#4654): add local/no-unconfined-path-join and drain it to zero Phase 4 of epic #4636 — the ratchet, and the phase that makes the epic hold. THE MEASUREMENT THAT RESHAPED THE PHASE. An AST census (the repo's own parser, not grep) found what the epic never enumerated: ADR-4650 named seven containment implementations; `src/` alone held roughly 24 more hand-rolled gates across ~13 files, several guarding a write or an `fs.rmSync`. Two verified by reading rather than pattern-matching — `research-store.cts` comments its own as "ensure the resolved file path stays inside the store dir" immediately before a write, and `capability-lifecycle.cts` gates `fs.rmSync` with one. So the epic's Done-when "one containment predicate, used at every site" was FALSE when Phase 3 reported it satisfied. It is true now: the rule is clean across src/, scripts/, gsd-core/bin/ and hooks/ with an EMPTY allowlist. WHY NOT THE RULE THE ISSUE PROPOSED. #4654 proposed flagging `path.join` whose first argument is a managed root and whose later arguments derive from argv. That is a taint analysis over 2046 call sites, in ESLint, without type information; "derives from argv" is not locally decidable. Any approximation either floods or is trivially evaded, and a rule that fires on hundreds of correct sites earns an allowlist of hundreds — the opposite of a ratchet. What is actually duplicated is the COMPARISON, not the join, and that has one recognizable shape. Arm 1 X.startsWith(Y + sep) the hand-rolled containment idiom Arm 2 a containment predicate called as a bare statement, answer discarded Arm 2 is the issue's "asserts the result was narrowed, not merely that a helper was called". Its example `validatePath(x, root).resolved` is already structurally impossible — Phase 3 un-exported `validatePath` — so the remaining expressible failure is ignoring the answer, which is the defect that recurred five times in this epic. The census found exactly one live instance (`milestone.cts:1643`); it now returns the proven `ContainedPath` so consumers stop re-deriving the path the comment above it was extracted to stop them re-deriving. The rule deliberately does NOT try to catch validate-one-path-use-another where the answer is used but a different variable flows onward. That needs flow analysis; the branded `ContainedPath` from Phase 3 is the defense there, and the two are complementary. PER-SITE FAMILY CHOICE, NOT A DEFAULT. Phase 3's lesson binds: collapsing a lexical site onto the realpath family broke four tests and was caught only by the matrix. Every migrated site was triaged individually. The six installer-migrations tree-walks and the six capability-lifecycle gates take the LEXICAL family because their operands are already realpath-resolved and they deliberately treat the final component as a link; boundary sites take realpath. TWO SITES WITH AN INVERTED CONTRACT, which a mechanical swap would have broken. `installer-migrations.cts:127` and `runtime-artifact-install-plan.cts:144` REJECT `target === root` by contract, while the canonical comparison ACCEPTS it. Swapped naively, a migration could `rmdir` the user's config root and a third-party descriptor could write at configHome itself. Both keep `=== root` as an explicit additional arm alongside the predicate call — the predicate decides containment, the call site keeps its own extra condition (ADR-4650 decision 6). ONE DUPLICATE DELETED OUTRIGHT: `planning-inspect.cts`'s `isWithinRoot` was byte-identical to `isContainedIn` and said so in its own docstring. `isContainedIn` is now exported for callers that have already resolved both operands and need only the comparison, with a doc note that a caller which has NOT resolved them must use a full predicate instead. THE MARKER, AND WHY IT IS NOT THE ALLOWLIST. Nine sites are justified holdouts and carry `// allow-handrolled-containment: <reason>` with a mandatory, reviewable reason. Two justifications: (a) not a containment decision — an ancestor-walk loop condition, sub-repo grouping, worktree identity matching, declared-path coverage; (b) it IS containment but the canonical predicate is unreachable — `capability-validator.cjs` is a committed pre-build `.cjs` and the compiled `security.cjs` is untracked build output, so requiring it would break a fresh clone. `scripts/lib/drift-scan.cjs` runs under `lint:ci` with the same exposure. The marker was renamed from `allow-lexical-prefix-match` mid-phase because that name asserted only (a) and would have stated something false at the (b) sites. A marker suppresses BEFORE the violation counter increments, so a file whose every occurrence is marked still reports `staleAllowlistEntry` — otherwise a drained entry lingers and silently re-permits the site later. DEMONSTRATED RED, per #4654: a hand-rolled copy reintroduced into a real `src/` file made `npm run lint` fail with the rule's full guidance message; removing it returned the tree to clean. Both halves recorded — red alone proves nothing, since a rule red for an unrelated reason looks identical. DISCLOSED: `defaultRequireFromInstallRoot` (gsd-tools.cjs) previously carried two distinct rejection messages and two manual realpath calls; routing it through `tryWithinRoot` collapses them to one message, and a missing module now surfaces as MODULE_NOT_FOUND rather than ENOENT. No test asserts either message. The security property is preserved and slightly strengthened — the candidate is realpathed and containment re-checked, and the dangling-symlink oracle closure comes along with it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4654): record the containment ratchet in CONTEXT.md and the security model Both entries previously described the seam without the thing that keeps it a seam. They now state what the rule bans, and — more usefully for whoever reads this next — what it deliberately does NOT attempt: deciding per path.join call whether an argument came from user input. That question is not locally decidable, and an approximation across ~2000 join sites would earn an exemption list of hundreds, which is the opposite of a ratchet. Also records the marker's two legitimate justifications and that its reason is mandatory, so the escape stays reviewable rather than becoming a mute button. Glossary gate 270 refs exit 0; install-tree goldens and CONTEXT-INDEX.json regenerated and confirmed byte-identical rather than assumed — which also confirms eslint-rules/ is not a shipped path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4654): close review findings and the two matrix failures MATRIX FAILURE 1 — a collapsed message broke a negative-proof test, and my evidence for collapsing it was wrong. I searched tests/ for the literal string "resolves outside its install root", found nothing, and reported that no test asserted it. The test matches a REGEX SUBSTRING, /outside its install root/, so the literal search missed it. What broke was "NEGATIVE PROOF: a symlinked module pointing OUTSIDE the install root is not loaded" — the test guarding the exact property I claimed was preserved. defaultRequireFromInstallRoot now does both checks again with both messages byte-identical, each routed through the canonical predicate, which is better than the original since that hand-rolled both comparisons. MATRIX FAILURE 2 — shipped migrations are checksum-locked, and a marker cannot serve there. migrationChecksum hashes plan.toString(), which INCLUDES comments, so a suppression marker inside a plan body drifts the baseline exactly as an edit does. Measured: with markers in place, two of the four still differed from their committed checksums. The four shipped bodies are now byte-identical to next, and the rule's config excludes those four paths BY NAME rather than by a directory wildcard, so a NEW migration is still covered. Six containment comparisons stay un-ratcheted there; that gap is recorded in the rule's Known gaps, in CONTEXT.md and in the security model rather than left implicit. Justification (c) is removed from the marker's documented reasons, because a marker was proven unable to express it. ADVERSARIAL REVIEW — the sharpest finding was that the rule banned the CORRECT shape while permitting the incorrect one: startsWith(root) with no separator is the genuinely unsafe form, since it accepts a sibling such as root-evil, and my own test blessed it as valid. Flagging every bare startsWith would swamp the rule, so that stays a STATED gap rather than a silent one. Closed for real: the template-literal spelling, which the census never saw because it only inspected plus-concatenation — that surfaced TWELVE more sites, now triaged and migrated. A separator reached through a const alias is now resolved via scope analysis. And isContainedIn, exported in Phase 3, was missing from the discarded-result set, so a bare no-op call went unflagged on the one function the epic funnels through. SECURITY REVIEW — the marker could over-suppress two ways: a block comment worked identically to a line comment, and one marker silently covered every violation sharing its line. It now requires a Line comment positioned after the flagged node ends, so it anchors to the node it trails. Four sites had dropped an unreachable-but-deliberate equality rejection against the root; each is restored as the call site's own arm. eslint.config.mjs still documented the OLD marker token, which my rename missed — it would have sent the next author in circles. A FALSE GREEN, recorded because it nearly stuck: lint:ci reported exit 0 from a stale eslint cache while twelve real violations existed. Every lint check here now clears the cache first. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4654): anchor a suppression marker to the violation it actually trails The matrix caught this; my own test caught it, on its first execution. The case "two violations on one line: trailing marker suppresses only the one it trails" expected 1 error and got 0 — both were suppressed. ROOT CAUSE: the anchoring accepted any Line comment on the node's line whose range started at or after the node's end. A trailing marker at the END of a line sits after EVERY node on that line, so that condition held for all of them. "After the node" does not identify WHICH node the marker trails. The fix reads as correct and is not. FIX: deferred reporting. Violations accumulate during traversal instead of being reported immediately; at Program:exit each marker claims exactly ONE pending violation — the one on its line whose end is nearest before the marker begins — and every unclaimed violation is then counted and reported. One marker, one suppression. An earlier violation sharing the line is still reported, which is the property the security review asked for and the previous attempt only appeared to deliver. The counter now increments at flush time rather than during traversal, so a suppressed occurrence still does not keep an allowlist entry alive. AND A TOOL THAT SHOULD HAVE EXISTED BEFORE THE FIRST MATRIX RUN. `node --test` is hard-blocked here, so this rule's test file could only ever be executed on the remote matrix — which is why a broken anchoring shipped into a run. ESLint's programmatic Linter API is not a test runner, and exercising the rule through it verifies every case locally in seconds. All 24 now pass locally, including the two-on-one-line case that failed remotely. That loop should have been built before the rule was first sent to the matrix rather than after it failed twice. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4654): backfill PR 4674 into the changeset and complete 70-docs.json The phase gate requires enablementSequence and the Diataxis quadrants; 70-docs now carries both, with the how-to quadrant skipped for a stated reason rather than an empty field. The audience for this deliverable is a contributor who trips the rule, and the task-oriented guidance reaches them in the ESLint message itself — which names the correct predicate, says how to choose between the realpath and lexical families, cites the Phase 3 regression caused by choosing wrong, and gives the marker syntax. A docs/how-to page would be a second, driftable copy read by nobody at the moment of failure. enablementSequence is recorded as what it actually is: a VERIFICATION sequence, not an enablement one. The rule is never off, so there is no off-to-on transition to describe. scripts/lint-docs-required.cjs now passes (ok_docs_updated) — it could not evaluate against the mandated pr:0 placeholder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Twisted Fate
|
6edd506cc7 |
fix(#4558): report a byte-identical restore destination as already_present (#4599)
* fix(#4558): report a byte-identical restore destination as already_present restore-custom-files treated a destination that is byte-identical to its backup exactly like a missing one: plan reported it as `eligible`, --apply re-copied the same bytes and reported `restored`, and both counters included it. Because update.md drives its restore question off eligible_count, the workflow re-offered the same no-op restore on every update and accepting it never settled anything. Emit a distinct `already_present` outcome for that case. It is excluded from eligible_count and restored_count, --apply writes nothing for it, and the backup is left intact. A differing destination is still skipped_destination_exists and a missing one still restores normally. Regression tests cover the identical-destination plan/apply paths, the idempotence-after-success cycle (missing -> restored -> silent plan), and a mixed backup. Docs for the outcome enum are updated to match. * fix(#4558): tighten already_present wording after review update.md's RESTORE_ELIGIBLE == 0 branch now also names the already-present case, CLI-TOOLS.md no longer calls the follow-up plan run "silent" (the entry is still reported, just never offered), and a test message reads correctly. * chore(#4558): add changeset fragment for #4599 * chore(#4558): acknowledge update.md growth from the restore-outcome guidance Emitted-Drift-Ack-Growth: update.md — added already_present restore-outcome guidance for #4558 --------- Co-authored-by: TwistedRiCen <16397953+TwistedRiCen@users.noreply.github.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
d6788a6805 |
test(#4523): migrate config/env/locking/perf batch to named timeout constants (#4673)
Batch 12 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/check-env.test.cjs, tests/config-get-default.test.cjs, tests/federated-config.test.cjs, tests/gsd-check-update-worker-platform-gate.test.cjs, tests/gsd-mcp-server-bin.test.cjs, tests/health-validation.test.cjs, tests/locking-bugs-1909-1916-1925-1927.test.cjs, tests/perf-316-state-lock-buffer-alloc.test.cjs, tests/perf-317-context-monitor-fs.test.cjs, and tests/pi-config-dir-env-override.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 10 files from the rule's allowlist. Ground truth via eslint matched the issue's stated 26 sites across 10 files exactly. Reuses PROBE_TIMEOUT_MS, GENERATOR_SCRIPT_TIMEOUT_MS, GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS, and INSTALL_TIMEOUT_MS across 4 files. No new shared constants needed -- every recurring value across files was independently verified to be a genuinely different operation class, per this migration's standing rule that numeric coincidence is never identity. Adds 11 new file-local constants, three of which are not real subprocess timeouts at all (a config-merge fixture value, and two node:test per-test timeout options bounding ReDoS/lock-retry regression backstops). Per the issue's explicit mandate, gsd-check-update-worker-platform-gate.test.cjs now imports (read-only) NPM_VIEW_TIMEOUT_MS from gsd-core/bin/check-latest-version.cjs for disclosure -- this file and that production module once independently guessed the same 15000ms value, causing the PR #4428 Windows double-SIGKILL collision. No site in this file's current bare literals actually wraps a live npm-view call needing margin arithmetic, so the import documents the historical relationship honestly rather than fabricating a computation. No src/bin file edited (only a read-only import added), no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
BeeHiggs
|
d1d9ee85a5 |
feat(#4142): thread convention through the completion-path membership seam (#3644)
* feat(#2761): gated heading-intro selection + one bracket identity grammar Foundation. Two owner-level changes plus a federated convention resolver; no reader consumes them yet. 1. GATED SELECTION, not an ungated widening. Widening every heading matcher requires the claim "no legacy ROADMAP contains a `[CODE.MM]` bracket followed by a digit", and that is false: `### [RFC.2119] 5:`, `### [v1.0] 2024:`, `### [ADR.612] 3:` and `### [ISO.8601] 2026:` are ordinary headings, and a widened reader claims each as a phase — moving phase_count and total_phases and adding W006 on projects that never opted in. No narrowing rescues it: the premise is about documents we do not control. `phaseHeadingPrefixSrcFor(baseline, convention, capturing?)` selects the pattern SOURCE at construction time. A project whose resolved `phase_id_convention` is not exactly 'bracket' compiles the same source string it compiled before. `baseline` is explicit because whether a site spells the any-bracket prefix or a bare `Phase\s+` is a fact about that site's history: handing the wider grammar to a bare site retro-grants tolerance it never had, in both directions — warnings appear, and a warning that fires today vanishes. Both bracket forms CAPTURE. `[GSD.999] Phase 07:` previously matched through the base alternative, which captures nothing, so a reader saw no bracket, fell back to the legacy token rule, and counted a labeled icebox heading while excluding the label-less one beside it — two derivations of one ROADMAP disagreeing. 2. ONE bracket identity grammar, one width rule. The milestone width is reconciled with the emit validator: pad2 output, so two digits or 3+ with no leading zero. Earlier spellings diverged in both directions — admitting `002`, which the validator rejects, and a bare `0` pad2 never produces — and the section recognizers accepted `[GSD.2]`, which SCOPED a milestone no phase heading could then resolve into, recreating the on-disk-count fallback this epic removes. An unpadded bracket is now uniformly malformed: it scopes nothing, bounds nothing, sections nothing. W005 on its directories is the surfacing signal. The milestone field is boundary-anchored, so a malformed run cannot match by its prefix (`GSD.002-01` read as sentinel `00`). Recognition stays case-insensitive because readers compile `/i`, but identity helpers match `[A-Z]`, so a captured id is folded first — otherwise `### [gsd.999] 07:` failed every sentinel test. The qualified key shares the width, the `(?=-|$)` boundary and the single-sub-phase shape of the directory token, because phaseTokenMatches returns unconditionally on a qualified hit: a key matching a directory isPhaseDirName rejects would be a final wrong answer. 3. resolvePhaseIdConvention federates workstream -> root exactly as config-loader does — including that root is a fallback only when a WORKSTREAM is active, so a project-scoped directory stands alone. loadConfig cannot serve this: it merges against CONFIG_DEFAULTS and drops keys it does not know, and this key is not among them. It governs the bracket-selection reads ONLY. PHASE_HEADING_PREFIX_SRC is left byte-identical: PR-1 shipped it, nothing consumes it, and it is superseded rather than redefined. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): roadmap.cts selects its heading grammar from the convention Six matchers build their intro through the gated selector, and cmdRoadmapAnalyze / cmdRoadmapGetPhase / getRoadmapPhaseWithFallback each resolve the convention ONCE per command and thread it down. Three sites take the any-bracket baseline (they already tolerated `[anything] Phase N`); three take label-only (they spelled a bare `Phase\s+`). Handing the wider grammar to a label-only site retro-grants tolerance it never had — and not only by adding matches: on a legacy repo an unchecked `- [ ] **[v1.0] Phase 05: Thing**` bullet would start SUPPRESSING the W006 that fires today. Sentinel handling under bracket ADDS a rule rather than replacing one: a bracketed heading is a sentinel when its bracket milestone is reserved (`### [GSD.999] 01:`) OR when its token is, so the engine-wide 0/999 backlog convention keeps applying to `### [GSD.02] 999:`. Replacing the token rule let a mid-migration ROADMAP — bracket headings plus a legacy backlog block, exactly the content this epic targets — add entries to the progress denominator. The captured id is folded before the identity test, so a lowercase `### [gsd.999] 07:` is excluded too. The DIRECTORY read is threaded too. `cmdRoadmapAnalyze` resolves the convention once and hands it to all four of its heading/checklist patterns, but the single `phaseTokenMatches` call that decides `disk_status`, `plan_count`, `summary_count`, `has_context` and `has_research` was left two-argument — so every canonical `{CODE}.{MM}-{PP}-slug` directory read as `no_directory` with zero counts, on the PR's own headline verb, while the SAME build resolved those same directories correctly in three other places on the same repo (W006/W007 via phaseTokenFromDir, `state json` via the milestone filter, and the W021 milestone-complete read through this very helper's three-argument form). It failed ONLY for the directory shape the convention exists to name: a mid-migration bracket repo carrying legacy `01-one` dirs resolved fine, which is why nothing caught it. Measured, bracket vs its flat-legacy twin: `[["01","no_directory",0,0],["02","no_directory",0,0]]` against `[["01","complete",1,1],["02","planned",1,0]]`. The oracle is the twin, computed in the same test run, plus exact literals — `grep disk_status tests/adr-612-*` was zero hits before this, so neither the fix nor a future regression had any gate at all. Disclosed: a ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted. Silent invisibility during the migration window is the deliberate trade against claiming phases on projects that never opted in. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): validate.cts selects its grammar; gated directory recognition The W006/W007 feeders take the resolved convention as a threaded parameter. These sites carry the letter-tolerant `[\w][\w.-]*` capture, which makes them where an ungated widening does the most damage: `### [RFC.2119] 5:` enters roadmapPhases as a phantom and becomes a W007 "in ROADMAP.md but no directory on disk" on a project that never opted in. buildRoadmapPhaseVariants also surfaces the tokens borne ONLY by sentinel-bracket headings. Surfaced rather than filtered in place because roadmapPhases feeds both a membership check and a missing-directory warning, and only the latter should ignore an icebox item. That set is OCCURRENCE-AWARE, and the subtlety is load-bearing: roadmapPhases is a TOKEN set, so `[GSD.999] 01` and `[GSD.02] 01` collapse to one entry. Keying suppression on the token alone let an icebox heading silence a REAL phase that happens to share its number — a false negative strictly worse than the warning it removed. A token is suppressed only when no non-sentinel heading bears it. Directory recognition is added as gated FUNCTIONS beside the exported RegExp constants, which stay byte-identical: the `{CODE}.{MM}-` prefix is string-indistinguishable from the letter-prefixed-decimal family this repo documents as ambiguous, and folding a branch in changes those constants' answers on exactly that family. A RegExp constant has nowhere to attach a gate. The recognizer mirrors the emit grammar and delegates the token to the canonical owner, so recognizer and resolver agree on rejected input as well as accepted. Both functions throw on a non-string, matching the call pattern they replace. buildRoadmapPhaseVariants' CHECKLIST scan is capturing, like its heading twin and like the sibling checklist scan in roadmap.cts, and for the reason that one states: the bracket id has to ride along or the sentinel filter is blind to `- [ ] **[GSD.999] 01: Icebox**`. Left un-capturing, the scan called every checklist token REAL, and the occurrence-aware un-suppression loop then deleted the icebox token the HEADING scan had correctly marked sentinel — so `validate consistency` warned that a bracket ICEBOX phase had no directory, in the HOUSE ROADMAP shape where an icebox appears as both a bold bullet and a detail heading. `validate health` stayed silent on that same repo, so the two verbs disagreed — which is the disagreement `sentinelPhases` exists to close. Both directions are pinned, because the failure mode of a careless fix here is the opposite one: a real phase sharing a sentinel's token must still warn. It does, in all four shapes that attack it (sentinel heading + real bullet, lowercase sentinel, sentinel after the real heading, colon-less bullet). Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): count bracket headings, and retire them, in both derivations Both `total_phases` derivations select their grammar from the resolved convention, in one commit — cmdStateSync already carries the comment that it mirrors buildStateFrontmatter "so both report consistent percents (#3242 Bug B)", so teaching one and not the other ships that divergence. The #1514 retirement filter widens WITH the counter it protects. The canonical gesture strikes the checklist BULLET and leaves the detail heading intact, so a bracket-form retirement went undetected and the phase stayed in the denominator forever. That is half a fix alone: the retired key is compared against phaseKeyFromDir, which called extractPhaseToken with no convention. Both halves land here. Under bracket the sentinel token rule composes as the full engine set {0, 999}, so this counter agrees with `roadmap analyze`, which has always excluded both — otherwise the two derivations report different numbers for one ROADMAP and the changeset's "excluded from every count" is false as written. The LEGACY path keeps its pre-existing 999-only rule: widening it there would move legacy totals, so the two stay split off the bracket path exactly as they are today. The sync-side assertion reads the PERCENT sync writes into the STATE.md body, not the frontmatter total_phases. Sync's own counter never reaches that field — the read derivation writes it — so asserting the frontmatter after a sync measures the read path twice and lets a mutation to the write-path guard survive. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#2761): verify.cts bracket-coherence W021 + selected milestone-complete read The shipped milestone-prefixed W021 gate keeps its ROOT-only config read, verbatim base semantics. Federating it silently moved a legacy convention's answer in BOTH directions on workstream repos — a W021 that fires at base vanishing, and one that is silent at base firing. resolvePhaseIdConvention governs the new bracket-selection reads only. B6, the milestone-complete check, keeps its ungated POSTURE (bug-557 pins it with an empty config) but selects its grammar from the convention. Inferring 'bracket' from the shape of a matched bracket ran a repo-failing check against a legacy ROADMAP that merely contained `### [RFC.2119] 5:`. Directory resolution widens with the heading read, so a bracket repo whose phases are on disk stays silent, and a bracket sentinel is not reported as unstarted. checkBracketCoherence is advisory and gated. Anchored to tokenizeHeadings so fenced examples cannot warn and heading level is structural. Its scope rules each close a way it silently did nothing or fired wrongly: only a genuine MILESTONE heading opens or closes a section (a `### Notes` used to reset scope and disable both sub-checks); a legacy `## v3.0` DOES close it; an M-NN or letter-suffixed phase heading raises missing-bracket and CONTINUES; a bare `#### 2026:` is not a phase; the full h2-h6 range is processed. Its section recognizer shares the one milestone width, so an unpadded `### [GSD.3] 05:` can no longer be a phase to the id grammar and a section to the section grammar at once, silently re-scoping every warning after it. validate consistency suppresses bracket sentinels in its missing-directory warning — the two verbs disagreed, health suppressing via notStartedPhases while consistency did not. The legacy reading is untouched, including its pre-existing wart that `### Phase 999:` still warns there. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2761): scope the milestone by its bracket; select the disk-side filter Two roadmap-parser reads, both of which made a bracket project's totals track the disk instead of the ROADMAP. The ADR pins the bracket milestone heading as `## [GSD.02] Foundation` — a name, no version — but scoping matched STATE's `milestone: v2.0` STRING against a heading, so the canonical form matched nothing and total_phases fell back to the directory count. The rule was re-derived in THREE places: extractCurrentMilestone plus two `milestoneBounded` guards; fixing one left the others falling back regardless, so they are now one gated helper. It matches the CANONICAL padded spelling only — accepting `0*N` bounded a milestone whose phases were invisible, which un-suppressed a progress percent computed off an unscoped disk count. getMilestonePhaseFilter's heading scan becomes the 14th selected read. On a bracket ROADMAP it collected nothing, so the filter degraded to pass-all and buildStateFrontmatter counted every other milestone's directories — making the bracket convention strictly worse than the M-NN one it supersedes on the property that matters most: totals must track the ROADMAP, not the disk. The DIRECTORY side of that same filter is selected with it. Teaching only the heading scan was half a fix and a worse one: `milestonePhaseNums` became non-empty, so the pass-all degrade stopped firing, but no bracket directory could satisfy the three legacy dir checks (numericRe fails on `GSD.02-05-five`, the custom-id match captures the project code `GSD`, and stripProjectCodePrefix does not strip a dotted prefix). Every bracket directory was rejected, and completed_phases / total_plans / completed_plans / percent all collapsed to 0 while `state sync` went on writing a percent off the unfiltered disk — `state json` reporting 0% on the same repo, in the same second, that STATE.md's body called 67%. That is the #3242 Bug B divergence this PR exists to avoid, and total_phases could not show it: `Math.max(phaseDirs.length, roadmapPhaseCount)` floors it at the ROADMAP count no matter how many directories are rejected. The dir side matches on the milestone-QUALIFIED id, delegated to the owner's gated `phaseTokenMatches(dir, id, 'bracket')`, not on the bare token: READING-B puts the milestone in the bracket, so `GSD.01-01-old-one` and `GSD.02-01-one` share the token `01` and only the qualified key separates them. The qualified ids are kept in their own set — a hyphen in `milestonePhaseNums` would flip `roadmapUsesHyphenedIds` and silently move the LEGACY dir path on a bracket repo — and the branch is ADDITIVE: on a miss it falls through to the three legacy checks, so a bracket project carrying legacy-shaped directories reads unchanged. Both are resolved lazily and gated, so the legacy path pays neither a config read nor a second scan and cannot change answer. The scoping call is also GUARDED: resolvePhaseIdConvention reaches planningDir, which throws a plain Error for a GSD_PROJECT/GSD_WORKSTREAM segment carrying `/`, `\` or `..`. At base the only planningDir call in extractCurrentMilestone sits inside the STATE-read try, so the function returned normally on such an environment; an unguarded one here let that escape and broke the never-throws invariant that getRoadmapPhaseInternal and getMilestoneInfo three hundred lines below carry #2245 / ADR-227 notes about. Unreachable through the CLI — GSD_WORKSTREAM is rejected up front by the workstream-name policy and GSD_PROJECT throws identically at base — but reachable by any in-process embedder, which is precisely who that invariant is for. The filter's own resolve call was already inside its try and is unaffected. The milestone-qualified key is formed only for a token that is itself a bracket phase token. `${bracketId}-${token}` is a string SPLICE, so a mid-migration heading carrying an M-NN label — `### [GSD.02] Phase 02-01:` — spliced to `GSD.02-02-01`, which the qualified-key grammar reads as milestone 02 / phase 02: the `-01` truncated, both such headings collapsing to one key, and the heading claiming `GSD.02-02-two`, the directory it does NOT name, while rejecting `GSD.02-01-one`, the one it does. The guard drops those headings back to the unqualified legacy path, restoring the base ACCEPTANCE VECTOR exactly — pinned against the milestone-prefixed reading of the same ROADMAP, which is base-identical on this shape. Scoped precisely, because the fixture moves one number that the guard does not touch: `total_phases` on it reads 1 at base and 2 here. That is the bracket heading COUNT this PR exists to add, not the splice — measured identical with and without the guard, and identical to what the canonical `### [GSD.02] 01:` spelling does on the same fixture (both read 2 with zero directories on disk, where base reads 0). The claim is base-equivalent ACCEPTANCE, not a base-equivalent reading. One consequence is stated rather than fixed: a heading whose token carries a hyphen still puts that hyphen into milestonePhaseNums and so still flips `roadmapUsesHyphenedIds`. Base does the same for that spelling, so preserving it is what keeps the shape base-equivalent; excluding the token would have moved answers versus base on malformed input. The comment at the qualified-set declaration is corrected to claim only what is true — it keeps QUALIFIED IDS out of that flag's input, not hyphens in general. The oracles ship with it, and they are the five numbers, not the one: the parity gate now asserts total_phases, completed_phases, total_plans, completed_plans AND percent, on both derivations, on two fixture shapes (one milestone; two milestones with stale prior-milestone directories on disk). The oracle is the flat-legacy twin, built in the same test run and compared number for number, plus exact literals so a shared wrong answer cannot pass. The oracle SUBSTITUTION is itself pinned. The M-NN spelling of these shapes could not serve, because buildStateFrontmatter's #2445 de-dup key captures only a directory's leading integer and collapses `02-01-one` / `02-02-two` / `02-03-three` to one — measured [3,0,1,0,0] against the flat-legacy twin's [3,2,3,2,67], identically at base and before this fix, and structurally unreachable from the bracket key space. That reasoning is only sound while it stays true, so a characterization test holds the M-NN reading down on the two numbers that do not depend on which directory wins the mtime race. Widen the de-dup key and it fails, instead of quietly invalidating the changeset's disclosure. Also adds the call-site pin. The structural table pins transcription against the selector; it cannot see a call site whose BASELINE ARGUMENT is wrong. Flipping verify.cts's milestone-complete site to the wider baseline grants a fires-on-every-repo check tolerance it has never had, and every behavioural test still passed. The pin reads the shipped sources and asserts the mode at each of the 14 sites, count-exact. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2761): pin the bracket read surfaces in the parity gate This gate exists because #2043 fixed one bug across five hand-edited copies of a rule and #2232 was the residual that survived, because a later reader could not tell the copies were one rule. PR-2 adds two consumers, so they belong here. Surface 7 — the heading read and the directory read must agree about WHICH phase a `MM-<seg>` pair names, across the shared width corpus, and the bracket and legacy spellings of one heading must yield the same token. Surface 8 — the two bracket directory readers, in BOTH directions. Agreement on ACCEPTED input was already pinned; agreement on REJECTED input is where they actually diverged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2761): changeset Disclosures for the PR body (deliberate, not defects): - phase_id_convention is not a CONFIG_DEFAULTS key, so loadConfig drops it and cannot serve as the convention resolver however the file is federated. This PR ships its own workstream->root resolver; adding the key and its value enum is later-slice work. - Convention matching is strictly === 'bracket'. A misspelled value reads as not-configured and the project keeps legacy behaviour silently. - An UNPADDED bracket milestone (`[GSD.2]`) is malformed: it scopes nothing, bounds nothing, sections nothing, and is not a phase id. W005 on its directories is the surfacing signal. - WIDTH UNIFICATION MOVED FOUR MERGED PR-1 EXPORT ANSWERS on non-canonical inputs, none of which toDir can emit and none of which had a bracket caller at base: isSentinelPhaseId('GSD.0-01', 'bracket') true -> false isSentinelPhaseId('GSD.0999-01', 'bracket') true -> false getMilestoneFromPhaseId('GSD.2-01', 'bracket') 'v2.0' -> null getMilestoneFromPhaseId('GSD.002-01', 'bracket') 'v2.0' -> null The canonical pad2 sentinel spelling `[GSD.00]` still tests true. - FLAG TO MAINTAINER: docs/adr/612:132 reads "Sentinel behavior (0.x / 999.x -> milestone null) is preserved". After the unification that holds for the canonical `00` spelling only, not for a bare `[GSD.0]`. ADR wording is yours; flagging the tension rather than editing it. - The bracket sentinel rule COMPOSES with the legacy one — a bracketed heading is a sentinel when its bracket milestone OR its token is reserved. Under bracket the state-side token rule is the full {0, 999} set so both derivations agree; the LEGACY path keeps its pre-existing 999-only rule, unchanged. - validate consistency's legacy reading is untouched, including the pre-existing wart that `### Phase 999:` warns there while validate health suppresses it. - find-phase still cannot resolve a bracket phase directory. phase-locator.cts is outside this PR's module set. Sibling PR #2559's matchPhaseDirs calls phaseTokenMatches without a convention, so whichever slice lands second must thread it through. - Four of the five bracket readers scan raw ROADMAP content, so a bracket heading inside a fenced code block is read as a phase. Pre-existing for the legacy spelling; parity, not a new class. - roadmapPhaseLookupSources gained no bracket source: nothing emits a milestone-qualified query into it yet. - roadmap validate remains a separate, unfederated convention reader. Pre-existing and base-identical, but two verbs can disagree about the active convention on one project. - _diskScanCache keys on cwd while the values it caches are now convention-dependent. Not reproducible through the CLI; pre-existing for the workstream dimension, widened here. Stated as inconclusive. - A ROADMAP written in bracket form before config.json is switched reads as empty rather than mis-counted — the deliberate migration-window trade. - THE READ AND WRITE PERCENTS STILL DIVERGE ON A MULTI-MILESTONE REPO, and that divergence is MIRRORED under bracket rather than closed. buildStateFrontmatter applies the milestone filter; cmdStateSync does its own fs.readdirSync and never calls it, so on a repo carrying prior-milestone directories the read path reports the SCOPED percent and the sync body reports the WHOLE-DISK one. Measured on the true base build ( |
||
|
Tom Boucher
|
329b7aadb4 |
Merge pull request #4672 from open-gsd/refactor/4653-drain-containment-duplicates
fix(#4653): make path containment one decision, narrow the export, and close a symlink escape — Phase 3 of #4636 |
||
|
sim
|
2a6f74027f |
chore(#4653): backfill PR 4672 into both changesets
Also corrects the Changed fragment: it named three containment exports as the only ones, which stopped being true when the lexical family was added to close DW1/DW9. It now describes one decision resolved two ways, and says why the lexical pair exists rather than leaving a reader to assume it is a weaker alternative to the realpath form. scripts/lint-docs-required.cjs now passes (ok_docs_updated) — it could not evaluate against the mandated pr:0 placeholder. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
bbc3f131be |
refactor(#4653): make containment ONE decision, resolved two ways
Satisfies #4653 DW1 and DW9, which were the phase's outstanding acceptance criteria: every other implementation must be deleted or route its containment DECISION through the canonical predicate, and no surviving wrapper may decide WHETHER a path is contained. Three implementations were being retained with their own comparisons, on the argument that each needs LEXICAL resolution — a realpath-based predicate is the wrong tool wherever a symlink must be preserved rather than resolved. That argument is correct about RESOLUTION and was being used to justify owning the DECISION too. Those are separable, and separating them is what closes the criteria honestly rather than by reinterpretation. isContainedIn(resolvedTarget, resolvedRoot, pathImpl?) module-internal is now the single place this repo decides containment. It is separator-aware, so a sibling merely sharing a prefix (`<root>-evil` against `<root>`) is still rejected. Two exported families sit on it and differ ONLY in how a candidate is resolved before the decision: assertWithinRoot / tryWithinRoot realpath-resolving assertWithinRootLexical / tryWithinRootLexical path.resolve only, no I/O The lexical pair carries `opts.pathImpl`, so win32 separator semantics stay testable off Windows — that seam already existed in isPathConfined and would have been lost by a naive collapse. The three call sites now take their decision from the predicate and keep only what is genuinely theirs: external-descriptor-trust isPathConfined delegates outright; pathImpl forwarded installer-migrations ensureInsideConfig delegates; keeps its own message and its LEXICAL fullPath, which callers consume for existsSync and journal rows gsd-tools.cjs isInsideDir delegates; keeps its own `target !== root` condition, and the separate symlink refusal above it stands DW5 is not weakened by this. That criterion binds the symlink oracle and the ancestor canonicalization; both are untouched. The only change inside validatePath is three comparison lines becoming one call, and the rejection string `Path escapes allowed directory: <resolved> is outside <base>` stays byte-identical because it is an observable CLI contract. What this does NOT do, stated plainly: the lexical family still cannot see a symlink. That is a property of lexical resolution, not a gap in the seam, and the three callers that need it are the three that must pair it with their own symlink refusal — which is exactly what the fix earlier in this phase added at the install sites. The doc comment says so at the definition, and CONTEXT.md and docs/explanation/security-model.md are corrected: they previously described these three as deliberately NOT routed through the predicate, which is no longer true. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
6f0e5ccf85 |
fix(#4636,#4653): close the symlink hole, revert a wrong collapse, fix six review findings
The RED checkpoint and two orthogonal reviews found eight defects. All fixed here.
THE COLLAPSE THAT WAS WRONG — installer-migrations. Routing ensureInsideConfig's
containment decision through the realpath-based canonical predicate broke four
tests, and the failure message says it plainly: "migration path escapes
configDir: extensions/gsd.cjs". That module's entire contract is that a
symlinked managed path is snapshotted, restored and backed up AS A LINK and
never dereferenced. The canonical predicate dereferences, then rejects the
result for escaping configDir — so it destroys exactly the thing the module
exists to preserve. Reverted to lexical, with the ruling recorded above the
function so it is not collapsed a third time. normalizeRelPath is the real
pre-gate there; it throws on absolute paths and '..' before this check runs.
That makes THREE deliberately-retained implementations, not two, and they share
one shape worth naming: a realpath-based predicate is the wrong tool wherever a
symlink must be PRESERVED rather than resolved. CONTEXT.md and
docs/explanation/security-model.md are corrected — both previously described
ensureInsideConfig as collapsed.
THE MISSED CONSUMER. tests/security-prompt-injection.security.test.cjs
destructures validatePath from the compiled lib; un-exporting it turned five
tests into TypeError. It appeared in my own earlier search output and I did not
follow it up. Translated under the same rule as the rest: assertions on the
rejection REASON go through assertWithinRoot, boolean-only through
tryWithinRoot.
VALIDATE-ONE-PATH-USE-ANOTHER, FOUND TWICE MORE. This is the fourth and fifth
occurrence in this epic of the exact defect it exists to prevent.
- scripts/check-glossary-refs.cjs decided containment on `token` and then
stat'd a separately re-joined path.join(ROOT, token). The ContainedPath is
now carried through to the probe, so the validated value is the probed one.
- src/init.cts computed skillPathContained and DISCARDED it, re-joining from
the raw input for the existsSync and read. The branded type exists to make
that a type error and here it was inert.
AND THE OVER-CORRECTION OF THAT FIX, caught before it shipped. The first attempt
also substituted the validated value into the EMITTED `ref` for a global skill.
That value is a display token, not a path anything reads through — the only fs
access in that branch runs on the lexical path beforehand — so substituting it
changed emitted output two ways: it is realpath-resolved, so a symlinked global
skills directory would have emitted its resolved target instead of the user's
own path, and it came from path.join, so Windows would have emitted a backslash
where the template has a literal '/'. Restored, with the distinction recorded:
the containment check there is a GATE, not a path producer.
A TEST THAT COULD NOT FAIL. The first symlink regression planted its symlink
from inside a hooked fs.readdirSync and never asserted the planting happened —
if the hook did not fire, the "nothing was written outside" assertion passed
trivially, green against vulnerable code. It now asserts the plant, matching its
sibling. The other two were re-checked: one already asserted its equivalent, the
other plants synchronously and cannot silently no-op.
THE SYMLINK FIX ITSELF, now that the tests are proven red on the matrix.
isPathConfined is lexical by design and structurally cannot see a symlink; three
callers relied on it with no defense of their own. install-engine.cts:1608 and
install-profiles.cts:880 refuse to mkdir/write through a link — mkdirSync with
recursive:true does NOT throw on an existing symlink-to-directory, so a planted
link redirected the SKILL.md write outside the install root.
install-profiles.cts:755 refuses to read through one — statSync FOLLOWS links,
so an outside file's contents were returned and installed as a skill body. Each
mirrors the guard retired-artifact-cleanup.cts:77 already uses.
Severity stated accurately rather than dramatically: only the read at :755 needs
no race. _removeGsdEntries sweeps a pre-planted link at :1608 before the write
loop, and :880's stageDir is a fresh mkdtemp, so both of those require winning a
window. They are fixed as defense-in-depth, not as live exploits.
ALSO: the Changed changeset claimed "every command's observable behavior [is]
unchanged". Three rejection messages are reworded. It now says so, and says that
none of them reveals a host path it previously hid. A stale comment in
verify.cts still named validatePath; an init.cts warning hardcoded "resolves
outside the project directory" for a check that also rejects absolute paths, NUL
bytes and empty strings; and the rationale deleted with check-glossary-refs'
retired helper is restored, noting honestly that a rejected token is now
realpath-resolved before rejection rather than rejected by string comparison.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
sim
|
9953d02184 |
docs(#4653): describe the consolidated path-containment seam in the security model
The security model's input-validation section still described path traversal as a per-call check with a macOS symlink footnote. It now describes what actually exists: one predicate, a module-internal engine, three exported shapes none of which can hand back a usable path when the answer is unsafe, the branded return type, and the named acceptance policy — including the point the old wording invited a reader to get wrong, that allowing an absolute candidate does not relax containment. Also records the two checks deliberately NOT routed through the predicate and why each is narrower or stricter rather than a second opinion, so a later cleanup pass does not read them as stragglers. Required by the Changed changeset: scripts/lint-docs-required.cjs makes Added/Changed/Deprecated/Removed fragments demand a file under docs/, and CONTEXT.md is at the repo root, so the glossary entry alone would not have satisfied it. The lint currently reports invalid_pr against the mandated pr:0 placeholder and becomes meaningful after backfill. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
5967f1939a |
docs(#4653): record the path-containment seam in CONTEXT.md and add the changeset
The glossary had no entry for the containment predicate at all, which is the epic's actual deliverable. The new entry states the engine/export split, the branded type, the named acceptance policy, the preserved message contract, and — the part most likely to be undone by a later cleanup — the two implementations deliberately NOT collapsed and why each is stricter or narrower rather than duplicative. Glossary gate re-run: 269 refs, exit 0. Install-tree goldens regenerated and confirmed byte-identical rather than assumed unchanged; lint:ci exits 0, so no conformance-tier drift either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
7086dcbbc9 |
test(#4636): failing-first coverage for symlink escape past lexical confinement
Tests only; no fix. These MUST fail. Found while draining the containment duplicates in Phase 3, not by looking for it: isPathConfined's docstring claims its callers are kept symlink-safe upstream, and checking that claim showed it does not hold for three of its four call sites. isPathConfined is lexical by design and structurally cannot see a symlink, so where the claim fails there is no defense at all. install-engine.cts:1608 mkdirSync(recursive) + writeFileSync under dest install-profiles.cts:880 the same shape under stageDir install-profiles.cts:755 statSync/readFileSync, and statSync FOLLOWS links SEVERITY, STATED HONESTLY, because the three are not equal and the first impression was wrong. :755 is the real one and needs no race. It reads <capDir>/skills/<stem>/SKILL.md. Nothing prunes that path and nothing randomizes it, so a symlink planted there is followed and its content is returned — and then written into the install tree as a skill body. :1608 and :880 are defense-in-depth against a local race, not plain write-through. _removeGsdEntries (install-engine.cts:851-854) rmSyncs every entry whose name starts with kind.prefix, and the skill names ARE prefix+stem — so a symlink planted before the call is unlinked before the write loop reaches it. :880's stageDir is a fresh mkdtempSync path, so its name cannot be guessed in advance either. Both require winning a window. That is also why the first two tests do not simply pre-plant a link and call the function: written that way they PASS today, against vulnerable code, for the wrong reason. They instead open the window deterministically by hooking a call the function is guaranteed to make — the technique CLAUDE.md section 4 prescribes for injecting filesystem conditions, and the one already used in tests/install-runtime-artifacts.test.cjs. No sleeps, no concurrency, no flakiness: the window is opened by a synchronous side effect, not raced for. Each test asserts the OUTCOME rather than the mechanism — the canary file outside the root is byte-identical afterwards, or the installed content does not contain it — so a fix is free to close the hole any way it likes. Symlink creation is guarded so the Windows lanes skip rather than fail, and no test uses chmod 0o000, which is vacuous on a bench running as root. Folded into tests/install-write-confinement.test.cjs, the owning suite, whose existing F2/M1 coverage coincidentally proves the gap: it tests the case where the destination DIRECTORY is a symlink, which is already defended, and never the per-entry case. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
cd58aaabf4 |
refactor(#4653): drain the containment duplicates and record the two rulings
Phase 3 of epic #4636, stage 3c. ADR-4650 decision 6: a wrapper may decide HOW to degrade, never WHETHER a path is contained. Four implementations are drained on that rule; two are retained, with the reasons recorded rather than assumed. DRAINED — the containment decision now comes from the canonical predicate: scripts/check-glossary-refs.cjs local isWithinRoot deleted outright. src/installer-migrations.cts ensureInsideConfig keeps its throw and its lexical fullPath; only the decision moves. src/planning-inspect.cts isPathContained keeps must-exist as its own condition; only the decision moves. Two of those are wrappers rather than deletions, and each is a wrapper for a reason that would have been a silent behavior change if collapsed naively: - `isPathContained` returns FALSE for a path that does not exist, because fs.realpathSync throws ENOENT and its catch swallows it. The canonical predicate does the opposite: for a missing target it walks up to the nearest existing ancestor and ACCEPTS a not-yet-created path under the root. Its callers at planning-inspect.cts:747 and :839 guard a phaseDir immediately before readdirSync, so under a naive swap a missing phaseDir would stop reporting scope UNREADABLE and start throwing ENOENT out of readdirSync. Existence is therefore kept as an explicit local requirement. - `ensureInsideConfig` returns a LEXICAL fullPath that both callers consume for existsSync and for journal entries. The canonical predicate realpath-resolves, so if configDir is itself a symlink the two differ. The decision is canonical; the returned value stays lexical. Its message is likewise preserved verbatim, which is why this uses tryWithinRoot plus an explicit throw rather than assertWithinRoot. `isWithinRoot` in planning-inspect is left in place and documented: it is a pure comparison over paths the CALLER has already resolved, which readDocument does inline specifically to keep a third degradation shape (exists-but-unreadable vs absent) that neither isPathContained nor the canonical predicate expresses. It is the comparison step of one implementation, not a second implementation. RETAINED, DELIBERATELY — gsd-core/bin/gsd-tools.cjs. My own design document said "collapse" and that was wrong. The file carries an explicit comment forbidding it, and the comment is correct: its three checks reject symlinks OUTRIGHT, which is strictly stricter than the canonical predicate, not a reimplementation of it. The canonical predicate accepts a link whose target lands inside the root — for a restore that is still wrong, because writing through the link overwrites whatever it points at instead of materializing a regular file. Collapsing would have reintroduced that hole. The comment is updated to name the current exported predicate, to record that this was reviewed under this phase and deliberately not collapsed, and to note that isInsideDir treats target === root as NOT contained — the one implementation in the repo that does. THE configHome RULING — retained lexical, and a false safety claim corrected. isPathConfined stays lexical because two of its callers must validate a destSubpath BEFORE the mkdirSync that creates it (install-engine.cts:1608, install-profiles.cts:880), where realpath cannot resolve and a realpath-based predicate would reject every legitimate install. Its docstring's justification, however, did not survive being checked. It cited capability-source.cts:491,577,675 as the upstream symlink rejection that made the lexical form safe. Read directly: :491 is a blank line before assertSafeId's JSDoc and :577 is an entry-count budget check. Neither is a symlink check. The real guards are :585-586 and :671-674. Worse than stale line numbers, the claim that this "keeps every caller of this function's callers symlink-safe" is false: that rejection lives in capability-source's staging path and covers only the capability-loader route to assertDescriptorConfined. Three other callers do not reach it, and only retired-artifact-cleanup.cts:69 carries its own defense (its lstatSync check at :77). The docstring now states what is actually true and cites the lines that actually exist. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
7e7a239a65 |
refactor(#4653): replace the allowAbsolute flag with a named acceptance policy
Phase 3 of epic #4636, stage 3b. Satisfies #4653's criterion that `opts.allowAbsolute` become "a named acceptance policy on the predicate, not a per-call-site boolean". The flag was actively misleading at the call site. `{ allowAbsolute: true }` reads as "containment is relaxed here". It never was: an absolute path that resolves outside the root is rejected exactly as a traversal is. The flag only ever controlled whether an absolute candidate was CONSIDERED. On a security predicate that is the wrong thing for a reviewer to have to infer, and 31 call sites were asking them to infer it. PathAcceptance.RelativeOnly relative candidates only PathAcceptance.AbsoluteInsideRoot absolute accepted, containment unchanged The three exported wrappers take the policy and translate it inward. validatePath keeps its internal `{ allowAbsolute }` opts and its body untouched — the engine is not re-derived here either, only the exported surface is renamed. MEASURED, NOT ESTIMATED. 31 call sites across 10 files, counted by walking the AST with the repo's own @typescript-eslint/parser rather than grepping: a text match would have folded in the options-type declaration, default parameter values and comments. All 31 pass the literal `true`; none passes `false` or a dynamic value, so the migration is uniform and `RelativeOnly` is purely the existing default made nameable. audit.cts alone holds 18 of them. This migration is compiler-verified in a way the containment-value migration in the previous commit was not: the parameter type changed from an object to a string union, so any missed site is a build error rather than a silent behavioral difference. That is why a 31-site mechanical edit is acceptable in the phase whose stated risk is the width of mechanical change. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
26384ca988 |
refactor(#4653): make validatePath module-internal
Phase 3 of epic #4636, stage 3a. ADR-4650 decision 2: the engine stops being a public shape. The only exported containment surface is now assertWithinRoot / tryWithinRoot / requireSafePath, none of which can hand a caller a usable path when the answer is unsafe. The src/security.cts diff is one keyword. The engine body is byte-identical — the dangling-symlink existence-oracle closure, the ancestor canonicalization and the separator-aware boundary test are untouched, which is the whole constraint this phase operates under. WHAT THE TRANSLATION COST, AND THE RULE THAT KEPT IT AT ZERO. Roughly thirty test call sites consumed validatePath directly, including the two BLOCKER regressions that are this refactor's safety net. Translating them all to `tryWithinRoot(...) === null` would have looked correct and silently destroyed one of them: BLOCKER-1 asserts the rejection reason contains "unresolvable symbolic link", which is what distinguishes a DANGLING symlink from an ordinary escape. tryWithinRoot returns a bare null and cannot tell those apart, so that assertion would have degenerated into "it failed somehow" — and the existence-oracle closure could regress with the test still green. So the rule applied throughout is: an assertion on the rejection REASON goes through assertWithinRoot, whose throw carries the engine's message verbatim; only assertions on the boolean go through tryWithinRoot. Under that rule no coverage is lost. BLOCKER-1 still pins "unresolvable symbolic link" and BLOCKER-2 still pins the exact canonicalized resolved value. Three success-path tests came out BETTER than they went in. They previously carried `expected safe:true, got error: ${result.error}` as an assertion message; routing them through assertWithinRoot means an engine regression now surfaces the real reason in the failure itself rather than as a hand-built string. The two describe blocks named after validatePath are renamed — a block named for a symbol the module no longer exports is a false signpost. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
4ebcec7750 |
refactor(#4653): narrow the containment export and migrate all 15 validatePath sites
Phase 3 of epic #4636, stages 1-2. Implements ADR-4650 decisions 1 and 2: one containment predicate, and an exported shape that cannot hand a caller a usable path when the answer is unsafe. THE EXPORT IS NOW A PAIR, BOTH RETURNING A BRANDED TYPE: assertWithinRoot(candidate, root, label?, opts?) -> ContainedPath (throws) tryWithinRoot(candidate, root, opts?) -> ContainedPath | null ADR-4650 names only the throwing form. That does not survive contact with the call sites: findPhaseArtifact probes a direct path, then a .planning/ path, then each readdir entry, and throwing on the first miss breaks it outright. Six of the fifteen sites need a non-throwing check. Recorded here rather than papered over. WHY BRANDED. validatePath returns { safe, resolved, error } and populates resolved with the ESCAPING path on the traversal branch — so a caller who skips the boolean gets an attacker-controlled value precisely in the dangerous case. tryWithinRoot returns exactly null there; assertWithinRoot throws. A plain string is not assignable to ContainedPath, so a migrated site that validates one path and then passes a different one is now a type error rather than a silent bug. That is the defect this epic exists to close, and I introduced it twice in Phase 2. THE ENGINE IS UNTOUCHED. validatePath's body is not re-derived — the diff shows zero edits to the dangling-symlink existence-oracle closure, the ancestor canonicalization (macOS /var vs /private/var), or the separator-aware boundary test. Each was acquired as a bug fix and a re-derivation would silently lose one. requireSafePath now delegates to assertWithinRoot, so there is one implementation beneath both names; its return type is branded, which is why its 13 call sites compile unchanged. A TYPESCRIPT LIMITATION, FIXED AT THE ROOT RATHER THAN WORKED AROUND. TS applies never-return control-flow narrowing only when the callee is a function declaration or a const with an EXPLICIT type annotation. Both routers do `const { error } = io` — destructured, unannotated — so `error(...)` did not narrow ContainedPath | null and four sites wanted a dead `throw new Error('unreachable')` after it. Annotating the const (`const error: typeof io.error = io.error`) makes TS narrow properly and the dead throws are gone. That annotation has a large, deliberate consequence: with narrowing working, `@typescript-eslint/no-unnecessary-type-assertion` fires at 37 sites in commands.cts where `as string` / `!` existed ONLY to paper over the missing narrowing. They are removed. The rule is type-aware and fires only where the assertion changes nothing, and both forms erase at compile time, so the emitted behavior is unchanged — but the module loses 37 unchecked casts over string | undefined, which is the same class of "trust me" the containment work is removing. Widening the diff here buys that. TWO SITES LOSE DIAGNOSTIC TEXT, deliberately. tryWithinRoot has no error channel, so init.cts's agent-skills warning and cmdPrSubrepo's rejection now name the condition rather than echoing validatePath's message. verify.cts had already stopped echoing it on purpose — the message embeds absolute host paths — so this makes the three agree instead of two-of-three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
889c7efba0 |
test(#4653): failing-first coverage for the narrowed containment export
Phase 3 of epic #4636. Tests only; no implementation. These MUST fail. A refactor changes what a good test looks like: the behavior under test must be IDENTICAL before and after, so most of this phase's safety comes from invariance rather than new assertions. That safety net already exists and is untouched here — tests/security.test.cjs already pins the two engine behaviors a re-derivation would silently lose: :177 a DANGLING symlink to a non-existent OUTSIDE target stays safe:false (the existence-oracle closure) :213 a not-yet-created file in a not-yet-created subdir under a non-canonical base stays safe:true (ancestor canonicalization) plus traversal, absolute in/out, null bytes, empty, non-string, and requireSafePath's throw. Those 0 deletions are the point: if any of them had to change, the engine would have changed, and the engine is not supposed to. What is new is the export surface Phase 3 introduces: assertWithinRoot(candidate, root, label?, opts?) -> ContainedPath (throws) tryWithinRoot(candidate, root, opts?) -> ContainedPath | null Two shapes rather than one, because several call sites need a NON-throwing check — findPhaseArtifact probes a direct path, then a .planning/ path, then each readdir entry, and throwing on the first miss would break it outright. ADR-4650 names only the throwing form; this is the gap between the ADR and the call sites, recorded rather than papered over. Rows that exist because they are the ones nobody enumerates: - tryWithinRoot must return EXACTLY null on escape, and its return must not contain the escaping path's basename. The shape being replaced populates its "resolved" field with the escaping path precisely on the traversal branch, so a caller who ignores the boolean gets a usable attacker-controlled value. That is the defect the narrowing exists to remove, so it is asserted directly. - A seeded parity property: tryWithinRoot returns non-null if and only if assertWithinRoot does not throw, and the values agree. Two exported shapes over one engine is a divergence pair by construction. - The rejection text still contains the phrase "escapes allowed directory". Another suite surfaces it through a user-facing "reason" field, and a refactor is exactly where wording drifts unnoticed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a0270a7945 | Merge pull request #4666 from open-gsd/fix/4652-containment-at-boundaries | ||
|
Rezolv
|
7bcfbe4542 | docs(#4629): add ADR-4629 — STATE.md write intent beyond frontmatter (#4645) | ||
|
sim
|
e1f169a378 |
chore(#4652): backfill changeset PR number to 4666
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
f64f8a0e7b |
test(#4652): correct the absolute-filename test to match the basename guard
The test asserted that an absolute filename is folded under the pending dir and fails as "not found" rather than being rejected. That held for exactly one commit. The basename guard rejects any name containing a separator before any join happens, so an absolute path never reaches containment or the filesystem at all. Now asserts the USAGE rejection the CLI actually emits, verified by running it. All four outside-file protections are kept unchanged — the file still exists, its content is byte-identical, it never lands in completed/, and cleanup runs in finally. Those are the assertions that carry the security value; only the claim about HOW the rejection happens was stale. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
c7956c162b |
fix(#4652): a todo name must be a basename — containment alone cannot say that
The verification checkpoint caught a real design gap, not a flaky test. Confining sourcePath/targetPath within todosRoot correctly rejects `../../escaped`, which leaves the root. It does NOT reject these, because they all land inside it: ../sibling.md -> <todosRoot>/sibling.md escapes pending/, not the root a/../../b.md -> <todosRoot>/b.md same sub/name.md -> <pendingDir>/sub/name.md inside pending/, but nested Three committed tests asserted these must be rejected and were right: the design says "a todo name is a basename, not a path", and #4327 requires the resolved path stay inside "the todos root (pending and completed subdirs)". Containment against a root is structurally incapable of expressing "basename" — it answers "is this inside?", and all three are. The wrong tool was reaching for the wrong question. A basename guard now runs BEFORE any path is joined: reject on a `/` or `\` separator, on a path.basename / path.win32.basename mismatch, on `.` / `..`, and on a NUL byte. Both separators are checked explicitly because on POSIX a literal backslash is an ordinary filename character to path.basename but not to path.win32.basename or to the user's intent — this repo has a documented bug class for exactly that asymmetry. Same predicate shape as findPhaseArtifact in check-command-router.cts, so the two agree. Containment is kept as defense-in-depth rather than replaced. The basename guard is the specific rule; containment is the backstop. Message wording matters here and is deliberate: `sub/name.md` does NOT escape its allowed directory, so reusing the escape message would have stated something false. It now says the name must be a plain filename, not a path. docs/CLI-TOOLS.md corrected again, in the opposite direction from last time. The previous revision said an absolute filename is "folded under the root" and 404s — true then, false now: the basename guard rejects it before any join happens. Two corrections to one paragraph in one phase is the cost of documenting behavior while it is still moving; the paragraph now matches the shipped code. Verified through the real CLI, not by calling the built function directly: all eight rejection cases produce the new USAGE message; `ok.md` still completes and moves to completed/; `missing.md` still gives "Todo not found". Also corrected the now-stale comment above the isFile() check — it described `.`/`..` reaching that line, which the basename guard now prevents. The check itself stays: a bare basename can still name a directory, FIFO or socket in pending/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
9c20b7b40a |
fix(#4652): use the validated path, collapse the duplication, correct two false claims
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> |
||
|
sim
|
374300da17 |
fix(#4652): confine every boundary that joins argv to a managed root
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> |
||
|
sim
|
3925839f2a |
test(#4652): failing-first coverage for the four unconfined boundaries
Phase 2 of epic #4636, absorbing #4327 and #4354. Tests only; no fix. These MUST fail. Four CLI boundaries join externally-supplied input to a managed root with no containment validation. Each was driven through the real CLI and confirmed unconfined before the assertions were written: todo complete <name> src/commands.cts cmdTodoComplete check predicate --phase-dir <dir> check-command-router cmdCheckPredicate check decision-coverage-plan <dir> check-command-router resolvePath check gap-analysis.plan-post <dir> check-command-router Boundary 1 is worse than the issue describes. #4327 reports that a traversal name "resolves outside the todos root", which reads as an information leak. Measured, it is destructive: `todo complete ../../../../b1out/leak.md` exited 0, MOVED the outside file into completed/, and unlinked the original. The file was gone. cmdTodoComplete ends in fs.unlinkSync(sourcePath), so an unconfined name does not merely read across the boundary, it consumes across it. Boundary 2 reproduces #4354 exactly: a BLOCKING gate returned {"block":false,"details":{"match":true}} sourced entirely from a SECURITY.md in a directory the caller chose, outside the project. Boundaries 3 and 4 are not named in the epic. Both accepted an outside phase dir and exited 0. Rows that exist because they are the ones nobody enumerates: - ORDERING. A real file is created outside the todos root, then the traversal name targeting it is asserted rejected AND the outside file asserted still present and unmoved. #4327 notes the existence check and the move target BOTH follow the unvalidated join, so a rejection that lands after the read has already leaked — and, per the finding above, after the unlink has already destroyed. - `a/../../b.md` — looks balanced, resolves outside. - --dry-run must reject too; a preview must not leak a resolved outside path. - ${PHASE_DIR} interpolation into a command-exit-zero predicate is the SECOND predicate kind, which a fix inside gate-predicate-evaluator.cts would miss. - An absolute path INSIDE the project must still be accepted at every boundary — absolute is not a synonym for escaping. Cross-boundary rows loop over one shared list of escaping inputs and assert all four reject with the same shape, so four sites adopting one predicate cannot drift into four rejection contracts. Property tests cover BOTH directions — outside is always rejected, inside is always accepted. A property asserting only rejection is satisfied by a predicate that rejects everything, which is the degenerate-implementation trap found in Phase 1's review. Both are seeded. Regressions fold into the owning module suites rather than a new tests/fix-NNNN-*.test.cjs, per scripts/lint-regression-test-names.cjs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c99d7bb2be |
test(#4522): migrate core CLI/domain state batch to named timeout constants (#4662)
Batch 11 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/state-document.test.cjs, tests/phase.test.cjs, tests/commands.test.cjs, tests/pattern.test.cjs, tests/adr-612-bracket-coherence.test.cjs, tests/adr-612-bracket-read-tolerance.test.cjs, tests/milestone-lock.test.cjs, tests/init.test.cjs, tests/state-todos-render.test.cjs, tests/quick-batch.test.cjs, tests/graphify.test.cjs, and tests/effort-surface-axis.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 12 files from the rule's allowlist. Ground truth via eslint found 25 sites, not the issue's stated 24 (phase.test.cjs has 5, not 4) -- disclosed in the PR body. Reuses PROBE_TIMEOUT_MS, GIT_TIMEOUT_MS, and LOOP_HOOK_POINT_CLI_TIMEOUT_MS across 8 files. Adds two new shared constants to tests/helpers/timeouts.cjs (each independently arrived at by 2 files in this batch, crossing the promotion bar): PATHOLOGICAL_INPUT_TEST_TIMEOUT_MS (node:test's own per-test timeout option, not a subprocess bound) and GSD_TOOLS_CLI_MODERATE_TIMEOUT_MS (a single gsd-tools.cjs CLI subcommand spawn, distinct tier from PROBE_TIMEOUT_MS/LOOP_HOOK_POINT_CLI_TIMEOUT_MS). Adds 3 file-local constants for values used by only 1 file in this batch. No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
241646a43a |
fix(#4651): classify .env names by final extension, and close the trailing-dot alias bypass — Phase 1 of #4636 (#4659)
* test(#4651): failing-first coverage for final-extension classification Phase 1 of epic #4636, absorbing #4580. Tests only; no fix. These MUST fail. The guard classifies a name by comparing everything after `.env.` as one token against a set whose members are FINAL EXTENSIONS. So `.env.local.example` yields suffix `local.example`, which is not a member, and a committed secret-free template is refused. That is a category error, not strictness. Two arms are covered because the same classification is hand-rolled twice in one file: `isSecretBasename` for Read/Bash, and `globAltSelectsSecret` (`lit.startsWith('.env.')`) for Grep globs. Fixing one alone would ship a guard that allows `cat .env.local.example` while refusing `Grep --glob '.env.local.example'` — the same file, the same hook, opposite answers. A cross-arm parity loop over one shared list asserts the two cannot drift. Rows that exist because they are the ones nobody enumerates: - `.env.example.local` must stay BLOCKED. Final extension is `local`; this is dotenv's documented local-override convention and a real secret. Any fix shaped as "contains example" admits it. - `.env.local.` must stay BLOCKED — empty final extension is not a member. - `.env.` must stay ALLOWED. Note #4580's proposed patch adds `if (suffix === '') return true;`, which flips it to blocked; that breaks the existing `allows` assertion in this suite and broadens the protected set, which epic #4636's non-goals forbid. Not applied. - `.env.local.exam*` (partial glob literal) must stay BLOCKED — it can select `.env.local`, and a partial literal cannot be classified. - `*.example` and `*` must stay ALLOWED — regression protection on the arm that already works. Local behavioral repro of the current guard, confirming the tests fail for the right reason rather than by construction: .env.local.example rc=2 (blocked) <- the defect .env.example rc=0 (allowed) .env.local rc=2 (blocked) .env.example.local rc=2 (blocked) .env. rc=0 (allowed) glob .env.local.example rc=2 <- the second arm Regressions are folded into the owning module's suite rather than a new tests/fix-NNNN-*.test.cjs file, per scripts/lint-regression-test-names.cjs. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4651): classify by final extension so .env.<name>.example is readable Phase 1 of epic #4636, absorbing #4580. Implements ADR-4650 decision 5. The guard compared everything after `.env.` as ONE token against a set whose members are FINAL EXTENSIONS. `.env.local.example` yielded `local.example`, which is not a member, so a committed, secret-free template was refused — the guard blocked the one file that exists so nobody has to open the real `.env`. That is a category error, not strictness. The fix is not "add local.example to the set"; it is to compare the right token. hooks/lib/filename-classification.js now owns that distinction and is the only place it is expressed. Both arms are fixed, because the same classification was hand-rolled twice in this one file: - isSecretBasename (Read/Bash) now tests finalExtension(suffix). - globAltSelectsSecret (Grep --glob) split its first branch. With no wildcard the alternative IS a whole filename, so it is classified exactly via isSecretBasename. With a wildcard present the literal is only a PARTIAL prefix (`.env.local.exam*` can still select `.env.local`) and cannot be classified, so the original conservative rule stays. Fixing only the first would have shipped a self-contradicting guard: `cat .env.local.example` allowed while `Grep --glob '.env.local.example'` refused — same file, same hook, opposite answers. A cross-arm parity loop over one shared list now asserts the two cannot drift. Two deliberate departures from #4580's suggested patch, both verified: - Its `if (suffix === '') return true;` is NOT applied. That flips `.env.` from allowed to blocked, breaking an existing assertion in this suite and broadening the protected set, which epic #4636's non-goals forbid. - `fullSuffix` was drafted alongside finalExtension and removed before commit: zero production consumers, and none planned (Phases 2-4 are containment, duplicate draining and the path-join ratchet, none of which classify filenames). A zero-caller export is dead code. The distinction is pinned instead by a test asserting finalExtension('local.example') is 'example' and explicitly NOT 'local.example'. The protected set is unchanged. `.env.example.local` stays BLOCKED — its final extension is `local`, dotenv's local-override convention and a real secret; any fix shaped as "contains example" admits it. Scoped out by measurement, not assumption: src/validate.cts:395 and src/phase.cts:1674 also hand-roll lastIndexOf('.'), but both parse phase identifiers (`3.2` -> parent `3`), owned by the phase-id.cts seam. Folding them in would repeat this same category error in the opposite direction. Checkpoint 1 (prove RED) on the tests-only commit 91d3d6e1: outcome=failed, 26 failures / 45330, all 26 in the two new test files, zero pre-existing. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4651): document the widened template exemption and cover the Bash arm Two findings from the isolated adversarial review, both fixed in place. 1. The header's "Stated cost" passage named only the four literal template names, but since this change the exemption keys on the FINAL EXTENSION, so the trusted set is `.env.<anything>.{example,sample,template,dist}` — an unbounded family. The reviewer demonstrated it: `.env.prod-real-secrets.example` is allowed. That is the deliberate and necessary cost of fixing #4580, but it was materially larger than what the header disclosed, and a silent expansion of a security guard's trusted set is not acceptable. The passage now states the family, the concrete bypass, and that it applies across Read, Grep and Bash alike. 2. The cross-arm parity loop asserted Read and the exact-literal Grep glob but not Bash, whose `namesSecret` -> `isSecretBasename` path is genuinely distinct. The Bash arm was covered only by two one-off tests outside the shared table, so the table could not have caught a drift there. The loop now drives all three arms from the same TEMPLATES/SECRETS arrays. No classification logic changed. The Read-arm behavioral table is byte-identical before and after: rc=0 for .env.local.example / .env.example / .env. ; rc=2 for .env.local / .env.example.local / .env / .secrets. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4651): treat trailing dots and spaces as aliases of the protected file Closes a Windows path-alias bypass surfaced by the isolated adversarial review of this phase. Maintainer-approved as in scope. Win32 strips trailing dots and spaces from every path component, so `.env.`, `.env..`, `.env `, `.env. `, `.env .`, `.secrets.` and `.secrets ` all resolve to the real `.env` / `.secrets` on Windows. The guard allowed every one of them — a bypass of a file it already protects, reachable from Read, Grep and Bash alike. `isSecretBasename` now normalizes the basename before classifying. The whole class is fixed, not the reported name. `.env.` alone would have left `.secrets.` and the trailing-space forms open, which is the same one-cause-explains-every-failure trap this epic exists to close. Two consequences, both measured rather than assumed: - `.env.example.` flips blocked -> ALLOWED. It aliases the already-trusted `.env.example` template, so this is correct; it was previously blocked only because the trailing dot broke final-extension parsing. - A Bash token that is exactly `.env` plus trailing whitespace flips allowed -> BLOCKED. Verified this is CONSISTENCY, not a new false-positive class: the bare `.env` token was ALREADY blocked as an operand in the same position before this change, so the alias now simply behaves like the thing it aliases. The header's "No whitespace trimming" guarantee is preserved and now stated precisely: leading and interior whitespace is still never trimmed, so prose like a commit message mentioning `.env` in a sentence stays prose and stays allowed. Only TRAILING dots and spaces are stripped. Two tests pin that. This lands at the same behavior #4580's proposed `if (suffix === '') return true;` would have produced for `.env.`, which this phase earlier rejected. The rejection was correct on its stated grounds — that line broadens the protected set, which epic #4636's non-goals forbid. The Windows framing is different: normalizing an alias of an already-protected file is not a broadening, and the fix is reached by normalization rather than by special-casing an empty suffix, so it generalizes to `.secrets.` and the space forms. Cannot be reproduced on this host — the remote matrix is Linux-only and Windows coverage arrives from CI — so this ships on the Win32 path-normalization contract plus the CI lane, and that limitation is stated rather than implied. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4651): one owner for path segmentation, closing a Read/Grep divergence Four findings from the two-axis review, all fixed in place. The real one: the guard had TWO path-segmentation rules. `lastSegment` (used by Read and Bash via `namesSecret`) splits on both `/` and `\`, while `classifyGrepGlob` hand-rolled its own on `/` only. Measured: Read of `config\.env` rc=2 BLOCKED Grep --glob 'config\.env' rc=0 ALLOWED Same logical file, opposite answers — precisely the divergence this epic exists to remove, sitting inside the file this phase was already fixing. `lastSegment` now lives in hooks/lib/filename-classification.js and both arms call it. All five path-bearing cases (both separators) now agree. Note on how this was nearly missed: the first measurement of it reported "both allow", which looked like the reviewer was wrong. That reading was a measurement artifact — `config\.env` inside a printf'd JSON payload is an invalid escape, so the hook fails open at rc=0 and the test was observing JSON breakage rather than the predicate. Re-measured with correct escaping, the divergence is real. The tests added here use properly escaped literals and were verified by running, not by reasoning about the escaping. Also fixed: - Both fast-check properties were satisfied by a degenerate always-return-'' implementation: "never contains a dot / is a suffix" and "never ends with dot-or-space / is a prefix" are both trivially true of the empty string. They now additionally pin content preservation — the removed tail must match /^[. ]*$/, and a name with nothing to strip must come back unchanged. - The cross-arm parity loop used only bare basenames, so it could not have caught the divergence above. It now covers path-bearing names with both separators. - That loop's description overclaimed: Read and Bash BOTH route through `namesSecret`, so they are not independent paths; only the Grep glob arm is genuinely separate. The description now says so rather than implying three-way independence. - `normalizeWindowsBasename` runs on every platform, not only Windows. Its doc now states that explicitly: the guard must answer identically everywhere, and a name is judged by what Win32 would resolve it to. No classification logic changed; the 12-name regression sweep is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4651): regenerate install-tree goldens, correct the guard's user-facing docs Three things, all consequences of the fix rather than new behavior. 1. Install-tree goldens. `hooks/lib/filename-classification.js` is a SHIPPED file — package.json `files` includes `hooks` — so every per-runtime install tree gains a path. Checkpoint 2 failed on exactly this: 11 failures, all in tests/golden-install-tree.test.cjs, against 45356 passing. Regenerated via scripts/gen-install-tree-fixtures.cjs; 11 goldens changed, matching the 11 failures one-for-one. This ripple was identified at design time and then not acted on. Fleet's impact preview named golden-install-tree.test.cjs before any code was written, and 40-design.md records it under "Ripples identified". Writing a risk down is not the same as discharging it, and a full matrix run was spent discovering something already known. 2. docs/USER-GUIDE.md made a precise and now-false claim about the guard's protected set: it named `.env.example` / `.sample` / `.template` / `.dist` as the four exempt names. The exemption keys on the FINAL EXTENSION, so the exempt set is the unbounded family `.env.<anything>.{example,sample,template,dist}`. The page now states that family, the widened residual, that order matters and only the last segment counts (`.env.example.local` is a secret), and that trailing dots and spaces are stripped because Windows resolves them to the protected file. A wrong user-facing model of what a security guard protects is worth correcting even though Fixed/Security changesets are exempt from the required-docs rule. docs/ARCHITECTURE.md and docs/INVENTORY.md say "templates such as `.env.example` exempt" — non-exhaustive, still true, deliberately left alone. Same for the ja-JP / zh-CN / ko-KR / pt-BR rows, which carry the same hedged phrasing; hand-translating a security description unreviewed is not something to do silently. 3. Two changeset fragments, not one. A refusal corrected is `Fixed`; a bypass closed is `Security`. Folding the second into the first would under-report it in the release notes. Both carry `pr: 0` for backfill once the PR exists. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4651): backfill changeset PR number to 4659 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
8fa2c3dbcf |
docs(#4650): record the path-containment and filename-classification design lock (#4655)
Phase 0 of epic #4636. ADR-4650 fixes the decisions Phases 1-4 inherit, so that four phases do not each invent them independently. The load-bearing decision is that the engine and the exported shape are separable. A resolver-based, symlink-safe containment predicate already exists as validatePath, and building the epic's literal assertWithinRoot() from scratch would create a sixth implementation of the very thing this epic consolidates -- while risking silent loss of behavior validatePath acquired as bug fixes (a closed dangling-symlink existence oracle, ancestor canonicalization for non-canonical roots, a separator-aware boundary test). But the epic's other clause is correct and lands on the current export: validatePath returns a boolean a caller can forget to check, and populates `resolved` with the escaping path precisely on the traversal branch. While that form stays exported, the Phase-4 ratchet could only assert that a helper was called -- validatePath(x, root).resolved would pass the rule. So: preserve the engine, narrow the export. assertWithinRoot becomes the only export and yields a branded ContainedPath. Also recorded, each found by measurement rather than from the epic text: - The rejection message text is a real contract. tests/quick-batch.test.cjs asserts a user-facing `reason` field matches /escapes allowed directory/, so the string reaches CLI consumers and Phase 3 must preserve it verbatim. - Two further unconfined boundaries the epic does not enumerate: resolvePath and gap-analysis.plan-post, both in check-command-router.cts. - --phase-dir also interpolates into ${PHASE_DIR} for command-exit-zero, so confining at the boundary covers both predicate kinds; the evaluator stays fs-free. - opts.allowAbsolute is a per-call-site liberality knob, which is an acceptance policy living exactly where this ADR says it must not. - planning-inspect's isWithinRoot is deliberately pure-string with no I/O; its contract differs, so Phase 3 decides rather than assumes. The acceptance policy is stated once: conservative about the resource, exact about the classification. #4580's guard was not too strict, it was wrong -- a category error comparing a whole suffix against a set of final extensions. ADR opens as Proposed; ratified at Phase 4 closeout per docs/adr/README.md. No changeset: the diff touches docs/adr/ only, which is outside USER_FACING_PREFIXES in scripts/changeset/lint.cjs. Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a2331c01f1 |
fix(#4568): widen the phase-number regex to accept N-segment ids at 6 shell/markdown sites (#4646)
* test(#4568): pin the N-segment phase-grammar defect across all 6 shell/markdown sites Manually traced against the current tree: the validating regex at code-review.md rejects a 3-segment id (23.1.2), and execute-plan.md's extraction truncates a 23.1.2-01-PLAN.md filename down to 1.2-01. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4568): widen the phase-number regex to accept N-segment ids at all 6 shell/markdown sites Widens `?` to `*` on the dotted-segment group at all 6 sites (byte-identical behavior for 1- and 2-segment ids, character class unchanged): code-review.md, code-review-fix.md, gsd-code-fixer.md, gsd-code-fixer.compact.md (validating sites, plus their comment/error-message text), execute-plan.md's plan-filename extraction, and plan-phase.md's --research-phase flag capture. Also disambiguates the nsegment-phase-grammar test's plan-phase.md anchor, which was matching an unrelated earlier `--research-phase` occurrence (line 77's generic-value capture) instead of the targeted site (line 131). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): extend lint-phase-id-drift to ban the single-segment phase regex in workflows/ and agents/ Adds findSingleSegmentPhaseRegexDrift, banning the bounded `[0-9]+(\.[0-9]+)?` shape (and its \d/doubled-backslash near-variants) on any phase-carrying line across gsd-core/workflows/**/*.md, gsd-core/references/**/*.md, and the newly-scanned agents/**/*.md, sanctioned the same way as the existing shell-arith rule. Wired into scanAll; confirmed zero violations against the real tree post-#4568 fix. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4568): add Fixed changeset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: regenerate conformance-tier manifests for the new test file The emitted-attribution gate also flags 4 files growing: code-review-fix.md (+21 bytes), code-review.md (+21 bytes), gsd-code-fixer.compact.md (+9 bytes), gsd-code-fixer.md (+6 bytes). The growth is the fix itself: each site's validation regex widened from a bounded single-optional-dotted-segment shape to the unbounded form, and the accompanying comment/error-message text grew by a few characters to mention the new 3-segment example. Emitted-Drift-Ack-Growth: code-review-fix.md — widens the phase-number validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the comment/error text (#4568) Emitted-Drift-Ack-Growth: code-review.md — widens the phase-number validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the comment/error text (#4568) Emitted-Drift-Ack-Growth: gsd-code-fixer.compact.md — widens the padded_phase validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the error text (#4568) Emitted-Drift-Ack-Growth: gsd-code-fixer.md — widens the padded_phase validation regex from a bounded single-dotted-segment shape to accept N-segment ids, and adds a 3-segment example to the comment/error text (#4568) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4568): backfill changeset pr number to 4646 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
4d65c248e5 |
fix(#4641): make test-conformance the sole Windows selector and narrow the tier to 28.5% (#4643)
* test(#4641): failing-first tests for the tier ceiling and a single Windows selector Tests only, committed ahead of the implementation so the RED run is real. - tests/platform-conformance-tier.test.cjs: tier-size ceiling asserted as a ratio against a live denominator (Windows 33%, macOS 25%); per-helper negative cases proving seam calls and path-call-plus-slash-literal are not platform signals; positive pins that genuine platform content, seam-bypassing spawns, chmod and symlink still classify in; macOS signal set and generated list unchanged. - tests/ci-full-lane-sharding.test.cjs: the test job has zero windows-latest rows and test-conformance still has 3 windows + 1 macOS. - tests/ci-test-scope.test.cjs: windows_tests is absent rather than empty, a non-tier test file no longer forces full_matrix, a RULE-pulled windows-hint test does, and resolveSelection rejects the retired windows scope. Refs #4589, #4591, #4592, #4593, #4603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): delete the second Windows selector and narrow the conformance tier Epic #4589's goal — the OS-agnostic bulk on Linux, a small explicitly-scoped conformance tier on real Windows/macOS — was not met. Measured on PR #4640 (run 34618834118): 7 non-Linux jobs, a 546/930 (58.7%) "tier", and 5 of 7 changed test files running on a real Windows runner twice. Two selectors, only one in the epic's scope. The test job's three scope:windows shards predate the epic (#494, sharded #3057) and gate on product_changed, not full_matrix, so they fire on every product PR whatever Phase 3's classifier decides. They are deleted; test-conformance becomes the sole Windows selector, as it already was for macOS. Non-Linux jobs 7 -> 4. Gating the lane instead was rejected as provably redundant: for a test file reachesConformanceTierOrSeam is literally CONFORMANCE_TIER_FILES.includes(file), and that same predicate sets full_matrix, which turns test-conformance on. Every file a gated lane would run is already covered in the same run. The lane's one non-redundant residue -- RULE-pulled tests matched by the isWindowsHint filename heuristic -- is ported into reachesConformanceTierOrSeam so it sets full_matrix instead of feeding a parallel lane. Two detectors matched the repo's own test idiom rather than any platform signal and carried 226 of the tier's sole-signal membership against 41 for the other eight: process-seam-subprocess (335 files, 118 unique) matches the tests/helpers.cjs entry points nearly every CLI test uses, and going through the seam is the opposite of a platform signal since shell-command-projection takes platform as an injected parameter; hardcoded-path-vs-path-call (328, 108) needs only a path call anywhere plus a slash literal anywhere, and that class is already enforced by ADR-1703's Linux-runnable ESLint rules. Both are removed. Tier 546 -> 254 (27.3%). src/ reachability is unchanged at 28 files, measured. Adds the size gate Phase 2 never had, as a ratio against a live denominator so it cannot stop binding as the suite grows. 292 files leave real-OS Windows execution. The drop-out set was audited: 14 have a platform-suggestive filename and all 14 are static source-text analyses or seam-mediated CLI tests. raw-child-process was investigated as a suspected false negative and left unchanged -- relaxing it adds 13 files, all false positives. macOS is untouched: MACOS_CATEGORIES is a separate array and the regenerated macos-conformance-tier.generated.cjs is byte-identical at 196 files. Fixes #4641 Refs #4589, #4591, #4592, #4593, #4603 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): register the new ADR path in the docs-guard exempt baseline tests/ci-test-scope.test.cjs references docs/adr/4641-windows-selector-consolidation.md in a comment justifying the retired windows scope; lint-docs-guard-registration tracks that reference set, so the baseline needs the new path. Verified the exemption still holds: the path is prose, not a filesystem read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): make the escalation tier-backed and drop every hardcoded count Three follow-ups from measuring the first pass rather than trusting it. The windows-hint escalation now requires tier membership as well as the filename hint. Setting full_matrix runs test-conformance, which runs only the tier; escalating on a test that is NOT in the tier costs four jobs and still never runs that test on Windows. Measured over the 16 RULES entries the narrowed predicate fires on exactly the same rules today, so this is correct-by-construction rather than a behavior change. The broader variant -- escalate on any tier member a rule pulls in, ignoring the hint -- was measured at 14/16 rules and rejected as over-broad. Removes the hardcoded counts. A hardcoded macOS tier length of 196 broke as soon as the rebase pulled in one new test file from #4253, which is the whole argument against them: the ceilings are ratios against a live denominator, the committed lists are pinned by comparison against a fresh classification of the live tree, and the three named probe files now assert on their SIGNAL rather than on membership in a literal list -- asserting by filename is the exact error this PR fixes in the classifier. Regenerates both lists against the rebased tree. Same-tree figures are now 547 -> 255 of 931 eligible (58.8% -> 27.4%), 292 entries removed and none added; macOS is unchanged at 197 with a zero-line diff. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): restore real-shell-spawn coverage and repair assertions the narrowing broke An isolated adversarial review found a real false negative. Removing the blanket process-seam-subprocess detector also removed the only coverage for tests that spawn a REAL shell: tests/helpers/process-seam.cjs's runHook spawns options.interpreter via real spawnSync, so runHook('-c', [script], { interpreter: 'bash' }) runs a real bash binary executing a shell script extracted from workflow markdown. The seam argument holds for src/shell-command-projection.cts, which takes platform as an injected parameter; it does NOT hold for the test helpers, which spawn real binaries. Conflating the two is what made the blanket detector look purely noisy -- it was 99% noise wrapping a real signal. Adds a narrow shell-interpreter-spawn category keyed on a real interpreter option. Measured 2026-09-11: 33 files match, 9 were outside the tier and are added back, taking it 255 -> 264 of 931 (27.4% -> 28.4%), still under the 33% ceiling. All 9 confirmed by reading the matching source line, zero comment or fixture matches. runGit-alone and non-node-spawnSeam alternatives were measured and rejected -- each adds 9 files but misses the counterexample entirely. Fixes a real bug the suite caught: jobs.test is ubuntu-only now that its scope:windows rows are gone, so it must wire GSD_STRICT_LIVE_CONFIG_GUARD strictly rather than carrying the Windows report-only carve-out. The carve-out now lives solely on test-conformance, whose matrix does include windows. Repairs seven pre-existing assertions the category removal invalidated, preserving each case's purpose rather than deleting coverage, and converts the last hardcoded tier bounds to live-derived ratios -- including the macOS sanity range that was still a magic [100, 350]. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): keep the confinement test on a real OS via a documented allowlist A security review found tests/external-descriptor-confinement.test.cjs had dropped out of the Windows tier. It must stay in, and no content signal can express why: it exercises isPathConfined (src/external-descriptor-trust.cts), which uses the AMBIENT path module -- path.resolve(root, target) and path.sep -- with no injection. Its win32 semantics (drive letters, UNC, separator) are only reachable by actually running on Windows, and it is a security-relevant write-confinement gate. A content classifier cannot see 'this module reads the ambient path module', so no regex belongs here. Adds ALWAYS_REAL_OS, a Map of path -> recorded reason, unioned into the Windows tier only. A Map rather than a list so an entry without a reason is impossible by construction, and tests assert every entry names a file that exists on disk so a stale entry fails loudly instead of rotting. This is the centrally- enumerated single source of truth epic #4589 Phase 2 asked for and ADR-1703's portability-vocab.cjs already models -- deliberately not a heuristic. Windows tier 264 -> 265 of 931 (28.5%), still under the 33% ceiling. macOS is untouched and byte-identical: the win32 concern does not apply to a POSIX runner, and a test asserts the allowlist does not leak into that tier. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#4641): inject the path impl into isPathConfined and correct the ADR count Two review findings, both fixed rather than dispositioned. A security review found tests/external-descriptor-confinement.test.cjs had left real-OS execution. The allowlist pinned it back, but that only restored INCIDENTAL coverage: isPathConfined used the ambient path module, and its test carried POSIX-only literals, so a win32 confinement escape was unverified on every platform including Windows. isPathConfined now takes an optional third parameter carrying the path implementation, defaulting to the ambient module. Blast radius is CRITICAL -- 53 affected symbols across 19 files -- so the change is purely additive and every existing two-argument caller is byte-identical. Tests now inject path.win32 and path.posix, covering a different drive letter, a cross-drive absolute, backslash and forward-slash traversal, UNC, and the startsWith prefix-boundary bug (.gsdEVIL against root .gsd) on both separators. Proved load-bearing: dropping the + p.sep from the prefix check fails exactly the two boundary cases and nothing else. Callers' suites 149/149. The spec review caught an off-by-one: the ADR narrated a 264-file tier while the committed list holds 265. The ADR now records the full chain 547 -> 255 -> 264 -> 265 (28.5%). Also corrects a stale comment in scripts/docs-guard-registry.cjs that narrated classify() as zeroing windows_tests, a key this change removes -- kept as historical narration but labelled as such. Refs #4641 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#131): make the unwritable-HOME test actually test something Found by sweeping for the root-bypass class after fixing commit-files-deletion. This one is the silent variant, and it was broken twice over. First, the condition: the test made a fake HOME unwritable with chmod 0o500. The gsd-test Docker bench runs as root, root bypasses mode bits, so HOME stayed writable and the hostile condition never existed. Replaced with a HOME whose PARENT is a regular file, so every write under it fails ENOTDIR at the VFS layer for every uid -- no permission check is involved at all. Second, and more fundamental: the probe was npm --version, which on npm 11.19.0 performs zero filesystem I/O against HOME. Proven rather than assumed -- neutralizing runNpm()'s isolation turned the sibling test red while this one stayed green, so its assertion could never detect the regression it guards, on any uid, with or without the condition fix. npm config get cache was tried next and proved vacuous the same way (it only string-resolves the path). The probe is now npm cache verify, which really does mkdir _cacache under HOME. Re-proved load-bearing after the change: with isolation neutralized the test now fails with ENOTDIR on <blocker>/home/.npm/_cacache. tests/helpers.cjs was restored and verified diff-clean; suite 13/13. Refs #4641 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): correct the net drop-out figure in ADR-4641 The Consequences section still said 292 files leave real-OS Windows execution. That was the count before the narrow shell-interpreter-spawn replacement restored 9 and ALWAYS_REAL_OS pinned 1. Net is 282. Also names both real-binary categories rather than only raw-child-process, and clarifies that the 14-file filename audit was against the 292 initially dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record the rejected concentration ceiling and its measurement Applying Goodhart's own question to the new ceiling -- how would you make this metric look good without improving what it represents -- surfaces a real weakness: a ratio can be satisfied by inflating the denominator, so adding OS-agnostic tests loosens it without narrowing the tier. The obvious companion gate was a sole-signal concentration ceiling, since the original defect was one detector carrying half the tier. Measured and rejected: peak concentration post-fix is raw-child-process at 53/265 = 20.0%, against the historic offenders at 21.6% and 19.8%. Any threshold above 20% misses the original defect; any threshold below it fails on a legitimate category. The discriminator is whether a signal is platform-meaningful, which no threshold encodes. Weakness disclosed rather than covered by a gate that does not bind. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#4641): add the changeset fragment for the confinement-check change changeset-lint failed on PR #4643: the PR touches user-facing paths and carried no fragment. The earlier no-changeset call matched #4604's CI-only precedent and was correct then; it was not revisited once the PR grew a src/ change, which is my miss. The fragment describes the real user-visible improvement: the external-descriptor write-confinement check's Windows semantics are now verified deterministically rather than only when the suite happened to run on Windows. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): correct the tier count in TESTING-SUITES.md Said the tier narrowed from 546 to 254. The final committed list is 265 of 931 eligible (58.8% -> 28.5%) after the shell-interpreter-spawn replacement restored 9 files and ALWAYS_REAL_OS pinned 1. Same error class the spec review caught in the ADR, in a live reference page rather than a dated record, so it states the current truth rather than carrying an amendment note. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record the measured aggregate from real CI job lists Epic #4589's closeout asserted its reduction from a static count; #4641's acceptance criterion asks for a figure read off a real run. Recorded here: test.yml job count 21 -> 15 and non-Linux 7 -> 4, comparing PR #4640's run against this PR's own. Against the true pre-epic baseline of 9, that is 9 -> 4. Also states the caveat that a PR's total CHECK count is not a clean before/after comparison, since many gates are path-scoped and this change touches a broader path set -- the like-for-like figure is the test.yml job count. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): compare job totals the same way on both sides The measured-aggregate table put #4640's COMPLETED run total (21) against this run's count at matrix-expansion time (15). Those are not the same measurement: the completed total includes the post-test Coverage gate and baseline-publisher jobs. Counted identically, it is 21 -> 17. The load-bearing figure, non-Linux jobs 7 -> 4, was correct and is unchanged. Called out in the table rather than silently corrected -- comparing two differently-derived numbers is exactly the error class this ADR is about. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): record measured conformance wall-clock and date the stale counterfactual Adds the per-job durations from both runs. The honest read is that this is a correctness win more than a speed one: file count fell 52% but wall-clock only 9-29%, because what was removed were the cheap static tests and what remains is concentrated in expensive spawn-heavy work. Stated explicitly so nobody expects a future narrowing to buy time proportional to file count. The load-bearing figure is windows shard 3/3: 40m24s against a 45-minute cap on the 547-file tier -- 90% of the cliff #869 and #3057 were both filed about -- pulled back to 31m27s. macOS moved the wrong way (17m48s -> 21m02s) while its tier was UNCHANGED at 197 files, which fixes that as runner variance and is noted as a caution against reading a single duration as signal. Also dates the symlink-keyword counterfactual, which cited a 254-file tier from before the replacement category and allowlist took it to its final 265. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#4641): re-measure against the rebased tree and disclose the allowlist's zero next gained #4644 mid-flight, so every absolute count shifted. Re-measured on the tree this actually ships against (932 eligible): 548 -> 257 by detector removal, 257 -> 266 once shell-interpreter-spawn restores 9. Net 282 removed, 9 restored. macOS 198, unchanged by this PR. The percentages did not move across three rebases (58.8% -> 28.5%), which is the whole argument for expressing the ceilings as ratios rather than counts -- noted in the ADR since it is now evidence rather than assertion. Also discloses that ALWAYS_REAL_OS now contributes ZERO files: this PR's own win32 test cases introduced the literal win32 into the pinned file, so it classifies in on content via win32-darwin-literal. The entry stays and the reason is written down, because the file's real-OS need is a property of the code under test (isPathConfined reads the ambient path module), not of the test's text -- the text that currently saves it is incidental and could be refactored away silently. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
db4d8a9bae |
fix(#4619): execute-phase computes decimal/N-segment phase numbers without breaking shell arithmetic (#4644)
* fix(#4619): execute-phase computes decimal/N-segment phase numbers without breaking shell arithmetic $((10#${PHASE_NUMBER})) is a hard bash/zsh syntax error when PHASE_NUMBER is decimal (01.1, from an inserted phase) or N-segment (23.1.2) — neither is valid shell-arithmetic syntax at all, and the failed expansion aborts the rest of the snippet in a non-interactive shell. safe_resume_gate runs unconditionally before trusting STATE.md or dispatching any executor, so execute-phase failed at its own gate before the first executor on any decimal phase, regardless of workflow.tdd_mode. Regression from #4194. Fixes all 4 sites: safe_resume_gate and the TDD gate in workflows/execute-phase.md, the completion-signal spot-check fallback in workflows/execute-phase/steps/completion-reconciliation.md, and the executor gate validation example in references/tdd.md. Each now zero-strips only the leading integer segment into a *_INT variable (via %%.* / # parameter expansion — always valid shell syntax regardless of what follows) and keeps the remainder as an escaped-dot string for the anchored commit- scope regex, exactly as issue #4619 verified in both bash and zsh. A plain integer phase (12, 01) computes byte-identically to before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4619): pin the decimal/N-segment fix and characterize the pre-fix bug Behavioral coverage via real bash execution: the old $((10#01.1)) form throws (characterizes the bug, matching the issue's own reproduction); the new form resolves 01.1 -> 1\.1 and 23.1.2 -> 23\.1\.2, unchanged for plain integers (12 -> 12, 01 -> 1); the resulting anchored ERE matches feat(01.1-03):/test(1.1-3): and correctly rejects feat(01-03):, feat(01.2-03):, feat(011-03):, feat(12-03): for a decimal phase — mirroring issue #4619's own verified table exactly. Updates safe-resume-gate-anchoring.test.cjs's 4 existing source-text assertions (one per site) to the new fixed text. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): refine the shell-arith drift detector to distinguish safe from unsafe arithmetic With #4619's fix in place, the guard's original "ban $((10#... outright, match any occurrence" was too blunt: it flagged a comment merely mentioning the pattern in prose, the now-safe $((10#$PHASE_INT)) arithmetic on an already-%%.*-stripped integer, and the always-safe plan-id arithmetic (plan ids are plain integers, never decimal). Refines the detector to skip full-line comments and to only flag a captured variable/placeholder name that contains "phase" and does NOT end in _INT/_int — the naming convention the #4619 fix establishes at all four sites for "already reduced to a safe integer." A plan-id variable was never phase-number arithmetic in the first place and is excluded on the same basis. This closes epic #4634's D6 ("lint-phase-id-drift... passes with no new exemptions") and D7 ("a decimal and N-segment phase id survive an end-to-end execute-phase selection without error") for real — the guard now reports zero violations across all five .cts/.md rules. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: regenerate conformance-tier manifests for the new test file Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4619): cover the plain-padded-integer near-miss matrix too Review found the anchored-ERE near-miss coverage only exercised the decimal case (PHASE_NUMBER=01.1); issue #4619's own worked table also verifies the plain padded-integer case (01 -> PHASE_N=1) against its own near-miss set (matches 01-03, rejects 01.1-03/011-03/12-03). Adds the missing assertion. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4619): add Fixed changeset Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4619): correct JS backslash-escaping in safe-resume-gate anchoring test The test's string-literal assertions for the PHASE_FRAC//./\\.} pattern wrote only 2 backslash characters in JS source, which single-quoted-string parsing collapses to 1 real backslash at runtime -- but the workflow/reference files actually contain 2 raw backslash bytes at that position (needed so bash's ${var//pattern/replacement} produces the correct single-backslash output). Write 4 backslash characters in the JS source at all 4 occurrences so the runtime string matches the files' real bytes. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4619): refresh the committed compact-content benchmark baseline The new PHASE_INT/PHASE_FRAC arithmetic lines added to gsd-core/workflows/execute-phase.md shifted its committed compaction-ratio baseline. Regenerate via `node scripts/benchmark-compact-content.cjs --write`. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4619): note the safe_resume_gate arithmetic growth in the test header The emitted-attribution gate flags execute-phase.md growing 91253 -> 91846 bytes (593 bytes). The growth is the fix: the safe_resume_gate and TDD RED block now derive PHASE_INT/PHASE_FRAC before computing PHASE_N, so a decimal/N-segment phase number (e.g. 01.1, 2.3.1) zero-strips its leading integer segment via base-10 arithmetic instead of forcing the whole value through $((10#...)) and hitting a hard shell syntax error on the first dot. A blank line previously separated the Emitted-Drift-Ack-Growth trailer from the Co-Authored-By trailer below it, which splits git's trailer-block detection: only the last contiguous non-blank run of Key: Value lines at the end of a commit message is recognized as trailers, so the growth ack was silently read as ordinary body text and the differential-attribution gate failed with the growth unacknowledged. Joining the two trailers into one contiguous block fixes it. Emitted-Drift-Ack-Growth: execute-phase.md — adds PHASE_INT/PHASE_FRAC derivation to the safe_resume_gate and TDD RED commit-scope grep so a decimal/N-segment phase number zero-strips its leading integer segment via base-10 arithmetic instead of failing on a non-numeric value (#4619) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4208): replace chmod-based restore-failure injection with a root-proof git shim `tests/commit-files-deletion.test.cjs`'s two restore-failure tests simulated an unwritable index via a `post-index-change` hook running `chmod a-w` on the git dir. That relies on the OS enforcing the *owner's own* permission bits against itself, which uid 0 (a routine identity inside this repo's Docker-based gsd-test benches) does not: every DAC check short-circuits true for root, so the write the chmod meant to block silently succeeds, the restore comes back clean, and the disclosure/rollback behavior under test never actually gets exercised. This is CLAUDE.md's own named anti-pattern for I/O-failure injection ("Cross-platform test IO-failure injection" — chmod tricks fail under root Docker/CI). It is confirmed as the actual root cause here, not a production defect: `src/commands.cts`'s `restoreRemovedEntries`/rollback-disclosure logic (added by #4253, merged just before this run) was hand-traced and manually reproduced end to end on an unprivileged workstation against a freshly built `gsd-core/bin/lib/commands.cjs`, and it already produces exactly the `staging_failed` + "could not be restored" / "could NOT be restored during rollback" results both tests assert. The other `post-index-change`-based tests in this file (a `sleep` to force a timeout; a real `update-index` to flip a restored entry's mode) are unaffected because neither depends on a permission check — consistent with only the two chmod-based tests failing on the real remote run. Replaces the chmod fixture with a fake `git` placed ahead of the real one on PATH that fails only `update-index --add --cacheinfo` — the one call the restore makes — unconditionally, regardless of privilege level. Every other git invocation execs straight through to the real binary, so the rest of each scenario (`rm --cached`, the restore's own `ls-files` verification, etc.) is exercised exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4619): backfill changeset pr number to 4644 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4619): feed the bash fixture script via stdin, not argv, to fix Windows CI Passing the script as a `-c "<script>"` argv element made it subject to Windows' CreateProcess command-line argument encoding, which silently dropped the escaped-dot backslashes before bash ever saw them (observed on PR #4644's windows-latest CI shard: `1\.1` came back as `1.1`). Feeding the same script via stdin instead removes argv entirely from the transport, so there is nothing for Windows to re-encode. POSIX behavior is unchanged. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
5e0a7b1b56 |
fix(#4433,#4569,#4126): consolidate the phase-identity seam at name-validity, allocation, and branch-slug (#4640)
* fix(#4433): apply the name-validity guard symmetrically to every milestone-name capture extractMilestoneHeadingName already refused a punctuation-only captured name (#4134), but its two sibling capture sites in getMilestoneInfo — the STATE.md-anchored 🚧-bullet match and the no-STATE.md in-progress 🚧-bullet fallback — skipped straight to a bare truthiness check, so a malformed bullet whose only content past the version was punctuation passed through as a real milestone name. Extracts the existing inline /[\p{L}\p{N}]/u check into a single shared hasNameableContent predicate and applies it at all three capture sites, so the guard is one owner rather than a copy that happened to land at only one of them. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4433): pin the name-validity guard at all three milestone-name capture sites Failing-first coverage for the hasNameableContent extraction: a punctuation-only 🚧-bullet name must not surface as a real milestone name, either on the STATE.md-anchored path or the no-STATE.md in-progress fallback, while a real name (including a digits-only one) still resolves COMPLETE exactly as before. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4569): consolidate decimal-phase-number allocation into one function cmdPhaseInsert allocated its next decimal sub-phase number by scanning only on-disk phases/ directories and ### Phase N.M: headings, never the roadmap summary checklist — so a decimal that existed only as a checklist bullet (no heading yet, no on-disk directory yet) was invisible, and phase insert could silently reallocate an already-used number. It also always nested one level deeper under afterPhase, with no way to request a sibling. cmdPhaseNextDecimal had its own separate, near-identical two-source scan (missing the checklist source too) — the exact "duplicate implementations kept in sync instead of deleted" pattern this issue exists to close. Extracts scanExistingDecimalPhaseNumbers (directories + headings + checklist bullets, in one place) and migrates both cmdPhaseInsert and cmdPhaseNextDecimal onto it — deleting cmdPhaseNextDecimal's own copy rather than patching it in parallel. Adds an allocation: 'nested' | 'sibling' argument to cmdPhaseInsert (default 'nested', matching every existing caller's behavior); a top-level phase with no existing decimal segment falls back to nested since there is no sibling level to join. No CLI flag wires 'sibling' yet — that is a separate, disclosed follow-up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4569): pin decimal-allocation coverage across phase insert and next-decimal Failing-first coverage for scanExistingDecimalPhaseNumbers: a checklist-only decimal must not be reallocated by phase insert; a decimal present in heading, checklist, and on-disk directory simultaneously must count once; an unrelated phase family's checklist bullet must not cross-pollute; and phase next-decimal (migrated onto the same shared helper) must see a checklist-only decimal too, closing the same gap in a second command. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): extend the phase-id drift guard for name-validity and shell arithmetic The epic's ratchet requirement: lint-phase-id-drift.cjs must cover the two new predicates this PR introduces, and must also scan shell inside gsd-core/workflows/**/*.md and gsd-core/references/**/*.md for integer-coercing phase-number arithmetic ($((10#...)) and friends), which neither the canonical TypeScript module nor a source-only lint can reach. Adds findNameValidityDrift (bans re-deriving /[\p{L}\p{N}]/u outside hasNameableContent's owner file) and findShellPhaseArithDrift + scanMarkdownShellArith (bans $((10#...)) in workflow/reference markdown, sanctioned via <!-- phase-id-owner: --> on the preceding line). scanRepo keeps its existing, narrower contract (src/**/*.cts only) so the already-passing "the live repo is clean" test is untouched; a new scanAll merges both for the CLI's full report. Running the guard directly against this tree correctly reports the 7 pre-existing #4619 shell sites (workflows/execute-phase.md x4, workflows/execute-phase/steps/completion-reconciliation.md x2, references/tdd.md x1) as violations — demonstrating the ratchet works, not fixing them. #4619 is a live regression tracked and fixed separately; this PR does not touch those markdown files. A characterization test pins the current count of 7 so a future change to that number is investigated rather than silently absorbed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4569): wire --sibling through phase insert's CLI so the argument is reachable cmdPhaseInsert's allocation parameter had no CLI path to 'sibling' — shipped, untested, unreachable code (code-review finding: a guaranteed surviving mutant). Adds --sibling to phase insert's argument parsing, threads it through, and documents the flag in docs/CLI-TOOLS.md. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4569): exercise --sibling end-to-end through the real CLI Confirms --sibling joins afterPhase's parent decimal level rather than nesting, and falls back to nested when afterPhase has no existing decimal segment (no sibling level to join). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4634): demonstrate the two new drift detectors end-to-end via a planted violation The epic asks for the guard to be "demonstrated by watching it go red" on a reintroduced copy. The two new detectors (name-validity, shell-arith) had only unit-level fixture tests; mirrors the existing bracket-rule's planted-violation-in-a-temp-tree test for both, proving they're actually wired into scanRepo/scanMarkdownShellArith end-to-end, not just correct in isolation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): consolidate the drift guard's own owner-sanction-check logic Standards review flagged the "walk to nearest preceding non-blank line, check for a phase-id-owner comment" logic as duplicated across all four detector functions in a PR whose whole point is eliminating exactly that pattern. Extracts isSanctionedByPrecedingComment, shared by all four; behavior-preserving (verified: identical output before/after, same 7 known #4619 violations, zero token/bracket/name-validity). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): add Fixed changeset for the name-validity guard and allocation consolidation pr:0 placeholder — backfilled once the real PR number exists. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4126): consolidate branch-name slug substitution into one shared renderer cmdCommit (commands.cts) and cmdInitExecutePhase (init.cts) each independently implemented branch-name template substitution, and both substituted the literal string 'phase' when phase_slug was empty or undeliverable — producing a non-identifying branch name (gsd/phase-08-phase) that contradicted the honestly-reported phase_slug: null in the same payload. Same structural defect as the other three gaps in this epic: two consumers reimplementing one concept independently instead of sharing an owner. Adds renderPhaseBranchName (src/phase-id.cts) as the sole owner: a real slug substitutes normally; an empty/undeliverable one drops the {slug} token plus one adjacent separator (collapsing/trimming the result) rather than substituting a placeholder word, for the shipped default template and any user-configured shape alike. Both call sites now delegate to it; the old inline duplicates are deleted, not kept in sync. {project} substitution stays a separate step in init.cts, unchanged, since it is a config-level field with its own fallback contract. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * test(#4126): pin renderPhaseBranchName and both migrated call sites Property-based coverage for the shared renderer's degrade-path invariant (output, when non-null, never contains {slug} and never starts/ends with a separator), plus example coverage for real-slug substitution, empty/null/ non-string slug, token position at either edge, a doubled-separator template, and the only-{slug} -> null case. One regression test each in commands.test.cjs and init.test.cjs confirms a phase with no derivable slug no longer produces a branch name ending in the literal '-phase'. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: route scanExistingDecimalPhaseNumbers through the canonical enumeration owner Caught by an actual gsd-test run, not a hypothesis: the new decimal-scan helper (fix(#4569)) enumerated phases/ directories via a raw fs.readdirSync, which the pre-existing phase-enumeration drift guard (#3185/#3882) correctly flags as an unsanctioned re-derivation outside its canonical owner (listAllPhaseDirs / isSentinelPhaseId). Ironic given this epic's own thesis, and exactly why the guard exists: consolidating one seam can reintroduce drift in an adjacent one if the new code doesn't route through what's already there. Migrates the enumeration to listAllPhaseDirs; identical decimal-detection output for every existing case. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore(#4634): extend the drift guard for branch-slug fallback; fix a real regex bug Adds the fourth detector the epic's ratchet section names ("both branch-name sites"): bans a `.replace('{slug}', ... || 'phase')` call outright, sanctioned via renderPhaseBranchName or a dedicated comment. Wired into scanRepo (no per-file exemption — this is a banned anti-pattern everywhere, not a grammar with one legitimate owner). Now that #4126's fix (prior commit) has landed, scanRepo reports zero violations across all four .cts-scanning rules, restoring the simple "the live repo is clean" assertion instead of a pinned-known-count characterization. Also fixes a real bug an actual gsd-test run caught: findNameValidityDrift's regex didn't tolerate the doubled-backslash template-string form its own test claimed to cover (0 !== 1) — widened to \{1,2} matching TOKEN_DRIFT_RE's existing tolerance for the same two forms. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4126): document the {slug} degrade behavior; update changeset for the full seam Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: detectPhaseNumberFromFiles wrongly rejected bare, slug-less phase directories Caught by an actual gsd-test run on the #4126 regression test, not a hypothesis: a bare phase directory with no slug remainder (e.g. .planning/phases/01/) has extractPhaseToken correctly return "01" — which is simply identical to the directory name in that case, not its no-match fallback. A stale `token !== phaseDir` check treated that equality as "no numeric token found" and rejected it regardless, leaving phaseNum null and silently skipping cmdCommit's phase-branching block entirely (the commit proceeded on whatever branch was already checked out instead of the phase branch). phaseTokenShape.test(normalized) already excludes every genuine non-phase case on its own: extractPhaseToken's real no-match fallback only fires for a dirName that doesn't start with a digit or short letter+digit prefix, and normalizePhaseName's leading-\d+ requirement rejects those regardless. The equality check was redundant for real rejections and actively wrong for bare-numeric directories. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * chore: backfill changeset PR number to 4640 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
0xdhx
|
4cc2a466b5 |
fix(#4208): add --files-removed so commit --files can record a move without a directory pathspec (#4253)
* fix(#4208): add --files-removed so commit --files can record a move without a directory pathspec `cmdCommit`'s `--files` list can stage an addition but never a deletion: the #2014 guard skips a missing explicit entry because the filesystem cannot tell "moved away" from "not written yet". A caller that moves a file therefore had two forms, both wrong — a directory entry records the move but also commits every unrelated file in that directory (a concurrent session's in-flight todo, in the unattended execute-phase sweep), and a file entry leaves the old path's deletion dangling with the todo tracked at both paths. `--files-removed <paths>` is the caller-declared delete intent. Each entry names a file, or a directory whose tracked-but-absent files are the removals; those paths are staged with `git rm --cached` and join the commit pathspec. `--files` keeps its skip-if-missing contract untouched. A file entry still present on disk fails the commit closed with the existing staging-failure rollback; a never-tracked path is a no-op. `--files-removed` alone is a declared scope, not the unscoped .planning/ sweep. The dispatcher previously folded every non-flag token after `--files` into that list, so a second list flag could not exist; each list now runs from its flag to the next `--` token. The execute-phase todo sweep names the moved todos on both sides from CLOSED[@], and cleanup's archive commit moves .planning/phases/ and .planning/quick/ under --files-removed. Fixes #4208 Emitted-Drift-Ack-Growth: cleanup.md — the archive commit moves phases/ and quick/ under --files-removed; the growth is one paragraph stating why those two directories must not be --files entries * chore(#4208): set changeset fragment pr to 4253 * fix(#4208): fit execute-phase.md under the ADR-857 ceiling and re-point the #2415 guard Three CI failures, all consequences of this PR's own change. 1. gsd-core/workflows/execute-phase.md was 93,577 bytes against the ADR-857 Phase 6 margin gate's <= 93,400 (hard ceiling 93,600). The three-line rationale comment plus the four-line array-building block added 318 bytes to a file that had only 141 of headroom on next. Move the rationale to docs/CLI-TOOLS.md -- which this PR already extends with the --files-removed contract, and which is where the ADR-857 gate wants call-site detail to live rather than in the host workflow -- and fold the array build onto one line. 93,577 -> 93,372. 2/3. tests/close-phase-todos-stage-deletion.test.cjs pinned the #2415 guarantee to its old MECHANISM: it regex-matched the literal .planning/todos/{completed,pending}/ directory pathspecs in the commit --files list. This PR deliberately replaced those with named files (a directory entry also committed an unrelated todo a concurrent session dropped in mid-close), so the guard failed on a change it should have accepted. Re-point it at the new mechanism without weakening it: assert the ADDED array reaches --files, the REMOVED array reaches --files-removed, STATE.md is still committed, and -- newly -- that the two arrays are built from $COMPLETED_DIR and $PENDING_DIR respectively. Verified by negative control: deleting --files-removed "${REMOVED[@]}" from the workflow still fails the test, so the #2415 regression remains caught. Note for the merge queue: #4233 also grows execute-phase.md (+114). The two are additive -- different regions, no textual conflict -- so with both landed the file reaches ~93,486, over the 93,400 margin though under the 93,600 hard ceiling. Whichever merges second will need to reclaim ~86 bytes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_0183892Y3fxxirte4WNmBKbv * fix(#4208): reclaim execute-phase.md bytes so the PR is net-neutral under the ADR-857 margin Rebasing onto next surfaced the byte-gate collision flagged earlier on this PR: #4284 grew execute-phase.md by 95 bytes (93,259 -> 93,354), so this PR's +113 landed at 93,467 against the <= 93,400 margin in tests/claude-orchestration.test.cjs. Compact the close_phase_todos step this PR already edits -- drop the PHASE_NUM indirection, fold the normaliser and the match guard, print the closed list with one printf, shorten the step's prose -- without touching the mechanism the #2415 guard pins (ADDED/REMOVED arrays, the plain mv). 93,467 -> 93,349: 5 bytes under the base, so the PR no longer spends any of next's 46 bytes of headroom. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): classify absent index entries before staging a removal; restore removed entries exactly on rollback Review of #4253 found three Majors with one root cause: the removal side judged presence by fs.lstatSync alone, where the addition side already reads `git ls-files -v` state. Absence from the worktree is not removal: - a submodule gitlink (mode 160000) whose directory was deleted by hand lists like a file and was `rm --cached` with no .gitmodules cleanup; - a skip-worktree path is never materialised by a cone-mode sparse checkout, so a directory entry over a sparse-excluded tree dropped that whole tree from the index; - an assume-unchanged path's worktree state is not something git itself consults; - an intent-to-add entry (`git add -N`) renders as a plain cached entry on the empty blob, yet nothing tracked exists to remove and no rollback can restore the flag. The index listing now carries each entry's `ls-files -v -s` tag, mode and stage. Only a plain cached (H), stage-0, non-gitlink entry is a removal candidate; every other state is left alone under a directory entry (exactly like a present file) and fails closed when named directly, with the state in the error. "Named directly" is decided on RESOLVED paths, not strings -- realpath of the longest existing prefix with the absent tail re-appended: an absolute path, `./x`, `--cwd`, or a symlinked spelling of the tree (macOS `/var` -> `/private/var`, where `process.cwd()` is the real path and the caller's absolute path is not -- CI on this round's first push) all resolve to the same entry, where a string compare against git's cwd-relative output silently took the directory polarity (pre-push review, driven; the symlink case is driven with an aliased fixture directory). The enumeration's domain is what `ls-files -v -s` can emit for an index entry, stated at the classifier. The third Major -- on an unborn HEAD a successful `rm --cached` was never rolled back when a later entry failed -- is fixed differently from the review's suggestion. Pushing the path into stagedPaths would put it on the commit pathspec, which a root commit refuses ("pathspec did not match", driven), and `git reset -- <path>` cannot restore an entry with no HEAD anyway. Instead every index entry this call removes is recorded (mode, blob) before the `rm` and put back with `update-index --cacheinfo` on rollback. That also restores a caller-pre-staged blob at a removed path exactly, where a reset would have silently replaced it with HEAD's version. The rollback is best-effort, as the addition-side reset already was, and the docs say so. Eight tests: gitlink under a directory entry, named directly, and named by absolute path; skip-worktree both forms; intent-to-add both forms; assume-unchanged named; unborn-HEAD partial failure restores the removal; pre-staged blob survives the rollback. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): drop the empty fenced block left dangling in cleanup.md's commit step Review nit on #4253: inserting the --files-removed rationale between the original bash block and its closing fence left an empty ```bash``` pair before </step>. Harmless at runtime, a formatting artifact of this PR's own diff; removed. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): a boolean flag inside a commit path list no longer ends the list Review minor on #4253: collectList stopped at the next `--` token, so a positional wedged between a boolean flag and the next list flag (`--files a --amend b --files-removed c`) was claimed by neither list and silently dropped -- a regression in shape against the old slice-to-end parse, which filtered `--` tokens and kept `b`. No current call site interleaves that way, but the gap was real. A list now runs to the next LIST flag (`--files` / `--files-removed`) and skips boolean flags on the way, and a REPEATED list flag merges its runs (`--files a --files b` -> [a, b]) as the slice-to-end parse did -- a first cut stopped at the repeat and dropped `b`, the same silent-drop shape one level over (pre-post comment audit). The only change #4208 makes to parsing is that a second list flag can exist. Tests: STATE.md wedged between --no-verify and --files-removed lands in the commit; both runs of a repeated --files reach it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * test(#4208): drive the reappearance window with a post-index-change hook Review nit on #4253: the defensive re-check for a file recreated between the absence test and `git rm --cached` -- the concurrent-session race this PR's own changeset names -- had no test. git fires post-index-change the moment `rm --cached` writes the index, so a hook that copies the file back exactly then exercises the window deterministically. The call reports staging_failed / "reappeared on disk", commits nothing, and the rollback restores the removed entry. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01MkU9ueBNHQzCpc3du5rKXm * fix(#4208): restore a staged removal when the call records nothing A `git rm --cached` that succeeds mutates the index whether or not a commit follows. Only the staging-failure rollback put those entries back, so a call that reached `nothing_to_commit` reported no state change while the removal sat staged -- riding along on the caller's next commit. The review named the unborn-HEAD, removal-only shape. Keying on `headExists` would have fixed half of it: the guard also fires with a real HEAD when the removed path is index-only (added, never committed), because `diff HEAD` reads clean with the path absent on both sides. Both shapes now restore, at both `nothing_to_commit` exits. The failure exits are deliberately left alone -- they report a failure rather than no-change, and the addition side leaves its own staged paths there too. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * refactor(#4208): lift declared-removal staging out of the cmdCommit hotspot `cmdCommit` was a critical-risk hotspot before this flag existed, and #4208 had inlined another ~270 lines into it. `stageDeclaredRemovals(cwd, removedDeclared)` now owns the index-state classification, path canonicalisation and entry recording, returning the pathspec entries and the recorded removals its caller merges. Pure motion: no branch, message or probe changed. Only the two accumulators became local names, and `restoreRemovedEntries` stays with the caller because the exits that restore are the caller's. cmdCommit 888 -> 625 lines here; the extracted helper is 277. (Figures corrected after publication: an earlier version of this message said 854 -> 591 and claimed the result was below cmdCommit's pre-#4208 shape. Both were wrong -- the count came from a faulty brace scanner, and `next`'s cmdCommit is 581, so this is above it, not below.) Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): property-test the two-list commit parser RULESET.TESTS.property-based-testing asks a parser for at least one property test asserting a domain invariant; `collectList` had only hand-picked examples, one per shape a review round had already broken. Hoisted it to module scope as `collectListFlagValues` and exported it in the file's existing exported-for-tests convention -- a parser reachable only by spawning the CLI can be tested one example at a time and no faster. Three properties over generated argv: every positional lands in exactly the run open at it whatever the flag order or count; no positional after the first list flag is dropped or double-claimed; and with `--files-removed` absent the parse equals the pre-#4208 slice-to-end parse. Controlled against two mutants -- a run ending at any `--` token, and a repeated list flag that does not merge -- each of which the properties catch. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): pin cleanup.md's archive commit to --files-removed execute-phase.md's rewrite is pinned by the #2415 guard in this file; cleanup.md's equivalent was not, so reverting its routing would have been caught by nothing -- the mechanism's unit tests never read this file and pass either way. Asserts the two archived directories are under --files-removed and NOT under --files (where a directory entry sweeps in a concurrent session's in-flight writes), and that the destinations and STATE.md stay on the additive half. Controlled by restoring the pre-#4208 sweep, which fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): pin that a symlink to a directory is one tracked path Review of #4253 read the `lstatSync(...).isDirectory()` test as a symlink-following defect. Driving it says the opposite: git tracks the link as a single blob (mode 120000) and does not traverse it, so the tracked paths "under" it live at the real directory and were never named by the caller. Following the link would stage those -- the directory sweep #4208 exists to remove -- while the named entry still sat present on disk. Pinned rather than changed, with the premise driven in the test body. Swapping `lstatSync` for `statSync` -- the prescription as written -- fails it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * chore(#4208): refresh the compact-content baseline for this PR's execute-phase edit The base range added `tests/benchmark-compact-content.test.cjs` and a committed token baseline over the compacted workflows. This PR edits `gsd-core/workflows/execute-phase.md`, so the baseline drifts by +12 tokens on that entry and on the aggregate. Refreshed with `node scripts/benchmark-compact-content.cjs --write`; the diff is those two entries and nothing else. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): report a removal the call could not put back Round review of this round found the restore itself unchecked: the helper ignored `update-index`'s exit code, so a FAILED restore still reported `nothing_to_commit` -- the same false "no state changed" the restore exists to prevent, surviving one level down on the restore-failure path. It now returns a boolean. The two no-change exits report `staging_failed` naming the paths left staged; the staging-failure rollback still ignores it, deliberately, because it is already reporting a failure and an unwritable index is usually the failure being reported. Driven with a post-index-change hook that makes the git dir unwritable the moment `rm --cached` lands, so the restore cannot take its lock. Reverting both guards fails the test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): disclose a removal the rollback could not restore Round review refuted the reasoning behind leaving the rollback path's restore unchecked. The claim was that this exit is already reporting a failure, so the restore's result adds nothing. The counterexample is the ordinary case: the reported failure is usually a DIFFERENT cause -- a contradictory declaration, a reappeared path -- so a caller reading `failures` sees only that cause and learns nothing about the removal still sitting in its index. The rollback now appends a disclosure entry per un-restored removal, naming the path. The reason and `file` still report the failure that caused the rollback; the disclosure is additive. Also moves the restore-failure test's chmod into a `finally`: `t.after` runs AFTER the parent `afterEach`, so a throw before it left the fixture undeletable. Both driven; reverting the disclosure fails the new test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): decide index state by observation, never by an exit code The restore added two commits earlier keyed both its record decision and its success verdict on git's exit code. An exit code answers "did the command succeed", never "did the index change" -- execGit collapses a spawn timeout to a non-zero exit, and a killed git can already have written the index. Round review drove four failures from that one assumption, in both directions: - a failed `rm` still contributed an entry, so the rollback disclosed a removal that was never staged (stale index.lock); - a timed-out `rm` whose write DID land contributed none, so a real mutation was neither restored nor disclosed; - a timed-out `update-index` whose write landed reported failure, publishing a "could NOT be restored" disclosure that was false; - and the read-back that replaced it omitted `-z`, so core.quotePath rendered `café.md` as `"caf\303\251.md"` and an exactly-restored entry read as not restored -- the same quoting defect this PR already fixed for `preStaged`. Everything now observes the index. A failed `rm` re-reads `ls-files -z` for the path: gone means this call owns the removal and records it; still there means nothing was staged; a probe that cannot answer becomes its own failure entry rather than an assumption. The restore verifies the same way, comparing the WHOLE entry (mode, blob, stage), because `--cacheinfo` restores all three and a path-only test accepts an entry that came back as something else. The verdict is three-valued -- `restored` / `not-restored` / `unverified` -- and the unverified wording says the restore could not be VERIFIED rather than that it failed. The rm's own failure is pushed ahead of any probe diagnostic so a timed-out removal keeps `timed_out: true` and its own message as the reported cause. Five regression cases, each negative-controlled against the shape it pins. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): treat a declared removal path as a path, not a pathspec An index path handed back to git is parsed as a PATHSPEC, and the removal side handed several back. Three driven harms, all of them the sweep-in this flag exists to remove, arriving through the operand rather than through a directory entry: - a tracked file literally named `.planning/*.md` made `rm --cached` GLOB: it removed `peer.md` and `stays.md` too, only the declared entry was recorded, so the rollback restored one of three and the other two rode out as staged deletions the result disclosed nowhere; - the same name reached `git commit -- <paths>`, which globbed and committed an undeclared `M peer.md` alongside the declared removal; - and the intent-to-add probe (`diff --cached` over the path) matched a STAGED PEER instead of itself, so an `add -N` entry was misclassified as ordinary content, removed, and restored by `--cacheinfo` -- which cannot restore the intent flag. It came back as a real staged addition. Every operand on this path is now `:(literal)`: the `rm`, both index probes, the intent-to-add probe, the restore read-back, the entry-level `ls-files` / `ls-tree`, and -- for the REMOVAL-derived entries only -- the downstream `ls-files` / dry-run / `diff HEAD` / `commit` pathspec. `--files` entries keep whatever pathspec behaviour they have today; that is not this change's to alter. `:(literal)` still resolves a directory to its descendants (driven), so the directory form is unchanged. Closes what an earlier cut of this commit declared as a residual: a filename beginning with `:` is now removable end to end, because the commit pathspec no longer reinterprets it. Also fixes a MINOR from the same review: cleanup.md's contract test checked the destinations' position relative to `--files-removed` but never that `--files` was present at all, so deleting the flag still passed. Un-literalising the seven sites fails three of the new tests. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * fix(#4208): scope the rollback to the caller's own name space Round review drove a rollback that destroyed the caller's own staged work. Two causes, one of them pre-existing: - `git diff --cached` prints REPO-relative paths whatever the cwd, while `stagedPaths` holds the caller's cwd-relative names. In a project nested inside its repo (`<repo>/sub/.planning/...`) the two name spaces never intersect, so `preStaged` matched NOTHING, every path landed in `toUnstage`, and the reset unstaged a caller-staged deletion and modification that this call had never touched. `--relative` makes the two sets comparable, and is a no-op when the project IS the repo root. This governs the `--files` side too and predates this flag. - the rollback's `reset` was the last place a removal-derived name reached git as a bare pathspec; it takes `asPathspec` like every other site. Driven on a nested fixture; dropping `--relative` fails the new test. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): gate six fixtures that Windows cannot construct CI's `test (windows-latest, 24, shard 2/3)` went red on this round. Two primitives the new fixtures rely on do not exist on Windows, both driven on a real Windows host rather than inferred: - a filename containing `*` or `:` cannot be created at all (`IOException` / `FileNotFoundException`), which is four of the pathspec fixtures; - `chmod` cannot make a directory unwritable — a write into a ReadOnly directory succeeds — so the two restore-failure fixtures cannot drive the failure they exist to drive. Each is skipped on win32 with its measured reason, in the repo's existing `{ skip: process.platform === 'win32' ? '<reason>' : false }` form. The behaviours they pin are platform-independent; only the fixtures are not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * test(#4208): build git's index-syntax path with forward slashes The remaining Windows red was mine, not the platform's: `git rev-parse :<path>` takes a forward-slash path, and `path.join` yields backslashes there, so git rejected it as an ambiguous argument. The hook in the same test already used the slash form. Not gated — the behaviour it pins is portable; only the argument was not. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_016gyGdweAdAG6nFv9Jx32vj * chore(#4208): refresh the compact-content baseline against the rebased base `next` moved the `new-project` split and the aggregate under this PR's execute-phase entry; regenerated with `scripts/benchmark-compact-content.cjs --write` so the only leaves differing from the base's copy are the execute-phase split and the aggregate it feeds. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUcGM4FWeZV4cqvR7QBtJh * chore(#4208): regenerate the macOS conformance tier for this PR's fixtures `next` gained the macOS-specific conformance tier (#4593) after this branch was cut. Its classifier (`scripts/gen-platform-conformance-tier.cjs --target macos`) now selects `tests/commit-files-deletion.test.cjs` on the `chmod-mode-bit` and `symlink-keyword` signals the PR's fixtures carry (the chmod-driven failed-restore cases and the symlink-to-directory case). Regenerated with `--target macos --write`; the platform tier was already in sync. The file was modified, not added, which is why the added-files check did not surface it. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FUcGM4FWeZV4cqvR7QBtJh --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
9f6f0d27dd |
docs(#4625): record the Codex native supervisor adapter as out-of-scope (#4637)
Codex's native subagent status is a fixed two-value enum
(CollabAgentToolCallStatus::{InProgress, Completed}) with no custom status
field, so the proposal's executing/verifying/completed triad cannot be
rendered in that view by anyone. Records the decision, and preserves the
reachable path -- codex exec --json ThreadEvents persisted by #4624, then
read at execute:wave:pre/post by an EoS capability.
Co-authored-by: sim <sim@local>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
|
||
|
Michel Moreira
|
1f84f45ed5 |
fix(#4259): fold shell continuations before the T6 docs-parity site scan (#4423)
* fix(#4259): fold shell continuations before the T6 docs-parity site scan The scan is $-anchored with [^\n]* on both sides of --grep=, so `git log` and `--grep=` had to share a physical line. A backslash-continued derivation — the natural way to write a git log carrying a long ERE — produced zero hits and T6 passed on it. Both generations of the assertion were defeated. The current anti-revert ban let a wrapped site through outright; at v1.12.0, where T6 instead asserted pattern conformance, a wrapped site was silently exempted from the very checks written to catch the macOS \b-no-op class, so it could have carried exactly the malformed pattern T6 exists to reject. A real candidate implementation for #3926 wrapped its derivation, passed T6, and was caught only by later manual review. Fold the continuations before matching rather than widening the regex: the assertion's message and its PHASE_SCOPE_NUM filter both assume one site is one string, and a [\s\S]*? would run the scan across unrelated statements. Correcting the input repairs everything built on the scan at once. The fold uses [ \t]* after the newline rather than \s* — the shell's own rule, and it cannot swallow a blank line and glue two unrelated statements. The scan is hoisted to findGrepSites so the controls can drive it directly: the continued form is caught, the same-line form still is, a benign --grep stays clean, a wrapped unrelated assignment is not glued, a continuation does not cross a blank line, and the live workflow files still report nothing — so this lands without editing any workflow to appease it. * fix(#4259): honor shell continuation boundaries * test(#4259): use canonical fenced-block scanner * test(#4259): centralize shell continuation scanning --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
aad96e0b5f |
test(#4521): migrate capability subsystem batch to named timeout constants (#4627)
Batch 10 of the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in tests/adr857-core-without-capabilities.test.cjs, tests/capability-cli.test.cjs, tests/capability-probe-fallback.test.cjs, tests/capability-state.test.cjs, tests/capability-trust.test.cjs, tests/capability-validator-task-content-resolver.test.cjs, and tests/capability-writer.test.cjs with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes these 7 files from the rule's allowlist. Reuses the existing PROBE_TIMEOUT_MS constant at 8 sites across 3 files. Adds 7 new file-local constants (no promotion to the shared helper needed this batch -- every new class is confined to exactly one file, below the two-file promotion bar): GSD_TOOLS_CLI_TIMEOUT_MS, FRAGMENT_PROBE_SNIPPET_TIMEOUT_MS, INSTALLED_RUNTIME_CLI_TIMEOUT_MS, FIXTURE_MCP_SERVER_TIMEOUT_VALUE, TASK_RESOLVER_FIXTURE_TIMEOUT_MS, TASK_RESOLVER_TIMEOUT_CEILING_MS, and TASK_RESOLVER_TIMEOUT_CEILING_PLUS_ONE_MS (the last two forming a boundary-coverage limit/limit+1 pair). No src/bin file touched, no numeric value changed anywhere. Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
0xdhx
|
7249bdddac |
fix(#4294): reserve a full progress bar for 100% and give the render half one owner (#4473)
* fix(#4294): reserve a full progress bar for 100% and give the render half one owner Six call sites each carried `Math.round((percent / 100) * width)` inline, and every copy rounded to a full bar before the percent reached 100: from 95 up at width 10, from 98 up at width 20. A project at 19/20 plans drew the same bar as a shipped one beside a number that said otherwise, and an out-of-range percent threw `RangeError` from the unguarded `'░'.repeat`. ADR-3180 Decision 7 gave the completion-RATIO derivation one owner (`clampPercentFromFraction`). This gives the RENDER half the same: `progressBarFilledCells` / `renderProgressBar` in phase-lifecycle.cts, with the `progress` table and bar renderers, the stats renderer, the gsd2 import writer, and #4231's `formatProgressMachineSegment` (which now serves both STATE.md writers) all drawing through it. Contract: below 100 the fill is held one cell short of the width, so only the saturating percents move (95-99 at width 10, 98-99 at width 20) and every other value in 0-100 renders as before — pinned by an exhaustive comparison against the legacy formula at both widths. Null / non-finite renders an empty bar; out-of-range is clamped, never thrown. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012hbFn24VWaxJBw8DUWjAmU * chore(#4294): set changeset fragment pr to 4473 * docs(#4294): correct the pre-fix inline call-site count to five The kernel's doc comment said SIX call sites carried their own `Math.round((percent / 100) * width)`. The base tree has five: three in `commands.cts` plus one each in `gsd2-import.cts` and `formatProgressMachineSegment`, the latter two using `/ 10` with the width already substituted (`pct` and `clamped` respectively). The six is #4294's count of consumers -- it counts `cmdStateUpdateProgress` and `syncCore` separately, but #4231 had already routed both through `formatProgressMachineSegment` (as it does `applyPostSyncPreservation`), so by this branch's base they share one copy. The comment now states the tree's count and records where the six comes from, so neither number reads as an error later. Comment-only; no behaviour change, and no change to compiled output. --------- Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
523be34133 |
fix(#4282): register PATTERNS.md as a canonical .planning/ artifact (#4618)
* test(#4282): prove PATTERNS.md is unrecognized by the artifact registry Regression test only, no fix yet: CANONICAL_EXACT in src/artifacts.cts was never updated when workflows/graduation.md started writing .planning/ PATTERNS.md, same omission class as the already-fixed #3224 (WINDOWS.md). Expected RED on this commit (src/artifacts.cts is unchanged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#4282): register PATTERNS.md as a canonical .planning/ artifact CANONICAL_EXACT in src/artifacts.cts was never updated when workflows/graduation.md started writing .planning/PATTERNS.md for the `patterns` graduation-target category -- same omission class as the already-fixed #3224 (WINDOWS.md). validate.health's W019 falsely flagged it as unrecognized on every repo that has run the graduation scan. Also backfilled 5 other pre-existing stale rows in gsd-core/templates/README.md's artifact table (WINDOWS.md, STATE-ARCHIVE.md, milestone.lock, state.json, skill-manifest.json) that were already in the source registry but missing from the docs table -- found while fixing this exact drift class, cheap to close alongside it. RED proven on f3dd791fb8cda18196803e7144ce20e506d6490b (test-only commit, gsd-test outcome:failed, exactly the new PATTERNS.md test failing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4282): fix stale function name in skill-manifest.json comment Review finding: both the source comment and the new docs row said "routeSkillManifest" -- no such symbol exists (verified via Memtrace); the actual function is cmdSkillManifest (src/init.cts). Copied verbatim from a pre-existing comment, not introduced by this PR, but cheap to fix alongside. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4282): add changeset fragment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#4282): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: isolate lint-vendored-deps-manifest.test.cjs's fixRow tests from the real vendor file Genuine, pre-existing defect found and fixed per this repo's no-defer policy (discovered while investigating a real CI failure during this PR's own merge attempt, user-directed investigation -- not deferred to a separate issue since it was actively blocking work and root-caused with concrete evidence, not speculation). Root cause: fixRow(row) (scripts/lint-vendored-deps.cjs) unconditionally does fs.copyFileSync(upstreamCjs, vendoredCjs) as its first line. All three tests in the #4573 describe block called fixRow(row) with the REAL js-yaml row, so all three wrote to the real, shared gsd-core/bin/lib/vendor/ js-yaml.cjs -- a file other test files' require() calls can read at any moment, since node --test runs files concurrently in this repo. fs.copyFileSync's write is not atomic against a concurrent reader on every filesystem; a concurrent require() elsewhere caught the file mid-overwrite and read a truncated file, crashing an entirely unrelated test (m9-statelock-write-error-orphan.test.cjs) with a SyntaxError. Confirmed via two real CI log fetches, not assumed: the exact same shard grouping (same 308 files) ran clean ~90 minutes earlier during PR #4615's own final merge CI, with the identical #3660 reap-fix code already present -- ruling out a deterministic connection to that change and confirming a genuine, non-deterministic timing race in this pre-existing test design. Fix: all three tests now redirect row.vendoredCjs to a private os.tmpdir() path via a cloned row object before calling fixRow, so the real vendored file is never touched. upstreamCjs stays pointed at the real node_modules copy (read-only, safe to share). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: also isolate fixRow's package.json pin-rewrite from the real file Review finding (major) on the previous race-condition fix: fixRow's pin -rewrite path still hardcoded path.join(ROOT, 'package.json'), so the third #4573 test still wrote the real, shared package.json -- read at module top-level by dozens of other test files, the same concurrent-file race class already fixed for the vendored .cjs copy. Adds an optional pkgRoot parameter (defaults to the real ROOT) threaded through readPinState/checkRow/fixRow -- fully backward-compatible, every existing call site (the CLI --fix path, any other caller) is unaffected since the default is unchanged. The pin-rewrite test now builds an isolated temp root (its own package.json + node_modules/js-yaml/package.json) and passes it explicitly, so the real package.json is never touched either. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix: use helpers.cleanup instead of raw fs.rmSync in test cleanup CI caught it: local/no-raw-rmsync-in-tests flagged the three t.after temp-dir cleanup calls added for the fixRow isolation fix. helpers.cleanup() carries the Windows-EBUSY retry budget (maxRetries/retryDelay) that raw fs.rmSync lacks. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Dennis Alexis Valin Dittrich
|
1316e03b84 |
fix(#4424): assert the launcher snippet's env surface is covered by the scrub lists (#4503)
* chore(#4424): assert launcher snippet env surface is covered by scrub lists SNIPPET_SCRUB is hand-maintained for vars TEST_ENV_BASE's registry-derived list can't carry. Nothing asserted the union actually covers every ${VAR:-default} arm in _runtime-launcher.snippet.sh, so a new runtime-home arm with no scrub entry could drift silently — the #4205 shape, one door over. Adds (A2): extracts every ${[A-Z_]+:-} capture from the snippet and checks membership in TEST_ENV_BASE, SNIPPET_SCRUB, or the two vars the snippet/fixtures set themselves (RUNTIME_DIR, GSD_TOOLS). * fix(#4424): allow digits in the (A2) fallback-var regex CodeRabbit review on fork PR #37: [A-Z_]+ silently drops any \${VAR:-...} capture whose name contains a digit (e.g. CLAUDE2_CONFIG_DIR) instead of flagging it uncovered, defeating the guard's own purpose. Matches bash identifier syntax instead: leading letter/underscore, then alnum/underscore. * fix(#4424): rename SELF_ASSIGNED to reflect RUNTIME_DIR's real provenance Gemini adversarial review (agy) on fork PR #37: RUNTIME_DIR is an external input the snippet reads via \${RUNTIME_DIR:-...}, never assigns — every fixture sets it in-script before sourcing the snippet. Only GSD_TOOLS is truly snippet-self-assigned. SELF_ASSIGNED conflated the two; renamed to CALLER_OR_SELF_ASSIGNED. No behavior change. Reviewed and rejected: moving RUNTIME_DIR into SNIPPET_SCRUB (blanking it is indistinguishable from unset to the resolver's own \${RUNTIME_DIR:-...} fallback, re-opening the #4205 ambient-leak this suite guards against — see the existing comment at line ~1801); widening the regex to mixed-case, colon-less \${VAR-default}, or \${VAR:=default} forms (none exist in the snippet, and the issue's own spec scopes this to \${[A-Z_]+:- captures); stripping bash comments before matching (the snippet is one physical line with zero '#' characters, so no comment can exist in it). * fix(#4424): guard CALLER_OR_SELF_ASSIGNED against silent future additions trek-e review on PR #4503: a future ${VAR:-default} arm could be dropped into this set without confirming it is genuinely caller-supplied/ self-assigned rather than a real coverage gap. Adds a comment requiring justification for any addition, pointing to SNIPPET_SCRUB as the default when in doubt. No behavior change. * fix(#4424): guard (A2) against a vacuous pass on empty extraction agy adversarial review (gemini-3.8-flash-high, /gsd-review lane) on PR #4503: if the snippet becomes unreadable/truncated/renamed, matchAll yields zero matches, uncovered stays [], and assert.deepStrictEqual passes vacuously — same "guards the guard" gap the sibling (E)-adjacent tests already close with assert.ok(files.length > 0, ...). Asserts extracted.length >= 15 before filtering. Reviewer's second finding (regex misses colon-less ${VAR-default}) is not applied: no such form exists in the snippet today, and 688cc1c already recorded this exact widening as scope creep the issue's own spec (${[A-Z_]+:-) does not ask for. --------- Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Michel Moreira
|
eadcba5f53 |
fix(#4481): anchor bold STATE field reads to line start (#4510)
* test(#4481): reproduce mid-sentence state field reads * fix(#4481): anchor bold STATE field reads to line start * docs(#4481): add changeset for #4510 * fix(#4481): align bold field readers with anchored writers |
||
|
Tom Boucher
|
43c48ce92e |
Merge pull request #4621 from open-gsd/test/4520-batch9-generators-doc-gates
test(#4520): migrate generators/doc-gates/attribution batch to named timeout constants |
||
|
sim
|
2eef8ada4f |
test(#4520): migrate generators/doc-gates/attribution batch to named timeout constants
Batch 9 of 17 in the ad hoc timeout literal migration (epic #4445). Replaces every bare numeric timeout/timeoutMs object-literal property in 14 files with a named constant, per eslint-rules/no-adhoc-timeout-literal.cjs. Removes the 14 files from the rule's allowlist. The issue's own guess ("all run a scripts/*.cjs generator or lint script once, BUILD_TIMEOUT_MS class") needed two corrections found by reading every site directly. First, BUILD_TIMEOUT_MS's own doc comment scopes it specifically to scripts/build-hooks.js, which none of this batch's generator/lint-script sites run — a new shared constant, GENERATOR_SCRIPT_TIMEOUT_MS, covers the class instead. Second, no-pending-3212-markers.test.cjs's single site spawns `git ls-files` directly, not a scripts/*.cjs script at all — routed to a second new shared constant, REAL_REPO_GIT_TIMEOUT_MS, promoted once emitted-attribution.test.cjs's own git-plumbing sites were found sharing the same class and value. Isolated Standards-axis review caught a further misclassification: one of REAL_REPO_GIT_TIMEOUT_MS's three emitted-attribution.test.cjs sites actually builds a fresh throwaway temp repo (createTempDir + git init), contradicting that constant's own real-repo-tree-only scope. Fixed with a new file-local FRESH_FIXTURE_GIT_TIMEOUT_MS holding the exact pre-existing value under an honest name, rather than reusing the shared GIT_FIXTURE_TIMEOUT_MS (which would have doubled the bound). emitted-attribution.test.cjs also gets two more file-local constants: HEAVY_REAL_TREE_TEST_TIMEOUT_MS (node:test's own per-test timeout option, not a spawn bound) and BUILD_HOOKS_UNDER_LOAD_TIMEOUT_MS (the same build-hooks.js script as the shared norm, at 4x its bound inside the suite's heaviest test). emitted-ack-trailer.test.cjs gets IMPOSSIBLY_SHORT_GIT_TIMEOUT_MS — the one value in this migration that is deliberately tiny (20ms), used to force a timeout in a negative test, not generous headroom. No src/bin file touched, no numeric value changed anywhere. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
a1a4182bda | Merge pull request #4616 from open-gsd/test/4519-batch8-security-scanners | ||
|
Michel Moreira
|
411199d08b |
fix(#4480): require a name column in roadmap phase tables (#4511)
* test(#4480): reproduce unnamed roadmap table phases * fix(#4480): require named roadmap phase tables * docs(#4480): add changeset for #4511 * test(#4480): cover phase name columns generatively --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
5e2055ab90 |
fix(#3660): reap a bounded check's orphaned worker after its own timeout kill (#4615)
* test(#3660): prove a bounded node-test check orphans its worker on timeout Regression test only, no fix yet: `execFileSync`'s timeout kills the direct `node --test` runner but never the per-file worker it forks by default since Node 22 (`--test-isolation=process`). The worker is reparented to PID 1 and can busy-loop forever while the bounded-check verdict still reports a clean fail-closed timeout. Adds three tests driven through the real, uninjected defaultRunCheck path: a hanging subject's worker must not survive the call, a control proving the liveness probe can actually distinguish alive-vs-dead, and a non-hanging failure proving the reap-gating logic added by the next commit doesn't change the ordinary-failure return shape. Expected RED on this commit (src/prohibition-enforcement.cts is unchanged). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): reap a bounded check's descendant worker after its own timeout kill execFileSync's timeout only signals the direct child (the node --test runner); since Node 22, node --test forks a per-file WORKER by default (--test-isolation=process), so a hung subject's worker survives the bound, gets reparented to PID 1, and busy-loops forever while the verdict still reports a clean fail-closed timeout. Adds execFileSyncReaping (wraps execFileSync, detached:true on POSIX) and reapDescendants(pid): POSIX process.kill(-pid, 'SIGKILL') against the process group, Windows an absolute-path taskkill /PID <pid> /T /F (never a bare PATH-resolved name -- PR #3681 review minor-9). The reap fires ONLY when this call's own timeout killed the child (the thrown error carries a signal) -- an ordinary non-zero-exit failure has signal:null and is left alone, which is the fix for PR #3681's Blocker-3 (that attempt reaped on every throw, risking a PGID-reuse collateral kill on a ordinary red run). All four execFileSync(process.execPath, ...) call sites now route through execFileSyncReaping: runNodeTestWithSubject, defaultRunCheck's node-test and lint-rule arms, defaultProveFailFirst's lint-rule arm (its node-test arm reuses runNodeTestWithSubject). RED proven on 0bd741fbc2b2ad0792fdf2361de14b68b2e3aea3 (test-only commit, gsd-test outcome:failed, exactly the new orphan-detection test failing). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): address code-review nits on the reap doc comments - Clarify execFileSyncReaping's gate covers a maxBuffer-triggered kill too, not just a timeout -- both set .signal, both are "this call's own bound". - Note reapDescendants' POSIX catch swallows any errno, not only ESRCH. No behavior change (tsc --noEmit clean, no-op for the compiler). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * debug(#3660): fix hardcoded Windows path + add temp diagnostics for CI reap failure Real defect #1 (fixed for good): taskkillPath() had a 'C:\Windows' literal fallback, tripping tests/hardcoded-paths.test.cjs's repo-wide scanner. Now returns null when neither SystemRoot nor windir is set, and the caller skips the Windows reap rather than guessing a path. Real defect #2 (under investigation): the prior GREEN gsd-test run showed the #3660 orphan-detection test STILL failing on linux-node24 even with the fix applied -- the worker survived. Isolated diagnostic scripts against the exact same execFileSync({detached:true})+process.kill(-pid) mechanism, including one using a REAL node --test worker, both confirm the mechanism works correctly on macOS (group-kill reaches the worker). This commit adds TEMPORARY stderr instrumentation (GSD-DEBUG-3660 tags) around the reap attempt to get direct evidence from the actual Linux CI environment before guessing further. Will be removed once the root cause is confirmed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): gate the reap on err.code === 'ETIMEDOUT', not err.signal Root cause of the prior GREEN run's failure, confirmed with real evidence from linux-node24 CI: execFileSync's thrown error on a genuine timeout-kill does NOT reliably set `.signal` -- on that environment it came back `signal: null, code: 'ETIMEDOUT', status: 7`, so the reap gate never fired. A separate macOS/Node run of the identical scenario showed `signal: 'SIGTERM'` for the same case -- neither field alone is safe across platforms/versions, but `code === 'ETIMEDOUT'` was present and correct in both. Verified via temporary stderr instrumentation on a real gsd-test run (now removed) before landing this, rather than guessing from the macOS-only result. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * debug(#3660): round-2 instrumentation -- ETIMEDOUT gate fix alone didn't work The err.code === 'ETIMEDOUT' gate fix (previous commit) did not resolve the failure -- same test still red on real Linux CI. Adding probes around the actual process.kill(-pid, 'SIGKILL') call itself to see whether it throws, and whether the group is observably alive/dead before and after, since the gate may now be firing correctly but the kill may not be reaching the worker's process group on this environment. Temporary, will be removed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): the fix was already correct -- the TEST's liveness probe was not Root cause of the two prior red rounds, confirmed via process-group probes on real Linux CI: process.kill(-pid, 'SIGKILL') succeeds (no throw) every time the ETIMEDOUT gate fires -- the worker genuinely IS killed. But process.kill(pid, 0) cannot tell a truly-running process from an already-killed ZOMBIE stuck unreaped: this bench's container has no init process collecting arbitrary orphans, so a killed worker (reparented to PID 1 on death) sits as a zombie forever, still answering kill(pid,0) with "exists" even though it is fully dead and burning zero CPU -- which is the actual harm #3660 is about. Test now reads /proc/<pid>/stat's process-state field on Linux and treats 'Z' (zombie) as dead, falling back to the plain kill(pid,0) probe elsewhere (no /proc on macOS/Windows). Also strips the round-2 GSD-DEBUG-3660b instrumentation now that its evidence has been used and the real root cause is fixed. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): merge duplicate doc comment, fix stale err.signal reference Leftover artifacts from the multi-round debugging: taskkillPath had two stacked doc comments (an edit only replaced the function body, not the original comment above it); a test comment still said "err.signal" after the gate was changed to err.code === 'ETIMEDOUT'. Comment-only, no behavior change (tsc --noEmit no-op). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): close a vacuous-test gap and harden isAlive's error handling Code review finding (major): none of the three #3660 regression tests ever asserted isAlive(pid) === true for a genuinely running process -- the real code path is fully synchronous, so there's no natural window to observe "alive" before "dead" inside those tests. A probe that always returned false would have passed all three vacuously. Added a standalone test proving isAlive(process.pid) reports true, using this test's own unambiguously-alive process, running before the three existing tests. Also hardened isAlive's /proc read-failure handling (minor finding): only ENOENT (process genuinely gone) now means "dead"; any other read error (EACCES, EIO, ...) reports "alive" (inconclusive) rather than risking a false "dead" that would silently mask a real regression. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): add changeset fragment Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(#3660): backfill changeset PR number Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(#3660): bound the taskkill spawnSync with a timeout CI caught it: local/require-subprocess-timeout (DEFECT.UNBOUNDED-SUBPROCESS) flagged the new spawnSync(taskkill, ...) call in reapDescendants' Windows branch for having no timeout. 5s bound -- a local OS command, not a network call; reapDescendants already treats any failure (including a hypothetical hang) identically via its existing try/catch, so the bound costs nothing and just prevents a stuck taskkill from blocking the caller forever. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> |