WEB-4814: merge hooks block instead of overwriting (preserve other tools)#140
Draft
vigneshsubbiah16 wants to merge 2 commits into
Draft
WEB-4814: merge hooks block instead of overwriting (preserve other tools)#140vigneshsubbiah16 wants to merge 2 commits into
vigneshsubbiah16 wants to merge 2 commits into
Conversation
…ols) The two MDM writers for Claude Code's managed-settings.json did a blind wholesale overwrite of the hooks key, clobbering any pre-existing hooks that belonged to other tools or prior configuration: - binary/src/unbound_hook/setup_cmd.py:_write_claude_managed_settings - claude-code/hooks/mdm/setup.py:setup_managed_hooks Replace both assignments with a per-event MERGE (mirrors the existing user-level merge in claude-code/hooks/setup.py:configure_claude_settings): preserve other tools' / hand-authored entries, and stay idempotent by stripping any stale Unbound-owned entry (matched on the hook binary / script path substring) before appending the current one. Coerces a non-dict existing hooks block to empty so a malformed file can never crash the root installer (fail-open). Tests at the outermost layer (setup_cmd.run / setup_managed_hooks): foreign hook survives, Unbound entry present exactly once, re-run does not duplicate, drifted-command stale entry is replaced, malformed hooks block does not crash. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Convergent reviewer hardenings on the merge-don't-overwrite path (both
elite + security reviewers approved these as cheap fixes that protect the
ticket's core "don't destroy other tools' hooks" guarantee).
Fix 1 (both reviewers): _entry_is_unbound matched `owner_substr in command`,
which would mis-classify — and DELETE — a foreign hook whose command merely
references our install path mid-string (e.g. a wrapper exec'ing our binary as
an argument). New _command_is_unbound anchors on path-prefix in the exact
forms our writers emit ("<path>" ..., bare <path> ..., and Windows
`py -3 "<path>"`), still tolerating trailing-arg/version drift so idempotency
holds. Applied identically to both the binary and python-MDM mirrors.
Fix 2 (security LOW-1): the python MDM wrote managed-settings.json via a
non-atomic write_text; a crash mid-write leaves a truncated file that Claude
Code silently ignores, dropping the security control. Mirror the binary's
tmp+os.replace atomic write. Existing chmod step preserves file mode.
Tests: added the Fix-1 guarantee to both suites (a foreign hook containing
our path as a substring survives; our own drifted entry is replaced once) —
both fail on pre-fix code, pass after — plus an atomic-write test. Existing
suites stay green.
Co-Authored-By: Claude Opus 4.8 <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
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.
WEB-4814 (Urgent)
The MDM hook installer overwrote Claude Code's managed-settings
hookskey wholesale, clobbering any pre-existing hooks belonging to other tools or prior configuration.Root cause
Two MDM writers did a blind assignment of the whole
hooksblock:binary/src/unbound_hook/setup_cmd.py→_write_claude_managed_settings()settings["hooks"] = _claude_hooks_config()claude-code/hooks/mdm/setup.py→setup_managed_hooks()settings["hooks"] = hooks_configThe read-before-write already preserved other top-level keys (
model, etc.) — only thehookskey was destroyed.Fix
Replace both assignments with a per-event merge that mirrors the merge pattern already proven in the user-level path (
claude-code/hooks/setup.py→configure_claude_settings()):hooksvalue that isn't a dict is coerced to empty rather than crashing the installer; on any JSON parse error the existing read-before-write already leaves the file intact + logs.Each writer passes its own owner substring: the binary path (
HOOK_BINARY) for the binary writer, the managedscript_pathfor the python writer (the latter covers both the Unix"path"and Windowspy -3 "path"command forms).Scope notes
_write_claude_managed_settings+ the python MDMsetup_managed_hooks(plus their tests), to stay non-conflicting with concurrent work on_run_discoveryin the same file. No reformatting of unrelated code._write_codex_managed_settings) has the same wholesale-overwrite shape but is out of scope for this ticket; thehooks.jsonit writes is Unbound-owned for enterprise installs. Flagging for a follow-up if Codex managed settings can ever be co-authored.hooks.jsonleft as-is: scoping found that file is Unbound-owned for enterprise installs, and it uses a different schema. Not changed.Tests (outermost layer)
Binary path —
binary/tests/test_setup_migration.py(runssetup_cmd.run([...])E2E against a sandboxed managed dir):hooksblock does not crash; Unbound hooks still writtenPython MDM path —
claude-code/hooks/test_setup.py(runssetup_managed_hooks()with network download stubbed):hooksblock does not crashPre-existing failures on clean
origin/staging(unrelated to this change, verified by stashing):test_hook_cli.py::test_frozen_session_start_makes_no_downloads(4) andtest_identity.py::test_keys_limited_to_identity_fields(1).Refs WEB-4814.
🤖 Generated with Claude Code