SUP-3237 Prepare jarvOS v0.6 secondbrain release candidate - #84
Conversation
|
Warning Review limit reached
More reviews will be available in 37 minutes and 42 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based credits. 🚦 How do rate limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan refill rate. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, the refill rate gradually slows as usage increases. The highest same-day bursts are limited more strictly. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (9)
📝 WalkthroughWalkthroughThis PR introduces portable ambient capture contracts and routing, adds session-source and universal capture entrypoints, centralizes config resolution and vault onboarding, expands provenance and journal repair tooling, adds note schema and knowledge-unit outputs, and introduces generated wiki, synthesis, retrieval evaluation, status reporting, and boundary documentation. Changesjarvos-secondbrain boundary and runtime stack
Sequence Diagram(s)sequenceDiagram
participant Tool as Session Tool
participant Adapter as Session Adapter
participant Capture as Universal Capture
participant Routing as Ambient Routing
participant Storage as Vault/Memory/Paperclip Adapters
Tool->>Adapter: parsed session
Adapter->>Capture: CaptureEvent v2
Capture->>Routing: normalized capture + frontmatter
Routing->>Storage: note/journal/memory/work actions
Storage-->>Capture: written results
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
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 |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f7f8ce09a1
ℹ️ 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 { | ||
| createVaultStorageAdapter, | ||
| } = require('./obsidian/src/vault-storage-adapter.js'); | ||
| const memoryRecord = require('../../jarvos-memory/src'); |
There was a problem hiding this comment.
When @jarvos/secondbrain is installed from its own npm tarball, this relative import escapes the package and looks for ../../jarvos-memory/src; the tarball only contains modules/jarvos-secondbrain, and adapters/index.js eagerly loads this file, so simply requiring the package fails with MODULE_NOT_FOUND. Use a declared package dependency such as @jarvos/memory or lazy-load this only when an injected memory adapter is absent.
Useful? React with 👍 / 👎.
| // A note belongs in a given day's journal Notes section if it was CREATED that day | ||
| // or UPDATED that day (per the journal-module contract: "created or updated that day"). | ||
| function noteMatchesDate(note, dateYmd, fmt) { | ||
| return fmt(note.createdAt) === dateYmd || note.updated === dateYmd; |
There was a problem hiding this comment.
For notes edited manually or by external tools that do not maintain updated frontmatter, note.updated is null even though the file mtime is on the audited day, so these notes are no longer returned by findNotesForDate and their journal backlinks are missed. This regresses the existing behavior/test that used mtime to satisfy the “created or updated that day” contract.
Useful? React with 👍 / 👎.
|
|
||
| const test = require('node:test'); | ||
| const assert = require('node:assert/strict'); | ||
| const Ajv = require('ajv'); |
There was a problem hiding this comment.
In a clean checkout there is no ajv dependency declared for the repo root or modules/jarvos-secondbrain, so npm test in this package fails immediately with Cannot find module 'ajv' before these skill-contract checks run. Add it as a dev dependency or avoid the external validator so the advertised package test command works.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 1
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
modules/jarvos-secondbrain/bridge/routing/src/three-package-router.js (1)
157-159:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAvoid attaching
noteRefwhen note persistence failed.Line 157 currently checks only truthiness, so a failed write result (e.g.,
written: false) can still producememory.noteRefto a non-existent note.Proposed fix
- if (keywordResult.note) { + const noteWritten = Boolean(keywordResult.note) + && keywordResult.note.written !== false + && keywordResult.note.error == null; + if (noteWritten) { params.noteRef = keywordResult.note.title || keywordResult.note.path || undefined; }🤖 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-secondbrain/bridge/routing/src/three-package-router.js` around lines 157 - 159, In the condition that checks `keywordResult.note` to assign `params.noteRef`, add an additional check to verify that the note was successfully written to persistence (checking the written status). Currently, the code only verifies that the note object exists, but does not confirm that the write operation succeeded. Modify the if condition to require both that the note exists AND that it was successfully persisted before assigning the noteRef value to params.
🟡 Minor comments (14)
modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lib/notes-config.js-25-31 (1)
25-31:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat
Source Materialas valid only when it is a directory.On Line 29,
existsSyncalso returns true for regular files. Returning a file path here can break downstream directory reads.Suggested fix
function getVaultSourceMaterialDir() { const config = loadConfig(); if (config.paths.sourceMaterial) return config.paths.sourceMaterial; const plainSourceMaterial = path.join(config.paths.vault, 'Source Material'); - if (fs.existsSync(plainSourceMaterial)) return plainSourceMaterial; + if (fs.existsSync(plainSourceMaterial) && fs.statSync(plainSourceMaterial).isDirectory()) { + return plainSourceMaterial; + } return path.join(config.paths.vault, '2 - Source Material'); }🤖 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-secondbrain/packages/jarvos-secondbrain-notes/src/lib/notes-config.js` around lines 25 - 31, In the getVaultSourceMaterialDir function, the check on line 29 uses fs.existsSync which returns true for both files and directories. Replace this check with a method that specifically validates that plainSourceMaterial is a directory, not just any existing path. Use fs.statSync with isDirectory() method or an equivalent approach, and wrap it in proper error handling to ensure only actual directories are returned as valid source material paths.modules/jarvos-secondbrain/bridge/config/src/shared-vault-onboarding.js-29-35 (1)
29-35:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRequire
NotesandJournalto be directories, not just existing paths.On Line 32 and Line 33,
existsSyncalone will also pass regular files. That can onboard an invalid vault and fail later when code assumes directory semantics.Suggested fix
function hasSharedVaultShape(vaultDir) { + const notesPath = path.join(vaultDir || '', 'Notes'); + const journalPath = path.join(vaultDir || '', 'Journal'); return Boolean( vaultDir - && fs.existsSync(path.join(vaultDir, 'Notes')) - && fs.existsSync(path.join(vaultDir, 'Journal')), + && fs.existsSync(notesPath) + && fs.existsSync(journalPath) + && fs.statSync(notesPath).isDirectory() + && fs.statSync(journalPath).isDirectory(), ); }🤖 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-secondbrain/bridge/config/src/shared-vault-onboarding.js` around lines 29 - 35, The hasSharedVaultShape function uses fs.existsSync to validate the Notes and Journal paths, but this method returns true for both files and directories. Replace the fs.existsSync calls for the 'Notes' and 'Journal' paths with checks that specifically verify these are directories, not just that they exist. Use fs.statSync combined with isDirectory() method on the returned stats object, or an equivalent method that ensures the paths are actual directories rather than regular files.modules/jarvos-secondbrain/bridge/config/src/shared-vault-onboarding.js-104-118 (1)
104-118:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winValidate option values so flags cannot be consumed as argument values.
On Line 104–118, value-taking flags always consume the next token. Inputs like
--vault --workspace /tmp/wssetvaultDirto--workspaceand desynchronize parsing.Suggested fix
function parseArgs(argv) { const options = {}; + function readValue(flag, index) { + const value = argv[index + 1]; + if (!value || value.startsWith('--')) { + throw new Error(`Missing value for ${flag}`); + } + return value; + } for (let index = 0; index < argv.length; index += 1) { const arg = argv[index]; - const next = argv[index + 1]; if (arg === '--vault') { - options.vaultDir = next; + options.vaultDir = readValue(arg, index); index += 1; } else if (arg === '--workspace') { - options.workspaceRoot = next; + options.workspaceRoot = readValue(arg, index); index += 1; } else if (arg === '--config') { - options.configPath = next; + options.configPath = readValue(arg, index); index += 1; } else if (arg === '--name') { - options.user = { ...(options.user || {}), name: next }; + options.user = { ...(options.user || {}), name: readValue(arg, index) }; index += 1; } else if (arg === '--timezone') { - options.user = { ...(options.user || {}), timezone: next }; + options.user = { ...(options.user || {}), timezone: readValue(arg, index) }; index += 1;🤖 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-secondbrain/bridge/config/src/shared-vault-onboarding.js` around lines 104 - 118, Add validation to prevent flags from being consumed as argument values in the option parsing logic. In the conditional blocks handling arg === '--vault', arg === '--workspace', arg === '--config', arg === '--name', and arg === '--timezone', add a check to verify that next exists and does not start with '--' before assigning it as the option value and incrementing the index. This prevents misalignment when flags appear consecutively without proper values.modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lint-source-material.js-68-72 (1)
68-72:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate help usage to the actual executable path.
Line 71 advertises
node scripts/lint-source-material.js, but this file lives undersrc/. The current help text will direct users to a non-existent path in package-local usage.🤖 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-secondbrain/packages/jarvos-secondbrain-notes/src/lint-source-material.js` around lines 68 - 72, The console.log statement displaying the usage help text in the linting script incorrectly references the command as `node scripts/lint-source-material.js` when the file is actually located in the `src/` directory. Update the Usage line in the console.log help message to show the correct path to this executable file, which should reflect where the file actually lives so that users can correctly invoke the script from the command line.modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/docs/SOURCE_MATERIAL_PROVENANCE_CONTRACT.md-35-37 (1)
35-37:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix the lint command path in the contract doc.
Line 36 points to
scripts/lint-source-material.js, but this PR adds the CLI at packagesrc/lint-source-material.js(and asjarvos-lint-source-materialviabin). This will mislead users running the documented audit command.Suggested doc patch
- node scripts/lint-source-material.js --json + node src/lint-source-material.js --json +# or, if installed via package bin: +# jarvos-lint-source-material --json🤖 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-secondbrain/packages/jarvos-secondbrain-notes/docs/SOURCE_MATERIAL_PROVENANCE_CONTRACT.md` around lines 35 - 37, The bash command in the SOURCE_MATERIAL_PROVENANCE_CONTRACT.md documentation references an incorrect path for the lint tool. Update the lint command from pointing to scripts/lint-source-material.js to use the correct CLI reference. Either update it to reference src/lint-source-material.js from the package location, or better yet, use the binary command jarvos-lint-source-material which is exposed via the bin configuration and provides a more user-friendly way to run the audit command.modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/migrations/note_contract_migration.py-525-525 (1)
525-525:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winApply the same path normalization to
JARVOS_ARTIFACTS_DIR.Line 525 skips
.expanduser().resolve()in the env-default branch, unlike the explicit--report-dirpath branch. This can produce literal~folders or cwd-dependent artifact locations.Suggested fix
- report_dir = Path(os.environ.get("JARVOS_ARTIFACTS_DIR", os.path.join(os.path.expanduser("~"), "clawd", "artifacts", "note-contract-migration"))) / utc_stamp() + base = Path( + os.environ.get( + "JARVOS_ARTIFACTS_DIR", + os.path.join(os.path.expanduser("~"), "clawd", "artifacts", "note-contract-migration"), + ) + ).expanduser().resolve() + report_dir = base / utc_stamp()🤖 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-secondbrain/packages/jarvos-secondbrain-notes/migrations/note_contract_migration.py` at line 525, The report_dir assignment is missing path normalization on the JARVOS_ARTIFACTS_DIR environment variable default branch. The path constructed from os.environ.get("JARVOS_ARTIFACTS_DIR", ...) needs to have .expanduser() and .resolve() applied to it, consistent with how the explicit --report-dir path is normalized elsewhere in the code. Convert the report_dir Path object to apply these normalization methods to ensure tilde expansion and absolute path resolution are consistently handled for both the environment variable branch and the default fallback path.modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/migrations/backfill_notes_frontmatter.py-214-214 (1)
214-214:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
--notes-dirdefaults before use.Line 214 now directly consumes
JARVOS_VAULT_NOTES; if it contains~, this script keeps it unexpanded (laterPath(args.notes_dir)), which can break discovery. Expand/resolve this path before use.Suggested fix
- ap.add_argument("--notes-dir", default=os.environ.get("JARVOS_VAULT_NOTES", os.path.join(os.path.expanduser("~"), "Vaults", "Vault v3", "Notes"))) + ap.add_argument( + "--notes-dir", + default=os.path.expanduser( + os.environ.get( + "JARVOS_VAULT_NOTES", + os.path.join(os.path.expanduser("~"), "Vaults", "Vault v3", "Notes"), + ) + ), + )🤖 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-secondbrain/packages/jarvos-secondbrain-notes/migrations/backfill_notes_frontmatter.py` at line 214, The `--notes-dir` default value on line 214 may contain an unexpanded `~` from the `JARVOS_VAULT_NOTES` environment variable, which will cause path discovery to fail when later used with `Path(args.notes_dir)`. Wrap the `os.environ.get("JARVOS_VAULT_NOTES", ...)` call with `os.path.expanduser()` to ensure any tilde characters in the environment variable are properly expanded to the user's home directory, matching the behavior already applied to the fallback default path.modules/jarvos-secondbrain/README.md-102-103 (1)
102-103:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize
jarvOScapitalization in the universal-capture section.
jarVOS-ownedis inconsistent with the release-wide capitalization normalization.📝 Suggested doc fix
-Agents should call the jarVOS-owned capture entrypoint instead of raw-writing +Agents should call the jarvOS-owned capture entrypoint instead of raw-writing🤖 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-secondbrain/README.md` around lines 102 - 103, The capitalization of `jarVOS-owned` in the universal-capture section is inconsistent with the release-wide capitalization normalization standard. Update the capitalization of `jarVOS-owned` in the sentence beginning with "Agents should call the jarVOS-owned capture entrypoint" to match the standardized capitalization used consistently throughout the release documentation.modules/jarvos-secondbrain/README.md-86-89 (1)
86-89:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winBootstrap-state bullet is outdated for this release.
The statement that bridge/adapter directories contain no logic no longer matches the current code surface and release scope, so the README now misrepresents what ships in v0.6.
📝 Suggested doc fix
-- Bridge and adapter directories are present but intentionally contain no logic yet. +- Bridge and adapter directories now contain capture/routing and adapter integration logic for the public secondbrain surface.🤖 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-secondbrain/README.md` around lines 86 - 89, The README.md statement in the bootstrap-state section that claims "Bridge and adapter directories are present but intentionally contain no logic yet" is outdated and no longer reflects the current v0.6 release implementation. Update or remove this bullet point to accurately represent that bridge and adapter directories now contain actual logic and implementation in this release, ensuring the documentation matches what is actually shipped in the code.modules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/capture-contract.js-228-232 (1)
228-232:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winEvidence offsets should enforce
start <= end.
startandendare each validated independently, but reversed spans are currently accepted.🛠️ Suggested fix
for (const field of ['start', 'end']) { if (entry[field] != null && (!Number.isInteger(entry[field]) || entry[field] < 0)) { errors.push(`${prefix}.${field} must be a non-negative integer`); } } + if ( + Number.isInteger(entry.start) && + Number.isInteger(entry.end) && + entry.end < entry.start + ) { + errors.push(`${prefix}.end must be greater than or equal to ${prefix}.start`); + } }🤖 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-secondbrain/packages/jarvos-ambient/src/intent/capture-contract.js` around lines 228 - 232, The validation loop for the `start` and `end` fields checks that each field is independently a non-negative integer, but does not enforce the constraint that start must be less than or equal to end. After the loop that validates individual fields, add an additional check to ensure that when both start and end are present and valid, start is not greater than end. If this condition is violated, push an error message to the errors array indicating that the start offset must be less than or equal to the end offset.modules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/salience-detector.js-137-143 (1)
137-143:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winManual salience override is not validated against canonical classes.
Any truthy
capture.salienceClassreplaces classifier output, including unknown values.🛠️ Suggested fix
'use strict'; +const { SALIENCE_CLASSES } = require('./capture-contract'); @@ - if (capture.salienceClass) { + if ( + typeof capture.salienceClass === 'string' && + SALIENCE_CLASSES.includes(capture.salienceClass) + ) { return { ...classification, salienceClass: capture.salienceClass, keywordOverride: false, }; }🤖 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-secondbrain/packages/jarvos-ambient/src/intent/salience-detector.js` around lines 137 - 143, The code in the salience-detector.js file currently accepts any truthy value for capture.salienceClass without validating it against canonical salience classes. To fix this, add a validation check before returning the overridden classification that verifies capture.salienceClass is a valid/canonical salience class value. Only return the object with the overridden salienceClass if the validation passes; otherwise, allow the code to continue to the next condition or return the original classification based on the classifier output.modules/jarvos-secondbrain/bridge/capture/README.md-3-3 (1)
3-3:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winFix product-name capitalization drift in docs.
Line 3 uses
jarVOS-owned; this conflicts with the release normalization target (jarvOS) and can cause public-facing inconsistency.Suggested fix
-jarVOS-owned universal capture entrypoint for intentional notes and ideas. +jarvOS-owned universal capture entrypoint for intentional notes and ideas.🤖 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-secondbrain/bridge/capture/README.md` at line 3, The product name in the README.md file has inconsistent capitalization. The text "jarVOS-owned" in the description uses a different capitalization format than the normalized standard "jarvOS". Update the product name reference in the introductory sentence to use the correct capitalization that matches the release normalization target to ensure consistency across all public-facing documentation.modules/jarvos-secondbrain/bridge/routing/README.md-13-14 (1)
13-14:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winNormalize product capitalization in this public doc.
Line 13 says
jarVOS-owned; this conflicts with the release-widejarvOScapitalization normalization and should be aligned.🤖 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-secondbrain/bridge/routing/README.md` around lines 13 - 14, The capitalization of the product name in the README.md file is inconsistent with the release-wide normalization standard. In the line describing the CaptureEvent v2 entrypoint that currently reads "jarVOS-owned CaptureEvent v2", change "jarVOS-owned" to "jarvOS-owned" to align with the standardized "jarvOS" capitalization convention used across the release.modules/jarvos-secondbrain/bridge/provenance/src/journal-note-audit.js-5-6 (1)
5-6:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winKeep the usage contract aligned with updated-note matching.
Line 5 says the audit detects notes “created” on the date, but Lines 152-155 now include notes whose frontmatter
updated:matches. Please update this wording so dry-run output and operator expectations match the actual audit scope.Proposed wording update
- * Detects notes created today (or on a given date) in the vault Notes/ - * directory and checks whether each is already linked in that day's journal + * Detects notes created or updated today (or on a given date) in the vault + * Notes/ directory and checks whether each is already linked in that day's journal🤖 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-secondbrain/bridge/provenance/src/journal-note-audit.js` around lines 5 - 6, The documentation comment at the beginning of the file describing the audit detection scope is outdated and does not match the current implementation. The comment at lines 5-6 states the audit detects notes "created" on the date, but the actual implementation in lines 152-155 includes notes where the frontmatter "updated:" field matches the date as well. Update the opening comment to clarify that the audit detects both notes created and updated on the given date to align the usage contract with the actual audit behavior and operator expectations.
🧹 Nitpick comments (6)
modules/jarvos-secondbrain/docs/contracts/JARVOS_SECONDBRAIN_PACKAGE_MAP_2026-03-25.md (1)
52-54: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueReduce repetitive sentence structure in the classification rule.
Three successive sentences (lines 52–54) begin with "If", affecting readability. Consider restructuring to vary the pattern.
📝 Suggested rephrase
## Rule of interpretation -If a surface owns human-readable content, it belongs in `jarvos-secondbrain`. -If it owns compact durable recall, it belongs in `jarvos-memory`. -If it owns execution state, it belongs in Paperclip. +- Human-readable content belongs in `jarvos-secondbrain`. +- Compact durable recall belongs in `jarvos-memory`. +- Execution state belongs in Paperclip.🤖 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-secondbrain/docs/contracts/JARVOS_SECONDBRAIN_PACKAGE_MAP_2026-03-25.md` around lines 52 - 54, The three classification rules in the document (lines 52-54) all begin with "If", creating repetitive sentence structure that impacts readability. Restructure these three sentences to vary the opening pattern while preserving the classification logic and clarity. You can vary the structure by rephrasing some sentences to use different grammatical constructions or inversions, ensuring each rule about human-readable content, compact durable recall, and execution state ownership remains clear and distinct.modules/jarvos-secondbrain/docs/architecture/automatic-secondbrain-public-boundary.md (1)
51-54: 🧹 Nitpick | 🔵 Trivial | 💤 Low valueReduce repetitive sentence structure in the checklist.
Four successive sentences (lines 51–54) begin with "Run", which affects readability. Consider varying the structure while preserving clarity.
📝 Suggested rephrase
Before public promotion: -- Run the secondbrain package tests. -- Run memory and ontology promotion-gate tests. -- Run release-intake lint. -- Run privacy/path scans over public docs, fixtures, generated pages, and config - templates. +- The secondbrain package tests should pass. +- Memory and ontology promotion-gate tests should pass. +- Release-intake lint should complete without errors. +- Privacy/path scans over public docs, fixtures, generated pages, and config templates should find no issues.(Alternately, use a bullet-list structure with imperative verbs: "Verify X", "Confirm Y", "Record Z".)
🤖 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-secondbrain/docs/architecture/automatic-secondbrain-public-boundary.md` around lines 51 - 54, The four consecutive checklist items starting with "Run" (secondbrain package tests, memory and ontology promotion-gate tests, release-intake lint, and privacy/path scans) create repetitive sentence structure that reduces readability. Vary the imperative verbs used to start each bullet point by replacing some instances of "Run" with alternative verbs such as "Verify", "Confirm", "Check", or "Execute" while preserving the clarity of what actions need to be performed for each item.modules/jarvos-secondbrain/tests/generated-wiki.test.js (1)
46-81: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression case for same-title sources from different note paths.
Current suite misses the collision scenario where two artifacts share a title. Add one test asserting both source pages are preserved with distinct output paths.
🤖 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-secondbrain/tests/generated-wiki.test.js` around lines 46 - 81, Add a new test case in the test file that specifically covers the scenario where multiple artifacts share the same title but originate from different note paths. This test should call writeArtifact at least twice with identical titles but different journalPath values, then invoke compileSecondbrainWiki and verify that both source pages are generated with distinct output paths to ensure they are not overwritten or merged. Include assertions to confirm both source page files exist and contain the expected content from their respective artifacts.modules/jarvos-secondbrain/tests/secondbrain-status.test.js (1)
19-109: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for
enabled: falseadapters in retrieval eval reports.This file should assert that disabled adapters do not affect
retrievalEval.total/status, so the status gate matches adapter enablement semantics.🤖 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-secondbrain/tests/secondbrain-status.test.js` around lines 19 - 109, Add a new test case after the existing tests to verify that adapters with enabled set to false do not affect the retrievalEval.total or status calculations. The test should create a scenario where the eval report contains both enabled and disabled adapters, then call buildSecondbrainStatus and renderSecondbrainStatus to assert that only the enabled adapters influence the overall retrievalEval status outcome, ensuring the status gate properly respects adapter enablement semantics.modules/jarvos-secondbrain/packages/jarvos-ambient/test/intent-exports.test.js (1)
244-299: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winAdd a regression test for substantive-idea skill dispatch
Please add one test asserting that an IDEA plan with
createNote === truealso includes anote-creationskill invocation (not only journal behavior). This would catch action/skill divergence in the IDEA path.Example test shape
+test('substantive idea includes note-creation skill invocation', () => { + const plan = routing.buildThreePackagePlan({ + text: 'Here is an idea: define adapter boundaries so routing stays portable and testable over time.', + date: '2026-05-19', + }); + + assert.equal(plan.route, 'idea'); + assert.equal(plan.createNote, true); + assert.ok(plan.actions.some((action) => action.kind === 'note')); + assert.ok(plan.skillInvocations.some((invocation) => invocation.skillId === 'note-creation')); +});🤖 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-secondbrain/packages/jarvos-ambient/test/intent-exports.test.js` around lines 244 - 299, Add a new test after the existing three tests to verify the IDEA salienceClass routing behavior. Create a test that calls routing.buildThreePackagePlan with salienceClass set to 'idea' and include text and confidence properties. Assert that the resulting plan has createNote equal to true, that the actions array includes an action with kind 'journal', and that the skillInvocations array includes an invocation with skillId 'note-creation'. This test will prevent divergence between the action and skill invocation behavior for the IDEA routing path.modules/jarvos-secondbrain/bridge/skills/index.js (1)
10-16: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winHarden the registry against external mutation.
Line 28 exports the live
contractsarray, so downstream code can mutate it and silently break contract lookup behavior in this module instance.Proposed fix
-const contracts = [ +const contracts = Object.freeze([ journalEntry, noteCreation, ideaParking, memoryPromotion, workIntake, -]; +]); function listSkillContracts() { return contracts.slice(); } module.exports = { skillContractSchema, - contracts, + contracts: contracts.slice(), listSkillContracts, getSkillContract, journalEntry,Also applies to: 26-30
🤖 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-secondbrain/bridge/skills/index.js` around lines 10 - 16, The contracts array is exported directly, allowing downstream code to mutate it and break contract lookup behavior. Apply Object.freeze() to the contracts array definition to prevent external mutation while still allowing read access. This will ensure the array cannot be modified by external code that imports it, protecting the integrity of the contract registry in this module instance.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7b70c210-40ea-4d24-a783-c48ea4b643eb
📒 Files selected for processing (125)
modules/jarvos-secondbrain/README.mdmodules/jarvos-secondbrain/adapters/ambient-local-storage-adapter.jsmodules/jarvos-secondbrain/adapters/claude-code/index.jsmodules/jarvos-secondbrain/adapters/codex/index.jsmodules/jarvos-secondbrain/adapters/index.jsmodules/jarvos-secondbrain/adapters/obsidian/src/vault-storage-adapter.jsmodules/jarvos-secondbrain/adapters/openclaw/README.mdmodules/jarvos-secondbrain/adapters/openclaw/index.jsmodules/jarvos-secondbrain/adapters/session-source/session-source-adapter.jsmodules/jarvos-secondbrain/bridge/capture/README.mdmodules/jarvos-secondbrain/bridge/capture/src/universal-capture.jsmodules/jarvos-secondbrain/bridge/config/README.mdmodules/jarvos-secondbrain/bridge/config/index.jsmodules/jarvos-secondbrain/bridge/config/jarvos-paths.jsmodules/jarvos-secondbrain/bridge/config/src/paperclip.jsmodules/jarvos-secondbrain/bridge/config/src/resolve-config.jsmodules/jarvos-secondbrain/bridge/config/src/shared-vault-onboarding.jsmodules/jarvos-secondbrain/bridge/dispatch/README.mdmodules/jarvos-secondbrain/bridge/dispatch/src/capture-dispatcher.jsmodules/jarvos-secondbrain/bridge/paperclip/README.mdmodules/jarvos-secondbrain/bridge/paperclip/client.jsmodules/jarvos-secondbrain/bridge/provenance/README.mdmodules/jarvos-secondbrain/bridge/provenance/src/journal-note-audit.jsmodules/jarvos-secondbrain/bridge/provenance/src/lib/provenance-config.jsmodules/jarvos-secondbrain/bridge/provenance/src/link-to-journal.jsmodules/jarvos-secondbrain/bridge/provenance/src/note-journal-contract.jsmodules/jarvos-secondbrain/bridge/provenance/src/notes-section-normalizer.jsmodules/jarvos-secondbrain/bridge/provenance/src/wiki-link-integrity.jsmodules/jarvos-secondbrain/bridge/routing/README.mdmodules/jarvos-secondbrain/bridge/routing/src/capture-contract.jsmodules/jarvos-secondbrain/bridge/routing/src/capture-that.jsmodules/jarvos-secondbrain/bridge/routing/src/keyword-capture-router.jsmodules/jarvos-secondbrain/bridge/routing/src/salience-detector.jsmodules/jarvos-secondbrain/bridge/routing/src/three-package-router.jsmodules/jarvos-secondbrain/bridge/routing/test/test-capture-system.jsmodules/jarvos-secondbrain/bridge/skills/README.mdmodules/jarvos-secondbrain/bridge/skills/contracts/idea-parking.jsmodules/jarvos-secondbrain/bridge/skills/contracts/journal-entry.jsmodules/jarvos-secondbrain/bridge/skills/contracts/memory-promotion.jsmodules/jarvos-secondbrain/bridge/skills/contracts/note-creation.jsmodules/jarvos-secondbrain/bridge/skills/contracts/work-intake.jsmodules/jarvos-secondbrain/bridge/skills/index.jsmodules/jarvos-secondbrain/bridge/skills/schemas/skill-contract.schema.jsonmodules/jarvos-secondbrain/bridge/synthesis/README.mdmodules/jarvos-secondbrain/bridge/synthesis/index.jsmodules/jarvos-secondbrain/bridge/synthesis/src/journal-spine-synthesis.jsmodules/jarvos-secondbrain/bridge/synthesis/src/retrieval-evals.jsmodules/jarvos-secondbrain/bridge/synthesis/src/secondbrain-status.jsmodules/jarvos-secondbrain/bridge/synthesis/src/spine-synthesis.jsmodules/jarvos-secondbrain/docs/architecture/automatic-secondbrain-public-boundary.mdmodules/jarvos-secondbrain/docs/architecture/jarvos-secondbrain-monorepo-spec.mdmodules/jarvos-secondbrain/docs/contracts/JARVOS_SECONDBRAIN_PACKAGE_MAP_2026-03-25.mdmodules/jarvos-secondbrain/docs/contracts/SESSION_SOURCE_ADAPTERS.mdmodules/jarvos-secondbrain/docs/decisions/engraph-retrieval-decision.mdmodules/jarvos-secondbrain/docs/migration/JARVOS_SECONDBRAIN_MONOREPO_BOOTSTRAP_CHECKLIST_2026-03-25.mdmodules/jarvos-secondbrain/docs/operations/manual-note-maintenance.mdmodules/jarvos-secondbrain/package.jsonmodules/jarvos-secondbrain/packages/jarvos-ambient/README.mdmodules/jarvos-secondbrain/packages/jarvos-ambient/package.jsonmodules/jarvos-secondbrain/packages/jarvos-ambient/src/adapters/contract.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/adapters/index.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/adapters/local-storage.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/adapters/skill-dispatch.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/index.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/capture-contract.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/index.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/keyword-capture-router.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/retroactive-capture.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/intent/salience-detector.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/src/routing/index.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/test/adapters.test.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/test/integration.test.jsmodules/jarvos-secondbrain/packages/jarvos-ambient/test/intent-exports.test.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/README.mdmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/config/journal-module.jsonmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/docs/JARVOS_SECONDBRAIN_JOURNAL_PACKAGE_DECISION_2026-03-25.mdmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/package.jsonmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/journal-maintenance.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/section-config.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/README.mdmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/docs/JARVOS_SECONDBRAIN_NOTES_PACKAGE_DECISION_2026-03-25.mdmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/docs/SOURCE_MATERIAL_PROVENANCE_CONTRACT.mdmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/migrations/backfill_notes_frontmatter.pymodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/migrations/note_contract_migration.pymodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/package.jsonmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/concept-title.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/knowledge-optimizer.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lib/note-schema.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lib/notes-config.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lint-frontmatter.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lint-source-material.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/manual-notes-maintenance.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/write-to-vault.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-wiki/README.mdmodules/jarvos-secondbrain/packages/jarvos-secondbrain-wiki/package.jsonmodules/jarvos-secondbrain/packages/jarvos-secondbrain-wiki/src/index.jsmodules/jarvos-secondbrain/scripts/jarvos-capture.jsmodules/jarvos-secondbrain/scripts/obsidian-note-journal-contract.jsmodules/jarvos-secondbrain/src/index.jsmodules/jarvos-secondbrain/tests/ambient-local-storage-adapter.test.jsmodules/jarvos-secondbrain/tests/capture-dispatcher.test.jsmodules/jarvos-secondbrain/tests/concept-title.test.jsmodules/jarvos-secondbrain/tests/config-resolution.test.jsmodules/jarvos-secondbrain/tests/generated-wiki.test.jsmodules/jarvos-secondbrain/tests/infer-title-concept.test.jsmodules/jarvos-secondbrain/tests/jarvos-paths-shim.test.jsmodules/jarvos-secondbrain/tests/jarvos-paths.test.jsmodules/jarvos-secondbrain/tests/journal-maintenance.test.jsmodules/jarvos-secondbrain/tests/journal-note-audit-touched.test.jsmodules/jarvos-secondbrain/tests/journal-spine-synthesis.test.jsmodules/jarvos-secondbrain/tests/keyword-capture-router.test.jsmodules/jarvos-secondbrain/tests/knowledge-units.test.jsmodules/jarvos-secondbrain/tests/paperclip-client.test.jsmodules/jarvos-secondbrain/tests/personality-note-journal-contract.test.jsmodules/jarvos-secondbrain/tests/retrieval-evals.test.jsmodules/jarvos-secondbrain/tests/secondbrain-status.test.jsmodules/jarvos-secondbrain/tests/section-config.test.jsmodules/jarvos-secondbrain/tests/session-source-adapters.test.jsmodules/jarvos-secondbrain/tests/skill-contracts.test.jsmodules/jarvos-secondbrain/tests/source-material-provenance-lint.test.jsmodules/jarvos-secondbrain/tests/spine-synthesis.test.jsmodules/jarvos-secondbrain/tests/three-package-router-invariant.test.jsmodules/jarvos-secondbrain/tests/universal-capture.test.jsmodules/jarvos-secondbrain/tests/wiki-link-integrity.test.jstests/modules-smoke-test.js
💤 Files with no reviewable changes (1)
- modules/jarvos-secondbrain/packages/jarvos-secondbrain-notes/src/lint-frontmatter.js
| function validateCaptureEvent(event = {}) { | ||
| const errors = []; | ||
|
|
||
| if (event.schemaVersion != null && !SUPPORTED_CAPTURE_EVENT_SCHEMA_VERSIONS.includes(String(event.schemaVersion))) { | ||
| pushEnumError( |
There was a problem hiding this comment.
validateCaptureEvent(null) crashes before validation completes.
The validator dereferences event.schemaVersion without first ensuring event is an object. Passing null throws instead of returning validation errors.
🛠️ Suggested fix
function validateCaptureEvent(event = {}) {
- const errors = [];
+ const errors = [];
+ if (!isPlainObject(event)) {
+ return ['CaptureEvent must be an object'];
+ }
if (event.schemaVersion != null && !SUPPORTED_CAPTURE_EVENT_SCHEMA_VERSIONS.includes(String(event.schemaVersion))) {🤖 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-secondbrain/packages/jarvos-ambient/src/intent/capture-contract.js`
around lines 258 - 262, The validateCaptureEvent function crashes when null is
explicitly passed because the default parameter event = {} does not override an
explicit null argument. Add a guard check at the beginning of the
validateCaptureEvent function to validate that event is a valid object before
attempting to access event.schemaVersion. If event is null, undefined, or not an
object, push an appropriate validation error to the errors array and continue
validation or return early. This ensures the validator gracefully handles
invalid input and returns validation errors instead of throwing.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eee14751f9
ℹ️ 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".
| return require('@jarvos/memory'); | ||
| } catch (packageError) { | ||
| try { | ||
| return require('../../../../jarvos-memory/src'); |
There was a problem hiding this comment.
When high-confidence captures route to memory, this loader resolves to @jarvos/memory's package main or the monorepo jarvos-memory/src fallback; both expose the schema-only createMemoryRecord that returns {written:false,path:null} and do not export checkMemoryDedup, while the durable writer/deduper lives under lib/memory-record.js. As a result decision/preference/lesson captures report a memory route but never create the MEMORY.md/memory/decisions artifacts or skip duplicates.
Useful? React with 👍 / 👎.
| ...captureEvent, | ||
| frontmatter, | ||
| }; | ||
| const routing = applyRoutingPlan(routingInput, { ...options, adapter }); |
There was a problem hiding this comment.
For the public jarvos-capture/captureWithJarvos path with an adapter that reports a failed note write (for example {written:false,error:'...'} or an ambient adapter result with ok:false), applyRoutingPlan still appends the planned [[note]] journal line and returns ok: true. The three-package path added a fail-closed guard for this exact invariant, so this entrypoint can still leave dangling journal backlinks whenever the note backend is unavailable or misrouted.
Useful? React with 👍 / 👎.
Summary
Verification
Notes
Summary by CodeRabbit
Release Notes
New Features
Improvements
Documentation