fix: write hooks to hooks.json#201
Open
benym wants to merge 44 commits into
Open
Conversation
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
|
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:
📝 WalkthroughWalkthroughThis release moves Codex hooks to ChangesCodex hook migration
Comet Open workflow
Superpowers artifact routing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant User
participant CometOpen
participant OpenSpec
participant ArtifactFiles
User->>CometOpen: start open workflow
CometOpen->>OpenSpec: query artifact status
OpenSpec-->>CometOpen: return ready or blocked artifacts
CometOpen->>OpenSpec: fetch artifact instructions
CometOpen->>ArtifactFiles: write and validate outputs
CometOpen->>OpenSpec: recheck completion
CometOpen-->>User: pause after complete integrity checks
sequenceDiagram
participant WriteTool
participant ClassicHookGuard
participant ClassicState
WriteTool->>ClassicHookGuard: submit Superpowers artifact write
ClassicHookGuard->>ClassicState: read projected Classic state
ClassicHookGuard->>ClassicHookGuard: match artifact slot and phase
ClassicHookGuard-->>WriteTool: allow write or return blocked diagnostics
Possibly related PRs
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 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 |
benym
changed the title
Greptile SummaryThis PR fixes Codex hook installation and cleanup. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (9): Last reviewed commit: "test: update classic contract projection" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Reviewer's GuideCodex phase-guard hooks are now written to .codex/hooks.json instead of settings.local.json, with robust migration and uninstall logic that only touches Comet-managed handlers, hardened command parsing for quoted Windows paths, bundle planning updated to use the new hook location, and release/test metadata bumped for 0.4.0-beta.5. Sequence diagram for Codex hook install and migrationsequenceDiagram
actor User
participant comet_cli
participant installCometHooksForPlatform
participant installClaudeCodeHooks
participant removeManagedHooksFromJsonFile
User ->> comet_cli: run init_or_update
comet_cli ->> installCometHooksForPlatform: installCometHooksForPlatform
installCometHooksForPlatform ->> installClaudeCodeHooks: installClaudeCodeHooks(baseDir, platformBase, skillsDir, hooksConfig, hookConfigFile, strictJson)
installClaudeCodeHooks ->> CodexHooksJson: write hooks to .codex/hooks.json
installClaudeCodeHooks -->> installCometHooksForPlatform: { installed: true }
installCometHooksForPlatform ->> removeManagedHooksFromJsonFile: removeManagedHooksFromJsonFile(platformBase/settings.local.json, Object.keys(hooksConfig))
removeManagedHooksFromJsonFile ->> LegacySettingsLocalJson: remove Comet managed handlers only
removeManagedHooksFromJsonFile -->> installCometHooksForPlatform: { removed, failed }
installCometHooksForPlatform -->> comet_cli: { installed, reason? }
Sequence diagram for Codex hook uninstall across canonical and legacy filessequenceDiagram
actor User
participant comet_cli
participant removeCometHooksForPlatform
participant removeManagedHooksFromJsonFile
User ->> comet_cli: run uninstall
comet_cli ->> removeCometHooksForPlatform: removeCometHooksForPlatform
removeCometHooksForPlatform ->> removeManagedHooksFromJsonFile: removeManagedHooksFromJsonFile(.codex/hooks.json, scriptRelPaths)
removeCometHooksFromJsonFile -->> removeCometHooksForPlatform: { removed, failed }
removeCometHooksForPlatform ->> removeManagedHooksFromJsonFile: removeManagedHooksFromJsonFile(settings.local.json, scriptRelPaths)
removeManagedHooksFromJsonFile -->> removeCometHooksForPlatform: { removed, failed }
removeCometHooksForPlatform -->> comet_cli: { removed, failed }
Flow diagram for bundle hook destination selectionflowchart TD
A[Platform.hookFormat == claude-code] --> B{platform.hookConfigFile set?}
B -->|yes| C[hookDestination = configRoot/platform.hookConfigFile]
B -->|no| D[hookDestination = configRoot/settings.local.json]
A --> E[Other hook formats use their existing defaults]
File-Level Changes
Assessment against linked issues
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@assets/skills/comet-open/SKILL.md`:
- Around line 76-79: Update the `comet state check` invocation in the command
examples to use the established `node "$COMET_STATE"` wrapper, matching the
other state-command invocations while preserving the existing `<name> design`
arguments.
In `@docs/superpowers/plans/2026-07-13-standard-superpowers-artifact-routing.md`:
- Line 453: Correct the truncated variable assignment in the
blockedUnmatchedSuperpowersArtifact example by changing the right-hand-side
identifier from slo to slot, preserving the surrounding code unchanged.
- Around line 579-593: Correct the truncated verification commands in the
documented steps: update “pnpm lin” and “pnpm tes” to the complete lint and test
commands, and update “git status --shor” to “git status --short.”
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: ef0c97a2-8dca-4a8f-8967-212e76c9de04
📒 Files selected for processing (9)
CHANGELOG.mdassets/skills-zh/comet-open/SKILL.mdassets/skills/comet-open/SKILL.mddocs/superpowers/plans/2026-07-13-standard-superpowers-artifact-routing.mddocs/superpowers/specs/2026-07-13-standard-superpowers-artifact-routing-design.mdtest/domains/skill/comet-open-batch-completion.test.tstest/domains/skill/comet-open-english-batch-completion.test.tstest/domains/skill/comet-open-recovery-semantics.test.tstest/domains/skill/skills.test.ts
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/app/uninstall.test.ts (1)
426-456: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove this
removeCometHooksForPlatformblock out oftest/app/uninstall.test.ts
This section exercises domain cleanup logic directly, so it belongs intest/domains/skill/. Keeptest/app/focused onapp/command and CLI behavior; the lateruninstallCommandtests already cover that path.🤖 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/app/uninstall.test.ts` around lines 426 - 456, Move the tests that directly exercise removeCometHooksForPlatform, including the malformed and unreadable historical Codex hook cases, from test/app/uninstall.test.ts into the appropriate test/domains/skill/ test file. Preserve their setup, assertions, and coverage there, while keeping test/app/ focused on uninstall command and CLI behavior.Source: Coding guidelines
🤖 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.
Nitpick comments:
In `@test/app/uninstall.test.ts`:
- Around line 426-456: Move the tests that directly exercise
removeCometHooksForPlatform, including the malformed and unreadable historical
Codex hook cases, from test/app/uninstall.test.ts into the appropriate
test/domains/skill/ test file. Preserve their setup, assertions, and coverage
there, while keeping test/app/ focused on uninstall command and CLI behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: e0072db9-1ded-4f01-92c9-0cb0fef82e35
📒 Files selected for processing (3)
domains/skill/platform-install.tsdomains/skill/uninstall.tstest/app/uninstall.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- domains/skill/uninstall.ts
- domains/skill/platform-install.ts
There was a problem hiding this comment.
🧹 Nitpick comments (2)
domains/skill/platform-install.ts (2)
975-976: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRedundant reassignment of
settings.hooks.
existingHooksis alreadysettings.hooks(same reference from line 951), so line 976 is a no-op. Removing it would reduce confusion about whether a new object is being assigned.♻️ Optional cleanup
existingHooks.PreToolUse = filtered; - settings.hooks = existingHooks; try {🤖 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 `@domains/skill/platform-install.ts` around lines 975 - 976, Remove the redundant settings.hooks reassignment after updating existingHooks in the hook configuration flow. Keep existingHooks.PreToolUse = filtered unchanged, since existingHooks already references settings.hooks.
958-972: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueEmpty matcher groups are left in place after all managed hooks are removed.
When a group contains only managed hooks, after filtering it becomes
{ matcher: '...', hooks: [] }. The empty group (and an emptyPreToolUsearray if all groups are empty) persists in the written file. This is likely intentional to preserve structure, but if cleanup of empty groups is desired, it could be added after the filter pass.🤖 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 `@domains/skill/platform-install.ts` around lines 958 - 972, The filtering logic leaves matcher groups with no remaining hooks in the persisted configuration. Update the cleanup around the filtered existingPreToolUse groups to remove groups whose resulting hooks array is empty, and ensure the enclosing PreToolUse array is omitted or removed when all groups are eliminated while preserving groups containing unmanaged hooks.
🤖 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.
Nitpick comments:
In `@domains/skill/platform-install.ts`:
- Around line 975-976: Remove the redundant settings.hooks reassignment after
updating existingHooks in the hook configuration flow. Keep
existingHooks.PreToolUse = filtered unchanged, since existingHooks already
references settings.hooks.
- Around line 958-972: The filtering logic leaves matcher groups with no
remaining hooks in the persisted configuration. Update the cleanup around the
filtered existingPreToolUse groups to remove groups whose resulting hooks array
is empty, and ensure the enclosing PreToolUse array is omitted or removed when
all groups are eliminated while preserving groups containing unmanaged hooks.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7adfda10-7148-4992-a029-59b3f8c066e2
📒 Files selected for processing (3)
domains/skill/platform-install.tsdomains/skill/uninstall.tstest/app/uninstall.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- domains/skill/uninstall.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
.codex/hooks.jsonfile.codex/settings.local.jsonfile while preserving user hooks, unknown fields, and matcher groups0.4.0-beta.5Root cause
Codex reused the Claude-shaped hook format and therefore inherited Claude Code's default
settings.local.jsondestination. Codex loads hooks fromhooks.json, so globally or locally installed Comet guards were written to a file Codex did not read.Impact
Existing Codex installs are migrated on init/update without overwriting invalid JSON or unrelated user configuration. Uninstall cleans both canonical and historical files, and failures in one file no longer prevent cleanup of the others.
Closes #199.
Validation
npx vitest run: 116 files passed, 1312 passed, 12 skippedpnpm lintpnpm buildgit diff --checkSummary by Sourcery
Fix Codex phase-guard hook installation and uninstall to use the canonical .codex/hooks.json file, safely migrate and clean up Comet-managed hooks from historical settings.local.json configurations, and update bundle planning, tests, and release metadata for version 0.4.0-beta.5.
Bug Fixes:
Enhancements:
Documentation:
Tests:
Chores:
Summary by CodeRabbit
.codex/hooks.json, with platform-aware hook config support.settings.local.jsonwhile preserving user-defined hooks./comet-opennow enforces stricter split validation, recovery, and gating to prevent partially initialized outputs.