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

@@ -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

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
 }
 

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

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

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()