Skip to content

fix(gate): recheck migration collisions at merge#2763

Closed
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-migration-collision-check-in-merge-process
Closed

fix(gate): recheck migration collisions at merge#2763
JSONbored wants to merge 1 commit into
mainfrom
codex/fix-migration-collision-check-in-merge-process

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Motivation

  • The live pre-merge migration collision check ran only during planning, leaving a TOCTOU window where a sibling PR could land a colliding migration before the staged/approved merge executed.
  • The intent is to ensure the same fresh-tree collision logic used in CI/planning is enforced at the final actuation boundary (including approval-queue replay) to prevent landing duplicate migration numbers.

Description

  • Extracted the filename-extraction and live-collision logic into a shared helper src/services/migration-collision-recheck.ts with migrationFilenamesForLiveRecheck and resolveLiveMigrationCollisionHold so planning and actuation use identical logic.
  • Updated the planner (src/queue/processors.ts) to call the shared helper instead of its private copy of the logic so the two paths stay consistent.
  • Added an actuation-time recheck in the executor (src/services/agent-action-executor.ts) that, for merge actions, refreshes the PR's changed files and the base tree and re-runs the live collision check; when a collision is found the executor applies the existing gittensory:migration-collision label + comment and denies the merge instead of executing a stale plan.
  • Added unit tests: test/unit/migration-collision-recheck.test.ts for helper behavior and a targeted regression in test/unit/agent-approval-queue.test.ts proving a staged merge is denied when a sibling migration lands before approval, plus a fast-path test that skips the recheck when the PR has no migration files.

Testing

  • Ran the focused unit suites covering the change: npx vitest run test/unit/agent-approval-queue.test.ts test/unit/migration-collision-recheck.test.ts and npx vitest run test/unit/agent-approval-queue.test.ts test/unit/agent-action-executor.test.ts test/unit/migration-collisions.test.ts test/unit/migration-tree.test.ts test/unit/migration-collision-recheck.test.ts and they passed locally (all related tests green).
  • Verified git diff --check (no whitespace/conflict markers) and the modified tests exercise collision/no-collision and fail-open behavior.
  • npm run typecheck could not be completed cleanly due to pre-existing unrelated test-fixture type errors in test/unit/queue.test.ts (missing securityFocus fixture field), not caused by this change.
  • npm audit --audit-level=moderate and the full npm run test:ci were blocked by network/registry constraints in the environment (npm audit returned a 403 Forbidden and actionlint setup hit network resolution issues), so those checks were not completed here.

Codex Task

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

loopover-orb Bot commented Jul 3, 2026

Copy link
Copy Markdown

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-03 19:42:20 UTC

5 files · 1 AI reviewer · no blockers · readiness 93/100 · CI green · dirty

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The extraction preserves the existing planner collision logic and reuses it from the merge executor, which is the right direction for closing the staged-action stale-decision path. The executor now gates the live base-tree check behind both the migration-file fast path and premergeContentRecheck, then denies the merge and records the existing collision label/comment path when a PR-number collision is found. The visible diff is coherent and covered by targeted helper plus approval-queue tests; the remaining concerns are non-blocking around documentation and test breadth for fail-open behavior.

Nits — 6 non-blocking
  • nit: src/services/agent-action-executor.ts:174 silently skips the actuation recheck when createInstallationToken fails, so add a focused test documenting that intentional fail-open behavior rather than leaving it implicit.
  • nit: src/services/agent-action-executor.ts:171 relies on stored pull_request_files for the PR-side filenames; the PR description says the executor refreshes changed files, so either adjust the description or add a pointer to the existing freshness check that makes the stored file list safe here.
  • nit: src/services/migration-collision-recheck.ts:7 lost several useful explanatory comments from the old in-file helper about why the union and removed-base subtraction are load-bearing, and keeping a shortened version near migrationFilenamesForLiveRecheck would make future edits safer.
  • Add one executor test where createInstallationToken rejects and confirm the merge path proceeds without calling listMigrationFilenamesAtRef or without denying, depending on the intended fail-open contract.
  • Tighten the PR description or implementation around “refreshes the PR's changed files,” because this diff reads them from listPullRequestFiles rather than fetching them live.
  • Touches a guarded path — held for manual review — A maintainer must review and merge this change.
Signal Result Evidence
Code review ✅ No 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: 60 registered-repo PR(s), 51 merged, 463 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 60 PR(s), 463 issue(s).
Gate result ⚠️ Not blocking Advisory; not blocking this PR.
Review context
  • Author: JSONbored
  • Role context: owner (maintainer lane)
  • Public audience mode: oss maintainer
  • Lane context: Repository registration is not available in the local Gittensory cache.
  • Public profile languages: not available
  • Official Gittensor activity: 60 PR(s), 463 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.
  • No action.
  • 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

@JSONbored JSONbored self-assigned this Jul 3, 2026
@JSONbored

Copy link
Copy Markdown
Owner Author

Closing as superseded: this exact feature (live migrations/** collision recheck at merge, #2550) already shipped on main via #2585, and the specific bug this PR targets (a stale cached live-tree snapshot defeating the recheck) was already fixed there too, in dce0fad5 ("fix(gate): remove the live migration-tree cache — it could serve a stale pre-merge snapshot"). Rebasing this branch and merging it as-is duplicates the live-tree fetch at a second call site (actuation time, on top of main's existing planning-time check), which is a real regression, not a fix — confirmed locally by queue.test.ts's treeCalls assertion going from 2 to 3 after reconciling the rebase conflict. No further action needed here.

@JSONbored JSONbored closed this Jul 3, 2026
@JSONbored
JSONbored deleted the codex/fix-migration-collision-check-in-merge-process branch July 4, 2026 19: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.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant