fix(#5408): deduplicate skills by basename during base composition

docs/ADRs/0045-forge-portable-harness-schema.md

@@ -187,7 +187,7 @@ resolved as follows:
 | Field type       | Merge behavior                                       | Nil vs empty                                          |
 |------------------|------------------------------------------------------|-------------------------------------------------------|
 | Scalar fields    | Forge value overrides top-level value                | Absent = inherit from top level                       |
-| `skills`         | Top-level list + forge-specific list (concatenated)  | Absent (nil) = inherit; `skills: []` = no forge-specific additions (top-level skills still apply) |
+| `skills`         | Merged with deduplication by basename (forge overrides top-level) | Absent (nil) = inherit; `skills: []` = no forge-specific additions (top-level skills still apply) |
 | `runner_env`     | Top-level map merged with forge map; forge keys win  | Absent (nil) = inherit; `runner_env: {}` = no forge-specific keys (top-level env still inherited) |
 | `validation_loop`| Forge value replaces top-level value entirely        | Absent (nil) = inherit from top level; explicit empty struct = intended to mean "no validation" but requires implementation changes (see note¹) |
 
@@ -359,7 +359,7 @@ itself is consumed during loading and is not present on the merged harness.
 The same inheritance table applies to base→child merging:
 
 - **Scalar fields** (agent, model, image, pre_script, etc.): child overrides base
-- **`skills`**: base list + child list (concatenated)
+- **`skills`**: merged with deduplication by basename (child overrides base)
 - **`runner_env`**: base map merged with child map; child keys win
 - **`validation_loop`**: child replaces base entirely (if non-nil)
 - **`host_files`**: concatenated (base + child); if both declare the same
@@ -609,7 +609,7 @@ forge-specific artifact. The harness and agent definition are portable.
   both are written atomically during `fullsend install`.
 
 - **Merge semantics add complexity.** The inheritance rules (scalars
-  override, skills concatenate, runner_env merges, validation_loop replaces)
+  override, skills merge with deduplication by basename, runner_env merges, validation_loop replaces)
   must be well-documented and tested. Edge cases — such as a forge block
   wanting to *remove* an inherited skill or runner_env key — are not
   supported by this design. If needed, a future extension could add explicit
@@ -657,7 +657,7 @@ forge-specific artifact. The harness and agent definition are portable.
   field type, matching the inheritance rules in the table above:
   - `skills`: nil = inherit top-level list; `skills: []` = no
     forge-specific additions (top-level skills still apply, since skills
-    uses concatenation semantics).
+    uses merge-with-deduplication-by-basename semantics).
   - `runner_env`: nil = inherit top-level map; `runner_env: {}` = no
     forge-specific keys (top-level env still inherited, since runner_env
     uses merge semantics).

docs/ADRs/0064-deprecate-customized-directory-overlay.md

@@ -37,7 +37,9 @@ customization scenario the overlay handled, with better ergonomics:
 - [ADR 0045](0045-forge-portable-harness-schema.md) added `base:`
   composition for harness files. A thin wrapper inherits an upstream harness
   by URL and overrides only the fields that differ, with proper merge
-  semantics (scalars override, skills concatenate, runner_env merges).
+  semantics (scalars override, skills merge with deduplication by basename
+  per [#5408](https://github.com/fullsend-ai/fullsend/pull/5408), runner_env
+  merges).
 
 - [ADR 0038](0038-universal-harness-access.md) added URL-based references
   for declarative resources (agents, skills, policies, schemas). Resources
@@ -55,7 +57,7 @@ Together these make the `customized/` directory overlay redundant:
 | Override a harness | `base:` composition (ADR 0045) |
 | Override an agent definition | Harness `agent:` field with path or URL (ADR 0038) |
 | Add/remove agents | `agents:` list in config (ADR 0058) |
-| Add custom skills | Harness `skills:` list with paths or URLs (ADR 0038); concatenated via `base:` (ADR 0045) |
+| Add custom skills | Harness `skills:` list with paths or URLs (ADR 0038); merged with deduplication by basename via `base:` (ADR 0045, [#5408](https://github.com/fullsend-ai/fullsend/pull/5408)) |
 | Override policies/schemas | Harness fields with paths or URLs (ADR 0038) |
 | Custom scripts | `pre_script`/`post_script` in harness; inherited from `base:` (ADR 0045) |
 | Custom env vars | `env:` in harness; merged via `base:` (ADR 0045) |

docs/ADRs/0070-portable-provider-profile-resolution.md

@@ -88,7 +88,7 @@ When a harness declares `base:`, the base YAML is fetched and parsed. The
 - Base entries come first, child entries append
 - Deduplication by profile `id` (from profile YAML) / provider `name` (from provider YAML)
 - Child wins in dedup conflicts
-- Same merge pattern as `skills` in ADR 0045
+- Concatenation with last-writer-wins dedup (note: skills in ADR 0045 now use basename-aware dedup per [#5408](https://github.com/fullsend-ai/fullsend/pull/5408); profiles and providers use simpler id/name dedup at resolution time)
 
 **Phase 2 — Resource resolution (`resolve.go`)**
 

docs/guides/dev/cli-internals.md

@@ -167,7 +167,7 @@ Migration actions per agent:
 | Custom | Not in upstream scaffold | Move files, register local path in config |
 | Modified | Standard scaffold agent, not in config | Compute `base:` composition harness via `DiffHarness`, register in config |
 
-The diff engine (`internal/harness/diff.go`) computes the minimal child harness that reproduces the customized version when composed with the upstream base. It mirrors `mergeBaseIntoChild` semantics: scalar overrides, slice concatenation extras, map merge deltas, and security fields always included.
+The diff engine (`internal/harness/diff.go`) computes the minimal child harness that reproduces the customized version when composed with the upstream base. It mirrors `mergeBaseIntoChild` semantics: scalar overrides, basename-aware skill overrides (matching `mergeSkills`), slice concatenation extras for plugins/providers, map merge deltas, and security fields always included.
 
 ### Command Decomposition
 

docs/guides/user/bring-your-own-agent.md

@@ -275,7 +275,8 @@ security:
 | Field type | Behavior |
 |-----------|----------|
 | Scalars (`model`, `pre_script`, `image`, etc.) | Child wins if non-empty |
-| `skills`, `plugins`, `providers`, `api_servers`, `openshell.profiles` | Concatenated (base + child) |
+| `skills` | Merged with deduplication by basename (child overrides base) |
+| `plugins`, `providers`, `api_servers`, `openshell.profiles` | Concatenated (base + child) |
 | `host_files` | Concatenated; child overrides by `dest` |
 | `env`, `runner_env` | Merged; child keys win |
 | `validation_loop`, `security` | Child replaces entirely |
@@ -366,7 +367,7 @@ Create a thin harness that inherits from the upstream code agent and adds your s
 base: https://raw.githubusercontent.com/fullsend-ai/fullsend/<sha>/internal/scaffold/fullsend-repo/harness/code.yaml#sha256=abc...
 
 skills:
-  - skills/my-custom-linting        # Concatenated with base skills
+  - skills/my-custom-linting        # Merged with base skills (child overrides by basename)
 
 timeout_minutes: 45                 # Override timeout (scalar → child wins)
 ```
@@ -420,7 +421,7 @@ env:
 Any harness field can be overridden. The [field merge rules](#field-merge-rules-for-base-and-forge) determine how your overrides combine with the base:
 
 - **Change model, timeout, image, scripts** — scalars replace the base value.
-- **Add skills, plugins, or host_files** — your entries are concatenated with the base's.
+- **Add skills** — your entries are merged with the base's by basename; same-named skills override the base entry. **Add plugins or host_files** — your entries are concatenated with the base's.
 - **Add or override env vars** — maps are merged; your keys win on collision.
 - **Replace validation or security config** — child replaces the entire block.
 

docs/plans/adr-0045-forge-portable-harness-phase1.md

@@ -49,7 +49,7 @@ PRs 1, 2, 7 can start in parallel. PR 4 depends on PRs 1, 2, and 3 (`loadRaw` fo
 - `validForgeKeys = map[string]bool{"github": true, "gitlab": true}`
 - `(h *Harness) ResolveForge(platform string) error` — merges forge overrides into harness in place per ADR rules:
   - Scalars: forge overrides if non-empty
-  - Skills: top-level + forge (concatenated)
+  - Skills: merged with deduplication by basename (forge overrides top-level)
   - RunnerEnv: top-level + forge map, forge wins on key conflict
   - ValidationLoop: forge replaces entirely if non-nil
   - Sets `h.Forge = nil` after merge (consumed)
@@ -149,7 +149,8 @@ For **lock file integration**, `base` URLs are recorded as `DependencyEntry` ent
   - After the full base chain is merged, calls `ResolveForge(opts.ForgePlatform)` once on the final merged result, then calls `Validate()`. This matches the ADR's resolution order: `base harness (recursive) → child overrides → ResolveForge(platform)`.
   - `mergeHarness(base, child *Harness)` — same inheritance rules as forge merge:
     - Scalars: child overrides base if non-zero
-    - Skills, Plugins, Providers, APIServers: concatenated (base + child)
+    - Skills: merged with deduplication by basename (child overrides base)
+    - Plugins, Providers, APIServers: concatenated (base + child)
     - RunnerEnv: base map merged with child map, child keys win
     - ValidationLoop, Security: child replaces if non-nil
     - HostFiles: concatenated (base + child order), last-writer-wins dedup by `Dest` (exact string comparison, no path canonicalization) — child entries override base entries with the same `Dest`

docs/plans/adr-0045-forge-portable-harness-phase2.md

@@ -358,7 +358,7 @@ The `HarnessWrappersLayer` maintains this mapping. A helper function `harnessNam
      - `Agent`, `Model`, `Image`, `Policy` inherited from base
      - `PreScript`, `PostScript` populated (from `forge.github:` after merge)
      - `RunnerEnv` contains both top-level keys (e.g., `FULLSEND_OUTPUT_SCHEMA`) and GitHub keys (e.g., `GH_TOKEN`) after forge resolution
-     - `Skills` contains both base skills and forge skills (concatenated)
+     - `Skills` merged from base and forge with deduplication by basename
      - `Forge` is nil (consumed by ResolveForge)
      - `Base` is empty (consumed by LoadWithBase)
 

internal/harness/compose.go

@@ -472,13 +472,12 @@ func mergeBaseIntoChild(base, child *Harness) {
 		child.SandboxTimeoutSeconds = base.SandboxTimeoutSeconds
 	}
 
-	// Concatenated slices: base + child.
-	// Pre-allocate new slices to avoid mutating base's backing array.
-	if base.Skills != nil {
-		merged := make([]string, 0, len(base.Skills)+len(child.Skills))
-		merged = append(merged, base.Skills...)
-		merged = append(merged, child.Skills...)
-		child.Skills = merged
+	// Skills: base + child with child-overrides-base-by-basename.
+	// A child skill whose directory basename matches a base skill replaces
+	// the base entry (same as host_files' override-by-dest). This allows
+	// child harnesses to override built-in skills via base: composition.
+	if base.Skills != nil || child.Skills != nil {
+		child.Skills = mergeSkills(base.Skills, child.Skills)
 	}
 	if base.Plugins != nil {
 		merged := make([]string, 0, len(base.Plugins)+len(child.Plugins))
@@ -1149,6 +1148,42 @@ func urlIndexPut(workspaceRoot, rawURL, hash string) error {
 	return os.WriteFile(idxPath, out, 0o600)
 }
 
+// mergeSkills concatenates base and child skill paths, with child entries
+// overriding base entries that resolve to the same sandbox directory name
+// (filepath.Base). This mirrors mergeHostFiles' override-by-dest behavior
+// and allows a child harness to replace a built-in skill by declaring a
+// same-named skill via base: composition (see #5408).
+//
+// Known limitation: if the base slice itself contains two entries with the
+// same basename (e.g., /cache/a/skill-x and /cache/b/skill-x), the second
+// entry silently overwrites the first in baseIndex. In practice this is
+// benign because duplicateDestinationNameError at bootstrap time catches
+// duplicate basenames within a single harness.
+func mergeSkills(base, child []string) []string {
+	baseIndex := make(map[string]int, len(base))
+	result := make([]string, 0, len(base)+len(child))
+
+	// Add base entries
+	for _, s := range base {
+		name := filepath.Base(s)
+		baseIndex[name] = len(result)
+		result = append(result, s)
+	}
+
+	// Add/override with child entries
+	for _, s := range child {
+		name := filepath.Base(s)
+		if idx, exists := baseIndex[name]; exists {
+			result[idx] = s // child overrides base
+		} else {
+			baseIndex[name] = len(result)
+			result = append(result, s)
+		}
+	}
+
+	return result
+}
+
 // mergeHostFiles concatenates base and child host files, with child entries
 // overriding base entries that have the same Dest path.
 func mergeHostFiles(base, child []HostFile) []HostFile {
@@ -1212,12 +1247,9 @@ func mergeForgeConfigInto(base, child *ForgeConfig) {
 		child.PostScript = base.PostScript
 	}
 
-	// Skills: concatenate (pre-allocate to avoid mutating base's backing array)
-	if base.Skills != nil {
-		merged := make([]string, 0, len(base.Skills)+len(child.Skills))
-		merged = append(merged, base.Skills...)
-		merged = append(merged, child.Skills...)
-		child.Skills = merged
+	// Skills: base + child with child-overrides-base-by-basename
+	if base.Skills != nil || child.Skills != nil {
+		child.Skills = mergeSkills(base.Skills, child.Skills)
 	}
 
 	// RunnerEnv: merge, child keys win

internal/harness/compose_test.go

@@ -106,10 +106,134 @@ skills:
 	h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
 	require.NoError(t, err)
 
-	// Skills concatenated: base + child
+	// Skills concatenated: base + child (no name collision)
 	assert.Equal(t, []string{"skill-a", "skill-b", "skill-c"}, h.Skills)
 }
 
+// TestLoadWithBase_ChildSkillOverridesBaseByBasename verifies that a child
+// skill whose directory basename matches a base skill replaces the base entry
+// instead of producing a duplicate that trips duplicateDestinationNameError
+// at bootstrap time (see #5408).
+func TestLoadWithBase_ChildSkillOverridesBaseByBasename(t *testing.T) {
+	dir := t.TempDir()
+
+	writeTestHarness(t, dir, "base.yaml", `
+agent: agents/test.md
+role: test
+skills:
+  - /cache/sha256/abc123/code-implementation
+  - /cache/sha256/def456/pr-review
+`)
+
+	path := writeTestHarness(t, dir, "child.yaml", `
+base: base.yaml
+skills:
+  - skills/code-implementation
+`)
+
+	h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
+	require.NoError(t, err)
+
+	// Child's code-implementation replaces base's, pr-review stays
+	require.Len(t, h.Skills, 2)
+	assert.Equal(t, "skills/code-implementation", h.Skills[0])
+	assert.Equal(t, "/cache/sha256/def456/pr-review", h.Skills[1])
+}
+
+// TestLoadWithBase_ChildSkillOverride_PreservesOrder verifies that when a
+// child overrides multiple base skills, the merged list preserves base
+// ordering for non-overridden entries and replaces overridden entries
+// in-place.
+func TestLoadWithBase_ChildSkillOverride_PreservesOrder(t *testing.T) {
+	dir := t.TempDir()
+
+	writeTestHarness(t, dir, "base.yaml", `
+agent: agents/test.md
+role: test
+skills:
+  - /cache/skill-a
+  - /cache/skill-b
+  - /cache/skill-c
+`)
+
+	path := writeTestHarness(t, dir, "child.yaml", `
+base: base.yaml
+skills:
+  - local/skill-b
+  - local/skill-d
+`)
+
+	h, _, err := LoadWithBase(context.Background(), path, ComposeOpts{})
+	require.NoError(t, err)
+
+	// skill-b replaced in-place, skill-d appended
+	assert.Equal(t, []string{
+		"/cache/skill-a",
+		"local/skill-b",
+		"/cache/skill-c",
+		"local/skill-d",
+	}, h.Skills)
+}
+
+// TestMergeSkills verifies the mergeSkills helper directly.
+func TestMergeSkills(t *testing.T) {
+	tests := []struct {
+		name  string
+		base  []string
+		child []string
+		want  []string
+	}{
+		{
+			name:  "no overlap appends",
+			base:  []string{"/base/skill-a"},
+			child: []string{"/child/skill-b"},
+			want:  []string{"/base/skill-a", "/child/skill-b"},
+		},
+		{
+			name:  "child overrides base by basename",
+			base:  []string{"/base/skill-a", "/base/skill-b"},
+			child: []string{"/child/skill-a"},
+			want:  []string{"/child/skill-a", "/base/skill-b"},
+		},
+		{
+			name:  "nil base",
+			base:  nil,
+			child: []string{"/child/skill-a"},
+			want:  []string{"/child/skill-a"},
+		},
+		{
+			name:  "nil child",
+			base:  []string{"/base/skill-a"},
+			child: nil,
+			want:  []string{"/base/skill-a"},
+		},
+		{
+			name:  "both nil",
+			base:  nil,
+			child: nil,
+			want:  []string{},
+		},
+		{
+			name:  "full override",
+			base:  []string{"/cache/sha256/abc/code-implementation"},
+			child: []string{"skills/code-implementation"},
+			want:  []string{"skills/code-implementation"},
+		},
+		{
+			name:  "duplicate child basename deduplicates",
+			base:  []string{"/base/skill-a"},
+			child: []string{"/child1/skill-b", "/child2/skill-b"},
+			want:  []string{"/base/skill-a", "/child2/skill-b"},
+		},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got := mergeSkills(tt.base, tt.child)
+			assert.Equal(t, tt.want, got)
+		})
+	}
+}
+
 func TestLoadWithBase_LocalBase_RunnerEnvMerge(t *testing.T) {
 	dir := t.TempDir()