feat(SUP-1506): integrate JarVOS GBrain memory stack#27
Merged
Conversation
📝 WalkthroughWalkthroughThis PR extends ChangesGBrain Graph Recall & Multi-Engine Evaluation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
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 docstrings
🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
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}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
modules/jarvos-gbrain/src/index.js (2)
1084-1102: 💤 Low valueDocument the intentional
ok = recallResult.okoverwrite forcompareRecall.In
compareRecallmode, the per-questionokis reset torecallResult.ok(then AND'd only withengines.qmd.ok/engines.gbrain_graph.okwhen present), so a failing direct-search engine no longer fails the question. This is exercised and asserted by the "score combined runtime recall separately from direct search" test, but theok =(vsok = ok && …) is easy to mistake for a bug on first read. A short comment captures the design intent.📝 Suggested comment
if (compareRecall) { const recallQuery = typeof entry === 'object' ? queryForEngine(entry, 'recall', query) : query; const expectedCandidates = recallExpectedCandidates(entry); const recallResult = runRecallEval( config, entry, recallQuery, expectedCandidates, dryRun, limit, graphDepth, graphSeedLimit, ); engines.gbrain_recall = recallResult; engineQueries.gbrain_recall = recallQuery; + // When recall scoring is enabled it becomes authoritative for the question's + // pass/fail; direct GBrain search status is reported per-engine but no longer + // fails the question on its own. QMD/graph engines (if enabled) still gate ok. ok = recallResult.ok; if (engines.qmd) ok = ok && engines.qmd.ok; if (engines.gbrain_graph) ok = ok && engines.gbrain_graph.ok; }🤖 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/src/index.js` around lines 1084 - 1102, The assignment ok = recallResult.ok in the compareRecall branch is intentional (it resets the per-question success to the recall evaluation result rather than preserving previous direct-search ok), so add a concise comment above that line explaining the design intent: that in compareRecall mode the question's pass/fail is determined by recallResult.ok and then only AND'd with engines.qmd.ok and engines.gbrain_graph.ok when present; reference the variables runRecallEval, recallResult, ok, engines.qmd and engines.gbrain_graph so reviewers know this overwrite is deliberate and not a typo.
82-104: ⚡ Quick winWrap shared-paths getter calls in a guard against runtime exceptions.
loadJarvosPathsonly catches resolution/requirefailures, butsharedPathOrFallbackinvokesjarvosPaths[getterName]()unguarded. If a future@jarvos/secondbrainbuild throws fromgetVaultDir()/getNotesDir()(e.g., on a misconfigured jarvOS workspace), that exception escapes fromresolveConfigand breaks every public API that resolves config first (createImportPlan,syncBrain,recallBundle,doctor, etc.). A small try/catch keeps the documented "fall back to portable defaults" guarantee intact.♻️ Proposed defensive call
function sharedPathOrFallback(jarvosPaths, getterName, fallback) { if (jarvosPaths && typeof jarvosPaths[getterName] === 'function') { - return jarvosPaths[getterName](); + try { + const value = jarvosPaths[getterName](); + if (typeof value === 'string' && value.trim()) return value; + } catch { + // Fall back to portable defaults if the shared resolver throws. + } } return fallback; }🤖 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/src/index.js` around lines 82 - 104, sharedPathOrFallback currently calls jarvosPaths[getterName]() without guarding runtime errors, so wrap that invocation in a try/catch: inside sharedPathOrFallback, check the function exists as you do, then call it inside a try block and return its result; on any exception catch it and return the provided fallback (optionally log the error). This ensures callers like resolveConfig (and downstream APIs createImportPlan, syncBrain, recallBundle, doctor) keep the documented "fall back to portable defaults" behavior even if getVaultDir/getNotesDir throw.
🤖 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 `@modules/jarvos-gbrain/scripts/jarvos-gbrain.js`:
- Line 61: The usage string printed via process.stdout.write in jarvos-gbrain.js
is missing the supported recall flags; update the printed help text (the
multi-line string passed to process.stdout.write) so the recall command line
includes the missing options --max-chars and --dry-run (keeping existing flags
like --no-qmd, --no-graph, --graph-seed, --graph-depth, --graph-seed-limit,
--limit, --timeout-ms, --format). Ensure the recall line in that string is
updated so CLI help accurately reflects all supported recall options.
- Around line 24-29: argValues currently treats any next argv token as a value
even if it's another flag (e.g., "-x"), causing silent misparsing; update the
argValues(name) function to only push process.argv[index+1] when it exists and
does not look like a flag (e.g., does not match /^-/), so skip/ignore tokens
that start with '-' as repeated-arg values; reference the argValues function and
the process.argv[index+1] check to implement this guard.
---
Nitpick comments:
In `@modules/jarvos-gbrain/src/index.js`:
- Around line 1084-1102: The assignment ok = recallResult.ok in the
compareRecall branch is intentional (it resets the per-question success to the
recall evaluation result rather than preserving previous direct-search ok), so
add a concise comment above that line explaining the design intent: that in
compareRecall mode the question's pass/fail is determined by recallResult.ok and
then only AND'd with engines.qmd.ok and engines.gbrain_graph.ok when present;
reference the variables runRecallEval, recallResult, ok, engines.qmd and
engines.gbrain_graph so reviewers know this overwrite is deliberate and not a
typo.
- Around line 82-104: sharedPathOrFallback currently calls
jarvosPaths[getterName]() without guarding runtime errors, so wrap that
invocation in a try/catch: inside sharedPathOrFallback, check the function
exists as you do, then call it inside a try block and return its result; on any
exception catch it and return the provided fallback (optionally log the error).
This ensures callers like resolveConfig (and downstream APIs createImportPlan,
syncBrain, recallBundle, doctor) keep the documented "fall back to portable
defaults" behavior even if getVaultDir/getNotesDir throw.
🪄 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: bd79b5c8-32f5-49dd-80bc-d4528c2b512a
📒 Files selected for processing (11)
README.mddocs/architecture/jarvos-architecture.mdmodules/README.mdmodules/jarvos-gbrain/README.mdmodules/jarvos-gbrain/config/eval-questions.jsonmodules/jarvos-gbrain/package.jsonmodules/jarvos-gbrain/scripts/jarvos-gbrain.jsmodules/jarvos-gbrain/src/index.jsmodules/jarvos-gbrain/test/gbrain.test.jsruntimes/openclaw/README.mdstarter-kit/README.md
Comment on lines
+24
to
+29
| function argValues(name) { | ||
| const values = []; | ||
| for (let index = 0; index < process.argv.length; index += 1) { | ||
| if (process.argv[index] === name && process.argv[index + 1]) values.push(process.argv[index + 1]); | ||
| } | ||
| return values; |
There was a problem hiding this comment.
Reject flag tokens as repeated-arg values.
On Line 27, argValues() will treat another flag as a value if a value is missing, which causes silent misparsing.
Suggested fix
function argValues(name) {
const values = [];
for (let index = 0; index < process.argv.length; index += 1) {
- if (process.argv[index] === name && process.argv[index + 1]) values.push(process.argv[index + 1]);
+ if (process.argv[index] !== name) continue;
+ const value = process.argv[index + 1];
+ if (!value || value.startsWith('--')) {
+ throw new Error(`${name} requires a value`);
+ }
+ values.push(value);
}
return values;
}🤖 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/scripts/jarvos-gbrain.js` around lines 24 - 29,
argValues currently treats any next argv token as a value even if it's another
flag (e.g., "-x"), causing silent misparsing; update the argValues(name)
function to only push process.argv[index+1] when it exists and does not look
like a flag (e.g., does not match /^-/), so skip/ignore tokens that start with
'-' as repeated-arg values; reference the argValues function and the
process.argv[index+1] check to implement this guard.
|
|
||
| function usage() { | ||
| process.stdout.write(`jarvos-gbrain\n\nCommands:\n plan [--manifest path]\n import [--dry-run] [--manifest path] [--brain-dir path] [--vault-dir path]\n sync [--dry-run] [--brain-dir path] [--gbrain-dir path]\n eval [--dry-run] [--eval-file path]\n doctor\n\n`); | ||
| process.stdout.write(`jarvos-gbrain\n\nCommands:\n plan [--manifest path]\n import [--dry-run] [--manifest path] [--brain-dir path] [--vault-dir path]\n sync [--dry-run] [--brain-dir path] [--gbrain-dir path]\n eval [--dry-run] [--eval-file path] [--compare-qmd] [--compare-graph] [--compare-recall]\n [--graph-depth n] [--graph-seed-limit n]\n [--limit n] [--timeout-ms n] [--qmd-bin path] [--qmd-mode search|query|vsearch]\n [--qmd-collection name] [--qmd-index name]\n graph [--dry-run] --seed slug [--seed slug] [--depth n] [--timeout-ms n]\n recall --query text [--no-qmd] [--no-graph] [--graph-seed slug] [--graph-depth n]\n [--graph-seed-limit n] [--limit n] [--timeout-ms n] [--format markdown]\n doctor\n\n`); |
There was a problem hiding this comment.
Keep usage() in sync with supported recall flags.
Line 61 omits supported recall options (--max-chars and --dry-run), so the CLI help is currently incomplete.
Suggested fix
- recall --query text [--no-qmd] [--no-graph] [--graph-seed slug] [--graph-depth n]
- [--graph-seed-limit n] [--limit n] [--timeout-ms n] [--format markdown]
+ recall [--dry-run] --query text [--no-qmd] [--no-graph] [--graph-seed slug]
+ [--graph-depth n] [--graph-seed-limit n] [--limit n] [--max-chars n]
+ [--timeout-ms n] [--format markdown]🤖 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/scripts/jarvos-gbrain.js` at line 61, The usage string
printed via process.stdout.write in jarvos-gbrain.js is missing the supported
recall flags; update the printed help text (the multi-line string passed to
process.stdout.write) so the recall command line includes the missing options
--max-chars and --dry-run (keeping existing flags like --no-qmd, --no-graph,
--graph-seed, --graph-depth, --graph-seed-limit, --limit, --timeout-ms,
--format). Ensure the recall line in that string is updated so CLI help
accurately reflects all supported recall options.
This was referenced May 11, 2026
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
main.Stack disposition
main.main.main; their changes are included here through the top branch.Verification
/Users/andrew/clawd/memory/projects/jarvos-gbrain/final-audit-preflight-2026-05-10.md.node /Users/andrew/clawd/scripts/jarvos-memory-final-audit.js.Paperclip
Summary by CodeRabbit
New Features
Documentation