-
Notifications
You must be signed in to change notification settings - Fork 41
merge the stop hook into an existing settings.json #500
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
Changes from 1 commit
9bc7357
93f46e3
9a3274f
d668650
adbf765
357e493
fc47142
190b2f5
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 |
|---|---|---|
|
|
@@ -73,6 +73,11 @@ func Merge(existing, generated []byte) (*MergeResult, error) { | |
| // Merge hooks.PreToolUse — replace the chunk-managed hook group by matcher. | ||
| mergeHooks(merged, generatedMap) | ||
|
|
||
| // Merge hooks.Stop — replace the chunk-managed group by command. Without | ||
| // this a repo that already had a settings.json keeps its commit hooks but | ||
| // never gets the Stop hook, so validation stops running at session end. | ||
| mergeStopHooks(merged, generatedMap) | ||
|
Contributor
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. i don't think you need a new function to merge the stop hooks, merge hooks just wasn't handling the case probably. i'd extend merge hooks to also include the stop hook merging as well.
Contributor
Author
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. from claude: done in adbf765 - kept the difference in how they merge, since it is not incidental: chunk owns the whole PreToolUse group (matched by matcher) but only a single Stop entry (matched by command), because a user may have their own entries in that same group and replacing the group would delete them. That is now documented on the one function instead of split across two, and the duplicated "create hooks if absent" block collapses into a no behavior change — the existing tests pin both halves independently: six exercise generated settings carrying only PreToolUse, six only Stop, three both. |
||
|
|
||
| mergedBytes, err := json.MarshalIndent(merged, "", " ") | ||
| if err != nil { | ||
| return nil, fmt.Errorf("marshal merged settings: %w", err) | ||
|
|
||
|
jesseworld22 marked this conversation as resolved.
|
Uh oh!
There was an error while loading. Please reload this page.