Repository navigation
Conversation
e421439 to
eec2ff5
Compare
berry-13
changed the title
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eec2ff5342
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const requestedNames = new Set(config.codingToolNames); | ||
| return new Set( | ||
| LOCAL_CODING_BUNDLE_NAMES.filter((name) => requestedNames.has(name)) | ||
| ); |
There was a problem hiding this comment.
When local.codingToolNames omits run_tools_with_code or run_tools_with_bash, this helper and the resolvers correctly remove those tools, but isProgrammaticRunnerAutoBound still returns true for every local coding bundle. AgentContext.getProgrammaticToolInstructionTargets() consequently emits system guidance for runners that have no bound schema and classifies them as directly executable, which can prompt unavailable tool calls; the auto-binding probe should consult the same selected-name set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3e85821. isProgrammaticRunnerAutoBound now consults the same selected-name set as the resolvers when local.codingToolNames is set. Regression test: 'reports programmatic runners as auto-bound only when allowlisted'.
| limit: number | undefined, | ||
| maxChars: number | ||
| ): string { | ||
| const lines = content.split('\n'); |
There was a problem hiding this comment.
With toolResultMaxChars enabled and a caller supplying a small limit for a large file, this split scans the entire file and allocates one string per line on every page before applying the offset and limit. That directly undoes the bounded newline-index walk in lineWindow above, so repeated paginated reads of a file near the 10 MiB default cap can become O(pages × file size) with large transient allocations; walk newline indices only until the requested character/line budget is reached.
AGENTS.md reference: AGENTS.md:L41-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3e85821. cappedLineWindow walks newline indices from the offset and stops at the line or character budget, so a page costs O(offset + page) with no full split; the hint no longer reports a total line count, which would need a full scan.
| const path = resolve(tmpdir(), `lc-output-${randomUUID()}.txt`); | ||
| writeFileSync(path, text, { encoding: 'utf8', mode: 0o600 }); |
There was a problem hiding this comment.
When local.exec.spawn delegates commands to an SSH/container runtime with no shared /tmp (or sandbox read policy excludes host temp files), an output that exceeds only the new result cap is written here with the orchestrator's fs and tmpdir(). The returned full output at /tmp/lc-output-... path is therefore inaccessible to subsequent bash_tool calls in the delegated execution world, defeating the promised recovery path; create the spill through the configured execution-world filesystem in an accessible workspace or expose it as a host-retrievable artifact.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 3e85821. The spill file is written only when commands run on the host (no exec.spawn, exec.fs or spawn override); with a delegated runtime the capped result says the full output was not kept instead of naming an unreachable path. Regression test: 'names no spill file when commands run through a delegated runtime'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d57a8bb0c0
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 21906f6e1c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| return text; | ||
| } | ||
| const head = text.slice(0, Math.floor(maxChars * 0.2)); | ||
| const tail = tailTruncate(text, Math.floor(maxChars * 0.8)); |
There was a problem hiding this comment.
When toolResultMaxChars is shorter than the trailing metadata (for example with a deep cwd, or with both full_output_path and working_directory present), this tail can omit exit_code: and timed_out: entirely while the head only contains stdout. createLocalBashExecutionTool still returns a successful tool invocation for a nonzero command, and both new verification hooks rely on failed() finding those strings, so a failing test is classified as passing and can terminate the Run early; keep execution-status markers outside the truncated payload or pass status structurally to the hooks.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0edf204. toolResultMaxChars now caps stdout and stderr before formatting (stderr gets at most half the budget), so the exit_code, timed_out and killed lines are always kept. Regression test: 'keeps the exit status of a failing command under a tiny cap' (exit 3 under a 40-character cap).
berry-13
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5b5a0336eb
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mergeToolsByName( | ||
| selectedNames == null | ||
| ? args.tools | ||
| : filterCloudflareCodingToolAllowlist(args.tools, selectedNames), |
There was a problem hiding this comment.
When a host pre-binds a bundle-named tool that local.codingToolNames omits, the explicit-binding path remains inconsistent: this new filter removes the schema only during Graph's final binding, but AgentContext.calculateInstructionTokens() has already counted the unfiltered tool, and getProgrammaticToolInstructionTargets() also treats an excluded runner as present through hasBoundTool(). Thus low-budget runs can compact against schemas that are never sent and the system prompt can instruct the model to call a runner with no bound schema; apply the same allowlist before AgentContext's prompt projections. This is fresh evidence beyond the fixed auto-binding case.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in b22870a. AgentContext now passes its tools through filterToolsByLocalAllowlist in the constructor, so instruction tokens, hasBoundTool and runner guidance see the same allowlisted set that is finally bound. Regression test: 'filters host-bound tools for prompt projections with the same allowlist'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b22870ae7d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? createExactTokenCountCache(tokenCounter) | ||
| : undefined; | ||
| this.tools = tools; | ||
| this.tools = filterToolsByLocalAllowlist(tools, toolExecution); |
There was a problem hiding this comment.
When an event-driven host provides an omitted bundle tool in toolDefinitions (for example, write_file with allowed_callers: ['direct', 'code_execution']) while allowlisting read_file and run_tools_with_code, this line filters only tools; getDirectProgrammaticCapabilityProjection() still treats every event definition as an implementation and tells the model that write_file is available, while resolveLocalExecutionTools() removes it from ToolNode's map. Fresh evidence beyond the fixed explicit-tools path is that toolDefinitions is still stored unfiltered later in this constructor; apply the same selected-name set to event definitions and their prompt projections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26b96e9. AgentContext now passes toolDefinitions through the same filterToolsByLocalAllowlist as tools, so event-driven definitions and their prompt projections see only allowlisted coding tools. Regression test: 'filters event-driven tool definitions with the local allowlist'.
| const fullDisplay = formatFallbackGrepDisplay({ matches, skipped }); | ||
| const display = | ||
| config.toolResultMaxChars != null && config.toolResultMaxChars > 0 | ||
| ? capLines(fullDisplay, config.toolResultMaxChars) |
There was a problem hiding this comment.
When ripgrep is unavailable and fallback search has both ordinary matches that fill the character budget and oversized-file skips, formatFallbackGrepDisplay() appends the [skipped: ...] diagnostics after the matches, so this generic cap can discard every skip line and replace them with [N more matching lines ...]. The model then receives a successful-looking partial search with no indication that some files were never searched; keep skip diagnostics outside match capping or budget them separately.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26b96e9. The fallback grep cap now applies to matches only, and the [skipped: ...] notices always follow. Regression test: 'keeps fallback grep skip notices when matches fill the cap' (no ripgrep, 200 matches, one oversize file, 300-character cap).
| * The file is readable by its owner only, since tool output may hold secrets. | ||
| */ | ||
| export function spillToFile(text: string): string { | ||
| const path = resolve(tmpdir(), `lc-output-${randomUUID()}.txt`); |
There was a problem hiding this comment.
When host-side bash_tool output exceeds local.toolResultMaxChars, each truncated stdout/stderr stream reaches this write and creates a new lc-output-* file, but a repo-wide search for that prefix finds no production unlink or run teardown (only the test deletes one). A long-running host therefore accumulates persistent command output until the temp volume fills; register these paths for cleanup once the run can no longer inspect them, or use a bounded expiring store.
AGENTS.md reference: AGENTS.md:L45-L45
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 26b96e9 by removing the spill file: capped bash output now says how many characters were left out and suggests a narrower rerun, so nothing is written to the host temp dir and nothing accumulates. The engine's existing spill for oversize output is unchanged.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 26126e9b9f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? createExactTokenCountCache(tokenCounter) | ||
| : undefined; | ||
| this.tools = tools; | ||
| this.tools = filterToolsByLocalAllowlist(tools, toolExecution); |
There was a problem hiding this comment.
When an omitted bundle-named tool is supplied through AgentInputs.graphTools (for example run_tools_with_code while only read_file is allowlisted), this filters only tools; fromConfig later copies graphTools unchanged at AgentContext.ts:186-187. The omitted schema is therefore counted and treated as a bound programmatic runner by AgentContext.ts:741-742,1413-1417, even though Graph.getPreparedToolsForBinding() finally removes it at Graph.ts:3202-3207, so low-budget runs account for a schema that is never sent and the prompt can advertise an unavailable runner. Fresh evidence beyond the earlier explicit-tools fix is this post-constructor, unfiltered graphTools assignment; apply the same filter before prompt projections.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79b2e14. AgentContext.fromConfig now passes graphTools through filterToolsByLocalAllowlist, so tools, toolDefinitions and graphTools all reach prompt projections already filtered. Regression test: 'drops graphTools outside the local coding allowlist'.
| if (keep != null && !keep.has(definition.name)) { | ||
| registry.delete(definition.name); |
There was a problem hiding this comment.
When a host registry already contains bash_tool, execute_code, or either programmatic runner and codingToolNames omits that name, this pruning loop never visits it because createLocalCodingToolDefinitions() only returns the file/search/compile definitions (LocalCodingTools.ts:1493-1526). The stale registry entry survives after the implementation and bound schema are removed, so merged capability snapshots—and, with an allowed runner, direct-only programmatic guidance—can still advertise the unavailable tool. Fresh evidence beyond the earlier schema-filtering fix is that runner names are absent from the generated definition list; prune omitted names across LOCAL_CODING_BUNDLE_NAMES before merging definitions.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79b2e14. resolveLocalToolRegistry now deletes every LOCAL_CODING_BUNDLE_NAMES entry outside codingToolNames before merging definitions, so pre-registered runner entries such as bash_tool are pruned too. Regression test added to 'prunes registry definitions outside the local allowlist'.
| matches.length > 0 | ||
| ? [capLines(matches.join('\n'), config.toolResultMaxChars), ...skipped].join('\n') | ||
| : formatFallbackGrepDisplay({ matches, skipped }); |
There was a problem hiding this comment.
When ripgrep is unavailable and a workspace contains many files larger than the fallback's 5 MiB limit, fallbackGrep creates one skipped diagnostic per file (LocalCodingTools.ts:1110-1113), but this branch appends every diagnostic after the capped matches and bypasses capLines entirely when there are no matches. A call with a small toolResultMaxChars can therefore return far beyond the configured context bound; retain the diagnostic signal by budgeting or summarizing skipped files and their total count within the cap.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 79b2e14 and 3a37784. Under toolResultMaxChars, fallback skip notices get at most a fifth of the budget, with a count of the ones left out, and matches get the rest; capLines also bounds each line by the cap. Regression tests: 'bounds many fallback grep skip notices by the cap' and the small-cap case in 'caps search lines and counts what it left out'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3a37784890
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| config.toolResultMaxChars != null && | ||
| config.toolResultMaxChars > 0 && | ||
| lines.length > 0 | ||
| ? capLines(joined, config.toolResultMaxChars) |
There was a problem hiding this comment.
When rg rejects a long invalid pattern (or a delegated local.exec.spawn returns a large stderr), the failure branch at lines 1252-1261 returns the full detail, and this new cap also bypasses stderr whenever lines.length === 0. Ripgrep echoes invalid patterns in its diagnostics, so a small toolResultMaxChars can still send output up to the separate 200,000-character process cap to the model; apply the cap to failure/no-match diagnostics as well as successful match lines.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2464136. ripgrep failure output and no-match stderr are now capped by toolResultMaxChars (head and tail), so a long echoed pattern cannot bypass the budget.
| return selectLocalCodingTools(config, [ | ||
| createLocalReadFileTool(config), | ||
| createLocalWriteFileTool(config, checkpointer), | ||
| createLocalEditFileTool(config, checkpointer), |
There was a problem hiding this comment.
When codingToolNames selects only one or two tools, this eagerly evaluated array still constructs all eleven DynamicStructuredTool instances (and the checkpointer above) before filtering them. I checked the model-binding path in Graph.ts:3197-3207: it calls this factory again on each binding pass, so the allowlist repeatedly allocates and discards every omitted tool; select factory functions by name before invoking them.
AGENTS.md reference: AGENTS.md:L41-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 2464136. createLocalCodingTools now selects tool names before constructing, so with codingToolNames only the listed tools are built. Regression test: 'constructs only the allowlisted tools'.
berry-13
changed the title
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: bde5199df3
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| mergeToolsByName( | ||
| selectedNames == null | ||
| ? args.tools | ||
| : filterCloudflareCodingToolAllowlist(args.tools, selectedNames), |
There was a problem hiding this comment.
When a host supplies AgentInputs.tools in a supported provider-native form (for example an OpenAI { type: 'function', function: { name: 'bash_tool' } } schema plus its toolMap) and allowlists only read_file, this branch calls a filter that preserves every entry without a top-level name; the omitted schema therefore remains bound while resolveLocalExecutionTools removes its implementation, exposing an unavailable call and retaining its prompt cost. Fresh evidence beyond the previously fixed GenericTool path is that GraphTools explicitly also accepts BindToolsInput[] and Google tool objects at src/types/graph.ts:410, so the filter must extract nested function names and filter/split declarations as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ce46e7. The shared allowlist filter now reads names in every GraphTools shape: top-level name, OpenAI's function.name, and Google functionDeclarations, whose omitted declarations are dropped while the rest of the group is kept. Regression test: 'filters provider-native tool schemas with the local allowlist'.
| const display = formatFallbackGrepDisplay({ matches, skipped }); | ||
| const display = | ||
| config.toolResultMaxChars != null && config.toolResultMaxChars > 0 | ||
| ? capFallbackGrepDisplay(matches, skipped, config.toolResultMaxChars) |
There was a problem hiding this comment.
When ripgrep is unavailable and the caller supplies a syntactically invalid regex near the 1,024-character fallback limit, new RegExp(pattern) includes the whole pattern in its exception message. Only successful fallback results pass through this capped branch; the FallbackGrepError catch below returns e.message verbatim, so a small toolResultMaxChars still produces roughly a kilobyte of tool content. Apply the same cap to fallback refusal diagnostics that is already applied to ripgrep failures.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9ce46e7. FallbackGrepError messages are capped by toolResultMaxChars like ripgrep failures. Regression test: 'caps fallback grep pattern errors' (900-character invalid pattern, 120-character cap).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9ce46e75fd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? createExactTokenCountCache(tokenCounter) | ||
| : undefined; | ||
| this.tools = tools; | ||
| this.tools = filterToolsByLocalAllowlist(tools, toolExecution); |
There was a problem hiding this comment.
When a caller reuses a nonzero AgentInputs.toolSchemaTokens cache produced for the unfiltered tool set and enables local.codingToolNames, this assignment removes schemas but fromConfig() still accepts the old aggregate and skips calculateInstructionTokens() at lines 208-212. Tight-context runs can therefore compact or reject based on schemas that are never bound; the cached total needs to be ignored or keyed to the effective allowlisted set.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 010f316. fromConfig now uses a cached toolSchemaTokens total only when the local allowlist removed none of the host's tools, toolDefinitions or graphTools; otherwise it recalculates. Regression test: 'ignores a cached toolSchemaTokens total once the local allowlist removes tools'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 010f316959
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if ( | ||
| toolSchemaTokens != null && | ||
| toolSchemaTokens > 0 && | ||
| !allowlistRemovedTools |
There was a problem hiding this comment.
When a host supplies cached toolSchemaTokens plus provider-native tools and the allowlist removes one while retaining another (for example OpenAI bash_tool and web_search schemas), this condition discards the cache. The fallback calculateInstructionTokens() path at lines 1435-1457 only counts entries with a .schema property, so retained { function: ... }, Google, or Bedrock schemas remain bound but contribute zero to the new total; tight-context runs can consequently under-budget the request and force provider overflow/recovery. Extend the recalculation path to count the supported native shapes before invalidating their cached aggregate.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9969ceb. The cached total is now discarded only when the allowlist removed tools and every retained instance or graph tool carries a .schema the recount measures; with a retained native OpenAI, Google or Bedrock schema the larger cached total is kept, so the budget can only over-estimate. Regression test: 'keeps the cached total when a retained tool is a native schema the recount cannot measure'.
| return skipText === '' ? 'No matches found.' : skipText; | ||
| } | ||
| const matchText = capLines( | ||
| matches.join('\n'), |
There was a problem hiding this comment.
On hosts without ripgrep, matching long single-line files reaches this branch; the fallback accepts files up to 5 MiB and defaults to 200 results, so matches.join('\n') can materialize roughly 1 GiB before capLines splits the whole string again even when toolResultMaxChars is only a few hundred. This defeats the opt-in cap with large transient allocations and can OOM the agent; truncate or budget matches while scanning rather than joining the uncapped array.
AGENTS.md reference: AGENTS.md:L41-L44
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 9969ceb. capLines now accepts the match array and caps it line by line (each line cut to the cap before measuring), so neither fallback nor ripgrep matches are joined into one uncapped string. Regression test: array and string inputs give the same capped result.
Adds LocalExecutionConfig options toolResultMaxChars, codingToolNames and toolIntentLabels, plus opt-in verify-before-stop and stop-on-passing-check PostToolBatch hooks. Defaults are unchanged.
A single read_file line longer than toolResultMaxChars is now cut, an errored test tool result no longer counts as verification, and local.codingToolNames drops host-bound coding tools outside the allowlist on every binding path, matching the Cloudflare engine.
A runner name inside another command no longer counts as a test run, a custom pattern with the g or y flag gives the same answer on every call, and toolResultMaxChars also bounds read_file calls that pass a limit.
Test run detection now splits the command outside quotes and requires the last command to start a runner, since its exit code is the one reported. Quoted separators no longer split a command, and runs whose exit code is replaced by a later command (echo, || true, a pipe) are not verification.
A 2>&1 redirection no longer splits the command, listing modes such as --collect-only and --listTests are not test runs, and a shell call that edits and then runs tests is judged by its tests.
Errored edit_file and write_file results are not changes, git apply --check and --dry-run are not edits, and cargo test --no-run and vitest list are not test runs.
A runner after || may not run, and go test -list, ctest -N and rspec --dry-run list tests without running them.
…ions A test only verifies edits from earlier tool batches, since calls in one batch run concurrently. Shell edit detection checks commands outside quotes. Programmatic runners outside codingToolNames are not reported as auto-bound. Capped reads walk newline indices instead of splitting the file, and capped bash output names a spill file only when commands run on the host.
A failing test in a tool batch leaves the change unverified even when a sibling test passes, and a shell call whose edit runs in the background or in a pipe does not verify that edit.
A trailing newline or semicolon no longer hides the test command, -list= style listing flags are recognized, and a line with || never counts as verification because its runner may not run.
A test command run in the background returns before the tests finish, and --setup-plan, --showConfig and similar modes inspect without running.
…ests Edits from earlier user turns no longer hold a read-only run, and a negative exit code from a signalled test process counts as a failure.
…udget Hook-injected context messages no longer start a new user turn, and the verify hook does not request a continuation when the run has no continuation budget left.
toolResultMaxChars now caps stdout and stderr before formatting, so the exit_code, timed_out and killed lines are always kept.
The verify-before-stop and stop-on-passing-check hooks will follow in their own pull request with structured exit status and per-lane state.
AgentContext filters host-bound coding tools through codingToolNames before counting instruction tokens and building runner guidance, and tools replaced one by one also drop the intent field when toolIntentLabels is false.
…ions Capped bash output now says how much was left out instead of writing a temp file, so nothing accumulates on the host. Fallback grep keeps its skipped-file notices outside the match cap, and AgentContext applies the local allowlist to event-driven tool definitions too.
AgentContext filters graphTools through codingToolNames before prompt projections, the registry prunes omitted runner entries for every bundle name, and fallback grep skip notices get their own share of toolResultMaxChars with a count of the ones left out.
codingToolNames now selects factories by name before constructing them, and ripgrep failure and no-match output is capped by toolResultMaxChars.
The intent-label opt-out needs one shared schema construction path to apply everywhere; it will follow in its own pull request.
The coding tool allowlist now reads nested OpenAI function names and filters Google functionDeclarations, and fallback grep pattern errors are capped by toolResultMaxChars.
…s unjoined A cached toolSchemaTokens total is recounted only when every retained tool is measurable by the recount, and capLines takes the match array directly so fallback grep never joins uncapped matches.
9969ceb to
a405e2c
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a405e2cc2c
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (typeof name === 'string') return name; | ||
| return typeof fn?.name === 'string' ? fn.name : undefined; |
There was a problem hiding this comment.
When a host passes a Bedrock-native schema such as { toolSpec: { name: 'bash_tool' } } and local.codingToolNames omits bash_tool, this extractor returns undefined, so the schema survives the allowlist even though pruneCloudflareCodingToolAllowlist() deletes its implementation from ToolNode; the model can then issue an unavailable call. Fresh evidence beyond the fixed OpenAI/Google cases is src/llm/bedrock/toolCache.ts:49-59, which already recognizes toolSpec.name as a supported tool-name shape; reuse a shared extractor or handle that shape here.
AGENTS.md reference: AGENTS.md:L32-L35
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0127853. The allowlist's name reader now also reads Bedrock's toolSpec.name, alongside name and OpenAI function.name. Regression test: a Bedrock bash_tool schema is dropped in 'filters provider-native tool schemas with the local allowlist'.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0127853488
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| ? createExactTokenCountCache(tokenCounter) | ||
| : undefined; | ||
| this.tools = tools; | ||
| this.tools = filterToolsByLocalAllowlist(tools, toolExecution); |
There was a problem hiding this comment.
When AgentSession.compact() is used for a run whose local.codingToolNames omits a host-supplied bundle tool, src/session/AgentSession.ts:1716-1724 invokes AgentContext.fromConfig() without this.runConfig.toolExecution, so this new filter sees undefined. The manual summarizer consequently retains and counts the omitted schema and binds it at src/summarization/node.ts:684-702, unlike the normal graph path; this can waste a tight context budget or make summarization emit an unavailable tool call. Pass the run's execution config into the compaction context as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in df37e12. AgentSession.compact() now passes this.runConfig.toolExecution to AgentContext.fromConfig, so the manual summarizer sees the same allowlisted tools as the graph path. Regression test: 'builds the manual compaction context with the run tool execution config'.
This branch has not been deployed
1 participant
Summary
Adds two opt-in options for local coding tools; defaults are unchanged.
local.toolResultMaxCharscaps read_file, grep_search and bash_tool results. Reads keep whole lines with an offset hint, grep cuts long lines and budgets skip notices, and bash caps stdout and stderr while always keeping the exit status lines, saying how much was left out.local.codingToolNamesselects which local coding tools are built and bound. It is enforced on every path: registry, bound tools, tool map, programmatic runner checks, and AgentContext's prompt projections (tools, tool definitions, graph tools, including OpenAI, Google and Bedrock schema shapes), matching the Cloudflare engine.Earlier revisions also had stop hooks and an intent-label opt-out; both were removed from this PR.
Change Type
Testing
Checklist