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