fix(varlock): regex() takes flags, and say so when a pattern splits - #1087
theoephraim wants to merge 5 commits into
Conversation
github-actions
Bot
added
maintainer
core:website
core:varlock
core:parser
core:vscode-plugin
labels
|
The changes in this PR will be included in the next version bump.
|
📦 Bundle size
dist/ only; native binaries are versioned separately and not counted here. |
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 3a8c2d7 | Commit Preview URL Branch Preview URL |
Sep 15 2026, 09:37 AM |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
There was a problem hiding this comment.
Important
The new token can silently absorb previously separate arguments or decorators when a later value supplies the closing slash.
Reviewed changes in 4068d47: the new regex tokenization path, escape preservation, runtime and editor flag parity, tests, documentation, and package bumps.
- Atomic parser token: adds
/pattern/flagshandling to decorator values, function arguments, and object/array literal elements. - Escape preservation: marks regex literals so escaped dollar signs reach runtime regex construction unchanged.
- Runtime and editor parity: recognizes the
dandvJavaScript regex flags in Varlock and VS Code diagnostics. - Regression coverage and docs: exercises quantifier commas and other regex syntax across parser/runtime paths and documents the accepted literal form.
azure/gpt-5.6-sol ｜ 𝕏
@env-spec/parser
varlock
@varlock/native-helper-darwin
@varlock/native-helper-linux-arm64
@varlock/native-helper-linux-x64
@varlock/native-helper-win32-x64
commit: |
There was a problem hiding this comment.
Important
The new diagnostic introduces no additional issue, but the prior parser-boundary finding remains unresolved.
Reviewed changes since the prior Pullfrog review: the new commit added a targeted hint for regex literals split by an unescaped inner slash.
- Split-regex hint: Detected malformed
matchesvalues that reach the existing mixed positional and named argument error, then attached guidance to escape the inner slash or quote the pattern. - Heuristic boundaries: Limited the hint to regex-accepting option names and kept similar path-shaped and unrelated mixed-argument errors unchanged.
- Regression coverage: Added focused tests for the malformed URL regex, an unrelated mixed-argument call, and a similar path-shaped call.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The decorator boundary remains bypassable when separator whitespace is consumed as a regex escape.
Reviewed changes since the prior Pullfrog review: the decorator-specific regex token and the documentation of intentionally merged function arguments.
- Protected ordinary decorator boundaries: Added
decRegexLiteralso a regex candidate stops before an ordinary whitespace-@sequence, including inside decorator function arguments. - Documented argument ambiguity: Recorded that slash-prefixed function arguments can merge through a later closing slash and explained that quoting preserves separate values.
- Added regression coverage: Tested adjacent decorators, decorator function arguments, email-style patterns, and the intentional argument merge.
azure/gpt-5.6-sol ｜ 𝕏
There was a problem hiding this comment.
Important
The new regression tests introduce no additional issue, but the prior escaped-whitespace decorator-boundary finding remains unresolved.
Reviewed changes since the prior Pullfrog review: the new commit pinned the historical path truncation cases that caused the earlier regex grammar rule to be removed.
- Covered item paths: Added exact assertions that slash-prefixed item values remain intact, with and without a trailing slash.
- Covered function arguments: Added exact assertions that path-shaped function arguments are not partially consumed as regex literals.
- Covered decorator values: Added an exact assertion that a slash-prefixed decorator value remains intact.
azure/gpt-5.6-sol ｜ 𝕏
`@type=string(matches=/^[0-9a-f]{7,40}$/)` fails with "cannot mix positional
args and named options": an unquoted `/pattern/` is an ordinary value, so the
comma in the quantifier ends it and `40}$/` becomes a positional arg.
The pattern is left alone - `/` stays an ordinary character, so paths are never
at risk of being read as regexes. Instead the explicit spelling is made complete
and the error says which one to reach for:
- regex("pattern", "flags") now takes flags as a second arg, so a pattern that
cannot be written unquoted no longer loses the ability to carry them
- the mixed-args error carries a tip when the args look like a split pattern
- `d` and `v` flags are recognized alongside the rest
- docs state the actual rule, and stop calling regex() deprecated
d2cfa5c to
70933e4
Compare
theoephraim
changed the title
There was a problem hiding this comment.
Important
The new two-argument regex() contract is not yet supported by VS Code diagnostics, which can mark runtime-valid values as invalid.
Reviewed changes since the prior Pullfrog review: the force-pushed replacement of grammar-level regex tokenization with an explicit regex(pattern, flags) path and targeted guidance for comma-split patterns.
- Removed grammar tokenization: Reverted the atomic regex token and retained the established ordinary-value parsing rules, resolving the prior path and decorator boundary regressions.
- Extended
regex(): Added an optional static flags argument across runtime resolver and type-option conversion paths, with invalid-regex error handling. - Added split-pattern guidance: Attached a focused tip to existing mixed-argument errors when a
matchespattern appears to have been split by a comma. - Updated flags and documentation: Added
dandvslash-literal flags and documented when patterns must use the explicit wrapper.
azure/gpt-5.6-sol ｜ 𝕏
| summary: '*(deprecated)* Creates a regular expression for use inside other functions.', | ||
| documentation: 'Deprecated — use `/pattern/flags` syntax instead. For example: `remap($VAR, /^dev.*/, result)`.', | ||
| summary: 'Creates a regular expression for use inside other functions.', | ||
| documentation: 'Takes a quoted pattern, with optional flags as a second argument. Prefer `/pattern/flags` when it fits (`remap($VAR, /^dev.*/, result)`), and use this when the pattern contains a `,`, a space, or a `)` - for example a quantifier: `regex("^[0-9a-f]{7,40}$")`.', |
There was a problem hiding this comment.
This advertises regex("pattern", "flags"), but extractRegexPattern() still recognizes only the one-argument wrapper. String, URL, and domain matches diagnostics therefore compile the entire wrapper text as the pattern and can reject values that Varlock accepts.
Technical details
# VS Code diagnostics do not understand flagged regex wrappers
## Affected sites
- `packages/vscode-plugin/src/intellisense-catalog.ts:474` - advertises the new optional flags argument.
- `packages/vscode-plugin/src/diagnostics-core.ts:22` - matches only `regex("pattern")` and falls back to treating a two-argument wrapper as raw pattern text.
- `packages/vscode-plugin/src/diagnostics-core.ts:343` - string diagnostics use the incorrect extracted pattern.
- `packages/vscode-plugin/src/diagnostics-core.ts:457` - URL diagnostics use the same helper.
- `packages/vscode-plugin/src/diagnostics-core.ts:508` - domain diagnostics use the same helper.
## Required outcome
- Parse the optional quoted flags argument in `extractRegexPattern()` so editor validation agrees with runtime validation.
- Add diagnostic coverage for a value accepted only because of the second argument, such as a lowercase value matched by an uppercase pattern with `"i"`.
|
Superseded approach: the first version of this PR lexed The fix instead completes the explicit spelling ( Filed #1089 for a separate problem found while probing this: quoting does not make a regex-shaped value literal, so the documented escape hatch in |
regex() takes the pattern source, so a slash-wrapped argument compiled to a
pattern matching the slashes themselves - silently, and it is the obvious thing
to write when moving a pattern off the unquoted form the new tip points away
from. Flags have their own argument here, so there is no reason to carry them
inside the string either.
Rejected rather than stripped: regex("/usr/lib/") could legitimately be a
pattern for a path, so the error names both fixes instead of guessing. The
quoted-string case is the same bug and needs a breaking change - see #1089.
99bb50b to
baa4a0b
Compare
There was a problem hiding this comment.
Important
The new rejection is internally consistent, but it introduces a breaking configuration change under a minor Varlock release.
Reviewed changes since the prior Pullfrog review: the new commit rejected slash-wrapped pattern sources passed to regex() and documented the replacement syntax.
- Rejected wrapped sources: Added the same focused
SchemaErrorto the general resolver and@typeoption conversion paths. - Documented migration: Explained that callers must remove delimiter slashes or escape them when the slashes are part of the pattern.
- Added regression coverage: Covered rejection of likely pasted literals and acceptance of escaped delimiter slashes.
azure/gpt-5.6-sol ｜ 𝕏
| if (typeof regexStr !== 'string') { | ||
| throw new SchemaError('expects a string'); | ||
| } | ||
| assertUnwrappedRegexSource(regexStr); |
There was a problem hiding this comment.
This rejects previously valid configurations such as regex("/usr/lib/"), which the comment itself identifies as a legitimate pattern source. Since varlock is already 1.19.0, releasing this under the current minor changeset can break existing schemas during a routine upgrade.
Technical details
# Breaking regex source rejection is marked as minor
## Affected sites
- `packages/varlock/src/env-graph/lib/resolver.ts:517` - changes an accepted `regex()` source into a schema error.
- `.bumpy/regex-fn-flags.md:3` - requests only a minor bump for `varlock`.
## Required outcome
- Either preserve existing valid slash-matching sources, or publish the intentional rejection as a major Varlock change with migration guidance.Quoting a regex literal whole - slashes and flags included - already works and
always has: matches="/^[0-9a-f]{7,40}$/i" carries the comma past the value rules
without changing what the value means. That is a one-character fix for the
reported error, so the tip and the docs name it first.
regex() stays the other spelling, for a pattern with enough slashes in it that
escaping each one hurts. The docs also drop two wrong claims: that a quoted
value is not detected as a regex, and that quoting a path-shaped match value in
remap() makes it literal. Neither is true - see #1089.
There was a problem hiding this comment.
Important
The latest commit introduces no additional issue, but the two existing review findings remain unresolved.
Reviewed changes since the prior Pullfrog review: corrected the recommended syntax for transporting regex literals that contain parser delimiters.
- Revised quoted-literal guidance: Documented that quoting the complete
/pattern/flagsvalue preserves regex detection while allowing commas, spaces, and closing parentheses. - Updated split-pattern tip: Pointed the existing mixed-argument diagnostic at the shorter quoted-literal fix while retaining
regex("pattern", "flags")as an alternative. - Added behavioral coverage: Verified comma-containing quoted literals with and without flags against both matching and non-matching values.
azure/gpt-5.6-sol ｜ 𝕏
Escaping the inner slashes and anchoring does it - `"/^\\/usr\\/lib\\/$/"` matches `/usr/lib/` and nothing else. The previous wording said there was no way to do this, which was wrong: it generalized from the bare escaped form (no wrapping slashes), which opts out of regex detection and then compares against the raw escaped string, so it never matches.
There was a problem hiding this comment.
Important
The latest documentation correction introduces no new issue, but the two existing review findings remain unresolved.
Reviewed changes since the prior Pullfrog review: corrected and verified the documented way to match a slash-wrapped value exactly.
- Corrected path guidance: Replaced the incorrect claim that slash-wrapped values cannot be matched literally with an escaped, anchored regex-literal example for
remap(). - Added behavioral coverage: Verified that the documented escaped pattern accepts
/usr/lib/and rejects a value that merely contains that path.
azure/gpt-5.6-sol ｜ 𝕏
- inner slashes in an unquoted literal are not required to be escaped: detection runs to the last slash, and the url tests already rely on that - the tip's example showed a flag the user's pattern did not have - pin the recommended quoted-literal spelling at the parser level too - drop a stale 'deprecated' from a test name; the docs no longer call regex() that
There was a problem hiding this comment.
Important
The latest commit introduces no new issue, but the two existing review findings remain unresolved.
Reviewed changes since the prior Pullfrog review: the documentation accuracy fixes, diagnostic wording refinement, parser regression coverage, and removal of stale deprecation language.
- Clarified slash handling: Documented that inner forward slashes may remain unescaped because regex-literal detection uses the final slash as the delimiter.
- Refined split-pattern guidance: Changed the diagnostic example so optional flags are clearly kept inside the quoted literal.
- Expanded parser coverage: Added an exact parser assertion for a quoted comma-containing literal with flags.
- Removed stale deprecation wording: Renamed the remaining
regex()option test and documentation now that the wrapper is supported rather than deprecated.
azure/gpt-5.6-sol ｜ 𝕏
1 participant
@type=string(matches=/^[0-9a-f]{7,40}$/)fails with "cannot mix positional args and named options". An unquoted/pattern/is an ordinary value, not a token the grammar knows about, so the comma in the quantifier ends the value: the args becomematches=/^[0-9a-f]{7plus a positional40}$/.The fix leaves the parser alone.
/stays an ordinary character, so no path is ever at risk of being read as a regex.The answer for the user turns out to already exist: quote the literal whole, slashes and flags included.
Regex detection runs on the value after the parser strips quotes, so quoting is pure transport -- it gets a
,, a space, or a)past the value rules without changing what the value means. Nothing in the docs said so, and the error didn't either. Now both do.Changes
fn(/usr/local/, /etc/)arrangement can't trip it.regex("pattern", "flags")takes flags as a second argument. It was capped at one arg, so the spelling meant for awkward patterns couldn't carry flags at all./.../literal passed toregex()is now an error. It used to compile to a pattern matching the slashes themselves, silently. Rejected rather than stripped, sinceregex("/usr/lib/")could legitimately be a pattern for a path; the error names both fixes (drop the slashes, or escape them as\/).dandvflags are recognized alongside the rest, in the runtime and in the VSCode diagnostics.remap()makes it literal. They now also show how to match a slash-wrapped value exactly -- escape the inner slashes and anchor -- which is covered by a test.Why not make
/.../a real tokenThat was the first version of this PR, and the grammar carried such a rule until #620 removed it for mis-lexing paths. The problem isn't fixable by a better rule:
/is a path character at both ends of a path, so anything inferring "regex" from/.../shape will sometimes capture paths. Delimiter-based lexing silently readsfn(/foo, bar/)-- a plausible two-path argument list -- as one regex, and narrower variants (allowing commas only inside{}) just make lexing depend on nesting.Not fixed here
#1089, now narrowed to a papercut: a match value escaped without wrapping slashes (
"\/usr\/lib\/") opts out of regex detection and is then compared against the raw escaped string, so it silently never matches. Everything is expressible today; that one shape just looks like it should work and doesn't.