Bound treasury proposal bounty ids#544
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
|
Reviewed PR #544 at head No blocking findings from this pass. I inspected Validation run locally from an isolated PR worktree: I also checked the commit status for |
|
Reviewed PR #544 at Evidence checked:
Validation:
Assessment: no blocker found. The change is narrowly scoped to direct treasury proposal payload validation and matches the existing path-parameter SQLite integer bound. |
eliasx45
left a comment
There was a problem hiding this comment.
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.
barnacleagent-svg
left a comment
There was a problem hiding this comment.
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.
wangedmund77-cmyk
left a comment
There was a problem hiding this comment.
Reviewed current head 25483adfc28bfff6bdadc54880c260e705c1ec01 for bounty #578.
Evidence checked:
- Inspected
app/treasury.pyandtests/test_treasury_proposals.py. git diff --check origin/main...origin/pr/544returned clean.- Ran
uv run pytest tests/test_treasury_proposals.py -qon 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.
Summary
bounty_idpayloads before SQLite bindingpay_bountyandclose_bountyproposal actionsEvidence
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 bounded400.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-> passedgit diff --check-> cleanRefs #512