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 3 commits 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.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdded ChangesContributor security guidance
Estimated code review effort: 1 (Trivial) | ~3 minutes Possibly related issues
Suggested labels: Suggested reviewers: 🚥 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
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.
Scope the denial-test guidance to each route's actual authorization rule instead of a blanket non-owner assertion, since several documented routes (star_repo, replica actions, bounty actions, author-based closing) intentionally authorize non-owners. Name the known_ungated exceptions (list_webhooks, list_replicas, list_protected_branches) instead of implying full repo-scoped read coverage. Describe the listed jobs as workflow checks that run on every PR rather than merge-blocking CI, matching the live main branch ruleset (one required review, no required status checks).
|
Fixed in 8397a4a:
|
Three rules a patch author cannot recover from the code alone, all three found by review rounds on #224 and #236: Anchor the verifying key outside the artifact. Deriving it from a field of the object under verification proves self-consistency, not authenticity, and identities here are permissionless. Found on #236 in the gl client and again on #224 in the server-side twin, where a self-signed certificate verified against an unauthenticated route. Drive the accepting verdict in a test. #224's verify endpoint shipped with sixteen tests, every one asserting the invalid path, which left it unable to observe a forged artifact being accepted. Treat a signed payload's field set as a versioned format. #224 grew it from seven fields to thirteen with no discriminator and no fallback, so every certificate issued before it now reports as tampered.
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 forged-certificate case a rejection test
AGENTS.md:25
This requires a handler to returnvalidfor a well-formed but forged artifact, even though the preceding rule correctly says that an artifact self-signed by the DID it names must fail the independent issuer-anchor check. It also reverses the test rationale: invalid-only cases miss an implementation that rejects everything, not one that accepts everything. Following this guidance can make a verification regression test bless the exact untrusted certificate it needs to catch. Require a trusted, anchored artifact for the positive case and assert rejection for the fully populated forged artifact. -
[P2] Rebase the guide and remove the retired ungated-handler exception
AGENTS.md:7
This is stale-base documentation drift: the branch predates the #94/#113 fixes, but it will add this guide cleanly to the current base.authz_guard::every_repo_scoped_handler_is_gatednow has an emptyknown_ungatedlist, and its required-gate rows coverlist_webhooks,list_replicas, andlist_protected_branches(list_webhooksalso has the owner gate). Leaving the exception here tells future contributors that those private-repository metadata surfaces are deliberately unprotected, contradicting the protections now onmainand inviting their removal. Rebase and update the guide to describe the current gates.
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