Add generic per-session admissionCommand policy hook#70
Conversation
The four capture hooks (session-start, user-prompt, post-tool-use, stop) each independently derive a session name from the hook's session_id and write to Honcho -- with no way for the host application to say "don't capture this one." Any process that fires these hooks (a subagent, a headless worker, a CI run, a test harness) accumulates durable memory state exactly as if it were the primary interactive session. This is a real problem for any multi-agent or multi-process Claude Code setup: worker/subagent sessions pollute the same peer/session space as the agent whose memory is actually meant to be durable, with no built-in way to tell them apart. Adds a new optional config key, "admissionCommand" (settable at the root config level or per-host, same pattern as "enabled"/"logging"/"sessionStrategy"). Unset (default): zero behavior change. Every session is admitted, exactly as today. Existing installs are unaffected. Set: before any of the four hooks creates a session/peer or writes a message, it calls a new isAdmitted(hookInput) helper in config.ts. That helper spawns the configured command, writes the hook's raw HookInput JSON to its stdin, and waits (5s timeout) for it to exit. Exit 0 admits; any non-zero exit, or a failure to spawn the command at all, rejects (fail-closed). A misconfigured or crashing admission policy should never silently degrade to "capture everything." isAdmitted() is called at the top of all four hooks -- session-start.ts, user-prompt.ts, post-tool-use.ts, and stop.ts -- immediately after hookInput is parsed and before any Honcho API call. A gate in only one hook leaks: each of the other three independently re-derives the session and writes to it regardless of what session-start decided. This is a generic extension point with zero knowledge of any specific downstream policy -- the command is just a path, invoked the same way regardless of what it checks (session identity, a peer allowlist, a rate limit, anything). Verified against a companion admission script that reads HookInput JSON on stdin, checks session_id against a recorded allowlist, and exits 0/1 accordingly (application-specific policy, not part of this patch). With admissionCommand unset, all four hooks behave identically to main (no regression). With it set, a non-admitted session_id causes all four hooks to exit 0 without any Honcho API call -- confirmed against a real 12-session sweep with zero log/API activity for every rejected session, and normal capture preserved for the admitted session. One note on an existing type gap this patch does NOT fix: post-tool-use.ts's local HookInput interface does not declare session_id, even though Claude Code's real PostToolUse event payload carries it. JSON.parse doesn't strip undeclared fields, so isAdmitted() still sees session_id at runtime -- this patch works correctly as written. Worth a separate, unrelated cleanup PR; not touched here to keep this patch minimal and single-purpose.
|
Warning Review limit reached
Next review available in: 42 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the 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 configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
WalkthroughAdds configurable admission commands to Honcho configuration and applies fail-closed admission checks before session initialization, prompt handling, tool-event capture, and stop-event message uploads. ChangesAdmission-gated capture
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
plugins/honcho/src/config.ts (1)
668-706: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider logging when admission is rejected.
isAdmittedis fail-closed and correct, but rejection is silent — the hooksprocess.exit(0)with no trace. A misconfigured or crashing admission command would cause Honcho capture to quietly stop with no diagnostic output, making it hard to distinguish "policy rejected" from "nothing happened."Adding a
logHookcall in each hook's rejection branch (or returning a reason string fromisAdmitted) would give users visibility when their admission policy is rejecting sessions.💡 Optional: log on rejection in each hook
// In each hook, e.g. session-start.ts: if (!isAdmitted(hookInput)) { + logHook("session-start", "Session rejected by admission policy"); process.exit(0); }🤖 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 `@plugins/honcho/src/config.ts` around lines 668 - 706, Update the admission rejection handling around isAdmitted and each capture hook’s rejection branch to emit a diagnostic through the existing logHook mechanism before process.exit(0). Include enough context to distinguish an admission-policy rejection from normal no-op behavior, while preserving fail-closed behavior and the current successful admission flow.
🤖 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 `@plugins/honcho/src/config.ts`:
- Around line 668-706: Update the admission rejection handling around isAdmitted
and each capture hook’s rejection branch to emit a diagnostic through the
existing logHook mechanism before process.exit(0). Include enough context to
distinguish an admission-policy rejection from normal no-op behavior, while
preserving fail-closed behavior and the current successful admission flow.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: f030b458-6eaf-45f6-9761-b3e61684468a
📒 Files selected for processing (5)
plugins/honcho/src/config.tsplugins/honcho/src/hooks/post-tool-use.tsplugins/honcho/src/hooks/session-start.tsplugins/honcho/src/hooks/stop.tsplugins/honcho/src/hooks/user-prompt.ts
The admissionCommand policy hook was wired into session-start, user-prompt, post-tool-use, and stop, but session-end was missed -- it also writes to Honcho (session.addMessages) and had no admission check. Fixes the gap with the same isAdmitted(hookInput) call, in the same position (right after hookInput is parsed, before any Honcho API call) and the same shape as the other four hooks.
pre-compact.ts calls honcho.session()/honcho.peer() (get-or-create per the SDK) and peer.chat() unconditionally, before any isAdmitted() check. A rejected session could still spawn session/peer records server-side on every auto-compaction — the same accrual class this PR's admissionCommand hook exists to stop. Adds the isAdmitted(hookInput) check in the same position and comment style as the other five gated hooks (session-start, post-tool-use, user-prompt, stop, session-end): immediately after hookInput is parsed, before any Honcho API contact. Also updates the isAdmitted() doc comment in config.ts to name all six gated hooks (was stale at "four") and the full set of Honcho API call shapes a rejected session must never reach. Co-Authored-By: Claude Sonnet <noreply@anthropic.com>
1 participant
Why
The four capture hooks (session-start, user-prompt, post-tool-use, stop)
each independently derive a session name from the hook's session_id and
write to Honcho -- with no way for the host application to say "don't
capture this one." Any process that fires these hooks (a subagent, a
headless worker, a CI run, a test harness) accumulates durable memory
state exactly as if it were the primary interactive session.
This is a real problem for any multi-agent or multi-process Claude Code
setup: worker/subagent sessions pollute the same peer/session space as
the agent whose memory is actually meant to be durable, with no built-in
way to tell them apart.
What this adds
A new optional config key,
admissionCommand(settable at the rootconfig level or per-host, same pattern as
enabled/logging/sessionStrategy).exactly as today. Existing installs are unaffected.
message, it calls a new
isAdmitted(hookInput)helper inconfig.ts.That helper spawns the configured command, writes the hook's raw
HookInput JSON to its stdin, and waits (5s timeout) for it to exit.
Exit 0 admits; any non-zero exit, or a failure to spawn the command at
all, rejects (fail-closed). A misconfigured or crashing admission
policy should never silently degrade to "capture everything."
isAdmitted()is called at the top of all four hooks -- session-start.ts,user-prompt.ts, post-tool-use.ts, and stop.ts -- immediately after
hookInput is parsed and before any Honcho API call. A gate in only one
hook leaks: each of the other three independently re-derives the session
and writes to it regardless of what session-start decided.
This is a generic extension point with zero knowledge of any specific
downstream policy -- the command is just a path, invoked the same way
regardless of what it checks (session identity, a peer allowlist, a rate
limit, anything). It's the same shape as
admissionCommand-style hooks inother tools (e.g. gitea's webhook allowlist pattern, kubectl admission
webhooks): the tool defines where the check runs and what a pass/fail
looks like; it never carries the policy itself.
One note on an existing type gap this patch does NOT fix
post-tool-use.ts's localHookInputinterface does not declaresession_id, even though Claude Code's real PostToolUse event payloadcarries it.
JSON.parsedoesn't strip undeclared fields, soisAdmitted()still seessession_idat runtime -- this patch workscorrectly as written. But the type is misleading on its own and could bite
someone reading only the interface. Worth a separate, unrelated cleanup PR
to add
session_id?: string;to that interface for accuracy; not touchedhere to keep this patch minimal and single-purpose.
Testing
Verified against a companion admission script (not part of this patch --
that's application-specific policy, lives in the consuming project) that
reads HookInput JSON on stdin, checks session_id against a recorded
allowlist, and exits 0/1 accordingly. Confirmed: with
admissionCommandunset, all four hooks behave identically to main (no regression). With it
set, a non-admitted session_id causes all four hooks to exit 0 without any
Honcho API call -- verified against a real 12-real-session sweep run
through the actual hook path (
bun run hooks/session-start.ts), with theHoncho client's own activity log showing zero log lines / zero API
calls for every rejected session_id, and normal session-creation +
addMessagescalls preserved for the one admitted (matching) session_id.Summary by CodeRabbit