Skip to content

Avoid duplicate HEY skills in Codex - #385

Open
almirsarajcic wants to merge 3 commits into
basecamp:mainfrom
almirsarajcic:fix/codex-skill-duplicate
Open

Avoid duplicate HEY skills in Codex#385
almirsarajcic wants to merge 3 commits into
basecamp:mainfrom
almirsarajcic:fix/codex-skill-duplicate

Conversation

@almirsarajcic

@almirsarajcicalmirsarajcic commented Sep 2, 2026

Copy link
Copy Markdown

Closes#287.

Problem

Current Codex discovers personal skills from ~/.agents/skills, so copying the same HEY skill into ~/.codex/skills creates two indistinguishable entries.

Change

  • Make the shared Agent Skills location the single source used by Codex.
  • Stop creating a second Codex-specific skill copy.
  • Update Codex setup, health checks, refresh behavior, and documentation to use the shared path.
  • During explicit setup and once-per-version refresh, remove an older Codex-specific copy only when hey-cli's ownership marker proves it is managed.
  • Preserve unmarked user-authored copies.
  • Preserve a managed legacy copy until the shared replacement has been installed and verified healthy.

The existing hey setup agents --remove path remains the supported uninstall flow. This PR does not change skill invocation policy; HEY remains available for normal implicit selection.

Verification

  • TMPDIR=/tmp go test ./internal/...
  • TMPDIR=/tmp go test ./internal/cmd ./internal/harness ./skills
  • go vet ./...
  • formatting and tidy checks
  • command-surface check
  • golangci-lint: 0 issues

The short TMPDIR avoids unrelated Unix-domain socket path-length failures in macOS TUI tests.


Summary by cubic

Stops Codex from seeing duplicate HEY skills by making the shared ~/.agents/skills location the single source. Setup, hey skill install, health checks, and version refresh no longer copy the skill into ~/.codex/skills; Codex discovers the shared skill directly.

Migration

  • Removes an existing legacy ~/.codex/skills/hey copy only during explicit setup or once-per-version refresh, and only when the ownership marker proves hey-cli created it and the shared replacement is healthy.
  • Leaves unmarked user-authored copies untouched, and keeps the legacy copy if installing or refreshing the shared skill fails.
  • Skips removal when CODEX_HOME aliases ~/.agents, so the shared skill is never deleted as its own legacy copy.
  • A managed legacy duplicate makes the Codex health check fail with a hint to run hey setup codex.
  • hey setup agents --remove remains the supported uninstall flow.

Written for commit 0cfb6ae. Summary will update on new commits.

Review in cubic

@almirsarajcic
almirsarajcic requested a review from a team as a code ownerSeptember 2, 2026 06:49
CopilotAI balanced review requested due to automatic review settings September 2, 2026 06:49

CopilotAI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟡 Changes recommended

The full setup wizard can short-circuit before removing a managed legacy Codex copy, leaving the duplicate integration in place.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Moves Codex integration to the shared Agent Skills path and safely migrates managed legacy copies.

Changes:

  • Uses ~/.agents/skills/hey for Codex setup and health checks.
  • Removes managed legacy Codex copies during installation or refresh.
  • Updates tests and documentation for the shared-path behavior.

[!TIP]
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.

File summaries
FileDescription
README.mdDocuments Codex shared-skill discovery and migration.
AGENTS.mdUpdates repository integration guidance.
internal/harness/harness.goAdds the canonical shared skill path.
internal/harness/codex.goMoves Codex health checks to the shared path.
internal/harness/codex_test.goTests shared-path health behavior.
internal/cmd/skill_refresh.goRemoves managed legacy copies during refresh.
internal/cmd/skill_refresh_test.goCovers refresh migration and preservation rules.
internal/cmd/skill_install.goStops Codex-specific copying and adds migration.
internal/cmd/skill_install_test.goTests installation and legacy-copy safety.
internal/cmd/setup_test.goUpdates wizard behavior coverage.
internal/cmd/setup_agents_test.goUpdates agent setup and removal coverage.
internal/cmd/setup_agents_remove.goRemoves the legacy path during uninstall.
internal/cmd/setup_agent.goConnects Codex through the shared skill.
Review details
  • Files reviewed: 13/13 changed files
  • Comments generated: 2
  • Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

Comment threadinternal/harness/codex.go
Comment threadinternal/harness/codex.go

@cubic-dev-aicubic-dev-aiBot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All reported issues were addressed across 13 files

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment threadinternal/cmd/skill_refresh.go Outdated
Comment threadinternal/cmd/skill_refresh.go Outdated
Comment threadinternal/cmd/skill_install.go
Comment threadinternal/cmd/skill_install.go Outdated
Comment threadinternal/harness/codex.go
Comment threadinternal/cmd/skill_install_test.go
Comment threadinternal/cmd/setup_agent.go Outdated
@almirsarajcic

Copy link
Copy Markdown
Author

Addressed all current review findings in 0cfb6ae:

  • make Codex health/preflight detect a managed legacy duplicate so repeated hey setup migrates it
  • never remove the legacy copy after a failed refresh
  • treat aliased shared/legacy paths as the same file, including CODEX_HOME=$HOME/.agents
  • migrate immediately after the shared install, before optional Claude setup
  • report shared Agent Skills home-resolution failures accurately
  • assert unmanaged legacy directories are never claimed

Added regression coverage for each path. Verified with go test ./internal/... (using TMPDIR=/tmp on macOS to keep Unix socket paths short), go vet ./..., make fmt-check, and golangci-lint v2.11.1 (0 issues).

Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

hey skill install duplicates /hey in Codex by writing both ~/.agents/skills/hey and ~/.codex/skills/hey

2 participants

@almirsarajcic