Skip to content

refactor(utils): extract describeError + ENOENT fs helpers into src/utils#171

Merged
aliasunder merged 1 commit into
mainfrom
claude/vault-bootstrap-setup-lk9df4
Jun 22, 2026
Merged

refactor(utils): extract describeError + ENOENT fs helpers into src/utils#171
aliasunder merged 1 commit into
mainfrom
claude/vault-bootstrap-setup-lk9df4

Conversation

@aliasunder

@aliasunder aliasunder commented Jun 22, 2026

Copy link
Copy Markdown
Owner

What

The third and final slice of the parser/IO restructure (after #168 and #169). Hoists two generic, domain-free helpers — each repeated across the codebase — into the existing src/utils/ leaf.

utils/describe-error.ts

describeError(error) — the error instanceof Error ? error.message : String(error) idiom, which appeared inline 12× across 8 files (and as a private helper in note-mover). Every site now imports the one implementation.

utils/fs.ts

readFileOrNull / readdirOrNull / fileExists — the ENOENT‑to‑null/false filesystem wrappers, previously private in vault-filesystem (readFileOrNull, readdirOrNull) and note-mover (fileExists). memory-store's readMemoryFileOrNull now delegates to readFileOrNull too, retiring another hand‑rolled ENOENT check.

These clear the bar for utils/: generic, zero domain knowledge, multi‑consumer. (isString/isDate were deliberately left out — low value — per the plan.)

Scope

No behavior change — pure extraction + call‑site rewiring. The only remaining instanceof Error in src/ is the canonical one inside describe-error.ts.

Tests

  • New utils/__tests__/describe-error.test.ts and utils/__tests__/fs.test.ts, including the non‑ENOENT rethrow contract (reading a directory as a file must propagate EISDIR, not be masked as "missing").

Verification

  • npm run build (tsc, server + cli) ✅
  • npm test927 passed
  • npm run lint

🤖 Generated with Claude Code

Summary by CodeRabbit

  • New Features

    • Enhanced error messaging with improved human-readable error descriptions throughout the application
    • Atomic file writing now supports optional hard links flag for better performance
  • Bug Fixes

    • File system operations are now more resilient with graceful handling of missing files
  • Refactor

    • Consolidated error handling logic across modules for consistency and maintainability
  • Tests

    • Added comprehensive test coverage for error handling and file system utilities

…tils

The third and final slice of the parser/IO restructure (after #168, #169).
Hoists two generic, domain-free helpers — repeated across the codebase — into
the existing utils/ leaf.

- utils/describe-error.ts: describeError(error) — the
  `instanceof Error ? .message : String(...)` idiom that appeared inline 12x
  across 8 files (and as a private helper in note-mover). All collapse onto it.
- utils/fs.ts: readFileOrNull / readdirOrNull / fileExists — the ENOENT-to-
  null/false fs wrappers, previously private in vault-filesystem and note-mover.
  memory-store's readMemoryFileOrNull now delegates to readFileOrNull.

No behavior change. New unit tests for both modules (incl. the non-ENOENT
rethrow contract). 927 tests, build + lint green.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
@qodo-code-review

Copy link
Copy Markdown

Qodo found 1 finding(s) but none met the inline_comments_severity_threshold (current: 1). Lower the threshold or switch comments_location_policy to 'summary' or 'both' to see them.

@aliasunder

Copy link
Copy Markdown
Owner Author

/@CodeRabbit

@coderabbitai

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

@aliasunder Sure! I'll kick off a review of this PR right away.

✅ 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

coderabbitai Bot commented Jun 22, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6c8df291-dacc-44c5-bb7f-138f6bda1fff

📥 Commits

Reviewing files that changed from the base of the PR and between ce0cc83 and 809ca46.

📒 Files selected for processing (14)
  • AGENTS.md
  • src/utils/__tests__/describe-error.test.ts
  • src/utils/__tests__/fs.test.ts
  • src/utils/describe-error.ts
  • src/utils/fs.ts
  • src/vault-mcp/mcp-core/prompt-definitions.ts
  • src/vault-mcp/mcp-core/tool-definitions.ts
  • src/vault-mcp/search/file-watcher.ts
  • src/vault-mcp/search/search-index.ts
  • src/vault-mcp/server.ts
  • src/vault-mcp/vault-operations/daily-notes.ts
  • src/vault-mcp/vault-operations/memory-store.ts
  • src/vault-mcp/vault-operations/note-mover.ts
  • src/vault-mcp/vault-operations/vault-filesystem.ts

📝 Walkthrough

Walkthrough

Introduces two new shared utility modules — src/utils/describe-error.ts (error-to-string conversion) and src/utils/fs.ts (ENOENT-safe readFileOrNull, readdirOrNull, fileExists) — with Vitest tests. Removes all duplicate local implementations across vault-filesystem.ts, note-mover.ts, and memory-store.ts, and migrates six additional consuming modules to describeError. Also extends atomicWriteFileExclusive with an optional hardLinksSupported flag.

Changes

Shared Utility Extraction and Consumer Migration

Layer / File(s) Summary
New shared describeError and fs utilities
src/utils/describe-error.ts, src/utils/fs.ts, src/utils/__tests__/describe-error.test.ts, src/utils/__tests__/fs.test.ts, AGENTS.md
Exports describeError(error: unknown): string, readFileOrNull, readdirOrNull, and fileExists with ENOENT suppression and rethrow for other errors; full Vitest coverage and AGENTS.md documentation added.
vault-filesystem.ts: remove local helpers, extend atomicWriteFileExclusive
src/vault-mcp/vault-operations/vault-filesystem.ts
Removes local readFileOrNull/readdirOrNull implementations and imports shared versions; adopts describeError in prune-error logging; extends atomicWriteFileExclusive with options?.hardLinksSupported to branch between link-based and atomic wx-placeholder + rename write strategies.
note-mover and memory-store: remove local duplicates
src/vault-mcp/vault-operations/note-mover.ts, src/vault-mcp/vault-operations/memory-store.ts
Removes local fileExists and describeError from note-mover.ts in favor of imports; replaces readMemoryFileOrNull's manual try/catch ENOENT logic in memory-store.ts with a direct call to shared readFileOrNull.
Adopt describeError across mcp-core, search, server, and vault-operations
src/vault-mcp/mcp-core/prompt-definitions.ts, src/vault-mcp/mcp-core/tool-definitions.ts, src/vault-mcp/search/file-watcher.ts, src/vault-mcp/search/search-index.ts, src/vault-mcp/server.ts, src/vault-mcp/vault-operations/daily-notes.ts
Replaces all remaining inline err instanceof Error ? err.message : String(err) patterns with describeError(err) in catch blocks, error log payloads, and tool response paths.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~12 minutes

Possibly related PRs

  • aliasunder/vault-cortex#160: Introduced note-mover.ts's vault_move_note implementation including the local fileExists and describeError definitions that this PR now replaces with shared utilities.
  • aliasunder/vault-cortex#163: Introduced/expanded pruneEmptyParents in vault-filesystem.ts; this PR changes its failure-path error logging to use describeError(error).
  • aliasunder/vault-cortex#164: Both PRs modify atomicWriteFileExclusive in vault-filesystem.ts to support a wx-reserve + rename fallback path for environments without hard-link support.
🚥 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 accurately and specifically describes the main refactoring: extracting describeError and ENOENT-safe filesystem helpers into src/utils.
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.

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

✨ 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 claude/vault-bootstrap-setup-lk9df4

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

❤️ Share

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

@aliasunder
aliasunder merged commit bffc7e2 into main Jun 22, 2026
15 checks passed
@aliasunder
aliasunder deleted the claude/vault-bootstrap-setup-lk9df4 branch June 22, 2026 15:59
aliasunder added a commit that referenced this pull request Jun 22, 2026
…ort conventions (#172)

## What

Capstone to the parser/IO restructure (#168, #169, #171). Adds a
**Module layering** section to `AGENTS.md` so the structure those PRs
built is durable and enforceable — recorded as rules, not just implied
by the file tree. Also lands the structure-polish follow-ups (folder
rename + boundary clarifications) agreed during review.

It documents:

- **What each folder is for** (`obsidian-markdown/` ·
`vault-operations/` · `mcp-core/` · `search/` · `oauth/` · `utils/`),
and that a module's folder follows **what it depends on**, not just its
topic.
- **Dependency direction** — `obsidian-markdown/` and `utils/` are
leaves; `vault-operations/` and `search/` build on them; `mcp-core/` and
the wiring depend on everything — plus the "top level is wiring only"
rule.
- **The `vault-filesystem.ts` ↔ `utils/fs.ts` boundary** — **policy vs.
adapter**: `utils/fs.ts` holds only policy-free `node:fs` wrappers,
while the atomic-write strategy, path-safety, and the `vaultFs` data API
stay in `vault-filesystem.ts`. "Mechanically generic" isn't enough to
demote load-bearing vault-I/O policy.
- **`utils/` admission** — generic with zero domain knowledge, clearing
one of two bars: it consolidates real duplication (multi-consumer), or
it is a self-contained, general-purpose primitive
(`mapWithConcurrency`).
- **Export style by module kind** — service/data-layer modules export a
single namespace object (or a factory-closure when stateful:
`createSearchIndex`/`createMemoryStore`); pure parsers/helpers use named
exports; `links.ts` is the deliberate namespace exception (its `/g`
`lastIndex` footgun).

### Structure polish (also in this PR)

- Renamed `vault-mcp/auth/` → `vault-mcp/oauth/` so it no longer
collides conceptually with the top-level `src/auth.ts` token utilities.

Docs + a folder rename — no behavior change.

🤖 Generated with [Claude Code](https://claude.com/claude-code)

---------

Co-authored-by: Claude <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants