eb336e9f777352d0e5682eec244d7ccdbda7a7d5
18 Commits
| Author | SHA1 | Message | Date | |
|---|---|---|---|---|
Cody Anderson
|
8c9265d4e5 |
fix(#3724): warning-only Dimension 3b findings no longer force the revision loop (#3758)
* fix(#3724): stop advisory Dimension 3b findings from forcing the revision loop Dimension 3b (undeclared/temporal coupling, #1954) is spec'd "never a blocker" but tagged severity: warning — the tier plan-phase's revision loop counts as must-fix — and the planner is never taught the rule, so every multi-wave phase touching shared mutable state replans at least once, and intentionally coupled plans re-flag identically every iteration to the stall prompt. Three coordinated changes: - gsd-plan-checker: retag 3b to severity: info, the tier references/revision-loop.md already exempts by design; recognize a coupling_justified frontmatter declaration in the Do-NOT-flag list so deliberate pairs converge. Additions are offset by trimming 3b motivation prose — the checker sits 45 bytes under its LARGE hard cap. - plan-phase step 12: INFO-only accept — an issues block with zero BLOCKER/WARNING entries accepts the plan and surfaces the advisories instead of re-entering the revision loop. Real blockers and warnings still gate unconditionally. - gsd-planner: slim pointer in assign_waves to the new progressive-disclosure reference gsd-core/references/planner-coupling.md (the planner sits 19 chars under its own cap), which carries the shared-mutable-state rule and the coupling_justified escape hatch so first-pass plans avoid the finding when the coupling is unintentional. Documented the coupling_justified field in docs/reference/plan-md.md. Growth acks per #2914; inventory manifest and install-tree fixtures regenerated for the new reference file. Closes #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): pin Dimension 3b at severity: info The severity retag makes the old assertion (severity: warning) stale; lock the advisory tier from both directions — info must be present, warning must not — so a future edit cannot silently re-arm the revision-loop trigger. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * chore(#3724): changeset fragment for PR #3758 Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * docs(#3724): roster planner-coupling.md in docs/INVENTORY.md The new reference was enumerated in the manifest and all 19 install-tree fixtures but missing its row in the Modular Planner Decomposition table — the roster half the manifest-sync test cannot check. (Review Blocker.) Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): cover all four acceptance criteria (review round 1) - plan-checker-coupling: the 3b severity assertion is now a PARITY check deriving the exempt tier from revision-loop.md's flow instead of hardcoding info — editing either side alone reds the suite. New describe pins the other three criteria: plan-phase's INFO-only accept clause (proven failing-first), the BLOCKER + WARNING count staying intact, the coupling_justified Do-NOT-flag exemption + fix_hint, and the planner pointer + planner-coupling.md content. - ack fragment: $comment's plan-phase figure corrected to +79B; the 2775 pin note carried forward into the gsd-planner.md entry, updated for upstream's #3761/#3764 Rule-paragraph anchor (which this diff leaves verbatim). The parallel-dependent-plans re-anchor this commit originally carried was superseded by upstream #3764 during review; this branch no longer touches that file. Refs #3724 Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 2 — align the stance enumeration, complete the template contract MAJOR: <adversarial_stance>'s severity enumeration gains the INFO bullet so it agrees with Dimension 3b's 'ALWAYS INFO' mandate instead of contradicting it. Funded by extracting the inline <examples> block to the new progressive- disclosure reference gsd-core/references/plan-checker-examples.md (@-inlined from the same spot; #1949 precedent), which also restores the 3b motivation clause round 1 traded away (Nit 4) and nets the agent file SMALLER than base (49107 -> 48486) — the extraction the byte pressure was owed. MINOR: gsd-core/templates/phase-prompt.md now carries coupling_justified, and the field's shape becomes one 'plan-id: reason' string per coupled peer so a plan justified against two peers can express it; docs/reference/plan-md.md's Type column names the shape. NIT: the 3409 ack's plan-phase entry no longer calls the #1168 workflow ratchet an 'XL tier'. Acks and derived artifacts updated accordingly (checker entry removed — a shrink needs no ack; INVENTORY roster row + regen:derived for the new file). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * test(#3724): derive the 3b negative severity assertion (review round 2) Every severity token in the 3b span must BE the tier revision-loop.md exempts, replacing the hardcoded severity:warning negative — if the loop's exemption ever moves, the failure names the real conflict instead of blaming the agent file with a mutually-unsatisfiable pair. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): refit the planner coupling pointer under the char cap Upstream #3299 (PR #3390) grew agents/gsd-planner.md to 49146 chars at the base, leaving 5 chars of headroom where the +16-char pointer was measured against 13 more. The pointer prose shortens to 'Non-file coupling:' — 49150 chars, back under the strict 49152-char cap — and the ack figures follow. The @-path the tests pin is unchanged. Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): re-home the plan-phase ack after the #3823 spent-fragment sweep Upstream #3078/#3823 deleted all fully-spent ack fragments, including 3409-unreachable-guard-arms.json, which carried this PR's plan-phase.md +79B append. Per the collision remedy that sweep added: take the deletion and home the still-live entry in this PR's own fragment. Figures re-measured at this merge base (90871 -> 90950 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): absorb the spent #3172 plan-phase fragment into this PR's ack Upstream #3825 shipped 3172-stated-failing-direction.json naming only plan-phase.md, now spent at the base — colliding with this PR's live plan-phase entry. Per the #3003 pattern the fully-spent single-path fragment is deleted and this fragment stays the path's one source; figures re-measured at this base (93073 -> 93152 LF bytes). Claude-Session: https://claude.ai/code/session_01GshUzpGjoxiw6uNRiFMHvM * fix(#3724): review round 3 — true up the ack figures, restore the wave comment The fragment's absolute sizes are re-measured and anchored to base |
||
|
Tom Boucher
|
9410f7e6e6 |
enhance(#3897): ADR-3473 §8.3 rungs 2-4 — runtime marker, derived Codex sandbox, short-form depends_on (#3941)
* test(#3897): failing-first coverage for §8.3 rungs 2-4 ADR-3473 §8.3 has four rungs; #3883/PR #3896 shipped the first. This pins the other three RED before any fix. Rung 2 — the install marker has four readers and resolveRuntime is not one. resolveRuntime resolves GSD_RUNTIME > config.runtime > 'claude' and reads no marker at all, while bin/install.js writes one (#2297) and FOUR hand-rolled readInstallRuntimeMarker copies exist: src/model-resolver.cts:65 (cached, with test seams), hooks/gsd-agent-isolation-guard.js:112, and TWICE in hooks/gsd-cursor-subagent-start.js at :346 and :355. Four copies of one rule. Fixtures and seam names mined from PR #3382 rather than re-derived; it implemented this rung and was closed "not on the merits". Rung 3 — the sandbox map, and the fallback that was the real defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: - all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements — the map carries nothing the contract does not - 24 roles fall through `|| 'read-only'`, of which 16 declare Write or Edit So the map is redundant and the silent fallback is the defect. The maintainer chose to derive but hold those 16 at read-only pending the question of whether Codex enforces sandbox_mode or merely advises; HALT.md records it. T20 asserts the emitted sandbox_mode PER ROLE against a captured baseline, not in aggregate — an aggregate passes while one role silently widens, which is the proxy-instead-of-identity shape this repo names. T24 and T25 fail on a stale hold, so the hold list cannot rot into the subset map being deleted. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: `git log -S shortFormToId` returns only documentation commits. That was the wrong instrument. Direct inspection of sdk/src/query/phase.ts at 11918dcc3^ shows five occurrences, and the tests match that code rather than a guess at its semantics — including first-write-wins on a duplicate short form. T43 asserts at the consumer's output: the emitted `waves` map from the real CLI, which pre-fix collapses to {"1":[...]} because every short-form edge is dropped. A unit assertion on resolveDependencyId would have passed throughout this defect's life. Observed RED, this tree: rung 2 11/11 fail — no marker rung, no seams rung 3 T23,T24,T25,T26,T30 fail; T28 fails (validate agents passes a TOML whose sandbox_mode disagrees — it checks presence only) rung 4 T42,T44 fail; T43,T49 fail with waves collapsed to a single wave 1 Green and staying green: T20/T21/T22/T27 as captured baselines, #3885's unresolvable-token warning and wave-verdict suppression, and #3785's display-mapping passthrough. If the third tier over-reaches, those go red — that is their job. Disclosed weakness: T45 (a canonical id with no dash is not short-form indexed) cannot be isolated behaviorally, because planMap always masks it. It is a non-crash boundary pin, weaker than the other rows, and is recorded as such rather than presented as equivalent. Design: .gsd/phase/feat-3897-adr3473-83-rungs/40-design.md Test matrix: .gsd/phase/feat-3897-adr3473-83-rungs/50-test-matrix.md Decision: .gsd/phase/feat-3897-adr3473-83-rungs/HALT.md Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * enhance(#3897): §8.3 rungs 2-4 — one marker reader, a derived sandbox, the third depends_on tier ADR-3473 §8.3 has four rungs. #3883/PR #3896 shipped the first. These are the other three. Rung 2 — the install marker had four readers, and resolveRuntime was not one. resolveRuntime resolved GSD_RUNTIME > config.runtime > 'claude' and read no marker, while bin/install.js writes one (#2297) and four hand-rolled readInstallRuntimeMarker copies existed: src/model-resolver.cts (cached, with seams), hooks/gsd-agent-isolation-guard.js, and twice in hooks/gsd-cursor-subagent-start.js. model-resolver's was already the house idiom, so it was promoted rather than replaced: src/runtime-slash.cts now owns it, and model-resolver plus both hooks delegate. The hooks reach it through ensureRuntimeBuild(), the seam lint-hooks-runtime-build-seam enforces. No import cycle existed - checked both directions before moving anything. The marker is the THIRD rung: env > project config > marker > 'claude'. N1 was checked rather than assumed, and my first reading of it was wrong. A marker holding an unknown name comes back essentially verbatim, which looked like a validation gap. Measured against the env rung with the same inputs - including "../../etc/passwd" and "claude;rm -rf /" - the two are identical, because they share resolveRuntimeNameFromCandidates. N1 asks for exactly that, and it is met. The residual (the shared normalizer normalizes shape, it does not validate against the known-runtime set) is pre-existing on the env rung and plausibly deliberate, since a new runtime should not need a code change. The marker also does not widen the trust boundary in any real sense: it lives inside the install tree beside the code, so anyone who can write it can write runtime-slash.cjs itself. Rung 3 — the map was redundant; the silent fallback was the defect. Measured across all 35 files in agents/, deriving workspace-write iff tools: declares Write or Edit: all 11 CODEX_AGENT_SANDBOX entries derive to their mapped value exactly, zero disagreements. The map carried nothing the contract did not already have, so it is DELETED rather than reconciled. What was actually broken is `|| 'read-only'`, which silently under-granted 24 of 35 roles. 16 of those 24 declare Write or Edit and would widen under derivation. Per the maintainer's decision (HALT.md), they are held at read-only pending the question of whether Codex enforces sandbox_mode or merely advises. Emitted TOML is therefore byte-identical for all 35 roles - asserted per role, not in aggregate, because an aggregate passes while one role silently widens. The hold list self-invalidates. A hold whose role no longer derives broader fails, and so does a hold naming a role with no agents/<name>.md. Without that it would rot into exactly the hand-maintained subset map being deleted, and this commit's own ledger claim would become false over time. Both cases were proved by injecting them and watching them throw. Two committed tests asserted the deleted map's existence and contents. They were pinning the thing being removed, so the tests moved rather than the production code: the 11 role-value pairs survive as a test-local PRE_3897_CODEX_AGENT_SANDBOX baseline, and the assertions now drive the real derivation against real agents/*.md. The coverage is preserved; only its source moved out of production code. validate agents gains checkCodexSandboxPosture, mirroring the existing checkCodexModelPosture: each installed TOML's sandbox_mode must equal the role's expected value, failing with role, expected and found. It previously checked file presence and manifest completeness only, so a TOML whose sandbox_mode disagreed passed. Rung 4 — shortFormToId, recovered rather than invented. I nearly reported this as another wrong §8.3 claim: git log -S returns only documentation commits. Wrong instrument. sdk/src/query/phase.ts at 11918dcc3^ carries five occurrences, and the implementation here matches that code rather than a guess at its semantics - including first-write-wins on a duplicate short form, deterministic from the sorted plan order. It resolves the bare plan number: depends_on: ["01"] now reaches 26-01-auth-hardening. That is a control-flow change, not a diagnostic one - plans that silently collapsed into a single wave 1 now execute in their declared waves, and execute-phase.md consumes those wave values. In-phase only, by construction: the map is built from this phase's rawPlans, so a same-named short form in another phase does not resolve. #3785's display-mapping passthrough and #3885's unresolvable-token warning and wave-verdict suppression are untouched and stay green. If the third tier had over-reached, those are what would have caught it. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open I introduced, and wire the posture check to its command Two blockers from review. Both are mine, and one is a security regression my own change created. 1. A held role could escape its hold by editing its own frontmatter. The Codex install loop set the sandbox identity from the agent's frontmatter `name:` field rather than from its filename, so the hold lookup keyed off a value the file itself declares: deriveCodexSandboxMode('gsd-doc-writer', <real file>) -> read-only deriveCodexSandboxMode('gsd-doc-writer-x', <same file, name: edited>) -> workspace-write deriveCodexSandboxMode('GSD-Doc-Writer', <same file, name: recased>) -> workspace-write What makes this a blocker rather than a nit is the DIRECTION. The deleted CODEX_AGENT_SANDBOX map had the identical lookup-key quirk, but it was an allowlist: an unmatched key fell back to read-only, which is safe. The new scheme derives workspace-write from the tool contract and uses the hold as a subtraction, so the same mismatch fails OPEN. I converted a fail-closed quirk into a fail-open one and did not notice; the isolated reviewer proved it by execution. Neither safety net caught it. validateCodexSandboxHolds only checks that <key>.md exists, never that a file's derived identity matches its key. checkCodexSandboxPosture looks the canonical source up by the installed TOML's filename, finds nothing for a renamed agent, and treats it as a custom non-roster agent — silently no violation. The identity is now the FILENAME STEM, which is what validateCodexSandboxHolds already validates and what an attacker editing frontmatter cannot change without renaming the file — at which point the existing validator catches it. The lookup is case-insensitive so a recase does not slip past either. The frontmatter name still drives the TOML body and filename, unchanged; only the sandbox identity moved. All 35 roster files were checked: name matches filename stem everywhere, so a stricter "they must agree or throw" invariant would have been safe against real content. It is deliberately NOT added — it would abort an install on a tampered file where emitting a correctly-derived read-only TOML is the safer outcome. Recorded as a fork rather than decided silently. 2. checkCodexSandboxPosture was exported and never called. cmdValidateAgents (src/verify.cts) called checkAgentsInstalled and checkCodexModelPosture only; grep for the sandbox check in that file returned nothing. So criterion 3 — "validate agents fails on semantic drift, not only on missing files" — was unmet, and `validate agents` behaved exactly as before. That is ADR-3473 Decision 2's named shape: a declared policy with no executor. It also meant the T28 test asserted at the helper's return value while the COMMAND stayed broken — the ADR-3180 Decision 4(b) failure this epic exists to close, committed by me while enforcing it elsewhere in the same epic. Now wired as an additive `sandbox_posture` field beside `codex_posture`, following the sibling precedent exactly. Drift is report-only, not a non-zero exit, because that is what checkCodexModelPosture does — two sibling posture checks disagreeing about whether a violation is fatal would be its own defect. The choice is recorded in a comment rather than left implicit. A consumer-output test now drives the real CLI and asserts on the emitted JSON, and was shown failing before the wiring and passing after. Also corrected a stale artifact: the design's Known limit L1 still claimed rung 3 was not in this deliverable, written while it was halted and false once the maintainer unblocked it. Verified after both fixes: the three bypass probes all return read-only, the per-role table is 35/35 byte-identical, and both hold self-invalidation cases still throw. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the marker rung, the derived sandbox, and the bare plan-number depends_on Reference: the runtime precedence ladder in docs/CLI-TOOLS.md gains the install marker rung; docs/COMMANDS.md documents validate agents' new sandbox_posture field; docs/reference/plan-md.md documents that depends_on accepts the bare plan number. Explanation: a docs/features fragment keyed id 3897, so it cannot collide with a concurrent PR hand-allocating a section number, regenerated into FEATURES.md. ADR-3473 §8.3 gains an ANSWER blockquote in the document's own correction style, recording what was measured and built against the section's 2026-08-26 correction - including the qualification that checkAgentsInstalled itself still checks presence only, and the semantic assertion lives in a sibling wired into validate agents rather than folded into it. No how-to. Both user-visible changes are zero-step: a non-Claude install resolving its own runtime, and plans executing in their declared waves, both happen without the user doing anything. docs/how-to/control-the-reported-host-runtime.md covers a DIFFERENT ladder (resolveReportedRuntime / agent_runtime) that this change does not touch, and was deliberately left alone rather than edited by association. No tutorial - nothing multi-step to walk through. docs/AGENTS.md unchanged: it documents Claude-side tools frontmatter, never Codex sandbox_mode, and the emitted tools contract did not change. The prompt layer documents depends_on only by example, not by schema, so nothing there needed editing - and few-shot-examples/plan-checker.md already showed depends_on: ['01'], which now actually resolves. Translated copies of plan-md.md are untouched; the project treats translations as community-maintained. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): move the sandbox derivation out of the installer, off the install path, and off a third parser The full suite came back with 26 failures across four files. Three distinct causes, mapped individually rather than assuming the first explained the rest. A. Requiring bin/install.js printed the GSD banner to stdout and corrupted `validate agents` JSON. Unexpected token '', "[36m ██"... is not valid JSON checkCodexSandboxPosture reached deriveCodexSandboxMode by lazily requiring bin/install.js, whose module load prints the ASCII banner. So the command emitted banner bytes before its JSON and every JSON consumer broke, including ten tests that predate this branch. src/ reaching into bin/ was backwards layering that happened to also be loud. The derivation now lives in src/codex-agent-toml.cts - the existing Codex TOML domain module, no new module and no six-gate ripple - and both bin/install.js and src/agent-install-check.cts import it. One owner, which is §8.3's rule applied to the fix for §8.3. B. The stale-hold throw fired on a legitimate partial source dir, and masked a security assertion. validateCodexSandboxHolds treated "this hold's .md is absent from the install SOURCE dir" as a stale hold and threw. A test fixture, or any partial install source, legitimately contains a couple of agents. Worse, it threw BEFORE the path-escape check, so a test asserting that a `../../evil` frontmatter name is rejected got my unrelated error instead of the traversal rejection it was written for. A fail-closed check of mine was hiding a real security check. The "no stale holds, shrink-only" invariant is a property of the repo's canonical agents/ roster, not of whatever directory an install happens to read. It is off the runtime path and enforced where it belongs, in the tests that already existed for it. A partial source dir now installs cleanly, and the evil-name case throws with its own escapes-configHome message again. C. T8 depended on ambient process.env state. The marker/env parity assertion round-tripped through live process.env. It now compares against resolveExplicitRuntime's already-exported dependency-injection parameter - deterministic and hermetic, same claim. Proven still falsifiable rather than assumed: with the marker rung's normalization temporarily bypassed the two rungs diverge ("codex\n../../etc/passwd" vs "codex-../../etc/passwd") and the assertion fails, then passes again once reverted. One correction folded in along the way. The first version of the move added private _extractFrontmatterAndBody/_extractFrontmatterField helpers to codex-agent-toml.cts - a THIRD copy of frontmatter extraction, where the graph already shows two (bin/install.js:2348, runtime-artifact-conversion.cts:893). Adding a third inside the epic whose thesis is one implementation per rule is not defensible. deriveCodexSandboxMode no longer parses anything: it takes (identity, toolsValue) and each caller supplies the tools value using the extractor it already has. Both helpers are deleted. The identity argument is still the filename stem, so the fail-open fix is untouched. Verified after all three: `validate agents --raw` emits parseable JSON with no banner and both posture fields; the four hold-bypass probes still return read-only; the per-role table is 35/35 byte-identical at 26 read-only / 9 workspace-write; the hold list is still 16. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): drop a dev-only transitive dep, make the derivation total, retire a stale fallback test Suite down to 7 failures from 26. Three more causes, mapped individually. A. My extractor import dragged in a script that does not exist in an installed tree. Cannot find module '../../../scripts/fix-slash-commands.cjs' Chain: src/agent-install-check.cts imported runtime-artifact-conversion.cjs, which requires command-roster.cjs, whose line 36 requires ../../../scripts/fix-slash-commands.cjs. That path exists in the repo and not in an install, so every test exercising a synthetic install dir died at module load. I picked that extractor for convenience without checking what it pulls in - the same mistake that produced the banner bug, one layer further out. agent-install-check now uses a single-purpose extractToolsLine on codex-agent-toml.cts. That is deliberately NOT a general frontmatter parser: we deleted those helpers a commit ago for good reason, and this reads one line. Verified from outside the repo root that requiring either module prints nothing and does not throw. B. A test pinned the deleted name-based fallback. 'defaults unknown agents to read-only' called generateCodexAgentToml with a fixture declaring tools: Read, Write, Edit. Under derivation an unknown agent with a writing contract correctly derives workspace-write - design row S6, a new writing role gets the contract, not the pin. The behavior it asserted was the silent fallback this rung deleted; identity no longer decides the sandbox. Replaced with two rows rather than a flipped string: no tools declared -> read-only (absence is not a grant), and Write/Edit declared -> workspace-write. Strictly more coverage than the row it replaces. C. The stale-hold check still threw per derivation call. Last commit took the roster-existence check off the install path, but deriveCodexSandboxMode itself still threw when a hold's role did not derive broader FOR THE CONTENT IT WAS HANDED - so it fired on any synthetic fixture for a held role. The throw is gone, and it cost nothing: if a held role's content does not derive broader, the hold pins read-only and derivation returns read-only anyway, so the hold is a no-op and there is nothing to fail about. The staleness invariant is a property of the real agents/ roster, and validateCodexSandboxHolds still enforces it there - confirmed against the real roster after the change, not assumed. deriveCodexSandboxMode is now total: every (identity, toolsValue) including undefined and null returns read-only or workspace-write, never throws. Verified: validate agents emits parseable JSON; the four hold-bypass probes return read-only; the per-role table is 35/35 at 26 read-only / 9 workspace-write. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): put the rung-3 decision in the shipped docs instead of pointing at an ignored path The ADR entry and the feature fragment both ended their rung-3 explanation with "see .gsd/phase/feat-3897-adr3473-83-rungs/45-decision-rung3-sandbox.md". That directory is gitignored (.gitignore:55), so the rationale for holding 16 roles at read-only was reachable only from the machine that produced it. A reader of the ADR got a pointer to nothing. Both now carry the reasoning inline: the criterion asks both that the sandbox derive from the declared tool contract and that no role gain a broader sandbox, and those cannot both hold, because a faithful derivation widens 16 roles the deleted map never listed and that fell through its silent read-only default. The resolution is derive-and-hold - the derivation owns the rule now, each hold is released as its enforcement question is answered, and a hold is reversible where a widened sandbox that turns out to be enforced is not. Checked before assuming this was a defect class: CONTEXT.md cites .gsd/phase/<slug>/40-design.md as its standard Design: provenance line in eight module entries, and four other shipped docs do the same. Citing a phase artifact is an established convention here, so those are left alone. What was wrong was specific to these two: they put load-bearing rationale behind the pointer instead of provenance. docs/FEATURES.md regenerated from the fragment via scripts/gen-features.cjs rather than hand-edited. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#3897): close a fail-open, stop a silent mis-resolution, and read a declaration as a declaration Two orthogonal reviews on the shipped sha. Three of the findings are the same failure class this epic exists to close, committed inside it. 1. BLOCKER - the sandbox was decided for one identity and applied to another. bin/install.js derived sandbox_mode for the filename stem and then wrote the result to `${name}.toml`, where name comes from the file's own frontmatter. Make the two disagree and a HELD role's artifact goes wide: rename gsd-doc-writer.md -> gsd-doc-writer-v2.md, keep name: gsd-doc-writer -> stem is unheld, derives workspace-write, lands on gsd-doc-writer.toml add any gsd-*.md whose frontmatter name: is a held role -> clobbers that role's toml with workspace-write Both emit read-only on origin/next, because the deleted map was an allowlist and a miss fell back safe. This is a regression my change introduced. The previous review round moved the HOLD KEY off frontmatter to the filename stem and left the OUTPUT PATH on frontmatter; my own comment at install.js:6985 calls that value attacker-editable, four lines above the line that uses it as the filename. The decision is now made over BOTH candidate identities, most-restrictive wins: if either the stem or the emitted name is held, the mode is read-only. 2. MAJOR - hold matching was toLowerCase() only, so confusables escaped. Turkish dotted/dotless i, fullwidth, NFD, trailing space/NBSP/dot/newline, ./ and ../agents/ all slipped the hold and emitted workspace-write. Identities are now basenamed, trimmed of NBSP/zero-width/control characters, NFKC-normalized and lowercased - and anything still carrying a character outside [a-z0-9._-] is treated as suspicious and derives read-only. We do not enumerate confusables; every shipped roster file is ASCII, so refusing to widen on an identity we cannot recognize is fail-closed with no false positives on real content. 3. MAJOR - the short-form depends_on tier mis-resolved SILENTLY. shortFormToId keyed on the last dash-segment of any canonical id with no constraint that it is a plan number, so a phase holding 09-FIX-auth-PLAN.md made depends_on: ["auth"] bind at wave 2 with zero warnings. This is the worst shape in the epic: the unresolvable-token warning fires on a DROPPED token, so a MIS-RESOLVED one is invisible and the tool reports a confident wave assignment built from a wrong edge. A wrong edge is worse than a missing one. The segment must now match /^\d+$/, which is exactly the contract docs/reference/plan-md.md already documents. This tier was recovered verbatim from the retired SDK lineage, which carried the same defect; we are deliberately NOT preserving it bug-for-bug, and the comment says so, so the next reader does not "restore" it. 4. MAJOR - the derivation was reading a declaration as an absence. extractToolsLine read one line, so a YAML list-form tools: block returned only its first item. Two roster files use list form, and gsd-nyquist-auditor declares Write and Edit there - parsed as "- Read", found no write tool, and emitted read-only. Rung 3's headline claim is that sandbox_mode derives from the declared tool contract; that claim was false for 2 of 35 roles and materially wrong for 1. Reading a declaration as an absence is the silent-drop class this epic exists to close. Renamed extractToolsValue and taught it both shapes. gsd-nyquist-auditor now derives workspace-write and joins CODEX_SANDBOX_HOLDS as its 17th entry, per the standing derive-and-hold decision - so emitted TOML stays byte-identical at 26 read-only / 9 workspace-write while the hold list finally records every role that would widen. A previous pass declined this fix because it moved the count; that inverts the priority. Byte-identity is preserved THROUGH the hold, not by leaving a parser broken. Divergence check, because this is where that bug hides: both paths feeding sandbox derivation - install.js's emitter and checkCodexSandboxPosture - now route through the one extractor. The tools readers in runtime-artifact-conversion and install.js's other frontmatter call sites serve Claude-side emission and do not feed sandbox derivation. Also fixed, each real: the posture check's `found` used a naive whole-file regex where its own sibling uses the block-aware scanner, so prose inside developer_instructions produced a false violation; `found` skipped truncatePostureValue and leaked a 300-char value into validate agents output; deriveCodexSandboxMode's absolute never-throws claim was false for an object with a throwing toString; T49 could not falsify cross-phase leakage (its target phase had its own 01, so a globally-scoped map passed too); T20/N6 iterated a hardcoded table and pinned the FIXTURE size, so a 36th agent would be silently unchecked; three tests reimplemented the code they were testing instead of importing it; and T2-T4 deleted GSD_RUNTIME without restoring it. Verified: hold list 17, gsd-nyquist-auditor derives workspace-write unheld and emits read-only held, roster 35/35 at 26/9, depends_on ["auth"] no longer resolves while ["01"] still does, both identity-bypass cases and every confusable vector emit read-only. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * docs(#3897): the hold list is 17, and the reason the 17th was missing The count read 16 because the derivation could not read the declaration it claimed to derive from: the tools reader was single-line, so a YAML list-form tools: block returned only its first item and gsd-nyquist-auditor's declared Write and Edit were read as an absence. Both the ADR entry and the feature fragment now carry the corrected count and the reason for it, rather than a silently updated number. Deriving from a declaration you cannot parse is not deriving, and a flattering count is worse than a wrong one because it looks settled. docs/FEATURES.md regenerated from the fragment. Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#3897): backfill changeset pr number Refs #3897 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: sim <sim@local> Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Behruz Nassre Esfahani
|
622f43353c |
fix(#3299): tracer feedback gate honors workflow.human_verify_mode (#3390)
* fix(#3299): tracer feedback gate honors workflow.human_verify_mode
The tracer feedback gate (#2294) predates `workflow.human_verify_mode`
(#3309, whose scope was the planner and verifier only), and branched on
auto-mode alone. Under the documented `end-of-phase` default an
interactive run therefore halted after EVERY `type="tracer"` task,
synthesizing a `checkpoint:human-verify` no planner ever emitted and
asking the user to retype a verdict the executor had just computed —
at the cost of a full executor cold-start each time.
Planner-side suppression cannot reach this halt because the executor
synthesizes it at runtime, which is why #3309 did not close it.
The gate now branches on HUMAN_VERIFY_MODE in the interactive path:
under `end-of-phase` an automated-only tracer `<verify>` is re-run and,
on success, expansion continues with no checkpoint. HALT-on-failure is
unchanged. `mid-flight`, `gate="blocking-human"`, and tracers carrying
genuine `<human-check>` evidence all still stop; the autonomous branch
is untouched.
`--default end-of-phase` on the config read is load-bearing, not
decorative: `workflow.human_verify_mode` is absent from SCHEMA_DEFAULTS,
so a bare `config-get` exits non-zero with `Key not found` on any
project whose config.json predates #3309 — which is the reporter's
exact config and every pre-existing project.
Both copies of the rule (workflows/execute-plan.md and
agents/gsd-executor.md) are updated together; the reference doc records
the seam and the human-check-still-halts rationale so it cannot recur.
Fixes #3299
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): add changeset
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): reconcile the canonical schema table and the stale acceptance test
Review round 1 (trek-e) — three items, all in the drift class this PR is
about, two of them landed inside this PR's own diff.
1. docs/reference/plan-md.md:233 — CONTEXT.md names this file the canonical
schema reference for the tracer task-type contract, and its Task-types row
still claimed interactive runs unconditionally present a
checkpoint:human-verify. CONTEXT.md and docs/AGENTS.md were updated in the
first round; this one was missed, so the authoritative reference was the
wrong answer. The row now carries the human_verify_mode-conditional
behavior and points at the canonical precedence chain.
2. tests/tracer-bullet.test.cjs — the docs assertion only checked that a
tracer ROW EXISTS, never its content, which is why CI could not see the
drift. It now asserts the row's actual claims and rejects the pre-#3299
wording. Separately, the #1945 acceptance test named 'interactive run emits
checkpoint:human-verify after the tracer' kept passing only because its
substrings still occur in the fallback clause, while its name asserted the
opposite of shipped behavior. Renamed and narrowed to what #1945 still
guarantees, plus a new interactiveIsConditional pin so the unconditional
prose cannot be restored under a passing substring check.
3. plan-md.md's <verify> row now documents that the legacy bare-text form
(valid, and still shown at :179) does not reach the #3299 auto-continue —
only a <verify> carrying <automated> does — so the benefit is silently
unreachable for tracers using that format.
Mutation-verified: reverting the plan-md row fails 1 test; reverting the
executor's interactive branch fails 4.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* fix(#3299): make the tracer gate reachable from the planner template, and bind the assertions
Peer review round 3 found two Majors, both verified by reproducing the
mutation before fixing.
MAJOR 1 — the fix was largely inert on its own default path.
agents/gsd-planner.md's Nyquist Rule (:191) says every <verify> includes
<automated>, but the tracer-specific template twelve lines later emitted the
legacy bare-text form. The gate auto-continues only on a <verify> carrying
only <automated>, so every tracer produced from the canonical template fell
to the STOP fallback and #3299's benefit was unreachable for exactly the task
type it targets. Template now wraps in <automated>; a contract assertion pins
it so the two cannot drift apart again.
MAJOR 2 — the new assertions did not bind condition to action.
Appending 'Nevertheless, interactive runs always present a
checkpoint:human-verify' to the canonical row, and 'then immediately STOP and
return a checkpoint:human-verify' to the auto-continue clause in BOTH
operative copies, restored unconditional interactive checkpointing and left
the suite 35/35 green. Every required keyword still matched. Fixed by:
- clause 2 must now contain no STOP outcome and emit no checkpoint at all —
'never a checkpoint' has to be true OF the clause, not merely stated in it;
- interactiveIsConditional replaced with the ordered-clause parse plus the
same no-STOP property, instead of proving only that HUMAN_VERIFY_MODE
appears somewhere on the line;
- the plan-md.md Autonomy cell is now pinned EXACTLY rather than by keyword
presence. Deliberately brittle: CONTEXT.md names that table the canonical
schema reference, so a wording change must be a conscious edit in both
places.
Mutation-verified after the fix: the combined semantic regression now fails 3
tests; reverting the planner template fails 1.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): exact-pin the safety clauses instead of blacklisting outcome verbs
Peer review round 4. Blacklisting did not hold, twice over:
- Round 3 banned literal STOP and the 'return a'/'present a' checkpoint
forms in the auto-continue clause. Round 4 defeated that by appending
'then pause and invoke checkpoint_protocol with a checkpoint:human-verify
before expansion' — none of the banned tokens, same restored interruption
after every successful tracer. 36/36 passed.
- The planner guard looked for <automated> anywhere inside <verify>, so
'<verify>[...]<!--<automated>--></verify>' satisfied it while leaving the
legacy bare form operative. 107/107 passed across tracer, planner and the
three size-cap suites.
Synonyms are unbounded; the clauses are not. Both are now pinned exactly on
normalized whitespace, the same approach already proven on the plan-md.md
Autonomy cell, with defence-in-depth checks behind them: no checkpoint-emitting
or blocking outcome in any wording inside clause 2, and the planner's <verify>
body must be exactly one non-empty <automated> child with no commented markup.
These pins are deliberately brittle. Each is a safety contract, so changing the
behavior must be a conscious edit in both the prose and the expectation.
Mutation-verified: the synonym-checkpoint mutation fails 1; the commented-out
wrapper fails 1; the round-3 literal-STOP + contradictory-doc-row regression
fails 3.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): strip comments, require uniqueness, pin whole regions
Peer review round 5. Exact-pinning one clause was still bypassable two ways,
both reproduced before fixing (each left the suite fully green):
- COMMENTED DECOYS. Put the correct text in an HTML comment followed by a live
wrong copy: every extractor selected the commented decoy. Worked against the
planner template, the canonical plan-md.md row, and both executor branches.
- SURROUNDING OVERRIDE. Insert 'after every tracer, pause and invoke
checkpoint_protocol before expansion, regardless of the mode-specific rules
below' immediately ABOVE the pinned clause, or 'ignore row 3; always wait for
approval' below the canonical table. The pinned text was untouched, so
equality held while the shipped meaning inverted.
The shape that holds, applied to every operative surface:
1. strip HTML comments BEFORE selecting, so a decoy cannot be chosen;
2. require the structural anchor to occur EXACTLY ONCE, so a live second copy
cannot hide behind a correct first one;
3. pin the ENTIRE decision region, not one clause, so no unparsed prefix or
suffix can override what the pin proves.
Applied to: the executor's whole tracer branch, execute-plan.md's whole
dispatch line, checkpoints.md's whole precedence section, and plan-md.md's
Autonomy cell.
Also addresses the round-5 Minor: the planner template is now asserted
STRUCTURALLY (exactly one <verify> in the fenced block, body exactly one
non-empty <automated> child) rather than pinning the descriptive placeholder
verbatim, so behavior-preserving wording changes no longer false-fail. The
clause and section pins keep their exact form — those have a safety rationale
the placeholder copy does not.
Mutation-verified, all six rounds: override-above-clause 1; commented decoy row
1; commented decoy branch 1; ignore-row-3 override 1; synonym checkpoint 1;
commented-out wrapper 2.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): drop the superseded exact-placeholder planner assertion
Peer review round 6, Minor. The round-5 brittleness fix ADDED a structural
planner assertion but left the old exact-placeholder one in place, so the
over-brittleness it was meant to remove was still live: rewording the
descriptive placeholder while preserving exactly one non-empty direct
<automated> child failed the old test and passed the new one.
Removed the old test. The structural assertion is the real contract — the gate
auto-continues on the SHAPE of the verify, not on the wording of a placeholder.
Verified both directions: a behavior-preserving reword now passes; reverting the
template to bare <verify> still fails.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): select operative prose via parsePredicates, not a hand-rolled scanner
Peer review round 7. I had judged the round-6 selector bypass adversarial-only
and out of scope, intending to disclose it. Both premises were wrong, and the
review said so:
- 'Needs new src API' — false. parsePredicates is ALREADY a public export and
internally uses the repo's interleaved fence/comment scanner. Instrumenting
candidate lines as throwaway predicate declarations borrows that scanner with
no src change at all.
- 'Adversarial-only' — false, and this is the part that mattered. Two ORDINARY
edits silently turned the guards into decoy checks:
* a forgotten '-->' comments the live rule through to EOF, and the
balanced-only stripper still saw and accepted the commented rule;
* a normal fenced documentation example of the rule, plus a whitespace-only
reformat of the live list item, made the selector choose the example.
Neither needs intent. A dangling comment is a typo; a fenced example is good
documentation. Together they reproduce exactly the accidental drift #3299 came
from — with CI green.
The selection layer now defers to parsePredicates for operativeness, uses
whitespace-tolerant anchors so a reformat cannot decouple the live line from its
pin, extracts regions by operative line index rather than string search, and
carries a self-guard test proving fenced / balanced-commented /
after-unclosed-comment copies are all excluded. The helper also ignores indexes
it did not inject, so a pre-existing GSDTEST.CANDIDATE line cannot pollute it.
Verified both ordinary-edit scenarios now fail the suite (each was green before).
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): close the operative-selection gaps the maintainer blocked on
trek-e's Blocker: the operative-line selection layer had three gaps, all
reachable by ordinary future doc edits rather than sabotage. He independently
found a fourth I had not disclosed. All are fixed.
1. INDENTATION PROMOTION (his find, not in my disclosure). The instrumentation
replaced a matched candidate with an UNINDENTED marker regardless of the
original line's indentation. A 4-space-indented CommonMark code block is not
skipped by parsePredicates (it accepts indented declarations by design), so
stripping the indent PROMOTED an indented decoy to operative — the exact
inversion of the guard's purpose. The marker now preserves the original
indent, and a candidate that is itself indented 4+ spaces is never injected.
2. NO SET MEMBERSHIP. The filter accepted any in-range integer, so a
pre-existing literal GSDTEST.CANDIDATE=<valid index> in source text could
pollute the count. Now filters on a Set of the indexes actually injected on
this call.
3. RAW FENCE SELECTION (planner). The template test matched the first raw
```xml fence after the marker with no fence/comment awareness — the one
selection in the suite that was not operative-aware — so a commented-out
decoy template between the marker and the real one would be selected while
the live template regressed. The opener must now be operative AND the first
non-blank line after the marker.
4. RAW END ANCHOR (regionFrom). The end anchor was tested against raw lines, so
a fenced example containing a ### / <type line truncated the pinned region
early — a false FAILURE on a legitimate doc edit. End anchors now go through
the same operative filter as start anchors.
Mutation-verified: the indented-decoy + whitespace-varied-anchor combination
and the commented-out fence decoy each now fail the suite (both passed clean
before). Truncation is confirmed fixed by extraction — the region spans the
full section and retains the content following a fenced example, where it
previously stopped at it.
Note on the remaining brittleness: adding a fenced example INSIDE a pinned
region still fails the whole-region exact pin. That is the intended tradeoff
for a safety contract, not the truncation defect, and is called out as such.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* test(#3299): allow-list operative indentation; pin marker provenance
Review round 9.
BLOCKER — the round-8 indentation guard was written as a DENY-list,
/^(?: {4,}|\t)/, and CommonMark has more indented-code forms than that
enumerates: " \t", " \t" and " \t" all open an indented code block and all
slipped through, so an indented decoy was still promoted to operative while the
live rule regressed (34/34 green). Inverted to an allow-list — only 0-3 literal
spaces is ordinary block indentation; anything else is code. Enumerating the
bad shapes was the error, not the specific regex.
MINOR — the injected-index Set validated the marker's VALUE but not its SOURCE.
A pre-existing literal `GSDTEST.CANDIDATE=<n>` could name an index that some
other (skipped) candidate had contributed to the set, and be accepted. Now also
requires p.line - 1 === Number(p.value): the predicate must have been parsed
from the line it names.
MINOR (false negative) — ```xml title=x is a valid CommonMark info string, and
requiring exactly ```xml failed the suite (33/34) on a behavior-preserving edit.
Both the opener assertion and the extraction now accept an info string.
Mutation-verified: the mixed " \t" decoy and the forged-provenance marker each
now fail; the info-string fence no longer false-fails.
KNOWN LIMITATION, disclosed on the PR rather than papered over: parsePredicates
is a predicate parser, not a general CommonMark operativeness oracle. Two
standards-valid constructs still read as operative — a lazy blockquote
continuation line (state opens only on a line that literally starts with ">"),
and a comment opened mid-line ("prose <!--", where state opens only when the
trimmed line STARTS with "<!--"). Closing those means either teaching the shared
src/context-predicates.cts about container/lazy-continuation state — a change to
a module every health rule consumes, well outside a tracer-gate fix — or
hand-rolling a CommonMark parser inside a test, which is how this suite got into
trouble in the first place. Left for the maintainer to scope.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
* chore(#3299): re-arm the execute-plan.md emitted-drift ack after the base merge
The #3299 ack rode on tests/emitted-drift-acks/2652-quick-diagnose-dispatch-isolation.json,
which upstream retired in
|
||
|
Tom Boucher
|
2f86278b5e |
fix(#3003): opt-in mechanism for intentional deletions in worktree.cleanup-wave (#3757)
* test(#3003): failing-first suite for declared deletions in cleanup-wave Binds the guard's opt-in before it exists, so the suite is RED against next. The rows that carry the weight are the over-authorization set: a directory declaration must not authorize its children, a glob declaration must authorize nothing, and a declaration must not act as a string prefix of another path. Each of those BLOCKS, and each would PASS under a prefix, glob, or startsWith matcher — which is how a path list quietly degrades into the boolean opt-in #3003 explicitly rejected. The glob row matters most: declaredScopePrefix already returns null ("matches everything") for a glob-leading pattern, correct for the advisory it serves and catastrophic for a gate. Also pinned: a failed deletion check blocks on its own reason rather than being filtered into a pass; the block detail names only the undeclared residue so the operator is not misdirected by paths that were fine; an entry with no declaration blocks exactly as before; junk and non-array declarations do not authorize; and a blocked entry still isolates rather than aborting the wave (#2852, which must stay fixed). Two advisory rows cover an interaction found while designing: git diff --name-only includes deleted paths, so without unioning the declaration into the #2596 scope check, authorizing a deletion would raise SCOPE_OUT_OF_DECLARED against the very path just authorized. A seeded property states the whole invariant the three over-authorization rows sample: a deletion merges iff its normalized path is in the declared set. * feat(#3003): declared deletions opt-in for the cleanup-wave guard The deletions guard blocked the merge-back of any executor branch whose diff removed a file, with no way to say a removal was intended. A plan that folded one test file into a sibling could not be merged by the tool meant to merge it, forcing a manual --no-ff outside the tool -- strictly less safe than what the guard protects against. A plan now declares removals in its own frontmatter (files_deleted), and that list rides the same path files_modified already travels: plan-document parse -> phase plan JSON -> the per-plan worktree gate -> record-agent/create --deletions -> declared_deletions on the manifest entry -> the guard. The guard blocks only the deletions NOT in that list. A path list rather than a boolean, per the pinned decision: a boolean disarms the guard for the whole entry, so an unexpected deletion riding along with a declared one would pass unnoticed. Matching is exact after the module's shared normalizer -- never a prefix, never a glob. Both would let one declaration authorize a whole set, which is the mass-deletion accident the guard exists to catch. That also means declaredScopePrefix is deliberately NOT reused here: it returns null ("matches everything") for a glob-leading pattern, which is right for the advisory it serves and would silently disarm a gate. The block detail now carries only the undeclared residue, so an operator is not sent looking at paths that were fine. A failed deletion check still blocks on its own reason and is never filtered into a pass. A blocked entry still isolates rather than aborting the wave (#2852). The #2596 scope advisory unions the declaration into its declared set -- git diff --name-only includes deleted paths, so without that, authorizing a deletion would immediately warn that the same path was out of declared scope. Optional and additive throughout: files_deleted is absent from PLAN_REQUIRED_FIELDS, a manifest entry without declared_deletions keeps the original unconditional block, and omitting --deletions leaves the on-disk entry shape untouched. Supersedes the spent #2856 emitted-drift ack entry for execute-phase.md, the same supersede that entry performed on #3370 and #3370 on #3324. * fix(#3003): wire --deletions on every dispatch surface, not just one Review found the feature inert on two of three dispatch paths. execute-phase.md (harness inline) passed --deletions, but the orchestrator-worktree path (executor-isolation-dispatch.md, worktree.create) and the Fleet-parallel batch path (capabilities/claude-orchestration/fragments/execute-wave-pre.md, worktree.record-agent) still passed only --files. A plan declaring files_deleted would have merged on one path and been blocked on the other two -- the exact bug #3003 exists to fix, left unfixed where most of the isolation actually runs. Worse, per-plan-worktree-gate.md already claimed --deletions was passed 'on the same worktree.record-agent / worktree.create calls', which was false for both untouched sites. A doc asserting coverage that does not exist is how a gap survives review. All four surfaces now pass the flag, verified by sweeping every .md under gsd-core/, capabilities/, commands/, skills/ and agents/ that invokes worktree.record-agent or worktree.create: each one that passes --files now also passes --deletions. The isolation-dispatch note explains why this flag, unlike --files, is not advisory -- omitting it does not skip a check, it blocks a merge the plan declared. Regenerates capability-registry.cjs, which the fragment edit made stale. Neither newly-grown file needs an emitted-drift ack: executor-isolation-dispatch.md sits under workflows/execute-phase/steps/ and execute-wave-pre.md under capabilities/, both outside currentSizes()'s non-recursive scan of gsd-core/workflows/ and agents/. * docs(#3003): document files_deleted where a plan author will actually find it The feature's entire user surface is one plan-frontmatter field, and the canonical reference for that frontmatter -- docs/reference/plan-md.md, the table that documents every other key -- never mentioned it. A field nobody can discover ships as a field nobody uses. Adds the files_deleted row and an example entry in all five locales (en, ja-JP, zh-CN, ko-KR, pt-BR), stating the property that makes the opt-in safe: matching is exact per path after separator normalization, with no globs and no directory prefixes, so a declaration can never authorize more than it literally lists, and omitting the field keeps the guard's original unconditional block. Also corrects two claims in the scope-conformance how-to that this change made false. Its opening paragraph described the recorded declared scope as files_modified alone; declared_deletions is now unioned into that comparison. Its "Renames are not detected specially" bullet asserted the deletions guard blocks any entry whose diff contains a deletion, full stop -- which was the whole point of #3003 and is no longer true. Reworked to say what now decides a rename's fate: declare the old path in files_deleted and both halves become ordinary paths for the advisory check, which is also why the old path needs no separate files_modified entry. Documentation that describes the pre-change behavior of the thing being changed is worse than no documentation, because a reader trusts it. * fix(#3003): close every review finding on the declared-deletions opt-in Two independent isolated reviewers, correctness and security. Neither found a blocker; both found real defects, and the directive treats a finding at any severity as blocking. All of them are fixed here. MAJOR -- the submodule worktree gate could not see a deletion-only plan. per-plan-worktree-gate.md intersected $SUBMODULE_PATHS against $PLAN_FILES alone, while $PLAN_DELETIONS was extracted and then never used. Before files_deleted existed, a path had to appear in files_modified to be planned at all, so the gate saw it; the new field plus the new docs telling authors a deleted path needs no files_modified entry opened a hole where a plan whose only submodule touch is a removal kept worktree isolation on -- the exact case #2772 disabled it for. Both channels now feed the intersection. Note the posture is deliberately the OPPOSITE of the cleanup-wave guard: there the channels stay apart because a deletion AUTHORIZATION must never be inferred; here they merge because a safety fallback must never MISS a touch. MAJOR -- same-wave conflict detection could not see a deletion. The planner's implicit-dependency rule compared files_modified only, so plan A editing src/x.ts and plan B declaring files_deleted: [src/x.ts] scored as conflict-free and ran in parallel: one branch removing what the other is writing, which is the sharpest conflict there is. Overlap is now computed across both channels. MINOR (both reviewers, one root cause) -- the advisory union gave one field two matching rules. declared_deletions was unioned into the scope list handed to planWaveScopeConformance, which reads it with prefix-and-glob semantics. So a field that is exact-match-only at the gate silently became wider at the advisory: ["*.md"], inert at the gate, yielded a null prefix meaning "matches everything" and muted the advisory completely, and ["src"] muted all of src/. The union also activated the advisory on plans that declared no modification scope at all, warning on every modified path. Replaced with subtraction from the findings, gated on files_modified alone. One field, one rule, everywhere. MINOR -- core.quotepath made the feature silently inert for non-ASCII paths. git emits "tests/\303\251.ts" C-escaped and quoted, which never equals the declared plain path, so a correctly declared deletion of tests/é.ts would block forever with nothing pointing at the encoding. Both diffs now pass -c core.quotepath=false. NIT -- flag() consumed a following flag as a value, so --deletions --files x swallowed --files and dropped both. Now treated as a missing declaration, which fails closed. Fixed at both call sites; the helper is duplicated verbatim in cmdWorktreeRecordAgent and cmdWorktreeCreate and leaving one would reintroduce it. TEST -- one test passed for the wrong reason. "a declared deletion is in scope for the advisory" asserted only that warnings omit the deleted path; under a full revert the entry blocks first, warnings come back empty, and the negative assertion passes anyway. It now asserts the entry actually merged, which is the load-bearing half. Four regressions added, one per fix above. Docs corrected rather than extended. The rename bullet in the scope-conformance how-to claimed a rename whose delete side is undeclared never reaches the advisory. Verified false: git's rename detection is on by default, so a pure rename is a single R entry that appears in no --diff-filter=D output and was never gated, before or after #3003. Only a rename that edits enough to fall below the similarity threshold decomposes into add+delete. The pre-existing sentence made the same wrong claim; this restates it correctly instead of sharpening the error. The localized plan-md.md reference edits are reverted: the PR template requires docs content added here to be English, and the translations already lag by three fields, so English-only is the repo's standing posture, not an oversight. Agent-file size caps respected: gsd-planner.md is XL-tier by bytes but carries a separate 49152-LF-CHAR cap asserted by four suites, so its edit is deliberately terse and lands at 49141 with 11 chars of headroom, with the rationale moved to docs/reference/plan-md.md, which has no cap. gsd-plan-checker.md lands at 49107 bytes, 45 under the LARGE cap. Both acks merged into the existing fragments that already name those paths, since two ack sources may never name the same path. * fix(#3003): decode git's path quoting instead of changing the git argv The previous commit's non-ASCII fix turned the remote suite red: 44 failures, 42 of them "unexpected git call: -c core.quotepath=false diff --diff-filter=D --name-only ...". The suite's git mocks match on exact argv, so adding two flags to the deletions diff and the advisory diff invalidated every existing fixture in tests/worktree-safety.test.cjs. Rewriting dozens of fixtures to accommodate one flag would be paying a large Hyrum's-law bill to fix a small defect. Both execGit calls are reverted to their original argv. The C-quoting is now decoded in normalizeScopePath instead, via a new decodeGitQuotedPath helper. That is the better fix on its own merits, not merely the cheaper one: the git argv is untouched so no fixture moves, the decode lands on the ONE normalizer already applied to both sides of the comparison so the declared and reported paths cannot disagree, and it holds regardless of the user's own core.quotepath setting rather than only when we remember to override it. A value not wrapped in a leading AND trailing quote is returned completely untouched, so the plain-ASCII path -- the overwhelmingly common case -- is byte-identical to before. Escapes decode to BYTES collected into a Buffer and UTF-8 decoded only at the end, because \303\251 is two bytes forming one character and decoding them separately yields mojibake. Malformed input never throws: a trailing lone backslash or a short octal escape degrades to the literal character, since one bad path must not take down a cleanup wave. Caught while reviewing the helper: the non-escape branch pushed a UTF-16 code unit rather than UTF-8 bytes. Git always escapes non-ASCII so its own output was fine, but this normalizer runs on the DECLARED side too, and an author may write a quoted path holding a literal é -- pushing 0xE9 alone is invalid UTF-8, so the declaration would decode to a replacement character and silently stop matching. That is precisely the failure this change removes, reintroduced on the other side of the comparison. Now converts whole code points, surrogate pairs intact. The other 2 failures: tests/parallel-dependent-plans.test.cjs pins the exact unbackticked substring "files_modified overlap" in gsd-planner.md, and rewording that comment to "declared-scope overlap" deleted it. The comment is restored verbatim and the files_deleted change rides in the pseudocode and the Rule sentence instead. Recorded in the ack fragment so the next contributor does not rediscover it the same way. Four regression tests cover the decode through the public cleanup-wave seam (the helper is module-private): a declared non-ASCII deletion merges against a C-quoted git report, the symmetric case where the DECLARATION is the quoted form, an undeclared non-ASCII deletion still blocks with the residue naming the decoded path an operator can act on, and a path merely containing a quote is left alone. Plain ASCII was already covered and is not duplicated. * fix(#3003): revert the leading-dash flag guard, the review nit was wrong The remote suite came back with 2 failures, down from 44, and both point at the same thing: tests/worktree-safety.test.cjs:7045 already pins the opposite contract, deliberately. test('a flag-shaped --files value is not re-parsed as a flag', ...) recordAgent(['--files', '--branch']) -> files_modified === ['--branch'] -> branch === 'worktree-agent-a1' ("the real --branch value must be untouched") So consuming the next argv element positionally, whatever its shape, is the tested intent of this parser, not an oversight. The security reviewer's nit claimed --deletions --files x would "swallow --files and drop both". It does not: each flag runs its own indexOf, so --deletions records the literal '--files' while --files independently still resolves to x. And that literal is a path git never reports as deleted, so it authorizes nothing -- already fail-closed with no guard at all. The guard bought no safety and silently changed --files behavior along the way, outside this issue's scope. Reverted at both call sites, which are byte-identical again, along with the test asserting the reverted behavior and the docs sentence describing it. The nit is recorded as REJECTED in the review artifact with the reasoning above, rather than as fixed -- a finding that turns out to be wrong should leave a trace of why, or the next reviewer files it again. docs/CLI-TOOLS.md now states the positional-read behavior plainly instead, so the next person meets it as documented intent rather than rediscovering it through a red suite. * chore(#3003): backfill changeset pr number to 3757 * test(#3003): cover parsePlanDocument's filesDeleted branch to clear the mutation gate CI's Stryker shard for plan-document failed at 73.28 against a break threshold of 75: 170 killed, 62 survived, 232 total. Eight of those survivors are the filesDeleted block this issue added to parsePlanDocument, which shipped with no direct coverage at all -- the field was exercised end to end through the cleanup-wave tests, but the parser itself was never called with a plan that declares it, so every mutant in the block lived. Four tests, each pinned to specific mutants rather than written for coverage percentage: - absent key yields exactly [] -- kills the array-literal seed (["Stryker was here"]) and the `fmDeleted = true` conditional, which would otherwise produce ["true"] - a scalar underscore `files_deleted:` wraps into a one-element array -- kills `fmDeleted = false`, the `&&` logical-operator swap, the `fm[""]` string mutation on the first operand, the emptied if-block, and the ternary's non-array branch - an array-valued hyphenated `files-deleted:` maps element-wise -- kills the `fm[""]` mutation on the SECOND operand (only reachable when the legacy hyphen alias is the one carrying the value) and the ternary's array branch - an empty list yields [] -- boundary case, and a genuinely distinct one from the absent key: [] is truthy in JS so it ENTERS the if, and only Array.isArray's true branch mapping over nothing produces the same [] Threshold arithmetic: 174 of 232 are needed for 75%, and these take it to about 178, so the shard clears with margin rather than landing on the line. Every expected value was confirmed by executing the built parser before being asserted, not inferred from reading the source. --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
7976b1ca0d |
feat(#1689): per-plan agent_hint executor routing (#3417)
* feat(#1689): per-plan agent_hint executor routing Option A per-plan specialist routing: a plan with an `agent_hint:` frontmatter field is dispatched to that subagent instead of gsd-executor when it resolves on the active runtime; absent/unresolved/disabled falls back to gsd-executor (byte-identical). Default-on via workflow.agent_hint_routing. - src/phase.cts: parse agent_hint into the plan-index JSON (plan_json.agent_hint) - agent-install-check.cts: resolveAgentHint() reuses getAgentsDir + runtime filename variants; probes project + global agent dirs; fails closed; rejects path-traversing names - gsd-tools.cjs: 'resolve-agent' query route (fail-closed to gsd-executor; --raw/--json) - execute-phase.md: lean per-plan reference + {EXECUTOR_TYPE} placeholder (host stays under the ADR-857 Phase 6 byte ceiling) - execute-phase/steps/per-plan-executor-routing.md: resolution logic (Agent()-based dispatch; advisory on orchestrator-worktree) - config: workflow.agent_hint_routing (validKey, default-on via SCHEMA_DEFAULTS, boolean validator) - docs (CONFIGURATION.md, plan-md.md), changeset, tests/agent-hint-routing-1689.test.cjs (17 tests) * chore(#1689): backfill changeset PR number (#3417) * chore(#1689): regenerate install-tree fixtures for new workflow fragment * chore(#1689): ack deliberate execute-phase.md growth (agent_hint routing) * test(#1689): SPAWN contract allows parameterized subagent_type placeholder agent-frontmatter's spawn-type checks scanned subagent_type="..." as a concrete agent name. execute-phase now uses subagent_type="{EXECUTOR_TYPE}" (a runtime placeholder resolved via resolve-agent, default gsd-executor). Skip {TOKEN} placeholders in both the known-type and <available_agent_types> checks; execute-phase still lists the built-in roster incl. gsd-executor. * fix(#1689): CI conformance for the routing fragment - per-plan-executor-routing.md: add the canonical runtime-launcher preamble to its gsd_run block (runtime-launcher-parity #373), matching sibling step fragments. - agent-install-check.cts: drop a literal ~/.claude/agents path from the resolveAgentHint JSDoc so it does not leak into the compiled engine .cjs (cline install leak guard). --------- Co-authored-by: sim <sim@local> |
||
|
Tom Boucher
|
ffd5370464 |
fix(#2903): use the command form that actually works in reader-facing docs (#3047)
* fix(#2903): use the command form that actually works in reader-facing docs Docs told readers to type the colon form, which no runtime registers -- 18 of 19 runtimes use slash-hyphen and the 19th uses shell-var -- so anyone copying an example got an unrecognized command. Swept 178 occurrences across 53 files, locale mirrors included so they do not re-diverge from English. The colon form is a source-authoring token, not a user-facing one: install-time converters key on it to produce the hyphen form runtimes actually register. So the sweep is scoped, and three things are deliberately left alone: - ADRs, which are a historical record; editing their prose falsifies what was written at the time. - The legacy release-notes archive, pending a maintainer decision on whether it follows the same historical carve-out. Excluding it keeps a later reversal additive rather than a revert. - Source artifacts under commands, workflows and agents, where the colon form is load-bearing. Rewriting those would break the installed-skill guarantee across every runtime -- the single largest hazard here. The plugin namespace form is a real, separate token and survives untouched. Adds a lint enforcing exactly that boundary, since the correct form genuinely differs by directory and nothing previously caught the drift. Also fixes a hardcoded colon form in the capability-matrix generator. The sweep alone would have left the generated matrix disagreeing with the template that produces it, so the fix is at the source and the output regenerated. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2903): stop the sweep misquoting source frontmatter Adversarial review caught three lines where the sweep rewrote a citation of the literal YAML name: key from a source command file. That key genuinely is the colon form -- this change's own carve-out logic says source-authoring tokens keep it -- so the docs ended up misquoting the real files. One of the three is an acceptance-checklist assertion, which the sweep turned into a false statement. Restored the three citations to match their sources verbatim, surgically: where a line carried both a name: citation and a real reader-facing slash command, only the citation reverted and the command stayed corrected. The guard needed the same distinction, or it would have flagged the restoration and reddened the build: a gsd:<cmd> token preceded by name: is a citation of a source token and is now permitted. The exemption is deliberately narrow -- a bare gsd:<cmd> anywhere else still fails -- with a test pinning that narrowness. Also makes the detection case-insensitive. Review found /GSD:next slipped through silently; no such casing exists in the tree today, so this closes a latent gap rather than fixing a live one. Swept the whole tree for further corrupted citations: none beyond the three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(#2903): retire the stale-next invariant and sweep next like every other command Maintainer decision on a genuine conflict between two contracts. Invariant #3054 banned the literal /gsd-next from user-facing docs because it named a retired workflow-advance command. But commands/gsd/next.md is a live command -- the state-aware smart-entry launcher -- and this issue requires docs to use the hyphen form every runtime actually registers. Both could not hold for this one command, so docs had been sidestepping the ban by keeping the colon form, which is exactly the defect this issue exists to remove. FEATURES.md already recorded the reassignment: the hyphen form "is not the retired workflow-advance command; it is reserved for the state-aware smart-entry launcher. Workflow advancement remains under /gsd-progress --next." With that reassignment the invariant's premise is obsolete and the guard now contradicts the documented command form, so it is retired with a comment recording why rather than deleted silently. next is now swept like every other command, and the earlier exemption added to the new guard is removed so nothing is special-cased. Four citations of the literal name: frontmatter key stay in colon form, because the source file really does carry name: gsd:next and a doc quoting it must reproduce it verbatim. Two of those lines were reworded to say which side is the frontmatter key and which is the slash command, since they previously conflated the two. Verified the retired scan would now genuinely fail against this tree -- the conflict was real and resolved, not dodged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * chore(#2903): backfill changeset pr number Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> |
||
|
Tom Boucher
|
9640968f8e |
fix(#2847): require gap_closure value in plan-gap-closure schema and bind validate_plan to it (#3018)
* test(#2847): add failing-first regression tests for gap-closure frontmatter schema gap --gaps did not load a machine-checked requirement for gap_closure: true. The planner's only validation gate (frontmatter.validate --schema plan) never required it, and plan-phase.md's downstream_consumer contract never mentioned it either, so gap-closure plans could pass validation while missing the field that /gsd:execute-phase --gaps-only filters on. These tests are RED against current production code: no plan-gap-closure schema exists yet, and neither agents/gsd-planner.md's validate_plan step nor plan-phase.md's downstream_consumer block references gap_closure conditionally. * fix(#2847): enforce gap_closure via plan-gap-closure schema --gaps did not load a machine-checked requirement for gap_closure: true. The planner's only validation gate (frontmatter.validate --schema plan) never required it, so a gap-closure plan could pass validation while missing the field /gsd:execute-phase --gaps-only filters on, silently spawning zero executors. Add a plan-gap-closure schema (every plan-required field plus gap_closure) and make the planner's validate_plan step select it when gap_closure mode is active, plan otherwise. Standard/reviews-mode plans are unaffected: plan's required fields are unchanged. plan-phase.md's downstream_consumer block was investigated for a symmetric mention but deliberately left untouched: it sits 36 bytes under the frozen ADR-857 PRE_PHASE6 ceiling and the validate_plan step in gsd-planner.md is the actual call site, needing no help from plan-phase.md's prose. * fix(#2847): compact validate_plan edit under gsd-planner.md size caps Merging origin/next (7 commits, including #2775's gsd-planner.md STRIDE-row edit) left only 22 chars of headroom under four separate hard-coded 49152-char caps on gsd-planner.md (planner-decomposition, precondition-element, reversibility-tagging, security.test.cjs). The verbose validate_plan prose from the previous commit overran all four. Compact the edit to a single line (net +17 chars vs origin/next) while keeping the functional content: schema name, mode condition, and the unchanged base required-fields list. Also: - Fix a real bug in the fix-2847 negative-assertion test: plan-phase.md mentions the literal string "<downstream_consumer>" twice in backtick-quoted prose before the actual opening tag, so a plain indexOf() grabbed the wrong start position and swallowed ~10KB of unrelated content (including a "gap_closure" hit in a Mode: enum line), producing a false failure. Anchor on the tag starting its own line instead. - Merge the emitted-drift-ack fragment for gsd-planner.md with the #2775 fragment brought in by the merge (both named the same path; two ack sources may never name the same path) and correct its byte delta to the actual final number. * fix(#2847): drop stale merge-inherited emitted-drift-ack fragments Merging origin/next brought in three new emitted-drift-ack fragments (1700, 2658, 2775) relative to this branch's fork point. #2775 collided with my own gsd-planner.md key and was already consolidated. #1700 and #2658 don't collide, but none of their entries name a path this branch's actual diff touches (git diff --name-only origin/next...HEAD) — the ripples they explain are already baked into the current next baseline, so they explain nothing here and the emitted-attribution gate correctly reports them as stale (verified live: spike-wrap-up.md from #1700). Delete both fragment files. Neither is referenced by any test beyond a stray comment pointing at an unrelated diagnosis artifact path, not the ack fragment itself. * fix(#2847): restore merge-inherited ack fragments deleted in error 1700-spike-manifest-idea-scoping.json and 2658-trae-instruction-file-path.json exist on origin/next (landed via other, already-merged PRs) and arrived on this branch unchanged via the origin/next merge. The previous commit deleted them to satisfy a stale-acknowledgment finding, but the finding was about the acks being MODIFIED in this diff, not about needing to stop existing — deleting them would have silently reverted two other PRs' already-merged, already-justified byte growth. Restored byte-identical to origin/next (git diff origin/next -- <path> empty for both). 2775-planner-package-legitimacy-gate.json stays consolidated into 2847-gap-closure-validate-plan-step.json: that one was a genuine hard key-collision (two fragments naming the same gsd-planner.md path, which lint-emitted-drift-ack hard-blocks), not a pass-through case. * fix(#2847): bind --schema to gap_closure mode, not hardcode it Prior revision left the validate_plan bash invocation unconditional (--schema plan)) while only the prose sentence above it described the gap_closure-mode branch. An agent executing the shown line literally always validated with the plan schema, so a gap-closure plan missing gap_closure: true still reported valid:true — #2847 reproducing unchanged. Existing tests didn't catch it: they checked for substring presence anywhere in the step, which the prose alone satisfied. Change the bash line to --schema "$SCHEMA" — a real shell-variable reference in the same placeholder convention this file already uses for "$PLAN_PATH" (never literally assigned; the agent resolves it from context, same as PLAN_PATH). A genuine if/then bash conditional already exists elsewhere in this file (load_project_state's INIT @file: check), confirming executed conditionals, not merely descriptive prose, are the established pattern here. Rewrite the regression test to assert on the bash block's literal --schema argument: reject a hardcoded plan) or plan-gap-closure) literal, require a variable reference, and require the step's prose to bind that same variable name. Verified RED against the prior revision and GREEN against this one before committing either state. * fix(#2847): CRLF-safe tests, drop unexplained ack, require gap_closure=true Four items from independent review, all landing together per request: 1. The #2847 regression test file had two CRLF-fragile regexes (local/no-crlf-fragile-split): a bare \n on readFileSync content means a real \r\n checkout returns invocationLine === null and all four executable-content assertions stop asserting anything while still reporting green. Both now use \r?\n. Prior lint report of exit 0 was a false green from a stale eslint cache. 2. The 2847 drift-ack fragment explained nothing: a direct edit to agents/gsd-planner.md is self-explaining, drift-acks exist for emitted-artifact ripple that cannot be traced to a changed source path. Deleted. Restored the 2775 fragment byte-identical to next (git diff --name-status next...HEAD -- tests/emitted-drift-acks/ now prints nothing) — it only conflicted with the now-deleted 2847 fragment, never needed touching itself. 3. plan-gap-closure validated gap_closure by PRESENCE only (unchanged since the original #2847 fix), so gap_closure: false satisfied it — --gaps-only filters strictly on gap_closure === true, so a false-valued plan still validates green and still spawns zero executors: #2847's exact reported symptom, one value away. Added an optional requiredValues map to FRONTMATTER_SCHEMAS; plan-gap-closure now requires gap_closure to equal the string "true" (extractFrontmatter parses every scalar as a string) in addition to being present. Every other schema/field keeps the original presence-only contract. The row that had documented the hole instead of closing it now asserts the fix; a matching unit test locks requiredValues on FRONTMATTER_SCHEMAS. 4. The "names the plain plan schema" assertion matched the bare substring "plan" anywhere in the step, which verify.plan-structure satisfies incidentally a few lines below — the assertion could not fail even if the plain-plan branch were deleted from the prose. Changed to match the standalone backtick-quoted plan token. * fix(#2847): remove contradictory leftover assertion in Row 6 test The gap_closure:false test asserted !present.includes('gap_closure') (correct — matches the implementation's fold-wrong-value-into-missing semantics) immediately followed by a stale, unedited leftover from an earlier draft of the same test asserting the opposite: present.includes('gap_closure'). The second could never pass once the first did; both were in the same diff. Verified before committing: searched every consumer of frontmatter.validate output (agents/gsd-planner.md, docs/CLI-TOOLS.md, all other test files) for any read of the present field — none exist. Nothing depends on "present" meaning "physically exists regardless of value correctness", so the implementation's fold (present/missing stay a full partition of required) is the right call; the test needed to agree with it, not the other way around. Manually replayed all six rows in the plan-gap-closure describe block against the built CLI to confirm each now passes. * fix(#2847): prototype-key guard, wrong-value diagnostic, doc fixes, vacuous tests Six items from an independent SHIP_VERDICT:no review, landing together per request: 1. Prototype-key crash (src/frontmatter.cts): FRONTMATTER_SCHEMAS[schemaName] was an unguarded lookup, so --schema __proto__ (also constructor, toString, hasOwnProperty, valueOf) resolved to an Object.prototype member instead of undefined, the `!schema` check never fired, and the command crashed with an uncaught TypeError and a stack trace instead of "Unknown schema". Now reachable from prompt state (--schema is an agent-bound $SCHEMA), not just an unreachable literal. Guarded with Object.prototype.hasOwnProperty.call before the lookup, checked and rejected before assignment so `schema`'s type stays non-optional. Added a test for all five prototype keys. 2. Wrong-value diagnostic (src/frontmatter.cts, agents/gsd-planner.md): the strict gap_closure === "true" check from the previous fix was correct (fail-closed) but silent about WHY — a plan with gap_closure: True got "missing", indistinguishable from genuinely absent, even though the field is plainly in the file. Added an `invalidValue` field to the validate JSON (present but wrong-valued, disjoint from missing/present) and updated validate_plan's prose to state the exact required literal and explain invalidValue, within the remaining byte budget (49130/49152). 3. docs/reference/plan-md.md: fixed three inaccuracies in the gap_closure row — "this field plus every field above" implied `requirements` (documented Required: Yes) is schema-enforced, it is not; "Type: boolean" implied YAML True/TRUE/yes/1 are accepted, they are rejected (exact string match on literal lowercase true); "must never carry it" stated an unenforced rule as fact. Also switched /gsd:plan-phase and /gsd:execute-phase to the house-style hyphen form for docs/. 4. Vacuous negative assertions (tests/fix-2847-gap-closure-frontmatter.test.cjs): RegExp#test coerces a null invocationLine to the string "null", so both hardcoded-literal checks passed vacuously even if the step or its bash block were deleted entirely. Added a truthy precondition check first. 5. Deleted vacuous/pass-always tests: four in tests/frontmatter.unit.test.cjs strictly subsumed by (or, for the "superset" test, tautologically guaranteed by the same spread as) the deepEqual exact-list test; two describe blocks in the #2847 regression file that were already GREEN at the RED commit (5e5897cd2f17ebf2fc55757bae651bbbeb236289) and pinned untouched files rather than covering anything this change altered — one of them additionally forbade any future legitimate gap_closure mention in plan-phase.md, a trap for whoever frees up that file's byte budget later. 6. .changeset/clever-newts-wake.md: switched /gsd:plan-phase and /gsd:execute-phase to /gsd-plan-phase and /gsd-execute-phase — changesets render verbatim into CHANGELOG.md with no converter in the path, so the colon form would have reached readers naming a command no runtime registers. * chore(#2847): backfill changeset pr number (#3018) --------- Co-authored-by: sim <sim@local> |
||
|
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
|
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
|
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
|
1720aacf0c |
feat(#1949): <precondition> task element — Design by Contract (#2422)
* test(#1949): add failing-first tests for <precondition> element Red phase for issue #1949 (Design by Contract: <precondition> element asserted before task execution). Tests assert: - docs/reference/plan-md.md documents the new <precondition> element - agents/gsd-planner.md @-references planner-preconditions.md and stays under the 49152-char cap (progressive-disclosure requirement) - gsd-core/references/planner-preconditions.md exists and documents the three emission cases mandated by the issue (user_setup / prior-phase artifact / env-var) and the contract triad mapping - agents/gsd-executor.md asserts <precondition> before task execution and routes unmet preconditions through existing checkpoint machinery - cmdVerifyPlanStructure (behavioral via runGsdTools) accepts plans both with and without <precondition> — the additive-validation guarantee - Parity assertion: plan-md.md and planner-preconditions.md agree on the canonical tag spelling (DEFECT.GENERATIVE-FIX-DIVERGENCE guard) Most 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). * feat(#1949): <precondition> task element — Design by Contract Add an optional <precondition> element to <task> in PLAN.md (issue #1949, The Pragmatic Programmer Topic 23). The front-of-task side of the plan contract — preconditions (before) ↔ postconditions (<verify>/<done>/ <acceptance_criteria>, after) ↔ invariants (must_haves.truths, across the whole plan). Together with the tracer-bullet proposal (#1945), this closes both ends of the 'outrunning your headlights' failure mode for an autonomous AI executor. Acceptance criteria met: - <precondition> is an optional element on <task>; plans that omit it validate unchanged (cmdVerifyPlanStructure checks for presence of required tags, does not reject unknown optional tags). - gsd-executor evaluates the precondition before any other task work. Unmet halts execution with a checkpoint:human-verify and no partial commit; met or absent produces no visible change to execution flow. Unmet is never auto-approved under AUTO_CFG=true — a missing prerequisite is a fact the executor cannot establish on its own. - gsd-planner emits <precondition> in exactly the three cases the issue mandates: user_setup consumption, prior-phase artifact dependency, and env-var/runtime-config dependency. - Tests cover met, unmet, and absent preconditions plus the additive- validator guarantee. Files: - gsd-core/references/planner-preconditions.md (NEW): full emission rules, the three cases with worked examples, format guidance, anti-patterns, the contract triad mapping, and the executor assertion contract. Progressive disclosure. - agents/gsd-planner.md: slim <precondition> note in Task Anatomy with @-reference to the new file. To stay under the 49152-char agent-file cap (27-char headroom before this change), the inline <comment_text_discipline> and <region_scoped_negative_gate> summaries are compressed to one-line pointers — their full rules already live in planner-antipatterns.md, so no content is lost. - agents/gsd-executor.md: new step 0 'Precondition check' in the execute_tasks loop, before the type dispatch, routing unmet through checkpoint_return_format. - docs/reference/plan-md.md: new Preconditions section in the schema reference, with the canonical example and the three emission cases. - CONTEXT.md: Precondition glossary entry as a sibling of Tracer Bullet. - docs/INVENTORY.md + INVENTORY-MANIFEST.json: row for the new references/planner-preconditions.md (regen via gen-inventory-manifest). - tests/precondition-element.test.cjs: failing-first tests covering schema docs, planner emission contract, executor assertion contract, reference-file presence + the three cases, behavioral additive- validator guarantee, and a parity assertion (DEFECT.GENERATIVE-FIX- DIVERGENCE guard). - .changeset/quick-hawks-bark.md: Added fragment. Companion to #1945 (tracer bullets). * chore(#1949): regen agent-size baseline + install-tree goldens Documented baseline regenerations required by the feat(#1949) prose changes (RULESET.AGENT_SIZE_BUDGET + golden-install-parity): - npm run size:baseline — locks in the new gsd-executor.md size (+1050 bytes: the precondition-check step 0 block). gsd-planner.md is net smaller (-142 bytes: compressed two inline summary blocks whose full rules already lived in planner-antipatterns.md to make room for the slim <precondition> pointer). No hard-cap breach. - npm run gen:golden — pick up the new references/planner-preconditions.md + the two changed agent files across all 18 runtime install trees. Both regens are CI-mandated after intentional agent/reference changes; see CLAUDE.md 'RULESET.AGENT_SIZE_BUDGET' and the comments in tests/golden-install-parity.test.cjs. * fix(#1949): bound <precondition> checks to read-only (security review) Apply the security-review finding (LOW, isolated /security-review subagent): the executor's 'run the cheapest check' phrasing for a plan-author-controlled prose line was broader than ideal — a hostile plan author could craft a <precondition> whose 'cheapest check' is side-effecting (curl to an attacker host under the guise of verification, rm -rf before checking, secret emission). The risk is inherited from GSD's existing plan-trust model (<verify>, <action>, <done> already direct the executor to run arbitrary shell), so <precondition> does not materially expand it. But the new prose actively directs execution ('run the check') rather than passively consuming the element, so the bound is worth making explicit. Tightened across all four surfaces that describe the check shape: - agents/gsd-executor.md step 0: 'Verify with read-only checks only — file existence, env var presence (no value output), idempotent GET /health-style pings. Do NOT run commands with side effects (writes, network POSTs, secret emission) as the check; if a side-effecting check seems required, halt and surface via checkpoint instead.' - gsd-core/references/planner-preconditions.md Format section: same bound, plus the halt-and-surface escape hatch. - docs/reference/plan-md.md Preconditions section: mirrored. - CONTEXT.md Precondition glossary entry: mirrored. Regenerated agent-size baseline (executor grew 46186 -> 46440; still under the 49152 cap) and install-tree goldens. * chore(#1949): backfill changeset pr number 2422 Per CONTRIBUTING.md changeset workflow + feature-builder directive Step 8.7: backfill the placeholder pr:0 with the real PR number immediately after gh pr create returns. Avoids the fail_invalid_fragment gate. * fix(#1949): cite [#1949] on allow-test-rule exemption (ADR-456) CI's lint:ci runs lint-allow-test-rule-refs which per ADR-456 requires every // allow-test-rule: exemption on a NEW test file to carry an issue reference (#NNN or URL). My earlier push omitted it. Local 'npm run lint' (eslint) does NOT run this check — only 'npm run lint:ci' does. CLAUDE.md explicitly warns: 'lint:ci ≠ lint — CI runs lint:ci; a local pass is not the gate.' I should have run lint:ci before pushing; correcting now. Pattern matches the companion feature's test file: tests/tracer-bullet.test.cjs:1 // allow-test-rule: source-text-is-the-product [#1945] |
||
|
Tom Boucher
|
13d181aedf |
fix(#2349): exclude status: superseded plans from phase completion counts (#2404)
Adds a status: superseded plan-frontmatter marker that scanPhasePlans excludes from both plan and summary counts, so a phase with a deliberately-unexecuted plan no longer reads incomplete forever (the plan-level analogue of #1514). Includes all-superseded completion handling and a bounded, symlink-safe frontmatter read. Fixes #2349. |
||
|
Tom Boucher
|
315d94f6d4 |
feat(#1945): tracer-first planning default + executor feedback gate (#2294)
* feat(#1945): tracer-first planning default + executor feedback gate Make "thin end-to-end slice first, verify, then expand" the default planning + execution discipline instead of the opt-in --mvp mode. - gsd-planner: first-class `type="tracer"` task; every plan LEADS with one production-quality end-to-end tracer slice by default; --no-tracer restores horizontal layers; --mvp/--tdd compose on top. - gsd-executor + execute-plan: post-tracer feedback gate — autonomous runs halt-on-fail before expansion, interactive runs emit checkpoint:human-verify after the tracer. - --no-tracer flag wired through plan-phase workflow/command/help/skill. - CONTEXT.md glossary defines tracer bullet vs prototype; docs + references reconciled. - tests/tracer-bullet.test.cjs: prose-contract + behavioral (verify plan-structure accepts tracer) coverage. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#1945): backfill changeset PR number to 2294 --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
ffd353f080 |
docs(#2240): clarify PLAN.md <execution_context> is install-relative, record #2238 wontfix (#2241)
The <execution_context> reference presented @~/.claude/gsd-core/... as canonical and described the paths as files "the executor reads before starting". Both are misleading: the prefix is install- and runtime-relative (Claude global vs Cursor .cursor/gsd-core vs an absolute --local path), so a committed plan is not clone-portable, and /gsd-execute-phase loads the workflow from its own installed copy rather than gating on the committed block. - docs/reference/plan-md.md: describe the install-relative, non-clone-portable nature of the block and contrast it with repository-relative <context>. - .out-of-scope/plan-md-execution-context-portability.md: record the #2238 wontfix decision (PLAN.md is a machine artifact; #2158 precedent) with a revisit-if condition. Refs #2238. Closes #2240. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> |
||
|
Tom Boucher
|
2d264da661 |
enhance(#968): region-scoped negative-grep idiom + cross-task conflict warning (#1320)
Adds a region/function-scoped negative-grep idiom to the gsd-planner verification guidance plus a warn-only `validate_plan` check (`scanFileWideNegativeGateConflict`) that flags when a task's file-wide negative grep bans a construct a sibling task legitimately requires elsewhere in the same file. ReDoS-safe (linear, no RegExp on author patterns); region-scoped gates are exempt. Warn-only — never errors, never flips `valid`. Closes #968 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
972a41a528 |
fix(#967): make verify key-links docs author-strict (from:/to: are file paths; symbols go in via:) (#990)
* fix(#967): make verify key-links docs author-strict (from:/to: are file paths; symbols go in via:) Closes #967 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * chore(#967): backfill changeset pr number (990) --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com> |
||
|
Tom Boucher
|
463cffd894 |
chore(#604): rename get-shit-done/ runtime directory to gsd-core/ (#615)
* chore(#604): rename get-shit-done/ runtime directory to gsd-core/ Renames the installed runtime directory `get-shit-done/` to `gsd-core/` so the on-disk name matches the package (`@opengsd/gsd-core`), repo, and binary (`gsd-tools`). The npm package name and binary are unchanged; npx/npm consumers are unaffected. Mechanical (bulk, ~90% of the diff): - `git mv get-shit-done gsd-core` - Swept path/identifier references across the repo via `perl -pe 's/get-shit-done(?!-\w)/gsd-core/g'`. The negative lookahead preserves the five legitimate slug variants that are NOT the directory: get-shit-done-{OLD,cc,classic,cli,redux} (old package/repo names). - Build/manifest wiring: package.json (bin, files, coverage globs), tsconfig.build.json (outDir), ~86 .gitignore build-output entries, stryker.config.mjs, scan-ignore files, install.js path strings. - Frozen (not rewritten): CHANGELOG.md history; translated docs (README.<locale>.md and docs/{ja-JP,ko-KR,pt-BR,zh-CN}/). New logic (review here): - src/installer-migrations/003-rename-get-shit-done-to-gsd-core.cts: a proper ADR-0008 installer migration. On upgrade it walks the legacy `~/.claude/get-shit-done/` tree, classifies each file via the prior install manifest, and emits remove-managed / backup-and-remove for managed files while PRESERVING unknown user-added files. Symlink-safe (skips a symlinked root and symlinked entries; bounds-checks every path under configDir). The framework rolls back on install failure. Emptied dirs may remain (framework has no recursive dir-removal primitive) — documented. - scripts/lint-legacy-dir-name.cjs: CI regression guard forbidding the bare `get-shit-done` directory token (split token to avoid self-match; case- insensitive; `(?!-\w)` lookahead allows the slug variants; allowlists CHANGELOG, translated docs, and `gsd-allow-legacy-name` marker lines). Wired into the lint-tests CI job. - Restored scripts/lint-package-identity-drift.cjs detection regexes (the mechanical sweep had wrongly rewritten the old-name patterns it exists to detect) and marked them as intentional legacy references. - TDD tests for the migration and the guard; do.md slash-command guard regex tightened so a `/gsd-core/bin` path segment is not mistaken for a command; changeset + docs/installer-migrations.md row added. Breaking: the installed runtime path moves `~/.claude/get-shit-done/` -> `~/.claude/gsd-core/`. Migration 003 removes the stale legacy dir's managed files (preserving user files) on upgrade. Users with custom hooks/configs hardcoding the old path must update them. Closes #604 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): unsweep pending changesets + allowlist injection-example docs CI fixes for the rename PR: - Do not sweep pending .changeset/*.md (ephemeral release-note fragments, like CHANGELOG); reverted those body edits so 5 pre-existing malformed fragments (missing type/pr) no longer enter the PR diff and trip docs-lint. Allowlisted .changeset/ in the legacy-name guard accordingly. - Allowlisted TEST-EXAMPLES.md and docs/explanation/security-model.md in prompt-injection-scan.sh: they contain intentional injection examples / security-model prose; the path-reference rewrites are kept. CodeQL alerts on this PR are pre-existing (alert lines unchanged by this PR; none in the new migration/guard) and are out of scope for the rename. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): resolve CodeQL alerts surfaced on this PR The rename diff touched files carrying pre-existing CodeQL findings; per the no-pre-existing-dismissal rule, fixing every surfaced alert rather than waving them off. All behavior-preserving: - scripts/ci-test-scope.cjs: build the config-path match from string .includes() instead of a RegExp over an arg-derived value (js/regex-injection). - src/profile-output.cts: escape backslashes before pipe-escaping desc/safeName so the table-cell escape is complete (js/incomplete-sanitization). - tests/{bug-2643,bug-2808,docs-parity-live-registry}: two-pass HTML-comment strip so a bare/unclosed `<!--` cannot survive (js/incomplete-multi-character-sanitization). - tests/inline-plan-threshold: drop the no-op `\s`->`\s` identity replace, keep the meaningful POSIX-class conversion (js/identity-replacement). Verified: build:lib green; the touched test files + ci-test-scope + profile-output suites pass; lint:legacy-name clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): correctly resolve remaining CodeQL alerts (regex-injection + sanitization) The prior commit's fixes for two alerts were ineffective: - ci-test-scope.cjs js/regex-injection: the alert is the CLI-arg-derived `file` reaching static regex `.test(file)` calls (not the config rule). Removed ALL regex over file/t — startsWith/includes/=== string checks + an isWindowsHint helper — so there is no regex sink for the tainted value. - js/incomplete-multi-character-sanitization (3 test files): a single `.replace(/<!--...-->/g,'')` can let `<!--` re-form. Replaced with a fixpoint loop (replace until stable) plus a final bare-opener strip. Verified: no regex over file/t remains; ci-test-scope + the 3 test suites pass; lint:legacy-name clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): make ci-test-scope + comment-strippers regex-free to clear CodeQL CodeQL flags the regex PATTERNS syntactically (regex-injection on the --files arg split; incomplete-multi-character-sanitization on the <!--...--> replace), so loop fixes do not satisfy it. Made these paths regex-free: - ci-test-scope.cjs splitFiles: char-by-char separator tokenizer (no /[,\\s]+/). - 3 test files: indexOf/slice HTML-comment stripper (no .replace(/<!--/)). Behavior preserved; ci-test-scope + the 3 suites pass; guard clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): unblock security base64 scan on the large rename diff The security job hit its 10m timeout: base64-scan.sh choked on the binary test fixture tests/feat-3594-parser-property-style.test.cjs (embedded NUL/ non-UTF8 bytes -> thousands of bogus blobs + "ignored null byte" warnings), and the ~800-file rename diff is slow to scan regardless. - scripts/base64-scan.sh: skip binary-by-content files (grep -Iq .) — they can't carry base64-obfuscated *text* and feeding NUL bytes through the per-line scanner is pathologically slow. collect_files already filtered binary *extensions*; this catches binary *content* in text extensions. - .github/workflows/security-scan.yml: raise the security job timeout 10m->30m to accommodate very large diffs (the scan itself is unchanged). Verified locally: scan skips the fixture, 0 "ignored null byte" warnings, 0 findings, exit 0. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): sweep get-shit-done refs introduced by merging next The branch was updated with next (#614/#384/#618 etc.), which reference the get-shit-done/ dir (still named that on next). Swept the stale references in the merged files to gsd-core so the rename stays consistent and lint:legacy-name passes: - commands/gsd/discuss-phase.md (runtime-launcher shim paths) - src/core.cts (getAgentsDir layout comments) - tests/bug-384-agents-runtime-aware.test.cjs (require path to runtime lib) Verified: guard 0 violations; build green. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): exclude gsd-core/ path segments from bug-3683 command cross-ref invariant The #614 runtime-launcher shim added to discuss-phase.md references `${_GSD_RUNTIME_ROOT}/gsd-core/bin/...`. bug-3683's REF_PATTERN excluded path-y refs only via lookbehind, but `}` precedes `/gsd-core/` in the shim, so it mis-read the directory path as a dangling `/gsd-core` command ref (same class as the #604 bug-2954 fix). Added a trailing `(?![\w-]*\/)` so `/gsd-<x>/...` path segments are not treated as slash-command references. Verified locally on BOTH platforms before pushing: - mac (node 26) full suite: 0 failures - gsd-test-runner (linux, node22 image) full suite: 0 failures - bug-3683 + bug-2954 pass. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): lazily resolve findProjectRoot in gsd-tools (harden flaky CI) CI intermittently failed state.test's gsd-tools subprocess with "findProjectRoot is not a function" (flip-flopping across legs; not reproducible on mac full suite, gsd-test linux full suite, test:unit, or state.test x8). findProjectRoot is a re-export from core.cjs (sourced from project-root.cjs); binding it via destructure at module-load can be undefined under a load-ordering edge. Resolve it lazily at call time via a small wrapper so the lookup happens after core.cjs is fully initialized. Verified green on BOTH platforms before pushing: - mac (node 26) full suite: 0 failures - gsd-test-runner (linux, node22) full suite: 0 failures - state.test.cjs: 106/106; gsd-tools loads cleanly. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(#604): allowlist verification-patterns.md placeholder examples in secret scan The rename git-mv'd references/verification-patterns.md into gsd-core/, pulling it into the secret-scan diff. It documents stub/placeholder RED-FLAG env-var examples (illustrative Stripe test-key / database-URL / API-key placeholders) — not real credentials. Added it to .secretscanignore with the strict annotation, mirroring the existing gsd-core/workflows/plan-phase.md exception. Verified locally: secret-scan-lint --strict OK; secret-scan --diff origin/next exits 0 with 0 findings. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> |
||
|
Tom Boucher
|
3bb2f8f1c5 |
docs: rebrand to GSD Core and restructure docs with Diataxis (#605)
* chore: wire docs/agents config into AGENTS.md Agent skills section
Add the `## Agent skills` discovery block pointing the engineering
skills at the existing docs/agents/{issue-tracker,triage-labels,domain}.md
files (issue tracker, triage label mapping, single-context domain docs).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: rebrand to GSD Core and restructure docs with Diataxis
Reorganise the root README and docs/ around the Diataxis framework
(tutorials, how-to guides, reference, explanation), add new how-to
guides and schema references (STATE.md / CONTEXT.md / PLAN.md /
planning artifacts), and cross-link the whole set. Update the lone
legacy gsd-build reference to open-gsd; keep internal get-shit-done/
filesystem paths unchanged (directory rename tracked separately in
open-gsd/gsd-core#604). Regenerate the ja-JP, ko-KR, pt-BR and zh-CN
localised trees to mirror the new structure.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
* docs: backfill changeset PR number (#605)
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---------
Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
|