From f08d4ecdda0b5ceb0f8f4faf92e99ec7ded9f296 Mon Sep 17 00:00:00 2001 From: itsmylife44 <34112129+itsmylife44@users.noreply.github.com> Date: Wed, 4 Feb 2026 17:47:08 +0100 Subject: [PATCH] refactor(agents): extract formatCustomSkillsBlock to eliminate duplication Address review feedback (P3): The User-Installed Skills block was duplicated verbatim in two if/else branches in both buildCategorySkillsDelegationGuide() and Atlas buildSkillsSection(). Extract shared formatCustomSkillsBlock() with configurable header level (#### vs **) so both builders reference a single source of truth. Ultraworked with [Sisyphus](https://github.com/code-yeongyu/oh-my-opencode) Co-authored-by: Sisyphus --- src/agents/atlas/utils.ts | 26 ++------- .../dynamic-agent-prompt-builder.test.ts | 44 +++++++++++++++ src/agents/dynamic-agent-prompt-builder.ts | 53 +++++++++++-------- 3 files changed, 78 insertions(+), 45 deletions(-) diff --git a/src/agents/atlas/utils.ts b/src/agents/atlas/utils.ts index 9aa035137..f912a0370 100644 --- a/src/agents/atlas/utils.ts +++ b/src/agents/atlas/utils.ts @@ -6,7 +6,7 @@ */ import type { CategoryConfig } from "../../config/schema" -import type { AvailableAgent, AvailableSkill } from "../dynamic-agent-prompt-builder" +import { formatCustomSkillsBlock, type AvailableAgent, type AvailableSkill } from "../dynamic-agent-prompt-builder" import { DEFAULT_CATEGORIES, CATEGORY_DESCRIPTIONS } from "../../tools/delegate-task/constants" export const getCategoryDescription = (name: string, userCategories?: Record) => @@ -70,7 +70,7 @@ export function buildSkillsSection(skills: AvailableSkill[]): string { return `| \`${s.name}\` | ${shortDesc} | ${source} |` }) - const customSkillNames = customSkills.map((s) => `"${s.name}"`).join(", ") + const customSkillBlock = formatCustomSkillsBlock(customRows, customSkills, "**") let skillsTable: string @@ -81,27 +81,9 @@ export function buildSkillsSection(skills: AvailableSkill[]): string { |-------|-------------| ${builtinRows.join("\n")} -**User-Installed Skills (HIGH PRIORITY):** - -The user installed these for their workflow. They MUST be evaluated for EVERY delegation. - -| Skill | When to Use | Source | -|-------|-------------|--------| -${customRows.join("\n")} - -> **CRITICAL**: The user installed ${customSkillNames} for a reason — USE THEM when the task overlaps with their domain. -> When in doubt, INCLUDE a user-installed skill rather than omit it.` +${customSkillBlock}` } else if (customSkills.length > 0) { - skillsTable = `**User-Installed Skills (HIGH PRIORITY):** - -The user installed these for their workflow. They MUST be evaluated for EVERY delegation. - -| Skill | When to Use | Source | -|-------|-------------|--------| -${customRows.join("\n")} - -> **CRITICAL**: The user installed ${customSkillNames} for a reason — USE THEM when the task overlaps with their domain. -> When in doubt, INCLUDE a user-installed skill rather than omit it.` + skillsTable = customSkillBlock } else { skillsTable = `| Skill | When to Use | |-------|-------------| diff --git a/src/agents/dynamic-agent-prompt-builder.test.ts b/src/agents/dynamic-agent-prompt-builder.test.ts index feb613ec0..7d9ff8af3 100644 --- a/src/agents/dynamic-agent-prompt-builder.test.ts +++ b/src/agents/dynamic-agent-prompt-builder.test.ts @@ -4,6 +4,7 @@ import { describe, it, expect } from "bun:test" import { buildCategorySkillsDelegationGuide, buildUltraworkSection, + formatCustomSkillsBlock, type AvailableSkill, type AvailableCategory, type AvailableAgent, @@ -159,3 +160,46 @@ describe("buildUltraworkSection", () => { expect(result).not.toContain("User-Installed Skills") }) }) + +describe("formatCustomSkillsBlock", () => { + const customSkills: AvailableSkill[] = [ + { name: "react-19", description: "React 19 patterns", location: "user" }, + { name: "tailwind-4", description: "Tailwind v4", location: "project" }, + ] + + const customRows = customSkills.map((s) => { + const source = s.location === "project" ? "project" : "user" + return `| \`${s.name}\` | ${s.description} | ${source} |` + }) + + it("should produce consistent output used by both builders", () => { + //#given: custom skills and rows + //#when: formatting with default header level + const result = formatCustomSkillsBlock(customRows, customSkills) + + //#then: contains all expected elements + expect(result).toContain("User-Installed Skills (HIGH PRIORITY)") + expect(result).toContain("CRITICAL") + expect(result).toContain('"react-19"') + expect(result).toContain('"tailwind-4"') + expect(result).toContain("| user |") + expect(result).toContain("| project |") + }) + + it("should use #### header by default", () => { + //#given: default header level + const result = formatCustomSkillsBlock(customRows, customSkills) + + //#then: uses markdown h4 + expect(result).toContain("#### User-Installed Skills") + }) + + it("should use bold header when specified", () => { + //#given: bold header level (used by Atlas) + const result = formatCustomSkillsBlock(customRows, customSkills, "**") + + //#then: uses bold instead of h4 + expect(result).toContain("**User-Installed Skills (HIGH PRIORITY):**") + expect(result).not.toContain("#### User-Installed Skills") + }) +}) diff --git a/src/agents/dynamic-agent-prompt-builder.ts b/src/agents/dynamic-agent-prompt-builder.ts index 3a5e61304..64156bd6b 100644 --- a/src/agents/dynamic-agent-prompt-builder.ts +++ b/src/agents/dynamic-agent-prompt-builder.ts @@ -166,6 +166,33 @@ export function buildDelegationTable(agents: AvailableAgent[]): string { return rows.join("\n") } +/** + * Renders the "User-Installed Skills (HIGH PRIORITY)" block used across multiple agent prompts. + * Extracted to avoid duplication between buildCategorySkillsDelegationGuide, buildSkillsSection, etc. + */ +export function formatCustomSkillsBlock( + customRows: string[], + customSkills: AvailableSkill[], + headerLevel: "####" | "**" = "####" +): string { + const customSkillNames = customSkills.map((s) => `"${s.name}"`).join(", ") + const header = headerLevel === "####" + ? `#### User-Installed Skills (HIGH PRIORITY)` + : `**User-Installed Skills (HIGH PRIORITY):**` + + return `${header} + +**The user has installed these custom skills. They MUST be evaluated for EVERY delegation.** +Subagents are STATELESS — they lose all custom knowledge unless you pass these skills via \`load_skills\`. + +| Skill | Expertise Domain | Source | +|-------|------------------|--------| +${customRows.join("\n")} + +> **CRITICAL**: Ignoring user-installed skills when they match the task domain is a failure. +> The user installed ${customSkillNames} for a reason — USE THEM when the task overlaps with their domain.` +} + export function buildCategorySkillsDelegationGuide(categories: AvailableCategory[], skills: AvailableSkill[]): string { if (categories.length === 0 && skills.length === 0) return "" @@ -188,7 +215,7 @@ export function buildCategorySkillsDelegationGuide(categories: AvailableCategory return `| \`${s.name}\` | ${desc} | ${source} |` }) - const customSkillNames = customSkills.map((s) => `"${s.name}"`).join(", ") + const customSkillBlock = formatCustomSkillsBlock(customRows, customSkills) let skillsSection: string @@ -199,29 +226,9 @@ export function buildCategorySkillsDelegationGuide(categories: AvailableCategory |-------|------------------| ${builtinRows.join("\n")} -#### User-Installed Skills (HIGH PRIORITY) - -**The user has installed these custom skills. They MUST be evaluated for EVERY delegation.** -Subagents are STATELESS — they lose all custom knowledge unless you pass these skills via \`load_skills\`. - -| Skill | Expertise Domain | Source | -|-------|------------------|--------| -${customRows.join("\n")} - -> **CRITICAL**: Ignoring user-installed skills when they match the task domain is a failure. -> The user installed ${customSkillNames} for a reason — USE THEM when the task overlaps with their domain.` +${customSkillBlock}` } else if (customSkills.length > 0) { - skillsSection = `#### User-Installed Skills (HIGH PRIORITY) - -**The user has installed these custom skills. They MUST be evaluated for EVERY delegation.** -Subagents are STATELESS — they lose all custom knowledge unless you pass these skills via \`load_skills\`. - -| Skill | Expertise Domain | Source | -|-------|------------------|--------| -${customRows.join("\n")} - -> **CRITICAL**: Ignoring user-installed skills when they match the task domain is a failure. -> The user installed ${customSkillNames} for a reason — USE THEM when the task overlaps with their domain.` + skillsSection = customSkillBlock } else { skillsSection = `#### Available Skills (Domain Expertise Injection)