fix(review-runs): anchor the census window on the predecessor run, not now-24h - #939
fix(review-runs): anchor the census window on the predecessor run, not now-24h#939tend-agent wants to merge 5 commits into
Conversation
tend-agent
left a comment
There was a problem hiding this comment.
The anchoring itself is right, and I ran both recipes against the live API to check them rather than reading them: status=success&per_page=10 returns newest-first with created_at on each run, max // empty yields a bare timestamp (gh's --jq is raw, so no quote-stripping needed), the [[ "$SINCE" < "$FLOOR" ]] lexicographic compare is sound because both sides are fixed-width ISO-8601 UTC, and the HOURS arithmetic resolved to 25 from yesterday's anchor. The stale-anchor and empty-predecessor branches both land where the description says. #931 touches this file too but at Steps 4–5, so no conflict.
One gap outside the diff, and it lands on this repo specifically. .claude/skills/running-tend/SKILL.md (the "Usage analysis" section, the token-report.sh line) still passes a literal 24. Per running-in-ci's "where the two conflict, the repo wins", that overlay outranks the bundled Step 2 — so on tend itself, the repo whose runs are the evidence in the description, an agent can still price the band with a hardcoded 24 and reopen exactly the gap Step 2 just closed. Making it "${HOURS:-24}" keeps the ad-hoc invocation working (where HOURS is unset) while letting the derived value win under review-runs. Happy to push that to this branch if you'd rather it land together than as a follow-up.
|
Good catch — the overlay does outrank Step 2 on this repo, so the fix was incomplete without it. Pushed |
…en windows The heading said 'completed since the previous run' but the query filtered created>=$SINCE. A run created before the anchor and still in progress at the predecessor's census was dropped there by status=completed and dropped again here — never censused by anyone, and that is exactly the long-running class Step 3 hunts for. Over-fetch by created and filter on updated_at, the shape list-recent-runs.sh already uses. The floor is 24h, a whole run lifetime rather than the 6h job cap: created_at starts at queue time, and a cancel-in-progress: false group can hold a run queued for hours before execution begins. Also trims Step 1's lead comment per CLAUDE.md's brevity rule.
tend-agent
left a comment
There was a problem hiding this comment.
The completion-axis rewrite is right and I checked it rather than read it: FETCH_FROM=$(date -u -d "$SINCE - 24 hours" ...) parses (GNU date takes the signed relative item after a timestamp), the updated_at >= $SINCE filter tiles against the predecessor's window with a small overlap rather than a gap, and a run re-entering the window on a later updated_at bump is what Step 5 dedups. The 24h over-fetch floor and the created= time-of-day check are both settled by the evidence in the other thread.
One new problem, introduced by Step 2's rewrite. $SINCE is set inside Step 1's code block and read inside Step 2's — a different Bash tool call, and shell state doesn't survive between them (running-in-ci: "Shell state doesn't persist between tool calls — re-derive REPO in each bash invocation or combine commands"). The two blocks are separated by the per-run jobs query and its classification, so the boundary is guaranteed, not incidental.
That would be a loud failure if date rejected the empty string. It doesn't — GNU date -d "" resolves to today at 00:00 UTC, so the arithmetic succeeds and HOURS silently becomes hours-since-midnight. This workflow's cron is 47 7 * * *, so the derived lookback lands around 8 instead of ~24, and Step 2 prices a narrower band than the literal 24 it replaced — the same gap this PR exists to close, reopened wider and with no symptom at the point of failure. Measured in this session:
$ export SINCE_PROBE=2026-08-09T08:01:45Z # call A
$ echo "[${SINCE_PROBE:-<unset>}]" # call B
[<unset>]
$ date -u -d "" +%Y-%m-%dT%H:%M:%SZ
2026-08-10T00:00:00Z
$ HOURS=$(( ( $(date -u +%s) - $(date -u -d "" +%s) + 3599 ) / 3600 )) # at 08:54Z
9
The overlay's "${HOURS:-24}" doesn't rescue this on tend: whichever way it composes, either HOURS is unset and it falls back to the literal 24 the overlay change was meant to retire, or it is set in Step 2's block to the wrong ~8.
Suggested fix inline: have Step 1 write the clamped anchor to a file and have Step 2 read it back. Re-deriving the predecessor query in Step 2 would also work but duplicates two API calls and can drift from Step 1's clamp. A bare : "${SINCE:?}" would at least make it loud, but the file keeps the two steps agreeing, which is the property the PR is after.
Knock-on, outside the diff: Step 4's outcome cross-check has the same cross-block read — gh pr list ... --jq '.[] | select(.closedAt > "'$SINCE'")', under the "check if a bot PR was merged or closed" example. With an empty $SINCE every non-null closedAt compares greater, so that check has been unbounded rather than windowed. It predates this PR, but the comment this PR adds ("Later steps read this same $SINCE") asserts the dependency holds, and the file fix repairs both at once — worth folding in here rather than leaving as a follow-up.
…n empty $SINCE Step 1 sets $SINCE in one Bash tool call; Steps 2 and 4 read it in others, where shell state is gone. Neither failed loudly: `date -d ""` is today's midnight rather than an error, so Step 2's HOURS became hours-since-midnight (9 instead of 25 on this cron) and priced a narrower band than the literal 24 it replaced. Step 4's `closedAt > "$SINCE"` compared against the empty string, admitting every closed bot PR ever — 62 instead of 10 here — so that cross-check was never windowed at all, predating this PR. Step 1 now writes the clamped anchor to /tmp/review-runs-since and both readers cat it back.
tend-agent
left a comment
There was a problem hiding this comment.
Checked the persisted anchor across a real Bash-call boundary rather than reasoning about it. Call A ran Step 1's block against this repo's tend-review-runs workflow and wrote /tmp/review-runs-since; call B saw $SINCE unset, read 2026-08-09T08:01:45Z back from the file, and derived HOURS=26. Step 4's form now windows: 8 closed bot PRs against 10 for the empty-string compare on the same gh pr list page. The echo sits after the clamp, so what's persisted is the value the census used.
One thing on the incremental itself: it's 9 comment lines for 3 lines of shell, and the second half of Step 2's block re-argues the call boundary that Step 1's comment already states two steps up. This file is loaded into every review-runs session, and the same brevity rule already cost this PR a trim at Step 1 — worth not spending it back here. Suggestion inline; the reason the cat isn't optional (the silent midnight fallback) is the part that has to survive.
Problem
review-runsStep 1 opened its window withdate -u -d '24 hours ago', which resolves when the agent runs the command — the run's start plus container boot and skill loading. The predecessor started at its own start time, earlier by whatever drift it saw, so the window opened strictly after the predecessor and dropped every run in the gap. The gap is never zero and never negative, so the census systematically under-counted rather than flaking. Step 2 clipped the same band independently, and by a wider margin, becausetoken-report.sh 24measured 24 hours back from its own later invocation.Reproduced on this repo, independently of the reporter's measurement on
max-sixty/cargo-affected. Yesterday'sreview-runsrun started2026-08-09T08:01:45Z; today's (31369350870) started08:16:33Z, so its window opened no earlier than that — a band of at least 14m48s, and wider by however long boot and skill loading took. Eight runs sat in it:Six of those are full agent sessions. The loss is also biased toward the runs that matter most to the audit: what happens in the minutes right after
review-runsstarts is largely the work that run triggers — its own PR getting reviewed, a mention firing on that review — and that is exactly the band the next run cannot see.This is distinct from #886 and #888, which decide how much of the window is visible. A fully paginated census of a window that opens too late still misses these runs, and Step 2 — the fallback that recovered the truncated runs in #888 — has the same defect.
Solution
Anchor
SINCEon the predecessor'screated_atso consecutive windows tile, and derive Step 2's lookback from the same anchor instead of a literal24.Three details beyond the reported shape:
status=success, notstatus=completed. The API'scompletedincludesfailureandcancelled, so anchoring on it would permanently strand the band of a predecessor that died before its census. Reaching for the last successful run covers that band on the next pass.$GITHUB_RUN_ID. A re-run attempt of the current run can surface as completed, and anchoring on itself collapses the window to zero. The workflow id is derived from the run rather than the file name.Testing
No test harness covers skill text, so both recipes were extracted verbatim from the edited file and executed here:
review-runsrun:SINCE=2026-08-09T08:01:45Z— the predecessor's start, exit 0.25 hours agodefault, exit 0.2026-08-01): clamps to the 49h floor.HOURSderivation from thatSINCE:25, exit 0.The
if [[ ... ]]; then ... fiform is deliberate over[[ ... ]] && SINCE=$FLOOR: the latter exits 1 when the test is false, which the agent's Bash tool reports as a failed block.`review-reviewers` — already anchored, no change needed
The report flagged
review-reviewersas possibly sharing the defect, unmeasured. It doesn't: it gets its window fromlist-recent-runs.sh, which anchors the completion window to the most recent intended cron tick rather than tonow, precisely so scheduler drift can't shift the window relative to actual start time. #845 extends that anchoring to every-N-hours crons.Review follow-ups
Two changes landed after review, both on this branch.
The census is now on completion, not creation. Anchoring alone made the creation windows tile, but Step 1's heading promised completed-since-the-predecessor and the query still filtered
created>=$SINCE. A run created before the anchor and still in progress at the predecessor's census was dropped there bystatus=completedand dropped again here — censused by nobody, and that is precisely the long-running class Step 3 exists to hunt. Step 1 now over-fetches bycreatedand filters onupdated_at, the shapelist-recent-runs.shalready uses.The over-fetch floor is 24h — a whole run lifetime, not the 6h hosted-runner job cap.
created_atstarts at queue time, and acancel-in-progress: falsegroup can hold a run queued for hours before execution begins; the longest completed run on this repo in the last three days spans 819 minutes wall-clock (31281456692), which a 6h floor still misses. Diffing the two floors against the live API in one pass, 24h is a strict superset that recovers exactly that run (242 -> 243). Also confirmed GitHub'screated=filter honors time-of-day rather than rounding to the date, which the sub-day anchor depends on.Step 1's lead comment is trimmed per CLAUDE.md's skill-authoring brevity rule; the argument lives here, not in a file loaded into every session.
The anchor is now persisted to a file. Step 1 set
$SINCEin one Bash tool call and Steps 2 and 4 read it in others, where shell state is gone. Neither failed loudly:date -d ""resolves to today's midnight rather than erroring, so Step 2'sHOURSsilently became hours-since-midnight — 9 rather than 25 on this cron, pricing a narrower band than the literal24it replaced. Step 4'sclosedAt > "$SINCE"compared against the empty string, which sorts below every timestamp, so that cross-check admitted every closed bot PR ever (62 here against 10 for the correct anchor) and had never been windowed at all — that half predates this PR. Step 1 now writes the clamped anchor to/tmp/review-runs-sinceand both readerscatit back, verified across a real call boundary.Closes #938 — automated triage