fix(core): parseKeyValueXml prefix-tag corruption + extractFirstSentence dead abbrevs + secrets settingUpdated - #11573
Conversation
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
…nce dead abbrevs + secrets settingUpdated (#11572) Three provable, unit-tested correctness bugs (Fable-5 hunt of quiet lanes): 1. parseKeyValueXml (utils.ts findMatchingXmlClose): indexOf(`<${tag}`) matched any tag whose name merely STARTS with the target, so a value containing `<textarea>` while closing `<text>` inflated depth → the close was never found, the field dropped, and `textarea` promoted to a top-level key. Used by live cloud evaluators on model output. Fix: bump depth only on exact tag-name match; skip prefix-extensions. 2. extractFirstSentence (utils/text-splitting.ts): the preceding-word match `\b(\w+)$` excludes `.`, extracting only "g" from "e.g", so the e.g/i.e abbreviation entries were dead and the first-sentence/TTS early-emit path chopped replies at "e.". Fix: capture `[\w.]+` and strip a trailing dot. 3. secrets setup service (features/secrets/setup/service.ts): the settingUpdated response bound `.replace` to the DEFAULT only (custom messages shipped raw {{settingName}}), and updatedKey read session.currentSettingKey AFTER it was reassigned to the next key. Fix: parenthesize like askSetting; return the just-answered key. Each fix has a mutation-checked test (reverting reds it). Also removed 3 stale gitignored .js artifacts under core/src that shadow the .ts (a parseKeyValueXml import without extension resolved to a stale build). Regression sweep: 482 tests across 74 files green.
16b84f5 to
30126d9
Compare
There was a problem hiding this comment.
Your trial has ended. Reactivate Greptile to resume code reviews.
lalalune
left a comment
lalalune
left a comment
There was a problem hiding this comment.
Reviewed and verified after rebasing onto current origin/develop (9a15c0f).
Local evidence on rebased head 30126d9:
git diff --check origin/develop...HEAD✅bun run --cwd packages/core test -- src/__tests__/structured-parser.test.ts src/utils/text-splitting.test.ts src/features/secrets/setup/service.test.ts✅ 3 files / 8 testsbunx @biomejs/biome@2.5.2 check packages/core/src/__tests__/structured-parser.test.ts packages/core/src/features/secrets/setup/service.test.ts packages/core/src/features/secrets/setup/service.ts packages/core/src/utils.ts packages/core/src/utils/text-splitting.test.ts packages/core/src/utils/text-splitting.ts✅bun run --cwd packages/core typecheck✅- broader regression slice:
bun run --cwd packages/core test -- src/__tests__ src/utils src/features/secrets✅ 75 files / 490 tests
The three fixes are narrow and directly covered: exact tag matching in XML close scanning, dotted abbreviation handling for first-sentence extraction, and secrets setup settingUpdated placeholder/updatedKey behavior.
…unctuation ebd5203 (#11573) fixed the dead e.g/i.e entries by matching [\w.]+ but anchored the match with (?:^|\s), which rejects abbreviations preceded by quotes/parens/asterisks ('"Dr' / '(Mr') that the original \b handled — the tts first-sentence early-emit path chopped mid-name ('He cited "Dr.'). drop the anchor: leftmost matching already captures the maximal trailing [\w.] run and any other char or start-of-string delimits it.
|
Claude encountered an error —— View job I'll analyze this and get back to you. |
…unctuation ebd5203 (#11573) fixed the dead e.g/i.e entries by matching [\w.]+ but anchored the match with (?:^|\s), which rejects abbreviations preceded by quotes/parens/asterisks ('"Dr' / '(Mr') that the original \b handled — the tts first-sentence early-emit path chopped mid-name ('He cited "Dr.'). drop the anchor: leftmost matching already captures the maximal trailing [\w.] run and any other char or start-of-string delimits it. (cherry picked from commit 99511f2)
2 participants
Fixes #11572. Three provable
@elizaos/corecorrectness bugs surfaced by a Fable-5 hunt of the quiet (non-money/cloud) lanes — each small, surgical, and mutation-checked.parseKeyValueXmldrops a field + promotes a bogus key —findMatchingXmlCloseusesindexOf(\<${tag}`), which matches any tag *starting with* the name, so<textarea>inside avalue inflates depth → close never found →textdropped,textarea` promoted. Hits live cloud evaluators (room-title, helpers) on model output.extractFirstSentencenever matches its owne.g/i.e—\b(\w+)$excludes., extracting"g"from"e.g"; the abbreviation entries are dead → the first-sentence / TTS early-emit path chops replies at"e."[\w.]+, strip a trailing dot before comparingsettingUpdatedships literal{{settingName}}for custom messages (.replacebound to the default only) and returns the next key asupdatedKey(read after reassignment)askSettingbranch; return the just-answered keyEvidence
structured-parser.test.ts: prefix-extended tag in a value round-trips ({text, thought}, notextareakey).text-splitting.test.ts(new):e.g./i.e.don't split;Mr./Dr.still don't; normal sentences do.secrets/setup/service.test.ts(new): customsettingUpdatedsubstitutes the name;updatedKeyis the answered key.__tests__/,utils/,features/secrets/)..jsbuild artifacts undercore/srcthat shadow the.ts(aparseKeyValueXmlimport without extension was resolving to a stale build — the known core-src-stale-.jstrap; the fix looked broken until I cleared them).N/A rows
<textarea>in a value;"e.g."); no model behavior changed. Bug 3 is UI-message plumbing.Authored via a Fable-5 hunt (found + specified) + mutation-checked implementation. A companion voice-UI batch (listener-leak / re-entrancy / error-handling) is coming separately.