Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 8 additions & 0 deletions .gittensory.yml.example
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,14 @@ settings:
# # Default: review-evasion.
# reviewEvasionComment: true # Post the public explanation comment before the enforcement close. Default: true.

# Merge-train FIFO gate (#selfhost-merge-train): without this, a PR merges the instant its OWN gate clears,
# with zero awareness of an older sibling PR still open in the same repo -- proven live to cause out-of-order
# merges and the conflicts that follow. "audit" logs what the gate WOULD hold, without actually holding
# anything -- validate the fix before enabling it for real. "enforce" actually defers a merge behind a
# still-viable older sibling, bounded by a 24h staleness cap so one stuck old PR can never block newer ones
# forever. Off by default.
# mergeTrainMode: off # off | audit | enforce. Default: off.

# Linked-issue HARD-RULE auto-close (#linked-issue-hard-rules): a DETERMINISTIC verdict about the
# linked issue itself (not an AI verdict), gated per-rule to block|off. Fires regardless of
# `guardrailHit`; still respects the `close` autonomy class and the owner/automation exemption. Off
Expand Down
8 changes: 8 additions & 0 deletions apps/gittensory-ui/public/openapi.json
Original file line number Diff line number Diff line change
Expand Up @@ -9293,6 +9293,14 @@
},
"publicQualityMetrics": {
"type": "boolean"
},
"mergeTrainMode": {
"type": "string",
"enum": [
"off",
"audit",
"enforce"
]
}
},
"required": [
Expand Down
8 changes: 8 additions & 0 deletions config/examples/gittensory.full.yml
Original file line number Diff line number Diff line change
Expand Up @@ -792,6 +792,14 @@ settings:
# # Default: review-evasion.
# reviewEvasionComment: true # Post the public explanation comment before the enforcement close. Default: true.

# Merge-train FIFO gate (#selfhost-merge-train): without this, a PR merges the instant its OWN gate clears,
# with zero awareness of an older sibling PR still open in the same repo -- proven live to cause out-of-order
# merges and the conflicts that follow. "audit" logs what the gate WOULD hold, without actually holding
# anything -- validate the fix before enabling it for real. "enforce" actually defers a merge behind a
# still-viable older sibling, bounded by a 24h staleness cap so one stuck old PR can never block newer ones
# forever. Off by default.
# mergeTrainMode: off # off | audit | enforce. Default: off.

# Linked-issue HARD-RULE auto-close (#linked-issue-hard-rules): a DETERMINISTIC verdict about the
# linked issue itself (not an AI verdict), gated per-rule to block|off. Fires regardless of
# `guardrailHit`; still respects the `close` autonomy class and the owner/automation exemption. Off
Expand Down
6 changes: 6 additions & 0 deletions migrations/0121_merge_train_mode.sql
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
-- Merge-train FIFO gate (#selfhost-merge-train). Off by default, same "opt-in, no surprise behavior
-- change" shape as review_evasion_protection: a PR merges the instant its own gate clears today, with
-- zero awareness of an older sibling PR still open in the same repo -- proven live to cause out-of-order
-- merges and the conflicts that follow. "audit" logs what the gate would hold without holding anything;
-- "enforce" actually defers a merge behind a still-viable older sibling.
ALTER TABLE repository_settings ADD COLUMN merge_train_mode TEXT NOT NULL DEFAULT 'off';
6 changes: 6 additions & 0 deletions src/db/repositories.ts
Original file line number Diff line number Diff line change
Expand Up @@ -645,6 +645,7 @@ export async function getRepositorySettings(env: Env, fullName: string): Promise
reviewEvasionProtection: normalizeReviewEvasionProtection(row.reviewEvasionProtection),
reviewEvasionLabel: row.reviewEvasionLabel,
reviewEvasionComment: row.reviewEvasionComment,
mergeTrainMode: normalizeMergeTrainMode(row.mergeTrainMode),
screenshotTableGate: parseScreenshotTableGateRow(row),
createdAt: row.createdAt,
updatedAt: row.updatedAt,
Expand Down Expand Up @@ -765,6 +766,7 @@ export async function upsertRepositorySettings(env: Env, settings: Partial<Repos
reviewEvasionProtection: normalizeReviewEvasionProtection(settings.reviewEvasionProtection),
reviewEvasionLabel: settings.reviewEvasionLabel ?? DEFAULT_REVIEW_EVASION_LABEL,
reviewEvasionComment: settings.reviewEvasionComment ?? true,
mergeTrainMode: normalizeMergeTrainMode(settings.mergeTrainMode),
screenshotTableGate: normalizeScreenshotTableGateConfig(settings.screenshotTableGate, []),
} satisfies RepositorySettings;
const db = getDb(env.DB);
Expand Down Expand Up @@ -7059,6 +7061,10 @@ function normalizeReviewEvasionProtection(value: string | null | undefined): "of
return value === "close" ? "close" : "off";
}

function normalizeMergeTrainMode(value: string | null | undefined): "off" | "audit" | "enforce" {
return value === "audit" || value === "enforce" ? value : "off";
}

// Config-driven before/after screenshot-table gate (#2006): the row stores whenLabels/whenPaths as JSON string
// arrays (mirroring contributorBlacklistJson's shape) across dedicated flat columns rather than one combined
// JSON blob, so a self-hoster can inspect/edit a single field (e.g. just the action) without round-tripping the
Expand Down
3 changes: 3 additions & 0 deletions src/db/schema.ts
Original file line number Diff line number Diff line change
Expand Up @@ -157,6 +157,9 @@ export const repositorySettings = sqliteTable("repository_settings", {
reviewEvasionProtection: text("review_evasion_protection").notNull().default("off"),
reviewEvasionLabel: text("review_evasion_label").notNull().default("review-evasion"),
reviewEvasionComment: integer("review_evasion_comment", { mode: "boolean" }).notNull().default(true),
// Merge-train FIFO gate (#selfhost-merge-train): off by default, same "opt-in, no surprise behavior change"
// shape as reviewEvasionProtection above.
mergeTrainMode: text("merge_train_mode").notNull().default("off"),
// Config-driven before/after screenshot-table gate (#2006): off by default. whenLabels/whenPaths are JSON
// string arrays (mirrors contributorBlacklistJson's shape); screenshotTableGateMessage is nullable ("no
// override" is a `.gittensory.yml`-only concept -- null here means "use the built-in default message").
Expand Down
1 change: 1 addition & 0 deletions src/openapi/schemas.ts
Original file line number Diff line number Diff line change
Expand Up @@ -770,6 +770,7 @@ export const RepositorySettingsSchema = z
reviewEvasionProtection: z.enum(["off", "close"]).optional(),
reviewEvasionLabel: z.string().nullable().optional(),
reviewEvasionComment: z.boolean().optional(),
mergeTrainMode: z.enum(["off", "audit", "enforce"]).optional(),
screenshotTableGate: z
.object({
enabled: z.boolean(),
Expand Down
2 changes: 2 additions & 0 deletions src/queue/processors.ts
Original file line number Diff line number Diff line change
Expand Up @@ -3161,6 +3161,8 @@ async function runAgentMaintenancePlanAndExecute(
agentDryRun: settings.agentDryRun,
installationPermissions,
authorLogin: pr.authorLogin,
mergeTrainMode: settings.mergeTrainMode,
pullRequestCreatedAt: pr.createdAt,
// CI-run cancellation on a contributor_cap close (#2462): the repo's own explicit setting always wins;
// null/undefined (unset) falls back to the install-wide CONTRIBUTOR_CAP_CANCEL_CI_DEFAULT env var.
contributorCapCancelCi: settings.contributorCapCancelCi ?? env.CONTRIBUTOR_CAP_CANCEL_CI_DEFAULT === "true",
Expand Down
62 changes: 62 additions & 0 deletions src/review/merge-train.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,62 @@
// FIFO merge-train gate (#selfhost-merge-train). Without this, a PR merges the instant its OWN gate clears,
// with zero awareness of an older sibling PR still open in the same repo -- proven live via a production D1
// query to cause hundreds of out-of-order merges per repo, and the conflicts that follow. This module is the
// pure decision only: a still-viable older sibling (not conflicted, not past the staleness cap) holds the
// newer PR's merge until the older one either merges, closes, or goes stale. Mirrors this codebase's existing
// "advisory, fail-open, defense-in-depth" lock philosophy (see claimTransientLock's own doc comment) rather
// than a hard, unbypassable serialization -- the staleness cap is the deliberate escape hatch so one stuck old
// PR can never block the repo's newer PRs forever.

/** The subset of a sibling PR's fields this gate actually needs -- kept minimal and independent of
* `PullRequestRecord`'s full shape so this module has zero import surface beyond plain data. */
export type MergeTrainSibling = {
number: number;
createdAt?: string | null | undefined;
mergeableState?: string | null | undefined;
};

/** How long an older sibling can hold up newer ones before it's excluded from blocking (24 hours, matching
* `REGATE_REPAIR_ATTEMPT_LOOKBACK_MS`'s own "genuinely stuck, not just mid-review" cutoff in
* src/queue/processors.ts). A normal review cycle (CI, AI review, human review) can easily run for hours; a
* genuinely stuck PR (its author vanished, review never completes) must not wedge every newer PR in the repo
* indefinitely, so this is the escape hatch, not a tight SLA. */
export const MERGE_TRAIN_MAX_WAIT_MS = 24 * 60 * 60 * 1000;

export type MergeTrainDecision = { wait: true; blockingPr: number } | { wait: false };

/** True when an older, still-viable sibling exists and `thisPrNumber` should wait its turn. A sibling never
* blocks when it is: the same PR, not older (by createdAt, falling back to PR number when createdAt is
* missing on either side -- mirrors the duplicate-winner election's own createdAt-then-number precedent),
* git-conflicted (`mergeableState === "dirty"` -- it isn't "about to merge," it's stuck), or past the
* staleness cap. Deterministic and total: same inputs always produce the same decision. */
export function shouldWaitForOlderSiblings(
thisPrNumber: number,
thisPrCreatedAt: string | null | undefined,
siblings: readonly MergeTrainSibling[],
nowMs: number,
): MergeTrainDecision {
const thisCreatedMs = thisPrCreatedAt ? Date.parse(thisPrCreatedAt) : Number.NaN;
const isOlder = (sibling: MergeTrainSibling): boolean => {
const siblingCreatedMs = sibling.createdAt ? Date.parse(sibling.createdAt) : Number.NaN;
// Both sides need a real, distinct createdAt to compare by date -- if either is missing (or they tie
// exactly), fall back to the lower PR number as "older" (matches the duplicate-winner election's own
// tie-break precedent elsewhere in this codebase; PR numbers are assigned sequentially at creation, so
// this fallback is a safe, always-available proxy for open order).
if (Number.isFinite(siblingCreatedMs) && Number.isFinite(thisCreatedMs) && siblingCreatedMs !== thisCreatedMs) {
return siblingCreatedMs < thisCreatedMs;
}
return sibling.number < thisPrNumber;
};
const viable = siblings
.filter((sibling) => sibling.number !== thisPrNumber)
.filter((sibling) => sibling.mergeableState !== "dirty")
.filter((sibling) => isOlder(sibling))
.filter((sibling) => {
const siblingCreatedMs = sibling.createdAt ? Date.parse(sibling.createdAt) : Number.NaN;
if (!Number.isFinite(siblingCreatedMs)) return true; // unknown age -- fail open toward still blocking
return nowMs - siblingCreatedMs < MERGE_TRAIN_MAX_WAIT_MS;
})
.sort((a, b) => a.number - b.number);
const blocker = viable[0];
return blocker ? { wait: true, blockingPr: blocker.number } : { wait: false };
}
2 changes: 2 additions & 0 deletions src/selfhost/metrics.ts
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,7 @@ const DEFAULT_METRIC_META: readonly (readonly [string, MetricMeta])[] = [
["gittensory_gate_decisions_total", { help: "Gate decisions by conclusion.", type: "counter" }],
["gittensory_precision_breaker_downgrades_total", { help: "Would-merge/would-close actions downgraded to a human hold by an accuracy circuit-breaker, by breaker direction.", type: "counter" }],
["gittensory_agent_disposition_total", { help: "Final agent disposition per PR pass (merge/close/hold), by repo, action class, blocker-code class, and autonomy level.", type: "counter" }],
["gittensory_merge_train_deferred_total", { help: "Merge-train FIFO gate deferrals (an older still-viable sibling held a merge), by repo and mode (audit/enforce).", type: "counter" }],
["gittensory_reviews_published_total", { help: "Published review comments.", type: "counter" }],
["gittensory_github_branch_protection_permission_denied_total", { help: "GitHub branch-protection reads denied by permissions.", type: "counter" }],
["gittensory_github_pr_files_fetch_total", { help: "GitHub pull-request file fetch attempts.", type: "counter" }],
Expand Down Expand Up @@ -154,6 +155,7 @@ const PRIVATE_REPO_LABEL_METRICS = new Set([
const ALWAYS_REDACT_REPO_LABEL_METRICS = new Set([
"gittensory_agent_disposition_total",
"gittensory_queue_backlog_by_repo",
"gittensory_merge_train_deferred_total",
]);
const redactedRepoLabels = new Map<string, string>();

Expand Down
40 changes: 40 additions & 0 deletions src/services/agent-action-executor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ import {
getGlobalModerationConfig,
insertNotificationDeliveryIfAbsent,
isGlobalAgentFrozen,
listOtherOpenPullRequests,
markPullRequestApproved,
markPullRequestMergeBlocked,
recordAuditEvent,
Expand Down Expand Up @@ -35,6 +36,7 @@ import {
type ModerationRuleType,
} from "../settings/moderation-rules";
import { incr } from "../selfhost/metrics";
import { shouldWaitForOlderSiblings } from "../review/merge-train";
import { captureError } from "../selfhost/sentry";

// The agent actor name on every audit record — the App acts on the maintainer's behalf per their configured
Expand Down Expand Up @@ -179,6 +181,15 @@ export type AgentActionExecutionContext = {
// custom label name is honored instead of only ever checking the literal default. `null` explicitly disables
// the manual-review label (and this guard with it); absent/undefined uses the default AGENT_LABEL_NEEDS_REVIEW.
manualReviewLabel?: string | null | undefined;
// Merge-train FIFO gate (#selfhost-merge-train), resolved by the CALLER (same "the executor has no settings
// access" shape as the fields above): "off" (default, unchanged behavior) | "audit" (log what would be held,
// never actually hold) | "enforce" (actually defer a merge behind a still-viable older sibling). Absent/
// undefined behaves exactly like "off".
mergeTrainMode?: "off" | "audit" | "enforce" | undefined;
// This PR's own creation time, resolved by the CALLER (already has the PR record in scope) — the merge-train
// gate below compares this against open siblings fetched fresh, since siblings are only ever fetched lazily
// when the gate is actually enabled (see step 8b), not threaded through every caller unconditionally.
pullRequestCreatedAt?: string | null | undefined;
};

export type ModerationContextSettings = {
Expand Down Expand Up @@ -450,6 +461,35 @@ export async function executeAgentMaintenanceActions(env: Env, ctx: AgentActionE
continue;
}
}
// 8b) merge-train FIFO gate (#selfhost-merge-train): a still-viable OLDER open sibling in this repo holds
// this merge until it merges, closes, or goes stale (see merge-train.ts's staleness cap). Siblings are
// fetched fresh here, lazily, ONLY when the gate is actually enabled for this repo — not threaded through
// every caller unconditionally, since the vast majority of merges never need this check. "audit" mode logs
// the decision but never actually holds anything, so it's safe to enable everywhere to validate the fix
// before switching a repo to "enforce".
if (action.actionClass === "merge" && ctx.mergeTrainMode && ctx.mergeTrainMode !== "off") {
const siblings = await listOtherOpenPullRequests(env, ctx.repoFullName, ctx.pullNumber);
const decision = shouldWaitForOlderSiblings(ctx.pullNumber, ctx.pullRequestCreatedAt, siblings, Date.now());
if (decision.wait) {
incr("gittensory_merge_train_deferred_total", { repo: ctx.repoFullName, mode: ctx.mergeTrainMode });
if (ctx.mergeTrainMode === "enforce") {
await audit("denied", `merge train: waiting for older mergeable sibling #${decision.blockingPr} — action not executed`);
continue;
}
// "audit" mode: record a SEPARATE, informational audit-trail entry (never through the shared `audit`
// closure above, which pushes into the SAME outcomes[] this function returns -- calling it here too
// would silently double the returned outcome count for this one action). The merge itself proceeds
// unaffected below.
await recordAuditEvent(env, {
eventType: "agent.action.merge_train_would_wait",
actor: "gittensory",
targetKey,
outcome: "denied",
detail: `merge train (audit mode): would wait for older mergeable sibling #${decision.blockingPr}`,
metadata: { repoFullName: ctx.repoFullName, pullNumber: ctx.pullNumber, blockingPr: decision.blockingPr },
}).catch(() => undefined);
}
}
// 9) live — perform the real mutation, recording success or the error.
try {
const detailOverride = await performAction(env, ctx, action);
Expand Down
2 changes: 2 additions & 0 deletions src/services/agent-approval-queue.ts
Original file line number Diff line number Diff line change
Expand Up @@ -407,6 +407,8 @@ export async function decidePendingAgentAction(env: Env, input: { id: string; de
agentPaused: settings.agentPaused,
agentDryRun: settings.agentDryRun,
installationPermissions: installation ? installation.permissions : null,
mergeTrainMode: settings.mergeTrainMode,
pullRequestCreatedAt: pr?.createdAt,
// CI-run cancellation on a contributor_cap close (#2462): a contributor_cap close CAN be staged for
// approval (close autonomy = auto_with_approval), so the accept-replay path needs this resolved the
// same way the live webhook path does (src/queue/processors.ts) for the cancel hook to fire here too.
Expand Down
8 changes: 8 additions & 0 deletions src/types.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1009,6 +1009,14 @@ export type RepositorySettings = {
* re-closes as the App -- a close the contributor cannot themselves reopen (#one-shot-reopen) -- applies
* the configured label/comment, and records a `review_evasion` moderation strike. */
reviewEvasionProtection?: "off" | "close" | undefined;
/** Merge-train FIFO gate (#selfhost-merge-train): without this, a PR merges the instant its OWN gate
* clears, with zero awareness of an older sibling PR still open in the same repo -- proven live to cause
* out-of-order merges and the conflicts that follow. `"off"` (the default) is unchanged behavior.
* `"audit"` logs what the gate WOULD hold, without actually holding anything -- the safe way to validate
* the fix before enabling it for real. `"enforce"` actually defers a merge behind a still-viable older
* sibling, bounded by a staleness cap (see {@link "../review/merge-train"}) so one stuck old PR can never
* block newer ones forever. */
mergeTrainMode?: "off" | "audit" | "enforce" | undefined;
/** Review-evasion protection: label applied alongside the enforcement close, gated on `close` autonomy
* like every other anti-abuse label (#label-scoping), mirroring {@link blacklistLabel}'s shape. `undefined`
* ⇒ the `"review-evasion"` default; explicit `null` ⇒ close without any label. */
Expand Down
Loading
Loading