Detail Bug Report

app.detail.dev/org_0f605249-55f3-46eb-880c-20ae3e45ae99/…/bug_56839f70-e105-4492-b554-8c95df216928

Introduced in 3f6adefefadecd7b2c6362978aebfc5e0a80bdbd by @ripgrim on Jul 17, 2026

Summary

• Context: Repos pin a version for each rule (rule_configs); at evaluation time resolveEffectiveRuleConfig decides the version that actually runs — auto-advance to the current version when the saved config parses under the new schema, hold on the pinned version when it does not.
• Bug: For min-merged-prs, the hold guard can never fire: a v1 config {min: N} (the only shape authorable under v1) parses under the v2 schema because trustedAfter carries a Zod default of 1, so every legacy v1 row silently auto-advances to min-merged-prs@2, where min means a completely different signal ("merged PRs in other people's repos" instead of "merged PRs in this repo") and a new trustedAfter: 1 exemption is applied.
• Actual vs. expected: The upgrade policy's own contract (§6 b) is that a behavior change never silently alters what an installed repo enforces — the hold exists precisely so an admin re-confirms rather than losing tuned settings to a silent change — but for this rule the maintainer's tuned threshold is silently re-interpreted on a different signal and a brand-new exemption is auto-created, with no proactive indicator (the hold guard dead, the Rules page silent).
• Impact: Repos that tuned min under v1 now enforce a materially different and looser gate — a contributor the maintainer meant to require min local merges from passes with a single local merge — i.e. a moderation gate is silently weakened in production with no proactive disclosure to the maintainer.

Code with Bug

/**
 * @2 config. `.describe()` on every field is the rule-owned label/help the
 * generated /rules form reads (never the raw key). The meaning changed: `min`
 * now counts merges in OTHER people's repos.
 */
export const minMergedPrsConfigSchemaV2 = z.object({
	min: z.number().int().min(0).default(1)
		.describe("minimum merged change requests in other people's repos"),
	trustedAfter: z.number().int().min(1).default(1)  // <-- BUG 🔴 default makes ANY v1 config parse
		.describe("merges in this repo that exempt a proven local contributor"),
});

const carriesForward = entry.configSchema.safeParse(row.config).success;
return carriesForward
	? {
			ref: `${row.ruleId}@${entry.version}`,
			enabled: row.enabled,
			config: row.config,  // <-- BUG 🔴 raw v1 config re-interpreted under v2 semantics
			held: false,
		}
	: { ref: `${row.ruleId}@${row.version}`, ... held: true };

Explanation

• resolveEffectiveRuleConfig treats “parses under new schema” as “lossless upgrade” and auto-advances the effective rule version.
• For min-merged-prs, the v2 schema uses Zod defaults (notably trustedAfter: 1), so a v1-authored config {min: N} always parses under v2.
• The resolver then carries the raw v1 config forward unchanged; at evaluation time it is parsed as v2, which applies defaults and v2 semantics.
• Because v2 changes the meaning of min and introduces a trustedAfter exemption, legacy v1 configs can become materially less strict without any hold/badge/re-confirmation flow.

Codebase Inconsistency

• The auto-advance test fixture uses a version: 1 row with config {min: 3, trustedAfter: 2} (a shape that could not be authored under v1), so it does not cover the realistic legacy v1 population ({min}-only). The hold test uses config: null, also missing the {min}-only case.

Recommended Fix

Make the hold guard fire for configs authored strictly under the prior schema's shape, rather than for schema-parse failure alone. Concretely: pin the shape that each version could author (for min-merged-prs, "no trustedAfter key" identifies v1 authorship), pass the stored version into the parse decision, and hold when the config was authored under the pinned version — so the maintainer gets the "your saved settings don't carry over; re-confirm" decision the upgrade policy promises.

History

This bug was introduced in commit 3f6adefe ("improvements"). That commit added the rule-versioning policy (§6 b) and resolveEffectiveRuleConfig, defining the auto-advance criterion as "config still parses under the new schema ⇒ carry it forward silently, lossless": const carriesForward = entry.configSchema.safeParse(row.config).success. The flaw slipped in because the policy equates schema parseability with semantic losslessness; git blame confirms the carriesForward logic (packages/contracts/src/rules.ts:801-814) is unchanged since that commit. The v2 minMergedPrsConfigSchemaV2 with its Zod defaults (trustedAfter: 1, min: 1) had already shipped in commit 8b250b53 three days earlier — a commit that only considered new repos' satisfiability and stored runs, not pinned v1 configs — so with the resolver in place, every v1 {min: N} config parses under v2 and auto-advances, defeating the hold guard. Commit 8b250b53 is the enabling precondition (it shipped the fatal defaults while resolveEffectiveRuleConfig did not yet exist), but 3f6adefe is the commit that most recently and clearly contributed the buggy existence: without its parse-success ⇒ auto-advance decision, no silent reinterpretation would occur at all.