feat: JSON template engine with shell-style variable substitution - #1
Conversation
📝 WalkthroughWalkthroughAdds a TypeScript JSON-template engine: scanner, sentinel-based compile (scan → sentinel-insert → single JSON.parse) and AST builder, Template runtime with bare vs embedded semantics, shell-style defaults, dotted-path own-property resolution, strict-mode errors, JSON-null helpers, tests, and documentation. Changes
Sequence DiagramsequenceDiagram
participant User
participant Compiler as Compiler\n(src/template.ts)
participant Scanner as Scanner\n(src/scanner.ts)
participant Parser as JSON_Parser
participant TreeBuilder as TreeBuilder\n(src/tree.ts)
participant Template as Template\n(instance)
participant Renderer as Renderer\n(render)
participant Context as Context
User->>Compiler: compile(templateString, options)
Compiler->>Scanner: scan(templateString)
Scanner-->>Compiler: ScannedExpr[]
Compiler->>Compiler: inject SENTINEL markers into text
Compiler->>Parser: JSON.parse(withSentinels)
Parser-->>Compiler: parsedValue
Compiler->>TreeBuilder: buildTree(parsedValue)
TreeBuilder-->>Compiler: AST (TNode)
Compiler->>Template: new Template(AST, variables, options.strict)
User->>Template: render(context)
Template->>Renderer: traverse AST nodes
Renderer->>Context: resolve dotted-paths (own-property lookup)
Context-->>Renderer: value or undefined
Renderer->>Renderer: apply fallbacks / coerce (bare vs embedded)
Renderer-->>Template: rendered JS value
Template-->>User: final value / toJSON string
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/scanner.test.ts`:
- Around line 83-85: The test "unterminated expression" is currently closing the
`${...}` and thus tests invalid JSON rather than an unterminated expression;
update the test input in the it block that calls compile to leave the expression
unclosed (e.g. change compile('{"v": ${oops}') to compile('{"v": ${oops') or
another string where the `${` has no matching `}`) so the compile function's
expression parser (compile) triggers TemplateParseError as intended.
In `@src/__tests__/template.test.ts`:
- Around line 163-179: Add two Vitest regression tests: (1) under the
strict-mode suite, add expectations that rendering with strict:true throws
UnresolvedVariableError for unresolved variables named "toString" and
"__proto__" when given a plain {} context by invoking compile(...).render({});
(2) add a test that a template string containing the engine's internal sentinel
character (the parsing placeholder used by the template engine) does not break
parsing and still respects defaults — compile a template that includes the
sentinel character plus a defaulted variable (use compile(..., { strict: true
}).render({})) and assert it returns the expected default value; reference
compile and UnresolvedVariableError to locate where to add these cases.
In `@src/template.ts`:
- Around line 47-53: The issue is repeated JSON parses at render time via
tryParseJSON when handling bare defaults; move parsing to compile-time and store
the parsed fallback so render() can use it directly. Modify the compile() logic
that processes expr.defaultValue (and the other occurrences noted around lines
113-115) to call tryParseJSON once and attach the resulting parsed value (or a
marker for invalid JSON) to the compiled template node, then update render() to
read that cached parsed fallback instead of calling
tryParseJSON(expr.defaultValue) repeatedly; keep the tryParseJSON helper for
compile-time use and ensure compiled nodes expose a field like parsedDefault or
defaultFallback used by the render path.
- Around line 25-31: The resolve function is traversing object properties via
bracket access which follows the prototype chain and allows inherited properties
like toString or __proto__ to resolve; update resolve to only access own
properties by checking Object.prototype.hasOwnProperty.call(current, part) (and
return undefined if it’s not an own property) before assigning current =
(current as Record<string, unknown>)[part]; keep the existing typeof/current
null checks and use the same function name resolve to locate the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 40de9431-d627-46c5-8e62-13c1406d7292
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
bc4ecf8 to
c0b948e
Compare
commit: |
|
Addressed all five review comments in a single amended commit: Fixes:
Documentation updates:
67 tests, @coderabbitai full review |
|
Triggering a full review of the updated PR now. ✅ Actions performedFull review triggered. |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/__tests__/scanner.test.ts`:
- Around line 82-98: Current tests in src/__tests__/scanner.test.ts only assert
error class for compile(...) calls; update each spec in the 'parse errors' suite
to also assert TemplateParseError.message contains an expected fragment and that
the error.offset equals the expected numeric position. For each case
(unterminated expression, empty expression, invalid variable name, invalid JSON
structure) capture the thrown error from compile(...) (e.g., using try/catch or
expect().toThrowError and returning the error), then assert on
error.message.includes('<expected fragment>') and error.offset === <expected
offset> to lock API behavior; reference the compile function and
TemplateParseError type when locating the tests to change.
In `@src/__tests__/template.test.ts`:
- Around line 157-162: Add a unit test to exercise hyphenated variable names
because VAR_NAME_RE permits hyphens but no test covers it: in the tests around
compile/render (see compile and render usage and the 'names set' suite), add a
new it block that compiles a template with a hyphenated variable (for example
compile('{"v": ${my-var}}')), calls .render({ 'my-var': 42 }), and asserts the
result equals { v: 42 } to ensure hyphenated keys are handled end-to-end.
In `@src/json.ts`:
- Around line 15-20: Rename the function isLiteral to a clearer name
(suggestion: isNonStringPrimitive) and update its JSDoc to state it returns true
for null, boolean, and number (explicitly excluding strings); change the
exported function name and all references/usages (calls, imports, tests, and
type annotations) from isLiteral to isNonStringPrimitive to avoid JSON
terminology ambiguity while preserving the same implementation (keep isNull,
typeof checks and boolean return logic).
In `@src/template.ts`:
- Around line 234-238: In the catch block that currently throws new
TemplateParseError(`Template is not valid JSON...`, 0) replace the hardcoded 0
with an extracted offset: parse the thrown error's message (cast to
SyntaxError/Error) with a small regex to find a numeric column/position (e.g.
"column X" or "position X") and pass that number into the TemplateParseError
constructor; if no numeric position can be extracted, fall back to 0. Update the
catch around JSON.parse in template parsing (the catch handling
TemplateParseError creation) to perform this extraction before throwing.
In `@src/tree.ts`:
- Around line 86-97: Replace unary plus numeric coercion with explicit integer
parsing for sentinel indices: in the branch returning { kind: 'bare', idx:
+bareMatch[1] } and in the loop pushing { idx: +m[1] } use Number.parseInt(...,
10) instead (e.g., parse the captured group with Number.parseInt(bareMatch[1],
10) and Number.parseInt(m[1], 10)) to make intent and radix explicit in the
parsing logic around SENTINEL_RE and the parts assembly.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: d87afd98-1a6e-4580-8a90-61c132a2f12c
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
| } catch (error) { | ||
| throw new TemplateParseError( | ||
| `Template is not valid JSON (after extracting variables): ${(error as Error).message}`, | ||
| 0, | ||
| ); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
JSON parse error offset is always 0, losing position information.
When JSON.parse fails, the error offset is hardcoded to 0. The actual error position within the modified JSON string could be extracted from the native SyntaxError.message (some engines include column info), but mapping it back to the original template is non-trivial due to sentinel substitution. The current approach is pragmatic given the complexity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/template.ts` around lines 234 - 238, In the catch block that currently
throws new TemplateParseError(`Template is not valid JSON...`, 0) replace the
hardcoded 0 with an extracted offset: parse the thrown error's message (cast to
SyntaxError/Error) with a small regex to find a numeric column/position (e.g.
"column X" or "position X") and pass that number into the TemplateParseError
constructor; if no numeric position can be extracted, fall back to 0. Update the
catch around JSON.parse in template parsing (the catch handling
TemplateParseError creation) to perform this extraction before throwing.
There was a problem hiding this comment.
@coderabbitai could you create a ticket for proper line/column tracking for better error reporting?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue create \
--repo kagal-dev/json-template \
--title "feat: proper line/column tracking in TemplateParseError for better error reporting" \
--body "## Summary
When \`JSON.parse\` fails during template compilation in \`src/template.ts\`, the thrown \`TemplateParseError\` currently hardcodes the offset to \`0\`, losing actual position information.
## Motivation
Accurate line/column offsets in \`TemplateParseError\` would significantly improve the developer experience by pointing directly to the problematic location in the original template string, rather than always reporting offset \`0\`.
## Suggested Approach
- Parse the native \`SyntaxError\` message from \`JSON.parse\` using a regex to extract numeric column/position info (e.g. \`\"column X\"\` or \`\"position X\"\` patterns present in V8/SpiderMonkey messages).
- Map that extracted position back to the original template string, accounting for the sentinel substitutions made during compilation.
- Fall back to \`0\` if no numeric position can be extracted from the error message.
- Consider also tracking line numbers (not just character offsets) in \`TemplateParseError\` for multi-line templates.
## Context
- Identified in PR `#1` (review comment): https://github.com/kagal-dev/json-template/pull/1#discussion_r2971923053
- Affected code: \`src/template.ts\`, catch block around \`JSON.parse\` in the \`compile()\` function.
## Requested by
`@amery`" \
--assignee amery
Length of output: 51
@amery, I've created the follow-up issue: #2
It covers:
- Extracting position info from the native
SyntaxErrormessage via regex (e.g."column X"/"position X"patterns) - Mapping that position back to the original template string (accounting for sentinel substitutions)
- Falling back to
0when no numeric position can be extracted - Optionally tracking line numbers as well for multi-line templates
There was a problem hiding this comment.
Actionable comments posted: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@README.md`:
- Around line 101-110: Add a short note under the "Dotted key paths" section
clarifying that dotted resolution only checks own properties (not inherited ones
like toString, constructor, __proto__), reference the example using compile(...)
and .render(...), and mention that this behavior is the intentional
security/prototype-chain fix (linking or pointing readers to AGENTS.md for more
details).
In `@src/__tests__/template.test.ts`:
- Around line 212-214: The test currently uses a malformed placeholder so the
scanner's unterminated-expression path is exercised instead of the
sentinel-default guard; update the fixture in the it('sentinel in default value
throws') test to use a well-formed template placeholder (e.g. replace '{"v":
"${x:-\uE000"' with '{"v":"${x:-\uE000}"}') so compile(...) hits the
sentinel-default branch and still throws TemplateParseError, ensuring the
sentinel guard is exercised by the test.
In `@src/index.ts`:
- Around line 11-14: Decide whether isNull and jsonNull should be part of the
public root API: if they are intended public entrypoints, add documentation for
isNull and jsonNull to the README alongside Template, exported errors, and
VERSION and keep the export in src/index.ts; otherwise remove their re-export
from src/index.ts (so only Template, errors, VERSION, etc. remain public) and
instead export them from an internal module (e.g., ./json) or an internal barrel
to keep the root API deliberate. Ensure references to the symbols isNull and
jsonNull in docs/tests are updated to the chosen public or internal path.
In `@src/template.ts`:
- Around line 156-160: The object materialization in the 'object' branch uses
direct assignment (object[k] = ...) which allows "__proto__" prototype
poisoning; in the case handling inside renderNode where you iterate
node.entries, replace direct property assignment with creating the property via
Object.defineProperty on the created object (use enumerable: true, configurable:
true, writable: true and set value to this.renderNode(child, context)) so keys
like "__proto__" become real own properties and cannot mutate the prototype
chain.
In `@src/tree.ts`:
- Around line 113-118: The interpolated-key errors always use offset 0; fix by
threading real expression offsets into buildTree (compile should call
buildTree(..., exprs.map(expr => expr.offset))) and change buildTree's signature
to accept that offsets array; in the SENTINEL_RE check use SENTINEL_RE.exec(k)
(not test) to get the match index and then pass offsets[match.index] (or the
corresponding mapped offset) into TemplateParseError instead of 0, ensuring
SENTINEL_RE.lastIndex is handled/reset as before.
In `@src/types.ts`:
- Around line 2-13: Make the exported TemplateVariable interface immutable by
marking all properties readonly (e.g., readonly raw, name, defaultValue, bare,
offset) so consumers cannot mutate metadata, and when exposing
Template.variables ensure the runtime freezes each element (Object.freeze) and
the array itself before returning to match the readonly type; update any
factory/constructor or getter that produces Template.variables to return a
frozen array of frozen TemplateVariable objects.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 83e013bc-4f11-4334-9afa-cc1f923fae69
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
| export { | ||
| isNull, | ||
| jsonNull, | ||
| } from './json'; |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Decide whether jsonNull / isNull belong in the root API.
Re-exporting these here makes them part of the package’s semver contract alongside Template, the errors, and VERSION. If they are intended entrypoint APIs, document them in the README with the rest of the surface; otherwise keep them internal so the root barrel stays deliberate.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/index.ts` around lines 11 - 14, Decide whether isNull and jsonNull should
be part of the public root API: if they are intended public entrypoints, add
documentation for isNull and jsonNull to the README alongside Template, exported
errors, and VERSION and keep the export in src/index.ts; otherwise remove their
re-export from src/index.ts (so only Template, errors, VERSION, etc. remain
public) and instead export them from an internal module (e.g., ./json) or an
internal barrel to keep the root API deliberate. Ensure references to the
symbols isNull and jsonNull in docs/tests are updated to the chosen public or
internal path.
| SENTINEL_RE.lastIndex = 0; | ||
| if (SENTINEL_RE.test(k)) { | ||
| throw new TemplateParseError( | ||
| 'Variable expressions in object keys are not supported', | ||
| 0, | ||
| ); |
There was a problem hiding this comment.
Use the real source offset for interpolated-key parse errors.
This currently throws TemplateParseError(..., 0) for every key-position variable, so all such failures point at the start of the document even though the sentinel already identifies the originating expression. Please thread the scanned offsets into buildTree() and use the matched index here.
🩹 Suggested direction
-export function buildTree(value: unknown): TNode {
+export function buildTree(
+ value: unknown,
+ exprOffsets: readonly number[],
+): TNode {
if (isLiteral(value)) {
return { kind: 'literal', value };
}
if (typeof value === 'string') {
@@
}
if (Array.isArray(value)) {
- return { kind: 'array', items: value.map(v => buildTree(v)) };
+ return { kind: 'array', items: value.map(v => buildTree(v, exprOffsets)) };
}
if (typeof value === 'object') {
const entries = Object.entries(value as Record<string, unknown>)
.map(([k, v]): [string, TNode] => {
SENTINEL_RE.lastIndex = 0;
- if (SENTINEL_RE.test(k)) {
+ const match = SENTINEL_RE.exec(k);
+ if (match) {
throw new TemplateParseError(
'Variable expressions in object keys are not supported',
- 0,
+ exprOffsets[Number(match[1])] ?? 0,
);
}
- return [k, buildTree(v)];
+ return [k, buildTree(v, exprOffsets)];
});compile() would then pass exprs.map(expr => expr.offset) into the root buildTree() call.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/tree.ts` around lines 113 - 118, The interpolated-key errors always use
offset 0; fix by threading real expression offsets into buildTree (compile
should call buildTree(..., exprs.map(expr => expr.offset))) and change
buildTree's signature to accept that offsets array; in the SENTINEL_RE check use
SENTINEL_RE.exec(k) (not test) to get the match index and then pass
offsets[match.index] (or the corresponding mapped offset) into
TemplateParseError instead of 0, ensuring SENTINEL_RE.lastIndex is handled/reset
as before.
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue view 2 --repo kagal-dev/json-template
Length of output: 1455
@amery, understood — deferring to #2.
Worth noting that #2 currently describes only the JSON.parse catch-block offset in compile(). This case (object-key sentinel errors in buildTree() always reporting offset: 0) is a related but distinct occurrence — would you like me to update #2's body to also call it out explicitly so it doesn't get overlooked?
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
gh issue view 2 --repo kagal-dev/json-template --json title,body,url
Length of output: 1431
🏁 Script executed:
#!/bin/bash
gh issue edit 2 --repo kagal-dev/json-template --body "## Summary
When \`JSON.parse\` fails during template compilation in \`src/template.ts\`, the thrown \`TemplateParseError\` currently hardcodes the offset to \`0\`, losing actual position information.
Additionally, \`buildTree()\` in \`src/tree.ts\` always throws \`TemplateParseError(..., 0)\` when it detects a variable expression in an object key, even though the sentinel encodes the originating expression index — which maps directly to a real source offset.
## Motivation
Accurate offsets in \`TemplateParseError\` would significantly improve the developer experience by pointing directly to the problematic location in the original template string, rather than always reporting offset \`0\`.
## Affected locations
### 1. \`compile()\` — \`JSON.parse\` catch block (\`src/template.ts\`)
- Parse the native \`SyntaxError\` message from \`JSON.parse\` using a regex to extract numeric column/position info (e.g. \`\"column X\"\` or \`\"position X\"\` patterns present in V8/SpiderMonkey messages).
- Map that extracted position back to the original template string, accounting for the sentinel substitutions made during compilation.
- Fall back to \`0\` if no numeric position can be extracted from the error message.
- Consider also tracking line numbers (not just character offsets) in \`TemplateParseError\` for multi-line templates.
### 2. \`buildTree()\` — object-key sentinel check (\`src/tree.ts\`)
\`buildTree()\` currently uses \`SENTINEL_RE.test(k)\` and hard-codes \`0\` as the offset:
\`\`\`ts
if (SENTINEL_RE.test(k)) {
throw new TemplateParseError(
'Variable expressions in object keys are not supported',
0, // always 0 — real offset is knowable
);
}
\`\`\`
The sentinel encodes the expression index (e.g. \`\uE000E2\uE000\` → index \`2\`), which can be looked up in the \`exprs\` array already available at compile time. The fix is to:
- Thread a \`readonly number[]\` of expression offsets into \`buildTree()\` (e.g. \`exprs.map(e => e.offset)\` passed from \`compile()\`).
- Switch from \`SENTINEL_RE.test(k)\` to \`SENTINEL_RE.exec(k)\` to capture the match group.
- Use \`exprOffsets[Number(match[1])] ?? 0\` as the \`TemplateParseError\` offset.
- Propagate \`exprOffsets\` into recursive \`buildTree()\` calls for arrays and object values.
## Context
- Identified in PR `#1` (review comment — \`JSON.parse\` offset): https://github.com/kagal-dev/json-template/pull/1#discussion_r2971923053
- Identified in PR `#1` (review comment — \`buildTree()\` object-key offset): https://github.com/kagal-dev/json-template/pull/1#discussion_r2971939323
- Affected code: \`src/template.ts\` (catch block), \`src/tree.ts\` (\`buildTree()\` object-key branch).
## Requested by
`@amery`"
Length of output: 51
@amery, done! #2 has been updated to cover both cases:
compile()—JSON.parsecatch block (src/template.ts): the original item, mappingSyntaxErrorposition back through sentinel substitutions.buildTree()— object-key sentinel check (src/tree.ts): the new item, switching fromSENTINEL_RE.test(k)toSENTINEL_RE.exec(k), threadingexprOffsetsthrough the recursive calls, and usingexprOffsets[Number(match[1])] ?? 0instead of the hard-coded0.
c0b948e to
eabd255
Compare
|
Addressed round 2 review (4 of 5 comments, 1 deferred): Implemented:
Also fixed:
Deferred:
68 tests, @coderabbitai review please |
|
The deferral of item 5 is well-reasoned — without mapping sentinel-string offsets back to the original template positions, any reported offset would be misleading. The issue Triggering a review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (5)
README.md (1)
101-110:⚠️ Potential issue | 🟡 MinorDocument the own-property lookup rule next to dotted paths.
The section doesn't mention that resolution only follows own properties, treating inherited keys like
toString,constructor, and__proto__as missing. This is a security-relevant behavior that users should be aware of.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@README.md` around lines 101 - 110, The README's "Dotted key paths" section omits that resolution only checks own properties (inherited keys like toString, constructor, __proto__ are treated as missing); update that section to state the own-property lookup rule explicitly and give a short note or example using the compile/render API (e.g., the compile(...) and .render(...) example) clarifying that only Object.prototype.hasOwnProperty-style own keys are followed and inherited properties are not resolved.src/types.ts (1)
2-13: 🛠️ Refactor suggestion | 🟠 Major
TemplateVariableproperties should bereadonly.The previous review comment about making these fields
readonlyremains unaddressed. SinceTemplateVariableis part of the public API and represents compile-time metadata that shouldn't be mutated, marking all properties asreadonlywould prevent accidental corruption and align with the documented known limitation inAGENTS.md.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/types.ts` around lines 2 - 13, Update the TemplateVariable interface so all its properties are readonly: mark raw, name, defaultValue (optional), bare, and offset as readonly in the TemplateVariable declaration; then fix any call sites that attempt to mutate a TemplateVariable (e.g., assignments to raw/name/defaultValue/bare/offset) by creating a new object or copying with spread before changing values to preserve immutability.src/tree.ts (1)
113-119:⚠️ Potential issue | 🟡 MinorUse the real source offset for interpolated-key parse errors.
The hardcoded
offset: 0means all key-position variable errors point to the document start, which is unhelpful for debugging. The sentinel match already identifies the expression index, which can be used to look up the real offset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree.ts` around lines 113 - 119, The error uses a hardcoded offset 0 when rejecting variable expressions in object keys; instead capture the actual match position from SENTINEL_RE and pass that into TemplateParseError. Replace the SENTINEL_RE.test(k) check with an exec (or use the RegExp match result) to obtain match.index (resetting SENTINEL_RE.lastIndex as needed), compute the real offset from that index, and construct the TemplateParseError with that computed offset rather than 0 (referencing SENTINEL_RE, the key variable k, and TemplateParseError).src/__tests__/template.test.ts (1)
217-219:⚠️ Potential issue | 🟡 MinorSentinel-default test still exercises the wrong code path.
The test input
'{"v": "${x:-\uE000"}'is missing the closing}of the placeholder expression. This causes the scanner to throw for an unterminated expression rather than the sentinel collision guard incompile(). The test passes but for the wrong reason.🔧 Proposed fix
it('sentinel in default value throws', () => { - expect(() => compile('{"v": "${x:-\uE000"}')).toThrow(TemplateParseError); + expect(() => compile('{"v": "${x:-\uE000}"}')).toThrow( + /reserved sentinel character/i, + ); });Note the added
}before the final"to properly close the${...}expression, and the regex assertion to verify the correct error path is exercised.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/__tests__/template.test.ts` around lines 217 - 219, The test currently uses a malformed placeholder '{"v": "${x:-\uE000"}' (missing the closing '}') so the scanner throws for an unterminated expression instead of exercising the sentinel collision guard in compile(); update the test input to properly close the placeholder (e.g., '{"v": "${x:-\uE000}"}') and strengthen the assertion to verify compile() throws TemplateParseError for the sentinel-default collision by matching the error message with a regex that targets the sentinel-related text.src/template.ts (1)
156-162:⚠️ Potential issue | 🔴 CriticalAdd protection against
__proto__poisoning in object materialization.Direct assignment
object[k] = ...whenk === "__proto__"invokes JavaScript's prototype setter instead of creating an own property. This affects templates with literal"__proto__"keys—the returned object's prototype chain is mutated, and serialization viatoJSON()may omit the key.The existing prototype key safety tests (lines 194-196 in src/tests/template.test.ts) cover variable references to
__proto__, but not literal"__proto__"keys in the template structure itself.🔒 Proposed fix using Object.defineProperty
case 'object': { const object: Record<string, unknown> = {}; for (const [k, child] of node.entries) { - object[k] = this.renderNode(child, context); + Object.defineProperty(object, k, { + value: this.renderNode(child, context), + enumerable: true, + configurable: true, + writable: true, + }); } return object; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/template.ts` around lines 156 - 162, The object materialization in renderNode (case 'object') directly assigns object[k] = ..., which triggers prototype mutation when k === "__proto__"; change the loop so that for the "__proto__" key you create an own enumerable property instead of using the assignment—e.g., compute the value with this.renderNode(child, context) and then call Object.defineProperty(object, "__proto__", { value, enumerable: true, writable: true, configurable: true }); for that key, while keeping the direct assignment for all other keys; this ensures literal "__proto__" entries become normal own properties and do not mutate the returned object's prototype.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@README.md`:
- Around line 101-110: The README's "Dotted key paths" section omits that
resolution only checks own properties (inherited keys like toString,
constructor, __proto__ are treated as missing); update that section to state the
own-property lookup rule explicitly and give a short note or example using the
compile/render API (e.g., the compile(...) and .render(...) example) clarifying
that only Object.prototype.hasOwnProperty-style own keys are followed and
inherited properties are not resolved.
In `@src/__tests__/template.test.ts`:
- Around line 217-219: The test currently uses a malformed placeholder '{"v":
"${x:-\uE000"}' (missing the closing '}') so the scanner throws for an
unterminated expression instead of exercising the sentinel collision guard in
compile(); update the test input to properly close the placeholder (e.g., '{"v":
"${x:-\uE000}"}') and strengthen the assertion to verify compile() throws
TemplateParseError for the sentinel-default collision by matching the error
message with a regex that targets the sentinel-related text.
In `@src/template.ts`:
- Around line 156-162: The object materialization in renderNode (case 'object')
directly assigns object[k] = ..., which triggers prototype mutation when k ===
"__proto__"; change the loop so that for the "__proto__" key you create an own
enumerable property instead of using the assignment—e.g., compute the value with
this.renderNode(child, context) and then call Object.defineProperty(object,
"__proto__", { value, enumerable: true, writable: true, configurable: true });
for that key, while keeping the direct assignment for all other keys; this
ensures literal "__proto__" entries become normal own properties and do not
mutate the returned object's prototype.
In `@src/tree.ts`:
- Around line 113-119: The error uses a hardcoded offset 0 when rejecting
variable expressions in object keys; instead capture the actual match position
from SENTINEL_RE and pass that into TemplateParseError. Replace the
SENTINEL_RE.test(k) check with an exec (or use the RegExp match result) to
obtain match.index (resetting SENTINEL_RE.lastIndex as needed), compute the real
offset from that index, and construct the TemplateParseError with that computed
offset rather than 0 (referencing SENTINEL_RE, the key variable k, and
TemplateParseError).
In `@src/types.ts`:
- Around line 2-13: Update the TemplateVariable interface so all its properties
are readonly: mark raw, name, defaultValue (optional), bare, and offset as
readonly in the TemplateVariable declaration; then fix any call sites that
attempt to mutate a TemplateVariable (e.g., assignments to
raw/name/defaultValue/bare/offset) by creating a new object or copying with
spread before changing values to preserve immutability.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03807efe-2912-41aa-967b-d33fe548bb1f
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
eabd255 to
8a5e994
Compare
|
Addressed round 3 review (5 of 6 comments, 1 deferred): Implemented:
Also fixed:
Deferred:
70 tests, @coderabbitai review please |
|
The deferral of item 6 (interpolated-key parse error offset) is well-reasoned — any offset derived from the sentinel-replaced string would need non-trivial mapping back to the original template to be meaningful, and Triggering a review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 186-191: Replace the wordy phrase "at the moment `${` is
encountered" with the more concise "when `${` is encountered" in the sentence
discussing inString and sentinel prefixes; update the sentence that references
the inString flag, `${`, and the scanner's backslash handling (`pos += 2`) to
use "when `${` is encountered" so the wording is shorter and consistent.
In `@src/template.ts`:
- Around line 32-38: resolve currently calls key.split('.') on every lookup
which allocates each time; change the implementation and call sites to
pre-tokenize dotted paths at compile time and use the array of path segments
during render. Introduce a cached representation (e.g., a Map<string, string[]>
or change node shape to store segments) so resolve(context, segments: string[])
or resolveSegments(context, segments) walks the precomputed segments with hasOwn
checks (use the existing hasOwn and current variable) instead of splitting on
each call; update any callers that pass a string key (e.g., code that uses
resolve(key)) to pass the cached segments produced during compilation.
- Around line 53-60: parseFallback currently only checks the raw defaultValue
for SENTINEL but JSON.parse can decode "\uE000" into the sentinel char; update
parseFallback and any JSON.parse usage in compile to validate the parsed
string/structure for presence of SENTINEL and reject (or treat as invalid) if
found—specifically, after JSON.parse in parseFallback and in compile,
recursively scan string values in the parsed result for the SENTINEL constant
and return has: false or throw as appropriate; add a regression test fixture
that supplies a JSON-escaped "\uE000" in a template/default and assert the
runtime rejects it (also apply the same fix to the code paths noted around lines
212-243).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bb0f43c-5c09-4a4f-a058-e14abbdd1256
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
8a5e994 to
40753b0
Compare
|
Addressed round 4 review (3 comments):
80 tests (23 scanner, 56 template, 1 VERSION), @coderabbitai review please |
🧩 Analysis chain🏁 Script executed: Length of output: 16464
Name is split twice
Triggering a review now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/scanner.ts`:
- Around line 100-103: The explicit check segments.length === 0 is unreachable
because name.split('.') always returns at least one element; simplify the
validation in the scanner by removing that clause and only test each segment
against SEGMENT_RE (i.e., replace the condition in the block using segments and
SEGMENT_RE), leaving the throw of TemplateParseError(`Invalid variable name
"${name}"`, start + 2) intact so invalid or empty segments still trigger the
same error.
In `@src/template.ts`:
- Around line 10-11: The regex SENTINEL_ESC_RE currently matches any "\uE000"
sequence even when the backslash is itself escaped (e.g. in
String.raw`{"v":"\\uE000"}`); change the check to only match escapes that will
actually decode to U+E000 by requiring the backslash before "u" to be unescaped
(i.e. ensure an odd number of backslashes immediately preceding the "u" so the
escape is active). Replace the /\\u[eE]000/ pattern with a regex that enforces
an unescaped backslash (for example: use a lookbehind or an explicit
odd-backslash pattern) and apply the same fix to the other occurrence mentioned
(lines ~225-231), and add a regression test that includes
String.raw`{"v":"\\uE000"}` to ensure double-escaped literals are not rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 530220ec-6e4c-4af3-9ea6-01a06f45af6e
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/template.ts (1)
10-11:⚠️ Potential issue | 🟠 MajorOnly reject active
\uE000escapes, not double-escaped literals.
/\\u[eE]000/also matches the second slash in\\uE000, so valid templates containing the literal six-character text\uE000are rejected beforeJSON.parse()even though they would not decode to the sentinel. The guard should only match odd backslash runs, and the reported offset should point at the slash that actually starts the active unicode escape.Suggested fix
-/** Matches a JSON-encoded `\uE000` or `\ue000` escape sequence (6 ASCII chars). */ -const SENTINEL_ESC_RE = /\\u[eE]000/; +/** + * Matches a JSON unicode escape that will decode to U+E000. + * Even-length backslash runs like `\\uE000` stay literal and must remain allowed. + */ +const SENTINEL_ESC_RE = /(^|[^\\])((?:\\\\)*)\\u[eE]000/; @@ const escMatch = template.match(SENTINEL_ESC_RE); if (escMatch) { + const offset = escMatch.index! + escMatch[1].length + escMatch[2].length; throw new TemplateParseError( String.raw`Template contains JSON-escaped sentinel sequence (\uE000)`, - escMatch.index!, + offset, ); }Please also add a regression for
String.raw\{"v":"\uE000"}`` in the sentinel suite.Also applies to: 225-230
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/template.ts` around lines 10 - 11, The current SENTINEL_ESC_RE (/\\u[eE]000/) incorrectly matches double-escaped literals; change the sentinel guard to only detect active unicode escapes by matching an odd run of backslashes before the u (e.g. a pattern that asserts an odd number of backslashes like /(^|[^\\])(?:\\\\)*\\u[eE]000/ or equivalent) and update any offset calculation that reports the match to point at the actual backslash that starts the active escape (use the capture/group index for the slash). Also add a regression test in the sentinel test suite for String.raw`{"v":"\\uE000"}` to ensure literal six-character sequences are not rejected.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/template.ts`:
- Around line 53-63: parseFallback currently returns a single parsed
object/array value that is reused across renders (via renderNode), which allows
mutations in one render to affect others; fix by ensuring mutable fallbacks are
not shared: in parseFallback (or in renderNode where the fallback is used)
detect when the parsed value is an object or array and return a deep copy (e.g.,
structured clone / JSON parse/stringify or a custom deepClone) instead of the
original cached reference so each render receives its own instance; update
references to parseFallback and renderNode to clone the fallback before
returning to callers.
- Around line 93-94: The listVariables() API currently returns mutable metadata
while Template.variables and each TemplateVariable are frozen; update
listVariables() to return an immutable structure by either returning the
already-frozen this.variables array or by returning a new frozen array with each
element Object.freeze(...) (ensuring each TemplateVariable is frozen too).
Locate the listVariables() method and replace its mutable return with a
reference to the frozen this.variables or construct/return
Object.freeze(variables.map(v => Object.freeze(v))). Apply the same change to
the other similar accessor method mentioned so both public APIs consistently
expose immutable TemplateVariable instances.
---
Duplicate comments:
In `@src/template.ts`:
- Around line 10-11: The current SENTINEL_ESC_RE (/\\u[eE]000/) incorrectly
matches double-escaped literals; change the sentinel guard to only detect active
unicode escapes by matching an odd run of backslashes before the u (e.g. a
pattern that asserts an odd number of backslashes like
/(^|[^\\])(?:\\\\)*\\u[eE]000/ or equivalent) and update any offset calculation
that reports the match to point at the actual backslash that starts the active
escape (use the capture/group index for the slash). Also add a regression test
in the sentinel test suite for String.raw`{"v":"\\uE000"}` to ensure literal
six-character sequences are not rejected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 90a18e16-2c89-464f-b130-224887be1653
📒 Files selected for processing (11)
AGENTS.mdREADME.mdsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
Add @stylistic/arrow-parens 'always' rule scoped to source files, and add .tmp to the ignores list. Signed-off-by: Alejandro Mery <amery@apptly.co>
40753b0 to
d6e0e71
Compare
|
Addressed round 5 review (4 comments):
Also: 84 tests (23 scanner, 60 template, 1 VERSION), @coderabbitai full review |
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 18021 🏁 Script executed: Length of output: 2429
1. Dead 2. /(?<=(?:^|[^\\])(?:\\\\)*)\\u[eE]000/The variable-length lookbehind correctly tracks even-depth backslash runs:
The approach is sound. The 3. 4. Triggering the full review now. [full review] |
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/tree.ts (1)
113-118:⚠️ Potential issue | 🟡 MinorUse real source offsets for key-position variable parse errors.
Line 117 always reports offset
0, so{"${key}": ...}failures point to the wrong location. Thread expression offsets intobuildTree()and map the sentinel index to its original${...}offset.💡 Proposed fix
-export function buildTree(value: unknown): TNode { +export function buildTree( + value: unknown, + exprOffsets: readonly number[], +): TNode { @@ - if (Array.isArray(value)) { - return { kind: 'array', items: value.map((v) => buildTree(v)) }; + if (Array.isArray(value)) { + return { kind: 'array', items: value.map((v) => buildTree(v, exprOffsets)) }; } @@ - SENTINEL_RE.lastIndex = 0; - if (SENTINEL_RE.test(k)) { + SENTINEL_RE.lastIndex = 0; + const keyMatch = SENTINEL_RE.exec(k); + if (keyMatch) { throw new TemplateParseError( 'Variable expressions in object keys are not supported', - 0, + exprOffsets[Number.parseInt(keyMatch[1], 10)] ?? 0, ); } - return [k, buildTree(v)]; + return [k, buildTree(v, exprOffsets)]; });Also update the compile call site to pass offsets, e.g.
buildTree(parsed, exprs.map(e => e.offset)).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree.ts` around lines 113 - 118, The TemplateParseError thrown when object keys contain variable expressions uses a hardcoded offset (0); update buildTree() to accept an array of expression offsets (e.g., offsets from compile’s parse step) and when you detect a sentinel via SENTINEL_RE map the sentinel match index back to the corresponding expression offset and pass that into new TemplateParseError instead of 0. Thread the offsets through the compile call site (call buildTree(parsed, exprOffsets) or similar) so SENTINEL_RE test can lookup the original ${...} offset by sentinel position and surface the correct source location.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@eslint.config.mjs`:
- Line 4: Replace the generic TODO comment "// TODO: upstream arrow-parens to
`@poupe/eslint-config`" with a tracked reference: create an upstream issue or PR
and update the comment to include the issue/PR number or URL (for example "TODO:
upstream arrow-parens to `@poupe/eslint-config` — tracking: `#123`" or full PR link)
so the intent is actionable and won't drift; ensure the updated comment includes
the maintainer or repo name and the tracking ID so reviewers can follow up
(refer to the existing TODO comment text to locate where to edit).
In `@README.md`:
- Around line 206-209: Update the README to stop referencing a non-existent
JsonNull alias: either document the actual exported symbols (jsonNull and
isNull) instead of "JsonNull" or add and export a JsonNull type alias from the
library; specifically edit the paragraph that mentions JsonNull (around the
explanation of bare unresolved variables) to reference jsonNull: null and
isNull(...) or describe the newly exported JsonNull alias so consumers see
correct available API symbols (jsonNull, isNull, or JsonNull) and aren’t misled.
---
Duplicate comments:
In `@src/tree.ts`:
- Around line 113-118: The TemplateParseError thrown when object keys contain
variable expressions uses a hardcoded offset (0); update buildTree() to accept
an array of expression offsets (e.g., offsets from compile’s parse step) and
when you detect a sentinel via SENTINEL_RE map the sentinel match index back to
the corresponding expression offset and pass that into new TemplateParseError
instead of 0. Thread the offsets through the compile call site (call
buildTree(parsed, exprOffsets) or similar) so SENTINEL_RE test can lookup the
original ${...} offset by sentinel position and surface the correct source
location.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 574c6fa0-b404-4b66-a5d6-3d99a9c092be
📒 Files selected for processing (12)
AGENTS.mdREADME.mdeslint.config.mjssrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
| import { defineConfig } from '@poupe/eslint-config'; | ||
| import { defineConfig, GLOB_SRC } from '@poupe/eslint-config'; | ||
|
|
||
| // TODO: upstream arrow-parens to @poupe/eslint-config |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
TODO should be tracked to avoid drift.
Line 4 has a TODO for upstreaming; consider linking an issue/PR ID so it stays actionable.
If you want, I can draft a short issue body for upstream tracking.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@eslint.config.mjs` at line 4, Replace the generic TODO comment "// TODO:
upstream arrow-parens to `@poupe/eslint-config`" with a tracked reference: create
an upstream issue or PR and update the comment to include the issue/PR number or
URL (for example "TODO: upstream arrow-parens to `@poupe/eslint-config` —
tracking: `#123`" or full PR link) so the intent is actionable and won't drift;
ensure the updated comment includes the maintainer or repo name and the tracking
ID so reviewers can follow up (refer to the existing TODO comment text to locate
where to edit).
d6e0e71 to
35826fe
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
src/tree.ts (1)
79-120:⚠️ Potential issue | 🟡 MinorUse the originating placeholder offset for key-position parse errors.
Line 117 still hard-codes
0, so${...}inside an object key always points users at the start of the template instead of the offending placeholder. The sentinel index already identifies the right scanned expression, so this error can report a real source offset.🩹 Suggested direction
-export function buildTree(value: unknown): TNode { +export function buildTree( + value: unknown, + exprOffsets: readonly number[], +): TNode { if (isNonStringPrimitive(value)) { return { kind: 'literal', value }; } @@ if (Array.isArray(value)) { - return { kind: 'array', items: value.map((v) => buildTree(v)) }; + return { kind: 'array', items: value.map((v) => buildTree(v, exprOffsets)) }; } @@ if (typeof value === 'object') { const entries = Object.entries(value as Record<string, unknown>) .map(([k, v]): [string, TNode] => { SENTINEL_RE.lastIndex = 0; - if (SENTINEL_RE.test(k)) { + const match = SENTINEL_RE.exec(k); + if (match) { throw new TemplateParseError( 'Variable expressions in object keys are not supported', - 0, + exprOffsets[Number.parseInt(match[1], 10)] ?? 0, ); } - return [k, buildTree(v)]; + return [k, buildTree(v, exprOffsets)]; });The root call in
src/template.tscan then passexprs.map(expr => expr.offset).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tree.ts` around lines 79 - 120, The object-key parse error currently always reports offset 0; update the throw in buildTree (the TemplateParseError raised when SENTINEL_RE.test(k) is true inside the Object.entries mapping) to accept and propagate the actual placeholder offset instead of 0: change the call site so buildTree can receive a placeholder-offset parameter (e.g., pass in the matching sentinel's offset when scanning keys using SENTINEL_RE) and rethrow TemplateParseError with that offset; ensure the root caller (in src/template.ts) passes exprs.map(expr => expr.offset) or otherwise supplies the originating expr offset into buildTree so the error reports the real source position.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/template.ts`:
- Line 107: The runtime mutability of the Set stored in this.names (created from
variables) should be prevented: after creating the Set via new
Set(variables.map(v => v.name)) call Object.freeze on it and keep the property
typed as ReadonlySet<string> so callers still see immutability at compile-time
and the runtime object is frozen; update the constructor/assignment that sets
this.names to freeze the Set instance (reference the this.names and variables
symbols in template.ts) so external code cannot mutate it at runtime.
---
Duplicate comments:
In `@src/tree.ts`:
- Around line 79-120: The object-key parse error currently always reports offset
0; update the throw in buildTree (the TemplateParseError raised when
SENTINEL_RE.test(k) is true inside the Object.entries mapping) to accept and
propagate the actual placeholder offset instead of 0: change the call site so
buildTree can receive a placeholder-offset parameter (e.g., pass in the matching
sentinel's offset when scanning keys using SENTINEL_RE) and rethrow
TemplateParseError with that offset; ensure the root caller (in src/template.ts)
passes exprs.map(expr => expr.offset) or otherwise supplies the originating expr
offset into buildTree so the error reports the real source position.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 685f7327-6121-4d30-a312-bda6729bac66
📒 Files selected for processing (12)
AGENTS.mdREADME.mdeslint.config.mjssrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
35826fe to
63b1ff3
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/tree.ts`:
- Line 94: The while condition uses a redundant null-coalescing on RegExp.exec —
change the loop to directly assign m from SENTINEL_RE.exec(value) (i.e., while
((m = SENTINEL_RE.exec(value)))) and remove the "?? undefined" so the null
return correctly breaks the loop; alternatively, if type checking complains,
widen the type of m to allow null instead of coalescing.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 4fb01f8b-b077-4be4-aed8-d86a6df575eb
📒 Files selected for processing (13)
AGENTS.mdREADME.mdeslint.config.mjspackage.jsonsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
Compile-once, render-many template engine that treats
JSON structure as a first-class concern. Variables in
bare positions preserve their native type; variables
inside strings concatenate as text.
- scan() with brace-depth-aware parsing and string
context tracking for bare vs embedded classification
- buildTree() converts post-JSON.parse values into a
template AST using sentinel-indexed markers
- Template class with render() and toJSON() methods
- Shell-style ${var:-default} with typed bare defaults
(JSON-parsed at compile time) and string embedded
defaults
- Bare object/array defaults deep-copied per render
via structuredClone (no cross-render mutation)
- Dotted key paths with per-segment validation and
pre-tokenized paths (split once at compile time);
own-property-only resolution (no prototype chain)
- listVariables() for static analysis without valid
JSON; returns frozen results matching Template API
- Strict mode (UnresolvedVariableError) and lenient
mode (null for bare, "" for embedded)
- Embedded non-primitive coercion via JSON.stringify
- Sentinel collision guard rejects literal U+E000 and
JSON-escaped \uE000 in input; double-escaped
\\uE000 (literal text) is allowed
- Object.defineProperty for safe __proto__ key handling
in rendered objects (no prototype pollution)
- TemplateVariable fields readonly + elements frozen
at construction time
- AGENTS.md with pipeline overview, key invariants,
and known limitations
- README.md with full API reference, own-property
rule, jsonNull/isNull documentation
- 84 tests across scanner, template, and index suites
Signed-off-by: Alejandro Mery <amery@apptly.co>
publint expects the git+ protocol prefix. Signed-off-by: Alejandro Mery <amery@apptly.co>
63b1ff3 to
b5c8ba0
Compare
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 (1)
src/index.ts (1)
1-1:⚠️ Potential issue | 🟠 MajorPotential Node 18 compatibility gap with JSON import attributes.
Line 1 uses
with { type: 'json' }, which is not supported in Node 18.0–18.19.x despite the package declaring"engines": { "node": ">=18.0.0" }. This syntax is only supported from Node 18.20.0 onwards. Since the build output preserves ESNext module syntax without transpilation, consumers on Node 18.0–18.19.x will fail at module load time.♻️ Safer Node-18-compatible alternative
-import pkg from '../package.json' with { type: 'json' }; +import { createRequire } from 'node:module'; + +const require = createRequire(import.meta.url); +const pkg = require('../package.json') as { version: string };🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/index.ts` at line 1, The import statement "import pkg from '../package.json' with { type: 'json' };" uses unsupported "with" JSON-import syntax; replace it with a Node-18-compatible approach such as reading and parsing the file via the fs module (e.g., use fs.readFileSync(new URL('../package.json', import.meta.url), 'utf8') and JSON.parse(...)) or, if you prefer import assertions, change to the valid ESM syntax "import pkg from '../package.json' assert { type: 'json' };" so consumers on Node 18.0–18.19.x won't fail at module load time—update the code where the original import line appears.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/template.ts`:
- Line 254: The template currently recomputes path segments with
expr.name.split('.') in compile; instead, add a segments: string[] field to the
ScannedExpr type in scanner.ts and populate it when each ScannedExpr is created
(e.g., split the name once during scanning), then update template.ts to use
expr.segments (replace expr.name.split('.')) wherever segments are needed so the
split work isn’t duplicated.
---
Outside diff comments:
In `@src/index.ts`:
- Line 1: The import statement "import pkg from '../package.json' with { type:
'json' };" uses unsupported "with" JSON-import syntax; replace it with a
Node-18-compatible approach such as reading and parsing the file via the fs
module (e.g., use fs.readFileSync(new URL('../package.json', import.meta.url),
'utf8') and JSON.parse(...)) or, if you prefer import assertions, change to the
valid ESM syntax "import pkg from '../package.json' assert { type: 'json' };" so
consumers on Node 18.0–18.19.x won't fail at module load time—update the code
where the original import line appears.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 5604a6f3-29e1-482f-bf30-77232aadafbb
📒 Files selected for processing (12)
AGENTS.mdREADME.mdpackage.jsonsrc/__tests__/scanner.test.tssrc/__tests__/template.test.tssrc/errors.tssrc/index.tssrc/json.tssrc/scanner.tssrc/template.tssrc/tree.tssrc/types.ts
| const exprs = scan(template); | ||
|
|
||
| const variables = exprs.map((expr) => toVariable(template, expr)); | ||
| const segments = exprs.map((expr) => expr.name.split('.')); |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor: Duplicate path splitting between scanner and template.
The scanner already validates segments via name.split('.') (scanner.ts line 100), but compile splits again here. Consider storing pre-split segments in ScannedExpr to avoid redundant work.
♻️ Optional: Store segments in ScannedExpr
In src/scanner.ts, add segments: string[] to ScannedExpr and populate it during scanning:
export interface ScannedExpr {
offset: number
length: number
name: string
+ segments: string[]
defaultValue?: string
inString: boolean
}Then in src/template.ts:
- const segments = exprs.map((expr) => expr.name.split('.'));
+ const segments = exprs.map((expr) => expr.segments);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/template.ts` at line 254, The template currently recomputes path segments
with expr.name.split('.') in compile; instead, add a segments: string[] field to
the ScannedExpr type in scanner.ts and populate it when each ScannedExpr is
created (e.g., split the name once during scanning), then update template.ts to
use expr.segments (replace expr.name.split('.')) wherever segments are needed so
the split work isn’t duplicated.
1 participant
Summary
${var:-default}variable substitutionas strings — position determined at compile time
paths (split once at compile time), and own-property-only
resolution (no prototype chain leakage)
listVariables()static analysisdefaults deep-copied per render via
structuredCloneJSON-escaped
\uE000sequences; double-escaped\\uE000(literal text) is correctly allowedObject.definePropertyfor safe__proto__key handling (no prototype pollution)TemplateVariablefieldsreadonly+ elements frozenat construction;
listVariables()returns frozen resultsStructure
Test plan
pnpm precommitpasses (build, lint, typecheck, test)Summary by CodeRabbit
New Features
Documentation
Tests
Style
Chores