feat(gate): force a fresh rebase + CI recheck immediately before merge when base has advanced#2616
Conversation
|
Warning 🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨🟨 ⏸️ Gittensory review result - manual review recommendedReview updated: 2026-07-02 17:07:24 UTC
⏸️ Suggested Action - Manual Review
Review summary Nits — 7 non-blocking
Review context
Contributor next steps
Signal definitions
🟩 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.
|
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #2616 +/- ##
=======================================
Coverage 96.02% 96.03%
=======================================
Files 233 233
Lines 26077 26108 +31
Branches 9474 9484 +10
=======================================
+ Hits 25041 25072 +31
Misses 425 425
Partials 611 611
🚀 New features to boost your workflow:
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
gittensory-ui | ecdddee | Commit Preview URL Branch Preview URL |
Jul 02 2026, 02:17 PM |
A successful forced update_branch produces a new head SHA, which reset a headSha-keyed counter back to attempt 0 on every force — making the MAX_FRESH_REBASE_FORCES cap unreachable via the exact path it exists to bound. Key by repo+PR number only; a 24h TTL still gives an eventual fresh start.
ecdddee to
2256b8f
Compare
Two issues found by the gate's own AI review on PR #2616: 1. The freshness check ran before planAgentMaintenanceActions and only checked mergeableState === "clean" (a git-level textual-conflict signal). A PR with pending CI, a gate blocker, or missing approval could still be git-clean, so it could burn the bounded retry cap on rebases nobody was about to act on -- exhausting it before the PR was ever actually merge-eligible. Moved the check to run after the full plan (gate/CI/blockers/precision breakers) resolves, and gated it on the plan containing a merge action that would execute now (requiresApproval stages for a human, not an immediate merge). 2. maintainerSettingsSchema in src/api/routes.ts omitted requireFreshRebaseWindowMinutes, so a dashboard/API PUT for the new setting was silently stripped by validation and never reached upsertRepositorySettings despite the DB/repository/OpenAPI wiring being complete. Added the field plus a round-trip test.
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
The AI review flagged a real defect: cachedFetchLivePullRequestMergeState
/ cachedFetchLivePullRequestState / cachedFetchLivePullRequestHeadSha
each wrote through only the ONE field they cared about, but all three
share a single prStateFetchedAt freshness stamp. A live fetch from any
one of them would make the OTHER two fields look "fresh" to a
subsequent reader despite never having been fetched, so that reader
would silently return undefined for a field that was simply never
populated -- indistinguishable from a confirmed-empty GitHub value.
All three narrow live-fetchers already hit the exact same GET
/pulls/{n} endpoint, just extracting one field each, so there's no
extra API cost to fixing this: a new internal fetchAndCachePrStateFields
helper fetches the full payload once and writes mergeable_state, state,
and headSha through together (headSha omitted, not nulled, when absent,
preserving the existing PARTIAL-UPDATE CONTRACT so a prior headSha the
files cache depends on is never cleared). It also only writes when the
fetch actually succeeds, so a transient failure no longer poisons the
cache with a false "confirmed fresh" stamp the way the old per-field
writes did. The three public, uncached narrow fetchers used by the
act-boundary/gate-override callers are untouched.
Also fixes a second, separate flagged issue: `{ token: "installation-token" }`
in six of this PR's own new test fixtures tripped the deterministic
generic_secret_assignment scanner (a keyword-shaped heuristic, not a
real credential format) -- renamed to `"fake-installation-token"`,
which the scanner's own placeholder-value allowlist already recognizes,
without touching the shared scanner itself.
Rebased onto current main (renumbered migration 0093->0094 to resolve
a collision with #2616, and again to catch up with #2632). Full local
gate (test:ci, npm audit) green; no other changes.
Summary
update_branchaction (which merges the current base into head and would naturally re-trigger CI on a truly-rebased branch) only fired whenliveMergeState === 'behind'inside the review-gating pass — not immediately before the merge action itself.mergeable_stateas the sole base-freshness signal, andmergeable_stateonly detects git-level TEXTUAL conflicts — a base that has advanced with a new, non-conflicting sibling commit (e.g. a second PR's distinct-but-colliding migration file) still readsclean.mergePullRequest's head-SHA pinning already closes on the PR's own side.Scope
gate.requireFreshRebaseWindowsetting (minutes, off by default), wired through the full config-as-code chain: migration0093, Drizzle schema,RepositorySettingstype, the DB round-trip inrepositories.ts,focus-manifest.ts's gate config parse/serialize/resolve, the OpenAPI schema, and.gittensory.yml.exampledocs.src/github/backfill.ts— newfetchLiveBaseBranchAdvancedAt: reads the base branch's live tip-commit timestamp viaGET /commits/{ref}.src/queue/processors.ts— the base-freshness check lives inrunAgentMaintenancePlanAndExecute, immediately beforeplanAgentMaintenanceActionsis called with merge-eligible inputs. When the base landed within the configured window,maybeForceFreshRebaseforces anupdate_branchthrough the SAME action class/write-permission/dry-run/kill-switch stackprReadyForReview's own BEHIND-branch path already uses — not a new action class, just a new trigger condition. A bounded retry cap (3 attempts, keyed per head SHA in the transient cache, mirroringciPendingDeferStuck's pattern) prevents a fast-moving base from live-locking the PR; past the cap it falls through to a normal merge with an audit trail. Every failure mode (unreadable base commit, cap reached, forced action not authorized) fails open to today's merge behavior.Safety
mergeableClean).Validation
npx vitest run test/unit/focus-manifest.test.ts test/unit/data-spine.test.ts test/unit/queue.test.ts— all passingqueue.test.tsscenarios (normal merge, forced rebase, off-by-default, bounded-retry fallback, unreadable base commit, unauthorized forced action) — each drives the realprocessJobpath and asserts onaudit_eventsnpm run typecheck— cleannpm run test:ci— full local gate greennpm run test:coverage— 100% branch coverage on all new codenpm audit --audit-level=moderate— 0 vulnerabilitiesCloses #2552