Skip to content

engine(gate): reject over-complex screenshotTableGate whenPaths globs instead of letting them match every path #9993

Description

@JSONbored

⚠️ Definition of Done: this issue must be completed in full, in a single PR. Do not split this
work across multiple PRs, and do not defer any Deliverable below to a follow-up issue. A PR that
satisfies only some of the Deliverables, stubs a required test, or leaves a checkbox
partially-done does NOT resolve this issue and will be closed.

Context

matchesAny fails TOWARD matching for a glob whose wildcard-group count exceeds the compiler's safety cap.
packages/loopover-engine/src/signals/change-guardrail.ts:114:

export function matchesAny(path: string, globs: string[]): boolean {
  const canonicalPath = canonicalize(path);
  return globs.some((g) => hasUnsafeWildcardCount(g) || globToRegExp(g).test(canonicalPath));
}

That is the right default for hardGuardrailGlobs, its original caller: an over-complex guardrail glob
still forces a human hold. It is the WRONG default for the screenshot-table gate's whenPaths, which
matchesAny also backs (via matchesAnyWithExclusions, whose include half "reuses matchesAny unchanged"
— see packages/loopover-engine/src/signals/change-guardrail.ts:146), and where "matches everything" means
"every PR in the repo is in scope".

whenPaths is normalized by normalizeStringList, which validates type, emptiness, count and length —
but never wildcard count. packages/loopover-engine/src/review/screenshot-table-gate.ts:42:

    if (typeof item !== "string" || item.trim().length === 0) {
      warnings.push(`settings.requireScreenshotTable.${field}[${index}] must be a non-empty string; ignoring it.`);
      continue;
    }
    out.push(item.trim().slice(0, maxChars));

The cap is MAX_GLOB_WILDCARD_GROUPS = 2 (packages/loopover-engine/src/signals/change-guardrail.ts:37),
where ** counts as ONE group. So a perfectly ordinary monorepo scoping glob such as
apps/**/src/**/*.tsx is 3 groups and trips it.

Consequences today, both silent (no warning is emitted anywhere):

  1. Include glob. isScreenshotTableGateInScope
    (packages/loopover-engine/src/review/screenshot-table-gate.ts:321) calls
    matchesAnyWithExclusions(file, config.whenPaths). With whenPaths: ["apps/**/src/**/*.tsx"] and
    action: "close", matchesAny returns true for EVERY changed file, so every PR in the repo is in
    scope for a close-tier visual gate. hasCommittedImageFile
    (packages/loopover-engine/src/review/screenshot-table-gate.ts:182) uses the same matcher and likewise
    flags any image anywhere in the repo. This is the Screenshot gate auto-closed non-visual PRs via apps/loopover-ui/public/** (openapi.json); verify sibling repos #9434 incident class (5 contributor PRs auto-closed
    one-shot for a generated openapi.json) with a wider blast radius, reached by a typo-free, plausible
    config value.
  2. Exclusion glob. The !-prefixed half deliberately compiles with globToRegExp directly, whose
    over-complex fallback is NEVER_MATCHES
    (packages/loopover-engine/src/signals/change-guardrail.ts:157). So !**/*.generated.* — 3 groups —
    excludes NOTHING, silently reverting the operator to the pre-Screenshot gate auto-closed non-visual PRs via apps/loopover-ui/public/** (openapi.json); verify sibling repos #9434 behaviour the exclusion feature exists
    to fix.

Every OTHER maintainer-supplied glob surface in the manifest already rejects this shape with a warning.
packages/loopover-engine/src/focus-manifest.ts:2300:

  if (hasUnsafeWildcardCount(normalized)) {
    warnings.push(`Manifest field "${field}" has too many wildcards to compile safely; ignoring it.`);
    return null;
  }

hasUnsafeWildcardCount is exported for exactly this reuse
(packages/loopover-engine/src/signals/change-guardrail.ts:62), and screenshot-table-gate.ts does not use it.

Requirements

  • normalizeStringList in packages/loopover-engine/src/review/screenshot-table-gate.ts must gain a
    caller-selected glob mode. When enabled, an entry whose glob body has an unsafe wildcard count
    (per hasUnsafeWildcardCount) is DROPPED with a warning naming the field and index, mirroring the
    message shape already used for the other rejection reasons in that function.
  • The wildcard check must be applied to whenPaths ONLY. whenLabels, requireViewports and
    requireThemes are not globs and must keep their current normalization byte-identical.
  • For a whenPaths entry beginning with !, the wildcard count must be measured on the glob body
    (the string after the leading !), not on the !-prefixed string, so an exclusion is judged by the
    pattern that matchesAnyWithExclusions actually compiles
    (packages/loopover-engine/src/signals/change-guardrail.ts:150).
  • A bare "!" entry (no body) must be dropped with a warning: matchesAnyWithExclusions currently routes
    it into the INCLUDE list because of its glob.length > 1 guard, which is not what an operator writing
    ! means.
  • Dropping an unsafe whenPaths entry must not disable the gate or reset the other entries: the remaining
    valid entries are kept, exactly like the existing per-entry drop behaviour.
  • What must NOT change: matchesAny's fail-toward-matching semantics
    (packages/loopover-engine/src/signals/change-guardrail.ts:114), globToRegExp's NEVER_MATCHES
    fallback, MAX_GLOB_WILDCARD_GROUPS, countWildcardGroups, and hardGuardrailGlobs behaviour must all
    stay byte-identical — the guardrail's fail-safe direction is correct and load-bearing.

⚠️ Required pattern: mirror normalizeOptionalGlob at
packages/loopover-engine/src/focus-manifest.ts:2300 — reuse the exported hasUnsafeWildcardCount
predicate and drop-with-a-warning. What does NOT satisfy this issue: (a) raising
MAX_GLOB_WILDCARD_GROUPS or adding a second, larger threshold for this caller — the cap's value is
backed by the benchmark documented at packages/loopover-engine/src/signals/change-guardrail.ts:24-29
and must not move; (b) changing matchesAny to fail toward NOT matching, which silently weakens
hardGuardrailGlobs across the whole repo; (c) adding a new parallel whenPaths matcher instead of
validating at the normalizer, which would leave the DB-sourced config path unvalidated; (d) a test-only
PR asserting the current fail-open behaviour.

Deliverables

  • packages/loopover-engine/src/review/screenshot-table-gate.ts
    normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["apps/**/src/**/*.tsx"] }, warnings)
    returns whenPaths: [] and pushes a warning naming settings.requireScreenshotTable.whenPaths[0].
  • packages/loopover-engine/src/review/screenshot-table-gate.ts
    normalizeScreenshotTableGateConfig({ enabled: true, whenPaths: ["!**/*.generated.*", "apps/ui/src/**"] }, warnings)
    returns whenPaths: ["apps/ui/src/**"] plus a warning for index 0.
  • packages/loopover-engine/src/review/screenshot-table-gate.ts — a whenPaths entry of exactly "!"
    is dropped with a warning.
  • A regression test at packages/loopover-engine/test/screenshot-table-gate.test.ts (this file does not
    exist yet; create it, importing from ../dist/review/screenshot-table-gate.js per the convention in
    packages/loopover-engine/test/content-lane-flag.test.ts) named for this bug, asserting that a
    3-wildcard-group whenPaths entry no longer puts an unrelated changed file
    (isScreenshotTableGateInScope(config, [], ["README.md"])) in scope — which it does today.
  • Root-suite coverage at test/unit/ (an existing screenshot-table-gate suite if one covers this
    normalizer, otherwise a new file whose exact path the PR states) asserting the same three
    normalization outcomes plus the preserved behaviour: a 2-group glob such as apps/ui/public/**/*.json
    is still accepted unchanged.

All Deliverables above are required in a single PR. A PR that satisfies only some of them — for example one
that adds the wildcard check but only tests it through normalizeScreenshotTableGateConfig and never
asserts the isScreenshotTableGateInScope behaviour change — does not resolve this issue.

Test Coverage Requirements

This repo enforces 99%+ Codecov patch coverage, branch-counted. vitest.config.ts's
coverage.include covers src/**/*.ts and packages/loopover-engine/src/**/*.ts — the touched path
packages/loopover-engine/src/review/screenshot-table-gate.ts IS measured.

Branches introduced or touched, each needing BOTH arms tested: the new glob-mode parameter (on for
whenPaths, off for whenLabels/requireViewports/requireThemes); the hasUnsafeWildcardCount check
(unsafe → dropped, safe → kept); the leading-! strip (entry with !, entry without); and the bare-"!"
guard (length 1 → dropped, length > 1 → body checked).

Engine lines are credited by two uploads whose hits are unioned — add the test to
packages/loopover-engine/test/** as well as any root test/** coverage, or the patch gate can still fail.

Expected Outcome

An operator whose whenPaths glob is too complex for the bounded compiler gets an explicit warning and
that entry is ignored, instead of silently widening a close-tier visual gate to every PR in the repo (or
silently disabling an exclusion). The screenshot gate's glob surface now validates the same way every other
maintainer-supplied glob in .loopover.yml already does.

Links & Resources

Metadata

Metadata

Assignees

No one assigned

    Labels

    gittensor:bugGittensor-scored bug fix — scores a 0.05x multiplier.help wantedExtra attention is needed

    Projects

    No projects

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions