Repository navigation
Fix searchMatch key mutation in regex lorebook - #1532
Conversation
cubicj
left a comment
There was a problem hiding this comment.
I reproduced all four defects from #1530 on current main and ran the same cases against this branch, a match only in the last scanned message, the leading slash case, a flagless key, and the second of two keys matching. All four activate here, the (?!)| workaround keys people use today keep matching, and the error cases behave the same as before, non-/ keys, unclosed regexes, and bad flags all still return false. The parse also handles slashes inside the pattern correctly, /a\/b/i and /[/]/g both split at the right place. pnpm check, pnpm test, and pnpm build pass on this head.
One change I would like to ask for. The parse and the new RegExp construction sit inside the message loop, but the keys never change during a scan, so every message re-parses and re-compiles every key, and recursive scanning repeats all of it each round. I would rather see the keys parsed and compiled once before the message loop. Two things to watch if you take this: a reused regex makes .test() stateful under the g and y flags, which your current version avoids by constructing a fresh regex each time, so the compiled regexes need their lastIndex reset or the flags handled some other way. And validating all keys up front means an entry with any malformed key is always inactive, where the current code stops validating later keys once an earlier one matches the first scanned message. I think the stricter behavior is the right one, but it is a small semantic change so I am calling it out.
Two smaller notes, neither blocking. A key of exactly / used to be skipped with the remaining keys still evaluated, and now it fails the parse and deactivates the entry. Nobody realistically has that key, I am noting it only because the PR body covers // but not this case. And in regex mode @@exclude_keys_all acts as ANY rather than ALL, since this branch returns on the first key match before arg.all is read. That is pre-existing rather than something this PR introduces, but regex exclude keys only start working with this fix, so this is where it will start to show.
Does hoisting the compilation look right to you, or is there a reason to prefer the per-iteration construction that I am missing?
|
Thanks for the thorough review :) I’ll move them before the scan as you suggested, and I’ll make sure to reset I’m currently traveling, so I’ll update the branch as soon as I’m back. |
970cd98 to
4fa72fb
Compare
cubicj
left a comment
There was a problem hiding this comment.
I went through 4fa72fb against the two asks from the last round. The parse and the RegExp construction now run once before the message scan, and lastIndex is reset before each .test() call. I compared the reused compiled regexes against fresh per-iteration construction across the constructible flag combinations, including repeated g and y rounds, and found no mismatches. The four defect cases from #1530 and the (?!)| workaround keys give the same results as my pass on 970cd98, and the upfront validation delta is now disclosed in the PR body.
pnpm check, pnpm test, and pnpm build are clean on this head, and the branch sits on current main as a single commit.
Approving — thanks for the careful follow-ups!
sub-hub
left a comment
There was a problem hiding this comment.
Thanks for the PR.
Reviewed the final head: the per-iteration arg.keys[0] mutation is gone, keys are parsed/compiled once with a proper /pattern/flags split, and lastIndex is reset for g/y regexes. I traced the only caller and didn't find any regressions; behavior for non-/ keys and invalid flags is unchanged.
This also matches reports from users: regex lorebook entries only ever matched the oldest message within the search depth (e.g. always message 0), and people were using /(?!)|... workarounds for the leading-slash issue. Those workaround keys keep working after this change.
Noting the intentional behavior change for release notes: a malformed regex key now makes the whole entry inactive (previously, flagless malformed keys were skipped).
Brings in upstream PR kwaroran#1532 (4fa72fb, "Fix searchMatch key mutation in regex lorebook"). Upstream range 2a511a3..979dbc9 has one non-merge commit; 6e0277b and 979dbc9 are upstream merge commits. The merge applied with no conflicts, and the staged change is only the regex branch of searchMatch in src/ts/process/lorebook.svelte.ts. The bug: a flagged regex key such as /dragon/i was rewritten to /dragon with its leading slash kept, so a regex lorebook key effectively never activated. A key with no flags (/dragon/) was skipped. Now each key is parsed as /pattern/flags, flags are limited to dgimsuvy, and a key that cannot be parsed or is not a valid regular expression makes that key list (primary or secondary) not match. lastIndex is reset as a defensive measure. Nothing persisted changes: the keys come from a split(',') on a cloned entry. Test: src/ts/process/tests/lorebookRegexKeys.svelte.test.ts has 10 tests that drive the real loadLoreBookV3Prompt. Five are reproducers that fail on the pre-merge code and five are compatibility guards that pass before and after. One reproducer key was corrected from /dragon/g to /dragon/gi because the data only has "Dragon". Review: an independent adversarial review (Gate 2) returned [APPROVE]. It confirmed the staged diff is only the regex branch, 10/10 tests pass on the merged code, and with the pre-merge source 5 fail and 5 pass. Records: Agents/Upstream-Merges.md gains two sections, a backfill for the 2026-10-09 merge 6f9c0b9 (keep-alive sound) and this merge. Parts of the backfill that its sources do not record are marked "not recorded". Known and not fixed (present in upstream too): searchMatch slices messages from length - scanDepth with no clamp, so a scan depth larger than the message count drops the oldest messages. It affects plain keys as well. Full checks on the merged tree, with the uncommitted Grid edit-mode work also present: pnpm test passes 667 of 668 files (13064 tests, 6 skipped); the 6 failures are all localeParity, from that work's five new English keys awaiting translation, not from this merge. pnpm check: 0 errors. pnpm build: succeeds. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Q6ABYpmiXZmtpyHTvmFkW






PR Checklist
Summary
Fixes the
arg.keys[0]mutation bug in regex lorebook key matching, which caused regex lorebook entries to silently fail on all messages after the first non-matching one.Related Issues
Fixes #1530
Changes
arg.keys[0]during regex key iteration insearchMatch(src/ts/process/lorebook.svelte.ts).patternandflagsvariables using^\/([\s\S]*)\/([dgimsuvy]*)$.{ regex, source }[]array before the message scan instead of being re-parsed and re-compiled for each message;lastIndexis reset before each.test()call forg/yflagged regexes./being retained as part of the pattern;arg.keys[0].Impact
Error behavior is preserved: keys that fail to parse or compile still return false, same as before.Updated: regex keys are validated up front, so any malformed key makes the entry inactive. See the verification comment below for details.
Additional Notes
Manual verification on a local node-hosted build, with a test lorebook covering each reported defect:
/studio/i— also covers the leading-slash defect);/flagless/);All four entries activate correctly on the fixed build (verified via the lore monitor and the actual prompt sent).
Minor edge case: an empty pattern key (
//) previously was silently skipped and now compiles to an empty regex (matches everything). This follows the parsing approach proposed and approved in #1530.Verification screenshots (lore monitor + actual request body)
All four test entries activate — TEST1 (match only in the last scanned message), TEST2 (
/studio/i, the leading-slash case from the issue), TEST3 (flagless/flagless/), TEST4 (second of two comma-separated keys):Footnotes
Modifies the behavior of prompting, requesting, or handling responses from AI models. ↩
Over 80% of the code is AI generated. ↩