From 3fad7225855080a328f009f3b8d211f28496108d Mon Sep 17 00:00:00 2001 From: garrytan Date: Thu, 8 Oct 2026 01:46:35 +0000 Subject: [PATCH] fix: reject malformed model policies and keep model pins portable --- bin/gstack-codex-probe | 2 +- bin/gstack-config | 2 +- docs/model-policy.md | 7 +++++- docs/troubleshooting.md | 3 +++ lib/model-policy-cli.ts | 5 +++-- lib/model-policy.ts | 6 +++--- scripts/resolve-codex-generation-model.ts | 2 +- test/model-policy-cli.test.ts | 26 +++++++++++++++++++++++ test/model-policy.test.ts | 22 ++++++++++++++++++- test/model-role-invocation.test.ts | 10 +++++++++ 10 files changed, 75 insertions(+), 10 deletions(-) diff --git a/bin/gstack-codex-probe b/bin/gstack-codex-probe index ac3032470..1f71a6cad 100755 --- a/bin/gstack-codex-probe +++ b/bin/gstack-codex-probe @@ -146,7 +146,7 @@ _gstack_codex_select_model() { # Codex config.toml (review_model first for native review), gstack default. # With $3 = plan-review the plan-review policy ranks the choice instead and # its record carries role, tier, source and source-aware repair; $4 names the - # target repository whose project settings can route to a custom endpoint. + # working-directory context forwarded to the shared role resolver. # The TOML is read by the TypeScript resolver; bash never parses it. local _kind="${1:-exec}" _explicit="${2:-}" _role="${3:-}" _cwd="${4:-}" _line _rc _rest case "$_kind" in exec|review) ;; *) echo "ERROR: _gstack_codex_select_model takes exec or review" >&2; return 2 ;; esac diff --git a/bin/gstack-config b/bin/gstack-config index 72b91ff2c..5a7ca4d94 100755 --- a/bin/gstack-config +++ b/bin/gstack-config @@ -710,7 +710,7 @@ case "${1:-}" in case "$KEY" in plan_review_tier) case "$VALUE" in frontier|smart|host) ;; *) _mp_valid="" ; _mp_rule="Valid values: frontier, smart, host." ;; esac ;; implementation_tier) case "$VALUE" in frontier|smart) ;; *) _mp_valid="" ; _mp_rule="Valid values: frontier, smart." ;; esac ;; - *_claude) printf '%s' "$VALUE" | LC_ALL=C grep -qE '^[][A-Za-z0-9._:/@+-]{1,512}$' || { _mp_valid=""; _mp_rule="An Anthropic model ID is 1-512 characters from A-Z a-z 0-9 . _ : / @ + [ ] -."; } ;; + *_claude) { [ "${#VALUE}" -le 512 ] && printf '%s' "$VALUE" | LC_ALL=C grep -qE '^[][A-Za-z0-9._:/@+-]+$'; } || { _mp_valid=""; _mp_rule="An Anthropic model ID is 1-512 characters from A-Z a-z 0-9 . _ : / @ + [ ] -."; } ;; *_openai) printf '%s' "$VALUE" | LC_ALL=C grep -qE '^[A-Za-z0-9._:/-]{1,100}$' || { _mp_valid=""; _mp_rule="An OpenAI model ID is 1-100 characters from A-Z a-z 0-9 . _ : / -."; } ;; esac if [ -z "$VALUE" ]; then diff --git a/docs/model-policy.md b/docs/model-policy.md index f5f50befc..18f8341b6 100644 --- a/docs/model-policy.md +++ b/docs/model-policy.md @@ -64,6 +64,10 @@ uses the existing [gstack state root](state-root.md), not a new per-repository settings file. Invalid writes are rejected and leave the previous value intact; use `unset`, not an empty value, to restore a default. +A malformed known setting (for example `plan_review_tier = smart`) stops +selection rather than acting as an absent key. Correct or remove the named line +in the reported config file; `unset` only removes correctly spelled `key:` records. + Codex plan-review readiness and dispatch share one deadline. Readiness is budgeted half the remaining time, with the existing termination grace. A slow preflight leaves less time for the review instead of extending the caller's @@ -107,7 +111,8 @@ is not proof of account availability. See [provider recovery](troubleshooting.md Detection is conservative, not a replacement for the CLI's settings engine. Claude checks environment, user settings, project/shared/local settings, local settings at the repository and worktree's main checkout, and local managed files. -Codex checks environment, user/active-profile settings and system configuration. +Codex checks environment, user/active-profile settings and system configuration; +project-local Codex config is not inspected. Any detected local custom-route signal requires an explicit model, even if native trust or precedence rules might suppress it. Server-managed or MDM policy and extra native CLI flags are not visible to this check; `--help` and diff --git a/docs/troubleshooting.md b/docs/troubleshooting.md index 08fd9651c..7744b12d2 100644 --- a/docs/troubleshooting.md +++ b/docs/troubleshooting.md @@ -52,6 +52,9 @@ setting: doing that could silently select the premium default. `smart`. Use `gstack-config unset ` to restore a default, not an empty value. A rejected `set` leaves the old value intact. Correct ownership/read permissions rather than deleting an unrelated state directory. +For a malformed line such as `plan_review_tier = smart`, edit the reported file +and correct or remove that line first: `unset` only removes canonical `key:` +records and cannot repair a misspelled delimiter or quoted key. **Expected result.** `gstack-models list` or the corresponding `resolve` command explains the winning source. No outside model call ran on this error. diff --git a/lib/model-policy-cli.ts b/lib/model-policy-cli.ts index ca51b9ac3..120c6a6fa 100644 --- a/lib/model-policy-cli.ts +++ b/lib/model-policy-cli.ts @@ -60,13 +60,14 @@ Settings (gstack-config set|unset ): model_frontier_claude, model_frontier_openai, model_smart_claude, model_smart_openai (default: shipped catalog ID) -A custom Anthropic or OpenAI endpoint (Bedrock, Vertex, Foundry, a base-URL +A detected custom Anthropic or OpenAI endpoint (Bedrock, Vertex, Foundry, a base-URL override or a Codex model_provider) never receives a shipped public ID: set a per-tier model, an environment override (plan-review only) or host mode. Detection reads the environment, the env block of Claude's user, project (.claude/settings.json, .claude/settings.local.json in the working directory and repository root) and managed settings files, and Codex's user and system -config.toml. It cannot see server-managed or MDM policy, claude --settings, or +config.toml. Project-local Codex config is not inspected. +It cannot see server-managed or MDM policy, claude --settings, or codex -c/--profile flags: if you route those to a custom endpoint, set a per-tier model or host mode. diff --git a/lib/model-policy.ts b/lib/model-policy.ts index b3cbbdb0e..02351563f 100644 --- a/lib/model-policy.ts +++ b/lib/model-policy.ts @@ -216,14 +216,14 @@ export function parseModelPolicyConfig(text: string, configPath: string): ModelP for (const key of POLICY_CONFIG_KEYS) { if (line.startsWith(`${key}:`)) { records.set(key, line.slice(key.length + 1).trim()); - } else if (new RegExp(`^\\s*${key}\\s*:`).test(line)) { + } else if (new RegExp(`^\\s*(?:-\\s+)?["']?${key}(?=[^A-Za-z0-9_@]|$)`).test(line)) { throw new ModelPolicyError({ reason: 'config_malformed', problem: `${configPath} has a malformed ${key} record`, cause: `write it as '${key}: ' at the start of the line, with no indentation or space before the colon`, key, source: 'config', - repair: [`gstack-config unset ${key}`, `gstack-config set ${key} `], + repair: [`edit ${configPath} and remove or correct the malformed ${key} line`, `then use gstack-config set ${key} `], docs: DOCS_CONFIG, }); } @@ -320,7 +320,7 @@ export interface CustomProviderDetection { export const CUSTOM_PROVIDER_UNSCANNED: Readonly> = Object.freeze({ anthropic: Object.freeze(['server-managed settings from the claude.ai console', 'MDM or OS policy (macOS profile, Windows registry)', 'a --settings flag passed to claude']), - openai: Object.freeze(['cloud-managed Codex config', '-c, --config or --profile flags passed to codex']), + openai: Object.freeze(['project-local Codex config.toml', 'cloud-managed Codex config', '-c, --config or --profile flags passed to codex']), }); function claudeManagedDir(platform: NodeJS.Platform): string { diff --git a/scripts/resolve-codex-generation-model.ts b/scripts/resolve-codex-generation-model.ts index faef7a8f1..5c298fdf0 100644 --- a/scripts/resolve-codex-generation-model.ts +++ b/scripts/resolve-codex-generation-model.ts @@ -200,7 +200,7 @@ export function resolveCodexPlanReviewModel(opts: { env?: Record; codexHome?: string; home?: string; - /** Target repository; its project settings can route to a custom endpoint. */ + /** Invocation context forwarded to the shared role resolver. */ cwd?: string; }): CodexPlanReviewSelection { const env = opts.env ?? process.env; diff --git a/test/model-policy-cli.test.ts b/test/model-policy-cli.test.ts index 9f0a4f339..3b06b5a0d 100644 --- a/test/model-policy-cli.test.ts +++ b/test/model-policy-cli.test.ts @@ -230,6 +230,32 @@ describe('gstack-models inspector', () => { }); describe('gstack-config model policy keys', () => { + test('Anthropic pins preserve the 512-character contract with a POSIX-bounded regex engine', () => { + const { root, env, state } = fixture(); + const bin = path.join(root, 'portable-bin'); + fs.mkdirSync(bin); + fs.writeFileSync(path.join(bin, 'grep'), `#!/usr/bin/env bun +import { spawnSync } from 'node:child_process'; +const args = process.argv.slice(2); +if (args.some(arg => [...arg.matchAll(/\\{\\d+,(\\d+)\\}/g)].some(match => Number(match[1]) > 255))) { + console.error('REG_BADMAX: repetition exceeds the BSD RE_DUP_MAX of 255'); + process.exit(2); +} +const result = spawnSync(${JSON.stringify(Bun.which('grep'))}, args, { stdio: 'inherit', timeout: 5000 }); +process.exit(result.status ?? 1); +`, { mode: 0o755 }); + const portable = { ...env, PATH: `${bin}${path.delimiter}${env.PATH}` }; + for (const key of ['model_frontier_claude', 'model_smart_claude']) { + for (const value of ['claude-fable-5-1', 'a'.repeat(512)]) { + expect(config(['set', key, value], portable).code).toBe(0); + expect(config(['get', key], portable).stdout).toBe(value); + } + const before = fs.readFileSync(path.join(state, 'config.yaml'), 'utf-8'); + expect(config(['set', key, 'a'.repeat(513)], portable).code).toBe(1); + expect(fs.readFileSync(path.join(state, 'config.yaml'), 'utf-8')).toBe(before); + } + }); + test('defaults, set, resolve again and unset restore', () => { const { env } = fixture(); expect(config(['get', 'plan_review_tier'], env)).toMatchObject({ code: 0, stdout: 'frontier' }); diff --git a/test/model-policy.test.ts b/test/model-policy.test.ts index cfe0be024..919c0d325 100644 --- a/test/model-policy.test.ts +++ b/test/model-policy.test.ts @@ -179,7 +179,12 @@ describe('strict policy config snapshot', () => { const error = policyError(() => parseModelPolicyConfig(text, '/c.yaml')); expect(error.reason).toBe(reason as ModelPolicyError['reason']); expect(error.key).toBe(key); - expect(error.repair.join('\n')).toContain(`gstack-config unset ${key}`); + if (reason === 'config_malformed') { + expect(error.repair.join('\n')).toContain(`edit /c.yaml and remove or correct the malformed ${key} line`); + expect(error.repair.join('\n')).not.toContain('gstack-config unset'); + } else { + expect(error.repair.join('\n')).toContain(`gstack-config unset ${key}`); + } expect(error.docs).toContain('troubleshooting.md#model-policy-config'); for (const step of error.repair) expect(step).not.toMatch(/[|]/); }); @@ -189,6 +194,21 @@ describe('strict policy config snapshot', () => { expect(policyError(() => resolvePlanReviewModel({ provider: 'openai', env })).reason).toBe('invalid_model_id'); }); + test('recognizable policy records never become absent keys because of malformed delimiters', () => { + const keys = ['plan_review_tier', 'implementation_tier', 'model_frontier_claude', 'model_frontier_openai', 'model_smart_claude', 'model_smart_openai']; + for (const key of keys) { + const value = key.startsWith('model_') ? 'custom-model' : 'smart'; + for (const line of [`${key} = ${value}`, `${key} ${value}`, `${key}; ${value}`, key, `"${key}": ${value}`, `- ${key}: ${value}`]) { + const error = policyError(() => parseModelPolicyConfig(`${line}\n`, '/owned/config.yaml')); + expect(error).toMatchObject({ reason: 'config_malformed', source: 'config', key }); + expect(error.repair).toContain(`edit /owned/config.yaml and remove or correct the malformed ${key} line`); + } + } + const absent = parseModelPolicyConfig('# plan_review_tier = smart\nlegacy_key = value\nplan_review_tier_notes: unrelated\nbrain_trust_policy@local: shared\n', '/owned/config.yaml'); + expect(absent.planReviewTier).toMatchObject({ value: 'frontier', origin: 'default' }); + expect(parseModelPolicyConfig('plan_review_tier: host\nplan_review_tier: smart\n', '/owned/config.yaml').planReviewTier).toMatchObject({ value: 'smart', origin: 'config' }); + }); + test('a directory, an unreadable file or a file state root is an error, never the default', () => { const dir = fixture(); fs.mkdirSync(path.join(dir.state, 'config.yaml')); diff --git a/test/model-role-invocation.test.ts b/test/model-role-invocation.test.ts index f2d1bdb38..b54b9bfdd 100644 --- a/test/model-role-invocation.test.ts +++ b/test/model-role-invocation.test.ts @@ -257,6 +257,16 @@ describe('Codex plan-review invocation binds one selection', () => { expect(custom.stderr).not.toContain('NOTICE: gstack plan reviews'); }); + test('malformed known policy records stop both provider invocations before any CLI dispatch', () => { + for (const host of ['claude', 'codex'] as const) { + const r = run(host, home('plan_review_tier = smart\n')); + expect(r.status).not.toBe(0); + expect(r.calls).toEqual([]); + expect(r.stdout + r.stderr).toContain('remove or correct the malformed plan_review_tier line'); + expect(r.stderr).not.toContain(NOTICE); + } + }); + test('a rejected role model is unusable with a source-aware repair and never dispatches', () => { const r = run('claude', home(), { FAKE_PROBE: 'model400' }); expect(r.status).toBe(1);