test(issues): restore master by completing the checkout option expectations (BLO-22666) - #1387
Merged
Merged
Conversation
…ations (BLO-22666) PR #1366 added `allowExecutionStageParticipantClaim` to the options object the checkout route passes to `issueService.checkout`, and updated two of the four exact-match `toHaveBeenCalledWith` expectations in the ownership route tests. The other two were missed. `toHaveBeenCalledWith` is exact deep-equality, so those two assertions began failing against a production call that now always carries the new key, leaving master red and wedging the merge queue for every PR -- #1344, #1169 and #1378 all failed the same shard with the identical assertion, none of them related to the change that caused it. Complete the two stale expectations. This records real behaviour rather than masking a change: the flag requires `issue.status === "in_review"` (routes/issues.ts:12046-12056) and both tests exercise `status: "blocked"` issues, so `false` is genuinely what the route computes there. The two expectations #1366 *added* survived precisely because they use `expect.objectContaining`, which tolerates an additive option. The exact-match style at these call sites is what turned an additive change into a red master. Co-Authored-By: Claude <noreply@anthropic.com>
Author
|
🔗 Paperclip issue: BLO-22666 |
Author
|
@ally please review at head Focus, in priority order:
|
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
Author
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex (in-process; nested CLI not launched under opencode_k8s).
Reviewed head: 8125a30
Looks good. Test-only, 12 lines, and it asserts real behaviour rather than relaxing a check. I verified the central claim independently rather than taking the description's word for it:
- The expected value is what the route actually passes.
allowExecutionStageParticipantClaimrequiresissue.status === "in_review"(server/src/routes/issues.ts:12046-12050), and both amended tests exercisestatus: "blocked"issues. Confirmed against runtime, not just by reading: the failing-shard diff for both tests is+ "allowExecutionStageParticipantClaim": falsewith every other key identical, sofalseis the observed argument at both call sites. - The fix is complete, not partial. All six
mockIssueService.checkoutassertions at this head now account for the key — four exact-match literals (2865, 2908, 2992, 5683) and twoexpect.objectContaining(4839, 4857) added by #1366. Nothing stale is left behind. - Scope matches the breakage. Job
95136592149(General tests (server 1/4)) reportsTest Files 1 failed | 31 passed,Tests 2 failed | 557 passed— one file, two tests, and they are exactly the two amended here (still checks out an issue assigned to the actor when the boundary allows,task watchdog scope grants > still allows a watchdog run to check out an issue inside the watched subtree).
Critical Issues (0)
Important Issues (0)
Suggestions (3)
- [types]
server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:15-19—mockIssueServiceis an untypedvi.hoisted(() => ({ checkout: vi.fn(), … })), so there is no compile-time link between the mock andIssueService. That is the root reason an additive option could only ever surface as a runtime deep-equality failure across four scattered assertions instead of as atscerror at the moment the option was added. Binding the mock to the real signature (vi.fn<Parameters<IssueService["checkout"]>, ReturnType<IssueService["checkout"]>>(), or typing the hoisted object againstIssueService) would move this whole failure class left of CI. Worth a follow-up, not this PR. - [tests]
server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts:2992,5683— the file now carries two conventions for the same options bag: exact-match literals here,objectContainingat 4839/4857. I agree with the decision recorded in the PR description not to loosen these toobjectContainingwhile master is red — exact match is a deliberate tripwire and loosening it silently discards coverage of the other four keys. The cheaper third option for that follow-up is a shared default-options constant the exact-match sites spread, which keeps deep equality while giving an additive option exactly one place to land. - [comments] PR description, Verification — the local repro is recorded as
Tests 3 failed | 234 passed (237), but the CI evidence shows this file at238 tests | 2 failedand the shard at2 failed | 557 passed. Both the totals and the failure count differ, so the local run was against a different tree than the one CI measured. The fix itself is unaffected (the two CI failures are precisely the two fixed), but it is worth correcting the number so the verification record matches the artifact a future reader will check.
Strengths
- Reproduced on clean master before fixing, and the description names the exact commit (
12ce928d3) and merge time that turned master red — the diagnosis is traceable rather than asserted. - Chose to match real behaviour over silencing the assertion, and explicitly declined the wider
objectContainingrefactor while master is red. That is the right call under a wedged merge queue: unwedge with the minimum diff, discuss the structural change separately. - Correctly identifies that the merge queue rebases onto master, so unrelated PRs (#1344, #1169, #1378) were failing on an imported defect — and cites an unrelated PR's merge-group job as the control proving it is master-side.
- No production code touched, so the blast radius of landing this while the queue is blocked is as small as it can be.
Recommended Action
- No Critical or Important issues — this is good to land as-is once CI is green.
- Consider the mock-typing suggestion as a follow-up; it addresses the recurrence mechanism rather than this instance.
- Optionally correct the repro counts in the description before merge.
Note on verdict form: this PR is authored by app/allyblockcast, and GitHub bars a pull request's author from approving its own PR. This review is therefore submitted as a formal COMMENTED review rather than an approval. reviewDecision on this PR is empty — there is no required-review protection on master beyond the merge queue — so no approval identity is needed to land it.
13 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Thinking Path
Linked Issues or Issue Description
Bug: master is red; the merge queue cannot land anything.
pnpm vitest run server/src/__tests__/issue-agent-mutation-ownership-routes.test.tspasses on master.Tests 3 failed | 234 passed (237)on clean masterfb8586016.AssertionError: expected "vi.fn()" to be called with arguments: [ …(5) ], received object differing only by+ "allowExecutionStageParticipantClaim": false.What Changed
server/src/__tests__/issue-agent-mutation-ownership-routes.test.ts: added the missingallowExecutionStageParticipantClaim: falseto the two exact-matchcheckoutexpectations that fix(issues): fence a second run off a live-locked pending in_review stage (BLO-22666) #1366 did not update (the peer-checkout case, and the watchdog in-subtree checkout case).No production code is touched.
Verification
fb8586016, no local edits):Tests 3 failed | 234 passed (237).12ce928d3landed (fix(claude-k8s): allowlist what agent pods inherit from the server pod (BLO-22514) #1377, test(heartbeat): drop the 3s waitForStarted bounds in the resume-cap case (BLO-20885) #1380, feat(issues): authoritative open-assignment census endpoint (BLO-22785) #1333) were green.Risks
Low risk — test-only, and it asserts real behaviour rather than relaxing a check. The flag requires
issue.status === "in_review"(server/src/routes/issues.ts:12046-12056); both affected tests exercisestatus: "blocked"issues, sofalseis genuinely what the route computes at those call sites. I deliberately did not loosen these toobjectContainingin this PR — that would be a wider change to make while master is red, and it is worth discussing separately as a way to stop an additive option breaking exact-match expectations again.Model Used
claude-opus-4-5, 1M context), extended thinking, with tool use and code execution.Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template