From be0987e891bf375e8512c4574b1e9730b7c35dc4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 04:53:56 +0000 Subject: [PATCH 1/4] start: record pull_requests[].head.sha as a third misleading surface (closes #1387) From ede6b0a92b8627bd567dafc65a6d9918cad55e81 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 04:56:18 +0000 Subject: [PATCH 2/4] ums: pull_requests[].head.sha tracks the PR, not the run Records a third way a review's commit attribution misleads, alongside the stale caption and the workflow_dispatch head_sha already in fully-clean.md. Unlike those two it points the confident direction: the field is a live pointer to the PR, so it reports the current head whatever the run read. Also names the near-miss for a PR-body staleness finding: a correction comment leaves the body's false text standing, so the next reviewer re-reads it and the finding survives. --- shared/workflow/address-every-comment.md | 47 +++++++++++++++ shared/workflow/fully-clean.cases.md | 73 ++++++++++++++++++++++++ shared/workflow/fully-clean.md | 42 ++++++++++++++ 3 files changed, 162 insertions(+) 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..e744cd99 100644 --- a/shared/workflow/fully-clean.cases.md +++ b/shared/workflow/fully-clean.cases.md @@ -510,3 +510,76 @@ 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`. + +Two limits 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 the sample returned empty (10 of 10), and +none of them sat on a currently-open PR's branch, so this sample cannot +separate "dispatch runs never populate the array" from "the array empties once +the PR closes". +The branch-level split supports the second: branches whose PRs are open +returned non-empty in every case, and branches whose PRs had merged returned +empty in every case.) 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 From 5bed1d6174007c75624862b1076be4cf8424b237 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 05:11:12 +0000 Subject: [PATCH 3/4] ums: settle the dispatch-versus-closure ambiguity with a counterexample The case record stated two limits, one of which was that the 60-run sample could not separate 'dispatch runs never populate pull_requests' from 'the array empties once the PR closes'. This PR's own review dispatch (run 31357711790) is a workflow_dispatch on an open PR's branch returning a non-empty array, which falsifies the first hypothesis outright and leaves closure as the explanation. --- shared/workflow/fully-clean.cases.md | 36 ++++++++++++++++++++++------ 1 file changed, 29 insertions(+), 7 deletions(-) diff --git a/shared/workflow/fully-clean.cases.md b/shared/workflow/fully-clean.cases.md index e744cd99..c57e1262 100644 --- a/shared/workflow/fully-clean.cases.md +++ b/shared/workflow/fully-clean.cases.md @@ -573,13 +573,35 @@ 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`. -Two limits on that measurement, stated rather than smoothed over. +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 the sample returned empty (10 of 10), and -none of them sat on a currently-open PR's branch, so this sample cannot -separate "dispatch runs never populate the array" from "the array empties once -the PR closes". -The branch-level split supports the second: branches whose PRs are open +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.) +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'], [(p['number'], p['head']['sha'][:8]) for p in r['pull_requests']])" +#=> workflow_dispatch ums/pr1384-pull-requests-head-sha [(1388, 'ede6b0a9')] +``` + +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. +Note the run was dispatched with `--ref` pointing at the PR branch, so its +`head_sha` and the field agree --- which is what a correctly-dispatched review +looks like, and is why this run cannot also serve as an example of the two +diverging.) From 19fc91b61af42de2a289ad6ff53bd61c9ebdd7b7 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 10 Aug 2026 05:32:42 +0000 Subject: [PATCH 4/4] ums: correct a false agreement claim my own commit falsified Review finding on #1388: the case record asserted that run 31357711790's head_sha and pull_requests[0].head.sha agree because the run was dispatched with --ref at the PR branch, and that it therefore could not illustrate the two diverging. Both halves were false. --ref pins head_sha and does nothing to the other field, which re-resolves on every read. Pushing 5bed1d61 -- the commit carrying the sentence -- is what separated them. Live: head_sha ede6b0a9, field 5bed1d61. Also folds in the two things the finding hands over: the run is a second case record showing the field's usefulness inverting while its behaviour stays constant, and the general lesson that a claim two live fields agree has a short shelf life when the commit stating it is itself the falsifying event. The corrected reading is dated, since the field tracks the PR head and will differ on any later run. --- shared/workflow/fully-clean.cases.md | 56 +++++++++++++++++++++++++--- 1 file changed, 50 insertions(+), 6 deletions(-) diff --git a/shared/workflow/fully-clean.cases.md b/shared/workflow/fully-clean.cases.md index c57e1262..d956cfaa 100644 --- a/shared/workflow/fully-clean.cases.md +++ b/shared/workflow/fully-clean.cases.md @@ -593,15 +593,59 @@ is a `workflow_dispatch` on an **open** PR's branch, and it returns a ```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'], [(p['number'], p['head']['sha'][:8]) for p in r['pull_requests']])" -#=> workflow_dispatch ums/pr1384-pull-requests-head-sha [(1388, 'ede6b0a9')] + | 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. -Note the run was dispatched with `--ref` pointing at the PR branch, so its -`head_sha` and the field agree --- which is what a correctly-dispatched review -looks like, and is why this run cannot also serve as an example of the two -diverging.) + +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.)