fix(move-note): rewrite relative asset links inside a moved note

[aliasunder]

Summary

moveNote resolved link targets only against .md note paths, so a relative-path link to an asset (![[../assets/photo.png]] or ![img](../assets/photo.png)) inside a moved note was left untouched and broke — Obsidian rewrites these on move. The rewriter now resolves against non-md vault files too, matching Obsidian's on-move link updating.

Follow-up to #336/#338, which fixed the indexing side

What changed

• links.resolveAsset — pure, array-based twin of the indexer's SQL resolveNonMarkdownFile: same tiers (full-filename exact → relative → suffix, then stem exact → relative → suffix/basename), same family ordering, same shortest-then-lexicographic tiebreak. The rewriter and the index can no longer disagree about where an asset link points.
• MD_LINK_PARTS generalized — captures any extension (or none) instead of anchoring on .md. Bonus fix in the same class: extensionless markdown links to notes ([text](Extensionless%20Note)) are now rewritten on move instead of silently breaking.
• Two-tier rewriteTarget — note tier first (a target matching both a note and an asset resolves to the note, matching the indexer's precedence), then the asset tier. Replacements keep the original link's form (basename/absolute/relative) and extension state: stem-form wikilinks stay extensionless (![[../../boards/Trip Route]]), markdown links keep their original extension.
• vaultFs.listAssets — lists non-md files with the same walk/filters as listNotes via a shared listVaultFilePaths helper; the move handler fetches both path lists from the filesystem (fresh — no index-lag dependency).
• stripExtension consolidated into the links namespace and reused by the indexer — single home for the multi-dot stem semantics (photo.png.canvas → photo.png).

Verification

• 1920 tests pass (57 files) — 36 new: 18 resolveAsset/stripExtension, 5 splitMarkdownLink (incl. updated .txt contract), 4 listAssets, 12 asset-rewrite move tests; all 59 pre-existing note-mover tests pass byte-identical (note behavior unchanged).
• Mutation-checked: family-order inversion fails the ordering test; disabling the asset tier fails all 7 asset-rewrite tests; re-.md-anchoring the parser fails the markdown-asset and extensionless tests — each for the intended reason.
• npm run lint 0 errors (317 pre-existing warnings, none in changed files), npm run prettier:check clean, npm run build green.
• Tool description updated for attachment coverage (TDQS self-score 4.85, Tier A — unchanged from baseline).

Test plan

New describe block moveNote — asset link rewriting: relative wikilink + markdown rewrites, basename/vault-absolute left alone (with move-happened guards), backlink-source asset links byte-identical, stem-form preservation, multi-dot stem resolution, extensionless markdown rewrite, note/asset collision precedence, same-named-asset shadowing, frontmatter embed rewrite.

Summary by CodeRabbit

• New Features

  • Note moves and renames now preserve relative links to attachments and other non-Markdown files.
  • Markdown links retain their original file extensions when rewritten.
  • Asset discovery now includes nested files while excluding hidden paths and Markdown notes.

• Bug Fixes

  • Improved resolution of extensionless, multi-dot, relative, and vault-absolute asset links during note moves.
  • Added deterministic handling when multiple matching assets exist.

• Tests

  • Expanded coverage for asset discovery, link parsing, resolution, and link rewriting.

[sourcery-ai]

Hey - I've left some high level feedback:

• The new asset resolution (resolveAsset) and extension handling logic in note-mover.ts has grown quite complex; consider splitting note vs asset resolution and link-form classification into smaller, focused helpers to improve readability and maintainability.
• Both listAssets and resolveAsset iterate over all non-markdown files for each move; for large vaults this may become expensive, so it could be worth introducing a lightweight in-memory index (e.g., by basename/stem) or reusing existing index data rather than repeatedly scanning arrays.

Prompt for AI Agents

Please address the comments from this code review:

## Overall Comments
- The new asset resolution (`resolveAsset`) and extension handling logic in `note-mover.ts` has grown quite complex; consider splitting note vs asset resolution and link-form classification into smaller, focused helpers to improve readability and maintainability.
- Both `listAssets` and `resolveAsset` iterate over all non-markdown files for each move; for large vaults this may become expensive, so it could be worth introducing a lightweight in-memory index (e.g., by basename/stem) or reusing existing index data rather than repeatedly scanning arrays.

---

Sourcery is free for open source - if you like our reviews please consider sharing them ✨

• X
• Mastodon
• LinkedIn
• Facebook

_{Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.}

[umm-actually]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
General	Make null-return explicit in resolveAsset The final two branches (suffix and basename stem matching) return the result of shortestOf directly, which can be null. The earlier tiers (exact, relative, full-path suffix) all guard with if (match) return match before falling through. If the suffix or basename stem tier finds no match, the function returns null — which is correct, but the asymmetry in control flow makes it easy to misread as a guaranteed return. Wrap the final two branches in if guards for consistency, or add a comment explaining that shortestOf returns null on empty input. src/vault-mcp/obsidian-markdown/links.ts [350-403] -const resolveAsset = (params: { - target: string - allAssetPaths: readonly string[] - sourcePath?: string -}): string | null => { - const { target, allAssetPaths, sourcePath } = params - const relativeTarget = sourcePath - ? posix.join(posix.dirname(sourcePath), target) - : null - - // ── Full-filename family: exact → relative → path suffix ── - - if (allAssetPaths.includes(target)) return target - - if (relativeTarget && allAssetPaths.includes(relativeTarget)) { - return relativeTarget - } - - const fullPathSuffixMatch = shortestOf( - allAssetPaths.filter((assetPath) => assetPath.endsWith(`/${target}`)), - ) - if (fullPathSuffixMatch) return fullPathSuffixMatch - - // ── Stem family: exact → relative → suffix/basename ── - - const exactStemMatch = shortestOf( - allAssetPaths.filter((assetPath) => stripExtension(assetPath) === target), - ) - if (exactStemMatch) return exactStemMatch - - if (relativeTarget) { - const relativeStemMatch = shortestOf( - allAssetPaths.filter( - (assetPath) => stripExtension(assetPath) === relativeTarget, - ), - ) - if (relativeStemMatch) return relativeStemMatch - } - - // A target with folder segments keeps them in the match (suffix on the - // stem); a bare name matches on the filename stem only. - if (target.includes("/")) { - return shortestOf( - allAssetPaths.filter((assetPath) => - stripExtension(assetPath).endsWith(`/${target}`), - ), - ) - } - return shortestOf( - allAssetPaths.filter( - (assetPath) => stripExtension(posix.basename(assetPath)) === target, +// A target with folder segments keeps them in the match (suffix on the +// stem); a bare name matches on the filename stem only. +if (target.includes("/")) { + const suffixStemMatch = shortestOf( + allAssetPaths.filter((assetPath) => + stripExtension(assetPath).endsWith(`/${target}`), ), ) + if (suffixStemMatch) return suffixStemMatch } +const basenameStemMatch = shortestOf( + allAssetPaths.filter( + (assetPath) => stripExtension(posix.basename(assetPath)) === target, + ), +) +if (basenameStemMatch) return basenameStemMatch +return null Suggestion importance[1-10]: 4 __ Why: The suggestion improves readability by making the null return explicit and consistent with earlier tiers. It does not change behavior and is a minor style enhancement.	Low
	Clarify asset desiredTarget comment When targetKind is "asset", desiredTarget is set to resolvedBefore (the asset's path). This is correct because assets don't move. However, the comment "leave other targets as-is" is ambiguous — it could be read as "leave the link text as-is" (i.e., return null). Clarify that desiredTarget for assets is the resolved asset path, which is then used to compute a replacement target if the source note's move breaks the relative link. src/vault-mcp/vault-operations/note-mover.ts [189-193] -const resolvedBefore = - resolvedNoteBefore ?? - links.resolveAsset({ - target: rawTarget, - allAssetPaths: context.allAssetPaths, - sourcePath: context.oldSourcePath, - }) -if (resolvedBefore === null) return null - -// Follow the moved note to its new location; leave other targets as-is. +// Follow the moved note to its new location; asset targets stay at their +// resolved path (assets never move — only the source note's shift can +// break the link). const desiredTarget = targetKind === "note" && resolvedBefore === context.oldTargetPath ? context.newTargetPath : resolvedBefore Suggestion importance[1-10]: 2 __ Why: The improved comment clarifies the behavior for asset targets, reducing ambiguity. It is a documentation-only change with very low impact on correctness or functionality.	Low

[aliasunder]

Responding to Sourcery's Overall Comments (review body, no inline threads):

1. Split note vs asset resolution / link-form classification into smaller helpers — already factored this way: asset resolution lives in links.resolveAsset (pure, parser layer, separate from links.resolve for notes), link-form classification is its own helper (classifyLinkForm), and replacement construction is buildReplacementTarget. rewriteTarget itself is the composition point. Splitting further would produce fragments that only make sense as "the part of rewriteTarget that does X", which the repo's premature-abstraction guard (AGENTS.md) rules out.

2. In-memory index instead of array scans for listAssets/resolveAsset — intentional. Asset paths are read from the filesystem rather than the search index because the index can be stale relative to a just-issued move. The per-link array scans mirror exactly how note resolution (links.resolve over allNotePaths) has always worked, and moveNote is an infrequent, single-shot operation whose cost is dominated by the directory walk and vault-wide rewrite I/O, not the in-memory scans. If profiling ever shows otherwise, a basename/stem map would be added symmetrically for notes and assets — not as a one-off here.

---

🔍 ship-check · pr-monitor · claude-fable-5

[aliasunder]

@CodeRabbit review

[coderabbitai]

✅ Action performed

Review finished.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

📝 Walkthrough

Walkthrough

The pull request adds vault-wide asset listing and resolution, preserves markdown link extensions, and updates note moves to rewrite relative asset links using the new asset paths. Tests cover parsing, resolution, filesystem listing, and moved-note link behavior.

Changes

Asset-aware note moving

Layer / File(s)	Summary
Vault asset inventory src/vault-mcp/vault-operations/vault-filesystem.ts, src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts	A shared vault walker lists visible notes and non-markdown assets, and vaultFs.listAssets is added with recursive listing tests.
Extension-preserving link resolution src/vault-mcp/obsidian-markdown/links.ts, src/vault-mcp/obsidian-markdown/__tests__/links.test.ts, src/vault-mcp/search/search-index.ts	Markdown parsing captures extensions, resolveAsset and stripExtension are exposed, and search indexing uses the shared extension helper.
Note move asset-link rewriting src/vault-mcp/vault-operations/note-mover.ts, src/vault-mcp/vault-operations/__tests__/note-mover.test.ts	Move-time rewriting resolves notes and assets, preserves link grammar and extensions, and tests moved-note, backlink, frontmatter, and locking paths.
Move tool contract and documentation src/vault-mcp/mcp-core/tools/vault-crud-tools.ts, AGENTS.md	The move tool fetches note and asset paths concurrently, passes assets to noteMover, and documents relative attachment rewriting and extension preservation.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

• aliasunder/vault-cortex#160: Directly overlaps the note-move and shared link-resolution changes.
• aliasunder/vault-cortex#213: Overlaps the vault filesystem traversal and listing logic.
• aliasunder/vault-cortex#247: Overlaps the moveNote orchestration path.

Suggested labels: Review effort 3/5

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly matches the main change: rewriting relative asset links when a note is moved.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch worktree-mover-asset-links

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

src/vault-mcp/obsidian-markdown/links.ts (1)

165-181: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

Decode the complete target before separating its extension.

Only the path group is decoded. A valid target such as photo.p%6Eg therefore cannot resolve photo.png, while photo%2Epng is incorrectly treated as extensionless during rewriting. Decode the reconstructed target once, then derive path and extension.

Proposed fix

   const prefix = parts[1]
   const encodedPath = parts[2]
-  const extension = parts[3] ?? ""
+  const encodedExtension = parts[3] ?? ""
   const heading = parts[4] ?? ""
   const closeParen = parts[5]
   const hasRequiredGroups =
     prefix !== undefined &&
     encodedPath !== undefined &&
     closeParen !== undefined
   if (!hasRequiredGroups) return null
+  const decodedTarget = safeDecodeURIComponent(
+    `${encodedPath}${encodedExtension}`,
+  )
+  const extension = posix.extname(decodedTarget)
   return {
     prefix,
-    path: safeDecodeURIComponent(encodedPath),
+    path:
+      extension.length > 0
+        ? decodedTarget.slice(0, -extension.length)
+        : decodedTarget,
     extension,
     heading,
     closeParen,
   }

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/vault-mcp/obsidian-markdown/links.ts` around lines 165 - 181, Update
splitMarkdownLink to decode the complete reconstructed target before separating
its extension. Preserve the prefix and heading handling, but combine the encoded
path with its extension, pass that target through safeDecodeURIComponent once,
then derive the returned path and extension from the decoded result so encoded
dots and extension characters are recognized correctly.

🤖 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.

Outside diff comments:
In `@src/vault-mcp/obsidian-markdown/links.ts`:
- Around line 165-181: Update splitMarkdownLink to decode the complete
reconstructed target before separating its extension. Preserve the prefix and
heading handling, but combine the encoded path with its extension, pass that
target through safeDecodeURIComponent once, then derive the returned path and
extension from the decoded result so encoded dots and extension characters are
recognized correctly.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3d7d2392-a792-4e30-957b-374c41059923

📥 Commits

Reviewing files that changed from the base of the PR and between 6e9c4dd and 3e209f2.

📒 Files selected for processing (9)

• AGENTS.md
• src/vault-mcp/mcp-core/tools/vault-crud-tools.ts
• src/vault-mcp/obsidian-markdown/__tests__/links.test.ts
• src/vault-mcp/obsidian-markdown/links.ts
• src/vault-mcp/search/search-index.ts
• src/vault-mcp/vault-operations/__tests__/note-mover.test.ts
• src/vault-mcp/vault-operations/__tests__/vault-filesystem.test.ts
• src/vault-mcp/vault-operations/note-mover.ts
• src/vault-mcp/vault-operations/vault-filesystem.ts

[aliasunder]

Responding to CodeRabbit's outside-diff finding (review body — decode the complete target before separating its extension in splitMarkdownLink):

Valid — fixed in 3152c59. The path group was decoded while the extension group stayed raw, so [alt](photo%2Epng) parsed as extensionless and [alt](photo.p%6Eg) never resolved — disagreeing with extractFromBody, which decodes the full target (and with Obsidian). The fix decodes the reconstructed target once and derives path/extension from the decoded result via the existing stripExtension helper (rather than posix.extname), keeping the module's extension semantics — last dot in the filename, dotfiles extensionless — in one place. Two splitMarkdownLink scenarios added, mutation-checked (both fail without the fix); 1922 tests green.

---

🔍 ship-check · pr-monitor · claude-fable-5