fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass #152

Merged
rodin merged 8 commits from issue-150 into main 2026-05-18 19:09:30 +00:00
2 changed files with 43 additions and 1 deletions
Showing only changes of commit 345f9a5aac - Show all commits
+1 -1
View File
10
@@ -61,7 +61,7 @@ func validateDocmapPath(localPath, resolvedRoot string) (string, error) {
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 explicitly states 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.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change 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 labels it defense-in-depth. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change needed.
Review

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. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change 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 labels it defense-in-depth. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. No action needed.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. No action needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. No action needed.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. Deliberate choice, no change needed.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. Deliberate choice, no change needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. Deliberate choice, no change needed.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. Deliberate choice, no change needed.
Review

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.

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

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.

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

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 #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)
Review

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 #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)
Review

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

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

test reply

test reply
Review

test reply

test reply
Review

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.

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

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.

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

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

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

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

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

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference `*docmapFlag` (the original `--docmap` value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.
Review

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference `*docmapFlag` (the original `--docmap` value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.
Review

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

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent but would create an inconsistency within the file.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent but would create an inconsistency within the file.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent but would create an inconsistency within the file.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent but would create an inconsistency within the file.
Review

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 specified a symlink — they want to see the path they actually provided.

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 specified a symlink — they want to see the path they actually provided.
Review

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 specified a symlink — they want to see the path they actually provided.

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 specified a symlink — they want to see the path they actually provided.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of os.SameFile and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of `os.SameFile` and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of os.SameFile and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of `os.SameFile` and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 #1 (ACK-NOT-VALID):** Acknowledged. The reviewer explicitly states 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.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 #1 (ACK-NOT-VALID):** Acknowledged. The reviewer explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink check removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.

**Finding #2 (ACK-NOT-VALID):** Acknowledged. The reviewer confirms no issue — the `ModeSymlink` check removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and this is a deliberate documented choice. No action needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink check removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.

**Finding #2 (ACK-NOT-VALID):** Acknowledged. The reviewer confirms no issue — the `ModeSymlink` check removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and this is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.

**Finding #4 (ACK-NOT-VALID):** `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.
Review

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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.

**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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.

**Finding #4 (ACK-NOT-VALID):** `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.
Review

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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.

**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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.
Review

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 change 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 labels it defense-in-depth. No change needed.
Review

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 change 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 labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the `os.SameFile` pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the `os.SameFile` pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.
Review

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

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

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. (Ref: fix plan comment 27994)

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. (Ref: fix plan comment 27994)
Review

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. (Ref: fix plan comment 27994)

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. (Ref: fix plan comment 27994)
// nodes, etc.) — ParseDocMapConfig expects a plain YAML file and would
// produce a confusing error on non-regular entries.
Review

[NIT] The comment on the Lstat call says 'ModeSymlink can never be set here; this is unreachable' — if it's truly unreachable, the symlink check was removed but the comment is slightly misleading since the Lstat itself is still reached (it's only the symlink mode check that's unreachable). The comment accurately documents the reasoning, but the phrasing 'this is unreachable' could be confused as referring to the Lstat rather than the removed symlink check. A minor clarity issue, not a correctness concern.

**[NIT]** The comment on the Lstat call says 'ModeSymlink can never be set here; this is unreachable' — if it's truly unreachable, the symlink check was removed but the comment is slightly misleading since the Lstat itself is still reached (it's only the symlink mode check that's unreachable). The comment accurately documents the reasoning, but the phrasing 'this is unreachable' could be confused as referring to the Lstat rather than the removed symlink check. A minor clarity issue, not a correctness concern.
if !fi.Mode().IsRegular() {
return fmt.Errorf("docmap must be a regular file")
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change 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 labels it defense-in-depth. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. No action needed.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. Deliberate choice, no change needed.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. Deliberate choice, no change needed.
Review

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.

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

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 #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)
Review

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

test reply

test reply
Review

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.

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

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

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

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference `*docmapFlag` (the original `--docmap` value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.
Review

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent but would create an inconsistency within the file.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent but would create an inconsistency within the file.
Review

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 specified a symlink — they want to see the path they actually provided.

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 specified a symlink — they want to see the path they actually provided.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of os.SameFile and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of `os.SameFile` and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 #1 (ACK-NOT-VALID):** Acknowledged. The reviewer explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink check removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.

**Finding #2 (ACK-NOT-VALID):** Acknowledged. The reviewer confirms no issue — the `ModeSymlink` check removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and this is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.

**Finding #4 (ACK-NOT-VALID):** `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.
Review

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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.

**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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.
Review

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 change 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 labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the `os.SameFile` pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.
Review

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

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. (Ref: fix plan comment 27994)

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. (Ref: fix plan comment 27994)
return "", fmt.Errorf("docmap must be a regular file")
Review

[NIT] The comment on the Lstat call says 'ModeSymlink can never be set' after EvalSymlinks, but the old defense-in-depth fi.Mode()&os.ModeSymlink != 0 check was removed. The comment is accurate and the removal is correct (the check was genuinely unreachable), but removing it does reduce defense-in-depth. Given the comment's explicit acknowledgment this is a deliberate and documented choice — no issue.

**[NIT]** The comment on the Lstat call says 'ModeSymlink can never be set' after EvalSymlinks, but the old defense-in-depth `fi.Mode()&os.ModeSymlink != 0` check was removed. The comment is accurate and the removal is correct (the check was genuinely unreachable), but removing it does reduce defense-in-depth. Given the comment's explicit acknowledgment this is a deliberate and documented choice — no issue.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change 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 labels it defense-in-depth. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. No action needed.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. Deliberate choice, no change needed.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. Deliberate choice, no change needed.
Review

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.

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

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 #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)
Review

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

test reply

test reply
Review

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.

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

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

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

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference `*docmapFlag` (the original `--docmap` value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.
Review

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent but would create an inconsistency within the file.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent but would create an inconsistency within the file.
Review

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 specified a symlink — they want to see the path they actually provided.

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 specified a symlink — they want to see the path they actually provided.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of os.SameFile and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of `os.SameFile` and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 #1 (ACK-NOT-VALID):** Acknowledged. The reviewer explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink check removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.

**Finding #2 (ACK-NOT-VALID):** Acknowledged. The reviewer confirms no issue — the `ModeSymlink` check removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and this is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.

**Finding #4 (ACK-NOT-VALID):** `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.
Review

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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.

**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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.
Review

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 change 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 labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the `os.SameFile` pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.
Review

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

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. (Ref: fix plan comment 27994)

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. (Ref: fix plan comment 27994)
}
Review

[NIT] After EvalSymlinks, using os.Lstat is equivalent to os.Stat since symlink components are resolved; keeping Lstat is fine but could be simplified to Stat for clarity.

**[NIT]** After EvalSymlinks, using os.Lstat is equivalent to os.Stat since symlink components are resolved; keeping Lstat is fine but could be simplified to Stat for clarity.
// Confine to resolvedRoot: use the fully-resolved path so that a directory
5
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 explicitly states 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.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. The comment phrasing is slightly imprecise but does not affect correctness.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.

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. Using the flag value is the correct UX. The resolved path is used internally for all I/O; the flag value is only used in error messages.
Review

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. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change 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 labels it defense-in-depth. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change needed.
Review

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. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change 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 labels it defense-in-depth. The narrow hardlink-swap window is a theoretical limitation of this approach, not a defect introduced here. No change needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. No action needed.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. No action needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice. No action needed.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and the removal is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. Deliberate choice, no change needed.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. Deliberate choice, no change needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create a mixed Stat/Lstat pattern in the same file that could confuse future readers. Deliberate choice, no change needed.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent at runtime but would create a mixed `Stat`/`Lstat` pattern in the same file that could confuse future readers. Deliberate choice, no change needed.
Review

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.

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

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.

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

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 #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)
Review

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 #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)
Review

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

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

test reply

test reply
Review

test reply

test reply
Review

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.

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

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.

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

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

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

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

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

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference `*docmapFlag` (the original `--docmap` value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.
Review

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference *docmapFlag (the original --docmap value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.

Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference `*docmapFlag` (the original `--docmap` value) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.
Review

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

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

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after EvalSymlinks), the comment is accurate, and the removal is a deliberate documented choice.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent but would create an inconsistency within the file.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent but would create an inconsistency within the file.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent but would create an inconsistency within the file.

Finding #4 (ACK-NOT-VALID): `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent but would create an inconsistency within the file.
Review

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 specified a symlink — they want to see the path they actually provided.

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 specified a symlink — they want to see the path they actually provided.
Review

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 specified a symlink — they want to see the path they actually provided.

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 specified a symlink — they want to see the path they actually provided.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of os.SameFile and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of `os.SameFile` and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of os.SameFile and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of `os.SameFile` and that "no action is required" — the code comment already labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 #1 (ACK-NOT-VALID):** Acknowledged. The reviewer explicitly states 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.
Review

Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states 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 #1 (ACK-NOT-VALID):** Acknowledged. The reviewer explicitly states 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.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink check removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.

**Finding #2 (ACK-NOT-VALID):** Acknowledged. The reviewer confirms no issue — the `ModeSymlink` check removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and this is a deliberate documented choice. No action needed.
Review

Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink check removal is correct (the check was genuinely unreachable after filepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.

**Finding #2 (ACK-NOT-VALID):** Acknowledged. The reviewer confirms no issue — the `ModeSymlink` check removal is correct (the check was genuinely unreachable after `filepath.EvalSymlinks`), the comment is accurate, and this is a deliberate documented choice. No action needed.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.

**Finding #4 (ACK-NOT-VALID):** `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.
Review

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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.

**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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.
Review

Finding #4 (ACK-NOT-VALID): os.Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid implicit symlink-follow semantics. Switching to Stat post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.

**Finding #4 (ACK-NOT-VALID):** `os.Lstat` is intentionally used here for consistency with `checkStaleDocs`, which also uses `Lstat` to avoid implicit symlink-follow semantics. Switching to `Stat` post-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.
Review

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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.

**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 would confuse users who specified a symlink — they would see a path they did not specify. Accepted UX tradeoff.
Review

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 change 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 labels it defense-in-depth. No change needed.
Review

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 change 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 labels it defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the `os.SameFile` pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.
Review

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the os.SameFile pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.

Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the `os.SameFile` pattern and states "no action is required." The code comment already labels this guard as defense-in-depth. No change needed.
Review

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

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

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. (Ref: fix plan comment 27994)

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. (Ref: fix plan comment 27994)
Review

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. (Ref: fix plan comment 27994)

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. (Ref: fix plan comment 27994)
+42
View File
3
@@ -649,3 +649,45 @@ mappings:
t.Errorf("expected exit 0 for './' prefixed covered file, got %d; stderr: %q", code, stderr)
}
}
// TestValidateDocmapPath_InRepoSymlinkAllowed verifies that an in-repo
// file-level symlink whose resolved target is still within the repo root is
// accepted. This is the positive case for the issue #150 behavioral change:
// only symlinks that escape the root are rejected; intra-repo symlinks are
// allowed because EvalSymlinks resolves the target and the confinement check
// is applied to the resolved path, not the symlink entry itself.
func TestValidateDocmapPath_InRepoSymlinkAllowed(t *testing.T) {
dir := t.TempDir()
// Create the real docmap file inside the repo root.
if err := os.MkdirAll(filepath.Join(dir, ".review-bot"), 0o755); err != nil {
t.Fatalf("MkdirAll: %v", err)
}
realDocmap := filepath.Join(dir, ".review-bot", "doc-map-real.yml")
if err := os.WriteFile(realDocmap, []byte("mappings: []\n"), 0o644); err != nil {
t.Fatalf("WriteFile: %v", err)
}
// Create a symlink inside the repo root that points to the real file
// (also inside the root).
symlinkPath := filepath.Join(dir, ".review-bot", "doc-map-link.yml")
if err := os.Symlink(realDocmap, symlinkPath); err != nil {
t.Skipf("cannot create symlink (platform may not support it): %v", err)
}
// Resolve dir to a symlink-free root, as runValidateDocmap does.
resolvedRoot, err := filepath.EvalSymlinks(dir)
if err != nil {
t.Fatalf("EvalSymlinks(dir): %v", err)
}
// In-repo symlink whose target is within root: must be accepted.
resolved, err := validateDocmapPath(symlinkPath, resolvedRoot)
if err != nil {
t.Fatalf("expected in-repo symlink to be accepted, got error: %v", err)
}
// The returned resolved path must be the real file (not the symlink entry).
if resolved == symlinkPath {
t.Errorf("expected resolved path to differ from symlink path")
}
}
Review

[NIT] The new TestValidateDocmapPath_InRepoSymlinkAllowed test checks if resolved == symlinkPath to verify that the returned path differs from the symlink. On some systems (e.g., macOS where /tmp is a symlink to /private/tmp), t.TempDir() already returns an EvalSymlinks-resolved path, but symlinkPath is constructed via filepath.Join(dir, ...) which may not yet be resolved. The comparison is reliable here because validateDocmapPath calls EvalSymlinks internally, so resolved will always differ from symlinkPath (a symlink entry). The assertion is correct but a comment clarifying why they must differ would help future readers.

**[NIT]** The new `TestValidateDocmapPath_InRepoSymlinkAllowed` test checks `if resolved == symlinkPath` to verify that the returned path differs from the symlink. On some systems (e.g., macOS where `/tmp` is a symlink to `/private/tmp`), `t.TempDir()` already returns an EvalSymlinks-resolved path, but `symlinkPath` is constructed via `filepath.Join(dir, ...)` which may not yet be resolved. The comparison is reliable here because `validateDocmapPath` calls `EvalSymlinks` internally, so `resolved` will always differ from `symlinkPath` (a symlink entry). The assertion is correct but a comment clarifying why they must differ would help future readers.