diff --git a/.github/scripts/phase-eval-status.mjs b/.github/scripts/phase-eval-status.mjs new file mode 100644 index 0000000000..7348d6f4ff --- /dev/null +++ b/.github/scripts/phase-eval-status.mjs @@ -0,0 +1,59 @@ +/** Prefix reserved for the single lifecycle status label. */ +export const STATUS_PREFIX = 'status:'; + +/** Terminal status entered before dispatching IMPL-EVAL. */ +export const IMPL_EVAL_STATUS = 'status:impl-eval'; + +/** GitHub's exact response message when a label is absent from an issue. */ +export const MISSING_LABEL_MESSAGE = 'Label does not exist'; + +/** + * Decide the idempotent status-label transition from a live issue-label set. + * + * @param {readonly string[]} liveLabels + * @returns {{ remove: string[], add: string[] }} + */ +export function decideImplEvalStatusTransition(liveLabels) { + return { + remove: liveLabels.filter((label) => label.startsWith(STATUS_PREFIX)), + add: [IMPL_EVAL_STATUS], + }; +} + +/** + * Apply the IMPL-EVAL transition through injected GitHub label operations. + * + * @param {{ + * listLabelsOnIssue: () => Promise, + * removeLabel: (label: string) => Promise, + * addLabels: (labels: string[]) => Promise, + * }} operations + */ +export async function applyImplEvalStatusTransition(operations) { + const liveLabels = await operations.listLabelsOnIssue(); + const decision = decideImplEvalStatusTransition(liveLabels); + + for (const label of decision.remove) { + try { + await operations.removeLabel(label); + } catch (error) { + if (!isMissingLabelError(error)) throw error; + } + } + + await operations.addLabels(decision.add); +} + +/** @param {unknown} error */ +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 === MISSING_LABEL_MESSAGE; +} + +/** @param {unknown} value */ +function isRecord(value) { + return typeof value === 'object' && value !== null; +} diff --git a/.github/scripts/phase-eval-status.test.ts b/.github/scripts/phase-eval-status.test.ts new file mode 100644 index 0000000000..4f1fd9f923 --- /dev/null +++ b/.github/scripts/phase-eval-status.test.ts @@ -0,0 +1,166 @@ +import { assertEquals, assertRejects, assertStringIncludes } from '@std/assert'; +import { + applyImplEvalStatusTransition, + decideImplEvalStatusTransition, + IMPL_EVAL_STATUS, + MISSING_LABEL_MESSAGE, +} from './phase-eval-status.mjs'; + +interface IssueLabelOperations { + listLabelsOnIssue(): Promise; + removeLabel(label: string): Promise; + addLabels(labels: string[]): Promise; +} + +function workflowStep(source: string, name: string): string { + const lines = source.split('\n'); + const start = lines.indexOf(` - name: ${name}`); + if (start < 0) throw new Error(`Missing workflow step: ${name}`); + let end = lines.length; + for (let index = start + 1; index < lines.length; index += 1) { + if (lines[index].startsWith(' - name: ')) { + end = index; + break; + } + } + return lines.slice(start, end).join('\n'); +} + +function operations( + labels: string[], + removeError?: (label: string) => unknown, +) { + const removed: string[] = []; + const added: string[][] = []; + const client: IssueLabelOperations = { + listLabelsOnIssue: () => Promise.resolve(labels), + removeLabel: (label: string) => { + const error = removeError?.(label); + if (error !== undefined) return Promise.reject(error); + removed.push(label); + return Promise.resolve(); + }, + addLabels: (next: string[]) => { + added.push(next); + return Promise.resolve(); + }, + }; + return { client, removed, added }; +} + +Deno.test('race regression: a concurrently removed status label does not fail cleanup', async () => { + const { client, removed, added } = operations( + ['status:impl', 'area:tooling'], + () => ({ status: 404, response: { data: { message: 'Label does not exist' } } }), + ); + + await applyImplEvalStatusTransition(client); + + assertEquals(removed, []); + assertEquals(added, [['status:impl-eval']]); +}); + +Deno.test('narrow tolerance: permission failures still fail cleanup', async () => { + const { client } = operations( + ['status:impl'], + () => ({ + status: 403, + response: { data: { message: 'Resource not accessible by integration' } }, + }), + ); + + await assertRejects(() => applyImplEvalStatusTransition(client)); +}); + +Deno.test('narrow tolerance: an unrelated 404 still fails cleanup', async () => { + const { client } = operations( + ['status:impl'], + () => ({ status: 404, response: { data: { message: 'Not Found' } } }), + ); + + await assertRejects(() => applyImplEvalStatusTransition(client)); +}); + +Deno.test('terminal decision contains exactly one status label', () => { + const decision = decideImplEvalStatusTransition([ + 'type:fix', + 'status:impl', + 'status:plan-eval', + 'area:tooling', + ]); + + assertEquals(decision, { + remove: ['status:impl', 'status:plan-eval'], + add: ['status:impl-eval'], + }); + const terminal = [ + 'type:fix', + 'area:tooling', + ...decision.add, + ]; + assertEquals(terminal.filter((label) => label.startsWith('status:')), ['status:impl-eval']); +}); + +Deno.test('generation deduplication remains before trigger creation', async () => { + const workflow = await Deno.readTextFile('.github/workflows/openhands-phase-eval.yml'); + const marker = + 'const marker = ``;'; + const claim = "String(comment.body ?? '').includes(marker)"; + const earlyReturn = 'if (existing) {'; + const create = 'github.rest.issues.createComment({'; + + assertStringIncludes(workflow, marker); + assertStringIncludes(workflow, claim); + assertStringIncludes(workflow, earlyReturn); + assertStringIncludes(workflow, create); + assertEquals(workflow.indexOf(marker) < workflow.indexOf(claim), true); + assertEquals(workflow.indexOf(claim) < workflow.indexOf(earlyReturn), true); + assertEquals(workflow.indexOf(earlyReturn) < workflow.indexOf(create), true); +}); + +Deno.test('status bookkeeping failures are attributed and dispatch remains conditionally eligible', async () => { + const workflow = await Deno.readTextFile('.github/workflows/openhands-phase-eval.yml'); + const transition = workflowStep(workflow, 'Enter IMPL-EVAL status on ready transition'); + const diagnostic = workflowStep( + workflow, + 'Record attributed IMPL-EVAL status-transition failure', + ); + const dispatch = workflowStep(workflow, 'Resolve and dispatch exactly one evaluator'); + + assertStringIncludes(transition, 'id: enter_impl_eval_status'); + assertStringIncludes(transition, 'continue-on-error: true'); + assertStringIncludes(transition, 'core.setOutput('); + assertStringIncludes(transition, "'failure_reason'"); + assertStringIncludes(diagnostic, "steps.enter_impl_eval_status.outcome == 'failure'"); + assertStringIncludes(diagnostic, 'evaluator dispatch attempt continues'); + assertStringIncludes(diagnostic, 'REQUEST_ACTOR: ${{ github.actor }}'); + assertStringIncludes(diagnostic, 'FAILURE_REASON:'); + assertStringIncludes(dispatch, '!cancelled()'); + assertStringIncludes( + dispatch, + "steps.require_chainable_trigger_token.outcome == 'success'", + ); + assertEquals(dispatch.includes('enter_impl_eval_status.outcome'), false); +}); + +Deno.test('inline cleanup transcription matches helper contract literals', async () => { + const workflow = await Deno.readTextFile('.github/workflows/openhands-phase-eval.yml'); + const transition = workflowStep(workflow, 'Enter IMPL-EVAL status on ready transition'); + + assertEquals(workflow.includes('Check out trusted phase-eval scripts'), false); + assertEquals(transition.includes('await import('), false); + assertStringIncludes(transition, 'github.rest.issues.listLabelsOnIssue'); + assertStringIncludes( + transition, + `const IMPL_EVAL_STATUS = '${IMPL_EVAL_STATUS}';`, + ); + assertStringIncludes( + transition, + `const MISSING_LABEL_MESSAGE = '${MISSING_LABEL_MESSAGE}';`, + ); + assertStringIncludes( + transition, + 'error?.response?.data?.message === MISSING_LABEL_MESSAGE', + ); + assertStringIncludes(transition, 'labels: [IMPL_EVAL_STATUS]'); +}); diff --git a/.github/workflows/openhands-phase-eval.yml b/.github/workflows/openhands-phase-eval.yml index 2dea9f810a..f3b5a5ea96 100644 --- a/.github/workflows/openhands-phase-eval.yml +++ b/.github/workflows/openhands-phase-eval.yml @@ -57,6 +57,7 @@ jobs: } >> "$GITHUB_STEP_SUMMARY" - name: Require chainable trigger token + id: require_chainable_trigger_token if: env.SKIP_IMPL != 'true' env: CHAIN_TOKEN: ${{ secrets.PAT_TOKEN }} @@ -67,24 +68,85 @@ jobs: fi - name: Enter IMPL-EVAL status on ready transition - if: env.SKIP_IMPL != 'true' && github.event.action == 'ready_for_review' + id: enter_impl_eval_status + if: >- + !cancelled() && + env.SKIP_IMPL != 'true' && + github.event.action == 'ready_for_review' && + steps.require_chainable_trigger_token.outcome == 'success' + continue-on-error: true uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: # This transition must use the same chainable token as dispatch. Repository Actions # policy may make GITHUB_TOKEN read-only even when workflow permissions request writes. github-token: ${{ secrets.PAT_TOKEN }} script: | - const owner = context.repo.owner; - const repo = context.repo.repo; - const issue_number = context.payload.pull_request.number; - const labels = context.payload.pull_request.labels.map((label) => label.name); - for (const label of labels.filter((name) => name.startsWith('status:'))) { - await github.rest.issues.removeLabel({ owner, repo, issue_number, name: label }); + try { + const owner = context.repo.owner; + const repo = context.repo.repo; + const issue_number = context.payload.pull_request.number; + const STATUS_PREFIX = 'status:'; + const IMPL_EVAL_STATUS = 'status:impl-eval'; + const MISSING_LABEL_MESSAGE = 'Label does not exist'; + const liveLabels = await github.paginate( + github.rest.issues.listLabelsOnIssue, + { owner, repo, issue_number, per_page: 100 }, + ); + for (const { name } of liveLabels) { + if (!name.startsWith(STATUS_PREFIX)) continue; + 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; + } + } + await github.rest.issues.addLabels({ + owner, + repo, + issue_number, + labels: [IMPL_EVAL_STATUS], + }); + } catch (error) { + const reason = (error instanceof Error ? error.message : String(error)) + .replace(/[\r\n\0]+/g, ' ') + .slice(0, 500); + core.setOutput( + 'failure_reason', + reason, + ); + throw error; } - await github.rest.issues.addLabels({ owner, repo, issue_number, labels: ['status:impl-eval'] }); + + - name: Record attributed IMPL-EVAL status-transition failure + if: >- + !cancelled() && + env.SKIP_IMPL != 'true' && + github.event.action == 'ready_for_review' && + steps.enter_impl_eval_status.outcome == 'failure' + env: + PR_NUMBER: ${{ github.event.pull_request.number }} + HEAD_SHA: ${{ github.event.pull_request.head.sha }} + REQUEST_ACTOR: ${{ github.actor }} + FAILURE_REASON: ${{ steps.enter_impl_eval_status.outputs.failure_reason }} + run: | + { + echo '## OpenHands phase evaluation' + echo + echo '**Status:** IMPL-EVAL status transition failed; evaluator dispatch attempt continues' + echo + printf -- '- Who: `@%s`\n' "$REQUEST_ACTOR" + printf -- '- PR: `#%s`\n' "$PR_NUMBER" + printf -- '- Head SHA: `%s`\n' "$HEAD_SHA" + printf -- '- Reason: `%s`\n' "$FAILURE_REASON" + } >> "$GITHUB_STEP_SUMMARY" - name: Resolve and dispatch exactly one evaluator - if: env.SKIP_IMPL != 'true' + if: >- + !cancelled() && + env.SKIP_IMPL != 'true' && + steps.require_chainable_trigger_token.outcome == 'success' uses: actions/github-script@3a2844b7e9c422d3c10d287c895573f7108da1b3 # v9 with: github-token: ${{ secrets.PAT_TOKEN }} diff --git a/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/context-pack.md b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/context-pack.md new file mode 100644 index 0000000000..21dd8e1ba4 --- /dev/null +++ b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/context-pack.md @@ -0,0 +1,44 @@ +# PR-F #1566 Context Pack + +## Objective + +Make the ready-for-review IMPL-EVAL status cleanup read live labels and tolerate only the specific +missing-label 404 race, while preserving generation deduplication and the single-status taxonomy. + +## Current state + +- Branch is based on `e67c1ba13` (`origin/main`). +- The supplied implementation brief is committed at `fe1d3b5e8` and pushed. +- Live issue #1566 has six acceptance boxes; PR evidence must map them with `box-index: 1..6`. +- Draft PR #1567 is open with `type:fix`, `area:tooling`, `priority:p2`, `status:impl`, and milestone + `0.0.6`; it remains draft. +- The S1 test file defines the extracted module contract, race regression, narrow 403/unrelated-404 + failures, terminal single-status state, and a guard for the unchanged generation-dedup ordering. +- S2 implementation and all functional/static gates are green. The asset generator produced no + generated-file drift; a final post-commit clean status remains to capture before handoff. +- Orchestrator review found that bookkeeping failures could still abort dispatch and that this PR + cannot bootstrap the trusted-base module on its own ready event. The labeled evaluation path is + orchestrator-owned; the review-fix slice makes checkout/transition failures non-blocking and + attributed without weakening the trusted-base boundary. +- Review-fix commit `7170d574b3` is pushed. Gates are green: 66 script tests plus scoped + check/lint/format, YAML parsing, and post-commit asset generation with an empty working tree. +- The PR body and S3 phase comment state the box-1 interpretation and bootstrap limitation. PR + #1567 remains draft with exactly `status:impl` and milestone `0.0.6`. +- Run `31598386001` showed the hidden event-history dependency: dispatch ran after the non-fatal + transition failure, then failed because no `status:impl-eval` labeled-event generation existed. + The owner-directed next landing removes checkout/import and transcribes the tested cleanup inline; + the helper and unit tests remain. +- Self-contained implementation commit `d7ea38f1cd` is pushed. All six local gates are green: 67 + script tests, scoped check/lint/format, YAML parsing, and post-commit asset generation followed by + an empty working-tree proof. + +## Locked boundaries + +Only the phase-eval workflow, `.github/scripts/`, and this slice directory may change. Do not alter +dispatch deduplication, triggers, conditions, model/trusted-base logic, #1564, or PR #1541. Do not +merge or mark the PR ready. + +## Next action + +Update the PR body/evidence and phase comment, then stop. The orchestrator owns the ready flip and +automatic DeepSeek retry; this agent must not trigger or merge. diff --git a/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/drift.md b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/drift.md new file mode 100644 index 0000000000..b31baa606f --- /dev/null +++ b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/drift.md @@ -0,0 +1,23 @@ +# PR-F #1566 Drift Log + +## Entries + +- 2026-08-12: The orchestrator-provided first bootstrap commit contained `implement.md` only. + Completed the mandatory harness artifact set in an immediate bootstrap follow-up before tests or + implementation; no product scope changed. +- 2026-08-12: Orchestrator review identified a bootstrap limitation and widened the reliability + invariant from one known 404 to all status-bookkeeping failures. `phase-eval-status.mjs` is absent + on `origin/main`, so PR #1567 cannot import it from the trusted base during its own ready event; + the orchestrator will evaluate this PR through the labeled path. The durable fix keeps trusted + base execution and makes checkout/transition failures attributed but non-blocking for dispatch. + The new independence test statically validates named workflow step contracts and dependencies; + it cannot simulate GitHub Actions runner status semantics locally, so its evidence is policy + structure plus YAML parsing rather than an end-to-end Actions execution. +- 2026-08-12: Run `31598386001` corrected the prior interpretation. Dispatch was conditionally + eligible and did run, but its data dependency on a `status:impl-eval` labeled-event generation + made successful dispatch impossible after transition failure. The static policy test's recorded + limitation was decisive; it is retained but no longer cited as end-to-end independence evidence. + Owner directed a self-contained first landing: inline the tested cleanup in the workflow and keep + the helper as its independently tested contract. Importing the helper is deferred to a follow-up + only after this PR merges and the helper is reachable from trusted `main`; no PR-head fallback is + permitted. diff --git a/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/implement.md b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/implement.md new file mode 100644 index 0000000000..2cdbd92e8c --- /dev/null +++ b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/implement.md @@ -0,0 +1,160 @@ +use harness + +# PR-F — #1566: phase-eval status cleanup races on an event snapshot and 404s the run + +You are the **implementation agent** for a small, deterministic automation fix. The defect is fully +characterised and reproduced; your job is the fix plus the tests that prove it, not investigation. + +Your orchestrator is a Claude Opus 5 high session in `/home/codex/repos/netscript-006-internals`. It +holds merge authority. + +## SKILL + +- `netscript-harness` — run artifacts, slice discipline, commit trail. +- `netscript-tools` — scoped validation wrappers; what is a verdict and what is not. +- `netscript-pr` — branch/PR/label mechanics, closing keywords, the fenced `acceptance-evidence` block. +- `openhands-handoff` — the phase-eval dispatch contract you must not break. +- `rtk` — prefix read-heavy `git`/`gh`/`grep`. + +## Identity + +| Field | Value | +| --- | --- | +| Worktree | `/home/codex/repos/ns006-labelrace` | +| Branch | `fix/1566-phase-eval-label-race` | +| Base | `e67c1ba13` (= `origin/main`) | +| Slice dir | `.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/` | +| Closes | #1566 | +| Route | Codex · gpt-5.6-sol · **low** | + +Work only in that worktree. No rebase, no force-push. Push with an explicit refspec. + +## The defect, already reproduced — do not re-investigate + +`.github/workflows/openhands-phase-eval.yml`, step **"Enter IMPL-EVAL status on ready transition"**: + +```js +const labels = context.payload.pull_request.labels.map((label) => label.name); +for (const label of labels.filter((name) => name.startsWith('status:'))) { + await github.rest.issues.removeLabel({ owner, repo, issue_number, name: label }); +} +await github.rest.issues.addLabels({ owner, repo, issue_number, labels: ['status:impl-eval'] }); +``` + +`context.payload.pull_request.labels` is a **snapshot from event-creation time**; the loop then issues +unconditional deletes against live state. + +Observed on PR #1541, head `0503991ab`, two dispatch runs **two seconds apart**: + +```text +31596291515 failure 12:24:36Z DELETE …/issues/1541/labels/status%3Aimpl - 404 +31596293364 success 12:24:38Z posted the authoritative trigger +``` + +**Exactly-once was not violated** — verified: one trigger marker for that head, +`generation=29339092792`. The generation dedup worked. Only the cleanup raced. So this is a **false red on +a PR whose evaluation succeeded**, which is the stimulus that teaches operators to discount red runs. It has +already cost this milestone an investigation into an apparent duplicate evaluator run that turned out not to +exist. + +## Design decision, made for you — the logic must become testable + +#1566's acceptance requires a **race regression test**. Inline `actions/github-script` JS in a workflow +cannot be unit-tested. So extract the decision into a checked-in module under `.github/scripts/` and have +the workflow call it — following the precedent already in that directory (`ci-classify-changes.ts` + +`ci-classify-changes.test.ts`, `draft-workflow-policy.test.ts`, `e2e-cli-event-policy.test.ts`). + +Shape that keeps it testable: a **pure** function deciding *which labels to remove and add* given the live +label set, plus a thin caller that performs the API calls and applies the narrow error tolerance. Inject the +GitHub client (or just the two operations you need) so the test can drive a client that throws on demand. +Do not invent a framework; match the neighbouring files' style. + +## Contract + +### C1 — cleanup is idempotent + +Read **live** labels immediately before removing (`issues.listLabelsOnIssue`) and remove only what is +actually present. Reading live is the primary fix; the tolerance in C2 is the belt to that braces, because +even a live read is racy in principle. + +### C2 — tolerance is narrow, and this is the part that is easy to get wrong + +Tolerate **only** a `404` that means *this label is not on this issue*. Rethrow everything else. + +A blanket `try { … } catch { }` around `removeLabel` **fails this slice**. It would swallow a `403` from a +permissions regression and a `404` from a wrong `issue_number`, converting a real failure into a silent +pass — which is the same false-green class the whole 0.0.6 internals lane exists to remove. Do not ship the +convenient version. + +Leave the workflow's `retry-exempt-status-codes` behaviour untouched; 404 being retry-exempt is correct. + +### C3 — generation deduplication is unchanged + +It already works under a genuine race. Do not restructure it, do not "improve" it. Prove it still holds +(acceptance box 4). + +### C4 — terminal state is exactly one `status:` label + +Per the taxonomy's single-status rule. After cleanup, `status:impl-eval` and nothing else `status:`-prefixed. + +## Acceptance mapping + +#1566 has **6** boxes. Read them from the live issue. Provide a fenced `acceptance-evidence` block in the PR +body using **`box-index: 1..6`** — **not** exact box text. Reason, learned expensively on PR #1560 two hours +ago: `acceptanceCheckboxes` keeps only each checkbox's **first raw line, backticks preserved**, so any box +that wraps in the issue body is unmatchable by exact text, and the author cannot see the wrapping. `box-index` +is stable against it. Do not repeat that failure. + +Box 2 is **proven RED** and box 3 is a **narrow-tolerance assertion** — both need tests that fail before your +change. Commit the failing tests, then the fix. + +## Gates — deliverables, not a checklist + +Paste real output with exit codes into your per-slice PR comment. + +| # | Gate | Command | +| --- | --- | --- | +| 1 | script tests | `deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/` | +| 2 | scoped type-check | `.llm/tools/run-deno-check.ts --root .github/scripts --ext ts` | +| 3 | scoped lint | `.llm/tools/run-deno-lint.ts --root .github/scripts --ext ts` | +| 4 | scoped format | `.llm/tools/run-deno-fmt.ts --root .github/scripts --ext ts` | +| 5 | **asset-barrel freshness** | `deno task gen:assets-barrel`, then `git status --porcelain` **must be empty** | +| 6 | workflow YAML still parses | confirm the edited workflow loads (a `gh workflow view` or a YAML parse is fine) | + +Gate 5 is not optional and is not obvious: tool sources are embedded as strings in +`packages/cli/src/kernel/assets/*.generated.ts`, so touching a bundled file makes them stale and reds +`ci.yml`'s `quality` job. This cost PR-E a full CI cycle. Run it **before** you consider the slice done — +if `.github/scripts/` turns out not to be bundled, the command is a no-op and costs nothing, and the empty +`git status` is your proof either way. + +`deno task e2e:cli` is out of scope. Do **not** apply `ci:skip-e2e`/`ci:skip-scaffold` yourself; the +orchestrator decides labels. + +## PR mechanics + +1. First commit is the slice-dir bootstrap; open the **draft PR** in that same session and comment per slice. +2. Slice artifacts in `.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/`: + `worklog.md`, `context-pack.md`, `drift.md`, updated in the **same commit** as the code they describe. +3. `## Scope` carries `Closes #1566` on its own line. Nothing else gets a closing keyword. +4. Labels: `type:fix`, `area:tooling`, `priority:p2`, `status:impl`, milestone `0.0.6`. Exactly one + `status:`. Do **not** apply `status:ready-merge` or `status:impl-eval`. +5. **Leave the PR in draft.** Draft → ready fires the formal IMPL-EVAL and is the orchestrator's action. +6. Resolve commit hashes in a **separate shell step** and paste the literal value — a previous slice posted + `(git rev-parse --short=10 HEAD)` unexpanded into two comments. + +## Boundaries + +- Touch only `.github/workflows/openhands-phase-eval.yml`, new/edited files under `.github/scripts/`, and + your slice dir. +- Do **not** change the workflow's `on:` triggers or dispatch conditions. +- Do **not** touch model resolution, the `eval:model:*` mapping, or the trusted-base-ref logic from #1552. +- Do **not** retrigger PR #1541 or comment on it. Its evaluation succeeded and its trigger is authoritative. +- Do **not** widen into #1564 (stale `base.sha` range computations). Same defect class, different surface. +- Do **not** add `deno-lint-ignore`, `@ts-ignore`, `as any`, `as unknown as`, or `quality-allow:`. +- Do **not** merge. + +## Escalate instead of going idle + +If a gate is red and you cannot green it, or a contract here is wrong, write it in your slice `drift.md`, +post it as a PR comment, and continue with what is not blocked. On this lane escalation has twice found the +orchestrator's brief wrong rather than the code — that is a good outcome, so raise it. diff --git a/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/worklog.md b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/worklog.md new file mode 100644 index 0000000000..cd1f7ca60b --- /dev/null +++ b/.llm/runs/release-0.0.6-internals--orchestration/slices/pr-f-1566/worklog.md @@ -0,0 +1,155 @@ +# PR-F #1566 Worklog + +## Identity + +- Worktree: `/home/codex/repos/ns006-labelrace` +- Branch: `fix/1566-phase-eval-label-race` +- Base: `e67c1ba13` +- Implementation route: Codex · GPT-5.6 Sol · low +- Supervising orchestrator: Claude · Opus 5 · high, `/home/codex/repos/netscript-006-internals` + +## Plan gate + +PLAN-EVAL: N/A. This is a small deterministic automation fix with the defect, design, scope, +acceptance criteria, and required gates fully specified in issue #1566 and `implement.md`. + +## Design + +- Public surface: `.github/scripts/` exports a pure label-transition decision and a thin injected + GitHub-operation caller; the workflow imports and invokes the caller. +- Domain vocabulary: live issue-label names, the `status:` prefix, the terminal + `status:impl-eval` label, and the missing-label REST error classification. +- Ports: injected `listLabelsOnIssue`, `removeLabel`, and `addLabels` operations provide the only + external seam used by tests and the workflow caller. +- Constants: the status prefix and terminal label are named module constants. +- Commit slices: + 1. Bootstrap the tracked slice artifacts and draft PR. + 2. Add RED regression tests for the removal race, narrow tolerance, generation dedup, and terminal + single-status state. + 3. Implement live-label cleanup and workflow integration, then run the six required gates. +- Deferred scope: generation dispatch logic, workflow triggers/conditions, model resolution, + trusted-base resolution, #1564 range computation, and PR #1541 are unchanged. +- Contributor path: extend the decision/caller module and its adjacent test file; keep workflow + inline code limited to client adaptation and invocation. + +## Progress + +- Bootstrap: complete; draft PR #1567 opened with required metadata and six indexed acceptance + mappings. +- Tests: RED regression suite added. Before implementation it fails because the extracted + `phase-eval-status.ts` production module does not yet exist; the named race and narrow-tolerance + assertions define the required caller contract. Generation dedup is guarded structurally in the + unchanged workflow script. +- Implementation: complete and pushed through the orchestrator review fix. +- Gates: complete, including post-commit asset-barrel generation and empty-status proof. +- IMPL-EVAL: owned by the separate orchestrator/evaluator transition; this agent leaves the PR draft. + +## RED evidence + +- `deno test --allow-read .github/scripts/phase-eval-status.test.ts` — exit 1 before the production + module exists (`TS2307 Cannot find module .github/scripts/phase-eval-status.ts`). + +## Implementation + +- Added `phase-eval-status.mjs`: pure transition decision plus an injected operation caller. +- The caller reads live labels, removes only live `status:` labels, tolerates only an Octokit-style + `404` with response message `Label does not exist`, and adds `status:impl-eval` once. +- The workflow checks out the live protected base ref with credentials disabled, imports that + trusted module, and adapts the existing GitHub client operations. Dispatch/dedup code is unchanged. +- Targeted regression suite after implementation: 5 passed, 0 failed. + +## Gate evidence + +| Gate | Result | +| --- | --- | +| Script tests | `deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/` — exit 0; 65 passed, 0 failed | +| Scoped type-check | `deno run --allow-read --allow-run .llm/tools/run-deno-check.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Scoped lint | `deno run --allow-read --allow-run .llm/tools/run-deno-lint.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Scoped format | Initial exit 1 on the new test import layout; formatted only the owned test/module, rerun exit 0; 6 files, 0 findings | +| Asset barrel | `deno task gen:assets-barrel` — exit 0; no generated file appeared in status. Final clean-status proof runs after the implementation commit. | +| Workflow YAML | `deno eval --no-lock` with `jsr:@std/yaml@^1.0.10` parsed `openhands-phase-eval.yml` — exit 0, `YAML_PARSE_OK` | + +## Reconcile + +- Slice S1: issue #1566 remained open; PR #1567 carries the sole closing keyword, six indexed + evidence entries, and exactly one lifecycle label (`status:impl`). No new comments changed scope. +- Slice S2: no issue/PR feedback required readjustment. The PR remains draft for orchestrator-owned + IMPL-EVAL; no skip, ready-merge, or impl-eval label was applied. + +## Orchestrator review fix + +- Finding 1 confirmed: because `phase-eval-status.mjs` is not yet on `main`, this PR's own + ready-for-review event cannot import the trusted-base module. The orchestrator will use the + existing labeled path for this PR's evaluation; this implementation does not trigger it. +- Finding 2 accepted: evaluator dispatch is the primary work; status mutation and its trusted + checkout are bookkeeping. Both bookkeeping steps now use `continue-on-error`, while the dispatch + step explicitly depends only on a successful chain-token check and `!cancelled()`—not on checkout + or transition outcomes. This preserves a hard failure when the required PAT is absent. +- The transition catches its error only to publish a `failure_reason` output, then rethrows so the + step retains a truthful failure outcome. A following attributed summary step records actor, PR, + head, checkout outcome, and reason before dispatch proceeds. +- Static regression coverage extracts the named workflow step blocks and asserts the non-blocking + edges, diagnostic fields, trusted-base/credential boundary, and absence of bookkeeping outcome + dependencies from dispatch. This proves the declared workflow policy; it is not a GitHub runner + simulation. +- Acceptance reading: live issue #1566 box 1 remains truthful. Its specific concurrent-removal race + is narrowly tolerated inside the caller, so that transition completes normally and applies + `status:impl-eval` exactly once. Other errors still fail the transition step truthfully but no + longer suppress evaluator dispatch. + +### Review-fix gate evidence + +| Gate | Result | +| --- | --- | +| Script tests | `deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/` — exit 0; 66 passed, 0 failed | +| Scoped type-check | `deno run --allow-read --allow-run .llm/tools/run-deno-check.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Scoped lint | `deno run --allow-read --allow-run .llm/tools/run-deno-lint.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Scoped format | `deno run --allow-read --allow-run .llm/tools/run-deno-fmt.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Asset barrel | `deno task gen:assets-barrel` — exit 0; after review-fix commit `7170d574b3`, `git status --porcelain` was empty with exit 0 | +| Workflow YAML | `deno eval --no-lock` with `jsr:@std/yaml@^1.0.10` — exit 0, `YAML_PARSE_OK` | + +- Review-fix reconcile: live issue wording supports box 1 without amendment; PR #1567 remains draft + with exactly `status:impl`. No trigger, condition, model, trusted-base lookup, #1541, or #1564 + scope changed. +- Review-fix implementation commit: `7170d574b3`; pushed with explicit refspec. The orchestrator + retains the labeled IMPL-EVAL transition and merge authority. + +## Evaluator-run correction and self-contained landing + +- Run `31598386001` disproved the prior end-to-end independence claim. `continue-on-error` worked: + the failed transition remained legible, its attributed diagnostic ran, and the dispatch step + started. Dispatch then failed because it requires a `status:impl-eval` labeled-event generation, + which cannot exist when the transition does not apply the label. The dependency is through + GitHub event history, not the step's declared `if:` condition. +- Owner-directed design: remove the checkout/import bootstrap and perform the cleanup inline in the + trusted `github-script` step. The inline code paginates live labels, removes only live `status:` + labels, tolerates only status 404 with exact message `Label does not exist`, rethrows everything + else, and adds only `status:impl-eval`. +- `.github/scripts/phase-eval-status.mjs` remains the independently unit-tested behavioral contract. + The workflow currently carries a transcription rather than importing it because this first + landing must be self-contained before the helper exists on trusted `main`. +- The existing workflow-policy test is retained with its precise scope: it proves the failed + transition is attributed and the dispatch step remains eligible under its declared conditions. + It does not prove the event-history generation dependency is satisfied. A separate explicitly + string-based parity assertion checks that the inline transcription and helper use the same exact + missing-label message and terminal label, including their comparison/addition sites. +- `continue-on-error` and the attributed failure summary remain. They do not make label generation + optional; they make future failures observable and allow the dispatch step to expose its own + generation precondition instead of being skipped. + +### Self-contained landing gate evidence + +| Gate | Result | +| --- | --- | +| Script tests | `deno test --allow-read --allow-env --allow-write --allow-run .github/scripts/` — exit 0; 67 passed, 0 failed | +| Scoped type-check | `deno run --allow-read --allow-run .llm/tools/run-deno-check.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Scoped lint | `deno run --allow-read --allow-run .llm/tools/run-deno-lint.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Scoped format | `deno run --allow-read --allow-run .llm/tools/run-deno-fmt.ts --root .github/scripts --ext ts` — exit 0; 6 files, 0 findings | +| Asset barrel | `deno task gen:assets-barrel` — exit 0; after implementation commit `d7ea38f1cd`, `git status --porcelain` was empty with exit 0 | +| Workflow YAML | `deno eval --no-lock` with `jsr:@std/yaml@^1.0.10` — exit 0, `YAML_PARSE_OK` | + +- Self-contained landing reconcile: PR #1567 is draft with exactly `status:impl`; the failed run + facts changed the design but not issue #1566's six-box acceptance mapping. No manual OpenHands + trigger, ready transition, waiver label, merge, #1541 action, or out-of-scope change occurred. +- Self-contained implementation commit: `d7ea38f1cd`; pushed with explicit refspec. The orchestrator + owns the ready flip and automatic DeepSeek retry.