feat: add Zero hook backend#43
Conversation
Zero automated PR reviewVerdict: No blockers found Blockers
Validation
ScopeHead: This deterministic review checks validation status and basic diff hygiene. A human reviewer still owns product judgment and design quality. |
anandh8x
left a comment
anandh8x
left a comment
There was a problem hiding this comment.
What's good
- Layered config (user + project) with
XDG_CONFIG_HOME/XDG_DATA_HOMEresolution, overrideable viacwd/envinjection — the test harness exploits this cleanly. - Zod schema validation with structured diagnostics (
io/json/schema/duplicate) — useful for surfacing errors without failing the whole load. - Project-overrides-user semantics with a
duplicatediagnostic when an id collides, and a stable alphabetical sort of merged hooks. - Atomic config writes via temp file +
renameinwriteZeroHooksConfig(src/zero-hooks/config.ts:130-136). redactZeroSecretsis applied to the JSON output ofhooks list --json(src/index.ts:444), consistent with the other redaction boundaries in the CLI.- Test coverage spans config layering, persistence round-trip via the store, selector matching, audit JSONL, and end-to-end CLI output as a subprocess.
Observations (non-blocking)
-
Audit append is not serialized.
ZeroHookAuditStore.append(src/zero-hooks/audit.ts:60-72) computessequenceviareadEvents().length + 1and thenappendFiles. Two concurrentappendStarted/appendCompletedcalls would compute the same sequence and the second writer's line would land out of order relative to sequence numbers — so the "stable sequence ordering" promised in the PR body isn't actually stable under concurrency. TheZeroMcpPermissionStorein #40 used awithWriteLockpromise chain for the same pattern; reusing the same shape (or in-process mutex around the read+append pair) would close it. -
ZeroHookConfigStore.listuser-bypass hack.new ZeroHookConfigStore({ configPath }).list()(src/zero-hooks/config.ts:154-159) passes${this.configPath}.user-missingto force the user layer to be absent, rather than reading a single file. It works but is opaque; aloadZeroHooksConfigFromPath(path)(or asource: 'project' | 'user'filter on the existing loader) would read more directly. -
CLI doesn't surface the audit store yet.
ZeroHookAuditStore.readEvents()exists and is tested, butzero hooks list(src/index.ts:431-449) only shows config. A follow-uphooks list --audit(orhooks audit) subcommand would make the audit file discoverable from the CLI without users hunting for~/.local/share/zero/hooks/audit.jsonl. -
Matcher support is
*-only.matchesHookMatcher(src/zero-hooks/config.ts:248-255) handles exact match and*wildcards only — no?/[abc]. Worth a one-line doc comment onZeroHookDefinition.matcherso users know what to expect.
No blockers. The race in #1 is the only one I'd want to see fixed before this lands in main.
|
@CodeRabbit review |
✅ Action performedReview finished.
|
WalkthroughAdds a zero hooks subsystem: types and barrel exports, layered config loading/merge and a CRUD store, selection/formatting utilities, a file-backed JSONL audit store, CLI "hooks list" command with redaction, and integration tests. ChangesZero Hooks Configuration and Audit System
Sequence Diagram(s)sequenceDiagram
participant CLI
participant loadZeroHooksConfig
participant formatZeroHookList
participant redactZeroString
CLI->>loadZeroHooksConfig: load config + diagnostics
loadZeroHooksConfig->>CLI: ZeroHookLoadResult
alt --json
CLI->>CLI: redactZeroSecrets(JSON)
else text
CLI->>formatZeroHookList: format config + diagnostics
formatZeroHookList->>redactZeroString: formatted text
redactZeroString->>CLI: redacted text
end
CLI->>CLI: write stdout/stderr or set exitCode
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (2)
tests/zero-hooks.test.ts (2)
31-55: ⚡ Quick winAnchor the spawned CLI path to the test file.
Line 35 resolves
src/index.tsfromprocess.cwd(), so this test can fail when Bun is launched from a subdirectory or an IDE runner. Resolve the entrypoint fromimport.meta.dirinstead to keep it deterministic.Suggested change
async function runZeroHooks( cwd: string, args: string[] = [] ): Promise<{ exitCode: number; stdout: string; stderr: string }> { - const child = Bun.spawn([process.execPath, join(process.cwd(), 'src/index.ts'), 'hooks', ...args], { + const cliEntrypoint = join(import.meta.dir, '..', 'src', 'index.ts'); + const child = Bun.spawn([process.execPath, cliEntrypoint, 'hooks', ...args], { cwd, env: { ...process.env, HOME: join(cwd, 'home'),As per coding guidelines,
tests/**/*.ts: Review tests for Bun test idioms, deterministic behavior, and meaningful coverage of safety decisions, provider/runtime flows, session events, and TUI command behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/zero-hooks.test.ts` around lines 31 - 55, The test's CLI spawn in runZeroHooks resolves 'src/index.ts' relative to process.cwd(), causing nondeterministic failures; update the spawn arguments to resolve the entrypoint using import.meta.dir (e.g., join(import.meta.dir, 'src/index.ts')) so the child process always launches the correct CLI from the test file's directory; keep the rest of runZeroHooks (env, pipes, and return structure) unchanged.
216-255: ⚡ Quick winAdd coverage for malformed audit lines.
This only proves the happy path. The audit reader's invalid-JSONL recovery is part of the backend contract in this PR, so a regression there would still pass this suite. Seed the file with one malformed line between two valid events and assert
readEvents()preserves the valid entries and ordering.As per coding guidelines,
tests/**/*.ts: Review tests for Bun test idioms, deterministic behavior, and meaningful coverage of safety decisions, provider/runtime flows, session events, and TUI command behavior.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/zero-hooks.test.ts` around lines 216 - 255, The current ZeroHookAuditStore test only covers the happy-path JSONL read/write flow, so add a malformed audit line case to exercise invalid-JSONL recovery. In the existing `appends and reads hook audit events as JSONL` test, seed the audit file with one bad line between two valid events, then verify `readEvents()` still returns only the valid entries in the correct order. Keep the assertions aligned with `ZeroHookAuditStore.readEvents()` behavior and the existing sequence/type checks.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/zero-hooks/audit.ts`:
- Around line 55-59: readEvents() currently throws on a malformed JSON line
which blocks append() because append() calls readEvents() to compute the next
sequence; change readEvents() to tolerate and recover from malformed lines when
used for append by skipping lines that fail JSON.parse (log a warning including
the line index and raw content) and continue building the events array so
sequence numbers can be computed; implement this either by adding an optional
parameter to readEvents(e.g., tolerateMalformed = false) and, when true (used by
append()), skip/record malformed lines instead of throwing, or by catching parse
errors inside the loop and continuing, ensuring append() still gets a correct
last sequence.
- Around line 64-76: The append method assigns sequence numbers by reading
current events then writing, which is racy under concurrent append calls;
serialize append operations in-process (e.g., add a private in-memory
mutex/queue on the class such as an appendLock or appendQueue) so that append()
obtains the lock, calls readEvents(), computes sequence, writes the file (mkdir
+ appendFile), and then releases the lock; ensure the lock is always released on
success or error so callers don't deadlock and keep using existing symbols
append, readEvents, auditPath, and now to locate and update the logic.
In `@src/zero-hooks/config.ts`:
- Around line 22-31: HookDefinitionSchema currently allows a "matcher" for all
events but selectZeroHooks ignores matched hooks when toolName is absent, which
silently makes sessionStart/sessionEnd hooks with a matcher impossible to
trigger; update HookDefinitionSchema to reject matcher for session hooks (or
emit a validation diagnostic) by adding a Zod refinement/superRefine on
HookDefinitionSchema that checks event (from HookEventSchema) and fails if event
is "sessionStart" or "sessionEnd" while matcher is present; also apply the same
validation change where similar schema logic exists (the other occurrence around
the 170-174 area) so any hook definitions for sessionStart/sessionEnd are
explicitly invalid when matcher is provided and produce a clear error message
referencing the field "matcher".
- Around line 124-160: The upsert/remove/setEnabled methods perform
read-modify-write against the same file (via list and writeZeroHooksConfig)
without any serialization, so concurrent commands can overwrite each other;
modify these methods (upsert, remove, setEnabled) to serialize mutations by
adding a file/operation lock or an optimistic-revision check around list ->
compute nextHooks -> writeZeroHooksConfig using this.configPath, e.g. obtain a
mutex or lock per configPath before calling list and release after
writeZeroHooksConfig, or include a revision/timestamp in the stored config and
retry the read-modify-write if the revision changed; ensure
normalizeHookDefinition, writeZeroHooksConfig and list are used within the
locked/transactional section and that methods return the same values on
success/failure as before.
- Around line 297-305: The matcher construction in matchesHookMatcher currently
builds an unbounded RegExp from hooks.json wildcards (via splitting and joining
to '.*'), which can be abused by many '*' segments; replace this by a safe
linear glob matcher: in matchesHookMatcher, split matcher on '*' and perform
sequential substring/anchor checks against toolName (handle leading/trailing '*'
as prefix/suffix rules) instead of new RegExp, or alternatively add strict
validation in the schema that enforces a maximum total length and a max count of
'*' before constructing any RegExp; update/selectZeroHooks tests to include
multi-'*' patterns and edge cases to cover the new behavior. Ensure you
reference the matchesHookMatcher function and the matcher validation where
hooks.json is parsed/validated.
---
Nitpick comments:
In `@tests/zero-hooks.test.ts`:
- Around line 31-55: The test's CLI spawn in runZeroHooks resolves
'src/index.ts' relative to process.cwd(), causing nondeterministic failures;
update the spawn arguments to resolve the entrypoint using import.meta.dir
(e.g., join(import.meta.dir, 'src/index.ts')) so the child process always
launches the correct CLI from the test file's directory; keep the rest of
runZeroHooks (env, pipes, and return structure) unchanged.
- Around line 216-255: The current ZeroHookAuditStore test only covers the
happy-path JSONL read/write flow, so add a malformed audit line case to exercise
invalid-JSONL recovery. In the existing `appends and reads hook audit events as
JSONL` test, seed the audit file with one bad line between two valid events,
then verify `readEvents()` still returns only the valid entries in the correct
order. Keep the assertions aligned with `ZeroHookAuditStore.readEvents()`
behavior and the existing sequence/type checks.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 451befde-e48f-4627-92d0-c21cf955d8cd
📒 Files selected for processing (6)
src/index.tssrc/zero-hooks/audit.tssrc/zero-hooks/config.tssrc/zero-hooks/index.tssrc/zero-hooks/types.tstests/zero-hooks.test.ts
|
Blockers
Non-Blocking
Looks Good
Validation
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Requesting changes because plain-text zero hooks list leaks raw secret-bearing hook args while the JSON path redacts them. Full details and validation are in my review comment.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/zero-hooks/audit.ts (1)
66-86: Append serialization looks correct; full-file read per append is a deferred scalability concern.The in-process
appendQueueresolves the sequence race, andmax(sequence)+1is robust against gaps. Note that each append re-reads and parses the entire audit file to compute the next sequence — that's O(n) per write / O(n²) over the log's lifetime. Fine while execution isn't wired, but cache the last sequence in memory (refreshed from disk on init) before this is exercised on a hot path. The per-instance queue also won't serialize twoZeroHookAuditStoreinstances (or processes) targeting the sameauditPath, so don't rely on it for cross-process ordering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/zero-hooks/audit.ts` around lines 66 - 86, Append currently re-reads the entire audit file in append() via readEvents() to compute sequence, causing O(n) per write; add an in-memory lastSequence property on ZeroHookAuditStore initialized by reading/parsing the file once (e.g., in the constructor or an async init method that calls readEvents() once) and use lastSequence+1 in append() instead of recomputing from readEvents(); ensure append() updates lastSequence after writing the event and keep existing appendQueue usage for intra-process serialization (note: this does not solve cross-process ordering for multiple instances writing to the same auditPath).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@src/zero-hooks/audit.ts`:
- Around line 66-86: Append currently re-reads the entire audit file in append()
via readEvents() to compute sequence, causing O(n) per write; add an in-memory
lastSequence property on ZeroHookAuditStore initialized by reading/parsing the
file once (e.g., in the constructor or an async init method that calls
readEvents() once) and use lastSequence+1 in append() instead of recomputing
from readEvents(); ensure append() updates lastSequence after writing the event
and keep existing appendQueue usage for intra-process serialization (note: this
does not solve cross-process ordering for multiple instances writing to the same
auditPath).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: db57a67f-dc02-4e48-b3f8-3ac6a449fd48
📒 Files selected for processing (4)
src/index.tssrc/zero-hooks/audit.tssrc/zero-hooks/config.tstests/zero-hooks.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/index.ts
|
Blockers Non-Blocking
Looks Good
Validation
|
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approved current head 3c61281 after verifying the hook-list redaction blocker and session-matcher/schema hardening are fixed, with local validation passing.
3 participants
Summary
zero hooks listin text and JSON formats so CLI users can inspect configured hooks and load diagnostics.Details
.zero/hooks.json.Validation
bunx tsc --noEmitbun test ./tests --timeout 15000(274 pass)bun run buildbun run smoke:build./zero --help./zero hooks list./zero hooks list --jsongit diff --check origin/main..HEADSummary by CodeRabbit
New Features
zero hooks listCLI subcommand with JSON and formatted text outputTests