Convergence controls: review-round cap and low-only wave soft stop #111
No reviewers
Labels
No labels
Compat/Breaking
Kind/Bug
Kind/Documentation
Kind/Enhancement
Kind/Feature
Kind/Security
Kind/Testing
Priority
Critical
Priority
High
Priority
Low
Priority
Medium
Reviewed
Confirmed
Reviewed
Duplicate
Reviewed
Invalid
Reviewed
Won't Fix
Status
Abandoned
Status
Blocked
Status
Need More Info
No milestone
No assignees
3 participants
Notifications
Due date
No due date set.
Reference
marvin8/wuming!111
Loading…
Reference in a new issue
No description provided.
Delete branch "feat/issue-84-convergence-controls"
Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?
On an active PR, every fix-push triggers a fresh review wave with no natural stopping point — #75 ran five waves with increasingly marginal findings.
Hard cap
WUMING_MAX_ROUNDS(integer, default0= unlimited; fail-closed parsing, negatives rejected). Round = every prior## WuMingreview on the PR + 1<!-- wuming:review-cap -->marker scanned against the existing conversation — and the run exits cleanlySoft diminishing-returns stop
## WuMing — low-severity wave) instead of pinned line-by-line to the diffBoth fail open: no prior reviews means first-wave behaviour. Respects
dry_run. Shipped withaction.ymlmax_roundsinput (defaults parity enforced by the #108 suite), README config row, AGENTS.md env entry, and Release-Notes.This completes wave 2 (convergence): #79, #85, #84. Cross-PR ground (#80, #77) is wave 3.
Closes #84
WuMing
Found 4 issue(s). See inline comments below.
@ -162,6 +162,7 @@ See the [Forgejo/GitHub Actions guide](https://wuming.marvin8.zone/getting-start| `WUMING_VERIFY` | `true` | When `true`, each candidate finding is fact-checked against the changed file's full content before posting; findings whose factual claims the file contradicts are dropped. Verification is fail-open: provider errors keep findings. Set to `false` to disable. Only `true`/`false` (case-insensitive) are accepted — any other value is an error, not a fallback to the default. || `WUMING_REVIEW_CONTEXT` | `true` | When `true`, re-review waves receive the most recent WuMing review's findings (scoped to each agent's routed files) plus the conversation posted after it as prompt context, so answered or declined findings are not re-raised. The context block is capped and loading it is fail-open: if the conversation cannot be fetched, the review runs without it. Set to `false` for fully stateless reviews. Only `true`/`false` (case-insensitive) are accepted — any other value is an error. || `WUMING_SKIP_UNCHANGED` | `true` | When `true`, files that have not changed since the commit of the most recent WuMing review are skipped on re-reviews, so each wave focuses on what the fix actually touched. The comparison is fail-open: if it cannot be fetched, or no prior review exists, every file is reviewed. Set to `false` to re-review every changed file on every wave. Only `true`/`false` (case-insensitive) are accepted — any other value is an error. || `WUMING_MAX_ROUNDS` | `0` | Maximum number of WuMing review waves per pull request (counting every prior WuMing review). When the cap is reached, further waves are skipped and a single notice comment is posted instead of an inline review. `0` (the default) means unlimited. Only integers are accepted — any other value is an error at startup. |docs [LOW]
The row says "Only integers are accepted — any other value is an error at startup", which implies any integer (including negatives) is valid. The AGENTS.md entry added in the same PR (line 109) states the parser is fail-closed on negative values (
ValueError), so a user who setsWUMING_MAX_ROUNDS=-1will hit a startup error despite the README suggesting the value is acceptable. Reword to match, e.g. "Only non-negative integers are accepted — non-integer or negative values are an error at startup."@ -9,6 +9,7 @@ wuming 0.12.0- The review-quality settings are now settable from the Forgejo/GitHub Action interface: `action.yml` has `verify` and `min_severity` inputs (previously they silently fell back to defaults).- Re-reviews now remember the conversation: each wave receives the most recent WuMing review's findings plus the replies that followed as context, so answered or declined items are not re-raised (`WUMING_REVIEW_CONTEXT=false` for the old stateless behaviour).- Re-reviews also skip files that haven't changed since the last WuMing wave, so each pass focuses on what the fix actually touched (`WUMING_SKIP_UNCHANGED=false` to review everything every time).- Review waves can now be bounded: set a maximum number of review rounds per pull request (`WUMING_MAX_ROUNDS`), and waves whose findings are all low-severity are summarised in a single comment instead of pinned to the diff.docs [LOW]
The soft-stop threshold is described here (and again in the paragraph at line 44) as "findings are all low-severity", but the implementation summary for this PR defines it as "all below HIGH severity" (unrecognised severities fail open toward inline) — i.e. a wave whose findings are all MEDIUM (or medium+low) is also collapsed into the single plain comment, and only a HIGH/CRITICAL finding forces an inline post. These two statements cannot both be correct, and users will be surprised when medium-severity findings stop appearing on the diff. State the actual threshold explicitly ("all findings below HIGH severity") in both lines, or correct the PR description if the code only collapses genuinely LOW-only waves.
@ -81,0 +89,4 @@number of review waves that already ran."""return sum(1 for thread in review_threads if thread.body.startswith(WUMING_REVIEW_MARKER)security [HIGH]
A01 Broken Access Control: count_wuming_reviews treats any review whose body starts with the WuMing marker as a prior WuMing wave, without verifying that the review author is the WuMing bot/app. A malicious PR participant can submit a review/comment beginning with the marker to inflate prior_rounds, prematurely trigger the WUMING_MAX_ROUNDS cap, or make a first wave behave as a diminishing wave. Verify thread.author (or the forge user login) matches the bot's identity before counting.
@ -180,0 +191,4 @@body="Looks good overall — merging.",inline_comments=(),)assert count_wuming_reviews is not Nonetests [LOW]
Tautological assertion:
assert count_wuming_reviews is not Nonecan never fail because the function is imported at module load. Remove it or replace with a meaningful precondition check.Wave-1 review responses (commit
d70f506)✅ Fixed — review identity is now verified, not just the marker
src/wuming/context.pyValid HIGH — the marker is public text, so author check is the actual control. Both
count_wuming_reviews()andnewest_wuming_review()(the latter carried the same flaw from #109) now requireauthor == "forgejo-actions"alongside the marker, matching the identification rule this repo's own CI uses. A forged-marker review from a participant no longer inflates the round count or fakes the prior-review signal; covered by a new test. Fail direction for deployments posting under a different account: waves read as first-wave (everything reviewed, no soft stop) — the safe side.✅ Fixed — tautological assertion removed
tests/test_context.pyLeftover from an inline-import refactor during development — removed.
✅ Fixed — README now states non-negative integers
README.mdReworded: "Only non-negative integers are accepted — non-integer or negative values are an error at startup", matching the AGENTS.md entry and the parser.
✅ Fixed — release notes now say "below HIGH severity"
Release-Notes.mdBoth spots (the bullet and the prose paragraph) now read "all below HIGH severity (low or medium)" / "all below HIGH severity" — matching the implementation, where a medium-only wave also collapses into the plain comment.
WuMing — low-severity wave
All 1 finding(s) this wave are below HIGH severity and a review already ran on this pull request, so they are posted here instead of inline.
src/wuming/context.py:63code [MEDIUM] The author guard added to newest_wuming_review() is not exercised by any visible test. The new test in tests/test_context.py only covers count_wuming_reviews(). If this guard regressed, a forged review/comment starting with the WuMing marker could again be treated as a prior wave and trigger the soft-diminishing-returns path. Add a test for newest_wuming_review() that verifies it ignores a non-bot-authored marker review and still finds the real bot-authored review (or returns None when only forged ones exist).