Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
47 changes: 47 additions & 0 deletions shared/workflow/address-every-comment.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
139 changes: 139 additions & 0 deletions shared/workflow/fully-clean.cases.md
Original file line number Diff line number Diff line change
Expand Up @@ -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.)
42 changes: 42 additions & 0 deletions shared/workflow/fully-clean.md
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down
Loading