fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass #152
@@ -50,8 +50,8 @@ func validateDocmapPath(localPath, resolvedRoot string) (string, error) {
|
|||||||
|
|
|||||||
return "", fmt.Errorf("cannot resolve path (symlink): %w", err)
|
return "", fmt.Errorf("cannot resolve path (symlink): %w", err)
|
||||||
}
|
}
|
||||||
|
|
||||||
// Lstat the resolved path — EvalSymlinks guarantees resolvedPath is
|
// Lstat the resolved path for size and existence checks — EvalSymlinks
|
||||||
|
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
test reply test reply
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the **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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the **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.
rodin
commented
Finding #4 (ACK-NOT-VALID): **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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference **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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
test reply test reply
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the **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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the **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.
rodin
commented
Finding #4 (ACK-NOT-VALID): **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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference **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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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)
|
|||||||
// symlink-free, so ModeSymlink can never be set here; this is unreachable.
|
// guarantees no symlink components remain, so ModeSymlink can never be set.
|
||||||
|
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
test reply test reply
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the **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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the **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.
rodin
commented
Finding #4 (ACK-NOT-VALID): **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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference **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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
test reply test reply
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the **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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the **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.
rodin
commented
Finding #4 (ACK-NOT-VALID): **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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference **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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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)
|
|||||||
fi, err := os.Lstat(resolvedPath)
|
fi, err := os.Lstat(resolvedPath)
|
||||||
if err != nil {
|
if err != nil {
|
||||||
|
sonnet-review-bot
commented
[MINOR] The Lstat + ModeSymlink check after EvalSymlinks is acknowledged as 'defense-in-depth' in the comment, but **[MINOR]** The Lstat + ModeSymlink check after EvalSymlinks is acknowledged as 'defense-in-depth' in the comment, but `filepath.EvalSymlinks` already guarantees the returned path is fully resolved with no symlinks. The comment correctly explains this, but the dead check (ModeSymlink can never be set on a fully-resolved path) adds noise. It's not incorrect, just permanently unreachable code that could mislead future readers into thinking the check provides real protection here.
sonnet-review-bot
commented
[NIT] The **[NIT]** The `os.Lstat` + `ModeSymlink` check after `filepath.EvalSymlinks` is documented as 'defense-in-depth', but `filepath.EvalSymlinks` guarantees the returned path contains no symlinks — the `ModeSymlink` bit can never be set on the result. The check is harmless but the accompanying comment calling it 'defense-in-depth' is misleading because it can never fire. Consider either removing the dead check or replacing the comment with a more accurate note like '// EvalSymlinks guarantees this is unreachable; kept for belt-and-suspenders'.
|
|||||||
return "", fmt.Errorf("cannot stat file: %w", err)
|
return "", fmt.Errorf("cannot stat file: %w", err)
|
||||||
|
|||||||
|
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the ModeSymlink removal is correct (the check was genuinely unreachable after 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
test reply test reply
rodin
commented
test reply test reply
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The 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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
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.
rodin
commented
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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #4 (ACK-NOT-VALID): 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the **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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly states this is a known limitation of the **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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the **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.
rodin
commented
Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the **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.
rodin
commented
Finding #4 (ACK-NOT-VALID): **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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference **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.
rodin
commented
Finding #4 (ACK-NOT-VALID): **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.
rodin
commented
Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference **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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID — Review #4814): The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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.
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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)
rodin
commented
Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the 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 states this is a known limitation of the
os.SameFilepattern 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.SameFilepattern 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 #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.
Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent at runtime but would create a mixedStat/Lstatpattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent at runtime but would create a mixedStat/Lstatpattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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--docmapvalue) 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 #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the
os.SameFilepattern 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.SameFilepattern 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 #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.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent at runtime but would create a mixedStat/Lstatpattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent at runtime but would create a mixedStat/Lstatpattern in the same file that could confuse future readers. The deliberate choice is noted in the comment.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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--docmapvalue) 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 #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the
os.SameFilepattern 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.SameFilepattern 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 #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.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent at runtime but would create a mixedStat/Lstatpattern in the same file that could confuse future readers. Deliberate choice, no change needed.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent at runtime but would create a mixedStat/Lstatpattern in the same file that could confuse future readers. Deliberate choice, no change needed.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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--docmapvalue) 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 #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of
os.SameFileand explicitly states no action required — the code comment already calls it defense-in-depth. No change needed. (Finding #1 from fix plan againsteb0ff3aa)Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer notes this is a known limitation of
os.SameFileand explicitly states no action required — the code comment already calls it defense-in-depth. No change needed. (Finding #1 from fix plan againsteb0ff3aa)Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the
os.SameFilepattern 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.SameFilepattern and that "no action is required" — the code comment already labels it defense-in-depth. No code change needed.test reply
test reply
Finding #1 (ACK-NOT-VALID): Acknowledged. This is a known OS-level limitation of the
os.SameFilepattern 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.SameFilepattern documented in the code comment as defense-in-depth. The reviewer explicitly states "no action required." The code is correct as-is.Finding #2 (ACK-NOT-VALID): Acknowledged. The
ModeSymlinkguard was genuinely unreachable afterEvalSymlinksand 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
ModeSymlinkguard was genuinely unreachable afterEvalSymlinksand its removal is correct. The comment accurately documents why. The reviewer confirms this is a deliberate, documented choice — no issue.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink follow. MixingStat/Lstatin 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.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink follow. MixingStat/Lstatin the same file would be more confusing than the minor clarity benefit. The reviewer notes this is fine and the PR is approved.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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--docmapvalue) for user clarity — showing a resolved path the user never specified would be surprising. The reviewer notes this is acceptable. No change needed.Finding #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the
os.SameFilepattern 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.SameFilepattern and that "no action is required" — the code comment already labels it defense-in-depth. No code change needed.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.
Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent but would create an inconsistency within the file.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent but would create an inconsistency within the file.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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--docmapvalue) 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 #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of
os.SameFileand 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.SameFileand 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 states this is a known limitation of the
os.SameFilepattern 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.SameFilepattern and that "no action is required" — the code comment already labels it defense-in-depth. No code change needed.Finding #2 (ACK-NOT-VALID): Acknowledged. The reviewer confirms no issue — the
ModeSymlinkcheck removal is correct (the check was genuinely unreachable afterfilepath.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
ModeSymlinkcheck removal is correct (the check was genuinely unreachable afterfilepath.EvalSymlinks), the comment is accurate, and this is a deliberate documented choice. No action needed.Finding #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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 #4 (ACK-NOT-VALID):
os.Lstatis intentionally used here for consistency withcheckStaleDocs, which also usesLstatto avoid implicit symlink-follow semantics. Switching toStatpost-EvalSymlinks would be equivalent in practice but inconsistent with the rest of the file. Deliberate choice — no change needed.Finding #5 (ACK-NOT-VALID): User-facing error messages intentionally reference
*docmapFlag(the original--docmapvalue) 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 #1 (ACK-NOT-VALID): Acknowledged. The reviewer explicitly notes this is a known limitation of the
os.SameFilepattern 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.SameFilepattern and that "no action is required" — the code comment already labels it 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.SameFilepattern 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.SameFilepattern 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): Acknowledged. The reviewer explicitly notes this is a known limitation of the
os.SameFilepattern 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.SameFilepattern 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.SameFilepattern 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.SameFilepattern and that "no action is required" — the code comment already labels it defense-in-depth. No code change needed. (Ref: fix plan comment 27994)