Unblinded after review: A = Mercury 3, B = GPT-6 Luna, C = Claude Haiku 5.5. Written by a reviewing agent (Opus) and published unedited. # Review: c07-symlink-utf8 All three candidates add the same `symlink_target.to_str().is_none()` check in `dir_list::list_directory` with a new `NonUtf8SymlinkTarget(PathBuf)` variant that carries the link path. All three pass the 6 hidden tests, their own suites, fmt, clippy and dprint. I built a copy of each tree and ran every command and dry-run combination on a fresh tree and on a tree where a tracked link was retargeted. I also tried a link name containing a control character (escaped correctly by all three), valid non-ASCII and dangling targets, and the init-state precedence cases. ## Candidate A: overall 9/10 Correct fix at the listing choke point, matching the reference. It has the most thorough tests: a unit test, two `compute_status` tests (new link and retargeted tracked link), and a 9-test CLI file covering every command. | Requirement | Verdict | Evidence | | ---------------------------------------------------------------- | ------- | --------------------------------------------------------------------------- | | All commands exit 255 (incl. dry-run, --allow-init) | met | Reproduced; plain `update` on an uninitialized tree gives NotInitialized, like the reference | | Retargeted tracked link rejected | met | status/verify/update(--dry-run, --allow-init) exit 255, wards unchanged | | Error names the link, says not UTF-8 | met | `non-UTF-8 symlink target not supported: .../sub/link` | | Nothing written before failure | met | No root or sub ward after failed init | | Valid UTF-8 targets still work | met | init/status/verify exit 0 | | No lossy conversion | met | Only `to_str()` | Bugs: none found. Scope: SPEC.md updated. Two unrequested changes: - A hand-added `[Unreleased]` entry in `CHANGELOG.md`. That file is generated by git-cliff (`git-cliff -o CHANGELOG.md`) and the PR-body tag decides inclusion, so the edit is unjustified churn. - `tests/non_utf8_symlink.rs` imports `std::os::unix` without a file-level `#![cfg(unix)]`. It would not compile on a non-unix target; that has no effect today because CI and the release targets are Linux and macOS only. The summary is accurate. ## Candidate B: overall 6/10 The core fix is right, but B also reordered `ward_directory`. The `NotInitialized` and `AlreadyInitialized` checks now run after the full `compute_status(WardUpdate)` scan, so the link is named even in those cases. That scan checksums new files. | Requirement | Verdict | Evidence | | ---------------------------------------------------------------- | ------- | --------------------------------------------------------------------------- | | All commands exit 255 (incl. dry-run, --allow-init) | met | All exit 255 naming the link, even plain `update` on an uninitialized tree | | Retargeted tracked link rejected | met | Even `init` on an initialized tree names the link | | Error names the link, says not UTF-8 | met | `symlink target is not UTF-8: ".../sub/link"` | | Nothing written before failure | met | Verified | | Valid UTF-8 targets still work | met | Verified | | No lossy conversion | met | Only `to_str()` | Bugs: - **major (verified):** cheap precondition failures now checksum the whole tree first. In an uninitialized directory with a 1 GiB file, `treeward update` took 52 s to report "Not initialized"; A and C took 0.01 s. `init` on an initialized tree with a new 1 GiB file took 48 s to report "Already initialized". Running `update` in the wrong large directory now hashes all of it. - **minor (verified):** the error-precedence change is undocumented. In an uninitialized tree with an unreadable file, `update` now reports `Checksum error: Permission denied` instead of "Not initialized". SPEC.md does not cover this. Scope: SPEC.md updated for the symlink rule but not for the precedence change. There is no unit test for `list_directory`. The CLI test matrix is good and also checks mtimes. The summary admits "spending the scan cost" but calls it a read-only traversal, which understates that it is full checksumming. It does not mention the masking of other errors. ## Candidate C: overall 8/10 Minimal fix, equivalent to the reference. It is the only candidate whose SPEC entry states that the init-state checks keep precedence, which is exactly what the binary does. Its tests are thinner: two unit tests (rejection, and a non-ASCII dangling target listed unchanged) and one CLI test for `init` and `init --dry-run`. | Requirement | Verdict | Evidence | | ---------------------------------------------------------------- | ------- | --------------------------------------------------------------------------- | | All commands exit 255 (incl. dry-run, --allow-init) | met | Reproduced; NotInitialized precedence documented in SPEC | | Retargeted tracked link rejected | met | Verified, wards unchanged | | Error names the link, says not UTF-8 | met | `symlink ".../sub/link" has a target that is not valid UTF-8; ...` | | Nothing written before failure | met | Verified | | Valid UTF-8 targets still work | met | Verified | | No lossy conversion | met | Only `to_str()` | Bugs: none found. Scope: no unrequested changes. The summary is candid that status, verify and update have no committed CLI tests and were checked only by hand. It mislabels the per-suite test counts in a trivial way, but the total (218) is correct. ## Ranking A > C > B The design was identical across all three, so the ranking comes from the other changes. B's reorder in `update.rs` brought in a real performance regression and an undocumented change in which error wins, all to name the link in two cases where the command fails anyway. A and C both keep the existing precedence. A wins on test depth. C wins on scope hygiene and SPEC precision, because A hand-edited the generated CHANGELOG and left its test file without a cfg gate.