diff --git a/docs/memory/cli/index.md b/docs/memory/cli/index.md index 22b7f6f1..16c11f15 100644 --- a/docs/memory/cli/index.md +++ b/docs/memory/cli/index.md @@ -10,3 +10,4 @@ description: "CLI orchestration, report writing, HTML generation, and the host p | [host-preflight](host-preflight.md) | hostPreflight.ts — the host environment checks the CLI runs before a session, and the platform-branched command-on-PATH probe behind them: `resolveCommandPath` locates with `where` on win32 (first output line) and `which` elsewhere, through defaulted process-boundary overrides that make both branches test-reachable without stubbing globals. | | [report-writer](report-writer.md) | ReportWriter (packages/cli/src/reportWriter.ts) — the run directory it emits (runner.log, input/, tests//, summary.json, run.json), the secret-redaction contract every write path crosses, device-log copy-then-redact, first-failure precedence, and the emitted-JSON key-omission contract that makes an absent optional field part of the schema | | [session-runner](session-runner.md) | sessionRunner.ts — prepareTestSession/executeTestOnSession/runGoal: the per-call ExecutionSessionState every phase records its acquisition into, the single finally that releases whatever is still held, the recording and device-log capture lifecycles, the guarded (never `!`-asserted) install-override preconditions and model-owned platform derivation, and the CLI's two sanctioned dependency seams (TestSessionDeps and testRunnerDependencies) | +| [test-compiler](test-compiler.md) | compileTestObjective (packages/cli/src/testCompiler.ts) and the secret-value exposure chain it anchors: only `${variables.*}` is substituted, so the compile-time guarantee covers the compiled objective alone — once typed, a resolved secret reaches the provider through the captured hierarchy and screenshots. Prompt text is redacted per input at the AIAgent seam when runtime bindings are wired; screenshots, app-rendered transformations of a value, and INFO planner output are not. | diff --git a/docs/memory/cli/report-writer.md b/docs/memory/cli/report-writer.md index ecc2c3f6..317a4772 100644 --- a/docs/memory/cli/report-writer.md +++ b/docs/memory/cli/report-writer.md @@ -21,6 +21,8 @@ Every value that reaches a file crosses `redactResolvedValue(value, bindings)` ( The environment snapshots carry reference forms only — `env.snapshot.yaml` mirrors the *config* `secrets` map (the `${ENV_VAR}` placeholder form) and `env.json` carries `secretReferences` (key plus env-var name). No resolved secret value is written anywhere beneath the run directory, and `reportWriter.test.ts` holds that as a whole-`runDir` sweep over every emitted file after a full lifecycle rather than as per-site assertions. +The contract is bounded in two ways worth naming, because the sweep's breadth invites reading it as global. It covers **write paths only** — the LLM prompt path is a separate seam, redacted per input inside `AIAgent` when runtime bindings are wired, and stdout INFO output crosses neither ([/cli/test-compiler.md](/cli/test-compiler.md)). And the sweep is a **byte-level** check on emitted files, so `tests//screenshots/NNN.jpg` passes it while still showing a typed secret in its pixels: the report carries secret-bearing images even though it carries no secret strings. + ## Device Log Artifact Flow The `_copyLogArtifact(testId, deviceLog, bindings)` private method handles device logs: diff --git a/docs/memory/cli/test-compiler.md b/docs/memory/cli/test-compiler.md new file mode 100644 index 00000000..cdae4686 --- /dev/null +++ b/docs/memory/cli/test-compiler.md @@ -0,0 +1,172 @@ +--- +type: memory +description: "compileTestObjective (packages/cli/src/testCompiler.ts) and the secret-value exposure chain it anchors: only `${variables.*}` is substituted, so the compile-time guarantee covers the compiled objective alone — once typed, a resolved secret reaches the provider through the captured hierarchy and screenshots. Prompt text is redacted per input at the AIAgent seam when runtime bindings are wired; screenshots, app-rendered transformations of a value, and INFO planner output are not." +--- +# Test Objective Compilation (cli) + +**Domain**: cli + +## Overview + +`compileTestObjective` (`packages/cli/src/testCompiler.ts`) renders a `TestDefinition` into the +planner objective: named sections (`Test Name`, `Test Path`, `Description`, `Setup`, `Steps`, +`Expected State`) with `${variables.*}` references interpolated, plus a trailing `Execution Rules` +block that teaches the model to treat a `${secrets.*}` placeholder as a logical token and echo it +verbatim. `VARIABLE_REFERENCE_PATTERN` never matches `${secrets.*}`, which is the repo's one +compile-time secret guarantee — and that guarantee is a single link in a longer chain. This file +carries the whole chain: what the compile-time scope covers, how a resolved secret value reaches the +model provider at runtime anyway, where redaction is wired, and what stays exposed. + +## Requirements + +### Requirement: The compile-time guarantee covers the compiled objective and nothing else +`VARIABLE_REFERENCE_PATTERN` MUST match `${variables.*}` only, so `${secrets.*}` tokens survive +compilation as literal text and no secret VALUE enters the compiled test artifact or the objective +portion of any prompt through placeholder substitution. Widening the pattern to secrets is the one +**durable** leak — values would be baked into the compiled artifact and into every planner objective +— which is why the guard comment on the pattern is load-bearing and MUST NOT be deleted as +redundant. + +The guarantee is exact, not general: an unresolved `${secrets.KEY}` (absent from +`bindings.secrets`) also stays a literal token, while a `${variables.*}` value that happens to equal +a secret value still lands in the compiled text, because this pattern cannot tell the two apart. + +#### Scenario: a secret placeholder survives compilation +- **GIVEN** a test whose step reads `Enter ${secrets.TOKEN} on the login screen` +- **WHEN** `compileTestObjective` renders it +- **THEN** the compiled objective still contains the literal `${secrets.TOKEN}` token, and the + appended `Execution Rules` instruct the model to echo it exactly + +### Requirement: A typed secret reaches the model provider through the runtime prompt path +Once `ActionExecutor._executeType` or `_executeDeeplink` resolves a placeholder through +`resolveRuntimePlaceholders` (`packages/common/src/repoPlaceholders.ts`) and the value lands on +screen, the next captured accessibility hierarchy carries it, and both prompt legs can emit it: + +- **Grounder leg** — `Hierarchy.toPromptElementsForGrounder` emits the typed field's `text`, + `hintText` and `error` verbatim ([/common/hierarchy.md](/common/hierarchy.md)). +- **Planner leg** — `toPromptElementsForPlanner` carries only `contentDesc`, and only for + image/button-class nodes, so it surfaces a value only where one was rendered into accessibility + text. +- **Screenshots** — the planner's pre/post-action screenshots and the grounder's screenshot show the + same screen regardless of either element serialization. + +Android password fields often mask text at the accessibility layer, but that is app- and +platform-dependent and MUST NOT be relied on: a plain-text field holding an API key, OTP or token +carries the value verbatim. The deeplink leg has no exposure of its own — a secret resolved into an +opened URL re-enters through whatever the resulting screen renders, so it inherits exactly the +hierarchy and screenshot status above (the span detail logs the unresolved `rawDeeplink`). + +### Requirement: Prompt text is redacted per input at the AIAgent seam +`AIAgent` (`packages/goal-executor/src/ai/AIAgent.ts`) accepts optional `bindings: RuntimeBindings`, +wired from `createSessionExecutor` (`packages/cli/src/sessionRunner.ts`) as +`config.runtimeBindings`. When bindings are present, `_buildPlannerPrompt` and +`_buildGrounderPrompt` MUST replace exact occurrences of resolved secret values with their +`${secrets.KEY}` placeholder — via `redactResolvedValue` from `@finalrun/common`, whose +longest-value-first alternation is reused rather than reimplemented — applying it: + +- to every free-text input individually (`testObjective`, `act`, `history`, each `remember` entry, + `preContext`, `appKnowledge`); +- to every string field of every element record and `availableApps` record, **before** + `JSON.stringify`; +- to the DEBUG detail blobs (`_detailPlannerRequest`, `_detailGrounderRequest`) and the INFO-level + grounder `act` output (`formatGrounderRequest`, `_summarizeGrounderRequest`), which print element + and prompt content to a console that no write-path redaction covers. + +There MUST be no redaction pass over assembled prompt text. The `Hierarchy` MUST NOT be mutated — +only serialized copies are rewritten — so index-based grounding (`flattenedHierarchy[idx]` → bounds +→ tap point) still resolves against the real on-screen values. Absent bindings, every path is a +strict no-op and prompt assembly is byte-identical. + +Seven tests in `packages/goal-executor/src/ai/test/AIAgent.test.ts` pin this, including one for each +failure mode a pass over assembled text would introduce (see Design Decisions). + +#### Scenario: the typed field is redacted and still locatable +- **GIVEN** bindings `{ secrets: { PASSWORD: 'hunter2-secret' } }` and a captured hierarchy whose + typed field has `text: 'hunter2-secret'` +- **WHEN** `AIAgent` builds the grounder prompt +- **THEN** the emitted `ui_elements` carries `${secrets.PASSWORD}` and never the raw value, while + `index`, `bounds`, `id` and `class` are unchanged and `flattenedHierarchy[idx]` still returns the + original text and bounds + +#### Scenario: a numeric secret colliding with a coordinate leaves structure intact +- **GIVEN** `secrets.PIN = '1080'` and an element with `bounds: [0, 0, 1080, 240]` +- **WHEN** the grounder prompt is built +- **THEN** the emitted `ui_elements` still parses as JSON, `bounds` is `[0, 0, 1080, 240]`, and only + the field whose string value equals the secret becomes `${secrets.PIN}` + +### Requirement: Residual exposure is stated, not closed +Prompts, provider-side logs and report artifacts MUST still be treated as secret-bearing. What +remains exposed after prompt-text redaction: + +- **Screenshots** — sent to the provider unredacted, carried in the `llmCall` observability records, + and written to the run report as `tests//screenshots/NNN.jpg`. Masking needs visual + field-region detection; dropping them would blind the planner. +- **App-rendered transformations** — a value the app truncates, reformats or partially masks is no + longer an exact match and passes through. +- **Structural strings** — exact matching also rewrites an element's `id`, `class` or `contentDesc` + when its content equals a secret value. That degrades the text the grounder reads; it cannot + corrupt the emitted JSON, because `index` and `bounds` are non-string fields. +- **INFO planner reasoning** — `formatPlannerReasoning` prints the planner's `thought`, `action` and + `reason` to stdout raw. + +No full-prompt logging path exists: the detail blobs carry prompt lengths and three-element +excerpts, and `runner.log` writes cross `ReportWriter`'s redaction +([/cli/report-writer.md](/cli/report-writer.md)). Enabling full prompt logging, or treating provider +logs as secret-free, is exactly the conclusion the guard comment forbids. + +## Design Decisions + +### The guarantee comment states its scope and its residuals +**Decision**: The comment on `VARIABLE_REFERENCE_PATTERN` names its own boundary — compile-time +scope only — then states the runtime exposure with the concrete chain +(`ActionExecutor._executeType` → captured hierarchy → `Hierarchy.toPromptElements*` → `AIAgent` +prompt assembly), which redaction seam covers what, and which residuals stay open. It describes the +shipped state, never an aspirational one. +**Why**: A confident wrong guarantee is worse than no comment: a reader who believes the invariant +is enforced end-to-end makes decisions premised on it — enabling prompt logging, treating provider +logs as secret-free, skipping redaction on a new prompt surface — and each is a silent disclosure. +Because the comment exists so a future reader does not delete the control, its accuracy standard is +the highest in the file. +**Rejected**: A short unqualified claim that secret values never enter LLM prompts — true only of +the compiled objective, and false in the direction that costs the most. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +### Prompt-path redaction lives at the AIAgent seam, per input, before serialization +**Decision**: Redaction sits in `AIAgent`, applied to each free-text input and each record string +field before assembly and `JSON.stringify`, with no pass over the assembled prompt and no mutation +of the `Hierarchy`. +**Why**: This is the only seam where both the hierarchy and the runtime bindings are in scope, and +it covers every hierarchy-carrying prompt regardless of caller — planner, all grounder features, +post-action hierarchy. Per-input application is what makes redaction structure-blind: it never sees +serialized JSON, so it cannot rewrite it. Leaving the `Hierarchy` untouched keeps device-side +grounding provably unaffected, since taps resolve through `flattenedHierarchy[idx]` rather than +through prompt text. +**Rejected**: (a) redaction inside `Hierarchy.toPromptElements*` — `packages/common` has no access +to runtime bindings; (b) a pass over the assembled prompt text — it fails in both directions, +rewriting structural values that happen to equal a secret (a numeric PIN equal to a bounds +coordinate corrupts the `ui_elements` JSON) and missing exact secrets that serialization escaped +(`"` → `\"`, `\` → `\\`); (c) no mitigation at all — the structural half of the risk is +unit-provable, so documentation alone would under-deliver. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +### A redacted value becomes its placeholder, not an opaque mask +**Decision**: `redactResolvedValue` substitutes the `${secrets.KEY}` token, the same logical token +the compiled objective's `Execution Rules` already define. +**Why**: A verify-flow that reads "the field now contains `${secrets.PASSWORD}`" reads a token whose +semantics the model was already taught, so the redacted prompt stays reasonable-about rather than +merely censored. +**Rejected**: An opaque mask (`***`) — carries no meaning the Execution Rules define, and gives the +model no way to reason about what the field holds. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +### Screenshots stay unredacted and the residual is documented +**Decision**: No screenshot masking is attempted, in the prompt path or in the report. The exposure +is recorded in the source comment and here instead. +**Why**: Masking requires OCR-grade field-region detection — expensive and unreliable — and blurring +or omitting the screenshot destroys the planner's primary signal. An honest residual leaves the next +maintainer able to reason about the actual risk; a partial fix presented as closure recreates the +original defect at larger scale. `ReportWriter`'s whole-`runDir` sweep is a byte-level check, so it +proves no secret *string* reaches the report and can say nothing about pixels. +**Rejected**: (a) OCR/region-based masking — cost and reliability; (b) omitting screenshots from +prompts or reports — blinds the planner and removes the report's most useful failure evidence. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment diff --git a/docs/memory/common/hierarchy.md b/docs/memory/common/hierarchy.md index 43dee4f0..e7f15921 100644 --- a/docs/memory/common/hierarchy.md +++ b/docs/memory/common/hierarchy.md @@ -18,6 +18,13 @@ failure at runtime rather than as a test failure, so the parse contract is pinne mutation-verified characterization tests in `packages/common/src/models/test/Hierarchy.test.ts` (44 tests, green against the pre-refactor source — see [/ci/pr-quality-gate.md](/ci/pr-quality-gate.md)). +Node text reaches the model provider verbatim from here — a field holding a typed secret included. +Redaction for that is downstream and generic over string fields: `AIAgent` rewrites exact resolved +secret values in the records these two builders return, before serializing them, whenever runtime +bindings are wired ([/cli/test-compiler.md](/cli/test-compiler.md)). It never mutates the +`Hierarchy`, which keeps holding the real on-screen values — so this file has no bindings access and +needs none. + ## Requirements ### Requirement: Two parse paths, deliberately not equivalent diff --git a/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.history.jsonl b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.history.jsonl new file mode 100644 index 00000000..7896343d --- /dev/null +++ b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.history.jsonl @@ -0,0 +1,18 @@ +{"action":"enter","driver":"fab-new","event":"stage-transition","stage":"intake","ts":"2026-07-31T10:34:03Z"} +{"args":"Fix factually wrong secret-leak guarantee comment in testCompiler.ts and assess/mitigate the real prompt-path secret leak it conceals","cmd":"fab-new","event":"command","ts":"2026-07-31T10:34:03Z"} +{"delta":"+0.0","event":"confidence","score":4.5,"trigger":"calc-score","ts":"2026-07-31T10:36:18Z"} +{"cmd":"fab-new","event":"command","ts":"2026-07-31T10:36:49Z"} +{"delta":"+0.0","event":"confidence","score":4.5,"trigger":"calc-score","ts":"2026-07-31T10:39:48Z"} +{"cmd":"fab-fff","event":"command","ts":"2026-07-31T10:40:22Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-31T10:40:34Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-31T10:41:47Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-31T10:55:52Z"} +{"event":"review","result":"failed","ts":"2026-07-31T11:07:17Z"} +{"action":"re-entry","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-31T11:07:17Z"} +{"action":"re-entry","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-31T11:14:29Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"hydrate","ts":"2026-07-31T11:23:33Z"} +{"event":"review","result":"passed","ts":"2026-07-31T11:23:33Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-31T11:29:29Z"} +{"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-07-31T11:32:11Z"} +{"cmd":"git-pr-review","event":"command","ts":"2026-07-31T11:33:52Z"} +{"event":"review","result":"passed","ts":"2026-07-31T11:49:39Z"} diff --git a/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.status.yaml b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.status.yaml new file mode 100644 index 00000000..b1b47c51 --- /dev/null +++ b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.status.yaml @@ -0,0 +1,52 @@ +id: gzzl +name: 260731-gzzl-fix-secret-prompt-leak-comment +created: 2026-07-31T10:34:03Z +created_by: droid-ash +change_type: fix +issues: [] +progress: + intake: done + apply: done + review: done + hydrate: done + ship: done + review-pr: done +plan: + generated: true + task_count: 6 + acceptance_count: 13 + acceptance_completed: 13 +confidence: + certain: 4 + confident: 3 + tentative: 0 + unresolved: 0 + score: 4.5 + fuzzy: true + dimensions: + signal: 83.6 + reversibility: 80.7 + competence: 85.0 + disambiguation: 80.7 +stage_metrics: + intake: {started_at: "2026-07-31T10:34:03Z", driver: fab-new, iterations: 1, completed_at: "2026-07-31T10:40:34Z"} + apply: {started_at: "2026-07-31T11:07:17Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T11:14:29Z"} + review: {started_at: "2026-07-31T11:14:29Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T11:23:33Z"} + hydrate: {started_at: "2026-07-31T11:23:33Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T11:29:29Z"} + ship: {started_at: "2026-07-31T11:29:29Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T11:32:11Z"} + review-pr: {started_at: "2026-07-31T11:32:11Z", driver: git-pr, iterations: 1, completed_at: "2026-07-31T11:49:39Z"} +prs: + - https://github.com/droid-ash/finalrun-agent/pull/175 +true_impact: + added: 889 + deleted: 35 + net: 854 + tests: + added: 215 + deleted: 1 + net: 214 + computed_at: "2026-07-31T11:32:11Z" + computed_at_stage: ship +summary: 'Documented the secret-value exposure chain: new cli/test-compiler (compile-time scope, runtime prompt path, AIAgent redaction seam, residuals), scope carve-outs on cli/report-writer, downstream-redaction pointer on common/hierarchy' +# true_impact: lazily created on first stage-finish that computes it (no placeholder here). +last_updated: 2026-07-31T11:49:39Z diff --git a/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md new file mode 100644 index 00000000..6c94d4b6 --- /dev/null +++ b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md @@ -0,0 +1,102 @@ +# Intake: Fix Secret-Leak Guarantee Comment and Assess the Prompt-Path Secret Leak + +**Change**: 260731-gzzl-fix-secret-prompt-leak-comment +**Created**: 2026-07-31 + +## Origin + +One-shot `/fab-new` invocation, originating from an independent adversarial review of PRs #168–#172 (the comment in question was written by the comment-content sweep, PR #171 / change `260731-vxq1-comment-content-sweep`). User's raw input: + +> Fix a security comment that is factually wrong, and assess the real leak it conceals. THE COMMENT: packages/cli/src/testCompiler.ts lines 6 to 7 claims that secret VALUES never enter the LLM prompt, the model provider logs, or compiled test artifacts. That is TRUE for the compiled objective and FALSE for the prompt. Verified chain: ActionExecutor.ts line 365 resolves a secrets placeholder and types the real value into a field; on the next action Hierarchy.toPromptElementsForGrounder emits node.text VERBATIM at packages/common/src/models/Hierarchy.ts line 215; AIAgent.ts lines 500 to 502 sends that to the provider unredacted; AIAgent.ts line 417 additionally attaches a post-action screenshot of the same screen. A grep for redact across packages/goal-executor/src/ai returns nothing — redactResolvedValue exists but is wired only into reportWriter.ts and ActionExecutor._redactRuntimeString for spans and errors, never the prompt path. REQUIRED, and the part that must ship: rewrite the comment so it states what is actually guaranteed. Scope the claim to the compiled objective and state plainly that resolved secret values DO reach the model provider via the post-typing accessibility hierarchy and via screenshots. A confident wrong guarantee is worse than no comment because it tells the next maintainer the invariant is already enforced and it is safe to enable full prompt logging. SECOND, assess whether the leak itself can be mitigated, and use real judgement rather than forcing a fix: redacting the field text in the hierarchy may break the grounder ability to locate or verify the element it just typed into, and a screenshot cannot be redacted cheaply, so full remediation may be impossible. If it is, say so explicitly and document the residual exposure accurately instead of pretending. If a safe partial mitigation exists — for example redacting only exact secret-value matches in node text while leaving structure intact, or making prompt logging opt-in with a warning — propose it, weigh it against grounding accuracy, and implement only if you can show grounding still works. Do NOT overstate the guarantee in whatever new comment you write. Context: this came out of an independent adversarial review of PRs #168 to #172. Do NOT commit or git add fab/backlog.md under any circumstance — it is intentionally untracked scratch and the user wants it kept that way; note that the git-pr expected-area guard would otherwise stage untracked files under fab/. + +Key decisions from the invocation: the comment rewrite is REQUIRED and must ship; the leak mitigation is judgment-delegated — assess honestly, implement only if a safe partial mitigation demonstrably preserves grounding, and if full remediation is impossible, say so and document the residual exposure accurately. + +## Why + +**The pain point.** The comment at `packages/cli/src/testCompiler.ts:3-11` (written by PR #171's security-control documentation pass) claims that because `VARIABLE_REFERENCE_PATTERN` never matches `${secrets.*}` tokens, "secret VALUES never enter the LLM prompt, the model provider's logs, or compiled test artifacts." That guarantee is true only for the **compiled objective text**. It is false for the prompt as a whole: once `ActionExecutor._executeType` resolves a `${secrets.*}` placeholder (`resolveRuntimePlaceholders`, `packages/goal-executor/src/ActionExecutor.ts:363-365`) and types the real value into a field, the very next planner/grounder call captures a fresh accessibility hierarchy in which that field's `node.text` **is the secret value**. `Hierarchy.toPromptElementsForGrounder` (`packages/common/src/models/Hierarchy.ts:214-215`) and `toPromptElementsForPlanner` emit `node.text` verbatim; `AIAgent.ts` serializes those elements into the prompt (`~500-502` grounder, `~410-421` planner) and additionally attaches screenshots (`request.screenshot` / `request.postActionScreenshot`) showing the same screen. Nothing in `packages/goal-executor/src/ai/` redacts anything — `redactResolvedValue` is wired only into `reportWriter.ts` and `ActionExecutor._redactRuntimeString` (spans/errors/report artifacts), never the prompt path. All of this was independently re-verified at intake. + +**The consequence of not fixing.** A confident wrong guarantee is worse than no comment: it tells the next maintainer the invariant is already enforced end-to-end, so decisions premised on it (e.g., enabling full prompt logging, treating provider-side logs as secret-free, skipping redaction when adding a new prompt surface) become silent secret-disclosure incidents. The comment is also load-bearing documentation of a "security control" (it was written expressly so a future reader doesn't delete the control), so its accuracy standard is highest. + +**Why this approach.** Fix the documentation to state exactly what is guaranteed (compile-time scope) and what is not (runtime prompt path), then assess mitigation with real engineering judgment rather than forcing a fix that breaks grounding. Masking the typed value in the hierarchy may prevent the grounder/planner from locating or verifying the field it just typed into (e.g., verifying "the field now contains the password" or re-focusing the field by its content); a screenshot cannot be cheaply redacted (would require OCR/region detection). So the honest outcome may be: accurate comment + partial mitigation (or none) + accurately documented residual exposure. Pretending full remediation exists would recreate the original defect at a bigger scale. + +## What Changes + +### 1. Comment rewrite in `packages/cli/src/testCompiler.ts` (REQUIRED — must ship) + +Current text (lines 3–11): + +```ts +// Secret-leak guard: this pattern deliberately matches ONLY ${variables.*} +// tokens, never ${secrets.*}. Secret placeholders are left as literal tokens in +// the compiled prompt — the Execution Rules appended below instruct the model to +// echo them verbatim — so secret VALUES never enter the LLM prompt, the model +// provider's logs, or compiled test artifacts. Secrets are substituted only +// downstream at the point of use (resolveRuntimePlaceholders in +// packages/common/src/repoPlaceholders.ts, called from ActionExecutor when +// typing or opening a deeplink). Widening this pattern to secrets would leak +// their values into every plan/grounder call. +``` + +Replace with a comment that: + +- **Scopes the guarantee to what this file controls**: `${secrets.*}` tokens are never substituted into the *compiled objective* — so the compiled test text, and every prompt containing only the objective, carries placeholders, not values. Keep the "widening this pattern would leak values into every plan/grounder call" warning — that part is true and load-bearing. +- **States the runtime exposure plainly**: once ActionExecutor resolves a placeholder and types the real value into a field, subsequent planner/grounder calls DO send the secret value to the model provider — via the captured accessibility hierarchy (`node.text` of the typed field, emitted verbatim by `Hierarchy.toPromptElementsForGrounder`/`ForPlanner`) and via screenshots of the same screen. Name the concrete files so the next maintainer can follow the chain (`ActionExecutor._executeType` → `Hierarchy.toPromptElements*` → `AIAgent` request assembly). +- **States the redaction wiring accurately**: `redactResolvedValue` protects report artifacts, spans, and error strings (reportWriter.ts, `ActionExecutor._redactRuntimeString`) — not the prompt path. +- **Warns against the specific premature conclusion**: it is NOT safe to assume prompts are secret-free (e.g., when enabling prompt logging or provider-side log retention). +- **Does not overstate** whatever mitigation (if any) ships in this same change — the comment must describe the post-change state exactly, including residual exposure. + +If mitigation from §3 ships, the comment (and any comment added at the mitigation site) must reflect the mitigated + residual state, not an aspirational one. + +### 2. Leak assessment (REQUIRED — a written, honest verdict) + +Produce an explicit assessment, carried in the plan/PR body and reflected in the rewritten comment(s) and affected memory: + +- **Hierarchy leg**: after `_executeType` types a resolved secret, the next capture's `node.text` (and possibly `hintText`/`error` fields) contains the value; both grounder and planner element serializations emit it. Password fields on Android often mask text at the accessibility layer (`•••`), but this is app/platform-dependent and NOT a guarantee — plain-text fields (API keys, OTP codes, tokens typed into non-password inputs) leak verbatim. +- **Screenshot leg**: `request.screenshot` / `request.postActionScreenshot` capture the same screen; masking would require visual detection of the field region — not cheaply or reliably achievable. Treat as residual exposure unless evidence otherwise. +- **Deeplink leg**: `resolveRuntimePlaceholders` is also called for deeplink opens; a secret embedded in a URL may subsequently appear in address bars / on-screen text captured by hierarchy and screenshots. Assess and document; same residual logic. +- **Verdict shape**: full remediation is likely impossible (screenshot leg); say so explicitly if confirmed, and document the residual exposure accurately. + +### 3. Partial mitigation (CONDITIONAL — implement only if grounding demonstrably survives) + +Candidate mitigations to weigh, in the user's suggested space: + +- **(a) Exact-match redaction in the element serialization path**: in `Hierarchy.toPromptElementsForGrounder`/`ForPlanner` (or at the AIAgent assembly seam, which has access to runtime bindings), replace exact occurrences of resolved secret *values* in `text`/`hintText`/`error` fields with the corresponding `${secrets.KEY}` placeholder (reusing `redactResolvedValue` semantics — longest-value-first substitution already exists in `packages/common/src/repoPlaceholders.ts`). Structure (index, bounds, class, id, isEditable, isFocused) stays intact, so element *location* should be unaffected; the risk is *verification* steps ("confirm the field shows X") and the model re-typing from what it reads. The plumbing question is real: `Hierarchy` (packages/common) has no knowledge of runtime bindings today — redaction likely belongs where both the hierarchy and the bindings are in scope (goal-executor's prompt assembly), not inside `Hierarchy` itself. +- **(b) No silent prompt logging**: verify whether any prompt-logging/tracing path exists today; if one exists or is trivially enabled, make it opt-in with an explicit secrets warning. If none exists, the rewritten comment's warning is the deliverable. +- **Gate for implementing (a)**: only if it can be shown that grounding still works — at minimum the existing goal-executor/grounder test suites pass, plus a targeted test showing an element whose `text` was redacted to a placeholder is still locatable by index/bounds/id and that typing-then-verifying flows are not broken. If that cannot be shown within this change's scope, ship the assessment + comment only, and record mitigation as consciously deferred with the reasoning. +- **Screenshots**: no mitigation attempted — document as residual exposure (cannot be redacted cheaply; blurring/omitting the screenshot would blind the planner). + +### 4. Constraint: `fab/backlog.md` must never be staged or committed + +`fab/backlog.md` is intentionally untracked scratch. The `/git-pr` expected-area guard would otherwise stage untracked files under `fab/` — this change's ship step MUST NOT `git add` it under any circumstance (stage specific paths explicitly rather than `git add fab/`). + +## Affected Memory + +- `cli/test-compiler`: (new) The compile-time secret-placeholder guarantee in `testCompiler.ts` — its actual scope (compiled objective only), the runtime prompt-path exposure it does NOT cover (post-typing hierarchy `node.text`, screenshots), where redaction IS wired (report artifacts, spans, errors) vs. is not (prompt assembly), and the residual-exposure verdict from this change's assessment. +- `common/hierarchy`: (modify — only if mitigation (a) lands in or around `toPromptElements*`) Note the redaction seam and that emitted `text`/`hintText`/`error` may carry placeholder substitutions; if no code change lands in common, no modification needed. +- `cli/report-writer`: (modify) One-line cross-reference: the secret-redaction contract it documents covers write paths only, not the LLM prompt path — pointer to `cli/test-compiler` for the prompt-path exposure. + +## Impact + +- `packages/cli/src/testCompiler.ts` — comment rewrite (no behavior change). +- `packages/goal-executor/src/ai/AIAgent.ts`, `packages/goal-executor/src/ActionExecutor.ts` — mitigation seam candidates (conditional); at minimum referenced by the new comment. +- `packages/common/src/models/Hierarchy.ts`, `packages/common/src/repoPlaceholders.ts` — conditional mitigation touch-points (`redactResolvedValue` reuse); at minimum referenced. +- Tests: existing suites in `packages/goal-executor` and `packages/cli` must stay green; conditional mitigation requires a new targeted test proving grounding survives redaction. +- No API, dependency, or CI changes. Git hygiene constraint on `fab/backlog.md` at ship time. + +## Open Questions + +- None blocking. The one genuine judgment call — whether to implement mitigation (a) — was explicitly delegated by the user with a decision criterion (implement only if grounding demonstrably still works; otherwise document honestly). Apply resolves it against that criterion and records the outcome. + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Certain | The comment rewrite ships regardless of the mitigation outcome, scopes the guarantee to the compiled objective, and states plainly that resolved secret values DO reach the provider via post-typing hierarchy text and screenshots | Explicit user requirement ("REQUIRED, and the part that must ship"); exact required content enumerated in the invocation | S:95 R:90 A:95 D:95 | +| 2 | Certain | The leak chain as described is real: `_executeType` resolves placeholders (ActionExecutor.ts:363-365), `toPromptElementsForGrounder` emits `node.text` verbatim (Hierarchy.ts:214-215), AIAgent sends hierarchy + screenshots unredacted, and `grep redact packages/goal-executor/src/ai` returns nothing | Independently re-verified at intake by reading each cited site; user's line numbers matched | S:90 R:90 A:95 D:95 | +| 3 | Confident | Mitigation is decided by apply against the user's stated gate: implement exact-value redaction only if grounding demonstrably survives (tests + targeted proof); otherwise ship assessment + honest documentation of residual exposure, and treat screenshots as unmitigable within this change | User explicitly delegated with criteria ("use real judgement rather than forcing a fix… implement only if you can show grounding still works") | S:85 R:65 A:75 D:70 | +| 4 | Confident | If redaction is implemented, it belongs at a seam with access to runtime bindings (goal-executor prompt assembly), not inside `Hierarchy` (packages/common), which has no bindings today; `redactResolvedValue`'s longest-first substitution is reused rather than reimplemented | Verified `Hierarchy` has no bindings access; reuse mandated by code-quality anti-pattern "duplicating existing utilities" | S:70 R:70 A:80 D:70 | +| 5 | Certain | `fab/backlog.md` is never staged or committed; ship stages explicit paths instead of directory-level `git add fab/` | Explicit user constraint with stated reason (git-pr expected-area guard would stage untracked fab/ files) | S:95 R:75 A:90 D:90 | +| 6 | Confident | Affected memory: new `cli/test-compiler` file (the exposure/guarantee contract), `cli/report-writer` gets a scope cross-reference, `common/hierarchy` modified only if code lands in common | Domain mapping inferred from memory index; hydrate may reasonably fold the new content into an existing cli file instead | S:60 R:80 A:65 D:55 | +| 7 | Certain | change_type is `fix` | Description begins "Fix a security comment…"; keyword inference matches; a factually wrong security comment is a defect | S:90 R:95 A:95 D:90 | + +7 assumptions (4 certain, 3 confident, 0 tentative, 0 unresolved). diff --git a/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md new file mode 100644 index 00000000..ba00c9ba --- /dev/null +++ b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md @@ -0,0 +1,176 @@ +# Plan: Fix Secret-Leak Guarantee Comment and Assess the Prompt-Path Secret Leak + +**Change**: 260731-gzzl-fix-secret-prompt-leak-comment +**Intake**: `intake.md` + +## Requirements + +### CLI: testCompiler secret-guarantee comment + +#### R1: The comment states exactly what is guaranteed and what is not +The comment at `packages/cli/src/testCompiler.ts` (currently lines 3–11) MUST be rewritten so that it: + +- scopes the guarantee to what the file controls: `${secrets.*}` tokens are never substituted into the **compiled objective**, so the compiled test text and the objective portion of every prompt carry placeholders, not values; +- keeps the true and load-bearing warning that widening `VARIABLE_REFERENCE_PATTERN` to secrets would substitute real values into every planner/grounder call; +- states plainly that resolved secret values DO reach the model provider at runtime — via the post-typing accessibility hierarchy (`node.text` of the typed field) and via screenshots of the same screen — naming the chain (`ActionExecutor._executeType` → captured hierarchy → `Hierarchy.toPromptElementsForGrounder`/`ForPlanner` → `AIAgent` prompt assembly) so the next maintainer can follow it; +- states where redaction IS wired (report artifacts, spans, error strings via `reportWriter.ts` and `ActionExecutor._redactRuntimeString`; plus whatever prompt-path redaction ships in this change) and where it is NOT (screenshots, non-exact renderings of a secret); +- warns that it is NOT safe to enable full prompt logging or treat provider-side logs as secret-free on the strength of this guarantee; +- describes the **post-change** state exactly — it MUST NOT overstate the shipped mitigation. + +- **GIVEN** a maintainer reading `testCompiler.ts` to decide whether prompt logging is safe to enable +- **WHEN** they read the rewritten comment +- **THEN** they learn the guarantee covers only the compiled objective, that prompts still carry secrets via screenshots (and via any secret rendering that is not an exact value match), and where the redaction seams live + +### Goal-Executor: prompt-path leak assessment + +#### R2: A written, honest assessment of the leak legs exists and is reflected in the comments +The change MUST carry an explicit assessment of the three leak legs (hierarchy, screenshot, deeplink) with an honest verdict, recorded in this plan's `### Design Decisions` and reflected in the rewritten comment(s). The verdict MUST state that full remediation is impossible within this change's scope (the screenshot leg cannot be redacted cheaply and omitting screenshots would blind the planner), and MUST accurately describe what the shipped mitigation covers and what remains residual. + +- **GIVEN** the shipped change +- **WHEN** the plan's Design Decisions and the source comments are read together +- **THEN** each leg's exposure, mitigation status, and residual risk are stated accurately, with no leg omitted or overstated + +### Goal-Executor: exact-value prompt-text redaction (mitigation) + +#### R3: AIAgent redacts resolved secret values from all outbound prompt text +`AIAgent` (packages/goal-executor/src/ai/AIAgent.ts) MUST accept optional `RuntimeBindings` and, when present, rewrite exact occurrences of resolved secret values back to their `${secrets.KEY}` placeholders in every prompt it assembles, reusing `redactResolvedValue` from `@finalrun/common` (no reimplementation): + +- element records produced by `toPromptElementsForPlanner`/`toPromptElementsForGrounder` (and `availableApps` records) MUST have their string fields redacted **before** JSON serialization (so a secret containing JSON-escapable characters is still caught); +- every free-text input (testObjective, history, each `remember` entry, act, preContext, appKnowledge) MUST be redacted individually BEFORE assembly — covering model-echo legs where the model transcribed an on-screen secret into `remember`/`analysis`. There MUST be NO redaction pass over assembled text containing serialized JSON: a post-serialization pass rewrites structural values that equal a secret (numeric PIN vs bounds coordinate → corrupted ui_elements JSON) and misses exact secrets altered by JSON escaping; +- the debug detail blobs (`_detailPlannerRequest`/`_detailGrounderRequest`) MUST pass through the same redaction, since they print element content; +- the `Hierarchy` object MUST NOT be mutated — redaction applies to serialized copies only, so index-based grounding (`flattenedHierarchy[idx]` → bounds → tap point) is untouched; +- `packages/cli/src/sessionRunner.ts` `createSessionExecutor` MUST pass `config.runtimeBindings` into the `AIAgent` construction; +- absent bindings, behavior MUST be byte-identical to today (passthrough). + +Screenshots are explicitly NOT redacted (see R2 / Non-Goals). + +- **GIVEN** bindings `{ secrets: { PASSWORD: 'hunter2-secret' } }` and a captured hierarchy whose typed field has `text: 'hunter2-secret'` +- **WHEN** `AIAgent` builds the next grounder or planner prompt +- **THEN** the outbound prompt text contains `${secrets.PASSWORD}` and never the raw value, while the element's `index`, `bounds`, `id`, and `class` fields are unchanged + +#### R4: Grounding demonstrably survives redaction +The mitigation ships ONLY with proof that grounding still works: the existing `packages/goal-executor`, `packages/cli`, and `packages/common` test suites pass, plus targeted tests showing (a) an element whose `text` was redacted to a placeholder is still locatable — `index`/`bounds`/`id` intact in the prompt and the un-mutated `Hierarchy` still resolves `flattenedHierarchy[idx]` to real bounds — and (b) the no-bindings path is unchanged. + +- **GIVEN** the targeted tests and existing suites +- **WHEN** they run after the mitigation lands +- **THEN** all pass, proving element location and typing flows are structurally unaffected + +### Process: repo hygiene constraint + +#### R5: `fab/backlog.md` is never staged or committed +This change MUST NOT `git add` or commit `fab/backlog.md` at any stage. Apply commits nothing at all; ship MUST stage explicit paths rather than `git add fab/`. + +- **GIVEN** the apply and ship stages of this change +- **WHEN** any git staging happens +- **THEN** `fab/backlog.md` remains untracked and unstaged + +### Non-Goals + +- Screenshot redaction — requires OCR-grade field-region detection; unreliable and expensive, and blurring/omitting the screenshot would blind the planner whose primary signal it is. Documented as residual exposure instead. +- A prompt-logging feature or opt-in flag — no full-prompt logging path exists today (`Logger.d` detail blobs carry element excerpts and prompt lengths, not the full prompt; `runner.log` writes are already redacted by `reportWriter`). The rewritten comment's warning is the deliverable for intake §3(b). +- Redaction inside `Hierarchy` (packages/common) — it has no access to runtime bindings; the seam belongs in goal-executor prompt assembly (intake assumption 4). +- Redacting non-exact renderings of a secret (truncated, reformatted, partially masked by the app) — impossible with value-match redaction; documented as residual. + +### Design Decisions + +#### Leak assessment: hierarchy leg — real, and mitigated by exact-value prompt redaction +**Decision**: Confirmed the leak chain end-to-end and mitigate it at the `AIAgent` prompt-assembly seam. `ActionExecutor._executeType` resolves `${secrets.*}` via `resolveRuntimePlaceholders` (ActionExecutor.ts:363-366) and types the real value; the next capture's `node.text` (and possibly `hintText`/`error`/`accessibilityText`) carries it; `Hierarchy.toPromptElementsForGrounder` (Hierarchy.ts:212-229) emits `text`/`hintText`/`error` verbatim and `toPromptElementsForPlanner` emits `contentDesc`; `AIAgent._buildGrounderPrompt`/`_buildPlannerPrompt` serialize those into the prompt and `_callLLM` sends them to the provider. Nothing in `packages/goal-executor/src/ai/` redacted anything before this change. Android password fields often mask text at the accessibility layer (`•••`) but that is app/platform-dependent — plain-text fields (API keys, OTPs, tokens) leak verbatim. The mitigation substitutes exact value occurrences with the `${secrets.KEY}` placeholder — the same logical token the compiled objective's Execution Rules already teach the model — so verify-flows ("the field now contains the password") read a token the model already understands, and structure (index/bounds/id/class/flags) is untouched. +**Why**: The seam has bindings in scope (wired from `sessionRunner`), covers every hierarchy-carrying prompt regardless of caller (planner, all grounder features, post-action hierarchy), and never mutates the `Hierarchy`, so device-side grounding (index → bounds → tap point) is provably unaffected. Every input is redacted individually before assembly and serialization — element/app record fields before `JSON.stringify` (escaping cannot hide a secret containing `"` or `\`), and each free-text input (objective, act, history, remember entries, preContext, appKnowledge) on its own — so redaction never touches serialized structure. +**Rejected**: (a) redaction inside `Hierarchy.toPromptElements*` — packages/common has no bindings access; (b) masking with an opaque string (`***`) — breaks the token semantics the Execution Rules define and gives the model no way to reason about the field's content; (c) no mitigation — the gate (existing suites + targeted structural proof) is satisfiable, so shipping assessment-only would under-deliver against the intake's criterion; (d) a whole-text pass over the assembled prompt (the first-cut design, failed in review cycle 1) — reproduced defects: a numeric secret equal to a bounds coordinate rewrote serialized structure and corrupted the ui_elements JSON, and a `"`/`\`-bearing secret inside the stringified `remember` array survived unredacted because escaping broke the exact match. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +#### Leak assessment: screenshot leg — real, NOT mitigated (residual exposure) +**Decision**: `request.preActionScreenshot`/`postActionScreenshot` (planner) and `request.screenshot` (grounder, visual grounder) capture the same screen that shows the typed value unless the app masks it visually; they are sent to the provider unredacted, ride in the `llmCall` observability records (`prompt: messages`), and are written to the run report (`screenshots/NNN.jpg`). No mitigation is attempted: masking requires visual detection of the field region (OCR-grade, unreliable, expensive) and omitting or blurring the screenshot would blind the planner. This is documented residual exposure — in this plan and in the rewritten `testCompiler.ts` comment. +**Why**: An honest residual beats a fake fix; the intake explicitly directs "no mitigation — document as residual exposure". +**Rejected**: OCR/region-based masking — cost and reliability; screenshot omission — destroys the planner's primary signal. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +#### Leak assessment: deeplink leg — real, partially mitigated via the hierarchy leg +**Decision**: `_executeDeeplink` resolves `${secrets.*}` into the URL it opens (ActionExecutor.ts:689-698). If the resulting screen renders the URL (browser address bar, error dialog), the value re-enters the captured hierarchy — now covered by the same prompt-text redaction — and the screenshot, which remains residual. The span detail logs `rawDeeplink` (the unresolved form), so the trace path was already safe. +**Why**: The deeplink leg has no seam of its own — its exposure IS the hierarchy + screenshot legs on the next capture, so it inherits exactly their mitigation status. +**Rejected**: refusing to resolve secrets in deeplinks — a behavior regression for legitimate authenticated-URL flows, out of scope. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +#### Verdict: full remediation is impossible; prompt-text leg is closed, screenshots stay open +**Decision**: After this change, resolved secret values no longer reach the provider through prompt TEXT for exact-value occurrences; they still reach the provider through screenshots, and through any on-screen rendering that is not an exact value match (truncation, reformatting, partial masking by the app). Provider-side logs and full prompt logging must therefore still be treated as secret-bearing. +**Why**: This is the accurate post-change state; both the comment and memory must carry it rather than an aspirational "fixed" claim — a confident wrong guarantee was the original defect. +**Rejected**: describing the change as closing the leak — overstatement, recreates the defect at bigger scale. +*Introduced by*: 260731-gzzl-fix-secret-prompt-leak-comment + +## Tasks + +### Phase 2: Core Implementation + +- [x] T001 Add optional `bindings?: RuntimeBindings` to `AIAgent` (packages/goal-executor/src/ai/AIAgent.ts): store it, add private redaction helpers (per-record-field and per-free-text-input, all applied BEFORE assembly/serialization, no assembled-text pass), apply them in `_buildPlannerPrompt` (objective, history, remember entries, preContext, appKnowledge, elements, post-action elements) and `_buildGrounderPrompt` (act, elements, availableApps), and route `_detailPlannerRequest`/`_detailGrounderRequest` payload fields plus the INFO-level act log sites (`formatGrounderRequest` call, `_summarizeGrounderRequest` snippet) through the same redaction; reuse `redactResolvedValue` from `@finalrun/common`; add a rationale comment stating the leak chain, the non-mutation property, why post-serialization redaction is forbidden, and the screenshot residual +- [x] T002 Wire `bindings: config.runtimeBindings` into the `createAiAgent` call in `createSessionExecutor` (packages/cli/src/sessionRunner.ts) +- [x] T003 Add targeted tests in packages/goal-executor/src/ai/test/AIAgent.test.ts: grounder prompt redacts a typed secret to `${secrets.KEY}` while `index`/`bounds`/`id` stay intact and the `Hierarchy` is not mutated (`flattenedHierarchy[idx]` still returns real text/bounds); planner prompt redacts pre/post-action elements and history/remember; a secret containing JSON-escapable characters (`"`, `\`) is still redacted; no-bindings construction leaves prompts unchanged + +### Phase 3: Integration & Edge Cases + +- [x] T004 Run the `packages/common`, `packages/goal-executor`, and `packages/cli` test suites plus workspace typecheck; fix any failures without weakening existing assertions + +### Phase 4: Polish + +- [x] T005 Rewrite the comment at packages/cli/src/testCompiler.ts:3-11 per R1, describing the post-change state exactly: compile-time scope of the guarantee, the runtime hierarchy/screenshot exposure with the concrete chain, where redaction is wired (report artifacts/spans/errors + the new AIAgent prompt seam) and not wired (screenshots, non-exact renderings), the prompt-logging warning, and the retained pattern-widening warning +- [x] T006 Cross-check every factual claim in the rewritten comment(s) and this plan's Design Decisions against the shipped code (file paths, behavior, residuals) so nothing overstates the mitigation; confirm `fab/backlog.md` is untouched and unstaged + +## Execution Order + +- T001 blocks T002 (constructor param must exist before wiring) and T003 (tests exercise the new helpers) +- T005 and T006 run last — the comment must describe the final shipped state + +## Acceptance + +### Functional Completeness + +- [x] A-001 R1: The rewritten `testCompiler.ts` comment scopes the guarantee to the compiled objective, states the runtime hierarchy + screenshot exposure with the concrete file chain, states where redaction is and is not wired, warns against enabling full prompt logging, and keeps the pattern-widening warning +- [x] A-002 R3: `AIAgent` accepts optional bindings and redacts exact resolved-secret occurrences from grounder and planner prompt text (element fields pre-serialization plus the assembled text), replacing them with `${secrets.KEY}` placeholders via the reused `redactResolvedValue` +- [x] A-003 R3: `sessionRunner.createSessionExecutor` passes `config.runtimeBindings` to the `AIAgent`, and constructing `AIAgent` without bindings leaves prompt assembly byte-identical to the pre-change behavior +- [x] A-004 R2: The three-leg leak assessment (hierarchy mitigated, screenshot residual, deeplink inheriting both) is recorded in this plan's Design Decisions and reflected accurately in the shipped comments + +### Behavioral Correctness + +- [x] A-005 R3: Redaction never mutates the `Hierarchy` object — after building a redacted prompt, `flattenedHierarchy[idx]` still returns the original `text` and `bounds`, so index-based grounding and tap coordinates are unaffected +- [x] A-006 R4: A targeted test proves an element whose `text` was redacted to a placeholder keeps `index`/`bounds`/`id` intact in the emitted prompt — *re-review cycle 1: now holds generally. The whole-text pass is gone; only `_redactPromptElements` (per string field, pre-`JSON.stringify`) and `_redactPromptText` (per free-text input) run. Re-probed against dist with `secrets.PIN='1080'` and `bounds:[0,0,1080,240]`: emitted `ui_elements` parses, `bounds` is `[0,0,1080,240]`, `text` is `${secrets.PIN}`. Covered by the regression test at AIAgent.test.ts "redaction leaves structural JSON intact when a secret equals a coordinate".* + +### Scenario Coverage + +- [x] A-007 R4: Existing `packages/goal-executor`, `packages/cli`, and `packages/common` suites pass unmodified (typing and grounding flows unbroken) + +### Edge Cases & Error Handling + +- [x] A-008 R3: A secret value containing JSON-escapable characters (`"`, `\`) is redacted (per-field redaction happens before `JSON.stringify`), and empty-string secrets are skipped without error + +### Code Quality + +- [x] A-009 Pattern consistency: New code follows the surrounding AIAgent/test patterns (private helpers, bracket-notation private access in tests, node:test + assert/strict) +- [x] A-010 No unnecessary duplication: `redactResolvedValue` is reused, not reimplemented; no new utility duplicates an existing one +- [x] A-011 No restatement comments: every new comment passes the deletion test (states rationale, cross-file coupling, or residual risk the code cannot show) + +### Security + +- [x] A-012 R2: No new secret-bearing surface is introduced, and every residual exposure (screenshots, non-exact renderings) is documented rather than hidden; the comment does not overstate the shipped mitigation — *re-review cycle 1: gap closed. `remember` entries are redacted per entry BEFORE `JSON.stringify` (AIAgent.ts:462). Re-probed against dist with `secrets.WEIRD='alpha"beta\\gamma'` in `remember`/`history`/`preContext`/`appKnowledge`: all four emit `${secrets.WEIRD}`, neither the raw nor the escaped form appears. Comment claims re-verified against current code.* +- [x] A-013 R5: `fab/backlog.md` was never staged or committed during this change + +## Notes + +- Check items as you review: `- [x]` +- All acceptance items must pass before `/fab-continue` (hydrate) +- If an item is not applicable, mark checked and prefix with **N/A**: `- [x] A-NNN **N/A**: {reason}` + +## Deletion Candidates + +- None — this change adds new functionality without making existing code redundant. The pre-existing redaction seams stay load-bearing: `ActionExecutor._redactRuntimeString` still covers spans/errors, and `reportWriter`'s per-field + device-log redaction still covers write paths the prompt seam never touches. + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Confident | Implement mitigation (a): the intake's gate is satisfiable — the AIAgent prompt-assembly seam has bindings in scope via sessionRunner, redaction is structure-preserving and non-mutating, and both properties are unit-provable alongside green existing suites | User delegated with an explicit criterion ("implement only if you can show grounding still works"); the seam analysis and non-mutation property make the structural half provable, though the model's semantic response to placeholders is inherently empirical | S:85 R:70 A:80 D:70 | +| 2 | Confident | Redact every input individually before assembly/serialization — element and app record fields before `JSON.stringify`, each free-text input (objective, act, history, remember entries, preContext, appKnowledge) on its own — with NO pass over the assembled prompt | Revised in rework cycle 1: the original whole-text pass corrupted serialized structure when a numeric secret equaled a coordinate and missed JSON-escaped secrets in `remember`; per-input redaction has neither failure mode | S:80 R:80 A:85 D:75 | +| 3 | Confident | Replace values with their `${secrets.KEY}` placeholder rather than an opaque mask | The compiled objective's Execution Rules already teach the model to treat that token as the logical secret, so verify-flows read a token with known semantics; an opaque mask would not carry that meaning | S:75 R:75 A:80 D:70 | +| 4 | Certain | Screenshots ship unredacted and are documented as residual exposure in both the plan and the comment | Explicit intake directive: "no mitigation attempted — document as residual exposure" | S:95 R:90 A:95 D:95 | +| 5 | Confident | The debug detail blobs (`_detail*Request`) are routed through the same redaction since AIAgent now holds bindings | Same leak class at the same seam for two lines of change; console output was the one write path not covered by reportWriter's redaction | S:60 R:85 A:80 D:70 | +| 6 | Confident | No prompt-logging opt-in flag is built (intake §3(b)): no full-prompt logging path exists today, so the comment's warning is the deliverable | Verified: detail blobs log prompt length and 3-element excerpts, not the full prompt; `runner.log` writes are redacted by reportWriter | S:70 R:85 A:80 D:75 | + +6 assumptions (1 certain, 5 confident, 0 tentative). diff --git a/packages/cli/src/sessionRunner.ts b/packages/cli/src/sessionRunner.ts index 005479d4..8e822e87 100644 --- a/packages/cli/src/sessionRunner.ts +++ b/packages/cli/src/sessionRunner.ts @@ -455,6 +455,9 @@ function createSessionExecutor( apiKeys: config.apiKeys, defaults: config.defaults, features: config.features, + // Enables AIAgent's prompt-path secret redaction; omitting this would + // silently ship prompts with resolved secret values in hierarchy text. + bindings: config.runtimeBindings, }); return dependencies.createExecutor({ diff --git a/packages/cli/src/testCompiler.ts b/packages/cli/src/testCompiler.ts index 284aa14d..f3deab8a 100644 --- a/packages/cli/src/testCompiler.ts +++ b/packages/cli/src/testCompiler.ts @@ -1,14 +1,37 @@ import type { TestDefinition, RuntimeBindings } from '@finalrun/common'; -// Secret-leak guard: this pattern deliberately matches ONLY ${variables.*} -// tokens, never ${secrets.*}. Secret placeholders are left as literal tokens in -// the compiled prompt — the Execution Rules appended below instruct the model to -// echo them verbatim — so secret VALUES never enter the LLM prompt, the model -// provider's logs, or compiled test artifacts. Secrets are substituted only -// downstream at the point of use (resolveRuntimePlaceholders in -// packages/common/src/repoPlaceholders.ts, called from ActionExecutor when -// typing or opening a deeplink). Widening this pattern to secrets would leak -// their values into every plan/grounder call. +// Secret-leak guard — COMPILE-TIME SCOPE ONLY. This pattern deliberately +// matches ${variables.*} tokens and never ${secrets.*}: secret placeholders +// stay literal tokens in the compiled objective (the Execution Rules appended +// below instruct the model to echo them verbatim), so neither the compiled +// test text nor the objective portion of any prompt receives a secret VALUE +// through placeholder substitution. (A ${variables.*} value that happens to +// equal a secret still lands in the compiled text — this pattern cannot tell.) +// Widening this pattern to secrets would bake real values into the compiled +// test artifact and the planner objective — the durable leak: grounder +// prompts carry no objective, and the AIAgent redaction seam below catches +// prompt text only when runtime bindings are wired. +// +// That is the WHOLE guarantee; it does not keep secret values out of LLM +// prompts at runtime. Once ActionExecutor._executeType (or _executeDeeplink) +// resolves a placeholder (resolveRuntimePlaceholders in +// packages/common/src/repoPlaceholders.ts) and the value lands on screen, the +// next captured accessibility hierarchy carries it — grounder prompts emit +// the typed field's text/hintText/error verbatim +// (Hierarchy.toPromptElementsForGrounder); planner prompts carry only +// contentDesc for image/button-class nodes, which can still surface a value +// rendered into accessibility text — and screenshots show the same screen. +// The prompt-assembly seam +// (AIAgent._buildPlannerPrompt/_buildGrounderPrompt in +// packages/goal-executor/src/ai/AIAgent.ts) redacts exact occurrences of +// resolved secret values from prompt TEXT back to their ${secrets.*} +// placeholders, but screenshots reach the model provider UNREDACTED, and any +// rendering of a secret that is not an exact value match (truncated, +// reformatted, partially masked by the app) passes through. +// redactResolvedValue also guards the write paths — report artifacts, spans, +// error strings (reportWriter.ts, ActionExecutor._redactRuntimeString) — but +// report screenshots are raw too. Do NOT enable full prompt logging or treat +// provider-side logs as secret-free on the strength of this guard. const VARIABLE_REFERENCE_PATTERN = /\$\{variables\.([A-Za-z0-9_-]+)\}/g; export function compileTestObjective( diff --git a/packages/goal-executor/src/ai/AIAgent.ts b/packages/goal-executor/src/ai/AIAgent.ts index 1677a264..4fcfeb77 100644 --- a/packages/goal-executor/src/ai/AIAgent.ts +++ b/packages/goal-executor/src/ai/AIAgent.ts @@ -43,10 +43,12 @@ import { PLANNER_ACTION_FAILED, PLANNER_ACTION_DEEPLINK, parseModel, + redactResolvedValue, type FeatureName, type FeatureOverrides, type ModelDefaults, type ReasoningLevel, + type RuntimeBindings, } from '@finalrun/common'; import { describeLLMTrace, @@ -204,6 +206,7 @@ export class AIAgent { private _apiKeys: Record; private _defaults: ModelDefaults; private _features: FeatureOverrides; + private _bindings?: RuntimeBindings; private _promptCache: Map = new Map(); // Cached Vercel AI SDK clients, keyed by provider/model @@ -214,10 +217,64 @@ export class AIAgent { apiKeys: Record; defaults: ModelDefaults; features?: FeatureOverrides; + /** + * Enables prompt-path secret redaction (see _redactPromptElements). + * Without bindings, prompts are assembled unredacted — callers that hold + * runtime secrets (sessionRunner) must pass them through. + */ + bindings?: RuntimeBindings; }) { this._apiKeys = params.apiKeys; this._defaults = params.defaults; this._features = params.features ?? {}; + this._bindings = params.bindings; + } + + /** + * Prompt-path secret redaction. The compile-time guard in + * packages/cli/src/testCompiler.ts keeps ${secrets.*} values out of the + * compiled objective only: once ActionExecutor._executeType types a resolved + * secret into a field, the next captured hierarchy carries the value in its + * node text/hint/error fields, and the model can echo an on-screen secret + * into remember/history. These two helpers rewrite exact occurrences of + * resolved secret values back to their ${secrets.KEY} placeholder — the same + * logical token the compiled objective's Execution Rules already teach the + * model — before the prompt leaves the process. + * + * Every input is redacted individually BEFORE assembly and serialization — + * deliberately, there is NO pass over the assembled prompt text. A + * post-JSON.stringify pass fails in both directions: it rewrites structural + * values that happen to equal a secret (a numeric PIN equal to a bounds + * coordinate corrupts the ui_elements JSON), and it misses exact secrets + * that serialization escaped (`"` → `\"`, `\` → `\\`). Only serialized + * copies are touched — the Hierarchy itself is never mutated, so + * index-based grounding (flattenedHierarchy[idx] → bounds → tap point) + * still resolves against the real on-screen values. + * + * Screenshots are NOT redacted here and DO still carry the secret to the + * provider — masking them needs visual field-region detection, and dropping + * them would blind the planner. Exact-match is also the ceiling: a value the + * app re-renders (truncated, reformatted, partially masked) is not caught. + */ + private _redactPromptText(value: string): string { + if (!this._bindings) return value; + return redactResolvedValue(value, this._bindings) ?? value; + } + + /** Per-element counterpart of _redactPromptText — see its doc comment. */ + private _redactPromptElements( + elements: Record[], + ): Record[] { + const bindings = this._bindings; + if (!bindings) return elements; + return elements.map((element) => { + const out: Record = {}; + for (const [key, value] of Object.entries(element)) { + out[key] = + typeof value === 'string' ? redactResolvedValue(value, bindings) : value; + } + return out; + }); } /** @@ -388,27 +445,36 @@ export class AIAgent { userParts.push({ type: 'image', image: request.preActionScreenshot }); } - let textPrompt = `Test objective: ${request.testObjective}\n`; + // Each free-text input is redacted on its own, before it meets any + // serialized JSON — see _redactPromptText for why there is no pass over + // the assembled prompt. History/remember can carry a secret the model + // transcribed from an earlier screenshot or hierarchy. + let textPrompt = `Test objective: ${this._redactPromptText(request.testObjective)}\n`; textPrompt += `Platform: ${request.platform}\n`; if (request.history) { - textPrompt += `\nHistory of actions taken so far:\n${request.history}\n`; + textPrompt += `\nHistory of actions taken so far:\n${this._redactPromptText(request.history)}\n`; } if (request.remember && request.remember.length > 0) { - textPrompt += `\nImportant context to remember:\n${JSON.stringify(request.remember)}\n`; + // Redacted per entry BEFORE JSON.stringify — escaping would otherwise + // hide a secret containing `"` or `\` from the exact-match rewrite. + const remember = request.remember.map((item) => this._redactPromptText(item)); + textPrompt += `\nImportant context to remember:\n${JSON.stringify(remember)}\n`; } if (request.preContext) { - textPrompt += `\nPre-context:\n${request.preContext}\n`; + textPrompt += `\nPre-context:\n${this._redactPromptText(request.preContext)}\n`; } if (request.appKnowledge) { - textPrompt += `\nApp knowledge:\n${request.appKnowledge}\n`; + textPrompt += `\nApp knowledge:\n${this._redactPromptText(request.appKnowledge)}\n`; } if (request.hierarchy) { - const elements = request.hierarchy.toPromptElementsForPlanner(request.platform); + const elements = this._redactPromptElements( + request.hierarchy.toPromptElementsForPlanner(request.platform), + ); textPrompt += `\nui_elements:\n${JSON.stringify(elements)}\n`; } @@ -417,7 +483,9 @@ export class AIAgent { } if (request.postActionHierarchy) { - const postElements = request.postActionHierarchy.toPromptElementsForPlanner(request.platform); + const postElements = this._redactPromptElements( + request.postActionHierarchy.toPromptElementsForPlanner(request.platform), + ); textPrompt += `\nPost-action ui_elements:\n${JSON.stringify(postElements)}\n`; } @@ -436,7 +504,9 @@ export class AIAgent { Logger.i(formatGrounderRequest({ step: request.traceStep, feature: request.feature, - act: request.act, + // Redacted like the prompt: console output never crosses + // reportWriter's redaction. + act: this._redactPromptText(request.act), })); } @@ -491,19 +561,26 @@ export class AIAgent { userParts.push({ type: 'image', image: request.screenshot }); } - let text = `act: ${request.act}\n`; + // `act` is planner-authored and can quote what the model read off a + // previous screen. Redacted on its own, before it meets the serialized + // element JSON — see _redactPromptText for why there is no assembled-text + // pass. + let text = `act: ${this._redactPromptText(request.act)}\n`; if (request.platform) { text += `platform: ${request.platform}\n`; } if (request.hierarchy) { - const elements = request.hierarchy.toPromptElementsForGrounder(request.platform); + const elements = this._redactPromptElements( + request.hierarchy.toPromptElementsForGrounder(request.platform), + ); text += `\nui_elements:\n${JSON.stringify(elements)}\n`; } if (request.availableApps) { - text += `\navailable_apps:\n${JSON.stringify(request.availableApps)}\n`; + const apps = this._redactPromptElements(request.availableApps); + text += `\navailable_apps:\n${JSON.stringify(apps)}\n`; } userParts.push({ type: 'text', text }); @@ -980,16 +1057,23 @@ export class AIAgent { ? req.hierarchy.toPromptElementsForGrounder(req.platform).length : 0; parts.push(`hierarchy=${hierarchyCount}`); - const actSnippet = req.act.length > 80 ? `${req.act.slice(0, 80)}…` : req.act; + // Redact before truncating — a snippet cut mid-secret would no longer + // exact-match the value. + const act = this._redactPromptText(req.act); + const actSnippet = act.length > 80 ? `${act.slice(0, 80)}…` : act; parts.push(`act="${actSnippet}"`); return parts.join(' '); } private _detailPlannerRequest(req: PlannerRequest, prompt: string): string { + // Field-by-field redaction, mirroring _buildPlannerPrompt: firstFew and + // history reproduce hierarchy content, console output never crosses + // reportWriter's redaction, and redacting the stringified blob instead + // would hit the two failure modes described on _redactPromptText. const payload = { logContext: req.logContext, platform: req.platform, - goal: req.testObjective, + goal: this._redactPromptText(req.testObjective), screenshot: req.preActionScreenshot ? `` : null, @@ -999,38 +1083,43 @@ export class AIAgent { hierarchy: req.hierarchy ? { count: req.hierarchy.toPromptElementsForPlanner(req.platform).length, - firstFew: req.hierarchy - .toPromptElementsForPlanner(req.platform) - .slice(0, 3), + firstFew: this._redactPromptElements( + req.hierarchy.toPromptElementsForPlanner(req.platform).slice(0, 3), + ), } : null, - history: req.history ? req.history.split('\n').filter(Boolean) : [], - remember: req.remember ?? [], - preContext: req.preContext ?? null, - appKnowledge: req.appKnowledge ?? null, + history: req.history + ? this._redactPromptText(req.history).split('\n').filter(Boolean) + : [], + remember: (req.remember ?? []).map((item) => this._redactPromptText(item)), + preContext: req.preContext ? this._redactPromptText(req.preContext) : null, + appKnowledge: req.appKnowledge ? this._redactPromptText(req.appKnowledge) : null, promptLength: prompt.length, }; return `[AI plan detail] ${this._formatLogContext(req.logContext, req.traceStep)} ${JSON.stringify(payload, null, 2)}`; } private _detailGrounderRequest(req: GrounderRequest, prompt: string): string { + // Field-by-field redaction — see _detailPlannerRequest. const payload = { logContext: req.logContext, feature: req.feature, platform: req.platform, - act: req.act, + act: this._redactPromptText(req.act), screenshot: req.screenshot ? `` : null, hierarchy: req.hierarchy ? { count: req.hierarchy.toPromptElementsForGrounder(req.platform).length, - firstFew: req.hierarchy - .toPromptElementsForGrounder(req.platform) - .slice(0, 3), + firstFew: this._redactPromptElements( + req.hierarchy.toPromptElementsForGrounder(req.platform).slice(0, 3), + ), } : null, - availableApps: req.availableApps ?? null, + availableApps: req.availableApps + ? this._redactPromptElements(req.availableApps) + : null, promptLength: prompt.length, }; return `[AI ground detail] ${this._formatLogContext(req.logContext, req.traceStep)} ${JSON.stringify(payload, null, 2)}`; diff --git a/packages/goal-executor/src/ai/test/AIAgent.test.ts b/packages/goal-executor/src/ai/test/AIAgent.test.ts index 0b96d079..ad3e2a86 100644 --- a/packages/goal-executor/src/ai/test/AIAgent.test.ts +++ b/packages/goal-executor/src/ai/test/AIAgent.test.ts @@ -4,19 +4,28 @@ import { FEATURE_GROUNDER, FEATURE_PLANNER, FEATURE_SCROLL_INDEX_GROUNDER, + Hierarchy, PLANNER_ACTION_ROTATE, PLANNER_ACTION_TAP, type FeatureName, type FeatureOverrides, type ModelDefaults, + type RuntimeBindings, } from '@finalrun/common'; -import { AIAgent, GrounderResponse, PlannerResponse } from '../AIAgent.js'; +import { + AIAgent, + GrounderRequest, + GrounderResponse, + PlannerRequest, + PlannerResponse, +} from '../AIAgent.js'; import { FatalProviderError } from '../providerFailure.js'; function makeAgent(overrides?: { defaults?: Partial; features?: FeatureOverrides; apiKeys?: Record; + bindings?: RuntimeBindings; }): AIAgent { const defaults: ModelDefaults = { provider: overrides?.defaults?.provider ?? 'google', @@ -33,9 +42,38 @@ function makeAgent(overrides?: { }, defaults, ...(overrides?.features !== undefined ? { features: overrides.features } : {}), + ...(overrides?.bindings !== undefined ? { bindings: overrides.bindings } : {}), }); } +function buildGrounderPrompt( + agent: AIAgent, + request: GrounderRequest, +): { userParts: unknown[]; text: string } { + return ( + agent as unknown as { + _buildGrounderPrompt: (req: GrounderRequest) => { + userParts: unknown[]; + text: string; + }; + } + )._buildGrounderPrompt(request); +} + +function buildPlannerPrompt( + agent: AIAgent, + request: PlannerRequest, +): { userParts: unknown[]; textPrompt: string } { + return ( + agent as unknown as { + _buildPlannerPrompt: (req: PlannerRequest) => { + userParts: unknown[]; + textPrompt: string; + }; + } + )._buildPlannerPrompt(request); +} + function parsePlannerResponse(output: unknown, rawText = ''): PlannerResponse { const agent = makeAgent(); return ( @@ -484,6 +522,182 @@ test('AIAgent rejects grounder responses that are not JSON objects', () => { ); }); +// ---------------------------------------------------------------------------- +// Prompt-path secret redaction +// ---------------------------------------------------------------------------- + +const SECRET_VALUE = 'hunter2-secret-value'; +const SECRET_BINDINGS: RuntimeBindings = { + secrets: { PASSWORD: SECRET_VALUE }, + variables: {}, +}; + +function hierarchyWithTypedSecret(text: string): Hierarchy { + return Hierarchy.fromFlatJson([ + { + text, + id: 'password_field', + class: 'android.widget.EditText', + bounds: [10, 20, 300, 80], + isEditable: true, + isFocused: true, + }, + { + text: 'Login', + id: 'login_button', + class: 'android.widget.Button', + bounds: [10, 100, 300, 160], + }, + ]); +} + +test('AIAgent redacts a typed secret from the grounder prompt but keeps the element locatable', () => { + const hierarchy = hierarchyWithTypedSecret(SECRET_VALUE); + const agent = makeAgent({ bindings: SECRET_BINDINGS }); + + const { text } = buildGrounderPrompt(agent, { + feature: FEATURE_GROUNDER, + act: `Verify the password field contains "${SECRET_VALUE}"`, + hierarchy, + platform: 'android', + }); + + assert.ok(text.includes('${secrets.PASSWORD}'), 'placeholder present'); + assert.ok(!text.includes(SECRET_VALUE), 'raw secret absent (elements and act line)'); + // Locating structure is untouched: index, id, class, bounds all survive. + assert.ok(text.includes('"index":0')); + assert.ok(text.includes('"id":"password_field"')); + assert.ok(text.includes('"bounds":[10,20,300,80]')); + assert.ok(text.includes('"isEditable":true')); +}); + +test('AIAgent prompt redaction never mutates the Hierarchy itself', () => { + const hierarchy = hierarchyWithTypedSecret(SECRET_VALUE); + const agent = makeAgent({ bindings: SECRET_BINDINGS }); + + buildGrounderPrompt(agent, { + feature: FEATURE_GROUNDER, + act: 'Tap the login button', + hierarchy, + platform: 'android', + }); + + // Index-based grounding resolves against the ORIGINAL nodes: the tap/type + // coordinates come from flattenedHierarchy[idx].bounds, not from the prompt. + assert.equal(hierarchy.flattenedHierarchy[0]!.text, SECRET_VALUE); + assert.deepEqual(hierarchy.flattenedHierarchy[0]!.bounds, [10, 20, 300, 80]); +}); + +test('AIAgent redacts secrets from planner hierarchy, history, and remember', () => { + // Planner elements require accessibility text on a button/image node. + const plannerHierarchy = Hierarchy.fromFlatJson([ + { + content_desc: SECRET_VALUE, + class: 'android.widget.Button', + bounds: [0, 0, 100, 50], + }, + ]); + const agent = makeAgent({ bindings: SECRET_BINDINGS }); + + const { textPrompt } = buildPlannerPrompt(agent, { + testObjective: 'Type ${secrets.PASSWORD} into the password field', + platform: 'android', + hierarchy: plannerHierarchy, + postActionHierarchy: plannerHierarchy, + history: `1. [type] Typed "${SECRET_VALUE}" into the field → SUCCESS\n`, + remember: [`The field now shows ${SECRET_VALUE}`], + }); + + assert.ok(textPrompt.includes('${secrets.PASSWORD}')); + assert.ok(!textPrompt.includes(SECRET_VALUE)); + assert.ok(textPrompt.includes('Post-action ui_elements:')); +}); + +test('AIAgent redacts secrets containing JSON-escapable characters', () => { + // Redaction runs before JSON.stringify; matching afterwards would miss this + // value because `"` and `\` are escaped in the serialized element text. + const weirdSecret = 'alpha"beta\\gamma'; + const hierarchy = hierarchyWithTypedSecret(weirdSecret); + const agent = makeAgent({ + bindings: { secrets: { WEIRD: weirdSecret }, variables: {} }, + }); + + const { text } = buildGrounderPrompt(agent, { + feature: FEATURE_GROUNDER, + act: 'Verify the field', + hierarchy, + platform: 'android', + }); + + assert.ok(text.includes('${secrets.WEIRD}')); + assert.ok(!text.includes('alpha')); + assert.ok(!text.includes('gamma')); +}); + +test('AIAgent redaction leaves structural JSON intact when a secret equals a coordinate', () => { + // Regression: a whole-text pass over the assembled prompt used to rewrite + // the 1080 inside bounds to ${secrets.PIN}, corrupting the ui_elements JSON. + const hierarchy = Hierarchy.fromFlatJson([ + { + text: '1080', + id: 'pin_field', + class: 'android.widget.EditText', + bounds: [0, 0, 1080, 240], + isEditable: true, + }, + ]); + const agent = makeAgent({ + bindings: { secrets: { PIN: '1080' }, variables: {} }, + }); + + const { text } = buildGrounderPrompt(agent, { + feature: FEATURE_GROUNDER, + act: 'Verify the PIN field', + hierarchy, + platform: 'android', + }); + + const serialized = text.match(/ui_elements:\n(.*)\n/)?.[1]; + assert.ok(serialized, 'ui_elements JSON present'); + const elements = JSON.parse(serialized) as Array>; + assert.deepEqual(elements[0]!['bounds'], [0, 0, 1080, 240]); + assert.equal(elements[0]!['text'], '${secrets.PIN}'); +}); + +test('AIAgent redacts an escapable secret echoed into remember', () => { + // Regression: remember was JSON.stringify-ed before redaction, so `"`/`\` + // in the secret were escaped and no longer exact-matched the value. + const weirdSecret = 'alpha"beta\\gamma'; + const agent = makeAgent({ + bindings: { secrets: { WEIRD: weirdSecret }, variables: {} }, + }); + + const { textPrompt } = buildPlannerPrompt(agent, { + testObjective: 'Log in', + platform: 'android', + remember: [`the field shows ${weirdSecret}`], + }); + + assert.ok(textPrompt.includes('${secrets.WEIRD}')); + assert.ok(!textPrompt.includes('alpha')); + assert.ok(!textPrompt.includes('gamma')); +}); + +test('AIAgent without bindings assembles prompts unredacted', () => { + const hierarchy = hierarchyWithTypedSecret(SECRET_VALUE); + const agent = makeAgent(); + + const { text } = buildGrounderPrompt(agent, { + feature: FEATURE_GROUNDER, + act: 'Verify the field', + hierarchy, + platform: 'android', + }); + + assert.ok(text.includes(SECRET_VALUE)); + assert.ok(!text.includes('${secrets.PASSWORD}')); +}); + // ---------------------------------------------------------------------------- // Retry behavior for plan() and ground() // ----------------------------------------------------------------------------