scripts/canonical_review.py's record is a read-modify-write of a tracked ledger with no lock, where its sibling scripts/local_review.py guards its own receipt write with held_lock and break_stale_lock.
The shape
cmd_record reads reports/canonical-review.json through read_ledger, merges the new entries into that dict, and writes the whole file back through write_ledger, then regenerates reports/canonical-review.md from the ledger it just wrote. Two record invocations overlapping lose one of the two passes: the second read happens before the first write lands, so the second write carries only its own entry merged onto the stale copy. The write_report that follows can also render counts from a ledger state that never existed on disk.
One record per unit is the shape the local-strict-review Skill's own loop suggests, so the overlapping case is reachable rather than theoretical, most obviously where a driver fans the passes out and records each as it returns.
Why it is not urgent
It fails closed. A lost record leaves that unit uncovered, so the next check refuses the push and names it, and the remedy is another pass and another record. Nothing false is recorded, and no push goes out claiming coverage nobody produced. The cost is a wasted pass rather than a wrong verdict, which is why this is filed rather than fixed in #1148.
Two ways to close it
Either reuse local_review.py's lock helpers, which already handle the stale-lock case and are tested, or make the documented loop a single invocation, since record already takes a repeatable --unit KEY=DIGEST and one call carrying every pair has no interleaving to lose. The second is smaller and removes the shape rather than guarding it, and the two are not exclusive.
How it was found
A delegated local-strict-review diff pass over #1148, which reproduced it rather than reasoning about it.
scripts/canonical_review.py'srecordis a read-modify-write of a tracked ledger with no lock, where its siblingscripts/local_review.pyguards its own receipt write withheld_lockandbreak_stale_lock.The shape
cmd_recordreadsreports/canonical-review.jsonthroughread_ledger, merges the new entries into that dict, and writes the whole file back throughwrite_ledger, then regeneratesreports/canonical-review.mdfrom the ledger it just wrote. Tworecordinvocations overlapping lose one of the two passes: the second read happens before the first write lands, so the second write carries only its own entry merged onto the stale copy. Thewrite_reportthat follows can also render counts from a ledger state that never existed on disk.One
recordper unit is the shape thelocal-strict-reviewSkill's own loop suggests, so the overlapping case is reachable rather than theoretical, most obviously where a driver fans the passes out and records each as it returns.Why it is not urgent
It fails closed. A lost record leaves that unit uncovered, so the next
checkrefuses the push and names it, and the remedy is another pass and another record. Nothing false is recorded, and no push goes out claiming coverage nobody produced. The cost is a wasted pass rather than a wrong verdict, which is why this is filed rather than fixed in #1148.Two ways to close it
Either reuse
local_review.py's lock helpers, which already handle the stale-lock case and are tested, or make the documented loop a single invocation, sincerecordalready takes a repeatable--unit KEY=DIGESTand one call carrying every pair has no interleaving to lose. The second is smaller and removes the shape rather than guarding it, and the two are not exclusive.How it was found
A delegated
local-strict-reviewdiff pass over #1148, which reproduced it rather than reasoning about it.