Skip to content

fix(review): scope merge-train FIFO to overlapping siblings, fix dead persistence#4068

Merged
JSONbored merged 1 commit into
mainfrom
fix/merge-train-overlap-and-persistence-bug
Jul 7, 2026
Merged

fix(review): scope merge-train FIFO to overlapping siblings, fix dead persistence#4068
JSONbored merged 1 commit into
mainfrom
fix/merge-train-overlap-and-persistence-bug

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

Two fixes to the merge-train FIFO gate (#selfhost-merge-train), both surfaced while working through the rollout decision for mergeTrainMode:

  • Overlap-scoping (design fix): the gate previously blocked a merge behind any older open sibling in the repo, which meant a PR sitting in manual review could stall unrelated PRs indefinitely. It now only blocks behind a sibling that shares a linked issue or a meaningful changed file (lockfiles and build-output directories excluded from counting) — the actual conflict-prevention purpose FIFO ordering exists for. An unrelated older sibling never blocks, even one stuck in manual review; a genuinely overlapping one still blocks, since that's exactly the case FIFO is meant to protect against.
  • Persistence bug (correctness fix): mergeTrainMode was computed correctly in upsertRepositorySettings's resolved object but was missing from both the INSERT values and UPDATE set column lists, so it silently never reached the database on either write path and always read back as the SQL default off. This made the setting completely un-settable via the normal settings-write path since it was introduced.

Test plan

  • npm run typecheck clean
  • merge-train.test.ts (25 tests, 100% branch coverage on the pure decision function) + agent-action-executor.test.ts + agent-approval-queue.test.ts + new repository-settings-merge-train-mode.test.ts (regression-guards both the INSERT-path and UPDATE-path persistence bug separately) + full queue.test.ts (954 tests total across the 5 files) all pass on a fresh worktree rebased on latest origin/main
  • Confirmed via lcov line/branch cross-reference that every added line is covered (either directly or as part of an already-covered enclosing statement)

… persistence

The merge-train gate blocked a merge behind ANY older open sibling, so a PR
stuck in manual review could stall unrelated PRs indefinitely. Scope the
block to siblings that share a linked issue or a meaningful changed file
(lockfiles/build output excluded) -- the actual conflict-prevention purpose
of FIFO ordering, since an unrelated older sibling never blocks regardless
of its review state.

Also fixes a critical, previously-shipped bug: mergeTrainMode was computed
correctly in upsertRepositorySettings but omitted from both the INSERT
values and UPDATE set column lists, so it silently never reached the DB on
either write path and always read back as the SQL default 'off'.
@superagent-security

Copy link
Copy Markdown
Contributor

Superagent didn't find any vulnerabilities or security issues in this PR.

@codecov

codecov Bot commented Jul 7, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 93.60%. Comparing base (b679697) to head (32d1ff7).
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@           Coverage Diff           @@
##             main    #4068   +/-   ##
=======================================
  Coverage   93.60%   93.60%           
=======================================
  Files         381      381           
  Lines       35627    35647   +20     
  Branches    13070    13076    +6     
=======================================
+ Hits        33347    33368   +21     
  Misses       1618     1618           
+ Partials      662      661    -1     
Files with missing lines Coverage Δ
src/db/repositories.ts 96.69% <ø> (+0.05%) ⬆️
src/queue/processors.ts 94.95% <ø> (ø)
src/review/merge-train.ts 100.00% <100.00%> (ø)
src/services/agent-action-executor.ts 96.95% <100.00%> (+0.10%) ⬆️
src/services/agent-approval-queue.ts 99.17% <ø> (ø)
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@loopover-orb loopover-orb Bot added the gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. label Jul 7, 2026
@loopover-orb

loopover-orb Bot commented Jul 7, 2026

Copy link
Copy Markdown
Contributor

Warning

🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-07 22:57:39 UTC

9 files · 1 AI reviewer · 2 blockers · readiness 93/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.

Review summary
This PR makes two independent fixes to the merge-train FIFO gate: (1) scopes the FIFO block to siblings that actually overlap via a shared linked issue or a shared non-lockfile/non-generated changed file, instead of blocking behind any older open PR in the repo, and (2) fixes a dead-write bug where `mergeTrainMode` was computed in the `resolved` settings object but omitted from both the INSERT and UPDATE column lists in `upsertRepositorySettings`, so it never persisted. Both fixes trace correctly to their real cause — the overlap filter is inserted into the existing filter chain in `shouldWaitForOlderSiblings` after the `isOlder`/dirty checks, and the persistence fix adds `mergeTrainMode` to exactly the two write-path object literals that were missing it. Test coverage is thorough: the pure-function tests cover issue-only overlap, file-only overlap, lockfile/dist exclusion, missing-data degradation, and the new `repository-settings-merge-train-mode.test.ts` regression-tests the INSERT and UPDATE paths separately plus an invalid-DB-value fail-closed case.

Nits — 5 non-blocking
  • This PR bundles two logically separate fixes (overlap-scoping behavior change + dead persistence bug) in one diff — consider whether the maintainer wants these split, since one is a behavior/design change and the other is a pure correctness fix with different risk profiles.
  • The PR references internal tags (`#selfhost-merge-train`, `#selfhost-merge-train-overlap`) rather than a linked GitHub issue number; confirm there's an eligible tracked issue this closes per repo convention.
  • `overlaps()` in src/review/merge-train.ts builds `new Set(sibling.changedFiles)` on every call inside the sibling filter chain — fine at current sibling-list sizes, but if this ever runs over a large open-PR count it's a minor repeated allocation (micro-perf, not a blocker).
  • The external brief flags a magic number in merge-train.ts (the low-signal path lists) — consider naming `LOW_SIGNAL_FILENAME_RE`/`LOW_SIGNAL_DIR_RE` entries via a shared constant if this list needs to grow, to avoid drift from `review-grounding.ts`'s `diffFilePriority` classification it was ported from.
  • Confirm `PullRequestRecord.linkedIssues` and `.changedFiles` (referenced via `pr.linkedIssues`/`pr?.linkedIssues` in processors.ts and agent-approval-queue.ts) are populated on the live webhook path and not just in test fixtures — the diff assumes these are already reliably resolved upstream.

Concerns raised — review before merging

  • No linked issue detected — If this PR is intended to solve an issue, link it explicitly in the PR body.
  • Maintainer requires a linked issue — Link the relevant issue (for example Closes #123) before opening the PR.
Signal Result Evidence
Code review ❌ 2 blockers 1 reviewer
Linked issue ⚠️ Missing No linked issue or no-issue rationale found.
Related work ✅ No active overlap found No same-issue or scoped active PR overlap found.
Change scope ✅ 20/20 Low review scope from cached public metadata (no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 51 registered-repo PR(s), 43 merged, 503 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 51 PR(s), 503 issue(s).
Gate result ❌ Blocking Repo-configured hard blocker found.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository is configured for direct PR review.
  • Public profile languages: Python, TypeScript, JavaScript, Ruby, Go, Kotlin, MDX, Shell
  • Official Gittensor activity: 51 PR(s), 503 issue(s).
  • PR-specific overlap: none found.
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Explain no-issue PR.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
Signal definitions
  • Related work = same linked issue, overlapping active PRs, or title/path similarity.
  • Change scope = cached public metadata such as size labels, draft state, and review-burden hints.
  • Validation posture = whether the PR provides enough public validation/test evidence for maintainer review.
  • Contributor workload = public contributor activity and cleanup pressure, not a repo-wide quality failure.
  • Contributor context = public GitHub/Gittensor identity context; non-Gittensor status is not a blocker.

🟩 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 Gittensory, a quiet PR intelligence layer for OSS maintainers.

  • Re-run Gittensory review

@loopover-orb loopover-orb Bot added the manual-review Gittensor contributor context label Jul 7, 2026
@JSONbored
JSONbored merged commit 5ad3eca into main Jul 7, 2026
10 checks passed
@JSONbored
JSONbored deleted the fix/merge-train-overlap-and-persistence-bug branch July 7, 2026 23:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:bug Gittensor-scored bug fix — scores a 0.05x multiplier. manual-review Gittensor contributor context

Development

Successfully merging this pull request may close these issues.

1 participant