mirror of
https://github.com/QwenLM/qwen-code.git
synced 2026-07-09 17:19:02 +00:00
Round-1 audit (3 angles × line-by-line + removed-behavior + cross-file) plus an open-ended design pass surfaced eight correctness issues. This commit lands all of them; the larger ACP / serve-mode structural items are documented for follow-up. Correctness fixes - headlessSafetyWarnings: `SANDBOX` env check reverted to plain truthy. The sandbox transport sets `SANDBOX` to `sandbox-exec` (macOS seatbelt) or the container name (`qwen-code-sandbox`), neither of which matches `isTruthyEnv`. The PR's strict-`1`/`true` check was emitting the "no sandbox" warning INSIDE real sandboxes. Match the rest of the codebase (sandboxConfig.ts, gemini.tsx, Footer.tsx, prompts.ts, …) which all treat any non-empty value as "sandboxed". - nonInteractiveCli main-loop abort: add `finalizeAssistantMessage()` before `routeAbort()`. The drain-item loop already had it (PR #4502 Critical bug #1); the main loop was asymmetric — stream-json consumers would see an unterminated `message_start` when a budget / SIGINT abort landed mid-stream. - nonInteractiveCli drain-loop `routeAbort`: also flush `flushQueuedNotificationsToSdk(localQueue)` and `finalizeOneShotMonitors()` before exiting. The old `return`-and- fall-through path went through the outer holdback loop, which did this flushing; switching to `routeAbort()` skipped it, so `task_started` envelopes lost their paired `task_notification`. - nonInteractiveCli catch handler: emit `adapter.emitResult({...})` BEFORE `handleBudgetExceededError`, with the budget message as `errorMessage` when budget tripped. Previously the budget handler `process.exit(55)`ed before the adapter could emit a terminal `result` envelope, so STREAM_JSON consumers never saw a stream terminator on budget exits and hung waiting for one. - runBudget: new `validateMaxToolCalls` mirrors `validateMaxWallTimeSetting`. yargs coerces non-numeric flag values (`--max-tool-calls abc`) to `NaN`, and the enforcer's `>= 0` gate treats `NaN` and negatives as "no limit", silently disabling the budget. Reject `NaN`, `Infinity`, fractional, and negative-other- than-`-1` values at both flag and settings layers. `0` remains legal (`first tick aborts`), unlike wall-time where 0 is fatal. - runBudget: new `MIN_WALL_TIME_SECONDS = 1` floor. Previously `--max-wall-time 500ms` parsed cleanly and aborted on the next event-loop tick before any model round-trip — almost certainly a typo (`5m`?) and not a useful guardrail at any rate. - nonInteractiveCli `tickToolCall`: exempt `ToolNames.STRUCTURED_OUTPUT`. Under `--json-schema` this is the terminal "I'm done" contract tool, not real work. Without the exemption a budget-edge completion is aborted as a false positive (model used N tools then emitted structured_output as call N+1 → exit 55 instead of success). - commands/serve.ts: emit the YOLO-no-sandbox warning at daemon startup when settings.json statically configures `tools.approvalMode: 'yolo'` with no `tools.sandbox` / `SANDBOX` env. The daemon can't use `getHeadlessYoloSafetyWarning` (no Config yet — sessions get their own) so we re-derive the predicate from settings. Per-session ACP override is documented as out of scope. Documentation - `docs/users/features/headless.md`: new "Scope" subsection under Run-level budgets explaining (a) `--max-tool-calls` counts top-level dispatches only — subagent / `agent` tool inner calls are not counted, (b) `structured_output` is exempt, (c) stream-json input mode resets budgets per user message, (d) `qwen serve` / ACP sessions do not currently consult budgets from settings.json. Tests - `runBudget.test.ts` grows from 32 → 41 tests: `validateMaxToolCalls` (NaN / Infinity / negatives / fractional), `parseDurationSeconds` sub-second rejection, `validateMaxWallTimeSetting` sub-second rejection. - `headlessSafetyWarnings.test.ts`: replaced the "still warns when SANDBOX is 0/false/no" case (which encoded the strict-check bug) with positive coverage for the real sandbox-set values (`sandbox-exec`, `qwen-code-sandbox`). All previously-green suites still green: cli/nonInteractiveCli (46), cli/gemini.test (23), cli/config/config.test (220), core/utils/errors (12), core/config/config.test (172). 337 tests across the touched suites. Won't-fix (out of scope, documented or pre-existing) - Unpaired `tool_use` in stream-json when a tool is aborted mid-execution — pre-existing structural gap (SIGINT mid-tool has the same outcome); PR amplifies it but doesn't introduce it. - Narrow SIGINT-vs-budget-timer race — already mitigated by `markExceeded`'s `signal.aborted` check. - `tickToolCall` increments past abort (cosmetic; only affects the `observed` value in the error envelope for a pathological caller).
This commit is contained in:
克竟
parent
3abc7d6b76
commit
40ae6dd0ff
8 changed files with 243 additions and 29 deletions
|
|
@ -252,11 +252,18 @@ Headless / CI runs combined with `--yolo` (or `--approval-mode=yolo`) auto-appro
|
|||
|
||||
Qwen Code can abort an unattended run when it crosses one of the following thresholds. Each is `-1` (unlimited) by default; setting any one is enough to bound runaway behavior. They are enforced cooperatively against the same `AbortController` that already carries SIGINT, so a budget abort emits a structured `FatalBudgetExceededError` (exit code **55**) — distinct from the turn-cap exit code 53 and SIGINT's 130 so CI scripts can branch on the reason.
|
||||
|
||||
| Flag | Settings key | What it bounds |
|
||||
| --------------------- | -------------------------- | ------------------------------------------------------------------------------------------------------------- |
|
||||
| `--max-wall-time` | `model.maxWallTimeSeconds` | Wall-clock duration of the whole run. Flag accepts `90` (s), `30s`, `5m`, `1h`, `500ms`. Settings is seconds. |
|
||||
| `--max-tool-calls` | `model.maxToolCalls` | Cumulative tool calls executed (counts successes _and_ failures — the model still consumes tokens on errors). |
|
||||
| `--max-session-turns` | `model.maxSessionTurns` | Number of user/model/tool turns; pre-existing. Exits with code 53 on overrun (distinct from budget exit 55). |
|
||||
| Flag | Settings key | What it bounds |
|
||||
| --------------------- | -------------------------- | ------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------- |
|
||||
| `--max-wall-time` | `model.maxWallTimeSeconds` | Wall-clock duration of the whole run. Flag accepts `90` (s), `30s`, `5m`, `1h`, `500ms`. Settings is seconds. |
|
||||
| `--max-tool-calls` | `model.maxToolCalls` | Cumulative top-level tool calls dispatched by the main run loop (counts successes _and_ failures — the model still consumes tokens on errors). See "Scope" below for subagent / structured-output exemptions. |
|
||||
| `--max-session-turns` | `model.maxSessionTurns` | Number of user/model/tool turns; pre-existing. Exits with code 53 on overrun (distinct from budget exit 55). |
|
||||
|
||||
#### Scope
|
||||
|
||||
- **`--max-tool-calls` counts top-level dispatches only.** When the model calls the `agent` tool, the dispatch counts as **1**; inner tool calls performed by the spawned subagent are **not** counted. A model that funnels work through subagents can do unbounded inner work under a small top-level budget. Combine with `--exclude-tools agent` if you need a tighter cap.
|
||||
- **`structured_output` is exempt.** Under `--json-schema`, the model's terminal `structured_output` call is the "I'm done" contract, not real work — it doesn't count against `--max-tool-calls` so a budget-edge completion isn't aborted as a false positive.
|
||||
- **Single-shot vs `--input-format stream-json`:** in stream-json input mode the daemon resets the budget counters at the start of every user message; the budget is per-message, not per-process.
|
||||
- **`qwen serve` / ACP sessions:** the daemon ACP session path does NOT currently consult `--max-wall-time` / `--max-tool-calls` from settings.json. These budgets only apply to single-shot `qwen -p` runs and to `--input-format stream-json` sessions. (`qwen serve` does emit the YOLO-no-sandbox warning at boot if `tools.approvalMode: 'yolo'` is set in settings.)
|
||||
|
||||
### Recommended combinations
|
||||
|
||||
|
|
|
|||
|
|
@ -13,6 +13,8 @@ import type { Argv, CommandModule } from 'yargs';
|
|||
import { writeStderrLine } from '../utils/stdioHelpers.js';
|
||||
import { DEFAULT_RING_SIZE } from '../serve/eventBus.js';
|
||||
import { MCP_BUDGET_WARN_FRACTION } from '@qwen-code/qwen-code-core';
|
||||
import { loadSettings } from '../config/settings.js';
|
||||
import { HEADLESS_YOLO_NO_SANDBOX_WARNING } from '../utils/headlessSafetyWarnings.js';
|
||||
|
||||
/**
|
||||
* Pause the current async function indefinitely. Used after the daemon
|
||||
|
|
@ -203,6 +205,32 @@ export const serveCommand: CommandModule<unknown, ServeArgs> = {
|
|||
);
|
||||
}
|
||||
|
||||
// Emit the headless-YOLO safety warning at daemon startup if
|
||||
// settings.json statically configures yolo + no sandbox. We can't
|
||||
// use `getHeadlessYoloSafetyWarning(config)` here because the daemon
|
||||
// hasn't constructed a `Config` yet — sessions get their own — so
|
||||
// we re-derive the predicate from the same settings.json the
|
||||
// sessions will load. Per-session override (the ACP client flipping
|
||||
// approval mode mid-session) is out of scope here; this warns about
|
||||
// a deployment that's wide-open at boot. Suppress with
|
||||
// QWEN_CODE_SUPPRESS_YOLO_WARNING=1.
|
||||
try {
|
||||
const loaded = loadSettings(argv.workspace ?? process.cwd());
|
||||
const merged = loaded.merged;
|
||||
const approvalMode = merged.tools?.approvalMode;
|
||||
const sandbox = merged.tools?.sandbox;
|
||||
const sandboxEnv = process.env['SANDBOX'];
|
||||
const suppress = process.env['QWEN_CODE_SUPPRESS_YOLO_WARNING'];
|
||||
const suppressed = suppress === '1' || suppress === 'true';
|
||||
if (approvalMode === 'yolo' && !sandbox && !sandboxEnv && !suppressed) {
|
||||
writeStderrLine(HEADLESS_YOLO_NO_SANDBOX_WARNING);
|
||||
}
|
||||
} catch {
|
||||
// Settings load can fail (corrupt JSON, etc.); don't block
|
||||
// daemon startup just to emit a warning — the existing settings
|
||||
// path will report the same error to the user via Session.
|
||||
}
|
||||
|
||||
// Lazy-load the serve module so non-serve invocations don't pay for
|
||||
// express + body-parser + qs in their startup path.
|
||||
const { runQwenServe } = await import('../serve/index.js');
|
||||
|
|
|
|||
|
|
@ -77,6 +77,7 @@ import { isWorkspaceTrusted } from './trustedFolders.js';
|
|||
import { writeStderrLine } from '../utils/stdioHelpers.js';
|
||||
import {
|
||||
parseDurationSeconds,
|
||||
validateMaxToolCalls,
|
||||
validateMaxWallTimeSetting,
|
||||
} from '../utils/runBudget.js';
|
||||
|
||||
|
|
@ -1162,6 +1163,35 @@ function resolveMaxWallTimeSeconds(argv: CliArgs, settings: Settings): number {
|
|||
return -1;
|
||||
}
|
||||
|
||||
/**
|
||||
* Resolves the tool-call budget for a run. Returns the validated count
|
||||
* (`-1` = unlimited). Order of precedence: `--max-tool-calls` flag, then
|
||||
* `model.maxToolCalls` from settings, else unlimited.
|
||||
*
|
||||
* Symmetric with `resolveMaxWallTimeSeconds`: yargs accepts `NaN` from
|
||||
* non-numeric flag values, and the enforcer's `>= 0` gate would silently
|
||||
* disable the budget for `NaN` / negatives. Validate up front so a typo
|
||||
* in a CI guardrail fails loudly.
|
||||
*/
|
||||
function resolveMaxToolCalls(argv: CliArgs, settings: Settings): number {
|
||||
if (argv.maxToolCalls !== undefined && argv.maxToolCalls !== null) {
|
||||
try {
|
||||
return validateMaxToolCalls(argv.maxToolCalls);
|
||||
} catch (err) {
|
||||
throw new Error(`--max-tool-calls: ${(err as Error).message}`);
|
||||
}
|
||||
}
|
||||
const fromSettings = settings.model?.maxToolCalls;
|
||||
if (typeof fromSettings === 'number') {
|
||||
try {
|
||||
return validateMaxToolCalls(fromSettings);
|
||||
} catch (err) {
|
||||
throw new Error(`settings.json: ${(err as Error).message}`);
|
||||
}
|
||||
}
|
||||
return -1;
|
||||
}
|
||||
|
||||
export function isDebugMode(argv: CliArgs): boolean {
|
||||
return (
|
||||
argv.debug ||
|
||||
|
|
@ -1778,7 +1808,7 @@ export async function loadCliConfig(
|
|||
maxSessionTurns:
|
||||
argv.maxSessionTurns ?? settings.model?.maxSessionTurns ?? -1,
|
||||
maxWallTimeSeconds: resolveMaxWallTimeSeconds(argv, settings),
|
||||
maxToolCalls: argv.maxToolCalls ?? settings.model?.maxToolCalls ?? -1,
|
||||
maxToolCalls: resolveMaxToolCalls(argv, settings),
|
||||
experimentalZedIntegration: argv.acp || argv.experimentalAcp || false,
|
||||
cronEnabled: settings.experimental?.cron ?? false,
|
||||
emitToolUseSummaries: settings.experimental?.emitToolUseSummaries ?? true,
|
||||
|
|
|
|||
|
|
@ -656,7 +656,15 @@ export async function runNonInteractive(
|
|||
// at exactly N executions: the (N+1)th tick aborts before the
|
||||
// tool runs. Ticking after would let the (N+1)th tool execute
|
||||
// and only then abort. See issue #4103.
|
||||
budgetEnforcer.tickToolCall();
|
||||
//
|
||||
// Exempt `structured_output`: under --json-schema this is the
|
||||
// terminal "I'm done" contract tool, not real work. Counting
|
||||
// it would abort an otherwise-valid completion at the budget
|
||||
// edge (e.g. budget=3, model used 3 tools then emits
|
||||
// structured_output as call #4 → exit 55 instead of success).
|
||||
if (requestInfo.name !== ToolNames.STRUCTURED_OUTPUT) {
|
||||
budgetEnforcer.tickToolCall();
|
||||
}
|
||||
if (abortController.signal.aborted) await routeAbort();
|
||||
const toolResponse = await executeToolCall(
|
||||
config,
|
||||
|
|
@ -794,6 +802,11 @@ export async function runNonInteractive(
|
|||
|
||||
for await (const event of responseStream) {
|
||||
if (abortController.signal.aborted) {
|
||||
// Pair the startAssistantMessage() above so stream-json mode
|
||||
// doesn't leave an unterminated message_start when a budget /
|
||||
// SIGINT abort lands mid-stream. Symmetric with the drain-item
|
||||
// loop fix below.
|
||||
adapter.finalizeAssistantMessage();
|
||||
await routeAbort();
|
||||
}
|
||||
// Use adapter for all event processing
|
||||
|
|
@ -914,7 +927,17 @@ export async function runNonInteractive(
|
|||
// final drain item surfaces as exit code 55 instead of
|
||||
// being silently swallowed by the outer success path
|
||||
// (drain-loop fall-through; see issue #4103 review).
|
||||
//
|
||||
// Also flush queued task notifications and finalize
|
||||
// one-shot monitors here. Previously this site used a
|
||||
// bare `return` and let control fall through to the
|
||||
// outer holdback loop, which did the flushing before
|
||||
// exiting; routing through `routeAbort` skips that
|
||||
// path, so we re-do it inline to preserve the
|
||||
// task_started↔task_notification pairing invariant.
|
||||
adapter.finalizeAssistantMessage();
|
||||
flushQueuedNotificationsToSdk(localQueue);
|
||||
finalizeOneShotMonitors();
|
||||
await routeAbort();
|
||||
}
|
||||
adapter.processEvent(event);
|
||||
|
|
@ -1181,16 +1204,20 @@ export async function runNonInteractive(
|
|||
|
||||
// If a run-level budget tripped during an awaited stream / tool
|
||||
// call, the underlying fetch's AbortError lands here before our
|
||||
// explicit `routeAbort` sites can fire. Re-route through the
|
||||
// budget handler so the user sees the friendly "Run aborted: …"
|
||||
// envelope (exit 55) instead of a raw "AbortError" line.
|
||||
// explicit `routeAbort` sites can fire. Capture the reason so we
|
||||
// can (a) include the friendly "Run aborted: …" message in the
|
||||
// adapter's terminal result envelope (STREAM_JSON consumers
|
||||
// depend on that envelope to close the stream cleanly) and (b)
|
||||
// exit with the budget handler's exit code 55 instead of the
|
||||
// generic `handleError` exit code 1 from a raw "AbortError".
|
||||
const budgetExceeded = budgetEnforcer.getExceeded();
|
||||
if (budgetExceeded) {
|
||||
await handleBudgetExceededError(config, budgetExceeded);
|
||||
}
|
||||
|
||||
// For JSON and STREAM_JSON modes, compute usage from metrics
|
||||
const message = error instanceof Error ? error.message : String(error);
|
||||
const message = budgetExceeded
|
||||
? budgetExceeded.message
|
||||
: error instanceof Error
|
||||
? error.message
|
||||
: String(error);
|
||||
const metrics = uiTelemetryService.getMetrics();
|
||||
const usage = computeUsageFromMetrics(metrics);
|
||||
// Get stats for JSON format output
|
||||
|
|
@ -1221,6 +1248,11 @@ export async function runNonInteractive(
|
|||
stats,
|
||||
});
|
||||
}
|
||||
if (budgetExceeded) {
|
||||
// Always exit AFTER emitResult so STREAM_JSON / JSON consumers
|
||||
// see a terminal result envelope before the process dies.
|
||||
await handleBudgetExceededError(config, budgetExceeded);
|
||||
}
|
||||
await handleError(error, config);
|
||||
} finally {
|
||||
// Cancel the wall-clock timer so it doesn't fire after a successful
|
||||
|
|
|
|||
|
|
@ -44,19 +44,29 @@ describe('getHeadlessYoloSafetyWarning', () => {
|
|||
expect(getHeadlessYoloSafetyWarning(cfg, {})).toBeNull();
|
||||
});
|
||||
|
||||
it('does not warn when SANDBOX env is strictly truthy', () => {
|
||||
it('does not warn when SANDBOX env is set to the value the sandbox transport actually writes', () => {
|
||||
const cfg = makeConfig(ApprovalMode.YOLO, undefined);
|
||||
// macOS seatbelt
|
||||
expect(
|
||||
getHeadlessYoloSafetyWarning(cfg, { SANDBOX: 'sandbox-exec' }),
|
||||
).toBeNull();
|
||||
// Docker / Podman container name
|
||||
expect(
|
||||
getHeadlessYoloSafetyWarning(cfg, { SANDBOX: 'qwen-code-sandbox' }),
|
||||
).toBeNull();
|
||||
// Generic truthy values
|
||||
expect(getHeadlessYoloSafetyWarning(cfg, { SANDBOX: '1' })).toBeNull();
|
||||
expect(getHeadlessYoloSafetyWarning(cfg, { SANDBOX: 'true' })).toBeNull();
|
||||
});
|
||||
|
||||
it('still warns when SANDBOX env is a non-truthy value like 0 / false / no', () => {
|
||||
it('warns when SANDBOX env is unset or empty string', () => {
|
||||
const cfg = makeConfig(ApprovalMode.YOLO, undefined);
|
||||
for (const val of ['0', 'false', 'no']) {
|
||||
expect(getHeadlessYoloSafetyWarning(cfg, { SANDBOX: val })).toBe(
|
||||
HEADLESS_YOLO_NO_SANDBOX_WARNING,
|
||||
);
|
||||
}
|
||||
expect(getHeadlessYoloSafetyWarning(cfg, {})).toBe(
|
||||
HEADLESS_YOLO_NO_SANDBOX_WARNING,
|
||||
);
|
||||
expect(getHeadlessYoloSafetyWarning(cfg, { SANDBOX: '' })).toBe(
|
||||
HEADLESS_YOLO_NO_SANDBOX_WARNING,
|
||||
);
|
||||
});
|
||||
|
||||
it('respects the explicit suppression env var when set to 1 or true', () => {
|
||||
|
|
|
|||
|
|
@ -31,10 +31,14 @@ export function getHeadlessYoloSafetyWarning(
|
|||
): string | null {
|
||||
if (config.getApprovalMode() !== ApprovalMode.YOLO) return null;
|
||||
if (config.getSandbox()) return null;
|
||||
// SANDBOX is set by the sandbox transport itself (container / seatbelt
|
||||
// wrapper). Use the same strict truthy check as the suppression env to
|
||||
// avoid `SANDBOX=false` / `SANDBOX=0` accidentally silencing the warning.
|
||||
if (isTruthyEnv(env['SANDBOX'])) return null;
|
||||
// `SANDBOX` is set by the sandbox transport itself: macOS seatbelt sets
|
||||
// it to `sandbox-exec`, Docker/Podman to the container name (e.g.
|
||||
// `qwen-code-sandbox`). Match the rest of the codebase
|
||||
// (sandboxConfig.ts, gemini.tsx, Footer.tsx, prompts.ts, …) which all
|
||||
// treat any non-empty value as "inside a sandbox". A strict 1/true
|
||||
// check here misfires inside real sandboxes, where the helper would
|
||||
// wrongly emit a "no sandbox" warning despite the run being contained.
|
||||
if (env['SANDBOX']) return null;
|
||||
if (isTruthyEnv(env['QWEN_CODE_SUPPRESS_YOLO_WARNING'])) return null;
|
||||
return HEADLESS_YOLO_NO_SANDBOX_WARNING;
|
||||
}
|
||||
|
|
|
|||
|
|
@ -8,6 +8,7 @@ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest';
|
|||
import {
|
||||
RunBudgetEnforcer,
|
||||
parseDurationSeconds,
|
||||
validateMaxToolCalls,
|
||||
validateMaxWallTimeSetting,
|
||||
} from './runBudget.js';
|
||||
|
||||
|
|
@ -19,8 +20,8 @@ describe('parseDurationSeconds', () => {
|
|||
[' 45 ', 45],
|
||||
['5m', 300],
|
||||
['1h', 3600],
|
||||
['500ms', 0.5],
|
||||
['1.5h', 5400],
|
||||
['1s', 1],
|
||||
])('parses %s as %d seconds', (input, expected) => {
|
||||
expect(parseDurationSeconds(input)).toBeCloseTo(expected);
|
||||
});
|
||||
|
|
@ -32,8 +33,18 @@ describe('parseDurationSeconds', () => {
|
|||
},
|
||||
);
|
||||
|
||||
it('rejects sub-second budgets — they fire before any model round-trip', () => {
|
||||
// Previously a tiny budget like `500ms` parsed cleanly and immediately
|
||||
// aborted the run on the next event-loop tick. That's a typo, not a
|
||||
// useful guardrail.
|
||||
expect(() => parseDurationSeconds('500ms')).toThrow(/minimum/i);
|
||||
expect(() => parseDurationSeconds('1ms')).toThrow(/minimum/i);
|
||||
expect(() => parseDurationSeconds('0.5')).toThrow(/minimum/i);
|
||||
});
|
||||
|
||||
it('rejects values larger than Node.js can safely time out on', () => {
|
||||
// 100 days in seconds is well above MAX_TIMEOUT_MS / 1000 (~24.8d).
|
||||
// The regex doesn't accept `d`, so `100d` fails as a format error;
|
||||
// `2400h` parses but exceeds MAX_WALL_TIME_SECONDS (~24.8d).
|
||||
expect(() => parseDurationSeconds('100d')).toThrow();
|
||||
expect(() => parseDurationSeconds('2400h')).toThrow();
|
||||
});
|
||||
|
|
@ -44,15 +55,20 @@ describe('validateMaxWallTimeSetting', () => {
|
|||
expect(validateMaxWallTimeSetting(-1)).toBe(-1);
|
||||
});
|
||||
|
||||
it('accepts positive numbers', () => {
|
||||
it('accepts positive numbers at or above the 1s floor', () => {
|
||||
expect(validateMaxWallTimeSetting(60)).toBe(60);
|
||||
expect(validateMaxWallTimeSetting(0.5)).toBeCloseTo(0.5);
|
||||
expect(validateMaxWallTimeSetting(1)).toBe(1);
|
||||
});
|
||||
|
||||
it('rejects 0 (mirrors CLI flag behavior — 0 is a foot-gun)', () => {
|
||||
expect(() => validateMaxWallTimeSetting(0)).toThrow();
|
||||
});
|
||||
|
||||
it('rejects sub-second values', () => {
|
||||
expect(() => validateMaxWallTimeSetting(0.5)).toThrow(/minimum/i);
|
||||
expect(() => validateMaxWallTimeSetting(0.001)).toThrow(/minimum/i);
|
||||
});
|
||||
|
||||
it('rejects negatives other than -1', () => {
|
||||
expect(() => validateMaxWallTimeSetting(-2)).toThrow();
|
||||
});
|
||||
|
|
@ -69,6 +85,47 @@ describe('validateMaxWallTimeSetting', () => {
|
|||
});
|
||||
});
|
||||
|
||||
describe('validateMaxToolCalls', () => {
|
||||
it('accepts -1 (unlimited sentinel)', () => {
|
||||
expect(validateMaxToolCalls(-1)).toBe(-1);
|
||||
});
|
||||
|
||||
it('accepts 0 (no-tool-calls-allowed sentinel)', () => {
|
||||
// Asymmetric with wall-time where 0 is fatal — for tool-calls, 0 means
|
||||
// "the first tick aborts", which is a legitimate "model must answer
|
||||
// without invoking tools" mode.
|
||||
expect(validateMaxToolCalls(0)).toBe(0);
|
||||
});
|
||||
|
||||
it('accepts positive integers', () => {
|
||||
expect(validateMaxToolCalls(5)).toBe(5);
|
||||
expect(validateMaxToolCalls(1000)).toBe(1000);
|
||||
});
|
||||
|
||||
it('rejects NaN — yargs coerces non-numeric flag values to NaN', () => {
|
||||
// `qwen -p '...' --max-tool-calls abc` would otherwise silently
|
||||
// disable the budget; the >= 0 gate in tickToolCall is false for NaN.
|
||||
expect(() => validateMaxToolCalls(Number.NaN)).toThrow();
|
||||
});
|
||||
|
||||
it('rejects Infinity', () => {
|
||||
expect(() => validateMaxToolCalls(Number.POSITIVE_INFINITY)).toThrow();
|
||||
});
|
||||
|
||||
it('rejects negatives other than -1', () => {
|
||||
// `--max-tool-calls=-5` (typo for `5`) would otherwise silently
|
||||
// disable the budget — the exact foot-gun the wall-time validator
|
||||
// was built to prevent.
|
||||
expect(() => validateMaxToolCalls(-5)).toThrow();
|
||||
expect(() => validateMaxToolCalls(-2)).toThrow();
|
||||
});
|
||||
|
||||
it('rejects fractional values', () => {
|
||||
expect(() => validateMaxToolCalls(1.5)).toThrow();
|
||||
expect(() => validateMaxToolCalls(0.5)).toThrow();
|
||||
});
|
||||
});
|
||||
|
||||
describe('RunBudgetEnforcer', () => {
|
||||
beforeEach(() => {
|
||||
vi.useFakeTimers();
|
||||
|
|
|
|||
|
|
@ -55,6 +55,14 @@ const SECOND = 1000;
|
|||
*/
|
||||
const MAX_TIMEOUT_MS = 2_147_483_647;
|
||||
const MAX_WALL_TIME_SECONDS = Math.floor(MAX_TIMEOUT_MS / SECOND);
|
||||
/**
|
||||
* Wall-clock budgets below 1s are almost always a typo (someone meant `1m`
|
||||
* or `1h`); accepting them silently produces a run that aborts on the next
|
||||
* event-loop tick before any model request returns. Round-trip latency to
|
||||
* any reasonable LLM is multiple seconds, so a sub-second budget is also
|
||||
* not a meaningful guardrail. Reject loudly.
|
||||
*/
|
||||
const MIN_WALL_TIME_SECONDS = 1;
|
||||
|
||||
/**
|
||||
* Parses a duration string used by `--max-wall-time`.
|
||||
|
|
@ -110,6 +118,11 @@ export function parseDurationSeconds(input: string): number {
|
|||
`Invalid duration "${input}": must be greater than zero. Omit the flag entirely if you don't want a wall-clock budget.`,
|
||||
);
|
||||
}
|
||||
if (seconds < MIN_WALL_TIME_SECONDS) {
|
||||
throw new Error(
|
||||
`Invalid duration "${input}": below the ${MIN_WALL_TIME_SECONDS}s minimum (probably a typo — did you mean ${input.replace(/ms\b/i, 's')}?). Sub-second wall-clock budgets fire before any model round-trip can complete.`,
|
||||
);
|
||||
}
|
||||
if (seconds > MAX_WALL_TIME_SECONDS) {
|
||||
throw new Error(
|
||||
`Invalid duration "${input}": exceeds the maximum supported wall-clock budget (${MAX_WALL_TIME_SECONDS}s ≈ 24 days). Use a smaller value.`,
|
||||
|
|
@ -143,6 +156,11 @@ export function validateMaxWallTimeSetting(value: number): number {
|
|||
`Use -1 to disable, not 0.`,
|
||||
);
|
||||
}
|
||||
if (value < MIN_WALL_TIME_SECONDS) {
|
||||
throw new Error(
|
||||
`model.maxWallTimeSeconds ${value} is below the ${MIN_WALL_TIME_SECONDS}s minimum. Sub-second budgets fire before any model round-trip can complete.`,
|
||||
);
|
||||
}
|
||||
if (value > MAX_WALL_TIME_SECONDS) {
|
||||
throw new Error(
|
||||
`model.maxWallTimeSeconds ${value} exceeds the maximum supported wall-clock budget (${MAX_WALL_TIME_SECONDS}s ≈ 24 days).`,
|
||||
|
|
@ -151,6 +169,34 @@ export function validateMaxWallTimeSetting(value: number): number {
|
|||
return value;
|
||||
}
|
||||
|
||||
/**
|
||||
* Validates a `maxToolCalls` value sourced from either the `--max-tool-calls`
|
||||
* CLI flag or `model.maxToolCalls` in settings.json. Mirrors
|
||||
* `validateMaxWallTimeSetting`: the enforcer treats anything `< 0` as "no
|
||||
* limit", so any non-`-1` negative would silently disable the budget. Reject
|
||||
* up front to keep the fail-loud philosophy symmetric across all budgets.
|
||||
*
|
||||
* `0` IS legal here — it means "no tool calls allowed; first tick aborts"
|
||||
* (asymmetric with wall-time where 0 is fatal). Documented in the schema.
|
||||
*/
|
||||
export function validateMaxToolCalls(value: number): number {
|
||||
if (value === -1) return -1;
|
||||
if (!Number.isFinite(value)) {
|
||||
throw new Error(`maxToolCalls must be a finite number; got ${value}.`);
|
||||
}
|
||||
if (!Number.isInteger(value)) {
|
||||
throw new Error(
|
||||
`maxToolCalls must be an integer (or -1 for unlimited); got ${value}.`,
|
||||
);
|
||||
}
|
||||
if (value < 0) {
|
||||
throw new Error(
|
||||
`maxToolCalls must be >= 0 (or -1 for unlimited); got ${value}. Use -1 to disable, not a negative number.`,
|
||||
);
|
||||
}
|
||||
return value;
|
||||
}
|
||||
|
||||
export class RunBudgetEnforcer {
|
||||
private readonly maxWallTimeSeconds: number;
|
||||
private readonly maxToolCalls: number;
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue