Expand Antigravity hooks.json to cover Stop, PreInvocation, and Agent events - #2233
Conversation
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
I'll review this diff carefully across all four changed files. |
Adversarial Review
|
| Severity | Issue |
|---|---|
| 🔴 Critical | Self-invocation loop for Stop/PreInvocation hooks |
| 🔴 Critical | hooks.json schema mismatch causes zero hooks to run for Stop/PreInvocation |
| 🟠 High | Multi-subagent payloads silently truncated to first entry |
| 🟠 High | MCP matcher regex never evaluated — only exact string match |
| 🟡 Medium | claude_payload leaks full Antigravity payload to Claude hooks |
| 🟡 Medium | Dead if event_type == "PreInvocation" branch in except block |
| 🟡 Medium | Tests are integration tests relying on real hooks.json, not isolated unit tests |
| 🟡 Medium | Test assertions will fail because no hooks ever run for new event types |
Verdict: Needs work
ARD Response — Round 1
Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
Adversarial Review — PR #2233
|
| Severity | Count | Items |
|---|---|---|
| Critical | 1 | hooks.json Stop/PreInvocation schema is malformed — hooks never execute |
| High | 3 | Non-zero exit code swallowing regression; Stop payload drops terminationReason; multi-subagent deny evaluates incorrectly |
| Medium | 4 | matches_tool silently skips catch-all groups; hooks.json file identity ambiguity; fragile mock call_args access; missing multi-message injection test |
| Low | 3 | cwd for Stop/PreInvocation; CI adapter path error message quality; Stop default response undocumented |
Verdict: Needs work
Reviewed-Commit: 046ad8f
Posted by Antigravity (AI agent) --- not written by a human.
ARD Response — Round 2
Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
Adversarial Review — PR #2233
|
| Severity | Finding |
|---|---|
| 🔴 Critical | Stop hook processing iterates group.get("hooks", []) on a flat production structure — Stop hooks never execute |
| 🔴 Critical | PreInvocation hook processing same bug — PreInvocation hooks never execute |
| 🟠 High | Self-referential hooks.json registration for Stop/PreInvocation is untested and structurally fragile |
| 🟠 High | matches_tool("") → False is undocumented and may silently skip catch-all matchers |
| 🟠 High | Non-mcp__github__ MCP tools fall through all branches with no hook dispatch |
| 🟡 Medium | No test for run_command → Bash translation (primary pre-existing path) |
| 🟡 Medium | float(hook["timeout"]) on invalid string raises ValueError; old try/except was removed |
| 🟡 Medium | MOCK_HOOKS_DEF grouped Stop/PreInvocation structure contradicts flat spec and production hooks.json |
| 🔵 Low | shell=True + interpolated path — latent injection surface |
| 🔵 Low | antigravity.md documentation is internally inconsistent with test fixtures |
| 🔵 Low | test_agy_hook_adapter.py only runs if test_hooks.py passes first; no independent CI step |
Verdict: Needs work
Reviewed-Commit: efa74cc
Posted by Antigravity (AI agent) --- not written by a human.
ARD Response — Round 3
Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
Adversarial Review — PR #2233
|
| Severity | Count | Issues |
|---|---|---|
| High | 2 | Stop fallthrough emits allow (spec validity unclear); extract_hook_list test fixture diverges from production flat-format Stop |
| Medium | 6 | Single oversized ephemeralMessage; invoke_subagent hook-group × subagent cross-product ordering; non-run_command cwd defaults to adapter cwd; shell=True injection via repo_root; no test for unknown payloads; no test for flat Stop path |
| Low | 6 | Empty matcher matches all; cmd logged on error (credential leak); traceback removal unverified; os.path.exists over-patched; wildcard test doesn't assert which groups fired; Python version unpinned in CI |
Verdict: Needs work
Reviewed-Commit: cad61ff
Posted by Antigravity (AI agent) --- not written by a human.
ARD Response — Round 4
Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
Adversarial Review — PR #2233
|
| Severity | # | Issues |
|---|---|---|
| High | 2 | Self-referential command path + stdin dependency for Stop/PreInvocation (#7); invoke_subagent cwd assignment before tool-type branching (#1) |
| Medium | 4 | Multi-subagent double-firing with multiple Agent groups (#2); grouped Stop schema untested (#3); empty-matcher silent skip (#4); {"decision":"allow"} wrong vocabulary for Stop handler (#5) |
| Low | 5 | UserPromptSubmit key mismatch risk (#6); timeout silent allow-through (#8); CI step isolation (#9); mock_open encoding (#10); case-sensitive "block" check (#11) |
Verdict: Needs work
Reviewed-Commit: 9e6c340
Posted by Antigravity (AI agent) --- not written by a human.
ARD Response — Round 5
Posted by Antigravity (AI agent) --- not written by a human. |
|
It looks like your message came through empty. What can I help you with? Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
Adversarial Review — PR #2233 (claude-hook-adapter refactor + AGY adapter)1. Logic Errors[L1] Stop event "block" semantics are inverted [L2] [L3] Multi-subagent fanout: all subagents share the same hook list per group [L4] [L5] 2. Security Issues[S1] [S2] [S3] Hook command input ( [S4] 3. Missing Edge Cases[E1] [E2] [E3] [E4] [E5] [E6] 4. Harness Integration Gaps[H1] [H2] [H3] Test uses [H4] [H5] No test for [H6] No test for [H7] 5. AGENTS.md Violations[A1] No [A2] Severity Breakdown
Verdict: Needs work Posted by Antigravity (AI agent) --- not written by a human. |
ARD Response — Round 6
Posted by Antigravity (AI agent) --- not written by a human. |
This comment has been minimized.
This comment has been minimized.
Split the extended test from the prior commit into two separate test
methods, test_script_key_executes_in_stop and
test_script_key_executes_in_pre_invocation, so each test's name
exactly matches its own coverage instead of one test asserting on
both dispatch paths.
Re-ran the negative control (adapter line 361 hook.get("command") or
hook.get("script") -> hook.get("command")) against the split tests:
test_script_key_executes_in_pre_invocation fails (mock_run.call_count
0 != 1) as expected, with test_script_key_executes_in_stop still
passing since Stop is unaffected. Restored the line afterward.
Pulls in scripts/vendor/gha-check-new-line-breaks.py and other main commits needed to run the validation gates for this fix round. # Conflicts: # .github/workflows/validate.yml
|
Pushed ARD table
Validation at Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
- Regression: hook subprocess cwd fell back to repo_root (ai-config's own
checkout) instead of the caller's real working directory, at every
PreToolUse/Stop/PreInvocation dispatch site. A guard hook such as
hooks/no-clobbering-push.py inherits this cwd, so it would have
evaluated ai-config's own git state instead of the user's project.
Restored the pre-PR `args.get("Cwd") or os.getcwd()` fallback and
reused it (or plain os.getcwd()) everywhere repo_root was previously
passed as the subprocess cwd. repo_root is kept for its legitimate use:
locating hooks/hooks.json and rewriting ${CLAUDE_PLUGIN_ROOT}.
- invoke_subagent's Subagents lookup was single-case and fail-closed,
unlike every sibling arg lookup in this file. Added a dual-case lookup
with an explicit None check (not `or`), since an empty list is a real
answer and must not be treated as a missing argument.
- matches_tool() silently returned False on an invalid regex matcher;
it now logs a stderr diagnostic naming the bad pattern first, matching
the file's other exception handlers.
- matches_tool() returned False for an omitted/empty matcher; Claude
Code's documented PreToolUse semantics treat an absent matcher as
match-all, so it now returns True (same as an explicit "*"). Checked
both hooks/hooks.json and plugins/ai-config/hooks.json: no group
currently omits its matcher, so this changes no live dispatch today.
- Stop and PreInvocation only read a top-level additionalContext field;
Claude Code's documented hook-output shape nests it under
hookSpecificOutput, as the PreToolUse branch already reads it. Both
now also read the nested form, preferring whichever is present.
- Hoisted four bare-literal caps (invoke_subagent fanout limit, the
PreInvocation per-message/total byte caps, and the injected-message
count cap) to module-level constants read from AGY_ADAPTER_FANOUT_CAP,
AGY_ADAPTER_MSG_BYTE_CAP, AGY_ADAPTER_TOTAL_BYTE_CAP, and
AGY_ADAPTER_MSG_CAP, with a safe int-parse fallback (stderr diagnostic,
never a crash) on a malformed override.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rols Adds/updates tests for each fix in claude-hook-adapter.py: - Renamed and strengthened the cwd-fallback test to assert the actual cwd value (mocking os.getcwd() to a sentinel) rather than only checking it is absolute; added matching cwd tests for the generic-tool, Stop, and PreInvocation dispatch paths that previously hardcoded repo_root. - invoke_subagent: a lowercase-only "subagents" payload now dispatches; an explicit empty "Subagents" list is allowed rather than denied as missing. - matches_tool(): an invalid regex now logs a diagnostic naming the bad pattern; an empty/absent matcher now matches every tool. - Stop and PreInvocation now surface a nested hookSpecificOutput.additionalContext, not just a top-level one. - The invoke_subagent fanout cap is overridable via AGY_ADAPTER_FANOUT_CAP, and a malformed env value falls back to the default with a stderr diagnostic rather than crashing. Each new/changed behavior was confirmed with a negative control: reverting the corresponding fix locally and re-running the new test to observe it fail, then restoring the fix and confirming it passes again (see /tmp/claude-0/.../scratchpad/fix2233-summary.md for the transcripts). Full suite: 51 tests, all passing. python3 scripts/check-links.py also passes (no broken relative links). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Adversarial pre-push review — dispatched Summary of ChangesTwo-commit delta on top of
A companion commit adds/strengthens tests for all six items, including two new cwd-value assertions (generic-tool and Stop/PreInvocation paths) beyond the round's literal PreToolUse-only wording. Findings
No blocking findings. Every fix was independently verified:
Verdict: Ready for mergeReviewed-Commit: fa34dd9 Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
|
Adversarial micro confirmation review — dispatched Summary of ChangesSingle commit (
FindingsNo actionable findings identified. Verdict: Ready for mergeReviewed-Commit: 25121347e3f7ecf6c211875b39c0999331cbb4a4 Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
|
ARD dispositions for the six-finding review round at
The pre-push adversarial review's own two minor findings (overclaiming cwd comment; Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
Two execution-confirmed bugs from the review at b4ca847 (round 2): 1. PreInvocation crashed with an uncaught AttributeError and emitted no output at all when a hook's parsed `systemMessage` / `additionalContext` / nested `hookSpecificOutput.additionalContext` value was non-string (dict/list/number). The json.loads() reassigning `text_out` was inside a try/except, but the subsequent `.encode("utf-8")` was not, so `text_out.encode(...)` died on a dict with no JSON ever printed to stdout. Fixed by coercing a non-string truthy `text_out` via `str(...)`, mirroring the coercion the PreToolUse and Stop branches already apply to their own systemMessage/additionalContext reads, and by wrapping the byte-capping logic in its own try/except so any further surprise in that block still emits valid JSON instead of dying with a traceback. 2. The total-byte-cap boundary logic appended an empty `trimmed_chunk` unconditionally when `remaining_bytes` landed mid a multi-byte UTF-8 character: `errors="ignore"` drops the incomplete trailing byte(s), producing "", which was still appended as another empty `ephemeralMessage` step and added 0 bytes to the running total -- so every remaining hook (up to the message cap) contributed another empty step. Fixed by only appending when `trimmed_chunk` is non-empty, and breaking out of further accumulation once no more content can fit (remaining_bytes exhausted, or trimmed to empty). The exact-boundary behavior `test_pre_invocation_thirty_kb_exact_boundary` covers is unchanged. Also adds a code comment at each of the three `hook.get("command") or hook.get("script")` extraction sites noting that `script` is a legacy diagnostic basename (never a runnable command line) and `command` is the canonical field -- a non-blocking note from the same review, no behavior change.
Adds regression tests for both bugs fixed in the prior commit, each
verified to fail on the pre-fix code and pass on the fix (negative
controls run and reverted; see PR discussion for the transcripts):
- test_pre_invocation_non_string_additional_context_does_not_crash:
a hook returning `{"additionalContext": {"nested": "object"}}` must
not crash the adapter and must emit a coerced-to-string
ephemeralMessage instead.
- test_pre_invocation_multibyte_boundary_no_empty_ephemeral_messages:
reproduces the reviewer's scenario (hooks consuming 29999 of the
30000-byte total cap, then hooks returning 2-byte UTF-8 characters
that cannot fit in the single remaining byte) and asserts no empty
ephemeralMessage entries appear, that accumulation stops once nothing
more can fit, and that the byte total lands exactly at 29999.
…t, byte-cap continue) Correct the three extraction-site comments: `script` is load-bearing for install-hooks.py's non-plugin path per hooks.json's own header, not a legacy/informational field -- it is simply not runnable by this adapter. Change the byte-cap boundary handling so an unfittable multi-byte chunk skips itself (continue) instead of ending accumulation (break): a later hook whose output starts with a narrower code point can still fill the 1-3 leftover budget bytes. A full cap still breaks. The boundary regression test now asserts the leftover byte is actually used by a subsequent ASCII hook and that all hooks run.
|
Adversarial pre-push review — dispatched Summary of ChangesTwo commits on top of
Two new regression tests were added: one drives the exact scenario from the round-2 review (29999/30000 bytes consumed, then a hook returning Findings
Both fixes were verified to actually cause the failures they claim to fix: reverting Verdict: Ready for mergeReviewed-Commit: c917dfd Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
|
Adversarial micro confirmation review — dispatched Summary of ChangesSingle commit addressing two minor findings from a prior clean review of FindingsNo actionable findings identified. Verdict: Ready for mergeReviewed-Commit: 0bde41b Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
|
ARD dispositions for the two-bug review round at
Both pre-push adversarial rounds above are clean at the pushed head Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
…timeout Three review findings from PR #2233's pre-push review: 1. `invoke_subagent`'s `isolation` field was set unconditionally from Antigravity's `Workspace` concept (values like "share"/"branch"), which is not the same enum as Claude Code's `isolation` mode ("worktree"/"remote"). hooks/flag-unassigned-worktree.py gates its warning on the truthiness of `isolation`, so any non-empty Workspace value silently suppressed that warning for every subagent launch. Add normalize_isolation(), which maps through only "worktree"/"remote" (case-insensitively) and returns None otherwise; the raw Workspace value is preserved under a separate "workspace" key for any downstream consumer. 2. The PreInvocation JSON-parse fallback was the file's only `except Exception: pass` with no diagnostic, unlike every sibling parse handler. It now logs to stderr while keeping the same raw-text fallback behavior. 3. The cmd/timeout resolution block (command-vs-script lookup, ${CLAUDE_PLUGIN_ROOT} substitution, timeout parsing with a 30s default) was duplicated verbatim at three call sites (PreToolUse, Stop, PreInvocation). Extracted into resolve_cmd_and_timeout(), used at all three sites with identical default-timeout behavior. Adds discriminating tests in scripts/test_agy_hook_adapter.py for each fix (positive + negative cases for isolation mapping, a diagnostic assertion for the parse fallback, and a cross-event-type default-timeout check). Verified each by reverting its fix, confirming the new test fails, then restoring and confirming it passes.
…roup The PreToolUse matcher was widened from the literal "run_command" to the regex alternation "run_command|invoke_subagent|send_message| define_subagent|mcp__github__.*", but whether Antigravity treats `matcher` as a regex at all is unverified (memories/antigravity.md says so itself). Per the pre-push review's de-risk suggestion, split run_command back into its own hook group with the literal matcher "run_command" (preserving its exact hooks list, including enforce-mwc-review-gate.py), and put the new tool names in a second group carrying the regex alternation for just the new coverage. A wrong regex assumption now costs only the new coverage, never the pre-existing merge-control gate. Updates memories/antigravity.md's PreToolUse passage to describe the two-group shape and the rationale. scripts/test_agy_hook_adapter.py's adapter dispatch tests exercise hooks/hooks.json (mocked), not plugins/ai-config/hooks.json, so the dispatch path needed no test updates for this fix. The prior commit already added test_plugins_hooks_json_run_command_split_into_its_own_group, which reads plugins/ai-config/hooks.json directly and asserts the two-group shape (run_command literal, separate regex group, enforce-mwc-review-gate.py still attached to run_command); this commit is what makes that test pass. Verified by reverting hooks.json to the single-group shape, confirming that test fails, then restoring and confirming it passes.
|
Adversarial pre-push review — dispatched Summary of ChangesTwo commits on top of
FindingsNo actionable findings identified. Verification performed:
Non-blocking style observation (not filed as a finding): the two new sentences added to Verdict: Ready for mergeReviewed-Commit: 289af1b Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
|
ARD dispositions for the review round at
Pre-push adversarial review above is clean at the pushed head Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
|
Claude finished review — View run Final statement, without reference to local test-run countsCode review of HEAD The PR's check/CI state is a separate fact, established by VerdictCode review: no defects identified in HEAD 289af1b. Separately and explicitly: this is not a statement about the PR's CI/check state, which was incomplete per the last status query in this transcript and must be re-queried before any merge-readiness conclusion is drawn. Reviewed commit: 289af1b Stopping Point: Not a clean stopping point / work remains queued: re-run Reviewed commit: 289af1b |
|
Per the user's ruling in the driving session, clean independent adversarial Claude verdicts suffice for this session, superseding cursor[bot]'s standing verdict (whose findings were independently verified addressed in later clean rounds). At head Posted by Claude Code (AI agent) --- not written by a human. Generated by Claude Code |
No description provided.