Repository navigation
Conversation
6 of 8 tasks
byapparov
changed the base branch from
main
to
test/issue-248-token-budgets-20261021
October 21, 2026 06:59
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}
| const server = Bun.serve({ | ||
| hostname: "127.0.0.1", | ||
| port: 0, | ||
| async fetch(request) { |
There was a problem hiding this comment.
🟡 Spawn/kill harness and userText() copied 4x.
Suggested change
| async fetch(request) { | |
| Extract the spawn/deadline/escalation/staged-stdin block and userText() into a shared module under packages/cli/test/cli/fixture/ and import it from the four suites, keeping only per-file Input knobs. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, packages/cli/test/cli/run-command-arguments.test.ts:21-213):
Problem: Spawn/kill harness and userText() copied 4x
Detail: This file defines a local capture() plus a byte-identical userText(), while test/cli/fixture/command-template-capture.ts and test/cli/fixture/integrated-capture.ts each carry the same ~150-line spawn/process-group/deadline/staged-stdin harness and the same userText(); run-stdin.test.ts re-implements it again as run(). Four near-identical harnesses (~700 overlapping lines) will drift independently (e.g. command-template-capture asserts expect(code).toBe(0) while integrated-capture does not; only some filter stdout lines to JSON candidates). A shared test/cli/fixture helper for spawn+group-kill+stdin-staging and one exported userText would match the repo's existing fixture-module pattern.
Suggested fix: Extract the spawn/deadline/escalation/staged-stdin block and userText() into a shared module under packages/cli/test/cli/fixture/ and import it from the four suites, keeping only per-file Input knobs.
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 file defines a local capture() plus a byte-identical userText(), while test/cli/fixture/command-template-capture.ts and test/cli/fixture/integrated-capture.ts each carry the same ~150-line spawn/process-group/deadline/staged-stdin harness and the same userText(); run-stdin.test.ts re-implements it again as run(). Four near-identical harnesses (~700 overlapping lines) will drift independently (e.g. command-template-capture asserts expect(code).toBe(0) while integrated-capture does not; only some filter stdout lines to JSON candidates). A shared test/cli/fixture helper for spawn+group-kill+stdin-staging and one exported userText would match the repo's existing fixture-module pattern.
|
|
||
| The marker can appear before or after `--`; repeated markers still read stdin once. Markers are removed from the model message, command arguments and derived session title. `--` ends option parsing, so `run -- -` also selects stdin. An operand such as `"literal -"` remains ordinary text. An explicit empty operand (`run ""`) does not select implicit stdin. Selected stdin is read to EOF and appended after one actual newline. Ordinary prompt operands are joined with spaces without adding artificial quotes or backslash escapes; quotes, backslashes and whitespace inside an operand remain its text. | ||
|
|
||
| Option values belong to their options: `--model=-` or `--command=-` does not select stdin, and scalar options also accept a separated hyphen value. Use `--file=-` to attach a file literally named `-`; `--file -` is missing its required file value. Multiple file values and repeated `--skill` flags retain their existing behavior. Required option values are checked by the argument parser; explicit `--title ""` remains supported. |
There was a problem hiding this comment.
🟡 README hyphen-value claim untrue for --port.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, README.md:66):
Problem: README hyphen-value claim untrue for --port
Detail: README states "scalar options also accept a separated hyphen value", but this PR's own test (packages/cli/test/cli/run-stdin.test.ts, "optional no-value port leaves standalone marker selected") proves `--port -` does NOT consume the hyphen: `--port` is declared `type: "number"` without `requiresArg`, so the bare `-` stays a message operand and selects stdin. Only options carrying `requiresArg` (model, command, session, agent, format, output-schema, output-result, file, title, attach, dir, variant, output-schema-retries) own a following hyphen value. The sentence over-generalizes and can mislead users into expecting `--port -` to bind.
Suggested fix: Qualify the sentence, e.g. "options that require a value (--model, --command, --title, --variant, …) also accept a separated hyphen value; optional-value flags such as --port do not consume one, so a following bare `-` still selects stdin."
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
README states "scalar options also accept a separated hyphen value", but this PR's own test (packages/cli/test/cli/run-stdin.test.ts, "optional no-value port leaves standalone marker selected") proves --port - does NOT consume the hyphen: --port is declared type: "number" without requiresArg, so the bare - stays a message operand and selects stdin. Only options carrying requiresArg (model, command, session, agent, format, output-schema, output-result, file, title, attach, dir, variant, output-schema-retries) own a following hyphen value. The sentence over-generalizes and can mislead users into expecting --port - to bind.
| - name: Install pinned public release without package scripts | ||
| shell: bash | ||
| run: | | ||
| npm install --prefix "$RUNNER_TEMP/aictrl-owned-cli" \ |
There was a problem hiding this comment.
⚪ Version/SHA pins duplicated across three files.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, .github/workflows/released-smoke.yml:33-42):
Problem: Version/SHA pins duplicated across three files
Detail: The pinned release version (@aictrl/cli@0.5.4) and binary SHA-256 f12a5425… are duplicated verbatim in .github/workflows/released-smoke.yml (lines 33-42), examples/released-smoke/workflow.example.yml, and examples/released-smoke/README.md with no cross-reference between the three. The active/example split is documented and deliberate, but any version bump must be edited in three places or the workflows and docs silently diverge from the artifact actually smoked.
Suggested fix: Add a "keep in sync with …" cross-reference comment in both workflow files and the README pin block, or source the version/SHA from a single shared file the runner reads.
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 pinned release version (@aictrl/cli@0.5.4) and binary SHA-256 f12a5425… are duplicated verbatim in .github/workflows/released-smoke.yml (lines 33-42), examples/released-smoke/workflow.example.yml, and examples/released-smoke/README.md with no cross-reference between the three. The active/example split is documented and deliberate, but any version bump must be edited in three places or the workflows and docs silently diverge from the artifact actually smoked.
| ) | ||
| }, 100), | ||
| ) | ||
| } |
There was a problem hiding this comment.
⚪ requestsBeforeEOF races the server's async push.
Suggested change
| } | |
| Push a placeholder into `requests` at request start (before awaiting request.json()), or re-check requests.length after a short settle delay before asserting 0. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, packages/cli/test/cli/fixture/command-template-capture.ts:148):
Problem: requestsBeforeEOF races the server's async push
Detail: The "no provider request before stdin EOF" guard reads requestsBeforeEOF = requests.length inside a setTimeout, but the Bun.serve fetch handler only pushes into `requests` after `await request.json()` resolves (line 31). There is no happens-before edge guaranteeing a request the CLI already sent before EOF is visible in the array when the timer fires, so a genuine read-stdin-after-provider-request violation could transiently be counted as 0 and pass the expect(requestsBeforeEOF).toBe(0) assertion. Same pattern is replicated in integrated-capture.ts and run-stdin.test.ts.
Suggested fix: Push a placeholder into `requests` at request start (before awaiting request.json()), or re-check requests.length after a short settle delay before asserting 0.
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 "no provider request before stdin EOF" guard reads requestsBeforeEOF = requests.length inside a setTimeout, but the Bun.serve fetch handler only pushes into requests after await request.json() resolves (line 31). There is no happens-before edge guaranteeing a request the CLI already sent before EOF is visible in the array when the timer fires, so a genuine read-stdin-after-provider-request violation could transiently be counted as 0 and pass the expect(requestsBeforeEOF).toBe(0) assertion. Same pattern is replicated in integrated-capture.ts and run-stdin.test.ts.
| : first && typeof first === "object" && "cmd" in first && Array.isArray(first.cmd) | ||
| ? first.cmd | ||
| : undefined | ||
| if (command && /(?:^|\/)bun$/.test(String(command[0])) && ["install", "add"].includes(String(command[1]))) { |
There was a problem hiding this comment.
⚪ Install guard misses bun aliases and other package managers.
Suggested change
| if (command && /(?:^|\/)bun$/.test(String(command[0])) && ["install", "add"].includes(String(command[1]))) { | |
| Normalize the first two command tokens and match a denylist: /^(?:.*\/)?(?:bun|bunx|npm|npx|pnpm|yarn)$/ for cmd[0] plus ['install','add','i','a','inst','exec','x'] for cmd[1]. |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, packages/cli/test/cli/fixture/stdin-guard.ts:13-14):
Problem: Install guard misses bun aliases and other package managers
Detail: The preload guard meant to prove the CLI-under-test never installs packages only blocks cmd[0] matching /(?:^|\/)bun$/ with cmd[1] exactly "install" or "add". "bun i"/"bun a" aliases and npm/npx/pnpm/yarn/bunx invocations pass through unblocked, and since the fetch patch only covers the current process, a spawned package manager has unguarded network access. This weakens the egress/install invariant the surrounding tests assert on (any network.some(allowed===false) throw).
Suggested fix: Normalize the first two command tokens and match a denylist: /^(?:.*\/)?(?:bun|bunx|npm|npx|pnpm|yarn)$/ for cmd[0] plus ['install','add','i','a','inst','exec','x'] for cmd[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 preload guard meant to prove the CLI-under-test never installs packages only blocks cmd[0] matching /(?:^|/)bun$/ with cmd[1] exactly "install" or "add". "bun i"/"bun a" aliases and npm/npx/pnpm/yarn/bunx invocations pass through unblocked, and since the fetch patch only covers the current process, a spawned package manager has unguarded network access. This weakens the egress/install invariant the surrounding tests assert on (any network.some(allowed===false) throw).
const original = globalThis.fetch\nconst spawn = Bun.spawn.bind(Bun)\nBun.spawn = ((...args: Parameters<typeof Bun.spawn>) => {\n const first: unknown = args[0]\n const command = Array.isArray(first)\n ? first\n : first && typeof first === "object" && "cmd" in first && Array.isArray(first.cmd)\n ? first.cmd\n : undefined\n if (command && /(?:^|\\/)bun$/.test(String(command[0])) && [\"install\", \"add\"].includes(String(command[1]))) {| function failure(result: Awaited<ReturnType<typeof run>>, code: string) { | ||
| expect(result.code, result.stderr + result.stdout).toBe(1) | ||
| expect(result.requests).toHaveLength(0) | ||
| const events = result.stdout |
There was a problem hiding this comment.
⚪ failure() JSON.parses every stdout line, no filter.
Suggested change
| const events = result.stdout | |
| Filter to JSON candidate lines before parsing, as run-command-arguments.test.ts does: .filter((line) => line.startsWith("{")). |
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, packages/cli/test/cli/run-stdin.test.ts:266-270):
Problem: failure() JSON.parses every stdout line, no filter
Detail: failure() parses stdout with .split("\n").filter(Boolean).map(JSON.parse) — unlike the sibling suites which filter to lines starting with "{". Any non-JSON stdout line emitted by the CLI or Bun runtime on these failing paths (warning, notice, partial NDJSON chunk boundary) makes JSON.parse throw a SyntaxError, turning the intended exit-code/INVOCATION_* classification assertion into an opaque test crash.
Suggested fix: Filter to JSON candidate lines before parsing, as run-command-arguments.test.ts does: .filter((line) => line.startsWith("{")).
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
failure() parses stdout with .split("\n").filter(Boolean).map(JSON.parse) — unlike the sibling suites which filter to lines starting with "{". Any non-JSON stdout line emitted by the CLI or Bun runtime on these failing paths (warning, notice, partial NDJSON chunk boundary) makes JSON.parse throw a SyntaxError, turning the intended exit-code/INVOCATION_* classification assertion into an opaque test crash.
Code reviewVerdict: Looks good — only minor / nit comments below. · 🔴 0 · 🟠 0 · 🟡 2 · ⚪ 4 · 0/6 resolved
🤖 Fix all 6 open findings with your agent📋 Out-of-diff findings (6)
Reviewed 31 files · 0 inline · view all 6 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
Review response — PR #257The six findings from review Issues addressedRequest-arrival observations now precede asynchronous body parsing in all three staged-stdin helpers. The README distinguishes required-value options from optional
Not addressedBroader package-manager alias coverage shares the existing manager-owned P2 guard-policy follow-up for 23 October. Fetch/spawn instrumentation is partial and is not an OS network sandbox. Pin synchronization and broad helper extraction are optional maintenance work, with no current pin mismatch or remaining behavior defect demonstrated by those findings. Verified falseStrict parsing of every collected stdout line deliberately detects NDJSON violations. Filtering warnings away would weaken the protocol check; transport chunks are accumulated before line splitting. Test plan
Configured templates/plugins remain trusted executable code. Receiver compatibility, attached lifecycle, Windows and the 18 real paired C1 trials remain open. No native-binary, release or paid-model qualification is claimed. Native findings returned no GitHub comment IDs. Each exact native ID was resolved by its unique review/file/line/native severity/title and full claim; no GitHub comment ID was used as a persistence target. |
| } | ||
| files.push(path.join(Global.Path.config, "AGENTS.md")) | ||
| files.push(path.join(Instance.directory, ".opencode", "AGENTS.md")) | ||
| if (!Flag.AICTRL_DISABLE_PROJECT_CONFIG) { |
There was a problem hiding this comment.
🟡 Project-instruction isolation fails open by default.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, packages/cli/src/session/instruction.ts:22):
Problem: Project-instruction isolation fails open by default
Detail: The new AICTRL_DISABLE_PROJECT_CONFIG gating is strictly opt-in: with the env var unset (the default — Flag parses it via truthy(), and the headless entrypoint never defaults it on), root AGENTS.md, .opencode/AGENTS.md and nested AGENTS.md from the repo checkout are still auto-injected into the system prompt of an agent that may hold CI credentials. A missed or misspelled env var therefore silently re-enables untrusted project instructions, and nothing is logged or asserted at startup that isolation engaged. The PR docs acknowledge the residual risk (they tell CI to keep instruction-file deletion workarounds), so this is a hardening recommendation — fail closed for unattended/headless runs and/or emit a startup event listing attached instruction files — rather than a regression introduced by the diff.
Suggested fix: For unattended use, default the gate ON in the headless/run entrypoint (e.g. treat headless as project-config-disabled unless AICTRL_ENABLE_PROJECT_CONFIG is set), or at minimum emit a startup log/structured event enumerating the instruction files attached (or stating discovery is disabled) so CI jobs can assert the isolation state instead of trusting a silently-absent env var.
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new AICTRL_DISABLE_PROJECT_CONFIG gating is strictly opt-in: with the env var unset (the default — Flag parses it via truthy(), and the headless entrypoint never defaults it on), root AGENTS.md, .opencode/AGENTS.md and nested AGENTS.md from the repo checkout are still auto-injected into the system prompt of an agent that may hold CI credentials. A missed or misspelled env var therefore silently re-enables untrusted project instructions, and nothing is logged or asserted at startup that isolation engaged. The PR docs acknowledge the residual risk (they tell CI to keep instruction-file deletion workarounds), so this is a hardening recommendation — fail closed for unattended/headless runs and/or emit a startup event listing attached instruction files — rather than a regression introduced by the diff.
files.push(path.join(Flag.AICTRL_CONFIG_DIR, "AGENTS.md"))
}
files.push(path.join(Global.Path.config, "AGENTS.md"))
if (!Flag.AICTRL_DISABLE_PROJECT_CONFIG) {
files.push(path.join(Instance.directory, ".opencode", "AGENTS.md"))
}
return files
}| The marker can appear before or after `--`; repeated markers still read stdin once. Markers are removed from the model message, command arguments and derived session title. `--` ends option parsing, so `run -- -` also selects stdin. An operand such as `"literal -"` remains ordinary text. An explicit empty operand (`run ""`) does not select implicit stdin. Selected stdin is read to EOF and appended after one actual newline. Ordinary prompt operands are joined with spaces without adding artificial quotes or backslash escapes; quotes, backslashes and whitespace inside an operand remain its text. | ||
|
|
||
| Option values belong to their options: `--model=-` or `--command=-` does not select stdin, and scalar options also accept a separated hyphen value. Use `--file=-` to attach a file literally named `-`; `--file -` is missing its required file value. Multiple file values and repeated `--skill` flags retain their existing behavior. Required option values are checked by the argument parser; explicit `--title ""` remains supported. | ||
| Required-value options own a separated hyphen: `--model -` and `--command -` do not select stdin, nor do their `=` forms. Optional `--port` leaves a separated `-` as the stdin operand (`--port -`). Use `--file=-` to attach a file literally named `-`; `--file -` is missing its required file value. Multiple file values and repeated `--skill` flags retain their existing behavior. Required option values are checked by the argument parser; explicit `--title ""` remains supported. |
There was a problem hiding this comment.
🟡 Hyphen rule vs --file sentence contradict each other.
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, README.md:66):
Problem: Hyphen rule vs --file sentence contradict each other
Detail: The rewritten paragraph now states the general rule "Required-value options own a separated hyphen", but two sentences later retains "`--file -` is missing its required file value" — and --file is itself a required-value option. Either the new rule over-generalizes across option shapes (scalar vs multi-value file options), or the older --file sentence is wrong; as written the paragraph contradicts itself, the same over-generalization class as the just-fixed --port claim. Readers cannot tell which options bind a separated "-" without testing.
Suggested fix: Qualify the rule to the option shape it holds for, e.g. "Scalar required-value options own a separated hyphen: `--model -` and `--command -` do not select stdin, nor do their `=` forms. Multi-value options do not: `--file -` is missing its required file value — use `--file=-` to attach a file literally named `-`." (Match whichever behavior the parser actually has for --file.)
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The rewritten paragraph now states the general rule "Required-value options own a separated hyphen", but two sentences later retains "--file - is missing its required file value" — and --file is itself a required-value option. Either the new rule over-generalizes across option shapes (scalar vs multi-value file options), or the older --file sentence is wrong; as written the paragraph contradicts itself, the same over-generalization class as the just-fixed --port claim. Readers cannot tell which options bind a separated "-" without testing.
The marker can appear before or after `--`; repeated markers still read stdin once. Markers are removed from the model message, command arguments and derived session title. `--` ends option parsing, so `run -- -` also selects stdin. An operand such as `"literal -"` remains ordinary text. An explicit empty operand (`run ""`) does not select implicit stdin. Selected stdin is read to EOF and appended after one actual newline. Ordinary prompt operands are joined with spaces without adding artificial quotes or backslash escapes; quotes, backslashes and whitespace inside an operand remain its text.
Required-value options own a separated hyphen: `--model -` and `--command -` do not select stdin, nor do their `=` forms. Optional `--port` leaves a separated `-` as the stdin operand (`--port -`). Use `--file=-` to attach a file literally named `-`; `--file -` is missing its required file value. Multiple file values and repeated `--skill` flags retain their existing behavior. Required option values are checked by the argument parser; explicit `--title ""` remains supported.
| @@ -0,0 +1,405 @@ | |||
| import { expect } from "bun:test" | |||
There was a problem hiding this comment.
⚪ Unused expect import and template const in new fixture.
--- a/packages/cli/test/cli/fixture/integrated-capture.ts
+++ b/packages/cli/test/cli/fixture/integrated-capture.ts
@@ -1,9 +1,7 @@
-import { expect } from "bun:test"
import { spawn, spawnSync } from "node:child_process"
import fs from "node:fs/promises"
import { existsSync, readFileSync } from "node:fs"
import os from "node:os"
import { Database } from "bun:sqlite"
import path from "node:path"
-const template = "FIRST<$1>\nSECOND<$2>\nREST<$3>"
const guard = path.join(import.meta.dir, "stdin-guard.ts")
🤖 Fix with your agent
Fix this code review finding (aictrl-dev/cli PR #257, packages/cli/test/cli/fixture/integrated-capture.ts:1):
Problem: Unused expect import and template const in new fixture
Detail: The new fixture imports `expect` from "bun:test" but never calls it (all assertions live in run-integrated-headless.test.ts, unlike sibling command-template-capture.ts), and declares `const template = "FIRST<$1>\nSECOND<$2>\nREST<$3>"` that is never referenced — the config uses an inline "STATIC<!…>" template string instead. Both are copy-paste residue from command-template-capture.ts that misleads readers into thinking the fixture asserts something or exercises the FIRST/SECOND/REST template.
Suggested fix: Delete the unused `expect` import and the unused `template` constant.
Suggested patch:
--- a/packages/cli/test/cli/fixture/integrated-capture.ts
+++ b/packages/cli/test/cli/fixture/integrated-capture.ts
@@ -1,9 +1,7 @@
-import { expect } from "bun:test"
import { spawn, spawnSync } from "node:child_process"
import fs from "node:fs/promises"
import { existsSync, readFileSync } from "node:fs"
import os from "node:os"
import { Database } from "bun:sqlite"
import path from "node:path"
-const template = "FIRST<$1>\nSECOND<$2>\nREST<$3>"
const guard = path.join(import.meta.dir, "stdin-guard.ts")
Implement the fix on the PR head branch and add a regression test that fails before the fix and passes after.
Why this matters
The new fixture imports expect from "bun:test" but never calls it (all assertions live in run-integrated-headless.test.ts, unlike sibling command-template-capture.ts), and declares const template = "FIRST<$1>\nSECOND<$2>\nREST<$3>" that is never referenced — the config uses an inline "STATIC<!…>" template string instead. Both are copy-paste residue from command-template-capture.ts that misleads readers into thinking the fixture asserts something or exercises the FIRST/SECOND/REST template.
import { expect } from "bun:test"
import { spawn, spawnSync } from "node:child_process"
import fs from "node:fs/promises"
import { existsSync, readFileSync } from "node:fs"
import os from "node:os"
import { Database } from "bun:sqlite"
import path from "node:path"
const template = "FIRST<$1>\nSECOND<$2>\nREST<$3>"
const guard = path.join(import.meta.dir, "stdin-guard.ts")
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 12 files · 0 inline · view all 3 findings ↗ · AI-generated — verify findings before acting · AI guidance and feedback aictrl · AI code review for fast-moving teams · aictrl.dev |
Member
Author
Review response — PR #257The three findings from review Issues addressed (pushed to this PR)
Both changes are in
Review claims verified false (no change needed)None in this review. Not addressed hereDefault-on project-instruction isolation / startup visibility is a real hardening recommendation, retained under existing CI-adoption security work245, manager review 23 October. Discovery remains enabled unless explicitly disabled. Changing that default affects repository instruction behavior; a startup instruction manifest would introduce an event/privacy contract. These need a separately scoped decision and compatibility tests. Current CI recipes explicitly disable project config; this is not an untrusted-checkout safety clearance. Released0.5.4 still requires the documented instruction-file workarounds until a corrected release is available. Test plan
No new native binary, receiver/Windows, real-task model, paid-provider or release qualification is claimed. Native IDs were resolved by unique review/file/line/severity/title/fullclaim; GitHub comment IDs were null. |
byapparov
merged commit Oct 22, 2026
44ae6b6
into
test/issue-248-token-budgets-20261021
1 check passed
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.
No linked issue. Refs #251, #252, #253, #254, #255, #256. Feature ownership remains with the individual PRs. Priority: P2.
Intent
Verify that the combined headless CLI candidate works as one CI job: select its work tools, decide whether to consume stdin, pass command operands without reinterpreting them, and disable automatic project instructions. This PR also closes the remaining
.opencode/AGENTS.mdand nested-instruction discovery paths when project configuration is disabled.Review scope: this is stacked on #255 (
test/issue-248-token-budgets-20261021). The selector, stdin and command-argument implementations come from earlier PRs; #257 adds their combined acceptance tests, the instruction-discovery correction, fixture hardening and documentation. The comparison below is released v0.5.4 → combined candidate, not a claim that every feature is introduced by this diff. The latest published release is still v0.5.4.Expected Impact on Users
2999d0f7--tools read,glob,grepfor this invocation, ortool_allowlistin config as a default. Permission denials still win. Selector supplied by #254.-to consume the stream: `producerAICTRL_DISABLE_PROJECT_CONFIG=truestill leaves.opencode/AGENTS.mdand nested instructions reachable.{ argv: string[], stdin?: string }; quoted/empty operands and selected stdin stay separate until the documented template projection. Supplied by #256; exercised here.tool_catalogCLI examples
These examples target a build containing this candidate. Configure the selected provider's credentials and set
MODELto a supportedprovider/model; installing the current npm release does not establish these new contracts.Selected stdin waits for EOF; this change adds no stdin deadline. Close the producer or supervise the job.
--model -and--command -treat the hyphen as their option value; they do not select stdin.Configuration: defaults, permissions and trusted CI policy
For a trusted CI profile, put this in
/opt/aictrl/ci-profile/aictrl.jsonand supply the referencedreview.mdthere:{ "tool_allowlist": ["read", "glob", "grep"], "permission": { "*": "deny", "read": "allow", "glob": "allow", "grep": "allow" }, "instructions": ["review.md"] }--tools, use final configtool_allowlist; without either, preserve normal tool availability.[]means no work tools. A CLI selection replaces the config preference for that invocation, while permissions can still deny selected tools.aictrl.json; when disabling project config, supply trusted policy through the profile or caller config instead.AICTRL_CONFIG_CONTENTis the inline caller alternative.instructionsresolve fromAICTRL_CONFIG_DIR; without that directory they are skipped. Absolute paths and URLs remain explicit inputs. A profileAGENTS.mdtakes precedence over the global fallback; both are trusted sources.Expected Outcomes
readdoes not undo a permission denial.Implementation
.opencode/AGENTS.mddiscovery and the nestedInstructionPrompt.resolvepath with the existing disable flag.Pinned references: tool selection/config, stdin normalization, instruction gates, combined acceptance cases.
Scope Caveat
This candidate remains unreleased.
--attachrejects any explicit or configured work-tool selection, including an empty list; the required receiver capability is not implemented here. Structured attached-command compatibility also remains unverified.tool_catalogis inventory, not an effective per-request allowlist. Schema output still requires its own permissions; a read-only work-tool example does not grant write orStructuredOutputaccess. Project discovery remains enabled by default. Trusted command templates/plugins and tools still require an isolated runner and scoped credentials.No real-model quality, provider-billing, Windows or new native-binary claim is made. The 18 paired real-task evaluations and receiver/lifecycle acceptance remain open. Keep release-specific instruction-file workarounds until the installed artifact includes and verifies the fix.
Test Plan
2999d0f7.Verification
Hosted CI 39630140079, for head
2999d0f7b7bf3bbca9ed93d00e73fbd931279732, is successful:These are existing CI results, rechecked for this description; no tests or model calls were rerun for the wording update. Current
gh pr checkslistsverifyas passed. Review/security/release approval is separate from this result.Risks and Rollout
Update prompt-plus-pipe callers to include
-when adopting the candidate. Keep #255 as the stacked dependency; no branch retarget, merge or release is part of this description update. Existing review dispositions remain linked in the audit below.Detailed verification history and review dispositions (historical evidence)
The private tested source has 620 pinned entries. All 13 stages finished with exit0 and no watchdog:
After the tested source was verified, one documentation-only repair restored the stdin example and instruction-isolation section while retaining the accepted schema-mode clarification. The other 619 source pins are unchanged; no runtime rerun is implied. Full source/QA hashes and exact process-cleanup observations are retained locally. The fixture reports its owned groups gone; it does not provide an independent child-PID inventory for all 19 cases.
Hosted verification and release gates
Published head:
2999d0f7b7bf3bbca9ed93d00e73fbd931279732, parentf3e7483. This review cleanup changes only two files: root README qualifies scalar required-value hyphen ownership, and the integration helper drops an unused import and template constant. The other618source entries remain unchanged; no parser, permission, SDK, instruction-policy or CI behavior changed. Normal push completed21October at15:55:44UTC. Normal pre-push checks passed six tasks (five cached, with a fresh CLI compiler check); cached SDK generation/build output is replayed logging. All620committed source bindings and literal symlink payloads were verified.Focused actual tests on the cleanup passed 127 tests / 0 skips / 0 failures / 780 assertions:19integrated cases and108stdin cases, plus direct CLI typecheck. Independent source review accepted the bounded change. QA disclosed an initial private wrong-README-path assertion and tests starting before corrected independent preflight; the owner's full620freeze preceded launch, and corrected preflight/fullpostchecks verify the same candidate. No prelaunch independent pass or product runtime failure is inferred.
Fresh hosted CI39630140079 for
2999d0f7passed 2,287tests /10skips /0failures /16,137assertions across159files, with a separate actual stub step4pass/20assertions. Immutable logs identify checkout940b2862327c0b4decaf4c3278ed8d6596213d07, parents ef49+2999, whose98aetree and all620Gitblob/modebindings exactly match the published correction. Build3/3 and typecheck6/6 passed. Independent inert-artifact audit verified all four reconstructed canonical prefixes and complete raw request bodies byte-for-byte against the previously audited f3e evidence. Reference counts3,441/2,001 (fullfirst3,453/2,013) are reused by exact byte identity; no fresh tokenizer execution or recount is claimed. Original1,250payload files are unchanged; recipe bindings come from actual checkout940b, not a separate embedded recipe-SHA manifest. Hosted cleanup assertions do not prove independently observed remote PID absence. The original six-poll observation window ended normally at 16:25:48UTC, after its16:25:44UTC cap. Across that window, no automated review on2999 was observed and no read failed; no current-head AI check was visible at the final poll. Absence of review is not approval. The window was not restarted; fresh CI is green while the review gate remains open. The original f3e window ended at15:34:48UTC; its AI review subsequently completed15:36:28UTC with three findings, addressed below. CodeQL remains untriggered under default-setup target policy because this PR targets an unprotected feature branch; no current-head security clearance is inferred.Earlier-head evidence: CI39622496092 on f3e passed2,287tests/10skips/0failures,16,137assertions/159files, separate stub4/20 and27Python controls. Actual checkout5bf20a27 had parents ef49+f3e and the exact previouse749tree/all620blob-modebindings. Its artifact independently reproduced four prefixes3,441/2,001, fullfirst3,453/2,013, with rawbytes unchanged. Recipe hashes bind through that checkout; there is no separate embedded recipe-SHA manifest or independently probed remote PID absence. This does not claim fresh hosted validation of2999. Earlier91f native/release evidence remains earlier-head evidence.
Three-finding review response: two cleanup fixes and one instruction-default/startup-visibility hardening deferral under existing245, manager review23October. All three exact native verdicts were written and read back, zero failures; lifecycle verification remains separate. Discovery is still enabled unless explicitly disabled, and there is no untrusted-checkout safety clearance.
Six-finding review response: two fixes, one manager-owned P2 guard-policy deferral for23October and three explained ignores; all six exact native verdicts were written and read back, with zero failures. The guard is explicitly partial instrumentation, not network confinement.
Configured, skill and MCP templates remain trusted executable code. Data binding is not an execution or network sandbox. Receiver compatibility, attached lifecycle acceptance and all18 real paired quality trials remain open. This correction has no new native-binary, provider-backed, Windows, billing or release qualification. The selector and combined candidate remain unreleased.