What happened?
Source inspection of v0.3.0 and main at 5acaa96b5a290b92f5b5214203af4886cb91d920 shows two limitations in the built-in approved requirement:
ListReviews requests one page and discards pagination metadata.
checkApproved returns true for any returned APPROVED review, without reducing reviews to each reviewer's current vote or checking outstanding change requests.
This can miss approvals on later pages or accept an older approval even when that reviewer subsequently requests changes. An approval from one reviewer also satisfies the requirement while another reviewer has an outstanding change request.
Evidence: ListReviews, checkApproved.
These findings are from source inspection; no live infrastructure apply was executed to reproduce them.
What did you expect to happen?
The built-in approval requirement should consider the full review history and current votes. Proposed semantics: at least one current approval and no outstanding change requests. If allowing outstanding change requests is intentional, document that behavior and provide an explicit stricter option.
Acceptance criteria:
- Follow all review pages; fail closed if fetching is incomplete or fails. If a safety cap is used, reaching it with more results available must not yield approval.
- Reduce voting reviews by stable reviewer identity and chronological order. A later
APPROVED, CHANGES_REQUESTED, or DISMISSED determines that reviewer's standing; COMMENTED and PENDING do not erase a vote.
- Do not count dismissed approvals.
- Under the proposed strict semantics, any reviewer's current
CHANGES_REQUESTED blocks apply, even when another reviewer approves.
- A change request subsequently replaced by approval no longer blocks.
- Add regression tests for each case and document the resulting semantics.
How do we reproduce it?
Use mocked GitHub review responses so no Terraform execution is necessary:
- Return a full first page containing only comment reviews, followed by a second page with an approval. Verify all pages are fetched and approval is recognized.
- Return reviewer A's
APPROVED, followed by A's CHANGES_REQUESTED. Expected: not approved.
- Return A's
APPROVED and B's CHANGES_REQUESTED. Expected under strict semantics: not approved.
- Return A's
CHANGES_REQUESTED, followed by A's APPROVED. Expected: approved, absent other blocking votes.
- Return only a dismissed approval. Expected: not approved.
- Fail the API request for a later page. Expected: refuse approval rather than decide from the partial list.
Did this work before?
No known working upstream version was identified. This behavior was found while comparing a downstream implementation that already handles pagination and latest-vote evaluation against upstream.
Environment
Neptune: v0.3.0 and main at 5acaa96b5a290b92f5b5214203af4886cb91d920 (source inspection).
Platform: intended GitHub Actions integration. Terraform version and operating system are not material to this API-level behavior.
What happened?
Source inspection of v0.3.0 and main at
5acaa96b5a290b92f5b5214203af4886cb91d920shows two limitations in the built-inapprovedrequirement:ListReviewsrequests one page and discards pagination metadata.checkApprovedreturns true for any returnedAPPROVEDreview, without reducing reviews to each reviewer's current vote or checking outstanding change requests.This can miss approvals on later pages or accept an older approval even when that reviewer subsequently requests changes. An approval from one reviewer also satisfies the requirement while another reviewer has an outstanding change request.
Evidence: ListReviews, checkApproved.
These findings are from source inspection; no live infrastructure apply was executed to reproduce them.
What did you expect to happen?
The built-in approval requirement should consider the full review history and current votes. Proposed semantics: at least one current approval and no outstanding change requests. If allowing outstanding change requests is intentional, document that behavior and provide an explicit stricter option.
Acceptance criteria:
APPROVED,CHANGES_REQUESTED, orDISMISSEDdetermines that reviewer's standing;COMMENTEDandPENDINGdo not erase a vote.CHANGES_REQUESTEDblocks apply, even when another reviewer approves.How do we reproduce it?
Use mocked GitHub review responses so no Terraform execution is necessary:
APPROVED, followed by A'sCHANGES_REQUESTED. Expected: not approved.APPROVEDand B'sCHANGES_REQUESTED. Expected under strict semantics: not approved.CHANGES_REQUESTED, followed by A'sAPPROVED. Expected: approved, absent other blocking votes.Did this work before?
No known working upstream version was identified. This behavior was found while comparing a downstream implementation that already handles pagination and latest-vote evaluation against upstream.
Environment
Neptune: v0.3.0 and main at
5acaa96b5a290b92f5b5214203af4886cb91d920(source inspection).Platform: intended GitHub Actions integration. Terraform version and operating system are not material to this API-level behavior.