fix(#5408): deduplicate skills by basename during base composition by fullsend-ai-coder[bot] · Pull Request #5409 · fullsend-ai/fullsend

fullsend-ai-coder · 2026-07-21T11:29:02Z

Summary

Fixes skill override via base: composition by deduplicating skills by basename during merge. Previously, a child harness declaring a same-named skill as a built-in produced two entries in the merged skills list, which triggered duplicateDestinationNameError at bootstrap time.

Related Issue

Fixes #5408

Changes

Added mergeSkills helper in compose.go that deduplicates skill paths by filepath.Base, mirroring mergeHostFiles' override-by-dest pattern (child replaces base entry in-place)
Updated mergeBaseIntoChild (compose.go) to use mergeSkills instead of concatenation
Updated mergeForgeConfigInto (compose.go) to use mergeSkills instead of concatenation
Updated mergeForgeConfig (forge.go) to use mergeSkills instead of append
duplicateDestinationNameError in claude.go is unchanged — it remains a safety net for collisions between two unrelated child skills

Testing

New unit tests: TestMergeSkills (6 sub-cases covering nil, no-overlap, override, full-override)
New integration tests: TestLoadWithBase_ChildSkillOverridesBaseByBasename, TestLoadWithBase_ChildSkillOverride_PreservesOrder
New forge test: TestResolveForge_SkillsOverrideByBasename
Existing tests pass: TestLoadWithBase_LocalBase_SkillsConcat, TestResolveForge_SkillsConcat, TestClaudeRuntime_Bootstrap_DuplicateSkillNames_FailsLoudly
Full go test ./internal/harness/... passes
go vet passes

Checklist

PR title follows Conventional Commits (correct type, ! for breaking changes)
Commits are signed off (DCO) — human and human-directed agent sessions only
I wrote this contribution myself and can explain all changes in it

Closes #5408

Post-script verification

Branch is not main/master (agent/5408-skill-override-dedup)
Secret scan passed (gitleaks — 90e75879bf2e67bfab34aa22fca6f125445a5ea8..HEAD)
PR body secret scan passed (gitleaks — no-git)
Pre-commit hooks passed (authoritative run on runner)
Tests ran inside sandbox

When a child harness declares a skill whose directory basename matches a base skill, the merge logic now replaces the base entry instead of concatenating both. This mirrors the existing mergeHostFiles override-by-dest pattern and prevents duplicateDestinationNameError from rejecting legitimate skill overrides at bootstrap time. The fix adds a mergeSkills helper used in three merge paths: mergeBaseIntoChild (compose.go), mergeForgeConfigInto (compose.go), and mergeForgeConfig (forge.go). The duplicateDestinationNameError guard in claude.go is retained as a safety net for genuinely accidental collisions between two unrelated child skills. Closes #5408

fullsend-ai-review · 2026-07-21T11:30:38Z

🤖 Finished Review · ✅ Success · Started 11:30 AM UTC · Completed 11:43 AM UTC
Commit: 1947bbc · View workflow run →

github-actions · 2026-07-21T11:30:59Z

Site preview

Preview: https://805996e8-site.fullsend-ai.workers.dev

Commit: e3b1a9bde95ec2da25388808184fa5c1951604c3

codecov · 2026-07-21T11:34:37Z

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

rh-hemartin · 2026-07-21T11:41:42Z

~~Testing at https://github.com/rh-hemartin-fullsendai/standalone-fullsend/actions/runs/29827076686/job/88622805904~~

Nevermind, I can't test this in my repository until we do get #5357 merged. Either way looks good to me.

fullsend-ai-review · 2026-07-21T11:43:40Z

Review

Findings

Low

[edge-case] internal/harness/compose.go — mergeSkills uses filepath.Base to determine skill identity. If two base skills share the same basename but have different directory paths (e.g., /cache/a/skill-x and /cache/b/skill-x), the second entry silently overwrites the first in baseIndex, and only the second is matchable by a child override. This is unlikely in practice — duplicate basenames within a single base already trigger duplicateDestinationNameError downstream — but is worth noting as a known limitation.

Previous run

Review

Findings

Medium

[stale-merge-semantics] docs/ADRs/0070-portable-provider-profile-resolution.md:91 — States "Same merge pattern as skills in ADR 0045" when describing profile/provider merging during base composition. This PR updates ADR 0045 to describe skills as "merged with deduplication by basename (child overrides base)," but profiles and providers still use plain concatenation in compose.go. The cross-reference is now misleading: readers following it to ADR 0045 will see basename deduplication described for skills, which profiles/providers do not use.
Remediation: Update line 91 to describe the actual merge pattern explicitly (concatenate base + child, deduplicate by profile id / provider name at resolution time) without referencing the skills pattern, since skills now use a different algorithm.

Low

[edge-case] internal/harness/compose.go — mergeSkills uses filepath.Base to determine skill identity. If two base skills share the same basename but have different directory paths (e.g., /cache/a/skill-x and /cache/b/skill-x), the second entry silently overwrites the first in baseIndex, and only the second is matchable by a child override. This is unlikely in practice — duplicate basenames within a single base already trigger duplicateDestinationNameError downstream — but is worth noting as a known limitation.

Previous run (2)

Review

Findings

Medium

[logic-error] internal/harness/diff.go:105 — DiffHarness uses diffStringSlice for skills, which compares entries by full path string. With the new merge-by-basename semantics, when a child overrides a base skill (e.g., base has /cache/sha256/abc/code-implementation and child has skills/code-implementation), the merged harness replaces the base entry in-place. When DiffHarness tries to reverse-engineer the child YAML from the merged result, diffStringSlice sees the original base path missing and sets removed = true, causing a spurious warning and aborting the diff. This breaks fullsend migrate for any harness that uses skill-override-by-basename. The same issue exists at line 425 in diffForgeConfig.
Remediation: Update diffStringSlice (or add a skills-specific variant) to compare by filepath.Base when diffing skills.
[stale-merge-semantics] docs/plans/adr-0045-forge-portable-harness-phase1.md:52 — States "Skills: top-level + forge (concatenated)" in the forge resolution rules, but skills now use deduplication by basename.
Remediation: Change to "Skills: merged with deduplication by basename (forge overrides top-level)".
[stale-merge-semantics] docs/plans/adr-0045-forge-portable-harness-phase1.md:152 — States "Skills, Plugins, Providers, APIServers: concatenated (base + child)" in the mergeHarness spec, but skills now use deduplication by basename.
Remediation: Split skills from the group: "Skills: merged with deduplication by basename (child overrides base); Plugins, Providers, APIServers: concatenated (base + child)".
[stale-merge-semantics] docs/plans/adr-0045-forge-portable-harness-phase2.md:361 — States "Skills contains both base skills and forge skills (concatenated)" in a test specification, but skills now use deduplication by basename.
Remediation: Change to "Skills merged from base and forge with deduplication by basename".

Low

[stale-merge-semantics] docs/ADRs/0064-deprecate-customized-directory-overlay.md:40 — States "skills concatenate" but skills now merge with deduplication by basename. ADR 0064 is Accepted, so only minor annotations are permitted.
Remediation: Add annotation: "(Note: as of Overriding a built-in skill via base: composition fails with duplicate sandbox-name error #5408, skills now merge with deduplication by basename; see ADR 0045.)"
[stale-merge-semantics] docs/ADRs/0064-deprecate-customized-directory-overlay.md:58 — Capability-replacement table states skills are "concatenated via base:", now outdated.
Remediation: Add annotation noting the change to merge-with-deduplication-by-basename semantics.
[stale-merge-semantics] docs/guides/dev/cli-internals.md:170 — States the diff engine mirrors mergeBaseIntoChild semantics with "slice concatenation extras", but skills now use deduplication by basename.
Remediation: Update to distinguish skills from other slices.

Previous run (3)

Review

Findings

High

[stale-merge-semantics] docs/ADRs/0045-forge-portable-harness-schema.md:362 — Line 362 documents that skills merging uses concatenation semantics: skills: base list + child list (concatenated). This PR changes the merge behavior to deduplicate by filepath.Base with child-overrides-base semantics. The same stale description appears at line 190 for forge-level skills merging.
Remediation: Update line 362 to describe deduplication-by-basename behavior. Update line 190 similarly.
[stale-merge-semantics] docs/guides/user/bring-your-own-agent.md:278 — Line 278 states that skills are "Concatenated (base + child)" in the Field merge rules table. This contradicts the new deduplication behavior. Lines 369 and 423 contain comments and prose reinforcing the concatenation model.
Remediation: Update line 278 to "Merged with deduplication by basename (child overrides base)". Update the comment at line 369 and line 423 similarly.

Medium

[missing-behavior-details] docs/ADRs/0045-forge-portable-harness-schema.md:612 — Line 612 summarizes the merge semantics and explicitly lists "skills concatenate": "The inheritance rules (scalars override, skills concatenate, runner_env merges, validation_loop replaces)". This summary is now incorrect. A similar reference at line 660 in the alternatives section also mentions concatenation.
Remediation: Update line 612 to replace "skills concatenate" with "skills merge with deduplication by basename". Review and update line 660 similarly.

Low

[edge-case] internal/harness/compose.go:1159 — Unlike mergeHostFiles which updates destIndex when appending new child entries, mergeSkills does not update baseIndex for appended child entries. If the child slice contains two entries with the same basename (e.g., ["/child1/skill-a", "/child2/skill-a"]), both would be appended rather than the second overriding the first. In practice this is benign because duplicateDestinationNameError at bootstrap time catches this misconfiguration. However, it is an asymmetry with mergeHostFiles that could surprise a future maintainer.
Remediation: Consider updating baseIndex when appending child entries, matching the pattern in mergeHostFiles.

Labels: PR modifies harness composition logic (internal/harness/) to fix skill deduplication during base: composition

ggallen · 2026-07-21T11:46:50Z

/fs-fix fix and resolve all the review issues that are actionable, both inline and in the sticky comment.

fullsend-ai-coder · 2026-07-21T11:48:25Z

🤖 Finished Fix · ✅ Success · Started 11:48 AM UTC · Completed 11:54 AM UTC
Commit: 7ada4e0 · View workflow run →

- ADR 0045: update skills merge description in forge inheritance table (line 190), base→child merge rules (line 362), complexity note (line 612), and nil-vs-empty section (line 660) to describe dedup-by-basename instead of concatenation - bring-your-own-agent.md: separate skills from plugins/providers in merge rules table (line 278), update example comment (line 369), and "What you can configure" prose (line 423) - compose.go mergeSkills: update baseIndex when appending new child entries, matching mergeHostFiles' pattern for consistent dedup of duplicate child basenames - compose_test.go: add test case for duplicate child basename dedup Addresses review feedback on #5409

fullsend-ai-coder · 2026-07-21T11:54:26Z

🔧 Fix agent — iteration 1 (human-triggered)

Addressed all 4 review findings: updated stale concatenation references in ADR 0045 (4 locations) and bring-your-own-agent.md (3 locations) to describe dedup-by-basename semantics; fixed mergeSkills to update baseIndex on child append matching mergeHostFiles pattern; added test for duplicate child basename edge case.

Fixed (4):

stale-merge-semantics in ADR 0045 lines 190 and 362 (docs/ADRs/0045-forge-portable-harness-schema.md): Updated forge inheritance table (line 190) and base-child merge rules (line 362) to describe dedup-by-basename instead of concatenation
stale-merge-semantics in bring-your-own-agent.md lines 278, 369, 423 (docs/guides/user/bring-your-own-agent.md): Separated skills from plugins/providers in merge rules table (line 278), updated example comment (line 369), and What you can configure prose (line 423) to describe dedup-by-basename semantics
missing-behavior-details in ADR 0045 lines 612 and 660 (docs/ADRs/0045-forge-portable-harness-schema.md): Updated complexity note (line 612) and nil-vs-empty section (line 660) to replace skills concatenate with dedup-by-basename description
edge-case: mergeSkills does not update baseIndex for appended child entries (internal/harness/compose.go): Added baseIndex[name] = len(result) when appending new child entries, matching mergeHostFiles pattern. Added test case for duplicate child basenames.

Tests: passed

_{Updated by fullsend fix agent}

github-actions · 2026-07-21T11:54:36Z

E2E tests did not run

E2E tests run automatically for org/repo members and collaborators on pull requests.

For other contributors, a maintainer must add the ok-to-test label after the latest push.

See E2E testing guide for details.

ggallen · 2026-07-21T12:22:44Z

/fs-review

fullsend-ai-review · 2026-07-21T12:24:19Z

🤖 Finished Review · ✅ Success · Started 12:24 PM UTC · Completed 12:40 PM UTC
Commit: 7ada4e0 · View workflow run →

Superseded by updated review

ggallen · 2026-07-21T12:41:47Z

/fs-fix fix and resolve all the review issues that are actionable, both inline and in the sticky comment.

fullsend-ai-coder · 2026-07-21T12:43:33Z

🤖 Finished Fix · ✅ Success · Started 12:43 PM UTC · Completed 12:52 PM UTC
Commit: 7ada4e0 · View workflow run →

Add diffSkills function that compares skill paths by filepath.Base instead of exact string, matching mergeSkills override-by-basename semantics. DiffHarness and diffForgeConfig now use diffSkills for skills instead of diffStringSlice, preventing spurious "child removes items from base" warnings when a child overrides a base skill by basename (e.g. fullsend migrate). Update stale "skills concatenate" references in plan docs (phase1.md, phase2.md), ADR 0064 (minor annotations on Accepted ADR), and cli-internals.md to describe merge-with-deduplication-by- basename semantics. Addresses review feedback on #5409

fullsend-ai-coder · 2026-07-21T12:52:46Z

🔧 Fix agent — iteration 2 (human-triggered)

Addressed all 7 review findings. Added diffSkills function for basename-aware skill diffing (fixes logic-error in DiffHarness/diffForgeConfig that would break fullsend migrate with skill overrides). Updated 5 stale doc references from concatenation to merge-with-deduplication-by-basename semantics.

Fixed (7):

logic-error: DiffHarness uses diffStringSlice for skills, breaking fullsend migrate with basename overrides (internal/harness/diff.go): Added diffSkills function that compares by filepath.Base instead of exact string, matching mergeSkills override-by-basename semantics. Replaced diffStringSlice calls for skills in both DiffHarness (line 105) and diffForgeConfig (line 425). Added tests for both DiffHarness and forge skill override by basename.
stale-merge-semantics: docs/plans/adr-0045-forge-portable-harness-phase1.md:52 says skills concatenated (docs/plans/adr-0045-forge-portable-harness-phase1.md): Changed 'Skills: top-level + forge (concatenated)' to 'Skills: merged with deduplication by basename (forge overrides top-level)'
stale-merge-semantics: docs/plans/adr-0045-forge-portable-harness-phase1.md:152 groups skills with other concatenated slices (docs/plans/adr-0045-forge-portable-harness-phase1.md): Split skills from Plugins/Providers/APIServers: 'Skills: merged with deduplication by basename (child overrides base)' on its own line, remaining slices still listed as concatenated
stale-merge-semantics: docs/plans/adr-0045-forge-portable-harness-phase2.md:361 says skills concatenated (docs/plans/adr-0045-forge-portable-harness-phase2.md): Changed 'Skills contains both base skills and forge skills (concatenated)' to 'Skills merged from base and forge with deduplication by basename'
stale-merge-semantics: docs/ADRs/0064-deprecate-customized-directory-overlay.md:40 says skills concatenate (docs/ADRs/0064-deprecate-customized-directory-overlay.md): Added minor annotation noting skills now merge with deduplication by basename per Overriding a built-in skill via base: composition fails with duplicate sandbox-name error #5408, with cross-reference link. ADR is Accepted so only annotation was appropriate.
stale-merge-semantics: docs/ADRs/0064-deprecate-customized-directory-overlay.md:58 says skills concatenated via base: (docs/ADRs/0064-deprecate-customized-directory-overlay.md): Updated capability-replacement table: changed 'concatenated via base:' to 'merged with deduplication by basename via base:' with Overriding a built-in skill via base: composition fails with duplicate sandbox-name error #5408 cross-reference
stale-merge-semantics: docs/guides/dev/cli-internals.md:170 says slice concatenation extras without distinguishing skills (docs/guides/dev/cli-internals.md): Updated diff engine description to distinguish basename-aware skill overrides (matching mergeSkills) from slice concatenation extras for plugins/providers

Tests: passed

_{Updated by fullsend fix agent}

ggallen · 2026-07-21T13:03:22Z

/fs-review

fullsend-ai-review · 2026-07-21T13:05:02Z

🤖 Finished Review · ✅ Success · Started 1:05 PM UTC · Completed 1:22 PM UTC
Commit: 7ada4e0 · View workflow run →

ggallen · 2026-07-21T13:37:16Z

/fs-fix fix and resolve all the review issues that are actionable, both inline and in the sticky comment.

fullsend-ai-coder · 2026-07-21T13:39:01Z

🤖 Finished Fix · ✅ Success · Started 1:39 PM UTC · Completed 1:45 PM UTC
Commit: 7ada4e0 · View workflow run →

…geSkills limitation - ADR 0070 line 91: replaced misleading cross-reference to skills merge pattern in ADR 0045. Profiles/providers use concatenation with id/name dedup, not the basename-aware dedup that skills now use. - compose.go mergeSkills: added code comment documenting the known limitation when base slice contains duplicate basenames. Addresses review feedback on #5409

fullsend-ai-coder · 2026-07-21T13:45:25Z

🔧 Fix agent — iteration 3 (human-triggered)

Addressed both review findings from the latest review cycle. Fixed misleading cross-reference in ADR 0070 that referenced skills merge pattern (now outdated since skills use basename-aware dedup). Documented the mergeSkills duplicate-basename limitation as a code comment.

Fixed (2):

stale-merge-semantics: ADR 0070 line 91 cross-references skills merge pattern which is now misleading (docs/ADRs/0070-portable-provider-profile-resolution.md): Updated line 91 to describe the actual profile/provider merge pattern (concatenation with last-writer-wins dedup by id/name) instead of cross-referencing skills, since skills now use a different algorithm (basename-aware dedup per Overriding a built-in skill via base: composition fails with duplicate sandbox-name error #5408). Added cross-reference to Overriding a built-in skill via base: composition fails with duplicate sandbox-name error #5408 for traceability.
edge-case: mergeSkills duplicate basename limitation in base slice (internal/harness/compose.go): Added code comment documenting the known limitation: if the base slice contains two entries with the same basename, the second silently overwrites the first in baseIndex. This is benign in practice because duplicateDestinationNameError catches duplicate basenames at bootstrap time.

Tests: passed

_{Updated by fullsend fix agent}

ggallen · 2026-07-21T13:45:57Z

/fs-review

fullsend-ai-review · 2026-07-21T13:47:25Z

🤖 Finished Review · ✅ Success · Started 1:47 PM UTC · Completed 2:01 PM UTC
Commit: 7ada4e0 · View workflow run →

fullsend-ai-coder Bot requested a review from a team as a code owner July 21, 2026 11:29

fullsend-ai-coder Bot added the ready-for-review Triggers review agent dispatch label Jul 21, 2026

fullsend-ai-coder Bot assigned ggallen Jul 21, 2026

github-actions Bot deployed to site-preview July 21, 2026 11:30 View deployment

rh-hemartin approved these changes Jul 21, 2026

View reviewed changes

This comment was marked as outdated.

Sign in to view

fullsend-ai-review Bot added component/harness Agent harness, config, and skills loading bug labels Jul 21, 2026

github-actions Bot deployed to site-preview July 21, 2026 11:56 View deployment

rh-hemartin added the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 21, 2026

ggallen mentioned this pull request Jul 21, 2026

Review agent skipped on bot-authored PRs due to collaborator permission check failing for GitHub App accounts #5188

Closed

fullsend-ai-review Bot added the requires-manual-review Review requires human judgment label Jul 21, 2026

github-actions Bot removed the ok-to-test Allow e2e CI to run after maintainer review (must be re-applied after each push) label Jul 21, 2026

github-actions Bot deployed to site-preview July 21, 2026 12:54 View deployment

github-actions Bot deployed to site-preview July 21, 2026 13:47 View deployment

fullsend-ai-review Bot approved these changes Jul 21, 2026

View reviewed changes

fullsend-ai-review Bot added ready-for-merge All reviewers approved — ready to merge and removed requires-manual-review Review requires human judgment labels Jul 21, 2026

ggallen added this pull request to the merge queue Jul 21, 2026

Merged via the queue into main with commit 30e3e72 Jul 21, 2026
24 of 25 checks passed

ggallen deleted the agent/5408-skill-override-dedup branch July 21, 2026 14:16

ggallen mentioned this pull request Jul 31, 2026

Add bot carve-out to review dispatch path for pull_request_target events #5706

Closed

qodo-code-review Bot mentioned this pull request Aug 17, 2026

feat(harness): implement CEL-guarded overlays (ADR 0088) #6285

Merged

6 tasks

Uh oh!

Conversation

fullsend-ai-coder Bot commented Jul 21, 2026

Summary

Related Issue

Changes

Testing

Checklist

Post-script verification

Uh oh!

fullsend-ai-review Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Site preview

Uh oh!

codecov Bot commented Jul 21, 2026

Codecov Report

Uh oh!

rh-hemartin commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Review

Findings

Low

Review

Findings

Medium

Low

Review

Findings

Medium

Low

Review

Findings

High

Medium

Low

Uh oh!

This comment was marked as outdated.

Uh oh!

ggallen commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-coder Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-coder Bot commented Jul 21, 2026

🔧 Fix agent — iteration 1 (human-triggered)

Uh oh!

github-actions Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

E2E tests did not run

Uh oh!

ggallen commented Jul 21, 2026

Uh oh!

fullsend-ai-review Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggallen commented Jul 21, 2026

Uh oh!

fullsend-ai-coder Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-coder Bot commented Jul 21, 2026

🔧 Fix agent — iteration 2 (human-triggered)

Uh oh!

ggallen commented Jul 21, 2026

Uh oh!

fullsend-ai-review Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ggallen commented Jul 21, 2026

Uh oh!

fullsend-ai-coder Bot commented Jul 21, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fullsend-ai-review Bot commented Jul 21, 2026 •

edited

Loading

github-actions Bot commented Jul 21, 2026 •

edited

Loading

rh-hemartin commented Jul 21, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 21, 2026 •

edited

Loading

ggallen commented Jul 21, 2026 •

edited

Loading

fullsend-ai-coder Bot commented Jul 21, 2026 •

edited

Loading

github-actions Bot commented Jul 21, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 21, 2026 •

edited

Loading

fullsend-ai-coder Bot commented Jul 21, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 21, 2026 •

edited

Loading

fullsend-ai-coder Bot commented Jul 21, 2026 •

edited

Loading

fullsend-ai-review Bot commented Jul 21, 2026 •

edited

Loading