Repository navigation
fix(openfactor): skip tool hooks when no orchestration daemon listens - #3068
Conversation
Every harness tool call started the ~90 MB Bun OpenFactor CLI, about 0.7 CPU-seconds each, only for the CLI to find no daemon socket and drop the event. Activation now wraps the non-prompt host hooks in a `test -S <daemon socket>` check and gives the OpenCode plugin the same check, so tool hooks cost one shell builtin outside orchestrated lanes. Prompt hooks stay ungated because the CLI records lane prompt receipts from them without a daemon. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
Warning This organization's free trial has ended, so Indent couldn't start this review. Manage billing to resume reviews on this repository.
Select any unchecked box below to run or rerun that agent. Pending (1)
Full resultsIndent Review Agent
|
|
Warning Review limit reachedYou've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. Next included review available in 59 minutes. View limit details
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 |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
The PR correctly implements the fix to skip tool hooks when no orchestration daemon listens. The changes are:
- config/openfactor/default.nix - Added
pkgs.jqto the package list (line 12) - config/openfactor/install.sh - Added a socket gate that checks for the orchestration daemon socket before booting the ~90MB Bun CLI (lines 26-77). The CLI now only starts when the daemon socket exists, eliminating the ~0.7s CPU overhead per hook call.
- spec/openfactor_spec.sh - Updated the expected plugin content to include
existsSyncimport (lines 132-136)
This optimizes performance by preventing unnecessary CLI launches when no orchestration daemon is listening. The gating ensures that tool hooks only run when the daemon is active, while prompt events remain ungated since they don't require a daemon. The changes are idempotent and maintain backward compatibility.
free ｜ 𝕏
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
There was a problem hiding this comment.
✅ No new issues found.
The openfactor socket-gate fix (commit 497958b) was already reviewed and approved by Pullfrog. Since that review, only a merge of
origin/main(commit 7a8cf5d) has landed on the branch. The merge brings in unrelated t3-connect browser-setup changes and coverage-spec updates — none of which touch the openfactor logic.
Reviewed changes
- OpenFactor socket gate (
config/openfactor/install.sh:51-77,config/openfactor/default.nix:12,spec/openfactor_spec.sh:131-186): Wraps non-promptopenfactor <harness>-hook <Event>commands in atest -S "<daemon socket>" || exit 0guard so tool-call hooks skip the ~90 MB Bun CLI boot when no orchestration daemon is listening. Prompt hooks remain ungated. The OpenCode plugin receives an equivalentexistsSynccheck. Idempotency is preserved by keepingopenfactorin the gated command string. Comprehensive shellspec tests cover gating, prompt preservation, idempotency, and CLI execution only when the daemon socket exists. - Merge of
origin/main(home-manager/services/t3-connect/setup-browser.sh,spec/coverage_spec.sh,spec/t3_browser_setup_spec.sh): Adds a defensive check for the T3 CLIbrowsersubcommand before running browser setup, with corresponding test coverage. No impact on the openfactor changes.
free ｜ 𝕏
Summary
openfactor hooks install, activation reads each host receipt'sconfigPath. In each JSON hook config it wraps every non-promptopenfactor <harness>-hook <Event>command assh -c 'test -S "<daemon socket>" || exit 0; exec openfactor ...'.existsSync(<daemon socket>)check before it spawns the CLI.${OPENFACTOR_ORCHESTRATION_SOCKET:-${OPENFACTOR_ORCHESTRATION_STATE:-.orchestration-state}/daemon.sock}.Why
On matic each PreToolUse/PostToolUse hook started the ~90 MB Bun CLI. Each start cost 0.67-0.75 s of user CPU and 0.37-0.42 s of wall time, and spiked to about 196% CPU per call across parallel tool calls. No orchestration daemon socket exists on the host, so the CLI posts nothing and exits. That was pure overhead on every tool call from every harness.
decided: gate in the activation wrapper rather than drop the hooks - lanes that run the daemon still get every event
decided: leave prompt events ungated - the CLI records lane prompt receipts from them without a daemon
decided: keep
openfactorin the gated command string -hooks installtreats an entry naming openfactor as installed, so re-runs stay idempotent instead of appending a second hookTest plan
shellspec spec/openfactor_spec.sh(11 examples): gates tool hooks, leaves prompt and foreign hooks unchanged, re-run is idempotent, CLI runs only when the socket exists, OpenCode plugin is gatedshellcheckon install.sh and the specnixfmt --check config/openfactor/default.nix🤖 Generated with Claude Code
Summary by cubic
Tool hooks installed by
openfactor hooks installnow check for the orchestration daemon socket before invoking the CLI, so tool calls outside orchestrated lanes no longer boot the ~90 MB Bun CLI just to drop the event—each start cost about 0.7 CPU-seconds per call.openfactor <harness>-hook <Event>command insh -c 'test -S "<daemon socket>" || exit 0; exec openfactor ...'; prompt hooks stay ungated because the CLI records lane prompt receipts without a daemon.existsSyncgate before spawning the CLI.openfactorin its name, so re-runninghooks installstays idempotent instead of appending a second hook.config/openfactor/default.nixaddspkgs.jq, used by activation to read each host receipt'sconfigPath.Written for commit 7a8cf5d. Summary will update on new commits.