refactor(agent): drop opaque orchestration prose from load_skill results - #3473
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 (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change simplifies runtime loaded-skill responses by removing continuation, message, tool, and note fields. It updates load-skill options, public exports, tests, and API reference links. ChangesRuntime skill response simplification
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 |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/runtime/load-skill-tool.test.ts (1)
141-142: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy liftUse the required BDD test API.
This changed test remains registered with
Deno.test. Convert the touched test cases todescribe()andit()from#veryfront/testing/bdd.ts. Use assertions from#veryfront/testing/assert.ts.As per coding guidelines, "
**/*.{test,spec}.ts: Usedescribe()andit()from#veryfront/testing/bdd.ts, use assertions from#veryfront/testing/assert.ts."🤖 Prompt for 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. In `@src/agent/runtime/load-skill-tool.test.ts` around lines 141 - 142, Convert the affected tests in load-skill-tool.test.ts from Deno.test to describe() and it() imported from `#veryfront/testing/bdd.ts`, and replace their assertions with imports from `#veryfront/testing/assert.ts`. Preserve the existing test cases and expectations while registering them through the required BDD API.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@src/agent/runtime/load-skill-tool.test.ts`:
- Around line 141-142: Convert the affected tests in load-skill-tool.test.ts
from Deno.test to describe() and it() imported from `#veryfront/testing/bdd.ts`,
and replace their assertions with imports from `#veryfront/testing/assert.ts`.
Preserve the existing test cases and expectations while registering them through
the required BDD API.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 6260f4d4-6723-4231-96fd-f78393d2365e
📒 Files selected for processing (6)
docs/api-reference/veryfront/agent.mdsrc/agent/index.tssrc/agent/runtime/load-skill-tool.test.tssrc/agent/runtime/load-skill-tool.tssrc/agent/runtime/skill-metadata.test.tssrc/agent/runtime/skill-metadata.ts
💤 Files with no reviewable changes (3)
- src/agent/index.ts
- src/agent/runtime/skill-metadata.test.ts
- src/agent/runtime/load-skill-tool.ts
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c5d84edc9
ℹ️ 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".
| delegationNote?: string; | ||
| /** Reference files this skill advertises, loadable via load_skill's `file` parameter. */ | ||
| references?: string[]; | ||
| /** Model override the skill declares, for the caller to forward when delegating. */ |
There was a problem hiding this comment.
When an agent's instructions already contain a complete <available_skills> element, buildAgentCallContext deliberately skips the generated block (src/agent/runtime/call-context.ts:168-178), and RUNTIME_LOAD_SKILL_DESCRIPTION does not contain LOAD_SKILL_OVERRIDE_FORWARDING. In a legacy invoke_agent run where a skill declares model, thinking, or maxSteps, the old nextStep and overrideNote supplied that missing instruction, but this response now emits only the raw values, so the model is never told to forward them and can delegate with the wrong configuration. Keep the forwarding guidance in trusted prompt or tool-description text outside the replaceable skills block.
Useful? React with 👍 / 👎.
load_skill returned a nextStep string carrying internal orchestration policy - "Continue the same turn after calling it", "Keep the root assistant visibly owning the work", delegation guidance - alongside the skill's actual instructions. It also returned overrideNote and referenceNote, two more prose fields, plus five fields left dead by veryfront/veryfront-issue-inbox#406. Every clause of nextStep is already in the <available_skills> system prompt, word for word. The result was restating trusted system policy as untrusted tool output, which is what this issue objects to. The result is now only structured data with a documented consumer: { skillId, instructions, references?, model?, thinking?, maxSteps? } Removed: nextStep, overrideNote, referenceNote, allowedTools, note, delegationTools, unavailableCurrentRunTools, delegationNote, the RuntimeLoadedSkillResponseMessages configuration surface, and the nextStep/messages tool options that existed to override the prose. The continuation signal now lives in exactly one place - the system prompt - rather than being duplicated into every load_skill result. Fixes veryfront/veryfront-issue-inbox#5. Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
2c5d84e to
8c30128
Compare
buildAgentCallContext skips the generated <available_skills> block when an agent's own instructions already contain one (call-context.ts:168). For those agents the system prompt carries whatever the author wrote, so it cannot be relied on to hold runtime orchestration policy. The tool description already carried continue-same-turn, root ownership, and the delegation threshold, but not override forwarding. Dropping nextStep and overrideNote therefore left an authored-block agent with no instruction to pass a skill's model, thinking, or maxSteps through to a legacy invoke_agent delegation. Move that clause into the tool description, which is always sent. It belongs in the trusted tool contract rather than in the result payload, which is what veryfront/veryfront-issue-inbox#5 asks for. Found by Codex review on #3473. Claude-Session: https://claude.ai/code/session_01Xo93b6StAu691YV9g8Fm53
|
Addressed both reviews. Pushed Codex — override forwarding for authored skill blocks: valid, fixedThis was a real gap and it undermined this PR's central claim, so thank you for catching it. My argument was "every clause of if (input.skills?.length && !hasBlock(input.instructions, AVAILABLE_SKILLS_BLOCK_NAME)) {For an agent with an authored block, the system prompt carries whatever the author wrote. My premise does not hold there. Checked what the always-present tool description carries:
So continuation, ownership, and the delegation threshold survived an authored block; override forwarding was the single clause that did not. Exactly as you described. Fix: moved Added a regression test pinning that the description carries all four clauses, so this cannot silently reopen. Verified non-vacuous — removing the clause fails it ( CodeRabbit — convert touched tests to
|
Fixes veryfront/veryfront-issue-inbox#5.
load_skillreturned anextStepstring carrying internal orchestration policy alongside the skill's actual instructions:The finding that makes this safe
Every clause of that string is already in the
<available_skills>system prompt, word for word. Rendered from currentmain:nextStepclauseSo the result was restating trusted system policy as untrusted tool output — exactly the duplication the issue objects to. Removing it deletes a copy, not a signal.
What the result looks like now
{ "skillId": "deploy", "instructions": "---\nname: deploy\n...\n---\nDo the deploy.", "model": "sonnet", "references": ["references/guide.md"] }Every field is structured, and every field has a documented consumer:
skillIdinstructionsreferencesload_skill'sfileparametermodel/thinking/maxStepsRemoved
Prose fields:
nextStep— the orchestration policy aboveoverrideNote— "Pass through any returned model, thinking, or maxSteps overrides…", verbatim in the system promptreferenceNote— "After this skill is loaded, use load_skill with thefileparameter…", which duplicates theload_skilltool descriptionDead since #3464, still on the type and still being copied by
copyLoadedSkillResponse:allowedTools,note,delegationTools,unavailableCurrentRunTools,delegationNoteassertRuntimeResponseMetadata, which validateddelegationToolsthat nothing emittedConfiguration surface that existed only to override the deleted prose:
RuntimeLoadedSkillResponseMessages,RuntimeLoadSkillToolMessages,RUNTIME_LOAD_SKILL_CONTINUATION_NOTE, and thenextStep/messagestool optionssrc/agent/index.tsavailableToolNamesis also dropped frombuildStrictRuntimeLoadedSkillResponse— it was read and never used onceoverrideNotewent. Its bound is still enforced, independently, byassertRuntimeBoundaryCollections.Acceptance criteria
load_skillschema documents every returned field and its consumer — each field onRuntimeLoadedSkillResponsenow carries a doc comment naming who reads itnextStepThe one criterion I cannot prove in tests
The continuation instruction now appears once (system prompt) instead of twice (system prompt + every
load_skillresult). Unit tests can prove the signal still exists; they cannot prove a model still obeys it.This is worth watching, because veryfront/veryfront-issue-inbox#392's residual is precisely "a child stopped after
load_skill". I measured that baseline while triaging it: 2 runs in 30 days acrossagent_run_eventmatched the stop-after-load_skill signature (the raw figure was 561, but 558 of those were one retiredcontrol-agentburst that ended 2026-07-15).So there is a concrete before-number to compare against. If the rate climbs above ~2/month after this ships, this change is the first suspect and reverting the
nextStepremoval is the obvious probe.Testing
src/skill/+src/agent/— 1186 passed, 2050 steps, 0 faileddeno task typecheck— 0 errorsdeno lint src/— cleandeno task docs:api-reference:check— currentdeno fmt --check— cleanTests asserting the removed fields were deleted rather than weakened (6 in
load-skill-tool.test.ts, 1 inskill-metadata.test.ts), all of which existed only to pin prose or its configurability. Where a test's real subject survived — accessor rejection, input bounds, iterator snapshotting — the removed-field half was dropped and the rest kept.On the full-tree run:
src/ cli/ tests/shows 26 failures against a 8-failure baseline, but the difference is entirely e2e/dev-server suites (CSS, MDX Pages, Static Files, Relative Import Resolution, skill-capabilities). I verifiedtests/e2e/features/skill-capabilities.test.tsfails identically with my changes stashed —403vs expected200, a local admission failure — and that file contains zero references tonextStep. These suites are non-deterministic under parallel dev servers in this checkout; CI runs them in dedicated jobs.🤖 Generated with Claude Code
Summary by CodeRabbit
Updates
Documentation