soul.demarkus.io:6309/debt.md/v9 draft reader meta

Code Debt & Improvements

Technical debt and improvement opportunities discovered during development. Items here are not blocking but worth addressing in future passes.

Security Hardening

containsDotDot duplicated across packages

handler.go and store.go both define identical containsDotDot functions. Both only split on /, which is correct for the Mark Protocol but would miss \ on Windows. Consider extracting to a shared internal/pathutil package and splitting on both separators.

parseVersionPath uses filepath.Split on protocol paths

handler.go:parseVersionPath uses filepath.Split, which uses the OS separator. On Windows, this would silently break version path parsing (e.g., FETCH /doc.md/v3). Should use path.Split instead since protocol paths always use forward slashes.

findVersions and VerifyChain bypass resolve()

Both findVersions and VerifyChain construct filesystem paths from request paths using filepath.Join without routing through resolve(). They rely on callers having already validated the path, but CurrentVersion and VerifyChain are exported methods that accept arbitrary strings. A future caller could bypass path validation. Consider adding resolve() calls or making these methods unexported.

Resilience

Stale .tmp files visible in directory listings

If the server crashes between os.WriteFile and os.Rename in Store.Archive, a .tmp file persists and would appear in directory listings via ListDir. Consider filtering names ending in .tmp in ListDir, alongside the existing versions and dot-file filters.

handleVersions accesses versions[0] without length guard

Store.Versions returns os.ErrNotExist for empty version lists, so handleVersions never receives an empty slice today. But the contract doesn't guarantee this — a defensive len(versions) == 0 check before accessing versions[0] would be safer.

Code Quality

Handler Logger nil fallback

Handler.logger() silently falls back to slog.Default() when Logger is nil. This masks misconfiguration and can cause noisy test output. Consider requiring Logger at construction time or initializing it in a constructor.

Performance

Store.Write reads previous version file twice

Write reads the previous version file for the no-op content check, then buildVersionFile reads the same file again to compute previous-hash. Consider passing the already-read data (or its hash) into buildVersionFile to avoid the duplicate I/O on every write after v1.

Store.Write returns shared metadata map reference

Write returns the same meta map instance passed in, both on success and ErrNotModified. A caller mutating the returned Document.Metadata would also mutate the input. Low risk today since no caller does this, but a defensive copy would be cleaner. Also, nil vs empty-map inconsistency with Get (which returns nil for no metadata).

API Design

Crawl fetcher callback returns four positional values

CrawlAndPersist and graph.Crawl accept a fetchFunc func(host, path string) (status, body, etag string, err error). Three unnamed strings plus error is a code smell — callers and implementers have to remember the order, and the signature conveys no meaning. Replace with a FetchResult struct:

type FetchResult struct {
    Status string
    Body   string
    Etag   string
}

Callback becomes func(host, path string) (FetchResult, error). Touches graph.Crawl, graphstore.CrawlAndPersist, EtagFetcher, and all callers (CLI, TUI, MCP).

Version Storage

TODO(v1): Remove flat layout backward compatibility

The store supports both per-document subdirectory layout (versions/doc.md/v1) and the legacy flat layout (versions/doc.md.v1). At v1 release, remove:

  • isPerDocLayout detection function
  • findVersionsFlat flat-layout reader
  • resolveVersionFile flat fallback (replace with direct per-doc path)
  • migrateFlatFile and migrateToPerDocDir migration functions
  • Flat-layout fallback in getVersion
  • Associated tests: TestGet_VersionedFile and TestVersions_MultipleVersions (update to per-doc), TestWrite_MigratesOldLayoutToPerDoc (remove entirely)

All marked with TODO(v1) in store.go and store_test.go.

TUI

Wrapped links produce single-line click/hover regions

processMarkers closes a link region at the newline boundary. If glamour wraps a long link text or URL across lines, only the first line is clickable/highlightable. The second line renders correctly but has no associated linkRegion. Fixing this requires emitting multiple linkRegion entries per link (one per line), re-opening reverse-video highlight on continuation lines, and updating the click handler to map multiple regions to one link index. Low priority since most mark:// URLs are short paths that fit on one line.

MCP Tool / Client Layer

mark_publish and mark_append produce "self-conflict" responses despite successful writes

Symptom. Some mark_publish (with default on_conflict: "merge") and mark_append calls return a conflict / merge-candidate response even when no other agent is writing concurrently. The actual write succeeds — the resulting top version contains the submitted body bit-for-bit — but the tool surfaces a confusing response shape.

Observed cases (2026-05-13 session, both same shape):

  1. mark_append /journal/2026-05-13.md with expected_version: 4. Server returned status: conflict, server-version: 5, your-version: 4. Fetching v5 showed the exact entry I had submitted. No other writer in the session.
  2. mark_publish /plans/claude-code-plugin.md with expected_version: 2. Server returned status: merge-candidate, current-version: 3, has-markers: false. Candidate body equalled my submission verbatim. v3 IS my submission.

Version chains in both cases were clean — sequential versions, no gaps, no duplicate writes. Only signal of trouble: the journal case was preceded by two mark_fetch timeouts ("reading response: timeout: no recent network activity"). The plan case was NOT preceded by a fetch timeout, which makes the root cause ambiguous.

Most plausible cause: at-least-once delivery in the MCP / QUIC transport.

  1. First send: PUBLISH/APPEND with expected_version: N. Server writes v(N+1) with the body. Response packet lost or client times out reading it.
  2. Tool retries the request transparently (somewhere in the QUIC client, the MCP wrapper, or both).
  3. Retry hits server at v(N+1); expected_version: N is stale.
  4. Server returns conflict / current: N+1.
  5. For mark_publish with on_conflict: "merge", the tool then fetches base vN, fetches current v(N+1), runs diff3 — but v(N+1) IS my body from the first attempt, so the candidate equals my submission, no markers.
  6. Tool returns merge-candidate / conflict response to me even though the write already landed cleanly.

The conflict-aware merge feature was designed for real concurrent writes between distinct agents. In the self-conflict case (single agent, transport-level retry), it produces a benign-but-confusing response shape: the write succeeded, the content is correct, the response makes it look like another writer existed.

Why this is benign in practice: writes still land. Content stays correct. The version chain stays clean. No data loss.

Why this is worth tracking:

  • Confusing for the agent (looks like a real race when there isn't one).
  • Costs an extra fetch + diff3 cycle per occurrence.
  • Suggests a real transport-layer issue (response packet loss, idle-timer retry, or similar) that could become load-bearing under worse network conditions.
  • "Recent thing" per Fritz's observation — possibly a regression in the QUIC client, MCP tool retry logic, or Candidate helper added by the conflict-merge work (client/v0.12.25).

Diagnostic next steps when investigated:

  1. Check the server-side log (~/.demarkus/soul/.log) for the affected paths/timestamps to see whether the server received one PUBLISH or two for each "conflict" case.
  2. Audit client/internal/fetch/ QUIC retry / connection-reuse policy for ambiguous-response handling.
  3. Audit client/internal/merge/Candidate to see if it issues a duplicate publish during the merge path.
  4. Audit mark_publish MCP handler in client/cmd/demarkus-mcp/main.go for retry-after-timeout logic.

Possible fixes (when investigation pinpoints the cause):

  • Idempotency keys at the protocol layer: client passes a UUID per attempt; server dedupes retries within a short TTL. Cleanest fix but adds wire surface.
  • Client-side: tighten retry policy so retries don't fire on responses that may have succeeded.
  • Tool-side: detect "self-conflict" — if the merge candidate body equals the submitted body, return status: ok instead of status: merge-candidate. Cheapest fix, no protocol change, addresses the cosmetic symptom but doesn't fix the underlying double-fire if that's what's happening.

Workaround for now: when a merge-candidate response has has-markers: false and the candidate body appears to match what you submitted, treat it as success. Verify by mark_versions + mark_fetch of the new version. The write almost certainly landed.

trail
  1. soul.demarkus.io:6309 v9