Repository navigation
feat(run): StructuredOutput publishes the validated result file (#140) - #142
Merged
Merged
Conversation
BREAKING CHANGE: in --output-schema mode the StructuredOutput tool takes
only {"path": "<file>"}. The model writes its final result as JSON to a
file in the working directory and passes the path; result objects passed
as tool arguments are rejected.
The CLI resolves the path with realpath, keeps it inside the working
directory, caps it at 2 MiB, reads it through one handle, validates it
with the shared Ajv validator and publishes the parsed file. Only {path}
reaches the provider, so provider schema conversion cannot drop result
fields (Gemini emptied map-typed fields, #139), and the model no longer
re-emits the whole result. File rejections are counted corrective turns
that ask the model to fix the file at that path.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019gASjVHuvN9AxK5WT8jzsN
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| if (relative.startsWith("..") || path.isAbsolute(relative)) | ||
| return fail("file", "path must stay inside the working directory") | ||
| // One handle for stat and read, so the checked file is the file read. | ||
| const handle = await fs.open(target, "r").catch(() => undefined) |
There was a problem hiding this comment.
🟠 Symlink-swap TOCTOU defeats path containment.
Suggested change
| const handle = await fs.open(target, "r").catch(() => undefined) | |
| Open with `fs.open(target, "r", fs.constants.O_NOFOLLOW)` and map ELOOP to the existing containment rejection, or after open call `await handle.realpath()` and re-verify it is still inside `base` before reading. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:123-124):
Problem: Symlink-swap TOCTOU defeats path containment
Detail: Containment is verified on the realpath-resolved `target`, but `fs.open(target, "r")` then re-traverses the path and follows symlinks again. A background process the model started via its bash tool can replace the final path component with a symlink pointing outside the working directory between the realpath check and the open (e.g. `(sleep 2; ln -sf ~/.ssh/creds.json result.json) &`), so stat/read operate on a file outside root and its contents are published as the structured result — defeating the exact containment boundary this PR introduces. The "one handle for stat and read" comment protects stat↔read identity, not the realpath↔open gap. Since `target` is already fully resolved, a legitimate in-root path cannot itself be a symlink at the final component, so O_NOFOLLOW would not break valid use.
Suggested fix: Open with `fs.open(target, "r", fs.constants.O_NOFOLLOW)` and map ELOOP to the existing containment rejection, or after open call `await handle.realpath()` and re-verify it is still inside `base` before reading.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Containment is verified on the realpath-resolved target, but fs.open(target, "r") then re-traverses the path and follows symlinks again. A background process the model started via its bash tool can replace the final path component with a symlink pointing outside the working directory between the realpath check and the open (e.g. (sleep 2; ln -sf ~/.ssh/creds.json result.json) &), so stat/read operate on a file outside root and its contents are published as the structured result — defeating the exact containment boundary this PR introduces. The "one handle for stat and read" comment protects stat↔read identity, not the realpath↔open gap. Since target is already fully resolved, a legitimate in-root path cannot itself be a symlink at the final component, so O_NOFOLLOW would not break valid use.
const target = await fs.realpath(path.resolve(base, input)).catch(() => undefined)
if (!target) return fail("file", "file not found")
const relative = path.relative(base, target)
if (relative.startsWith("..") || path.isAbsolute(relative))
return fail("file", "path must stay inside the working directory")
// One handle for stat and read, so the checked file is the file read.
const handle = await fs.open(target, "r").catch(() => undefined)
if (!handle) return fail("file", "file not readable")
try {
const stat = await handle.stat()| to 64 characters; paths also have an overall length bound. | ||
| Validation feedback goes to the model in the same session through the `invalid` tool. | ||
| File diagnostics use keyword `file` (`file not found`, `path must stay inside the | ||
| working directory`, `path must be a regular file`, a size over 2 MiB) or `parse` |
There was a problem hiding this comment.
🟡 EVENTS.md file-diagnostics list incomplete.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, EVENTS.md:584-586):
Problem: EVENTS.md file-diagnostics list incomplete
Detail: `OutputSchema.file` emits six fixed diagnostics, but the EVENTS.md list added by this PR quotes only three `file` strings verbatim, paraphrases the size error as "a size over 2 MiB" instead of the exact `file must not exceed 2097152 bytes`, and omits "file not readable" entirely. The unit tests assert these exact strings, so consumers matching on documented messages will not find the full set.
Suggested fix: Add `file not readable` to the keyword-`file` list and quote the size diagnostic exactly (e.g. `file must not exceed 2097152 bytes`) so all six fixed strings are documented verbatim.
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.file emits six fixed diagnostics, but the EVENTS.md list added by this PR quotes only three file strings verbatim, paraphrases the size error as "a size over 2 MiB" instead of the exact file must not exceed 2097152 bytes, and omits "file not readable" entirely. The unit tests assert these exact strings, so consumers matching on documented messages will not find the full set.
File diagnostics use keyword `file` (`file not found`, `path must stay inside the\nworking directory`, `path must be a regular file`, a size over 2 MiB) or `parse`\n(`file must contain complete, valid JSON`); schema diagnostics use Ajv keywords.\nFeedback for a rejected file goes to the model as the StructuredOutput tool error and
| object) go to the model through the `invalid` tool. | ||
| A prose-only finish emits the fixed diagnostic | ||
| `{ "path": "", "keyword": "missing", "message": "call StructuredOutput with the final result" }` | ||
| and receives an ephemeral reminder before the next turn while budget remains. |
There was a problem hiding this comment.
🟡 Prose-only diagnostic still teaches old contract.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, EVENTS.md:591-592):
Problem: Prose-only diagnostic still teaches old contract
Detail: This PR rewires the StructuredOutput contract to file form and updates the ephemeral reminder text, but the retained fixed diagnostic for a prose-only finish (emitted in prompt.ts and asserted unchanged in the tests) still tells the model "call StructuredOutput with the final result" — the old contract wording this PR explicitly removes everywhere else. On a prose-only retry the model receives contradictory instructions (diagnostic vs reminder), inviting it to call the tool with the result as arguments, which is now rejected.
Suggested fix: Update the missing-tool-call diagnostic in prompt.ts (and this EVENTS.md example) to the file form, e.g. `"message": "write the final result as JSON to a file in the working directory, then call StructuredOutput with its path"`, keeping the diagnostic and reminder consistent.
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 rewires the StructuredOutput contract to file form and updates the ephemeral reminder text, but the retained fixed diagnostic for a prose-only finish (emitted in prompt.ts and asserted unchanged in the tests) still tells the model "call StructuredOutput with the final result" — the old contract wording this PR explicitly removes everywhere else. On a prose-only retry the model receives contradictory instructions (diagnostic vs reminder), inviting it to call the tool with the result as arguments, which is now rejected.
object) go to the model through the `invalid` tool.\nA prose-only finish emits the fixed diagnostic\n{ \"path\": \"\", \"keyword\": \"missing\", \"message\": \"call StructuredOutput with the final result\" }\nand receives an ephemeral reminder before the next turn while budget remains.
| validate: ReturnType<typeof compile>, | ||
| ): Promise<{ value: unknown } | { errors: Diagnostic[] }> { | ||
| const fail = (keyword: string, message: string) => ({ errors: [{ path: "", keyword, message }] }) | ||
| const base = await fs.realpath(root) |
There was a problem hiding this comment.
🟡 Unguarded fs errors bypass counted rejection.
Suggested change
| const base = await fs.realpath(root) | |
| Wrap the fs operations as Diagnostics: `const base = await fs.realpath(root).catch(() => undefined); if (!base) return fail("file", "working directory unavailable")`, wrap stat/readFile in try/catch returning `fail("file", "result file unreadable")`, and swallow `close()` errors in the finally so every OS failure becomes a counted rejection. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:115-129):
Problem: Unguarded fs errors bypass counted rejection
Detail: Every failure in `file()` is funneled into a bounded Diagnostic via `fail()`, except `fs.realpath(root)`, and `handle.stat()`/`handle.readFile()`/`handle.close()` are likewise unguarded. If the working directory is missing (deleted mid-session via the model's own shell, or `Instance.directory` misconfigured) or a read fails with EIO, `execute` rejects with a raw ENOENT/EACCES/EIO error instead of a Diagnostic. That path never invokes `onReject`, so no `structured_output_rejected` event is emitted, `attempts` is not incremented, the corrective budget is bypassed, and the model receives unstructured, unbounded OS error text (embedding the absolute root path) instead of bounded guidance. Repro: working directory removed via shell → model calls StructuredOutput → `fs.realpath(root)` throws ENOENT out of execute; rejection not counted, no `{path:"",keyword:"file"}` diagnostic.
Suggested fix: Wrap the fs operations as Diagnostics: `const base = await fs.realpath(root).catch(() => undefined); if (!base) return fail("file", "working directory unavailable")`, wrap stat/readFile in try/catch returning `fail("file", "result file unreadable")`, and swallow `close()` errors in the finally so every OS failure becomes a counted rejection.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Every failure in file() is funneled into a bounded Diagnostic via fail(), except fs.realpath(root), and handle.stat()/handle.readFile()/handle.close() are likewise unguarded. If the working directory is missing (deleted mid-session via the model's own shell, or Instance.directory misconfigured) or a read fails with EIO, execute rejects with a raw ENOENT/EACCES/EIO error instead of a Diagnostic. That path never invokes onReject, so no structured_output_rejected event is emitted, attempts is not incremented, the corrective budget is bypassed, and the model receives unstructured, unbounded OS error text (embedding the absolute root path) instead of bounded guidance. Repro: working directory removed via shell → model calls StructuredOutput → fs.realpath(root) throws ENOENT out of execute; rejection not counted, no {path:"",keyword:"file"} diagnostic.
const fail = (keyword: string, message: string) => ({ errors: [{ path: "", keyword, message }] })
const base = await fs.realpath(root)
// realpath follows symlinks, so a link pointing outside the root is rejected too.
const target = await fs.realpath(path.resolve(base, input)).catch(() => undefined)
if (!target) return fail("file", "file not found")
const relative = path.relative(base, target)| const target = await fs.realpath(path.resolve(base, input)).catch(() => undefined) | ||
| if (!target) return fail("file", "file not found") | ||
| const relative = path.relative(base, target) | ||
| if (relative.startsWith("..") || path.isAbsolute(relative)) |
There was a problem hiding this comment.
🟡 Containment check falsely rejects in-root ..foo paths.
--- a/packages/cli/src/session/output-schema.ts
+++ b/packages/cli/src/session/output-schema.ts
@@ -118,7 +118,7 @@
if (!target) return fail("file", "file not found")
const relative = path.relative(base, target)
- if (relative.startsWith("..") || path.isAbsolute(relative))
+ if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative))
return fail("file", "path must stay inside the working directory")
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:120-121):
Problem: Containment check falsely rejects in-root ..foo paths
Detail: `relative.startsWith("..")` over-matches: any file or directory inside the working directory whose name begins with `..` (e.g. `..tmp-result.json` or `..data/result.json`) yields a relative path starting with `..` and is rejected with "path must stay inside the working directory" even though realpath already resolved it inside root. The false rejection burns a counted corrective attempt (with `--output-schema-retries 0` it fails the run outright) and the feedback misleads the model, whose only real fix is renaming the file. Repro: model writes a schema-valid result to `<workdir>/..tmp-result.json` and calls StructuredOutput with `{"path":"..tmp-result.json"}` → realpath resolves inside root, `path.relative` returns `..tmp-result.json`, `startsWith("..")` is true → false containment rejection.
Suggested fix: Use the standard containment test that only rejects a true parent traversal: `relative === ".." || relative.startsWith(".." + path.sep) || path.isAbsolute(relative)`.
Suggested patch:
--- a/packages/cli/src/session/output-schema.ts
+++ b/packages/cli/src/session/output-schema.ts
@@ -118,7 +118,7 @@
if (!target) return fail("file", "file not found")
const relative = path.relative(base, target)
- if (relative.startsWith("..") || path.isAbsolute(relative))
+ if (relative === ".." || relative.startsWith(`..${path.sep}`) || path.isAbsolute(relative))
return fail("file", "path must stay inside the working directory")
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
relative.startsWith("..") over-matches: any file or directory inside the working directory whose name begins with .. (e.g. ..tmp-result.json or ..data/result.json) yields a relative path starting with .. and is rejected with "path must stay inside the working directory" even though realpath already resolved it inside root. The false rejection burns a counted corrective attempt (with --output-schema-retries 0 it fails the run outright) and the feedback misleads the model, whose only real fix is renaming the file. Repro: model writes a schema-valid result to <workdir>/..tmp-result.json and calls StructuredOutput with {"path":"..tmp-result.json"} → realpath resolves inside root, path.relative returns ..tmp-result.json, startsWith("..") is true → false containment rejection.
const target = await fs.realpath(path.resolve(base, input)).catch(() => undefined)
if (!target) return fail("file", "file not found")
const relative = path.relative(base, target)
if (relative.startsWith("..") || path.isAbsolute(relative))
return fail("file", "path must stay inside the working directory")
// One handle for stat and read, so the checked file is the file read.| try { | ||
| const stat = await handle.stat() | ||
| if (!stat.isFile()) return fail("file", "path must be a regular file") | ||
| if (stat.size > MAX_FILE_BYTES) return fail("file", `file must not exceed ${MAX_FILE_BYTES} bytes`) |
There was a problem hiding this comment.
🟡 Uncapped read and FIFO open can hang or overread.
Suggested change
| if (stat.size > MAX_FILE_BYTES) return fail("file", `file must not exceed ${MAX_FILE_BYTES} bytes`) | |
| Read with a hard byte limit (`handle.read` into a preallocated MAX_FILE_BYTES+1 buffer, fail with the size diagnostic when exceeded) and open with `fs.constants.O_NONBLOCK` so a FIFO open returns immediately and the subsequent fstat rejects it as non-regular. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:128-129):
Problem: Uncapped read and FIFO open can hang or overread
Detail: Two check-then-act gaps on the result file handle. (1) The 2 MiB cap is enforced via `stat.size` before `handle.readFile("utf8")`, but readFile drains to EOF regardless of the earlier size: a background writer the model started (e.g. `while :; do head -c 100M /dev/zero >> result.json; done &`) can grow the file between stat and read, making the CLI buffer an arbitrarily large string — a model-triggerable memory-exhaustion path despite the cap; the same handle pins file identity, not length. (2) `fs.open(target, "r")` on a FIFO (`mkfifo result.json`) blocks until a writer appears, so `stat.isFile()` is never reached, the execute promise never settles, and the whole headless run hangs with no rejection or timeout.
Suggested fix: Read with a hard byte limit (`handle.read` into a preallocated MAX_FILE_BYTES+1 buffer, fail with the size diagnostic when exceeded) and open with `fs.constants.O_NONBLOCK` so a FIFO open returns immediately and the subsequent fstat rejects it as non-regular.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Two check-then-act gaps on the result file handle. (1) The 2 MiB cap is enforced via stat.size before handle.readFile("utf8"), but readFile drains to EOF regardless of the earlier size: a background writer the model started (e.g. while :; do head -c 100M /dev/zero >> result.json; done &) can grow the file between stat and read, making the CLI buffer an arbitrarily large string — a model-triggerable memory-exhaustion path despite the cap; the same handle pins file identity, not length. (2) fs.open(target, "r") on a FIFO (mkfifo result.json) blocks until a writer appears, so stat.isFile() is never reached, the execute promise never settles, and the whole headless run hangs with no rejection or timeout.
const handle = await fs.open(target, "r").catch(() => undefined)
if (!handle) return fail("file", "file not readable")
try {
const stat = await handle.stat()
if (!stat.isFile()) return fail("file", "path must be a regular file")
if (stat.size > MAX_FILE_BYTES) return fail("file", `file must not exceed ${MAX_FILE_BYTES} bytes`)\n const text = await handle.readFile("utf8")| model?: Provider.Model | ||
| /** The tool takes {path} to the JSON result file, which must stay inside this directory (#140). */ | ||
| root: string | ||
| onReject?: (errors: OutputSchema.Diagnostic[]) => Promise<string> |
There was a problem hiding this comment.
🟡 Optional onReject silently skips retry accounting.
Suggested change
| onReject?: (errors: OutputSchema.Diagnostic[]) => Promise<string> | |
| Make `onReject` required alongside `root` (the PR already ships a breaking change to this input type), or emit the counted rejection inside `execute` before consulting `input.onReject`. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/prompt.ts:1130):
Problem: Optional onReject silently skips retry accounting
Detail: File-content rejections are only counted (attempts incremented, `structured_output_rejected` emitted) when the caller supplies `onReject` — the prompt loop's `rejection()` is the sole counter. The production caller wires it, but the new API makes `onReject` optional while making `root` required: a caller that omits it (a plugin, or the unchanged live-test harness) gets file errors that never increment `attempts`, so the `attempts < retryCount + 1` budget never trips for file rejections and the corrective loop is no longer bounded by the retry count.
Suggested fix: Make `onReject` required alongside `root` (the PR already ships a breaking change to this input type), or emit the counted rejection inside `execute` before consulting `input.onReject`.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
File-content rejections are only counted (attempts incremented, structured_output_rejected emitted) when the caller supplies onReject — the prompt loop's rejection() is the sole counter. The production caller wires it, but the new API makes onReject optional while making root required: a caller that omits it (a plugin, or the unchanged live-test harness) gets file errors that never increment attempts, so the attempts < retryCount + 1 budget never trips for file rejections and the corrective loop is no longer bounded by the retry count.
model?: Provider.Model\n /** The tool takes {path} to the JSON result file, which must stay inside this directory (#140). */\n root: string\n onReject?: (errors: OutputSchema.Diagnostic[]) => Promise<string>\n onSuccess: (output: unknown) => void | Promise<void>\n }): AITool & { id: "StructuredOutput" } {| if ("errors" in loaded) { | ||
| const message = input.onReject | ||
| ? await input.onReject(loaded.errors) | ||
| : OutputSchema.error(loaded.errors).message |
There was a problem hiding this comment.
⚪ Model path echoed unescaped in tool error.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/prompt.ts:1160):
Problem: Model path echoed unescaped in tool error
Detail: `args.path` is model-controlled and PATH_SCHEMA only bounds its length (maxLength 1024, no pattern), so a value containing newlines or ANSI escape sequences is interpolated verbatim into the thrown error and the corrective feedback surfaced in the transcript and logs, enabling line-spoofing in terminal output. No secret leak (the value is model-authored), but it should be sanitized like other echoed inputs.
Suggested fix: JSON-encode or strip control characters from the path in the message, e.g. `` throw new Error(`Fix the file at ${JSON.stringify(args.path)}, ...`) ``.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
args.path is model-controlled and PATH_SCHEMA only bounds its length (maxLength 1024, no pattern), so a value containing newlines or ANSI escape sequences is interpolated verbatim into the thrown error and the corrective feedback surfaced in the transcript and logs, enabling line-spoofing in terminal output. No secret leak (the value is model-authored), but it should be sanitized like other echoed inputs.
| if (!contract) throw new Error("Missing output schema") | ||
| return rejection(OutputSchema.parse(raw, contract.validate)) | ||
| // Only the {path} arguments reach the SDK boundary; file content errors come from execute. | ||
| return rejection(OutputSchema.parse(raw, OutputSchema.compile(OutputSchema.PATH_SCHEMA))) |
There was a problem hiding this comment.
⚪ PATH_SCHEMA compiled twice, once per rejection.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/prompt.ts:742):
Problem: PATH_SCHEMA compiled twice, once per rejection
Detail: PATH_SCHEMA is now compiled in two places: `reject(raw)` compiles a fresh Ajv validator on every malformed-arguments rejection, while `createStructuredOutputTool` compiles its (provider-transformed) `{path}` validator once per tool. Two compilation sites for the same schema, one in a per-call path, diverge from the compile-once pattern used beside it and risk drifting apart.
Suggested fix: Compile PATH_SCHEMA once at module scope (e.g. export a compiled validator from output-schema.ts next to PATH_SCHEMA) and reuse it in both `reject(raw)` and the tool factory.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
PATH_SCHEMA is now compiled in two places: reject(raw) compiles a fresh Ajv validator on every malformed-arguments rejection, while createStructuredOutputTool compiles its (provider-transformed) {path} validator once per tool. Two compilation sites for the same schema, one in a per-call path, diverge from the compile-once pattern used beside it and risk drifting apart.
const reject = (raw: string) => {\n if (!contract) throw new Error("Missing output schema")\n // Only the {path} arguments reach the SDK boundary; file content errors come from execute.\n return rejection(OutputSchema.parse(raw, OutputSchema.compile(OutputSchema.PATH_SCHEMA)))\n }
Code reviewVerdict: Address the major findings before merging. · 🔴 0 · 🟠 1 · 🟡 7 · ⚪ 2 · 0/10 resolved
🤖 Fix all 10 open findings with your agent📋 Out-of-diff findings (10)
Reviewed 12 files · 0 inline · view all 10 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
- open with O_NOFOLLOW (ELOOP is the containment rejection) and re-check that the opened inode is still the file the path resolves to inside the working directory, so a final-component or parent-directory symlink swap after the check cannot publish an outside file - open with O_NONBLOCK so a FIFO is rejected as non-regular instead of hanging the run, and read at most 2 MiB + 1 bytes so a file that grows after stat is rejected by size - containment rejects only real parent traversal, so in-root names such as `..result.json` are accepted - an unavailable working directory or a failed stat/read is a fixed, counted diagnostic; close errors are swallowed - onReject is required; PATH_SCHEMA compiles once; the corrective message JSON-encodes the submitted path - the prose-only `missing` diagnostic uses the file form, like the reminder; EVENTS.md lists every fixed file diagnostic verbatim - live integration tests run in a temporary directory and check the published value is the file passed by path Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gASjVHuvN9AxK5WT8jzsN
Contributor
Author
Review response — PR #142All 10 findings verified TRUE and fixed in commit Issues addressed (pushed to this PR)
Gates on Context for the 🟠 finding: the model also has bash, so it can copy any readable file into the working directory. Path containment is therefore not a confidentiality boundary against the model. It stops a path argument alone (for example, from prompt injection) from publishing an outside file. The swap race was a real gap in that guard, and it is closed. Review claims verified false (no change needed)None. Not addressed hereNone. |
| if (!contains(base, target)) return fail("file", OUTSIDE) | ||
| // O_NOFOLLOW refuses a final component swapped for a symlink after the check (ELOOP). | ||
| // O_NONBLOCK makes opening a FIFO return at once; fstat then rejects it as non-regular. | ||
| const handle = await fs |
There was a problem hiding this comment.
🟡 O_NOFOLLOW/O_NONBLOCK silently no-op on Windows.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:139-140):
Problem: O_NOFOLLOW/O_NONBLOCK silently no-op on Windows
Detail: On win32, Node's `fs.constants.O_NOFOLLOW` and `O_NONBLOCK` are undefined, so `O_RDONLY | undefined | undefined` coerces to plain `O_RDONLY`: the final-component symlink refusal (ELOOP arm) and the non-blocking FIFO open silently no-op on Windows. Path containment still holds via the post-open realpath + dev/ino re-check, but the defense-in-depth the comments claim is POSIX-only, the ELOOP catch arm is dead code on Windows, and the untested-hang protection for named pipes is weakened there. The test suite explicitly supports Windows (FIFO test skips on win32), so this is a supported platform.
Suggested fix: Detect platform support once, e.g. `const NOFOLLOW = process.platform === "win32" ? 0 : fs.constants.O_NOFOLLOW` (same for O_NONBLOCK), and add a comment that Windows containment relies solely on the post-open realpath + dev/ino re-check.
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 win32, Node's fs.constants.O_NOFOLLOW and O_NONBLOCK are undefined, so O_RDONLY | undefined | undefined coerces to plain O_RDONLY: the final-component symlink refusal (ELOOP arm) and the non-blocking FIFO open silently no-op on Windows. Path containment still holds via the post-open realpath + dev/ino re-check, but the defense-in-depth the comments claim is POSIX-only, the ELOOP catch arm is dead code on Windows, and the untested-hang protection for named pipes is weakened there. The test suite explicitly supports Windows (FIFO test skips on win32), so this is a supported platform.
| properties: { | ||
| path: { | ||
| type: "string", | ||
| maxLength: 1024, |
There was a problem hiding this comment.
🟡 PATH_SCHEMA says relative; absolute in-root works.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:100):
Problem: PATH_SCHEMA says relative; absolute in-root works
Detail: The provider-facing `path` property description reads "Path of the JSON result file, relative to the working directory", but `OutputSchema.file()` also accepts absolute paths that resolve inside the root — `packages/cli/README.md` explicitly documents "absolute paths inside it are accepted" and the new test "publishes the validated file contents" asserts an absolute in-root path succeeds. This description is the contract the provider and model see (it is the only schema sent to the provider), so it misstates accepted inputs and can make the model avoid valid absolute paths or distrust the tool description.
Suggested fix: Word the description to match behavior and docs, e.g. "Path of the JSON result file in the working directory (relative, or absolute inside it)".
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 provider-facing path property description reads "Path of the JSON result file, relative to the working directory", but OutputSchema.file() also accepts absolute paths that resolve inside the root — packages/cli/README.md explicitly documents "absolute paths inside it are accepted" and the new test "publishes the validated file contents" asserts an absolute in-root path succeeds. This description is the contract the provider and model see (it is the only schema sent to the provider), so it misstates accepted inputs and can make the model avoid valid absolute paths or distrust the tool description.
| const current = again && contains(base, again) ? await fs.stat(again).catch(() => undefined) : undefined | ||
| if (!current || current.dev !== stat.dev || current.ino !== stat.ino) return fail("file", OUTSIDE) | ||
| const size = `file must not exceed ${MAX_FILE_BYTES} bytes` | ||
| if (stat.size > MAX_FILE_BYTES) return fail("file", size) |
There was a problem hiding this comment.
⚪ 2 MiB zeroing Buffer.alloc on every call.
Suggested change
| if (stat.size > MAX_FILE_BYTES) return fail("file", size) | |
| Use `Buffer.allocUnsafe(MAX_FILE_BYTES + 1)` — safe because only the filled prefix is parsed via subarray — keeping the per-call allocation to avoid concurrent-call aliasing. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #142, packages/cli/src/session/output-schema.ts:154):
Problem: 2 MiB zeroing Buffer.alloc on every call
Detail: Every StructuredOutput invocation (including each corrective retry) allocates and zeroes a 2 MiB+1 buffer via `Buffer.alloc`, but only the filled prefix `[0, length)` is ever used (`fill` reads at most buffer.length; `buffer.subarray(0, length)` is parsed). The zero-fill is unnecessary work repeated per attempt and dominates the read cost for small result files. Impact is small (a single memset per tool call), hence NIT.
Suggested fix: Use `Buffer.allocUnsafe(MAX_FILE_BYTES + 1)` — safe because only the filled prefix is parsed via subarray — keeping the per-call allocation to avoid concurrent-call aliasing.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Every StructuredOutput invocation (including each corrective retry) allocates and zeroes a 2 MiB+1 buffer via Buffer.alloc, but only the filled prefix [0, length) is ever used (fill reads at most buffer.length; buffer.subarray(0, length) is parsed). The zero-fill is unnecessary work repeated per attempt and dominates the read cost for small result files. Impact is small (a single memset per tool call), hence NIT.
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 2 · ⚪ 1 · 0/3 resolved
🤖 Fix all 3 open findings with your agent📋 Out-of-diff findings (3)
Reviewed 13 files · 0 inline · view all 3 findings ↗ aictrl · AI code review for fast-moving teams · aictrl.dev |
…review 2) - the provider-facing path description says relative or absolute inside the working directory, matching file() and the README - O_NOFOLLOW/O_NONBLOCK default to 0 where undefined (win32); containment there rests on the post-open inode re-check, now pinned by a test that opens without the POSIX flags - the 2 MiB + 1 read buffer is allocUnsafe: only the filled prefix is parsed and each call has its own buffer Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019gASjVHuvN9AxK5WT8jzsN
Contributor
Author
Review response — PR #142 (second review)All 3 findings from the second review verified TRUE and fixed in commit Issues addressed (pushed to this PR)
Gates on Review claims verified false (no change needed)None. Not addressed hereNone. |
This was referenced Oct 10, 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.
Summary
In
--output-schemamode the model already writes and checks its result file, then has to re-emit the whole object asStructuredOutputtool arguments. The CLI published that re-emitted copy and never read the file. On Gemini the provider's schema conversion dropsadditionalProperties, so map-typed fields came back as{}(#139). In prod this emptied every diagram'selementsin all 5 Gemini PR Explanation runs on 2026-10-10. The re-emission also cost about 5–9k output tokens per run.With this change,
StructuredOutputtakes only{ "path": "<file>" }. The CLI reads that file, validates it against the canonical schema with Ajv, and publishes the parsed file. Only{ path }reaches the provider, so provider schema conversion can no longer drop result fields.BREAKING (schema-mode callers only)
--output-schemamode (andformat: json_schemaprompts over the server API),StructuredOutputaccepts only{ "path": "<file>" }. Passing the result object as tool arguments is rejected at the tool boundary.--dir, or the attached server's session directory) and pass that file's path.$RESULT" must say "write the result to a file and call StructuredOutput with its path". For the aictrl app this meansprompt-builder.ts(structured paragraph),pr-explanation-output-contract.ts:45,48and theexplain-changeskill (step "call it with the contents of$RESULT"), and checking that$RESULTsits inside the CLI's--dir. The executor pins the CLI version, so nothing changes until that pin is bumped together with those prompt edits.structured_output_rejecteddiagnostics (all listed verbatim in EVENTS.md): keywordfilewithworking directory unavailable,file not found,path must stay inside the working directory,file not readable,path must be a regular fileorfile must not exceed 2097152 bytes, and keywordparsewithfile must contain complete, valid JSON. The NDJSON v1 schema already treats enum-like strings as an open set.missingdiagnostic message and the corrective reminder now both say: "write the final result as JSON to a file in the working directory, then call StructuredOutput with its path". Consumers that match the oldmissingmessage must update.Not affected: runs without
--output-schema(no StructuredOutput tool),--output-resultwriting, exit codes, event shapes, the retry budget.Note on the issue's acceptance criteria: #140 listed "backward compatible with the inline-args form". We dropped that deliberately. Keeping the inline form would leave the lossy path open to any model that picks it. There would also be two contracts to document and test. Schema mode now has exactly one form.
Design
OutputSchema.PATH_SCHEMAis{ path: string (maxLength 1024) }withadditionalProperties: false. It is the tool's input schema, transformed per provider as before.OutputSchema.file(root, path, validate):realpath, so a symlink that points outside is rejected;..,../…or absolute), so..result.jsonis accepted;O_NOFOLLOW(ELOOPis the containment rejection) andO_NONBLOCK(a FIFO is rejected as non-regular instead of hanging);statandread, and reads at most 2 MiB + 1 bytes, so a file that grows afterstatis rejected by size;rejection(): they are counted against--output-schema-retries, emitstructured_output_rejected, and return to the model as the tool errorFix the file at "<JSON-encoded path>", then call StructuredOutput again with its path. <diagnostics>. A small edit to the file replaces a full re-emission.{ path }object) still go through the SDK repair path (invalidtool) and are counted, as before.createStructuredOutputToolhas a single form.rootandonRejectare required, andPATH_SCHEMAis compiled once (OutputSchema.shape).Evidence
The reproduction from #139 (6-key map, keys
queued->running,[*]->queued,TASKS.org_id,"1",A,done->[*]), with the model told to writeexpected.jsontoresult.jsonand call StructuredOutput with that path. The POC commit was built as a single binary. Vertex requests went through a logging pass-through proxy.minPropertieselements: {}2/2 (silent loss)minProperties: 1minPropertiesminProperties: 110/10 runs published all 6 keys identical to
expected.json, and Ajv accepted every published file. Every run took one attempt.On the wire, the Vertex
StructuredOutputfunction declaration carries only{ path }.The live runs were on the POC commit (ed45294e4b). This PR then deleted the dead inline branch (production never reached it), added
maxLength: 1024topath, and reworded the prose-only reminder.The PR head was then rebuilt and re-run on the same reproduction (
PR-*runs):minPropertiesminProperties: 1On the wire, the Vertex declaration is
{"required":["path"],"type":"object","properties":{"path":{"type":"string",...}}}. The Gemini converter dropsmaxLength, as expected, and the local Ajv check still enforces it.Tests
test/session/structured-output.test.ts: tool factory tests rewritten for the single form. They cover:{ path }only, and the canonical schema is description text without$schema;{}, a non-string path, extra fields and paths over 1024 characters;../, symlink);Review fixes (f72bd0b): regression tests for these cases:
stat;..-prefixed in-root names;The swap tests swap inside
fs.open, between the check and the open. We confirmed each security test fails with its fix removed:O_NOFOLLOW: both swap tests fail;O_NONBLOCK: the FIFO test hangs and fails;..test fails.The live
structured-output-integration.test.ts(gated onANTHROPIC_API_KEY) now runs in a temporary git directory. It asserts that StructuredOutput's input is{ path }and that the file equals the published value. No key is available locally or in CI, so it is type-checked only.test/session/output-schema.test.tsandtest/cli/run-output-schema.test.ts: the fixture providers write each would-be argument payload to a file and call StructuredOutput with its path. The existing budget, rejection, parallel-call, prose-only, Gemini-transform and--output-resulttests now run through the file path. The Gemini test asserts that only{ path }reaches the provider while the canonical numeric enum stays authoritative.test/session/prompt-tool-loop.test.ts: the follow-up StructuredOutput call passes a result file.Gates, run locally on the final tree:
bun run typecheck(workspace, turbo): 6/6 tasks pass;bun turbo test --concurrency=100%(the CI command): 1677 pass, 7 skip, 0 fail on the review-fix head;prettier --checkon the changed files: clean;bun run script/build.ts --single: builds, and the binary was used for thePR-*runs above.Closes #140
Refs #139 #141
🤖 Generated with Claude Code
https://claude.ai/code/session_019gASjVHuvN9AxK5WT8jzsN