fix(hooks): fail-fast stop/exit-plan hooks when no reviewer is engaged (#103)

[josephschmitt]

Problem

Monocle installs Claude hooks into ~/.claude/settings.json with timeout: 345600 seconds (96h). The Stop hook (monocle hooks on-stop) and the PermissionRequest→ExitPlanMode hook (monocle hooks exit-plan) connect to the engine socket and issue a blocking request that waits for reviewer feedback. When the engine is running but no reviewer is actively driving Monocle, the hook could block for up to 96h, so the agent appears hung between turns.

Root cause

The "no socket at all" case was already fail-fast — client.Connect returns ErrNotRunning when the socket file is missing, and each hook exits 0 on connect failure. The real gap: autospawn detaches the engine so it survives TUI close, kept alive by a grace period + idle timeout. During that window the socket exists and connects, so the server-side blocking wait (FeedbackQueue.WaitForFeedbackCancellable) never returns — it only unblocks on client disconnect or a reviewer submit, never on "no reviewer present".

SubscriberCount is not a valid "reviewer present" signal (the TUI subscribes with Passive:true and isn't counted), so the fix can't key off it.

Fix — server-bounded wait (primary)

• protocol: added an optional MaxWaitMs field to AwaitReviewMsg and PollFeedbackMsg. Zero preserves the historical unbounded behaviour (backward compatible).
• engine: new boundedWaitCancel derives a cancel channel that fires on the original client disconnect OR after MaxWaitMs. On timeout, WaitForFeedbackCancellable returns nil without consuming the queue — exactly the same shape as a client disconnect — so handleAwaitReview returns HasActivity=true with no verdict and handlePollFeedback returns HasFeedback=false. Both are the "no feedback, turn proceeds" results the hooks already emit as allow/normal-stop.
• hooks: on-stop and exit-plan now pass a 10s MaxWaitMs.

Fix — backstop

• Reduced the two installed settings.json hook timeouts from 345600 to 3600 (1h), so even a stale engine binary that ignores MaxWaitMs can't wedge the agent for days. Extracted to a named blockingHookTimeoutSecs constant.

Pause-flow tradeoff (important)

The bound is honoured only when no pause is requested. If the reviewer pressed P (pause_requested), boundedWaitCancel returns the original cancel unchanged and the engine blocks until the reviewer submits, just as before — an attentive reviewer is never cut off mid-review. When no pause is requested, the wait fails fast (~10s), which is the explicitly requested behaviour. This is a deliberate, conservative split rather than a blanket short timeout.

Tests

• internal/core/engine_test.go: bounded handleAwaitReview/handlePollFeedback return promptly with the no-feedback result on a dirty session with no reviewer; a submission within the window is still delivered; and with pause_requested set the short bound is ignored (call keeps blocking until submit).
• internal/core/socketserver_test.go: end-to-end over the real socket — bounded AwaitReview on a dirty session returns promptly with HasActivity=true and no verdict.
• internal/adapters/claude_test.go: timeout assertions updated to the new value via the constant.

devbox run -- make build, make test, and make lint all pass.

Docs: docs/reference/cli.mdx hook-timeout table updated (345600s → 3600s) with a note explaining the fail-fast / pause behaviour.

Closes #103

🤖 Generated with Claude Code

[greptile-apps]

Greptile Summary

Fixes the agent-hung-between-turns problem caused by on-stop and exit-plan hooks blocking indefinitely when Monocle's engine is alive but no reviewer is present. The fix adds a server-side bounded wait (MaxWaitMs) that times out after ~10 s when no reviewer submits, while deliberately preserving the full blocking behaviour when a pause has been explicitly requested (including a TOCTOU re-check at timer fire time).

• Protocol change (messages.go): adds optional MaxWaitMs to AwaitReviewMsg and PollFeedbackMsg; zero means unbounded, so old clients are unaffected.
• Engine (engine_impl.go): new boundedWaitCancel derives a cancel channel that fires after maxWaitMs OR on the original cancel, whichever comes first; the pause flag is re-checked at timer fire time to close the TOCTOU window flagged in a previous review.
• Backstop (claude.go, hooks.go): installed hook timeout reduced from 345600 s (96 h) to 3600 s (1 h) so even a stale engine binary that ignores MaxWaitMs cannot wedge the agent for days.

Confidence Score: 5/5

Safe to merge. The bounded-wait change is backward compatible (zero = unbounded), the pause flow is preserved by design, and the TOCTOU race from the previous review is addressed with a re-check at timer fire time backed by a dedicated test.

The core boundedWaitCancel logic is correct: the timer goroutine, cancel/stop channels, and defer cleanup all compose cleanly with WaitForFeedbackCancellable's own cond-based signalling. Previous review concerns (TOCTOU race, missing defer stop()) are both addressed. Protocol addition uses omitempty so zero-value clients are unaffected. The backstop timeout reduction is a pure improvement.

No files require special attention.

Important Files Changed

Filename	Overview
internal/core/engine_impl.go	Adds boundedWaitCancel with timer goroutine; both handleAwaitReview and handlePollFeedback now use it correctly with defer stop(); TOCTOU re-check at timer fire time is present and tested
internal/core/engine_test.go	Four new tests cover: fail-fast path, feedback-delivered-within-window, pause-set-before-call (initial snapshot), and the TOCTOU race (pause set after the goroutine starts). Coverage is thorough for handleAwaitReview; the handlePollFeedback late-pause path is not repeated separately since boundedWaitCancel is shared.
internal/protocol/messages.go	Adds MaxWaitMs int with omitempty to both PollFeedbackMsg and AwaitReviewMsg; zero value preserves historic unbounded behaviour — backward compatible
internal/adapters/claude.go	Extracts blockingHookTimeoutSecs = 3600 constant and applies it to both blocking hook entries, replacing the hardcoded 345600
cmd/monocle/hooks.go	Adds blockingHookMaxWait = 10s constant; both on-stop and exit-plan hooks now populate MaxWaitMs in their requests
internal/core/socketserver_test.go	End-to-end test over real socket confirms bounded AwaitReview returns promptly with HasActivity=true and no verdict on timeout
internal/adapters/claude_test.go	Timeout assertions updated to use the new blockingHookTimeoutSecs constant; no logic changes
docs/reference/cli.mdx	Timeout table updated from 345600s to 3600s; explanatory paragraph added for the fail-fast / pause behaviour

Sequence Diagram

sequenceDiagram
    participant Agent as Claude Agent
    participant Hook as on-stop / exit-plan hook
    participant Engine as Monocle Engine
    participant Reviewer as Reviewer (TUI)

    Agent->>Hook: turn ends / ExitPlanMode
    Hook->>Engine: "AwaitReviewMsg{Wait:true, MaxWaitMs:10000}"
    alt "Reviewer present & submits within 10 s"
        Reviewer->>Engine: Submit(verdict)
        Engine-->>Hook: "HasActivity=true, Action=verdict"
        Hook-->>Agent: block / allow (verdict)
    else No reviewer — timer fires at 10 s
        Engine-->>Hook: "HasActivity=true, Action="" (no verdict)"
        Hook-->>Agent: allow (proceed normally)
    else Reviewer pressed P (pause_requested)
        Note over Engine: IsPauseRequested() re-checked at timer fire
        Note over Engine: timer goroutine discards bound, keeps blocking
        Reviewer->>Engine: Submit(verdict)
        Engine-->>Hook: "HasActivity=true, Action=verdict"
        Hook-->>Agent: block (reviewer verdict honoured)
    end

Loading

_{Reviews (2): Last reviewed commit: "fix(core): honor late pause request and ..." | Re-trigger Greptile}