fix: use provided value in fillForm instead of LLM-hallucinated arguments#1805
fix: use provided value in fillForm instead of LLM-hallucinated arguments#1805elliotllliu wants to merge 2 commits intobrowserbase:mainfrom
Conversation
…ents The fillForm tool receives a value parameter for each field but never uses it. When observe() returns results, the LLM hallucinates placeholder values (e.g. "test@example.com") in the arguments array. This causes wrong values to be typed into form fields. Override the observe result arguments with the actual value from the fields array when the method is "fill", ensuring user-provided values are used for form input. Fixes #1789
|
Greptile SummaryThis PR fixes a real bug in the Key changes:
Issues found:
Confidence Score: 3/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Caller as Agent/Caller
participant FillForm as fillFormTool
participant Observe as v3.observe()
participant Act as v3.act()
Caller->>FillForm: fields=[{action, value}, ...]
FillForm->>Observe: instruction (actions only, no values)
Observe-->>FillForm: observeResults[i] (may contain hallucinated arguments)
loop for each observeResults[i]
alt res.method === "fill" && fields[i]?.value
FillForm->>FillForm: Override res.arguments with fields[i].value
end
FillForm->>Act: v3.act(res, actOptions)
Act-->>FillForm: actResult
end
FillForm-->>Caller: { success, actions, playwrightArguments }
Last reviewed commit: e4e65ec |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| for (let i = 0; i < observeResults.length; i++) { | ||
| const res = observeResults[i]; | ||
|
|
||
| // Override LLM-hallucinated arguments with the actual value | ||
| // provided by the caller to prevent placeholder values like | ||
| // "test@example.com" from being typed instead of real input. | ||
| if (res.method === "fill" && fields[i]?.value) { | ||
| res.arguments = [fields[i].value]; | ||
| } |
There was a problem hiding this comment.
Index alignment between observeResults and fields is not guaranteed
The fix pairs observeResults[i] with fields[i] by position, but observe() may not always return exactly one result per field in the same order. If the LLM inserts intermediate steps (e.g., a "click" to focus a field before a "fill"), the indices shift and the wrong value gets applied to the wrong field.
Example of a broken case:
fields = [
{ action: "type email", value: "user@example.com" },
{ action: "type password", value: "secret123" },
]
observeResults = [
{ method: "click", ... }, // i=0 → skipped (correct)
{ method: "fill", ... email }, // i=1 → fields[1].value = "secret123" ← WRONG
{ method: "fill", ... password }, // i=2 → fields[2]?.value = undefined ← no override
]
A more robust approach would be to count only the fill results seen so far to determine which fields entry to use:
let fillIndex = 0;
for (let i = 0; i < observeResults.length; i++) {
const res = observeResults[i];
// Override LLM-hallucinated arguments with the actual value
// provided by the caller to prevent placeholder values like
// "test@example.com" from being typed instead of real input.
if (res.method === "fill") {
if (fields[fillIndex]?.value) {
res.arguments = [fields[fillIndex].value];
}
fillIndex++;
}
// ... rest of loop
}This way, the first fill result always maps to fields[0], the second to fields[1], etc., regardless of any interleaved non-fill actions.
| // Override LLM-hallucinated arguments with the actual value | ||
| // provided by the caller to prevent placeholder values like | ||
| // "test@example.com" from being typed instead of real input. | ||
| if (res.method === "fill" && fields[i]?.value) { |
There was a problem hiding this comment.
Empty string values are silently ignored
The condition fields[i]?.value is falsy when value is an empty string "". This means a caller that explicitly wants to clear a field (e.g., { action: "clear the search box", value: "" }) would fall back to the LLM's hallucinated argument instead of using the intended empty string.
Use a more explicit check:
| if (res.method === "fill" && fields[i]?.value) { | |
| if (res.method === "fill" && fields[i]?.value !== undefined) { |
![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 1 file
Confidence score: 3/5
- Potential user-impacting bug in
packages/core/lib/v3/agent/tools/fillform.ts: index-based mapping betweenfields[i].valueandobserveResults[i]can misassign values if observe output order/count differs. - Valid empty-string inputs are dropped in
packages/core/lib/v3/agent/tools/fillform.tsdue to a truthy guard, which could prevent intended overrides; this drives the moderate risk score. - Pay close attention to
packages/core/lib/v3/agent/tools/fillform.ts- ensure field/value mapping and empty-string handling are correct.
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/fillform.ts">
<violation number="1" location="packages/core/lib/v3/agent/tools/fillform.ts:65">
P2: Empty-string field values are ignored due to a truthy guard, so valid caller input (`""`) does not override observed fill arguments.</violation>
<violation number="2" location="packages/core/lib/v3/agent/tools/fillform.ts:65">
P1: Index-based mapping of `fields[i].value` to `observeResults[i]` can misassign form values when observe output order/count differs.</violation>
</file>
Since this is your first cubic review, here's how it works:
- cubic automatically reviews your code and comments on bugs and improvements
- Teach cubic by replying to its comments. cubic learns from your replies and gets better over time
- Add one-off context when rerunning by tagging
@cubic-dev-aiwith guidance or docs links (includingllms.txt) - Ask questions if you need clarification on any suggestion
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Replace truthy check (fields[i]?.value) with != null so that empty-string values are preserved as valid form input - Only apply index-based value overrides when observeResults.length matches fields.length to prevent misassignment when counts diverge
|
Thanks for the review @cubic-dev-ai — both issues were valid. Fixed in the latest push:
|
1 participant
Summary
Fixes #1789 — The
fillFormtool now uses the actualvalueparameter instead of LLM-hallucinated placeholder values.Problem
The
fillFormtool receives{ action, value }for each field, but only passesactiontoobserve(). The LLM then hallucinates placeholder values liketest@example.comin the observe results, which get typed into form fields instead of the real values.This breaks login forms, 2FA flows, search queries, and any workflow where specific values matter.
Fix
After
observe()returns results, override theargumentsarray with the actualvaluefrom thefieldsparameter whenmethod === "fill":Changes
packages/core/lib/v3/agent/tools/fillform.tsSummary by cubic
fillFormnow uses the caller-providedvaluefor each field instead of hallucinated placeholders fromobserve(). Preserves empty-string inputs and prevents wrong values in login, 2FA, and search. Fixes #1789.observe()arguments withfields[i].valueusing!= nullchecks.observeResults.length === fields.lengthto avoid misassignment.Written for commit 7db7b4f. Summary will update on new commits. Review in cubic