Skip to content

Share treasury executor pass runner#904

Closed
pqmfei wants to merge 1 commit into
ramimbo:mainfrom
pqmfei:pqmfei/846-share-treasury-executor-pass-runner
Closed

Share treasury executor pass runner#904
pqmfei wants to merge 1 commit into
ramimbo:mainfrom
pqmfei:pqmfei/846-share-treasury-executor-pass-runner

Conversation

@pqmfei
Copy link
Copy Markdown
Contributor

@pqmfei pqmfei commented Jun 5, 2026

Bounty #846

Summary

  • add a shared private _run_logged_pass() helper for treasury executor pass and bounty-board refresh logging/error handling
  • keep the existing success/failure log messages, once-mode failure exit, and board-refresh exception swallowing behavior unchanged
  • add regression coverage that a board-refresh failure is swallowed and the scheduler continues to the next refresh window

Duplicate check

Validation

  • .\.venv\Scripts\python.exe -m pytest tests\test_treasury_executor_script.py -q -> 9 passed
  • .\.venv\Scripts\python.exe -m pytest tests\test_treasury_executor_script.py tests\test_treasury_executor.py -q -> 17 passed
  • .\.venv\Scripts\python.exe -m ruff check scripts\treasury_executor.py tests\test_treasury_executor_script.py -> passed
  • .\.venv\Scripts\python.exe -m ruff format --check scripts\treasury_executor.py tests\test_treasury_executor_script.py -> 2 files already formatted
  • .\.venv\Scripts\python.exe -m mypy scripts\treasury_executor.py -> success
  • git diff --check -> clean

Scope: maintainability-only cleanup in the read/write scheduler wrapper. No proposal execution semantics, bounty board payloads, ledger behavior, wallet behavior, treasury mutation rules, payout behavior, admin-token behavior, private data, exchange, bridge, cash-out, or MRWK price behavior changed.

Summary by CodeRabbit

  • Refactor

    • Improved code organization by centralizing execution and logging logic for treasury operations, reducing code duplication.
  • Tests

    • Enhanced test coverage to verify the treasury executor's resilience, ensuring it continues operating normally when board refresh operations encounter errors.

@coderabbitai
Copy link
Copy Markdown

coderabbitai Bot commented Jun 5, 2026

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro Plus

Run ID: 0c45661e-a503-4cb2-91c4-8c46b614a46f

📥 Commits

Reviewing files that changed from the base of the PR and between d4d0e48 and 158afa0.

📒 Files selected for processing (2)
  • scripts/treasury_executor.py
  • tests/test_treasury_executor_script.py

📝 Walkthrough

Walkthrough

The PR refactors scripts/treasury_executor.py to eliminate duplicate try/except logging blocks by introducing a _run_logged_pass helper function. This helper runs a callable, logs exceptions via logging.exception, returns a success boolean, and logs a success message with a sorted JSON report. Both the treasury proposal execution and bounty board refresh paths now use this helper, and a new test validates that the loop continues after a board refresh failure.

Changes

Executor logging refactor

Layer / File(s) Summary
Logging helper and treasury execution refactor
scripts/treasury_executor.py
Adds Callable import, introduces _run_logged_pass helper that centralizes run-and-log logic, and refactors the treasury execution branch to use it instead of inline exception handling. The --once exit code on failure is preserved.
Apply logging helper to bounty board refresh
scripts/treasury_executor.py
Updates the bounty board refresh branch to call _run_logged_pass with appropriate success and failure messages, removing inline exception handling.
Test board refresh failure resilience
tests/test_treasury_executor_script.py
Adds test_enabled_loop_continues_after_board_refresh_failure to verify the loop continues executing after a board refresh raises RuntimeError, confirming two scheduled refresh attempts at 60s and 120s before the loop terminates.
🚥 Pre-merge checks | ✅ 6
✅ Passed checks (6 passed)
Check name Status Explanation
Title check ✅ Passed The title 'Share treasury executor pass runner' directly names the main refactoring: consolidating treasury executor pass and bounty-board refresh handling into a shared helper function.
Description check ✅ Passed The description covers all required template sections: summary of changes, evidence with bounty tracking and validation results, duplicate checks, test evidence with comprehensive validation steps, and clear scope boundaries.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
Mergework Public Artifact Hygiene ✅ Passed PR contains only maintainability refactoring with operational logging; no investment, price, cash-out, or fabricated payout claims present.
Bounty Pr Focus ✅ Passed Diff matches stated scope: added _run_logged_pass helper consolidating try/except/logging; test verifies board-refresh failures are swallowed; no unrelated changes to other files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.


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

Copy link
Copy Markdown

@catcherintheroad-hub catcherintheroad-hub 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 158afa0652f5f2e7fac4487023215378c8f300b1 as a non-author.

Evidence checked:

  • inspected scripts/treasury_executor.py and tests/test_treasury_executor_script.py;
  • confirmed _run_logged_pass() centralizes the existing try/log-success/log-exception pattern while preserving --once failure exit behavior for treasury execution;
  • confirmed board refresh failures remain swallowed in the continuous loop and the new regression covers a failed refresh at 60s followed by another refresh attempt at 120s;
  • verified the diff is limited to the treasury executor script and its focused test;
  • checked GitHub state before review: PR open, mergeable=MERGEABLE, CodeRabbit successful, no prior human reviews, and no #838 claim for PR #904 visible on the bounty issue.

Validation run on this exact head:

  • uv run --python 3.12 --extra dev python -m pytest tests/test_treasury_executor_script.py -q -> 9 passed.
  • uv run --python 3.12 --extra dev python -m pytest tests/test_treasury_executor_script.py tests/test_treasury_executor.py -q -> 17 passed.
  • uv run --python 3.12 --extra dev ruff check scripts/treasury_executor.py tests/test_treasury_executor_script.py -> passed.
  • uv run --python 3.12 --extra dev ruff format --check scripts/treasury_executor.py tests/test_treasury_executor_script.py -> 2 files already formatted.
  • uv run --python 3.12 --extra dev mypy scripts/treasury_executor.py -> success.
  • uv run --python 3.12 --extra dev python scripts/docs_smoke.py -> docs smoke ok.
  • git diff --check origin/main...HEAD -> clean.
  • git merge-tree --write-tree origin/main HEAD -> clean tree c76e9e40c1bf82f4135bdec79c20dc3d49a01461.

No blocker found in the focused maintainability refactor. Caveat: GitHub still reports mergeStateStatus=UNSTABLE because the hosted rollup currently only shows CodeRabbit; I did not see the full hosted Quality/readiness/docs/image check on this PR yet. Scope remains treasury executor logging/test behavior only; no proposal execution semantics, payout, ledger mutation, wallet behavior, admin-token behavior, private data, exchange, bridge, cash-out, or MRWK price behavior changed.

Copy link
Copy Markdown
Contributor

@szx19970521 szx19970521 left a comment

Choose a reason for hiding this comment

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

Reviewed PR #904 at current head 158afa0652f5f2e7fac4487023215378c8f300b1.

Evidence checked:

  • inspected scripts/treasury_executor.py and tests/test_treasury_executor_script.py;
  • confirmed _run_logged_pass() preserves the treasury executor --once failure path by returning 1 when run_once() raises;
  • confirmed continuous-loop board refresh failures remain logged and non-fatal, with the next refresh still scheduled;
  • confirmed the refactor keeps the existing JSON-sorted success log shape for executor and board-refresh reports;
  • checked GitHub state before review: this is not my PR, PR is open, mergeable, and the current head is 158afa0652f5f2e7fac4487023215378c8f300b1.

Validation on the reviewed head:

  • python -m pytest tests\test_treasury_executor_script.py -q -> 9 passed.
  • python -m ruff check scripts\treasury_executor.py tests\test_treasury_executor_script.py -> passed.
  • python -m ruff format --check scripts\treasury_executor.py tests\test_treasury_executor_script.py -> 2 files already formatted.
  • python -m mypy scripts\treasury_executor.py -> success.

Scope reviewed: treasury executor pass logging/refactor and focused regression coverage only. No wallet material, ledger mutation, treasury proposal execution, payout execution, admin-token behavior, secrets, bridge/exchange/cash-out behavior, or MRWK price behavior was used.

Copy link
Copy Markdown

@Errordog2 Errordog2 left a comment

Choose a reason for hiding this comment

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

Reviewed the treasury executor pass-runner sharing refactor.

Validation performed locally:

  • .venv\Scripts\python.exe -m pytest tests/test_treasury_executor_script.py -> 9 passed
  • .venv\Scripts\python.exe -m ruff check scripts/treasury_executor.py tests/test_treasury_executor_script.py -> passed
  • .venv\Scripts\python.exe -m ruff format --check scripts/treasury_executor.py tests/test_treasury_executor_script.py -> passed
  • .venv\Scripts\python.exe -m mypy scripts/treasury_executor.py -> passed
  • git diff --check -> passed
  • git merge-tree $(git merge-base origin/main HEAD) origin/main HEAD -> clean merge

The extracted _run_logged_pass preserves the existing once-mode failure behavior for executor passes, keeps continuous-mode board refresh failures non-fatal, and the added test covers retrying after a board refresh exception. Looks good to me.

@ramimbo
Copy link
Copy Markdown
Owner

ramimbo commented Jun 7, 2026

Closing this stale PR because its referenced bounty is no longer live/payable and the branch is dirty or unstable against current main. If the work is still relevant, please resubmit it against a live bounty with current evidence.

@ramimbo ramimbo closed this Jun 7, 2026
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.

5 participants