Skip to content

feat(hooks): user-configurable lifecycle hooks (external command, 3-surface)#114

Merged
cnjack merged 3 commits into
mainfrom
feat/hooks
Jul 5, 2026
Merged

feat(hooks): user-configurable lifecycle hooks (external command, 3-surface)#114
cnjack merged 3 commits into
mainfrom
feat/hooks

Conversation

@cnjack

@cnjack cnjack commented Jul 5, 2026

Copy link
Copy Markdown
Owner

What

A user-configurable hook system: run your own commands at key points in the agent loop — before/after a tool runs, on prompt submit, on session start, and when the agent is about to stop. Same model as Claude Code / codex (JSON over stdin/stdout, exit-code control, matcher scoping).

Events: SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure, Stop.

Why / design

Full design in internal-doc/hooks-design.md. Key choices:

  • Transport-agnostic. One internal/hooks.Dispatcher is injected via context, so TUI, Web and ACP all share a single implementation instead of wiring hooks per surface.
  • Mounted at the middleware/runner layer, not the per-transport event handler — that's where tool approval/execution already converges, and it's the only layer that can deny / rewrite args / modify results.
  • Tool hooks = two eino middlewares around the approval gate. pre sits outside approval (so PreToolUse can deny, rewrite args, or pre-approve to skip the prompt); post sits inside (so it sees the real tool error and can rewrite the result).
  • Unified continuation loop. The runner's existing todo-guard and goal-guard auto-continuation loops merge with the new Stop hook into one pipeline with a single umbrella budget, one cancellation check, and stop_hook_active loop-prevention — instead of three independent loops that could compound or bypass each other.

Security

A hook is arbitrary code execution the instant its event fires (SessionStart runs on startup). So only ~/.jcode/hooks.json (user layer) loads by default; project layers (.jcode/hooks.json[.local]) require JCODE_HOOKS_TRUST_PROJECT=1. This closes a clone-and-run RCE / approval-bypass vector. Trust-on-first-use (per-hook hash confirmation) is the planned follow-up.

Notes for reviewers

  • Matchers are exact-by-default ("write" does not also match todowrite); regex opts in via metacharacters ("mcp__.*"). jcode tool names plus Claude Code aliases (Bash/Write/…) both match.
  • Fail-safe throughout: a hook that times out, crashes, or returns an unexpected exit code never blocks the agent.
  • Derived from an adversarial review pass — two high-severity findings (project-hook RCE, Web/ACP not wired) were fixed here.

Testing

  • 28 unit/integration tests: dispatcher (real subprocesses), middleware (deny / updatedInput / pre-approve / modifiedResult), continuation precedence & bounding, matcher, 3-layer config.
  • 2 real-LLM agent-eval cases (hook_pretooluse_deny, hook_posttooluse_side_effect).
  • Public docs at site/docs/overview/hooks.md with 8 use-case snippets, each verified against sample payloads.

Not in this PR (follow-ups)

Trust-on-first-use UI, /hooks management panel, Pre/PostCompact events, surfacing hook SystemMessage in the UI, teammate/subagent hook coverage.

Generated with Jack AI bot

Summary by CodeRabbit

  • New Features

    • Added user-configurable hooks for session start, prompt submission, tool use, stop, and compaction events.
    • Tool hooks can now block actions, adjust inputs/results, and add extra context during runs.
    • Hooks now apply consistently across interactive, agent, web, and continuation flows.
  • Documentation

    • Added full hook setup and usage docs, including configuration, matching rules, safety behavior, and examples.
  • Tests

    • Expanded test coverage for hook loading, matching, execution, approvals, and continuation behavior.

…urface)

Add a hook system that runs user-configured external commands at key points
of the agent loop, mirroring the Claude Code / codex model (JSON over
stdin/stdout, exit-code control, matcher scoping).

Events: SessionStart, UserPromptSubmit, PreToolUse, PostToolUse,
PostToolUseFailure, Stop.

Design (see internal-doc/hooks-design.md): a transport-agnostic
internal/hooks.Dispatcher is injected via context, so TUI, Web and ACP all
share one implementation. Tool hooks mount as two eino middlewares around the
approval gate (pre outside so PreToolUse can deny / rewrite args / pre-approve;
post inside so it sees the real tool error). The runner's todo/goal
auto-continuation loops merge with the Stop hook into a single continuation
pipeline with one umbrella budget, one cancellation check, and stop_hook_active
loop prevention.

Security: only the user layer (~/.jcode/hooks.json) loads by default; project
layers (.jcode/hooks.json[.local]) require JCODE_HOOKS_TRUST_PROJECT=1, since a
hook is arbitrary code execution the moment its event fires — closing a
clone-and-run RCE / approval-bypass vector. Trust-on-first-use is the follow-up.

Matchers are exact-by-default (so "write" doesn't also hit todowrite); regex
opts in via metacharacters. Fail-safe throughout: timeout/crash/bad exit never
blocks the agent.

Tested: 28 unit/integration tests (dispatcher, middleware, continuation,
matcher, config); 2 real-LLM agent-eval cases; public docs at
site/docs/overview/hooks.md with verified use-case snippets.

Generated with Jack AI bot
@coderabbitai

coderabbitai Bot commented Jul 5, 2026

Copy link
Copy Markdown

Review Change Stack

Warning

Review limit reached

@cnjack, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 39 minutes

Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available.
You're only billed for reviews past your plan's rate limits ($0.25/file).

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: ef6fbf7f-eb65-4882-b60f-60c17a2ddf51

📥 Commits

Reviewing files that changed from the base of the PR and between 5a3b4a9 and fc76b11.

📒 Files selected for processing (10)
  • internal-doc/hooks-design.md
  • internal/command/acp.go
  • internal/command/interactive.go
  • internal/hooks/config.go
  • internal/hooks/dispatcher.go
  • internal/hooks/exec.go
  • internal/hooks/proc_other.go
  • internal/hooks/proc_unix.go
  • internal/hooks/types.go
  • internal/runner/continuation_test.go
📝 Walkthrough

Walkthrough

This PR introduces a new internal/hooks package implementing a command-based hook system for tool-use and lifecycle events (SessionStart, UserPromptSubmit, PreToolUse, PostToolUse, PostToolUseFailure, PreCompact, PostCompact, Stop). It wires hook dispatch into agent middleware, approval flow, runner continuation logic, and transport entry points (ACP, interactive TUI, web server), and adds design docs, user docs, and eval test cases.

Changes

Hooks feature

Layer / File(s) Summary
Design document
internal-doc/hooks-design.md
Documents the hook paradigm, mount points, event set, config/execution protocol, internal architecture, trust model, phased implementation plan, and design rationale.
Core types and context helpers
internal/hooks/types.go, internal/hooks/context.go
Defines Event, Permission, Payload, Decision, HookSpec, HookGroup, Config types and context helpers for attaching a dispatcher/pre-approval flag.
Tool matcher logic
internal/hooks/match.go
Implements alias resolution, cached regex compilation, and exact-vs-regex matcher semantics for tool names.
Config loading
internal/hooks/config.go
Loads/merges layered hooks.json files (user/project/local), gates project trust via env var, and provides Config.Empty().
Hook command execution
internal/hooks/exec.go
Runs hook commands via shell with timeout/fail-safe handling, exit-code interpretation, structured stdout parsing, and env variable injection.
Dispatcher orchestration
internal/hooks/dispatcher.go
Implements Dispatcher, NewDispatcher, Fire folding logic, async hook handling, permission precedence, and no-op dispatcher.
Hooks package tests
internal/hooks/hooks_test.go
Covers matcher, config merge/trust/warnings, and dispatcher deny/allow/rewrite/timeout/cancellation/Stop behavior.
Agent hook middleware
internal/agent/agent.go, internal/agent/hook_middleware.go, internal/agent/hook_middleware_test.go
Adds pre/post tool-use hook middleware around approval middleware, with unit tests.
Approval short-circuit and unified continuation loop
internal/runner/approval.go, internal/runner/runner.go, internal/runner/continuation_test.go
Skips approval prompting when pre-approved; unifies todo/goal continuation with Stop hook via continuationSource.
Transport wiring
internal/command/acp.go, internal/command/interactive.go, internal/web/server.go
Wires session hook dispatchers into ACP, interactive TUI (SessionStart/UserPromptSubmit), and web server run paths.
User docs and eval test cases
site/docs/overview/hooks.md, agent-eval/suite/testcases.json
Documents hooks configuration/contract/examples and adds PreToolUse deny and PostToolUse side-effect eval test cases.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
    participant Runner
    participant PreHookMiddleware
    participant Dispatcher
    participant HookCommand
    participant ApprovalMiddleware
    participant Tool
    participant PostHookMiddleware

    Runner->>PreHookMiddleware: invoke tool call
    PreHookMiddleware->>Dispatcher: Fire(PreToolUse, payload)
    Dispatcher->>HookCommand: run configured hook via shell
    HookCommand-->>Dispatcher: exit code + stdout/stderr
    Dispatcher-->>PreHookMiddleware: Decision (deny/allow/updatedInput)
    alt Decision denied
        PreHookMiddleware-->>Runner: return deny message
    else Decision allow/no-op
        PreHookMiddleware->>ApprovalMiddleware: forward call (pre-approved flag optional)
        ApprovalMiddleware->>Tool: execute tool
        Tool-->>ApprovalMiddleware: result/error
        ApprovalMiddleware->>PostHookMiddleware: pass result
        PostHookMiddleware->>Dispatcher: Fire(PostToolUse/Failure, payload)
        Dispatcher->>HookCommand: run configured hook
        HookCommand-->>Dispatcher: exit code + stdout/stderr
        Dispatcher-->>PostHookMiddleware: Decision (modifiedResult/context)
        PostHookMiddleware-->>Runner: final result
    end
Loading
sequenceDiagram
    participant Runner
    participant TodoGoalState
    participant HooksDispatcher
    participant StopHookCommand

    Runner->>Runner: continuationSource(todo, goal, stopBlock)
    alt todo incomplete
        Runner->>TodoGoalState: continue lap (todo)
    else goal active
        Runner->>TodoGoalState: continue lap (goal)
    else Stop hook configured
        Runner->>HooksDispatcher: Fire(Stop, payload)
        HooksDispatcher->>StopHookCommand: run Stop hook
        StopHookCommand-->>HooksDispatcher: exit code / continue flag
        HooksDispatcher-->>Runner: Decision (block/continue)
        alt Decision blocks
            Runner->>Runner: continue lap (stop_hook_active)
        else Decision allows
            Runner->>Runner: finish run
        end
    else no mechanism requests continuation
        Runner->>Runner: finish run
    end
Loading

Possibly related PRs

  • cnjack/jcode#3: Both PRs modify the tool-approval flow in internal/runner/approval.go.
  • cnjack/jcode#106: Both PRs modify agent-eval/suite/testcases.json, adding related eval test cases.
  • cnjack/jcode#107: Both PRs modify the approval decision flow in internal/runner/approval.go.
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: a user-configurable external-command hook system integrated across lifecycle surfaces.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch feat/hooks

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

exec.CommandContext SIGKILLs sh on deadline, but a grandchild it spawned keeps the stdout pipe open, so cmd.Wait blocks until the grandchild exits and defeats the timeout. WaitDelay force-closes the pipes so runHook returns promptly. Fixes TestFireTimeoutFailSafe on Linux CI.

Generated with Jack AI bot

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 10

🧹 Nitpick comments (2)
internal/hooks/config.go (1)

86-89: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Prefix the wrapped JSON error.

Keep wrapped errors tagged with the subsystem so callers that receive loadFile errors directly still get useful context.

Proposed fix
-	if err := json.Unmarshal(data, &c); err != nil {
-		return Config{}, fmt.Errorf("invalid JSON: %w", err)
+	if err := json.Unmarshal(data, &c); err != nil {
+		return Config{}, fmt.Errorf("hooks: invalid JSON: %w", err)
 	}

As per coding guidelines, use fmt.Errorf("tool_name: %w", err) for wrapped errors in non-tool code.

🤖 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 `@internal/hooks/config.go` around lines 86 - 89, The JSON unmarshal error in
loadFile is wrapped without subsystem context; update the error returned from
the json.Unmarshal path in Config loading to prefix the wrapped error with the
tool/subsystem name while still using %w. Keep the fix localized to the loadFile
logic in internal/hooks/config.go, so direct callers receive a tagged error like
the rest of non-tool code.

Source: Coding guidelines

internal/runner/continuation_test.go (1)

6-6: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Local const cap shadows the builtin cap().

Harmless here since the builtin isn't used in this scope, but the repo's linter config enables revive/gocritic, some of which flag redefined predeclared identifiers. Trivial rename avoids the risk.

🧹 Suggested rename
 func TestContinuationSource(t *testing.T) {
-	const cap = 3
+	const todoCapConst = 3
 	cases := []struct {
 		name           string
 		todoIncomplete bool
 		todoUsed       int
 		goalActive     bool
 		stopBlock      bool
 		want           string
 	}{
-		{"todo wins over goal and stop", true, 0, true, true, "todo"},
-		{"todo within cap", true, 2, false, false, "todo"},
-		{"todo exhausted falls to goal", true, cap, true, false, "goal"},
-		{"todo exhausted falls to stop", true, cap, false, true, "stop"},
+		{"todo wins over goal and stop", true, 0, true, true, "todo"},
+		{"todo within cap", true, 2, false, false, "todo"},
+		{"todo exhausted falls to goal", true, todoCapConst, true, false, "goal"},
+		{"todo exhausted falls to stop", true, todoCapConst, false, true, "stop"},
 		{"goal beats stop", false, 0, true, true, "goal"},
 		{"stop only", false, 0, false, true, "stop"},
 		{"nothing continues", false, 0, false, false, ""},
 		{"todo cap zero skips todo", true, 0, false, true, "stop"},
 	}
 	for _, c := range cases {
 		t.Run(c.name, func(t *testing.T) {
-			todoCap := cap
+			todoCap := todoCapConst
🤖 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 `@internal/runner/continuation_test.go` at line 6, The local const named cap in
continuation_test.go shadows the builtin cap(), which can trigger
revive/gocritic lint warnings. Rename the constant in the relevant test scope to
a non-predeclared identifier (for example, something test-specific) and update
its uses in the surrounding continuation test setup.
🤖 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 `@internal-doc/hooks-design.md`:
- Around line 80-84: The fenced code blocks in hooks-design.md are missing
language annotations, which triggers markdownlint MD040. Update the affected
fences in the documented sections, including the block around the hookMiddleware
diagram and the other referenced blocks, by adding the closest appropriate
language tag such as text so the markdown linter passes.
- Around line 217-220: The user-level hooks config path is incorrect in the
three-layer merge description. Update the hooks configuration list in
hooks-design.md so the user-level entry matches the implemented loader and PR
intent by using the `~/.jcode/hooks.json` path; keep the project and local
entries unchanged, and make sure the `hooks` terminology stays consistent with
the existing `.jcode` config layout.

In `@internal/command/acp.go`:
- Around line 673-675: The ACP prompt hook dispatcher is being attached too late
in the flow, so `SessionStart` and `UserPromptSubmit` cannot affect the prompt
before it is persisted. Move the
`hooks.NewSessionDispatcher`/`hooks.WithDispatcher` setup earlier in `acp.go`,
before `RecordUser` and `sess.history` updates, and make sure `SessionStart` is
fired once and `UserPromptSubmit` runs before any prompt recording. Use the
existing `promptCtx`, `sess`, and `config.Logger().Printf` wiring so hook
behavior matches the TUI path.
- Around line 673-675: ACP hook dispatcher setup should not assume a recorder
exists, because sess.rec is optional and sess.rec.UUID() can panic before
runner.Run. Update the hook metadata construction around hooks.WithDispatcher
and hooks.NewSessionDispatcher to guard access to the recorder, using a safe
fallback or skipping recorder-derived fields when sess.rec is nil, while
preserving the existing ACP hook behavior when a recorder is present.

In `@internal/command/interactive.go`:
- Around line 1007-1012: The hook dispatcher is being created before the resume
path restores the recorder UUID, so hook payloads and SessionStart can be bound
to the temporary session instead of the resumed one. In interactive.go, update
the flow around NewSessionDispatcher, WithDispatcher, and the SessionStart
dispatch so they run only after resume handling has called
st.rec.SetUUID(resumeUUID), or rebuild st.hookDisp and st.ctx immediately after
that UUID swap. Keep the dispatcher/session-start wiring aligned with the final
recorder UUID used by the resumed session.

In `@internal/hooks/dispatcher.go`:
- Around line 117-121: The hook dispatch path in dispatcher.go currently appends
and runs every HookSpec as a command, regardless of HookSpec.Type. Update the
filtering in the hook collection/execution flow around matchesTool so only specs
with type "command" are executed, and treat empty type only if you intend to
keep backward-compatible default behavior. For any unknown type, log a warning
and skip it before execution.

In `@internal/hooks/exec.go`:
- Around line 47-56: The hook execution in execHook is not fully torn down on
timeout and its stdout/stderr capture is unbounded. Update the
exec.CommandContext setup so the shell runs in its own process group and ensure
timeout cleanup kills the whole group, not just the parent shell. Replace the
plain bytes.Buffer capture in execHook with capped/truncating output handling
for both stdout and stderr, and keep the existing context timeout, cwd, and env
behavior intact.

In `@internal/hooks/hooks_test.go`:
- Around line 201-213: The timeout path in internal/hooks/exec.go is only
canceling the shell wrapper, so child processes can survive and keep the hook
running past the limit. Update the hook execution in the relevant exec helper to
terminate the full process tree on timeout, using process-group cleanup or
cmd.WaitDelay/custom cmd.Cancel, and keep the fail-safe allow behavior exercised
by TestFireTimeoutFailSafe.

In `@internal/web/server.go`:
- Around line 827-830: Move hook execution earlier in the web prompt flow so it
matches the TUI path: in the server handler around runner.Run, create and apply
the session dispatcher from hooks.NewSessionDispatcher before any user message
is recorded or appended to history. Ensure SessionStart runs once up front, then
invoke UserPromptSubmit before RecordUser and the history update, so hooks can
deny or modify the prompt before it reaches runner.Run.

In `@site/docs/overview/hooks.md`:
- Around line 46-56: The hooks overview omits the `PreCompact` and `PostCompact`
events even though `internal/hooks/types.go` exposes them and `Trigger` can
dispatch them. Update the events table and surrounding copy in the hooks docs to
mention all 8 events, adding entries for `PreCompact` and `PostCompact` with
their firing timing and typical use so `hooks.json` users can configure them
correctly.

---

Nitpick comments:
In `@internal/hooks/config.go`:
- Around line 86-89: The JSON unmarshal error in loadFile is wrapped without
subsystem context; update the error returned from the json.Unmarshal path in
Config loading to prefix the wrapped error with the tool/subsystem name while
still using %w. Keep the fix localized to the loadFile logic in
internal/hooks/config.go, so direct callers receive a tagged error like the rest
of non-tool code.

In `@internal/runner/continuation_test.go`:
- Line 6: The local const named cap in continuation_test.go shadows the builtin
cap(), which can trigger revive/gocritic lint warnings. Rename the constant in
the relevant test scope to a non-predeclared identifier (for example, something
test-specific) and update its uses in the surrounding continuation test setup.
🪄 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

Run ID: d0208931-0306-4d94-9fc8-0fe42a34dc56

📥 Commits

Reviewing files that changed from the base of the PR and between 46e56a7 and 5a3b4a9.

📒 Files selected for processing (19)
  • agent-eval/suite/testcases.json
  • internal-doc/hooks-design.md
  • internal/agent/agent.go
  • internal/agent/hook_middleware.go
  • internal/agent/hook_middleware_test.go
  • internal/command/acp.go
  • internal/command/interactive.go
  • internal/hooks/config.go
  • internal/hooks/context.go
  • internal/hooks/dispatcher.go
  • internal/hooks/exec.go
  • internal/hooks/hooks_test.go
  • internal/hooks/match.go
  • internal/hooks/types.go
  • internal/runner/approval.go
  • internal/runner/continuation_test.go
  • internal/runner/runner.go
  • internal/web/server.go
  • site/docs/overview/hooks.md

Comment thread internal-doc/hooks-design.md Outdated
Comment thread internal-doc/hooks-design.md
Comment thread internal/command/acp.go Outdated
Comment on lines +673 to +675
// Inject the hook dispatcher so PreToolUse/PostToolUse/Stop hooks run on ACP
// too (parity with the TUI); reloaded per turn so hooks.json edits hot-apply.
promptCtx = hooks.WithDispatcher(promptCtx, hooks.NewSessionDispatcher(config.ConfigDir(), sess.env.Pwd(), sess.rec.UUID(), config.Logger().Printf))

@coderabbitai coderabbitai Bot Jul 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Wire ACP prompt-level hooks before persisting the prompt.

This dispatcher is attached only after RecordUser and sess.history updates, so UserPromptSubmit cannot block/augment ACP prompts and SessionStart context is never injected. That leaves ACP able to bypass prompt hook policies enforced by the TUI. Create/reuse the dispatcher earlier, fire SessionStart once, and run UserPromptSubmit before recording/history updates.

🤖 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 `@internal/command/acp.go` around lines 673 - 675, The ACP prompt hook
dispatcher is being attached too late in the flow, so `SessionStart` and
`UserPromptSubmit` cannot affect the prompt before it is persisted. Move the
`hooks.NewSessionDispatcher`/`hooks.WithDispatcher` setup earlier in `acp.go`,
before `RecordUser` and `sess.history` updates, and make sure `SessionStart` is
fired once and `UserPromptSubmit` runs before any prompt recording. Use the
existing `promptCtx`, `sess`, and `config.Logger().Printf` wiring so hook
behavior matches the TUI path.

🩺 Stability & Availability | 🔴 Critical | ⚡ Quick win

Guard optional ACP recorders before building hook metadata.

Line 675 unconditionally calls sess.rec.UUID(), but this file treats the recorder as optional elsewhere. ACP sessions without a recorder will panic before runner.Run.

🐛 Proposed fix
-	promptCtx = hooks.WithDispatcher(promptCtx, hooks.NewSessionDispatcher(config.ConfigDir(), sess.env.Pwd(), sess.rec.UUID(), config.Logger().Printf))
+	hookSessionID := string(params.SessionId)
+	if sess.rec != nil {
+		hookSessionID = sess.rec.UUID()
+	}
+	promptCtx = hooks.WithDispatcher(promptCtx, hooks.NewSessionDispatcher(config.ConfigDir(), sess.env.Pwd(), hookSessionID, config.Logger().Printf))
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
// Inject the hook dispatcher so PreToolUse/PostToolUse/Stop hooks run on ACP
// too (parity with the TUI); reloaded per turn so hooks.json edits hot-apply.
promptCtx = hooks.WithDispatcher(promptCtx, hooks.NewSessionDispatcher(config.ConfigDir(), sess.env.Pwd(), sess.rec.UUID(), config.Logger().Printf))
// Inject the hook dispatcher so PreToolUse/PostToolUse/Stop hooks run on ACP
// too (parity with the TUI); reloaded per turn so hooks.json edits hot-apply.
hookSessionID := string(params.SessionId)
if sess.rec != nil {
hookSessionID = sess.rec.UUID()
}
promptCtx = hooks.WithDispatcher(promptCtx, hooks.NewSessionDispatcher(config.ConfigDir(), sess.env.Pwd(), hookSessionID, config.Logger().Printf))
🤖 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 `@internal/command/acp.go` around lines 673 - 675, ACP hook dispatcher setup
should not assume a recorder exists, because sess.rec is optional and
sess.rec.UUID() can panic before runner.Run. Update the hook metadata
construction around hooks.WithDispatcher and hooks.NewSessionDispatcher to guard
access to the recorder, using a safe fallback or skipping recorder-derived
fields when sess.rec is nil, while preserving the existing ACP hook behavior
when a recorder is present.

✅ Confirmed as addressed by @cnjack

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fixed the panic — guard sess.rec and fall back to params.SessionId. The prompt-level part (UserPromptSubmit/SessionStart on ACP) is deferred to a documented follow-up (design §11): tool + Stop hooks already run on ACP, and project hooks are off by default, so it is an incompleteness rather than an escalation.

🤖 Addressed by Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cnjack, sounds good — the guard for sess.rec with fallback to params.SessionId matches what's now in the file, so that panic risk is resolved.

On the deferral of UserPromptSubmit/SessionStart wiring for ACP: that's a reasonable call given tool + Stop hooks already run on ACP and project hooks are off by default. Documenting it as a known gap in the design doc is a good way to track it so it doesn't get lost. Thanks for clarifying the scope.

Comment thread internal/command/interactive.go
Comment thread internal/hooks/dispatcher.go
Comment thread internal/hooks/exec.go Outdated
Comment thread internal/hooks/hooks_test.go
Comment thread internal/web/server.go
Comment on lines +827 to +830
// Inject the hook dispatcher so PreToolUse/PostToolUse/Stop hooks run on the
// Web surface too (parity with the TUI); reloaded per turn for hot-apply.
hookCtx := hooks.WithDispatcher(runCtx, hooks.NewSessionDispatcher(config.ConfigDir(), eng.env.Pwd(), recorder.UUID(), config.Logger().Printf))
resp := runner.Run(hookCtx, agent, history, eng.eventHandler, recorder, eng.todoStore, eng.env.GoalStore, s.tracer, eng.tokenUsage)

@coderabbitai coderabbitai Bot Jul 5, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift

Run web prompt hooks before recording/history updates.

Line 829 only makes runner-level hooks available after the web path has already emitted/recorded the user message and appended history. UserPromptSubmit cannot deny/augment web prompts, and SessionStart context is never applied, so web bypasses prompt hook policies unlike the TUI. Build/fire the dispatcher earlier, apply SessionStart once, and execute UserPromptSubmit before RecordUser and history append.

🤖 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 `@internal/web/server.go` around lines 827 - 830, Move hook execution earlier
in the web prompt flow so it matches the TUI path: in the server handler around
runner.Run, create and apply the session dispatcher from
hooks.NewSessionDispatcher before any user message is recorded or appended to
history. Ensure SessionStart runs once up front, then invoke UserPromptSubmit
before RecordUser and the history update, so hooks can deny or modify the prompt
before it reaches runner.Run.

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Deferred (documented in design §11): tool + Stop hooks already run on Web (dispatcher injected into the run ctx). Wiring UserPromptSubmit/SessionStart on Web/ACP needs surface-specific deny semantics + a per-session SessionStart lifecycle. Project hooks are off by default (only the user own ~/.jcode/hooks.json), so this is an incompleteness, not a policy bypass.

🤖 Addressed by Claude Code

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@cnjack, that's a fair distinction — thanks for the clarification. Tool-level and Stop hooks already provide the main safety/observability surface on Web, and gating UserPromptSubmit/SessionStart behind proper per-session lifecycle + deny semantics rather than bolting it on ad hoc is the right call. Since project-level hooks require explicit opt-in via JCODE_HOOKS_TRUST_PROJECT=1, the exposure here is limited to a user's own trusted ~/.jcode/hooks.json, so treating this as a documented incompleteness (tracked in design §11) rather than a blocking security gap makes sense. No action needed for this PR.

Comment thread site/docs/overview/hooks.md
…ening)

acp: guard optional recorder before sess.rec.UUID() (panic). interactive: rebuild dispatcher + fire SessionStart after resume restores the UUID, gate the setup SessionStart to fresh sessions (avoid double-fire and wrong session_id). dispatcher: skip unsupported hook types. exec: cap stdout/stderr at 1 MiB and run hooks in their own process group (unix) so a timeout kills the whole tree, not just sh. types: drop unwired PreCompact/PostCompact/Trigger so code and docs agree. Plus doc path fix, error prefix, and const rename nits.

prompt-level hooks (UserPromptSubmit/SessionStart) remain TUI-only in v1; ACP/Web parity is a documented follow-up.

Generated with Jack AI bot
@cnjack cnjack merged commit a83871f into main Jul 5, 2026
3 checks passed
@cnjack cnjack deleted the feat/hooks branch July 5, 2026 02:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant