Skip to content

fix: keep bounty verifier star errors shaped#4881

Merged
Scottcjn merged 1 commit into
Scottcjn:mainfrom
Asti1982:codex/harden-bounty-verifier-tests
May 14, 2026
Merged

fix: keep bounty verifier star errors shaped#4881
Scottcjn merged 1 commit into
Scottcjn:mainfrom
Asti1982:codex/harden-bounty-verifier-tests

Conversation

@Asti1982
Copy link
Copy Markdown
Contributor

Summary

  • keep verify_stars() error responses shaped like successful responses
  • cover the failure path that previously omitted is_star_king and repos
  • replace the empty test_verifier.py placeholder with offline unit tests that stub GitHub/Gemini/dotenv dependencies

Root cause

generate_report() expects verify_stars() to return count, is_star_king, and repos. On GithubException, verify_stars() returned only error and count, so transient GitHub API failures could turn the verifier report path into a KeyError.

Validation

  • python -m pytest tools\bounty-bot-pro\tests\test_verifier.py -q -> 2 passed
  • python -m py_compile tools\bounty-bot-pro\verifier.py tools\bounty-bot-pro\tests\test_verifier.py -> passed; emits the existing markdown-backtick SyntaxWarning already covered by separate open PRs
  • git diff --check -- tools\bounty-bot-pro\verifier.py tools\bounty-bot-pro\tests\test_verifier.py
  • python tools\bcos_spdx_check.py --base-ref origin/main

@github-actions
Copy link
Copy Markdown
Contributor

Welcome to RustChain! Thanks for your first pull request.

Before we review, please make sure:

  • Your PR has a BCOS-L1 or BCOS-L2 label
  • New code files include an SPDX license header
  • You've tested your changes against the live node

Bounty tiers: Micro (1-10 RTC) | Standard (20-50) | Major (75-100) | Critical (100-150)

A maintainer will review your PR soon. Thanks for contributing!

@github-actions github-actions Bot added size/M PR: 51-200 lines BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) labels May 12, 2026
Copy link
Copy Markdown
Contributor

@saim256 saim256 left a comment

Choose a reason for hiding this comment

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

I reviewed the verifier error-path change and the added regression coverage. The fix keeps verify_stars() returning the same report shape on GithubException, which prevents generate_report() from failing on missing count/is_star_king/repos keys when GitHub rate limits or API failures occur.

Validation performed locally:

  • python -m pytest tools\bounty-bot-pro\tests\test_verifier.py -q -> 2 passed
  • python -m py_compile tools\bounty-bot-pro\verifier.py tools\bounty-bot-pro\tests\test_verifier.py -> syntax OK; existing invalid-escape warning on the wallet markdown string remains non-blocking
  • git diff --check origin/main...pr-4881 -> passed
  • python tools\bcos_spdx_check.py --base-ref origin/main -> OK

No blocking issues found.

Copy link
Copy Markdown
Contributor

@loganoe loganoe 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 verifier error-shape fix and the new focused tests. The change preserves the report contract by returning count/is_star_king/repos even when PyGithub raises, and the generated-report path now has regression coverage around reward inputs. Verified with: PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 pytest -q tools/bounty-bot-pro/tests/test_verifier.py (2 passed; existing verifier SyntaxWarning remains unrelated).

Copy link
Copy Markdown
Contributor

@508704820 508704820 left a comment

Choose a reason for hiding this comment

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

Code Review: Keep Bounty Verifier Star Errors Shaped

Summary

Adds 93-line test file for the bounty verifier tool with stub-based module loading. Tests star/PR verification with mocked GitHub and Google API dependencies.

What Works Well

  1. Stub module loading: Creates mock github and google modules for testing without real API calls
  2. 93 lines: Good coverage of verifier logic
  3. importlib.util: Clean module loading pattern

Verdict: Approve

Good test coverage for the bounty verification tool with proper dependency mocking.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great contribution! LGTM.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

Great contribution! LGTM.

Copy link
Copy Markdown
Contributor

@loganoe loganoe left a comment

Choose a reason for hiding this comment

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

Approved. The change keeps verify_stars() error returns compatible with generate_report() by preserving the count, is_star_king, and repos keys on the GithubException path, and the new offline tests cover both the failure shape and the report calculation path.

Validation performed: git diff --check origin/main...HEAD, PYTEST_DISABLE_PLUGIN_AUTOLOAD=1 python3 -m pytest -q tools/bounty-bot-pro/tests/test_verifier.py, and python3 -m py_compile tools/bounty-bot-pro/verifier.py tools/bounty-bot-pro/tests/test_verifier.py all pass. The compile/test run still emits the pre-existing invalid-backtick escape warning in verifier.py, which is outside this patch's changed line.

Copy link
Copy Markdown

@shuibui shuibui left a comment

Choose a reason for hiding this comment

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

Code Review: Approve

Good fix.

**Verdict: Approve.

Copy link
Copy Markdown

@shuibui shuibui left a comment

Choose a reason for hiding this comment

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

Code Review: Approve

Good fix. Addresses the issue correctly.

**Verdict: Approve.

Copy link
Copy Markdown

@shuibui shuibui left a comment

Choose a reason for hiding this comment

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

Code Review: Approve

Good fix. Addresses the issue correctly.

**Verdict: Approve.

Copy link
Copy Markdown
Contributor

@jaxint jaxint left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks for contributing. Approved.

Copy link
Copy Markdown

@TJCurnutte TJCurnutte left a comment

Choose a reason for hiding this comment

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

Verified review

Approved for PR #4881 at head f3600bb5494fad86a3255bcb386aca4ceb2dc1e8.

I inspected the PR metadata/diff for: fix: keep bounty verifier star errors shaped.
Changed scope: tools/bounty-bot-pro/tests/test_verifier.py (+93/-0), tools/bounty-bot-pro/verifier.py (+6/-1).

Local validation

  • git diff --check origin/main...HEAD — pass
  • python3 tools/bcos_spdx_check.py --base-ref origin/main — pass
  • python3 -B -m py_compile tools/bounty-bot-pro/tests/test_verifier.py tools/bounty-bot-pro/verifier.py — pass
  • python3 -B -m pytest tools/bounty-bot-pro/tests/test_verifier.py -q — pass
  • added-line secret pattern scan — pass

Review reasoning

  • The diff is focused on the stated security/validation/bugfix scope and is small enough to review in this bounty tick.
  • Whitespace, SPDX, syntax/static, and added-line secret checks passed for this exact head.
  • I did not find a blocker in the changed files or validation output.

Boundary: this review certifies the current diff/head only; payout/merge is not assumed.

Copy link
Copy Markdown
Contributor

@himanalot himanalot left a comment

Choose a reason for hiding this comment

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

Reviewed tools/bounty-bot-pro/verifier.py and the new offline tests. verify_stars() now returns the same report shape on GithubException as on success (count, is_star_king, and repos), which prevents the report path from turning transient GitHub errors into KeyErrors. The tests stub GitHub/Gemini/dotenv dependencies and cover both the failure shape and generated report reward inputs.

I do not see a blocking issue in this patch. Approved.

@guangningsun
Copy link
Copy Markdown
Contributor

PR Review — Standard Quality ✓

PR: #4881 — Fix bounty verifier star errors shaped

What I reviewed

  • tools/bounty-bot-pro/verifier.py
  • tools/bounty-bot-pro/tests/test_verifier.py (new 93-line test file)

Observations

  1. Error shaping in the bounty verifier is a UX + reliability improvement. When the star verification fails, returning a shaped/structured error (rather than a raw exception or empty response) allows the bounty bot to handle failures gracefully and report meaningful status to users.

  2. New 93-line test file for test_verifier.py covers the error handling path. This is a good test coverage addition for a component that interacts with external APIs (GitHub stars), which can fail in many ways (rate limits, network errors, etc.).

LGTM.

Bounty: #2782
Disclosure: I received RTC compensation for this review.

@Scottcjn Scottcjn merged commit 92209df into Scottcjn:main May 14, 2026
12 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

BCOS-L1 Beacon Certified Open Source tier BCOS-L1 (required for non-doc PRs) size/M PR: 51-200 lines

Projects

None yet

Development

Successfully merging this pull request may close these issues.

10 participants