22 Commits

Author	SHA1	Message	Date
Jakub Zych	6cfa0c55d2	refactor: drop 12 runtimes, keep Claude, Codex, OpenCode, Cursor, ZCode, Antigravity ... Removes kilo, kimi, kimi-code, copilot, windsurf, augment, trae, qwen, hermes, cline, codebuddy and pi end to end: capability descriptors, installer branches and converters (bin/install.js 14.9k -> 11.2k lines), TypeScript converters, hook surfaces and runtime homes, review lanes qwen/kimi-code, the two pi migrations, Kimi payload normalization in the hook guards, dead hostBehaviors vocabulary, launcher home probes, fixtures, runtime-specific tests and the prose that presented them as supported. Installer output for the six kept runtimes is byte-identical to before the prune. The Kimi tool-vocabulary tests in workflow-guard, read-guard and read-injection-scanner are left in place pending a decision.	2026-10-06 20:02:40 +02:00
Jakub Zych	a9a7a328e6	refactor: hard-fork GSD -> MSD (Make Software Done) ... Mechanical rename produced by scripts/msd-rename.cjs: gsd/Gsd/GSD -> msd/Msd/MSD across contents and paths, upstream package/repo coordinates -> @golem15/msd-core and golem15com/msd-core. Deep links into upstream history, sibling upstream packages, the GSD-2 import feature, CHANGELOG.md and .changeset/ are kept as-is. Hand edits on top: MSD block-letter banner and logos, LICENSE copyright line, package/plugin identity, regenerated lockfile, install-tree fixtures, derived registries and benchmark baseline; migration checksum baseline re-locked (MSD keeps its own install state, so no install had applied the old sums); sort-order and regex-escaped expectations in tests adjusted.	2026-10-06 01:47:40 +02:00
Dennis Alexis Valin Dittrich	18c899def5	enhance(#4209): optional external source reviewer lanes for /gsd:code-review (#4323) ... * test(01-01): define reviewer-support trait contract Add failing coverage for step.supportsReviewerLanes (#4209 DISP-02): validator rejects non-boolean values with an exact field path, accepts missing/true/false, and the real code-review capability.json steps must declare supportsReviewerLanes: true. Add loop-resolver projection coverage proving the trait reaches activeHooks verbatim for a provider-neutral synthetic step (not code-review-specific), and that omitted/false values stay inert (no key on the active hook). All 8 new assertions fail today: the validator has no such field, and loop-resolver has nothing to project. RED before GREEN. * feat(01-01): declare reviewer-capable steps Add step.supportsReviewerLanes (#4209 DISP-02): a strict optional boolean opt-in trait, step-scoped (not capability-wide). Only a literal true validates and projects; false/omitted stay inert (no key on the projected active hook), and every non-boolean type fails capability-validator.cjs with an exact field-path error. Opt both existing code-review steps (execute:post, execute:wave:post) into the trait in capabilities/code-review/capability.json. Project the validated field through src/loop-resolver.cts into activeHooks so a provider-neutral generic interpreter can read it without any code-review-specific knowledge. Document the field in docs/reference/capability-manifest.md and regenerate gsd-core/bin/lib/capability-registry.cjs via the generator (never hand-edited). Makes all 8 RED assertions from the prior commit pass. * test(01-02): define shared reviewer dispatch - Add tests/reviewer-step-dispatch.test.cjs covering dispatchReviewerLanes: inert when the supportsReviewerLanes trait is off or nothing is selected, exactly-once plan/invoke per selected lane, duplicate-alias dedup, the bounded metadata-only source-review prompt (repo root, paths+baseSha, depth, four fixed prohibitions), and capability-neutral reuse via a second synthetic step context. - RED: module under test (src/reviewer-step-dispatch.cts) does not exist yet, so require() fails and every assertion is unreached. * feat(01-02): dispatch reviewers for opted-in steps - Add src/reviewer-step-dispatch.cts: dispatchReviewerLanes(input, deps), ONE interpreter for a step's supportsReviewerLanes trait. Reuses resolveReviewerSelection for selection and resolveLanePlan for planning (both already-existing, pure building blocks); invocation is the one required, caller-injected seam (deps.invoke) since runLane needs OS-aware spawn plumbing this module does not own. - trait !== true, or a selection resolving to zero lanes, dispatches nothing (zero plan/invoke calls). Each selected lane is planned and invoked exactly once, in the selector's deduped/sorted order. - buildSourceReviewPrompt assembles a metadata-only bounded prompt (repo root, canonical paths + base SHA, depth, four fixed prohibitions) — never file contents — written once per dispatch and shared across every invoked lane. - GREEN: tests/reviewer-step-dispatch.test.cjs now passes. * test(01-02): define reviewer dispatch failures - Extend tests/reviewer-step-dispatch.test.cjs with the fail-closed matrix: an explicitly requested lane the selector could not resolve still lets the OTHER resolved lane run, but the aggregate result must never read as a clean success (and 'every explicit lane unavailable' must be distinguishable from the plain no-flags-passed inert case); request-level validation (path traversal, absolute paths outside repoRoot, empty/non-string paths, missing depth/base SHA) halts the whole dispatch before any lane is planned or invoked; a per-lane prompt-budget overflow hard-fails only that lane before invoke while its sibling still runs. - RED: src/reviewer-step-dispatch.cts does not yet implement any of these guards, so 9 of the new assertions fail against the current (Task 1) implementation. * fix(01-02): fail closed in reviewer dispatch - src/reviewer-step-dispatch.cts: add the fail-closed guards the prior commit deliberately left out. An explicitly requested lane the selector could not resolve no longer lets the aggregate read as a clean success — lanes that DID resolve still run and keep their results (never narrow the requested set), but selection.errors now flips the aggregate ok to false, and 'every explicit lane unavailable' is now distinguishable (SELECTION_FAILED) from the plain no-flags-passed inert case (NO_LANES_SELECTED). - Add request-level validation (validatePaths, depth/baseSha presence) that halts the WHOLE dispatch before any lane is planned or invoked: path traversal, absolute paths outside repoRoot, empty/non-string paths, and missing provenance are all rejected up front. - Add per-lane prompt-budget enforcement (resolveBudget, mirroring gsd-tools.cjs's budgetFor convention including budget 0 = unbounded): a lane whose resolved budget the prompt exceeds hard-fails before invoke runs for it, without cancelling a sibling lane already planned. - Document the supportsReviewerLanes trait and its dispatch-step interpreter in gsd-core/references/loop-hook-dispatch.md. - GREEN: all 19 tests in tests/reviewer-step-dispatch.test.cjs pass; no regressions in the review-lane/reviewer-selection/prompt-budget suites (356 passing). * test(01-03): define optional source reviewer flow RED: assert code-review.md dispatches roster-derived reviewer-lane flags through a single review-lane dispatch-step call (DISP-01..05), that the no-flag path stays byte-for-behavior unchanged (COMP-01), and that external evidence reaching the internal reviewer prompt is marked unverified (CONS-02). Also covers the CLI contract directly: no-op with no explicit selection, and fail-closed on an explicit unknown lane (SAFE-07) via real gsd-tools.cjs subprocess calls. * feat(01-03): route optional source reviewers GREEN: code-review.md gains a dispatch_reviewer_lanes step that matches canonical reviewer-lane flags against the merged first-party + installed roster (never a hand-maintained list) and, only when at least one is present, calls the shared reviewer-step interpreter exactly once with the already-resolved repo root, file scope, depth, and base SHA. Its evidence paths are appended to the internal reviewer prompt via ${EXTERNAL_EVIDENCE_BLOCK}, explicitly marked unverified. No reviewer-lane flag leaves the internal-only dispatch byte-for-behavior unchanged (COMP-01). Deviation (Rule 3 — blocking issue): 01-02 documented `review-lane dispatch-step` (gsd-core/references/loop-hook-dispatch.md) as the CLI route `dispatchReviewerLanes` wires through, but never implemented the gsd-tools.cjs subcommand — the workflow's call had nothing to reach. Add it to the existing review-lane router, reusing the same effort-aware plan building and runner deps `plan`/`invoke` already use (factored into buildLaneRunnerDeps to avoid duplicating the spawn/http/fs seam). Guard the CLI's own `detected` set on whether an explicit flag was passed: resolveReviewerSelection's no-explicit-selection fallback is "select every detected reviewer" (the correct default for /gsd:review), and passing it an unconditionally non-empty detected set would silently invoke the whole roster on every no-flag code review, violating COMP-01. * test(01-03): define external finding consolidation RED: assert gsd-code-reviewer.md treats <external_reviewer_evidence> as untrusted input — independently re-verifies every claim against the actual current source, resists a prompt-injection attempt embedded in evidence text, and folds a verified claim into the existing Narrative Findings section with no second REVIEW.md schema (CONS-01..03). Also assert code-review.md's EXTERNAL_EVIDENCE_BLOCK restates the four fixed source-review prohibitions (SAFE-03..06) at the internal-reviewer handoff. * feat(01-03): consolidate external review evidence GREEN: gsd-code-reviewer.md's load_context parses <external_reviewer_evidence> as untrusted data, independently re-verifies every cited claim against the actual current source before it can appear in REVIEW.md, and explicitly resists prompt injection embedded in evidence text (never a command, no matter what it claims to be). A verified claim folds into the existing Narrative Findings section with (external: {slug}) provenance — one REVIEW.md schema only, no separate external-findings section. code-review.md's EXTERNAL_EVIDENCE_BLOCK now restates the four fixed source-review prohibitions (SAFE-03..06) at the internal-reviewer handoff. * fix(01-02): gitignore the reviewer-step-dispatch build artifact 01-02 added src/reviewer-step-dispatch.cts but never added its npm run build:lib output to .gitignore, unlike every sibling gsd-core/bin/lib/*.cjs generated file. Left it showing as untracked noise in git status. * docs(01-04): publish user and command contract for reviewer-lane source review - Document optional reviewer-lane flags on /gsd-code-review in USER-GUIDE.md and COMMANDS.md: opt-in, no source bodies in prompts, no fallback on failure, findings independently consolidated into the single REVIEW.md - Add the same contract to the docs/features/code-review-pipeline.md fragment and regenerate docs/FEATURES.md from it - Preserve /gsd-review as the plan-review command; cross-reference it rather than duplicating the reviewer roster - Pick up docs/INVENTORY-MANIFEST.json and skills/gsd-code-review/SKILL.md drift owned by source already shipped in Plans 01-01/01-03 but never regenerated (npm run regen:derived had not been run in this worktree) * docs(01-04): align architecture and agent ownership docs for reviewer-lane trait - ARCHITECTURE.md: trace the #4209 capability trait (supportsReviewerLanes) through the shared dispatchReviewerLanes interpreter to the existing review-lane plan/invoke machinery, ending at gsd-code-reviewer as the sole REVIEW.md consolidator - AGENTS.md: document gsd-code-reviewer's full-context verification scope and its treatment of external reviewer evidence as unverified input - No new diagram, abstraction, or config key; docs/CONFIGURATION.md is unchanged since the feature adds no setting or default * fix(01-02): eslint-ignore the reviewer-step-dispatch build artifact Same gap as the earlier .gitignore fix: 01-02 added src/reviewer-step-dispatch.cts but never added its generated gsd-core/bin/lib/reviewer-step-dispatch.cjs output to eslint.config.mjs's ignore list like every sibling generated file, so tsc's emitted __importDefault CommonJS-interop var tripped no-var. * fix(01-04): add the reviewer-step-dispatch.cjs roster row to docs/INVENTORY.md 01-04 regenerated docs/INVENTORY-MANIFEST.json (which now lists cli_modules/reviewer-step-dispatch.cjs) but the hand-written roster row in docs/INVENTORY.md — required by design, since a role sentence cannot be generated — was never added. * fix(01-01): update the code-review capability-step fixture for supportsReviewerLanes refactor-trigger-cli.test.cjs's preservesCodeReviewHookShapeAlongsideRefactorHook strict-deep-equals the code-review step's exact shape at execute:post; 01-01 added supportsReviewerLanes: true to that step and this fixture was not updated. * chore(01-03): acknowledge emitted-doc growth for code-review.md and gsd-code-reviewer.md Both files grew as a direct, intended consequence of wiring optional reviewer lanes into /gsd:code-review (the new dispatch_reviewer_lanes step and the untrusted-evidence consolidation contract) — not incidental drift. Emitted-Drift-Ack-Growth: code-review.md — new dispatch_reviewer_lanes step and EXTERNAL_EVIDENCE_BLOCK wiring for optional reviewer lanes (#4209) Emitted-Drift-Ack-Growth: gsd-code-reviewer.md — untrusted external-evidence consolidation contract for optional reviewer lanes (#4209) * test(01-05): define WR-01/WR-02 reliability contract for dispatchReviewerLanes From internal code review: dispatched must be false when zero lanes actually reached plan(), and a throwing plan()/invoke() for one lane must not discard results already collected for a sibling lane — matching the fail-closed pattern gsd-tools.cjs already uses for the same resolveLanePlan call (#2494/#2605/#1698/#1936/#2073/#2176/#2589/#2794). Refs: gsd-core-dks.16, gsd-core-dks.17 * fix(01-05): close WR-01/WR-02/IN-01/IN-02 from internal review - WR-01: dispatched now tracks whether any lane actually reached plan(), not results.length — an unresolvable selected slug no longer reports dispatched:true. - WR-02: plan()/writePromptFile()/invoke() wrapped per-lane so a throw for one lane can never discard results already collected for a sibling lane, matching the same guard gsd-tools.cjs already has around the identical resolveLanePlan call. - IN-01: documents the intentional budget===0-is-unbounded convention (#2797) the caller already relies on. - IN-02: review-lane dispatch-step no longer blocks indefinitely on an un-piped interactive TTY; fails closed to empty paths instead. Refs: gsd-core-dks.16, gsd-core-dks.17 * docs(01-05): add changeset fragment for PR #17 * fix(01-03): allowlist prompt-injection-scan false positive on the untrusted-evidence contract agents/gsd-code-reviewer.md's untrusted-evidence section and its pinning regression test both quote injection phrases as the exact attack they defend against/detect — same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class as the existing allowlist entries, not an actual injection vector. * test(01-05): extend WR-02 coverage to writePromptFile/invoke throws; DIFF_BASE-empty skip From CodeRabbit review: WR-02's earlier fix only wrapped plan() — writePromptFile()/deps.invoke() still ran unguarded, so a throw there still aborted every later selected lane. Also covers the dispatch_reviewer_lanes DIFF_BASE-empty-provenance gap (explicit lanes silently not running when no prior review and no phase-start commit exist). * fix(01-05): skip dispatch_reviewer_lanes with a clear warning when DIFF_BASE cannot be resolved Previously an explicit reviewer-lane request with no prior review and no resolvable phase-start commit reached dispatch-step with an empty --base-sha, which fails closed via missing_provenance — correct, but silent about why explicitly requested lanes didn't run. Now skip dispatch entirely in that case with a stderr warning naming the actual cause. * fix(01-05): wrap writePromptFile/invoke in the same per-lane try/catch as plan() WR-02's original fix only guarded plan() — a throw from writePromptFile() or deps.invoke() still aborted the whole dispatch, discarding results already collected for lanes processed earlier in the loop. CodeRabbit caught the gap; WR-02b/WR-02c pin it. * fix(01-05): WR-02b mock must throw only on the first writePromptFile() call The committed mock threw unconditionally, so codex's retry also threw and failed for the same reason as claude's — the test could not distinguish 'sibling still runs' from 'sibling also breaks'. Gate the throw to the first call, matching WR-02/WR-02c's single-failure intent. * fix(#4209): close review findings from adversarial + critical-code-reviewer pass Two independent reviews (agy adversarial review, Opus critical-code-reviewer + ponytail) found 6 Blocking and 7 Required issues in the reviewer-lane dispatch wiring around dispatchReviewerLanes. All 13 tracked in gsd-core-dks.18-30 and fixed here: - dispatch-step's reducer silently swallowed whole-dispatch rejections (invalid paths, missing provenance, etc); it now checks parsed.ok/reason. - spawn_reviewer recomputed its own stale DIFF_BASE, diverging from the LAST_REVIEW_COMMIT-aware value dispatch_reviewer_lanes uses on re-review; now shares the single compute_file_scope derivation. - the external reviewer prompt had no actual review request or citation requirement, only prohibitions; added both. - removed the supportsReviewerLanes trait plumbing (capability registry, validator, loop-resolver, docs, tests) — it was never consulted by the real dispatch path, which gates on explicit CLI flags instead. - flag-resolution require() was a fragile cwd-relative literal that failed silently on non-vendored installs; now resolves via GSD_TOOLS's own directory and warns instead of swallowing failure. - reducer didn't unwrap the @file: overflow protocol for large payloads. - deduplicated resolveBudget/budgetFor into one resolveLaneBudget. - lane artifacts now write to a mktemp run dir instead of $PHASE_DIR, so a second dispatch can't overwrite prior evidence. - validatePaths rejects control characters, closing a markdown-injection vector into the external prompt via crafted filenames. - reworded the one line that tripped prompt-injection-scan.sh instead of allowlisting the whole production prompt file. - fixed a stale docstring range and a dispatched-field ordering bug. - added 3 integration tests executing the actual reducer against synthetic dispatch-step JSON, replacing markdown-substring-only assertions. 771/771 tests pass across every touched suite; tsc --noEmit clean. * fix(#4209): wire supportsReviewerLanes as the maintainer's required reusable trait The maintainer's approval on issue #4209 explicitly redirected implementation shape: reviewer-lane dispatch must be a reusable capability/step-dispatch trait ("supportsReviewerLanes"), not code-review.md hand-wiring the call itself. My previous commit (e2558326) deleted that trait entirely after finding it declared-but-never-consulted, which was backwards — the fix was to wire it, not remove it. Restores the trait (capability.json, generated registry, validator, loop-resolver.cts, docs, tests) and wires it for real: dispatch_reviewer_lanes now resolves its own active hook via `gsd_run loop render-hooks` for the configured workflow.code_review_point and only proceeds to CLI-flag matching when supportsReviewerLanes reads true. Explicit flags no longer bypass the trait; a matching flag with the trait false resolves zero slugs (proven by a new integration test executing the real fence with both trait states). Emitted-Drift-Ack-Growth: gsd-core/workflows/code-review.md — the dispatch_reviewer_lanes step grows a trait-resolution fence (#4209 maintainer redirect requires the capability layer, not the workflow, own the opt-in decision). * fix(#4209): dispatch-step self-verifies the reviewer-lane trait via --cap-id/--point Both an agy adversarial review and an Opus critical-code-reviewer pass independently found the same gap in my previous commit (9b2c3773d): the trait check I wired into code-review.md only protected code-review's OWN invocation — gsd-tools.cjs's dispatch-step handler still hardcoded `trait: true` unconditionally, so a second capability declaring supportsReviewerLanes would get zero enforcement from the shared CLI unless it correctly re-implemented the ~15-line render-hooks scrape itself. That is exactly the "each workflow.md hand-wiring the call" the maintainer's redirect said to eliminate. Moves the trait check into dispatch-step itself: given --cap-id/--point, it self-invokes `loop render-hooks <point>` (relocating the one subprocess code-review.md used to spawn for this, not adding a new one) and derives the real trait from that capId's active hook, rather than trusting a caller-passed boolean. code-review.md now only passes --cap-id code-review --point "$CODE_REVIEW_POINT" and no longer resolves or gates on the trait itself — the ~20-line scrape it previously carried is gone. Any other capability opts into the identical enforcement by declaring the trait and passing the same two flags. Replaced the two tests that stipulated SUPPORTS_REVIEWER_LANES as an input variable (they proved a bash branch honors a variable, not that the variable reflects the real capability manifest) with three integration tests that invoke the real dispatch-step CLI against the real first-party capability registry: the real code-review trait resolves true, an unknown --cap-id resolves false (trait_not_enabled, fail-closed), and omitting --cap-id/--point entirely resolves false (no context means no opt-in). Also: reject \x7f/U+2028/U+2029 in validatePaths' control-character check (agy-F1 was incomplete), and delete the promptWritten per-lane coupling flag — the prompt write is idempotent, so writing it once per lane instead of gating on "did any lane write it yet" removes a latent bug where a deps.plan override that ever varies promptPath per lane would silently skip writing for a later lane. Emitted-Drift-Ack-Growth: gsd-core/workflows/code-review.md — net line count drops (the trait scrape moved into dispatch-step), but the file still grew this session across multiple commits; acknowledging per the growth-tracking convention. * fix(#4209): remove per-run token waste from the shipped prompts Runtime prompt content, not session tokens: two real, per-invocation token costs in the code that ships. 1. agents/gsd-code-reviewer.md's critical_rules restated nearly all of load_context step 5's ~180-word untrusted-evidence contract in ~90 more words, breaking this section's own established terse one-liner style (every other rule here is 1-2 sentences). This prompt loads fresh on every /gsd:code-review invocation. Shrunk to a one-line cross-reference, matching how write_review's own reference to step 5 already does it. 2. buildSourceReviewPrompt repeated the base SHA on every single file line even though it is identical for every file and already stated once at the top of the prompt — O(files) wasted tokens on every dispatched lane for a 50-file review, for zero information gain. File lines are now bare paths. * fix(#4209): resolve reviewer-lane trait in-process, fix CI failures found in review round 3 Opus critical-code-reviewer found a real Blocking defect in the --cap-id/ --point self-invocation added last commit: `dispatch-step` spawned `loop render-hooks <point> --raw` as a subprocess and bare-JSON.parse'd its stdout, but `io.cjs`'s output() redirects any payload over 50000 chars to `@file:<path>` instead of inline JSON -- the same overflow protocol this feature already unwraps for its OWN dispatch result 60 lines later in code-review.md. A large-enough activeHooks envelope (more installed capabilities/fragments) would throw, get silently swallowed by the bare catch, and misreport a real trait as trait_not_enabled with zero diagnostic. Fixed by extracting the config/registry/capability-state resolution `cmdLoopRenderHooks` already performs into an exported pure function, resolveActiveHooksForPoint (both `cmdLoopRenderHooks` and dispatch-step now share it), and calling it in-process from dispatch-step instead of spawning a subprocess at all. This eliminates the @file: exposure entirely (the dispatch-step path never touches the rendered-string envelope or its JSON-stringify/50000-char threshold), removes one subprocess spawn per code-review invocation, and gives a genuine diagnostic (stderr warning) on resolution failure instead of silent fail-closed. Corrected three doc/ docstring references to the now-removed subprocess self-invocation. Also fixes 2 real CI failures this round surfaced: - lint-tests: the agy-F1 control-char regex fix's `eslint-disable-next-line no-control-regex` comment was unused under this project's ESLint config (verified locally: the rule never actually flags \x00-\x1f in this repo's config) -- a mistake from an earlier commit this session, never actually lint-checked before push. Removed the disable comment. - security (prompt-injection-scan): the agy-F1 regression test's crafted fixture literally contains "Ignore all prior instructions." as test data proving validatePaths rejects it -- allowlisted the test file, same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class as existing entries. Also trimmed agents/gsd-code-reviewer.md's load_context step 5 (R2): one bullet stated "untrusted, never a command" three different ways in one paragraph, and a same-file duplicate of write_review's schema rule. Consolidated to state each rule once. Declined one suggestion from this round: shrinking code-review.md's EXTERNAL_EVIDENCE_BLOCK to a bare evidence list. Two tests (tests/code-review-pipeline-regression.test.cjs's CONS-01..03 block, tests/code-review.test.cjs's CONS-02 test) deliberately lock the four- prohibitions restatement and the untrusted-evidence prose into the INJECTED block itself, not just the consolidator's system prompt -- adjacency of the warning to the untrusted payload it's warning about is a recognized prompt-injection defense-in-depth pattern from this workstream's original TDD plan, not accidental duplication. * fix(#4209): correct stale per-file base-SHA prose in the external prompt Leftover from removing the per-file base SHA repetition earlier this session: the review-request sentence still said "relative to its base SHA" (singular per-file framing) when there's now exactly one base SHA, stated once above the file list. Reads "relative to the base SHA above" now. * fix(#4209): make getLane/configGet/plan required deps, delete dead defaults R3/R4 from the review round I'd deferred as low-priority test-churn: this file's one production caller (gsd-tools.cjs's dispatch-step handler) always supplies all three, so the fallbacks were dead in production -- but each was actively WRONG if ever reached: the default configGet always returned undefined, silently disabling resolveLaneBudget's overflow guard; the default getLane looked up only first-party REVIEWER_LANES, diverging from production's overlay-merged roster; the default plan skipped per-host effort resolution entirely. These defaults were introduced by this PR's own earlier work (this file did not exist before #4209 -- first commit a760bfcda, 01-02), not inherited from elsewhere, so there's no external caller depending on the lenient contract. Turned out free to fix: making the three deps required and deleting defaultGetLane/defaultPlan needed zero test changes -- every existing test that actually reaches the per-lane loop already supplies getLane/plan explicitly, and configGet's only real dependent (the budget-overflow tests) already supplies it too. 788/788 tests pass unchanged, tsc/lint clean. * fix(#4209): define depth semantics for the external reviewer lane Verified this was a real bug, not a match to existing convention as I'd claimed when declining the suggestion earlier this session: the internal gsd-code-reviewer agent's own system prompt carries a full <depth_levels> block defining what quick/standard/deep mean and do (agents/gsd-code- reviewer.md:68-99). The external reviewer lane has no access to that persona at all -- it only ever sees buildSourceReviewPrompt's bounded text, which sent the bare depth label with zero definition to a third-party CLI with no other source of truth for what "standard" means. Added depthMeaning(), condensed from the internal reviewer's own <depth_levels> definitions so the two stay consistent, and interpolated it into the review-request sentence. 150/150 tests pass, tsc/lint clean. * fix(#4209): merge dispatch_reviewer_lanes' split fences into one shell invocation CR-01 (Opus critical-code-reviewer, confirmed by direct execution): the roster-matching fence set EXPLICIT_JOINED/EXPLICIT_REVIEWER_SLUGS, and a SEPARATE later fence read them via ${#EXPLICIT_REVIEWER_SLUGS[@]} to decide whether to dispatch at all. This file's own documented rule (its depth-resolution guard, stated explicitly a few hundred lines earlier) is that a guard and the extraction it protects must run as one shell control-flow decision, because markdown-fenced blocks do not share shell state -- this step violated its own file's rule for the entire feature's gating condition. Merged the roster-resolution fence and the dispatch-decision fence into one continuous bash block, removing the intervening prose that split them. Fixed the stderr-based failure detection in the same edit (RQ-01: checking whether stderr is non-empty misfires on any benign Node warning; now checks the actual exit status of the roster-resolution command). Verified by extracting the merged fence and executing it standalone, driving both branches: --codex resolves EXPLICIT_JOINED=codex, SLUGS_COUNT=1, and a real dispatch-step call succeeds; no flags resolves EXPLICIT_JOINED empty, SLUGS_COUNT=0, dispatch-step never invoked (COMP-01). 141/141 workflow tests pass, tsc/lint clean. * fix(#4209): depthMeaning accuracy, injection defense on all embedded fields, hoisted prompt write Batch of Required/Suggestion fixes from the Opus critical-code-reviewer + writing-for-agents pass: - CR-02/CR-03: depthMeaning() dropped real categories from quick (empty catch blocks, commented-out code) and deep (error propagation, state mutation consistency, circular dependencies) relative to the real <depth_levels> block, and had zero test coverage. Restored full accuracy and added tests that read the real agents/gsd-code-reviewer.md file directly, so drift between the two can't recur silently. Unrecognised depth now normalizes to standard's definition, matching that agent's own documented rule, instead of rendering an undefined bare label. - RQ-04: depth/baseSha/repoRoot/runDir land in the same markdown prompt `paths` does, but weren't checked for control characters like paths were (agy-F1's original finding). Hoisted CONTROL_CHAR to module scope and applied it to all four fields at the same provenance-check boundary. runDir previously had zero validation at all. - S1: deleted the dead `identity` parameter on `invoke` -- the one production caller already ignores it, no test read it by name. - S2: hoisted the shared prompt write above the per-lane loop -- promptPath is derived from runDir alone (constant across lanes by construction), so writing it once is both correct and cheaper than the per-lane write R1 introduced earlier this session. Discovered and fixed a real regression from the naive version of this hoist: an unguarded throw would have escaped dispatchReviewerLanes as an uncaught exception instead of a clean per-lane failure. Added a new PROMPT_WRITE_FAILED whole-dispatch reason, matching the existing validatePaths/MISSING_PROVENANCE halt pattern, with a dedicated regression test. - S3: moved `planned = true` past the budget-overflow gate, so `dispatched` only reports true once a lane has cleared BOTH plan and budget checks. - S5: relayed gsd-code-reviewer.md's own "performance issues are out of scope unless also correctness issues" policy into the external-lane prompt, which previously had no such guidance and could return findings the internal reviewer's own contract excludes. - RQ-05 (partial): shrunk this file's own header docstring's restatement of the trait-reuse architecture to a pointer at gsd-core/references/loop-hook-dispatch.md, the canonical home. 234/234 tests pass across the full reviewer-lane test suite, tsc/lint clean. * fix(#4209): dedupe roster-merge logic, consolidate trait architecture prose, add step completion criterion RQ-02: added a `review-lane explicit-from-argv` subcommand that reuses the SAME merged-roster logic (`laneBySlug`) `dispatch-step`/`plan`/`invoke` already share. code-review.md's ~18-line inline `node -e` reimplementing `loadRegistry`+`mergeReviewerLanes` (a rename-only copy of the block in gsd-tools.cjs) is now a single call to this subcommand -- the exact violation code-review-flags.cjs's own header warns against ("this is the canonical flag-parsing surface -- do not replicate inline bash parsing"). RQ-03: an empty --cap-id XOR --point now warns distinctly from the legitimate no-context opt-out (both absent) -- a caller that named a capability without its point was silently indistinguishable from a correct opt-out. Also hardened the CODE_REVIEW_POINT config-get fallback: it only ever fires when the config-get COMMAND ITSELF fails (config-get already resolves the manifest's own schema default in the normal case), but that failure was previously silent. RQ-05/W-01/W-12/W-13: the "supportsReviewerLanes is a reusable trait resolved inside dispatch-step" explanation was restated in full in 5 places across this session's own review cycles. Consolidated to ONE canonical statement in gsd-core/references/loop-hook-dispatch.md; the other 4 (this file's own header, gsd-tools.cjs's comment, docs/ARCHITECTURE.md, code-review.md's step-opening comment) now point at it instead. W-05/W-06: loop-hook-dispatch.md described "false or non-boolean" as two inert cases when capability-validator.cjs already rejects non-boolean at load -- restated as the two cases that actually reach this code. Removed a "do not hand-roll trait resolution" prohibition whose target no longer exists once the positive description precedes it. W-04: deleted a no-op sentence in agents/gsd-code-reviewer.md ("missing block means proceed as normal") -- an absent optional block already means proceed as normal without being told. W-08/W-09: replaced longhand "zero selection/plan/invoke calls" and the made-up compound "byte-for-behavior [un]changed" with the token this session's own docs already coined for this concept (inert) and the word that means what byte-for-behavior was reaching for (unchanged). W-10: dispatch_reviewer_lanes had no completion criterion -- added one sentence naming the checkable end state (EXTERNAL_EVIDENCE_BLOCK is set, either populated or empty). This exact sentence would have caught the cross-fence bug fixed two commits ago at authoring time. Declined from this round, with reasoning: W-02/W-03 (trim the untrusted-evidence restatement in EXTERNAL_EVIDENCE_BLOCK/critical_rules) -- two tests deliberately lock this as intentional adjacency-based prompt-injection defense-in-depth, not accidental duplication (see this branch's own earlier commit). S4 (wrap LANE_RUN_DIR in a creation-site `trap ... EXIT`) -- would fire at the end of the CREATING fence, before spawn_reviewer's agent ever reads the evidence files, given this file's own documented fenced-block execution model; the existing named cross-reference between creation and cleanup already satisfies the co-location concern without introducing that regression. 853/853 tests pass across the full reviewer-lane test suite, tsc/lint clean. * fix(#4209): merge CODE_REVIEW_POINT into dispatch_reviewer_lanes' one fence, stop test from spawning real codex Round-5 review (agy) found the same cross-fence-split bug CR-01 already fixed for EXPLICIT_JOINED/EXPLICIT_REVIEWER_SLUGS: CODE_REVIEW_POINT's config-get fallback lived in an earlier, separate fence from the fence that consumes it via --point, split only by prose (not a guard, per this step's own documented rule). Merged into the single continuous fence and added a structural test asserting exactly one bash fence in the step. The new end-to-end regression test for this used --codex, which drives the fence's real `review-lane dispatch-step` call and, with the codex binary present on PATH, spawns the real external CLI — which then blocks on interactive auth with no stdin (BL-01). Stubbed gsd_run for `review-lane dispatch-step` only (captures argv instead of executing), keeping the real config-get/explicit-from-argv calls the test is actually about. * fix(#4209): split control-char vs missing provenance reason, realpath-check path escapes, stale comment Round-5 review (Opus) warning-tier findings: - WR-04: MISSING_PROVENANCE covered both "field absent" and "field present but a control-character injection attempt" — a caller distinguishing a config problem from a security event couldn't tell them apart. Split into MISSING_PROVENANCE (absent) and INVALID_PROVENANCE (present but invalid). - WR-05: validatePaths' containment check was lexical only (path.resolve), so a symlink whose own path sits inside repoRoot could still point outside it. Added an fs.realpathSync check (ENOENT-tolerant — a git-diff path can legitimately name a file already deleted in a stale worktree), realpathing repoRoot itself too so a symlinked repoRoot (e.g. /tmp on macOS) doesn't false-positive-reject its own real children. - WR-08: a comment in the per-lane loop still said a throwing writePromptFile() was caught there — stale since the prompt write was hoisted above the loop in an earlier round. WR-03 (validate depth against the quick/standard/deep enum) was considered and declined: this dispatcher is deliberately capability-neutral (see the existing "synthetic step context" test, which passes a non-code-review depth label on purpose to prove no code-review-specific special-casing exists). WR-01 (double registry load), WR-02 (trim-vs-hard-fail budget semantics), and WR-07 (reason omitted on the aggregate return) were verified against source and are not bugs — see review notes. * docs(#4209): document LANE_RUN_DIR's early-exit trade-off as accepted, not a gap Round-5 review (Opus, BL-03) flagged that an early exit between dispatch_reviewer_lanes and commit_review leaks the run-scoped temp dir. A trap-based cleanup was considered and rejected: if a step genuinely runs as a separate process, a trap set at creation time would fire at the end of that SAME fence, deleting the directory before spawn_reviewer/commit_review ever read it — worse than the leak it would fix. review.md's own gather_context/cleanup pair for the identical resource class (a run-scoped reviewer temp dir) already makes and documents this exact trade-off: cleanup runs only on a documented success path, and a leftover $TMPDIR entry is explicitly called cheaper than destroyed evidence. Recording that precedent here so this isn't re-raised as a live gap in a future review. * fix(#4209): register the WR-05 symlink-escape test's synthetic docs/ path reviewer-step-dispatch.test.cjs's "capability-neutral reuse" fixture passes paths: ['docs/spec.md'] as a synthetic, never-read path proving the dispatcher has no code-review-specific special-casing. lint-docs-guard- registration correctly flagged this as an unregistered docs/ path reference — add the docs-guard-exempt marker and its pinned baseline entry, the same pattern every other synthetic docs/ literal in this test suite already uses. * fix(#4209): backfill changeset pr: field with the real upstream PR number changeset-lint's fail_pr_field_drift caught the fragment still pointing at the fork PR (17) instead of the upstream one (open-gsd/gsd-core#4323) this branch is now also open against. * docs(#4209): amend ADR-2782 for the supportsReviewerLanes step-trait seam trek-e's review (2026-09-07, gsd-core#4323) found a real ADR gap: every decision in ADR-2782 (D1-D9) and every prior dated amendment governs the `role: "reviewer"` capability body and its one consumer, /gsd:review. This PR's actual new seam - a `supportsReviewerLanes: true` trait on an ordinary feature capability's `steps[]` entry, projected through loop-resolver.cts and resolved in-process via resolveActiveHooksForPoint - is a different capability axis (steps/gates/contributions) that the ADR's own scope note explicitly places out of reach. Per docs/contributor-standards.md's "Amending an accepted ADR", an in-place dated section is the established, lighter-weight path for an addition that stays within the ADR's existing decisions - used twice already in this same file - so this appends a third dated entry documenting the new seam, its consumer, and why it reuses the existing D1-D9-governed plan/invoke machinery rather than adding a second one. No decision is reversed; no new Amends/Amended-by pair is needed since the steps/gates/contributions axis already carries reciprocal links to ADR-857 and ADR-894. * fix(#4209): close two test-quality gaps trek-e's review found Minor 1: validatePaths (a path-shape parser guarding the prompt- injection/path-traversal trust boundary) had only example-based coverage, violating ADR-456's rule that parsers/budget limits carry at least one fast-check property test. Adds three: safe-segment paths are never rejected, a single leading "../" always escapes the one-segment repoRoot, and a control character anywhere is always rejected - one property per rejection reason validatePaths owns. Minor 2: the budget-overflow check (`estimatedTokens > budget`) was only ever exercised far below budget or at budget:0 (unbounded), never at the exact threshold crossing where a `>` vs `>=` off-by-one would hide. Adds three exact-boundary tests using the real estimateTokens/ buildSourceReviewPrompt the module calls internally, so the resolved token count is exact rather than approximated: budget == estimate (must pass), budget == estimate - 1 (must fail), budget == estimate + 1 (must pass). Also extracts okPlan()'s fixture timeoutMs into a named constant - local/no-adhoc-timeout-literal (#4446) landed on next after this branch was authored and flagged the pre-existing literal on rebase; it is fixture data for a synthetic plan object dispatchReviewerLanes never waits on, a distinct class from tests/helpers/timeouts.cjs's real subprocess norms. * fix(#4209): update docs-guard-registration baseline for the new ADR citation reviewer-step-dispatch.test.cjs's new fast-check property tests cite docs/adr/456-test-rigor-architecture.md in a justifying comment (never a real read). lint-docs-guard-registration fingerprints every docs/ path string an exempted test file mentions and fails on drift so a human re-confirms the exemption still holds - re-confirmed, and the baseline is updated to match. * fix(#4209): point changeset pr: field at the fork PR for CI validation changeset-lint's fail_pr_field_drift check compares the fragment's pr: field against the PR the CI run is actually attached to (GITHUB_EVENT_PATH), not a fixed target. Rehearsing this branch on fork PR davdittrich/gsd-core#17 needs pr: 17 to pass that check; the prior commit's pr: 4323 (the real open-gsd upstream PR number) is correct for that PR but fails here. Backfill to 4323 happens again, as the last commit, immediately before the approved push to open-gsd#4323 - never leaving pr: 17 on the branch that ships upstream. * fix(#4209): reject promptChannel:none lanes from source-review dispatch CodeRabbit found a real scope mismatch: coderabbit's lane declares promptChannel: 'none' and reviews the working tree on its own terms, fed nothing (review.md:367). Silently dispatching it through dispatchReviewerLanes would ignore the bounded paths/depth/baseSha scope buildSourceReviewPrompt promises and let the lane review whatever it independently sees fit, violating this interpreter's own scoped, metadata-only contract. Reject before plan()/invoke(), same as an unresolved slug. * fix(#4209): scope CONS-02 test to the evidence-block line, not the whole file CodeRabbit found the whole-file match on workflowContent would still pass if UNVERIFIED and re-open/reopen appeared in two unrelated parts of this 1000+-line workflow, proving nothing about the actual evidence block's contract. Line-filtered via splitLines (not a bare-\n regex spanning readFileSync content) so this stays CRLF-portable and passes local/no-unbounded-quantifier and local/no-crlf-fragile-split. * fix(#4209): guard DISPATCH_JSON substitution and capture its stderr CodeRabbit found the dispatch-step command substitution unguarded: a non-zero exit could leave DISPATCH_JSON empty (or halt the step under errexit with no warning), and the downstream reducer would only ever report the generic unparseable_dispatch_output reason, discarding the command's own diagnostic. Guarded like the existing CODE_REVIEW_POINT/ EXPLICIT_JOINED calls above it: capture stderr to a temp file, surface it in a warning on failure, and fall back to a parseable dispatch_ command_failed JSON stub so the reducer's existing reason-reporting path still fires. * docs(#4209): fix byte-for-behavior wording and missing colon, regenerate CodeRabbit found "byte-for-behavior" should read "byte-for-byte" (the established repo term for output-identical unchanged behavior) and a missing colon after the bold "Optional external reviewer lanes (#4209)" lead-in in docs/features/code-review-pipeline.md. Fixed in the two hand-authored sources (commands/gsd/code-review.md, docs/features/ code-review-pipeline.md) and regenerated the two derived projections (skills/gsd-code-review/SKILL.md via gen-plugin-skills.cjs, docs/ FEATURES.md via gen-features.cjs) so they stay in sync. * fix(#4209): drop the fabricated DISPATCH_JSON fallback stub (Windows CI) The prior fix's fallback `DISPATCH_JSON='{"ok":false,...}'` embeds double-quoted JSON keys inside a single-quoted shell literal. That extra quote density, inside an already quote-heavy ~8KB driver string, passed bash -n and the full local suite on Linux but broke Windows Git-Bash: `dispatch_reviewer_lanes computes CODE_REVIEW_POINT ... end to end (#4209 round 5)` failed on two Windows CI shards with `bash -c: unexpected EOF while looking for matching '''` — a Windows argv-to- command-line re-quoting edge case, reproducible on rerun, not a flake. Root-caused via gh api job logs plus a byte-identical local reconstruction of the test's own driver script. Fix: drop the fabricated stub. The downstream node -e reducer already falls back to reason `unparseable_dispatch_output` on any JSON.parse failure, so an empty/partial DISPATCH_JSON on command failure is still handled correctly, with zero new quoting risk. * revert(#4209): drop the DISPATCH_JSON stderr-guard nitpick (Windows CI) Two materially different mechanisms for the same CodeRabbit Nitpick ("Trivial | Quick win") both broke Windows Git-Bash reproducibly: a single-quoted JSON-literal fallback ("bash -c: unexpected EOF ... matching '''") and, after removing that, a plain `head -1 "$VAR"` inside a nested command substitution ("unexpected EOF ... matching '"'"). Both passed bash -n and the full local suite on Linux every time; both failed the SAME test deterministically on Windows CI. Two attempts at the same class of fix (nested-quote construction near this exact step) is the retry limit - reverting to the original, already-shipped, Windows-verified unguarded form rather than continuing to guess at a third quoting mechanism for a Trivial- severity nitpick. Logged as bug-221/bug-222 in .wolf/buglog.json for anyone attempting this again: the fix belongs outside this specific markdown-fence-driver test harness (e.g., a real .sh helper script) if it's worth doing at all. * fix(#4209): backfill changeset pr: field to the real upstream PR before push Fork validation (davdittrich/gsd-core#17) needed pr: 17 to satisfy changeset-lint's PR-number check while rehearsing there; this is the last commit before the approved push to the real upstream PR (open-gsd/gsd-core#4323), so the field points at that PR number again. --------- Co-authored-by: Test <test@test.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-09-07 22:52:33 -04:00
Tom Boucher	2f64e6230a	feat(#3676): quick-batch command, workflow, and isolation integration (#4212) ... * test(#3676): add failing tests for quick-batch dispatch core Failing-first tests for Phase 4 of epic #3344 (ADR-1239 "Quick-batch binding"): quick-batch-dispatch.test.cjs / .property.test.cjs cover the new pure decision-logic module (arg validation, effective concurrency, deterministic merge order, spawn backpressure, verification/merge routing, cleanup-entry construction — design doc rows 3-15,24,26-28, 30-36,39; property rows 51-53). quick-batch-update-items.test.cjs covers the new updateBatchItems export on src/quick-batch.cts (rows 15,22-23, including the negative cycle-rejection case). quick-batch-command-router.test.cjs covers the new gsd-tools quick-batch CLI family (rows 46-47). These reference modules/ exports that do not exist yet. * feat(#3676): implement quick-batch dispatch core, updateBatchItems, and command router Phase 4 of epic #3344 (ADR-1239 "Quick-batch binding") CORE decision layer — CLI verbs and pure orchestration logic only; no workflow markdown, no Agent()/git-worktree I/O. - src/quick-batch-dispatch.cts (new): pure decision functions consumed by the (separate, follow-up) /gsd:quick-batch workflow markdown — parseQuickBatchArgs, computeEffectiveConcurrency, computeMergeOrder, computeSpawnPlan, routeVerificationOutcome, routeMergeOutcome, buildCleanupManifestEntry (the last parses caller-supplied plan text via the existing parsePlanDocument; no filesystem access). - src/quick-batch.cts: adds updateBatchItems, resolving the design doc's Open Question 1 as ONE additive export on this module instead of the second, independent BATCH.json writer the design doc originally proposed. Reuses the same withPlanningLock transaction shape, computeWaves, and platformWriteSync call resumeBatch/ completeQuickItem already use; fails closed without persisting on an unknown item, an unknown/self dependency, or an introduced cycle. - src/quick-batch-command-router.cts (new): gsd-tools quick-batch CLI family, wired into HOST_COMMAND_ROUTERS (gsd-core/bin/gsd-tools.cjs) as a first-party always-on command (like /gsd:quick), not the opt-in capability-registry path graphify uses. Verbs: create/update/resume/ complete (wrap quick-batch.cts) and effective-concurrency/ merge-eligible/spawn-plan/verification-routing/merge-routing/ cleanup-entry/parse-args (wrap quick-batch-dispatch.cts). Design doc rows covered: 3-15, 22-24, 26-28, 30-39, 46-47. Property rows 51-53. Rows covering workflow markdown / Agent() dispatch / `git worktree` behavior (16-21, 25, 29, 40-45, 48-50) remain for the follow-up markdown-authoring pass, per the phase brief's explicit scope boundary. * docs(#3676): register quick-batch-dispatch/command-router modules in bookkeeping surfaces New-.cts-module ripple for the two Phase 4 modules (epic #3344, ADR-1239 "Quick-batch binding"): .gitignore (compiled .cjs artifacts, ADR-457 build-at-publish), eslint.config.mjs (lint the .cts source, not the emitted .cjs), docs/INVENTORY.md + docs/INVENTORY-MANIFEST.json (via `node scripts/gen-inventory-manifest.cjs --write`, after `npm run build:lib`), and CONTEXT.md glossary entries for "Quick-Batch Dispatch Core Module" and "Quick-Batch Command Router Module", plus an update to the existing "Quick-Batch Core Primitives Module" entry documenting the new updateBatchItems export. * test(#3676): fold updateBatchItems tests into quick-batch.test.cjs (fix lint-test-file-count) scripts/lint-test-file-count.cjs buckets any quick-batch-*.test.cjs file under the quick-batch production module by longest-prefix match, and that module is already at its 2-file cap (quick-batch.test.cjs + quick-batch.property.test.cjs). The standalone tests/quick-batch-update-items.test.cjs added in the prior commit pushed it to 3 and failed `npm run lint:ci`. Fold its content into quick-batch.test.cjs (append-only — no existing test in that file is modified) and update the CONTEXT.md glossary reference to match. Surfaced while re-running `GITHUB_BASE_REF=next npm run lint:ci` after `npm ci` (this worktree previously had no local node_modules, which also made gen-scripts-cli-exit/gen-hooks-cli-exit/gen-exit-code-* unable to resolve typescript — resolved by npm ci, no code change needed there). `npm run lint:ci` and `npx tsc -p tsconfig.build.json --noEmit` are both green after this fix. * test(#3676): add failing tests for the quick-batch command/workflow markdown Failing-first tests for Phase 4's markdown-authoring pass (epic #3344, ADR-1239 "Quick-batch binding"): gsd-quick-batch-workflow.test.cjs covers commands/gsd/quick-batch.md's frontmatter/objective/process, gsd-core/workflows/quick-batch.md's byte-size boundary (row 49, ADR 1610 NEW_FILE_CAP) and step-fragment count, the isolation model (rows 20-22), the executor single-writer invariant (row 18), merge validation reusing the existing bounded primitive (row 25), the optional research/plan-checker/verification leaves (rows 16,17,19, 30,31), planning-failure blocking execution (row 29), the submodule guard (rows 36,44), and the new agents/gsd-planner.md quick-batch mode (rows 13-15). gsd-quick-batch-quick-regression.test.cjs covers row 48 (ordinary /gsd:quick stays byte-identical). Named `gsd-quick-batch-*` (not `quick-batch-*`) so lint-test-file-count's longest-prefix bucketing doesn't fold these markdown-only tests into the already-capped quick-batch/quick-batch-dispatch/ quick-batch-command-router production-module buckets from the CORE pass. These reference files that do not exist yet. * feat(#3676): author the quick-batch command, workflow, and planner mode Phase 4 markdown-authoring pass (epic #3344, ADR-1239 "Quick-batch binding") — the orchestration layer that calls into Pass 1's CLI verbs (src/quick-batch-command-router.cts). - commands/gsd/quick-batch.md (new): frontmatter/objective/process, delegates argument validation to `quick-batch parse-args` (parseQuickBatchArgs) rather than re-deriving the grammar. - gsd-core/workflows/quick-batch.md (new, 11843 bytes — under ADR 1610's 32768-byte NEW_FILE_CAP for a brand-new file) + 9 lazy-loaded step fragments under gsd-core/workflows/quick-batch/steps/: resume-mode, batch-init, research-phase (flag:--research), planner-wave (+ nested plan-checker-loop when --validate), worktree-dispatch, merge-wave, verification-wave (flag:--validate), completion. Covers design doc rows 3-45: capacity/isolation resolution (reusing dispatch-isolation-gate.md verbatim), per-DAG- layer planning with full-task-catalog prompts and always-required depends_on/files_modified frontmatter, serialized worktree create/ merge/cleanup via the existing worktree.cleanup-wave primitive, deterministic wave-order merging, verification routing (human_needed/gaps_found), the executor single-writer invariant, submodule fail-loud guard, and #1941 fork-base auto-degrade. - agents/gsd-planner.md: additive new `load_mode_context` bullet for `**Mode:** quick-batch`, pointing at the new gsd-core/references/planner-quick-batch.md reference (documents the always-required depends_on/files_modified contract, reusing the existing frontmatter grammar — no new keys). Existing modes byte-identical, only a new bullet added. - src/init.cts (+init-command-router.cts, +command-aliases.cts): cmdInitQuickBatch / `init.quick-batch` — model profiles, commit_docs, roadmap/planning existence checks, and the section_manifest field gating research-phase/verification-wave (reuses the existing flag:--research/flag:--validate WHEN_VOCABULARY atoms — no new atom needed). Rows 16-21, 25, 29, 36, 38, 39, 44, 46-50 covered structurally by the prior test(#3676) commit; rows 3-15, 22-24, 26-28, 30-35, 37, 40-43, 45 covered by construction (verb wiring, single-writer prompt constraints, crash-window resume via unmodified Phase 3 primitives). * docs(#3676): regenerate skills/inventory/section-manifest/install-tree; baseline the intentional word-splitting pattern npm run regen:derived output for the new command/workflow/reference (epic #3344, ADR-1239 "Quick-batch binding"): - skills/gsd-quick-batch/SKILL.md (generated from commands/gsd/quick-batch.md) - docs/INVENTORY.md rows for /gsd-quick-batch, quick-batch.md, planner-quick-batch.md, and the quick-batch-dispatch.cjs/ quick-batch-command-router.cjs CLI-module rows' now-live `/gsd-quick-batch` cross-reference (was "(separate, follow-up)") + docs/INVENTORY-MANIFEST.json (`node scripts/gen-inventory-manifest.cjs --write`) - gsd-core/workflows/section-manifest.json (`npm run gen:section-manifest`) — research-phase/verification-wave gsd:section entries for the new quick-batch workflow - tests/fixtures/install-tree/*.json (`npm run gen:install-tree`) — the new command/workflow/skill/reference files now ship to every runtime scripts/lint-workflow-shellcheck-baseline.json: 3 new entries for gsd-core/workflows/quick-batch.md's intentional flag-token/$ARGUMENTS word-splitting (SC2046/SC2086) — the same deliberate unquoted-optional- flag pattern gsd-core/workflows/quick.md already carries baselined (e.g. `$DISCUSS_PARAM $RESEARCH_PARAM` in quick.md's own Step 2); quoting would break the intended "omit this arg when the flag is false" splitting. * fix(#3676): close prompt-injection and argv/glob-injection gaps in quick-batch leaf dispatch Security review pass findings, both confirmed real: 1. HIGH — prompt injection, no boundaries. Every leaf-dispatch fragment interpolated the raw, attacker-influenced task ${description} (and the shared ${TASK_CATALOG_TABLE}, broadcasting every item's raw description into every planner's prompt in the layer) straight into Agent() prompt bodies with no boundary. Fixed by wrapping every such interpolation in a <security_context> + DATA_START/DATA_END boundary, matching the CONCRETE convention already implemented in this repo (agents/gsd-debug-session-manager.md, agents/gsd-debugger.md, gsd-core/workflows/debug.md) — commands/gsd/quick.md's own <security_notes> only asserts this convention in prose, so the debug-agent files are the real precedent followed here. Added a new <security_notes> block to commands/gsd/quick-batch.md (it had none) documenting both this fix and the one below. 2. MEDIUM — unquoted $ARGUMENTS -> argv/glob injection. gsd-core/workflows/quick-batch.md and commands/gsd/quick-batch.md both ran `gsd_run quick-batch parse-args --raw -- $ARGUMENTS` UNQUOTED, causing shell word-splitting and pathname expansion on raw task-list text before the parser ever saw it. Fixed at the source: added a `--text <string>` form to the `parse-args` verb (src/quick-batch-command-router.cts) that accepts the ENTIRE $ARGUMENTS as ONE quoted argv element and does the whitespace split itself, in Node — which is never glob-aware, unlike the shell. Both call sites now use `--text "$ARGUMENTS"`. The `-- <tokens>` form is kept for direct/test callers that already have a real argv array. The SC2086 baseline entry added for the original unquoted line is now stale (`node scripts/lint-workflow-shellcheck.cjs` no longer reports it) and has been removed; the two SC2046 entries for the UNRELATED, still-unquoted `$([ "$VALIDATE_MODE" = true ] && echo --validate)`-style conditional-flag splitting remain — that line only ever expands to one of a few known-safe literal strings (never raw user text), matching quick.md's own already-baselined convention exactly. Tests: quick-batch-command-router.test.cjs covers the new --text form (token splitting, glob-shaped text passing through literally unexpanded, whitespace-only input). gsd-quick-batch-workflow.test.cjs asserts the DATA_START/DATA_END boundary on every leaf prompt (research-phase/planner-wave/plan-checker-loop/verification-wave, including the shared task catalog) and the quoted --text call sites. * fix(#3676): strengthen test-depth gaps in rows 9, 18, 24, 34, 35 Spec review pass findings — the test matrix claimed "yes" coverage these assertions did not actually support: - Row 9 (--jobs 0/-1/abc hostile case): previously asserted rejection only. Added an end-to-end assertion (tests/quick-batch-command-router.test.cjs, committed alongside the security fix that touches the same file) that .planning/quick-batches/ is never created for any rejected value — createBatch is genuinely never reached. - Row 18 (--resume <unknown-batch-id>): previously only exercised a hand-corrupted BATCH.json, never a genuinely nonexistent batch directory. Added the real nonexistent-id case (also in quick-batch-command-router.test.cjs). - Row 24 (post-planning updateBatchItems racing a concurrent completeQuickItem for a different item, both through withPlanningLock): zero test existed. Added a property test (tests/quick-batch.property.test.cjs, appended — Phase 3's own file, no existing test touched) exercising both call orders and asserting no lost update in the final on-disk manifest — the same technique Phase 3's own row-15 lock-contention property test uses (sequential calls through the real lock; a working mutex makes any interleaving equivalent to some serial order, so this is the same claim a literal concurrent-thread test would make without OS-level threading). - Row 34 (worktree preserved on merge_failed) and row 35 (undeclared- deletion detection): both were previously asserted only at the pure routeMergeOutcome level. Added tests/gsd-quick-batch-merge-integration.test.cjs using the SAME real-git-fixture pattern tests/worktree-safety.test.cjs already establishes for executeWorktreeWaveCleanupPlan (real repo, real worktree, a REAL merge conflict / a REAL file deletion diffed against declared_deletions) — asserting the actual worktree directory survives on disk, not just that a pure function returns a preserveWorktree:true field. Named gsd-quick-batch-* so lint-test- file-count's bucketing doesn't fold it into any capped module bucket. Row 48 (/gsd:quick regression) intentionally left as-is per the reviewer's own framing: the byte-identity claim is already mechanically proven by the changed-path diff (git diff --name-only empty on those two paths IS byte-identity), and a genuine execution- level regression test would require actually running the workflow — out of scope for this repo's unit-test model (no other quick.md regression test in this repo does that either). * docs(#3676): add the changeset and user-facing docs the command needed Standards review pass findings — both HARD: - Missing changeset. None of the 6 prior #3676 commits touched .changeset/*. /gsd-quick-batch is a new user-facing command; CLAUDE.md/CONTRIBUTING.md require one. Added .changeset/silly-rams-caper.md (type: Added, pr: 0 placeholder — backfilled after the PR opens, matching CLAUDE.md's own documented convention and Phase 3's own precedent, #4190's .changeset/mellow-yaks-squeak.md). Uses the docs-convention hyphen form `/gsd-quick-batch` throughout, never the source-artifact colon form (`scripts/lint-docs-command-form.cjs` confirms 0 violations; that check scans docs/**, not .changeset/, so it was never actually in scope for the fragment itself, but the wording still follows the doc convention for consistency, matching how Phase 3's own fragment named the not-yet-shipped command). - Missing docs. Added docs/how-to/batch-quick-tasks.md (Diátaxis how-to, matching docs/how-to/handle-quick-and-fast-tasks.md's existing convention for /gsd-quick /gsd-fast) covering --jobs, --validate, --research, --resume, --file, the capacity/isolation interaction, and resume/failure recovery. Cross-linked from docs/README.md's how-to index and from handle-quick-and-fast-tasks.md's own "Related" section. Added a /gsd-quick-batch section to docs/COMMANDS.md (same table format as the existing /gsd-quick entry) and docs/features/quick-batch.md (REQ-QB-01..12, same frontmatter shape as docs/features/quick-mode.md) — regenerated docs/FEATURES.md (179 features) and skills/gsd-quick-batch/SKILL.md via the standard generators. * fix(#3676): close docs-parity, attribution, and generated-registry gaps gsd-test caught gsd-test's real run against 155e8975b3 found 43 failures, all rooted in this phase's own new command/workflow never being registered across ~10 independent generated/hand-maintained registries this repo keeps in parity by convention. Root-caused each, no test weakened or special-cased. - help.md ↔ commands/gsd/ bidirectional parity (docs-parity-live- registry.test.cjs): added a /gsd:quick-batch entry to gsd-core/workflows/help/modes/full.md (the real help.md content; gsd-core/workflows/help.md is a thin dispatcher) documenting every flag (--file/--jobs/--validate/--research/--resume), matching the existing /gsd:quick entry's format. - gen-section-manifest.test.cjs: quick-batch.md's `gsd_run query init.quick-batch` invocation used inline `$([ ... ] && echo --flag)` substitutions, which never satisfy the test's exact-whitespace-token / assigned-variable detection (the trailing `))` glued onto `--research` in the compound substitution broke the "exact token" match). Rewrote to the same VALIDATE_PARAM/RESEARCH_PARAM two-line pattern gsd-core/workflows/quick.md's own Step 2 already uses. - runtime-launcher-parity.test.cjs: the 8 quick-batch/steps/*.md fragments that call gsd_run each needed their OWN embedded copy of the canonical shim preamble (every workflow .md that calls gsd_run carries its own copy — reading one file does not persist shell state into another). Ran `node scripts/sync-runtime-launcher.cjs`, which inserted it before each file's first gsd_run call. plan-checker-loop.md correctly has none — it never calls gsd_run directly. - Namespace routing (skill-manifest.test.cjs, install-nested- layout.test.cjs, runtime-artifact-layout-surface.test.cjs): added `quick-batch` to commands/gsd/ns-workflow.md's `requires:` array and routing table (same namespace `quick` already routes through), and to src/clusters.cts's `utility` cluster (same cluster `quick` already belongs to). Verified by hand-running installRuntimeArtifacts + applySurface for augment/cline against a real temp install: exactly 6 top-level gsd-ns-* router dirs, gsd-quick-batch correctly nested under gsd-ns-workflow/skills/, never re-flattened. - mcp-server-catalog.test.cjs: hardcoded command count 71 -> 72 (a brand-new command is a real count change, not a bug this test should hide). - model-omit-when-inherit-guard.test.cjs: added the canonical `<!-- #2517 model-omit-on-inherit -->` marker block to gsd-core/workflows/quick-batch.md (every leaf dispatch — planner/ researcher/checker/executor/verifier — lives in a steps/ fragment, read combined with the host by this test's own readWorkflowCombined, same as quick.md's own research-phase.md carries it for its gated section). Also fixed a genuine pre-existing inconsistency in the test's own "#2711: the guarded set is derived from dispatch sites" check: its `nonDispatching` computation read the BARE host file while `derived` (the set it's checked against) reads the combined host+steps content — inconsistent with that same test file's own #2994 doc comment explaining why the combined read is necessary. quick-batch.md is the first workflow whose EVERY model="{...}" dispatch site lives in a mandatory (never gated) steps/ fragment — extracted to stay under ADR-1610's tighter NEW_FILE_CAP for a brand-new file — which is what exposed the mismatch. Fixed by using the same readWorkflowCombined read in both places. - skill-frontmatter-contract.test.cjs: shortened commands/gsd/quick-batch.md's frontmatter `description` from 107 to 91 chars (<=100 budget), and added `quick-batch.md` to the hand- maintained KNOWN_SKILLS consolidation allowlist with a #3676 justification comment (a genuinely new first-party command, not a consolidation of an existing skill). - workflow-fragments-emission.install.test.cjs: added `quick-batch.md` to the hand-maintained MARKED_WORKFLOWS set (composeWorkflow is deliberately NOT a no-op for it — its research-phase/verification- wave sections are gated). - Regenerated all downstream artifacts (npm run build:lib && npm run regen:derived && npm run gen:plugin-skills -- --write && npm run gen:features -- --write): skills/gsd-quick-batch/SKILL.md, skills/gsd-ns-workflow/SKILL.md, install-tree fixtures for augment/cline/hermes/qwen/trae/zcode. - emitted-attribution.test.cjs: agents/gsd-planner.md's #3676 addition (one new `load_mode_context` bullet pointing at the new gsd-core/references/planner-quick-batch.md reference) grew the file 124 bytes without an acknowledgment trailer. Acknowledged below — the growth is the deliberate, additive, single-bullet change from the earlier feat(#3676) commit, not drift. Verified: npm run build:lib clean, npx tsc -p tsconfig.build.json --noEmit clean, GITHUB_BASE_REF=next npm run lint:ci fully green (includes lint-workflow-shellcheck, lint-test-file-count, lint-docs-command-form). The deep install/spawn/registry tests gsd-test actually runs (docs-parity-live-registry, gen-section-manifest, runtime-launcher-parity, install-nested-layout, runtime-artifact-layout-surface, skill-manifest, skill-frontmatter- contract, mcp-server-catalog, model-omit-when-inherit-guard, workflow-fragments-emission) are not part of lint:ci — each fix above was independently verified by hand-invoking the exact production function the failing test calls (installRuntimeArtifacts, applySurface, composeWorkflow, the CLUSTERS union, the section-manifest forwarding regex) against the real repo tree and confirming the expected shape. Emitted-Drift-Ack-Growth: gsd-planner.md — additive #3676 quick-batch mode bullet in load_mode_context (one new line pointing at gsd-core/references/planner-quick-batch.md); not drift. * fix(#3676): trim the /gsd:quick-batch help.md entry to fit the LARGE tier line budget skill-frontmatter-contract.test.cjs's "feature #3039: tiered help — size budgets" enforces a SEPARATE line-count ceiling for gsd-core/workflows/help/modes/full.md (FULL_BUDGET = 844 lines, tighten-only ratchet, scripts/lib/allowlist-ratchet.cjs's assertTightCeiling) — independent of the skill-frontmatter description- length budget and consolidation allowlist I touched in the prior round; those are unrelated checks in the same test FILE, not the same check. Root cause: the /gsd:quick-batch entry I added to full.md in the docs-parity fix round was 17 lines, pushing the file from 834 to 851 lines — 7 over the 844 ceiling. Condensed the entry (merged the per-flag bullet list into one dense "Flags:" line, dropped from 3 Usage examples to 1) to 844 lines exactly — at the ceiling with zero slack, which assertTightCeiling accepts (it only fails on actualMax > ceiling, or on slack > grace when the ceiling is too LOOSE — zero slack triggers neither). Verified after trimming: full.md still contains a live /gsd:quick-batch reference (bidirectional parity) and all 5 argument-hint flags (--jobs/--validate/--research/--resume/--file) still appear as literal tokens (docs-parity-live-registry.test.cjs's own flag-coverage check, re-run by hand against the trimmed content). Verified: npm run build:lib clean, npx tsc -p tsconfig.build.json --noEmit clean, GITHUB_BASE_REF=next npm run lint:ci fully green. * docs(#3676): backfill changeset pr number to 4212 Follow-up to fix(#3676) commits — .changeset/silly-rams-caper.md's pr:0 placeholder backfilled with the real PR number now that gh api POST /pulls has returned it (#4212). Matches CLAUDE.md's PR Number Handling convention and Phase 3's own #4190 precedent (708c5a3f8c). Doc-only (root-level .changeset/*.md fragment), exempt from a fresh gsd-test run per pre-pr-gate.sh's DOC_ONLY_RE. * fix(#3676): resolve prompt-injection-scan false positive on test fixture tests/quick-batch.test.cjs:232's row 11b regression proves the task-list parser carries a prompt-injection-shaped task description through createBatch as inert data, never interpreted. The fixture has to be a real "ignore all previous instructions..." phrase or the test asserts nothing, but the full-file --diff scan flagged it once unrelated edits in the same file pulled it into the changed-file set. Add the file to prompt-injection-scan.sh's ALLOWLIST, matching the sanctioned, precedented exemption already used for other legitimate security-regression fixtures (tests/windsurf-conversion.test.cjs, tests/health-validation.test.cjs, tests/continuation-grammar-parity.test.cjs) per DEFECT.PROMPT-INJECTION-SCAN-COLLISION. --------- Co-authored-by: sim <sim@local>	2026-09-02 22:38:31 -04:00
Tom Boucher	1e67ec9737	enhance(#3908): the scanners distinguish an empty diff from one they could not compute (#3937) ... * feat(#3908): the scanners distinguish an empty diff from one they could not compute collect_files ended 2>/dev/null || true, which destroyed the evidence three ways: the redirect discarded git's diagnostic, the pipe replaced git's status with grep's, and || true forced success regardless. Four distinct conditions - an established-empty diff, a bad ref, no repository, and a repository with no commits - all reported clean, and a secret scanner reporting clean because git failed is indistinguishable from an all-clear to any gate consuming it. git now runs separately from the filter so its status and diagnostic both survive. An established-empty diff exits NO_INPUT; a scope that could not be established exits UNAVAILABLE; the usage sites move off 2 to USAGE. || true is retained on the filter alone, where it is correct: a diff of only images is empty, not failed. Codes are sourced from a generated shell fragment rather than written into three scripts, so a re-allocation cannot desync them, and a missing fragment fails loudly instead of falling back to literals. The security workflow is updated in the same change: without it, a docs-only PR would newly fail the job. * fix(#3908): keep scanner stderr out of the file list, and drop try/finally from test bodies Capturing git and find output with 2>&1 was right for the failure path but wrong for the success path: a warning emitted alongside a successful diff flowed into the file list and was treated as a filename. stderr is now captured separately, forwarded as a warning on success and as the diagnostic on failure, and never folded into the list. Also converts the control tests' try/finally blocks to t.after(), which CONTRIBUTING bans inside a test body because it masks failures. * chore(#3908): backfill changeset pr number * docs(#3908): record the scanners' four-outcome exit contract SECURITY.md is root-level, so the docs gate correctly held: a Changed fragment owes a file under docs/. The contract also belongs where the feature is described, as REQ-SCAN-INJ-05. docs/FEATURES.md is GENERATED from per-feature fragments (#3840) - the first edit went into the generated file and gen-features --check caught it, which is the same edit-the-output drift this epic exists to close. The fragment is the source; FEATURES.md is regenerated. --------- Co-authored-by: sim <sim@local>	2026-08-27 13:11:13 -04:00
JusticeWay	77374cfc32	fix(#2528): resolve digit-leading phase directories by bare number (#2559) ... * fix(#2528): resolve digit-slug phase dirs by bare number — tokenizer rewind, shared bare-integer fallback, resolution-path parity gate extractPhaseToken welded 2-digit slug words onto the phase token (phase 10 named "24/7 Autonomy" -> dir 10-24-7 -> token 10-24), making digit-prefixed phase names unresolvable by bare number across every phase verb. - phase-id: continuation segments must be the PURE 2-digit zero-padded form the write side emits; a 1-digit terminator rewinds the absorbed run (10-24-7 -> 10) while >=2-digit terminators keep the locked #2232 round-trip (14-06-2026-photos -> 14-06). - phase-id: new matchPhaseDirs owner — primary exact-token match plus a bare-integer leading-digit-run fallback for shapes the tokenizer cannot rewind (05-80-20-cleanup); collisions stay #2237-loud. - locator/find-phase/phase-plan-index all delegate selection to the owner; plan-index gains the previously missing multi-match guard. - tests: #2528 unit + fast-check metamorphic blocks; new 9-scenario resolution-path parity gate across all three paths. Fixes #2528 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * chore(#2528): add changeset for PR #2559 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(#2528): align validation token grammar * fix: address phase token review * fix: restore phase grammar parity for numeric slugs * docs: document digit-leading phase resolution * docs: clarify ambiguous phase resolution behavior * docs: register canonical phase directory selectors * fix: align prefixed deep phase token parsing * fix(#2528): route the fourth resolution site through matchPhaseDirs Review BLOCKER. smart-entry.cts::detectVerifyFailed resolved the current phase's directory with its own `.find(phaseTokenMatches)` and never reached the shared selection, so the bare-integer-fallback family the issue names — `05-80-20-cleanup`, `30-12-factor-refactor` — resolved nowhere. The miss is silent by construction: an unresolved phase reports "not failed", which is byte-identical to a healthy one, so a failed verification simply never surfaced in /gsd or /gsd:progress. `entries` is already sorted and matchPhaseDirs filters without reordering, so matches[0] reproduces the previous selection exactly wherever the old code resolved at all. Wiring it into phase-resolution-parity.test.cjs as a fourth path then exposed a second, older defect in the same function: phaseTokenFromDirName shape-probed the UNSTRIPPED token, so a project-code-prefixed directory (`MEM-05-…`, tokenizing to `MEM-05-80-20`) failed the leading-digit test and was dropped before any resolution ran — every phase in a project-coded plan was invisible to this check. The probe now runs on the stripped token; the returned value is unchanged, so the comparePhaseNum sort is untouched. Path 4 has no JSON surface to compare, so the gate observes selection indirectly: plant the failing artifact in exactly one directory and a passing one everywhere else, then read the boolean. Reverting either fix turns 5 of the 10 corpus scenarios red. * refactor(#2528): collapse the duplicated extractCanonicalPlanId Review MAJOR. The function existed as two independent, byte-identical copies — src/core-utils.cts and src/phase.cts — and this PR had to patch BOTH with the same single-digit-slug rewind rule. That is the generative fix divergence CLAUDE.md names, and only the core-utils copy was under test, so a future one-sided patch would have silently split plan-id canonicalization between the plan listing and everything else. Removed rather than parity-tested: core-utils was already the leaf owner and already exported it, and phase.cts already imported that module, so there is no second surface left for a parity test to police. * test(#2528): pin matchPhaseDirs at the digit-width boundaries Review MAJOR. The bare-integer fallback's correctness rests entirely on capturing each directory's whole leading digit run before the zero-strip compare; a regex that stopped short would turn every query into a prefix match, and "1" would claim 10, 100, and 12 alike. The existing coverage was example-based and never touched that boundary. Adds the explicit 9/10 and 1/10/100 cases — including the forms where only the wider directories exist, so an exact-width neighbour cannot satisfy the assertion — plus a fast-check property over arbitrary distinct leading runs. The property is stated as an invariant on the result (every returned directory's leading run IS the query) rather than an expected list, so it covers primary and fallback matches alike and cannot be satisfied by reimplementing the selection in the test. Both fail when the fallback regex is degraded to a prefix match. * fix(#2528): route the remaining eight consumers through matchPhaseDirs phaseTokenMatches had eight consumers left that each rebuilt the directory selection around it by hand: phases-list, next-decimal, phase-remove, the W021 milestone-consistency check, schema-drift, the init-manager overview, milestone-complete's disk check, and roadmap analyze. Every one of them reproduced the reported symptom in full after the tokenizer was fixed. None of them derives a displayed phase number from the matched directory, so none needs phaseNumberForMatch; the change at each site is the selection and nothing else. matchPhaseDirs filters without reordering, so matches[0] reproduces the prior .find() choice wherever the old code resolved at all. phaseTokenMatches now has no call sites outside phase-id.cts. It stays exported as the primitive matchPhaseDirs is built from and as a pinned canonical surface, but no consumer reaches past the owner to it. * test(#2528): extend the parity gate to the migrated consumers Each of the eight is observed through the surface a user sees, not through the matcher, with a no-directory control so the assertions cannot be satisfied by a consumer that resolves unconditionally. init-manager and roadmap-analyze are additionally asserted to agree with each other. * refactor(#2528): own the case-flexible phase grammar and the leading-digit-run fragment validate.cts derived its case-flexible regex sources by running `replaceAll('A-Z', 'A-Za-z')` over two constants exported by phase-id.cts. That passes lint-phase-id-drift.cjs — there is no literal copy of the grammar — but it depends on the owner rendering that exact substring. The day phase-id.cts expresses the same class any other way the replaceAll silently no-ops and validate.cts narrows to uppercase-only. The failure mode is a NON-match, so nothing throws and no uppercase-only fixture notices. Both variants are now derived once, beside the sources they widen, and imported. Also names the leading digit run the bare-integer fallback selects on. It was spelled `/^(\d+)(?:-|$)/` where the fallback filters and `/^\d+/` where phaseNumberForMatch reads the number back off the winner; selecting on one run and displaying another would resolve a directory and then label it with a number that never matched it. * fix(#2528): refuse to remove a phase when two directories claim its number cmdPhaseRemove was the only migrated site taking matches[0] with no multi-match guard. Every sibling resolution path returns ambiguous_matches and refuses to choose; this one is the DESTRUCTIVE path, so choosing silently is strictly worse than anywhere else. With 05-80-20-a and 05-90-till-late on disk, `phase remove 5 --force` deleted one of them and renumbered every phase after it — where the base resolved nothing, deleted nothing, and the corpus in tests/phase-resolution-parity.test.cjs already declared that exact input ambiguous. The refusal is emitted before any file is touched and carries both candidates. CONSUMER_SCENARIOS could not express the case — every row is binary, resolving to one directory or to none — so the gate gains a dedicated ambiguous test. It asserts on the filesystem, not only on the reported directory_deleted: a null printed after an rmSync would satisfy every other check. * fix(#2528): pair digit-leading phase directories with their roadmap phase in validate health W006/W007 are the ninth site of this bug class and the one a `phaseTokenMatches` grep could never surface: they resolve roadmap↔disk by intersecting TOKEN SETS, which is a dir→token labelling rather than the query→dir selection matchPhaseDirs owns. On the canonical fixture the label is wrong in both directions at once, so `validate health` reported "Phase 5 in ROADMAP.md but no directory on disk" AND "Phase 05-80-20 exists on disk but not in ROADMAP.md" for the same directory. collectDiskPhases now keeps the directory names behind each token, so W006 can ask the canonical matcher whether a roadmap phase resolves to a real directory, and W007 — which iterates directories and therefore has no query to resolve — gets the inverse mapping it never had: a directory is claimed when some roadmap phase resolves to it. Both checks are additive: the token intersection still decides every shape it already decided, and the resolution can only REMOVE a warning. The regression test carries controls in the other direction — a roadmap phase with no directory must still raise W006, an unclaimed directory must still raise W007 — so it cannot be satisfied by a check that stopped reporting. * docs(#2528): state and pin the directory-side scope of the bare-integer fallback The matchPhaseDirs docblock claimed deep-decomposition lookups were untouched. That is true of the QUERY side only — no non-bare query enters the fallback — but the DIRECTORY side is what changed classification: a bare `5` now reaches a lone `05-01-auth` and resolves it (phase_number "05", phase_name "01-auth") where the base found nothing. The widening is irreducible from directory names alone. `05-01-auth` (sub-phase 5.1) and `30-12-factor-refactor` (phase 30 named "12-Factor Refactor") are the same `NN-NN-<slug>` shape, and the discriminator that would separate them — "is the second segment a valid decimal sub-phase" — accepts `5.1` and `30.12` equally. Any rule strong enough to exclude the first excludes the second, which is the defect #2528 exists to fix. So the tie is broken in favour of resolving, the docblock now says so, and the consequence is bounded where it matters: two such directories are two matches, and every caller (including phase remove) refuses to choose. Pins both directions, since nothing observed the directory side before. * fix(#2528): count surviving phases by identity in phase remove's STATE resync #2640 landed on `next` while this branch was open. Its STATE.md phase-count resync re-derives "which directory was removed" from the query with `phaseTokenMatches`, which is the tenth site of this issue's defect: the bare-integer fallback resolves `05-80-20-cleanup` for query `5`, but the token predicate does not, so the just-deleted directory is counted as still present and the written `Total Phases` is one too high. `targetDir` already IS the directory that was removed, and the block is gated on it being non-null, so identity answers the question exactly — which is also what the comment above the filter already claimed it did. This keeps `phaseTokenMatches` out of `phase.cts` rather than re-importing it to satisfy one call site: the module's public surface should not grow for a question that does not need re-derivation. Pinned in the parity gate with a control on a directory the tokenizer reads correctly, so the assertion is about the digit-leading shape and not about the counting rule changing for everything. * fix(#2528): let the resolution layer own the digit-leading slug family alone The tokenizer rewind this fix carried — pop the last absorbed continuation when the segment that stopped the scan is a bare single digit — reads "10-24-7-autonomy" (phase 10 named "24/7 Autonomy") correctly and silently re-reads "10-24-7-zip" (sub-phase 10.24 named "7-Zip Integration") from "10-24" to "10". The two names are string-identical in shape, so no local signal separates them; the rule traded the reported ambiguity for the symmetric one a level down, on a 15-caller chokepoint whose output also feeds query-less derivations (STATE.md phase counts, W007, the #2562 key surface). A well-formed sub-phase directory became unresolvable by its own id — the very symptom #2528 was filed about. It also bought nothing. The bare-integer fallback in matchPhaseDirs already resolves "10-24-7-autonomy" for query "10" whatever the token is: no primary match, bare query, leading digit run "10". The reported case was covered twice, by two rules, and the two disagreed about the case nobody reported. So the rewind is removed rather than narrowed, in the tokenizer and in the five surfaces kept in lockstep with it (BRACKET_PHASE_TOKEN_SOURCE, PHASE_TOKEN_FROM_DIR_RE, canonicalPlanStem's pair grammar and its collision branch, roadmap-parser's numericRe, extractCanonicalPlanId), together with the SINGLE_DIGIT_RUN_SEGMENT_SOURCE owner constant they shared. Disambiguation now lives only where a QUERY exists to disambiguate against, which is the same bounded mechanism the "05-80-20-cleanup" shape already used. Measured, not argued: over 29800 generated directory names, extractPhaseToken is byte-identical to `next` on every input except the lowercase-continuation class ("01-20a", "05-80-20-25abc") — a rule about the segment itself, not a guess about its neighbour. Both readings now stay reachable by their own ids: matchPhaseDirs(['10-24-7-autonomy'], '10') -> the dir (fallback) matchPhaseDirs(['10-24-7-zip'], '10') -> the dir (fallback) matchPhaseDirs(['10-24-7-zip'], '10-24') -> the dir (primary) * test(#2528): pin the one-continuation boundary the rewind had no coverage for The regressing shape was invisible to the suite by construction, not by luck: the deep-rewind property built its cases from `continuationArb` with `minLength: 2`, so it never exercised the single-continuation case — exactly one genuine sub-phase level before a digit-leading slug — and every hand-written fixture used the ambiguous shape only where "phase-plus-slug" was the intended reading. `continuationArb` is now `minLength: 1` and the property states the invariant instead of the old rule: for any prefix, phase, 1-5 continuations and any one-digit terminator, the token equals the FULL continuation run on both the imperative and the regex surface, and `matchPhaseDirs([dir], token)` returns that dir. That third assertion is the one that catches the class on its own — the old behaviour made a well-formed directory unresolvable by its own id, which is a property, not a fixture. Around it: "10-24-7-zip" and "10-24-3d-printer" now sit beside "10-24-7-autonomy" everywhere the family is pinned, so the two readings can never diverge again; the 24/7 metamorphic property asserts the RESOLUTION result rather than the token (the token is precisely the part no surface may decide); the end-to-end parity corpus gains "a sub-phase with a digit-leading slug resolves by its full id" across all four resolution paths; and the milestone-scoping residual is pinned in three directions rather than left to prose. Mutation: re-inserting the rewind and rebuilding turns 9 tests red, the `minLength: 1` property first, and nothing else. Build success checked separately (build:lib reports 0 `error TS`), so the mutation reached the artifact under test. * test(#2528): pin the #2946 guard against digit-leading phase directories The #2946 fix makes the milestone-complete unstarted-phase guard run unconditionally, so whether it fires now rides entirely on the directory-resolution owner this PR replaces. Two cases, both with STATE.md carrying no `milestone:` field so the #2946 path is the one exercised: - ROADMAP Phase 5, disk `05-80-20-cleanup` → guard must stay silent. RED on next (fail-closed: the guard blocks a legitimate one-way-door operation because phaseTokenMatches resolves neither 05 nor 80 for that directory). - ROADMAP Phase 80, same directory → guard must still fire. Green on both sides; it pins the fail-open direction against a future widening of the matcher. * fix(#3175): stop the injection scanner reading RegExp.exec as code execution The apostrophe fix in 27aa40f6 replaced ["\x27] with a real ["'] class. The old class never contained an apostrophe at all (POSIX bracket expressions do not honour backslash escapes, so it was the set ", \, x, 2, 7), so only exec(" matched. Single-quoted method calls now match for the first time, and RegExp.prototype.exec takes a subject string, not code: any PR touching a file that tests a regex goes red. On next, six files match the scanner's own pattern across 16 method calls. A plain [^[:alnum:]] boundary cannot separate the two forms because . is not alnum, so exec gets [^[:alnum:].] and the command-execution vector moves to a dedicated member-call pattern. Bare exec('rm -rf /'), cp.exec(...) and child_process.exec(...) all still fire. Mutation: reverting the boundary reds 1 test and only it; removing the member-call pattern reds the 2 non-weakening tests and only them. Co-Authored-By: Claude <noreply@anthropic.com> * fix(#2528): keep exec( detection receiver-blind, allowlist the two grammar suites The left boundary [^[:alnum:].] added in 58a7b560 excluded a preceding dot, which dropped every member-position .exec('…') from the scanner. The follow-up receiver pattern only restored three literal spellings (child_process, childProcess, cp), so require('child_process').exec('…') — the most common Node spelling of the vector this pattern exists to catch — became invisible, along with any opaque receiver (conn.exec, shelljs.exec). Revert the pattern to its receiver-blind form and handle the RegExp.prototype .exec false positive where the script already handles this class: per-file ALLOWLIST entries for the two phase-token grammar suites. Mutation-checked — removing the two entries reds exactly those two files and nothing else. The four assertions written around the old patterns are replaced by a table-driven set covering all six spellings, including the three the narrowed pattern silently lost. Co-Authored-By: Claude <noreply@anthropic.com> * docs(#2528): pin the three undeclared grammar edges, correct the ambiguity claim Review round 10 asked for declaration, not behavior change, on four items. All four have zero production consumers or preserve their caller's prior rule, so each is pinned as a test or corrected in prose rather than reverted. - BRACKET_PHASE_TOKEN_SOURCE: the (?=-|$) terminator is what keeps the bracket read path in step with the other surfaces, and it costs the display shapes (`05.03: Title`, `12A: X`, `05.03]` no longer tokenize). Pinned so widening the terminator class is a deliberate act rather than a lookahead deletion. - canonicalPlanStem: uppercase plan suffixes still strip, lowercase and dotted sub-plans now fall through. Dead export; pinned as a decision on record. - getMilestonePhaseFilter: `12A-01-foo` now yields `12A-01`, matching what `12-01-foo` has always yielded. The letter suffix was the only reason a sub-phase directory folded into its parent phase's milestone window; the two shapes now agree. Not named in the review — found auditing the same commit. - matchPhaseDirs docblock claimed every caller refuses on multi-match. Four do; five take matches[0]. Replaced the claim with the actual two-tier policy and the honest caveat that the bare fallback makes multi-match newly reachable for queries that previously found nothing. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-08-12 20:45:04 -04:00
Rezolv	e87fb409ee	enhance(#2573): stamp STATE.md with its commit and surface a freshness hint (#2622) ... * enhance(#2573): stamp STATE.md with its commit and surface a commit-age freshness hint Adds a `state_head` stamp to STATE.md and derives a tri-state commit-age freshness proxy (state_commits_behind / state_commit_stale) through state.cjs's readStateHeadFreshness, surfaced on smart-entry signals and as health W024. The proxy is advisory: classify() deliberately does NOT consume it (ADR-1787 locks the classification/routing boundary — a signal, not a route). Composes with #3099 and #1882 (both merged to next after this branch): the commit-age proxy reads `state_head` while the LAST_ACTIVITY_UNPARSEABLE diagnostic reads `last_activity` — two different fields, not "two staleness signals on one field." A new regression test asserts a STATE.md carrying both an unparseable last_activity AND a valid state_head resolves each independently (diagnostic fires once; freshness reads state_head, commits_behind 0). Rebased onto next (flattened): resolved the add/add conflicts in src/smart-entry.cts (kept both the #2573 freshness import/derivation and the #3099 diagnostic import/call) and tests/smart-entry.unit.test.cjs (kept both describe blocks). Drift-ack for health.md's W024 row is unchanged (12348 B). Tests: smart-entry 62, state/state-transition/health/verify 639, all pass. * chore(#2573): allowlist health-validation test in the prompt-injection scan The scanner's `exec('` code-execution pattern matches the benign `re.exec('<phase-id>')` RegExp method calls in the phase-ID grammar tests (pre-existing: 16 such calls on next, this PR adds none). The file entered the diff-mode scan's changed-file set only because #2573's W024 state_head assertions touch it. Allowlist it alongside the other test files that carry pattern-matching content as data (same DEFECT.PROMPT-INJECTION-SCAN-COLLISION class). Scanner self-test 38/0; diff scan 14 files, 0 findings.	2026-08-11 17:10:23 -04:00
Tom Boucher	27aa40f65e	fix(#3023): stage pi's shared hook bundle outside pi's reserved hooks/ directory (#3175) ... * test(#3023): failing-first guard — pi must not stage hooks in its reserved dir pi reserves <configDir>/hooks as its deprecated extension location and warns on every startup when it exists. Assert a pi install stages the shared hook bundle under gsd-hooks/ instead, manifests it there, and never creates hooks/. Also adds pi to the local-scope dir table in install-shared.cjs: pi was in RUNTIME_META but not LOCAL_DIR_NAME, so scope:'local' resolved path.join(root, undefined) and no local pi install could be exercised. Fails before the fix. Verified via the remote runner. * fix(#3023): stage pi's shared hook bundle outside pi's reserved hooks/ dir pi reserves <configDir>/hooks as its now-deprecated extension location and warns on every startup when that directory merely exists — checkDeprecatedExtensionDirs() guards the warning with a bare existsSync(), unlike its tools/ sibling. GSD staged its shared hook bundle exactly there, and pi's advised remediation (move it to extensions/) would break the adapter's paths and expose GSD's .js helpers to pi's extension auto-discovery. The bundle directory name is now runtime-descriptor-driven: hostBehaviors .sharedHooksDirName, defaulting to 'hooks' so all 18 other runtimes are byte-identical. pi sets 'gsd-hooks'. The name is validated as a single path segment — separators, dot-only segments, trailing dots, absolute paths, NUL, and Windows reserved device names all fall back to the default, because the value is joined onto a user's config root and written to. Renamed in place rather than relocated: hook scripts resolve siblings via __dirname/.., so a depth change would silently break them. - install / uninstall / manifest sites all read the resolved name - pi/gsd.cjs probes gsd-hooks then hooks, so dev checkouts and half-upgraded trees still resolve; the never-throws contract is preserved - new migration 009 retires the legacy pi hooks/ dir on upgrade, using a new non-recursive remove-empty-dir engine primitive (rmdirSync only, symlink-refusing, containment-guarded); ADR-0008 amended accordingly - fixes two latent name-dependencies the rename exposed: the stale-hook scan and the injection scanner's self-exclusion both hardcoded 'hooks' Verified on the remote runner. Closes #3023 * fix(#3023): close review findings and align emitted provenance with the rename Adversarial review found two defects, and the remote runner found four failure clusters. All fixed here. Review BLOCKER — detect-custom-files was blind to the renamed bundle. GSD_PREFIX_MANAGED_DIRS in gsd-tools.cjs hardcoded 'hooks', so for pi the whole gsd-hooks/ tree was invisible to the custom-file scan and user-added files there were never backed up before the next update's clean-install wipe. The dir set now resolves via the .gsd-runtime marker plus the shipped capability registry (never bin/install.js, which is not shipped into installed trees), and falls back to scanning every known candidate when the runtime cannot be determined — over-scanning is safe, under-scanning is the data loss. Review MAJOR — the pi adapter bound to an empty bundle. resolveSharedHooksDir accepted any directory, so an interrupted install left gsd-hooks/ winning over a fully-staged legacy hooks/ and every hook silently no-opped. A candidate now qualifies only if it is non-empty. Remote-runner clusters: - emitted-provenance had no rule for the gsd-hooks/ family; added two pi-scoped rules pointing at the same sources the existing hooks/ rules use. The table is total, so an unattributed family is a hard failure by design. - pi tests in install-minimal-hooks and the install integration suite asserted the old layout; updated to derive the dir name from the descriptor rather than hardcoding either name. - 19 unrelated-looking failures on node22 only were a leaked fs mock: t.after() runs in registration order, cleanup was registered before mock.restoreAll(), and node22's JS rimraf calls the public fs.rmdirSync while node24's native path does not — so the EACCES stub leaked process-wide on one lane. Restore now runs first. Verified on the remote runner. * fix(#3023): honor PI_CODING_AGENT_DIR, ack the rename ripple, fix expandTilde pi resolves its agent dir as PI_CODING_AGENT_DIR ?? ~/<CONFIG_DIR_NAME>/agent (packages/coding-agent/src/config.ts). GSD's pi descriptor declared an empty configHome.env, so a user with that variable set had GSD installed where pi never looks. Added the env name; the dot-home-nested resolver already handled the override, so no resolver logic changed. Also fixes expandTilde in the shared runtime-homes resolver, found while adding that: it hardcoded os.homedir() and ignored the opts.home every caller threads, so EVERY runtime's tilde-valued env override (claude, antigravity, windsurf, pi) silently resolved against the real home. That is a correctness bug and a test-escape hazard — a sandboxed test asserting on a tilde override reached the developer's actual home directory. Now threaded through every branch; behavior with no injected home is unchanged. Adds the emitted-drift ack fragment for the 58 pi paths whose emitted location moved with the rename. The provenance rules satisfy the totality gate; the differential gate needs the ack because the hook sources are byte-unchanged — only the installer's target directory moved. The two hook files this branch genuinely edits stay attributed and are not double-acked. Note on piConfig.configDir: it is read from pi's OWN installed package.json (getPackageDir walks up from pi's __dirname), alongside piConfig.name — a white-label setting for a redistributed pi fork, not a per-project user setting. Documented accordingly rather than treated as an unsupported override. Verified on the remote runner. * fix(#3023): reject blank env overrides, pin adapter/descriptor parity Three review findings, all fixed. A whitespace-only config-dir override was accepted verbatim: the guard was `if (val)`, falsy only for the empty string, so PI_CODING_AGENT_DIR=' ' resolved to a literal three-space directory name instead of falling back to the descriptor default. Fixed across every env-consuming branch — dot-home, dot-home-nested, all three xdg steps, and generic-agents-root — not just pi's. Non-blank values are still never trimmed, so '~/My Agent Dir' keeps working. pi/gsd.cjs's probe list and the descriptor were two independent sources of truth for the bundle directory name; a future rename would have desynced them silently and left every pi hook quiet with no error. The probe list stays deliberate — it must resolve in a dev checkout and a half-upgraded tree, where the registry's answer would be wrong — so this adds the parity assertion the repo's generative-fix-divergence rule calls for: the descriptor value must be the FIRST candidate, and the default must remain present. Changeset body rewritten to cover the two later user-facing fixes it had not caught up with. Verified on the remote runner. * chore(#3023): backfill changeset PR number * fix(#3023): anchor injection-scan patterns and fix a macOS detection hole CI's security job flagged CONTEXT.md:124 — pre-existing prose reading 'not the same fact as a genuinely empty or absent one'. The match was the 'act as a' INSIDE 'f-act as a': the pattern had no left word boundary, so any word ending in act tripped it (fact, impact, contract, artifact, interact, redact, abstract). My four-line CONTEXT.md edit dragged the latent false positive into this PR because the scan is diff-scoped by file but reads whole files. Anchored with (^|[^[:alnum:]]) rather than rewording maintainer-owned prose, which would have left the class alive for the next PR touching any file saying 'fact as a'. Auditing the rest of the list for the same class surfaced a real detection hole: the eval/exec/Function patterns matched a quote via \x27, a GNU-grep-only hex escape. BSD/macOS grep reads it as four literal characters, so single-quoted eval('...')/exec('...') payloads were NEVER detected there while passing on GNU-grep CI. Replaced with a literal apostrophe class. Boundaries were added only where a real word-suffix collision exists; exec, jailbreak, developer mode and the role-manipulation family were audited and deliberately left unanchored. 22 new cases cover both directions — the false positives now scan clean, and every real payload still fires, including the quote/punctuation/start-of-line boundary forms. Also builds this branch's injection test fixture at runtime instead of carrying the literal phrase, so the payload keeps its teeth without tripping the scan. Verified on the remote runner. --------- Co-authored-by: sim <sim@local>	2026-08-07 13:41:21 -04:00
0xdhx	a8b40fa53f	fix(#2547): fail closed on crashing and path-shadowing Kimi payloads (#2595) ... * fix(#2547): fail closed on a malformed Kimi edit list in normalizeKimiPayload `normalizeKimiPayload` rebuilt old_string/new_string with `String(e.old ?? '')`. `??` guards the value, not the dereference, so a nullish entry in a Kimi `edit` list threw a TypeError at the top of the handler, before any tool dispatch. Each guard's outer `catch { process.exit(0) }` swallowed that crash and emitted the same exit code as "nothing to report" — turning a should-BLOCK call into a silent allow. Two hard blocks were bypassable: * gsd-worktree-path-guard's cross-git-root write block (#260) — a StrReplaceFile write whose path resolves to a different git root is correctly blocked with a well-formed edit list, and silently allowed with `edit: [null]`. * gsd-workflow-guard's force-add block on agent-* branches — a Shell payload carrying a spurious `edit: [null]` field walks past it. The Bash path never reads `edit`; the field only has to be present to trigger the crash. Fixed with `e?.old` / `e?.new`, landed identically in all five copies so tests/kimi-guard-normalization-parity.test.cjs's byte-identity assertion still holds. The crash boundary is nullish specifically, not "non-object": `('x').old` and `(7).old` are legal reads yielding undefined, so string/number entries never threw. Both are kept as controls proving the fix did not change their behaviour. Regression coverage is folded into the owning suites per CONTRIBUTING.md (no new bug-* files). Negative-controlled: the nullish cases exit 0 against pre-fix guards and exit 2 after, with positive controls (the equivalent well-formed payload blocks) and negative controls (in-worktree writes and benign commands still pass) alongside. Refs #2547 * test(#2547): exercise the production Kimi payload shape in read-guard tests The `#2304: Kimi tool vocabulary engages the read guard` cases send payloads with no `session_id`, and runHook injects none. A live Kimi turn always carries one — kimi-cli's hooks/events.py `_base()` sets it unconditionally, and soul/kimisoul.py calls `set_session_id()` at the top of every turn before tool dispatch, so the ContextVar's `default=""` never reaches a tool call. gsd-read-guard treats any non-empty `data.session_id` as "Claude Code already enforces read-before-edit, skip" (#2520). So the advisory those tests assert fires only for a shape production never sends: the tests were green, and the guard was dormant on Kimi. A sibling #2520 case in the same file asserts the skip when `session_id` IS present — both passed, and the production shape hits the skip. Two changes, test-validity only: * Retitle the #2304 block to say what it proves — the tool VOCABULARY is normalized through to the Write/Edit branch — with a comment warning not to read it as production evidence. * Add a #2547 block asserting behaviour against the production shape (session_id populated), including a case that pins the delta directly: the same payload fires without session_id and is silent with it. The #2547 block characterizes a known gap; it does not endorse it. Redesigning how the guard discriminates runtimes is explicitly out of scope for #2547. If a later change makes the advisory fire on Kimi these tests are supposed to fail — update them then rather than dropping the coverage. Refs #2547 * docs(#2547): scope the Kimi guard-engagement claim to what Kimi enforces #2518 engaged the guards' Kimi matchers and the release notes describe the result as "All seven guard hooks now engage on Kimi", singling out the prompt-injection read scanner as "the security-relevant guard" taken "from silently dormant to engaged". That is not achievable for the scanner at the emit layer. gsd-read-injection-scanner.js is a PostToolUse hook, and kimi-cli's dispatch never inspects PostToolUse hook results: src/kimi_cli/soul/ toolset.py awaits PreToolUse and honours `result.action == "block"`, but fires PostToolUse via asyncio.create_task() and returns the ToolResult without awaiting it — the done_callback only retrieves the task's own exception. So no output shape the scanner emits can block or flag a Kimi tool call, and `security.injection_blocking` cannot take effect there. Reshaping the scanner's output would not change this; the enforcement gap is in kimi-cli's PostToolUse handling, which is out of scope here. This corrects the claim rather than the code — there is no gsd-core emit fix that would make it true: * .changeset/2304-kimi-guard-tool-name.md — the fragment is unreleased, so it would otherwise ship this as a CHANGELOG security claim. Headline narrowed to "normalize Kimi's payload shape" and a scope paragraph added naming what actually blocks on Kimi (the two PreToolUse blocks) versus what cannot. * docs/migration/kimi-to-kimi-code.md — the scanner was listed under "Every GSD `PreToolUse` guard"; it is PostToolUse. Corrected, and the "What about the dormant guards?" section now splits enforceable from not-enforceable instead of saying Phase 0 "fixed all seven". * hooks/gsd-read-injection-scanner.js — the same scope note in the file's own Kimi rationale comment, where the next contributor to touch the normalization will actually read it. Comment only; the shared normalization block is untouched and byte-identity still holds. Refs #2547 * chore(#2547): regenerate golden install-parity fixtures for the guard fix The golden install-parity fixtures record a content hash per installed file, so changing the five guard hooks changes their hashes across every runtime's fixture. Regenerated with the full sweep (build, gen:golden, size:baseline) rather than a single generator — running gen:golden alone leaves tests/workflow-size-baseline.json stale and loses CI jobs to a regeneration that looked complete. The size baselines came out unchanged (no workflow or agent bodies touched) and the hash delta is confined to exactly the five guards: gsd-prompt-guard, gsd-read-guard, gsd-read-injection-scanner, gsd-workflow-guard, gsd-worktree-path-guard. Refs #2547 * fix(#2547): guard the String() coercion in normalizeKimiPayload too Found by adversarial review of the first commit, then reproduced against pristine next: `e?.old` closes the nullish dereference but leaves a second route to the same crash-to-allow. `{"toString": null}` is valid JSON, and coercing it throws `TypeError: Cannot convert object to primitive value` — so an edit entry that IS a well-formed object still crashes normalization, still lands in the outer `catch { process.exit(0) }`, and still downgrades a should-BLOCK call to a silent allow. Confirmed on both hard blocks: {"tool_name":"Shell","tool_input":{ "command":"git add -f secret.env", "edit":[{"old":{"toString":null},"new":"x"}]}} -> exit 0 (was) {"tool_name":"StrReplaceFile","tool_input":{ "path":"<main-repo>/src/index.ts", "edit":[{"old":{"toString":null},"new":"x"}]}} -> exit 0 (was) Both exit 2 now. The coercion is wrapped rather than replaced with a `typeof === 'string'` test on purpose. Degrading only the non-coercible entry keeps stringification identical for every value that CAN coerce — numbers, arrays, plain objects — which matters because gsd-prompt-guard scans new_string for injection patterns, and a `typeof` test would silently stop scanning content that reaches that scan today (e.g. `new: ["ignore all previous instructions"]` currently stringifies and is scanned). Verified: zero behaviour change across string, number, bool, null, array-of-strings, nested array, plain object and `__proto__`-keyed input; only the throwing case changes, from crash to ''. Regression cases are negative-controlled against the previous commit: the four new coercion-trap tests fail with only the `e?.old` fix in place and pass with this one. Refs #2547 * chore(#2547): cover the String() coercion vector in the changeset The release note described only the nullish-dereference route. Both routes reach the same fail-open, so both belong in the changelog entry, along with why the coercion is wrapped rather than type-tested. Refs #2547 * chore(#2547): point the changeset fragment at the real PR number The fragment has to exist before `gh pr create` runs, so it carried the issue number as a placeholder. Corrected to 2595 now that the PR is open. Refs #2547 * fix(#2547): make Kimi's `path` authoritative over a model-supplied `file_path` normalizeKimiPayload copied Kimi's `path` into `file_path` only when `file_path === undefined`, so any `file_path` the model chose to include won outright. Every guard reads `file_path`; kimi-cli executes on `path`. The guard therefore inspected one file while the write landed on another. This bypass needs no crash. A payload pairing a cross-root `path` with a spurious `file_path: ""` left gsd-worktree-path-guard reading an empty string and exiting 0, while the identical write without the extra key blocked — the same cross-root write the #260 block exists to catch. The shadowing also preserved a non-string `file_path` (`[]`), which threw inside that guard's path.isAbsolute() and reached its outer `catch { process.exit(0) }`: the same crash-to-allow the rest of #2547 closes, reached through the guard's own read rather than through normalization. Reachability is not speculative. kimi-cli's soul/toolset.py json-parses the model's raw tool arguments and passes that dict verbatim as tool_input to PreToolUse, performing typed validation only later inside tool.call() — after the hook has already decided. So the model controls extra keys in tool_input at the moment the guard runs. kimi-cli's file tools carry no `file_path` field at all (src/kimi_cli/tools/file/write.py, replace.py), so a `file_path` in a Kimi payload is always model-supplied. `path` now wins outright. Overwriting can only ever narrow what a guard inspects to the path that will actually be written, so it cannot under-block. Normalization returns early for non-Kimi tool names, so the native Claude Code contract (file_path governs) is untouched. Landed identically across all five inlined copies; the byte-identity assertion in tests/kimi-guard-normalization-parity.test.cjs enforces that. * test(#2547): cover the file_path-shadowing bypass in the #260 guard suite Four cases, each exiting 0 (bypass) against the pre-fix guards: a spurious empty-string file_path, an in-worktree decoy file_path, and non-string file_path values (array and object) that additionally crashed path.isAbsolute() into the outer catch. Two controls that are not bypass cases and matter as much: - an in-worktree write carrying a cross-root DECOY file_path must still exit 0. Pre-fix this blocked, because the decoy won; the guard now follows the path kimi-cli executes on in both directions, so the fix narrows what is inspected without over-blocking. - a native Claude Edit (no `path` field) must still block on file_path alone. normalizeKimiPayload returns early for non-Kimi tool names, and this pins that the non-Kimi contract did not move. It passes both pre- and post-fix by design. Negative-controlled: run against the pre-fix hooks, the four bypass cases and the decoy control fail, and the native-Claude control passes. * test(#2547): back the totality claim with property tests over fc.anything() This PR claims the fix "makes normalization total over the inputs JSON can express" — a for-all guarantee — while the tests backing it are example-based, each shape added reactively after a crash was found by hand (the String() coercion trap was itself found by adversarial review after the first commit shipped). Example-based tests cannot substantiate a for-all claim; they record the counterexamples someone happened to think of. Four properties over fc.anything(), which is exactly the JSON-expressible domain the claim names: (a) totality over any tool_input (b) totality over any edit list — the crash surface both #2547 fixes targeted (c) `path` always wins over any model-supplied `file_path` (the review blocker invariant: a guard reading file_path can never be aimed at a file other than the one kimi-cli writes) (d) a non-Kimi tool_name passes through untouched — the native Claude contract normalizeKimiPayload is inlined per hook with no runtime binding, so there is nothing to require. The block is extracted from hook source and evaluated via the SAME extraction contract kimi-guard-normalization-parity.test.cjs uses, so a source edit that breaks one breaks both instead of silently testing a stale block. An extraction floor test fails loudly if the extraction yields a no-op. Non-vacuous, and checked rather than assumed: against pristine pre-#2547 `next`, (a), (b) and (c) all FAIL and (d) passes. (a) needed the fix that makes it meaningful — a bare fc.anything() for tool_input passed even against the live defect, because arbitrary generation essentially never invents the `edit` key the crash lives behind, so the generator is biased onto the keys normalization actually reads and unioned back with unbiased input. * chore(#2547): cover the shadowing vector in the changeset and regen goldens Golden install-parity churn is hash-only, on exactly the five hook files this round changed. gsd-phase-boundary.sh is deliberately unchanged. * test(#2547): make the property test able to kill the coercion mutant Review Major 1: the generative test added to stop the NEXT counterexample could not kill the one it was written for. Reproduced the reviewer's matrix independently — against the shipped generator, a mutant reverting `editText` to the unguarded `String(v ?? '')` passed all four properties. Cause, confirmed by measurement: the edit-array ENTRIES were bare `fc.anything()`, which essentially never invents an `old`/`new` key, so `e?.old` was always undefined and `String(undefined ?? '')` never coerced anything. That is the same vacuity the file's own comment describes one level up, reproduced one level down. The review's prescribed fix — bias the entry onto `{old, new}` — is necessary but NOT sufficient, and this is the part worth recording: measured over 20,000 draws, bare `fc.anything()` yields a non-coercible value 3 times (0.015%). At `numRuns: 200` an `old` key holding a hostile value essentially never co-occurs, and the mutant survives the entry bias too. Both levels need biasing — the entry onto the keys normalization reads, and the VALUE onto the shape that actually throws. `{"toString": <non-function>}` is that shape and stays inside the "JSON-expressible" domain the claim names (JSON.parse produces it verbatim); `fc.anything({withNullPrototype: true})` would also kill the mutant but widens the domain past what the PR asserts, so it is not used. Verified: M1 now dies at every seed tried (1/7/42/99/4242/31337, failing within 3-31 cases) while HEAD stays green at all of them. Also closes three coverage gaps the review listed as nits — properties (e) totality over any JSON value as the WHOLE payload, (f) the tool_output → tool_response mapping (including that an existing tool_response is not clobbered), and (g) an empty edit list reconstructing nothing. Property (e) required a one-line fix in the normalizer itself: `JSON.parse ('null')` is null, and null/primitive payloads threw on the `data.tool_name` read — falsifying the "total over the inputs JSON can express" claim. Harmless in practice (the throw landed in the same fail-open catch as the exit 0 it now takes deliberately), but the claim should be true as stated. Landed byte-identically across all five copies. Refs #2547 * fix(#2547): make old_string/new_string authoritative, not merely defaulted Review Major 2: the shadowing class the prior round's BLOCKER closed for `file_path` survived one field over. `old_string`/`new_string` were still reconstructed only `if (input.<field> === undefined)`, so a model-supplied value won. The argument for making `path` authoritative applies verbatim here. kimi-cli's StrReplaceFile schema is `path` + `edit` only (src/kimi_cli/tools/file/replace.py @ 4a550ef) and carries no `old_string`/`new_string` at all, so either key appearing in a Kimi payload is always model-supplied — exactly like `file_path`. Verified end-to-end against the reviewer's payload: a cross-root write carrying `new_string: ""` alongside an injected `edit[].new` left gsd-prompt-guard reading '' and returning at its `if (!content)` guard, so the injection advisory never fired and the reconstructed content was never scanned. `new_string: null` behaved identically. Negative-controlled: both produce empty output against pre-fix source and fire the advisory after. Chose unconditional reconstruction over the offered `typeof` alternative deliberately. A type test closes `""`/`null` but leaves the interesting case open — a benign NON-EMPTY decoy (`new_string: "chore: tidy"`) shadows just as effectively and passes any type test. The new suite includes that case specifically; it is what discriminates between the two candidate fixes. Also pins the kimi-cli SHA in the authoritative-path comment, as requested — it cited file names with no version while the issue pins 4a550ef. Landed byte-identically across all five inlined copies; the parity test's byte-identity assertion holds. Refs #2547 * fix(#2547): close the non-string file_path crash-to-allow at every read site Review Major 3: the crash-to-allow was closed only as a side effect of `path` masking the bad value, while the changeset read as though it were closed outright. Confirmed both of the review's reachability claims: `[]`/`{}`/`42` are truthy, survive the `!rawFilePath` early-out, and throw inside path.isAbsolute() into the outer `catch { process.exit(0) }`; and normalization returns early for native Claude Code payloads (KIMI_TOOL_NAMES has no 'Edit' entry), so `{"tool_name":"Edit","tool_input":{"file_path":[]}}` reached it untouched — this guard's original #260 surface. Reproduced on a real fixture: string cross-root path exits 2, the identical payload with `[]` or `{}` exits 0. Swept the class rather than the instance. Five more untyped read sites across four other hooks, each one line from a type-strict or method-dependent call. Census of what each can actually do: gsd-worktree-path-guard.js:173 BLOCKS -> live bypass (the review's finding) gsd-prompt-guard.js:128 scanner -> silenced the injection scan, the same outcome as Major 2 by another route; verified empirically gsd-workflow-guard.js:206 advisory only (its exit-2 is the Bash force-add path, which reads `command`, not `file_path`) gsd-read-guard.js:141 advisory only gsd-read-injection-scanner.js:213 advisory only gsd-windsurf-pre-write.js:75 ALREADY TYPED — the shape adopted here All six now read typed. The workflow-guard site keeps its truthiness fallback (`(typeof x === 'string' && x) || ...`) because a bare type test would let an empty `file_path` shortcut the `path` fallback. Also declares one swept hit NOT fixed: `gsd-workflow-guard.js:175` reads `command` untyped on a genuinely blocking path. Same shape, but not exploitable — unlike file_path/path there is no second field carrying the executable value, so a non-string command cannot smuggle a real `git add -f` past the block. Left alone rather than widen this PR into the Bash path. The regression gate is a SOURCE-level invariant, not a behavioural one, and that is deliberate: the fixed read and the crashing read are black-box identical — both end at exit 0, one via the catch and one via the early-out. A test asserting exit 0 on a non-string payload passes against the unfixed code, which is the same false-green the review flagged in the existing `['non-string file_path (array)', []]` cases. Repeating it one level up would be no better. tests/kimi-guard-typed-payload-reads.test.cjs fails if any hook regresses to an untyped read (negative-controlled: it reports all five pre-fix sites with correct file:line). The behavioural cases requested — non-string file_path with NO `path` key — are added to worktree-safety.test.cjs and labelled honestly as documenting the explicit fail-open rather than detecting a revert. Also states the relative-path premise (review Minor 5) at the early-out that depends on it: "always safe" holds only while every runtime reaching there resolves relative paths against the tool CWD. Claude Code satisfies it by requiring absolute paths; kimi-cli's resolution behaviour is NOT verified here and is recorded as an unverified premise rather than an asserted bypass. Refs #2547 * docs(#2547): correct the changeset's closed-claim and fold the misattributed note Review Major 3 also flagged the fragment: it said the non-string vector "threw inside that guard's path.isAbsolute() ... `path` now wins outright", which reads as closed when it was closed only conditionally. Rewritten to state what is now true — closed unconditionally at all six read sites — and extended with the Major 2 finding. Review Minor 6 (the #2547 scope note living in a `pr: 2518` fragment) turns out to understate the problem. Rendering the changelog and re-parsing it shows the note is not merely misattributed — it is DROPPED. serializeChangelog emits each fragment as a single `- ` bullet, and parseChangelog terminates a bullet at the first non-continuation line, so everything after a blank line is lost on re-parse. Audited all 44 fragments: exactly one was lossy — 2304-kimi-guard-tool-name.md, losing 656 of 1730 characters, i.e. precisely that second paragraph. Folding it into this PR's fragment fixes the attribution and the silent loss together; all 44 now round-trip losslessly. That same mechanism is why the remaining nit — reformat this fragment's ~2,000-character paragraph for readability — is NOT applied. A paragraph break or a bullet list would silently truncate the entry at the first blank line (verified for both). The single-paragraph form is load-bearing under the current serializer, not an authoring preference. Worth its own issue; noted in the PR thread rather than worked around here. Refs #2547 * chore(#2547): regenerate golden install parity after rebase onto next Rebased onto `next` @ 9138271b (the PR had gone BEHIND by 20 commits; the review's closing nit asked for it). The replay was CLEAN — no conflicts — and that is exactly why this commit exists. These fixtures are one key per installed file, so when the PR pins five hook entries and the base rewrites others', the two edits land on different lines of the same JSON. Git merges them silently and correctly AS TEXT while attesting nothing about whether the merged hashes are still valid. Verified rather than assumed: per-key equivalence against the old base showed the base had moved 22 of the 27 keys this PR pins in every runtime fixture, and tests/golden-install-parity.test.cjs failed on 10 runtimes immediately after the clean rebase. A push without this regen would have gone out red. Regenerated with `npm run build && npm run gen:golden` under a throwaway HOME/CLAUDE_CONFIG_DIR (the generators invoke the installer); live-profile canary clean before and after. Contamination check: every key differing from the base's committed fixture resolves to a file this PR actually touches — the five guard hooks, under both the `hooks/` and `.kimi/hooks/` install layouts, and nothing else. Derived from the PR's changed-file set rather than a feature-name filter, which is what would have mislabelled the registration surfaces. Size baselines re-checked and NOT regenerated: this PR moves no workflow or agent, and the base's own baselines are current (agent-size-budget, workflow-size-budget, workflow-size, update-size-baseline all green). Refs #2547 * chore(#2547): allowlist the field-shadowing security test in the injection scan The new regression suite tripped the repo's own prompt-injection scan — a test for the injection scanner setting off the injection scanner. The fixture has to be a real injection phrase for the test to assert anything: it is precisely the content gsd-prompt-guard must still scan once a model-supplied `new_string` can no longer shadow the reconstructed `edit[].new`. Weakening it to a benign string would make the suite vacuous. Allowlisted rather than obfuscated, because that is this repo's established convention for the class — tests/read-injection-scanner.security.test.cjs, tests/security-prompt-injection.security.test.cjs, tests/prompt-injection-scan.security.test.cjs and four others carry real payloads as test DATA and are listed for exactly this reason. Splitting the literal to dodge the grep would work but would make this one file inconsistent with its five peers and leave the next reader wondering why. Verified with the CI invocation itself (`scripts/prompt-injection-scan.sh --diff upstream/next`): 26 files scanned, 0 findings. The .cjs codebase scan does not cover tests/ and is unaffected (73 tests green). Refs #2547 --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-07-28 18:31:40 -04:00
Tom Boucher	f08b177215	feat(#1726): G1-G6 portability AST rules; fix all offenders; delete the ratchet (Phase 4) (#1731) ... Phase 4 of epic #1702. Closes #1726.	2026-06-25 18:24:39 -04:00
Tom Boucher	c57e0d56c2	feat(#1720): no-unguarded-nonportable-exec AST rule; retire the regex script (Phase 3) (#1723) ... Phase 3 of epic #1702. Closes #1720.	2026-06-25 16:25:48 -04:00
Alex V.	a63684c222	enhance(#1577): WebFetch/WebSearch injection isolation + opt-in blocking (#1585) ... * fix(#1577): isolate WebFetch/WebSearch ingress + opt-in injection blocking Split A of #1573 (security-critical). Scans WebFetch/WebSearch output (the largest untrusted channel) in gsd-read-injection-scanner; shared untrusted-input-boundary reference @-included by the 8 ingest agents (randomized per-wrap delimiters, in-prompt self-scan guard, task-anchoring); opt-in security.injection_blocking (default advisory — non-breaking). arXiv: 2506.05739 (PPA), 2507.15219 (PromptArmor), 2504.20472 (Referencing), 2503.00061 (defense-in-depth). * fix(#1577): address review — honest blocking docs, config key, ADR, property test, revert localized - A1: rewrote the opt-in-blocking doc + Security changeset honestly — the PostToolUse hook is a circuit-breaker (halts the agent's next step), NOT a redactor; it does not scrub content already in the transcript. The prompt-level data/instruction boundary is the primary control. - A2: registered security.injection_blocking in the config schema + defaults manifests (default false) + an e2e config-roundtrip test; the dotted setter writes the nested shape the hook reads. - A3: reverted the 4 hand-edited localized security-model.md (canonical EN only, per convention). - A5: ADR-1577 (untrusted-input boundary + opt-in blocking; redaction-vs-circuit-breaker rationale). - A6: property test — scanner never crashes / only emits valid JSON on unicode/large/malformed input. - Also: inventory (untrusted-input-boundary.md) + agent-size baseline (8 ingest agents) + drift-guard matcher update (Read -> Read|WebFetch|WebSearch). A7 (content<20 early-exit) left as the noted pre-existing follow-up. * fix(#1577): allowlist untrusted-input-boundary.md in injection-scan CI gate The new reference quotes injection phrases ('ignore previous instructions', 'you are now…') as examples agents must NOT comply with, tripping the repo's own prompt-injection-scan.sh diff gate (the standalone 'security' CI job, red on HEAD). Allowlist it alongside the other security docs (security-model.md, TEST-EXAMPLES.md) that legitimately demonstrate injection patterns. The JS scanner test doesn't scan references/, so only the shell gate needed it. Verified: scan --diff origin/next -> 0 findings; scanner JS test 15/15. * fix(#1577): cover AC #2's gsd-ui-researcher + gsd-assumptions-analyzer trek-e Major 1: the @-included set dropped two AC #2 agents. Restore them so no named web-ingress agent is uncovered, keeping the two justified additions (gsd-ai-researcher, gsd-domain-researcher). Final set = AC's 8 + 2 = 10. - gsd-ui-researcher carries the full WebSearch/WebFetch + MCP-fetch toolset. - gsd-assumptions-analyzer reads 5-15 codebase source files (external/source- document ingress per the boundary), though it has no web tools. INGEST_AGENTS in the isolation test now asserts all 10; size baselines regenerated (+60 bytes each, both well under the DEFAULT cap); changeset reworded 8 -> 10. Verified: untrusted-input-isolation 14/14; agent-size-budget 39/39. * docs(#1577): document security.injection_blocking + boundary seam trek-e Major 2 + Minor: - docs/CONFIGURATION.md: add the top-level security.injection_blocking key to the Full Schema and a Security Settings subsection, distinguishing it from the workflow.security_* namespace; honest circuit-breaker-not-redactor framing matching ADR-1577 / security-model. - CONTEXT.md: add the 'Untrusted-input boundary' seam glossary entry. Verified: lint:docs ok; config-field-docs + contributor-standards green. * test(#1577): make read-injection property test git-text, not binary trek-e nit (and more): the file embedded a raw U+FFFF AND a raw NUL byte as degenerate-edge inputs. The NUL is what actually made git classify it binary (git binary = NUL in first 8K). Replace both with text-safe escapes that keep the identical runtime values: '\\x00' and String.fromCodePoint(0xFFFF). File now diffs/blames line-by-line. Verified: property test 2/2; no NUL/raw-noncharacter bytes remain. * docs(#1577): align untrusted boundary docs Name all 10 ingress agents in INVENTORY/security-model and allowlist the intentional read-injection property corpus for the prompt-injection scanner. * docs(#1577): align ADR ingest agent count Update ADR-1577 from 8 to 10 ingest agents so it matches the actual boundary include set and the rest of the docs. --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-06-24 17:07:23 -04:00
Tom Boucher	c63fa35b0a	fix(#1615): allowlist windsurf-conversion.test.cjs in prompt-injection-scan ... The commandName validation tests legitimately contain real injection payloads (newline + system-role override phrases, fake [SYSTEM] tags, jailbreak strings) to prove the validator rejects them. The scanner cannot distinguish a test fixture asserting rejection from an actual injection attempt, so CI failed on the test that adds the security control. Added tests/windsurf-conversion.test.cjs to scripts/prompt-injection-scan.sh ALLOWLIST with a comment citing the defect class. Also added DEFECT.PROMPT-INJECTION-SCAN-COLLISION-WITH-TESTS to CONTEXT.md so the pattern is documented. Initial draft of that predicate ITSELF triggered the scanner (it quoted the literal injection phrase as an example) — reworded to use descriptive references ('scanner-matching payload', 'instruction-override phrase') since the scanner scans CONTEXT.md too. That meta-collision is now called out in the fix-forward and prevention subkeys.	2026-06-23 15:05:07 -04:00
Joe	e12a2abfd8	feat(#441): add /gsd-capture --list-seeds for seed listing and audit (#722) ... * feat(#441): add /gsd-capture --list-seeds for seed listing and audit Seeds (.planning/seeds/SEED-NNN-slug.md) could only be created (--seed), enriched (--enrich), or auto-surfaced at /gsd-new-milestone. There was no way to browse or audit parked seeds on demand. This adds a read-only listing, following the established --list → workflow pattern (per the approved scope on - gsd-tools `list-seeds [status]` (cmdListSeeds in src/commands.cts): scans the seeds dir, returns { count, seeds[], summary } JSON with each seed's id, slug, status, scope, trigger_when, planted, title. Optional case-insensitive status filter. User-controlled content is sanitized (sanitizeForDisplay) and every path validated (requireSafePath); read-only. Independent of audit.scanSeeds, which only returns unimplemented seeds for the milestone surface. - /gsd-capture --list-seeds routes to a new read-only list-seeds workflow that renders the seed table. Closes #441 * chore(#441): point changeset fragment at PR #722 * test(#441): allowlist list-seeds test in prompt-injection scan The test asserts that list-seeds neutralizes injection payloads (<system>, [INST]) embedded in seed content, so the fixtures legitimately contain those patterns — same as the sibling security tests already on the allowlist. * fix(#441): use canonical /gsd:capture colon form in list-seeds workflow Claude-facing source (commands/, agents/, gsd-core/workflows/, ...) must use the /gsd:<cmd> colon form per ADR/CONTEXT.md; the hyphen /gsd-<cmd> form is retired there (enforced by bug-2543-gsd-slash-namespace.test.cjs). The new list-seeds workflow used the hyphen form. * docs(#441): sync help full.md + INVENTORY for --list-seeds Adds the --list-seeds entry to the help reference (help/modes/full.md, per bug-2954 argument-hint↔help parity) and registers the new list-seeds workflow in docs/INVENTORY.md (88→89) and the generated INVENTORY-MANIFEST.json. * docs(#441): add --list-seeds how-to + drop phantom statuses Addresses CHANGES_REQUESTED on PR #722 (two documentation blockers): - USER-GUIDE.md Seeds section (how-to): extend the task to cover auditing parked seeds on demand via --list-seeds, including the status filter — kept task-oriented per Diataxis how-to mode. - CLI-TOOLS.md (reference): drop phantom statuses implemented|rejected from the list-seeds filter vocabulary; the system only produces dormant|active|triggered (src/audit.cts scanSeeds). Reference must be factually accurate and complete. * fix(#441): guard non-scalar status frontmatter in cmdListSeeds A seed with a bare `status:` line (extractFrontmatter yields {}) or a `status: [a, b]` value (yields an array) crashed the whole audit list: `(fm.status || 'dormant').toLowerCase()` throws a TypeError on a non-string. Coerce every frontmatter read through a `fmStr` helper (mirrors the existing `typeof fm.id === 'string'` guard), so a non-scalar status falls back to dormant and non-scalar scope/trigger_when/title can no longer leak a raw array/object into the JSON contract. Title is now capped symmetrically. Adds regression coverage for empty and array `status:` and non-scalar fields. Refs #441 * docs(#441): align list-seeds workflow status vocabulary The load_seeds step listed `implemented` as an example status filter, but the real seed vocabulary is dormant|active|triggered (src/audit.cts scanSeeds); `implemented` has no producer. Matches the earlier CLI-TOOLS.md correction. Refs #441 * refactor(#441): extract pure deriveSeedIdentity; match raw status in list-seeds Pull the seed_id/slug derivation out of cmdListSeeds into a pure, exported deriveSeedIdentity(stem, rawFmId) so the parsing contract can be property-tested in-process (review minor #1). No behavior change. Filter comparison now matches the raw lowercased status (both sides already normalized) instead of sanitizeForDisplay(status); sanitization is for output, not matching (review nit #3). * test(#441): add fast-check property coverage and count=1 boundary for list-seeds Adds tests/list-seeds.property.test.cjs with four fast-check properties over deriveSeedIdentity (never-throws, string-only contract, canonical id->seed_id/slug invariant, filename-prefix fallback) per RULESET.TESTS.property-based-testing (review minor #1). Adds an N==1 status-filter boundary case to list-seeds.test.cjs (review minor #2). * chore(#441): sync runtime launcher snippet into list-seeds workflow Propagate the current _runtime-launcher.snippet.sh (with non-Claude runtime home probes) into the new list-seeds.md workflow via scripts/sync-runtime-launcher.cjs, satisfying bug-891 (E) propagation. * test(#441): record list-seeds.md in workflow size baseline (#1074) --------- Co-authored-by: Tom Boucher <trekkie@nomorestars.com>	2026-06-22 00:59:41 -04:00
Colin	533b518553	fix(security-scan): update scanner self-exemption allowlists for renamed suite files ... The three shell scanners exempt their own adversarial test fixtures by exact filename; the *.security.test.cjs renames broke those entries, so the PR diff scan flagged the scanners' own test payloads. Verified locally with all three scanners in --diff origin/next mode (0 findings) and the security suite (207/207). The .sh files were missed in the original reference sweep because the rename grep filtered to .cjs/.yml/.json/.md extensions. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>	2026-06-10 00:15:02 -04:00
Tom Boucher	b238baddbf	fix(#663): resolve open CodeQL/Dependabot security alerts (ReDoS, prototype pollution, workflow perms, qs DoS) (#665) ... * fix(#663): resolve open CodeQL/Dependabot security alerts - ReDoS: collapse ambiguous nested quantifiers in phase-heading regexes (verify/validate/commands) and the plan-filename lookahead (phase) to provably-equivalent non-backtracking forms - prototype pollution: guard __proto__/constructor/prototype in setConfigValue - remove dead no-op .replace(/-/g,'-') in phase.cts - escape all regex metachars in bug-2839 test - add contents:read permissions to security-scan + install-smoke workflows - pin qs >= 6.15.2 via overrides (DoS GHSA) - broaden prompt-injection allowlist to translated security-model docs Closes #663 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#663): regression tests for prototype-pollution guard and roadmap-phase ReDoS Behavioral test that config-set rejects __proto__/constructor/prototype keys without polluting Object.prototype, plus a ReDoS guard (timing-bound) and behavior-preservation assertions for the collapsed phase-heading regexes. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#663): make ReDoS regression assert structured result, not elapsed time Replace elapsed-time assertions (which tripped local/no-elapsed-assertion ESLint rule and were unsound for synchronous ReDoS) with structured-result assertions on adversarial inputs: assert that malformed phase headings/ unchecked-item lines without a terminating colon/space yield an empty Set, which is both the correct behavior and an exercise of the fixed linear regex on the catastrophic-backtracking input shape. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(#663): add Security changeset fragment for #665 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(#663): fold prototype-pollution regression into config.test.cjs The standalone bug-663-config-prototype-pollution.test.cjs was a 9th config-module test file, tripping lint-test-file-count (the allowlist is ratcheted and must not grow). Consolidated into config.test.cjs instead. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>	2026-06-04 08:54:15 -04:00
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>	2026-06-02 18:35:29 -04:00
Tom Boucher	835dd6ab44	test(3596): adversarial security/prompt-injection abuse suite (#3654) ... * test(3596): adversarial security/prompt-injection abuse suite Adds `tests/security-prompt-injection.test.cjs` and a fixtures directory at `tests/fixtures/adversarial/security/` covering the attack classes enumerated in #3596: - Command substitution / backticks / heredoc payloads in workstream names — sentinel-file probes prove no shell is spawned, slugifier neutralises the input. - Path traversal through `--ws` and slash-bearing workstream names — rejected with structured `--json-errors` payload, no stack trace, no filesystem mutation outside the project root. - Fake `<system>` / `[SYSTEM]` / `<<SYS>>` / `[INST]` boundary tags — sanitizeForPrompt neutralises every form; structural negative property locked across all six styles in one place. - Zero-width / bidi-override codepoints — stripped per the documented codepoint set; asserted via codePoint inspection, not regex literals. - Hostile read of CONTEXT.md / PLAN.md / ROADMAP.md fixtures — `gsd-read-injection-scanner.js` surfaces the advisory; excluded paths and non-Read tools stay silent; malformed JSON does not crash the hook. - Hostile write of `.planning/` files — `gsd-prompt-guard.js` emits a `PreToolUse` advisory; non-Write/Edit tools stay silent. - Fake `ghp_*` / `sk-*` env tokens — never echoed in CLI stdout or stderr under hostile inputs; covered under `// allow-test-rule: structural-regression-guard` because the only way to assert byte-level absence is `.includes(token)` against the captured streams. - `validatePath`, `validateShellArg`, `validatePhaseNumber`, `validateFieldName` — focused negative-input contract pins. Pinned behavior gaps (documented, NOT fixed in this PR): - `<instructions>` is intentionally whitelisted by both the scanner and the sanitiser (GSD's own prompt scaffolding). Two REGRESSION GUARD tests lock that contract. - The current `scanForInjection` does NOT flag malicious markdown links (javascript:/data:/embedded-credentials URLs). PINNED with negative-proof so any future scope extension fails the assertion and forces a deliberate update to the acceptance map. - `prompt-builder.ts` does not yet wrap plan/context markdown in an "untrusted data" envelope. That seam lives on the TS side and is covered by `sdk/src/prompt-builder.test.ts`; out of scope for a CJS test file. Mentioned in the file header. Verification: - `node --test tests/security-prompt-injection.test.cjs` → 73 tests pass. - `node scripts/lint-no-source-grep.cjs` → 0 violations across 546 test files (one `allow-test-rule: structural-regression-guard` annotation on this file for the token-absence assertions). - `node scripts/run-tests.cjs` → 9730 tests pass, 0 fail. Refs #3596 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com> * fix(3596): allow adversarial fixtures in scan + harden graphify status parse * fix(3596): skip adversarial security fixtures in secret scan --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>	2026-05-17 00:34:53 -04:00
Tom Boucher	c35997fb0b	feat(hooks): add gsd-read-injection-scanner PostToolUse hook (#2201) (#2328) ... * feat: add /gsd-spec-phase — Socratic spec refinement with ambiguity scoring (#2213) Introduces `/gsd-spec-phase <phase>` as an optional pre-step before discuss-phase. Clarifies WHAT a phase delivers (requirements, boundaries, acceptance criteria) with quantitative ambiguity scoring before discuss-phase handles HOW to implement. - `commands/gsd/spec-phase.md` — slash command routing to workflow - `get-shit-done/workflows/spec-phase.md` — full Socratic interview loop (up to 6 rounds, 5 rotating perspectives: Researcher, Simplifier, Boundary Keeper, Failure Analyst, Seed Closer) with weighted 4-dimension ambiguity gate (≤ 0.20 to write SPEC.md) - `get-shit-done/templates/spec.md` — SPEC.md template with falsifiable requirements (Current/Target/Acceptance per requirement), Boundaries, Acceptance Criteria, Ambiguity Report, and Interview Log; includes two full worked examples - `get-shit-done/workflows/discuss-phase.md` — new `check_spec` step detects `{padded_phase}-SPEC.md` at startup; displays "Found SPEC.md — N requirements locked. Focusing on implementation decisions."; `analyze_phase` respects `spec_loaded` flag to skip "what/why" gray areas; `write_context` emits `<spec_lock>` section with boundary summary and canonical ref to SPEC.md - `docs/ARCHITECTURE.md` — update command/workflow counts (74→75, 71→72) Closes #2213 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * feat(hooks): add gsd-read-injection-scanner PostToolUse hook (#2201) Adds a new PostToolUse hook that scans content returned by the Read tool for prompt injection patterns, including four summarisation-specific patterns (retention-directive, permanence-claim, etc.) that survive context compression. Defense-in-depth for long GSD sessions where the context summariser cannot distinguish user instructions from content read from external files. - Advisory-only (warns without blocking), consistent with gsd-prompt-guard.js - LOW severity for 1-2 patterns, HIGH for 3+ - Inlined pattern library (hook independence) - Exclusion list: .planning/, REVIEW.md, CHECKPOINT, security docs, hook sources - Wired in install.js as PostToolUse matcher: Read, timeout: 5s - Added to MANAGED_HOOKS for staleness detection - 19 tests covering all 13 acceptance criteria (SCAN-01–07, EXCL-01–06, EDGE-01–06) Closes #2201 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(ci): add read-injection-scanner files to prompt-injection-scan allowlist Test payloads in tests/read-injection-scanner.test.cjs and inlined patterns in hooks/gsd-read-injection-scanner.js legitimately contain injection strings. Add both to the CI script allowlist to prevent false-positive failures. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(test): assert exitCode, stdout, and signal explicitly in EDGE-05 Addresses CodeRabbit feedback: the success path discarded the return value so a malformed-JSON input that produced stdout would still pass. Now captures and asserts all three observable properties. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-04-16 17:22:31 -04:00
Tom Boucher	2703422be8	refactor(tests): standardize to node:assert/strict and t.after() per CONTRIBUTING.md (#1675) ... * refactor(tests): standardize to node:assert/strict and t.after() per CONTRIBUTING.md - Replace require('node:assert') with require('node:assert/strict') across all 73 test files to enforce strict equality (no type coercion) - Replace try/finally cleanup blocks with t.after() hooks in core.test.cjs and hooks-opt-in.test.cjs per the test lifecycle standards - Utility functions in codex-config and security-scan retain try/finally as that is appropriate for per-function resource guards, not lifecycle hooks Closes #1674 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * perf(tests): add --test-concurrency=4 to test runner for parallel file execution Node.js --test-concurrency controls how many test files run as parallel child processes. Set to 4 by default, configurable via TEST_CONCURRENCY env var. Fixes tests at a known level rather than inheriting os.availableParallelism() which varies across CI environments. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> * fix(security): allowlist verify.test.cjs in prompt-injection scanner tests/verify.test.cjs uses <human>...</human> as GSD phase task-type XML (meaning "a human should verify this step"), which matches the scanner's fake-message-boundary pattern for LLM APIs. This is a false positive — add it to the allowlist alongside the other test files that legitimately contain injection-adjacent patterns. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>	2026-04-04 14:29:03 -04:00
Tom Boucher	98f05d43b8	fix: security scan self-detection and Windows test compatibility ... - Add base64-scan.sh and secret-scan.sh to prompt injection scanner allowlist (scanner was flagging its own pattern strings) - Skip executable bit check on Windows (no Unix permissions) - Skip bash script execution tests on Windows (requires Git Bash) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>	2026-03-24 13:30:15 -04:00
Tom Boucher	feec5a37a2	ci(security): add prompt injection, base64, and secret scanning ... Add CI security pipeline to catch prompt injection attacks, base64-obfuscated payloads, leaked secrets, and .planning/ directory commits in PRs. This is critical for get-shit-done because the entire codebase is markdown prompts — a prompt injection in a workflow file IS the attack surface. New files: - scripts/prompt-injection-scan.sh: scans for instruction override, role manipulation, system boundary injection, DAN/jailbreak, and tool call injection patterns in changed files - scripts/base64-scan.sh: extracts base64 blobs >= 40 chars, decodes them, and checks decoded content against injection patterns (skips data URIs and binary content) - scripts/secret-scan.sh: detects AWS keys, OpenAI/Anthropic keys, GitHub PATs, Stripe keys, private key headers, and generic credential patterns - .github/workflows/security-scan.yml: runs all three scans plus a .planning/ directory check on every PR - .base64scanignore / .secretscanignore: per-repo false positive allowlists - tests/security-scan.test.cjs: 51 tests covering script existence, pattern matching, false positive avoidance, and workflow structure All scripts support --diff (CI), --file, and --dir modes. Cross-platform (macOS + Linux). SHA-pinned actions. Environment variables used for github context in run blocks (no direct interpolation). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>	2026-03-24 13:23:51 -04:00