ums: pull_requests[].head.sha tracks the PR, not the run - #1388
Conversation
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.
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.
|
Triage of the red It belongs to run 31357711790, whose conclusion is A fresh review is already running -- run Worth recording that this PR's own subject showed up in its diagnosis. The cancelled run's
Those disagree for exactly the reason this PR documents: the second field is a live pointer to the pull request rather than a record of what the run read. Here it happens to be the more useful of the two, since it correctly says the PR has moved past the commit whose check went red -- which is what makes the red check stale rather than actionable. On #1384 the same disagreement pointed the other way and made a superseded verdict look current. The field is not wrong in either case; it simply answers a different question than the one a reader asks of it, which is the entry's whole point. Other checks at the current head are green: Continuing to watch until this merges or closes. Generated by Claude Code |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
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 5bed1d6 -- the commit carrying the sentence -- is what separated them. Live: head_sha ede6b0a, field 5bed1d6. 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.
ARD round 1 --- 1 finding, AddressedPushed Addressed: the
|
| Figure | ede6b0a9 |
5bed1d61 |
19fc91b6 |
|---|---|---|---|
| Added lines | 162 | 184 | 228 |
fully-clean.cases.md |
512 -> 585 | 512 -> 607 | 512 -> 651 |
| Relative links | 1336 | 1336 | 1337 |
markdownlint-cli2 0 issues, check-new-line-breaks clean at pinned
209bfb76, memory size gate clean, 0 broken links, 0 banned punctuation in 228
added lines.
Generated by Claude Code
|
Claude finished review — View run Code reviewThis round contains exactly one commit since the last review round (
The fix fully resolves that finding, and I re-verified it carefully rather than just reading the prose:
No new issues were introduced by this fix. The one open finding from the prior round is closed. VerdictReady for merge — the sole finding from the previous review round (stale, self-falsifying "field agrees with |
Closes #1387.
UMS pass for lessons from PR #1384's review lifecycle (merged as
edfab8d8).Every figure below was derived by command at
19fc91b6, the head this PRpushes, with the deriving command published beside it.
Corrections to this body
Dogfooding the rule this PR adds to
address-every-comment.md: the bodyis edited and the correction recorded, so nothing is silently overwritten
and any earlier comment citing the old numbers still resolves.
ede6b0a95bed1d6119fc91b6(current)fully-clean.cases.md#hitsCauses:
5bed1d61recorded a counterexample that arrived mid-review;19fc91b6addresses the round-1 review finding.What landed, and where
pull_requests[].head.shais a third misleading SHA surfacefully-clean.md(rule) +fully-clean.cases.md(two case records)address-every-comment.mdaddress-every-comment.mdfind-near-duplicates.pydefaults to theskills/*/SKILL.mdcorpusskills/find-overlap/SKILL.mdlines 68 and 109Lesson 1 --- placement rationale
fully-clean.mdis auto-loaded (CLAUDE.md:443);review-verdict-pitfalls.mdand every
.cases.mdare not. So the operative rule goes in the auto-loadedfile and the measurements go in the case record.
It slots in as the third entry in an existing sequence, after "a review
comment's header SHA can be stale" and "that remedy assumes the run checked out
the PR head". The increment: the first two mislead toward staleness, which
invites a needless re-trigger; this field misleads toward currency, which is
the confident direction.
Lesson 3 --- why it was skipped
ardi.mdalready says to "treat any round thatchanges the diff as expiring every figure the body already states, not only the
figure that round was about". A self-review commit is a round that changes the
diff, so the existing rule covers the trigger completely and prescribes an
identical remedy. Adding a sharpening would be the duplicate
challenge-redundant-content.mdwarns about.
Lesson 4b --- why nothing was written
--corpusdefaults toskills/*/SKILL.md, confirmed via--help.skills/find-overlap/SKILL.md:68already states it in prose, and line 109 showsthe
--corpus 'memories/*.md'override. The session's error was not readingthat, which is not a corpus gap.
The brief's central claim did not reproduce, and I did not assert it
The brief stated that run
31354330266hadhead_sha=a0ef37c2whilepull_requests[0].head.sharead7fe25776. My unauthenticated REST readreturned
pull_requests: []--- empty. Per the brief's own instruction not totrust it, I did not record that claim.
Instead I derived a stronger, currently-reproducible finding that explains
both the brief's observation and the present emptiness: the field is a live
pointer resolved at read time, so it reports the PR's current head whatever
commit the run checked out, and the array empties once the PR closes.
#=> examined 60; non-empty 14; field==current head 14; run head_sha differs 8The MCP
actions_getget_workflow_runomitspull_requestsentirely --- thefield the whole lesson is about --- so the measurement went through raw REST.
Round 1 review: finding Addressed
The review at
5bed1d61returned Needs more work with one finding, and itwas correct. The case record asserted that run
31357711790'shead_shaandpull_requests[0].head.shaagree because the run was dispatched with--refatthe PR branch, and that it therefore could not illustrate the two diverging.
Both halves were false, and my own commit falsified them. Verified live
rather than from the reviewer's quote:
#=> workflow_dispatch ums/pr1384-pull-requests-head-sha ede6b0a9 [(1388, '5bed1d61')]--refpinshead_shaand does nothing to the other field, which re-resolveson every read. Pushing
5bed1d61--- the commit carrying the sentence --- iswhat separated them.
Fixed on the merits in
19fc91b6, not by pasting the suggested wording. Thereviewer named one sentence; sweeping the passage found the code block's
#=>output was stale too, so both were corrected. Three things folded in:
update_triggerneeds a freshrun_once_atto re-arm a fired one-shot #1384the field pointed the confident direction and made a superseded verdict look
current. Here it points the useful direction and correctly says the PR moved
past the commit whose check went red. Same field, opposite usefulness,
neither reading wrong --- which supports the thesis better than ums:
update_triggerneeds a freshrun_once_atto re-arm a fired one-shot #1384 alone.claim that two live fields agree has a short shelf life, and where the
sentence is written into a commit, that commit is itself the falsifying
event. Carried as a Do/Don't pair.
will differ on any later run --- applying the new rule to the sentence
stating it.
fully-clean.mdneeded no change: itsDobullet already says to read thefield as a fact about the PR's current head and nothing else, which correctly
applied prevents the
--refmisconception.The red
review / require-reviewon run31357711790is acancelledrun ata superseded head, not a failure with findings (
completed/cancelled,confirmed via the API). Already triaged in a PR comment; not re-dispatched.
Verification
All at
19fc91b6, after committing, with the three-dot range. Everyline-scoped scan was re-run after the commit, since a diff-editing pass expires
them.
Diff scope (
git diff --stat origin/main...HEAD):Per-file line counts,
git show origin/main:<f>againstgit show HEAD:<f>:shared/workflow/fully-clean.mdshared/workflow/fully-clean.cases.mdshared/workflow/address-every-comment.mdAdded-line scan over
git diff -U0 origin/main...HEAD:All 4 column-1 hits are
#=>output lines inside fenced code blocks, which iswhy
markdownlint-cli2reports clean below rather than MD018.Checks:
markdownlint-cli2Linting: 499 files/Summary: 0 issues in 0 filescheck-new-line-breaks.py@ pinned209bfb76No lines missing semantic breaks.scripts/check-memory-file-size.pyNo memory file exceeds 1200 lines.scripts/check-links.pyChecked 1337 relative links across 473 markdown files/ no broken linksscripts/validate-skills.pyscripts/check-vendored-drift.pyThe line-break check ran against a
ghaclone pinned at209bfb76bd412aa10d4d56a809d791a6dc8f10df, read out ofvalidate.yml:127, soit matches what CI runs rather than a shallow
main.Dupe-check --- normalized (
re.sub(r"[`*_\s]+", " ", s).lower()) onboth needle and haystack, across 318 markdown files. Every needle is taken
from the diff's own added lines, so each is a known positive that demonstrates
the instrument fires. All six return 0 hits outside this PR's 3 files:
pull_requests[].head.sha,live pointer to the pull request,the array empties when the PR closes,the only surface separating the candidates,a correction comment does not clear it,the next reviewer re-reads the body.Test suite: 12 suites run, 2 failed ---
test_ai_session(4 cases) andtest_compare_shell_forms(1 case). Both reproduce with identical counts on aclean
origin/main, extracted viagit archive origin/main | tar -xto ascratch directory rather than mutating this worktree. Already tracked in #1382,
so no duplicate was filed. Neither suite exercises any of the three markdown
files this PR touches.
Advisory context budget:
check-context-closure.pyreports over budget bothbefore and after --- 1,192,175 bytes at
origin/mainagainst 1,196,891 atede6b0a9.CLAUDE.mdis untouched at 82,438 characters (55.0% of theharness's 150,000-char cap); both later commits touch only the
non-auto-loaded
.cases.md, so they add nothing to the loaded budget.