Add deferred tool schema loading - #3178
Conversation
![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: c8eb920a24
ℹ️ 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".
Critical re-review — finalScore: 97/100 Reviewed exact head The implementation now satisfies the deferred-loading contract and the adversarial review cases:
Verification
Residual risk (non-blocking)
Merge the documented companion API PR Total: 97/100 — ready after the required repository approval. |
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Critical independent re-review
Score: 84/100
Verdict: BLOCK — DO NOT MERGE
Blocking findings: 2, plus blocked API prerequisite
This supersedes the earlier 97/100 assessment at head 44c73948886904e2124c7afc7f095fd8503c109d.
-
HIGH — the spec-required blocking live measurement is not auditable. The diff contains evaluator code and synthetic fixtures, while the PR body reports only aggregate token reduction. No saved live
hiandHelloartifact or check records all required raw provider usage, cache creation/read tokens, billable input, provider input cost, catalog fingerprint/counts, duration, completion, and gate result. The CI run exposes coverage artifacts only and has no live token-gate check. The explicit blocking measurement remains unverified. -
MEDIUM/HIGH — the public mode contract has silent bypasses.
resolveToolLoading()andagent()accept an unsafe JavaScript/TypeScript value such as"auto"; public config keeps it while runtime silently treats it as deferred. Only Markdown rejectsautoanddefered. Separately,runtime/index.ts:1146-1173forces every request-scoped tool replacement to eager, overriding explicit deferred and operational resolution. Validate programmatic input and either honor deferred replacement planning or document and narrowly test the compatibility exception.
Additional required follow-up:
- New public exports in
src/eval/index.tsandsrc/provider/shared/index.tsare absent from generated API references. - Native-search capability allowlists are duplicated across replay and provider adapters, creating drift risk.
Evidence: final-SHA CI is green, both prior review threads are resolved, git diff --check passes, and a fresh focused run passed 64/64 across exposure, replay, auth, hosted preparation, provider adapters, and benchmark logic.
The companion API PR is also blocked by two trust-boundary defects and must merge first.
Final: 84/100. Below the 90 merge threshold; not merged.
Final critical re-reviewScore: 96/100 This supersedes the prior 84/100 blocking review. Independent exact-head reviews:
Resolved in the final review loop:
Verification:
Compatibility is stated precisely: public TypeScript and wire contracts are compatible, while omitted Merge order remains: API prerequisite veryfront-api#4219, then this framework PR. |
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Critical re-review
Score: 82/100
Verdict: BLOCK — DO NOT MERGE
Standards axis: 88/100 — 3 findings, worst MEDIUM
Spec axis: 95/100 — 1 finding, worst MEDIUM
This review covers exact head c8b475580767d445a6ef017a7adbc3658f4c5c74 and supersedes my prior 84/100 review.
Blocking findings
-
HIGH — the cross-repo writer-token contract is incompatible. API PR #4219 mints
tokenUse: "run_event_writer"; this Framework head accepts only"project_scoped_service_account"insrc/agent/service/auth.ts:419-427. The header is rejected before execution atsrc/agent/hosted/chat-request-parser.ts:149-160with403 INVALID_RUN_EVENT_APPEND_TOKEN. Framework tests still mint the obsolete claim, so both repositories remain green independently. Align the verifier and add a shared/cross-repo contract fixture. -
HIGH — the blocking live measurement does not attest or reproduce the final Framework head. The manifest pins Framework revision
ffe38c7562be32c01f815a93cce8a9675e5ce302, not this head, and the captured producer invokes the removed__vfToolLoadingOverrideinput while the final code accepts benchmark controls only through the new internal adapter. Rerun thehiandHellogates against the final code and preserve the updated raw/sanitized report and producer manifest.
Evidence-quality follow-up
- The checked canary records only that one live call occurred, not the canary result, observed
list_projectscall, completion, or error state. - The stored catalog fingerprint covers names rather than canonical full definitions, so schema/description drift can evade the measurement identity.
- Low-severity standards findings remain: one
@std/assertimport instead of the repository assertion alias and a generic unregistered error in the internal benchmark adapter.
Verified closed
Programmatic auto/defered rejection, request-scoped deferred loading, internal-only benchmark controls, generated references, BDD conversion, and centralized provider capability gates are fixed. The saved hi and Hello records contain the required usage/count/cost/duration fields and pass the numeric thresholds for the older measured revision.
Evidence: all final-SHA CI checks are green; no unresolved review threads; git diff --check passes; fresh focused verification passed 69 tests / 50 steps. These isolated suites do not detect the API/Framework claim mismatch or refresh the live measurement.
Final: 82/100. Below the 90 merge threshold; API prerequisite is also blocked; not merged.
Independent exact-head review — 95/100Reviewed head: All post-runtime changes are confined to benchmark tests and frozen evidence. Exact report, manifest, release gate, all 12 Agent source snapshots/Git blobs, Framework package tarball/integrity, and privacy scans match. The 40-case Agent privacy suite covers common credential names, benign near-misses, deterministic collision preservation, and zero unsafe sensitive-key leaves. Final live gate: 12/12 deterministic cases, five calls, both prompts at 24,410 → 6,365 effective input tokens (73.92%), 48 authorized schemas retained, and exact completed |
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Critical re-review
Score: 86/100
Verdict: BLOCK — DO NOT MERGE
Standards axis: passing, no material blocker
Spec axis: 86/100 — 2 HIGH blockers
Reviewed exact head 06fb4f88caff103bb396b736941fb3935ebb495e, superseding the prior 82/100 review.
Blocking findings
-
HIGH — the committed live producer still endorses the forbidden
search_mcp_toolsfallback. Issue #339 requires removing it completely with no alias.tests/fixtures/eval/tool-loading-live-producer/agent/agents/veryfront.md.snapshotstill instructs the fallback, and the producer test snapshot asserts that behavior. The API head also keeps the legacy tool for staged rollout. This means the saved evidence is not for the canonical final contract. Remove the fallback from the producer/eval or update the canonical spec to approve the staged exception. -
HIGH — live evidence is pinned to an earlier Framework package revision, not this PR head. The release gate records framework revision
f66f3bd2162772a5df6abf5aa0fd66b0edd18ac9and tree72a5b639..., while the reviewed head is06fb4f88caff103bb396b736941fb3935ebb495e. Although the post-f66f3bd2diff is currently evidence/tests only, the release-gate regression checks internal consistency and never requires the recorded package revision/tree to equal the final reviewed head. Repack/rerun thehiandHellogates against the final head or add an executable proof that the measured package is byte-identical to it.
Verified closed
The API/Framework writer-token contract now matches (run_event_writer, exact scopes, project/run binding); programmatic mode validation, request-scoped deferred loading, provider-native gates, canary recording, privacy scans, and catalog provenance are covered. Fresh targeted verification passed 40 tests / 63 steps, and native-provider suites passed. All exact-head CI checks are green and git diff --check passes.
Final: 86/100. Below the 90 merge threshold; API prerequisite is also not mergeable.
kwakayama
left a comment
kwakayama
left a comment
There was a problem hiding this comment.
Critical exact-head re-review
Score: 95/100
Verdict: APPROVE for the staged rollout
Reviewed head: 06fb4f88caff103bb396b736941fb3935ebb495e
This supersedes the prior 86/100 review. The rollout owner has explicitly approved the N/N-1 legacy-search boundary, eager small pre-authorized child forks, and delayed removal in draft API cleanup #4221.
Standards
0 findings. Independent review found no documented-standard violations or material baseline smells across the exact diff.
Spec
The provider-neutral deferred-loading contract, trusted checkpoint/replay boundaries, request-scoped replacement behavior, native-provider gating, rollback mode, and schema-free bounded search are implemented and covered. The live benchmark uses the canonical Agent plus hosted tool assembly and the private Framework benchmark adapter. Post-measurement commits change only evidence/tests.
Verification
- Fresh focused suite: 20 tests / 8 steps passed.
- Fresh final-head
deno task build:npm: pass. - Fresh final-head npm tarball SHA-256:
0c83e11b8418d581395eb4ed6202e51244f64eafded56069b5ac993a89d714f1, exactly matching the frozen live evidence package. git diff --check: pass.- Exact-head GitHub CI: all required format, lint, typecheck, unit, integration, coverage, browser/binary E2E, npm smoke, audit, and CodeQL checks passed.
Residual risk is limited to the benchmark being a controlled hosted-path reconstruction rather than a full API transport run; separate authorization, lifecycle, checkpoint, and integration suites cover those boundaries.
Final: 95/100. Above the 90 merge threshold; merge after API #4219 and Studio #6288.
…search-durability # Conflicts: # docs/api-reference/veryfront/agent.md # src/agent/types.ts
Critical review score: 97/100 — APPROVENo blocking correctness, security, or spec findings remain at Key evidence:
Release version is Cloud durable rounds remain the post-deploy release gate tracked in veryfront/veryfront-issue-inbox#353. |
|
@kojiwakayama all critical review findings are resolved and the current head is independently scored 97/100 on both standards and spec. Local exact-package durability is 8/8 and the full pre-push gate is green. Your required repository approval is now the only non-CI merge gate; auto-merge is already enabled. |
Final exact-head review — 96/100Reviewed head: Review axes
CI and verification
Remaining riskOne low-severity duplication remains in model-visible catalog reduction between initial assembly and steering refresh. It is covered by tests and is better handled as a small follow-up than by expanding this already-large PR. Merge state: |
Final critical review — 97/100 (APPROVE)Reviewed exact head
Evidence:
One non-blocking LOW remains: the hosted deferred visibility reducer is duplicated in Recommendation: merge. Auto-merge/merge queue is enabled; repository-required independent approval is the only remaining gate. |
2 participants
Summary
This PR implements the framework portion of the Deferred Tool Discovery for veryfront-agent PRD.
It reduces model input cost by keeping a broad
tools: truecatalog authorized but out of the initial model context. The framework exposestool_search, loads matching authorized schemas for the next model step, and preserves loaded state across trusted hosted resume without widening authorization.There is no new public schema-loading option. The existing
toolsselector is the public contract.Canonical
toolscontracttools: undefinedtool_searchwhen nothing is searchabletools: truetool_search, not the full catalogtools: { my_project_tool: true }tools: truemeans that all tools already authorized in the current scope are available on demand. It never grants global, cross-project, cross-agent, or cross-tenant access.Search behavior
tool_search({ query })searches only the current authorized project-tool catalog.providerToolsremain a separate execution surface and are not searched or loaded by frameworktool_search.Direct and hosted execution
The framework fallback is provider-neutral and works without Veryfront Cloud. Focused proof covers direct-provider execution with the sequence:
tool_search -> selected authorized tool -> completionHosted execution uses the same framework search. Before continuation, it persists a private v1 loaded-tool checkpoint through the durable run-event contract. Restore is bound to the exact run and re-intersects loaded names with current authorization.
Security and durability invariants:
run_event_writerservice identity and scopes may write the private event;PRD acceptance evidence
hicompletes in one model step with no tool call.tool_searchonly), timestamp, model, and thresholds without credentials or schemas.tool_search -> selected toolis covered across generate, stream, respond, direct providers, hosted execution, and durable resume.toolsexpose no project tools; explicit maps remain immediately callable without search.The checked-in
hiartifact is intentionally labeled as a direct-provider framework fixture, not a production hosted-agent capture. The exact production Agent capture remains a cross-repository rollout gate.Cleanup in this revision
toolLoadingcompatibility path.Compatibility and rollback
tools?: true | Record<string, Tool | boolean>.providerTools.search_mcp_toolsremoval is deliberately separate in draft cleanup PR Bind merge-queue review to the queued source #4221.Out of scope
Consistent with the PRD, this PR does not add:
Cross-repository rollout
Framework behavior is proven locally and by direct live provider execution. Production promotion still requires two consecutive successful cloud durable rounds using unique run/conversation IDs, sanitized artifacts, the token KPI, and the exact staging-verified Agent artifact.
Verification
Reviewed exact head:
417703d54f600af66199d170913b47baa8988657.tool_search -> read_release_markerproof: passed; schema-free result; target executed exactly once.hiproof: 5,276 eager / 648 deferred input tokens; 87.718% reduction.References
docs/evidence/deferred-tool-discovery-hi-anthropic.json.deferred-tool-discovery-prd.pdf, dated 2026-07-31.