Extract bounty attempt reservation helpers#362
Conversation
|
Warning Review limit reached
More reviews will be available in 25 seconds. Learn how PR review limits work. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more reviews become available, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Repository UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
This PR extracts bounty-attempt helper logic from app/main.py into a dedicated app/bounty_attempts.py module and adds tests to validate key behaviors.
Changes:
- Added
app/bounty_attempts.pywith reusable helpers for attempt status, serialization, warning generation, and expiring stale attempts. - Updated
app/main.pyto use the extracted helpers instead of inline implementations. - Added tests covering effective “expired” status in dict output and expiring stale attempts + overlap warnings.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| tests/test_bounty_attempt_helpers.py | Adds tests for extracted bounty-attempt helper behavior (expiry + warnings). |
| app/main.py | Removes inline helper implementations and switches usage to app.bounty_attempts. |
| app/bounty_attempts.py | Introduces the extracted helper functions used by API and warning/expiry logic. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| def bounty_attempt_to_dict(attempt: BountyAttempt, now: datetime) -> dict[str, Any]: | ||
| return { | ||
| "id": attempt.id, | ||
| "bounty_id": attempt.bounty_id, | ||
| "submitter_account": attempt.submitter_account, | ||
| "source_url": attempt.source_url, | ||
| "status": attempt_effective_status(attempt, now), | ||
| "expires_at": as_utc(attempt.expires_at).isoformat(), | ||
| "created_at": as_utc(attempt.created_at).isoformat(), | ||
| "updated_at": as_utc(attempt.updated_at).isoformat(), | ||
| } |
| def attempt_effective_status(attempt: BountyAttempt, now: datetime) -> str: | ||
| if attempt.status == "active" and as_utc(attempt.expires_at) <= now: | ||
| return "expired" | ||
| return attempt.status |
| def bounty_attempt_warnings(session: Session, bounty: Bounty, now: datetime) -> list[str]: | ||
| warnings: list[str] = [] | ||
| awards_remaining = max(0, bounty.max_awards - bounty.awards_paid) | ||
| if bounty.status != "open": | ||
| warnings.append(f"bounty is {bounty.status}") | ||
| awards_remaining = 0 | ||
| if awards_remaining <= 0: | ||
| warnings.append("bounty has no award slots remaining") | ||
| active_count = session.scalar( | ||
| select(func.count()) | ||
| .select_from(BountyAttempt) | ||
| .where(*active_attempt_conditions(bounty.id, now)) | ||
| ) | ||
| if active_count and active_count > 1: | ||
| warnings.append(f"bounty has {active_count} active attempts") | ||
| return warnings |
Karry2019web
left a comment
Karry2019web
left a comment
There was a problem hiding this comment.
Review ✅ APPROVED
FILES INSPECTED:
app/bounty_attempts.py— new module (73 lines) withas_utc,attempt_effective_status,bounty_attempt_to_dict,active_attempt_conditions,bounty_attempt_warnings,expire_stale_bounty_attemptsapp/main.py— removed_as_utc,_attempt_effective_status,bounty_attempt_to_dict(old version),_active_attempt_conditions,bounty_attempt_warnings,expire_stale_bounty_attempts; removedupdatefrom sqlalchemy imports (now only needed in new module); replaced 8 call sites with module functionstests/test_bounty_attempt_helpers.py— new test file (108 lines), 2 focused tests
BEHAVIOR CHECKED:
as_utc: identical to original_as_utc. Naive → UTC assignment, aware → UTC conversion. No change.attempt_effective_status: identical to_attempt_effective_status. Same "active + expired_at ≤ now → expired" logic.bounty_attempt_to_dict: Notable change: parameternowis now required (datetime) instead of optional (datetime | None = Nonewith_utc_now()fallback). Verified all 8 callers passnowexplicitly — this is a safe simplification that removes the implicit fallback.active_attempt_conditions: identical to_active_attempt_conditions.bounty_attempt_warnings: identical logic — same award-remaining calculation, same status check, same active-count query. Only the reference to_active_attempt_conditionsis replaced withactive_attempt_conditions.expire_stale_bounty_attempts: identical logic — sameupdatequery with bounty_id, status, expires_at conditions; same optional submitter_account filter.updateimport moved fromapp/main.pytoapp/bounty_attempts.py— correct since main.py no longer usesupdatedirectly (verified: all SQL UPDATE operations in main.py already went through the extracted function).
TESTS VERIFIED:
test_bounty_attempt_dict_reports_effective_expired_status: Creates a BountyAttempt model object (not persisted) withexpires_atin the past. Assertsattempt_effective_statusreturns"expired"andbounty_attempt_to_dictreturns the correct dict shape with ISO-format timestamps and+00:00UTC offset. Pure unit test — no DB needed.test_bounty_attempt_helpers_expire_stale_attempts_and_warn_on_overlap: Full integration test usingsqlite_urlfixture. Creates a bounty with 3 attempts: 1 stale (alice, expired 5 min ago), 2 active (bob, carol). Callsexpire_stale_bounty_attemptswith alice's account and verifies only alice is expired. Then assertsbounty_attempt_warningscorrectly reports "2 active attempts". Good coverage of expiration + warning paths.
CODE QUALITY:
- Clean extraction: all 6 removed private helpers are exactly the logic moved to bounty_attempts.py. Zero behavioral changes (except the
nowparameter simplification which is strictly safer). - No new dependencies — only
sqlalchemy+app.models. from __future__ import annotationsconsistent with repo convention.- Narrow
updateimport only in the module that needs it. - Tests are deterministic (no
time.sleep, no flaky fixtures). - The
as_utchelper is small but useful — it was fragmented across callers and now has a focused home.
SUMMARY: Clean extraction with excellent test coverage. The now parameter tightening from optional to required is a modest but correct API improvement since every caller already provides it. APPROVED — no blockers.
tolga-tom-nook
left a comment
tolga-tom-nook
left a comment
There was a problem hiding this comment.
Reviewed PR #362 at head cf6efb3 for Bounty #320 / review round #340.
Files inspected: app/bounty_attempts.py, app/main.py, and tests/test_bounty_attempt_helpers.py. The extraction keeps the route-level auth/validation/session flow in app.main while moving status derivation, active-attempt predicates, warning construction, serialization, and stale-attempt expiry into a small helper module. I checked the call sites for list/create/release paths and the helper tests cover naive/aware UTC serialization, effective expired status, scoped stale expiry, and overlap warnings.
Validation run locally on this head:
python3 -m pytest tests/test_bounty_attempt_helpers.py tests/test_api_mcp.py -q-> 78 passedpython3 -m ruff check app/bounty_attempts.py tests/test_bounty_attempt_helpers.py-> passedpython3 -m mypy app/bounty_attempts.py-> passed
No blocker found. One small non-blocking note: I intentionally included tests/test_api_mcp.py in the focused run because the PR leaves app.main wiring intact and this catches accidental MCP/app-construction regressions around the changed import surface.
carpedkm
left a comment
carpedkm
left a comment
There was a problem hiding this comment.
I found one helper-boundary issue in the extraction.
attempt_effective_status() normalizes attempt.expires_at with as_utc(), but it compares that aware datetime directly to the caller-provided now. Because now is now part of the helper API, a direct helper caller that passes a naive UTC datetime gets a runtime TypeError instead of the old effective-status behavior.
Repro on current head cf6efb3:
attempt_effective_status TypeError can't compare offset-naive and offset-aware datetimes
bounty_attempt_to_dict TypeError can't compare offset-naive and offset-aware datetimes
The route callers currently pass an aware _utc_now(), so this is not showing up in the route tests. But the extraction makes the helper directly importable/tested, and it already has as_utc() specifically to normalize datetime inputs. I would normalize now inside attempt_effective_status() before comparing, and either do the same for active_attempt_conditions() or explicitly document/enforce aware now at that boundary. A focused regression can pass datetime(2026, 5, 26, 12, 0) as now and assert the effective status stays stable.
Validation I ran:
. .venv/bin/activate && python -m pytest tests/test_bounty_attempt_helpers.py tests/test_api_mcp.py -q-> 78 passed.. .venv/bin/activate && python -m ruff check app/bounty_attempts.py tests/test_bounty_attempt_helpers.py-> passed.. .venv/bin/activate && python -m mypy app/bounty_attempts.py app/main.py-> success.git diff --check origin/main...HEAD-> clean.
No secrets, wallet private keys, payout credentials, private vulnerability details, deployment values, or MRWK price claims are included.
|
Maintainer note after #320 filled: this PR still references closed bounty #320. If you want it considered for the current code-health round, rebase against main, update the bounty reference to #377, and keep the scope distinct from already accepted #328, #337, #364, #375, and #376. This is not accepted or paid as-is. |
Refs #320
Summary
app/main.pyintoapp/bounty_attempts.py.Complexity reduced
app/main.pyno longer owns the attempt-reservation helper logic used by both REST and MCP paths. The attempt-specific rules now live beside each other in a small module with direct tests, making future reservation behavior changes easier to review without scanning the main app wiring.Validation
python -m pytest-> 337 passedpython -m ruff format --check .-> 50 files already formattedpython -m ruff check .-> All checks passedpython -m mypy app-> Success: no issues found in 15 source files