refactor(hook): add typed notification pipeline - #2029
Merged
Conversation
Record the hook slice in the existing harness architecture goal instead of opening a second record. The slice covers the contract defect (event name and payload are two uncorrelated types), the four drifted Stop/SessionEnd implementations, the per-event settings and session reads paid by installations with no hook configured, the triple clone of untruncated tool payloads, and the non-deterministic command start order. It explicitly excludes hook decision semantics. Letting a user-configured shell command block, replace, cancel, or rewrite provider requests and tool arguments needs a versioned stdout protocol, a conflict and timeout policy, argument revalidation, a permission recheck after rewriting, and a trust surface. That is a product feature with its own security model, and DeepChat has no in-process participant that could use it today.
The hook contract was one flat all-optional context, so the event name and its
payload were uncorrelated: dispatch('Stop', { tool }) compiled, and adding an
event name failed no build. Replace it with a discriminated HookEvent union plus
two compile-time guards, one that fails when the union and HOOK_EVENT_NAMES
drift apart and one that pins the combinations the union must refuse.
RuntimeHookSink becomes a scope factory that binds the immutable session facts of
one turn, so no call site assembles an envelope and Stop plus SessionEnd has a
single implementation. The four previous copies had already drifted: the ACP path
dropped usage entirely.
Delivery defects fixed:
- HookService re-read hooksNotifications per event. That key is a sensitive app
setting, so each read was a database read, a JSON deep clone, a Zod
normalization and two JSON.stringify calls, paid twice per tool call even with
no hook configured. The service now owns its configuration and derived
subscription index, refreshed atomically on every write path.
- An unsubscribed event now costs one set lookup: no envelope resolution, no
projection, no clone, no settings read, no session read, no spawn.
- Tool events were structuredCloned three times before truncation, copying
multi-megabyte arguments and responses for a payload that keeps 1200
characters. Projection now truncates first and clones only the permission
record.
- Events were delivered through independent microtasks with per-event async
enrichment, so command start order did not match event order. Delivery is now
serialized per session and independent across sessions.
- Payload enrichment read the session row whenever workdir was falsy, so a
session with no project directory paid a read per event. Only an unanswered
field triggers a lookup now.
- The messageId to promptPreview fallback read a message row and reported the
assistant's own output as the user's prompt, because every producer passes an
assistant message id. The lookup and its query port are removed; previews come
from the producer that owns the prompt.
- observeTerminal resolved the project directory outside the sink's guard, so a
stale-instance assertion could propagate into run settlement. Resolution now
happens inside the guard and degrades to an unanswered field.
payloadVersion, the stdin payload shape, environment variables, command
placeholders, the command timeout and the settings contract are unchanged.
user.promptPreview is now empty where it previously carried assistant output,
and time is captured when the event occurs rather than when its payload is built.
Regenerate after the hook contract change. The report records two structural effects: the hook/observer.ts to hook/index.ts cycle is gone because the observer now depends on the neutral event contract instead of the service module, and interactionCoordinator drops one outgoing dependency after losing the session settings port it only used to resolve a hook envelope.
The subscription index and the delivery filter each decided independently whether a hook can run. A future edit to one and not the other would let the index advertise fewer events than the filter accepts, and an unadvertised event is dropped at the producer without any log line. Both now read the same predicate.
notify only decided whether the event was observed and then queued a projection; deliver re-read the current configuration. A hook enabled or edited between those two points therefore ran an event that happened before it existed. Reproduction: hook A subscribes to PostToolUse, notify accepts an event, the same tick calls updateConfig adding hook B, and B receives the earlier event with its prompt, tool arguments and response. Slow enrichment or a queued session widens the window. An accepted event now carries the id and command of every hook eligible at that moment, and a hook runs only when it is still eligible under the same command. A hook enabled or edited afterwards never receives an earlier event, one disabled meanwhile stops receiving queued ones, and editing a command while keeping the hook id does not run old events under the new command. Revalidation happens immediately before spawning rather than when the delivery is dequeued, because enrichment is asynchronous and eligibility can change across that await. A cheap pre-enrichment check keeps an event that nothing can consume from paying for a session lookup.
The command timeout only set a flag and sent SIGKILL; the promise still resolved on close or error. A SIGKILLed shell can leave a process tree holding the inherited stdio, so close may never arrive and testHookCommand from the settings page would stay pending forever. The timeout now settles its own result, and a later close only clears the timer and unregisters the child. Captured stdout and stderr also grew without limit for the whole timeout window before being truncated to 2000 characters, so a chatty hook could hold thirty seconds of output in the main process and concurrent hooks multiplied it. Capture now stops at the same limit the diagnostics are truncated to, which leaves the reported value identical because truncation keeps the head. The previous timeout test emitted close by hand, so it proved the kill and not the settlement. It now asserts the promise resolves with no close event at all.
buildConfigSnapshot froze the object the settings port returned, along with its hooks and their event arrays. HookSettings currently normalizes into a fresh object every read, so nothing observed it, but the freeze was an input side effect no signature declared: a port that returned shared or cached state would have had that state silently frozen from under it. The snapshot now copies before freezing, and getConfigSnapshot returns a mutable copy rather than the frozen internal one, which restores the pre-cache property that a caller could do anything with the value it received.
The union relied on excess property checking, which only refuses surplus fields on a fresh literal. Assigned to a variable first, an object carrying both stop and tool still satisfied HookEventBody, so the central contract of this slice did not hold on the one path a caller is most likely to take when building an event dynamically. Each variant is now closed against every fact it does not declare. The rejection guards previously proved only that a required fact cannot be missing; they now also pin foreign facts, including SessionEnd carrying stop and SessionStart carrying usage, neither of which the open union could reject.
Two claims overstated what the code does. The record described one scope binding the immutable facts of a turn. A turn actually creates three, because settlement reaches the sink from RunLifecycleCoordinator without the turn's scope, so envelopes are resolved at three points and a mid-turn setting change can still be observed by later events. Threading one scope through the run lifecycle adds a state carrier to that owner and is recorded as deferred instead. The terminal correction claimed usage no longer depends on the entry point. ACP still reports null because AcpObserverPort.terminal carries no token accounting; what the single projection removes is the drift between four implementations. Also records the configuration ownership rule, the timeout settlement and output bound, and adds acceptance criteria for each.
The review fixes changed no module graph, so the regenerated report differs only in the commit it was measured from.
Contributor
📝 WalkthroughWalkthroughChangesTyped Hook Notification Pipeline
Model Provider Catalog
Architecture Plans and Baselines
ACP Registry Update
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant AgentRuntime
participant RuntimeHookScope
participant HookService
participant HookCommand
AgentRuntime->>RuntimeHookScope: scope(session facts)
RuntimeHookScope->>HookService: emit typed HookEvent
HookService->>HookService: project, gate, and queue event
HookService->>HookCommand: execute accepted hook
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
test/main/hook/hookService.test.ts (2)
552-591: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
structuredClonespy can leak when an assertion fails.
clone.mockRestore()is the last statement in both tests, so a failingexpectabove it leaves the global spy installed for every subsequent test in the file. Restore in a hook instead.♻️ Proposed change
describe('HookService payload projection', () => { beforeEach(() => { spawnMock.mockReset() spawnMock.mockImplementation(() => { const child = new FakeChild() queueMicrotask(() => child.emit('close', 0)) return child }) }) + + afterEach(() => { + vi.restoreAllMocks() + })Then drop the trailing
clone.mockRestore()calls (remember to addafterEachto thevitestimport).🤖 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 `@test/main/hook/hookService.test.ts` around lines 552 - 591, Update the tests around “truncates tool previews before any clone runs” and “clones only the permission record” to restore the global structuredClone spy from an afterEach hook, adding afterEach to the Vitest imports. Remove the trailing clone.mockRestore() calls so cleanup still runs when an assertion fails.
238-245: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueThe same
spawnMockbeforeEachis repeated in all four describes.Hoisting it to a single file-level
beforeEach(or asetupSpawn()helper) removes four identical copies.Also applies to: 402-409, 543-550, 731-738
🤖 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 `@test/main/hook/hookService.test.ts` around lines 238 - 245, Deduplicate the repeated spawnMock setup across the four describe blocks by moving the shared mockReset/mockImplementation logic into one file-level beforeEach or a setupSpawn helper. Preserve creation of FakeChild instances and asynchronous close emission for every test, and remove the duplicate per-describe hooks.test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts (1)
52-73: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
createHookObserveris duplicated verbatim across two test files. Both files hand-roll the sameHookEvent→ dispatcher mapping and the samenoopHookObserver, so every newHookEventvariant must be mirrored in both.
test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts#L52-L73: movecreateHookObserverandnoopHookObserverinto a shared test helper module and import them here.test/main/session/runtimeIntegration.test.ts#L23-L44: import the shared helper instead of redefining it.🤖 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 `@test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts` around lines 52 - 73, Move the shared createHookObserver mapping and noopHookObserver definition into a common test helper, then remove the duplicated definitions and import the helper in test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts lines 52-73 and test/main/session/runtimeIntegration.test.ts lines 23-44. Preserve the existing HookObserver behavior and dispatcher payload mapping at both sites.
🤖 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/main/hook/index.ts`:
- Around line 479-480: Update the finalize paths in the child error handler and
stdin-write failure handler to wrap their truncateText(...) output with
redactSensitiveText(...), using the existing secrets value. Keep redaction
consistent with the timeout and close handlers before passing command output to
finalize(...).
---
Nitpick comments:
In `@test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts`:
- Around line 52-73: Move the shared createHookObserver mapping and
noopHookObserver definition into a common test helper, then remove the
duplicated definitions and import the helper in
test/main/agent/deepchat/harness/deepChatAgentHarness.test.ts lines 52-73 and
test/main/session/runtimeIntegration.test.ts lines 23-44. Preserve the existing
HookObserver behavior and dispatcher payload mapping at both sites.
In `@test/main/hook/hookService.test.ts`:
- Around line 552-591: Update the tests around “truncates tool previews before
any clone runs” and “clones only the permission record” to restore the global
structuredClone spy from an afterEach hook, adding afterEach to the Vitest
imports. Remove the trailing clone.mockRestore() calls so cleanup still runs
when an assertion fails.
- Around line 238-245: Deduplicate the repeated spawnMock setup across the four
describe blocks by moving the shared mockReset/mockImplementation logic into one
file-level beforeEach or a setupSpawn helper. Preserve creation of FakeChild
instances and asynchronous close emission for every test, and remove the
duplicate per-describe hooks.
🪄 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: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 15d1fdc4-f7b4-41d6-8164-03bdb5cab43c
📒 Files selected for processing (33)
docs/architecture/baselines/agent-system-layered-runtime-baseline.jsondocs/architecture/baselines/dependency-report.mddocs/architecture/baselines/main-kernel-boundary-baseline.mddocs/architecture/baselines/main-kernel-migration-scoreboard.jsondocs/architecture/baselines/main-kernel-migration-scoreboard.mddocs/architecture/baselines/zero-inbound-candidates.mddocs/architecture/deepchat-agent-harness-boundaries/plan.mddocs/architecture/deepchat-agent-harness-boundaries/spec.mddocs/architecture/deepchat-agent-harness-boundaries/tasks.mdresources/acp-registry/registry.jsonresources/model-db/providers.jsonsrc/main/agent/acp/compatibility/dependencies.tssrc/main/agent/deepchat/harness/createDeepChatAgentHarness.tssrc/main/agent/deepchat/loop/notificationObserver.tssrc/main/agent/deepchat/loop/ports.tssrc/main/agent/deepchat/runtime/deepChatLoopRunner.tssrc/main/agent/deepchat/runtime/interactionCoordinator.tssrc/main/agent/deepchat/runtime/runtimeHookSink.tssrc/main/agent/deepchat/runtime/turnCoordinator.tssrc/main/app/composition.tssrc/main/hook/events.tssrc/main/hook/index.tssrc/main/hook/observer.tssrc/main/hook/routes.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/agent/deepchat/runtime/dispatch.test.tstest/main/agent/deepchat/runtime/process.test.tstest/main/agent/deepchat/runtime/runtimeHookSink.test.tstest/main/app/compositionBoundaries.test.tstest/main/evals/nativeAgent/harness.tstest/main/hook/hookService.test.tstest/main/routes/dispatcher.test.tstest/main/session/runtimeIntegration.test.ts
Only the close and timeout paths ran hook stdout and stderr through redactSensitiveText. The spawn-error path and the stdin-write-failure path formatted the same output with truncateText alone, so a hook that had already written a bearer token, a Discord or Telegram webhook URL, the session id or the workspace path to stderr surfaced it unredacted in the settings test panel when the command then failed to start or could not be fed its payload. Hoisting `secrets` in the timeout change made the omission reachable but not obvious, so all four paths now report through one diagnostics() helper rather than repeating the formatting. Repetition is what let the two paths drift apart.
The structuredClone spies were restored by a trailing mockRestore(), so any failing assertion above it left the spy installed on globalThis for every later case in the file. Restoration moves to an afterEach hook that runs regardless. The identical spawn stub was also declared in four describe blocks, and the HookEvent to dispatcher mapping was duplicated verbatim between the harness and runtime integration suites, where every new event variant had to be mirrored in both. The stub becomes one file-level hook and the mapping one shared fixture.
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/main/hook/index.ts (1)
510-525: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRelease children on early settlement.
The timeout and stdin-write-failure paths kill the child and call
finalize, but never remove it fromactiveChildren. Ifclosedoes not arrive—as this timeout path explicitly anticipates—repeated failures retain staleChildProcessreferences and makestop()revisit them indefinitely.Add
this.activeChildren.delete(child)in both paths, or centralize that cleanup infinalize.🧹 Proposed cleanup
const timeout = setTimeout(() => { timedOut = true try { child.kill('SIGKILL') } catch { // ignore } + this.activeChildren.delete(child) // A killed shell can keep its process tree alive and never emit `close`, so the timeout // settles the result itself instead of waiting for an exit that may never arrive.Apply the equivalent cleanup after the stdin-write-failure
child.kill(...)call.Also applies to: 559-580
🤖 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/main/hook/index.ts` around lines 510 - 525, Remove the child from activeChildren whenever execution settles before the close event: add this.activeChildren.delete(child) in both the timeout handler around finalize and the stdin-write-failure path after child.kill(...), or centralize equivalent cleanup in finalize while preserving normal close handling.
🤖 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.
Outside diff comments:
In `@src/main/hook/index.ts`:
- Around line 510-525: Remove the child from activeChildren whenever execution
settles before the close event: add this.activeChildren.delete(child) in both
the timeout handler around finalize and the stdin-write-failure path after
child.kill(...), or centralize equivalent cleanup in finalize while preserving
normal close handling.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: cbd12251-fad3-4136-8e45-5bb74992c7e7
📒 Files selected for processing (5)
src/main/hook/index.tstest/main/agent/deepchat/harness/deepChatAgentHarness.test.tstest/main/hook/hookObserverFixture.tstest/main/hook/hookService.test.tstest/main/session/runtimeIntegration.test.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/main/hook/hookService.test.ts
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
1 participant
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.
Summary
This PR replaces the untyped hook fan-out with a typed, deterministic notification pipeline.
HookEventdiscriminated union with compile-time coverage and invalid-combination guards.RuntimeHookScopeemitters and a singleStop→SessionEndprojection.HookService.messageId→ prompt fallback that exposed assistant output as user input.nullproject directories.closeand bound captured stdout/stderr.The design and deferred decisions are recorded in:
docs/architecture/deepchat-agent-harness-boundaries/Motivation
The previous hook contract used an event name plus an all-optional context, so invalid combinations compiled and adding an event could silently escape exhaustive handling.
The delivery path also had several correctness and reliability problems:
StopandSessionEndwere assembled independently in four runtime paths.user.promptPreview.Delivery semantics
Compatibility
The following public behavior remains unchanged:
payloadVersion: 1Intentional value corrections:
user.promptPreviewis populated only when supplied by the producer; tool events and resumedSessionStartevents no longer contain assistant output.timeis captured when the event is accepted rather than after asynchronous enrichment.usage: nullbecause its terminal port has no canonical token accounting.Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores