-
Notifications
You must be signed in to change notification settings - Fork 339
fix: detect our own hooks by ownership instead of "file would change" #653
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -35,11 +35,26 @@ public struct CodexHookFileMutation: Equatable, Sendable { | |
| public var contents: Data? | ||
| public var changed: Bool | ||
| public var hasRemainingHooks: Bool | ||
|
|
||
| public init(contents: Data?, changed: Bool, 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 | ||
|
|
||
| /// - 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, | ||
| hasRemainingHooks: Bool, | ||
| ownHooksPresent: Bool = false | ||
| ) { | ||
| self.contents = contents | ||
| self.changed = changed | ||
| self.hasRemainingHooks = hasRemainingHooks | ||
| self.ownHooksPresent = ownHooksPresent | ||
| } | ||
| } | ||
|
|
||
|
|
@@ -133,12 +148,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 | ||
|
Comment on lines
+157
to
163
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win Use an ownership-specific predicate. Line 154 calls
Add 🤖 Prompt for AI Agents |
||
| } | ||
|
|
||
|
|
@@ -150,12 +171,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. | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📐 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
SendableandCodable.”📍 Affects 2 files
Sources/OpenIslandCore/CodexHookInstaller.swift#L38-L54(this comment)Sources/OpenIslandCore/HookHealthCheck.swift#L27-L29🤖 Prompt for AI Agents
Source: Coding guidelines