Repository navigation
feat: select work tools per headless run - #254
Merged
Merged
Conversation
byapparov
changed the base branch from
fix/issue-244-tool-guidance-20261021
to
main
October 21, 2026 02:50
byapparov
changed the base branch from
main
to
fix/issue-244-tool-guidance-20261021
October 21, 2026 03:12
This was referenced Oct 21, 2026
byapparov
marked this pull request as ready for review
October 21, 2026 10:00
![aictrl-dev[bot]](https://avatars.githubusercontent.com/in/2817739?s=60&v=4){kind=small}
| await fail( | ||
| "Work-tool selection is not supported with --attach: this client cannot verify server-side enforcement. Remove --tools and local tool_allowlist or run locally.", | ||
| "INVOCATION_INVALID_ARGUMENTS", | ||
| ) |
There was a problem hiding this comment.
🟡 Attach preflight fetch failure crashes runs unguarded.
Suggested change
| ) | |
| Wrap the readToolSelection preflight in try/catch and route failures through fail(...) with a clear message (or treat discovery errors as "selection unknown → fail closed"), and give the well-known fetch a timeout so offline hosts degrade gracefully instead of rejecting the await chain. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/cli/cmd/run.ts:1221-1224):
Problem: Attach preflight fetch failure crashes runs unguarded
Detail: Every `aictrl run --attach` now awaits Config.readToolSelection(process.cwd()) → resolve(false), which runs Auth.all(), filesystem walks, and an unconditional fetch() of each well-known auth URL, with no try/catch. A network failure of that fetch (or any malformed config making load() throw) rejects the preflight as an unstructured error even when the user configured no tool selection at all — previously attach worked without touching local config. README sanctions failing on "invalid local configuration", but an unreachable well-known host now also breaks attach runs that used to work, bypassing the structured fail(..., "INVOCATION_INVALID_ARGUMENTS") path.
Suggested fix: Wrap the readToolSelection preflight in try/catch and route failures through fail(...) with a clear message (or treat discovery errors as "selection unknown → fail closed"), and give the well-known fetch a timeout so offline hosts degrade gracefully instead of rejecting the await chain.
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 aictrl run --attach now awaits Config.readToolSelection(process.cwd()) → resolve(false), which runs Auth.all(), filesystem walks, and an unconditional fetch() of each well-known auth URL, with no try/catch. A network failure of that fetch (or any malformed config making load() throw) rejects the preflight as an unstructured error even when the user configured no tool selection at all — previously attach worked without touching local config. README sanctions failing on "invalid local configuration", but an unreachable well-known host now also breaks attach runs that used to work, bypassing the structured fail(..., "INVOCATION_INVALID_ARGUMENTS") path.
invocation.phase("bootstrap")
if (args.attach) {
// This checkout has no receiver/capability contract that can enforce selection.
const configured = await Config.readToolSelection(process.cwd())
if (configured !== undefined) {|
|
||
| const token = setup ? await Control.token() : undefined | ||
| if (token) { | ||
| } |
There was a problem hiding this comment.
🟡 Empty if(token){} dead block kept on rewritten line.
Suggested change
| } | |
| Delete the empty `if (token) {}` block and the token binding if nothing consumes it. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/config/config.ts:137-138):
Problem: Empty if(token){} dead block kept on rewritten line
Detail: The refactor rewrites the feeding statement to `const token = setup ? await Control.token() : undefined` but keeps the pre-existing empty `if (token) {}` branch directly below it. The block is dead in both modes now — in readonly mode token is always undefined, and in setup mode Control.token() runs purely to feed an empty branch. Since the PR already touches both lines, this is the natural moment to remove it.
Suggested fix: Delete the empty `if (token) {}` block and the token binding if nothing consumes 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 refactor rewrites the feeding statement to const token = setup ? await Control.token() : undefined but keeps the pre-existing empty if (token) {} branch directly below it. The block is dead in both modes now — in readonly mode token is always undefined, and in setup mode Control.token() runs purely to feed an empty branch. Since the PR already touches both lines, this is the natural moment to remove it.
const token = setup ? await Control.token() : undefined
if (token) {
}| @@ -1,3 +1,4 @@ | |||
| import { ToolSelection } from "../tool/selection" | |||
There was a problem hiding this comment.
🟡 Relative imports drift from the @/ alias convention.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/compaction.ts:1):
Problem: Relative imports drift from the @/ alias convention
Detail: The PR adds relative imports for the new module in several files — compaction.ts:1 and message-v2.ts:1 ("../tool/selection"), prompt.ts:1-3 ("../config/config", "../tool/batch", "../tool/selection"), tool-catalog.ts (all-relative), and run.ts:2-3 ("../../tool/selection", "../../config/config") — while the surrounding/adjacent imports in those same files use the package's "@/" alias, and the same PR imports the identical module as "@/tool/selection" in session/llm.ts and "@/config/config" in cli/cmd/generate.ts. Same modules, two specifier styles in one PR; the alias works for these paths (llm.ts proves it), so this looks like drift, not cycle avoidance.
Suggested fix: Switch the new imports to "@/tool/selection", "@/config/config", "@/tool/batch", and "@/session/tool-catalog" in compaction.ts, message-v2.ts, prompt.ts, tool-catalog.ts, and run.ts to match the established alias convention.
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 PR adds relative imports for the new module in several files — compaction.ts:1 and message-v2.ts:1 ("../tool/selection"), prompt.ts:1-3 ("../config/config", "../tool/batch", "../tool/selection"), tool-catalog.ts (all-relative), and run.ts:2-3 ("../../tool/selection", "../../config/config") — while the surrounding/adjacent imports in those same files use the package's "@/" alias, and the same PR imports the identical module as "@/tool/selection" in session/llm.ts and "@/config/config" in cli/cmd/generate.ts. Same modules, two specifier styles in one PR; the alias works for these paths (llm.ts proves it), so this looks like drift, not cycle avoidance.
import { ToolSelection } from "../tool/selection"
import { BusEvent } from "@/bus/bus-event"
import { Bus } from "@/bus"| if (failed.toolCall.toolName === "StructuredOutput" && input.structured) { | ||
| input.structured.repairs.add(failed.toolCall.toolCallId) | ||
| const error = await input.structured.reject(failed.toolCall.input) | ||
| if (!tools.invalid || !ToolGuidance.isProtocol(tools.invalid)) return null |
There was a problem hiding this comment.
🟡 structured.reject runs even when repair returns null.
Suggested change
| if (!tools.invalid || !ToolGuidance.isProtocol(tools.invalid)) return null | |
| Evaluate the guard first and only await structured.reject(...) on the path that actually returns the repair to the "invalid" tool. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/llm.ts:188-189):
Problem: structured.reject runs even when repair returns null
Detail: The StructuredOutput repair path now awaits input.structured.reject(...) before the new protocol guard, so the rejection (events/retry accounting) is recorded even when the function then returns null because the native "invalid" tool is absent — e.g. a custom .aictrl/tool/invalid.ts shadows the builtin. Previously reject() only executed when a repair was actually returned, so one bad call now produces both a persisted rejection and an unrepaired tool-call error, skewing corrective-retry accounting.
Suggested fix: Evaluate the guard first and only await structured.reject(...) on the path that actually returns the repair to the "invalid" 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
The StructuredOutput repair path now awaits input.structured.reject(...) before the new protocol guard, so the rejection (events/retry accounting) is recorded even when the function then returns null because the native "invalid" tool is absent — e.g. a custom .aictrl/tool/invalid.ts shadows the builtin. Previously reject() only executed when a repair was actually returned, so one bad call now produces both a persisted rejection and an unrepaired tool-call error, skewing corrective-retry accounting.
input.structured.repairs.add(failed.toolCall.toolCallId)
const error = await input.structured.reject(failed.toolCall.input)
if (!tools.invalid || !ToolGuidance.isProtocol(tools.invalid)) return null
return {| const selection: NonNullable<Tool.Context["selection"]> = { | ||
| names, | ||
| restricted: snapshot.allowlist !== null, | ||
| assert(name, batch = false) { |
There was a problem hiding this comment.
🟡 Selection facade duplicated 3x with divergent messages.
Suggested change
| assert(name, batch = false) { | |
| Add a ToolSelection.facade(names, catalog, opts) factory in packages/cli/src/tool/selection.ts and use it from prompt.ts (both the subtask stub and resolveTools) and tool/registry.ts so assert messages and freezing stay identical. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/prompt.ts:975-982):
Problem: Selection facade duplicated 3x with divergent messages
Detail: Three hand-rolled copies of the context.selection facade now exist: the pending-subtask one in run() (with a stub execute that always throws), the resolveTools one (with batch checks), and the re-wrap in tool/registry.ts for plugin contexts. The two assert error strings have already diverged ("is excluded by this invocation's tool selection/permissions" vs "is unavailable or excluded by ..."), and each copy freezes/duplicates names differently. A shared factory would keep behavior and messages aligned as this surface evolves.
Suggested fix: Add a ToolSelection.facade(names, catalog, opts) factory in packages/cli/src/tool/selection.ts and use it from prompt.ts (both the subtask stub and resolveTools) and tool/registry.ts so assert messages and freezing stay identical.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Three hand-rolled copies of the context.selection facade now exist: the pending-subtask one in run() (with a stub execute that always throws), the resolveTools one (with batch checks), and the re-wrap in tool/registry.ts for plugin contexts. The two assert error strings have already diverged ("is excluded by this invocation's tool selection/permissions" vs "is unavailable or excluded by ..."), and each copy freezes/duplicates names differently. A shared factory would keep behavior and messages aligned as this surface evolves.
const selection: NonNullable<Tool.Context["selection"]> = {
names,
restricted: snapshot.allowlist !== null,
assert(name, batch = false) {
if (!names.includes(name))
throw new Error(`Tool '${name}' is unavailable or excluded by this invocation's tool selection/permissions`)| await Bun.write(path.join(directory, ".aictrl/tool/invalid.ts"), collision) | ||
| }, | ||
| }) | ||
| expect(result.requests.length).toBeGreaterThanOrEqual(1) |
There was a problem hiding this comment.
🟡 Unfalsifiable >=1 request-count assertions.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/test/cli/run-tool-selection.test.ts:1197-1198):
Problem: Unfalsifiable >=1 request-count assertions
Detail: identity-unknown-repair (and the same pattern around lines 1214, 1279, and 1291) asserts `result.requests.length` >= 1, which passes for any positive request count. A regression causing a runaway repair loop, repeated inference rounds, or duplicated sessions still passes; every other test in the file asserts exact counts. These should pin the expected round count.
Suggested fix: Replace with exact counts (e.g. expect(result.requests).toHaveLength(2) for parent + repair round) as done in identity-native-invalid.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
identity-unknown-repair (and the same pattern around lines 1214, 1279, and 1291) asserts result.requests.length >= 1, which passes for any positive request count. A regression causing a runaway repair loop, repeated inference rounds, or duplicated sessions still passes; every other test in the file asserts exact counts. These should pin the expected round count.
expect(result.requests.length).toBeGreaterThanOrEqual(1)
expect(result.markers["work-marker.json"]).toBeNull()| ? undefined | ||
| : await Promise.resolve() | ||
| .then(() => ToolSelection.parse(requested)) | ||
| .catch((error) => fail(error.message, "INVOCATION_INVALID_ARGUMENTS")) |
There was a problem hiding this comment.
⚪ Parse error masks clearer --attach rejection.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/cli/cmd/run.ts:493-497):
Problem: Parse error masks clearer --attach rejection
Detail: ToolSelection.parse(requested) runs before the `args.attach && requested !== undefined` rejection, so `aictrl run --attach URL --tools "read ,bash"` reports the whitespace parse error instead of the more decisive "work-tool selection is not supported with --attach" message, which applies to any --tools value combined with --attach. Both fail before configuration discovery, so the documented ordering guarantee holds — only the reported message is suboptimal.
Suggested fix: Move the attach+requested check above the parse: a selector combined with --attach is invalid regardless of its syntax.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
ToolSelection.parse(requested) runs before the args.attach && requested !== undefined rejection, so aictrl run --attach URL --tools "read ,bash" reports the whitespace parse error instead of the more decisive "work-tool selection is not supported with --attach" message, which applies to any --tools value combined with --attach. Both fail before configuration discovery, so the documented ordering guarantee holds — only the reported message is suboptimal.
if (args.attach && requested !== undefined) {
await fail(
"Work-tool selection is not supported with --attach: this client cannot verify server-side enforcement. Remove --tools or run locally.",
"INVOCATION_INVALID_ARGUMENTS",
)
}|
|
||
| // Mirror only dynamic canonical environment reads. Module-load Flag constants | ||
| // must not acquire different precedence in read-only configuration discovery. | ||
| export function settings(environment?: Record<string, string>) { |
There was a problem hiding this comment.
⚪ Mixed home/config path sources in discovery pass.
Suggested change
| export function settings(environment?: Record<string, string>) { | |
| Thread the overlay through a parameterized Global.Path (config + home) so every read in one resolve pass shares precedence, or document that settings()/home() overlay keys are limited to those the well-known loop can actually set. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/config/paths.ts:28-40):
Problem: Mixed home/config path sources in discovery pass
Detail: directories() mixes path sources: the first entry is module-load Global.Path.config (never overlay-aware) while the home walk start/stop uses the overlay-aware config.home, and loadGlobal() likewise reads Global.Path.config directly. A well-known overlay value for HOME/AICTRL_TEST_HOME (the mechanism this PR adds for AICTRL_CONFIG_DIR/AICTRL_CONFIG_CONTENT) would therefore diverge global-config reads from home-walk reads within the same resolve() pass. No current writer populates HOME in the overlay, so this is latent rather than live.
Suggested fix: Thread the overlay through a parameterized Global.Path (config + home) so every read in one resolve pass shares precedence, or document that settings()/home() overlay keys are limited to those the well-known loop can actually set.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
directories() mixes path sources: the first entry is module-load Global.Path.config (never overlay-aware) while the home walk start/stop uses the overlay-aware config.home, and loadGlobal() likewise reads Global.Path.config directly. A well-known overlay value for HOME/AICTRL_TEST_HOME (the mechanism this PR adds for AICTRL_CONFIG_DIR/AICTRL_CONFIG_CONTENT) would therefore diverge global-config reads from home-walk reads within the same resolve() pass. No current writer populates HOME in the overlay, so this is latent rather than live.
export function settings(environment?: Record<string, string>) {
const disabled = environment?.AICTRL_DISABLE_PROJECT_CONFIG
return {
disableProject:
disabled === undefined ? Flag.AICTRL_DISABLE_PROJECT_CONFIG : ["true", "1"].includes(disabled.toLowerCase()),
configDir: environment?.AICTRL_CONFIG_DIR ?? Flag.AICTRL_CONFIG_DIR,
home: (environment?.AICTRL_TEST_HOME ?? process.env.AICTRL_TEST_HOME) || home(environment),| ...(input.format?.type === "json_schema" ? ["StructuredOutput"] : []), | ||
| ] | ||
| ToolSelection.validate( | ||
| input.toolAllowlist ?? (ceiling === undefined ? (preference ?? null) : null), |
There was a problem hiding this comment.
⚪ Config allowlist: strict at top, silent under child.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/prompt.ts:1278-1280):
Problem: Config allowlist: strict at top, silent under child
Detail: The same config tool_allowlist value is strictly validated (throws on unknown/unavailable names) when no ceiling is present, but silently intersected — possibly to nothing — under a child ceiling. Since config is global while MCP catalogs are per-project/per-model, a config naming a project-specific tool hard-fails every top-level prompt in other projects (or while that MCP server is down), while the identical value inherited by child Task sessions is quietly narrowed to empty. The code comment declares the split intended, but the dual semantics are surprising enough to deserve explicit documentation or a warning when an intersection empties.
Suggested fix: Document the split in the README's selection section, or log a warning when a non-empty inherited/config selection intersects to an empty effective set.
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 same config tool_allowlist value is strictly validated (throws on unknown/unavailable names) when no ceiling is present, but silently intersected — possibly to nothing — under a child ceiling. Since config is global while MCP catalogs are per-project/per-model, a config naming a project-specific tool hard-fails every top-level prompt in other projects (or while that MCP server is down), while the identical value inherited by child Task sessions is quietly narrowed to empty. The code comment declares the split intended, but the dual semantics are surprising enough to deserve explicit documentation or a warning when an intersection empties.
ToolSelection.validate(\n input.toolAllowlist ?? (ceiling === undefined ? (preference ?? null) : null),\n [...catalog.entries.keys()].filter((name) => !controls.includes(name)),\n catalog.known,\n controls,\n )| controls, | ||
| ) | ||
| } | ||
| if (selection.allowlist) Object.freeze(selection.allowlist) |
There was a problem hiding this comment.
⚪ Empty allowlist array escapes Object.freeze.
--- a/packages/cli/src/session/prompt.ts
+++ b/packages/cli/src/session/prompt.ts
@@ -1284,1 +1284,1 @@
- if (selection.allowlist) Object.freeze(selection.allowlist)
+ if (selection.allowlist !== null) Object.freeze(selection.allowlist)
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/prompt.ts:1284-1285):
Problem: Empty allowlist array escapes Object.freeze
Detail: `if (selection.allowlist) Object.freeze(selection.allowlist)` skips the freeze when the allowlist is [] (falsy) — exactly the `--tools ''` case — and the following Object.freeze(selection) is shallow, so the persisted snapshot's empty allowlist stays mutable in memory. No mutation path exists today, but the guard contradicts the facade's immutable-names contract and is a one-token fix.
Suggested fix: Use `if (selection.allowlist !== null) Object.freeze(selection.allowlist)` so the empty-array selection is frozen like every other allowlist.
Suggested patch:
--- a/packages/cli/src/session/prompt.ts
+++ b/packages/cli/src/session/prompt.ts
@@ -1284,1 +1284,1 @@
- if (selection.allowlist) Object.freeze(selection.allowlist)
+ if (selection.allowlist !== null) Object.freeze(selection.allowlist)
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
if (selection.allowlist) Object.freeze(selection.allowlist) skips the freeze when the allowlist is [] (falsy) — exactly the --tools '' case — and the following Object.freeze(selection) is shallow, so the persisted snapshot's empty allowlist stays mutable in memory. No mutation path exists today, but the guard contradicts the facade's immutable-names contract and is a one-token fix.
if (selection.allowlist) Object.freeze(selection.allowlist)
Object.freeze(selection)
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 14 · ⚪ 5 · 0/19 resolved
🤖 Fix all 19 open findings with your agent📋 Out-of-diff findings (19)
Reviewed 26 files · 0 inline · view all 19 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
byapparov
changed the base branch from
fix/issue-244-tool-guidance-20261021
to
main
October 21, 2026 12:39
Member
Author
Review response — PR #254Addressed the 19-finding review with two bounded corrections in Issues addressed
Validation: 179 passed, 4 existing skips, 0 failed, 1,019 assertions; CLI typecheck passed; eight attach controls passed. Actual ready-barrier deadline, early-exit and TERM-fault controls prove owned child/grandchild absence before manual intervention. Baseline failures and a hostile external-adopter negative control are retained. An external adopter withholding waitpid can retain its own zombie; the helper reports bounded cleanup failure, and does not claim arbitrary orphan reaping or Windows coverage. Compatible receiver and real-model evaluation gates remain open. Full verdict table
Not addressedBroader package-manager coverage in the private test guard is deferred for a scoped policy/control review (manager-owned, first review 23 October). The current installer uses the covered Bun path. These partial fetch/spawn controls are not an OS network sandbox; no broader confinement claim is made. No new provider/network timeout, permission widening, Task auto-inclusion or broad facade/import refactor is introduced. Verified falsePending-Task recovery must not widen selection; canonical test timeout is 30 seconds; generic errors already reach structured reporting; one malformed structured call is not double-counted; active selections intentionally validate before noReply persistence; selection never grants denied permissions; empty arrays are truthy and already frozen. The table uses exact returned native finding IDs. All native githubCommentId values were null, so identity was verified against each unique review, file, line, native severity, title and full claim/suggestion. The native-only compaction row is included even though the older parser omitted it. |
byapparov
changed the base branch from
main
to
fix/issue-244-tool-guidance-20261021
October 21, 2026 12:44
| ? undefined | ||
| : await Promise.resolve() | ||
| .then(() => ToolSelection.parse(requested)) | ||
| .catch((error) => fail(error.message, "INVOCATION_INVALID_ARGUMENTS")) |
There was a problem hiding this comment.
🟡 Invalid --tools prints raw ZodError JSON to user.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/cli/cmd/run.ts:493):
Problem: Invalid --tools prints raw ZodError JSON to user
Detail: toolAllowlist is computed via Promise.resolve().then(() => ToolSelection.parse(requested)).catch((error) => fail(error.message, …)). ToolSelection.parse throws a zod ZodError (Names.parse on the split array), and ZodError.message is the serialized JSON issues array, so `aictrl run --tools "read, bash"` (or "a,,b") prints a raw JSON blob on stderr instead of the authored "Tool names must not contain surrounding whitespace" text, unlike every neighbouring hand-written fail() message in this handler. The same raw ZodError JSON reaches users via Config.readToolSelection (Names.parse) on the --attach preflight path.
Suggested fix: Map zod issues to readable text before failing, e.g. .catch((error) => fail(error instanceof z.ZodError ? error.issues.map((i) => i.message).join("; ") : String(error), "INVOCATION_INVALID_ARGUMENTS")), or have ToolSelection.parse throw a plain Error carrying the first issue message. A plain try/catch would also drop the odd Promise.resolve().then wrapper.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
toolAllowlist is computed via Promise.resolve().then(() => ToolSelection.parse(requested)).catch((error) => fail(error.message, …)). ToolSelection.parse throws a zod ZodError (Names.parse on the split array), and ZodError.message is the serialized JSON issues array, so aictrl run --tools "read, bash" (or "a,,b") prints a raw JSON blob on stderr instead of the authored "Tool names must not contain surrounding whitespace" text, unlike every neighbouring hand-written fail() message in this handler. The same raw ZodError JSON reaches users via Config.readToolSelection (Names.parse) on the --attach preflight path.
const toolAllowlist =
requested === undefined
? undefined
: await Promise.resolve()
.then(() => ToolSelection.parse(requested))
.catch((error) => fail(error.message, "INVOCATION_INVALID_ARGUMENTS"))
if (args.attach && requested !== undefined) {| if (options.setup) { | ||
| await Filesystem.writeJson(path.join(Global.Path.config, "config.json"), result) | ||
| await fs.unlink(legacy) | ||
| } |
There was a problem hiding this comment.
🟡 Readonly attach preflight still imports legacy config JS.
Suggested change
| } | |
| Hoist the whole legacy branch behind `if (options.setup)` so readonly discovery never imports/executes the legacy module (it would then simply not contribute config in readonly mode), or read the legacy data without executing code. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/config/config.ts:1224):
Problem: Readonly attach preflight still imports legacy config JS
Detail: In loadGlobal, the legacy `Global.Path.config/config` module is dynamically imported unconditionally; only the write-back (writeJson + unlink) is guarded by options.setup. The readonly discovery pass used as the --attach preflight (resolve(false) via Config.readToolSelection) therefore still executes the legacy config module — arbitrary user JS with top-level side effects — during what config.ts and the README describe as a pass that "never … initializes plugins/dependencies or rewrites config". Executing user config code is a side effect the preflight invariant claims not to have (normal runs import it too, but there it is expected setup).
Suggested fix: Hoist the whole legacy branch behind `if (options.setup)` so readonly discovery never imports/executes the legacy module (it would then simply not contribute config in readonly mode), or read the legacy data without executing code.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
In loadGlobal, the legacy Global.Path.config/config module is dynamically imported unconditionally; only the write-back (writeJson + unlink) is guarded by options.setup. The readonly discovery pass used as the --attach preflight (resolve(false) via Config.readToolSelection) therefore still executes the legacy config module — arbitrary user JS with top-level side effects — during what config.ts and the README describe as a pass that "never … initializes plugins/dependencies or rewrites config". Executing user config code is a side effect the preflight invariant claims not to have (normal runs import it too, but there it is expected setup).
result = mergeConfig(result, rest)\n if (options.setup) {\n await Filesystem.writeJson(path.join(Global.Path.config, \"config.json\"), result)\n await fs.unlink(legacy)\n }| abort, | ||
| callID: part.callID, | ||
| extra: { bypassAgentCheck: true }, | ||
| selection: { |
There was a problem hiding this comment.
🟡 Subtask selection facade misses Object.freeze.
--- a/packages/cli/src/session/prompt.ts
+++ b/packages/cli/src/session/prompt.ts
@@ -578,7 +578,7 @@
- selection: {
+ selection: Object.freeze({
names,
restricted: selection.allowlist !== null,
assert(name) {
@@ -586,7 +586,7 @@
async execute() {
throw new Error("Nested direct dispatch is unavailable before subtask setup")
},
- },
+ }),
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/prompt.ts:578):
Problem: Subtask selection facade misses Object.freeze
Detail: Of the three selection facades this PR constructs, resolveTools calls Object.freeze(selection) and registry.ts wraps its plugin copy in Object.freeze({...}), but the pending-subtask facade injected into the Tool.Context in run() is left mutable. Anything holding that ctx (including TaskTool, which derives the child ceiling from ctx.selection?.restricted/names) can replace assert/names/restricted before the ceiling is computed, contradicting the README contract "The facade and names are immutable" and continuing the per-copy drift already noted for this triplicated facade.
Suggested fix: Wrap the literal in Object.freeze (names already comes from the frozen ToolCatalog.names array); better, extract the shared frozen facade factory the duplication finding asks for.
Suggested patch:
--- a/packages/cli/src/session/prompt.ts
+++ b/packages/cli/src/session/prompt.ts
@@ -578,7 +578,7 @@
- selection: {
+ selection: Object.freeze({
names,
restricted: selection.allowlist !== null,
assert(name) {
@@ -586,7 +586,7 @@
async execute() {
throw new Error("Nested direct dispatch is unavailable before subtask setup")
},
- },
+ }),
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Of the three selection facades this PR constructs, resolveTools calls Object.freeze(selection) and registry.ts wraps its plugin copy in Object.freeze({...}), but the pending-subtask facade injected into the Tool.Context in run() is left mutable. Anything holding that ctx (including TaskTool, which derives the child ceiling from ctx.selection?.restricted/names) can replace assert/names/restricted before the ceiling is computed, contradicting the README contract "The facade and names are immutable" and continuing the per-copy drift already noted for this triplicated facade.
extra: { bypassAgentCheck: true },
selection: {
names,
restricted: selection.allowlist !== null,
assert(name) {
if (!names.includes(name))
throw new Error(`Tool '${name}' is excluded by this invocation's tool selection/permissions`)
},
async execute() {
throw new Error("Nested direct dispatch is unavailable before subtask setup")
},
},| if (catalog) { | ||
| const controls = [ | ||
| ...ToolCatalog.controls(catalog), | ||
| ...(input.format?.type === "json_schema" ? ["StructuredOutput"] : []), |
There was a problem hiding this comment.
🟡 Schema-mode controls match by name, not identity.
Suggested change
| ...(input.format?.type === "json_schema" ? ["StructuredOutput"] : []), | |
| Either qualify the README sentence ("outside schema mode, custom tools with those names are still work tools") or make the controls exclusion identity-aware (exclude only the harness-created StructuredOutput definition, not the name) in ToolCatalog.names/ToolSelection.validate. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/src/session/prompt.ts:1275):
Problem: Schema-mode controls match by name, not identity
Detail: Schema-mode controls are matched by bare name (contract ? ["StructuredOutput"] : [], both here and in run()'s ToolCatalog.names call, plus validate()'s controls), while identity checks elsewhere correctly use ToolGuidance.isProtocol/builtin markers. A user-defined custom tool literally named "StructuredOutput" (a valid work tool outside schema mode — the PR's own test declares it "executes as work outside schema mode") is therefore excluded from effective names and rejected with "is a protocol/control tool, not a selectable work tool" whenever --output-schema is used. This contradicts the README's blanket claim "Custom tools with those names are still work tools; protocol identity belongs to the harness-created definition."
Suggested fix: Either qualify the README sentence ("outside schema mode, custom tools with those names are still work tools") or make the controls exclusion identity-aware (exclude only the harness-created StructuredOutput definition, not the name) in ToolCatalog.names/ToolSelection.validate.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Schema-mode controls are matched by bare name (contract ? ["StructuredOutput"] : [], both here and in run()'s ToolCatalog.names call, plus validate()'s controls), while identity checks elsewhere correctly use ToolGuidance.isProtocol/builtin markers. A user-defined custom tool literally named "StructuredOutput" (a valid work tool outside schema mode — the PR's own test declares it "executes as work outside schema mode") is therefore excluded from effective names and rejected with "is a protocol/control tool, not a selectable work tool" whenever --output-schema is used. This contradicts the README's blanket claim "Custom tools with those names are still work tools; protocol identity belongs to the harness-created definition."
const controls = [\n ...ToolCatalog.controls(catalog),\n ...(input.format?.type === \"json_schema\" ? [\"StructuredOutput\"] : []),\n ]\n ToolSelection.validate(| .split("\n") | ||
| .filter(Boolean) | ||
| .map((line) => JSON.parse(line)) | ||
| .find((event) => event.type === "tool_use" && event.part.tool === "delegate") |
There was a problem hiding this comment.
🟡 JSON.parse of all stdout lines masks real regressions.
Suggested change
| .find((event) => event.type === "tool_use" && event.part.tool === "delegate") | |
| Parse defensively: collect per-line parse failures and expect(parsedFailures).toEqual([]) (or throw new Error("unparsable stdout: " + result.stdout) before the deep-equal), so unexpected CLI output surfaces as an actionable assertion rather than a SyntaxError. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/test/cli/run-tool-selection.test.ts:1148):
Problem: JSON.parse of all stdout lines masks real regressions
Detail: The public-facade/MCP tests JSON.parse every stdout line (and critical-noreply does JSON.parse(result.stdout.trim())). Any stray non-JSON stdout line the CLI ever emits (version notice, deprecation banner, partial flush) makes JSON.parse throw an opaque SyntaxError instead of an expect failure showing the actual output — masking the real regression (unexpected stdout content, or a changed role/model shape) and hiding the deep-equal diff that would explain it.
Suggested fix: Parse defensively: collect per-line parse failures and expect(parsedFailures).toEqual([]) (or throw new Error("unparsable stdout: " + result.stdout) before the deep-equal), so unexpected CLI output surfaces as an actionable assertion rather than a SyntaxError.
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 public-facade/MCP tests JSON.parse every stdout line (and critical-noreply does JSON.parse(result.stdout.trim())). Any stray non-JSON stdout line the CLI ever emits (version notice, deprecation banner, partial flush) makes JSON.parse throw an opaque SyntaxError instead of an expect failure showing the actual output — masking the real regression (unexpected stdout content, or a changed role/model shape) and hiding the deep-equal diff that would explain it.
const event = result.stdout\n .split(\"\\n\")\n .filter(Boolean)\n .map((line) => JSON.parse(line))\n .find((event) => event.type === \"tool_use\" && event.part.tool === \"delegate\")| .filter(Boolean) | ||
| .map((line) => JSON.parse(line) as { origin: string; path: string; allowed: boolean }) | ||
| if (input.codex) | ||
| requests.push( |
There was a problem hiding this comment.
⚪ Deadline error undercounts codex requests.
Suggested change
| requests.push( | |
| Inside deadlineFailure() (or the deadline catch), read and count codexLog lines before formatting the message so the diagnostics include intercepted codex requests. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/test/cli/run-tool-selection.test.ts:391):
Problem: Deadline error undercounts codex requests
Detail: When the deadline promise wins the race, run() throws deadlineFailure() immediately and the codexLog merge into `requests` (plus the network-ledger assertion) never executes. For codex runs the thrown "providerRequests=N" therefore only counts local server requests: a hung Codex child reports providerRequests=0 even though the codex capture file may contain the stuck request, actively misleading triage of exactly the hangs this deadline exists to diagnose.
Suggested fix: Inside deadlineFailure() (or the deadline catch), read and count codexLog lines before formatting the message so the diagnostics include intercepted codex requests.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
When the deadline promise wins the race, run() throws deadlineFailure() immediately and the codexLog merge into requests (plus the network-ledger assertion) never executes. For codex runs the thrown "providerRequests=N" therefore only counts local server requests: a hung Codex child reports providerRequests=0 even though the codex capture file may contain the stuck request, actively misleading triage of exactly the hangs this deadline exists to diagnose.
if (input.codex)\n requests.push(\n ...(await Bun.file(codexLog).text())\n .split(\"\\n\")\n .filter(Boolean)\n .map((line) => JSON.parse(line) as Request),\n )| sessionID: event.sessionID, | ||
| messageID: event.part.messageID, | ||
| agent: expect.any(String), | ||
| directory: path.dirname(result.command[3]), |
There was a problem hiding this comment.
⚪ command[3] is preload path; assertion passes by accident.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/test/cli/run-tool-selection.test.ts:1153):
Problem: command[3] is preload path; assertion passes by accident
Detail: The public-mcp assertion expects directory: path.dirname(result.command[3]). The command array is [execPath, "run", "--preload", preload, "--conditions=browser", selected, …], so command[3] is the network-guard preload file, not the CLI entry. It passes only because dirname(preload) happens to equal the child cwd (tmp.path). The index reads as the entry path, and moving the preload file into a subdirectory would break the assertion spuriously — or stop comparing against the actual cwd.
Suggested fix: Have run() expose the spawn cwd (e.g. result.cwd) and assert against it explicitly instead of deriving it from an unrelated command-array index.
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 public-mcp assertion expects directory: path.dirname(result.command[3]). The command array is [execPath, "run", "--preload", preload, "--conditions=browser", selected, …], so command[3] is the network-guard preload file, not the CLI entry. It passes only because dirname(preload) happens to equal the child cwd (tmp.path). The index reads as the entry path, and moving the preload file into a subdirectory would break the assertion spuriously — or stop comparing against the actual cwd.
expect(result.markers[\"caller-marker.json\"]).toMatchObject({\n sessionID: event.sessionID,\n messageID: event.part.messageID,\n agent: expect.any(String),\n directory: path.dirname(result.command[3]),|
|
||
| async function run(input: Input) { | ||
| const fixturePath = [path.dirname(process.execPath), "/usr/local/bin", "/usr/bin", "/bin"].join(path.delimiter) | ||
| const ripgrep = Bun.which("rg", { PATH: fixturePath }) |
There was a problem hiding this comment.
⚪ Missing ripgrep fails all ~75 tests, no skip path.
Suggested change
| const ripgrep = Bun.which("rg", { PATH: fixturePath }) | |
| Hoist the which() check to module scope and gate the suites with describe.skipIf(!ripgrep) (keeping the actionable message in the skip reason), or fail once in a module-level setup test. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/test/cli/run-tool-selection.test.ts:62):
Problem: Missing ripgrep fails all ~75 tests, no skip path
Detail: Bun.which("rg") failure throws inside run(), so in an environment without ripgrep every test in this ~75-test file fails individually with the same error rather than skipping or failing once at module scope. That floods CI/local output with duplicates (and can swallow unrelated failures in the same run), and re-runs the which() lookup per test.
Suggested fix: Hoist the which() check to module scope and gate the suites with describe.skipIf(!ripgrep) (keeping the actionable message in the skip reason), or fail once in a module-level setup test.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
Bun.which("rg") failure throws inside run(), so in an environment without ripgrep every test in this ~75-test file fails individually with the same error rather than skipping or failing once at module scope. That floods CI/local output with duplicates (and can swallow unrelated failures in the same run), and re-runs the which() lookup per test.
const ripgrep = Bun.which(\"rg\", { PATH: fixturePath })\n if (!ripgrep)\n throw new Error(\n \"Selector CLI fixtures require a real ripgrep executable on the child PATH. On Ubuntu install ripgrep, then verify rg --version; fixture network downloads are forbidden.\",\n )| const chunks = [ | ||
| { | ||
| id: "selector-fixture", | ||
| object: "chat.completion.chunk", |
There was a problem hiding this comment.
⚪ Scripted call reads stale requests.at(-1) after barrier.
Suggested change
| object: "chat.completion.chunk", | |
| Capture the parsed request in a local const before any barrier await (const req = await request.json(); requests.push(req)) and pass that request to the scripted callback instead of re-reading requests.at(-1). |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/cli/test/cli/run-tool-selection.test.ts:112):
Problem: Scripted call reads stale requests.at(-1) after barrier
Detail: The fixture server pushes the parsed request, may await a runtime barrier (concurrent-sessions, sibling-children, cancel-model, overlap-child-resume), and only afterwards shifts the scripted call and evaluates it against requests.at(-1) — which after a barrier await is whichever request landed last across all in-flight sessions, not the one being answered. It currently returns the intended null only because that stale request happens to contain an assistant message; any future non-null scripted response for a barrier-held request gets cross-wired to the wrong session, yielding wrong tool ceilings and false results.
Suggested fix: Capture the parsed request in a local const before any barrier await (const req = await request.json(); requests.push(req)) and pass that request to the scripted callback instead of re-reading requests.at(-1).
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The fixture server pushes the parsed request, may await a runtime barrier (concurrent-sessions, sibling-children, cancel-model, overlap-child-resume), and only afterwards shifts the scripted call and evaluates it against requests.at(-1) — which after a barrier await is whichever request landed last across all in-flight sessions, not the one being answered. It currently returns the intended null only because that stale request happens to contain an assistant message; any future non-null scripted response for a barrier-held request gets cross-wired to the wrong session, yielding wrong tool ceilings and false results.
requests.push((await request.json()) as Request)\n if (input.runtime === \"concurrent-sessions\") {\n if (requests.length >= 2) releaseConcurrent?.()\n await concurrentBarrier\n }\n…\n const scripted = calls.shift()\n const call = typeof scripted === \"function\" ? scripted(requests) : scripted| readonly restricted: boolean | ||
| assert(name: string): void | ||
| execute(name: string, args: unknown): Promise<{ title: string; output: string; metadata: Record<string, unknown> }> | ||
| } |
There was a problem hiding this comment.
⚪ Plugin selection.execute type omits attachments.
Suggested change
| } | |
| Add `attachments?: unknown[]` (or the SDK file-part shape) to the plugin selection.execute return type so the public facade matches Tool.Context.selection. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #254, packages/plugin/src/tool.ts:25):
Problem: Plugin selection.execute type omits attachments
Detail: The internal Tool.Context.selection.execute resolves to { title, metadata, output, attachments?: FilePart-like[] }, but the published plugin ToolContext.selection.execute return type omits attachments. The runtime object (registry.ts delegates to the CLI facade) does return attachments for tools that produce them, so plugins performing nested dispatch lose them from the type level — a shape-parity gap between the internal and public facades this PR introduces.
Suggested fix: Add `attachments?: unknown[]` (or the SDK file-part shape) to the plugin selection.execute return type so the public facade matches Tool.Context.selection.
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 internal Tool.Context.selection.execute resolves to { title, metadata, output, attachments?: FilePart-like[] }, but the published plugin ToolContext.selection.execute return type omits attachments. The runtime object (registry.ts delegates to the CLI facade) does return attachments for tools that produce them, so plugins performing nested dispatch lose them from the type level — a shape-parity gap between the internal and public facades this PR introduces.
selection?: {\n readonly names: readonly string[]\n readonly restricted: boolean\n assert(name: string): void\n execute(name: string, args: unknown): Promise<{ title: string; output: string; metadata: Record<string, unknown> }>\n }
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 7 · ⚪ 6 · 0/13 resolved
🤖 Fix all 13 open findings with your agent📋 Out-of-diff findings (13)
Reviewed 27 files · 0 inline · view all 13 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
|
All 13 fresh findings from review Three fixed, three deferred, seven ignored (one false premise).
The legacy loader explicitly requests TOML parsing; see Bun’s TOML import-attribute documentation. Its read-only policy input remains available without migration writes. Verification: the normal main integration passed 179 selector tests/four existing skips, 230 argv/stdin tests and 19 actual local-provider cases / 22 requests, workspace/wire checks, eight attach controls and three owned lifecycle controls. The follow-up changes exactly three files; the other 594 tracked entries retain the tested merge bytes. The changed public 20 ms canary passes ordinary cleanup and correctly fails the injected cleanup fault at its real AggregateError assertion. Canonical attachment consumers/negative shapes/internal-public compatibility and direct CLI/SDK/plugin compilers pass. Both scopes passed independent review. Normal push hooks passed all six tasks with zero cache hits, including SDK generation/plugin build; all 597 tracked bytes and Git modes were verified unchanged afterward. Current-head CodeQL passed with zero results/errors/warnings. Hosted CI39611443485 passed 2,257 tests / 10 skips / 0 failures, with 15,719 assertions across 156 files. Actual checkout 380e994 has the same tree as published 46f98d5; all 597 tracked blob hashes and Git modes match. Six scheduled five-minute polls ended at 14:19:53 UTC without a current-head formal review. The review gate remains pending; this observation is not approval. Receiver/C1, paid-model quality/billing, native/Windows, egress-sandbox and release gates remain open. |
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
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.
Refs #246.
Intent
Headless jobs currently declare the full work-tool catalog unless callers encode selection as permission denials. Add a persistent
tool_allowlistin the existing configuration, withaictrl run --tools <exact,names>as a per-invocation override.Expected Impact on Users
Workflow authors can define the tools each job needs in configuration: a review or explanation step can omit editing and delegation tools without changing permission policy. Omitted selection keeps the default catalog; an explicit empty list exposes no work tools. Selection only narrows availability and never grants denied permissions. This candidate remains unreleased; reference-token measurements from the earlier selector candidate are qualified below.
Configuration and overrides
Put the workflow default in the existing
aictrl.jsonoraictrl.jsonc:{ "tool_allowlist": ["read", "glob", "grep"] }Run without repeating that list:
aictrl run "Review this repository"Override it for one invocation when needed:
Precedence is explicit
--tools→ final merged configtool_allowlist→ default catalog. Lists replace rather than merge; the override does not change the next invocation's configuration.[]or--tools ''selects no work tools. ExistingAICTRL_CONFIGsupports a job-specific configuration file; this adds a field to the existing system.Behavior
context.selectionfacade. Nested dispatch results retain optional file/symbol/resource attachment metadata.StructuredOutputis reserved for the harness protocol; a custom tool with that name remains selectable as work outside schema mode. Selection never grants protocol permission.Boundaries
Explicit or locally configured selection with
--attachfails before receiver work because this client cannot verify remote enforcement. Discovery failures retain structured terminal events. Successful remote selection is not implemented.Shell/plugin host code remains trusted; selection and fixture fetch/spawn guards are not an OS sandbox. Caller attachments and command-template expansion remain input preparation. Real-model C1 evaluation and supported receiver compatibility remain open; issue #246 stays open. No provider billing/cache savings, model-quality, native/Windows or release clearance is claimed.
Diff size and review scope
At this head versus current main: 25 files, +6,828 / -383 lines.
Generated output and tests account for89.5% of additions. The largest generated expansion is the canonical v2
Configdeclaration (3,267 lines), plusUserMessage: the SDK build bridge now aliases the legacy types to those canonical schemas. This is a public type-authority change requiring review, alongside the tool-selection fields. The generation inputs/build bridge govern the checked-in output.session/prompt.tswas already1,711 lines on main; this PR adds162/removes30, leaving1,843 lines. Its structural decomposition and repeated selection views are being assessed separately; no broad refactor or behavior change is included by this description update.Current verification
Current corrective commit:
46f98d5993ad56ec033506d5e66f5da85ccc6179, following normal main-integration merge26c53b22120b997568d02e60659eb758a5fa1cf9(parents prior correctionede414a and main4e56b02c).380e9942combines main4e56b02cwith this head; its tree exactly equals published46f98d59, with all 597 tracked blob hashes and Git modes verified. Current-head CodeQL passed with zero results/errors/warnings. Six scheduled five-minute polls ended at 14:19:53 UTC without a current-head formal review. The review gate remains pending; this observation is not approval. Production logs show an automatic-review skip for this PR at its configured two-execution ceiling. The projected logs omit the head SHA, so they do not directly bind the skip to this commit. The ceiling is intentional policy; no counter reset, limit change or manual re-review was triggered. No temporary-base bridge is used.Review dispositions
Original 19-finding response: two fixes, one tracked deferral and16 ignores; all 19 verdicts were written/read back. Seven false low-severity findings were automatically dismissed.
Fresh review 5600212158, submitted 21 October at 13:12:17 UTC onede414a, has 13 additional findings: three fixes, three manager-owned P3 follow-ups for 23 October and seven ignores. The three fixes above are source/QA accepted. Full 13-finding response is posted; all 13 exact verdicts were written and read back, with zero failures. Existing 19 verdicts remain unchanged.
Earlier scripted measurements and hosted verification
The earlier same-binary selector experiment used five default and five selected runs with four model calls/three actual reads in every positive short task. Median first-input reference tokens were 3,459→2,019, session input 15,150→9,390 (−38.02%), output 59→59 and total 15,209→9,449 (−37.87%). These are offline
o200k_baseestimates from the earlier measured candidate, not fresh measurements of46f98d, provider charges or task-quality evidence. Timing was diagnostic, with no speed claim. Reproduction, raw data and exact source linkage.The earlier ede414a CI39601479686 passed 2,024 tests / 10 skips / 0 failures and CodeQL passed. Its captured checkout55eb6a80 combined older main6c3b5483 with ede414a, so it does not validate the current main integration. The initial fresh-runner ripgrep download failure remains in CI39535319219; CI now provisions real ripgrep while retaining the fixture network guard.
Local evidence distinguishes scripted provider/source execution from native release qualification. Workspace typecheck invoked SDK/plugin build dependencies during integration QA: tracked source and SDK bytes stayed exact, while ignored SDK output physical modes changed 0644→0664 under the host umask. Follow-up checks used direct no-emit compilers. An external adopter withholding waitpid can retain its own zombie; that historical case remains a bounded cleanup failure, not arbitrary orphan reaping.