docs(agents): Codify recurring review blockers in AGENTS.md - #857
docs(agents): Codify recurring review blockers in AGENTS.md#857euxaristia wants to merge 3 commits into
Conversation
Capture the high-frequency classes that drive multi-round maintainer reviews (stale bases, path canonicalization, secret handling, partial setup, missing regression tests, scope honesty) so coding agents fix them before first review. Refs Gitlawb#856
|
Warning Review limit reached
Next review available in: 58 minutes Your organization has reached its usage spending cap. Adjust your spending cap in the billing tab. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
WalkthroughAGENTS.md adds a “Common Review Blockers” section. It covers fresh bases, cross-platform paths, security boundaries, atomic shared-state updates, regression tests, and accurate pull-request scope. ChangesReview Guidance
Estimated code review effort: 1 (Trivial) | ~3 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 91-93: Update the “Atomic shared state” guidance in AGENTS.md to
require writing complete contents to a temporary file and then atomically
replacing the target, rather than treating exclusive creation or locking alone
as atomic replacement. Separately require serialization of the full
read-modify-write sequence for lockfiles and shared stores, while noting that
reader protection depends on atomic replacement.
- Around line 80-87: Update the security guidance in the “Security edges”
section to prohibit relying on pre-open EvalSymlinks containment checks alone,
and require open/use operations that atomically bind containment, such as
no-follow or handle-relative APIs and their platform equivalents for symlinks
and reparse points.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3bdf64d2-47ca-484e-aabd-3370f7ec8737
📒 Files selected for processing (1)
AGENTS.md
Require open-time containment binding instead of pre-open EvalSymlinks, and require temp-file then atomic replace rather than exclusive create alone. Refs Gitlawb#856
|
Addressed both CodeRabbit findings in
@coderabbitai full review |
|
✅ Action performedFull review finished. |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@AGENTS.md`:
- Around line 86-89: Clarify the containment guidance to reject
final-component-only no-follow protection as insufficient. Require rooted or
handle-relative, traversal-resistant opening that prevents parent symlink or
Windows reparse-point redirection; if no-follow APIs are used, require them for
every traversed component and account for platform-specific reparse behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c230ccf2-da8b-4b6a-958a-cef4c331290b
📒 Files selected for processing (1)
AGENTS.md
There was a problem hiding this comment.
♻️ Duplicate comments (1)
AGENTS.md (1)
86-89:⚠️ Potential issue | 🟠 MajorRequire traversal-wide containment for no-follow APIs.
The current wording can permit a final-component-only implementation. On Linux,
O_NOFOLLOWdoes not prevent symlink traversal in earlier path components. Require rooted or handle-relative traversal, or explicitly require no-follow protection for every traversed component and the corresponding Windows reparse-point protections. (man7.org)Proposed wording
- Bind containment at open/use time (no-follow or handle-relative APIs, and - platform equivalents for reparse points). + Bind containment at open/use time with rooted or handle-relative, + traversal-resistant APIs. If a no-follow API is used, apply it to every + traversed component and enforce the platform's reparse-point protections; + final-component-only no-follow is insufficient.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@AGENTS.md` around lines 86 - 89, Update the containment guidance in AGENTS.md to require protection across the entire path traversal, not only the final component. Explicitly require rooted or handle-relative traversal, or no-follow protections for every traversed component, with equivalent Windows reparse-point safeguards; retain the requirement that containment is enforced at open/use time.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Duplicate comments:
In `@AGENTS.md`:
- Around line 86-89: Update the containment guidance in AGENTS.md to require
protection across the entire path traversal, not only the final component.
Explicitly require rooted or handle-relative traversal, or no-follow protections
for every traversed component, with equivalent Windows reparse-point safeguards;
retain the requirement that containment is enforced at open/use time.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1947cf8f-10ec-4024-862d-c919971ef46e
📒 Files selected for processing (1)
AGENTS.md
Final-component-only O_NOFOLLOW is insufficient on Linux and reparse points need platform protections. Align the Security edges guidance with the full-path containment requirement from review.
|
Addressed remaining CodeRabbit finding in
@coderabbitai full review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '--- Repository state ---'
git status --short
git branch --show-current
git log -1 --oneline
printf '%s\n' '--- AGENTS.md outline ---'
ast-grep outline AGENTS.md --items all || true
printf '%s\n' '--- Changed files and complete diff from merge base ---'
base="$(git merge-base HEAD origin/main 2>/dev/null || git merge-base HEAD main)"
printf 'Merge base: %s\n' "$base"
git diff --name-status "$base"...HEAD
git diff --no-ext-diff --unified=80 "$base"...HEAD -- AGENTS.md
printf '%s\n' '--- Current Common Review Blockers section with context ---'
rg -n -A80 -B8 '^## Common Review Blockers\b' AGENTS.md
printf '%s\n' '--- Whitespace validation ---'
git diff --check "$base"...HEADLength of output: 7545 🏁 Script executed: #!/bin/bash
set -u
base="$(git merge-base HEAD origin/main 2>/dev/null)"
printf '%s\n' '--- Branch ancestry ---'
if [ -n "${base:-}" ]; then
printf 'Merge base with origin/main: %s\n' "$base"
if git merge-base --is-ancestor origin/main HEAD; then
printf '%s\n' 'HEAD contains origin/main.'
else
printf '%s\n' 'HEAD does not contain origin/main.'
fi
else
printf '%s\n' 'Could not determine a merge base with origin/main.'
fi
printf '%s\n' '--- Files changed from merge base ---'
if [ -n "${base:-}" ]; then
git diff --name-only "$base"...HEAD
fi
printf '%s\n' '--- Final Common Review Blockers content ---'
awk '
/^## 4\. Common Review Blockers$/ { show=1 }
show { printf "%d:%s\n", NR, $0 }
' AGENTS.md
printf '%s\n' '--- Diff whitespace check ---'
if [ -n "${base:-}" ]; then
if git diff --check "$base"...HEAD; then
printf '%s\n' 'git diff --check passed.'
else
printf '%s\n' 'git diff --check failed.'
fi
fi
printf '%s\n' '--- Commit changed-file scope ---'
git show --format='commit %H%n%s' --name-only --no-renames HEADLength of output: 4030
No findings. The containment guidance now correctly rejects final-component-only no-follow protection and requires traversal-wide protection. The PR contains only the intended ✅ Action performedFull review finished. Your included review limit is currently reached under our Fair Usage Limits Policy. This review may still proceed through usage-based billing if eligible. Your next included review will be available in 58 minutes. |
Vasanthdev2004
left a comment
Vasanthdev2004
left a comment
There was a problem hiding this comment.
Approving. This is docs-only, the numbering follows on correctly from §3, and the guidance is accurate — I checked it against real findings rather than just reading it.
Several of these map one-to-one onto things that were live in the review queue this week, which is a good sign the section is describing the actual failure modes rather than generic advice:
- "A stale head that rolls back mainline commits is a hard blocker, not a merge-time detail" — this was a P0 on my own #808 yesterday, where the branch was 16 commits behind and would have reverted the 0.6.0 release manifest along with 134 other files.
- "final-component-only no-follow is insufficient" — I pushed a fix for exactly this today (
dd3520a6).FILE_FLAG_OPEN_REPARSE_POINTonly protects the last component, soos.MkdirAllthrough a junctioned.githad elevated setup creating objects outside the workspace before the check ever ran. - "roll back only what this run created; never destroy pre-existing resources you did not create" and "never report success when cleanup or unlock failed" — both were findings on my sandbox PRs this week, and both were real.
- "that is a check-to-use race" on
EvalSymlinks-then-open is the correct diagnosis and the right remedy.
So this isn't theoretical. Thanks for writing it up — turning a rough review stretch into something that stops the next one is a genuinely useful contribution, and it will save reviewers repeating themselves.
Two small thoughts, neither blocking, take or leave:
The atomic-replace guidance is right on POSIX but worth a Windows caveat — os.Rename over an existing file fails there if any reader holds a handle without FILE_SHARE_DELETE, so "atomically replace" needs a retry or ReplaceFile to actually hold up cross-platform. Given the section explicitly asks for platform truth, that one's a bit self-referential.
"Do not bundle unrelated fixes" is the one I'd expect to cause the most friction in practice, since it's often only visible as unrelated in hindsight. Might be worth a note that splitting is cheap before review and expensive after.
Neither needs changing before merge.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
LGTM
@euxaristia please feel free to contribute adapted versions of these to Node and OpenClaude if you would like. This a quite a good concept to help keep things more uniform for all Gitlawb projects.
3 participants
Summary
Maintainer review threads (especially multi-round ones) keep restating the same defect classes: stale merge bases that roll back mainline work, non-canonical path tests that break macOS/Windows smoke, secrets in argv or unredacted error paths, fail-open ownership checks, partial setup without safe rollback, missing regression tests, and PR claims that do not match what shipped.
This PR adds a short Common Review Blockers section to
AGENTS.mdso coding agents and contributors address those classes before the first review, without bloating the file (~1.9KB / 32 lines).Closes #856
Changes
AGENTS.mdt.TempDirpitfalls)Test plan
git diff HEAD --checkAGENTS.mdpick up the new sectionSummary by CodeRabbit