diff --git a/.changeset/patient-tunas-tumble.md b/.changeset/patient-tunas-tumble.md new file mode 100644 index 000000000..c06aaf510 --- /dev/null +++ b/.changeset/patient-tunas-tumble.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 2715 +--- +**The claude-orchestration Workflow backend now honors your model settings** — with that BETA capability enabled, every plan was dispatched with no model at all, so `model_overrides`, `model_policy` and `model_profile` were silently ignored and each agent ran on whatever the session happened to be using. Plans now run on the same model the normal dispatch path would have used, and the generated script states which model was applied. Two consequences to expect: agents that were inheriting the session model will now run on the model your profile selects, and the first run after upgrading re-executes any in-flight resumable run, because the dispatch options changed. (#2686) diff --git a/CONTEXT.md b/CONTEXT.md index 320adc462..f69d34c2e 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -245,7 +245,7 @@ ADR-1244 Phase 4 (D5+D6) orchestration seam (`gsd-core/bin/lib/capability-lifecy ADR-1244 Phase 5 (D7) registry-driven dispatch of capability command families. First-party families (`graphify`/`intel`/`audit`, shipped in `bin/lib/`) dispatch via `dispatchCapabilityCommand` (`gsd-core/bin/gsd-tools.cjs`) against the FROZEN `capability-registry.cjs` `commandFamilies` (confined to `bin/lib/`) — unchanged. Third-party (installed overlay) families dispatch via `dispatchOverlayCapabilityCommand`: after the first-party path returns false, it calls `loadRegistry({ includeInstalled, cwd })` and dispatches a family iff its `capId` is in `_overlay.commandRoots` — which `capability-loader.cjs` populates ONLY for accepted overlay capabilities that declare `commands` AND pass the loader's activation gate (a **committed** ledger entry, present and non-`_pending`, PLUS — for PROJECT scope — a matching user consent record in the Capability Consent Store; GLOBAL scope needs no consent record). A bundle dropped on disk with no install (no ledger entry) or no on-this-machine consent is NOT command-dispatchable. The router module is `require()`'d FROM the capability's install root via `defaultRequireFromInstallRoot` (bare-`.cjs` basename + `realpath` containment, rejecting `..` traversal and symlink escape); same own-property/function/sync-only guards as the first-party path. Wired into the `runCommand` default arm before "Unknown command". A repo-planted project ledger no longer activates anything on its own (#1459) — see `docs/explanation/capability-trust-model.md` "project-scope trust boundary". ### Claude Orchestration Capability -Default-off, BETA, claude-only Capability (`capabilities/claude-orchestration/`, `role: feature`, `runtimeCompat.supported: ["claude"]`, `tier: full`, `activationKey: claude_orchestration.enabled`) adopting Claude Code's Workflow tool (the engine behind `/effort ultracode`, Agent SDK ≥ v0.3.149) as an optional parallel-execution backend for the GSD loop, and folding the `gsd-ultraplan-phase` plan-offload under the same runtime gate (#1143; ADR-1143). Pure, fail-closed core in `gsd-core/bin/lib/claude-orchestration.cjs` (generated from `src/claude-orchestration.cts`): `detectWorkflowBackend({ runtimeId, hostIntegration, config, agentSdkVersion }) → { available, backend:'workflow'|'inline', reason }` (gate ladder: enabled → Claude → execution_backend ≠ inline → host dispatch nested+background → valid Agent SDK → SDK ≥ floor; every miss degrades to `inline`, never throws); `emitWorkflowScript({ phaseDir, waves, runId, budgetTokens? }) → { ok, script, summary }` mapping waves → `parallel()` stage barriers, plans → `agent({ agentType:'gsd-executor', isolation:'worktree' })`, `files_modified` overlap → separate sequential stages (greedy first-fit), `resumeFromRunId` wired to the run id, shared `budget(tokens)`; all interpolated identifiers validated script-safe (no `"`,`\`,control chars) and briefs JSON-quoted (review anti-injection). Registers two loop contributions at WIRED points only (execute:wave:pre/execute:pre are declared but not rendered, same constraint external-job documents): `execute:wave:post into:executor` (Workflow-backend guidance) and `plan:post into:planner` (ultraplan ownership declaration), both `when: claude_orchestration.enabled`, `onError: skip`. Federated config keys (`claude_orchestration.enabled` default false, `execution_backend` enum auto|workflow|inline default auto, `min_agent_sdk_version` string default "0.3.149") live only in the registry — uninstall removes them cleanly. Pre-release versions of the floor compare below GA (SemVer precedence). Restores the wave parallelism + plan-checker + verifier that #853 forces inline on Claude Code; on any runtime lacking the Workflow tool, behaviour is byte-identical to today. BETA v1 ships detection + emission + declarative ultraplan ownership + a `claude-orchestration` command family (`gsd-tools claude-orchestration detect-backend|emit-workflow`, router `gsd-core/bin/lib/claude-orchestration-command-router.cjs` from `src/claude-orchestration-command-router.cts`); full install-profile migration of the ultraplan skill into `skills[]` is a follow-up (CLUSTERS/profile gate). Test anchors: `tests/claude-orchestration.test.cjs`, `tests/claude-orchestration-command-router.test.cjs`. +Default-off, BETA, claude-only Capability (`capabilities/claude-orchestration/`, `role: feature`, `runtimeCompat.supported: ["claude"]`, `tier: full`, `activationKey: claude_orchestration.enabled`) adopting Claude Code's Workflow tool (the engine behind `/effort ultracode`, Agent SDK ≥ v0.3.149) as an optional parallel-execution backend for the GSD loop, and folding the `gsd-ultraplan-phase` plan-offload under the same runtime gate (#1143; ADR-1143). Pure, fail-closed core in `gsd-core/bin/lib/claude-orchestration.cjs` (generated from `src/claude-orchestration.cts`): `detectWorkflowBackend({ runtimeId, hostIntegration, config, agentSdkVersion }) → { available, backend:'workflow'|'inline', reason }` (gate ladder: enabled → Claude → execution_backend ≠ inline → host dispatch nested+background → valid Agent SDK → SDK ≥ floor; every miss degrades to `inline`, never throws); `emitWorkflowScript({ phaseDir, waves, runId, budgetTokens?, executorModel? }) → { ok, script, summary }` mapping waves → `parallel()` stage barriers, plans → `agent({ agentType:'gsd-executor', isolation:'worktree', model? })`, `files_modified` overlap → separate sequential stages (greedy first-fit), `resumeFromRunId` wired to the run id, shared `budget(tokens)`; all interpolated identifiers validated script-safe (no `"`,`\`,control chars) and briefs JSON-quoted (review anti-injection). #2686: `executorModel` — resolved for `gsd-executor` from the same config the inline path reads, defaulted by the router so no caller change is needed, `--executor-model` to pin — is emitted per plan and OMITTED when it resolves to `inherit`/empty/whitespace/non-string (#2517: an empty model 404s on runtimes without native tier aliases); a value carrying an unscriptable character is rejected outright (`ok:false`) rather than quoted, because the ADR-1411 provenance header interpolates it into a `//` comment where U+2028/U+2029 would terminate the comment and execute the remainder. `resolveWaveDispatch` forwards it. Registers two loop contributions at WIRED points only (execute:wave:pre/execute:pre are declared but not rendered, same constraint external-job documents): `execute:wave:post into:executor` (Workflow-backend guidance) and `plan:post into:planner` (ultraplan ownership declaration), both `when: claude_orchestration.enabled`, `onError: skip`. Federated config keys (`claude_orchestration.enabled` default false, `execution_backend` enum auto|workflow|inline default auto, `min_agent_sdk_version` string default "0.3.149") live only in the registry — uninstall removes them cleanly. Pre-release versions of the floor compare below GA (SemVer precedence). Restores the wave parallelism + plan-checker + verifier that #853 forces inline on Claude Code; on any runtime lacking the Workflow tool, behaviour is byte-identical to today. BETA v1 ships detection + emission + declarative ultraplan ownership + a `claude-orchestration` command family (`gsd-tools claude-orchestration detect-backend|emit-workflow`, router `gsd-core/bin/lib/claude-orchestration-command-router.cjs` from `src/claude-orchestration-command-router.cts`); full install-profile migration of the ultraplan skill into `skills[]` is a follow-up (CLUSTERS/profile gate). Test anchors: `tests/claude-orchestration.test.cjs`, `tests/claude-orchestration-command-router.test.cjs`. ### Loop Extension Point A named, stable site on a host loop step (per-step `pre`/`post` plus per-wave in Execute; 12 total) where Capabilities register hooks. Three hook kinds: `step` (runs as its own sequenced unit), `contribution` (injects into the core step's prompt/context), and `gate` (checks and optionally blocks via a declared `blocking` flag). Each hook declares the artifacts it produces and consumes; hook order is derived by topological sort of that produces/consumes graph (capability-id tiebreak), which also defines data flow — file-artifact based, surviving `/clear` and fresh executor contexts. Hooks are surfaced by runtime resolution with concrete projection: the workflow calls a query that resolves the active hooks and returns fully-rendered, ordered markdown for the executor. Failure is default-resilient — a non-gate hook that errors is skipped with a warning; a hook may opt into `onError: halt`. Part of the Capability system. ADR-857 phase 3c ships the registry-consuming query layer: `gsd-core/bin/lib/loop-resolver.cjs` exposes `resolveLoopHooks({ point, registry, config })` (pure, no I/O), `renderLoopHooks(resolved)` (pure markdown renderer), and `cmdLoopRenderHooks(cwd, point, raw, opts)` (I/O entry point); activated via `gsd-tools loop render-hooks ` which emits `{ point, activeHooks[], rendered }`. Activation is driven by `when` (dotted config key resolved against `loadConfig`), with inline literal `__proto__`/`constructor`/`prototype` prototype-pollution guard. The first phase-6 cutovers wiring workflows to this query have landed — ui-phase at `plan:pre` and ui-review at `verify:post` (in `plan-phase.md`/`autonomous.md`); further per-feature cutovers are ongoing. diff --git a/gsd-core/bin/lib/claude-orchestration-command-router.cjs b/gsd-core/bin/lib/claude-orchestration-command-router.cjs index f6ff36fb4..da3655c6a 100644 --- a/gsd-core/bin/lib/claude-orchestration-command-router.cjs +++ b/gsd-core/bin/lib/claude-orchestration-command-router.cjs @@ -22,7 +22,7 @@ * `claude_orchestration.*` keys from .planning/config.json. Emits * { available, backend, reason }. * - * emit-workflow --waves --run-id [--phase-dir ] [--budget ] + * emit-workflow --waves --run-id [--phase-dir ] [--budget ] [--executor-model ] * Reads a wave/plan manifest JSON file and emits the generated Workflow * script + summary. The manifest shape matches emitWorkflowScript's input: * { waves: [{ id, plans: [{ id, brief, files_modified: string[], use_worktree?: boolean }] }] }. @@ -32,7 +32,7 @@ * * resolve-wave-dispatch --waves --run-id [--runtime ] * [--agent-sdk-version ] [--no-nested-dispatch] [--phase-dir ] - * [--budget ] + * [--budget ] [--executor-model ] * #2285 — the single composed seam a PRE-wave dispatch-backend selector * (`execute:wave:pre`) uses: resolves detect-backend + emit-workflow in * ONE call. Emits { backend: 'inline'|'workflow', reason, script?, summary? }. @@ -53,14 +53,16 @@ const core = require("./claude-orchestration.cjs"); const configLoader = require("./config-loader.cjs"); // eslint-disable-next-line @typescript-eslint/no-require-imports const runtimeSlash = require("./runtime-slash.cjs"); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- model-resolver.cjs is an export= CommonJS module +const modelResolver = require("./model-resolver.cjs"); const { output } = io; const { detectWorkflowBackend, emitWorkflowScript, resolveWaveDispatch } = core; const CAPABLE_HOST = { dispatch: { nested: true, background: true } }; function usage(error) { error('Usage: gsd-tools claude-orchestration [...]\n' + ' detect-backend [--runtime ] [--agent-sdk-version ] [--no-nested-dispatch]\n' + - ' emit-workflow --waves --run-id [--phase-dir ] [--budget ]\n' + - ' resolve-wave-dispatch --waves --run-id [--runtime ] [--agent-sdk-version ] [--no-nested-dispatch] [--phase-dir ] [--budget ]'); + ' emit-workflow --waves --run-id [--phase-dir ] [--budget ] [--executor-model ]\n' + + ' resolve-wave-dispatch --waves --run-id [--runtime ] [--agent-sdk-version ] [--no-nested-dispatch] [--phase-dir ] [--budget ] [--executor-model ]'); } function argValue(args, flag) { const i = args.indexOf(flag); @@ -158,6 +160,32 @@ function resolveDetectionArgs(args, cwd) { const hostIntegration = noNested ? { dispatch: { nested: false, background: true } } : CAPABLE_HOST; return { runtimeId, hostIntegration, agentSdkVersion }; } +/** + * #2686 — resolve the `gsd-executor` model this dispatch should carry. + * + * Defaults from the project config rather than requiring a flag. The Workflow + * backend previously emitted no model at all, so `model_overrides` / + * `model_policy` / `model_profile` were silently inert on that path while the + * inline path honored them. Reading the same source the inline path reads is + * what makes the two backends agree by construction: an orchestrator that never + * learns about a new flag would otherwise silently keep the old bug. + * + * `--executor-model` exists only to pin/override. Resolution is side-effect-free + * and fails closed to `undefined` (emission then omits the key, i.e. exactly the + * pre-#2686 output) rather than guessing a model. + */ +function resolveExecutorModel(args, cwd) { + const pinned = argValue(args, '--executor-model'); + if (pinned !== undefined) + return pinned; + try { + const resolved = modelResolver.resolveModelInternal(cwd, 'gsd-executor'); + return typeof resolved === 'string' ? resolved : undefined; + } + catch { + return undefined; + } +} /** * Read and parse a `--waves ` manifest file. * @@ -196,7 +224,7 @@ function cmdDetectBackend(args, cwd, raw) { /** * Emit a Workflow script from a wave/plan manifest file. */ -function cmdEmitWorkflow(args, _cwd, raw, error) { +function cmdEmitWorkflow(args, cwd, raw, error) { const wavesPath = argValue(args, '--waves'); const runId = argValue(args, '--run-id'); const phaseDir = argValue(args, '--phase-dir') || '.planning/phases/current'; @@ -219,6 +247,7 @@ function cmdEmitWorkflow(args, _cwd, raw, error) { runId, waves: read.waves, budgetTokens: budget, + executorModel: resolveExecutorModel(args, cwd), }); if (!result.ok) { error('emit-workflow: ' + result.reason); @@ -261,6 +290,7 @@ function cmdResolveWaveDispatch(args, cwd, raw, error) { runId, waves: read.waves, budgetTokens: budget, + executorModel: resolveExecutorModel(args, cwd), }); output(result, raw); } diff --git a/gsd-core/bin/lib/claude-orchestration.cjs b/gsd-core/bin/lib/claude-orchestration.cjs index d2f730722..1b9f8b127 100644 --- a/gsd-core/bin/lib/claude-orchestration.cjs +++ b/gsd-core/bin/lib/claude-orchestration.cjs @@ -264,16 +264,52 @@ function partitionStages(plans) { function quoteString(s) { return JSON.stringify(s); } +/** + * #2686 — the single decision of whether a resolved executor model is emittable, + * and what to emit. Shared by `agentOptions` (the emission) and the provenance + * comment (the claim about it) so the two can never disagree — a generated + * comment asserting something the generator does not actually do is the exact + * failure #2686 was filed for. + * + * Returns the model to emit, or `undefined` for "emit nothing": + * - non-string → malformed config; omit rather than throw, matching the + * defensive typeof guard `mapClaudeOverrideForRuntime` + * already carries in model-resolver for the same reason. + * - empty/whitespace → #2517: emitting `model: ""` 404s on runtimes without + * native tier aliases. Trimmed, so `" "` is also "none". + * - "inherit" → same rule; matched case-insensitively after trimming, + * since config is user-authored free text. + * + * NOTE it does NOT reject unscriptable characters — that is a hard input error, + * not a silent omission, and is rejected up front by `emitWorkflowScript` so the + * caller sees a reason instead of quietly losing their model routing. + */ +function emittableModel(executorModel) { + if (typeof executorModel !== 'string') + return undefined; + const trimmed = executorModel.trim(); + if (trimmed.length === 0) + return undefined; + if (trimmed.toLowerCase() === 'inherit') + return undefined; + return trimmed; +} /** * Render the `agent()` options object for a single plan — `isolation: "worktree"` * ONLY when the plan's `use_worktree` is not explicitly `false` (#2772 / #2285 * finding 1). This is the single place that decides worktree isolation for the * Workflow backend; it must never diverge from the inline path's per-plan gate. */ -function agentOptions(p) { - return p.use_worktree === false - ? '{ agentType: "gsd-executor" }' - : '{ agentType: "gsd-executor", isolation: "worktree" }'; +function agentOptions(p, executorModel) { + const parts = ['agentType: "gsd-executor"']; + if (p.use_worktree !== false) + parts.push('isolation: "worktree"'); + // #2686: carry the resolved executor model so this backend honors + // model_overrides / model_policy / model_profile exactly as the inline path. + const model = emittableModel(executorModel); + if (model !== undefined) + parts.push('model: ' + quoteString(model)); + return '{ ' + parts.join(', ') + ' }'; } /** * True if `s` is a safe identifier/path token to interpolate into the generated @@ -301,7 +337,7 @@ function emitWorkflowScript(input) { if (input === null || input === undefined || typeof input !== 'object') { return { ok: false, reason: 'invalid_input' }; } - const { phaseDir, waves, runId } = input; + const { phaseDir, waves, runId, executorModel } = input; // Identifiers/paths interpolated into the generated script must be free of any // character that could terminate a comment, break out of a string literal, or // smuggle control bytes — reject up front (security: #1143 review Finding 1). @@ -311,6 +347,19 @@ function emitWorkflowScript(input) { if (!isScriptableIdentifier(runId)) { return { ok: false, reason: 'runId must be a non-empty string without newlines/quotes/backslash/control chars' }; } + // #2686 security: the resolved model is interpolated into BOTH an object + // literal (safe under quoteString) and a `//` provenance comment (NOT safe + // under quoteString — U+2028/U+2029 are LineTerminators that end a single-line + // comment in every engine, so a hostile model id would make the rest of the + // line live code). Reject the whole emission rather than silently dropping the + // model: an unscriptable id is malformed input, and `resolveWaveDispatch` maps + // an emit failure to the inline backend WITH a reason, so the user sees it. + // Only a STRING carrying such a character is rejected. A non-string is a + // malformed config rather than an injection attempt, and stays on the existing + // defensive path: `emittableModel` omits it and emission proceeds. + if (typeof executorModel === 'string' && UNSCRIPTABLE_CHAR_RE.test(executorModel)) { + return { ok: false, reason: 'executorModel must not contain newlines/quotes/backslash/control/line-separator chars' }; + } if (!Array.isArray(waves) || waves.length === 0) { return { ok: false, reason: 'waves must be a non-empty array' }; } @@ -387,6 +436,26 @@ function emitWorkflowScript(input) { lines.push('// Composes the SAME gsd-executor agent as the inline path, so artifacts (SUMMARY.md)'); lines.push('// and commits are produced identically. Worktree isolation is per-plan (use_worktree)'); lines.push('// and mirrors execute-phase.md step 2.5\'s submodule gate exactly (#2772 / #2285).'); + // #2686 / ADR-1411: state which model was applied — or that none resolved — + // so an opted-in user can SEE the routing decision instead of having to read + // the emitted options. A fallback must be a visible value, never silent. + // + // SECURITY: this is a `//` comment, and U+2028/U+2029 are ECMAScript + // LineTerminators that END a single-line comment in every engine — the ES2019 + // change legalized them inside string LITERALS only, so `quoteString` alone is + // NOT sufficient here even though it is sufficient in the object literal + // above. An unscriptable model id would otherwise close the comment and make + // the remainder live top-level code. `emitWorkflowScript` rejects such ids + // before reaching this point (see the validation above), which is what makes + // interpolating here safe. + const provenanceModel = emittableModel(executorModel); + if (provenanceModel !== undefined) { + lines.push('// model: ' + quoteString(provenanceModel) + ' (resolved for gsd-executor, same source as the inline path)'); + } + else { + lines.push('// model: none applied — resolved to "inherit"/empty, so each agent inherits the'); + lines.push('// orchestrator model (#2517: emitting an empty model 404s on some runtimes).'); + } lines.push('//'); // resumeFromRunId is a Workflow TOOL INPUT parameter, not a script function — // calling it threw "resumeFromRunId is not defined" (#2590). The run id is @@ -424,7 +493,7 @@ function emitWorkflowScript(input) { // parallel() could bound concurrency. lines.push('await parallel(['); for (const p of stagePlans) { - lines.push(' () => agent(' + quoteString(p.brief) + ', ' + agentOptions(p) + '),'); + lines.push(' () => agent(' + quoteString(p.brief) + ', ' + agentOptions(p, executorModel) + '),'); } lines.push('])'); } @@ -489,6 +558,7 @@ function resolveWaveDispatch(input) { waves: input.waves, runId: input.runId, budgetTokens: input.budgetTokens, + executorModel: input.executorModel, }); if (!emitted.ok) { return { backend: 'inline', reason: 'emit_failed: ' + emitted.reason }; diff --git a/src/claude-orchestration-command-router.cts b/src/claude-orchestration-command-router.cts index bd7964efe..64160c71c 100644 --- a/src/claude-orchestration-command-router.cts +++ b/src/claude-orchestration-command-router.cts @@ -21,7 +21,7 @@ * `claude_orchestration.*` keys from .planning/config.json. Emits * { available, backend, reason }. * - * emit-workflow --waves --run-id [--phase-dir ] [--budget ] + * emit-workflow --waves --run-id [--phase-dir ] [--budget ] [--executor-model ] * Reads a wave/plan manifest JSON file and emits the generated Workflow * script + summary. The manifest shape matches emitWorkflowScript's input: * { waves: [{ id, plans: [{ id, brief, files_modified: string[], use_worktree?: boolean }] }] }. @@ -31,7 +31,7 @@ * * resolve-wave-dispatch --waves --run-id [--runtime ] * [--agent-sdk-version ] [--no-nested-dispatch] [--phase-dir ] - * [--budget ] + * [--budget ] [--executor-model ] * #2285 — the single composed seam a PRE-wave dispatch-backend selector * (`execute:wave:pre`) uses: resolves detect-backend + emit-workflow in * ONE call. Emits { backend: 'inline'|'workflow', reason, script?, summary? }. @@ -50,6 +50,8 @@ import core = require('./claude-orchestration.cjs'); import configLoader = require('./config-loader.cjs'); // eslint-disable-next-line @typescript-eslint/no-require-imports import runtimeSlash = require('./runtime-slash.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- model-resolver.cjs is an export= CommonJS module +import modelResolver = require('./model-resolver.cjs'); const { output } = io; const { detectWorkflowBackend, emitWorkflowScript, resolveWaveDispatch } = core; @@ -67,8 +69,8 @@ function usage(error: (msg: string, reason?: string) => void): void { error( 'Usage: gsd-tools claude-orchestration [...]\n' + ' detect-backend [--runtime ] [--agent-sdk-version ] [--no-nested-dispatch]\n' + - ' emit-workflow --waves --run-id [--phase-dir ] [--budget ]\n' + - ' resolve-wave-dispatch --waves --run-id [--runtime ] [--agent-sdk-version ] [--no-nested-dispatch] [--phase-dir ] [--budget ]', + ' emit-workflow --waves --run-id [--phase-dir ] [--budget ] [--executor-model ]\n' + + ' resolve-wave-dispatch --waves --run-id [--runtime ] [--agent-sdk-version ] [--no-nested-dispatch] [--phase-dir ] [--budget ] [--executor-model ]', ); } @@ -164,6 +166,31 @@ function resolveDetectionArgs(args: string[], cwd?: string): { runtimeId: string return { runtimeId, hostIntegration, agentSdkVersion }; } +/** + * #2686 — resolve the `gsd-executor` model this dispatch should carry. + * + * Defaults from the project config rather than requiring a flag. The Workflow + * backend previously emitted no model at all, so `model_overrides` / + * `model_policy` / `model_profile` were silently inert on that path while the + * inline path honored them. Reading the same source the inline path reads is + * what makes the two backends agree by construction: an orchestrator that never + * learns about a new flag would otherwise silently keep the old bug. + * + * `--executor-model` exists only to pin/override. Resolution is side-effect-free + * and fails closed to `undefined` (emission then omits the key, i.e. exactly the + * pre-#2686 output) rather than guessing a model. + */ +function resolveExecutorModel(args: string[], cwd: string): string | undefined { + const pinned = argValue(args, '--executor-model'); + if (pinned !== undefined) return pinned; + try { + const resolved = modelResolver.resolveModelInternal(cwd, 'gsd-executor') as unknown; + return typeof resolved === 'string' ? resolved : undefined; + } catch { + return undefined; + } +} + /** Discriminated result for readWavesManifest — see doc comment below. */ type WavesReadResult = | { ok: true; waves: unknown } @@ -208,7 +235,7 @@ function cmdDetectBackend(args: string[], cwd: string, raw: boolean): void { /** * Emit a Workflow script from a wave/plan manifest file. */ -function cmdEmitWorkflow(args: string[], _cwd: string, raw: boolean, error: (msg: string, reason?: string) => void): void { +function cmdEmitWorkflow(args: string[], cwd: string, raw: boolean, error: (msg: string, reason?: string) => void): void { const wavesPath = argValue(args, '--waves'); const runId = argValue(args, '--run-id'); const phaseDir = argValue(args, '--phase-dir') || '.planning/phases/current'; @@ -234,6 +261,7 @@ function cmdEmitWorkflow(args: string[], _cwd: string, raw: boolean, error: (msg runId, waves: read.waves as EmitInput['waves'], budgetTokens: budget, + executorModel: resolveExecutorModel(args, cwd), }); if (!result.ok) { @@ -282,6 +310,7 @@ function cmdResolveWaveDispatch(args: string[], cwd: string, raw: boolean, error runId, waves: read.waves as EmitInput['waves'], budgetTokens: budget, + executorModel: resolveExecutorModel(args, cwd), }); output(result, raw); diff --git a/src/claude-orchestration.cts b/src/claude-orchestration.cts index ef0713e0b..a8ecff38f 100644 --- a/src/claude-orchestration.cts +++ b/src/claude-orchestration.cts @@ -283,6 +283,14 @@ interface EmitInput { waves: Wave[]; runId: string; budgetTokens?: number; + /** + * #2686 — the resolved `gsd-executor` model, threaded so this backend honors + * model_overrides / model_policy / model_profile exactly as the inline path + * does. Omitted from the emitted options when it is `"inherit"`, empty, or not + * a string (#2517: an empty model 404s on runtimes without native tier + * aliases). When absent the emitted script is byte-identical to pre-#2686. + */ + executorModel?: string; } interface EmitOk { @@ -351,16 +359,48 @@ function quoteString(s: string): string { return JSON.stringify(s); } +/** + * #2686 — the single decision of whether a resolved executor model is emittable, + * and what to emit. Shared by `agentOptions` (the emission) and the provenance + * comment (the claim about it) so the two can never disagree — a generated + * comment asserting something the generator does not actually do is the exact + * failure #2686 was filed for. + * + * Returns the model to emit, or `undefined` for "emit nothing": + * - non-string → malformed config; omit rather than throw, matching the + * defensive typeof guard `mapClaudeOverrideForRuntime` + * already carries in model-resolver for the same reason. + * - empty/whitespace → #2517: emitting `model: ""` 404s on runtimes without + * native tier aliases. Trimmed, so `" "` is also "none". + * - "inherit" → same rule; matched case-insensitively after trimming, + * since config is user-authored free text. + * + * NOTE it does NOT reject unscriptable characters — that is a hard input error, + * not a silent omission, and is rejected up front by `emitWorkflowScript` so the + * caller sees a reason instead of quietly losing their model routing. + */ +function emittableModel(executorModel: unknown): string | undefined { + if (typeof executorModel !== 'string') return undefined; + const trimmed = executorModel.trim(); + if (trimmed.length === 0) return undefined; + if (trimmed.toLowerCase() === 'inherit') return undefined; + return trimmed; +} + /** * Render the `agent()` options object for a single plan — `isolation: "worktree"` * ONLY when the plan's `use_worktree` is not explicitly `false` (#2772 / #2285 * finding 1). This is the single place that decides worktree isolation for the * Workflow backend; it must never diverge from the inline path's per-plan gate. */ -function agentOptions(p: Plan): string { - return p.use_worktree === false - ? '{ agentType: "gsd-executor" }' - : '{ agentType: "gsd-executor", isolation: "worktree" }'; +function agentOptions(p: Plan, executorModel?: unknown): string { + const parts = ['agentType: "gsd-executor"']; + if (p.use_worktree !== false) parts.push('isolation: "worktree"'); + // #2686: carry the resolved executor model so this backend honors + // model_overrides / model_policy / model_profile exactly as the inline path. + const model = emittableModel(executorModel); + if (model !== undefined) parts.push('model: ' + quoteString(model)); + return '{ ' + parts.join(', ') + ' }'; } /** @@ -389,7 +429,7 @@ function emitWorkflowScript(input: EmitInput | null | undefined): EmitOk | EmitE if (input === null || input === undefined || typeof input !== 'object') { return { ok: false, reason: 'invalid_input' }; } - const { phaseDir, waves, runId } = input; + const { phaseDir, waves, runId, executorModel } = input; // Identifiers/paths interpolated into the generated script must be free of any // character that could terminate a comment, break out of a string literal, or // smuggle control bytes — reject up front (security: #1143 review Finding 1). @@ -399,6 +439,19 @@ function emitWorkflowScript(input: EmitInput | null | undefined): EmitOk | EmitE if (!isScriptableIdentifier(runId)) { return { ok: false, reason: 'runId must be a non-empty string without newlines/quotes/backslash/control chars' }; } + // #2686 security: the resolved model is interpolated into BOTH an object + // literal (safe under quoteString) and a `//` provenance comment (NOT safe + // under quoteString — U+2028/U+2029 are LineTerminators that end a single-line + // comment in every engine, so a hostile model id would make the rest of the + // line live code). Reject the whole emission rather than silently dropping the + // model: an unscriptable id is malformed input, and `resolveWaveDispatch` maps + // an emit failure to the inline backend WITH a reason, so the user sees it. + // Only a STRING carrying such a character is rejected. A non-string is a + // malformed config rather than an injection attempt, and stays on the existing + // defensive path: `emittableModel` omits it and emission proceeds. + if (typeof executorModel === 'string' && UNSCRIPTABLE_CHAR_RE.test(executorModel)) { + return { ok: false, reason: 'executorModel must not contain newlines/quotes/backslash/control/line-separator chars' }; + } if (!Array.isArray(waves) || waves.length === 0) { return { ok: false, reason: 'waves must be a non-empty array' }; } @@ -477,6 +530,25 @@ function emitWorkflowScript(input: EmitInput | null | undefined): EmitOk | EmitE lines.push('// Composes the SAME gsd-executor agent as the inline path, so artifacts (SUMMARY.md)'); lines.push('// and commits are produced identically. Worktree isolation is per-plan (use_worktree)'); lines.push('// and mirrors execute-phase.md step 2.5\'s submodule gate exactly (#2772 / #2285).'); + // #2686 / ADR-1411: state which model was applied — or that none resolved — + // so an opted-in user can SEE the routing decision instead of having to read + // the emitted options. A fallback must be a visible value, never silent. + // + // SECURITY: this is a `//` comment, and U+2028/U+2029 are ECMAScript + // LineTerminators that END a single-line comment in every engine — the ES2019 + // change legalized them inside string LITERALS only, so `quoteString` alone is + // NOT sufficient here even though it is sufficient in the object literal + // above. An unscriptable model id would otherwise close the comment and make + // the remainder live top-level code. `emitWorkflowScript` rejects such ids + // before reaching this point (see the validation above), which is what makes + // interpolating here safe. + const provenanceModel = emittableModel(executorModel); + if (provenanceModel !== undefined) { + lines.push('// model: ' + quoteString(provenanceModel) + ' (resolved for gsd-executor, same source as the inline path)'); + } else { + lines.push('// model: none applied — resolved to "inherit"/empty, so each agent inherits the'); + lines.push('// orchestrator model (#2517: emitting an empty model 404s on some runtimes).'); + } lines.push('//'); // resumeFromRunId is a Workflow TOOL INPUT parameter, not a script function — // calling it threw "resumeFromRunId is not defined" (#2590). The run id is @@ -517,7 +589,7 @@ function emitWorkflowScript(input: EmitInput | null | undefined): EmitOk | EmitE // parallel() could bound concurrency. lines.push('await parallel(['); for (const p of stagePlans) { - lines.push(' () => agent(' + quoteString(p.brief) + ', ' + agentOptions(p) + '),'); + lines.push(' () => agent(' + quoteString(p.brief) + ', ' + agentOptions(p, executorModel) + '),'); } lines.push('])'); } @@ -553,6 +625,8 @@ interface ResolveWaveDispatchInput { waves: Wave[]; runId: string; budgetTokens?: number; + /** #2686 — forwarded verbatim to `emitWorkflowScript`. */ + executorModel?: string; } interface ResolveWaveDispatchInline { @@ -615,6 +689,7 @@ function resolveWaveDispatch(input: ResolveWaveDispatchInput | null | undefined) waves: input.waves, runId: input.runId, budgetTokens: input.budgetTokens, + executorModel: input.executorModel, }); if (!emitted.ok) { diff --git a/tests/claude-orchestration.test.cjs b/tests/claude-orchestration.test.cjs index d274ee874..0cef3b240 100644 --- a/tests/claude-orchestration.test.cjs +++ b/tests/claude-orchestration.test.cjs @@ -739,3 +739,283 @@ describe('inline-fallback parity', () => { assert.ok(r.script.includes('SUMMARY.md'), 'same SUMMARY.md artifact as inline dispatch'); }); }); + +// ─── #2686 — executor-model threading ──────────────────────────────────────── +// +// The Workflow backend emitted every agent() call with no model at all, so +// model_overrides / model_policy / model_profile were silently inert on that +// path while the inline path honored them — the invisible partial application +// ADR-1411 prohibits. These pin the parity the generated script already claims. + +describe('#2686 — Workflow backend model threading', () => { + const { + resolveWaveDispatch: resolveWaveDispatch2686, + } = require('../gsd-core/bin/lib/claude-orchestration.cjs'); + const modelResolver2686 = require('../gsd-core/bin/lib/model-resolver.cjs'); + const { runGsdTools: runTools2686, createTempProject: mkProject2686, cleanup: rm2686 } = + require('./helpers.cjs'); + + const WAVES_2686 = [ + { id: '1', plans: [ + { id: '01', brief: 'Implement the auth module', files_modified: ['src/auth.ts'] }, + { id: '02', brief: 'Add the config loader', files_modified: ['src/config.ts'] }, + ] }, + ]; + + const emit2686 = (extra) => emitWorkflowScript({ + phaseDir: '.planning/phases/01-demo', + runId: 'wf_test2686', + waves: WAVES_2686, + ...extra, + }); + + // The emitted agent() options objects only. Scoped deliberately: the #2686 + // provenance header legitimately contains the token "model:", so a whole-script + // regex would report a false positive on the omit path. + // + // Brace-and-string aware rather than /\{[^}]*\}/: a model value may legitimately + // contain a brace, and a naive class would truncate the object there and silently + // stop testing what it claims to test. + const optionsOf = (script) => { + const out = []; + const re = /\bagent\(/g; + let m; + while ((m = re.exec(script)) !== null) { + const open = script.indexOf('{', m.index); + if (open === -1) continue; + let i = open + 1; + let depth = 1; + let str = null; + while (i < script.length && depth > 0) { + const ch = script[i]; + if (str) { + if (ch === '\\') i += 1; + else if (ch === str) str = null; + } else if (ch === '"' || ch === "'") str = ch; + else if (ch === '{') depth += 1; + else if (ch === '}') depth -= 1; + i += 1; + } + if (depth === 0) out.push(script.slice(open, i)); + } + return out.filter((o) => o.includes('agentType')); + }; + + test('#2686: the Workflow backend dispatches the same model the inline path would use', () => { + const dir = mkProject2686('gsd-2686-'); + try { + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ model_profile: 'balanced', model_overrides: { 'gsd-executor': 'opus' } }), + ); + // The inline path reads exactly this. Deriving BOTH sides from the resolver + // (rather than hardcoding either) is what makes this a parity assertion. + const inlineModel = modelResolver2686.resolveModelInternal(dir, 'gsd-executor'); + assert.equal(inlineModel, 'opus', 'precondition: the override must resolve'); + + const res = emit2686({ executorModel: inlineModel }); + assert.ok(res.ok, `emit failed: ${res.reason}`); + const calls = res.script.match(/agent\([\s\S]*?\{[^}]*\}/g) || []; + assert.ok(calls.length >= 2, `expected >=2 agent() calls, got ${calls.length}`); + for (const call of calls) { + assert.match( + call, + /model:\s*"opus"/, + 'every dispatched plan must carry the model the inline path resolved — ' + + 'without it model_overrides/model_policy are silently inert on this backend (#2686).', + ); + } + } finally { + rm2686(dir); + } + }); + + test('#2686: omits the model key when the resolved model is inherit or empty', () => { + // #2517: an empty/inherit model must be OMITTED, never emitted — emitting it + // 404s on runtimes without native tier aliases. + for (const value of ['inherit', 'INHERIT', ' inherit ', ' ', '', undefined, null, 42, {}, []]) { + const res = emit2686({ executorModel: value }); + assert.ok(res.ok, `emit failed for ${JSON.stringify(value)}: ${res.reason}`); + const opts = optionsOf(res.script); + assert.ok(opts.length >= 2, `expected per-plan options objects, got ${opts.length}`); + for (const o of opts) { + assert.doesNotMatch( + o, + /model:/, + `executorModel=${JSON.stringify(value)} must omit the model key entirely (#2517)`, + ); + } + } + }); + + test('#2686: emits byte-identical output when no model resolves', () => { + // The compatibility contract: every existing caller and assertion is unaffected + // unless a model actually resolves. + const withNothing = emit2686({}); + const withInherit = emit2686({ executorModel: 'inherit' }); + const withEmpty = emit2686({ executorModel: '' }); + assert.ok(withNothing.ok && withInherit.ok && withEmpty.ok); + assert.equal(withInherit.script, withNothing.script); + assert.equal(withEmpty.script, withNothing.script); + }); + + test('#2686: model threading does not disturb the per-plan worktree gate', () => { + // #2772 / #2285 finding 1 — agentOptions is "the single place that decides + // worktree isolation; it must never diverge from the inline path's per-plan gate." + const res = emitWorkflowScript({ + phaseDir: '.planning/phases/01-demo', + runId: 'wf_test2686b', + executorModel: 'sonnet', + waves: [{ id: '1', plans: [ + { id: '01', brief: 'plan without isolation', files_modified: ['a.ts'], use_worktree: false }, + { id: '02', brief: 'plan with isolation', files_modified: ['b.ts'] }, + ] }], + }); + assert.ok(res.ok, `emit failed: ${res.reason}`); + const calls = res.script.match(/agent\([\s\S]*?\{[^}]*\}/g) || []; + assert.equal(calls.length, 2, 'per-plan options objects must remain one per plan'); + + const noWt = calls.find((c) => c.includes('plan without isolation')); + const wt = calls.find((c) => c.includes('plan with isolation')); + assert.ok(noWt && wt, 'both plans must be present'); + assert.doesNotMatch(noWt, /isolation:/, 'use_worktree:false must still suppress isolation'); + assert.match(noWt, /model:\s*"sonnet"/, 'model must still be threaded on the no-worktree plan'); + assert.match(wt, /isolation:\s*"worktree"/, 'default plan must still get worktree isolation'); + assert.match(wt, /model:\s*"sonnet"/, 'model must be threaded on the worktree plan'); + }); + + test('#2686: a model id carrying a script-breaking character is rejected outright', () => { + // The model id is externally-supplied config (model_overrides / model_policy) + // reaching a CODE GENERATOR. It is interpolated into BOTH an object literal + // and a `//` provenance comment. + // + // quoteString (JSON.stringify) is sufficient for the object literal but NOT + // for the comment: U+2028 / U+2029 are ECMAScript LineTerminators that END a + // single-line comment in every engine — the ES2019 change legalized them + // inside string LITERALS only. A raw one would close the comment and make the + // rest of the line live top-level code. Hence: reject, do not merely quote. + const hostile = [ + 'evil\u2028process.exit(42);//', // proven comment-terminator injection + 'evil\u2029process.exit(42);//', + 'a\nb', 'a\rb', 'a"b', 'a\\b', 'a\tb', 'a\u0000b', 'a\u007fb', + ]; + for (const id of hostile) { + const res = emit2686({ executorModel: id }); + assert.equal( + res.ok, + false, + `executorModel ${JSON.stringify(id)} must be REJECTED, not emitted — it can ` + + 'terminate the provenance comment and execute as top-level code.', + ); + assert.match(res.reason, /executorModel must not contain/); + } + }); + + test('#2686: the emitted provenance comment cannot become live code', () => { + // Execution-level proof, not a shape check: run the emitted header through a + // parser and confirm the model value never escapes its comment/literal. A + // previous version of this test asserted only that JSON.stringify was used, + // which passed against the vulnerable generator. + const good = emit2686({ executorModel: 'opus' }); + assert.ok(good.ok); + const header = good.script.split('\n').filter((l) => l.startsWith('// model:')); + assert.equal(header.length, 1, 'exactly one provenance line'); + for (const line of header) { + assert.doesNotMatch(line, /[\u2028\u2029\r\n]/, 'no LineTerminator may survive into the comment'); + } + // Every emitted comment line must still be a comment after parsing: wrapping + // the header in a function body must produce no executable statement. + const commentBlock = good.script.split('\n').filter((l) => l.startsWith('//')).join('\n'); + assert.doesNotThrow(() => new Function(commentBlock + '\nreturn 1;')); + assert.equal(new Function(commentBlock + '\nreturn 1;')(), 1); + }); + + test('#2686: resolveWaveDispatch forwards the executor model to emission', () => { + const res = resolveWaveDispatch2686({ + runtimeId: 'claude', + hostIntegration: { dispatch: { nested: true, background: true } }, + config: { 'claude_orchestration.enabled': true }, + agentSdkVersion: '99.0.0', + phaseDir: '.planning/phases/01-demo', + runId: 'wf_test2686c', + waves: WAVES_2686, + executorModel: 'haiku', + }); + assert.equal(res.backend, 'workflow', `expected workflow backend, got ${res.backend}: ${res.reason}`); + assert.match( + res.script, + /model:\s*"haiku"/, + 'the #2285 composed seam the orchestrator actually calls must forward the model', + ); + }); + + test('#2686: the CLI defaults the executor model from project config', () => { + const dir = mkProject2686('gsd-2686-cli-'); + try { + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ model_profile: 'balanced', model_overrides: { 'gsd-executor': 'opus' } }), + ); + const wavesPath = path.join(dir, 'waves.json'); + fs.writeFileSync(wavesPath, JSON.stringify({ waves: WAVES_2686 })); + + // No --executor-model: the router must resolve it from config, so the fix + // applies with NO caller change. + const dflt = runTools2686( + ['claude-orchestration', 'emit-workflow', '--waves', wavesPath, '--run-id', 'wf_cli1'], + dir, + ); + assert.ok(dflt.success, `emit-workflow failed: ${dflt.error}`); + assert.match(JSON.parse(dflt.output).script, /model:\s*"opus"/); + + // Explicit flag wins. + const pinned = runTools2686( + ['claude-orchestration', 'emit-workflow', '--waves', wavesPath, '--run-id', 'wf_cli2', + '--executor-model', 'haiku'], + dir, + ); + assert.ok(pinned.success, `emit-workflow failed: ${pinned.error}`); + assert.match(JSON.parse(pinned.output).script, /model:\s*"haiku"/); + } finally { + rm2686(dir); + } + }); + + test('#2686: emitted options round-trip any resolved model (property)', () => { + // Three-way contract over ARBITRARY strings: + // unscriptable char → ok:false (rejected; it could terminate the // comment) + // trims to empty or "inherit" (any case) → ok:true, model key OMITTED (#2517) + // otherwise → ok:true, model key emitted as the TRIMMED value + // Mirrors UNSCRIPTABLE_CHAR_RE in src/claude-orchestration.cts, which is not + // exported. The control-character class is the POINT of the check — these are + // exactly the bytes that must be rejected — so the rule is disabled here rather + // than the class weakened. + // eslint-disable-next-line no-control-regex + const UNSCRIPTABLE = /[\r\n"\\\x00-\x1f\x7f\u2028\u2029]/; + fc.assert( + fc.property(fc.string({ maxLength: 40 }), (model) => { + const res = emit2686({ executorModel: model }); + if (UNSCRIPTABLE.test(model)) { + assert.equal(res.ok, false, `${JSON.stringify(model)} must be rejected`); + return; + } + assert.ok(res.ok, `${JSON.stringify(model)} should emit: ${res.reason}`); + const opts = optionsOf(res.script); + assert.ok(opts.length >= 2, `expected per-plan options, got ${opts.length}`); + const trimmed = model.trim(); + const shouldEmit = trimmed.length > 0 && trimmed.toLowerCase() !== 'inherit'; + for (const o of opts) { + if (shouldEmit) { + assert.ok( + o.includes('model: ' + JSON.stringify(trimmed)), + `expected model ${JSON.stringify(trimmed)} in ${o}`, + ); + } else { + assert.doesNotMatch(o, /model:/); + } + } + }), + { numRuns: 300 }, + ); + }); +}); diff --git a/tests/fix-2285-claude-orchestration-wiring.test.cjs b/tests/fix-2285-claude-orchestration-wiring.test.cjs index ec0d82ab2..bb5fc0a4d 100644 --- a/tests/fix-2285-claude-orchestration-wiring.test.cjs +++ b/tests/fix-2285-claude-orchestration-wiring.test.cjs @@ -285,12 +285,17 @@ describe('C. resolveWaveDispatch composes detectWorkflowBackend + emitWorkflowSc '--phase-dir', '.planning/phases/01-foo', '--runtime', 'claude', '--agent-sdk-version', ABOVE_FLOOR_SDK, + // #2686: the router now DEFAULTS the executor model from project config, + // so pin it on both sides — otherwise this compares a config-resolved CLI + // run against a pure call that was given no model, and the equality this + // test exists to prove would be testing the default instead of the seam. + '--executor-model', 'sonnet', '--raw', ], tmp); assert.strictEqual(res.success, true, 'CLI command must succeed; stderr: ' + (res.error || '')); const parsed = JSON.parse(res.output); - const direct = resolveWaveDispatch(baseInput()); + const direct = resolveWaveDispatch(baseInput({ executorModel: 'sonnet' })); assert.strictEqual(parsed.backend, direct.backend); assert.strictEqual(parsed.script, direct.script); assert.deepStrictEqual(parsed.summary, direct.summary); @@ -453,7 +458,10 @@ describe('F. Workflow backend never forces worktree isolation on a submodule / u const result = resolveWaveDispatch(baseInput({ ...waveWithSubmodulePlan() })); assert.strictEqual(result.backend, 'workflow'); assert.match(result.script, /agent\("normal plan", \{ agentType: "gsd-executor", isolation: "worktree" \}\)/); - assert.match(result.script, /agent\("submodule plan", \{ agentType: "gsd-executor" \}\)/); + // #2686: the options object legitimately gained an optional `model` key, so assert + // the invariant this test exists to protect — agentType present, isolation absent — + // rather than a frozen literal that any future additive key would break. + assert.match(result.script, /agent\("submodule plan", \{ agentType: "gsd-executor"[^}]*\}\)/); assert.ok( !/agent\("submodule plan"[^)]*isolation/.test(result.script), 'the submodule-touching plan must NEVER be emitted with forced worktree isolation', @@ -483,7 +491,8 @@ describe('F. Workflow backend never forces worktree isolation on a submodule / u assert.strictEqual(res.success, true, 'CLI command must succeed; stderr: ' + (res.error || '')); const parsed = JSON.parse(res.output); assert.strictEqual(parsed.backend, 'workflow'); - assert.match(parsed.script, /agent\("submodule plan", \{ agentType: "gsd-executor" \}\)/); + // #2686: additive `model` key — see the note on the pure-seam test above. + assert.match(parsed.script, /agent\("submodule plan", \{ agentType: "gsd-executor"[^}]*\}\)/); assert.ok(!/agent\("submodule plan"[^)]*isolation/.test(parsed.script)); } finally { cleanup(tmp);