From 8cd7f9ad4e553367c5e49a67daafbf7458c61694 Mon Sep 17 00:00:00 2001 From: Kyle Carberry Date: Mon, 24 Aug 2026 15:36:49 -0700 Subject: [PATCH] fix(agent/agentcontext): preserve skill source precedence (#28443) Duplicate workspace skills were sorted by source path before chat hydration, so alphabetical paths could override the order configured in `CODER_AGENT_EXP_SKILLS_DIRS`. Resolve duplicate valid skill names before deterministic sorting and persistence, with user-declared sources evaluated in configured order ahead of built-in and working-directory discovery. This PR was generated by Coder Agents. --- agent/agentcontext/defaults.go | 4 +- agent/agentcontext/manager.go | 24 ++++---- agent/agentcontext/manager_test.go | 24 ++++++++ agent/agentcontext/resolve.go | 49 +++++++++++++--- agent/agentcontext/resolve_test.go | 92 ++++++++++++++++++++++++++++++ 5 files changed, 171 insertions(+), 22 deletions(-) diff --git a/agent/agentcontext/defaults.go b/agent/agentcontext/defaults.go index 272fcc1030d..bd383967901 100644 --- a/agent/agentcontext/defaults.go +++ b/agent/agentcontext/defaults.go @@ -1,7 +1,7 @@ package agentcontext -// defaultBuiltinRoots returns the scan roots layered in front -// of any user-added sources. These mirror the paths the legacy +// defaultBuiltinRoots returns the scan roots layered after +// user-added sources and before working-directory discovery. These mirror the // agentcontextconfig API resolves at every chat hydrate. The // list is intentionally tolerant of missing entries; the // resolver silently skips canonicalization failures and diff --git a/agent/agentcontext/manager.go b/agent/agentcontext/manager.go index 45d40d4acf6..0985a33412f 100644 --- a/agent/agentcontext/manager.go +++ b/agent/agentcontext/manager.go @@ -568,10 +568,22 @@ func (m *Manager) SetReady() { } // scanRootsLocked returns the list of ScanRoots to feed the -// resolver and watcher. The Manager's mutex must be held. +// resolver and watcher. Roots are ordered by descending skill priority so +// duplicate skill names honor user-declared source order before built-in and +// working-directory discovery. The Manager's mutex must be held. func (m *Manager) scanRootsLocked() []ScanRoot { builtinRoots := defaultBuiltinRoots() out := make([]ScanRoot, 0, 1+len(builtinRoots)+len(m.sources)) + for _, s := range m.sources { + out = append(out, ScanRoot{Path: s.Path, UserSource: s.Path}) + } + for _, r := range builtinRoots { + canonical, err := CanonicalizePath(r) + if err != nil { + continue + } + out = append(out, ScanRoot{Path: canonical}) + } if m.workingDir != nil { if wd := strings.TrimSpace(m.workingDir()); wd != "" { // The working directory is a single scan root. The @@ -584,16 +596,6 @@ func (m *Manager) scanRootsLocked() []ScanRoot { out = append(out, ScanRoot{Path: wd}) } } - for _, r := range builtinRoots { - canonical, err := CanonicalizePath(r) - if err != nil { - continue - } - out = append(out, ScanRoot{Path: canonical}) - } - for _, s := range m.sources { - out = append(out, ScanRoot{Path: s.Path, UserSource: s.Path}) - } return out } diff --git a/agent/agentcontext/manager_test.go b/agent/agentcontext/manager_test.go index 69eef5b2d02..3ee7849c6e3 100644 --- a/agent/agentcontext/manager_test.go +++ b/agent/agentcontext/manager_test.go @@ -349,6 +349,30 @@ func TestManager_InitialSourcesSeeded(t *testing.T) { require.Equal(t, src, snap.Resources[0].SourcePath) } +func TestManager_InitialSkillSourcesPreservePrecedence(t *testing.T) { + t.Parallel() + wd := t.TempDir() + firstRoot := filepath.Join(t.TempDir(), "b", "skills") + secondRoot := filepath.Join(t.TempDir(), "a", "skills") + mustWriteSkill(t, firstRoot, "make-coffee", "first root") + mustWriteSkill(t, secondRoot, "make-coffee", "second root") + mustWriteSkill(t, filepath.Join(wd, ".agents", "skills"), "make-coffee", "working directory") + + m := newTestManager(t, agentcontext.ManagerOptions{ + WorkingDir: func() string { return wd }, + AllowedRoots: []string{wd, firstRoot, secondRoot}, + InitialSources: []agentcontext.Source{ + {Path: firstRoot}, + {Path: secondRoot}, + }, + }) + + snap := m.Snapshot() + require.Len(t, snap.Resources, 1) + require.Equal(t, filepath.Join(firstRoot, "make-coffee"), snap.Resources[0].Source) + require.Equal(t, "first root", snap.Resources[0].Description) +} + // TestManager_SeedSourcesLateBindsAfterManifest models the // agent's behavior when CODER_AGENT_EXP_*_DIRS contains a // relative path that cannot resolve until the manifest's diff --git a/agent/agentcontext/resolve.go b/agent/agentcontext/resolve.go index 92e713783b9..3d8b3353764 100644 --- a/agent/agentcontext/resolve.go +++ b/agent/agentcontext/resolve.go @@ -159,6 +159,7 @@ func (r *Resolver) ResolveContext(ctx context.Context, roots []ScanRoot) Snapsho if err := ctx.Err(); err != nil { return Snapshot{SnapshotError: err.Error()} } + resources = deduplicateSkills(resources) resources, totalBytes := res.applyCaps(resources) // Append MCP server resources after the filesystem caps @@ -249,7 +250,7 @@ func (r *Resolver) walk(ctx context.Context, roots []ScanRoot) (resources []Reso // resolve to the same file (e.g. overlapping ancestors, or a // built-in root nested under a project root) do not // double-count it. - seenID := make(map[string]struct{}) + seenID := make(map[string]int) for _, root := range dedup { if err := ctx.Err(); err != nil { @@ -257,14 +258,35 @@ func (r *Resolver) walk(ctx context.Context, roots []ScanRoot) (resources []Reso } r.discoverIn(root, &resources, seenID) } + resources = slices.DeleteFunc(resources, func(resource Resource) bool { + return resource.ID == "" + }) return resources, snapErrs } +// deduplicateSkills keeps the first valid skill with each name. walk returns +// resources in scan-root order, so this selects the winner before deterministic +// sorting and persistence discard root ordering. +func deduplicateSkills(resources []Resource) []Resource { + seen := make(map[string]struct{}) + out := resources[:0] + for _, resource := range resources { + if resource.Kind == KindSkill && resource.Status == StatusOK { + if _, ok := seen[resource.Name]; ok { + continue + } + seen[resource.Name] = struct{}{} + } + out = append(out, resource) + } + return out +} + // discoverIn inspects a single scan root. A root that points at a // file is classified directly. A directory root contributes its // top-level instruction files and .mcp.json plus skills from the // fixed container locations under it. The walk goes no deeper. -func (r *Resolver) discoverIn(root ScanRoot, out *[]Resource, seenID map[string]struct{}) { +func (r *Resolver) discoverIn(root ScanRoot, out *[]Resource, seenID map[string]int) { info, err := os.Stat(root.Path) if err != nil { // Missing roots silently fall through. The user either @@ -288,7 +310,7 @@ func (r *Resolver) discoverIn(root ScanRoot, out *[]Resource, seenID map[string] // .mcp.json that sit directly in root.Path. Nested files are // ignored: instruction files and MCP configs are recognized only // at a scan root's top level. -func (r *Resolver) discoverTopLevelFiles(root ScanRoot, out *[]Resource, seenID map[string]struct{}) { +func (r *Resolver) discoverTopLevelFiles(root ScanRoot, out *[]Resource, seenID map[string]int) { entries, err := os.ReadDir(root.Path) if err != nil { return @@ -320,13 +342,22 @@ func (r *Resolver) discoverTopLevelFiles(root ScanRoot, out *[]Resource, seenID } } -// appendResource adds res to out unless an earlier resource -// already claimed its ID. -func appendResource(out *[]Resource, seenID map[string]struct{}, res Resource) { - if _, dup := seenID[res.ID]; dup { +// appendResource adds res to out unless an earlier resource already claimed +// its ID. A valid occurrence replaces an earlier invalid one because the same +// path can be outside a narrow scan root but valid inside a later broader root. +// Replacements tombstone the earlier occurrence so valid discovery order is +// preserved without repeatedly shifting the resource slice. +func appendResource(out *[]Resource, seenID map[string]int, res Resource) { + if previous, dup := seenID[res.ID]; dup { + if (*out)[previous].Status == StatusOK || res.Status != StatusOK { + return + } + (*out)[previous] = Resource{} + seenID[res.ID] = len(*out) + *out = append(*out, res) return } - seenID[res.ID] = struct{}{} + seenID[res.ID] = len(*out) *out = append(*out, res) } @@ -614,7 +645,7 @@ func (r *Resolver) readSkillMeta(scanRoot, path string, info fs.FileInfo, userSo // emitSkillsFromContainer scans the immediate children of a // recognized skills-container directory and emits one Skill // resource per subdirectory whose SKILL.md parses cleanly. -func (r *Resolver) emitSkillsFromContainer(container string, root ScanRoot, out *[]Resource, seenID map[string]struct{}) { +func (r *Resolver) emitSkillsFromContainer(container string, root ScanRoot, out *[]Resource, seenID map[string]int) { entries, err := os.ReadDir(container) if err != nil { return diff --git a/agent/agentcontext/resolve_test.go b/agent/agentcontext/resolve_test.go index a8ea63e951f..57705c3b87d 100644 --- a/agent/agentcontext/resolve_test.go +++ b/agent/agentcontext/resolve_test.go @@ -91,6 +91,98 @@ func TestResolver_SkillsContainerEmitsEachSubdir(t *testing.T) { }, kinds) } +func TestResolver_DuplicateSkillNamesFirstRootWins(t *testing.T) { + t.Parallel() + dir := t.TempDir() + firstRoot := filepath.Join(dir, "b", "skills") + secondRoot := filepath.Join(dir, "a", "skills") + mustWriteSkill(t, firstRoot, "make-coffee", "first root") + mustWriteSkill(t, secondRoot, "make-coffee", "second root") + + r := &agentcontext.Resolver{} + snap := r.Resolve([]agentcontext.ScanRoot{ + {Path: firstRoot}, + {Path: secondRoot}, + }) + + require.Len(t, snap.Resources, 1) + require.Equal(t, filepath.Join(firstRoot, "make-coffee"), snap.Resources[0].Source) + require.Equal(t, "first root", snap.Resources[0].Description) +} + +func TestResolver_DuplicateSkillNamesFirstValidRootWins(t *testing.T) { + t.Parallel() + dir := t.TempDir() + firstRoot := filepath.Join(dir, "b", "skills") + secondRoot := filepath.Join(dir, "a", "skills") + mustWriteFile(t, filepath.Join(firstRoot, "make-coffee", "SKILL.md"), + "---\nname: drink-tea\ndescription: invalid\n---\nBody") + mustWriteSkill(t, secondRoot, "make-coffee", "second root") + + r := &agentcontext.Resolver{} + snap := r.Resolve([]agentcontext.ScanRoot{ + {Path: firstRoot}, + {Path: secondRoot}, + }) + + require.Len(t, snap.Resources, 2) + winner := findResource(t, snap.Resources, agentcontext.KindSkill, filepath.Join(secondRoot, "make-coffee")) + require.Equal(t, agentcontext.StatusOK, winner.Status) + require.Equal(t, "second root", winner.Description) +} + +func TestResolver_DuplicateSkillIDPrefersValidOccurrence(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("symlink creation requires elevated privileges on Windows") + } + wd := t.TempDir() + skillDir := filepath.Join(wd, ".agents", "skills", "make-coffee") + target := filepath.Join(wd, "shared", "SKILL.md") + mustWriteFile(t, target, + "---\nname: make-coffee\ndescription: valid from broad root\n---\nBody") + require.NoError(t, os.MkdirAll(skillDir, 0o755)) + require.NoError(t, os.Symlink(target, filepath.Join(skillDir, "SKILL.md"))) + + r := &agentcontext.Resolver{} + snap := r.Resolve([]agentcontext.ScanRoot{ + {Path: filepath.Join(wd, ".agents", "skills")}, + {Path: wd}, + }) + + require.Len(t, snap.Resources, 1) + require.Equal(t, agentcontext.StatusOK, snap.Resources[0].Status) + require.Equal(t, "valid from broad root", snap.Resources[0].Description) +} + +func TestResolver_DuplicateSkillIDUsesValidDiscoveryOrder(t *testing.T) { + t.Parallel() + if runtime.GOOS == "windows" { + t.Skip("symlink creation requires elevated privileges on Windows") + } + wd := t.TempDir() + firstSkillDir := filepath.Join(wd, ".agents", "skills", "make-coffee") + target := filepath.Join(wd, "shared", "SKILL.md") + mustWriteFile(t, target, + "---\nname: make-coffee\ndescription: valid from broad root\n---\nBody") + require.NoError(t, os.MkdirAll(firstSkillDir, 0o755)) + require.NoError(t, os.Symlink(target, filepath.Join(firstSkillDir, "SKILL.md"))) + + secondRoot := filepath.Join(t.TempDir(), "skills") + mustWriteSkill(t, secondRoot, "make-coffee", "first valid root") + + r := &agentcontext.Resolver{} + snap := r.Resolve([]agentcontext.ScanRoot{ + {Path: filepath.Join(wd, ".agents", "skills")}, + {Path: secondRoot}, + {Path: wd}, + }) + + require.Len(t, snap.Resources, 1) + require.Equal(t, filepath.Join(secondRoot, "make-coffee"), snap.Resources[0].Source) + require.Equal(t, "first valid root", snap.Resources[0].Description) +} + func TestResolver_SkillNameMismatchInvalid(t *testing.T) { t.Parallel() dir := t.TempDir()