**Finding 1**
1. Severity: Medium
2. File and line: `src/DarcsWeb/Darcs.hs:286-292`, `app/Main.hs:433-453`
3. What is wrong: `isCloneSubPath` is described and tested as a strict allowlist, but the implementation accepts any path with prefix `inventories/`, `patches/`, or `pristine.hashed/` as long as it has at least two segments. `serveClone` then serves the resolved file verbatim. That means the new "private except for clone objects" boundary is not actually enforced for nested files under those directories.
4. Why it can fail in practice: A repository containing `_darcs/patches/private/secret.txt` or `_darcs/pristine.hashed/debug/notes` would now be downloadable via `/clone/...`, even though the patch explicitly claims unrelated `_darcs` contents stay private. This is a secure-by-default regression, not just a style issue.
5. Minimal fix suggestion: Narrow the accepted shapes to the exact object layouts darcs uses, including only the bucketing scheme you actually intend to support, and add negative tests for extra nesting such as `patches/a/b` and `pristine.hashed/x/y`.
**Finding 2**
1. Severity: Medium
2. File and line: `src/DarcsWeb/Darcs.hs:286-293`, `test/Properties/Clone.hs:186-201`
3. What is wrong: The new allowlist deliberately rejects `_darcs/prefs/motd` and `format`, and the new property tests lock that behavior in. `darcs help manpage` documents `_darcs/prefs/motd` as data shown to users who clone or pull from a repository, so the patch is changing observable clone behavior rather than just hiding unused internals.
4. Why it can fail in practice: HTTP clones served by darcsweb will no longer match darcs's documented clone surface for repositories that rely on MOTD, and possibly other clone-visible metadata such as `format`. Because the tests only assert the predicate, not a real `darcs clone` against the endpoint, CI would still go green while the protocol surface regresses.
5. Minimal fix suggestion: Add an end-to-end clone test that exercises `darcs clone` against the HTTP endpoint and use that to derive the allowlist. At minimum, stop rejecting documented clone-visible metadata until the client behavior has been verified empirically.
**Finding 3**
1. Severity: Low
2. File and line: `app/Main.hs:72-76`
3. What is wrong: `parseOpts` changed from `foldl (flip id)` to `foldr ($)`. That reverses precedence for duplicate options. With the new code, `darcsweb -c first.conf -c second.conf` selects `first.conf` instead of the last `-c`.
4. Why it can fail in practice: Wrapper scripts and service units commonly append overrides. After this change, an appended `-c` no longer overrides an earlier one, so darcsweb can start with the wrong configuration file even though the command line looks correct.
5. Minimal fix suggestion: Apply option updates in input order again with `foldl (flip id)` or an equivalent left-to-right fold, and add a small regression test for duplicate `-c` arguments.
**Residual risks**
- I could not run `stack test` in this sandbox because Stack tries to create a global pantry lock under `/home/fritjof/.stack`, which is mounted read-only here. The review is therefore source-based plus local darcs CLI/manpage checks, not a full test run.
- The clone endpoint change is security-sensitive and protocol-sensitive at the same time. Without an end-to-end clone test, there is still meaningful risk of both overexposure and accidental incompatibility.
**Missing test coverage**
- No test checks duplicate command-line option precedence for `parseOpts`.
- No test exercises `serveClone` against a real `darcs clone`; the new properties only test the predicate in isolation.
- No negative test covers overbroad nested paths such as `patches/a/b`, `inventories/a/b`, or `pristine.hashed/a/b`.
**Changed behavior that remains unverified**
- The new clone allowlist has not been verified against actual darcs client requests.
- The new safe-canonicalization paths for static files and repositories have not been exercised by tests for permission errors, symlink loops, or invalid path bytes.
- The `readMaybe`-based port parsing path has no direct regression test for malformed or duplicated config/CLI inputs.
