Skip to content

feat(gate): re-check migration-file collisions against live main before merge (#2550)#2585

Merged
JSONbored merged 3 commits into
mainfrom
feat/premerge-migration-collision-recheck
Jul 2, 2026
Merged

feat(gate): re-check migration-file collisions against live main before merge (#2550)#2585
JSONbored merged 3 commits into
mainfrom
feat/premerge-migration-collision-recheck

Conversation

@JSONbored

Copy link
Copy Markdown
Owner

Summary

  • Adds a live, opt-in premerge recheck for migrations/**-touching PRs: scripts/check-migrations.mjs only validates a PR against its own branch snapshot at CI time, so it can never see a sibling PR that merges a same-numbered migration file to the base branch in the meantime. This exact race has hit this repo 4 times (0015/0017, 0074, 0075/0076, most recently 0090 — PR feat(agent-actions): auto-close a contributor's PR over the open-PR cap (#2270) #2479 + PR fix(selfhost): cap historical PR file hydration and cache by head SHA #2527 → PR fix(migrations): grandfather the shipped 0090 duplicate #2532 grandfather patch).
  • Immediately before an agent-driven merge, gate.premergeContentRecheck (config-as-code only, off by default, mirrors gate.dryRun's no-DB-column pattern) fetches the base branch's CURRENT migrations/ filenames, unions them with this PR's own new migration files, and runs the same collision-detection logic CI uses. A live collision holds the merge — never a close — with a distinct gittensory:migration-collision label and a rebase-needed comment.
  • Config-gated AND path-gated: a PR that doesn't touch migrations/**, or a repo that hasn't opted in, pays zero extra latency/API calls.
  • The collision-detection algorithm is extracted into a shared, fs-free module (src/db/migration-collisions.ts) imported by BOTH the CI script and the live Worker-side recheck, so the two can never silently disagree about what counts as a collision. scripts/check-migrations.mjs now runs via tsx (already an established devDependency) instead of plain node, since it needs to resolve that .ts import — verified byte-identical CLI output before/after.
  • A short (180s) cross-invocation cache on the live tree fetch, keyed by repo+baseRef (shared across every PR checking the same base), so the recurring maintenance sweep doesn't re-issue the full recursive Trees API call on every ~2-minute pass for the life of an open PR.

Validation history

This went through a design workflow (4 parallel research agents → synthesis) before implementation, then a dedicated adversarial-review workflow (4 review dimensions → independent adversarial verification of every finding) before opening this PR. The adversarial pass caught — and this PR fixes — a real, confirmed correctness bug in the first draft: deriving "this PR's own migration filenames" from the shared changedPathsForGuardrail helper (which unions both a renamed file's OLD and NEW names for its own unrelated guardrail-matching purpose) meant a contributor simply renaming their own not-yet-merged migration file — including performing the EXACT renumber remediation this feature's own hold comment recommends — would self-collide or leave a stale, permanent false hold. Fixed by deriving the PR's own migration filenames directly from the changed-files list (current path only, excluding removed files). Three regression tests confirm the fix (and were verified to actually fail against the pre-fix code before being fixed).

The adversarial pass also surfaced and this PR addresses: no caching on the live Trees API call (added the 180s cross-invocation cache above), GitHub's truncated tree-response flag not being checked (now checked, fails open/inconclusive on a truncated response), and three test-coverage gaps (the prNumbers scoping filter that excludes unrelated pre-existing collisions, multiple simultaneous colliding numbers reaching the webhook level, and the real KNOWN_MIGRATION_DUPLICATES grandfather list interacting correctly at the webhook level) — all now covered.

Scope

  • The PR title follows type(scope): short summary Conventional Commit format.
  • This PR is focused (one feature: the live migration-collision recheck) and does not mix unrelated backend, UI, MCP, docs, dependency, or deploy changes.
  • This follows CONTRIBUTING.md and does not reintroduce GitHub Pages, VitePress, site/, or CNAME.
  • Linked to feat(gate): re-check migration-file collisions against live main before merge #2550.

Validation

  • git diff --check
  • npm run actionlint (via npm run test:ci)
  • npm run typecheck
  • npm run test:coverage locally — 100% branch coverage on every new/changed line in src/db/migration-collisions.ts, src/github/migration-tree.ts, and the new sections of src/queue/processors.ts / src/settings/agent-actions.ts (verified via manual coverage/lcov.info BRDA inspection); the few remaining uncovered branches in touched files are pre-existing, outside this diff.
  • npm run test:workers (via npm run test:ci)
  • npm run build:mcp / npm run test:mcp-pack (via npm run test:ci)
  • npm run ui:openapi:check (via npm run test:ci) — no OpenAPI schema change needed; gate.premergeContentRecheck follows the config-as-code-only, no-DB-column pattern already established by gate.dryRun/gate.size.mode, which are likewise absent from the OpenAPI schema (a pre-existing gap tracked separately under fix(openapi): add 11 missing RepositorySettings fields + a structural-drift CI check #2556).
  • npm run ui:lint / ui:typecheck / ui:test / ui:build (via npm run test:ci)
  • npm audit --audit-level=moderate
  • New/changed behavior has unit/integration tests for new branches, fallback paths, and sanitizer boundaries: pure-function tests (test/unit/migration-collisions.test.ts, test/unit/migration-tree.test.ts), planner short-circuit tests (test/unit/agent-actions.test.ts), config-as-code parse/round-trip/resolve tests (test/unit/focus-manifest.test.ts), the refactored CI script's existing test suite re-verified (test/unit/check-migrations-script.test.ts), and 16 webhook-level integration tests in test/unit/queue.test.ts covering: hold-on-collision, no-collision-merges, path-gate-skip, off-by-default-skip, fetch-failure-fail-open, no-base-ref-fail-open, cross-pass caching, corrupt-cache fallback, the prNumbers unrelated-collision exclusion, multiple simultaneous collisions, the real grandfathered-pair non-interference, and the 3 rename/removed-file regression tests for the bug the adversarial review caught.

Safety

  • No secrets, wallet details, hotkeys, coldkeys, user PATs, private keys, raw trust scores, private rankings, or private maintainer evidence are exposed.
  • Public GitHub text (the hold comment) stays sanitized and only names public information (migration filenames, which are already public in the repo).
  • No auth/cookie/CORS/GitHub App/Cloudflare/session changes.
  • API/OpenAPI behavior unchanged (see Validation note above on why no schema update is needed here).
  • No UI changes — backend/gate-only.
  • UI Evidence — not applicable (no visible UI change).
  • Docs updated (.gittensory.yml.example documents the new gate.premergeContentRecheck key); no changelog edit.

Notes

…re merge (#2550)

scripts/check-migrations.mjs only validates a PR against its own branch
snapshot at CI time; it can never see a sibling PR that merges a
same-numbered migration file to the base branch in the meantime. That exact
race hit this repo four times (0015/0017, 0074, 0075/0076, 0090). This adds
a live, opt-in recheck (gate.premergeContentRecheck) immediately before an
agent-driven merge: fetch the base branch's current migrations/ filenames,
union them with this PR's own new migration files, and run the same
collision-detection logic CI uses. A live collision holds the merge with a
distinct gittensory:migration-collision label and a rebase-needed comment
instead of merging blind. Config-gated (off by default) and path-gated (only
a PR touching migrations/** pays the extra GitHub API call), with a short
cross-invocation cache so the recurring maintenance sweep doesn't re-fetch
the same repo+base-ref on every pass.

The collision-detection algorithm is extracted into a shared, fs-free
module (src/db/migration-collisions.ts) that both the CI script and the
live Worker-side recheck import, so the two can never silently disagree
about what counts as a collision; scripts/check-migrations.mjs now runs via
tsx instead of plain node so it can resolve that .ts import.
@dosubot dosubot Bot added the size:L label Jul 2, 2026
@loopover-orb

loopover-orb Bot commented Jul 2, 2026

Copy link
Copy Markdown

Warning

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

⏸️ Gittensory review result - manual review recommended

Review updated: 2026-07-02 11:30:31 UTC

15 files · 1 AI reviewer · no blockers · readiness 82/100 · CI green · clean

⏸️ Suggested Action - Manual Review

  • Touches a guarded path — held for manual review

Review summary
The change adds a config-gated live migration filename collision check before automated merges and correctly shares the collision detector with the existing CI script. The visible wiring covers config parsing, effective settings, planner labeling, live GitHub tree reads, and focused tests for the main collision and rename/removal cases. I do not see a reachable production break in the provided diff, but the new live-tree helper and one cache-regression test should be tightened.

Nits — 6 non-blocking
  • nit: src/github/migration-tree.ts:35 treats a 200 response with a missing or non-array `tree` field as an empty migration list even though the function comment says malformed bodies return `null`; validate `Array.isArray(body?.tree)` and return `null` for malformed responses.
  • nit: test/unit/queue.test.ts:5825 models the uncached race by processing the same PR again after a successful merge, which does not match the real sibling-PR race; use two open PRs on the same base so the test proves cache freshness on the reachable path.
  • nit: src/queue/processors.ts:1878 performs the live tree fetch for any migrations-touching PR with the flag enabled, before proving the planner would attempt a merge; consider gating this closer to merge eligibility to keep the feature's premerge cost and labels limited to actual merge candidates.
  • src/github/migration-tree.ts:35: change the response parsing to return `null` when `body` is not an object or `body.tree` is present but not an array.
  • test/unit/queue.test.ts:5825: rewrite the uncached regression as PR A/PR B or a non-merged first pass, then assert the second live read observes the changed base tree before PR B merges.
  • 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 ✅ No-issue rationale PR body explains why no issue is linked.
Related work ⚠️ 2 scoped overlaps Top overlaps are listed below; lower-confidence bulk is hidden.
Change scope ❌ 8/20 High review scope from cached public metadata (size label size:L; no linked issue context).
Validation posture ✅ 25/25 PR body includes validation/test evidence.
Contributor workload ✅ 10/10 Author activity: 65 registered-repo PR(s), 55 merged, 553 issue(s).
Contributor context ✅ Confirmed Gittensor contributor JSONbored; Gittensor profile; 65 PR(s), 553 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: 65 PR(s), 553 issue(s).
  • Related work: Titles/paths share 11 meaningful terms. (issue #2550)
  • Related work: Titles/paths share 6 meaningful terms. (issue #2550, issue #2563)
Contributor next steps
  • Treat this as maintainer-lane context rather than normal contributor-lane activity.
  • Review top overlaps.
  • Add a concise scope and risk note.
  • No action.
  • Link the issue being solved, or explicitly explain why this is a no-issue PR.
  • Check active issues and PRs before submitting.
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 gittensor gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier. labels Jul 2, 2026
@JSONbored JSONbored self-assigned this Jul 2, 2026
@codecov

codecov Bot commented Jul 2, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.99%. Comparing base (ea07de7) to head (6920402).
⚠️ Report is 11 commits behind head on main.
✅ All tests successful. No failed tests found.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2585      +/-   ##
==========================================
+ Coverage   95.98%   95.99%   +0.01%     
==========================================
  Files         229      231       +2     
  Lines       25810    25879      +69     
  Branches     9389     9415      +26     
==========================================
+ Hits        24774    24843      +69     
  Misses        425      425              
  Partials      611      611              
Files with missing lines Coverage Δ
src/db/migration-collisions.ts 100.00% <100.00%> (ø)
src/github/migration-tree.ts 100.00% <100.00%> (ø)
src/queue/processors.ts 92.18% <100.00%> (+0.08%) ⬆️
src/settings/agent-actions.ts 94.24% <100.00%> (+0.12%) ⬆️
src/signals/focus-manifest.ts 99.26% <100.00%> (+<0.01%) ⬆️
🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

…ale pre-merge snapshot (#2550)

The 180s TTL cache in cachedListMigrationFilenamesAtRef was keyed by repo+baseRef
without the base SHA, so a snapshot taken before a sibling PR merged its own
colliding migration could be served to a later-processed PR within the TTL
window — silently defeating the exact race this recheck exists to catch. The
live tree fetch is cheap and already rides the existing GitHub rate-limit
admission/backoff path, so correctness wins over the saved call.
Comment thread scripts/check-migrations.mjs Outdated
@superagent-security superagent-security Bot added the pr:flagged PR flagged for review by security analysis. label Jul 2, 2026
…heck and harden the tsx shebang

Renaming an already-merged base migration (same number, e.g. a typo fix)
unioned both the old name (still live on main pre-merge) and the new name,
self-colliding even though the merged tree would only ever have one file at
that number. Now the PR's own removed/renamed-away base filenames are
subtracted from the live tree before the union.

Also harden scripts/check-migrations.mjs's shebang with --no-install so a
missing local tsx fails closed instead of npx silently fetching an
unverified package from the registry, per the security scanner finding.
@superagent-security superagent-security Bot removed the pr:flagged PR flagged for review by security analysis. label Jul 2, 2026
@JSONbored
JSONbored merged commit 3d34fb4 into main Jul 2, 2026
12 checks passed
@JSONbored
JSONbored deleted the feat/premerge-migration-collision-recheck branch July 2, 2026 11:34
@github-actions github-actions Bot mentioned this pull request Jul 2, 2026
12 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

gittensor:feature Gittensor-scored feature linked to a feature issue — scores a 0.25x multiplier.

Development

Successfully merging this pull request may close these issues.

feat(gate): re-check migration-file collisions against live main before merge

1 participant