fix(hooks): reconcile and remove legacy standalone work-loop hooks (#397) - #414
Conversation
The obsolete standalone `brigade-work-loop.py` hook survived upgrades because `is_managed_handler` never recognized it, so install/update reconciliation preserved it as a foreign user hook. That stale hook read each untracked file fully into memory and OOMed on large cache blobs. - Recognize legacy standalone registrations (`is_legacy_handler`, anchored to executable position so unrelated hooks are never matched) and drop them during install/update reconciliation, idempotently and without touching genuine foreign hooks. - Surface a stale legacy registration via `legacy_handler_count` in hook status and a doctor WARN naming the settings file and the exact repair command (`brigade work hooks install --target <path>`). - Prefer the session cwd and explicit cd/--target/git -C targets over incidental bare-path arguments when attributing a Bash command to a wired repo, so a command that merely mentions another repo no longer enrolls it. - Document model caches, virtualenvs, and generated databases as snapshot-skipped directories. The managed fingerprint path already hashes via `git hash-object` (bounded) and skips ignored dirs, so peak RSS does not scale with file size; regression tests lock that in alongside the reconciliation, attribution, and doctor behavior. Closes #397 Co-authored-by: Cursor <cursoragent@cursor.com>
|
@coderabbitai review |
|
bugbot run |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 0bcb6d2. Configure here.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
|
Important Review skippedAuto reviews are limited based on label configuration. 🏷️ Required labels (at least one) (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Repository: escoffier-labs/coderabbit/.coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughClaude hook management now detects legacy standalone work-loop handlers, removes them during reconciliation, reports their presence in status and doctor output, and preserves foreign hooks. Runtime target resolution also considers an explicitly supplied working directory, with regression coverage for large-file fingerprinting and incidental repository paths. ChangesLegacy Claude hook handling
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Claude
participant status_payload
participant doctor
participant hooks_install
Claude->>status_payload: inspect Claude hook settings
status_payload-->>doctor: legacy handler count and events
doctor-->>Claude: WARN and repair command
Claude->>hooks_install: install hooks for target
hooks_install-->>Claude: reconciled managed settings
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/brigade/claude_hooks/package.py (1)
71-100: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winExtract shared command-tokenization helper.
The dict/type/command/
shlex.splitpreamble here duplicatesis_managed_handler's preamble (lines 37-48) verbatim. Consider factoring out a shared helper so both predicates stay in sync if tokenization rules change.♻️ Proposed refactor
+def _command_tokens(value: object) -> list[str] | None: + if not isinstance(value, dict): + return None + if value.get("type") != "command": + return None + command = value.get("command") + if not isinstance(command, str): + return None + try: + return shlex.split(command) + except ValueError: + return None + + def is_legacy_handler(value: object) -> bool: ... - if not isinstance(value, dict): - return False - if value.get("type") != "command": - return False - command = value.get("command") - if not isinstance(command, str): - return False - try: - tokens = shlex.split(command) - except ValueError: - return False + tokens = _command_tokens(value) + if tokens is None: + return False if not tokens: return False🤖 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/brigade/claude_hooks/package.py` around lines 71 - 100, Extract the shared dict/type/command validation and shlex tokenization from is_legacy_handler and is_managed_handler into a helper that returns parsed command tokens or an empty/nullable result for invalid input. Update both predicates to use this helper while preserving their existing matching logic and behavior.
🤖 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/brigade/claude_hooks/install_cmd.py`:
- Around line 38-43: Update _without_managed_or_legacy to pass its event
argument to is_managed_handler when identifying managed handlers, matching the
event-scoped behavior used by status_payload. Preserve handlers whose embedded
event does not match the current event, while continuing to remove only managed
handlers scoped to that event and legacy standalone handlers.
---
Nitpick comments:
In `@src/brigade/claude_hooks/package.py`:
- Around line 71-100: Extract the shared dict/type/command validation and shlex
tokenization from is_legacy_handler and is_managed_handler into a helper that
returns parsed command tokens or an empty/nullable result for invalid input.
Update both predicates to use this helper while preserving their existing
matching logic and behavior.
🪄 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: Repository: escoffier-labs/coderabbit/.coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 880f1304-0836-4c87-b41c-5ec046117091
⛔ Files ignored due to path filters (1)
docs/technical-guide.mdis excluded by!docs/**,!**/docs/**
📒 Files selected for processing (7)
src/brigade/claude_hooks/install_cmd.pysrc/brigade/claude_hooks/package.pysrc/brigade/claude_hooks/runtime.pysrc/brigade/doctor.pytests/test_claude_hooks_doctor.pytests/test_claude_hooks_runtime.pytests/test_claude_hooks_settings_merge.py
| def _without_managed_or_legacy(groups: object, event: str) -> list[object]: | ||
| """Drop managed and legacy standalone handlers, preserving genuine foreign hooks. | ||
|
|
||
| Groups that become empty after dropping handlers are removed so install/update | ||
| reconciliation is idempotent and does not leave stale empty arrays behind. | ||
| """ |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
event param unused for managed-handler scoping — mismatches status_payload's scoping.
is_managed_handler(handler) here (no event arg) matches any managed-shaped command regardless of its embedded --event, but status_payload (line 194) calls is_managed_handler(handler, event), which is event-scoped. A managed-shaped handler under a mismatched event key would be reported as "foreign" by status but silently removed here during install/update — a small but real preservation-guarantee mismatch.
🔧 Proposed fix
foreign = [
- handler for handler in handlers if not is_managed_handler(handler) and not is_legacy_handler(handler)
+ handler
+ for handler in handlers
+ if not is_managed_handler(handler, event) and not is_legacy_handler(handler)
]Also applies to: 55-57
🤖 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/brigade/claude_hooks/install_cmd.py` around lines 38 - 43, Update
_without_managed_or_legacy to pass its event argument to is_managed_handler when
identifying managed handlers, matching the event-scoped behavior used by
status_payload. Preserve handlers whose embedded event does not match the
current event, while continuing to remove only managed handlers scoped to that
event and legacy standalone handlers.
Extract `_command_tokens` so is_managed_handler and is_legacy_handler apply the same command-type/`shlex.split` rules and stay in sync. Behavior-preserving. Co-authored-by: Claude <noreply@anthropic.com>
1 participant
What
Fixes #397. An upgraded install kept the obsolete standalone
brigade-work-loop.pyhook registered becauseis_managed_handlernever recognized it, so install/update reconciliation preserved it as a foreign user hook. That stale hook read each untracked file fully into memory (digest.update(path.read_bytes())) and OOMed on a ~10 GB untracked cache blob.Changes
package.py,install_cmd.py):is_legacy_handlerrecognizes standalonebrigade-work-loop.pyregistrations, anchored to executable position (tokens[0] is the script, or a python interpreter followed by the script as first non-flag arg) so commands that merely mention the name (grep -r brigade-work-loop.py ., a trailing comment, a quoted path) are never matched and never deleted. Install/update reconciliation drops legacy handlers idempotently while preserving genuine foreign hooks and dropping emptied groups.install_cmd.py,doctor.py):legacy_handler_count+legacy_eventsin hook status (disjoint fromforeign_handler_count); doctor emits a WARN that preserves the base state signal and appends the settings file + exact repair commandbrigade work hooks install --target <path>.runtime.py):wired_target_from_payloadprefers the session cwd and explicitcd/--target/git -Ctargets over incidental bare-path arguments, and only enrolls cwd when the payload actually supplies one, so a Bash command that mentions another wired repo no longer enrolls it.git hash-object(streamed, bounded) and the directory fallback is stat-only; no in-process whole-file reads. Peak RSS does not scale with file size. A regression test locks this in.technical-guide.md): names model caches, virtualenvs, and generated databases as snapshot-skipped directories.Tests
Regression tests (using
tmp_target/tmp_path, never the real workspace): legacy anchoring incl. false-positive guards, legacy+managed coexistence in one group, multi-event removal, disjoint foreign/legacy counts, uninstall preserving foreign hooks, doctor normal-vs-legacy behavior, no-cwd attribution, and a mocked-large untracked file fingerprinted without reading its bytes.Verification
./scripts/verifygreen (ruff, format, mypy, version_sync, managed_snapshot, pytest coverage 82.42% ≥ 78 floor). Captured receipt:20260721-153145-work-verify-ab0bf2.Note
Medium Risk
Changes Claude hook reconciliation and Bash repo attribution in the work-loop runtime; mistakes could drop wrong hooks or mis-attribute sessions, but behavior is heavily tested and legacy matching is narrowly anchored.
Overview
Fixes stale
brigade-work-loop.pyhooks that survived install/update and could OOM on large untracked files. Hook install/update/uninstall now strips legacy standalone script registrations (executable-position matching only) alongside managed handlers, while keeping foreign hooks and dropping empty groups idempotently.Status and doctor expose
legacy_handler_count/legacy_events(separate from foreign handlers) and WARN withbrigade work hooks install --target <path>when legacy entries remain.Runtime tightens which repo a Bash tool call is attributed to: session
cwdis preferred over incidental paths in the command, andcwdis only considered when the payload actually provides it—so mentioning another wired repo inrgno longer enrolls that repo.Docs note that work-loop snapshot fingerprints skip common gitignored dirs (caches,
.venv,node_modules, etc.). Tests cover legacy removal, doctor messaging, attribution edge cases, and large-file fingerprinting without in-process full reads.Reviewed by Cursor Bugbot for commit 0bcb6d2. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit
brigade doctornow warns when legacy handlers are detected and provides a repair command.