Skip to content

fix(local-rest-api): replace hardcoded Create-Target-If-Missing with agent-controlled option#72

Open
grimlor wants to merge 13 commits intojacksteamdev:mainfrom
grimlor:fix/patch-heading-target-qualification
Open

fix(local-rest-api): replace hardcoded Create-Target-If-Missing with agent-controlled option#72
grimlor wants to merge 13 commits intojacksteamdev:mainfrom
grimlor:fix/patch-heading-target-qualification

Conversation

@grimlor
Copy link
Copy Markdown

@grimlor grimlor commented Mar 12, 2026

Pull Request Description

Replace the hardcoded "Create-Target-If-Missing": "true" header in both patch_active_file and patch_vault_file handlers with logic driven by a new optional createTargetIfMissing boolean parameter on ApiPatchParameters. This gives agents explicit control over whether missing targets (headings, blocks, frontmatter fields) are created during patch operations, rather than always creating them unconditionally.

Additionally extracts the duplicated header-building logic into a pure buildPatchHeaders function with BDD test coverage, fixes pre-existing test failures in parseTemplateParameters, and adds Create-Target-If-Missing as a formal parameter in the OpenAPI spec.

Fixes #71

Type of Change

  • 🐛 Bug fix (non-breaking change that fixes an issue)
  • 📚 Documentation update
  • 🔧 Internal/tooling change

Testing

How has this been tested?

  • Local Obsidian vault testing
  • MCP server functionality verified
  • Claude Desktop integration tested

9 new BDD tests for buildPatchHeaders covering required fields, optional fields, and createTargetIfMissing behavior. All 22 tests across the mcp-server package pass (including 4 previously broken parseTemplateParameters tests).

Architecture Compliance

  • Follows feature-based architecture patterns (see /docs/project-architecture.md)
  • Uses ArkType for runtime validation where applicable
  • Implements proper error handling
  • Follows coding standards in .clinerules

Checklist

  • My code follows the project's coding standards
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to documentation (if applicable)
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes

Security Considerations

  • No hardcoded secrets or API keys
  • Input validation implemented where needed
  • No new security vulnerabilities introduced
  • Follows minimum permission principles

Additional Context

Commits:

  1. feat(local-rest-api): add createTargetIfMissing option to patch handlers — add optional boolean to ApiPatchParameters, wire into both handlers
  2. refactor(local-rest-api): extract buildPatchHeaders as pure, testable function — deduplicate header logic, add 9 BDD tests
  3. fix(templates): update tests and fix stateful regex in parseTemplateParameters — update test templates from tp.user.promptArg to tp.mcpTools.prompt, remove /g flag from TEMPLATER_END_TAG regex that caused lastIndex to skip matches across loop iterations
  4. style(tests): move diagnostic comments above assertions for bun:test idiom — reformat test diagnostics for bun:test conventions
  5. docs(openapi): add Create-Target-If-Missing as formal parameter to PATCH endpoints — header was referenced in prose examples but missing from the formal parameters section across all three PATCH endpoints

grimlor added 4 commits March 11, 2026 18:04
Replace hardcoded "Create-Target-If-Missing": "true" header with a
conditional based on the new optional createTargetIfMissing parameter
in ApiPatchParameters. This gives agents explicit control over whether
missing targets are created during patch operations.
… function

Extract duplicated header-building logic from patch_active_file and
patch_vault_file into buildPatchHeaders(). Add BDD test coverage for
required fields, optional fields, and createTargetIfMissing behavior.
…arameters

Update test templates from tp.user.promptArg to tp.mcpTools.prompt to
match the current implementation renamed in 111297e. Remove unnecessary
/g flag from TEMPLATER_END_TAG regex which caused .test() to advance
lastIndex across loop iterations, skipping matches in multi-parameter
content.
@netlify
Copy link
Copy Markdown

netlify bot commented Mar 12, 2026

Deploy Preview for superb-starlight-b5acb5 canceled.

Name Link
🔨 Latest commit 9cc116c
🔍 Latest deploy log https://app.netlify.com/projects/superb-starlight-b5acb5/deploys/69bf4e74ebbd7e0008984385

grimlor added 4 commits March 11, 2026 19:57
…idiom

bun:test's expect() does not accept diagnostic message arguments like
Jest. Move comments from inside expect() calls to above the assertions
where they serve as readable context without being silently ignored.
…TCH endpoints

The header was referenced in prose examples but missing from the formal
parameters section. Add it to all three PATCH endpoints (active file,
periodic note, vault file) with default "false" matching the new
agent-controlled behavior.
@grimlor grimlor force-pushed the fix/patch-heading-target-qualification branch from 9c453a7 to 27f0f02 Compare March 21, 2026 22:40
@grimlor grimlor force-pushed the fix/patch-heading-target-qualification branch from 5f10d80 to 9cc116c Compare March 22, 2026 02:05
istefox added a commit to istefox/obsidian-mcp-tools that referenced this pull request Apr 11, 2026
…uster A)

Fix three independent bugs in patch_active_file and patch_vault_file that
together made the tools dangerous and unusable for many real-world cases:

1. Silent content corruption with nested headings
   The Local REST API markdown-patch library indexes headings by full
   hierarchical path with the "::" delimiter ("Top Level::Section A"). The
   server was sending partial leaf names ("Section A") verbatim, and because
   Create-Target-If-Missing was hardcoded to true, the lookup failed silently
   and a new heading was appended at EOF instead of patching the intended
   one. The server now parses the target file, resolves the leaf name to
   its full ancestor path via a new resolveHeadingPath helper, and sends
   that to the API. Strict mode is exposed through the new
   createTargetIfMissing optional parameter so agents can opt into
   explicit errors instead of auto-creation.

2. Non-ASCII heading failures
   Target and Target-Delimiter HTTP headers were sent raw, which fails for
   Japanese, emoji, or any heading with characters outside the ASCII
   header grammar. Both headers are now URL-encoded — crucially, *after*
   path resolution, so the indexer lookup still sees plain strings.

3. Missing trailing newlines on append
   When operation == "append", content without trailing newlines would run
   visually into the next section. Normalized to end with "\n\n".

Approach combines three community PRs never merged upstream:
- PR jacksteamdev#72 (grimlor): resolveHeadingPath + createTargetIfMissing
- PR jacksteamdev#69 (marcoaperez): URL-encode Target / Target-Delimiter
- PR jacksteamdev#48 (vanmarkic): test coverage patterns for header encoding

Adds patchVaultFile.test.ts with 10 unit tests for resolveHeadingPath:
empty content, no headings, H1 match, H2 partial, deep H3 nesting,
missing leaf, duplicate names, custom delimiter, stack reset on level
return, and non-heading lines mid-paragraph.

Performance note: resolving a partial heading name adds one extra HTTP
request per PATCH call (to fetch the target file content). Full-path
targets (already containing the delimiter) skip the resolution step.

Manual end-to-end verification pending on Stefano's Obsidian + Local
REST API setup.

Refs: jacksteamdev#30, jacksteamdev#71
Co-Authored-By: Claude Opus 4.6 (1M context) <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.

patch_vault_file silently corrupts content when targeting headings in nested sections

1 participant