fix: address review findings - git-master identity check, test fixtures, regression strength
This commit is contained in:
@@ -221,10 +221,10 @@ describe("resolveSkillContentAsync", () => {
|
||||
it("prefers exact match over short name match async", async () => {
|
||||
// given: an exact skill name "debugging" and a nested "superpowers/debugging"
|
||||
createNestedSkill(testConfigDir, "superpowers", "debugging", "nested debugging")
|
||||
// Also create the exact match by placing it at dir root
|
||||
const dir = join(testConfigDir, "skills")
|
||||
mkdirSync(dir, { recursive: true })
|
||||
writeFileSync(join(dir, "debugging.md"), "---\nname: debugging\ndescription: exact debugging\n---\nexact match content")
|
||||
// Exact match as a non-namespaced dir with SKILL.md
|
||||
const exactDir = join(testConfigDir, "skills", "debugging")
|
||||
mkdirSync(exactDir, { recursive: true })
|
||||
writeFileSync(join(exactDir, "SKILL.md"), "---\nname: debugging\ndescription: exact debugging\n---\nexact match content")
|
||||
|
||||
// when: resolving by name "debugging"
|
||||
const result = await resolveSkillContentAsync("debugging")
|
||||
@@ -468,9 +468,9 @@ describe("resolveMultipleSkillsAsync", () => {
|
||||
|
||||
it("prefers exact match over short name in batch", async () => {
|
||||
// given: an exact skill and a nested skill with same base name
|
||||
const dir = join(testConfigDir, "skills")
|
||||
mkdirSync(dir, { recursive: true })
|
||||
writeFileSync(join(dir, "debugging.md"), "---\nname: debugging\ndescription: exact debugging\n---\nexact match content")
|
||||
const exactDir = join(testConfigDir, "skills", "debugging")
|
||||
mkdirSync(exactDir, { recursive: true })
|
||||
writeFileSync(join(exactDir, "SKILL.md"), "---\nname: debugging\ndescription: exact debugging\n---\nexact match content")
|
||||
createNestedSkill(testConfigDir, "superpowers", "debugging", "nested content")
|
||||
|
||||
// when: resolving "debugging" in batch
|
||||
|
||||
@@ -14,7 +14,7 @@ export function resolveSkillContent(skillName: string, options?: SkillResolution
|
||||
const skill = skills.find((builtinSkill) => builtinSkill.name === skillName)
|
||||
if (!skill) return null
|
||||
|
||||
if (skillName === "git-master") {
|
||||
if (skill.name === "git-master") {
|
||||
return injectGitMasterConfig(skill.template, options?.gitMasterConfig)
|
||||
}
|
||||
|
||||
@@ -30,18 +30,18 @@ export function resolveMultipleSkills(
|
||||
disabledSkills: options?.disabledSkills,
|
||||
teamModeEnabled: options?.teamModeEnabled,
|
||||
})
|
||||
const skillMap = new Map(skills.map((skill) => [skill.name, skill.template]))
|
||||
const skillMap = new Map(skills.map((skill) => [skill.name, skill]))
|
||||
|
||||
const resolved = new Map<string, string>()
|
||||
const notFound: string[] = []
|
||||
|
||||
for (const name of skillNames) {
|
||||
const template = skillMap.get(name)
|
||||
if (template) {
|
||||
if (name === "git-master") {
|
||||
resolved.set(name, injectGitMasterConfig(template, options?.gitMasterConfig))
|
||||
const match = skillMap.get(name)
|
||||
if (match) {
|
||||
if (match.name === "git-master") {
|
||||
resolved.set(name, injectGitMasterConfig(match.template, options?.gitMasterConfig))
|
||||
} else {
|
||||
resolved.set(name, template)
|
||||
resolved.set(name, match.template)
|
||||
}
|
||||
} else {
|
||||
notFound.push(name)
|
||||
@@ -61,7 +61,7 @@ export async function resolveSkillContentAsync(
|
||||
|
||||
const template = await extractSkillTemplate(skill)
|
||||
|
||||
if (skillName === "git-master") {
|
||||
if (skill.name === "git-master") {
|
||||
return injectGitMasterConfig(template, options?.gitMasterConfig)
|
||||
}
|
||||
|
||||
@@ -81,7 +81,7 @@ export async function resolveMultipleSkillsAsync(
|
||||
const skill = matchSkillByName(allSkills, name)
|
||||
if (skill) {
|
||||
const template = await extractSkillTemplate(skill)
|
||||
if (name === "git-master") {
|
||||
if (skill.name === "git-master") {
|
||||
resolved.set(name, injectGitMasterConfig(template, options?.gitMasterConfig))
|
||||
} else {
|
||||
resolved.set(name, template)
|
||||
|
||||
@@ -3228,8 +3228,10 @@ describe("sisyphus-task", () => {
|
||||
}
|
||||
})
|
||||
|
||||
test("resolves short named discovered skill without reporting not found", async () => {
|
||||
test("resolves short named discovered skill and flows content into prompt", async () => {
|
||||
// given: a nested discovered skill under a temp config dir
|
||||
// (intentionally verifies the full integration path: delegate-task -> skill-resolver ->
|
||||
// resolveMultipleSkillsAsync -> matchSkillByName, not just unit-testing the resolver)
|
||||
const { join } = require("node:path")
|
||||
const { tmpdir } = require("node:os")
|
||||
const { mkdirSync, writeFileSync } = require("node:fs")
|
||||
@@ -3247,14 +3249,21 @@ describe("sisyphus-task", () => {
|
||||
|
||||
const { createDelegateTask } = require("./tools")
|
||||
const mockManager = { launch: async () => ({}) }
|
||||
|
||||
let promptBody: any
|
||||
const promptMock = async (input: any) => {
|
||||
promptBody = input.body
|
||||
return { data: {} }
|
||||
}
|
||||
|
||||
const mockClient = {
|
||||
app: { agents: async () => ({ data: [] }) },
|
||||
config: { get: async () => ({ data: { model: SYSTEM_DEFAULT_MODEL } }) },
|
||||
session: {
|
||||
get: async () => ({ data: { directory: "/project" } }),
|
||||
create: async () => ({ data: { id: "ses_shortname_test" } }),
|
||||
prompt: async () => ({ data: {} }),
|
||||
promptAsync: async () => ({ data: {} }),
|
||||
prompt: promptMock,
|
||||
promptAsync: promptMock,
|
||||
messages: async () => ({
|
||||
data: [{ info: { role: "assistant" }, parts: [{ type: "text", text: "Done" }] }],
|
||||
}),
|
||||
@@ -3286,8 +3295,11 @@ describe("sisyphus-task", () => {
|
||||
toolContext
|
||||
)
|
||||
|
||||
// then: should NOT report "Skills not found"
|
||||
// then: must NOT return "Skills not found" (failing means short name wasn't resolved)
|
||||
expect(result).not.toContain("Skills not found")
|
||||
// and the resolved skill content must have been injected into the prompt body
|
||||
expect(promptBody).toBeDefined()
|
||||
expect(promptBody.system).toContain("Debug instructions")
|
||||
})
|
||||
})
|
||||
|
||||
|
||||
Reference in New Issue
Block a user