mirror of
https://github.com/garrytan/gstack.git
synced 2026-11-05 16:34:00 +01:00
fix: reject malformed model policies and keep model pins portable
This commit is contained in:
1 parent
3daba9a20f
commit
3fad722585
10 files changed
+75
-10
No files matched your search
@@ -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
|
||||
|
||||
+1
-1
@@ -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
|
||||
|
||||
@@ -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
|
||||
|
||||
@@ -52,6 +52,9 @@ setting: doing that could silently select the premium default.
|
||||
`smart`. Use `gstack-config unset <key>` 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.
|
||||
|
||||
@@ -60,13 +60,14 @@ Settings (gstack-config set|unset <key>):
|
||||
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.
|
||||
|
||||
|
||||
+3
-3
@@ -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}: <value>' 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} <value>`],
|
||||
repair: [`edit ${configPath} and remove or correct the malformed ${key} line`, `then use gstack-config set ${key} <value>`],
|
||||
docs: DOCS_CONFIG,
|
||||
});
|
||||
}
|
||||
@@ -320,7 +320,7 @@ export interface CustomProviderDetection {
|
||||
|
||||
export const CUSTOM_PROVIDER_UNSCANNED: Readonly<Record<ModelProvider, readonly string[]>> = 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 {
|
||||
|
||||
@@ -200,7 +200,7 @@ export function resolveCodexPlanReviewModel(opts: {
|
||||
env?: Record<string, string | undefined>;
|
||||
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;
|
||||
|
||||
@@ -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' });
|
||||
|
||||
@@ -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'));
|
||||
|
||||
@@ -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);
|
||||
|
||||
Reference in new issue
Block a user