4f6935e29bca40f904f8f73b506ffec4d1eded3a
798 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Tom Boucher
|
6a9babda69 |
chore(#2798): declare the eleven reviewer lanes as manifest data (#2837)
* chore(#2798): declare the eleven reviewer lanes as manifest data Phase 5a of epic #2782, delivering ADR-2782 D9 (roster half) and D3. - Five reviewers GSD never installs into become lane-only role:reviewer capabilities with no runtime body, no runtimeCompat and no install surface: gemini, coderabbit, ollama, lm-studio, llama-cpp. Before this they had no descriptor at all and lived as a hardcoded NON_RUNTIME_REVIEWER_SLUGS tail, which is now deleted outright. - The six hosts that are ALSO reviewers gain a reviewer body alongside their runtime body. Their runtime bodies are byte-identical to next -- verified per capability against the git blob, not asserted -- so no install behaviour moves. - KNOWN_REVIEWER_SLUGS derives from declared bodies via an exported deriveReviewerSlugs(registry). hostBehaviors.reviewerCli survives as a derived legacy alias for one release; where a capability carries both, the body wins and the slug appears once. Alias removal is Phase 7 (#2801). THE KEYSTONE: the roster is the SAME ELEVEN SLUGS as before -- antigravity, claude, coderabbit, codex, cursor, gemini, llama_cpp, lm_studio, ollama, opencode, qwen. This phase changes HOW the roster is derived, not WHO is in it, and the test asserts that literal list rather than a count. kimi-code is deliberately NOT declared here. It is net-new with no invoke_reviewers leg, so declaring it now would make it selectable but not invocable -- present in --all, selected, emitting an empty section for the whole 5a-to-5b window -- and would break Phase 1's parity assertion. It lands in 5b alongside the iteration that can run it. Legacy kimi (the Python CLI) is not a reviewer at all and gains nothing. The highest-value test is declaredManifestLanesMatchThePhase1Descriptor: it deep-compares all eleven declared bodies against REVIEWER_LANES field-by-field, including probe and invoke sub-fields. All eleven are byte-identical, key order included. The epic's premise is that the manifest and the core descriptor describe the same lane with NO translation layer, and Phase 2's review already caught one divergence that every other test missed. Two ADR corrections folded in, as Phases 1-3 each did: 1. PHASE ORDER. The ADR runs Phase 4 (federated config) before 5a and #2798 claims a dependency on 4. That is inverted and makes Phase 4 unsatisfiable: D9 assigns review.<host>_host to lane capabilities that do not exist until THIS phase creates them, and a federated config slice must live inside capabilities/<id>/capability.json. Real graph: Phase 2 -> 5a -> 4. 2. #2798's INVENTORY acceptance item is vacuous. The inventory catalogs bin/lib/*.cjs modules, not capability directories -- antigravity, opencode and qwen appear zero times in it -- and gen-inventory-manifest --check passes with the five new dirs and no edit. Also corrected a stale line in Phase 2's own ADR amendment: it recorded the slug pattern as /^[a-z][a-z0-9_-]*$/, but Phase 2's security review widened the shipped pattern to /^[a-z0-9][a-z0-9_-]*$/ to match Phase 1's exported LANE_SLUG_RE. The prose had not followed the code. Closes #2798 * fix(#2798): catalogue reviewer capabilities in the generated matrix The capability matrix rendered exactly two tables, feature and runtime, via renderTable(caps, role) filtering on c.role === role. ADR-2782 D3 added a THIRD role, so every role:"reviewer" capability was silently dropped from the first-party catalogue. The drift guard did not catch it, and could not: --check compares generated output against the committed file, and both omitted the five lanes identically, so it reported "up to date" while five shipped capabilities were invisible in the one document that is supposed to list what ships. A guard blind to an entire role is not guarding. This phase is what exposed it -- it ships the first role:"reviewer" capabilities -- so it is fixed here rather than deferred (CLAUDE.md: a defect found while working is fixed in the current change, which overrides one-concern-per-PR). Verified red-before-green: with a lane row deleted from the matrix, --check now exits 1; restored, it exits 0. Before this fix the lanes were absent entirely, so there was nothing for the guard to compare. Phase 6 (#2800) still owns enriching the matrix with lane-specific detail (slug/flag/transport columns) and the locale parity gate. This is the narrower fix: the capabilities APPEAR at all. * fix(#2798): close two hardening gaps and record three limits durably Isolated security review (5 targets, no blockers) reproduced two gaps in the new deriveReviewerSlugs. Both are unreachable through the checked-in registry -- it is generated, JSON-sourced and code-reviewed -- but the function is EXPORTED for reuse and carries no other validation, so it must not depend on its caller. - A whitespace-only slug passed the length>0 test verbatim and occupied a roster entry it could never match. Slugs are now trimmed before the emptiness test. A blank body correctly falls through to the legacy alias rather than DROPPING the lane, which would have been worse than the blank slug. - KNOWN_REVIEWER_SLUGS is computed at require() time, so an uncaught throw there breaks import for EVERY consumer rather than degrading selection. It is now guarded, yielding an empty roster on a malformed registry. That is a visible degradation, not a silent one: under D4 an explicitly requested reviewer that is unavailable is an ERROR, so /gsd:review --claude against an empty roster fails loudly. This also removes an asymmetry -- the sibling capability-trust module documents its collectors as TOTAL and wraps them for exactly this reason. Also records three findings that previously existed ONLY in squash-merged PR bodies, which is not a durable record: - ADR-2782 D5 gains an implementation note explaining why the resolved host is deliberately EXCLUDED from the disclosure signature. Rule 1 says consent binds the resolved host; the loader has no config resolver, so folding it in would make the loader and lifecycle compute different signatures for one manifest and re-prompt forever. The binding is split: signature covers the SHA-pinned manifest fields, the consent record stores the resolved host, and Phase 5b re-resolves at invocation -- which is where rule 4 already puts the check. A reader comparing rule 1 to the code would otherwise conclude it is unimplemented. - CONTEXT.md's capability-trust entry still described THREE executable surfaces. Phase 3 added the fourth and made that false; corrected here, since it is drift this epic introduced rather than Phase 6's new-glossary-term work. - stableJson documents the NaN/Infinity/undefined -> null signature collision and why it is unreachable (JSON grammar has no such literal, so JSON.parse throws first). Reachability rests entirely on the ingest path staying JSON.parse-only, so the note lives where someone would break it. * chore(#2798): backfill changeset pr number to 2837 |
||
|
Tom Boucher
|
982e83f0f7 |
docs(#2705): single-source the legacy ADR range; classify 0174/0656 as mis-padded modern (#2836)
* test(#2705): failing-first regression for single-sourced ADR legacy range * docs(#2705): single-source the legacy ADR range; classify 0174/0656 as mis-padded modern * fix(#2705): align README prose with test vocabulary (mis-padded modern; backtick-tolerant range regex) (review) * docs(changeset): #2705 single-source legacy ADR range * docs(changeset): backfill #2705 PR number to 2836 * chore: trigger clean CI run (prior run cancelled by rapid backfill push) |
||
|
Tom Boucher
|
69dbf28ca7 |
feat(#2796): reviewer lane as a fourth trust-disclosure class (#2826)
* feat(#2796): reviewer lane as a fourth trust-disclosure class Phase 3 of epic #2782, delivering ADR-2782 D5. A reviewer lane is piped the plan text, requirements, research findings and CONTEXT.md decisions, and its output is read back into REVIEWS.md -- an egress channel for the most sensitive artifacts GSD produces. Making lanes pluggable WITHOUT a disclosure class would open a data-exfiltration path behind a manifest field, which is why this gates the feature rather than following it. - discloseExecutableSurfaces was cyclomatic 51 / cognitive 99 / 110 lines with risk_level critical. Rather than grow it, it is now a short orchestrator over four extracted collectors (hooks, commands, mcp -- behaviour-preserving -- plus the new lane collector), each independently testable. That is also what makes the 80% mutation threshold survivable: 51 branches in one function cannot be mutation-covered by whole-function tests. - A spawn lane discloses its binary AND its full declared args, in rendered and raw form. Binary-only disclosure would be insufficient and not hypothetically: a lane declaring python3 with innocuous args could later change them to ['-c', '<program>'] without the binary changing. That is the bug class #1459 already fixed for MCP servers. - An openai-http lane has no binary, so it discloses the destination host and the config key naming it. A localhost destination is disclosed and distinguished from a remote one. Both forms name the egress payload classes. THE CONSTRAINT THAT SHAPED THE DESIGN: the lane element is appended to the disclosure signature ONLY when at least one lane is declared. signatureForManifest is the consent key both the loader and the lifecycle compare, so appending unconditionally would have changed every installed capability's signature and re-prompted every user for every capability on their next upgrade -- for a feature they do not use. Two pre-change goldens are asserted byte-for-byte as the tripwire. The resolved host is deliberately NOT in the signature. The loader has no config resolver, so including it would make the loader and the lifecycle compute different signatures for the same manifest and produce a permanent false-mismatch loop. It is disclosed and recorded instead; Phase 5b re-resolves and compares at invocation, which is D5 rule 4's own placement. reviewsSection and timeoutFloorMs are also excluded from the signature: a cosmetic change must not force re-consent, because a prompt carrying no security information is how users learn to click through. A lane's binary is NOT existence-checked against the staged bundle. It is a PATH tool, never a bundle artifact; treating it like a hook script would add every lane to missingArtifacts and block every lane install. Two defects fixed beyond the fourth class: - isLocalHostValue mis-parsed a scheme-less host: new URL('localhost:1234') does NOT throw, it reads 'localhost' as the URL scheme and yields an empty hostname, so a bare host:port would have been reported as non-local. Now falls back on an empty hostname rather than only on a caught throw. - The orchestrator's safeCollect closes a PRE-EXISTING totality gap in the other three classes: a null manifest, or one with a throwing getter or Proxy trap, previously threw out of disclosure -- which runs on an UNVALIDATED manifest at install time. No well-formed input changes; all 51 existing trust tests pass. Closes #2796 * fix(#2796): close four disclosure gaps found by the isolated security review All four were REPRODUCED by execution against the shipped module, and all four passed the existing 41-test suite while live -- each exists because the matrix did not think to ask. B (MEDIUM, reachable via plain JSON). Non-string argv members were folded into the consent SIGNATURE but dropped from the human-facing text, because the summary rendered the string-filtered args rather than the raw declared array. A manifest declaring args ['--json', 7, {mode:'exfiltrate-everything'}, true] printed as '--json' alone -- the host still receives the rest, so the user consented to a surface never shown. That directly contradicts this design's own Kerckhoffs claim that nothing about a lane is hidden. The summary now renders the raw array, with non-strings shown in a visible form, and never throws on a circular or BigInt member. F (MEDIUM, reachable). The [local] flag is design-load-bearing, and it was dropped for every loopback form except the dotted quad and the bare hostname. Bracketed IPv6 was mangled by splitting on the address's own colons ([::1]:8080 became '['), and legacy IPv4 encodings were not recognised at all. A browser, curl and the OS resolver all treat 127.1, 2130706433, 0x7f000001 and 0177.0.0.1 as loopback. isLocalHostValue now handles bracketed and bare IPv6, IPv4-mapped loopback, and inet_aton shorthand/decimal/hex/octal. The dangerous direction was already clean and is now pinned by tests: localhost.evil.com, http://user@localhost@evil.com and friends stay REMOTE. C (LOW). An empty reviewer body flipped hasExecutable true and perturbed the disclosure signature, producing a re-consent prompt whose only content was '(no binary declared)'. A prompt carrying no security information is the click-through-training harm this design explicitly refuses for reviewsSection and timeoutFloorMs; refusing it there and permitting it here was inconsistent. A body declaring nothing recognised is no longer a lane. The test is deliberately broad -- any ONE recognised field suffices -- because requiring specifically a binary, or specifically a slug, would let a lane declaring only the other slip through unconsented, which is the far worse failure. Pinned in both directions. D (LOW-MEDIUM). Disclosure runs BEFORE validation, so a mis-cased or unrecognised transport reaches this code. Keying on an exact string sent a lane that plainly declares a hostConfigKey down the spawn branch, printing '(no binary declared)' for a lane egressing to a live remote host, and left its resolvedHost blank -- which reads as 'no destination', the precise thing the design forbids. Both the collector and the summary now branch on the declared SHAPE, so such a lane discloses its key and either a resolved host or the explicit unresolved marker. Two further findings were reproduced but confirmed NOT reachable through the real pipeline and are recorded as known limits rather than fixed: a selective-throw Proxy blanking a whole lane, and NaN/Infinity/undefined colliding to 'null' in a signature. Every production manifest reaches disclosure through readManifestBounded's strict JSON.parse, which cannot produce a Proxy, a getter, a BigInt, a circular reference, NaN or Infinity. The 0/-0 sub-case IS reachable via valid JSON but is inert -- String(0) === String(-0), so a spawned process receives identical argv. 9 regression tests added (50 total in this file, up from 41). * chore(#2796): backfill changeset pr number to 2826 |
||
|
Tom Boucher
|
46e84d5e39 |
chore(#2795): reviewer manifest body + registry harvest, validation, forward-compat (#2823)
* chore(#2795): reviewer manifest body + registry harvest, validation, forward-compat Phase 2 of epic #2782 under ADR-2782. Delivers D1, D2, D3, D7, D8 and the four Phase-1 vocabulary amendments (A1-A4). - VALID_ROLES gains "reviewer"; the reviewer body is admissible on role:runtime (a host that is also a reviewer keeps one manifest) and on the new role:reviewer (a lane that is not an install target). A reviewer body on role:feature is an error: declaring one is an assertion of lane-ness. - validateReviewerBody + validateLaneProbe + validateLaneInvoke: nine closed enums, a transport discriminator selecting mutually-exclusive invoke sub-shapes, bounded probes (D7), and outputArg required-iff outputChannel is file-arg and forbidden otherwise. - Absent-safe (D4.1): only `undefined` is absent. null/{}/[]/false/0 are malformed assertions and error. 39 of 39 shipped capabilities depend on this. - collectReviewerWarnings: an unknown field inside the body warns, never errors, so a forward-built manifest degrades visibly instead of failing the build. - D8 uniqueness (slug / flags / reviewsSection) lives in validateCrossCapability, so it is enforced at build time over first-party AND at load time over the merged first-party union overlay set, with first-party-wins falling out of the loader's existing ordering rather than a new provenance check. - Config harvest widened past the role==="feature" branch in both the generator and the ownership loop. The often-cited cause of the stranded reviewer config keys -- the runtime body forbidding feature-only fields -- is not the mechanism: `config` is not in FEATURE_FIELDS_FORBIDDEN_ON_RUNTIME. The cause is two harvest sites that never read it. Verified inert: no shipped capability declares config on a non-feature role, and the generated registry is unchanged. Three ADR corrections are folded in (Phase 1 set the precedent of amending in-phase): the misattributed config-stranding cause, D3's inverted profile-membership claim, and the specified capability folder names for lm_studio / llama_cpp, which would have failed the id kebab-case invariant. Closes #2795 * chore(#2795): collapse nine enum checks into one validateEnumField helper Standards-axis review findings, both applied: - Duplicated Code: the enum-membership + enumerate-the-members error shape repeated near-verbatim at nine call sites. Routing them through one helper makes "the error names its valid members" structural rather than a convention repeated nine times, where it would drift. That property is load-bearing until Phase 6 ships the prose reference, because these errors are currently the only documentation of the vocabulary. - Speculative Generality: the isReservedName() pre-check on every enum field was inert. A VALID_* set never contains __proto__/constructor/prototype, so membership alone already rejects them, and "must be one of: ..." is more actionable than "is a reserved name". The literal guards remain where they do real work -- the key-derived write sites in the registry generator and the claim() accumulator. The reserved-name test now asserts all three reserved names are rejected via enum membership, rather than one name via a branch that no longer exists. * fix(#2795): align lane slug grammar with Phase 1 and wire the load-time diagnostic channel Spec-axis review findings, both applied. (1) The slug grammar had diverged from Phase 1's core descriptor. Phase 1 exports LANE_SLUG_RE = /^[a-z0-9][a-z0-9_-]*$/ (leading digit permitted); the manifest validator required a leading LETTER. A slug the core descriptor accepts -- a model-named lane such as 4o-mini -- would have been rejected by the manifest validator, which is exactly the translation layer ADR-2782 exists to delete. It was inert only because all eleven shipped slugs begin with a letter, so nothing else would have caught it until a third party shipped such a lane. The grammar cannot be reduced to one definition: Phase 1's module compiles to gitignored build output, and capability-validator.cjs is a committed plain .cjs that must load on a fresh worktree before build:lib has ever run. That makes this the repo's DEFECT.GENERATIVE-FIX class, so the duplication now carries a parity assertion -- laneSlugGrammarMatchesPhase1Descriptor -- which compares both the source grammar and the accept/reject verdict for a shared input set, and fails if the two ever drift again. (2) collectReviewerWarnings had exactly one caller: the build-time generator, which only ever sees first-party in-repo manifests. The real third-party overlay loader never called it and ValidatorModule did not declare it, so ADR-2782 D4.3 -- an unknown field inside a reviewer body is ignored WITH A WARNING -- surfaced nowhere at runtime, which is precisely the case D4.3 exists for. loadRegistry now collects those diagnostics on the accept path, behind a typeof-guard (an older built validator without the function still loads) and a try/catch (ADR-1244 D2's never-crash contract outranks a diagnostic). They land in a NEW OverlayMeta.diagnostics field rather than OverlayMeta.warnings, because warnings records capabilities that were SKIPPED and a consumer treating every entry as inactive would mislabel a working lane. Covered end-to-end by overlayLaneWithUnknownFieldIsAcceptedAndDiagnosed, which drives a real global-scope overlay through loadRegistry and asserts the lane is accepted, produces no skip warning, and yields a diagnostic naming the field. * fix(#2795): make the reviewer validators honour their documented totality contract Isolated adversarial review finding (MAJOR), reproduced by execution. validateReviewerBody documents itself as "TOTAL: returns an array of error strings for ANY input and never throws", and the overlay loader contracts every validator to RETURN errors -- #1461 OVL-1 records a validator that THREW and would have crashed every consumer of loadRegistry. The contract was false at ten sites: JSON.stringify throws on a BigInt and on a circular structure, and every enum/scalar rejection path interpolated the rejected value into its own rejection message. Reading the value could throw too, before any message was built, via a throwing getter or a Proxy get/ownKeys trap. Not reachable through a capability.json today -- every ingestion path is a plain JSON.parse of file text, which cannot express any of those shapes. Fixed anyway: the contract is stated on an EXPORTED function, and a caller must not have to re-derive today's reachability analysis to know whether it holds. Two layers, because serialization safety alone is insufficient: - describeValue() renders any value without throwing, so messages stay useful (a BigInt now reads "got: 10n" rather than degrading to a generic fallback). - A structural try/catch around validateReviewerBody and collectReviewerWarnings makes the guarantee absolute rather than argued, covering read-time throws that fire before any message exists. The same review found the property test guarding this contract was FALSE CONFIDENCE, which is the more important half. fc.anything() at default constraints emits no BigInt, no circular reference, no getter and no Proxy -- 20,000 sampled draws produced zero of each -- so the test was named for a contract its generator could not reach. Even withBigInt is insufficient under whole-value fuzzing, because the defect needs an exotic value in a specifically NAMED field and random key names never land on one. The property is now field-targeted across all twelve reviewer fields, and a companion test enumerates the shapes fast-check cannot generate at all (BigInt, circular, throwing getter, symbol, function, null-prototype) across scalar positions, array-element positions, and read-time traps. Verified red-before-green: with the fix reverted both property tests fail; with it restored all 119 pass. * chore(#2795): backfill changeset pr number to 2823 |
||
|
Tom Boucher
|
8b44a0da43 |
chore(#2794): single-source the reviewer invocation contract + parity assertion (#2820)
* chore(#2794): single-source the reviewer invocation contract Phase 1 of epic #2782 (ADR-2782). Introduces one core descriptor table as the declared contract for all 11 cross-AI reviewer lanes, and the DEFECT.GENERATIVE-FIX parity assertion the roster has never had. The lane contract lived in three unrelated surfaces — the roster, ~640 lines of hand-authored per-CLI bash in invoke_reviewers, and the write_reviews section headings — so cross-cutting fixes landed per-leg (#2494 and #2605 were the same empty-output defect filed twice). - src/review-lane-descriptor.cts: frozen table declaring per lane the slug, flags, probe, invoke shape, timeout floor, empty-output policy, REVIEWS.md section, evidence class, required binaries, prompt-budget key and handler. Field names track ADR-2782 D1/D2/D6/D7 verbatim so Phase 2 harvests the shape with no translation layer. It declares; it does not execute — invoke_reviewers iterates in Phase 5b. - checkReviewerLaneParity: bidirectional parity across descriptor, roster, invoke_reviewers legs and write_reviews sections. Forward-only would miss the failure it exists to catch (#2718 added a leg, #2781 was the drift). ADR-1517 instance headings are exempt per D8. - Legs carry an explicit <!-- reviewer-lane: slug --> marker; five non-lane bold labels share the bold-then-fence shape a heuristic matcher would key on. - ADR-2782 D4: an explicitly-flagged reviewer that cannot run is now an error in both the core module and the workflow prose that mirrors it. A code-only change would be unobservable — the module has no production caller; the workflow narrates the policy. Discovery paths (--all, review.default_reviewers) stay lenient. - Fixes the qwen leg, the last one discarding stderr to /dev/null. Two ADR-2782 D2 vocabulary widenings were forced by surveying the shipped legs: promptChannel 'none' (CodeRabbit is fed no prompt) and outputChannel 'file-arg' (Codex writes via -o and discards stdout, #1698). Both are additive and closed; Phase 2 owns the validator. Closes #2690 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2794): make the parity checker total and pin the lane slug grammar Findings from the orthogonal review passes. Spec axis — the module claimed its vocabulary tracked ADR-2782 D1/D2 "verbatim" while diverging in three undisclosed ways, which is the translation layer Phase 2 was supposed to be spared: - `transport` moves from `invoke.transport` to the LANE level, a sibling of `probe`/`invoke`, exactly as D1's manifest example places it. The nested form read better as a TS discriminated union; the union is now discriminated at the lane level instead, which costs nothing. - The header and the CONTEXT.md glossary now enumerate all FOUR widenings (adding `outputArg` and `flags[]`), not two. Standards axis — CLAUDE.md requires a fast-check property test for a parser, and `checkReviewerLaneParity` parses markdown for markers and headings. Adding one found two real defects that the hand-written matrix missed: - NOT TOTAL: a malformed descriptor entry threw on `lane.flags` iteration, contradicting the module's own "never throws" claim. Every field is now narrowed from `unknown` at the trust boundary and reported as MALFORMED_LANE / INVALID_SLUG. This matters because Phase 2 feeds this function third-party overlay data, and a parity gate that crashes is indistinguishable from one never run. - SILENT GRAMMAR MISMATCH: LEG_MARKER_RE captures only [a-z0-9_-], so a slug outside that class was unmatchable — its marker could be present and correct and the scan would still report LEG_MARKER_MISSING forever. LANE_SLUG_RE now pins the grammar and a violating slug is reported INVALID_SLUG. A loud named violation beats a silent miss. Generators are document-shaped, not writer-seeded (CONTRIBUTING #2371): seeding from the module's own matchers could only produce documents those matchers already recognize. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2794): register the new bin/lib module in the ESLint ignore list The remote runner caught this; lint:ci did not, because the invariant lives in the test suite rather than the lint chain: tests/repo-invariants.test.cjs "each bin/lib/*.cjs is linted xor ignored according to migration state" -> tsc-generated bin/lib modules not yet added to ESLint ignore list: review-lane-descriptor.cjs Adding a src/*.cts module ripples to six surfaces (.gitignore, the ESLint ignore list, docs/INVENTORY-MANIFEST.json, the CONTEXT.md glossary, the capability/inventory manifests, and any size baseline). The other five were covered; this was the miss. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2794): amend ADR-2782 D1/D2/D8 with the vocabulary Phase 1 surfaced Building the Phase 1 descriptor table against all eleven shipped legs is the first time every lane's contract was written in one place, and it surfaced four cases the ADR's original survey did not cover. Amending the design lock rather than diverging from it, so Phase 2 (#2795) implements the manifest validator against the amended vocabulary instead of rediscovering the gaps. All four are additive widenings of closed enums; no decision reverses: - D2 promptChannel gains `none` — coderabbit is fed no prompt at all, it reviews the working-tree diff. - D2 outputChannel gains `file-arg` — the ADR called a file-writing lane a shape a real CLI *could* take; codex already is one, writing via -o/--output-last-message and discarding stdout (#1698). - D2 gains `outputArg`, required iff file-arg — knowing the review lands in a file is useless without the argument naming it. - D1 `flag` becomes `flags[]` and D8's uniqueness flattens across lanes — antigravity is selected by both --antigravity and --agy, which a single-valued field cannot express. This is the same evidence path that produced the openai-http transport: the vocabulary widens on a lane that exists, under review, never on speculation. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2794): backfill changeset pr number to 2820 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
9624167eec |
fix(#2810): accept the documented effortSurface axis on EoS registry entries (#2813)
* fix(#2810): accept the documented effortSurface axis on EoS registry entries The EoS registry schema required an exact eight-key `interactions.axes` object, while `docs/registries/README.md` and `CONTEXT.md` both documented nine keys including `effortSurface`. An entry that faithfully mirrored its upstream descriptor's `effortSurface` key was rejected outright. `effortSurface` reached the runtime-descriptor vocabulary through ADR-1239 amendment #2481 (`HOST_INTEGRATION_AXES`), but the registry's hand-maintained copy of that vocabulary never picked it up. The runtime-descriptor surface is guarded by tests/host-integration-validator-parity.test.cjs; the registry copy had no equivalent guard, which is what let the two drift. Accept `effortSurface` as an OPTIONAL ninth axis validated against the canonical ['argv','none'] rather than a required one: registry entries mirror their upstream registry/eos-entry.json byte-for-byte, so requiring it would retroactively invalidate every entry published before the amendment. Adds tests/registry-axes-parity.test.cjs, which asserts that every key shared between the registry vocabulary and HOST_INTEGRATION_AXES has an identical enum array, plus limit-1/limit/limit+1 boundary coverage on the axes key set. Closes #2810 * test(#2810): fail when a canonical axis is added but never mirrored The enum-equality assertion compares only keys the registry and HOST_INTEGRATION_AXES already share, so it is blind to the exact drift that produced #2810: a new canonical axis appears and the registry copy is never told. Verified by simulation — mutating an enum is caught, adding a new canonical key is not. Assert instead that every HOST_INTEGRATION_AXES key is either modeled by the registry or named in an explicit NOT_MODELLED allowlist (subagentToolkit and isolation, both dispatch sub-fields the registry collapses into its free-form dispatch summary). Adding a canonical axis now fails until someone decides which bucket it belongs in. The allowlist is itself guarded against going stale. Refs #2810 * fix(#2810): harden the axis value lookup with the CodeQL barrier pattern Both orthogonal reviews flagged the same line: `AXES[key] !== undefined` is not an own-property test, and the bracket reads are shaped like a prototype-pollution sink even though the unknown-key gate above provably makes them unreachable. Switch the presence test to `Object.hasOwn` and add the repo's inline literal guards (`capability-state.cts:146-155`, "Prototype-pollution guard (inline literal, CodeQL barrier)"), which CodeQL can follow where it cannot follow the `.includes()` filter that actually does the work. Behavior is unchanged — re-verified all five axes key-count shapes plus a genuine own `__proto__` property built through JSON.parse (the shape a third-party registry PR would submit): it is rejected as an unknown key and Object.prototype is untouched. Refs #2810 * chore(#2810): backfill changeset PR number |
||
|
Tom Boucher
|
7f13ee5373 |
enhance(#2793): ADR-2782 — reviewer lane becomes a declared capability surface (#2809)
* docs(#2793): add ADR-2782 — reviewer lane capability surface Design lock for epic #2782. Declares a reviewer lane as capability data rather than a core patch across three unrelated surfaces. Key decisions: - D2 transport discriminator (spawn | openai-http) — a survey of all twelve lanes found three that are HTTP endpoints with no binary, which invalidated the single-invoke-shape draft. - D4 the reviewer body is optional and absent-safe at every layer. - D5 a fourth executable-surface disclosure class covering the lane binary or host AND its egress payload classes. - D6 handler is a closed first-party enum, upholding ADR-1016; the consequence — third-party lanes are data-only — is stated plainly. - D7 probe kinds wider than existence, and every probe bounded. Amends ADR-857, ADR-894, ADR-1016, ADR-1244. Also records the D7/D8-extended-by-ADR-1244 marker on ADR-857 that ADR-1244 D8 promised but never added. Closes #2793 * docs(#2793): address orthogonal review findings on ADR-2782 Two blockers from the isolated adversarial pass: - D5 disclosed the spawn binary but not its args, reopening the #1459 bug class already fixed for MCP servers (binary python3 + args -c <program>). args are now disclosed and signature-bound. - hostConfigKey resolves from .planning/config.json, which is mutable after consent with no integrity check, so a lane consented against localhost could be silently redirected to a remote host by an ordinary PR. The resolved host is now consent-bound and re-verified on the invocation path; a mismatch blocks the lane. Majors and spec gaps: - D4 gains an explicit-selection carve-out. Absent-safe governs discovery, never a lane the user named; the current selector records that as info, which Phase 1 now corrects. - D4 gains a warning delivery channel. - D6 enumerates the handler closed-enum members; a closed enum whose membership is left to the implementing phase is not closed. - D6 records aider and plandex as concrete lanes the vocabulary cannot express, rather than claiming sufficiency it did not verify. - D2 gains evidenceClass, requiresBinaries, promptBudgetKey for per-lane divergence that was only prose, and motivates the one-member outputChannel enum. - reviewer.requires renamed requiresBinaries — it collided with the envelope requires (capability deps) at a different nesting depth. - Antigravity two-level timeout: Context cited it then dropped it; now explicitly delegated to the handler. - D9 gains a per-key ownership table, including three keys that stay central because they are policy across lanes, not lane properties. - Phase table maps every decision D1-D9 to a delivering phase; D6 handler modules and D5 invocation-time re-verification were previously unclaimed. - American English per house style. |
||
|
clezcoding
|
84bfef0818 |
docs(#2533): refresh gsd-cursor EoS metadata (#2792)
Co-authored-by: clezcoding <clezcoding@users.noreply.github.com> |
||
|
Tom Boucher
|
1e3c995e6f |
fix(#2789): scope the emitted-drift ack to the diff that introduced it (#2803)
* fix(#2789): scope the emitted-drift ack to the diff that introduced it Every input to `diffEmitted` is base-relative -- `baseline` vs `current`, `changedPaths` from `git diff base...HEAD` -- except the ack set, which was read absolutely, from the working tree only. A differential machine consulting a non-differential input. So `staleAcks` asks exactly one question, "did a delta consume you?", and that cannot distinguish an ack that never explained anything (an authoring mistake) from one whose ripple is now absorbed into the base (the ack's SUCCESS condition). After merge an ack is in the second state but reports as the first. The trigger is ordinary. Actions sets GITHUB_BASE_REF on pull_request events only, so a push to `next` falls through to origin/next -- the very commit under test. Both sides build identical content, no deltas remain, and every live ack is reported stale. PR #2768 acked a deliberate 40866 -> 42020 byte growth, was green on its own lane, and reddened `next` the moment it merged. It also reds every PR branching off the poisoned base, and since publish-emitted-baseline is gated on the test job, it blocked baseline publication too. Give the ack the base side it was missing. `diffEmitted` now takes `baseAck` -- the same document at the base ref, via `readAckFileAtRef`. An entry already present there is SPENT: it may no longer consume a delta and is never reported stale, only surfaced as `spentAcks` for tidying. An entry new or reworded in this diff stays live, and if nothing consumes it that genuinely fails, with blame on the author who just wrote it. This closes a hazard the IMPLEMENTATION named but could not prevent -- a leftover ack silently pre-clearing the next ripple on its path. (ADR-2719 §3 asserted only that TOUCHING the file is the alarm; its residual-risk list never covered pre-clearing, and §3 now carries an amendment.) Verified against the two-PR laundering sequence -- land an innocuous ack, then change the artifact -- which passed silently before and now fails on both the hash pass and the size ratchet. Three things the design has to get right, each of which was wrong first: - A read failure on the base document THROWS; only absence-at-the-ref returns null. Returning null on error LOOKS armed (every entry stays live) but a live entry's defining power is that it CONSUMES a delta, so null is armed on the staleness axis and DISARMED on consumption -- silently the whole pre-#2789 gate. `git show` cannot tell absence from fault, so absence is established with `ls-tree`. - Re-arming a spent ack costs actual PROSE. Internal whitespace and the zero-width family collapse, and `runtime` is not compared: a doubled space, an invisible character, or a decorative field would otherwise re-arm an ack whose justification still describes the previous ripple, showing a reviewer nothing. - `baseAck` is REQUIRED once an ack declares entries -- omission is an error, not a silent "inherit nothing" -- so a dropped argument fails loudly instead of quietly restoring this bug with the suite green. Because a corrupt document ON THE BASE is expensive (the loud base-side failure reds every ack-carrying PR), scripts/lint-emitted-drift-ack.cjs blocks one from landing. It is standalone rather than importing parseAck -- scripts/ ships in the npm package and tests/ does not -- so a parity test runs both surfaces over one corpus and fails on divergence; it caught one immediately, a `null` document, now classed as policy rather than schema. Deadlock is separately foreclosed: a tree carrying no ack never reads the base, so the PR that DELETES a corrupt file still lands. `readAckFileAtRef` takes an injected git runner so all four branches are tested deterministically; it never executes in the remote runner, where the real-tree test skips for want of a base ref. It also refuses an option-shaped ref, since execFileSync's array form stops shell metacharacters but not git's own option parsing. Rejected: skipping the differential when base == HEAD. It treats the symptom, costs real coverage on the push-to-next lane, and does nothing about the downstream PRs the same flaw was reddening. Deletes the now-spent tests/emitted-drift-ack.json, and updates the CONTEXT.md canon and ADR-2719 §3: presence is no longer the alarm -- a LIVE entry is, and a spent one is inert. Closes #2789 * chore(#2789): backfill changeset PR number |
||
|
clezcoding
|
5296ff152d |
docs(#2533): list gsd-cursor EoS integration (#2581)
* docs: add gsd-cursor to the EoS registry Appends the gsd-cursor EoS entry to docs/registries/eos.json and regenerates docs/registries/eos-registry.md. Discussion: open-gsd/gsd-core#2578. * docs(#2533): regenerate eos-registry.md (id-sorted) + add changeset Addresses review on #2581: the generator sorts entries by id, so gsd-cursor (< gsd-omp) must appear first in both the table and the detail sections. Also adds the required .changeset Added fragment (mirrors precedent #2448). * docs(#2533): regenerate eos-registry.md via gen-registry.cjs (markdown escaping) The hand-written markdown left ( ) and _ unescaped; scripts/gen-registry.cjs escapes them (\(...\), model\_profile\_overrides), so gen-registry.cjs --check (part of lint:ci / the lint-tests job) failed. This is the generator's exact output; node scripts/gen-registry.cjs --check now passes. * docs(#2533): regenerate eos-registry.md for corrected compat floor Regenerated via scripts/gen-registry.cjs after the enginesGsd fix. node scripts/gen-registry.cjs --check passes (exit 0). * docs(#2533): correct gsd-cursor enginesGsd floor to >=1.8.0 (real release) The >=1.39.0 floor referenced a legacy-lineage version number that no current @opengsd/gsd-core release has ever reached (latest is 1.8.0). Per trek-e's review, corrected to >=1.8.0 — the current release, which ships the model_profile_overrides.<runtime> config key gsd-cursor uses. * docs(#2533): regenerate eos-registry.md (enginesGsd >=1.0.0, install v1.0.1) * docs(#2533): mirror gsd-cursor enginesGsd >=1.0.0 + repoint install to v1.0.1 Per review: enginesGsd must mirror what the package declares. Corrected engines.gsd upstream in gsd-cursor to >=1.0.0 and cut v1.0.1; this mirrors that range and pins install at the new tag. |
||
|
0xdhx
|
a8b40fa53f |
fix(#2547): fail closed on crashing and path-shadowing Kimi payloads (#2595)
* fix(#2547): fail closed on a malformed Kimi edit list in normalizeKimiPayload
`normalizeKimiPayload` rebuilt old_string/new_string with
`String(e.old ?? '')`. `??` guards the value, not the dereference, so a
nullish entry in a Kimi `edit` list threw a TypeError at the top of the
handler, before any tool dispatch. Each guard's outer
`catch { process.exit(0) }` swallowed that crash and emitted the same exit
code as "nothing to report" — turning a should-BLOCK call into a silent
allow.
Two hard blocks were bypassable:
* gsd-worktree-path-guard's cross-git-root write block (#260) — a
StrReplaceFile write whose path resolves to a different git root is
correctly blocked with a well-formed edit list, and silently allowed
with `edit: [null]`.
* gsd-workflow-guard's force-add block on agent-* branches — a Shell
payload carrying a spurious `edit: [null]` field walks past it. The
Bash path never reads `edit`; the field only has to be present to
trigger the crash.
Fixed with `e?.old` / `e?.new`, landed identically in all five copies so
tests/kimi-guard-normalization-parity.test.cjs's byte-identity assertion
still holds.
The crash boundary is nullish specifically, not "non-object": `('x').old`
and `(7).old` are legal reads yielding undefined, so string/number entries
never threw. Both are kept as controls proving the fix did not change
their behaviour.
Regression coverage is folded into the owning suites per CONTRIBUTING.md
(no new bug-* files). Negative-controlled: the nullish cases exit 0
against pre-fix guards and exit 2 after, with positive controls (the
equivalent well-formed payload blocks) and negative controls (in-worktree
writes and benign commands still pass) alongside.
Refs #2547
* test(#2547): exercise the production Kimi payload shape in read-guard tests
The `#2304: Kimi tool vocabulary engages the read guard` cases send
payloads with no `session_id`, and runHook injects none. A live Kimi turn
always carries one — kimi-cli's hooks/events.py `_base()` sets it
unconditionally, and soul/kimisoul.py calls `set_session_id()` at the top
of every turn before tool dispatch, so the ContextVar's `default=""` never
reaches a tool call.
gsd-read-guard treats any non-empty `data.session_id` as "Claude Code
already enforces read-before-edit, skip" (#2520). So the advisory those
tests assert fires only for a shape production never sends: the tests were
green, and the guard was dormant on Kimi. A sibling #2520 case in the same
file asserts the skip when `session_id` IS present — both passed, and the
production shape hits the skip.
Two changes, test-validity only:
* Retitle the #2304 block to say what it proves — the tool VOCABULARY is
normalized through to the Write/Edit branch — with a comment warning
not to read it as production evidence.
* Add a #2547 block asserting behaviour against the production shape
(session_id populated), including a case that pins the delta directly:
the same payload fires without session_id and is silent with it.
The #2547 block characterizes a known gap; it does not endorse it.
Redesigning how the guard discriminates runtimes is explicitly out of
scope for #2547. If a later change makes the advisory fire on Kimi these
tests are supposed to fail — update them then rather than dropping the
coverage.
Refs #2547
* docs(#2547): scope the Kimi guard-engagement claim to what Kimi enforces
#2518 engaged the guards' Kimi matchers and the release notes describe the
result as "All seven guard hooks now engage on Kimi", singling out the
prompt-injection read scanner as "the security-relevant guard" taken "from
silently dormant to engaged". That is not achievable for the scanner at
the emit layer.
gsd-read-injection-scanner.js is a PostToolUse hook, and kimi-cli's
dispatch never inspects PostToolUse hook results: src/kimi_cli/soul/
toolset.py awaits PreToolUse and honours `result.action == "block"`, but
fires PostToolUse via asyncio.create_task() and returns the ToolResult
without awaiting it — the done_callback only retrieves the task's own
exception. So no output shape the scanner emits can block or flag a Kimi
tool call, and `security.injection_blocking` cannot take effect there.
Reshaping the scanner's output would not change this; the enforcement gap
is in kimi-cli's PostToolUse handling, which is out of scope here.
This corrects the claim rather than the code — there is no gsd-core emit
fix that would make it true:
* .changeset/2304-kimi-guard-tool-name.md — the fragment is unreleased,
so it would otherwise ship this as a CHANGELOG security claim.
Headline narrowed to "normalize Kimi's payload shape" and a scope
paragraph added naming what actually blocks on Kimi (the two
PreToolUse blocks) versus what cannot.
* docs/migration/kimi-to-kimi-code.md — the scanner was listed under
"Every GSD `PreToolUse` guard"; it is PostToolUse. Corrected, and the
"What about the dormant guards?" section now splits enforceable from
not-enforceable instead of saying Phase 0 "fixed all seven".
* hooks/gsd-read-injection-scanner.js — the same scope note in the
file's own Kimi rationale comment, where the next contributor to touch
the normalization will actually read it. Comment only; the shared
normalization block is untouched and byte-identity still holds.
Refs #2547
* chore(#2547): regenerate golden install-parity fixtures for the guard fix
The golden install-parity fixtures record a content hash per installed
file, so changing the five guard hooks changes their hashes across every
runtime's fixture. Regenerated with the full sweep (build, gen:golden,
size:baseline) rather than a single generator — running gen:golden alone
leaves tests/workflow-size-baseline.json stale and loses CI jobs to a
regeneration that looked complete.
The size baselines came out unchanged (no workflow or agent bodies
touched) and the hash delta is confined to exactly the five guards:
gsd-prompt-guard, gsd-read-guard, gsd-read-injection-scanner,
gsd-workflow-guard, gsd-worktree-path-guard.
Refs #2547
* fix(#2547): guard the String() coercion in normalizeKimiPayload too
Found by adversarial review of the first commit, then reproduced against
pristine next: `e?.old` closes the nullish dereference but leaves a second
route to the same crash-to-allow.
`{"toString": null}` is valid JSON, and coercing it throws
`TypeError: Cannot convert object to primitive value` — so an edit entry
that IS a well-formed object still crashes normalization, still lands in
the outer `catch { process.exit(0) }`, and still downgrades a should-BLOCK
call to a silent allow. Confirmed on both hard blocks:
{"tool_name":"Shell","tool_input":{
"command":"git add -f secret.env",
"edit":[{"old":{"toString":null},"new":"x"}]}} -> exit 0 (was)
{"tool_name":"StrReplaceFile","tool_input":{
"path":"<main-repo>/src/index.ts",
"edit":[{"old":{"toString":null},"new":"x"}]}} -> exit 0 (was)
Both exit 2 now.
The coercion is wrapped rather than replaced with a `typeof === 'string'`
test on purpose. Degrading only the non-coercible entry keeps
stringification identical for every value that CAN coerce — numbers,
arrays, plain objects — which matters because gsd-prompt-guard scans
new_string for injection patterns, and a `typeof` test would silently stop
scanning content that reaches that scan today (e.g. `new: ["ignore all
previous instructions"]` currently stringifies and is scanned). Verified:
zero behaviour change across string, number, bool, null, array-of-strings,
nested array, plain object and `__proto__`-keyed input; only the throwing
case changes, from crash to ''.
Regression cases are negative-controlled against the previous commit: the
four new coercion-trap tests fail with only the `e?.old` fix in place and
pass with this one.
Refs #2547
* chore(#2547): cover the String() coercion vector in the changeset
The release note described only the nullish-dereference route. Both routes
reach the same fail-open, so both belong in the changelog entry, along with
why the coercion is wrapped rather than type-tested.
Refs #2547
* chore(#2547): point the changeset fragment at the real PR number
The fragment has to exist before `gh pr create` runs, so it carried the
issue number as a placeholder. Corrected to 2595 now that the PR is open.
Refs #2547
* fix(#2547): make Kimi's `path` authoritative over a model-supplied `file_path`
normalizeKimiPayload copied Kimi's `path` into `file_path` only when
`file_path === undefined`, so any `file_path` the model chose to include won
outright. Every guard reads `file_path`; kimi-cli executes on `path`. The guard
therefore inspected one file while the write landed on another.
This bypass needs no crash. A payload pairing a cross-root `path` with a
spurious `file_path: ""` left gsd-worktree-path-guard reading an empty string
and exiting 0, while the identical write without the extra key blocked — the
same cross-root write the #260 block exists to catch. The shadowing also
preserved a non-string `file_path` (`[]`), which threw inside that guard's
path.isAbsolute() and reached its outer `catch { process.exit(0) }`: the same
crash-to-allow the rest of #2547 closes, reached through the guard's own read
rather than through normalization.
Reachability is not speculative. kimi-cli's soul/toolset.py json-parses the
model's raw tool arguments and passes that dict verbatim as tool_input to
PreToolUse, performing typed validation only later inside tool.call() — after
the hook has already decided. So the model controls extra keys in tool_input at
the moment the guard runs. kimi-cli's file tools carry no `file_path` field at
all (src/kimi_cli/tools/file/write.py, replace.py), so a `file_path` in a Kimi
payload is always model-supplied.
`path` now wins outright. Overwriting can only ever narrow what a guard inspects
to the path that will actually be written, so it cannot under-block.
Normalization returns early for non-Kimi tool names, so the native Claude Code
contract (file_path governs) is untouched.
Landed identically across all five inlined copies; the byte-identity assertion
in tests/kimi-guard-normalization-parity.test.cjs enforces that.
* test(#2547): cover the file_path-shadowing bypass in the #260 guard suite
Four cases, each exiting 0 (bypass) against the pre-fix guards: a spurious
empty-string file_path, an in-worktree decoy file_path, and non-string
file_path values (array and object) that additionally crashed
path.isAbsolute() into the outer catch.
Two controls that are not bypass cases and matter as much:
- an in-worktree write carrying a cross-root DECOY file_path must still exit
0. Pre-fix this blocked, because the decoy won; the guard now follows the
path kimi-cli executes on in both directions, so the fix narrows what is
inspected without over-blocking.
- a native Claude Edit (no `path` field) must still block on file_path alone.
normalizeKimiPayload returns early for non-Kimi tool names, and this pins
that the non-Kimi contract did not move. It passes both pre- and post-fix
by design.
Negative-controlled: run against the pre-fix hooks, the four bypass cases and
the decoy control fail, and the native-Claude control passes.
* test(#2547): back the totality claim with property tests over fc.anything()
This PR claims the fix "makes normalization total over the inputs JSON can
express" — a for-all guarantee — while the tests backing it are example-based,
each shape added reactively after a crash was found by hand (the String()
coercion trap was itself found by adversarial review after the first commit
shipped). Example-based tests cannot substantiate a for-all claim; they record
the counterexamples someone happened to think of.
Four properties over fc.anything(), which is exactly the JSON-expressible
domain the claim names:
(a) totality over any tool_input
(b) totality over any edit list — the crash surface both #2547 fixes targeted
(c) `path` always wins over any model-supplied `file_path` (the review blocker
invariant: a guard reading file_path can never be aimed at a file other
than the one kimi-cli writes)
(d) a non-Kimi tool_name passes through untouched — the native Claude contract
normalizeKimiPayload is inlined per hook with no runtime binding, so there is
nothing to require. The block is extracted from hook source and evaluated via
the SAME extraction contract kimi-guard-normalization-parity.test.cjs uses, so
a source edit that breaks one breaks both instead of silently testing a stale
block. An extraction floor test fails loudly if the extraction yields a no-op.
Non-vacuous, and checked rather than assumed: against pristine pre-#2547 `next`,
(a), (b) and (c) all FAIL and (d) passes. (a) needed the fix that makes it
meaningful — a bare fc.anything() for tool_input passed even against the live
defect, because arbitrary generation essentially never invents the `edit` key
the crash lives behind, so the generator is biased onto the keys normalization
actually reads and unioned back with unbiased input.
* chore(#2547): cover the shadowing vector in the changeset and regen goldens
Golden install-parity churn is hash-only, on exactly the five hook files this
round changed. gsd-phase-boundary.sh is deliberately unchanged.
* test(#2547): make the property test able to kill the coercion mutant
Review Major 1: the generative test added to stop the NEXT counterexample
could not kill the one it was written for. Reproduced the reviewer's matrix
independently — against the shipped generator, a mutant reverting `editText`
to the unguarded `String(v ?? '')` passed all four properties.
Cause, confirmed by measurement: the edit-array ENTRIES were bare
`fc.anything()`, which essentially never invents an `old`/`new` key, so
`e?.old` was always undefined and `String(undefined ?? '')` never coerced
anything. That is the same vacuity the file's own comment describes one level
up, reproduced one level down.
The review's prescribed fix — bias the entry onto `{old, new}` — is necessary
but NOT sufficient, and this is the part worth recording: measured over 20,000
draws, bare `fc.anything()` yields a non-coercible value 3 times (0.015%). At
`numRuns: 200` an `old` key holding a hostile value essentially never
co-occurs, and the mutant survives the entry bias too. Both levels need
biasing — the entry onto the keys normalization reads, and the VALUE onto the
shape that actually throws.
`{"toString": <non-function>}` is that shape and stays inside the
"JSON-expressible" domain the claim names (JSON.parse produces it verbatim);
`fc.anything({withNullPrototype: true})` would also kill the mutant but widens
the domain past what the PR asserts, so it is not used.
Verified: M1 now dies at every seed tried (1/7/42/99/4242/31337, failing
within 3-31 cases) while HEAD stays green at all of them.
Also closes three coverage gaps the review listed as nits — properties (e)
totality over any JSON value as the WHOLE payload, (f) the tool_output →
tool_response mapping (including that an existing tool_response is not
clobbered), and (g) an empty edit list reconstructing nothing.
Property (e) required a one-line fix in the normalizer itself: `JSON.parse
('null')` is null, and null/primitive payloads threw on the `data.tool_name`
read — falsifying the "total over the inputs JSON can express" claim. Harmless
in practice (the throw landed in the same fail-open catch as the exit 0 it now
takes deliberately), but the claim should be true as stated. Landed
byte-identically across all five copies.
Refs #2547
* fix(#2547): make old_string/new_string authoritative, not merely defaulted
Review Major 2: the shadowing class the prior round's BLOCKER closed for
`file_path` survived one field over. `old_string`/`new_string` were still
reconstructed only `if (input.<field> === undefined)`, so a model-supplied
value won.
The argument for making `path` authoritative applies verbatim here.
kimi-cli's StrReplaceFile schema is `path` + `edit` only
(src/kimi_cli/tools/file/replace.py @ 4a550ef) and carries no
`old_string`/`new_string` at all, so either key appearing in a Kimi payload is
always model-supplied — exactly like `file_path`.
Verified end-to-end against the reviewer's payload: a cross-root write
carrying `new_string: ""` alongside an injected `edit[].new` left
gsd-prompt-guard reading '' and returning at its `if (!content)` guard, so the
injection advisory never fired and the reconstructed content was never
scanned. `new_string: null` behaved identically. Negative-controlled: both
produce empty output against pre-fix source and fire the advisory after.
Chose unconditional reconstruction over the offered `typeof` alternative
deliberately. A type test closes `""`/`null` but leaves the interesting case
open — a benign NON-EMPTY decoy (`new_string: "chore: tidy"`) shadows just as
effectively and passes any type test. The new suite includes that case
specifically; it is what discriminates between the two candidate fixes.
Also pins the kimi-cli SHA in the authoritative-path comment, as requested —
it cited file names with no version while the issue pins 4a550ef.
Landed byte-identically across all five inlined copies; the parity test's
byte-identity assertion holds.
Refs #2547
* fix(#2547): close the non-string file_path crash-to-allow at every read site
Review Major 3: the crash-to-allow was closed only as a side effect of `path`
masking the bad value, while the changeset read as though it were closed
outright. Confirmed both of the review's reachability claims: `[]`/`{}`/`42`
are truthy, survive the `!rawFilePath` early-out, and throw inside
path.isAbsolute() into the outer `catch { process.exit(0) }`; and normalization
returns early for native Claude Code payloads (KIMI_TOOL_NAMES has no 'Edit'
entry), so `{"tool_name":"Edit","tool_input":{"file_path":[]}}` reached it
untouched — this guard's original #260 surface.
Reproduced on a real fixture: string cross-root path exits 2, the identical
payload with `[]` or `{}` exits 0.
Swept the class rather than the instance. Five more untyped read sites across
four other hooks, each one line from a type-strict or method-dependent call.
Census of what each can actually do:
gsd-worktree-path-guard.js:173 BLOCKS -> live bypass (the review's finding)
gsd-prompt-guard.js:128 scanner -> silenced the injection scan, the
same outcome as Major 2 by another
route; verified empirically
gsd-workflow-guard.js:206 advisory only (its exit-2 is the Bash
force-add path, which reads
`command`, not `file_path`)
gsd-read-guard.js:141 advisory only
gsd-read-injection-scanner.js:213 advisory only
gsd-windsurf-pre-write.js:75 ALREADY TYPED — the shape adopted here
All six now read typed. The workflow-guard site keeps its truthiness fallback
(`(typeof x === 'string' && x) || ...`) because a bare type test would let an
empty `file_path` shortcut the `path` fallback.
Also declares one swept hit NOT fixed: `gsd-workflow-guard.js:175` reads
`command` untyped on a genuinely blocking path. Same shape, but not
exploitable — unlike file_path/path there is no second field carrying the
executable value, so a non-string command cannot smuggle a real `git add -f`
past the block. Left alone rather than widen this PR into the Bash path.
The regression gate is a SOURCE-level invariant, not a behavioural one, and
that is deliberate: the fixed read and the crashing read are black-box
identical — both end at exit 0, one via the catch and one via the early-out.
A test asserting exit 0 on a non-string payload passes against the unfixed
code, which is the same false-green the review flagged in the existing
`['non-string file_path (array)', []]` cases. Repeating it one level up would
be no better. tests/kimi-guard-typed-payload-reads.test.cjs fails if any hook
regresses to an untyped read (negative-controlled: it reports all five
pre-fix sites with correct file:line).
The behavioural cases requested — non-string file_path with NO `path` key —
are added to worktree-safety.test.cjs and labelled honestly as documenting the
explicit fail-open rather than detecting a revert.
Also states the relative-path premise (review Minor 5) at the early-out that
depends on it: "always safe" holds only while every runtime reaching there
resolves relative paths against the tool CWD. Claude Code satisfies it by
requiring absolute paths; kimi-cli's resolution behaviour is NOT verified here
and is recorded as an unverified premise rather than an asserted bypass.
Refs #2547
* docs(#2547): correct the changeset's closed-claim and fold the misattributed note
Review Major 3 also flagged the fragment: it said the non-string vector "threw
inside that guard's path.isAbsolute() ... `path` now wins outright", which
reads as closed when it was closed only conditionally. Rewritten to state what
is now true — closed unconditionally at all six read sites — and extended with
the Major 2 finding.
Review Minor 6 (the #2547 scope note living in a `pr: 2518` fragment) turns out
to understate the problem. Rendering the changelog and re-parsing it shows the
note is not merely misattributed — it is DROPPED. serializeChangelog emits each
fragment as a single `- ` bullet, and parseChangelog terminates a bullet at the
first non-continuation line, so everything after a blank line is lost on
re-parse. Audited all 44 fragments: exactly one was lossy —
2304-kimi-guard-tool-name.md, losing 656 of 1730 characters, i.e. precisely
that second paragraph. Folding it into this PR's fragment fixes the
attribution and the silent loss together; all 44 now round-trip losslessly.
That same mechanism is why the remaining nit — reformat this fragment's
~2,000-character paragraph for readability — is NOT applied. A paragraph break
or a bullet list would silently truncate the entry at the first blank line
(verified for both). The single-paragraph form is load-bearing under the
current serializer, not an authoring preference. Worth its own issue; noted in
the PR thread rather than worked around here.
Refs #2547
* chore(#2547): regenerate golden install parity after rebase onto next
Rebased onto `next` @
|
||
|
Rezolv
|
7e8f6a6d7d |
enhance(#2572): run the verify-summary artifact check against phase SUMMARYs (#2685)
* enhance(#2572): run the verify-summary artifact check against phase SUMMARYs (W025) The artifact<->git check has existed since the beginning but was only ever pointed at .planning/research/SUMMARY.md (new-project.md:1145, new-milestone.md:425). Phase summaries -- the ones that actually claim "I created these files" -- were never checked. - extract verifySummaryCore from cmdVerifySummary: same checks, lifted out of the output() wrapper so callers consume {passed, checks, errors} directly instead of shelling out and re-parsing JSON; cmdVerifySummary is now a thin adapter over it - validate.health gains advisory W025 per phase SUMMARY with missing files Advisory only: appends to warnings[], never touches status escalation beyond the channel's own warning semantics, the repair set, or readVerificationStatus. Resolves both open questions from triage: (a) commits_exist is deliberately NOT surfaced -- its hash pattern matches any hex-shaped token in prose, too loose to show a user; (b) a phase carries N per-plan summaries plus a legacy bare SUMMARY.md, so all of them are checked via the repo-wide filter. * chore(#2572): add changeset fragment * feat(#2572): move the SUMMARY artifact check to phase completion Responds to the #2685 review. Three substantive changes. Seam (Blocker 2). The check now runs in cmdPhaseComplete, the seam the issue body cited (src/phase.cts:~1745), not validate.health. That channel does exist: cmdPhaseComplete declares warnings[], populates it from the UAT/VERIFICATION pre-scan, and emits it. The cycle objection raised against the earlier deviation holds for state.cts only -- verify.cts has no transitive import path to phase.cts, so phase.cts -> verify.cjs adds no cycle (verified over every src/*.cts). Moving it also retires the retroactive firing across all historical phases: this fires once, at completion, for the phase being completed. Extraction (Blocker 1). Pattern 2 now excludes [ and ] from its path class. The SUMMARY templates prescribe a YAML flow sequence (key-files.created: [a.ts, b.ts]) and the label matches case-insensitively, so the class previously captured the literal [ and produced a candidate that can never exist on disk -- firing on healthy projects built from the templates GSD itself ships. Stripping frontmatter was the other offered remedy; measured across all three shipped templates it is a no-op on top of the exclusion, so it is not carried. Consequence named in-code: the key-files block still is not read, which needs a real frontmatter parse. Also narrowed to the noise classes confirmed in review -- globs, bare hostnames, and paths resolving outside the project are skipped rather than reported, and the containment guard the old comment claimed now actually exists. Budget (Majors 1 and 3). verifySummaryCore takes a checkCommits option; phase completion passes false, so the discarded git cat-file probes are not spawned at all. It also passes Infinity, so every referenced file is reported instead of the first two -- a summary listing twelve files of which nine are missing now says nine, not zero. The verb keeps its historical 2-file default. Tests (Major 2). The vacuous fixtures are gone with the health block. The replacements use /-bearing paths that genuinely extract, and each fix was mutation-checked: un-anchoring pattern 2, dropping the glob, hostname or containment filter, forcing commit checking on, and re-capping at 2 each fail at least one test. * docs(#2572): describe the phase-completion SUMMARY artifact check The W025 text under /gsd-health is withdrawn with the health seam; the check is documented where it now runs, under `phase complete` in docs/CLI-TOOLS.md. Both the docs and the changeset previously overclaimed: they said a referenced file not on disk is warned about, while at most two candidates per SUMMARY were ever examined. The cap is gone at this seam, so the claim now holds -- and the text states the limits that remain, rather than leaving them to be discovered: the key-files frontmatter block is not read, commit hashes are not resolved, and globs, URLs, bare hostnames and out-of-project paths are skipped rather than reported. --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> |
||
|
Rezolv
|
6932fb16d7 |
enhance(#1699): require read-and-cite provenance for in-repo discrete values (#2768)
* test(#1699): failing-first contract tests for in-repo value provenance
Nine assertions on the deployed gsd-phase-researcher contract: the discrete-value taxonomy, the same-session Read requirement, path-AND-line-range citation, grep-alone exclusion, the verbatim quote and paraphrase ban, the quote-is-the-checkable-artifact guard, [ASSUMED] routing for unquoted skeleton values, a no-regression guard on the pre-existing package name provenance rule, and a single-definition-site guard.
Eight of the nine fail against the unmodified agent on origin/next; the ninth is the no-regression invariant and passes in both states, which is the intended enhancement shape.
Added to tests/research-agent-profiles.test.cjs rather than a new file: that file already carries the allow-test-rule exemption <runtime-contract-is-the-product> research agent .md content is the governed surface, so no new allowlist entry and no change to the per-module test-file count.
* enhance(#1699): require read-and-cite provenance for in-repo discrete values
The claim-provenance system governed external facts (npm registry, official docs, Context7, package-name provenance). For an in-repo discrete value -- an enum, schema or type union, error code, status constant, or filesystem path -- [VERIFIED] could be earned from training memory or a bare codebase grep, which proves a string occurs, not that the definition was read.
A drifted value passes into RESEARCH.md, is lifted by the planner into PLAN.md's <interfaces> context block, and is trusted by the executor, where it fails at parse()/typecheck as a mid-execution deviation -- the most expensive place to discover it.
The rule lands beside its structural sibling, the package name provenance rule, since both say existence is not verification. The verbatim quote is named as the load-bearing artifact: a citation with no quote does not earn the tag, however precise the line range looks. That keeps the rule falsifiable against the file rather than a self-report, which is the Goodhart guard.
Scope note: the quote goes in RESEARCH.md beside the claim, NOT in an <interfaces> block. <interfaces> appears zero times in this agent on next -- it is planner-side, defined at gsd-core/references/planner-interface-context.md:15 as a PLAN.md structure. The issue text and triage both said <interfaces>; instructing the researcher to populate a block it does not emit would be undefined.
Defined once at the definition site; the source-hierarchy recap is deliberately untouched, since restating it is the paraphrase-drift mode META.RULE.brief-no-paraphrase names.
* test(#1699): regenerate agent size baseline and golden parity fixtures
Regenerated via npm run size:baseline and npm run gen:golden, never hand-edited. agent-size-baseline gsd-phase-researcher.md 40866 to 42020 (LARGE tier, cap 49152, 7132 bytes headroom remaining, per ADR-1610's per-file baseline guard). 18 of 19 golden fixtures updated; pi.json is unchanged because the pi runtime ships zero agents.
* chore(#1699): add changeset for the in-repo value citation rule
User-facing behavior change in gsd-phase-researcher, so a Changed fragment is required. pr: 2768.
* test(#1699): acknowledge the gsd-phase-researcher growth for the emitted-drift gate
CI test (ubuntu-latest, 22) failed on emitted-attribution.test.cjs: gsd-phase-researcher.md grew 1154 bytes (40866 -> 42020) without an acknowledgment. The differential emitted-attribution gate landed on next in
|
||
|
Tom Boucher
|
e276cc7f00 |
enhance(#2778): make the size-ratchet failure name its own remedy (#2780)
* fix(#2778): exempt intentionally-absent paths from the glossary gate check-glossary-refs asserts that every backticked tests/ token in CONTEXT.md resolves on disk. tests/emitted-drift-ack.json (ADR-2719 section 3) is absent on a healthy next BY DESIGN — it appears only inside a PR that needs it, which is what makes touching it the alarm. It passed before only by accident of backtick pairing: CONTEXT.md's RULESET entries are themselves backtick-wrapped and contain backticks, so the token happened to fall outside a code span. Any edit that shifted the parity exposed it. A gate that passes by luck is not passing. The exemption is exact, not a prefix hole: a sibling missing tests/ path still fails, and a test locks that. * feat(#2778): make the size-ratchet failure name its own remedy The growth branch stated a requirement and withheld the means of satisfying it: no ack file named, no schema, no key format, and no do-not-regenerate line — so the likeliest guess was to hunt for a baseline that #2724 deleted. Observed live on #2543. All remediation now comes from one frozen REMEDIATION export whose example document is rendered from ACK_VERSION, so the taught schema cannot drift from the schema parseAck accepts. A round-trip test feeds the printed document back through parseAck. The report is now built as a typed IR (buildReport) that formatReport renders, so tests assert on structure rather than prose, per CONTRIBUTING.md's raw-text-matching rule. Two defects found and fixed inline while building: - diffEmitted's validation early-return omitted newFileCapExceeded while formatReport reads its length, so the branch that reports a failed git diff threw a TypeError instead of naming the problem. - Printing one complete ack document per failing branch made each read as the whole file, so pasting the second over the first silently lost an acknowledgment. One document now covers the whole report. Closes #2778 * chore(#2778): backfill changeset pr number to 2780 |
||
|
Rezolv
|
16e59d0db5 |
fix(#2691): repair seven dangling references in the ADR corpus and contributor docs (#2692)
* fix(#2691): repair five dangling references in the ADR corpus and contributor docs
Found by the 2026-07-24 ADR corpus audit; each mechanism re-reproduced live
against next @
|
||
|
0xdhx
|
3274db2757 |
fix(#2526): remove gsd-ui-auditor's uncallable Playwright-MCP block (#2594)
* fix(#2526): drop gsd-ui-auditor's uncallable Playwright-MCP block The agent declares `tools: Read, Write, Bash, Grep, Glob, Skill` — no `mcp__*` grant of any kind — while its body presented a `<playwright_mcp_approach>` block as the *preferred* capture path. That branch was unreachable by construction: the availability check had a fixed answer, the three `mcp__playwright__*` calls could never dispatch, and the "when Playwright-MCP is NOT available" fallback was the only branch that ever ran — 39 lines of instruction loaded on every /gsd-ui-review spawn that also invited the model to claim a capture path it could not take. Remove the dead block, leaving the CLI screenshot path as the sole documented approach. Guard the class in tests/mcp-tool-inheritance.test.cjs, which already owns agent MCP-grant parity: the new block generalizes the #1284 researcher check from two agents and one dispatch table to every agents/*.md and its whole body — no agent may document an `mcp__<server>__*` namespace absent from its own `tools:` declaration. Frontmatter is read through the canonical parser (gsd-core/bin/lib/frontmatter.cjs) rather than a hand-rolled scan, so inline CSV, block sequences, flow arrays, quoted scalars and full-line comments are handled by construction; inline comments inside a scalar survive that parser, so they are stripped explicitly. The check is server-level by design, ignores prose metavariables like `mcp__X__*`, matches hyphenated server ids, and carries a discovery guard plus negative controls for every documented boundary so it cannot decay into a vacuous pass. The session-level Playwright-MCP pass in gsd-core/workflows/ui-review.md is deliberately untouched — workflow files carry no fixed allowlist, so their availability check is genuinely runtime-detected and honest. Fixes #2526 * chore(#2526): set changeset fragment pr to 2594 The fragment shipped with the documented `pr: 0` placeholder because the PR number does not exist until the PR is opened, and scripts/changeset/parse.cjs rejects `pr <= 0`. Now that the PR is open, set the real number so changeset-lint passes. * test(#2526): cover the two-char server-id boundary of the metavariable exclusion The length-1 "prose metavariable" exclusion was tested at length=1 and at real ids (>=3 chars), but never at length=2 — the limit+1 boundary where a server id starts being recognized. Review finding on #2594: `mcp__ab__foo` in a body with no grant must flag `['ab']`. * fix(#2526): treat a bare mcp__* grant as covering every server `grantedServers()` stripped `mcp__*` to the empty string and dropped it via `if (server)`, so an allowlist that grants every MCP server read as granting none — and the guard then fired against a body the grant plainly covered. That is the one input shape that inverts the check, turning it against a correct agent rather than merely missing a bad one. A `/^mcp__\*+$/` token now sets a GRANT_ALL sentinel that short-circuits `ungrantedServers()`. The sentinel `*` is outside REFERENCE_RE's character class, so no body reference can collide with it. A bare `mcp__` with no wildcard stays a typo rather than a grant and keeps failing closed. No agent uses the `mcp__*` spelling today, so this was latent rather than live. Two negative controls pin both halves. * fix(#2526): scan the frontmatter description for MCP references too `ungrantedServers()` scanned `stripFrontmatter(content)` only, so an `mcp__foo__bar` reference in the `description` field escaped the check. That field ships with the agent and the dispatcher reads it, which makes a dead reference there exactly as dead as one in the body. Only `description` is added to the scanned surface, never the whole frontmatter: `tools:` is the grant list itself, so scanning it would let every allowlist satisfy itself and turn the guard vacuous. A negative control pins that boundary alongside the new positive case. All 34 per-agent tests still pass with the wider surface, so no live agent verdict changes — this was latent. * test(#2526): give multi-character placeholders a convention the checker knows The metavariable exclusion is `length === 1`, so the natural placeholders `mcp__SRV__*` and `mcp__SERVER__*` were flagged as real references — and the failure message then offered an author two remedies ("grant the namespace or drop the block") that both misread what they wrote. Adopts the angle-bracket half of the suggested fix: `mcp__<SERVER>__*` is the sanctioned multi-character placeholder, exempt by construction because `<` is outside the reference pattern's character class. This pins an existing property rather than adding a special case. Declines the all-caps half. An all-caps exemption would be a false NEGATIVE for any real server spelled in caps, and a guard that misses a dead reference fails in exactly the direction this check exists to prevent. The bare-caps form keeps firing; the message now names the convention as a third remedy. Also corrects "grants neither" in that message, which was wrong for any count other than two. * test(#2526): pin the zero-length server id, completing the boundary triple `mcp____foo` yields `[]`, but for a different reason than the length-1 case: it is unrepresentable by `/mcp__([A-Za-z0-9_-]+?)__/g` since `+?` requires at least one character, so the pattern skips it before the metavariable exclusion is ever consulted. Pinning limit-1 completes the 0/1/2 boundary rule on its own terms and records which mechanism owns the case. * docs(#2526): correct every drifted AGENTS.md Tools row, not just the one The review asked for the one-line `gsd-ui-auditor` correction (missing `Skill`). Sweeping the defect class first — every `**Tools**` row in docs/AGENTS.md against its agent's `tools:` frontmatter — found it was 26 of 34 rows, so the one-line framing was the reviewer's premise rather than the population. Breakdown of the 26: * 21 omitted `Skill`, 6 omitted `Edit` (overlapping) — under-promises, the same drift class as #2526 but in the harmless direction. * 8 wrote `mcp (context7)` as shorthand while frontmatter granted up to 8 servers (firecrawl, exa, tavily, ref, jina, perplexity, both context7s). * 1 was actively wrong: gsd-debug-session-manager documented `Task`, a tool name that no longer exists — the #2526 shape at the doc layer, naming a capability that cannot dispatch. Every row is now the frontmatter `tools:` value verbatim, which is also what makes the parity guard in the following commit non-brittle. The diff is 26 insertions / 26 deletions, all Tools rows. * test(#2526): guard AGENTS.md Tools rows against agent frontmatter The 26 corrected rows in the previous commit were free to drift because nothing asserted the role card and the frontmatter agreed — the same reason the #2526 block itself survived. Correcting them without an invariant just resets the clock. Lands in agent-classification-parity.test.cjs rather than a new file: that suite already owns docs/AGENTS.md as a contract surface, already carries the `allow-test-rule` exemption for treating the doc as the product, and file count is the unit of CI overhead (docs/TESTING-SUITES.md). Compares the row to the frontmatter value VERBATIM, not as a set — a set comparison would keep accepting the "mcp (context7)" shorthand that hid eight grants behind one, which is the under-documentation half of the drift. Carries the same discovery guard #2526's own check uses: a section with a granted `tools:` but no **Tools** row fails loudly, so deleting a row cannot silently retire its assertion. Both halves are negative-controlled — against the pre-fix doc it fails naming 26 rows (gsd-ui-auditor:339 among them), and with a row deleted it fails on the missing-row assertion. * chore(#2526): note the AGENTS.md drift correction in the changeset The role cards are user-visible, and 26 of them documented a tool set the agent did not have. Type, `pr: 2594`, and the trailing `(#2526)` are unchanged. * fix(#2526): use a CRLF-safe split in the AGENTS.md Tools-row guard `lint-tests` (npm run lint:ci) rejected `rawAgentsMd.split('\n')` under the repo's local/no-crlf-fragile-split rule: Windows autocrlf yields CRLF, so a trailing \r rides into the parsed line. Switched to `/\r?\n/`. Caught by CI on the round-3 push before the response comment went out. * docs(#2526): correct the drift tallies stated in c5a9607e Re-derived the census programmatically from the pre-fix doc instead of by eye. The 26-of-34 headline was right; the breakdown was not. Skill omitted 21 -> 22 Edit omitted 6 -> 7 "mcp (context7)" shorthand 8 rows -> 7 rows The 8 was conflating two things: 8 rows omitted MCP grants entirely, but only 7 of them used the "mcp (context7)" shorthand — gsd-executor listed no MCP at all. Also names the one `Agent` omission (gsd-debug-session-manager, the row that still read `Task`). c5a9607e's message keeps the wrong numbers rather than rewriting a pushed branch mid-review; the test comment and changeset are the durable statements and both are corrected here. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com> |
||
|
Tom Boucher
|
1c1af70a4b |
refactor(#2724): delete the committed golden fixtures and size baselines (#2767)
* test(#2724): delete golden-install-parity fixtures, test, and generator Removes the 19 committed path->hash manifests, the two per-file size baselines, tests/golden-install-parity.test.cjs, and scripts/gen-golden-install-parity-zcode.cjs. These were pure functions of the source tree (ADR-2719); the differential attribution check (tests/emitted-attribution.test.cjs + tests/emitted-provenance.test.cjs) is now the sole gate for emitted-artifact propagation. tests/fixtures/install-tree/*.json and tests/golden-install-tree.test.cjs are unchanged (ADR-2719 section 7 exception). Follow-up commits fix the resulting bookkeeping: scripts/ci-test-scope.cjs's existence guard, .gitattributes, package.json scripts, the emitted-provenance totality guard's IO, the differential check's baseline acquisition, CI wiring to publish/restore the baseline artifact, and docs. * refactor(#2724): make the differential attribution check self-sufficient Three fixes required to delete the golden fixtures without breaking CI: - scripts/ci-test-scope.cjs: remove tests/golden-install-parity.test.cjs from the three rules that named it. #2759's missingRuleTestFiles guard hard-throws at module load if a rule names a test file absent from disk, which would break the changes job on every PR the moment the fixture-deletion commit landed. - tests/helpers/emitted-provenance.cjs: loadManifests() read the committed golden fixture directory. With that directory deleted at every future ref, this would throw at module load forever, taking the Phase 2 totality guard down with it. Rebuilt from real installer spawns (MANIFEST_FAMILIES + runMinimalInstall + buildParityManifest), the same shape emitted-runtime.cjs's currentManifests() already uses. - tests/emitted-attribution.test.cjs / tests/helpers/emitted-runtime.cjs: the real-tree test's baseline acquisition swaps from baselineManifestsAtRef(base) (git show at a ref that no longer carries fixtures) to resolveBaseline()'s documented precedence: env, then the on-disk cache, then an in-job build. The build fallback (buildBaselineAtRef, new) checks out base into a throwaway git worktree and runs the new scripts/gen-emitted-baseline.cjs there -- no npm ci needed, since bin/install.js and the test helper shells are Node-builtins-only. That script also publishes the baseline artifact from CI's push-to-next job (wired in a follow-up commit). * refactor(#2724): retire the merge-driver bridge and per-file size baselines The Phase 1 bridge (#2721) is retired now that the artifacts it guarded are deleted: scripts/git-merge-regen-driver.cjs, its test, and the 'setup:merge-driver' npm script are removed, and the .gitattributes merge=gsd-regen/linguist-generated block for the three deleted-path globs is dropped. tests/fixtures/install-tree/*.json keeps its normal merge behavior, unchanged (ADR-2719 section 7). scripts/update-size-baseline.cjs and its test are removed: their sole purpose was regenerating tests/workflow-size-baseline.json and tests/agent-size-baseline.json, both deleted. The 'size:baseline' npm script and its step in 'regen:derived' go with it. The per-file baseline describe blocks in tests/workflow-size-budget.test.cjs and tests/agent-size-budget.test.cjs are removed for the same reason; the independent loose-tier hard caps are untouched. The differential attribution check's size ratchet (tests/emitted-diff.cjs, already shipped in #2723) is the replacement anti-creep mechanism. 'npm run gen:golden' is replaced by 'npm run gen:install-tree', which keeps regenerating tests/fixtures/install-tree/*.json (the one artifact family ADR-2719 section 7 keeps committed); tests/golden-install-tree.test.cjs's error messages point at the new command name. tests/golden-parity-single-source.test.cjs's anti-divergence guard (#2266) is retargeted from the two deleted golden-parity consumers to their two replacements (tests/helpers/emitted-runtime.cjs and tests/helpers/emitted-provenance.cjs), which import buildParityManifest the same way — the divergence risk the guard exists for is unchanged. Also wires CI: a new publish-emitted-baseline job runs scripts/gen-emitted-baseline.cjs after a push to next and caches the result keyed on the sha; the test and test-full jobs restore that cache on pull_request events, keyed on the PR's base sha, and export GSD_EMITTED_BASELINE for tests/emitted-attribution.test.cjs's real-tree test to pick up. * docs(#2724): flip ADR-2719 to Accepted and update contributor docs Status: Proposed -> Accepted. Regenerated docs/adr/README.md index. CONTRIBUTING.md, docs/TESTING-SUITES.md, and CONTEXT.md (RULESET. EMITTED_ATTRIBUTION, RULESET.WORKFLOW_SIZE_BUDGET, RULESET. AGENT_SIZE_BUDGET, and the Emitted Artifact Provenance glossary entry) no longer point at the deleted golden-install-parity fixtures, size baselines, gen:golden, UPDATE_GOLDEN, or the setup:merge-driver / git-merge-regen-driver.cjs bridge. Editing shipped content now requires zero manual fixture regeneration, documented against the differential attribution check instead of the deleted commands. * docs(#2724): add changeset for removed golden-parity commands * fix(#2724): drop stale scripts/update-size-baseline.cjs glossary ref check-glossary-refs.cjs verifies every backtick-wrapped scripts/*.cjs token in CONTEXT.md resolves to a real file. The RULESET. EMITTED_ATTRIBUTION rewrite named the deleted script inside backticks, which the checker reads as a live reference, not historical prose. * test(#2724): retarget ci-test-scope tests off the deleted golden test tests/ci-test-scope.test.cjs asserted specific RULES entries select tests/golden-install-parity.test.cjs, and that every rule selecting it also selects both emitted gates. Both premises broke when the golden test was deleted (#2724): the deleted filename never re-appears in targeted_tests, and there was no longer a third file for the gates to travel alongside. Retargeted the two selection describe blocks to assert tests/emitted-provenance.test.cjs directly (the drift guard the golden gate's rules were retargeted to), and simplified the third block to assert the two emitted gates always travel together, without reference to the golden filename. * docs(#2724): repoint two contributor how-to guides at the differential check Both guides told contributors to regenerate a baseline against tests/golden-install-parity.test.cjs, which #2724 deletes. Repointed at the differential attribution check (tests/emitted-attribution.test.cjs, ADR-2719), which needs no manual regeneration step. * fix(#2724): repair phase6-capstone-conformance's deleted-baseline read An independent orthogonal review caught a real regression this branch introduced into a test file the branch's diff never touched: tests/phase6-capstone-conformance.test.cjs read tests/workflow-size-baseline.json (deleted earlier in this branch) with no fallback, so the whole suite would throw ENOENT the moment this branch landed. The test's actual intent — prove the host-loop workflow files are real, tracked, non-empty docs — is preserved by asserting the live byte count via the same shared counter (scripts/workflow-size.cjs) the size guards already use, instead of a committed snapshot. Also, from the same review: a stale doc comment in scripts/workflow-size.cjs still named the deleted scripts/update-size-baseline.cjs as a consumer, and buildBaselineAtRef's cleanup in tests/helpers/emitted-runtime.cjs left two fs.rmSync calls unguarded against masking the primary result/error, inconsistent with the try/catch already wrapping the git cleanup beside them. Both fixed. A doc comment was added to baselineFamilyNamesAtRef explaining why it (and its siblings) are kept despite having no production caller post-cutover — they still answer real questions about refs that predate the cutover. * fix(#2724): repair three real regressions found by remote verification 1. tests/emitted-provenance.test.cjs's two hostile-input tests (non-object manifest, unreadable fixture) drove loadManifests(tmp) and monkeypatched fs.readFileSync, both premised on the deleted fixture-directory read this branch already replaced with real installer spawns -- the negative assertions silently stopped firing. loadManifests() now accepts injected {families, install, build, clean} (defaulting to production values), giving the tests a real seam to drive a bad build result and a build failure through the ACTUAL loader instead of a reimplementation, and added coverage that clean() still runs on both paths. 2. .github/workflows/test.yml's two 'Export GSD_EMITTED_BASELINE' steps hardcoded shell: bash, which is wrong on windows-latest (native pwsh) and on test-full's macos-latest legs (native zsh per that job's own matrix) -- the repo's H1 shell policy (tests/policy-shell-pinning .test.cjs) caught it. Replaced the inline bash script with scripts/ci-export-emitted-baseline-env.cjs, a plain Node script: a bare 'node <path>' command line has no shell-specific syntax, so it runs correctly under bash, zsh, and pwsh without a shell override. tests/phase6-capstone-conformance.test.cjs's deleted-baseline read (caught by the same remote run, at a commit prior to this one) was already fixed in d0c3b1242 and is not touched here; verified still passing after these changes. * fix(#2724): revive ADR-1610's new-file size cap inside the differential An isolated review caught a real regression: deleting tests/workflow-size-baseline.json silently dropped NEW_FILE_CAP (ADR-1610 Decision point 3, the Codex project_doc_max_bytes anchor) with no successor. tests/helpers/emitted-diff.cjs's size ratchet already 'continue's past any file absent from sizeBaseline -- exactly the files this cap exists to bound -- so a brand-new workflow file sized 32,769-40,960 bytes passed CI clean and shipped, then risked silent truncation at the Codex anchor at runtime. ADR-1610 is Accepted and never referenced anywhere in this branch. Fix: NEW_FILE_CAP=32768 revived inside emitted-diff.cjs's own size-ratchet loop, keyed off the SAME hasOwnProperty(sizeBaseline, name) signal the growth check already computes -- 'new' is exactly 'present in sizeCurrent, absent from sizeBaseline'. Not ack-able, matching the tier hard caps it sits beside: the fix is extraction, not an acknowledgment entry. Documented, disclosed narrowing: the pure differential module cannot see XL_WORKFLOWS/LARGE_WORKFLOWS tiering (tests/workflow-size-budget.test.cjs's classification), so a legitimately large new file must extract rather than tier in, one release earlier than an existing file would need to. ADR-1610 itself is left unamended -- this restores its decision rather than re-litigating it. Also fixes a stale comment plus a redundant real 19-installer-spawn assertion left over from the pre-injection-seam version of tests/emitted-provenance.test.cjs's build-failure test, and annotates 3 of 4 stale golden-fixture citations in docs/reference/host-integration-capability-matrix.md as superseded (the 4th is an accurate historical PR narrative, left alone). * fix(#2724): repair three red CI defects on the golden-fixture cutover Windows-only provenance false attribution (defect A): the `hooks-built` provenance rule attributed `hooks/<name>.cmd` to itself. Those shims are Windows-only installer output (ensureCodexHooksJsonSessionStart / ensureCodexHooksJsonEvent, both in src/runtime-hooks-surface.cts) wrapping the same-named `.js` hook — no `.cmd` file is ever tracked in the repo, so the self-attribution resolved to a path that exists on no platform. Only windows-latest ever emits the key, so this only failed there. Fixed by special-casing `.cmd` inside the SAME `hooks-built` rule (not a dedicated rule) — a dedicated rule would match zero paths, and therefore report as a dead rule, on every non-Windows lane of the same totality guard. `sources` already supported per-match functions; `transforms` is extended to support the same shape so the attribution can vary by match within one rule. Baseline bootstrap was structurally impossible (defect B): `buildBaselineAtRef` ran `scripts/gen-emitted-baseline.cjs` from INSIDE the base-ref worktree, but that script is new in this PR and therefore absent at any base ref that predates it — every call failed closed with "Cannot find module". Fixed by running the PR checkout's own generator against the worktree via a new `--dir` parameter, decoupling "which copy of the script runs" from "which tree it measures" (`currentManifests`/`currentSizes` gained a `repoRoot` override, threaded down to `runMinimalInstall`'s new `installScript` override). This is not just a bootstrap fix: a differential needs ONE measurement schema applied to both sides, or the two stop being comparable the moment that schema evolves — running each side's own copy would silently reintroduce that risk. Verified locally end-to-end against real origin/next: resolves a valid {version, sha, manifests, sizes} artifact with the correct sha and no leaked worktree. Changeset placeholder (defect C): `pr: 0` -> `pr: 2767`, which is what let docs-lint evaluate the fragment for the first time; it already passes (docs/TESTING-SUITES.md and friends already document the removed scripts). Also fixed while in this file: an eslint no-unused-vars warning surfaced by the changed lint run (unused `cleanup` import in tests/emitted-provenance.test.cjs). Added regression coverage for both A and B: a cross-platform spot-check that drives the real hooks-built rule against `.cmd` keys directly (not through a real Windows install), and a real-tree test that drives buildBaselineAtRef against a base ref verified (via git cat-file) to lack the generator, both skipping honestly rather than false-passing when their precondition does not hold. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 * fix(#2724): repair false .cmd byte-provenance and a permanently-skipping regression test Two isolated-review findings on PR #2767: - `hooks-built`'s `.cmd` branch attributed the Windows shim's bytes to the wrapped `hooks/<name>.js` script, asserting a byte-provenance link that does not exist — traced against buildCodexHookWindowsShimIR (src/runtime-hooks-surface.cts), only the script's NAME (a literal in that same file) flows into the .cmd bytes, never its content. Point `sources` at HOOKS_WINDOWS_SHIM_SRC instead, matching the code-derived convention used elsewhere in the table. Since `sources` is checked before `transforms` in the differential, the wrong mapping silently excused any .cmd byte movement caused by editing the wrapped .js file. - The `buildBaselineAtRef` regression test skipped unless a resolvable base ref still lacked scripts/gen-emitted-baseline.cjs — true only until this PR merges, after which every base ref carries the file and the test skips forever with zero ongoing coverage. Rebuilt hermetically: synthesize the missing-generator condition in-place via git plumbing (a throwaway commit, child of HEAD, with just that one file removed from a scratch index), never touching the real working tree, HEAD, or index, and never depending on ambient history or remotes. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 * fix(#2724): tolerate the remote runner's dubious-ownership git mount in the emitted baseline path The runner container mounts the repo at a path owned by a different uid than the process running the suite, so git's dubious-ownership protection refuses every git operation there. GitHub Actions never hits this because actions/checkout registers the workspace as safe automatically; this runner's container does not. buildBaselineAtRef is the production build-fallback the sole remaining emitted gate depends on (resolveBaseline's in-job-build leg), not just a test helper, so the fix is in the shared git() wrapper (emitted-runtime.cjs) that every caller — resolveChangedPaths, resolveBase, buildBaselineAtRef's worktree add/remove/prune, and the hermetic regression test added in the prior commit — funnels through, plus gen-emitted-baseline.cjs's own rev-parse (now reusing that same wrapper instead of a second execFileSync, so the fix has one source of truth). Each call declares -c safe.directory=<the exact directory it already operates on>, never the * wildcard. Audited every other helper on this surface (emitted-diff.cjs, emitted-baseline.cjs, install-shared.cjs) for the same gap: none of them shell out to git at all. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01W5kQs6ZufZDySC6zDJfYP6 --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
a613caaeef |
enhance(#2721): regenerating merge driver, regen:derived, and a name for the emitted-artifact family (#2730)
* test(#2721): failing-first suite for the gsd-regen driver and CONTEXT.md parity Tests precede the implementation per the TDD gate. The driver module does not exist yet, so tests/git-merge-regen-driver.test.cjs fails at require time; the contributor-standards parity assertions fail against next as it stands today, where the standards doc names two CONTEXT.md headings that have never existed. Refs #2721 * feat(#2721): add the gsd-regen merge driver and regen:derived The golden parity manifests and the two size baselines are pure functions of the source tree, so their only correct merge is "recompute" -- something git's ours/theirs interface cannot express. 140 of 143 conflicted-file instances across the open PR queue are these files. The driver deliberately does NOT regenerate. Four probes established that at merge-driver time neither the working tree nor the index reflects the merge: both hold the ours side, a file added by theirs does not exist yet, and MERGE_HEAD is unwritten. Git also invokes the driver once per conflicted path (20 here). A regenerating driver would therefore read the ours-side tree and emit a plausible-but-wrong hash manifest -- worse than a conflict, because a conflict is visible. So it accepts %A, runs zero subprocesses, records the resolved paths, and prints one notice pointing at npm run regen:derived. Staleness stays caught where it already was, by golden-install-parity in CI. Every failure path degrades toward today's behaviour (a normal conflict). install-tree is deliberately excluded per ADR-2719 section 7. Also folded in, per the no-defer rule: workflow-size.cjs claimed .md files have no eol=lf in .gitattributes; git check-attr shows eol: lf, set by .gitattributes line 2 since #1088. Refs #2721 * docs(#2721): document regen:derived and the gsd-regen merge driver Adds the how-to a contributor actually reaches for when the generated parity manifests or size baselines conflict, in both places they would look: the merge-conflict path in CONTRIBUTING.md and the full guide in TESTING-SUITES.md, including what the driver deliberately does not do (it does not clear GitHub's CONFLICTING label, and it does not regenerate mid-merge). Also scopes the new contributor-standards parity assertion to the doc's own CONTEXT.md section. Its first run flagged `## Decision`, `## Consequences` and `## Standards followed`, which the doc attributes to an ADR body and a PR body rather than to CONTEXT.md -- a doc-wide extractor would have demanded CONTEXT.md grow headings that do not belong to it. Refs #2721 * fix(#2721): stop passing %P to the merge driver — shell injection The isolated adversarial review found, and I independently reproduced, local arbitrary command execution. Git does not invoke a merge driver with an argv array. It substitutes %O %A %B %L %P textually into the configured string and runs the whole thing through a shell, and $(...) executes inside POSIX double quotes -- so quoting the placeholder does not neutralise it. %O/%A/%B are git-generated temp names and %L is an integer, but %P is the file's own path, chosen freely by any contributor. A branch renaming a covered fixture to evil$(touch PWNED_SENTINEL).json executed that command on the machine of every maintainer who merged it, and the merge still reported success. Fix removes the input rather than filtering it: %P is no longer registered, so the driver receives no attacker-controlled argument at all. The marker records a count instead of path names. A metacharacter filter would have been a guess about shell grammar; passing nothing is a property. Re-ran the identical exploit against the fixed command: nothing executed, conflict still resolved. Two regressions guard it -- a platform-independent assertion that the registered command carries no %P, and a real merge driven by the actual planInstall output with a $(...) filename. Also from review: CLI dispatch had no coverage at all (CONTRIBUTING's "CLI and command routing" matrix), which is why runInstall/runStatus now take {repoRoot} -- hardcoding REPO_ROOT was what made them untestable. Renamed planResolution to resolveAndRecord since the plan* prefix promised purity it did not have. Reconciled the eleven-vs-twelve generator count across CONTEXT.md, CONTRIBUTING.md and the changeset. Refs #2721 * test(#2721): scope safe.directory for the check-attr helper The 66f4d85a run failed 11 assertions, all in the .gitattributes scoping block, with "fatal: detected dubious ownership in repository at '/work'". The test container checks the repo out at a path its user does not own, so git refuses check-attr outright. Everything else passed (27,185). `check-attr` is a pure read of .gitattributes -- no hooks, no filters -- so the exemption is scoped to that one invocation. It is deliberately NOT applied to the driver's own production `git config` calls, which run in the user's own clone and should keep the protection. Refs #2721 * test(#2721): delete the stale assertion that the driver command carries %P The plex2 run on bdfd0856 left exactly two failures, both this test: it still asserted the pre-fix command string, i.e. the vulnerable behaviour. Deleted rather than relaxed, per RULESET.TESTS.delete-bad-tests -- its useful half is already covered, in both directions, by registeredDriverCommandNeverPassesThePlaceholderForTheFilePath. Refs #2721 * test(#2721): drive the end-to-end merges from the real planInstall output The e2e helper hand-rolled its own driver registration, and still carried %P. That meant the five real-git tests were not exercising the production command string at all -- planInstall could drift and they would keep passing. They now register exactly what a contributor gets from npm run setup:merge-driver. Refs #2721 * chore(#2721): backfill changeset pr number to 2730 |
||
|
Tom Boucher
|
9a76ca6783 |
fix(#1882): distinguish unterminated frontmatter from absent frontmatter (#2712)
* fix(#1882): distinguish unterminated frontmatter from absent frontmatter
extractFrontmatter returned {} both for a document with no frontmatter and for
one whose fence was opened and never closed, so a file truncated mid-write was
byte-identical to a legitimate no-metadata file. Verified live through
`gsd-tools frontmatter get`: both printed {} with exit 0 and nothing on stderr.
Per ADR-1411's "corrupt is not absent" amendment the {} return is preserved
exactly -- no caller may break -- and the cause is surfaced out-of-band as a
deduplicated, unconditional stderr diagnostic. That mechanism lands as a shared
leaf module rather than a per-site copy because three sibling findings in the
same epic need it identically; four hand-rolled copies of one behaviour is the
generative-fix-divergence defect class.
The discriminator is deliberately not "opened but never closed". A Markdown
document whose first line is a thematic break takes that exact branch, so
flagging on the missing fence alone reports corruption on good Markdown -- the
failure mode this class of check has shipped with before. The unterminated
region is instead run through extractFrontmatter's own parser (extracted as
parseYamlRegion so the probe and the real parse can never diverge) and reported
only when it yields at least one key.
Also folds an inline defect found while working: src/config-loader.cts carried
two NUL bytes in the JSDoc added by this epic's Phase 1 (
|
||
|
Tom Boucher
|
90ba0ef10b |
docs(#2720): submit ADR-2719 — emitted-artifact attribution design contract (#2726)
Replaces the committed golden-install-parity hash manifests and per-file size baselines with a computed conservation law: every emitted path whose hash moves must be attributable, via a declarative provenance table, to a path the PR actually changed. Supersedes ADR-2264 Decision §2-§4 and its Amendment; ADR-2264 Phase 1 (the single-source buildParityManifest and exclusion constants) is retained and depended upon. Satisfies ADR-2264 AC1 rather than rewording it away, per that ADR's own 2026-07-17 audit. Docs-only. Both sides of the supersession edited together; ADR index regenerated with gen-adr-index.cjs --write. Closes #2720 Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
1008aabd31 |
fix(#2615): document the effortSurface axis in the host-integration matrix (#2698)
* fix(#2615): document the effortSurface axis in the host-integration matrix #2481 added `effortSurface` as the ninth negotiated `hostIntegration` axis and wrote documentation-sourced values into 18 descriptors, but never touched `docs/reference/host-integration-capability-matrix.md`. The matrix that ADR-1239 designates the cited source of truth had zero occurrences of the axis: no entry in the axes legend, and no row in any of the per-runtime tables. `src/host-integration.cts` states "every value is documented or explicitly 'undocumented'" — for this axis that was false for every runtime. Adds the legend entry (the `argv` / `none` / `undocumented` vocabulary, plus why there is deliberately no config-file member) and an `effortSurface` row to all 19 per-runtime tables. Every citation is carried over from #2481's own commit message, where the values were sourced: - claude argv -- `claude --help` documents `--effort <level>` - opencode argv -- `opencode run --help` documents `--variant` - codex argv -- `model_reasoning_effort` is a config.toml key, not a dedicated flag, so the generic `-c key=value` override is the only argv route (still argv) - 15 hosts undocumented -- their docs state no reasoning setting kimi-code is the nineteenth section (added by #2603 after #2481) and is the one runtime with no declared value. Its row and a Documentation-gaps entry record why rather than inventing one: Kimi Code documents `/effort` (alias `/thinking`), but only as an INTERACTIVE slash command — `-m, --model` is the only model-adjacent argv. Neither vocabulary member is accurate (`none` would deny a mechanism the host has, `argv` would claim one it does not expose), so closing that gap needs a vocabulary decision, which is a negotiation change and not a documentation one. The absent value already degrades closed exactly as the sentinel does. The regression test derives its runtime list from the registry rather than hardcoding it, so a runtime added later fails until its matrix row exists — the ratchet whose absence let #2481 add an axis with nothing catching the missing docs. Closes #2615 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * docs(#2615): honest citations for the undocumented rows; fix the four stale 8-axis lists Two findings from the orthogonal review of the first commit. 1. The 15 `undocumented` rows shared byte-identical text — "searched the runtime's official docs (see Sources consulted above)" — which is weaker than this file's own convention ("no authoritative doc — searched: <url>") and, worse, implies a per-host targeted search that did not happen: each section's Sources-consulted list was gathered for OTHER axes and contains no CLI-reference or reasoning-effort source. The rows now say plainly what the finding is — an ABSENCE established by #2481's cross-host survey — and cite that survey rather than implying a URL was checked per host. 2. Four normative docs still described "the eight negotiated axes" and omitted effortSurface entirely. The worst of them is docs/how-to/add-or-update-a-host-integration.md — the maintainer's own guide for onboarding a host, whose Step 2 axis table would have a maintainer reproduce exactly the gap #2615 exists to close. Also fixed: docs/reference/host-integration-interface.md (which calls itself the normative reference and had no effortSurface row at all), docs/how-to/author-a-host-plugin.md, docs/registries/README.md ("**exactly** the eight … axes keys"), and CONTEXT.md's matching EoS-registry sentence. Deliberately NOT changed, because they are historical records rather than current contract: docs/whats-new-1.7.0.md and docs/FEATURES.md's 1.7.0 entry (effortSurface shipped in 1.8.0 via #2481 — rewriting them would falsify the release history), ADR-1239's pre-amendment body (already superseded by its own "Amendment (2026-07-21): effortSurface axis (#2481)"), and ADR-1016's "original eight axes", which refers to a different axis set entirely. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * chore(#2615): backfill changeset PR number (#2698) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
3eb1cede26 |
fix(#1880): distinguish a corrupt config from an absent one (epic #1879 Phase 1) (#2688)
* test(#1880): prove corrupt config is indistinguishable from absent Failing-first. Encodes the issue's runtime repro: a trailing comma in .planning/config.json currently yields source:builtin-defaults with degraded:false - byte-identical to the file not existing - and the user's entire configuration is silently discarded. Asserts on the typed surface (CONFIG_REASON, _warnedUnusableConfig) rather than diagnostic prose, per the ADR-1411 amendment's test-methodology clause and CONTRIBUTING.md's raw-text-matching rule. IO failure is injected by monkeypatching fs.readFileSync and restoring in t.after(), never chmod 0o000 (root bypasses mode bits). Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1880): distinguish a corrupt config from an absent one loadConfigResolved wrapped the read, the JSON.parse and the entire config build in one try with one catch, so ENOENT, EACCES and SyntaxError all fell through to the same defaults and the branches returned degraded:false - actively asserting health over discarded configuration. A single trailing comma in .planning/config.json silently replaced the user's whole config, reporting source:builtin-defaults degraded:false, byte-identical to having no config file at all. ConfigResolution now carries a machine-readable reason. Genuine absence keeps degraded:false / not_configured; a file that exists but cannot be used sets degraded:true with config_unparseable or config_unreadable. The same split applies to the root config and to ~/.gsd/defaults.json. Control flow is deliberately unchanged. preflight_check reports cyclomatic 141 / cognitive 196 and 93 dependents on this function, with the guidance that small edits beat one big one, so faults are CAPTURED at the existing read sites and stamped onto the returns rather than the try/catch being restructured. Also carries the ADR-1411 amendment's wiring clause: loadConfig returns .config alone to ~51 call sites and would never see the new field, so an unusable file emits a deduplicated stderr diagnostic keyed on resolved path plus errno. Without it the reason would be an unreachable field and the user whose config was discarded would still get no signal - the actual defect. Registers the config-loader seam in lint-resolution-provenance, which until now guarded only agent-skills. Caller audit: ConfigResolution.degraded has exactly one consumer outside this module, cmdAgentSkills (src/init.cts:2259), which destructures {config, source, degraded} - adding a field does not break it. Its --json IR now reports degraded:true for a corrupt config, which is the intended fix and the one observable behavior change. Closes #1880 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1880): degrade when any config on the path is unusable, not just the last Two defects found by isolated adversarial review of the first cut. BLOCKER: the success-path return did not consult configFault. A corrupt ROOT config whose workstream override happened to parse returned degraded:false / reason:resolved - the root's settings silently dropped, which is the exact failure this issue closes, reappearing for any project using workstreams. The stderr diagnostic fired, so the out-of-band half worked while the in-band half reported a clean resolve; a --json consumer saw health. MAJOR: reason was derived from Object.keys(parsed) - the root+workstream MERGE - so an empty workstream file inheriting a non-empty root reported resolved despite carrying no settings. Emptiness is now judged on the file actually read, snapshotted before normalizeLegacyKeys mutates it. Also: corrects the ConfigResolution JSDoc, which still described the pre-#1880 degraded contract; adds a fast-check property asserting a PRESENT file is never reported not_configured whatever its bytes (CONTRIBUTING.md parser rule); and asserts the literal enum values so the provenance lint's configured_empty/not_configured markers check real assertions rather than incidental prose in test titles. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1880): reject valid JSON that is not a config object at the read seam The fast-check property added in the previous commit failed on both node lanes: a config.json containing 0, "str", [], null or true is valid JSON, so it parsed "ok", then threw downstream in normalizeLegacyKeys, and the outer catch reported not_configured - a PRESENT file reported as absent, which is precisely the collapse this issue exists to close. The property asserts a present file is never not_configured, and it caught it. _readConfigFile now validates shape, not just parseability (ADR-227: check the semantic shape at a trust boundary, not merely the type). A non-object JSON document is an unusable config, reported config_unparseable. Adds named regression cases for each non-object form alongside the property, so the class is documented and not only randomly sampled. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#1880): backfill changeset pr number (pr:0 -> 2688) Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2452): record a fetch-time shallow failure instead of crashing This guard failed CI on ubuntu-24 while passing on ubuntu-22 and windows-24 for the same commit, and passed on other PRs. Not a flake and not caused by the change under test - a real fragility in the test. runnerDiff ran the base fetch OUTSIDE its try and only guarded the diff, so it assumed the failure mode is always 'fetch succeeds, diff reports no merge base'. At a shallow boundary that lands short of the merge base, git can instead fail during the FETCH ('unable to parse commit' - the boundary commit's parent is not available). Which stage git fails at is version and transport dependent, so on some runners the error escaped runnerDiff and crashed the test rather than being recorded as the ok:false the assertions expect. Both stages mean the same thing for what this guard protects: a shallow base ref cannot resolve the three-dot diff. Also drops two assert.match calls against git's stderr prose. 'no merge base' and 'unable to parse commit' are the same condition reported at different stages, and CONTRIBUTING prohibits raw text matching on subprocess output. The typed outcome (ok === false) is the contract; the tests now assert that plus the presence of a cause. Found while investigating the red lane on #2688; fixed here per the no-defer rule rather than filed. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c07734216f |
fix(#2603): document kimi-code in the host-integration capability matrix (#2687)
* fix(#2603): document kimi-code in the host-integration matrix; correct 3 inherited axes The matrix — ADR-1239's deployment source-of-truth — had a section for 18 of 19 installed runtimes but none for `kimi-code`, so its `hostIntegration` axes shipped with no citation and no evidence quote. Sourcing every axis independently against Kimi Code CLI's own docs (the issue's explicit requirement — `kimi` and `kimi-code` are distinct products) showed three values had been inherited from the Python `kimi` descriptor rather than sourced: - `embeddingMode` imperative -> declarative. Kimi Code plugins are a `kimi.plugin.json` manifest plus markdown Skills with no in-process programmatic API (docs/en/customization/plugins.md) — the same shape as `codex`. - `dispatch.nested` false -> true. The `coder` built-in "can dispatch its own nested sub-agents when a task decomposes naturally" (docs/en/customization/agents.md). The Python `kimi` CLI genuinely prohibits nesting; Kimi Code does not. - `dispatch.maxDepth` 1 -> "undocumented". Nesting is documented but no depth bound is published, so the fail-closed sentinel applies over a guessed integer. `dispatch.namedDispatch` deliberately stays `false`: GSD's kimi-code artifact layout installs Agent Skills only (no `agents` kind), so no named GSD subagent is registered with the host and `resolveDispatchType` maps every role onto coder/explore/plan. Flipping it would reintroduce the dispatch failure recorded in docs/migration/kimi-to-kimi-code.md. The matrix records the host-capability nuance under Documentation gaps instead. Behaviourally inert: `namedDispatch:false` already caps nested/maxDepth/background/ backgroundDispatch to false/0 in the effective axes (host-integration.cts:493-499), and the install adapter is not selected by `embeddingMode` (install.js:543 always uses the imperative adapter). The one visible effect is the curated profile pin, which moves programmatic-cli -> declarative-cli. Also fixes the axes legend, which omitted the `built-in-only` subagentToolkit member that has been in the closed vocabulary since kimi-code shipped. Same defect class and countermeasure as #2598: pin the corrected values and require the matrix to agree with the descriptor, because a descriptor/matrix disagreement is how the gap survived. Closes #2603 Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * fix(#2603): report the maxDepth `undocumented` sentinel as a sentinel, not as malformed Surfaced by the orthogonal review of this change. `negotiateHostCapabilities` emits a sentinel-specific warning for every dispatch sub-axis carrying the documented `undocumented` value — namedDispatch, nested, background, subagentToolkit, backgroundDispatch, isolation — except `maxDepth`, which fell through to the numeric guard and reported `host dispatch.maxDepth is missing or not a number — treating as 0`. That message is indistinguishable from a genuinely malformed descriptor, so a correctly fail-closed descriptor reads as broken. Six shipped runtimes carry the sentinel here (antigravity, augment, opencode, trae, windsurf, zcode) and this PR's kimi-code correction adds a seventh, which is why it is fixed here rather than left in place. The numeric guard keeps firing for genuinely malformed values; both paths still degrade `effective.dispatch.maxDepth` closed to 0. Covered by three tests, including the boundary case that the sentinel carve-out must not swallow a real malformed value. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01QT3ibz5qJuDuGqpTGRYVGf * chore(#2603): backfill changeset PR number (#2687) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
a3853472de |
fix(#2598): declare OpenCode subagent dispatch synchronous, not background (#2682)
* fix(#2598): declare OpenCode subagent dispatch synchronous, not background capabilities/opencode/capability.json advertised dispatch.background: true and dispatch.backgroundDispatch: true. negotiateHostCapabilities and every degradationFor / shouldFlattenDispatch consumer trusts these per-field values, so declaring a capability the host lacks OVERSTATES it — the opposite of the fail-closed posture the negotiation is built for. The issue's own citations needed checking before acting: the host-integration matrix (ADR-1239's designated deployment source-of-truth) documented `true` with NEWER evidence than the issue cited, and explicitly marked the issue's sst/opencode#5887 reference as a stale snapshot superseded by #2087. git log confirms #2087 deliberately flipped these from false to true, citing OpenCode v1.15.0/v1.17 as "background subagents enabled by default in all modes". Applying the issue as filed would, on that evidence, have REGRESSED a deliberate update. So the claim was verified against current upstream rather than either document. `packages/opencode/src/effect/runtime-flags.ts` on `dev` today reads: experimentalBackgroundSubagents: enabledByExperimental("OPENCODE_EXPERIMENTAL_BACKGROUND_SUBAGENTS") `enabledByExperimental` falls back to the `experimental` flag and `bool()` defaults to false — the parameter is hidden from the model unless an operator opts in by env var. Upstream #29638 is still OPEN and confirms the session loop `tasks.pop()`s one subtask at a time. #2087's "default-on in all modes" reading does not hold against current dev. The issue's CONCLUSION is therefore right even though part of its evidence was superseded: concurrent dispatch cannot be relied on, so both fields are false. The matrix rows are corrected with the verified citation rather than reverted to the old #5887 quote, so the record shows why the value is false TODAY rather than re-asserting evidence that was legitimately superseded. Neighbouring sub-fields are untouched and pinned by test: namedDispatch, subagentToolkit, and isolation:'orchestrator-worktree' (which works via `opencode run --dir` at the OS process level and is unaffected — #2584 does not depend on this value either way). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * fix(#2598): re-pin the dispatch contract tests to synchronous OpenCode dispatch gsd-test on the descriptor change came back FAILED (5 unique, both node versions). The failures were not incidental — they were deliberate contract-pin tests encoding #2087's decision, one named literally "background UPGRADE": tests/host-integration-descriptors.test.cjs - EXPECTED_FLATTEN[opencode] === false (background-eligible) - the derived background-eligible set pin tests/opencode-imperative-reference.test.cjs - "descriptor declares background dispatch true/true (v1.15/v1.17 upgrade)" - "background UPGRADE changes shouldFlattenDispatch: false now" So this is a recorded decision being reversed, not drift being corrected, and it is reversed on evidence: current upstream `dev` gates the capability behind OPENCODE_EXPERIMENTAL_BACKGROUND_SUBAGENTS (default false) and upstream #29638 (OPEN) confirms the session loop still handles one subtask at a time. The issue is filed by the maintainer and explicitly directs "update golden-parity / validator fixtures as needed", which sanctions re-pinning. Behavioral consequence, verified: shouldFlattenDispatch(opencode) now returns TRUE, so GSD serializes opencode dispatch instead of trusting concurrency it cannot get. That is the correct fail-closed direction and is safe today — no shipped GSD flow drives OpenCode background waves (per the issue), and isolation:'orchestrator-worktree' is unaffected because it works at the OS process level via `opencode run --dir`, not via the native subagent. Each re-pinned test now asserts the retracted contract in the opposite direction — feeding the #2087 axes back in must still yield "would not flatten" — so a silent re-flip of either field is caught rather than merely un-asserted. lint:ci exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * chore(#2598): backfill changeset pr number (#2682) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
a5633bb32f |
enhance(#2671): brand raw vs calibrated token types so double-application is a compile error (#2676)
* test(#2671): add failing-first brand-typing compile fixtures * feat(#2671): brand raw vs calibrated token types * refactor(#2671): hoist type-compile into a before() hook Two review responses: - The fixture compile ran in the describe() body, so it executed at collection time even when the block was filtered out, and a failed precondition collapsed eight independent assertions into one opaque describe-level failure. A before() hook is this repo's documented idiom and preserves per-test granularity. - parseTokensFlag now records WHY it returns an unbranded number: it validates the magnitude of --tokens, but the basis is decided by --calibrated, so branding here would be wrong for half its callers. The assertion belongs to cmdEstimateCheck, its only caller. * test(#2671): pin each brand diagnostic to its OFFENDING marker Adversarial review demonstrated that asserting only exactly-one-diagnostic- at-code-N is not airtight. Repairing a fixture's brand violation while injecting an unrelated error of the same code (a string passed as the budget argument) still yielded exactly one TS2345, so the fixture would have reported green while no longer testing its regression at all. Each bad-* fixture now routes its violating value through a const named OFFENDING, and the test asserts the diagnostic's start offset falls inside that node — located through the AST, so it survives reformatting and never pattern-matches source text. Replaying the proof-of-concept against the new assertion rejects it: the diagnostic lands on the budget literal, not the marker. Also corrects a doc comment that claimed the program type-checks all of src/; it covers phase-estimation.cts and its transitive dependencies. * chore(#2671): backfill changeset PR number (#2676) |
||
|
Tom Boucher
|
0d08c32048 |
fix(#2590): emit Workflow scripts the Workflow tool accepts; make the backend reachable (#2681)
* fix(#2590): emit Workflow scripts the Workflow tool accepts; make the backend reachable Every emitted script was rejected. Four invalid constructs, the first fatal on its own, so the Workflow backend could never dispatch a wave: 1. no `export const meta = {…}` first statement -> whole script rejected 2. resumeFromRunId("<id>") -> "resumeFromRunId is not defined". It is a Workflow TOOL INPUT parameter, not a script function. The run id still reaches the caller via summary.resumeRunId, to pass as that input. 3. budget(<n>) -> "budget is not a function". `budget` is a read-only object { total, spent(), remaining() } fed by the caller's token directive; a script cannot set it. Recorded as intent in a comment. 4. parallel(agent(…), agent(…)) -> "parallel() expects an array of functions". Now parallel([() => agent(…), …]) — passing agent() results directly also started every agent eagerly, before parallel() could bound concurrency. The single-plan stage had its own branch with the same parallel() defect; both branches are now one array-emitting path. Waves also emit phase() calls whose titles match meta.phases exactly, so progress groups correctly. Two secondary defects kept the script from ever being REACHED — which is why this shipped undetected: 5. NOTHING resolved the Agent SDK version. The fragment claimed there was "no scriptable way" to introspect it and told callers to omit the flag, so gate 5 returned agent_sdk_version_unknown on every automated run while `capability state` still reported active:true. True for bash, false for Node: the router now reads the installed @anthropic-ai/claude-agent-sdk version, walking node_modules up the tree and reading package.json directly — require.resolve throws ERR_PACKAGE_PATH_NOT_EXPORTED because the SDK's exports map does not expose ./package.json. Precedence: explicit flag > GSD_AGENT_SDK_VERSION > installed. Fail-closed is preserved; an unresolvable version still declines to inline. A too-old SDK now reports the truthful agent_sdk_version_below_floor instead of unknown. 6. The runtime fallback was `--runtime > GSD_RUNTIME > 'unknown'`, diverging from the canonical `GSD_RUNTIME > config.runtime > 'claude'`, so any invocation without --runtime reported runtime_not_claude on an ordinary Claude project. Now delegates to runtime-slash.resolveRuntime. The fragment's `${AGENT_SDK_VERSION:+--agent-sdk-version "$AGENT_SDK_VERSION"}` snippet is removed rather than repaired: it was also shell-dependent — zsh does not word-split unquoted parameter expansions, so it collapsed to a single argv element, argValue() never matched, and the run failed into the same agent_sdk_version_unknown, indistinguishable from genuinely unknown. Auto- resolution removes the need for the construct entirely. Verified with the issue's own repro: no flags now reaches the version gate; an SDK above the floor yields backend:"workflow" with a script that parses as a real ES module. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * fix(#2590): sync generated registry, repair sibling tests, reject duplicate wave ids Findings from the isolated review, all fixed. HIGH — gsd-core/bin/lib/capability-registry.cjs was stale, and `lint:ci` was already RED because of it. The registry embeds the fragment text INLINE, so the shipped/installed copy still taught the exact broken contract this PR fixes: the old `${AGENT_SDK_VERSION:+…}` bash line and the "OMIT the flag when unknown" guidance. Regenerated. (I had read `lint:ci` by grepping its output instead of checking its exit code, so I recorded a red chain as green — checking $? now.) HIGH — three existing tests asserted the OLD broken shape and would have failed CI; none was touched by the first commit: tests/fix-2285-claude-orchestration-wiring.test.cjs — matched resumeFromRunId("…") tests/claude-orchestration.test.cjs — .includes('budget(') tests/claude-orchestration-command-router.test.cjs — .includes('budget(') Each now asserts the corrected contract: the id/pool reaches the caller via summary, and neither construct is ever CALLED. Two sibling assertions had also gone vacuous — `.includes('resumeFromRunId')` still passed, but only because the new explanatory COMMENT contains that substring, not because anything is wired. Rewritten to assert the real property. MEDIUM — duplicate wave ids were never rejected. Plan-id uniqueness was checked within a wave, but nothing checked wave ids across waves. That was harmless before; it is not now, because each wave emits a `phase("Wave <id>")` call plus a matching meta.phases entry and the tool matches titles by exact string — two waves sharing an id would collapse into one progress group and misattribute the second wave's agents to the first. Rejected at validation, with tests either side of the boundary. MEDIUM — the fragment contradicted itself (its "Manifest construction" header still listed $AGENT_SDK_VERSION as orchestrator-built) and, more seriously, never told the orchestrator to pass summary.resumeRunId as the Workflow tool's resumeFromRunId INPUT. Since this PR moves resume from a broken in-script call to a tool-invocation input, an implementer following only the fragment would have silently regressed phase-resume to a no-op. Both fixed. MEDIUM — docs/how-to/enable-claude-orchestration-workflow-backend.md and docs/explanation/claude-orchestration-capability.md documented `resumeFromRunId("<id>")` and `budget(<tokens>)` as current correct output — teaching the bug as the feature. Updated to the real contract, including the required meta block and the thunk-array parallel() form. (The changeset is `Fixed`, so the docs gate exempts this; it is corrected because it is wrong, not because a gate demanded it.) LOW — the router's top-of-file comment still described the divergent `--runtime > GSD_RUNTIME > 'unknown'` chain as current, ninety lines above the fix; and inserting resolveInstalledAgentSdkVersion had orphaned resolveDetectionArgs' JSDoc above the wrong function. Both repaired. lint:ci now exits 0 (verified by exit code, not by reading output). Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * chore(#2590): backfill changeset pr number (#2681) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
c3958018dd |
docs(#2674): amend ADR-1411 — corrupt is not absent (epic #1879 Phase 0) (#2678)
* docs(#2674): amend adr-1411 with the corrupt-is-not-absent house pattern ADR-1411 reasons only about a resolution miss. It is silent on input that is present but not usable, which is how five engine read paths (#1879) could fold an unusable input into the value meaning 'genuinely absent' without contradicting an Accepted ADR. Read together, ADR-1411 and ADR-227 converge and do not license throwing as the cluster's answer: ADR-227 requires malformed input to be coerced rather than propagated and carves out only genuinely-fatal fields, while ADR-1411 already permits a fallback provided it is 'a visible value, not a silent substitution'. The defect in these five sites is therefore not that they fall back but that they fall back invisibly. Records the pattern that follows: every current return value is preserved, and the cause is made visible in-band where the result already carries a provenance envelope, or out-of-band via a deduplicated stderr diagnostic where it returns a bare value it cannot extend. Throwing stays confined to ADR-227's genuinely-fatal carve-out, decided per call. Also names the per-applier caller audit and the lint-resolution-provenance registry gap. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#2674): prove the warning-state reset misses the unknown-key dedup set The two existing cases in this suite only pass because each picks a key name no other case reuses, so neither can observe whether the reset the beforeEach calls actually runs. Failing-first: asserts the exported _warnedUnknownConfigKeys is empty after _resetRuntimeWarningCacheForTests(). It is not - the helper clears only _warnedConfigKeys despite documenting itself as resetting per-process warning state. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2674): reset the unknown-key dedup set with the runtime warning cache _resetRuntimeWarningCacheForTests documents itself as resetting per-process warning state but cleared only _warnedConfigKeys, leaving _warnedUnknownConfigKeys populated across cases. The suite that exists to test that set - 'loadConfig - unknown-key warning dedup' - calls the helper in beforeEach expecting exactly this, so the reset was a silent no-op for it; both cases passed only because each picked a key name the other never reused. Any later case reusing a key would have had its warning suppressed by leaked state. Found while amending ADR-1411, which names this dedup guard as the pattern five downstream PRs (#1880-#1884) will adopt - shipping the ADR without the fix would have propagated the footgun to each of them. Folded in here per CLAUDE.md's no-defer rule rather than filed. RED verified on 3c4895841 (test only, no fix): linux-node22 reported 'FAIL tests/config-loader.test.cjs - the documented per-process warning-state reset must clear the unknown-key dedup set too'. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2674): document src/ in the changeset-lint trigger list CONTRIBUTING.md presented the Changeset Required trigger list as bin/, gsd-core/, agents/, commands/, hooks/, sdk/src/ - omitting src/, which scripts/changeset/lint.cjs has in USER_FACING_PREFIXES. src/ is the TypeScript source of truth compiled into gsd-core/bin/lib/*.cjs, so it is the most-edited user-facing path in the repo and the omission sends any contributor who touches it into a CI failure the doc says cannot happen. Also documents that the lint reads GITHUB_BASE_REF, which only CI sets, so running it bare locally reports success without evaluating the branch. This PR hit exactly that: a local run said ok_fragment_present and CI failed fail_missing_fragment on the same diff. Found while opening this PR; folded in per the no-defer rule. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2674): add Fixed changeset for the src/ trigger-list and reset fixes Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#2674): restore the round-2 review corrections to the amendment These edits were made in response to the second isolated review pass but never staged: later commits used targeted `git add <file>` for the test and the source fix, so the two markdown files stayed dirty and shipped nothing. The branch carried the round-1 text, including the ADR-227 misquote the reviewer raised as a blocker. Restores: the unconditional-diagnostic clause (ADR-227's GSD_DEBUG opt-in was never implemented, so citing it as the precedent was wrong), the dedup key, #1882 folded into the out-of-band mechanism instead of a fourth mechanism-less category, the narrowed caller-audit rationale, and the test-methodology clause. Refs #1879 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
c87f6f358e |
enhance(#1854): offer restore for user-added files backed up on update (#2679)
* test(#1854): failing-first coverage for user-files-backup restore Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * feat(#1854): offer restore for user-added files backed up on update Adds a restore-custom-files gsd-tools verb and wires it into update.md as a restore_custom_files step: plan, compatibility-check against the newly installed release, then restore only on explicit opt-in. The backup is never deleted, a shipped path is never overwritten, and a single unwritable entry does not abort the rest. Also drops the jq pipe from update-context field extraction (#2589 class, missed by that sweep) and repairs a broken code fence in docs/CLI-TOOLS.md. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1854): reject symlinked restore destinations and backup roots Self-review of the restore path found two write-through holes: copyFileSync follows a symlinked destination, so a link planted at the restore target wrote outside the config dir with every ancestor still a real directory; and statSync on the backup root followed a link, letting the walk read arbitrary files and present them as the user's own backup. Both now lstat. Also marks the report's path/detail strings as untrusted data in update.md so the rendered step cannot carry instructions into the runtime model. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#1854): move the update-context jq guard into the #2589 sweep update.md joins the AUDITED list rather than carrying a duplicate assertion in the backup-restore suite, and the guard gains a negative-proof companion so 'no jq pipe' cannot pass by the fields simply no longer being read. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1854): validate manifest files map shape before trusting it Security review flagged that Object.keys on a non-plain-object files field yields numeric-index keys matching nothing, so the managed-path check dies silently while manifest_found still reports true. Shape, not just type (ADR-227): an array or scalar files map is now an unusable manifest. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1854): size the restore prompt by eligible_count Spec review found the prompt was driven by entries.length, so a backup holding only blocked entries asked "Restore 1 file(s)?" when accepting would restore zero. The question now reads eligible_count, and an all-blocked backup reports its reasons instead of offering a choice that cannot be honored. The decline path names the resolved backup_dir rather than the bare directory name. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * test(#1854): use t.skip on hosts without symlink support A bare return in a node:test body registers as a PASS, so the four symlink guards silently reported green on unprivileged Windows instead of skipping. Adds the dangling-link destination case the security review called out, and moves outside-dir teardown to t.after so a failing assert cannot leak it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#1854): unfence restore hint, regen goldens, widen install timeout Three gate failures from the c99d612a5 run, all root-caused: 1. capability-registry (3): update.md's decline message put an instructional 'gsd-tools ...' line in an UNTAGGED fence, and the guard treats untagged fences as shell blocks. Retagged both display blocks as text and switched the hint to the resolved 'node <config-dir>/.../gsd-tools.cjs' form users can actually paste. 2. golden-install-parity (19): update.md and gsd-tools.cjs ship, so every runtime fixture moved. Regenerated; the diff is exactly those two hashes per fixture, no other drift. 3. install.test.cjs (5): one real failure, four cascades. The Cursor suite's before hook died on 'spawnSync ETIMEDOUT' at the 60s cap while the node22 lane passed the SAME commit in 12.7s. A full install measures 13-30s idle, so 60s was under 2x headroom and shrinks with every file added to the payload. Raised to 120s, matching the heavy case already in this file. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#1854): backfill changeset pr number to 2679 --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
bd570618d4 |
feat(#2632): executor actuals and the closed estimate-calibration loop (#2672)
* feat(#2632): record executor actuals and close the estimate calibration loop * fix(#2632): calibrate against the raw projection so the loop converges * test(#2632): add closed-loop convergence guard and codify the feedback-loop rule * fix(#2632): pair calibration samples per plan; atomic write; amend adr * chore(#2632): backfill changeset pr to 2672 * fix(#2632): retry renameSync on transient windows errnos and clean up the temp |
||
|
Tom Boucher
|
920a5f3f06 |
fix(#2589): use --raw/--pick for config/model/verify lookups, drop jq dep (#2673)
* fix(#2589): use --raw/--pick for config/model/verify lookups, drop jq dep The reviewer/workflow config lookups resolved scalars and object fields with a `gsd_run query <cmd> … | jq … 2>/dev/null || <default>` shape. On any machine without jq (the default on Windows/Git-Bash) the jq stage fails with exit 127, the failure is swallowed by 2>/dev/null + the trailing || default, and the variable comes back EMPTY — the configured per-lane model/host/budget is silently dropped and the lane falls back to CLI defaults with no diagnostic. gsd-tools ships native flags that do the same job with no external dep: config-get <key> --raw (strips JSON quotes off a scalar) resolve-model <id> --pick model (descends an object) resolve-execution … --pick <f> (same) verification.status … --pick status Replaced every jq-piped config/model/verify lookup across review.md (×23), plan-phase.md, ship.md, debug.md (incl. the redundant boolean coercion — --raw returns true/false as bare tokens natively), autonomous.md (×2), ai-integration-phase.md (×4), and eval-review.md. The legitimate structured-JSON jq sites that parse HTTP curl responses (.choices[0], jq -rs, jq -n --rawfile) are untouched — only the jq-replaceable lookups moved to the native flags. Adds tests/fix-2589-config-get-no-jq.test.cjs: a source-invariant guard asserting no audited workflow pipes config-get/resolve-model/resolve-execution/verification.status to jq (fails-first on the pre-fix text, passes after). * test(#2589): update autonomous-converge jq assertion to --pick; regen golden fixtures Two test consequences of the workflow-doc edits in the prior commit: 1. tests/autonomous-converge.test.cjs pinned the OLD jq-dependent shape (`verification.status … | jq -r '.status//empty'`) as the canonical routing contract. The test's INTENT is correct (route human validation through canonical verification.status) but it over-specified the MECHANISM (the jq pipe). Updated the assertion to match the new native --pick status shape; the contract being guarded (canonical verification.status read before the human_needed branch) is unchanged. 2. The golden-install-parity fixtures (19 runtimes) record a content hash of every installed workflow .md; the 7 edited workflows changed those hashes. Regenerated via `npm run gen:golden` (the test's own failure message instructs this). Only the 7 edited workflow hashes changed in each fixture. * fix(#2589): declare jq a prerequisite for the lanes that still need it; repair test file Three defects in the first cut of the #2589 fix: 1. tests/autonomous-converge.test.cjs was a JavaScript syntax error. The regex literal /...2>\/dev/null .../ left the second slash unescaped, terminating the literal early and parsing `null` as regex flags: SyntaxError: Invalid regular expression flags The whole file failed to load, so every assertion in it — including the #1522 and #1526 guards — silently stopped running. Replaced with the string-compare form already used at line 202 for the sibling shell-snippet assertion. 2. lint:ci failed. tests/fix-2589-config-get-no-jq.test.cjs buckets into the capped `config` production module via its `config-get-...` effective prefix, making it a novel offender against the 2-file cap. The test is about workflow documents, not the config module, so it is renamed to fix-2589-workflow-jq-dependency.test.cjs (free prefix) rather than growing the allowlist with a module that does not actually need a 5th test file. 3. The fix deleted the repo's only jq-prerequisite declaration. review.md:244 ("install jq if missing") was the anchor plan-review-convergence.md cites by line number, and it went away with the jq pipes — while the ollama, lm_studio, llama_cpp, opencode, and agy lanes still hard-require jq to parse HTTP /v1/chat/completions responses, opencode's JSONL event stream, and agy's conversation cache. On a jq-less host those five lanes swallow exit 127 into empty output: the same silent-degradation class #2589 exists to close. detect_clis now probes jq alongside the other prerequisites and emits jq:available / jq:missing, and the five dependent lanes are treated as undetected when it is absent, with an install hint. The six lanes that do not need jq stay selectable. plan-review-convergence.md now cites the section by name instead of a line number that moves. Regression guards added to the renamed test file: review.md must keep the jq probe and must name all five dependent lanes, and no workflow may cite review.md by line number. Workflow-size baseline and the 19 install-parity goldens regenerated for the review.md / plan-review-convergence.md edits. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * fix(#2589): decide the ship verification gate on a single verification.status read Isolated review finding (medium). Pre-fix, ship.md captured verification.status ONCE into $VERIFICATION and picked status / next_action / next_command off that cached JSON with three jq calls. --pick takes a single dot-path field, so the mechanical conversion issued three separate queries up front: three node spawns that each re-read the phase VERIFICATION.md and re-derive the commit-time vs mtime staleness comparison, on every ship — including the common passing path that never uses the two message fields. It also meant the gate's verdict and the message shown to the user were derived from three reads with no guarantee they observed the same state. The gate now reads `status` once and decides. The two message-only fields are read on the blocking path only, after PHASE_VERIFICATION_INCOMPLETE is already determined — so the passing path costs one query instead of three, and a concurrent write between reads can no longer make the gate and its message disagree, because the block/allow decision no longer depends on them. Adding a multi-field --pick to gsd-tools would have collapsed this to one query, but that changes the flag's output contract and belongs in its own change. Regression guard in tests/fix-2589-workflow-jq-dependency.test.cjs: ship.md must read verification.status exactly three times total, the block decision must follow the status read, and next_action / next_command must both appear after the blocking prose so they cannot drift back onto the passing path. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * docs(#2589): document the jq prerequisite for the five reviewer lanes that need it /gsd-review's ollama, lm_studio, llama_cpp, opencode, and agy lanes parse JSON GSD does not produce (OpenAI-compatible /v1/chat/completions responses, OpenCode's JSONL event stream, Antigravity's conversation cache), so they require jq on PATH. Nothing in docs/ said so. Records which five lanes need it, which six do not, that reading configured models/hosts/budgets no longer requires jq at all, and what /gsd-review now does when jq is absent. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015TCwhbMuY37DzRMCfzTABJ * chore(#2589): backfill changeset pr number (#2673) --------- Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
89b673e40d |
feat(#2631): planner emits estimate and plan-checker surfaces the over-budget flag (#2670)
* test(#2631): failing-first planner estimate emission and over-budget surfacing * feat(#2631): emit plan estimate and surface the over-budget split recommendation * fix(#2631): extract sizing prose to references to fit planner and plan-phase caps * fix(#2631): move estimate check to plan-checker; fix template regex and caps * fix(#2631): restore ALWAYS split literal and keep gsd_run after the launcher preamble * fix(#2631): invoke estimate-check after the launcher preamble in plan-checker * fix(#2631): stop double-applying calibration; repair COMMANDS table and stale reference * chore(#2631): backfill changeset pr to 2670 * chore(#2631): backfill changeset pr to 2670 |
||
|
Tom Boucher
|
46ba02acde |
feat(#2630): phase-estimation module, smart-zone config key, and cli verbs (#2661)
* feat(#2630): add phase-estimation module, smart-zone config key, and cli verbs * fix(#2630): document smart_zone_tokens, refresh golden fixtures, fix null-proto property assertions * fix(#2630): align smart_zone_tokens write/read validation and harden estimation tests * chore(#2630): backfill changeset pr to 2661 |
||
|
Tom Boucher
|
6ee4349272 |
fix(#2537): extract offer_next step to references/ (~3.3KB headroom restored) (#2642)
* fix(#2537): extract offer_next step to references/ (~3.3KB headroom restored) * chore(#2537): backfill changeset pr to 2642 |
||
|
Tom Boucher
|
6ad30f74b6 |
feat(#2584): Phase 3 — scheduler consumer + isolation adapters (#2635)
Final phase of #2584 (ADR-1239 Codex-binding amendment). execute-phase now negotiates dispatch.isolation and dispatches through the matching adapter, so a wave's independent plans run concurrently on six runtimes instead of one — with no runtime=== branch in the scheduler. harness-worktree passes the host's declared isolation flag (claude, cursor); orchestrator-worktree creates the worktree via the Phase-2 verb and spawns the executor into it with the resolved argv/cwd (codex, opencode, kimi, kimi-code); none stays sequential. Undeclared/unknown/unresolvable isolation degrades to none — never an unisolated parallel run. Fixes two shipped Phase-2 descriptors that per-host research found would fail at spawn: kimi lacked its headless flag (would launch the interactive TUI and hang the orchestrator), and kimi-code named a non-existent binary (Kimi Code installs as 'kimi'). Adds the worktree-path root confinement Phase 2 deferred here, and leading-dash guards on the resolver's prompt/cwd matching the existing git-argument guard. Closes #2627 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
1a9ae7601b |
docs(#2629): adr — phase effort estimation & calibration design lock (#2636)
* docs(#2629): adr — phase effort estimation & calibration design lock * docs(#2629): annotate phase-0 status and link adr cross-reference * docs(#2629): derive estimate confidence from sample count, not self-rating |
||
|
Tom Boucher
|
57b2bd8368 |
fix(#2491): finish todos/done -> todos/completed rename (14 stale refs) + guard (#2626)
* test(#2491): add todos/done rename under-sweep guard * fix(#2491): finish todos/done -> todos/completed rename (14 stale refs) * chore(#2491): backfill changeset pr to 2626 * test(#2491): fix lint-legacy-dir-name + allow-test-rule-refs (split legacy token, add issue ref) |
||
|
Tom Boucher
|
0f46fa366f | docs(#2606): ratify ADR-612 getMilestoneFromPhaseId bracket return-form (vN.0) (#2607) | ||
|
Tom Boucher
|
ec7978c0b4 | feat(#2584): add dispatch.isolation sub-field, descriptors, validator + negotiation (#2604) | ||
|
Rezolv
|
155c08facf |
docs(#2197): drop --validate docs for /gsd-plan-phase and /gsd-execute-phase (#2574)
* docs(#2197): drop --validate docs for /gsd-plan-phase and /gsd-execute-phase These two commands never parse --validate (silent no-op); the flag is real only for /gsd-quick. Remove the false flag-table rows and CLI examples across COMMANDS.md and the how-to guides (en + ja-JP/zh-CN/ ko-KR/pt-BR mirrors), and correct the manager.flags.execute example from --validate to --cross-ai (a flag execute-phase actually parses). /gsd-quick's real --validate docs are left untouched. Ref #2197 * docs(#2197): add changeset for --validate docs removal --------- Co-authored-by: CI Rebase Check <ci@gsd-redux> |
||
|
Tom Boucher
|
be3bf97eff |
docs(#2584): ADR-1239 Codex-binding amendment + dispatch.isolation capability (Phase 0) (#2600)
* docs(#2584): add ADR-1239 Codex-binding amendment + dispatch.isolation capability * chore(#2584): backfill changeset PR number (#2600) |
||
|
Tom Boucher
|
77bf21b3a6 |
fix(#1995): widen worktree branch regex to accept agent-<id> namespace (#2548)
* test(#1995): regression test for agent-<id> branch namespace Add failing-first tests proving that normalizeCleanupManifestEntry and planWorktreeRecordAgent reject Claude Code's current agent-<id> isolation branches (only worktree-agent-<id> is accepted). Boundary tests cover both namespaces plus rejection cases. * fix(#1995): widen worktree branch regex to accept agent-<id> namespace Claude Code's isolation="worktree" branch naming changed from worktree-agent-<id> to agent-<id>. Widen the regex in all 7 locations from ^worktree-agent-[A-Za-z0-9._/-]+$ to ^(worktree-)?agent-[A-Za-z0-9._/-]+$ so both namespaces are accepted. Introduce a shared WORKTREE_AGENT_BRANCH_RE constant in src/worktree-safety.cts to prevent future drift. Closes #1995 * fix(#1995): update workflow guards, test assertions, and baselines Widen the branch-check regex in execute-phase.md and execute-plan.md. Update all test assertions that checked for ^worktree-agent- to expect the widened ^(worktree-)?agent- pattern. Regenerate golden-install-parity fixtures, agent-size-baseline, and workflow-size-baseline. Closes #1995 * fix(#1995): update extractCwdGuardBash sanity check for widened regex The e2e test's sanity check verified the extracted bash block contained 'worktree-agent-'. After widening to '(worktree-)?agent-', update the check to match the new pattern. * fix(#1995): widen missed workflow-guard branch check + changeset + lint fixes - hooks/gsd-workflow-guard.js: widen startsWith('worktree-agent-') to /^(worktree-)?agent-/ regex — same defect class, was missed in prior commit - tests/worktree.test.cjs: fix indentation regression from prior edit - Add .changeset/1995-worktree-agent-branch-namespace.md (pr:0 placeholder) Found by orthogonal code review (Step 4). * fix(#1995): regenerate golden + size baselines for workflow-guard change * docs(#1995): backfill changeset PR number (2548) |
||
|
Tom Boucher
|
d579daa3ed |
docs(#2505): Phase 6 — migration guide + built-in-only subagent-toolkit enum (#2538)
* docs(#2512): Phase 6 — migration guide + built-in-only subagent-toolkit enum * fix #2512: update CONTRACT-PIN for built-in-only subagentToolkit value * docs(changeset): backfill PR #2538 for Phase 6 (#2512) |
||
|
Tom Boucher
|
f654c24a3e |
feat(#2505): Phase 4 — runtime-aware subagent dispatch (Option A; resolve-dispatch-type query) (#2525)
* feat(#2508): Phase 4 Option A — runtime-aware subagent dispatch via resolve-dispatch-type query (#2505) * fix(#2508): prose-variant preamble (avoid scanner-tripping literals) + namedDispatch===false-only mapping * fix(#2508): remove leftover old-preamble lines (keep prose variant only) * fix #2508: prose-only reference file * test #2508: regen golden install parity after workflow preamble additions * fix #2508: remove preamble from plan-phase.md (Phase 6 capstone ceiling); regen size+golden baselines * docs(changeset): backfill PR #2525 for Phase 4 (#2508) |
||
|
Tom Boucher
|
bf8f320083 |
feat(#2505): Phase 1 — EoS descriptor split (kimi-code capability.json + drift-guard registration) (#2519)
* feat(#2454): add kimi-code as an EoS capability (Node Kimi Code CLI) PR 1 of N for #2454. Establishes the EoS descriptor foundation for splitting GSD's kimi support into two distinct products per the user's directive: - kimi (existing): Moonshot's Python kimi-cli (~/.kimi, runtime: python) - kimi-code (new): Moonshot's Node Kimi Code CLI (~/.kimi-code, runtime: node, KIMI_CODE_HOME env) Per ADR-1239 EoS, runtime behavior is driven by capabilities/<id>/capability.json descriptors, not hardcoded branches in install.js. The new descriptor uses the existing primitives (dot-home configHome, skills artifactLayout, kimi-hooks-toml hooksSurface — same TOML [[hooks]] format Kimi Code reads per its docs). Critical Kimi Code constraint reflected in the descriptor: hostIntegration.dispatch.namedDispatch: false hostIntegration.dispatch.builtInSubagents: ['coder', 'explore', 'plan'] hostBehaviors.namedSubagentsSupported: false Kimi Code's official docs confirm only 3 built-in subagents with NO custom- subagent registration (the [subagent] table only has timeout_ms). The kimi-agents YAML layout (used by Python kimi-cli) is therefore NOT in kimi-code's artifactLayout. Schema adjustments: - subagentToolkit set to 'undocumented' (the existing escape hatch); the schema enum (full/read-only) lacks a 'limited'/'built-in-only' value. A follow-up PR can extend the schema enum to add 'built-in-only' as a first-class axis value reflecting Kimi Code's documented model. Registration: - capabilities/kimi-code/capability.json (new descriptor, modeled on codex) - bin/install.js: allRuntimes array + --all list + --kimi-code flag - gsd-core/bin/shared/runtime-aliases.manifest.json: kimi-code aliases (kimi-code, kimicode, kimi_code) - src/runtime-name-policy.cts: FALLBACK_ALIASES map - gsd-core/bin/lib/capability-registry.cjs: regenerated via scripts/gen-capability-registry.cjs --write Tests: - tests/multi-runtime-select.test.cjs updated for the new runtime count (18) + new --kimi-code flag test + 'All' shortcut renumbered 18 → 19. Out of scope for PR 1 (follow-up PRs in the sequence): - Install-time decision logic (kimi vs kimi-code detection / prompt) - agent-install-check semantics for kimi-code (verify Agent Skills presence) - cmdAgentSkills fallback returning subagent prompt content - Workflow template mapping (named agents → built-in coder/explore/plan) - Migration guidance for users currently on 'kimi' who are actually on Kimi Code - Schema enum extension for subagentToolkit: 'built-in-only' Refs #2454, #2095 (EoS/kimi migration epic), ADR-1239 (EoS). * fix(#2454): complete drift-guard registrations for kimi-code runtime The drift guards caught every surface that pins runtime enumeration. Each update is mechanical, driven by the guard's named failure mode: - src/runtime-name-policy.cts RUNTIME_LABELS: 'Kimi Code' label for kimi-code - src/runtime-name-policy.cts RUNTIME_FLAG_IDS: add kimi-code to the isKimiCode predicate generator - bin/install.js runtimeMap: option '11' → 'kimi-code', renumber downstream entries (11..17 → 12..18), ALL_RUNTIMES_OPTION 18 → 19 - gsd-core/bin/shared/model-catalog.json runtimeTierDefaults: kimi-code entry (null/null/null — same as kimi, no model tier defaults until configured) - docs/reference/capability-matrix.md: regenerated via scripts/gen-capability-matrix.cjs --write (kimi-code row added) - tests/global-config-home-fragment.test.cjs GOLDEN_FRAGMENT_MAP: kimi-code → '.kimi-code' - tests/fixtures/golden-install-parity/*.json: regenerated via npm run gen:golden (the runtime-aliases.manifest.json hash changed; all 17 runtime fixtures updated) The capability-registry is already regenerated from the prior commit. * test(#2454): update drift-guard tests for kimi-code runtime registration Multiple drift guards pin runtime enumeration counts and option numbering. Each update is mechanical, driven by the guard's named failure mode: - tests/runtime-flags.test.cjs: EXPECTED_FLAGS gains isKimiCode (16 → 17); 'all 16 flags' → 'all 17 flags' in test names + messages. - tests/multi-runtime-select.test.cjs: parseRuntimeInput option renumbering cascade — kilo moves 11→12, opencode 12→13, pi 13→14, qwen 14→15, trae 15→16, windsurf 16→17, zcode 17→18, All 18→19. New single-choice test for kimi-code (option 11). Prompt test updated for new numbering. - tests/host-integration-descriptors.test.cjs: EXPECTED_PROFILES gains kimi-code → 'programmatic-cli' (terminal CLI per Kimi Code docs); EXPECTED_FLATTEN gains kimi-code → false (backgroundDispatch:true per docs, same as Python kimi/opencode). - tests/global-config-home-fragment.test.cjs: table-count test renamed 13 → 14 table runtimes (kimi-code added to GOLDEN_FRAGMENT_MAP earlier). * fix(#2454): empty artifactLayout for kimi-code (PR 1 scope) The skills kind requires a converter (existing converters are per-runtime like convertClaudeCommandToKimiSkill). PR 1 of this multi-PR sequence only registers the descriptor; the actual Agent Skills converter (and a new 'convertClaudeCommandToKimiCodeSkill' function) lands in PR 2 alongside the install-time decision logic. Empty artifactLayout.global is valid and means 'nothing to install yet via the layout seam'. Also: added kimi-code to RUNTIME_META in tests/helpers/install-shared.cjs (localDir .kimi-code, globalSuffix .kimi-code), and added Kimi Code as option 11 in install.js's buildRuntimePromptText (renumbered downstream options 11..17 → 12..18, All 18 → 19). * fix(#2454): camelCase runtimeFlags for hyphenated ids (kimi-code → isKimiCode) The runtimeFlags generator previously produced 'isKimi-code' (hyphen preserved) for the new kimi-code runtime id. Property names with hyphens are awkward for consumers (flags['isKimi-code'] instead of flags.isKimiCode). The new runtimeIdToFlagName helper folds -[a-z] boundaries to uppercase, producing the conventional PascalCase flag name. The 16 prior single-word runtime ids are unaffected (the regex finds no hyphens). * fix(#2454): update remaining drift-guard tests + gen kimi-code fixtures - tests/runtime-flags.test.cjs drift guard: use proper kebab-case conversion (isKimiCode → kimi-code, not 'kimicode') so the registry comparison doesn't false-positive on hyphenated runtime ids. - tests/multi-runtime-select.test.cjs: fix kilo/opencode/pi/qwen/trae single-choice tests for the renumbered options (kilo 11→12, opencode 12→13, pi 13→14, qwen 14→15, trae 15→16). - tests/install.test.cjs: Kilo integration option 11→12, prompt test regex updated. - tests/fixtures/golden-install-parity/kimi-code.json + install-tree/ kimi-code.json: generated via UPDATE_GOLDEN=1 + UPDATE_INSTALL_TREE=1. The kimi-code install produces the standard GSD install layout (skills, contexts, references, etc.) — 436 paths, same shape as other runtimes that have no custom converter yet. * fix(#2454): add kimi-code install contract + global config home fragment - src/runtime-name-policy.cts GLOBAL_CONFIG_HOME_FRAGMENTS: add kimi-code → '.kimi-code' so getGlobalConfigHomeFragment returns the correct path instead of falling through to the default '.claude'. - tests/installer-migration-install.integration.test.cjs RUNTIME_INSTALL_CONTRACTS: kimi-code entry (same surface as kimi for PR 1; PR 2 will specialize once the Agent Skills converter lands). - tests/multi-runtime-select.test.cjs: fix space-separated-choices test for the renumbered kilo option (11 → 12). - tests/fixtures/golden-install-parity/kimi-code.json + install-tree/ kimi-code.json: regenerated after rebasing onto current next (new planner-reversibility.md from #2471 etc. now included). * test(#2454): skip kimi-code install contract until PR 2 ships install layout The end-to-end install test (tests/installer-migration-install.integration .test.cjs) asserts every allRuntimes entry installs a runtime-specific artifact surface. PR 1 of #2454 registers kimi-code in allRuntimes + the capability descriptor + flags + labels, but the install LAYOUT (Agent Skills converter + global AGENTS.md at $KIMI_CODE_HOME/AGENTS.md) lands in PR 2. The SKIP_INSTALL_CONTRACT set marks this exclusion explicit and self-removing — PR 2 removes the entry alongside adding the install surface, restoring the contract loop to full coverage. * fix(#2454): restore compact model-catalog.json format (M1 review) Per code-review M1: my prior 'fix(#2454): complete drift-guard registrations' commit used python json.dump(indent=2) which inflated the file from 165→607 lines (every nested entry got expanded) and lost the trailing newline. The semantic change was just a 3-line kimi-code entry. Restored the original hybrid format (top-level indent=2 + inner entries' one-line style) and added kimi-code in matching form. Regenerated golden install parity + install tree fixtures since the model-catalog.json hash changed. * fix(#2454): update CONTEXT.md allRuntimes glossary (17 → 18, add kimi-code) CI lint-tests job failed on the glossary drift guard (scripts/check-glossary-refs.cjs --check): ✗ CONTEXT.md's allRuntimes enum-count sentence claims 17 values but bin/install.js's allRuntimes array has 18. ✗ CONTEXT.md's allRuntimes member list has drifted from bin/install.js (missing from CONTEXT.md's list: kimi-code). Missed in the prior commits because gsd-test does not run the glossary check (it's a CI lint-tests-only check). Updating CONTEXT.md's two claims to 18 values + kimi-code in the member list. * chore(#2505): regen capability-registry + stamp kimi-code version 1.8.0 (#2511) * docs(changeset): Phase 1 kimi-code runtime Added (#2511) * test(#2511): regen kimi-code golden parity fixture after Phase 0 guard normalization lands * docs(changeset): backfill PR #2519 for Phase 1 (#2511) |
||
|
Tom Boucher
|
09b535ac00 |
feat(#2481): add a negotiated effortSurface axis and wire invocation-time effort
ADR-1239 gains a ninth negotiated axis, effortSurface (argv | none), declaring how
a host accepts reasoning effort. ADR-443 is amended in the same change because its
recorded deferral is what the axis resolves: its Unblock condition offered paths
(a) and (b) and stated the choice was 'a maintainer call this file records but does
not make'. Path (a) is selected and satisfied here.
Before this, effort reached a runtime only through install-time channels
(EFFORT_RENDERING's frontmatter/api), so reviewer CLIs spawned as subprocesses
silently inherited whatever effort sat in the user's own global CLI config. The
review lane now resolves one universal effort through the ADR-443 cascade and
renders it per host through the negotiated descriptor.
Every per-host value is documentation-sourced, never inferred:
- claude argv -- verified via 'claude --help' (--effort <level>)
- opencode argv -- verified via 'opencode run --help' (--variant)
- codex argv -- codex-rs/exec/src/cli.rs: model_reasoning_effort is NOT a CLI
flag (config.toml key only), so the global -c override is the
only argv route
- 15 hosts undocumented -- their docs state no reasoning setting; the sentinel
fails closed rather than inheriting a profile baseline
No config-file vocabulary member: the only host that ever had one (Gemini CLI's
thinkingConfig) was removed as a sunset runtime by
|
||
|
Tom Boucher
|
c5e0371775 |
feat(#1951): reversibility tagging — gate one-way-door decisions (#2471)
* test(#1951): add failing-first tests for reversibility tagging Red phase for issue #1951 (reversibility tagging: classify decisions by undo cost, gate one-way doors behind a checkpoint:decision). Tests assert, per the issue's acceptance criteria: - discuss-phase CONTEXT.md template records a **Reversibility:** field with a rationale on captured decisions, and states it is optional - gsd-planner @-references planner-reversibility.md and stays under the 49152-char agent cap (LARGE_CAP, tests/agent-size-budget.test.cjs) - a one-way rating inserts a checkpoint:decision before the dependent task; reversible inserts none; costly is flagged but never blocks - the taxonomy defaults to reversible when unsure (checkpoint-fatigue guard) and inserting a checkpoint implies autonomous: false - docs/reference/plan-md.md documents <reversibility> as optional with all three ratings - --no-reversibility-gates parses to REVERSIBILITY_GATES=false, is injected into the planner prompt, and is advertised in the command argument-hint and help full mode (argument-hint parity) - the override suppresses the gate but still persists the rating - cmdVerifyPlanStructure accepts every rating and the absent case (additive-validator guarantee, behavioral via runGsdTools) - parity: thinking-models-planning.md #4 adopts the canonical three-level taxonomy and the binary REVERSIBLE/IRREVERSIBLE vocabulary is gone - no content loss from the planner extraction made to fit under the cap Prose-contract assertions are Red until the implementation lands. The behavioral validator assertions pass immediately — regression guards proving the validator already accepts unknown optional tags. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * feat(#1951): reversibility tagging — gate one-way-door decisions Classify planning decisions by what undoing them would cost, and give a one-way door a human beat before the agent walks through it (issue #1951, The Pragmatic Programmer Topic 15 'Reversibility'; Bezos's one-way/two-way door framing). Acceptance criteria met: - discuss-phase records an optional reversibility rating with a rationale on <decisions> entries in the phase CONTEXT.md template. Unrated decisions are treated as reversible, so existing phases are unaffected. - a one-way rating makes gsd-planner insert a checkpoint:decision before the task that implements the decision, reusing the existing checkpoint mechanism -- no new checkpoint machinery. - reversible ratings trigger no checkpoint; costly ratings are flagged in the plan but never block. - the rating persists on the task as the optional <reversibility rating=> element. cmdVerifyPlanStructure accepts every rating and the absent case; the structural validator does not reject unknown optional tags. - --no-reversibility-gates (REVERSIBILITY_GATES=false) suppresses checkpoint insertion for intentionally-unattended runs while still recording ratings -- the override changes what stops the run, not what the plan remembers. Single taxonomy, not two: references/thinking-models-planning.md #4 already shipped a binary REVERSIBLE/IRREVERSIBLE classification and is loaded by both gsd-planner and gsd-plan-checker. It is rewritten onto the canonical three-level vocabulary and now points at planner-reversibility.md as the taxonomy owner, with a parity test that fails if the surfaces diverge (DEFECT.GENERATIVE-FIX-DIVERGENCE). agents/gsd-planner.md sat 47 chars under the 49152 LARGE_CAP, so the checkpoint DO/DON'T guidance was relocated verbatim into planner-antipatterns.md -- already @-referenced from the same section for the same topic, so the planner still loads it and nothing was dropped. A test guards the relocation against content loss. Files: gsd-core/references/planner-reversibility.md (NEW, canonical taxonomy + emission rules + anti-patterns), gsd-planner.md, plan-phase workflow/command/help (flag wiring + parity), plan-md.md schema, discuss-phase context template, CONTEXT.md glossary, INVENTORY + manifest, size baselines, install goldens, plugin skills regen, changeset. Closes #1951 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#1951): address orthogonal review findings Two isolated reviewers (correctness + security), neither of which authored the change. Every finding fixed: Security — the rationale is untrusted input (ADR-1577). It originates in conversation and flows CONTEXT.md -> planner -> PLAN.md -> executor, each hop an LLM reading the previous hop's output, with no validation on the path. planner-reversibility.md and the discuss-phase template now state it is data and never instructions, and name the </reversibility> early-termination hazard explicitly -- a rationale that closes its own element injects sibling structure the executor reads as real tasks. Four tests guard it. Correctness 1 — nothing machine-enforced the feature's own promise: a task rated one-way with no preceding checkpoint:decision validated as fully clean, so a planner error silently reopened the gap this feature exists to close. cmdVerifyPlanStructure now warns on an ungated one-way rating. A warning, not an error: <reversibility> stays additive and the plan stays valid. Four tests cover ungated (warns), gated (silent), still-valid, and reversible/costly never flagged. Correctness 2 — pass-always test. The --no-reversibility-gates parse test substring-matched the whole workflow file, and plan-phase.md prose mentions both tokens in one sentence, so it passed with the bash conditional deleted: it was testing the documentation, not the parser. Now scoped to the fenced bash blocks and matched as one physical line, with a negative control confirming prose alone cannot satisfy it. Correctness 3 — costly had no itemized emission rule, only one-way did, so two agents could diverge on whether to tag costly at all. Correctness 4 — template convention break: the example ratings were bare while every sibling field uses [...] to signal substitution, inviting an LLM to copy one-way/costly forward as boilerplate. Now bracketed. Correctness 5 — latent false-green: .includes('reversible') also matches inside irreversible/irreversibility, which appear in anti-pattern prose, so a surface that dropped the real taxonomy entry would still pass. Now word-boundary matched. ADR-857 phase-6 ceiling — the first gsd-test run caught plan-phase.md 1216 bytes over its frozen 94519 ceiling (it had 49 bytes of headroom on next). The ceiling may only rise for privileged host machinery, and reversibility gating is optional-feature logic, so the wiring was slimmed to its minimum and the explanatory prose moved to the reference files the planner already loads. plan-phase.md is now 94400 bytes -- 119 under the ceiling and 70 bytes SMALLER than on next, so the host loop shrank while gaining the feature, which is what phase 6 ratchets toward. The tracer contract (tests/tracer-bullet.test.cjs) is unchanged. Lint — fixed an unnecessary non-null assertion in verify.cts and a CRLF-fragile bare \n regex in the new test (DEFECT.WINDOWS-CRLF-TEST- PORTABILITY, the #1658/#1668/#2206/#2449/#2450 class). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1951): checkpoint fixture must carry the common task elements The gated-one-way fixture built a checkpoint:decision task from the abbreviated skeleton in gsd-planner.md, which shows only the checkpoint-specific elements (<decision>/<context>/<resume-signal>). cmdVerifyPlanStructure requires <name> and <action> on EVERY task regardless of type, so the fixture failed validation for reasons that had nothing to do with reversibility: errors: ["Task missing <name> element", "Task 'unnamed' missing <action>"] Caught by gsd-test on 14d14a39 (2 failures, both this fixture). The canonical shape is in tests/verify.test.cjs:266 — a checkpoint task carries <name>/<files>/<action>/<verify> like any other. Fixture corrected to match. Verified behaviorally against the real gsd-tools CLI across all four cases: gated one-way (valid, silent), ungated one-way (valid, warns), costly (valid, silent), absent (valid, silent). Not a product defect: the validator's every-task contract is intentional and pre-existing, and docs/reference/plan-md.md scopes its required-element list to type=auto/tracer only because those are the elements a planner must author, not because checkpoints are exempt from <name>. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1951): backfill changeset pr number to 2471 * fix(#1951): CodeQL incomplete-sanitization + prompt-injection scan collision Both CI failures were real defects in code this PR added, not false positives. CodeQL js/incomplete-sanitization (high), reversibility-tagging.test.cjs:46 — the namesRating helper built its regex with `rating.replace(/[-]/g, '\\-')`, which escapes the hyphen but not backslash, so the escape was incomplete. It was also unnecessary: `-` carries no special meaning outside a character class. Replaced with a complete metacharacter escape (backslash included). Word-boundary behavior verified unchanged across all three ratings — notably that "irreversible" prose still does not satisfy a "reversible" match, which is the false-green this helper exists to prevent. Prompt injection scan — the checkpoint fixture used the human-verification child element inside <verify>. That tag name is a fake-instruction-boundary pattern in scripts/prompt-injection-scan.sh, and the scan runs over changed files, so copying the shape from tests/verify.test.cjs (unflagged only because it is not in this diff) tripped the gate. Switched to the documented plain-prose <verify> form. The first attempt at that fix failed the same gate a second time: the comment explaining the collision quoted the offending tag literally. The comment now names it in prose instead — the scanner does not care whether a match is code or commentary, which is the whole point of the DEFECT.PROMPT-INJECTION-SCAN-COLLISION note in CLAUDE.md. Verified locally before push: scan reports 0 findings across 57 changed files, eslint clean, and both fixtures still validate as designed (gated one-way silent, ungated one-way warns, neither errors). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1951): record measured cost and halve gsd-tools spawns The Windows shard 1/3 job timeout was traced to the sharding layer, not to this PR's assertions — see #2472. Two contributing factors were this file's own, and are fixed here. 1. tests/test-timings.json had no entry for reversibility-tagging.test.cjs, so scripts/run-tests.cjs weighted it at the table's median fallback (~315ms) for LPT chunk packing. It actually measures 5595ms — an 18x under-weight. Recorded the measured value from the green gsd-test run (max across the node22/node24 lanes, per gen-test-timings.cjs's convention). Only this one entry: a full regen churns 634 entries of run-to-run drift, and the table is explicitly advisory and un-gated, so a 637-line diff does not belong in a feature PR. 2. Each verifyPlan() spawns gsd-tools, which dominates this file's cost. Spawns cut from 9 to 6 with no coverage lost: - the ungated-one-way warning and its stays-valid assertion now share one plan instead of building the same plan twice; - the reversible/costly never-flagged-as-ungated test was strictly subsumed by the additive suite, which already runs those two ratings ungated and asserts no /reversibilit/ warning at all — and the gate warning's text contains both "reversibility" and "one-way", so the broader assertion catches it. It only re-spawned gsd-tools twice to prove the same thing. Both are symptom fixes. The shard imbalance itself (19/11/10 minutes against a 20-minute cap, from a cost-blind round-robin partition that also reshuffles downstream files whenever one is inserted) is tracked in #2472. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#1951): checkpoint fixture adopts the #2444 type-branched contract Surfaced by rebasing onto next, which gained #2444 (branch plan-structure validation on task type=checkpoint:*) while this PR was in review. cmdVerifyPlanStructure no longer applies one required-element set to every task. A checkpoint:decision now requires <name> + <resume-signal> + <decision> + <options>, and is exempt from the <action>/<verify>/<done>/ <files> set that auto and tracer tasks carry. The gated-one-way fixture predated that split and failed on the new requirement: errors: ["Task 'Task 0: Confirm the on-disk format' missing <options>"] Fixture rewritten to mirror the checkpoint:decision contract exactly — real <options> with two <option> children — rather than padding it with fields checkpoints no longer need. That also drops the plain-prose <verify> the earlier revision carried purely to dodge the prompt-injection scan; a checkpoint task has no <verify> requirement at all, so the workaround is moot. Verified against the real gsd-tools CLI across all four cases: gated one-way (valid, silent), ungated one-way (valid, warns), costly (valid, silent), absent (valid, silent). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
909a3b180b |
fix(#2470): install pi's extension as gsd.js so pi actually discovers it (#2478)
* test(#2470): failing-first — pi extension must satisfy pi's auto-discovery filter pi auto-discovers extensions/ entries through isExtensionFile(), which accepts only .ts and .js. GSD installs its extension as gsd.cjs, so pi silently skips it: no /gsd command, no error, no log line. Encodes pi's discovery PREDICATE rather than a literal filename, so the contract keeps holding across future renames, and adds the migration-006 test matrix for retiring the stale gsd.cjs left in pre-fix installs. Red until the fix lands. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): install pi's extension as gsd.js so pi actually discovers it pi auto-discovers extensions/ entries via isExtensionFile(), which accepts only .ts and .js and skips everything else silently. capabilities/pi declared the dest as gsd.cjs, so the extension installed correctly and was then ignored forever: no /gsd command, no error, no log line. Install it as gsd.js. The in-repo source stays pi/gsd.cjs — tests require() it directly and .cjs is unambiguous CommonJS; only the installed name has to satisfy pi, and pi loads accepted files through jiti, which handles CJS and ESM alike. (The reporter's premise that ~/.pi/agent/package.json declares "type":"commonjs" does not hold — pi never writes that file.) Renaming an installed artifact requires a migration record, so add 006 to retire the stale gsd.cjs from pre-fix installs; without it the old path drops out of the manifest and uninstall can never remove it. The migration plans nothing for an unmanifested gsd.cjs: emitting remove-managed there would have the executor downgrade it to preserve-user and mark it blocked, failing the install for anyone who hand-placed their own file. Also pins body-parser >=2.3.0 (GHSA-v422-hmwv-36x6). The advisory reaches the production tree transitively via the Claude Agent SDK and fails the npm-integrity gate, blocking any PR; pinned via the existing overrides idiom. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): address orthogonal review findings + register migration checksum Code review: - pi/gsd.cjs's install docstring still told readers to copy the file to extensions/gsd.cjs — the exact silently-broken state this PR fixes. Anyone following it recreated the bug. - Two stale extensions/gsd.cjs comments in install-minimal-hooks.test.cjs. Security review: - _installNativePluginIfDeclared confined nativePlugin.dir but joined nativePlugin.file onto the validated directory unchecked, so a descriptor whose file carried .., an absolute path, or a NUL byte would have written outside configHome. Not reachable in a shipped build (descriptors are first-party and compiled into the capability registry), but file is exactly the field this PR changes. Confine the full dest path instead; for a well-formed descriptor this resolves identically to the previous mkdir(dir) + join(dir, file). Covered by four new write-confinement tests. Also register migration 006 in the #670 EXPECTED_CHECKSUMS baseline — shipped migration bodies are locked to a committed checksum and a new migration fails CI until it is listed. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): never dereference a symlinked managed path when snapshotting fs.copyFileSync follows symlinks, so a managed path replaced by a link had the REFERENT's bytes copied into the migration journal's rollback and backup trees — a gsd.cjs symlinked at a private key would land that key's contents under gsd-migration-journal/. Deletion was already safe (fs.rmSync unlinks the link, never the target); the copy was not. Nothing GSD installs is ever a symlink, so the faithful snapshot of a symlinked managed path is the link itself. copyPreservingSymlink recreates it, which keeps rollback fidelity (restore re-creates the same link) while never reading the referent. Scoped the pre-delete to the symlink branch only, so the regular-file path keeps copyFileSync's overwrite-in-place and a mid-restore failure cannot destroy the destination. The restore-side existence check moves to lstat, since existsSync follows a link whose target is gone and would silently skip the restore. This lives in the engine all six migrations share, so 000-005 are hardened too. Also regenerates the pi golden-parity hash: correcting pi/gsd.cjs's own install docstring changes the extension's content, which the golden suite caught. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2470): symlink-preserve the in-apply failure-recovery restore too The previous commit routed three copy sites through copyPreservingSymlink but missed a fourth: the catch block inside applyInstallerMigrationPlan, which replays rollback snapshots taken earlier in the SAME apply attempt. Those snapshots are symlinks precisely because of that commit, so the raw copyFileSync there dereferenced them and wrote the referent's bytes to the LIVE install path — worse than the journal-tree leak it was meant to fix, since it is user-visible and at a predictable location. Verified by experiment rather than assertion: with the pre-fix line restored, the managed path comes back as a REGULAR FILE containing the referent's bytes; with the fix it comes back as a symlink and the bytes appear nowhere. The accompanying test injects the failure by letting the delete succeed and then throwing once, modelling a later step failing after the delete. That ordering is load-bearing — an earlier draft injected before the delete, which leaves the live path in place, so the pre-fix copyFileSync hit a same-file collision and threw instead of leaking. That draft passed against the bug it was written to catch; this one fails against it. Adds the missing rollback() coverage as well: a restored symlinked managed path must come back as a link pointing at its original target. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#2470): read the backup location from the journal, not the plan The new backup-content assertion read backupRelPath off result.plan.actions, where it is always null: the planner reserves the field and apply chooses the concrete location, recording it in the journal. The assertion therefore failed on "backup path must be recorded for the user" rather than on anything about the behavior it was written to check. Read it from the journal, which is the authoritative record. Verified by executing all four new test bodies in-process against the built engine — the backup file exists and holds the locally patched content. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#2470): backfill changeset pr number to 2478 * chore(#2470): backfill changeset pr number to 2478 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
953b8043ea |
fix(#2456): weight test chunks by measured cost and pack with LPT (#2463)
* fix(#2456): weight test chunks by measured cost and pack with LPT scripts/run-tests.cjs guessed each test file's cost from its filename (basename matching /^(?:install|codex-)/ scored 12, everything else 1). Measured durations show that guess is wrong in both directions: installer-migration-authoring.test.cjs scored 12 while running ~0.1s, and the two most expensive files in the suite both scored 1 — run-tests-harness.test.cjs never matched the prefix, and release-tarball-smoke.install.test.cjs was missed because the regex is anchored to the START of the basename. Chunks were therefore balanced by file COUNT, not cost. On the real shard 2/3 the two heaviest files packed into the SAME chunk, leaving the slowest chunk 2.8x the lightest and sitting near the 600s per-chunk timeout while other chunks idled. Weight each file by its measured duration from a checked-in, regenerable timings table and pack with LPT (heaviest first, into the lightest chunk). On the same shard this drops the slowest chunk from 383s to 238s and the imbalance from 2.79x to 1.00x, and separates the two heavy files. Timings are advisory, never gated: an unknown file falls back to the table's median weight, a missing or corrupt table falls back to uniform weight, and a count-based floor guarantees the packer never produces fewer chunks than plain count-based packing would. Closes #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): harden chunk packing against degenerate knobs and table keys Follow-up hardening found while reviewing the packer, fixed inline. The chunk knobs are read from the environment with Number(), so a typo (RUN_TESTS_MAX_FILES_PER_CHUNK=abc) yields NaN and an explicit 0 yields 0. Both flow into the new chunk-count arithmetic: NaN made Math.ceil return NaN, Array.from({length: NaN}) produce zero bins, and packChunks' retry loop spin forever — a hung CI job with no output. Zero made the count Infinity and threw RangeError: Invalid array length. The previous count-based packer degraded to a single chunk instead, so this was a regression introduced by the LPT rewrite. Normalize the knobs at the environment boundary (positiveNumberEnv: anything not a positive finite number falls back to the default) and guard packChunks itself, since it is exported and cannot assume its caller normalized. Non-finite weights from an arbitrary weightOf are clamped too. RUN_TESTS_CHUNK_TIMEOUT_MS gets the same treatment. Also resolve timing-table lookups with Object.hasOwn: the table is JSON-parsed, so a bare index would walk the prototype chain and return a function for a file named constructor.test.cjs or toString.test.cjs. The typeof guard already rejected that, but the lookup now resolves correctly rather than relying on the downstream check. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): correct prototype-lookup rationale and guard generator keys Two findings from independent security review, fixed inline. The makeFileWeigher comment claimed a bare table lookup "would return a FUNCTION for a file named constructor.test.cjs". That premise is false: basename('constructor.test.cjs') is 'constructor.test.cjs', which is not an Object.prototype key, and walkTestFiles only ever collects *.test.cjs. The prototype chain was never reachable from a real selection, and the existing typeof guard already rejected the function it would return, so Object.hasOwn is defense-in-depth rather than a behavior change. The comment now says that instead of asserting something untrue. The accompanying test inherited the same false premise: it fed constructor.test.cjs and asserted a median fallback that would have held with or without the guard, so it passed for a reason unrelated to what it claimed to prove. It now uses BARE keys (constructor, toString, valueOf, hasOwnProperty, __proto__) — the only inputs that actually resolve on Object.prototype — and asserts the real exported contract: any key absent from the table weighs the median, never a function. gen-test-timings.cjs built its output object by computed-key assignment from basenames taken out of a reporter stream it does not control — the js/prototype-polluting-assignment shape, and this repo has a CodeQL barrier for exactly that pattern. It was not exploitable (the value is always a rounded number, so the __proto__ setter is a silent no-op), but it silently DROPPED such an entry rather than reporting it. Validate every key against a test-basename pattern and fail loudly instead, and build the table with a null prototype. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#2456): replace tautological chunking tests and clamp chunk count Six findings from independent correctness review, all reproduced and fixed inline. The two subprocess tests written to carry the #2088 guarantee forward were tautological: every seeded file weighed exactly 1, so both passed under the OLD prefix-heuristic packer and with the timings file deleted entirely. Neither could fail for the reason it existed. Both are rebuilt so the old algorithm produces a different packing and the assertion goes red: the spread test now uses three expensive files named so the old heuristic scored them 1 alongside three trivial `install-`-prefixed files it scored 12 — inverted from real cost, giving {2,2,1,1} under the old packer versus {2,2,2} under measured weights. The companion test covers the other direction: four trivial `install-` files the old heuristic split into four single-file chunks now stay in one. packChunks clamped the chunk count from below but not above, so a legitimate but tiny budget (RUN_TESTS_MAX_FILES_PER_CHUNK=1e-9, which positiveNumberEnv accepts) asked for 637,000,000,000 bins and threw RangeError. More chunks than files is never useful; the count now clamps at one file per chunk. The generator's basename-collision guard compared full dirnames, so two OS lanes reporting the same file under different container roots (/work/tests vs C:/work/tests) flagged every shared basename as a collision — on the script's own documented multi-lane usage. Detection is now scoped per stream, where the root is constant; a genuine same-lane collision is still caught. Also: the LPT tie-break compared raw paths, so a path separator (0x2F vs 0x5C) could order a subdir file differently per platform, contradicting the documented byte-identical guarantee — it now normalizes separators. loadTestTimings now honors schema_version instead of writing it and never reading it, falling back to uniform weight on an unknown version. A comment claiming an all-uniform suite "chunks exactly as it did before" was false and contradicted by this PR's own test: the chunk count is preserved, the composition is not. And the missing-table test created a temp dir it never cleaned up, for a path that only needed to not exist. Refs #2456 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
455ad49ae3 |
feat(#2296): config-gated provider escalation on quota-exceeded (#2458)
* test(#2296): failing-first coverage for provider escalation on quota-exceeded
Covers the provider-escalation ladder layered onto EXEC.CLASSIFY: back-compat
(no escalation block without --failure-class), cap boundaries at
min(max_escalations, list length) at limit-1/limit/limit+1, opt-in gating,
malformed/hostile provider_escalation config, the --failure-class CLI negative
matrix, config-key registration, and a fast-check budget-limit property.
Red until the resolver, CLI flag, and manifest key land.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* feat(#2296): config-gated provider escalation on quota-exceeded
The dynamic_routing tier ladder escalates within one provider, which does not
help when that provider is what ran out of quota. Add an opt-in provider ladder
layered on the existing EXEC.CLASSIFY seam.
- model-resolver: resolveProviderEscalation walks dynamic_routing.provider_escalation
capped at min(max_escalations, list length), reporting from/to/attempted/exhausted.
Invalid entries are dropped (ADR 227 shape validation). Stays a leaf module —
the quota-class policy decision is the caller's, per the CONTEXT.md contract.
- agent-command-router: export a frozen AGENT_FAILURE_CLASSES so the new CLI
validator cannot drift from the classifier that produces the values.
- resolve-execution: --failure-class flag; emits an escalation block ONLY when
passed, so the existing JSON contract is byte-identical for every caller.
- config-schema.manifest: register dynamic_routing.provider_escalation.
- execute-phase step 7.1: auto-escalate, honor Retry-After, fail loudly naming
every model tried once the ladder is spent.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* fix(#2296): extract quota recovery to a reference fragment; regen goldens
The step 7.1a addition pushed gsd-core/workflows/execute-phase.md from 93390 to
95111 LF bytes, past the frozen ADR-857 Phase 6 ceiling (hard <93600, margin
<=93400) asserted by tests/fix-2285-claude-orchestration-wiring.test.cjs. The
base sat 10 bytes under the margin, so no inline wording would have fit.
That gate's own rationale is that optional-feature detail belongs in a fragment,
not the host loop. Moved BOTH the new provider-escalation branch and the
pre-existing manual recovery prompt into
gsd-core/references/execute-phase-quota-recovery.md, leaving step 7.1 as a
one-line pointer. execute-phase.md is now 92880 bytes — 510 SMALLER than base.
Also regenerates the fixtures that legitimately moved because three shipped
files changed (gsd-tools.cjs, config-schema.manifest.json, execute-phase.md):
golden-install-parity + install-tree for all 16 runtimes, INVENTORY.md +
INVENTORY-MANIFEST.json for the new reference, and the workflow size baseline.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* test(#2351): make the C1 orphan-reaping test load-independent
tests/run-with-timeout.test.cjs C1 asserted the child heartbeat file exists
after a 1s group-kill window, but the child only wrote it on the first 100ms
setInterval tick. Nothing synchronized the two: on a loaded container the group
is SIGKILLed before that tick lands, the file never appears, and the assertion
fails for a reason unrelated to reaping. Observed failing on both linux-node22
and linux-node24.
The behavior actually under test is the FREEZE assertion (heartbeat stops
advancing => descendant was reaped, not orphaned). That is unaffected by
sampling once more at t=0.
Child now writes its first heartbeat synchronously at startup before arming the
interval, and the kill window widens 1s -> 3s to cover child boot under load.
Both remove the timing dependency; neither weakens what the test proves.
Refs #2296
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* chore(#2296): backfill pr:2458 in .changeset/rapid-jays-bark.md
* chore(#2296): regenerate fixtures after rebase onto #2402
The rebase conflicted on the generated golden-install-parity fixtures and
workflow-size-baseline.json because #2402 (
|
||
|
Tom Boucher
|
b6e6a22fce |
fix(#2402): honor response_language across orchestrator output + UAT checkpoint renderer (#2457)
* fix(#2402): honor response_language across orchestrator output + UAT checkpoint renderer Replays the in-flight bot branch fix/2402-response-language-orchestrator-coverage (seven commits, never pushed) onto current origin/next as a single squashed commit. The original work was substantial and correct; this commit preserves its full scope, trimmed where rebase conflicts + workflow size budgets required it. Three independent layers where response_language was being dropped are closed: Layer 1 — orchestrator-facing directives across workflows. Adds the strong "All user-facing output in this workflow MUST be presented in {response_language}; technical terms, code, paths, and subagent prompts stay in English" directive to ~40 workflows that previously either lacked it entirely (verify-work, new-project, new-milestone, quick, manager, and ~35 more) or carried only the weak subagent-prompt-only form (plan-phase, execute-phase). The directive covers narration between tool calls and banner output, not just the AskUserQuestion prompts. Layer 2 — UAT checkpoint renderer (src/uat.cts). buildCheckpoint now accepts an optional responseLanguage parameter and renders the frame strings ("CHECKPOINT: Verification Required", "Type `pass` or describe what's wrong.") in any of 9 languages (English/Spanish/French/German/Portuguese/Japanese/ Chinese/Korean/Italian) with an alias table covering ~30 input variants (en, es, español, ja, 日本語, etc.). cmdRenderCheckpoint reads config.response_language via loadConfig(cwd) and passes it through, so the byte-for-byte block verify-work.md reprints verbatim is already localized when written — preserving the anti-injection hygiene rule at verify-work.md (the model is forbidden to translate after the fact). CJK display width is computed by East Asian Width property ranges (W/F) so the right ║ border of the banner stays aligned for full-width characters. English fallback is byte-identical to the pre-fix behavior when response_language is unset or unrecognized. Layer 3 — literal English report templates in execute-phase. The top-of- workflow directive covers all template sites (templates are a structural source, not literal output). Inline render-language notes that previously sat at each template site were removed during the squash because they pushed execute-phase.md over its frozen pre-phase-6 byte ceiling (93600 — ADR-857 Phase 6 capstone). The single top directive covers the same surface with fewer bytes. Also extends src/docs.cts and src/init.cts to propagate response_language into the init JSON bundle of the additional workflows so the directive can read it. Tests added: - tests/uat.test.cjs: buildCheckpoint with unset/unrecognized language falls back to English default; recognized language swaps only the two frame strings while structural lines stay untouched; CJK display-width regression (independent recomputation of East Asian Width W/F ranges). - tests/workspace.test.cjs, tests/docs-update.test.cjs: response_language wiring through docs.cts/init.cts. References: #2402; reporter's three-layer triage + Layer-4 follow-up; the byte-for-byte anti-injection hygiene rule at verify-work.md (the reason Layer 2 must be renderer-side, not model-translated). This is a squash of the in-flight bot branch — seven commits representing the original implementation plus its subsequent fix/CJK-padding/test/ changeset/regen cycles, none of which were ever pushed or PR'd. The squash captures the final coherent state. * chore(#2402): backfill pr:2457 in .changeset/2402-response-language-orchestrator-coverage.md * chore(#2402): regen golden + size baseline after rebase against #2315 (PR #2451) Rebase conflicts were entirely in generated artifacts (golden-install-parity fixtures + workflow-size-baseline.json). After taking theirs during rebase, regenerated cleanly against the merged source tree. |