Skip to content

Bound treasury proposal bounty ids#544

Open
Baijack-star wants to merge 1 commit into
ramimbo:governance-mvp-proposalsfrom
Baijack-star:codex/b512-treasury-bounty-id-bound
Open

Bound treasury proposal bounty ids#544
Baijack-star wants to merge 1 commit into
ramimbo:governance-mvp-proposalsfrom
Baijack-star:codex/b512-treasury-bounty-id-bound

Conversation

@Baijack-star
Copy link
Copy Markdown

Summary

  • reject oversized direct treasury proposal bounty_id payloads before SQLite binding
  • cover both direct pay_bounty and close_bounty proposal actions

Evidence

This follows up on the current-head review finding in #512 / PR #458: oversized direct proposal payload ids returned 500 Internal Server Error, while the neighboring proposal path-id guard already returns a bounded 400.

Validation

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 ./.venv/bin/python -m pytest tests/test_treasury_proposals.py -q -> 23 passed
  • ./.venv/bin/python -m ruff check app/treasury.py tests/test_treasury_proposals.py -> passed
  • ./.venv/bin/python -m ruff format --check app/treasury.py tests/test_treasury_proposals.py -> passed
  • git diff --check -> clean

Refs #512

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented May 27, 2026

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 814b6009-6c3c-4b3d-bfb2-e5b631d54721

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review

Comment @coderabbitai help to get the list of available commands and usage tips.

@kbaajjg-max
Copy link
Copy Markdown

Reviewed PR #544 at head 25483adfc28bfff6bdadc54880c260e705c1ec01.

No blocking findings from this pass. I inspected app/treasury.py and tests/test_treasury_proposals.py; the new SQLITE_INTEGER_MAX guard is applied after the positive-id check and before proposal creation for both pay_bounty and close_bounty, so oversized IDs are rejected before they can reach SQLite-backed proposal persistence. The new parametrized regression test covers both actions with 2**63, asserts the 400 detail, and verifies no TreasuryProposal row is created.

Validation run locally from an isolated PR worktree:

python -m pytest tests/test_treasury_proposals.py -k oversized_bounty_id -q
2 passed, 21 deselected

ruff check app/treasury.py tests/test_treasury_proposals.py
All checks passed

I also checked the commit status for 25483adfc28bfff6bdadc54880c260e705c1ec01; CodeRabbit and the Quality, readiness, docs, and image checks workflow were both successful.

@tinyopsstudio
Copy link
Copy Markdown

Reviewed PR #544 at 25483adfc28bfff6bdadc54880c260e705c1ec01 for the treasury direct-proposal bounty-id bounds path.

Evidence checked:

  • inspected app/treasury.py, app/path_params.py, and tests/test_treasury_proposals.py;
  • confirmed direct pay_bounty and close_bounty proposal payloads now reject bounty_id > SQLITE_INTEGER_MAX before proposal persistence;
  • confirmed the existing positive-id checks remain in place before the new upper-bound guard;
  • confirmed the regression test covers both direct proposal actions and asserts no TreasuryProposal row is created on the bounded 400 response.

Validation:

  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_treasury_proposals.py::test_direct_pay_and_close_proposals_reject_oversized_bounty_id -q -> 2 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest tests/test_treasury_proposals.py -q -> 23 passed
  • PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 uv run --extra dev python -m pytest -q -> 436 passed
  • uv run --extra dev ruff check app/treasury.py tests/test_treasury_proposals.py -> passed
  • uv run --extra dev ruff format --check app/treasury.py tests/test_treasury_proposals.py -> 2 files already formatted
  • git diff --check origin/governance-mvp-proposals...HEAD -> clean

Assessment: no blocker found. The change is narrowly scoped to direct treasury proposal payload validation and matches the existing path-parameter SQLite integer bound.

Copy link
Copy Markdown

@eliasx45 eliasx45 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 25483adfc28bfff6bdadc54880c260e705c1ec01 for the direct treasury proposal bounty-id bound.

No blocker found. I verified the new SQLITE_INTEGER_MAX guard is applied after the existing positive-id check and before proposal persistence for both direct pay_bounty and close_bounty payloads. The regression covers both actions with 2**63, asserts the bounded 400 response, and verifies no TreasuryProposal row is inserted.

Validation run from the checked-out PR branch:

git diff --check origin/governance-mvp-proposals...HEAD
# clean

.\.venv\Scripts\python.exe -m pytest tests\test_treasury_proposals.py::test_direct_pay_and_close_proposals_reject_oversized_bounty_id -q
# 2 passed

.\.venv\Scripts\python.exe -m pytest tests\test_treasury_proposals.py -q
# 23 passed

.\.venv\Scripts\python.exe -m ruff check app\treasury.py tests\test_treasury_proposals.py
# All checks passed

.\.venv\Scripts\python.exe -m ruff format --check app\treasury.py tests\test_treasury_proposals.py
# 2 files already formatted

.\.venv\Scripts\python.exe -m mypy app\treasury.py
# Success: no issues found in 1 source file

.\.venv\Scripts\python.exe scripts\docs_smoke.py
# docs smoke ok

.\.venv\Scripts\python.exe -m pytest -q
# 436 passed

GitHub PR checks are green for this head: CodeRabbit status and the quality/readiness/docs/image workflow are successful.

Copy link
Copy Markdown

@barnacleagent-svg barnacleagent-svg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Verdict: APPROVED

Scope: Adds SQLITE_INTEGER_MAX bounds to pay_bounty and close_bounty treasury proposals. Prevents oversized bounty IDs from being proposed.

Checklist:

  • Diff: +32/-0 across 2 files
  • Uses existing SQLITE_INTEGER_MAX constant
  • Parametrized test covers both pay and close actions
  • Follows existing validation patterns

Conclusion: Clean validation hardening. Ready to merge.

Copy link
Copy Markdown

@wangedmund77-cmyk wangedmund77-cmyk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed current head 25483adfc28bfff6bdadc54880c260e705c1ec01 for bounty #578.

Evidence checked:

  • Inspected app/treasury.py and tests/test_treasury_proposals.py.
  • git diff --check origin/main...origin/pr/544 returned clean.
  • Ran uv run pytest tests/test_treasury_proposals.py -q on the PR head: 23 passed, 1 StarletteDeprecationWarning.
  • GitHub mergeability is clean/MERGEABLE.

No blocker found. The added bounds cover both direct pay_bounty and close_bounty proposal payloads, which is the right layer for rejecting oversized bounty ids before proposal creation.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants