Repository navigation
feat(run): enforce output schemas with bounded corrective retries (aictrl#5958) - #129
Conversation
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| } | ||
| console.error(cause) | ||
| console.error(schema ? "Structured output run failed" : cause) | ||
| process.exitCode = 1 |
There was a problem hiding this comment.
🟠 reject() overwrites structured-failure exit 3 with 1.
| process.exitCode = 1 | |
| In reject(), do not clobber the structured exit code: `if (process.exitCode !== OUTPUT_FAILED_EXIT) process.exitCode = 1` (or `process.exitCode ??= 1` semantics), so a 3 set by complete() or the session_error handler survives while genuine provider errors still get 1. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.ts:979-980):
Problem: reject() overwrites structured-failure exit 3 with 1
Detail: complete() sets process.exitCode = OUTPUT_FAILED_EXIT (3) guarded by `!process.exitCode`, but every reject() path (prompt/command `.catch(reject)` via throwOnError in attach mode, loopDone `.catch(reject)`) unconditionally executes `process.exitCode = 1` right after `await complete(error)`. When the run fails with a structured-output failure (e.g. a StructuredOutputError already set exit 3 via the session_error handler, then the events loop throws and routes through reject()), reject() overwrites 3 with 1, so the process exit code disagrees with the emitted terminal structured_output event and the documented 0/2/3 contract. Genuine provider errors should keep 1, so the fix must distinguish rather than clobber.
Suggested fix: In reject(), do not clobber the structured exit code: `if (process.exitCode !== OUTPUT_FAILED_EXIT) process.exitCode = 1` (or `process.exitCode ??= 1` semantics), so a 3 set by complete() or the session_error handler survives while genuine provider errors still get 1.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
complete() sets process.exitCode = OUTPUT_FAILED_EXIT (3) guarded by !process.exitCode, but every reject() path (prompt/command .catch(reject) via throwOnError in attach mode, loopDone .catch(reject)) unconditionally executes process.exitCode = 1 right after await complete(error). When the run fails with a structured-output failure (e.g. a StructuredOutputError already set exit 3 via the session_error handler, then the events loop throws and routes through reject()), reject() overwrites 3 with 1, so the process exit code disagrees with the emitted terminal structured_output event and the documented 0/2/3 contract. Genuine provider errors should keep 1, so the fix must distinguish rather than clobber.
| const lastThree = history.slice(-DOOM_LOOP_THRESHOLD) | ||
|
|
||
| if ( | ||
| !( |
There was a problem hiding this comment.
🟠 Doom-loop guard trusts model-controlled tool input.
| !( | |
| Exclude only tool parts with internal provenance: tag repaired calls with a server-side marker (synthetic flag or dedicated part type) instead of matching the model-controlled input.tool field; never branch a safety guard on unvalidated model input. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/processor.ts:236-240):
Problem: Doom-loop guard trusts model-controlled tool input
Detail: The doom-loop exclusion added for structured output keys on `value.input?.tool === "StructuredOutput"`, but `input` is model-submitted data: a model can repeatedly emit a tool call named "invalid" with `{tool:"StructuredOutput"}` and permanently suppress the repetition guard, enabling an unbounded tool-call loop (agent.steps defaults to Infinity) and unbounded LLM cost. Conversely, if tool parts store input as a JSON string, the exclusion never matches the PR's own repaired calls, so legitimate corrective retries can falsely trip the doom loop. A safety guard should not branch on unvalidated model input.
Suggested fix: Exclude only tool parts with internal provenance: tag repaired calls with a server-side marker (synthetic flag or dedicated part type) instead of matching the model-controlled input.tool field; never branch a safety guard on unvalidated model input.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The doom-loop exclusion added for structured output keys on value.input?.tool === "StructuredOutput", but input is model-submitted data: a model can repeatedly emit a tool call named "invalid" with {tool:"StructuredOutput"} and permanently suppress the repetition guard, enabling an unbounded tool-call loop (agent.steps defaults to Infinity) and unbounded LLM cost. Conversely, if tool parts store input as a JSON string, the exclusion never matches the PR's own repaired calls, so legitimate corrective retries can falsely trip the doom loop. A safety guard should not branch on unvalidated model input.
| ]) | ||
| }, 25000) | ||
|
|
||
| test("schema config NDJSON clips the specific compilation cause", async () => { |
There was a problem hiding this comment.
🟠 Last spawn test missing explicit 25s timeout.
--- a/packages/cli/test/cli/run-output-schema.test.ts
+++ b/packages/cli/test/cli/run-output-schema.test.ts
@@ -633,3 +633,3 @@
expect(event?.message).toStartWith("schema.json: strict mode: unknown keyword:")
expect((event?.message as string).length).toBe(300)
-})
+}, 25000)
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/test/cli/run-output-schema.test.ts:627-635):
Problem: Last spawn test missing explicit 25s timeout
Detail: The test "schema config NDJSON clips the specific compilation cause" spawns a full `bun run` child process (CLI boot + config load + Ajv compile) but, unlike every other spawning test in this file (all pass 25000), it omits the timeout argument, so it runs under bun:test's 5s default. On slow/loaded CI the child alone can exceed 5s, producing an unrelated timeout failure instead of testing the 300-char clipping contract.
Suggested fix: Append the sibling timeout: `}, 25000)`.
Suggested patch:
--- a/packages/cli/test/cli/run-output-schema.test.ts
+++ b/packages/cli/test/cli/run-output-schema.test.ts
@@ -633,3 +633,3 @@
expect(event?.message).toStartWith("schema.json: strict mode: unknown keyword:")
expect((event?.message as string).length).toBe(300)
-})
+}, 25000)
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The test "schema config NDJSON clips the specific compilation cause" spawns a full bun run child process (CLI boot + config load + Ajv compile) but, unlike every other spawning test in this file (all pass 25000), it omits the timeout argument, so it runs under bun:test's 5s default. On slow/loaded CI the child alone can exceed 5s, producing an unrelated timeout failure instead of testing the 300-char clipping contract.
| }), | ||
| ) | ||
| await Bun.write(path.join(tmp.path, "schema.json"), options.content ?? JSON.stringify(schema)) | ||
| if (options.locked) await fs.mkdir(path.join(tmp.path, "locked"), { mode: 0o555 }) |
There was a problem hiding this comment.
🟠 0o555 "unwritable parent" test fails when run as root.
| if (options.locked) await fs.mkdir(path.join(tmp.path, "locked"), { mode: 0o555 }) | |
| Make the case root-independent: skip when `process.getuid?.() === 0`, or replace the mode-bit fixture with a root-proof condition (parent is a file — already covered separately — or mock fs.access). |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/test/cli/run-output-schema.test.ts:106):
Problem: 0o555 "unwritable parent" test fails when run as root
Detail: The "unwritable result parent" config-error case relies on `fs.mkdir(mode 0o555)` making the directory non-writable, but the implementation check (run.output.ts) uses `fs.access(dir, W_OK|X_OK)`. POSIX grants root W_OK on any directory regardless of mode bits, so when the suite runs as root (the default in most Docker/CI containers) the config check passes, the CLI proceeds to a model request, and the assertions `expect(result.exit).toBe(2)` and `expect(result.requests).toEqual([])` fail — the suite is red only because of the runner's uid.
Suggested fix: Make the case root-independent: skip when `process.getuid?.() === 0`, or replace the mode-bit fixture with a root-proof condition (parent is a file — already covered separately — or mock fs.access).
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The "unwritable result parent" config-error case relies on fs.mkdir(mode 0o555) making the directory non-writable, but the implementation check (run.output.ts) uses fs.access(dir, W_OK|X_OK). POSIX grants root W_OK on any directory regardless of mode bits, so when the suite runs as root (the default in most Docker/CI containers) the config check passes, the CLI proceeds to a model request, and the assertions expect(result.exit).toBe(2) and expect(result.requests).toEqual([]) fail — the suite is red only because of the runner's uid.
| code, | ||
| message: `Invocation failed during ${phase}`, | ||
| message: | ||
| code === "OUTPUT_SCHEMA_CONFIG" |
There was a problem hiding this comment.
🟡 OUTPUT_SCHEMA_CONFIG literal duplicated in 2 files.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.invocation.ts:38):
Problem: OUTPUT_SCHEMA_CONFIG literal duplicated in 2 files
Detail: run.ts passes the bare literal "OUTPUT_SCHEMA_CONFIG" to fail()/invocation.abort, and run.invocation.ts compares `code === "OUTPUT_SCHEMA_CONFIG"` to switch message behaviour. The protocol string is hardcoded in two modules with no shared constant, even though the sibling exit codes were properly exported from run.output.ts (OUTPUT_CONFIG_EXIT/OUTPUT_FAILED_EXIT). A typo in either copy silently reverts the config-error message to the generic "Invocation failed during ..." text documented in EVENTS.md.
Suggested fix: Export a shared constant (e.g. OUTPUT_SCHEMA_CONFIG_CODE alongside OUTPUT_CONFIG_EXIT in run.output.ts) and use it in both run.ts and the run.invocation.ts comparison.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
run.ts passes the bare literal "OUTPUT_SCHEMA_CONFIG" to fail()/invocation.abort, and run.invocation.ts compares code === "OUTPUT_SCHEMA_CONFIG" to switch message behaviour. The protocol string is hardcoded in two modules with no shared constant, even though the sibling exit codes were properly exported from run.output.ts (OUTPUT_CONFIG_EXIT/OUTPUT_FAILED_EXIT). A typo in either copy silently reverts the config-error message to the generic "Invocation failed during ..." text documented in EVENTS.md.
| @@ -0,0 +1,62 @@ | |||
| import path from "path" | |||
| import fs from "fs/promises" | |||
| import { OutputSchema } from "../../session/output-schema" | |||
There was a problem hiding this comment.
⚪ Relative import bypasses "@/session" alias.
| import { OutputSchema } from "../../session/output-schema" | |
| Use `import { OutputSchema } from "@/session/output-schema"` in both run.output.ts and run.ts to match generate.ts and the other cmd/ files. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.output.ts:3):
Problem: Relative import bypasses "@/session" alias
Detail: run.output.ts imports OutputSchema via "../../session/output-schema" and run.ts adds the same relative import, while the sibling command file generate.ts (same directory) uses the alias form "@/session/message-v2" and "@/provider/termination" for cross-directory imports. The new cross-module imports deviate from the established '@/' alias convention.
Suggested fix: Use `import { OutputSchema } from "@/session/output-schema"` in both run.output.ts and run.ts to match generate.ts and the other cmd/ files.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
run.output.ts imports OutputSchema via "../../session/output-schema" and run.ts adds the same relative import, while the sibling command file generate.ts (same directory) uses the alias form "@/session/message-v2" and "@/provider/termination" for cross-directory imports. The new cross-module imports deviate from the established '@/' alias convention.
| OUTPUT_CONFIG_EXIT, | ||
| ) | ||
| return undefined | ||
| }) |
There was a problem hiding this comment.
⚪ Unreachable return after process.exit in fail().
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.ts:425):
Problem: Unreachable return after process.exit in fail()
Detail: The outputSchema catch handler awaits fail(...) and then returns undefined, but fail() ends with process.exit(OUTPUT_CONFIG_EXIT), which never returns, so the `return undefined` (and downstream `schema`-is-undefined handling) is dead code. Neighbouring failure paths in this file carry no post-exit returns; the unreachable fallback implies execution continues without a schema, which never happens.
Suggested fix: Type the catch as never (e.g. `.catch(async (error: unknown): Promise<never> => { await fail(...); throw error })`), or make fail() return a discriminated result instead of process.exit so the undefined path is genuinely reachable.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The outputSchema catch handler awaits fail(...) and then returns undefined, but fail() ends with process.exit(OUTPUT_CONFIG_EXIT), which never returns, so the return undefined (and downstream schema-is-undefined handling) is dead code. Neighbouring failure paths in this file carry no post-exit returns; the unreachable fallback implies execution continues without a schema, which never happens.
| }) | ||
| if (result && result.ok) { | ||
| await Filesystem.write(filepath, await result.text()) | ||
| .then((response) => (response.ok ? response.text() : undefined)) |
There was a problem hiding this comment.
⚪ Body-read failure mislabeled as fetch failure.
| .then((response) => (response.ok ? response.text() : undefined)) | |
| Split the handlers: catch on the fetch logs "Failed to fetch models.dev"; a separate catch around response.text() logs "Failed to read models.dev response". |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/provider/models.ts:113-117):
Problem: Body-read failure mislabeled as fetch failure
Detail: The single `.catch` now spans both the fetch and the `response.text()` read, logging "Failed to fetch models.dev" for body-read failures (e.g. connection reset mid-transfer after a successful response). The fetch itself succeeded, so the log message points operators at the wrong failure stage.
Suggested fix: Split the handlers: catch on the fetch logs "Failed to fetch models.dev"; a separate catch around response.text() logs "Failed to read models.dev response".
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The single .catch now spans both the fetch and the response.text() read, logging "Failed to fetch models.dev" for body-read failures (e.g. connection reset mid-transfer after a successful response). The fetch itself succeeded, so the log message points operators at the wrong failure stage.
| // Inject StructuredOutput tool if JSON schema mode enabled | ||
| if (lastUser.format?.type === "json_schema") { | ||
| const reject = async (input: string | OutputSchema.Diagnostic[]) => { | ||
| if (!contract) throw new Error("Missing output schema") |
There was a problem hiding this comment.
⚪ reject(input) shadows outer prompt input param.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/prompt.ts:727):
Problem: reject(input) shadows outer prompt input param
Detail: The new `const reject = async (input: string | OutputSchema.Diagnostic[])` is declared inside `prompt = fn(PromptInput, async (input) => ...)`, shadowing the outer PromptInput parameter named `input` that is still live in the same scope (used earlier in the OutputSchema.compile preflight). The shadow invites copy-paste mistakes; the nearby `contract!.format.retryCount` non-null assertion also duplicates the guard that `if (!contract)` / `available` already establish.
Suggested fix: Rename the parameter (e.g. `reject(raw: string | OutputSchema.Diagnostic[])`) and rely on the already-narrowed `contract.format` instead of `contract!`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new const reject = async (input: string | OutputSchema.Diagnostic[]) is declared inside prompt = fn(PromptInput, async (input) => ...), shadowing the outer PromptInput parameter named input that is still live in the same scope (used earlier in the OutputSchema.compile preflight). The shadow invites copy-paste mistakes; the nearby contract!.format.retryCount non-null assertion also duplicates the guard that if (!contract) / available already establish.
| "export type Event =\n | EventSessionStructuredOutput\n | EventSessionStructuredOutputRejected\n", | ||
| ) | ||
| } | ||
| await legacy.write(source) |
There was a problem hiding this comment.
⚪ Event-union patch guard checks one of two members.
| await legacy.write(source) | |
| Guard on both inserted members (or the combined two-line snippet), and restore the "; update the SDK schema bridge" hint to the thrown errors. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/sdk/script/build.ts:79):
Problem: Event-union patch guard checks one of two members
Detail: The idempotency guard for the legacy Event union tests `source.includes(" | EventSessionStructuredOutput\n")` but the replacement inserts both EventSessionStructuredOutput and EventSessionStructuredOutputRejected. If the generated file ever contains only the first member (partial regeneration or future edit), the block silently re-inserts the Rejected member a second time. The new error strings also drop the old "update the SDK schema bridge" remediation hint the previous single-type patch carried, making build failures less actionable.
Suggested fix: Guard on both inserted members (or the combined two-line snippet), and restore the "; update the SDK schema bridge" hint to the thrown errors.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The idempotency guard for the legacy Event union tests source.includes(" | EventSessionStructuredOutput\n") but the replacement inserts both EventSessionStructuredOutput and EventSessionStructuredOutputRejected. If the generated file ever contains only the first member (partial regeneration or future edit), the block silently re-inserts the Rejected member a second time. The new error strings also drop the old "update the SDK schema bridge" remediation hint the previous single-type patch carried, making build failures less actionable.
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 4 · 🟡 13 · ⚪ 7 · 0/24 resolved
🤖 Fix all 24 open findings with your agent📋 Out-of-diff findings (24)
Reviewed 24 files · 0 inline · view all 24 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Review response — PR #129Triage of the aictrl code review (24 findings) at head Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
|
reject() preserved exit 3 whenever process.exitCode already held it, but complete() sets 3 for any structured failure and runs first inside reject(). When the provider rejected the prompt before the SSE session.error arrived, a genuine provider failure therefore exited 3. Track the session-reported StructuredOutputError explicitly and key the exit code on that instead. The attach provider-rejection fixture now answers before sending any event, pinning the ordering that CI hit; reverting the fix makes it fail (1 vs 3). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
|
||
| export function compile(schema: Record<string, unknown>) { | ||
| if (schema.type !== "object") throw new Error('schema root must have type "object"') | ||
| const key = JSON.stringify(schema) |
There was a problem hiding this comment.
🟠 Unbounded client schema compiled with no size guard.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/output-schema.ts:24):
Problem: Unbounded client schema compiled with no size guard
Detail: OutputSchema.compile() accepts any schema with no size, depth or complexity bound, and it now sits on a trust boundary: prompt() compiles `input.format.schema` straight from PromptInput/CommandInput (`format: MessageV2.Format.optional()`), which any SDK/attach-server client can supply, and schemas are also persisted on user messages and re-compiled from storage on resume. The cache key is `JSON.stringify(schema)` over the raw object — every prompt-loop iteration re-serializes the whole schema even on cache hits, and the 32-entry cache retains arbitrarily large key strings (memory/CPU amplification). Ajv's own docs warn that compiling untrusted schemas is unsafe: deeply nested $ref chains can exhaust the stack during codegen and a pathological `pattern` compiles to a native RegExp that ReDoSes the session thread on every model-submitted argument validation (this ReDoS aspect was previously raised and remains unaddressed).
Suggested fix: Enforce a hard byte cap on the serialized schema before stringify/compile (e.g. reject when Buffer.byteLength(JSON.stringify(schema)) > ~64 KB with a config-style error), use a truncated SHA-256 of the serialized schema as the cache key instead of the raw string, and consider Ajv `code: { regExp }` with a safe engine (RE2-style) so pattern compilation cannot backtrack catastrophically.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
OutputSchema.compile() accepts any schema with no size, depth or complexity bound, and it now sits on a trust boundary: prompt() compiles input.format.schema straight from PromptInput/CommandInput (format: MessageV2.Format.optional()), which any SDK/attach-server client can supply, and schemas are also persisted on user messages and re-compiled from storage on resume. The cache key is JSON.stringify(schema) over the raw object — every prompt-loop iteration re-serializes the whole schema even on cache hits, and the 32-entry cache retains arbitrarily large key strings (memory/CPU amplification). Ajv's own docs warn that compiling untrusted schemas is unsafe: deeply nested $ref chains can exhaust the stack during codegen and a pathological pattern compiles to a native RegExp that ReDoSes the session thread on every model-submitted argument validation (this ReDoS aspect was previously raised and remains unaddressed).
export function compile(schema: Record<string, unknown>) {
if (schema.type !== "object") throw new Error('schema root must have type "object"')
const key = JSON.stringify(schema)
const cached = cache.get(key)
if (cached) return cached| if (lastUser.format?.type === "json_schema") { | ||
| const rejection = async (errors: OutputSchema.Diagnostic[]) => { | ||
| if (!contract) throw new Error("Missing output schema") | ||
| if (attempts < contract.format.retryCount + 1) { |
There was a problem hiding this comment.
🟠 Omitted retryCount makes budget NaN, breaks SDK runs.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/prompt.ts:728):
Problem: Omitted retryCount makes budget NaN, breaks SDK runs
Detail: The wire contract (packages/sdk/src/v2/gen/types.gen.ts in this PR) declares OutputFormat retryCount as optional (`retryCount?: number`), and prompt()/command() store client-supplied `format` verbatim (`format: input.format`) with no default. Every budget computation then does arithmetic on undefined: `attempts < contract.format.retryCount + 1` (prompt.ts:728) → `0 < NaN` → false, so `available` (prompt.ts:745) is false from the first step and onSuccess throws "StructuredOutput attempt budget exhausted" on the model's FIRST valid StructuredOutput call. `attempts = Math.min(attempts + 1, format.retryCount + 1)` yields NaN, and the `rejected > contract.format.retryCount` exhaustion check (prompt.ts:877) is never true, so the run can only end as missing/step_limit. Any SDK/attach client that sends `format: {type: "json_schema", schema}` without retryCount can never get an accepted result even with perfectly valid model output. The CLI flag path masks this because run.output.ts always fills `retryCount: input.retries ?? 2`.
Suggested fix: Normalize once where the contract is built (prompt.ts ~382-385): `contract = request.format?.type === "json_schema" ? { format: { ...request.format, retryCount: request.format.retryCount ?? 2 }, validate: OutputSchema.compile(request.format.schema) } : undefined` — or add a `.default(2)`/`.catch(2)` on the retryCount field of MessageV2.Format so every consumer (rejection(), available, onSuccess Math.min, exhaustion check, maxAttempts publish) sees a real number.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The wire contract (packages/sdk/src/v2/gen/types.gen.ts in this PR) declares OutputFormat retryCount as optional (retryCount?: number), and prompt()/command() store client-supplied format verbatim (format: input.format) with no default. Every budget computation then does arithmetic on undefined: attempts < contract.format.retryCount + 1 (prompt.ts:728) → 0 < NaN → false, so available (prompt.ts:745) is false from the first step and onSuccess throws "StructuredOutput attempt budget exhausted" on the model's FIRST valid StructuredOutput call. attempts = Math.min(attempts + 1, format.retryCount + 1) yields NaN, and the rejected > contract.format.retryCount exhaustion check (prompt.ts:877) is never true, so the run can only end as missing/step_limit. Any SDK/attach client that sends format: {type: "json_schema", schema} without retryCount can never get an accepted result even with perfectly valid model output. The CLI flag path masks this because run.output.ts always fills retryCount: input.retries ?? 2.
})
const rejection = async (errors: OutputSchema.Diagnostic[]) => {
if (!contract) throw new Error("Missing output schema")
if (attempts < contract.format.retryCount + 1) {
attempts++
rejected++| throw new Error(`${input.schema}: invalid JSON`) | ||
| } | ||
| })() | ||
| if (!schema || typeof schema !== "object" || Array.isArray(schema)) |
There was a problem hiding this comment.
🟡 Two distinct config errors share identical message.
--- a/packages/cli/src/cli/cmd/run.output.ts
+++ b/packages/cli/src/cli/cmd/run.output.ts
@@ -29,2 +29,2 @@
- throw new Error(`${input.schema}: schema root must have type "object"`)
+ throw new Error(`${input.schema}: schema root must be a JSON object`)
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.output.ts:29-30):
Problem: Two distinct config errors share identical message
Detail: run.output.ts:29-30 throws `"${input.schema}: schema root must have type \"object\""` when the parsed schema file is an array/scalar, and OutputSchema.compile (output-schema.ts:23) throws the byte-identical `'schema root must have type "object"'` when an object root lacks `type: "object"` — which run.output.ts:42 re-wraps into the exact same message. Two distinct failure conditions (non-object JSON vs object without type "object") produce identical configuration errors, so operators cannot distinguish them, and the first message is factually wrong (that check never inspected `type`).
Suggested fix: Use a distinct message for the JSON-shape check, e.g. `${input.schema}: schema root must be a JSON object`, and leave the `type: "object"` wording exclusively to the OutputSchema.compile failure path.
Suggested patch:
--- a/packages/cli/src/cli/cmd/run.output.ts
+++ b/packages/cli/src/cli/cmd/run.output.ts
@@ -29,2 +29,2 @@
- throw new Error(`${input.schema}: schema root must have type "object"`)
+ throw new Error(`${input.schema}: schema root must be a JSON object`)
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
run.output.ts:29-30 throws "${input.schema}: schema root must have type \"object\"" when the parsed schema file is an array/scalar, and OutputSchema.compile (output-schema.ts:23) throws the byte-identical 'schema root must have type "object"' when an object root lacks type: "object" — which run.output.ts:42 re-wraps into the exact same message. Two distinct failure conditions (non-object JSON vs object without type "object") produce identical configuration errors, so operators cannot distinguish them, and the first message is factually wrong (that check never inspected type).
const schema: unknown = (() => {
try {
return JSON.parse(text)
} catch {
throw new Error(`${input.schema}: invalid JSON`)
}
})()
if (!schema || typeof schema !== "object" || Array.isArray(schema))
throw new Error(`${input.schema}: schema root must have type "object"`)| abort() | ||
| await Shutdown.flush() | ||
| return | ||
| } |
There was a problem hiding this comment.
🟡 Early return leaves events loop running after flush.
| } | |
| Settle the loop before returning: `abort(); await Promise.race([loopDone, Shutdown.flush()])` (or break the events stream), or drop the early return and reuse the existing await loopDone / race-with-promptResult path so complete()/reject run exactly once. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.ts:1048-1052):
Problem: Early return leaves events loop running after flush
Detail: With --output-schema, a failed prompt/command request is swallowed via `request.catch(reject)` (reject runs complete() → emits structured_output failed + terminal lifecycle and sets the exit code), then the handler executes `abort(); await Shutdown.flush(); return` without awaiting loopDone. The events-consumption loop is still live: when the aborted session goes idle, loop() resolves and fires complete() a second time, and any structured_output_rejected/status events still in flight are emitted after the Shutdown.flush. If loop() itself rejects, its attached `.catch(reject)` re-runs reject, printing a duplicate `console.error(JSON.stringify(classified))` after the handler returned. NDJSON consumers can observe late/interleaved events or duplicated stderr racing process exit.
Suggested fix: Settle the loop before returning: `abort(); await Promise.race([loopDone, Shutdown.flush()])` (or break the events stream), or drop the early return and reuse the existing await loopDone / race-with-promptResult path so complete()/reject run exactly once.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
With --output-schema, a failed prompt/command request is swallowed via request.catch(reject) (reject runs complete() → emits structured_output failed + terminal lifecycle and sets the exit code), then the handler executes abort(); await Shutdown.flush(); return without awaiting loopDone. The events-consumption loop is still live: when the aborted session goes idle, loop() resolves and fires complete() a second time, and any structured_output_rejected/status events still in flight are emitted after the Shutdown.flush. If loop() itself rejects, its attached .catch(reject) re-runs reject, printing a duplicate console.error(JSON.stringify(classified)) after the handler returned. NDJSON consumers can observe late/interleaved events or duplicated stderr racing process exit.
const request = sdk.session.prompt(input(sessionID, opts))
await (schema ? request.catch(reject) : request)
}
if (schema && error) {
abort()
await Shutdown.flush()
return
}| continue | ||
| } | ||
| await SessionCompaction.prune({ sessionID }) | ||
| if (contract) { |
There was a problem hiding this comment.
🟡 "Terminal" structured_output re-emitted per prompt run.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/prompt.ts:896-903):
Problem: "Terminal" structured_output re-emitted per prompt run
Detail: EVENTS.md (added in this PR) specifies `structured_output` is "Exactly one terminal event per schema-enabled session". The implementation publishes Session.Event.StructuredOutput once per prompt() loop completion with no per-session guard. A schema-enabled session that receives a second prompt (SDK session reuse, attach mode, or a resumed session whose loop restarts with the preserved outputFormat on the last user message) emits the "terminal" event again for the same session, violating the documented once-per-session invariant. The CLI side dedups with `!structured` (run.ts), but the session-side publisher has no such guard.
Suggested fix: Either track per-session emission state so the terminal event is published only once per schema-enabled session, or relax EVENTS.md to "exactly one terminal event per schema-enabled prompt run".
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
EVENTS.md (added in this PR) specifies structured_output is "Exactly one terminal event per schema-enabled session". The implementation publishes Session.Event.StructuredOutput once per prompt() loop completion with no per-session guard. A schema-enabled session that receives a second prompt (SDK session reuse, attach mode, or a resumed session whose loop restarts with the preserved outputFormat on the last user message) emits the "terminal" event again for the same session, violating the documented once-per-session invariant. The CLI side dedups with !structured (run.ts), but the session-side publisher has no such guard.
continue
}
await SessionCompaction.prune({ sessionID })
if (contract) {
await Bus.publish(Session.Event.StructuredOutput, {
sessionID,
outcome: outcome ?? {| if (lastUser.format?.type === "json_schema") { | ||
| const rejection = async (errors: OutputSchema.Diagnostic[]) => { | ||
| if (!contract) throw new Error("Missing output schema") | ||
| if (attempts < contract.format.retryCount + 1) { |
There was a problem hiding this comment.
🟡 Rejection telemetry skipped once retry budget spent.
| if (attempts < contract.format.retryCount + 1) { | |
| Always publish Session.Event.StructuredOutputRejected and count the attempt for every rejected call; keep the budget gate only for deciding whether another corrective turn is allowed (the reminder/exhaustion logic), or amend EVENTS.md to document that post-budget rejections are not emitted. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/prompt.ts:728-738):
Problem: Rejection telemetry skipped once retry budget spent
Detail: EVENTS.md (added in this PR) states `structured_output_rejected` is "Emitted once per rejected StructuredOutput attempt, including unparseable JSON and a model turn that finishes in prose without a final tool call", and that `attempts` in the terminal event "counts StructuredOutput calls, including rejected/unparseable ones". But rejection() only increments `attempts`/`rejected` and publishes the Bus event inside `if (attempts < contract.format.retryCount + 1)`. Once the budget is spent (e.g. a second unparseable/parallel StructuredOutput call in the same step, or an unparseable argument routed through the structured reject path on a later step), rejected attempts produce no `structured_output_rejected` telemetry at all and are not counted in `attempts`, contradicting the documented contract and undercounting the terminal event.
Suggested fix: Always publish Session.Event.StructuredOutputRejected and count the attempt for every rejected call; keep the budget gate only for deciding whether another corrective turn is allowed (the reminder/exhaustion logic), or amend EVENTS.md to document that post-budget rejections are not emitted.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
EVENTS.md (added in this PR) states structured_output_rejected is "Emitted once per rejected StructuredOutput attempt, including unparseable JSON and a model turn that finishes in prose without a final tool call", and that attempts in the terminal event "counts StructuredOutput calls, including rejected/unparseable ones". But rejection() only increments attempts/rejected and publishes the Bus event inside if (attempts < contract.format.retryCount + 1). Once the budget is spent (e.g. a second unparseable/parallel StructuredOutput call in the same step, or an unparseable argument routed through the structured reject path on a later step), rejected attempts produce no structured_output_rejected telemetry at all and are not counted in attempts, contradicting the documented contract and undercounting the terminal event.
const rejection = async (errors: OutputSchema.Diagnostic[]) => {
if (!contract) throw new Error("Missing output schema")
if (attempts < contract.format.retryCount + 1) {
attempts++
rejected++
await Bus.publish(Session.Event.StructuredOutputRejected, {
sessionID,
attempt: attempts,
maxAttempts: contract.format.retryCount + 1,
errors,
})
}
return OutputSchema.error(errors).message
}| if (lastUser.format?.type === "json_schema") { | ||
| const rejection = async (errors: OutputSchema.Diagnostic[]) => { | ||
| if (!contract) throw new Error("Missing output schema") | ||
| if (attempts < contract.format.retryCount + 1) { |
There was a problem hiding this comment.
🟡 Wire-supplied retryCount never clamped or bounded.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/session/prompt.ts:728):
Problem: Wire-supplied retryCount never clamped or bounded
Detail: The corrective-retry budget comes from client-supplied `format.retryCount` (PromptInput/CommandInput via SDK/attach), not from the validated `--output-schema-retries` CLI flag, and nothing clamps it: run.output.ts only validates the CLI flag (safe integer >= 0). A negative retryCount makes `attempts < retryCount + 1` false from step one, so the contract is permanently unavailable and every run fails; a huge value (e.g. 1e9) turns each prompt into up to retryCount+1 billable model round-trips before giving up — a cost/DoS amplification from any API client. (Whether MessageV2.Format's zod schema bounds this is not visible in the diff; if it does not, this is live.)
Suggested fix: Clamp at the trust boundary when building the contract: `retryCount = Math.max(0, Math.min(Math.trunc(format.retryCount ?? 2), 10))`, and/or add `.int().min(0).max(10)` to the retryCount field of MessageV2.Format so server- and SDK-supplied values cannot bypass the CLI flag limits.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The corrective-retry budget comes from client-supplied format.retryCount (PromptInput/CommandInput via SDK/attach), not from the validated --output-schema-retries CLI flag, and nothing clamps it: run.output.ts only validates the CLI flag (safe integer >= 0). A negative retryCount makes attempts < retryCount + 1 false from step one, so the contract is permanently unavailable and every run fails; a huge value (e.g. 1e9) turns each prompt into up to retryCount+1 billable model round-trips before giving up — a cost/DoS amplification from any API client. (Whether MessageV2.Format's zod schema bounds this is not visible in the diff; if it does not, this is live.)
const rejection = async (errors: OutputSchema.Diagnostic[]) => {
if (!contract) throw new Error("Missing output schema")
if (attempts < contract.format.retryCount + 1) {
attempts++
rejected++| : [{ type: "session.status", properties: { sessionID: "ses_fixture", status: { type: "idle" } } }]), | ||
| ]) | ||
| controller!.enqueue(new TextEncoder().encode(`data: ${JSON.stringify(event)}\n\n`)) | ||
| if (name === "provider rejection") |
There was a problem hiding this comment.
🟡 Attach fixture enqueues via controller! before ready.
| if (name === "provider rejection") | |
| Buffer events until the controller is ready: keep a `pending: string[]` filled by the message handler and flushed in the /event start(), or have the message handler await a promise resolved in start() (with a timeout), instead of the `controller!` non-null assertion. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/test/cli/run-output-schema.test.ts:575):
Problem: Attach fixture enqueues via controller! before ready
Detail: In the attach-matrix fixture, `let controller` is assigned only inside the /event ReadableStream's start(), while the /session/ses_fixture/message handler enqueues with a non-null assertion `controller!.enqueue(...)`. If the CLI's POST to /message reaches the server before the /event stream's start() has executed (connect scheduling under heavy CI load), controller is undefined, the TypeError turns the fetch into a 500 and the test fails intermittently. The handler also keeps enqueuing into whatever controller was last assigned with no guard for a cancelled/dropped SSE stream, so enqueue can throw after a mid-test disconnect.
Suggested fix: Buffer events until the controller is ready: keep a `pending: string[]` filled by the message handler and flushed in the /event start(), or have the message handler await a promise resolved in start() (with a timeout), instead of the `controller!` non-null assertion.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
In the attach-matrix fixture, let controller is assigned only inside the /event ReadableStream's start(), while the /session/ses_fixture/message handler enqueues with a non-null assertion controller!.enqueue(...). If the CLI's POST to /message reaches the server before the /event stream's start() has executed (connect scheduling under heavy CI load), controller is undefined, the TypeError turns the fetch into a 500 and the test fails intermittently. The handler also keeps enqueuing into whatever controller was last assigned with no guard for a cancelled/dropped SSE stream, so enqueue can throw after a mid-test disconnect.
: name === "provider rejection"
? []
: [{ type: "session.status", properties: { sessionID: "ses_fixture", status: { type: "idle" } } }]),
])
controller!.enqueue(new TextEncoder().encode(`data: ${JSON.stringify(event)}\n\n`))
if (name === "provider rejection")| stderr: "pipe", | ||
| }, | ||
| ) | ||
| using cleanup = { [Symbol.dispose]: () => proc.kill() } |
There was a problem hiding this comment.
🟡 Spawned child lacks SIGKILL watchdog; leaks on hang.
| using cleanup = { [Symbol.dispose]: () => proc.kill() } | |
| Add `const timer = setTimeout(() => proc.kill("SIGKILL"), 20000)` right after spawn and `clearTimeout(timer)` in a finally once proc.exited resolves, matching the sibling spawning tests in run-output-schema.test.ts. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/test/provider/models.test.ts:32-37):
Problem: Spawned child lacks SIGKILL watchdog; leaks on hang
Detail: The test spawns `bun -e` running ModelsDev.refresh() against a fixture server whose response body never completes, then awaits Promise.all([stdout, stderr, proc.exited]) with no watchdog. The only kill is `using cleanup = { [Symbol.dispose]: () => proc.kill() }`, which runs when the enclosing async scope completes — if the child does not exit on its own, the pending Promise.all keeps the test body suspended forever and Symbol.dispose never executes for either `cleanup` or `server`: the `bun -e` child is orphaned and the fixture server keeps its port. Every sibling spawning test added in this PR (run-output-schema.test.ts) guards with `setTimeout(() => proc.kill("SIGKILL"), 20000)`; this one omits it.
Suggested fix: Add `const timer = setTimeout(() => proc.kill("SIGKILL"), 20000)` right after spawn and `clearTimeout(timer)` in a finally once proc.exited resolves, matching the sibling spawning tests in run-output-schema.test.ts.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The test spawns bun -e running ModelsDev.refresh() against a fixture server whose response body never completes, then awaits Promise.all([stdout, stderr, proc.exited]) with no watchdog. The only kill is using cleanup = { [Symbol.dispose]: () => proc.kill() }, which runs when the enclosing async scope completes — if the child does not exit on its own, the pending Promise.all keeps the test body suspended forever and Symbol.dispose never executes for either cleanup or server: the bun -e child is orphaned and the fixture server keeps its port. Every sibling spawning test added in this PR (run-output-schema.test.ts) guards with setTimeout(() => proc.kill("SIGKILL"), 20000); this one omits it.
using cleanup = { [Symbol.dispose]: () => proc.kill() }
const [stdout, stderr, exit] = await Promise.all([
new Response(proc.stdout).text(),
new Response(proc.stderr).text(),
proc.exited,
])| } | ||
| if (input.retries !== undefined && (!Number.isSafeInteger(input.retries) || input.retries < 0)) | ||
| throw new Error("--output-schema-retries must be an integer >= 0") | ||
| const text = await Bun.file(input.schema) |
There was a problem hiding this comment.
⚪ Module mixes Bun and node fs APIs, skips Filesystem util.
| const text = await Bun.file(input.schema) | |
| Route reads/writes through the existing Filesystem utility (as models.ts does), or at minimum settle on a single runtime API instead of mixing `Bun.file`/`Bun.write` with `fs.stat`/`fs.rename` in the same file. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #129, packages/cli/src/cli/cmd/run.output.ts:17):
Problem: Module mixes Bun and node fs APIs, skips Filesystem util
Detail: The new module reads the schema via `Bun.file(...).text()`, writes the result via `Bun.write(...)`, and stats/renames via node `fs/promises`, while sibling code in the same PR (packages/cli/src/provider/models.ts refresh()) uses the codebase's `Filesystem` wrapper for writes. Mixing three IO styles in one 63-line module diverges from the established file-access pattern and makes the atomicity guarantees EVENTS.md documents harder to audit.
Suggested fix: Route reads/writes through the existing Filesystem utility (as models.ts does), or at minimum settle on a single runtime API instead of mixing `Bun.file`/`Bun.write` with `fs.stat`/`fs.rename` in the same file.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new module reads the schema via Bun.file(...).text(), writes the result via Bun.write(...), and stats/renames via node fs/promises, while sibling code in the same PR (packages/cli/src/provider/models.ts refresh()) uses the codebase's Filesystem wrapper for writes. Mixing three IO styles in one 63-line module diverges from the established file-access pattern and makes the atomicity guarantees EVENTS.md documents harder to audit.
import path from "path"
import fs from "fs/promises"
import { OutputSchema } from "@/session/output-schema"
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 2 · 🟡 7 · ⚪ 1 · 0/10 resolved
🤖 Fix all 10 open findings with your agent📋 Out-of-diff findings (10)
Reviewed 24 files · 0 inline · view all 10 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
Terms used here
The problemThe pieces existed but enforced nothing. The idea: one validator, one budget, enforced at every boundaryThe schema is compiled once with Ajv (Ajv2020 when it declares draft 2020-12; Invalid or truncated arguments go back to the model through the existing The parts that look easy and aren't
The enforcement loopThe lifecycle spans the CLI preflight, the session loop and the repair channel. Config errors exit 2 before any model call; rejected attempts loop back while budget remains; only a value surviving final re-validation reaches the result file. flowchart TD
flags["--output-schema file"] --> preflight["Preflight: read, parse, compile schema"]
preflight -->|config error| cfgfail["Exit 2 before any model call"]
preflight --> turn["Model turn with StructuredOutput tool, toolChoice required"]
turn --> step_call{"Valid JSON matching the schema?"}
step_call -->|no| repair["Rejected attempt: invalid-tool repair with bounded diagnostics"]
repair --> budget{"Attempt budget left?"}
budget -->|yes| turn
budget -->|no| fail3["Failed: exhausted or missing, exit 3"]
step_call -->|yes| capture["Capture value and re-validate at acceptance"]
capture --> accept["Accepted: result file written atomically, exit 0"]
classDef fail fill:#fef2f2,stroke:#dc2626,color:#991b1b
classDef retry fill:#fffbeb,stroke:#d97706,color:#92400e
classDef ok fill:#f0fdf4,stroke:#16a34a,color:#166534
class fail3,cfgfail fail
class accept ok
What is live today
Two pre-existing bugs ride along: Reviewer notesReading order. Start at Enforcement chain. Repair channel and processor interplay. CLI wiring. Flags at run.ts:352-357; preflight fail with Side fixes. Attach adapter: run.ts:487-490 ( Probe points. (1) Does a plain run change? Test coverage. Written at 487f616 by aictrl · Explore the diagrams and code interactively in aictrl → |
1 participant
Implements aictrl-dev/aictrl#5958: enforce the requested output schema in headless runs, with bounded corrective retries.
Problem
format: { type: "json_schema" }and theStructuredOutputtool existed, but:jsonSchema()and no validator, so malformed arguments were captured as a successful result (reproduced through the real tool boundary before the fix: a missing required field was stored as{});retryCountwas declared but never read;aictrl runhad no way to pass a schema or receive the validated result.What changes
CLI surface (
aictrl run):--output-schema <file>: JSON Schema (object root) for the final result.--output-schema-retries <n>: additional corrective attempts (default 2). 0 means exactly one attempt; N means at most N+1.--output-result <file>: written atomically (temp file + rename) with only the validated value, and only on success. Without it, formatted mode prints the value and JSON mode carries it in the terminal event.2= schema configuration error (detected before any model request);3= structured output failed (exhausted / missing / step limit). Existing codes are unchanged.structured_output_rejectedper rejected attempt (bounded diagnostics: path, keyword, message; never submitted values) and exactly one terminalstructured_output(acceptedwith the value, orfailedwithexhausted | missing | step_limit | aborted | error). Documented inEVENTS.md.Enforcement:
allErrors; no coercion, defaults or stripping. This matches the executor's result checker, so verdicts agree (parity test over a hostile corpus).execute, again at session acceptance, and again before the result file is written.invalid-tool repair route, with diagnostics capped at 10 errors, 64-char path segments and under 2 KB, never echoing values. Logs in schema mode drop raw errors and inputs.toolChoice: "required"stays in schema mode. A prose-only finish is a counted corrective attempt with a reminder; the run fails only when the budget is spent.ProviderTransform.schema(Gemini sanitisation); local validation always uses the canonical schema.Also fixed (pre-existing, separate commits)
--attachwas broken for every run: the legacy SDK client takes{ path, body }, butrunpassed flat{ sessionID, ... }, so prompts never reached the session and the CLI hung. A single adapter now shapes all attach calls; a regression test fails without it.TimeoutErrorfrom the models.dev catalog refresh when the response body stalled (response.text()was outside the catch). Fixed, with a stalled-body regression test.Verification
packages/cli:bun run test→ 1646 pass, 7 skip, 0 fail.bun turbo typecheckgreen; prettier andgit diff --checkclean.severity) and repaired.exhaustedwith no result file at retries 0.missing(exit 3, no file). The hostile schema repaired on attempt 2. Config errors carry their specific cause in NDJSON.Out of scope
Native provider constraints (aictrl-dev/aictrl#5959) and executor/server integration (aictrl-dev/aictrl#5960, which will call
aictrl run --output-schema .contract/result.schema.json --output-result <work file>).🤖 Generated with Claude Code