Must-fix issues
1. Severity: High
File and line: `app/Main.hs:75`
The quality problem: Replacing `foldl (flip id)` with `foldr ($)` silently reverses option update precedence. With repeated flags such as `-c a -c b`, the first occurrence now wins instead of the last one, which is the opposite of the old behavior and of normal CLI expectations.
Why it matters long-term: This makes the option pipeline harder to reason about and turns future non-idempotent flags into easy sources of surprising behavior.
A practical improvement: Restore left-to-right application order, for example with `foldl' (flip ($)) defaultOpts updates`, or revert to the previous fold and add a regression test for repeated `-c`.
2. Severity: Medium
File and line: `src/DarcsWeb/Darcs.hs:283-293`
The quality problem: `isCloneSubPath` is described as a strict allowlist, but the `("inventories" : _ : _)`, `("patches" : _ : _)`, and `("pristine.hashed" : _ : _)` cases admit any deeper descendant under those directories. That is broader than the documented contract and makes the policy harder to audit.
Why it matters long-term: The next time darcs or local tooling drops unrelated files below one of those directories, they become public automatically even though the surrounding comments and tests suggest a tightly pinned surface.
A practical improvement: Encode the expected path shapes explicitly, ideally at exact depth with filename validation that matches the clone protocol, and add regression tests for rejected cases like `patches/hash/extra` and `inventories/x/y`.
Optional improvements
1. Severity: Medium
File and line: `src/DarcsWeb/Darcs.hs:196-222`
The quality problem: `getRepoSummary` still materializes `PatchSummary` values for the entire patch list just to derive count, last change, recent patches, and tags. The summary route only renders 10 recent patches and 5 tags, but the helper is coupled to the full listing representation.
Why it matters long-term: The function's contract is already wider than the page needs, so future summary-page changes will keep threading through an overstuffed tuple and can accidentally preserve whole-history work that no caller needs.
A practical improvement: Replace the tuple with a dedicated summary record and accumulate only the data the summary page needs: count, latest date, first N recent patches, and tag summaries.
Areas that may become maintenance risks later
- `safeCanonicalize` and `readRepoDescription` both fail closed by discarding the underlying exception. That is appropriate for HTTP responses, but without any logging it will be hard to distinguish malformed input from permission problems or repository corruption in production.
- `RepoInfo` assembly now lives in both `checkRepo` and `getRepoSummary`. If metadata rules change later, the index and summary paths can drift unless that construction is centralized.
Any testing or structure gaps worth watching
- There is no regression test covering repeated CLI options after the `parseOpts` refactor.
- The new clone allowlist tests cover named happy paths, but they do not pin the deeper descendant cases that the current implementation accepts.
- I could not run the suite in this environment: `stack test --fast` is blocked by sandbox writes under `~/.stack`, and `cabal test` fails dependency resolution against the installed `base` and `scotty` set.
