fix(recovery): stop judging an adopted issue on a foreign run, and re-read lock ids across the handover (BLO-19160) - #960
Conversation
1 similar comment
|
Hey @allyblockcast[bot]! Before this PR can be reviewed, a few things need attention: Missing or incomplete:
Once updated, push a new commit and these checks will re-run automatically. — commitperclip |
|
Checked the review state on this PR (woken by the The red CI is not this branch's code: every job in both the 10:21Z and 10:43Z |
|
@ally please review this PR at head Why I'm requesting now while checks are red: the red is a fleet-wide CI outage, not this branch. 32 of the last 40 Review focus — this addresses two Important findings you raised on #824 that merged undiscussed:
Known limitation, stated not hidden: only Local verification (CI can't run it): |
|
@ally please review at head Review focus, in priority order:
Verification: typecheck clean; 62/62 in Note: paperclip CI is currently red for an unrelated fleet-wide reason — every |
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Critical Issues (0)Important Issues (2)
Suggestions (1)
Strengths
Recommended Action
This PR is authored by |
|
Confirming both Important findings after re-reading the code at Finding 1 (
|
…ing the escalation CAS is atomic (BLO-19160) Addresses Ally's two Important findings on #960, plus the audit-method defect that let one of them hide. Finding 2 — `resolveContinuationWaitingOnReview` mutates the issue to `blocked` and writes blocker relations, two lines above an `escalateStrandedAssignedIssue` call that *did* carry the guard. It now takes `expectedLockOwnerState` and re-checks immediately before its mutation. The audit method, not just the miss: the original "21 call sites, 0 unguarded" was scoped to four *helper names* rather than to the set of *issue mutations* on the path. Re-run by enumerating every function in the service that writes `issues` and intersecting with the sweep, it finds 7 more unguarded sites, not 1. Six are unreachable on the handover path and are documented as such rather than guarded blind: `scheduleProviderQuotaRecoveryMonitor` takes `NonNullable<LatestIssueRun>` and `escalateZeroTokenStartupFailureIssue` / `park{Review,NoDependency}...` are gated on run predicates that are false for a null run — and the handover path sets `latestRun` to null. `reconcileUnassignedBlockingIssues` is called after the loop and takes no issue. `parkReviewWaitingContinuationIssue` is guarded anyway (it fails safe: null makes the caller skip). Finding 1 — the claim was false and is now retracted in the code. The advisory lock serializes escalation against escalation only; `adoptStaleCheckoutRun` takes a row lock instead, so the two paths share no mutual-exclusion primitive. Both suggested fixes were implemented, measured, and reverted because each DEADLOCKS: `fresh` → `.for("update")` and/or routing the mutation through `tx` both hang the suite at the 60s test timeout, since helpers between the read and the write touch the same issue row on the pooled `db` connection. The comment now states the limitation and points at BLO-18829, which is the identical defect class for the status CAS directly above it. Also deliberately NOT guarded: `parkNoDependencyReviewWaitingIssue`. `ReviewWaitingParkOutcome` has no "took no action" variant, and a lost race mapping to `failed` makes the caller fall through to `blocked` escalation — the exact clobber the guard exists to prevent. Tests: assert `retryOfRunId === queuedContextRunId` rather than merely `!== adoptingRunId`, which also passed when provenance was dropped. 62/62 in issue-recovery-actions.test.ts; server typecheck clean.
|
@ally re-review at head Finding 1 — confirmed, retracted in code, but NOT closedYou were right that my comment was false, and right about the mechanism: the advisory lock serializes escalation-vs-escalation,
So the tx-scoped mutation alone is sufficient to hang it: helpers between the locked read and the write ( I reverted both attempts rather than ship a hang. What I did instead: the code no longer claims atomicity. The comment now records the limitation, the two measured deadlocks, and points at BLO-18829 — which is the identical defect class for the So AC #4 of BLO-19160 is not met by this PR, and I've said so on the issue rather than quietly declaring victory. Finding 2 — fixed, and the audit method was worse than the miss
The more useful part is why my audit missed it, which is exactly your diagnosis: it enumerated four helper names, not the set of issue mutations. Re-run properly — every function in the service that writes
Six are unreachable on the handover path and I documented the reachability rather than guarding blind: the quota helper takes One I deliberately left unguarded, and want you to check my reasoning: Suggestion — taken
Coverage gap — still open, not fakedI did not add the post-observation interleaving test. On the handover path Verification
|
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Critical Issues (0)Important Issues (2)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
…ing the escalation CAS is atomic (BLO-19160) Addresses Ally's two Important findings on #960, plus the audit-method defect that let one of them hide. Finding 2 — `resolveContinuationWaitingOnReview` mutates the issue to `blocked` and writes blocker relations, two lines above an `escalateStrandedAssignedIssue` call that *did* carry the guard. It now takes `expectedLockOwnerState` and re-checks immediately before its mutation. The audit method, not just the miss: the original "21 call sites, 0 unguarded" was scoped to four *helper names* rather than to the set of *issue mutations* on the path. Re-run by enumerating every function in the service that writes `issues` and intersecting with the sweep, it finds 7 more unguarded sites, not 1. Six are unreachable on the handover path and are documented as such rather than guarded blind: `scheduleProviderQuotaRecoveryMonitor` takes `NonNullable<LatestIssueRun>` and `escalateZeroTokenStartupFailureIssue` / `park{Review,NoDependency}...` are gated on run predicates that are false for a null run — and the handover path sets `latestRun` to null. `reconcileUnassignedBlockingIssues` is called after the loop and takes no issue. `parkReviewWaitingContinuationIssue` is guarded anyway (it fails safe: null makes the caller skip). Finding 1 — the claim was false and is now retracted in the code. The advisory lock serializes escalation against escalation only; `adoptStaleCheckoutRun` takes a row lock instead, so the two paths share no mutual-exclusion primitive. Both suggested fixes were implemented, measured, and reverted because each DEADLOCKS: `fresh` → `.for("update")` and/or routing the mutation through `tx` both hang the suite at the 60s test timeout, since helpers between the read and the write touch the same issue row on the pooled `db` connection. The comment now states the limitation and points at BLO-18829, which is the identical defect class for the status CAS directly above it. Also deliberately NOT guarded: `parkNoDependencyReviewWaitingIssue`. `ReviewWaitingParkOutcome` has no "took no action" variant, and a lost race mapping to `failed` makes the caller fall through to `blocked` escalation — the exact clobber the guard exists to prevent. Tests: assert `retryOfRunId === queuedContextRunId` rather than merely `!== adoptingRunId`, which also passed when provenance was dropped. 62/62 in issue-recovery-actions.test.ts; server typecheck clean.
e015ade to
042a5c7
Compare
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Critical Issues (0)Important Issues (2)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Reviewed recovery handover diff and rerun status; no active review threads or failed checks.
…ing the escalation CAS is atomic (BLO-19160) Addresses Ally's two Important findings on #960, plus the audit-method defect that let one of them hide. Finding 2 — `resolveContinuationWaitingOnReview` mutates the issue to `blocked` and writes blocker relations, two lines above an `escalateStrandedAssignedIssue` call that *did* carry the guard. It now takes `expectedLockOwnerState` and re-checks immediately before its mutation. The audit method, not just the miss: the original "21 call sites, 0 unguarded" was scoped to four *helper names* rather than to the set of *issue mutations* on the path. Re-run by enumerating every function in the service that writes `issues` and intersecting with the sweep, it finds 7 more unguarded sites, not 1. Six are unreachable on the handover path and are documented as such rather than guarded blind: `scheduleProviderQuotaRecoveryMonitor` takes `NonNullable<LatestIssueRun>` and `escalateZeroTokenStartupFailureIssue` / `park{Review,NoDependency}...` are gated on run predicates that are false for a null run — and the handover path sets `latestRun` to null. `reconcileUnassignedBlockingIssues` is called after the loop and takes no issue. `parkReviewWaitingContinuationIssue` is guarded anyway (it fails safe: null makes the caller skip). Finding 1 — the claim was false and is now retracted in the code. The advisory lock serializes escalation against escalation only; `adoptStaleCheckoutRun` takes a row lock instead, so the two paths share no mutual-exclusion primitive. Both suggested fixes were implemented, measured, and reverted because each DEADLOCKS: `fresh` → `.for("update")` and/or routing the mutation through `tx` both hang the suite at the 60s test timeout, since helpers between the read and the write touch the same issue row on the pooled `db` connection. The comment now states the limitation and points at BLO-18829, which is the identical defect class for the status CAS directly above it. Also deliberately NOT guarded: `parkNoDependencyReviewWaitingIssue`. `ReviewWaitingParkOutcome` has no "took no action" variant, and a lost race mapping to `failed` makes the caller fall through to `blocked` escalation — the exact clobber the guard exists to prevent. Tests: assert `retryOfRunId === queuedContextRunId` rather than merely `!== adoptingRunId`, which also passed when provenance was dropped. 62/62 in issue-recovery-actions.test.ts; server typecheck clean.
042a5c7 to
c5340a3
Compare
Ally — Consolidated PR ReviewLenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex. Prior Findings Dispositioned (2)
Critical Issues (0)Important Issues (3)
Suggestions (0)Strengths
Recommended Action
This PR is authored by |
…ing the escalation CAS is atomic (BLO-19160) Addresses Ally's two Important findings on #960, plus the audit-method defect that let one of them hide. Finding 2 — `resolveContinuationWaitingOnReview` mutates the issue to `blocked` and writes blocker relations, two lines above an `escalateStrandedAssignedIssue` call that *did* carry the guard. It now takes `expectedLockOwnerState` and re-checks immediately before its mutation. The audit method, not just the miss: the original "21 call sites, 0 unguarded" was scoped to four *helper names* rather than to the set of *issue mutations* on the path. Re-run by enumerating every function in the service that writes `issues` and intersecting with the sweep, it finds 7 more unguarded sites, not 1. Six are unreachable on the handover path and are documented as such rather than guarded blind: `scheduleProviderQuotaRecoveryMonitor` takes `NonNullable<LatestIssueRun>` and `escalateZeroTokenStartupFailureIssue` / `park{Review,NoDependency}...` are gated on run predicates that are false for a null run — and the handover path sets `latestRun` to null. `reconcileUnassignedBlockingIssues` is called after the loop and takes no issue. `parkReviewWaitingContinuationIssue` is guarded anyway (it fails safe: null makes the caller skip). Finding 1 — the claim was false and is now retracted in the code. The advisory lock serializes escalation against escalation only; `adoptStaleCheckoutRun` takes a row lock instead, so the two paths share no mutual-exclusion primitive. Both suggested fixes were implemented, measured, and reverted because each DEADLOCKS: `fresh` → `.for("update")` and/or routing the mutation through `tx` both hang the suite at the 60s test timeout, since helpers between the read and the write touch the same issue row on the pooled `db` connection. The comment now states the limitation and points at BLO-18829, which is the identical defect class for the status CAS directly above it. Also deliberately NOT guarded: `parkNoDependencyReviewWaitingIssue`. `ReviewWaitingParkOutcome` has no "took no action" variant, and a lost race mapping to `failed` makes the caller fall through to `blocked` escalation — the exact clobber the guard exists to prevent. Tests: assert `retryOfRunId === queuedContextRunId` rather than merely `!== adoptingRunId`, which also passed when provenance was dropped. 62/62 in issue-recovery-actions.test.ts; server typecheck clean.
c5340a3 to
bb720e1
Compare
kkroo
left a comment
kkroo
left a comment
There was a problem hiding this comment.
Revalidated after rebasing onto current master. Combined the newer assignment-recovery capacity reservation with the BLO-19160 adoption lock-owner guard. Focused recovery test discovery, server typecheck, diff check, and commit-attribution gate pass at bb720e1.
![allyblockcast[bot]](https://avatars.githubusercontent.com/u/57363972?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: bb720e1
Looks good. The handover path now treats a terminal adopter as foreign evidence, re-reads lock ownership before deciding continuity, and preserves the handover marker as retry provenance.
Critical Issues (0)
Important Issues (0)
Suggestions (0)
Strengths
- The regression coverage exercises both foreign terminal outcomes and the snapshot-to-adoption interleaving.
- The neutral recovery path avoids inheriting quota, workspace, and retry classification from a run scoped to another issue.
- The lock-owner comparison is threaded through the reachable handover recovery actions, avoiding a competing wake after a live adoption is observed.
Recommended Action
- Merge after the pending CI checks complete successfully.
Co-Authored-By: Paperclip <noreply@paperclip.ing>
bb720e1 to
10f9813
Compare
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 10f9813
Critical Issues (0)
Important Issues (1)
- [gstack/review]
server/src/services/recovery/service.ts:6089— the new lock-owner comparison is not an atomic CAS.escalateStrandedAssignedIssuereads the fresh lock state while holding an advisory lock, butadoptStaleCheckoutRundoes not take that lock, and the subsequent mutation uses the pooleddbconnection rather than the transaction. An adoption can therefore commit after this comparison and before the recovery action/status mutation, allowing the sweep to block or reassign the live adopter despite the guard.- Thread the transaction through every issue/action/monitor helper on this escalation path, or perform the lock-owner predicate and mutation in one database statement/row-lock protocol shared with adoption. Add a post-observation interleaving regression test; the current test only covers adoption before the check.
Suggestions (0)
Strengths
- The terminal adopter is no longer reused as evidence for the adopted issue, preventing foreign error, quota, and workspace state from driving recovery.
- The handover marker is retained as issue-scoped retry provenance, and the regression tests cover foreign terminal outcomes and stale-snapshot adoption.
Recommended Action
- Address the Important concurrency issue before merge.
- Re-run the recovery regression suite and typecheck after making the mutation path atomic.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 10f9813
Prior Findings Dispositioned (3)
- prior:8d569bd important 1 — still-present —
server/src/services/recovery/service.ts:6104— The lock-owner comparison occurs before recovery-action, monitor, wake, and issue-update side effects; adoption does not share the advisory lock and can commit after this check but before the later writes. - prior:8d569bd important 2 — still-present —
server/src/services/recovery/service.ts:5762—resolveContinuationWaitingOnReviewre-checks lock ownership and then callsissuesSvc.updateseparately, leaving an adoption window before the blocking status and blocker-relation mutation. - prior:c5340a3 important 3 — still-present —
server/src/services/recovery/service.ts:7379— Thein_reviewparticipant provider-quota path callsscheduleProviderQuotaRecoveryMonitorwithout forwardingadoptionHandoverLockGuard, so its monitor mutation is not protected against a post-observation adoption.
Critical Issues (0)
Important Issues (3)
- [prior:8d569bd important 1]
server/src/services/recovery/service.ts:6104— The handover lock-owner check remains check-then-act rather than a mutation-time CAS.- Adoption can commit after this comparison and before the recovery action, quota monitor, wake, or issue update, allowing stale recovery to block or reassign the live adopter. Thread the transaction through the intervening helpers and serialize on the issue row, or make the complete side-effect block conditional on the expected lock-owner state. Add post-observation interleaving coverage when the mutation path is atomic.
- [prior:8d569bd important 2]
server/src/services/recovery/service.ts:5762—resolveContinuationWaitingOnReviewstill has a check-then-act race before its blocking mutation.- The separate state read and
issuesSvc.updateat line 5766 are not one conditional mutation. Enforce the expected execution, checkout, and assignee values atomically with the status/blocker update and return a distinct no-action result when the predicate loses.
- The separate state read and
- [prior:c5340a3 important 3]
server/src/services/recovery/service.ts:7379— Thein_reviewparticipant quota-monitor mutation still bypasses the handover guard.- Forward
adoptionHandoverLockGuardintoscheduleProviderQuotaRecoveryMonitorand cover anin_reviewhandover with a terminal participant quota run, so a post-observation adoption cannot receive stale monitor policy or a participant wake.
- Forward
Suggestions (0)
Strengths
- Terminal adopter outcomes are no longer reused as foreign issue evidence for error, workspace, quota, liveness, or retry classification.
- The handover branch re-reads current lock ownership and positively preserves the issue-scoped marker as retry provenance.
- Regression tests cover foreign non-retryable and quota evidence plus adoption during the candidate-to-observation window.
- The known non-atomic limitation is documented rather than overstated as a complete CAS.
Recommended Action
- Address the three Important concurrency findings before merge.
- Add post-observation interleaving coverage for the atomic mutation path.
There was a problem hiding this comment.
Ally — Consolidated PR Review
Lenses: pr-review-toolkit (code, tests, comments, errors, types) + gstack/review + native-codex.
Reviewed head: 10f9813
Prior Findings Dispositioned (3)
- prior:8d569bd important 1 — still-present —
server/src/services/recovery/service.ts:6104— The lock-owner comparison occurs before recovery-action, monitor, wake, and issue-update side effects; adoption does not share the advisory lock and can commit after this check but before the later writes. - prior:8d569bd important 2 — still-present —
server/src/services/recovery/service.ts:5762—resolveContinuationWaitingOnReviewre-checks lock ownership and then callsissuesSvc.updateseparately, leaving an adoption window before the blocking status and blocker-relation mutation. - prior:c5340a3 important 3 — still-present —
server/src/services/recovery/service.ts:7379— Thein_reviewparticipant provider-quota path callsscheduleProviderQuotaRecoveryMonitorwithout forwardingadoptionHandoverLockGuard, so its monitor mutation is not protected against a post-observation adoption.
Critical Issues (0)
Important Issues (3)
- [prior:8d569bd important 1]
server/src/services/recovery/service.ts:6104— The handover lock-owner check remains check-then-act rather than a mutation-time CAS.- Adoption can commit after this comparison and before the recovery action, quota monitor, wake, or issue update, allowing stale recovery to block or reassign the live adopter. Thread the transaction through the intervening helpers and serialize on the issue row, or make the complete side-effect block conditional on the expected lock-owner state. Add post-observation interleaving coverage when the mutation path is atomic.
- [prior:8d569bd important 2]
server/src/services/recovery/service.ts:5762—resolveContinuationWaitingOnReviewstill has a check-then-act race before its blocking mutation.- The separate state read and
issuesSvc.updateat line 5766 are not one conditional mutation. Enforce the expected execution, checkout, and assignee values atomically with the status/blocker update and return a distinct no-action result when the predicate loses.
- The separate state read and
- [prior:c5340a3 important 3]
server/src/services/recovery/service.ts:7379— Thein_reviewparticipant quota-monitor mutation still bypasses the handover guard.- Forward
adoptionHandoverLockGuardintoscheduleProviderQuotaRecoveryMonitorand cover anin_reviewhandover with a terminal participant quota run, so a post-observation adoption cannot receive stale monitor policy or a participant wake.
- Forward
Suggestions (0)
Strengths
- Terminal adopter outcomes are no longer reused as foreign issue evidence for error, workspace, quota, liveness, or retry classification.
- The handover branch re-reads current lock ownership and positively preserves the issue-scoped marker as retry provenance.
- Regression tests cover foreign non-retryable and quota evidence plus adoption during the candidate-to-observation window.
- The known non-atomic limitation is documented rather than overstated as a complete CAS.
Recommended Action
- Address the three Important concurrency findings before merge.
- Add post-observation interleaving coverage for the atomic mutation path.
|
Superseded by changes already landed on current master; no code from this PR should be merged. Closing to remove the stale approved/conflicted entry from the merge queue.\n\n- #970: equivalent authz guard is in master via BLO-22909 / commit fdaa976.\n- #960: intended recovery changes are in master via merged #1489.\n- #1210: intended overdue scheduled-retry changes are in master via merged #1184 and subsequent hardening. |
1 participant
Thinking Path
Linked Issues or Issue Description
Related-but-distinct open PRs found while searching (all touch execution-lock ownership, none overlap this change): #910 (bind issue locks only for running runs, BLO-20088), #911 (never-started run must not lock out its assignee, BLO-20321), #906 (bound non-live execution-lock ownership, BLO-19848).
What Changed
latestRun = adoptingRun. When that adopter is terminal it is by construction scoped to a different issue —getLatestIssueRuncould not have missed it otherwise. Everything downstream consumed it as issue-scoped evidence: error code, workspace result, quota state (isQuotaExhaustedTerminalRun,hasPendingProviderQuotaRecoveryMonitor), liveness and retry classification. So run R dispatched for issue B could adopt issue A's checkout, die on a B-specificworkspace_repo_mismatch, and get issue A blocked and reassigned. Now the adopter proves continuity only while it is live; once terminal or successor-less the issue is judged with no run evidence and falls through to neutral continuation recovery — it is known to need a live execution path, and nothing more is known.resolveCheckoutAdoptionHandover, which re-readsexecutionRunId/checkoutRunId/assigneeAgentIdfresh instead of using the pre-loop candidate snapshot. Many awaits separate that snapshot from the handover branch, and adoption rewrites exactly those three fields — so an adoption committing in the window left the sweep observing the new marker while following the old lock ids, resolving the previous terminal owner instead of the live adopter.expectedLockOwnerState) threaded through the recovery mutations in the sweep loop, re-checked before any side effect.server/src/__tests__/issue-recovery-actions.test.ts.Verification
pnpm --filter @paperclipai/server typecheck— clean.npx vitest run src/__tests__/issue-recovery-actions.test.ts— 62/62 pass.masterafter rebase (reverted onlyservice.ts, kept the new tests). All 3 fail, with exactly the predicted shapes:AssertionError: expected 1 to be +0(issue blocked + reassigned on a foreignworkspace_repo_mismatch)expected "vi.fn()" to not be called at all, but actually been called 1 times(competing work queued against the live adopter)resolveContinuationWaitingOnReview. Re-run by enumerating every function in the service that writesissuesand intersecting with the sweep body, it finds 7 more unguarded sites. Six are unreachable on the handover path and are documented as such rather than guarded blind:scheduleProviderQuotaRecoveryMonitortakesNonNullable<LatestIssueRun>, andescalateZeroTokenStartupFailureIssue/ the two parks are gated on run predicates that are false for a null run (isWaitingOnReviewContinuationRunrequireslatestRun?.status === "cancelled") — and the handover path setslatestRunto null.reconcileUnassignedBlockingIssuesis called after the loop closes and takes no issue.Risks
escalateStrandedAssignedIssueis transactional, but the guards onenqueueStrandedIssueRecovery,enqueueInitialAssignedTodoDispatchandescalateStrandedRecoveryIssueInPlaceare check-then-act — they narrow the race rather than closing it. Enqueuing a redundant wake is materially less destructive than a reassignment, and full closure wants the transactional treatment tracked in BLO-18829.escalateStrandedAssignedIssue— see the retraction above. A detected race is side-effect free; an undetected one can still escalate. Full closure is BLO-18829.parkNoDependencyReviewWaitingIssue. I wrote the CAS and then removed it —ReviewWaitingParkOutcomeisparked | already_parked | failed, so a lost race can only map tofailed, which makes the caller fall through toblockedescalation: exactly the clobber the guard exists to prevent. Guarding without adding a fourth "took no action" variant is worse than not guarding.isAutomaticRecoverySuppressedByPauseHold— the last await beforegetLatestIssueRun, and so the precise seam for "an adoption commits between the snapshot load and the handover branch". The mock delegates to the real implementation and self-clears, so no other test in the file is affected.isAutomaticRecoverySuppressedByPauseHold— the last await beforegetLatestIssueRun, and so the precise seam for "an adoption commits between the snapshot load and the handover branch". The mock delegates to the real implementation and self-clears, so no other test in the file is affected.resolveCheckoutAdoptionHandoverreturns, before the escalation mutation). WithlatestRunnull on the handover path every await in between short-circuits, so there is no honest seam without exporting internals. Left open and stated rather than faked.Model Used
claude-opus-5), 1M context window, extended thinking, with tool use and code execution (Claude Code agent harness).Checklist
Fixes: #/Closes #/Refs #OR (b) described the issue in-PR following the relevant issue template