Repository navigation
fix(run): output-schema review follow-ups and provider constraint hints - #134
Merged
Merged
Conversation
Contributor
Author
|
Live verification (compiled lite binary at 2cef742; real Vertex gemini-3.8-flash and GLM):
|
`aictrl run` changes into --dir before reading --output-schema, so relative schema and result paths resolve against --dir (as --file paths already do). A relative path given from the shell's directory then failed with a bare "schema.json: cannot read schema file". Resolve both paths up front and name the absolute path in every configuration error, and document the behaviour. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
|
Added |
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| @@ -41,8 +41,9 @@ aictrl run --output-schema result.schema.json --output-schema-retries 2 \ | |||
| ``` | |||
|
|
|||
| - `--output-schema <file>`: JSON Schema file, forwarded in local and `--attach` runs. | |||
There was a problem hiding this comment.
🟡 64 KiB schema cap is enforced but undocumented.
Suggested change
| - `--output-schema <file>`: JSON Schema file, forwarded in local and `--attach` runs. | |
| Add to the `--output-schema <file>` bullet (and EVENTS.md's schema-validation paragraph): "The serialized schema must not exceed 64 KiB." |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/README.md:43):
Problem: 64 KiB schema cap is enforced but undocumented
Detail: output-schema.ts now rejects serialized schemas over 64 KiB ("serialized schema must not exceed 64 KiB") with test coverage, but none of the three docs this PR updates (README.md, packages/cli/README.md, EVENTS.md) mention the cap — the --output-schema bullet and the retries bullet document other bounds (0–10, 2 KB diagnostics) while staying silent on the new size limit. Users hitting the config error on a large schema have no documented contract.
Suggested fix: Add to the `--output-schema <file>` bullet (and EVENTS.md's schema-validation paragraph): "The serialized schema must not exceed 64 KiB."
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
output-schema.ts now rejects serialized schemas over 64 KiB ("serialized schema must not exceed 64 KiB") with test coverage, but none of the three docs this PR updates (README.md, packages/cli/README.md, EVENTS.md) mention the cap — the --output-schema bullet and the retries bullet document other bounds (0–10, 2 KB diagnostics) while staying silent on the new size limit. Users hitting the config error on a large schema have no documented contract.
- `--output-schema <file>`: JSON Schema file, forwarded in local and `--attach` runs.
- `--output-schema-retries <n>`: additional corrective turns, integer from 0 to 10,
default 2. Zero permits one model request; N permits at most N+1 corrective
requests. Each rejected call is counted, including multiple calls in one step.
- `--output-result <file>`: atomically write only the validated JSON value,
| try { | ||
| while (true) { | ||
| while (queue.length > 0) { | ||
| while (!opts.signal.aborted) { |
There was a problem hiding this comment.
🟡 Abort drops queued events from subscribe() stream.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/cli/cmd/run.ts:1166-1170):
Problem: Abort drops queued events from subscribe() stream
Detail: On the new failure paths run.ts calls subscription.abort() while loop() may still have unconsumed events in the subscribe() queue. Both generator loops exit on abort without draining, so events already received from GlobalBus/SSE (e.g. a structured_output_rejected published just before a provider failure) are silently dropped from the NDJSON/JSON output. This may be deliberate ordering control (the new fixture asserts exactly one "reason" line), but terminal rejection telemetry published pre-failure is lost with no signal to the consumer.
Suggested fix: On abort, drain remaining queued events before breaking (e.g. after the aborted check, `while (queue.length > 0) yield queue.shift()`), or have run.ts await one final stream flush after subscription.abort() before consuming loopDone.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
On the new failure paths run.ts calls subscription.abort() while loop() may still have unconsumed events in the subscribe() queue. Both generator loops exit on abort without draining, so events already received from GlobalBus/SSE (e.g. a structured_output_rejected published just before a provider failure) are silently dropped from the NDJSON/JSON output. This may be deliberate ordering control (the new fixture asserts exactly one "reason" line), but terminal rejection telemetry published pre-failure is lost with no signal to the consumer.
while (!opts.signal.aborted) {
while (queue.length > 0 && !opts.signal.aborted) {
yield queue.shift()
}
if (opts.signal.aborted) break
await new Promise<void>((r) => {
resolve = r
})
}
} finally {| type: z.literal("json_schema"), | ||
| schema: z.record(z.string(), z.any()).meta({ ref: "JSONSchema" }), | ||
| retryCount: z.number().int().min(0).default(2), | ||
| retryCount: z.number().int().min(0).max(10).default(2), |
There was a problem hiding this comment.
🟡 retryCount .max(10) breaks persisted values >10.
Suggested change
| retryCount: z.number().int().min(0).max(10).default(2), | |
| Either document the contract break, or clamp instead of rejecting on parse: `retryCount: z.number().int().min(0).transform((n) => Math.min(n, 10)).default(2)` so pre-existing values clamp rather than fail replay. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/message-v2.ts:86):
Problem: retryCount .max(10) breaks persisted values >10
Detail: Tightening OutputFormatJsonSchema.retryCount with .max(10) changes the message/wire-format contract: values >10 were previously valid (only the CLI guarded >= 0, so an older client or SDK caller could send retryCount 50), and they now fail zod parse on the message/replay path instead of clamping. If persisted sessions or attach peers can carry old values, replay breaks after upgrade with no migration path.
Suggested fix: Either document the contract break, or clamp instead of rejecting on parse: `retryCount: z.number().int().min(0).transform((n) => Math.min(n, 10)).default(2)` so pre-existing values clamp rather than fail replay.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Tightening OutputFormatJsonSchema.retryCount with .max(10) changes the message/wire-format contract: values >10 were previously valid (only the CLI guarded >= 0, so an older client or SDK caller could send retryCount 50), and they now fail zod parse on the message/replay path instead of clamping. If persisted sessions or attach peers can carry old values, replay breaks after upgrade with no migration path.
.object({
type: z.literal("json_schema"),
schema: z.record(z.string(), z.any()).meta({ ref: "JSONSchema" }),
retryCount: z.number().int().min(0).max(10).default(2),
})| : `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024), { stream: true })}` | ||
| const result = tool({ | ||
| description: STRUCTURED_OUTPUT_DESCRIPTION, | ||
| description: `${STRUCTURED_OUTPUT_DESCRIPTION}\n\n${description}`, |
There was a problem hiding this comment.
🟡 Schema text embedded in tool description as instructions.
Suggested change
| description: `${STRUCTURED_OUTPUT_DESCRIPTION}\n\n${description}`, | |
| Strip or bound long free-text annotation fields (description, title, examples, errorMessage) before embedding, or document that --output-schema content is fully trusted instruction input. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/prompt.ts:1135):
Problem: Schema text embedded in tool description as instructions
Detail: The tool description now embeds the verbatim serialized schema, promoting schema-supplied free text (description/title/errorMessage/examples fields) from the structured inputSchema channel into the natural-language instruction channel. The schema author is normally the trusted operator, but any caller that sources the schema from less-trusted content gets that content treated as agent instructions. Consider documenting the schema file as fully trusted instruction input or stripping long annotation fields before embedding.
Suggested fix: Strip or bound long free-text annotation fields (description, title, examples, errorMessage) before embedding, or document that --output-schema content is fully trusted instruction 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 tool description now embeds the verbatim serialized schema, promoting schema-supplied free text (description/title/errorMessage/examples fields) from the structured inputSchema channel into the natural-language instruction channel. The schema author is normally the trusted operator, but any caller that sources the schema from less-trusted content gets that content treated as agent instructions. Consider documenting the schema file as fully trusted instruction input or stripping long annotation fields before embedding.
const result = tool({
description: `${STRUCTURED_OUTPUT_DESCRIPTION}\n\n${description}`,| @@ -1127,8 +1125,14 @@ export namespace SessionPrompt { | |||
| }): AITool & { id: "StructuredOutput" } { | |||
| const validate = input.validate ?? OutputSchema.compile(input.schema) | |||
There was a problem hiding this comment.
🟡 64 KiB cap bypassed when custom validate is passed.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/prompt.ts:1126):
Problem: 64 KiB cap bypassed when custom validate is passed
Detail: The new 64 KiB guard lives only inside OutputSchema.compile, but createStructuredOutputTool skips compile entirely when the caller passes input.validate (`input.validate ?? OutputSchema.compile(...)`). On that path JSON.stringify(input.schema) and Buffer.from(canonical) run on an unbounded schema and the tool is built with no size cap at all. Callers that inject a custom validator (createStructuredOutputTool is exported) silently bypass the limit the PR introduces.
Suggested fix: Hoist the cap out of compile (e.g. export OutputSchema.canonicalize(schema) that stringifies, enforces 64 KiB, returns {text, hash}) and call it unconditionally in createStructuredOutputTool before the validate fallback.
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 64 KiB guard lives only inside OutputSchema.compile, but createStructuredOutputTool skips compile entirely when the caller passes input.validate (input.validate ?? OutputSchema.compile(...)). On that path JSON.stringify(input.schema) and Buffer.from(canonical) run on an unbounded schema and the tool is built with no size cap at all. Callers that inject a custom validator (createStructuredOutputTool is exported) silently bypass the limit the PR introduces.
}): AITool & { id: "StructuredOutput" } {
const validate = input.validate ?? OutputSchema.compile(input.schema)
const schema = Object.fromEntries(Object.entries(input.schema).filter(([key]) => key !== "$schema"))
const canonical = JSON.stringify(input.schema)
const bytes = Buffer.from(canonical)
const description =
bytes.length <= 8 * 1024| }): AITool & { id: "StructuredOutput" } { | ||
| const validate = input.validate ?? OutputSchema.compile(input.schema) | ||
| const schema = Object.fromEntries(Object.entries(input.schema).filter(([key]) => key !== "$schema")) | ||
| const canonical = JSON.stringify(input.schema) |
There was a problem hiding this comment.
⚪ Embedded schema copy diverges from provider inputSchema.
Suggested change
| const canonical = JSON.stringify(input.schema) | |
| Stringify the same object passed to jsonSchema() (post-$schema-strip, post-ProviderTransform) for the embedded copy, or at minimum stringify the stripped `schema` variable instead of `input.schema`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/prompt.ts:1128):
Problem: Embedded schema copy diverges from provider inputSchema
Detail: canonical = JSON.stringify(input.schema) serializes the raw schema including $schema, while the schema advertised as the provider inputSchema strips $schema and applies ProviderTransform.schema. The text labelled "Canonical JSON Schema" therefore matches neither the inputSchema the model sees nor a key-order-normalized form, so the provider can be steered by two divergent copies. Embedding the validator's copy is arguably the intent, but the naming overstates it and the $schema line is dead weight in the description.
Suggested fix: Stringify the same object passed to jsonSchema() (post-$schema-strip, post-ProviderTransform) for the embedded copy, or at minimum stringify the stripped `schema` variable instead of `input.schema`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
canonical = JSON.stringify(input.schema) serializes the raw schema including $schema, while the schema advertised as the provider inputSchema strips $schema and applies ProviderTransform.schema. The text labelled "Canonical JSON Schema" therefore matches neither the inputSchema the model sees nor a key-order-normalized form, so the provider can be steered by two divergent copies. Embedding the validator's copy is arguably the intent, but the naming overstates it and the $schema line is dead weight in the description.
const schema = Object.fromEntries(Object.entries(input.schema).filter(([key]) => key !== "$schema"))\n const canonical = JSON.stringify(input.schema)\n const bytes = Buffer.from(canonical)| const description = | ||
| bytes.length <= 8 * 1024 | ||
| ? `Canonical JSON Schema: ${canonical}` | ||
| : `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024), { stream: true })}` |
There was a problem hiding this comment.
⚪ stream:true decode drops split trailing character.
--- a/packages/cli/src/session/prompt.ts
+++ b/packages/cli/src/session/prompt.ts
@@ -1131,7 +1131,7 @@
const description =
bytes.length <= 8 * 1024
? `Canonical JSON Schema: ${canonical}`
- : `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024), { stream: true })}`
+ : `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024))}`
const result = tool({🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/prompt.ts:1133):
Problem: stream:true decode drops split trailing character
Detail: decode(bytes.subarray(0, 8*1024), { stream: true }) promises more input will follow, so an incomplete trailing multi-byte sequence is buffered and — since decode() is never called again — silently dropped (1–3 bytes vanish rather than becoming U+FFFD). Cosmetic since the blob is already truncated JSON, but stream mode is the wrong tool for a one-shot slice.
Suggested fix: Decode without stream mode: new TextDecoder().decode(bytes.subarray(0, 8 * 1024)) — non-stream decode emits U+FFFD for the split codepoint instead of dropping it.
Suggested patch:
--- a/packages/cli/src/session/prompt.ts
+++ b/packages/cli/src/session/prompt.ts
@@ -1131,7 +1131,7 @@
const description =
bytes.length <= 8 * 1024
? `Canonical JSON Schema: ${canonical}`
- : `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024), { stream: true })}`
+ : `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024))}`
const result = tool({
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
decode(bytes.subarray(0, 8*1024), { stream: true }) promises more input will follow, so an incomplete trailing multi-byte sequence is buffered and — since decode() is never called again — silently dropped (1–3 bytes vanish rather than becoming U+FFFD). Cosmetic since the blob is already truncated JSON, but stream mode is the wrong tool for a one-shot slice.
const description =
bytes.length <= 8 * 1024
? `Canonical JSON Schema: ${canonical}`
: `Canonical JSON Schema (truncated; the validator enforces the full schema): ${new TextDecoder().decode(bytes.subarray(0, 8 * 1024), { stream: true })}`
const result = tool({| @@ -753,7 +751,7 @@ export namespace SessionPrompt { | |||
| async onSuccess(output) { | |||
| if (structuredOutput !== undefined) return | |||
| if (!available) throw new Error("StructuredOutput attempt budget exhausted") | |||
There was a problem hiding this comment.
⚪ Error still says 'attempt budget' after reterming.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/prompt.ts:753):
Problem: Error still says 'attempt budget' after reterming
Detail: This PR deliberately reterms the vocabulary: maxAttempts = the corrective turn budget, `attempt` = the per-call counter that may exceed it (EVENTS.md, CLI help, READMEs, tests). The unchanged error "StructuredOutput attempt budget exhausted" still conflates the two by calling the budget an "attempt budget", undercutting the reterming done everywhere else in the same PR.
Suggested fix: Rename the message to "StructuredOutput corrective turn budget exhausted" to match the documented terms.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
This PR deliberately reterms the vocabulary: maxAttempts = the corrective turn budget, attempt = the per-call counter that may exceed it (EVENTS.md, CLI help, READMEs, tests). The unchanged error "StructuredOutput attempt budget exhausted" still conflates the two by calling the budget an "attempt budget", undercutting the reterming done everywhere else in the same PR.
async onSuccess(output) {\n if (structuredOutput !== undefined) return\n if (!available) throw new Error("StructuredOutput attempt budget exhausted")\n attempts++\n structuredOutput = output\n },| // Extract the subscribe method body | ||
| const subscribeBody = source.slice(subscribeIdx, subscribeIdx + 600) | ||
| const subscribeBody = source.slice(subscribeIdx, source.indexOf("return { stream }", subscribeIdx)) | ||
| // GlobalBus.on must appear BEFORE the generator definition |
There was a problem hiding this comment.
⚪ Sentinel indexOf -1 silently widens slice body.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/test/cli/run-event-race.test.ts:14):
Problem: Sentinel indexOf -1 silently widens slice body
Detail: subscribeBody = source.slice(subscribeIdx, source.indexOf("return { stream }", subscribeIdx)): if the sentinel is ever renamed/removed, indexOf returns -1 and slice(subscribeIdx, -1) yields nearly the whole file, so the ordering assertions run against unrelated code and can false-pass/false-fail instead of erroring clearly. (Same pattern in the second test.)
Suggested fix: const end = source.indexOf("return { stream }", subscribeIdx); expect(end).toBeGreaterThan(-1); then slice(subscribeIdx, end). Apply to both tests.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
subscribeBody = source.slice(subscribeIdx, source.indexOf("return { stream }", subscribeIdx)): if the sentinel is ever renamed/removed, indexOf returns -1 and slice(subscribeIdx, -1) yields nearly the whole file, so the ordering assertions run against unrelated code and can false-pass/false-fail instead of erroring clearly. (Same pattern in the second test.)
const subscribeIdx = source.indexOf("async subscribe(")\n expect(subscribeIdx).toBeGreaterThan(-1)\n // Extract the subscribe method body\n const subscribeBody = source.slice(subscribeIdx, source.indexOf("return { stream }", subscribeIdx))\n // GlobalBus.on must appear BEFORE the generator definition| const canonical = { type: "object", description: "x".repeat(9 * 1024) } | ||
| const tool = SessionPrompt.createStructuredOutputTool({ schema: canonical, onSuccess() {} }) | ||
| expect(tool.description).toContain("truncated; the validator enforces the full schema") | ||
| expect(tool.description?.split(": ").at(-1)).toBe(JSON.stringify(canonical).slice(0, 8 * 1024)) |
There was a problem hiding this comment.
⚪ Truncation test asserts chars, code truncates bytes.
Suggested change
| expect(tool.description?.split(": ").at(-1)).toBe(JSON.stringify(canonical).slice(0, 8 * 1024)) | |
| Assert against the byte-truncated expectation: new TextDecoder().decode(Buffer.from(JSON.stringify(canonical)).subarray(0, 8 * 1024)) — or keep the char-slice with a comment that the fixture must stay ASCII. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/test/session/output-schema.test.ts:701):
Problem: Truncation test asserts chars, code truncates bytes
Detail: The assertion JSON.stringify(canonical).slice(0, 8 * 1024) slices UTF-16 code units while the implementation truncates BYTES (Buffer.from(canonical).subarray(0, 8*1024) decoded); the two coincide only for the ASCII "x".repeat fixture. A future multi-byte fixture would fail (or pass for the wrong reason), so the test encodes the wrong invariant.
Suggested fix: Assert against the byte-truncated expectation: new TextDecoder().decode(Buffer.from(JSON.stringify(canonical)).subarray(0, 8 * 1024)) — or keep the char-slice with a comment that the fixture must stay ASCII.
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 assertion JSON.stringify(canonical).slice(0, 8 * 1024) slices UTF-16 code units while the implementation truncates BYTES (Buffer.from(canonical).subarray(0, 8*1024) decoded); the two coincide only for the ASCII "x".repeat fixture. A future multi-byte fixture would fail (or pass for the wrong reason), so the test encodes the wrong invariant.
test("tool description caps the canonical schema at 8 KiB and explains truncation", () => {\n const canonical = { type: "object", description: "x".repeat(9 * 1024) }\n const tool = SessionPrompt.createStructuredOutputTool({ schema: canonical, onSuccess() {} })\n expect(tool.description).toContain("truncated; the validator enforces the full schema")\n expect(tool.description?.split(": ").at(-1)).toBe(JSON.stringify(canonical).slice(0, 8 * 1024))\n})
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 6 · ⚪ 7 · 2/13 resolved
🤖 Fix all 11 open findings with your agent📋 Out-of-diff findings (13)
Reviewed 13 files · 0 inline · view all 13 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
| // Relative paths resolve against --dir (run changes into it first), like --file. | ||
| // Report the resolved path so a misplaced relative path is obvious. | ||
| const file = path.resolve(input.schema) | ||
| const text = await Bun.file(file) |
There was a problem hiding this comment.
🟡 Absolute path prefix can push reason past 300-char cap.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/cli/cmd/run.output.ts:21-35):
Problem: Absolute path prefix can push reason past 300-char cap
Detail: Config-error messages now embed the fully resolved absolute path (`${file}: cannot read schema file`, `${file}: invalid JSON`, `${file}: schema root must be a JSON object`, `${file}: serialized schema must not exceed 64 KiB`, `${file}: <ajv strict-mode message>`), but the CLI caps invocation_error messages at 300 chars (the test harness asserts message <= 300). The path prefix is unbounded (PATH_MAX 4096): with a cwd/--dir nesting deep enough that the prefix approaches ~260+ chars, the reason suffix falls past the cap and users get only path text with no failure cause. The base behavior echoed the user-supplied (typically short relative) path, so the degradation is introduced by the absolutization added here; the NDJSON clip test was even reworked (toStartWith(path.sep)) showing awareness of the longer prefix.
Suggested fix: When composing these errors, bound the path portion (e.g. keep the last ~120 chars with an ellipsis marker) or emit `reason (path: ${file})` so the 300-char truncation eats path text rather than the diagnostic reason.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Config-error messages now embed the fully resolved absolute path (${file}: cannot read schema file, ${file}: invalid JSON, ${file}: schema root must be a JSON object, ${file}: serialized schema must not exceed 64 KiB, ${file}: <ajv strict-mode message>), but the CLI caps invocation_error messages at 300 chars (the test harness asserts message <= 300). The path prefix is unbounded (PATH_MAX 4096): with a cwd/--dir nesting deep enough that the prefix approaches ~260+ chars, the reason suffix falls past the cap and users get only path text with no failure cause. The base behavior echoed the user-supplied (typically short relative) path, so the degradation is introduced by the absolutization added here; the NDJSON clip test was even reworked (toStartWith(path.sep)) showing awareness of the longer prefix.
// Relative paths resolve against --dir (run changes into it first), like --file.
// Report the resolved path so a misplaced relative path is obvious.
const file = path.resolve(input.schema)
const text = await Bun.file(file)
.text()
.catch(() => {
throw new Error(`${file}: cannot read schema file`)
})| 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 text = JSON.stringify(schema) |
There was a problem hiding this comment.
🟡 64 KiB cap bounds bytes, not schema depth.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/session/output-schema.ts:25):
Problem: 64 KiB cap bounds bytes, not schema depth
Detail: The new DoS guard in OutputSchema.compile limits only the serialized byte size of the schema. Ajv compile-time codegen and validate-time recursion scale with structural depth, not byte length: ~9,000 nested {"not":{...}} layers (7 bytes/level) fit well under 64 KiB and are a known Ajv pattern for "Maximum call stack size exceeded" (RangeError crashes the process) or pathological compile cost. This is distinct from the recorded "cap bypassed when custom validate is passed" finding: here the cap executes but is insufficient. The trust boundary is not only the local --output-schema flag — OutputFormatJsonSchema.schema is parsed from persisted/replayed session messages via z.record(z.string(), z.any()) (message-v2.ts) with no structural constraints, so a crafted or shared session file can reach compile() with a depth-bomb schema; the content-hash cache key means each distinct crafted schema forces a fresh compile.
Suggested fix: Before hashing/compiling, walk the schema and enforce structural bounds (e.g. max nesting depth ~100, max node/keyword count), throwing the same user-facing schema error on violation; optionally wrap the Ajv compile in try/catch to convert RangeError into a graceful OutputSchema error instead of a process crash.
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 DoS guard in OutputSchema.compile limits only the serialized byte size of the schema. Ajv compile-time codegen and validate-time recursion scale with structural depth, not byte length: ~9,000 nested {"not":{...}} layers (7 bytes/level) fit well under 64 KiB and are a known Ajv pattern for "Maximum call stack size exceeded" (RangeError crashes the process) or pathological compile cost. This is distinct from the recorded "cap bypassed when custom validate is passed" finding: here the cap executes but is insufficient. The trust boundary is not only the local --output-schema flag — OutputFormatJsonSchema.schema is parsed from persisted/replayed session messages via z.record(z.string(), z.any()) (message-v2.ts) with no structural constraints, so a crafted or shared session file can reach compile() with a depth-bomb schema; the content-hash cache key means each distinct crafted schema forces a fresh compile.
export function compile(schema: Record<string, unknown>) {
if (schema.type !== "object") throw new Error('schema root must have type "object"')
const text = JSON.stringify(schema)
if (Buffer.byteLength(text) > 64 * 1024) throw new Error("serialized schema must not exceed 64 KiB")
const key = createHash("sha256").update(text).digest("hex")
const cached = cache.get(key)| const event = result.events.find((event) => event.type === "invocation_error") | ||
| expect(event?.code).toBe("OUTPUT_SCHEMA_CONFIG") | ||
| expect(event?.message).toStartWith("schema.json: strict mode: unknown keyword:") | ||
| expect(event?.message).toStartWith(path.sep) |
There was a problem hiding this comment.
🟡 toStartWith(path.sep) fails on Windows drive paths.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/test/cli/run-output-schema.test.ts:725):
Problem: toStartWith(path.sep) fails on Windows drive paths
Detail: The updated NDJSON clip assertion checks absoluteness with expect(event?.message).toStartWith(path.sep). On Windows, path.resolve in run.output.ts yields a drive-anchored path like C:\tmp\schema.json, which does not start with path.sep (\), so the assertion fails even though the production behavior (resolved absolute path in the error) is correct. The same patch's other new expectations (${path.sep}home${path.sep}schema.json: cannot read schema file) use substring matching and are platform-safe, making this one line the inconsistent outlier.
Suggested fix: Assert resolution platform-neutrally, e.g. keep the toContain(`${path.sep}schema.json: strict mode: unknown keyword:`) check and drop the toStartWith(path.sep) prefix assertion, or assert path.isAbsolute() on the message's path portion instead.
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 updated NDJSON clip assertion checks absoluteness with expect(event?.message).toStartWith(path.sep). On Windows, path.resolve in run.output.ts yields a drive-anchored path like C:\tmp\schema.json, which does not start with path.sep (), so the assertion fails even though the production behavior (resolved absolute path in the error) is correct. The same patch's other new expectations (${path.sep}home${path.sep}schema.json: cannot read schema file) use substring matching and are platform-safe, making this one line the inconsistent outlier.
expect(result.requests).toEqual([])
const event = result.events.find((event) => event.type === "invocation_error")
expect(event?.code).toBe("OUTPUT_SCHEMA_CONFIG")
expect(event?.message).toStartWith(path.sep)
expect(event?.message).toContain(`${path.sep}schema.json: strict mode: unknown keyword:`)
expect((event?.message as string).length).toBe(300)| Zero permits exactly one model request. A step can contain multiple StructuredOutput | ||
| calls; every rejected call is reported and counted even after the budget is spent. | ||
| `--output-result <file>` writes only accepted JSON using two-space indentation and a trailing newline, with a temporary file | ||
| in the destination directory followed by an atomic rename. Failures preserve the |
There was a problem hiding this comment.
⚪ Rewritten EVENTS.md sentence left unwrapped at ~121 chars.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, EVENTS.md:546):
Problem: Rewritten EVENTS.md sentence left unwrapped at ~121 chars
Detail: The edit reflowed the --output-schema-retries paragraph but joined the --output-result sentence onto a single ~121-character line ("`--output-result <file>` writes only accepted JSON using two-space indentation and a trailing newline, with a temporary file"). Every surrounding line in EVENTS.md, including the untouched continuation "in the destination directory followed by an atomic rename", wraps at ~80 columns, and the pre-PR text wrapped this same sentence across two lines. The companion line 543 (~86 chars) is also slightly over. Re-wrap the paragraph to preserve the file's markdown source convention.
Suggested fix: Split line 546 so the sentence wraps at ~80 columns, e.g. "...trailing newline, with a temporary\nfile in the destination directory followed by an atomic rename."
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 edit reflowed the --output-schema-retries paragraph but joined the --output-result sentence onto a single ~121-character line ("--output-result <file> writes only accepted JSON using two-space indentation and a trailing newline, with a temporary file"). Every surrounding line in EVENTS.md, including the untouched continuation "in the destination directory followed by an atomic rename", wraps at ~80 columns, and the pre-PR text wrapped this same sentence across two lines. The companion line 543 (~86 chars) is also slightly over. Re-wrap the paragraph to preserve the file's markdown source convention.
`--output-schema-retries <n>` permits N additional corrective turns (0–10, default 2).
Zero permits exactly one model request. A step can contain multiple StructuredOutput
calls; every rejected call is reported and counted even after the budget is spent.
`--output-result <file>` writes only accepted JSON using two-space indentation and a trailing newline, with a temporary file
in the destination directory followed by an atomic rename. Failures preserve the
| @@ -41,8 +41,11 @@ aictrl run --output-schema result.schema.json --output-schema-retries 2 \ | |||
| ``` | |||
|
|
|||
There was a problem hiding this comment.
⚪ Shared path-resolution note nested under one flag's bullet.
Suggested change
| Move the shared note to after the `--output-result` bullet (or into the surrounding prose), or split it so each flag's bullet carries only its own path-resolution sentence. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/README.md:42-43):
Problem: Shared path-resolution note nested under one flag's bullet
Detail: The added note "Relative `--output-schema` and `--output-result` paths resolve against `--dir`..." is indented as a continuation of the `--output-schema` bullet, but it governs two flags and forward-references `--output-result`, whose own bullet appears three bullets later ("--output-result <file>: atomically write..."). Every other sub-line in these bullets describes only its own flag. The shared behavior reads better placed where both flags are in scope.
Suggested fix: Move the shared note to after the `--output-result` bullet (or into the surrounding prose), or split it so each flag's bullet carries only its own path-resolution sentence.
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 added note "Relative --output-schema and --output-result paths resolve against --dir..." is indented as a continuation of the --output-schema bullet, but it governs two flags and forward-references --output-result, whose own bullet appears three bullets later ("--output-result : atomically write..."). Every other sub-line in these bullets describes only its own flag. The shared behavior reads better placed where both flags are in scope.
- `--output-schema <file>`: JSON Schema file, forwarded in local and `--attach` runs.
Relative `--output-schema` and `--output-result` paths resolve against `--dir`
when it is given (as `--file` paths do); configuration errors print the resolved path.
- `--output-schema-retries <n>`: additional corrective turns, integer from 0 to 10,
| } | ||
|
|
||
| const events = await sdk.event.subscribe() | ||
| const subscription = new AbortController() |
There was a problem hiding this comment.
⚪ `subscription` names an AbortController, not a subscription.
Suggested change
| const subscription = new AbortController() | |
| Rename the controller to something like `subscriptionAbort` (or `eventAbort`) and keep `events` as the subscription handle, so `subscriptionAbort.abort()`/`.signal` read unambiguously. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #134, packages/cli/src/cli/cmd/run.ts:564-565):
Problem: `subscription` names an AbortController, not a subscription
Detail: The new binding `const subscription = new AbortController()` is named after the resource it aborts, while the actual subscription handle returned by sdk.event.subscribe({ signal: subscription.signal }) is named `events`. Later reads like subscription.abort() initially suggest the subscription object itself has an abort method. The file already names its session-abort path distinctly (function abort() at ~L863), so the controller should follow the same controller-vs-action naming to avoid confusion with the event subscription it cancels.
Suggested fix: Rename the controller to something like `subscriptionAbort` (or `eventAbort`) and keep `events` as the subscription handle, so `subscriptionAbort.abort()`/`.signal` read unambiguously.
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 binding const subscription = new AbortController() is named after the resource it aborts, while the actual subscription handle returned by sdk.event.subscribe({ signal: subscription.signal }) is named events. Later reads like subscription.abort() initially suggest the subscription object itself has an abort method. The file already names its session-abort path distinctly (function abort() at ~L863), so the controller should follow the same controller-vs-action naming to avoid confusion with the event subscription it cancels.
return false
}
const subscription = new AbortController()
const events = await sdk.event.subscribe({ signal: subscription.signal })
let error: string | undefined
const startTime = Date.now()
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 3 · ⚪ 3 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 13 files · 0 inline · view all 6 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
- Bound schema structure (depth 64, 10k nested objects) as well as bytes: ~2,000 nested levels fit in 16 KB yet overflow Ajv's recursive codegen with a RangeError. The check is iterative. - Lead configuration errors with the reason and put the resolved path last, so the 300-char clip drops path text rather than the cause. - Rename the event-stream AbortController to subscriptionAbort. - Platform-neutral clip assertion; re-wrap EVENTS.md; move the shared path-resolution note out of a single flag's bullet. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Contributor
Author
Review response — PR #134Triage of both aictrl review rounds on this PR (13:45Z, 13 findings; 14:08Z, 6 findings). One round-1 finding was a duplicate already resolved. Fixes are pushed; the head is Issues addressed (pushed to this PR)
Review claims verified false (no change needed)
Not addressed here
|
This was referenced Oct 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #129 (released in v0.4.6). It addresses the second automated review round on #129, which posted after merge, and two improvements from live provider probes.
Review round 2 (verdicts recorded on #129)
schema root must be a JSON object. Thetype: "object"wording stays with the compiler.structured_output,session_completeand the classified stderr error each appear exactly once.StructuredOutputcall emitsstructured_output_rejectedand is counted in the terminalattempts, including extra calls after the budget is spent. The budget only decides whether another corrective request happens; the N+1 request bound is unchanged.retryCountis capped at 10, both on the wire schema and for--output-schema-retries.run.output.ts.retryCountNaN finding was verified false (prompt()zod-parses its input, so the default of 2 applies).Provider improvements (live-probe evidence on aictrl-dev/aictrl#5959)
pattern,additionalProperties, min/max and$reffrom tool parameters, so Gemini never saw those constraints and learned them only from rejections. The compact canonical schema is now appended to theStructuredOutputdescription (capped at 8 KiB; past the cap it says the validator enforces the full schema).tool_choicebut not reliably"required". Normal schema steps keeprequired, so the agent keeps its tools. The corrective turn after a prose-only finish forcesStructuredOutputby name.Verification
packages/cli:bun run testgives 1664 pass, 7 skip, 0 fail.bun turbo typecheckis green; prettier andgit diff --checkare clean.🤖 Generated with Claude Code