fix(engine): reject over-complex screenshotTableGate whenPaths globs at normalization - #10115
Conversation
…at normalization whenPaths back matchesAnyWithExclusions, whose include half (matchesAny) FAILS TOWARD MATCHING for a glob whose wildcard-group count exceeds the compiler cap -- correct for hardGuardrailGlobs (an over-complex guardrail glob still forces a human hold) but wrong for whenPaths, where matching everything means every PR in the repo is in scope for a close-tier visual gate. normalizeStringList validated type/emptiness/count/length but never wildcard count, so an ordinary monorepo scoping glob like apps/**/src/**/*.tsx (3 groups) silently scoped every PR, and a 3-group exclusion like !**/*.generated.* compiled to NEVER_MATCHES and excluded nothing -- both with no warning. Validate whenPaths at the normalizer with the same hasUnsafeWildcardCount predicate every other manifest glob surface already uses (mirrors focus-manifest's normalizeOptionalGlob): an over-complex entry is dropped with a warning naming the field and index, measured on the glob BODY so an exclusion is judged by the pattern matchesAnyWithExclusions actually compiles, and a bare '!' (which would mis-route into the include list) is dropped too. whenLabels / requireViewports / requireThemes normalization, matchesAny's fail-toward-matching semantics, MAX_GLOB_WILDCARD_GROUPS and hardGuardrailGlobs are all unchanged. Closes JSONbored#9993
|
Tip ✅ LoopOver review result - approve/merge recommendedReview updated: 2026-07-31 08:12:43 UTC
Review summary Nits — 3 non-blocking
Decision drivers
Context & advisory signals — never blocks the verdict
Linked issue satisfactionAddressed Review context
Contributor next steps
Signal definitions
🧪 Chat with LoopOverAsk LoopOver a question about this PR directly in a comment — grounded only in the same cached, public-safe facts shown above, never a new claim.
Full command reference: https://loopover.ai/docs/loopover-commands 🧪 Experimental — new and may change. 🟩 Safe / merged · 🟦 Advisory · 🟨 Held for review · 🟥 Blocked / closed 💰 Earn for open-source contributions like this. Gittensor lets GitHub contributors earn for the work they already do — register to start earning →. Checked by LoopOver, a quiet PR intelligence layer for OSS maintainers.
|
|
Superagent didn't find any vulnerabilities or security issues in this PR. |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #10115 +/- ##
=======================================
Coverage 91.95% 91.95%
=======================================
Files 931 931
Lines 113921 113935 +14
Branches 27504 27514 +10
=======================================
+ Hits 104757 104772 +15
+ Misses 7863 7858 -5
- Partials 1301 1305 +4
Flags with carried forward coverage won't be shown. Click here to find out more.
|
What & why
matchesAnyfails toward matching for a glob whose wildcard-group count exceeds the compiler's safety cap:That is the right default for
hardGuardrailGlobs(an over-complex guardrail glob still forces a human hold). It is the wrong default for the screenshot-table gate'swhenPaths, whichmatchesAnyalso backs (viamatchesAnyWithExclusions), where "matches everything" means "every PR in the repo is in scope".whenPathswas normalized for type/emptiness/count/length but never wildcard count, so:whenPaths: ["apps/**/src/**/*.tsx"](3 groups) makesmatchesAnyreturntruefor every changed file → every PR in scope for a close-tier visual gate, reachable by a typo-free, plausible monorepo scoping value.!**/*.generated.*(3-group body) compiles withglobToRegExpdirectly, whose over-complex fallback isNEVER_MATCHES, so it excludes nothing — silently reverting the operator to the pre-exclusion behaviour.Both silent (no warning anywhere). Every other maintainer-supplied glob surface in the manifest already rejects this shape with a warning via the exported
hasUnsafeWildcardCount—screenshot-table-gate.tswas the one that didn't.The fix
Give
normalizeStringLista caller-selected glob mode, enabled forwhenPathsonly. An entry whose glob body (the string after a leading!) has an unsafe wildcard count is dropped with a warning naming the field and index — mirroringfocus-manifest.ts'snormalizeOptionalGlob. Measuring on the body means an exclusion is judged by the patternmatchesAnyWithExclusionsactually compiles; a bare"!"(no body, whichmatchesAnyWithExclusionswould mis-route into the include list) is dropped too. Remaining valid entries are kept, exactly like the existing per-entry drop behaviour.Unchanged:
whenLabels/requireViewports/requireThemesnormalization is byte-identical;matchesAny's fail-toward-matching semantics,globToRegExp'sNEVER_MATCHESfallback,MAX_GLOB_WILDCARD_GROUPS,countWildcardGroups, andhardGuardrailGlobsbehaviour are all untouched (the guardrail's fail-safe direction is correct and load-bearing). The cap value is not moved and no second threshold is added.Tests
packages/loopover-engine/test/screenshot-table-gate.test.ts, node:test): a 3-groupwhenPathsglob is dropped with a warning and no longer puts an unrelated file (README.md) in scope while the valid entry (apps/ui/src/App.tsx) still is; an exclusion is judged by its body and a bare"!"is dropped; a 2-group glob and a valid exclusion are preserved unchanged.test/unit/screenshot-table-gate.test.ts): the same three normalization outcomes plus theisScreenshotTableGateInScopebehaviour change.mainand pass with the fix.Validation
packages/loopover-engine/src/review/screenshot-table-gate.tsis 100% line and branch (theglobmode, the!-body/no-!arms, and thebody === "" || unsafearms). Engine lines credited via the root-vitest upload; the added test is also inpackages/loopover-engine/test/**for the dual-upload union.npm run typecheckclean for these files;npm run engine-parity:drift-checkpasses; the engine's ownnode --testsuite is green;npm run dead-exports:checkclean.git diff --checkclean; no schema/migration/generated-artifact change.Closes #9993