Skip to content

fix(SUP-1496): align gbrain vault path resolution#19

Merged
levineam merged 3 commits into
mainfrom
sup-1496-gbrain-path-resolver
May 10, 2026
Merged

fix(SUP-1496): align gbrain vault path resolution#19
levineam merged 3 commits into
mainfrom
sup-1496-gbrain-path-resolver

Conversation

@levineam

@levineam levineam commented May 10, 2026

Copy link
Copy Markdown
Owner

Summary

  • Align @jarvos/gbrain with the shared jarVOS vault path resolver from @jarvos/secondbrain
  • Add vault and notes path checks to jarvos-gbrain doctor
  • Update GBrain module docs to document the shared resolver default instead of the stale ObsidianVault path

Why

SUP-1496 is the first execution slice under SUP-1425. The GBrain bridge was still defaulting to ~/Documents/ObsidianVault, while Andrew's configured jarVOS vault lives under ~/Vaults/Vault v3. Before importing curated memory, the module needs to use the same canonical vault resolver as the rest of jarVOS.

Validation

  • node --check modules/jarvos-gbrain/src/index.js && node --check modules/jarvos-gbrain/scripts/jarvos-gbrain.js
  • node --test modules/jarvos-gbrain/test/*.test.js
  • npm test --prefix modules/jarvos-gbrain
  • node modules/jarvos-gbrain/scripts/jarvos-gbrain.js doctor
  • node tests/modules-smoke-test.js
  • bash scripts/smoke-test.sh
  • npm test
  • npm test --prefix modules/jarvos-secondbrain
  • git diff --check

Parent: SUP-1425
Child: SUP-1496

Summary by CodeRabbit

  • New Features

    • Added JARVOS_NOTES_DIR env var and improved vault/notes path resolution with shared resolver and layered fallbacks.
  • Documentation

    • Updated configuration docs and defaults for vault and notes paths.
  • Bug Fixes

    • Added filesystem existence checks for vault and notes directories.
  • Tests

    • Added tests covering resolved vault/notes defaults and updated doctor tests.
  • Chores

    • Declared @jarvos/secondbrain as an optional peer dependency.

Review Change Stack

@coderabbitai

coderabbitai Bot commented May 10, 2026

Copy link
Copy Markdown

Warning

Rate limit exceeded

@levineam has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 45 minutes and 40 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 9794b6bc-b2af-4a53-9861-c8a3caa79047

📥 Commits

Reviewing files that changed from the base of the PR and between 9021432 and 968ff07.

📒 Files selected for processing (2)
  • modules/jarvos-gbrain/src/index.js
  • modules/jarvos-gbrain/test/gbrain.test.js
📝 Walkthrough

Walkthrough

The PR integrates the shared jarvos-paths resolver into gbrain configuration, updates vault/notes defaults to "Vault v3", adds optional loader/helpers and resolveConfig fallbacks, extends doctor() to validate paths, updates tests, and declares an optional peerDependency for @jarvos/secondbrain.

Changes

jarvOS Path Resolution Integration

Layer / File(s) Summary
Configuration Documentation
modules/README.md, modules/jarvos-gbrain/README.md
Updated path defaults for JARVOS_VAULT_DIR from ~/Documents/ObsidianVault to ~/Documents/Vault v3 and added JARVOS_NOTES_DIR; documented override-first resolution via shared resolver.
Constants & Manifest
modules/jarvos-gbrain/src/index.js, modules/jarvos-gbrain/package.json
Changed DEFAULT_VAULT_DIR to Documents/Vault v3, introduced optional jarvos-paths module constant, and added @jarvos/secondbrain as an optional peerDependency.
Path Loading Helpers
modules/jarvos-gbrain/src/index.js
Added helpers to conditionally require the jarvos-paths module (package or monorepo source) and call getter functions with fallbacks.
Config Resolution Logic
modules/jarvos-gbrain/src/index.js
Reworked resolveConfig to prefer explicit overrides/env/defaults then optional getters for vaultDir; notesDir selection now depends on whether a vault override was provided and can use env, derived <vault>/Notes, or optional getter.
Path Validation
modules/jarvos-gbrain/src/index.js
Extended doctor() to verify filesystem existence of config.vaultDir and config.notesDir.
Test Infrastructure
modules/jarvos-gbrain/test/gbrain.test.js
Added JARVOS_PATHS_MODULE constant and withEnv() helper to manage env vars and reset config cache during tests.
resolveConfig Test
modules/jarvos-gbrain/test/gbrain.test.js
New test creates jarvos.config.json in a temp clawd dir and asserts resolveConfig derives vaultDir and notesDir from shared jarvOS vault paths.
doctor() Test Updates
modules/jarvos-gbrain/test/gbrain.test.js
Updated existing doctor tests to pass vaultDir and notesDir in options, including the shell-metacharacter safety test.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

  • levineam/jarvOS#13: Integrates optional use of @jarvos/secondbrain’s shared path resolver and aligns defaults; strongly related to resolver usage here.
  • levineam/jarvOS#17: Prior changes to the gbrain module (src/index.js, README, tests) that this PR further modifies.

Poem

🐰 Vault v3 now leads the trail,
Resolver whispers, paths don't fail,
Notes sit tidy, joined in tow,
Tests nod yes and docs now show,
A rabbit hums — the garden's whole.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly describes the main change: aligning gbrain vault path resolution with the shared resolver, which is the primary objective of this pull request.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch sup-1496-gbrain-path-resolver

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@levineam
levineam marked this pull request as ready for review May 10, 2026 10:27

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 6a79a7c3c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread modules/jarvos-gbrain/src/index.js Outdated
Comment on lines +14 to +20
const JARVOS_PATHS_MODULE = path.resolve(
MODULE_ROOT,
'..',
'jarvos-secondbrain',
'bridge',
'config',
'jarvos-paths.js',

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Use an installable resolver path for shared config

When @jarvos/gbrain is used as an installed package rather than run from this monorepo source layout, this hard-coded sibling path points at node_modules/@jarvos/jarvos-secondbrain/..., which does not exist, so loadJarvosPaths() silently falls back and ignores jarvos.config.json/JARVOS_CLAWD_DIR. In that documented package/API usage, callers relying on the shared jarvOS resolver will import from the fallback ~/Documents/Vault v3 instead of the configured vault; the resolver needs to be loaded through an installable dependency/export or otherwise locate the shared config outside the source-tree-only sibling path.

Useful? React with 👍 / 👎.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 9021432d6d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

const notes = path.join(vault, 'Installed Notes');

try {
fs.rmSync(packageRoot, { recursive: true, force: true });

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Badge Avoid deleting the real peer dependency during tests

When this test runs in a checkout where modules/jarvos-gbrain/node_modules/@jarvos/secondbrain is an actual installed peer/workspace link, this setup removes it before writing the fake fixture and removes it again in finally, leaving the developer/CI install mutated after npm test --prefix modules/jarvos-gbrain. Use a temporary project directory or isolated NODE_PATH/cwd for the fake package instead of writing into the package's real node_modules.

Useful? React with 👍 / 👎.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick comments (1)
modules/jarvos-gbrain/package.json (1)

25-27: ⚡ Quick win

Use a semver-compatible peer range instead of exact version pin for @jarvos/secondbrain.

The exact pin to "0.1.0" can create unnecessary friction when compatible patch or minor versions are released. Since the peer dependency is optional, using ^0.1.0 allows flexibility for compatible updates while maintaining safety in the early 0.x stage.

Proposed change
   "peerDependencies": {
-    "@jarvos/secondbrain": "0.1.0"
+    "@jarvos/secondbrain": "^0.1.0"
   },
🤖 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 `@modules/jarvos-gbrain/package.json` around lines 25 - 27, Update the peer
dependency declaration for `@jarvos/secondbrain` in package.json to use a
semver-compatible range rather than an exact pin: replace the exact version
"0.1.0" under the "peerDependencies" key with a caret range like "^0.1.0" so
compatible patch/minor updates are allowed while still constraining 0.x
compatibility.
🤖 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.

Nitpick comments:
In `@modules/jarvos-gbrain/package.json`:
- Around line 25-27: Update the peer dependency declaration for
`@jarvos/secondbrain` in package.json to use a semver-compatible range rather than
an exact pin: replace the exact version "0.1.0" under the "peerDependencies" key
with a caret range like "^0.1.0" so compatible patch/minor updates are allowed
while still constraining 0.x compatibility.

ℹ️ Review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 0971c297-d1e9-4498-bb3f-61b642e02038

📥 Commits

Reviewing files that changed from the base of the PR and between 6a79a7c and 9021432.

📒 Files selected for processing (3)
  • modules/jarvos-gbrain/package.json
  • modules/jarvos-gbrain/src/index.js
  • modules/jarvos-gbrain/test/gbrain.test.js
🚧 Files skipped from review as they are similar to previous changes (2)
  • modules/jarvos-gbrain/test/gbrain.test.js
  • modules/jarvos-gbrain/src/index.js

@levineam
levineam merged commit 696078a into main May 10, 2026
6 checks passed
@levineam
levineam deleted the sup-1496-gbrain-path-resolver branch May 10, 2026 14:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant