Rodin rodin
  • Joined on 2026-04-23
rodin commented on pull request rodin/review-bot#152 2026-05-16 00:22:07 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

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 on pull request rodin/review-bot#152 2026-05-16 00:18:43 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

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 on pull request rodin/review-bot#152 2026-05-16 00:18:38 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

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 on pull request rodin/review-bot#152 2026-05-16 00:18:32 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

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 on pull request rodin/review-bot#152 2026-05-16 00:18:27 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

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 on pull request rodin/review-bot#152 2026-05-16 00:14:16 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Finding #2 (ACK-NOT-VALID): Acknowledged. The comment saying 'ModeSymlink can never be set' is accurate — EvalSymlinks guarantees no symlinks remain, so that check was genuinely unreachable. The removal was intentional and documented. No action needed.

rodin commented on pull request rodin/review-bot#152 2026-05-16 00:14:16 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Finding #4 (ACK-NOT-VALID): Acknowledged. Lstat is intentionally used here for consistency with checkStaleDocs, which also uses Lstat to avoid follow-on symlink semantics. Switching to Stat post-EvalSymlinks would be equivalent at runtime but would create an inconsistency across the file that could confuse future readers. Keeping Lstat is the more conservative and consistent choice.

rodin commented on pull request rodin/review-bot#152 2026-05-16 00:14:16 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Finding #5 (ACK-NOT-VALID): Acknowledged. User-facing error messages intentionally reference the original --docmap flag value, not the resolved path. Showing the resolved path in errors would confuse users who passed a symlink — they'd see a path they never specified. Using the flag value is the correct UX here.

rodin commented on pull request rodin/review-bot#152 2026-05-16 00:14:06 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

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

rodin commented on pull request rodin/review-bot#152 2026-05-15 23:33:57 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Fix Plan against eb0ff3aa69f152dd995de91c88227d3e32ac2917:

Addressing bot-review findings from the latest round (sonnet/4814, security/4816, gpt/4817) — all evaluated against eb0ff3aa:

rodin commented on pull request rodin/review-bot#152 2026-05-15 23:30:31 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Self-Review: PR #152

Self-review against eb0ff3aa69f152dd995de91c88227d3e32ac2917

Phase 1: Independent Findings

None — diff looks clean.

Reviewed: validateDocmapPath signature change…

rodin pushed to issue-150 at rodin/review-bot 2026-05-15 23:23:20 +00:00
eb0ff3aa69 nit(#150): clarify why resolved != symlinkPath in InRepoSymlinkAllowed test
c76e7dcd2e fix(#150): add os.SameFile check after open to close Lstat→open TOCTOU window
Compare 2 commits »
rodin commented on pull request rodin/review-bot#152 2026-05-15 23:22:09 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Fix Plan against d6bab7a9cf6ce3d084d8f40a91a51c1a8fc084e7:

Addressing bot-review findings from the current HEAD round (sonnet/4810, security/4812, gpt/4813):

rodin commented on pull request rodin/review-bot#152 2026-05-15 23:18:01 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Self-Review: PR #152

Self-review against d6bab7a9cf6ce3d084d8f40a91a51c1a8fc084e7

Phase 1: Independent Findings

rodin commented on pull request rodin/review-bot#152 2026-05-15 23:11:36 +00:00
fix(#150): add EvalSymlinks to validateDocmapPath — close dir-symlink bypass

Fix Plan against d6bab7a9cf6ce3d084d8f40a91a51c1a8fc084e7

Addressing self-review findings from 345f9a5 (all 3 findings resolved):

rodin pushed to issue-150 at rodin/review-bot 2026-05-15 23:11:21 +00:00
d6bab7a9cf fix(#150): close residual TOCTOU with LimitedReader at docmap open
4359518e50 nit(#150): report original --docmap flag value in parse error, not resolved path
6e11107c77 nit(#150): fix misleading 'this is unreachable' in Lstat comment
Compare 3 commits »
rodin commented on pull request rodin/review-bot#158 2026-05-15 22:50:08 +00:00
fix(#157): add never-close constraint to spec, S9 invariant, and regression test

Self-review against ec6fdbff4290a7fbf9fc7277aebabc4e7d5dc02d

Assessment: Clean

Verification at ec6fdbff

Finding #1 (Sonnet NIT) — §9 prose run-on: Fixed. Commit splits the final §9…

rodin commented on pull request rodin/review-bot#158 2026-05-15 22:43:42 +00:00
fix(#157): add never-close constraint to spec, S9 invariant, and regression test

Fix Plan against ec6fdbff4290a7fbf9fc7277aebabc4e7d5dc02d:

  • Sonnet #1 (NIT): paragraph break in §9
  • GPT #1 (MINOR): clarify §8 S8/S10 wording
rodin commented on pull request rodin/review-bot#158 2026-05-15 22:43:41 +00:00
fix(#157): add never-close constraint to spec, S9 invariant, and regression test

Fix Plan against ec6fdbff4290a7fbf9fc7277aebabc4e7d5dc02d:

Acknowledging Finding #1 (sonnet, NIT) and Finding #1 (gpt, MINOR). Will adjust §8 wording (S8/S10 clarity) and add a paragraph break…