Repository navigation
Conversation
### What problem does this PR solve? Related PR: apache#68499 Problem Summary: Bulk REST review creation can reject a valid set of inline findings with HTTP 422 on a large PR. Create one pending review, add each finding with a separate GraphQL thread mutation, verify the complete draft, and publish once. Preserve the existing input schema, frozen target guards, exclusive submission intent, exact final readback, and summary-only path. Record staged progress and GraphQL errors; clean up only owned pending drafts before publication, without retrying uncertain writes. ### Release note None ### Check List (For Author) - Test: Unit Test: 275 tests OK, 19 platform skips; real gh HTTP tests use loopback only. Manual test: all five original apache#68499 findings accepted in one draft via separate GraphQL mutations, then draft deleted. Frozen diff mapping replay matched all five anchors. Actionlint and git diff --check. - Behavior changed: Yes, inline review submission is staged before publishing. - Does this need documentation: Yes, operational guidance updated in this PR. Live formal publication and the GitHub Actions identity are not yet validated.
|
Thank you for your contribution to Apache Doris. Please clearly describe your PR:
|
|
skip buildall |
### What problem does this PR solve? Related PR: apache#68657, apache#69447 Problem Summary: The preflight long-body test assumed inline comments were in the initial REST review request. Verify the final readback body instead so the same assertion covers both direct and staged draft submission. ### Release note None ### Check List (For Author) - Test: Run submit_review tests independently and the complete five-PR integration suite. - Behavior changed: No production change. - Does this need documentation: No.
|
/review |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Static review of ed13bb8c750e4ebda81427b2b81eb5089e742e40: one verified P2 finding in the new diff parser; no P0/P1 findings or existing blockers. Non-UTF-8 bytes and bare carriage returns in textual diffs can prevent valid inline findings from being delivered. All four changed files and the runner/API/status interactions were reviewed. No additional user focus was supplied.
The reviewed flow is: validate target and input → map positions → claim exclusive intent → create one pending review → add threads sequentially → verify the complete draft → publish once → verify final delivery.
Critical checkpoint conclusions:
- Goal and proof: staging addresses bulk inline submission while preserving one final review, but the inline finding identifies a valid-input failure. Added tests were inspected; no tests or builds were run.
- Scope and reuse: changes remain focused on the helper, its tests, diagnostics documentation and one artifact entry; common ownership and verification logic is factored into helpers.
- Concurrency: requests are sequential; the per-PR workflow queue, exclusive intent and pre-publication competing-review check preserve existing duplicate controls. No new lock order applies.
- Lifecycle: partial drafts cannot satisfy completion; pre-publication cleanup checks exact ownership. An uncertain publication is verified by readback without another write. Outage, budget and hard-termination cleanup limits are documented.
- Configuration: no operator configuration was added.
- Compatibility: the input schema, summary-only path and verification of already submitted reviews remain compatible; no database symbol or storage-format change applies.
- Thrift/protobuf: no changes.
- Parallel paths: summary-only, existing-blocker-only, staged inline, lost-response and capacity-recovery paths were inspected; no additional defect was verified.
- Conditional/error handling: invalid anchors, changed targets, mismatched locations/sets and ownership changes fail explicitly; cleanup preserves the primary failure.
- Test coverage: positive staging and negative write/ownership/head/set cases are present. Non-UTF-8 and bare-CR patch inputs lack coverage. Staged integration mocks use a new-file diff where position equals line, so a realistic mixed-hunk/LEFT REST round trip remains a validation limit.
- Expected results: no regression output files changed; new API assertions were checked statically without claiming their execution.
- Observability: draft stage, review/thread IDs and finding location are journaled; GraphQL logical errors retain HTTP diagnostics; the workflow explicitly collects the draft journal.
- Persistence/failover: exclusive submission intent remains the retry guard, and the journal is diagnostic rather than proof of delivery. No Doris EditLog or FE failover change applies.
- Writes/atomicity/crashes: publication is gated by complete draft verification and final readback; documented interruption cases can require manual cleanup of a pending draft.
- FE/BE propagation: not applicable to these CI-only changes.
- Performance: patch parsing and sequential staging scale linearly with diff size and finding count; target checks serve the frozen-target guarantee. No further actionable performance issue was found.
- Other issues: no additional verified defect or unresolved candidate remains. Live formal publication with the Actions identity remains rollout validation, as the PR documents; this static review did not create experimental drafts or validate that end to end.
The API shape assessment used the GitHub review REST contract and GraphQL review-thread schema.
Review converged after 3 round(s).
| locations = {} | ||
| old_path = new_path = position = None | ||
| old_line = new_line = 0 | ||
| with (context / "pr.diff").open() as handle: |
There was a problem hiding this comment.
[P2] Preserve raw bytes and LF boundaries when reading the diff
On the UTF-8 runner, Path.open() strictly decodes the whole patch. Git emits raw text bytes, and the existing regression-test/data/load_p0/stream_load/csv_with_none_utf8_data.csv contains invalid UTF-8 (0xb4) without NULs. A PR whose patch includes that row raises UnicodeDecodeError here, even if every finding targets another source file, preventing review delivery.
Default universal newline handling also splits bare carriage returns in CSV data: for patch bytes +a\rb\n+c\n, +c is diff position 2, but this loop sees +a, b, +c and rejects position 2 as non-commentable. The test_csv_line_end_lf_crlf fixtures already contain such bytes.
Use byte-preserving decoding with LF-only iteration, or parse bytes and decode only metadata, and cover both inputs. Changing only newline="\n" leaves the decoding failure; newline="" still splits bare CR.
What problem does this PR solve?
Related PR: #68499
The review runner's bulk REST create-review request rejects five valid inline findings on #68499 with HTTP 422 (
Position could not be resolved; line/side also fails). In a controlled experiment at the same head/base, all five original findings were accepted when added sequentially to one PENDING review through GraphQL. The experimental draft was deleted afterward. The exact GitHub internal diff limit is not established.For reviews with inline findings, this change creates one draft, adds one thread per request, verifies the complete comment set, then publishes once. It converts the existing path/position/body input to line/side using the authoritative local PR diff. Summary-only reviews keep their current submission path.
The invocation marker, exclusive submission intent, frozen head/base checks, priority/convergence gate, and exact final readback remain required. The helper rechecks competing bot reviews before publication. Add failures trigger best-effort cleanup only of the exactly owned pending draft; uncertain writes are never retried.
review-draft.jsonis included in failure artifacts, and GraphQL errors in HTTP 200 responses retain HTTP diagnostics. Ambiguous publication, unavailable cleanup, and hard termination boundaries are documented.Live formal publication and the GitHub Actions identity still need rollout validation. The existing experiment validates draft construction, not end-to-end CI delivery.
Release note
None
Check List (For Author)
RUN_GH_HTTP_TESTS=1 python3 -m unittest discover -s .github/scripts -p 'test_*.py'— 275 tests OK, 19 platform skips. Real gh HTTP tests use only a loopback server with fake credentials.git diff --check; actionlint v1.7.7, excluding only its unsupported existingconcurrency.queuekey (optional ShellCheck/Pyflakes disabled)..github/scripts/review-github-diagnostics.mdin this PR.Check List (For Reviewer who merge this PR)