Repository navigation
fix(run): output-schema review follow-ups and provider constraint hints #134
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
2c58138
2e438e2
2cef742
7184b47
d9feced
4487718
0f53783
1f1bf14
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -34,34 +34,43 @@ aictrl run --format json "scan for secrets" | jq '.properties.part.text' | |||||
| while the agent can continue using tools. The schema root must be `type: "object"`. | ||||||
| Schemas use Ajv defaults (strict validation, draft-07); a declared draft 2020-12 | ||||||
| schema uses Ajv2020. Invalid configuration exits before any model request. | ||||||
| Serialized schemas are limited to 64 KiB, depth 64 and 10,000 nested objects. | ||||||
|
|
||||||
| ```bash | ||||||
| aictrl run --output-schema result.schema.json --output-schema-retries 2 \ | ||||||
| --output-result result.json "review this change" | ||||||
| ``` | ||||||
|
|
||||||
| - `--output-schema <file>`: JSON Schema file, forwarded in local and `--attach` runs. | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 64 KiB schema cap is enforced but undocumented.
Suggested change
🤖 Fix with your agentWhy this mattersoutput-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-retries <n>`: additional corrective attempts, integer >= 0, | ||||||
| default 2. Zero permits one attempt; N permits at most N+1 attempts. | ||||||
| The file is trusted instruction input: annotations (`description`, `title`, | ||||||
| `examples`) are shown to the model in the StructuredOutput tool description. | ||||||
| - `--output-schema-retries <n>`: additional corrective turns, integer from 0 to 10, | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ Docs mix 'corrective requests' and 'corrective turns'. 🤖 Fix with your agentWhy this mattersREADME.md and EVENTS.md standardize on "corrective turns" (EVENTS once glossing "turns/requests"), but packages/cli/README.md introduces "N permits at most N+1 corrective requests" / "Zero permits one model request" — a third term in the same reterming pass. If the request-vs-turn distinction is deliberate (parallel calls within a turn), say so; otherwise pick one term for the budget. |
||||||
| default 2. Zero permits only the initial turn; N permits the initial turn and | ||||||
| at most N corrective turns. Each rejected call is counted, including multiple | ||||||
| calls in one step. | ||||||
| - `--output-result <file>`: atomically write only the validated JSON value, | ||||||
| indented with two spaces and a trailing newline. Failures preserve an existing | ||||||
| file and leave an absent file absent. The destination directory must exist. | ||||||
|
|
||||||
| Relative `--output-schema` and `--output-result` paths resolve against `--dir` when | ||||||
| it is given (as `--file` paths do); configuration errors print the resolved path. | ||||||
|
|
||||||
| Ajv strict mode rejects unknown schema keywords and unregistered `format` | ||||||
| values such as `date-time` and `uri` as configuration errors (exit **2**). | ||||||
| The result parent directory must exist and be writable at configuration time. | ||||||
|
|
||||||
| The retries and result flags require `--output-schema`. Without a result file, | ||||||
| formatted mode prints validated JSON at the end; `--format json` emits it in a | ||||||
| terminal `structured_output` event. Rejections emit `structured_output_rejected` | ||||||
| terminal `structured_output` event, exactly one per prompt run. A headless | ||||||
| `aictrl run` has one prompt run. Rejections emit `structured_output_rejected` | ||||||
| with bounded JSON-pointer diagnostics. See [EVENTS.md](../../EVENTS.md) for the contract. | ||||||
| No valid result means failure, including missing output and the agent step limit; | ||||||
| there is no prose fallback. A prose-only finish consumes one attempt and receives | ||||||
| a reminder to call StructuredOutput while retries remain. Earlier tools are not | ||||||
| replayed for corrective attempts. | ||||||
| replayed for corrective turns. | ||||||
|
|
||||||
| Exit codes: **0** for accepted results, **2** for schema configuration errors, | ||||||
| **3** for exhausted attempts, missing output or a step limit without a result. | ||||||
| **3** for exhausted corrective turns, missing output or a step limit without a result. | ||||||
| Provider failures and stream timeouts retain **1**; SIGINT/SIGTERM retain | ||||||
| **130**/**143**. Invocations without `--output-schema` keep their existing output. | ||||||
|
|
||||||
|
|
||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,5 @@ | ||
| import path from "path" | ||
| // Bun content I/O preserves createPath: false; fs/promises supplies metadata and atomic rename/removal. | ||
| import fs from "fs/promises" | ||
| import { OutputSchema } from "@/session/output-schema" | ||
|
|
||
|
|
@@ -12,32 +13,37 @@ export async function outputSchema(input: { schema?: string; retries?: number; r | |
| throw new Error("--output-schema-retries and --output-result require --output-schema") | ||
| return | ||
| } | ||
| 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) | ||
| if (input.retries !== undefined && (!Number.isSafeInteger(input.retries) || input.retries < 0 || input.retries > 10)) | ||
| throw new Error("--output-schema-retries must be an integer between 0 and 10") | ||
| // Relative paths resolve against --dir (run changes into it first), like --file. | ||
| // Report the resolved path after the reason, so clipping (300 chars) drops path | ||
| // text rather than the cause. | ||
| const file = path.resolve(input.schema) | ||
| const text = await Bun.file(file) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🟡 Absolute path prefix can push reason past 300-char cap. 🤖 Fix with your agentWhy this mattersConfig-error messages now embed the fully resolved absolute path ( // 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`)
}) |
||
| .text() | ||
| .catch(() => { | ||
| throw new Error(`${input.schema}: cannot read schema file`) | ||
| throw new Error(`cannot read schema file (${file})`) | ||
| }) | ||
| const schema: unknown = (() => { | ||
| try { | ||
| return JSON.parse(text) | ||
| } catch { | ||
| throw new Error(`${input.schema}: invalid JSON`) | ||
| throw new Error(`invalid JSON (${file})`) | ||
| } | ||
| })() | ||
| if (!schema || typeof schema !== "object" || Array.isArray(schema)) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ⚪ Divergent root-schema error wording between layers. 🤖 Fix with your agentWhy this mattersThe config layer now throws "${file}: schema root must be a JSON object" (array/null/scalar roots) while OutputSchema.compile throws 'schema root must have type "object"' (non-object })()\n if (!schema || typeof schema !== "object" || Array.isArray(schema))\n throw new Error(`${input.schema}: schema root must be a JSON object`)\n const canonical = schema as Record<string, unknown> |
||
| throw new Error(`${input.schema}: schema root must have type "object"`) | ||
| throw new Error(`schema root must be a JSON object (${file})`) | ||
| const canonical = schema as Record<string, unknown> | ||
| const validate = (() => { | ||
| try { | ||
| return OutputSchema.compile(canonical) | ||
| } catch (error) { | ||
| throw new Error(`${input.schema}: ${error instanceof Error ? error.message : "cannot compile schema"}`) | ||
| throw new Error(`${error instanceof Error ? error.message : "cannot compile schema"} (${file})`) | ||
| } | ||
| })() | ||
| if (input.result) { | ||
| const directory = path.dirname(input.result) | ||
| const result = path.resolve(input.result) | ||
| const directory = path.dirname(result) | ||
| const writable = await fs | ||
| .stat(directory) | ||
| .then(async (stat) => { | ||
|
|
@@ -46,7 +52,7 @@ export async function outputSchema(input: { schema?: string; retries?: number; r | |
| return true | ||
| }) | ||
| .catch(() => false) | ||
| if (!writable) throw new Error(`${input.result}: output-result parent directory must exist and be writable`) | ||
| if (!writable) throw new Error(`output-result parent directory must exist and be writable (${result})`) | ||
| } | ||
| return { format: { type: "json_schema" as const, schema: canonical, retryCount: input.retries ?? 2 }, validate } | ||
| } | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
⚪ Shared path-resolution note nested under one flag's bullet.
🤖 Fix with your agent
Why this matters
The added note "Relative
--output-schemaand--output-resultpaths resolve against--dir..." is indented as a continuation of the--output-schemabullet, 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.