diff --git a/src/review/advisory-ai-routing-config.ts b/src/review/advisory-ai-routing-config.ts index ca793f4f98..7e9b5d2a24 100644 --- a/src/review/advisory-ai-routing-config.ts +++ b/src/review/advisory-ai-routing-config.ts @@ -1,41 +1,6 @@ -import type { AdvisoryAiRoutingConfig } from "../types"; - -export const DEFAULT_ADVISORY_AI_ROUTING: AdvisoryAiRoutingConfig = { - slop: false, - e2eTestGen: false, - planner: false, - summaries: false, - chatQa: false, - chatQaFrontierFallback: false, - intentRouting: false, -}; - -function normalizeField(value: unknown, field: keyof AdvisoryAiRoutingConfig, warnings: string[]): boolean { - if (value === undefined) return DEFAULT_ADVISORY_AI_ROUTING[field]; - if (typeof value === "boolean") return value; - warnings.push(`settings.advisoryAiRouting.${field} must be a boolean; using the default "${DEFAULT_ADVISORY_AI_ROUTING[field]}".`); - return DEFAULT_ADVISORY_AI_ROUTING[field]; -} - -/** - * Normalize a raw `.loopover.yml settings.advisoryAiRouting` value into a typed config, fail-safe: any - * malformed field falls back to its own (false) default and pushes a warning rather than rejecting the - * whole block. Mirrors `normalizeUnlinkedIssueGuardrailConfig`'s per-field discipline. - */ -export function normalizeAdvisoryAiRoutingConfig(input: unknown, warnings: string[]): AdvisoryAiRoutingConfig { - if (input === undefined) return { ...DEFAULT_ADVISORY_AI_ROUTING }; - if (typeof input !== "object" || input === null || Array.isArray(input)) { - warnings.push("settings.advisoryAiRouting must be an object; using the default (every capability off)."); - return { ...DEFAULT_ADVISORY_AI_ROUTING }; - } - const record = input as Record; - return { - slop: normalizeField(record.slop, "slop", warnings), - e2eTestGen: normalizeField(record.e2eTestGen, "e2eTestGen", warnings), - planner: normalizeField(record.planner, "planner", warnings), - summaries: normalizeField(record.summaries, "summaries", warnings), - chatQa: normalizeField(record.chatQa, "chatQa", warnings), - chatQaFrontierFallback: normalizeField(record.chatQaFrontierFallback, "chatQaFrontierFallback", warnings), - intentRouting: normalizeField(record.intentRouting, "intentRouting", warnings), - }; -} +// advisory-ai-routing-config, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained twin +// of the engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/advisory-ai-routing-config.ts (imported via relative source path, not the +// published package, to match this repo's existing engine-consumption convention — see +// src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/advisory-ai-routing-config"; diff --git a/src/review/cla-check.ts b/src/review/cla-check.ts index bdc8a66ae8..77519ee034 100644 --- a/src/review/cla-check.ts +++ b/src/review/cla-check.ts @@ -1,88 +1,5 @@ -import type { AdvisoryFinding } from "../types"; - -/** Finding code raised when `gate.claMode` is opted in (advisory/block) and neither configured detection method - * (the PR body consent phrase, or the named CLA-bot check-run) confirms consent. ALWAYS severity "warning" at - * generation time — mirrors `manifest_missing_tests`/`manifest_linked_issue_required` (focus-manifest.ts): a - * single finding code whose escalation to a hard blocker is decided entirely by the configured gate MODE - * (isConfiguredGateBlocker, src/rules/advisory.ts), not by this evaluator. */ -export const CLA_CONSENT_MISSING_CODE = "cla_consent_missing"; -/** Finding code emitted when check-run detection is the ONLY configured method and its conclusion could not be - * resolved (a transient fetch failure, not a resolved "no such check-run"). Mirrors `pre_merge_check_unresolved` - * (review/pre-merge-checks.ts): isEvaluationBlocker (advisory.ts) treats this as a NEUTRAL gate (HELD, - * re-evaluates automatically) — never silently skipping a hard requirement and never hard-closing the - * contributor on a transient resolution miss. */ -export const CLA_CHECK_UNRESOLVED_CODE = "cla_check_unresolved"; - -export type ClaCheckConfig = { - /** Public-safe-filtered consent phrase a maintainer requires somewhere in the PR body (case-insensitive - * substring match), e.g. "I have read and agree to the CLA". `null` ⇒ phrase-match detection is not configured. */ - consentPhrase: string | null; - /** Name of a separate CLA-bot check-run this repo also runs (e.g. "CLA Assistant Lite"). When set, a - * `success`/`neutral` conclusion for a check-run with this exact name (case-insensitive) also satisfies - * consent. `null` ⇒ check-run detection is not configured. */ - checkRunName: string | null; -}; - -/** - * Evaluate `.loopover.yml gate.claMode` + `gate.cla` (consentPhrase / checkRunName) against a PR — - * DETERMINISTICALLY, mirroring the pre-merge-checks title/description phrase-match pattern (review/pre-merge-checks.ts) - * exactly: a case-insensitive substring match against already-resolved PR data, no AI judgment. Consent is - * satisfied when EITHER configured method holds (an "either" contract, not "all", because a repo may only be able - * to detect ONE method for a given PR — e.g. no check-run data was resolved): the PR body contains - * `consentPhrase`, OR a check-run named `checkRunName` concluded `success`/`neutral`. When NEITHER method is - * configured (both null), there is nothing to evaluate — no finding (byte-identical, matches `pre_merge_checks`' - * empty-checks behavior). - * - * `checkRunConclusion` is `undefined` when the caller could not resolve check-run data at all (a transient - * fetch failure, or the predicted-gate metadata-only path, which never sees live check-runs) — that is NOT the - * same as a resolved-but-absent check-run (`null`, "no check-run with this name exists"). When check-run - * detection is configured and its conclusion is unresolved (a transient fetch failure, or "not yet run"), this - * HOLDS (`cla_check_unresolved`) instead of failing closed — exactly like an unresolved changed-file set HOLDS - * a path-gated pre-merge check rather than silently skipping (auto-merge bypass) or hard-closing on a - * transient miss. This applies EVEN WHEN `consentPhrase` is ALSO configured but not (yet) satisfied: per the - * "either method holds ⇒ satisfied" contract above, an unresolved check-run might still satisfy consent, so - * deciding purely from a not-yet-satisfied phrase would hard-fail a PR the check-run could have saved (#2564 - * gate-review finding). A hold only degrades to a hard `cla_consent_missing` once EVERY configured method has - * been definitively resolved and none of them is satisfied. Pure + side-effect-free; the caller pushes the - * finding into the advisory before the gate evaluates. - */ -export function evaluateClaCheck( - config: ClaCheckConfig, - ctx: { body?: string | null | undefined; checkRunConclusion?: string | null | undefined }, -): AdvisoryFinding[] { - // A blank/whitespace-only consentPhrase is treated as unset (null), mirroring the config-as-code path's - // normalizeOptionalString (packages/loopover-engine/src/focus-manifest.ts): otherwise `"".includes("")` (or - // any body `.includes("")`) is unconditionally true, silently satisfying consent for every PR — the DB-backed - // dashboard `claConsentPhrase` field has no non-empty validation and reaches here via `?? null` unchanged (#5838). - const consentPhrase = config.consentPhrase !== null && config.consentPhrase.trim().length > 0 ? config.consentPhrase : null; - if (consentPhrase === null && config.checkRunName === null) return []; // nothing configured ⇒ no finding - const phraseSatisfied = consentPhrase !== null && (ctx.body ?? "").toLowerCase().includes(consentPhrase.toLowerCase()); - const checkRunSatisfied = config.checkRunName !== null && (ctx.checkRunConclusion === "success" || ctx.checkRunConclusion === "neutral"); - if (phraseSatisfied || checkRunSatisfied) return []; - // A configured check-run whose conclusion is unresolved: cannot confirm OR deny consent via that method, so - // HOLD rather than fail closed — regardless of whether consentPhrase is ALSO configured (a not-yet-satisfied - // phrase does not mean consent is definitively absent while the check-run could still satisfy it). - if (config.checkRunName !== null && ctx.checkRunConclusion === undefined) { - return [ - { - code: CLA_CHECK_UNRESOLVED_CODE, - severity: "warning", - title: `CLA check held — "${config.checkRunName}" not resolved`, - detail: `LoopOver could not resolve the "${config.checkRunName}" check-run's conclusion for this PR; the gate is held and re-evaluates automatically.`, - action: "No action needed — the gate re-evaluates once the check-run's conclusion is available.", - }, - ]; - } - const missing: string[] = []; - if (consentPhrase !== null) missing.push(`the PR description must contain "${consentPhrase}"`); - if (config.checkRunName !== null) missing.push(`the "${config.checkRunName}" check must pass`); - return [ - { - code: CLA_CONSENT_MISSING_CODE, - severity: "warning", - title: "CLA consent not confirmed", - detail: `This PR does not confirm contributor license agreement consent: ${missing.join(" or ")}.`, - action: "Add the required CLA consent phrase to the PR description, or complete the CLA check, then re-run the gate.", - }, - ]; -} +// cla-check, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained twin of the engine +// copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/cla-check.ts (imported via relative source path, not the published +// package, to match this repo's existing engine-consumption convention — see src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/cla-check"; diff --git a/src/review/enrichment-analyzer-names.ts b/src/review/enrichment-analyzer-names.ts index 1d13489cd4..821c9cdd61 100644 --- a/src/review/enrichment-analyzer-names.ts +++ b/src/review/enrichment-analyzer-names.ts @@ -1,68 +1,6 @@ -// Canonical REES enrichment-analyzer name registry (#2050). The single source of truth for the analyzer keys that -// both the operator `REES_ANALYZERS` env list and the per-repo `.loopover.yml` `review.enrichment` toggles are -// validated against. A leaf module with no imports, so the review wiring and the signals-layer manifest parser can -// share it without a heavy or circular dependency. - -export const REES_ANALYZER_NAMES = [ - "dependency", - "dependencyDiff", - "lockfileDrift", - "secret", - "license", - "installScript", - "heavyDependency", - "hardcodedUrl", - "actionPin", - "eol", - "redos", - "provenance", - "codeowners", - "secretLog", - "assetWeight", - "typosquat", - "commitSignature", - "iacMisconfig", - "nativeBuild", - "history", - "docCommentDrift", - "duplication", - "duplicationDelta", - "churnHotspot", - "blameLink", - "approvalIntegrity", - "ciCheckSignals", - "undocumentedExport", - "staleBranch", - "commitHygiene", - "pendingReviewRequests", - "testRatio", - "migrationSafety", - "looseRange", - "terminology", - "todoMarker", - "magicNumber", - "conflictMarker", - "debugLeftover", - "sizeSmell", - "floatingPromise", - "deepNesting", - "errorSwallow", - "complexity", - "complexityDelta", - "unsafeAny", - "a11y", - "i18n", - "unusedExport", - "exhaustiveness", - "flakyTest", - "commitLint", - "apiBreak", - "deprecatedDep", - "revertRecurrence", - "coverageDelta", - "callerImpact", -] as const; - -export type ReesAnalyzerName = (typeof REES_ANALYZER_NAMES)[number]; - -export const REES_ANALYZER_NAME_SET: ReadonlySet = new Set(REES_ANALYZER_NAMES); +// enrichment-analyzer-names, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained twin +// of the engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/enrichment-analyzer-names.ts (imported via relative source path, not the +// published package, to match this repo's existing engine-consumption convention — see +// src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/enrichment-analyzer-names"; diff --git a/src/review/guardrail-config.ts b/src/review/guardrail-config.ts index 54cbedc725..c249e008f2 100644 --- a/src/review/guardrail-config.ts +++ b/src/review/guardrail-config.ts @@ -1,77 +1,5 @@ -import type { RepositorySettings } from "../types"; - -// This is a Set-membership guardrail list (order doesn't matter, unlike the loaders' priority-ordered candidate -// lists): a contributor PR touching the canonical `.loopover.*` config file gets hard-guardrail protection. -export const CONFIG_AS_CODE_GUARDRAIL_GLOBS = [ - ".loopover.yml", - ".loopover.yaml", - ".loopover.json", - ".github/loopover.yml", - ".github/loopover.yaml", - ".github/loopover.json", - "**/codecov.yml", - "**/codecov.yaml", - "**/.codecov.yml", -]; - -export const WORKFLOW_AND_RUNTIME_GUARDRAIL_GLOBS = [ - ".github/workflows/**", - "scripts/**", - "wrangler.jsonc", - "src/selfhost/**", -]; - -export const ENGINE_DECISION_GUARDRAIL_GLOBS = [ - "src/rules/**", - "src/services/**", - "src/settings/agent-actions.ts", - "src/settings/agent-execution.ts", - "src/settings/agent-sweep.ts", - "src/settings/autonomy.ts", - "src/queue/**", - "src/github/pr-actions.ts", - "src/github/app.ts", - "src/github/backfill.ts", - // #4197: writes a real commit onto a CONTRIBUTOR's own PR branch (not a branch loopover owns) — the same - // guardrail tier as pr-actions.ts/app.ts for the same reason, a new GitHub-write surface. - "src/github/e2e-test-commit.ts", - "src/scoring/**", - "src/auth/**", - "src/review/safety.ts", - "src/review/guardrail-config.ts", - "src/review/cutover-gate.ts", - "src/review/linked-issue-hard-rules.ts", - "src/review/outcomes-wire.ts", -]; - -// Default, safe-by-default invariant set (restored by #3943 after the original pure-config-as-code design -// let a `.loopover.yml` edit silently remove its own guardrail protection). Repo settings can only ADD to -// this set UNLESS the repo explicitly opts in via `hardGuardrailGlobsOverridesInvariants` (below). -export const DEFAULT_HARD_GUARDRAIL_GLOBS = [ - ...CONFIG_AS_CODE_GUARDRAIL_GLOBS, - ...WORKFLOW_AND_RUNTIME_GUARDRAIL_GLOBS, - ...ENGINE_DECISION_GUARDRAIL_GLOBS, -]; - -/** - * Resolve hard-guardrail path globs from the already-effective repo settings. - * - * Safe by default (#3943): `DEFAULT_HARD_GUARDRAIL_GLOBS` is an invariant floor, and a repo's configured - * `hardGuardrailGlobs` is ADDED to it (deduplicated), never allowed to shrink it — so an ordinary - * `.loopover.yml` edit (even a careless or malicious one) can only ever widen guardrail protection. - * - * Full self-hoster control, opt-in (config-as-code mandate): a repo that explicitly sets - * `hardGuardrailGlobsOverridesInvariants: true` takes complete ownership of its guardrail list — - * `hardGuardrailGlobs` is then used EXACTLY as given (including an explicit `[]` to disable path guardrails - * entirely), REPLACING rather than adding to the built-in floor. This is deliberately a second, explicit - * field rather than reusing `hardGuardrailGlobs: []`'s presence/absence, so opting out of the safety net is - * always a conscious, separately-visible decision in the config file, not a side effect of trimming a list. - */ -export function resolveHardGuardrailGlobs( - settings: Pick | null | undefined, -): string[] { - const configured = settings?.hardGuardrailGlobs; - const configuredList = Array.isArray(configured) ? configured : []; - if (settings?.hardGuardrailGlobsOverridesInvariants === true) return [...configuredList]; - return Array.from(new Set([...DEFAULT_HARD_GUARDRAIL_GLOBS, ...configuredList])); -} +// guardrail-config, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained twin of the +// engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/guardrail-config.ts (imported via relative source path, not the published +// package, to match this repo's existing engine-consumption convention — see src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/guardrail-config"; diff --git a/src/review/linked-issue-hard-rules-config.ts b/src/review/linked-issue-hard-rules-config.ts index 990ff7c28f..e253b1fd67 100644 --- a/src/review/linked-issue-hard-rules-config.ts +++ b/src/review/linked-issue-hard-rules-config.ts @@ -1,85 +1,6 @@ -import type { LinkedIssueHardRulesConfig, LinkedIssueHardRulesMode } from "../types"; - -const VALID_LINKED_ISSUE_HARD_RULE_MODES: readonly LinkedIssueHardRulesMode[] = ["block", "off"]; -const DEFAULT_CLOSE_DELAY_SECONDS = 30; -const MAX_CLOSE_DELAY_SECONDS = 300; - -export const DEFAULT_LINKED_ISSUE_HARD_RULES: LinkedIssueHardRulesConfig = { - ownerAssignedClose: "off", - assignedIssueClose: "off", - missingPointLabelClose: "off", - maintainerOnlyLabelClose: "off", - pointBearingLabels: [], - maintainerOnlyLabels: [], - defaultLabelRepo: false, - verifyBeforeClose: true, - closeDelaySeconds: DEFAULT_CLOSE_DELAY_SECONDS, -}; - -export function isLinkedIssueHardRuleMode(value: unknown): value is LinkedIssueHardRulesMode { - return typeof value === "string" && (VALID_LINKED_ISSUE_HARD_RULE_MODES as readonly string[]).includes(value); -} - -function normalizeStringList(value: unknown, field: string, warnings: string[]): string[] { - if (value === undefined) return []; - if (!Array.isArray(value)) { - warnings.push(`settings.linkedIssueHardRules.${field} must be an array; using no labels.`); - return []; - } - const labels: string[] = []; - for (const [index, item] of value.entries()) { - if (typeof item !== "string" || item.trim().length === 0) { - warnings.push(`settings.linkedIssueHardRules.${field}[${index}] must be a non-empty string; ignoring it.`); - continue; - } - labels.push(item.trim()); - } - return labels; -} - -function normalizeMode( - value: unknown, - field: "ownerAssignedClose" | "assignedIssueClose" | "missingPointLabelClose" | "maintainerOnlyLabelClose", - warnings: string[], -): LinkedIssueHardRulesMode { - if (value === undefined) return DEFAULT_LINKED_ISSUE_HARD_RULES[field]; - if (isLinkedIssueHardRuleMode(value)) return value; - warnings.push(`settings.linkedIssueHardRules.${field} must be one of block, off; using the default "${DEFAULT_LINKED_ISSUE_HARD_RULES[field]}".`); - return DEFAULT_LINKED_ISSUE_HARD_RULES[field]; -} - -function normalizeBoolean(value: unknown, field: "defaultLabelRepo" | "verifyBeforeClose", warnings: string[]): boolean { - if (value === undefined) return DEFAULT_LINKED_ISSUE_HARD_RULES[field]; - if (typeof value === "boolean") return value; - warnings.push(`settings.linkedIssueHardRules.${field} must be a boolean; using the default "${DEFAULT_LINKED_ISSUE_HARD_RULES[field]}".`); - return DEFAULT_LINKED_ISSUE_HARD_RULES[field]; -} - -function normalizeCloseDelaySeconds(value: unknown, warnings: string[]): number { - if (value === undefined) return DEFAULT_LINKED_ISSUE_HARD_RULES.closeDelaySeconds; - if (typeof value !== "number" || !Number.isFinite(value) || value < 0) { - warnings.push(`settings.linkedIssueHardRules.closeDelaySeconds must be a non-negative number; using the default "${DEFAULT_CLOSE_DELAY_SECONDS}".`); - return DEFAULT_CLOSE_DELAY_SECONDS; - } - return Math.min(MAX_CLOSE_DELAY_SECONDS, Math.floor(value)); -} - -export function normalizeLinkedIssueHardRulesConfig(input: unknown, warnings: string[]): LinkedIssueHardRulesConfig { - if (input === undefined) return { ...DEFAULT_LINKED_ISSUE_HARD_RULES, pointBearingLabels: [], maintainerOnlyLabels: [] }; - if (typeof input !== "object" || input === null || Array.isArray(input)) { - warnings.push("settings.linkedIssueHardRules must be an object; using the default all-off policy."); - return { ...DEFAULT_LINKED_ISSUE_HARD_RULES, pointBearingLabels: [], maintainerOnlyLabels: [] }; - } - const record = input as Record; - return { - ownerAssignedClose: normalizeMode(record.ownerAssignedClose, "ownerAssignedClose", warnings), - assignedIssueClose: normalizeMode(record.assignedIssueClose, "assignedIssueClose", warnings), - missingPointLabelClose: normalizeMode(record.missingPointLabelClose, "missingPointLabelClose", warnings), - maintainerOnlyLabelClose: normalizeMode(record.maintainerOnlyLabelClose, "maintainerOnlyLabelClose", warnings), - pointBearingLabels: normalizeStringList(record.pointBearingLabels, "pointBearingLabels", warnings), - maintainerOnlyLabels: normalizeStringList(record.maintainerOnlyLabels, "maintainerOnlyLabels", warnings), - defaultLabelRepo: normalizeBoolean(record.defaultLabelRepo, "defaultLabelRepo", warnings), - verifyBeforeClose: normalizeBoolean(record.verifyBeforeClose, "verifyBeforeClose", warnings), - closeDelaySeconds: normalizeCloseDelaySeconds(record.closeDelaySeconds, warnings), - }; -} +// linked-issue-hard-rules-config, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained +// twin of the engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/linked-issue-hard-rules-config.ts (imported via relative source path, not +// the published package, to match this repo's existing engine-consumption convention — see +// src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/linked-issue-hard-rules-config"; diff --git a/src/review/linked-issue-label-propagation.ts b/src/review/linked-issue-label-propagation.ts index 0430691c93..81c2b70343 100644 --- a/src/review/linked-issue-label-propagation.ts +++ b/src/review/linked-issue-label-propagation.ts @@ -1,116 +1,6 @@ -import type { LinkedIssueLabelPropagationConfig, LinkedIssueLabelPropagationMapping, LinkedIssueLabelPropagationMode } from "../types"; - -export type { LinkedIssueLabelPropagationConfig, LinkedIssueLabelPropagationMapping, LinkedIssueLabelPropagationMode } from "../types"; - -// Linked-issue label PROPAGATION (#priority-linked-issue-gate). Generic, config-driven mechanism: when a -// linked/closing issue already carries a configured label, copy a mapped label onto the PR. Built specifically -// so a maintainer-reward/bonus label (e.g. `gittensor:priority`) can NEVER be inferred from a PR's title, -// changed files, AI output, or existing PR labels — only ever from a linked issue that ALREADY carries it. -// Generic beyond that one use case: any self-hoster can map any issue label to any PR label, exclusive -// (replaces the normal bug/feature type label, like priority does) or additive (applied alongside it). -// -// PURE config types + normalizer only — no GitHub/fetch/Env-dependent imports. `focus-manifest.ts`'s YAML -// parser imports this module directly, and `focus-manifest.ts` is itself pulled into the loopover-ui -// workspace's isolated typecheck (via `apps/loopover-ui/src/lib/registration-workspace.ts`), which has no -// visibility into the Worker's ambient `Env` type. The actual GitHub fetch orchestrator -// (`fetchLinkedIssueLabelsForPropagation`) lives in `linked-issue-label-propagation-fetch.ts` instead, kept -// out of this file specifically so the UI workspace's typecheck never has to resolve `Env`. - -// Fail-SAFE default: propagation OFF, no mappings. A self-hoster must explicitly opt in per repo. -export const DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION: LinkedIssueLabelPropagationConfig = { - enabled: false, - mode: "exclusive_type_label", - mappings: [], -}; - -// Exported so `focus-manifest.ts`'s sparse-override parser can check whether a raw `mode` value is -// actually valid before deciding to copy the normalizer's (possibly defaults-filled-on-invalid) result. -export const VALID_LINKED_ISSUE_LABEL_PROPAGATION_MODES: readonly LinkedIssueLabelPropagationMode[] = ["exclusive_type_label"]; - -function normalizeMapping(input: unknown, index: number, warnings: string[]): LinkedIssueLabelPropagationMapping | null { - if (typeof input !== "object" || input === null || Array.isArray(input)) { - warnings.push(`settings.linkedIssueLabelPropagation.mappings[${index}] must be an object; ignoring it.`); - return null; - } - const record = input as Record; - const issueLabel = typeof record.issueLabel === "string" ? record.issueLabel.trim() : ""; - const prLabel = typeof record.prLabel === "string" ? record.prLabel.trim() : ""; - if (issueLabel.length === 0 || prLabel.length === 0) { - warnings.push(`settings.linkedIssueLabelPropagation.mappings[${index}] must have non-empty "issueLabel" and "prLabel" strings; ignoring it.`); - return null; - } - // `removeOtherTypeLabels` picks exclusive (replaces the type label, the gittensor:priority case) vs. - // additive (applied alongside it) -- silently coercing a present-but-wrong-shaped value (e.g. a quoted - // `"true"` string) to `false` could flip an intended-exclusive mapping to additive without any signal, - // so a present, non-boolean value drops the whole entry with a warning instead (omitted is still a - // normal, unwarned default of `false`). - if (record.removeOtherTypeLabels !== undefined && typeof record.removeOtherTypeLabels !== "boolean") { - warnings.push(`settings.linkedIssueLabelPropagation.mappings[${index}].removeOtherTypeLabels must be a boolean; ignoring this mapping.`); - return null; - } - // Unlike `removeOtherTypeLabels`, a malformed value here can only ever be warned-and-defaulted (never - // dropped) -- defaulting to `undefined`/strict is always the SAFE direction (no mapping accidentally - // starts trusting maintainer-authored issues), so there is no silent-flip risk that would justify - // discarding an otherwise-valid mapping over it. - let trustMaintainerAuthoredIssue: boolean | undefined; - if (record.trustMaintainerAuthoredIssue !== undefined) { - if (typeof record.trustMaintainerAuthoredIssue === "boolean") { - trustMaintainerAuthoredIssue = record.trustMaintainerAuthoredIssue; - } else { - warnings.push(`settings.linkedIssueLabelPropagation.mappings[${index}].trustMaintainerAuthoredIssue must be a boolean; ignoring it.`); - } - } - // Same parse contract as trustMaintainerAuthoredIssue just above (#priority-reward-maintainer-trust): - // malformed is warned-and-defaulted to undefined/strict, never silently coerced, never a reason to drop - // an otherwise-valid mapping. - let trustMaintainerAuthoredIssueForReward: boolean | undefined; - if (record.trustMaintainerAuthoredIssueForReward !== undefined) { - if (typeof record.trustMaintainerAuthoredIssueForReward === "boolean") { - trustMaintainerAuthoredIssueForReward = record.trustMaintainerAuthoredIssueForReward; - } else { - warnings.push(`settings.linkedIssueLabelPropagation.mappings[${index}].trustMaintainerAuthoredIssueForReward must be a boolean; ignoring it.`); - } - } - return { issueLabel, prLabel, removeOtherTypeLabels: record.removeOtherTypeLabels === true, trustMaintainerAuthoredIssue, trustMaintainerAuthoredIssueForReward }; -} - -/** Defaults-fill a per-repo `linkedIssueLabelPropagation` override into an always-complete, safe config — - * mirrors `normalizeCommandAuthorizationPolicy`'s defaults-fill pattern - * (`src/settings/command-authorization.ts`). Malformed mapping entries are dropped with a warning; valid - * entries in the same array are kept (matches `commandAuthorization`'s per-entry `commands` validation). */ -export function normalizeLinkedIssueLabelPropagationConfig(input: unknown, warnings: string[]): LinkedIssueLabelPropagationConfig { - if (input === undefined) return { ...DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION, mappings: [] }; - if (typeof input !== "object" || input === null || Array.isArray(input)) { - warnings.push("settings.linkedIssueLabelPropagation must be an object; propagation stays disabled."); - return { ...DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION, mappings: [] }; - } - const record = input as Record; - let enabled = DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION.enabled; - if (record.enabled !== undefined) { - if (typeof record.enabled === "boolean") { - enabled = record.enabled; - } else { - warnings.push(`settings.linkedIssueLabelPropagation.enabled must be a boolean; using the default "${DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION.enabled}".`); - } - } - let mode: LinkedIssueLabelPropagationMode = DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION.mode; - if (record.mode !== undefined) { - if (typeof record.mode === "string" && (VALID_LINKED_ISSUE_LABEL_PROPAGATION_MODES as readonly string[]).includes(record.mode)) { - mode = record.mode as LinkedIssueLabelPropagationMode; - } else { - warnings.push(`settings.linkedIssueLabelPropagation.mode must be one of ${VALID_LINKED_ISSUE_LABEL_PROPAGATION_MODES.join(", ")}; using the default "${DEFAULT_LINKED_ISSUE_LABEL_PROPAGATION.mode}".`); - } - } - let mappings: LinkedIssueLabelPropagationMapping[] = []; - if (record.mappings !== undefined) { - if (Array.isArray(record.mappings)) { - mappings = record.mappings.flatMap((entry, index) => { - const normalized = normalizeMapping(entry, index, warnings); - return normalized ? [normalized] : []; - }); - } else { - warnings.push("settings.linkedIssueLabelPropagation.mappings must be an array; using no mappings."); - } - } - return { enabled, mode, mappings }; -} +// linked-issue-label-propagation, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained +// twin of the engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/linked-issue-label-propagation.ts (imported via relative source path, not +// the published package, to match this repo's existing engine-consumption convention — see +// src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/linked-issue-label-propagation"; diff --git a/src/review/pre-merge-checks.ts b/src/review/pre-merge-checks.ts index 77bd38f1d9..c9f27e9cad 100644 --- a/src/review/pre-merge-checks.ts +++ b/src/review/pre-merge-checks.ts @@ -1,67 +1,5 @@ -import { matchesManifestPath, type PreMergeCheck } from "../signals/focus-manifest"; -import type { AdvisoryFinding } from "../types"; - -/** Finding code for a FAILED advisory (default) pre-merge check — surfaced but NEVER blocks. */ -export const PRE_MERGE_CHECK_ADVISORY_CODE = "pre_merge_check_failed"; -/** Finding code for a FAILED pre-merge check the maintainer marked `enforce: true` — a hard gate blocker - * (isConfiguredGateBlocker treats this code as blocking, like secret_leak). */ -export const PRE_MERGE_CHECK_BLOCKING_CODE = "pre_merge_check_required"; -/** Finding code emitted when an ENFORCED `whenPaths`-gated check cannot be evaluated because the PR's changed-file - * set could not be resolved. isEvaluationBlocker (advisory.ts) treats this as a NEUTRAL gate (HELD, re-evaluates - * automatically) — never silently skipping a hard requirement (auto-merge bypass) and never hard-closing the - * contributor on a transient resolution miss. (#review-audit) */ -export const PRE_MERGE_CHECK_UNRESOLVED_CODE = "pre_merge_check_unresolved"; - -/** - * Evaluate the maintainer's `.loopover.yml review.pre_merge_checks` against a PR — DETERMINISTICALLY, with no AI - * judgment. A check with `whenPaths` applies only when a changed path matches; it PASSES only when EVERY configured - * assertion holds (the title contains `titleContains`, the body contains `descriptionContains`, and the - * `requireLabel` label is present — all case-insensitive). Each FAILED check yields ONE finding: - * `pre_merge_check_required` (severity critical → the gate blocks under enforce) or `pre_merge_check_failed` - * (severity warning → advisory). Pure + side-effect-free; the caller pushes the findings into the advisory before - * the gate evaluates. Empty `checks` ⇒ no findings (byte-identical). - */ -export function evaluatePreMergeChecks( - checks: PreMergeCheck[], - ctx: { title?: string | null | undefined; body?: string | null | undefined; labels?: string[] | null | undefined; changedPaths: string[]; filesResolved?: boolean | undefined }, -): AdvisoryFinding[] { - const title = (ctx.title ?? "").toLowerCase(); - const body = (ctx.body ?? "").toLowerCase(); - const labels = (ctx.labels ?? []).map((label) => label.toLowerCase()); - const filesResolved = ctx.filesResolved ?? true; // absent ⇒ caller asserts a trustworthy changedPaths set - const findings: AdvisoryFinding[] = []; - for (const check of checks) { - // when_paths gate: a check with whenPaths applies ONLY to PRs that touch a matching path; an unmatched check - // is N/A (no finding). Empty whenPaths ⇒ the check always applies (title/description/label only). - if (check.whenPaths.length > 0) { - if (!filesResolved) { - // The changed-file set could not be resolved, so we cannot evaluate this path gate. HOLD the gate for an - // ENFORCED check (re-evaluates when files resolve) instead of silently skipping a hard requirement (which - // would let a guarded PR auto-merge). An advisory check is just dropped (no noise on a transient miss). - if (check.enforce) - findings.push({ - code: PRE_MERGE_CHECK_UNRESOLVED_CODE, - severity: "warning", - title: `Pre-merge check held — changed files not resolved: ${check.name}`, - detail: `LoopOver could not resolve this PR's changed files to evaluate the path-gated check "${check.name}"; the gate is held and re-evaluates automatically.`, - action: "No action needed — the gate re-evaluates once the PR's files are available.", - }); - continue; - } - if (!ctx.changedPaths.some((path) => check.whenPaths.some((glob) => matchesManifestPath(path, glob)))) continue; - } - const unmet: string[] = []; - if (check.titleContains !== null && !title.includes(check.titleContains.toLowerCase())) unmet.push(`the title must contain "${check.titleContains}"`); - if (check.descriptionContains !== null && !body.includes(check.descriptionContains.toLowerCase())) unmet.push(`the description must contain "${check.descriptionContains}"`); - if (check.requireLabel !== null && !labels.includes(check.requireLabel.toLowerCase())) unmet.push(`the "${check.requireLabel}" label must be applied`); - if (unmet.length === 0) continue; // every configured assertion held → the check passed - findings.push({ - code: check.enforce ? PRE_MERGE_CHECK_BLOCKING_CODE : PRE_MERGE_CHECK_ADVISORY_CODE, - severity: check.enforce ? "critical" : "warning", - title: `Pre-merge check not satisfied: ${check.name}`, - detail: `This PR does not satisfy the maintainer pre-merge check "${check.name}": ${unmet.join("; ")}.`, - action: "Update the PR to satisfy the check, then re-run the gate.", - }); - } - return findings; -} +// pre-merge-checks, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained twin of the +// engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/pre-merge-checks.ts (imported via relative source path, not the published +// package, to match this repo's existing engine-consumption convention — see src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/pre-merge-checks"; diff --git a/src/review/screenshot-table-gate.ts b/src/review/screenshot-table-gate.ts index d31b45f7b9..28abd96400 100644 --- a/src/review/screenshot-table-gate.ts +++ b/src/review/screenshot-table-gate.ts @@ -1,373 +1,6 @@ -import { matchesAny } from "../signals/change-guardrail"; -import type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types"; - -export type { ScreenshotTableGateAction, ScreenshotTableGateConfig } from "../types"; - -// Config-driven before/after screenshot-table gate (#2006). Contributor visual/frontend PRs are unreviewable -// at a glance without before/after evidence — this is a DETERMINISTIC (no AI, zero hallucination risk) check -// that a PR's body contains a markdown table with image markup, scoped to the repo's configured labels/paths. -// Mirrors the shape of contributor-blacklist.ts / linked-issue-hard-rules-config.ts: a normalizer (DB JSON or -// `.loopover.yml` → validated config) plus a pure evaluator the trigger calls with live PR facts. Off by -// default (`enabled: false`) — a self-hoster opts in per repo, never hard-coded for any one project. - -const MAX_LABELS = 50; -const MAX_PATHS = 50; -const MAX_LABEL_CHARS = 100; -const MAX_PATH_CHARS = 300; -const MAX_MATRIX_DIMENSION = 12; -const MAX_MATRIX_TOKEN_CHARS = 40; -const MAX_SKILL_FILE_URL_CHARS = 300; - -// Extensions treated as "an image file" for the committed-image-file check below. Deliberately excludes SVG: -// an SVG can embed script/foreign-object content, so it is never accepted as review evidence anywhere in this -// repo (see the PR template's own UI Evidence rule) — a committed .svg is caught by neither this check nor the -// body-table one, exactly like the template's existing screenshots-must-be-raster rule. -const IMAGE_EXTENSIONS = [".png", ".jpg", ".jpeg", ".gif", ".webp"]; - -export const DEFAULT_SCREENSHOT_TABLE_GATE: ScreenshotTableGateConfig = { - enabled: false, - whenLabels: [], - whenPaths: [], - action: "close", - requireViewports: [], - requireThemes: [], -}; - -const VALID_ACTIONS: readonly ScreenshotTableGateAction[] = ["close", "advisory"]; - -export function isScreenshotTableGateAction(value: unknown): value is ScreenshotTableGateAction { - return typeof value === "string" && (VALID_ACTIONS as readonly string[]).includes(value); -} - -function normalizeStringList(value: unknown, field: string, max: number, maxChars: number, warnings: string[]): string[] { - if (value === undefined) return []; - if (!Array.isArray(value)) { - warnings.push(`settings.requireScreenshotTable.${field} must be an array; ignoring it.`); - return []; - } - const out: string[] = []; - for (const [index, item] of value.entries()) { - if (out.length >= max) { - warnings.push(`settings.requireScreenshotTable.${field} is capped at ${max} entries; dropping the rest.`); - break; - } - if (typeof item !== "string" || item.trim().length === 0) { - warnings.push(`settings.requireScreenshotTable.${field}[${index}] must be a non-empty string; ignoring it.`); - continue; - } - out.push(item.trim().slice(0, maxChars)); - } - return out; -} - -/** Normalize a raw `requireScreenshotTable` value (DB JSON or `.loopover.yml`) into a validated config. Never - * throws: malformed fields fall back to the default (disabled/empty), matching every other settings normalizer - * in this codebase. */ -export function normalizeScreenshotTableGateConfig(input: unknown, warnings: string[]): ScreenshotTableGateConfig { - if (input === undefined || input === null) return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }; - if (typeof input !== "object" || Array.isArray(input)) { - warnings.push("settings.requireScreenshotTable must be an object; using the default (disabled)."); - return { ...DEFAULT_SCREENSHOT_TABLE_GATE, whenLabels: [], whenPaths: [], requireViewports: [], requireThemes: [] }; - } - const record = input as Record; - const enabled = typeof record.enabled === "boolean" ? record.enabled : DEFAULT_SCREENSHOT_TABLE_GATE.enabled; - if (record.enabled !== undefined && typeof record.enabled !== "boolean") { - warnings.push(`settings.requireScreenshotTable.enabled must be a boolean; using the default "${DEFAULT_SCREENSHOT_TABLE_GATE.enabled}".`); - } - const action = isScreenshotTableGateAction(record.action) - ? record.action - : (() => { - if (record.action !== undefined) warnings.push(`settings.requireScreenshotTable.action must be "close" or "advisory" (#4110 removed request_changes/comment as dead config surface); using the default "close".`); - return DEFAULT_SCREENSHOT_TABLE_GATE.action; - })(); - const message = typeof record.message === "string" && record.message.trim().length > 0 ? record.message.trim() : undefined; - if (record.message !== undefined && message === undefined) { - warnings.push("settings.requireScreenshotTable.message must be a non-empty string; using the default message."); - } - const skillFileUrl = normalizeSkillFileUrl(record.skillFileUrl, warnings); - return { - enabled, - whenLabels: normalizeStringList(record.whenLabels, "whenLabels", MAX_LABELS, MAX_LABEL_CHARS, warnings), - whenPaths: normalizeStringList(record.whenPaths, "whenPaths", MAX_PATHS, MAX_PATH_CHARS, warnings), - action, - requireViewports: normalizeStringList(record.requireViewports, "requireViewports", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), - requireThemes: normalizeStringList(record.requireThemes, "requireThemes", MAX_MATRIX_DIMENSION, MAX_MATRIX_TOKEN_CHARS, warnings), - ...(message !== undefined ? { message } : {}), - ...(skillFileUrl !== undefined ? { skillFileUrl } : {}), - }; -} - -/** Validate a `skillFileUrl` override: same trust/validation level as `message` above (a trusted - * maintainer-authored config value, never fetched server-side -- it is only ever embedded as TEXT in a - * GitHub comment/close reason, so there is no SSRF surface here to guard against, unlike a URL the - * server would dereference). Malformed values are dropped with a warning, never silently coerced. */ -function normalizeSkillFileUrl(value: unknown, warnings: string[]): string | undefined { - if (value === undefined) return undefined; - if (typeof value !== "string" || value.trim().length === 0 || value.trim().length > MAX_SKILL_FILE_URL_CHARS) { - warnings.push(`settings.requireScreenshotTable.skillFileUrl must be a non-empty string no longer than ${MAX_SKILL_FILE_URL_CHARS} characters; ignoring it.`); - return undefined; - } - return value.trim(); -} - -/** Linear-time markdown table separator check. The previous single-regex form nested unbounded `\\s*` inside a - * repeated group and could catastrophically backtrack on attacker-controlled PR bodies; this splits on `|` and - * validates each cell independently instead. */ -const TABLE_SEPARATOR_CELL = /^\s*:?-{3,}:?\s*$/; - -function isMarkdownTableSeparatorRow(line: string): boolean { - const trimmed = line.trim(); - if (!trimmed || !/-{3,}/.test(trimmed)) return false; - const withoutEdgePipes = trimmed.replace(/^\|/, "").replace(/\|$/, "").trim(); - const cells = withoutEdgePipes.split("|"); - return cells.every((cell) => TABLE_SEPARATOR_CELL.test(cell)); -} - -/** True when `body` contains at least one markdown TABLE region (`| ... |` header + separator row) whose cells - * embed image markup — either `![alt](url)` or an `` tag — inside the table. A screenshot pasted as a - * bare inline image OUTSIDE any table does not count (the contract requires captioned thumbnails INSIDE a - * table, not a wall of raw images). Deliberately simple/regex-based (no markdown AST dependency) — false - * negatives fail toward "no table found" (in-scope PRs still need a real table), false positives fail toward - * "table found" (never blocks a PR that plausibly complied); both directions are acceptable for a - * first-pass deterministic heuristic that a maintainer can always override by hand. */ -export function hasImageBearingMarkdownTable(body: string | null | undefined): boolean { - if (!body) return false; - const lines = body.split(/\r?\n/); - const tableRowPattern = /^\s*\|.*\|\s*$/; - const imagePattern = /!\[[^\]]*\]\([^)]+\)|]*>/i; - for (let i = 0; i < lines.length - 1; i += 1) { - // `i < lines.length - 1` guarantees both indices are in bounds; the `?? ""` fallbacks only exist to - // satisfy noUncheckedIndexedAccess and are never actually reached. - /* v8 ignore next -- defensive: the loop bound above guarantees lines[i] always exists here. */ - const header = lines[i] ?? ""; - /* v8 ignore next -- defensive: the loop bound above guarantees lines[i + 1] always exists here. */ - const separator = lines[i + 1] ?? ""; - if (!tableRowPattern.test(header) || !isMarkdownTableSeparatorRow(separator)) continue; - // Found a table (header + separator). Scan its body rows (until a blank line or a non-table line) for - // image markup in any cell. - let j = i + 2; - /* v8 ignore next -- defensive: the `j < lines.length` guard above guarantees lines[j] always exists here. */ - while (j < lines.length && tableRowPattern.test(lines[j] ?? "")) { - if (imagePattern.test(lines[j] ?? "")) return true; - j += 1; - } - } - return false; -} - -/** True when `body` has a large inline image OUTSIDE of any markdown table — a common way contributors dodge - * the table requirement (paste screenshots directly into the body instead of inside a captioned table row). */ -export function hasImageOutsideTable(body: string | null | undefined): boolean { - if (!body) return false; - const lines = body.split(/\r?\n/); - const tableRowPattern = /^\s*\|.*\|\s*$/; - const imagePattern = /!\[[^\]]*\]\([^)]+\)|]*>/i; - return lines.some((line) => imagePattern.test(line) && !tableRowPattern.test(line)); -} - -/** True when any changed file path is an image under a scoped path (a screenshot committed to the repo instead - * of uploaded to the PR body via GitHub's CDN, per the contract). `scopedPaths` should be the SAME glob list - * used for scope matching (`whenPaths`) so this only flags an image landing where visual work is expected — - * not an unrelated asset (e.g. a favicon) added anywhere else in the repo. Empty `scopedPaths` (no path scoping - * configured) checks every changed path. */ -export function hasCommittedImageFile(changedFiles: string[], scopedPaths: string[]): boolean { - return changedFiles.some((file) => { - const lower = file.toLowerCase(); - if (!IMAGE_EXTENSIONS.some((ext) => lower.endsWith(ext))) return false; - return scopedPaths.length === 0 || matchesAny(file, scopedPaths); - }); -} - -const IMAGE_CELL_PATTERN = /!\[[^\]]*\]\([^)]+\)|]*>/i; - -/** One data row of a detected markdown table: the cell texts in source order (leading/trailing pipes and - * whitespace stripped). Deliberately a SEPARATE table-detection pass from {@link hasImageBearingMarkdownTable} - * rather than a shared refactor of it -- that function's exact behavior is pinned by existing tests, and this - * one needs actual cell contents (not just "does some cell have an image"), so duplicating its short - * header+separator detection loop keeps both independently simple instead of risking a regression in either - * from a shared-code change. */ -export function extractTableRows(body: string | null | undefined): string[][] { - if (!body) return []; - const lines = body.split(/\r?\n/); - const tableRowPattern = /^\s*\|.*\|\s*$/; - const rows: string[][] = []; - for (let i = 0; i < lines.length - 1; i += 1) { - /* v8 ignore next -- defensive: the loop bound above guarantees lines[i] always exists here. */ - const header = lines[i] ?? ""; - /* v8 ignore next -- defensive: the loop bound above guarantees lines[i + 1] always exists here. */ - const separator = lines[i + 1] ?? ""; - if (!tableRowPattern.test(header) || !isMarkdownTableSeparatorRow(separator)) continue; - let j = i + 2; - /* v8 ignore next -- defensive: the `j < lines.length` guard above guarantees lines[j] always exists here. */ - while (j < lines.length && tableRowPattern.test(lines[j] ?? "")) { - /* v8 ignore next -- defensive: same loop-bound guarantee as above. */ - const line = lines[j] ?? ""; - const cells = line - .trim() - .replace(/^\|/, "") - .replace(/\|$/, "") - .split("|") - .map((cell) => cell.trim()); - rows.push(cells); - j += 1; - } - i = j - 1; - } - return rows; -} - -// Matches EITHER markdown image syntax (`![alt](url)`, optionally with a trailing `"title"`) OR an `` tag, capturing the URL from whichever alternative matched -- covers a bare `![]()` cell and the -// PR template's own clickable-thumbnail convention (`[![before](url)](url)`, where the OUTER `[...](...)` is -// the click-through link and this pattern correctly targets the INNER `!`-prefixed image markup instead). -const CELL_IMAGE_URL_PATTERN = /!\[[^\]]*\]\(([^)\s]+)(?:\s+"[^"]*")?\)|]*\bsrc=["']([^"']+)["'][^>]*>/i; - -function extractCellImageUrl(cell: string): string | null { - const match = cell.match(CELL_IMAGE_URL_PATTERN); - if (!match) return null; - /* v8 ignore next -- defensive: whichever alternative of CELL_IMAGE_URL_PATTERN matched always captures a - * non-empty group (both require at least one non-`)`/non-`"` character), so this fallback is unreachable. */ - return match[1] ?? match[2] ?? null; -} - -/** The image URLs found in each detected table row (source order), for rows with at least two — a real - * before/after pair worth comparing, not a single decorative image or caption-only row. Reuses - * {@link extractTableRows}'s own header+separator detection rather than re-scanning the body. A row with - * MORE than two images (e.g. a desktop+mobile matrix row) keeps every image; callers that only want a pair - * slice it themselves. */ -export function extractTableRowImageUrls(body: string | null | undefined): string[][] { - return extractTableRows(body) - .map((row) => row.map(extractCellImageUrl).filter((url): url is string => url !== null)) - .filter((urls) => urls.length >= 2); -} - -/** One (viewport, theme) combination the matrix must cover. `theme: null` means the theme dimension isn't - * required at all (a repo can require viewport coverage without color-mode coverage). */ -export type ScreenshotMatrixPair = { viewport: string; theme: string | null }; - -/** The full set of (viewport, theme) pairs `config` requires, or `[]` when matrix mode is off. Matrix mode - * turns on via `requireViewports` alone -- `requireThemes` with an empty `requireViewports` has no effect, - * since there is no viewport to cross it against. */ -export function requiredScreenshotMatrixPairs(config: ScreenshotTableGateConfig): ScreenshotMatrixPair[] { - if (config.requireViewports.length === 0) return []; - if (config.requireThemes.length === 0) return config.requireViewports.map((viewport) => ({ viewport, theme: null })); - const pairs: ScreenshotMatrixPair[] = []; - for (const viewport of config.requireViewports) { - for (const theme of config.requireThemes) pairs.push({ viewport, theme }); - } - return pairs; -} - -/** True when some row's first cell (the row LABEL, e.g. "Desktop · Light") mentions both `pair.viewport` and - * `pair.theme` (case-insensitive substring match -- tolerant of whatever separator character the contributor - * used between them) AND that row has at least two image-bearing cells among the rest (before + after). */ -function rowSatisfiesMatrixPair(row: string[], pair: ScreenshotMatrixPair): boolean { - // `?? ""` only exists to satisfy noUncheckedIndexedAccess -- `extractTableRows`'s `.split("|")` always - // produces at least one cell, even for an empty-string row, so `row[0]` is never actually undefined here. - /* v8 ignore next -- defensive: see the comment above. */ - const label = (row[0] ?? "").toLowerCase(); - if (!label.includes(pair.viewport.toLowerCase())) return false; - if (pair.theme !== null && !label.includes(pair.theme.toLowerCase())) return false; - const imageCells = row.slice(1).filter((cell) => IMAGE_CELL_PATTERN.test(cell)).length; - return imageCells >= 2; -} - -/** The subset of `pairs` with NO satisfying row anywhere in `body`'s tables. Empty ⇒ full coverage. */ -export function missingScreenshotMatrixPairs(body: string | null | undefined, pairs: ScreenshotMatrixPair[]): ScreenshotMatrixPair[] { - if (pairs.length === 0) return []; - const rows = extractTableRows(body); - return pairs.filter((pair) => !rows.some((row) => rowSatisfiesMatrixPair(row, pair))); -} - -function formatMatrixPair(pair: ScreenshotMatrixPair): string { - return pair.theme === null ? pair.viewport : `${pair.viewport} · ${pair.theme}`; -} - -/** Build the rejection reason for a matrix violation, naming exactly which viewport/theme combinations are - * still missing a real before+after pair -- so the contributor knows precisely what to add, not just that - * "something" is missing. */ -export function buildScreenshotMatrixMessage(missing: ScreenshotMatrixPair[]): string { - const list = missing.map(formatMatrixPair).join(", "); - const dimensionLabel = missing.some((pair) => pair.theme !== null) ? "viewport × theme" : "viewport"; - return ( - "This pull request changes UI/visual code but its screenshot evidence is incomplete. Every required " + - `${dimensionLabel} combination needs its own before/after image pair in a labeled table row (e.g. ` + - '"Desktop · Light | before | after"). Still missing: ' + - `${list}.\n\nPlease resubmit with the remaining rows filled in.` - ); -} - -/** Append a contributor skill-file link to an auto-generated rejection message (#4540 follow-up). A no-op - * when `skillFileUrl` is unset -- callers only reach this on the AUTO-GENERATED path (a `message` - * override already owns its entire text and is never passed through here). */ -function appendSkillLink(text: string, skillFileUrl: string | undefined): string { - return skillFileUrl ? `${text}\n\nSee ${skillFileUrl} for the exact format and examples.` : text; -} - -/** True when the PR is IN SCOPE for the gate: it carries one of `config.whenLabels` OR touches a path matching - * one of `config.whenPaths`. Both empty ⇒ every PR is in scope (an operator who enables the gate with no - * scoping at all wants it enforced everywhere). Only one non-empty list configured ⇒ that list alone decides - * scope (the other, empty list can never exclude a PR the configured one matched). */ -export function isScreenshotTableGateInScope(config: ScreenshotTableGateConfig, prLabels: string[], changedFiles: string[]): boolean { - if (config.whenLabels.length === 0 && config.whenPaths.length === 0) return true; - const wantedLabels = new Set(config.whenLabels.map((label) => label.toLowerCase())); - const labelMatch = config.whenLabels.length > 0 && prLabels.some((label) => wantedLabels.has(label.toLowerCase())); - const pathMatch = config.whenPaths.length > 0 && changedFiles.some((file) => matchesAny(file, config.whenPaths)); - return labelMatch || pathMatch; -} - -export const DEFAULT_SCREENSHOT_CONTRACT_MESSAGE = - "This pull request changes UI/visual code but its description is missing a before/after screenshot table. " + - "Every changed page/feature needs a **markdown table** with a before column and an after column, each cell a " + - "clickable thumbnail (uploaded to the PR, not committed to the repo) with a caption below — for example:\n\n" + - "| Before | After |\n| --- | --- |\n| [![before](url)](url) — caption | [![after](url)](url) — caption |\n\n" + - "Please resubmit with the table filled in."; - -export type ScreenshotTableGateResult = { - violated: boolean; - reason: string | null; -}; - -const NO_VIOLATION: ScreenshotTableGateResult = { violated: false, reason: null }; - -/** PURE evaluator. Off (`enabled: false`) or out-of-scope (no configured label/path match) ⇒ no violation. - * `botCaptureSatisfied` ⇒ no violation regardless of mode (an automated capture is equivalent to a - * hand-authored table, and the bot doesn't (yet) shoot a full viewport/theme matrix -- see #4535's scope note). - * - * Two modes, chosen by whether `config.requireViewports` is non-empty (#4535): - * - MATRIX mode: every required (viewport, theme) pair (`requiredScreenshotMatrixPairs`) must have a labeled - * before/after row. Violated ⇒ the reason names exactly which pairs are still missing. - * - PRESENCE mode (the original #2006 behavior, unchanged): in scope AND (no image-bearing table in the body - * OR an image pasted outside a table OR a committed image file under a scoped path) ⇒ violated, with the - * configured (or default) templated message as the reason. */ -export function evaluateScreenshotTableGate(input: { - config: ScreenshotTableGateConfig; - prBody: string | null | undefined; - prLabels: string[]; - changedFiles: string[]; - /** #4110: true when the bot's own before/after capture pipeline (review.visual.enabled) already produced a - * REAL before+after render pair for this PR's current head — evidence equivalent to a hand-authored table. - * A successful automated capture satisfies the gate on its own, ahead of (and regardless of) the body-table - * anti-gaming checks below — those exist to stop a contributor from FAKING compliance without the bot's - * help, which doesn't apply once the bot has already proven the change visually. Absent/false ⇒ - * byte-identical to pre-#4110 behavior (body-table evidence only). */ - botCaptureSatisfied?: boolean | undefined; -}): ScreenshotTableGateResult { - const { config } = input; - if (!config.enabled) return NO_VIOLATION; - if (!isScreenshotTableGateInScope(config, input.prLabels, input.changedFiles)) return NO_VIOLATION; - if (input.botCaptureSatisfied === true) return NO_VIOLATION; - - const matrixPairs = requiredScreenshotMatrixPairs(config); - if (matrixPairs.length > 0) { - const missing = missingScreenshotMatrixPairs(input.prBody, matrixPairs); - if (missing.length === 0) return NO_VIOLATION; - return { violated: true, reason: config.message ?? appendSkillLink(buildScreenshotMatrixMessage(missing), config.skillFileUrl) }; - } - - const hasTable = hasImageBearingMarkdownTable(input.prBody); - const outsideTable = hasImageOutsideTable(input.prBody); - const committedImage = hasCommittedImageFile(input.changedFiles, config.whenPaths); - if (hasTable && !outsideTable && !committedImage) return NO_VIOLATION; - return { violated: true, reason: config.message ?? appendSkillLink(DEFAULT_SCREENSHOT_CONTRACT_MESSAGE, config.skillFileUrl) }; -} +// screenshot-table-gate, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained twin of +// the engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/screenshot-table-gate.ts (imported via relative source path, not the +// published package, to match this repo's existing engine-consumption convention — see +// src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/screenshot-table-gate"; diff --git a/src/review/unlinked-issue-guardrail-config.ts b/src/review/unlinked-issue-guardrail-config.ts index f3463d126c..b84ff576b1 100644 --- a/src/review/unlinked-issue-guardrail-config.ts +++ b/src/review/unlinked-issue-guardrail-config.ts @@ -1,47 +1,6 @@ -import type { UnlinkedIssueGuardrailConfig, UnlinkedIssueGuardrailMode } from "../types"; - -const VALID_UNLINKED_ISSUE_GUARDRAIL_MODES: readonly UnlinkedIssueGuardrailMode[] = ["hold", "off"]; -const DEFAULT_MIN_CONFIDENCE = 0.85; - -export const DEFAULT_UNLINKED_ISSUE_GUARDRAIL: UnlinkedIssueGuardrailConfig = { - mode: "off", - minConfidence: DEFAULT_MIN_CONFIDENCE, -}; - -export function isUnlinkedIssueGuardrailMode(value: unknown): value is UnlinkedIssueGuardrailMode { - return typeof value === "string" && (VALID_UNLINKED_ISSUE_GUARDRAIL_MODES as readonly string[]).includes(value); -} - -function normalizeMode(value: unknown, warnings: string[]): UnlinkedIssueGuardrailMode { - if (value === undefined) return DEFAULT_UNLINKED_ISSUE_GUARDRAIL.mode; - if (isUnlinkedIssueGuardrailMode(value)) return value; - warnings.push(`settings.unlinkedIssueGuardrail.mode must be one of hold, off; using the default "${DEFAULT_UNLINKED_ISSUE_GUARDRAIL.mode}".`); - return DEFAULT_UNLINKED_ISSUE_GUARDRAIL.mode; -} - -function normalizeMinConfidence(value: unknown, warnings: string[]): number { - if (value === undefined) return DEFAULT_UNLINKED_ISSUE_GUARDRAIL.minConfidence; - if (typeof value !== "number" || !Number.isFinite(value) || value < 0 || value > 1) { - warnings.push(`settings.unlinkedIssueGuardrail.minConfidence must be a number between 0 and 1; using the default "${DEFAULT_MIN_CONFIDENCE}".`); - return DEFAULT_MIN_CONFIDENCE; - } - return value; -} - -/** - * Normalize a raw `.loopover.yml settings.unlinkedIssueGuardrail` value into a typed config, - * fail-safe: any malformed field falls back to its own default and pushes a warning rather than - * rejecting the whole block. Mirrors `normalizeLinkedIssueHardRulesConfig`'s per-field discipline. - */ -export function normalizeUnlinkedIssueGuardrailConfig(input: unknown, warnings: string[]): UnlinkedIssueGuardrailConfig { - if (input === undefined) return { ...DEFAULT_UNLINKED_ISSUE_GUARDRAIL }; - if (typeof input !== "object" || input === null || Array.isArray(input)) { - warnings.push("settings.unlinkedIssueGuardrail must be an object; using the default off policy."); - return { ...DEFAULT_UNLINKED_ISSUE_GUARDRAIL }; - } - const record = input as Record; - return { - mode: normalizeMode(record.mode, warnings), - minConfidence: normalizeMinConfidence(record.minConfidence, warnings), - }; -} +// unlinked-issue-guardrail-config, converged onto @loopover/engine (#6203). This src/ file was a hand-maintained +// twin of the engine copy; it is now a thin re-export shim so the single implementation lives at +// packages/loopover-engine/src/review/unlinked-issue-guardrail-config.ts (imported via relative source path, not +// the published package, to match this repo's existing engine-consumption convention — see +// src/settings/auto-close-exempt.ts). +export * from "../../packages/loopover-engine/src/review/unlinked-issue-guardrail-config"; diff --git a/test/unit/agent-actions.test.ts b/test/unit/agent-actions.test.ts index e02e89f7af..2c2db57ec3 100644 --- a/test/unit/agent-actions.test.ts +++ b/test/unit/agent-actions.test.ts @@ -1819,7 +1819,7 @@ describe("CONCRETE_EVIDENCE_BLOCKER_CODES parity — hand-typed literals still m { code: "surface_lane_reject", file: "src/review/content-lane-wire.ts" }, { code: "manifest_missing_tests", file: "src/signals/focus-manifest.ts" }, { code: "manifest_linked_issue_required", file: "src/signals/focus-manifest.ts" }, - { code: "pre_merge_check_required", file: "src/review/pre-merge-checks.ts" }, + { code: "pre_merge_check_required", file: "packages/loopover-engine/src/review/pre-merge-checks.ts" }, { code: "lockfile_tamper_risk", file: "src/review/lockfile-tamper.ts" }, { code: "missing_linked_issue", file: "src/rules/advisory.ts" }, { code: "self_authored_linked_issue", file: "src/rules/advisory.ts" }, diff --git a/test/unit/check-engine-parity-script.test.ts b/test/unit/check-engine-parity-script.test.ts index cc14be4bd5..65dfbd4793 100644 --- a/test/unit/check-engine-parity-script.test.ts +++ b/test/unit/check-engine-parity-script.test.ts @@ -92,14 +92,16 @@ describe("check-engine-parity script", () => { it("discovers real in-scope pairs in the repository (regression guard)", () => { const pairs = discoverEngineParityPairs({ root: process.cwd() }); - // Floor tracks the count of still-hand-duplicated in-scope twins, minus a small margin so unrelated - // additions don't trip it while a broken scanner returning ~0 still does. #6194 converged the last four - // settings twins (autonomy/command-authorization/contributor-blacklist/pr-type-label) onto their engine - // shims, dropping the floor from 14 to 10. #6204 converged two more (change-guardrail.ts and - // preflight-limits.ts, both in src/signals/) onto their shims, dropping the real count to 9 — the - // `.some()` structural checks below are the real guard. - expect(pairs.length).toBeGreaterThanOrEqual(9); - expect(pairs.some((pair: EngineParityPair) => pair.fileName === "guardrail-config.ts")).toBe(true); + // Tracked the count of still-hand-duplicated in-scope twins as a floor (with margin) through #6194 (14→10), + // #6204 (change-guardrail.ts/preflight-limits.ts in src/signals/), and #6203 (the 9 remaining src/review/ + // twins: advisory-ai-routing-config/cla-check/enrichment-analyzer-names/guardrail-config/ + // linked-issue-hard-rules-config/linked-issue-label-propagation/pre-merge-checks/screenshot-table-gate/ + // unlinked-issue-guardrail-config) — the in-scope hand-duplicated set is now genuinely empty, so `toBe(0)` + // is the precise (not vacuous, unlike a ">= 0" floor) regression guard: any real duplicate reappearing + // fails this immediately. The `.some()` checks below name specific already-converged files individually, + // so a regression on any ONE of them is diagnosable without re-running discovery by hand. + expect(pairs.length).toBe(0); + expect(pairs.some((pair: EngineParityPair) => pair.fileName === "guardrail-config.ts")).toBe(false); expect(pairs.some((pair: EngineParityPair) => pair.fileName === "change-guardrail.ts")).toBe(false); expect(pairs.some((pair: EngineParityPair) => pair.fileName === "duplicate-winner.ts")).toBe(false); expect(pairs.some((pair: EngineParityPair) => pair.fileName === "check-names.ts")).toBe(false);