Repository navigation
Conversation
|
✅ KSAI Review: Finished
Run report (federated) · 1 paid run · $0.1006 total
Reviewing cost $0.1000; deciding how to run it cost $0.0006 |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
| Check | Result |
|---|---|
| Scope | Postman importer reference normalisation (normaliseJsonPath) and its unit tests |
| Mandate | typescript-code-reviewer (typescript-code-review) |
| Findings | 3 Low |
| Findings audit | Audited by findings-auditor subagent: F1 downgraded Medium to Low (pre-existing, engine premise unverifiable in tree), F2 kept with corrected output string, F3 merged into F2 replacement, F4 kept. No missed bugs, all anchors verified. |
The rewrite correctly widens bracket-notation normalisation to every non-identifier Postman reference. Traced regex captures and quote escaping match all asserted outputs; _ is bound to the render context, so the single-wrap form resolves.
Additional Risk pass: No additional concerns found. No secrets, injection, or resource issues in the two changed files.
There was a problem hiding this comment.
🟡 Changes recommended
Unresolved import regressions affect GraphQL body serialization and variable resolution.
3 open findings
What changed in this PR
Expands Postman variable conversion so names containing spaces, dots, leading digits, and other non-identifier characters use bracket notation.
Changes:
- Replaces dash-only normalization with identifier validation.
- Adds tests for special names, mixed references, and unchanged text.
| File | Description |
|---|---|
| packages/insomnia/src/main/importers/importers/postman.ts | Expands variable-name normalization. |
| packages/insomnia/src/main/importers/importers/postman.test.ts | Adds focused normalization tests. |
🧠 Review effort: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| return `{{${replaced}}}`; | ||
| return input.replace(/{{\s*([^{}]*?)\s*}}/g, (match, name: string) => { | ||
| // If the name is empty or a valid identifier, leave it as is. | ||
| if (!name || VALID_IDENTIFIER.test(name)) { |
| return match; | ||
| } | ||
| // Otherwise, force bracket notation for the name. | ||
| return `{{${forceBracketNotation('_', name)}}}`; |
|
✅ KSAI Review: Finished
Run report (federated) · 1 paid run · $0.0734 total
Reviewing cost $0.0729; deciding how to run it cost $0.0005 |
There was a problem hiding this comment.
| Check | Result |
|---|---|
| Scope | Postman importer: variable-reference normalisation (identifier whitelist gate, bracket-notation rewrite) and its unit tests |
| Mandate | typescript-code-reviewer |
| Findings | 1 Medium, 1 Low |
| Findings audit | One findings-auditor subagent verified both findings against the code: Finding 1 downgraded High to Medium with reworded body, Finding 2 upheld, no missed bugs. |
The rewrite fixes real gaps in the old dash-only rewriter, and the tests pin the new outputs. The whitelist follows ECMAScript identifier rules, not the LiquidJS grammar the strings render under. Non-ASCII names are the exposed case.
Additional Risk pass found nothing: no secrets, no new injection surface, no leak or growth risks.
|
|
||
| // old: {{ arr-name-with-dash }} | ||
| // new: {{ _['arr-name-with-dash'] }} | ||
| const VALID_IDENTIFIER = /^[\p{ID_Start}_$][\p{ID_Continue}$\u200C\u200D]*$/u; |
There was a problem hiding this comment.
🟠 Medium bug
VALID_IDENTIFIER accepts names by ECMAScript identifier rules. The engine is LiquidJS (package.json: liquidjs ^10.27.0), whose lexer accepts ASCII identifiers only — unverified against the installed source. An imported variable named 中文字符 stays as {{中文字符}}, so render throws instead of resolving, and postman.test.ts:57-59 codifies it. Bracket form _['中文字符'] renders (liquid-compat.test.ts:41-42). Fix: scope the regex to the engine's grammar; non-ASCII names get bracket notation; foo, _bar1, $randomInt keep passing.
| const VALID_IDENTIFIER = /^[\p{ID_Start}_$][\p{ID_Continue}$\u200C\u200D]*$/u; | |
| const VALID_IDENTIFIER = /^[a-zA-Z_$][a-zA-Z0-9_$]*$/; |
React 👍 if this finding helped, 👎 if it did not.
| // If the name is empty or a valid identifier, leave it as is. | ||
| if (!name || VALID_IDENTIFIER.test(name)) { | ||
| return match; | ||
| } | ||
| // Otherwise, force bracket notation for the name. | ||
| return `{{${forceBracketNotation('_', name)}}}`; |
There was a problem hiding this comment.
🔵 Low delete
Both comments restate the adjacent if/else: the condition line and the return say the same. The why lives in the block comment at postman.ts:99-102, and the empty-capture branch is pinned by postman.test.ts:76. Remove them.
| // If the name is empty or a valid identifier, leave it as is. | |
| if (!name || VALID_IDENTIFIER.test(name)) { | |
| return match; | |
| } | |
| // Otherwise, force bracket notation for the name. | |
| return `{{${forceBracketNotation('_', name)}}}`; | |
| if (!name || VALID_IDENTIFIER.test(name)) { | |
| return match; | |
| } | |
| return `{{${forceBracketNotation('_', name)}}}`; |
React 👍 if this finding helped, 👎 if it did not.
d37d51c to
f4ae274
Compare
Summary
PM variable names can contain invalid identifiers (
{{var Space}},{{1st}},{{a/b}}), butInsomnia renders
{{name}}as a plain lookup, which only works for identifier-like names. PreviouslynormaliseJsonPathonly rewrote names containing-, so every other special name imported unchanged andfailed to resolve.
normaliseJsonPathnow rewrites any{{ name }}whose name is not a valid identifier to{{ _['name'] }}. Valid names are left untouched.INS-3935