refactor(agent): reduce <available_skills> to the skill catalogue - #3475
Conversation
The block carried nine sentences of orchestration policy around the skill list: how load_skill behaves, continue-the-same-turn, root ownership, which delegation tools to use, when to delegate, not to narrate delegation, and not to call absent tools. It also appended each skill tool's call signature, and emitted model/thinking/maxSteps per record. Almost none of that is about which skills exist. Delegation and output-style policy belong to the agent's instructions; how load_skill behaves belongs to its tool description; model/thinking/maxSteps are returned structurally by load_skill, which is when a caller needs them. One sentence stays, and it is not orchestration: skill names and descriptions are author-supplied and get interpolated into trusted context, so the records are labelled as data. Two load_skill tools exist - the standalone one the agent() factory registers and the hosted one - and only the hosted description stated the orchestration contract. Trimming the block alone would have left every factory-built agent with that policy in no prompt at all. The shared clauses now live in src/skill/load-skill-policy.ts, which both tools import; src/agent may import from src/skill but not the reverse. Also drops availableToolNames and includeSkillToolUsage from BuildAgentCallContextInput, the prompt options, and CreateVeryfrontCloudRuntimeSystemMessagesInput, where a conditional spread had been hiding a discarded field from the typechecker. Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
|
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 (4)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe change centralizes ChangesSkill prompt and load_skill policy
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 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: cd043c1eed
ℹ️ 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."; |
There was a problem hiding this comment.
When an agent has an explicit delegates binding and loads a skill with model, thinking, or maxSteps overrides, this shared clause directs it to forward them through invoke_agent; however, both the factory and hosted scoped-delegation paths expose only agent_<id> tools, not invoke_agent. Such runs can therefore attempt a nonexistent tool or fail to apply the overrides. Keep this clause conditional on the legacy invoke_agent inventory rather than placing it in the description shared by every load_skill tool.
Useful? React with 👍 / 👎.
| export function buildRuntimeAvailableSkillsPromptBlock( | ||
| skills: readonly RuntimeSkillDefinition[], | ||
| options: RuntimeSkillPromptOptions = {}, | ||
| ): string { |
There was a problem hiding this comment.
buildRuntimeAvailableSkillsPromptBlock is exported through veryfront/agent, so removing its optional second parameter breaks existing consumers that pass availableToolNames or includeSkillToolUsage with an Expected 1 arguments, but got 2 type error. The catalogue behavior can change while retaining a deprecated, ignored compatibility parameter instead of making this refactor a public API break.
AGENTS.md reference: AGENTS.md:L9-L14
Useful? React with 👍 / 👎.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@src/skill/load-skill-policy.ts`:
- Around line 32-34: Align createLoadSkillTool with the shared
LOAD_SKILL_OVERRIDE_FORWARDING contract by returning normalized model, thinking,
and maxSteps override fields alongside its existing result fields, while keeping
allowedTools absent and preserving compatibility for callers. Update or add
tests covering both load_skill loaders to verify the structured result contract
and override forwarding behavior.
🪄 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: 3da5dc5d-d08d-4d80-ad31-e617d606ef87
📒 Files selected for processing (17)
docs/api-reference/veryfront/agent.mddocs/api-reference/veryfront/skill.mdsrc/agent/conversation/delegation-policy.tssrc/agent/factory-call-context.test.tssrc/agent/factory.test.tssrc/agent/factory.tssrc/agent/hosted/cloud-runtime-system-messages.test.tssrc/agent/hosted/cloud-runtime-system-messages.tssrc/agent/runtime/call-context.test.tssrc/agent/runtime/call-context.tssrc/agent/runtime/load-skill-tool.tssrc/agent/runtime/skill-prompt.test.tssrc/agent/runtime/skill-prompt.tssrc/internal-agents/run-system-prompt.tssrc/skill/load-skill-policy.tssrc/skill/tools.test.tssrc/skill/tools.ts
💤 Files with no reviewable changes (5)
- src/agent/factory.test.ts
- src/internal-agents/run-system-prompt.ts
- src/agent/factory-call-context.test.ts
- src/agent/runtime/skill-prompt.test.ts
- src/agent/hosted/cloud-runtime-system-messages.ts
…urns it
The shared load_skill clauses included "pass through any returned model,
thinking, or maxSteps overrides". createLoadSkillTool - the loader the
agent() factory registers - returns { skillId, instructions, references,
scripts } and never those fields, so a factory-built agent was told to
forward something its load_skill cannot produce.
Move that clause out of the shared set and onto the hosted description
only, which is the loader that actually returns overrides. A test pins
the asymmetry in both directions.
Found by CodeRabbit and Codex review on #3475.
Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
|
Both reviews addressed. Pushed Override forwarding in the shared clauses — valid, fixedCodeRabbit and Codex found this from different angles and both were right.
That was my error: I bundled four clauses as "the contract" without checking that both loaders could satisfy all four. Fix: Two tests pin it: both descriptions carry the shared clauses, and the factory description does not carry override forwarding. Codex's related point — that scoped-delegation runs expose Removing the second parameter of
|
1 participant
Follow-up to #3473 (veryfront/veryfront-issue-inbox#5). The system prompt should list which skills exist; it should not carry orchestration policy.
Before
Plus, for callers that opted in, an appendix restating each skill tool's call signature.
After
Why each piece went
<available_skills>tag says this.model/thinking/maxStepsper recordload_skill, which is when a caller needs them for delegation.Kept: the untrusted-metadata sentence. Skill names and descriptions are author-supplied and are interpolated into trusted context; that line marks the records as data. It is a trust boundary, not orchestration.
The blocker this had to fix first
An adversarial review of the first draft caught a real defect, verified independently before acting on it.
There are two
load_skilltools, and only one stated the orchestration contract:createRuntimeLoadSkillTool→hosted/project-steering-adapter.ts:226agent()factorycreateLoadSkillTool(src/skill/tools.ts:482)So for every factory-built agent, this block was the only carrier. Trimming it alone would have dropped continue-same-turn, root ownership, the delegation threshold, and override forwarding into nothing. The deleted code said so itself, in a docstring this PR also removes: "agents built by the
agent()factory have carried them in the prompt since the factory rendered its own skill manifest."Fix: the shared clauses now live in
src/skill/load-skill-policy.ts, which both tools import.src/agentmay import fromsrc/skillbut not the reverse, so this is the only direction that works without breaking a boundary the repo maintains;lint:module-boundariesandlint:dependency-boundariesboth pass.delegation-policy.tsre-exports them, so agent-side import sites are unchanged.Both descriptions now state the same contract and differ only in their own mechanics:
A regression test pins that both carry
LOAD_SKILL_POLICY_CLAUSES. Verified non-vacuous — removing the clauses from either tool fails it.#3473 is unaffected. It removed
nextStepfrombuildStrictRuntimeLoadedSkillResponse, which only the hosted tool uses; the factory tool returns{skillId, instructions, references, scripts}and never hadnextStep.A silently discarded field
CreateVeryfrontCloudRuntimeSystemMessagesInput.availableToolNameswas still declared and forwarded intobuildAgentCallContextafter that input type dropped the field. The value arrived through a conditional spread, which suppresses TypeScript's excess-property check, sodeno checkpassed while the value was discarded and its producers kept computing it. Removed from the input type, the forwarding, and the caller.Cascade
The block no longer depends on the run's tool inventory at all, which made a chain of things dead:
availableToolNamesandincludeSkillToolUsageonBuildAgentCallContextInputand the prompt optionsresolveConfiguredToolNamesinfactory.ts, and aconfiguredToolNamesparameter with itbuildStrictRuntimeSkillDelegationGuidance,buildStrictSkillToolUsage,snapshotAvailableToolNames,getStrictScopedDelegateToolNames,includesExactString,MAX_RUNTIME_SKILL_AVAILABLE_TOOL_NAMESsrc/internal-agents/run-system-prompt.ts:132, found bylint:test-typecheckrather than by the entrypoint typecheckTesting
src/skill/,src/agent/,src/internal-agents/— 1195 passed, 2172 steps, 0 faileddeno task typecheck— 0 errorsdeno lint(4881 files) — cleanlint:test-typecheck— "51 grandfathered files, 0 new"lint:module-boundaries,lint:dependency-boundaries— OKdocs:api-reference:check— currentTests asserting the removed prose were deleted rather than weakened;
call-context.test.tswas rewritten to assert the block contains the catalogue and that none of the six removed prose fragments appear.Worth watching
The continuation instruction now reaches the model once (the tool description) rather than twice. Baseline for the stop-after-
load_skillsymptom, measured while triaging veryfront/veryfront-issue-inbox#392: 2 runs in 30 days. If that climbs, this PR and #3473 are the first suspects.🤖 Generated with Claude Code
Summary by CodeRabbit