From bae9b969fe23d2ab1c4fa1057b1bf5559cf2027d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BC=98=E8=83=9C?= Date: Wed, 12 Aug 2026 11:10:09 +0800 Subject: [PATCH 1/2] fix: detect our own hooks by ownership instead of "file would change" MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 #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) --- .../HookInstallationCoordinator.swift | 40 ++++- .../OpenIslandCore/ClaudeHookInstaller.swift | 52 +++++- .../CodexHookInstallationManager.swift | 2 +- .../OpenIslandCore/CodexHookInstaller.swift | 36 +++- Sources/OpenIslandCore/HookHealthCheck.swift | 37 +++- .../ClaudeHooksTests.swift | 58 +++++++ .../HookHealthCheckTests.swift | 162 ++++++++++++++++++ 7 files changed, 368 insertions(+), 19 deletions(-) create mode 100644 Tests/OpenIslandCoreTests/HookHealthCheckTests.swift diff --git a/Sources/OpenIslandApp/HookInstallationCoordinator.swift b/Sources/OpenIslandApp/HookInstallationCoordinator.swift index f70f0cd94..0d962ac78 100644 --- a/Sources/OpenIslandApp/HookInstallationCoordinator.swift +++ b/Sources/OpenIslandApp/HookInstallationCoordinator.swift @@ -472,9 +472,17 @@ final class HookInstallationCoordinator { guard let self else { return } let binaryURL = self.hooksBinaryURL + let expectsClaude = self.expectsInstalledHooks(.claudeCode) + let expectsCodex = self.expectsInstalledHooks(.codex) let (claudeReport, codexReport, openCodeReport) = await Task.detached(priority: .utility) { - let claude = HookHealthCheck.checkClaude(hooksBinaryURL: binaryURL) - let codex = HookHealthCheck.checkCodex(hooksBinaryURL: binaryURL) + let claude = HookHealthCheck.checkClaude( + hooksBinaryURL: binaryURL, + expectsInstalledHooks: expectsClaude + ) + let codex = HookHealthCheck.checkCodex( + hooksBinaryURL: binaryURL, + expectsInstalledHooks: expectsCodex + ) let openCode = HookHealthCheck.checkOpenCode() return (claude, codex, openCode) }.value @@ -500,9 +508,17 @@ final class HookInstallationCoordinator { // Re-run health checks first let binaryURL = hooksBinaryURL + let expectsClaude = expectsInstalledHooks(.claudeCode) + let expectsCodex = expectsInstalledHooks(.codex) let (claudeReport, codexReport, openCodeReport) = await Task.detached(priority: .utility) { - let claude = HookHealthCheck.checkClaude(hooksBinaryURL: binaryURL) - let codex = HookHealthCheck.checkCodex(hooksBinaryURL: binaryURL) + let claude = HookHealthCheck.checkClaude( + hooksBinaryURL: binaryURL, + expectsInstalledHooks: expectsClaude + ) + let codex = HookHealthCheck.checkCodex( + hooksBinaryURL: binaryURL, + expectsInstalledHooks: expectsCodex + ) let openCode = HookHealthCheck.checkOpenCode() return (claude, codex, openCode) }.value @@ -536,8 +552,14 @@ final class HookInstallationCoordinator { if repaired { try? await Task.sleep(for: .milliseconds(500)) let (updatedClaude, updatedCodex, updatedOpenCode) = await Task.detached(priority: .utility) { - let claude = HookHealthCheck.checkClaude(hooksBinaryURL: binaryURL) - let codex = HookHealthCheck.checkCodex(hooksBinaryURL: binaryURL) + let claude = HookHealthCheck.checkClaude( + hooksBinaryURL: binaryURL, + expectsInstalledHooks: expectsClaude + ) + let codex = HookHealthCheck.checkCodex( + hooksBinaryURL: binaryURL, + expectsInstalledHooks: expectsCodex + ) let openCode = HookHealthCheck.checkOpenCode() return (claude, codex, openCode) }.value @@ -798,6 +820,12 @@ final class HookInstallationCoordinator { /// install. `.untouched` and `.uninstalled` both return false; /// untouched agents are surfaced to the user via the first-run /// onboarding window and the empty-state banner instead. + /// Whether the user has asked for this agent's hooks, so a config with none + /// of ours in it is a fault worth reporting rather than the desired state. + func expectsInstalledHooks(_ agent: AgentIdentifier) -> Bool { + intentStore.intent(for: agent) == .installed + } + func shouldAutoInstall(_ agent: AgentIdentifier) -> Bool { guard intentStore.intent(for: agent) == .installed else { return false diff --git a/Sources/OpenIslandCore/ClaudeHookInstaller.swift b/Sources/OpenIslandCore/ClaudeHookInstaller.swift index 00550663d..ec5ce18b2 100644 --- a/Sources/OpenIslandCore/ClaudeHookInstaller.swift +++ b/Sources/OpenIslandCore/ClaudeHookInstaller.swift @@ -117,6 +117,7 @@ public enum ClaudeHookInstaller { var rootObject = try loadRootObject(from: existingData) var hooksObject = rootObject["hooks"] as? [String: Any] ?? [:] var mutated = false + var ownHooksPresent = false for spec in eventSpecs { let existingGroups = hooksObject[spec.name] as? [Any] ?? [] @@ -126,6 +127,10 @@ public enum ClaudeHookInstaller { mutated = true } + if containsOwnManagedHook(in: existingGroups, managedCommand: managedCommand) { + ownHooksPresent = true + } + if cleanedGroups.isEmpty { hooksObject.removeValue(forKey: spec.name) } else { @@ -143,7 +148,7 @@ public enum ClaudeHookInstaller { return ClaudeHookFileMutation( contents: contents, changed: mutated || contents != existingData, - managedHooksPresent: mutated, + managedHooksPresent: ownHooksPresent, hasClaudeIslandHooks: containsClaudeIslandHook(in: hooksObject) ) } @@ -230,6 +235,23 @@ public enum ClaudeHookInstaller { } } + private static func containsOwnManagedHook(in groups: [Any], managedCommand: String?) -> Bool { + groups.contains { item in + guard let group = item as? [String: Any], + let hooks = group["hooks"] as? [Any] else { + return false + } + + return hooks.contains { hook in + guard let hook = hook as? [String: Any] else { + return false + } + + return isOwnManagedHook(hook, managedCommand: managedCommand) + } + } + } + private static func containsClaudeIslandHook(in hooksObject: [String: Any]) -> Bool { hooksObject.values.contains { value in let groups = value as? [Any] ?? [] @@ -275,6 +297,10 @@ public enum ClaudeHookInstaller { return group } + /// Whether a hook entry should be dropped when we (re)write our own hooks. + /// Deliberately broad: it also covers the closed-source Vibe Island bridge + /// so a user migrating from that app doesn't end up with two islands + /// reacting to every event. private static func isManagedHook(_ hook: [String: Any], managedCommand: String?) -> Bool { guard let command = hook["command"] as? String else { return false @@ -287,6 +313,30 @@ public enum ClaudeHookInstaller { return isLegacyOpenIslandHookCommand(command) } + /// Whether a hook entry is *ours* — the command we installed, or our hook + /// CLI under its current or pre-rename name. + /// + /// This is the question "are Open Island's hooks installed?", and it must + /// stay narrower than ``isManagedHook(_:managedCommand:)``. Answering it + /// with the broad predicate made a leftover `vibe-island-bridge` entry from + /// the closed-source app read as a successful install, which reported + /// "hooks installed" in Settings and suppressed the startup repair in + /// `shouldAutoInstall` — so no hook of ours ever ran. + private static func isOwnManagedHook(_ hook: [String: Any], managedCommand: String?) -> Bool { + guard let command = hook["command"] as? String else { + return false + } + + if let managedCommand, command == managedCommand { + return true + } + + // Path-independent fallback: the app bundle may have moved since + // install, which invalidates the recorded command but not ownership. + let normalized = command.lowercased() + return normalized.contains("openislandhooks") || normalized.contains("vibeislandhooks") + } + private static func isManagedHookForInstall(_ hook: [String: Any], replacingCommand: String) -> Bool { if isManagedHook(hook, managedCommand: replacingCommand) { return true diff --git a/Sources/OpenIslandCore/CodexHookInstallationManager.swift b/Sources/OpenIslandCore/CodexHookInstallationManager.swift index d83bcce6b..1080bf40a 100644 --- a/Sources/OpenIslandCore/CodexHookInstallationManager.swift +++ b/Sources/OpenIslandCore/CodexHookInstallationManager.swift @@ -62,7 +62,7 @@ public final class CodexHookInstallationManager: @unchecked Sendable { let managedHooksPresent = ((try? CodexHookInstaller.uninstallHooksJSON( existingData: hooksData, managedCommand: managedCommand - ))?.changed) == true + ))?.ownHooksPresent) == true return CodexHookInstallationStatus( codexDirectory: codexDirectory, diff --git a/Sources/OpenIslandCore/CodexHookInstaller.swift b/Sources/OpenIslandCore/CodexHookInstaller.swift index 48bf1fe86..4708f7729 100644 --- a/Sources/OpenIslandCore/CodexHookInstaller.swift +++ b/Sources/OpenIslandCore/CodexHookInstaller.swift @@ -35,11 +35,23 @@ public struct CodexHookFileMutation: Equatable, Sendable { public var contents: Data? public var changed: Bool public var hasRemainingHooks: Bool + /// 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 } } @@ -133,12 +145,18 @@ public enum CodexHookInstaller { var rootObject = try loadRootObject(from: existingData) var hooksObject = rootObject["hooks"] as? [String: Any] ?? [:] var mutated = false + var ownHooksPresent = false for spec in eventSpecs { let existingGroups = hooksObject[spec.name] as? [Any] ?? [] let cleanedGroups = sanitize(groups: existingGroups, managedCommand: managedCommand) - if cleanedGroups.count != existingGroups.count || containsManagedHook(in: existingGroups, managedCommand: managedCommand) { + let hadOwnHooks = containsManagedHook(in: existingGroups, managedCommand: managedCommand) + if hadOwnHooks { + ownHooksPresent = true + } + + if cleanedGroups.count != existingGroups.count || hadOwnHooks { mutated = true } @@ -150,12 +168,22 @@ public enum CodexHookInstaller { } if hooksObject.isEmpty { - return CodexHookFileMutation(contents: nil, changed: mutated, hasRemainingHooks: false) + return CodexHookFileMutation( + contents: nil, + changed: mutated, + hasRemainingHooks: false, + ownHooksPresent: ownHooksPresent + ) } rootObject["hooks"] = hooksObject let data = try serialize(rootObject) - return CodexHookFileMutation(contents: data, changed: mutated || data != existingData, hasRemainingHooks: true) + return CodexHookFileMutation( + contents: data, + changed: mutated || data != existingData, + hasRemainingHooks: true, + ownHooksPresent: ownHooksPresent + ) } /// Enables the current Codex hooks feature flag and migrates the legacy flag when present. diff --git a/Sources/OpenIslandCore/HookHealthCheck.swift b/Sources/OpenIslandCore/HookHealthCheck.swift index 328e8c6b1..cb19ec504 100644 --- a/Sources/OpenIslandCore/HookHealthCheck.swift +++ b/Sources/OpenIslandCore/HookHealthCheck.swift @@ -24,6 +24,9 @@ public struct HookHealthReport: Equatable, Sendable { case manifestMissing(expectedPath: String) /// The OpenCode plugin file is missing even though it should be installed. case pluginMissing(expectedPath: String) + /// The user asked for these hooks, but none of ours are in the config. + /// Usually another tool rewrote the file and dropped our entries. + case hooksMissing(configPath: String) public var description: String { switch self { @@ -41,6 +44,8 @@ public struct HookHealthReport: Equatable, Sendable { "Installation manifest missing: \(expectedPath)" case .pluginMissing(let expectedPath): "OpenCode plugin file is missing: \(expectedPath)" + case .hooksMissing(let configPath): + "Open Island hooks are missing from \(configPath) — another tool may have rewritten it." } } @@ -55,7 +60,7 @@ public struct HookHealthReport: Equatable, Sendable { public var isAutoRepairable: Bool { switch self { - case .staleCommandPath, .binaryNotExecutable, .manifestMissing, .pluginMissing: + case .staleCommandPath, .binaryNotExecutable, .manifestMissing, .pluginMissing, .hooksMissing: true default: false @@ -97,10 +102,16 @@ public struct HookHealthReport: Equatable, Sendable { /// Performs deep health checks on hook installations, beyond the simple "managed hooks present" check. public enum HookHealthCheck { /// Check Claude Code hook health. + /// + /// - Parameter expectsInstalledHooks: Whether the user has asked for these + /// hooks (persisted intent is `.installed`). Only then is an empty config + /// a problem — otherwise "no hooks of ours" is the desired state, and + /// reporting it would auto-repair hooks the user deliberately removed. public static func checkClaude( claudeDirectory: URL = ClaudeConfigDirectory.resolved(), hooksBinaryURL: URL? = nil, managedHooksBinaryURL: URL = ManagedHooksBinary.defaultURL(), + expectsInstalledHooks: Bool = false, fileManager: FileManager = .default ) -> HookHealthReport { var issues: [HookHealthReport.Issue] = [] @@ -151,13 +162,19 @@ public enum HookHealthCheck { } } - // 3. Check manifest - if fileManager.fileExists(atPath: settingsPath), - hasOpenIslandHooks(in: settingsURL, fileManager: fileManager) { + // 3. Check that our own hooks are actually there, and the manifest. + let hasOwnHooks = hasOpenIslandHooks(in: settingsURL, fileManager: fileManager) + + if hasOwnHooks { let legacyManifestURL = claudeDirectory.appendingPathComponent(ClaudeHookInstallerManifest.legacyFileName) if !fileManager.fileExists(atPath: manifestURL.path) && !fileManager.fileExists(atPath: legacyManifestURL.path) { issues.append(.manifestMissing(expectedPath: manifestURL.path)) } + } else if expectsInstalledHooks, !issues.contains(.configMalformedJSON(path: settingsPath)) { + // Don't claim the hooks are gone when we couldn't parse the file — + // the malformed-JSON issue already describes that case, and + // re-installing over unparseable settings would not help. + issues.append(.hooksMissing(configPath: settingsPath)) } return HookHealthReport( @@ -169,10 +186,13 @@ public enum HookHealthCheck { } /// Check Codex hook health. + /// + /// - Parameter expectsInstalledHooks: See ``checkClaude(claudeDirectory:hooksBinaryURL:managedHooksBinaryURL:expectsInstalledHooks:fileManager:)``. public static func checkCodex( codexDirectory: URL = FileManager.default.homeDirectoryForCurrentUser.appendingPathComponent(".codex", isDirectory: true), hooksBinaryURL: URL? = nil, managedHooksBinaryURL: URL = ManagedHooksBinary.defaultURL(), + expectsInstalledHooks: Bool = false, fileManager: FileManager = .default ) -> HookHealthReport { var issues: [HookHealthReport.Issue] = [] @@ -214,13 +234,16 @@ public enum HookHealthCheck { } } - // 3. Check manifest - if fileManager.fileExists(atPath: hooksPath), - hasOpenIslandHooks(in: hooksURL, fileManager: fileManager) { + // 3. Check that our own hooks are actually there, and the manifest. + let hasOwnHooks = hasOpenIslandHooks(in: hooksURL, fileManager: fileManager) + + if hasOwnHooks { let legacyManifestURL = codexDirectory.appendingPathComponent(CodexHookInstallerManifest.legacyFileName) if !fileManager.fileExists(atPath: manifestURL.path) && !fileManager.fileExists(atPath: legacyManifestURL.path) { issues.append(.manifestMissing(expectedPath: manifestURL.path)) } + } else if expectsInstalledHooks, !issues.contains(.configMalformedJSON(path: hooksPath)) { + issues.append(.hooksMissing(configPath: hooksPath)) } return HookHealthReport( diff --git a/Tests/OpenIslandCoreTests/ClaudeHooksTests.swift b/Tests/OpenIslandCoreTests/ClaudeHooksTests.swift index 25c92fef2..f7ca07d97 100644 --- a/Tests/OpenIslandCoreTests/ClaudeHooksTests.swift +++ b/Tests/OpenIslandCoreTests/ClaudeHooksTests.swift @@ -74,6 +74,64 @@ struct ClaudeHooksTests { #expect(!FileManager.default.fileExists(atPath: uninstalled.manifestURL.path)) } + /// A leftover hook from the closed-source Vibe Island app must not read as + /// "our hooks are installed". It used to, which made Settings show the hooks + /// as present and suppressed the startup repair, so no hook of ours ran. + @Test + func claudeStatusReportsHooksAbsentWhenOnlyVibeIslandBridgeHookExists() throws { + let rootURL = FileManager.default.temporaryDirectory + .appendingPathComponent("open-island-claude-foreign-\(UUID().uuidString)", isDirectory: true) + let claudeDirectory = rootURL.appendingPathComponent(".claude", isDirectory: true) + let managedHooksBinaryURL = rootURL + .appendingPathComponent("managed", isDirectory: true) + .appendingPathComponent("OpenIslandHooks") + + defer { + try? FileManager.default.removeItem(at: rootURL) + } + + try FileManager.default.createDirectory(at: managedHooksBinaryURL.deletingLastPathComponent(), withIntermediateDirectories: true) + try Data("claude-hook".utf8).write(to: managedHooksBinaryURL) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: managedHooksBinaryURL.path) + + let bridgeCommand = "/bin/sh -c '[ -x \"$HOME/.vibe-island/bin/vibe-island-bridge\" ] && \"$HOME/.vibe-island/bin/vibe-island-bridge\" --source claude; exit 0'" + let settings: [String: Any] = [ + "hooks": [ + "SessionStart": [["hooks": [["type": "command", "command": bridgeCommand]]]], + "UserPromptSubmit": [["hooks": [["type": "command", "command": bridgeCommand]]]], + ], + ] + try FileManager.default.createDirectory(at: claudeDirectory, withIntermediateDirectories: true) + try JSONSerialization.data(withJSONObject: settings, options: [.prettyPrinted, .sortedKeys]) + .write(to: claudeDirectory.appendingPathComponent("settings.json"), options: .atomic) + + let manager = ClaudeHookInstallationManager( + claudeDirectory: claudeDirectory, + managedHooksBinaryURL: managedHooksBinaryURL + ) + + let status = try manager.status() + #expect(!status.managedHooksPresent) + + // Installing still claims the slots — the two apps cannot both drive + // the island — and now reports itself as present. + let installed = try manager.install(hooksBinaryURL: managedHooksBinaryURL) + #expect(installed.managedHooksPresent) + + let settingsObject = try jsonObject(from: Data(contentsOf: installed.settingsURL)) + let hooksObject = try #require(settingsObject["hooks"] as? [String: Any]) + let allCommands: [String] = hooksObject.values + .compactMap { $0 as? [Any] } + .flatMap { $0 } + .compactMap { $0 as? [String: Any] } + .compactMap { $0["hooks"] as? [Any] } + .flatMap { $0 } + .compactMap { $0 as? [String: Any] } + .compactMap { $0["command"] as? String } + #expect(!allCommands.contains(bridgeCommand)) + #expect(allCommands.contains(where: { $0.contains("OpenIslandHooks") })) + } + @Test func claudeTranscriptDiscoveryRecoversRecentSessions() throws { let rootURL = FileManager.default.temporaryDirectory diff --git a/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift b/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift new file mode 100644 index 000000000..f56921cab --- /dev/null +++ b/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift @@ -0,0 +1,162 @@ +import Foundation +import Testing +@testable import OpenIslandCore + +struct HookHealthCheckTests { + /// Creates a temp `.claude` directory containing `settings.json` with the + /// given hook commands, plus an executable stand-in for the hooks binary. + private func makeClaudeFixture( + commands: [String], + root: URL + ) throws -> (claudeDirectory: URL, hooksBinaryURL: URL) { + let claudeDirectory = root.appendingPathComponent(".claude", isDirectory: true) + let hooksBinaryURL = root + .appendingPathComponent("managed", isDirectory: true) + .appendingPathComponent("OpenIslandHooks") + + try FileManager.default.createDirectory(at: hooksBinaryURL.deletingLastPathComponent(), withIntermediateDirectories: true) + try Data("hook".utf8).write(to: hooksBinaryURL) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: hooksBinaryURL.path) + + try FileManager.default.createDirectory(at: claudeDirectory, withIntermediateDirectories: true) + let groups = commands.map { ["hooks": [["type": "command", "command": $0]]] } + let settings: [String: Any] = ["hooks": ["SessionStart": groups]] + try JSONSerialization.data(withJSONObject: settings, options: [.prettyPrinted, .sortedKeys]) + .write(to: claudeDirectory.appendingPathComponent("settings.json"), options: .atomic) + + return (claudeDirectory, hooksBinaryURL) + } + + private func temporaryRoot() -> URL { + FileManager.default.temporaryDirectory + .appendingPathComponent("open-island-health-\(UUID().uuidString)", isDirectory: true) + } + + private func reportsHooksMissing(_ report: HookHealthReport) -> Bool { + report.issues.contains(where: { issue in + if case .hooksMissing = issue { return true } + return false + }) + } + + @Test + func claudeHealthReportsHooksMissingWhenUserAskedForThemAndOnlyForeignHooksRemain() throws { + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + + let fixture = try makeClaudeFixture( + commands: ["/bin/sh -c '\"$HOME/.vibe-island/bin/vibe-island-bridge\" --source claude'"], + root: root + ) + + let report = HookHealthCheck.checkClaude( + claudeDirectory: fixture.claudeDirectory, + hooksBinaryURL: fixture.hooksBinaryURL, + expectsInstalledHooks: true + ) + + let settingsPath = fixture.claudeDirectory.appendingPathComponent("settings.json").path + #expect(report.issues.contains(.hooksMissing(configPath: settingsPath))) + #expect(!report.isHealthy) + #expect(report.repairableIssues.contains(.hooksMissing(configPath: settingsPath))) + } + + /// The user turned these hooks off on purpose — an empty config is correct, + /// and reporting it would let auto-repair reinstall them behind their back. + @Test + func claudeHealthStaysQuietAboutMissingHooksWhenUserDidNotAskForThem() throws { + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + + let fixture = try makeClaudeFixture( + commands: ["/usr/local/bin/some-other-tool --hook"], + root: root + ) + + let report = HookHealthCheck.checkClaude( + claudeDirectory: fixture.claudeDirectory, + hooksBinaryURL: fixture.hooksBinaryURL, + expectsInstalledHooks: false + ) + + #expect(!reportsHooksMissing(report)) + #expect(report.isHealthy) + } + + @Test + func claudeHealthDoesNotReportHooksMissingWhenOurHooksArePresent() throws { + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + + let fixture = try makeClaudeFixture( + commands: ["'/Applications/Open Island.app/Contents/Helpers/OpenIslandHooks' --source claude"], + root: root + ) + + let report = HookHealthCheck.checkClaude( + claudeDirectory: fixture.claudeDirectory, + hooksBinaryURL: fixture.hooksBinaryURL, + expectsInstalledHooks: true + ) + + #expect(!reportsHooksMissing(report)) + } + + /// Malformed JSON already has its own issue, and re-installing over a file + /// we cannot parse would not fix anything. + @Test + func claudeHealthPrefersMalformedJSONOverHooksMissing() throws { + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + + let claudeDirectory = root.appendingPathComponent(".claude", isDirectory: true) + let hooksBinaryURL = root + .appendingPathComponent("managed", isDirectory: true) + .appendingPathComponent("OpenIslandHooks") + try FileManager.default.createDirectory(at: hooksBinaryURL.deletingLastPathComponent(), withIntermediateDirectories: true) + try Data("hook".utf8).write(to: hooksBinaryURL) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: hooksBinaryURL.path) + try FileManager.default.createDirectory(at: claudeDirectory, withIntermediateDirectories: true) + let settingsURL = claudeDirectory.appendingPathComponent("settings.json") + try Data("{ not json".utf8).write(to: settingsURL, options: .atomic) + + let report = HookHealthCheck.checkClaude( + claudeDirectory: claudeDirectory, + hooksBinaryURL: hooksBinaryURL, + expectsInstalledHooks: true + ) + + #expect(report.issues.contains(.configMalformedJSON(path: settingsURL.path))) + #expect(!reportsHooksMissing(report)) + } + + @Test + func codexHealthReportsHooksMissingWhenUserAskedForThem() throws { + let root = temporaryRoot() + defer { try? FileManager.default.removeItem(at: root) } + + let codexDirectory = root.appendingPathComponent(".codex", isDirectory: true) + let hooksBinaryURL = root + .appendingPathComponent("managed", isDirectory: true) + .appendingPathComponent("OpenIslandHooks") + try FileManager.default.createDirectory(at: hooksBinaryURL.deletingLastPathComponent(), withIntermediateDirectories: true) + try Data("hook".utf8).write(to: hooksBinaryURL) + try FileManager.default.setAttributes([.posixPermissions: 0o755], ofItemAtPath: hooksBinaryURL.path) + try FileManager.default.createDirectory(at: codexDirectory, withIntermediateDirectories: true) + + let hooksURL = codexDirectory.appendingPathComponent("hooks.json") + let hooks: [String: Any] = [ + "hooks": ["SessionStart": [["hooks": [["type": "command", "command": "/usr/local/bin/other --hook"]]]]], + ] + try JSONSerialization.data(withJSONObject: hooks, options: [.prettyPrinted, .sortedKeys]) + .write(to: hooksURL, options: .atomic) + + let report = HookHealthCheck.checkCodex( + codexDirectory: codexDirectory, + hooksBinaryURL: hooksBinaryURL, + expectsInstalledHooks: true + ) + + #expect(report.issues.contains(.hooksMissing(configPath: hooksURL.path))) + } +} From 018069b329bdb7d47369b0bfa99b2a6e902fb8e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=E4=BC=98=E8=83=9C?= Date: Wed, 12 Aug 2026 11:24:48 +0800 Subject: [PATCH 2/2] docs: document the helpers and cases added for hook ownership 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) --- Sources/OpenIslandCore/ClaudeHookInstaller.swift | 2 ++ Sources/OpenIslandCore/CodexHookInstaller.swift | 3 +++ Tests/OpenIslandCoreTests/HookHealthCheckTests.swift | 11 +++++++++++ 3 files changed, 16 insertions(+) diff --git a/Sources/OpenIslandCore/ClaudeHookInstaller.swift b/Sources/OpenIslandCore/ClaudeHookInstaller.swift index ec5ce18b2..8aad0f4e0 100644 --- a/Sources/OpenIslandCore/ClaudeHookInstaller.swift +++ b/Sources/OpenIslandCore/ClaudeHookInstaller.swift @@ -235,6 +235,8 @@ public enum ClaudeHookInstaller { } } + /// Whether any hook in `groups` is one of ours. + /// The ownership counterpart to ``containsManagedHook(in:managedCommand:)``. private static func containsOwnManagedHook(in groups: [Any], managedCommand: String?) -> Bool { groups.contains { item in guard let group = item as? [String: Any], diff --git a/Sources/OpenIslandCore/CodexHookInstaller.swift b/Sources/OpenIslandCore/CodexHookInstaller.swift index 4708f7729..3dfa56fcc 100644 --- a/Sources/OpenIslandCore/CodexHookInstaller.swift +++ b/Sources/OpenIslandCore/CodexHookInstaller.swift @@ -42,6 +42,9 @@ public struct CodexHookFileMutation: Equatable, Sendable { /// this — never ``changed`` — to answer "are our hooks installed?". public var ownHooksPresent: Bool + /// - Parameter ownHooksPresent: Defaults to `false` so existing callers that + /// only describe a write keep compiling; the uninstall path, which is the + /// one asked about installed state, always passes it explicitly. public init( contents: Data?, changed: Bool, diff --git a/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift b/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift index f56921cab..0856b1d4b 100644 --- a/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift +++ b/Tests/OpenIslandCoreTests/HookHealthCheckTests.swift @@ -27,11 +27,16 @@ struct HookHealthCheckTests { return (claudeDirectory, hooksBinaryURL) } + /// A unique, unused scratch directory. Not created here — the fixture + /// helpers create what they need under it, and each test removes the whole + /// tree in a `defer`. private func temporaryRoot() -> URL { FileManager.default.temporaryDirectory .appendingPathComponent("open-island-health-\(UUID().uuidString)", isDirectory: true) } + /// Whether the report carries a `hooksMissing` issue, ignoring its path. + /// Several tests assert only on presence or absence. private func reportsHooksMissing(_ report: HookHealthReport) -> Bool { report.issues.contains(where: { issue in if case .hooksMissing = issue { return true } @@ -39,6 +44,9 @@ struct HookHealthCheckTests { }) } + /// The regression this PR fixes: a leftover hook from the closed-source app + /// occupies the config, ours are gone, and the user did ask for ours. That + /// has to surface as a repairable error rather than a clean bill of health. @Test func claudeHealthReportsHooksMissingWhenUserAskedForThemAndOnlyForeignHooksRemain() throws { let root = temporaryRoot() @@ -83,6 +91,7 @@ struct HookHealthCheckTests { #expect(report.isHealthy) } + /// The healthy case, guarding against a check that fires unconditionally. @Test func claudeHealthDoesNotReportHooksMissingWhenOurHooksArePresent() throws { let root = temporaryRoot() @@ -130,6 +139,8 @@ struct HookHealthCheckTests { #expect(!reportsHooksMissing(report)) } + /// Codex reaches the same conclusion through a different config file, so it + /// gets its own case rather than relying on the Claude path's coverage. @Test func codexHealthReportsHooksMissingWhenUserAskedForThem() throws { let root = temporaryRoot()