From cf6d3b3be5a3342780494c436f31871b05caacfc Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 9 Jun 2026 08:42:16 -0400 Subject: [PATCH 1/4] fix(#925): context monitor echoes the invoking hook event name (#927) Read `data.hook_event_name` from the stdin payload and fall back to the Gemini/non-Gemini heuristic only when the field is absent or blank. Fixes Claude Code rejecting output with "expected Stop but got PostToolUse" when the monitor is called by Stop, SubagentStop, or PreCompact hooks. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .../925-context-monitor-hook-event-name.md | 5 + hooks/gsd-context-monitor.js | 3 +- ...5-context-monitor-hook-event-name.test.cjs | 205 ++++++++++++++++++ 3 files changed, 212 insertions(+), 1 deletion(-) create mode 100644 .changeset/925-context-monitor-hook-event-name.md create mode 100644 tests/bug-925-context-monitor-hook-event-name.test.cjs diff --git a/.changeset/925-context-monitor-hook-event-name.md b/.changeset/925-context-monitor-hook-event-name.md new file mode 100644 index 000000000..8a3dd76d3 --- /dev/null +++ b/.changeset/925-context-monitor-hook-event-name.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 926 +--- +**`gsd-context-monitor.js` now echoes the actual invoking hook event name** — instead of hardcoding `hookEventName: "PostToolUse"` (or `"AfterTool"` for Gemini), the hook reads `data.hook_event_name` from the stdin payload and falls back to the runtime heuristic only when the field is absent or blank; this fixes Claude Code rejecting hook output with `"expected Stop but got PostToolUse"` when the monitor is invoked by the Stop, SubagentStop, or PreCompact hooks registered in PR #821. (#925) diff --git a/hooks/gsd-context-monitor.js b/hooks/gsd-context-monitor.js index 5aa93f67f..991535f40 100644 --- a/hooks/gsd-context-monitor.js +++ b/hooks/gsd-context-monitor.js @@ -182,7 +182,8 @@ process.stdin.on('end', () => { const output = { hookSpecificOutput: { - hookEventName: process.env.GEMINI_API_KEY ? "AfterTool" : "PostToolUse", + hookEventName: (data.hook_event_name && data.hook_event_name.trim()) + || (process.env.GEMINI_API_KEY ? "AfterTool" : "PostToolUse"), additionalContext: message } }; diff --git a/tests/bug-925-context-monitor-hook-event-name.test.cjs b/tests/bug-925-context-monitor-hook-event-name.test.cjs new file mode 100644 index 000000000..9257d5936 --- /dev/null +++ b/tests/bug-925-context-monitor-hook-event-name.test.cjs @@ -0,0 +1,205 @@ +/** + * Regression test for bug #925 + * + * hooks/gsd-context-monitor.js hardcodes `hookEventName: "PostToolUse"` (or + * "AfterTool" for Gemini) regardless of which hook event invoked it. Since + * PR #821 the same script is also registered under Stop, SubagentStop, and + * PreCompact in hooks/hooks.json. Claude Code rejects output whose + * hookSpecificOutput.hookEventName doesn't echo the triggering event: + * + * "expected Stop but got PostToolUse" + * + * Fix: derive hookEventName from the parsed stdin payload's `hook_event_name` + * field (already available in the data object), falling back to the + * Gemini / non-Gemini heuristic for runtimes that don't send it. + */ + +'use strict'; + +const { test, describe } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const os = require('node:os'); +const path = require('node:path'); +const { execFileSync } = require('node:child_process'); + +const MONITOR_PATH = path.join(__dirname, '..', 'hooks', 'gsd-context-monitor.js'); + +/** + * Write a bridge metrics file and invoke the context monitor with the given + * payload fields. Returns the parsed stdout object (or null if the hook + * produced no output). + * + * remainingPct must be <= 35 to cross the WARNING threshold so the hook + * actually emits output. + */ +function runMonitor({ hookEventName, sessionId, remainingPct = 30, usedPct = 70, env = {} }) { + const bridgePath = path.join(os.tmpdir(), `claude-ctx-${sessionId}.json`); + fs.writeFileSync(bridgePath, JSON.stringify({ + session_id: sessionId, + remaining_percentage: remainingPct, + used_pct: usedPct, + timestamp: Math.floor(Date.now() / 1000), + })); + + const payload = { session_id: sessionId, cwd: os.tmpdir() }; + if (hookEventName !== undefined) { + payload.hook_event_name = hookEventName; + } + + let stdout = ''; + try { + stdout = execFileSync(process.execPath, [MONITOR_PATH], { + input: JSON.stringify(payload), + encoding: 'utf-8', + timeout: 5000, + env: { ...process.env, ...env }, + }); + } catch (e) { + stdout = e.stdout || ''; + } finally { + try { fs.unlinkSync(bridgePath); } catch { /* noop */ } + try { + fs.unlinkSync(path.join(os.tmpdir(), `claude-ctx-${sessionId}-warned.json`)); + } catch { /* noop */ } + } + + if (!stdout) return null; + return JSON.parse(stdout); +} + +function makeSessionId(suffix) { + return `test-925-${suffix}-${Date.now()}-${Math.random().toString(36).slice(2)}`; +} + +// ─── hookEventName echoing ──────────────────────────────────────────────────── + +describe('bug #925: context monitor echoes the invoking hook event name', () => { + test('hookEventName is "Stop" when payload contains hook_event_name: "Stop"', () => { + const out = runMonitor({ hookEventName: 'Stop', sessionId: makeSessionId('stop') }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold (remaining=30)'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'Stop', + `Expected hookEventName "Stop" but got "${out.hookSpecificOutput?.hookEventName}". ` + + 'The hook must echo the hook_event_name from stdin, not hardcode "PostToolUse".' + ); + }); + + test('hookEventName is "SubagentStop" when payload contains hook_event_name: "SubagentStop"', () => { + const out = runMonitor({ hookEventName: 'SubagentStop', sessionId: makeSessionId('subagent-stop') }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'SubagentStop', + `Expected hookEventName "SubagentStop" but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); + + test('hookEventName is "PreCompact" when payload contains hook_event_name: "PreCompact"', () => { + const out = runMonitor({ hookEventName: 'PreCompact', sessionId: makeSessionId('precompact') }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'PreCompact', + `Expected hookEventName "PreCompact" but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); + + test('hookEventName is "PostToolUse" when payload contains hook_event_name: "PostToolUse"', () => { + const out = runMonitor({ hookEventName: 'PostToolUse', sessionId: makeSessionId('posttools') }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'PostToolUse', + `Expected hookEventName "PostToolUse" but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); +}); + +// ─── Fallback behaviour (no hook_event_name in payload) ────────────────────── + +describe('bug #925: context monitor falls back to heuristic when hook_event_name absent', () => { + test('falls back to "PostToolUse" when hook_event_name is absent (non-Gemini)', () => { + const env = { ...process.env }; + delete env.GEMINI_API_KEY; + const out = runMonitor({ + hookEventName: undefined, + sessionId: makeSessionId('fallback-non-gemini'), + env: { GEMINI_API_KEY: '' }, // ensure unset + }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'PostToolUse', + `Expected fallback "PostToolUse" for non-Gemini but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); + + test('falls back to "AfterTool" when hook_event_name is absent and GEMINI_API_KEY is set', () => { + const out = runMonitor({ + hookEventName: undefined, + sessionId: makeSessionId('fallback-gemini'), + env: { GEMINI_API_KEY: 'fake-key-for-test' }, + }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'AfterTool', + `Expected fallback "AfterTool" for Gemini but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); + + test('falls back to "PostToolUse" when hook_event_name is an empty string (non-Gemini)', () => { + const out = runMonitor({ + hookEventName: '', + sessionId: makeSessionId('fallback-empty'), + env: { GEMINI_API_KEY: '' }, + }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'PostToolUse', + `Expected fallback "PostToolUse" for empty hook_event_name but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); + + test('falls back to "PostToolUse" when hook_event_name is whitespace-only (non-Gemini)', () => { + // trim() makes " " → "" which is falsy, so the || fallback fires + const out = runMonitor({ + hookEventName: ' ', + sessionId: makeSessionId('fallback-whitespace'), + env: { GEMINI_API_KEY: '' }, + }); + assert.ok(out, 'hook must emit output when context is below WARNING threshold'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'PostToolUse', + `Expected fallback "PostToolUse" for whitespace-only hook_event_name but got "${out.hookSpecificOutput?.hookEventName}".` + ); + }); +}); + +// ─── Critical threshold also echoes the event name ─────────────────────────── + +describe('bug #925: critical threshold warning also uses correct hookEventName', () => { + test('CRITICAL warning emitted under Stop also echoes "Stop"', () => { + const out = runMonitor({ + hookEventName: 'Stop', + sessionId: makeSessionId('critical-stop'), + remainingPct: 20, + usedPct: 80, + }); + assert.ok(out, 'hook must emit output at critical threshold (remaining=20)'); + assert.strictEqual( + out.hookSpecificOutput?.hookEventName, + 'Stop', + `Expected hookEventName "Stop" at critical threshold, got "${out.hookSpecificOutput?.hookEventName}".` + ); + assert.match( + out.hookSpecificOutput?.additionalContext || '', + /CONTEXT CRITICAL/, + 'Output should be a CRITICAL warning at remaining=20' + ); + }); +}); From b866b95296aa6a277baa578c5e9c042e122ca64b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 9 Jun 2026 08:42:25 -0400 Subject: [PATCH 2/4] fix(#921,#922): orchestrators must not fork; plan-phase Agent gate is attempt-based (#926) `context: fork` strips the `Agent` tool from a subagent's environment. Spawning orchestrators (`/gsd-autonomous`, `/gsd-execute-phase`, `/gsd-plan-phase`) depend on `Agent` to dispatch sub-agents; running them forked silently disables the core capability they exist to provide (#921). Remove `context: fork` from all three command frontmatter files. `effort: xhigh` (introduced by #769) is preserved. The `` Agent-availability guard added by #913 was checking whether `Agent` was present *before* attempting the call. On runtimes where the tool list is dynamically resolved this produced false-negative aborts in sessions that have the tool (#922). Replace the introspection-based pattern with an attempt-based gate: always attempt the `Agent()` call; stop only if a real tool-unavailable error is returned. This preserves #853's backgrounded-session close-off and #913's intent of preventing inline role-collapse, while eliminating false negatives. Tests updated: enh-769-context-fork-effort.install.test.cjs asserts the three orchestrators lack `context: fork` and that the converter still passes the field through for non-orchestrator commands; plan-phase-drift- guard.test.cjs adds four assertions for the attempt-based gate language; workflow-size-budget unchanged (budgets not exceeded). Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .changeset/921-922-orchestrators-no-fork.md | 5 ++ commands/gsd/autonomous.md | 1 - commands/gsd/execute-phase.md | 1 - commands/gsd/plan-phase.md | 1 - docs/COMMANDS.md | 2 +- docs/explanation/context-engineering.md | 10 ++- gsd-core/workflows/plan-phase.md | 11 +-- ...h-769-context-fork-effort.install.test.cjs | 70 +++++++++++-------- tests/plan-phase-drift-guard.test.cjs | 67 ++++++++++++++++++ tests/workflow-size-budget.test.cjs | 4 +- 10 files changed, 128 insertions(+), 44 deletions(-) create mode 100644 .changeset/921-922-orchestrators-no-fork.md diff --git a/.changeset/921-922-orchestrators-no-fork.md b/.changeset/921-922-orchestrators-no-fork.md new file mode 100644 index 000000000..92e481ef7 --- /dev/null +++ b/.changeset/921-922-orchestrators-no-fork.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 921 +--- +**`/gsd-plan-phase`, `/gsd-execute-phase`, `/gsd-autonomous` no longer carry `context: fork`** — these are spawning orchestrators; a forked subagent context has no `Agent` tool, preventing them from spawning the subagents they require. `effort: xhigh` is preserved. Fixes `/gsd:autonomous` halting with "running as a forked subagent" on 1.4.1 (#921). Also replaces the introspection-based Agent-availability check in `plan-phase`'s `` block with an attempt-based gate: the workflow now always attempts the `Agent()` call and only stops if a real tool-unavailable error is returned, eliminating false-negative aborts in top-level sessions (#922). diff --git a/commands/gsd/autonomous.md b/commands/gsd/autonomous.md index fce955925..9a13ebf21 100644 --- a/commands/gsd/autonomous.md +++ b/commands/gsd/autonomous.md @@ -2,7 +2,6 @@ name: gsd:autonomous description: Run all remaining phases autonomously — discuss→plan→execute per phase argument-hint: "[--from N] [--to N] [--only N] [--interactive]" -context: fork effort: xhigh allowed-tools: - Read diff --git a/commands/gsd/execute-phase.md b/commands/gsd/execute-phase.md index b7acb5885..c7eb7bdca 100644 --- a/commands/gsd/execute-phase.md +++ b/commands/gsd/execute-phase.md @@ -2,7 +2,6 @@ name: gsd:execute-phase description: Execute all plans in a phase with wave-based parallelization argument-hint: " [--wave N] [--gaps-only] [--interactive] [--tdd]" -context: fork effort: xhigh allowed-tools: - Read diff --git a/commands/gsd/plan-phase.md b/commands/gsd/plan-phase.md index 47549dfbf..d4f071145 100644 --- a/commands/gsd/plan-phase.md +++ b/commands/gsd/plan-phase.md @@ -2,7 +2,6 @@ name: gsd:plan-phase description: Create detailed phase plan (PLAN.md) with verification loop argument-hint: "[phase] [--auto] [--research] [--skip-research] [--research-phase ] [--view] [--gaps] [--skip-verify] [--prd ] [--ingest ] [--ingest-format ] [--reviews] [--text] [--tdd] [--mvp]" -context: fork effort: xhigh allowed-tools: - Read diff --git a/docs/COMMANDS.md b/docs/COMMANDS.md index 438bcb6dd..4b3b67ba0 100644 --- a/docs/COMMANDS.md +++ b/docs/COMMANDS.md @@ -14,7 +14,7 @@ The hyphen and colon forms are *runtime-specific spellings of the same command*. ### Skill Runtime Behavior (Claude Code) -Heavy workflow skills (`/gsd-plan-phase`, `/gsd-execute-phase`, `/gsd-autonomous`) carry `context: fork` in their frontmatter. On Claude Code, this runs each skill in an isolated subagent context window, protecting the main session's context budget. The skills also declare `effort: xhigh`, signalling maximum token budget to the runtime. +Heavy workflow skills (`/gsd-plan-phase`, `/gsd-execute-phase`, `/gsd-autonomous`) declare `effort: xhigh`, signalling maximum token budget to the runtime. These skills are spawning orchestrators — they must run at top level so they retain the `Agent` tool needed to spawn subagents. They do **not** carry `context: fork` (see #921). Quick-status skills (`/gsd-progress`, `/gsd-stats`) declare `effort: low`, directing the runtime to use a minimal token budget for fast reads. diff --git a/docs/explanation/context-engineering.md b/docs/explanation/context-engineering.md index 799a7b2a3..77869c33c 100644 --- a/docs/explanation/context-engineering.md +++ b/docs/explanation/context-engineering.md @@ -90,13 +90,11 @@ Claude Code exposes a `FileChanged` event in addition to session-lifecycle hooks Requiring a `/clear` to pick up a config edit would destroy the very continuity the context-engineering design is trying to protect. By watching for `FileChanged` on `config.json`, GSD can reload configuration mid-session — adjusting model profiles, context-window thresholds, or routing preferences — without the user losing their place. The working context survives; the configuration updates beneath it. -### Forked context for heavy skills +### Effort signals for heavy and light skills -Beyond passive monitoring, GSD uses an active strategy for skills whose work is large and bursty: they run in a **forked context** (`context: fork` in the skill definition). +Beyond passive monitoring, GSD uses `effort:` frontmatter to signal the token budget appropriate for each skill. Heavy orchestrator skills (`plan-phase`, `execute-phase`, `autonomous`) declare `effort: xhigh`; quick-status skills (`progress`, `stats`) declare `effort: low`. -Skills like `plan-phase`, `execute-phase`, and `autonomous` do a great deal of work — spawning multiple subagents, reading large files, iterating over multiple plans. If that work happened in the main session, it would consume a substantial share of the orchestrator's context budget. The forked context prevents this: the skill runs in an isolated context of its own, does its heavy lifting there, and the main session's headroom is preserved. - -This is the same context-engineering principle as the fresh-context subagent model — applied not at the session boundary, but at the skill-invocation boundary. The difference is one of granularity. The phase loop spawns fresh subagents to protect each agent from its siblings' noise. Forked context protects the orchestrating session from the skill's own accumulated noise while the skill runs. +Note: an earlier version of GSD also applied `context: fork` to these three heavy skills to protect the main session's context budget. This was removed (#921) because `plan-phase`, `execute-phase`, and `autonomous` are **spawning orchestrators** — their core function is to spawn subagents (`gsd-planner`, `gsd-executor`, etc.), and a forked subagent context does not have the `Agent` tool. Context isolation for these skills comes from the subagents they spawn, not from forking the orchestrator itself. Complementing this, quick-status skills explicitly declare low effort in their definitions. This is a budget-conscious signal in the opposite direction: these skills read minimal state and return concise output, keeping their own footprint small by design. @@ -108,7 +106,7 @@ This machinery is worth being honest about. **Headroom tracking is a heuristic.** The hooks give GSD a signal, not a guarantee. A single model call can consume tokens unpredictably depending on the response length, tool use, and caching behaviour. GSD uses headroom estimates to warn and steer, not to make hard guarantees about what will fit. -**Forked context is isolated.** The forked work cannot see uncommitted state in the main session. This is not a bug — it is necessary for isolation — but it means anything the forked skill needs to know must be on disk before the fork occurs. This is precisely why `.planning/` exists as the shared substrate: plan files, `STATE.md`, `CONTEXT.md`, and `config.json` are all durable, file-system artefacts that any context — main or forked — can read. The context-engineering design is self-consistent: the same principle that makes fresh-context subagents work (shared state lives in files, not in a conversation) is what makes forked context viable. See also [Multi-agent orchestration](multi-agent-orchestration.md) for how `.planning/` serves the same role across the orchestrator → agent boundary. +**Subagents are isolated.** A spawned subagent cannot see uncommitted state in the orchestrating session. This is not a bug — it is necessary for independence — but it means anything the subagent needs must be on disk before it is spawned. This is precisely why `.planning/` exists as the shared substrate: plan files, `STATE.md`, `CONTEXT.md`, and `config.json` are all durable, file-system artefacts that any context — orchestrator or subagent — can read. The context-engineering design is self-consistent: the same principle that makes fresh-context subagents work (shared state lives in files, not in a conversation) is what makes the multi-agent architecture viable. See also [Multi-agent orchestration](multi-agent-orchestration.md) for how `.planning/` serves the same role across the orchestrator → agent boundary. --- diff --git a/gsd-core/workflows/plan-phase.md b/gsd-core/workflows/plan-phase.md index caf56c32a..ec0001033 100644 --- a/gsd-core/workflows/plan-phase.md +++ b/gsd-core/workflows/plan-phase.md @@ -46,10 +46,13 @@ discuss-phase early-exit path. It does NOT authorize inline role performance for plan-phase agents. **Other runtimes:** -If the Agent tool is genuinely absent (e.g. a backgrounded Claude Code agent per -#853, or a non-Claude runtime that does not expose Agent/agent), log the gap and -stop — do NOT perform researcher/planner/checker roles inline. Independent agent -contexts are required for the plan-checker gate to be meaningful. +Do not pre-judge Agent availability by introspection. Always attempt the actual +Agent() call for gsd-phase-researcher, gsd-planner, and gsd-plan-checker. Only +a real tool-unavailable error returned by Agent() is a reliable absence signal — +never stop based on a self-assessed "I think Agent is unavailable." If the call +fails with a tool-unavailable error, log the gap and stop — do NOT collapse +researcher/planner/checker roles inline. Independent agent contexts are required +for the plan-checker gate to be meaningful. diff --git a/tests/enh-769-context-fork-effort.install.test.cjs b/tests/enh-769-context-fork-effort.install.test.cjs index 422128ec7..a7a658592 100644 --- a/tests/enh-769-context-fork-effort.install.test.cjs +++ b/tests/enh-769-context-fork-effort.install.test.cjs @@ -4,20 +4,28 @@ // transformation is asserted — not inspected for string presence. /** - * #769 — context:fork + effort: frontmatter on heavy workflow skills. + * #769 — effort: frontmatter on heavy workflow skills. + * #921 — spawning orchestrators must NOT carry context: fork. + * + * Context: context:fork was added by #769 to protect context budget, but + * plan-phase, execute-phase, and autonomous are spawning orchestrators — a + * forked subagent has no Agent/Task tool, breaking their core function. + * effort: xhigh is preserved; context: fork is removed from these three. + * The converter still passes context: fork through if a source file has it + * (for any future leaf skill that legitimately needs isolation). * * Verifies: - * 1. Source commands/gsd/autonomous.md has context: fork and effort: xhigh - * 2. Source commands/gsd/execute-phase.md has context: fork and effort: xhigh - * 3. Source commands/gsd/plan-phase.md has context: fork and effort: xhigh + * 1. Source commands/gsd/autonomous.md does NOT have context: fork, has effort: xhigh + * 2. Source commands/gsd/execute-phase.md does NOT have context: fork, has effort: xhigh + * 3. Source commands/gsd/plan-phase.md does NOT have context: fork, has effort: xhigh * 4. Source commands/gsd/progress.md has effort: low * 5. Source commands/gsd/stats.md has effort: low - * 6. Claude global install: SKILL.md for autonomous has context: fork and effort: xhigh - * 7. Claude global install: SKILL.md for execute-phase has context: fork and effort: xhigh - * 8. Claude global install: SKILL.md for plan-phase has context: fork and effort: xhigh + * 6. Claude global install: SKILL.md for autonomous has effort: xhigh, NOT context: fork + * 7. Claude global install: SKILL.md for execute-phase has effort: xhigh, NOT context: fork + * 8. Claude global install: SKILL.md for plan-phase has effort: xhigh, NOT context: fork * 9. Claude global install: SKILL.md for progress has effort: low * 10. Claude global install: SKILL.md for stats has effort: low - * 11. convertClaudeCommandToClaudeSkill preserves context: fork field + * 11. convertClaudeCommandToClaudeSkill still passes context: fork through (for non-orchestrator skills) * 12. convertClaudeCommandToClaudeSkill preserves effort: field */ @@ -91,11 +99,15 @@ function runClaudeGlobalInstall(claudeHome) { // ─── describe 1: Source command files have correct frontmatter ──────────────── -describe('#769 source commands: heavy skills have context: fork and effort: xhigh', () => { - test('commands/gsd/autonomous.md has context: fork', () => { +// #921/#922: spawning orchestrators must NOT carry context: fork — a forked +// subagent has no Agent/Task tool, making it impossible for orchestrators to +// spawn their required subagents. context: fork is appropriate only for leaf +// skills that do not themselves dispatch agents. effort: xhigh is preserved. +describe('#769/#921 source commands: spawning orchestrators have effort: xhigh but NOT context: fork', () => { + test('commands/gsd/autonomous.md does NOT have context: fork (#921)', () => { const fm = readFrontmatter(path.join(SOURCE_COMMANDS_DIR, 'autonomous.md')); - assert.match(fm, /^context:[ \t]*fork$/m, - `autonomous.md frontmatter must have context: fork\nActual:\n${fm}`); + assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, + `autonomous.md is a spawning orchestrator and must NOT have context: fork (#921)\nActual:\n${fm}`); }); test('commands/gsd/autonomous.md has effort: xhigh', () => { @@ -104,10 +116,10 @@ describe('#769 source commands: heavy skills have context: fork and effort: xhig `autonomous.md frontmatter must have effort: xhigh\nActual:\n${fm}`); }); - test('commands/gsd/execute-phase.md has context: fork', () => { + test('commands/gsd/execute-phase.md does NOT have context: fork (#921)', () => { const fm = readFrontmatter(path.join(SOURCE_COMMANDS_DIR, 'execute-phase.md')); - assert.match(fm, /^context:[ \t]*fork$/m, - `execute-phase.md frontmatter must have context: fork\nActual:\n${fm}`); + assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, + `execute-phase.md is a spawning orchestrator and must NOT have context: fork (#921)\nActual:\n${fm}`); }); test('commands/gsd/execute-phase.md has effort: xhigh', () => { @@ -116,10 +128,10 @@ describe('#769 source commands: heavy skills have context: fork and effort: xhig `execute-phase.md frontmatter must have effort: xhigh\nActual:\n${fm}`); }); - test('commands/gsd/plan-phase.md has context: fork', () => { + test('commands/gsd/plan-phase.md does NOT have context: fork (#921)', () => { const fm = readFrontmatter(path.join(SOURCE_COMMANDS_DIR, 'plan-phase.md')); - assert.match(fm, /^context:[ \t]*fork$/m, - `plan-phase.md frontmatter must have context: fork\nActual:\n${fm}`); + assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, + `plan-phase.md is a spawning orchestrator and must NOT have context: fork (#921)\nActual:\n${fm}`); }); test('commands/gsd/plan-phase.md has effort: xhigh', () => { @@ -238,7 +250,9 @@ describe('#769 convertClaudeCommandToClaudeSkill: preserves context and effort f // ─── describe 3: Claude global install — SKILL.md files include new fields ──── -describe('#769 Claude global install: SKILL.md files preserve context: fork and effort:', () => { +// #921/#922: after install, spawning orchestrators must NOT carry context: fork +// in their emitted SKILL.md. effort: xhigh is still emitted (preserved from source). +describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files have effort: xhigh but NOT context: fork', () => { let tmpDir; let claudeHome; @@ -252,12 +266,12 @@ describe('#769 Claude global install: SKILL.md files preserve context: fork and cleanup(tmpDir); }); - test('gsd-autonomous SKILL.md has context: fork after global install', () => { + test('gsd-autonomous SKILL.md does NOT have context: fork after global install (#921)', () => { runClaudeGlobalInstall(claudeHome); const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'autonomous'); const fm = readFrontmatter(skillPath); - assert.match(fm, /^context:[ \t]*fork$/m, - `gsd-autonomous SKILL.md must have context: fork\nActual:\n${fm}`); + assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, + `gsd-autonomous is a spawning orchestrator; its SKILL.md must NOT have context: fork (#921)\nActual:\n${fm}`); }); test('gsd-autonomous SKILL.md has effort: xhigh after global install', () => { @@ -268,12 +282,12 @@ describe('#769 Claude global install: SKILL.md files preserve context: fork and `gsd-autonomous SKILL.md must have effort: xhigh\nActual:\n${fm}`); }); - test('gsd-execute-phase SKILL.md has context: fork after global install', () => { + test('gsd-execute-phase SKILL.md does NOT have context: fork after global install (#921)', () => { runClaudeGlobalInstall(claudeHome); const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'execute-phase'); const fm = readFrontmatter(skillPath); - assert.match(fm, /^context:[ \t]*fork$/m, - `gsd-execute-phase SKILL.md must have context: fork\nActual:\n${fm}`); + assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, + `gsd-execute-phase is a spawning orchestrator; its SKILL.md must NOT have context: fork (#921)\nActual:\n${fm}`); }); test('gsd-execute-phase SKILL.md has effort: xhigh after global install', () => { @@ -284,12 +298,12 @@ describe('#769 Claude global install: SKILL.md files preserve context: fork and `gsd-execute-phase SKILL.md must have effort: xhigh\nActual:\n${fm}`); }); - test('gsd-plan-phase SKILL.md has context: fork after global install', () => { + test('gsd-plan-phase SKILL.md does NOT have context: fork after global install (#921)', () => { runClaudeGlobalInstall(claudeHome); const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'plan-phase'); const fm = readFrontmatter(skillPath); - assert.match(fm, /^context:[ \t]*fork$/m, - `gsd-plan-phase SKILL.md must have context: fork\nActual:\n${fm}`); + assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, + `gsd-plan-phase is a spawning orchestrator; its SKILL.md must NOT have context: fork (#921)\nActual:\n${fm}`); }); test('gsd-plan-phase SKILL.md has effort: xhigh after global install', () => { diff --git a/tests/plan-phase-drift-guard.test.cjs b/tests/plan-phase-drift-guard.test.cjs index 9ef1e5bf3..3ae66f09d 100644 --- a/tests/plan-phase-drift-guard.test.cjs +++ b/tests/plan-phase-drift-guard.test.cjs @@ -193,3 +193,70 @@ describe('plan-phase workflow: top-level spawn guard (#913)', () => { ); }); }); + +// ─── (D) Attempt-based Agent gate (#922) ───────────────────────────────────── + +describe('plan-phase workflow: attempt-based Agent availability gate (#922)', () => { + // Extract the runtime_compatibility block for targeted assertions + const rtBlock = (() => { + const m = workflow.match(/([\s\S]*?)<\/runtime_compatibility>/); + return m ? m[1] : ''; + })(); + + // Extract the "Other runtimes" clause specifically + const otherRuntimesClause = (() => { + const m = rtBlock.match(/\*\*Other runtimes[^*]*\*\*[^\n]*\n([\s\S]*?)(?=\n\*\*|$)/); + return m ? m[0] : rtBlock; + })(); + + test('Other runtimes clause does not authorize stopping on a self-assessed absence (#922)', () => { + // The pre-#922 wording ("if the Agent tool is genuinely absent") let the model + // self-assess and stop without ever attempting a call. The fixed wording must + // not contain phrasing that authorizes that pattern. + const forbiddenPatterns = [ + /if the Agent tool is genuinely absent/i, + /if.*Agent.*genuinely absent/i, + ]; + for (const pattern of forbiddenPatterns) { + assert.ok( + !pattern.test(otherRuntimesClause), + `plan-phase "Other runtimes" clause must not authorize stopping on a self-assessed Agent absence — ` + + `use attempt-based gate instead (#922). Found: ${otherRuntimesClause.trim()}` + ); + } + }); + + test('Other runtimes clause pins "Always attempt the actual Agent() call" language (#922)', () => { + // Pin the exact contract phrase so a future edit that changes to "try to determine + // availability" or "check if Agent is available" does not silently reintroduce introspection. + assert.ok( + otherRuntimesClause.includes('Always attempt the actual') || + otherRuntimesClause.includes('always attempt the actual'), + `plan-phase "Other runtimes" clause must pin "Always attempt the actual Agent() call" (or equivalent) (#922). ` + + `Found: ${otherRuntimesClause.trim()}` + ); + }); + + test('Other runtimes clause pins "real tool-unavailable error" as the only valid stop signal (#922)', () => { + // Must tie the stop to a real returned error, not a self-assessed absence. + assert.ok( + otherRuntimesClause.includes('real tool-unavailable error') || + otherRuntimesClause.includes('tool-unavailable error returned'), + `plan-phase "Other runtimes" clause must state only a real tool-unavailable error from Agent() authorizes stopping (#922). ` + + `Found: ${otherRuntimesClause.trim()}` + ); + }); + + test('Other runtimes clause still prohibits inline role collapse (#922 preserves #913)', () => { + // Even after the attempt-based rewrite the clause must keep the no-inline-collapse guard. + const hasNoInline = + otherRuntimesClause.toLowerCase().includes('do not') && + (otherRuntimesClause.toLowerCase().includes('inline') || + otherRuntimesClause.toLowerCase().includes('collapse')); + assert.ok( + hasNoInline, + `plan-phase "Other runtimes" clause must still prohibit inline role collapse even with the attempt-based gate (#922). ` + + `Found: ${otherRuntimesClause.trim()}` + ); + }); +}); diff --git a/tests/workflow-size-budget.test.cjs b/tests/workflow-size-budget.test.cjs index 7713b2be7..c8b96bd9d 100644 --- a/tests/workflow-size-budget.test.cjs +++ b/tests/workflow-size-budget.test.cjs @@ -82,7 +82,7 @@ const GRACE = 3000; // XL high-water mark is execute-phase.md — note that under LINES it was // plan-phase; bytes genuinely re-rank the tier, which is the point of #717. // actualMax=92525 (execute-phase, #913 inline-fallback scope clarification); -// slack=475 ≤ GRACE. plan-phase.md=90501 (#913 runtime_compatibility block + label rename), new-project.md=58110. +// slack=475 ≤ GRACE. plan-phase.md=90748 (#922 attempt-based Agent gate), new-project.md=58110. const XL_BUDGET = 93000; // LARGE high-water mark is docs-update.md. actualMax=54410 (#891 launcher shim expansion); // slack=1590 ≤ GRACE. quick.md=45710, autonomous.md=38030. @@ -96,7 +96,7 @@ const DEFAULT_BUDGET = 40000; // pattern that future shrinks should follow. Byte counts noted for reference. const XL_WORKFLOWS = new Set([ 'execute-phase', // 92525 bytes (tier high-water mark; grew in #913 inline-fallback scope clarification) - 'plan-phase', // 90501 bytes (grew in #913 runtime_compatibility block + label rename) + 'plan-phase', // 90748 bytes (grew in #922 attempt-based Agent gate) 'new-project', // 55850 bytes ]); From 03d9fcdd65e4cf9754b39ce93baf380f6d7d259b Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 9 Jun 2026 08:59:20 -0400 Subject: [PATCH 3/4] fix(#924): revert Claude to flat skill layout so concrete skills are discoverable (#928) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #883 nested Claude skills 3 levels deep under gsd-ns-*/skills//SKILL.md. Claude Code's Skill tool scans only one level under ~/.claude/skills/ — nested concrete skills were never listed and Skill(skill="gsd-plan-phase") calls failed. Revert to flat layout: all ~61 concrete skills at ~/.claude/skills/gsd-/SKILL.md. The 6 other runtimes confirmed as non-recursive scanners (cline, qwen, hermes, augment, trae, antigravity) retain their nested layout — only Claude changes. Tradeoff: ~61 top-level skill dirs return to the flat install, but they are discoverable and invokable. Nested concretes were invisible to the Skill tool entirely. Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 --- .changeset/924-claude-flat-skill-layout.md | 5 + src/runtime-artifact-layout.cts | 10 +- tests/bug-2808-skill-hyphen-name.test.cjs | 3 +- .../bug-924-claude-flat-skill-layout.test.cjs | 189 ++++++++++++++++++ ...h-769-context-fork-effort.install.test.cjs | 23 ++- tests/install-nested-layout.test.cjs | 23 ++- tests/issue-69-surface-keeps-nested.test.cjs | 59 +++++- tests/runtime-artifact-layout.test.cjs | 36 ++-- 8 files changed, 300 insertions(+), 48 deletions(-) create mode 100644 .changeset/924-claude-flat-skill-layout.md create mode 100644 tests/bug-924-claude-flat-skill-layout.test.cjs diff --git a/.changeset/924-claude-flat-skill-layout.md b/.changeset/924-claude-flat-skill-layout.md new file mode 100644 index 000000000..2fcfb9d7b --- /dev/null +++ b/.changeset/924-claude-flat-skill-layout.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 924 +--- +**Claude global install reverted to flat skill layout so concrete skills are discoverable.** PR #883 introduced nested skill layout for Claude (`~/.claude/skills/gsd-ns-/skills//SKILL.md`), but Claude Code's skill discovery scans only one level under `~/.claude/skills/` — nested concrete skills were never listed in the Skill-tool available-skills list and direct `Skill(skill="gsd-plan-phase")` calls stopped working. This fix reverts Claude to the flat layout (`~/.claude/skills/gsd-/SKILL.md`) so all ~61 concrete skills are top-level and immediately discoverable. The 6 other runtimes that confirmed non-recursive scanning (cline, qwen, hermes, augment, trae, antigravity) retain their nested layout. (#924) diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index 18d37eab0..edc1868cf 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -281,8 +281,6 @@ function convertedCommandsKind( // flat conservatively. Verified June 2026: // // NEST (confirmed non-recursive / one-level scan): -// claude — https://code.claude.com/docs/en/skills + anthropics/claude-code#28266 -// (scans one level under ~/.claude/skills; nested skills not auto-listed) // cline — cline/cline skills.ts scanSkillsDirectory uses flat fs.readdir // qwen — QwenLM/qwen-code skill-load.ts flat readdir ("depth 2 enough") // hermes — hermes-agent.nousresearch.com/docs/user-guide/features/skills @@ -296,6 +294,12 @@ function convertedCommandsKind( // opencode — sst/opencode skill/index.ts glob "skills/**/SKILL.md" // kilo — Kilo-Org/kilocode (opencode fork, same ** glob) // +// FLAT (reverted from nested — nested skills not discoverable by Skill tool, #924): +// claude — https://code.claude.com/docs/en/skills + anthropics/claude-code#28266 +// (one-level scan under ~/.claude/skills — but Skill-tool errors on unknown +// names rather than re-routing via the router; concrete skills must be +// at the top level so Skill(skill="gsd-plan-phase") succeeds) +// // FLAT (nested-scan behaviour unconfirmed → conservative): // codex — developers.openai.com/codex/skills/ // copilot — docs.github.com/en/copilot/concepts/agents/about-agent-skills @@ -325,7 +329,7 @@ function resolveRuntimeArtifactLayout(runtime: string, configDir: string, scope: agentsKind('agents', 'gsd-', configDir), ]; } else { - kinds = [skillsKind('skills', 'gsd-', 'convertClaudeCommandToClaudeSkill', 'claude', configDir, true /* #69 nested: non-recursive scan, see matrix above */)]; + kinds = [skillsKind('skills', 'gsd-', 'convertClaudeCommandToClaudeSkill', 'claude', configDir)]; } break; diff --git a/tests/bug-2808-skill-hyphen-name.test.cjs b/tests/bug-2808-skill-hyphen-name.test.cjs index 88b1a01be..db95ef051 100644 --- a/tests/bug-2808-skill-hyphen-name.test.cjs +++ b/tests/bug-2808-skill-hyphen-name.test.cjs @@ -175,7 +175,8 @@ describe('bug-2808: SKILL.md name: uses hyphen form', () => { // Use the real COMMANDS_DIR as the source via .gsd-source marker. // installRuntimeArtifacts('claude', configDir, 'global') writes to // configDir/skills/ using the same converter as the shim did. - // With the full profile, skills are nested: gsd-ns-/skills//SKILL.md + // With the full profile (#924 fix), skills are FLAT: gsd-/SKILL.md + // (nested layout reverted for Claude — Claude Code scans only one level). const configDir = path.join(tmp, 'config'); fs.mkdirSync(configDir, { recursive: true }); fs.writeFileSync(path.join(configDir, '.gsd-source'), COMMANDS_DIR + '\n'); diff --git a/tests/bug-924-claude-flat-skill-layout.test.cjs b/tests/bug-924-claude-flat-skill-layout.test.cjs new file mode 100644 index 000000000..9f4436184 --- /dev/null +++ b/tests/bug-924-claude-flat-skill-layout.test.cjs @@ -0,0 +1,189 @@ +// allow-test-rule: source-text-is-the-product +// Reads installed SKILL.md files from a real install run — +// testing their on-disk layout tests the deployed contract. + +/** + * Regression test for bug #924. + * + * PR #883 accidentally nested concrete gsd-* skills 3 levels deep for the + * Claude global install: + * + * ~/.claude/skills/gsd-ns-/skills//SKILL.md + * + * Claude Code's skills discovery scans only ONE level under ~/.claude/skills/, + * so nested concretes were never listed in the Skill-tool available-skills list. + * Direct `Skill(skill="gsd-plan-phase")` calls stopped working. + * + * Fix: revert Claude to the FLAT layout — concrete skills at the top level: + * + * ~/.claude/skills/gsd-/SKILL.md + * + * The 6 ns-* routers are also top-level entries in the flat layout (they are + * concrete skills themselves). No nested skills/ subdirs for Claude. + * + * Other 6 runtimes (cline, qwen, hermes, augment, trae, antigravity) stay nested. + */ + +'use strict'; + +process.env.GSD_TEST_MODE = '1'; + +const { describe, test, before, after } = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); +const os = require('node:os'); + +const ROOT = path.join(__dirname, '..'); +const COMMANDS_GSD = path.join(ROOT, 'commands', 'gsd'); + +const { installRuntimeArtifacts } = require('../bin/install.js'); +const { cleanup } = require('./helpers.cjs'); +const { + loadSkillsManifest, + resolveProfile, +} = require('../gsd-core/bin/lib/install-profiles.cjs'); +const { applySurface } = require('../gsd-core/bin/lib/surface.cjs'); +const { resolveRuntimeArtifactLayout } = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + +const MANIFEST = loadSkillsManifest(COMMANDS_GSD); +const RESOLVED_FULL = resolveProfile({ modes: ['full'], manifest: MANIFEST }); + +// --------------------------------------------------------------------------- +// #924 regression: Claude global install must use FLAT layout +// --------------------------------------------------------------------------- + +describe('bug-924: claude global install uses flat skill layout (concrete skills discoverable)', () => { + let tmpDir; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-924-claude-flat-')); + installRuntimeArtifacts('claude', tmpDir, 'global', RESOLVED_FULL); + }); + + after(() => { + if (tmpDir) { + try { cleanup(tmpDir); } catch { /* best-effort */ } + } + }); + + test('claude global: concrete skills are at the TOP LEVEL of skills/ (flat, directly discoverable)', () => { + const skillsDir = path.join(tmpDir, 'skills'); + assert.ok(fs.existsSync(skillsDir), `skills/ dir must exist under ${tmpDir}`); + + const topLevel = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); + + // Flat layout must have MANY more than 6 top-level gsd-* entries (concrete skills). + // Pre-#924-fix nested layout had exactly 6 (only routers). Flat must have >= 60. + assert.ok( + topLevel.length >= 60, + `Claude global must have >= 60 gsd-* top-level skill dirs (concrete flat layout). ` + + `Got ${topLevel.length}: [${topLevel.slice(0, 10).join(', ')}${topLevel.length > 10 ? ', …' : ''}]. ` + + 'Nested layout detected — #924 regression: Claude must be flat.', + ); + }); + + test('claude global: gsd-plan-phase is directly at the top level of skills/', () => { + const skillsDir = path.join(tmpDir, 'skills'); + const planPhaseDir = path.join(skillsDir, 'gsd-plan-phase'); + assert.ok( + fs.existsSync(path.join(planPhaseDir, 'SKILL.md')), + `skills/gsd-plan-phase/SKILL.md must exist at top level for Claude global install. ` + + 'Concrete skill buried in nested layout — #924 regression.', + ); + }); + + test('claude global: gsd-execute-phase is directly at the top level of skills/', () => { + const skillsDir = path.join(tmpDir, 'skills'); + assert.ok( + fs.existsSync(path.join(skillsDir, 'gsd-execute-phase', 'SKILL.md')), + `skills/gsd-execute-phase/SKILL.md must exist at top level for Claude global install.`, + ); + }); + + test('claude global: gsd-code-review is directly at the top level of skills/', () => { + const skillsDir = path.join(tmpDir, 'skills'); + assert.ok( + fs.existsSync(path.join(skillsDir, 'gsd-code-review', 'SKILL.md')), + `skills/gsd-code-review/SKILL.md must exist at top level for Claude global install.`, + ); + }); + + test('claude global: gsd-ns-workflow is at the top level as a concrete skill (no nested skills/ subdir)', () => { + const skillsDir = path.join(tmpDir, 'skills'); + const nsWorkflowDir = path.join(skillsDir, 'gsd-ns-workflow'); + assert.ok( + fs.existsSync(path.join(nsWorkflowDir, 'SKILL.md')), + `skills/gsd-ns-workflow/SKILL.md must exist at top level (router as concrete skill).`, + ); + + // In the FLAT layout, gsd-ns-workflow/ must NOT have a skills/ subdir. + // A skills/ subdir means nested layout was applied (the #924 regression). + assert.ok( + !fs.existsSync(path.join(nsWorkflowDir, 'skills')), + `skills/gsd-ns-workflow/skills/ must NOT exist in flat layout (nested layout detected — #924 regression).`, + ); + }); + + test('claude global: no concrete skill is nested under gsd-ns-*/skills//SKILL.md', () => { + const skillsDir = path.join(tmpDir, 'skills'); + const topLevel = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-ns-')); + + for (const nsDir of topLevel) { + const nestedSkillsDir = path.join(skillsDir, nsDir, 'skills'); + assert.ok( + !fs.existsSync(nestedSkillsDir), + `${nsDir}/skills/ must NOT exist in Claude flat layout (#924 regression: nested layout detected).`, + ); + } + }); +}); + +// --------------------------------------------------------------------------- +// #924 regression: applySurface on Claude must also preserve flat layout +// (no re-nesting after surface update) +// --------------------------------------------------------------------------- + +describe('bug-924: applySurface on claude preserves flat layout (no re-nesting)', () => { + let tmpDir; + + before(() => { + tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-924-surface-')); + installRuntimeArtifacts('claude', tmpDir, 'global', RESOLVED_FULL); + }); + + after(() => { + if (tmpDir) { + try { cleanup(tmpDir); } catch { /* best-effort */ } + } + }); + + test('claude: applySurface keeps concrete skills at the top level (flat, no re-nesting)', () => { + const skillsDir = path.join(tmpDir, 'skills'); + + // Sanity: install must produce flat layout (>= 60 top-level gsd-* dirs) + const topLevelAfterInstall = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); + assert.ok( + topLevelAfterInstall.length >= 60, + `Install must produce flat layout with >= 60 gsd-* dirs. Got ${topLevelAfterInstall.length}.`, + ); + + // Run applySurface (full surface → full profile) + const layout = resolveRuntimeArtifactLayout('claude', tmpDir, 'global'); + applySurface(tmpDir, layout, MANIFEST); + + // After applySurface: still flat + const topLevelAfterSurface = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); + assert.ok( + topLevelAfterSurface.length >= 60, + `After applySurface: must still have >= 60 gsd-* top-level dirs (flat). ` + + `Got ${topLevelAfterSurface.length}. Re-nesting detected.`, + ); + + // gsd-plan-phase must remain directly accessible + assert.ok( + fs.existsSync(path.join(skillsDir, 'gsd-plan-phase', 'SKILL.md')), + 'After applySurface: gsd-plan-phase/SKILL.md must remain at top level.', + ); + }); +}); diff --git a/tests/enh-769-context-fork-effort.install.test.cjs b/tests/enh-769-context-fork-effort.install.test.cjs index a7a658592..c2c69b5ff 100644 --- a/tests/enh-769-context-fork-effort.install.test.cjs +++ b/tests/enh-769-context-fork-effort.install.test.cjs @@ -41,7 +41,12 @@ const os = require('node:os'); const { install, convertClaudeCommandToClaudeSkill } = require('../bin/install.js'); const { cleanup } = require('./helpers.cjs'); -const { nestedSkillPath } = require('./helpers/nested-layout.cjs'); + +// #924: Claude global install is now FLAT — concrete skills are at the top level. +// flatSkillPath returns: /gsd-/SKILL.md +function flatSkillPath(skillsRoot, stem) { + return path.join(skillsRoot, `gsd-${stem}`, 'SKILL.md'); +} const REPO_ROOT = path.resolve(__dirname, '..'); const SOURCE_COMMANDS_DIR = path.join(REPO_ROOT, 'commands', 'gsd'); @@ -268,7 +273,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-autonomous SKILL.md does NOT have context: fork after global install (#921)', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'autonomous'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'autonomous'); const fm = readFrontmatter(skillPath); assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, `gsd-autonomous is a spawning orchestrator; its SKILL.md must NOT have context: fork (#921)\nActual:\n${fm}`); @@ -276,7 +281,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-autonomous SKILL.md has effort: xhigh after global install', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'autonomous'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'autonomous'); const fm = readFrontmatter(skillPath); assert.match(fm, /^effort:[ \t]*xhigh$/m, `gsd-autonomous SKILL.md must have effort: xhigh\nActual:\n${fm}`); @@ -284,7 +289,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-execute-phase SKILL.md does NOT have context: fork after global install (#921)', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'execute-phase'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'execute-phase'); const fm = readFrontmatter(skillPath); assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, `gsd-execute-phase is a spawning orchestrator; its SKILL.md must NOT have context: fork (#921)\nActual:\n${fm}`); @@ -292,7 +297,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-execute-phase SKILL.md has effort: xhigh after global install', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'execute-phase'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'execute-phase'); const fm = readFrontmatter(skillPath); assert.match(fm, /^effort:[ \t]*xhigh$/m, `gsd-execute-phase SKILL.md must have effort: xhigh\nActual:\n${fm}`); @@ -300,7 +305,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-plan-phase SKILL.md does NOT have context: fork after global install (#921)', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'plan-phase'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'plan-phase'); const fm = readFrontmatter(skillPath); assert.doesNotMatch(fm, /^context:[ \t]*fork$/m, `gsd-plan-phase is a spawning orchestrator; its SKILL.md must NOT have context: fork (#921)\nActual:\n${fm}`); @@ -308,7 +313,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-plan-phase SKILL.md has effort: xhigh after global install', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'plan-phase'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'plan-phase'); const fm = readFrontmatter(skillPath); assert.match(fm, /^effort:[ \t]*xhigh$/m, `gsd-plan-phase SKILL.md must have effort: xhigh\nActual:\n${fm}`); @@ -316,7 +321,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-progress SKILL.md has effort: low after global install', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'progress'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'progress'); const fm = readFrontmatter(skillPath); assert.match(fm, /^effort:[ \t]*low$/m, `gsd-progress SKILL.md must have effort: low\nActual:\n${fm}`); @@ -324,7 +329,7 @@ describe('#769/#921 Claude global install: spawning-orchestrator SKILL.md files test('gsd-stats SKILL.md has effort: low after global install', () => { runClaudeGlobalInstall(claudeHome); - const skillPath = nestedSkillPath(path.join(claudeHome, 'skills'), 'gsd-', 'stats'); + const skillPath = flatSkillPath(path.join(claudeHome, 'skills'),'stats'); const fm = readFrontmatter(skillPath); assert.match(fm, /^effort:[ \t]*low$/m, `gsd-stats SKILL.md must have effort: low\nActual:\n${fm}`); diff --git a/tests/install-nested-layout.test.cjs b/tests/install-nested-layout.test.cjs index f3a9fd50f..89bd00cc5 100644 --- a/tests/install-nested-layout.test.cjs +++ b/tests/install-nested-layout.test.cjs @@ -32,7 +32,8 @@ const { COMMANDS_GSD, ROUTER_STEMS, routerChildren } = require('./helpers/nested // --------------------------------------------------------------------------- const NEST = [ - { runtime: 'claude', scope: 'global', skillsSub: 'skills', prefix: 'gsd-' }, + // Claude reverted to flat (#924: nested layout breaks Skill-tool discovery on Claude Code). + // Only the 6 runtimes below keep the nested layout. { runtime: 'cline', scope: 'global', skillsSub: 'skills', prefix: 'gsd-' }, { runtime: 'qwen', scope: 'global', skillsSub: 'skills', prefix: 'gsd-' }, { runtime: 'hermes', scope: 'global', skillsSub: 'skills/gsd', prefix: '' }, @@ -42,6 +43,9 @@ const NEST = [ ]; const FLAT = [ + // Claude reverted to flat (#924): Claude Code scans only one level under ~/.claude/skills/ + // so nested concretes were never discoverable by the Skill tool. + { runtime: 'claude', scope: 'global', skillsSub: 'skills' }, { runtime: 'cursor', scope: 'global', skillsSub: 'skills' }, { runtime: 'codex', scope: 'global', skillsSub: 'skills' }, { runtime: 'copilot', scope: 'global', skillsSub: 'skills' }, @@ -200,10 +204,13 @@ for (const { runtime, scope, skillsSub, prefix } of NEST) { } // --------------------------------------------------------------------------- -// claude extra: total top-level gsd- count must equal exactly 6 +// claude extra: total top-level gsd- count must be >= 60 (FLAT, #924) +// +// Pre-#924 (nested) this block asserted exactly 6 (only routers). +// Post-#924 (flat) Claude has all concrete skills at the top level. // --------------------------------------------------------------------------- -describe('claude: total top-level gsd- entries == 6', () => { +describe('claude: total top-level gsd- entries >= 60 (flat layout, #924)', () => { let tmpDir; before(() => { @@ -216,15 +223,15 @@ describe('claude: total top-level gsd- entries == 6', () => { } }); - test('claude: total top-level gsd- skill entries == 6', () => { + test('claude: >= 60 gsd-* top-level skill entries (concrete flat layout, not nested)', () => { const skillsDir = path.join(tmpDir, 'skills'); assert.ok(fs.existsSync(skillsDir), 'skills/ dir must exist'); const topLevel = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); - assert.strictEqual( - topLevel.length, - 6, - `Expected exactly 6 gsd-* top-level entries under claude/skills, got ${topLevel.length}: [${topLevel.join(', ')}]`, + assert.ok( + topLevel.length >= 60, + `Expected >= 60 gsd-* top-level entries under claude/skills (flat layout after #924 fix). ` + + `Got ${topLevel.length}: [${topLevel.slice(0, 10).join(', ')}${topLevel.length > 10 ? ', …' : ''}]`, ); }); }); diff --git a/tests/issue-69-surface-keeps-nested.test.cjs b/tests/issue-69-surface-keeps-nested.test.cjs index 3fa38d32b..ec10dcb2d 100644 --- a/tests/issue-69-surface-keeps-nested.test.cjs +++ b/tests/issue-69-surface-keeps-nested.test.cjs @@ -8,6 +8,10 @@ // // Fix (install-profiles.cts): gate nesting on full OR full-equivalent (all routerStems // present in the concrete Set) so that the surface path preserves nesting. +// +// NOTE: As of #924 Claude has been REVERTED to FLAT. This test now uses Cline as the +// representative nested runtime. The original claude-global test below is updated to +// assert the flat layout (>= 60 top-level gsd-* entries, concrete skills discoverable). 'use strict'; @@ -29,18 +33,19 @@ const { resolveRuntimeArtifactLayout } = require('../gsd-core/bin/lib/runtime-ar const { cleanup } = require('./helpers.cjs'); describe('issue-69: applySurface preserves nested skill layout (no re-flatten)', () => { - test('claude global full: applySurface keeps 6 router dirs and nested gsd-ns-workflow/skills/plan-phase/SKILL.md', (t) => { + // #924: Claude is now flat; use Cline as the representative nested runtime. + test('cline global full: applySurface keeps 6 router dirs and nested gsd-ns-manage/skills/help/SKILL.md', (t) => { const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-69-surface-')); t.after(() => { try { cleanup(tmpDir); } catch { /* best-effort */ } }); // Step 1: full install const manifest = loadSkillsManifest(COMMANDS_GSD); const resolved = resolveProfile({ modes: ['full'], manifest }); - installRuntimeArtifacts('claude', tmpDir, 'global', resolved); + installRuntimeArtifacts('cline', tmpDir, 'global', resolved); const skillsDir = path.join(tmpDir, 'skills'); - // Sanity: install must produce nested layout + // Sanity: install must produce nested layout (6 top-level router dirs) const topLevelAfterInstall = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); assert.strictEqual( topLevelAfterInstall.length, @@ -53,7 +58,7 @@ describe('issue-69: applySurface preserves nested skill layout (no re-flatten)', ); // Step 2: applySurface (full surface, no surface state file → resolves to full) - const layout = resolveRuntimeArtifactLayout('claude', tmpDir, 'global'); + const layout = resolveRuntimeArtifactLayout('cline', tmpDir, 'global'); applySurface(tmpDir, layout, manifest); // Step 3: assert nested layout is preserved after applySurface @@ -77,4 +82,50 @@ describe('issue-69: applySurface preserves nested skill layout (no re-flatten)', 'After applySurface: gsd-plan-phase/ must NOT exist at top level (#69 re-flatten regression guard)', ); }); + + // #924 companion: Claude must use FLAT layout and applySurface must NOT re-nest it. + test('claude global full: install produces flat layout and applySurface preserves it (#924)', (t) => { + const tmpDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-924-69-')); + t.after(() => { try { cleanup(tmpDir); } catch { /* best-effort */ } }); + + const manifest = loadSkillsManifest(COMMANDS_GSD); + const resolved = resolveProfile({ modes: ['full'], manifest }); + installRuntimeArtifacts('claude', tmpDir, 'global', resolved); + + const skillsDir = path.join(tmpDir, 'skills'); + + // Install must produce FLAT layout (>= 60 gsd-* dirs) + const topLevelAfterInstall = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); + assert.ok( + topLevelAfterInstall.length >= 60, + `Claude install must produce >= 60 gsd-* top-level dirs (flat, #924). Got ${topLevelAfterInstall.length}.`, + ); + + // gsd-plan-phase must be directly at top level + assert.ok( + fs.existsSync(path.join(skillsDir, 'gsd-plan-phase', 'SKILL.md')), + 'After claude install: gsd-plan-phase/SKILL.md must be at top level (flat layout, #924)', + ); + + // No nested skills/ subdirs under gsd-ns-* in Claude + assert.ok( + !fs.existsSync(path.join(skillsDir, 'gsd-ns-workflow', 'skills')), + 'After claude install: gsd-ns-workflow/skills/ must NOT exist (flat layout, no nesting, #924)', + ); + + // applySurface must preserve flat layout + const layout = resolveRuntimeArtifactLayout('claude', tmpDir, 'global'); + applySurface(tmpDir, layout, manifest); + + const topLevelAfterSurface = fs.readdirSync(skillsDir).filter((n) => n.startsWith('gsd-')); + assert.ok( + topLevelAfterSurface.length >= 60, + `After applySurface: claude must still have >= 60 gsd-* dirs (flat preserved). Got ${topLevelAfterSurface.length}.`, + ); + + assert.ok( + fs.existsSync(path.join(skillsDir, 'gsd-plan-phase', 'SKILL.md')), + 'After applySurface: gsd-plan-phase/SKILL.md must remain at top level (#924)', + ); + }); }); diff --git a/tests/runtime-artifact-layout.test.cjs b/tests/runtime-artifact-layout.test.cjs index 7e45474e8..e02f02c37 100644 --- a/tests/runtime-artifact-layout.test.cjs +++ b/tests/runtime-artifact-layout.test.cjs @@ -417,7 +417,7 @@ describe('stage — skills kind (claude global)', () => { assert.ok(entries.length >= 1, 'at least one skill dir should be staged'); }); - test('stage with skills="*" nests all commands/gsd/*.md under 6 routers (claude)', () => { + test('stage with skills="*" produces flat layout for claude (#924: reverted from nested)', () => { const layout = resolveRuntimeArtifactLayout('claude', FAKE_STAGE_DIR, 'global'); const skillsKind = layout.kinds.find(k => k.kind === 'skills'); assert.ok(skillsKind, 'should have a skills kind'); @@ -425,32 +425,22 @@ describe('stage — skills kind (claude global)', () => { const stagedDir = skillsKind.stage(PROFILE_FULL); assert.ok(fs.existsSync(stagedDir), 'stagedDir must exist'); - // Claude is a NESTING runtime: full profile produces exactly 6 gsd-ns-* router dirs. + // #924: Claude is reverted to FLAT. Full profile produces >= 60 top-level gsd-* dirs. + // (Previously nested: exactly 6 gsd-ns-* router dirs. That broke Skill-tool discovery.) const topEntries = fs.readdirSync(stagedDir); - assert.strictEqual(topEntries.length, 6, `full profile should have exactly 6 router dirs, got ${topEntries.length}`); + assert.ok( + topEntries.length >= 60, + `full profile should have >= 60 top-level skill dirs (flat layout, #924), got ${topEntries.length}`, + ); for (const entry of topEntries) { - assert.ok(entry.startsWith('gsd-ns-'), `top-level entry should be a gsd-ns-* router: ${entry}`); - // Each router has its own SKILL.md. - const routerSkillMd = path.join(stagedDir, entry, 'SKILL.md'); - assert.ok(fs.existsSync(routerSkillMd), `router SKILL.md must exist in ${entry}`); - // Each router has a skills/ subdirectory with nested children. + assert.ok(entry.startsWith('gsd-'), `entry should start with gsd-: ${entry}`); + // Each skill dir has its own SKILL.md at the top level. + const skillMd = path.join(stagedDir, entry, 'SKILL.md'); + assert.ok(fs.existsSync(skillMd), `SKILL.md must exist at top level in ${entry}`); + // No nested skills/ subdirectory: flat layout means no nesting. const skillsSubdir = path.join(stagedDir, entry, 'skills'); - assert.ok(fs.existsSync(skillsSubdir), `skills/ subdir must exist in ${entry}`); - assert.ok(fs.statSync(skillsSubdir).isDirectory(), `${entry}/skills must be a directory`); + assert.ok(!fs.existsSync(skillsSubdir), `skills/ subdir must NOT exist in ${entry} (flat layout, #924)`); } - - // Total SKILL.md files across all routers + nested children must be large (proves no skill was dropped). - function countSkillMdFiles(dir) { - let count = 0; - for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { - const fullPath = path.join(dir, entry.name); - if (entry.isDirectory()) count += countSkillMdFiles(fullPath); - else if (entry.name === 'SKILL.md') count++; - } - return count; - } - const totalSkillMd = countSkillMdFiles(stagedDir); - assert.ok(totalSkillMd >= 60, `full profile should have >= 60 total SKILL.md files (routers + children), got ${totalSkillMd}`); }); }); From 86845340dcdcb80b069999dc9b8066be05a0642f Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Tue, 9 Jun 2026 09:17:00 -0400 Subject: [PATCH 4/4] chore(#930): remove self-masking next dist-tag repoint from release finalize (#931) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * chore(#930): remove self-masking next dist-tag repoint from release finalize The "Clean up next dist-tag" step silently failed under OIDC trusted publishing (which can't write dist-tags) while unconditionally reporting success via || true + an echo. It also violated the release model by trying to repoint @next→stable; @next is managed exclusively by the rc job's --tag next publish. Closes #930 Co-Authored-By: Claude Sonnet 4.6 * docs: update ADR-660 to reflect removal of next dist-tag repoint The finalize job no longer runs `npm dist-tag add … next`; update the ADR-660 description of step 4 to match the new behavior — @next is managed exclusively by the rc job. Co-Authored-By: Claude Sonnet 4.6 --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Claude Sonnet 4.6 --- .github/workflows/release.yml | 10 ---------- docs/adr/660-release-from-next-head.md | 2 +- 2 files changed, 1 insertion(+), 11 deletions(-) diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 19867304a..4a0c6de93 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -642,16 +642,6 @@ jobs: --post \ --allow-missing-webhook - - name: Clean up next dist-tag - if: ${{ !inputs.dry_run }} - env: - VERSION: ${{ inputs.version }} - run: | - # Point next to the stable release so @next never returns something - # older than @latest. This prevents stale pre-release installs. - npm dist-tag add "@opengsd/gsd-core@${VERSION}" next 2>/dev/null || true - echo "✓ next dist-tag updated to v${VERSION}" - - name: Verify publish if: ${{ !inputs.dry_run }} env: diff --git a/docs/adr/660-release-from-next-head.md b/docs/adr/660-release-from-next-head.md index 9100940b0..ea531257c 100644 --- a/docs/adr/660-release-from-next-head.md +++ b/docs/adr/660-release-from-next-head.md @@ -79,7 +79,7 @@ or a movable tag — as the RC surface.** Concretely: 4. **RC = the `@next` dist-tag, full stop.** Testers run `npm i -g @opengsd/gsd-core@next`. Because each `rc` run is cut from `next` HEAD, every rc.N already includes all prior fixes. No long-lived branch, no tag movement. `finalize` promotes the released version to `@latest` - (and keeps the existing `npm dist-tag add … next` so `@next` never trails `@latest`). + (`@next` remains the prerelease channel managed exclusively by the `rc` job; `finalize` does not repoint it). 5. **Everything else stays:** custom changesets + CHANGELOG render, release-notes formatter, smoke-test gates, provenance, `main`/`next`, `auto-backmerge` (main→next).