MRWK bounty: advisory attempt reservations#327
Closed
duongynhi000005-oss wants to merge 1 commit into
Closed
Conversation
📝 WalkthroughWalkthroughThis PR introduces a bounty attempt system allowing submitters to register submission attempts for a bounty with automatic time-based expiry (4 hours default). Attempts transition through active/expired/released states and track submitter identity, source URLs, and branch references. The implementation spans database schema, ORM model, ledger service lifecycle management, and REST API endpoints with admin-controlled release capabilities. ChangesBounty Attempt System
Sequence DiagramsequenceDiagram
participant Client
participant API as API Handler
participant Service as Ledger Service
participant DB as Database
Client->>API: POST /bounties/{id}/attempts
API->>Service: register_bounty_attempt()
Service->>DB: expire due attempts
Service->>DB: validate bounty open & slots
Service->>DB: verify submitter uniqueness
Service->>DB: insert active attempt with expires_at
Service->>API: BountyAttempt
API->>Client: 200 (attempt + warnings)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 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: 5
🤖 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/db.py`:
- Around line 89-106: The runtime schema migration in _migrate_schema is missing
the index on expires_at used by _expire_bounty_attempts; add a CREATE INDEX IF
NOT EXISTS ix_bounty_attempts_expires_at ON bounty_attempts (expires_at) within
the same connection.execute/text block that creates the other indexes (alongside
ix_bounty_attempts_bounty_id, ix_bounty_attempts_status and
uq_bounty_attempt_active) so the runtime schema matches the Alembic migration
and expiration queries remain efficient.
In `@app/main.py`:
- Around line 964-966: The code loads all attempts via list_bounty_attempts and
then filters in Python to count active items; change this to perform the
filter/count in the DB instead. Update list_bounty_attempts (or add a new
helper) to accept a status or active-only flag (e.g., status="active") or add a
dedicated count_active_bounty_attempts(session, bounty_id) that issues a DB
query counting rows WHERE bounty_id = bounty.id AND status = 'active', then
replace the current calls around list_bounty_attempts and
result["attempts_active"] to use that DB-level filtered count to avoid loading
inactive attempts.
- Around line 988-1053: The three endpoints api_bounty_attempts,
api_register_bounty_attempt, and api_release_bounty_attempt accept raw bounty_id
integers and skip the centralized bounds/positive check; update each to validate
bounty_id with the existing _positive_bounty_id guard (either by changing the
route parameter to bounty_id: int = Depends(_positive_bounty_id) or by
explicitly calling _positive_bounty_id(bounty_id) at the start of the function)
so invalid/too-large IDs produce the same 400 behavior and still enforce
SQLITE_INTEGER_MAX and related checks used by other bounty routes.
In `@migrations/versions/0004_bounty_attempts.py`:
- Around line 27-36: The migration is missing the index for the
submitter_account column declared in the ORM; add an op.create_index call to
create "ix_bounty_attempts_submitter_account" on the "bounty_attempts" table for
the ["submitter_account"] column (alongside the existing op.create_index calls
including
"ix_bounty_attempts_bounty_id"/"ix_bounty_attempts_status"/"ix_bounty_attempts_expires_at"
and the unique "uq_bounty_attempt_active"), and update the downgrade to drop
that index (drop_index "ix_bounty_attempts_submitter_account") so Alembic
migrations match the ORM model.
In `@tests/test_api_mcp.py`:
- Around line 1763-1847: Tests are missing assertions for the new GET
/api/v1/bounties/{bounty_id} fields (attempts_active / attempt_warnings) and for
the attempts-listing active_only filter; update the two test functions
(test_bounty_attempt_api_tracks_visibility_and_warnings and
test_bounty_attempt_api_rejects_paid_bounties_and_releases_attempts) to call GET
/api/v1/bounties/{bounty_id} and assert the response includes the correct
attempts_active and attempt_warnings values after creating/releases/paying
attempts, and add a call to GET
/api/v1/bounties/{bounty_id}/attempts?active_only=true to assert only active
attempts are returned and warnings behave as expected.
🪄 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: 3509e59a-5829-45c8-ba4f-df7db627bbdb
📒 Files selected for processing (8)
app/db.pyapp/ledger/service.pyapp/main.pyapp/models.pymigrations/versions/0004_bounty_attempts.pytests/test_api_mcp.pytests/test_ledger.pytests/test_migrations.py
Comment on lines
+89
to
+106
| connection.execute( | ||
| text( | ||
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_bounty_id " | ||
| "ON bounty_attempts (bounty_id)" | ||
| ) | ||
| ) | ||
| connection.execute( | ||
| text( | ||
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_status " | ||
| "ON bounty_attempts (status)" | ||
| ) | ||
| ) | ||
| connection.execute( | ||
| text( | ||
| "CREATE UNIQUE INDEX IF NOT EXISTS uq_bounty_attempt_active " | ||
| "ON bounty_attempts (bounty_id, submitter_account, active_marker)" | ||
| ) | ||
| ) |
There was a problem hiding this comment.
Missing expires_at index in runtime migration.
The Alembic migration at line 30 creates ix_bounty_attempts_expires_at, but _migrate_schema does not. This index is used for efficient expiration queries in _expire_bounty_attempts. Consider adding it for consistency:
Proposed fix
connection.execute(
text(
"CREATE UNIQUE INDEX IF NOT EXISTS uq_bounty_attempt_active "
"ON bounty_attempts (bounty_id, submitter_account, active_marker)"
)
)
+ connection.execute(
+ text(
+ "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_expires_at "
+ "ON bounty_attempts (expires_at)"
+ )
+ )📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| connection.execute( | |
| text( | |
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_bounty_id " | |
| "ON bounty_attempts (bounty_id)" | |
| ) | |
| ) | |
| connection.execute( | |
| text( | |
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_status " | |
| "ON bounty_attempts (status)" | |
| ) | |
| ) | |
| connection.execute( | |
| text( | |
| "CREATE UNIQUE INDEX IF NOT EXISTS uq_bounty_attempt_active " | |
| "ON bounty_attempts (bounty_id, submitter_account, active_marker)" | |
| ) | |
| ) | |
| connection.execute( | |
| text( | |
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_bounty_id " | |
| "ON bounty_attempts (bounty_id)" | |
| ) | |
| ) | |
| connection.execute( | |
| text( | |
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_status " | |
| "ON bounty_attempts (status)" | |
| ) | |
| ) | |
| connection.execute( | |
| text( | |
| "CREATE UNIQUE INDEX IF NOT EXISTS uq_bounty_attempt_active " | |
| "ON bounty_attempts (bounty_id, submitter_account, active_marker)" | |
| ) | |
| ) | |
| connection.execute( | |
| text( | |
| "CREATE INDEX IF NOT EXISTS ix_bounty_attempts_expires_at " | |
| "ON bounty_attempts (expires_at)" | |
| ) | |
| ) |
🤖 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/db.py` around lines 89 - 106, The runtime schema migration in
_migrate_schema is missing the index on expires_at used by
_expire_bounty_attempts; add a CREATE INDEX IF NOT EXISTS
ix_bounty_attempts_expires_at ON bounty_attempts (expires_at) within the same
connection.execute/text block that creates the other indexes (alongside
ix_bounty_attempts_bounty_id, ix_bounty_attempts_status and
uq_bounty_attempt_active) so the runtime schema matches the Alembic migration
and expiration queries remain efficient.
Comment on lines
+964
to
+966
| attempts = list_bounty_attempts(session, bounty_id=bounty.id) | ||
| active_attempts = [attempt for attempt in attempts if attempt.status == "active"] | ||
| result["attempts_active"] = len(active_attempts) |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Avoid loading inactive attempts when only active counts are needed.
Line 964 currently pulls all attempt rows, then Line 965 filters in Python. This is avoidable work on a hot detail endpoint and grows with historical attempt volume.
⚡ Proposed fix
- attempts = list_bounty_attempts(session, bounty_id=bounty.id)
- active_attempts = [attempt for attempt in attempts if attempt.status == "active"]
+ active_attempts = list_bounty_attempts(
+ session,
+ bounty_id=bounty.id,
+ include_inactive=False,
+ )🤖 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 964 - 966, The code loads all attempts via
list_bounty_attempts and then filters in Python to count active items; change
this to perform the filter/count in the DB instead. Update list_bounty_attempts
(or add a new helper) to accept a status or active-only flag (e.g.,
status="active") or add a dedicated count_active_bounty_attempts(session,
bounty_id) that issues a DB query counting rows WHERE bounty_id = bounty.id AND
status = 'active', then replace the current calls around list_bounty_attempts
and result["attempts_active"] to use that DB-level filtered count to avoid
loading inactive attempts.
Comment on lines
+988
to
+1053
| @app.get("/api/v1/bounties/{bounty_id}/attempts") | ||
| def api_bounty_attempts(bounty_id: int, active_only: bool = Query(False)) -> dict[str, Any]: | ||
| with session_scope(db_url) as session: | ||
| bounty = session.get(Bounty, bounty_id) | ||
| if bounty is None: | ||
| raise HTTPException(status_code=404, detail="bounty not found") | ||
| attempts = list_bounty_attempts( | ||
| session, | ||
| bounty_id=bounty_id, | ||
| include_inactive=not active_only, | ||
| ) | ||
| active_count = sum(1 for attempt in attempts if attempt.status == "active") | ||
| warnings: list[str] = [] | ||
| if bounty.status != "open": | ||
| warnings.append("bounty_not_open") | ||
| if bounty.awards_paid >= bounty.max_awards: | ||
| warnings.append("no_award_slots_remaining") | ||
| if active_count > 1: | ||
| warnings.append("multiple_active_attempts") | ||
| return { | ||
| "bounty_id": bounty_id, | ||
| "active_count": active_count, | ||
| "warnings": warnings, | ||
| "attempts": [bounty_attempt_to_dict(attempt) for attempt in attempts], | ||
| } | ||
|
|
||
| @app.post("/api/v1/bounties/{bounty_id}/attempts") | ||
| async def api_register_bounty_attempt(bounty_id: int, request: Request) -> dict[str, Any]: | ||
| data = await _json_object(request) | ||
| with session_scope(db_url) as session: | ||
| try: | ||
| attempt = register_bounty_attempt( | ||
| session, | ||
| bounty_id=bounty_id, | ||
| submitter_account=_required_str(data, "submitter_account"), | ||
| source_url=_optional_str(data, "source_url") if data.get("source_url") is not None else None, | ||
| branch_ref=_optional_str(data, "branch_ref") if data.get("branch_ref") is not None else None, | ||
| expires_in_seconds=_optional_int(data, "expires_in_seconds", 14400), | ||
| ) | ||
| except KeyError as exc: | ||
| raise HTTPException(status_code=400, detail=f"{exc.args[0]} is required") from exc | ||
| except LedgerError as exc: | ||
| raise HTTPException(status_code=400, detail=str(exc)) from exc | ||
| attempts = list_bounty_attempts(session, bounty_id=bounty_id, include_inactive=False) | ||
| payload = bounty_attempt_to_dict(attempt) | ||
| payload["warnings"] = ["multiple_active_attempts"] if len(attempts) > 1 else [] | ||
| return payload | ||
|
|
||
| @app.post("/api/v1/bounties/{bounty_id}/attempts/{attempt_id}/release") | ||
| async def api_release_bounty_attempt( | ||
| bounty_id: int, | ||
| attempt_id: int, | ||
| request: Request, | ||
| admin_login: str = Depends(require_admin_token), | ||
| ) -> dict[str, Any]: | ||
| data = await _json_object(request) | ||
| released_by = _optional_str(data, "released_by", admin_login) | ||
| with session_scope(db_url) as session: | ||
| attempt = session.get(BountyAttempt, attempt_id) | ||
| if attempt is None or attempt.bounty_id != bounty_id: | ||
| raise HTTPException(status_code=404, detail="bounty attempt not found") | ||
| try: | ||
| released = release_bounty_attempt(session, attempt_id=attempt_id, released_by=released_by) | ||
| except LedgerError as exc: | ||
| raise HTTPException(status_code=400, detail=str(exc)) from exc | ||
| return bounty_attempt_to_dict(released) |
There was a problem hiding this comment.
Apply the existing path-ID validation to all attempt endpoints.
Line 989, Line 1015, and Line 1038 accept raw IDs and skip _positive_bounty_id / integer bound checks used in other bounty routes. This creates inconsistent 400/404 behavior and bypasses centralized SQLITE_INTEGER_MAX guards.
🔧 Proposed fix
`@app.get`("/api/v1/bounties/{bounty_id}/attempts")
def api_bounty_attempts(bounty_id: int, active_only: bool = Query(False)) -> dict[str, Any]:
+ bounty_id = _positive_bounty_id(bounty_id)
with session_scope(db_url) as session:
bounty = session.get(Bounty, bounty_id)
if bounty is None:
raise HTTPException(status_code=404, detail="bounty not found")
@@
`@app.post`("/api/v1/bounties/{bounty_id}/attempts")
async def api_register_bounty_attempt(bounty_id: int, request: Request) -> dict[str, Any]:
+ bounty_id = _positive_bounty_id(bounty_id)
data = await _json_object(request)
with session_scope(db_url) as session:
@@
`@app.post`("/api/v1/bounties/{bounty_id}/attempts/{attempt_id}/release")
async def api_release_bounty_attempt(
bounty_id: int,
attempt_id: int,
request: Request,
admin_login: str = Depends(require_admin_token),
) -> dict[str, Any]:
+ bounty_id = _positive_bounty_id(bounty_id)
+ if attempt_id <= 0:
+ raise HTTPException(status_code=400, detail="attempt id must be positive")
+ if attempt_id > SQLITE_INTEGER_MAX:
+ raise HTTPException(status_code=400, detail="attempt id is too large")
data = await _json_object(request)🤖 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 988 - 1053, The three endpoints
api_bounty_attempts, api_register_bounty_attempt, and api_release_bounty_attempt
accept raw bounty_id integers and skip the centralized bounds/positive check;
update each to validate bounty_id with the existing _positive_bounty_id guard
(either by changing the route parameter to bounty_id: int =
Depends(_positive_bounty_id) or by explicitly calling
_positive_bounty_id(bounty_id) at the start of the function) so
invalid/too-large IDs produce the same 400 behavior and still enforce
SQLITE_INTEGER_MAX and related checks used by other bounty routes.
Comment on lines
+27
to
+36
| ) | ||
| op.create_index("ix_bounty_attempts_bounty_id", "bounty_attempts", ["bounty_id"]) | ||
| op.create_index("ix_bounty_attempts_status", "bounty_attempts", ["status"]) | ||
| op.create_index("ix_bounty_attempts_expires_at", "bounty_attempts", ["expires_at"]) | ||
| op.create_index( | ||
| "uq_bounty_attempt_active", | ||
| "bounty_attempts", | ||
| ["bounty_id", "submitter_account", "active_marker"], | ||
| unique=True, | ||
| ) |
There was a problem hiding this comment.
Missing index on submitter_account column.
The ORM model at app/models.py:78 defines submitter_account with index=True, but this migration does not create the corresponding ix_bounty_attempts_submitter_account index. This inconsistency means the index will exist when using Base.metadata.create_all() but not when using Alembic migrations.
Proposed fix
op.create_index("ix_bounty_attempts_bounty_id", "bounty_attempts", ["bounty_id"])
+ op.create_index("ix_bounty_attempts_submitter_account", "bounty_attempts", ["submitter_account"])
op.create_index("ix_bounty_attempts_status", "bounty_attempts", ["status"])And in downgrade:
def downgrade() -> None:
op.drop_index("uq_bounty_attempt_active", table_name="bounty_attempts")
op.drop_index("ix_bounty_attempts_expires_at", table_name="bounty_attempts")
op.drop_index("ix_bounty_attempts_status", table_name="bounty_attempts")
+ op.drop_index("ix_bounty_attempts_submitter_account", table_name="bounty_attempts")
op.drop_index("ix_bounty_attempts_bounty_id", table_name="bounty_attempts")
op.drop_table("bounty_attempts")🤖 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 `@migrations/versions/0004_bounty_attempts.py` around lines 27 - 36, The
migration is missing the index for the submitter_account column declared in the
ORM; add an op.create_index call to create
"ix_bounty_attempts_submitter_account" on the "bounty_attempts" table for the
["submitter_account"] column (alongside the existing op.create_index calls
including
"ix_bounty_attempts_bounty_id"/"ix_bounty_attempts_status"/"ix_bounty_attempts_expires_at"
and the unique "uq_bounty_attempt_active"), and update the downgrade to drop
that index (drop_index "ix_bounty_attempts_submitter_account") so Alembic
migrations match the ORM model.
Comment on lines
+1763
to
+1847
| def test_bounty_attempt_api_tracks_visibility_and_warnings(sqlite_url: str) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=3213, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/3213", | ||
| title="Attempt API", | ||
| reward_mrwk="20", | ||
| max_awards=2, | ||
| acceptance="Attempt API stays advisory.", | ||
| ) | ||
| bounty_id = bounty.id | ||
|
|
||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
|
|
||
| first = client.post( | ||
| f"/api/v1/bounties/{bounty_id}/attempts", | ||
| json={"submitter_account": "github:alice", "expires_in_seconds": 60}, | ||
| ) | ||
| assert first.status_code == 200 | ||
| assert first.json()["warnings"] == [] | ||
|
|
||
| second = client.post( | ||
| f"/api/v1/bounties/{bounty_id}/attempts", | ||
| json={"submitter_account": "github:bob", "expires_in_seconds": 60}, | ||
| ) | ||
| assert second.status_code == 200 | ||
| assert second.json()["warnings"] == ["multiple_active_attempts"] | ||
|
|
||
| visible = client.get(f"/api/v1/bounties/{bounty_id}/attempts").json() | ||
| assert visible["active_count"] == 2 | ||
| assert visible["warnings"] == ["multiple_active_attempts"] | ||
| assert {item["submitter_account"] for item in visible["attempts"]} == {"github:alice", "github:bob"} | ||
|
|
||
|
|
||
| def test_bounty_attempt_api_rejects_paid_bounties_and_releases_attempts( | ||
| sqlite_url: str, monkeypatch | ||
| ) -> None: | ||
| create_schema(sqlite_url) | ||
| with session_scope(sqlite_url) as session: | ||
| ensure_genesis(session) | ||
| bounty = create_bounty( | ||
| session, | ||
| repo="ramimbo/mergework", | ||
| issue_number=3214, | ||
| issue_url="https://github.com/ramimbo/mergework/issues/3214", | ||
| title="Attempt release", | ||
| reward_mrwk="20", | ||
| acceptance="Release admin flow should be tested.", | ||
| ) | ||
| bounty_id = bounty.id | ||
|
|
||
| monkeypatch.setenv("MERGEWORK_ADMIN_TOKEN", "t" * 32) | ||
| client = TestClient(create_app(database_url=sqlite_url, webhook_secret="secret")) | ||
| created = client.post( | ||
| f"/api/v1/bounties/{bounty_id}/attempts", | ||
| json={"submitter_account": "github:alice", "expires_in_seconds": 60}, | ||
| ).json() | ||
| released = client.post( | ||
| f"/api/v1/bounties/{bounty_id}/attempts/{created['id']}/release", | ||
| json={}, | ||
| headers={"x-mergework-admin-token": "t" * 32}, | ||
| ) | ||
| assert released.status_code == 200 | ||
| assert released.json()["status"] == "released" | ||
|
|
||
| with session_scope(sqlite_url) as session: | ||
| pay_bounty( | ||
| session, | ||
| bounty_id=bounty_id, | ||
| to_account="github:alice", | ||
| submission_url="https://github.com/ramimbo/mergework/pull/3214", | ||
| accepted_by="maintainer", | ||
| verifier_result={"label": "mrwk:accepted"}, | ||
| ) | ||
|
|
||
| rejected = client.post( | ||
| f"/api/v1/bounties/{bounty_id}/attempts", | ||
| json={"submitter_account": "github:bob", "expires_in_seconds": 60}, | ||
| ) | ||
| assert rejected.status_code == 400 | ||
| assert rejected.json()["detail"] == "bounty is not open" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
Expand tests to cover the full new API contract (detail warnings + active_only filter).
The new tests validate core flows, but they don’t assert the added attempts_active / attempt_warnings fields on GET /api/v1/bounties/{bounty_id} or active_only=true behavior on the attempts listing endpoint.
✅ Suggested additions
def test_bounty_attempt_api_tracks_visibility_and_warnings(sqlite_url: str) -> None:
@@
visible = client.get(f"/api/v1/bounties/{bounty_id}/attempts").json()
assert visible["active_count"] == 2
assert visible["warnings"] == ["multiple_active_attempts"]
assert {item["submitter_account"] for item in visible["attempts"]} == {"github:alice", "github:bob"}
+ bounty_detail = client.get(f"/api/v1/bounties/{bounty_id}").json()
+ assert bounty_detail["attempts_active"] == 2
+ assert bounty_detail["attempt_warnings"] == ["multiple_active_attempts"]
@@
def test_bounty_attempt_api_rejects_paid_bounties_and_releases_attempts(
@@
assert released.status_code == 200
assert released.json()["status"] == "released"
+ active_only = client.get(f"/api/v1/bounties/{bounty_id}/attempts?active_only=true").json()
+ assert active_only["active_count"] == 0
+ assert active_only["attempts"] == []As per coding guidelines, tests/**/*.py: Add or update tests for changed behavior.
🤖 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 `@tests/test_api_mcp.py` around lines 1763 - 1847, Tests are missing assertions
for the new GET /api/v1/bounties/{bounty_id} fields (attempts_active /
attempt_warnings) and for the attempts-listing active_only filter; update the
two test functions (test_bounty_attempt_api_tracks_visibility_and_warnings and
test_bounty_attempt_api_rejects_paid_bounties_and_releases_attempts) to call GET
/api/v1/bounties/{bounty_id} and assert the response includes the correct
attempts_active and attempt_warnings values after creating/releases/paying
attempts, and add a call to GET
/api/v1/bounties/{bounty_id}/attempts?active_only=true to assert only active
attempts are returned and warnings behave as expected.
shootingallday
suggested changes
May 25, 2026
Contributor
shootingallday
left a comment
shootingallday
left a comment
There was a problem hiding this comment.
Requesting changes because the branch currently fails the repository Ruff gate in touched files.
Evidence checked on PR head 2dce96db5dbcf3b242a235a0c473c0e0ba422c79:
uv run --extra dev python -m pytest tests/test_migrations.py tests/test_api_mcp.py::test_bounty_attempt_api_tracks_visibility_and_warnings -q-> 2 passed.git diff --check origin/main...HEAD-> clean.uv run --extra dev ruff check app/db.py app/main.py migrations/versions/0004_bounty_attempts.py tests/test_api_mcp.py-> failed.
Ruff failures observed:
app/main.py:1import block is unsorted.app/main.py:1023andapp/main.py:1024line too long in optionalsource_url/branch_refarguments.app/main.py:1050line too long in therelease_bounty_attempt(...)call.tests/test_api_mcp.py:1798line too long in the attempt submitter set assertion.
Please run Ruff format/check on the touched files and push the formatting fix. After that, the feature behavior can be reviewed against the broader attempt-reservation semantics.
aunysillyme
suggested changes
May 25, 2026
aunysillyme
left a comment
aunysillyme
left a comment
There was a problem hiding this comment.
Requesting changes because the attempt-reservation implementation has several schema/API correctness gaps beyond the already-reported formatting failure.
Evidence checked on the PR diff:
app/models.pydeclaressubmitter_accountandexpires_atwith indexes, and Alembic createsix_bounty_attempts_expires_at, but the runtime_migrate_schema()path inapp/db.pycreates only thebounty_id,status, and unique active-attempt indexes. Runtime-created databases therefore miss theexpires_atindex used by expiration scans.- The Alembic migration creates indexes for
bounty_id,status, andexpires_at, but it omits thesubmitter_accountindex declared by the ORM model. That makes migration-created schema differ from metadata for a field used in submitter-specific duplicate/release flows. - New routes under
/api/v1/bounties/{bounty_id}/attemptsaccept rawbounty_id: intwithout the existing_positive_bounty_idguard used by other bounty endpoints, so oversized or invalid integer IDs will not get the same SQLite-bound-safe validation behavior. - The bounty-detail endpoint calls
list_bounty_attempts()with the default inclusive mode and then filters active attempts in Python. That loads inactive/expired/released rows just to count active attempts; the helper should support an active-only/count query and use DB-level filtering. - Tests cover create/release basics, but do not assert the added
attempts_active/attempt_warningsfields onGET /api/v1/bounties/{id}, nor theactive_only=truelisting behavior after release/expiration.
Requested fixes:
- Make runtime schema creation and Alembic migration match the ORM indexes, including
expires_atandsubmitter_account. - Route new bounty-attempt endpoints through the same positive/SQLite-bounded bounty-id validation as the existing bounty APIs.
- Use DB-level active attempt filtering/counting instead of loading all attempts and filtering in Python.
- Add API tests for bounty-detail warning fields and
active_only=truelisting behavior.
No secrets, wallet private keys, private payout details, deployment values, private vulnerability details, or MRWK price claims were reviewed or disclosed.
ramimbo
added
duplicate
Owner
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.
Refs #321\n\nAdds advisory bounty attempt reservations with register/list/release/expire flows, warning surfacing, migration, and tests.\n\nValidation: PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 .venv/bin/python -m pytest -q
Summary by CodeRabbit
New Features
Tests