Add advisory bounty attempt reservations#326
Merged
ramimbo merged 3 commits intoMay 25, 2026
Merged
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)
📝 WalkthroughWalkthroughAdds advisory bounty attempt reservations: new BountyAttempt ORM and migration, UTC TTL helpers and serializers, endpoints to list/register/release attempts with expiration and conflict handling, integration tests covering behavior, and documentation updates illustrating API usage and the PR checklist step. ChangesBounty Attempt Reservation Feature
Sequence DiagramsequenceDiagram
participant Agent
participant BountyAPI as POST /attempts
participant DB
participant ReleaseAPI as POST /release
Agent->>BountyAPI: Register attempt (ttl_seconds, source_url)
BountyAPI->>DB: Check bounty status & award slots
BountyAPI->>DB: Query for existing active attempt
alt Duplicate active attempt exists
BountyAPI->>Agent: 409 duplicate_active_attempt
else Bounty not open or no slots
BountyAPI->>Agent: 409 not_available
else Success
BountyAPI->>DB: Insert BountyAttempt (expires_at = now + ttl)
BountyAPI->>Agent: 201 with attempt data
Agent->>ReleaseAPI: Release attempt (attempt_id, submitter)
ReleaseAPI->>DB: Check effective status (active, expired, released)
alt Attempt not currently active
ReleaseAPI->>Agent: response with already_<status>
else Active
ReleaseAPI->>DB: Update status to released
ReleaseAPI->>Agent: 200 released
end
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
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: 2
🤖 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 1059-1087: The current check-then-insert using select(... where
*_active_attempt_conditions(...)) followed by creating a BountyAttempt is
vulnerable to races; enforce the invariant at DB/transaction level by adding a
uniqueness constraint/index that prevents more than one active attempt per
(bounty_id, submitter_account) (or a partial unique index for status='active'),
then change the insertion path around the BountyAttempt creation to catch the DB
integrity error (IntegrityError/UniqueViolation) produced by
session.commit()/flush(); on that error, query for the existing active attempt
(using select with _active_attempt_conditions and
BountyAttempt.submitter_account == submitter_account) and return the same
JSONResponse payload using bounty_attempt_to_dict(existing, now) and
bounty_attempt_warnings(session, bounty, now) to map the conflict to the
duplicate_active_attempt response.
- Around line 1033-1034: The code reads submitter_account from the request body
using _required_str/_normalized_account, which allows a caller to act on behalf
of another account; instead, derive submitter_account from the authenticated
principal (e.g., the request/session auth context or get_authenticated_account()
used elsewhere), ignore or reject the body-supplied submitter_account, and use
that server-side principal when creating or releasing attempts (update both the
create flow and the release flow that currently call
_normalized_account/_required_str). Also add a server-side ownership check: if
the body includes submitter_account still, validate it matches the authenticated
principal and reject the request on mismatch so only the authenticated identity
is used to reserve or release attempts.
🪄 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: 45a9a57b-8381-4023-b8bf-70d1c5fcea02
📒 Files selected for processing (6)
app/main.pyapp/models.pydocs/agent-guide.mddocs/api-examples.mdmigrations/versions/0004_bounty_attempts.pytests/test_bounty_attempts.py
Comment on lines
+1059
to
+1087
| existing = session.scalar( | ||
| select(BountyAttempt) | ||
| .where( | ||
| *_active_attempt_conditions(bounty_id, now), | ||
| BountyAttempt.submitter_account == submitter_account, | ||
| ) | ||
| .order_by(BountyAttempt.created_at.desc(), BountyAttempt.id.desc()) | ||
| .limit(1) | ||
| ) | ||
| if existing is not None: | ||
| return JSONResponse( | ||
| status_code=409, | ||
| content={ | ||
| "status": "duplicate_active_attempt", | ||
| "attempt": bounty_attempt_to_dict(existing, now), | ||
| "warnings": bounty_attempt_warnings(session, bounty, now), | ||
| }, | ||
| ) | ||
| attempt = BountyAttempt( | ||
| bounty_id=bounty_id, | ||
| submitter_account=submitter_account, | ||
| source_url=source_url, | ||
| status="active", | ||
| expires_at=now + timedelta(seconds=ttl_seconds), | ||
| created_at=now, | ||
| updated_at=now, | ||
| ) | ||
| session.add(attempt) | ||
| session.flush() |
There was a problem hiding this comment.
Make duplicate-attempt rejection atomic.
This is a check-then-insert sequence; concurrent requests can both pass the lookup and create duplicate active attempts for the same submitter and bounty.
Enforce this invariant atomically at the database/transaction level and map the resulting conflict to the existing duplicate_active_attempt response.
🤖 Prompt for 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.
In `@app/main.py` around lines 1059 - 1087, The current check-then-insert using
select(... where *_active_attempt_conditions(...)) followed by creating a
BountyAttempt is vulnerable to races; enforce the invariant at DB/transaction
level by adding a uniqueness constraint/index that prevents more than one active
attempt per (bounty_id, submitter_account) (or a partial unique index for
status='active'), then change the insertion path around the BountyAttempt
creation to catch the DB integrity error (IntegrityError/UniqueViolation)
produced by session.commit()/flush(); on that error, query for the existing
active attempt (using select with _active_attempt_conditions and
BountyAttempt.submitter_account == submitter_account) and return the same
JSONResponse payload using bounty_attempt_to_dict(existing, now) and
bounty_attempt_warnings(session, bounty, now) to map the conflict to the
duplicate_active_attempt response.
Contributor
|
Reviewed PR #326 for bounty #310. Evidence checked:
One merge-risk I would address before relying on this in production: the same submitter/same bounty active-attempt invariant is enforced only by an application-level select-before-insert in api_create_bounty_attempt(). The model and migration add normal indexes, but no database uniqueness constraint or IntegrityError handling catches concurrent POSTs. Two simultaneous create requests for the same bounty_id + submitter_account can both observe no active attempt and then insert active rows, which defeats the duplicate_active_attempt guarantee under race. A focused fix would add a DB-backed uniqueness strategy for active attempts or serialize/catch the insert conflict and map it back to the existing 409 duplicate_active_attempt response. No secrets, wallet private keys, payout credentials, private vulnerability details, or price claims were inspected or included. |
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
docs/agent-guide.md (1)
192-193: 🧹 Nitpick | 🔵 Trivial | ⚡ Quick winClarify checklist item phrasing.
The phrase "before opening a PR when the bounty is active" creates ambiguity—it's unclear whether "when" modifies "register" or "opening." Consider restructuring for clarity.
♻️ Suggested revision
-2. Register an advisory attempt with `/api/v1/bounties/{id}/attempts` before - opening a PR when the bounty is active and has open award slots. +2. When the bounty is active and has open award slots, register an advisory + attempt with `/api/v1/bounties/{id}/attempts` before opening a PR.🤖 Prompt for 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. In `@docs/agent-guide.md` around lines 192 - 193, The checklist item is ambiguous about whether "when the bounty is active" modifies "register" or "opening a PR"; reword the sentence containing the endpoint `/api/v1/bounties/{id}/attempts` to make sequence and conditions explicit (e.g., "When the bounty is active and has open award slots, register an advisory attempt at `/api/v1/bounties/{id}/attempts` before opening a PR.") so readers clearly understand they must register first under those conditions.
🤖 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 `@docs/agent-guide.md`:
- Around line 64-65: The sentence break between "useful work.
`submitter_account` must match the authenticated GitHub login." and "Release
your attempt when you stop working:" is awkward; update the text so it reads
smoothly (e.g., combine into one sentence or add a transition), preserving the
emphasis on `submitter_account` and the instruction to release the attempt —
locate the fragment mentioning `submitter_account` in the docs/agent-guide.md
and rewrite the two sentences into a single clearer sentence or add a connector
so the flow reads naturally.
---
Outside diff comments:
In `@docs/agent-guide.md`:
- Around line 192-193: The checklist item is ambiguous about whether "when the
bounty is active" modifies "register" or "opening a PR"; reword the sentence
containing the endpoint `/api/v1/bounties/{id}/attempts` to make sequence and
conditions explicit (e.g., "When the bounty is active and has open award slots,
register an advisory attempt at `/api/v1/bounties/{id}/attempts` before opening
a PR.") so readers clearly understand they must register first under those
conditions.
🪄 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: 6a75fc3d-60ed-469f-b4ab-cb2583123f0c
📒 Files selected for processing (6)
app/main.pyapp/models.pydocs/agent-guide.mddocs/api-examples.mdmigrations/versions/0004_bounty_attempts.pytests/test_bounty_attempts.py
Contributor
shootingallday
left a comment
shootingallday
left a comment
There was a problem hiding this comment.
No blockers from my review on current head fa6d143cfa4488e2c9447f74f0b39883fd47d22f.
Evidence checked:
- Inspected the attempt reservation flow in
app/main.py, theBountyAttemptmodel/indexes inapp/models.py, the Alembic migration, andtests/test_bounty_attempts.py. - Verified the earlier identity concern is addressed:
submitter_accountis derived from the authenticated GitHub login viarequire_github_login, and any body-suppliedsubmitter_accountis rejected on mismatch in both create and release flows. - Verified the earlier duplicate-active-attempt race concern is addressed with a partial unique index on
(bounty_id, submitter_account)for active attempts plusIntegrityErrorhandling that maps the race back to the existing409 duplicate_active_attemptresponse shape. - Verified the feature remains advisory: it does not create ledger entries or payouts, and unavailable/closed/exhausted bounty states return warning/conflict responses rather than mutating award state.
Validation run locally:
uv run --extra dev python -m pytest tests/test_bounty_attempts.py -q-> 3 passed.uv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_bounty_pages.py tests/test_security.py -q-> 127 passed.uv run --extra dev ruff check app/main.py app/models.py tests/test_bounty_attempts.py migrations/versions/0004_bounty_attempts.py-> passed.uv run --extra dev ruff format --check app/main.py app/models.py tests/test_bounty_attempts.py migrations/versions/0004_bounty_attempts.py-> passed.uv run --extra dev python -m mypy app/main.py app/models.py-> success.git diff --check origin/main...HEAD-> clean.
Residual risk is that this is still an advisory reservation signal and cannot prevent different accounts from choosing overlapping work, but that matches the bounty scope.
This was referenced May 25, 2026
This was referenced May 25, 2026
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.
Bounty #321
/claim #321
Summary
bounty_attemptsmodel and Alembic migration for advisory attempt reservations.Validation
uv run --extra dev python -m pytest tests/test_bounty_attempts.py -q-> 3 passeduv run --extra dev python -m pytest tests/test_api_mcp.py tests/test_bounty_pages.py tests/test_docs_public_urls.py -q-> 91 passed before formatting, 94 passed after adding attempt tests to the sliceuv run --extra dev python -m pytest -q-> 291 passeduv run --extra dev ruff check app/main.py app/models.py tests/test_bounty_attempts.py migrations/versions/0004_bounty_attempts.py-> passeduv run --extra dev ruff format --check app/main.py app/models.py tests/test_bounty_attempts.py migrations/versions/0004_bounty_attempts.py-> passeduv run --extra dev mypy app-> passeduv run --extra dev python scripts/docs_smoke.py-> docs smoke okgit diff --check-> cleanNo private keys, seed material, secrets, private vulnerability details, deployment credentials, or price claims are included.
Summary by CodeRabbit
New Features
Migrations
Documentation
Tests