[Claimed #1777] fix: add variable substitution to keys tool#1813
[Claimed #1777] fix: add variable substitution to keys tool#1813github-actions[bot] wants to merge 2 commits intomainfrom
Conversation
The `keys` tool with `method: "type"` was not calling `substituteVariables()` before typing, so `%variableName%` tokens were typed literally instead of being resolved. This is easy to trigger when the agent clears a field (Ctrl+A → Delete) then types a variable value — it stays on the `keys` tool for the whole flow. - Accept `variables` parameter in `keysTool` (matching `typeTool`) - Call `substituteVariables()` before `page.type()` in the "type" branch - Pass `variables` to `keysTool` in `createAgentTools` - Return original token in result to avoid exposing sensitive values
Matches the pattern used by typeTool and actTool so the LLM agent knows %variableName% syntax is available when variables are present.
github-actions
bot
added
external-contributor
|
Greptile SummaryThis PR fixes variable substitution ( Key changes:
Issue found: The Confidence Score: 3/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A["keysTool called\n(v3, variables?)"] --> B{variables\nprovided?}
B -- Yes --> C["Build valueDescription\nwith available var names"]
B -- No --> D["Use default\nvalueDescription"]
C --> E["Return tool(...)"]
D --> E
E --> F["execute called\n(method, value, repeat)"]
F --> G{method?}
G -- type --> H["substituteVariables(value, variables)\n→ actualValue"]
H --> I["page.type(actualValue, delay:100)\n× times"]
I --> J["recordAgentReplayStep\nwith original value token"]
J --> K["return { success, method,\nvalue ← original token, times }"]
G -- press --> L["page.keyPress(value, delay:100)\n× times\n⚠️ NO substitution"]
L --> M["recordAgentReplayStep"]
M --> N["return { success, method, value, times }"]
Last reviewed commit: 9c30883 |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| const valueDescription = hasVariables | ||
| ? `The text to type, or the key/combo to press (Enter, Tab, Cmd+A). Use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}` | ||
| : "The text to type, or the key/combo to press (Enter, Tab, Cmd+A)"; |
There was a problem hiding this comment.
Variable substitution description applies to both methods
The valueDescription hints that %variableName% substitution is available for the value field, but substituteVariables() is only called inside the method === "type" branch (line 43). If the LLM reads this description and uses method="press" with a token like %apiKey%, the literal string %apiKey% will be forwarded directly to page.keyPress(), which will either fail or press unexpected keys — the substitution silently does not occur.
The description should be scoped to clarify that variable substitution only works with method="type":
| const valueDescription = hasVariables | |
| ? `The text to type, or the key/combo to press (Enter, Tab, Cmd+A). Use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}` | |
| : "The text to type, or the key/combo to press (Enter, Tab, Cmd+A)"; | |
| const valueDescription = hasVariables | |
| ? `The text to type (method="type"), or the key/combo to press (method="press") (Enter, Tab, Cmd+A). When using method="type", use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}` | |
| : "The text to type, or the key/combo to press (Enter, Tab, Cmd+A)"; |
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
2 issues found across 2 files
Confidence score: 2/5
- There is a high-confidence security risk in
packages/core/lib/v3/agent/tools/keys.ts: on the error path, substituted secret values can be exposed to the LLM instead of staying as%variableName%tokens. packages/core/lib/v3/agent/tools/keys.tsalso has a behavior mismatch wherevalueDescriptionadvertises%variableName%substitution, butsubstituteVariables()only runs formethod === "type", so other methods may fail or behave inconsistently.- Given the concrete secret-leak impact (severity 7/10, confidence 8/10) plus a second medium-severity logic issue, this looks risky to merge without a fix.
- Pay close attention to
packages/core/lib/v3/agent/tools/keys.ts- sanitize exception paths to avoid leaking resolved secrets and align variable substitution behavior with the documented tool contract.
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="packages/core/lib/v3/agent/tools/keys.ts">
<violation number="1" location="packages/core/lib/v3/agent/tools/keys.ts:10">
P2: The `valueDescription` tells the LLM that `%variableName%` substitution is available for the `value` field, but `substituteVariables()` is only called in the `method === "type"` branch. If the agent uses `method="press"` with a variable token (e.g., `%apiKey%`), the literal `%apiKey%` string will be passed to `page.keyPress()`, which will either fail or press unintended keys. Scope the variable-substitution hint to `method="type"` only.</violation>
<violation number="2" location="packages/core/lib/v3/agent/tools/keys.ts:45">
P1: Custom agent: **Exception and error message sanitization**
The error path can leak substituted secret values to the LLM. The success path correctly returns the original `%variableName%` token, but if `page.type(actualValue)` throws, the catch block returns the raw `(error as Error).message` without sanitizing out the substituted secret. Wrap the `page.type()` call so that any error in this branch returns a sanitized message that uses the original `value` token, not `actualValue`.</violation>
</file>
Architecture diagram
sequenceDiagram
participant LLM as Agent (LLM)
participant Tool as keysTool
participant Utils as substituteVariables()
participant Browser as Playwright Page
participant Recorder as V3 Recorder
Note over LLM,Tool: Tool initialized with available "variables" map
LLM->>Tool: execute({ method, value, repeat })
alt method === "type"
Tool->>Utils: NEW: substituteVariables(value, variables)
Utils-->>Tool: actualValue (e.g. "secret123")
loop repeat count
Tool->>Browser: CHANGED: page.type(actualValue)
end
Tool->>Recorder: recordAgentReplayStep(value)
Note right of Tool: NEW: instruction uses original token <br/> to prevent secret leakage in logs
Tool-->>LLM: { success: true, value }
Note right of Tool: NEW: returns original token (e.g. %password%) <br/> to LLM context
else method === "press"
loop repeat count
Tool->>Browser: page.press(value)
end
Tool->>Recorder: recordAgentReplayStep(value)
Tool-->>LLM: { success: true, value }
end
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
| const actualValue = substituteVariables(value, variables); | ||
| for (let i = 0; i < times; i++) { | ||
| await page.type(value, { delay: 100 }); | ||
| await page.type(actualValue, { delay: 100 }); |
There was a problem hiding this comment.
P1: Custom agent: Exception and error message sanitization
The error path can leak substituted secret values to the LLM. The success path correctly returns the original %variableName% token, but if page.type(actualValue) throws, the catch block returns the raw (error as Error).message without sanitizing out the substituted secret. Wrap the page.type() call so that any error in this branch returns a sanitized message that uses the original value token, not actualValue.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/agent/tools/keys.ts, line 45:
<comment>The error path can leak substituted secret values to the LLM. The success path correctly returns the original `%variableName%` token, but if `page.type(actualValue)` throws, the catch block returns the raw `(error as Error).message` without sanitizing out the substituted secret. Wrap the `page.type()` call so that any error in this branch returns a sanitized message that uses the original `value` token, not `actualValue`.</comment>
<file context>
@@ -36,14 +39,17 @@ Use method="press" for navigation keys (Enter, Tab, Escape, Backspace, arrows) a
+ const actualValue = substituteVariables(value, variables);
for (let i = 0; i < times; i++) {
- await page.type(value, { delay: 100 });
+ await page.type(actualValue, { delay: 100 });
}
v3.recordAgentReplayStep({
</file context>
| await page.type(actualValue, { delay: 100 }); | |
| try { | |
| await page.type(actualValue, { delay: 100 }); | |
| } catch (e) { | |
| return { | |
| success: false, | |
| error: `Failed to type "${value}"`, | |
| }; | |
| } |
| export const keysTool = (v3: V3, variables?: Variables) => { | ||
| const hasVariables = variables && Object.keys(variables).length > 0; | ||
| const valueDescription = hasVariables | ||
| ? `The text to type, or the key/combo to press (Enter, Tab, Cmd+A). Use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}` |
There was a problem hiding this comment.
P2: The valueDescription tells the LLM that %variableName% substitution is available for the value field, but substituteVariables() is only called in the method === "type" branch. If the agent uses method="press" with a variable token (e.g., %apiKey%), the literal %apiKey% string will be passed to page.keyPress(), which will either fail or press unintended keys. Scope the variable-substitution hint to method="type" only.
Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At packages/core/lib/v3/agent/tools/keys.ts, line 10:
<comment>The `valueDescription` tells the LLM that `%variableName%` substitution is available for the `value` field, but `substituteVariables()` is only called in the `method === "type"` branch. If the agent uses `method="press"` with a variable token (e.g., `%apiKey%`), the literal `%apiKey%` string will be passed to `page.keyPress()`, which will either fail or press unintended keys. Scope the variable-substitution hint to `method="type"` only.</comment>
<file context>
@@ -1,21 +1,24 @@
+export const keysTool = (v3: V3, variables?: Variables) => {
+ const hasVariables = variables && Object.keys(variables).length > 0;
+ const valueDescription = hasVariables
+ ? `The text to type, or the key/combo to press (Enter, Tab, Cmd+A). Use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}`
+ : "The text to type, or the key/combo to press (Enter, Tab, Cmd+A)";
+
</file context>
| ? `The text to type, or the key/combo to press (Enter, Tab, Cmd+A). Use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}` | |
| ? `The text to type (method="type"), or the key/combo to press (method="press") (Enter, Tab, Cmd+A). When using method="type", use %variableName% to substitute a variable value. Available: ${Object.keys(variables).join(", ")}` |
2 participants
Mirrored from external contributor PR #1777 after approval by @pirate.
Original author: @trillville
Original PR: #1777
Approved source head SHA:
9c30883ba601e39d6e17ddfe91dfd72f00d4f7cd@trillville, please continue any follow-up discussion on this mirrored PR. When the external PR gets new commits, this same internal PR will be marked stale until the latest external commit is approved and refreshed here.
Original description
Fixes #1776
Fix
variablesparameter inkeysTool(matchingtypeTool)substituteVariables()beforepage.type()in themethod === "type"branchvariablestokeysToolincreateAgentToolsSummary by cubic
Fixes variable substitution in the keys tool when method="type" so %variableName% tokens are resolved before typing. Aligns behavior with the type tool and prevents secret leakage in tool outputs.
Written for commit 9c30883. Summary will update on new commits. Review in cubic