ddde001af6f99bcc6404a80dfbce99d1c0c81ced
5447 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
ddde001af6 |
enhance(#3873): the STATE.md schema — one owner, generated artifacts (#3880)
* test(#3873): failing-first locale parity, plus tripwires for what must not move Pins ADR-3473 §8.8 at the artifact a reader actually sees. The English STATE.md reference carries a Status lifecycle section that is missing from all four translations — the section documenting the status enum whose clobbering is #3853. The test derives the heading set rather than hard-coding the missing one, and names the locale and the heading when it fails. Two tripwires that must pass today and after. The field-drift guard still catches a re-derived fallback ladder: §8.8 instructs deleting that script, and that instruction rests on a wrong premise about what it guards, so the test stops a future reader from deleting it on the ADR's word. And last_activity's label resolution is pinned to what ships today, because it is declared in one of the two tables this phase consolidates and not the other — the consolidation must not silently pick a side. The locale test buckets under docs rather than state, which is what it tests; that bucket is allowlisted with justification rather than folded into an unrelated docs suite. It reads only markdown, so it carries no allow-test-rule marker — a marker there would suppress nothing and would grow the unverified pool against its ceiling. Refs #3873 * feat(#3873): one schema owns the STATE.md key set, three tables become projections ADR-3473 §8.8. The key set was declared in four places that had to agree by hand and already did not: FIELD_CLASSIFICATION, FRONTMATTER_BODY_SOURCE, FRONTMATTER_KEY_TO_BODY_LABEL and buildStateFrontmatter's emit behavior. One frozen null-prototype schema now declares each key's type, enum, cardinality, source, preservation, body source, body label, accepted parse shapes and whether it is emitted unconditionally; the three tables are derived from it at module load. The projections are byte-identical to the literals they replace, key order included, and the parity tests compare against verbatim copies of today's tables rather than re-deriving both sides from the schema — a parity test fed from one source proves nothing, which is how a consolidation ships a changed policy under a green test. last_activity was the live disagreement: present in one table, absent from the other. The schema declares what ships today rather than the tidier answer, and a test pins it. The schema is a leaf module and owns the four field-policy types, re-exported from state-transition so existing importers are untouched — the same split health-diagnostic-types made to break a CJS require cycle. Refs #3873 * feat(#3873): generate the schema-derived regions, parity-check the prose tables ADR-3473 §8.8's generator half. gen-state-md-docs.cjs owns marked regions in the shipped template and all five reference docs, follows gen-features.cjs's fail-closed contract, and is wired into regen:derived and lint:generated-sync. The Status lifecycle section was missing from all four translations — the section documenting the status enum behind #3853 — and is now generated into every locale. Field cardinality is a new generated table: pure schema data, no prose, so nothing to lose. The Field-reference and Status-values tables are parity-CHECKED rather than generated. Their Purpose, When-populated and Matched-text columns are genuinely hand-translated per locale, and §8.8 itself says prose stays hand-translated; generating them from an English registry would overwrite four locales' translations on every write. The row set is checked against the schema instead, so a key added to one and not the other fails, which is what field drift actually means. Building that check found last_activity_desc undocumented in all five tables. Three keys the docs describe are absent from the schema — active_phase, next_action, next_phases. They are grandfathered by name, not by wildcard, so a fourth fails: a declared gap with a forcing function rather than a silent one. Refs #3873 * fix(#3873): declare what the parsers do, and close the shape-parity gap Two declarations in the new schema described intended behavior rather than actual — the defect class this epic exists to end, committed inside the epic. Both were caught by executing the parsers instead of reading their docstrings. current_plan.acceptedShapes claimed ['N', 'N of M']. Standalone, the hybrid shape errors; the path that looks like support is parseInt truncating '2 of 5' to 2 and discarding the rest. Narrowed to ['N']. The parser is deliberately NOT fixed here: that is #3784 and PR #3791 is already doing it. When #3791 lands this row must widen, and the shape test will go red until it does — the schema and the parser cannot drift apart quietly, which is what §8.8's checked-not- generated rule is for. STATUS_LIFECYCLE_ENUM claimed to be the closed set status can hold. normalizeStateStatus passes unrecognized prose through unchanged, so it is not closed at runtime. The seven members are the canonical values it maps onto; the docstring now says that and the test asserts the real lenient contract. Closes the acceptance item that a test asserts the parsers accept exactly the declared shapes: the check is table-driven over every row carrying acceptedShapes, guarded against passing vacuously on an empty set, and fails loudly if a future row has no registered driver. Adds the unwired-label throw and the fast-check property that every projection agrees with its schema row. Refs #3873 * fix(#3873): keep the shipped template's frontmatter first, and make row 27 able to fail The remote matrix caught 12 failures with one cause. Making the template's frontmatter a generated region wrapped it in its own yaml fence ahead of the markdown fence, so extractFileTemplate and readShippedStateTemplateBody — which both match the single markdown block — found the heading first, not the frontmatter. That breaks the contract every new project's STATE.md is created from: bug #21 and epic #1969 B8 pin that the File Template block starts with frontmatter and carries gsd_state_version. The markers now sit inside the single markdown fence, so the fence opens before the frontmatter and the region still ends ahead of the heading. Same layout as before this phase, with markers embedded rather than a second fence. Row 27 existed to catch exactly this and did not, because it was writer-seeded: it asserted against the generator's own output shape, so it passed on the broken template. It now parses the fence the way production does and was verified to fail against the broken shape before being trusted against the fixed one. A test that would not have caught the bug it exists to prevent is worse than no test. The emitted-attribution failure was separate and the fragment was the wrong remedy: gsd-core/templates/state.md self-attributes under a verbatim-copy identity rule, so a diff touching it needs no acknowledgment. Fragment deleted rather than left explaining nothing. Refs #3873 * docs(#3873): how to change the STATE.md schema The phase gate was right and my docs artifact was wrong. I listed lint:generated-sync as the second enablement step, which is a verification command dressed as one, and then claimed a one-step sequence owed no how-to. The real sequence is build:lib then regen:derived, and the ordering is a trap: the generator reads the COMPILED schema, so regenerating before building regenerates against the previous schema and commits artifacts that look plausible while disagreeing with the code just written. A reference table cannot carry an ordering dependency; that is what the how-to test is for. The page covers adding, changing and removing a key, every reason code the check emits and what to do about each, what is generated versus hand-translated and why the two prose-bearing tables are parity-checked instead of generated, adding a language, and the three grandfathered keys. Indexed from docs/README.md. Refs #3873 * chore(#3873): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3b18eff388 |
enhance(#3872): what a command reports it wrote — the transaction diff (#3878)
* test(#3872): failing-first regressions for what a command reports it wrote Pins ADR-3473 §8.7 at the consumer's output. state planned-phase advances current_phase on disk and never reports it, and reports progress.total_plans which reconcileReportedFields silently drops because it cannot resolve a dotted key against nested frontmatter. Both directions of #3818's own before/after diff, reproduced against the real CLI. Also pins the two properties the change must not break: a fully-failed patch still reports an empty updated array, which is what state.cts:607's success boolean depends on; and two content-identical writes differ in last_updated alone. That second one measured state_head NOT to be ambient — it is recomputed every write but only changes when git HEAD moved — so the provenance exclusion is a one-element set, with a companion test pinning that state_head does change when HEAD moves. Refs #3872 * feat(#3872): derive what a command reports from the transaction diff ADR-3473 §8.7. reconcileReportedFields compared the transform's own output against persisted bytes and then filtered what preservation had restored by its FIELD_CLASSIFICATION policy. Both are replaced by one comparison of persisted against the pre-write state the transaction already holds, surfaced to the command through the same caller-allocates out-param idiom divergedFields established. Both of the old directions fall out of that single comparison: a field the transform reported but the pipeline discarded is persisted-equals-snapshot and drops out, and a field nobody reported but the write moved is different and appears. The classification filter is deleted, not relocated — no policy test remains anywhere in the reporting path. Reporting is at dotted-leaf granularity, enumerated from the progress.* rows FIELD_CLASSIFICATION already declares rather than by walking user data to arbitrary depth. That closes a live defect: plannedPhaseCore already pushed progress.total_plans and reconcileReportedFields silently dropped it, because a flat hasOwnProperty cannot resolve a dotted key against nested frontmatter. Current Position was lost the same way and is fixed in the same place. The exclusion is one field, last_updated, and it is by provenance rather than by classification: it is the only field measured to change on every write regardless of content. state_head was measured NOT to qualify — it is recomputed every write but only changes when git HEAD moved. Without that exclusion state.patch's success boolean, which is updated.length > 0, would be permanently true and a fully-failed patch would report success. Refs #3872 * fix(#3872): cover the matrix, and close a prototype-chain read the coverage found Review found 20 of 29 test-matrix rows uncovered. Covering them found two real defects rather than merely documenting the intended behavior. bodyLabelFor read FRONTMATTER_KEY_TO_BODY_LABEL with a bare bracket index on a plain object literal, so a field named __proto__, constructor or toString resolved to the inherited prototype member and leaked a non-string value into the updated array. Fixed with an own-property check, mirroring the discipline resolveFrontmatterPath already had. The security-relevant matrix row proved it before the fix. applyPostSyncPreservation still carried its own inline copy of the value comparison alongside the new stateFieldValuesDiffer, which is two live copies of one rule introduced by the epic that exists to remove them. Routed through the single owner. Adds the fast-check property that a field appears iff its persisted value differs from the snapshot, the string-versus-number representation boundary, dotted paths into missing parents and into scalars, deleted and added keys, and the preserve-if-placeholder pair that proves no classification test survives in the reporting path. Refs #3872 * docs(#3872): document the transaction diff on the write path The updated array's contract belongs where the write path is described. States the iff rule, leaf granularity, the single provenance exclusion and why state_head is deliberately not one, and closes with the consequence a reader actually needs: these arrays are longer than they used to be, because they used to under-report. Refs #3872 * fix(#3872): a derived leaf materializing is not a change the caller made The remote matrix caught 17 failures with two causes. The substantive one is that progress is source: disk, and the disk cannot change during a STATE.md write — the write only touches STATE.md. So a progress block appearing where the snapshot had none is the scanner populating a document that had never been synced. The bytes moved; nothing the caller did moved them. That is the same shape as last_updated one level up, so the provenance rule is generalized rather than special-cased: a field appears iff its persisted value changed for a reason attributable to this write's action, and two cases are not attributable — a field stamped unconditionally on every save, and a declared derived leaf materializing from a source that did not change. Crucially this does not consult the preservation policy, so the filter §8.7 deleted stays deleted; it uses the declared leaf set to know which keys are derived. This had a second production consumer the earlier review concluded did not exist: cmdStatePlannedPhase gates publishStateContract on updated.length, and its own inline comment predicts exactly this failure. A no-op call was publishing state.json. advancePlanNoOpDoesNotPublish genuinely encoded pre-§8.7 behavior and moves. E2 and E6 had carved out total_plans as reportable-on-materialization, an error introduced earlier on this branch rather than a pre-existing pin, and are corrected with it. Refs #3872 * chore(#3872): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
8f674281fd |
chore(#3875): sweep the spent ack fragments and automate the sweep (#3877)
* chore(#3875): sweep the spent ack fragments and automate the sweep
next has been red on every push since
|
||
|
Tom Boucher
|
1863f5569c |
enhance(#3871): the state transaction — mandatory snapshot, open()/rebuild() (#3874)
* test(#3871): failing-first regressions for the dropped curated progress block Pins ADR-3473 §8.6 / #3756 at the consumer's output: state record-session and state add-decision on an archived-milestone project drop the curated progress frontmatter entirely, exit 0, and report nothing. Reproduced against the real CLI before writing the tests, not inferred from the issue text. Also adds the unit-level probe that applyStatePreservation's preserve-always row is inert on a resyncing write, and an over-preservation guard that an empty project is never inflated. Refs #3871 * feat(#3871): make the STATE.md pre-write snapshot mandatory via open()/rebuild() ADR-3473 §8.6. StatePreservationInput's nullable preFm and the always-present preFmSnapshot were the same extractFrontmatter call, one of them nulled on resync — a policy flag baked into a snapshot. Both collapse into a single StateTransaction whose snapshot cannot be absent: openStateTransaction() applies preservation, rebuildStateTransaction() does not, and both carry the snapshot because the reporting phase needs it either way. An absent snapshot is now a construction failure; an empty one stays legal, because that is what a document with no parseable frontmatter honestly has. writeStateMd requires a rebuild transaction, which types ADR-3408 §8.3's closed exception list at both call sites (state sync, health --repair) instead of matching them as strings in a ratcheted baseline. Fixes the dropped curated progress block: an all-zero or absent derived total set is an unmeasured scan, not a measurement, so the curated block stands. Also fixes two defects surfaced while building — preserve-always reported a mutation even when it restored an identical value, and it re-entered the curated object by reference, which would alias the snapshot the next phase diffs against. Refs #3871 * fix(#3871): close the three remaining subsumed defects and restore the arm the type does not replace Review of the first two commits found four things. The guard shrink deleted the seam-bypass axis whole, but only its writeStateMd( arm became redundant. Its other arm catches a call site re-assembling syncStateFrontmatter + applyPostSyncPreservation instead of the owned composition, which the transaction type does not make unrepresentable and which #3469 found live. Restored as findCompositionBypasses, terminal rather than ratcheted. Three of the four issues this phase claims were untouched. All three are the epic's own shape and are fixed at the seam: current_phase_name is reasserted from the curated value when the caller names none, and cmdStateJson stops carrying a hand-maintained list parallel to FIELD_CLASSIFICATION and projects it instead. The construction failure that is the point of this phase had no test. Every enumerated matrix row now has one, including the measured-versus-unmeasured coercion boundary and a seeded property that no curated key is ever dropped. ADR-3473 §8.6 said the guard 'keeps only its raw-write check'. Verified against next: there was no raw-write check, and four other checks it does not name. Amended in place with the evidence. ARCHITECTURE.md separately advertised a preservation policy the code had deleted. Refs #3871 * fix(#3871): do not let the unmeasured-scan rule block an explicitly-requested resync The remote matrix caught over-preservation, the failure this phase's own negative space says must not happen. state update Progress re-derives the block from the body the caller just rewrote; on a project with no phase dirs the derivation yields zero totals, the unmeasured rule read that as 'the scan measured nothing', and the stale curated percent was restored over the resync the user asked for. preserve-always already said what the missing condition was: never overwrite unless the caller explicitly names this field. explicitProgressField carries it and is derived from shouldResyncStateProgress, not set by hand at a call site, so it cannot drift from what the caller asked for. Two defects found in the same mechanism and fixed with it. readModifyWriteStateMd enumerates its option keys, so a new option was silently dropped rather than rejected. And the raw-write axis captured its first argument up to the first comma, which lands inside a nested path.join, so a write to a STATE.md literal was invisible to it — the prove-it-can-fail test caught that one immediately. No test assertion was weakened; all three frontmatter rows encode #3242, #1969 B3 and #1972 and stand unchanged. Refs #3871 * docs(#3871): record why the raw-write check is kept, not why it was named The amendment justified findRawStateWrites as 'written because §8.6 requires it to exist', which is cargo-culting the contract and would have been the wrong reason to keep anything. The real reason is that writeStateMd acquires the STATE.md lockfile and a raw fs.writeFileSync acquires nothing, so this is a lock bypass and lost-update is the #500/#905/#1230 family — and after this phase it is the one reachable path into the file that nothing else covers. Also records why ADR-3408 §8.6's deletion of the 'clear' policy is not the precedent it looks like: 'clear' was dead vocabulary in a closed enum, this is coverage of a reachable path. Refs #3871 * chore(#3871): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
382bf7c423 |
fix(#3706): deliver the resolved reasoning effort to OpenCode subagents (#3867)
* test(#3706): failing-first coverage for OpenCode variant emission and frontmatter escaping * fix(#3706): emit the resolved reasoning effort as OpenCode's variant key `query resolve-execution` resolved an effort level for every agent, but the OpenCode bake wrote only `model:` — the effort never reached the generated agent, so subagents ran at whatever the runtime defaulted the model to. This is the effort-side twin of the model-side defect fixed in #3705. The key is written only when an `effort` block is actually configured. `resolveInstallTimeEffort` always returns a level (the catalog default is `high`), so gating on its return value would stamp `variant: high` into every existing OpenCode install — and OpenCode resolves a variant name against a `variants` map in the user's `opencode.jsonc`, so a value nobody declared is not a safe default. Gating on `readGsdEffectiveEffortConfig` keeps installs that never asked for effort routing byte-identical. Kilo does not receive the key: `EFFORT_ARGV` declares surfaces for claude, opencode and codex and has no kilo entry. This is deliberately asymmetric with the model side, where #2794 J8 requires the two runtimes to resolve alike. Both frontmatter sinks now route through `frontmatterScalar`, which quotes and escapes any value that is not a plain scalar. The raw interpolation predates this change, but it was already shown by execution during the #3705 security review to let a config value containing a newline inject additional top-level keys (`tools:`, `permission:`) into a generated agent file. This change adds a second write to that sink, so it is closed here rather than doubled. * fix(#3706): quote frontmatter values YAML would not read back verbatim Self-review of the predicate added in the previous commit. Treating /^[A-Za-z0-9._:/@+-]+$/ as 'safe to emit bare' answers the wrong question: a value can match it and still not round-trip. - A leading '@' is a YAML *reserved* indicator and may not open a plain scalar at all, so a scoped ID like '@org/model' emitted bare is a parse error, not an ambiguity — the whole agent file becomes unreadable. - 'no' / 'y' / 'off' / 'null' resolve to booleans and null, so a variant with one of those names would match no entry in the user's variants map. - '12:30' resolves to 750 under YAML 1.1 sexagesimal, and ':' is legal mid-identifier here, so the form is reachable rather than contrived. Real model IDs pass every clause and stay bare, so already-generated files remain byte-identical. * fix(#3706): route variant through the declared effort seam and cover the live path Addresses six findings from the isolated review, all confirmed by execution. The tests were the serious one: they required `../bin/install.js` while the fix landed in src/, which compiles to gsd-core/bin/lib/. They exercised a different copy of the converter than the one the bake actually uses, so the whole suite was green-by-construction against unchanged code and the remote run failed all 13. Every case now runs against BOTH copies from one table, which doubles as the parity assertion the generative-fix note in runtime-artifact-conversion.cts asks for, and bin/install.js carries the mirrored change. Emission no longer hand-rolls the value. It goes through `renderEffortArgv`, the declared OpenCode effort seam (EFFORT_ARGV.opencode: its own supported set and clamp). That is what rejects a level that is not a wire value — above all `inherit`, which per #3533 (10d) means "omit the key and follow the host default" and was previously written literally, naming a variant that cannot resolve. Reachable two ways, both now pinned: an agent_overrides entry and a routing_tier_defaults entry. A bare effort.default does NOT reach a tiered agent (the #3531 tier ladder answers first), so a test written against `default` alone asserts nothing — that is pinned too. The plain-scalar decision moved into frontmatter.cts beside `scalarNeedsDoubleQuoting` rather than sitting next to it as a second, weaker predicate. `agentScalarNeedsDoubleQuoting` is a documented superset: it adds a trailing `:` (read as a nested mapping key, which fails the whole frontmatter), boolean/null words, and numeric-looking values including YAML 1.1 sexagesimal. Docs now state the cascade plainly: the gate is on effort being configured at all, not on the individual agent being named, so every generated OpenCode agent gets a variant line once any effort block exists. * test(#3706): assert the two frontmatterScalar copies cannot diverge A hand-picked adversarial corpus plus a fast-check property over YAML-significant strings, both run against bin/install.js and the live src copy. Verified the property can actually fail: mutating one copy's quoting rule is killed well inside the run budget. * fix(#3706): close the review findings — predicate, seam, and dead mirror Third review round; every item below was confirmed by execution. The scalar predicate was wrong in two families, both found by a round-trip property test rather than by reading. Basing it on scalarNeedsDoubleQuoting dropped the "first character must be alphanumeric" clause, so `~`, `.inf`, `.nan`, `+1`, `-0` and `.5` went out bare and came back as null/floats/ints; and that base predicate only inspects the FIRST character, so an embedded `: ` (a nested mapping, i.e. a parse error) or ` #` (a comment, i.e. silent truncation) also passed. Dates round out the set: `2026-08-25` opens alphanumeric, survives every other clause, and YAML resolves it to a Date. The property now asserts the contract directly over generated values instead of trusting an enumerated character list. The bin/install.js mirror is gone. Its premise was false — install.js already requires bin/lib at :65 — and it was unreachable besides: install.js's convertClaudeToOpencodeFrontmatter has no `isAgent: true` call site, because its agents path resolves converters from the compiled module. It was a third copy of the YAML rules serving a test rather than a caller, so the file is back to origin/next and the tests target the live copy only. Effort clamping moved to `clampEffortForHost`, which renderEffortArgv now delegates to. The layout was calling renderEffortArgv with a hardcoded 'argv' to borrow its clamp, which read as if the frontmatter key were gated on the invocation-time axis. It is not: claude declares effortSurface "argv" and independently bakes an effort: key. One capability table, one clamp, two channels that no longer pretend to be each other. Also corrects an earlier claim of mine: adding EFFORT_RENDERING.opencode would NOT have made `effort sync` write the wrong key, because it guards on the runtime name before it ever renders. The seam choice stands on other grounds. `effort sync` still skips OpenCode, but its stated reason claimed OpenCode "does not use effort: frontmatter", which this change makes false — so the message now says what is actually true. * docs(#3706): restate the changeset around the round-trip contract * fix(#3706): restore the changeset fragment belonging to #3809 An earlier commit in this branch picked the first file in .changeset/ by glob order instead of the fragment created for this issue, and overwrote agile-geese-squeak.md (PR 3815 / #3809) with this change's body. Restored verbatim from origin/next; this change's text now lives in its own patient-cranes-parade.md, where it was created. * feat(#3706): maintain the OpenCode variant key from effort sync Install bakes the resolved effort into OpenCode agent frontmatter as `variant:`, so `effort sync` has to maintain it or a config change only takes effect on reinstall — and its skip message claimed OpenCode does not use frontmatter effort at all, which this issue made false. cmdEffortSyncOpencode mirrors the codex branch: resolve per agent, clamp through the declared OpenCode capability, then write, strip, or skip. A null target means the key must not exist, which covers both "no effort configured" and "resolved to inherit or to an unsupported level" — the same states under which install writes nothing, so sync and install agree by construction. The frontmatter line-editors are key-parameterised rather than copied: setEffortFrontmatter / removeEffortFrontmatter are now thin wrappers over the same internals the variant path uses, and a test pins that the claude `effort:` behavior did not move. The child-process test harness fixes both HOME and USERPROFILE, so the hermetic-config assertions cannot pass vacuously on Windows. * fix(#3706): scope the frontmatter line editors to the matched block Found by the security review of the sync path, reported as correctness rather than vulnerability, and reproduced against pre-fix code before being fixed. Both editors matched the frontmatter with a regex that can match a block after a preamble, then derived the EOL and the opening-fence length from the START OF THE FILE. On a CRLF document with a preamble those disagree, the offsets shift by one byte, and the reassembled document comes back with a mangled fence (`---\rname: x`). Both now take the EOL from the matched block. `setFrontmatterKeyLine` additionally did a whole-file `/m` replace when the key already existed, gated only on the key being present in the frontmatter body — so a preamble line starting with the same key was rewritten instead of the frontmatter one. It now replaces inside the frontmatter span only, which is the hazard `removeFrontmatterKeyLine` already documented and guarded against. Neither is reachable from an install-written `gsd-*.md` (those begin at byte 0 with `---`), and both predate this change — but the editors are in this diff because #3706 key-parameterised them, so they are fixed here rather than left for the next caller to trip over. Three regression tests, each confirmed to fail against the pre-fix build. * fix(#3706): treat a present-but-empty key as present, and pin the real seam Fourth review round. The MAJOR one: both sync branches read the current value with `(.+?)`, which needs at least one character, so a key present with an EMPTY value read as "key absent". When the target was also null the code concluded "already correct" and skipped — leaving the key in the file, where it reads back as YAML `null`: exactly the unresolvable-variant state this change exists to prevent. Whitespace decided whether it fired, since `variant: ` matched and `variant:` did not. Presence and value are now separate questions at both the opencode and the claude branch. The OpenCode writer now follows the codex branch rather than the claude one: tmp file plus retryRenameSync with orphan cleanup, and a write failure skips that agent and is reported instead of aborting the sweep. Same granularity, same transient-Windows-lock exposure, so the hardened sibling was the right precedent. Also: the generic line-editors escape their interpolated key, the JSDoc stranded by the clampEffortForHost extraction is back on renderEffortArgv, and a cast that declared a nullable function as non-nullable is corrected. Tests close the gaps the review listed — empty value (both spellings), CRLF round-trip through write and strip, the symlink guard, a body line starting `variant:`, a file with no frontmatter, and the YAML classes that actually broke the predicate. The new layout-seam test drives the real stage() path and was verified to FAIL when `variant` is removed from the converter call; a seam test that survives cutting the seam is worse than none. * fix(#3706): clear the round-five review findings No blockers or majors this round; the repo's review gate is zero-tolerance, so the minors are cleared too. A duplicated key was only half-stripped: the strip regex had no `g` flag, so a frontmatter carrying the key twice lost one occurrence, reported success, and left the "a null target means the key must not exist" invariant false on disk — converging only on a second run. Such a document is already invalid YAML, so this is robustness rather than a live corruption path, but a successful sync has to leave the invariant true. A run in which every write failed still summarised as `ok`, so a caller could not tell "nothing to do" from "everything failed". The OpenCode branch now reports `failed` when any write failed. The write-failure path was also the newest code in the change with no coverage at all; it now has a test that injects the failure by monkeypatching the write, per CLAUDE.md §4, rather than by chmod — mode bits do not bite under root in CI. `CodexEffortSyncWriteFailure` is renamed `EffortSyncWriteFailure` now that two branches share it. Removed a guard on the claude concrete path that was provably unreachable — no member of EFFORT_SET renders null there, so it read as protection that did not exist. The claude inherit path's presence check is load-bearing and untouched. Three stale statements corrected: the OpenCode result shape matches codex's, not claude's, now that it emits write_failures; the `thread()` test helper now calls `clampEffortForHost` so it genuinely mirrors the layout instead of merely claiming to; and a test helper restored `USERPROFILE` by assignment, writing the literal string "undefined" into the environment on POSIX — it deletes now. * fix(#3706): converge the set path, degrade on unreadable files, preserve mode Rounds five and six of review. No blockers or majors; the review gate is zero-tolerance, so the minors are cleared too. `setFrontmatterKeyLine` was the mirror of a defect already fixed in its sibling: `remove` was made global, `set` was not, so on a frontmatter carrying the key twice it rewrote the first and left a stale second. Last-wins YAML readers honour the stale value while the sync's own first-occurrence read reports "in sync" — permanently non-converging. It now collapses to exactly one occurrence, in the position of the first, so ordinary single-occurrence documents stay byte-identical (verified across seven shapes before and after). An unreadable agent file used to throw and abort the entire sweep, while a failed WRITE in the same loop degraded into a report. The OpenCode branch now reports read failures alongside write failures; the claude branch degrades to a skip without a new result field, because its shape is long-standing and widely consumed and one bad file aborting the sweep is the actual defect. The tmp+rename publish dropped the original file's mode — a plain writeFileSync preserves it, a rename does not — so a 0600 agent came back 0644. Both the OpenCode and the codex branch now carry the original's permission bits across the publish, masked with 0o7777: the raw stat mode includes the file-type bits, and POSIX leaves those unspecified for chmod. Linux is the only OS the remote matrix runs, so relying on Darwin's tolerance would have been untestable here. Also documents the `from` contract on EffortSyncChange (null means the key was absent, '' means present with an empty value — a distinction earlier rounds introduced and then collapsed in the output), adds OpenCode to the docs paragraph enumerating where the key is omitted under inherit, and records in a comment that the 'failed' summary reaches only raw mode and does not change the exit code, which is a CLI-contract change affecting all three branches and is deliberately not made here. * fix(#3706): guard the codex read, close the tmp permission window, rename the failure type Round seven, plus one thing I found myself. `cmdEffortSyncCodex` still had an unguarded `fs.readFileSync` — a read fault on one agent exited 1 and aborted the whole sweep. The claude and opencode branches were both guarded earlier this round and codex was missed, with the unguarded read sitting ten lines above the chmod block the previous commit did edit. It now reports read failures the way the OpenCode branch does, and a read failure flips its summary to `failed` — which write failures did not do there either, so both are corrected for consistency. The tmp file was created at the default mode and only tightened afterwards, so a 0600 agent's contents sat in a 0644 file for the length of the publish. I measured the window rather than assuming it, then closed it by passing the mode at creation. The chmod after the write is deliberately RETAINED and commented: the `mode` option only applies when the file is actually created, so a leftover tmp from an earlier crashed run would be truncated and reused at its old mode, and the chmod is what corrects that. `EffortSyncWriteFailure` is renamed `EffortSyncFileFailure` — it was typing a `read_failures` array, the same naming-lie the `Codex…` prefix had last round. Also pins the codex mode preservation with a test. It only writes on a path that genuinely rewrites the file, so the fixture is an Anthropic-flavoured model pin the sync strips, and the test asserts the content changed before checking the mode — otherwise it would pass on a sync that did nothing. * fix(#3706): guard the claude writes and share one escaping rule The security sign-off caught a comment of mine that was factually wrong: the new claude read guard said the failure is folded in "like the write path in this same loop does", and there was no write guard in that loop. Rather than correct the sentence, both claude write sites are now guarded the way the read is — a failed file is skipped, the sweep continues, and the raw summary token flips to `failed`. The JSON shape stays frozen deliberately, because it is long-standing and widely consumed; the token is the channel that can carry the signal without a compatibility risk, which is the reviewer's own suggestion. That makes all three branches consistent: reads and writes guarded everywhere, per-file failures degrade instead of aborting, and every branch reports `failed` rather than `ok` when something did not sync. `setFrontmatterKeyLine` interpolated its value raw while the install-side writer quoted through the shared helpers — two writers of the same frontmatter key disagreeing on escaping, the divergence class this repo requires closed. They now share one rule. Verified no churn: all six effort levels are plain scalars and emit byte-identically, with claude's documented minimal-to-low clamp the only difference in the table, exactly as before. * fix(#3706): publish claude agent writes atomically too Both reviewers found this independently, and it is data loss rather than a reporting gap. The claude branch wrote in place, so `fs.writeFileSync`'s O_TRUNC meant a post-open fault left the agent file truncated or half-written: an injected ENOSPC produced an empty file, and under `ulimit -f` a 60000-byte agent came back as 512 bytes of wrong content. The guard added earlier this round then counted that destroyed file as `skipped`, which in JSON mode is indistinguishable from "already in sync" — so a caller would have read the sweep as clean while an agent on disk was corrupt. It now publishes the way the codex and opencode branches already do: write to a tmp file created at the original's masked mode, chmod, then retryRenameSync, with the tmp unlinked and the agent skipped on any failure. The corrupting case is gone rather than merely reported, which matters because this branch deliberately takes no new result key. I had claimed all three branches were consistent after the previous commit. That was true for degradation and reporting and not for atomicity; the reviewer caught the overclaim. It is true now. Also sorts the claude file list, which the other two branches already did — readdir order is platform-dependent, so leaving it unsorted made the reported `changes` ordering differ across machines for identical inputs. * chore(#3706): backfill the changeset PR number pr:0 placeholder replaced with the real PR now that gh api returned it. * test(#3706): kill the frontmatter mutants this change introduced CI's Stryker frontmatter shard scored 60.58 against a break floor of 62. The cause is documented in the lane's own config, from #1882: this PR added a multi-clause predicate to frontmatter.cts and exported the escaper, but the tests constraining them live in tests/runtime-converters.test.cjs, which that shard does not run — so every mutant in the new code was uncovered there even though the behaviour is tested elsewhere. The fix is assertions that kill real mutants, per the repo's own instruction, not a lowered floor and not a Stryker disable: scripts/mutation-matrix.cjs is untouched. Each clause of agentScalarNeedsDoubleQuoting now has a true case AND a near-miss that must answer the opposite way, so flipping the clause fails a specific named test — alnum-first against `a-b`, trailing `:` against `foo:bar`, embedded `: ` against `a:b`, embedded ` #` against `a#b`, the word list against `yes1`/`nullish`, the numeric forms against `1a`/`0xzz`, the timestamp against `2026-08-25x`, plus the case-insensitive spellings that pin the `i` flag. escapeDoubleQuoted is pinned on exact output, including a case constructed so that escaping in the wrong ORDER yields a different string. Two of my expectations were wrong and are asserted as the code actually behaves: `12:99` is NOT quoted, because the sexagesimal alternative never range-checks minutes and so does not match — which is right, since YAML would not read it as sexagesimal either; and `20260825` is quoted by the numeric clause rather than the timestamp one, being a bare integer. * chore(#3706): ratchet the frontmatter mutation floor to 65 The lane measured 66.67 on PR 3867 after the mutant-killing unit tests landed — above its pre-change 63.35 baseline, not merely recovered. Step 3 of this file's own HOW TO UPDATE procedure says to set minScore = floor(measured) - 1 in the same diff, so 62 becomes 65 and the improvement is locked in rather than left free to slide back. The ledger of measured scores now records the new measurement, why the shard broke in the first place (logic added to frontmatter.cts whose only tests lived in a file this lane does not run — the same trap the #1882 note describes), and one discrepancy: step 3 also says to update "the matching RATCHET_BASELINE entry", but no such declaration exists in this file. The name appears only in that comment, so minScore and the ledger are all there is to update. * fix(#3706): update RATCHET_BASELINE alongside the raised floor The ratchet test caught the previous commit: it raised COVERED['frontmatter'] .minScore to 65 without updating the baseline that mirrors it, which is exactly the mismatch that guard exists to make visible in review. I had claimed RATCHET_BASELINE did not exist. It does — in tests/mutation-matrix-ratchet.test.cjs, not in scripts/mutation-matrix.cjs, which is the only file I searched before concluding it was a stale reference. The ledger comment is corrected to say where it lives and to record that the guard caught the error rather than leaving my wrong claim on the record. * docs(#3706): put the mutation ledger entries back under their own dates The 2026-08-25 measurement was spliced into the middle of the 2026-06-14 list, so adr-parser, config-schema, active-workstream-store and core-utils ended up sitting under the wrong heading and misattributing their measurement dates. That ledger is what a future change reads to calibrate a floor, so a wrong date there is not cosmetic. Each measurement is now under the date it was taken. Also drops the first-person account of my own mistake from the entry — the factual half (where RATCHET_BASELINE lives, and that it is updated in the same diff) is what a reader needs; the confession is not. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
86fa2917d7 |
enh(#3866): dispatch step and contribution hooks at verify:pre (#3869)
* test(#3866): pin that verify:pre must dispatch every hook kind verify-work.md's verify_pre_hooks step dispatches only `kind == "gate"`, so getWiredKinds reports verify:pre -> {gate} and gen-capability-registry rejects any capability declaring a step or contribution there. The verify lane is therefore closed to capabilities that want to contribute to what UAT covers rather than refuse to let it start. Failing-first: the step, contribution, and exact-kind-set rows are RED; the pre-existing gate row is a green regression pin so the new arms cannot orphan the arm verify:pre already had. Refs #3866 * feat(#3866): dispatch step and contribution hooks at verify:pre verify_pre_hooks dispatched `kind == "gate"` only, so getWiredKinds reported verify:pre -> {gate} and gen-capability-registry's validateHooksWired rejected any capability declaring a step or contribution there. A capability could refuse to let UAT start; it could not contribute to what UAT covers. Add contribution and step arms mirroring execute:wave:post, deferring to references/loop-hook-dispatch.md and carrying its ref.command in-context validation guard ahead of any shell-use prose. A verify:pre step is advisory: it never blocks the start of UAT and an erroring step is routed by its own onError. The gate arm and its check guard are untouched. Give extract_tests an additive consumption seam for the artefacts those steps declare via the existing steps[].produces field -- no new registry field, no new ordering, no invented filename. Manifest-supplied artefact names are validated in-context against an allowlist and resolved only inside PHASE_DIR. With no producing step the derivation is unchanged, pinned by test rather than asserted in prose. Review findings folded in: the artefact-name allowlist (isolated adversarial pass), the artefact-shape contract and the seam-inertness tests (spec axis), and the reference/how-to split so one constraint has one source of truth (standards axis). Closes #3866 * chore(#3866): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7bbbe495d7 |
docs(#3473): ADR-3473 — enforcement by construction, one owner per invariant (Phase 0) (#3870)
Phase 0 of epic #3473 ships this ADR alone. Every rule in §8 carries a status of Enforced or Required — Phase N. §8.1–§8.5 state the epic's B1–B5 criteria as contract rules with no phase assigned yet. §8.6–§8.8 assign Phases 1–3 to the STATE.md family that ADR-3180 and ADR-3408 left between them: the write seam's unvalidated input, its classification-excluded reporting, and the schema transcribed by hand into nine artifacts. Derivation authority is explicitly NOT in scope here — it lands as ADR-3180 Amendment 8, generalizing §7.5's already-locked sentence. Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
e40e9670f8 |
fix(#3705): consult model_policy in the install-time bake so agent frontmatter matches dispatch (#3863)
* test(#3705): failing-first coverage for model_policy in the install-time bake * fix(#3705): consult model_policy in the install-time bake so frontmatter matches dispatch * fix(#3705): inject the effective runtime into the policy so runtime_tiers is reached * test(#3705): use assert.doesNotMatch, the assertion that exists * chore(#3705): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
sim
|
f7df920681 |
chore(#3841): backfill changeset PR number
Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
67335c498c |
fix(#3841): express the anchor semantically so the pretty payload still verifies
The remote matrix caught a regression I introduced in the previous commit. The anchor check was implemented as a byte-prefix match against IDENTITY_RAW_PREFIX, which describes the `--raw` wire format -- but `cmdRuntimeIdentity` without that flag pretty-prints at indent 2, and the classifier is handed BOTH serializations. Only the shell is restricted to `--raw`. The pre-existing test that runs the real verb with no flag went red: `+ 'unparseable' - 'ok'`. No local gate caught it. build:lib, eslint and lint:ci were green throughout, because none of them execute tests. The anchor now reproduces its two properties semantically instead of byte-wise, and both hold for either serialization: the payload begins at the first byte of stdout, and `packageName` serializes first. IDENTITY_RAW_PREFIX stays exported with its own tests -- it is the wire contract for the shell, not a general classifier predicate, and conflating those was the error. Adds the two rows the matrix was missing: the default pretty serialization verifies, and a pretty payload with `packageName` not first does not. The design and matrix now record that they enumerated only the inputs the SHELL produces and assumed the classifier's input set was the same. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
sim
|
5e997de5f0 |
fix(#3841): honor the payload anchor in the classifier, dedup the fixture
Three review findings, all fixed.
The isolated security review found a SECOND divergence the design missed. The
classifier parses structurally, so it accepted `packageName` at any key
position; the shell's `case` is anchored at the start of stdout. For
`{"note":"x","packageName":"<us>",...}` the classifier said ok and the shell
said unverified -- a fail-open disagreement, and none of the original ten parity
rows caught it because every one put `packageName` first. Certifying agreement
that does not hold would have been worse than shipping no parity suite. The
classifier now honors the anchor on its ok arm, which is what the module already
claimed to do: IDENTITY_RAW_PREFIX is documented as "ANCHORED, never a substring
search". A foreign packageName stays identity_mismatch wherever it appears, so
that arm is untouched. Parity rows P11-P13 added.
The spec review found the test matrix marked the empty-string `packageName` row
as already covered. It was not -- the `.length > 0` guard is a distinct path
from "no packageName key at all", which is tested. Added, and the matrix
corrected to say it was wrong.
The standards review flagged ~45 lines of fixture helpers duplicated verbatim
between the two preamble describes. Extracted to one `makeIdentityFixture()`.
Changeset rewritten: it described only the exit-code half of the diff.
Refs #3841
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
||
|
sim
|
3b8e4f3e5a |
fix(#3841): parse the identity payload before consulting the probe exit code
`classifyIdentityProbe` short-circuited on a non-zero exit BEFORE it looked at stdout, so a tool that had already proved its identity and merely exited non-zero was classified `no_identity_verb`. The launcher preamble that this module speaks for reads stdout only -- its command substitution discards the status -- so the two surfaces disagreed on exactly that input: shell `ok`, classifier `no_identity_verb`. Measured against the real snippet, not inferred. That matters because the classifier is the engine for the announced hard-fail phase and has no production caller yet. An install verified by today's warn phase would have been refused the moment hard-fail landed, in the phase where that stops the run rather than printing a line. Nothing recorded or tested the difference. The classifier moves rather than the shell: the shell is the shipped path with observable dependents, the classifier has none. The predecessor defence is untouched -- a usage screen yields no usable payload, so it still falls through to the exit-code branch. Adds the cross-surface parity test the gauntlet requires for two surfaces implementing one decision: ten probe behaviors driven through both the real snippet and the classifier, asserting they agree with each other. Surfaced by re-running the feature-implementation directive's design and QA steps against the code merged in #3848, which shipped without them. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
8bfb5c47c9 | fix(#3842): recover ack paths from pull requests past the per-PR file cap (#3857) | ||
|
Tom Boucher
|
27318ecec3 |
fix(#3704): rewrite fnm versioned node paths to the stable alias on macOS and Linux (#3856)
* test(#3704): failing-first coverage for fnm versioned-path normalization on POSIX * fix(#3704): rewrite fnm versioned node paths to the stable alias on POSIX * fix(#3704): share one root normalizer across both fnm branches so no baked path carries a doubled separator * chore(#3704): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
aa6c332b5a |
fix(#3701): resolve next_phase from the roadmap, selecting the numerically lowest successor (#3852)
* test(#3701): failing-first coverage for roadmap-order next_phase resolution * fix(#3701): resolve next_phase from roadmap order, keeping the disk scan for spelling and fallback * fix(#3701): select the numerically lowest successor in both scans, not the first row encountered * chore(#3701): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
308c17505c |
fix(#3840): reject a malformed feature order instead of coercing it (#3851)
* fix(#3840): reject a malformed feature `order` instead of coercing it `scripts/gen-features.cjs` was the one field validated by coercion rather than by shape. `Number('')` is 0, and `0x10`, `0b11`, `0o17`, `1e3`, `1.` and `.5` all coerce to finite numbers, so a fragment declaring a bare `order:` sorted to position 0 -- ahead of every real feature, in both the body and the generated table of contents -- with zero violations, a clean `--check` and `--write` exiting 0. That is a fail-open in a gate whose entire contract is a typed violation rather than a silent guess. `order` is now shape-checked against an optionally-signed decimal literal before coercion, mirroring how ID_RE guards `id`. The finite check stays: the regex alone would admit a literal long enough to overflow to Infinity. Surfaced by re-running the feature-implementation directive's design and QA steps against the code merged in #3845, which shipped without them. Refs #3840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3840): backfill changeset PR number Refs #3840 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
|
fb2d122d7f |
feat(#3841): assert gsd-tools identity on every state-mutating verb (#3848)
* feat(#3841): assert gsd-tools identity before any state-mutating verb only this package publishes. The path-based branches — a project-local install, a runtime config directory — had no such guarantee; they trusted their configured location. This closes them. Mechanism: once resolution finishes, and before any verb runs, the preamble probes the tool it picked with `runtime-identity --raw` and matches the answer with a shell `case` pattern ANCHORED to the start of the compact payload (`{"packageName":"@opengsd/gsd-core"`). An unanchored substring match accepts the decoy `{"packageName":"get-shit-done-cc","note":"@opengsd/gsd-core"}`, which any colliding package could publish. The outcome is exported as the two-valued `GSD_IDENTITY_STATUS` (`ok`/`unverified`), so the gate is asserted on a VALUE rather than on warning prose. Rollout is warn-then-fail per the #3146 ruling: `unverified` prints one line naming BOTH causes and continues, because `no_identity_verb` cannot tell a foreign package from an `@opengsd/gsd-core` older than the verb, and at rollout the old-version case is the common one. The blocker was byte budget, not design. The preamble is inlined into 112 shipped files and several sat within single-digit bytes of frozen ceilings (`gsd-verifier.md` 16 bytes, `gsd-executor.md` 33, `execute-phase.md` 234); a first attempt broke five of them. What made room was collapsing the resolver's twenty near-identical `elif [ -f … ]` arms into one candidate-list helper (`_gsd_at`), which buys far more than the assertion costs. The preamble is now 2,624 bytes against 4,500 — a net 1,876 bytes SMALLER per inlined file, so every capped file moved away from its ceiling rather than toward it. No cap raised, no size-budget exception added, no override token emitted. Resolution order, every runtime-home probe, the `unset -f gsd_run` re-source fix, the fail-closed `exit 1`, and the `CLAUDE_ENV_FILE` persistence are all preserved byte-for-byte in substring terms; the snippet still begins with `_GSD_SHIM_NAME=` and still ends with `fi`, which the parity extractors anchor on. `gsd-core/references/gsd-run-resolver.md` is re-synced byte-equal. Also fixes two stale claims found in passing: CONTEXT.md and FEATURES.md both described an `[ -x ]` guard as the load-bearing re-source defense. That guard was tried and REMOVED in #3831 — it rejected the bare function name, fell through every branch, and hit `exit 1`, which kills a sourced caller's shell. `unset -f gsd_run` is the actual mechanism. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3841): pair the anchor's brace by requiring a closed identity payload The matrix went red on `tests/new-project-mvp-prompt.test.cjs` — "new-project.md has unbalanced braces: net depth 2" — plus a knock-on report from its parent `bug #1516` describe, which is the same failure counted once at the child and once at the block. Root cause: that guard (:182-189, mirroring #3784 bd53925f) walks characters and increments on `{`, decrements on `}`, with no awareness of shell quoting. It scans `new-project.md` PLUS every `new-project/steps/*.md`, and both `new-project.md` and `steps/auto-mode-config.md` carry one inlined preamble copy — hence net 2 from a snippet that was off by exactly one. The unpaired brace was the `{` inside the single-quoted `case` pattern of the identity anchor, which is correct shell and invisible to a text scanner. Fix in the snippet, not the guard. The pattern now anchors at BOTH ends: `'{"packageName":"@opengsd/gsd-core"'*'}'`. That balances 51/51 with a brace that does real work rather than a cosmetic pair — a truncated payload whose prefix matches now fails too, where before it verified. Safe for any future additive field: a JSON object's own closing brace is always the last character, whatever type the last value has, which is pinned by two negative-space tests (a nested object and an array-valued last key must both still verify). Cost: +3 bytes, against the 1,873 the resolver fold already gave back. The alternative considered and rejected was dropping the literal `{` for a `?` glob. It balances too, but weakens the anchor from "must be an opening brace" to "must be any one character", and the anchor is the entire point. Two guards added so this cannot recur silently: - runtime-launcher-parity (F0) pins brace balance at the SNIPPET, so the next edit to that pattern fails on the file it broke instead of surfacing three files downstream in a test whose name mentions neither the launcher nor this issue. It also asserts depth never goes negative, since a `}` preceding its `{` nets to zero while being unbalanced at every prefix. - runtime-identity gains behavioral truncated-payload and trailing-garbage fixtures, so the added `}` is proven load-bearing rather than merely present. Verified: snippet 51/51 braces; new-project combined net depth 0; the seven other preamble-bearing files with nonzero depth are unchanged from merged next (their own prose, not the preamble, and not in any guard's scan set); all 112 inlined copies and the resolver reference re-synced byte-equal; sync:launcher idempotent on the second run. Refs #3841 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3841): backfill changeset PR number 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
|
8d8e9ef5eb |
fix(#3842): stage the emitted-drift-ack sweep so it does not conflict in-flight PRs (#3847)
* fix(#3842): stage the emitted-drift-ack sweep around open PRs The guard-no-ack-on-next sweep (#3078) deleted every all-spent fragment under tests/emitted-drift-acks/ unconditionally. When an open PR still modified the same fragment file, that delete became a modify/delete conflict on the PR's next merge attempt -- the exact shared-file conflict fragments were adopted (#2914) to eliminate, reintroduced by the sweep itself. The first real sweep hit three open, outside- contributor PRs simultaneously (#3330, #3774, #3648), each with the swept fragment as its only conflicting path. assertNoAllSpentFragments now accepts an optional openPrTouchedPaths set (or the sentinel 'unknown') and partitions all-spent fragments into "safe to sweep" and "held" -- a held fragment is reported informationally, never as a failure, and is swept once the touching PR merges or closes. fetchOpenPrTouchedAckPaths computes the touched set with a single `gh pr list --json number,files` call. The guard-next codepath is factored out of main() into the exported, dependency- injectable runGuardNext() so this wiring is unit-testable without a real, network-dependent `gh` binary. The new behavior is strictly opt-in via a --defer-to-open-prs flag, wired only from the guard-no-ack-on-next job in test.yml (which also gains pull-requests: read and a GH_TOKEN env for the `gh` call). Every pre-#3842 caller -- including every existing test -- is unaffected when the flag is omitted. CONTRIBUTING.md's "Why fragments, not one file (#2914)" section now documents the staged-sweep policy so it no longer reads as though fragments are unconditionally conflict-free once spent. Refs #3842 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3842): unify the failed-open-PR-check message to one greppable phrase The remote matrix (commit 4ec4527fb) failed two tests on the fail-closed path: assertNoAllSpentFragments's 'unknown' sentinel branch described the failure as "...could not be determined this run...", while runGuardNext's catch around fetchOpenPrTouchedAckPaths described the same condition as "open-PR check failed (<err>)". Both messages were genuinely present and informative (not missing or empty), but they used different wording for the same fail-closed condition, so there is no single string a human scanning CI output can search for to find out why nothing got swept. Unify both sites on "open-PR check unavailable" -- runGuardNext's line now reads "open-PR check unavailable — <err.message>", and assertNoAllSpentFragments's holdAll message now leads with "deferred (open-PR check unavailable): ...". This is a real fix to the fail-safe path's diagnosability, not a relaxed test assertion: the two now-failing tests already expected this exact phrase, and the fix makes the code say what the tests (correctly) expected instead of loosening them to match arbitrary prior wording. Refs #3842 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3842): backfill changeset PR number and retype to Fixed pr:0 backfilled to 3847. Retyped Changed -> Fixed: the change repairs broken sweep behaviour rather than adding any, and its contributor-facing documentation lives in CONTRIBUTING.md at the repo root, which lint-docs-required does not count. 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
|
de95c03f72 |
fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source (#3846)
* test(#3699): failing-first coverage for derived-key reporting and the case-D fallback * fix(#3699): report why a derived frontmatter key was not written, and repair a missing body source * fix(#3699): scope session-field writes to ## Session so an archived line cannot absorb the update * fix(#3699): resolve the session writer from body labels only, so a frontmatter key never writes the body * chore(#3699): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
36375513b9 |
feat(#3840): generate docs/FEATURES.md from per-feature fragments (#3845)
* feat(#3840): generate docs/FEATURES.md from per-feature fragments docs/FEATURES.md was hand-maintained, and every feature PR wrote into two shared mutable cells: the '### N.' heading whose integer was hand-allocated at authoring time, and the hand-maintained table of contents. Concurrent PRs all picked the same next integer, and two PRs adding differently numbered features still collided on the TOC. #3831 was renumbered 165 -> 166 -> 167 -> 168 across successive rebases, each collision also costing a full matrix verification run because the sha-keyed pass marker dies with the rebase. Mechanism: one fragment per feature at docs/features/<slug>.md carrying id/title/group (and an optional order) in frontmatter, consolidated by scripts/gen-features.cjs --write|--check into a marker-delimited region of docs/FEATURES.md that holds BOTH the TOC and every section body. Group headings and their order are derived too - a group sorts by its lowest-ordered member - so there is no shared registry to edit either; optional per-group prose lives in docs/features/_groups/<slug>.md. A contributor adds exactly one new file. Wired into regen:derived and lint:generated-sync alongside the eight existing generators, matching gen-adr-index.cjs's CLI shape and typed-REASON reporting. Migration froze all 168 existing numbers verbatim: identical section set, identical order, identical bodies. Two defects found in the tree are fixed inline rather than carried forward - the '## Related' block had been spliced into the middle of the document, orphaning §142's Reference line, and four inbound anchors were already broken on next (FEATURES.md#runtime-identity in two files, and #143-spec-phase-edge-completeness-probe off by one). Since the repo has no link checker, --check now validates every inbound FEATURES.md#anchor by resolved target, so that class cannot ship silently again; locale FEATURES.md files resolve elsewhere and stay out of scope. Refs #3840 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3840): carry upstream §69 delta into its fragment and harden the generator Review found section 69 missing '[--strict]' and REQ-STATE-05/06 versus origin/next. Root cause was a stale base, not extraction loss: those lines landed in |
||
|
Tom Boucher
|
394bf384be |
fix(#3696): report the last_activity invariant and make the verdict gateable with --strict (#3844)
* test(#3696): failing-first coverage for the last_activity invariant and --strict exit status * fix(#3696): report the last_activity invariant and make the verdict gateable with --strict * fix(#3696): agree with the real reader on last_activity, and stop reporting structure as truncation * chore(#3696): backfill changeset PR number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
6905726d9c |
chore(#3833): gate every PR compute lane behind a mergeability preflight (#3843)
* test(#3833): failing-first suite for the PR mergeability preflight * chore(#3833): gate every PR compute lane behind a mergeability preflight * fix(#3833): assert the preflight gate on parsed yaml and guard status-function if * fix(#3833): run stub-backed cli tests in-process to avoid a spawnsync deadlock * fix(#3833): fail the preflight open when its script is absent at the base sha --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
63abcface9 |
feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools (#3831)
* feat(#3146): resolve gsd_run so workflows cannot reach a foreign gsd-tools The predecessor package get-shit-done-cc publishes a colliding gsd-tools bin whose phases.clear DELETES where this package's ARCHIVES, and both print success-shaped output against a gitignored .planning/ -- which is how #3129 cost a user 43 phase directories with no error and nothing recoverable from git. The launcher's PATH branch now resolves gsd_run, published only by this package and self-locating via its own symlink chain to the sibling shim, instead of the colliding gsd-tools. A foreign handler becomes unreachable from PATH, and when no gsd_run is reachable the resolver fails closed rather than falling back -- that fallback was the vulnerability. This is smaller than the branch it replaces, which matters: the preamble is inlined into 113 shipped files and agents/gsd-verifier.md sits 2 bytes under a red-line size cap. unset -f gsd_run leads the preamble so a re-source is idempotent. Without it, command -v finds the shell function, returns a bare name, and the resolver falls through to an exit 1 that kills a sourced caller's shell. Adds gsd-tools runtime-identity, a manual diagnostic reporting this runtime's package coordinates over the baked package-identity (#498) and readHostVersion, with a strict total classifier: only a JSON object with an exact packageName verifies, since JSON.parse admits 0/"str"/[]/null/true. An inlined identity assertion was built and reviewed first, then withdrawn -- it breaks five frozen size ceilings and no assertion fits in 2 bytes. Closes #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3146): stop sync:launcher relocating a deliberate preamble placement Pre-existing defect, surfaced by this PR because sync is a no-op unless the snippet content actually changes. transformFile inserts the preamble into the first block that CALLS gsd_run, but gsd-core/workflows/explore.md deliberately places it in a bootstrap-only block that DEFINES gsd_run without calling it -- its own comment explains why: declining the research offer must not leave Step 5's commit call unbootstrapped. Stripping empties that block of calls, so the preamble migrated forward and broke the define-before-use invariant tests/explore-command.test.cjs pins. Reproduced on a pristine origin/next checkout with the base snippet and base file, so this was not introduced here. The insertion target now honours a block that already carried the preamble, falling back to the first calling block for files that have none yet. Adds a behavioral regression test over a two-block fixture. Also updates three runtime-launcher-parity tests that pinned the removed PATH fallback to gsd-tools. Their intent is preserved -- the PATH stub is renamed gsd_run so it is reachable by the new resolver, and the RUNTIME_DIR-wins test still asserts the stub is never invoked. Fixture shebangs move to an absolute /bin/sh, because the fixture PATH is deliberately restricted and #!/usr/bin/env sh could not resolve. Refs #3146 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3146): backfill changeset PR number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3146): document the FEATURES.md section-numbering practice The monotonically increasing section number in docs/FEATURES.md is the most frequent merge-conflict source in this repo, and it has TWO conflict cells, not one: the ### N. heading and the hand-maintained table of contents. Two PRs adding differently numbered features still collide on the TOC, so renumbering alone does not make a branch safe. This branch alone was renumbered 165 -> 166 -> 167 -> 168 across successive rebases. Adds a CONTRIBUTING section stating the practice: allocate the number last, never pre-emptively renumber, take max+1 after a rebase and update the TOC in the same commit, and never renumber someone else's section. Fork contributors are told explicitly they may leave the number to a maintainer at merge rather than chasing the counter. Agents are told to lease the allocation and to include the file in their published touched set. Records the durable fix as planned rather than pretending it exists: FEATURES.md should be generated from per-feature fragments the way CHANGELOG.md is generated from .changeset/, and the way tests/emitted-drift-acks/ works (#2914). Also renumbers this branch's own section to 168, leaving 167 to the PR already in flight. Refs #3146 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
|
aaf47c5fc2 |
fix(#3691): let every reviewer lane take a prompt cap, and make the documented global resolve (#3832)
* test(#3691): failing-first coverage for the reviewer prompt budget No prompt cap can reach any CLI reviewer lane, by any configuration. Two independent defects compound: all nine `transport: spawn` lanes declare `promptBudgetKey: null`, so `budgetFor` returns on its first line; and the documented global `review.max_prompt_tokens` is advertised in the schema manifest but declared nowhere, so the resolver never materializes it and `budgetFor`'s fallback is dead code. Adds to tests/reviewer-config-federation.test.cjs, which already owns the per-reviewer budget config-set/config-get idiom: - a CLI lane inherits the global cap (RED: reports null) - an http lane with the -1 sentinel inherits the global cap (RED: reports null) - the resolved review surface carries max_prompt_tokens at all (RED: absent) - per-lane overrides the global on a CLI lane - the sentinel boundary: -1 inherits, 0 means do-not-trim and must NOT read as unset, 1 is the smallest real budget — the regression budgetFor's own comment warns about - anti-tightening pins that must stay green: an empty config leaves every lane null, the three existing budgeted lanes are unchanged, and config-set still rejects a per-reviewer key naming something that is not a declared lane - a fast-check property over the resolution contract itself, with -1, 0 and non-finite inputs generated explicitly rather than left to chance Every row was reproduced by hand against the real CLI before being written, so the RED/GREEN split is observed rather than predicted. Refs #3691 * fix(#3691): let every reviewer lane take a prompt cap, and make the global resolve No prompt cap could reach any CLI reviewer lane, by any configuration. Two independent defects compounded. The nine spawn-transport lanes — claude, coderabbit, antigravity, cursor, gemini, codex, kimi-code, opencode, qwen — declared `promptBudgetKey: null`, so `budgetFor` returned on its first line and `review-lane plan` reported `promptBudget: null` no matter what was configured. Each now declares `review.max_prompt_tokens_per_reviewer.<slug>` with the same `-1`-is-unset sentinel the three local-server lanes already use. Separately, the central `review.max_prompt_tokens` was listed in the schema manifest's validKeys and documented as a supported setting, but declared nowhere — the resolved surface is built from capability declarations plus the defaults manifest, and neither carried it. `configGet` returned undefined and `budgetFor`'s documented fallback was dead code. It is now declared with a `null` default, exactly as docs/CONFIGURATION.md already specified, so the default behavior is unchanged: nothing configured means nothing trims. Two things the diagnosis had not predicted, found and fixed while implementing: - `REVIEWER_LANES` in src/review-lane-descriptor.cts is a second, hardcoded registration site that `mergeReviewerLanes` prefers over the capability registry on a slug collision. Editing only the capability files left every CLI lane still null. Both sites now agree. - The generated `gsd-core/bin/lib/capability-registry.cjs` was stale and masked the capability edits; regenerated with `npm run gen:capability-registry` rather than hand-edited. docs/CONFIGURATION.md said "Only lanes that declare a budget key accept one — today ollama, lm_studio and llama_cpp". That is false as of this change and is corrected rather than left to rot. The trim-versus-refuse question the issue raises is deliberately not taken up here: the refusal path already exists for the case that matters — a reviewer whose minimum set exceeds its budget is skipped rather than sent a misleading prompt — and trimming above that floor is the documented, shipped design of the feature. Changing it would alter behavior for the three lanes that already work, which is not what the issue asks for. Fixes #3691 * fix(#3691): document the new global and narrow an invariant this change obsoleted The full suite surfaced two consequences of giving every CLI lane a budget key. `review.max_prompt_tokens` entered CONFIG_DEFAULTS without a matching entry in the planning-config reference, which config-field-docs guards. Documented, including the sentinel semantics a reader needs: a per-lane value overrides the global, `-1` means unset and inherits it, and `0` means "do not trim that lane" and is not unset. The #2797 federation guard asserted that "a lane with no model flag and no host owns no config keys". That held only because budget keys existed solely on the three local-server lanes, all of which have hosts. A lane can now legitimately own a config key for a third reason, so qwen tripped it. The assertion is narrowed rather than weakened: such a lane must still own no model key and no host key, and may own at most its own `review.max_prompt_tokens_per_reviewer.<slug>` — never another lane's. That is strictly more specific in the dimensions that still matter. Proven to still bite: hypothetically giving qwen a `review.models.qwen` key fails it with `model/host: review.models.qwen`. The name and comment cite #3691 for why the premise changed, so a reader sees a deliberate narrowing, not erosion. Checked the sibling assertions in that describe block; the other three do not rest on the obsolete premise and are untouched. Refs #3691 * fix(#3685): port the write-flag content-change contract to its three sibling sites #3685 fixed `phase complete`'s `roadmap_updated` / `state_updated`, which reported `fs.existsSync(path)` rather than whether the transaction wrote anything. Three sibling sites carried the identical defect and are ported here. - `cmdPhaseRemove` reported `roadmap_updated: true`, hardcoded. `updateRoadmapAfterPhaseRemoval` now returns whether the content changed and the flag reports it. #2640/#2974 already fixed `state_updated` at this same call site and left this one behind, so the correct shape was adjacent. - `cmdMilestoneComplete` reported `state_updated: fs.existsSync(statePath)` — byte-identical to #3685's bug in a different command. - `cmdMilestoneComplete` reported `milestones_updated: true`, hardcoded, never consulting the MILESTONES.md write. `gsd-core/workflows/remove-phase.md:100` extracts `roadmap_updated` for display and never branches on it, so the flip from always-true to content-based changes no workflow behavior. Verified by reading the step, not assumed. One trap found while implementing: the obvious in-memory `finalContent !== originalStateContent` comparison — copying `cmdPhaseComplete`'s shipped shape verbatim — gives a FALSE POSITIVE for milestone completion. `platformWriteSync` normalizes Markdown at write time, and the milestone-closure transform regenerates `## Current Position` fresh on every call, so its pre-normalize output always differs from the already-normalized file on disk even when the persisted bytes are identical. The comparison is therefore made against the post-write on-disk content. `cmdPhaseComplete`'s own comparisons are left untouched — their repeat-no-op tests pass, so they are not exposed to this artifact. `milestones_updated` has no reachable no-op: the MILESTONES.md write unconditionally appends an entry every call. Only the true direction is pinned, documented inline rather than faked with a passing test. Refs #3685 * fix(#3685): compare write-flag content through the writer's own normalizer An independent reviewer disproved a claim made while porting #3685's contract to its sibling sites: that `cmdPhaseComplete`'s comparisons were not exposed to the Markdown-normalization artifact already diagnosed in `cmdMilestoneComplete`. `platformWriteSync` normalizes on write — CRLF stripped, blank-line runs collapsed, a blank line inserted after a heading, a single trailing newline enforced. Every flag that compares the PRE-normalization in-memory string against the on-disk pre-image can therefore report a change when the persisted bytes are identical. `cmdMilestoneComplete` had been worked around by re-reading the file after the write; the other sites compared raw strings. All of them now go through one exported seam, `contentChangedAfterNormalize(filePath, before, after)`, which normalizes both sides exactly as the writer does. That removes the extra disk read the milestone workaround needed, and makes the sites agree by construction rather than by four independent implementations of one rule — the divergence the repo names as an anti-pattern. Reachability, stated precisely rather than uniformly: the seam is load-bearing at `cmdPhaseComplete`'s `roadmapUpdated`, `requirementsUpdated` and `stateUpdated`, where section-rewrite logic genuinely regenerates content into a different-but-normalization-equivalent shape. At `updateRoadmapAfterPhaseRemoval` it is defense-in-depth: the no-match branch never reassigns `content`, so the raw comparison was already correct there. The first analysis claimed the reverse; this is the corrected finding. Also fixes an unsound test premise the remote suite caught. The byte-identity precondition in `roadmap_updated is false when ROADMAP.md comes out byte-identical` asserted against a hand-authored, un-normalized fixture — so the very first write reformatted it and the file could not come back identical. The fixture is now written already-normalized, so the assertion compares a normalized pre-image against a normalized post-image and still fails if the flag regresses to a hardcoded `true`. Not platform-specific; it reproduces on macOS too, and the earlier local check simply never exercised it. The sibling true-direction and milestone tests were checked for the same premise and do not share it — they assert `notEqual`, or compare two post-write states produced through the same normalizing seam. Refs #3685 * chore(changeset): backfill PR number for #3691 fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
c933184b97 |
enhance(#3172): require a stated failing direction for every automated acceptance command (#3825)
* test(#3172): failing-first suite for the stated failing-direction probe Pins the <fails_when> pairing walk, placeholder denylist, MISSING sentinel exemption, degraded-read contract, CLI arm and the plan-authoring contract text. RED by construction: the module exports it requires do not exist yet. Executed on the remote runner. * feat(#3172): require a stated failing direction for every automated acceptance command Every runnable <automated> command now carries a <fails_when> sibling naming what output constitutes failure. A command with no expressible failure mode is not an acceptance test: it reads as rigour and is not falsifiable. - verify-command-grounding gains a failing-direction probe sharing the existing <automated> grammar, MISSING sentinel and walk guard rather than copying them - gsd-tools check verify-failure-directions <N> backs it; plan-phase dispatches it and hands the JSON to gsd-plan-checker check 8f - Dimension 8 detail extracted to references to stay under the agent size cap Verified on the remote runner. * fix(#3172): close four review findings in the failing-direction probe - MISSING_SENTINEL_RE matched an env-var assignment prefix (MISSING=1 cmd), so a real command was exempted from the new blocking gate. Tightened the SHARED constant rather than adding a second copy. - Both token regexes scanned to EOF on unclosed openers (O(n^2), 1562ms at 40k). Bodies are now non-crossing; 1ms, byte-identical on well-formed input. The pre-existing AUTOMATED_BLOCK_RE carried the same defect and is fixed here too. - probePhaseFailingDirections reported status 'ok' when one plan was unreadable, conflating 'could not look' with 'nothing to report'. - Extracted the phase-resolution block both check arms had copied verbatim. Also corrects a docs/AGENTS.md dimension list stale since #2401. Verified on the remote runner. * fix(#3172): project the planner rule onto the spawn contract, settle emitted bookkeeping The remote runner refuted the planner-side edit. agents/gsd-planner.md is frozen under a 49152-LF-char cap asserted by four suites and sat at 49,146 — six chars of headroom — so the +537 of authoring rule blew it. #3297/#3645 already settled where such a rule goes: the planner spawn contract in plan-phase.md, beside <tracked_source_paths>. The agent file is reverted to origin/next verbatim. - plan-phase.md gains <failing_direction_contract>; tests row 30 now asserts the contract there and row 30b guards the freeze in both directions - plan-phase.md growth acknowledged by APPENDING to the 3409 fragment, per the precedent that two ack sources may never name the same path - install-tree fixtures regenerated for the three new reference files Verified on the remote runner. * chore(#3172): backfill PR number into the changeset fragment pr:0 -> pr:3825 now that the PR exists. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7a41248c4f |
fix(#3685): report phase-complete write flags from the transaction, not the filesystem (#3826)
* test(#3685): failing-first regression coverage for phase-complete write flags `phase complete` reports `roadmap_updated`/`state_updated` from `fs.existsSync(path)`, so both read `true` whenever the file merely exists — including when the transaction wrote nothing. Add the regression tests that prove it, plus the negative-space and true-direction pins, before the fix. New in tests/phase.test.cjs: - roadmap_updated is false when the transaction rewrites nothing (FAILS today) - state_updated is false when the transaction rewrites nothing, and stays false on a third consecutive run (FAILS today) - both flags are true when the transaction genuinely rewrites (pins the true direction so the fix cannot be tightened into always-false) - each flag stays false when its file is absent The STATE.md cases pin the clock via GSD_TEST_MODE + GSD_NOW_MS (src/clock.cts:43-70) because syncStateFrontmatter stamps a millisecond-resolution `last_updated:` on every write, which would otherwise make the no-op unobservable. Also strengthens four pre-existing `=== true` assertions on these fields that passed vacuously: each now pairs the flag assertion with a content-changed assertion against a pre-call snapshot, so the `true` is earned. Refs #3685 * fix(#3685): report phase-complete write flags from the transaction, not the filesystem `phase complete` computed `roadmap_updated` and `state_updated` as `fs.existsSync(path)`, so both read `true` for any project that had the file at all — including a run that rewrote nothing. The flags are the only signal a caller has that the rollup landed, so a no-op was indistinguishable from a successful write and a stale ROADMAP went unnoticed until something downstream read wrong numbers. Both flags now reflect whether that file's content actually changed in the transaction, computed at the existing `writes.push({filePath, before, after})` sites — the same contract `requirements_updated` has honored since #2316-3, and the same correction #2640/#2974 already applied to `phase remove`. Nothing about what gets written changes; only what gets reported. Fixes #3685 * chore(changeset): backfill PR number for #3685 fragment --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
596540f864 |
feat(#3227): publish machine-readable state contract at step boundaries (#3824)
* feat(#3227): publish machine-readable state contract at step boundaries Adds src/state-contract.cts, a best-effort publisher that writes .planning/state.json (contract 1.0.0) at 11 step-boundary commands, so external tools read a versioned contract instead of parsing STATE.md and ROADMAP.md heuristically. Composes existing owners rather than re-deriving: phase rows come from a new locateProgressTable extracted from deriveProgressFromRoadmap (so the snapshot can never disagree with GSD's own progress counters), milestone identity from getMilestoneInfo, and next from classifyProject. Owners are required lazily to avoid the state -> state-contract -> smart-entry -> state require cycle. Also fixes a pre-existing defect in scripts/lint-test-file-count.cjs (maintainer-approved as a second concern): testEffectivePrefix never stripped the suite qualifier, so 65 dotted test files counted against no module and 9 mis-bucketed into a shorter one. Allowlist re-baselined for the 74 files the gate can now see. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3227): backfill PR number into the changeset fragment pr:0 -> pr:3824 now that the PR exists. Doc-only. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3227): shape hostile-name fixtures away from the scan corpus The two hostile-input fixtures used a literal phrase from scripts/prompt-injection-scan.sh's corpus, so CI's Security Scan redded on this file. These tests assert that an arbitrary phase name round-trips into state.json as inert data -- the property holds for any string, so the injection flavor is illustrative, not load-bearing. Reshaped to a hyphenated fake instruction tag, which stays hostile-looking while matching none of the scanner's patterns. Allowlisting the file was rejected: that mechanism is for suites whose subject IS injection defense, and it would blind the scanner to this whole file permanently. See DEFECT.PROMPT-INJECTION-SCAN-COLLISION. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3227): ratchet the state-contract mutation floor to its measured score The module was registered at minScore 50, the ratchet's minimum permitted floor for a newly-registered module whose score had not been measured. This PR's own Stryker shard measured 66.25% (run 32769289750, job 97565813640), so the floor moves to floor(measured) - 1 = 65, per the rule the registry documents. 66.25 is below TARGET_MUTATION_SCORE (80), so this stays a ratchet candidate: raise as the tests improve, never lower. 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
|
fb9823e1e1 |
fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON (#3828)
* test(#3689): failing-first coverage for the ledger table/JSON agreement guard `.planning/WINDOWS.md` renders its markdown table from the fenced JSON that is its source of truth, but nothing checks the two still agree before a write overwrites the table. `windows append` / `waive` / `fixed` therefore discard a drifted cell silently, and erase a table-only row entirely, both at exit 0. Adds to tests/broken-windows.test.cjs: - five refusal cases that fail today, covering all three write commands, a drifted cell, a table-only row, and drift on a non-first row; each asserts the typed reason via GSD_JSON_ERRORS and that the file is byte-identical after the refusal, so a guard that refuses only after writing cannot pass - six anti-tightening pins that must stay green: an agreeing ledger, the first-write ENOENT path, #2893 trailing-prose preservation, #3657 3-backtick fence tolerance, escaped pipes and backslashes in a description, and the zero-entry placeholder table - a fast-check property pinning the round trip the guard depends on — extractTableRegion(renderLedger(l)) === renderTable(l.entries) — because a false refusal on a clean ledger would be worse than the bug Fixtures are built by running the real CLI and then perturbing only the table, so frontmatter and JSON stay consistent and the pre-existing counts cross-check still passes; a hand-written ledger would pass these for the wrong reason. Refs #3689 * fix(#3689): refuse a ledger write when the rendered table disagrees with its JSON `.planning/WINDOWS.md` renders its markdown table from the fenced JSON that is its source of truth, and `writeLedgerAtomic` regenerated that table on every `windows append` / `waive` / `fixed` without ever checking the two still agreed. A hand-edited cell was silently reverted; a row that existed only in the table vanished entirely. Both at exit 0, with nothing on stdout to say so. The write seam now compares the on-disk table against `renderTable(<entries parsed from the on-disk JSON>)` before regenerating anything, and refuses with a typed `windows_ledger_table_drift` error naming the drifted row ids and the remedy. Because the check sits at the single write seam, all three commands inherit it, and the file is left byte-identical on refusal. Deliberately not enforced in `parseLedger`: hardening the read would break `windows status` and the ship gate on exactly the ledgers an operator needs to inspect to diagnose the drift. Two hazards handled explicitly, both discovered in review of the first draft: - The pre-image read now distinguishes ENOENT from every other errno, per the #1950-H2 fail-closed-on-unreadable invariant `readLedgerOrNull` already honors. A bare catch would have let an unreadable pre-image skip the guard and write anyway. - Both the entries baseline and the table extraction pass the pre-image's own frontmatter `total_count` to `locateJsonBlock`. Without that hint the no-expectation fallback binds to the LATEST fenced JSON array in the file, which is the operator's prose block whenever that prose contains one — the exact case #2893 exists for — refusing every write on a ledger that never drifted. A regression test covers it. Also extends the CONTEXT.md Broken Windows Ledger glossary entry: the table is a third projection of the same source, cross-checked at the write seam, and the frozen REASON enum gains WINDOWS_LEDGER_TABLE_DRIFT. Fixes #3689 * fix(#3689): bind prose preservation to the pre-image's own ledger block Found while reviewing the table drift guard: the #2893 trailing-prose preservation in `writeLedgerAtomic` passed `ledger.total_count` — the POST-mutation count — as the disambiguation hint for a lookup over the PRE-image. On an append the pre-image holds N entries while the hint says N+1, so the hint can never match and `locateJsonBlock` falls through to its last-array-shaped-span fallback. When the operator's trailing prose itself contains a fenced JSON array — the ordinary case #2893 was written to protect — that prose block wins the fallback. The preserved region is then computed from the prose fence rather than the ledger fence, and everything between them, including the operator's own text above the array, is silently dropped on the next write. Reproduced against the real CLI: a prose block reading "Operator notes above the array, IMPORTANT DO NOT LOSE THIS TEXT." plus a fenced 3-element array came back empty after one `windows append`. Both the prose lookup and the drift guard now share one pre-image-derived `preImageExpectedTotal`, taken from the pre-image's own frontmatter, so they bind to the same and correct block. The existing trailing-prose regression test is strengthened to assert the prose survives byte-for-byte rather than merely that the command exited 0 — asserting only the exit code is why this was invisible. Refs #3689 * fix(#3689): anchor table extraction on the header row, not a line-prefix scan Independent review found the drift guard could brick a ledger nobody had hand-edited. `validateDescription` accepts a description containing a raw newline, and `renderTable`'s cell escaping covers backslash and pipe but not newlines — so such a description renders a row that physically spans two file lines, the second of which does not begin with `|`. `extractTableRegion` bounded the table by walking backward over the contiguous run of `|`-prefixed lines, so it stopped at that split. In the common case where the row's tail is the last line before the fence it returned null, and every subsequent append/waive/fixed was refused with "table region could not be located" — permanently, with no CLI recovery path, on a ledger that never drifted. A false refusal is worse than the bug this guard exists to fix. The region is now anchored on the header row `renderTable` always emits, running from its last line-start occurrence to the end of the pre-fence text. The boundary is the fence rather than a line prefix, so a multi-line row is captured whole, re-renders byte-identically, and compares equal. The header literal is hoisted to one constant both `renderTable` branches and the extractor share, so the two surfaces cannot drift apart. Deliberately unchanged: `cell()` and `validateDescription`. The cosmetic corruption a newline causes in the rendered table is pre-existing, and either escaping it or rejecting the input would change what existing ledgers render to or what input is accepted. Also closes a coverage gap the standards review raised: the non-ENOENT pre-image read branch — the one that stops an unreadable file from bypassing the guard — now has a behavioral test that injects EACCES by monkeypatching `fs.readFileSync` for that one path and restoring it in a `finally`, never by `chmod 0o000` (root ignores mode bits, so that would pass with zero coverage). The #3689 property generator no longer strips newlines out of descriptions, which is why this was invisible to it. Refs #3689 * chore(changeset): backfill PR number for #3689 fragment * chore(changeset): backfill PR number for #3689 fragment * fix(#3689): terminate the header scan when the match sits at index 0 `extractTableRegion`'s backward search for the table header could loop forever. On a rejected match at index 0 it set `searchFrom = idx - 1`, i.e. `-1`; `String.prototype.lastIndexOf` clamps its position argument into `[0, length]`, so the next iteration searched from 0, found the same match, rejected it identically, and set `-1` again. The loop made no progress. Reachable only through the exported `extractTableRegion` — `writeLedgerAtomic` reaches it after `parseFrontmatterStrict` has already succeeded, so the candidate region begins with the `---` frontmatter fence and a match at index 0 is impossible. Latent rather than live, but an exported `for(;;)` that can fail to advance is not something to ship. Confirmed by running the pre-fix compiled function on `TABLE_HEADER_LINE + 'X\n' + <a valid json fence>` as a backgrounded child: it was still alive after five seconds having printed nothing, and had to be killed. Post-fix the same input returns `null` promptly — correct, since the sole header occurrence fails the end-of-line test and no valid header exists. A regression here would stall the suite rather than fail it, so the new test also asserts the returned value rather than relying on termination alone. No wall-clock assertion is involved. Refs #3689 * test(#3034): publish the lane trace before the done-file that releases dependents `preservesSelectionOrderParallelDespiteCompletionOrder` forces a reverse completion order with a dependency chain rather than sleeps: each stub lane waits on `done-<dep>` before finishing. It then ended with touch "$RUN_DIR/done-$slug" echo "end:$slug" >> "$TRACE" Those are two unsynchronized operations in separate shell processes. A dependent's `wait_for_file` unblocks the instant the upstream's `touch` lands, but the upstream's own `echo` has not necessarily run — so if the upstream is descheduled between the two, the dependent can run its whole body and append its `end:` line first. The done-file was published before the state it signals. Observed on the remote runner as `[end:claude, end:codex, end:gemini]` where selection order demands `[end:claude, end:gemini, end:codex]`. The failure was in the fixture's own self-check, before it reached the assertion #3034 exists to make. Not a flake and not a wall-clock margin: this branch passed the full suite twice at 14f494644 and 90c5d7a03, and the only delta in the failing run was one added test in tests/broken-windows.test.cjs — an unrelated module. Adding load elsewhere in the suite was enough to invert it, which is what a real race does. Swapping the pair establishes a genuine happens-before: anything a dependent can observe is written before the file that releases it. A comment records why, so the order is not tidied back. The production path is unaffected and was independently confirmed correct — `invoke_reviewers` joins every lane with `wait`, then aggregates by iterating DISPATCH_SLUGS in selection order, reading per-slug result files. It consumes no completion-order signal at all. Refs #3034 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
a2387a0545 |
feat(#3034): add opt-in parallel reviewer lanes (#3822)
* test(#3034): failing-first coverage for opt-in parallel reviewer lanes Executes the real invoke_reviewers dispatch block from review.md against a stubbed gsd_run seam rather than pattern-matching the workflow text, so the two properties that actually carry risk are observable: that every lane is joined before aggregation, and that concurrent lanes cannot tear a line in gsd-review-lane-results.jsonl. Concurrency is proven by a barrier fixture, not by elapsed time -- each stub lane blocks until all lanes have checked in, which can only complete if they overlap. Red against the current sequential dispatch, by design. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#3034): add opt-in parallel reviewer lanes Reviewer lanes within one review pass inspect the same immutable plan snapshot and have no dependency on one another, but were dispatched strictly one at a time, so a multi-reviewer pass cost roughly the sum of its lanes. The serialization is a deliberate protection against provider rate limits, so it stays the default; review.parallel_lanes opts a project out of it. The loop body is hoisted into run_review_lane so the sequential and concurrent paths share one body -- two hand-synced dispatch bodies is the divergence class ADR-2782 spent a phase deleting. Each lane writes a slug-scoped result file, concatenated in selection order after the join: concurrent O_APPEND is atomic only below PIPE_BUF, and write_reviews parses that JSONL to render the models:/model_sources: frontmatter, so a torn line is a broken REVIEWS.md rather than a cosmetic log defect. Aggregating in selection order also keeps the artifact byte-identical between the two paths. The guard is strict equality on "true" and falls back to sequential when config-get fails -- the opposite polarity from the commit_docs guard, because failing open here fires the very requests the default prevents. Also corrects docs/COMMANDS.md and its four locale mirrors, which described --all as running every configured reviewer in parallel when dispatch was in fact sequential. Closes #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3034): de-duplicate dispatch slugs and scope lane locals Review finding (Standards axis): a slug repeated in SELECTED_REVIEWERS would put two concurrent background jobs on the same > -truncated per-lane result file. The shared-append form this replaced could not corrupt itself that way, so de-duplicating is what keeps the concurrent path no worse than the sequential one. Selection de-dupes today -- the roster is a Set and review.default_reviewers normalizes lowercase-unique -- but reachability analysis is not a contract, which is the same reason the roster derivation itself is guarded. Splitting once into DISPATCH_SLUGS also removes the duplicated tr-split the same review flagged: the dispatch and aggregation loops now share one list, which is what guarantees they walk the same slugs in the same order. A plain string accumulator rather than an array, because zsh and bash disagree on array indexing and this block runs under both. Also scopes run_review_lane's locals. Not a live fix -- each dispatched call already forks its own subshell -- but it makes the isolation a property of the function rather than of the dispatch mechanism happening to fork. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#3034): acknowledge review.md growth, drop spent 2295 ack The differential attribution gate reported review.md growing 4173 bytes (30712 -> 34885) with no live acknowledgment. Adds the per-PR fragment it asks for, naming only the one path it reported. Deleting tests/emitted-drift-acks/2295-resolved-model.json is required, not opportunistic. That fragment declared review.md and nothing else, and its ripple is already absorbed into the base, so it is spent -- it can no longer clear anything, which is why the gate still reported review.md as unacknowledged. It could not simply be left alone either: two ack sources may never name the same path, so it blocked this PR's fragment outright. CONTRIBUTING is explicit that a fragment whose last entry is removed gets deleted with it, because an empty fragment signals nothing while its presence reads as a live alarm. Refs #3034 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3034): backfill changeset PR number Refs #3034 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
|
a84f756303 |
fix(#3078): sweep all-spent ack fragments on next, name the collision remedy (#3823)
* fix(#3078): sweep all-spent ack fragments on next, name the collision remedy
`guard-no-ack-on-next` only ever watched the legacy tests/emitted-drift-ack.json.
#2914 exempted the fragment directory on the premise that a persisting fragment
"cannot conflict with any other PR". Fragments do not share a FILE, but they do
share a PATH KEY SPACE, and a path claimed by two sources is a hard failure in
the same script -- so a fully-spent fragment on next owns keys it can no longer
gate, and the next PR to grow one of those paths can declare it neither there
(spent) nor in its own fragment (duplicate). Measured at the sweep: 45 fragments
owning 403 paths, up from 13/272 at triage 19 days earlier.
- `assertNoAllSpentFragments` fails a fragment only when EVERY surviving entry is
spent against the copy at HEAD^, so a partially spent fragment -- and the
re-arm-by-appending route #2639/#2993 ship on -- keeps working.
- `ackProse` duplicates the gate's zero-width/whitespace stripping across the
scripts-ship/tests-do-not line, bounded by a prose-parity test.
- The guard job's checkout takes fetch-depth: 2; at depth 1 HEAD^ is absent and
every fragment reads as brand-new, i.e. the guard passes vacuously.
- The duplicate-ack error now names both resolutions, since the guard is
post-merge by design and cannot stop the colliding PR.
- All 45 spent fragments deleted, 0000-legacy-migration.json included, and the
three tests that pinned its permanence corrected.
Verification is the remote runner (gsd-test), not a local suite.
Closes #3078
* fix(#3078): make the prose-parity test two-sided, cover the git seam, base on the pre-push tip
Three review findings, all fixed:
- The parity test was a tautology: it checked ACK_INVISIBLE against a
hardcoded list matching its own definition, never against the gate. The
gate's INVISIBLE and its reason normalizer (hoisted out of diffEmitted as
normalizeAckReason) are now exported for that sole purpose, and the test
sweeps 0x00-0xFFFF against both surfaces. Mutation-checked: adding a
codepoint to one side and not the other now fails.
- resolveBaseRef, readFragmentAtRef and assertUsableBaseRef had zero direct
coverage -- the tests reimplemented the git reads in a local helper, so the
ls-tree-vs-show discrimination, the root-commit fallback and the
option-injection guard were never executed. All are exported and tested
against real temp repositories now, plus an end-to-end --base-ref subprocess.
- HEAD^ is not 'the state of next before this push'. The default branch allows
REBASE merges, so one push can carry N commits, and a 2-commit rebase-merge
whose first commit adds a fragment would be told to git rm it on the very
push that introduced it. CI now passes github.event.before via --base-ref and
fetches it explicitly; HEAD^ remains only the local fallback.
Also adds the safe.directory guard every other git call in this repo carries
(#2767), and stops naming the deleted migration fragment by filename in
CONTEXT.md, which tripped lint-removed-but-needed.
Refs #3078
* fix(#3078): keep the fragment directory alive after the sweep empties it
Sweeping every fragment leaves the directory untracked, and check-glossary-refs
then fails: CONTEXT.md references tests/emitted-drift-acks, which no longer
exists. The empty directory IS the intended steady state, so it has to survive
its own remedy.
Adds tests/emitted-drift-acks/README.md documenting the create/use/delete
lifecycle where a contributor actually meets it, matching the existing
tests/qa/smell-acks/README.md precedent. Every reader filters on .json, so the
README is invisible to the gate.
Also sweeps #3809's ack fragment, which the rebase onto origin/next brought in
and the new guard immediately reported as all-spent -- its own remedy applied.
Refs #3078
* fix(#3078): guard the added tests' git calls, drop a second fragment-existence pin
Both defects surfaced by the remote runner (linux-node24, 4/37445 failed).
- The new --base-ref E2E test ran `git rev-parse HEAD` against the checkout
without the #2767 safe.directory guard. The runner mounts the repo at a path
owned by another uid, so git refused every operation there with 'detected
dubious ownership'. Every git call the new tests make now names its own
specific directory as safe, via one local helper, mirroring safeDirArgs in
helpers/emitted-runtime.cjs.
- tests/agent-tracked-source-rule.test.cjs pinned the existence and contents of
the 3645 and 3409 ack fragments. That is a merged PR's paperwork, not live
behavior: once the growth is in next's baseline the acks are spent and this
PR's guard sweeps them. The third assertion pinned the hand-appended
workaround for the exact collision #3078 removes. Deleted; #3645's real
protection is the two behavioral tests above it, untouched.
Also restores #3809's ack fragment, which merged one commit before this branch.
Deleting an ack in the same window as its introducing PR races any consumer
whose baseline predates it -- the runner's container proved it, resolving
origin/next to
|
||
|
Tom Boucher
|
8442d984b9 |
fix(#3809): route runtime-loaded markdown through the gsd_run launcher (#3815)
* test(#3809): generalize dead-ref guard into a rule table (failing first)
The #2020 guard hardcoded `sdk/(src|dist|handlers)/` — the three dead paths
that had caused that storm. That proved those three paths were gone and said
nothing about the class, so #3809 reproduced the identical Windows find.exe
storm under a different token and the guard could not see it.
Replaces the single regex with a rule table over the same runtime-loaded
markdown surface, adds `commands/` to the scan set (previously uncovered),
and adds rule B: the runtime shim filename must never appear in command
position, because it is not a PATH command and an agent that meets it falls
back to locating the file.
Rule B's matcher is deliberately lenient — the launcher's own resolver
assignment, `node <path>/<shim>` calls, bare paths, and prose that names the
file all stay unflagged, each pinned by a negative-space row.
This commit is expected to FAIL: 50 offenders across 23 files remain in the
tree. The remediation lands next.
Refs #3809
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3809): route every workflow call through the gsd_run launcher
50 places across 23 runtime-loaded workflow, agent, reference, and command
files instructed the agent to run the runtime shim by filename. That filename
is not on PATH under any name -- package.json ships gsd-core, gsd-tools,
gsd_run and gsd-mcp-server -- so the call exited 127, the file-shaped token
sent the agent looking for the file, and on Git Bash for Windows the resulting
`find /` walked the entire drive (7268 CPU-seconds in the report) until
somebody killed it by hand.
CONTEXT.md -> Runtime Launcher Module already makes gsd_run the single entry
point: "Canonical space-safe shell preamble (`gsd_run`) used by every workflow
bash block to invoke the GSD runtime CLI." These sites predate that rule --
they trace to
|
||
|
Tom Boucher
|
8ed105c8a4 |
fix(#3684): resume verified-unmarked phases at update_roadmap (#3814)
* test(#3684): failing-first rows for the verified-unmarked resume * fix(#3684): resume verified-unmarked phases at update_roadmap * test(#3684): heading-shaped roadmap fixture, plain phase.complete calls * fix(#3684): fit under the pre-phase-6 margin, fix pins and verify call * fix(#3684): padding-normalize the marked-complete join, assert STATE idempotency * test(#3684): anchor fixes, node jq mirror, characterized STATE delta * chore(#3684): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4b84be1da4 |
fix(#3683): wire gated learnings extraction into completion, align copy path (#3810)
* test(#3683): failing-first rows for learnings source resolution and wiring pins * fix(#3683): wire gated learnings extraction into completion, align copy path * test(#3683): register the learnings suite in the docs-guard lane, drop unverified markers * fix(#3683): close review findings — per-item parsing, readdir guards, docs paths * fix(#3683): route phase enumeration through the locator seam, fix assertion targets * fix(#3683): merge execute-phase ack into the 3003 fragment, fix fidelity targets * fix(#3663): replace the spent execute-phase ack entry with the 3683 re-arm * chore(#3683): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
31fcb833ec |
fix(#3679): gate pr-branch verify on planning-tree deletions (#3803)
* test(#3679): failing-first rows pinning planning preservation and the verify deletion gate * fix(#3679): gate pr-branch verify on planning-tree deletions * test(#3679): extract hashes via rev-parse and de-vacuate the pure-code pin * fix(#3679): close review findings — merged ack, pinned prose gate * fix(#3679): close two-axis review findings — no-renames gate, structural pin * chore(#3679): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
9d65cd5404 |
fix(#3664): warn when config-dir targets a foreign-agent destination (#3794)
* test(#3664): failing-first rows for the config-dir foreign-agent warning * fix(#3664): warn when config-dir targets a foreign-agent destination * test(#3664): fold the foreign-agent warning rows into the install-regressions suite * fix(#3664): close review findings — kimi-agents kind, gsd.md ownership, e2e gate * test(#3664): sync boolean call sites and the path-vocab registries * chore(#3664): backfill changeset pr number * test(#3663): skip the posix case-pin on win32 where folding is the fix --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
314ea20fa4 |
fix(#3663): fold path casing only on win32 in the w027 active-worktree check (#3793)
* test(#3663): failing-first rows for w027 path-casing normalization * fix(#3663): fold path casing only on win32 in the w027 active-worktree check * fix(#3663): close review findings — seam-owned compare key, deterministic case pin * chore(#3663): backfill changeset pr number --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
4af59f8dd3 |
fix(#3662): resolve managed hook node runners at hook-fire time (#3790)
* test(#3662): failing-first suite for runtime-resolving hook runners * fix(#3662): resolve managed hook node runners at hook-fire time * fix(#3662): close review findings and document the resolver * fix(#3662): close adversarial and security review findings * chore(#3662): backfill changeset pr number * test(#3662): honor win32 skip return and platform-aware sh runner pin * test(#3662): pin the bare win32-claude sh-hook shape omitting the bash runner --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
cf15682d1c |
enhance(#3028): responsive Markdown separators instead of fixed-width rules (#3789)
* feat(#3028): responsive Markdown separators instead of fixed-width rules Stage banners, checkpoints, completion and error panels used fixed-width runs of box-drawing characters -- a 53-column heavy rule and a 62-column double-line box. Those runs are ordinary text to a Markdown-rendering host, so in a narrower pane they wrap and the border comes apart from the heading it framed. Shipped content now emits an ATX heading for a titled section and a blank-line-delimited --- for a break between sections, both of which adapt to the available width. The same convention is applied to the three code sites that built these strings at runtime: the UAT checkpoint renderer, the milestone-close audit report, and the TDD review checkpoint table. Removing the box also removes its only reason to exist -- the east-asian-width padding helpers that kept its right border aligned (checkpointBoxLine, displayWidth, isWideCodePoint, ZERO_WIDTH_MARK_RE, CHECKPOINT_BOX_WIDTH). RTL directional isolation is unchanged. The convention is specified in gsd-core/references/ui-brand.md and enforced across all shipped content by tests/responsive-separators.test.cjs. Refs #3028 * test(#3028): pin the heading form in checkpoint and audit-report assertions These suites asserted the exact box borders and the 62-column padded banner interior. With the box gone they assert the ### heading form, the --- break and the bolded instruction line, and each now carries a positive assertion that no box character remains -- which is what pins the fix rather than merely tolerating it. Language coverage is converted, not dropped: Japanese, Chinese, Korean, Hindi and Arabic all still assert their rendered banner, and the Arabic case still asserts the RTL directional isolates the box removal must not disturb. Adds a case for a banner longer than the old inner width, which previously produced a ragged border and now has none. Refs #3028 * chore(#3028): acknowledge execute-plan.md growth from the checkpoint display spec The checkpoint_protocol display spec described the drawn box; it now describes the heading, the --- break and the bolded action prompt, which costs 22 bytes (40111 -> 40133, 827 under the cap). Appended to the existing #3370 fragment rather than filed as a new one: a growth ack keys on the bare filename and #3370 already declares execute-plan.md, so a second source naming it would be a hard duplicate-key error. Same supersede-by-append route #3370 took for the spent #2652 fragment. Refs #3028 * docs(#3028): state the load-bearing half of the separator rule, and amend the zh-CN reference Review found three things. The rule as first written demanded a blank line above AND below every ---. Only the one above is load-bearing: it is what stops CommonMark reading the rule as a setext underline for the line above. The one below is cosmetic, because a thematic break is a leaf block. The rule now says that, with the reason, instead of asserting a stricter form the content does not keep. The zh-CN reference had received the mechanical box-to-heading swap but none of the prose behind it: it still claimed a 62-character checkpoint width and still listed --- among forbidden mixed banner styles, so it contradicted the convention it was translating. It now carries the separator section, the setext reasoning, the unconditional-vs-per-runtime rationale and a corrected anti-pattern list, in Chinese. The user guide asserted that a heading is not a degradation anywhere. That is an assertion, not a demonstration. It now says what was actually traded away in a plain terminal, points at the recorded rationale, and invites the report that would justify the capability flag instead. Refs #3028 * chore(#3028): backfill changeset PR number Refs #3028 --------- Co-authored-by: sim <sim@local> |
||
|
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
|
||
|
Behruz Nassre Esfahani
|
622f43353c |
fix(#3299): tracer feedback gate honors workflow.human_verify_mode (#3390)
* fix(#3299): tracer feedback gate honors workflow.human_verify_mode
The tracer feedback gate (#2294) predates `workflow.human_verify_mode`
(#3309, whose scope was the planner and verifier only), and branched on
auto-mode alone. Under the documented `end-of-phase` default an
interactive run therefore halted after EVERY `type="tracer"` task,
synthesizing a `checkpoint:human-verify` no planner ever emitted and
asking the user to retype a verdict the executor had just computed —
at the cost of a full executor cold-start each time.
Planner-side suppression cannot reach this halt because the executor
synthesizes it at runtime, which is why #3309 did not close it.
The gate now branches on HUMAN_VERIFY_MODE in the interactive path:
under `end-of-phase` an automated-only tracer `<verify>` is re-run and,
on success, expansion continues with no checkpoint. HALT-on-failure is
unchanged. `mid-flight`, `gate="blocking-human"`, and tracers carrying
genuine `<human-check>` evidence all still stop; the autonomous branch
is untouched.
`--default end-of-phase` on the config read is load-bearing, not
decorative: `workflow.human_verify_mode` is absent from SCHEMA_DEFAULTS,
so a bare `config-get` exits non-zero with `Key not found` on any
project whose config.json predates #3309 — which is the reporter's
exact config and every pre-existing project.
Both copies of the rule (workflows/execute-plan.md and
agents/gsd-executor.md) are updated together; the reference doc records
the seam and the human-check-still-halts rationale so it cannot recur.
Fixes #3299
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): add changeset
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): reconcile the canonical schema table and the stale acceptance test
Review round 1 (trek-e) — three items, all in the drift class this PR is
about, two of them landed inside this PR's own diff.
1. docs/reference/plan-md.md:233 — CONTEXT.md names this file the canonical
schema reference for the tracer task-type contract, and its Task-types row
still claimed interactive runs unconditionally present a
checkpoint:human-verify. CONTEXT.md and docs/AGENTS.md were updated in the
first round; this one was missed, so the authoritative reference was the
wrong answer. The row now carries the human_verify_mode-conditional
behavior and points at the canonical precedence chain.
2. tests/tracer-bullet.test.cjs — the docs assertion only checked that a
tracer ROW EXISTS, never its content, which is why CI could not see the
drift. It now asserts the row's actual claims and rejects the pre-#3299
wording. Separately, the #1945 acceptance test named 'interactive run emits
checkpoint:human-verify after the tracer' kept passing only because its
substrings still occur in the fallback clause, while its name asserted the
opposite of shipped behavior. Renamed and narrowed to what #1945 still
guarantees, plus a new interactiveIsConditional pin so the unconditional
prose cannot be restored under a passing substring check.
3. plan-md.md's <verify> row now documents that the legacy bare-text form
(valid, and still shown at :179) does not reach the #3299 auto-continue —
only a <verify> carrying <automated> does — so the benefit is silently
unreachable for tracers using that format.
Mutation-verified: reverting the plan-md row fails 1 test; reverting the
executor's interactive branch fails 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): make the tracer gate reachable from the planner template, and bind the assertions
Peer review round 3 found two Majors, both verified by reproducing the
mutation before fixing.
MAJOR 1 — the fix was largely inert on its own default path.
agents/gsd-planner.md's Nyquist Rule (:191) says every <verify> includes
<automated>, but the tracer-specific template twelve lines later emitted the
legacy bare-text form. The gate auto-continues only on a <verify> carrying
only <automated>, so every tracer produced from the canonical template fell
to the STOP fallback and #3299's benefit was unreachable for exactly the task
type it targets. Template now wraps in <automated>; a contract assertion pins
it so the two cannot drift apart again.
MAJOR 2 — the new assertions did not bind condition to action.
Appending 'Nevertheless, interactive runs always present a
checkpoint:human-verify' to the canonical row, and 'then immediately STOP and
return a checkpoint:human-verify' to the auto-continue clause in BOTH
operative copies, restored unconditional interactive checkpointing and left
the suite 35/35 green. Every required keyword still matched. Fixed by:
- clause 2 must now contain no STOP outcome and emit no checkpoint at all —
'never a checkpoint' has to be true OF the clause, not merely stated in it;
- interactiveIsConditional replaced with the ordered-clause parse plus the
same no-STOP property, instead of proving only that HUMAN_VERIFY_MODE
appears somewhere on the line;
- the plan-md.md Autonomy cell is now pinned EXACTLY rather than by keyword
presence. Deliberately brittle: CONTEXT.md names that table the canonical
schema reference, so a wording change must be a conscious edit in both
places.
Mutation-verified after the fix: the combined semantic regression now fails 3
tests; reverting the planner template fails 1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): exact-pin the safety clauses instead of blacklisting outcome verbs
Peer review round 4. Blacklisting did not hold, twice over:
- Round 3 banned literal STOP and the 'return a'/'present a' checkpoint
forms in the auto-continue clause. Round 4 defeated that by appending
'then pause and invoke checkpoint_protocol with a checkpoint:human-verify
before expansion' — none of the banned tokens, same restored interruption
after every successful tracer. 36/36 passed.
- The planner guard looked for <automated> anywhere inside <verify>, so
'<verify>[...]<!--<automated>--></verify>' satisfied it while leaving the
legacy bare form operative. 107/107 passed across tracer, planner and the
three size-cap suites.
Synonyms are unbounded; the clauses are not. Both are now pinned exactly on
normalized whitespace, the same approach already proven on the plan-md.md
Autonomy cell, with defence-in-depth checks behind them: no checkpoint-emitting
or blocking outcome in any wording inside clause 2, and the planner's <verify>
body must be exactly one non-empty <automated> child with no commented markup.
These pins are deliberately brittle. Each is a safety contract, so changing the
behavior must be a conscious edit in both the prose and the expectation.
Mutation-verified: the synonym-checkpoint mutation fails 1; the commented-out
wrapper fails 1; the round-3 literal-STOP + contradictory-doc-row regression
fails 3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): strip comments, require uniqueness, pin whole regions
Peer review round 5. Exact-pinning one clause was still bypassable two ways,
both reproduced before fixing (each left the suite fully green):
- COMMENTED DECOYS. Put the correct text in an HTML comment followed by a live
wrong copy: every extractor selected the commented decoy. Worked against the
planner template, the canonical plan-md.md row, and both executor branches.
- SURROUNDING OVERRIDE. Insert 'after every tracer, pause and invoke
checkpoint_protocol before expansion, regardless of the mode-specific rules
below' immediately ABOVE the pinned clause, or 'ignore row 3; always wait for
approval' below the canonical table. The pinned text was untouched, so
equality held while the shipped meaning inverted.
The shape that holds, applied to every operative surface:
1. strip HTML comments BEFORE selecting, so a decoy cannot be chosen;
2. require the structural anchor to occur EXACTLY ONCE, so a live second copy
cannot hide behind a correct first one;
3. pin the ENTIRE decision region, not one clause, so no unparsed prefix or
suffix can override what the pin proves.
Applied to: the executor's whole tracer branch, execute-plan.md's whole
dispatch line, checkpoints.md's whole precedence section, and plan-md.md's
Autonomy cell.
Also addresses the round-5 Minor: the planner template is now asserted
STRUCTURALLY (exactly one <verify> in the fenced block, body exactly one
non-empty <automated> child) rather than pinning the descriptive placeholder
verbatim, so behavior-preserving wording changes no longer false-fail. The
clause and section pins keep their exact form — those have a safety rationale
the placeholder copy does not.
Mutation-verified, all six rounds: override-above-clause 1; commented decoy row
1; commented decoy branch 1; ignore-row-3 override 1; synonym checkpoint 1;
commented-out wrapper 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): drop the superseded exact-placeholder planner assertion
Peer review round 6, Minor. The round-5 brittleness fix ADDED a structural
planner assertion but left the old exact-placeholder one in place, so the
over-brittleness it was meant to remove was still live: rewording the
descriptive placeholder while preserving exactly one non-empty direct
<automated> child failed the old test and passed the new one.
Removed the old test. The structural assertion is the real contract — the gate
auto-continues on the SHAPE of the verify, not on the wording of a placeholder.
Verified both directions: a behavior-preserving reword now passes; reverting the
template to bare <verify> still fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): select operative prose via parsePredicates, not a hand-rolled scanner
Peer review round 7. I had judged the round-6 selector bypass adversarial-only
and out of scope, intending to disclose it. Both premises were wrong, and the
review said so:
- 'Needs new src API' — false. parsePredicates is ALREADY a public export and
internally uses the repo's interleaved fence/comment scanner. Instrumenting
candidate lines as throwaway predicate declarations borrows that scanner with
no src change at all.
- 'Adversarial-only' — false, and this is the part that mattered. Two ORDINARY
edits silently turned the guards into decoy checks:
* a forgotten '-->' comments the live rule through to EOF, and the
balanced-only stripper still saw and accepted the commented rule;
* a normal fenced documentation example of the rule, plus a whitespace-only
reformat of the live list item, made the selector choose the example.
Neither needs intent. A dangling comment is a typo; a fenced example is good
documentation. Together they reproduce exactly the accidental drift #3299 came
from — with CI green.
The selection layer now defers to parsePredicates for operativeness, uses
whitespace-tolerant anchors so a reformat cannot decouple the live line from its
pin, extracts regions by operative line index rather than string search, and
carries a self-guard test proving fenced / balanced-commented /
after-unclosed-comment copies are all excluded. The helper also ignores indexes
it did not inject, so a pre-existing GSDTEST.CANDIDATE line cannot pollute it.
Verified both ordinary-edit scenarios now fail the suite (each was green before).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): close the operative-selection gaps the maintainer blocked on
trek-e's Blocker: the operative-line selection layer had three gaps, all
reachable by ordinary future doc edits rather than sabotage. He independently
found a fourth I had not disclosed. All are fixed.
1. INDENTATION PROMOTION (his find, not in my disclosure). The instrumentation
replaced a matched candidate with an UNINDENTED marker regardless of the
original line's indentation. A 4-space-indented CommonMark code block is not
skipped by parsePredicates (it accepts indented declarations by design), so
stripping the indent PROMOTED an indented decoy to operative — the exact
inversion of the guard's purpose. The marker now preserves the original
indent, and a candidate that is itself indented 4+ spaces is never injected.
2. NO SET MEMBERSHIP. The filter accepted any in-range integer, so a
pre-existing literal GSDTEST.CANDIDATE=<valid index> in source text could
pollute the count. Now filters on a Set of the indexes actually injected on
this call.
3. RAW FENCE SELECTION (planner). The template test matched the first raw
```xml fence after the marker with no fence/comment awareness — the one
selection in the suite that was not operative-aware — so a commented-out
decoy template between the marker and the real one would be selected while
the live template regressed. The opener must now be operative AND the first
non-blank line after the marker.
4. RAW END ANCHOR (regionFrom). The end anchor was tested against raw lines, so
a fenced example containing a ### / <type line truncated the pinned region
early — a false FAILURE on a legitimate doc edit. End anchors now go through
the same operative filter as start anchors.
Mutation-verified: the indented-decoy + whitespace-varied-anchor combination
and the commented-out fence decoy each now fail the suite (both passed clean
before). Truncation is confirmed fixed by extraction — the region spans the
full section and retains the content following a fenced example, where it
previously stopped at it.
Note on the remaining brittleness: adding a fenced example INSIDE a pinned
region still fails the whole-region exact pin. That is the intended tradeoff
for a safety contract, not the truncation defect, and is called out as such.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): allow-list operative indentation; pin marker provenance
Review round 9.
BLOCKER — the round-8 indentation guard was written as a DENY-list,
/^(?: {4,}|\t)/, and CommonMark has more indented-code forms than that
enumerates: " \t", " \t" and " \t" all open an indented code block and all
slipped through, so an indented decoy was still promoted to operative while the
live rule regressed (34/34 green). Inverted to an allow-list — only 0-3 literal
spaces is ordinary block indentation; anything else is code. Enumerating the
bad shapes was the error, not the specific regex.
MINOR — the injected-index Set validated the marker's VALUE but not its SOURCE.
A pre-existing literal `GSDTEST.CANDIDATE=<n>` could name an index that some
other (skipped) candidate had contributed to the set, and be accepted. Now also
requires p.line - 1 === Number(p.value): the predicate must have been parsed
from the line it names.
MINOR (false negative) — ```xml title=x is a valid CommonMark info string, and
requiring exactly ```xml failed the suite (33/34) on a behavior-preserving edit.
Both the opener assertion and the extraction now accept an info string.
Mutation-verified: the mixed " \t" decoy and the forged-provenance marker each
now fail; the info-string fence no longer false-fails.
KNOWN LIMITATION, disclosed on the PR rather than papered over: parsePredicates
is a predicate parser, not a general CommonMark operativeness oracle. Two
standards-valid constructs still read as operative — a lazy blockquote
continuation line (state opens only on a line that literally starts with ">"),
and a comment opened mid-line ("prose <!--", where state opens only when the
trimmed line STARTS with "<!--"). Closing those means either teaching the shared
src/context-predicates.cts about container/lazy-continuation state — a change to
a module every health rule consumes, well outside a tracer-gate fix — or
hand-rolling a CommonMark parser inside a test, which is how this suite got into
trouble in the first place. Left for the maintainer to scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): re-arm the execute-plan.md emitted-drift ack after the base merge
The #3299 ack rode on tests/emitted-drift-acks/2652-quick-diagnose-dispatch-isolation.json,
which upstream retired in
|
||
|
Behruz Nassre Esfahani
|
a44d513566 |
fix(#3712): confine in-process installs to a sandboxed HOME (#3725)
* fix(#3712): confine in-process installs to a sandboxed HOME
A runtime kind may declare a global `home` override resolved from os.homedir()
rather than from the caller's configDir — codex's skills kind (`home: ".agents"`,
ADR-1239 / #2088) is the only live case. Sandboxing configDir/targetDir does not
contain it, and assertDestWithinConfigHome cannot see the class: that gate
confines a destSubpath to whatever root it is handed, and here the root IS the
escaped home. So an in-process caller that forgot to sandbox HOME wrote to, and
pruned gsd-* entries from, the developer's REAL ~/.agents/skills.
tests/agent-descriptor-parity.install.test.cjs's K1 loop did exactly that: it
iterates every agents-kind runtime (codex included) with a sandboxed targetDir
and an un-sandboxed HOME. Reproduced against a canary home on next @
|
||
|
Tom Boucher
|
1178c5f995 |
test(#3108): make the overlay ENOENT tolerance and hooks/dist readiness check honest (#3772)
* test(#3108): failing-first suite for the overlay vanish-retry and hooks/dist staleness RED by construction, and deliberately narrower than the issue. #3108 reports a bare `ENOENT ... link '/work/hooks/dist/gsd-session-state.sh'` and attributes it to hooks/dist never having been built. That mechanism cannot produce that error: buildOverlayRepo enumerates with readdirSync and links what it enumerated, so a directory that never existed yields no names and no link is ever attempted. The error requires the file to have existed at readdir and vanished before the link -- which is the atomic-replace race placeVanishableLeaf was written for in #3285, three days AFTER this issue was filed. What is still genuinely broken, and what these tests bind to: placeVanishableLeaf's retry is unguarded. On ENOENT it re-checks existsSync and calls attempt() once more, bare. A second atomic replace inside that window throws an unhandled ENOENT of exactly the reported shape. Rows 1/2/3/5/6/7 fence the surrounding contract -- most already pass, which is the point: they are what stops the fix from widening into "swallow every error". Row 6 in particular covers a non-ENOENT on the RETRY, the exact path the new code will live on. ensureHooksDist's staleness predicate is extension-blind. It rebuilds only when hooks/dist is absent or holds zero .js files, while build-hooks.js also ships .sh -- including gsd-session-state.sh, the very file in the report. A dist with .js present and every .sh missing reads as populated and the rebuild is skipped. Rows 10 and 11 are mirrored on purpose: asserting only the .sh direction would permit swapping one extension heuristic for another, so both directions force the predicate to be about the expected set (HOOKS_TO_COPY, which build-hooks.js already exports) rather than about counting an extension. Rows 12 and 13 are cost guards. ensureHooksDist runs per suite and its rebuild is a real subprocess, so a predicate that over-triggers turns a correctness fix into a throughput regression nobody attributes to it; and hooks/dist legitimately carries files the expected list does not name, so an exact-set match would rebuild forever. The warning-text row is t.skip()'d rather than faked: the only ways to assert it were a source-grep (banned by local/no-source-grep) or a full overlay build, and a test that cannot be written honestly is better skipped visibly than written vacuously. Filename note: this started as fix-3108-*.test.cjs and tripped lint-regression-test-names, which bans new fix/bug/issue-NNNN files, then as install-overlay-helpers.test.cjs and tripped lint-test-file-count, whose `install` bucket is already at its limit. Module-named under the overlay bucket satisfies both. The allowlists were left untouched -- both are empty, so nothing here is grandfathered and adding an entry would have been the wrong instinct. * fix(#3108): guard the overlay retry and make the hooks/dist check see .sh Two holes, both reachable from the failure #3108 reports, neither of them the cause it names. placeVanishableLeaf's retry was bare. On ENOENT it re-checked existsSync and called attempt() once more with no catch, so a second atomic replace landing inside that window threw an unhandled ENOENT -- exactly the reported `ENOENT ... link '/work/hooks/dist/gsd-session-state.sh'`. Two vanishes inside the window means the same thing one does: the path is going away and is not part of the snapshot. It now returns false and skips the leaf, reaching the conclusion the single-vanish case already reached. Still ONE retry. No loop, no backoff, no sleep -- the existing comment argues that a timing-based wait here would be the flake rather than the fix, and that reasoning did not change. Non-ENOENT still propagates from either attempt, which is the invariant a careless widening would eat; the suite pins it on the retry path specifically, because a fix that guarded only the first attempt would look right and be wrong. ensureHooksDist could not see the file class that caused the report. It rebuilt only when hooks/dist was absent or held zero .js files, while build-hooks.js also ships .sh -- including gsd-session-state.sh itself. A dist with .js present and every .sh missing read as populated and the rebuild was skipped. The predicate is now membership against build-hooks.js's own exported HOOKS_TO_COPY, so it asks "is everything expected present" instead of counting an extension, and it cannot be blind to a file class again. It is extracted as isHooksDistStale(dir) with ensureHooksDist calling it, so there is one predicate rather than two that can drift. Extra unexpected entries are explicitly not stale -- hooks/dist legitimately accumulates subdirectory and hooks/lib output, and an exact-set match would rebuild forever. One readdirSync into a Set, no per-entry existsSync, no stat: it runs per suite and its rebuild is a real subprocess, so an over-triggering predicate would turn this into a throughput regression nobody would attribute to it. The skipped-leaf warning now names `npm run build:hooks`. It already named the cause; a reader still had to know what produces that directory. Deliberately NOT done: nothing here makes an absent hooks/dist fail. Absence is a legitimate package shape that bin/install.js:11191 treats as "nothing to verify", and six of the seven install suites never read the directory at all. * fix(#3108): count hooks/dist subdirectories, and never throw out of the predicate Two gaps found reviewing the predicate I had just written. It ignored HOOKS_SUBDIRS_TO_COPY. That is ["lib"], and hooks/dist/lib carries gsd-graphify-rebuild.sh, so a dist with all 27 top-level files but no lib/ read as populated. That is precisely the blindness the .js-count heuristic had, one level down: a whole file class invisible to the check. Fixing the extension case and leaving the subdirectory case would have been half a fix, and the half left behind is the one nobody would look at again. Subdir names are bare (no slashes), so they slot into the same top-level readdir Set — no second readdirSync, no stat. Whether lib is really a directory is not checked; that would cost a stat per entry and buys nothing, since the build owns that. It could also throw. existsSync passing does not make readdirSync safe: the path may be a regular file (ENOTDIR), unreadable (EACCES), or retired in the race between the two calls. This helper runs in every install suite's before(), so an unhandled throw there fails a suite on a condition it cannot act on. Unreadable is indistinguishable from unusable for this question, and rebuilding is idempotent, so it now reports stale instead. Both are the same shape as the original bug and the same shape as each other: a guard that answers "is this ready" must not have a blind spot or a hard edge, because every caller treats a false negative as "carry on". Two existing tests asserted a "complete" dist without lib and had to be corrected to keep meaning what their names claim, rather than being left passing against a definition of complete that no longer holds. * test(#3108): close the review findings, including a half-closed subdir check An isolated correctness reviewer found no blockers and four real gaps. The wiring was untested. Every Group-2 test exercised the pure predicate; none called ensureHooksDist. So restoring the old inline .js-count check INSIDE ensureHooksDist -- keeping isHooksDistStale exported and correct -- left the whole suite green, and that wiring is the actual #3108 defect. Two tests now drive ensureHooksDist itself through the process seam: build invoked exactly once when stale, never when fresh. The second is the one a permissive revert fails. Reaching that seam meant requiring process-seam as a module object rather than destructuring runNode, so a test can replace it in place. That is a testability affordance in a test helper, not a production change, and it is commented as such so it does not read as an accident later. The subdir check was only half closed, and the half left open was the important one. It required `lib` to be PRESENT in the top-level readdir, never looked inside -- so an EMPTY dist/lib, missing gsd-graphify-rebuild.sh, still read as populated. That is precisely the missing-file-class case the subdir check was added to catch, which made the fix a gesture at the problem rather than a fix. Each subdir entry must now be a readable, NON-EMPTY directory. A stray regular file named `lib` throws ENOTDIR into the same try/catch and reads stale too. Cost stayed honest: one extra readdirSync total (there is exactly one subdir entry), no stat, no per-expected-file syscall. Probed against the real hooks/dist -- still reports fresh, so no suite gains a rebuild. Two nits, both real: the error-code sweep re-tested EACCES already covered standalone, and the property ignored presentAtFinalAttempt whenever vanishCount was not 1, making roughly half the 200 runs duplicates. The flag now varies meaningfully across the whole range and the assertions depend on it. One reviewer finding was already stale: the subdir and ENOTDIR work was uncommitted when the reviewer snapshotted the tree, and had landed in a67aefb9c before the report arrived. Verified rather than assumed. * fix(#3108): stop the vanish tolerance from swallowing a dest-side ENOENT A defect this PR introduced, caught by an isolated security reviewer. linkSync(src, dest) throws ENOENT for the DESTINATION path too, not only for a vanished source. The widened retry caught that, saw the source still present, retried, got the same dest-side ENOENT, and returned false -- recording the leaf as "vanished mid-walk" and printing a warning that tells the reader to run `npm run build:hooks`. A remedy with nothing to do with the actual cause, an overlay quietly short a file, and the install under test proceeding against an incomplete tree. It also falsified the function's own documented invariant, which says in as many words: "Returns false only when the path left the source tree entirely." Widening the tolerance without re-reading the sentence above it is how that happens. The retry now re-checks existsSync(srcPath) before tolerating: source still present means the ENOENT was about something else and it propagates untouched. Chose the existsSync re-check over comparing retryErr.path to srcPath -- err.path normalization is not guaranteed across platforms, and a path-equality test is a subtler thing to get wrong later. The FIRST catch was probed and is already correct: for a dest-side ENOENT the source is present, so it falls through to the retry rather than returning false. Left unchanged rather than "fixed" symmetrically. Two regression pins, deliberately opposed: a dest-side ENOENT with the source present must THROW, and a genuinely absent source must still return false. The second exists because the obvious over-correction -- always rethrow on the retry -- passes the first and silently undoes what this PR set out to fix. Also closed the skipped placeholder. It claimed no non-flaky seam existed for asserting the warning text; the reviewer pointed out an injectable `warn` param is trivial, and they were right. buildOverlayRepo now takes opts.warn defaulting to console.warn (byte-identical for every existing caller) and the skip is replaced by real tests: fires with the remedy named on a skipped leaf, silent on a clean walk. "No seam exists" was a design choice presented as a constraint. Recorded the sequential-only constraint at the two sites that monkeypatch fs process-wide: adding { concurrency: true } to this file would cross-contaminate every other suite in the process. Better written down than rediscovered. Known limit, disclosed rather than fixed here: a legitimately dropped leaf can still pass vacuously downstream -- agent-fragments-emission asserts a negative over filesContaining, and mcp-catalog-parity has only an anti-vacuity floor of one. That is a pre-existing property of those suites and the tolerance #3285 already chose; this change narrows which drops are possible rather than adding the completeness assertion those suites lack. * fix(#3108): discriminate ENOENT by the dest parent, not by re-checking the source The previous commit's dest-side guard was wrong, and the remote run said so: "a leaf that vanishes again during the retry is skipped, not a bare ENOENT" Got unwanted exception. Actual message: "ENOENT: no such file or directory" That test was right and the guard was wrong. It rethrew when existsSync(srcPath) was still true, on the theory that a present source means the ENOENT was about the destination. But in the genuine race the source is being atomically REPLACED, so it is legitimately present again at the re-check while the ENOENT was entirely source-side. The gate therefore threw on precisely the race #3285 exists to tolerate -- trading one misclassification for a worse one, since the old bug was a bare crash and the new one broke the working tolerance. The security reviewer's alternative discriminator does not work either, and a probe settles it. Node populates BOTH `path` and `dest` on a link ENOENT, and `err.path` is the SOURCE in both directions: linkSync(existingSrc, missingDir/a.txt) -> ENOENT path=<source> dest=<dest> linkSync(missingSrc, validDest) -> ENOENT path=<source> dest=<dest> So the error object cannot tell you which side failed. What CAN: the dest parent. buildOverlayRepo builds its own dest tree -- place() mkdirSync's recursively into a private mkdtempSync root no other process touches -- so a missing dest parent is always a bug (Windows MAX_PATH, a concurrent cleanup, a bad dest), never the replace race. A present dest parent means the ENOENT was about the source, which is the case we tolerate. placeVanishableLeaf therefore takes an optional destPath and uses the dest parent as the sole discriminator when it has one; with no destPath it behaves exactly as before. linkOrCopyFile and the copy-mode call site both pass it, because those are the two places that actually know the destination. The doc comment now records BOTH failed discriminators and why each fails -- existsSync because the source is legitimately replaced mid-race, err.path because it names the source either way. Those are the two things a future reader reaches for first, and both look correct until they are not. The dest-side regression pin was rewritten to drive the real mechanism: a real temp source and a dest whose parent does not exist, through linkOrCopyFile. Previously it forced a throw through a present source, which is what encoded the wrong theory into a test and made it look verified. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
004e9dd741 |
fix(#3007): resolve Codex reasoning effort per model and make every clamp visible (#3765)
* test(#3007): failing-first suite for per-model Codex effort capability RED by construction. Binds to behavior renderEffortForRuntime does not yet have: an optional third `model` argument, a per-model advertised-level table, `max` passing through instead of clamping to `xhigh`, `minimal` clamping to `low`, `ultra` rejected outright, and clamp visibility (`requested`/`clamped`/ `reason`) so a downgrade is legible from resolver output rather than silent. Two of these pin defects that exist on next today: - `max` is discarded. Both Codex models whose catalog entries are retrievable (sol, luna) advertise `max`; GSD clamps it to `xhigh` and reports nothing. - `minimal` is emitted to a model that refuses it. providerPresets.openai. haiku.low pairs gpt-5.6-luna with reasoning_effort "minimal", and luna's advertised floor is `low`. GSD is sending a value into a document Codex itself validates. The parity test is what pins that fixed, and it names the offending path/model/effort when it trips. Also corrects tests/model-resolver.test.cjs:351, which asserted renderEffortForRuntime('codex','max').value === 'xhigh' -- the defect pinned as though it were a contract. ADR-443 recorded "Codex has no max" as fact and it was true when written; Codex has since added both `max` and `ultra`. That is a stale premise, so the assertion is corrected here rather than worked around. The property test asserts the invariant the whole change exists for: a rendered effort is always a level the target model actually advertises, or an explicit rejection. There is no third outcome. * fix(#3007): resolve Codex effort per model, and make every clamp visible Codex declares supported_reasoning_levels per MODEL and validates against it, so a single per-runtime capability set cannot be right for all of them. GSD's was wrong in both directions at once. `max` reaches Codex now. ADR-443 recorded "Codex has no max" as fact and clamped max -> xhigh on that basis; it was accurate when written, and Codex has since added both `max` and `ultra`. Every Codex model whose catalog entry is retrievable advertises `max`, so the clamp was discarding a level the provider supports, silently, on the most-used path. `minimal` stops reaching Codex. No Codex model advertises it -- both retrievable entries floor at `low` -- yet providerPresets.openai.haiku.low paired gpt-5.6-luna with reasoning_effort "minimal". GSD was writing a value the receiver validates and refuses into a file the receiver reads. Being unconservative in what you send is the half of Postel's rule with no defensible reading, so that preset is corrected and a parity test pins it. `ultra` is refused rather than laddered. Codex's own catalog calls it "Maximum reasoning with automatic task delegation": at ultra, effective_multi_agent_mode returns Proactive and Codex spawns sub-agents on its own initiative, underneath GSD's orchestration rather than inside it (#2167). It is a mode switch, not a reasoning depth, so it is not added to the universal ladder -- which stays provider-agnostic by ADR-443's design -- and it is rejected even for gpt-5.6-sol, which does advertise it. Clamping it down to `max` was considered and rejected: that silently discards what the user actually asked for. Clamping is now visible. RenderedEffort carries requested/clamped/reason and resolve-execution surfaces them. The previous table clamped correctly but invisibly, so a user asking for `max` on Codex had no way to find out they were getting `xhigh` -- exactly the failure mode the robustness principle's modern critique warns about, and why "be liberal" has to mean "liberal and loud". Also closes a latent trap found while reviewing the implementation: the clamp-up loop walks the ladder upward, and for a future model advertising `ultra` but not `max` it would have selected `ultra` as the clamp target -- re-entering by the back door the mode the rejection above exists to keep out. A clamp may never produce a value that a direct request for that value would refuse. Unreachable with today's catalog, which is why no test caught it; a test now asserts the invariant directly. Signature stability is preserved: the third `model` argument is optional and the two-argument form still resolves, against the family baseline. That form's BEHAVIOR does change for `max` and `minimal`, and it must -- keeping the old answer would have fixed the defect only where a model happened to be threaded through and left it live everywhere else. tests/model-resolver.test.cjs:351 asserted the defect as if it were a contract and is corrected here rather than worked around. * fix(#3007): close every review finding on the Codex effort alignment Two isolated reviewers, correctness and security. Both found the same two blockers, and the per-model work was inert on every surface that matters until this commit. BLOCKER — resolve-execution never passed the model and discarded the clamp. cmdResolveExecution called the two-argument form and emitted only effort_rendered/effort_param/effort_propagation, so the per-model table was unreachable from production code (tests were its only caller) and requested/ clamped/reason were computed and thrown away. Requested outcome 3 names "the effective rendered effort in resolver output" specifically, so the feature was unmet on the exact surface the issue asks for. Now passes the resolved model and emits effort_requested / effort_clamped / effort_clamp_reason, flat, matching the existing key convention rather than introducing a nested object. BLOCKER — the docs described output that did not exist. CONFIGURATION.md showed a nested {"effort": ...} sample; the real result is flat and those keys were absent entirely. A reference doc asserting a JSON path a reader can copy is worse than no doc. Corrected against the actual emitted key set. MAJOR — the argv channel still shipped both original defects. EFFORT_ARGV.codex kept minimal in its supported set and still clamped max down to xhigh, so the invocation-time and install-time channels disagreed about the same runtime's capability: --host codex with max emitted xhigh while the generated TOML said max. This is the repo's documented generative-fix-divergence class, so both tables now cross-reference each other and a parity test fails if they ever diverge again. MAJOR — malformed catalog data failed OPEN and could crash the CLI. A null _baseline became an EMPTY Set that is nonetheless truthy, so the nullish fallback never fired and every effort rendered as null. And a non-array value made the Set constructor throw at module load — model-catalog.cjs is required across the whole CLI, so one bad JSON value killed every command, not just codex effort. Guarded on size and filtered to array values; both degrade to the hardcoded baseline. MAJOR — value widened to a nullable string with two consumers left behind. runtime-artifact-conversion passed it straight into injectEffortFrontmatter (a null effort key in generated frontmatter); install-effort-resolver still declared a non-nullable return, a structural lie that silently defeated null checking. Both corrected, both omitting the key on null — the same posture as 'inherit', where omission means "follow the host default". MAJOR — the per-model table is inert today, and the docs now say so. All three shipped models advertise the same usable range and ultra (sol's only differentiator) is rejected for every model, so no observable output differs by model. The table stays because Codex declares capability per model and the sets are free to diverge — a single per-runtime assumption is precisely what went stale and produced this issue — but overselling it as a visible per-model feature would have been the same class of error as the doc blocker above. Tests: three passed under a full revert and are strengthened rather than deleted, since each guards a real contract (#3533's inherit rule, the undeclared-host rule, off-ladder handling) — they now also assert the clamp-visibility fields, which only exist after this change. The fast-check property is kept for its shrinking, and a deterministic nested loop over the full cross-product now sits beside it so coverage is exhaustive rather than sampled. Also folded in earlier: bin/install.js generated the Codex TOML with the two-arg form and would have written a literal null reasoning effort on the ultra path; CONTEXT.md's Model Catalog Module glossary entry now records CODEX_MODEL_EFFORT. The installer defect was found by the co-change gate, not by a reviewer — install.js is a historical co-change partner of model-catalog.cts that this diff had not touched. * test(#3007): correct assertions that pinned Codex's stale effort premise Thirteen pre-existing tests encoded "Codex has no max" as fact and failed on the shipped commit. Every one is a stale pin, not a defect: each was probed against the built module before its expectation was changed, and none failed for a reason other than this premise correction. Kept as its own commit per CONTRIBUTING — a test-fixture correction made stale by a production change must not ride inside another commit, because the release-sdk hotfix cherry-pick filter routes by subject prefix and a correction buried under the wrong prefix ships a half-state (v1.42.3, #3621). The most valuable one was tests/model-resolver.test.cjs's cross-provider validity invariant, which hardcoded the Codex enum as `minimal|low|medium|high|xhigh` and failed with "real API would 400". That message is now false in both directions: Codex accepts `max`, and rejects `minimal`, which no model advertises. The enum is corrected to `low|medium|high|xhigh|max` and the guard is kept intact — it is exactly the "would the real API refuse this" check worth having, and it was right to fail here. It simply carried the stale fact in its own fixture. Test NAMES were corrected alongside their assertions wherever the name asserted the old behavior — "max is Anthropic-only", "max clamps to xhigh", "minimal passthrough". A renamed test that still claims the old thing is worse than a failing one, and a green test whose name states a falsehood is how the next reader inherits the wrong premise. Both channels are covered: install-time (renderEffortForRuntime, and the generated .toml in install-runtime-artifacts) and invocation-time argv (effort-surface-axis). They were deliberately brought into agreement in this change, so their assertions had to move together. Each site carries a #3007 comment recording that Codex gained max/ultra and that capability is declared per model, so a future reader can tell this was a deliberate premise correction rather than a test bent to fit an implementation. * test(#3007): separate the effort-precedence case from the clamp case The previous stale-assertion pass over-corrected one test. It saw `effort: { default: 'max' }` on codex expecting `effort_rendered: 'xhigh'`, assumed the xhigh came from the max→xhigh clamp #3007 removes, renamed it to "max passes through" and changed the expectation to `max`. The remote runner disagreed. Reproduced against the real CLI: with that config and `gsd-planner`, the resolver emits `effort: "xhigh"`, `effort_requested: "xhigh"`, `effort_clamped: false`. The xhigh is produced by effort-resolution PRECEDENCE — gsd-planner is heavy/opus tier and its routing-tier default outranks `effort.default` — so `max` never reaches the renderer at all. The test says nothing about clamping and never did; it only looked like a clamp pin because both mechanisms happened to yield the same string. Restored to `xhigh` and renamed to say what it actually tests. It now also asserts `effort_clamped === false` and `effort_requested === 'xhigh'`, which is what makes it impossible to mistake for a clamp pin again: those two fields prove the value is what the resolver produced rather than something the renderer downgraded. Before #3007 there was no way to tell the two apart from the output — which is precisely why the previous pass could not tell them apart either. Added the test that was actually missing: `effort.agent_overrides`, which outranks the tier default, so the requested level genuinely reaches the renderer and `max` survives to `effort_rendered` end-to-end through the real CLI. Verified by probe before asserting. One test now pins the precedence rule and the other pins the #3007 behavior, and neither can be read as the other. That the clamp-visibility fields are what resolved this is a small argument for having added them. * chore(#3007): backfill changeset pr number to 3765 * test(#3007): put model-catalog under the mutation gate The Stryker shard showed as `skipping` on this PR despite the diff rewriting model-catalog's effort logic. That was legitimate, not a detection bug: `model-catalog` was never in scripts/mutation-matrix.cjs's COVERED map, so the whole module — including everything #3007 touches — sat entirely outside mutation scoring with has_work "false". Registered, with a dedicated spawn-free surface. tests/model-catalog.unit.test.cjs is new: 44 in-process tests, no runGsdTools, no child process, no filesystem, no temp dirs. That shape is not stylistic — it is the #2790 precedent this file already documents. Stryker's command runner treats a whole `node --test <file>` invocation as ONE test costing whatever its slowest case costs, and re-runs it per mutant, so pointing a shard at tests/model-resolver.test.cjs (which uses runGsdTools throughout) would reproduce exactly the 15-minute shard-cap cancellation #2790 hit. The integration file is unaffected and keeps running in full in the normal test job. Coverage spans the module rather than only the diff, because the score is measured over the whole file: effort rendering across every model and ladder level in both channels, the prototype-chain host guard, the exported enums and maps, isAnthropicFlavoredModel's provider namespacings, the profile projections, nextTier, and mergeEffortTierDefaults. The last two were nearly left out and are worth naming — every uncovered exported function is score given away, and mergeEffortTierDefaults turned out to have a genuinely interesting contract (#3531: a partial override merges over the built-ins rather than replacing them, and isValid gates the VALUE, not the tier name, so an unknown tier key is still merged in). Every expectation was probed against the built module before being asserted. minScore is 1 and that is a PLACEHOLDER, flagged as such in the registry comment. Floors in this repo are measured, not chosen — the existing entries sit at 94, 75 and 56 — and they can only be measured in CI, because mutation shards run `node --test`, which is hard-blocked locally. The first CI run on this branch reports the real number and the floor gets ratcheted to it before merge. A placeholder of 1 reaching `next` would make the gate decorative: it would pass whether or not a single mutant is ever killed. Note the target is "never regress from measured", not a fixed 80 — planning-inspect sits at 56 and is documented as an accepted ratchet candidate. * test(#3007): bootstrap model-catalog's mutation floor legally The placeholder floor was structurally illegal and the remote run said so. tests/mutation-matrix-ratchet.test.cjs guards the guard: every COVERED module must carry a matching RATCHET_BASELINE entry in the same diff, minScore must EQUAL that baseline, and it must be at least 50. `minScore: 1` failed all three. That is the ratchet working exactly as intended — a floor nobody can satisfy accidentally is the point of it. Bootstrapped at 50 in both places. Fifty is not a measured score and the comment says so plainly: it is the minimum the guard permits, and it coincides with Stryker's own configured `break` threshold, so it is the lowest legal starting point for a module that has never been measured. It still must be ratcheted to floor(measured) - 1 before this PR merges. Also corrected a real defect in the file's own instructions. "HOW TO UPDATE" step 1 read "Run the per-module Stryker shard locally" — which cannot be done here, and which the same file contradicts eighty lines further down, where the #2790 scores are recorded as "not a local run; mutation shards run `node --test`, hard-blocked in this repo's local environment". stryker.config.mjs confirms the command runner invokes `node --test` once per mutant, and .claude/hooks/block-local-node-test.sh denies exactly that. So the documented first step sends the next contributor at a wall. Rewritten to describe the path that works — push, read the measured score off the CI shard, then set the floor and its baseline together in one diff — and to say why local measurement is not available, so nobody rediscovers it the slow way. GOODHART SAFETY is untouched. The two-step is inherent to the environment rather than a shortcut: a floor cannot be measured before the first CI run exists, and the guard rightly refuses to accept an unmeasured one below its minimum. * test(#3007): ratchet model-catalog's mutation floor to its measured score The shard ran in CI and reported 59.62% — 248 mutants killed, 168 survived, no timeouts, no errors (run 32605073352, job 97108869486). Floor set to 58 per this file's own rule, minScore = floor(measured) - 1, which is the same arithmetic every sibling entry used: 57.03 to 56, 76.58 to 75, 95.65 to 94. Both halves moved together, because the ratchet guard asserts minScore equals its RATCHET_BASELINE entry and would reject them drifting apart. The spawn-free unit surface is vindicated by the clock: 57 seconds, against a 15-minute shard cap and a 9m46s frontmatter shard in the same run. That was the whole reason for creating tests/model-catalog.unit.test.cjs rather than pointing the shard at tests/model-resolver.test.cjs — #2790 recorded shards being CANCELLED at that cap when they targeted a runGsdTools-heavy integration file. The registry comment is rewritten rather than deleted. It previously warned that the floor was provisional and must not ship that way; leaving that text next to a measured floor would make the file lie in the other direction. It now records the measurement the way the sibling entries do, including that 59.62 sits below TARGET (80) and is therefore a ratchet candidate like planning-inspect at 56 — comfortably clear of its own floor with real room to grow. Raise it as the tests improve; never lower it. Worth stating plainly: 168 surviving mutants is not a clean bill of health. It is an honest floor for a module that had NO mutation coverage at all an hour ago, and it is now pinned so it cannot silently regress. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
3fd03bec4c |
fix(#3760): refuse a legacy-key migration into a non-object config section (#3767)
* test(#3760): failing-first regression for non-object config section Locks the contract from the issue's Expected section before any fix exists: a legacy-key section holding a string, number, boolean or array must be preserved verbatim, reported, and never persisted in an expanded form. Covers both blocks the issue names (branching_strategy -> git.*, sub_repos -> planning.*), the migrateOnDisk multiRepo branch that shares the shape, and the loader write paths that are what actually reach the user's config.json. Includes the negative-space cases that must keep hoisting ({} , null, absent section, canonical-nested-wins) and two fast-check properties. Refs #3760 * fix(#3760): refuse a legacy-key migration into a non-object config section normalizeLegacyKeys hoisted a legacy top-level key into its canonical nested section by spreading `result[section] ?? {}`. `??` guards only null and undefined, so a section holding a string was enumerated by index — `{...'main'}` is `{0:'m',1:'a',2:'i',3:'n'}` — while a number or boolean spread to `{}` and the value vanished. Because a fired block always pushed a Normalization, and every caller treats a non-empty normalizations array as 'config is dirty', that shape was written back to .planning/config.json and the original value became unrecoverable. Both blocks the issue names are fixed via one shared hoistLegacyKey helper, plus the two further sites that share the shape and are reachable from the same input: migrateOnDisk's multiRepo branch, and the loader's two `if (!planning) planning = {}` guards, where a non-empty string is truthy and the following assignment threw a strict-mode TypeError that the enclosing catch swallowed — discarding the user's entire config. A present non-object section now blocks its own migration. The section, the legacy key, and the file are left byte-identical; no Normalization is pushed, so nothing marks the config dirty; the refusal is reported in-band as `skipped[]` and out-of-band through the ADR-1411 warnUnusableInput seam (new frozen reason config_section_not_object). null and undefined keep their long-standing 'absent' meaning and still create the section. This is the nested-section analog of the ADR-227 shape check _readConfigFile already performs on the top-level document: valid JSON is not a config object. isConfigSection is exported and shared by both modules rather than copied. Fixes #3760 * fix(#3760): keep the multiRepo marker when planning cannot receive it Follow-up from the isolated adversarial review, and the same defect class as the two blocks the issue names — in the block it did not name. normalizeLegacyKeys block 3 deleted `multiRepo` and pushed a Normalization before anything consulted the planning section, deferring 'can this section receive sub_repos?' to the caller that runs filesystem detection. By then the marker was already gone and the config was already dirty, so with {"multiRepo":true,"planning":"docs"} the loader wrote the file back with multiRepo removed, the sub_repos injection silently no-opped against the string, and no diagnostic was emitted at all. migrateOnDisk warned for the same input; the ~30-caller loadConfig path did not. Section validity is knowable from the parsed config alone — detection is only needed for the VALUE, not for whether the destination can hold it. The refusal moves into block 3: the marker is kept, no Normalization is pushed, and a skipped entry is recorded, so all three callers inherit the preservation and the diagnostic together. The caller-side guards drop to pure narrowing. Also from review: skipped[] now reports sectionType ('string' | 'number' | 'boolean' | 'array') instead of sectionValue. migrateOnDisk's report is printed verbatim by `migrate-config`, and this module already masks config values on the set/unset output path; the type is the whole diagnostic and the value is still in the file. And `migrate-config --raw` no longer answers a refused migration with 'No legacy keys found — config is already canonical.' Legacy keys WERE found and declined, and the decline is the one thing only the user can fix by hand. Refs #3760 * fix(#3760): keep configuration.cjs dependency-free; emit from its callers The remote matrix caught a regression my own change introduced: adding `require('./unusable-input.cjs')` to configuration.cts broke the #3571 install-layout contract. `configuration.cjs` must load from a layout holding only itself plus bin/shared/*.manifest.json — the installer does not co-locate arbitrary siblings — so the new require failed at load time: Cannot find module './unusable-input.cjs' Require stack: - /tmp/gsd-3571-.../.codex/gsd-core/bin/lib/configuration.cjs pinned by 'co-located bin/shared manifests let configuration.cjs load without sdk/shared' in tests/install.test.cjs (3 failures). The contract is deliberate and the test is right, so the module goes back to zero sibling requires and the out-of-band diagnostic moves to the callers that already carry a dependency budget and hold the resolved path: cmdMigrateConfig (config.cts) and loadConfigResolved (config-loader.cts). normalizeLegacyKeys keeps reporting refusals in-band via skipped[], which is what lets it be pure and dependency-free at the same time. The emission-count and dedup assertions move to tests/config-loader.test.cjs, where the diagnostic now originates. A new assertion pins the inverse for the module itself — migrateOnDisk must emit ZERO diagnostics while still reporting skipped[] — so regrowing a sibling require fails a unit test instead of only the install suite. CONTEXT.md records why the emitter is the caller. Refs #3760 * chore(#3760): backfill changeset pr number to 3767 --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
b6977d9d11 |
fix(#3762): enforce docs/INVENTORY.md roster rows, backfill 32 gaps (#3766)
* test(#3762): failing-first roster gate for docs/INVENTORY.md rows
Anchors the human half of the inventory-drift rule: every entry in
docs/INVENTORY-MANIFEST.json must have a hand-written row in
docs/INVENTORY.md. Expected RED on this commit -- next carries 32
unrostered surfaces, which is the defect the gate exists to catch.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3762): enforce docs/INVENTORY.md roster rows, backfill 32 gaps
docs/INVENTORY.md calls itself the authoritative roster of every shipped
GSD surface, and CLAUDE.md's inventory-drift rule requires both a roster
row and a manifest regen. Only the manifest half was anchored, so a PR
could ship a surface, regenerate the manifest, omit the row, and stay
green -- as PR #3758 did with gsd-core/references/planner-coupling.md.
Adds the roster half to tests/inventory-manifest-sync.test.cjs, backed by
a pure matcher in tests/helpers/inventory-roster.cjs, and backfills the 32
surfaces already missing rows on next.
Fixes #3762
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(#3762): harden roster matcher against fenced blocks and trim exports
Skips fenced code regions when splitting level-2 sections so a documented
'## ' example inside a fence cannot truncate a family section (false red)
or contribute a phantom row (false pass); makes the heading pattern linear
rather than a backtracking lazy match; narrows the module surface to the
three names the gate consumes.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(#3762): close two false-RED gaps found in orthogonal review
Indented headings: CommonMark permits an ATX heading to carry 1-3 leading
spaces, and a ^##-anchored pattern read such a document as having no family
sections at all -- reporting all six missing, a structural red for zero real
drift. Reproduced, then fixed and pinned.
Link-wrapped cells: a row written as [`x.md`](../x.md) was not recognized,
though docs/INVENTORY.md already uses that form elsewhere. Unwrapping now
peels a whole-cell link and a whole-cell code span, and only layers that
wrap the cell entirely -- a file mentioned mid-prose is still not a row, so
the false red is not traded for a false pass.
Adds a second fast-check property over the commands source-link rule, the
one family whose matching rule differs from the other five.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* test(#3762): boundary trio for the fence-marker length limit
RULESET.TESTS.boundary-coverage — the matcher's only numeric limit is the
fence marker's {3,}. Exercises 2 (inline markup, swallows nothing), 3, and
4 characters, for both backtick and tilde delimiters.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* chore: remove stray pwned_cmdsub injection-test canary from the repo root
A zero-byte file committed by
|
||
|
Tom Boucher
|
9ade7926ca |
test(#3761): replace vacuous wave/files_modified gate with an anchored one (#3764)
The test 'planner has validation step or quality gate for wave/files_modified consistency' asserted a six-token disjunction over whole-file substrings, five of which occur ZERO times in agents/gsd-planner.md (validate_waves, wave_validation, same wave — the last killing the entire quality-gate arm, since that file has no quality_gate block). The sixth matched one incidental pseudocode comment, so deleting the wave-ordering gate outright left the suite green. Per RULESET.TESTS.delete-bad-tests (CONTEXT.md:595) the vacuous test is DELETED and replaced, not patched. The replacement anchors on the normative **Rule:** sentence inside <step name="assign_waves"> — scoped to that step rather than to the whole document, and requiring all four semantic clauses (wave scope, files_modified subject, prohibition, overlap predicate) to hold within a single sentence, so co-occurring words scattered across a paragraph do not pass. agents/gsd-planner.md is unchanged: the anchor already exists there and survives PR #3758's edits to the same region. Teeth proven on the remote runner: a control commit paired these tests with a deliberately gutted assign_waves step that retained the incidental phrase. The new anchored test failed while the sibling test using the old predicate's surviving arm stayed green — same input, opposite verdicts. Fixes #3761 Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
2f86278b5e |
fix(#3003): opt-in mechanism for intentional deletions in worktree.cleanup-wave (#3757)
* test(#3003): failing-first suite for declared deletions in cleanup-wave Binds the guard's opt-in before it exists, so the suite is RED against next. The rows that carry the weight are the over-authorization set: a directory declaration must not authorize its children, a glob declaration must authorize nothing, and a declaration must not act as a string prefix of another path. Each of those BLOCKS, and each would PASS under a prefix, glob, or startsWith matcher — which is how a path list quietly degrades into the boolean opt-in #3003 explicitly rejected. The glob row matters most: declaredScopePrefix already returns null ("matches everything") for a glob-leading pattern, correct for the advisory it serves and catastrophic for a gate. Also pinned: a failed deletion check blocks on its own reason rather than being filtered into a pass; the block detail names only the undeclared residue so the operator is not misdirected by paths that were fine; an entry with no declaration blocks exactly as before; junk and non-array declarations do not authorize; and a blocked entry still isolates rather than aborting the wave (#2852, which must stay fixed). Two advisory rows cover an interaction found while designing: git diff --name-only includes deleted paths, so without unioning the declaration into the #2596 scope check, authorizing a deletion would raise SCOPE_OUT_OF_DECLARED against the very path just authorized. A seeded property states the whole invariant the three over-authorization rows sample: a deletion merges iff its normalized path is in the declared set. * feat(#3003): declared deletions opt-in for the cleanup-wave guard The deletions guard blocked the merge-back of any executor branch whose diff removed a file, with no way to say a removal was intended. A plan that folded one test file into a sibling could not be merged by the tool meant to merge it, forcing a manual --no-ff outside the tool -- strictly less safe than what the guard protects against. A plan now declares removals in its own frontmatter (files_deleted), and that list rides the same path files_modified already travels: plan-document parse -> phase plan JSON -> the per-plan worktree gate -> record-agent/create --deletions -> declared_deletions on the manifest entry -> the guard. The guard blocks only the deletions NOT in that list. A path list rather than a boolean, per the pinned decision: a boolean disarms the guard for the whole entry, so an unexpected deletion riding along with a declared one would pass unnoticed. Matching is exact after the module's shared normalizer -- never a prefix, never a glob. Both would let one declaration authorize a whole set, which is the mass-deletion accident the guard exists to catch. That also means declaredScopePrefix is deliberately NOT reused here: it returns null ("matches everything") for a glob-leading pattern, which is right for the advisory it serves and would silently disarm a gate. The block detail now carries only the undeclared residue, so an operator is not sent looking at paths that were fine. A failed deletion check still blocks on its own reason and is never filtered into a pass. A blocked entry still isolates rather than aborting the wave (#2852). The #2596 scope advisory unions the declaration into its declared set -- git diff --name-only includes deleted paths, so without that, authorizing a deletion would immediately warn that the same path was out of declared scope. Optional and additive throughout: files_deleted is absent from PLAN_REQUIRED_FIELDS, a manifest entry without declared_deletions keeps the original unconditional block, and omitting --deletions leaves the on-disk entry shape untouched. Supersedes the spent #2856 emitted-drift ack entry for execute-phase.md, the same supersede that entry performed on #3370 and #3370 on #3324. * fix(#3003): wire --deletions on every dispatch surface, not just one Review found the feature inert on two of three dispatch paths. execute-phase.md (harness inline) passed --deletions, but the orchestrator-worktree path (executor-isolation-dispatch.md, worktree.create) and the Fleet-parallel batch path (capabilities/claude-orchestration/fragments/execute-wave-pre.md, worktree.record-agent) still passed only --files. A plan declaring files_deleted would have merged on one path and been blocked on the other two -- the exact bug #3003 exists to fix, left unfixed where most of the isolation actually runs. Worse, per-plan-worktree-gate.md already claimed --deletions was passed 'on the same worktree.record-agent / worktree.create calls', which was false for both untouched sites. A doc asserting coverage that does not exist is how a gap survives review. All four surfaces now pass the flag, verified by sweeping every .md under gsd-core/, capabilities/, commands/, skills/ and agents/ that invokes worktree.record-agent or worktree.create: each one that passes --files now also passes --deletions. The isolation-dispatch note explains why this flag, unlike --files, is not advisory -- omitting it does not skip a check, it blocks a merge the plan declared. Regenerates capability-registry.cjs, which the fragment edit made stale. Neither newly-grown file needs an emitted-drift ack: executor-isolation-dispatch.md sits under workflows/execute-phase/steps/ and execute-wave-pre.md under capabilities/, both outside currentSizes()'s non-recursive scan of gsd-core/workflows/ and agents/. * docs(#3003): document files_deleted where a plan author will actually find it The feature's entire user surface is one plan-frontmatter field, and the canonical reference for that frontmatter -- docs/reference/plan-md.md, the table that documents every other key -- never mentioned it. A field nobody can discover ships as a field nobody uses. Adds the files_deleted row and an example entry in all five locales (en, ja-JP, zh-CN, ko-KR, pt-BR), stating the property that makes the opt-in safe: matching is exact per path after separator normalization, with no globs and no directory prefixes, so a declaration can never authorize more than it literally lists, and omitting the field keeps the guard's original unconditional block. Also corrects two claims in the scope-conformance how-to that this change made false. Its opening paragraph described the recorded declared scope as files_modified alone; declared_deletions is now unioned into that comparison. Its "Renames are not detected specially" bullet asserted the deletions guard blocks any entry whose diff contains a deletion, full stop -- which was the whole point of #3003 and is no longer true. Reworked to say what now decides a rename's fate: declare the old path in files_deleted and both halves become ordinary paths for the advisory check, which is also why the old path needs no separate files_modified entry. Documentation that describes the pre-change behavior of the thing being changed is worse than no documentation, because a reader trusts it. * fix(#3003): close every review finding on the declared-deletions opt-in Two independent isolated reviewers, correctness and security. Neither found a blocker; both found real defects, and the directive treats a finding at any severity as blocking. All of them are fixed here. MAJOR -- the submodule worktree gate could not see a deletion-only plan. per-plan-worktree-gate.md intersected $SUBMODULE_PATHS against $PLAN_FILES alone, while $PLAN_DELETIONS was extracted and then never used. Before files_deleted existed, a path had to appear in files_modified to be planned at all, so the gate saw it; the new field plus the new docs telling authors a deleted path needs no files_modified entry opened a hole where a plan whose only submodule touch is a removal kept worktree isolation on -- the exact case #2772 disabled it for. Both channels now feed the intersection. Note the posture is deliberately the OPPOSITE of the cleanup-wave guard: there the channels stay apart because a deletion AUTHORIZATION must never be inferred; here they merge because a safety fallback must never MISS a touch. MAJOR -- same-wave conflict detection could not see a deletion. The planner's implicit-dependency rule compared files_modified only, so plan A editing src/x.ts and plan B declaring files_deleted: [src/x.ts] scored as conflict-free and ran in parallel: one branch removing what the other is writing, which is the sharpest conflict there is. Overlap is now computed across both channels. MINOR (both reviewers, one root cause) -- the advisory union gave one field two matching rules. declared_deletions was unioned into the scope list handed to planWaveScopeConformance, which reads it with prefix-and-glob semantics. So a field that is exact-match-only at the gate silently became wider at the advisory: ["*.md"], inert at the gate, yielded a null prefix meaning "matches everything" and muted the advisory completely, and ["src"] muted all of src/. The union also activated the advisory on plans that declared no modification scope at all, warning on every modified path. Replaced with subtraction from the findings, gated on files_modified alone. One field, one rule, everywhere. MINOR -- core.quotepath made the feature silently inert for non-ASCII paths. git emits "tests/\303\251.ts" C-escaped and quoted, which never equals the declared plain path, so a correctly declared deletion of tests/é.ts would block forever with nothing pointing at the encoding. Both diffs now pass -c core.quotepath=false. NIT -- flag() consumed a following flag as a value, so --deletions --files x swallowed --files and dropped both. Now treated as a missing declaration, which fails closed. Fixed at both call sites; the helper is duplicated verbatim in cmdWorktreeRecordAgent and cmdWorktreeCreate and leaving one would reintroduce it. TEST -- one test passed for the wrong reason. "a declared deletion is in scope for the advisory" asserted only that warnings omit the deleted path; under a full revert the entry blocks first, warnings come back empty, and the negative assertion passes anyway. It now asserts the entry actually merged, which is the load-bearing half. Four regressions added, one per fix above. Docs corrected rather than extended. The rename bullet in the scope-conformance how-to claimed a rename whose delete side is undeclared never reaches the advisory. Verified false: git's rename detection is on by default, so a pure rename is a single R entry that appears in no --diff-filter=D output and was never gated, before or after #3003. Only a rename that edits enough to fall below the similarity threshold decomposes into add+delete. The pre-existing sentence made the same wrong claim; this restates it correctly instead of sharpening the error. The localized plan-md.md reference edits are reverted: the PR template requires docs content added here to be English, and the translations already lag by three fields, so English-only is the repo's standing posture, not an oversight. Agent-file size caps respected: gsd-planner.md is XL-tier by bytes but carries a separate 49152-LF-CHAR cap asserted by four suites, so its edit is deliberately terse and lands at 49141 with 11 chars of headroom, with the rationale moved to docs/reference/plan-md.md, which has no cap. gsd-plan-checker.md lands at 49107 bytes, 45 under the LARGE cap. Both acks merged into the existing fragments that already name those paths, since two ack sources may never name the same path. * fix(#3003): decode git's path quoting instead of changing the git argv The previous commit's non-ASCII fix turned the remote suite red: 44 failures, 42 of them "unexpected git call: -c core.quotepath=false diff --diff-filter=D --name-only ...". The suite's git mocks match on exact argv, so adding two flags to the deletions diff and the advisory diff invalidated every existing fixture in tests/worktree-safety.test.cjs. Rewriting dozens of fixtures to accommodate one flag would be paying a large Hyrum's-law bill to fix a small defect. Both execGit calls are reverted to their original argv. The C-quoting is now decoded in normalizeScopePath instead, via a new decodeGitQuotedPath helper. That is the better fix on its own merits, not merely the cheaper one: the git argv is untouched so no fixture moves, the decode lands on the ONE normalizer already applied to both sides of the comparison so the declared and reported paths cannot disagree, and it holds regardless of the user's own core.quotepath setting rather than only when we remember to override it. A value not wrapped in a leading AND trailing quote is returned completely untouched, so the plain-ASCII path -- the overwhelmingly common case -- is byte-identical to before. Escapes decode to BYTES collected into a Buffer and UTF-8 decoded only at the end, because \303\251 is two bytes forming one character and decoding them separately yields mojibake. Malformed input never throws: a trailing lone backslash or a short octal escape degrades to the literal character, since one bad path must not take down a cleanup wave. Caught while reviewing the helper: the non-escape branch pushed a UTF-16 code unit rather than UTF-8 bytes. Git always escapes non-ASCII so its own output was fine, but this normalizer runs on the DECLARED side too, and an author may write a quoted path holding a literal é -- pushing 0xE9 alone is invalid UTF-8, so the declaration would decode to a replacement character and silently stop matching. That is precisely the failure this change removes, reintroduced on the other side of the comparison. Now converts whole code points, surrogate pairs intact. The other 2 failures: tests/parallel-dependent-plans.test.cjs pins the exact unbackticked substring "files_modified overlap" in gsd-planner.md, and rewording that comment to "declared-scope overlap" deleted it. The comment is restored verbatim and the files_deleted change rides in the pseudocode and the Rule sentence instead. Recorded in the ack fragment so the next contributor does not rediscover it the same way. Four regression tests cover the decode through the public cleanup-wave seam (the helper is module-private): a declared non-ASCII deletion merges against a C-quoted git report, the symmetric case where the DECLARATION is the quoted form, an undeclared non-ASCII deletion still blocks with the residue naming the decoded path an operator can act on, and a path merely containing a quote is left alone. Plain ASCII was already covered and is not duplicated. * fix(#3003): revert the leading-dash flag guard, the review nit was wrong The remote suite came back with 2 failures, down from 44, and both point at the same thing: tests/worktree-safety.test.cjs:7045 already pins the opposite contract, deliberately. test('a flag-shaped --files value is not re-parsed as a flag', ...) recordAgent(['--files', '--branch']) -> files_modified === ['--branch'] -> branch === 'worktree-agent-a1' ("the real --branch value must be untouched") So consuming the next argv element positionally, whatever its shape, is the tested intent of this parser, not an oversight. The security reviewer's nit claimed --deletions --files x would "swallow --files and drop both". It does not: each flag runs its own indexOf, so --deletions records the literal '--files' while --files independently still resolves to x. And that literal is a path git never reports as deleted, so it authorizes nothing -- already fail-closed with no guard at all. The guard bought no safety and silently changed --files behavior along the way, outside this issue's scope. Reverted at both call sites, which are byte-identical again, along with the test asserting the reverted behavior and the docs sentence describing it. The nit is recorded as REJECTED in the review artifact with the reasoning above, rather than as fixed -- a finding that turns out to be wrong should leave a trace of why, or the next reviewer files it again. docs/CLI-TOOLS.md now states the positional-read behavior plainly instead, so the next person meets it as documented intent rather than rediscovering it through a red suite. * chore(#3003): backfill changeset pr number to 3757 * test(#3003): cover parsePlanDocument's filesDeleted branch to clear the mutation gate CI's Stryker shard for plan-document failed at 73.28 against a break threshold of 75: 170 killed, 62 survived, 232 total. Eight of those survivors are the filesDeleted block this issue added to parsePlanDocument, which shipped with no direct coverage at all -- the field was exercised end to end through the cleanup-wave tests, but the parser itself was never called with a plan that declares it, so every mutant in the block lived. Four tests, each pinned to specific mutants rather than written for coverage percentage: - absent key yields exactly [] -- kills the array-literal seed (["Stryker was here"]) and the `fmDeleted = true` conditional, which would otherwise produce ["true"] - a scalar underscore `files_deleted:` wraps into a one-element array -- kills `fmDeleted = false`, the `&&` logical-operator swap, the `fm[""]` string mutation on the first operand, the emptied if-block, and the ternary's non-array branch - an array-valued hyphenated `files-deleted:` maps element-wise -- kills the `fm[""]` mutation on the SECOND operand (only reachable when the legacy hyphen alias is the one carrying the value) and the ternary's array branch - an empty list yields [] -- boundary case, and a genuinely distinct one from the absent key: [] is truthy in JS so it ENTERS the if, and only Array.isArray's true branch mapping over nothing produces the same [] Threshold arithmetic: 174 of 232 are needed for 75%, and these take it to about 178, so the shard clears with margin rather than landing on the line. Every expected value was confirmed by executing the built parser before being asserted, not inferred from reading the source. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
738f42f4fd |
feat(#2398): consensus gate for CYCLE_SUMMARY on multi-reviewer runs (#3755)
* test(#2398): failing-first suite for the CYCLE_SUMMARY consensus gate Binds the gate before it exists, so the suite is RED against next. The load-bearing rows are the two the closed PR #2417 did not have. The B2 regression row asserts a judgment-class lone HIGH counts WITHOUT corroboration when its raiser is unmarked — if anyone re-couples that class to corroboration, more reviewers again produce a weaker gate than one, which is what closed #2417. The parity row asserts every marker literal the gate names is one review-lane-runner actually emits, so the gate cannot key on a signal nothing produces; a mutation row and a seeded fast-check property prove that guard runs its failure branch rather than only reading a correct tree. Also pinned: gate position before Counting rules, the untouched CYCLE_SUMMARY line shape the orchestrator greps, fence balance, the single-reviewer no-op, classification by what a claim asserts rather than by citation presence, the all-marked fail-open, current_actionable staying out of scope, and the leading-marker requirement that stops a review which merely quotes a marker from suppressing its own findings. * feat(#2398): consensus gate for CYCLE_SUMMARY on multi-reviewer runs With review.reviewer_instances running several reviewer identities off one adapter, any single instance's fabricated HIGH could force a full replan cycle on its own. Across ~9 real cycles on two projects each of four instances fabricated at least once, and each was also the most accurate reviewer in some other cycle, so dropping to fewer reviewers trades away real signal. The gate engages only when 2+ reviewers actually ran, and weighs a lone HIGH by what the claim asserts rather than by whether anyone agreed with it. An existence claim -- a symbol, file, flag, commit or ID exists, is absent, or says something specific -- counts only if source-grounding confirms it or another reviewer raised the same concern. A judgment claim -- a design or correctness property -- counts unless that reviewer's own section opens with an evidence-quality discount marker the review lane already stamps ([reviewed-without-source-citations] #3194, [reviewed-without-repo-access] #2176, or a diff-only lane). That split is what resolves B2, the finding that closed PR #2417. B2 showed the approved wording made more reviewers produce a WEAKER gate than one: condition (a) pointed at the source-grounding pass, which verifies every symbol THE PLAN cites and never takes reviewer claims as input, so a genuine architectural HIGH that one reviewer caught and another missed was neither groundable nor corroborated and stopped gating. Judgment-class findings are therefore exempt from corroboration entirely -- reviewers catch materially different classes of issue, and demanding two of them independently raise the same architectural concern suppresses exactly what a multi-reviewer setup exists to surface. Guards on the gate itself: an all-marked cycle disengages it, so a cycle in which nothing was verified can never be counted as converged; the marker must OPEN a reviewer's section, so a review that merely quotes a marker does not suppress its own findings; a suppressed HIGH stays listed and tagged rather than dropped; current_actionable is untouched; and a single-reviewer run is unchanged. No new command, config key, or dependency -- the gate reads signals that already exist. The CYCLE_SUMMARY line shape the orchestrator greps is unchanged; only the integer it computes moves, and only for 2+ reviewers. Known limit, inherited rather than introduced: SOURCE_CITATION_RE checks citation presence, not resolution, which src/review-lane-runner.cts records as a deliberate #3194 scope boundary. A fabricated but plausible file:line still gates. Scope revised and re-approved on the issue before any code was written. * test(#2398): make marker parity behavioral, and stop overclaiming the gate Review found the parity tests were vacuous: they asserted a marker STRING appeared in review-lane-runner.cjs's source text, never requiring the module or calling the stampers, so they would pass even if stampUngroundedReview were broken or never invoked. They now invoke the real exported functions and assert what those functions PRODUCE — that an uncited review gains a leading marker blockquote, that a review carrying a file:line does not, that a self-reported blind review is stamped, and that stamping is idempotent. Removing the source read also removes an incidental no-source-grep evasion via a parameterized path. Review also found the changeset headline false for the class it matters most in. The discount markers detect 'cited nothing' and 'had no repo access'; they cannot detect 'drew a wrong conclusion from a real citation', so a judgment-class finding invented by an evidence-bearing reviewer still counts alone. That is the deliberate side of the tradeoff jags-faith named when closing #2417 — the alternative is requiring corroboration for design findings, which is B2 — but the changeset claimed lone hallucinations no longer force a cycle, full stop. Corrected there, and stated plainly in docs/COMMANDS.md and the design record. Also dropped the reviewer-instances.md entry from the emitted-drift ack: the growth ratchet's currentSizes() scans only gsd-core/workflows/ and agents/ (tests/helpers/emitted-runtime.cjs:916-929), so references/ is outside it and that entry acknowledged a delta the gate cannot see. * chore(#2398): backfill changeset pr number to 3755 --------- Co-authored-by: sim <sim@local> |
||
|
Behruz Nassre Esfahani
|
444069d601 |
fix(#3613): copy component dirs into the plugin-validate fixture (#3627)
* fix(#3613): copy component dirs into the plugin-validate fixture C2 builds its synthetic plugin root by symlinking commands/, hooks/ and skills/ into a temp dir. `claude plugin validate` (>=2.1.233) reads component directories without following symlinks and warns on each one, and `--strict` promotes a warning to a non-zero exit — so the assertion failed on how the fixture was built, not on the manifest under test. Copy the directories with fs.cpSync instead. The validated tree still holds only plugin.json and the three component directories, so nothing else from the repo root is placed where the validator can read it, and the #2665 CLI-config isolation is untouched. The construction helper is self-cleaning: its callers' try/finally only begins once it returns, so a throw partway through would strand a half-built root on disk. The pre-refactor code ran these same steps inside C2's own try, and that teardown guarantee is preserved rather than narrowed. Its cleanup is best-effort so a teardown error cannot replace the real construction error. Add C3, an unconditional tripwire asserting the fixture exposes real, non-symlinked component directories at any depth. C2 never runs in CI — no job under .github/workflows/ installs the `claude` binary — so a revert to symlinks would pass every CI lane and surface only as a red suite on contributor machines. C3 is not a substitute for C2's end-to-end check and cannot be shown red against this base, since the helper it calls arrives with it; C2 is the failing-first artifact. C3 enforces a symlink-free fixture throughout, deliberately stricter than the CLI's own boundary. Measured on 2.1.234, `plugin validate --strict` exits 1 for a symlinked component dir and for a symlink one level inside skills/, and 0 for two levels in or for symlinks under commands/ or hooks/. Encoding that external, undocumented line would be more fragile than a superset costing one walk over ~176 files. The walk uses an explicit stack rather than readdirSync's `recursive: true`, which follows directory symlinks — a link pointing outward would otherwise traverse an unrelated tree, or a cycle, before the assertion ran. Correct the Section C docstring, which claimed C2 provides defence-in-depth coverage it cannot provide in CI. Whether to provision the CLI in a CI job is a maintainer call and is left open. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3613): skip hooks/dist and .dist-staging when building the fixture The recursive copy added for #3613 races the hook builders. build-hooks.js writes atomically through a per-PID hooks/.dist-staging-<pid> and removes it when done, and nine test files invoke that script from their before() hooks, so a recursive walk can enumerate a staging directory and then lstat it after the owning process deleted it — the ENOENT that #3656 just fixed in the cold-tree fixture. Copy entry-by-entry and skip by NAME BEFORE anything stats it, reusing shouldCopyHookEntry from tests/helpers/cold-runtime-lib-fixture.cjs rather than re-deriving the rule. A filter applied after the stat would not close it. The predicate is already pinned including its over-match cases, which a loose startsWith('dist') would get wrong: dist-staging-no-dot and distant.js must both be kept. Excluding hooks/dist is independently right for this fixture — a real marketplace install contains neither dist nor a transient staging dir, the same reasoning that keeps the repo-root CLAUDE.md out of the validated tree. C2 still validates clean without it. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3613): validate agents/ too, and scope the hooks predicate Three review Minors. agents/ ships in package.json `files` and IS auto-validated by the CLI — verified: a frontmatter-less agents/*.md exits 1. Including it was pointless while the fixture symlinked, because the CLI read nothing through a symlink; now that the tree is real it is the last shipped component tree C2 could not see. Proven to buy coverage rather than bytes: planting a frontmatter-less agent now reds C2, which it could not do before this change. shouldCopyHookEntry is documented as a hooks/ entry filter, so it now runs only for hooks/. Applying it to the other trees was harmless today but silently encoded a hooks-shaped exclusion into them — a future commands/dist would have vanished from the validated tree with no signal. assert.deepEqual -> deepStrictEqual on the nested-symlink assertion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3613): scope the fixture to the three directories the issue names Review findings, all five. Major 1 — `agents/` dropped from COMPONENT_DIRS. The observation behind adding it was right (the CLI does auto-validate it; a frontmatter-less agents/*.md exits 1), but it is a NEW gate the issue does not ask for, on the largest of the trees, and since C2 never runs in CI it would be red only on contributor machines with `claude` installed — the same worst-of-both-states #3613 exists to remove. Worth having as its own issue, where "should CI provision the CLI" gets answered for it too. Major 2 — C3 no longer passes on a fixture that validates nothing. buildValidationPluginRoot() mkdir's every component dir before the entry loop, so a copy that stops happening leaves real, EMPTY directories and all three structural assertions still hold (an empty tree contains no symlinks). Adds a non-empty assertion plus a known entry per tree, so a partial copy is caught as well. Stubbing the copy now reds C3 with "commands/ is EMPTY"; dropping just commands/gsd reds it too. Neither did before. Minor 1 — the measured table was wrong, and the mistake was measuring an inert file. Re-measured on CLI 2.1.239, reproducing the review's 2.1.237 result: a symlinked skills/<name>/SKILL.md at depth 2 exits 1, while a stray symlinked *.md at the same depth exits 0. The boundary is not depth at all, it is whether the symlink is a file the CLI reads as a component. Table replaced with that. Minor 2 — the hooks name filter is now local instead of importing cold-runtime-lib-fixture.cjs's, whose docstring scopes it to the cold-tree fixture and which had exactly one caller. Two consumers across fixtures with different requirements and nothing asserting they stay compatible is how a later cold-tree change silently alters what this fixture validates. Nit 1 — cost figures re-measured: ~1.06 MB over 176 files, not 2.1 MB over 243. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> * fix(#3613): correct the hooks row and the count the agents/ removal left behind Three comment-prose items from review, no code change. N1 — the corrected table gained a new wrong row, erring unsafe. "symlink inside commands/ or hooks/ -> 0" is false for hooks/hooks.json, which is the single most likely thing anyone would symlink there. Re-measured on CLI 2.1.239, matching the review's 2.1.237 figures on every cell: symlink inside commands/ (a dir, or a component *.md) 0 stray symlinked dir or *.js inside hooks/ 0 symlinked hooks/hooks.json 1 hooks/hooks.json is now its own row, and is named in the C3 assertion message alongside SKILL.md — that message is what the next engineer actually reads. N2 — "the four component directories" was residue from the revision that also copied agents/; it survived the very edit that removed it, three lines above a sentence saying three. Now three. N3 — the promised agents/ follow-up is filed as #3751 and the comment cites it, rather than promising an issue that did not exist. It frames the real decision (should CI provision the claude CLI) rather than just asking for the directory back. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
0062f6d033 |
fix(#2845): stop the dimension parity guard counting a back-reference (#3752)
next went red on
|