fix(dispatch): require params at the MCP boundary (closes #210)

[aaronsb]

Summary

• Closes vault.update silently corrupts files to literal "undefined" when content parameter is missing #210 — vault.update with missing content was writing the literal 9-byte string "undefined" to the target file, with a nominal-success response (✓ Updated: undefined).
• Fixes the class of bug, not just the instance: the same String(params.x) footgun existed at every vault-mutating dispatch site. edit.append would have appended "undefined"; edit.window would have searched for or replaced with "undefined". Path-only String(params.path) sites (view, edit.patch, edit.at_line, edit.from_buffer) produced cryptic "File not found: undefined" instead of a clean dispatch rejection.
• Adds requireParamStr(params, key, action, hint?) in operations/shared.ts — throws at the MCP boundary when a required string param is missing or wrong-typed, so a malformed call cannot reach a vault sink. Sweeps all affected dispatch sites in vault.ts and router.ts.
• vault.update's content guard carries a hint pointing at vault.patch / edit.window — that's the wrong-tool path that produced the corruption in the wild.
• Lifts edit.*'s path guard above FileLockManager.withLock so a missing path can't take a lock on the literal string "undefined".
• updateFile / deleteFile now return { success, path } so formatFileWrite renders Updated: <path> instead of Updated: undefined — belt-and-suspenders for the second issue called out in vault.update silently corrupts files to literal "undefined" when content parameter is missing #210.

Files changed

• src/semantic/operations/shared.ts — requireParamStr helper
• src/semantic/operations/vault.ts — guards on create / update / delete
• src/semantic/router.ts — guards on edit.window, edit.append, edit.patch, edit.at_line, edit.from_buffer, view.file, view.window, view.open_in_obsidian; lock-key guard hoisted
• src/utils/obsidian-api.ts — updateFile / deleteFile return path
• tests/dispatch-param-guards.test.ts — new regression suite (9 cases)

Test plan

• npm test — 305/305 across 24 suites, including the new 9
• npm run build — type-checks clean
• npm run lint — no new warnings (5 pre-existing, unrelated to this change in main.ts / image-handler.ts)
• Confirmed the exact #210 reproduction shape ({action:"update", path, mode, search, replacement} with no content) now throws before any vault.modify call — verified by recording-API test that asserts zero mutations on the malformed call AND that existing.md content is untouched.
• Manual smoke against a live Obsidian vault — the unit tests prove the guard runs; would be nice to confirm a real client sees the new error message and recovery hint. Happy to do that if requested before merge.

Closes #210.

[github-actions]

✅ Build succeeded! Artifacts are available in the Actions tab.

[aaronsb]

Review Summary

Focused review against the PR's six asks. One BLOCKING finding in the recovery hint; everything else is sound or NIT-tier.

---

BLOCKING

vault.update's content-guard hint points at a non-existent tool

src/semantic/operations/vault.ts:134

"For partial replacement, use vault.patch with operation='replace', oldText, newText — or edit.window for fuzzy in-place edits.",

There is no vault.patch action — the vault dispatch switch (src/semantic/operations/vault.ts:17-141) has list/read/fragments/create/update/delete/search/move/rename/copy/split/combine/concatenate. patch lives under edit.* (src/semantic/router.ts:170). A user (or worse, an LLM client) following this hint will call a tool that doesn't exist, get a routing error, and lose more trust than they would from a generic "missing content" message. Bad recovery hints are worse than no hints — that was the explicit ask in the PR description.

The signature is otherwise correct for edit.patch with operation: 'replace': the router accepts oldText/newText (camelCase) at the input boundary (src/semantic/router.ts:176-177) and remaps to snake_case for patchVaultFile internally.

Suggested fix — change the action name only:

"For partial replacement, use edit.patch with operation='replace', oldText, newText — or edit.window for fuzzy in-place edits.",

---

NON-BLOCKING

1. edit.window without oldText/newText rejects before searching the file — the test name overpromises

tests/dispatch-param-guards.test.ts:148-156

The assertion is expect(api.mutations).toEqual([]). That holds regardless of whether the guard runs, because RecordingAPI's existing.md content ('original') doesn't contain the literal string 'undefined', so even a leaked performWindowEdit call would fail fuzzy-match, return { isError: true }, and never hit updateFile. The test passes today because of the guard and because of incidental content shape. To genuinely prove "before searching the file" you'd need to count getFile calls — e.g. add a reads: string[] recorder to RecordingAPI and assert api.reads is empty on the malformed call.

Not blocking because the guard itself is correct (router.ts:145-146) and the corruption-class case for vault.update (the actual #210 shape) is stringently proven by the immediately-preceding test. But the over-strong test name is the kind of thing that masks a future regression.

2. edit.* without path test only exercises action: 'append'

tests/dispatch-param-guards.test.ts:161-169

The path-hoist applies to all five edit actions (window, append, patch, at_line, from_buffer). Worth a describe.each over the action list, or at minimum a comment noting that the lock-hoist is structurally shared so one case is representative.

3. Comment indentation glitch

src/semantic/router.ts:140 — // (five spaces) where surrounding block is four. Will trip a linter if you ever tighten indent rules.

---

NIT

• requireParamStr accepts empty string "". For path parameters that's not what you want (will defer to getAbstractFileByPath("") → "File not found: "). For content on vault.update, "" is a legitimate "truncate to empty" call. Both match pre-existing behaviour, so no regression — but if you ever want stricter path validation, a separate requireNonEmptyParamStr or a { allowEmpty: false } option would cover it. Out of scope for this PR.
• requireParamStr type-discrimination on null: typeof null === 'object' so a null param yields "got object". Functionally correct (rejects), message is fine. Not worth handling separately unless callers care.

---

Sweep verification

grep -rn "String(params\\." src/ returns zero hits — the class of bug is gone from src/. Remaining String(...) calls in src/semantic/ (String(i+1).padStart(...), String((f as Record<string, unknown>).source) after explicit narrowing) are non-param coercion. Other vault-mutating actions (move, rename, copy, split, combine, concatenate) already used paramStr() with explicit !path || !destination checks, not the String(...) footgun.

Return-shape widening (updateFile/deleteFile → { success, path })

Audited every consumer:

• src/security/secure-obsidian-api.ts:111,143 — ReturnType<ObsidianAPI[...]> inherits the widening, no change needed.
• src/tools/window-edit.ts:54,108,313 and src/semantic/router.ts:219 and all ctx.api.updateFile/deleteFile calls in src/semantic/operations/vault.ts — all use await ... and ignore the return value or pass it straight to the formatter.
• src/formatters/vault.ts:315-320 — FileWriteResponse.path: string was already required. Pre-PR, updateFile's { success: true } return failed that contract silently and rendered "Updated: undefined". The widening makes the runtime match the type. This is a fix, not a risk.

No downstream consumer destructures the narrow shape. The widening is purely additive.

CLAUDE.md / SOLID alignment

• Validate at system boundaries — requireParamStr is exactly the boundary-validation pattern the project doctrine calls for. Lifted above FileLockManager.withLock so a missing path can't take a lock on the literal "undefined" is the same pattern applied one level deeper. Good.
• IObsidianAPI stability — updateFile / deleteFile widen their return shape additively. Existing field (success) preserved, type-narrow consumers unaffected. Compatible.
• Error-shape compatibility — requireParamStr throws plain Error, which the router's handleError already wraps into the standard { code: 'UNKNOWN_ERROR', message, recovery_hints } shape. Consistent.
• Single Responsibility — helper lives in operations/shared.ts alongside the other paramX helpers. Right home.

Test coverage

305/305 across 24 suites pass on fix/210-dispatch-param-guards. The corruption-class case for the exact #210 reproduction shape is stringently proven (mutations array empty AND source file content untouched). The edit.window and edit.* path-hoist tests are correct but coverage-thin (see NON-BLOCKING #1, #2).

---

Disposition

Fix the BLOCKING hint string (one-character action namespace error: vault.patch → edit.patch), then ship. Non-blockers can land in a follow-up.

[github-actions]

✅ Build succeeded! Artifacts are available in the Actions tab.