From 50d5368adddea17a7e8551155672102e22a201b8 Mon Sep 17 00:00:00 2001 From: Tom Boucher Date: Sat, 15 Aug 2026 07:00:44 -0400 Subject: [PATCH] =?UTF-8?q?fix(#3533):=20effort=20inherit=20=E2=80=94=20ex?= =?UTF-8?q?pressible,=20omitted=20at=20writers,=20never=20re-added=20(#354?= =?UTF-8?q?1)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit --- .changeset/sharp-ibex-tumble.md | 5 ++ bin/install.js | 17 +++-- docs/CONFIGURATION.md | 9 ++- src/commands.cts | 44 ++++++++++++- src/model-catalog.cts | 7 +++ src/model-resolver.cts | 7 ++- tests/commands.test.cjs | 79 ++++++++++++++++++++++++ tests/effort-surface-axis.test.cjs | 17 +++++ tests/install-runtime-artifacts.test.cjs | 55 +++++++++++++++++ tests/model-resolver.test.cjs | 52 +++++++++++++++- 10 files changed, 284 insertions(+), 8 deletions(-) create mode 100644 .changeset/sharp-ibex-tumble.md diff --git a/.changeset/sharp-ibex-tumble.md b/.changeset/sharp-ibex-tumble.md new file mode 100644 index 000000000..97c969060 --- /dev/null +++ b/.changeset/sharp-ibex-tumble.md @@ -0,0 +1,5 @@ +--- +type: Added +pr: 3541 +--- +**Effort now supports `inherit` — "follow the session" is a first-class, declarable choice** — `effort.agent_overrides`, `routing_tier_defaults`, and `effort.default` accept `inherit`; the install-time writer omits the `effort:` frontmatter key for agents resolving to it (Codex omits the `model_reasoning_effort` pin), and `effort sync --apply` no longer re-adds a hand-stripped key — an absent key under `inherit` is in-sync, and a present one is stripped. An explicit `inherit` never escalates on failed attempts. (#3533) diff --git a/bin/install.js b/bin/install.js index b36956134..c6dff753c 100755 --- a/bin/install.js +++ b/bin/install.js @@ -4282,8 +4282,12 @@ function generateCodexAgentToml(agentName, agentContent, modelOverrides = null, // follows GSD. Keep those knobs coupled unless GSD also pins the model. if (hasPinnedModel) { const _universalEffortCodex = resolveInstallTimeEffort(effortCfg, resolvedName !== agentName ? resolvedName : agentName); - const _renderedEffortCodex = _getGsdEffortCatalog().renderEffortForRuntime('codex', _universalEffortCodex).value; - lines.push(`model_reasoning_effort = ${JSON.stringify(_renderedEffortCodex)}`); + // #3533 (10d): 'inherit' means OMIT the pin — the agent follows the host's + // own effort default. Never write the literal. + if (_universalEffortCodex !== 'inherit') { + const _renderedEffortCodex = _getGsdEffortCatalog().renderEffortForRuntime('codex', _universalEffortCodex).value; + lines.push(`model_reasoning_effort = ${JSON.stringify(_renderedEffortCodex)}`); + } } // #774 — Emit service_tier and model_verbosity for light-tier agents. @@ -11255,8 +11259,13 @@ function install(isGlobal, runtime = DEFAULT_RUNTIME, options = {}) { const _effortCfg = readGsdEffectiveEffortConfig(targetDir); const _agentName = entry.name.replace(/\.md$/, ''); const _universalEffort = resolveInstallTimeEffort(_effortCfg, _agentName); - const _renderedEffort = _getGsdEffortCatalog().renderEffortForRuntime(runtime, _universalEffort).value; - content = injectEffortFrontmatter(content, _renderedEffort); + // #3533 (10d): 'inherit' means the effort: key must NOT exist — + // Claude Code then follows the session effort. The canonical source + // agents carry no effort key, so skipping injection is the whole job. + if (_universalEffort !== 'inherit') { + const _renderedEffort = _getGsdEffortCatalog().renderEffortForRuntime(runtime, _universalEffort).value; + content = injectEffortFrontmatter(content, _renderedEffort); + } const _disallowedTools = READONLY_AGENT_DISALLOWED_TOOLS[_agentName]; if (_disallowedTools) content = injectDisallowedToolsFrontmatter(content, _disallowedTools); } diff --git a/docs/CONFIGURATION.md b/docs/CONFIGURATION.md index cbaf11ac1..a187be6d3 100644 --- a/docs/CONFIGURATION.md +++ b/docs/CONFIGURATION.md @@ -1414,7 +1414,14 @@ The model-catalog's `reasoning_effort` per-tier hint is a legacy field kept for | `effort.routing_tier_defaults.heavy` | enum | `"xhigh"` | Effort for heavy-tier agents (deep reasoning). | | `effort.agent_overrides.` | enum | (none) | Per-agent effort override. Beats tier defaults. | -Valid effort values: `minimal`, `low`, `medium`, `high`, `xhigh`, `max`. +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 +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. `query resolve-execution --json` reports two effort views ([#3534](https://github.com/open-gsd/gsd-core/issues/3534)): `effort` is the **resolved** config-cascade value; `effort_effective` is what the installed diff --git a/src/commands.cts b/src/commands.cts index 41e4bf13c..17fdb9fb7 100644 --- a/src/commands.cts +++ b/src/commands.cts @@ -106,7 +106,8 @@ interface CommitToSubrepoRepoResult { interface EffortSyncChange { agent: string; from: string | null; - to: string; + // #3533 (10d): to === null is the typed IR for omission (inherit strips the key). + to: string | null; } // ─── Phase Status ───────────────────────────────────────────────────────────── @@ -744,6 +745,28 @@ function setEffortFrontmatter(content: string, effortValue: string): string { return content.slice(0, closingStart) + `effort: ${effortValue}${eol}` + content.slice(closingStart); } +/** + * #3533 (10d) — remove exactly the frontmatter `effort:` 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. + */ +function removeEffortFrontmatter(content: 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, + // 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; + if (!lineRe.test(fmBody)) return content; + const strippedFm = fmBody.replace(lineRe, ''); + const openLen = 3 + (/^---\r\n/.test(content) ? 2 : 1); + const closingStart = match.index + openLen + fmBody.length; + return content.slice(0, match.index + openLen) + strippedFm + content.slice(closingStart); +} + /** * #488 — Re-sync effort: frontmatter in all installed gsd-*.md agent files to * match the current effort config, without requiring a full reinstall. @@ -811,6 +834,25 @@ function cmdEffortSync(cwd: string, raw: boolean, opts?: { dryRun?: boolean; con // Resolve using install-time logic: home defaults merged with project config. const universalEffort = resolveInstallTimeEffort(effortCfg, agentName); + + // #3533 (10d): 'inherit' means the key must NOT exist. An absent key is + // the CORRECT state (in sync, skipped) — before #3533 absence read as null + // 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; } + const effortMatchInherit = /^effort:[ \t]*(.+?)[ \t]*$/m.exec(fmMatchInherit[1]); + if (!effortMatchInherit) { skipped++; continue; } + changes.push({ agent: agentName, from: effortMatchInherit[1], to: null }); + synced++; + if (!dryRun) { + fs.writeFileSync(filePath, removeEffortFrontmatter(content)); + } + continue; + } + const rendered = renderEffortForRuntime(runtime, universalEffort); const newEffortValue = rendered.value; diff --git a/src/model-catalog.cts b/src/model-catalog.cts index bdfb48580..f68b14198 100644 --- a/src/model-catalog.cts +++ b/src/model-catalog.cts @@ -325,6 +325,13 @@ export function renderEffortArgv( * Render a universal effort string for a specific runtime. */ export function renderEffortForRuntime(runtime: string, universalEffort: string): RenderedEffort { + // #3533 (10d): 'inherit' is not a wire level on ANY runtime — it means + // "omit the key / pass no argument and follow the session/host default". + // Renderers must never emit it as a literal; null param/channel tells + // resolve-execution consumers there is no propagation. + if (universalEffort === 'inherit') { + return { value: 'inherit', param: null, channel: null }; + } const spec = EFFORT_RENDERING[runtime]; if (!spec) { return { value: universalEffort, param: null, channel: null }; diff --git a/src/model-resolver.cts b/src/model-resolver.cts index 3b21ca03c..63cbbe11a 100644 --- a/src/model-resolver.cts +++ b/src/model-resolver.cts @@ -744,7 +744,12 @@ function resolveProviderEscalation( // ─── #443 — Unified effort + fast_mode resolvers ───────────────────────────── const VALID_EFFORTS = ['minimal', 'low', 'medium', 'high', 'xhigh', 'max']; -const EFFORT_SET = new Set(VALID_EFFORTS); +// #3533 (10d): the VOCABULARY carries one more member than the LADDER — +// 'inherit' is a declarable effort choice ("follow the session", expressed by +// OMITTING the effort key at the writer) but not a level nextEffort may step +// into. Keeping it out of VALID_EFFORTS means escalation (resolveEffortForTier) +// never walks past an explicit inherit: nextEffort('inherit') is null. +const EFFORT_SET = new Set([...VALID_EFFORTS, 'inherit']); /** * Walk one step up the effort ladder from `e`. diff --git a/tests/commands.test.cjs b/tests/commands.test.cjs index 82af45739..45e9e2711 100644 --- a/tests/commands.test.cjs +++ b/tests/commands.test.cjs @@ -3726,6 +3726,85 @@ description: Executes GSD phase plans Body of the agent. `; +describe('#3533 effort sync: inherit means the key must not exist', () => { + test('10d: sync does not re-add a hand-stripped key when inherit is configured', () => { + const tmpDir = makeTmpDir('effort-sync-inherit-absent-'); + const agentsDir = makeAgentsDir(tmpDir); + fs.writeFileSync(path.join(agentsDir, 'gsd-executor.md'), AGENT_WITHOUT_EFFORT); + // Tier standard -> inherit. + writePlanningConfig(tmpDir, { routing_tier_defaults: { light: 'high', standard: 'inherit', heavy: 'xhigh' } }); + + const { cmdEffortSync } = require('../gsd-core/bin/lib/commands.cjs'); + const result = captureOutput(() => + cmdEffortSync(tmpDir, false, { dryRun: false, configDir: tmpDir, runtime: 'claude' }) + ); + + assert.equal(result.synced, 0, `absent key + inherit is IN SYNC, not drift: ${JSON.stringify(result.changes)}`); + assert.equal(result.changes.length, 0, 'no change may be reported for an absent key under inherit'); + const after = fs.readFileSync(path.join(agentsDir, 'gsd-executor.md'), 'utf8'); + assert.ok(!/^effort:/m.test(after), 'the effort: key must NOT be re-added'); + + cleanup(tmpDir); + }); + + test('10d: sync strips the key when inherit is configured and a value is present', () => { + const tmpDir = makeTmpDir('effort-sync-inherit-strip-'); + const agentsDir = makeAgentsDir(tmpDir); + // Fixture carries its own name so the survivor assertion below is + // satisfiable (AGENT_WITH_EFFORT names gsd-planner — wrong file). + fs.writeFileSync(path.join(agentsDir, 'gsd-executor.md'), AGENT_WITH_EFFORT.replace('name: gsd-planner', 'name: gsd-executor')); + writePlanningConfig(tmpDir, { default: 'inherit' }); + + const { cmdEffortSync } = require('../gsd-core/bin/lib/commands.cjs'); + const result = captureOutput(() => + cmdEffortSync(tmpDir, false, { dryRun: false, configDir: tmpDir, runtime: 'claude' }) + ); + + assert.equal(result.synced, 1); + assert.equal(result.changes[0].agent, 'gsd-executor'); + assert.equal(result.changes[0].from, 'medium'); + assert.equal(result.changes[0].to, null, 'to: null is the typed IR for omission'); + const after = fs.readFileSync(path.join(agentsDir, 'gsd-executor.md'), 'utf8'); + assert.ok(!/^effort:/m.test(after), 'the effort: line must be stripped'); + assert.ok(after.includes('name: gsd-executor'), 'every other frontmatter line survives'); + assert.ok(after.includes('Body of the agent.'), 'the body survives'); + + cleanup(tmpDir); + }); + + test('10d: strip preserves CRLF files and leaves comments and sibling keys intact', () => { + const tmpDir = makeTmpDir('effort-sync-inherit-crlf-'); + const agentsDir = makeAgentsDir(tmpDir); + const crlfAgent = [ + '---', + 'name: gsd-executor', + '# a hand comment that must survive', + 'effort: high', + 'description: Executes GSD phase plans', + '---', + 'Body.', + '', + ].join('\r\n'); + const agentPath = path.join(agentsDir, 'gsd-executor.md'); + fs.writeFileSync(agentPath, crlfAgent); + writePlanningConfig(tmpDir, { agent_overrides: { 'gsd-executor': 'inherit' } }); + + const { cmdEffortSync } = require('../gsd-core/bin/lib/commands.cjs'); + const result = captureOutput(() => + cmdEffortSync(tmpDir, false, { dryRun: false, configDir: tmpDir, runtime: 'claude' }) + ); + + assert.equal(result.synced, 1, `expected one strip: ${JSON.stringify(result.changes)}`); + const after = fs.readFileSync(agentPath, 'utf8'); + assert.ok(!/^effort:/m.test(after), 'effort line gone'); + assert.ok(after.includes('\r\n'), 'CRLF endings preserved'); + assert.ok(after.includes('# a hand comment that must survive'), 'comment preserved'); + assert.ok(/^description: Executes GSD phase plans\r?$/m.test(after), 'sibling key preserved'); + + cleanup(tmpDir); + }); +}); + describe('feat-488: effort sync command', () => { test('dry-run mode reports pending changes without writing files', () => { const tmpDir = makeTmpDir('effort-sync-dry-'); diff --git a/tests/effort-surface-axis.test.cjs b/tests/effort-surface-axis.test.cjs index 938dfbb5f..d1ce1652a 100644 --- a/tests/effort-surface-axis.test.cjs +++ b/tests/effort-surface-axis.test.cjs @@ -409,6 +409,23 @@ describe('#2481 live path — resolve-execution carries invocation-time effort', }); }); +describe('#3533 inherit renders no host argv argument', () => { + test('a project configuring inherit resolves effort inherit and renders NO argv', (t2) => { + const dir = createTempProject(); + t2.after(() => cleanup(dir)); + fs.writeFileSync( + path.join(dir, '.planning', 'config.json'), + JSON.stringify({ effort: { agent_overrides: { 'gsd-planner': 'inherit' } } }, null, 2), + ); + const out = JSON.parse( + runGsdTools('query resolve-execution gsd-planner --host claude', dir).output, + ); + assert.equal(out.effort, 'inherit'); + assert.deepEqual(out.effort_argv, [], 'inherit must render no argument'); + assert.equal(out.effort_propagation, null); + }); +}); + describe('#2481 — the escalation surface renders argv (CLI-level, not a workflow claim)', () => { // NAMING IS DELIBERATE. This exercises `resolve-execution --attempt` directly, // which is the CLI surface ADR-443's blocker explicitly EXCLUDES when it asks diff --git a/tests/install-runtime-artifacts.test.cjs b/tests/install-runtime-artifacts.test.cjs index eece01b34..8da221010 100644 --- a/tests/install-runtime-artifacts.test.cjs +++ b/tests/install-runtime-artifacts.test.cjs @@ -4040,6 +4040,61 @@ describe('#443 resolveInstallTimeEffort: invalid tokens fall through to valid ef }); }); +// ─── describe 5d: #3533 (10d) — inherit omits the effort key at install ────── + +describe('#3533 inherit: install writes NO effort key when the agent resolves to inherit', () => { + let tmpDir; + let claudeHome; + let codexHome; + + beforeEach(() => { + tmpDir = makeTmpDir('gsd-3533-inherit-'); + const projectDir = path.join(tmpDir, 'project'); + claudeHome = path.join(projectDir, '.claude'); + codexHome = path.join(projectDir, '.codex'); + fs.mkdirSync(claudeHome, { recursive: true }); + fs.mkdirSync(codexHome, { recursive: true }); + fs.mkdirSync(path.join(projectDir, '.planning'), { recursive: true }); + }); + + afterEach(() => { + cleanup(tmpDir); + }); + + function writeProjectConfig(config) { + const projectDir = path.dirname(claudeHome); + fs.writeFileSync( + path.join(projectDir, '.planning', 'config.json'), + JSON.stringify(config, null, 2) + ); + } + + test('claude .md frontmatter has no effort: key for an inherit-resolving agent', () => { + writeProjectConfig({ effort: { agent_overrides: { 'gsd-planner': 'inherit' } } }); + runGlobalInstall('claude', claudeHome); + const fm = readFrontmatter(path.join(claudeHome, 'agents', 'gsd-planner.md')); + assert.doesNotMatch(fm, /^effort:/m, + `an inherit-resolving agent must carry NO effort key\nActual:\n${fm}`); + // A non-inherit agent still gets its concrete value. + const fmExecutor = readFrontmatter(path.join(claudeHome, 'agents', 'gsd-executor.md')); + const m = fmExecutor.match(/^effort:\s*(\S+)$/m); + assert.ok(m && m[1] === 'high', `gsd-executor keeps its concrete tier value, got: ${m && m[1]}`); + }); + + test('codex .toml omits model_reasoning_effort for an inherit-resolving pinned agent', () => { + writeProjectConfig({ + runtime: 'codex', + model_overrides: { 'gsd-planner': 'gpt-5.6-sol' }, + effort: { agent_overrides: { 'gsd-planner': 'inherit' } }, + }); + runGlobalInstall('codex', codexHome); + const toml = fs.readFileSync(path.join(codexHome, 'agents', 'gsd-planner.toml'), 'utf8'); + assert.match(toml, /^model\s*=\s*"gpt-5.6-sol"$/m, 'the model pin stays'); + assert.doesNotMatch(toml, /^model_reasoning_effort\s*=/m, + `inherit must not pin a literal effort level\nActual:\n${toml.slice(0, 400)}`); + }); +}); + // ─── describe 5: Source stays clean ────────────────────────────────────────── describe('#443 Source purity: agents/gsd-planner.md has no effort: key', () => { diff --git a/tests/model-resolver.test.cjs b/tests/model-resolver.test.cjs index d56ef896a..07946a190 100644 --- a/tests/model-resolver.test.cjs +++ b/tests/model-resolver.test.cjs @@ -290,13 +290,63 @@ describe('resolveEffortInternal', () => { test('VALID_EFFORTS and EFFORT_SET are consistent', () => { assert.ok(Array.isArray(VALID_EFFORTS)); assert.ok(EFFORT_SET instanceof Set); - assert.strictEqual(EFFORT_SET.size, VALID_EFFORTS.length); + // #3533 (10d): the VOCABULARY (EFFORT_SET) carries one more member than + // the LADDER (VALID_EFFORTS) — 'inherit' is a declarable effort choice + // but not a level nextEffort may step into. + assert.strictEqual(EFFORT_SET.size, VALID_EFFORTS.length + 1); + assert.ok(EFFORT_SET.has('inherit'), "EFFORT_SET must accept 'inherit'"); + assert.ok(!VALID_EFFORTS.includes('inherit'), "the escalation ladder must NOT contain 'inherit'"); for (const e of VALID_EFFORTS) { assert.ok(EFFORT_SET.has(e), `EFFORT_SET missing: ${e}`); } }); }); +// ─── #3533 (10d): effort inheritance ────────────────────────────────────────── + +describe('#3533 effort inherit: expressible at every layer, never a wire level', () => { + let tmpDir; + beforeEach(() => { tmpDir = makeTempProject(); }); + afterEach(() => { cleanup(tmpDir); }); + + test('inherit accepted at every cascade layer (runtime)', () => { + writeConfig(tmpDir, { effort: { agent_overrides: { 'gsd-executor': 'inherit' } } }); + assert.strictEqual(resolveEffortInternal(tmpDir, 'gsd-executor'), 'inherit'); + + writeConfig(tmpDir, { effort: { routing_tier_defaults: { heavy: 'inherit' } } }); + assert.strictEqual(resolveEffortInternal(tmpDir, 'gsd-planner'), 'inherit'); + + writeConfig(tmpDir, { effort: { default: 'inherit' } }); + assert.strictEqual(resolveEffortInternal(tmpDir, 'completely-unknown-agent-xyz'), 'inherit'); + // A tiered agent under an inherit tier default also inherits (tier layer won). + assert.strictEqual(resolveEffortInternal(tmpDir, 'gsd-planner'), 'inherit'); + + assert.strictEqual(resolveEffortInternal(tmpDir, 'gsd-executor', { override: 'inherit' }), 'inherit'); + }); + + test('explicit inherit does not escalate', () => { + writeConfig(tmpDir, { + effort: { routing_tier_defaults: { heavy: 'inherit' } }, + dynamic_routing: { enabled: true, escalate_on_failure: true, max_escalations: 3 }, + }); + assert.strictEqual(resolveEffortForTier(tmpDir, 'gsd-planner', 2), 'inherit'); + }); + + test('renderEffortForRuntime inherit never yields a wire level', () => { + const { renderEffortForRuntime } = require('../gsd-core/bin/lib/model-catalog.cjs'); + for (const runtime of ['claude', 'codex', 'something-unknown']) { + const r = renderEffortForRuntime(runtime, 'inherit'); + assert.strictEqual(r.value, 'inherit', `${runtime}: value`); + assert.strictEqual(r.param, null, `${runtime}: param`); + assert.strictEqual(r.channel, null, `${runtime}: channel`); + } + // Concrete levels unchanged. + assert.strictEqual(renderEffortForRuntime('claude', 'minimal').value, 'low'); + assert.strictEqual(renderEffortForRuntime('codex', 'max').value, 'xhigh'); + assert.strictEqual(renderEffortForRuntime('claude', 'xhigh').value, 'xhigh'); + }); +}); + // ─── nextEffort ──────────────────────────────────────────────────────────────── describe('nextEffort', () => {