Skip to content

Commit c70e811

Browse files
committed
feat(selfhost): per-repo opt-out for the active-review reconciliation sweep
Add review.activeReviewReconciliation: false as an explicit per-repo FORCE-OFF, mirroring review.prReconciliation/review.sweepWatchdog exactly -- config-as-code for this class of fleet-wide sweep is meant to be both globally toggleable AND fine-tunable per repo, which the initial cut missed. Wire the opt-out into runActiveReviewReconciliation so a repo's stale rows are skipped before any GitHub call is spent, failing open on a manifest-load error like every sibling check. Also fixes a pre-existing bug in the engine package's own test script found while re-verifying this change: `rm -rf dist-test` deleted the compiled test output but left `.test.tsbuildinfo` behind, so a second `npm run test` invocation could see the stale incremental cache, skip re-emitting entirely, and `node --test` would then report 0 tests found as a trivial success -- silently no-op-ing the engine's whole test suite while the gate still passed. Clearing both together forces a real recompile every run. And fixes a flaky timestamp-inequality assertion in the out-of-order-webhook regression test added in the prior commit (two upserts can land in the same test millisecond), matching the same caveat already documented on the sibling review-latency-metric tests in this file.
1 parent 283fb8f commit c70e811

8 files changed

Lines changed: 83 additions & 7 deletions

File tree

.loopover.yml.example

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -539,6 +539,13 @@ review:
539539
# Bool or null. Default: null/true -- no change to today's watched-repo-set behavior.
540540
# prReconciliation: false
541541

542+
# Per-repo FORCE-OFF for the active-review-tracking reconciliation sweep (#webhook-reorder-clobber) --
543+
# `false` excludes just this repo's rows from the sweep's scan even though the operator's
544+
# LOOPOVER_ACTIVE_REVIEW_RECONCILIATION flag is on. Deliberately FORCE-OFF-ONLY (no `true` override),
545+
# mirroring `prReconciliation` immediately above. Bool or null. Default: null/true -- no change to today's
546+
# scanned-row-set behavior.
547+
# activeReviewReconciliation: false
548+
542549
# Repeat-false-positive suppression (#2179, part of #1964). Bool | null. Default: null/false — byte-identical
543550
# (no suppression-store read, no matching). Also requires the operator's LOOPOVER_REVIEW_MEMORY env flag to
544551
# be on -- this manifest field alone cannot enable it. When both are on, an advisory (non-blocking) AI finding

config/examples/loopover.full.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -553,6 +553,13 @@ review:
553553
# Bool or null. Default: null/true -- no change to today's watched-repo-set behavior.
554554
# prReconciliation: false
555555

556+
# Per-repo FORCE-OFF for the active-review-tracking reconciliation sweep (#webhook-reorder-clobber) --
557+
# `false` excludes just this repo's rows from the sweep's scan even though the operator's
558+
# LOOPOVER_ACTIVE_REVIEW_RECONCILIATION flag is on. Deliberately FORCE-OFF-ONLY (no `true` override),
559+
# mirroring `prReconciliation` immediately above. Bool or null. Default: null/true -- no change to today's
560+
# scanned-row-set behavior.
561+
# activeReviewReconciliation: false
562+
556563
# Repeat-false-positive suppression (#2179, part of #1964). Bool | null. Default: null/false — byte-identical
557564
# (no suppression-store read, no matching). Also requires the operator's LOOPOVER_REVIEW_MEMORY env flag to
558565
# be on -- this manifest field alone cannot enable it. When both are on, an advisory (non-blocking) AI finding

packages/loopover-engine/package.json

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -73,7 +73,7 @@
7373
],
7474
"scripts": {
7575
"build": "tsc -p tsconfig.json",
76-
"test": "npm run build && rm -rf dist-test && tsc -p tsconfig.test.json && node --test \"dist-test/**/*.test.js\""
76+
"test": "npm run build && rm -rf dist-test .test.tsbuildinfo && tsc -p tsconfig.test.json && node --test \"dist-test/**/*.test.js\""
7777
},
7878
"dependencies": {
7979
"@anthropic-ai/claude-agent-sdk": "^0.3.205",

packages/loopover-engine/src/focus-manifest.ts

Lines changed: 20 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -499,7 +499,9 @@ export type FocusManifestPrReconciliationConfig = {
499499
* (LOOPOVER_ACTIVE_REVIEW_RECONCILIATION), declared under top-level `activeReviewReconciliation:`
500500
* (#webhook-reorder-clobber). Same shape and precedence as `prReconciliation:` above -- the sweep re-checks
501501
* `active_review_tracking` rows a delayed webhook job left stuck "active" for a PR that already closed.
502-
* Not present ⇒ the caller falls back to the LOOPOVER_ACTIVE_REVIEW_RECONCILIATION env var.
502+
* Not present ⇒ the caller falls back to the LOOPOVER_ACTIVE_REVIEW_RECONCILIATION env var. Distinct from
503+
* the per-repo FORCE-OFF under `review.activeReviewReconciliation` (which only excludes one repo's rows
504+
* from the scan set once this fleet-wide gate is ON).
503505
*/
504506
export type FocusManifestActiveReviewReconciliationConfig = {
505507
present: boolean;
@@ -793,6 +795,14 @@ export type FocusManifestReviewConfig = {
793795
* above, for the identical reason. A manifest-load error fails OPEN (the repo stays watched). null/true
794796
* (default, absent) ⇒ no change to today's watched-repo-set behavior. */
795797
prReconciliation: boolean | null;
798+
/** `review.activeReviewReconciliation` (#webhook-reorder-clobber): explicit per-repo FORCE-OFF for the
799+
* active-review-tracking reconciliation sweep (`runActiveReviewReconciliation`,
800+
* `src/review/active-review-reconciliation.ts`) — `false` excludes this repo's stale `active_review_tracking`
801+
* rows from the sweep's scan even though the global `LOOPOVER_ACTIVE_REVIEW_RECONCILIATION` kill-switch is
802+
* on. Deliberately FORCE-OFF-ONLY (no `true` override), mirroring `selftune`/`sweepWatchdog`/`prReconciliation`
803+
* above, for the identical reason. A manifest-load error fails OPEN (the repo's rows stay eligible). null/true
804+
* (default, absent) ⇒ no change to today's scanned-row-set behavior. */
805+
activeReviewReconciliation: boolean | null;
796806
/** `review.memory` (#2179, config slice of #1964): when true, gates repeat-false-positive SUPPRESSION —
797807
* before an advisory (non-blocking) AI finding is surfaced in the unified review comment, it is matched
798808
* against this repo's stored `review_suppression` signals (a maintainer's own past false-positive
@@ -1412,7 +1422,7 @@ const EMPTY_MANIFEST: FocusManifest = {
14121422
publicNotes: [],
14131423
gate: { ...EMPTY_GATE_CONFIG },
14141424
settings: {},
1415-
review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null },
1425+
review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null },
14161426
features: { ...EMPTY_FEATURES_CONFIG },
14171427
experimental: { ...EMPTY_EXPERIMENTAL_CONFIG },
14181428
contentLane: { ...EMPTY_CONTENT_LANE_CONFIG },
@@ -1453,7 +1463,7 @@ function emptyManifest(source: FocusManifestSource, warnings: string[] = []): Fo
14531463
warnings,
14541464
gate: { ...EMPTY_GATE_CONFIG },
14551465
settings: {},
1456-
review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null },
1466+
review: { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null },
14571467
features: { ...EMPTY_FEATURES_CONFIG },
14581468
experimental: { ...EMPTY_EXPERIMENTAL_CONFIG },
14591469
contentLane: { ...EMPTY_CONTENT_LANE_CONFIG },
@@ -2937,7 +2947,7 @@ function parsePublicSafeText(value: JsonValue | undefined, field: string, warnin
29372947
* throws; invalid/unsafe values are dropped with warnings.
29382948
*/
29392949
function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestReviewConfig {
2940-
const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null };
2950+
const empty: FocusManifestReviewConfig = { present: false, footerText: null, note: null, fields: {}, enrichmentAnalyzers: {}, profile: null, tone: null, securityFocus: null, inlineComments: null, fixHandoff: null, autoMergeSummary: null, suggestions: null, changedFilesSummary: null, effortScore: null, impactMap: null, cultureProfile: null, selftune: null, sweepWatchdog: null, prReconciliation: null, activeReviewReconciliation: null, reviewMemory: null, findingCategories: null, inlineCommentsPerCategory: null, minFindingSeverity: null, maxFindings: { ...EMPTY_MAX_FINDINGS_CONFIG }, commentVerbosity: null, e2eTestDelivery: null, e2eTestAutoTrigger: null, pathInstructions: [], instructions: null, excludePaths: [], pathFilters: [], preMergeChecks: [], autoReview: { ...EMPTY_AUTO_REVIEW_CONFIG }, aiModel: { ...EMPTY_SELF_HOST_AI_MODEL_CONFIG }, visual: { ...EMPTY_VISUAL_CONFIG }, linkedIssueSatisfaction: null, sharedConfigSource: null };
29412951
if (value === undefined || value === null) return empty;
29422952
if (typeof value !== "object" || Array.isArray(value)) {
29432953
warnings.push(`Manifest field "review" must be a mapping; ignoring it.`);
@@ -2984,6 +2994,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo
29842994
const selftune = normalizeOptionalBoolean(r.selftune, "review.selftune", warnings);
29852995
const sweepWatchdog = normalizeOptionalBoolean(r.sweepWatchdog, "review.sweepWatchdog", warnings);
29862996
const prReconciliation = normalizeOptionalBoolean(r.prReconciliation, "review.prReconciliation", warnings);
2997+
const activeReviewReconciliation = normalizeOptionalBoolean(r.activeReviewReconciliation, "review.activeReviewReconciliation", warnings);
29872998
const reviewMemory = normalizeOptionalBoolean(r.memory, "review.memory", warnings);
29882999
const findingCategories = normalizeOptionalBoolean(r.finding_categories, "review.finding_categories", warnings);
29893000
const inlineCommentsPerCategory = normalizeOptionalNonNegativeInt(
@@ -3028,6 +3039,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo
30283039
selftune !== null ||
30293040
sweepWatchdog !== null ||
30303041
prReconciliation !== null ||
3042+
activeReviewReconciliation !== null ||
30313043
reviewMemory !== null ||
30323044
findingCategories !== null ||
30333045
inlineCommentsPerCategory !== null ||
@@ -3069,6 +3081,7 @@ function parseReviewConfig(value: JsonValue | undefined, warnings: string[]): Fo
30693081
selftune,
30703082
sweepWatchdog,
30713083
prReconciliation,
3084+
activeReviewReconciliation,
30723085
reviewMemory,
30733086
findingCategories,
30743087
inlineCommentsPerCategory,
@@ -3174,6 +3187,7 @@ function computeReviewConfigPresent(review: Omit<FocusManifestReviewConfig, "pre
31743187
review.selftune !== null ||
31753188
review.sweepWatchdog !== null ||
31763189
review.prReconciliation !== null ||
3190+
review.activeReviewReconciliation !== null ||
31773191
review.reviewMemory !== null ||
31783192
review.findingCategories !== null ||
31793193
review.inlineCommentsPerCategory !== null ||
@@ -3221,6 +3235,7 @@ export function overlayReviewConfig(
32213235
selftune: pickOverlayNullable(override.selftune, base.selftune),
32223236
sweepWatchdog: pickOverlayNullable(override.sweepWatchdog, base.sweepWatchdog),
32233237
prReconciliation: pickOverlayNullable(override.prReconciliation, base.prReconciliation),
3238+
activeReviewReconciliation: pickOverlayNullable(override.activeReviewReconciliation, base.activeReviewReconciliation),
32243239
reviewMemory: pickOverlayNullable(override.reviewMemory, base.reviewMemory),
32253240
findingCategories: pickOverlayNullable(override.findingCategories, base.findingCategories),
32263241
inlineCommentsPerCategory: pickOverlayNullable(override.inlineCommentsPerCategory, base.inlineCommentsPerCategory),
@@ -3796,6 +3811,7 @@ export function reviewConfigToJson(review: FocusManifestReviewConfig): JsonValue
37963811
if (review.selftune !== null) out.selftune = review.selftune;
37973812
if (review.sweepWatchdog !== null) out.sweepWatchdog = review.sweepWatchdog;
37983813
if (review.prReconciliation !== null) out.prReconciliation = review.prReconciliation;
3814+
if (review.activeReviewReconciliation !== null) out.activeReviewReconciliation = review.activeReviewReconciliation;
37993815
if (review.reviewMemory !== null) out.memory = review.reviewMemory;
38003816
if (review.findingCategories !== null) out.finding_categories = review.findingCategories;
38013817
if (review.inlineCommentsPerCategory !== null) out.inline_comments_per_category = review.inlineCommentsPerCategory;

src/review/active-review-reconciliation.ts

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,12 @@ export async function runActiveReviewReconciliation(env: Env, nowMs: number = Da
9898
try {
9999
const repo = await getRepository(env, row.repoFullName);
100100
if (!repo || typeof repo.installationId !== "number") continue;
101+
// Per-repo opt-out (#webhook-reorder-clobber): mirrors pr-reconciliation.ts's watchedRepos() FORCE-OFF
102+
// exactly -- an explicit per-repo `review.activeReviewReconciliation: false` excludes just this repo's
103+
// rows from the sweep even though the fleet-wide gate is on. A manifest-load error fails OPEN (the
104+
// row stays eligible), matching the surrounding scan's own settings-blip fail-safe.
105+
const manifest = await loadRepoFocusManifest(env, row.repoFullName).catch(() => null);
106+
if (manifest?.review.activeReviewReconciliation === false) continue;
101107
const token = (await createInstallationToken(env, repo.installationId).catch(() => undefined)) ?? env.GITHUB_PUBLIC_TOKEN;
102108
const admissionKey = githubRateLimitAdmissionKeyForToken(env, token, repo.installationId);
103109
const liveState = await fetchLivePullRequestState(env, row.repoFullName, row.pullNumber, token, admissionKey);

test/unit/active-review-reconciliation.test.ts

Lines changed: 36 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -11,6 +11,7 @@ import * as repositoriesModule from "../../src/db/repositories";
1111
import * as backfillModule from "../../src/github/backfill";
1212
import { counterValue, resetMetrics } from "../../src/selfhost/metrics";
1313
import { upsertRepoFocusManifest } from "../../src/signals/focus-manifest-loader";
14+
import * as focusManifestLoaderModule from "../../src/signals/focus-manifest-loader";
1415
import { createTestEnv } from "../helpers/d1";
1516

1617
const SELF_REPO = "JSONbored/loopover";
@@ -177,6 +178,41 @@ describe("runActiveReviewReconciliation (#webhook-reorder-clobber)", () => {
177178
expect(liveSpy).not.toHaveBeenCalled();
178179
});
179180

181+
it("REGRESSION: an explicit review.activeReviewReconciliation: false excludes an otherwise-eligible repo's rows from the scan entirely", async () => {
182+
const env = createTestEnv();
183+
await seedStaleActiveRow(env, "owner/opted-out", 9, 9507, STALE_ACTIVE_REVIEW_MIN_AGE_MS + 60_000);
184+
await upsertRepoFocusManifest(env, "owner/opted-out", { review: { activeReviewReconciliation: false } });
185+
const liveSpy = vi.spyOn(backfillModule, "fetchLivePullRequestState");
186+
187+
const reconciled = await runActiveReviewReconciliation(env);
188+
189+
expect(reconciled).toEqual([]);
190+
expect(liveSpy).not.toHaveBeenCalled(); // opted-out before any GitHub call is ever spent
191+
expect(await hasActiveReviewForHeadSha(env, "owner/opted-out", 9, "sha1")).toBe(true); // row untouched
192+
});
193+
194+
it("an explicit review.activeReviewReconciliation: true is a no-op -- the repo's rows are scanned exactly as when unset", async () => {
195+
const env = createTestEnv();
196+
await seedStaleActiveRow(env, "owner/opted-in", 10, 9508, STALE_ACTIVE_REVIEW_MIN_AGE_MS + 60_000);
197+
await upsertRepoFocusManifest(env, "owner/opted-in", { review: { activeReviewReconciliation: true } });
198+
vi.spyOn(backfillModule, "fetchLivePullRequestState").mockResolvedValueOnce("closed");
199+
200+
const reconciled = await runActiveReviewReconciliation(env);
201+
202+
expect(reconciled).toEqual([{ repoFullName: "owner/opted-in", pullNumber: 10 }]);
203+
});
204+
205+
it("fails OPEN on a manifest-load error for the row's own repo -- a config-read blip must never silently exclude a row from reconciliation", async () => {
206+
const env = createTestEnv();
207+
await seedStaleActiveRow(env, "owner/manifest-errors", 11, 9509, STALE_ACTIVE_REVIEW_MIN_AGE_MS + 60_000);
208+
vi.spyOn(focusManifestLoaderModule, "loadRepoFocusManifest").mockRejectedValueOnce(new Error("manifest load failed"));
209+
vi.spyOn(backfillModule, "fetchLivePullRequestState").mockResolvedValueOnce("closed");
210+
211+
const reconciled = await runActiveReviewReconciliation(env);
212+
213+
expect(reconciled).toEqual([{ repoFullName: "owner/manifest-errors", pullNumber: 11 }]);
214+
});
215+
180216
it("fails safe per-row: an error on one row is logged and the scan continues to the next row", async () => {
181217
const env = createTestEnv();
182218
await seedStaleActiveRow(env, "owner/erroring-repo", 6, 9504, STALE_ACTIVE_REVIEW_MIN_AGE_MS + 60_000);

test/unit/db-parsers.test.ts

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -721,7 +721,10 @@ describe("database row parser hardening", () => {
721721
number: 36, title: "PR", state: "open", user: { login: "bob" }, head: { sha: "a2" }, labels: [], updated_at: "2026-07-21T12:05:00.000Z",
722722
});
723723
expect(pushed.headSha).toBe("a2");
724-
expect(pushed.headShaObservedAt).not.toBe(first.headShaObservedAt);
724+
// Not asserting inequality against first.headShaObservedAt (both could land in the same millisecond in
725+
// a fast test run, matching the same caveat the review-latency-metric tests above already document) --
726+
// typeof "string" here plus the exact-preservation assertion below is the deterministic, non-flaky check.
727+
expect(typeof pushed.headShaObservedAt).toBe("string");
725728

726729
// A delayed job for the OLDER `review_requested` event (before the push) finally dequeues, still carrying
727730
// the STALE head "a1". Without resolvedHeadSha driving headShaChanged, this would look like "a2 -> a1", a

0 commit comments

Comments
 (0)