Skip to content

fix(agentic): make phase-eval status cleanup race-safe - #1567

Merged
rickylabs merged 9 commits into
mainfrom
fix/1566-phase-eval-label-race
Aug 12, 2026
Merged

fix(agentic): make phase-eval status cleanup race-safe#1567
rickylabs merged 9 commits into
mainfrom
fix/1566-phase-eval-label-race

Conversation

@rickylabs

@rickylabs rickylabs commented Aug 12, 2026

Copy link
Copy Markdown
Owner

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

Slices

  • S0 Harness slice bootstrap — fe1d3b5e8, 8ddc47fdf
  • S1 RED regression tests — 72cf4b7c24
  • S2 Live-label cleanup implementation — dc43e106a5
  • S3 Failure attribution and dispatch-attempt visibility — 7170d574b3, 5b4d8caf59
  • S4 Self-contained inline first landing — d7ea38f1cd, c4814ffab5

Validation

  • deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/ — exit 0; 67 passed, 0 failed.
  • Scoped check — exit 0; 6 files, 0 findings.
  • Scoped lint — exit 0; 6 files, 0 findings.
  • Scoped format — exit 0; 6 files, 0 findings.
  • deno task gen:assets-barrel — exit 0; subsequent git status --porcelain empty on terminal head c4814ffab5.
  • Workflow parsed with 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 31598386001 showed that dispatch did run after the non-fatal transition failure, then failed because no status:impl-eval labeled-event generation existed. The dependency is through GitHub event history, not the dispatch step's declared if: 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.mjs remains 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 trusted main after this PR merges.

continue-on-error and 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 exist case, so the transition completes normally and applies status:impl-eval exactly once.

Harness

  • Run dir: .llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/
  • Phase: impl complete; awaiting orchestrator-owned ready transition and automatic DeepSeek IMPL-EVAL.
  • PLAN-EVAL: N/A — small deterministic fix with locked contract, scope, acceptance, and gates.
  • Do not merge until the mandatory separate-session IMPL-EVAL is complete.

Drift / Debt

  • Run 31598386001, the hidden event-history dependency, the corrected scope of the static workflow-policy test, and the post-merge import follow-up are recorded in drift.md.

Definition of Done

  • Live labels are read before status cleanup and the terminal state has exactly one status: label.
  • Only a missing-label 404 is tolerated; permission and other-resource failures are rethrown.
  • The first landing is self-contained with no checkout, dynamic import, or PR-head fallback.
  • Transition failures remain attributed and the dispatch attempt remains visible.
  • Generation deduplication remains unchanged and is covered by test.
  • All six required gates pass with recorded exit codes.
  • Separate-session IMPL-EVAL is triggered by the orchestrator-owned ready transition.
issue: 1566
entries:
  - box-index: 1
    evidence: "Inline live-label transition plus race regression: the exact missing-label 404 completes normally and applies status:impl-eval once."
  - box-index: 2
    evidence: "Pre-fix RED commit 72cf4b7c24; targeted test exited 1 before the production module, then passed after implementation."
  - box-index: 3
    evidence: "phase-eval-status.test.ts asserts 403 and unrelated-resource 404 rejection; only exact message Label does not exist is tolerated."
  - box-index: 4
    evidence: "Generation marker/existing-claim/early-return ordering regression test passes; dispatch script is unchanged."
  - box-index: 5
    evidence: "Pure decision test removes all live status-prefixed labels and adds only status:impl-eval; string parity test guards the inline terminal literal."
  - box-index: 6
    evidence: "S4 IMPL comment records 67/67 script tests and all six gate exit codes on terminal head c4814ffab5."

rickylabs and others added 2 commits August 12, 2026 14:33
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

Copy link
Copy Markdown
Owner Author

[PHASE: IMPL]

Harness slice bootstrapped and draft PR opened.

Copy link
Copy Markdown
Owner Author

[PHASE: IMPL]

S1 committed the pre-fix RED regression contract.

  • Commit: 72cf4b7c24
  • Command: deno test --allow-read .github/scripts/phase-eval-status.test.ts
  • Exit code: 1 (expected RED)
  • Failure: TS2307 Cannot find module .github/scripts/phase-eval-status.ts
  • Coverage encoded before implementation: concurrent missing-label race, narrow 403 and unrelated-404 rethrows, terminal single-status state, and unchanged generation-dedup ordering.

Next: add the production module and workflow adapter, then demonstrate this same suite green.

Copy link
Copy Markdown
Owner Author

[PHASE: IMPL]

S2 landed the race-safe live-label transition and is ready for orchestrator review.

  • Commit: dc43e106a5
  • Scope: trusted-base module import, live listLabelsOnIssue read, pure transition decision, injected API caller, and narrow missing-label 404 tolerance.
  • Dedup: the generation marker/claim/create-comment block is unchanged; its ordering guard passes.
  • Draft state: retained. Labels remain type:fix, area:tooling, priority:p2, status:impl; milestone 0.0.6.

Gate output

  1. deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/
    • exit code 0
    • ok | 65 passed | 0 failed
  2. deno run --allow-read --allow-run .llm/tools/run-deno-check.ts --root .github/scripts --ext ts
    • exit code 0
    • 6 files selected; 0 failed batches; 0 findings
  3. deno run --allow-read --allow-run .llm/tools/run-deno-lint.ts --root .github/scripts --ext ts
    • exit code 0
    • 6 files selected; 0 findings
  4. deno run --allow-read --allow-run .llm/tools/run-deno-fmt.ts --root .github/scripts --ext ts
    • initial exit code 1: one import-layout finding in the owned test
    • formatted only phase-eval-status.test.ts and phase-eval-status.mjs
    • rerun exit code 0: 6 files selected; 0 findings
  5. deno task gen:assets-barrel
    • exit code 0
    • subsequent git status --porcelain: empty, exit code 0
  6. YAML parse using deno eval --no-lock with jsr:@std/yaml@^1.0.10
    • exit code 0
    • YAML_PARSE_OK

RED → GREEN

  • RED commit 72cf4b7c24: targeted regression command exited 1 because the production module did not exist.
  • GREEN commit dc43e106a5: the same named contract suite passes 5/5.

Next

The 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.

rickylabs commented Aug 12, 2026

Copy link
Copy Markdown
Owner Author

[PHASE: IMPL]

S3 addresses both orchestrator findings without weakening the trusted-base boundary.

  • Commit: 7170d574b3
  • Checkout: still base.ref, still persist-credentials: false, now non-blocking because it serves bookkeeping only.
  • Transition: still imports only the trusted-base module, still reads live labels, and still rethrows 403/unrelated 404 errors. continue-on-error preserves a truthful failed step outcome without aborting dispatch.
  • Diagnostic: a following step writes actor, PR, head SHA, trusted-checkout outcome, and a bounded one-line failure reason to GITHUB_STEP_SUMMARY.
  • Dispatch: explicitly requires !cancelled() and a successful chain-token check; it has no dependency on checkout or transition outcomes.
  • Bootstrap: this PR cannot obtain the module from current main on its own ready event. No PR-head fallback was added. The orchestrator-owned labeled path remains the evaluation route for fix(agentic): make phase-eval status cleanup race-safe #1567.

Acceptance reading

Box 1 remains truthful as written. The concurrent-removal case is handled inside the caller, so the transition completes normally and applies status:impl-eval once. The non-fatal workflow path applies only to different failures, which remain visible as a failed transition step plus attributed summary.

Gate output

  1. Script tests — exit 0: 66 passed | 0 failed
  2. Scoped type-check — exit 0: 6 files, 0 findings
  3. Scoped lint — exit 0: 6 files, 0 findings
  4. Scoped format — exit 0: 6 files, 0 findings
  5. deno task gen:assets-barrel — exit 0; post-commit git status --porcelain empty, exit 0
  6. Workflow YAML parse — exit 0: YAML_PARSE_OK

The new test extracts named workflow step blocks and asserts the declared independence/diagnostic/security policy. As recorded in drift.md, this is static workflow-policy evidence, not a local simulation of GitHub runner semantics.

PR #1567 remains draft with exactly status:impl. No IMPL-EVAL trigger, merge, #1541 action, or CI-skip label was performed.

Harness evidence closure: 5b4d8caf59. Asset generation was rerun on that terminal head (exit 0), and git status --porcelain remained empty (exit 0).

@rickylabs

Copy link
Copy Markdown
Owner Author

[PHASE: REVIEW] [VERDICT: PASS]

Orchestrator slice review at head 5b4d8caf5. Re-verified by the reviewer, not relayed.

Independently executed

deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/   →  66 passed | 0 failed
git merge-base origin/main HEAD → e67c1ba13 == origin/main                      →  base is fresh

The narrow tolerance is genuinely narrow

function 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 404 and the specific message. A 403 from a permissions regression and a 404 for a
different resource both rethrow, each asserted by its own test. This was the easiest thing in the slice to
get wrong — a blanket catch would have passed every functional test while converting real failures into
silent passes — and it was not gotten wrong.

Two things the implementer added that I did not ask for, both correct

  1. Trusted-base checkout. The job holds issues: write; importing the module from the PR head would
    let a branch author execute code with write permissions. It checks out base.ref with
    persist-credentials: false instead. Pinning to the base branch tip rather than the recorded
    base.sha is also the right form — the same lesson as fix(agentic): resolve evaluator prompt from current trusted base #1552 and fix(ci): pull_request.base.sha is stale on long-lived PRs, so every gate that computes a range from it silently inspects the wrong changeset #1564, applied unprompted.
  2. Failure-reason sanitisation before it reaches GITHUB_STEP_SUMMARY: CR/LF/NUL stripped, capped at
    500 chars. A step summary is Markdown, so a multi-line error could break the report or inject into it.

The structural fix, which matters more than the 404

The original defect was not really the 404 — it was that bookkeeping could fail dispatch. This job
exists to dispatch exactly one evaluator; the status-label transition is housekeeping. A failure in the
housekeeping aborted the job and prevented the dispatch, producing a red run implying dispatch had gone
wrong when it had not. That is the same pathology as #1566 one level up, and fixing only the 404 would have
left it intact for the next hiccup.

Now: both bookkeeping steps are continue-on-error, the transition still rethrows so its outcome is
truthfully failure, a dedicated step records actor / PR / head SHA / trusted-checkout outcome / bounded
reason to the step summary, and dispatch is gated only on !cancelled() and the chain-token check. Not
silent, not fatal, and the checkout outcome is reported separately so the bootstrap case is distinguishable
from a permissions failure.

Bootstrap: superseded by the fix, and this PR now demonstrates it

phase-eval-status.mjs is absent on main, so this PR's own transition step will fail on
MODULE_NOT_FOUND. Before S3 that aborted the job and the PR could not obtain an evaluator at all — it
self-blocked. With continue-on-error the step fails truthfully, the attributed diagnostic fires, and
dispatch proceeds.

So the ready flip is viable after all, and it is the better route: it exercises the new resilience on the very
PR that introduces it. The orchestrator-owned labeled workaround noted in the IMPL comment is no longer
needed. Expect one failed step and an attributed summary on this PR's first evaluation run — that is the
fix working, not a regression.
It self-heals the moment this merges.

Acceptance reading — accepted

Box 1 stands as worded. The concurrent-removal case is handled inside the caller, so the transition
completes normally and applies status:impl-eval once; the non-fatal path applies only to other failures.
The implementer defended the stronger reading with a reason rather than stretching the box, which is the
right instinct.

Honest limit, correctly self-reported

The new workflow-policy test extracts named step blocks and asserts the declared independence, diagnostic and
security policy. As drift.md records, that is static workflow-policy evidence, not a simulation of runner
semantics
— it proves the YAML declares the property, not that GitHub honours it. The runner behaviour is
proven by this PR's own run. Saying so rather than counting it as behavioural coverage is the correct call.

Flipping to ready.

@rickylabs
rickylabs marked this pull request as ready for review August 12, 2026 12:50
rickylabs added a commit that referenced this pull request Aug 12, 2026
…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
@rickylabs
rickylabs marked this pull request as draft August 12, 2026 12:52

Copy link
Copy Markdown
Owner Author

[PHASE: IMPL]

S4 lands the owner-directed self-contained transition after run 31598386001 exposed the hidden labeled-event-generation dependency.

  • Implementation commit: d7ea38f1cd
  • Harness evidence commit: c4814ffab5
  • Removed the trusted checkout and dynamic import entirely.
  • The github-script step now paginates live labels inline, removes only live status: labels, tolerates only status 404 with exact response message Label does not exist, rethrows everything else, and adds only status:impl-eval.
  • Kept continue-on-error and the attributed failure summary.
  • Kept .github/scripts/phase-eval-status.mjs and its behavioral tests. A separate explicitly string-based assertion checks only transcription parity for the exact missing-label message and terminal label; it is not presented as behavioral workflow execution evidence.
  • Corrected the policy-test claim: it proves attribution and declared conditional eligibility, not satisfaction of the GitHub event-history generation dependency.
  • Recorded the helper import as a follow-up valid only after this PR merges and the helper exists on trusted main. No PR-head fallback exists.

Gates

  1. Script tests: exit 067 passed | 0 failed
  2. Scoped type-check: exit 0 — 6 files, 0 findings
  3. Scoped lint: exit 0 — 6 files, 0 findings
  4. Scoped format: exit 0 — 6 files, 0 findings
  5. deno task gen:assets-barrel: exit 0; terminal-head git status --porcelain: empty, exit 0
  6. Workflow YAML parse: exit 0YAML_PARSE_OK

PR #1567 remains draft at exactly status:impl. No waiver label, manual OpenHands trigger, ready transition, merge, or #1541 action was performed. The orchestrator owns the ready flip and automatic DeepSeek retry.

@rickylabs

Copy link
Copy Markdown
Owner Author

[PHASE: REVIEW] [VERDICT: PASS — flipping to ready]

Bootstrap correction verified at head f6def9946 (the re-synced head; the correction itself was
c4814ffab). Executed by the reviewer.

The bootstrap failure is resolved by removing its cause, not by tolerating it

grep -E "Check out trusted|await import|GITHUB_WORKSPACE|phase-eval-status.mjs"
  .github/workflows/openhands-phase-eval.yml   →  no matches

The dynamic import and the trusted-checkout step are gone, so there is nothing to be absent from
main and ERR_MODULE_NOT_FOUND cannot recur. The transition should now simply succeed, apply
status:impl-eval, emit the labeled event, and give dispatch the generation it requires — which is the
dependency that actually broke run 31598386001, not the import itself.

The inline cleanup keeps every property that mattered

const 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 404 and the exact message; everything else rethrows. Same
predicate as the helper, and no new trust surface — no checkout, no import, no PR-head code executed in a
job holding issues: write.

Executed at the flip head

deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/   →  67 passed | 0 failed
deno task gen:assets-barrel                                                    →  exit 0, git status empty
git merge-base origin/main HEAD                                                →  3c9dc1f39 == origin/main

Base re-synced before the flip, deliberately

main had moved to 3c9dc1f39 while this PR was being corrected, leaving the branch's merge-base at
e67c1ba13. Merged main in before flipping, so the base is fresh at the moment the gates compute
their ranges — this lane's standing rule since the stale-base.sha finding (#1564), where a stale base made
code-quality's two-dot range enumerate nine already-merged files from other lanes and report success having
inspected none of the PR under review. Re-syncing after a flip would move the head and invalidate the very
verdict the flip produces.

Known, deliberate residue

The workflow now carries a transcription of .github/scripts/phase-eval-status.mjs rather than importing
it. The helper and its tests remain the independently tested contract, and drift between the two is guarded
only by a string-level assertion on the tolerance message and terminal label — static evidence, not
behavioural
. Adopting the import is a follow-up that is valid only once the helper is reachable from trusted
main, i.e. after this merges. Same two-stage sequence that failed here, in the correct order.

Evaluation

Flipping to ready so the automatic DeepSeek IMPL-EVAL triggers exactly once. No impl-eval:skip — this
PR changes the evaluator dispatch path, and executing it is what found both the bootstrap self-block and the
event-history dependency between the transition and dispatch. No manual trigger.

@rickylabs
rickylabs marked this pull request as ready for review August 12, 2026 13:00
@rickylabs

Copy link
Copy Markdown
Owner Author

@openhands-agent model=openrouter/deepseek/deepseek-v4-flash-0731 output=pr-comment iterations=800 phase=impl head=f6def9946dfc657967abca8b29468f759dddbe18

Trusted base SHA: 3c9dc1f
Evaluated head SHA: f6def99

use harness

SKILL

  • netscript-harness — apply the formal IMPL-EVAL protocol and verdict vocabulary.
  • openhands-handoff — publish one machine-readable OpenHands verdict.
  • netscript-tools — run the smallest decisive repository-native gates without mutating source.
  • netscript-doctrine — apply package/plugin doctrine when the changed surface requires it.

Act as the formal IMPL-EVAL session for this pull request. Do not edit files, create commits, push,
or repair findings. The trigger metadata supplies the trusted base SHA and immutable head SHA: read
the evaluator protocol, verdict definitions, and selected profiles from that base commit, then
evaluate the PR body, linked issues, run artifacts, final diff, review threads, and architecture
debt at the immutable head. Verify the approved plan or recorded PLAN-EVAL: N/A, design
checkpoint, acceptance criteria, static/runtime/consumer gates, public surface, lock hygiene, and
false-done states. For documentation changes, also read every changed document fully and hand-test
representative executable claims.

Return concise, severity-ranked findings with exact evidence and required action. End with exactly
one supported verdict line using OPENHANDS_VERDICT: PASS, OPENHANDS_VERDICT: FAIL_FIX,
OPENHANDS_VERDICT: FAIL_RESCOPE, OPENHANDS_VERDICT: FAIL_DEBT, or
OPENHANDS_VERDICT: FAIL_PLAN. Write the same verdict to OPENHANDS_SUMMARY_PATH.

@rickylabs

rickylabs commented Aug 12, 2026

Copy link
Copy Markdown
Owner Author

OpenHands Agent — Completed

OPENHANDS_VERDICT: PASS

Model: openrouter/deepseek/deepseek-v4-flash-0731
Provider: OPENROUTER
Job status: success
Verdict source: summary-file
Verdict comment: #1567 (comment)
Summary source: agent
Housekeeping: ack=success trace=success commit-artifacts=skipped replies=skipped draft-pr=skipped
Trace: .llm/tmp/run/openhands/pr-1567/run-31599209037-1 (in the Actions artifact)
Artifact: openhands-agent-31599209037-1

IMPL-EVAL — PR #1567 fix(agentic): make phase-eval status cleanup race-safe

Evaluator session for head f6def9946dfc657967abca8b29468f759dddbe18 vs trusted base
3c9dc1f3907c605d2d30d76f5a20ade1e4754736. Verdict vocabulary enforced.

Summary

The change fixes a race in the phase-eval IMPL_EVAL status cleanup: the previous code removed
status: labels from the stale context.payload.labels snapshot, so when a concurrent run had
already removed a label, the second run's removeLabel 404'd with Label does not exist and the
step failed. The PR switches the cleanup to a live paginate(listLabelsOnIssue) read, narrow-tolerates
only the 404 + "Label does not exist" missing-label error (rethrowing everything else), and applies
the terminal status:impl-eval label — while leaving the dispatch/generation-dedup block unchanged.
The workflow wiring became self-contained (inline transition, no trusted-base checkout / dynamic
import), with the extraction helper retained as the tested behavioral contract plus a string-parity
guard. All five escalation-gate tiers, the running suite, and the doc/run artifacts were inspected.

Changes reviewed (final diff base → head)

  • .github/scripts/phase-eval-status.mjs — add applyImplEvalStatusTransition (live list → remove
    status:* → add status:impl-eval; MISSING_LABEL_MESSAGE = "Label does not exist"; narrow
    404 tolerance, rethrow otherwise).
  • .github/scripts/phase-eval-status.test.ts — race / no-race / generation / STATUS_PREFIX and
    MISSING_LABEL_MESSAGE literal guards, parity test for the inline transcription constants.
  • .github/workflows/openhands-phase-eval.yml — added Require chainable trigger token (PAT
    actions:write), inline transition replacing checkout+dynamic-import, error attribution + explicit
    failure exit, dispatch gated on the chainable token. No trusted-base import remains.
  • .llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/{implement,worklog,context-pack,drift}.md
    — run artifacts recording PLAN-EVAL N/A, design checkpoint, gate evidence, drift.

Validation performed

  • Reproduced the failing local gate (root env) and ran the suite in the supported invocations; all
    escalate-tier gates pass; .github/scripts scoped check / lint / fmt clean (0 findings).
  • Workflow YAML parses (@std/yaml); structure matches expected job/step graph; no stray
    trusted-base import / actions/checkout / await import references remain in the workflow.
  • Race semantics confirmed correct: live label read before removal, removeLabel 404 narrow-matched
    to MISSING_LABEL_MESSAGE, all other errors rethrown; single terminal status:impl-eval.
  • Generation-dedup / dispatch block unchanged (not part of this fix).
  • deno task gen:assets-barrel with LD_LIBRARY_PATH unset → exit 0; git status --porcelain empty
    (no deno.lock or repo churn).
  • CI: quality, surface-diff, classify, desktop-native, scaffold-static green on head; check-test
    pending. close-gate red is expected (issue fix(agentic): phase-eval status cleanup removes labels from an event snapshot, so a concurrent dispatch fails the run with a 404 #1566 acceptance checkboxes still unticked at impl-eval
    stage, PR not status:ready-merge) — not a blocker against PASS here.
  • Reviewer verdict PASS already recorded; PR flipped draft → ready.

Findings (severity-ranked)

  • LOW — Production helper is orphaned (accepted residue). phase-eval-status.mjs is reachable
    only from its test, not imported by the workflow; the shipped inline transcription is the live path
    and is guarded only by the two-string parity test, not by the behavioral race test. This is
    explicitly recorded in drift.md as owner-directed "known, deliberate residue" with a documented
    follow-up to adopt the import post-merge. Behavior is structurally equivalent end-to-end; string
    parity covers the invariant constants. Not a false-done; not blocking.
  • LOW — RED evidence for box 2 is "module missing," not a behavioral RED against the pre-fix
    inline logic.
    The race test fails before the change because the extracted module did not exist
    (TS2307), which is the only meaningful "fails-before" for a from-scratch extraction under the
    self-contained-first design. The test does genuinely assert race/no-race behavior. Acceptable;
    noted for review transparency.
  • INFO — Inline/helper parity is literal-only. Any future behavioral divergence between the
    inline transcription and the helper would not be caught by the parity test. Mitigated by the
    documented post-merge import follow-up and README/doctrine drift note.

No blocking findings. No review threads open on the PR (prior review threads resolved).

Responses to review / issue comments

  • Reviewed the pre-existing reviewer thread set and the body's own "Verdict: PASS" record; no
    unanswered current threads block. The documented bootstrap regression (c4814ffab) was corrected
    before head and is covered by the green .github/scripts lint/check gates.

Remaining risks

OPENHANDS_VERDICT: PASS

Run: https://github.com/rickylabs/netscript/actions/runs/31599209037

@rickylabs

Copy link
Copy Markdown
Owner Author

OPENHANDS_VERDICT: PASS

IMPL-EVAL — race-safe phase-eval status cleanup (#1567)

Evaluated head f6def994 vs trusted base 3c9dc1f. The change reads live labels
(paginate(listLabelsOnIssue)) before cleanup, narrow-tolerates only the 404 +
"Label does not exist" missing-label error (rethrowing everything else), applies terminal
status:impl-eval, and leaves generation-dedup / dispatch unchanged. Wiring is self-contained
(no trusted-base checkout / dynamic import), gated on a chainable trigger token with explicit
failure attribution.

Validation: escalate-tier gates pass; .github/scripts scoped check/lint/fmt clean; workflow
YAML parses; gen:assets-barrel → clean tree (no lock churn); CI green on head (close-gate red is
the expected pre-merge state while #1566 acceptance boxes are unticked — not a blocker here).

Findings (all non-blocking):

  • LOW — phase-eval-status.mjs is production-orphaned (test-reachable only); shipped inline path
    guarded by literal parity, not the behavioral race test. Documented owner-directed residue in
    drift.md with a post-merge import follow-up.
  • LOW — RED for box 2 is "module missing" (TS2307) rather than a behavioral RED against the pre-fix
    inline logic; acceptable for a from-scratch extraction.
  • INFO — inline/helper parity is literal-only; future behavioral divergence not auto-caught until the
    import follow-up lands.

No unresolved review threads; reviewer PASS already recorded and PR flipped to ready. No blocking
defects, false-dones, plan/scope defects, or lock regressions found.

This verdict comment was generated by an AI agent (OpenHands) on behalf of the evaluating session.

@rickylabs

Copy link
Copy Markdown
Owner Author

[EVIDENCE — Definition-of-Done box ticked by the orchestrator, with the run identities]

close-gate was red on one unchecked PR-body box, not on the issue acceptance (all six #1566 boxes mirrored
cleanly). The box was:

Separate-session IMPL-EVAL is triggered by the orchestrator-owned ready transition.

The implementer correctly left it unchecked — the claim was not yet true when it handed over. It is true now,
and this is the evidence rather than an assertion:

Fact Value
Ready transition performed by the orchestrator, not the implementer
Trigger markers at this head exactly 1gen=29340872564 phase=impl head=f6def9946
Evaluator run 31599209037
Verdict PASS
PR head at verdict, and now f6def9946 — unchanged

Head-matched before consuming, per this lane's rule after gh-watch reported a superseded run's PASS as
terminal in 0 seconds on PR #1560. A verdict comment proves nothing until its head is compared against the
live PR head.

Ticking the box is a body edit, not a commit, so the head stays f6def9946 and the verdict it was issued
against remains the verdict for what merges. That ordering matters: pushing instead would have moved the head
and invalidated the PASS — which is how PR #1560 needed two IMPL-EVAL cycles.

This is pre-merge gate check 7 doing its job. close-gate validates issue boxes; the PR body's own
checklist is separate, and the one box that could not have been true at hand-off is exactly the one it caught.

@rickylabs
rickylabs merged commit b79eca5 into main Aug 12, 2026
46 of 48 checks passed
rickylabs added a commit that referenced this pull request Aug 12, 2026
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
@github-actions github-actions Bot added the canary:0.0.6-canary.3 Published NetScript prerelease 0.0.6-canary.3 label Aug 12, 2026
@rickylabs
rickylabs deleted the fix/1566-phase-eval-label-race branch August 13, 2026 13:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(agentic): phase-eval status cleanup removes labels from an event snapshot, so a concurrent dispatch fails the run with a 404

1 participant