fix: detect our own hooks by ownership instead of "file would change" - #653
Open
css521 wants to merge 2 commits into
Open
fix: detect our own hooks by ownership instead of "file would change"#653css521 wants to merge 2 commits into
css521 wants to merge 2 commits into
Conversation
Open Island could silently stop receiving hook events while reporting itself as healthy and installed. Reproduced on a machine that also had the closed-source Vibe Island app: `~/.claude/settings.json` contained zero `OpenIslandHooks` entries — the slots held a leftover `vibe-island-bridge` hook — yet Settings showed "Claude hooks installed", the health panel showed no error, and the startup repair never fired. No hook of ours ran, so the island stayed on "no open terminal sessions" with an agent running in the next tab. The cause is one predicate answering two different questions: - ClaudeHookInstaller.isLegacyOpenIslandHookCommand() matches `vibe-island-bridge`, which is correct for "drop this when we write our own hooks" but wrong for "are our hooks installed?". Because `status()` derived managedHooksPresent from that removal pass, a foreign hook read as a successful install, and shouldNAutoInstall()'s `.installed && !present` repair — written for exactly this case — was never reached. - CodexHookInstallationManager.status() had the same shape via `mutation.changed`, which is also true when the file merely re-serializes differently (we write sorted, pretty-printed JSON). Split the two concerns: - Keep isManagedHook() broad, so installing still claims the slots; two islands reacting to every event is not a supported state. - Add isOwnManagedHook() / ownHooksPresent, matching only the command we installed or our hook CLI by name (current and pre-rename), and derive managedHooksPresent from that in both installers. - Add HookHealthReport.Issue.hooksMissing: error severity and auto-repairable, so a config another tool rewrote is now visible and self-healing rather than silent. hooksMissing is reported only when the caller passes expectsInstalledHooks (persisted intent is `.installed`). repairHooksIfNeeded() does not consult the intent store, so reporting it unconditionally would reinstall hooks a user deliberately removed and regress Octane0411#324. Side effect worth noting: a user who only ever had the closed-source app's hooks is now recorded as `.untouched` rather than `.installed` by migrateIntentStoreIfNeeded(), so they get onboarding instead of a silent takeover. Verified locally: scripts/lint-strings.sh and scripts/check-docs.sh pass. swift build / swift test could not complete on this machine — a cold worktree needs to re-clone Sparkle and swift-markdown-ui, and the network here moves ~2 MB in 10 minutes — so both are left to CI. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe change makes hook health checks intent-aware. It distinguishes Open Island-owned hooks from legacy managed hooks and reports missing hooks only when installation is expected. Claude and Codex tests cover foreign hooks, malformed configuration, healthy states, and intentional absence. ChangesHook health and ownership
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant HookInstallationCoordinator
participant intentStore
participant HookHealthCheck
participant HookInstaller
HookInstallationCoordinator->>intentStore: Read persisted agent intent
HookInstallationCoordinator->>HookHealthCheck: Pass hook installation expectation
HookHealthCheck->>HookInstaller: Check Open Island hook ownership
HookInstaller-->>HookHealthCheck: Return ownership state
HookHealthCheck-->>HookInstallationCoordinator: Return health report
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@Sources/OpenIslandCore/CodexHookInstaller.swift`:
- Around line 154-160: Update CodexHookInstaller’s ownership detection by adding
containsOwnManagedHook and isOwnManagedHook, matching the ClaudeHookInstaller
pattern, and use the ownership-specific predicate when setting ownHooksPresent
and deciding mutation. Exclude legacyManagedStatusMessage from ownership checks,
while retaining it only for cleanup if needed. Add a regression test covering
entries with “Managed by Vibe Island” and verify
CodexHookInstallationManager.status() does not report Open Island hooks as
installed.
- Around line 38-54: Make CodexHookFileMutation in
Sources/OpenIslandCore/CodexHookInstaller.swift conform to Codable. Also update
HookHealthReport and every nested model type in
Sources/OpenIslandCore/HookHealthCheck.swift to conform to Codable, preserving
their existing Sendable conformances and behavior.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 25149c66-cc58-4832-b848-83a29356f071
📒 Files selected for processing (7)
Sources/OpenIslandApp/HookInstallationCoordinator.swiftSources/OpenIslandCore/ClaudeHookInstaller.swiftSources/OpenIslandCore/CodexHookInstallationManager.swiftSources/OpenIslandCore/CodexHookInstaller.swiftSources/OpenIslandCore/HookHealthCheck.swiftTests/OpenIslandCoreTests/ClaudeHooksTests.swiftTests/OpenIslandCoreTests/HookHealthCheckTests.swift
Comment on lines
+38
to
+54
| /// Whether hooks of ours were actually found in the file. | ||
| /// | ||
| /// Distinct from ``changed``, which is also true when the file merely | ||
| /// re-serializes differently (we write sorted, pretty-printed JSON). Use | ||
| /// this — never ``changed`` — to answer "are our hooks installed?". | ||
| public var ownHooksPresent: Bool | ||
|
|
||
| public init(contents: Data?, changed: Bool, hasRemainingHooks: Bool) { | ||
| public init( | ||
| contents: Data?, | ||
| changed: Bool, | ||
| hasRemainingHooks: Bool, | ||
| ownHooksPresent: Bool = false | ||
| ) { | ||
| self.contents = contents | ||
| self.changed = changed | ||
| self.hasRemainingHooks = hasRemainingHooks | ||
| self.ownHooksPresent = ownHooksPresent |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Make the modified models Codable.
Sources/OpenIslandCore/CodexHookInstaller.swift#L38-L54: AddCodabletoCodexHookFileMutation.Sources/OpenIslandCore/HookHealthCheck.swift#L27-L29: AddCodabletoHookHealthReportand its nested model types.
As per coding guidelines, “All models must be Sendable and Codable.”
📍 Affects 2 files
Sources/OpenIslandCore/CodexHookInstaller.swift#L38-L54(this comment)Sources/OpenIslandCore/HookHealthCheck.swift#L27-L29
🤖 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 `@Sources/OpenIslandCore/CodexHookInstaller.swift` around lines 38 - 54, Make
CodexHookFileMutation in Sources/OpenIslandCore/CodexHookInstaller.swift conform
to Codable. Also update HookHealthReport and every nested model type in
Sources/OpenIslandCore/HookHealthCheck.swift to conform to Codable, preserving
their existing Sendable conformances and behavior.
Source: Coding guidelines
Comment on lines
+154
to
160
| let hadOwnHooks = containsManagedHook(in: existingGroups, managedCommand: managedCommand) | ||
| if hadOwnHooks { | ||
| ownHooksPresent = true | ||
| } | ||
|
|
||
| if cleanedGroups.count != existingGroups.count || hadOwnHooks { | ||
| mutated = true |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Use an ownership-specific predicate.
Line 154 calls containsManagedHook. That predicate accepts legacyManagedStatusMessage, so a Vibe Island-only Codex configuration sets ownHooksPresent to true.
CodexHookInstallationManager.status() then reports Open Island hooks as installed. Intent migration can suppress the required repair.
Add containsOwnManagedHook and isOwnManagedHook, as in ClaudeHookInstaller. Do not use legacy status messages to establish ownership. Add a regression test with Managed by Vibe Island entries.
🤖 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 `@Sources/OpenIslandCore/CodexHookInstaller.swift` around lines 154 - 160,
Update CodexHookInstaller’s ownership detection by adding containsOwnManagedHook
and isOwnManagedHook, matching the ClaudeHookInstaller pattern, and use the
ownership-specific predicate when setting ownHooksPresent and deciding mutation.
Exclude legacyManagedStatusMessage from ownership checks, while retaining it
only for cleanup if needed. Add a regression test covering entries with “Managed
by Vibe Island” and verify CodexHookInstallationManager.status() does not report
Open Island hooks as installed.
CodeRabbit's docstring coverage check flagged 4 of 11 functions in the previous commit as undocumented. Covers the remaining seven, and records why each new test exists rather than restating its name. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
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
managedHooksPresentfrom actual ownership in both the Claude and Codex installers, instead of from "the removal pass would change the file"HookHealthReport.Issue.hooksMissing(error severity, auto-repairable) so a config another tool rewrote is visible and self-healing rather than silentThe failure
Open Island can stop receiving hook events while reporting itself as installed and healthy.
Reproduced on a machine that also had the closed-source Vibe Island app installed.
~/.claude/settings.jsonheld zeroOpenIslandHooksentries — a leftovervibe-island-bridgehook occupied the slots — and yet:Other hooks coexist: …notice, no errorinstalledNet effect: the island showed "no open terminal sessions" with a Claude Code agent running in the next terminal tab. Feeding one event straight into the bridge by hand produced a correct, fully-attached session row, which isolated the break to hook invocation rather than the bridge,
AppModel, terminal resolution, or persistence.Root cause
One predicate answering two questions.
ClaudeHookInstaller.isLegacyOpenIslandHookCommand()matchesvibe-island-bridge. That is right for "drop this entry when we write our own hooks" — two islands reacting to every event is not a supported state — but wrong for "are our hooks installed?".status()derivedmanagedHooksPresentfrom that removal pass, so a foreign hook counted as ours. That false positive then propagated:claudeHooksInstalledtrue→ Settings shows the hooks as presentshouldAutoInstall(.claudeCode).installed && !present→false, so the repair written for exactly this case never firesHookHealthCheck.checkClaudeCodexHookInstallationManager.status()had the same shape throughmutation.changed, which is additionally true when the file merely re-serializes differently — we write sorted, pretty-printed JSON, so anyhooks.jsonnot already in that exact form read as installed.Approach
isManagedHook()stays broad; installing still claims the slots as before. A newisOwnManagedHook()/ownHooksPresentanswers the ownership question, matching only the command we recorded at install time or our hook CLI by name —OpenIslandHooksand the pre-renameVibeIslandHooks— so an app bundle that moved since install still reads as ours.hooksMissingis reported only when the caller passesexpectsInstalledHooks.repairHooksIfNeeded()does not consult the intent store, so reporting it unconditionally would reinstall hooks a user turned off on purpose and regress #324.Behaviour change worth flagging
A user who only ever had the closed-source app's hooks is now recorded as
.untouchedrather than.installedbymigrateIntentStoreIfNeeded(). They get first-run onboarding instead of a silent takeover, which seems like the better default, but it is a visible change for that group.Not covered here
KimiHookInstaller,GeminiHookInstallerandCursorHookInstallercarry the same broad-predicate shape. They are left alone becauseHookHealthCheckdoes not cover those agents, so there is no reporting path to fix alongside them, and I have no reproduction for them. Worth a follow-up.Verification
Passing locally:
scripts/lint-strings.sh— thelintstep of the CI harnessscripts/check-docs.sh— thedocsstepOpenIslandCoreandOpenIslandCoreTestscompile clean, including both new test files. Six of the seven changed files live inOpenIslandCore, which has no external dependencies, so I built it through a throwawayPackage.swiftcontaining only that target and its tests.Not run locally, both for environment reasons rather than anything about the change:
Testing.framework, which ships with Xcode, and this machine has only CommandLineTools. The bundle builds, thendlopenfails on the framework.swift buildofOpenIslandApp— a cold worktree has to re-clone Sparkle and swift-markdown-ui, and this network moved about 2 MB in ten minutes.HookInstallationCoordinator.swiftis the one changed file this leaves unbuilt.Please treat the harness run as the gate. The run is currently sitting at
action_required, as outside-contributor runs on this repo do, so it needs a maintainer to approve it.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests