diff --git a/.changeset/patient-cranes-parade.md b/.changeset/patient-cranes-parade.md new file mode 100644 index 000000000..5db274f71 --- /dev/null +++ b/.changeset/patient-cranes-parade.md @@ -0,0 +1,5 @@ +--- +type: Fixed +pr: 3867 +--- +**OpenCode subagents now carry the reasoning effort GSD resolved for them** — `query resolve-execution` reported an effort level that never reached the generated OpenCode agent, so every subagent ran at whatever the runtime defaulted the model to, silently ignoring `effort` config. The bake now emits a `variant` key alongside `model`, and `effort sync` maintains it, so changing effort config no longer needs a reinstall. The key is written only when effort is actually configured; `inherit` and any level OpenCode does not accept omit it rather than naming a variant that cannot resolve. Config-supplied `model` and `variant` values are also quoted whenever YAML would not read them back verbatim — previously a value containing a newline could inject extra top-level keys into a generated agent file, and values like `no`, `12:30`, `@org/model` or a bare date were silently retyped or truncated. (#3706) diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index fef50dc8f..ad7441299 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1560,7 +1560,13 @@ Control the reasoning effort of agent invocations with a single config. The univ minimal < low < medium < high < xhigh < max ``` -Effort is rendered per-runtime: `output_config.effort` for Claude (Claude Code subagent `effort` frontmatter / `CLAUDE_CODE_EFFORT_LEVEL` env), `model_reasoning_effort` for Codex (Responses API `reasoning.effort`). +Effort is rendered per-runtime: `output_config.effort` for Claude (Claude Code subagent `effort` frontmatter / `CLAUDE_CODE_EFFORT_LEVEL` env), `model_reasoning_effort` for Codex (Responses API `reasoning.effort`), and `variant` for OpenCode (agent frontmatter). + +**OpenCode `variant` is opt-in ([#3706](https://github.com/open-gsd/gsd-core/issues/3706)).** OpenCode resolves a `variant` name against the variants available for the agent's model — the built-in sets are provider-specific (Anthropic ships `high` and `max`; OpenAI the full ladder) and you can define your own in `opencode.jsonc`. GSD writes the key only when an `effort` block is actually configured; with no `effort` config the generated agent carries no `variant` line and OpenCode applies its own default. + +Note that the gate is on effort being configured **at all**, not on the individual agent being named. Once any `effort` block exists, the usual cascade resolves a level for *every* agent — an `agent_overrides` entry for one agent still leaves the others resolving through `routing_tier_defaults` and the tier ladder — so every generated OpenCode agent gets a `variant` line, not only the one you named. Two levels are never written: `inherit` (which means "follow the host default", so the key is omitted) and any level outside OpenCode's supported set. Runtimes with no declared effort surface — Kilo among them — never receive the key at all. + +`effort sync` maintains the key too, so changing `effort` config does not require a reinstall: it writes the newly resolved `variant` into each installed OpenCode agent, and removes the key when the agent resolves to `inherit` or to a level OpenCode does not accept — the same states under which install writes nothing. **Cross-provider clamping:** `minimal` is Anthropic-unsupported — it clamps to `low` on Claude. @@ -1658,8 +1664,9 @@ The model-catalog's `reasoning_effort` per-tier hint is a legacy field kept for Valid effort values: `minimal`, `low`, `medium`, `high`, `xhigh`, `max`, and `inherit` ([#3533](https://github.com/open-gsd/gsd-core/issues/3533)). `inherit` means "follow the session/host default" — it is a declarable choice, not a level: -at install time the agent's `effort:` frontmatter key (claude) or `model_reasoning_effort` -pin (Codex `.toml`) is **omitted** for an agent resolving to `inherit`; `effort sync` treats +at install time the agent's `effort:` frontmatter key (claude), `model_reasoning_effort` +pin (Codex `.toml`), or `variant:` frontmatter key (OpenCode) is **omitted** for an agent +resolving to `inherit`; `effort sync` treats an absent key as the correct in-sync state and strips a present one; no runtime ever receives the literal. An explicit `inherit` also never escalates on failed attempts — your choice outranks the automatic ladder. diff --git a/scripts/mutation-matrix.cjs b/scripts/mutation-matrix.cjs index bc3af2802..10dab79f4 100644 --- a/scripts/mutation-matrix.cjs +++ b/scripts/mutation-matrix.cjs @@ -114,12 +114,24 @@ const TARGET_MUTATION_SCORE = 80; // Measured CI scores 2026-06-14 (issue #1187, timeout-free — source of truth): // context-utilization 92.31% → floor 80 (target already met) // prompt-budget 68.33% → floor 66 (local was 99.6% — TIMEOUT INFLATION; CI is the truth) -// frontmatter 63.35% → floor 62 +// frontmatter 63.35% → floor 62 (SUPERSEDED — see 2026-08-25 below) // adr-parser 69.30% → floor 68 // config-schema 54.55% → floor 52 (local was 69.7% — TIMEOUT INFLATION; CI is the truth) // active-workstream-store 81.91% → floor 80 // core-utils 77.52% → floor 75 // +// Measured CI score 2026-08-25 (#3706, PR 3867): +// frontmatter 66.67% → floor 65 +// #3706 added agentScalarNeedsDoubleQuoting to frontmatter.cts and exported +// escapeDoubleQuoted, but the tests constraining them lived in +// tests/runtime-converters.test.cjs, which this lane does NOT run — the same trap the +// #1882 note on the frontmatter entry describes. The shard fell to 60.58 and broke the +// floor. Direct unit tests for both were added to tests/frontmatter.unit.test.cjs, each +// clause paired with a near-miss that must answer the opposite way, which took the module +// above its pre-change score. Floor ratcheted per the HOW TO UPDATE formula above, and +// RATCHET_BASELINE — which lives in tests/mutation-matrix-ratchet.test.cjs, not here — is +// updated in the same diff as that procedure requires. +// // LESSON: floors MUST be calibrated from CI mutation runs (CI runs with // timeout≈0, deterministic). Local runs count timeouts as kills and // inflate scores significantly (prompt-budget: 99.6% local vs 68.3% CI; @@ -169,7 +181,7 @@ const COVERED = { // no test in the shard, so the module's score drops even though the behaviour is tested. 'tests/unusable-input.test.cjs', ], - minScore: 62, + minScore: 65, }, 'adr-parser': { cjs: 'gsd-core/bin/lib/adr-parser.cjs', diff --git a/src/commands.cts b/src/commands.cts index b2055ca7a..a41ac1119 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -10,6 +10,7 @@ import fs from 'node:fs'; import path from 'node:path'; import { normalizeEol } from './text-lines.cjs'; import { execGit, platformWriteSync, platformReadSync, platformEnsureDir, isSpawnTimeout, retryRenameSync } from './shell-command-projection.cjs'; +import { escapeRegex } from './pattern.cjs'; import { requireSafePath, sanitizeForDisplay } from './security.cjs'; // eslint-disable-next-line @typescript-eslint/no-require-imports import ioMod = require('./io.cjs'); @@ -51,7 +52,7 @@ import planningWorkspace = require('./planning-workspace.cjs'); const { planningDir, planningPaths } = planningWorkspace; // eslint-disable-next-line @typescript-eslint/no-require-imports import frontmatter = require('./frontmatter.cjs'); -const { extractFrontmatter } = frontmatter; +const { extractFrontmatter, agentScalarNeedsDoubleQuoting, escapeDoubleQuoted } = frontmatter; // eslint-disable-next-line @typescript-eslint/no-require-imports import modelProfiles = require('./model-profiles.cjs'); const { MODEL_PROFILES, VALID_PHASE_TYPES } = modelProfiles; @@ -106,6 +107,9 @@ interface CommitToSubrepoRepoResult { interface EffortSyncChange { agent: string; + // #3533 (10d) / #3706: from === null means the key was ABSENT; from === '' + // means the key was PRESENT with an empty value. Collapsing both to null + // would make "no key" and "empty key" indistinguishable in sync output. from: string | null; // #3533 (10d): to === null is the typed IR for omission (inherit strips the key). to: string | null; @@ -738,45 +742,113 @@ function effortSurfaceForHost(cwd: string, host: string): string { } /** - * #488 — Replace or inject the `effort:` value in YAML frontmatter. + * #488 — Replace or inject the `:` value in YAML frontmatter. * Unlike injectEffortFrontmatter (install.js), this overwrites an existing value. + * #3706: key-parameterised so the same line-editor serves both claude's + * `effort:` and OpenCode's `variant:`. #3706: all offsets (eol, openLen, + * closingStart) are derived from the MATCHED BLOCK, not the start of the + * file, and the existing-key replace is scoped to the frontmatter span only. */ -function setEffortFrontmatter(content: string, effortValue: string): string { - const eol = /^---\r\n/.test(content) ? '\r\n' : '\n'; +function setFrontmatterKeyLine(content: string, key: string, value: string): string { const fmRe = /^---\r?\n([\s\S]*?)^---\r?$/m; const match = fmRe.exec(content); if (!match) return content; const fmBody = match[1]; - if (/^effort:/m.test(fmBody)) { - return content.replace(/^(effort:)[ \t]*.*$/m, `$1 ${effortValue}`); - } + // Both writers of these frontmatter keys — this sync path and the + // install-side `frontmatterScalar` in runtime-artifact-conversion.cts — + // now share one escaping rule: quote via `agentScalarNeedsDoubleQuoting` + + // `escapeDoubleQuoted` (both from frontmatter.cts) rather than each + // interpolating `value` raw/differently. + const renderedValue = agentScalarNeedsDoubleQuoting(value) ? `"${escapeDoubleQuoted(value)}"` : value; + // EOL comes from the MATCHED BLOCK, not the start of the file. With a + // preamble the two can disagree, and on a CRLF document that misaligns every + // offset below by one byte and mangles the opening fence. + const eol = /^---\r\n/.test(match[0]) ? '\r\n' : '\n'; const openLen = 3 + eol.length; - const closingStart = match.index + openLen + fmBody.length; - return content.slice(0, closingStart) + `effort: ${effortValue}${eol}` + content.slice(closingStart); + const bodyStart = match.index + openLen; + const closingStart = bodyStart + fmBody.length; + // #3706: key is now generic (not just the literal 'effort'/'variant' + // callers happen to pass today) — escape it before interpolating into the + // RegExp so a future caller can't have its key metacharacters reinterpreted. + const keyLineRe = new RegExp(`^(${escapeRegex(key)}:)[ \\t]*.*$`, 'm'); + if (keyLineRe.test(fmBody)) { + // #3706: a duplicated `:` line is already invalid YAML, but a + // non-first-wins reader (last-wins) would otherwise honour a stale + // second occurrence left behind by a naive single-hit replace, while + // this function's own single-hit read reports "in sync" — a + // permanently non-converging state. Use a GLOBAL replace with a + // first-hit flag so every occurrence collapses to exactly one, IN THE + // POSITION of the first occurrence (never delete-then-append, which + // would move the key to the end of the frontmatter and churn every + // already-generated single-occurrence file). + const escaped = escapeRegex(key); + let seen = false; + const newBody = fmBody.replace( + new RegExp(`^${escaped}:[ \\t]*.*(\\r?\\n?)`, 'gm'), + (_m, nl) => { + if (!seen) { + seen = true; + return `${key}: ${renderedValue}${nl}`; + } + return ''; + }, + ); + // Replace INSIDE the frontmatter span only: a whole-file /m replace would + // rewrite an earlier preamble line that happens to start with this key. + return content.slice(0, bodyStart) + newBody + content.slice(closingStart); + } + return content.slice(0, closingStart) + `${key}: ${renderedValue}${eol}` + content.slice(closingStart); } /** - * #3533 (10d) — remove exactly the frontmatter `effort:` line (and its line + * #3533 (10d) — remove exactly the frontmatter `:` line (and its line * ending) so an agent configured for `inherit` carries NO key. Mirrors the * codex-agent-toml strip discipline: targeted line removal, EOL-aware, every * other byte (comments, sibling keys, the body) untouched. + * #3706: key-parameterised so the same line-editor serves both claude's + * `effort:` and OpenCode's `variant:`. #3706: openLen is derived from the + * MATCHED BLOCK, not the start of the file — a preamble on a CRLF document + * would otherwise misalign every offset below. */ -function removeEffortFrontmatter(content: string): string { +function removeFrontmatterKeyLine(content: string, key: string): string { // Scoped to the FIRST frontmatter block (not a whole-file /m match): a - // preamble or body line starting with `effort:` (a fenced config example, + // preamble or body line starting with `:` (a fenced config example, // a thematic-break flanked fragment) must never be the line removed. const fmRe = /^---\r?\n([\s\S]*?)^---\r?$/m; const match = fmRe.exec(content); if (!match) return content; const fmBody = match[1]; - const lineRe = /^effort:[ \t]*.*\r?\n?/m; + // #3706: same generic-key escape as setFrontmatterKeyLine above. + const lineRe = new RegExp(`^${escapeRegex(key)}:[ \\t]*.*\\r?\\n?`, 'm'); if (!lineRe.test(fmBody)) return content; - const strippedFm = fmBody.replace(lineRe, ''); - const openLen = 3 + (/^---\r\n/.test(content) ? 2 : 1); + // A duplicate `:` mapping key is already invalid YAML (a document with + // two `effort:`/`variant:` lines does not parse), so this is robustness + // against a malformed document, not a live corruption path. Still, "a null + // target means the key must not exist" is an invariant this function must + // leave true on disk — a non-global replace here would strip only the + // FIRST occurrence and require a second run to converge. Use a fresh + // global RegExp for the strip so every occurrence in the frontmatter body + // is removed in one pass. + const stripAllRe = new RegExp(`^${escapeRegex(key)}:[ \\t]*.*\\r?\\n?`, 'gm'); + const strippedFm = fmBody.replace(stripAllRe, ''); + // Same rule as setFrontmatterKeyLine: the EOL must come from the matched + // block, not the start of the file, or a preambled CRLF document misaligns. + const eol = /^---\r\n/.test(match[0]) ? '\r\n' : '\n'; + const openLen = 3 + eol.length; const closingStart = match.index + openLen + fmBody.length; return content.slice(0, match.index + openLen) + strippedFm + content.slice(closingStart); } +/** #488 — Replace or inject the `effort:` value in YAML frontmatter. */ +function setEffortFrontmatter(content: string, effortValue: string): string { + return setFrontmatterKeyLine(content, 'effort', effortValue); +} + +/** #3533 (10d) — remove exactly the frontmatter `effort:` line (and its line ending). */ +function removeEffortFrontmatter(content: string): string { + return removeFrontmatterKeyLine(content, 'effort'); +} + /** * #488 — Re-sync effort: frontmatter in all installed gsd-*.md agent files to * match the current effort config, without requiring a full reinstall. @@ -803,6 +875,15 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con return; } + // #3706: install now bakes OpenCode's resolved effort into agent + // frontmatter under the `variant:` key (not `effort:`), so OpenCode gets + // its own sync path — mirroring the codex branch above — rather than + // falling into the generic "does not use effort: frontmatter" skip. + if (runtime === 'opencode') { + cmdEffortSyncOpencode(cwd, raw, dryRun, opts.configDir); + return; + } + if (runtime !== 'claude') { output({ synced: 0, skipped: 0, changes: [], dry_run: dryRun, reason: `runtime '${runtime}' does not use effort: frontmatter` }, raw, ''); return; @@ -816,8 +897,8 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con // package-root bin/install.js, which the installer never copies into a runtime home). // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method const { readGsdEffectiveEffortConfig, resolveInstallTimeEffort } = require('./install-effort-resolver.cjs') as { - readGsdEffectiveEffortConfig(cwd: string): Record; - resolveInstallTimeEffort(cfg: Record, agentName: string): string; + readGsdEffectiveEffortConfig(cwd: string): Record | null; + resolveInstallTimeEffort(cfg: Record | null, agentName: string): string; }; const effortCfg = readGsdEffectiveEffortConfig(cwd); @@ -832,15 +913,47 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con const files = fs.readdirSync(agentsDir).filter(f => { if (!f.startsWith('gsd-') || !f.endsWith('.md')) return false; try { return fs.lstatSync(path.join(agentsDir, f)).isFile(); } catch { return false; } - }); + }).sort(); // #3706: sorted like the codex and + // opencode branches — readdir order is platform-dependent, so leaving it unsorted makes the + // reported `changes` ordering differ across machines for identical inputs. + const changes: EffortSyncChange[] = []; let synced = 0; let skipped = 0; + // Local-only counter: reads AND writes are both guarded in this loop (an + // unreadable or unwritable agent file must not abort the whole sweep), but + // this result shape (`{synced, skipped, changes, dry_run, agents_dir}`) is + // long-standing and widely consumed, so it deliberately gains NO new key + // (no `read_failures`/`write_failures`, unlike the codex/opencode branches + // below). Instead every per-file failure — read or write — is folded into + // `skipped` and rides the raw-mode summary token below — `output()`'s + // third argument is never merged into the emitted JSON object (see io.cts + // `output()`: it is only read when `raw === true`, entirely replacing the + // JSON payload), so flipping it to `'failed'` costs nothing in the wire + // shape while still surfacing the failure to a raw-mode caller. The three + // branches differ on that reporting shape, but are now also consistent in + // HOW they publish: every write below goes through the same tmp-file + + // chmod + retryRenameSync atomic-publish sequence used by + // cmdEffortSyncCodex and cmdEffortSyncOpencode, so a fault mid-write can + // never leave an agent file truncated or empty. + let fileFailureCount = 0; for (const file of files) { const agentName = file.replace(/\.md$/, ''); const filePath = path.join(agentsDir, file); - const content = fs.readFileSync(filePath, 'utf8'); + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch { + // An unreadable agent file must not abort the whole sweep. Deliberately + // NOT adding a new field here: this result shape (`{synced, skipped, + // changes, dry_run, agents_dir}`) is long-standing and widely consumed, + // so the failure is folded into `skipped` only, with no + // read_failures/write_failures list — see `fileFailureCount` above. + skipped++; + fileFailureCount++; + continue; + } // Resolve using install-time logic: home defaults merged with project config. const universalEffort = resolveInstallTimeEffort(effortCfg, agentName); @@ -850,16 +963,66 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con // drift and the sync re-added a hand-stripped key on every apply. A // present key under inherit is stripped, reported as {from, to: null}. if (universalEffort === 'inherit') { - // eslint-disable-next-line local/no-unbounded-quantifier -- same lazy `*?` bounded by the `^---$/m` closing anchor as the concrete-path fmMatch below; duplicated here so the inherit branch validates against the same frontmatter span the strip targets const fmMatchInherit = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(content); if (!fmMatchInherit) { skipped++; continue; } + // Presence and value are distinct questions: `effort:` with an EMPTY + // value is a key that IS present but whose captured value is null (the + // `(.+?)` group requires at least one char). Deciding "already correct" + // from a null value alone is wrong here — it would leave an + // unresolvable `effort: null` key on disk forever. Test presence with + // its own regex, and only compare values once presence is known. + const effortPresentInherit = /^effort:/m.test(fmMatchInherit[1]); + if (!effortPresentInherit) { skipped++; continue; } const effortMatchInherit = /^effort:[ \t]*(.+?)[ \t]*$/m.exec(fmMatchInherit[1]); - if (!effortMatchInherit) { skipped++; continue; } - changes.push({ agent: agentName, from: effortMatchInherit[1], to: null }); - synced++; + // `effortPresentInherit` is guaranteed true here (checked above), so a + // failed value match means the key is present with an EMPTY value — + // report `''`, not `null`, so "present-but-empty" is never conflated + // with "absent" in the sync output. if (!dryRun) { - fs.writeFileSync(filePath, removeEffortFrontmatter(content)); + // Atomic publish AND mode preservation, same discipline as + // cmdEffortSyncCodex/cmdEffortSyncOpencode: write to a sibling tmp + // file, chmod it to match filePath's existing (masked) mode, then + // retryRenameSync it over the target so filePath is either the old + // bytes or the new ones, never half-written and never dropped to a + // default mode. On any failure the tmp file is unlinked (best-effort) + // and the write is reported (folded into `skipped`/`fileFailureCount`, + // no new field), not thrown, so the remaining agents still get + // processed. ONE failure path for this site — no nested try/catch. + const tmpPathInherit = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so its mode can be passed at + // CREATION time — a plain `writeFileSync(tmpPath, data)` creates the + // tmp file at the default `0666 & ~umask` even when filePath is more + // restrictive. Best-effort only: a stat failure must not abort the + // sync, since the content write is what matters, not the mode. + let originalModeInherit: number | undefined; + try { + originalModeInherit = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync( + tmpPathInherit, + removeEffortFrontmatter(content), + originalModeInherit !== undefined ? { mode: originalModeInherit } : undefined, + ); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its + // OLD mode instead, and this chmod is what corrects that case. + // Best-effort only: a chmod failure must not abort the sync, since + // the content write is what matters, not the mode. + try { + if (originalModeInherit !== undefined) fs.chmodSync(tmpPathInherit, originalModeInherit); + } catch { /* non-fatal: proceed with default tmp-file mode */ } + retryRenameSync(tmpPathInherit, filePath); + } catch { + try { fs.unlinkSync(tmpPathInherit); } catch { /* already gone or never created */ } + skipped++; + fileFailureCount++; + continue; + } } + changes.push({ agent: agentName, from: effortMatchInherit ? effortMatchInherit[1] : '', to: null }); + synced++; continue; } @@ -868,24 +1031,79 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con const rendered = renderEffortForRuntime(runtime, universalEffort); const newEffortValue = rendered.value as string; - // eslint-disable-next-line local/no-unbounded-quantifier -- lazy `*?` bounded by the `^---$/m` closing anchor, no nested quantifier, measured linear to 5MB (no-closing-marker adversarial input) const fmMatch = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(content); if (!fmMatch) { skipped++; continue; } + // Presence and value are distinct questions here too: `currentEffort` + // reads null both when the key is ABSENT and when it is present with an + // EMPTY value. `effortPresent` disambiguates those two for the reported + // `from` below (never `null` when the key is present but empty) — but it + // has no bearing on the skip check that follows: `newEffortValue` is + // never null on this path (guarded above), so an absent key already + // yields `currentEffort === null !== newEffortValue` without consulting + // presence separately. + const effortPresent = /^effort:/m.test(fmMatch[1]); const effortMatch = /^effort:[ \t]*(.+?)[ \t]*$/m.exec(fmMatch[1]); - const currentEffort = effortMatch ? effortMatch[1] : null; + // `null` (key absent) and `''` (key present, value empty) are distinct + // states `effortPresent` deliberately disambiguates — collapsing both to + // `null` here would make the reported `from` lie about which case fired. + const currentEffort = effortPresent ? (effortMatch ? effortMatch[1] : '') : null; if (currentEffort === newEffortValue) { skipped++; continue; } + if (!dryRun) { + // Atomic publish AND mode preservation, same discipline as + // cmdEffortSyncCodex/cmdEffortSyncOpencode: write to a sibling tmp + // file, chmod it to match filePath's existing (masked) mode, then + // retryRenameSync it over the target so filePath is either the old + // bytes or the new ones, never half-written and never dropped to a + // default mode. On any failure the tmp file is unlinked (best-effort) + // and the write is reported (folded into `skipped`/`fileFailureCount`, + // no new field), not thrown, so the remaining agents still get + // processed. ONE failure path for this site — no nested try/catch. + const tmpPathSet = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so its mode can be passed at CREATION + // time — a plain `writeFileSync(tmpPath, data)` creates the tmp file + // at the default `0666 & ~umask` even when filePath is more + // restrictive. Best-effort only: a stat failure must not abort the + // sync, since the content write is what matters, not the mode. + let originalModeSet: number | undefined; + try { + originalModeSet = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync( + tmpPathSet, + setEffortFrontmatter(content, newEffortValue), + originalModeSet !== undefined ? { mode: originalModeSet } : undefined, + ); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its OLD + // mode instead, and this chmod is what corrects that case. + // Best-effort only: a chmod failure must not abort the sync, since + // the content write is what matters, not the mode. + try { + if (originalModeSet !== undefined) fs.chmodSync(tmpPathSet, originalModeSet); + } catch { /* non-fatal: proceed with default tmp-file mode */ } + retryRenameSync(tmpPathSet, filePath); + } catch { + try { fs.unlinkSync(tmpPathSet); } catch { /* already gone or never created */ } + skipped++; + fileFailureCount++; + continue; + } + } + changes.push({ agent: agentName, from: currentEffort, to: newEffortValue }); synced++; - - if (!dryRun) { - fs.writeFileSync(filePath, setEffortFrontmatter(content, newEffortValue)); - } } - output({ synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir }, raw, synced > 0 ? 'changed' : 'ok'); + output( + { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir }, + raw, + fileFailureCount > 0 ? 'failed' : synced > 0 ? 'changed' : 'ok', + ); } /** One `{agent, field, from}` strip reported by {@link cmdEffortSyncCodex} — `to` is always omission (`null`). */ @@ -903,8 +1121,8 @@ interface CodexEffortSyncRefusal { reason: string; } -/** A write that failed mid-sync (fs fault), reported so the remaining agents still get processed. */ -interface CodexEffortSyncWriteFailure { +/** A read or write that failed mid-sync (fs fault), reported so the remaining agents still get processed. */ +interface EffortSyncFileFailure { agent: string; file: string; error: string; @@ -919,8 +1137,9 @@ interface CodexEffortSyncWriteFailure { * document is refused and reported, never partially rewritten (40-design.md * "Reconciliation" — parseCodexAgentToml is the STRICT half of the reader/ * writer split). Result shape is additive over the claude branch's - * `{synced, skipped, changes, dry_run, agents_dir}` — `refused` and - * `write_failures` are new fields, never a reshape of the existing ones. + * `{synced, skipped, changes, dry_run, agents_dir}` — `refused`, + * `write_failures`, and `read_failures` are new fields, never a reshape of + * the existing ones. */ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): void { // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method @@ -944,14 +1163,26 @@ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): const changes: CodexEffortSyncChange[] = []; const refused: CodexEffortSyncRefusal[] = []; - const writeFailures: CodexEffortSyncWriteFailure[] = []; + const writeFailures: EffortSyncFileFailure[] = []; + const readFailures: EffortSyncFileFailure[] = []; let synced = 0; let skipped = 0; for (const file of files) { const agentName = file.replace(/\.toml$/, ''); const filePath = path.join(agentsDir, file); - const content = fs.readFileSync(filePath, 'utf8'); + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch (err) { + // An unreadable agent file must not abort the whole sweep — mirrors the + // opencode branch's own read guard, reported under its own + // `read_failures` key so a caller can tell "never read" apart from + // "read but write failed". + skipped++; + readFailures.push({ agent: agentName, file: filePath, error: err instanceof Error ? err.message : String(err) }); + continue; + } const parsed = parseCodexAgentToml(content); if (!parsed.ok) { @@ -998,8 +1229,29 @@ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): // bare fs.renameSync) carries the transient-Windows-lock retry per // DEFECT.WINDOWS-FS-OPS. const tmpPath = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so the original mode is available to + // pass at creation time, not just at chmod time afterward — otherwise + // the tmp file is briefly created at the default `0666 & ~umask` + // (world-readable under a typical 022 umask) even when filePath is + // e.g. 0600, exposing its contents for the window between creation and + // chmod. Best-effort: a stat failure must not abort the sync, since the + // content write is what matters, not the mode. + let originalMode: number | undefined; try { - fs.writeFileSync(tmpPath, renderCodexAgentToml(doc)); + originalMode = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync(tmpPath, renderCodexAgentToml(doc), originalMode !== undefined ? { mode: originalMode } : undefined); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its OLD + // mode instead, and this chmod is what corrects that case. Mask off + // the file-type bits fs.statSync().mode carries (POSIX leaves + // chmod's handling of those unspecified); best-effort only, since + // the content write is what matters, not the mode. + try { + if (originalMode !== undefined) fs.chmodSync(tmpPath, originalMode); + } catch { /* non-fatal: proceed with default tmp-file mode */ } retryRenameSync(tmpPath, filePath); } catch (err) { // Reported, not thrown — the remaining agents still get processed. @@ -1016,9 +1268,183 @@ function cmdEffortSyncCodex(raw: boolean, dryRun: boolean, configDir?: string): } output( - { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir, refused, write_failures: writeFailures }, + { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir, refused, write_failures: writeFailures, read_failures: readFailures }, raw, - synced > 0 ? 'changed' : 'ok', + writeFailures.length > 0 || readFailures.length > 0 ? 'failed' : synced > 0 ? 'changed' : 'ok', + ); +} + +/** + * #3706 — the OpenCode branch of `cmdEffortSync`. Maintains the `variant:` + * frontmatter key install now bakes into every `~/.config/opencode/agents/ + * gsd-*.md` (or configDir-relative equivalent), mirroring exactly what + * install writes: a resolved universal effort clamped through + * `clampEffortForHost('opencode', ...)`. Null means the key must be ABSENT — + * #3533 (10d): an absent key is the correct state under `inherit`, and a + * level OpenCode does not accept must never be written, so both collapse to + * the same `target: null` and the same removal path. Result shape is + * additive over the claude branch, matching the CODEX branch's + * `{synced, skipped, changes, dry_run, agents_dir, write_failures}`. + */ +function cmdEffortSyncOpencode(cwd: string, raw: boolean, dryRun: boolean, configDir?: string): void { + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { getGlobalConfigDir } = require('./runtime-homes.cjs') as { getGlobalConfigDir(runtime: string, explicitDir?: string | null): string }; + const agentsDir = path.join(configDir || getGlobalConfigDir('opencode'), 'agents'); + + if (!fs.existsSync(agentsDir)) { + output({ synced: 0, skipped: 0, changes: [], dry_run: dryRun, agents_dir: agentsDir, reason: 'agents directory not found' }, raw, ''); + return; + } + + // Skip symlinks — matches the claude branch's existing guard (only write + // regular files, never follow a symlink into clobbering its target). + const files = fs + .readdirSync(agentsDir) + .filter(f => { + if (!f.startsWith('gsd-') || !f.endsWith('.md')) return false; + try { return fs.lstatSync(path.join(agentsDir, f)).isFile(); } catch { return false; } + }) + .sort(); + + // Use install-time resolvers: they merge ~/.gsd/defaults.json with project + // config, matching the exact logic used when agents were originally + // installed. Resolved once, outside the loop, like the claude branch. + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { readGsdEffectiveEffortConfig, resolveInstallTimeEffort } = require('./install-effort-resolver.cjs') as { + readGsdEffectiveEffortConfig(cwd: string): Record | null; + resolveInstallTimeEffort(cfg: Record | null, agentName: string): string; + }; + const effortCfg = readGsdEffectiveEffortConfig(cwd); + + // eslint-disable-next-line @typescript-eslint/no-require-imports, @typescript-eslint/unbound-method + const { clampEffortForHost } = require('./model-catalog.cjs') as { clampEffortForHost(host: string, effort: string): string | null }; + + const changes: EffortSyncChange[] = []; + const writeFailures: EffortSyncFileFailure[] = []; + const readFailures: EffortSyncFileFailure[] = []; + let synced = 0; + let skipped = 0; + + for (const file of files) { + const agentName = file.replace(/\.md$/, ''); + const filePath = path.join(agentsDir, file); + let content: string; + try { + content = fs.readFileSync(filePath, 'utf8'); + } catch (err) { + // An unreadable agent file must not abort the whole sweep — degrade + // like the write path below does, and report it under its own + // `read_failures` key so a caller can tell "never read" apart from + // "read but write failed". + skipped++; + readFailures.push({ agent: agentName, file: filePath, error: err instanceof Error ? err.message : String(err) }); + continue; + } + + // `target === null` covers both "no effort configured" (inherit) and "a + // level OpenCode does not accept" — both must produce NO `variant:` key, + // exactly what install writes. + const universal = effortCfg ? resolveInstallTimeEffort(effortCfg, agentName) : null; + const target = universal ? clampEffortForHost('opencode', universal) : null; + + const fmMatch = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(content); + if (!fmMatch) { skipped++; continue; } + + // Presence and value are distinct questions: `variant:` with an EMPTY + // value is a key that IS present but whose captured value is null (the + // `(.+?)` group requires at least one char). Deciding "already correct" + // from a null-vs-null comparison alone is wrong when target is also + // null — it would leave an unresolvable `variant: null` key on disk + // forever. Test presence with its own regex, and only compare values + // once presence is known. + const variantPresent = /^variant:/m.test(fmMatch[1]); + const variantMatch = /^variant:[ \t]*(.+?)[ \t]*$/m.exec(fmMatch[1]); + // `null` (key absent) and `''` (key present, value empty) are distinct + // states this code deliberately tracks via `variantPresent` above — a + // reported `from` that collapses both to `null` would make "no key" and + // "empty key" indistinguishable in the sync output, even though only one + // of them actually has a `variant:` line to remove. + const currentVariant = variantPresent ? (variantMatch ? variantMatch[1] : '') : null; + + if (target === null) { + if (!variantPresent) { skipped++; continue; } + } else if (variantPresent && currentVariant === target) { + skipped++; + continue; + } + + changes.push({ agent: agentName, from: currentVariant, to: target }); + synced++; + + if (!dryRun) { + // Atomic publish AND mode preservation, same discipline as + // cmdEffortSyncCodex above: write to a sibling tmp file, chmod it to + // match filePath's existing (masked) mode, then retryRenameSync it over + // the target so filePath is either the old bytes or the new ones, never + // half-written and never dropped to a default mode. On failure the + // write is reported, not thrown, so the remaining agents still get + // processed. + const tmpPath = `${filePath}.tmp.${process.pid}`; + // Stat filePath BEFORE the write so its mode can be passed at CREATION + // time — a plain `writeFileSync(tmpPath, data)` creates the tmp file at + // the default `0666 & ~umask` (world-readable under a typical 022 + // umask) even when filePath is e.g. 0600, exposing its contents for + // the window between creation and the chmod below. Mask off the + // file-type bits (e.g. S_IFREG 0o100000) that fs.statSync().mode + // carries alongside the permission bits — POSIX leaves chmod's + // handling of those bits unspecified, and the remote matrix runs Linux + // only (Darwin tolerating the full mode is not evidence it is safe + // there). Best-effort only: a stat failure must not abort the sync, + // since the content write is what matters, not the mode. + let originalMode: number | undefined; + try { + originalMode = fs.statSync(filePath).mode & 0o7777; + } catch { /* non-fatal: fall back to writing without an explicit mode */ } + try { + fs.writeFileSync( + tmpPath, + target === null ? removeFrontmatterKeyLine(content, 'variant') : setFrontmatterKeyLine(content, 'variant', target), + originalMode !== undefined ? { mode: originalMode } : undefined, + ); + // Not redundant with the `mode` option above: `mode` only applies + // when the file is actually created (O_CREAT). A leftover tmp file + // from an earlier crashed run would be reused (truncated) at its OLD + // mode instead, and this chmod is what corrects that case. + // Best-effort only: a chmod failure must not abort the sync, since + // the content write is what matters, not the mode. + try { + if (originalMode !== undefined) fs.chmodSync(tmpPath, originalMode); + } catch { /* non-fatal: proceed with default tmp-file mode */ } + retryRenameSync(tmpPath, filePath); + } catch (err) { + try { fs.unlinkSync(tmpPath); } catch { /* already gone or never created */ } + changes.pop(); + synced--; + skipped++; + writeFailures.push({ agent: agentName, file: filePath, error: err instanceof Error ? err.message : String(err) }); + continue; + } + } + } + + // Any failure — a write OR a read — must not report 'ok' or 'changed': + // either would hide that at least one agent's on-disk state is now unknown + // (unread) or unchanged despite being reported as a pending change (write + // failed after being pushed onto `changes`/`synced`). `write_failures` and + // `read_failures` take priority over the synced-count-derived summary below, + // even when other agents in the same run succeeded. + // + // Known limitation, deliberately not fixed here: `output()` only honors its + // third argument when `raw === true`, and this command's process always + // exits 0 regardless of the summary string — so `if gsd-tools effort sync; + // then` reads success in a shell even on a run where every write failed. + // Making the exit code reflect failure would be a CLI-contract change + // affecting all three cmdEffortSync* branches (claude, codex, opencode) and + // is out of scope for this fix. + output( + { synced, skipped, changes, dry_run: dryRun, agents_dir: agentsDir, write_failures: writeFailures, read_failures: readFailures }, + raw, + writeFailures.length > 0 || readFailures.length > 0 ? 'failed' : synced > 0 ? 'changed' : 'ok', ); } diff --git a/src/frontmatter.cts b/src/frontmatter.cts index 95f0696aa..6ae7785b2 100644 --- a/src/frontmatter.cts +++ b/src/frontmatter.cts @@ -378,6 +378,54 @@ function scalarNeedsDoubleQuoting(s: string): boolean { return false; } +/** + * #3706 — Does this value need double-quoting when written as an AGENT + * frontmatter scalar (`model:`, `variant:`)? + * + * Deliberately a superset of `scalarNeedsDoubleQuoting` rather than a second, + * competing predicate: that one answers "can this open a plain scalar safely", + * which is necessary but not sufficient for a value that must ROUND-TRIP as the + * exact string it went in as. Keeping both here is the point — they are two + * answers to one question and drift the moment they live apart. + * + * The extra clauses, each an observed mis-parse rather than a precaution: + * + * - a non-alphanumeric first character. `scalarNeedsDoubleQuoting` rejects the + * indicators that cannot OPEN a scalar, but YAML still resolves plenty of + * values that open legally: `~` and `.inf`/`.nan` become null and floats, + * and a leading sign or dot (`+1`, `-0`, `.5`) becomes a number. Requiring + * alphanumeric-first covers that whole family at once, and costs nothing: + * every real model ID and effort level starts alphanumeric. + * - a trailing `:` — `model: foo:` is read as a nested mapping key and fails + * the whole frontmatter with "bad indentation of a mapping entry". + * - a boolean/null word — YAML 1.1 readers resolve `no`/`y`/`off`/`null` to + * non-strings, so a variant named `no` arrives as `false`. + * - a numeric-looking value, including the YAML 1.1 sexagesimal form: `12:30` + * resolves to the integer 750, and `:` is legal mid-identifier here. + * - a date. `2026-08-25` starts alphanumeric and survives every clause above, + * yet YAML resolves it to a Date object rather than a string. + */ +const YAML_WORD_SCALAR_RE = /^(?:y|n|yes|no|true|false|on|off|null)$/i; +const YAML_NUMERIC_RE = /^(?:\d[\d_]*(?:\.[\d_]*)?(?:[eE][-+]?\d+)?|0[xXbBoO][0-9a-fA-F_]+|\d[\d_]*(?::[0-5]?\d)+(?:\.[\d_]*)?)$/; +// YAML 1.1 timestamp: a bare ymd, optionally followed by a time part. +const YAML_TIMESTAMP_RE = /^\d{4}-\d{1,2}-\d{1,2}(?:[Tt ].*)?$/; + +function agentScalarNeedsDoubleQuoting(s: string): boolean { + if (scalarNeedsDoubleQuoting(s)) return true; + if (!/^[A-Za-z0-9]/.test(s)) return true; + // A plain scalar ENDS at `: ` or ` #` wherever they appear — the base + // predicate only inspects the first character, because its question is + // whether the scalar can legally open. `a: b` makes the line a nested + // mapping (a parse error at this indent) and `a #b` silently truncates to + // `a`. Both were found by the round-trip property, not by inspection. + if (/:\s/.test(s) || /\s#/.test(s)) return true; + if (s.endsWith(':')) return true; + if (YAML_WORD_SCALAR_RE.test(s)) return true; + if (YAML_NUMERIC_RE.test(s)) return true; + if (YAML_TIMESTAMP_RE.test(s)) return true; + return false; +} + function reconstructFrontmatter(obj: Frontmatter): string { const lines: string[] = []; // #3257: read the full-line-comment channel (set by parseYamlRegion when comments @@ -938,6 +986,11 @@ function cmdFrontmatterValidate(cwd: string, filePath: string, schemaName: strin } export = { + // #3706: shared with the agent-frontmatter writers so a config-supplied + // `model:`/`variant:` value cannot break out of its scalar. Previously private + // here while those writers interpolated raw — one escaper, three call sites. + escapeDoubleQuoted, + agentScalarNeedsDoubleQuoting, extractFrontmatter, UNTERMINATED_KEY_THRESHOLD, // Additive alias (#644 prohibition-probe schema contract): the probe round-trip seam reads a diff --git a/src/model-catalog.cts b/src/model-catalog.cts index a6c598002..de56c8e2b 100644 --- a/src/model-catalog.cts +++ b/src/model-catalog.cts @@ -351,6 +351,33 @@ export interface RenderedEffortArgv { host: string; } +/** + * Clamp a universal effort level to what a host actually accepts, or null. + * + * #3706 — extracted from `renderEffortArgv` so the two effort CHANNELS can share + * one capability table without one pretending to be the other. `EFFORT_ARGV[host]` + * describes what levels the host understands (`supported` + `clamp`); whether that + * reaches the host as a CLI flag or as a baked frontmatter key is the caller's + * business. The install-time OpenCode `variant:` writer needs the former without + * the latter, and hardcoding `'argv'` at that call site to borrow this logic read + * as if the frontmatter key were gated on the invocation-time axis. It is not — + * claude declares `effortSurface: "argv"` and independently bakes an `effort:` key. + * + * Returns null for a level the host does not accept, which every caller treats as + * "emit nothing". `inherit` lands here too: per #3533 (10d) it is not a wire level + * on any runtime, so it is in no `supported` set and must never be written out. + */ +export function clampEffortForHost(host: string, universalEffort: string): string | null { + // Own-property lookup only: a plain `EFFORT_ARGV[host]` resolves `__proto__` + // and friends to inherited members that carry no clamp/render. + if (typeof host !== 'string' || !Object.prototype.hasOwnProperty.call(EFFORT_ARGV, host)) return null; + const spec = EFFORT_ARGV[host]; + if (!spec || typeof spec.clamp !== 'function') return null; + if (typeof universalEffort !== 'string' || universalEffort.length === 0) return null; + const clamped = spec.clamp(universalEffort); + return spec.supported.has(clamped) ? clamped : null; +} + /** * Render the invocation-time effort argument for a host. * @@ -369,12 +396,13 @@ export function renderEffortArgv( // (and `constructor`/`toString`) to inherited members, which are truthy but // carry no `clamp`/`render` — a hostile host id would throw instead of // degrading. The host id reaches here from a descriptor, i.e. untrusted JSON. - if (typeof host !== 'string' || !Object.prototype.hasOwnProperty.call(EFFORT_ARGV, host)) return empty; + // #3706: the clamp/supported half lives in clampEffortForHost so the + // install-time channel can reuse it; this function keeps the axis gate and + // the argv rendering. One table, one clamp, two channels. + const clamped = clampEffortForHost(host, universalEffort); + if (clamped === null) return empty; const spec = EFFORT_ARGV[host]; - if (!spec || typeof spec.clamp !== 'function' || typeof spec.render !== 'function') return empty; - if (typeof universalEffort !== 'string' || universalEffort.length === 0) return empty; - const clamped = spec.clamp(universalEffort); - if (!spec.supported.has(clamped)) return empty; + if (typeof spec.render !== 'function') return empty; return { argv: spec.render(clamped), value: clamped, host }; } diff --git a/src/runtime-artifact-conversion.cts b/src/runtime-artifact-conversion.cts index f3e412583..32f8d1115 100644 --- a/src/runtime-artifact-conversion.cts +++ b/src/runtime-artifact-conversion.cts @@ -35,6 +35,7 @@ const { getDirName } = runtimeNamePolicy; import capabilityRegistry = require('./capability-registry.cjs'); import hostIntegration = require('./host-integration.cjs'); import { posixNormalize } from './shell-command-projection.cjs'; +import frontmatterModule = require('./frontmatter.cjs'); import { escapeRegex as escapeRegExp } from './pattern.cjs'; import { scanFencedBlocks } from './markdown-sectionizer.cjs'; // #2870: install-scope.cts is a leaf-tier sibling (imports only @@ -1828,7 +1829,27 @@ function neutralizeAgentReferences(content, instructionFile) { return c; } -function convertClaudeToOpencodeFrontmatter(content, { isAgent = false, modelOverride = null } = {}) { +/** + * Render one frontmatter `key: value` line whose value came from user config. + * + * #3706: both `model:` and `variant:` interpolate a value read from + * `.planning/config.json` / `~/.gsd/defaults.json`. Raw interpolation lets a value + * containing a newline inject additional TOP-LEVEL frontmatter keys into the + * generated agent file — proven by execution during the #3705 security review + * (`"sonnet\ntools: [\"*\"]\npermission: bypass"` produced two extra keys). + * + * That sink predates #3706, but this change adds a SECOND write to it, so it is + * closed here rather than doubled. Both the decision and the escaping live in + * frontmatter.cts so there is exactly one set of YAML scalar rules; a value that + * round-trips bare is still emitted bare, so generated files do not churn. + */ +function frontmatterScalar(key: string, value: string): string { + return frontmatterModule.agentScalarNeedsDoubleQuoting(value) + ? `${key} "${frontmatterModule.escapeDoubleQuoted(value)}"` + : `${key} ${value}`; +} + +function convertClaudeToOpencodeFrontmatter(content, { isAgent = false, modelOverride = null, variant = null } = {}) { // Replace tool name references in content (applies to all files) let convertedContent = content; convertedContent = convertedContent.replace(/\bAskUserQuestion\b/g, 'question'); @@ -1967,7 +1988,23 @@ function convertClaudeToOpencodeFrontmatter(content, { isAgent = false, modelOve // respected on OpenCode (which uses static agent frontmatter, not inline // Task() model parameters). See #2256. if (modelOverride) { - newLines.push(['model:', modelOverride].join(' ')); + newLines.push(frontmatterScalar('model:', modelOverride)); + } + // #3706: deliver the RESOLVED reasoning effort to the spawned subagent. + // `query resolve-model` reported an effort that never reached OpenCode, so + // every subagent ran at whatever opencode.jsonc defaults the model to — for + // a custom provider commonly the most expensive setting. + // + // OpenCode ONLY: `EFFORT_ARGV` declares surfaces for claude/opencode/codex + // and NO kilo entry, so the Kilo converter below deliberately does not emit + // this. Unlike the model side — where #2794 J8 requires kilo and opencode to + // resolve identically — there is no kilo effort surface to render into, and + // inventing one would emit a key that runtime never documented. + // + // Omitted entirely when absent, per #1156's rule for `model: inherit`: + // never an empty or sentinel value, let the runtime use its own default. + if (variant) { + newLines.push(frontmatterScalar('variant:', variant)); } } @@ -2155,7 +2192,7 @@ function convertClaudeToKiloFrontmatter(content, { isAgent = false, modelOverrid // model emission exactly (#2093 UPGRADE 2 / ADR-1239; Kilo is an OpenCode // fork with the same static-frontmatter model constraint). See #2256. if (modelOverride) { - newLines.push(['model:', modelOverride].join(' ')); + newLines.push(frontmatterScalar('model:', modelOverride)); } newLines.push(...buildKiloAgentPermissionBlock(agentTools)); } diff --git a/src/runtime-artifact-layout.cts b/src/runtime-artifact-layout.cts index c033463cf..99f5a353f 100644 --- a/src/runtime-artifact-layout.cts +++ b/src/runtime-artifact-layout.cts @@ -42,6 +42,10 @@ const conversionExports = runtimeArtifactConversion as Record & // rather than re-deriving the chain per runtime. // eslint-disable-next-line @typescript-eslint/no-require-imports import installModelOverrideResolver = require('./install-model-override-resolver.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- export= CommonJS module, same as the sibling resolver import above +import installEffortResolver = require('./install-effort-resolver.cjs'); +// eslint-disable-next-line @typescript-eslint/no-require-imports -- export= CommonJS module, same as the sibling resolver import above +import modelCatalog = require('./model-catalog.cjs'); import { posixNormalize } from './shell-command-projection.cjs'; // #2870: `isGlobalScope` centralizes the `scope === 'global'` boolean // projection both kind-builder closures below need at the converters' @@ -432,7 +436,7 @@ function convertedAgentsKind( // boolean `stageAgentsForRuntimeWithConverter`'s positional API // requires — see its doc comment in install-scope.cts. const rawConverter = _resolveNamedConverter(converterName, 'agents') as - (content: string, arg2?: boolean | { isAgent?: boolean; modelOverride?: string | null }) => string; + (content: string, arg2?: boolean | { isAgent?: boolean; modelOverride?: string | null; variant?: string | null }) => string; // #2875 Part 2 (J5-J8): kilo/opencode agent converters take an options // bag (`{isAgent, modelOverride}`), not the `isGlobal` boolean every @@ -449,11 +453,40 @@ function convertedAgentsKind( const overrideTargetDir = agentCtx?.targetDir ?? configDir; const modelOverrides = installModelOverrideResolver.readGsdEffectiveModelOverrides(overrideTargetDir); const runtimeResolver = installModelOverrideResolver.readGsdRuntimeProfileResolver(overrideTargetDir); + // #3706: the resolved reasoning effort, threaded exactly as the model is — + // config read ONCE per stage(), per-agent value resolved per file. + // + // Gated on the effort config being PRESENT, not on a value coming back. + // `resolveInstallTimeEffort` always returns something (measured: 'high' + // even with no project config and no effort block), so "skip when + // resolution yields no value" has no trigger and would stamp `variant:` + // into every generated agent file for every existing install. OpenCode's + // built-in variant sets are provider-specific upstream (Anthropic ships + // only `high`/`max`), so a level GSD resolved is not guaranteed to name a + // variant the user's provider actually has — emitting one unasked-for is + // the risk this gate avoids. #1156's rule for `model: inherit` is the + // precedent: do not emit a key the runtime may not understand. + // + // OpenCode only; the kilo converter ignores the field (no EFFORT_ARGV.kilo). + const effortConfig = converterName === 'convertClaudeToOpencodeFrontmatter' + ? installEffortResolver.readGsdEffectiveEffortConfig(overrideTargetDir) + : null; converter = (content, _isGlobal, meta) => { const modelOverride = meta ? installModelOverrideResolver.resolveAgentModelOverride(meta.agentName, modelOverrides, runtimeResolver) : null; - return rawConverter(content, { isAgent: true, modelOverride }); + // The universal level is NOT emitted raw. `clampEffortForHost` is the + // declared OpenCode effort capability (EFFORT_ARGV.opencode: its own + // `supported` set + `clamp`), and it is what rejects a level that is + // not a wire value — most importantly `inherit`, which per #3533 (10d) + // means "omit the key and follow the host default" and must never be + // written literally. Anything unsupported clamps to null, which omits + // the key rather than inventing one. + const universal = effortConfig && meta + ? installEffortResolver.resolveInstallTimeEffort(effortConfig, meta.agentName) + : null; + const variant = universal ? modelCatalog.clampEffortForHost('opencode', universal) : null; + return rawConverter(content, { isAgent: true, modelOverride, variant }); }; } else { // isGlobal is threaded so scope-aware agent converters (copilot, antigravity) diff --git a/tests/effort-sync-installed-runtime.test.cjs b/tests/effort-sync-installed-runtime.test.cjs index 3bf710c00..88dc88795 100644 --- a/tests/effort-sync-installed-runtime.test.cjs +++ b/tests/effort-sync-installed-runtime.test.cjs @@ -21,11 +21,18 @@ const { test, describe } = require('node:test'); const assert = require('node:assert/strict'); const fs = require('node:fs'); const path = require('node:path'); +const os = require('node:os'); const { runNode } = require('./helpers/process-seam.cjs'); const { runMinimalInstall } = require('./helpers/install-shared.cjs'); const { cleanup } = require('./helpers.cjs'); +// Absolute path to the built module, spawned in a child process below so the +// JSON `cmdEffortSync` writes straight to fd 1 (via io.cjs's writeAllSync) can +// actually be captured — overriding process.stdout.write in-process does not +// intercept that write. +const COMMANDS_CJS = path.join(__dirname, '..', 'gsd-core', 'bin', 'lib', 'commands.cjs'); + describe('#2071: effort sync runs in an installed runtime (no package-root bin/install.js)', () => { test('effort sync does not crash reaching for the un-shipped bin/install.js', () => { if (process.platform === 'win32') return; // install layout is POSIX-path-shaped @@ -65,3 +72,1182 @@ describe('#2071: effort sync runs in an installed runtime (no package-root bin/i } }); }); + +// #3706 — OpenCode's `effort sync` path (cmdEffortSyncOpencode) maintains the +// `variant:` frontmatter key install bakes into `~/.config/opencode/agents/ +// gsd-*.md`, mirroring the pre-existing claude/`effort:` and codex branches. +// Each case is run against a fresh sandbox: a project `cwd` (holding +// `.planning/config.json`), a `configDir` (holding `agents/`), and an +// isolated `home` (HOME env, so `~/.gsd/defaults.json` can never leak in from +// the real developer machine) — cmdEffortSync merges home defaults with the +// project config, so a hermetic test must control both sources. +describe('#3706: effort sync maintains OpenCode variant: frontmatter', () => { + function makeSandbox() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-effort-sync-opencode-')); + const cwd = path.join(root, 'project'); + const configDir = path.join(root, 'runtime-home'); + const agentsDir = path.join(configDir, 'agents'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.mkdirSync(home, { recursive: true }); + return { root, cwd, configDir, agentsDir, home }; + } + + function writeProjectEffortConfig(cwd, effort) { + fs.mkdirSync(path.join(cwd, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(cwd, '.planning', 'config.json'), + JSON.stringify(effort === undefined ? {} : { effort }), + ); + } + + function writeAgent(agentsDir, name, lines) { + const filePath = path.join(agentsDir, name); + fs.writeFileSync(filePath, lines.join('\n')); + return filePath; + } + + /** + * Invokes `cmdEffortSync(cwd, false, opts)` in a CHILD process (required + * per module — see COMMANDS_CJS comment above) and parses the JSON result + * `output()` writes to fd 1. Callers that only need the on-disk effect of + * the sync (most cases below) still go through this so the harness stays + * single-shaped, but only cases 2 and 6 actually assert on the returned + * `result`. + */ + function runEffortSync({ cwd, home, configDir, runtime, dryRun }) { + const opts = { dryRun, configDir, runtime }; + const script = [ + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + return JSON.parse(spawned.stdout.slice(jsonStart)); + } + + test('writes the resolved variant into an agent that has none', () => { + // Protects: cmdEffortSyncOpencode's happy-path write — a fresh agent + // with no `variant:` key gets the resolved effort injected verbatim. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^variant: xhigh$/m); + } finally { + cleanup(root); + } + }); + + test('a synced agent keeps its original file mode', () => { + // The tmp-file + rename publish does not preserve mode the way an + // in-place writeFileSync would — cmdEffortSyncOpencode stat+chmods the + // tmp file before the rename specifically to compensate. Mode bits are + // not meaningful on Windows (no POSIX permission model), so skip there. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + fs.chmodSync(filePath, 0o600); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^variant: xhigh$/m, 'the sync must have actually rewritten the file'); + assert.strictEqual( + fs.statSync(filePath).mode & 0o777, 0o600, + 'the published file must keep the original file mode', + ); + } finally { + cleanup(root); + } + }); + + test('the temp file is never more permissive than the agent it replaces', () => { + // A plain `writeFileSync(tmpPath, data)` creates the tmp file at the + // default `0666 & ~umask`, then only tightens it afterward via chmod — + // for a 0600 agent that briefly leaves its contents world-readable + // inside agents/. Assert the mode is correct at CREATION, not just + // after the later chmod, by monkeypatching fs.writeFileSync in the + // child process to record the on-disk mode immediately after the real + // write runs. Mode bits are not meaningful on Windows, so skip there. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + fs.chmodSync(filePath, 0o600); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + "const fs = require('node:fs');", + 'const captures = [];', + 'const originalWriteFileSync = fs.writeFileSync;', + 'fs.writeFileSync = function (targetPath, data, options) {', + ' const result = originalWriteFileSync.call(fs, targetPath, data, options);', + ' if (typeof targetPath === "string" && /\\.tmp\\.\\d+$/.test(targetPath)) {', + ' captures.push({ options: options || null, modeAfterWrite: fs.statSync(targetPath).mode & 0o777 });', + ' }', + ' return result;', + '};', + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + 'process.stdout.write("###CAPTURE_START###" + JSON.stringify(captures) + "###CAPTURE_END###");', + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `capture child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const match = /###CAPTURE_START###([\s\S]*?)###CAPTURE_END###/.exec(spawned.stdout); + assert.ok(match, `expected capture marker on stdout, got:\n${spawned.stdout}`); + const captures = JSON.parse(match[1]); + assert.strictEqual(captures.length, 1, `expected exactly one tmp-file write, got:\n${JSON.stringify(captures)}`); + + // The tmp file's mode immediately after creation must already match + // the target agent's 0600, not the default 0644 a bare writeFileSync + // would produce. + assert.strictEqual( + captures[0].modeAfterWrite, 0o600, + 'the tmp file must never be created more permissive than the agent it replaces', + ); + + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^variant: xhigh$/m, 'the sync must have actually rewritten the file'); + assert.strictEqual(fs.statSync(filePath).mode & 0o777, 0o600, 'the published file must keep the original file mode'); + } finally { + cleanup(root); + } + }); + + test('reports the change without writing under dry run', () => { + // Protects: dry-run reports the pending change but leaves the file + // byte-identical — no write happens until dryRun is explicitly false. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const before = fs.readFileSync(filePath); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: true }); + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'dry run must not touch the file on disk'); + assert.strictEqual(result.synced, 1); + assert.deepStrictEqual(result.changes[0], { agent: 'gsd-executor', from: null, to: 'xhigh' }); + } finally { + cleanup(root); + } + }); + + test('an already-correct agent is skipped, not rewritten', () => { + // Protects: an agent already carrying the resolved variant is reported + // skipped and its bytes are left completely untouched (no rewrite churn). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'high' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: high', '---', '', 'Body.', + ]); + const before = fs.readFileSync(filePath); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'an in-sync agent must not be rewritten'); + assert.strictEqual(result.skipped, 1); + assert.strictEqual(result.synced, 0); + } finally { + cleanup(root); + } + }); + + test('inherit strips the key rather than writing it literally', () => { + // #3533 (10d) — inherit means the key must not exist; writing + // `variant: inherit` would name a variant that cannot resolve. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'inherit' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: high', '---', '', 'Body.', + ]); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + assert.match(content, /^name: gsd-executor$/m); + assert.match(content, /^description: x$/m); + assert.match(content, /^mode: subagent$/m); + assert.deepStrictEqual(result.changes[0], { agent: 'gsd-executor', from: 'high', to: null }); + } finally { + cleanup(root); + } + }); + + test('no effort config at all strips a stale key', () => { + // Matches what install bakes with no config: an agent that carries a + // stale `variant:` key from a prior sync must lose it, not keep it. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, undefined); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: high', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('a missing agents directory is reported, not thrown', () => { + // Protects: an install with no agents/ subdir under configDir must + // report the condition structurally, never throw out of cmdEffortSync. + const { root, cwd, configDir, home } = makeSandbox(); + try { + cleanup(path.join(configDir, 'agents')); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + assert.strictEqual(result.reason, 'agents directory not found'); + } finally { + cleanup(root); + } + }); + + test('files that are not gsd-*.md are ignored', () => { + // Protects: the gsd-*.md filter — a non-gsd file sitting in agents/ must + // never be synced or skipped, i.e. never even enter the loop. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + writeAgent(agentsDir, 'not-gsd.md', [ + '---', 'name: not-gsd', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const result = runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + assert.strictEqual(result.synced, 0); + assert.strictEqual(result.skipped, 0); + } finally { + cleanup(root); + } + }); + + test('the document body is untouched', () => { + // Protects: the frontmatter line-editors only ever touch the frontmatter + // span — a body containing a colon, a `#`, and a `---` rule must survive + // byte-identical past the closing frontmatter delimiter. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'max' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', '---', '', + 'Body with: colons #hash and a --- rule.', '', 'more.', + ]); + const before = fs.readFileSync(filePath, 'utf8'); + const beforeBody = before.slice(before.indexOf('---', 3) + 3); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(filePath, 'utf8'); + const afterBody = after.slice(after.indexOf('---', 3) + 3); + assert.strictEqual(afterBody, beforeBody); + assert.match(after, /^variant: max$/m); + } finally { + cleanup(root); + } + }); + + test('a key present with an empty value is removed, not mistaken for absent', () => { + // The value regex's `(.+?)` requires at least one character, so a bare + // `variant:` line (zero-width value) used to read as "key absent" rather + // than "key present, value empty" — presence and value are now distinct + // questions, and a stale empty-valued key must still be stripped when no + // effort config resolves one. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, undefined); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant:', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('a key present with a whitespace-only value is removed too (the spelling that always worked)', () => { + // Trailing whitespace after the colon always matched the old regex (it is + // not zero-width), so this spelling never exhibited the absent-vs-empty + // bug above — pinned as the control that makes the bug look like a + // whitespace lottery rather than a real presence/value distinction. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, undefined); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'variant: ', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('CRLF agents keep their line endings through a write and a strip', () => { + // Key-parameterising the frontmatter editors (claude's effort: vs + // opencode's variant:) is exactly the kind of change that would regress + // CRLF handling if the line ending were baked in per-key rather than + // detected from the matched frontmatter block. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const beforeWrite = [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ].join('\r\n'); + fs.writeFileSync(filePath, beforeWrite); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const afterWrite = fs.readFileSync(filePath, 'utf8'); + assert.match(afterWrite, /^variant: xhigh\r$/m); + assert.doesNotMatch(afterWrite, /(? { + // A symlinked gsd-*.md must never have its TARGET file rewritten — the + // sync must skip it structurally (readdir's Dirent reports a symlink + // entry as not-a-file), not follow it and edit whatever it points to. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + const targetPath = path.join(root, 'outside-target.md'); + const targetContent = [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ].join('\n'); + fs.writeFileSync(targetPath, targetContent); + const linkPath = path.join(agentsDir, 'gsd-executor.md'); + try { + fs.symlinkSync(targetPath, linkPath); + } catch { + return; // platform cannot create symlinks (e.g. unprivileged Windows) — skip + } + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(targetPath, 'utf8'); + assert.strictEqual(after, targetContent, 'the symlink target must be left untouched'); + } finally { + cleanup(root); + } + }); + + test('a body line starting with variant: is never the line edited', () => { + // Scoping-by-content, not a whole-file /m replace: a body line that + // happens to start with `variant:` must survive untouched while the + // frontmatter gains its own key. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', + 'variant: in-the-body', '', 'more.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.ok(content.includes('variant: in-the-body'), 'the body line must survive untouched'); + const fmEnd = content.indexOf('\n---', 4); + assert.match(content.slice(0, fmEnd), /^variant: xhigh$/m, 'the frontmatter must gain its own variant: line'); + } finally { + cleanup(root); + } + }); + + test('a file with no frontmatter at all is skipped, not corrupted', () => { + // No `---` fences at all: the sync must leave the file byte-identical + // rather than guessing where a frontmatter block would go. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + 'Just a body, no frontmatter fences at all.', + ]); + const before = fs.readFileSync(filePath); + runEffortSync({ cwd, home, configDir, runtime: 'opencode', dryRun: false }); + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'a file with no frontmatter must be byte-identical afterward'); + } finally { + cleanup(root); + } + }); + + test('the claude path still writes effort:, not variant:', () => { + // The frontmatter line-editors were key-parameterised for #3706; this is + // the control that the pre-existing claude behavior did not move. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'claude', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^effort: xhigh$/m); + assert.doesNotMatch(content, /^variant:/m); + } finally { + cleanup(root); + } + }); + + test('a claude read failure reports failed without changing the result shape', () => { + // Protects: the claude branch of cmdEffortSync deliberately does NOT gain + // a `read_failures` key on a read failure — that result shape + // (`{synced, skipped, changes, dry_run, agents_dir}`) is long-standing + // and widely consumed. The failure is surfaced only through output()'s + // third argument (the raw-mode summary token), which is never merged + // into the JSON object. Asserts both halves: the raw token is 'failed', + // and the JSON result's key set is exactly the historic five, with no + // read_failures/write_failures key added. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-unreadable': 'xhigh' } }); + writeAgent(agentsDir, 'gsd-unreadable.md', [ + '---', 'name: gsd-unreadable', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const makeScript = raw => [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.md')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, ${raw}, ${JSON.stringify(opts)});`, + ].join('\n'); + + const jsonRun = runNode(['-e', makeScript(false)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + jsonRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${jsonRun.outcome}):\nstdout: ${jsonRun.stdout}\nstderr: ${jsonRun.stderr}`, + ); + const jsonStart = jsonRun.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${jsonRun.stdout}`); + const result = JSON.parse(jsonRun.stdout.slice(jsonStart)); + assert.strictEqual(result.skipped, 1); + assert.deepStrictEqual( + Object.keys(result).sort(), + ['agents_dir', 'changes', 'dry_run', 'skipped', 'synced'], + 'the claude result shape must gain no read_failures/write_failures key', + ); + + const rawRun = runNode(['-e', makeScript(true)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + rawRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${rawRun.outcome}):\nstdout: ${rawRun.stdout}\nstderr: ${rawRun.stderr}`, + ); + assert.strictEqual(rawRun.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${rawRun.stdout}`); + } finally { + cleanup(root); + } + }); + + test('a claude write failure is reported and does not abort the sweep', () => { + // Protects: the claude branch's fs.writeFileSync (both the inherit/strip + // call site and the concrete/set call site) used to sit outside any try, + // so a single unwritable agent file threw and aborted the entire sweep. + // Injected deterministically by monkeypatching fs.writeFileSync inside + // the child script for exactly ONE agent's path — per this repo's + // CLAUDE.md §4, chmod-based injection is forbidden (root bypasses mode + // bits under Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-broken': 'xhigh', 'gsd-executor': 'xhigh' } }); + const brokenPath = writeAgent(agentsDir, 'gsd-broken.md', [ + '---', 'name: gsd-broken', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const okPath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const brokenBefore = fs.readFileSync(brokenPath); + + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const makeScript = raw => [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-broken.md')) {`, + ` throw new Error('injected write failure');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, ${raw}, ${JSON.stringify(opts)});`, + ].join('\n'); + + const jsonRun = runNode(['-e', makeScript(false)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + jsonRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${jsonRun.outcome}):\nstdout: ${jsonRun.stdout}\nstderr: ${jsonRun.stderr}`, + ); + const jsonStart = jsonRun.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${jsonRun.stdout}`); + const result = JSON.parse(jsonRun.stdout.slice(jsonStart)); + assert.strictEqual(result.skipped, 1); + assert.deepStrictEqual( + Object.keys(result).sort(), + ['agents_dir', 'changes', 'dry_run', 'skipped', 'synced'], + 'the claude result shape must gain no read_failures/write_failures key', + ); + + const brokenAfter = fs.readFileSync(brokenPath); + assert.ok(brokenBefore.equals(brokenAfter), 'the failing agent file must be byte-unchanged'); + + const okContent = fs.readFileSync(okPath, 'utf8'); + assert.match(okContent, /^effort: xhigh$/m, 'the sweep must continue past the failing agent'); + + const rawRun = runNode(['-e', makeScript(true)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + rawRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${rawRun.outcome}):\nstdout: ${rawRun.stdout}\nstderr: ${rawRun.stderr}`, + ); + assert.strictEqual(rawRun.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${rawRun.stdout}`); + } finally { + cleanup(root); + } + }); + + test('a claude agent survives a fault mid-publish', () => { + // Protects: the claude branch now publishes atomically (tmp-write + + // chmod + retryRenameSync), same discipline as cmdEffortSyncOpencode. An + // ENOSPC-mid-write injected on the TMP path must never truncate or empty + // the real agent file — pre-fix, an in-place `writeFileSync(filePath, + // ...)` truncates via O_TRUNC before the fault, leaving the file empty. + // Injected deterministically by monkeypatching fs.writeFileSync inside + // the child script for exactly the `.tmp.` path — per this repo's + // CLAUDE.md §4, chmod-based injection is forbidden (root bypasses mode + // bits under Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const before = fs.readFileSync(filePath); + + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const makeScript = raw => [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + // cmdEffortSync's claude branch writes to `.tmp.` + // before renaming over the real path — throw only for that tmp + // write, simulating an ENOSPC fault after the original file has + // already been truncated by a naive in-place write (which this + // atomic-publish path no longer performs). + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-executor.md.tmp.')) {`, + ` throw new Error('injected ENOSPC mid-write');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, ${raw}, ${JSON.stringify(opts)});`, + ].join('\n'); + + const jsonRun = runNode(['-e', makeScript(false)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + jsonRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${jsonRun.outcome}):\nstdout: ${jsonRun.stdout}\nstderr: ${jsonRun.stderr}`, + ); + + const after = fs.readFileSync(filePath); + assert.ok(before.equals(after), 'the original agent file must be byte-identical after a fault mid-publish'); + + const leftoverTmp = fs.readdirSync(agentsDir).filter(f => f.includes('.tmp.')); + assert.deepStrictEqual(leftoverTmp, [], `no orphan tmp file must remain in agents_dir, found: ${leftoverTmp.join(', ')}`); + + const rawRun = runNode(['-e', makeScript(true)], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + rawRun.exitCode, 0, + `cmdEffortSync child process failed (outcome=${rawRun.outcome}):\nstdout: ${rawRun.stdout}\nstderr: ${rawRun.stderr}`, + ); + assert.strictEqual(rawRun.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${rawRun.stdout}`); + } finally { + cleanup(root); + } + }); + + test('a claude sync preserves the agent file mode', () => { + // The tmp-file + rename publish does not preserve mode the way an + // in-place writeFileSync would — cmdEffortSync's claude branch + // stat+chmods the tmp file before the rename specifically to compensate, + // mirroring cmdEffortSyncOpencode. Mode bits are not meaningful on + // Windows (no POSIX permission model), so skip there. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': 'xhigh' } }); + const filePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + fs.chmodSync(filePath, 0o600); + runEffortSync({ cwd, home, configDir, runtime: 'claude', dryRun: false }); + const content = fs.readFileSync(filePath, 'utf8'); + assert.match(content, /^effort: xhigh$/m, 'the sync must have actually rewritten the file'); + assert.strictEqual( + fs.statSync(filePath).mode & 0o777, 0o600, + 'the published file must keep the original file mode', + ); + } finally { + cleanup(root); + } + }); + + test('effort values are written unquoted', () => { + // Control for the shared-escaping fix: setFrontmatterKeyLine now routes + // through the same agentScalarNeedsDoubleQuoting/escapeDoubleQuoted + // helpers the install-side frontmatterScalar writer uses, but every + // effort level the sync actually writes today is a plain scalar — the + // output must stay byte-identical (unquoted) across runtimes/levels. + for (const level of ['low', 'xhigh']) { + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-executor': level } }); + const claudePath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd, home, configDir, runtime: 'claude', dryRun: false }); + const claudeContent = fs.readFileSync(claudePath, 'utf8'); + assert.match(claudeContent, new RegExp(`^effort: ${level}$`, 'm')); + assert.doesNotMatch(claudeContent, /^effort: "/m); + } finally { + cleanup(root); + } + + const { root: root2, cwd: cwd2, configDir: configDir2, agentsDir: agentsDir2, home: home2 } = makeSandbox(); + try { + writeProjectEffortConfig(cwd2, { agent_overrides: { 'gsd-executor': level } }); + const opencodePath = writeAgent(agentsDir2, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + runEffortSync({ cwd: cwd2, home: home2, configDir: configDir2, runtime: 'opencode', dryRun: false }); + const opencodeContent = fs.readFileSync(opencodePath, 'utf8'); + assert.match(opencodeContent, new RegExp(`^variant: ${level}$`, 'm')); + assert.doesNotMatch(opencodeContent, /^variant: "/m); + } finally { + cleanup(root2); + } + } + }); + + test('a write failure on one agent is reported and does not stop the sweep', () => { + // Protects: cmdEffortSyncOpencode's atomic tmp-write + rename can fail + // mid-sync (fs fault). The failure must be reported per-agent in + // write_failures, the remaining agents must still be processed, the + // failing agent's file must be byte-unchanged, and cmdEffortSync must + // never throw. Injected deterministically by monkeypatching + // fs.writeFileSync inside the child script — per this repo's CLAUDE.md + // §4, chmod-based injection is forbidden (root bypasses mode bits under + // Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-broken': 'xhigh', 'gsd-executor': 'xhigh' } }); + const brokenPath = writeAgent(agentsDir, 'gsd-broken.md', [ + '---', 'name: gsd-broken', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const okPath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const brokenBefore = fs.readFileSync(brokenPath); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + // The tmp-write + rename publish (cmdEffortSyncOpencode) writes to + // `.tmp.` before renaming over the real path — throw + // only for that one agent's tmp file, so every other write (and any + // fs machinery the require chain itself performs) passes through + // unmodified. + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-broken.md.tmp.')) {`, + ` throw new Error('injected write failure');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + const result = JSON.parse(spawned.stdout.slice(jsonStart)); + + assert.strictEqual(result.write_failures.length, 1); + assert.strictEqual(result.write_failures[0].agent, 'gsd-broken'); + + const brokenAfter = fs.readFileSync(brokenPath); + assert.ok(brokenBefore.equals(brokenAfter), 'the failing agent file must be byte-unchanged'); + + const okContent = fs.readFileSync(okPath, 'utf8'); + assert.match(okContent, /^variant: xhigh$/m, 'the sweep must continue past the failing agent'); + } finally { + cleanup(root); + } + }); + + test('an unreadable agent file is reported and does not abort the sweep', () => { + // Protects: cmdEffortSyncOpencode's fs.readFileSync used to sit outside + // any try, so a single unreadable agent file threw and aborted the + // entire sweep, unlike the write path in the same loop which degrades + // into write_failures. Injected deterministically by monkeypatching + // fs.readFileSync inside the child script — per this repo's CLAUDE.md + // §4, chmod-based injection is forbidden (root bypasses mode bits under + // Docker/CI and it leaks resources). + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-unreadable': 'xhigh', 'gsd-executor': 'xhigh' } }); + writeAgent(agentsDir, 'gsd-unreadable.md', [ + '---', 'name: gsd-unreadable', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + const okPath = writeAgent(agentsDir, 'gsd-executor.md', [ + '---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.md')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + const result = JSON.parse(spawned.stdout.slice(jsonStart)); + + assert.strictEqual(result.read_failures.length, 1); + assert.strictEqual(result.read_failures[0].agent, 'gsd-unreadable'); + + const okContent = fs.readFileSync(okPath, 'utf8'); + assert.match(okContent, /^variant: xhigh$/m, 'the sweep must continue past the unreadable agent'); + } finally { + cleanup(root); + } + }); + + test('the summary reports failed when an opencode write could not complete', () => { + // Protects: the raw-mode summary TOKEN cmdEffortSync passes as output()'s + // third argument (never merged into the JSON object — io.cts `output()` + // only reads it when `raw === true`, entirely replacing the JSON + // payload) must flip to 'failed' when a write_failures entry exists, even + // though the JSON result itself carries no such flag. Captured by + // running the child with `raw: true` and asserting on the literal stdout + // text `output()` writes for that mode, not on parsed JSON. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, { agent_overrides: { 'gsd-broken': 'xhigh' } }); + writeAgent(agentsDir, 'gsd-broken.md', [ + '---', 'name: gsd-broken', 'description: x', 'mode: subagent', '---', '', 'Body.', + ]); + + const opts = { dryRun: false, configDir, runtime: 'opencode' }; + const script = [ + `const fs = require('node:fs');`, + `const originalWriteFileSync = fs.writeFileSync;`, + `fs.writeFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-broken.md.tmp.')) {`, + ` throw new Error('injected write failure');`, + ` }`, + ` return originalWriteFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, true, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + assert.strictEqual(spawned.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${spawned.stdout}`); + } finally { + cleanup(root); + } + }); +}); + +// ADR-2313 D7 (#3243) — the Codex branch (cmdEffortSyncCodex) strips a stale +// Anthropic-flavored `model` pin (and its coupled `model_reasoning_effort`) +// from every installed `~/.codex/agents/.toml`, publishing via the same +// tmp-file + chmod + retryRenameSync discipline as the OpenCode branch above. +describe('#3243: effort sync strips Anthropic-flavored model pins from Codex agents', () => { + function makeSandbox() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-effort-sync-codex-')); + const cwd = path.join(root, 'project'); + const configDir = path.join(root, 'runtime-home'); + const agentsDir = path.join(configDir, 'agents'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.mkdirSync(home, { recursive: true }); + return { root, cwd, configDir, agentsDir, home }; + } + + function writeAgent(agentsDir, name, lines) { + const filePath = path.join(agentsDir, name); + fs.writeFileSync(filePath, lines.join('\n')); + return filePath; + } + + test('a synced codex agent keeps its original file mode', () => { + // The tmp-file + rename publish does not preserve mode the way an + // in-place writeFileSync would — cmdEffortSyncCodex stat+chmods the tmp + // file before the rename specifically to compensate, same discipline as + // the OpenCode branch. Mode bits are not meaningful on Windows, so skip + // there. The reachable rewrite path is the Anthropic-model-strip: a + // `.toml` agent carrying an Anthropic-flavored `model` value. + if (process.platform === 'win32') return; + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + const before = ['model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', ''].join('\n'); + const filePath = writeAgent(agentsDir, 'gsd-executor.toml', [before]); + fs.chmodSync(filePath, 0o600); + + const opts = { dryRun: false, configDir, runtime: 'codex' }; + const script = [ + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + + const after = fs.readFileSync(filePath, 'utf8'); + assert.notStrictEqual(after, before, 'the sync must have actually rewritten the file'); + assert.doesNotMatch(after, /claude/i, 'the Anthropic-flavored model pin must have been stripped'); + assert.strictEqual( + fs.statSync(filePath).mode & 0o777, 0o600, + 'the published file must keep the original file mode', + ); + } finally { + cleanup(root); + } + }); + + test('an unreadable codex agent is reported and does not abort the sweep', () => { + // Protects: cmdEffortSyncCodex's fs.readFileSync used to sit outside any + // try, so a single unreadable agent file would throw and abort the + // entire sweep instead of degrading into read_failures like the write + // path in the same loop. Injected deterministically by monkeypatching + // fs.readFileSync inside the child script — per this repo's CLAUDE.md + // §4, chmod-based injection is forbidden (root bypasses mode bits under + // Docker/CI and it leaks resources). The Anthropic-flavored `model` pin + // is the reachable codex rewrite path (verified by execution above), so + // the sibling agent uses that fixture to prove it still gets rewritten. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeAgent(agentsDir, 'gsd-unreadable.toml', [ + 'model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', '', + ]); + const okPath = writeAgent(agentsDir, 'gsd-ok.toml', [ + 'model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', '', + ]); + const okBefore = fs.readFileSync(okPath, 'utf8'); + + const opts = { dryRun: false, configDir, runtime: 'codex' }; + const script = [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.toml')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + const jsonStart = spawned.stdout.indexOf('{'); + assert.notStrictEqual(jsonStart, -1, `expected JSON output on stdout, got:\n${spawned.stdout}`); + const result = JSON.parse(spawned.stdout.slice(jsonStart)); + + assert.strictEqual(result.read_failures.length, 1); + assert.strictEqual(result.read_failures[0].agent, 'gsd-unreadable'); + + const okAfter = fs.readFileSync(okPath, 'utf8'); + assert.notStrictEqual(okAfter, okBefore, 'the sweep must continue past the unreadable agent and still rewrite the sibling'); + assert.doesNotMatch(okAfter, /claude/i, 'the sibling agent must have had its Anthropic-flavored model pin stripped'); + } finally { + cleanup(root); + } + }); + + test('the summary reports failed when a codex sync could not complete', () => { + // Protects: same raw-mode summary TOKEN contract as the opencode case + // above — output()'s third argument is never merged into the JSON + // object, so a raw-mode caller can only see the failure via this token. + // Captured the same way: run with `raw: true` and assert on the literal + // stdout text, not on parsed JSON. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeAgent(agentsDir, 'gsd-unreadable.toml', [ + 'model = "claude-sonnet-4"', 'model_reasoning_effort = "high"', '', + ]); + + const opts = { dryRun: false, configDir, runtime: 'codex' }; + const script = [ + `const fs = require('node:fs');`, + `const originalReadFileSync = fs.readFileSync;`, + `fs.readFileSync = function (targetPath, ...rest) {`, + ` if (typeof targetPath === 'string' && targetPath.includes('gsd-unreadable.toml')) {`, + ` throw new Error('injected read failure');`, + ` }`, + ` return originalReadFileSync.call(fs, targetPath, ...rest);`, + `};`, + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, true, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + assert.strictEqual(spawned.stdout.trim(), 'failed', `expected the raw summary token 'failed', got:\n${spawned.stdout}`); + } finally { + cleanup(root); + } + }); +}); + +// #3706 — setFrontmatterKeyLine / removeFrontmatterKeyLine are not exported +// directly; they are reached through the exported cmdEffortSync (claude +// runtime, `effort:` key), the only public entry point that exercises them. +describe('#3706: frontmatter line editors are scoped to the matched block', () => { + function makeSandbox() { + const root = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-effort-sync-fm-scope-')); + const cwd = path.join(root, 'project'); + const configDir = path.join(root, 'runtime-home'); + const agentsDir = path.join(configDir, 'agents'); + const home = path.join(root, 'home'); + fs.mkdirSync(cwd, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.mkdirSync(home, { recursive: true }); + return { root, cwd, configDir, agentsDir, home }; + } + + function writeProjectEffortConfig(cwd, override) { + fs.mkdirSync(path.join(cwd, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(cwd, '.planning', 'config.json'), + JSON.stringify({ effort: { agent_overrides: { 'gsd-executor': override } } }), + ); + } + + function runEffortSync({ cwd, home, configDir }) { + const opts = { dryRun: false, configDir, runtime: 'claude' }; + const script = [ + `const { cmdEffortSync } = require(${JSON.stringify(COMMANDS_CJS)});`, + `cmdEffortSync(${JSON.stringify(cwd)}, false, ${JSON.stringify(opts)});`, + ].join('\n'); + const spawned = runNode(['-e', script], { + cwd, + env: { ...process.env, HOME: home, USERPROFILE: home }, + }); + assert.strictEqual( + spawned.exitCode, 0, + `cmdEffortSync child process failed (outcome=${spawned.outcome}):\nstdout: ${spawned.stdout}\nstderr: ${spawned.stderr}`, + ); + } + + test('a CRLF document with a preamble keeps its opening fence intact', () => { + // Pre-fix: openLen was derived from `/^---\r\n/.test(content)` (start of + // file, which here is "Preamble line", not CRLF) rather than from the + // matched frontmatter block, so the CRLF fence misaligned by one byte. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'inherit'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = 'Preamble line\r\n\r\n---\r\nname: x\r\neffort: high\r\n---\r\n\r\nBody.\r\n'; + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const after = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual(after, 'Preamble line\r\n\r\n---\r\nname: x\r\n---\r\n\r\nBody.\r\n'); + assert.ok(after.includes('---\r\nname: x\r\n'), 'the CRLF opening fence must survive intact'); + assert.ok(after.startsWith('Preamble line\r\n\r\n'), 'preamble must survive untouched'); + assert.ok(after.endsWith('\r\n\r\nBody.\r\n'), 'body must survive untouched'); + assert.doesNotMatch(after, /^-{1,2}\r?\n/m, 'no truncated/stray fence'); + } finally { + cleanup(root); + } + }); + + test('a preamble line starting with the key is not the line rewritten', () => { + // Pre-fix: `content.replace(keyLineRe, ...)` was a whole-file /m replace + // gated only on the key being present in fmBody, so the FIRST matching + // line in the whole file (the preamble) was rewritten instead of the + // frontmatter line. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'xhigh'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = 'effort: not-the-frontmatter\n\n---\nname: x\neffort: high\n---\n\nBody.\n'; + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const after = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual(after, 'effort: not-the-frontmatter\n\n---\nname: x\neffort: xhigh\n---\n\nBody.\n'); + // Bytes-based scoping (not a whole-file regex): the preamble line must + // precede the first `---` fence, and only the line AFTER that fence may + // read `effort: xhigh`. + const firstFence = after.indexOf('---'); + assert.ok(after.slice(0, firstFence).includes('effort: not-the-frontmatter'), 'preamble line must be untouched'); + assert.ok(after.slice(firstFence).includes('effort: xhigh'), 'frontmatter line must carry the new value'); + } finally { + cleanup(root); + } + }); + + test('a document whose frontmatter starts at byte 0 is byte-identical to before', () => { + // No-regression control: a normal install-written agent (frontmatter at + // byte 0, LF) must see only the one targeted line change, nothing else. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'xhigh'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.'].join('\n'); + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const afterSet = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual( + afterSet, + ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'effort: xhigh', '---', '', 'Body.'].join('\n'), + ); + + writeProjectEffortConfig(cwd, 'inherit'); + const beforeRemove = ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', 'effort: high', '---', '', 'Body.'].join('\n'); + fs.writeFileSync(filePath, beforeRemove); + runEffortSync({ cwd, home, configDir }); + const afterRemove = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual( + afterRemove, + ['---', 'name: gsd-executor', 'description: x', 'mode: subagent', '---', '', 'Body.'].join('\n'), + ); + } finally { + cleanup(root); + } + }); + + test('a duplicated key converges to one occurrence on a single set', () => { + // Protects: setFrontmatterKeyLine used to rewrite only the FIRST + // occurrence of a duplicated key, leaving a stale second occurrence in + // place. A last-wins YAML reader would then honour the stale value while + // this function's own first-occurrence read reports "in sync" — a + // permanently non-converging state. One run must collapse a duplicated + // key to exactly one occurrence, carrying the new value, in the position + // of the FIRST occurrence. + const { root, cwd, configDir, agentsDir, home } = makeSandbox(); + try { + writeProjectEffortConfig(cwd, 'xhigh'); + const filePath = path.join(agentsDir, 'gsd-executor.md'); + const before = [ + '---', 'name: gsd-executor', 'effort: low', 'description: x', 'effort: high', '---', '', 'Body.', + ].join('\n'); + fs.writeFileSync(filePath, before); + runEffortSync({ cwd, home, configDir }); + const after = fs.readFileSync(filePath, 'utf8'); + assert.strictEqual( + after, + ['---', 'name: gsd-executor', 'effort: xhigh', 'description: x', '---', '', 'Body.'].join('\n'), + 'exactly one effort: line must remain, at the position of the first occurrence, carrying the resolved value', + ); + const occurrences = after.match(/^effort:/gm) || []; + assert.strictEqual(occurrences.length, 1, 'exactly one effort: line must remain'); + } finally { + cleanup(root); + } + }); +}); diff --git a/tests/frontmatter.unit.test.cjs b/tests/frontmatter.unit.test.cjs index 54e7d82dc..023674dc8 100644 --- a/tests/frontmatter.unit.test.cjs +++ b/tests/frontmatter.unit.test.cjs @@ -25,6 +25,8 @@ const { noOpObjectListSetError, parseMustHavesBlock, FRONTMATTER_SCHEMAS, + agentScalarNeedsDoubleQuoting, + escapeDoubleQuoted, } = require('../gsd-core/bin/lib/frontmatter.cjs'); // ─── extractFrontmatter ─────────────────────────────────────────────────────── @@ -1512,3 +1514,236 @@ describe('stripFrontmatter', () => { assert.strictEqual(stripFrontmatter(''), ''); }); }); + +// ─── agentScalarNeedsDoubleQuoting (#3706) ──────────────────────────────────── +// +// Mutant-killing discipline: every boolean clause gets a true case AND a +// near-miss false case one edit away, so flipping any operator/character +// class in the source changes at least one assertion here. + +describe('agentScalarNeedsDoubleQuoting: delegates to scalarNeedsDoubleQuoting', () => { + test('empty string needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting(''), true); + }); + test('embedded double quote needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a"b'), true); + }); + test('embedded backslash needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a\\b'), true); + }); + test('embedded control char needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a\u0001b'), true); + }); + test('leading whitespace needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting(' abc'), true); + }); + test('trailing whitespace needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('abc '), true); + }); + for (const c of ['#', '&', '*', '!', '|', '>', '%', '@', '`', '[', ']', '{', '}', ',', "'"]) { + test(`leading indicator "${c}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(`${c}foo`), true); + }); + } + test('near-miss: plain word does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('plainword'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: first character must be alphanumeric', () => { + for (const v of ['~', '.inf', '.nan', '+1', '-1', '-0', '.5']) { + test(`non-alphanumeric first char "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: alphanumeric-first with internal hyphen does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a-b'), false); + }); + test('near-miss: digit-first non-numeric word does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('0abc'), false); + }); + test('near-miss: alphanumeric-first with trailing hyphen does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('x-'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: trailing colon', () => { + test('a bare trailing colon needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('foo:'), true); + }); + test('near-miss: colon followed by more text does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('foo:bar'), false); + }); + test('near-miss: single-char key:value shape does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a:b'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: embedded ": " (colon + whitespace)', () => { + test('colon followed by space needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a: b'), true); + }); + test('colon followed by tab needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a:\tb'), true); + }); + test('near-miss: colon with no following whitespace does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a:b'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: embedded " #" (whitespace + hash)', () => { + test('space followed by hash needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a #b'), true); + }); + test('tab followed by hash needs quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a\t#b'), true); + }); + test('near-miss: hash with no preceding whitespace does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a#b'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: boolean/null words (case-insensitive)', () => { + for (const v of ['y', 'n', 'yes', 'no', 'true', 'false', 'on', 'off', 'null']) { + test(`lowercase word "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + for (const v of ['YES', 'No', 'TRUE', 'Null']) { + test(`mixed-case word "${v}" needs quoting (case-insensitivity)`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: "yes1" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('yes1'), false); + }); + test('near-miss: "nope" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('nope'), false); + }); + test('near-miss: "nullish" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('nullish'), false); + }); + test('near-miss: "onward" is not an exact word match', () => { + assert.equal(agentScalarNeedsDoubleQuoting('onward'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: numeric-looking values', () => { + for (const v of [ + '1', '123', '1.5', '1.', '1e5', '1E5', '1e+5', '1e-5', '1_000', + '0x1F', '0b101', '0o17', '0X1f', '12:30', '1:2:3', '12:30.5', + ]) { + test(`numeric form "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: "1a" (digit then letter) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('1a'), false); + }); + test('near-miss: "a1" (letter then digit) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('a1'), false); + }); + test('near-miss: "x1e5" (non-digit first char) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('x1e5'), false); + }); + test('near-miss: "0xzz" (invalid hex digits) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('0xzz'), false); + }); + // Verified against the real implementation: the sexagesimal group only + // consumes a leading [0-5]? then one mandatory digit per ":" segment, so + // "12:99" cannot fully match YAML_NUMERIC_RE (the second "9" is left over) + // and no other clause fires either. The code does NOT reject an + // out-of-range (60-99) minute-like group — pinned here as actual behavior, + // not the originally assumed "false because minutes must be 0-59". + test('"12:99" does not need quoting (sexagesimal regex cannot consume the trailing digit)', () => { + assert.equal(agentScalarNeedsDoubleQuoting('12:99'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: YAML timestamp', () => { + for (const v of [ + '2026-08-25', '2026-8-5', '2026-08-25T10:00:00Z', '2026-08-25 10:00:00', + ]) { + test(`timestamp form "${v}" needs quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), true); + }); + } + test('near-miss: "2026-08" (missing day) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('2026-08'), false); + }); + // Verified against the real implementation: this string never reaches the + // timestamp clause at all — the pure-digit numeric clause (YAML_NUMERIC_RE's + // first alternative matches any \d[\d_]* string) fires first and returns + // true. So "20260825" IS true, but for a different reason than "looks like + // a date"; it does not exercise YAML_TIMESTAMP_RE. + test('"20260825" (no hyphens) needs quoting via the numeric clause, not the timestamp clause', () => { + assert.equal(agentScalarNeedsDoubleQuoting('20260825'), true); + }); + // This is the case that actually pins YAML_TIMESTAMP_RE's trailing + // `(?:[Tt ].*)?$` anchor: the numeric clause cannot match (hyphens present, + // no colon), so only the timestamp regex is left to decide, and it rejects + // trailing text that isn't introduced by "T"/"t"/" ". + test('"2026-08-25x" (trailing junk not preceded by T/t/space) does not need quoting', () => { + assert.equal(agentScalarNeedsDoubleQuoting('2026-08-25x'), false); + }); +}); + +describe('agentScalarNeedsDoubleQuoting: real-world values that must stay unquoted', () => { + for (const v of [ + 'sonnet', 'synthetic/hf:zai-org/GLM-5.2', 'gpt-5.6-luna', 'claude-opus-5', + 'high', 'xhigh', 'minimal', 'a.b', 'GLM-5.2', + ]) { + test(`"${v}" does not need quoting`, () => { + assert.equal(agentScalarNeedsDoubleQuoting(v), false); + }); + } +}); + +// ─── escapeDoubleQuoted (#1779 / #3497) ──────────────────────────────────────── + +describe('escapeDoubleQuoted: exact output strings', () => { + test('backslash is escaped before the quote it precedes (ordering matters)', () => { + // Input: a, \, ", b. If the quote were escaped BEFORE the backslash, the + // backslash added in front of the quote would then itself get doubled by + // a subsequent backslash pass, producing a different (wrong) string. The + // real order (backslash first, then quote) yields exactly 3 backslashes + // followed by the quote. + const input = 'a' + '\\' + '"' + 'b'; + const expected = 'a' + '\\'.repeat(3) + '"' + 'b'; + assert.equal(escapeDoubleQuoted(input), expected); + }); + + test('double quote alone', () => { + assert.equal(escapeDoubleQuoted('"'), '\\"'); + }); + + test('newline alone', () => { + assert.equal(escapeDoubleQuoted('\n'), '\\n'); + }); + + test('tab alone', () => { + assert.equal(escapeDoubleQuoted('\t'), '\\t'); + }); + + test('carriage return alone', () => { + assert.equal(escapeDoubleQuoted('\r'), '\\r'); + }); + + test('a C0 control char (0x01) becomes lowercase zero-padded \\xHH', () => { + assert.equal(escapeDoubleQuoted('\u0001'), '\\x01'); + }); + + test('DEL (0x7f) becomes \\x7f', () => { + assert.equal(escapeDoubleQuoted('\u007f'), '\\x7f'); + }); + + test('plain string with no specials is returned unchanged', () => { + assert.equal(escapeDoubleQuoted('plain'), 'plain'); + }); + + test('combined input exercising every escape in one pass', () => { + const input = 'a\\b"c\nd\te\rf\u0001g\u007fh'; + const expected = 'a\\\\b\\"c\\nd\\te\\rf\\x01g\\x7fh'; + assert.equal(escapeDoubleQuoted(input), expected); + }); +}); diff --git a/tests/mutation-matrix-ratchet.test.cjs b/tests/mutation-matrix-ratchet.test.cjs index bba8a90e5..51960461e 100644 --- a/tests/mutation-matrix-ratchet.test.cjs +++ b/tests/mutation-matrix-ratchet.test.cjs @@ -194,7 +194,7 @@ const RATCHET_BASELINE = { 'context-utilization': 80, 'context-composer': 66, // #2929: extracted from prompt-budget; same floor as prompt-budget 'prompt-budget': 66, // CI 68.33% 2026-06-14; was 90 (timeout-inflated local) - 'frontmatter': 62, + 'frontmatter': 65, // #3706: raised from 62; measured 66.67 on PR 3867 'adr-parser': 68, 'config-schema': 52, // CI 54.55% 2026-06-14; was 68 (timeout-inflated local) 'active-workstream-store': 80, diff --git a/tests/runtime-converters.test.cjs b/tests/runtime-converters.test.cjs index aa216ab5d..5905f8471 100644 --- a/tests/runtime-converters.test.cjs +++ b/tests/runtime-converters.test.cjs @@ -2208,3 +2208,374 @@ describe('bug-2876: skill+agent converters emit YAML-quoted description', () => }); }); } + +// ─── #3706: OpenCode subagent effort (variant) + frontmatter escaping ──────── +// +// `query resolve-execution` reported an effort (high/xhigh) that never reached +// OpenCode: the bake wrote `model:` and nothing else, so every subagent ran at +// whatever opencode.jsonc defaults the model to. The effort-side twin of #3705. +// +// bin/install.js has its own copy of these converters, but no `isAgent: true` +// call site: its agents install path runs through the compiled bin/lib module, +// not through its own copy. The live copy in src/runtime-artifact-conversion.cts +// (compiled to gsd-core/bin/lib/runtime-artifact-conversion.cjs) is therefore +// the one the bake actually uses, and the one under test here. +const liveConversion = require('../gsd-core/bin/lib/runtime-artifact-conversion.cjs'); + +const OPENCODE_CONVERTERS = [['src (live bake)', liveConversion.convertClaudeToOpencodeFrontmatter]]; +const KILO_CONVERTERS = [['src (live bake)', liveConversion.convertClaudeToKiloFrontmatter]]; + +describe('#3706: OpenCode agent variant (resolved effort)', () => { + const AGENT = ['---', 'name: gsd-executor', 'description: x', 'model: sonnet', '---', '', 'Body.'].join('\n'); + const hasKey = (out, key) => new RegExp(`^${key}:`, 'm').test(out); + + for (const [label, convert] of OPENCODE_CONVERTERS) { + test(`[${label}] emits variant alongside model`, () => { + const out = convert(AGENT, { + isAgent: true, modelOverride: 'synthetic/hf:zai-org/GLM-5.2', variant: 'high', + }); + assert.match(out, /^model: synthetic\/hf:zai-org\/GLM-5\.2$/m); + assert.match(out, /^variant: high$/m); + }); + + test(`[${label}] carries whatever level was resolved, not a fixed one`, () => { + assert.match(convert(AGENT, { isAgent: true, variant: 'xhigh' }), /^variant: xhigh$/m); + }); + + test(`[${label}] omits the key entirely when no variant is supplied`, () => { + // The control that keeps every existing install byte-identical. + const out = convert(AGENT, { isAgent: true, modelOverride: 'M' }); + assert.ok(!hasKey(out, 'variant'), `expected no variant key, got:\n${out}`); + assert.match(out, /^model: M$/m, 'the model side is unaffected'); + }); + + test(`[${label}] commands get neither model nor variant`, () => { + const out = convert(AGENT, { isAgent: false, modelOverride: 'M', variant: 'high' }); + assert.ok(!hasKey(out, 'variant')); + assert.ok(!hasKey(out, 'model')); + }); + + test(`[${label}] adding a variant perturbs no other line`, () => { + const withV = convert(AGENT, { isAgent: true, modelOverride: 'M', variant: 'high' }); + const without = convert(AGENT, { isAgent: true, modelOverride: 'M' }); + assert.match(withV, /^mode: subagent$/m); + assert.deepEqual( + withV.split('\n').filter((l) => !/^variant:/.test(l)), + without.split('\n'), + ); + }); + } + + for (const [label, convert] of KILO_CONVERTERS) { + test(`[${label}] Kilo does NOT emit variant even when one is passed`, () => { + // The asymmetry, pinned. EFFORT_ARGV declares surfaces for claude/opencode/ + // codex and has no kilo entry — unlike the model side, where #2794 J8 + // requires the two runtimes to resolve identically. + const out = convert(AGENT, { isAgent: true, modelOverride: 'M', variant: 'high' }); + assert.ok(!hasKey(out, 'variant'), `kilo has no declared effort surface; got:\n${out}`); + assert.match(out, /^model: M$/m, 'but the model override still applies'); + }); + } +}); + +// ─── #3706: config-supplied values cannot inject frontmatter keys ──────────── +// +// `model:` and now `variant:` both interpolate values read from +// .planning/config.json / ~/.gsd/defaults.json. Raw interpolation let a value +// containing a newline inject additional TOP-LEVEL keys — proven by execution +// during the #3705 security review. The sink predates #3706; this change adds a +// SECOND write to it, so it is closed here rather than doubled. +describe('#3706: frontmatter values are escaped, not interpolated raw', () => { + const AGENT = ['---', 'name: gsd-executor', 'description: x', '---', '', 'Body.'].join('\n'); + + for (const [label, convert] of OPENCODE_CONVERTERS) { + test(`[${label}] a newline-bearing model value cannot add top-level keys`, () => { + const out = convert(AGENT, { + isAgent: true, modelOverride: 'sonnet\ntools: ["*"]\npermission: bypass', + }); + assert.ok(!/^tools:/m.test(out), `injected a tools key:\n${out}`); + assert.ok(!/^permission: bypass$/m.test(out), `injected a permission key:\n${out}`); + assert.match(out, /^model: "/m, 'the value is quoted'); + }); + + test(`[${label}] a newline-bearing variant value cannot add top-level keys`, () => { + const out = convert(AGENT, { isAgent: true, variant: 'high\nevil: yes' }); + assert.ok(!/^evil:/m.test(out), `injected a key:\n${out}`); + assert.match(out, /^variant: "/m); + }); + } + + for (const [label, convert] of KILO_CONVERTERS) { + test(`[${label}] Kilo model values are escaped too — the same sink exists there`, () => { + const out = convert(AGENT, { isAgent: true, modelOverride: 'sonnet\ntools: ["*"]' }); + assert.ok(!/^tools:/m.test(out), `injected a tools key:\n${out}`); + }); + } +}); + +// ─── #3706: values that look plain but do not round-trip ──────────────────── +// +// Matching /^[A-Za-z0-9._:\/@+-]+$/ is not the same question as "does YAML read +// this back as the exact string that went in". Each row below is wrong when +// emitted bare; the last is the control that keeps real installs unchanged. +describe('#3706: YAML-ambiguous scalars are quoted, not emitted bare', () => { + const AGENT = ['---', 'name: gsd-executor', 'description: x', '---', '', 'Body.'].join('\n'); + + const QUOTED = [ + // A PARSE ERROR bare, not merely ambiguous: '@' is a YAML reserved + // indicator and may not open a plain scalar, so the whole agent file + // becomes unreadable. + '@org/model', + // A trailing ':' reads as a nested mapping key — "bad indentation of a + // mapping entry" kills the frontmatter. + 'foo:', + // YAML 1.1 resolves these to booleans/null, so a variant named 'no' + // arrives as `false` and matches no entry in the user's variants map. + 'no', 'yes', 'true', 'off', 'y', 'n', 'null', + // '12:30' resolves to the integer 750; ':' is legal mid-identifier here, + // so the form is reachable rather than contrived. + '12:30', '0755', '1.5', '0x1f', + // #3706 hardening: '(.+?)' in the value regex needs one character before + // this fix, so an empty/whitespace-only value read as "key absent" rather + // than "key present, value empty" — these are the YAML-ambiguous classes + // that predicate missed once presence and value became distinct questions. + '~', '.inf', '.Inf', '.nan', '+1', '-1', '-0', '+1.5', '.5', '0x1F', + '2026-08-25', '2026-08-25T10:00:00Z', 'a: b', 'a #b', + ]; + // The whole point of the predicate: no churn for anybody's existing files. + const BARE = ['sonnet', 'synthetic/hf:zai-org/GLM-5.2', 'gpt-5.6-luna', 'claude-opus-5', 'x-', 'a.b', 'GLM-5.2']; + + for (const [label, convert] of OPENCODE_CONVERTERS) { + const modelLine = (v) => + (convert(AGENT, { isAgent: true, modelOverride: v }) + .split('\n').find((l) => l.startsWith('model:')) ?? ''); + + for (const v of QUOTED) { + test(`[${label}] quotes ${JSON.stringify(v)}`, () => { + assert.equal(modelLine(v), `model: "${v}"`); + }); + } + for (const v of BARE) { + test(`[${label}] leaves ${JSON.stringify(v)} bare`, () => { + assert.equal(modelLine(v), `model: ${v}`); + }); + } + test(`[${label}] the same predicate governs variant, not just model`, () => { + assert.match(convert(AGENT, { isAgent: true, variant: 'no' }), /^variant: "no"$/m); + }); + } +}); + +// ─── #3706: the threading seam — where the bug actually was ───────────────── +// +// Every test above exercises a converter directly. #3706 broke one layer up, in +// runtime-artifact-layout's convertedAgentsKind: the converter had no variant to +// write because nothing resolved or passed one. A converter-only suite goes green +// while the value still never arrives, so the resolve→render chain is pinned here. +describe('#3706: install-time effort resolves and renders for OpenCode', () => { + const effortResolver = require('../gsd-core/bin/lib/install-effort-resolver.cjs'); + const catalog = require('../gsd-core/bin/lib/model-catalog.cjs'); + + // Exactly the composition the layout performs per agent. + const thread = (effortCfg, agentName) => { + const universal = effortCfg ? effortResolver.resolveInstallTimeEffort(effortCfg, agentName) : null; + return universal ? catalog.clampEffortForHost('opencode', universal) : null; + }; + + test('no effort config at all yields no variant', () => { + // The gate. resolveInstallTimeEffort ALWAYS returns a level ('high' from the + // catalog default) even for a null config, so gating on its return value + // would stamp `variant:` into every existing OpenCode install. + assert.equal(effortResolver.resolveInstallTimeEffort(null, 'gsd-executor'), 'high'); + assert.equal(thread(null, 'gsd-executor'), null); + }); + + test('a configured agent override reaches the emitted value', () => { + assert.equal(thread({ agent_overrides: { 'gsd-executor': 'xhigh' } }, 'gsd-executor'), 'xhigh'); + }); + + test('a configured default reaches an agent with no catalog tier', () => { + // effort.default is only consulted for agents the tier ladder does not + // answer for; see the tiered-agent test below for the other half. + assert.equal(thread({ default: 'low' }, 'not-a-catalog-agent'), 'low'); + }); + + test("'inherit' is never emitted as a literal", () => { + // #3533 (10d): inherit means "omit the key and follow the host default" and + // is not a wire level on ANY runtime. It IS a member of EFFORT_SET, so the + // resolver returns it verbatim — the declared render seam is what drops it. + // Writing `variant: inherit` would name a variant that cannot resolve. + for (const cfg of [ + { agent_overrides: { 'gsd-executor': 'inherit' } }, + { routing_tier_defaults: { light: 'inherit', standard: 'inherit', heavy: 'inherit' } }, + ]) { + assert.equal(effortResolver.resolveInstallTimeEffort(cfg, 'gsd-executor'), 'inherit'); + assert.equal(thread(cfg, 'gsd-executor'), null, `should omit for ${JSON.stringify(cfg)}`); + } + }); + + test('a bare effort.default does NOT reach a tiered agent', () => { + // Not obvious, and it is why the `inherit` case above goes through + // agent_overrides instead: for an agent WITH a catalog tier the manifest + // tier ladder (#3531) answers before effort.default is ever consulted, so + // `default: 'inherit'` leaves a tiered agent at its tier value. Pinned + // because a test written against `default` alone would assert nothing. + assert.equal(effortResolver.resolveInstallTimeEffort({ default: 'inherit' }, 'gsd-executor'), 'high'); + assert.equal(thread({ default: 'inherit' }, 'gsd-executor'), 'high'); + }); + + test('a level OpenCode does not accept is dropped, not emitted', () => { + // 'ultra' is a real EFFORT_SET member that OpenCode's declared supported set + // does not contain. Omitting beats naming a variant that cannot resolve. + assert.equal(catalog.renderEffortArgv('opencode', 'ultra', 'argv').value, null); + }); + + test('the full universal ladder OpenCode does accept passes through', () => { + for (const lvl of ['minimal', 'low', 'medium', 'high', 'xhigh', 'max']) { + assert.equal(catalog.renderEffortArgv('opencode', lvl, 'argv').value, lvl); + } + }); + + test('an empty model override omits the key rather than writing an empty value', () => { + // Found by the property below, which originally asserted a round-trip for + // every generated string and failed on "". An empty override is falsy, so + // no `model:` line is written at all — that is the #2256/J7 contract (omit + // the key, never `model: ""`), so the property is scoped to non-empty + // values and the empty case is pinned here instead of being generated away. + const out = liveConversion.convertClaudeToOpencodeFrontmatter( + ['---', 'name: x', 'description: y', '---', '', 'Body.'].join('\n'), + { isAgent: true, modelOverride: '' }, + ); + assert.ok(!/^model:/m.test(out), `expected no model key, got:\n${out}`); + }); + + test('every emitted model line parses back to the exact value', () => { + // The real contract, stated directly: whatever a user puts in config, the + // generated frontmatter must read back as that same string. A per-character + // rule list is only a means to this end, and this is what catches the case + // nobody thought to enumerate. + const fc = require('fast-check'); + const yaml = require('js-yaml'); + const chars = ':@-_./"\\ \n\t#&*!|>%`{}[],?~+abcXY019'; + fc.assert( + fc.property( + fc.string({ unit: fc.constantFrom(...chars.split('')), minLength: 1 }), + (value) => { + const out = liveConversion.convertClaudeToOpencodeFrontmatter( + ['---', 'name: x', 'description: y', '---', '', 'Body.'].join('\n'), + { isAgent: true, modelOverride: value }, + ); + const fmBody = /^---\r?\n([\s\S]*?)^---\r?$/m.exec(out)[1]; + return yaml.load(fmBody).model === value; + }, + ), + { numRuns: 2000 }, + ); + }); +}); + +// ─── #3706: the layout seam actually threads the variant ───────────────────── +// +// Every test above calls a converter directly. Direct-converter tests all +// still pass if a future edit drops `variant` from the `rawConverter(...)` +// call inside runtime-artifact-layout.cts's convertedAgentsKind, or flips its +// `converterName ===` gate — this drives the REAL staging path +// (resolveRuntimeArtifactLayout -> the OpenCode 'agents' kind's stage()) so +// that regression cannot hide behind converter-only coverage. +describe('#3706: the layout seam actually threads the variant', () => { + const fs = require('node:fs'); + const os = require('node:os'); + const path = require('node:path'); + const { cleanup } = require('./helpers.cjs'); + const { resolveRuntimeArtifactLayout } = require('../gsd-core/bin/lib/runtime-artifact-layout.cjs'); + + // Fixture layout mirrors the .gsd-source marker convention + // (findAgentsSourceRoot): the marker at /.gsd-source points to + // /commands/gsd, and agents/ is resolved as its sibling. + function makeConfigDirFixture(agentContent) { + const configDir = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-configdir-')); + const commandsDir = path.join(configDir, 'commands', 'gsd'); + const agentsDir = path.join(configDir, 'agents'); + fs.mkdirSync(commandsDir, { recursive: true }); + fs.mkdirSync(agentsDir, { recursive: true }); + fs.writeFileSync(path.join(configDir, '.gsd-source'), commandsDir + '\n', 'utf8'); + fs.writeFileSync(path.join(agentsDir, 'gsd-executor.md'), agentContent, 'utf8'); + return configDir; + } + + function makeProjectRootWithEffort(effort) { + const projectRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-project-')); + fs.mkdirSync(path.join(projectRoot, '.planning'), { recursive: true }); + fs.writeFileSync( + path.join(projectRoot, '.planning', 'config.json'), + JSON.stringify({ effort }), + 'utf8', + ); + return projectRoot; + } + + // resolveInstallTimeEffort also merges ~/.gsd/defaults.json (os.homedir()), + // so HOME/USERPROFILE are pinned to an empty temp dir for the duration of + // each stage() call to keep the assertion hermetic against the real + // developer machine's home directory. + function stageWithFakeHome(agentKind, resolvedProfile, agentCtx) { + const fakeHome = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-home-')); + const realHome = process.env.HOME; + const realUserProfile = process.env.USERPROFILE; + process.env.HOME = fakeHome; + process.env.USERPROFILE = fakeHome; + try { + return agentKind.stage(resolvedProfile, agentCtx); + } finally { + // On POSIX, USERPROFILE (and potentially HOME) is unset before this + // helper runs — `process.env.X = undefined` would coerce to the + // literal string "undefined" and leak that into the environment for + // the rest of the test process. Restore by deletion when the saved + // value was genuinely absent. + if (realHome === undefined) delete process.env.HOME; + else process.env.HOME = realHome; + if (realUserProfile === undefined) delete process.env.USERPROFILE; + else process.env.USERPROFILE = realUserProfile; + cleanup(fakeHome); + } + } + + const AGENT_SOURCE = ['---', 'name: gsd-executor', 'description: x', '---', '', 'Body.'].join('\n'); + const resolvedProfile = { name: 'full', skills: '*', agents: new Set() }; + + test('a configured effort reaches the real OpenCode agents kind staging output', () => { + const configDir = makeConfigDirFixture(AGENT_SOURCE); + const projectRoot = makeProjectRootWithEffort({ agent_overrides: { 'gsd-executor': 'xhigh' } }); + try { + const layout = resolveRuntimeArtifactLayout('opencode', configDir, 'global'); + const agentKind = layout.kinds.find((k) => k.kind === 'agents'); + assert.ok(agentKind, 'opencode global layout must include an agents kind'); + + const agentCtx = { runtime: 'opencode', pathPrefix: '', attribution: null, targetDir: projectRoot }; + const stagedDir = stageWithFakeHome(agentKind, resolvedProfile, agentCtx); + const stagedContent = fs.readFileSync(path.join(stagedDir, 'gsd-executor.md'), 'utf8'); + assert.match(stagedContent, /^variant: xhigh$/m, `expected variant: xhigh in staged output:\n${stagedContent}`); + } finally { + cleanup(configDir); + cleanup(projectRoot); + } + }); + + test('no effort config at all reaches the real staging output with no variant key', () => { + const configDir = makeConfigDirFixture(AGENT_SOURCE); + const noConfigRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'gsd-3706-seam-noconfig-')); + try { + const layout = resolveRuntimeArtifactLayout('opencode', configDir, 'global'); + const agentKind = layout.kinds.find((k) => k.kind === 'agents'); + + const agentCtx = { runtime: 'opencode', pathPrefix: '', attribution: null, targetDir: noConfigRoot }; + const stagedDir = stageWithFakeHome(agentKind, resolvedProfile, agentCtx); + const stagedContent = fs.readFileSync(path.join(stagedDir, 'gsd-executor.md'), 'utf8'); + assert.doesNotMatch(stagedContent, /^variant:/m, `expected no variant key in staged output:\n${stagedContent}`); + } finally { + cleanup(configDir); + cleanup(noConfigRoot); + } + }); +}); +