docs(agents): add AGENTS.md carrying the authorization rules PRs most often break#209
docs(agents): add AGENTS.md carrying the authorization rules PRs most often break#209beardthelion wants to merge 1 commit into
Conversation
… often break Encode the contributor-facing gate rules (the three owner-gate forms in use and the by-design non-owner exceptions, path-scoped reads and the listings contract, denial semantics, deny-test expectations, migration versioning, client denial surfacing, MSRV and the exact CI gate set) in a root AGENTS.md that coding agents load automatically, and point CONTRIBUTING.md at it. Enforcement labels state only what is on main today.
📝 WalkthroughWalkthroughAdded ChangesContributor security guidance
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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: 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`:
- Line 18: Update the gated-handler testing guidance in AGENTS.md to require
denial tests based on each route’s authorization rule, rather than universally
requiring non-owner denial. Preserve non-owner tests only where non-owners are
unauthorized, and require tests for the identities that are actually
unauthorized for routes such as star_repo, replica actions, bounty actions, and
author-based closing, including exact denial statuses and non-leaking response
bodies.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
|
|
||
| ## Adding or changing routes | ||
|
|
||
| - A new gated handler needs a test proving the gate fires: sign as a non-owner and as an anonymous caller, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Scope the denial-test requirement by authorization rule.
This blanket requirement says every new gated handler must deny a non-owner, but line 10 correctly documents routes where non-owners are valid callers (star_repo, replica actions, bounty actions, and author-based closing). Require tests for the identities that are unauthorized under each route’s rule instead.
Proposed wording
-- A new gated handler needs a test proving the gate fires: sign as a non-owner and as an anonymous caller, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape.
+- A new gated handler needs tests proving its route-specific gate fires: exercise an unauthorized authenticated caller and an anonymous caller where applicable, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape for owner-gated routes.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - A new gated handler needs a test proving the gate fires: sign as a non-owner and as an anonymous caller, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape. | |
| A new gated handler needs tests proving its route-specific gate fires: exercise an unauthorized authenticated caller and an anonymous caller where applicable, assert the exact denial status each way, and assert the body leaks nothing. The non-owner tests in `crates/gitlawb-node/src/test_support.rs` show the shape for owner-gated routes. |
🤖 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` at line 18, Update the gated-handler testing guidance in AGENTS.md
to require denial tests based on each route’s authorization rule, rather than
universally requiring non-owner denial. Preserve non-owner tests only where
non-owners are unauthorized, and require tests for the identities that are
actually unauthorized for routes such as star_repo, replica actions, bounty
actions, and author-based closing, including exact denial statuses and
non-leaking response bodies.
jatmn
left a comment
jatmn
left a comment
There was a problem hiding this comment.
I found issues that need to be addressed before this is ready.
Findings
-
[P1] Make the denial-test instruction match each route's actual authorization policy
AGENTS.md:18
This tells every new gated handler to reject a signed non-owner, but several routes documented immediately above intentionally authorize non-owners: any reader may star a readable repo, replica registration is bound to the replica DID, and bounty/close actions have their own role policies. A contributor following this mandatory rule will either assert denial for a valid request or add an incorrect owner gate to make the test pass, while potentially missing the identity that is actually unauthorized. Require denial cases based on the handler's specific authorization rule (retaining non-owner denial for owner-only routes), alongside the applicable anonymous/no-leak assertions. -
[P2] Do not describe the repo-scoped read guard as covering handlers it explicitly exempts
AGENTS.md:7
The new invariant says every repo-scoped endpoint binds a caller before serving data, butauthz_guard::every_repo_scoped_handler_is_gatedcurrently exemptslist_webhooks,list_replicas, andlist_protected_branchesinknown_ungated; each handler only looks up the repo and returns its metadata. The following description of the guard does not name those live exceptions, so this security guide presents the existing private-repo metadata exposure as protected. Either land the gates, or call out the exact tracked exceptions and keep contributors from treating those surfaces as safe. -
[P2] Correct the statement that the listed CI jobs block merges
AGENTS.md:37
The livemainruleset requires one approving review but has no required-status-check rule; the branch-protection API likewise has no required checks. The PR workflow does run the listed jobs, but a failing job does not itself block a merge under the current repository rules. Since this section is explicitly presented as CI reality, describe these as workflow checks (or make them required) rather than telling contributors they are merge-blocking.
2 participants
Adds a root
AGENTS.mdwritten for coding agents and human contributors alike, plus a pointer from CONTRIBUTING's PR guidelines. It carries only what a patch author cannot recover from the code alone: the owner-gate forms in use and the write routes that are non-owner by design, the path argument contract onauthorize_repo_read, denial semantics, the deny-test expectations for new and removed routes, migration versioning, client denial surfacing, and what CI actually blocks on vs what is convention.Two ground rules held while writing it: every named identifier, test, and CI claim was checked against current main, so the enforcement labels describe only what is merged today; and nothing already covered by README, CONTRIBUTING, SECURITY, or the PR template is repeated. The deny suite in review (#194, #195) is referenced with wording that stays true whether or not it has landed in a given checkout.
Docs-only: no runtime impact, no code or CI changes.
Summary by CodeRabbit