Repository navigation
Preserve Codex plugin config during hook setup - #4270
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
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:
📝 WalkthroughWalkthroughCompute codex hook-trust entries to remove, add a regex-based escaped-key extractor, refactor marker removal to preserve unowned lines and apply replacements, and add regression tests ensuring plugin tables and third‑party trust behave correctly during install/uninstall. ChangesCodex Hook Trust Block Removal with Plugin Preservation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 15 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (15 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 998ae9fe6f
ℹ️ 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".
| if codexHookTrustTableEscapedKey(from: lines[index]) != nil { | ||
| index += 1 | ||
| while index < lines.endIndex, !tomlLineIsAnyTableHeader(lines[index]) { | ||
| index += 1 |
There was a problem hiding this comment.
Handle TOML variants when pruning stale trust tables
When removing a stale cmux-codex-hook-trust block, this logic only treats a line as a hook-trust table if codexHookTrustTableEscapedKey matches the exact [hooks.state."..."] shape. Valid TOML headers such as [hooks.state."..."] # comment (or other equivalent formatting) are not matched, so the stale trust table is preserved inside the block and a fresh table is appended later, which can leave duplicate/obsolete trust entries and potentially invalid config. This regression comes from switching from whole-block deletion to selective preservation without broad table-header parsing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed. The trust table parser now accepts valid TOML header spacing and trailing comments, so stale cmux trust tables are pruned before fresh trust entries are appended.
— Claude Code
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 `@tests/test_codex_feed_hooks.py`:
- Around line 1422-1426: The test currently asserts plugin tables appear
strictly before the fresh trust block by comparing
config_toml.index('[plugins."browser@openai-bundled"]') >
config_toml.index(trust_begin), which rejects cases where the plugin is after
the trust block; change this to ensure the plugin is outside the trust block
instead: compute trust_begin_index = config_toml.index(trust_begin) and
trust_end_index = config_toml.index(trust_begin.replace(" begin", " end")) (or
find the corresponding end marker), then assert that the plugin index is either
< trust_begin_index or > trust_end_index (i.e., not between begin and end) so
the plugin table can validly appear after the trust block.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: 8f9b3166-7fc0-4842-9d46-d626ff6e8cd1
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_codex_feed_hooks.py
Greptile SummaryThis PR fixes
Confidence Score: 5/5Safe to merge — the selective trust-block rewrite is well-structured, deferred replacement is correct, and six targeted regression tests cover the failure scenarios that motivated the fix. The TOML rewrite logic correctly identifies cmux-owned trust entries via exact key, key-prefix+hash, and legacy hash sets, and preserves everything else. Deferred replacement (collecting all changes before applying them) avoids partial-mutation on malformed-block returns. Hash parsing in codexHookTrustTrustedHash handles the realistic input range (hex SHA-256 values) without issues. The install and uninstall paths are both updated consistently, and the new tests exercise the edge cases end-to-end. No files require special attention. Important Files Changed
Reviews (13): Last reviewed commit: "Prune legacy Codex trust on install" | Re-trigger Greptile |
| private static func codexHookTrustBlockUnownedLines(from lines: ArraySlice<String>) -> [String] { | ||
| var preserved: [String] = [] | ||
| var index = lines.startIndex | ||
| while index < lines.endIndex { | ||
| if codexHookTrustTableEscapedKey(from: lines[index]) != nil { | ||
| index += 1 | ||
| while index < lines.endIndex, !tomlLineIsAnyTableHeader(lines[index]) { | ||
| index += 1 | ||
| } | ||
| continue | ||
| } | ||
| preserved.append(lines[index]) | ||
| index += 1 | ||
| } | ||
| return ranges.isEmpty ? .notFound : .removed | ||
| return preserved | ||
| } |
There was a problem hiding this comment.
Non-table-header lines before the first
[hooks.state."..."] entry are silently preserved
The outer loop's fall-through path (preserved.append(lines[index])) fires for any line that does not match codexHookTrustTableEscapedKey. If cmux ever writes a bare key-value or comment line before the first trust-table header inside the block (e.g. a future # cmux-version annotation), those lines will be emitted into the surrounding document as "unowned" content. The current format never produces such lines, but there is no guard that makes this assumption explicit. If the format evolves, a version skew between the writer and this reader could silently promote internal lines to permanent document content.
There was a problem hiding this comment.
Addressed. The preservation helper now only preserves full unowned TOML tables and skips loose internal marker content.
— Claude Code
| @@ -20646,14 +20646,32 @@ export default function cmuxPiSessionExtension(pi: ExtensionAPI) { | |||
| guard let endIndex = lines[index...].firstIndex(of: cmuxCodexHookTrustEnd) else { | |||
| return .malformed | |||
| } | |||
| ranges.append(index...endIndex) | |||
| index = endIndex + 1 | |||
| let preservedLines = codexHookTrustBlockUnownedLines( | |||
| from: lines[(index + 1)..<endIndex] | |||
| ) | |||
| lines.replaceSubrange(index...endIndex, with: preservedLines) | |||
| removedBlock = true | |||
| index += preservedLines.count | |||
| } | |||
|
|
|||
| for range in ranges.reversed() { | |||
| lines.removeSubrange(range) | |||
| return removedBlock ? .removed : .notFound | |||
| } | |||
There was a problem hiding this comment.
Inline mutation before a
.malformed return is a silent behavioral change
The old code collected all ranges first and only mutated lines after the loop, meaning a .malformed return always left lines unmodified. The new code mutates lines inline: if a valid first block is processed and a second unclosed block is encountered, the first block's content has already been rewritten before .malformed is returned. Callers that follow .malformed with stripMalformedCmuxCodexHookTrustMarker now receive a partially-updated array. In practice this is an improvement (the first valid block is properly cleaned up), but the contract change is not documented and is not covered by a test with two blocks where the second is malformed.
There was a problem hiding this comment.
Fixed. The trust-block rewrite is back to deferred replacement, so a later malformed block returns without partially mutating the lines.
— Claude Code
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 `@CLI/cmux.swift`:
- Around line 20706-20717: The function codexHookTrustTableEscapedKey currently
compiles the NSRegularExpression on every call; change it to use a cached
compiled regex by declaring a static let (e.g., static let
codexHookTrustTableRegex = try! NSRegularExpression(pattern: ...)) at the type
scope and reuse that instance inside codexHookTrustTableEscapedKey instead of
calling try? NSRegularExpression(pattern:) each time; ensure the exact same
pattern string is used and keep the rest of the matching logic the same.
🪄 Autofix (Beta)
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: ASSERTIVE
Plan: Pro
Run ID: f46738ff-b58a-4181-9e7b-2fcdd637e33e
📒 Files selected for processing (2)
CLI/cmux.swifttests/test_codex_feed_hooks.py
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:20674">
P2: Non-table lines inside the stale trust block are now discarded, which can still drop user TOML content that is not in a table.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: d6079f1ae2
ℹ️ 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".
| private static let cmuxCodexHookTrustEnd = | ||
| "# cmux-codex-hook-trust-f5cc24da-7a09-4b20-a756-89e7786f6738 end" | ||
| private static let codexHookTrustTableHeaderRegex = try! NSRegularExpression( | ||
| pattern: #"^\s*\[\s*hooks\s*\.\s*state\s*\.\s*"((?:[^"\\\n]|\\.)*)"\s*\]\s*(#.*)?$"# |
There was a problem hiding this comment.
Accept literal-quoted hook trust headers during block cleanup
The new trust-block pruning keeps any table whose header does not match codexHookTrustTableHeaderRegex, but that regex only accepts double-quoted keys. TOML also allows literal-quoted keys (e.g. [hooks.state.'...']), so a stale cmux-managed trust table in that valid form will now be preserved inside the old block and a fresh trust table is appended later, leaving duplicate/stale trust state and potentially invalid TOML due table redefinition. Before this commit, the whole marked block was removed, so this regression is introduced by the selective-preservation path.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c7cfa4d24b
ℹ️ 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".
| if !escapedKeys.contains(escapedKey) { | ||
| preserved.append(contentsOf: lines[tableStart..<index]) |
There was a problem hiding this comment.
Drop stale cmux trust tables when rewriting trust marker
codexHookTrustBlockUnownedLines now preserves any [hooks.state."…"] table whose key is not in escapedKeys, but during install/uninstall escapedKeys only contains keys derived from the current hooks layout. If a user has stale cmux-owned trust entries with old indices (for example after hook-group reordering or prior format drift), those tables are treated as unowned here, moved outside the marker, and survive both reinstall and uninstall. This regresses cleanup behavior from full-block removal and leaves orphaned cmux trust state in config.toml.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ea81f1363b
ℹ️ 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".
| private static let cmuxCodexHookTrustEnd = | ||
| "# cmux-codex-hook-trust-f5cc24da-7a09-4b20-a756-89e7786f6738 end" | ||
| private static let codexHookTrustTableHeaderRegex = try! NSRegularExpression( | ||
| pattern: #"^\s*\[\s*hooks\s*\.\s*state\s*\.\s*"((?:[^"\\\n]|\\.)*)"\s*\]\s*(#.*)?$"# |
There was a problem hiding this comment.
Match literal-quoted hooks.state headers when pruning trust block
codexHookTrustTableHeaderRegex only matches [hooks.state."..."], so valid TOML headers like [hooks.state.'...'] are treated as unowned during removeCmuxCodexHookTrustBlock. In an install/uninstall rewrite, stale cmux-managed trust tables in that literal-quoted form are preserved from the old marker and then a fresh trust block is appended, which can leave stale trust state and may re-define the same table key in config.toml.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0b1fb085ce
ℹ️ 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".
| guard value.hasPrefix("\""), value.hasSuffix("\""), value.count >= 2 else { continue } | ||
| return String(value.dropFirst().dropLast()) |
There was a problem hiding this comment.
Parse trusted_hash values with TOML trailing comments
codexHookTrustTrustedHash only recognizes trusted_hash when the trimmed value both starts and ends with ", so valid TOML like trusted_hash = "sha256:..." # comment is treated as missing. In install/uninstall flows that rely on trustedHash + key-prefix to delete stale cmux-managed trust rows with old indices, this causes those stale rows to be preserved and a fresh trust block to be appended, leaving duplicate/orphaned cmux trust state in config.toml.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:20793">
P2: Prefix-based trust cleanup is gated by current hashes, so stale cmux-owned trust entries can be left behind when hashes don’t match (or no entries are currently detected).</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0a68a651d8
ℹ️ 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".
| guard let trustedHash, trustedHashes.contains(trustedHash) else { | ||
| return false | ||
| } | ||
| return escapedKeyPrefixes.contains { escapedKey.hasPrefix($0) } |
There was a problem hiding this comment.
Drop stale cmux trust rows even when hashes changed
codexHookTrustEscapedKeyIsRemoved only applies the hooks.json path-prefix cleanup when the table’s trusted_hash is in the current trustedHashes set. During upgrades where cmux’s hook command/shape changed (so legacy cmux trust rows have the same key prefix but an old hash), those rows are now treated as unowned and preserved when rewriting the marked block, then a fresh trust block is appended. Before this commit the whole marked block was removed, so these stale cmux-managed trust rows did not survive install/uninstall rewrites.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
1 issue found across 2 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="CLI/cmux.swift">
<violation number="1" location="CLI/cmux.swift:20142">
P2: Including synthetic trust entries from `buildHooksDict(for:)` makes uninstall remove trust tables by key for canonical cmux slots, which can delete non-cmux same-file trust entries that reuse those keys.</violation>
</file>
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f118e05ec4
ℹ️ 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".
| guard let trustedHash, trustedHashes.contains(trustedHash) else { | ||
| return false | ||
| } | ||
| return escapedKeyPrefixes.contains { escapedKey.hasPrefix($0) } |
There was a problem hiding this comment.
Remove stale prefixed trust rows even after hash changes
The prefix-based cleanup now bails out unless trusted_hash is in the current trustedHashes set, so legacy cmux-managed trust rows for the same hooks.json path are preserved whenever a prior cmux version produced a different hash. In upgrade/reinstall/uninstall flows this leaves stale cmux trust tables in config.toml and appends a fresh block, regressing the old full-block cleanup behavior.
Useful? React with 👍 / 👎.
| let value = String(trimmed[valueStart...]).trimmingCharacters(in: .whitespaces) | ||
| guard value.hasPrefix("\""), value.hasSuffix("\""), value.count >= 2 else { continue } | ||
| return String(value.dropFirst().dropLast()) |
There was a problem hiding this comment.
Parse trusted_hash values with trailing TOML comments
This parser only accepts values that end directly with ", so valid TOML like trusted_hash = "sha256:..." # comment is treated as missing. When that happens, codexHookTrustEscapedKeyIsRemoved cannot apply prefix-based stale-row deletion, so old cmux trust entries inside the marker can survive and be duplicated after install/uninstall rewrites.
Useful? React with 👍 / 👎.
|
You're iterating quickly on this pull request. To help protect your rate limits, cubic has paused automatic reviews on new pushes for now—when you're ready for another review, comment |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 120d916aa6
ℹ️ 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".
| guard key == "trusted_hash" else { continue } | ||
| let valueStart = trimmed.index(after: equalsIndex) | ||
| let value = String(trimmed[valueStart...]).trimmingCharacters(in: .whitespaces) | ||
| guard value.hasPrefix("\""), value.hasSuffix("\""), value.count >= 2 else { continue } |
There was a problem hiding this comment.
Accept literal-quoted trusted_hash values during trust cleanup
codexHookTrustTrustedHash only recognizes trusted_hash values wrapped in double quotes, but TOML also permits literal strings ('sha256:…'). When a stale cmux-managed trust table inside the marker uses that valid form, this parser returns nil, so codexHookTrustEscapedKeyIsRemoved skips prefix+hash cleanup and the stale row is preserved during install/uninstall rewrites. That can leave orphaned cmux trust state (and duplicate trust blocks after a reinstall) instead of pruning old cmux-owned entries.
Useful? React with 👍 / 👎.
Summary
Testing
documents plugin table was removed../scripts/reload.sh --tag hookplugPYTHONPATH=tests CMUX_CLI_BIN=... python3 - <<'PY' ... test_install_preserves_plugin_tables_inside_stale_cmux_hook_trust_marker ... PYTMPDIR=/tmp CMUX_CLI_BIN=... python3 tests/test_codex_feed_hooks.pyIssues
cmux hooks setupproposed deleting Codex plugin tables from~/.codex/config.toml.Note
Medium Risk
Touches Codex hook install/uninstall logic that rewrites user
config.toml, so mistakes could delete or mis-handle user/plugin configuration despite added regressions. Scope is contained to Codex hook trust parsing/removal paths.Overview
Fixes Codex hook install/uninstall to stop deleting unrelated
config.tomlcontent when stale cmux hook-trust markers have drifted to include other tables/lines.Install now pre-cleans existing hook-trust by removing only cmux-owned trust entries (including legacy hashes) using key-prefix +
trusted_hashmatching, then writes a single fresh trust block; uninstall similarly removes cmux-owned/legacy trust while preserving third-party and user trust.Updates TOML parsing to treat any table header as a boundary (regex-based
[hooks.state."…"]detection) and adds regressions covering plugin-table preservation, dotted-feature capture, uninstall retry cleanup, and legacy trust removal.Reviewed by Cursor Bugbot for commit 087b31e. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by cubic
Fixes Codex hook install/uninstall so
cmux hooks setupstops deleting plugin tables or unrelated config. Install now also prunes legacy Codex trust while preserving user and third‑party trust; uninstall still removes legacy trust and commands.[plugins.*]and loose lines; handle whitespace/comments; cache the header regex."<hooks.json path>:"prefixes whentrusted_hashmatches known cmux or legacy hashes; keep third‑party and same‑file user trust; enable hooks even if dottedfeatures.*lines were captured.:0:0key and third‑party trust.Written for commit 087b31e. Summary will update on new commits. Review in cubic
Summary by CodeRabbit