fix(agentic): make phase-eval status cleanup race-safe - #1567
Conversation
The cleanup step removes status labels read from an event-payload snapshot, so a concurrent dispatch 404s and reds a run whose evaluation succeeded. Reproduced on PR #1541 at head 0503991 across runs 31596291515 and 31596293364, with exactly-once verified intact. Refs #1566 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R1uTFgh4emCPxSs7m72Pqf
|
[PHASE: IMPL] Harness slice bootstrapped and draft PR opened.
|
|
[PHASE: IMPL] S1 committed the pre-fix RED regression contract.
Next: add the production module and workflow adapter, then demonstrate this same suite green. |
|
[PHASE: IMPL] S2 landed the race-safe live-label transition and is ready for orchestrator review.
Gate output
RED → GREEN
NextThe Claude Opus 5 orchestrator holds review/merge authority. It should substantively review this slice, then perform draft → ready when formal IMPL-EVAL should fire. This implementation session did not trigger or self-certify IMPL-EVAL. |
|
[PHASE: IMPL] S3 addresses both orchestrator findings without weakening the trusted-base boundary.
Acceptance readingBox 1 remains truthful as written. The concurrent-removal case is handled inside the caller, so the transition completes normally and applies Gate output
The new test extracts named workflow step blocks and asserts the declared independence/diagnostic/security policy. As recorded in PR #1567 remains draft with exactly Harness evidence closure: |
|
[PHASE: REVIEW] [VERDICT: PASS] Orchestrator slice review at head Independently executedThe narrow tolerance is genuinely narrowfunction isMissingLabelError(error) {
if (!isRecord(error) || error.status !== 404) return false;
const response = error.response;
if (!isRecord(response)) return false;
const data = response.data;
return isRecord(data) && data.message === 'Label does not exist';
}Requires Two things the implementer added that I did not ask for, both correct
The structural fix, which matters more than the 404The original defect was not really the 404 — it was that bookkeeping could fail dispatch. This job Now: both bookkeeping steps are Bootstrap: superseded by the fix, and this PR now demonstrates it
So the ready flip is viable after all, and it is the better route: it exercises the new resilience on the very Acceptance reading — acceptedBox 1 stands as worded. The concurrent-removal case is handled inside the caller, so the transition Honest limit, correctly self-reportedThe new workflow-policy test extracts named step blocks and asserts the declared independence, diagnostic and Flipping to ready. |
…failing dispatch The reported defect was a label-removal race off an event snapshot. Review found two larger things: the fix self-blocked, because it imports a module absent from main so the introducing PR's own transition aborted the job and no evaluator was dispatched; and the structural defect was that housekeeping could fail dispatch at all, which is the same pathology as #1566 one level up. Resolved non-fatally and non-silently: the transition still rethrows so its outcome is truthful, an attributed step records the checkout outcome and a bounded reason, and dispatch depends on neither. The fix retires its own bootstrap problem, so #1567 demonstrates it on its own CI run. Refs #1566 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R1uTFgh4emCPxSs7m72Pqf
|
[PHASE: IMPL] S4 lands the owner-directed self-contained transition after run
Gates
PR #1567 remains draft at exactly |
|
[PHASE: REVIEW] [VERDICT: PASS — flipping to ready] Bootstrap correction verified at head The bootstrap failure is resolved by removing its cause, not by tolerating itThe dynamic import and the trusted-checkout step are gone, so there is nothing to be absent from The inline cleanup keeps every property that matteredconst liveLabels = await github.paginate(github.rest.issues.listLabelsOnIssue, …);
…
try { await github.rest.issues.removeLabel({ owner, repo, issue_number, name }); }
catch (error) {
const missingLabel = error?.status === 404 &&
error?.response?.data?.message === MISSING_LABEL_MESSAGE;
if (!missingLabel) throw error;
}Live paginated read; tolerance requires Executed at the flip headBase re-synced before the flip, deliberately
Known, deliberate residueThe workflow now carries a transcription of EvaluationFlipping to ready so the automatic DeepSeek IMPL-EVAL triggers exactly once. No |
|
@openhands-agent model=openrouter/deepseek/deepseek-v4-flash-0731 output=pr-comment iterations=800 phase=impl head=f6def9946dfc657967abca8b29468f759dddbe18 Trusted base SHA: 3c9dc1f use harness SKILL
Act as the formal IMPL-EVAL session for this pull request. Do not edit files, create commits, push, Return concise, severity-ranked findings with exact evidence and required action. End with exactly |
OpenHands Agent — CompletedOPENHANDS_VERDICT: PASS Model: IMPL-EVAL — PR #1567
|
|
OPENHANDS_VERDICT: PASS IMPL-EVAL — race-safe phase-eval status cleanup (#1567)Evaluated head Validation: escalate-tier gates pass; Findings (all non-blocking):
No unresolved review threads; reviewer PASS already recorded and PR flipped to ready. No blocking This verdict comment was generated by an AI agent (OpenHands) on behalf of the evaluating session. |
|
[EVIDENCE — Definition-of-Done box ticked by the orchestrator, with the run identities]
The implementer correctly left it unchecked — the claim was not yet true when it handed over. It is true now,
Head-matched before consuming, per this lane's rule after Ticking the box is a body edit, not a commit, so the head stays This is pre-merge gate check 7 doing its job. |
PR #1567 merged at b79eca5, closing #1566. Merged by the release coordinator during a Claude 529 outage and re-verified here rather than accepted: main contains it, the helper is present on trusted main, both items are status:shipped. The deliberate follow-up is now valid and only now: the workflow shipped a transcription rather than an import, because importing a module absent from trusted main is what made the first attempt self-block. The general shape is that a change introducing a trusted-path dependency cannot also be the change that first consumes it. Refs #1566, #1403, #1380, #1549 Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01R1uTFgh4emCPxSs7m72Pqf
Summary
Make the IMPL-EVAL status transition race-safe and self-contained: read live labels inline, narrowly tolerate only the missing-label 404, apply
status:impl-eval, and retain attributed failure diagnostics. The tested helper remains the behavioral contract; generation deduplication and evaluator dispatch logic are unchanged.Scope
Closes fix(agentic): phase-eval status cleanup removes labels from an event snapshot, so a concurrent dispatch fails the run with a 404 #1566
Slices
fe1d3b5e8,8ddc47fdf72cf4b7c24dc43e106a57170d574b3,5b4d8caf59d7ea38f1cd,c4814ffab5Validation
deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/— exit 0; 67 passed, 0 failed.deno task gen:assets-barrel— exit 0; subsequentgit status --porcelainempty on terminal headc4814ffab5.jsr:@std/yaml@^1.0.10— exit 0,YAML_PARSE_OK.deno task e2e:cli— out of scope by issue contract.Run correction and design
Run
31598386001showed that dispatch did run after the non-fatal transition failure, then failed because nostatus:impl-evallabeled-event generation existed. The dependency is through GitHub event history, not the dispatch step's declaredif:condition.The final first-landing design therefore has no checkout and no dynamic import. The workflow performs the live-label transition inline. There is deliberately no PR-head fallback.
.github/scripts/phase-eval-status.mjsremains the independently unit-tested behavioral contract; an explicitly string-based test checks that the inline transcription uses the same exact missing-label message and terminal label. Importing the helper is deferred until it is reachable from trustedmainafter this PR merges.continue-on-errorand the attributed diagnostic remain. They make failures legible and allow the dispatch attempt to expose its own generation precondition; they do not make the labeled-event generation optional.Acceptance interpretation
Issue #1566 box 1 remains truthful as written. A concurrent removal is the narrowly tolerated
404 Label does not existcase, so the transition completes normally and appliesstatus:impl-evalexactly once.Harness
.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/Drift / Debt
31598386001, the hidden event-history dependency, the corrected scope of the static workflow-policy test, and the post-merge import follow-up are recorded indrift.md.Definition of Done
status:label.