feat(agent): depersonalize the load_skill tool schema for a shared cache prefix (RFC 0001) - #3433
Conversation
…refix) The load_skill tool definition baked the project's skill IDs into its description, its skillId enum, and the enum's describe() text — so the tools array differed per project and fragmented the prompt-cache prefix. Advertise a STATIC schema + description instead: skill IDs live in the <available_skills> context block, not the tool definition. The per-project dynamic schema is still used for .parse() validation at execution, so all runtime enforcement (valid IDs, body-reload/reference rules) is preserved — the model just no longer sees the per-project enum. Inventory bounds/ proxy-safety validation and the private-authority scope refresh are kept. Retargets the schema-advertising tests to a shared STATIC_LOAD_SKILL_INPUT_SCHEMA constant (now guarding state-independence); enforcement stays covered by the execute() tests. Implements RFC 0001 (veryfront-agent #1788), load_skill slice. Gates before merge: full agent-slice suite; live eval on model skill-picking.
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe load-skill tool now advertises a project-independent static schema. Runtime validation still enforces loading and reference rules. Skill prompts and inventory requests expose authorized skill IDs with bounded pagination. ChangesRuntime skill discovery and authorization
Estimated code review effort: 4 (Complex) | ~60 minutes Merge Risk: 🔵 Low · up to The PR makes the load_skill schema project-independent while retaining runtime authorization, and the supplied checks pass. One bounded follow-up remains: a regression test may be skipped by Node or Bun runners because it uses Deno.test(), so owner awareness is warranted; no other merge-blocking risk is shown. Sequence Diagram(s)sequenceDiagram
participant Agent
participant load_skill
participant SkillContext
participant SkillCatalog
Agent->>load_skill: Request inventory page
load_skill->>SkillContext: Read authorized skill IDs
SkillContext->>SkillCatalog: Build bounded catalog page
SkillCatalog-->>load_skill: Return skillIds and nextCursor
load_skill-->>Agent: Return inventory page
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/agent/runtime/load-skill-tool.test.ts`:
- Line 2159: Replace the affected Deno.test declarations in the load-skill-tool
tests with the repository BDD API by importing describe and it from
`#veryfront/testing/bdd.ts`, grouping the tests under describe blocks as
appropriate, and defining each case with it while preserving the existing test
bodies and assertions.
In `@src/agent/runtime/load-skill-tool.ts`:
- Line 1352: Update the public description returned by the runtime skill tool in
load-skill-tool.ts at lines 1352-1352 to say “You must not invent IDs.”, and
update the corresponding assertion in load-skill-tool.test.ts at lines 2808-2812
to verify the revised requirement text.
🪄 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: 84c0a32c-7e9d-4844-8685-59ecde8a0ada
📒 Files selected for processing (2)
src/agent/runtime/load-skill-tool.test.tssrc/agent/runtime/load-skill-tool.ts
Runs `deno task docs` to satisfy the CI api-reference staleness check.
The draft branch had diverged substantially from main. Integrating current runtime changes keeps the static provider schema compatible with the repository's active skill policy while preserving legacy alias execution behind runtime validation. Constraint: The provider-facing schema must remain project-independent without removing runtime compatibility for existing .md aliases. Rejected: Regenerate every API reference file | the clean main branch already fails the generator check for all generated modules, so mass churn would obscure this slice. Confidence: high Scope-risk: moderate Directive: Keep the advertised load_skill schema static; enforce project-specific authority only during execution. Tested: Focused load-skill tool suite, formatting, and diff checks before integration. Not-tested: Live provider skill-picking evaluation requires external credentials and project context.
📦 Client bundle boundary
A server module in a client graph aborts hydration in the browser. New leaks fail CI; known leaks are tracked in |
The call-context split now sits on current main without restoring removed skill-orchestration arguments or stale catalogue prose. Modified tests use the repository BDD surface, and public call-context documentation uses ASCII punctuation. Constraint: The full provider tools invariant still depends on PR #3433 making the advertised load_skill definition project-independent. Rejected: Reintroduce removed available-tool orchestration options | current main intentionally centralizes that policy outside the skill catalogue. Confidence: medium Scope-risk: moderate Directive: Do not claim full shared-prefix equality until the tools regression passes with PR #3433 integrated. Tested: 21 focused runtime and hosted call-context tests; targeted format, lint, typecheck, and diff checks. Not-tested: Full provider tools equality and full repository suite pending PR #3433 integration.
The layered call-context branch now integrates the static advertised load_skill contract from PR #3433 and locks the combined invariant across standalone and hosted project contexts. Different project IDs and skill manifests produce identical provider tools and identical first system messages. Constraint: PR #3435 depends on PR #3433 because project-specific load_skill descriptions or schemas invalidate the shared prefix. Rejected: Duplicate the load_skill depersonalization in this branch | merging the verified dependency preserves one implementation and its focused coverage. Confidence: high Scope-risk: moderate Directive: Keep project skill IDs in the dynamic available_skills block and runtime validation, never in the provider-advertised load_skill definition. Tested: 97 focused load_skill, shared-prefix, call-context, agent-definition, and hosted-system-message tests; targeted format, lint, typecheck, and diff checks. Not-tested: Full repository suite will run through the branch pre-push hook.
The provider-facing schema adds source lines in the public agent module. Regenerating with the CI-pinned Deno version updates only the affected source anchors and closes the branch-specific lint failure without unrelated documentation churn. Constraint: Generated API references are validated with Deno 2.7.7 in CI. Rejected: Regenerate with the ambient Deno version | it produces unrelated drift across every module. Confidence: high Scope-risk: narrow Directive: Use the CI-pinned Deno version when refreshing generated API references. Tested: Pinned `docs:api-reference:check` and generated-file diff inspection. Not-tested: Live provider skill-picking evaluation still requires external credentials.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
![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: ea84f2c06f
ℹ️ 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".
|
Addressed the remaining P1 in Exact-head local verification:
@codex review |
A static load_skill schema protects the shared provider prefix, but the dynamic catalog truncated summaries after thirty entries and pointed to the removed schema inventory. Retain bounded full summaries for the first entries and publish an escaped, compact ID list for every omitted skill in the uncached catalog tail. Constraint: The load_skill provider schema must remain byte-identical across projects.\nRejected: Restore project-specific schema enums | that breaks the shared cache prefix.\nRejected: Render every full summary | that makes prompt size scale with all skill metadata.\nConfidence: high\nScope-risk: narrow\nDirective: If catalog summaries remain bounded, omitted authorized IDs must stay present in dynamic context.\nTested: red-green skill-prompt regression; 89 focused tests and 23 nested steps; targeted format, lint, typecheck, and diff check.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 115049baa7
ℹ️ 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".
Preserve the independently published omitted-ID repair and its accessor, injection, compatibility, and generated-doc regressions while retaining per-ID serialization that does not invoke inherited array hooks. Constraint: The remote branch advanced without rewriting the reviewed base. Rejected: Serialize the omitted ID array directly | inherited Array.prototype.toJSON hooks could alter prompt output. Confidence: high Scope-risk: narrow Directive: Preserve all catalog discovery and injection tests when modifying the runtime skill prompt. Tested: 92 focused tests and 23 nested steps, targeted format, lint, typecheck, API reference generation, and diff checks.
The static project-independent schema rejected lowercase .md IDs that runtime validation and the dynamic skill catalog explicitly support. Align the advertised pattern and bound without reintroducing project-specific enums. Constraint: The tool schema must stay byte-identical across projects for prompt caching. Rejected: Strip .md from the dynamic catalog | canonical project skill IDs may include the suffix. Confidence: high Scope-risk: narrow Directive: Keep static provider validation aligned with every canonical runtime skill ID form. Tested: 93 focused tests and 23 nested steps, targeted format, lint, typecheck, API reference generation, and diff checks.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5ae35eb60b
ℹ️ 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".
Remote concurrency added bounded discovery, but placed overflow guidance inside the authorized-ID array block. Merge the concurrent work while keeping authorized_skill_ids parseable as an array-only block and leaving overflow discovery in a separate block that points to load_skill cursor paging. Constraint: PR #3433 requires authored complete available_skills to suppress the generated catalog while still exposing authorized IDs through parseable prompt data. Constraint: Remote advanced during pre-push, so this integrates without rewriting either branch head. Rejected: Keep overflow copy inside authorized_skill_ids | consumers can no longer treat the block as array-only data. Rejected: Rebase away the concurrent remote commit | branch history had already moved and non-force integration was required. Confidence: high Scope-risk: narrow Directive: Keep authorized_skill_ids array-only; put cursor instructions in adjacent discovery text. Tested: deno fmt --check on touched runtime files Tested: deno lint on touched runtime files Tested: deno check on touched runtime files and src/agent/index.ts Tested: focused runtime tests passed 104 tests Tested: deno task docs and docs:api-reference:check Not-tested: live model skill-picking eval requiring configured provider credentials
Provider sanitizers can flatten union object variants, so the static load_skill schema now advertises one project-independent object with dependency constraints instead of a Zod-derived anyOf. Runtime parsing still uses the stricter execution schema, while provider-visible metadata keeps file tied to skillId and keeps cursor separate from load inputs. Constraint: Static tool schemas must stay byte-identical across projects and survive Anthropic and Google schema sanitizers. Rejected: Keep the static Zod union | Anthropic merges the branches and Google drops root anyOf, advertising invalid combinations. Rejected: Add project-specific tool variants | that would break the shared-cache static schema contract. Confidence: high Scope-risk: narrow Directive: Do not reintroduce root anyOf/oneOf/allOf to the static load_skill provider schema without testing provider sanitization. Tested: deno fmt --check on touched runtime files Tested: deno lint on touched runtime files Tested: deno check on touched runtime files and src/agent/index.ts Tested: focused runtime tests passed 105 tests Tested: deno task docs and docs:api-reference:check Not-tested: provider-side live tool-call validation against external model APIs
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b98c16e9e1
ℹ️ 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".
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c560a29062
ℹ️ 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".
|
@codex review Exact head 30ab742 adds the red-green generated-catalog recomposition regression and preserves authored-catalog compatibility. Verification: 107 focused tests plus 28 test steps passed; full deno fmt check, focused deno check/lint, generated API reference check, and git diff --check passed. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 30ab7422c8
ℹ️ 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".
|
@codex review Exact head be75774 replaces prose matching with a reserved generated-catalog marker and includes the hosted preassembled-skill boundary required by the prior failing unit shard. Red-green verification: authored-prose preservation and hosted selector tests failed before their fixes and pass now. Expanded suite: 142 tests plus 44 test steps passed; full format, focused check/lint, test-typecheck baseline, generated API docs, and git diff --check pass. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: be757742a3
ℹ️ 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".
The PR branch advanced with PR #3433's generated skill provenance and structured cache metadata fixes after the hosted preassembled-context merge. This merge preserves that remote lineage, keeps the hosted preassembled marker regression, and confirms the combined runtime context behavior remains compatible. Constraint: The PR branch advanced twice during local validation, so every remote descendant had to be integrated before push. Rejected: Push the previous validated head | would discard PR #3433 lineage now present on the PR branch. Confidence: high Scope-risk: moderate Directive: Keep PR #3433 lineage merged into this branch when validating PR #3435 because the skill catalog provenance fixes now interact with layered call context. Tested: deno fmt on touched runtime/docs files; focused hosted/factory/runtime suites passed; Deno check on touched runtime and hosted files passed; docs API reference check passed; .husky/pre-push passed on the same tree. Not-tested: Live provider request outside mocked hosted runtime tests
|
@codex review exact head c8a45e4. Please verify the red-green duplicate-skill paging fix: discovery cursors now derive from first-seen deduplicated skill IDs, matching load_skill authorization. Focused skill/runtime tests, public checks, lint, format, typecheck baseline, and generated API reference checks pass. |
|
Codex Review: Didn't find any major issues. Keep them coming! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
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". |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c8a45e4568
ℹ️ 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".
PR #3435 must explicitly include the review-follow-up correction from the merged #3433 stack. The guide content already preserved the hosted nested form and local/project flat form split, so this merge commit records the exact 00cdb66 ancestry without changing the final wording beyond the conflict-preserved first-page flat inventory call. Constraint: Root audit requires exact 00cdb66 ancestry or equivalent non-destructive integration on #3435. Rejected: Empty cherry-pick | it preserved content but did not make 00cdb66 an ancestor for the audit gate. Confidence: high Scope-risk: narrow Tested: deno fmt --check docs/guides/skills.md Tested: git diff --check Tested: deno test --preload=src/testing/preload.ts --no-check --allow-all src/internal-agents/run-system-prompt.test.ts src/internal-agents/run-stream.test.ts src/agent/runtime/agent-runtime-step.test.ts src/agent/hosted/child-fork-step-message-preparation.test.ts Tested: deno test --preload=src/testing/preload.ts --no-check --allow-all src/agent/factory-call-context.test.ts src/agent/runtime/model-resolution.test.ts src/agent/runtime/model-transport.test.ts src/agent/runtime/agent-runtime-step.test.ts src/agent/runtime/refresh.test.ts src/agent/runtime/call-context.test.ts extensions/ext-llm-anthropic/src/anthropic-request-builder.test.ts src/agent/hosted/child-fork-step-message-preparation.test.ts src/agent/hosted/child-fork-execution-runner.test.ts src/internal-agents/run-system-prompt.test.ts src/internal-agents/run-stream.test.ts Not-tested: full repository test suite before push
2 participants
Implements the
load_skillslice of RFC 0001 (veryfront-agent #1788): make theload_skilltool definition project-independent so thetoolsarray can join the shared prompt-cache prefix, while preserving exhaustive runtime skill-ID discovery when authored catalogs replace the generated one.Problem
load_skillpreviously baked project-specific skill IDs into the tool definition. Becausetoolsprecedesystemin the request, per-project tool definitions fragment the shared prompt-cache prefix. Authored<available_skills>blocks can also suppress the generated catalog, so the model still needs a separate, injection-safe list of authorized runtime skill IDs.Change
Advertise a static schema and description; keep the dynamic schema for validation.
inputSchemaJsonadvertises a project-independent schema.buildRuntimeLoadSkillDescriptionreturns invariant copy and points the model at<available_skills>or<authorized_skill_ids>.buildAgentCallContextappends an array-only<authorized_skill_ids>block when authored<available_skills>is present, including the empty authorized set as[].<authorized_skill_ids>stays exhaustive. The bounded omitted-ID summary applies only to generated<available_skills>text.load_skillconsumers can omitskillIdand page authorized IDs withcursor; runtime execution still validates against the actual authorized skill manifest and reference authorization state.<available_skills>omits a hidden suffix, the prompt now gives the first hidden cursor so paging continues after the IDs already embedded in the prompt.Validation
Current head:
563a462729fda26c574f11d7f944676ca6090d33.Focused checks on the exact head:
deno fmt --checkon touched runtime/docs files: pass,Checked 7 files.deno linton touched runtime files: pass,Checked 6 files.deno checkon touched runtime files plussrc/agent/index.ts: pass.skill-prompt,call-context, andload-skill-tool:102 passed (25 steps), 0 failed.deno task docs:api-reference:check: pass, generated docs are current.git diff --check: pass.Full pre-push gate evidence from this branch:
cf1691617ed593c26f16fe00e59928b527fec8b9: pass,3829 passed (28514 steps), 0 failed, 1 ignored, plus format, lint, typecheck, and cwd suites.3785 passed (28514 steps), 0 failed, 1 ignoredbefore a Deno runner pending-promise flake aborted the hook; no test failed.Gates
563a462729fda26c574f11d7f944676ca6090d33.Summary by CodeRabbit
New Features
load_skillcan list authorized skill IDs with cursor-based pagination when no skill is selected.Bug Fixes
Documentation