Repository navigation
fix(code-placeholders): harden shell placeholder interpolation in arithmetic contexts - #8760
waleedlatif1 wants to merge 1 commit into
Conversation
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
9dfab5f to
1a74dc6
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
1 issue found across 3 files
Confidence score: 3/5
shell.tsmisses arithmetic commands invoked throughcommandorbuiltin, sobuiltin let n={{KEY}}can bypass arithmetic detection. Extend detection to recognize these prefixes.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. When an issue isn't valid or won't be fixed in this PR, reply in its thread with the reason and then resolve the thread. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="apps/sim/lib/execution/code-placeholders/shell.ts">
<violation number="1" location="apps/sim/lib/execution/code-placeholders/shell.ts:659">
P2: `command` and `builtin` can prefix Bash builtins, but this treats the prefix as the command and ignores the following `let`; `builtin let n={{KEY}}` bypasses arithmetic detection. Recognize these invocation prefixes before selecting the command word.</violation>
</file>
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
1a74dc6 to
f0150b8
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
f0150b8 to
fbc91c8
Compare
|
@cubic-dev-ai review this PR |
fbc91c8 to
6d07b57
Compare
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
6d07b57 to
0477e23
Compare
1f8cc67 to
77eef69
Compare
77eef69 to
d50f692
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
Reply with feedback, questions, or to request a fix.
Turn on auto-fix | Re-trigger cubic
d50f692 to
91315e4
Compare
91315e4 to
a3c63ce
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
All reported issues were addressed across 3 files
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
a3c63ce to
807488d
Compare
807488d to
2f3e963
Compare
|
@cubic-dev-ai review this PR |
@waleedlatif1 I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
No issues found across 3 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
You've manually re-run cubic several times on this PR. Each manual re-review checks the full PR again and counts toward your usage quota. To preserve your usage limits, we recommend letting cubic automatically review new commits.
Turn on auto-fix | Re-trigger cubic
| if (declared) { | ||
| // A re-declaration resets the name's type before this one's attributes apply, so a later | ||
| // `declare -a` (indexed) clears an earlier `-A` (associative) and vice versa. | ||
| if (command.associativeOption || command.indexedOption) clearNameType(declared) |
There was a problem hiding this comment.
declare -a and declare -A erase a previously tracked integer attribute, but Bash keeps it unless +i clears it. For example, declare -i n; declare -a n; n="{{KEY}}" compiles. With KEY=values[$(printf injected >&2)], Bash runs the embedded command. Preserve the integer attribute when updating the array type.
How this was verified: The compiled example ran in Bash and printed injected from the supplied assignment value.
| if (command.nameArgumentBuiltin && command.sawCommandWord && SHELL_BARE_NAME.test(word)) { | ||
| clearNameType(word) |
There was a problem hiding this comment.
unset -f removes functions, not variables, but this branch clears variable attributes for its name arguments. declare -i n; unset -f n; n="{{KEY}}" therefore compiles even though Bash keeps n integer. With KEY=values[$(printf injected >&2)], the assignment runs the embedded command. Track the unset options and clear variable attributes only when the command removes a variable.
How this was verified: The compiled example ran in Bash and printed injected while unset -f left the integer variable unchanged.
This branch was previously deployed
1 participant
Summary
$(( )),$[ ]and(( ))were covered.-eq/-ne/-lt/-le/-gt/-geinside[[ ]]letargumentsdeclare/typeset/local -iassignments, and later assignments to names declared integer${a[...]},a[...]=,([...]=), and quotedname[...]passed to builtins that take a variable name)${name:offset:length}[ ]/testbuiltin, default-value expansions (${x:-...}) and plain heredocs outside arithmetic still compile as before. Detection is conservative where a position cannot be decided precisely.Type of Change
Testing
rejects shell placeholders whose values enter arithmetictable with 30 cases. All of them fail on the previous scanner.bunx vitest run lib/execution/(52 files, 1177 tests), rootbun run test,bun run lint,bun run type-check,bun run check:audits(58 audits).Checklist
test-auditauthoring gate)