fix: add variable substitution to keys step during cache replay#1978
fix: add variable substitution to keys step during cache replay#1978a7med3liamin wants to merge 1 commit intobrowserbase:mainfrom
Conversation
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
🦋 Changeset detectedLatest commit: 2149aa2 The changes in this PR will be included in the next version bump. This PR includes changesets to release 4 packages
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
|
This PR is from an external contributor and must be approved by a stagehand team member with write access before CI can run. |
There was a problem hiding this comment.
No issues found across 2 files
Confidence score: 5/5
- Automated review surfaced no issues in the provided summaries.
- No files require special attention.
Architecture diagram
sequenceDiagram
participant Cache as AgentCache
participant Vars as variables.js
participant Ctx as V3Context
participant Page as Playwright Page
Note over Cache,Page: Replay cached "keys" step (method: "type")
Cache->>Cache: Identify "keys" step type
Cache->>Cache: CHANGED: replayAgentKeysStep(step, ctx, variables)
Cache->>Ctx: awaitActivePage()
Ctx-->>Cache: page instance
alt method === "type"
Cache->>Vars: NEW: substituteVariables(text, variables)
Vars-->>Cache: resolvedText (e.g. "2023-01-01")
loop times (default 1)
Cache->>Page: CHANGED: page.type(resolvedText, { delay: 100 })
end
else method === "press"
loop times (default 1)
Cache->>Page: page.keyboard.press(keys)
end
end
Cache-->>Cache: Return step completion
|
sorry for the trouble can you actually merge your two PRs into a single unified pr / this PR. then I will claim it. Our github workflow is a little clunky right now because we have to "claim" external contributor PRs in order to run CI, so it's easier to do fewer PRs that have more changes all in one. |
Two fixes for %variableName% token handling in the keys tool: 1. Live execution: pass variables to keysTool, call substituteVariables() before page.type(), and update the schema description to advertise available variables to the LLM. Records the original token in the cache entry and returns it to the LLM to avoid exposing sensitive values. (Original fix by @trillville in browserbase#1777) 2. Cache replay: pass variables to replayAgentKeysStep in AgentCache and call substituteVariables() before page.type(). Without this, cached keys steps replay by typing literal %variableName% tokens instead of resolved values. Fixes browserbase#1776 Co-Authored-By: trillville <trillville@users.noreply.github.com> Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
004aed7 to
2149aa2
Compare
|
No worries at all @pirate. Done, merged @trillville's live execution fix from #1777 into this PR alongside the cache replay fix. |
…che replay (#1983) Mirrored from external contributor PR #1978 after approval by @pirate. Original author: @a7med3liamin Original PR: #1978 Approved source head SHA: `2149aa265a04dc37154d5a84411f3ab4d1045897` @a7med3liamin, 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 - [x] Check the [documentation](https://docs.stagehand.dev/) for relevant information - [x] Search existing [issues](https://github.com/browserbase/stagehand/issues) to avoid duplicates Fixes #1776 ## Problem The `keys` tool has no variable substitution in either the live execution or cache replay paths. When the agent uses `%variableName%` tokens with the keys tool, the literal token string gets typed instead of the resolved value. ## Fix This PR combines two fixes into one: ### 1. Live execution (original fix by @trillville from #1777) - Accept `variables` parameter in `keysTool` (matching `typeTool`) - Call `substituteVariables()` before `page.type()` in the `method === "type"` branch - Pass `variables` to `keysTool` in `createAgentTools` - Update schema description to advertise available variables to the LLM - Return original token in result to avoid exposing sensitive values to LLM ### 2. Cache replay (new fix) - Import `substituteVariables` in `AgentCache.ts` - Pass `variables` through to `replayAgentKeysStep` - Call `substituteVariables(text, variables)` before `page.type()` in the replay path Without fix #2, cached `keys` steps with `method="type"` replay by typing literal `%variableName%` tokens even when variables are provided, since `replayAgentKeysStep` had no access to the variables map. ## Credit The live execution fix (part 1) is from @trillville's work in #1777/#1813. We merged it here with the cache replay fix per @pirate's request to consolidate into a single PR. <!-- external-contributor-pr:owned source-pr=1978 source-sha=2149aa265a04dc37154d5a84411f3ab4d1045897 claimer=pirate --> <!-- This is an auto-generated description by cubic. --> --- ## Summary by cubic Add variable substitution to the `keys` tool for both live execution and cache replay so `%variableName%` tokens are resolved before typing. This fixes cases where literal tokens were typed and brings parity with the `type` tool. - **Bug Fixes** - Pass `variables` into `keys` and call `substituteVariables()` before `page.type()`; update the input schema to list available variables. - In cache replay, forward `variables` to `replayAgentKeysStep` and substitute before typing to avoid replaying literal tokens. - Record and return the original tokenized value (not the resolved value) to avoid leaking sensitive data. <sup>Written for commit abb3905. Summary will update on new commits. <a href="https://cubic.dev/pr/browserbase/stagehand/pull/1983">Review in cubic</a></sup> <!-- End of auto-generated description by cubic. --> --------- Co-authored-by: Ahmed Ali <a7med3liamin@gmail.com> Co-authored-by: trillville <trillville@users.noreply.github.com> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: Nick Sweeting <git@sweeting.me>
|
The mirrored PR #1983 has been merged into |
Fixes #1776
Problem
The
keystool has no variable substitution in either the live execution or cache replay paths. When the agent uses%variableName%tokens with the keys tool, the literal token string gets typed instead of the resolved value.Fix
This PR combines two fixes into one:
1. Live execution (original fix by @trillville from #1777)
variablesparameter inkeysTool(matchingtypeTool)substituteVariables()beforepage.type()in themethod === "type"branchvariablestokeysToolincreateAgentTools2. Cache replay (new fix)
substituteVariablesinAgentCache.tsvariablesthrough toreplayAgentKeysStepsubstituteVariables(text, variables)beforepage.type()in the replay pathWithout fix #2, cached
keyssteps withmethod="type"replay by typing literal%variableName%tokens even when variables are provided, sincereplayAgentKeysStephad no access to the variables map.Credit
The live execution fix (part 1) is from @trillville's work in #1777/#1813. We merged it here with the cache replay fix per @pirate's request to consolidate into a single PR.