fix(bggrep): accept a leading (?i) flag group and explain the others - #24
Merged
Merged
Conversation
Callers write PCRE habits into `pattern`, and JavaScript's RegExp has no inline
flag syntax at all — flags are a second argument — so `(?i)error` is a syntax
error. That cost a real tool call in practice: a search with
`(?i)\(fail\)|error:|…` came back as `Invalid regular expression: Invalid group`
and had to be worked around by hand. The tool also offered no way to ask for
case-insensitivity, which is why its default pattern hand-writes both `Error:`
and `error:`.
- `splitInlineFlags()` translates a LEADING `(?i)`, `(?m)` or `(?s)` group into
the equivalent JS flags, so `(?i)error` searches case-insensitively.
- Only the leading position is translated. PCRE scopes a mid-pattern `(?i)` to
the REST of the pattern, so stripping one there would silently WIDEN the match
instead of honoring it; those still fail, now with `inlineFlagHint()`
explaining that JS has no inline flags and naming the alternatives.
- Flags are threaded through all three compile sites — the worker, the sync
fallback and the up-front validation — so they cannot disagree. The worker
takes them via `workerData`, and `matchLinesWithBudget`'s trailing
`workerSource` parameter became `opts: { workerSource?, flags? }`.
- Every message and `details.pattern` now echoes the pattern as the caller wrote
it, flags included, while matching uses the translated source.
- While in that function: the two inline `import("node:worker_threads")` types
became a top-level `import type` (erased at compile time, so the runtime
import — dynamic precisely because this fallback exists for runtimes without
worker_threads — stays dynamic, with a comment naming why).
Not attempted: general PCRE compatibility. Constructs like `\K`, atomic groups,
possessive quantifiers and conditionals have no faithful JS translation, so
accepting them would quietly change what a search means.
Verified: `tsc --noEmit` clean; 227 tests pass (224 existing plus case-insensitive
matching, the mid-pattern explanation, and the translation table); and a live pi
session calling bggrep with `(?i)mixed_case_line` returned `L2: MIXED_case_LINE`,
while `line(?i)more` came back with the hint instead of a bare engine message.
README and the run-bg skill document the accepted form.
Contributor
CI report
Ref: |
…x the wording Review of the first commits found two gaps and one inaccuracy. - `inlineFlagHint` returned "" whenever the RAW pattern started with a flag group, so `(?is)a(?m)b` — a leading group we honour plus a SECOND group that is the actual cause — reported a bare engine error with no explanation. That is the only case the hint exists for. It is now keyed on the pattern AFTER the leading group is stripped, which keeps every previous assertion (a leading group we honoured is still not the reason for a later failure) while restoring the hint for `(?is)a(?m)b` and `(?i)(?i)b`. - The sync fallback's `flags` parameter was exercised by no test: only the worker is reachable from the tool in this suite (worker_threads exists on Node and Bun), so a regression dropping the argument at either fallback call site would have shipped silently — the exact bug class this branch fixes. The parity test now asserts both paths honour "i" and that the flagless calls miss, so the assertion tests the flag rather than a pattern that always hits. - `details.pattern` is asserted to echo the caller's flagged pattern. - Wording: Node 24 and Bun 1.3.6 DO support scoped modifier groups (`(?i:…)`, ES2025), which scope exactly as PCRE does — so "JS RegExp has no inline flags" was false. The hint, the tool description, the README row and the skill row now say "no unscoped inline flags" and note that a scoped group is native anywhere. (This also meant updating the two assertions that matched the old phrasing.) Verified: `tsc --noEmit` clean; 227 tests pass, including the three added above.
# Conflicts: # README.md # skills/run-bg/SKILL.md
lloydsk
force-pushed
the
fix/bggrep-inline-flags
branch
from
September 26, 2026 02:23
5c1a77c to
5692bfa
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Agents write PCRE habits into
pattern, and JavaScript'sRegExphas no unscoped inline flag syntax — flags are the second argument — so(?i)erroris a syntax error. That cost a real tool call in practice: a search with(?i)\(fail\)|error:|…came back asInvalid regular expression: Invalid groupand had to be worked around by hand. The tool also offered no way to request case-insensitivity at all, which is why its default pattern hand-writes bothError:anderror:.Change
(?i),(?m)or(?s)group is translated into the equivalent JS flags, so(?i)errorsearches case-insensitively.(?i)to the rest of the pattern, so stripping it there would silently widen the match instead of honoring it. Those still fail — now with an explanation.workerData;matchLinesWithBudget's trailingworkerSourceparameter becameopts: { workerSource?, flags? }.details.patternecho the pattern as the caller wrote it, flags included, while matching uses the translated source.Not attempted
General PCRE compatibility.
\K, atomic groups, possessive quantifiers and conditionals have no faithful JS translation, so accepting them would quietly change what a search means.Follow-up from review (third commit)
Two gaps and one inaccuracy, all found by review:
inlineFlagHintreturned "" whenever the raw pattern started with a flag group — so(?is)a(?m)b(a leading group we honour plus a second group that is the real cause) produced a bare engine error with no explanation. That is the only case the hint exists for. It is now keyed on the pattern after the leading group is stripped, which keeps every earlier assertion while restoring the hint for(?is)a(?m)band(?i)(?i)b— both now pinned by tests.flagsparameter had no test. Only the worker is reachable from the tool in this suite (worker_threadsexists on Node and Bun), so a regression dropping the argument at either fallback call site would have shipped silently — precisely the bug class this branch fixes. The parity test now asserts both paths honouri, and that the flagless calls miss.(?i:…), ES2025), which scope exactly as PCRE does — so "JS RegExp has no inline flags" was false. Every surface now says "no unscoped inline flags" and notes that a scoped group is native anywhere. That change also required updating two assertions that matched the old phrasing.Verification
tsc --noEmitclean.(?i), the mid-pattern explanation, the translation table, the fallback/worker flag parity, anddetails.pattern.Docs
The README
bggreprow and theskills/run-bgGrep row state the accepted form, that a scoped group is native, and that no unscoped inline-flag syntax exists.