fix: make review and claim proof atomic

codeacme17 · codeacme17 · commit e25b21285b92 · 2026-07-24T16:24:43.000+08:00
diff --git a/loops/issue-dev-loop/LOOP.md b/loops/issue-dev-loop/LOOP.md
@@ -65,7 +65,7 @@ Use a combo trigger. Run `triggers/detect-work.mjs` before starting a Codex impl
 
 ### 1. Claim and snapshot
 
-Recheck the issue immediately before mutation. `loopctl start` atomically creates the remote `codex/issue-N` branch at the verified base SHA as the cross-worktree reservation, then applies `loop:claimed` and creates the local run. GitHub permits only one creator for a new ref, so a concurrent loser exits before any checkpoint is published. If labeling fails, release the reservation only after verifying it still points to the exact base SHA. If a process crash nevertheless leaves a branch without a resumable checkpoint or open PR, the next trigger returns `claim_recovery`, wakes the loop, and immediately escalates the issue and branch to the owner instead of silently excluding it; never delete or reuse that reservation without explicit owner confirmation. Also reject another active run or open issue branch PR. Capture the issue title/body/labels/URL, base SHA, and acceptance criteria. Use one run ID across logs, handoffs, `screen-shots`, evidence, review comments, notifications, branch metadata, and the PR body.
+Recheck the issue immediately before mutation. `loopctl start` atomically creates the remote `codex/issue-N` branch at the verified base SHA as the cross-worktree reservation, then applies `loop:claimed` and creates the local run. GitHub permits only one creator for a new ref, so a concurrent loser exits before any checkpoint is published. If labeling fails, release the reservation with a single atomic `--force-with-lease=<exact-base-sha>` ref deletion; an advanced branch fails the lease and is never deleted. If a process crash nevertheless leaves a branch without a resumable checkpoint or open PR, the next trigger returns `claim_recovery`, wakes the loop, and immediately escalates the issue and branch to the owner instead of silently excluding it; never delete or reuse that reservation without explicit owner confirmation. Also reject another active run or open issue branch PR. Capture the issue title/body/labels/URL, base SHA, and acceptance criteria. Use one run ID across logs, handoffs, `screen-shots`, evidence, review comments, notifications, branch metadata, and the PR body.
 
 ### 2. Isolate
 
diff --git a/loops/issue-dev-loop/SKILL.md b/loops/issue-dev-loop/SKILL.md
@@ -43,7 +43,7 @@ Push only the issue branch and create a **draft** PR targeting `dev` using `temp
 
 After the draft PR exists, spawn the project agent `echo_ui_pr_reviewer` with a fresh context. Give it only the issue snapshot, acceptance criteria, repository instructions, base SHA, head SHA, diff, CI results, and evidence manifest. Do not give it executor conversation history or rationale.
 
-Publish every round through the installed wrapper with role `reviewer`; the executor identity may not author it. Every PR write must include `--repo codeacme17/echo-ui` (or use the full configured PR URL). Use `gh pr review --comment --body <body>` for a body-only review and include the exact run/cycle/round/head marker; the gate rejects body files, skipped rounds, duplicates, and publications outside the next durable cycle. When file/line findings require inline comments, use only the reviewer's gated `POST repos/codeacme17/echo-ui/pulls/<recorded-pr>/reviews` API shape with `event=COMMENT`, the exact durable head, and validated inline fields. Post findings verbatim as one review plus inline comments. Each finding needs a run-wide stable ID, severity, confidence, evidence, and expected resolution. Record the GitHub review URL for each round. Accepted repairs must start after that round was submitted, and executor replies must be posted through the automation wrapper after the corresponding `$implement` invocation finishes. Follow `review/REVIEW.md` and `review/response-policy.md`.
+Publish every round through the installed wrapper with role `reviewer`; the executor identity may not author it. Every PR write must include `--repo codeacme17/echo-ui` (or use the full configured PR URL). Use `gh pr review --comment --body <body>` for a body-only review and include the exact run/cycle/round/head marker; the gate rejects body files, skipped rounds, duplicates, and publications outside the next durable cycle. When file/line findings require inline comments, use only the reviewer's gated `POST repos/codeacme17/echo-ui/pulls/<recorded-pr>/reviews` API shape with `event=COMMENT`, the exact durable head, and validated inline fields. Post findings verbatim as one review plus inline comments. Each finding needs a run-wide stable ID, severity, confidence, evidence, expected resolution, and the published GitHub inline comment ID or `null` for a body-only finding. Record the GitHub review URL for each round. Accepted repairs must start after that round was submitted, and executor replies must be posted through the automation wrapper after the corresponding `$implement` invocation finishes. One executor response comment may resolve only one finding. Follow `review/REVIEW.md` and `review/response-policy.md`.
 
 The executor must classify every finding as `accepted`, `rejected`, `needs-human`, `stale`, or `already-fixed`:
 
diff --git a/loops/issue-dev-loop/agents/echo-ui-pr-reviewer.toml b/loops/issue-dev-loop/agents/echo-ui-pr-reviewer.toml
@@ -9,7 +9,8 @@ package compatibility, Web Audio lifecycle behavior, React behavior, accessibili
 security, regressions, and missing tests. Ignore executor conversation history and
 do not modify files. Avoid style-only or speculative findings. Return a structured
 review with run-wide `RVW-<cycle>-<round>-<sequence>` IDs, severity P0-P3, confidence, file and line, concrete
-evidence, reproduction when possible, and expected resolution. Return PASS when no
+evidence, reproduction when possible, expected resolution, and the GitHub inline comment ID after
+publication (`null` for body-only findings). Return PASS when no
 actionable findings exist. Publish GitHub reviews only through the installed executable
 `"$ECHO_UI_LOOP_CONTROL_PLANE/scripts/with-github-identity" --loop-root
 "$ECHO_UI_LOOP_TARGET_ROOT" reviewer -- ...` launcher; the wrapper must verify the
diff --git a/loops/issue-dev-loop/references/github-operations.md b/loops/issue-dev-loop/references/github-operations.md
@@ -24,7 +24,7 @@ The shell launcher removes Node preload hooks before starting the router. The ro
 
 1. Select only open issues labeled `codex-ready`.
 2. Exclude `loop:claimed` and any open PR that references or closes the issue. An existing `codex/issue-<number>` branch without a resumable checkpoint or open PR is `workType: claim_recovery`, not a quiet no-op: notify the owner and require confirmation before cleanup or reuse.
-3. Let `loopctl start --base-sha <full-origin-dev-sha>` acquire the local claim, recheck every page of open PRs, atomically create `codex/issue-<number>` at that base SHA, apply `loop:claimed`, and capture the authoritative issue; do not add the label or remote branch manually first. If labeling fails, the trusted start command verifies that the ref is still at the reservation SHA and releases it. A crash that leaves only the remote ref is detected on the next wake and escalated instead of being silently skipped.
+3. Let `loopctl start --base-sha <full-origin-dev-sha>` acquire the local claim, recheck every page of open PRs, atomically create `codex/issue-<number>` at that base SHA, apply `loop:claimed`, and capture the authoritative issue; do not add the label or remote branch manually first. If labeling fails, the trusted start command uses one exact `--force-with-lease` deletion, so an advanced ref fails atomically instead of being removed. A crash that leaves only the remote ref is detected on the next wake and escalated instead of being silently skipped.
 4. Record the issue snapshot and claim timestamp before implementation. A second active local run for the issue is rejected.
 5. Immediately publish and validate an active checkpoint after the claim. Repeat after each durable phase; the next phase re-fetches the exact state-journal comment and refuses to advance on local-only proof. On a fresh wake, `reconcile` returns `workType: resume`, branch, and expected head before considering a new issue. Fetch that branch, create a clean isolated worktree at the exact head, then run `restore-checkpoint`; restoration blocks on the wrong branch, head, or dirty state.
 
@@ -49,7 +49,7 @@ The low-privilege `pull_request` workflow `Issue dev loop evidence` runs only wh
 "$ECHO_UI_LOOP_CONTROL_PLANE/scripts/with-github-identity" --loop-root "$ECHO_UI_LOOP_TARGET_ROOT" automation -- gh run download <run-database-id> --name issue-dev-loop-<run-id>-<head-sha> --dir "$ECHO_UI_LOOP_TARGET_ROOT/evidence/<run-id>"
 ```
 
-Push only through the installed `with-github-identity ... automation -- git push ...` route. The wrapper rejects reviewer pushes, force pushes, and explicit pushes to `dev` or `main`.
+Push only through the installed `with-github-identity ... automation -- git push ...` route. The wrapper rejects reviewer pushes, force pushes, and explicit pushes to `dev` or `main`; the only lease-bearing exception is the trusted `start` command's exact-base atomic deletion of its just-created claim reservation after labeling fails.
 
 Use the artifact URL emitted by `actions/upload-artifact` as `record-evidence --publication-url` and the downloaded artifact manifest as `--manifest`. From a worktree whose `HEAD` is the artifact's exact candidate head, the installed runtime first validates the protected diff, then independently downloads the artifact, requires a successful low-privilege `pull_request` run of the named unchanged workflow, validates workflow-run SHA and owner-merged base SHA, and byte-compares the manifest. Reject a run whose PR, base, branch, candidate head, workflow path, or manifest digests differ from the recorded run.
 
diff --git a/loops/issue-dev-loop/review/REVIEW.md b/loops/issue-dev-loop/review/REVIEW.md
@@ -14,6 +14,7 @@ Return `PASS` or actionable findings. Each finding must include:
 - severity `P0`, `P1`, `P2`, or `P3`
 - confidence `high`, `medium`, or `low`
 - file and line when applicable
+- GitHub inline comment ID after publication, or `null` for a body-only finding
 - concrete impact and evidence
 - reproduction or failing test when practical
 - expected resolution
@@ -27,7 +28,7 @@ The fresh reviewer publishes each round verbatim through `reviewerGitHubLogin` a
 
 Before the final round is published, write the complete cycle result with a temporary/unassigned final `reviewUrl` and run the installed `loopctl review-digest --result <path>`. Put its returned `<!-- issue-dev-loop:<run-id>:review-result-sha256:<digest> -->` marker in the final body. After GitHub assigns the review URL, replace the temporary value and rerun `review-digest`; it must be unchanged. This canonical publication digest replaces only review URLs with a fixed placeholder and therefore avoids a URL↔digest cycle. The later recorded full-file digest still protects the final URLs and every other byte. The reviewer must not downgrade severity or omit findings.
 
-After all replies are posted, create one cycle result matching `result.schema.json`. Its `cycle` is the next durable review cycle number, and it contains every round's review URL, every finding, its final classification, response URL, and evidence. Executor replies include both the readable evidence (and accepted fix SHA) plus `<!-- issue-dev-loop:<run-id>:<finding-id>:<classification> -->`. Rejected P0/P1 findings additionally require an independent or owner COMMENT publication containing `<!-- issue-dev-loop:<run-id>:<finding-id>:adjudication:<verdict>:head:<sha> -->`. Independent `REJECT_FINDING` adjudication is a separate body-only, non-approving GitHub review published through the reviewer identity; it is bound to an existing current-head reviewer finding, cannot be duplicated, and does not consume a review cycle round. Run `record-review` with the final GitHub review URL; it paginates every reviewer-authored review on the recorded PR and requires one-to-one membership for the current run/cycle before it verifies replies, identities, timestamps, ancestry, adjudications, and markers. The publication gate rejects skipped or duplicate cycle rounds. Generic events cannot forge this reserved gate.
+After all replies are posted, create one cycle result matching `result.schema.json`. Its `cycle` is the next durable review cycle number, and it contains every round's review URL, every finding, its GitHub `inlineCommentId` (or `null` for a body-only finding), final classification, unique response URL, and evidence. Each fetched inline comment ID/path/line/body must bind exactly one durable finding, and one executor response comment may classify only one finding. Executor replies include both the readable evidence (and accepted fix SHA) plus `<!-- issue-dev-loop:<run-id>:<finding-id>:<classification> -->`. Rejected P0/P1 findings additionally require an independent or owner COMMENT publication containing `<!-- issue-dev-loop:<run-id>:<finding-id>:adjudication:<verdict>:head:<sha> -->`. Independent `REJECT_FINDING` adjudication is a separate body-only, non-approving GitHub review published through the reviewer identity; it is bound to an existing current-head reviewer finding, cannot be duplicated, and does not consume a review cycle round. Run `record-review` with the final GitHub review URL; it paginates every reviewer-authored review on the recorded PR and requires one-to-one membership for the current run/cycle before it verifies replies, identities, timestamps, ancestry, adjudications, and markers. The publication gate rejects skipped or duplicate cycle rounds. Generic events cannot forge this reserved gate.
 
 ## Completion
 
diff --git a/loops/issue-dev-loop/review/result.schema.json b/loops/issue-dev-loop/review/result.schema.json
@@ -43,6 +43,7 @@
                 "severity",
                 "confidence",
                 "headSha",
+                "inlineCommentId",
                 "problem",
                 "evidence",
                 "expectedResolution",
@@ -57,6 +58,7 @@
                 "severity": { "enum": ["P0", "P1", "P2", "P3"] },
                 "confidence": { "enum": ["high", "medium", "low"] },
                 "headSha": { "type": "string", "pattern": "^[0-9a-fA-F]{40}$" },
+                "inlineCommentId": { "type": ["integer", "null"], "minimum": 1 },
                 "path": { "type": ["string", "null"] },
                 "line": { "type": ["integer", "null"], "minimum": 1 },
                 "problem": { "type": "string", "minLength": 1 },
diff --git a/loops/issue-dev-loop/scripts/github-command-gate.mjs b/loops/issue-dev-loop/scripts/github-command-gate.mjs
@@ -41,15 +41,29 @@ async function main() {
         : args
   if (tool !== 'credential') {
     if (tool === 'git' && args[0] === 'push') {
-      throw new Error('authenticated descendant processes cannot push')
+      const branch = authorization?.issue?.branch
+      const expectedRollback = [
+        'push',
+        `--force-with-lease=refs/heads/${branch}:${authorization?.issue?.baseSha}`,
+        'origin',
+        `:refs/heads/${branch}`,
+      ]
+      if (
+        authorization?.rootIntent !== 'start' ||
+        authorization?.issue?.status !== 'starting' ||
+        args.length !== expectedRollback.length ||
+        args.some((argument, index) => argument !== expectedRollback[index])
+      ) {
+        throw new Error('authenticated descendant processes cannot push')
+      }
     }
     assertDescendantCommandPolicy({
       role,
       tool,
       args,
       authorization,
     })
-    if (tool === 'git' && ['fetch', 'ls-remote'].includes(args[0])) {
+    if (tool === 'git' && ['push', 'fetch', 'ls-remote'].includes(args[0])) {
       await assertSafeRemoteGitConfiguration({
         realGit: executable,
         environment: process.env,
diff --git a/loops/issue-dev-loop/scripts/lib/evidence.mjs b/loops/issue-dev-loop/scripts/lib/evidence.mjs
@@ -168,6 +168,22 @@ function validateReviewEvidence(review, headSha) {
       if (!['high', 'medium', 'low'].includes(finding.confidence)) {
         throw new Error(`${findingId}.confidence must be high, medium, or low`)
       }
+      const hasInlineLocation =
+        typeof finding.path === 'string' &&
+        finding.path.length > 0 &&
+        Number.isInteger(finding.line) &&
+        finding.line > 0
+      const hasInlineCommentId =
+        Number.isInteger(finding.inlineCommentId) && finding.inlineCommentId > 0
+      if (
+        (finding.path != null || finding.line != null) !== hasInlineLocation ||
+        (finding.inlineCommentId !== null && !hasInlineCommentId) ||
+        hasInlineLocation !== hasInlineCommentId
+      ) {
+        throw new Error(
+          `${findingId}.inlineCommentId must bind exactly one inline path and line, or be null`,
+        )
+      }
       assertNonEmpty(finding.problem, `${findingId}.problem`)
       assertNonEmpty(finding.evidence, `${findingId}.evidence`)
       assertNonEmpty(finding.expectedResolution, `${findingId}.expectedResolution`)
@@ -599,6 +615,7 @@ export async function recordReview({
         commentFindingIds.size !== 1 ||
         !finding ||
         inlineFindingIds.has(commentFindingId) ||
+        finding.inlineCommentId !== Number(commentId) ||
         finding.path !== comment.path ||
         !Number.isInteger(finding.line) ||
         ![comment.line, comment.original_line].includes(finding.line) ||
@@ -668,6 +685,7 @@ export async function recordReview({
   ) {
     throw new Error('published review is not bound to the recorded live PR head')
   }
+  const responseCommentIds = new Set()
   for (const round of reviewSummary.roundDetails) {
     const publication = publications.get(round.round)
     const reviewSubmittedAt = Date.parse(publication.submittedAt)
@@ -681,6 +699,11 @@ export async function recordReview({
       ) {
         throw new Error(`${finding.findingId} response is not on the reviewed PR`)
       }
+      const responseCommentId = `${responseTarget.kind}:${responseTarget.commentId}`
+      if (responseCommentIds.has(responseCommentId)) {
+        throw new Error('one executor response comment cannot adjudicate multiple findings')
+      }
+      responseCommentIds.add(responseCommentId)
       const responseEndpoint =
         responseTarget.kind === 'review_comment'
           ? `repos/${responseTarget.owner}/${responseTarget.repo}/pulls/comments/${responseTarget.commentId}`
diff --git a/loops/issue-dev-loop/scripts/lib/finalization-proof.mjs b/loops/issue-dev-loop/scripts/lib/finalization-proof.mjs
@@ -1,16 +1,15 @@
 import { createHash } from 'node:crypto'
-import path from 'node:path'
 
 import {
   assertNonEmpty,
   defaultGitHubApi,
   parseGitHubTarget,
   parsePullCommentUrl,
-  readJson,
   sameGitHubLogin,
   sameRepository,
 } from './common.mjs'
 import {
+  checkpointJournalConfiguration,
   parseCheckpointRecord,
   verifyPublishedCheckpoint,
 } from './checkpoint-proof.mjs'
@@ -243,20 +242,7 @@ export async function verifyFailedOrBlockedNotification({
   return comment
 }
 
-export async function finalizationJournalConfiguration(loopRoot) {
-  const channel = await readJson(
-    path.resolve(loopRoot, '..', '_shared', 'owner-channel', 'channel.json'),
-  )
-  if (
-    !Number.isInteger(channel.stateIssueNumber) ||
-    channel.stateIssueNumber < 1 ||
-    !channel.automationGitHubLogin
-  ) {
-    throw new Error('owner channel must configure stateIssueNumber and automationGitHubLogin')
-  }
-  const [owner, repo] = channel.repository.split('/')
-  return { channel, owner, repo }
-}
+export const finalizationJournalConfiguration = checkpointJournalConfiguration
 
 export async function verifyPullNotificationComment({
   url,
diff --git a/loops/issue-dev-loop/scripts/lib/github-identity.mjs b/loops/issue-dev-loop/scripts/lib/github-identity.mjs
diff --git a/loops/issue-dev-loop/scripts/lib/issue-claim.mjs b/loops/issue-dev-loop/scripts/lib/issue-claim.mjs
diff --git a/loops/issue-dev-loop/tests/github-identity-routing.test.mjs b/loops/issue-dev-loop/tests/github-identity-routing.test.mjs
diff --git a/loops/issue-dev-loop/tests/runtime.test.mjs b/loops/issue-dev-loop/tests/runtime.test.mjs
