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 5 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>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe change adds shared skill-root ownership detection. Legacy upgrades skip conflicting tools and preserve their legacy artifacts. Tests and documentation cover ownership, Codex upgrades, and cleanup behavior. ChangesShared skill ownership protection
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant LegacyUpgrade
participant SharedSkillRootOwner
participant SkillGenerator
participant LegacyCleanup
LegacyUpgrade->>SharedSkillRootOwner: Check shared-root ownership
SharedSkillRootOwner-->>LegacyUpgrade: Return owner or undefined
LegacyUpgrade->>SkillGenerator: Generate skills when no other owner exists
LegacyUpgrade->>LegacyCleanup: Pass skippedSharedSkillTools
LegacyCleanup-->>LegacyUpgrade: Preserve skipped tools' legacy artifacts
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
…ests Adversarial review of the shared-root ownership guard surfaced one real integration defect and the review asks from alfred/CodeRabbit. Defect: when the guard skips a legacy Codex upgrade because the `.agents` root is owned by another tool, the caller's immediate legacy cleanup still deleted Codex's repo-local `.codex/prompts/openspec-*.md`. That violates the cleanup contract (remove X only because replacement Y was written): no replacement is written for a skipped tool, so its legacy files must stay. `upgradeLegacyTools` now reports `skippedSharedSkillTools`, and `performImmediateLegacyCleanup` exempts those tools' repo-local artifacts via a new `omitToolLegacyArtifacts` helper. Refactored the per-artifact tool matching out of `getToolsFromLegacyArtifacts` so both share one matcher. Tests (addressing the review + the defect): - update.test.ts: hijack test now asserts Codex is absent from the persisted configured-tool set and that the skip names the established owner. - update.test.ts: inverse no-root case proves a first-time Codex upgrade still writes the `codex` marker via the real UpdateCommand path. - update.test.ts: a skipped tool's repo-local `.codex/prompts` is preserved. - legacy-cleanup.test.ts: unit coverage for omitToolLegacyArtifacts. - shared-skill-target.test.ts: assert sharedSkillRootOwner resolves 'agents'. Docs + changeset updated to describe the preserve-on-skip behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Deploying openspec-docs with
|
| Latest commit: |
f8ea958
|
| Status: | ✅ Deploy successful! |
| Preview URL: | https://3003a301.openspec-docs.pages.dev |
| Branch Preview URL: | https://fix-legacy-upgrade-agents-ow.openspec-docs.pages.dev |
|
Pushed @alfred-openspec — added the two
@coderabbitai — the hijack test now also asserts Codex is absent from the persisted configured-tool set ( New defect fixed: when the guard skips a legacy upgrade, the caller's immediate cleanup was still deleting that tool's repo-local |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
git show --stat --oneline f8ea958
git show --format=fuller --find-renames --find-copies -- src/core/update.ts src/core/legacy-cleanup.ts src/core/shared-skill-target.ts test/core/update.test.ts test/core/legacy-cleanup.test.ts test/core/shared-skill-target.test.ts f8ea958
echo '--- source outlines ---'
for f in src/core/update.ts src/core/legacy-cleanup.ts src/core/shared-skill-target.ts; do
if [ -f "$f" ]; then
ast-grep outline "$f" --items all
fi
done
echo '--- relevant definitions and call sites ---'
rg -n -C 8 \
'upgradeLegacyTools|performImmediateLegacyCleanup|skippedSharedSkillTools|omitToolLegacyArtifacts|getConfiguredToolsForProfileSync|sharedSkillRootOwner' \
src testLength of output: 63614
The added tests cover:
The static inspection found no remaining issue in the reported flow. I did not execute the test suite in this environment. ✏️ Learnings added
You are interacting with an AI system. |
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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/update.ts`:
- Around line 896-900: Preserve skipped-tool ownership through deferred global
cleanup: update the deferred cleanup calls in src/core/update.ts lines 896-900
and 933-937 to pass skippedSharedSkillTools, and update
deferredGlobalCleanup/getLegacyGlobalPromptMatches so matching removable prompts
are excluded for skipped tool IDs. Add coverage for a skipped Codex upgrade
owned by an agents shared root with legacy global prompts; both sites require
the same change.
In `@test/core/legacy-cleanup.test.ts`:
- Around line 1497-1518: Extend the legacy cleanup tests around
omitToolLegacyArtifacts with a backslash-delimited
'.codex\prompts\openspec-explore.md' path and verify it is omitted when skipping
'codex'. Ensure the assertion also confirms hasLegacyArtifacts is recomputed
correctly after removing the Windows-style artifact.
In `@test/core/update.test.ts`:
- Around line 635-639: Wrap the UpdateCommand execution in the test around await
new UpdateCommand({ force: true }).execute(testDir) with a try/finally block,
and move both stdoutSpy.mockRestore() and stderrSpy.mockRestore() into finally
so the process stream spies are always restored when the command throws.
- Around line 699-704: Update the preservation assertion in the test around the
existing FileSystemUtils.fileExists check to read
legacyPrompts/openspec-explore.md and compare its contents with the expected
legacy repo-local prompt, rather than asserting existence alone. Keep the
existing .openspec-target content assertion unchanged.
🪄 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: ae37773a-59b4-4b98-bf84-b8fdcf1fc522
📒 Files selected for processing (7)
.changeset/fix-legacy-upgrade-agents-ownership.mddocs/supported-tools.mdsrc/core/legacy-cleanup.tssrc/core/update.tstest/core/legacy-cleanup.test.tstest/core/shared-skill-target.test.tstest/core/update.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- test/core/shared-skill-target.test.ts
- .changeset/fix-legacy-upgrade-agents-ownership.md
| await this.performImmediateLegacyCleanup( | ||
| projectPath, | ||
| detection, | ||
| legacyUpgrade.skippedSharedSkillTools | ||
| ); |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Preserve deferred global prompts for skipped tools.
Both paths pass skippedSharedSkillTools to immediate cleanup only. They retain the unfiltered detection for deferred global cleanup. A skipped Codex upgrade can then remove matching global Codex prompts even though the existing .agents skills belong to agents and no Codex replacement was generated.
Pass the skipped tool IDs to deferred cleanup. Filter getLegacyGlobalPromptMatches(detection) before it selects removable prompts. Add coverage for a Codex upgrade skipped by an agents-owned shared root with legacy global prompts.
src/core/update.ts#L896-L900: retain skipped-tool ownership state withdeferredGlobalCleanup.src/core/update.ts#L933-L937: apply the same deferred-cleanup exclusion in the interactive path.
📍 Affects 1 file
src/core/update.ts#L896-L900(this comment)src/core/update.ts#L933-L937
🤖 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/update.ts` around lines 896 - 900, Preserve skipped-tool ownership
through deferred global cleanup: update the deferred cleanup calls in
src/core/update.ts lines 896-900 and 933-937 to pass skippedSharedSkillTools,
and update deferredGlobalCleanup/getLegacyGlobalPromptMatches so matching
removable prompts are excluded for skipped tool IDs. Add coverage for a skipped
Codex upgrade owned by an agents shared root with legacy global prompts; both
sites require the same change.
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The follow-up covers the real update --force path and the inverse first-time Codex case, and skipped-tool cleanup now preserves legacy prompts when no replacement was written. Fresh install, build, lint, focused and full test suites, and CodeQL pass. Approved.
|
Re-triggering CI/Security: their workflow dispatches were dropped during the GitHub Actions major outage on 2026-08-06, so they never ran for f8ea958. Reopening to re-fire the pull_request checks on the same commit (no code change). |
…rade Address the outstanding CodeRabbit review notes on #1522. The fix itself is confirmed correct by three independent adversarial reviews — these are test-only hardening that locks in the guarantees the fix promises: - Assert the global ~/.codex/prompts survives (byte-for-byte) in the hijack scenario. Previously the test set the prompt up but never checked it was preserved; on unfixed code Codex would be generated, its 'explore' workflow would read as installed, and the deferred global cleanup would delete the prompt — so this assertion fails without the fix. - Assert the repo-local .codex/prompts is preserved by content, not mere existence (distinguishes 'left untouched' from 'deleted+rewritten'). - Restore the stdout/stderr spies in a finally so a throw can't swallow output for the rest of the suite. - Cover backslash-delimited (Windows) paths in omitToolLegacyArtifacts. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Follow-up: test hardening for the remaining CodeRabbit notes (commit 3bc0cc1)Ran three independent adversarial reviews (data-loss, correctness/edge, tests+docs) over the diff. Consensus: the fix is correct — no source change needed. CodeRabbit's data-loss hypothesis (deferred global cleanup deleting This push is test-only and locks in the guarantees the fix already delivers:
All checks green across linux/macOS/windows; CodeRabbit review completed. The 2 long-standing environment-only local failures ( |
alfred-openspec
left a comment
alfred-openspec
left a comment
There was a problem hiding this comment.
The follow-up is test-only hardening that locks in byte-for-byte global and repo-local prompt preservation, restores stream spies safely, and covers Windows-style paths. Fresh frozen install, build, lint, all 239 focused tests, and the exact-head hosted matrix pass. Approved.
2 participants
Status
LGTM — surgical fix for an init/update interaction found by an adversarial swarm on post-v1.8.0 main, now hardened by a second adversarial-review pass. Not a release blocker for the published v1.8.0 artifact (latent, opt-in path); good fast-follow. No separate tracking issue — the interaction is between two merged PRs, #1511 (Codex →
.agents) and #1303 (vendor-neutralagentstarget); both are referenced below.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.Second defect (found in adversarial review of the fix): even once skill generation was skipped, the caller's immediate legacy cleanup still deleted the skipped tool's repo-local
.codex/prompts/openspec-*.md. That violates the cleanup contract — files are removed because a replacement was written — leaving the tool with no OpenSpec integration and destroying the legacy signal the skip relies on to re-offer later.How it was fixed
sharedSkillRootOwner(projectPath, toolId)/sharedSkillRootOwnedByOther(...)inshared-skill-target.tsreturn the established owner 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. An empty/unclaimed root returns undefined, so a genuine first-time Codex upgrade still creates its skills.upgradeLegacyToolsnow reportsskippedSharedSkillTools, andperformImmediateLegacyCleanupexempts those tools' repo-local artifacts via a newomitToolLegacyArtifactshelper — so a skipped tool's.codex/promptsis preserved, not deleted with nothing put in its place.Replication / proof (live CLI on this branch)
Bug scenario —
agentstarget + global~/.codex/prompts, thenupdate --force:Counter-check — Codex-only user, no
.agentsyet, thenupdate --force:Repo-local preservation —
agentstarget + repo-local.codex/prompts/openspec-*.md, thenupdate --force: the agents marker is unchanged and the.codex/promptsfile survives (previously deleted).Tests
test/core/shared-skill-target.test.ts— the predicate (marker-owned, inferred-owned, codex-owned, empty root, non-shared tool, ambiguous-tree owner resolution).test/core/update.test.ts— threeUpdateCommandend-to-end cases: (a) hijack blocked + Codex absent from the persisted configured-tool set + the skip names the owner; (b) inverse no-root first-time Codex upgrade still writes thecodexmarker; (c) a skipped tool's repo-local.codex/promptsis preserved.test/core/legacy-cleanup.test.ts— unit coverage foromitToolLegacyArtifacts.3860 tests pass locally (the 2 failures are pre-existing, environment-only, and pass on CI).
Review responses
UpdateCommandtest for the actual legacy-upgrade path + the inverse no-root case): both added — see (a) and (b) above; the boundary is now exercised through the real command, not just the helper.sharedSkillRootOwnerresolves the owner): both added.Notes
Companion PR (scenario-loss parity) fixes the other, unrelated defect from the same swarm.
🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
.agentsskill trees and ownership markers during Codex upgrades..agentstree to proceed normally.Documentation
.agentsownership handling and explicit Codex initialization requirements.