feat(#2661): warn when repo skills are shadowed

[shairevivo]

Summary

Warn before Claude runs when a repo-level skill is shadowed by a same-named harness skill installed at the higher-precedence personal level. The warning points users to unique names for extension or base: harness composition for intentional overrides.

Related Issue

Closes #2661

Changes

• detect collisions against real .claude/skills/<name>/SKILL.md entries
• emit an actionable warning without changing existing precedence
• cover collision and non-collision behavior with filesystem-backed tests
• update runtime, user, and augmentation-skill guidance for the current ADR 0064 customization model

Testing

• make lint passes (stage changes first, then run)
• Tests added/updated for new or modified logic
• go test -race ./internal/cli -run TestWarnRepoSkillCollisions -count=1
• Approximate patch coverage: warnRepoSkillCollisions 93.3%
• make e2e-test locally — blocked before test execution because the local GitHub identity cannot create lock repositories in the halfsend-* pool orgs; CI will run with the repository's cross-org e2e credentials

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

[github-actions]

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.

[qodo-code-review]

PR Summary by Qodo

Warn when Claude repo skills are shadowed by harness skills

✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

AI Description

• Warn before Claude starts when harness skills shadow same-named repository skills.
• Validate collisions only for discoverable repository skills containing regular SKILL.md files.
• Document unique-name extension and base: composition override paths.

Diagram

graph TD
  A["Agent startup"] --> B{"Claude runtime?"} -->|Yes| C["Harness skills"] --> D["Collision check"] --> F{"Same skill name?"} -->|Yes| G["Warning output"]
  E["Repo skills"] --> D
  F -->|No| H["Continue bootstrap"]
  B -->|No| H

Loading

High-Level Assessment

The host-side Claude preflight is the appropriate approach: it uses the existing harness skill list, inspects the repository before upload, preserves established precedence, and avoids imposing Claude-specific semantics on other runtimes. Moving detection into runtime bootstrap would add coupling without improving behavior.

Files changed (7) +105 / -8

Enhancement (2) +43 / -0

run.goRun skill collision checks before Claude bootstrap +3/-0

Run skill collision checks before Claude bootstrap

• Invokes repository skill collision detection for Claude runtimes after harness bootstrap inputs are assembled and before sandbox content is installed.

internal/cli/run.go

skill_collision.goDetect and warn about shadowed repository skills +40/-0

Detect and warn about shadowed repository skills

• Builds a set of harness skill basenames, scans '.claude/skills', and warns for matching entries with a regular 'SKILL.md'. Missing or unreadable skill directories and non-skill entries are ignored without affecting startup.

internal/cli/skill_collision.go

Tests (1) +50 / -0

skill_collision_test.goCover collision warning and non-collision behavior +50/-0

Cover collision warning and non-collision behavior

• Adds filesystem-backed tests verifying actionable output for a real shadowed skill and no output for unique or invalid repository skill entries.

internal/cli/skill_collision_test.go

Documentation (4) +12 / -8

runtime-implementation.mdDocument warned skill shadowing in runtime precedence +1/-1

Document warned skill shadowing in runtime precedence

• Updates the runtime layering diagram to state that higher-precedence Fullsend skills still win while repository shadowing now emits a warning.

docs/contributing/runtime-implementation.md

customizing-agents.mdClarify collision warnings for agent customizations +2/-1

Clarify collision warnings for agent customizations

• Notes that same-named repository and built-in skills are ignored with a warning, directing users to the detailed precedence guidance.

docs/guides/user/customizing-agents.md

customizing-with-skills.mdExplain warned collisions and supported override paths +5/-3

Explain warned collisions and supported override paths

• Replaces the silent-shadowing guidance with the new warning behavior. Recommends unique skill names for extension and derived harness 'base:' composition for intentional overrides.

docs/guides/user/customizing-with-skills.md

SKILL.mdUpdate augmentation guidance for warned skill collisions +4/-3

Update augmentation guidance for warned skill collisions

• Teaches augmentation authors that same-named built-ins trigger warnings and documents unique naming versus 'base:' composition as the supported extension and override strategies.

skills/author-fullsend-augmentations/SKILL.md

[github-actions]

Site preview

Preview: https://ebf21bd4-site.fullsend-ai.workers.dev

Commit: 6ad9cbb9680a3b9d15e0360ddf6a4564407fc271

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

[qodo-code-review]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (1) 📜 Skill insights (0)

1. Non-skills trigger shadow warning ✓ Resolved 🐞 Bug ≡ Correctness

Description

warnRepoSkillCollisions adds every harness source basename without checking that the source
contains a regular SKILL.md, so a local configured directory that Claude cannot discover as a
personal skill still causes a valid repo skill with that basename to be reported as shadowed. The
run then uploads the marker-less directory successfully, making the warning incorrect even though
the repo skill remains the only discoverable skill.

Code

internal/cli/skill_collision.go[R15-17]

+	for _, skillDir := range harnessSkillDirs {
+		if skillDir != "" {
+			harnessSkills[filepath.Base(skillDir)] = struct{}{}

Relevance

●●● Strong

Concrete false-positive correctness issue; recent CLI history accepts narrowly scoped behavioral
fixes and regression coverage.

PR-#6784
PR-#6261

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

The user guide defines a skill as a directory containing required SKILL.md; harness validation
only constrains URL entries and overrides, while Claude bootstrap uploads every supplied directory
without checking that marker. The new map construction therefore labels a class of accepted local
directories as higher-precedence skills even when they are not discoverable.

docs/guides/user/customizing-with-skills.md[12-16]
internal/harness/harness.go[952-982]
internal/runtime/claude.go[66-77]
internal/cli/skill_collision.go[14-18]
internal/cli/skill_collision.go[27-38]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Only include harness sources that are actual discoverable skills; a directory without a regular `SKILL.md` must not cause a repo skill to be reported as shadowed.

## Issue Context
Local harness skill paths are not currently validated for a `SKILL.md`, and Claude bootstrap uploads any configured directory. Mirror the marker check already applied to repository entries, including suitable tests for a marker-less harness directory.

## Fix Focus Areas
- internal/cli/skill_collision.go[14-18]
- internal/cli/skill_collision_test.go[15-29]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. base: jargon lacks definition ✓ Resolved 📜 Skill insight ✧ Quality

Description

The guide introduces derived harness and base: composition without defining or linking those
terms at their first use. Readers cannot reliably interpret the newly recommended override path
without searching later sections.

Code

docs/guides/user/customizing-with-skills.md[92]

+or use a derived harness with `base:` composition for an intentional override.

Relevance

●●● Strong

Recent docs precedent explicitly accepted defining or linking jargon on first use.

PR-#6455

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 1062083 requires jargon to be defined or linked on first occurrence. The changed line
introduces derived harness and base: composition without either treatment; the explanation
appears only later under the override section.

docs/guides/user/customizing-with-skills.md[88-92]
Skill: writing-user-docs

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Define or link the `derived harness` and `base:` composition terminology where the override path is first introduced.

## Issue Context
PR Compliance ID 1062083 requires domain-specific jargon in guides to be defined inline or linked on first use. The guide explains this mechanism later, so a direct link to that section is sufficient.

## Fix Focus Areas
- docs/guides/user/customizing-with-skills.md[92-92]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

3. E2E tests not passed 📘 Rule violation ▣ Testability

Description

This PR changes internal/cli/, but the PR description explicitly states that make e2e-test was
blocked and did not run locally, with no successful latest-commit result provided. The critical CLI
change therefore lacks the required evidence of a passing end-to-end test run.

Code

internal/cli/run.go[R1646-1648]

+	if rt.Name() == "claude" {
+		warnRepoSkillCollisions(hostRepositoryDir, boot.SkillDirs(), printer)
+	}

Relevance

●● Moderate

E2E evidence is explicitly missing, but history lacks a close precedent requiring this exact test
gate.

ⓘ Recommendations generated based on similar findings in past PRs

Evidence

Rule 1062051 applies to changes under internal/cli/ and requires evidence that make e2e-test
succeeded. The cited changed lines alter CLI execution behavior, while the configured E2E workflow
runs make e2e-test; the supplied PR description records the local test as blocked rather than
passed.

Rule 1062051: Run end-to-end tests for critical internal modules before merge
internal/cli/run.go[1646-1648]
.github/workflows/e2e.yml[168-170]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
Obtain a successful `make e2e-test` result for the latest PR commit before merge.

## Issue Context
Changes under `internal/cli/` trigger PR Compliance ID 1062051. The repository workflow contains the E2E command, but the PR description says the local run was blocked and provides no successful CI result.

## Fix Focus Areas
- internal/cli/run.go[1646-1648]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

Context sources

✅ Compliance rules (platform): 62 rules

✅ Skills: writing-user-docs, writing-adrs

Review mode: ⚖️ Balanced: This is a runtime behavior change in the CLI that affects skill discovery warnings, with filesystem edge cases and integration-path implications; it is more than a trivial localized edit, but not dense enough to warrant redundant extended review.

Tip of the day

💡 Did you know, you can describe a rule in plain language on the Rules page and Qodo drafts it for you

More tips ↗ | Customize Qodo ↗ | Qodo docs ↗

[rh-hemartin]

/fs-review

[fullsend-ai-review]

🤖 Finished Review · ✅ Success · Started 10:24 AM UTC · Completed 10:43 AM UTC

Commit: 7bdcd8d · View workflow run →

Runtime: claude · Model: opus → claude-opus-4-6 · Effort: high · Cost: $6.01

[fullsend-ai-review]

Risk Assessment: moderate (2/5)

Details

Small additive feature (7 files, 187 lines) with new files, tests, and doc updates. The 3-line touch to run.go (a high-churn file) elevates risk slightly, but the change is a simple function call insertion with no behavior modification to existing code. Issue scope aligns well with implementation.

[fullsend-ai-review]

Review

Findings

Medium

• [protected-path] skills/author-fullsend-augmentations/SKILL.md — This PR modifies a protected path (skills/). The change updates the augmentation authoring guide to reflect the new collision warning behavior. Human approval is required for protected-path changes regardless of context.

Low

• [consistency] internal/cli/skill_collision.go:41 — isReadableSkillMarker only checks for uppercase SKILL.md, while scanSkillDir in bootstrap_scan.go checks three casings ("SKILL.md", "skill.md", "Skill.md"). A repo skill with a differently-cased marker recognized by scanSkillDir would be missed by collision detection.
  Remediation: Consider matching the multi-casing pattern from scanSkillDir.

• [scope-drift] internal/cli/skill_collision.go:13 — Issue Log a warning when a repo skill is shadowed by a personal-level skill #2661 specifies detecting collisions between repo-level and personal-level skills, but the PR uses boot.SkillDirs() which returns harness-composed skill sources (potentially broader than personal-level only). The broader scope is arguably more useful since any higher-precedence skill shadows the repo skill regardless of source.

• [intent-alignment] internal/cli/skill_collision.go:42 — Issue Log a warning when a repo skill is shadowed by a personal-level skill #2661 mentions customized/skills/ (ADR-0035) as the override mechanism, but the PR directs users to base: harness composition (ADR-0064). This substitution is architecturally correct — ADR-0064 supersedes ADR-0035 and the customized/ overlay has been removed.

[fullsend-ai-review]

See the review comment for full details.

[ralphbean]

Thanks!