shuke987 opened a new pull request, #68996: URL: https://github.com/apache/doris/pull/68996
### What problem does this PR solve? Issue Number: N/A Problem Summary: The review runner simultaneously required unanimous `NO_NEW_VALUABLE_FINDINGS` before submission and required submission after at most three rounds. When round 3 still produced candidates, the agent could mark its goal blocked and withhold verified findings. This happened in the reviews of [#67549](https://github.com/apache/doris/actions/runs/38222336278) and [#68499](https://github.com/apache/doris/actions/runs/38271916203). Separate verified delivery from convergence: - Submit after convergence or the third round. At the cap, publish verified findings and disclose remaining questions or coverage gaps; unverified candidates must not become inline findings. - Require `converged`, `rounds`, and `incomplete_reason` in the submission. Non-convergence requires round 3 and a nonempty reason. The trusted helper renders this declaration into the review body and binds it to the existing digest and exact API readback. - A verified delivery completes execution, but non-convergence produces a failing review status even with zero P0/P1. Both the source and aggregate status explain non-convergence. Reviews without P0/P1 remain GitHub comments; no artificial blocking finding is created. - Preserve head/base validation, submission deduplication, capacity recovery, stale-head cancellation, and existing authorized local/skip overrides. The prompt and helpers are loaded from the same workflow revision. Existing invocations retain their old schema; new invocations reject missing convergence fields rather than defaulting to a pass. This change does not update the agent binary or model. ### Release note None ### Check List (For Author) - Test: - [x] Unit Test: `PYTHONDONTWRITEBYTECODE=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py'` - macOS / Python 3.12: 166 tests, passed, 3 Linux-only tests skipped. - Linux arm64 / Python 3.13: 166 tests, passed, 2 tests requiring jq skipped. The actual Bash/jq workflow tests passed on macOS; Linux covered process cleanup and descendant handling. - Coverage includes incomplete delivery with zero P0/P1, strict schema validation, altered convergence declarations, exact readback, lost POST responses, stale heads, capacity after delivery, local/skip overrides, and source-to-aggregate status propagation. - [x] Manual test: `actionlint -shellcheck= .github/workflows/code-review-runner.yml` and diff whitespace checks passed. - Workflow tests execute the actual Bash/jq result and status blocks with GitHub writes captured locally. No live model review or production canary was run; model compliance with the revised prompt still needs validation after rollout. - Behavior changed: - [x] Yes. Verified findings are delivered at the three-round limit while incomplete reviews continue to block the automated check. - Does this need documentation? - [x] No. Internal review workflow; its embedded prompt and generated review/status output describe the behavior. <details> <summary>Self-review: code-review skill Part 1.3 checkpoints</summary> Flow: review prompt → declared submission → trusted helper validation and GitHub readback → runner result/event → workflow source status → aggregate status. | Checkpoint | Conclusion | | --- | --- | | Goal and proof | Deterministic tests prove verified incomplete delivery and a failed gate, including zero P0/P1; live model behavior is not claimed. | | Scope and clarity | Limited to the CI review prompt, submission/result metadata, status explanation, and their tests. | | Concurrency and locks | No new threads or locks. Existing exclusive submission-intent creation, token-bound readback, and live head checks remain; duplicate and stale-head cases are covered. | | Lifecycle and initialization | No new resource lifecycle or static initialization. Existing runner cleanup and superseded handling remain covered, including Linux process tests. | | Configuration | No configuration knobs added. Three rounds remains the existing limit. | | Compatibility | Submission schema changes together with the pinned workflow/helper revision. Missing fields fail validation; existing status context formats remain compatible. | | Thrift/protobuf | Not applicable; no protocol definitions changed. | | Parallel paths | Normal delivery and delivery followed by capacity failure carry the same metadata and verdict. Existing local/skip override semantics are preserved. | | Conditions | Exact boolean/integer validation prevents ambiguous declarations; incomplete delivery requires round 3 and a reason. Description matching only explains an already-failing source and cannot authorize success. | | Coverage | Unit and workflow-shell tests cover both convergence outcomes, priority combinations, malformed inputs, readback mismatches, retry deduplication, and stale contexts. | | Test results | Both platform runs passed with complementary skips; no product regression output files changed. | | Observability | Review body, runner log, result JSON, completion event, and source/aggregate descriptions expose convergence. No extra API requests are introduced for this metadata. | | Transactions and persistence | No product persistence changes. The submission intent persists convergence fields before POST and retains the existing recovery mechanism. | | Data writes and atomicity | GitHub posting stays on the trusted helper path; a lost POST response is recovered by exact readback without a second post. Head changes still prevent successful verification. | | FE/BE variable passing | Not applicable; no FE/BE changes. | | Performance | Constant-size metadata and one status classification; no extra model rounds, GitHub queries, or polling. | | Other issues and limits | Round count and consensus remain reviewer declarations; the helper validates their schema and binds them to the delivered review. A real model canary is still needed to validate instruction following. | | Error handling | Invalid or missing metadata and mismatched readback fail explicitly. Incomplete review is a successful delivery with a separate failed gate. | | Memory safety / data correctness | No native memory, storage visibility, delete bitmap, or EditLog changes; existing review ownership and exact comment-set verification remain intact. | | BE nullability | Not applicable. | </details> ### Check List (For Reviewer who merge this PR) - [ ] Confirm the release note - [ ] Confirm test cases - [ ] Confirm document - [ ] Add branch pick label -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
