fix(install): Codex add-only replace; refuse disabled/unknown enablement - #824
Conversation
Native `codex plugin remove` deletes the plugin config subtree, so `agent-bundle install codex` now refreshes through add-only. Capture config.toml around add and restore it when inventory reported enabled=false or when add fails after mutation. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
🦋 Changeset detectedLatest commit: 2ebfcd8 The changes in this PR will be included in the next version bump. This PR includes changesets to release 2 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
…tings The reinstall recipe is add-only; the prose still names plugin remove so operators know why that verb is not in the executable block. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
HOLD merge (owner note)Merge is held at Parent constraints (must land before merge):
Cloud worker owns the surgical revise + PR body rewrite around the corrected final approach. Independent review + proof required before any merge. |
Independent review: FAIL @
|
commit: |
Keep native add-only for enabled Codex replacements so nested MCP overrides survive. Pinned Codex has no settings-preserving update API, so disabled or unknown-enablement replacements fail closed before any host mutation instead of capturing or rewriting config.toml. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
State that the native plugin CLI has no qualified settings-preserving update API. Do not deny app-server config/value/write. Record that the plugin-list enabled snapshot is not atomic against a concurrent edit of the same plugin flag between list and add. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Align install and INSTALL.md assertions with the production AB7004 / Codex reinstall phrase. Co-authored-by: Zack Jackson <ScriptedAlchemy@users.noreply.github.com>
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2ebfcd87ba
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Decided before any host verb runs, so the marketplace ownership check sees the pre-install state. | ||
| const recorded = await receiptIdentity(); | ||
| if (replaced) { | ||
| const replaceRemovesPlugin = replaced && host !== 'codex'; |
There was a problem hiding this comment.
Preserve clean replacements on pinned Codex 0.147
When a supported Codex 0.147.0 user replaces a bundle that removed or renamed a file, this unconditional exclusion skips the only step that cleans the cache. The pre-change native instructions explicitly recorded that plugin add re-copies the snapshot but never deletes files removed by a rebuild, while the repository still pins and supports 0.147.0; the commit's stated real-host proof covers only 0.154. Installation can therefore report replaced and write the new receipt while obsolete skills or hooks remain, and the manifest-based Doctor comparison can ignore files no longer declared. Gate add-only replacement on a proven host version, raise the supported version and pin, or provide another settings-preserving clean replacement.
Useful? React with 👍 / 👎.
After rebasing onto main, the Codex add-only install tests still used bare recursive rm in finally blocks; the remove-tree lint gate correctly failed.
#823) * test(support): add removeTree, a retrying recursive delete for test teardown A late writer into a tree being removed makes rm reject with ENOTEMPTY. One helper retries the transient codes so each call site does not carry its own maxRetries. * test: replace bare recursive rm in tests with removeTree and gate it in lint scripts/check-test-remove-tree.mjs fails pnpm lint on rm(..., { recursive: true }) without maxRetries under packages/*/tests. 1156 call sites across 204 files moved to removeTree. * ci: retrigger checks after unreproducible removeTree lint failure * fix(test): convert leftover recursive teardowns and catch aliased rm After merging main, three install-surface teardowns still used bare recursive rm. Convert them to removeTree, and teach the lint gate to recognize namespace and aliased Node fs removal bindings so those forms cannot escape the gate. * fix(test): make remove-tree lint syntax-aware for comments and $ aliases Mask comments/strings before scanning, escape imported names literally, and read recursive/maxRetries from real options properties so GPT P2 cases pass. * test(support): remove the tmp dir the persistent-ENOTEMPTY test leaves behind * test: declare scripts/check-test-remove-tree.mjs for the gate test's typecheck * fix(test): parse remove-tree lint calls with typescript-5 Replace comment/string masking with an AST walk so options come only from the second argument (including quoted keys), and regex/template edge cases no longer hide or invent recursive rm hits. * fix(test): parse Node fs removal bindings via TypeScript AST Stop forging bare `rm` bindings from text regex so local identifiers and commented-out imports cannot bypass or falsely trip the removeTree lint gate. * test(remove-tree): cover AST binding regressions for GPT P2 Lock commented bare imports, non-fs rm imports, and aliased/namespace maxRetries pass paths so removalBindings stays ImportDeclaration-backed. * fix(test): route #824 install teardown through removeTree After rebasing onto main, the Codex add-only install tests still used bare recursive rm in finally blocks; the remove-tree lint gate correctly failed. * ci: retrigger checks after cancelled CI attempts on removeTree PR
Codex replacement currently runs
plugin removebeforeplugin add. Remove deletes the plugin's settings subtree, so an enabled plugin with a disabled nested MCP server loses that override during an upgrade.Replacement now uses native
plugin addwithout removal, preserving nested settings and refreshing the cached artifact. Disabled or unknown-enablement replacements refuse before mutation (AB7004), since native add enables the plugin and the plugin CLI offers no qualified way to preserve its disabled state. Failed add/receipt recovery also avoids removing an existing Codex plugin. Claude, fresh installs and explicit uninstall retain their existing behavior.The installer does not write or restore
config.toml. Native inventory is a snapshot: a concurrent edit to this same plugin's enabled flag between list and add is not atomically protected. Unrelated configuration is never rewound.Validation: independent exact-head review PASS+NOTES at
2ebfcd87ba803a4a6b2f80288ebee809d4ae41e8; isolated actual Codex 0.154 add-only refresh/removal-of-obsolete-files/nested-override proof; enabled/disabled/unknown and add/receipt-failure regression tests; native CI, host-install and Ubuntu/macOS/Windows filesystem checks, build/types/release gates and EN/ZH documentation checks. A patch changeset is included.