# 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:

```go
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.