fix(SUP-3332): reconcile final secondbrain mirror deltas - #95
Conversation
|
Warning Review limit reached
Next review available in: 37 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?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 reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. 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, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (65)
📝 WalkthroughWalkthroughThis PR reverses the medium-confidence "flagged-review" capture routing across capture, dispatch, three-package/ambient routing, and skills contracts, replacing it with an "ignored" outcome and a caller-provided reviewQueue idea-parking flow. It adds durable-note capture flags controlling note promotion, removes the journal Flagged section, adds journal single-writer conflict detection and mobile journaling index repair, and adds a CI secret-scan job. ChangesCapture routing and idea-parking behavior change
Journal single-writer guard and mobile journaling index repair
CI Secret Scan
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Poem 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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: c23a94f3e7
ℹ️ 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 { | ||
| createMemoryRecord, | ||
| checkMemoryDedup, | ||
| } = require('../../../../jarvos-memory/src'); |
There was a problem hiding this comment.
When plan.routeToMemory is true for high-confidence decision/preference/fact/lesson captures, this import leaves checkMemoryDedup undefined because modules/jarvos-memory/src/index.js does not export it. The later call throws TypeError: checkMemoryDedup is not a function after journal/note storage has already been applied, so eligible captures fail instead of producing a memory result.
Useful? React with 👍 / 👎.
| if (keywordResult.note) { | ||
| params.noteRef = keywordResult.note.title || keywordResult.note.path || undefined; |
There was a problem hiding this comment.
If the storage adapter returns a failed note object, e.g. { written: false, error: ... }, this truthiness check still copies the note title/path into the memory record. That creates memory noteRefs pointing at notes that were not persisted; the previous noteWriteSucceeded guard avoided this and the same failure case is still handled for journal backlinks above.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7a9c597d89
ℹ️ 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".
| frontmatterToObject, | ||
| parseFrontmatter, | ||
| } = require('./lib/note-schema'); | ||
| } = require('../../../../scripts/lib/note-schema.js'); |
There was a problem hiding this comment.
When manual-notes-maintenance is run or even required, this path resolves to modules/scripts/lib/note-schema.js, which is not present in this tree; the parser still lives under this package at src/lib/note-schema.js. That makes the manual-note maintenance CLI fail with MODULE_NOT_FOUND before it can parse args, so --dry-run, --apply, and watch mode are unusable.
Useful? React with 👍 / 👎.
| const { createMemoryRecord } = require('./memory-record'); | ||
| const { HindsightAdapter } = require('./hindsight-adapter'); | ||
| const { getHindsightConfig } = require('./memory-config'); |
There was a problem hiding this comment.
In this commit modules/jarvos-memory/src/lib contains only memory-promotion.js and memory-schema.js, so requiring this new module throws MODULE_NOT_FOUND on these sibling imports before callers can reach reviewCandidate, promoteCandidate, or the recall helpers. The existing implementations are under the package-level lib/ tree, so the new memory promotion surface is currently unloadable.
Useful? React with 👍 / 👎.
| function resetGeneratedOutput(outputDir) { | ||
| assertManagedOutputDir(outputDir); | ||
| for (const entry of ['concepts', 'sources', 'daily', 'index.md']) { | ||
| fs.rmSync(path.join(outputDir, entry), { recursive: true, force: true }); | ||
| } |
There was a problem hiding this comment.
When outputDir is accidentally pointed at an existing folder that was not created by this generator, resetGeneratedOutput now unconditionally removes concepts, sources, daily, and index.md. That can delete user-authored content in a misconfigured visible wiki path; the previous managed-marker/nonempty-directory guard prevented this destructive reset.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (3)
modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/README.md (1)
13-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueLGTM! Consider also documenting the new mobile journaling index detection/repair behavior (plugin disabling, index rewrite, backups) somewhere in this README, since it's a new capability with real side effects not currently mentioned.
Also applies to: 25-28, 41-43
🤖 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-journal/README.md` at line 13, The README currently describes JarvOS as an automated writer but does not mention the new mobile journaling index detection/repair behavior. Update the README narrative near the existing product description and any relevant feature sections to document that the journaling workflow can detect a broken index, disable the plugin when needed, rewrite the index, and create backups. Use the existing README content and the journal package description to place this capability where users will see its side effects.modules/jarvos-secondbrain/bridge/capture/src/universal-capture.js (1)
102-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winFive overlapping durable-note flags.
createNote,createDurableNote,durable,durableNote, andstandaloneNoteall appear to express the same "promote to a note" intent. Consider consolidating to a single canonical field (with legacy aliases normalized internally) to reduce contract surface and caller confusion.🤖 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/src/universal-capture.js` around lines 102 - 106, The capture contract in universal-capture.js exposes five overlapping note-promotion flags, which creates duplicate semantics and caller confusion. Consolidate the raw.createNote, raw.createDurableNote, raw.durable, raw.durableNote, and raw.standaloneNote fields into one canonical flag in the capture normalization logic, and map any legacy aliases to that single internal field inside the capture flow so existing callers still work.modules/jarvos-secondbrain/bridge/provenance/src/note-journal-contract.js (1)
48-55: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueError message omits the
forceDurablebypass.The guard accepts
substantive,createDurableNote, orforceDurableas escape hatches, but the thrown message only tells callers to usesubstantive:trueorcreateDurableNote:true. Minor doc/DX gap for callers who discoverforceDurableonly by reading source.🤖 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/note-journal-contract.js` around lines 48 - 55, The guard in note-journal-contract.js throws an error from the lightweight idea check without mentioning the forceDurable escape hatch, even though that flag is accepted in the condition. Update the thrown message in the lightweight idea branch so it clearly lists forceDurable alongside substantive and createDurableNote, keeping the guidance in sync with the logic in the same validation block.
🤖 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 @.github/workflows/ci.yml:
- Around line 141-143: The secret-scan pattern in the CI workflow is too broad
and is causing likely false positives in docs. Tighten the regex used by the
fallback scan in the workflow so it targets value-shaped secrets instead of bare
words like token, secret, password, and bearer, and consider limiting the `git
grep` fallback to non-markdown or non-doc paths. Update the scan logic around
the pattern definition and `scan_cmd` setup so legitimate documentation does not
fail the job.
- Around line 126-128: The checkout step in the read-only scan job currently
persists the Actions token in the local git config. Update the actions/checkout
configuration in this job to disable credential persistence by setting
persist-credentials to false, alongside the existing fetch-depth setting, so the
scan can read history without leaving credentials behind.
- Around line 141-146: The secret-scan logic in the CI workflow is inconsistent
between the fallback and diff-based paths, so make both paths use the same
case-insensitive matching and only inspect added content. Update the scan
command selection around the secret pattern in the workflow so the whole-repo
fallback and the diff path behave the same, and adjust the diff-based grep in
the scan command to ignore removed lines and diff headers while keeping the
existing pattern set.
In `@modules/jarvos-secondbrain/bridge/capture/README.md`:
- Around line 49-53: The capture routing docs currently mention only
createDurableNote, but the capture layer also recognizes several durable-note
aliases. Update the README guidance around the idea capture rules to explicitly
list all supported flags in the capture flow, including createNote, durable,
durableNote, standaloneNote, and createDurableNote, so callers know the full
alias set handled by the capture layer.
In `@modules/jarvos-secondbrain/bridge/routing/src/three-package-router.js`:
- Around line 35-44: The `applyThreePackagePlan` flow in
`three-package-router.js` is importing and calling `checkMemoryDedup`, but
`modules/jarvos-memory/src/index.js` does not export it, so the memory capture
path will fail at runtime. Fix this by either exporting `checkMemoryDedup` from
the `jarvos-memory` index to match the existing `createMemoryRecord` import, or
remove the call from `applyThreePackagePlan` and any related dedup logic if it
is no longer needed.
In
`@modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/journal-maintenance.js`:
- Around line 307-315: The fallback in resolveJournalingIndexPath ignores the
computed fileName and always uses JOURNALING_INDEX_FILE, so custom journaling
names are lost when no configured path matches. Update
resolveJournalingIndexPath to use the resolved fileName in both the matchingPath
branch and the fallback branch, keeping the existing path resolution logic in
journal-maintenance.js intact.
- Around line 404-417: The `repairMobileJournalingIndex` guard currently treats
a missing `Journaling.md` as stale content, so `main()` can create the index in
vaults that were never using the Journaling plugin. Update the applicability
check in `repairMobileJournalingIndex` to only repair when the journaling setup
is actually relevant (for example, the plugin is enabled/targets the journal, or
an index already exists), and use that condition together with `before.ok`
before returning the unchanged result.
---
Nitpick comments:
In `@modules/jarvos-secondbrain/bridge/capture/src/universal-capture.js`:
- Around line 102-106: The capture contract in universal-capture.js exposes five
overlapping note-promotion flags, which creates duplicate semantics and caller
confusion. Consolidate the raw.createNote, raw.createDurableNote, raw.durable,
raw.durableNote, and raw.standaloneNote fields into one canonical flag in the
capture normalization logic, and map any legacy aliases to that single internal
field inside the capture flow so existing callers still work.
In `@modules/jarvos-secondbrain/bridge/provenance/src/note-journal-contract.js`:
- Around line 48-55: The guard in note-journal-contract.js throws an error from
the lightweight idea check without mentioning the forceDurable escape hatch,
even though that flag is accepted in the condition. Update the thrown message in
the lightweight idea branch so it clearly lists forceDurable alongside
substantive and createDurableNote, keeping the guidance in sync with the logic
in the same validation block.
In `@modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/README.md`:
- Line 13: The README currently describes JarvOS as an automated writer but does
not mention the new mobile journaling index detection/repair behavior. Update
the README narrative near the existing product description and any relevant
feature sections to document that the journaling workflow can detect a broken
index, disable the plugin when needed, rewrite the index, and create backups.
Use the existing README content and the journal package description to place
this capability where users will see its side effects.
🪄 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: 6a1fc9d4-1cf8-415a-b44a-d23746e05167
📒 Files selected for processing (21)
.github/workflows/ci.ymlmodules/jarvos-secondbrain/bridge/capture/README.mdmodules/jarvos-secondbrain/bridge/capture/src/universal-capture.jsmodules/jarvos-secondbrain/bridge/dispatch/README.mdmodules/jarvos-secondbrain/bridge/dispatch/src/capture-dispatcher.jsmodules/jarvos-secondbrain/bridge/provenance/src/note-journal-contract.jsmodules/jarvos-secondbrain/bridge/routing/README.mdmodules/jarvos-secondbrain/bridge/routing/src/three-package-router.jsmodules/jarvos-secondbrain/bridge/skills/README.mdmodules/jarvos-secondbrain/bridge/skills/contracts/idea-parking.jsmodules/jarvos-secondbrain/bridge/skills/schemas/skill-contract.schema.jsonmodules/jarvos-secondbrain/packages/jarvos-ambient/src/routing/index.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/src/journal-maintenance.jsmodules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/section-config.jsmodules/jarvos-secondbrain/tests/capture-dispatcher.test.jsmodules/jarvos-secondbrain/tests/journal-maintenance.test.jsmodules/jarvos-secondbrain/tests/personality-note-journal-contract.test.jsmodules/jarvos-secondbrain/tests/skill-contracts.test.jsmodules/jarvos-secondbrain/tests/universal-capture.test.js
💤 Files with no reviewable changes (2)
- modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/section-config.js
- modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/config/journal-module.json
| - uses: actions/checkout@v4 | ||
| with: | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Set persist-credentials: false for this read-only scan job.
The checkout persists the Actions token in the local git config, but this job only reads history. Disabling persistence reduces credential-exposure surface.
🔒 Proposed change
- uses: actions/checkout@v4
with:
fetch-depth: 0
+ persist-credentials: false📝 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.
| - uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| - uses: actions/checkout@v4 | |
| with: | |
| fetch-depth: 0 | |
| persist-credentials: false |
🧰 Tools
🪛 zizmor (1.26.1)
[warning] 126-128: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
🤖 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 @.github/workflows/ci.yml around lines 126 - 128, The checkout step in the
read-only scan job currently persists the Actions token in the local git config.
Update the actions/checkout configuration in this job to disable credential
persistence by setting persist-credentials to false, alongside the existing
fetch-depth setting, so the scan can read history without leaving credentials
behind.
Source: Linters/SAST tools
| pattern='(api[_-]?key|secret|token|password|bearer|private[_-]?key|BEGIN (RSA|OPENSSH|EC|DSA|PRIVATE) KEY|ghp_|sk-[A-Za-z0-9]|xox[baprs]-)' | ||
| if [[ -z "$base" || "$base" =~ ^0+$ ]] || ! git cat-file -e "$base^{commit}" 2>/dev/null; then | ||
| scan_cmd=(git grep -nI -E "$pattern" -- .) | ||
| else | ||
| scan_cmd=(bash -c "git diff -U0 '$base'...HEAD -- | grep -E -i '$pattern'") | ||
| fi |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Two scan paths behave differently and the diff path over-matches.
- Case sensitivity mismatch: the diff path (Line 145) uses
grep -E -i, but the whole-repo fallback (Line 143) usesgit grep -Ewithout-i. Since the pattern keywords are lowercase, the fallback silently misses uppercase/mixed-case variants likeAPI_KEY,SECRET, orBearer. - Diff path matches removals and headers:
git diff -U0 ... | grep ...scans the whole diff output, so it flags secrets that are being removed (leading-lines) and can match diff header lines (+++/---). Restrict to added content to avoid false positives.
🔧 Proposed fix
- if [[ -z "$base" || "$base" =~ ^0+$ ]] || ! git cat-file -e "$base^{commit}" 2>/dev/null; then
- scan_cmd=(git grep -nI -E "$pattern" -- .)
- else
- scan_cmd=(bash -c "git diff -U0 '$base'...HEAD -- | grep -E -i '$pattern'")
- fi
+ if [[ -z "$base" || "$base" =~ ^0+$ ]] || ! git cat-file -e "$base^{commit}" 2>/dev/null; then
+ scan_cmd=(git grep -nI -E -i "$pattern" -- .)
+ else
+ scan_cmd=(bash -c "git diff -U0 '$base'...HEAD -- | grep -E '^\+' | grep -Ev '^\+\+\+' | grep -E -i '$pattern'")
+ fi📝 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.
| pattern='(api[_-]?key|secret|token|password|bearer|private[_-]?key|BEGIN (RSA|OPENSSH|EC|DSA|PRIVATE) KEY|ghp_|sk-[A-Za-z0-9]|xox[baprs]-)' | |
| if [[ -z "$base" || "$base" =~ ^0+$ ]] || ! git cat-file -e "$base^{commit}" 2>/dev/null; then | |
| scan_cmd=(git grep -nI -E "$pattern" -- .) | |
| else | |
| scan_cmd=(bash -c "git diff -U0 '$base'...HEAD -- | grep -E -i '$pattern'") | |
| fi | |
| pattern='(api[_-]?key|secret|token|password|bearer|private[_-]?key|BEGIN (RSA|OPENSSH|EC|DSA|PRIVATE) KEY|ghp_|sk-[A-Za-z0-9]|xox[baprs]-)' | |
| if [[ -z "$base" || "$base" =~ ^0+$ ]] || ! git cat-file -e "$base^{commit}" 2>/dev/null; then | |
| scan_cmd=(git grep -nI -E -i "$pattern" -- .) | |
| else | |
| scan_cmd=(bash -c "git diff -U0 '$base'...HEAD -- | grep -E '^\+' | grep -Ev '^\+\+\+' | grep -E -i '$pattern'") | |
| fi |
🤖 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 @.github/workflows/ci.yml around lines 141 - 146, The secret-scan logic in
the CI workflow is inconsistent between the fallback and diff-based paths, so
make both paths use the same case-insensitive matching and only inspect added
content. Update the scan command selection around the secret pattern in the
workflow so the whole-repo fallback and the diff path behave the same, and
adjust the diff-based grep in the scan command to ignore removed lines and diff
headers while keeping the existing pattern set.
| - Raw `idea:` / `Idea:` captures append to Journal Ideas only by default, even | ||
| when the idea is a long single-line thought. | ||
| - Substantive `idea:` captures create a note and link that note from Journal | ||
| Ideas. | ||
| Ideas only when the capture explicitly sets `substantive:true`, supplies a | ||
| title, or sets a durable-note flag such as `createDurableNote:true`. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Document the full durable-note alias set.
The routing contract here only names createDurableNote, but the capture layer also normalizes createNote, durable, durableNote, and standaloneNote. Please list the supported aliases so callers do not assume the others are unsupported.
Suggested wording
- Substantive `idea:` captures create a note and link that note from Journal
- Ideas only when the capture explicitly sets `substantive:true`, supplies a
- title, or sets a durable-note flag such as `createDurableNote:true`.
+ Substantive `idea:` captures create a note and link that note from Journal
+ Ideas only when the capture explicitly sets `substantive:true`, supplies a
+ title, or sets a durable-note flag such as `createNote:true`,
+ `createDurableNote:true`, `durable:true`, `durableNote:true`, or
+ `standaloneNote:true`.📝 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.
| - Raw `idea:` / `Idea:` captures append to Journal Ideas only by default, even | |
| when the idea is a long single-line thought. | |
| - Substantive `idea:` captures create a note and link that note from Journal | |
| Ideas. | |
| Ideas only when the capture explicitly sets `substantive:true`, supplies a | |
| title, or sets a durable-note flag such as `createDurableNote:true`. | |
| - Raw `idea:` / `Idea:` captures append to Journal Ideas only by default, even | |
| when the idea is a long single-line thought. | |
| - Substantive `idea:` captures create a note and link that note from Journal | |
| Ideas only when the capture explicitly sets `substantive:true`, supplies a | |
| title, or sets a durable-note flag such as `createNote:true`, | |
| `createDurableNote:true`, `durable:true`, `durableNote:true`, or | |
| `standaloneNote:true`. |
🤖 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` around lines 49 - 53,
The capture routing docs currently mention only createDurableNote, but the
capture layer also recognizes several durable-note aliases. Update the README
guidance around the idea capture rules to explicitly list all supported flags in
the capture flow, including createNote, durable, durableNote, standaloneNote,
and createDurableNote, so callers know the full alias set handled by the capture
layer.
| function resolveJournalingIndexPath(journalDir, data) { | ||
| const vaultRoot = path.dirname(journalDir); | ||
| const fileName = String(data?.fileName || JOURNALING_INDEX_FILE).trim() || JOURNALING_INDEX_FILE; | ||
| const configuredPaths = configuredJournalingPaths(data); | ||
| const matchingPath = configuredPaths | ||
| .map((entry) => resolveConfigPath(entry, vaultRoot)) | ||
| .find((configuredPath) => configuredPath && isSameOrSubPath(configuredPath, journalDir)); | ||
| return matchingPath ? path.join(matchingPath, fileName) : path.join(journalDir, JOURNALING_INDEX_FILE); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Fallback ignores the resolved fileName.
The computed fileName (which can come from data.fileName) is dropped in the no-match fallback, which always resolves to the hardcoded JOURNALING_INDEX_FILE ("Journaling.md") instead. If a vault configures a custom journaling file name, detection/repair will target the wrong path.
🐛 Proposed fix
- return matchingPath ? path.join(matchingPath, fileName) : path.join(journalDir, JOURNALING_INDEX_FILE);
+ return matchingPath ? path.join(matchingPath, fileName) : path.join(journalDir, fileName);📝 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.
| function resolveJournalingIndexPath(journalDir, data) { | |
| const vaultRoot = path.dirname(journalDir); | |
| const fileName = String(data?.fileName || JOURNALING_INDEX_FILE).trim() || JOURNALING_INDEX_FILE; | |
| const configuredPaths = configuredJournalingPaths(data); | |
| const matchingPath = configuredPaths | |
| .map((entry) => resolveConfigPath(entry, vaultRoot)) | |
| .find((configuredPath) => configuredPath && isSameOrSubPath(configuredPath, journalDir)); | |
| return matchingPath ? path.join(matchingPath, fileName) : path.join(journalDir, JOURNALING_INDEX_FILE); | |
| } | |
| function resolveJournalingIndexPath(journalDir, data) { | |
| const vaultRoot = path.dirname(journalDir); | |
| const fileName = String(data?.fileName || JOURNALING_INDEX_FILE).trim() || JOURNALING_INDEX_FILE; | |
| const configuredPaths = configuredJournalingPaths(data); | |
| const matchingPath = configuredPaths | |
| .map((entry) => resolveConfigPath(entry, vaultRoot)) | |
| .find((configuredPath) => configuredPath && isSameOrSubPath(configuredPath, journalDir)); | |
| return matchingPath ? path.join(matchingPath, fileName) : path.join(journalDir, fileName); | |
| } |
🤖 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-journal/src/journal-maintenance.js`
around lines 307 - 315, The fallback in resolveJournalingIndexPath ignores the
computed fileName and always uses JOURNALING_INDEX_FILE, so custom journaling
names are lost when no configured path matches. Update
resolveJournalingIndexPath to use the resolved fileName in both the matchingPath
branch and the fallback branch, keeping the existing path resolution logic in
journal-maintenance.js intact.
| function repairMobileJournalingIndex(journalDir, opts = {}) { | ||
| const before = detectMobileJournalingIndex(journalDir); | ||
| const desiredIndex = renderJournalingLinkIndex(journalDir); | ||
| const currentIndex = readTextSafe(before.indexPath) || ''; | ||
| const indexStale = currentIndex !== desiredIndex; | ||
| const pluginConflict = before.conflicts.some((conflict) => conflict.kind === 'obsidian-plugin:journaling'); | ||
| if (before.ok && !indexStale) { | ||
| return { | ||
| ...before, | ||
| changed: false, | ||
| backupPaths: [], | ||
| repairStatus: 'unchanged', | ||
| }; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and inspect the functions around the reported lines.
FILE="modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/journal-maintenance.js"
echo "== file outline =="
ast-grep outline "$FILE" --view expanded || true
echo
echo "== relevant slice =="
sed -n '1,260p' "$FILE" | nl -ba | sed -n '1,260p'
echo
echo "== later slice around repairMobileJournalingIndex =="
sed -n '260,520p' "$FILE" | nl -ba | sed -n '260,520p'Repository: levineam/jarvOS
Length of output: 3991
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find all references to the key helpers and the main entry point.
rg -n "repairMobileJournalingIndex|detectMobileJournalingIndex|renderJournalingLinkIndex|main\(" modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src -S
echo
echo "== related tests =="
rg -n "Journaling|mobile journaling|repairMobileJournalingIndex|detectMobileJournalingIndex|renderJournalingLinkIndex" modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal -SRepository: levineam/jarvOS
Length of output: 4902
🏁 Script executed:
#!/bin/bash
set -euo pipefail
FILE="modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/journal-maintenance.js"
echo "== detect/render/repair slice =="
sed -n '300,460p' "$FILE" | cat -n
echo
echo "== test files mentioning journaling index =="
find modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal -path '*/test*' -o -path '*/tests*' | sort
echo
echo "== test snippets =="
rg -n "Journaling|repairMobileJournalingIndex|detectMobileJournalingIndex|renderJournalingLinkIndex|mobile journaling" modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal -SRepository: levineam/jarvOS
Length of output: 10459
repairMobileJournalingIndex writes Journaling.md for untouched vaults. (modules/jarvos-secondbrain/packages/jarvos-secondbrain-journal/src/journal-maintenance.js:404-417)
before.ok only means there’s no plugin conflict or embeds, but a missing index still makes indexStale true, so main() will create Journal/Journaling.md on every run even when the Journaling plugin was never enabled. Gate this on actual applicability (plugin enabled/targets journal, or an existing index) instead of a pure content diff.
🤖 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-journal/src/journal-maintenance.js`
around lines 404 - 417, The `repairMobileJournalingIndex` guard currently treats
a missing `Journaling.md` as stale content, so `main()` can create the index in
vaults that were never using the Journaling plugin. Update the applicability
check in `repairMobileJournalingIndex` to only repair when the journaling setup
is actually relevant (for example, the plugin is enabled/targets the journal, or
an index already exists), and use that condition together with `before.ok`
before returning the unchanged result.
Port the final clawd mirror deltas for secondbrain capture and journal maintenance into the public jarvOS tree. Add a CI secret-scan check for reconcile PRs. Co-Authored-By: Paperclip <noreply@paperclip.ing>
7a9c597 to
4648e88
Compare
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 71c790aea1
ℹ️ 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 { sourcePathFor } = require('../../../packages/jarvos-secondbrain-notes/src/knowledge-optimizer'); | ||
| const { getVaultNotesDir, getVaultJournalDir } = require('./lib/provenance-config'); | ||
| const { frontmatterToObject, parseFrontmatter } = require('../../../packages/jarvos-secondbrain-notes/src/lib/note-schema'); | ||
| const { frontmatterToObject, parseFrontmatter } = require('../../../../scripts/lib/note-schema'); |
There was a problem hiding this comment.
When the Obsidian note/journal contract CLI is required or executed, this path resolves to modules/scripts/lib/note-schema, which is not present; the only note-schema.js in the tree is under packages/jarvos-secondbrain-notes/src/lib. This makes scripts/obsidian-note-journal-contract.js fail with MODULE_NOT_FOUND before it can handle any supported personality, so the contract smoke path is unusable.
Useful? React with 👍 / 👎.
| openclaw: 'openclaw', | ||
| codex: 'codex', | ||
| 'claude-code': 'claude-code', | ||
| hermes: 'hermes', | ||
| }; |
There was a problem hiding this comment.
For Hermes sessions, createHermesSessionAdapter() still calls createSessionSourceAdapter('hermes'), but normalizeSessionToCaptureEvents() rejects any tool not present in this TOOL_SOURCE map and returns unsupported-source-tool. Since the public contract still lists hermes as a supported source, every Hermes transcript now normalizes to zero capture events.
Useful? React with 👍 / 👎.
| } | ||
|
|
||
| function ensurePortableWorkspaceFiles(workspace) { | ||
| const schemaSource = path.resolve(__dirname, '..', '..', 'jarvos.config.schema.json'); |
There was a problem hiding this comment.
When initProfile() initializes a fresh workspace, __dirname is modules/jarvos/src, so this resolves to modules/jarvos.config.schema.json; that file does not exist, while the schema lives at the repository root. As a result ensurePortableWorkspaceFiles() leaves configSchema as missing-source, and the immediate doctor run reports the newly initialized workspace as missing jarvos.config.schema.json.
Useful? React with 👍 / 👎.
Summary
SUP-3283lightweight rawIdea:captures stay journal-only unless durable intent is explicit.SUP-3301/#727medium-confidence salience is ignored by default instead of writing aFlaggedjournal section.SUP-3306journal maintenance now guards mobile journaling conflicts, repairs stale/sync-replayed indexes, preserves existing auto sections on source failures, and keeps known-good/audit backup behavior.Secret scanjob so reconcile PRs carry a required diff-scoped secret-pattern check.Branch Reconcile Notes
origin/SUP-2261/reconcile-jarvos-core: superseded by the public-canonicalSUP-2261PR already onmain(a6ec23d, PR SUP-2261: reconcile jarvos-coding + jarvos-skills into public (public canonical) #70). Direct branch merge would revert/delete newer public release work, so it is not merged.SUP-2266/jarvos-v050-release: no live local/remote ref found in the clean public checkout. Publicmainalready contains v0.5.0 (f4679d1) and the follow-up v0.5 install-bundle PR (a1dd2a6), so no additional port is needed.Verification
node --test modules/jarvos-secondbrain/tests/capture-dispatcher.test.js modules/jarvos-secondbrain/tests/universal-capture.test.js modules/jarvos-secondbrain/tests/personality-note-journal-contract.test.js modules/jarvos-secondbrain/tests/journal-maintenance.test.js modules/jarvos-secondbrain/tests/skill-contracts.test.jsnpm testgit diff --checkScope Check
origin/mainat/Users/andrew/clawd-worktrees/SUP-3332-paperclip.git diff --name-status origin/main..HEADis limited to.github/workflows/ci.ymlandmodules/jarvos-secondbrain/**.Summary by CodeRabbit
New Features
Bug Fixes
Documentation