Skip to content

fix(review-runs): anchor the census window on the predecessor run, not now-24h - #939

Open
tend-agent wants to merge 5 commits into
mainfrom
fix/issue-938
Open

fix(review-runs): anchor the census window on the predecessor run, not now-24h#939
tend-agent wants to merge 5 commits into
mainfrom
fix/issue-938

Conversation

@tend-agent

@tend-agent tend-agent commented Aug 10, 2026

Copy link
Copy Markdown
Collaborator

Problem

review-runs Step 1 opened its window with date -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, because token-report.sh 24 measured 24 hours back from its own later invocation.

Reproduced on this repo, independently of the reporter's measurement on max-sixty/cargo-affected. Yesterday's review-runs run started 2026-08-09T08:01:45Z; today's (31369350870) started 08: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:

2026-08-09T08:03:08Z review-reviewers 31302588340
2026-08-09T08:12:32Z ci               31302963325
2026-08-09T08:12:32Z tend-review      31302963274
2026-08-09T08:13:08Z ci               31302989157
2026-08-09T08:13:08Z tend-review      31302989224
2026-08-09T08:14:21Z tend-mention     31303039387
2026-08-09T08:16:03Z tend-mention     31303111366
2026-08-09T08:16:11Z tend-mention     31303117289

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-runs starts 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 SINCE on the predecessor's created_at so consecutive windows tile, and derive Step 2's lookback from the same anchor instead of a literal 24.

Three details beyond the reported shape:

  • status=success, not status=completed. The API's completed includes failure and cancelled, 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.
  • Clamp a stale or missing anchor at 49h. A fresh repo has no predecessor, and after an outage a week-old anchor would pull in a week of runs. 49h lets the window absorb one skipped day without unbounded growth; Step 5 already dedups findings a widened window sees twice.
  • Exclude $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:

  • Anchored form, simulating today's review-runs run: SINCE=2026-08-09T08:01:45Z — the predecessor's start, exit 0.
  • Empty-predecessor branch: falls through to the 25 hours ago default, exit 0.
  • Stale anchor (2026-08-01): clamps to the 49h floor.
  • HOURS derivation from that SINCE: 25, exit 0.

The if [[ ... ]]; then ... fi form 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-reviewers as possibly sharing the defect, unmeasured. It doesn't: it gets its window from list-recent-runs.sh, which anchors the completion window to the most recent intended cron tick rather than to now, 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 by status=completed and dropped again here — censused by nobody, and that is precisely the long-running class Step 3 exists to hunt. Step 1 now over-fetches by created and filters on updated_at, the shape list-recent-runs.sh already uses.

The over-fetch floor is 24h — a whole run lifetime, not the 6h hosted-runner 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; 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's created= 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 $SINCE in 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's HOURS silently became hours-since-midnight — 9 rather than 25 on this cron, pricing a narrower band than the literal 24 it replaced. Step 4's closedAt > "$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-since and both readers cat it back, verified across a real call boundary.


Closes #938 — automated triage

@tend-agent tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread plugins/tend-ci-runner/skills/review-runs/SKILL.md
Comment thread plugins/tend-ci-runner/skills/review-runs/SKILL.md Outdated
@tend-agent

Copy link
Copy Markdown
Collaborator Author

Good catch — the overlay does outrank Step 2 on this repo, so the fix was incomplete without it. Pushed "${HOURS:-24}" to this branch (99ec210), with a line naming where $HOURS comes from so an ad-hoc reader doesn't have to infer it from review-runs Step 1.

…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 tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread plugins/tend-ci-runner/skills/review-runs/SKILL.md Outdated
Comment thread plugins/tend-ci-runner/skills/review-runs/SKILL.md
Comment thread plugins/tend-ci-runner/skills/review-runs/SKILL.md
…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 tend-agent left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread plugins/tend-ci-runner/skills/review-runs/SKILL.md Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

review-runs: the window opens at now - 24h measured from the agent, so it starts after the predecessor run and clips the runs that predecessor triggered

1 participant