fix: support python fallback in continuous-learning observe hook#350
fix: support python fallback in continuous-learning observe hook#350tsubasakong wants to merge 5 commits intoaffaan-m:mainfrom
Conversation
📝 WalkthroughWalkthroughAdds a SCRIPT_DIR-based DEFAULT_PLUGIN_ROOT and a fallback so CLAUDE_PLUGIN_ROOT defaults to it; adds PYTHON_CMD detection in observe.sh that prefers Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~12 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
No issues found across 2 files
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
![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
🤖 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 15-23: The early-exit branch that runs when no Python is found
should first drain stdin to avoid leaving the upstream printf writer hanging; in
the no-Python else branch (the block that echoes "[observe] No python found,
skipping observation" and exits), consume/redirect stdin (e.g. by piping/reading
to /dev/null or using cat >/dev/null) before calling exit so the caller's writer
doesn't receive SIGPIPE and fail the pipeline.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: f93907fe-e5e4-409a-8446-bb7ec5d98697
📒 Files selected for processing (2)
scripts/hooks/run-with-flags-shell.shskills/continuous-learning-v2/hooks/observe.sh
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: this PR has merge conflicts. Please rebase or resolve.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
affaan-m
left a comment
affaan-m
left a comment
There was a problem hiding this comment.
Automated review: checks are failing. Please fix failures before review.
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/hooks/run-with-flags-shell.sh`:
- Around line 10-12: Remove the duplicated SCRIPT_DIR computation and ensure
CLAUDE_PLUGIN_ROOT is exported so child processes can read it; specifically,
keep a single declaration of SCRIPT_DIR and DEFAULT_PLUGIN_ROOT, set
CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-${DEFAULT_PLUGIN_ROOT}}" and add
export CLAUDE_PLUGIN_ROOT (so run-with-flags.js and its getPluginRoot() can
access it from the environment).
In `@skills/continuous-learning-v2/hooks/observe.sh`:
- Around line 15-24: Remove the early hardcoded Python detection block that
exits when no python binary is found and instead invoke the existing
resolve_python_cmd() (which honors CLV2_PYTHON_CMD) at the script entrypoint
before reading stdin; alternatively source detect-project.sh first so
CLV2_PYTHON_CMD is available and then call resolve_python_cmd() to set
PYTHON_CMD (or fail gracefully) — update the top of observe.sh to rely on
resolve_python_cmd() and remove the initial command -v python* check that causes
an early exit.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: c29bc3bc-f977-450c-8c72-f5a76777c58e
📒 Files selected for processing (2)
scripts/hooks/run-with-flags-shell.shskills/continuous-learning-v2/hooks/observe.sh
| SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" | ||
| DEFAULT_PLUGIN_ROOT="$(cd "${SCRIPT_DIR}/../.." && pwd)" | ||
| CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-${DEFAULT_PLUGIN_ROOT}}" |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
cat -n scripts/hooks/run-with-flags-shell.shRepository: affaan-m/everything-claude-code
Length of output: 1379
🏁 Script executed:
# Find hook scripts and check if any reference CLAUDE_PLUGIN_ROOT
find scripts/hooks -type f -name "*.sh" -o -name "*.js" | head -20Repository: affaan-m/everything-claude-code
Length of output: 793
🏁 Script executed:
# Search for references to CLAUDE_PLUGIN_ROOT in the codebase
rg 'CLAUDE_PLUGIN_ROOT' --type sh --type jsRepository: affaan-m/everything-claude-code
Length of output: 1571
🏁 Script executed:
# Check if run-with-flags.js reads CLAUDE_PLUGIN_ROOT from process.env
cat -n scripts/hooks/run-with-flags.js | head -40Repository: affaan-m/everything-claude-code
Length of output: 1389
🏁 Script executed:
# Search for how hooks are configured/registered to see what gets passed as REL_SCRIPT_PATH
rg "REL_SCRIPT_PATH|run-with-flags" scripts/ -A 2 -B 2 | head -50Repository: affaan-m/everything-claude-code
Length of output: 1581
Export CLAUDE_PLUGIN_ROOT so child hooks can access it.
Lines 7–12 contain duplicate SCRIPT_DIR computation and set CLAUDE_PLUGIN_ROOT without exporting it. Child hook scripts—particularly Node scripts like run-with-flags.js—that call getPluginRoot() expect CLAUDE_PLUGIN_ROOT to be available in their process environment. Without the export, they cannot access the fallback value set here.
Suggested fix
SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
-PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-$(cd "${SCRIPT_DIR}/../.." && pwd)}"
-
-SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)"
DEFAULT_PLUGIN_ROOT="$(cd "${SCRIPT_DIR}/../.." && pwd)"
-CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-${DEFAULT_PLUGIN_ROOT}}"
+export CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-${DEFAULT_PLUGIN_ROOT}}"
+PLUGIN_ROOT="$CLAUDE_PLUGIN_ROOT"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/hooks/run-with-flags-shell.sh` around lines 10 - 12, Remove the
duplicated SCRIPT_DIR computation and ensure CLAUDE_PLUGIN_ROOT is exported so
child processes can read it; specifically, keep a single declaration of
SCRIPT_DIR and DEFAULT_PLUGIN_ROOT, set
CLAUDE_PLUGIN_ROOT="${CLAUDE_PLUGIN_ROOT:-${DEFAULT_PLUGIN_ROOT}}" and add
export CLAUDE_PLUGIN_ROOT (so run-with-flags.js and its getPluginRoot() can
access it from the environment).
| # Pick an available Python executable (python3 preferred, python fallback) | ||
| if command -v python3 >/dev/null 2>&1; then | ||
| PYTHON_CMD="python3" | ||
| elif command -v python >/dev/null 2>&1; then | ||
| PYTHON_CMD="python" | ||
| else | ||
| cat >/dev/null | ||
| echo "[observe] No python found, skipping observation" >&2 | ||
| exit 0 | ||
| fi |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, check if the file exists and explore the repository structure
find . -type f -name "observe.sh" 2>/dev/null | head -20Repository: affaan-m/everything-claude-code
Length of output: 123
🏁 Script executed:
# List the continuous-learning-v2 directory structure
fd . "skills/continuous-learning-v2" -type f | head -30Repository: affaan-m/everything-claude-code
Length of output: 245
🏁 Script executed:
# Read the observe.sh file
cat -n ./skills/continuous-learning-v2/hooks/observe.shRepository: affaan-m/everything-claude-code
Length of output: 10034
🏁 Script executed:
# Read the detect-project.sh file to understand CLV2_PYTHON_CMD contract
cat -n ./skills/continuous-learning-v2/scripts/detect-project.sh | head -100Repository: affaan-m/everything-claude-code
Length of output: 3979
🏁 Script executed:
# Read start-observer.sh
cat -n ./skills/continuous-learning-v2/agents/start-observer.sh | head -100Repository: affaan-m/everything-claude-code
Length of output: 4040
Honor CLV2_PYTHON_CMD before exiting.
The early Python detection on Lines 15–24 skips PATH check for CLV2_PYTHON_CMD and exits at Line 23 before detect-project.sh is sourced at Line 96. This breaks any environment where only CLV2_PYTHON_CMD provides a valid interpreter. The resolve_python_cmd() function on Lines 41–58 correctly checks CLV2_PYTHON_CMD first, but it is never reached if Lines 15–24 exit early.
Remove the early hardcoded check and instead call resolve_python_cmd() before reading stdin, or source detect-project.sh and check CLV2_PYTHON_CMD first at the entry point.
🤖 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 15 - 24, Remove
the early hardcoded Python detection block that exits when no python binary is
found and instead invoke the existing resolve_python_cmd() (which honors
CLV2_PYTHON_CMD) at the script entrypoint before reading stdin; alternatively
source detect-project.sh first so CLV2_PYTHON_CMD is available and then call
resolve_python_cmd() to set PYTHON_CMD (or fail gracefully) — update the top of
observe.sh to rely on resolve_python_cmd() and remove the initial command -v
python* check that causes an early exit.
|
Closing as superseded. The Windows/python fallback and related observe-hook fixes already landed via the merged hook reliability work in #371 and follow-up fixes this week, so this branch no longer needs to merge separately. |
2 participants
Description
skills/continuous-learning-v2/hooks/observe.sh(python3preferred,pythonfallback)python3invocations with the resolved command to support Windows Git Bash environmentsCLAUDE_PLUGIN_ROOTfallback inscripts/hooks/run-with-flags-shell.shbased on script location when the env var is unsetValidation:
bash -n skills/continuous-learning-v2/hooks/observe.shbash -n scripts/hooks/run-with-flags-shell.shCloses #349
Type of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesChecklist
node tests/run-all.js)Summary by CodeRabbit
Bug Fixes
Chores