Fix: unified environment behavior#975
Open
SSgumS wants to merge 3 commits into
Open
Conversation
All hook scripts read AGENTMEMORY_* vars at module level (const declarations before main()). If those vars live only in ~/.agentmemory/.env and not in the shell environment or settings.json, they were silently ignored. Fix: - Add src/hooks/_env.ts: synchronous .env parser that populates process.env for keys not already set (explicit env takes priority). - Import side-effect in src/hooks/_project.ts (already inlined first by the bundler into every hook that imports _project). - Direct import in the four hooks that bypass _project.ts: pre-tool-use, session-end, post-commit, stop. - Patch all 14 compiled plugin/scripts/*.mjs with an inline equivalent so the installed plugin works without a rebuild.
|
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. |
SSgumS
changed the title
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 (14)
🚧 Files skipped from review as they are similar to previous changes (14)
📝 WalkthroughWalkthroughThe PR adds startup loading of ChangesEnvironment bootstrap across hooks and scripts
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 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.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/hooks/_env.ts (1)
8-30: 📐 Maintainability & Code Quality | 🔵 TrivialRefactor
src/hooks/_env.tsto use the existingdotenvlibraryThe custom
.envparser insrc/hooks/_env.tsduplicates functionality already available via thedotenvdependency. The existing implementation manually handles whitespace, quoting, and comments while strictly avoiding the override of existing environment variables. This behavior matchesdotenv.config()defaults exactly.Replace the manual parsing logic with:
dotenv.config({ path: join(homedir(), ".agentmemory", ".env") });This change reduces boilerplate and improves maintainability. The custom parser is still needed in
src/hooks/_env.mjs(or similar bundled artifacts) if Node.js module resolution preventsdotenvusage, but can be safely removed from the source file where Node.js runtime is available.🤖 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/_env.ts` around lines 8 - 30, `src/hooks/_env.ts` reimplements `.env` loading manually even though `dotenv` already matches the same parsing and non-override behavior. Update the module’s initialization logic to use `dotenv.config()` with the existing env path built from `homedir()` and `join()`, and remove the custom line-by-line parsing/try-catch block. Keep the unique `envPath` construction and the `dotenv` import in place so the source hook stays simple and consistent with the existing dependency.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.
Inline comments:
In `@plugin/scripts/pre-compact.mjs`:
- Around line 6-21: The environment loader block is present in the other hook
scripts but missing from diagnostics.mjs, leaving that script inconsistent with
the shared source of truth. Update diagnostics.mjs to include the same _env
loader behavior used by pre-compact.mjs and notification.mjs, or alternatively
consolidate the loader into a shared strategy so all hook entrypoints, including
diagnostics.mjs, resolve environment variables consistently.
In `@src/hooks/_project.ts`:
- Line 3: The module initialization order in _project.ts is wrong because
"./_env.js" is imported after the node:child_process and node:path imports,
which can delay environment setup. Move the "./_env.js" import to the very top
of src/hooks/_project.ts so it runs before any other module-level imports or
initialization, matching the pattern used in stop.ts and ensuring
AGENTMEMORY_PROJECT_NAME is available immediately.
---
Nitpick comments:
In `@src/hooks/_env.ts`:
- Around line 8-30: `src/hooks/_env.ts` reimplements `.env` loading manually
even though `dotenv` already matches the same parsing and non-override behavior.
Update the module’s initialization logic to use `dotenv.config()` with the
existing env path built from `homedir()` and `join()`, and remove the custom
line-by-line parsing/try-catch block. Keep the unique `envPath` construction and
the `dotenv` import in place so the source hook stays simple and consistent with
the existing dependency.
🪄 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: a5221502-2e88-407e-89b3-8678dc224d20
📒 Files selected for processing (19)
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.mjssrc/hooks/_env.tssrc/hooks/_project.tssrc/hooks/post-commit.tssrc/hooks/pre-tool-use.tssrc/hooks/session-end.tssrc/hooks/stop.ts
dotenv is already a project dependency (^17.4.2) and provides the same non-override behavior (dotenv.config() does not overwrite existing keys) with no custom parsing or error handling needed.
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 `@src/hooks/_env.ts`:
- Line 8: The dotenv loader in the _env hook is emitting runtime banners on
stdout, which can interfere with hook protocol output. Update the dotenv.config
call in the _env setup to pass quiet: true while keeping the existing
non-override behavior unchanged, and make the same adjustment in the shared
loader path used by the generated plugin/scripts copies so the fix propagates
consistently.
🪄 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: ee0c022e-c674-458e-8ac4-4d5757bce8c5
📒 Files selected for processing (14)
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.mjssrc/hooks/_env.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- plugin/scripts/stop.mjs
- plugin/scripts/post-tool-use.mjs
dotenv v17 logs "◇ injected env (N) from ..." to stdout by default. Hooks use stdout for protocol output (session-start writes context there), so this banner corrupts the payload. quiet: true silences it.
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.
fix(hooks): load ~/.agentmemory/.env before reading process.env
All hook scripts read AGENTMEMORY_* vars at module level (const declarations before main()). If those vars live only in ~/.agentmemory/.env and not in the shell environment or settings.json, they were silently ignored.
Fix:
Summary by CodeRabbit
New Features
~/.agentmemory/.envat startup (quietly), before configuration is read—covering session, tool, stop, and related hooks.Bug Fixes