Skip to content

test(scope-requests): add grant-enforcement E2E, tighten approve tolerance, add deny negative test#2108

Open
hognek wants to merge 1 commit into
jaylfc:devfrom
hognek:feat/test-1921-follow-up
Open

test(scope-requests): add grant-enforcement E2E, tighten approve tolerance, add deny negative test#2108
hognek wants to merge 1 commit into
jaylfc:devfrom
hognek:feat/test-1921-follow-up

Conversation

@hognek

@hognek hognek commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Follow-up to PR #1921. Three test gaps filled:

  1. E2E grant-enforcement test — proves an approved scope grant actually unlocks a route. Full flow: agent requests decisions_write → admin approves → agent uses the token on POST /api/decisions → 200. Would have caught Project routes ignore agent grants: an approved project_tasks scope grants nothing #2095 before it shipped.

  2. Tighten test_agent_cannot_approve_its_own_request — replaced assert resp.status_code in (401, 403) with exact 401. The middleware does not pass a registry JWT through to /approve (only the create path is allowlisted), so it falls to the session gate → 401, never 403.

  3. Deny negative test — new test_agent_cannot_deny_its_own_request asserting an agent's own registry token cannot reach the deny endpoint. Same auth model as approve.

All 3 new tests pass. 18/18 total in test_agent_scope_requests.py.

Summary by CodeRabbit

  • Bug Fixes
    • Confirmed that agents cannot approve or deny their own scope requests.
    • Ensured denied self-service requests remain pending and do not create grants.
    • Verified that approved permissions unlock access to the corresponding decisions endpoint.

…rance, add deny negative test

Three test gaps filled following PR jaylfc#1921 merge:

1. E2E grant-enforcement test (test_approved_scope_grant_unlocks_route_e2e):
   agent self-requests decisions_write → admin approves → agent uses token
   on POST /api/decisions → assert 200. Proves the full grant-enforcement
   chain is wired end to end. Regression guard for issue jaylfc#2095.

2. Tighten test_agent_cannot_approve_its_own_request:
   replace (401, 403) tolerance with exact 401. The middleware does not pass
   a registry JWT through to the approve handler (only the create path is
   allowlisted), so it falls to the session gate → 401 exactly.

3. Deny negative test (test_agent_cannot_deny_its_own_request):
   same auth model as approve — middleware does not allowlist the deny
   endpoint for registry JWTs, so an agent token on the deny endpoint
   falls through to the session gate → 401.
@hognek
hognek marked this pull request as ready for review July 22, 2026 16:04
@qodo-code-review

Copy link
Copy Markdown

Qodo reviews are paused for this user.

Troubleshooting steps vary by plan Learn more →

On a Teams plan?
Reviews resume once this user has a paid seat and their Git account is linked in Qodo.
Link Git account →

Using GitHub Enterprise Server, GitLab Self-Managed, or Bitbucket Data Center?
These require an Enterprise plan - Contact us
Contact us →

@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The scope-request tests now require registry tokens to receive 401 on approval and denial routes, verify denied requests remain pending without grants, and cover successful decisions-route access after administrative approval.

Changes

Scope request authorization

Layer / File(s) Summary
Registry token route gating
tests/test_agent_scope_requests.py
Approval and denial attempts using an agent registry token return 401; denial creates no grant and leaves the request pending.
Approved grant route access
tests/test_agent_scope_requests.py
An administrator-approved decisions_write grant allows the agent token to call /api/decisions successfully and return the expected decision fields.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

  • jaylfc/taOS#1921: Covers the related scope-request middleware allowlist and session-gated approval/denial behavior.

Suggested reviewers: jaylfc

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately reflects the added grant-enforcement E2E test and the approve/deny scope-request behavior changes.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@gitar-bot

gitar-bot Bot commented Jul 22, 2026

Copy link
Copy Markdown

Important

You are using the Gitar free plan. Upgrade to unlock code review, CI analysis, auto-apply, custom automations, and more.

Gitar

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant