model_overrides / models.<phaseType> were silently inert for gsd-assumptions-analyzer,
gsd-code-reviewer, and gsd-code-fixer on Claude Code: resolveModelInternal honors them,
but the workflows spawned these agents with no model= param, so the resolved value
never reached the Agent tool and the agents inherited the session model — no warning.
Fix — thread each agent's resolved model at every spawn site (the established
plan-phase pattern; the architecture-consistent Claude mechanism, since 13 other
agents already thread their model):
- discuss-phase-assumptions.md: `resolve-model gsd-assumptions-analyzer --raw`
→ ANALYZER_MODEL, threaded.
- code-review.md + code-review-fix.md (re-review): `resolve-model gsd-code-reviewer --raw`
→ REVIEWER_MODEL, threaded.
- code-review-fix.md (both fixer spawns): `resolve-model gsd-code-fixer --raw`
→ FIXER_MODEL, threaded (same silently-inert bug, same file — folded in per review).
- quick.md review step: was reusing `{executor_model}` for gsd-code-reviewer (so the
reviewer's own override was ignored); init.quick now resolves `reviewer_model`
(gsd-code-reviewer) and the spawn threads it.
resolve-model --raw returns the bare model string (resolve-execution --raw would
return effort — wrong). The resolver maps these agents to phaseType discuss /
verification / execution, so models.<phaseType> apply too.
Scope: the three agents reachable from the two issue-named workflows + quick.md. The
wider systemic class (other agents in UNTOUCHED workflows with the same pattern) stays
documented on the issue for a maintainer-scoped structural decision (thread-at-source
vs embed-at-install like #2256), not widened here.
Docs: the stale "discuss — reserved, no subagent today" model-profile tables now list
gsd-assumptions-analyzer and the verification row includes gsd-code-reviewer, across
the English docs, the shipped gsd-core/references/model-profiles.md reference, and the
ja-JP / zh-CN / ko-KR / pt-BR locale mirrors.
Tests:
- tests/model-resolver.test.cjs: #2072 acceptance — model_overrides and
models.discuss/verification/execution resolve for all three agents.
- tests/model-routing-spawn-threading.test.cjs: every spawn of the three agents threads
a resolved model (fails pre-fix); a header-precise parity guard fails the suite if a
new un-threaded spawn of any of them regresses.
All 16 golden-install-parity fixtures + the workflow size baseline regenerated for the
changed shipped files (4 workflows + the reference doc); bin/lib is excluded from parity.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A GSD verification gate resolves a project's test command and runs it. vitest
defaults to WATCH mode in an interactive TTY — exactly where a user runs
`gsd-execute-phase` — so a resolved `npm test`/`pnpm test` backed by vitest never
exited and the orchestrator waited indefinitely. Recovery needed the user to
manually prompt "something blocking?".
Fix — one shared helper + a bounded, surfacing timeout on the test-command gates:
- New pure module src/normalize-test-command.cts + `gsd-tools query
normalize-test-command` verb: rewrites a resolved command to a best-effort
one-shot form (direct vitest → `vitest run`; jest `--watch` → `--watchAll=false`;
a package-manager `test` script whose package.json runner is watch-vitest →
`CI=true` prefix; handles `--dir`; already-one-shot commands unchanged —
never double-flagged). Named `normalize-test-command` (not `test-*`) so the
file does not match node --test's default `test-*` discovery glob.
- The three gates that HUNG or silently-continued route through that ONE helper
and bound execution with `timeout $(config-get workflow.test_gate_timeout)`
(new config key, default 600s): the regression gate (extracted to
execute-phase/steps/regression-gate.md since execute-phase.md is size-frozen —
it shrank 93528→93132; ABORTS on exit 124), the post-merge gate, and the
audit-fix gate (previously an UNBOUNDED `eval`). All name watch/dev mode on 124.
- verify-phase's gate was ALREADY bounded (a fixed `timeout 300`, not a hang), so
it only gains the normalizer (so a watch runner exits fast) + a watch-mode hint
on 124, staying under its frozen 40960-byte tier cap.
Security hardening (review): the normalizer only rewrites a runner named as a
standalone command TOKEN (so `run-vitest.js`/`make test-vitest`/paths are never
mangled), is length-capped and uses only linear-time split-based scanning (no
super-linear backtracking on an adversarial `workflow.test_command`), and reads
package.json only when it is a regular file (never blocks on a FIFO via `--dir`).
Config key `workflow.test_gate_timeout` (seconds, default 600) registered in the
schema manifest + templates/config.json + docs/CONFIGURATION.md (mirrors
workflow.cross_ai_timeout). New module registered in .gitignore, eslint ignores,
inventory manifest/index. All 16 golden-install-parity fixtures + workflow size
baseline regenerated for the changed shipped files; bin/lib is excluded from the
parity manifest.
Tests: tests/normalize-test-command.test.cjs (normalizer units incl. security
hardening) and tests/test-gate-watch-mode.test.cjs (the three core gates route
through the shared helper + configured timeout + exit-124 watch-mode hint;
verify-phase asserted as normalize-only/already-bounded).
tests/execute-phase-active-flags.test.cjs repointed at the extracted step;
tests/planner-language-regression.test.cjs allowlist comment updated.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A phase whose slug's first word is a bare single digit (dir
`46-6-rs-pipeline-orchestrator`, roadmap phase "6 Rs Pipeline Orchestrator" →
slug `6-rs-…`) had its phase token over-collected as `46-6` instead of `46`, so
`gsd-tools` phase-by-number lookups (init.plan-phase, init.phase-op, and
downstream execute/verify/ship) resolved phase_dir=null / has_context=false.
Root cause: an over-broad "looks numeric" test (`/^\d/`, `\d+(?:-\d+)*`)
classified token segments and could not distinguish a legitimate zero-padded
sub-phase segment (`01`, `02`) from a single-digit slug word (`6`). Zero-padded
phase/sub-phase segments are always ≥2 digits, so requiring ≥2 digits is the
structural distinguisher. Applied consistently across every same-class
implementation the triage identified (fixing extractPhaseToken alone leaves the
health-check and milestone-filter subsystems exposed):
- src/phase-id.cts extractPhaseToken — a pure-numeric leading segment continues
only with ≥2-digit segments; a letter-prefixed milestone id (`M1`) still
admits its single-digit sub-phase (`M1-2`).
- src/validate.cts PHASE_TOKEN_FROM_DIR_RE (W005/W006/W007 health checks) and
canonicalPlanStem (I001 plan/summary pairing).
- src/roadmap-parser.cts isDirInMilestone numericRe (getMilestonePhaseFilter —
highest blast radius: a false negative silently excludes a phase from the
milestone).
- src/core-utils.cts + its verbatim duplicate src/phase.cts extractCanonicalPlanId.
Regression tests added across tests/{phase-id,health-validation,core-utils,
roadmap-parser}.test.cjs using the shared `46-6-rs-…` fixture, asserting the
token resolves to `46` (not `46-6`) while legit multi-segment tokens
(`01-02`, `02-03-04`, `M1-2`, `68-01`, `02-01`) are unchanged. The
roadmap-parser test exercises the real getMilestonePhaseFilter end-to-end.
The residual ≥2-digit-leading-slug case (e.g. `NN-2024-roadmap`) stays out of
scope — subsumed by the #612 / #565 phase-ID convention work, per the issue.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
`gsd-tools config-set <key> null` — the "Clear" action documented in
settings-integrations.md / settings-advanced.md — previously fell through the
value parser as the literal STRING "null" and persisted it. Consequences:
"cleared" keys stayed set (config-get returned truthy "null"), and for secret
keys (brave_search/firecrawl/exa_search) a masked success line hid a truthy
4-char value on disk that integrations could pass along as a real credential.
There was also no unset/delete verb at all.
Fix: parse a bare `null` to JS null and short-circuit to a real UNSET that
DELETES the key from config.json — the semantic the docs already describe
("Remove the stored key" / "remove the key by setting it to null"). Deleting
(not persisting JSON null) is the correct clear: a persisted null is still a
present value consumers must special-case.
- src/config.cts:
- parse block: `else if (val === 'null') parsedValue = null;`
- cmdConfigSet: when parsedValue === null, short-circuit BEFORE the typed
per-key validator gauntlet (so clearing an enum/boolean/number key removes
it rather than being rejected) and before the project_code special-case;
mask the previous value for secret keys in the output.
- new `unsetConfigValue()` + `_unsetNestedValue()` mirroring setConfigValue/
_setNestedValue: same prototype-pollution guard, but never creates missing
intermediates and never prunes empty parents; returns { previousValue,
existed }. Unsetting a never-set key is an idempotent no-op success.
- tests/config.test.cjs: new suite covering non-secret routing key, secret key,
typed-enum-key bypass (context), idempotent unset, literal-"null"-on-disk
guard, the unset-path prototype-pollution guard (alert #26 parity), and a
4-segment deep-nested unset.
- tests/review-model-config.test.cjs: update the stale round-trip test that
codified the bug (asserted config-set null → config-get returns "null") to the
fixed contract — the model key is removed; the review workflow's
`[ -n "$VAR" ] && [ "$VAR" != "null" ]` guard handles the empty read as
"no override → reviewer default", same as the old "null" sentinel.
The 4 documented "Clear" flows (settings-integrations.md, settings-advanced.md)
were verified — their prose already describes removal, so the fix makes them
accurate rather than aspirational; no doc wording change required.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Kilo and ZCode both declare `hooksSurface: 'none'` and have no plugin surface,
so the GSD installer staged lifecycle hook scripts (hooks/*.js, hooks/*.sh,
hooks/lib/) plus a `{"type":"commonjs"}` package.json marker into their config
dirs where nothing ever invokes them — dead weight (#1821).
The installer's two hook-copy guards at bin/install.js were still on the legacy
hardcoded runtime-name list and never excluded Kilo or ZCode. Add
`&& !isKilo && !isZcode` to both (isZcode added to the install() runtimeFlags
destructure).
OpenCode — which #1821 also named — is deliberately NOT excluded: since the
issue was filed, #1914 shipped a native OpenCode plugin (plugins/gsd-core.js)
that spawns those exact staged hooks via OpenCode's event bus and requires both
the hook scripts and the CommonJS package.json marker. Excluding OpenCode would
regress #1914, so its hooks stay live. The genuinely-dead cases are Kilo & ZCode.
- bin/install.js: add `&& !isKilo && !isZcode` to the hooks/dist copy guard and
the hooks/lib copy guard; document the OpenCode-vs-Kilo/ZCode split.
- tests/install-minimal-hooks.test.cjs: regression test asserting Kilo and ZCode
receive no gsd-*.js/.sh hooks or hooks/lib, while OpenCode keeps its hooks +
#1914 plugin and Claude keeps its hooks (over-exclusion guard).
- tests/fixtures/golden-install-parity/{kilo,zcode}.json: drop the 21 hooks/*
entries and the package.json marker they no longer receive (opencode unchanged).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
A skills-only role:feature third-party capability installed 'active' but its skills never reached the runtime surface, capability enable/set rejected it as 'unknown capability', and capability list disagreed with capability state. Three defects, fix shape 1b (teach resolveSurface, no on-disk linking):
D1 (materialization): resolveSurface built the surfaced skills Set only from the on-disk manifest; third-party cap skills live at ~/.gsd/capabilities/<id>/skills/ and never entered the Set -> surfaced:false. Fix: union registry.capabilityClusters values into the Set in the full-profile branch (idempotent for first-party, additive for third-party; prototype-pollution guarded).
D2 (enable/set unknown): setCapabilityState validated the capId against the static first-party registry instead of the composed overlay-aware registry. Fix: validate against loadRegistry({includeInstalled}) like capability-state.cts does.
D3 (list vs state): capability list derived status purely from ledger existence, never surface composition. Fix: add a 'surfaced' field to list rows sourced from the same resolver capability state uses, so list and state agree.
Regression coverage: tests/issue-2045-third-party-skills-surface.test.cjs asserts all five acceptance criteria (resolveSurface union, state surfaced:true, enable/set not-unknown, list/state agreement, first-party + unknown-id regression). gsd-test: 23916/23916 green (linux-node22 + linux-node24).
- warn (don't silently ignore) when --runtime is an unknown runtime that
canonicalizeRuntimeName rejects; the warning surfaces via warnings[] so a
typo like --runtime cluade or a runtime known to runtime-homes but not the
alias manifest (e.g. grok) no longer silently resolves to the persisted
runtime's config dir on this diagnostic command [M-1]
- add end-to-end CLI test for loop render-hooks --runtime (the exact command
the bug report calls out as silently no-op'ing) [L-2]
- add closed-vocabulary rejection test: crafted --runtime values
(../../etc/passwd, __proto__, --config-dir, garbage) are rejected, warn,
and fall through to the persisted runtime — pins the security-load-bearing
contract [NIT-01]
- add boundary tests: --config-dir wins over --runtime (precedence); missing
--runtime value errors with USAGE [N-1]
Both orthogonal reviews returned APPROVE with no Critical/High findings.
Security review confirmed --runtime cannot coerce getGlobalConfigDir into an
arbitrary path (closed-vocabulary Map lookup + registry hash-key gate) and
does not expand the trust surface beyond the existing operator-controlled
--config-dir flag.
The --runtime parsing + help-text edit to gsd-core/bin/gsd-tools.cjs changes
the installed file's content (gsd-tools.cjs is installed and compared by the
golden snapshot, unlike gsd-core/bin/lib/ which is excluded). Regenerated via
UPDATE_GOLDEN=1; every runtime's manifest updates exactly one line (the
gsd-tools.cjs hash).
resolveCapabilityRuntimeState derived the config dir from resolveRuntime(cwd)
(GSD_RUNTIME -> config.runtime -> 'claude') when no --config-dir was passed,
so a repo with persisted runtime:'codex' resolved the config dir to ~/.codex
where the Claude skill isn't installed -> surfaced:false / hooks silently
no-op when the operator drove from Claude Code. capability state and loop
render-hooks parsed only --config-dir, never --runtime, so there was no way
to assert the actually-active runtime.
Add a runtimeOverride param to resolveCapabilityRuntimeState (canonicalized
via runtime-name-policy so aliases like codex-app work); when present it
short-circuits the persisted-runtime fallback and resolves getGlobalConfigDir
for the explicit runtime. Thread --runtime through cmdCapabilityState and
cmdLoopRenderHooks, and parse it in gsd-tools.cjs for both commands (dual
--runtime X / --runtime=X form, mirroring --config-dir and the existing
capability-set --runtime precedent). Help text updated.
Without the override, behavior is byte-identical to today (regression-guarded).
Mirrors the #1160 installed-layout block for the runtime auto-detection gap.
Covers: runtimeOverride='claude' bypasses persisted config.runtime:'codex';
no override still honours persisted runtime (regression guard); alias
canonicalization (codex-app -> codex); and an end-to-end CLI test proving
'capability state --runtime claude' resolves the Claude config dir despite a
persisted runtime:'codex'.
Expected RED against unfixed resolveCapabilityRuntimeState (no runtimeOverride
param) and unfixed gsd-tools.cjs (no --runtime parsing for capability state /
loop render-hooks).
- restructure _loadFlatCommandsGsdManifest try/catch to wrap read+parse+set
together (mirrors loadSkillsManifest exactly), so a thrown parser degrades
both keys to [] — closes the latent catch-scope parity drift [Nit-1]
- add boundary test: gsd-.md (empty stem) skipped, gsd-x.md single-char stem
kept (slice(4,-3) boundary) [Low-1]
- add unreadable-file test (POSIX-gated): both keys degrade to [] [Low-2]
- strengthen parity test to also compare requires + _calls_agents_ VALUES,
not just the stem set [Nit-2]
Both orthogonal reviews returned APPROVE with no Critical/High findings.
Security review confirmed no new trust-boundary crossing, prototype-pollution
immune (Map/Set throughout), and symlink/path-traversal surface identical to
the pre-existing nested loader (not a regression).
_resolveManifest only recognized the nested source layout (commands/gsd/*.md)
and the installed-runtime skills layout (skills/gsd-<stem>/SKILL.md). A flat
source install (Claude local project shape: commands/gsd-<stem>.md, no
commands/gsd/ subdir) matched neither branch, so the manifest came back empty
and resolveSurface materialized the full profile to an empty Set — silently
reporting every skill-bearing capability as surfaced:false / enabled:false /
active:false. The nyquist/code-review/security/ui verify:post and execute:post
hooks never fired even with their workflow.* toggles on.
Add a third branch: when commandsGsdDir is absent, scan dirname(commandsGsdDir)
for gsd-<stem>.md files, strip the gsd- prefix, and build the same Map shape
the nested loader produces (requires via shared parseRequires, companion
_calls_agents_<stem> via shared parseCallsAgents). Falls through to the
installed-skills branch when the flat dir has no gsd-*.md files (precedence:
nested > flat-source > installed).
Also export parseCallsAgents from install-profiles so capability-state reuses
the SAME parser the nested loader uses (no drift; mirrors the existing
parseRequires export+reuse pattern).
Mirrors the #1160 installed-layout tests for the flat source layout
(<repo>/commands/gsd-<stem>.md, no commands/gsd/ subdir). Covers stem
extraction (strip gsd- prefix), requires parsing via shared parseRequires,
companion _calls_agents_ key parity, _resolveManifest flat-branch detection,
precedence (flat-empty falls through to installed), and a generative-parity
assertion that the flat loader and nested loader produce identical stem sets
for the real command tree.
Expected RED against unfixed capability-state.cts (_resolveManifest has no
flat branch; _loadFlatCommandsGsdManifest not exported).
- add typeof guard so a non-string override passes through verbatim instead of
crashing on .startsWith (preserves pre-fix no-crash behaviour) [LOW-1]
- use Object.hasOwn() for the alias lookup so __proto__/constructor cannot
return a truthy non-string from the plain object literal [LOW-D3]
- cap the unmappable-override stderr warning at 64 chars so an oversized or
secret-shaped value cannot leak in full to stderr/logs [LOW-D4]
- remove the unused mapClaudeOverrideForRuntime export (helpers are covered
behaviourally via resolveModelInternal/resolveModelForTier) [NIT]
- add resolveModelForTier unmappable-override fall-through test (closes the
mutation-score gap) [MEDIUM-1]
- add case-sensitivity contract test (Claude-Sonnet-5 passes through verbatim) [LOW-2]
Both orthogonal reviews returned APPROVE with no Critical/High findings.
model_overrides values that are full Claude model IDs (claude-sonnet-5,
claude-opus-4-8, claude-haiku-4-5, claude-fable-5) were returned verbatim on
the claude runtime and handed to the Claude Agent tool, whose typed model
parameter documents only tier aliases (opus/sonnet/haiku/fable). The
model_policy path already mapped full IDs -> aliases via
CLAUDE_POLICY_ID_TO_ALIAS (#1144); model_overrides skipped that mapping, so
the two resolver paths produced different shapes for the same underlying
Claude model. The fix mirrors #1144 on the override path via a shared
mapClaudeOverrideForRuntime helper used by both resolveModelInternal and
resolveModelForTier. Bare aliases pass through verbatim; non-Claude runtimes
and non-Claude custom/vendor values keep full IDs verbatim (parity). An
unmappable Claude ID (e.g. claude-opus-4-5) warns once to stderr and falls
through to tier resolution, exactly as the model_policy path already does.
Alias mapping is also the documented best practice (prevents staleness when
new model versions ship).
