fix(train): tell the contributor when the merge train defers a merge the surface already announced - #9840
Conversation
…the surface already announced Observed live on #9837: the published panel said the PR was MERGING -- the planner's disposition legitimately concluded wouldMerge before the executor's train check ran -- and then the enforce-mode train denial recorded itself AUDIT-ONLY. Publicly the PR claimed an action that never happened, which reads as the bot silently breaking its word (and, to an operator, as the old lost-merge bug recurring when it is actually FIFO queueing working as designed: 9837 was waiting on overlapping older sibling 9833, whose CI was still running). Post one comment naming the blocker and what happens next. Dedup via the AUDIT TRAIL, not a transient lock: claimTransientLock fails OPEN on any deployment without a transient cache bound, which would repeat the comment on every denial pass. The comment's own audit row (recorded only after a successful post, so a failed post retries next pass) is the durable "did we say this" fact everywhere, keyed per (waiting PR, blocker) so a NEW blocker gets its own line and the SAME blocker never repeats within the 24h train window.
|
Warning ⏸️ LoopOver review result - manual review recommendedReview updated: 2026-07-29 14:55:10 UTC
Review summary Nits — 5 non-blocking
Concerns raised — review before merging
📋 Copy for AI agents — paste into your coding agentDecision drivers
Context & advisory signals — never blocks the verdict
Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. Decision record
🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #9840 +/- ##
==========================================
- Coverage 91.65% 90.77% -0.88%
==========================================
Files 916 916
Lines 112763 112773 +10
Branches 27087 27089 +2
==========================================
- Hits 103348 102367 -981
- Misses 8126 9315 +1189
+ Partials 1289 1091 -198
Flags with carried forward coverage won't be shown. Click here to find out more.
|
Codecov flagged the two remaining lines: the branch where createIssueComment REJECTS. That path is the whole reason the audit row is written after the post rather than before -- a transient GitHub failure must leave no row, so the next pass retries instead of believing the contributor was told. Pins all three states: a failed post still denies and records nothing, the next pass retries, and once it succeeds it stops.
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
loopover-ui | 4b9ced7 | Commit Preview URL Branch Preview URL |
Jul 29 2026, 02:39 PM |
Codecov's last uncovered line was the .catch() on the dedup row's own write. It is deliberately fail-open: if that best-effort write fails, the worst case is one duplicate comment on a later pass, and the denial plus merge suppression must be unaffected. Pinned by failing ONLY that event type, so the denial's own audit write still succeeds and the test proves the branch rather than the mock.
What the maintainer saw
#9837: fully approved, green, panel said it was merging — then nothing. It looked exactly like the lost-merge class we already fixed.
What actually happened (not that bug)
The merge train working as designed: #9833 overlaps #9837 and is older, so FIFO queues #9837 behind it; #9833's CI was still running. When #9833 completes,
maybeEnqueueSiblingRegateForMergedPrwakes the waiter, and #9483 already covers the blocker-closed/aged-out/evicted cases.The real defect is honesty: the planner's disposition legitimately concludes
wouldMergebefore the executor's step-8 train check runs, so the published surface announces a merge — and the enforce-mode denial then records itself audit-only. Publicly the PR claims an action that never happened. A contributor reads a broken promise; an operator reads a regression.The fix
On an enforce-mode train denial, post one comment: queued behind #N (overlapping work, opened first), merges automatically when it clears, no action needed.
Dedup via the audit trail, deliberately not a transient lock. The first implementation used
claimTransientLockand the regression test caught why that's wrong: it fails open ({acquired: true}) on any deployment without a transient cache bound, which would repeat the comment on every denial pass — every few minutes, for up to 24 hours. The comment's own audit row is the durable "did we already say this" fact on every deployment, keyed per (waiting PR, blocker) so a new blocker gets its own line and the same blocker never repeats within the train window. The row is recorded only after a successful post, so a failed post retries instead of silently believing the contributor was told.Tests
Regression: enforce-mode denial posts exactly one comment naming the blocker; a second pass behind the same blocker denies again but does not repeat it; the merge is never called. Full suite: 25,289 passed.
dead-source-files,command-redelivery-guards,dispatch-gate-reasonsgreen.