* test(#3714): failing-first coverage for the dropped Codex worktree model override Pins the argv contract for the orchestrator-worktree process dispatch: an explicit model_overrides pin must reach the child as --model, while an unpinned, empty, inherit, or profile-only configuration must emit no flag at all. Five of the eight matrix rows are CONTROLS that pass before the fix. They carry the weight here because this is an over-emission bug waiting to happen: resolve-model returns 'sonnet' for the unpinned, empty and profile-only cases, so a fix that threads its return value into argv would satisfy the positive row and emit --model sonnet to Codex on every unpinned install -- the documented 400 that ADR-2313 exists to prevent. The controls are what separate the correct fix from the obvious one. * fix(#3714): deliver an explicitly pinned model to the Codex worktree executor resolveOrchestratorExec had no model input at all -- the descriptor carried only command/args/cwdFlag/promptFlag -- so a resolved override had no way to reach the spawned process even in principle. The baked gsd-executor.toml could not compensate because this path spawns a process rather than dispatching a named agent. Three parts, and the third is the load-bearing one. The descriptor gains modelFlag (codex: --model), keeping the per-host knowledge as descriptor data exactly as cwdFlag and promptFlag already are, so the scheduler grows no per-host branch. No other runtime declares it. The seam appends [modelFlag, model] and stays MECHANICAL: it does not know the inherit sentinel, does not know which models Codex rejects, and reads no config. Its sibling codex-agent-toml states that rule outright -- callers decide what to strip. Argv order is baseArgs, model, cwd, prompt so the prompt remains the final positional token. Omitting the model is byte-identical to before. A model starting with '-' now fails closed as unsafe_leading_dash_model, the same hazard the prompt and cwd guards already reject and which was silently accepted before. The policy lives at the caller and passes ONLY an explicit, non-sentinel per-agent pin. Passing null as the runtime resolver is what keeps profile and tier derived models out of argv, which is what Codex's session-only model posture requires: the model resolver returns 'sonnet' for the unpinned, empty and profile-only cases, and emitting that revives the documented 400 from #2310/#2311 that ADR-2313 removed. So the gate is the presence of an explicit pin, never that a value came back. * fix(#3714): enforce the real-Codex value policy the sibling surface already applies Review found one root cause behind a BLOCKER, two MAJORs, two MINORs and an argv-injection finding: the dispatch path gated on the PRESENCE of an explicit pin but never applied the VALUE policy that generateCodexAgentToml already applies to the same config key. Textbook generative divergence -- and the parity row I wrote tested resolver parity, not this policy, so it could never have caught it. BLOCKER: a global ~/.gsd/defaults.json model_overrides.gsd-executor of 'sonnet', 'opus' or 'claude-sonnet-4-5' reached codex exec --model verbatim. That is the documented 400 from #2310/#2311, arriving through the explicit-pin door rather than the tier door. The issue asks for an explicit REAL-CODEX pin; real-Codex was unenforced. Now dropped with a warning via the isAnthropicFlavoredModel predicate #3241 single-sourced for exactly this reason. Also: values are trimmed, so a whitespace-only pin is blank rather than --model " "; 'inherit' is matched case- and whitespace-insensitively, so 'Inherit' and ' inherit ' no longer reach the wire; and a value outside a model-id charset is dropped with a warning. That last one closes the injection surface -- .planning/config.json travels with a clone, and values like 'gpt-5 -c approval_policy=never' or a command-substitution value previously reached argv verbatim, where the spawner is an agent writing bash. Every rejection DROPS AND WARNS rather than failing closed. An unusable exec is not degraded, it is fatal: the dispatch step halts the wave after the worktree already exists, so a config typo would have aborted execute-phase. The stale comment claiming it degrades to sequential is corrected. Separately, the seam's own empty-model handling contradicted the committed contract and failed four tests on the remote runner. An absent, null or empty model is not an error -- it means use the host default, the same degradation cwdFlag:null already expresses. Unlike a prompt, where empty is a hang rather than a degraded run, so that one stays fail-closed. Non-string values still fail closed. Tests: the CLI rows were not HOME-hermetic and read the developer's real ~/.gsd/defaults.json, which is precisely the file the BLOCKER is about; HOME and USERPROFILE are now sandboxed per call. Adds the global-pin regression, the injection shapes, the case-variant inherit rows, and a real cross-surface divergence guard. * fix(#3714): close the flag-shaped pin, the case-variant alias, and the warning sink Round two of review found three more defects, two of which both engines reached independently, and all three were mine. The charset allowlist put the dash INSIDE the character class, so a value made only of allowed characters passed the pin policy silently and then tripped the seam's leading-dash guard, producing exec:null. That is the wave-fatal path the whole drop-and-warn design exists to avoid, reachable from a committed config file: -c, --config, -p and --dangerously-skip-permissions all reproduced it. It also regressed hosts with no model flag at all, where a dash pin turned a previously working kimi-code dispatch into exec:null. The first character is now anchored, so a flag-shaped value is dropped and warned like every other rejection, and the comment that claimed this path was unreachable is corrected. isAnthropicFlavoredModel folded case on its substring arm but not on its alias-set arm, so SONNET, Sonnet, OPUS and HAIKU all reached Codex argv while lowercase sonnet was correctly dropped -- the same 400 the drop exists to prevent. The predicate is the one #3241 single-sourced so these surfaces cannot diverge, so folding case there fixes the install-side .toml surface too. The warning wrote the rejected value RAW to stderr. Every value that fails the charset test contains by definition the characters the charset excludes, so it was a guaranteed-reachable raw-to-terminal sink: an OSC sequence in a committed config reached the operator's terminal byte for byte, and truncation could sever an escape before its reset. The value is now sanitized before truncation. Also from review: the allowlist rejected Vertex version pins like text-bison@002, a false positive on a real model id; the policy ran host-neutrally so hosts with no model flag printed a misleading drop warning on every dispatch; the invalid_model branch had no test at all; and the changeset disclosed only that a pin is delivered, not that an unusable one is now dropped with a warning. * fix(#3714): single-source the model-id charset, bound the pin, keep the flag diagnosis Round three found no blocking findings on either engine. These are the three correctness items left in code I added. The charset existed TWICE -- once to accept a pin, once to render a rejected one in the warning -- and the two copies had already drifted inside a single commit: '@' was added to the accept class and not the render class, so a Vertex-shaped value rejected for some other reason rendered as text-bison?002. Both are now derived from one definition, with a parity test asserting every character the matcher accepts survives the sanitizer unchanged, so they cannot drift again. A pin reached argv unbounded. CLAUDE.md documents the hazard: execFileSync aborts on Windows above 32,767 characters of argv. A model id has no reason to be long, so a pin over 200 characters is dropped and warned rather than truncated -- a truncated model id is a different model id. Boundary rows at 199, 200 and 201. The first character is now required to be alphanumeric, so '@evil' and '/c' no longer reach argv. Security rates both inert on codex today, so this is hardening rather than a live defect; it is here because it is one character of regex and resolveOrchestratorExec documents itself as a general descriptor-to-argv seam that other hosts may adopt. Tightening the anchor made the leading-dash branch unreachable and, with it, regressed the diagnosis: '-c' began reporting 'unsafe characters' instead of 'looks like a flag/option'. The dash check now runs before the charset test, which both restores the actionable message for the most likely user typo and keeps the branch live. A test pins the distinction between the three rejection messages so the branch cannot silently die again. * test(#3714): make the charset parity guard actually guard, and remove a false-green trap Review proved by mutation that my parity test could not do what its own comment claimed. It bound the expected character set to a local that was assigned and discarded, and the shared definition was not exported, so widening that definition left the test green. A comment overstating what a test guards is worse than no comment, because the next reader trusts it. The shared body is now exported and asserted equal, and I watched the assertion fail against a widened definition before keeping it. The sanitizer regex was module-scope, carried the g flag and was exported. Production only uses it with replace, which resets lastIndex, so there was no live bug -- but test() on a g-flagged regex alternates between calls, so any future test reaching for it would false-green. The g-flagged copy is now internal to the one call site that needs it and the exported companion carries no flags, which removes the footgun rather than documenting it. Also notes at the shared definition that it is interpolated into both a positive and a negated character class, so only plain characters and ranges are safe to add. No runtime behavior changes: the accept set, the anchor, the length cap, the sanitizer output, the rejection ordering and all three message texts are byte-identical, re-verified through the real CLI. * chore(#3714): backfill changeset pr number * chore(#3714): re-trigger CI after the GitHub Actions outage The workflow runs for this branch were created during the Actions major outage on 2026-08-26 and never got scheduled. They are wedged: GitHub reports them queued, refuses to cancel them, and refuses to rerun them because it believes the workflow is already running. The Tests run is also pinned to a superseded sha, so no test run exists for the current head at all. Actions is operational again and the repo-wide queue has drained, so a fresh push is what creates schedulable runs. This commit is empty on purpose: nothing about the change is being altered, and the verified content is byte-identical to 7860c6ccc. --------- Co-authored-by: sim <sim@local>
216 lines
8.8 KiB
JavaScript
216 lines
8.8 KiB
JavaScript
// Guards the dispatch model-pin VALUE policy in gsd-core/bin/gsd-tools.cjs
|
|
// (`resolveDispatchModelPin`, `MODEL_ID_CHARSET_RE`, `MODEL_ID_CHARSET_BODY`,
|
|
// `MODEL_ID_SANITIZE_STRIP_RE`, `MODEL_ID_MAX_LENGTH`):
|
|
//
|
|
// - Item 1: the accept-class (matcher) and the render-class (sanitizer)
|
|
// are single-sourced from one character-class body and can never drift
|
|
// apart again the way they already did once (accept regex gained '@'
|
|
// for Vertex pins; sanitizer keep-class did not, so a rejected
|
|
// Vertex-shaped pin rendered "text-bison?002" instead of
|
|
// "text-bison@002").
|
|
// - Item 2: a pin longer than MODEL_ID_MAX_LENGTH (200) is dropped with a
|
|
// warning rather than reaching argv truncated. Boundary rows at
|
|
// limit-1/limit/limit+1 (199/200/201).
|
|
// - Item 3: the leading character must be alphanumeric, closing off
|
|
// '@'/'/' -shaped values (`@evil`, `/c`) from reaching argv, while five
|
|
// legitimate real-world model ids continue to pass unchanged.
|
|
//
|
|
// Every rejection path must degrade to "no model" (drop-and-warn) — never
|
|
// `undefined` being skipped in favor of an exception, and never a value
|
|
// that still contains an unsafe character escaping to argv.
|
|
|
|
const { describe, test } = require('node:test');
|
|
const assert = require('node:assert/strict');
|
|
const fc = require('fast-check');
|
|
|
|
const gsdTools = require('../gsd-core/bin/gsd-tools.cjs');
|
|
const {
|
|
resolveDispatchModelPin,
|
|
MODEL_ID_CHARSET_RE,
|
|
MODEL_ID_CHARSET_BODY,
|
|
MODEL_ID_SANITIZE_STRIP_RE,
|
|
MODEL_ID_MAX_LENGTH,
|
|
} = gsdTools;
|
|
|
|
/** Capture process.stderr.write() calls made during `fn()`. */
|
|
function captureStderr(fn) {
|
|
const original = process.stderr.write;
|
|
const chunks = [];
|
|
process.stderr.write = (chunk) => {
|
|
chunks.push(String(chunk));
|
|
return true;
|
|
};
|
|
try {
|
|
fn();
|
|
} finally {
|
|
process.stderr.write = original;
|
|
}
|
|
return chunks.join('');
|
|
}
|
|
|
|
describe('#3714 follow-up: dispatch model-pin VALUE policy', () => {
|
|
test('MODEL_ID_MAX_LENGTH is the documented 200', () => {
|
|
assert.strictEqual(MODEL_ID_MAX_LENGTH, 200);
|
|
});
|
|
|
|
test('five legitimate real-world model ids all pass through unchanged', () => {
|
|
const ids = [
|
|
'gpt-5.6-terra',
|
|
'synthetic/hf:zai-org/GLM-5.2',
|
|
'text-bison@002',
|
|
'gpt-4o_mini',
|
|
'azure/deployment',
|
|
];
|
|
for (const id of ids) {
|
|
const stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin(`agent-${id}`, id), id);
|
|
});
|
|
assert.strictEqual(stderr, '', `expected no warning for legitimate id "${id}"`);
|
|
}
|
|
});
|
|
|
|
test('item 3: leading "@" and leading "/" are dropped with a warning, never reach argv', () => {
|
|
for (const bad of ['@evil', '/c']) {
|
|
const stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin(`agent-${bad}`, bad), undefined);
|
|
});
|
|
assert.match(stderr, /gsd: warning/);
|
|
assert.match(stderr, /unsafe characters/);
|
|
}
|
|
});
|
|
|
|
test('leading-dash values report the flag/option message, not the generic unsafe-characters message (LEADING_DASH_RE must run before MODEL_ID_CHARSET_RE)', () => {
|
|
for (const bad of ['-c', '--config', '-', '--', '-p']) {
|
|
const stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin(`agent-${bad}`, bad), undefined);
|
|
});
|
|
assert.match(stderr, /gsd: warning/);
|
|
assert.match(stderr, /looks like a flag\/option, not a model id \(leading "-"\)/);
|
|
assert.doesNotMatch(stderr, /unsafe characters/);
|
|
}
|
|
});
|
|
|
|
test('non-dash out-of-charset values still report the generic unsafe-characters message', () => {
|
|
for (const bad of ['@evil', 'has a space']) {
|
|
const stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin(`agent-x`, bad), undefined);
|
|
});
|
|
assert.match(stderr, /gsd: warning/);
|
|
assert.match(stderr, /unsafe characters/);
|
|
assert.doesNotMatch(stderr, /flag\/option/);
|
|
}
|
|
});
|
|
|
|
test('item 2: boundary rows at limit-1/limit/limit+1 (199/200/201)', () => {
|
|
const at199 = 'a'.repeat(199);
|
|
const at200 = 'a'.repeat(200);
|
|
const at201 = 'a'.repeat(201);
|
|
|
|
let stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin('agent-199', at199), at199);
|
|
});
|
|
assert.strictEqual(stderr, '', '199 chars must emit with no warning');
|
|
|
|
stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin('agent-200', at200), at200);
|
|
});
|
|
assert.strictEqual(stderr, '', '200 chars must emit with no warning');
|
|
|
|
stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin('agent-201', at201), undefined);
|
|
});
|
|
assert.match(stderr, /gsd: warning/);
|
|
assert.match(stderr, /exceeds the maximum model id length \(200 characters\)/);
|
|
});
|
|
|
|
test('item 2: an over-length pin is dropped, never truncated into a shorter value', () => {
|
|
// A truncated model id is a different model id — the resolver must
|
|
// never return a 200-char prefix of a 5000-char input.
|
|
const huge = 'a'.repeat(5000);
|
|
const result = captureStderr(() => resolveDispatchModelPin('agent-huge', huge));
|
|
assert.match(result, /exceeds the maximum model id length/);
|
|
});
|
|
|
|
test('item 1 (parity): a rejected Vertex-shaped pin renders "@" correctly, not "?"', () => {
|
|
// Append a control byte so the value fails the charset test (and is
|
|
// therefore routed through the sanitizer) while still containing '@'.
|
|
const rawValue = 'text-bison@002' + String.fromCharCode(27);
|
|
const stderr = captureStderr(() => {
|
|
assert.strictEqual(resolveDispatchModelPin('agent-vertex', rawValue), undefined);
|
|
});
|
|
assert.match(stderr, /"text-bison@002\?"/, 'the "@" must survive the sanitizer unchanged');
|
|
assert.doesNotMatch(stderr, /text-bison\?002/, 'the "@" must never be sanitized to "?"');
|
|
});
|
|
|
|
test('item 1 (parity, exhaustive): every character in the accept-class body survives the sanitizer unchanged', () => {
|
|
// Every printable character that MODEL_ID_CHARSET_RE accepts as a
|
|
// non-leading character must also survive MODEL_ID_SANITIZE_STRIP_RE
|
|
// unchanged — the two are derived from one shared body, so this can
|
|
// never regress silently again.
|
|
const acceptedNonLeadingChars = 'A-Za-z0-9._:/@-';
|
|
// Expand the class body into a concrete character list (letters, digits,
|
|
// and the literal punctuation), independent of the regex-escaping used
|
|
// to define it, so the assertion doesn't just re-check the definition
|
|
// against itself.
|
|
const chars = [];
|
|
for (let c = 65; c <= 90; c++) chars.push(String.fromCharCode(c)); // A-Z
|
|
for (let c = 97; c <= 122; c++) chars.push(String.fromCharCode(c)); // a-z
|
|
for (let c = 48; c <= 57; c++) chars.push(String.fromCharCode(c)); // 0-9
|
|
chars.push('.', '_', ':', '/', '@', '-');
|
|
assert.ok(chars.length > 0);
|
|
|
|
for (const ch of chars) {
|
|
const value = 'x' + ch; // 'x' keeps the leading-char anchor satisfied
|
|
assert.match(value, MODEL_ID_CHARSET_RE, `"${value}" should be accepted by MODEL_ID_CHARSET_RE`);
|
|
const sanitized = value.replace(MODEL_ID_SANITIZE_STRIP_RE, '?');
|
|
assert.strictEqual(sanitized, value, `"${ch}" is accepted but was sanitized away`);
|
|
}
|
|
// Assert the literal class body above EQUALS the exported production
|
|
// value, so this test fails if gsd-tools.cjs's MODEL_ID_CHARSET_BODY is
|
|
// edited (e.g. widened) without updating this test's expectations.
|
|
assert.strictEqual(
|
|
acceptedNonLeadingChars,
|
|
MODEL_ID_CHARSET_BODY,
|
|
'this test\'s expected charset has drifted from gsd-tools.cjs\'s MODEL_ID_CHARSET_BODY',
|
|
);
|
|
});
|
|
|
|
test('item 1 (property): fast-check — any string accepted by MODEL_ID_CHARSET_RE is unchanged by the sanitizer', () => {
|
|
const bodyChar = fc.constantFrom(
|
|
...'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789._:/@-'.split(''),
|
|
);
|
|
const leadChar = fc.constantFrom(
|
|
...'ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789'.split(''),
|
|
);
|
|
fc.assert(
|
|
fc.property(leadChar, fc.array(bodyChar, { maxLength: 40 }), (lead, rest) => {
|
|
const value = lead + rest.join('');
|
|
assert.match(value, MODEL_ID_CHARSET_RE);
|
|
const sanitized = value.replace(MODEL_ID_SANITIZE_STRIP_RE, '?');
|
|
assert.strictEqual(sanitized, value);
|
|
}),
|
|
);
|
|
});
|
|
|
|
test('every rejection path degrades to undefined (drop-and-warn), never throws', () => {
|
|
const inputs = [
|
|
'@evil',
|
|
'/c',
|
|
'-c',
|
|
'--config',
|
|
'a'.repeat(201),
|
|
'sonnet',
|
|
String.fromCharCode(27) + 'malicious',
|
|
'',
|
|
' ',
|
|
'inherit',
|
|
'INHERIT',
|
|
];
|
|
for (const input of inputs) {
|
|
assert.doesNotThrow(() => {
|
|
captureStderr(() => resolveDispatchModelPin('agent-x', input));
|
|
}, `resolveDispatchModelPin must never throw for input ${JSON.stringify(input)}`);
|
|
}
|
|
});
|
|
});
|