# 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).

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


## Broker OIDC AllowDomains gate (shipped 2026-06-08, a8d39a5)

Three known gaps left open when the broker-global `OIDC.AllowDomains`
hd gate landed. Code is correct on the predicate level (9-row unit
test in `authz_test.go` `TestOIDCDomainAllowed`) but coverage and
defensive posture have soft edges. Context lives in
`/journal/2026-06-08.md`.

### Kind smoke does not exercise the AllowDomains gate

`deploy/kind/up.sh --with-mcp-smoke` covers the bare auth-code grant
end-to-end against mock-oauth2-server (PR3 of the auth-code plan).
It does NOT drive a request through a non-matching `hd` and assert
the broker rejects. The gate at all three `verifier.Exchange` call
sites (`server.go` `authCallback`, `oauth_authorize.go`
`authCodeCallback`, `device.go` `deviceCallback`) is therefore
mocked-only.

**Fix:** add a stage in `--with-mcp-smoke` that boots the broker
with `oidc.allowDomains: ["allowed.example"]`, signs in via
mock-oauth2-server with a token carrying `hd: "blocked.example"`,
asserts the redirect carries `error=access_denied` (auth-code path)
and the device-flow polling client sees `access_denied`. Mirrors
the PR3 stage's shape.

### Behavioral tests missing for two of three gate sites

Only `TestOAuthAuthorizeAllowDomainsRejectsForeignHD` exercises the
gate end-to-end (auth-code path). The bare `/auth/callback` and
device-flow callback gates are wired by inspection only; covered
by the predicate unit test but not by an integration assertion that
the right error surface fires (403 vs `Deny(deviceCode)` vs
`renderDeviceDone`).

**Fix:** two parallel tests, modeled on the auth-code one.
Device-flow: assert `s.deviceStore.LookupByDeviceCode` returns
status=denied after the callback fires with a foreign-hd claim.
Bare callback: assert HTTP 403 with body "domain not permitted".

### Claims→`*Claims` refactor widened the mutation surface

Adding `HD string` to `Claims` pushed the struct from 64 to 80 bytes
and tripped gocritic's `hugeParam` at five sites, forcing the
project-wide switch of `Claims`-by-value to `*Claims` (including
the context-stored value via `ctxWithClaims` / `claimsFromCtx`).

Mechanical, vet+lint+tests all green: but two callers now mutate
through the pointer:

- `gateWrite(claims *Claims, ...)` does `claims.Email =
  canonicalEmail(claims.Email)`. Previously mutated a local value
  copy; now mutates the caller's `*Claims` (which in MCP-handler
  callers is the context-stored value).
- `meInstall` does `claims.Email = strings.ToLower(strings.TrimSpace(
  claims.Email))` on the context-stored `*Claims`.

In current callers both mutations are invisible (idempotent
lowercase, or the request ends right after). Still a sharp edge a
future contributor will hit. **Fix:** snapshot at the two mutation
sites: `claims := *claims` shadows the parameter with a local
value so the mutation can't leak. Two lines, no behavior change in
the current call graph.

### Refresh path does not re-gate on hd (intentional, but worth recording)

If `AllowDomains` is tightened after grants are out, existing
refresh tokens keep minting fresh id_tokens until `RefreshTokenTTL`
expires them. Acceptable for an early POC (no grants out yet) but
means AllowDomains is NOT a "kick someone out" lever today; only
a "stop new sign-ins" lever. Re-gating refresh requires reading
the cached `Claims.HD` on `Issue` / refresh-grant and rejecting if
it's no longer in `AllowDomains`. Defer until a deployment actually
needs the kick-out behavior.


## Related documents

- [Conflict merge plan](/plans/conflict-merge.md): merge-candidate feature behind the self-conflict debt item
- [Broker auth-code grant plan](/plans/broker-auth-code-grant.md): PR3 smoke stage the AllowDomains gaps build on
- [Journal 2026-06-08](/journal/2026-06-08.md): context for the AllowDomains gate debt
- [Security hardening plan](/plans/security-hardening.md): plan covering the path-safety items listed here


## Graph node identity depends on how the address was typed

`demarkus graph` keys nodes by the raw start URL while the MCP crawler canonicalizes to `host:port` first, so the same document lands under two keys depending on which client crawled it and whether the operator typed the port. A shared `~/.mark/graph.json` then answers backlinks from only half its data. One-line fix in `client/cmd/demarkus/main.go` (canonicalize before `CrawlAndPersist`, matching `client/cmd/demarkus-mcp/main.go`), plus a regression test asserting both clients produce identical node keys for the same document. Detail in [/debugging.md](/debugging.md). Related gap: [/plans/graph-completeness.md](/plans/graph-completeness.md).

## Two byte-identical copies of the Obsidian plugin plan

`/plans/obsidian-plugin.md` and `/plugins/obsidian/plan.md` have the same content hash (`sha256-d421c1e3...`). Neither body claims to supersede the other, so both were left in place during the 2026-08-17 catalog sweep. Decide which path owns the subject, then archive the other or make it a stub with `rel-superseded-by`.