From bb13d5b9ee9e781bc439f661b4ec17fa65cc20d4 Mon Sep 17 00:00:00 2001 From: Danny Avila Date: Sun, 19 Apr 2026 10:03:23 -0700 Subject: [PATCH] =?UTF-8?q?=F0=9F=AA=9C=20chore:=20Plumb=20`allowedTools`?= =?UTF-8?q?=20through=20`resolveManualSkills`=20(#12744)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 🪜 chore: Plumb `allowedTools` through `resolveManualSkills` Tiny shape-only precursor shared by Phase 5 (`always-apply`) and Phase 6 (frontmatter runtime enforcement). Adds `allowedTools?: string[]` to `ResolvedManualSkill` and widens the `getSkillByName` return type in `ResolveManualSkillsParams` and `InitializeAgentDbMethods` to carry the same field. The resolver forwards `skill.allowedTools` verbatim when present. No runtime behavior change — the field is populated but not yet consumed. Phase 6 will union the per-skill allowlist into the agent's effective tool set for the turn; Phase 5's `ResolvedAlwaysApplySkill` will mirror this shape. Landing this first eliminates the type-shape race between the two phases. * 🪜 style: Drop phase-name refs from `allowedTools` JSDoc for longevity JSDoc that cites "Phase N will X" goes stale the moment that phase ships. Swap for "future runtime enforcement" so the docs age with the code. --- .../api/src/agents/__tests__/skills.test.ts | 40 +++++++++++++++++++ packages/api/src/agents/initialize.ts | 6 +++ packages/api/src/agents/skills.ts | 20 +++++++++- 3 files changed, 65 insertions(+), 1 deletion(-) diff --git a/packages/api/src/agents/__tests__/skills.test.ts b/packages/api/src/agents/__tests__/skills.test.ts index 6dfa100604..df91bf8ecc 100644 --- a/packages/api/src/agents/__tests__/skills.test.ts +++ b/packages/api/src/agents/__tests__/skills.test.ts @@ -604,6 +604,7 @@ describe('resolveManualSkills', () => { name: string; body: string; author: Types.ObjectId; + allowedTools?: string[]; }; const buildGetSkillByName = @@ -649,6 +650,45 @@ describe('resolveManualSkills', () => { expect(result).toEqual([{ name: 'my-skill', body: 'MY SKILL BODY' }]); }); + it('passes allowedTools through when the skill doc carries the field', async () => { + const owned: SkillDoc = { + ...mkSkill('with-tools', userOid, 'body'), + allowedTools: ['execute_code', 'read_file'], + }; + const result = await resolveManualSkills({ + names: ['with-tools'], + getSkillByName: buildGetSkillByName({ 'with-tools': owned }), + accessibleSkillIds: [owned._id], + userId, + }); + expect(result).toEqual([ + { name: 'with-tools', body: 'body', allowedTools: ['execute_code', 'read_file'] }, + ]); + }); + + it('omits allowedTools when the skill doc does not declare it', async () => { + const owned = mkSkill('no-tools', userOid, 'body'); + const [resolved] = await resolveManualSkills({ + names: ['no-tools'], + getSkillByName: buildGetSkillByName({ 'no-tools': owned }), + accessibleSkillIds: [owned._id], + userId, + }); + expect(resolved).toEqual({ name: 'no-tools', body: 'body' }); + expect(resolved).not.toHaveProperty('allowedTools'); + }); + + it('preserves an empty allowedTools array (distinguishes "declared none" from "undeclared")', async () => { + const owned: SkillDoc = { ...mkSkill('empty-tools', userOid, 'body'), allowedTools: [] }; + const [resolved] = await resolveManualSkills({ + names: ['empty-tools'], + getSkillByName: buildGetSkillByName({ 'empty-tools': owned }), + accessibleSkillIds: [owned._id], + userId, + }); + expect(resolved).toEqual({ name: 'empty-tools', body: 'body', allowedTools: [] }); + }); + it('silently skips names with no backing skill (typo / ACL miss) without failing the batch', async () => { const real = mkSkill('real', userOid); const result = await resolveManualSkills({ diff --git a/packages/api/src/agents/initialize.ts b/packages/api/src/agents/initialize.ts index f94eb6c012..ec4e4539fe 100644 --- a/packages/api/src/agents/initialize.ts +++ b/packages/api/src/agents/initialize.ts @@ -201,6 +201,12 @@ export interface InitializeAgentDbMethods extends EndpointDbMethods { name: string; body: string; author: import('mongoose').Types.ObjectId; + /** + * Skill-declared tool allowlist, forwarded verbatim from the skill doc. + * Surfaced so the resolver can carry it onto `ResolvedManualSkill` for + * future runtime enforcement without a second round-trip. + */ + allowedTools?: string[]; } | null>; } diff --git a/packages/api/src/agents/skills.ts b/packages/api/src/agents/skills.ts index 6d9eafbff3..84d019e3a5 100644 --- a/packages/api/src/agents/skills.ts +++ b/packages/api/src/agents/skills.ts @@ -316,6 +316,13 @@ export interface ResolveManualSkillsParams { name: string; body: string; author: Types.ObjectId | string; + /** + * Skill-declared tool allowlist, forwarded verbatim from the skill doc. + * Surfaced on `ResolvedManualSkill` so future runtime enforcement can + * union it into the agent's effective tool set for the turn without + * re-fetching the document. Populated by the DB method when available. + */ + allowedTools?: string[]; } | null>; /** ACL-accessible skill IDs for this user (already scoped by `scopeSkillIds`). */ accessibleSkillIds: Types.ObjectId[]; @@ -330,6 +337,13 @@ export interface ResolveManualSkillsParams { export interface ResolvedManualSkill { name: string; body: string; + /** + * Skill-declared tool allowlist passed through from the skill doc. Present + * only when the skill author declared `allowed-tools` in frontmatter. + * Currently populated but not consumed — future runtime enforcement will + * union these into the agent's effective tool set for the turn. + */ + allowedTools?: string[]; } /** @@ -414,7 +428,11 @@ export async function resolveManualSkills( logger.warn(`[resolveManualSkills] Skill "${name}" is inactive for this user — skipping`); return null; } - return { name: skill.name, body: skill.body }; + const resolved: ResolvedManualSkill = { name: skill.name, body: skill.body }; + if (skill.allowedTools !== undefined) { + resolved.allowedTools = skill.allowedTools; + } + return resolved; } catch (err) { logger.warn( `[resolveManualSkills] Failed to resolve skill "${name}":`,