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

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

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.
Tool retries the request transparently (somewhere in the QUIC client, the MCP wrapper, or both).
Retry hits server at v(N+1); expected_version: N is stale.
Server returns conflict / current: N+1.
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.
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:

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.
Audit client/internal/fetch/ QUIC retry / connection-reuse policy for ambiguous-response handling.
Audit client/internal/merge/Candidate to see if it issues a duplicate publish during the merge path.
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.

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. Related gap: /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.

Update 2026-08-17: graph node identity fixed on a branch

Fixed on fix/cli-graph-url-canonicalization, unmerged. The fix landed one layer lower than this entry predicted: not in the CLI, but in graphstore.CrawlAndPersist, which canonicalizes startURL through the caller's parseURL before crawling. That repairs all four crawlers at once (CLI, TUI, broker, MCP) and makes the invariant unbreakable by a new caller, instead of patching the one client that happened to violate it.

A second symptom showed up while fixing it: EtagFetcher keys etags as mark://host:port/path while nodes were keyed by the raw start URL, so Merge never attached an etag to any node from a portless crawl. Conditional re-crawl silently degraded to a full refetch every run. Both symptoms are covered by regression tests that fail without the fix.

Update 2026-08-17: merged as PR #318

Landed on main as 78eeebe. Review caught a regression in the first cut: canonicalizing before graph.Crawl ran its own mark:// prefix check meant an external start URL with a nil parser panicked, where it had previously been recorded as an external node. The merged version parses only mark:// starts and returns an explicit error when one arrives without a parser, since Crawl would otherwise dereference the nil func inside a worker goroutine and take the process down where no caller can recover. Four regression tests cover it, each verified to fail without the guard.

Still open, and the reason this entry stays: normalization happens only at the crawl entry. SeedFromExport still ingests foreign /graph.md rows verbatim, the read paths (GetNode, Backlinks, BacklinksEnriched) still trust the caller's key, and an absolute body link written without a port still creates a second node because links.Resolve passes through anything containing ://. The proposed follow-on is to make identity omit the default port, matching the broker's world-name form and RFC 3986 scheme-based normalization, injected into the store and applied at every door. That changes the published /graph.md interchange format, so it wants an ADR rather than a quiet patch.