chore(agent): remove the unwired tool-discovery code superseded by tool_search - #3504
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
🚧 Files skipped from review as they are similar to previous changes (6)
📝 WalkthroughWalkthroughThe PR documents the shipped deferred ChangesDeferred tool discovery
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![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: 740b4acf1f
ℹ️ 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".
…ol_search Deferred schema loading around `tool_search` is the shipped tool-discovery mechanism. An earlier `search_tools` / `load_tools` design was merged as primitives and never wired into a runtime. Its code, its provider-cap pinning, and its architecture doc all stayed in the tree, so the repository advertised a discovery mechanism a hosted run could not reach. Nothing constructed `createSearchToolsTool` or `createLoadToolsTool`, nothing populated `toolDiscoveryContext`, and nothing passed `pinnedToolNames` or `getActivatedToolNames`, so the per-step refresh branch was unreachable. The `activatedRemoteToolNames` execution gate was therefore also dead: it was only ever supplied through the context nothing set. `TOOL_DISCOVERY_TOOL_NAMES` reserved two never-registered names against the provider tool budget. Removed: - `search-tools-tool.ts`, `load-tools-tool.ts`, `tool-discovery-events.ts`, `tool-discovery-context.ts`, and their tests - `tool-discovery-execution-gate.test.ts`, which only covered the dead activated-set override - the `toolDiscoveryContext` input on `prepareHostedChatRuntimeToolAssembly` - the `activatedRemoteToolNames` option on the hosted project remote tool source - the `pinnedToolNames` / `getActivatedToolNames` step-refresh branch - the dead name pinning in `runtime-essential-tools.ts` Kept: the generic per-step `forkToolNames` override on `ForkRuntimeStepPreparation`. It is a real extension point on an exported preparer contract with a clean fallback, so only its comments changed to stop referencing the deleted mechanism. Rewrote `docs/architecture/28-model-driven-tool-discovery.md` to describe what runs: the deferred/eager mode split, why the provider cap no longer truncates the catalog, `tool_search` bounds and ranking, checkpoint-based resume, and the committed 87.7% token-reduction measurement. No behaviour change. Every removed path was unreachable in production. Refs veryfront-issue-inbox#434, follow-up to #34.
…ery removal Deleting the unwired discovery modules shifted line numbers in chat-runtime-tool-assembly.ts, child-fork-step-message-preparation.ts, and project-remote-tool-source.ts, so the generated source links in docs/api-reference/veryfront/agent.md went stale and ci (lint) failed on docs:api-reference:check. Pure line-number drift: 14 rows changed, and no symbol was added or removed. The deleted code was never part of the published API surface.
kwakayama
force-pushed
the
chore/remove-unwired-tool-discovery
branch
from
August 9, 2026 12:42
7d4a94e to
398b9fe
Compare
…rop em dashes Addresses both Codex P1 review comments on #3504. P1: preserve the activated-tool execution gate. `createHostedProjectRemoteToolSource` and `CreateHostedProjectRemoteToolSourceInput` are exported through `veryfront/agent`, so a published caller can supply a broad `allowedToolNames` set together with a narrower `activatedRemoteToolNames` set. Removing the latter made the broad allowlist the sole catalog gate, silently widening what an external caller could list and execute. AGENTS.md:9 requires preserving public API compatibility unless a breaking change was asked for, and issue 434 asked only for dead-code cleanup. The review flagged one instance. The same defect applied to four more public types it did not flag, all restored here: - `CreateHostedProjectRemoteToolSourceInput.activatedRemoteToolNames` - `CreateHostedProjectRemoteToolSourcesInput` (inherits the field via `Omit`) - `PrepareHostedChatRuntimeToolAssemblyInput.toolDiscoveryContext` - `PrepareHostedChildForkRuntimeStepMessagesInput.pinnedToolNames` and `.getActivatedToolNames` - `HostedChildForkRuntimeStepMessages.forkToolNames` All are marked `@deprecated` with the reason, and their behavioral tests are restored. Verified field-by-field: all four public types now match `origin/main` exactly, and the generated API reference shows zero symbols added or removed. Still removed, because none of it was ever reachable from the barrel: `search-tools-tool.ts`, `load-tools-tool.ts`, `tool-discovery-events.ts`, their tests, and the `TOOL_DISCOVERY_TOOL_NAMES` pinning in `runtime-essential-tools.ts` (`resolveHostedRuntimeAllowedToolNames` is not exported either). The `RuntimeToolDiscoveryContext` callbacks `onToolsActivated` and `onToolsActivationRejected` are also dropped: their only invoker was `createLoadToolsTool`, which was never exported, so no external caller could ever have made them fire. P2: replace the prohibited dash characters. AGENTS.md:54 bans em and en dashes in public copy. The rewritten doc had 8. It is now at 0, and the diff introduces none anywhere. This file was the only one of 26 architecture docs containing them, and the only one with a numbered Title-Case heading, so the title is now sentence case to match its siblings and AGENTS.md:47.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@docs/architecture/28-model-driven-tool-discovery.md`:
- Around line 42-46: Clarify in the deferred-mode measurement description
whether bootstrap tools are excluded from the initial exposure count; if they
are included, update the reported count and corresponding token comparison to
account for form_input, load_skill, and tool_search. Keep the bootstrap-tool
behavior and eager-mode description unchanged.
- Around line 42-46: Update the eager-mode documentation around the binding
description and cap semantics to state explicitly how bindings exceeding the cap
are handled, consistent with selectProviderCompatibleToolNames. Restrict the
claim that the cap never limits searchable or executable tools to deferred mode,
and revise references to bindings being small by construction so they do not
contradict oversized eager bindings.
- Around line 36-40: Update the TypeScript example defining toolLoadingMode so
it is complete and copyable by including declarations for input and
RuntimeToolLoadingMode plus any required surrounding context, or replace the
snippet with a link to the exact implementation and explicitly label it as a
source excerpt.
- Around line 80-84: Update the checkpoint-restoration documentation around
restoreToolExposureState to state that restoration preserves the loaded exposure
state, while current authorization remains enforced at execution time through
isRemoteToolNameAllowed. Replace the claim that capability is never silently
downgraded with wording that acknowledges permission changes can make restored
tools unavailable.
- Around line 34-40: Update prepareHostedChatRuntimeToolAssembly to normalize
input.allowedToolNames to null before selecting RuntimeToolLoadingMode, then
derive "deferred" versus "eager" from that normalized value. Add a regression
test covering an omitted allowedToolNames property and asserting the deferred
mode.
In `@src/agent/hosted/chat-runtime-tool-assembly.ts`:
- Around line 123-124: Update both deprecation notices to use “Use” instead of
“Prefer”: change the notice in src/agent/hosted/chat-runtime-tool-assembly.ts
lines 123-124 and the corresponding notice in
src/agent/hosted/child-fork-step-message-preparation.ts lines 40-41, preserving
the remaining wording.
In `@src/agent/hosted/project-remote-tool-source.ts`:
- Around line 70-78: Document that activatedRemoteToolNames set to null
overrides allowedToolNames and disables name filtering in both the comments at
src/agent/hosted/project-remote-tool-source.ts lines 70-78 and 101-102; preserve
the existing null behavior rather than changing it to fall back via ??.
In `@src/agent/runtime/tool-discovery-context.ts`:
- Around line 2-14: Preserve the deprecated public members of
RuntimeToolDiscoveryContext and the related
PrepareHostedChatRuntimeToolAssemblyInput context shape as optional
declarations, including the catalog-entry type and activation callbacks removed
by this change. Keep these members ignored at runtime, and retain their exported
types so existing callers and compatibility tests continue to compile.
- Around line 2-11: Update the documentation for activatedRemoteToolNames in
RuntimeToolDiscoveryContext to state that callers own and must create a fresh
Set for every run, preventing names retained from earlier runs from overriding
the current run’s allowedToolNames.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 9431848f-fb80-4256-b659-dd92d7de1247
📒 Files selected for processing (14)
docs/api-reference/veryfront/agent.mddocs/architecture/28-model-driven-tool-discovery.mdsrc/agent/hosted/chat-runtime-tool-assembly.tssrc/agent/hosted/child-fork-step-message-preparation.tssrc/agent/hosted/project-remote-tool-source.tssrc/agent/hosted/runtime-essential-tools.test.tssrc/agent/hosted/runtime-essential-tools.tssrc/agent/runtime/load-tools-tool.test.tssrc/agent/runtime/load-tools-tool.tssrc/agent/runtime/search-tools-tool.test.tssrc/agent/runtime/search-tools-tool.tssrc/agent/runtime/tool-discovery-context.tssrc/agent/runtime/tool-discovery-events.test.tssrc/agent/runtime/tool-discovery-events.ts
💤 Files with no reviewable changes (7)
- src/agent/runtime/load-tools-tool.test.ts
- src/agent/runtime/load-tools-tool.ts
- src/agent/runtime/tool-discovery-events.test.ts
- src/agent/runtime/tool-discovery-events.ts
- src/agent/runtime/search-tools-tool.test.ts
- src/agent/runtime/search-tools-tool.ts
- src/agent/hosted/runtime-essential-tools.ts
Applies eight of nine findings. The ninth is a real pre-existing issue that this PR does not touch, reported separately rather than fixed here. Applied: - Restore `RuntimeToolCatalogEntry` and the `onToolsActivated` / `onToolsActivationRejected` members of `RuntimeToolDiscoveryContext`. The type is reachable from the public `PrepareHostedChatRuntimeToolAssemblyInput` field, so a caller constructing the context with those properties would stop compiling. Both callbacks are documented as never invoked: their only caller was `createLoadToolsTool`, which was never exported, so no external caller could ever have made them fire. Retaining them is also what the architecture doc now promises. - Document that `activatedRemoteToolNames: null` is not a fallback. It overrides `allowedToolNames` and disables name filtering entirely, because `isRemoteToolNameAllowed` treats a nullish set as allow-all. Only an omitted property falls back. Documented at both the declaration and the use site. - Document caller ownership of the activated Set: the framework never creates this context, and a Set reused across runs leaks the earlier run's tool names into the later one. - Use "Use" rather than "Prefer" in both deprecation notices (AGENTS.md:51). - Reconcile the bootstrap-tool count with the measurement. Bootstrap tools are filtered against the authorized set, so a run authorizing neither `form_input` nor `load_skill` exposes `tool_search` alone, which is why the evidence reports one initially exposed tool. - State eager-mode cap semantics instead of assuming them. An eager binding larger than the provider cap is still truncated alphabetically; nothing enforces that bindings stay small. The no-truncation guarantee is specific to deferred mode. - Correct the resume claim. `restoreToolExposureState` restores exposure, not authorization: execution re-checks `isRemoteToolNameAllowed`, so a tool whose permissions changed becomes unavailable at call time even though its schema was restored. The previous wording overstated this as never downgrading capability. - Label the loading-mode snippet as a source excerpt and link the implementation, since it was not complete or copyable (AGENTS.md:56). Not applied, reported instead: normalizing `allowedToolNames` before selecting the loading mode. The mode check is unchanged by this PR and is pre-existing on main. The framework's only caller passes `input.options.allowedTools ?? null`, so the deferred path is reached correctly in production; the inconsistency needs an external caller that omits the optional property. Changing it is a runtime behavior change that wants its own failing test, which is out of scope for a dead-code removal.
Contributor
Author
|
Addressed all review comments in Codex, both P1s appliedPreserve the activated-tool execution gate. Correct, and the fix was broader than the comment.
All marked Replace the prohibited dash characters. Applied. The rewrite had 8; the diff now introduces none anywhere. This file was the only one of 26 architecture docs containing them and the only one with a numbered Title-Case heading, so the title is now sentence case to match its siblings and AGENTS.md:47. CodeRabbit, 8 of 9 appliedApplied: restored Two worth calling out: The callbacks are restored, with a caveat. The Not applied, reported insteadNormalize I am not fixing it in this PR, for three reasons:
Happy to open it as its own issue if you would like it tracked. |
This was referenced Aug 9, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Removes the
search_tools/load_toolstool-discovery code. It was merged as tested primitives and never wired into a runtime.tool_search(#3178) is the mechanism that actually ships.No behaviour change — every removed path was unreachable in production.
Why
The tree advertised a discovery mechanism a hosted run could not reach:
createSearchToolsToolorcreateLoadToolsTooltoolDiscoveryContext, so theactivatedRemoteToolNamesexecution gate was only ever supplied through a context nothing setpinnedToolNames/getActivatedToolNames, so the per-step refresh branch inprepareHostedChildForkRuntimeStepMessageswas unreachableTOOL_DISCOVERY_TOOL_NAMESreserved two never-registered names against the provider tool budgetdocs/architecture/28-model-driven-tool-discovery.mddocumented the whole thing as the design, with no mention oftool_searchReading the tree to answer "how does an agent discover tools" produced the wrong answer.
Removed
src/agent/runtime/search-tools-tool.ts+ testsrc/agent/runtime/load-tools-tool.ts+ testsrc/agent/runtime/tool-discovery-events.ts+ testsrc/agent/runtime/tool-discovery-context.tssrc/agent/runtime/tool-discovery-execution-gate.test.tsPlus the dead seams: the
toolDiscoveryContextinput onprepareHostedChatRuntimeToolAssembly, theactivatedRemoteToolNamesoption on the hosted project remote tool source, thepinnedToolNames/getActivatedToolNamesbranch, and the name pinning inruntime-essential-tools.ts.Deliberately kept
The generic per-step
forkToolNamesoverride onForkRuntimeStepPreparationinsrc/agent/streaming/fork-runtime-stream.ts.ForkRuntimeStepPrepareris an exported contract, the override is genuinely generic, and it falls back toinput.forkToolNameswhen absent. Only its comments changed, to stop referencing the deleted mechanism.Docs
28-model-driven-tool-discovery.mdrewritten to describe what runs: the deferred/eager mode split onallowedToolNames === null, why the provider cap no longer truncates the authorization catalog in deferred mode,tool_searchranking and its bounds, checkpoint-based resume viaAGENT_RUN_TOOL_EXPOSURE_CHECKPOINT, and the committed 87.7% token-reduction measurement. Status moved from "implementation in progress" to "shipped".It also records the one real limitation: deferred mode is conditional on the agent having no explicit binding, so a bound agent whose binding exceeds the provider cap still takes the eager path.
Verification
src/agent+src/toolsuites: 1188 passed, 0 faileddeno check src/agent/index.ts src/tool/index.tscleandeno lint(4880 files) anddeno fmt --check(4145 files) cleandocs:check-links: all 1225 links OKlint:test-typecheck:child-fork-step-message-preparation.test.tsnow typechecks cleanly, so it is removed from the baseline (51 → 50)lint:module-boundaries: passesNote on the module-boundaries baseline
The check reports one broad import that can be locked in, but I confirmed against a clean
origin/mainworktree that this is pre-existing and unrelated to these changes —getEntityInfo.ts -> #veryfront/utils. Left it alone rather than absorbing someone else's improvement into this diff. The check exits 0 either way.Refs veryfront-issue-inbox#434, follow-up to #34.
Summary by CodeRabbit
Documentation
tool_search, including authorization, bounded searches, persistence, and token-efficiency improvements.Refactor
Tests