stable
11 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Jakub Zych
|
a9a7a328e6 |
refactor: hard-fork GSD -> MSD (Make Software Done)
Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD across contents and paths, upstream package/repo coordinates -> @golem15/msd-core and golem15com/msd-core. Deep links into upstream history, sibling upstream packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is. Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line, package/plugin identity, regenerated lockfile, install-tree fixtures, derived registries and benchmark baseline; migration checksum baseline re-locked (MSD keeps its own install state, so no install had applied the old sums); sort-order and regex-escaped expectations in tests adjusted. |
||
|
0xdhx
|
472f585f7c |
fix(#3726)!: require --confirm before milestone complete mutates (#3774)
* fix(#3726): require --confirm before milestone complete mutates `milestone complete <version>` is a one-way door — ROADMAP.md and REQUIREMENTS.md archived, every phase directory in the milestone MOVED, STATE.md rewritten — and ran unconditionally on first invocation through every invocation path, including `query milestone.complete <version>`, whose `query` meta-prefix reads as a read-only namespace but performs no filtering (#167's invocation-compatibility shim + #3243's dotted-form normalization). The gate lives on the destructive command itself, not on the `query` prefix (the prefix is an intentional invocation mechanism, not a permission boundary — restricting it would break dozens of shipped workflow callers). Without --confirm and without --dry-run the command now refuses via error() before reading anything beyond its arg checks, so an unconfirmed invocation is a guaranteed no-op on disk. --dry-run still previews with no confirmation needed and is now documented in the usage block (it was only documented for the sibling archive-quick). --force keeps its narrow meaning — bypassing the TRUNCATED-scope and unstarted-phase guards — and does not double as the mutation opt-in. --confirm follows the existing `phases clear --confirm` idiom in the same module. complete-milestone.md's two invocations pass --confirm (the workflow has gathered explicit user intent by that step). Existing tests get --confirm appended — pre-change behavior is exactly confirmed behavior — and a #3726 regression block covers: refusal + full-tree byte-identity on both invocation forms, --force not satisfying the gate, --dry-run still passing without confirmation, and --confirm proceeding. The refusal tests fail against pre-fix code (negative control run). Fixes #3726 * docs(#3726): document the --confirm requirement in CLI-TOOLS and COMMANDS Cross-AI review of the fix diff (codex, pre-create) caught three shipped doc sites still instructing the now-refused bare invocation: the CLI-TOOLS.md milestone-complete synopsis + flag table, and COMMANDS.md's two guard-override instructions (`--force` alone now refuses without --confirm). Localized CLI-TOOLS copies already lag the English synopsis (no --force/--dry-run either) and follow the translation pipeline, not this fix. * chore(#3726): set changeset fragment pr to 3774 * test(#3726): confirm-gate CI repairs — QA scenario caller + growth ack Two CI reds from the --confirm gate, both this branch's own misses: - tests/qa/scenarios/milestone-rollover.json invoked `milestone complete 1.0 --force` as a JSON arg-array fixture — a caller shape the test sweep (which grepped runGsdTools/runSdkQuery in tests/*.cjs) never enumerated. Adds --confirm; the scenario's boundary-crossing contract is otherwise untouched. - complete-milestone.md's +420-byte --confirm note trips the emitted-attribution growth ratchet. Acknowledged as a #3726 append to the existing complete-milestone.md entry in 3409-unreachable-guard-arms.json (two ack sources may never name the same path, per that fragment's own precedent). Local: lint-emitted-drift-ack ok; loop-walk.qa 115/115 green sandboxed. * docs(#3726): CLI-TOOLS.md guard-override sentences say --force --confirm Review Major 1: the truncated-window and unstarted-phase guard paragraphs still told the reader to "Pass `--force` to override", which now refuses (--force alone does not satisfy the confirmation gate), while the flag table 470 lines later said the opposite. Mirror the docs/COMMANDS.md pair so the file no longer contradicts itself. * docs(#3726): synopsis renders --confirm and --dry-run as alternatives Review Nit 1: `milestone complete <version> --confirm [--dry-run]` read as "a dry run still needs --confirm", the opposite of AC 3. Render the pair as `(--confirm | --dry-run)` in the CLI-TOOLS.md synopsis and the usage docblock, and let the flag rows carry the rule. * test(#3726): pass --confirm in base-added milestone fixtures; re-file the growth ack Rebase onto next (26 commits) surfaced three tests the gate now refuses: the #3685 write-flag contract pair in tests/milestone.test.cjs and the `milestone complete` boundary fixture in tests/state-contract.test.cjs all invoke the command bare. Each now passes --confirm (a mutating run is exactly what they assert on). The +420 byte complete-milestone.md growth ack rode on 3409-unreachable-guard-arms.json, which #3078 swept from next as fully spent — hence the modify/delete conflict. Re-filed under a fresh fragment named for this issue, never resurrecting the swept one. * test(#3726): pin the present-but-falsy arm of the confirmation gate Review Minor 1: the boundary triple covered absent and present but not present-but-falsy. The gate is an exact-token match, so --confirm=false and --confirm=0 refuse today — pinned (canonical + query forms, whole .planning/ tree byte-identical) so a future `=`-aware or prefix-matching parser cannot silently turn --confirm=false into a confirmed run of an irreversible command. * test(#3726): drop --confirm from dry-run-only invocations Review Nit 2: --confirm was mass-appended to 14 pre-existing --dry-run invocations that never needed it, so each stopped standing as incidental proof that a preview needs no confirmation. Reverted to the pre-PR form; the dedicated AC-3 test carries the explicit assertion. * docs(#3726): sync the localized CLI-TOOLS synopsis with the confirm gate REQ-I18N-02 (docs/features/internationalized-documentation.md) requires translations to stay synchronized with the English source. The four localized CLI-TOOLS.md guides still advertised a bare `milestone complete <version>`, which now exits 1. Render the English synopsis verbatim — `(--confirm | --dry-run)` plus the `[--force]` and `[--archive-quick]` flags the translations had also fallen behind on. * test(#3726): drop --confirm from the remaining preview-only invocations Round 2 reverted the --confirm appends on --dry-run-only invocations in tests/milestone.test.cjs, but four more sat in two files the sweep missed: tests/milestone-archive.test.cjs (three) and tests/milestone-window-single-owner.test.cjs (one). Each is a preview run whose whole purpose is to document that a preview mutates nothing, so `--dry-run ... --confirm` contradicted the semantics the test exists to pin. Dropping the token restores each as incidental proof that a preview needs no confirmation; the dedicated AC-3 test keeps the explicit assertion. No assertion added, relaxed, or removed — the change is four tokens. * chore(#3726): migrate the emitted-drift ack from a fragment to a commit trailer #3954 (ADR-3942) moved emitted-drift acknowledgments out of tests/emitted-drift-acks/ and into git commit trailers, and the fragment directory no longer exists on next. The reason this PR's fragment carried moves verbatim into the Emitted-Drift-Ack-Growth trailer on this commit; the fragment file is removed rather than resurrected. Emitted-Drift-Ack-Growth: complete-milestone.md — #3726: +420 bytes (40186 -> 40606). The archive_milestone step's two `milestone complete` invocations now pass the required --confirm flag (the command refuses to mutate without it — the archive is irreversible), with a note explaining the flag and pointing at --dry-run for previews. Deliberate runtime-loaded workflow text for the new gate, not converter drift. * fix(#3726): name --confirm in the version-required refusal The documented arg-discovery path (gsd-tools.cjs top-level usage: invoke the command without args and the error lists what is required) stopped at `version required for milestone complete (e.g., v1.0)` — one required argument short. Discovering --confirm took a second round trip through the gate. The refusal now reads `… — and --confirm to mutate`, pinned by a test that also asserts the version-less invocation leaves .planning/ untouched. * test(#3726): pin the milestone complete docs against a silent regression The changeset is `type: Fixed`, which the docs-required lint exempts, so nothing in CI would notice a later edit that reinstated the bare-`--force` override prose or dropped `--confirm` from the synopsis. Four tests in tests/milestone.test.cjs now pin: the synopsis line in docs/CLI-TOOLS.md and its four localized mirrors; the `--confirm` flag row; both guard-override instructions in docs/CLI-TOOLS.md and docs/COMMANDS.md, by guard name (a substring match on each instruction's `--force --confirm` text); and — as an identity ratchet over the milestone-complete sections — every `--force` sentence or clause that lacks `--confirm`, so a new bare instruction in its own sentence or clause fails whatever its wording. Named residual: a bare instruction spliced into the same clause as a compliant one coalesces with it and passes the ratchet; the by-name pins are what keep the four known instructions from losing the pairing that way. The file is registered in scripts/docs-guard-registry.cjs so the pin runs on the PR that changes those docs, not only after merge. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
e20744eacb |
enhance(#3884): failure is a value — strict argv, and --pick that signals absence (#3922)
* test(#3884): failing-first coverage for strict argv and absence-signalling --pick ADR-3473 §8.4 says failure is a value. Three families currently encode failure as success, and this commit pins each one RED before the fix lands. Measured on this tree, 2026-08-26: gsd-tools generate-slug "test" --pick nonexistent -> empty stdout, exit 0 (#3365) gsd-tools audit-open --pick nonexistent_field -> dumps the entire human-readable audit report, exit 0 gsd-tools generate-slug "Hello World" --raw --pick bogus -> prints "hello-world", another field's value, exit 0 gsd-tools query state.planned-phase 3 (positional, no --phase) -> exit 0; STATE.md's "Phase: 2 of 5 (Widget Support)" is overwritten to "Phase: null - READY TO EXECUTE" and the frontmatter gains a corrupted current_phase_name (#3358) tests/pick-flag.test.cjs:27 previously asserted the #3365 defect as the contract ("returns empty string for missing field", success === true). That assertion is replaced by the required behavior rather than deleted. The new parseNamedArgs block calls the spec-object signature that does not exist yet, so it fails today by construction. The 11 existing behavior-lock tests are left untouched here; they are corrected in the implementation commit. C1/C4 assert at the consumer's output - STATE.md's bytes - per ADR-3180 Decision 4(b). A unit assertion on the parser would have passed throughout this defect's life. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3884): failure is a value — strict argv, and --pick that signals absence Implements ADR-3473 §8.4. Absence, emptiness and failure stop being interchangeable ways to say "I could not answer". parseNamedArgs (src/command-arg-projection.cts) Takes a spec object with a REQUIRED `positionals: number | 'rest'` and returns the hub's Result shape instead of a bare Record. Declaring the positional arity is what makes #3358's call site unrepresentable rather than merely detectable: an unrecognized flag or a token past the declared boundary is now InvalidArgs, naming the offending token and listing the accepted flags. The legacy positional-array call shape throws a TypeError — an internal invariant violation per ADR-3473 Decision 2, so a stale hand-written .cjs call site fails loudly instead of destructuring undefined off a Result. parseNamedArgsOrExit projects a failure onto the caller's error(); it is a projection over the one parser, not a second parser. Measured before, against a STATE.md with a populated phase-2 block: query state.planned-phase 3 (positional, no --phase) -> exit 0; "Phase: 2 of 5 (Widget Support)" overwritten to "Phase: null - READY TO EXECUTE", frontmatter gains a corrupted current_phase_name After: exit 1, `unexpected positional argument "3"`, STATE.md byte-identical. The flag form is unchanged and still updates STATE.md. --pick <field> (gsd-core/bin/gsd-tools.cjs) extractField returns {found,value}, and the pick block no longer shares one catch between "output was not JSON" and "field was absent". An absent field exits 1 with pick_field_absent, naming the field and the keys that do exist; non-JSON output exits 1 with pick_output_not_json instead of dumping the command's entire output. A field that is PRESENT with value null, '', 0 or false still prints at exit 0 — that is an answer, not a failure, and it is what keeps `--pick count` printing 0 on a fresh project. Measured before: `audit-open --pick nonexistent_field` printed the whole human-readable audit report at exit 0, and `generate-slug X --raw --pick bogus` printed "hello-world" — a different field's value, confidently, at exit 0. ADR-3409 Decision 7 explicitly deferred this contract fix to #3473; this is it. The sub-issue's "returns 0 when the count is zero OR absent" wording is superseded by the ADR rule it implements: zero prints 0, absence exits non-zero. Defaulting absence to 0 would demote "could not answer" to "the answer is zero" — the hazard docs/how-to/resolve-unreachable-guard-findings.md already warns against. Guard ledger (ADR-3473 Decision 6) scripts/lint-unreachable-guard-drift.cjs Detector A is RETIRED. Its premise — that a `--pick ... || echo` arm can never fire — is now false, so the shape it forbade is the correct idiom and keeping it would forbid the fix. Detector B (glob-consuming cat/ls, a nullglob mechanism this change does not touch) is retained in full, as are the shared scanner, the escape-marker parser and the baseline. Net: -1 detector, 0 added. The file is not deleted. Call-site audit 45 prompt-layer --pick invocations, every one a plain X=$(...) assignment — none in an if test, && chain, or a pipeline whose status is consumed, and no shell block in workflows/commands/agents/references sets -e. Of the 13 (command, field) pairs the prompt layer reads, 10 are always present; the 3 sometimes-absent ones each sit behind a prior found/existence check. No ADR-3409-class "field the command never produces" remains. Design: .gsd/phase/feat-3884-failure-is-a-value/40-design.md Test matrix: .gsd/phase/feat-3884-failure-is-a-value/50-test-matrix.md Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): escape untrusted tokens in diagnostics, and cover five unpinned rows Two review findings, both fixed here rather than recorded as limits. 1. A newline in an untrusted token forged a second stderr line. Before, plain-text mode: $ gsd-tools query state.planned-phase $'foo\nError: forged second line' Error: unexpected positional argument "foo Error: forged second line" After: Error: unexpected positional argument "foo\nError: forged second line" --json-errors mode was never affected — io.error runs that payload through JSON.stringify. Plain-text mode writes 'Error: ' + message verbatim, and the three new InvalidArgs reasons plus the two new --pick diagnostics all interpolate a token that comes straight from argv. Fixed with ONE shared helper, formatDiagnosticToken (src/io.cts), applied at every interpolation site — not a copy per site. It is deliberately NOT applied inside error() itself: several callers in this tree emit intentional multi-line diagnostics, and escaping newlines there would mangle them. The available-top-level-keys list needed the same treatment for a reason the review did not anticipate: `frontmatter get <file>` reads an ARBITRARY user document and echoes that document's own keys into the diagnostic. Verified reachable — a frontmatter key containing a newline reaches the key list — so formatKeyForDiagnosticList is guarding a live path, not a hypothetical one. Ordinary keys still render plain and unquoted; a fix that merely dropped the key would also have passed a "one line" assertion, so the test pins the escaped key's presence too. 2. Five behavior-table rows were implemented but nothing pinned them: B7 a dotted path that dies partway B9 bracket syntax on a non-array B10 a negative array index, in and out of range B14 a JSON root that is not an object B17 an @file: payload over 50KB B17 is the load-bearing one. output() writes @file:<path> instead of inline JSON past 50000 characters, and --pick resolves that BEFORE parsing; with no test, a future reordering of those two steps turns every large result into a false pick_output_not_json. The fixture seeds 1200 phase directories and measures the payload at 62474 characters, asserting the spill actually happened rather than assuming it. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): correct the strict-argv surface against a full verification run The first full run came back with 90 failures across 12 files, none in the new tests. They were the argv surface telling me what it actually is. Ten root causes; each classified before anything was changed. I over-implemented, and that is reverted. ADR-3473 §8.4 says parseNamedArgs rejects "unrecognized and positional tokens". It says nothing about a value flag whose value is missing. Making that an error was my design decision, not the rule, and it broke a deliberately recorded contract: `--prd` with no value resolving to null (tests/init.test.cjs emptyPrdValueIsFalsyAndTreatedAsAbsent, row B5; tests/section-manifest-init-facts.test.cjs "flag-shaped value"). The "requires a value" branch is deleted outright rather than kept behind an option — an unused strictness mode is speculative generality. Unknown-flag and unexpected-positional rejection, which is what §8.4 actually mandates, is unchanged. --wave needed a third flag kind the original design did not anticipate. `--wave N` is documented (commands/gsd/execute-phase.md:4,48) and the shipped workflow reconstructs and passes it (execute-phase.md:84), while #2932 records token-PRESENCE semantics: the CLI cares only that the flag appeared, and the value belongs to the workflow layer. That is neither a boolean flag nor a value flag, so `optionalValueFlags` now exists — presence-only in `data`, and the validation cursor consumes a following non-flag token so it is not reported as a stray positional. Every other declared boolean flag was checked against every argument-hint and prose usage in commands/, workflows/, agents/ and docs/; `--wave` is the only one of this shape. Five tests were pinning forms that never worked. tests/adr857-core-without-capabilities.test.cjs passed `init plan-phase --phase 01-stub`, but the documented form is positional (docs/CLI-TOOLS.md:776) and the handler reads args[2] — which for that form is the literal string "--phase". Measured on the pre-fix build against a real .planning/phases/01-stub/ directory: init plan-phase 01-stub -> phase_found=true init plan-phase --phase 01-stub -> phase_found=false The test asserted only exit 0 and key presence, so it had been green while proving nothing about phase resolution. Corrected to the documented form and strengthened to assert phase_found === true. Same class in state.test.cjs (`--plan-count`, a flag that does not exist; the real one is `--plans`), milestone-archive.test.cjs (`init new-milestone --json`, silently ignored), and concurrency-safety.test.cjs (a bare positional field name whose OR-assertion passed because a whole-document dump happens to contain the substring it looked for). Six handlers had no argv validation at all — the same #3358 shape this phase exists to close, found while fixing the rest: init verify-work / phase-op / review / todos / remove-workspace read args[2] with nothing checking the rest, and validate health read --repair/--backfill through a bare args.includes() scan that bypassed the parser entirely. All now go through the seam, so the flag has one owner. tests/init-debug.test.cjs rows C4/C5 asserted that an unrecognized flag must NOT fail. That is the behavior §8.4 removes, and Decision 8 says a caller's local expectation does not override §8, so they are inverted and renamed — a test still called "ignores an unrecognized flag" while asserting rejection would be its own defect. Row C6's point is its PWNED canary; that assertion is kept verbatim and only its exit-status expectation changed, because the hostile token is now rejected rather than absorbed. The blast-radius estimate in 40-design.md is corrected rather than quietly left wrong. get_impact reported MEDIUM / 8 symbols upstream, and that was accurate for what the graph can see — parseNamedArgs's callers. It cannot see that those callers' handlers accept argv shapes wider than the code reading args[2] suggests, which is where the real surface was. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3884): withdraw the validate-health tightening, finish the A2/A3 revert Second full run: 46 failures, down from 90. Four causes, two of them mine. Reverted `validate health` entirely — it was scope creep, and it broke a real flag. ~30 of the 46 read `unknown flag "--json"; accepted: --repair, --backfill`. The previous commit routed `validate health` through the parser on the reasoning that a flag should have one owner. That was wrong twice over: §8.4 names parseNamedArgs and count queries, and `validate health` was never a parseNamedArgs call site — it read its flags, just not through the parser, so it had no silent-drop defect to fix. Tightening it omitted `--json`, which the health-diagnostic suites use heavily. The handler is now byte-for-behaviour back to its pre-branch form. `validate context` stays converted: it genuinely was a call site, and its `--json` is now declared rather than read by a second `args.includes` scan. The five handlers that had NO validation at all — init verify-work / phase-op / review / todos / remove-workspace — stay fixed. Those read args[2] with nothing checking the rest, which is the #3358 shape this phase owns. Finished the A2/A3 revert. Three tests still encoded the deleted "a value flag with a missing value is an error" rule, including one added by the previous commit for that rule. All three now assert the reverted null contract, and the ones whose titles said "rejected" are renamed — a test named for a contract it no longer asserts is its own defect. `--wave=` and `--wave --weird` are correctly rejected. Neither is documented in commands/gsd/execute-phase.md, gsd-core/workflows/execute-phase.md or docs/, and neither is emitted by the shipped prompt layer, so both are unrecognized tokens that §8.4 mandates rejecting. `doesNotConsumeFollowingFlagAsWaveValue` keeps the property it exists for — asserted directly now, at the parser, that `--wave` does not swallow a following flag as its value — and only its exit-status expectation changed. A contradiction inside this branch, surfaced by the audit and resolved the safe way. Two pre-existing #3573 tests call `state begin-phase '2'` and `state planned-phase '2'` with a bare positional, relying on the old permissive parser to ignore it. This branch's own #3358 regression test requires that exact argv to be REJECTED. The two are mutually exclusive. Widening the router to accept a bare positional — mirroring complete-phase — would have silently re-opened #3358, and was verified to do exactly that: with the widened router, `query state.planned-phase 3` returned exit 0 and wrote current_phase_name again. It is reverted. docs/CLI-TOOLS.md:116 and docs/COMMANDS.md:2192 document only the `--phase N` form for both verbs, so the two #3573 tests move to it. Their assertions were never about the call shape — only that total_phases survives the resync — and both still pass. complete-phase is untouched: its bare positional IS documented, and it keeps the dynamic boundary and the negative-space note that record why. The audit that produced this is in the PR body: for every handler whose declaration changed, the flags it reads anywhere in its body, the flags the shipped surface documents, and the shapes the suite passes, compared. The `--json` miss was a pattern, not an accident — declaring a handler's flags from its parseNamedArgs call alone misses whatever it reads elsewhere. Refs #3884 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3884): backfill the changeset PR number Refs #3884 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
|
107eb8c1d9 |
feat(#3753): run docs guards on the PR that changes the docs they read (#3787)
A PR whose diff is entirely under docs/ runs zero tests, so a guard whose INPUT
is shipped prose cannot protect the PR lane of the diffs it exists to check. Its
only firing opportunity is after merge, on the shared branch -- which is how next
went red on
|
||
|
Tom Boucher
|
b42cb4fb29 |
fix(#3597): count scenario expectation failures in the QA gate, and fix the workstream scope split it exposed (#3607)
* fix(#3597): count scenario expectation failures in the QA ratchet gate buildReport counts totals.violations as oracle violations PLUS scenario expectFailures, but collectFindings read only step.violations. A scenario whose declared expect failed therefore produced ok:false and violations:1 in the report while the ratchet printed "0 violations" and exited 0. multi-workstream has failed that way on every CI run since 2026-08-10, when #3217 (PR #3318) made computeProgressPercent withhold a percentage whose scope is not COMPLETE. The walk detected the change the day it landed; nothing was listening. - collectFindings returns a third bucket, expectationFailures, carrying no fingerprint so it can never be baselined or acked away - both modes of main() print and gate on it; the summary line reports it - guard runMain(main) behind require.main === module, so the QA suite can require the script to test collectFindings without running a real walk (that import side effect is why the gate logic had no test) - multi-workstream now asserts the true contract: phase_scope unreadable and percent null, per ADR-3180 7.6 rule 4 - the perturbation test asserts scenario ok, closing the test-side half Closes #3597 * fix(#3597): resolve the milestone window against the active workstream listMilestonePhaseDirs defaulted its ws option to null. planningDir treats undefined as "resolve the ambient workstream" and null as "force the project root", so that default suppressed the ambient resolution every other planning-path read uses. All 18 call sites derive phasesDir ambiently via planningPaths(cwd), so the counts came from the workstream while the milestone window came from the root .planning/ROADMAP.md — the exact numerator/denominator scope split ADR-3180 7.6 rule 3 forbids. workstream create migrates that root roadmap away, so the read threw and scope stayed UNREADABLE, and rule 4 then correctly withheld the percentage. Proof: with a workstream tree byte-unchanged, copying its own ROADMAP to the project root flipped --ws alpha progress from phase_scope:unreadable/percent:null to complete/100. This is the defect the loop QA walk was pointing at all along; the scenario expectation is restored to percent:100 rather than bent to match the bug. - pass ws through as undefined so ambient resolution applies - multi-workstream asserts phase_scope complete + percent 100 - regression test in completion-ratio-scope-withholding covers a workstream-only project with no root ROADMAP - replace the vacuous require.main test: runMain defers through a promise, so the in-process timing check passed against the unguarded file too; a child-process spawn now observes the guard for real - tie the oracle-violation test to expectationFailures, and cover the absent-key, multi-scenario and zero-step report shapes in parity - flatten scenario-authored strings before rendering them into the step summary and CI logs (forged markdown / ANSI injection) - widen the scenario contract assertions past perturbation-* so multi-workstream is actually covered test-side Closes #3597 * fix(#3597): flatten scenario-authored strings on the CI-log output path The step-summary path already routed findings through flattenUntrusted; the check-mode NEW-smell and STALE-entry console.error blocks, and the repro line in both printers, still interpolated raw. detail carries a scenario-authored expect[].path verbatim, and reason/scenario/id come from contributor-authored baseline and ack fragments validated only as non-empty strings. A crafted path could print a forged summary line into the CI log directly above the real one, plus ANSI repaint and unbounded length. Exit codes are unaffected — this is log spoofing, not gate bypass. * fix(#3597): refuse to archive on an unreadable milestone window; close review gaps Resolving the milestone window against the active workstream can leave the window UNREADABLE when that workstream has no ROADMAP of its own. getMilestonePhaseFilter throws, the window degrades to a pass-all fallback, and milestone complete would then move every phase dir -- breaking the guarantee stated at the archive site that no out-of-window directory is touched. milestone complete now refuses to archive when the window is UNREADABLE and reports the refusal; --dry-run previews the same refusal from the same shared derivation. The guard is scoped to UNREADABLE, not to every non-COMPLETE scope. A broader condition regressed ordinary root projects: the QA walk caught milestone-rollover leaving 01-parser on disk, which then tripped the #1447 abort in phases clear. UNSCOPED and TRUNCATED are pre-existing classifications and keep their existing behavior. Review fixes: - the workstream regression test asserted complete/100 but its fixture wrote no workstream STATE.md, so it resolved unscoped/null and the test failed; it now asserts a milestone and genuinely fails-first - the parity test hand-supplied totals.violations, hardcoding the very formula under test; at least one case now goes through the real buildReport - drop a vacuous qa-report.json assertion (jsonOut defaults to null, so no report is written by either shape) - buildRepro emitted a repo-relative binary path after cd-ing into a temp project, so every repro died with MODULE_NOT_FOUND; it now resolves an absolute path - flattenUntrusted truncated the repro to 300 chars, handing reviewers a command that looks complete and is not; length capping is now opt-out for repro while newline/control/backtick stripping still applies * chore(#3597): backfill changeset pr number (#3607) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
98ecb2ba8c |
enhance(#2142): archive quick tasks at milestone close-out (#3592)
* test(#2142): failing-first coverage for quick-task archival at milestone close-out * enhance(#2142): archive quick tasks at milestone close-out * fix(#2142): resolve review findings — readme injection, move/reset ordering, owned state write * fix(#2142): fold archival under milestone namespace, expose index IR, dedupe reset decision * test(#2142): assert archive-dir-relative summary path in index IR * docs(#2142): backfill changeset pr number to 3592 * test(#2142): skip newline-fixture injection test on windows (control chars illegal in path names) --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
6dbc124018 | enhance(#3180): the sibling validators share one envelope and one owner — Phase 12 (#3407) | ||
|
Tom Boucher
|
481ac7c71b |
fix(#2946): run milestone complete unstarted-phase guard independent of STATE (#3081)
* test(#2946): milestone complete unstarted-phase guard fails open on STATE desync Row 1 of the test matrix: the regression test that fails first. Adds seven cases to tests/milestone.test.cjs covering the desync, absent, no-file, --force-override, mismatch-WARNING, fresh-project-noop, and sentinel-skip behaviors. RED on next: the guard's entire scan is nested inside `if (stateVersion && stateVersion === version)`, so any STATE.md milestone: value that does not exactly equal the version argument skips the scan with no warning — functionally an implicit --force on a one-way-door operation. * fix(#2946): run milestone complete unstarted-phase guard independent of STATE The entire ROADMAP phase-directory scan was nested inside `if (stateVersion && stateVersion === version)`, so any STATE.md milestone: value that did not exactly string-equal the version argument — a desynced value, or no milestone: field at all — skipped the scan with no warning, functionally an implicit --force. The operation the guard fronts is a one-way door: ROADMAP.md and REQUIREMENTS.md are archived and phase directories are MOVED into .planning/milestones/<version>-phases/. The scan was already driven by the version argument through getMilestonePhaseFilter / extractCurrentMilestone; the STATE match was a redundant second gate that shadowed and broke it. Decouple: the scan now runs whenever --force is absent, and a present-but-mismatched STATE milestone: field emits a WARNING naming both values so the suspicious condition is visible rather than silent. A fresh project with no Phase headings in the scoped slice still yields an empty scan (no false positives) — the intent the STATE-match short-circuit was reaching for, now achieved by the scan itself. * docs(#2946): document milestone complete --force, --dry-run, and the unstarted-phase guard The CLI-TOOLS reference signature omitted --force and --dry-run entirely, and neither the unstarted-phase guard nor its override was documented anywhere user-facing. Add a flags table and a factual guard description to the Reference page (CLI-TOOLS.md), and a practical guard note to the /gsd-complete-milestone How-to (COMMANDS.md) covering what to do when the guard fires and the new STATE-mismatch WARNING (#2946). American English per CONTRIBUTING.md language policy. * fix(#2946): emit STATE-mismatch WARNING as JSON in --json-errors mode Follow-up to the guard decoupling: a structured caller using --json-errors parses stderr line-by-line as JSON, so the plain-text WARNING would break such a parser. Honor getJsonErrorMode() and emit a structured JSON object ({ ok, level, message }) in that mode, plain text otherwise — mirroring io.cts error()'s JSON shape. Addresses the isolated-review observation (~45% but credible, since --json-errors is a documented CLI flag). * fix(#2946): address review — drop JSON-mode WARNING scope creep, tighten test assertions Standards + spec review findings (code-review two-axis + isolated adversarial): 1. The --json-errors JSON WARNING branch (commit dee719404) was scope creep the issue never asked for, AND emitted ok:true for a suspicious-condition warning (a category error — a stderr JSON parser keying on ok would treat the suspicious state as success), AND its comment falsely claimed to mirror io.cts error()'s {ok:false,reason,message} shape. Dropped: the WARNING is now plain-text stderr only, matching the existing [gsd-tools] WARNING convention (state.cts). The issue asked for 'at minimum warn', not a structured JSON surface. 2. The WARNING test asserted on /WARNING/ regex (raw-text matching on stderr prose). Tightened to assert on the stable operator-facing tokens — the WARNING: marker and both version literals the operator must see — not the surrounding formatter prose. Added a paired negative test confirming no WARNING is emitted for an absent milestone: field (a missing declaration is a normal fresh-project state, not suspicious drift). * fix(#2946): sanitize STATE milestone value before stderr WARNING interpolation Security review (minor): stateVersion is read from a user-controlled file (STATE.md) and is not validated like the CLI version arg. Sanitize before interpolating into the WARNING — strip ANSI/control chars (/[\x00-\x1f\x7f]/g -> '?') and truncate to 80 chars — so a corrupted or hostile STATE.md cannot echo terminal escapes or secret-looking strings verbatim into a CI log or terminal aggregator (CONTRIBUTING.md security: secret-looking values in stderr). version is already constrained to [A-Za-z0-9._-] by ARCHIVE_VERSION_LABEL_RE upstream, so it needs no sanitization. * test(#2946): correct stale fixtures that relied on the guard being silently disabled Four pre-existing tests broke under the #2946 fix because their fixtures only passed thanks to the bug — the unstarted-phase guard was skipping on STATE mismatch, so fixtures with missing or non-matching phase directories slipped through. The tests exercise version-forwarding / version-scoping, not the guard, so give them legitimate directories: - milestone.test.cjs #3043: dirs were '103.old'/'104.old'/'108.new' (dot), which phaseTokenMatches rejects — renamed to hyphen form. The v3.6 stats scoping still yields 1 phase (getMilestonePhaseFilter scopes correctly); the guard now sees all three phases as having directories. - milestone-archive.test.cjs 'returns version in response data': ROADMAP listed Phase 1 but no directory was created. Added 01-foundation so the scan is satisfied. Per CONTRIBUTING.md, test-fixture corrections land as their own test: commit, not bundled into fix: (release hotfix cherry-pick routes by prefix). * chore(#2946): backfill changeset PR number 3081 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
a28dcec981 |
chore(#597): replace count-based ratchet guards with AST lint + named-set allowlists (#603)
The windows-test-parity ratchet greps test source for fs.rmSync-without-
maxRetries (and six other Windows-portability anti-patterns), failing when an
integer offender COUNT exceeds a frozen baseline (rmSync: 95). A count ratchet
is a Goodhart metric: fixing one offender and adding another keeps the count
constant, so a new defect slips through green. Replace it — and every other
count ratchet in the repo — with a layered, masking-proof design.
Behavioral seam test
- tests/helpers-cleanup.test.cjs proves helpers.cleanup() carries the Windows
EBUSY retry budget. cleanup() delegates retries to Node's fs.rmSync via
maxRetries (it owns no loop), so the test asserts the option contract
(recursive/force/maxRetries>0/retryDelay>0) + real-FS removal + the cwd-guard,
rather than a loop that does not exist. The EBUSY risk is now tested ONCE at
the helper, not approximated textually at every call site.
Write-time ESLint rule (AST-accurate, replaces the grep)
- eslint-rules/no-raw-rmsync-in-tests.cjs (error in tests/**/*.test.cjs) bans
raw fs.rmSync, steering to cleanup(). Catches member, computed (fs['rmSync']),
destructured and aliased forms; escape hatch is inline
`// eslint-disable-next-line local/no-raw-rmsync-in-tests -- <reason>` only.
- Migrated 336 raw fs.rmSync teardown calls across ~116 test files to cleanup().
~18 genuinely load-bearing sites (mid-test SUT/fault-injection removals,
error-swallowing or name-colliding local teardown helpers) keep the raw call
with an inline eslint-disable + reason.
Shared anti-ratchet primitive
- scripts/lib/allowlist-ratchet.cjs:
- assertWithinAllowlist: fails on NOVEL ids (new offender introduced) AND on
STALE ids (a known offender was fixed but not pruned) — identity, not count,
and a ratchet DOWN toward zero.
- assertTightCeiling: a size/length budget whose ceiling must stay within a
grace band of the high-water mark, so budgets may only tighten, never creep.
Ratchets converted onto the primitive
- windows-test-parity-guard.test.cjs: rmSync rule deleted (now ESLint-enforced);
the remaining six patterns moved from integer baselines to named-set
allowlists with ratchet-down.
- scripts/lint-test-file-count.{cjs,allowlist.json}: per-module integer counts →
named filename sets (closes the swap-a-file-keep-the-count blind spot); a
module dropping under cap now FAILS to force pruning its allowlist entry.
- enh-2790 skill-count `<= 63` → named skill allowlist (ratchets toward ~58).
Size budgets hardened (tighten-only)
- agent-size / workflow-size / feat-3039 help-tiered: ceilings lowered to the
current high-water mark and an assertTightCeiling anti-creep check added per
tier. Fixed external-contract limits (description ≤100 chars, agent ≤100 KB)
are intentionally left as-is — they are not grandfathered creeping budgets.
No user-facing behavior change (tests + tooling only); no USER_FACING_PREFIXES
touched, so no changeset fragment is required.
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|
||
|
Tom Boucher
|
a77883e131 | fix(#192): retire sdk assumptions in lint and regression tests | ||
|
Tom Boucher
|
9f57f7488e |
refactor(tests): consolidate Milestone Module — 10 files → 4 (#3754)
* refactor(tests): consolidate Milestone Module — 10 files → 4 Closes #3753 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(changeset): correct fragment format for 3753-consolidate-milestone-tests Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(changeset): add required frontmatter to 3753 changeset fragment Adds `type: Fixed` and `pr: 3753` frontmatter so docs-lint passes. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(tests): bug-2943 execFileSync timeout 5s→15s for Windows starvation PR #3754's milestone consolidation reshuffles run-tests.cjs chunk composition. On Windows/Node 22 under --test-concurrency=4, bug-2943 subprocesses now share concurrency slots with bug-2760-codex-install subtests (8–15s each), starving past the 5000ms timeout. 15s covers the observed 13.5s worst case with headroom. Refs #3753 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |