reviewer: stamp_fresh() silently no-ops on a fresh spawn's own verdict (likely read-after-write race) nf-itdf ← Beads

open priority 2 task unassigned

Twinned from https://github.com/cfsh/ninefold/issues/1176 by tools/beads/import_github.py's reconcile pass.

Design 012 T2 (#1111, PR #1172) added Dispatcher.stamp_fresh() — called right after a successful spawn(), it reads back the verdict the spawn just posted via newest_verdict_line(), computes a content-identity digest, and posts a follow-up comment stamping did=<16 hex> onto it.

Observed: on PR #1163, a genuinely fresh spawn (`./tools/pr/review_now.py 1163 — "no verdict yet") posted its REVIEWER-VERDICT` review normally, and spawn() printed #1163: done — ... and returned cleanly (exit 0, no WARN). But stamp_fresh() posted no follow-up comment at all — silent no-op.

~10 minutes later, manually calling Dispatcher(gh).stamp_fresh(1163) with the exact same inputs worked immediately and posted the stamp (https://github.com/cfsh/ninefold/pull/1163#issuecomment-5351512219). Every input it reads resolved cleanly on the retry: newest_verdict_line found the review, parsed.certifies(live_head) was true, contribution_id() computed a real digest.

One data point, not reproduced under controlled conditions — filing so it doesn't get lost, not claiming a diagnosis. stamp_fresh() has three silent early-returns (design 012 §8 T2, tools/reviewer/engine.py): no verdict found, verdict already has did=, or parsed.certifies(live_head) fails. None of them print anything, so there's no log line to distinguish "nothing to do" from "the read raced the write". Plausible cause: `gh pr view --json reviews,comments (GraphQL) reading back a review immediately after gh pr review (which post_verdict.py` uses to post it) — a read-after-write lag between the two APIs, hit only because stamp_fresh re-reads instead of using the body it just watched get posted.

If this turns out to be real and not a one-off: stamp_fresh is the ONLY path that stamps did= onto a genuinely fresh verdict (the common case); delta_reverify's restamp doesn't have this timing shape since it's triggered by a separate, later invocation. A silent no-op there means most fresh verdicts might never carry did= at all in production, undermining the whole point of T2 for anything that isn't a later restamp.

Possible fixes, unassessed: (a) a short bounded retry/poll in stamp_fresh before giving up, (b) at least a WARN print on each early-return path so a real occurrence is diagnosable from dispatch logs rather than silent, (c) pass the just-posted body through in-process instead of re-reading it back from GitHub, if spawn() can recover it.

Unclaimed — no session should treat this as active work without triage.

Dependencies

None.

Comments

No comments.

Add a comment