Reference solution
A strong review should identify the following issues, in roughly this priority order.
Findings
1. An audit failure retries an already-accepted reviewer request
Location: src/reviewer-assignment.ts:53-58 and src/pull-request-opened.ts:23-26
The audit write is awaited in the same failure boundary as the GitHub request. If GitHub accepts the reviewers and audit.record then rejects, the handler returns 500 with retry: true. A redelivery will call GitHub again even though the externally visible operation already succeeded.
Catch and report the audit failure separately. Once GitHub has accepted the request, return a successful, non-retryable webhook response while surfacing the audit failure through an operational recovery path.
2. Duplicate webhook deliveries repeat all side effects
Location: src/pull-request-opened.ts:8-16
The event's deliveryId is never used, so every delivery invokes reviewer assignment again. Since the webhook is explicitly at least once, an ordinary redelivery can repeat both the GitHub request and audit event even when no code path fails.
Make processing durably idempotent by delivery ID, including concurrent duplicate deliveries, and replay the stored result without repeating accepted side effects.
3. Sensitive-file detection implements the wrong path rules
Location: src/reviewer-assignment.ts:8-10
path.includes("auth") misses both permissions/ and .github/workflows/. It also produces false positives for unrelated names such as src/author.ts.
Match normalized repository-relative paths against the three explicit protected directories, with directory-boundary-aware checks.
4. A sensitive pull request can proceed without an eligible security specialist
Location: src/reviewer-assignment.ts:38-48
The fallback searches the original engineers array, so it can select the author or an inactive or out-of-office security specialist. If no security specialist exists, the conditional does nothing and the ordinary reviewers are still sent to GitHub. Replacing selected[0] can also discard the only owning-team reviewer even when a valid two-person combination exists.
Build the final selection from the eligible pool while satisfying the security and owning-team constraints together. If no eligible security specialist exists, leave the pull request unassigned.
5. The pull-request author remains eligible
Location: src/reviewer-assignment.ts:12-14
The eligibility filter does not compare an engineer's ID with pullRequest.authorId. The author can therefore be selected through either the ordinary loops or the security fallback.
Exclude the author before ranking and use that same eligible pool for every selection path.
6. The output is not guaranteed to contain distinct reviewers
Location: src/reviewer-assignment.ts:18-36
Selection operates on array entries rather than engineer IDs. The first loop can add repeated entries directly, while the second loop's selected.includes(engineer) only detects the same object reference. Separate objects with the same ID can produce duplicate reviewer IDs.
Deduplicate candidates by stable engineer ID before selection, or track selected IDs while filling the slots.
7. Candidate ranking is incomplete and mutates caller-owned input
Location: src/reviewer-assignment.ts:12-14
The comparator only considers openReviewCount; it omits the required oldest-assignment and stable-ID tie-breakers. Calling sort also reorders the caller's engineers array in place, directly violating the contract.
Sort a copy with a complete comparator covering open reviews, assignment age—including an explicit policy for null—and stable engineer ID.
8. Owning-team preference consumes both slots and overrides ranking
Location: src/reviewer-assignment.ts:18-26
The first loop fills as many as two slots with owning-team engineers. The requirement only asks for at least one eligible owning-team reviewer; after that constraint is satisfied, the remaining slot should still follow the stated ranking.
Select the highest-ranked eligible owning-team engineer for the required preference, then fill the remaining slot from the highest-ranked remaining candidates.
9. The service requests an empty reviewer list
Location: src/reviewer-assignment.ts:51-53
When no reviewer is eligible, the code still calls GitHub with an empty array. That turns a legitimate unassigned outcome into an unnecessary external request that may fail or create misleading audit data.
Return the empty assignment before invoking GitHub, and only record an assignment audit when a reviewer request was accepted.
10. The tests do not verify the behaviors named in their descriptions or the stated contract
Location: src/reviewer-assignment.test.ts:24-61
The same-team test only checks result length, so it passes without selecting a same-team reviewer. The availability test also checks only length rather than the selected ID or GitHub arguments. There is no coverage for author exclusion, protected paths, missing or unavailable security specialists, distinct IDs, tie-break ordering, input immutability, duplicate deliveries, or the different GitHub and audit failure outcomes.
Assert the exact returned IDs and dependency arguments, and add handler-level tests for idempotency and failure classification.
Reasonable non-findings
- Returning fewer than two reviewers when fewer are eligible is acceptable; the contract says "at most two," not exactly two.
- Ascending
openReviewCount is the correct first ranking dimension. The defect is the missing tie-breakers and later selection logic overriding that ranking.
- Filtering inactive and out-of-office engineers in the ordinary candidate pool is correct. The security fallback is what bypasses that filter.
- Calling GitHub before recording a successful-assignment audit is a reasonable order. The issue is treating a later audit failure as if GitHub had also failed.
- No specialty preference is required for pull requests outside the three protected path groups.