fix(update): don't hijack the agents target on legacy Codex upgrade - #1522
fix(update): don't hijack the agents target on legacy Codex upgrade#1522clay-good wants to merge 2 commits into
Conversation
Codex and the vendor-neutral `agents` target share `.agents/skills`. In upgradeLegacyTools, a Codex install inferred only from global ~/.codex/prompts wrote Codex skills into `.agents` and flipped the ownership marker agents -> codex, silently rewriting an existing agents-owned tree. The main generation path reconciles shared-target ownership first; this legacy-upgrade path did not. Add sharedSkillRootOwnedByOther() and skip generation when a different tool already owns the shared root (marker or existing tree), while still allowing a genuine first-time Codex upgrade with no `.agents` yet. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
📝 WalkthroughWalkthroughThe change adds shared skill-root ownership detection. Legacy tool upgrades now skip generation when another tool owns the shared ChangesShared skill ownership protection
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant LegacyUpgrade
participant SharedSkillRootOwner
participant SkillGenerator
LegacyUpgrade->>SharedSkillRootOwner: Check shared-root ownership
SharedSkillRootOwner-->>LegacyUpgrade: Return owner or undefined
LegacyUpgrade->>SkillGenerator: Generate skills when no other owner exists
LegacyUpgrade-->>LegacyUpgrade: Skip generation when another tool owns root
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 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 |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The ownership predicate looks right and the focused suite passes, but the regression only tests the helper. Please add an UpdateCommand test for the actual legacy-upgrade path: existing agents-owned .agents/skills plus global legacy Codex prompts, then non-interactive update --force, asserting the generic files and agents marker remain unchanged. Add the inverse no-root case proving first-time Codex generation still writes the codex marker. This control-flow boundary is the bug.
Harden the agents-target ownership fix after a multi-agent review:
- Add an integration test that runs the real update flow for the bug
scenario (agents-owned .agents + a legacy global Codex prompt) and asserts
the marker stays `agents` and skills keep generic `/openspec-` syntax. A
unit test of the predicate can't catch a future refactor that stops calling
it; this can.
- Name the owning tool in the skip message ("...managed by another tool
(Shared .agents skills)") via a new sharedSkillRootOwner() helper that
sharedSkillRootOwnedByOther now delegates to.
- Add a unit case for the ambiguous-tree branch (existing skills, no marker,
no inferable syntax) and one asserting sharedSkillRootOwner names agents.
- Document the known, harmless re-offer tradeoff (a skipped tool isn't
recorded as configured, so a persistent legacy prompt re-offers it).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/core/update.test.ts (1)
616-639: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winAssert that the skipped Codex tool is not persisted.
The test proves that the
.agents/skillstree is not rewritten. It does not prove thatcodexis absent from the persisted configured-tool set. Add an assertion against the persisted configuration or the existinggetConfiguredToolsForProfileSync(testDir)helper. Otherwise, a regression can preserve the marker and syntax while recording Codex for later updates. The PR objective requires both generation and configuration to be skipped.🤖 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 `@test/core/update.test.ts` around lines 616 - 639, Extend the test around UpdateCommand.execute so it also verifies Codex is absent from the persisted configured-tool set, using getConfiguredToolsForProfileSync(testDir) or the existing persisted configuration access. Keep the current marker and syntax assertions, and assert the configured tools do not include codex.
🤖 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 `@test/core/shared-skill-target.test.ts`:
- Around line 78-79: Extend the test for the ambiguous shared-skill tree to
assert the inferred owner directly by checking that
sharedSkillRootOwner(projectPath, 'codex') returns 'agents', while retaining the
existing sharedSkillRootOwnedByOther boolean assertion.
---
Nitpick comments:
In `@test/core/update.test.ts`:
- Around line 616-639: Extend the test around UpdateCommand.execute so it also
verifies Codex is absent from the persisted configured-tool set, using
getConfiguredToolsForProfileSync(testDir) or the existing persisted
configuration access. Keep the current marker and syntax assertions, and assert
the configured tools do not include codex.
🪄 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: 73b189f0-86f4-46d0-ab3f-4ca601d57059
📒 Files selected for processing (4)
src/core/shared-skill-target.tssrc/core/update.tstest/core/shared-skill-target.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/update.ts
| expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); | ||
| }); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Assert the inferred owner, not only the boolean.
Line [78] verifies only that a non-codex owner exists. It does not verify that the ambiguous tree resolves to agents, although the test name and comment claim that result. Add an exact sharedSkillRootOwner(projectPath, 'codex') assertion.
Proposed assertion
+ expect(sharedSkillRootOwner(projectPath, 'codex')).toBe('agents');
expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); | |
| }); | |
| expect(sharedSkillRootOwner(projectPath, 'codex')).toBe('agents'); | |
| expect(sharedSkillRootOwnedByOther(projectPath, 'codex')).toBe(true); | |
| }); |
🤖 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 `@test/core/shared-skill-target.test.ts` around lines 78 - 79, Extend the test
for the ambiguous shared-skill tree to assert the inferred owner directly by
checking that sharedSkillRootOwner(projectPath, 'codex') returns 'agents', while
retaining the existing sharedSkillRootOwnedByOther boolean assertion.
2 participants
Status
LGTM — surgical fix for an init/update interaction found by an adversarial swarm on post-v1.8.0 main. Not a release blocker for the published v1.8.0 artifact (latent, opt-in path); good fast-follow.
What was wrong
Codex (#1511) and the vendor-neutral
agentstarget (#1303) both write to.agents/skills. The main generation path reconciles which tool owns a shared root before writing, butupgradeLegacyToolsinupdate.tsdid not.So for a project that owns the
agentstarget (.agents/skills/.openspec-target == "agents", generic/openspec-skills) whose user also has leftover global~/.codex/prompts/*from an old Codex install: onopenspec update --force(or any non-interactive run), Codex is auto-selected as an unconfigured legacy tool, and the loop wrote Codex$openspec-skills into.agents/skillsand calledwriteSharedSkillTarget(projectPath, 'codex')— silently rewriting the generic tree with Codex-specific syntax and flipping the ownership markeragents → codex. Future updates then suppress theagentstarget entirely.How it was fixed
New
sharedSkillRootOwnedByOther(projectPath, toolId)inshared-skill-target.tsreturns true only when the shared root already carries an ownership signal (a marker or an existing generated tree) and reconciliation resolves it to a different tool.upgradeLegacyToolsconsults it and skips skill generation for that tool, leaving the established owner in place — mirroring the one-writer ruleinitapplies when both targets are selected.Crucially, an empty/unclaimed root returns false, so a genuine first-time Codex upgrade (no
.agentsyet) still creates its skills — no over-correction.Replication / proof (live CLI on this branch)
Bug scenario —
agentstarget + global~/.codex/prompts, thenupdate --force:Counter-check — Codex-only user, no
.agentsyet, thenupdate --force:New
test/core/shared-skill-target.test.tscovers the predicate (marker-owned, inferred-owned, codex-owned, empty root, non-shared tool). 285 tests pass across update/init/migration/detection suites.Notes
Companion PR (scenario-loss parity) fixes the other, unrelated defect from the same swarm.
🤖 Generated with Claude Code
Summary by CodeRabbit
.agentsskill trees and ownership markers during Codex upgrades..agentstree continue to work as expected.