Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink follow. Mixing Stat/Lstat in the same file would be more confusing than the minor clarity benefit. The reviewer notes this is fine and the PR is approved.
Finding #2 (ACK-NOT-VALID): Acknowledged. The ModeSymlink guard was genuinely unreachable after EvalSymlinks and its removal is correct. The comment accurately documents why. The reviewer confirms this is a deliberate, documented choice — no issue.
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the os.SameFile pattern documented in the code comment as defense-in-depth. The reviewer explicitly states "no action required." The code is correct as-is.
Test review with inline comment
TEST: ACK-NOT-VALID — test reply to thread 27980
All findings from the current HEAD round (sonnet/4814, security/4816, gpt/4817 against eb0ff3aa) have been addressed:
Thread 27980 (sonnet NIT — os.SameFile TOCTOU limitation): ACK-NOT-VALI…
Test reply with reply_to
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and that "no action is required" — code comment already calls it defense-in-depth.
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and that "no action is required" — the code comment already labels…
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and that "no action is required" — the code comment already labels it defense-in-depth. No code change needed.
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) rather than the resolved path. Showing the resolved path in errors would confuse users who passed a symlink — they would see a path they never specified. The resolved path is used for all I/O; the flag value is only used in error messages, which is the correct UX.
Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally kept here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. While Stat and Lstat are equivalent after filepath.EvalSymlinks, mixing them in the same file would create inconsistency that could confuse future readers. The deliberate choice is noted in the code comment.
Finding #2 (ACK-NOT-VALID): Acknowledged. The comment saying 'ModeSymlink can never be set' is accurate — filepath.EvalSymlinks guarantees no symlinks remain, so the old check was genuinely unreachable. Its removal was intentional and documented. No action needed.
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and that no action is required — the code comment already calls it defense-in-depth. The narrow hardlink-swap race is a theoretical limitation of the approach, not a defect introduced here.
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of os.SameFile and explicitly states no action required — the code comment already calls it defense-in-depth. No change needed. (Finding #1 from fix plan against eb0ff3aa)
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) rather than the resolved path. Showing the resolved path in errors would confuse users who passed a symlink — they would see a path they never specified. The resolved path is used for all I/O; the original flag value is only used in error messages. Correct UX, no change needed.