diff --git a/shared/workflow/address-every-comment.md b/shared/workflow/address-every-comment.md index 6c43252f..2eb9102b 100644 --- a/shared/workflow/address-every-comment.md +++ b/shared/workflow/address-every-comment.md @@ -375,6 +375,53 @@ overwriting, since the wrong version is what earlier comments respond to. - **Don't:** treat a clean `grep` over the diff as evidence every paraphrase is synced --- the description was never in it. +**Answering a body-staleness finding with a correction comment does not clear +it, and this corpus's own visible-correction convention is what makes that +move attractive.** +The paragraph above says to state the correction as history in the body. +It does not name the near-miss, which is to leave the body alone and post a +comment instead --- and that reads as compliance rather than as evasion, +because [`ardi`](ardi.md)'s rule for a published wrong SHA prescribes exactly a +visible note. + +The two artifacts differ in who re-reads them, which is what decides it. +A claim in a comment thread is read once, so a later comment correcting it +reaches everyone the original reached. +A PR body is re-read from scratch by every subsequent reviewer, so the stale +figure is still the first thing the next round meets. +A comment therefore leaves the finding live, and the round buys nothing. + +Both halves are needed rather than either alone. +Editing removes the false figure, and recording the correction inside the body +keeps earlier rounds that cite the old numbers resolvable --- which is the +thing the paragraph above is protecting. +A short "Corrections to this body" table does both at once. + +The re-dispatch this costs is the part that usually goes unweighed, and it is +cheap. +A confirming round --- head unchanged, one finding to re-verify --- came in at +roughly 40% of the first full round's cost on the PR below, so fixing the body +and re-dispatching beats arguing the finding down. + +- **Do:** edit the body **and** record the correction inside it, so nothing is + silently overwritten and earlier rounds still resolve. +- **Don't:** answer a body-staleness finding with a comment --- the next + reviewer re-reads the body, so the finding survives it. +- **Don't:** treat the drift risk in rewriting a long body as a reason to leave + it; re-deriving every figure is what the round already requires. + +(Morrison-Lab/ai-config#1384, 2026-08-10. +An earlier finding in the same PR was answered with a comment-only correction, +reasoned from the risk of drift in rewriting a long body assembled across +several rounds. +That reasoning was sound about the risk and wrong about the outcome. +The next round re-read the body and raised three stale figures --- a prose +figure its own commit `7fe25776` had corrected, plus a line count and a +diffstat --- and the fix was a body edit carrying a `Corrections to this body` +table, which round 3 then confirmed resolved and cited by name. +The three rounds cost `$11.1760`, `$8.4658`, and `$4.5018`; the last is the +confirming round.) + **A body that reports volatile external state goes stale with no edit of yours, so the trigger above never fires on it.** That trigger is an action *you* take, and this is the case where you take diff --git a/shared/workflow/fully-clean.cases.md b/shared/workflow/fully-clean.cases.md index 85a963fb..d956cfaa 100644 --- a/shared/workflow/fully-clean.cases.md +++ b/shared/workflow/fully-clean.cases.md @@ -510,3 +510,142 @@ The survivor is simply the one nothing followed; it posted a genuine verdict at The session's own reading of `gh run list` counted four colliding dispatches, because that list reports `headBranch: main` for every one of them --- two of the four were other PRs' reviews and were never in #1281's group at all.) + +## `pull_requests[].head.sha` named a commit pushed after the run started + +(Morrison-Lab/ai-config#1384, 2026-08-10, merged as `edfab8d8`. +Review run +[31354330266](https://github.com/Morrison-Lab/ai-config/actions/runs/31354330266) +was a `workflow_dispatch` at `a0ef37c2`, started `04:03:25Z`. +Commit `7fe25776` was authored `04:05:18Z`, after the run began, and became the +PR's head. +The run's `pull_requests[0].head.sha` was reported during the session as +`7fe25776` --- the newer commit --- while its `head_sha` correctly read +`a0ef37c2`. + +The body settled it. +That review's verification section quotes the prose figure as "40 to 151 +seconds", which exists only at the older commit: + +```bash +git show a0ef37c2:memories/claude-code-scheduling.md | grep -n '40 to 151' +#=> 189:`run_once_at + 24h` the same rows are off by 40 to 151 seconds, tracking each +git show 7fe25776:memories/claude-code-scheduling.md | sed -n '189p' +#=> `run_once_at + 24h` the same 18 rows spread from 0.6 to 556 seconds over. +``` + +Timing would also have caught this one, per the block above, since the run +predates the commit. +What the field added was **positive evidence pointing the other way**, which is +why it needed its own entry: it made "the verdict may already cover current +content" a live hypothesis rather than an idle one. + +The specific reading is **not reproducible now**, because #1384 has merged and +the array empties on close --- all three of its review runs return +`pull_requests: []` today. +The general behaviour is reproducible, and was measured over the 60 most recent +runs in this repo on 2026-08-10: + +```bash +curl -sS "https://api.github.com/repos/Morrison-Lab/ai-config/actions/runs?per_page=60" \ + > runs.json +python3 -c " +import json, urllib.request +runs = json.load(open('runs.json'))['workflow_runs'] +cur = {} +def head(n): + if n not in cur: + u = f'https://api.github.com/repos/Morrison-Lab/ai-config/pulls/{n}' + cur[n] = json.load(urllib.request.urlopen(u))['head']['sha'] + return cur[n] +ne = [r for r in runs if (r.get('pull_requests') or [])] +agree = sum(1 for r in ne if r['pull_requests'][0]['head']['sha'] == head(r['pull_requests'][0]['number'])) +stale = sum(1 for r in ne if r['head_sha'] != r['pull_requests'][0]['head']['sha']) +print(f'examined {len(runs)}; non-empty {len(ne)}; field==current head {agree}; run head_sha differs {stale}') +" +#=> examined 60; non-empty 14; field==current head 14; run head_sha differs 8 +``` + +So the field equalled the PR's current head in **14 of 14** cases, and in **8 +of those 14** the run's own `head_sha` was a different commit. +The sharpest single instance is PR #1374, open at the time. +Eight of its runs span four distinct `head_sha` values (`8e7a2526`, +`af838843`, `158d311d`, `d1d02a19`), and every one of the eight reports +`d1d02a19` --- the PR's head at read time --- as its `pull_requests[0].head.sha`. + +One limit on that measurement, stated rather than smoothed over. +The instrument is validated in both directions, since 14 of 60 runs returned a +non-empty array --- so an empty read is informative rather than a broken query. +But every `workflow_dispatch` run in that sample returned empty (10 of 10), and +none of them sat on a currently-open PR's branch, so the sample by itself could +not separate "dispatch runs never populate the array" from "the array empties +once the PR closes". +The branch-level split already favoured the second: branches whose PRs are open +returned non-empty in every case, and branches whose PRs had merged returned +empty in every case. + +That ambiguity is now settled, by a counterexample this entry's own PR +produced. +`Morrison-Lab/ai-config#1388`'s review dispatch, run +[31357711790](https://github.com/Morrison-Lab/ai-config/actions/runs/31357711790), +is a `workflow_dispatch` on an **open** PR's branch, and it returns a +**non-empty** array: + +```bash +curl -sS "https://api.github.com/repos/Morrison-Lab/ai-config/actions/runs/31357711790" \ + | python3 -c "import json,sys; r=json.load(sys.stdin); print(r['event'], r['head_branch'], r['head_sha'][:8], [(p['number'], p['head']['sha'][:8]) for p in r['pull_requests']])" +#=> workflow_dispatch ums/pr1384-pull-requests-head-sha ede6b0a9 [(1388, '5bed1d61')] +``` + +That output is **read at 2026-08-10, when #1388's head was `5bed1d61`**, and +the second SHA is expected to differ on any later run. +`head_sha` is fixed at `ede6b0a9` for the life of the run; the array's entry +tracks the PR, so re-running the command reports whatever that PR's head is at +read time. +Read a mismatch between the two as the normal case rather than as a discrepancy +to reconcile. + +So "dispatch runs never populate the array" is false, and the emptiness the +sample recorded is explained by PR **closure** rather than by trigger type. +One counterexample is enough here, because the hypothesis it refutes was a +universal. + +The same run is a **second case record for the entry's own thesis**, and it +arrived by falsifying a sentence this file previously carried. +That sentence said the run was dispatched with `--ref` at the PR branch, so its +`head_sha` and the field agreed, and it therefore could not also illustrate the +two diverging. +Both halves were wrong. +`--ref` pins `head_sha` and does nothing to the other field, which re-resolves +on every read, so the agreement was never a guarantee --- only a fact about +dispatch time, before any further push. +Pushing the very commit that recorded the sentence is what separated them, and +the output above is the corrected reading. + +What the run then shows is the field's usefulness **inverting** while its +behaviour stays constant. +On #1384 it pointed at a commit newer than the one reviewed, which made a +superseded verdict look current --- the confident direction, and the reason +this entry exists. +Here it points at a commit newer than the one whose `require-review` went red, +which correctly says the PR has moved past that commit and the red check is a +`cancelled` run at a superseded head. +Neither reading is wrong, because the field reports the PR's current head in +both. +What differs is the question being asked of it, which is the entry's thesis +stated twice rather than once. + +The transferable lesson is one artifact further in than the PR-body staleness +that +[`address-every-comment`](address-every-comment.md) documents. +A sentence asserting that two **live** fields agree is a state claim with a +short shelf life, and where that sentence is written into a commit, the commit +is itself the event that can falsify it. +So a claim of agreement needs the time it was true at, or it needs to be a +claim about mechanism instead --- and mechanism is what to reach for, since +`--ref` supports a claim about `head_sha` alone. + +- **Do:** date a claim that two live fields agree, or state the mechanism that + makes one of them stable, rather than asserting the agreement flatly. +- **Don't:** infer from `--ref` that `pull_requests[].head.sha` is pinned; it + pins `head_sha`, and the other field re-resolves on every read.) diff --git a/shared/workflow/fully-clean.md b/shared/workflow/fully-clean.md index 648b9d10..cf189083 100644 --- a/shared/workflow/fully-clean.md +++ b/shared/workflow/fully-clean.md @@ -690,6 +690,48 @@ A verdict that is empirically wrong about present file content is conclusive pro - **Do:** cross-check a stale-suspected verdict's specific claims against the file directly, rather than only against run metadata. - **Don't:** trust `head_sha` as "the commit reviewed" on a workflow-dispatch-triggered run --- that guarantee only holds for push/pull_request-triggered runs, which check out the PR head by construction. +**A third surface names a commit the run never read, and unlike the two above +it points the confident direction: the run object's own +`pull_requests[].head.sha`.** +Both rules above leave you without a usable SHA --- a caption that may be +stale, and a `head_sha` that names the dispatch ref. +A workflow run object also carries a `pull_requests[]` array, and its +`head.sha` looks like the missing answer. +It is not an answer about the run at all. +That field is a live pointer to the pull request, resolved when you read it, so +it reports the PR's **current** head whatever commit the run checked out or +diffed. + +The failure direction inverts relative to the caption case. +A stale caption reads as a stale review, which invites a needless re-trigger. +This field reads as a **current** review, so it argues that the verdict already +covers your latest push --- and it argues that with a real SHA matching your +branch tip exactly. + +The field also empties once the PR closes, so on a merged PR it answers nothing +rather than answering wrongly. +Read an empty array as carrying no information, not as a finding. + +So the field cannot distinguish the case this criterion is about, and the +remedy is the one the block above already reaches for in its last line: +**read the review body.** +Find a figure, a quotation, or a claim in the verdict whose value differs +between the candidate commits, and check which one it states. +A verdict empirically wrong about present file content read an earlier commit, +whatever any SHA field says. + +- **Do:** settle which commit a review read from a discriminating claim in its + own body, since that is the only surface separating the candidates. +- **Do:** read `pull_requests[].head.sha` as a fact about the PR's current + head, useful for nothing else. +- **Don't:** read that field naming your latest commit as evidence the review + covered it --- it names the current head unconditionally. +- **Don't:** read an empty `pull_requests` as evidence about the run; the array + empties when the PR closes. + +See [`fully-clean.cases.md`](fully-clean.cases.md), "`pull_requests[].head.sha` +named a commit pushed after the run started". + **A clean CI run and a clean review verdict are a snapshot, not a standing guarantee of mergeability.** `main` can advance after your last check --- including gaining its own independent addition that collides with yours