Validate report script source arguments#832
Conversation
📝 WalkthroughWalkthroughThree CLI scripts gain a shared ChangesCLI Argument Validation Pattern
Possibly related issues
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Comment |
vondutchi
force-pushed
the
codex/report-source-validation-807
branch
from
3e8e146 to
bbba6bd
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 23a049ba-5072-4912-a927-0a0f5458b295
📒 Files selected for processing (6)
scripts/claim_inventory.pyscripts/pr_queue_health.pyscripts/proposed_work_triage.pytests/test_claim_inventory.pytests/test_pr_queue_health.pytests/test_proposed_work_triage.py
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/test_claim_inventory.py (1)
291-311:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing
--repo ""regression case.The parametrization includes
["--repo", " "](whitespace) but omits["--repo", ""](empty string). Both boundary conditions should be tested to prove the validation rejects empty and whitespace-only values.Add:
(["--repo", ""], "--repo must be a non-empty value")As per coding guidelines,
Add or update tests for changed behaviorandFocus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.tests/test_pr_queue_health.py (1)
107-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover the empty-string
--repocase too.The test matrix exercises
["--repo", " "]but not["--repo", ""]. Since this change validates empty and whitespace-only source values, add(["--repo", ""], "--repo must be a non-empty value")to prove both invalid forms are rejected.As per coding guidelines,
Add or update tests for changed behaviorandFocus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.tests/test_proposed_work_triage.py (1)
406-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the explicit empty
--repoassertion.This regression test covers
["--input", ""]and["--input", " "]and["--repo", " "], but omits["--repo", ""]. Add(["--repo", ""], "--repo must be a non-empty value")to exercise the empty-string boundary.As per coding guidelines,
Add or update tests for changed behaviorandFocus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3172a354-140a-4ecc-b8b8-dacfa087cb78
📒 Files selected for processing (6)
scripts/claim_inventory.pyscripts/pr_queue_health.pyscripts/proposed_work_triage.pytests/test_claim_inventory.pytests/test_pr_queue_health.pytests/test_proposed_work_triage.py
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 2
♻️ Duplicate comments (3)
tests/test_claim_inventory.py (1)
291-311:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the missing
--repo ""regression case.The parametrization includes
["--repo", " "](whitespace) but omits["--repo", ""](empty string). Both boundary conditions should be tested to prove the validation rejects empty and whitespace-only values.Add:
(["--repo", ""], "--repo must be a non-empty value")As per coding guidelines,
Add or update tests for changed behaviorandFocus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.tests/test_pr_queue_health.py (1)
107-127:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winCover the empty-string
--repocase too.The test matrix exercises
["--repo", " "]but not["--repo", ""]. Since this change validates empty and whitespace-only source values, add(["--repo", ""], "--repo must be a non-empty value")to prove both invalid forms are rejected.As per coding guidelines,
Add or update tests for changed behaviorandFocus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.tests/test_proposed_work_triage.py (1)
406-427:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAdd the explicit empty
--repoassertion.This regression test covers
["--input", ""]and["--input", " "]and["--repo", " "], but omits["--repo", ""]. Add(["--repo", ""], "--repo must be a non-empty value")to exercise the empty-string boundary.As per coding guidelines,
Add or update tests for changed behaviorandFocus on whether tests prove the changed behavior and include negative, replay, boundary, or regression cases where relevant.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 3172a354-140a-4ecc-b8b8-dacfa087cb78
📒 Files selected for processing (6)
scripts/claim_inventory.pyscripts/pr_queue_health.pyscripts/proposed_work_triage.pytests/test_claim_inventory.pytests/test_pr_queue_health.pytests/test_proposed_work_triage.py
🛑 Comments failed to post (2)
tests/test_proposed_work_triage.py (2)
771-771:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove redundant import statement.
pytestis already imported at line 8. This redundant import inside the function is unnecessary and unconventional.
782-782:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove redundant import statement.
pytestis already imported at line 8. This redundant import inside the function is unnecessary and unconventional.
keilogic
left a comment
keilogic
left a comment
There was a problem hiding this comment.
Thanks, this looks good. I did not find a current-head blocker.
The three report scripts now stop empty and whitespace-only --input / --repo source values at argparse with bounded errors, and the explicit args.input is not None checks preserve the offline/live branch selection.
I also checked the explicit --repo "" boundary CodeRabbit called out for all three scripts. Each exits with code 2, reports the expected --repo must be a non-empty value error, and does not traceback. Adding those exact cases to the parametrized tests would make the regression matrix cleaner, and the two redundant local pytest imports in tests/test_proposed_work_triage.py could be tidied, but neither is a runtime blocker for this patch.
Validation I ran:
python -m pytest tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py -q-> 52 passedpython scripts/docs_smoke.py-> docs smoke okpython -m ruff check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> passedpython -m ruff format --check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> passed- Direct probes for
--input "",--input " ",--repo "", and--repo " "across all three scripts -> bounded argparse errors, no tracebacks git diff --check origin/main...HEAD-> passedgit merge-tree --write-tree origin/main HEAD-> clean
One note on type checking: targeted mypy over the three scripts still reports an existing no-any-return at the unchanged _gh_issue_search() return path in scripts/proposed_work_triage.py; I did not treat that as introduced by this PR.
gshaowei6
left a comment
gshaowei6
left a comment
There was a problem hiding this comment.
Reviewed current head bbba6bdd9e501af2f0f647ac74766d1b517c4dc9 against origin/main 8f197562f3e6fe5d1a61a57456c36eda4309b0d1.
I think this needs one more pass. The new _require_non_empty_arg() helpers validate with value.strip(), but they return the original untrimmed value. That leaves padded, non-empty source arguments outside the bounded validation path and still lets them fall into raw downstream tracebacks.
Direct probes on this head:
python scripts/claim_inventory.py --repo " ramimbo/mergework "reachesgh issue listwith the padded repo and raises a rawRuntimeError/GraphQL repository error.python scripts/pr_queue_health.py --repo " ramimbo/mergework "reachesgh pr listwith the padded repo and raises a rawRuntimeError/GraphQL repository error.python scripts/proposed_work_triage.py --repo " ramimbo/mergework "reaches the live GitHub search path and raises the raw repository resolution error.python scripts/claim_inventory.py --input " tests/fixtures/missing.json "reaches_load_input()and raises a rawFileNotFoundErrorfor the space-padded path.python scripts/pr_queue_health.py --input " tests/fixtures/missing.json "does the same.
That is still the same source-argument validation family as #807. I would either return the stripped value from the helper or reject values whose stripped form differs, then add coverage for padded non-empty --repo / --input cases so these scripts fail through argparse instead of raw tracebacks.
Validation I ran:
- With
TMP/TEMPpointed inside the worktree:python -m pytest tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py -q-> 52 passed. python -m ruff check scripts/proposed_work_triage.py scripts/claim_inventory.py scripts/pr_queue_health.py tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py-> passed.python -m ruff format --check ...-> 6 files already formatted.python scripts/docs_smoke.py-> docs smoke ok.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean treecfd9b3d305cdf66804502d0c3c6ed72180a6044a.- Hosted
Quality, readiness, docs, and image checksand CodeRabbit are passing.
I also tried mypy on the three touched scripts; it still reports scripts/proposed_work_triage.py:397: Returning Any from function declared to return list[dict[str, Any]]. The raw traceback path above is the blocker for me.
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/proposed_work_triage.py (1)
545-546:⚠️ Potential issue | 🟠 Major | ⚡ Quick winValidate offline fixture shape before calling
analyze_proposed_work().This branch now accepts any JSON value. A fixture like
[]or"x"will parse successfully here and then crash with anAttributeErroronceanalyze_proposed_work()doesdata.get(...). The other two report scripts already reject non-object fixtures up front, so this should do the same and add a regression test for it.As per coding guidelines, `Add or update tests for changed behavior`.Proposed fix
+def _load_input(path: str) -> dict[str, Any]: + data = json.loads(Path(path).read_text(encoding="utf-8")) + if not isinstance(data, dict): + raise ValueError("proposed-work input must be a JSON object") + return data + + def main(argv: list[str] | None = None) -> int: @@ if args.input is not None: if args.payment_bounty_issue: parser.error("--payment-bounty-issue is only valid in live --repo mode") - input_path = Path(_require_non_empty_arg(parser, "--input", args.input)) - data = json.loads(input_path.read_text(encoding="utf-8")) + data = _load_input(_require_non_empty_arg(parser, "--input", args.input)) else: repo = _require_non_empty_arg(parser, "--repo", args.repo) data = load_live_issues(
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 0b56b234-0012-4339-bce3-e23c35ed2ae2
📒 Files selected for processing (6)
scripts/claim_inventory.pyscripts/pr_queue_health.pyscripts/proposed_work_triage.pytests/test_claim_inventory.pytests/test_pr_queue_health.pytests/test_proposed_work_triage.py
Zejia
left a comment
Zejia
left a comment
There was a problem hiding this comment.
I think this needs one more small fix on the current head.
The earlier padded --repo / --input issue is fixed: I verified the three report scripts now reject leading/trailing whitespace through argparse. However, scripts/proposed_work_triage.py still parses any JSON value from --input and then passes it to analyze_proposed_work(), which expects an object. A non-object fixture therefore still produces a raw traceback instead of a bounded CLI/input validation error.
Direct repros on head 22db26d27276e38571e72c8f6d3786b114c5f8cd:
printf '[]' > "$tmp"; python scripts/proposed_work_triage.py --input "$tmp" --format json->AttributeError: 'list' object has no attribute 'get'printf '"x"' > "$tmp"; python scripts/proposed_work_triage.py --input "$tmp" --format json->AttributeError: 'str' object has no attribute 'get'
A valid object fixture still works:
printf '{"issues": [], "bounties": []}' > "$tmp"; python scripts/proposed_work_triage.py --input "$tmp" --format json-> exits 0 and emits the expected empty report shape.
Suggested fix: add a _load_input() path for proposed_work_triage.py like the other report scripts and reject non-object JSON before calling analyze_proposed_work(), with regression coverage for at least one non-object fixture.
Validation I ran:
python -m pytest tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py -q-> 61 passedpython scripts/docs_smoke.py-> docs smoke okpython -m ruff check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> passedpython -m ruff format --check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> 6 files already formattedgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean tree5b2a0872c1412e9cc61708642209045faf37fd15- Hosted Quality/readiness/docs/image and CodeRabbit checks are passing
Targeted mypy over the three touched scripts still reports the existing scripts/proposed_work_triage.py:397 no-any-return; I did not treat that as introduced by this PR. No private data, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used.
There was a problem hiding this comment.
Actionable comments posted: 1
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d06dba8-b58a-4e49-98bb-db28df0a0f57
📒 Files selected for processing (2)
scripts/proposed_work_triage.pytests/test_proposed_work_triage.py
| def _load_input(path: str) -> dict[str, Any]: | ||
| data = json.loads(Path(path).read_text(encoding="utf-8")) | ||
| if not isinstance(data, dict): | ||
| raise ValueError("proposed-work input must be a JSON object") | ||
| return data |
There was a problem hiding this comment.
Report invalid offline fixtures through parser.error().
A non-object --input fixture still escapes as ValueError here, so the CLI prints a traceback instead of the same SystemExit(2)/usage error path used for the new empty-source validation. Please route fixture read/parse/shape failures through parser.error(...) as well.
Proposed fix
-def _load_input(path: str) -> dict[str, Any]:
- data = json.loads(Path(path).read_text(encoding="utf-8"))
- if not isinstance(data, dict):
- raise ValueError("proposed-work input must be a JSON object")
+def _load_input(parser: argparse.ArgumentParser, path: str) -> dict[str, Any]:
+ try:
+ data = json.loads(Path(path).read_text(encoding="utf-8"))
+ except OSError as exc:
+ parser.error(f"--input could not be read: {exc}")
+ except json.JSONDecodeError as exc:
+ parser.error(f"--input must contain valid JSON: {exc.msg}")
+ if not isinstance(data, dict):
+ parser.error("--input must be a JSON object")
return data
@@
- data = _load_input(_require_non_empty_arg(parser, "--input", args.input))
+ data = _load_input(parser, _require_non_empty_arg(parser, "--input", args.input))Also applies to: 549-552
gshaowei6
left a comment
gshaowei6
left a comment
There was a problem hiding this comment.
Reviewed current head 4d483268ee645cccecd0f4557241319926282d15 against origin/main 8f197562f3e6fe5d1a61a57456c36eda4309b0d1.
I think this still needs one more pass. The prior blank/padded source-argument blockers are fixed: direct CLI probes for empty, whitespace-only, and leading/trailing-space --input / --repo values now exit through argparse with code 2 and no traceback across scripts/claim_inventory.py, scripts/pr_queue_health.py, and scripts/proposed_work_triage.py.
The remaining blocker is the new proposed-work offline fixture shape path. scripts/proposed_work_triage.py now rejects non-object JSON before analyze_proposed_work(), but _load_input() raises a raw ValueError. When the script is invoked as a real CLI, that still prints a Python traceback instead of the bounded CLI/input validation error requested for this validation hardening work.
Direct probes on this head using the project .venv:
printf '[]' > "$tmp"; .venv/Scripts/python.exe scripts/proposed_work_triage.py --input "$tmp" --format json-> exits 1 withTraceback, ending at_load_input()withValueError: proposed-work input must be a JSON object.printf '"x"' > "$tmp"; .venv/Scripts/python.exe scripts/proposed_work_triage.py --input "$tmp" --format json-> same traceback path.printf '{"issues": [], "bounties": []}' > "$tmp"; .venv/Scripts/python.exe scripts/proposed_work_triage.py --input "$tmp" --format json-> exits 0 and emits the expected empty report JSON.
This is a current-head follow-up to the previous 22db26d... review, not the same finding repeated unchanged. That earlier head crashed later with AttributeError after passing a non-object fixture into analyze_proposed_work(). This head moved the check earlier, but the user-facing CLI behavior is still a raw traceback.
I would route the non-object fixture error through parser.error(...) (or pass the parser into the loader and use the same bounded error path) and update test_proposed_work_triage_rejects_non_object_offline_fixture so it expects SystemExit(2), empty stdout, and no traceback. A subprocess/entrypoint regression would also catch this exact gap.
Validation I ran:
.venv/Scripts/python.exe -m pytest tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py -q-> 62 passed.venv/Scripts/python.exe -m ruff check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> passed.venv/Scripts/python.exe -m ruff format --check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> 6 files already formatted.venv/Scripts/python.exe scripts/docs_smoke.py-> docs smoke okgit diff --check origin/main...HEAD-> cleangit merge-tree --write-tree origin/main HEAD-> clean treef66442c60c844b7b81019b562650e65e55bf3c4c- Hosted
Quality, readiness, docs, and image checksand CodeRabbit status are successful on the current head
Targeted mypy over the three touched scripts still reports the existing scripts/proposed_work_triage.py:397 no-any-return; I did not treat that as introduced by this PR. No private data, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out, or price behavior was used.
|
Current-head blocker still reproduces on 4d48326: a non-object offline fixture for proposed-work triage still exits through a Python traceback instead of a bounded CLI/input error.\n\nRepro:\n\n |
xiefuzheng713-alt
left a comment
xiefuzheng713-alt
left a comment
There was a problem hiding this comment.
Reviewed current head 4d483268ee645cccecd0f4557241319926282d15 as a non-author.
The blank and padded --input / --repo guards are working, and the focused test suite passes. One current-head blocker still reproduces: the proposed-work offline fixture shape check raises a raw ValueError from _load_input(), so invoking the real CLI with a non-object fixture still exits through a traceback instead of the bounded argparse/input-error path this validation hardening is trying to provide.
Direct repro on this head:
printf '[]' > <tmp>
uv run --extra dev python scripts/proposed_work_triage.py --input <tmp> --format json
# exit 1 with Traceback ending at:
# ValueError: proposed-work input must be a JSON object
Suggested fix: route the non-object fixture error through parser.error(...) or catch ValueError around _load_input() and convert it to SystemExit(2). Please also update test_proposed_work_triage_rejects_non_object_offline_fixture to assert the CLI-shaped behavior: exit code 2, empty stdout, the expected bounded error message in stderr, and no Traceback.
Validation I ran:
uv run --extra dev pytest tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py -q-> 62 passed.- direct non-object fixture CLI probe above -> exit 1 with traceback.
uv run --extra dev ruff check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> passed.uv run --extra dev ruff format --check scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py tests/test_proposed_work_triage.py-> 6 files already formatted.uv run --extra dev mypy scripts/claim_inventory.py scripts/pr_queue_health.py scripts/proposed_work_triage.py-> existingscripts/proposed_work_triage.py:397no-any-return only; I did not treat it as introduced by this PR.git diff --check origin/main...HEAD-> clean.git merge-tree --write-tree origin/main HEAD-> clean tree182e48cacaec2094a3caaabea7dfb63b738b4797.
GitHub state checked before review: PR REST state reports mergeable=true, mergeable_state=clean; hosted Quality/readiness/docs/image and CodeRabbit statuses are success. No private data, secrets, wallet material, payout execution, treasury mutation, exchange, bridge, cash-out behavior, or MRWK price behavior was used.
6 participants
Bounty #761
Source issue: #807
Summary
--input/--reposource values in proposed-work triage, claim inventory, and PR queue healthEvidence
#807 reproduced raw tracebacks or malformed
ghinvocations for empty source arguments across the remaining report helpers.Verification
/tmp/mergework-py312-venv/bin/python -m pytest tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py/tmp/mergework-py312-venv/bin/python scripts/docs_smoke.py/tmp/mergework-py312-venv/bin/python -m ruff check scripts/proposed_work_triage.py scripts/claim_inventory.py scripts/pr_queue_health.py tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.py/tmp/mergework-py312-venv/bin/python -m ruff format --check scripts/proposed_work_triage.py scripts/claim_inventory.py scripts/pr_queue_health.py tests/test_proposed_work_triage.py tests/test_claim_inventory.py tests/test_pr_queue_health.pygit diff --checkNo changes to review candidate or submission quality source validation, no report scoring rewrite, no GitHub mutation, treasury, payout, wallet, private data, secret, bridge, exchange, off-ramp, cash-out, or price behavior.
Summary by CodeRabbit
Bug Fixes
Tests