diff --git a/src/review/content-lane-wire.ts b/src/review/content-lane-wire.ts index 2d60c8172a..0a3fd09cbe 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,32 +25,39 @@ // 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 { GITTENSORY_GATE_CHECK_NAME } from "./check-names"; +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 { 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"; +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. 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"; -/** 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 }; } +/** 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. */ @@ -106,14 +116,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 +150,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 +178,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 +191,19 @@ 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 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 + * of "metagraphed") is visible in the PR comment instead of silently degrading to structural-only review. */ export async function evaluateWithSurfaceLane( env: Env, repoFullName: string, @@ -188,9 +216,45 @@ 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; + // 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) { + // 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, 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..c3de4d99d6 --- /dev/null +++ b/src/review/content-lane/spec-resolver.ts @@ -0,0 +1,89 @@ +// 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 && Object.hasOwn(REGISTRY_VALIDATORS, 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 ?? {}), + }; +} + +/** + * True when `config.validatorId` is set but does not match any REGISTRY_VALIDATORS entry — most likely an + * operator typo in `.gittensory.yml`'s `contentLane.validatorId` (e.g. "metagraph" instead of "metagraphed"). + * `buildRegistryLaneSpecFromConfig` above already degrades this to structural-only gating silently (never a + * crash — a brand-new registry with no validator contributed yet is a legitimate config), which makes a typo + * indistinguishable from a deliberate choice. Checked as a SEPARATE pure function so a caller (see + * `evaluateWithSurfaceLane` in `content-lane-wire.ts`) can surface it as an operator-visible advisory finding + * without changing `buildRegistryLaneSpecFromConfig`'s established `RegistryLaneSpec | null` return contract. + * Returns the offending id, or null when there is nothing to warn about (no validatorId configured, or it + * resolves). PURE. + */ +export function unregisteredValidatorId(config: FocusManifestContentLaneConfig | null | undefined): string | null { + if (!config?.validatorId) return null; + return Object.hasOwn(REGISTRY_VALIDATORS, config.validatorId) ? null : config.validatorId; +} + +/** The validatorId strings a maintainer's `.gittensory.yml` `contentLane.validatorId` can currently reference — + * exposed so a caller can render a helpful "known validators are: X, Y" hint alongside an + * `unregisteredValidatorId` warning, without reaching into REGISTRY_VALIDATORS directly. */ +export function registeredValidatorIds(): string[] { + return Object.keys(REGISTRY_VALIDATORS); +} + +/** + * 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..df716ce438 100644 --- a/src/signals/change-guardrail.ts +++ b/src/signals/change-guardrail.ts @@ -12,9 +12,77 @@ 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). 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; +} + +// 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. */ -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. + * + * 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) { @@ -36,10 +104,18 @@ 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/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..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"; @@ -49,9 +50,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 +63,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 +220,7 @@ export type FocusManifest = { settings: FocusManifestSettings; review: FocusManifestReviewConfig; features: FocusManifestFeaturesConfig; + contentLane: FocusManifestContentLaneConfig; warnings: string[]; }; @@ -269,6 +293,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 +319,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 +340,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 +609,86 @@ 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; +} + +/** 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; + if (normalized.length > MAX_ITEM_LENGTH) { + // 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; + } + if (hasUnsafeWildcardCount(normalized)) { + warnings.push(`Manifest field "${field}" has too many wildcards to compile safely; ignoring it.`); + return null; + } + return normalized; +} + +/** + * 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 +1190,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 +1205,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/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 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..b931ac39d1 --- /dev/null +++ b/test/unit/content-lane-spec-resolver.test.ts @@ -0,0 +1,157 @@ +import { describe, expect, it } from "vitest"; +import { buildRegistryLaneSpecFromConfig, registeredValidatorIds, resolveRegistryLaneSpec, unregisteredValidatorId } 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("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"; + + 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..b110aec6aa 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,310 @@ 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("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"; + 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"]); + }); + + 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([]); }); }); diff --git a/test/unit/focus-manifest.test.ts b/test/unit/focus-manifest.test.ts index ee2bf488b7..c29a9e0f5d 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,124 @@ 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("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); + 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("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).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", () => { + // 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 (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" }, + }); + 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("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" } }); + 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);