Repository navigation
Conversation
Gate: only the trusted Bits publisher app's PASS on the exact open-main head counts, shared as a conjunction across PRs sharing a head. Fail closed on edited or unknown-bot reviews; never fall back to other tokens. Trusted publisher and allowlist live in the Chainguard policy; the event receiver fans the publisher per distinct resolved head.
❌ ErrorsYour PR has failed checks. Please review the issues below and take necessary action before merging. 🚦 5 Pipeline jobs failed
Useful? React with 👍 / 👎 This comment will be updated automatically if new data arrives.🔗 Commit SHA: ddeb78e | Docs | View more details | Give us feedback! |
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Scan-Build Report
Bug Summary
Filter Results by FileReports
|
|||||||||||||||||||||||||||||||||||||||||||||||||
|
@DataDog review |
CI Test ResultsRun: #43480868685 | Commit:
Status Overview
Legend: ✅ passed | ❌ failed | ⚪ skipped | 🚫 cancelled Summary: Total: 32 | Passed: 32 | Failed: 0 Updated: 2026-11-13 18:38:57 UTC |
![datadog-datadog-prod-us1[bot]](https://avatars.githubusercontent.com/u/365230?s=60&v=4){kind=small}
There was a problem hiding this comment.
The publisher can emit success without revalidating its snapshot, and response-body failures can prevent a blocking status update. Both defects should be corrected before enforcement activation.
🤖 Bits Code Review · Commit a3c8e5f
revalidated_decision again reads S(i), computes D(i), re-reads and fingerprint-compares S(i+1); a mismatch discards D(i) and restarts from the new snapshot, three mismatches return unstable and the publish CLI exits 1 without writing. Drops an unused import from the publish tests.
…dler A timeout or reset during response.read() now falls into the existing retry and ApiError conversion instead of escaping the fetch loop; the best-effort summary-write swallow gets the comment the linter wants.
A non-retryable non-2xx whose body read fails raises ApiError after one attempt instead of retrying like a transport failure; a 2xx body read keeps retrying. Pins the revalidation flicker case (S1 pass, S2 edited, S3 pass again) returning the last revalidated decision.
|
❌ 38 passed, 2 failed out of 40 expected configurations Test Matrix
Failure Detailsmusl-x64-hotspot-jdk25Tracer+profiler: musl-x64-openj9-jdk25Profiler-only: Links
|
Codex Review Summary: ✅ CompletedThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
A GraphQL response containing null or an array crashes the publisher before it writes a blocking error status, leaving any previous success visible. This undermines the fail-closed guarantee once enforcement is activated.
🤖 Bits Code Review · Commit 2808034
| if not isinstance(document, dict) or document.get("errors"): | ||
| raise ApiError("GraphQL errors for PR %s: %r" % (number, document.get("errors"))) |
There was a problem hiding this comment.
If the reviews endpoint returns JSON null or [], constructing this error calls document.get(...) and raises AttributeError instead of ApiError. The publisher then terminates without writing the intended blocking api-error status, leaving any earlier success visible. Once enforcement is activated, this violates the gate’s fail-closed behavior. Reject the non-object root before accessing its fields.
| if not isinstance(document, dict) or document.get("errors"): | |
| raise ApiError("GraphQL errors for PR %s: %r" % (number, document.get("errors"))) | |
| if not isinstance(document, dict): | |
| raise ApiError("malformed GraphQL response for PR %s" % (number,)) | |
| if document.get("errors"): | |
| raise ApiError("GraphQL errors for PR %s: %r" % (number, document.get("errors"))) |
React: 👍 useful · 👎 wrong · 😕 unclear · 👀 right, but not acting on it
What each reaction means
- 👍 Useful: correct, and you're fixing it or will.
- 👎 Wrong: the finding is incorrect, for example the bug can't happen or the code was misread.
- 😕 Unclear: you can't tell what it means or why it matters.
- 👀 Right, not acting: correct, but intended, out of scope for this PR, a known risk, or too minor.
🤖 Bits Code Review · Open Bits AI session
There was a problem hiding this comment.
Fixed in f1d5a11 — the non-dict root now raises ApiError before any field access, so a JSON null/[] reviews response fails closed into the blocking error status instead of crashing the publisher with AttributeError. Pinned at both levels: the ApiError at the fetch seam and the published error status end-to-end; reverting the guard is verified to fail both tests.
A JSON null or [] root raised AttributeError inside the ApiError message expression instead of ApiError, terminating the publisher before the blocking status was written. The non-dict root now raises ApiError and flows into the error decision.
There was a problem hiding this comment.
More details
The PR adds a trusted, current-head Bits review gate with snapshot revalidation and deferred activation. The GraphQL crash is fixed: non-object responses now produce a blocking error status; no reportable findings remain, while production activation and enforcement remain unvalidated.
🤖 Bits Code Review · Commit f1d5a11
A rate-limited response whose body read fails now sets the rate-limit window and raises RateLimitError at exhaustion instead of retrying as a plain transport failure; the body-read classification boundaries (status 500 and 300) are pinned by tests.
rate_limited_last resets every attempt, so a mixed sequence that started rate-limited but ended in plain server errors reports a transport ApiError, not RateLimitError.
Agentic review (Sphinx, standard effort)
Medium
Low
🤖 Generated with Claude Code |
- restore the contract's unmatched-selector fallback: a workflow_run matching no open-main PR resolves the run head plus every open-main head, not the run head alone; pin the one-variable base-change guard - read_statuses asks for per_page=100 so the 20-page cap fails at about 2000 statuses instead of 600 - the default transport refuses redirects, so the Authorization header (the status token on the POST) can never follow one to another host - resolve gets timeout-minutes 10 and publish 15: without a timeout they inherit the 360-minute default while _fetch can sleep 300 s per retry, and a stalled publish holds its per-SHA concurrency group - pin the unpinned suppression-comparison branches (state, target_url, foreign context) and drop RunnerDiscoveryTest's wrong AC-12 label
The two DefaultTransportTest tests only inspect the module objects, so reverting _urllib_transport to plain urlopen left them green while reintroducing redirect-token forwarding. The new test drives the transport through a recording opener swapped into _NO_REDIRECT_OPENER and fails on that revert (verified by mutation).
Security follow-ups on the publisher and the allowlist1. Make the app binding of The Octo-STS policy holds up. Octo-STS anchors every pattern, and the Without a source binding, a same-repo PR can add a workflow with The design already plans the binding: expected publisher app
2. Source of the eight-ID allowlist All eight IDs resolve to Datadog-owned
|
What does this PR do?:
Adds the Bits review merge gate: a merge-blocking CI status (
bits-review/head) published per distinct resolved open-main head and enforced by a Chainguard trust policy. Only the trusted Bits publisher app's PASS on the exact head SHA counts; results are conjoined across PRs sharing that head. The gate fails closed: edited or unknown-bot reviews block, and there is no token fallback. The event receiver fans the publisher out per distinct resolved head. The publish decision is revalidated: two consecutive snapshots must fingerprint-match before anything is written, and three mismatches suppress the write entirely.Motivation:
A Bits review that has gone stale (head moved) or was edited after the fact must never authorize a merge. The gate makes the merge decision derive only from the trusted publisher's current-head result, so stale-green and tampered reviews cannot pass a PR.
Additional Notes:
doc/BitsReviewMergeGateDesign.md.rate_limited_laststale-flag LOW is fixed (the flag now names the last attempt's classification, pinned both directions). The empty-P(H)-after-mismatch LOW is rejected: the empty path never writes a wrong status (at worst an older one stays visible for one trigger cycle, then the next sweep overwrites it); the transient-listing premise is unobserved for this endpoint and must land in the sub-second window between S(i) and S(i+1); the dominant real cause is the PR having genuinely closed/merged/moved heads — where exit 0 is correct and treating it as unstable would red-job routine merge timing for no safety gain; and the job exit code is not the blocking signal — the status is.How to test the change?:
bash .github/scripts/tests/test_bits_review_gate.sh— hermetic suite: "Ran 296 tests in 9.208s OK" on headc71c481(runner, policy, API, publish, workflow-contract families).maincheckout withpersist-credentials: false, receivertimeout-minutes: 5, no secrets or token references.For Datadog employees:
dd:platform-security-reviewskill, or file a request via the PSEC review form).bewairealso runs automatically on every PR.