[Formatter Feature] 1) Allow {...} to contain whitespace for readability 2) Allow nested conditionals within conditionals #384
Conversation
DavidGracias
changed the title
WalkthroughRefactors BaseFormatter parsing to support nested variables, comparators, and global modifiers, introduces ParseValue.tools tokens, renames public type aliases, expands RegexBuilder APIs, updates compile and format flows, adds helper parsers, and standardises error handling. Name/description template compilation is parallelised. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant BF as BaseFormatter
participant RB as RegexBuilder
participant PV as ParseValue
participant Helpers as parseVariable/parseModifiedVariable
Caller->>BF: compileTemplatesAsync(nameStr, descStr)
activate BF
BF->>BF: compileTemplate(nameStr)
BF->>BF: compileTemplate(descStr)
note over BF: Parallel via Promise.all
BF-->>Caller: precompiledNameFunction, precompiledDescriptionFunction
deactivate BF
Caller->>BF: format(input)
activate BF
BF->>PV: Inject tools.removeline/tools.newline
BF->>Helpers: parseModifiedVariable(..., FullStringModifiers)
Helpers->>RB: build...RegexPattern() (var/mod/comparator/check/tz)
RB-->>Helpers: Patterns
Helpers->>PV: Resolve nested variables + modifiers
Helpers-->>BF: ResolvedVariable or error
BF-->>Caller: Formatted string (post-processed using tools tokens)
deactivate BF
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
would nested conditionals make something like this possible? |
@hugo9655, yes, but this is currently possible with conditionals, which was added in #381 adding nested conditionals does make it more intuitive however and allow for more options. |
|
ohh i see, cool then. |
DavidGracias
force-pushed
the
davidgracias/update_base_ts-infinite_nesting_and_whitespace_ok
branch
from
bafe799 to
6ac09c2
Compare
| json: string | null; | ||
| jsonf: string | null; | ||
| } & typeof DebugToolReplacementConstants; | ||
| tools: { |
There was a problem hiding this comment.
Add tools to parseValue so that it's not as "hardcoded" and rather a more built in variableTemplate
packages/core/src/formatters/base.ts
Outdated
| const placeHolder = " "; | ||
|
|
||
| // Iterate through all {...} matches | ||
| while ((matches = re.exec(str))) { |
There was a problem hiding this comment.
while ((matches = re.exec(str))) { replaced with manual variable checking (i.e. going through string character by character) and moved below logic to compileTemplateHelper
| }; | ||
| } | ||
|
|
||
| /** |
There was a problem hiding this comment.
The existing logic that we know and love, moved into this helper functions (this was the core parsing logic in the previous while loop)
At this point, we can assume modifiedVariable is valid, has no unexpected whitespace, and has all nested variables resolved!
DavidGracias
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
packages/core/src/formatters/base.ts (3)
1106-1106: Inconsistent token: {tools.newLine} vs {tools.newline}Post-processing replaces
{tools.newline}, but DebugToolReplacementConstants uses{tools.newLine}(capital L). Tokens won’t be replaced, leaking into output.Apply this diff to standardise on lowercase
newline:- {tools.newLine} + {tools.newline}Repeat for all occurrences shown in this block.
Also applies to: 1114-1114, 1121-1121, 1138-1138, 1156-1156
318-327: Only the first{debug.*}placeholder is replaced
String.prototype.replacereplaces a single occurrence. If a template contains multiple{debug.X}tokens, later ones remain.Apply this diff to replace all occurrences safely without relying on
replaceAll:- for (const key in DebugToolReplacementConstants) { - str = str.replace( - `{debug.${key}}`, - DebugToolReplacementConstants[ - key as keyof typeof DebugToolReplacementConstants - ] - ); - } + for (const key in DebugToolReplacementConstants) { + const token = `{debug.${key}}`; + const val = + DebugToolReplacementConstants[key as keyof typeof DebugToolReplacementConstants]; + str = str.split(token).join(val); + }
785-788: Numeric coercion doesn’t remove thousands separators without a space
replace(/,\s/g, '')removes only “comma + space”. Values like “1,234” won’t parse as intended.Apply this diff:
- const [parsedNumericValue, parsedNumericCheck] = [ - Number(stringValue.replace(/,\s/g, '')), - Number(stringCheck.replace(/,\s/g, '')), - ]; + const [parsedNumericValue, parsedNumericCheck] = [ + Number(stringValue.replace(/[,\s]/g, '')), + Number(stringCheck.replace(/[,\s]/g, '')), + ];
🧹 Nitpick comments (1)
packages/core/src/formatters/base.ts (1)
601-602: Debug logging left in hot path
console.logstatements will spam logs and slow parsing. Use the existing logger at debug level, or remove.Apply this diff to remove logs:
- console.log(`resolvedVariables: ${resolvedVariables}`); ... - console.log(`comparing ${prev.result} ${compareKey} ${cur.result}`);- console.log(`parsing variable: `); - console.log(`parsed variable ${variableType}.${propertyName}, with value '${property?.toString()}' and type '${typeof property}'`); ... - console.log(`applying modifier: ${lastModMatched} to result: ${result}`); ... - console.log(`result is undefined from modifier: ${lastModMatched}`);Or replace with:
logger.debug(/* same messages */);Also applies to: 611-612, 702-704, 708-716
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/formatters/base.ts(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T16:46:18.507Z
Learnt from: DavidGracias
PR: Viren070/AIOStreams#375
File: packages/core/src/formatters/base.ts:0-0
Timestamp: 2025-09-14T16:46:18.507Z
Learning: In packages/core/src/formatters/base.ts, the modifier definitions intentionally include regex patterns (like "join('.*?')", "$.*?", "^.*?") that require pattern matching functionality. The buildModifierRegexPattern() method should not fully escape all regex metacharacters as this would break the intended regex matching behavior within modifier definitions.
Applied to files:
packages/core/src/formatters/base.ts
🧬 Code graph analysis (1)
packages/core/src/formatters/base.ts (1)
packages/core/src/db/schemas.ts (1)
UserData(466-466)
🪛 ast-grep (0.39.5)
packages/core/src/formatters/base.ts
[warning] 421-421: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildComparatorRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 426-426: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildModifierRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 680-683: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(${this.regexBuilder.buildModifierRegexPattern()})${singleModTerminator},
'g'
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 824-824: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${findStartChar}\\s*,\\s*${findEndChar})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (3)
packages/core/src/formatters/base.ts (3)
300-304: Nice: tool tokens exposed via ParseValueThe explicit
toolsentries make templates more readable and allow post-processing without special-casing elsewhere.
923-929: Modifier regex builder: aligns with “don’t over-escape” learningYou correctly escape only the delimiters while keeping
.*?parts functional for hardcoded patterns (e.g., join/replace). This preserves intended pattern matching.Please confirm test coverage for:
- replace() with both single and double quotes
- join() with both single and double quotes
- prefix conditionals using >=, >, <=, <, =, $, ^, ~ with numeric and string comparisons
Also applies to: 1059-1074
421-423: Dynamic RegExp — base builders use internal constants; verify env-sourced patterns
- buildComparatorRegexPattern / buildModifierRegexPattern in packages/core/src/formatters/base.ts are built from internal constant maps (ComparatorConstants / ModifierConstants) — the new RegExp(...) calls that use them are fed only by those constants (low risk).
- Found env/user-controlled constructions: packages/core/src/utils/env.ts (new RegExp(x / x.pattern) — env parsing compiles/validates patterns) and packages/core/src/db/users.ts (Env.TRUSTED_UUIDS split and compiled via new RegExp(u)). Ensure these env values are intentionally trusted, sanitised or allow‑listed.
- Also review packages/core/src/parser/streams.ts (regex built from service.knownNames) to confirm knownNames are internal.
packages/core/src/formatters/base.ts
Outdated
| let i = str.indexOf('{'); | ||
| while (++i < str.length) { | ||
| if (!['{', '}'].includes(str[i])) continue; |
There was a problem hiding this comment.
Off-by-one skips the first “{” → variables at the start are never parsed
The pre-increment loop starts scanning one character after the first “{”, so the opening brace is missed and the first variable template is skipped. This breaks parsing for leading variables.
Apply this diff to process the first brace correctly and fast-path when no variables exist:
- let i = str.indexOf('{');
- while (++i < str.length) {
+ let i = str.indexOf('{');
+ if (i === -1) {
+ // No variables – return as-is
+ return (_: ParseValue) => str;
+ }
+ i -= 1; // ensure the first '{' is processed by the pre-increment loop
+ while (++i < str.length) {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let i = str.indexOf('{'); | |
| while (++i < str.length) { | |
| if (!['{', '}'].includes(str[i])) continue; | |
| let i = str.indexOf('{'); | |
| if (i === -1) { | |
| // No variables – return as-is | |
| return (_: ParseValue) => str; | |
| } | |
| i -= 1; // ensure the first '{' is processed by the pre-increment loop | |
| while (++i < str.length) { | |
| if (!['{', '}'].includes(str[i])) continue; |
🤖 Prompt for AI Agents
In packages/core/src/formatters/base.ts around lines 357–359, the loop uses a
pre-increment which advances past the first '{' and therefore skips any variable
template at the start of the string; change the scanning logic to process the
found brace rather than starting one character after it and add a fast-path when
there are no braces: find the first '{' with str.indexOf('{'), if it returns -1
return/skip parsing, otherwise start the loop at that index (or increment only
when advancing) so the initial '{' is examined, and keep the existing checks for
'{' and '}' while advancing the index explicitly when characters are skipped.
| if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${propertyName})}` }); // should never happen | ||
| // end of PARSE VARIABLE logic |
There was a problem hiding this comment.
Error message may read “unknown_propertyName(stream.undefined)”
When propertyName resolution fails, interpolating propertyName yields “undefined”. Prefer echoing the raw token for clarity.
Apply this diff:
- if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${propertyName})}` }); // should never happen
+ if (!propertyName) {
+ const rawProp = baseString.split(new RegExp(`[^a-zA-Z]`))[0];
+ return () => ({ error: `{unknown_propertyName(${variableType}.${rawProp})}` });
+ }Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In packages/core/src/formatters/base.ts around lines 674-675, the error path
currently interpolates propertyName which can be undefined producing messages
like "unknown_propertyName(stream.undefined)"; change the returned error closure
to include the original raw token text used to resolve the property (e.g.,
propertyToken.text or the parser token variable that holds the literal) instead
of propertyName, or if that token variable is not available, convert
propertyName to a safe string fallback (e.g., String(propertyName ??
'<unknown_token>')) so the error message never contains "undefined" and clearly
echoes the raw token.
| const checkTrue = `(?<mod_check_true>.*)`; | ||
| const checkFalse = `(?<mod_check_false>.*)`; | ||
| return `\\[(?<mod_check>"${checkTrue}${this.checkTFSplit}${checkFalse}")\\]`; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Greedy .* in check pattern; contradicts comment and risks over‑matching
Comment says “Use [^"]*”, but code uses .*. With nested content, this can over‑capture.
Apply this diff:
- const checkTrue = `(?<mod_check_true>.*)`;
- const checkFalse = `(?<mod_check_false>.*)`;
+ const checkTrue = `(?<mod_check_true>[^"]*)`;
+ const checkFalse = `(?<mod_check_false>[^"]*)`;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const checkTrue = `(?<mod_check_true>.*)`; | |
| const checkFalse = `(?<mod_check_false>.*)`; | |
| return `\\[(?<mod_check>"${checkTrue}${this.checkTFSplit}${checkFalse}")\\]`; | |
| } | |
| const checkTrue = `(?<mod_check_true>[^"]*)`; | |
| const checkFalse = `(?<mod_check_false>[^"]*)`; | |
| return `\\[(?<mod_check>"${checkTrue}${this.checkTFSplit}${checkFalse}")\\]`; | |
| } |
🤖 Prompt for AI Agents
packages/core/src/formatters/base.ts around lines 957 to 960: the check pattern
uses greedy `.*` for mod_check_true and mod_check_false which contradicts the
comment and can over-match; change both to `[^"]*` so they only match up to the
next quote (i.e., replace `(?<mod_check_true>.*)` and `(?<mod_check_false>.*)`
with `(?<mod_check_true>[^"]*)` and `(?<mod_check_false>[^"]*)`) and keep the
surrounding template/escaping the same.
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/formatters/base.ts (2)
317-339: Replace all occurrences of debug tokens, not just the first one per keyString.prototype.replace without a global regex only replaces the first match. Multiple {debug.key} tokens won’t all be expanded.
Apply this diff to ensure all occurrences are replaced:
- for (const key in DebugToolReplacementConstants) { - str = str.replace( - `{debug.${key}}`, - DebugToolReplacementConstants[ - key as keyof typeof DebugToolReplacementConstants - ] - ); - } + for (const key in DebugToolReplacementConstants) { + const val = + DebugToolReplacementConstants[ + key as keyof typeof DebugToolReplacementConstants + ]; + // Replace ALL occurrences for this debug key + str = str.split(`{debug.${key}}`).join(val); + }
785-788: Numeric comparator parsing misses plain “1,234” (expects a space after the comma)The pattern /,\s/ only removes a comma followed by a space. Use a broader replacement to strip commas and whitespace for numeric comparisons.
Apply this diff:
- const [parsedNumericValue, parsedNumericCheck] = [ - Number(stringValue.replace(/,\s/g, '')), - Number(stringCheck.replace(/,\s/g, '')), - ]; + const [parsedNumericValue, parsedNumericCheck] = [ + Number(stringValue.replace(/[,\s]/g, '')), + Number(stringCheck.replace(/[,\s]/g, '')), + ];
🧹 Nitpick comments (1)
packages/core/src/formatters/base.ts (1)
601-601: Remove console.log noise; use the existing logger or guard behind a debug flagDirect console.log in hot paths will spam logs in production. Prefer logger.debug or strip them before release.
Apply this diff to remove them:
- console.log(`resolvedVariables: ${resolvedVariables}`); + // logger.debug(`resolvedVariables: ${JSON.stringify(resolvedVariables)}`);- console.log(`comparing ${prev.result} ${compareKey} ${cur.result}`); + // logger.debug(`comparing ${prev.result} ${compareKey} ${cur.result}`);- console.log(`parsing variable: `); - console.log(`parsed variable ${variableType}.${propertyName}, with value '${property?.toString()}' and type '${typeof property}'`); + // logger.debug(`parsed variable ${variableType}.${propertyName}, type '${typeof property}'`);- console.log(`applying modifier: ${lastModMatched} to result: ${result}`); + // logger.debug(`applying modifier: ${lastModMatched}`);- console.log(`result is undefined from modifier: ${lastModMatched}`); + // logger.debug(`unknown modifier produced undefined: ${lastModMatched}`);Also applies to: 610-610, 702-703, 709-709, 715-715
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/formatters/base.ts(13 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T16:46:18.507Z
Learnt from: DavidGracias
PR: Viren070/AIOStreams#375
File: packages/core/src/formatters/base.ts:0-0
Timestamp: 2025-09-14T16:46:18.507Z
Learning: In packages/core/src/formatters/base.ts, the modifier definitions intentionally include regex patterns (like "join('.*?')", "$.*?", "^.*?") that require pattern matching functionality. The buildModifierRegexPattern() method should not fully escape all regex metacharacters as this would break the intended regex matching behavior within modifier definitions.
Applied to files:
packages/core/src/formatters/base.ts
🧬 Code graph analysis (1)
packages/core/src/formatters/base.ts (1)
packages/core/src/db/schemas.ts (1)
UserData(466-466)
🪛 ast-grep (0.39.5)
packages/core/src/formatters/base.ts
[warning] 421-421: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildComparatorRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 426-426: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildModifierRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 680-683: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(${this.regexBuilder.buildModifierRegexPattern()})${singleModTerminator},
'g'
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 824-824: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${findStartChar}\\s*,\\s*${findEndChar})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (7)
packages/core/src/formatters/base.ts (7)
357-359: Good fix: no longer skipping the first “{”Initial index set to -1 with pre-increment correctly processes a leading variable. This resolves the off-by-one noted earlier.
923-929: Regex builder preserves intended metacharacters inside hardcoded modifier patternsOnly selective characters are escaped, keeping . and ? intact for patterns like join('.*?') and replace(...) as required by prior guidance.
Please confirm that no additional dynamic modifier patterns (beyond hardcoded ones) are expected to contribute metacharacters; if so, we should document/guard them similarly.
300-304: Nice: explicit tools tokens in ParseValue for post‑processingThis cleanly decouples formatting tokens from the parser and enables the newline/removeline pass.
559-567: Comparator and nested‑conditional scaffolding looks soundSplitting on comparator wrapper and delegating each side to parseModifiedVariable aligns with the PR goal for nesting.
Please add tests for the nested example in the PR description (and the “stream.encode/visualTags” variant) to lock behaviour across regressions.
817-830: Quote‑agnostic parsing for replace()/join() is brittle; use a single regex with back‑referencesCurrent substring/split logic assumes single quotes and builds a regex from variable characters. This breaks for double‑quoted forms and is harder to reason about.
Apply these diffs:
- case mod.startsWith('replace(') && mod.endsWith(')'): { - const findStartChar = mod.charAt(`replace(`.length + 1); // either " or ' - const findEndChar = mod.charAt(mod.length - `)`.length - 1); // either " or ' - - // Extract the separator from replace(['"]...<matching'">, ['"]...<matching'">) - const content = _mod.substring(`replace('`.length, _mod.length - `')`.length); - - // split on findStartChar<whitespace?>,<whitespace?>findEndChar - const [key, replaceKey, shouldBeUndefined] = content.split( - new RegExp(`${findStartChar}\\s*,\\s*${findEndChar}`) - ); - - if (!shouldBeUndefined && key && replaceKey) - return variable.replaceAll(key, replaceKey); - } + case mod.startsWith('replace(') && mod.endsWith(')'): { + const m = _mod.match(/^replace\((['"])(.*?)\1\s*,\s*(['"])(.*?)\3\)$/); + if (m) { + const key = m[2]; + const replaceKey = m[4]; + return variable.replaceAll(key, replaceKey); + } + break; + }- case mod.startsWith('join(') && mod.endsWith(')'): { - // Extract the separator from join('separator') or join("separator") - const separator = _mod.substring(`join('`.length, _mod.length - `')`.length); - return variable.join(separator); - } + case mod.startsWith('join(') && mod.endsWith(')'): { + const m = _mod.match(/^join\((['"])(.*?)\1\)$/); + if (m) { + const separator = m[2]; + return variable.join(separator); + } + break; + }Also applies to: 844-847
669-675: Error message may print “undefined” for unknown property; echo raw token insteadWhen propertyName resolution fails, interpolating propertyName yields “undefined”. Use the raw token for clarity.
Apply this diff:
- const propertyName = find(propertyNameKeys, baseString.split(new RegExp(`[^a-zA-Z]`))[0].toLowerCase()); - if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${propertyName})}` }); // should never happen + const rawProp = baseString.split(new RegExp(`[^a-zA-Z]`))[0]; + const propertyName = find(propertyNameKeys, rawProp.toLowerCase()); + if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${rawProp})}` }); // should never happen
381-387: Greedy capture for mod_check can over‑match across quotes; constrain to non‑quote charsUsing (.?)/. can span past the intended quote, especially with nested content. Use [^"]* for robustness in both the runtime extractor and the regex builder.
Apply these diffs:
- const checkTF = potentialVariableTemplate.match( - /\[\s*"(?<mod_check_true>.*?)"\s*\|\s*\|\s*"(?<mod_check_false>.*?)"\s*\]\s*\}$/ - ); + const checkTF = potentialVariableTemplate.match( + /\[\s*"(?<mod_check_true>[^"]*)"\s*\|\s*\|\s*"(?<mod_check_false>[^"]*)"\s*\]\s*\}$/ + );- const checkTrue = `(?<mod_check_true>.*)`; - const checkFalse = `(?<mod_check_false>.*)`; + const checkTrue = `(?<mod_check_true>[^"]*)`; + const checkFalse = `(?<mod_check_false>[^"]*)`;Also applies to: 955-960
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/formatters/base.ts (1)
321-343: Don’t drop intentional blank lines; replace all debug tokens; handle both newline tokens
- str.replace only replaces the first occurrence.
- Filtering out empty lines removes intentional spacing.
- Replace both {tools.newline} and {tools.newLine}.
Apply this diff:
- for (const key in DebugToolReplacementConstants) { - str = str.replace( - `{debug.${key}}`, - DebugToolReplacementConstants[ - key as keyof typeof DebugToolReplacementConstants - ] - ); - } + for (const key in DebugToolReplacementConstants) { + str = str.replaceAll( + `{debug.${key}}`, + DebugToolReplacementConstants[ + key as keyof typeof DebugToolReplacementConstants + ] + ); + } @@ - return (parseValue: ParseValue) => - process(parseValue) - .replace(/\\n/g, '\n') - .split('\n') - .filter( - (line) => line.trim() !== '' && !line.includes('{tools.removeline}') - ) - .join('\n') - .replace(/\{tools.newline\}/g, '\n'); + return (parseValue: ParseValue) => { + const out = process(parseValue) + .replace(/\\n/g, '\n') + .replace(/\{tools\.newline\}/gi, '\n'); + return out + .split('\n') + .filter((line) => !line.includes('{tools.removeline}')) + .join('\n'); + };
🧹 Nitpick comments (6)
packages/core/src/formatters/base.ts (6)
304-308: Token mismatch: {tools.newLine} vs {tools.newline}DebugToolReplacementConstants uses {tools.newLine} (capital L) but post-processing only replaces {tools.newline}. Normalise to one spelling or handle both.
Apply either fix:
Option A (normalise constants; outside this hunk):
TypeScript (outside selected lines)// In DebugToolReplacementConstants: replace all occurrences of {tools.newLine} -> {tools.newline}Option B (handle both tokens here):
- .replace(/\{tools.newline\}/g, '\n'); + .replace(/\{tools\.newline\}/gi, '\n');
425-440: Dynamic split regexes: acceptable but keep an eye on performanceBoth comparator/modifier splits build regexes from whitelisted patterns. Low risk, but worth guarding if patterns grow.
Consider precompiling these regexes once on the builder and reusing them to avoid rebuilding per variable.
595-631: Replace console.log with logger.debug (or remove) to avoid noisy outputThese logs will spam in production and leak internal states.
Apply this diff:
- console.log(`resolvedVariables: ${resolvedVariables}`); + logger.debug(`resolvedVariables: ${JSON.stringify(resolvedVariables)}`); @@ - console.log(`comparing ${prev.result} ${compareKey} ${cur.result}`); + logger.debug(`comparing ${prev.result} ${compareKey} ${cur.result}`);
668-679: Fix “unknown_propertyName(stream.undefined)” error textWhen resolution fails, echo the raw token, not “undefined”.
Apply this diff:
- const propertyName = find(propertyNameKeys, baseString.split(new RegExp(`[^a-zA-Z]`))[0].toLowerCase()); - if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${propertyName})}` }); // should never happen + const rawPropToken = baseString.split(new RegExp(`[^a-zA-Z]`))[0]; + const propertyName = find(propertyNameKeys, rawPropToken.toLowerCase()); + if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${rawPropToken})}` }); // should never happen
706-713: Replace console.log with logger.debug (or remove)Keep library quiet unless debug is enabled.
Apply this diff:
- console.log(`parsing variable: `); - console.log(`parsed variable ${variableType}.${propertyName}, with value '${property?.toString()}' and type '${typeof property}'`); + logger.debug(`parsed ${variableType}.${propertyName} -> '${property?.toString()}' (${typeof property})`); @@ - console.log(`result is undefined from modifier: ${lastModMatched}`); + logger.debug(`modifier produced undefined: ${lastModMatched}`);Also applies to: 719-729
1103-1168: Debug demo uses {tools.newLine}; align with runtime tokenTo ensure the demo renders new lines, switch to {tools.newline} everywhere in DebugToolReplacementConstants.
TypeScript (outside selected lines)
// Replace all {tools.newLine} -> {tools.newline} in DebugToolReplacementConstants
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/formatters/base.ts(14 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T16:46:18.507Z
Learnt from: DavidGracias
PR: Viren070/AIOStreams#375
File: packages/core/src/formatters/base.ts:0-0
Timestamp: 2025-09-14T16:46:18.507Z
Learning: In packages/core/src/formatters/base.ts, the modifier definitions intentionally include regex patterns (like "join('.*?')", "$.*?", "^.*?") that require pattern matching functionality. The buildModifierRegexPattern() method should not fully escape all regex metacharacters as this would break the intended regex matching behavior within modifier definitions.
Applied to files:
packages/core/src/formatters/base.ts
🧬 Code graph analysis (1)
packages/core/src/formatters/base.ts (1)
packages/core/src/db/schemas.ts (1)
UserData(466-466)
🪛 ast-grep (0.39.5)
packages/core/src/formatters/base.ts
[warning] 425-425: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildComparatorRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 430-430: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildModifierRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 684-687: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(${this.regexBuilder.buildModifierRegexPattern()})${singleModTerminator},
'g'
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 828-828: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${findStartChar}\\s*,\\s*${findEndChar})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (14)
packages/core/src/formatters/base.ts (14)
107-110: Good addition: built-in tooling placeholdersAdding tools.removeline/tools.newline to ParseValue is a clean way to support post-processing.
116-116: Type alias rename is clearParseValueToString naming reads better than the old CompiledParseFunction.
118-122: Return type for variable resolution reads wellParseValueToVariable clarifies intent.
129-131: Precompiled function fields updated correctlyTypes now align with new aliases.
147-155: Parallel template compilation LGTMPromise.all for name/description is appropriate.
356-365: Manual scan start index is correctStarting at -1 then pre-increment avoids the “skip-first-{” bug.
633-654: check[] result coercion path is soundBoolean-only enforcement with an explicit error is correct.
684-698: Modifier extraction looks good; regex is appropriately constrainedUsing a terminator lookahead avoids over-capturing when multiple modifiers exist.
927-932: Regex escaping honours pattern-based modifiers (good)You’re not over-escaping .+? which preserves hardcoded pattern keys like join('.*?') and >=.+?.
985-991: FullStringModifiers type is clear and future-proofShaping mod_check and mod_tzlocale here keeps parsing clean.
1064-1078: Good coverage for pattern-based modifier regexesAll replace() quote combinations are accounted for; aligns with not over-escaping.
847-851: join(...) only supports single quotes; breaks on double quotesUse a quote‑agnostic regex.
Apply this diff:
- case mod.startsWith('join(') && mod.endsWith(')'): { - // Extract the separator from join('separator') or join("separator") - const separator = _mod.substring(`join('`.length, _mod.length - `')`.length); - return variable.join(separator); - } + case mod.startsWith('join(') && mod.endsWith(')'): { + const m = _mod.match(/^join\((['"])(.*?)\1\)$/); + if (m) { + const separator = m[2]; + return variable.join(separator); + } + break; + }
961-964: Greedy capture in check[] risks over-matching; use [^"]*Greedy .* can swallow nested quotes; align with the inline parser.
Apply this diff:
- const checkTrue = `(?<mod_check_true>.*)`; - const checkFalse = `(?<mod_check_false>.*)`; + const checkTrue = `(?<mod_check_true>[^"]*)`; + const checkFalse = `(?<mod_check_false>[^"]*)`;
820-834: replace(...) only supports single quotes; breaks on double quotesThis is a correctness bug. Use a quote‑agnostic regex.
Apply this diff:
- case mod.startsWith('replace(') && mod.endsWith(')'): { - const findStartChar = mod.charAt(`replace(`.length + 1); // either " or ' - const findEndChar = mod.charAt(mod.length - `)`.length - 1); // either " or ' - - // Extract the separator from replace(['"]...<matching'">, ['"]...<matching'">) - const content = _mod.substring(`replace('`.length, _mod.length - `')`.length); - - // split on findStartChar<whitespace?>,<whitespace?>findEndChar - const [key, replaceKey, shouldBeUndefined] = content.split( - new RegExp(`${findStartChar}\\s*,\\s*${findEndChar}`) - ); - - if (!shouldBeUndefined && key && replaceKey) - return variable.replaceAll(key, replaceKey); - } + case mod.startsWith('replace(') && mod.endsWith(')'): { + const m = _mod.match(/^replace\((['"])(.*?)\1\s*,\s*(['"])(.*?)\3\)$/); + if (m) { + const key = m[2]; + const replaceKey = m[4]; + return variable.replaceAll(key, replaceKey); + } + break; + }
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
packages/core/src/utils/env.ts (1)
424-439: Revert the sensitive logging default toggle.Defaulting
LOG_SENSITIVE_INFOtotruemakes every deployment start emitting secrets/PII unless an operator explicitly overrides the env var. That’s a regression from the previous safe default and creates an avoidable data-leak risk.packages/core/src/formatters/base.ts (1)
816-847:replace()/join()still reject the double-quoted forms.Both helpers substr on
"replace('"/"join('", so inputs like::replace("foo","bar")or::join(" • ")fail—yet those syntaxes are documented and used. We can parse both quoting styles (and mixed whitespace) with a single case-insensitive regex.case mod.startsWith('replace(') && mod.endsWith(')'): { - const findStartChar = mod.charAt(`replace(`.length + 1); // either " or ' - const findEndChar = mod.charAt(mod.length - `)`.length - 1); // either " or ' - - // Extract the separator from replace(['"]...<matching'">, ['"]...<matching'">) - const content = _mod.substring(`replace('`.length, _mod.length - `')`.length); - - // split on findStartChar<whitespace?>,<whitespace?>findEndChar - const [key, replaceKey, shouldBeUndefined] = content.split( - new RegExp(`${findStartChar}\\s*,\\s*${findEndChar}`) - ); - - if (!shouldBeUndefined && key && replaceKey) - return variable.replaceAll(key, replaceKey); + const match = _mod.match( + /^replace\((['"])(.*?)\1\s*,\s*(['"])(.*?)\3\)$/i + ); + if (match && match[2] && match[4] !== undefined) { + return variable.replaceAll(match[2], match[4]); + } + break; } case mod.startsWith('join(') && mod.endsWith(')'): { - // Extract the separator from join('separator') or join("separator") - const separator = _mod.substring(`join('`.length, _mod.length - `')`.length); - return variable.join(separator); + const match = _mod.match(/^join\((['"])(.*?)\1\)$/i); + if (match) return variable.join(match[2]); + break; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
packages/core/src/formatters/base.ts(15 hunks)packages/core/src/utils/env.ts(2 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T16:46:18.507Z
Learnt from: DavidGracias
PR: Viren070/AIOStreams#375
File: packages/core/src/formatters/base.ts:0-0
Timestamp: 2025-09-14T16:46:18.507Z
Learning: In packages/core/src/formatters/base.ts, the modifier definitions intentionally include regex patterns (like "join('.*?')", "$.*?", "^.*?") that require pattern matching functionality. The buildModifierRegexPattern() method should not fully escape all regex metacharacters as this would break the intended regex matching behavior within modifier definitions.
Applied to files:
packages/core/src/formatters/base.ts
🧬 Code graph analysis (1)
packages/core/src/formatters/base.ts (1)
packages/core/src/db/schemas.ts (1)
UserData(466-466)
🪛 ast-grep (0.39.5)
packages/core/src/formatters/base.ts
[warning] 425-425: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildComparatorRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 430-430: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildModifierRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 685-688: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(${this.regexBuilder.buildModifierRegexPattern()})${singleModTerminator},
'g'
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 824-824: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${findStartChar}\\s*,\\s*${findEndChar})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: test
| const checkTF = potentialVariableTemplate.match( | ||
| /\[\s*"(?<mod_check_true>.*?)"\s*\|\s*\|\s*"(?<mod_check_false>.*?)"\s*\]\s*\}$/ | ||
| ); | ||
| const modCheck = checkTF?.groups && { | ||
| true: checkTF?.groups?.mod_check_true, | ||
| false: checkTF?.groups?.mod_check_false, | ||
| }; | ||
| const relativeTrueIndex = | ||
| checkTF?.[0].indexOf(`"${modCheck?.true}"`)! + `"`.length; | ||
| const relativeFalseIndex = | ||
| checkTF?.[0].lastIndexOf(`"${modCheck?.false}"`)! + `"`.length; | ||
| const modCheckWithOffsets = modCheck && { | ||
| ...modCheck, | ||
| offsetFromVariable: relativeTrueIndex, | ||
| offsetFromTrueCase: | ||
| relativeFalseIndex - (relativeTrueIndex + modCheck.true.length), | ||
| }; // end of CHECK TRUE/FALSE |
There was a problem hiding this comment.
Nested checks break on embedded conditionals or quotes.
The new mod_check matcher still uses .*? for the true/false branches, so the first " inside the branch terminates the capture. Real examples like {...["📹 {stream.encode}{stream.visualTags::exists[" • ..."]}"||"..."]} now resolve mod_check.true to an empty string, making nested conditionals impossible—the very feature this PR is meant to add. Please switch to a deterministic parser (walk the substring, count brackets/quotes, split on || only at depth 0) instead of this fragile regex.
🤖 Prompt for AI Agents
packages/core/src/formatters/base.ts around lines 385 to 401: the current
regex-based extraction for mod_check (true/false branches) fails on nested
quotes/brackets because it uses non-deterministic ".*?" captures; replace this
with a deterministic parser that scans the relevant substring char-by-char,
tracking quote state and bracket depth, and only treats the top-level '||'
separator as the split point (i.e., ignore '||' inside quotes or nested
brackets). Implement: locate the opening '[' for the check, iterate forward
maintaining a stack/depth for [] and a boolean for being inside
single/double/backticks (respecting escapes), record the split index when
depth==0 and not inside a quote and the next two chars are "||", then extract
the true/false branch substrings using the exact indices found, and compute
relativeTrueIndex/relativeFalseIndex from those indices so offsetFromVariable
and offsetFromTrueCase remain correct.
DavidGracias
force-pushed
the
davidgracias/update_base_ts-infinite_nesting_and_whitespace_ok
branch
from
9f3a578 to
80f45a7
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
packages/core/src/formatters/base.ts (1)
321-343: Make tools token matching case-insensitive and normalise example literals
- In packages/core/src/formatters/base.ts (compileTemplate, lines 321–343) change:
to.filter(line => line.trim() !== '' && !line.includes('{tools.removeline}')) … .replace(/\{tools\.newline\}/g, '\n').filter(line => line.trim() !== '' && !/\{tools\.removeline\}/i.test(line)) … .replace(/\{tools\.newline\}/gi, '\n')- Update all
{tools.newLine}literals in the same file (e.g. around lines 1111, 1119, 1126, 1143, 1161) to{tools.newline}.
🧹 Nitpick comments (2)
packages/core/src/formatters/base.ts (2)
353-354: Avoid placeholder collision with user contentUsing a single-character placeholder ('X') risks accidental removal when the same character occurs at replacement positions. Use a unique sentinel.
Apply this diff:
- const PLACEHOLDER = 'X'; // any string of length > 0 + // Use an improbable sentinel to avoid accidental collisions with user content + const PLACEHOLDER = '§§VAR§§';
674-685: Improve unknown variable/property error messagesInterpolate raw tokens to avoid “undefined” in errors and to aid debugging.
Apply this diff:
- const variableTypeKeys = Object.keys(this.regexBuilder.hardcodedParseValueKeysForRegexMatching); - const variableType = find(variableTypeKeys, baseString.split('.')[0].toLowerCase()); - if (!variableType) return () => ({ error: `{unknown_variableType(${variableType})}` }); // should never happen + const variableTypeKeys = Object.keys(this.regexBuilder.hardcodedParseValueKeysForRegexMatching); + const rawVariableType = baseString.split('.')[0]; + const variableType = find(variableTypeKeys, rawVariableType.toLowerCase()); + if (!variableType) return () => ({ error: `{unknown_variableType(${rawVariableType})}` }); // should never happen baseString = baseString.substring(variableType.length + `.`.length); const propertyNameKeys = Object.keys(this.regexBuilder.hardcodedParseValueKeysForRegexMatching[variableType as keyof ParseValue]!); - const propertyName = find(propertyNameKeys, baseString.split(new RegExp(`[^a-zA-Z]`))[0].toLowerCase()); - if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${propertyName})}` }); // should never happen + const rawProperty = baseString.split(new RegExp(`[^a-zA-Z]`))[0]; + const propertyName = find(propertyNameKeys, rawProperty.toLowerCase()); + if (!propertyName) return () => ({ error: `{unknown_propertyName(${variableType}.${rawProperty})}` }); // should never happen
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
packages/core/src/formatters/base.ts(15 hunks)
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-09-14T16:46:18.507Z
Learnt from: DavidGracias
PR: Viren070/AIOStreams#375
File: packages/core/src/formatters/base.ts:0-0
Timestamp: 2025-09-14T16:46:18.507Z
Learning: In packages/core/src/formatters/base.ts, the modifier definitions intentionally include regex patterns (like "join('.*?')", "$.*?", "^.*?") that require pattern matching functionality. The buildModifierRegexPattern() method should not fully escape all regex metacharacters as this would break the intended regex matching behavior within modifier definitions.
Applied to files:
packages/core/src/formatters/base.ts
🧬 Code graph analysis (1)
packages/core/src/formatters/base.ts (1)
packages/core/src/db/schemas.ts (1)
UserData(466-466)
🪛 ast-grep (0.39.5)
packages/core/src/formatters/base.ts
[warning] 425-425: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildComparatorRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 430-430: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp((${this.regexBuilder.buildModifierRegexPattern()}))
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 690-693: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(
(${this.regexBuilder.buildModifierRegexPattern()})${singleModTerminator},
'g'
)
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
[warning] 829-829: Regular expression constructed from variable input detected. This can lead to Regular Expression Denial of Service (ReDoS) attacks if the variable contains malicious patterns. Use libraries like 'recheck' to validate regex safety or use static patterns.
Context: new RegExp(${findStartChar}\\s*,\\s*${findEndChar})
Note: [CWE-1333] Inefficient Regular Expression Complexity [REFERENCES]
- https://owasp.org/www-community/attacks/Regular_expression_Denial_of_Service_-_ReDoS
- https://cwe.mitre.org/data/definitions/1333.html
(regexp-from-variable)
🔇 Additional comments (6)
packages/core/src/formatters/base.ts (6)
146-155: LGTM: compile name/description in parallelGood perf win and clean separation of concerns.
361-369: LGTM: off‑by‑one scanning fixedStarting at -1 and pre‑increment ensures the first '{' is processed. This resolves the prior skip of leading variables.
821-835: replace(...) parsing is quote‑fragile and constructs regex from variable inputUse a single, safe regex supporting both quote styles. This also removes ReDoS risk flagged by static analysis.
Apply this diff:
- case mod.startsWith('replace(') && mod.endsWith(')'): { - const findStartChar = mod.charAt(`replace(`.length + 1); // either " or ' - const findEndChar = mod.charAt(mod.length - `)`.length - 1); // either " or ' - - // Extract the separator from replace(['"]...<matching'">, ['"]...<matching'">) - const content = _mod.substring(`replace('`.length, _mod.length - `')`.length); - - // split on findStartChar<whitespace?>,<whitespace?>findEndChar - const [key, replaceKey, shouldBeUndefined] = content.split( - new RegExp(`${findStartChar}\\s*,\\s*${findEndChar}`) - ); - - if (!shouldBeUndefined && key && replaceKey) - return variable.replaceAll(key, replaceKey); - } + case mod.startsWith('replace(') && mod.endsWith(')'): { + const m = _mod.match(/^replace\((['"])(.*?)\1\s*,\s*(['"])(.*?)\3\)$/); + if (m) { + const key = m[2]; + const replaceKey = m[4]; + return variable.replaceAll(key, replaceKey); + } + break; + }
848-852: join(...) parsing only supports single quotes; add dual‑quote supportMirror the approach used for replace(...) to accept both '...' and "..." safely.
Apply this diff:
- case mod.startsWith('join(') && mod.endsWith(')'): { - // Extract the separator from join('separator') or join("separator") - const separator = _mod.substring(`join('`.length, _mod.length - `')`.length); - return variable.join(separator); - } + case mod.startsWith('join(') && mod.endsWith(')'): { + const m = _mod.match(/^join\((['"])(.*?)\1\)$/); + if (m) { + const separator = m[2]; + return variable.join(separator); + } + break; + }
962-965: Greedy capture in check pattern can over‑matchThe pattern uses .* and will eat quotes; even per the comment, it should be quote‑bounded.
Apply this diff:
- const checkTrue = `(?<mod_check_true>.*)`; - const checkFalse = `(?<mod_check_false>.*)`; + const checkTrue = `(?<mod_check_true>[^"]*)`; + const checkFalse = `(?<mod_check_false>[^"]*)`;
385-401: Delegate nested-conditional parsing toextractCheckSuffixThe existing regex in
packages/core/src/formatters/base.ts(lines 385–401) fails on escaped quotes and embedded||; replace it with:- // CHECK TRUE/FALSE logic - const checkTF = potentialVariableTemplate.match( - /\[\s*"(?<mod_check_true>.*?)"\s*\|\s*\|\s*"(?<mod_check_false>.*?)"\s*\]\s*\}$/ - ); - const modCheck = checkTF?.groups && { - true: checkTF?.groups?.mod_check_true, - false: checkTF?.groups?.mod_check_false, - }; - const relativeTrueIndex = - checkTF?.[0].indexOf(`"${modCheck?.true}"`)! + `"`.length; - const relativeFalseIndex = - checkTF?.[0].lastIndexOf(`"${modCheck?.false}"`)! + `"`.length; - const modCheckWithOffsets = modCheck && { - ...modCheck, - offsetFromVariable: relativeTrueIndex, - offsetFromTrueCase: - relativeFalseIndex - (relativeTrueIndex + modCheck.true.length), - }; // end of CHECK TRUE/FALSE + // CHECK TRUE/FALSE logic (robust split that supports nested conditionals and quotes) + const modCheckWithOffsets = this.extractCheckSuffix(potentialVariableTemplate);Add this helper in
BaseFormatter(outside the shown range):// Parses …["<true>"||"<false>"]} at the end of the variable template. // Returns true/false strings plus offsets used by the nested-variable replacer. private extractCheckSuffix(s: string): | { true: string; false: string; offsetFromVariable: number; offsetFromTrueCase: number } | undefined { // Trim trailing whitespace and closing brace let end = s.length - 1; while (end >= 0 && /\s|\}/.test(s[end])) end--; if (end < 0 || s[end] !== ']') return undefined; // Find matching '[' while respecting quotes and escapes let inSingle = false, inDouble = false, esc = false, depth = 0; let open = -1, sep = -1; for (let i = 0; i <= end; i++) { const ch = s[i]; if (esc) { esc = false; continue; } if (ch === '\\') { esc = true; continue; } if (!inSingle && ch === '"') { inDouble = !inDouble; continue; } if (!inDouble && ch === "'") { inSingle = !inSingle; continue; } if (inSingle || inDouble) continue; if (ch === '[') { depth++; if (open === -1) open = i; continue; } if (ch === ']') { depth--; continue; } if (ch === '|' && s[i + 1] === '|' && depth === 1) { sep = i; i++; continue; } } if (open === -1 || sep === -1) return undefined; const rawTrue = s.slice(open + 1, sep).trim(); const rawFalse = s.slice(sep + 2, end).trim(); const unquote = (raw: string) => { if (!raw) return ''; const q = raw[0]; if ((q === '"' || q === "'") && raw[raw.length - 1] === q) { return raw.slice(1, -1); } return raw; }; const trueVal = unquote(rawTrue); const falseVal = unquote(rawFalse); // Compute offsets from the start of the variable template let tStart = open + 1; while (/\s/.test(s[tStart])) tStart++; if (s[tStart] === '"' || s[tStart] === "'") tStart++; let fStart = sep + 2; while (/\s/.test(s[fStart])) fStart++; if (s[fStart] === '"' || s[fStart] === "'") fStart++; return { true: trueVal, false: falseVal, offsetFromVariable: tStart, offsetFromTrueCase: fStart - tStart - trueVal.length, }; }
4 participants
Example (all versions below work)
{variableType1.propertyName1::modifier1a:modifier1b::comparator1and2::variableType2.propertyName2::modifier2["{variableType3.propertyName3::modifier3["nested true"||"nested false"]"|""]}Summary by CodeRabbit
New Features
Performance
Bug Fixes
Refactor