fix: scope git worktrees to shared project id#935
Closed
wbugitlab1 wants to merge 2 commits into
Closed
Conversation
Signed-off-by: wbu <wbu@live.de>
|
@wbugitlab1 is attempting to deploy a commit to the rohitg00's projects Team on Vercel. A member of the Team first needs to authorize it. |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (23)
🚧 Files skipped from review as they are similar to previous changes (20)
📝 WalkthroughWalkthroughThe PR replaces basename-based project identification with a stable SHA-256-hashed Git common-directory identifier across all hook scripts and plugin ChangesCanonical Git-based project scoping across hooks, MCP tools, and API
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook/Plugin Script
participant ProjectTS as src/hooks/_project.ts
participant Git as git rev-parse --git-common-dir
participant REST as /agentmemory/observe or /enrich
Hook->>ProjectTS: resolveProject(data.cwd)
ProjectTS->>ProjectTS: resolveCwd(data.cwd) → normalized dir
alt AGENTMEMORY_PROJECT_ID set
ProjectTS-->>Hook: return trimmed PROJECT_ID
else AGENTMEMORY_PROJECT_NAME set
ProjectTS-->>Hook: return trimmed PROJECT_NAME
else
ProjectTS->>Git: execFileSync git rev-parse --git-common-dir
Git-->>ProjectTS: common dir path
ProjectTS->>ProjectTS: realpathSync → SHA-256 hash
ProjectTS-->>Hook: return "git:<hash>"
end
Hook->>REST: POST {project: "git:<hash>", cwd: resolveCwd(data.cwd), ...}
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
test/hook-project.test.ts (1)
143-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
path.basename()for cross-platform fallback assertionLine 143 hardcodes POSIX separators (
"/"), so this assertion can fail on Windows paths. Usebasename(dir)instead.Proposed fix
- expect(resolveProject(dir)).toBe(dir.split("/").pop()); + expect(resolveProject(dir)).toBe(basename(dir));🤖 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 `@test/hook-project.test.ts` at line 143, Replace the hardcoded POSIX path separator in the assertion on line 143 of test/hook-project.test.ts. Instead of using dir.split("/").pop(), use path.basename(dir) to ensure the test for the resolveProject function works correctly across different operating systems including Windows. This provides cross-platform compatibility for the test assertion.
🧹 Nitpick comments (2)
test/worktree-project-scope.test.ts (1)
50-96: ⚡ Quick winAlign test doubles with repository mock pattern
This suite uses ad-hoc
makeMockSdk/makeMockKV; repository test guidelines require thevi.mock("iii-sdk")pattern withsdk.trigger,kv.get,kv.set, andkv.listmocks for consistency with the rest of the suite.As per coding guidelines, “
test/**/*.test.ts: Mock pattern for tests: usevi.mock('iii-sdk')with mock implementations ofsdk.trigger,kv.get,kv.set,kv.list; follow patterns intest/crystallize.test.ts.”🤖 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 `@test/worktree-project-scope.test.ts` around lines 50 - 96, The test suite uses custom ad-hoc mock factories makeMockKV and makeMockSdk instead of the repository's standard mocking pattern. Replace these custom factory functions with vi.mock('iii-sdk') that provides mock implementations for sdk.trigger, kv.get, kv.set, and kv.list methods. Then update the registerMemorySearch function to use the mocked sdk and kv modules directly instead of calling the factory functions. Follow the mocking pattern demonstrated in test/crystallize.test.ts for consistency with repository guidelines.Source: Coding guidelines
src/hooks/_project.ts (1)
63-68: ⚡ Quick winRemove WHAT-style resolution-order comment block.
This block explains implementation steps rather than intent; prefer self-documenting naming and keep only rationale comments when necessary.
As per coding guidelines, "In TypeScript source code, avoid code comments explaining WHAT — use clear naming instead."
🤖 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/hooks/_project.ts` around lines 63 - 68, Remove the entire multi-line comment block that lists the resolution order steps (items 1 through 4) in src/hooks/_project.ts. This comment explains WHAT the code does rather than WHY, and should be replaced with self-documenting code and variable naming. If there is important rationale about why this resolution order is necessary, condense it into a brief single-line comment explaining the intent rather than the implementation steps.Source: Coding guidelines
🤖 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.
Outside diff comments:
In `@test/hook-project.test.ts`:
- Line 143: Replace the hardcoded POSIX path separator in the assertion on line
143 of test/hook-project.test.ts. Instead of using dir.split("/").pop(), use
path.basename(dir) to ensure the test for the resolveProject function works
correctly across different operating systems including Windows. This provides
cross-platform compatibility for the test assertion.
---
Nitpick comments:
In `@src/hooks/_project.ts`:
- Around line 63-68: Remove the entire multi-line comment block that lists the
resolution order steps (items 1 through 4) in src/hooks/_project.ts. This
comment explains WHAT the code does rather than WHY, and should be replaced with
self-documenting code and variable naming. If there is important rationale about
why this resolution order is necessary, condense it into a brief single-line
comment explaining the intent rather than the implementation steps.
In `@test/worktree-project-scope.test.ts`:
- Around line 50-96: The test suite uses custom ad-hoc mock factories makeMockKV
and makeMockSdk instead of the repository's standard mocking pattern. Replace
these custom factory functions with vi.mock('iii-sdk') that provides mock
implementations for sdk.trigger, kv.get, kv.set, and kv.list methods. Then
update the registerMemorySearch function to use the mocked sdk and kv modules
directly instead of calling the factory functions. Follow the mocking pattern
demonstrated in test/crystallize.test.ts for consistency with repository
guidelines.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 92cf2504-fe6e-4c98-8b46-47f0449c8fb7
📒 Files selected for processing (39)
plugin/scripts/notification.mjsplugin/scripts/post-commit.mjsplugin/scripts/post-tool-failure.mjsplugin/scripts/post-tool-use.mjsplugin/scripts/pre-compact.mjsplugin/scripts/pre-tool-use.mjsplugin/scripts/prompt-submit.mjsplugin/scripts/session-end.mjsplugin/scripts/session-start.mjsplugin/scripts/stop.mjsplugin/scripts/subagent-start.mjsplugin/scripts/subagent-stop.mjsplugin/scripts/task-completed.mjsplugin/skills/agentmemory-config/REFERENCE.mdplugin/skills/agentmemory-mcp-tools/REFERENCE.mdsrc/functions/remember.tssrc/hooks/_project.tssrc/hooks/notification.tssrc/hooks/post-tool-failure.tssrc/hooks/post-tool-use.tssrc/hooks/pre-compact.tssrc/hooks/pre-tool-use.tssrc/hooks/prompt-submit.tssrc/hooks/session-start.tssrc/hooks/subagent-start.tssrc/hooks/subagent-stop.tssrc/hooks/task-completed.tssrc/mcp/server.tssrc/mcp/standalone.tssrc/mcp/tools-registry.tssrc/triggers/api.tstest/api-memories-project.test.tstest/hook-project.test.tstest/mcp-project-scope.test.tstest/mcp-standalone-proxy.test.tstest/mcp-standalone.test.tstest/pre-tool-use-project.test.tstest/remember-project-scope.test.tstest/worktree-project-scope.test.ts
Signed-off-by: wbu <wbu@live.de>
wbugitlab1
force-pushed
the
fix/worktree-project-scope-wbu
branch
from
eda7537 to
7f460bb
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
git:<hash>project scopeAGENTMEMORY_PROJECT_ID/AGENTMEMORY_PROJECT_NAMEoverrides and non-git basename fallbackVerification
npm test -- test/hook-project.test.ts test/pre-tool-use-project.test.ts test/worktree-project-scope.test.ts test/api-memories-project.test.ts test/mcp-project-scope.test.ts test/mcp-standalone.test.ts test/mcp-standalone-proxy.test.ts test/remember-project-scope.test.ts test/context-injection.test.ts test/copilot-plugin.test.tspassed: 10 files, 97 testsnpm run buildpassednpm run skills:checkpassedgitleaks detect --source . --log-opts origin/main..HEAD --redactpassed: 2 commits scanned, no leakssemgrep scan --config p/default --error --metrics=off $(git diff --name-only origin/main..HEAD)passed: 39 files, 0 findingsNotes
npm testpreviously failed only unrelatedtest/fs-watcher.test.tstests outside this PR scope.Summary by CodeRabbit
Release Notes
projectparameter across tools and APIs.AGENTMEMORY_PROJECT_ID/AGENTMEMORY_PROJECT_NAMEenvironment variables for explicit project selection.token_budgettomemory_recall.cwdare now normalized consistently across hooks/observations, using deterministic Git-based hashing for stability across worktrees.