Extract admin route registration [Bounty #390]#391
Merged
ramimbo merged 2 commits intoMay 26, 2026
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
💤 Files with no reviewable changes (1)
📝 WalkthroughWalkthroughAdmin routes ( ChangesAdmin Routes Modularization
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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 `@app/admin_routes.py`:
- Around line 71-82: The handler currently declares a Request parameter named
request which is immediately deleted (see the "del request" statement); remove
the unused parameter from the endpoint signature and delete the "del request"
line so the signature only contains the used parameters (e.g., repo,
issue_number, issue_url, title, reward_mrwk, max_awards, acceptance,
csrf_token_value, admin_login/Depends(require_admin)); adjust any imports only
if Request becomes unused elsewhere.
🪄 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: Repository UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: bf6a755b-2835-4e73-82be-78df42996d4a
📒 Files selected for processing (3)
app/admin_routes.pyapp/main.pytests/test_admin_routes.py
DYSfu
reviewed
May 26, 2026
Contributor
DYSfu
left a comment
DYSfu
left a comment
There was a problem hiding this comment.
Reviewed current head d0688e3 against origin/main; no blockers found.
Evidence:
- Inspected
app/admin_routes.py, theapp.mainregistration point,tests/test_admin_routes.py, and existing admin security coverage intests/test_security.py. - Verified the extracted admin route registration preserves
/admin/login,/admin/callback,/admin/logout,/admin, and/admin/bountiesbehavior, including OAuth redirect handling, admin cookie logout, CSRF validation for cookie admins, and API-token bypass behavior. - Verified the earlier CodeRabbit unused-
requestnit is already addressed ind0688e3. - GitHub quality and CodeRabbit checks are green.
Validation:
uv run --extra dev python -m pytest tests/test_admin_routes.py tests/test_security.py::test_admin_bounty_form_requires_csrf_for_cookie_auth tests/test_security.py::test_admin_page_renders_safe_webhook_events_for_cookie_admin -q-> 4 passed.uv run --extra dev python -m pytest -q-> 379 passed.uv run --extra dev python -m ruff check app/main.py app/admin_routes.py tests/test_admin_routes.py-> passed.uv run --extra dev python -m ruff format --check app/main.py app/admin_routes.py tests/test_admin_routes.py-> 3 files already formatted.uv run --extra dev python -m mypy app/main.py app/admin_routes.py-> success.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.
s0584273828-ctrl
approved these changes
May 26, 2026
Contributor
s0584273828-ctrl
left a comment
s0584273828-ctrl
left a comment
There was a problem hiding this comment.
Reviewed current PR #391 at head 61f8d27. No blockers found.
Evidence checked:
- Inspected the PR diff against current
origin/main; it is limited toapp/admin_routes.py,app/main.py, andtests/test_admin_routes.py. - Verified
register_admin_routes()moves/admin/login,/admin/callback,/admin/logout,/admin, and/admin/bountiesout ofapp.mainwhile preserving the same injected dependencies for settings, templates, admin login lookup, OAuth configuration, CSRF token generation/verification, and DB session use. - Checked the
/admin/bountiesform keeps the publiccsrf_tokenfield name viaForm(None, alias="csrf_token")while avoiding the old local-name collision. - Confirmed the new route tests cover login/callback/logout wiring and the unauthenticated
/adminOAuth-not-configured fallback.
Validation run locally on PR head:
env MERGEWORK_DATABASE_URL=sqlite:////private/tmp/mergework-pr391-review-clean.sqlite uv run --extra dev python -m pytest tests/test_admin_routes.py tests/test_admin_helpers.py -q-> 7 passedenv MERGEWORK_DATABASE_URL=sqlite:////private/tmp/mergework-pr391-review-clean2.sqlite uv run --extra dev python -m pytest -q-> 379 passeduv run --extra dev ruff check app/admin_routes.py app/main.py tests/test_admin_routes.py-> passeduv run --extra dev python -m mypy app/admin_routes.py app/main.py-> successgit diff --check origin/main...HEAD-> clean
Contributor
|
Small correction to my review note: the GitHub review is anchored on latest head |
ramimbo
added
mrwk:accepted
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
/admin/login,/admin/callback,/admin/logout,/admin, and/admin/bountiesroute registration out ofapp/main.pyintoapp/admin_routes.py.app/main.pyfocused on app wiring while passing the existing auth, OAuth, CSRF, settings, database, and template dependencies into the admin route boundary.Complexity reduced
app/main.pyno longer owns the admin HTML/form route bodies or admin-specific redirect/logout wiring. Admin page and form routing now live beside the existing admin context/form helpers, whileapp/main.pyonly registers the boundary with shared dependencies. Public admin behavior, CSRF checks, OAuth redirect behavior, and bounty creation responses are preserved.Validation
uv run --extra dev python -m ruff check .-> passed.uv run --extra dev python -m ruff format --check .-> 67 files already formatted.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.uv run --extra dev python -m mypy app-> success.uv run --extra dev python -m pytest -q-> 379 passed.git diff --check-> clean.MRWK
Bounty #390
No private keys, seed material, secrets, deployment credentials, private vulnerability details, payout credentials, or MRWK price claims are included.
Summary by CodeRabbit
New Features
Tests