fix(skill): stop hardcoding invoke_agent in load-skill override-forwarding prose - #3485
Conversation
…rding prose Some runs expose only scoped delegate tools named agent_<id> and no invoke_agent, so the hardcoded name pointed the model at a tool that wasn't there. Reword the hosted-only override-forwarding clause to refer to "the available delegation tool" instead. Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
|
Warning Review limit reached
Next review available in: 15 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Repository UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThe load-skill override guidance now uses the available delegation tool instead of naming ChangesLoad-skill delegation policy
Estimated code review effort: 1 (Trivial) | ~5 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9b5cc7a0eb
ℹ️ 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".
| */ | ||
| export const LOAD_SKILL_OVERRIDE_FORWARDING = | ||
| "Pass through any returned model, thinking, or maxSteps overrides to invoke_agent when delegating."; | ||
| "Pass through any returned model, thinking, or maxSteps overrides to the available delegation tool when delegating."; |
There was a problem hiding this comment.
Limit override forwarding to tools that support it
When a hosted agent declares delegates, buildLocalTools replaces invoke_agent with scoped agent_<id> tools, whose schema accepts only input and whose wrapper forwards only that value; applySkillDelegationOverridesToToolInput also recognizes only invoke_agent. After such an agent loads a skill with model, thinking, or maxSteps, this generic instruction therefore tells it to pass settings that its available delegation tool cannot represent, so the skill's requested settings are not applied. Keep the clause conditional on legacy invoke_agent, or implement override propagation for scoped delegate tools.
AGENTS.md reference: AGENTS.md:L13-L13
Useful? React with 👍 / 👎.
Generalising the clause to "the available delegation tool" was wrong. It
is not a naming problem: override forwarding only ever works for
invoke_agent, by design.
applySkillDelegationOverridesToToolInput returns its input unchanged for
any tool name other than invoke_agent, and scoped agent_<id> delegates
accept only { input } (AgentToolInput). A scoped delegate therefore
cannot carry model, thinking, or maxSteps at all. The generalised wording
told the model to do something impossible on those runs - worse than the
original, which at least named the one tool where it works.
State the condition in the prose instead. On a run without invoke_agent
the instruction is a no-op; on a run with it, it is accurate. No dynamic
description needed.
Found by Codex review on this PR.
Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
|
Codex is right, and the finding is sharper than the issue I filed. Pushed The original framing was wrongveryfront/veryfront-issue-inbox#411 treated this as a naming problem: prose hardcodes That made it worse. Verified on this branch:
So on a scoped-delegation run, Override forwarding is The fixConditional in the prose, so it is static-safe: a no-op on runs without The test now pins the condition rather than the absence of a tool name, and the module comment records why the condition is load-bearing so the next person does not "simplify" it back. Verification
|
Fixes veryfront/veryfront-issue-inbox#411
Summary
LOAD_SKILL_OVERRIDE_FORWARDINGinsrc/skill/load-skill-policy.tshard-coded the tool nameinvoke_agent, but some runs expose only scoped delegate tools namedagent_<id>and noinvoke_agentat all. On those runs the policy prose named a tool that isn't present.This is pre-existing (found by Codex review on PR #3475, not introduced by it) and low severity — there's no measured impact from it. It's filed and fixed for consistency with the standard already applied to the surrounding prompt surface, not because of an observed failure.
Investigation
Swept every
invoke_agentoccurrence insrc/:Every hit besides
LOAD_SKILL_OVERRIDE_FORWARDINGis either:INVOKE_AGENT_TOOL_ID, registry lookups, tracing span names,Set/array membership checks), orinvoke_agenttool is actually executing (e.g. depth-limit errors, cancellation-retry messages inchild-invoke-tool.ts/child-tool-input.ts/default-invoke-agent-tool.ts), so the name is accurate in that context — the tool did exist for that call.Only
LOAD_SKILL_OVERRIDE_FORWARDINGis prose describing a delegation tool ahead of knowing whether it exists in the current run, so it's the only change in scope.Change
Before:
After:
This wording is correct regardless of which delegation tool (
invoke_agentor a scopedagent_<id>) exists in a given run, since tool descriptions are static strings and can't be made dynamic to the run's tool inventory.Design constraint preserved
LOAD_SKILL_OVERRIDE_FORWARDINGremains appended only to the hosted tool's description (RUNTIME_LOAD_SKILL_DESCRIPTIONinsrc/agent/runtime/load-skill-tool.ts), not to the factory-builtcreateLoadSkillTooldescription insrc/skill/tools.ts— only the hosted loader returns override fields (model/thinking/maxSteps). The existing pinning test for that asymmetry (src/skill/tools.test.ts, "promises override forwarding only from the loader that returns overrides") is untouched and still passes.Files changed
src/skill/load-skill-policy.ts— rewordedLOAD_SKILL_OVERRIDE_FORWARDINGsrc/skill/tools.test.ts— added a regression test pinning the new wording and asserting it no longer hardcodesinvoke_agentTest plan
does not hardcode a specific delegation tool nameinsrc/skill/tools.test.ts. Verified it fails against the old wording (git stashof the policy file only) and passes after the fix.DENO_TESTING=1 VF_DISABLE_LRU_INTERVAL=1 NODE_ENV=production LOG_FORMAT=text deno test --preload=src/schemas/_test-setup.ts --parallel --allow-all --no-check src/skill/ src/agent/→1183 passed (2057 steps) | 0 faileddeno task typecheck→ exit 0deno lint→ exit 0, "Checked 4882 files"deno task lint:test-typecheck→ exit 0, "Test typecheck baseline holds: 51 grandfathered files, 0 new."deno task docs:api-reference:check→ exit 0, "docs/api-reference is current (43 files)" (no regeneration needed)deno fmt src/→ no changes3769 passed (28028 steps) | 0 failed | 1 ignored (5 steps)Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
Summary by CodeRabbit
Bug Fixes
Tests