fix(clv2): resolve cwd to git root before project detection#831
Conversation
📝 WalkthroughWalkthroughThe Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
Greptile SummaryThis PR fixes a bug in Confidence Score: 5/5Safe to merge — the fix is minimal, correct, and all edge cases are handled. The shell change is a tightly-scoped 3-line addition that correctly resolves the git root before setting CLAUDE_PROJECT_DIR, with graceful fallback for non-git directories and a missing git binary. End-to-end tests actually invoke observe.sh and verify both the success and fallback paths. No P0/P1 issues were found. No files require special attention. Important Files Changed
Reviews (2): Last reviewed commit: "fix(clv2): resolve cwd to git root befor..." | Re-trigger Greptile |
| // ────────────────────────────────────────────────────── | ||
| // Group 3: E2E — observe.sh cwd extraction with subdirectory | ||
| // ────────────────────────────────────────────────────── | ||
|
|
||
| console.log('\n--- E2E: observe.sh cwd resolution ---'); |
There was a problem hiding this comment.
The Group 3 tests are labelled E2E: observe.sh cwd resolution, but they never actually invoke observe.sh. Instead they reproduce the relevant shell snippet inline (lines 175-182 and 205-212). A true E2E test would pipe a hook JSON payload into observe.sh and verify the resulting CLAUDE_PROJECT_DIR or project entry in projects.json.
Because the logic is duplicated rather than exercised through the real script, these tests would not catch regressions in observe.sh itself (e.g. if the variable name _GIT_ROOT were accidentally changed in the script but not in the tests). Consider renaming the group to --- Unit: git root resolution snippet --- or, ideally, invoking observe.sh directly via a minimal stub environment.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
| test('git rev-parse fails gracefully outside a git repo', () => { | ||
| const testDir = createTempDir(); | ||
|
|
||
| try { | ||
| const result = runBash( | ||
| `git -C "${toBashPath(testDir)}" rev-parse --show-toplevel 2>/dev/null || echo ""` | ||
| ); | ||
| assert.strictEqual(result, '', 'Should return empty string outside a git repo'); | ||
| } finally { | ||
| cleanupDir(testDir); | ||
| } | ||
| }); |
There was a problem hiding this comment.
TMPDIR is inside a git repository
createTempDir() uses os.tmpdir() to create the temp directory. On some CI environments (Docker-in-Docker, devcontainer, etc.) TMPDIR or /tmp can itself reside within a git worktree, in which case git rev-parse --show-toplevel would succeed and return a non-empty string — causing this "should return empty" assertion to fail.
A more robust approach is to ensure the temp path is not inside a git repo by using GIT_CEILING_DIRECTORIES to cap traversal:
const result = runBash(
`GIT_CEILING_DIRECTORIES="${toBashPath(testDir)}" git -C "${toBashPath(testDir)}" rev-parse --show-toplevel 2>/dev/null || echo ""`
);Alternatively, use --git-dir pointing to a definitely-absent path to force failure.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/hooks/observe-subdirectory-detection.test.js (2)
42-47: Avoid silently swallowing cleanup errors.Line 45-Line 47 suppresses all cleanup failures, which can mask temp-dir leakage in CI. Please handle/report the error explicitly while keeping cleanup best-effort.
As per coding guidelines, "Always handle errors explicitly at every level and never silently swallow errors".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/observe-subdirectory-detection.test.js` around lines 42 - 47, The cleanupDir function is silently swallowing errors which can hide temp-dir leaks; change the catch to explicitly report the failure while preserving best-effort cleanup: in cleanupDir(dir) catch the thrown error from fs.rmSync and log/report it (e.g., console.error or the test logger) including the dir path and error.stack/message, but do not rethrow so cleanup remains best-effort; ensure you reference the function name cleanupDir and include the failing dir for clarity in the message.
174-182: “E2E” cases currently duplicate implementation logic instead of executingobserve.sh.These tests can drift from the real script behavior. Consider invoking
skills/continuous-learning-v2/hooks/observe.shwith controlled stdin/env so assertions validate the actual hook path end-to-end.Refactor sketch
- // Simulate the cwd extraction + git root resolution logic from observe.sh - const script = [ - `STDIN_CWD="${toBashPath(subDir)}"`, - 'if [ -n "$STDIN_CWD" ] && [ -d "$STDIN_CWD" ]; then', - ' _GIT_ROOT=$(git -C "$STDIN_CWD" rev-parse --show-toplevel 2>/dev/null || true)', - ' CLAUDE_PROJECT_DIR="${_GIT_ROOT:-$STDIN_CWD}"', - 'fi', - 'echo "$CLAUDE_PROJECT_DIR"', - ].join('\n'); + // Execute actual observe.sh with hook JSON and inspect resulting project-scoped output + const input = JSON.stringify({ cwd: subDir, tool_name: 'Read', tool_input: {} }); + execFileSync('bash', [observeShPath, 'post'], { + input, + cwd: repoRoot, + env: { ...process.env, CLV2_CONFIG: '/nonexistent-config.json' }, + stdio: 'pipe', + }); + // Assert via created project artifacts / detection output rather than duplicated inline shell logicAlso applies to: 205-212
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/hooks/observe-subdirectory-detection.test.js` around lines 174 - 182, The test currently constructs a local "script" string that duplicates observe.sh logic (setting STDIN_CWD, resolving _GIT_ROOT, and echoing CLAUDE_PROJECT_DIR); instead, update the test to execute the real hook (observe.sh) with controlled stdin/env and capture its stdout for assertions: spawn or exec the shell script (skills/continuous-learning-v2/hooks/observe.sh) passing STDIN_CWD via echo/pipe or environment, ensure git behavior is deterministic by using a temporary repo or mocking git -C, then assert on the script's output (CLAUDE_PROJECT_DIR) instead of the local "script" variable to keep tests end-to-end and avoid drift from the real implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/hooks/observe-subdirectory-detection.test.js`:
- Around line 42-47: The cleanupDir function is silently swallowing errors which
can hide temp-dir leaks; change the catch to explicitly report the failure while
preserving best-effort cleanup: in cleanupDir(dir) catch the thrown error from
fs.rmSync and log/report it (e.g., console.error or the test logger) including
the dir path and error.stack/message, but do not rethrow so cleanup remains
best-effort; ensure you reference the function name cleanupDir and include the
failing dir for clarity in the message.
- Around line 174-182: The test currently constructs a local "script" string
that duplicates observe.sh logic (setting STDIN_CWD, resolving _GIT_ROOT, and
echoing CLAUDE_PROJECT_DIR); instead, update the test to execute the real hook
(observe.sh) with controlled stdin/env and capture its stdout for assertions:
spawn or exec the shell script (skills/continuous-learning-v2/hooks/observe.sh)
passing STDIN_CWD via echo/pipe or environment, ensure git behavior is
deterministic by using a temporary repo or mocking git -C, then assert on the
script's output (CLAUDE_PROJECT_DIR) instead of the local "script" variable to
keep tests end-to-end and avoid drift from the real implementation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6ad39361-1c96-4eb9-9c84-21ed27bc9126
📒 Files selected for processing (2)
skills/continuous-learning-v2/hooks/observe.shtests/hooks/observe-subdirectory-detection.test.js
There was a problem hiding this comment.
2 issues found across 2 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="tests/hooks/observe-subdirectory-detection.test.js">
<violation number="1" location="tests/hooks/observe-subdirectory-detection.test.js:149">
P2: This assertion is environment-dependent because the command can traverse to a parent git worktree when the temp directory is inside one, which makes the test flaky on some CI setups. Bound git discovery (for example with `GIT_CEILING_DIRECTORIES`) before asserting an empty result.</violation>
<violation number="2" location="tests/hooks/observe-subdirectory-detection.test.js:174">
P2: The E2E tests simulate `observe.sh` logic with inline shell snippets instead of executing the real script, leaving integration regressions in `observe.sh` undetected.</violation>
</file>
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
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const subDir = path.join(repoDir, 'src', 'components'); | ||
| fs.mkdirSync(subDir, { recursive: true }); | ||
|
|
||
| // Simulate the cwd extraction + git root resolution logic from observe.sh |
There was a problem hiding this comment.
P2: The E2E tests simulate observe.sh logic with inline shell snippets instead of executing the real script, leaving integration regressions in observe.sh undetected.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At tests/hooks/observe-subdirectory-detection.test.js, line 174:
<comment>The E2E tests simulate `observe.sh` logic with inline shell snippets instead of executing the real script, leaving integration regressions in `observe.sh` undetected.</comment>
<file context>
@@ -0,0 +1,237 @@
+ const subDir = path.join(repoDir, 'src', 'components');
+ fs.mkdirSync(subDir, { recursive: true });
+
+ // Simulate the cwd extraction + git root resolution logic from observe.sh
+ const script = [
+ `STDIN_CWD="${toBashPath(subDir)}"`,
</file context>
|
thanks. queued for review. |
6931f04 to
3f6a14a
Compare
…-project-detection fix(clv2): resolve cwd to git root before project detection
…-project-detection fix(clv2): resolve cwd to git root before project detection
Summary
observe.shcaptures tool events, it extractscwdfrom the hook JSON and setsCLAUDE_PROJECT_DIR. Previously it used the raw cwd, so subdirectories (e.g.my-repo/docs/) created bogus project entries inprojects.jsoninstead of being attributed to the parent repo.git rev-parse --show-toplevelbefore settingCLAUDE_PROJECT_DIR. Falls back to raw cwd for non-git directories.Root Cause
In
observe.shlines 72-74,CLAUDE_PROJECT_DIRwas set to the rawcwdfrom hook JSON. Sincedetect-project.shusesCLAUDE_PROJECT_DIRwith highest priority (before git detection), subdirectories became separate "projects."Changes
skills/continuous-learning-v2/hooks/observe.sh— 3-line change: resolve cwd to git root, fall back to raw cwdtests/hooks/observe-subdirectory-detection.test.js— 6 new tests (content checks, behavior, E2E)Test plan
ajvdep unchanged)Summary by cubic
Resolve
cwdto the git repo root in CLv2 observation so events from subfolders map to the correct project. Prevents bogus entries inprojects.json, falling back to rawcwdfor non-git dirs.skills/continuous-learning-v2/hooks/observe.shto setCLAUDE_PROJECT_DIRtogit -C "$STDIN_CWD" rev-parse --show-toplevel(fallback to"$STDIN_CWD").Written for commit 3f6a14a. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests