Extract bounty attempt routes from app main#337
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
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 (2)
📝 WalkthroughWalkthroughThis PR refactors bounty attempt functionality from ChangesBounty Attempt Module Extraction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
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/bounty_attempts.py`:
- Around line 45-46: bounty_attempt_to_dict currently assigns now = now or
_utc_now() but does not ensure injected now is timezone-aware UTC, causing
TypeError when _attempt_effective_status uses _as_utc(attempt.expires_at) <=
now; update bounty_attempt_to_dict to normalize any provided now via _as_utc (or
convert naive datetimes to UTC) so the value passed into
_attempt_effective_status is always an aware UTC datetime; reference the
functions bounty_attempt_to_dict, _attempt_effective_status, _as_utc, and
_utc_now when making the change.
🪄 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: c9496cee-19be-4de9-945f-802e0a890e50
📒 Files selected for processing (3)
app/bounty_attempts.pyapp/main.pytests/test_bounty_attempts.py
MolhamHamwi
left a comment
MolhamHamwi
left a comment
There was a problem hiding this comment.
Reviewed the extraction on head 079c38b. Evidence: checked app/bounty_attempts.py against the removed app/main.py routes, verified the same validation paths are still wired through register_bounty_attempt_routes, inspected tests/test_bounty_attempts.py for serializer coverage, and confirmed GitHub's Quality/readiness check is green. I did not find a blocking issue in the moved code; local pytest could not run here because this runner only has Python 3.9 while the project requires >=3.12.
|
Maintainer update: holding this with mrwk:needs-info. Please address the valid timestamp-normalization issue in app/bounty_attempts.py so injected now values are normalized before comparison with expires_at. No PR bounty payout is recorded while it is held. |
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Reviewed the bounty-attempt route extraction for #320.
What I checked:
app/bounty_attempts.py: moved attempt serialization, active/expired status calculation, warning construction, stale-attempt expiry, attempt registration, duplicate detection, list filtering, and release route behavior out ofapp/main.py.app/main.py: now wires the extracted route module with the same dependencies for login, JSON parsing, string/int parsing, account normalization, bounty id validation, and SQLite max id checks.tests/test_bounty_attempts.py: added direct serializer coverage for expired active attempts while preserving the existing route-level register/list/duplicate/release checks.
Validation run locally:
uv run --extra dev python -m pytest tests/test_bounty_attempts.py::test_bounty_attempt_serializer_reports_expired_effective_status tests/test_bounty_attempts.py::test_bounty_attempts_register_list_duplicate_and_release -q-> 2 passed.uv run --extra dev python -m pytest tests/test_bounty_attempts.py tests/test_api_mcp.py -q-> 77 passed.uv run --extra dev ruff check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py-> passed.uv run --extra dev ruff format --check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py-> passed.uv run --extra dev mypy app/bounty_attempts.py app/main.py-> passed.uv run --extra dev python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.
Behaviorally this looks like a clean extraction. Repository-level blocker: GitHub currently reports mergeState DIRTY, likely because main has moved after nearby bounty-attempt/MCP work. The branch needs a rebase/refresh before it can merge.
AugmentSecurity
force-pushed
the
bounty-320-admin-routes
branch
from
1a68850 to
55181b6
Compare
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/bounty_attempts.py`:
- Around line 146-147: The handlers call data = await json_object(request) which
raises a 400 on empty POST bodies; change both occurrences (the block that calls
json_object(request) before attempt_submitter_account(...) and the similar one
later) to accept empty bodies by treating missing/invalid JSON as an empty dict:
either check request.content_length/request.body and use {} when empty, or wrap
the await json_object(request) in a try/except (catching the invalid-json/error
thrown) and set data = {} on failure, then pass data into
attempt_submitter_account as before so bodyless create/release requests succeed.
🪄 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: 07a0b802-d380-4a33-8a43-fbe24e7e092c
📒 Files selected for processing (3)
app/bounty_attempts.pyapp/main.pytests/test_bounty_attempts.py
AugmentSecurity
force-pushed
the
bounty-320-admin-routes
branch
from
55181b6 to
7db500d
Compare
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/main.py`:
- Around line 27-33: Replace the private helper usage in main.py by adding a
public listing helper in app.bounty_attempts (e.g., def
list_bounty_attempts(...):) that encapsulates the filtering/formatting logic
currently using _active_attempt_conditions and _utc_now and reuses
bounty_attempt_to_dict / bounty_attempt_warnings and the same query logic used
by register_bounty_attempt_routes; then change main.py to call that new public
list_bounty_attempts from the place that invokes _call_mcp_tool(...,
name="list_bounty_attempts") instead of importing _active_attempt_conditions or
_utc_now directly.
🪄 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: 9284909f-ad86-42b9-9bdc-5cada0e84871
📒 Files selected for processing (3)
app/bounty_attempts.pyapp/main.pytests/test_bounty_attempts.py
TateLyman
left a comment
TateLyman
left a comment
There was a problem hiding this comment.
Rechecked latest head after the follow-up Encapsulate bounty attempt listing commit. The previous CodeRabbit concern about importing private attempt-listing helpers into app/main.py is addressed: main.py now calls the public list_bounty_attempts() helper from app.bounty_attempts, and that helper owns the same active/expired filtering, serialization, and warning construction used by the route module.\n\nValidation on current head c244647:\n- uv run --extra dev python -m pytest tests/test_bounty_attempts.py tests/test_api_mcp.py -q -> 81 passed.\n- uv run --extra dev python -m pytest -q -> 337 passed.\n- uv run --extra dev ruff check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py -> passed.\n- uv run --extra dev ruff format --check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py -> passed.\n- uv run --extra dev python -m mypy app/bounty_attempts.py app/main.py -> passed.\n- uv run --extra dev python scripts/docs_smoke.py -> docs smoke ok.\n- git diff --check origin/main...HEAD -> clean.\n\nNo new blocker found in the refreshed extraction. GitHub currently shows mergeState UNSTABLE only because CodeRabbit is still pending on the latest commit; hosted CI is green.
|
Maintainer update: the Resolved:
Validation:
Hosted CI is green and CodeRabbit is green on the latest push. |
ayskobtw-lil
left a comment
ayskobtw-lil
left a comment
There was a problem hiding this comment.
No blockers found on current head c244647 after the needs-info follow-up.
Evidence checked:
- Inspected
app/bounty_attempts.pyagainst the removedapp.mainattempt routes and confirmed route registration still wires list/create/release behavior through the same login, account normalization, TTL, public URL, bounty id, and SQLite integer validators. - Verified the previous timestamp-normalization concern is addressed:
bounty_attempt_to_dict()normalizes injectednow, and_attempt_effective_status()compares UTC-normalizedexpires_atvalues. - Checked
list_bounty_attempts()now provides a public helper boundary for both route and MCP usage, soapp.mainno longer imports the private active-attempt helpers. - Confirmed empty POST bodies now default to the authenticated GitHub login for both create and release paths, preserving the existing ownership checks.
Validation on this PR head:
python -m pytest tests/test_bounty_attempts.py tests/test_api_mcp.py -q-> 81 passedpython -m ruff check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py tests/test_api_mcp.py-> passedgit diff --check origin/main...HEAD-> clean
ramimbo
added
mrwk:paid
5 participants
Refs #320
Summary
app/main.pyintoapp/bounty_attempts.py.app/main.pyresponsible for app wiring and shared dependencies while the attempt reservation behavior lives behind a focused route-registration boundary.Complexity reduced
The active-attempt reservation feature was adding model-specific route logic, timestamp normalization, stale-attempt cleanup, duplicate handling, and warning construction directly to
app/main.py. This PR extracts that coherent subsystem without changing the public API, MCP behavior, ledger, wallet, webhook, admin, or payout paths.Validation
./.venv/bin/python -m pytest tests/test_bounty_attempts.py tests/test_api_mcp.py -q-> 77 passed./.venv/bin/python -m pytest -q-> 319 passed./.venv/bin/python -m ruff check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py-> passed./.venv/bin/python -m ruff format --check app/bounty_attempts.py app/main.py tests/test_bounty_attempts.py-> passed./.venv/bin/python -m mypy app/bounty_attempts.py app/main.py-> passed./.venv/bin/python scripts/docs_smoke.py-> docs smoke okgit diff --check-> cleanNo secrets, wallet private keys, payout credentials, private vulnerability details, deployment values, or MRWK price claims are included.
Summary by CodeRabbit
New Features
Tests