fix: harden observe loop prevention#427
Conversation
📝 WalkthroughWalkthroughThe observe hook script now implements lazy project-aware initialization, adds sentinel-based retry prevention, introduces automated purge routines for 30-day-old observations, and enhances input parsing with HOOK_PHASE propagation via Python. Multiple environment and path-based guard layers skip non-human or undesired sessions early. Test coverage expands with new helper functions and test cases validating skip conditions. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
📝 Coding Plan
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 |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
skills/continuous-learning-v2/hooks/observe.sh (1)
103-123: Move Layers 1-3 ahead of Python/bootstrap too.They are now before project detection, but
CLAUDE_CODE_ENTRYPOINT,ECC_HOOK_PROFILE, andECC_SKIP_OBSERVEstill run after interpreter probing andcwdparsing. Hoisting those three checks aboveresolve_python_cmd/STDIN_CWDextraction would make the skipped-session path fully cheap.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 103 - 123, The CLAUDE_CODE_ENTRYPOINT, ECC_HOOK_PROFILE, and ECC_SKIP_OBSERVE checks should run before any interpreter probing or cwd parsing so skipped sessions take the cheap path; move the Layer 1–3 case/condition blocks that check CLAUDE_CODE_ENTRYPOINT, ECC_HOOK_PROFILE, and ECC_SKIP_OBSERVE to occur before resolve_python_cmd and before any STDIN_CWD extraction/processing, ensuring no calls to resolve_python_cmd or any cwd parsing happen when those env-vars indicate an automated or minimal run.tests/hooks/hooks.test.js (1)
2450-2452: Also assert thatprojects.jsonstays untouched in the early-exit cases.These assertions only prove that
~/.claude/homunculus/projects/was not created.detect-project.shalso persists observer state in~/.claude/homunculus/projects.json, so a regression that only updates the registry would still satisfy the current tests.🧪 Suggested assertion pattern
+ const homunculusDir = path.join(homeDir, '.claude', 'homunculus'); assert.ok(!fs.existsSync(path.join(homeDir, '.claude', 'homunculus', 'projects')), 'non-cli entrypoints should exit before project detection runs'); + assert.ok(!fs.existsSync(path.join(homunculusDir, 'projects.json')), 'non-cli entrypoints should not touch the project registry');Also applies to: 2471-2473, 2497-2499, 2519-2521, 2542-2544
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/hooks.test.js` around lines 2450 - 2452, The test currently asserts that the projects directory wasn't created but misses verifying that the projects registry file remains unchanged; before invoking the subject, read and store the contents (or existence) of path.join(homeDir, '.claude', 'homunculus', 'projects.json') and after the run assert the file either still does not exist or its contents are strictly equal to the stored value; update the assertions near the references in tests/hooks/hooks.test.js (the blocks around lines with assert.strictEqual(...), assert.ok(...), assert.deepStrictEqual(...)) and apply the same check to the other early-exit cases mentioned (the blocks at the other line ranges) so regressions that modify projects.json are caught.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 156-161: SENTINEL_FILE is using PROJECT_ROOT which can be set to
the hook cwd; change it to use the resolved project directory to avoid
per-subdir locks: update the SENTINEL_FILE assignment in observe.sh (variable
name SENTINEL_FILE, fallback variable CLV2_OBSERVER_SENTINEL_FILE) to use
PROJECT_DIR as the canonical project root (from scripts/detect-project.sh)
instead of PROJECT_ROOT so the sentinel is written to
${PROJECT_DIR}/.observer.lock.
---
Nitpick comments:
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 103-123: The CLAUDE_CODE_ENTRYPOINT, ECC_HOOK_PROFILE, and
ECC_SKIP_OBSERVE checks should run before any interpreter probing or cwd parsing
so skipped sessions take the cheap path; move the Layer 1–3 case/condition
blocks that check CLAUDE_CODE_ENTRYPOINT, ECC_HOOK_PROFILE, and ECC_SKIP_OBSERVE
to occur before resolve_python_cmd and before any STDIN_CWD
extraction/processing, ensuring no calls to resolve_python_cmd or any cwd
parsing happen when those env-vars indicate an automated or minimal run.
In `@tests/hooks/hooks.test.js`:
- Around line 2450-2452: The test currently asserts that the projects directory
wasn't created but misses verifying that the projects registry file remains
unchanged; before invoking the subject, read and store the contents (or
existence) of path.join(homeDir, '.claude', 'homunculus', 'projects.json') and
after the run assert the file either still does not exist or its contents are
strictly equal to the stored value; update the assertions near the references in
tests/hooks/hooks.test.js (the blocks around lines with assert.strictEqual(...),
assert.ok(...), assert.deepStrictEqual(...)) and apply the same check to the
other early-exit cases mentioned (the blocks at the other line ranges) so
regressions that modify projects.json are caught.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: ba1aff1e-ff15-47d3-ace4-bb44520d1a37
📒 Files selected for processing (2)
skills/continuous-learning-v2/hooks/observe.shtests/hooks/hooks.test.js
| source "${SKILL_ROOT}/scripts/detect-project.sh" | ||
| PYTHON_CMD="${CLV2_PYTHON_CMD:-$PYTHON_CMD}" | ||
|
|
||
| OBSERVATIONS_FILE="${PROJECT_DIR}/observations.jsonl" | ||
|
|
||
| SENTINEL_FILE="${CLV2_OBSERVER_SENTINEL_FILE:-${PROJECT_ROOT:-$PROJECT_DIR}/.observer.lock}" |
There was a problem hiding this comment.
SENTINEL_FILE is still scoped to the current cwd, not the resolved project.
Because Line 82 exports CLAUDE_PROJECT_DIR="$STDIN_CWD", skills/continuous-learning-v2/scripts/detect-project.sh prefers the hook cwd when populating PROJECT_ROOT. With Line 161 using PROJECT_ROOT here, a prompt under one subdirectory writes .observer.lock there only; a later hook from a sibling directory in the same repo will miss that lock and re-enter the observe loop.
💡 Suggested fix
-SENTINEL_FILE="${CLV2_OBSERVER_SENTINEL_FILE:-${PROJECT_ROOT:-$PROJECT_DIR}/.observer.lock}"
+# Keep the circuit breaker keyed to the resolved project, not the current hook cwd.
+SENTINEL_FILE="${CLV2_OBSERVER_SENTINEL_FILE:-${PROJECT_DIR}/.observer.lock}"📝 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.
| source "${SKILL_ROOT}/scripts/detect-project.sh" | |
| PYTHON_CMD="${CLV2_PYTHON_CMD:-$PYTHON_CMD}" | |
| OBSERVATIONS_FILE="${PROJECT_DIR}/observations.jsonl" | |
| SENTINEL_FILE="${CLV2_OBSERVER_SENTINEL_FILE:-${PROJECT_ROOT:-$PROJECT_DIR}/.observer.lock}" | |
| source "${SKILL_ROOT}/scripts/detect-project.sh" | |
| PYTHON_CMD="${CLV2_PYTHON_CMD:-$PYTHON_CMD}" | |
| OBSERVATIONS_FILE="${PROJECT_DIR}/observations.jsonl" | |
| # Keep the circuit breaker keyed to the resolved project, not the current hook cwd. | |
| SENTINEL_FILE="${CLV2_OBSERVER_SENTINEL_FILE:-${PROJECT_DIR}/.observer.lock}" |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/continuous-learning-v2/hooks/observe.sh` around lines 156 - 161,
SENTINEL_FILE is using PROJECT_ROOT which can be set to the hook cwd; change it
to use the resolved project directory to avoid per-subdir locks: update the
SENTINEL_FILE assignment in observe.sh (variable name SENTINEL_FILE, fallback
variable CLV2_OBSERVER_SENTINEL_FILE) to use PROJECT_DIR as the canonical
project root (from scripts/detect-project.sh) instead of PROJECT_ROOT so the
sentinel is written to ${PROJECT_DIR}/.observer.lock.
…ness-skills fix: harden observe loop prevention
Summary
observe.shloop-prevention so automated sessions exit before project detection mutates observer stateECC_SKIP_OBSERVE,agent_id, and skip-path handlingWhy this follow-up exists
#415is already merged tomain#414is closed and GitHub refuses reopening it, so this PR carries the refreshed branch follow-up on top of currentmainTest plan
node tests/hooks/hooks.test.jsSummary by cubic
Prevents observe.sh loops by running skip guards before project detection, so automated or subagent sessions exit without changing project state. Keeps the sentinel circuit-breaker behavior and adds regression tests for skip cases and path handling.
ECC_SKIP_OBSERVE=1, presence ofagent_id, and default/custom skip paths (trimmed).observations.jsonl.ECC_SKIP_OBSERVE,agent_id, default skip dirs, trimmed custom skip paths, and ignoring empty entries.Written for commit 5e48187. Summary will update on new commits.
Summary by CodeRabbit