fix(#2686): thread the resolved executor model into the Workflow backend (#2715)

* test(#2686): failing-first parity guard for Workflow-backend model threading

The Workflow backend emitted every agent() call with no model, so
model_overrides / model_policy / model_profile were silently inert on that path
while the inline path honored them (ADR-1411). Neither existing suite contained
the string 'model' at all.

The centrepiece derives BOTH sides from resolveModelInternal(cwd,'gsd-executor')
rather than hardcoding either, so it asserts backend parity rather than a fixed
string. Also covers: omit-on-inherit/empty (#2517), byte-identical output when
nothing resolves, the #2772/#2285 per-plan worktree gate, adversarial model ids
reaching the code generator, the #2285 composed seam, CLI config-defaulting, and
a fast-check round-trip property.

RED expected: no model key is emitted anywhere, and --executor-model does not exist.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPq9ovaovP2UvSVLjD4Lso

* fix(#2686): thread the resolved executor model into the Workflow backend

The Workflow backend emitted every agent() call with no model at all, so
model_overrides / model_policy / model_profile_overrides / model_profile were
silently inert on that path while the inline path honored all of them. The model
was not dropped at the last step — it was absent from the whole seam:
agentOptions() took no model, EmitInput had no field to carry one, and
ResolveWaveDispatchInput (the #2285 seam the orchestrator actually calls) could
not forward one. The generated script asserted the parity it broke.

VERIFY-FIRST, which #2686 flags as the question that decides the fix: the
Workflow tool's agent() DOES accept a per-call model. Its documented signature is
  agent(prompt, opts?: { label?, phase?, schema?, model?, effort?, isolation?, agentType? })
so fix branch 1 applies and branch 2 (declare model routing unavailable) is ruled
out. ADR-1143:24's option enumeration omitting `model` is an incomplete
enumeration, not a decision to exclude it.

- agentOptions(p, executorModel) emits `model` only when it is a non-empty string
  that is not "inherit" (#2517: an empty model 404s on runtimes without native
  tier aliases). A non-string is a malformed config: omit, never throw.
- executorModel threaded through EmitInput and ResolveWaveDispatchInput.
- The CLI resolves gsd-executor from project config by DEFAULT rather than
  requiring a flag, reading the same source the inline path reads. An
  orchestrator that never learns about a new flag would otherwise silently keep
  the old bug. --executor-model exists only to pin/override.
- ADR-1411 provenance: the generated header now states which model was applied,
  or that none resolved and why. A fallback must be a visible value.

Compatibility: when nothing resolves, the emitted options object is byte-identical
to before, so every existing caller and assertion is unaffected.

Behavior change (Hyrum's Law): opted-in users move from session inheritance to the
catalog-resolved executor model. Adding a `model` key also changes agent() opts,
which invalidates the cached prefix of any in-flight resumeFromRunId run — a
one-time re-execution. Both disclosed in the changeset.

Fixes #2686

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPq9ovaovP2UvSVLjD4Lso

* fix(#2686): reject script-breaking model ids and share the emit predicate

The isolated adversarial review found a BLOCKER in my own provenance comment,
proven by execution (the emitted script exited 42 from an injected statement).

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 (JSON.stringify) is sufficient for the `model: "..."` object
literal but NOT for the `// model: ...` provenance line I added: a raw U+2028 in
a model id closed the comment and made the rest of the line live top-level code.
The value is reachable from `.planning/config.json` (model_overrides /
model_policy), which `mapClaudeOverrideForRuntime` passes through verbatim on any
non-claude runtime — attacker-influenceable in a cloned repo.

`emitWorkflowScript` now rejects a string executorModel carrying any character in
UNSCRIPTABLE_CHAR_RE — the same class `isScriptableIdentifier` already applied to
phaseDir/runId, which is proof the codebase knew this hazard. Rejection is
ok:false with a reason rather than a silent drop, and resolveWaveDispatch maps an
emit failure to the inline backend WITH that reason, so the degradation is
visible. A non-string stays on the existing defensive path (omit, never throw) —
that is malformed config, not an injection attempt.

Also from the reviews:

- The predicate deciding "is this model emittable" was duplicated between the
  emission and the comment asserting it. Extracted to emittableModel() so a
  generated comment can never claim something the generator did not do — the
  exact failure class #2686 was filed for.
- That predicate now trims and lower-cases before comparing, closing a real
  #2517-class gap: " " and "INHERIT" were previously emitted verbatim.
- The adversarial test was pass-always against this very vulnerability — it
  asserted only that JSON.stringify appeared. Replaced with the real contract
  (rejection) plus an execution-level check that no LineTerminator survives into
  the comment. A raw U+2028 had also been committed into that test's fixture
  array where a tab was intended; both are now explicit \u escapes.
- optionsOf in the test was /\{[^}]*\}/, which truncated at any brace a generated
  model contained — silently not testing what it claimed. Now brace- and
  string-aware.

Stale-test corrections in tests/fix-2285-*: three assertions froze the exact
options literal `{ agentType: "gsd-executor" }`. The object legitimately gained
an optional additive `model` key, so they now assert the invariant they exist to
protect (agentType present, isolation absent) rather than a frozen literal. The
CLI-vs-pure equality test pins --executor-model on both sides; otherwise it
compared a config-resolved CLI run against a pure call given no model.

CONTEXT.md glossary updated for the changed emitWorkflowScript signature and the
new rejection rule (CLAUDE.md: the glossary is a PR gate for core-module changes).

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPq9ovaovP2UvSVLjD4Lso

* test(#2686): fix the options extractor and model the rejection path

Two defects in my own test helper, caught by the full matrix:

- optionsOf anchored on /\(\s*\{/ — a '(' immediately followed by '{'. The
  emitted shape is agent("brief", { ... }), so that never matched and the helper
  returned an empty array, making every assertion over it vacuously true. It now
  anchors on agent( and takes the first balanced, string-aware {...} after it.

- The fast-check property predated the security fix and asserted ok:true for any
  generated string. Strings carrying an unscriptable character are now rejected,
  so the property models the real three-way contract: unscriptable -> ok:false;
  trims to empty or 'inherit' (any case) -> omitted; otherwise -> emitted as the
  trimmed value.

Verified locally against the built module: omit values clean, both plans carry
the model on the parity path, property passes 500 runs at seed 42. Test file
re-scanned for raw hazardous codepoints — zero; the U+2028/U+2029 cases are
explicit \u escapes.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPq9ovaovP2UvSVLjD4Lso

* test(#2686): scope no-control-regex on the mirrored unscriptable-char class

The class is the point of the assertion — those bytes are exactly what must be
rejected — so the rule is disabled at that line rather than the class weakened.
UNSCRIPTABLE_CHAR_RE is not exported from src/claude-orchestration.cts, hence
the mirror.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPq9ovaovP2UvSVLjD4Lso

* chore(#2686): backfill changeset PR number

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01DPq9ovaovP2UvSVLjD4Lso

---------

Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Tom Boucher
2026-07-27 16:24:21 -04:00
committed by GitHub
parent 1c93df04db
commit 09477f925e
8 changed files with 524 additions and 26 deletions

View File

@@ -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 },
);
});
});

View File

@@ -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);