fix: scope git worktrees to shared project id#929
Closed
wbugitlab1 wants to merge 2 commits into
Closed
Conversation
Signed-off-by: Willi Budzinski <w.budzinski@telekom.de>
|
Someone 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 due to trivial changes (2)
🚧 Files skipped from review as they are similar to previous changes (18)
📝 WalkthroughWalkthroughReplaces basename-based project IDs with stable ChangesCanonical Git Project Scoping and MCP Project Filtering
Sequence Diagram(s)sequenceDiagram
participant Hook as Hook (pre-tool-use)
participant Resolve as resolveProject
participant Git as git rev-parse
participant Hash as sha256
participant Enrich as /agentmemory/enrich
Hook->>Resolve: resolveProject(data.cwd)
alt AGENTMEMORY_PROJECT_ID set
Resolve-->>Hook: return trimmed PROJECT_ID
else AGENTMEMORY_PROJECT_NAME set
Resolve-->>Hook: return trimmed PROJECT_NAME
else
Resolve->>Git: --git-common-dir in cwd
Git-->>Resolve: common-dir path
Resolve->>Hash: sha256(realpath(parent))
Hash-->>Resolve: hex digest
Resolve-->>Hook: return "git:<hash>"
end
Hook->>Enrich: POST {project, cwd, ...}
Enrich-->>Hook: response
sequenceDiagram
participant Client as MCP Client
participant Tools as tools-registry
participant Server as mcp::tools::call
participant Search as mem::search
participant API as /agentmemory/search
Client->>Tools: memory_recall(query, project?)
Tools-->>Client: schema with project field
Client->>Server: call memory_recall
Server->>Search: {query, project, limit, format}
alt Proxy mode
Search->>API: POST + project parameter
API-->>Search: filtered results
else Local fallback
Search->>Search: filter InMemoryKV by project
end
Search-->>Server: results
Server-->>Client: results
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 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 |
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@plugin/scripts/notification.mjs`:
- Around line 58-59: The issue is that cwd.trim() can crash at runtime when cwd
contains a non-string truthy value (since it comes from parsed stdin JSON). The
current guard at cwd && cwd.trim() only checks truthiness, not the string type.
Add a typeof check to ensure cwd is a string before calling trim() on it in all
affected locations. Apply this same guard to the identical resolver blocks in
plugin/scripts/notification.mjs (lines 58-59),
plugin/scripts/post-tool-failure.mjs, plugin/scripts/post-tool-use.mjs, and
plugin/scripts/pre-compact.mjs. Change the condition from cwd && cwd.trim() to
typeof cwd === 'string' && cwd.trim() so that non-string values safely fall
through to the fallback process.cwd() call.
In `@src/hooks/pre-tool-use.ts`:
- Line 98: The code passes data.cwd directly to resolveProject without
validating its type, which can cause runtime errors if cwd is not a string (the
resolveProject function likely accesses string methods like trim). Add a runtime
typeof check to validate that data.cwd is actually a string before calling
resolveProject, and handle the case where it is not a string appropriately.
After fixing src/hooks/pre-tool-use.ts, regenerate
plugin/scripts/pre-tool-use.mjs to keep the bundled hook in sync with the
source.
In `@test/pre-tool-use-project.test.ts`:
- Around line 26-30: The test is not hermetic because AGENTMEMORY_PROJECT_ID and
AGENTMEMORY_PROJECT_NAME environment variables from the runner environment can
affect the test behavior, causing it to validate override behavior instead of
cwd-based resolution. Remove or explicitly exclude these two variables from the
env object configuration (where you spread process.env) in both locations
mentioned in the comment to ensure the test environment is isolated and only
tests the intended cwd-based resolution path.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 26627530-2927-4f23-85ee-7a71f2b6ff07
📒 Files selected for processing (30)
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/pre-tool-use.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
| typeof data.project === "string" && data.project.trim().length > 0 | ||
| ? data.project.trim() | ||
| : undefined; | ||
| const project = resolveProject(data.cwd as string | undefined); |
Contributor
There was a problem hiding this comment.
Validate data.cwd before calling resolveProject.
A non-string cwd from hook payload can throw at runtime (trim access), causing this hook to exit unexpectedly. Parse cwd with a runtime typeof check first.
Suggested fix
- const project = resolveProject(data.cwd as string | undefined);
+ const cwd = typeof data.cwd === "string" ? data.cwd : undefined;
+ const project = resolveProject(cwd);Please regenerate plugin/scripts/pre-tool-use.mjs after fixing this source file so the bundled hook stays in sync.
📝 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.
Suggested change
| const project = resolveProject(data.cwd as string | undefined); | |
| const cwd = typeof data.cwd === "string" ? data.cwd : undefined; | |
| const project = resolveProject(cwd); |
🤖 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/pre-tool-use.ts` at line 98, The code passes data.cwd directly to
resolveProject without validating its type, which can cause runtime errors if
cwd is not a string (the resolveProject function likely accesses string methods
like trim). Add a runtime typeof check to validate that data.cwd is actually a
string before calling resolveProject, and handle the case where it is not a
string appropriately. After fixing src/hooks/pre-tool-use.ts, regenerate
plugin/scripts/pre-tool-use.mjs to keep the bundled hook in sync with the
source.
Author
|
@coderabbitai review |
Contributor
✅ Action performedReview finished.
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@plugin/scripts/subagent-start.mjs`:
- Around line 53-56: The resolveCwd function validates the cwd parameter by
checking cwd.trim().length but returns the untrimmed cwd value, allowing
whitespace-padded strings to propagate into downstream operations like project
hashing. Modify the return statement in resolveCwd to return cwd.trim() instead
of cwd when the trimmed length is greater than zero, ensuring whitespace is
consistently removed before the value is used.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: e6dbcfcf-3dee-4876-a9c3-33927cb2e8aa
📒 Files selected for processing (23)
plugin/scripts/notification.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-start.mjsplugin/scripts/subagent-start.mjsplugin/scripts/subagent-stop.mjsplugin/scripts/task-completed.mjssrc/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.tstest/hook-project.test.tstest/pre-tool-use-project.test.ts
🚧 Files skipped from review as they are similar to previous changes (10)
- plugin/scripts/task-completed.mjs
- plugin/scripts/pre-tool-use.mjs
- plugin/scripts/notification.mjs
- plugin/scripts/post-tool-failure.mjs
- plugin/scripts/prompt-submit.mjs
- plugin/scripts/post-tool-use.mjs
- plugin/scripts/session-start.mjs
- plugin/scripts/subagent-stop.mjs
- test/hook-project.test.ts
- plugin/scripts/pre-compact.mjs
Signed-off-by: wbu <wbu@live.de>
wbugitlab1
force-pushed
the
fix/worktree-project-scope
branch
from
feeb504 to
c234cbd
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 run buildpassednpm run skills:checkpassednpx vitest run test/worktree-project-scope.test.ts test/hook-project.test.ts test/api-memories-project.test.ts test/mcp-project-scope.test.ts test/pre-tool-use-project.test.ts test/remember-project-scope.test.ts test/mcp-standalone.test.ts test/mcp-standalone-proxy.test.tspassed: 8 files, 72 testsgitleaks protect --staged --redactpassedgitleaks detect --source . --log-opts 'origin/main..HEAD' --redactpassedgit diff --name-only origin/main..HEAD | xargs semgrep scan --config p/default --error --metrics=offpassed: 30 files, 0 findingsgit diff --check origin/main..HEADpassedNotes
npm testcurrently has unrelated local failures intest/fs-watcher.test.ts; this file is not touched by this PR. Other first-run timeout failures passed when rerun in isolation.semgrep scan --config p/default --error --metrics=off .reports existing findings outside this PR diff.gitleaks detect --source . --redactreports one historical finding; the PR commit itself is clean.Manual smoke
A local build of this change was tested with two linked worktrees for the same repo. Hook-generated sessions from both worktrees resolved to the same project id:
Summary by CodeRabbit
Release Notes
New Features
AGENTMEMORY_PROJECT_ID(preferred) orAGENTMEMORY_PROJECT_NAME.projectparameter tomemory_recalland smart search tools.projectfiltering withincludeUnscoped.Improvements
cwdis normalized in event payloads.Documentation
AGENTMEMORY_PROJECT_IDand the newprojectparameter.Tests