fix(codex): install skills in canonical agents directory - #1511
Conversation
Deploying openspec-docs with
|
| Latest commit: |
ed0a220
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://82592e08.openspec-docs.pages.dev |
| Branch Preview URL: | https://codex-fix-1509-codex-skills.openspec-docs.pages.dev |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughCodex now generates skills under ChangesCodex shared skill migration
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (5)
src/core/shared-skill-target.ts (1)
82-92: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
writeSharedSkillTargetcreates the skills directory unconditionally.Line 90 runs
mkdirSyncon<root>/skillsbefore the marker write. Every caller insrc/core/init.tsandsrc/core/update.tsline 287 runs inside a skills-generation branch, so the directory already exists there. The call atsrc/core/update.tsline 1055 runs outside the skills branch, so a commands-only run can create an empty.agents/skills/tree holding only the marker. A guard keeps the marker tied to a real skill tree.♻️ Optional guard
const target = markerPath(projectPath, tool.skillsDir); FileSystemUtils.assertProjectArtifactPath(projectPath, target); + if (!fs.existsSync(path.dirname(target))) return; fs.mkdirSync(path.dirname(target), { recursive: true }); fs.writeFileSync(target, `${toolId}\n`, 'utf-8');🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/shared-skill-target.ts` around lines 82 - 92, Update writeSharedSkillTarget so it does not create the skills directory when no skills tree exists. Before the mkdirSync call, verify the tool’s skills directory already exists under projectPath and return early when it does not; preserve the existing marker write behavior for existing shared skill trees.docs/supported-tools.md (1)
145-149: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider documenting how an unmarked shared tree is resolved.
reconcileSharedSkillTargetsinsrc/core/shared-skill-target.tsapplies two more rules that a user can observe. An unmarked.agents/skillstree keeps the vendor-neutral meaning. An unmarked tree becomes Codex-owned when OpenSpec-managed skills still exist under.codex/skills. Adding one sentence here explains why an upgraded project can report either owner.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@docs/supported-tools.md` around lines 145 - 149, Update the shared-tree documentation near the `.openspec-target` marker description to explain the unmarked-tree rules from reconcileSharedSkillTargets: an unmarked `.agents/skills` remains vendor-neutral, but becomes Codex-owned when OpenSpec-managed skills still exist under `.codex/skills`.src/core/init.ts (2)
792-804: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicated legacy-migration reporting loop. Both files now repeat the same loop that calls
migrateLegacyToolDirs, logsdescribeLegacyMigration, and reportskeptInPlaceNotice. The shared root cause is the absence of one helper for this reporting. Four copies exist in total, includingsrc/core/init.tslines 209-218 andsrc/core/update.tslines 339-348.
src/core/init.ts#L792-L804: replace this loop with a call to a new exported helper, for examplereportLegacyMigrations(projectPath, [tool.value], 'after-generation')insrc/core/migration.ts.src/core/update.ts#L1075-L1084: replace this loop with the same helper call, keepingthis.reportKeptInPlacebehavior in the helper or passing a reporter callback.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/init.ts` around lines 792 - 804, Create an exported reportLegacyMigrations helper in src/core/migration.ts that performs the shared migrateLegacyToolDirs iteration and migration logging, while supporting the existing this.reportKeptInPlace behavior required by src/core/update.ts. Replace the duplicated loops at src/core/init.ts:792-804 and src/core/update.ts:1075-1084 with calls using projectPath, [tool.value], and 'after-generation'; preserve each caller’s kept-in-place reporting behavior.
603-607: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShared-root policy is hard-coded here and duplicates
reconcileSharedSkillTargets.Lines 603-605 encode the Codex/agents pair by literal id.
reconcileSharedSkillTargetsalready resolves overlapping roots generically byskillsDir. If another tool later adopts.agents, this branch will not cover it, and the two rules can disagree. Group the selection byskillsDirinstead, and reuse the shared reconciliation for the tie-break.The drop is also silent. The user asked for
agentsand receives no message that Codex now owns the tree. A one-line notice matches the documented behavior indocs/supported-tools.mdlines 120-124.♻️ Suggested notice
const reconciledToolIds = toolIds.includes('codex') && toolIds.includes('agents') ? toolIds.filter((toolId) => toolId !== 'agents') : toolIds; + if (reconciledToolIds.length !== toolIds.length) { + console.log( + chalk.dim('Codex and the shared agents target use .agents/skills; keeping the Codex-rendered tree.') + ); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/init.ts` around lines 603 - 607, Replace the literal `codex`/`agents` filtering that produces `reconciledToolIds` with grouping by each tool’s `skillsDir`, then reuse `reconcileSharedSkillTargets` to select the owner for overlapping roots. Preserve non-overlapping tools, and emit a one-line notice when `agents` is requested but Codex owns the shared tree, using the existing logging or notice mechanism.src/core/config.ts (1)
3-16: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the OpesPEC skill names from
WORKFLOW_TO_SKILL_DIR.
OPENSPEC_SKILL_NAMESduplicates the mapping already insrc/core/profile-sync-drift.ts.hasLegacySkills()reads this list, so a workflow added toWORKFLOW_TO_SKILL_DIRandALL_WORKFLOWSwould not create legacy skill detection for shared roots like.codex/skills/. Import by value fromprofile-sync-driftifconfig.jsmust avoid theprofile-sync-drift.jsimport cycle.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/core/config.ts` around lines 3 - 16, Replace the duplicated OPENSPEC_SKILL_NAMES list in config.ts with a value derived from WORKFLOW_TO_SKILL_DIR in profile-sync-drift.ts, ensuring hasLegacySkills() automatically recognizes newly added workflows. Preserve the existing exported constant contract, and use an import-by-value approach if needed to avoid the config/profile-sync-drift dependency cycle.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/migration.ts`:
- Line 156: Update the timing filter in collectLegacyToolMigrations so
legacy.timing (defaulting to before-generation) is enforced regardless of apply
mode, ensuring dry-run results match apply behavior. Also derive the timing flag
consistently when findLegacyToolMigrations invokes migrateSkillDirs(), using the
same normalized legacy timing value.
In `@src/core/profile-sync-drift.ts`:
- Around line 86-87: Update the identity comparison in the profile-sync loop to
pass both legacySkill and currentSkill through
FileSystemUtils.canonicalizeExistingPath() before comparing them. Preserve the
existing continue behavior when the canonicalized paths match.
- Around line 89-90: Invert the comparison in the profile-sync drift check
around the legacySkill handling so equal contents return no drift and differing
contents report drift; preserve the existing same-file behavior. Add regressions
in profile-sync-drift tests covering distinct copied legacy files with equal and
different content.
In `@src/core/update.ts`:
- Line 1055: Move the writeSharedSkillTarget(projectPath, tool.value) call into
the skills-generation branch, alongside the code that writes skill files,
matching the existing patterns at the other call sites. Ensure tools using
delivery: 'commands' or otherwise skipping skill generation do not update the
shared ownership marker.
In `@test/core/update.test.ts`:
- Around line 492-523: Update the Codex symlink tests around the shown test and
the related cases at 525-554 to create directory links with the appropriate
directory type hint (such as 'dir') for Windows compatibility. Handle
environments where symlink creation is not permitted by skipping the affected
tests rather than failing, while preserving the existing assertions when setup
succeeds.
---
Nitpick comments:
In `@docs/supported-tools.md`:
- Around line 145-149: Update the shared-tree documentation near the
`.openspec-target` marker description to explain the unmarked-tree rules from
reconcileSharedSkillTargets: an unmarked `.agents/skills` remains
vendor-neutral, but becomes Codex-owned when OpenSpec-managed skills still exist
under `.codex/skills`.
In `@src/core/config.ts`:
- Around line 3-16: Replace the duplicated OPENSPEC_SKILL_NAMES list in
config.ts with a value derived from WORKFLOW_TO_SKILL_DIR in
profile-sync-drift.ts, ensuring hasLegacySkills() automatically recognizes newly
added workflows. Preserve the existing exported constant contract, and use an
import-by-value approach if needed to avoid the config/profile-sync-drift
dependency cycle.
In `@src/core/init.ts`:
- Around line 792-804: Create an exported reportLegacyMigrations helper in
src/core/migration.ts that performs the shared migrateLegacyToolDirs iteration
and migration logging, while supporting the existing this.reportKeptInPlace
behavior required by src/core/update.ts. Replace the duplicated loops at
src/core/init.ts:792-804 and src/core/update.ts:1075-1084 with calls using
projectPath, [tool.value], and 'after-generation'; preserve each caller’s
kept-in-place reporting behavior.
- Around line 603-607: Replace the literal `codex`/`agents` filtering that
produces `reconciledToolIds` with grouping by each tool’s `skillsDir`, then
reuse `reconcileSharedSkillTargets` to select the owner for overlapping roots.
Preserve non-overlapping tools, and emit a one-line notice when `agents` is
requested but Codex owns the shared tree, using the existing logging or notice
mechanism.
In `@src/core/shared-skill-target.ts`:
- Around line 82-92: Update writeSharedSkillTarget so it does not create the
skills directory when no skills tree exists. Before the mkdirSync call, verify
the tool’s skills directory already exists under projectPath and return early
when it does not; preserve the existing marker write behavior for existing
shared skill trees.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e974bb39-86f8-446a-b51f-5b25e9b53708
📒 Files selected for processing (17)
docs/how-commands-work.mddocs/migration-guide.mddocs/supported-tools.mddocs/troubleshooting.mdsrc/core/available-tools.tssrc/core/config.tssrc/core/init.tssrc/core/migration.tssrc/core/profile-sync-drift.tssrc/core/shared-skill-target.tssrc/core/shared/tool-detection.tssrc/core/update.tstest/core/available-tools.test.tstest/core/init.test.tstest/core/profile-sync-drift.test.tstest/core/shared/tool-detection.test.tstest/core/update.test.ts
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/shared/skill-content-equivalence.ts`:
- Around line 8-16: Restrict the normalization logic in the skill-content
equivalence helper to the exact generated invocation and generatedBy locations
instead of replacing matching text throughout the document. Validate generatedBy
against the expected generated-version format, preserving custom invocation
examples and non-version values as differences. Add regression coverage for both
custom invocation text and non-version generatedBy values.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 5271c7f8-771f-4480-b84c-2177753f5884
📒 Files selected for processing (12)
docs/supported-tools.mdsrc/core/migration.tssrc/core/profile-sync-drift.tssrc/core/shared-skill-target.tssrc/core/shared/skill-content-equivalence.tssrc/core/shared/tool-detection.tstest/core/available-tools.test.tstest/core/init.test.tstest/core/migration.test.tstest/core/profile-sync-drift.test.tstest/core/shared/tool-detection.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (5)
- test/core/shared/tool-detection.test.ts
- test/core/migration.test.ts
- docs/supported-tools.md
- src/core/shared/tool-detection.ts
- src/core/migration.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
One blocker at current head 433755d: normalizeGeneratedSkill() rewrites any generatedBy value and matching invocation text anywhere in the document, so a customized legacy skill such as generatedBy: "my-private-fork" can compare equal to the generated replacement and be deleted. Please restrict equivalence to known generated metadata/reference shapes and add regressions showing custom generatedBy values and hand-authored invocation examples remain divergent.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/core/shared/skill-content-equivalence.ts`:
- Around line 3-4: Update GENERATED_VERSION in the version-equivalence logic to
require non-empty prerelease and build identifiers, so malformed values such as
trailing-dot versions are rejected before normalization. Add a regression test
covering malformed version-like generatedBy values and assert that the
comparator returns false, preserving distinct custom values and preventing
classifyManagedFile from treating them as equivalent.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 22c103d8-d9b0-4f8a-bcf0-13f970875785
📒 Files selected for processing (4)
src/core/migration.tssrc/core/profile-sync-drift.tssrc/core/shared/skill-content-equivalence.tstest/core/shared/skill-content-equivalence.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/migration.ts
…skills-path # Conflicts: # src/core/update.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the exact rebased head. The prior deletion blocker is fixed: equivalence now normalizes only valid semver metadata in YAML frontmatter and allowlisted generated OpenSpec dual references, while custom generatedBy values, malformed versions, and hand-authored invocation examples remain divergent and preserved. The canonical .agents/skills ownership, after-generation .codex migration, user-sibling preservation, and symlink/junction containment paths are covered. Fresh build, lint, diff checks, and 364 focused tests pass; the full hosted matrix including Windows and CodeQL is green.
…skills-path # Conflicts: # src/core/available-tools.ts # src/core/config.ts # src/core/init.ts # src/core/migration.ts # src/core/profile-sync-drift.ts # src/core/shared/tool-detection.ts # src/core/update.ts
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
Re-reviewed the exact current head after the MiniMax/global-skills merge. The conflict resolution preserves both sides: Codex shared-root ownership, legacy .codex migration, and custom-file safety remain intact, while global skill targets stay independently detected, scanned, updated, and protected from project-local cleanup. Fresh frozen build, lint, diff checks, and 379 combined Codex/global-skill tests pass; the full hosted Linux/macOS/Windows, security, CodeQL, and CodeRabbit matrix is green.
…ission-AI#1518) * chore(release): add catch-up changeset for Rovo, Codex dir, status Cover three user-facing PRs that merged without changesets so they appear in the v1.8.0 CHANGELOG: - Fission-AI#1516 Atlassian Rovo Dev CLI (new tool) - Fission-AI#1511 Codex skills move to shared .agents directory - Fission-AI#1505 openspec status separates planning from implementation Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * chore(release): correct isPlanningComplete wording in changeset Skipped planning artifacts count as satisfied without being written; say "every non-skipped planning artifact exists" to match the CLI and agent-contract docs (alfred/CodeRabbit review). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
2 participants
Status
LGTM.
What was wrong
openspec initandopenspec updatewrote Codex project skills to the legacy.codex/skillsdirectory. Current Codex discovery uses.agents/skills, so setup could report success while Codex could not find the generated skills. The genericagentstarget onmainestablished.agents/skills, but did not migrate Codex or reconcile repositories that had configured both targets.How it was fixed
.agents/skillsand keep Codex skills-only in every delivery mode.$openspec-*and/openspec-*handoffs when Codex and generic agents share the directory..codexskills. Remove only equivalent generated files while preserving material edits, custom skills, and sibling configuration.agentsskills never authorize that cleanup.Replication / proof
On
main,openspec init --tools codexwrites skills under.codex/skills. A repository that selected bothcodexandagentscan also retain two generated trees with incompatible invocation syntax.With this change, Codex writes one canonical
.agents/skillstree that remains usable by generic agents. Existing unmarked dual-target repositories consolidate automatically; materially edited legacy skills remain in place and are reported. Marker-only commands configurations remain stable across repeated updates.Verification:
main: 126 test files, 3,764 tests passedpnpm run buildpnpm run lintgit diff --checkCloses #1509
Notes / nits
dist/race between tests that launch builds; the final full run used--maxWorkers=1and passed all 3,764 tests.