From db04178f313d4c67f48311a63bbd7d509d921b33 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:24:33 -0700 Subject: [PATCH 1/5] refactor(content-lane): remove metagraphed-specific hardcoding from the registry-review engine gittensory is meant to be installed by any self-hosted repo maintainer, not just JSONbored/metagraphed. The RegistryLaneSpec abstraction was already generic, but nothing let a second maintainer actually reach it without editing gittensory's own TypeScript source and redeploying. - Rename runMetagraphedSurfaceGate to runRegistrySurfaceGate; stop re-exporting netuid-verification.ts's Bittensor-only helpers from the generic content-lane barrel (closes #2433). - Add assessAppendedEntry/assessProviderEntry callback fields to RegistryLaneSpec; the orchestrator calls the spec-supplied validators instead of hardcoded imports, so a different registry can supply its own domain validator without touching shared engine code (closes #2434). - Add a contentLane: block to .gittensory.yml (FocusManifestContentLaneConfig) and a resolveRegistryLaneSpec resolver mirroring resolveConvergedFeature's precedence (env kill-switch -> per-repo config -> allowlist default), so a second maintainer's registry repo can activate the deterministic surface lane purely from their own config, with today's zero-config behavior for metagraphed unchanged (closes #2435). - Glob fields (entryFileGlob/providerFileGlob/artifactGlob) are capped at parse time to a safe wildcard count: an adversarial-review pass on this change found the shared glob-to-RegExp compiler is exponential-time on chained wildcards (empirically ~19s at 5 chained wildcards), so the new config surface rejects an over-complex glob before it ever reaches RegExp compilation. --- src/review/content-lane-wire.ts | 84 ++++---- src/review/content-lane/index.ts | 19 +- src/review/content-lane/orchestrator.ts | 35 +++- src/review/content-lane/registry-logic.ts | 15 ++ src/review/content-lane/spec-resolver.ts | 66 ++++++ src/signals/change-guardrail.ts | 6 +- src/signals/focus-manifest-loader.ts | 3 +- src/signals/focus-manifest.ts | 138 ++++++++++++- test/unit/content-lane-orchestrator.test.ts | 31 ++- test/unit/content-lane-spec-resolver.test.ts | 125 ++++++++++++ test/unit/content-lane-wire.test.ts | 200 ++++++++++++++++--- test/unit/focus-manifest.test.ts | 95 +++++++++ 12 files changed, 724 insertions(+), 93 deletions(-) create mode 100644 src/review/content-lane/spec-resolver.ts create mode 100644 test/unit/content-lane-spec-resolver.test.ts diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index 2d60c8172a..4dc583ca1c 100644 --- a/src/review/content-lane-wire.ts +++ b/src/review/content-lane-wire.ts @@ -1,13 +1,16 @@ -// Content/registry surface-lane HOST ADAPTER (#1255 convergence). `runSurfaceReview` is a pure, AI-FREE, -// structured-data adjudicator for registry-submission PRs (metagraphed's surfaces[]/providers/candidates). This -// file is the thin host wiring that lets its deterministic verdict drive the SAME gate disposition (check-run + -// auto-action + public comment) the generic gate produces: the flag + per-repo allowlist guard, the GitHub-backed -// loadFile, and the verdict → GateCheckEvaluation conversion. +// Content/registry surface-lane HOST ADAPTER (#1255 convergence, spec resolution #2435). `runSurfaceReview` is a +// pure, AI-FREE, structured-data adjudicator for registry-submission PRs. This file is the thin host wiring that +// lets its deterministic verdict drive the SAME gate disposition (check-run + auto-action + public comment) the +// generic gate produces: the flag + per-repo RegistryLaneSpec resolution, the GitHub-backed loadFile, and the +// verdict → GateCheckEvaluation conversion. // -// FLAG-GATED + DEFAULT-OFF: GITTENSORY_REVIEW_CONTENT_LANE must be truthy AND the repo must be in the per-repo -// GITTENSORY_REVIEW_REPOS cutover allowlist. When off (the default) the caller takes no new branch, runs no -// fetch, and `gateEvaluation` is byte-identical to today. The verdict NEVER depends on an AI model, so this is -// independent of the AI-reviewer accuracy work (the surface lane emits none of the AI_JUDGMENT_BLOCKER_CODES). +// FLAG-GATED + DEFAULT-OFF: GITTENSORY_REVIEW_CONTENT_LANE must be truthy, AND `resolveRegistryLaneSpec` +// (content-lane/spec-resolver.ts) must resolve a spec for this repo — either an explicit per-repo `.gittensory.yml` +// `contentLane:` config, or (today's zero-config default) the repo being in the GITTENSORY_REVIEW_REPOS cutover +// allowlist, which resolves to METAGRAPHED_LANE_SPEC. When off / unresolved (the default for any repo that hasn't +// opted in) the caller takes no new branch, runs no fetch, and `gateEvaluation` is byte-identical to today. The +// verdict NEVER depends on an AI model, so this is independent of the AI-reviewer accuracy work (the surface lane +// emits none of the AI_JUDGMENT_BLOCKER_CODES). // // SAFETY (three deliberate guards): // 1. A generic HARD (non-AI-judgment) blocker — e.g. a committed secret detected before this runs — is PRESERVED: @@ -22,12 +25,14 @@ // adjudicator for this structured data — an AI opinion has no standing to veto it, only a real deterministic // blocker does (see guard #1). import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation, isAiJudgmentOnlyFailure } from "../rules/advisory"; -import type { AdvisoryFinding, AdvisorySeverity } from "../types"; -import { type ContentLaneEnv, isContentLaneEnabled } from "./content-lane/flag"; +import { isContentLaneEnabled } from "./content-lane/flag"; import { runSurfaceReview, type SurfaceReviewInput, type SurfaceReviewResult } from "./content-lane/orchestrator"; -import { METAGRAPHED_LANE_SPEC } from "./content-lane/registry-logic"; -import { isConvergenceRepoAllowed } from "./cutover-gate"; +import type { RegistryLaneSpec } from "./content-lane/registry-logic"; +import { resolveRegistryLaneSpec } from "./content-lane/spec-resolver"; import { makeGithubFileFetcher } from "./grounding-wire"; +import type { FocusManifest } from "../signals/focus-manifest"; +import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; +import type { AdvisoryFinding, AdvisorySeverity } from "../types"; // Deterministic surface-lane finding codes. DELIBERATELY NOT in AI_JUDGMENT_BLOCKER_CODES; surface closes are // facts, and blocker findings must never be flipped to merge by green CI. @@ -35,15 +40,6 @@ const SURFACE_REJECT_CODE = "surface_lane_reject"; const SURFACE_MANUAL_CODE = "surface_lane_manual"; const SURFACE_TITLE = "Registry surface review"; -/** True when the deterministic surface lane should drive the gate for `repoFullName`: the flag is on AND the - * repo is in the per-repo cutover allowlist. Flag-OFF (default) ⇒ the caller takes no new branch. */ -export function isContentLaneWired( - env: ContentLaneEnv & { GITTENSORY_REVIEW_REPOS?: string | undefined }, - repoFullName: string, -): boolean { - return isContentLaneEnabled(env) && isConvergenceRepoAllowed(env, repoFullName); -} - function surfaceFinding(code: string, severity: AdvisorySeverity, summary: string): AdvisoryFinding { return { code, title: SURFACE_TITLE, severity, detail: summary, publicText: summary }; } @@ -106,14 +102,16 @@ export function applySurfaceGate( }; } -/** Run the deterministic surface review for a registry-submission PR and return its gate evaluation, or `null` - * to defer to the generic gate (not a submission, or an unreadable file — see below). Mutates `advisory.findings` - * so the reason renders in the unified public comment. NEVER throws on a fetch blip — the file fetcher is - * fail-safe. `loadFileOverride` is injected by unit tests; production builds a lazy GitHub-Contents-backed loader - * so a non-submission PR (the common case) pays for no fetch at all. `files` carries each changed file's GitHub +/** Run the deterministic surface review for a registry-submission PR against `spec` (the caller's already-resolved + * RegistryLaneSpec — see `resolveRegistryLaneSpec`) and return its gate evaluation, or `null` to defer to the + * generic gate (not a submission, or an unreadable file — see below). Mutates `advisory.findings` so the reason + * renders in the unified public comment. NEVER throws on a fetch blip — the file fetcher is fail-safe. + * `loadFileOverride` is injected by unit tests; production builds a lazy GitHub-Contents-backed loader so a + * non-submission PR (the common case) pays for no fetch at all. `files` carries each changed file's GitHub * status so a null BASE read can be told apart from an absent base (see the defer guard). */ -export async function runMetagraphedSurfaceGate( +export async function runRegistrySurfaceGate( env: Env, + spec: RegistryLaneSpec, args: { installationId: number | null | undefined; repoFullName: string; @@ -138,12 +136,13 @@ export async function runMetagraphedSurfaceGate( // null read is a transient fetch blip, NOT an absent base) — would make a valid submission read as empty/ // invalid → a spurious one-shot close. Defer to the generic gate instead. A null base for an ADDED file is // the expected brand-new-entry case and is left to the orchestrator, whose spec-driven entry-count policy - // decides the verdict (METAGRAPHED_LANE_SPEC allows any number of clean entries — see maxAppendedEntries). + // decides the verdict (the resolved spec's own maxAppendedEntries — e.g. METAGRAPHED_LANE_SPEC allows any + // number of clean entries). if (ref === "head" && content === null) deferUnreadable = true; if (ref === "base" && content === null && statusByPath.get(path) === "modified") deferUnreadable = true; return content; }; - const result = await runSurfaceReview(METAGRAPHED_LANE_SPEC, { + const result = await runSurfaceReview(spec, { changedFiles: args.files.map((file) => file.path), loadFile, opts: { secretsScan: true, sourceUrlValidation: true }, @@ -165,9 +164,12 @@ export function resolveSurfaceRefs( return { headSha: pr.headSha ?? "", baseRef: pr.baseRef ?? repo?.defaultBranch ?? "" }; } -/** The processor SEAM in one testable call: when the surface lane is wired for this repo, run it and merge its - * verdict onto the generic gate (preserving generic hard blockers); otherwise return the generic evaluation - * unchanged. `getChangedFiles` is a thunk so an unwired repo resolves no files (no extra diff load). +/** The processor SEAM in one testable call: when a RegistryLaneSpec resolves for this repo (see + * `resolveRegistryLaneSpec` — an explicit per-repo `.gittensory.yml` `contentLane:` config, or the + * GITTENSORY_REVIEW_REPOS allowlist default), run the surface lane against it and merge its verdict onto the + * generic gate (preserving generic hard blockers); otherwise return the generic evaluation unchanged. + * `getChangedFiles` is a thunk so an unresolved repo resolves no files (no extra diff load). The env kill-switch + * is checked BEFORE loading the manifest, so a globally-disabled lane pays no manifest-load I/O either. * * When `applySurfaceGate`'s AI-judgment override fires (an AI-judgment-only generic failure is overridden by a * decisive surface merge), the AI-judgment finding(s) are ALSO removed from `args.advisory.findings` — that @@ -175,7 +177,12 @@ export function resolveSurfaceRefs( * (src/review/unified-comment-bridge.ts) to render the "Code review" reviewer note, bypassing the gate * evaluation entirely. Without this cleanup, the public comment would still show "Concerns raised — review * before merging" quoting the overridden AI defect even though the gate the same comment reports is a clean - * merge — a visible, confusing contradiction of the override this function just made. */ + * merge — a visible, confusing contradiction of the override this function just made. + * + * `loadManifestOverride` is injected by unit tests (mirrors `runRegistrySurfaceGate`'s `loadFileOverride`) so + * they never hit the real cached-manifest loader's D1/network I/O; production omits it and gets the real, + * cached `loadRepoFocusManifest`. A manifest-load failure degrades to null (the allowlist-default resolution + * path), never a thrown error. */ export async function evaluateWithSurfaceLane( env: Env, repoFullName: string, @@ -188,9 +195,14 @@ export async function evaluateWithSurfaceLane( advisory: { findings: AdvisoryFinding[] }; getChangedFiles: () => Promise<{ path: string; status?: string | null | undefined }[]>; }, + loadManifestOverride?: (env: Env, repoFullName: string) => Promise, ): Promise { - if (!gateEnabled || !isContentLaneWired(env, repoFullName)) return gateEvaluation; - const surfaceGate = await runMetagraphedSurfaceGate(env, { + if (!gateEnabled || !isContentLaneEnabled(env)) return gateEvaluation; + const loadManifest = loadManifestOverride ?? loadRepoFocusManifest; + const manifest = await loadManifest(env, repoFullName).catch(() => null); + const spec = resolveRegistryLaneSpec(env, manifest, repoFullName); + if (!spec) return gateEvaluation; + const surfaceGate = await runRegistrySurfaceGate(env, spec, { installationId: args.installationId, repoFullName, pr: resolveSurfaceRefs(args.pr, args.repo), diff --git a/src/review/content-lane/index.ts b/src/review/content-lane/index.ts index 50d0e733b7..7b0f9ead34 100644 --- a/src/review/content-lane/index.ts +++ b/src/review/content-lane/index.ts @@ -12,8 +12,15 @@ // - duplicates (awesome) : duplicate-detection + protected-edit gate // - source-evidence (a.) : source-URL reachability gate (injectable fetch) // - security-scan (a.) : embedded-secret + pipe-to-shell scan -// - registry-logic (meta): candidate/provider gates, netuid GROUNDING, dedup keys, freshness, scope -// - netuid-verification : taostats + public-registry netuid identity (fail-open; taostats key optional) +// - registry-logic (meta): the GENERIC surface-model engine (RegistryLaneSpec, scope classification, duplicate +// detection) plus metagraphed's OWN domain-specific validators (candidate/provider +// gates, netuid GROUNDING, dedup keys, freshness) — the latter are metagraphed's own +// reference implementation, still exported here for a future registry to use as a +// template, not because they're generic. +// +// NOT re-exported here (metagraphed's own domain plumbing, no reason for a different registry to import it): +// taostats + public-registry netuid GROUNDING lookups (fail-open; taostats key optional) — import directly from +// ./netuid-verification if you're specifically working on metagraphed's own validators. // // DEFERRED / engine-entangled (NOT ported here — see the port report): the dual-AI review // orchestration (needs the inference adapter + the gate engine), content-RAG (Vectorize/D1/Queue), @@ -110,11 +117,3 @@ export { type Verdict, } from "./registry-logic"; export { runSurfaceReview, diffAppendedSurfaceEntries, type SurfaceReviewInput, type SurfaceReviewResult } from "./orchestrator"; -export { - checkNetuidExists, - fetchSubnetRecord, - fetchTaostatsSubnetIdentity, - type NetuidVerificationEnv, - type SubnetRecord, - type TaostatsIdentity, -} from "./netuid-verification"; diff --git a/src/review/content-lane/orchestrator.ts b/src/review/content-lane/orchestrator.ts index d591b059ab..8f69392873 100644 --- a/src/review/content-lane/orchestrator.ts +++ b/src/review/content-lane/orchestrator.ts @@ -5,9 +5,10 @@ // 3. resolves the appended surfaces[] entries by diffing head vs base, capped at the spec's maxAppendedEntries // (omitted ⇒ today's strict single-entry-only default), // 4. rejects a duplicate appended entry when the spec opts into duplicateKeyFields (omitted ⇒ off), and -// 5. validates EACH remaining appended entry independently and returns one aggregate verdict from -// assessSubnetDocument / assessProviderDocument: close if any entry is invalid, manual if any (remaining) -// needs manual review, merge only when every entry is clean. +// 5. validates EACH remaining appended entry independently via the spec's OWN assessAppendedEntry / +// assessProviderEntry validators (the orchestrator never hardcodes a domain-specific validator — a spec +// with no validator configured gets "manual") and returns one aggregate verdict: close if any entry is +// invalid, manual if any (remaining) needs manual review, merge only when every entry is clean. // Pure + injectable: unit tests pass a loadFile stub, so no network. The live wiring (a per-repo, // flag-gated branch in the review body) is a separate follow-up. import { @@ -15,8 +16,6 @@ import { type ProviderAssessment, type RegistryLaneSpec, type Verdict, - assessProviderDocument, - assessSubnetDocument, classifyRegistryPrScope, findDuplicateAppendedEntry, toCoreVerdict, @@ -93,6 +92,12 @@ function duplicateEntryCloseSummary(duplicate: unknown): string { return `A surface submission must not duplicate an entry already in this PR or already in the registry${detail} — resubmit without the duplicate.`; } +// A spec with no domain-specific validator configured yet (RegistryLaneSpec.assessAppendedEntry / +// assessProviderEntry) still gets structural gating (scope, entry-count cap, duplicate detection), but the +// orchestrator can't itself judge the entry's content — route to manual review rather than merge or close. +const NO_VALIDATOR_ENTRY_SUMMARY = "No validator is configured for this registry's surface entries — routing to review."; +const NO_VALIDATOR_PROVIDER_SUMMARY = "No validator is configured for this registry's provider submissions — routing to review."; + /** * Aggregate N independent per-entry assessments into ONE verdict: close if ANY entry is invalid, manual if ANY * (of the remainder) needs manual review (e.g. auth_required), merge only if EVERY entry is clean — mirroring the @@ -126,9 +131,11 @@ function pickAggregateAssessment(assessments: Assessment[]): Assessment { * (a malformed/violating entry, an out-of-range append count, a duplicate entry when the spec opts into * duplicateKeyFields, a bundled "mixed-files" PR, an invalid provider) CLOSES with a resubmit message. A PR that * is NOT a registry submission at all returns `null` — the surface lane does not apply, so the caller falls - * through to the generic gate. The only residual MANUAL comes from the per-entry validator (an authenticated - * interface needing a human to confirm the public auth scheme) — a "very few" case, and one bad entry among - * several still closes the whole PR (see pickAggregateAssessment). + * through to the generic gate. Residual MANUAL comes from two places: the spec's OWN per-entry validator (e.g. + * an authenticated interface needing a human to confirm the public auth scheme — a "very few" case, and one bad + * entry among several still closes the whole PR, see pickAggregateAssessment) — or, structurally, a spec with no + * `assessAppendedEntry`/`assessProviderEntry` configured yet, which still gets scope/count/duplicate gating but + * can't itself judge entry content. */ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceReviewInput): Promise { const scope = classifyRegistryPrScope(spec, input.changedFiles); @@ -149,7 +156,11 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev } const headRaw = await input.loadFile(directFile, "head"); if (scope.isProvider) { - return fromProvider(assessProviderDocument(safeParseJson(headRaw), input.opts)); + const assessProvider = spec.assessProviderEntry; + if (!assessProvider) { + return { verdict: "manual", summary: NO_VALIDATOR_PROVIDER_SUMMARY }; + } + return fromProvider(assessProvider(safeParseJson(headRaw), input.opts)); } const baseRaw = await input.loadFile(directFile, "base"); const appendedEntries = diffAppendedSurfaceEntries(headRaw, baseRaw, spec.collectionField); @@ -162,9 +173,13 @@ export async function runSurfaceReview(spec: RegistryLaneSpec, input: SurfaceRev if (duplicate !== null) { return { verdict: "close", summary: duplicateEntryCloseSummary(duplicate[0]) }; } + const assessEntry = spec.assessAppendedEntry; + if (!assessEntry) { + return { verdict: "manual", summary: NO_VALIDATOR_ENTRY_SUMMARY }; + } const headDoc = safeParseJson(headRaw); const assessment = pickAggregateAssessment( - appendedEntries.map((appendedEntry) => assessSubnetDocument(headDoc, { ...input.opts, appendedEntry })), + appendedEntries.map((appendedEntry) => assessEntry(headDoc, { ...input.opts, appendedEntry })), ); return { verdict: toCoreVerdict(assessment.verdict), summary: assessment.summary, reason: assessment.reason }; } diff --git a/src/review/content-lane/registry-logic.ts b/src/review/content-lane/registry-logic.ts index 86b311c5ef..cd5f5039d1 100644 --- a/src/review/content-lane/registry-logic.ts +++ b/src/review/content-lane/registry-logic.ts @@ -683,6 +683,17 @@ export interface RegistryLaneSpec { * JSON comparison for a non-string value). Generic — the engine only duck-types the configured field names off * each entry, it never assumes any domain-specific shape (kind/netuid/etc. are metagraphed's own vocabulary). */ duplicateKeyFields?: readonly string[]; + /** Validates ONE appended surfaces[] entry against the whole document (root shape + the specific entry) and + * returns its Assessment — the registry's own domain-specific semantic check (shape/safety/business rules). + * The orchestrator calls this once per appended entry and aggregates the results; it never validates anything + * itself, so a different registry supplies its own function here without touching the orchestrator. Omitted + * ⇒ the orchestrator returns "manual" for entry submissions to this registry (structural gating — scope, + * entry-count cap, duplicate detection — still applies; there's just no domain-specific check configured yet). */ + assessAppendedEntry?: (document: unknown, opts: { secretsScan?: boolean; sourceUrlValidation?: boolean; appendedEntry: unknown }) => Assessment; + /** Validates a flat provider-submission document and returns its ProviderAssessment — the registry's own + * domain-specific check, analogous to `assessAppendedEntry` but for the provider-file scope. Omitted ⇒ the + * orchestrator returns "manual" for provider submissions to this registry. */ + assessProviderEntry?: (document: unknown, opts?: { secretsScan?: boolean; sourceUrlValidation?: boolean }) => ProviderAssessment; } export type RegistryPrScope = "entry-submission" | "provider-submission" | "mixed-files" | "not-direct-submission"; @@ -801,4 +812,8 @@ export const METAGRAPHED_LANE_SPEC: RegistryLaneSpec = { // `url` alone (a subnet's surfaces are distinct interfaces; the same url appearing twice — in one PR or against // an entry already registered — is a resubmission, not a new surface). duplicateKeyFields: ["url"], + // metagraphed's own domain-specific semantic validators (netuid/kind/public_safe/auth_required shape+safety + // checks) — supplied here, not hardcoded into the orchestrator, so a different registry can supply its own. + assessAppendedEntry: assessSubnetDocument, + assessProviderEntry: assessProviderDocument, }; diff --git a/src/review/content-lane/spec-resolver.ts b/src/review/content-lane/spec-resolver.ts new file mode 100644 index 0000000000..c1635a90bc --- /dev/null +++ b/src/review/content-lane/spec-resolver.ts @@ -0,0 +1,66 @@ +// Per-repo RegistryLaneSpec resolution (#2435 — closes the "only metagraphed can use this" gap). Before this, +// content-lane-wire.ts hard-selected METAGRAPHED_LANE_SPEC for every repo in the GITTENSORY_REVIEW_REPOS +// allowlist; a different self-hosted maintainer's registry could only be onboarded by editing gittensory's own +// TypeScript source. This mirrors resolveConvergedFeature's precedence (review/feature-activation.ts): env +// kill-switch → per-repo `.gittensory.yml` config → allowlist default — but resolves to a whole spec OBJECT (or +// null/inactive) instead of a boolean, so it lives alongside the content-lane engine rather than in +// feature-activation.ts itself, which only knows about boolean converged features. +import { globToRegExp } from "../../signals/change-guardrail"; +import type { FocusManifest, FocusManifestContentLaneConfig } from "../../signals/focus-manifest"; +import { isConvergenceRepoAllowed } from "../cutover-gate"; +import { type ContentLaneEnv, isContentLaneEnabled } from "./flag"; +import { assessProviderDocument, assessSubnetDocument, METAGRAPHED_LANE_SPEC, type RegistryLaneSpec } from "./registry-logic"; + +/** + * Code-registered, PR-reviewed domain validators a maintainer's `.gittensory.yml` `contentLane.validatorId` can + * reference by name — mirrors the existing `GatePolicyPack` pattern (`gate.pack` in `.gittensory.yml`, branched + * on in `rules/predicted-gate.ts`): config picks a string id that selects one of a small, code-reviewed set of + * behavior bundles, rather than a maintainer supplying arbitrary logic through config. Semantic validation stays + * a deliberate, bounded, one-time code contribution (a new validator module + a one-line registration here, + * using metagraphed's own module as the template) — everything else about a registry (file patterns, entry-count + * cap, dedup fields) is pure config, no code change required. + */ +const REGISTRY_VALIDATORS: Record> = { + metagraphed: { assessAppendedEntry: assessSubnetDocument, assessProviderEntry: assessProviderDocument }, +}; + +/** + * Builds a RegistryLaneSpec from a manifest's `contentLane:` block. Returns null when the config isn't "present" + * (parseContentLaneConfig already treats a partial config — missing entryFileGlob/collectionField — as absent, + * so `present` here always implies both are set). Glob fields compile via the SAME bounded glob compiler used + * for guardrail paths (change-guardrail.ts) — never a raw regex from a maintainer-supplied string, matching this + * codebase's established ReDoS-avoidance convention. An unregistered `validatorId` degrades to structural gating + * only (no domain-specific validator), the same degraded mode a spec with no validatorId configured at all gets + * — never a crash or a silent skip of the count/dedup checks. + */ +export function buildRegistryLaneSpecFromConfig(config: FocusManifestContentLaneConfig): RegistryLaneSpec | null { + if (!config.present || !config.entryFileGlob || !config.collectionField) return null; + const validator = config.validatorId ? REGISTRY_VALIDATORS[config.validatorId] : undefined; + return { + entryFilePattern: globToRegExp(config.entryFileGlob), + collectionField: config.collectionField, + ...(config.providerFileGlob ? { providerFilePattern: globToRegExp(config.providerFileGlob) } : {}), + ...(config.artifactGlob ? { artifactPattern: globToRegExp(config.artifactGlob) } : {}), + ...(config.maxAppendedEntries !== null ? { maxAppendedEntries: config.maxAppendedEntries } : {}), + ...(config.duplicateKeyFields.length > 0 ? { duplicateKeyFields: config.duplicateKeyFields } : {}), + ...(validator ?? {}), + }; +} + +/** + * Resolve the effective RegistryLaneSpec for a repo, or null when the content lane is inactive. PURE + + * synchronous (takes an already-loaded manifest), mirroring `resolveConvergedFeature`'s precedence: env + * kill-switch (off ⇒ null, no per-repo override can turn it back on) → an explicit per-repo `contentLane:` + * config → the allowlist-based default (METAGRAPHED_LANE_SPEC — today's zero-config behavior, UNCHANGED for any + * repo that hasn't opted into its own config) → inactive. + */ +export function resolveRegistryLaneSpec( + env: ContentLaneEnv & { GITTENSORY_REVIEW_REPOS?: string | undefined }, + manifest: Pick | null | undefined, + repoFullName: string, +): RegistryLaneSpec | null { + if (!isContentLaneEnabled(env)) return null; + const configured = manifest?.contentLane ? buildRegistryLaneSpecFromConfig(manifest.contentLane) : null; + if (configured) return configured; + return isConvergenceRepoAllowed(env, repoFullName) ? METAGRAPHED_LANE_SPEC : null; +} diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index 337e018d83..437f3f2c66 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -13,8 +13,10 @@ function canonicalize(value: string): string { } /** Convert a path glob (`*` matches within a segment, `**` matches across `/`) to an anchored RegExp. The - * glob is canonicalized first, so matching is case-insensitive against a canonicalized path. */ -function globToRegExp(glob: string): RegExp { + * glob is canonicalized first, so matching is case-insensitive against a canonicalized path. Exported for + * reuse anywhere a maintainer-supplied path pattern needs compiling — never compile a raw regex string from + * config (ReDoS risk); this linear-time glob compiler is the one safe path pattern this codebase uses. */ +export function globToRegExp(glob: string): RegExp { const canonical = canonicalize(glob); let re = ""; for (let i = 0; i < canonical.length; i += 1) { diff --git a/src/signals/focus-manifest-loader.ts b/src/signals/focus-manifest-loader.ts index 0ae40c66ec..8c8a74a48b 100644 --- a/src/signals/focus-manifest-loader.ts +++ b/src/signals/focus-manifest-loader.ts @@ -1,7 +1,7 @@ import { listSignalSnapshots, persistSignalSnapshot } from "../db/repositories"; import type { JsonValue } from "../types"; import { nowIso } from "../utils/json"; -import { featuresConfigToJson, gateConfigToJson, MAX_FOCUS_MANIFEST_BYTES, parseFocusManifest, parseFocusManifestContent, reviewConfigToJson, settingsOverrideToJson, type FocusManifest, type FocusManifestSource, type RepoReviewContext } from "./focus-manifest"; +import { contentLaneConfigToJson, featuresConfigToJson, gateConfigToJson, MAX_FOCUS_MANIFEST_BYTES, parseFocusManifest, parseFocusManifestContent, reviewConfigToJson, settingsOverrideToJson, type FocusManifest, type FocusManifestSource, type RepoReviewContext } from "./focus-manifest"; import { GITTENSORY_REPO_FOCUS_MANIFEST_YAML, resolveGittensorySelfRepoFullName } from "../config/gittensory-repo-focus-manifest"; export const REPO_FOCUS_MANIFEST_SIGNAL = "repo-focus-manifest"; @@ -283,6 +283,7 @@ function manifestToJson(manifest: FocusManifest): Record { settings: settingsOverrideToJson(manifest.settings), review: reviewConfigToJson(manifest.review), features: featuresConfigToJson(manifest.features), + contentLane: contentLaneConfigToJson(manifest.contentLane), }; } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 066126dbb3..3e637b30eb 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -49,9 +49,11 @@ export type FocusManifestGateConfig = { // `.gittensory.yml`. Each feature ALSO has a GLOBAL env flag (GITTENSORY_REVIEW_*) that stays a master // kill-switch (the feature never runs when its env flag is off, regardless of this block). See // review/feature-activation.ts for the resolver (env kill-switch → per-repo override → env-allowlist default). -// NOTE: only the per-PR REVIEW features whose every activation site is migrated are listed here. grounding, -// screenshots, and contentLane stay on the GITTENSORY_REVIEW_REPOS allowlist for now (grounding + contentLane are -// coupled to the merge/close DISPOSITION path; screenshots' capture path needs dedicated coverage) — a follow-up. +// NOTE: only the per-PR REVIEW features whose every activation site is migrated are listed here. grounding and +// screenshots stay on the GITTENSORY_REVIEW_REPOS allowlist for now (grounding is coupled to the merge/close +// DISPOSITION path; screenshots' capture path needs dedicated coverage) — a follow-up. contentLane got its own +// richer `contentLane:` block below (#2435) instead of a boolean here, since it resolves to a whole +// RegistryLaneSpec, not an on/off toggle — see resolveRegistryLaneSpec in review/content-lane/spec-resolver.ts. export const CONVERGED_FEATURE_KEYS = ["rag", "reputation", "unifiedComment", "safety"] as const; export type ConvergedFeatureKey = (typeof CONVERGED_FEATURE_KEYS)[number]; @@ -60,6 +62,26 @@ export type ConvergedFeatureKey = (typeof CONVERGED_FEATURE_KEYS)[number]; * `GITTENSORY_REVIEW_REPOS` allowlist default, so an operator who sets nothing keeps today's behavior. */ export type FocusManifestFeaturesConfig = { present: boolean } & Record; +/** + * Per-repo registry-review lane configuration (`contentLane:` block, #2435) — lets a self-hosted maintainer + * configure their OWN registry (structural file-scope patterns + entry-count cap + dedup fields) without a + * gittensory code change. `entryFileGlob` and `collectionField` are the two REQUIRED fields to build a usable + * spec; `present` is true only when both are set (a partial config degrades to "not configured," not a broken + * half-spec — see `parseContentLaneConfig`). `validatorId` optionally references a code-registered domain + * validator (`review/content-lane/spec-resolver.ts`'s `REGISTRY_VALIDATORS`); omitted ⇒ structural gating only + * (scope/count/dedup), no domain-specific semantic check — see `RegistryLaneSpec.assessAppendedEntry`. + */ +export type FocusManifestContentLaneConfig = { + present: boolean; + entryFileGlob: string | null; + providerFileGlob: string | null; + artifactGlob: string | null; + collectionField: string | null; + maxAppendedEntries: number | null; + duplicateKeyFields: string[]; + validatorId: string | null; +}; + /** * Generic repository-settings override declared in `.gittensory.yml` under `settings:`. A partial of * {@link RepositorySettings} — every behaviour a maintainer can toggle in the dashboard can be set here @@ -197,6 +219,7 @@ export type FocusManifest = { settings: FocusManifestSettings; review: FocusManifestReviewConfig; features: FocusManifestFeaturesConfig; + contentLane: FocusManifestContentLaneConfig; warnings: string[]; }; @@ -269,6 +292,17 @@ const EMPTY_FEATURES_CONFIG: FocusManifestFeaturesConfig = { safety: null, }; +const EMPTY_CONTENT_LANE_CONFIG: FocusManifestContentLaneConfig = { + present: false, + entryFileGlob: null, + providerFileGlob: null, + artifactGlob: null, + collectionField: null, + maxAppendedEntries: null, + duplicateKeyFields: [], + validatorId: null, +}; + const EMPTY_MANIFEST: FocusManifest = { present: false, source: "none", @@ -284,6 +318,7 @@ const EMPTY_MANIFEST: FocusManifest = { settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, features: { ...EMPTY_FEATURES_CONFIG }, + contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, warnings: [], }; @@ -304,7 +339,16 @@ export function isFocusManifestPublicSafe(text: string): boolean { } function emptyManifest(source: FocusManifestSource, warnings: string[] = []): FocusManifest { - return { ...EMPTY_MANIFEST, source, warnings, gate: { ...EMPTY_GATE_CONFIG }, settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, features: { ...EMPTY_FEATURES_CONFIG } }; + return { + ...EMPTY_MANIFEST, + source, + warnings, + gate: { ...EMPTY_GATE_CONFIG }, + settings: {}, + review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, + features: { ...EMPTY_FEATURES_CONFIG }, + contentLane: { ...EMPTY_CONTENT_LANE_CONFIG }, + }; } function normalizeStringList(value: JsonValue | undefined, field: string, warnings: string[]): string[] { @@ -564,6 +608,88 @@ export function featuresConfigToJson(features: FocusManifestFeaturesConfig): Jso return out; } +/** A positive INTEGER count (not a score/confidence) — e.g. `contentLane.maxAppendedEntries` counts discrete + * surfaces[] entries, so a fractional value (a likely typo) would render a nonsensical contributor-facing close + * message ("append between 1 and 2.5 entries"). Rejects fractional and non-positive values alike. */ +function normalizeOptionalPositiveInteger(value: JsonValue | undefined, field: string, warnings: string[]): number | null { + if (value === undefined || value === null) return null; + if (typeof value === "number" && Number.isInteger(value) && value > 0) return value; + warnings.push(`Manifest field "${field}" must be a positive whole number; ignoring it.`); + return null; +} + +// A glob compiled to RegExp (review/content-lane/spec-resolver.ts's globToRegExp reuse of the guardrail-path +// compiler) chains a `[^/]*` per `*` — MULTIPLE chained wildcards separated by literal characters can +// catastrophically backtrack on an adversarial near-miss input (verified empirically: 5 chained wildcards against +// a maximally-adversarial 300-char input took ~19 SECONDS; 3 stays under 5ms even at that same length). No +// legitimate single-purpose entry-file glob for this feature needs more than a couple of wildcards, so this caps +// wildcard count at parse time — well before the string ever reaches RegExp compilation — rather than trying to +// make the compiled pattern itself provably safe. +const MAX_GLOB_WILDCARDS = 3; + +/** Normalize + bound a maintainer-supplied glob string: trims/length-caps like any other string field, AND caps + * the number of `*` wildcard characters (see MAX_GLOB_WILDCARDS) so it can never compile into a + * catastrophically-backtracking RegExp downstream. A glob over the cap is REJECTED (warns, returns null) rather + * than truncated — silently cutting wildcards out of a maintainer's pattern would silently change its meaning, + * which is worse than making them fix an over-complex glob. */ +function normalizeOptionalGlob(value: JsonValue | undefined, field: string, warnings: string[]): string | null { + const normalized = normalizeOptionalString(value, field, warnings); + if (normalized === null) return null; + if (normalized.length > MAX_ITEM_LENGTH) { + warnings.push(`Manifest field "${field}" truncated an over-long glob.`); + } + const bounded = normalized.slice(0, MAX_ITEM_LENGTH); + const wildcardCount = (bounded.match(/\*/g) ?? []).length; + if (wildcardCount > MAX_GLOB_WILDCARDS) { + warnings.push(`Manifest field "${field}" has too many wildcards (${wildcardCount} > ${MAX_GLOB_WILDCARDS}); ignoring it.`); + return null; + } + return bounded; +} + +/** + * Parse the optional `contentLane:` mapping — per-repo registry-review lane configuration (#2435). `entryFileGlob` + * and `collectionField` are REQUIRED to build a usable spec; a config missing either — including a glob rejected + * by `normalizeOptionalGlob`'s wildcard cap — degrades to "not configured" (a warning, falling through to the + * allowlist default) rather than a broken half-spec. Glob fields stay plain strings here — compiling them to + * RegExp is the resolver's job (`review/content-lane/spec-resolver.ts`), not the parser's, so this file stays + * free of a RegExp-from-config compile step; it's still this file's job to keep an over-complex glob from ever + * reaching that compile step at all. + */ +function parseContentLaneConfig(value: JsonValue | undefined, warnings: string[]): FocusManifestContentLaneConfig { + if (value === undefined || value === null) return { ...EMPTY_CONTENT_LANE_CONFIG }; + if (typeof value !== "object" || Array.isArray(value)) { + warnings.push('Manifest field "contentLane" must be a mapping; ignoring it.'); + return { ...EMPTY_CONTENT_LANE_CONFIG }; + } + const record = value as Record; + const entryFileGlob = normalizeOptionalGlob(record.entryFileGlob, "contentLane.entryFileGlob", warnings); + const providerFileGlob = normalizeOptionalGlob(record.providerFileGlob, "contentLane.providerFileGlob", warnings); + const artifactGlob = normalizeOptionalGlob(record.artifactGlob, "contentLane.artifactGlob", warnings); + const collectionField = normalizeOptionalString(record.collectionField, "contentLane.collectionField", warnings); + const maxAppendedEntries = normalizeOptionalPositiveInteger(record.maxAppendedEntries, "contentLane.maxAppendedEntries", warnings); + const duplicateKeyFields = normalizeStringList(record.duplicateKeyFields, "contentLane.duplicateKeyFields", warnings); + const validatorId = normalizeOptionalString(record.validatorId, "contentLane.validatorId", warnings); + if (!entryFileGlob || !collectionField) { + warnings.push('Manifest field "contentLane" requires both entryFileGlob and collectionField; ignoring it.'); + return { ...EMPTY_CONTENT_LANE_CONFIG }; + } + return { present: true, entryFileGlob, providerFileGlob, artifactGlob, collectionField, maxAppendedEntries, duplicateKeyFields, validatorId }; +} + +/** Serialize a contentLane config back into the parse-compatible `contentLane:` shape so a cached snapshot + * round-trips through {@link parseContentLaneConfig} unchanged. Returns null when nothing is configured. */ +export function contentLaneConfigToJson(contentLane: FocusManifestContentLaneConfig): JsonValue { + if (!contentLane.present || !contentLane.entryFileGlob || !contentLane.collectionField) return null; + const out: Record = { entryFileGlob: contentLane.entryFileGlob, collectionField: contentLane.collectionField }; + if (contentLane.providerFileGlob !== null) out.providerFileGlob = contentLane.providerFileGlob; + if (contentLane.artifactGlob !== null) out.artifactGlob = contentLane.artifactGlob; + if (contentLane.maxAppendedEntries !== null) out.maxAppendedEntries = contentLane.maxAppendedEntries; + if (contentLane.duplicateKeyFields.length > 0) out.duplicateKeyFields = contentLane.duplicateKeyFields; + if (contentLane.validatorId !== null) out.validatorId = contentLane.validatorId; + return out; +} + function normalizeOptionalEnum(value: JsonValue | undefined, field: string, allowed: readonly T[], warnings: string[]): T | null { if (value === undefined || value === null) return null; if (typeof value === "string" && (allowed as readonly string[]).includes(value)) return value as T; @@ -1065,6 +1191,7 @@ export function parseFocusManifest(raw: unknown, source?: FocusManifestSource): settings: parseSettingsOverride(record.settings, warnings), review: parseReviewConfig(record.review, warnings), features: parseFeaturesConfig(record.features, warnings), + contentLane: parseContentLaneConfig(record.contentLane, warnings), warnings, }; if ( @@ -1079,7 +1206,8 @@ export function parseFocusManifest(raw: unknown, source?: FocusManifestSource): !manifest.gate.present && Object.keys(manifest.settings).length === 0 && !manifest.review.present && - !manifest.features.present + !manifest.features.present && + !manifest.contentLane.present ) { warnings.push("Manifest contained no recognized focus fields; falling back to deterministic signals."); manifest.present = false; diff --git a/test/unit/content-lane-orchestrator.test.ts b/test/unit/content-lane-orchestrator.test.ts index 9af6b47d8b..99ff401244 100644 --- a/test/unit/content-lane-orchestrator.test.ts +++ b/test/unit/content-lane-orchestrator.test.ts @@ -4,6 +4,8 @@ import { FLAT_PROVIDER_PATTERN, METAGRAPHED_LANE_SPEC, SUBNET_ENTRY_PATTERN, + assessProviderDocument, + assessSubnetDocument, type RegistryLaneSpec, } from "../../src/review/content-lane/registry-logic"; import { diffAppendedSurfaceEntries, runSurfaceReview, type SurfaceReviewInput } from "../../src/review/content-lane/orchestrator"; @@ -14,12 +16,15 @@ const newEntry2 = { kind: "openapi", url: "https://api2.example.ai", source_url: const SUBNET = "registry/subnets/foo.json"; const PROVIDER = "registry/providers/acme.json"; // A spec-less-backward-compat stand-in: the default single-entry cap (no maxAppendedEntries override), otherwise -// identical to metagraphed's file layout so SUBNET still classifies as an entry-submission. +// identical to metagraphed's file layout (incl. reusing its real validators — this fixture tests the STRUCTURAL +// layer at a different cap, not a different domain) so SUBNET still classifies as an entry-submission. const STRICT_SPEC: RegistryLaneSpec = { entryFilePattern: SUBNET_ENTRY_PATTERN, providerFilePattern: FLAT_PROVIDER_PATTERN, artifactPattern: ARTIFACT_PATTERN, collectionField: "surfaces", + assessAppendedEntry: assessSubnetDocument, + assessProviderEntry: assessProviderDocument, }; // Inject a file loader keyed by `${ref}:${path}` so the orchestrator never hits the network. @@ -307,4 +312,28 @@ describe("runSurfaceReview (deterministic + decisive: merge/close, rarely manual summary: "A surface submission must not duplicate an entry already in this PR or already in the registry — resubmit without the duplicate.", }); }); + + // A spec with no domain-specific validator configured yet: structural gating (scope/count/dedup) still applies, + // but the orchestrator can't itself judge entry content — routes to manual instead of merging or closing blind. + const UNVALIDATED_SPEC: RegistryLaneSpec = { + entryFilePattern: SUBNET_ENTRY_PATTERN, + providerFilePattern: FLAT_PROVIDER_PATTERN, + collectionField: "surfaces", + }; + + it("routes a clean single-entry append to MANUAL when the spec has no assessAppendedEntry configured", async () => { + const r = await review([SUBNET], { [`head:${SUBNET}`]: doc([existing, newEntry]), [`base:${SUBNET}`]: doc([existing]) }, UNVALIDATED_SPEC); + expect(r).toEqual({ + verdict: "manual", + summary: "No validator is configured for this registry's surface entries — routing to review.", + }); + }); + + it("routes a provider submission to MANUAL when the spec has no assessProviderEntry configured", async () => { + const r = await review([PROVIDER], { [`head:${PROVIDER}`]: JSON.stringify({ provider: { id: "acme", name: "Acme", website_url: "https://acme.example" } }) }, UNVALIDATED_SPEC); + expect(r).toEqual({ + verdict: "manual", + summary: "No validator is configured for this registry's provider submissions — routing to review.", + }); + }); }); diff --git a/test/unit/content-lane-spec-resolver.test.ts b/test/unit/content-lane-spec-resolver.test.ts new file mode 100644 index 0000000000..730a272664 --- /dev/null +++ b/test/unit/content-lane-spec-resolver.test.ts @@ -0,0 +1,125 @@ +import { describe, expect, it } from "vitest"; +import { buildRegistryLaneSpecFromConfig, resolveRegistryLaneSpec } from "../../src/review/content-lane/spec-resolver"; +import { METAGRAPHED_LANE_SPEC } from "../../src/review/content-lane/registry-logic"; +import { parseFocusManifest, type FocusManifestContentLaneConfig } from "../../src/signals/focus-manifest"; + +const EMPTY_CONFIG: FocusManifestContentLaneConfig = { + present: false, + entryFileGlob: null, + providerFileGlob: null, + artifactGlob: null, + collectionField: null, + maxAppendedEntries: null, + duplicateKeyFields: [], + validatorId: null, +}; + +describe("buildRegistryLaneSpecFromConfig", () => { + it("returns null when the config is not present", () => { + expect(buildRegistryLaneSpecFromConfig(EMPTY_CONFIG)).toBeNull(); + }); + + it("returns null when required fields are missing despite present:true (defensive — parseContentLaneConfig never produces this, but the builder doesn't trust that alone)", () => { + expect(buildRegistryLaneSpecFromConfig({ ...EMPTY_CONFIG, present: true, entryFileGlob: null, collectionField: "items" })).toBeNull(); + expect(buildRegistryLaneSpecFromConfig({ ...EMPTY_CONFIG, present: true, entryFileGlob: "registry/*.json", collectionField: null })).toBeNull(); + }); + + it("builds a minimal spec from just the two required fields", () => { + const spec = buildRegistryLaneSpecFromConfig({ ...EMPTY_CONFIG, present: true, entryFileGlob: "registry/items/*.json", collectionField: "items" }); + expect(spec).not.toBeNull(); + expect(spec?.entryFilePattern.test("registry/items/foo.json")).toBe(true); + expect(spec?.entryFilePattern.test("registry/items/foo/bar.json")).toBe(false); // single-segment glob, not ** + expect(spec?.collectionField).toBe("items"); + expect(spec?.providerFilePattern).toBeUndefined(); + expect(spec?.artifactPattern).toBeUndefined(); + expect(spec?.maxAppendedEntries).toBeUndefined(); + expect(spec?.duplicateKeyFields).toBeUndefined(); + expect(spec?.assessAppendedEntry).toBeUndefined(); + expect(spec?.assessProviderEntry).toBeUndefined(); + }); + + it("compiles providerFileGlob and artifactGlob via the same bounded glob compiler (not a raw regex)", () => { + const spec = buildRegistryLaneSpecFromConfig({ + ...EMPTY_CONFIG, + present: true, + entryFileGlob: "registry/items/*.json", + providerFileGlob: "registry/providers/*.json", + artifactGlob: "public/**/*.json", + collectionField: "items", + }); + expect(spec?.providerFilePattern?.test("registry/providers/acme.json")).toBe(true); + expect(spec?.providerFilePattern?.test("registry/providers/nested/acme.json")).toBe(false); + expect(spec?.artifactPattern?.test("public/a/b/c.json")).toBe(true); // ** crosses segments + }); + + it("passes maxAppendedEntries and duplicateKeyFields through when set", () => { + const spec = buildRegistryLaneSpecFromConfig({ + ...EMPTY_CONFIG, + present: true, + entryFileGlob: "registry/items/*.json", + collectionField: "items", + maxAppendedEntries: 3, + duplicateKeyFields: ["url"], + }); + expect(spec?.maxAppendedEntries).toBe(3); + expect(spec?.duplicateKeyFields).toEqual(["url"]); + }); + + it("resolves a registered validatorId to its code-registered validator pair", () => { + const spec = buildRegistryLaneSpecFromConfig({ + ...EMPTY_CONFIG, + present: true, + entryFileGlob: "registry/subnets/*.json", + collectionField: "surfaces", + validatorId: "metagraphed", + }); + expect(spec?.assessAppendedEntry).toBeDefined(); + expect(spec?.assessProviderEntry).toBeDefined(); + }); + + it("degrades to structural-gating-only (no validator) for an UNREGISTERED validatorId — never throws", () => { + const spec = buildRegistryLaneSpecFromConfig({ + ...EMPTY_CONFIG, + present: true, + entryFileGlob: "registry/items/*.json", + collectionField: "items", + validatorId: "some-registry-nobody-registered-yet", + }); + expect(spec).not.toBeNull(); + expect(spec?.assessAppendedEntry).toBeUndefined(); + expect(spec?.assessProviderEntry).toBeUndefined(); + }); +}); + +describe("resolveRegistryLaneSpec (precedence: env kill-switch → per-repo config → allowlist default → inactive)", () => { + const REPO = "SomeoneElse/other-registry"; + + it("is null when the env kill-switch is off, even with an explicit config or an allowlist entry", () => { + const manifest = parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json", collectionField: "items" } }); + expect(resolveRegistryLaneSpec({ GITTENSORY_REVIEW_REPOS: REPO }, manifest, REPO)).toBeNull(); + }); + + it("falls back to the allowlist default (METAGRAPHED_LANE_SPEC) when no per-repo config is present", () => { + const manifest = parseFocusManifest(null); + const spec = resolveRegistryLaneSpec({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO }, manifest, REPO); + expect(spec).toBe(METAGRAPHED_LANE_SPEC); + }); + + it("is null when there's no config AND the repo is not in the allowlist — inactive", () => { + const manifest = parseFocusManifest(null); + expect(resolveRegistryLaneSpec({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: "Other/repo" }, manifest, REPO)).toBeNull(); + }); + + it("an explicit per-repo config WINS over the allowlist default, even for a repo not in the allowlist at all", () => { + const manifest = parseFocusManifest({ contentLane: { entryFileGlob: "registry/items/*.json", collectionField: "items" } }); + const spec = resolveRegistryLaneSpec({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: "Other/repo" }, manifest, REPO); + expect(spec).not.toBeNull(); + expect(spec).not.toBe(METAGRAPHED_LANE_SPEC); + expect(spec?.collectionField).toBe("items"); + }); + + it("a null/undefined manifest degrades to the allowlist-default path, not a crash", () => { + expect(resolveRegistryLaneSpec({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO }, null, REPO)).toBe(METAGRAPHED_LANE_SPEC); + expect(resolveRegistryLaneSpec({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO }, undefined, REPO)).toBe(METAGRAPHED_LANE_SPEC); + }); +}); diff --git a/test/unit/content-lane-wire.test.ts b/test/unit/content-lane-wire.test.ts index 9e05938050..577f56f9c6 100644 --- a/test/unit/content-lane-wire.test.ts +++ b/test/unit/content-lane-wire.test.ts @@ -1,7 +1,9 @@ import { afterEach, describe, expect, it, vi } from "vitest"; import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation } from "../../src/rules/advisory"; -import { applySurfaceGate, evaluateWithSurfaceLane, isContentLaneWired, resolveSurfaceRefs, runMetagraphedSurfaceGate, surfaceVerdictToGate } from "../../src/review/content-lane-wire"; +import { applySurfaceGate, evaluateWithSurfaceLane, resolveSurfaceRefs, runRegistrySurfaceGate, surfaceVerdictToGate } from "../../src/review/content-lane-wire"; import type { SurfaceReviewInput } from "../../src/review/content-lane/orchestrator"; +import { METAGRAPHED_LANE_SPEC } from "../../src/review/content-lane/registry-logic"; +import { parseFocusManifest, type FocusManifest } from "../../src/signals/focus-manifest"; import type { AdvisoryFinding } from "../../src/types"; const env = {} as unknown as Env; @@ -16,17 +18,12 @@ const validProvider = JSON.stringify({ provider: { id: "acme", name: "Acme", web // A loadFile stub keyed by `${ref}:${path}` (mirrors the orchestrator test) so the adapter never hits the network. const loader = (files: Record): SurfaceReviewInput["loadFile"] => (path, ref) => Promise.resolve(files[`${ref}:${path}`] ?? null); const gate = (over: Partial): GateCheckEvaluation => ({ enabled: true, conclusion: "success", title: "Gate", summary: "", blockers: [], warnings: [], ...over }); +// A no-`contentLane:`-config manifest — evaluateWithSurfaceLane's resolver falls through to the +// GITTENSORY_REVIEW_REPOS allowlist default (METAGRAPHED_LANE_SPEC), matching today's zero-config behavior. +const noConfigManifest = (): Promise => Promise.resolve(parseFocusManifest(null)); afterEach(() => vi.unstubAllGlobals()); -describe("isContentLaneWired", () => { - it("requires BOTH the flag and the per-repo allowlist", () => { - expect(isContentLaneWired({ GITTENSORY_REVIEW_REPOS: REPO }, REPO)).toBe(false); // flag off - expect(isContentLaneWired({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: "OtherOrg/other" }, REPO)).toBe(false); // not allowlisted - expect(isContentLaneWired({ GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO }, REPO)).toBe(true); - }); -}); - describe("surfaceVerdictToGate", () => { it("merge → success with no finding", () => { const { evaluation, finding } = surfaceVerdictToGate({ verdict: "merge", summary: "ok" }); @@ -144,9 +141,9 @@ describe("applySurfaceGate", () => { }); }); -describe("runMetagraphedSurfaceGate (injected loader — adapter logic)", () => { +describe("runRegistrySurfaceGate (injected loader — adapter logic)", () => { const run = (files: { path: string; status?: string | null }[], stub: Record, advisory = { findings: [] as AdvisoryFinding[] }) => - runMetagraphedSurfaceGate(env, { installationId: 0, repoFullName: REPO, pr: { headSha: "HEAD", baseRef: "BASE" }, advisory, files }, loader(stub)); + runRegistrySurfaceGate(env, METAGRAPHED_LANE_SPEC, { installationId: 0, repoFullName: REPO, pr: { headSha: "HEAD", baseRef: "BASE" }, advisory, files }, loader(stub)); it("defers (null) for a non-submission PR", async () => { expect(await run([{ path: "README.md", status: "added" }], {})).toBeNull(); @@ -218,6 +215,11 @@ describe("evaluateWithSurfaceLane (the processor seam helper)", () => { expect(await evaluateWithSurfaceLane({} as unknown as Env, REPO, true, generic, baseArgs)).toBe(generic); }); + it("returns the generic gate unchanged when the flag is on but NO spec resolves for this repo (no config, not in the allowlist — no file resolve)", async () => { + const unresolvedEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: "Some/OtherRepo" } as unknown as Env; + expect(await evaluateWithSurfaceLane(unresolvedEnv, REPO, true, generic, baseArgs, noConfigManifest)).toBe(generic); + }); + it("when wired, runs the surface lane via the real GitHub loader and overrides the gate", async () => { const bodies: Record = { "HEAD:registry/subnets/foo.json": doc([existing, newEntry]), @@ -231,13 +233,20 @@ describe("evaluateWithSurfaceLane (the processor seam helper)", () => { return body === undefined ? new Response("missing", { status: 404 }) : new Response(body); }); const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; - const out = await evaluateWithSurfaceLane(wiredEnv, REPO, true, generic, { - installationId: null, // → unauthenticated fetcher; only the stub is hit - pr: { headSha: "HEAD", baseRef: "BASE" }, - repo: { defaultBranch: "main" }, - advisory: { findings: [] }, - getChangedFiles: async () => [{ path: SUBNET, status: "modified" }], - }); + const out = await evaluateWithSurfaceLane( + wiredEnv, + REPO, + true, + generic, + { + installationId: null, // → unauthenticated fetcher; only the stub is hit + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory: { findings: [] }, + getChangedFiles: async () => [{ path: SUBNET, status: "modified" }], + }, + noConfigManifest, + ); expect(out?.conclusion).toBe("success"); // a clean append merges, overriding the generic gate }); @@ -261,13 +270,20 @@ describe("evaluateWithSurfaceLane (the processor seam helper)", () => { const advisory = { findings: [aiConsensusDefect, otherWarning] }; const genericAiOnly = gate({ conclusion: "failure", blockers: [aiConsensusDefect], warnings: [] }); const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; - const out = await evaluateWithSurfaceLane(wiredEnv, REPO, true, genericAiOnly, { - installationId: null, - pr: { headSha: "HEAD", baseRef: "BASE" }, - repo: { defaultBranch: "main" }, - advisory, - getChangedFiles: async () => [{ path: SUBNET, status: "modified" }], - }); + const out = await evaluateWithSurfaceLane( + wiredEnv, + REPO, + true, + genericAiOnly, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory, + getChangedFiles: async () => [{ path: SUBNET, status: "modified" }], + }, + noConfigManifest, + ); expect(out?.conclusion).toBe("success"); // The overridden ai_consensus_defect must be gone from advisory.findings too — otherwise the unified-comment // bridge would still recover it via consensusDefectFromFindings and render "Concerns raised" over a merge. @@ -281,13 +297,141 @@ describe("evaluateWithSurfaceLane (the processor seam helper)", () => { // A real (non-AI) blocker alongside the AI one means isAiJudgmentOnlyFailure is false — no cleanup should run. const genericMixed = gate({ conclusion: "failure", blockers: [aiConsensusDefect, secret], warnings: [] }); const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; - await evaluateWithSurfaceLane(wiredEnv, REPO, true, genericMixed, { + await evaluateWithSurfaceLane( + wiredEnv, + REPO, + true, + genericMixed, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory, + getChangedFiles: async () => [{ path: "README.md", status: "modified" }], // not a registry submission → surface defers (null) + }, + noConfigManifest, + ); + expect(advisory.findings).toEqual([aiConsensusDefect, secret]); + }); + + it("defaults to the REAL loadRepoFocusManifest when no override is injected, and still degrades safely on a fake env", async () => { + // No loadManifestOverride argument at all — exercises the production default (`loadManifestOverride ?? + // loadRepoFocusManifest`). A fake env with no D1 binding makes the real loader reject fast; `.catch(() => + // null)` still routes it to the allowlist-default resolution path rather than throwing out of this function. + const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const out = await evaluateWithSurfaceLane(wiredEnv, REPO, true, generic, { installationId: null, pr: { headSha: "HEAD", baseRef: "BASE" }, repo: { defaultBranch: "main" }, - advisory, + advisory: { findings: [] }, getChangedFiles: async () => [{ path: "README.md", status: "modified" }], // not a registry submission → surface defers (null) }); - expect(advisory.findings).toEqual([aiConsensusDefect, secret]); + expect(out).toBe(generic); + }); + + it("routes an unresolved/thrown manifest load to the allowlist default rather than throwing (fail-safe)", async () => { + const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const throwingLoader = (): Promise => Promise.reject(new Error("simulated D1/network failure")); + const out = await evaluateWithSurfaceLane( + wiredEnv, + REPO, + true, + generic, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory: { findings: [] }, + getChangedFiles: async () => [{ path: "README.md", status: "modified" }], // not a registry submission → surface defers (null) + }, + throwingLoader, + ); + expect(out).toBe(generic); // degrades to the allowlist-default resolution path, never throws + }); + + it("activates the surface lane for a NON-metagraphed repo purely from an explicit contentLane: config — no allowlist entry needed", async () => { + const OTHER_REPO = "SomeoneElse/other-registry"; + const OTHER_ENTRY = "registry/items/foo.json"; + const otherDoc = (items: unknown[]) => JSON.stringify({ items }); + const bodies: Record = { + [`HEAD:${OTHER_ENTRY}`]: otherDoc([{ url: "https://api.example.org/new" }]), + [`BASE:${OTHER_ENTRY}`]: otherDoc([]), + }; + vi.stubGlobal("fetch", async (url: string | URL) => { + const m = /\/contents\/(.+)\?ref=(.+)$/.exec(String(url)); + if (!m) return new Response("nope", { status: 404 }); + const path = m[1]!.split("/").map(decodeURIComponent).join("/"); + const body = bodies[`${decodeURIComponent(m[2]!)}:${path}`]; + return body === undefined ? new Response("missing", { status: 404 }) : new Response(body); + }); + // Flag on, but OTHER_REPO is NOT in GITTENSORY_REVIEW_REPOS — proving activation comes from the config alone. + const configuredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const configuredManifest = (): Promise => + Promise.resolve( + parseFocusManifest({ + contentLane: { entryFileGlob: "registry/items/*.json", collectionField: "items" }, + }), + ); + const out = await evaluateWithSurfaceLane( + configuredEnv, + OTHER_REPO, + true, + undefined, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory: { findings: [] }, + getChangedFiles: async () => [{ path: OTHER_ENTRY, status: "modified" }], + }, + configuredManifest, + ); + // No validatorId was configured → structural gating only (a valid, non-duplicate, in-scope append) → + // manual, NOT merge/close — the concrete proof this is reachable via config alone (see spec-resolver tests + // for the full config→spec resolution matrix). + expect(out?.conclusion).toBe("neutral"); + }); + + it("a CONFIG-RESOLVED spec's duplicateKeyFields drives the same end-to-end dedup close as METAGRAPHED_LANE_SPEC's own — the whole pipeline, not just the code-defined default spec", async () => { + const OTHER_REPO = "SomeoneElse/other-registry"; + const OTHER_ENTRY = "registry/items/foo.json"; + const otherDoc = (items: unknown[]) => JSON.stringify({ items }); + const dup1 = { url: "https://api.example.org/new" }; + const dup2 = { url: "https://api.example.org/new", note: "a same-PR resubmission of the same url" }; + const bodies: Record = { + [`HEAD:${OTHER_ENTRY}`]: otherDoc([dup1, dup2]), + [`BASE:${OTHER_ENTRY}`]: otherDoc([]), + }; + vi.stubGlobal("fetch", async (url: string | URL) => { + const m = /\/contents\/(.+)\?ref=(.+)$/.exec(String(url)); + if (!m) return new Response("nope", { status: 404 }); + const path = m[1]!.split("/").map(decodeURIComponent).join("/"); + const body = bodies[`${decodeURIComponent(m[2]!)}:${path}`]; + return body === undefined ? new Response("missing", { status: 404 }) : new Response(body); + }); + const configuredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const configuredManifest = (): Promise => + Promise.resolve( + parseFocusManifest({ + contentLane: { entryFileGlob: "registry/items/*.json", collectionField: "items", duplicateKeyFields: ["url"] }, + }), + ); + const advisory = { findings: [] as AdvisoryFinding[] }; + const out = await evaluateWithSurfaceLane( + configuredEnv, + OTHER_REPO, + true, + undefined, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory, + getChangedFiles: async () => [{ path: OTHER_ENTRY, status: "modified" }], + }, + configuredManifest, + ); + expect(out?.conclusion).toBe("failure"); // the same-PR url duplicate closes, exactly as the METAGRAPHED_LANE_SPEC regression test above proves + expect(advisory.findings.map((f) => f.code)).toEqual(["surface_lane_reject"]); }); }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index ee2bf488b7..c0142fe76e 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -2,6 +2,7 @@ import { describe, expect, it } from "vitest"; import { buildFocusManifestGuidance, compileFocusManifestPolicy, + contentLaneConfigToJson, deriveContributionLanes, featuresConfigToJson, gateConfigToJson, @@ -482,6 +483,7 @@ describe("compileFocusManifestPolicy", () => { settings: {}, review: { present: false, footerText: null, note: null, fields: {}, profile: null, inlineComments: null, pathInstructions: [], instructions: null, excludePaths: [], preMergeChecks: [] }, features: { present: false, rag: null, reputation: null, unifiedComment: null, safety: null }, + contentLane: { present: false, entryFileGlob: null, providerFileGlob: null, artifactGlob: null, collectionField: null, maxAppendedEntries: null, duplicateKeyFields: [], validatorId: null }, warnings: [], }); expect(policy.publicSafe.entryGuidance).toContain("Keep PRs focused."); @@ -1021,6 +1023,99 @@ describe("parseFocusManifest gate config", () => { expect(featuresConfigToJson(parseFocusManifest({ features: {} }).features)).toBeNull(); }); + it("parses the contentLane: block (#2435 per-repo registry-lane config), round-trips it, and makes the manifest present", () => { + const m = parseFocusManifest({ + contentLane: { + entryFileGlob: "registry/items/*.json", + providerFileGlob: "registry/providers/*.json", + artifactGlob: "public/**/*.json", + collectionField: "items", + maxAppendedEntries: 5, + duplicateKeyFields: ["url"], + validatorId: "acme-registry", + }, + }); + expect(m.present).toBe(true); + expect(m.contentLane).toEqual({ + present: true, + entryFileGlob: "registry/items/*.json", + providerFileGlob: "registry/providers/*.json", + artifactGlob: "public/**/*.json", + collectionField: "items", + maxAppendedEntries: 5, + duplicateKeyFields: ["url"], + validatorId: "acme-registry", + }); + // Round-trips through contentLaneConfigToJson → parseFocusManifest unchanged. + expect(parseFocusManifest({ contentLane: contentLaneConfigToJson(m.contentLane) }).contentLane).toEqual(m.contentLane); + }); + + it("requires BOTH entryFileGlob and collectionField for contentLane: — a partial config warns and is ignored (not a broken half-spec)", () => { + const missingCollectionField = parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json" } }); + expect(missingCollectionField.contentLane.present).toBe(false); + expect(missingCollectionField.warnings.some((w) => /contentLane.*requires both/.test(w))).toBe(true); + const missingEntryFileGlob = parseFocusManifest({ contentLane: { collectionField: "items" } }); + expect(missingEntryFileGlob.contentLane.present).toBe(false); + expect(missingEntryFileGlob.warnings.some((w) => /contentLane.*requires both/.test(w))).toBe(true); + // The whole manifest stays absent when contentLane is the ONLY (incomplete) field set. + expect(missingCollectionField.present).toBe(false); + }); + + it("contentLane: a non-mapping value warns and is ignored; a non-positive maxAppendedEntries warns and is dropped", () => { + expect(parseFocusManifest({ contentLane: ["nope"] }).warnings.some((w) => /"contentLane" must be a mapping/.test(w))).toBe(true); + const m = parseFocusManifest({ + contentLane: { entryFileGlob: "registry/*.json", collectionField: "items", maxAppendedEntries: -1 }, + }); + expect(m.contentLane.maxAppendedEntries).toBeNull(); + expect(m.warnings.some((w) => /contentLane\.maxAppendedEntries/.test(w))).toBe(true); + }); + + it("contentLane: a FRACTIONAL maxAppendedEntries is rejected (would render a broken 'append between 1 and 2.5 entries' message downstream)", () => { + const m = parseFocusManifest({ + contentLane: { entryFileGlob: "registry/*.json", collectionField: "items", maxAppendedEntries: 2.5 }, + }); + expect(m.contentLane.maxAppendedEntries).toBeNull(); + expect(m.warnings.some((w) => /contentLane\.maxAppendedEntries.*whole number/.test(w))).toBe(true); + // A clean positive integer still passes through unchanged. + expect( + parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json", collectionField: "items", maxAppendedEntries: 5 } }).contentLane + .maxAppendedEntries, + ).toBe(5); + }); + + it("contentLane: glob fields are truncated at MAX_ITEM_LENGTH like any other string field", () => { + const overLong = "registry/" + "a".repeat(400) + ".json"; + const m = parseFocusManifest({ contentLane: { entryFileGlob: overLong, collectionField: "items" } }); + expect(m.contentLane.entryFileGlob?.length).toBeLessThanOrEqual(300); + expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*truncated/.test(w))).toBe(true); + }); + + it("SECURITY (ReDoS): a glob with too many wildcards is REJECTED at parse time rather than ever reaching RegExp compilation", () => { + // 5 chained single-segment wildcards is empirically catastrophic against an adversarial input (verified + // ~19s in manual testing) — must never survive parsing to reach globToRegExp at all. + const pathological = "registry/*-*-*-*-*-final.json"; + const m = parseFocusManifest({ contentLane: { entryFileGlob: pathological, collectionField: "items" } }); + expect(m.contentLane.entryFileGlob).toBeNull(); + expect(m.contentLane.present).toBe(false); // entryFileGlob is REQUIRED — a rejected glob degrades to absent + expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*too many wildcards/.test(w))).toBe(true); + // A glob AT the cap (3 wildcards) is accepted; the optional providerFileGlob/artifactGlob fields are dropped + // individually (with a warning) without invalidating the whole block, since only entryFileGlob/collectionField + // are required. + const atCap = parseFocusManifest({ + contentLane: { entryFileGlob: "registry/*/*/*.json", providerFileGlob: "providers/*-*-*-*-*.json", collectionField: "items" }, + }); + expect(atCap.contentLane.present).toBe(true); + expect(atCap.contentLane.entryFileGlob).toBe("registry/*/*/*.json"); + expect(atCap.contentLane.providerFileGlob).toBeNull(); + expect(atCap.warnings.some((w) => /contentLane\.providerFileGlob.*too many wildcards/.test(w))).toBe(true); + }); + + it("contentLaneConfigToJson returns null for an absent config, and omits unset optional fields", () => { + expect(contentLaneConfigToJson(parseFocusManifest(null).contentLane)).toBeNull(); + const m = parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json", collectionField: "items" } }); + expect(contentLaneConfigToJson(m.contentLane)).toEqual({ entryFileGlob: "registry/*.json", collectionField: "items" }); + }); + it("parses aiReviewAllAuthors from the settings: block (generic override)", () => { const parsed = parseFocusManifest({ settings: { aiReviewAllAuthors: true , closeOwnerAuthors: false} }); expect(parsed.settings.aiReviewAllAuthors).toBe(true); From 2ca08ee3330de32f05ed7183e356522e91eec630 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 17:58:49 -0700 Subject: [PATCH 2/5] fix(content-lane): warn on an unregistered contentLane.validatorId MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit buildRegistryLaneSpecFromConfig already degraded an unregistered validatorId to structural-only gating silently (a legitimate mode for a registry with no validator yet), which made an operator typo (e.g. "metagraph" instead of "metagraphed") indistinguishable from a deliberate choice — no signal reached the maintainer. Add unregisteredValidatorId()/registeredValidatorIds() to spec-resolver.ts and surface a non-blocking advisory finding from evaluateWithSurfaceLane so a bad validatorId shows up directly in the PR comment, naming the offending id and the known registered ids. Also switch the REGISTRY_VALIDATORS lookup to Object.hasOwn instead of bracket-truthiness, so a validatorId matching an inherited Object.prototype key (e.g. "toString") is correctly reported as unregistered rather than silently matching a prototype method. --- src/review/content-lane-wire.ts | 23 ++++- src/review/content-lane/spec-resolver.ts | 25 +++++- test/unit/content-lane-spec-resolver.test.ts | 34 +++++++- test/unit/content-lane-wire.test.ts | 90 ++++++++++++++++++++ 4 files changed, 168 insertions(+), 4 deletions(-) diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index 4dc583ca1c..6d1136c78e 100644 --- a/src/review/content-lane-wire.ts +++ b/src/review/content-lane-wire.ts @@ -28,7 +28,7 @@ import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation, isAiJudgmentOnlyFa import { isContentLaneEnabled } from "./content-lane/flag"; import { runSurfaceReview, type SurfaceReviewInput, type SurfaceReviewResult } from "./content-lane/orchestrator"; import type { RegistryLaneSpec } from "./content-lane/registry-logic"; -import { resolveRegistryLaneSpec } from "./content-lane/spec-resolver"; +import { registeredValidatorIds, resolveRegistryLaneSpec, unregisteredValidatorId } from "./content-lane/spec-resolver"; import { makeGithubFileFetcher } from "./grounding-wire"; import type { FocusManifest } from "../signals/focus-manifest"; import { loadRepoFocusManifest } from "../signals/focus-manifest-loader"; @@ -38,12 +38,25 @@ import type { AdvisoryFinding, AdvisorySeverity } from "../types"; // facts, and blocker findings must never be flipped to merge by green CI. const SURFACE_REJECT_CODE = "surface_lane_reject"; const SURFACE_MANUAL_CODE = "surface_lane_manual"; +const SURFACE_UNKNOWN_VALIDATOR_CODE = "surface_lane_unknown_validator_id"; const SURFACE_TITLE = "Registry surface review"; function surfaceFinding(code: string, severity: AdvisorySeverity, summary: string): AdvisoryFinding { return { code, title: SURFACE_TITLE, severity, detail: summary, publicText: summary }; } +/** A diagnostic (non-blocking) finding for a `.gittensory.yml` `contentLane.validatorId` that doesn't match any + * code-registered validator — most likely an operator typo. Without this, `buildRegistryLaneSpecFromConfig` + * degrades silently to structural-only gating (a legitimate mode for a registry with no validator yet), which + * makes a typo indistinguishable from a deliberate choice. Surfaced the SAME way a surface verdict is (pushed + * onto `advisory.findings`) so it renders directly in the PR comment an operator is already reading, rather than + * requiring a separate manifest-diagnostics lookup. */ +function unregisteredValidatorIdFinding(badId: string): AdvisoryFinding { + const knownText = registeredValidatorIds().join(", "); + const summary = `contentLane.validatorId "${badId}" is not a registered validator (known: ${knownText}); falling back to structural-only review with no domain validator.`; + return surfaceFinding(SURFACE_UNKNOWN_VALIDATOR_CODE, "warning", summary); +} + /** Convert the deterministic surface verdict into a gate evaluation. merge→success, manual→neutral * (a warning, not auto-closed and not a failing required check), and any decisive non-merge/non-manual verdict (close) → failure with a single * critical blocker. Returns the finding to splice into the advisory so the public comment renders the reason. */ @@ -182,7 +195,11 @@ export function resolveSurfaceRefs( * `loadManifestOverride` is injected by unit tests (mirrors `runRegistrySurfaceGate`'s `loadFileOverride`) so * they never hit the real cached-manifest loader's D1/network I/O; production omits it and gets the real, * cached `loadRepoFocusManifest`. A manifest-load failure degrades to null (the allowlist-default resolution - * path), never a thrown error. */ + * path), never a thrown error. + * + * An unregistered `contentLane.validatorId` in the loaded manifest pushes a non-blocking diagnostic finding + * (`unregisteredValidatorIdFinding`) onto `args.advisory.findings` so an operator typo (e.g. "metagraph" instead + * of "metagraphed") is visible in the PR comment instead of silently degrading to structural-only review. */ export async function evaluateWithSurfaceLane( env: Env, repoFullName: string, @@ -200,6 +217,8 @@ export async function evaluateWithSurfaceLane( if (!gateEnabled || !isContentLaneEnabled(env)) return gateEvaluation; const loadManifest = loadManifestOverride ?? loadRepoFocusManifest; const manifest = await loadManifest(env, repoFullName).catch(() => null); + const badValidatorId = unregisteredValidatorId(manifest?.contentLane); + if (badValidatorId) args.advisory.findings.push(unregisteredValidatorIdFinding(badValidatorId)); const spec = resolveRegistryLaneSpec(env, manifest, repoFullName); if (!spec) return gateEvaluation; const surfaceGate = await runRegistrySurfaceGate(env, spec, { diff --git a/src/review/content-lane/spec-resolver.ts b/src/review/content-lane/spec-resolver.ts index c1635a90bc..c3de4d99d6 100644 --- a/src/review/content-lane/spec-resolver.ts +++ b/src/review/content-lane/spec-resolver.ts @@ -35,7 +35,7 @@ const REGISTRY_VALIDATORS: Record { }); }); +describe("registeredValidatorIds", () => { + it("lists the code-registered validator ids (currently just metagraphed)", () => { + expect(registeredValidatorIds()).toEqual(["metagraphed"]); + }); +}); + +describe("unregisteredValidatorId (operator-typo diagnostic — separate from buildRegistryLaneSpecFromConfig's silent structural-only degrade)", () => { + it("is null when no validatorId is configured at all", () => { + expect(unregisteredValidatorId(EMPTY_CONFIG)).toBeNull(); + expect(unregisteredValidatorId({ ...EMPTY_CONFIG, validatorId: null })).toBeNull(); + }); + + it("is null for a registered validatorId", () => { + expect(unregisteredValidatorId({ ...EMPTY_CONFIG, validatorId: "metagraphed" })).toBeNull(); + }); + + it("returns the offending id for an unregistered validatorId", () => { + expect(unregisteredValidatorId({ ...EMPTY_CONFIG, validatorId: "some-registry-nobody-registered-yet" })).toBe("some-registry-nobody-registered-yet"); + }); + + it("degrades to null (not a crash) for a null/undefined config", () => { + expect(unregisteredValidatorId(null)).toBeNull(); + expect(unregisteredValidatorId(undefined)).toBeNull(); + }); + + it("SECURITY: a validatorId matching an inherited Object.prototype key is still reported as unregistered (Object.hasOwn, not `in`/bracket-truthiness)", () => { + expect(unregisteredValidatorId({ ...EMPTY_CONFIG, validatorId: "toString" })).toBe("toString"); + expect(unregisteredValidatorId({ ...EMPTY_CONFIG, validatorId: "constructor" })).toBe("constructor"); + expect(unregisteredValidatorId({ ...EMPTY_CONFIG, validatorId: "hasOwnProperty" })).toBe("hasOwnProperty"); + }); +}); + describe("resolveRegistryLaneSpec (precedence: env kill-switch → per-repo config → allowlist default → inactive)", () => { const REPO = "SomeoneElse/other-registry"; diff --git a/test/unit/content-lane-wire.test.ts b/test/unit/content-lane-wire.test.ts index 577f56f9c6..f3148cbeab 100644 --- a/test/unit/content-lane-wire.test.ts +++ b/test/unit/content-lane-wire.test.ts @@ -434,4 +434,94 @@ describe("evaluateWithSurfaceLane (the processor seam helper)", () => { expect(out?.conclusion).toBe("failure"); // the same-PR url duplicate closes, exactly as the METAGRAPHED_LANE_SPEC regression test above proves expect(advisory.findings.map((f) => f.code)).toEqual(["surface_lane_reject"]); }); + + it("an UNREGISTERED contentLane.validatorId (operator typo) pushes a non-blocking diagnostic finding into advisory.findings, alongside the degraded structural-only verdict", async () => { + const OTHER_REPO = "SomeoneElse/other-registry"; + const OTHER_ENTRY = "registry/items/foo.json"; + const otherDoc = (items: unknown[]) => JSON.stringify({ items }); + const bodies: Record = { + [`HEAD:${OTHER_ENTRY}`]: otherDoc([{ url: "https://api.example.org/new" }]), + [`BASE:${OTHER_ENTRY}`]: otherDoc([]), + }; + vi.stubGlobal("fetch", async (url: string | URL) => { + const m = /\/contents\/(.+)\?ref=(.+)$/.exec(String(url)); + if (!m) return new Response("nope", { status: 404 }); + const path = m[1]!.split("/").map(decodeURIComponent).join("/"); + const body = bodies[`${decodeURIComponent(m[2]!)}:${path}`]; + return body === undefined ? new Response("missing", { status: 404 }) : new Response(body); + }); + const configuredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const configuredManifest = (): Promise => + Promise.resolve( + parseFocusManifest({ + contentLane: { entryFileGlob: "registry/items/*.json", collectionField: "items", validatorId: "metagraph" }, // typo — should be "metagraphed" + }), + ); + const advisory = { findings: [] as AdvisoryFinding[] }; + const out = await evaluateWithSurfaceLane( + configuredEnv, + OTHER_REPO, + true, + undefined, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory, + getChangedFiles: async () => [{ path: OTHER_ENTRY, status: "modified" }], + }, + configuredManifest, + ); + // Structural gating still runs in degraded (no-validator) mode — a clean, non-duplicate, in-scope append → + // manual (the same "no validator configured" degraded verdict an omitted validatorId gets), NOT a crash. + expect(out?.conclusion).toBe("neutral"); + const codes = advisory.findings.map((f) => f.code); + expect(codes).toContain("surface_lane_unknown_validator_id"); + const warning = advisory.findings.find((f) => f.code === "surface_lane_unknown_validator_id"); + expect(warning?.severity).toBe("warning"); + expect(warning?.detail).toContain('"metagraph"'); + expect(warning?.detail).toContain("metagraphed"); // the known-id hint names the real registered id + }); + + it("a REGISTERED contentLane.validatorId pushes NO unknown-validator diagnostic", async () => { + const advisory = { findings: [] as AdvisoryFinding[] }; + const registeredManifest = (): Promise => + Promise.resolve(parseFocusManifest({ contentLane: { entryFileGlob: "registry/subnets/*.json", collectionField: "surfaces", validatorId: "metagraphed" } })); + const configuredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: "Some/OtherRepo" } as unknown as Env; + await evaluateWithSurfaceLane( + configuredEnv, + REPO, + true, + generic, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory, + getChangedFiles: async () => [{ path: "README.md", status: "modified" }], // not a submission → surface defers + }, + registeredManifest, + ); + expect(advisory.findings.map((f) => f.code)).not.toContain("surface_lane_unknown_validator_id"); + }); + + it("an omitted validatorId (today's zero-config default) pushes NO unknown-validator diagnostic", async () => { + const advisory = { findings: [] as AdvisoryFinding[] }; + const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + await evaluateWithSurfaceLane( + wiredEnv, + REPO, + true, + generic, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory, + getChangedFiles: async () => [{ path: "README.md", status: "modified" }], + }, + noConfigManifest, + ); + expect(advisory.findings).toEqual([]); + }); }); From 63cc3088096397a412565825abb0a0eb27f82782 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:26:59 -0700 Subject: [PATCH 3/5] fix(content-lane): hold the gate on an unreadable manifest, reject over-long globs Two gaps flagged in evaluateWithSurfaceLane and normalizeOptionalGlob: - A non-allowlisted repo's ONLY way to configure a registry content lane is its own .gittensory.yml. A manifest-load failure was caught as null and silently treated the same as "no contentLane configured" -- letting a registry-submission PR merge unevaluated on nothing more than a transient read blip. Now holds the gate neutral in that specific case (never overriding a real generic hard blocker, which is always preserved). - An over-long contentLane glob (entryFileGlob/providerFileGlob/ artifactGlob) was truncated to MAX_ITEM_LENGTH and still returned, silently compiling a DIFFERENT file-scope pattern than configured. Now rejected outright, matching the function's own doc comment and the established pattern used elsewhere in this file. --- src/review/content-lane-wire.ts | 41 +++++++++++++-- src/signals/focus-manifest.ts | 12 +++-- test/unit/content-lane-wire.test.ts | 79 +++++++++++++++++++++++++++++ test/unit/focus-manifest.test.ts | 15 ++++-- 4 files changed, 136 insertions(+), 11 deletions(-) diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index 6d1136c78e..0a3fd09cbe 100644 --- a/src/review/content-lane-wire.ts +++ b/src/review/content-lane-wire.ts @@ -25,6 +25,7 @@ // adjudicator for this structured data — an AI opinion has no standing to veto it, only a real deterministic // blocker does (see guard #1). import { AI_JUDGMENT_BLOCKER_CODES, type GateCheckEvaluation, isAiJudgmentOnlyFailure } from "../rules/advisory"; +import { GITTENSORY_GATE_CHECK_NAME } from "./check-names"; import { isContentLaneEnabled } from "./content-lane/flag"; import { runSurfaceReview, type SurfaceReviewInput, type SurfaceReviewResult } from "./content-lane/orchestrator"; import type { RegistryLaneSpec } from "./content-lane/registry-logic"; @@ -194,8 +195,11 @@ export function resolveSurfaceRefs( * * `loadManifestOverride` is injected by unit tests (mirrors `runRegistrySurfaceGate`'s `loadFileOverride`) so * they never hit the real cached-manifest loader's D1/network I/O; production omits it and gets the real, - * cached `loadRepoFocusManifest`. A manifest-load failure degrades to null (the allowlist-default resolution - * path), never a thrown error. + * cached `loadRepoFocusManifest`. A manifest-load failure never throws OUT of this function: for an allowlisted + * repo it still degrades to the allowlist-default spec (unaffected, since that path never reads the manifest); + * for a non-allowlisted repo — whose ONLY way to configure a spec is that same manifest — it instead holds the + * gate neutral (unless a real generic hard blocker is already present, which is always preserved) rather than + * silently looking identical to "this repo has no content-lane configured at all". * * An unregistered `contentLane.validatorId` in the loaded manifest pushes a non-blocking diagnostic finding * (`unregisteredValidatorIdFinding`) onto `args.advisory.findings` so an operator typo (e.g. "metagraph" instead @@ -216,11 +220,40 @@ export async function evaluateWithSurfaceLane( ): Promise { if (!gateEnabled || !isContentLaneEnabled(env)) return gateEvaluation; const loadManifest = loadManifestOverride ?? loadRepoFocusManifest; - const manifest = await loadManifest(env, repoFullName).catch(() => null); + // loadRepoFocusManifest itself already degrades a fetch/parse blip to an EMPTY manifest (a legitimate "no + // config" signal) internally, so this catch only fires for a rarer failure outside that (e.g. the cache + // read/write layer). Track that distinctly from a genuinely-empty manifest: for a repo NOT on the + // isConvergenceRepoAllowed cutover list, `contentLane:` in its own `.gittensory.yml` is the ONLY way to + // resolve a spec (#2435) -- so `manifest` reading as absent here is indistinguishable, downstream, from + // "this repo never configured content-lane at all", and would silently skip the registry gate on nothing + // more than a transient read failure for exactly the self-hosted-maintainer use case this PR exists to + // support. An allowlisted repo is unaffected either way, since its fallback (METAGRAPHED_LANE_SPEC) never + // depends on the manifest. + let manifest: FocusManifest | undefined; + let manifestLoadFailed = false; + try { + manifest = await loadManifest(env, repoFullName); + } catch { + manifestLoadFailed = true; + } const badValidatorId = unregisteredValidatorId(manifest?.contentLane); if (badValidatorId) args.advisory.findings.push(unregisteredValidatorIdFinding(badValidatorId)); const spec = resolveRegistryLaneSpec(env, manifest, repoFullName); - if (!spec) return gateEvaluation; + if (!spec) { + // A real hard blocker the generic gate already raised (e.g. a committed secret) must never be cleared by + // this path — mirrors applySurfaceGate's own guard #1. Only override when there is nothing to preserve. + if (!manifestLoadFailed || (gateEvaluation && gateEvaluation.blockers.length > 0)) return gateEvaluation; + // We could not read this repo's manifest AND it resolved to no spec — cannot rule out a configured + // contentLane block being silently skipped. Hold rather than let this look identical to "not configured". + return { + enabled: true, + conclusion: "neutral", + title: `${GITTENSORY_GATE_CHECK_NAME} — held for human review`, + summary: "The repo's .gittensory.yml could not be read, so Gittensory cannot confirm whether a registry content-lane is configured for this repo. The gate is held for a human reviewer rather than silently skipping the registry check. It re-evaluates on the next update.", + blockers: [], + warnings: gateEvaluation?.warnings ?? [], + }; + } const surfaceGate = await runRegistrySurfaceGate(env, spec, { installationId: args.installationId, repoFullName, diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index 3e637b30eb..a34b0af7a7 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -636,15 +636,19 @@ function normalizeOptionalGlob(value: JsonValue | undefined, field: string, warn const normalized = normalizeOptionalString(value, field, warnings); if (normalized === null) return null; if (normalized.length > MAX_ITEM_LENGTH) { - warnings.push(`Manifest field "${field}" truncated an over-long glob.`); + // REJECT, not truncate: cutting characters out of a glob changes which files it matches (e.g. a + // mid-directory-name cut can turn a narrow, intended pattern into one that matches an unrelated path + // prefix, or one that never matches anything) — silently compiling a DIFFERENT pattern than the + // maintainer configured is worse than making them shorten an over-complex glob. + warnings.push(`Manifest field "${field}" is an over-long glob (${normalized.length} > ${MAX_ITEM_LENGTH} chars); ignoring it.`); + return null; } - const bounded = normalized.slice(0, MAX_ITEM_LENGTH); - const wildcardCount = (bounded.match(/\*/g) ?? []).length; + const wildcardCount = (normalized.match(/\*/g) ?? []).length; if (wildcardCount > MAX_GLOB_WILDCARDS) { warnings.push(`Manifest field "${field}" has too many wildcards (${wildcardCount} > ${MAX_GLOB_WILDCARDS}); ignoring it.`); return null; } - return bounded; + return normalized; } /** diff --git a/test/unit/content-lane-wire.test.ts b/test/unit/content-lane-wire.test.ts index f3148cbeab..b110aec6aa 100644 --- a/test/unit/content-lane-wire.test.ts +++ b/test/unit/content-lane-wire.test.ts @@ -349,6 +349,85 @@ describe("evaluateWithSurfaceLane (the processor seam helper)", () => { expect(out).toBe(generic); // degrades to the allowlist-default resolution path, never throws }); + it("REGRESSION (#confirmed-bug): holds the gate NEUTRAL — rather than silently passing — when a NON-allowlisted repo's manifest fails to load, since that's the only way it could have configured a contentLane", async () => { + // OTHER_REPO is NOT in GITTENSORY_REVIEW_REPOS, so its only path to a resolved spec is an explicit + // contentLane: config in its OWN .gittensory.yml. If we can't even read that file, we cannot tell "this + // repo never configured content-lane" apart from "it did, but we couldn't check this pass" — silently + // falling through to the plain generic (clean) evaluation would let a real registry submission merge + // unevaluated. + const OTHER_REPO = "SomeoneElse/other-registry"; + const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const throwingLoader = (): Promise => Promise.reject(new Error("simulated D1/network failure")); + const out = await evaluateWithSurfaceLane( + wiredEnv, + OTHER_REPO, + true, + generic, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory: { findings: [] }, + getChangedFiles: async () => { + throw new Error("getChangedFiles must NOT be called — held before any surface-lane fetch"); + }, + }, + throwingLoader, + ); + expect(out?.conclusion).toBe("neutral"); + expect(out?.blockers).toEqual([]); + expect(out?.title).toMatch(/held for human review/i); + }); + + it("REGRESSION (#confirmed-bug): the neutral hold has empty warnings when there was no generic gate evaluation at all", async () => { + const OTHER_REPO = "SomeoneElse/other-registry"; + const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const throwingLoader = (): Promise => Promise.reject(new Error("simulated D1/network failure")); + const out = await evaluateWithSurfaceLane( + wiredEnv, + OTHER_REPO, + true, + undefined, // no generic gate evaluation to fall back to for warnings + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory: { findings: [] }, + getChangedFiles: async () => { + throw new Error("getChangedFiles must NOT be called — held before any surface-lane fetch"); + }, + }, + throwingLoader, + ); + expect(out?.conclusion).toBe("neutral"); + expect(out?.warnings).toEqual([]); + }); + + it("REGRESSION (#confirmed-bug): a real generic hard blocker survives a NON-allowlisted repo's manifest-load failure — never cleared to neutral", async () => { + const OTHER_REPO = "SomeoneElse/other-registry"; + const secret: AdvisoryFinding = { code: "secret_leak", title: "Secret", severity: "critical", detail: "leaked" }; + const genericWithBlocker = gate({ conclusion: "failure", blockers: [secret], warnings: [] }); + const wiredEnv = { GITTENSORY_REVIEW_CONTENT_LANE: "true", GITTENSORY_REVIEW_REPOS: REPO } as unknown as Env; + const throwingLoader = (): Promise => Promise.reject(new Error("simulated D1/network failure")); + const out = await evaluateWithSurfaceLane( + wiredEnv, + OTHER_REPO, + true, + genericWithBlocker, + { + installationId: null, + pr: { headSha: "HEAD", baseRef: "BASE" }, + repo: { defaultBranch: "main" }, + advisory: { findings: [] }, + getChangedFiles: async () => { + throw new Error("getChangedFiles must NOT be called — held before any surface-lane fetch"); + }, + }, + throwingLoader, + ); + expect(out).toBe(genericWithBlocker); // the real hard blocker is preserved, not overridden to neutral + }); + it("activates the surface lane for a NON-metagraphed repo purely from an explicit contentLane: config — no allowlist entry needed", async () => { const OTHER_REPO = "SomeoneElse/other-registry"; const OTHER_ENTRY = "registry/items/foo.json"; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index c0142fe76e..e8df556612 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1050,6 +1050,12 @@ describe("parseFocusManifest gate config", () => { expect(parseFocusManifest({ contentLane: contentLaneConfigToJson(m.contentLane) }).contentLane).toEqual(m.contentLane); }); + it("accepts a wildcard-free (literal) entryFileGlob — a single exact-path registry has no `*` to count", () => { + const m = parseFocusManifest({ contentLane: { entryFileGlob: "registry/items.json", collectionField: "items" } }); + expect(m.contentLane.entryFileGlob).toBe("registry/items.json"); + expect(m.warnings.some((w) => /entryFileGlob/.test(w))).toBe(false); + }); + it("requires BOTH entryFileGlob and collectionField for contentLane: — a partial config warns and is ignored (not a broken half-spec)", () => { const missingCollectionField = parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json" } }); expect(missingCollectionField.contentLane.present).toBe(false); @@ -1083,11 +1089,14 @@ describe("parseFocusManifest gate config", () => { ).toBe(5); }); - it("contentLane: glob fields are truncated at MAX_ITEM_LENGTH like any other string field", () => { + it("REGRESSION: an over-long contentLane glob is REJECTED, not truncated — truncation would silently compile a DIFFERENT pattern than configured", () => { + // A prior version truncated an over-long glob to MAX_ITEM_LENGTH and still returned it, which changes which + // files it matches (e.g. a mid-directory-name cut can match an unrelated path prefix, or match nothing). const overLong = "registry/" + "a".repeat(400) + ".json"; const m = parseFocusManifest({ contentLane: { entryFileGlob: overLong, collectionField: "items" } }); - expect(m.contentLane.entryFileGlob?.length).toBeLessThanOrEqual(300); - expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*truncated/.test(w))).toBe(true); + expect(m.contentLane.entryFileGlob).toBeNull(); + expect(m.contentLane.present).toBe(false); // entryFileGlob is REQUIRED — a rejected glob degrades to absent + expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*over-long glob/.test(w))).toBe(true); }); it("SECURITY (ReDoS): a glob with too many wildcards is REJECTED at parse time rather than ever reaching RegExp compilation", () => { From 47f71f14a1ad1bbea1ec27354671e74427c523a1 Mon Sep 17 00:00:00 2001 From: ghost <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:35:35 -0700 Subject: [PATCH 4/5] fix(signals): cap globToRegExp wildcard count to prevent ReDoS (#2445) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * fix(signals): cap globToRegExp wildcard count to prevent ReDoS hardGuardrailGlobs (src/review/guardrail-config.ts) and any future maintainer-supplied glob compiled via globToRegExp are vulnerable to catastrophic backtracking on chained `*` wildcards: 5 chained wildcards against a 300-char adversarial path took ~19 seconds. Cap wildcard count at compile time (MAX_GLOB_WILDCARDS = 6, well above any of the ~10 real guardrail globs today). An over-complex glob is treated as matching every path rather than failing open, since a guardrail's job is to force manual review on uncertainty — mirroring isGuardrailHit's existing "unknown ⇒ treat as a hit" fail-safe direction. * fix(signals): bake the ReDoS wildcard cap into globToRegExp itself The wildcard-count guard previously lived only in matchesAny's wrapper, so any other direct caller of the exported globToRegExp (e.g. content-lane/spec-resolver.ts) could still compile and .test() a pathological glob and hit catastrophic backtracking — the exact gap this PR set out to close. globToRegExp now short-circuits an over-complex glob to a never-matching sentinel regex instead of compiling it, so every caller (present or future, direct or indirect) is protected automatically. "Never matches" (not "matches everything") is the correct default for the general-purpose compiler, since a false "matches everything" would misclassify unrelated files for a non-guardrail caller; matchesAny keeps its own override to the opposite fail direction for guardrail semantics specifically. * fix(signals): count wildcard GROUPS, not raw * characters, for the ReDoS cap The flagged blocker was correct: MAX_GLOB_WILDCARDS=6 let the PR's own motivating example (5 chained wildcards, empirically catastrophic) through uncapped. Lowering the raw-character cap to 2 fixed that but broke a real consumer -- content-lane/spec-resolver.ts's artifactGlob "public/**/*.json" has 3 raw '*' characters (** counts as two) despite being empirically instant even against a 4,000-char adversarial path, because a `**` globstar compiles to a single .* group, not two independent wildcards. Re-benchmarked with the right unit (wildcard GROUPS, where a ** pair is one group): 2 groups stays sub-second even at 32,000 adversarial chars; 3 groups is already dangerous (over 2s at ~4,000 chars for a chained-* shape, over 100ms at ~1,600 chars for a chained-** shape); 4+ groups is catastrophic (35s at 1,614 chars for 4 chained ** groups). Caps MAX_GLOB_WILDCARD_GROUPS at 2 -- the highest value proven safe -- via a new countWildcardGroups that mirrors globToRegExp's own tokenization (consuming a ** pair, and its trailing /, as one group), so both the original flagged case and real 2-group globs like public/**/*.json are handled correctly. --- src/signals/change-guardrail.ts | 77 ++++++++++++++++++++++++++-- test/unit/change-guardrail.test.ts | 80 +++++++++++++++++++++++++++++- 2 files changed, 153 insertions(+), 4 deletions(-) diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index 437f3f2c66..51fff896c4 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -12,11 +12,74 @@ function canonicalize(value: string): string { return value.replace(/\\/g, "/").replace(/^\.\//, "").replace(/^\/+/, "").toLowerCase(); } +// globToRegExp's COMPILATION is linear-time, but the COMPILED pattern's .test() can be polynomial-to-exponential +// time on an adversarial near-miss input when MULTIPLE wildcard GROUPS chain in one glob (a "group" is one `*` +// OR one `**` — a `**` pair compiles to a SINGLE `.*`, not two independent wildcards, so it must be counted as +// ONE group, not two characters; see countWildcardGroups below). Both group TYPES contribute to the same danger +// once chained — `[^/]*` groups separated by a literal that class doesn't exclude (e.g. "-", not "/") back- +// track ambiguously, and `.*` groups back-track ambiguously EVEN when "/"-separated, since `.*` crosses `/` +// freely. Re-benchmarked against `path` lengths GitHub can plausibly deliver via a deeply nested file path in a +// malicious PR (both `path` and, via `.gittensory.yml`'s contentLane.*Glob fields, the glob itself can be +// attacker-influenced in the same PR): +// 2 wildcard groups (any mix of `*`/`**`, any arrangement): sub-second even at a wildly implausible 32,000- +// char adversarial path (worst case observed: ~400ms) — quadratic, bounded, never a +// realistic hang. +// 3 wildcard groups: OVER 2 SECONDS at just ~4,000 chars for one chained-`*` shape, over 100ms at ~1,600 +// chars for a chained-`**` shape — already dangerous well within a plausible path length. +// 4+ wildcard groups: confirmed catastrophic — 35 SECONDS at just 1,614 chars for 4 chained `**` groups. +// hardGuardrailGlobs today are 100% hardcoded engine constants (see review/guardrail-config.ts) — no +// maintainer/contributor input reaches globToRegExp via that path today, and none of those real globs exceed 1 +// wildcard group — but it is also exported for reuse by other maintainer-config-driven consumers +// (content-lane/spec-resolver.ts, whose real globs like "public/**/*.json" are exactly 2 groups: this cap must +// stay inclusive of that legitimate shape, not just "safer than before"), so the cap lives INSIDE globToRegExp +// itself (not just in a wrapper like matchesAny below) — every caller, present or future, direct or indirect, is +// protected automatically rather than needing to separately remember the risk. The boundary is set at the +// highest GROUP count proven safe by the benchmark above (2) — a boundary that itself sits inside the +// empirically dangerous range would defeat the point of a cap. +const MAX_GLOB_WILDCARD_GROUPS = 2; + +/** Count `*` GROUPS in `glob` — a `**` pair is ONE group (it compiles to a single `.*`, see globToRegExp), not + * two. Mirrors globToRegExp's own tokenization exactly (including consuming a `**`'s trailing `/`) so the count + * reflects the actual number of backtracking-capable groups the compiled RegExp will contain, not raw `*` + * character count (which would double-count every globstar and reject legitimate globs like + * "public/**\/*.json" — 2 real groups — as if they were 3-groups-dangerous). */ +function countWildcardGroups(glob: string): number { + let count = 0; + for (let i = 0; i < glob.length; i += 1) { + if (glob.charAt(i) !== "*") continue; + count += 1; + if (glob.charAt(i + 1) === "*") { + i += 1; // consume the second star of the "**" pair — one group, not two + if (glob.charAt(i + 1) === "/") i += 1; // `**/` also matches zero segments, mirroring globToRegExp + } + } + return count; +} + +/** True if `glob` has more wildcard GROUPS than can be safely compiled to a RegExp without risking catastrophic + * backtracking (see the MAX_GLOB_WILDCARD_GROUPS rationale above). */ +function hasUnsafeWildcardCount(glob: string): boolean { + return countWildcardGroups(glob) > MAX_GLOB_WILDCARD_GROUPS; +} + +// A RegExp that never matches any input, at any position — the safe, conservative compiled form of an +// over-complex glob. "Never matches" (not "matches everything") is the correct default HERE because +// globToRegExp has no context on caller intent, and a false "matches everything" would be actively wrong for a +// non-guardrail caller (e.g. content-lane file-scope matching, where "matches everything" would misclassify +// every changed file as a registry submission). A caller whose OWN semantics want the opposite fail direction +// (a security guardrail, where under-protection is worse than an unnecessary hold) checks hasUnsafeWildcardCount +// itself and overrides — see matchesAny below. +const NEVER_MATCHES = /^(?!)$/; + /** Convert a path glob (`*` matches within a segment, `**` matches across `/`) to an anchored RegExp. The * glob is canonicalized first, so matching is case-insensitive against a canonicalized path. Exported for * reuse anywhere a maintainer-supplied path pattern needs compiling — never compile a raw regex string from - * config (ReDoS risk); this linear-time glob compiler is the one safe path pattern this codebase uses. */ + * config (ReDoS risk); this linear-time glob compiler is the one safe path pattern this codebase uses. + * + * An over-complex glob (see MAX_GLOB_WILDCARD_GROUPS) short-circuits to NEVER_MATCHES instead of being compiled — + * this function never returns a RegExp that risks catastrophic backtracking on .test(), for any input. */ export function globToRegExp(glob: string): RegExp { + if (hasUnsafeWildcardCount(glob)) return NEVER_MATCHES; const canonical = canonicalize(glob); let re = ""; for (let i = 0; i < canonical.length; i += 1) { @@ -38,10 +101,18 @@ export function globToRegExp(glob: string): RegExp { return new RegExp(`^${re}$`); } -/** True if `path` matches any of the globs (`*` within a segment, `**` across `/`), case-insensitively. */ +/** + * True if `path` matches any of the globs (`*` within a segment, `**` across `/`), case-insensitively. A glob + * with more wildcards than can be safely compiled (see hasUnsafeWildcardCount) is treated as matching EVERY + * path — fail SAFE TOWARD GUARDING, mirroring isGuardrailHit's own "unknown ⇒ treat as a hit" philosophy (an + * over-complex guardrail glob still forces manual review) rather than the NEVER_MATCHES default globToRegExp + * itself falls back to, which would silently disable the maintainer's intended protection — the worse failure + * mode for a safety guardrail specifically (see globToRegExp's own docstring for why NEVER_MATCHES is still the + * right default for globToRegExp as a general-purpose compiler). + */ export function matchesAny(path: string, globs: string[]): boolean { const canonicalPath = canonicalize(path); - return globs.some((g) => globToRegExp(g).test(canonicalPath)); + return globs.some((g) => hasUnsafeWildcardCount(g) || globToRegExp(g).test(canonicalPath)); } /** diff --git a/test/unit/change-guardrail.test.ts b/test/unit/change-guardrail.test.ts index 7246724c2f..65ab3a1bb0 100644 --- a/test/unit/change-guardrail.test.ts +++ b/test/unit/change-guardrail.test.ts @@ -1,5 +1,45 @@ import { describe, expect, it } from "vitest"; -import { changedPathsHittingGuardrail, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; +import { changedPathsHittingGuardrail, globToRegExp, isGuardrailHit, matchesAny } from "../../src/signals/change-guardrail"; + +describe("globToRegExp (the exported compiler itself — must be safe for ANY direct caller, not just matchesAny)", () => { + it("compiles an ordinary glob to a working anchored RegExp", () => { + expect(globToRegExp("scripts/**").test("scripts/build.mjs")).toBe(true); + expect(globToRegExp("src/*.ts").test("src/auth.ts")).toBe(true); + expect(globToRegExp("src/*.ts").test("src/auth/session.ts")).toBe(false); + }); + + it("SECURITY (ReDoS): called DIRECTLY (bypassing matchesAny entirely) on a pathological glob, resolves instantly against a genuinely adversarial multi-KB path and matches nothing — the cap lives inside the compiler itself, not just in matchesAny's wrapper", () => { + // 3 chained single-segment wildcards is already empirically dangerous (over 2 seconds at ~4,000 chars — see + // MAX_GLOB_WILDCARD_GROUPS's rationale), so this glob alone proves the cap rejects the FIRST unsafe value, not + // just an extreme one. + const pathological = "src/*-*-*-final.ts"; + const adversarialPath = "src/" + "a-".repeat(2000) + "X"; // ~4,000 chars — the empirically dangerous length for 3 wildcards + const start = Date.now(); + const compiled = globToRegExp(pathological); + expect(compiled.test(adversarialPath)).toBe(false); + expect(compiled.test("completely/unrelated/path.md")).toBe(false); + expect(compiled.test("")).toBe(false); + expect(compiled.test("src/a-b-c-final.ts")).toBe(false); // even a "near miss" that would otherwise match + expect(Date.now() - start).toBeLessThan(1000); + }); + + it("a glob AT the safe cap (2 wildcards), called directly, still compiles and matches normally — proves the cap is inclusive, not exclusive", () => { + const atCap = "src/*/*.ts"; + expect(globToRegExp(atCap).test("src/a/f.ts")).toBe(true); + expect(globToRegExp(atCap).test("src/a/f.js")).toBe(false); + }); + + it("SECURITY (ReDoS, correctness of the group-vs-character count): a single `**` globstar is ONE wildcard group (not two), so it never gets anywhere near the cap on its own", () => { + expect(globToRegExp("scripts/**").test("scripts/deep/nested/build.mjs")).toBe(true); + }); + + it("a `**` globstar PLUS a single `*` — 3 raw star CHARACTERS but only 2 wildcard GROUPS — compiles and matches normally, not the fail-safe path. This is the real content-lane/spec-resolver.ts shape (e.g. an artifactGlob like \"public/**/*.json\"); counting raw `*` characters instead of groups would wrongly reject it", () => { + const mixed = "public/**/*.json"; + expect(globToRegExp(mixed).test("public/deep/nested/report.json")).toBe(true); + expect(globToRegExp(mixed).test("public/report.json")).toBe(true); // `**/` also matches zero segments + expect(globToRegExp(mixed).test("public/deep/nested/report.txt")).toBe(false); // wrong extension + }); +}); describe("change-guardrail glob matching", () => { it("`**` matches across path separators (a guarded dir guards its whole subtree)", () => { @@ -53,6 +93,44 @@ describe("change-guardrail glob matching", () => { // FAIL-SAFE (#1062): guardrails configured but the changed-file set is empty (unknown) ⇒ treat as a hit. expect(isGuardrailHit([], globs)).toBe(true); }); + + it("SECURITY (ReDoS): a glob with too many chained wildcards no longer risks catastrophic backtracking — it fails SAFE TOWARD GUARDING (matches every path) instead of ever compiling the pathological pattern", () => { + // 3 chained single-segment wildcards is already empirically dangerous (see MAX_GLOB_WILDCARD_GROUPS's rationale: + // over 2 seconds at a ~4,000-char adversarial path) — one over the cap, proving the boundary itself is safe, + // not just an extreme over-the-top example. Must resolve INSTANTLY even against that adversarial length. + const pathological = "src/*-*-*-final.ts"; + const adversarialPath = "src/" + "a-".repeat(2000) + "X"; + const start = Date.now(); + // A pathological guardrail glob still HOLDS the PR for manual review (the safe direction for a guardrail — + // silently disabling protection would be far worse than an unnecessary hold). + expect(matchesAny(adversarialPath, [pathological])).toBe(true); + expect(matchesAny("completely/unrelated/path.md", [pathological])).toBe(true); + expect(matchesAny("", [pathological])).toBe(true); + expect(Date.now() - start).toBeLessThan(1000); + expect(changedPathsHittingGuardrail(["unrelated/file.ts"], [pathological])).toEqual(["unrelated/file.ts"]); + expect(isGuardrailHit(["unrelated/file.ts"], [pathological])).toBe(true); + }); + + it("SECURITY (ReDoS): a glob AT the safe cap (2 wildcards) still compiles and matches NORMALLY (not the fail-safe path)", () => { + // Exactly 2 stars: at the cap, still safely compiled/evaluated — proves the cap is inclusive, not exclusive, + // and that ordinary (non-pathological) multi-wildcard globs keep their real matching semantics. This is also + // the shape of nearly every real guardrail glob in production (a single `**` = 2 wildcard characters). + const atCap = "src/*/*.ts"; + expect(matchesAny("src/a/f.ts", [atCap])).toBe(true); + expect(matchesAny("src/a/f.js", [atCap])).toBe(false); // wrong extension — genuinely doesn't match + }); + + it("a wildcard-free literal glob (e.g. an exact guarded file like '.gittensory.yml') is never treated as unsafe", () => { + expect(matchesAny(".gittensory.yml", [".gittensory.yml"])).toBe(true); + expect(matchesAny("other-file.yml", [".gittensory.yml"])).toBe(false); + }); + + it("a mix of one pathological glob among otherwise-fine globs still forces a hold for ANY path (fail-safe dominates)", () => { + const globs = ["docs/**", "src/*-*-*-final.ts"]; + // "docs/**" alone would not match this path, but the pathological glob's fail-safe short-circuits matchesAny + // to true for every path once any configured glob is judged unsafe to compile. + expect(matchesAny("completely/unrelated.md", globs)).toBe(true); + }); }); // #flood-readiness: the live hard-guardrail globs must guard crucial files that live OUTSIDE the dir-prefix From 536e46a205cca5e2fd272b38a8d85fcf70c98b87 Mon Sep 17 00:00:00 2001 From: JSONbored <49853598+JSONbored@users.noreply.github.com> Date: Wed, 1 Jul 2026 18:49:16 -0700 Subject: [PATCH 5/5] fix(content-lane): share the wildcard-safety predicate between glob parsing and compilation normalizeOptionalGlob (focus-manifest.ts) capped contentLane globs at 3 raw '*' characters, but globToRegExp (change-guardrail.ts) rejects any glob with more than 2 wildcard GROUPS (a '**' pair counts as one group, not two) by compiling it to NEVER_MATCHES. A glob like "a*b*c*.json" (3 groups, no '**' pairs) was accepted as configured but silently could never match any file once compiled -- a content lane that looks active but never fires. Export hasUnsafeWildcardCount from change-guardrail.ts and reuse it directly in normalizeOptionalGlob instead of an independently-counted threshold, so the parser and compiler can never drift apart again. --- src/signals/change-guardrail.ts | 7 +++++-- src/signals/focus-manifest.ts | 29 ++++++++++++----------------- test/unit/focus-manifest.test.ts | 26 +++++++++++++++++++++----- 3 files changed, 38 insertions(+), 24 deletions(-) diff --git a/src/signals/change-guardrail.ts b/src/signals/change-guardrail.ts index 51fff896c4..df716ce438 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -57,8 +57,11 @@ function countWildcardGroups(glob: string): number { } /** True if `glob` has more wildcard GROUPS than can be safely compiled to a RegExp without risking catastrophic - * backtracking (see the MAX_GLOB_WILDCARD_GROUPS rationale above). */ -function hasUnsafeWildcardCount(glob: string): boolean { + * backtracking (see the MAX_GLOB_WILDCARD_GROUPS rationale above). Exported so any OTHER glob-accepting config + * surface (e.g. focus-manifest.ts's contentLane.*Glob parsing) can reject an over-complex glob using the SAME + * predicate globToRegExp itself enforces — a caller with its own, independently-counted threshold could accept + * a glob globToRegExp then silently compiles to NEVER_MATCHES, configuring a lane that can never activate. */ +export function hasUnsafeWildcardCount(glob: string): boolean { return countWildcardGroups(glob) > MAX_GLOB_WILDCARD_GROUPS; } diff --git a/src/signals/focus-manifest.ts b/src/signals/focus-manifest.ts index a34b0af7a7..6ec35db001 100644 --- a/src/signals/focus-manifest.ts +++ b/src/signals/focus-manifest.ts @@ -3,6 +3,7 @@ import type { GatePolicyPack, GateRuleMode, JsonValue, RepositorySettings } from import { normalizeAutonomyPolicy, normalizeAutoMaintainPolicy } from "../settings/autonomy"; import { normalizeCommandAuthorizationPolicy } from "../settings/command-authorization"; import { mergeContributorBlacklists, normalizeContributorBlacklist } from "../settings/contributor-blacklist"; +import { hasUnsafeWildcardCount } from "./change-guardrail"; import { PUBLIC_LOCAL_PATH_INLINE } from "./redaction"; export type FocusManifestSource = "repo_file" | "api_record" | "none"; @@ -618,20 +619,15 @@ function normalizeOptionalPositiveInteger(value: JsonValue | undefined, field: s return null; } -// A glob compiled to RegExp (review/content-lane/spec-resolver.ts's globToRegExp reuse of the guardrail-path -// compiler) chains a `[^/]*` per `*` — MULTIPLE chained wildcards separated by literal characters can -// catastrophically backtrack on an adversarial near-miss input (verified empirically: 5 chained wildcards against -// a maximally-adversarial 300-char input took ~19 SECONDS; 3 stays under 5ms even at that same length). No -// legitimate single-purpose entry-file glob for this feature needs more than a couple of wildcards, so this caps -// wildcard count at parse time — well before the string ever reaches RegExp compilation — rather than trying to -// make the compiled pattern itself provably safe. -const MAX_GLOB_WILDCARDS = 3; - -/** Normalize + bound a maintainer-supplied glob string: trims/length-caps like any other string field, AND caps - * the number of `*` wildcard characters (see MAX_GLOB_WILDCARDS) so it can never compile into a - * catastrophically-backtracking RegExp downstream. A glob over the cap is REJECTED (warns, returns null) rather - * than truncated — silently cutting wildcards out of a maintainer's pattern would silently change its meaning, - * which is worse than making them fix an over-complex glob. */ +/** Normalize + bound a maintainer-supplied glob string: trims/length-caps like any other string field, AND + * rejects one globToRegExp (review/content-lane/spec-resolver.ts's reuse of the guardrail-path compiler) would + * itself refuse to compile safely. Reuses `hasUnsafeWildcardCount` — globToRegExp's OWN safety predicate — + * rather than a locally-counted threshold: a caller that counts wildcards differently (e.g. raw `*` characters, + * which double-counts a `**` pair as 2 groups instead of 1) can accept a glob globToRegExp then silently + * compiles to NEVER_MATCHES, configuring a lane that is "present" but can never activate on any changed file + * (#confirmed-bug). A glob over the cap is REJECTED (warns, returns null) rather than truncated — silently + * cutting wildcards out of a maintainer's pattern would silently change its meaning, which is worse than making + * them fix an over-complex glob. */ function normalizeOptionalGlob(value: JsonValue | undefined, field: string, warnings: string[]): string | null { const normalized = normalizeOptionalString(value, field, warnings); if (normalized === null) return null; @@ -643,9 +639,8 @@ function normalizeOptionalGlob(value: JsonValue | undefined, field: string, warn warnings.push(`Manifest field "${field}" is an over-long glob (${normalized.length} > ${MAX_ITEM_LENGTH} chars); ignoring it.`); return null; } - const wildcardCount = (normalized.match(/\*/g) ?? []).length; - if (wildcardCount > MAX_GLOB_WILDCARDS) { - warnings.push(`Manifest field "${field}" has too many wildcards (${wildcardCount} > ${MAX_GLOB_WILDCARDS}); ignoring it.`); + if (hasUnsafeWildcardCount(normalized)) { + warnings.push(`Manifest field "${field}" has too many wildcards to compile safely; ignoring it.`); return null; } return normalized; diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index e8df556612..c29a9e0f5d 100644 --- a/test/unit/focus-manifest.test.ts +++ b/test/unit/focus-manifest.test.ts @@ -1107,18 +1107,34 @@ describe("parseFocusManifest gate config", () => { expect(m.contentLane.entryFileGlob).toBeNull(); expect(m.contentLane.present).toBe(false); // entryFileGlob is REQUIRED — a rejected glob degrades to absent expect(m.warnings.some((w) => /contentLane\.entryFileGlob.*too many wildcards/.test(w))).toBe(true); - // A glob AT the cap (3 wildcards) is accepted; the optional providerFileGlob/artifactGlob fields are dropped - // individually (with a warning) without invalidating the whole block, since only entryFileGlob/collectionField - // are required. + // A glob AT the cap (2 wildcard GROUPS — matches globToRegExp's own MAX_GLOB_WILDCARD_GROUPS) is accepted; + // the optional providerFileGlob/artifactGlob fields are dropped individually (with a warning) without + // invalidating the whole block, since only entryFileGlob/collectionField are required. const atCap = parseFocusManifest({ - contentLane: { entryFileGlob: "registry/*/*/*.json", providerFileGlob: "providers/*-*-*-*-*.json", collectionField: "items" }, + contentLane: { entryFileGlob: "registry/*/*.json", providerFileGlob: "providers/*-*-*-*-*.json", collectionField: "items" }, }); expect(atCap.contentLane.present).toBe(true); - expect(atCap.contentLane.entryFileGlob).toBe("registry/*/*/*.json"); + expect(atCap.contentLane.entryFileGlob).toBe("registry/*/*.json"); expect(atCap.contentLane.providerFileGlob).toBeNull(); expect(atCap.warnings.some((w) => /contentLane\.providerFileGlob.*too many wildcards/.test(w))).toBe(true); }); + it("REGRESSION (#confirmed-bug): rejects a glob using the SAME wildcard-GROUP predicate globToRegExp itself enforces, not a raw `*`-character count", () => { + // The exact defect the gate flagged: a glob with 3 wildcard GROUPS (no `**` pairs to consolidate) was + // previously ACCEPTED here (a raw-character count topped out at 3) but compiles to NEVER_MATCHES in + // globToRegExp (whose group-count cap is 2) — configuring a lane that is "present" but can never activate. + const threeGroups = parseFocusManifest({ contentLane: { entryFileGlob: "a*b*c*.json", collectionField: "items" } }); + expect(threeGroups.contentLane.entryFileGlob).toBeNull(); + expect(threeGroups.contentLane.present).toBe(false); + expect(threeGroups.warnings.some((w) => /contentLane\.entryFileGlob.*too many wildcards/.test(w))).toBe(true); + // A `**` pair counts as ONE group (mirroring globToRegExp's own countWildcardGroups), so this 2-group glob — + // the exact shape spec-resolver.ts's own real METAGRAPHED_LANE_SPEC-adjacent globs use — is still accepted + // even though it has 3 raw `*` characters. + const globstarShape = parseFocusManifest({ contentLane: { entryFileGlob: "public/**/*.json", collectionField: "items" } }); + expect(globstarShape.contentLane.entryFileGlob).toBe("public/**/*.json"); + expect(globstarShape.contentLane.present).toBe(true); + }); + it("contentLaneConfigToJson returns null for an absent config, and omits unset optional fields", () => { expect(contentLaneConfigToJson(parseFocusManifest(null).contentLane)).toBeNull(); const m = parseFocusManifest({ contentLane: { entryFileGlob: "registry/*.json", collectionField: "items" } });