diff --git a/batch/lib/kernel-client.sh b/batch/lib/kernel-client.sh index b330cc7..bb7442a 100644 --- a/batch/lib/kernel-client.sh +++ b/batch/lib/kernel-client.sh @@ -852,9 +852,9 @@ _kernel_record_ci() { # # implementation output; the kernel refuses a review whose phase is not the # phase of the state it is recorded from. The front half's spec and plan # reviews do not come through here. -_kernel_record_review() { # [key] [terminal] +_kernel_record_review() { # [key] [terminal] [head_sha] [merge_base_sha] [delta_digest] local run_id="$1" generation="$2" raw="$3" artifact="$4" base="$5" context="$6" - local key="${7:-}" terminal="${8:-}" + local key="${7:-}" terminal="${8:-}" head_sha="${9:-}" merge_base="${10:-}" digest="${11:-}" local verdict; verdict=$(_kernel_verdict "$raw" "$terminal") # No early return: every input now maps to something submittable -- a mapped # verdict, or `unmapped:...` which the kernel refuses visibly. The guard that @@ -865,18 +865,22 @@ _kernel_record_review() { # < _kernel_warn "incomplete verdict binding (artifact='$artifact' base='$base' context='$context') -- not recording a review" return 0 fi + # What was reviewed (spec §4): included only when the derivation pinned a + # head. Values are shas and a hex digest, so they need no JSON escaping. + local range="" + [ -n "$head_sha" ] && range="\"head_sha\":\"$head_sha\",\"merge_base_sha\":\"$merge_base\",\"delta_digest\":\"$digest\"," # An empty key must not become `--idempotency-key ""`: the CLI would take the # empty string as the key rather than falling back to its default, and every # record_review in the run would collide on it. if [ -n "$key" ]; then _kernel command --run-id "$run_id" --generation "$generation" \ --name record_review --idempotency-key "$key" \ - --payload-json "{\"verdict\":\"$verdict\",\"phase\":\"implementation\",\"artifact_hash\":\"$artifact\",\"base_sha\":\"$base\",\"context_bundle_hash\":\"$context\",\"policy_version\":$_KERNEL_POLICY_VERSION}" + --payload-json "{\"verdict\":\"$verdict\",\"phase\":\"implementation\",\"artifact_hash\":\"$artifact\",\"base_sha\":\"$base\",\"context_bundle_hash\":\"$context\",${range}\"policy_version\":$_KERNEL_POLICY_VERSION}" return 0 fi _kernel command --run-id "$run_id" --generation "$generation" \ --name record_review \ - --payload-json "{\"verdict\":\"$verdict\",\"phase\":\"implementation\",\"artifact_hash\":\"$artifact\",\"base_sha\":\"$base\",\"context_bundle_hash\":\"$context\",\"policy_version\":$_KERNEL_POLICY_VERSION}" + --payload-json "{\"verdict\":\"$verdict\",\"phase\":\"implementation\",\"artifact_hash\":\"$artifact\",\"base_sha\":\"$base\",\"context_bundle_hash\":\"$context\",${range}\"policy_version\":$_KERNEL_POLICY_VERSION}" } # _kernel_request_merge diff --git a/batch/run-queue.sh b/batch/run-queue.sh index a3beca9..0987b07 100755 --- a/batch/run-queue.sh +++ b/batch/run-queue.sh @@ -1170,8 +1170,8 @@ _pr_merge_state() { fi } -_post_cross_review_status() { - local item="$1" pr="$2" sha="${3:-}" attempt err +_post_cross_review_status() { # [sha] [state] [description] + local item="$1" pr="$2" sha="${3:-}" state="${4:-success}" description="${5:-cross-vendor review PASS (Bircher)}" attempt err # Head sha: the caller may PIN it (the sweep pins the reviewed head so the status # is never posted on an unreviewed push); otherwise fetch the current head, with a # few retries (gh pr view can transiently fail too). @@ -1184,6 +1184,18 @@ _post_cross_review_status() { done fi [ -n "$sha" ] || { echo "[batch:merge] WARN $item: no head sha for PR #$pr -> cross-review status skipped" >&2; return 1; } + # ALREADY THERE -> nothing to do. The derivation posts a status for every + # verdict (spec §4), so by the time the merge path gets here a success + # usually exists with a description naming the reviewed range; re-posting + # would overwrite that description with the legacy text. + if [ "$state" = success ] && ! _deadline_passed "${PREMERGE_DEADLINE_AT:-}" \ + && [ "$(_net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + gh api "repos/$REPO/commits/$sha/status" \ + -q '.statuses[] | select(.context=="bircher/cross-review") | .state' 2>/dev/null \ + | grep -cx 'success')" != 0 ]; then + echo "[batch:merge] $item: bircher/cross-review=success already on ${sha:0:7} -> not re-posting" >&2 + return 0 + fi # Post, then read the status back to confirm it landed. Retry both with # exponential backoff; log the REAL gh error (no more 2>/dev/null) so a # non-transient cause is diagnosable next time. @@ -1195,9 +1207,9 @@ _post_cross_review_status() { _deadline_passed "${PREMERGE_DEADLINE_AT:-}" && break err=$(_effect status_check "status:$sha" \ "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ - gh api "repos/$REPO/statuses/$sha" -X POST -f state=success \ + gh api "repos/$REPO/statuses/$sha" -X POST -f "state=$state" \ -f context=bircher/cross-review \ - -f description="cross-vendor review PASS (Bircher)" 2>&1 >/dev/null) + -f description="$description" 2>&1 >/dev/null) # ...and before the verification too. Skipping it after a successful POST costs a # confirmation, so the caller merges best-effort and branch protection decides -- # which is the existing conservative path, not a new one. @@ -1208,8 +1220,8 @@ _post_cross_review_status() { if [ "$(_net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ gh api "repos/$REPO/commits/$sha/status" \ -q '.statuses[] | select(.context=="bircher/cross-review") | .state' 2>/dev/null \ - | grep -cx 'success')" != 0 ]; then - echo "[batch:merge] $item: posted+verified bircher/cross-review=success on ${sha:0:7} (attempt $attempt)" >&2 + | grep -cx "$state")" != 0 ]; then + echo "[batch:merge] $item: posted+verified bircher/cross-review=$state on ${sha:0:7} (attempt $attempt)" >&2 return 0 fi echo "[batch:merge] WARN $item: cross-review status not confirmed on ${sha:0:7} (attempt $attempt/5)${err:+: $err}" >&2 @@ -1234,67 +1246,6 @@ _sha256() { fi | cut -d' ' -f1 } -# _pr_delta_digest -> a stable digest of the PR's OWN delta, -# or rc 1 when that delta cannot be established. -# -# GitHub's compare is three-dot, so `base...head` is the change the PR contributes -# and never the base branch's own commits. That property is what makes the digest -# comparable across an update-branch: that operation MERGES the base into the head -# (it does not rebase), so the new head CONTAINS the new base, the merge-base IS -# the new base, and the compare still yields only the PR's work. -# -# rc 1 is NOT "assume equal" - the caller turns it into an escalation. The -# unprovable cases are real: GitHub caps the changed-file list at 300, omits `patch` -# for binaries, pure renames and anything over its size limit, and truncates a large -# tree. Digesting a partial answer would produce a confident wrong one, which here -# means merging code no reviewer read. -_pr_delta_digest() { - local base="$1" ref="$2" cmp tree count - cmp=$(gh api "repos/$REPO/compare/${base}...${ref}" 2>/dev/null) || return 1 - [ -n "$cmp" ] || return 1 - count=$(printf '%s' "$cmp" | jq -r '.files | length' 2>/dev/null) || return 1 - case "$count" in ''|*[!0-9]*) return 1 ;; esac - # 0 files = nothing to compare (degenerate, and it would make two unrelated empty - # answers look equal); >= 300 = GitHub's documented cap for the changed-file list, - # so at exactly 300 it may be truncated. - { [ "$count" -ge 1 ] && [ "$count" -lt 300 ]; } || return 1 - # A file whose patch GitHub withheld leaves a hole in the comparison. - printf '%s' "$cmp" | jq -e 'any(.files[]; has("patch") | not)' >/dev/null 2>&1 && return 1 - # The compare payload carries NO mode and NO type - verified against the API, where - # files[] is filename/status/sha/patch plus counts and URLs. That gap is load-bearing - # here: git stores a symlink's TARGET as its blob content, so a symlink pointing at - # "x" and a regular file containing "x" share a blob sha AND project to an identical - # patch. Digesting the compare alone would let a base that changed a path's TYPE - # merge as though the PR's delta were untouched. The tree carries mode and type; a - # TRUNCATED tree cannot answer for every path, so it fails closed like the rest. - tree=$(gh api "repos/$REPO/git/trees/${ref}?recursive=1" 2>/dev/null) || return 1 - [ -n "$tree" ] || return 1 - # Require an explicit false. `== true` would PROCEED on an absent or null - # `truncated`, i.e. treat an answer we did not get as a reassuring one. - printf '%s' "$tree" | jq -e '.truncated == false' >/dev/null 2>&1 || return 1 - # Note this escalates whenever the base touched a file the PR also touches: the - # resulting blob sha (and often the patch context) moves. That is CORRECT rather - # than merely cautious - the merged file then combines both changes, and the - # reviewer never saw that combination. The feature is for the common case where - # the base moved elsewhere in the tree. - # Canonical form: sorted by filename, sorted keys, and EVERY field that identifies - # the change - path, rename origin, status, resulting blob, patch, and the tree - # entry's mode|type. `--slurpfile` rather than `--argjson` keeps a large compare - # payload off the command line, where a big PR would hit ARG_MAX. - printf '%s' "$tree" \ - | jq -cS --slurpfile c <(printf '%s' "$cmp") ' - (.tree | map({key: .path, value: (.mode + "|" + .type)}) | from_entries) as $m - | [ $c[0].files - | sort_by(.filename)[] - | { filename, - previous_filename: (.previous_filename // null), - status, - sha, - patch, - entry: ($m[.filename] // "ABSENT") } ]' 2>/dev/null \ - | _sha256 -} - # _restamp_if_delta_unchanged # -> rc 0 and RESTAMPED_HEAD= when the updated head provably carries the # SAME change the reviewer passed; rc 1 (caller escalates) otherwise. @@ -1329,10 +1280,21 @@ _restamp_if_delta_unchanged() { [ "${BIRCHER_STATUS_BACKOFF:-1}" = 0 ] || sleep $((attempt * 2)) done [ -n "$new" ] || { echo "[batch:sweep] $item: PR #$pr head never moved after update-branch -> escalate" >&2; return 1; } - old_digest=$(_pr_delta_digest "$base" "$reviewed") || old_digest="" - new_digest=$(_pr_delta_digest "$base" "$new") || new_digest="" + # STDERR IS NOT DISCARDED. A CLI that cannot start -- no interpreter, an + # import error, a bad PYTHONPATH -- is indistinguishable from an unprovable + # delta once its message is thrown away, and the escalation below then + # misdiagnoses it as GitHub withholding a patch. It goes to the run log, as + # the derive call's does. + old_digest=$( PYTHONPATH="$(_kernel_pythonpath)" \ + _net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + "${BIRCHER_PY:-python3}" -m coordinator.cli delta \ + --repo "$REPO" --base "$base" --ref "$reviewed") || old_digest="" + new_digest=$( PYTHONPATH="$(_kernel_pythonpath)" \ + _net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + "${BIRCHER_PY:-python3}" -m coordinator.cli delta \ + --repo "$REPO" --base "$base" --ref "$new") || new_digest="" if [ -z "$old_digest" ] || [ -z "$new_digest" ]; then - echo "[batch:sweep] $item: PR #$pr delta not provable (compare failed/truncated/patch withheld) -> escalate" >&2 + echo "[batch:sweep] $item: PR #$pr delta not provable (compare failed/truncated/patch withheld, or the delta CLI failed -- see its stderr above) -> escalate" >&2 return 1 fi if [ "$old_digest" != "$new_digest" ]; then @@ -1340,7 +1302,18 @@ _restamp_if_delta_unchanged() { return 1 fi echo "[batch:sweep] $item: PR #$pr delta PROVEN identical across the update (${reviewed:0:7} -> ${new:0:7}) -> re-stamping the review" >&2 - _post_cross_review_status "$item" "$pr" "$new" || return 1 + local prior_desc restamp_desc + prior_desc=$(_net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + gh api "repos/$REPO/commits/$reviewed/status" \ + -q '.statuses[] | select(.context=="bircher/cross-review") | .description' 2>/dev/null | head -1) + # TRUNCATED TO 140, GitHub's limit for a status description, because this one + # is COMPOSED: the prior description (already up to 140) plus the re-stamp + # suffix is longer than the limit by construction, and GitHub truncates the + # overflow wherever it lands -- which would cut the suffix that says this is + # a re-stamp. The derivation's own descriptions are truncated at their + # producer (`attest.describe`); this is the same rule at this one. + restamp_desc="${prior_desc:-cross-vendor review PASS (Bircher)} (re-stamped on ${new:0:7}, delta unchanged)" + _post_cross_review_status "$item" "$pr" "$new" success "${restamp_desc:0:140}" || return 1 RESTAMPED_HEAD="$new" return 0 } @@ -1550,9 +1523,19 @@ _await_mergeable_state() { return 1 } -# merge_ready_pr -> rc 0 (merged or deferred; MERGE_NOTE set on +# merge_ready_pr [reviewed_sha] [status_description] +# -> rc 0 (merged or deferred; MERGE_NOTE set on # deferral) | rc 2 (HALT the run: main went red and the merge was reverted, or -# main CI never resolved). B-1 in-run merge: merging each ready PR before the +# main CI never resolved). +# +# `status_description` is the FALLBACK description for the cross-review status, +# used only when the derivation's own post is missing (it normally is not: the +# derivation posts a status for every verdict, naming the range it reviewed). +# The caller supplies it because the caller is what holds the range; passed +# empty -- every caller but `run_item` -- `_post_cross_review_status` keeps its +# legacy text, which is all that can honestly be said when the range is unknown. +# +# B-1 in-run merge: merging each ready PR before the # next item launches means every later implementer branches from a main that # already contains its siblings - the merge-order conflict class disappears. # Safety: watch MAIN's CI on the merge commit; on red, revert (throwaway @@ -1561,7 +1544,7 @@ _await_mergeable_state() { MERGE_NOTE="" MERGE_RETRY_ELIGIBLE="" merge_ready_pr() { - local item="$1" pr="$2" expected_sha="${3:-}" + local item="$1" pr="$2" expected_sha="${3:-}" status_desc="${4:-}" MERGE_NOTE="" MERGE_RETRY_ELIGIBLE=0 # #71: evidence that what LANDED was not what was reviewed. Two variables, not one: @@ -1612,7 +1595,7 @@ merge_ready_pr() { # protected repo self-merges without an approving review. No-op on repos that # don't require the check. local _status_confirmed=1 - if ! _post_cross_review_status "$item" "$pr" "$expected_sha"; then + if ! _post_cross_review_status "$item" "$pr" "$expected_sha" success "$status_desc"; then _status_confirmed=0 # Best-effort: attempt the merge anyway. On a repo that REQUIRES the check the # merge below is BLOCKED and defers (retry-eligible); on a repo that does NOT @@ -2404,7 +2387,7 @@ print("halted" if d.get("halted") else "clear", len(d.get("pending") or []))' 2> echo "[batch:recover-pr] $code: no generation to bind this recovery to (the run is halted, or the kernel is unreachable) -> not deriving and not merging; needs a human" >&2 return 1 fi - local rec r_outcome r_review r_note r_sha r_ci r_settled_pr + local rec r_outcome r_review r_note r_sha r_ci r_settled_pr r_merge_base r_delta_digest # An EMPTY tuple is a CRASH, not a verdict. observe_outcome has a # single exit and always emits five fields, so no output means it died before # reaching that line -- and `rec=$(...)` swallows the death into an empty @@ -2427,7 +2410,7 @@ print("halted" if d.get("halted") else "clear", len(d.get("pending") or []))' 2> # added to fix: `observe_outcome` runs the same `_settle_pr`, so recovery can # review and comment a sibling PR and then authorize and merge the stale one # it was invoked with. - IFS='|' read -r r_outcome r_review r_note r_sha r_ci _ _ r_settled_pr < -> the read-only reviewer sub-agent input. -# Mirrors the cross-review skill's reviewer template: fetch the PR branch, -# read whole files, run gates each with an inline PATH export, end with an -# exact VERDICT line (findings above it). -_recovery_review_prompt() { - local pr="$1" sha="${2:-}" - # #66: the worktree is created at the EXACT captured commit, not at FETCH_HEAD. - # `pull/N/head` is a MOVING ref: a push between capture and the reviewer's fetch - # would have it read one commit while the merge pinned another, and a later - # force-push back to the captured sha would then merge code the reviewer never - # read. Checking out the sha removes the ambiguity mechanically rather than by - # asking the reviewer to verify it. If the sha is no longer reachable from the - # ref, the checkout fails and so does the review -- which is the correct - # outcome, not a regression. - local _co="FETCH_HEAD"; [ -n "$sha" ] && _co="$sha" - # A UNIQUE worktree per REVIEWER. The `-oob` suffix does the work: both - # reviewers review the same commit, so a sha-derived nonce alone would still - # collide. Both wrote `/tmp/review-`; - # on muesli #745 the second died on "already exists" and, with only PASS - # and FAIL on offer, reported FAIL for a PR it had not read. - local _nonce="${_co:0:8}"; [ -n "$_nonce" ] || _nonce=head - # No `rm -rf` in the setup line: omnigent's blast_radius guardrail denies - # the catastrophic set, and `rm -rf /tmp/review-...` reads as `rm -rf /...`. - # On muesli #759 (2026-09-10) the reviewer's setup was rejected before it - # read anything, three reviews in a row derived as FAIL, and both repair - # rounds were spent on a review that never happened. `git worktree remove - # --force` deletes the directory itself; `git worktree prune` clears a stale - # registration; a leftover plain directory (a crashed run, a killed session: - # muesli #711 round 2 answered BLOCKED over one) is MOVED aside, not deleted, - # so the add always starts clean and nothing here can read as a delete. - cat </dev/null; git worktree prune; [ ! -e /tmp/review-$pr-$_nonce-oob ] || mv /tmp/review-$pr-$_nonce-oob /tmp/review-$pr-$_nonce-oob.stale.\$\$; git worktree add --detach /tmp/review-$pr-$_nonce-oob $_co; cd /tmp/review-$pr-$_nonce-oob. -You are reviewing EXACTLY commit $_co. If that checkout fails, STOP and report it -- do not review a different commit. -READ the changed files AND enough surrounding code to verify correctness -- do NOT judge from the diff alone. -Run the gates you can, EACH as ONE command prefixed with 'export PATH=/root/bin:\$PATH &&' (e.g. 'export PATH=/root/bin:\$PATH && go build ./...', '... && go vet ./...', client '... && npm run typecheck' / '... && npx vitest run', plugin '... && pytest'); DB-backed 'go test' needs a DB the runner lacks, so for THOSE you must not simply accept a green check. -A green check is a CLAIM, not evidence: for any gate you could not run yourself, open the run log (\`gh pr checks $pr\` to find the run, then \`gh run view --log\`) and RECONCILE it with the check's conclusion -- a step can execute, report failing tests, and STILL be reported green if its exit code was swallowed (\`|| true\`, continue-on-error, a wrapper that always exits 0). Quote the log line showing test counts or the failure, and NAME every gate you delegated rather than ran. If you cannot reach the log, say so and treat that gate as UNVERIFIED -- do not report it as passing. (muesli #705 shipped a CI gate that reported success while tests failed; it passed review because the reviewer was told to trust the check.) -If the change acquires a resource that must be released -- a capture device, stream, handle, lock or subscription -- verify its FAILURE paths are tested, not just the happy path; a missing release-on-error test is a blocking finding. (muesli #666 left a microphone recording when a capture start failed.) Keep that scope narrow: do not treat every state change as in scope. -Report blocking / non-blocking / suggestion findings, then a FINAL LINE that is EXACTLY 'VERDICT: PASS', 'VERDICT: FAIL', or 'VERDICT: BLOCKED'. -Use BLOCKED, and ONLY BLOCKED, when you could not review at all -- the checkout failed, the tooling was unavailable, the commit was unreachable. BLOCKED means "I formed no opinion"; FAIL means "I reviewed this and it must not merge". They are routed differently and confusing them is expensive: a reviewer that could not check out its worktree once emitted FAIL, and the run recorded a code rejection for a PR nobody had read. -Put findings BEFORE the verdict so the verdict is the last line even if output is long. -EOF -} - # _reconcile_item_pr -> the open PR number to act on. # A coordinator that opens a fresh branch/PR for a CI-red retry (run #20: item # i141 left #178 red + #179 green, both open) leaves run-queue tracking only the @@ -2736,13 +2682,14 @@ _derive_budget() { printf '%s' "$floor" } -# _derived_width_ok -> rc 0 if it is exactly one line of eight fields. +# _derived_width_ok -> rc 0 if it is exactly one line of ten fields. # -# FAIL CLOSED. The old reader could not tell a seven-field tuple from an -# eight-field one: `read -r a..h` simply leaves `h` empty, so a short result -- -# version skew against an older coordinator, a truncated write, a partial -# failure -- silently restored the very behaviour the eighth field was added to -# remove, and the caller went on to authorize and merge a stale PR. +# FAIL CLOSED. The old reader could not tell a short tuple from a full one: +# `read -r a..j` simply leaves the tail empty, so a short result -- version +# skew against an older coordinator, a truncated write, a partial failure -- +# silently restored the very behaviour the later fields were added to remove: +# the caller went on to authorize and merge a stale PR, or record a verdict +# with no reviewed range. # # Also rejects embedded newlines: `read` consumes only the FIRST line, so a # multi-line result would be parsed as its first line and the rest discarded @@ -2754,7 +2701,7 @@ _derived_width_ok() { # every input -- the check rejected everything, including valid tuples. case $line in *$'\n'*) return 1 ;; esac n=$(printf '%s' "$line" | awk -F'|' '{print NF}') - [ "$n" = 8 ] + [ "$n" = 10 ] } # _max_revisions -> how many repair rounds this run may spend, 0-5. @@ -4364,7 +4311,8 @@ run_item() { # The kernel's record of this run. Item codes recur across attempts, so a # minted id carries the epoch; a RESUMED one is whatever the kernel already # holds (below). - BIRCHER_KERNEL_DB="${BIRCHER_KERNEL_DB:-$BUNDLE_DIR/.run/kernel.db}"; export BIRCHER_KERNEL_DB + BIRCHER_KERNEL_DB="${BIRCHER_KERNEL_DB:-$BUNDLE_DIR/.run/kernel.db}" + export BIRCHER_KERNEL_DB mkdir -p "$(dirname "$BIRCHER_KERNEL_DB")" 2>/dev/null || true local _iss; _iss=$(_item_issue "$prompt") local _base_sha; _base_sha=$(git -C "$WORKDIR" rev-parse HEAD 2>/dev/null) @@ -4932,6 +4880,8 @@ run_item() { local _rev_key="" local _rev_round=0 local _rev_terminal="" + local _merge_base="" + local _delta_digest="" if [ "${_blind:-0}" = 1 ]; then # Unchanged from the marker era, and still correct: the cancel was never # confirmed, so the coordinator may still be running. Deriving an outcome @@ -4977,6 +4927,8 @@ run_item() { # for the same reason: every signal looks normal. _rev_key="" _rev_left=0 + _merge_base="" + _delta_digest="" if [ "$(_max_revisions)" != 0 ] && [ -n "${BIRCHER_RUN_ID:-}" ]; then # `used|left|confirmed`. A LOOKUP FAILURE leaves _rev_left at 0, which # ends the loop and escalates -- deliberately, because the alternative @@ -5000,22 +4952,24 @@ run_item() { # wearing a verdict's clothes. if [ -z "${obs//[[:space:]]/}" ]; then echo "[batch] $item: derivation produced NO tuple -> it failed; escalating rather than reading it as a verdict" >&2 - obs="escalated|na|outcome derivation failed (no tuple); needs a human||na|unknown||" + obs="escalated|na|outcome derivation failed (no tuple); needs a human||na|unknown||||" fi - # EIGHT fields. The last is the PR the DERIVATION settled on, which is not + # TEN fields. The eighth is the PR the DERIVATION settled on, which is not # always the one passed in: it discards an abandoned PR, discovers one by - # code or issue linkage, and adopts a CI-green sibling. Reading seven - # absorbed the eighth into `resubmissions` silently -- `read` does not - # error on a short variable list, it concatenates the remainder into the - # last name. + # code or issue linkage, and adopts a CI-green sibling. The last two are + # the reviewed range (spec §4): the merge-base against the PR's base and + # the digest of the PR's own delta at the reviewed head. Reading fewer + # names than the tuple has absorbs the remainder into the last one + # silently -- `read` does not error on a short variable list, it + # concatenates the remainder into the last name. # WIDTH CHECKED BEFORE PARSING. A short tuple leaves `_settled_pr` empty, # which used to mean "keep the PR I started with" -- indistinguishable from # a derivation that legitimately settled on nothing. if ! _derived_width_ok "$obs"; then - echo "[batch] $item: derivation returned a malformed tuple (not eight fields on one line) -> escalating rather than guessing which field is which" >&2 - obs="escalated|na|derivation returned a malformed tuple; needs a human||na|unknown||" + echo "[batch] $item: derivation returned a malformed tuple (not ten fields on one line) -> escalating rather than guessing which field is which" >&2 + obs="escalated|na|derivation returned a malformed tuple; needs a human||na|unknown||||" fi - IFS='|' read -r outcome review note observed_head _obs_ci ci_first resubmissions _settled_pr <"$rdir/omnigent" <<'SH' #!/usr/bin/env bash # A reviewer that BLOCKS, with findings shaped like the real ones: multiple -# paragraphs, a pipe, and newlines -- all three of which the eight-field tuple +# paragraphs, a pipe, and newlines -- all three of which the ten-field tuple # cannot carry, which is the whole reason the findings travel by file. printf 'Blocking:\n- the retry loop reads `a | b` and drops b\n- no test covers the empty case\n\nVERDICT: FAIL\n' exit 0 @@ -5843,7 +5811,7 @@ SH *) echo "FAIL repair: a FAIL with rounds left must derive 'revise', got '$rev_out'"; exit 1 ;; esac _derived_width_ok "$rev_out" \ - || { echo "FAIL repair: the revise tuple is not eight fields on one line: '$rev_out'"; exit 1; } + || { echo "FAIL repair: the revise tuple is not ten fields on one line: '$rev_out'"; exit 1; } [ -s "$rdir/findings.txt" ] \ || { echo "FAIL repair: no findings were written for a revise"; exit 1; } grep -q 'drops b' "$rdir/findings.txt" \ @@ -6259,7 +6227,7 @@ SH # DISCOVERS it, so the field carrying it back is exactly what this asserts. # Before the field existed the discovery was used internally and the caller # never learned of it. - [ "$rec_disc" = "ready|codex:pass|out-of-band review PASS|a502a88e20f959c908d00871ee7f25572512dd6d|green|unknown||300" ] \ + [ "$rec_disc" = "ready|codex:pass|out-of-band review PASS|a502a88e20f959c908d00871ee7f25572512dd6d|green|unknown||300||" ] \ || { echo "FAIL recover discovery-adopt (1b): '$rec_disc'"; rm -rf "$ddir"; exit 1; } rm -rf "$ddir" echo "recover discovery-adopt (1b) OK" @@ -6324,7 +6292,7 @@ SH local rec_iss rec_iss=$(PATH="$idir:$PATH" WORKDIR="$idir" REPO=demo/demo SERVER=http://x \ RECOVERY_REVIEWER=codex observe_outcome iwrong iwrong "" 307) - [ "$rec_iss" = "ready|codex:pass|out-of-band review PASS|a502a88e20f959c908d00871ee7f25572512dd6d|green|unknown||305" ] \ + [ "$rec_iss" = "ready|codex:pass|out-of-band review PASS|a502a88e20f959c908d00871ee7f25572512dd6d|green|unknown||305||" ] \ || { echo "FAIL recover issue-linkage adopt: '$rec_iss'"; rm -rf "$idir"; exit 1; } rm -rf "$idir" echo "issue-linkage fallback (_discover_pr_by_issue + recover) OK" @@ -6992,7 +6960,10 @@ if [ "$1" = "api" ]; then # empty, so a test that set it exercised nothing and passed without ever creating the # condition it named. if printf '%s\n' "$@" | grep -q '/status'; then - if printf '%s\n' "$@" | grep -q -- '-q'; then cat "${FAKE_STATUS_STORE:-/dev/null}" 2>/dev/null; exit 0; fi + if printf '%s\n' "$@" | grep -q -- '-q'; then + [ "${STATUS_PRESENT:-0}" = 1 ] && { printf 'success\n'; exit 0; } + cat "${FAKE_STATUS_STORE:-/dev/null}" 2>/dev/null; exit 0 + fi SJ="${FAKE_STATUS_JSON:-}"; [ -n "$SJ" ] || SJ='{"statuses":[]}' printf '[%s]' "$SJ"; exit 0 fi @@ -7026,6 +6997,26 @@ SH # happy path: mergeable -> merged -> main CI green -> rc 0, empty MERGE_NOTE ( PATH="$mdir:$PATH" REPO=demo/demo MAIN_CI_TIMEOUT=31 FAKE_STATUS_STORE="$mdir/s1" merge_ready_pr demo 7 headsha1234567 >/dev/null 2>&1 rc=$?; [ $rc -eq 0 ] && [ -z "$MERGE_NOTE" ] ) || { echo "FAIL merge_ready_pr happy path"; exit 1; } + # THE FALLBACK STATUS CARRIES THE RANGE the caller threads through, and the + # legacy text only when the caller has no range to name. The fallback fires + # exactly when the derivation's own post is missing, so posting the legacy + # text there OVERWRITES the merge head's range-naming description with one + # that names nothing -- which is the status ceasing to attest to anything. + : > "$mdir/desc1.log" + ( PATH="$mdir:$PATH" REPO=demo/demo MAIN_CI_TIMEOUT=31 FAKE_STATUS_STORE="$mdir/s8" \ + FAKE_GH_LOG="$mdir/desc1.log" \ + merge_ready_pr demo 7 headsha1234567 \ + "cross-review PASS (Bircher fallback) on 2742ae4..headsha" >/dev/null 2>&1 ) + grep -q 'description=cross-review PASS (Bircher fallback) on 2742ae4..headsha' "$mdir/desc1.log" \ + || { echo "FAIL merge_ready_pr did not thread the range description to the cross-review status"; cat "$mdir/desc1.log"; exit 1; } + # ...and with no description threaded, the legacy text is still what lands. + : > "$mdir/desc2.log" + ( PATH="$mdir:$PATH" REPO=demo/demo MAIN_CI_TIMEOUT=31 FAKE_STATUS_STORE="$mdir/s9" \ + FAKE_GH_LOG="$mdir/desc2.log" \ + merge_ready_pr demo 7 headsha1234567 >/dev/null 2>&1 ) + grep -q 'description=cross-vendor review PASS (Bircher)' "$mdir/desc2.log" \ + || { echo "FAIL merge_ready_pr lost the legacy description when no range was threaded"; cat "$mdir/desc2.log"; exit 1; } + echo "merge_ready_pr OK (threads the fallback status description)" # ELAPSED-TIME GUARD. The happy path now ENTERS the main-CI watch (the shims used to # return an empty merge sha to skip it, which is the unsafe shortcut #62 removed), so # this asserts the watch is actually bounded by MAIN_CI_POLL_INTERVAL and exits on the @@ -7290,6 +7281,14 @@ SH && grep -q 'state=success' "$slog" \ && grep -q 'context=bircher/cross-review' "$slog" \ || { echo "FAIL merge_ready_pr: cross-review status not posted"; exit 1; } + # Gate integrity: a success already on the head is not re-posted. The + # derivation posts on every verdict; the merge path's own post is a fallback. + : > "$slog" + ( PATH="$mdir:$PATH" REPO=demo/demo FAKE_GH_LOG="$slog" STATUS_PRESENT=1 \ + _post_cross_review_status demo 7 headsha >/dev/null 2>&1 ) + grep -q 'statuses/headsha' "$slog" \ + && { echo "FAIL _post_cross_review_status: re-posted a success that was already on the head"; exit 1; } + echo "_post_cross_review_status skips a present success OK" rm -rf "$mdir" echo "merge_ready_pr OK (incl. #10 cross-review status)" # Task 3 (codex P2-1): status-post unconfirmed -> BEST-EFFORT merge (let branch @@ -7843,6 +7842,16 @@ if [ "$1" = "api" ]; then [ "${FAKE_PROT_RC:-0}" = 0 ] || { echo "HTTP 500 something broke" >&2; exit 1; } [ -n "${FAKE_PROT_CONTEXTS:-}" ] && { printf '%s\n' "$FAKE_PROT_CONTEXTS"; exit 0; } exit 0; } + # merge_base()/delta_digest() (spec §4): a ready recovery now reads the + # PR's compare and tree, which this fixture never modeled. EMPTY, not the + # catch-all below: that fixed two-line string is neither valid JSON (so it + # would silently become an unprovable delta, harmless) NOR pipe/newline-free + # (so as `merge_base`'s raw, unparsed answer it corrupts the derive tuple -- + # a pipe adds a field and a newline trips `_derived_width_ok`'s embedded- + # newline guard, which does not care whether the width required is eight or + # ten). Empty is what `merge_base()`'s `.strip()` and `delta_digest()`'s + # `json.loads(... or "null")` both already treat as "unprovable". + printf '%s\n' "$@" | grep -q '/compare/' && exit 0 printf 'completed|success\ncompleted|success\n'; exit 0 fi exit 0 @@ -7876,42 +7885,6 @@ SH && { echo "FAIL recover_pr_cmd: stamped cross-review with an unusable head ('$_bad')"; rm -rf "$prdir"; exit 1; } done echo "recover_pr_cmd unusable-head refused OK (#66)" - # #66: the prompt must pin the checkout to the CAPTURED sha. The first cut - # passed the sha in and the function ignored it, still fetching the moving - # `pull/N/head` -- so the reviewer could read a different commit than the one - # the merge pinned. Assert the sha reaches the prompt text. - local _pp - _pp=$(REPO=demo/demo _recovery_review_prompt 9 a502a88e20f959c908d00871ee7f25572512dd6d) - case "$_pp" in - # The path now carries a per-review nonce so two reviewers cannot collide - # on it (muesli #745). The PROPERTY is unchanged and is what this asserts: - # the CAPTURED sha, not the moving pull/N/head, is what gets checked out. - *"worktree add --detach /tmp/review-9-a502a88e-oob a502a88e20f959c908d00871ee7f25572512dd6d"*) : ;; - *) echo "FAIL _recovery_review_prompt: checkout not pinned to the captured sha"; exit 1 ;; - esac - case "$_pp" in - *"reviewing EXACTLY commit a502a88e20f959c908d00871ee7f25572512dd6d"*) : ;; - *) echo "FAIL _recovery_review_prompt: prompt does not name the reviewed commit"; exit 1 ;; - esac - # The setup line must not carry `rm -rf`: the guardrail rejects it before - # the reviewer reads anything (muesli #759, three reviews derived as FAIL). - case "$_pp" in - *"rm -rf"*) echo "FAIL _recovery_review_prompt: the setup line carries rm -rf, which the guardrail rejects"; exit 1 ;; - esac - case "$_pp" in - *"git worktree prune; [ ! -e /tmp/review-9-a502a88e-oob ] || mv /tmp/review-9-a502a88e-oob /tmp/review-9-a502a88e-oob.stale."*) : ;; - *) echo "FAIL _recovery_review_prompt: the setup line does not prune and move a leftover aside before adding the worktree"; exit 1 ;; - esac - # With no sha it must still work (pre-#66 behaviour), rather than emitting an - # empty checkout target. - _pp=$(REPO=demo/demo _recovery_review_prompt 9) - case "$_pp" in - # With no sha the nonce falls back to `head`, and the checkout target is - # still FETCH_HEAD -- which is what this asserts. - *"worktree add --detach /tmp/review-9-FETCH_HE-oob FETCH_HEAD"*) : ;; - *) echo "FAIL _recovery_review_prompt: no-sha fallback broken"; exit 1 ;; - esac - echo "_recovery_review_prompt pins the reviewed commit OK (#66)" # BEHIND PR: update-branch FIRST, then review + merge : > "$prdir/log"; : > "$prdir/store" ( PATH="$prdir:$PATH" REPO=demo/demo SERVER=http://x WORKDIR="$prdir" \ @@ -8514,12 +8487,18 @@ SH _post_cross_review_status demo 7 headsha1234567 >/dev/null 2>&1 ) [ -s "$_sd/calls" ] \ || { echo "FAIL #71: with budget remaining, the status post must still reach gh"; rm -rf "$_sd"; exit 1; } - # THE CHECK BEFORE THE VERIFICATION needs the deadline to expire DURING the POST -- - # an already-expired one breaks at the first check and never reaches it. So the fake - # POST outlives the remaining budget, and exactly ONE call must be made. + # THE CHECK BEFORE THE VERIFICATION needs the deadline to expire DURING a call -- + # an already-expired one breaks at the first check and never reaches it. The + # short-circuit's own `commits//status` read is that call now: it is issued + # before the retry loop, `*status*` (not `*statuses*`) makes the fake sleep through + # it too, and `timeout` kills it at the 1s cap -- so the read alone consumes the + # whole deadline DETERMINISTICALLY, not on however long fork/exec happens to take. + # By the time the retry loop's leading `_deadline_passed` check runs, the deadline + # is already spent, so neither the POST nor the verification is ever issued. + # Exactly ONE call -- the short-circuit's read -- must be made. local _sd2; _sd2=$(mktemp -d); : > "$_sd2/calls" printf '%s\n' '#!/usr/bin/env bash' 'echo "$*" >> "$GH_CALLS"' \ - 'case "$*" in *statuses*) sleep 2 ;; esac' 'exit 1' > "$_sd2/gh" + 'case "$*" in *status*) sleep 2 ;; esac' 'exit 1' > "$_sd2/gh" printf '%s\n' '#!/usr/bin/env bash' 'if [ "$1" = "-k" ]; then shift 2; fi' 'shift' 'exec "$@"' > "$_sd2/timeout" chmod +x "$_sd2/gh" "$_sd2/timeout" ( PATH="$_sd2:$PATH"; export PATH; GH_CALLS="$_sd2/calls"; export GH_CALLS @@ -8527,7 +8506,7 @@ SH _TIMEOUT_BIN_LOADED= _TIMEOUT_BIN_CACHE= \ _post_cross_review_status demo 7 headsha1234567 >/dev/null 2>&1 ) [ "$(wc -l < "$_sd2/calls" | tr -d ' ')" = 1 ] \ - || { echo "FAIL #71: a deadline expiring during the POST must stop before the verification (got $(wc -l < "$_sd2/calls") calls)"; rm -rf "$_sd2"; exit 1; } + || { echo "FAIL #71: a deadline expiring during the short-circuit read must stop before the POST and the verification (got $(wc -l < "$_sd2/calls") calls)"; rm -rf "$_sd2"; exit 1; } rm -rf "$_sd2"; unset _sd2 # THE BACKOFF must be capped to what remains. Unbounded, attempt 2's 8s sleep starts # while ~1s of budget is left and overruns by seven -- the phase exceeded by WAITING diff --git a/docs/design/ARCHITECTURE.md b/docs/design/ARCHITECTURE.md index 88b5a44..6fb60e1 100644 --- a/docs/design/ARCHITECTURE.md +++ b/docs/design/ARCHITECTURE.md @@ -121,9 +121,16 @@ session has settled and exits when the tuple is printed. | `human.py` | human interaction: the cursor, the discriminator, the batch rules, dismissals and their replies | | `phases.py` | the front-half loop itself: `retire_owed`, `publish_owed`, parking and resumption; a shaping round before the spec — a one-piece ruling or a slice plan (`coordinator/shape.py`); the slice plan's review and gate; `file_owed` (`coordinator/filing.py`), which files an accepted plan's children in four idempotent passes; and `sweep_sliced` (`coordinator/sweep.py`), run by each wave before it generates the queue | -It returns an eight-field pipe-delimited tuple: +It returns a ten-field pipe-delimited tuple: - outcome|review|note|sha|ci|ci_first|resubmissions|pr + outcome|review|note|sha|ci|ci_first|resubmissions|pr|merge_base|delta_digest + +The last two are what the reviewer actually read (gate integrity, spec §4): +`merge_base` is the merge-base of the PR's base branch with the reviewed head, +and `delta_digest` is the digest of the PR's own three-dot delta at that head. +The runner records both on the `review_verdict` fact, so a verdict says what it +covered rather than only that it happened; the digest is also what lets a +re-stamp after an `update-branch` prove the reviewed content is unchanged. > **TARGET —** the coordinator is the orchestrator. No tuple, no subprocess > boundary, no pipe-delimited transport: the derivation's result is an object @@ -456,9 +463,17 @@ Mode defaults are already `BIRCHER_EFFECT_MODE=kernel` and `BIRCHER_KERNEL_MODE=enforce`. `--source queue` drains `queue/*.md` instead. **The merge gate on muesli:** branch protection requires `review-gate`, NOT -`bircher/cross-review`. They chain — bircher posts cross-review, a workflow -reacts to that status event and posts review-gate ~7s later. `strict: true` is -set, so a PR whose base has moved becomes `BEHIND` and is refused. +`bircher/cross-review`. `bircher/cross-review` is posted by the derivation for +every verdict on the reviewed head: `success` for PASS, `failure` for FAIL +with the blocking count, `error` when no verdict was produced, with ` +round ... on ..` as the description; the merge path +re-posts only when no success is present, and a re-stamp after a +content-identical update-branch keeps the reviewed description. The target +repository's `review-gate` workflow reacts to that status event and posts its +own `review-gate` status on the head ~7s later, requiring `success` on +`bircher/cross-review`; it reads the state only, never the description. +`strict: true` is set, so a PR whose base has moved becomes `BEHIND` and is +refused. --- @@ -682,5 +697,5 @@ programme exists to catch. **What is NOT a gap.** These are done and should not be reopened: the kernel's state machine, effect classes, fact vocabulary and mode switches; the derived outcome replacing the marker as the decision input; effect routing with its -enumerating guards; the pre-merge gate; the eight-field boundary's fail-closed +enumerating guards; the pre-merge gate; the ten-field boundary's fail-closed width check. diff --git a/docs/design/effect-site-inventory.md b/docs/design/effect-site-inventory.md index adbc168..287ce26 100644 --- a/docs/design/effect-site-inventory.md +++ b/docs/design/effect-site-inventory.md @@ -36,19 +36,19 @@ grep -nE "gh .*--add-label|--remove-label" batch/run-queue.sh | 1095 | `curl -X POST $SERVER/v1/sessions/$1/events` (send prompt) | `session_control` | | 1107 | `curl -X POST $SERVER/v1/sessions/$1/events` (stop_session) | `session_control` | | 1121 | `curl -X DELETE $SERVER/v1/sessions/$1` | `session_control` | -| 1198 | `gh api repos/$REPO/statuses/$sha -X POST` | `status_check` | -| 1662 | `gh pr merge --squash --delete-branch` | `merge` | -| 1866 | `git push origin HEAD:main` | `ref_update` | -| 2048 | `gh api repos/$REPO/pulls/$pr/update-branch -X PUT -f expected_head_sha=$sha` (sweep) | `ref_update` | -| 2192 | `git push origin $oid:refs/heads/$branch` (publish) | `ref_update` | -| 2196 | `gh pr create --head $branch --base main` (publish) | `pull_request` | -| 2393 | `gh api repos/$REPO/pulls/$pr/update-branch -X PUT` (recover-pr) | `ref_update` | -| 2643 | `gh pr close` | `pull_request` | -| 3881 | `gh issue comment` | `comment` | -| 3882 | `gh issue edit --remove-label` | `issue_or_label` | -| 3883 | `gh issue edit --add-label` | `issue_or_label` | -| 3899 | `gh issue close` | `issue_or_label` | -| 4508 | `gh issue edit --add-label bircher:running` | `issue_or_label` | +| 1210 | `gh api repos/$REPO/statuses/$sha -X POST` | `status_check` | +| 1645 | `gh pr merge --squash --delete-branch` | `merge` | +| 1849 | `git push origin HEAD:main` | `ref_update` | +| 2031 | `gh api repos/$REPO/pulls/$pr/update-branch -X PUT -f expected_head_sha=$sha` (sweep) | `ref_update` | +| 2175 | `git push origin $oid:refs/heads/$branch` (publish) | `ref_update` | +| 2179 | `gh pr create --head $branch --base main` (publish) | `pull_request` | +| 2376 | `gh api repos/$REPO/pulls/$pr/update-branch -X PUT` (recover-pr) | `ref_update` | +| 2589 | `gh pr close` | `pull_request` | +| 3828 | `gh issue comment` | `comment` | +| 3829 | `gh issue edit --remove-label` | `issue_or_label` | +| 3830 | `gh issue edit --add-label` | `issue_or_label` | +| 3846 | `gh issue close` | `issue_or_label` | +| 4456 | `gh issue edit --add-label bircher:running` | `issue_or_label` | **1519 is `merge`, not `pull_request`.** An earlier draft of the M1-4 plan classified it as `pull_request`. M1-3 split `merge` into its own class @@ -67,11 +67,11 @@ suppression nobody wrote down is a suppression nobody re-reads. | Line | Text | Why it is not a call | |---|---|---| -| 1689 | `MERGE_NOTE="merge deferred: gh pr merge failed"` | assignment value | -| 7357 | `[ "$MERGE_NOTE" = "merge deferred: gh pr merge failed" ]` | string comparison | -| 8299 | `_contains "$_body" '_effect ref_update … git push origin'` | selftest asserting the source contains it | -| 8300 | `echo "FAIL #62: the recovery git push must be routed AND bounded"` | failure message | -| 8323 | `echo "FAIL #62: … a git push that ignores SIGTERM …"` | failure message | +| 1672 | `MERGE_NOTE="merge deferred: gh pr merge failed"` | assignment value | +| 7356 | `[ "$MERGE_NOTE" = "merge deferred: gh pr merge failed" ]` | string comparison | +| 8272 | `_contains "$_body" '_effect ref_update … git push origin'` | selftest asserting the source contains it | +| 8273 | `echo "FAIL #62: the recovery git push must be routed AND bounded"` | failure message | +| 8296 | `echo "FAIL #62: … a git push that ignores SIGTERM …"` | failure message | ## Reads — not journalled @@ -91,14 +91,14 @@ named here. | Line | Call | Class | Why not routed | |---|---|---|---| -| 3657 | `gh run rerun` | *(none)* | **Re-triggers a workflow. Unrouted, and UNDETECTED until 2026-08-29** — `MUTATION` enumerates verbs and `gh run` was a noun it had never been taught, so a whole command family was invisible. Second instance of that class in one day; the first hid behind a wrapper. Now detected, and enumerated by `test_every_gh_subcommand_is_classified` so a new `gh ` cannot join silently. **Left unrouted deliberately:** it creates no object and changes no repository content — it re-runs an existing workflow after an INFRASTRUCTURE failure, and its only influence on a kernel decision is via CI status, which the derivation re-observes rather than trusts. Routing it needs an effect class for "trigger a workflow", which the journal does not have — a design decision, not a migration step. Its real cost is CI minutes, bounded by `BIRCHER_CI_RERUN_MAX` (default 4). | -| 3659 | `gh run rerun` | *(none)* | Same site class as the other `gh run rerun` row above — see it for the reasoning. **Named, not numbered**, because the repointing tool remaps the Line column and cannot follow a line number written in prose: this reference had been stale since the row moved to 3457. | +| 3604 | `gh run rerun` | *(none)* | **Re-triggers a workflow. Unrouted, and UNDETECTED until 2026-08-29** — `MUTATION` enumerates verbs and `gh run` was a noun it had never been taught, so a whole command family was invisible. Second instance of that class in one day; the first hid behind a wrapper. Now detected, and enumerated by `test_every_gh_subcommand_is_classified` so a new `gh ` cannot join silently. **Left unrouted deliberately:** it creates no object and changes no repository content — it re-runs an existing workflow after an INFRASTRUCTURE failure, and its only influence on a kernel decision is via CI status, which the derivation re-observes rather than trusts. Routing it needs an effect class for "trigger a workflow", which the journal does not have — a design decision, not a migration step. Its real cost is CI minutes, bounded by `BIRCHER_CI_RERUN_MAX` (default 4). | +| 3606 | `gh run rerun` | *(none)* | Same site class as the other `gh run rerun` row above — see it for the reasoning. **Named, not numbered**, because the repointing tool remaps the Line column and cannot follow a line number written in prose: this reference had been stale since the row moved to 3457. | | 1017 | `curl -X POST $SERVER/v1/sessions` | `session_control` | **Its response body is parsed, but that is NOT what blocks routing — corrected 2026-08-29.** The recorded reason was that routing needs the intent contract to carry a response. It does not: `perform` already returns the executor's stdout as the external object id, and `_create_session` was routed on exactly that basis, capturing the response and parsing it afterwards. The ACTUAL blocker is `-w`. The upload uses `-w '\n%{http_code}'` to capture the status alongside the body, and `-w` is deliberately absent from the `session_control` contract because its value supports `%output{path}` — an arbitrary filesystem write from a class that exists to control a session. Checked directly: the contract refuses this argv, reading the `-w` value as the URL. **The route out is `--fail-with-body`** (curl 8.14.1 on the runner supports it), which yields the body AND a non-zero exit on an HTTP error, making `-w` unnecessary. Not done here because it changes what the caller receives on failure, on the coordinator's hot path where every run begins. | ## Kernel-executor sites (not shell) -The shaping phase performs its effects from Python through `perform` (`v2/coordinator/filing.py`, `v2/coordinator/sweep.py`), never from `run-queue.sh`, so the routing detector above has no line to cite. They are listed here so the inventory stays the authority on what mutates: +The shaping phase performs its effects from Python through `perform` (`v2/coordinator/filing.py`, `v2/coordinator/sweep.py`), and the derivation performs its own two through `perform_effect` (`v2/coordinator/outcome.py`), never from `run-queue.sh`, so the routing detector above has no line to cite. They are listed here so the inventory stays the authority on what mutates: | Where | Call | Effect class | Bound by | |---|---|---|---| @@ -109,3 +109,5 @@ The shaping phase performs its effects from Python through `perform` (`v2/coordi | `coordinator/filing.py` pass 4 | `gh issue edit --add-label bircher:sliced --remove-label bircher:running` | `issue_or_label` | `umbrella_label` | | `coordinator/sweep.py` | `gh issue comment --body 'bircher: sliced complete…'` | `comment` | `umbrella_close`; `filing_complete` and a same-generation `children_observed_closed` | | `coordinator/sweep.py` | `gh issue close ` | `issue_or_label` | `parent_close`; as above, and only when the parent was observed open | +| `coordinator/outcome.py` `_post_status` | `gh api repos//statuses/ -X POST -f state -f context=bircher/cross-review -f description` | `status_check` | the reviewed head, and the CONTENT of the post: the key is `status::`, so a later verdict on the same head is a different effect rather than a replay of the earlier one. Posted for EVERY verdict (spec §4), best effort -- a failure here does not change the derived outcome | +| `coordinator/outcome.py` `derive` | `gh pr comment --repo --body ` | `comment` | the body's own digest: `pr-comment::`. Best effort, for the same reason | diff --git a/docs/design/scar-effect-matrix.md b/docs/design/scar-effect-matrix.md index e9399bc..e2c2544 100644 --- a/docs/design/scar-effect-matrix.md +++ b/docs/design/scar-effect-matrix.md @@ -28,10 +28,10 @@ v1's own `--self-test` catches the mutation: where it does not, the v2 test is | v1 behaviour | source | mutation that breaks it | v2 owner | test fixture | fault injected | expected durable events | effects | |---|---|---|---|---|---|---|---| | CI failure classification | `run-queue.sh:270` `_classify_ci_failure` `echo genuine` | `-gt 0` → `-ge 0`; or always `infra` — **v1 binds, both directions** | not yet ported — M1-5 | v1 `--self-test` | 0 and 3 failed steps | `external_observation` | none | -| Merge gate fail-closed | `run-queue.sh:3784` `_merge_gate` `printf 'skip'` | drop the empty-head `skip` branch — **v1 binds** | kernel `request_merge` head binding | v1 `--self-test` | no head supplied | `command_rejected` | none | -| Merge-authorizing status publication | `run-queue.sh:1196` `_post_cross_review_status` `_effect status_check` | write the `gh api` call directly instead of through `_effect` — **v2 binds** | kernel effect journal, class `status_check` | `test_routing.py` | an unrouted call planted in the source | `effect_intended`, `effect_confirmed` | `status_check` | -| Merge orchestration | `run-queue.sh:1660` `merge_ready_pr` `_effect merge` | route as `pull_request` instead of `merge` — **v2 binds** | `perform` + `revalidate_merge` | `test_merge_revalidation.py` | reviewed artifact deleted after authorization | `command_rejected`, no `effect_intended` | `merge` | -| Recovery push is bounded | `run-queue.sh:1866` `merge_ready_pr` `revert-push:$pr` | cap → `-`; or `_net_run` drops `-k` — **v1 binds both** | effect adapter cap → `_net_run` | v1 `--self-test` #62 | the bound removed | `effect_uncertain` on timeout | `ref_update` | +| Merge gate fail-closed | `run-queue.sh:3731` `_merge_gate` `printf 'skip'` | drop the empty-head `skip` branch — **v1 binds** | kernel `request_merge` head binding | v1 `--self-test` | no head supplied | `command_rejected` | none | +| Merge-authorizing status publication | `run-queue.sh:1208` `_post_cross_review_status` `_effect status_check` | write the `gh api` call directly instead of through `_effect` — **v2 binds** | kernel effect journal, class `status_check` | `test_routing.py` | an unrouted call planted in the source | `effect_intended`, `effect_confirmed` | `status_check` | +| Merge orchestration | `run-queue.sh:1643` `merge_ready_pr` `_effect merge` | route as `pull_request` instead of `merge` — **v2 binds** | `perform` + `revalidate_merge` | `test_merge_revalidation.py` | reviewed artifact deleted after authorization | `command_rejected`, no `effect_intended` | `merge` | +| Recovery push is bounded | `run-queue.sh:1849` `merge_ready_pr` `revert-push:$pr` | cap → `-`; or `_net_run` drops `-k` — **v1 binds both** | effect adapter cap → `_net_run` | v1 `--self-test` #62 | the bound removed | `effect_uncertain` on timeout | `ref_update` | | Reopening reverted issues is bounded | `run-queue.sh:298` `_reopen_reverted_issues` `_effect issue_or_label` | cap → `-` — **v1 binds** | effect adapter cap → `_net_run` | v1 `--self-test` #62 | the bound removed | `effect_intended` | `issue_or_label` | | The adapter is actually wired in | `run-queue.sh:20` `«top-level»` `effect-adapter.sh` | remove the `source` line — **v1 binds, via #50** | `batch/lib/effect-adapter.sh` | v1 `--self-test` | adapter not sourced | none — the run cannot start | none | | Effect denial fails closed | `effect-adapter.sh:84` `_effect` `BIRCHER_EFFECT_MODE:-deny` | default mode `deny` → `legacy` — **v2 binds** | `batch/lib/effect-adapter.sh` | `test_fault_injection.py` | `BIRCHER_EFFECT_MODE` unset | none — refused before execution | none | @@ -40,7 +40,7 @@ v1's own `--self-test` catches the mutation: where it does not, the v2 test is | v1 behaviour | source | disposition | |---|---|---| -| PR recovery checkout chaining | `run-queue.sh:2201` `recover_pr_cmd` `recover_pr_cmd()` | **Unprobed.** The plan proposed joining commands with `;` instead of `&&` as the breaking mutation. Not executed this session, so no claim is made about whether v1 binds it. Carried to M1-5. | +| PR recovery checkout chaining | `run-queue.sh:2184` `recover_pr_cmd` `recover_pr_cmd()` | **Unprobed.** The plan proposed joining commands with `;` instead of `&&` as the breaking mutation. Not executed this session, so no claim is made about whether v1 binds it. Carried to M1-5. | ## Retired — the code they described no longer exists diff --git a/docs/superpowers/plans/2026-09-19-gate-integrity.md b/docs/superpowers/plans/2026-09-19-gate-integrity.md new file mode 100644 index 0000000..141490a --- /dev/null +++ b/docs/superpowers/plans/2026-09-19-gate-integrity.md @@ -0,0 +1,1206 @@ +# Gate Integrity Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** A `bircher/cross-review` status is posted for every review verdict, names the reviewed range, and is backed by exactly one reviewer prompt that tells the reviewer the range; the verdict fact records what was reviewed. + +**Architecture:** The reviewer prompt lives only in Python (`v2/coordinator/review.py`) and gains one sentence naming `origin/...`. The derivation (`v2/coordinator/outcome.py`) posts the status itself, through the same journalled effect path it already uses for the PR comment, and carries the merge-base sha and the three-dot delta digest out to the runner, which records them on the `review_verdict` fact. The bash copies of the prompt and of the delta digest go; the bash status post becomes an idempotent fallback. + +**Tech Stack:** Python 3 (no third-party packages; `pytest` 8.3.4 run as `cd v2 && python -m pytest -q`), bash (`bash batch/run-queue.sh --self-test`, hermetic, fake `gh` on PATH), GitHub REST via `gh api`, SQLite kernel journal. + +**Spec:** `docs/superpowers/specs/2026-09-19-back-half-closed-loop-design.md`, section 4 "Gate integrity" (the first of that spec's five PRs). + +## Global Constraints + +- No AI attribution anywhere: no `Co-Authored-By`, no `Claude-Session`, no "Generated with" in any commit message or PR body. Commit as the repository's existing author identity (`git log -1 --format='%an <%ae>'` on `main`). +- The muesli `review-gate` workflow is out of scope and unchanged; it keeps requiring a `bircher/cross-review` status with state `success` on the head. +- The status description format is exactly ` round blocking)|no verdict> on ..`, truncated to 140 characters (GitHub's limit). +- The range sentence in the prompt names `git diff origin/...` where `` is the PR's base branch, and says a head commit touching one file does not narrow the review. +- Every GitHub mutation goes through the effect lifecycle (`perform_effect` in Python, `_effect` in bash); nothing posts a status unjournalled in kernel mode. +- Nothing in the front half, the shaping phase, or the kernel's transition table changes in this PR. +- Work on branch `spec/back-half-closed-loop` in worktree `/Users/jonw/bircher/.worktrees/back-half-closed-loop` (it holds the spec); the PR targets `main`. Bircher PR #101 (`fix/recovery-review-range`) is superseded by Task 1 and is closed, not merged, in Task 9. + +--- + +## File structure + +| file | responsibility after this plan | +|---|---| +| `v2/coordinator/review.py` | the only reviewer prompt; `review_prompt` and `dispatch` take the PR's base branch | +| `v2/coordinator/attest.py` (new) | pure helpers: count blocking findings, map a verdict to a status state, render the description | +| `v2/coordinator/delta.py` (new) | the PR's three-dot delta digest, ported from bash; the only implementation | +| `v2/coordinator/outcome.py` | `Deps` gains base/merge-base/digest/round deps; `Derived` carries merge-base and digest (ten fields); `derive` posts the status | +| `v2/coordinator/wiring.py` | wires the three new deps to `gh` | +| `v2/coordinator/cli.py` | `derive` computes the round from the journal; new `delta` subcommand for bash | +| `v2/kernel/commands.py`, `v2/kernel/events.py` | `review_verdict` stores `head_sha`, `merge_base_sha`, `delta_digest`; schema version 2 | +| `batch/lib/kernel-client.sh` | `_kernel_record_review` accepts the three range fields | +| `batch/run-queue.sh` | ten-field tuple; `_recovery_review_prompt` and `_pr_delta_digest` deleted; `_post_cross_review_status` idempotent with optional state/description; restamp uses the CLI digest | +| tests | `v2/tests/coordinator/test_attest.py`, `test_delta.py` (new); `test_ci_and_review.py`, `test_outcome.py`, `test_repair_loop.py`, `v2/tests/kernel/test_review_r3b.py`, `v2/tests/execution/test_settled_pr_reaches_the_caller.py` (modified); the bash self-test | + +--- + +### Task 1: One reviewer prompt, and it names the range + +**Files:** +- Modify: `v2/coordinator/review.py:84-129` (`_PROMPT`, `review_prompt`) and `:132-181` (`dispatch`) +- Modify: `v2/coordinator/wiring.py:107-111` (`do_review`) +- Modify: `batch/run-queue.sh` — delete `_recovery_review_prompt` (comment block from ~2573 through the function's closing `}` at ~2615) and its self-test assertions (~7879-7912) +- Modify: `v2/tests/coordinator/test_ci_and_review.py:456-480` +- Modify: `v2/tests/execution/test_observe_execution.py:10` (docstring only) + +**Interfaces:** +- Produces: `review_prompt(pr: str, repo: str, sha: str = "", nonce: str = "", base: str = "main") -> str`; `dispatch(pr, repo, sha, *, reviewer, bundle_dir, server, log_path, run=None, base: str = "main") -> tuple[str | None, str]`. +- Consumed by Task 5, which wires `base` from `gh`. + +- [ ] **Step 1: Write the failing tests** — replace `test_the_rendered_prompt_is_byte_identical_to_the_bash` (line 456) with these two, keeping `test_an_absent_sha_falls_back_to_FETCH_HEAD`: + +```python +def test_the_prompt_names_the_range_against_the_prs_base(): + p = review_prompt("7", "o/r", "abc123", base="develop") + assert "git diff origin/develop...abc123" in p + assert "NOT only the head commit's own diff" in p + # The pinned checkout is unchanged by naming the range. + assert "You are reviewing EXACTLY commit abc123." in p + + +def test_the_base_defaults_to_main(): + assert "git diff origin/main...abc123" in review_prompt("7", "o/r", "abc123") + + +def test_the_range_reaches_the_runner(tmp_path): + seen = {} + + def run(argv, cwd): + seen["argv"] = argv + return _R(0, "VERDICT: PASS") + + dispatch("7", "o/r", "dead", reviewer="claude_code", bundle_dir="/b", + server="http://s", log_path=str(tmp_path / "l"), run=run, base="release") + assert any("origin/release...dead" in a for a in seen["argv"]), "the base must reach the prompt" +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_ci_and_review.py -k "range or defaults_to_main" -v` +Expected: FAIL — `review_prompt() got an unexpected keyword argument 'base'` and `dispatch() got an unexpected keyword argument 'base'`. + +- [ ] **Step 3: Add the sentence and the parameter.** In `_PROMPT`, insert this line directly after the `You are reviewing EXACTLY commit {co}. ...` line: + +``` +The change under review is the WHOLE pull request as it stands at that commit: every file in `git diff origin/{base}...{co}` (from the PR's merge-base with {base} up to the pinned head), NOT only the head commit's own diff. A head commit that touches one file does not narrow the review to one file. +``` + +Change the signature and the format call: + +```python +def review_prompt(pr: str, repo: str, sha: str = "", nonce: str = "", base: str = "main") -> str: + ... + return _PROMPT.format(pr=pr, repo=repo, co=(sha or "FETCH_HEAD"), + nonce=(nonce or (sha or "head")[:8]), base=(base or "main")) +``` + +In `dispatch`, add `base: str = "main"` as the last keyword parameter and pass `base=base` where it calls `review_prompt(...)`. Replace the docstring sentence above `_PROMPT` that mentions `test_the_rendered_prompt_is_byte_identical_to_the_bash` with: `#: The only copy: the bash `_recovery_review_prompt` it was ported from is gone (gate integrity, spec §4), so nothing keeps two prompts in step because there is one.` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_ci_and_review.py -v` +Expected: all PASS (the byte-identity test no longer exists). + +- [ ] **Step 5: Wire the base in `do_review`** (`v2/coordinator/wiring.py`): + +```python + def base_of(pr): + try: + return _gh(["api", f"repos/{repo}/pulls/{pr}", "--jq", ".base.ref"]).strip() or "main" + except GhError: + return "main" + + def do_review(pr, sha): + return review.dispatch( + str(pr), repo, sha, reviewer=reviewer, bundle_dir=bundle_dir, + server=server, + log_path=os.environ.get("BIRCHER_REVIEW_LOG") or f"/tmp/review-{item}.log", + base=base_of(pr)) +``` + +(`base_of` is also added to `Deps` in Task 4; define it here once and pass it there in Task 5.) + +- [ ] **Step 6: Delete the bash copy.** In `batch/run-queue.sh` remove the whole `_recovery_review_prompt` function together with its leading comment block (`# _recovery_review_prompt -> the read-only reviewer sub-agent input.` through the closing `}` after the heredoc's `EOF`). In the self-test, remove from the comment `# #66: the prompt must pin the checkout to the CAPTURED sha.` through the line `echo "_recovery_review_prompt pins the reviewed commit OK (#66)"` inclusive. Confirm nothing else references it: + +Run: `grep -n "_recovery_review_prompt" batch/run-queue.sh batch/lib/*.sh v2 -r` +Expected: no output. + +Edit the docstring at `v2/tests/execution/test_observe_execution.py:10` so it no longer claims the prompt "renders byte-identically to the bash it came from"; say instead that the prompt is the coordinator's own. + +- [ ] **Step 7: Run the bash self-test and the whole pytest suite** + +Run: `bash batch/run-queue.sh --self-test > /tmp/selftest.log 2>&1; echo rc=$?; tail -3 /tmp/selftest.log` and `cd v2 && python -m pytest -q` +Expected: self-test rc=0 with no `FAIL` line mentioning `_recovery_review_prompt`; pytest all green. (The runner's self-test fails on a root-only test unrelated to this work; run the self-test locally as a non-root user.) + +- [ ] **Step 8: Commit** + +```bash +git add v2/coordinator/review.py v2/coordinator/wiring.py batch/run-queue.sh v2/tests/coordinator/test_ci_and_review.py v2/tests/execution/test_observe_execution.py +git commit -m "One reviewer prompt, and it names the range" +``` + +--- + +### Task 2: What a status attests to (pure helpers) + +**Files:** +- Create: `v2/coordinator/attest.py` +- Test: `v2/tests/coordinator/test_attest.py` + +**Interfaces:** +- Produces: `CONTEXT = "bircher/cross-review"`; `blocking_count(text: str) -> int`; `state_for(verdict: str | None) -> str` (`success`/`failure`/`error`); `describe(vendor: str, round_n: int, verdict: str | None, blocking: int, merge_base: str, head: str) -> str`. +- Consumed by Task 4. + +- [ ] **Step 1: Write the failing tests** + +```python +import pytest + +from coordinator.attest import CONTEXT, blocking_count, describe, state_for + +CODEX_FAIL = """I'll check out the commit. +Blocking findings + +- `src/main/livePromptRelay.ts:206-209` provides `stopAll()` but it is never called. +- The eight-template cap is vulnerable to concurrent creates. + +Non-blocking findings + +- None. + +Suggestions + +- Make snapshot/reread state consistent. + +Verification + +- `go build ./...`: passed +- `go vet ./...`: passed + +VERDICT: FAIL +""" + +CODEX_NUMBERED = """Blocking findings + +1. Hosted deployments have no live-prompts transport. +2. Trashing an active note does not end its live work. +3. Required feature-level E2E coverage is absent. + +Non-blocking findings + +- None. + +VERDICT: FAIL +""" + +CODEX_PASS = """The commit only corrects a regression test.Blocking findings: None. + +Non-blocking findings: None. + +Suggestions: None. + +VERDICT: PASS +""" + + +@pytest.mark.parametrize("text, expected", [ + (CODEX_FAIL, 2), + (CODEX_NUMBERED, 3), + (CODEX_PASS, 0), + ("", 0), + ("no headings at all\nVERDICT: PASS", 0), +]) +def test_blocking_findings_are_counted_from_the_blocking_section_only(text, expected): + assert blocking_count(text) == expected + + +def test_context_is_the_gate_the_repository_requires(): + assert CONTEXT == "bircher/cross-review" + + +@pytest.mark.parametrize("verdict, state", [ + ("PASS", "success"), ("FAIL", "failure"), (None, "error"), ("", "error"), ("BLOCKED", "error"), +]) +def test_state_follows_the_verdict_and_silence_is_an_error(verdict, state): + assert state_for(verdict) == state + + +def test_the_description_names_vendor_round_verdict_and_range(): + d = describe("codex", 3, "PASS", 0, "2742ae4f" + "0" * 32, "e4ea3eb7" + "1" * 32) + assert d == "codex round 3 PASS on 2742ae4..e4ea3eb" + + +def test_a_fail_carries_its_blocking_count(): + assert describe("codex", 4, "FAIL", 3, "a" * 40, "b" * 40) == "codex round 4 FAIL (3 blocking) on aaaaaaa..bbbbbbb" + + +def test_no_verdict_says_so_and_an_unknown_base_is_a_question_mark(): + assert describe("claude_code", 1, None, 0, "", "b" * 40) == "claude_code round 1 no verdict on ?..bbbbbbb" + + +def test_the_description_never_exceeds_githubs_limit(): + assert len(describe("x" * 200, 1, "PASS", 0, "a" * 40, "b" * 40)) <= 140 +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_attest.py` +Expected: FAIL with `ModuleNotFoundError: No module named 'coordinator.attest'`. + +- [ ] **Step 3: Write the module** + +```python +"""What a cross-review status attests to (spec §4, gate integrity). + +Pure. The description names the vendor, the round, the verdict and the +merge-base..head range, so a person reading the PR's checks can see exactly +what was reviewed; the state follows the verdict, and silence is an error, +never a pass. +""" + +from __future__ import annotations + +import re + +#: The context muesli's `review-gate` workflow requires `success` on. +CONTEXT = "bircher/cross-review" + +#: GitHub truncates longer descriptions; truncate deliberately instead. +_MAX_DESCRIPTION = 140 + +_BLOCKING = re.compile(r"^\W*blocking findings\W*(.*)$", re.I) +_OTHER_SECTION = re.compile(r"^\W*(non-blocking findings|suggestions|verification|verdict)\b", re.I) +#: A list item at the section's own indentation: `- `, `* `, `• `, `1. `, `1) `. +_ITEM = re.compile(r"^ {0,2}(?:[-*•]|\d+[.)])\s+\S") +_NONE = re.compile(r"^(none|n/a|no blocking findings)\.?$", re.I) + + +def blocking_count(text: str) -> int: + """How many items the reviewer listed under its blocking-findings heading. + + Counts list items between the heading and the next section heading, at + the section's own indentation (a nested sub-bullet elaborates a finding, + it is not another one). A heading followed on the same line by `None` + counts nothing; a heading followed by a finding on the same line counts + one. Text with no heading counts nothing, so an unstructured FAIL posts + `0 blocking` rather than a guess. + """ + count, inside = 0, False + for line in (text or "").splitlines(): + m = _BLOCKING.match(line) + if m: + inside = True + rest = m.group(1).strip() + if rest and not _NONE.match(rest): + count += 1 + continue + if inside and _OTHER_SECTION.match(line): + break + if inside and _ITEM.match(line): + count += 1 + return count + + +def state_for(verdict: str | None) -> str: + """`success` for PASS, `failure` for FAIL, `error` for anything else. + + BLOCKED, an empty verdict and a reviewer that never produced one all + land on `error`: none of them is an approval, and the gate requires + `success` by name. + """ + return {"PASS": "success", "FAIL": "failure"}.get(verdict or "", "error") + + +def describe(vendor: str, round_n: int, verdict: str | None, blocking: int, + merge_base: str, head: str) -> str: + """` round on ..`.""" + if verdict == "PASS": + word = "PASS" + elif verdict == "FAIL": + word = f"FAIL ({blocking} blocking)" + else: + word = "no verdict" + rng = f"{(merge_base or '')[:7] or '?'}..{(head or '')[:7] or '?'}" + return f"{vendor} round {round_n} {word} on {rng}"[:_MAX_DESCRIPTION] +``` + +- [ ] **Step 4: Run the tests to verify they pass** + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_attest.py -v` +Expected: all PASS. + +- [ ] **Step 5: Commit** + +```bash +git add v2/coordinator/attest.py v2/tests/coordinator/test_attest.py +git commit -m "Name what a cross-review status attests to" +``` + +--- + +### Task 3: The delta digest in Python, and only there + +**Files:** +- Create: `v2/coordinator/delta.py` +- Modify: `v2/coordinator/cli.py` (new `delta` subcommand beside `derive`) +- Modify: `batch/run-queue.sh` — `_restamp_if_delta_unchanged` (~1298-1350) calls the CLI; `_pr_delta_digest` and its comment block deleted +- Test: `v2/tests/coordinator/test_delta.py` + +**Interfaces:** +- Produces: `canonical_delta(compare: dict, tree: dict) -> str | None` (the canonical JSON text, or None when unprovable); `delta_digest(repo: str, base: str, ref: str, gh=_gh) -> str` (sha256 hex, or `""` when unprovable); CLI `python3 -m coordinator.cli delta --repo --base --ref ` printing the hex digest, rc 0, or nothing with rc 1. +- Consumed by Task 4 (`Deps.delta_digest`) and by the bash restamp. + +- [ ] **Step 1: Write the failing tests.** The fixtures are the self-test's own (`batch/run-queue.sh` ~7662 and ~7673), so the Python answer is pinned to what the bash answered for the same input before the bash was deleted: + +```python +import hashlib +import json +import subprocess +from pathlib import Path + +import pytest + +from coordinator.delta import canonical_delta, delta_digest + +COMPARE_B1 = {"files": [{"filename": "a.txt", "status": "modified", + "sha": "blob0000000000000000000000000000000000b1", + "patch": "@@ -1 +1 @@\n-old\n+new"}]} +COMPARE_B2 = {"files": [{"filename": "a.txt", "status": "modified", + "sha": "blob0000000000000000000000000000000000b2", + "patch": "@@ -1 +1 @@\n-old\n+SOMETHING ELSE"}]} +TREE = {"truncated": False, "tree": [{"path": "a.txt", "mode": "100644", "type": "blob"}]} + + +def _gh_for(compare, tree): + def gh(args): + joined = " ".join(args) + if "/compare/" in joined: + return json.dumps(compare) + if "/git/trees/" in joined: + return json.dumps(tree) + raise AssertionError(f"unexpected gh call {args}") + return gh + + +def test_the_canonical_form_is_sorted_compact_and_carries_mode_and_type(): + text = canonical_delta(COMPARE_B1, TREE) + assert text == ('[{"entry":"100644|blob","filename":"a.txt","patch":"@@ -1 +1 @@\\n-old\\n+new",' + '"previous_filename":null,"sha":"blob0000000000000000000000000000000000b1",' + '"status":"modified"}]') + + +def test_the_digest_is_the_sha256_of_the_canonical_form(): + text = canonical_delta(COMPARE_B1, TREE) + assert delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) == hashlib.sha256(text.encode()).hexdigest() + + +def test_a_changed_patch_changes_the_digest(): + assert delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) != \ + delta_digest("o/r", "main", "b2", gh=_gh_for(COMPARE_B2, TREE)) + + +def test_a_path_absent_from_the_tree_is_marked_absent(): + assert '"entry":"ABSENT"' in canonical_delta(COMPARE_B1, {"truncated": False, "tree": []}) + + +@pytest.mark.parametrize("compare, tree", [ + ({"files": []}, TREE), # 0 files + ({"files": [dict(COMPARE_B1["files"][0]) for _ in range(300)]}, TREE), # GitHub's cap + ({"files": [{"filename": "a.txt", "status": "modified", "sha": "x"}]}, TREE), # patch withheld + (COMPARE_B1, {"truncated": True, "tree": []}), # truncated tree + (COMPARE_B1, {"tree": []}), # truncated absent, not false +]) +def test_every_unprovable_case_yields_no_digest(compare, tree): + assert canonical_delta(compare, tree) is None + assert delta_digest("o/r", "main", "b1", gh=_gh_for(compare, tree)) == "" + + +def test_a_failed_lookup_is_unprovable_not_a_crash(): + def gh(args): + raise RuntimeError("boom") + assert delta_digest("o/r", "main", "b1", gh=gh) == "" + + +def test_the_cli_prints_the_digest_or_nothing(tmp_path, monkeypatch): + # A fake gh on PATH, answering by URL shape as the self-test's does. + fake = tmp_path / "gh" + fake.write_text('#!/bin/bash\ncase "$*" in *"/compare/"*) printf %s \'' + json.dumps(COMPARE_B1) + + '\' ;; *"/git/trees/"*) printf %s \'' + json.dumps(TREE) + '\' ;; *) exit 1 ;; esac\n') + fake.chmod(0o755) + env = {"PATH": f"{tmp_path}:/usr/bin:/bin", "BIRCHER_GH_REPO": "o/r"} + r = subprocess.run(["python3", "-m", "coordinator.cli", "delta", "--repo", "o/r", "--base", "main", "--ref", "b1"], + capture_output=True, text=True, env=env, cwd=str(Path(__file__).resolve().parents[2])) + assert r.returncode == 0 + assert r.stdout.strip() == delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) +``` + +- [ ] **Step 2: Run them to verify they fail** + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_delta.py` +Expected: FAIL with `ModuleNotFoundError: No module named 'coordinator.delta'`. + +- [ ] **Step 3: Before writing the port, freeze the bash answer.** With the bash function still present, render its digest for the fixture so the port is checked against it once: + +```bash +cd /Users/jonw/bircher/.worktrees/back-half-closed-loop +tmp=$(mktemp -d); cat > "$tmp/gh" <<'SH' +#!/bin/bash +case "$*" in + *"/compare/"*) printf '%s' '{"files":[{"filename":"a.txt","status":"modified","sha":"blob0000000000000000000000000000000000b1","patch":"@@ -1 +1 @@\n-old\n+new"}]}' ;; + *"/git/trees/"*) printf '%s' '{"truncated":false,"tree":[{"path":"a.txt","mode":"100644","type":"blob"}]}' ;; + *) exit 1 ;; +esac +SH +chmod +x "$tmp/gh" +PATH="$tmp:$PATH" REPO=o/r bash -c 'source <(sed -n "/^_sha256()/,/^}/p; /^_pr_delta_digest()/,/^}/p" batch/run-queue.sh); _pr_delta_digest main b1' +``` + +Record the printed hex. Step 4's port must reproduce it: add `test_the_port_reproduces_the_bash_digest_for_the_fixture` asserting `delta_digest("o/r","main","b1", gh=_gh_for(COMPARE_B1, TREE)) == ""`. If it does not match, the canonical JSON differs (key order, spacing, unicode or null rendering); fix the port, never the expected value. + +- [ ] **Step 4: Write the module** + +```python +"""The PR's own three-dot delta, digested (gate integrity, spec §4). + +Ported from `_pr_delta_digest` in `batch/run-queue.sh`, which is gone: one +implementation, used by the derivation (to record what was reviewed on the +verdict fact) and by the runner's re-stamp after an update-branch (through +`coordinator.cli delta`). + +GitHub's compare is three-dot, so `base...ref` is the change the PR +contributes and never the base branch's own commits. That property is what +makes the digest comparable across an update-branch: that operation MERGES +the base into the head (it does not rebase), so the new head CONTAINS the new +base, the merge-base IS the new base, and the compare still yields only the +PR's work. + +An unprovable delta is `None`, never "assume equal": GitHub caps the +changed-file list at 300, omits `patch` for binaries, pure renames and +anything over its size limit, and truncates a large tree. Digesting a partial +answer would produce a confident wrong one, which here means merging code no +reviewer read. +""" + +from __future__ import annotations + +import hashlib +import json + +from coordinator.ci import GhError, _gh + +#: GitHub's documented cap on a compare's file list; at exactly 300 it may be truncated. +_FILE_CAP = 300 + + +def canonical_delta(compare: dict, tree: dict) -> str | None: + """The canonical text of the delta, or None when it cannot be established.""" + files = (compare or {}).get("files") + if not isinstance(files, list): + return None + if not (1 <= len(files) < _FILE_CAP): + return None + if any("patch" not in f for f in files): + return None + # Require an explicit false: an absent or null `truncated` is an answer we + # did not get, not a reassuring one. + if (tree or {}).get("truncated") is not False: + return None + entries = {e.get("path"): f"{e.get('mode')}|{e.get('type')}" for e in (tree.get("tree") or [])} + rows = [] + for f in sorted(files, key=lambda f: f["filename"]): + rows.append({ + "filename": f["filename"], + "previous_filename": f.get("previous_filename"), + "status": f.get("status"), + "sha": f.get("sha"), + "patch": f.get("patch"), + "entry": entries.get(f["filename"], "ABSENT"), + }) + # Sorted keys, no whitespace, unicode kept raw: the same bytes `jq -cS` produced. + return json.dumps(rows, sort_keys=True, separators=(",", ":"), ensure_ascii=False) + + +def delta_digest(repo: str, base: str, ref: str, gh=_gh) -> str: + """sha256 of the canonical delta of `base...ref`, or "" when unprovable.""" + try: + compare = json.loads(gh(["api", f"repos/{repo}/compare/{base}...{ref}"]) or "null") + tree = json.loads(gh(["api", f"repos/{repo}/git/trees/{ref}?recursive=1"]) or "null") + except (GhError, ValueError, TypeError, RuntimeError): + return "" + text = canonical_delta(compare, tree) + if text is None: + return "" + return hashlib.sha256(text.encode("utf-8")).hexdigest() +``` + +- [ ] **Step 5: Add the CLI subcommand** in `v2/coordinator/cli.py`, next to the `derive` parser: + +```python + dl = subs.add_parser("delta") + dl.add_argument("--repo", required=True) + dl.add_argument("--base", required=True) + dl.add_argument("--ref", required=True) +``` + +and in the dispatch section, before `if a.mode == "derive":`: + +```python + if a.mode == "delta": + from coordinator.delta import delta_digest + os.environ["BIRCHER_GH_REPO"] = a.repo + digest = delta_digest(a.repo, a.base, a.ref) + if not digest: + return 1 + print(digest, end="") + return RC_OK +``` + +- [ ] **Step 6: Run the tests to verify they pass** (including the frozen-digest test from Step 3) + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_delta.py -v` +Expected: all PASS. + +- [ ] **Step 7: Point the restamp at the CLI and delete the bash function.** In `_restamp_if_delta_unchanged` replace + +```bash + old_digest=$(_pr_delta_digest "$base" "$reviewed") || old_digest="" + new_digest=$(_pr_delta_digest "$base" "$new") || new_digest="" +``` + +with + +```bash + old_digest=$(_net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + python3 -m coordinator.cli delta --repo "$REPO" --base "$base" --ref "$reviewed" 2>/dev/null) || old_digest="" + new_digest=$(_net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + python3 -m coordinator.cli delta --repo "$REPO" --base "$base" --ref "$new" 2>/dev/null) || new_digest="" +``` + +(`python3 -m coordinator.cli` is how `observe_outcome` already invokes the coordinator; copy its working-directory handling from that call site, ~2995-3036, so the module resolves.) Delete `_pr_delta_digest` and its comment block (from `# _pr_delta_digest -> ...` through the function's `}`); keep `_sha256`, which other code uses. + +Run: `grep -n "_pr_delta_digest" batch/run-queue.sh batch/lib/*.sh` +Expected: no output. + +- [ ] **Step 8: Run the self-test; the two delta fixtures are the oracle** + +Run: `bash batch/run-queue.sh --self-test > /tmp/selftest.log 2>&1; echo rc=$?; grep -n "sweep-behind" /tmp/selftest.log` +Expected: rc=0; both `sweep-behind-changed` (delta CHANGED → no re-stamp) and `sweep-behind-same` (delta identical → re-stamped) still pass. If the Python CLI cannot find `coordinator` from the self-test's working directory, mirror `observe_outcome`'s `cd`/`PYTHONPATH` handling exactly. + +- [ ] **Step 9: Commit** + +```bash +git add v2/coordinator/delta.py v2/coordinator/cli.py batch/run-queue.sh v2/tests/coordinator/test_delta.py +git commit -m "One delta digest, in the coordinator" +``` + +--- + +### Task 4: The derivation records the range and posts a status for every verdict + +**Files:** +- Modify: `v2/coordinator/outcome.py:27-115` (`Deps`, `Derived`) and `:169-266` (`derive`) +- Modify: `v2/tests/coordinator/test_outcome.py`, `v2/tests/coordinator/test_repair_loop.py:109-113` +- Test: additions to `v2/tests/coordinator/test_outcome.py` + +**Interfaces:** +- Consumes: Task 2's `attest` helpers. +- Produces: `Deps.base_of: callable = lambda pr: "main"`, `Deps.merge_base: callable = lambda base, head: ""`, `Deps.delta_digest: callable = lambda base, head: ""`, `Deps.round_number: int = 1`; `Derived.merge_base: str = ""`, `Derived.delta_digest: str = ""`; `Derived.FIELDS == 10`; `as_line()` = `outcome|review|note|sha|ci|ci_first|resubmissions|pr|merge_base|delta_digest`. +- Consumed by Tasks 5 and 7. + +- [ ] **Step 1: Write the failing tests.** Find how `test_outcome.py` builds a `Deps` for a green PR with a PASS review (it has one near line 33; copy its construction into a helper if none exists) and add: + +```python +def _green_pass_deps(effects, verdict="PASS", out="VERDICT: PASS"): + return Deps( + checks=lambda pr: "ci|pass", + head_of=lambda pr: "e4ea3eb7" + "1" * 32, + review=lambda pr, sha: (verdict, out), + effect=lambda cls, key, argv: effects.append((cls, key, argv)) or "ok", + base_of=lambda pr: "develop", + merge_base=lambda base, head: "2742ae4f" + "0" * 32, + delta_digest=lambda base, head: "d" * 64, + round_number=3, + reviewer="codex", + repo="o/r", + ) + + +def test_the_line_has_ten_fields_and_carries_the_range(): + effects = [] + r = derive("i", "c", "7", "", deps=_green_pass_deps(effects)) + assert r.FIELDS == 10 + fields = r.as_line().split("|") + assert len(fields) == 10 + assert fields[8] == "2742ae4f" + "0" * 32 + assert fields[9] == "d" * 64 + + +def test_a_pass_posts_a_success_status_naming_the_range(): + effects = [] + derive("i", "c", "7", "", deps=_green_pass_deps(effects)) + posts = [e for e in effects if e[0] == "status_check"] + assert len(posts) == 1 + cls, key, argv = posts[0] + assert key == "status:" + "e4ea3eb7" + "1" * 32 + ":3" + assert "repos/o/r/statuses/e4ea3eb7" + "1" * 32 in argv + assert "state=success" in argv + assert "context=bircher/cross-review" in argv + assert "description=codex round 3 PASS on 2742ae4..e4ea3eb" in argv + + +def test_a_fail_posts_failure_with_its_blocking_count(): + effects = [] + out = "Blocking findings\n\n- one\n- two\n\nNon-blocking findings\n\n- None.\n\nVERDICT: FAIL\n" + derive("i", "c", "7", "", deps=_green_pass_deps(effects, verdict="FAIL", out=out)) + argv = [e for e in effects if e[0] == "status_check"][0][2] + assert "state=failure" in argv + assert "description=codex round 3 FAIL (2 blocking) on 2742ae4..e4ea3eb" in argv + + +def test_no_verdict_posts_error(): + effects = [] + derive("i", "c", "7", "", deps=_green_pass_deps(effects, verdict="NONE", out="rambling")) + argv = [e for e in effects if e[0] == "status_check"][0][2] + assert "state=error" in argv + assert "no verdict on 2742ae4..e4ea3eb" in " ".join(argv) + + +def test_a_failed_status_post_does_not_change_the_outcome(): + def effect(cls, key, argv): + if cls == "status_check": + raise RuntimeError("github is down") + return "ok" + d = _green_pass_deps([]) + d.effect = effect + r = derive("i", "c", "7", "", deps=d) + assert r.outcome == "ready" + + +def test_no_status_is_posted_without_a_pinned_head(): + effects = [] + d = _green_pass_deps(effects) + d.head_of = lambda pr: "not-a-sha" + derive("i", "c", "7", "", deps=d) + assert not [e for e in effects if e[0] == "status_check"] + + +def test_red_ci_posts_nothing_and_carries_no_range(): + effects = [] + d = _green_pass_deps(effects) + d.checks = lambda pr: "ci|fail" + r = derive("i", "c", "7", "", deps=d) + assert not [e for e in effects if e[0] == "status_check"] + assert r.merge_base == "" and r.delta_digest == "" +``` + +Also change `test_repair_loop.py:111` to `assert len(d.as_line().split("|")) == Derived.FIELDS == 10` and give its `Derived(...)` construction two trailing keyword arguments `merge_base="", delta_digest=""` only if it uses positional `findings` (it uses `findings=` by keyword, so no change there). + +- [ ] **Step 2: Run them to verify they fail** + +Run: `cd v2 && python -m pytest -q tests/coordinator/test_outcome.py -k "ten_fields or posts or status or pinned_head or carries_no_range" -v` +Expected: FAIL — `Deps.__init__() got an unexpected keyword argument 'base_of'`. + +- [ ] **Step 3: Widen `Deps` and `Derived`.** In `Deps`, after `repo: str = ""`: + +```python + #: The PR's base branch, for the reviewer's range and the merge-base. + base_of: callable = lambda pr: "main" + #: (base, head) -> merge-base sha, or "" when unknown. + merge_base: callable = lambda base, head: "" + #: (base, head) -> the three-dot delta digest, or "" when unprovable. + delta_digest: callable = lambda base, head: "" + #: Which review round this is, from the journal (revisions used + 1). + round_number: int = 1 +``` + +In `Derived`, after `findings: str = ""`: + +```python + #: What the review covered: the merge-base with the PR's base, and the + #: digest of the PR's own delta at the reviewed head. Both ride out on + #: every derivation that pinned a head, so the runner can record them on + #: the verdict fact (spec §4). + merge_base: str = "" + delta_digest: str = "" +``` + +Change `FIELDS = 8` to `FIELDS = 10`, and `as_tuple`/`as_line` to append the two: + +```python + def as_tuple(self): + return (self.outcome, self.review, self.note, self.sha, self.ci, + self.ci_first, self.resubmissions, self.pr, self.merge_base, + self.delta_digest) + + def as_line(self) -> str: + """The TEN-field form the shell parses. ...""" # keep the existing warning text + r = "" if self.resubmissions is None else self.resubmissions + return (f"{self.outcome}|{self.review}|{self.note}|{self.sha}|" + f"{self.ci}|{self.ci_first}|{r}|{self.pr}|{self.merge_base}|{self.delta_digest}") +``` + +- [ ] **Step 4: Post the status and carry the range in `derive`.** Add `from coordinator import attest` at the top. Inside `derive`, replace the block from `ci, verdict, reviewed_sha, reviewer_out = "na", None, "", ""` through the review call with: + +```python + ci, verdict, reviewed_sha, reviewer_out = "na", None, "", "" + merge_base, digest = "", "" + if pr: + ci = _settle_ci(item, pr, d, rerun_max) + + if ci == "green": + head = (d.head_of(pr) or "").strip() + if _FULL_SHA.match(head): + reviewed_sha = head + else: + d.log(f"{item}: could not capture a full 40-hex head for PR #{pr} " + f"(got {head or ''!r}) -> cannot pin a merge") + d.log(f"{item}: PR #{pr} CI green -> {d.reviewer} review at {reviewed_sha[:7]}") + verdict, reviewer_out = d.review(pr, reviewed_sha) + if verdict == "NONE": + verdict = None + if reviewed_sha: + # WHAT WAS REVIEWED, captured against the same pinned head the + # reviewer was told to check out (spec §4): the merge-base with + # the PR's base and the digest of the PR's own delta. + base = d.base_of(pr) or "main" + merge_base = (d.merge_base(base, reviewed_sha) or "").strip() + digest = d.delta_digest(base, reviewed_sha) or "" + _post_status(item, pr, reviewed_sha, verdict, reviewer_out, + merge_base, d) +``` + +and add, at module level below `_settle_ci`: + +```python +def _post_status(item, pr, head, verdict, reviewer_out, merge_base, d: Deps) -> None: + """Every verdict posts `bircher/cross-review` on the reviewed head: success, + failure, or error, with the range in the description (spec §4). A FAIL + can no longer leave an older success standing. + + BEST EFFORT, as the PR comment is: the outcome is derived from the + repository and is already correct; a transient GitHub failure here must + not turn a derivation into an escalation. The runner's own idempotent + post before the merge is the fallback for a missing success. + """ + blocking = attest.blocking_count(reviewer_out) if verdict == "FAIL" else 0 + state = attest.state_for(verdict) + desc = attest.describe(d.reviewer, d.round_number, verdict, blocking, merge_base, head) + key = f"status:{head}:{d.round_number}" + try: + d.effect("status_check", key, + ["gh", "api", f"repos/{d.repo}/statuses/{head}", "-X", "POST", + "-f", f"state={state}", "-f", f"context={attest.CONTEXT}", + "-f", f"description={desc}"]) + d.log(f"{item}: posted {attest.CONTEXT}={state} on {head[:7]} ({desc})") + except Exception as exc: # noqa: BLE001 + d.log(f"{item}: failed to post {attest.CONTEXT}={state} on PR #{pr} " + f"({type(exc).__name__}: {exc}) -> continuing; the outcome is " + f"derived from the repository and is unaffected") +``` + +Finally, the return: `merge_base` and `digest` ride out whenever a head was pinned, so change the final `return Derived(...)` to pass `merge_base=merge_base if reviewed_sha else "", delta_digest=digest if reviewed_sha else ""` as keywords after `findings=...`. + +- [ ] **Step 5: Run the coordinator tests** + +Run: `cd v2 && python -m pytest -q tests/coordinator -v` +Expected: all PASS, including every pre-existing test in `test_outcome.py` and `test_repair_loop.py` (their `Derived(...)` constructions use keyword `findings`, so the two new trailing fields default to `""`). + +- [ ] **Step 6: Commit** + +```bash +git add v2/coordinator/outcome.py v2/tests/coordinator/test_outcome.py v2/tests/coordinator/test_repair_loop.py +git commit -m "Post a cross-review status for every verdict, naming the range" +``` + +--- + +### Task 5: Wire the range to `gh`, and the round to the journal + +**Files:** +- Modify: `v2/coordinator/wiring.py:28-150` (`live_deps`) +- Modify: `v2/coordinator/cli.py:475-500` (`derive`) +- Test: `v2/tests/coordinator/test_wiring.py` if it exists, else add to `v2/tests/coordinator/test_outcome.py` + +**Interfaces:** +- Consumes: Task 1's `base_of`, Task 3's `delta_digest`, Task 4's `Deps` fields. +- Produces: `live_deps(..., round_number: int = 1)`. + +- [ ] **Step 1: Write the failing test.** `live_deps` is built from `gh`; test the round computation, which is pure, by extracting it: + +```python +from coordinator.cli import _round_number + + +def test_the_round_is_revisions_used_plus_one(tmp_path): + from kernel.store import Store + db = tmp_path / "k.db" + Store.open(str(db)) # an empty journal + assert _round_number(str(db), "run-1") == 1 + + +def test_no_journal_means_round_one(): + assert _round_number("", "") == 1 + assert _round_number("/nonexistent/path.db", "run-1") == 1 +``` + +- [ ] **Step 2: Run it to verify it fails** + +Run: `cd v2 && python -m pytest -q tests/coordinator -k round_number -v` +Expected: FAIL with `ImportError: cannot import name '_round_number'`. + +- [ ] **Step 3: Implement.** In `cli.py`, above the `main`/dispatch function: + +```python +def _round_number(db: str, run_id: str) -> int: + """Which review round the derivation is about to record: revisions used + so far, from the journal, plus one. No journal, no run, or an unreadable + database means round one -- the description must never block a status. + """ + if not db or not run_id or not os.path.exists(db): + return 1 + try: + from kernel.store import Store + from coordinator.observe import revisions_used + return revisions_used(Store.open(db).facts_for(run_id)) + 1 + except Exception: # noqa: BLE001 + return 1 +``` + +and in the `derive` branch pass it: `round_number=_round_number(os.environ.get("BIRCHER_KERNEL_DB", ""), os.environ.get("BIRCHER_RUN_ID", ""))` into `live_deps(...)`. + +In `wiring.py`, add the parameter `round_number: int = 1` to `live_deps`, and wire the three deps in the `Deps(...)` construction (import `delta` from `coordinator` beside `review`): + +```python + def merge_base(base, head): + try: + return _gh(["api", f"repos/{repo}/compare/{base}...{head}", + "--jq", ".merge_base_commit.sha"]).strip() + except GhError: + return "" +``` + +and in `return Deps(...)`: + +```python + base_of=base_of, + merge_base=merge_base, + delta_digest=lambda base, head: delta.delta_digest(repo, base, head), + round_number=round_number, +``` + +- [ ] **Step 4: Run the tests** + +Run: `cd v2 && python -m pytest -q tests/coordinator tests/execution -x` +Expected: all PASS. + +- [ ] **Step 5: Commit** + +```bash +git add v2/coordinator/wiring.py v2/coordinator/cli.py v2/tests/coordinator +git commit -m "Wire the reviewed range and the round into the derivation" +``` + +--- + +### Task 6: The verdict fact records the range + +**Files:** +- Modify: `v2/kernel/commands.py:646-668` (the `record_review` fact payload) +- Modify: `v2/kernel/events.py` (`SCHEMA_VERSIONS[EventKind.REVIEW_VERDICT]`) +- Test: `v2/tests/kernel/test_review_r3b.py` + +**Interfaces:** +- Produces: `review_verdict` payload keys `head_sha`, `merge_base_sha`, `delta_digest` (strings or `None`); schema version 2. +- Consumed by Task 7's runner call and by the next PR's step function. + +- [ ] **Step 1: Write the failing test.** Copy the setup of `test_request_merge_with_an_accepted_review_is_authorized` in `v2/tests/kernel/test_authorization.py` (a run with an implementation output, then `record_review` with `accept`) into `test_review_r3b.py` as a helper, and add: + +```python +def test_the_verdict_records_what_it_reviewed(store_and_run): + store, run_id = store_and_run + payload = _accept_payload(store, run_id) # the helper's artifact/base/context binding + payload.update({"head_sha": "e" * 40, "merge_base_sha": "2" * 40, "delta_digest": "d" * 64}) + submit(store, command("record_review", run_id, payload)) + fact = [f for f in store.facts_for(run_id) if f.kind == EventKind.REVIEW_VERDICT][-1] + assert fact.payload["head_sha"] == "e" * 40 + assert fact.payload["merge_base_sha"] == "2" * 40 + assert fact.payload["delta_digest"] == "d" * 64 + assert fact.schema_version == 2 + + +def test_a_verdict_without_a_range_records_none_not_garbage(store_and_run): + store, run_id = store_and_run + submit(store, command("record_review", run_id, _accept_payload(store, run_id))) + fact = [f for f in store.facts_for(run_id) if f.kind == EventKind.REVIEW_VERDICT][-1] + assert fact.payload["head_sha"] is None + assert fact.payload["merge_base_sha"] is None + assert fact.payload["delta_digest"] is None +``` + +(Use the file's existing fixture and `submit`/`command` helpers by their real names; `_accept_payload` is the helper you write from the copied setup.) + +- [ ] **Step 2: Run it to verify it fails** + +Run: `cd v2 && python -m pytest -q tests/kernel/test_review_r3b.py -k "records_what_it_reviewed or without_a_range" -v` +Expected: FAIL with `KeyError: 'head_sha'`. + +- [ ] **Step 3: Store the fields.** In the `record_review` payload dict in `commands.py`, after `"generation": cmd.generation,` add: + +```python + # What was reviewed (spec §4, gate integrity): the pinned + # head, its merge-base with the PR's base, and the digest + # of the PR's own delta. None when the caller had none + # (a human ruling, or a derivation that pinned no head). + "head_sha": cmd.payload.get("head_sha"), + "merge_base_sha": cmd.payload.get("merge_base_sha"), + "delta_digest": cmd.payload.get("delta_digest"), +``` + +In `events.py` change `EventKind.REVIEW_VERDICT: 1,` to `EventKind.REVIEW_VERDICT: 2,` with the comment `# 2: head_sha, merge_base_sha, delta_digest (gate integrity)`. + +- [ ] **Step 4: Run the kernel tests** + +Run: `cd v2 && python -m pytest -q tests/kernel -x` +Expected: all PASS (the schema-version tests assert the map's values are `>= 1` and match, so 2 is accepted). + +- [ ] **Step 5: Commit** + +```bash +git add v2/kernel/commands.py v2/kernel/events.py v2/tests/kernel/test_review_r3b.py +git commit -m "The verdict fact records what it reviewed" +``` + +--- + +### Task 7: The runner reads ten fields and records the range + +**Files:** +- Modify: `batch/lib/kernel-client.sh` (`_kernel_record_review`, ~855-880) +- Modify: `batch/run-queue.sh` — `_derived_width_ok` (~1010), the `IFS='|' read -r ...` in `run_item` (~5019), the `_kernel_record_review` call (~5062), and the two `obs="escalated|..."` fallbacks (~5003 and ~5013) which must now carry ten fields +- Modify: `v2/tests/execution/test_settled_pr_reaches_the_caller.py`, `v2/tests/execution/test_run_item_argument_wiring.py` (lines that assert `_kernel_record_review`'s argument list) + +**Interfaces:** +- Consumes: Task 4's ten-field line; Task 6's payload keys. +- Produces: `_kernel_record_review [key] [terminal] [head_sha] [merge_base_sha] [delta_digest]`. + +- [ ] **Step 1: Write the failing tests.** In `test_settled_pr_reaches_the_caller.py`, `test_the_shell_reads_as_many_fields_as_the_line_emits` already asserts the `read -r` name count equals `Derived.FIELDS`; it fails by itself once Task 4 landed. Add, in `test_run_item_argument_wiring.py` beside the existing `_kernel_record_review` assertions: + +```python +def test_the_review_record_carries_the_range(): + src = _run_item_source() # the file's existing helper that slices run_item's text + call = src[src.index("_kernel_record_review \"$BIRCHER_RUN_ID\""):] + call = call[:call.index("\n")] + for name in ('"$observed_head"', '"$_merge_base"', '"$_delta_digest"'): + assert name in call, f"{name} must reach the review record" +``` + +Run: `cd v2 && python -m pytest -q tests/execution/test_settled_pr_reaches_the_caller.py tests/execution/test_run_item_argument_wiring.py -v` +Expected: FAIL — the name count is 8 against `FIELDS == 10`, and the call lacks the range. + +- [ ] **Step 2: Widen the client.** In `_kernel_record_review`, extend the signature comment and locals: + +```bash +_kernel_record_review() { # [key] [terminal] [head_sha] [merge_base_sha] [delta_digest] + local run_id="$1" generation="$2" raw="$3" artifact="$4" base="$5" context="$6" + local key="${7:-}" terminal="${8:-}" head_sha="${9:-}" merge_base="${10:-}" digest="${11:-}" +``` + +Build the range fragment once, before the two `_kernel command` calls, and splice it into both payloads immediately after `\"context_bundle_hash\":\"$context\",`: + +```bash + # What was reviewed (spec §4): included only when the derivation pinned a + # head. Values are shas and a hex digest, so they need no JSON escaping. + local range="" + [ -n "$head_sha" ] && range="\"head_sha\":\"$head_sha\",\"merge_base_sha\":\"$merge_base\",\"delta_digest\":\"$digest\"," +``` + +so each payload reads `...\"context_bundle_hash\":\"$context\",${range}\"policy_version\":$_KERNEL_POLICY_VERSION}`. + +- [ ] **Step 3: Widen the runner.** In `_derived_width_ok` change `[ "$n" = 8 ]` to `[ "$n" = 10 ]` and its comment from eight to ten. In `run_item` change the read to: + +```bash + IFS='|' read -r outcome review note observed_head _obs_ci ci_first resubmissions _settled_pr _merge_base _delta_digest < /tmp/selftest.log 2>&1; echo rc=$?; grep -c FAIL /tmp/selftest.log` +Expected: pytest all PASS; self-test rc=0 and `0` FAIL lines. The self-test's fake derivations emit tuples in its fixtures (search the self-test for `|green|` and `|unknown||`); any fixture tuple that is still eight fields must be widened to ten by appending `||`. + +- [ ] **Step 5: Commit** + +```bash +git add batch/lib/kernel-client.sh batch/run-queue.sh v2/tests/execution +git commit -m "The runner records the reviewed range on the verdict" +``` + +--- + +### Task 8: The bash status post becomes an idempotent fallback that keeps the description + +**Files:** +- Modify: `batch/run-queue.sh` — `_post_cross_review_status` (~1173-1227), `_restamp_if_delta_unchanged` (~1339) +- Modify: the self-test's fake `gh` shims that answer `commits/*/status` (~7448, ~7595, ~7830) if they do not already return the stored state +- Modify: `docs/design/ARCHITECTURE.md` — the paragraph describing the cross-review status + +**Interfaces:** +- Produces: `_post_cross_review_status [sha] [state] [description]`; with `state` defaulting to `success` and `description` to the legacy text. A `success` already present on the sha returns 0 without posting. + +- [ ] **Step 1: Make the self-test fail first.** Add to the self-test, next to the `merge_ready_pr OK (incl. #10 cross-review status)` block, a case where the fake `gh` answers `commits//status` with a `success` for the context before any POST, and assert no POST happens: + +```bash + # Gate integrity: a success already on the head is not re-posted. The + # derivation posts on every verdict; the merge path's own post is a fallback. + : > "$slog"; STATUS_PRESENT=1 _post_cross_review_status demo 7 headsha >/dev/null 2>&1 + grep -q 'statuses/headsha' "$slog" \ + && { echo "FAIL _post_cross_review_status: re-posted a success that was already on the head"; exit 1; } + echo "_post_cross_review_status skips a present success OK" +``` + +and teach that block's fake `gh` to answer the read with `success` when `STATUS_PRESENT=1` (mirror the shape of the existing shim at ~7448: `printf '%s\n' "$@" | grep -q 'commits/.*/status' && [ "${STATUS_PRESENT:-0}" = 1 ] && { printf 'success\n'; exit 0; }`). + +Run: `bash batch/run-queue.sh --self-test > /tmp/selftest.log 2>&1; grep -n "re-posted" /tmp/selftest.log` +Expected: the new FAIL line appears. + +- [ ] **Step 2: Rewrite the function's head.** Replace the signature line and add the short-circuit and the two optional parameters: + +```bash +_post_cross_review_status() { # [sha] [state] [description] + local item="$1" pr="$2" sha="${3:-}" state="${4:-success}" description="${5:-cross-vendor review PASS (Bircher)}" attempt err +``` + +After the `[ -n "$sha" ] || { ... return 1; }` guard, insert: + +```bash + # ALREADY THERE -> nothing to do. The derivation posts a status for every + # verdict (spec §4), so by the time the merge path gets here a success + # usually exists with a description naming the reviewed range; re-posting + # would overwrite that description with the legacy text. + if [ "$state" = success ] && [ "$(_net_run "$(_cap_to "$BIRCHER_PREMERGE_TIMEOUT" "${PREMERGE_DEADLINE_AT:-}")" \ + gh api "repos/$REPO/commits/$sha/status" \ + -q '.statuses[] | select(.context=="bircher/cross-review") | .state' 2>/dev/null \ + | grep -cx 'success')" != 0 ]; then + echo "[batch:merge] $item: bircher/cross-review=success already on ${sha:0:7} -> not re-posting" >&2 + return 0 + fi +``` + +In the POST, replace `-f state=success` with `-f state=$state` and `-f description="cross-vendor review PASS (Bircher)"` with `-f description="$description"`; in the verification `grep -cx 'success'` becomes `grep -cx "$state"`; in the two log lines replace the literal `success` with `$state`. + +- [ ] **Step 3: The restamp keeps the reviewed description.** In `_restamp_if_delta_unchanged`, before `_post_cross_review_status "$item" "$pr" "$new" || return 1`, read the description off the reviewed head and carry it: + +```bash + local prior_desc + prior_desc=$(gh api "repos/$REPO/commits/$reviewed/status" \ + -q '.statuses[] | select(.context=="bircher/cross-review") | .description' 2>/dev/null | head -1) + _post_cross_review_status "$item" "$pr" "$new" success \ + "${prior_desc:-cross-vendor review PASS (Bircher)} (re-stamped on ${new:0:7}, delta unchanged)" || return 1 +``` + +- [ ] **Step 4: Run the self-test** + +Run: `bash batch/run-queue.sh --self-test > /tmp/selftest.log 2>&1; echo rc=$?; grep -n "FAIL\|skips a present success" /tmp/selftest.log` +Expected: rc=0, the new OK line, no FAIL. The `sweep-behind-same` fixture must still see a post on the new head (its fake `gh` stores nothing for the new sha, so the short-circuit does not fire). + +- [ ] **Step 5: Document.** In `docs/design/ARCHITECTURE.md`, find the sentence describing `bircher/cross-review` and replace it with: "`bircher/cross-review` is posted by the derivation for every verdict on the reviewed head: `success` for PASS, `failure` for FAIL with the blocking count, `error` when no verdict was produced, with ` round ... on ..` as the description; the merge path re-posts only when no success is present, and a re-stamp after a content-identical update-branch keeps the reviewed description." + +- [ ] **Step 6: Commit** + +```bash +git add batch/run-queue.sh docs/design/ARCHITECTURE.md +git commit -m "The runner's status post is a fallback that keeps the reviewed description" +``` + +--- + +### Task 9: Whole-suite gates, the PR, and #101's retirement + +**Files:** +- No code. Verification, the PR body file, and closing PR #101. + +- [ ] **Step 1: Run everything** + +Run: `cd v2 && python -m pytest -q` then `bash batch/run-queue.sh --self-test > /tmp/selftest.log 2>&1; echo rc=$?; tail -2 /tmp/selftest.log` +Expected: pytest all PASS; self-test rc=0. + +- [ ] **Step 2: Prove the two copies are gone** + +Run: `grep -rn "_recovery_review_prompt\|_pr_delta_digest\|byte_identical_to_the_bash" batch v2 docs/design | grep -v "docs/superpowers"` +Expected: no output. + +- [ ] **Step 3: Write the PR body to a file** at `$TMPDIR/gate-integrity-pr.md` (never interpolate it on the command line), no attribution lines: + +```markdown +## What + +Gate integrity, the first of the five PRs in `docs/superpowers/specs/2026-09-19-back-half-closed-loop-design.md` (§4). + +- One reviewer prompt, in the coordinator, naming the range `origin/...`; the bash copy and the byte-identity test are gone. +- The derivation posts `bircher/cross-review` for every verdict: `success` / `failure` (with the blocking count) / `error`, described as ` round on ..`, through the journalled effect path. +- The verdict fact records `head_sha`, `merge_base_sha` and `delta_digest` (schema version 2); the runner passes them through a ten-field derivation tuple. +- The delta digest has one implementation, in Python, used by the derivation and by the runner's re-stamp. +- The runner's own status post is now an idempotent fallback that keeps the reviewed description. + +## Why + +On muesli PR #769 a cross-review of one commit posted a green gate on a 38-file PR, and a later FAIL posted nothing, leaving that green standing. The record is `docs/design/front-half-live-log.md`. Supersedes #101. + +## Verification + +- `cd v2 && python -m pytest -q`: all green. +- `bash batch/run-queue.sh --self-test`: rc 0 as a non-root user (the runner's root-only self-test failure is pre-existing and unrelated). +- Acceptance lines from the spec satisfied here: every recorded verdict has a status whose state matches the verdict and whose description names the range; exactly one reviewer prompt exists and it names the range. +``` + +- [ ] **Step 4: Push and open the PR; close #101 as superseded** + +```bash +git push -u origin spec/back-half-closed-loop +gh pr create --repo abedegno/bircher --base main --head spec/back-half-closed-loop \ + --title "Gate integrity: one prompt, a status for every verdict, the range on the fact" \ + --body-file "$TMPDIR/gate-integrity-pr.md" +gh pr close 101 --repo abedegno/bircher --comment "Superseded by the gate-integrity PR, which carries the range sentence in the coordinator's prompt (the only copy now)." +``` + +- [ ] **Step 5: Cross-review.** Run the codex plugin review over the branch diff before asking for a merge; fix blocking findings on the branch and re-run until clear. + +--- + +## Self-review against the spec (§4) + +- *One reviewer prompt naming the range, base from the PR:* Task 1 (prompt, `base` parameter, `base_of` from `gh`), Task 5 (wired). +- *Every verdict posts a status; FAIL cannot leave an older success standing:* Task 4 (`_post_status` on PASS/FAIL/none), Task 8 (fallback does not overwrite). +- *Description names vendor, round, verdict, range:* Task 2 (`describe`), Task 5 (`round_number` from the journal). +- *The verdict fact records merge-base, head, digest; re-stamp stays provable by digest:* Tasks 3, 6, 7, 8. +- *Posting is a journalled effect:* Task 4 uses `d.effect("status_check", ...)`, which is `perform_effect` in live wiring; Task 8's bash path uses `_effect`. +- *#101 reworked, not merged as it stands:* Task 9. +- *Muesli `review-gate` unchanged:* no task touches it. +- Not in this PR by design: the container toolchain (spec's PR 5). + +Placeholder scan: every code step shows its code; the two "copy the existing helper" instructions in Tasks 6 and 7 name the exact test and helper to copy from. Type consistency: `Deps.base_of/merge_base/delta_digest/round_number` (Task 4) match `live_deps` (Task 5); `Derived.merge_base/delta_digest` and `FIELDS == 10` (Task 4) match the shell's ten names (Task 7); `_kernel_record_review`'s positional order (Task 7) matches its call site; `attest.CONTEXT/state_for/describe/blocking_count` (Task 2) are the names Task 4 imports. diff --git a/docs/superpowers/specs/2026-09-19-back-half-closed-loop-design.md b/docs/superpowers/specs/2026-09-19-back-half-closed-loop-design.md new file mode 100644 index 0000000..72eb8e3 --- /dev/null +++ b/docs/superpowers/specs/2026-09-19-back-half-closed-loop-design.md @@ -0,0 +1,149 @@ +# The back half closes its own loop + +**Status:** design, approved section by section in conversation on 2026-09-19; not yet implemented. + +**Supersedes, in part:** `2026-08-31-repair-loop-design.md` ("The coordinator repairs, bounded"). The bound goes; the ownership split it settled stands. + +## Why + +Muesli issue #764 was implemented by a run whose session stopped twelve minutes before CI finished red. The batch layer derived a terminal outcome from the red CI, recorded the run as `ended`, and from that moment the pipeline was blind to the pull request. Every later step, six in all, was a person launching `--recover-pr` by hand: a repair, a cross-review that read one commit of thirty-eight and passed, a prompt fix, four review rounds each finding one real defect in a different subsystem, and the merge. The record is `docs/design/front-half-live-log.md`, from "Overnight: #764 to a red PR" onward. + +Three properties of the current back half produced that week, each verified in the code: + +1. **A run can end while its PR is open.** `record_run_outcome` is legal from every active state and is called unconditionally at the end of every `run_item` pass (`batch/run-queue.sh` ~5212). `ended` is the one state no command may leave (`v2/kernel/authz.py`, `_TRANSITIONS`). Waves resume only open runs (`batch/lib/kernel-client.sh` `_kernel_find_run open` excludes `ended`), so the next wave that meets the item mints a fresh run rather than look at the stranded PR. +2. **Red CI is an outcome, not a repair trigger.** `classify` in `v2/coordinator/observe.py` returns `failed` on `ci == "red"` before any verdict is consulted. The repair loop engages only on a review FAIL with rounds remaining, and only inside the pass that dispatched the implementer. +3. **The gate can attest to nothing.** `_post_cross_review_status` (`run-queue.sh` ~1173) posts only `success`; a FAIL posts nothing, so an older green stands on a head whose real review failed. The reviewer prompt pins a commit but never names a range, and one reviewer read that as "the head commit's diff". The prompt exists twice, in bash and Python, held identical by a byte-comparison test; the range fix on branch `fix/recovery-review-range` touched only the bash copy. + +Two more gaps cost rounds rather than autonomy: nothing checks the accepted plan's named deliverables against the PR (the implementer disclosed two dropped files in the PR body and the fourth review found them), and neither the implementer's nor the reviewer's container can run the database-backed or Node test suites, so both ship or judge on CI logs. + +The target this design serves, agreed with the user: **E11**, one real muesli issue from queued to merged PR with no human action beyond the spec and plan gates, every cross-review status on the PR attesting to a named range, and the round log written by the pipeline. + +## What exists and is reused + +- The kernel's transition table and its self-test that every command declares its states (`v2/kernel/authz.py`). +- Parks as facts orthogonal to state (`EventKind.PARKED`), with reasons in `PARK_REASONS`, the park notice on the issue, and the human replies `retry`, `correct`, `stop` read from the carrier session (front-half design §3). +- `record_implementation_output`, `record_ci_observation`, `record_review` with `_BACK_HALF_DESTINATIONS`, `request_merge`, `record_merge_outcome`, `cancel_run`, `resume_run`. +- `derive` and `classify` in `v2/coordinator` (ground truth from the repository), the review dispatch in `v2/coordinator/review.py`, `_repair_round` and `merge_ready_pr` in bash, and the effect lifecycle `effect_intended → effect_confirmed | effect_uncertain → effect_reconciled`. +- The journal sweep in `batch/issues-to-queue.sh` that re-queues parked, sliced and disputed runs each wave. +- `_pr_delta_digest`, the sha256 of the PR's three-dot compare, used today only to prove an `update-branch` changed nothing. + +## Scope + +In: the kernel legality changes below; a pure step function in the coordinator; the batch loop replaced by one-step passes; the no-progress park; status posting on every verdict with a named range; one reviewer prompt; orphan adoption; state-derived labels; a pipeline-owned round log; the plan-conformance check; test infrastructure for the agent containers. + +Out: the front half and the shaping phase, which do not change; the implementer's own prompt; any change to the muesli `review-gate` workflow, which keeps requiring `success` on the head; the choice of reviewing vendor, which keeps rotating. + +## 1. The state machine + +No new states. A repair round is an implementation round: the back half is `planned → implementing → reviewing → merge_requested → merged`, and a repair re-enters it at `planned` through one door, described next. + +**`request_repair`**, a new command, legal from `implementing` and `reviewing`, destination `planned`. Its payload names the cause and the evidence: + +| cause | evidence | raised by | +|---|---|---| +| `ci_red` | the observed head and the set of failing job names | the step function, from a red CI observation on the current head | +| `review_fail` | the reviewed head and the verdict's blocking-finding fingerprints | the step function, from a recorded FAIL | +| `plan_conformance` | the observed head and the plan-named paths absent from the PR | the step function, once, after implementation output is first recorded | + +It is refused from `planned` (there is nothing to repair yet) and from every front-half state. It is the only door back to `planned` in the back half: `record_review` no longer moves a back-half run on `request_revision` or `reject` (both now stay at `reviewing`, like `accept`), so a FAIL is a fact the step function answers with `request_repair(review_fail)`, and the red-CI short circuit in `classify` goes for the same reason: a red observation is ground truth the step function turns into a `ci_red` repair, never into an outcome. From `planned`, the repair implementer's dispatch is the existing `start_implementation`. + +The PR's own state, `open`, `closed` or `merged`, is recorded as an external observation beside every CI observation, so the kernel can read it when it decides whether a run may end. + +**`record_run_outcome`** is legal from `merged` and `cancelled` as today, and from an active state only when the run holds no `implementation_output` fact, or when its newest PR observation says the PR is closed without merging. Once a PR exists and is open, the only exits are a merge or a human `stop`. This is E11's property as a refusal. + +**`park`** becomes legal from `planned`, `implementing` and `reviewing`. `PARK_REASONS` gains `no_progress` (§3). `no_verdict` and `budget_exhausted` apply in the back half with their existing meanings. **`grant_round`** and **`cancel_run`** widen to the same three states so the replies work: `retry` records a grant, `stop` cancels, `correct` is a person pushing to the branch and then replying `retry`. + +The back-half revision bound (`BIRCHER_MAX_REVISIONS`, `_max_revisions`, `revisions_left`) is removed. Rounds are counted from the journal for the scorecard and the log, and nothing refuses a round on count. + +## 2. The driver: one step per pass + +The loop is the wave timer. Each pass performs, for each open back-half run, idempotent steps until none is ready, and returns. + +**The step function** is pure Python in the coordinator, `next_step(journal, ground) -> Step`, beside `derive` and `classify`. `ground` is what `derive` already reconstructs from the repository: PR state and head, CI on that head as `pending | green | red` with failing job names, and the newest review verdict for that head as `PASS | FAIL | none` with its findings. It returns exactly one of: + +| step | when | +|---|---| +| `wait` | CI pending on the head; or the round's implementer session is still alive, read from the session as today | +| `repair(cause, evidence)` | CI red, or a recorded FAIL, or a plan-conformance gap, and no repair is yet recorded for this head | +| `review` | CI green and no verdict recorded for this head | +| `merge` | a PASS recorded for this head | +| `park(reason)` | §3's conditions | +| `done` | the run is `merged` or `cancelled`; the runner records the run outcome and the scorecard row | + +**The runner performs the step.** It calls the step function, does the one thing it names (dispatch a repair session, dispatch a review, request and perform the merge, park), records the resulting fact through the kernel, and re-derives. Dispatching stays in bash: the repair-loop design settled that session creation is bash-only and the coordinator judges, and that split stands. The `while :` loop in `run_item` (~4944 to 5123) and its per-pass allowance variables go; `observe_outcome`'s role becomes "derive ground truth", nothing more. + +**Waves resume every open back-half run.** The journal sweep in `issues-to-queue.sh` also re-queues any open run in `planned`, `implementing` or `reviewing` that holds implementation output. A session death, a crash, or a vendor outage means the next wave resumes from the journal. + +**A repair round** is a fresh implementer session on the PR's existing branch, briefed with the cause and its evidence and told to push to that branch, exactly as `_repair_round` does today; its turn ends as turns end. + +## 3. Unbounded rounds, the no-progress park + +**Progress** is judged from facts, each round against the round before it: + +- a `ci_red` repair made progress if the next CI observation on a new head is green, or red with a different set of failing job names; +- a `review_fail` repair made progress if the next verdict for a new head is PASS, or FAIL with a different set of blocking-finding fingerprints; +- a `plan_conformance` repair made progress if the next conformance check names fewer paths. + +A **finding fingerprint** is the sha1 of the finding's first file path, lower-cased, joined by one space to its first sentence with whitespace collapsed. The coordinator computes the fingerprints when it records a verdict and stores them on the `review_verdict` fact; the reviewer prompt already asks for findings as a list with a file reference. + +**`no_progress`** is raised when two consecutive repair rounds each fail to make progress against their own predecessor: the same failing jobs three rounds running, the same fingerprints three rounds running, or the same missing paths three rounds running. The first repair round has no predecessor and always counts as progress. One repeat is tolerated because CI can be flaky and a reviewer occasionally re-raises a finding it should have closed; a second repeat is going in circles. + +**`no_verdict`** parks at once when the reviewer produced no verdict for the head. **`budget_exhausted`** parks when the usage gate refuses a dispatch, with the reset time in the notice. The usage ceiling refuses dispatches regardless of anything above. + +**A park from outside:** the issue receives the park notice the front half writes, naming the reason, the round, and the three replies; the PR receives the same as a comment; the label becomes `bircher:parked`; nothing on the PR is touched until a reply. `retry` resets the progress comparison window, so the next round is judged against itself. + +## 4. Gate integrity + +This section lands first, as its own PR, because nothing in §1 to §3 is safe on a gate that can lie. + +**One reviewer prompt.** The Python prompt in `v2/coordinator/review.py` is the only one; `_recovery_review_prompt` in bash and the byte-identity test go. The prompt names the range in one sentence: the change under review is every file in `git diff origin/...`, where `` is the PR's base branch, and a head commit touching one file does not narrow the review. Branch `fix/recovery-review-range` (bircher PR #101) is reworked to this shape and is not merged as it stands. + +**Every verdict posts a status** on the reviewed head, context `bircher/cross-review`: `success` for PASS, `failure` for FAIL with the count of blocking findings, `error` when no verdict was produced. The description is ` round on ..`. A FAIL can no longer leave an older `success` standing; the muesli `review-gate` check keeps requiring `success` on the head and does not change. + +**The verdict fact records the range.** `review_verdict` carries the merge-base sha, the head sha, and the three-dot delta digest. A re-stamp after a content-identical `update-branch` stays provable by digest; anything else gets a fresh review. + +**Posting is an authorised effect** through the intended, confirmed, uncertain, reconciled lifecycle every other GitHub effect uses. An uncertain post halts for reconciliation rather than being retried blindly. + +## 5. Plan conformance, orphans, labels, the round log + +**Plan conformance** runs once, when implementation output is first recorded and before any review is dispatched. The coordinator extracts from the accepted plan every path on a `**Files:**` line under `create` and `modify`, reads the PR's changed files, and raises `request_repair(plan_conformance)` naming every `create` path absent from the PR and every `modify` path unchanged in it. No vendor is spent. An implementer that believes a named path should not exist lists it under a `## Plan deviations` heading in the PR body with one line of reason; a listed path is exempt and the exemption is written to the round log, so the check cannot be bypassed quietly. + +**Orphan adoption.** Each wave lists open PRs whose head branch carries an item code and which have no open run. For each, it starts a run directly at `implementing` (`run_started` already carries its opening state) with `base_sha` set to the merge-base, records `implementation_output` (PR number and head), and the step loop takes it from there. A legacy run left `ended` with an open PR is adopted this way exactly once; the old journal is not edited. + +**`--recover-pr` is retired.** It remains for one release as an alias that adopts one PR and performs one step, then goes. + +**Labels follow the journal.** Each pass sets the issue's label from the run's state: `bircher:running` while the run is open and stepping, `bircher:parked` on a park, `bircher:escalated` only after `stop`, none while nothing is open. The stale `bircher:running` on #764 came from a writeback that ran once per pass; a state-derived label cannot go stale. + +**The round log.** The pipeline owns one comment per PR, marked ``, and appends one line per step: the round, the action, the range, and the verdict or the failing jobs. The by-hand E10 log is what that comment should have contained. + +## 6. Testing, proof, and order of work + +**Kernel, from the refusal side,** against a real journal: `record_run_outcome` on a run with implementation output and an open PR is refused; `request_repair` from `planned` is refused; `park` with `no_progress` from `queued` is refused; a back-half `grant_round` and `cancel_run` are accepted; the every-command-declares-its-states self-test covers the additions. + +**The step function by replay.** #764's journal at the moment its session died with red CI: expected `repair(ci_red)`, not an ending. #769's six verdicts: expected five repairs and a merge, no park, because every round changed the fingerprints. Two identical FAIL fingerprint sets: expected `park(no_progress)`. Each replay is a fixture file checked in with the test. + +**Fault injection where the last design found its bugs:** session death between a recorded verdict and its repair dispatch; a status post that returns uncertain; a wave that finds the same run already stepping. Each against a live database, as the lifecycle tests are today. + +**E11.** First on bircher-smoke, with an issue whose implementation is arranged to fail CI once and a review once, so both repair causes fire before the merge; a second smoke issue is arranged to repeat a finding until it parks, and is resumed with `retry`. Then one real muesli issue, hands off after its plan gate. E11 passes only if no park occurred on the muesli issue; a park there is a person in the loop, and the proof fails. The proof is the pipeline's own round comment, checked line by line against the journal. + +**Order of PRs:** + +1. Gate integrity (§4), with #101 folded in. +2. The kernel changes and the step function (§1 to §3), with the batch loop replaced behind them. +3. Adoption, labels and the round log (§5, without conformance). +4. Plan conformance. +5. Postgres and the Node toolchain in the review and repair containers. + +Each PR goes through the pipeline's own cross-review; the codex plugin reviews what the pipeline cannot review about itself. The implementation plan is written per PR, in this order, each plan owning the acceptance lines below that its PR satisfies. + +## Acceptance + +- A run whose PR is open cannot be recorded `ended`; the kernel refuses it and the test proves the refusal. +- A red CI observation on an open PR produces a `ci_red` repair, replayed from #764's journal. +- Six alternating verdicts produce five repairs and a merge with no park, replayed from #769's journal. +- Two identical FAIL fingerprint sets produce `park(no_progress)`; a `retry` reply resumes. +- Every recorded verdict has a `bircher/cross-review` status on its head whose state matches the verdict and whose description names the range. +- Exactly one reviewer prompt exists in the repository and it names the range. +- An open PR on an item-code branch with no open run is adopted within one wave. +- A PR missing a plan-named `create` path without a listed deviation receives a `plan_conformance` repair before any review. +- E11 passes on bircher-smoke and then on one muesli issue, with the round log matching the journal. diff --git a/skills/cross-review/SKILL.md b/skills/cross-review/SKILL.md index 75186f3..fdbd94a 100644 --- a/skills/cross-review/SKILL.md +++ b/skills/cross-review/SKILL.md @@ -33,8 +33,13 @@ anyone needs to read through. the raw vendor name. The reviewer reads the REAL code (not a diff excerpt): its `input` instructs it to `export PATH=/root/bin:$PATH`, then - `git fetch origin pull//head && git worktree add --detach /tmp/review-- FETCH_HEAD`, - `cd /tmp/review--`, read the changed files AND their surrounding context, + `git fetch origin pull//head && git fetch origin :refs/remotes/origin/ +&& git worktree add --detach /tmp/review-- FETCH_HEAD`, + `cd /tmp/review--`, review the WHOLE pull request as it stands + at that commit — every file in `git diff origin/...FETCH_HEAD`, where + `` is the PR's base branch, NOT only the head commit's own diff (a head + commit that touches one file does not narrow the review to one file) — + read the changed files AND their surrounding context, run the gates it can — prefixing EACH gate command with `export PATH=/root/bin:$PATH &&` in the same shell call, since the reviewer's shell may not persist env between calls (`export PATH=/root/bin:$PATH && go @@ -58,8 +63,13 @@ build ./...`, `... && go vet ./...`, client `... && npm run typecheck` / `... `sys_session_send(agent="claude_code"|"codex", title="review-", args={purpose: "review", input: "Review PR # against this contract: . First: export PATH=/root/bin:$PATH; git fetch origin -pull//head; git worktree add --detach /tmp/review-- FETCH_HEAD; cd -/tmp/review--. READ the changed files and enough surrounding code to +pull//head; git fetch origin :refs/remotes/origin/; git worktree +add --detach /tmp/review-- FETCH_HEAD; cd +/tmp/review--. The change under review is the WHOLE pull request +as it stands at that commit: every file in `git diff origin/...FETCH_HEAD` +(from the PR's merge-base with up to the checked-out head), NOT only the +head commit's own diff. A head commit that touches one file does not narrow the +review to one file. READ the changed files and enough surrounding code to verify each contract point — do NOT judge from the diff alone. Run the gates you can, each as ONE command prefixed with `export PATH=/root/bin:$PATH &&` (e.g. `export PATH=/root/bin:$PATH && go build ./...`) since your shell may not diff --git a/v2/coordinator/attest.py b/v2/coordinator/attest.py new file mode 100644 index 0000000..5001fe5 --- /dev/null +++ b/v2/coordinator/attest.py @@ -0,0 +1,116 @@ +"""What a cross-review status attests to (spec §4, gate integrity). + +Pure. The description names the vendor, the round, the verdict and the +merge-base..head range, so a person reading the PR's checks can see exactly +what was reviewed; the state follows the verdict, and silence is an error, +never a pass. +""" + +from __future__ import annotations + +import re + +#: The context muesli's `review-gate` workflow requires `success` on. +CONTEXT = "bircher/cross-review" + +#: GitHub truncates longer descriptions; truncate deliberately instead. +_MAX_DESCRIPTION = 140 + + +def _heading(phrase: str) -> str: + """`Phrase` or `PHRASE`: a heading is capitalised, prose is not.""" + return f"(?:{phrase}|{phrase.upper()})" + + +#: A heading starts its line, or is glued to the end of the sentence before +#: it (`...conclusions.Blocking findings`, as the live reviewers write), and +#: may carry Markdown heading and bold markers (`## `, `### **`) in front. +#: Case-sensitive on purpose: "found no blocking findings issues" is prose +#: and opens no section; "the final verdict logic" is prose and closes none. +_LEAD = r"(?:^|[.!?:]\s*)[#*\s]*" +#: After the phrase: a word boundary, then the heading's own punctuation -- +#: bold markers, a parenthetical count (`Blocking findings (2)`, which is a +#: COUNT and not a finding), a colon. Group 1 is that colon when there was +#: one; group 2 is the rest of the line. +_BLOCKING = re.compile( + _LEAD + _heading("Blocking findings") + r"\b\**\s*(?:\(\d+\))?\s*(:)?\s*\**\s*(.*)$") +#: A closing heading STARTS its line (after heading and bold markers) and may +#: carry any trailer at all: `Non-blocking findings -- none worth naming` and +#: `## Suggestions` both close the blocking section. The sentence-punctuation +#: lead is deliberately NOT used here -- a bullet reading +#: `- : Verification of the lease is missing` is a finding, not a heading, and +#: closing on it would silently drop every finding under it. +_OTHER_SECTION = re.compile( + r"^[#*\s]*(?:" + "|".join(_heading(p) for p in + ("Non-blocking findings", "Suggestions", "Verification", "Verdict")) + + r")\b") +#: A list item at column zero: `- `, `* `, `• `, `1. `, `1) `. An indented +#: item elaborates the finding above it and is not another finding. +_ITEM = re.compile(r"^(?:[-*•]|\d+[.)])\s+\S") +_NONE = re.compile(r"^(none|n/a|no blocking findings)\.?\**$", re.I) + + +def blocking_count(text: str) -> int: + """How many items the reviewer listed under its blocking-findings heading. + + Counts list items between the heading and the next section heading, at + column zero (a nested sub-bullet elaborates a finding, it is not another + one). A heading followed on the same line by `None` counts nothing; a + heading followed by a COLON and a finding on the same line counts one; a + parenthetical count (`Blocking findings (2)`) is the section's own count + and is never a finding. Text with no heading counts nothing, so an + unstructured FAIL posts `0 blocking` rather than a guess. + + A heading is recognised at the start of a line -- Markdown heading and + bold markers ahead of it are allowed (`## Blocking findings`, + `### **Blocking findings**`) -- or glued to the end of the sentence before + it, in heading case only; a prose mention of the phrase mid-sentence opens + nothing and closes nothing. The phrase CONTINUING as prose with no colon + (`Blocking findings were all resolved.`) is such a mention: it opens no + section, because treating it as a heading both counts the sentence as a + finding and makes every list below it blocking. Items count at column + zero; an indented item elaborates the finding above it. + """ + count, inside = 0, False + for line in (text or "").splitlines(): + m = _BLOCKING.search(line) + if m: + colon, rest = m.group(1), m.group(2).strip() + # Prose continuing off the phrase with no colon is a mention of + # the section, not the section (`Blocking findings were all + # resolved.`). A heading ends the line, or introduces its findings + # with a colon. + if rest and not colon: + continue + inside = True + if rest and not _NONE.match(rest): + count += 1 + continue + if inside and _OTHER_SECTION.search(line): + break + if inside and _ITEM.match(line): + count += 1 + return count + + +def state_for(verdict: str | None) -> str: + """`success` for PASS, `failure` for FAIL, `error` for anything else. + + BLOCKED, an empty verdict and a reviewer that never produced one all + land on `error`: none of them is an approval, and the gate requires + `success` by name. + """ + return {"PASS": "success", "FAIL": "failure"}.get(verdict or "", "error") + + +def describe(vendor: str, round_n: int, verdict: str | None, blocking: int, + merge_base: str, head: str) -> str: + """` round on ..`.""" + if verdict == "PASS": + word = "PASS" + elif verdict == "FAIL": + word = f"FAIL ({blocking} blocking)" + else: + word = "no verdict" + rng = f"{(merge_base or '')[:7] or '?'}..{(head or '')[:7] or '?'}" + return f"{vendor} round {round_n} {word} on {rng}"[:_MAX_DESCRIPTION] diff --git a/v2/coordinator/cli.py b/v2/coordinator/cli.py index 3b5bfa7..f746b41 100644 --- a/v2/coordinator/cli.py +++ b/v2/coordinator/cli.py @@ -77,6 +77,21 @@ def _maybe_stdin(value: str) -> str: return sys.stdin.read() +def _round_number(db: str, run_id: str) -> int: + """Which review round the derivation is about to record: revisions used + so far, from the journal, plus one. No journal, no run, or an unreadable + database means round one -- the description must never block a status. + """ + if not db or not run_id or not os.path.exists(db): + return 1 + try: + from kernel.store import Store + from coordinator.observe import revisions_used + return revisions_used(Store.open(db).facts_for(run_id)) + 1 + except Exception: # noqa: BLE001 + return 1 + + def main(argv=None) -> int: p = argparse.ArgumentParser(prog="bircher-coordinator") subs = p.add_subparsers(dest="mode", required=True) @@ -123,6 +138,11 @@ def main(argv=None) -> int: ef.add_argument("--timeout", type=float, default=None) ef.add_argument("cmd", nargs=argparse.REMAINDER) + dl = subs.add_parser("delta") + dl.add_argument("--repo", required=True) + dl.add_argument("--base", required=True) + dl.add_argument("--ref", required=True) + dv = subs.add_parser("derive") dv.add_argument("--item", required=True) dv.add_argument("--code", default="") @@ -472,6 +492,16 @@ def main(argv=None) -> int: return RC_EFFECT_DENIED return RC_OK + if a.mode == "delta": + from coordinator.delta import delta_digest + # `_gh` reads the repo from here rather than from an unexported global. + os.environ["BIRCHER_GH_REPO"] = a.repo + digest = delta_digest(a.repo, a.base, a.ref) + if not digest: + return RC_FAILED + print(digest, end="") + return RC_OK + if a.mode == "derive": # Imported here so the rest of the CLI stays usable when the world is # not reachable -- `wiring` builds real gh and effect callables. @@ -498,7 +528,10 @@ def main(argv=None) -> int: server=a.server, bundle_dir=a.bundle_dir, poll_interval=a.poll_interval, ci_wait=a.ci_wait, rerun_wait=a.rerun_wait, - revisions_left=a.revisions_left), + revisions_left=a.revisions_left, + round_number=_round_number( + os.environ.get("BIRCHER_KERNEL_DB", ""), + os.environ.get("BIRCHER_RUN_ID", ""))), rerun_max=a.rerun_max) # Written BEFORE the tuple is printed, and ATOMICALLY: the caller reads # the tuple, sees `revise`, and then reads this file. Printing first diff --git a/v2/coordinator/delta.py b/v2/coordinator/delta.py new file mode 100644 index 0000000..d2323ca --- /dev/null +++ b/v2/coordinator/delta.py @@ -0,0 +1,87 @@ +"""The PR's own three-dot delta, digested (gate integrity, spec §4). + +Ported from `_pr_delta_digest` in `batch/run-queue.sh`, which is gone: one +implementation, used by the derivation (to record what was reviewed on the +verdict fact) and by the runner's re-stamp after an update-branch (through +`coordinator.cli delta`). + +GitHub's compare is three-dot, so `base...ref` is the change the PR +contributes and never the base branch's own commits. That property is what +makes the digest comparable across an update-branch: that operation MERGES +the base into the head (it does not rebase), so the new head CONTAINS the new +base, the merge-base IS the new base, and the compare still yields only the +PR's work. + +An unprovable delta is `None`, never "assume equal": GitHub caps the +changed-file list at 300, omits `patch` for binaries, pure renames and +anything over its size limit, and truncates a large tree. Digesting a partial +answer would produce a confident wrong one, which here means merging code no +reviewer read. +""" + +from __future__ import annotations + +import hashlib +import json + +from coordinator.ci import GhError, _gh + +#: GitHub's documented cap on a compare's file list; at exactly 300 it may be truncated. +_FILE_CAP = 300 + + +def canonical_delta(compare: dict, tree: dict) -> str | None: + """The canonical text of the delta, or None when it cannot be established.""" + files = (compare or {}).get("files") + if not isinstance(files, list): + return None + if not (1 <= len(files) < _FILE_CAP): + return None + if any("patch" not in f for f in files): + return None + # Require an explicit false: an absent or null `truncated` is an answer we + # did not get, not a reassuring one. + if (tree or {}).get("truncated") is not False: + return None + entries = {e.get("path"): f"{e.get('mode')}|{e.get('type')}" for e in (tree.get("tree") or [])} + rows = [] + for f in sorted(files, key=lambda f: f["filename"]): + rows.append({ + "filename": f["filename"], + "previous_filename": f.get("previous_filename"), + "status": f.get("status"), + "sha": f.get("sha"), + "patch": f.get("patch"), + "entry": entries.get(f["filename"], "ABSENT"), + }) + # Sorted keys, no whitespace, unicode kept raw: the same bytes `jq -cS` produced. + return json.dumps(rows, sort_keys=True, separators=(",", ":"), ensure_ascii=False) + + +def delta_digest(repo: str, base: str, ref: str, gh=_gh) -> str: + """sha256 of the canonical delta of `base...ref`, or "" when unprovable. + + Hashes the canonical text WITH a trailing newline appended, not the bare + text `canonical_delta` returns. The retired bash `_pr_delta_digest` piped + `jq -cS`'s stdout -- which jq always newline-terminates -- straight into + `sha256sum`, so that byte was part of every digest it ever produced. + Dropping it here would still be internally consistent (every digest this + module computes agrees with every other one), but it would silently + diverge from what the retired implementation actually hashed for + identical delta content, which is exactly the property + `test_the_port_reproduces_the_bash_digest_for_the_fixture` checks against + a bash-produced oracle value. + """ + try: + compare = json.loads(gh(["api", f"repos/{repo}/compare/{base}...{ref}"]) or "null") + tree = json.loads(gh(["api", f"repos/{repo}/git/trees/{ref}?recursive=1"]) or "null") + # OSError too: `_gh` execs a binary, and a runner with no `gh` on PATH + # raises FileNotFoundError, not GhError. An unprovable delta is the + # answer here in every case -- a traceback out of this function aborts + # a derivation that is otherwise complete and correct. + except (GhError, ValueError, TypeError, RuntimeError, OSError): + return "" + text = canonical_delta(compare, tree) + if text is None: + return "" + return hashlib.sha256((text + "\n").encode("utf-8")).hexdigest() diff --git a/v2/coordinator/outcome.py b/v2/coordinator/outcome.py index 4f80d4e..92f4359 100644 --- a/v2/coordinator/outcome.py +++ b/v2/coordinator/outcome.py @@ -15,6 +15,7 @@ import re from dataclasses import dataclass, field +from coordinator import attest from coordinator.ci import DEFAULT_IGNORED as _DEFAULT_IGNORED from coordinator.observe import classify @@ -68,6 +69,14 @@ class Deps: #: against the wrong target. Second instance of this shape; `publish_cmd` #: was the first. repo: str = "" + #: The PR's base branch, for the reviewer's range and the merge-base. + base_of: callable = lambda pr: "main" + #: (base, head) -> merge-base sha, or "" when unknown. + merge_base: callable = lambda base, head: "" + #: (base, head) -> the three-dot delta digest, or "" when unprovable. + delta_digest: callable = lambda base, head: "" + #: Which review round this is, from the journal (revisions used + 1). + round_number: int = 1 @dataclass(frozen=True) @@ -94,24 +103,31 @@ class Derived: #: LAST in the field order deliberately: `pr` is passed positionally by #: `derive`, and inserting anything before it silently rebinds arguments. findings: str = "" + #: What the review covered: the merge-base with the PR's base, and the + #: digest of the PR's own delta at the reviewed head. Both ride out on + #: every derivation that pinned a head, so the runner can record them on + #: the verdict fact (spec §4). + merge_base: str = "" + delta_digest: str = "" def as_tuple(self): return (self.outcome, self.review, self.note, self.sha, self.ci, - self.ci_first, self.resubmissions, self.pr) + self.ci_first, self.resubmissions, self.pr, self.merge_base, + self.delta_digest) #: Field count, so a consumer can assert it rather than assume it. The #: absorption hazard below is silent, and silence is what made the missing #: `pr` field survive a live run that looked like it worked. - FIELDS = 8 + FIELDS = 10 def as_line(self) -> str: - """The EIGHT-field form the shell parses. A caller reading seven + """The TEN-field form the shell parses. A caller reading fewer absorbs the last into its neighbour, silently -- `read -r a b c` puts every remaining field into `c`, so a short reader does not error, it corrupts one value.""" r = "" if self.resubmissions is None else self.resubmissions return (f"{self.outcome}|{self.review}|{self.note}|{self.sha}|" - f"{self.ci}|{self.ci_first}|{r}|{self.pr}") + f"{self.ci}|{self.ci_first}|{r}|{self.pr}|{self.merge_base}|{self.delta_digest}") def _settle_pr(item, code, pr, issue, d: Deps) -> str: @@ -166,9 +182,48 @@ def _settle_ci(item, pr, d: Deps, rerun_max: int) -> str: return ci +def _post_status(item, pr, head, verdict, reviewer_out, merge_base, d: Deps) -> None: + """Every verdict posts `bircher/cross-review` on the reviewed head: success, + failure, or error, with the range in the description (spec §4). A FAIL + can no longer leave an older success standing. + + BEST EFFORT, as the PR comment is: the outcome is derived from the + repository and is already correct; a transient GitHub failure here must + not turn a derivation into an escalation. The runner's own idempotent + post before the merge is the fallback for a missing success. + """ + blocking = attest.blocking_count(reviewer_out) if verdict == "FAIL" else 0 + state = attest.state_for(verdict) + desc = attest.describe(d.reviewer, d.round_number, verdict, blocking, merge_base, head) + # CONTENT-ADDRESSED, exactly as the PR comment's key below is, and for the + # same reason. `status::` did not change when the VERDICT + # changed: the round only advances on a recorded `review_verdict` fact, and + # an `error` (no verdict) or a terminal FAIL records none -- so a later + # derivation of the SAME head posting a different state replayed the same + # key, `kernel/effects.py` returned the prior external id WITHOUT executing, + # and the new state was never posted. A gate left showing a superseded + # verdict is the exact failure this section exists to make impossible. + stamp = hashlib.sha256(f"{state}\n{desc}".encode()).hexdigest()[:16] + key = f"status:{head}:{stamp}" + try: + d.effect("status_check", key, + ["gh", "api", f"repos/{d.repo}/statuses/{head}", "-X", "POST", + "-f", f"state={state}", "-f", f"context={attest.CONTEXT}", + "-f", f"description={desc}"]) + # "recorded", not "posted": `d.effect` is idempotent, so a replayed key + # returns the prior external id without a network call. Claiming a post + # happened on that path is a log line that cannot be read as evidence. + d.log(f"{item}: recorded status effect {attest.CONTEXT}={state} " + f"on {head[:7]} ({desc})") + except Exception as exc: # noqa: BLE001 + d.log(f"{item}: failed to post {attest.CONTEXT}={state} on PR #{pr} " + f"({type(exc).__name__}: {exc}) -> continuing; the outcome is " + f"derived from the repository and is unaffected") + + def derive(item: str, code: str, pr: str, issue: str, *, deps: Deps, rerun_max: int = 4) -> Derived: - """The whole derivation. Returns the eight fields.""" + """The whole derivation. Returns the ten fields.""" d = deps pr = _settle_pr(item, code, pr, issue, d) @@ -179,6 +234,7 @@ def derive(item: str, code: str, pr: str, issue: str, *, deps: Deps, ci_first, resubmissions = d.history(branch) ci, verdict, reviewed_sha, reviewer_out = "na", None, "", "" + merge_base, digest = "", "" if pr: ci = _settle_ci(item, pr, d, rerun_max) @@ -196,6 +252,25 @@ def derive(item: str, code: str, pr: str, issue: str, *, deps: Deps, verdict, reviewer_out = d.review(pr, reviewed_sha) if verdict == "NONE": verdict = None + if reviewed_sha: + # WHAT WAS REVIEWED, captured against the same pinned head the + # reviewer was told to check out (spec §4): the merge-base with + # the PR's base and the digest of the PR's own delta. + base = d.base_of(pr) or "main" + # VALIDATED LIKE THE HEAD IS. `merge_base` is whatever the + # lookup printed -- `null`, an error line, anything -- and it + # is spliced into the pipe-delimited tuple, into the status + # description and, through the runner, into hand-built JSON. + # Anything that is not a 40-hex sha is "unknown" (`""`), which + # the description already renders as `?`. + raw_base = (d.merge_base(base, reviewed_sha) or "").strip() + merge_base = raw_base if _FULL_SHA.match(raw_base) else "" + if raw_base and not merge_base: + d.log(f"{item}: merge-base for PR #{pr} is not a 40-hex sha " + f"({raw_base!r}) -> recording it as unknown") + digest = d.delta_digest(base, reviewed_sha) or "" + _post_status(item, pr, reviewed_sha, verdict, reviewer_out, + merge_base, d) o = classify(pr or None, ci, verdict, reviewer=d.reviewer, revisions_left=d.revisions_left) @@ -263,4 +338,6 @@ def derive(item: str, code: str, pr: str, issue: str, *, deps: Deps, # multi-paragraph review. return Derived(o.outcome, o.review, o.note, sha_out, o.ci, ci_first, resubmissions, str(pr or ""), - findings=(reviewer_out if o.outcome == "revise" else "")) + findings=(reviewer_out if o.outcome == "revise" else ""), + merge_base=merge_base if reviewed_sha else "", + delta_digest=digest if reviewed_sha else "") diff --git a/v2/coordinator/review.py b/v2/coordinator/review.py index 7827b6f..1bbf13c 100644 --- a/v2/coordinator/review.py +++ b/v2/coordinator/review.py @@ -74,16 +74,13 @@ def extract_verdict(text: str, hash8: str | None = None) -> str | None: return None -#: The review prompt, ported VERBATIM from `_recovery_review_prompt`. -#: -#: Prose, not logic -- but prose carrying its own scars (muesli #705's CI gate -#: that reported success while tests failed; #666's microphone left recording; -#: #66's moving `pull/N/head` ref). A paraphrase would drop one silently, so -#: `test_the_rendered_prompt_is_byte_identical_to_the_bash` renders both and -#: compares. +#: The only copy: the bash `_recovery_review_prompt` it was ported from is +#: gone (gate integrity, spec §4), so nothing keeps two prompts in step +#: because there is one. _PROMPT = r"""Review PR #{pr} in {repo} as an INDEPENDENT, READ-ONLY reviewer. Do NOT edit, commit, or open/update any PR. -First: export PATH=/root/bin:$PATH; git fetch origin pull/{pr}/head; git worktree remove --force /tmp/review-{pr}-{nonce}-oob 2>/dev/null; git worktree prune; [ ! -e /tmp/review-{pr}-{nonce}-oob ] || mv /tmp/review-{pr}-{nonce}-oob /tmp/review-{pr}-{nonce}-oob.stale.$$; git worktree add --detach /tmp/review-{pr}-{nonce}-oob {co}; cd /tmp/review-{pr}-{nonce}-oob. +First: export PATH=/root/bin:$PATH; git fetch origin pull/{pr}/head; git fetch origin {base}:refs/remotes/origin/{base}; git worktree remove --force /tmp/review-{pr}-{nonce}-oob 2>/dev/null; git worktree prune; [ ! -e /tmp/review-{pr}-{nonce}-oob ] || mv /tmp/review-{pr}-{nonce}-oob /tmp/review-{pr}-{nonce}-oob.stale.$$; git worktree add --detach /tmp/review-{pr}-{nonce}-oob {co}; cd /tmp/review-{pr}-{nonce}-oob. You are reviewing EXACTLY commit {co}. If that checkout fails, STOP and report it -- do not review a different commit. +The change under review is the WHOLE pull request as it stands at that commit: every file in `git diff origin/{base}...{co}` (from the PR's merge-base with {base} up to the pinned head), NOT only the head commit's own diff. A head commit that touches one file does not narrow the review to one file. READ the changed files AND enough surrounding code to verify correctness -- do NOT judge from the diff alone. Run the gates you can, EACH as ONE command prefixed with 'export PATH=/root/bin:$PATH &&' (e.g. 'export PATH=/root/bin:$PATH && go build ./...', '... && go vet ./...', client '... && npm run typecheck' / '... && npx vitest run', plugin '... && pytest'); DB-backed 'go test' needs a DB the runner lacks, so for THOSE you must not simply accept a green check. A green check is a CLAIM, not evidence: for any gate you could not run yourself, open the run log (`gh pr checks {pr}` to find the run, then `gh run view --log`) and RECONCILE it with the check's conclusion -- a step can execute, report failing tests, and STILL be reported green if its exit code was swallowed (`|| true`, continue-on-error, a wrapper that always exits 0). Quote the log line showing test counts or the failure, and NAME every gate you delegated rather than ran. If you cannot reach the log, say so and treat that gate as UNVERIFIED -- do not report it as passing. (muesli #705 shipped a CI gate that reported success while tests failed; it passed review because the reviewer was told to trust the check.) @@ -93,7 +90,7 @@ def extract_verdict(text: str, hash8: str | None = None) -> str | None: Put findings BEFORE the verdict so the verdict is the last line even if output is long.""" -def review_prompt(pr: str, repo: str, sha: str = "", nonce: str = "") -> str: +def review_prompt(pr: str, repo: str, sha: str = "", nonce: str = "", base: str = "main") -> str: """The prompt handed to the reviewer. #66: the worktree is created at the EXACT captured commit, not FETCH_HEAD. @@ -126,11 +123,11 @@ def review_prompt(pr: str, repo: str, sha: str = "", nonce: str = "") -> str: # `fatal: '/tmp/review-745' already exists`. Nothing cleans these up # either -- the runner still holds worktrees from smoke PRs #11-#16. return _PROMPT.format(pr=pr, repo=repo, co=(sha or "FETCH_HEAD"), - nonce=(nonce or (sha or "head")[:8])) + nonce=(nonce or (sha or "head")[:8]), base=(base or "main")) def dispatch(pr: str, repo: str, sha: str, *, reviewer: str, bundle_dir: str, - server: str, log_path: str, run=None) -> tuple[str | None, str]: + server: str, log_path: str, run=None, base: str = "main") -> tuple[str | None, str]: """Dispatch a reviewer and read its verdict. Returns (verdict, output). `verdict` is `PASS`, `FAIL`, or None. **None is not a soft PASS.** A @@ -167,7 +164,7 @@ def dispatch(pr: str, repo: str, sha: str, *, reviewer: str, bundle_dir: str, # so the fallback degrades to "slightly less informative", not "collides". gen = os.environ.get("BIRCHER_GENERATION", "").strip() nonce = f"{(sha or 'head')[:8]}-g{gen}" if gen.isdigit() else "" - prompt = review_prompt(pr, repo, sha, nonce=nonce) + prompt = review_prompt(pr, repo, sha, nonce=nonce, base=base) r = runner(["omnigent", "run", f"agents/{reviewer}", "--server", server, "-p", prompt], bundle_dir) out = r.stdout or "" diff --git a/v2/coordinator/wiring.py b/v2/coordinator/wiring.py index 95f61bf..9003178 100644 --- a/v2/coordinator/wiring.py +++ b/v2/coordinator/wiring.py @@ -11,7 +11,7 @@ import time from coordinator import ci as ci_mod -from coordinator import discovery, review +from coordinator import delta, discovery, review from coordinator.ci import GhError, _gh from coordinator.effects import perform_effect from coordinator.observe import ci_history @@ -27,7 +27,8 @@ def _int(name: str, default: int) -> int: def live_deps(item: str, *, repo: str, reviewer: str, server: str, bundle_dir: str, poll_interval: int, ci_wait: int = 1500, - rerun_wait: int = 900, revisions_left: int = 0, log=None) -> Deps: + rerun_wait: int = 900, revisions_left: int = 0, + round_number: int = 1, log=None) -> Deps: """Wire `derive` to the real world. EVERYTHING IS PASSED IN. Nothing here reads `REPO`, `SERVER`, `BUNDLE_DIR` @@ -102,11 +103,25 @@ def pr_state(pr): # could not read would lose the only candidate the item has. return ("", "") + def base_of(pr): + try: + return _gh(["api", f"repos/{repo}/pulls/{pr}", "--jq", ".base.ref"]).strip() or "main" + except GhError: + return "main" + + def merge_base(base, head): + try: + return _gh(["api", f"repos/{repo}/compare/{base}...{head}", + "--jq", ".merge_base_commit.sha"]).strip() + except GhError: + return "" + def do_review(pr, sha): return review.dispatch( str(pr), repo, sha, reviewer=reviewer, bundle_dir=bundle_dir, server=server, - log_path=os.environ.get("BIRCHER_REVIEW_LOG") or f"/tmp/review-{item}.log") + log_path=os.environ.get("BIRCHER_REVIEW_LOG") or f"/tmp/review-{item}.log", + base=base_of(pr)) def close_sibling(loser, winner): perform_effect( @@ -150,6 +165,10 @@ def ci_of(pr): ignore=ignore, repo=repo, revisions_left=revisions_left, + base_of=base_of, + merge_base=merge_base, + delta_digest=lambda base, head: delta.delta_digest(repo, base, head), + round_number=round_number, ) diff --git a/v2/kernel/commands.py b/v2/kernel/commands.py index e50cbf1..dc4d67d 100644 --- a/v2/kernel/commands.py +++ b/v2/kernel/commands.py @@ -672,6 +672,13 @@ def _submit(store, cmd: Command, actor: str | None, *, fenced: bool, ruling: str # proof can pair a ruling with its seat's brief # (front.brief_for reads this). "generation": cmd.generation, + # What was reviewed (spec §4, gate integrity): the pinned + # head, its merge-base with the PR's base, and the digest + # of the PR's own delta. None when the caller had none + # (a human ruling, or a derivation that pinned no head). + "head_sha": cmd.payload.get("head_sha"), + "merge_base_sha": cmd.payload.get("merge_base_sha"), + "delta_digest": cmd.payload.get("delta_digest"), }, ) _side_fact(store, cmd, actor) diff --git a/v2/kernel/events.py b/v2/kernel/events.py index f7a238a..3a4c921 100644 --- a/v2/kernel/events.py +++ b/v2/kernel/events.py @@ -66,7 +66,7 @@ class EventKind: EventKind.COMMAND_ACCEPTED: 1, EventKind.COMMAND_REJECTED: 1, EventKind.ARTIFACT_CREATED: 1, - EventKind.REVIEW_VERDICT: 1, + EventKind.REVIEW_VERDICT: 2, # 2: head_sha, merge_base_sha, delta_digest (gate integrity) EventKind.TRANSITION: 1, EventKind.OBSERVATION: 1, EventKind.HUMAN_RULING: 1, diff --git a/v2/tests/coordinator/test_attest.py b/v2/tests/coordinator/test_attest.py new file mode 100644 index 0000000..2e5b2d0 --- /dev/null +++ b/v2/tests/coordinator/test_attest.py @@ -0,0 +1,234 @@ +import pytest + +from coordinator.attest import CONTEXT, blocking_count, describe, state_for + +CODEX_FAIL = """I'll check out the commit. +Blocking findings + +- `src/main/livePromptRelay.ts:206-209` provides `stopAll()` but it is never called. +- The eight-template cap is vulnerable to concurrent creates. + +Non-blocking findings + +- None. + +Suggestions + +- Make snapshot/reread state consistent. + +Verification + +- `go build ./...`: passed +- `go vet ./...`: passed + +VERDICT: FAIL +""" + +CODEX_NUMBERED = """Blocking findings + +1. Hosted deployments have no live-prompts transport. +2. Trashing an active note does not end its live work. +3. Required feature-level E2E coverage is absent. + +Non-blocking findings + +- None. + +VERDICT: FAIL +""" + +CODEX_PASS = """The commit only corrects a regression test.Blocking findings: None. + +Non-blocking findings: None. + +Suggestions: None. + +VERDICT: PASS +""" + +CODEX_GLUED = """Local build, vet, and typecheck are green. I found a lifecycle defect that needs a close read.Blocking findings + +1. Hosted deployments have no live-prompts transport. +2. Trashing an active note does not end its live work. +3. Required feature-level E2E coverage is absent. + +Non-blocking findings + +- None. + +VERDICT: FAIL +""" + +CODEX_WORDS_IN_BULLETS = """Blocking findings + +- The final verdict logic is unreachable in prod. +- Auth check is skipped on the admin route; see the verification notes. +- Timer leak on unmount. + +Non-blocking findings + +- None. + +VERDICT: FAIL +""" + +CODEX_PHRASE_IN_PROSE = """We reviewed things and found no blocking findings issues really but lets see. + +Blocking findings + +- Real finding one. +- Real finding two. + +Non-blocking findings + +- None. + +VERDICT: FAIL +""" + +CODEX_NESTED = """Blocking findings + +- Real finding one. + - elaboration of one, not a second finding +- Real finding two. + +Non-blocking findings: None. + +VERDICT: FAIL +""" + +CODEX_BOLD = """**Blocking findings** + +- a +- b + +**Non-blocking findings** + +- None. + +**VERDICT: FAIL** +""" + +# --- the shapes a live reviewer writes that the first cut counted wrong ----- +# Every fixture below was MEASURED against the previous regexes and came back +# with the wrong number; the count beside each one in the parametrize list is +# what the text plainly says. + +#: A Markdown heading. `#` ahead of the phrase was not allowed, so this +#: counted 0 -- a two-finding FAIL posted as `FAIL (0 blocking)`. +CODEX_HASH_HEADING = """## Blocking findings + +- The relay never releases its socket on an early return. +- The cap is racy under concurrent creates. +""" + +#: Heading markers AND bold together, as GPT-5 writes them. Counted 0. +CODEX_HASH_BOLD_HEADING = """### **Blocking findings** + +- The lease is renewed after it has already expired. +""" + +#: A closing heading that itself carries `#` markers. The closer was anchored +#: with the same lead as the opener but required a SHORT trailer, so `## +#: Non-blocking findings` did not close and its bullets were counted as +#: blocking -- 1 became 4. +CODEX_HASH_CLOSERS = """## Blocking findings + +- The only real one. + +## Non-blocking findings + +- A naming nit. +- Another nit. + +## Suggestions + +- Consider a helper. +""" + +#: A closing heading with a prose trailer. `Non-blocking findings -- none +#: worth naming` did not close either, so the bullet after it was blocking. +CODEX_PROSE_TRAILER_CLOSER = """Blocking findings + +- The only real one. + +Non-blocking findings -- none worth naming + +- A nit that is not blocking. +""" + +#: The section's own count. `(2)` was captured as the rest of the heading +#: line and counted as a third finding. +CODEX_PARENTHETICAL_COUNT = """Blocking findings (2) + +- The first. +- The second. +""" + +#: The phrase continuing as PROSE. With no boundary after it this opened a +#: section and counted the sentence itself, so a clean PASS reported +#: `FAIL (1 blocking)` when the verdict went the other way. +CODEX_PHRASE_CONTINUES = """Blocking findings were all resolved. +""" + +#: A bullet that happens to contain a closing heading's word after a colon. +#: The closer must be anchored at the START of the line, or this finding +#: closes the section it belongs to and the one below it is lost. +CODEX_HEADING_WORD_IN_BULLET = """Blocking findings + +- The lease is never released: Verification of the lease is missing too. +- A second, real finding. + +Non-blocking findings: None. +""" + + +@pytest.mark.parametrize("text, expected", [ + (CODEX_FAIL, 2), + (CODEX_NUMBERED, 3), + (CODEX_PASS, 0), + (CODEX_GLUED, 3), + (CODEX_WORDS_IN_BULLETS, 3), + (CODEX_PHRASE_IN_PROSE, 2), + (CODEX_NESTED, 2), + (CODEX_BOLD, 2), + (CODEX_HASH_HEADING, 2), + (CODEX_HASH_BOLD_HEADING, 1), + (CODEX_HASH_CLOSERS, 1), + (CODEX_PROSE_TRAILER_CLOSER, 1), + (CODEX_PARENTHETICAL_COUNT, 2), + (CODEX_PHRASE_CONTINUES, 0), + (CODEX_HEADING_WORD_IN_BULLET, 2), + ("", 0), + ("no headings at all\nVERDICT: PASS", 0), +]) +def test_blocking_findings_are_counted_from_the_blocking_section_only(text, expected): + assert blocking_count(text) == expected + + +def test_context_is_the_gate_the_repository_requires(): + assert CONTEXT == "bircher/cross-review" + + +@pytest.mark.parametrize("verdict, state", [ + ("PASS", "success"), ("FAIL", "failure"), (None, "error"), ("", "error"), ("BLOCKED", "error"), +]) +def test_state_follows_the_verdict_and_silence_is_an_error(verdict, state): + assert state_for(verdict) == state + + +def test_the_description_names_vendor_round_verdict_and_range(): + d = describe("codex", 3, "PASS", 0, "2742ae4f" + "0" * 32, "e4ea3eb7" + "1" * 32) + assert d == "codex round 3 PASS on 2742ae4..e4ea3eb" + + +def test_a_fail_carries_its_blocking_count(): + assert describe("codex", 4, "FAIL", 3, "a" * 40, "b" * 40) == "codex round 4 FAIL (3 blocking) on aaaaaaa..bbbbbbb" + + +def test_no_verdict_says_so_and_an_unknown_base_is_a_question_mark(): + assert describe("claude_code", 1, None, 0, "", "b" * 40) == "claude_code round 1 no verdict on ?..bbbbbbb" + + +def test_the_description_never_exceeds_githubs_limit(): + assert len(describe("x" * 200, 1, "PASS", 0, "a" * 40, "b" * 40)) <= 140 diff --git a/v2/tests/coordinator/test_ci_and_review.py b/v2/tests/coordinator/test_ci_and_review.py index 4c43bfc..402d8c9 100644 --- a/v2/tests/coordinator/test_ci_and_review.py +++ b/v2/tests/coordinator/test_ci_and_review.py @@ -15,9 +15,6 @@ keep_blocking, normalize) from coordinator.review import extract_verdict -REPO_ROOT = pathlib.Path(__file__).resolve().parents[3] -RUN_QUEUE = REPO_ROOT / "batch" / "run-queue.sh" - #: The bash these replaced, CAPTURED AS IT SHIPPED. #: @@ -453,20 +450,33 @@ def test_a_pr_with_no_workflow_runs_is_genuine(): from coordinator.review import dispatch, review_prompt -def test_the_rendered_prompt_is_byte_identical_to_the_bash(): - """Prose carrying scars (#705, #666, #66). A paraphrase would drop one - silently, so both are rendered and compared rather than eyeballed.""" - src = RUN_QUEUE.read_text().splitlines() - i = next(k for k, l in enumerate(src) - if l.startswith("_recovery_review_prompt() {")) - end = next(k for k in range(i + 1, len(src)) if src[k] == "}") - fn = "\n".join(src[i:end + 1]) - script = (f'set -uo pipefail\nREPO="$REPO_IN"\n{fn}\n' - '_recovery_review_prompt "$PR_IN" "$SHA_IN"') - r = subprocess.run(["bash", "-c", script], capture_output=True, text=True, - env={"PATH": "/usr/bin:/bin", "REPO_IN": "o/r", - "PR_IN": "7", "SHA_IN": "abc123"}) - assert r.stdout.rstrip("\n") == review_prompt("7", "o/r", "abc123").rstrip("\n") +def test_the_prompt_names_the_range_against_the_prs_base(): + p = review_prompt("7", "o/r", "abc123", base="develop") + assert "git diff origin/develop...abc123" in p + # ...and the range is FETCHABLE. The setup fetched only the pull head, so + # on a non-default base `origin/develop` need not exist in the reviewer's + # clone and the diff the sentence names fails outright. The `:` + # form is the one that updates the remote-tracking ref. + assert "git fetch origin develop:refs/remotes/origin/develop" in p + assert "NOT only the head commit's own diff" in p + # The pinned checkout is unchanged by naming the range. + assert "You are reviewing EXACTLY commit abc123." in p + + +def test_the_base_defaults_to_main(): + assert "git diff origin/main...abc123" in review_prompt("7", "o/r", "abc123") + + +def test_the_range_reaches_the_runner(tmp_path): + seen = {} + + def run(argv, cwd): + seen["argv"] = argv + return _R(0, "VERDICT: PASS") + + dispatch("7", "o/r", "dead", reviewer="claude_code", bundle_dir="/b", + server="http://s", log_path=str(tmp_path / "l"), run=run, base="release") + assert any("origin/release...dead" in a for a in seen["argv"]), "the base must reach the prompt" def test_an_absent_sha_falls_back_to_FETCH_HEAD(): diff --git a/v2/tests/coordinator/test_delta.py b/v2/tests/coordinator/test_delta.py new file mode 100644 index 0000000..cb32264 --- /dev/null +++ b/v2/tests/coordinator/test_delta.py @@ -0,0 +1,99 @@ +import hashlib +import json +import subprocess +from pathlib import Path + +import pytest + +from coordinator.delta import canonical_delta, delta_digest + +COMPARE_B1 = {"files": [{"filename": "a.txt", "status": "modified", + "sha": "blob0000000000000000000000000000000000b1", + "patch": "@@ -1 +1 @@\n-old\n+new"}]} +COMPARE_B2 = {"files": [{"filename": "a.txt", "status": "modified", + "sha": "blob0000000000000000000000000000000000b2", + "patch": "@@ -1 +1 @@\n-old\n+SOMETHING ELSE"}]} +TREE = {"truncated": False, "tree": [{"path": "a.txt", "mode": "100644", "type": "blob"}]} + + +def _gh_for(compare, tree): + def gh(args): + joined = " ".join(args) + if "/compare/" in joined: + return json.dumps(compare) + if "/git/trees/" in joined: + return json.dumps(tree) + raise AssertionError(f"unexpected gh call {args}") + return gh + + +def test_the_canonical_form_is_sorted_compact_and_carries_mode_and_type(): + text = canonical_delta(COMPARE_B1, TREE) + assert text == ('[{"entry":"100644|blob","filename":"a.txt","patch":"@@ -1 +1 @@\\n-old\\n+new",' + '"previous_filename":null,"sha":"blob0000000000000000000000000000000000b1",' + '"status":"modified"}]') + + +def test_the_digest_is_the_sha256_of_the_canonical_form(): + text = canonical_delta(COMPARE_B1, TREE) + # +"\n": the retired bash `_pr_delta_digest` piped `jq -cS`'s stdout -- + # always newline-terminated -- straight into `sha256sum`, so that byte is + # part of every digest it produced. Matched here, not stripped, so this + # digest agrees with the bash oracle for identical content (see + # test_the_port_reproduces_the_bash_digest_for_the_fixture below). + assert delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) == hashlib.sha256((text + "\n").encode()).hexdigest() + + +def test_the_port_reproduces_the_bash_digest_for_the_fixture(): + # The frozen answer from the retired `_pr_delta_digest`, captured before + # its deletion by running it (unmodified) against this exact fixture: + # cd .../back-half-closed-loop + # tmp=$(mktemp -d); cat > "$tmp/gh" <<'SH' ... SH; chmod +x "$tmp/gh" + # PATH="$tmp:$PATH" REPO=o/r bash -c '; _pr_delta_digest main b1' + # -> d87efb25c38472efc67874cce8fe3f4a7efde9c89324e6577112556031e323e9 + assert delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) == \ + "d87efb25c38472efc67874cce8fe3f4a7efde9c89324e6577112556031e323e9" + + +def test_a_changed_patch_changes_the_digest(): + assert delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) != \ + delta_digest("o/r", "main", "b2", gh=_gh_for(COMPARE_B2, TREE)) + + +def test_a_path_absent_from_the_tree_is_marked_absent(): + assert '"entry":"ABSENT"' in canonical_delta(COMPARE_B1, {"truncated": False, "tree": []}) + + +@pytest.mark.parametrize("compare, tree", [ + ({"files": []}, TREE), # 0 files + ({"files": [dict(COMPARE_B1["files"][0]) for _ in range(300)]}, TREE), # GitHub's cap + ({"files": [{"filename": "a.txt", "status": "modified", "sha": "x"}]}, TREE), # patch withheld + (COMPARE_B1, {"truncated": True, "tree": []}), # truncated tree + (COMPARE_B1, {"tree": []}), # truncated absent, not false +]) +def test_every_unprovable_case_yields_no_digest(compare, tree): + assert canonical_delta(compare, tree) is None + assert delta_digest("o/r", "main", "b1", gh=_gh_for(compare, tree)) == "" + + +def test_a_failed_lookup_is_unprovable_not_a_crash(): + """FileNotFoundError, not RuntimeError: the failure the name describes is + a runner with no `gh` on PATH, and that is an OSError. Raising a + RuntimeError proved only that the tuple already listed RuntimeError.""" + def gh(args): + raise FileNotFoundError(2, "No such file or directory: 'gh'") + assert delta_digest("o/r", "main", "b1", gh=gh) == "" + + +def test_the_cli_prints_the_digest_or_nothing(tmp_path, monkeypatch): + # A fake gh on PATH, answering by URL shape as the self-test's does. + fake = tmp_path / "gh" + fake.write_text('#!/bin/bash\ncase "$*" in *"/compare/"*) printf %s \'' + json.dumps(COMPARE_B1) + + '\' ;; *"/git/trees/"*) printf %s \'' + json.dumps(TREE) + '\' ;; *) exit 1 ;; esac\n') + fake.chmod(0o755) + env = {"PATH": f"{tmp_path}:/usr/bin:/bin", "BIRCHER_GH_REPO": "o/r"} + r = subprocess.run(["python3", "-m", "coordinator.cli", "delta", "--repo", "o/r", "--base", "main", "--ref", "b1"], + capture_output=True, text=True, env=env, cwd=str(Path(__file__).resolve().parents[2])) + assert r.returncode == 0 + assert r.stdout.strip() == delta_digest("o/r", "main", "b1", gh=_gh_for(COMPARE_B1, TREE)) diff --git a/v2/tests/coordinator/test_outcome.py b/v2/tests/coordinator/test_outcome.py index 6eca02f..dfb8f78 100644 --- a/v2/tests/coordinator/test_outcome.py +++ b/v2/tests/coordinator/test_outcome.py @@ -3,6 +3,8 @@ Every dependency arrives in one injected object, so these exercise `derive` rather than a rearrangement of it. """ +import hashlib + import pytest from coordinator.outcome import Deps, Derived, derive @@ -22,14 +24,14 @@ def _deps(**over): return d -def test_the_tuple_has_eight_fields_in_order(): - """Eight since the settled PR joined it. The width is asserted against +def test_the_tuple_has_ten_fields_in_order(): + """Ten since the reviewed range joined it. The width is asserted against `Derived.FIELDS` as well as the literal, because the shell's reader is checked against that constant and two numbers that can disagree will.""" r = derive("i1", "i1", "7", "", deps=_deps()) - assert len(r.as_tuple()) == 8 == Derived.FIELDS + assert len(r.as_tuple()) == 10 == Derived.FIELDS assert r.outcome == "ready" - assert r.as_tuple()[5:] == ("true", 0, "7") + assert r.as_tuple()[5:] == ("true", 0, "7", "", "") assert len(r.as_line().split("|")) == Derived.FIELDS @@ -104,7 +106,10 @@ def test_a_GENUINE_red_is_never_re_run(): def test_the_comment_is_posted_through_the_effect_path(): d = _deps() derive("i1", "i1", "7", "", deps=d) - assert [c for c, _k, _a in d.posted] == ["comment"] + # The status posts BEFORE the comment: it rides the same pinned head the + # verdict was captured against, and the comment documents the outcome + # that head's verdict fed into. + assert [c for c, _k, _a in d.posted] == ["status_check", "comment"] def test_the_comment_carries_no_bircher_status_line(): @@ -124,7 +129,8 @@ def test_an_abandoned_tracked_pr_is_dropped_and_rediscovered(): discover_by_code=lambda code: ["9"]) r = derive("i1", "i1", "7", "", deps=d) assert r.outcome == "ready" - assert d.posted and "9" in d.posted[0][2] + # The comment is the LAST post: the status precedes it. + assert d.posted and "9" in d.posted[-1][2] def test_no_pr_anywhere_is_a_timeout(): @@ -146,7 +152,7 @@ def test_exactly_one_issue_match_adopts_and_two_do_not(): def test_a_reconciled_sibling_replaces_the_tracked_pr(): d = _deps(reconcile=lambda code, pr: "6") derive("i1", "i1", "5", "", deps=d) - assert "6" in d.posted[0][2] + assert "6" in d.posted[-1][2] def test_a_TERMINAL_outcome_carries_no_sha(): @@ -241,8 +247,9 @@ def test_the_comment_key_is_stable_for_the_same_body(): d2 = _deps() derive("i1", "i1", "7", "", deps=d2) - key1 = d1.posted[0][1] - key2 = d2.posted[0][1] + # The comment is the LAST post: the status precedes it. + key1 = d1.posted[-1][1] + key2 = d2.posted[-1][1] # NOTE: this equality DOES NOT BIND the defect. `hash()` is stable WITHIN # a process, so both derives agree even with the bug present -- confirmed # by mutation: restoring `abs(hash(body))` left this assertion passing and @@ -252,7 +259,7 @@ def test_the_comment_key_is_stable_for_the_same_body(): # THIS is the binding assertion: the key must equal a specific, # process-independent digest, which `hash()` cannot produce. - body = d1.posted[0][2][-1] + body = d1.posted[-1][2][-1] assert key1 == f"pr-comment:7:{hashlib.sha256(body.encode()).hexdigest()[:16]}" @@ -365,7 +372,9 @@ def test_the_comment_is_addressed_to_the_repository_under_management(): d = _deps(repo="owner/target") derive("i1", "i1", "7", "", deps=d) - argv = d.posted[0][2] + # The comment is the LAST post: the status precedes it, and names its + # repository in the URL rather than a `--repo` flag. + argv = d.posted[-1][2] assert "--repo" in argv, "the comment effect must name its repository" assert argv[argv.index("--repo") + 1] == "owner/target", ( f"the comment was addressed to the wrong repository: {argv}") @@ -384,7 +393,8 @@ def test_derive_returns_the_pr_it_settled_on_not_the_one_passed_in(): assert r.pr == "738", ( "derive discarded the abandoned #737 and discovered #738, but returned " f"{r.pr!r} to the caller that has to merge it") - assert r.as_line().split("|")[-1] == "738" + # `pr` is field 8 of 10 -- the range fields ride out after it. + assert r.as_line().split("|")[7] == "738" def test_derive_returns_a_reconciled_sibling(): @@ -398,3 +408,144 @@ def test_derive_returns_the_original_pr_when_nothing_changed(): """The common case must round-trip, or the caller adopts a wrong value.""" r = derive("i1", "i1", "7", "", deps=_deps()) assert r.pr == "7" + + +def _green_pass_deps(effects, verdict="PASS", out="VERDICT: PASS"): + return Deps( + checks=lambda pr: "ci|pass", + head_of=lambda pr: "e4ea3eb7" + "1" * 32, + review=lambda pr, sha: (verdict, out), + effect=lambda cls, key, argv: effects.append((cls, key, argv)) or "ok", + base_of=lambda pr: "develop", + merge_base=lambda base, head: "2742ae4f" + "0" * 32, + delta_digest=lambda base, head: "d" * 64, + round_number=3, + reviewer="codex", + repo="o/r", + ) + + +def test_the_line_has_ten_fields_and_carries_the_range(): + effects = [] + r = derive("i", "c", "7", "", deps=_green_pass_deps(effects)) + assert r.FIELDS == 10 + fields = r.as_line().split("|") + assert len(fields) == 10 + assert fields[8] == "2742ae4f" + "0" * 32 + assert fields[9] == "d" * 64 + + +def _status_key(head, state, desc): + """The key the derivation must compute: the head, then the CONTENT of the + post. Written out here rather than imported so the test states the format + independently of the code that produces it.""" + return f"status:{head}:" + hashlib.sha256(f"{state}\n{desc}".encode()).hexdigest()[:16] + + +def test_a_pass_posts_a_success_status_naming_the_range(): + effects = [] + derive("i", "c", "7", "", deps=_green_pass_deps(effects)) + posts = [e for e in effects if e[0] == "status_check"] + assert len(posts) == 1 + cls, key, argv = posts[0] + assert key == _status_key("e4ea3eb7" + "1" * 32, "success", + "codex round 3 PASS on 2742ae4..e4ea3eb") + assert "repos/o/r/statuses/e4ea3eb7" + "1" * 32 in argv + assert "state=success" in argv + assert "context=bircher/cross-review" in argv + assert "description=codex round 3 PASS on 2742ae4..e4ea3eb" in argv + + +def test_a_fail_posts_failure_with_its_blocking_count(): + effects = [] + out = "Blocking findings\n\n- one\n- two\n\nNon-blocking findings\n\n- None.\n\nVERDICT: FAIL\n" + derive("i", "c", "7", "", deps=_green_pass_deps(effects, verdict="FAIL", out=out)) + argv = [e for e in effects if e[0] == "status_check"][0][2] + assert "state=failure" in argv + assert "description=codex round 3 FAIL (2 blocking) on 2742ae4..e4ea3eb" in argv + + +def test_no_verdict_posts_error(): + effects = [] + derive("i", "c", "7", "", deps=_green_pass_deps(effects, verdict="NONE", out="rambling")) + argv = [e for e in effects if e[0] == "status_check"][0][2] + assert "state=error" in argv + assert "no verdict on 2742ae4..e4ea3eb" in " ".join(argv) + + +def test_two_verdicts_on_one_head_do_not_share_an_effect_key(): + """THE REPLAY. The round only advances on a recorded `review_verdict` + fact, and an `error` (no verdict) or a terminal FAIL records none -- so + `status::` was the SAME key for a FAIL and for the PASS that + superseded it, and `kernel/effects.py` returns the prior external id on a + replayed key WITHOUT executing. The later verdict was never posted and the + gate kept showing the earlier one. + """ + first, second = [], [] + out = "Blocking findings\n\n- one\n\nVERDICT: FAIL\n" + derive("i", "c", "7", "", deps=_green_pass_deps(first, verdict="FAIL", out=out)) + derive("i", "c", "7", "", deps=_green_pass_deps(second)) # a PASS, same head + fail_key = [e for e in first if e[0] == "status_check"][0][1] + pass_key = [e for e in second if e[0] == "status_check"][0][1] + assert fail_key != pass_key, ( + "a FAIL and a later PASS on the same head share an effect key, so the " + "PASS replays the FAIL's journalled effect and never posts") + # ...and both still name the head they are about. + head = "e4ea3eb7" + "1" * 32 + assert fail_key.startswith(f"status:{head}:") and pass_key.startswith(f"status:{head}:") + + +def test_an_unparseable_merge_base_is_recorded_as_unknown(): + """`gh` prints `null` for a merge-base it cannot compute, and that text was + taken raw into the tuple, the description and (through the runner) into + hand-built JSON. It gets the head's own 40-hex validation.""" + effects = [] + d = _green_pass_deps(effects) + d.merge_base = lambda base, head: "null\n" + r = derive("i", "c", "7", "", deps=d) + assert r.merge_base == "" + assert r.as_line().split("|")[8] == "" + argv = [e for e in effects if e[0] == "status_check"][0][2] + assert "description=codex round 3 PASS on ?..e4ea3eb" in argv + + +def test_a_failed_status_post_does_not_change_the_outcome(): + def effect(cls, key, argv): + if cls == "status_check": + raise RuntimeError("github is down") + return "ok" + d = _green_pass_deps([]) + d.effect = effect + r = derive("i", "c", "7", "", deps=d) + assert r.outcome == "ready" + + +def test_no_status_is_posted_without_a_pinned_head(): + effects = [] + d = _green_pass_deps(effects) + d.head_of = lambda pr: "not-a-sha" + derive("i", "c", "7", "", deps=d) + assert not [e for e in effects if e[0] == "status_check"] + + +def test_red_ci_posts_nothing_and_carries_no_range(): + effects = [] + d = _green_pass_deps(effects) + d.checks = lambda pr: "ci|fail" + r = derive("i", "c", "7", "", deps=d) + assert not [e for e in effects if e[0] == "status_check"] + assert r.merge_base == "" and r.delta_digest == "" + + +def test_the_round_is_revisions_used_plus_one(tmp_path): + from coordinator.cli import _round_number + from kernel.store import Store + db = tmp_path / "k.db" + Store.open(str(db)) # an empty journal + assert _round_number(str(db), "run-1") == 1 + + +def test_no_journal_means_round_one(): + from coordinator.cli import _round_number + assert _round_number("", "") == 1 + assert _round_number("/nonexistent/path.db", "run-1") == 1 diff --git a/v2/tests/coordinator/test_repair_loop.py b/v2/tests/coordinator/test_repair_loop.py index eddd204..836c6c2 100644 --- a/v2/tests/coordinator/test_repair_loop.py +++ b/v2/tests/coordinator/test_repair_loop.py @@ -108,7 +108,7 @@ def test_the_findings_never_enter_the_pipe_delimited_line(): from coordinator.outcome import Derived d = Derived("revise", "cx:fail", "n", "a" * 40, "green", "true", 0, "7", findings="blocking:\n- one | two\n- three") - assert len(d.as_line().split("|")) == Derived.FIELDS == 8 + assert len(d.as_line().split("|")) == Derived.FIELDS == 10 assert "\n" not in d.as_line() assert "blocking" not in d.as_line() diff --git a/v2/tests/execution/test_env_boundary_contract.py b/v2/tests/execution/test_env_boundary_contract.py index e16b334..6b1d227 100644 --- a/v2/tests/execution/test_env_boundary_contract.py +++ b/v2/tests/execution/test_env_boundary_contract.py @@ -64,6 +64,14 @@ # before creating it, so an absent generation degrades to a less # informative name and not to a collision. "BIRCHER_GENERATION": "runner", + # RUNNER-SET: run-queue.sh exports both before the main loop / at run + # start and they stay exported for the process's remaining lifetime, so + # `derive`'s subprocess inherits them same as BIRCHER_GENERATION above. + # `_round_number` reads them to compute the review round from the kernel + # journal -- absent, empty, or an unreadable database all degrade to + # round 1 rather than blocking a status post. + "BIRCHER_KERNEL_DB": "runner", + "BIRCHER_RUN_ID": "runner", } _READ = re.compile( diff --git a/v2/tests/execution/test_front_half_seam.py b/v2/tests/execution/test_front_half_seam.py index 72ba011..a582c0c 100644 --- a/v2/tests/execution/test_front_half_seam.py +++ b/v2/tests/execution/test_front_half_seam.py @@ -104,9 +104,9 @@ def _heredoc_to_herestring(src): restriction it routes around. Matched exactly, so an edit to the real line fails loudly here rather than silently reverting.""" old = ("IFS='|' read -r outcome review note observed_head _obs_ci ci_first " - "resubmissions _settled_pr < deleted in C8 Phase 2 Their replacements are `tests/coordinator/test_ci_and_review.py` (which also -proves the review PROMPT renders byte-identically to the bash it came from) and -`tests/coordinator/test_outcome.py`, which drives `derive` through eighteen -cases -- more than this file ever had, and without a shell-extraction rig. +covers the review PROMPT, which is the coordinator's own -- there is no bash +copy to compare it against) and `tests/coordinator/test_outcome.py`, which +drives `derive` through eighteen cases -- more than this file ever had, and +without a shell-extraction rig. Kept as a stub rather than deleted so the retirement is visible in the tree rather than only in a commit message. diff --git a/v2/tests/execution/test_run_item_argument_wiring.py b/v2/tests/execution/test_run_item_argument_wiring.py index ac9a315..1071dc0 100644 --- a/v2/tests/execution/test_run_item_argument_wiring.py +++ b/v2/tests/execution/test_run_item_argument_wiring.py @@ -127,6 +127,13 @@ #: distinct from OUT_HASH so a call site that threads the artifact where the #: context belongs is visible. CTX_HASH = "ctxbundle" + "0" * 55 +#: What the stubbed `observe_outcome`'s ninth field echoes -- the merge-base +#: between the PR's base and the reviewed head (spec §4). A sha, distinct from +#: HEAD_SHA/REVIEWED_SHA so a call site that threads the wrong one is visible. +MERGE_BASE_SHA = "d" * 40 +#: The tenth field: the digest of the PR's own delta at the reviewed head. +#: A hex digest, distinct from OUT_HASH/CTX_HASH for the same reason. +DELTA_DIGEST = "e" * 64 VENDOR = "claude_code" REVIEWER = "codex" PR = "777" @@ -254,9 +261,9 @@ def _heredoc_to_herestring(run_item_src): pairs = [ ( "IFS='|' read -r outcome review note observed_head _obs_ci ci_first " - "resubmissions _settled_pr <