From 9b5a2a4c8853ec6022292cfbc0d0fedd3b17c2c2 Mon Sep 17 00:00:00 2001 From: droid-ash Date: Fri, 31 Jul 2026 18:56:34 +0000 Subject: [PATCH 1/3] fix: Fix Redaction Prompt Corruption redactResolvedValue used unanchored substring matching, so a secret value equal to (or a substring of) its own placeholder token could nest into invalid tokens, and short secret values could mangle arbitrary prose once PR #175 wired redaction into the LLM prompt path. Protect existing placeholder tokens as a trailing alternation branch and skip values under 3 characters, with regression tests and corrected comments/docs. --- docs/memory/cli/index.md | 2 +- docs/memory/cli/report-writer.md | 8 +- docs/memory/cli/test-compiler.md | 35 +++- docs/memory/common/index.md | 3 +- docs/memory/common/repo-placeholders.md | 177 ++++++++++++++++ docs/memory/index.md | 2 +- .../.history.jsonl | 17 ++ .../.status.yaml | 51 +++++ .../intake.md | 159 +++++++++++++++ .../plan.md | 193 ++++++++++++++++++ .../intake.md | 2 +- .../plan.md | 2 +- packages/cli/src/testCompiler.ts | 5 +- packages/common/src/repoPlaceholders.ts | 49 ++++- .../common/src/test/repoPlaceholders.test.ts | 70 +++++++ packages/goal-executor/src/ai/AIAgent.ts | 6 +- .../goal-executor/src/ai/test/AIAgent.test.ts | 42 ++++ 17 files changed, 803 insertions(+), 20 deletions(-) create mode 100644 docs/memory/common/repo-placeholders.md create mode 100644 fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl create mode 100644 fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml create mode 100644 fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md create mode 100644 fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md create mode 100644 packages/common/src/test/repoPlaceholders.test.ts diff --git a/docs/memory/cli/index.md b/docs/memory/cli/index.md index 16c11f1..2b1b8c0 100644 --- a/docs/memory/cli/index.md +++ b/docs/memory/cli/index.md @@ -10,4 +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. | +| [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 values, sub-3-char secret values, and INFO planner output are not. | diff --git a/docs/memory/cli/report-writer.md b/docs/memory/cli/report-writer.md index 317a477..887a3bd 100644 --- a/docs/memory/cli/report-writer.md +++ b/docs/memory/cli/report-writer.md @@ -17,11 +17,13 @@ Four public emit orchestrators — `writeRunInputs`, `writeTestRecord`, `writeTe ## Secret Redaction -Every value that reaches a file crosses `redactResolvedValue(value, bindings)` (exported from `@finalrun/common`, defined in its `repoPlaceholders.ts`), which rewrites a resolved secret value back to its `${secrets.}` placeholder: runner-log lines, a test record's `message` and `analysis`, step JSON (action text and url, analysis, thought, trace and timing detail strings), the synthetic failure message, and the copied device log. +Every value that reaches a file crosses `redactResolvedValue(value, bindings)` (exported from `@finalrun/common`, defined in its `repoPlaceholders.ts` — [/common/repo-placeholders.md](/common/repo-placeholders.md)), which rewrites a resolved secret value back to its `${secrets.}` placeholder: runner-log lines, a test record's `message` and `analysis`, step JSON (action text and url, analysis, thought, trace and timing detail strings), the synthetic failure message, and the copied device log. -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 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 **redactable** 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. +"Redactable" is the operative word, and the sweep cannot show more than it: its fixture secret is a 17-character value, so what the sweep proves is bounded by the shared function's own bounds. A secret value shorter than `MIN_REDACTABLE_SECRET_LENGTH` (3) is never redacted, so a 1–2 character secret value **does** reach files beneath the run directory raw — an accepted residual, since substituting such a value would rewrite every incidental occurrence of that character sequence in a log line or step record instead. Conversely, an authored `${secrets.KEY}` token already present in report text — test steps like `Enter ${secrets.email}` carry them routinely — is consumed whole by the redaction alternation's last branch and emitted verbatim, never rewritten into a nested token. + +The contract is bounded in two further 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 diff --git a/docs/memory/cli/test-compiler.md b/docs/memory/cli/test-compiler.md index cdae468..20dceeb 100644 --- a/docs/memory/cli/test-compiler.md +++ b/docs/memory/cli/test-compiler.md @@ -1,6 +1,6 @@ --- 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." +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 values, sub-3-char secret values, and INFO planner output are not." --- # Test Objective Compilation (cli) @@ -60,9 +60,12 @@ hierarchy and screenshot status above (the span detail logs the unresolved `rawD `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: +`_buildGrounderPrompt` MUST replace occurrences of resolved secret values with their +`${secrets.KEY}` placeholder — via `redactResolvedValue` from `@finalrun/common`, whose matching +semantics are reused rather than reimplemented, and whose bounds the prompt path therefore inherits +in full: unanchored longest-value-first matching, an existing placeholder token consumed whole rather +than rewritten, and no redaction of a value shorter than `MIN_REDACTABLE_SECRET_LENGTH` (3) +([/common/repo-placeholders.md](/common/repo-placeholders.md)). Redaction applies: - to every free-text input individually (`testObjective`, `act`, `history`, each `remember` entry, `preContext`, `appKnowledge`); @@ -77,8 +80,10 @@ only serialized copies are rewritten — so index-based grounding (`flattenedHie → 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). +The "Prompt-path secret redaction" block in `packages/goal-executor/src/ai/test/AIAgent.test.ts` pins +this behaviour case by case, including one test for each failure mode a pass over assembled text would +introduce (see Design Decisions) and one for each way unanchored value matching can corrupt prompt +text rather than protect it. #### Scenario: the typed field is redacted and still locatable - **GIVEN** bindings `{ secrets: { PASSWORD: 'hunter2-secret' } }` and a captured hierarchy whose @@ -94,6 +99,19 @@ failure mode a pass over assembled text would introduce (see Design Decisions). - **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}` +#### Scenario: the objective's own placeholder token survives its value's redaction +- **GIVEN** bindings `{ secrets: { PASSWORD: 'PASSWORD' } }` and a `testObjective` reading + `Type ${secrets.PASSWORD} into the field, then type PASSWORD again` +- **WHEN** the planner prompt is built +- **THEN** the prompt carries `Type ${secrets.PASSWORD} into the field, then type ${secrets.PASSWORD} + again` and never a nested `${secrets.${secrets.PASSWORD}}` token + +#### Scenario: a secret value too short to redact leaves the prompt untouched +- **GIVEN** bindings `{ secrets: { TOKEN: 's' } }` and any planner request +- **WHEN** the planner prompt is built +- **THEN** the assembled text is byte-identical to a bindings-less agent's assembly of the same + request — the value reaches the provider raw rather than mangling every `s` in the prompt + ### 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: @@ -103,6 +121,11 @@ remains exposed after prompt-text redaction: 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. +- **Secret values shorter than three characters** — never redacted anywhere, so they reach the + provider (and the report, and spans) raw. The trade is deliberate: substituting a 1–2 character + value rewrites every incidental occurrence of that character sequence and turns the prompt into + garbage, which corrupts model input rather than protecting anything + ([/common/repo-placeholders.md](/common/repo-placeholders.md)). - **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. diff --git a/docs/memory/common/index.md b/docs/memory/common/index.md index 966b491..f0b124c 100644 --- a/docs/memory/common/index.md +++ b/docs/memory/common/index.md @@ -1,5 +1,5 @@ --- -description: "`packages/common` — the base of the dependency graph, imported by every other package: the UI-hierarchy parse contract that planner and grounder prompts are built from (two deliberately non-equivalent parse paths, presence-based alias resolution, two bounds forms, unvalidated field casts), and the `CliEnv` environment-loading contract (dotenv/OS precedence, falsy-value traps, and where model and reasoning-level validation lives)" +description: "`packages/common` — the base of the dependency graph, imported by every other package: the UI-hierarchy parse contract planner and grounder prompts are built from (two non-equivalent parse paths, presence-based alias resolution, two bounds forms, unvalidated casts), the `${variables.*}`/`${secrets.*}` placeholder contract in both directions (with the bounds and residuals of secret redaction), and the `CliEnv` environment-loading contract (dotenv/OS precedence, falsy-value traps)" --- # Common Documentation @@ -9,3 +9,4 @@ description: "`packages/common` — the base of the dependency graph, imported b |------|-------------| | [env](env.md) | `CliEnv` environment loading (`packages/common/src/env.ts`): `load` layers `.env.` → plain `.env` (fill-only) → OS env (highest precedence), `includeDotEnv` opts out only on a literal `false`, and `getRequired`'s falsy check makes an empty string as missing as an absent key. Model and reasoning-level validation live in `constants.ts` beside their level lists, while `env.ts`'s re-export block is a deliberate backward-compat shim for the one import path that still routes through it. | | [hierarchy](hierarchy.md) | UI-hierarchy parse contract (`common/src/models/Hierarchy.ts`, consumed by four goal-executor files): `fromJsonString` dispatches array→flat / object→tree, and the paths are deliberately not equivalent — only the flat path shortens `:id/` ids, infers `isImage` from the class, takes `identifier`; alias resolution is `??`-presence via `_pick`/`orDefault`, never truthiness, so `false` and `''` survive; bounds are a 4-array or a left/top/right/bottom object; reads are unvalidated casts. | +| [repo-placeholders](repo-placeholders.md) | `repoPlaceholders.ts` (`packages/common`) — both directions of `${variables.*}`/`${secrets.*}` handling, and the `redactResolvedValue` contract every prompt, report and span write crosses: unanchored substring matching sorted longest-value-first (the substring-leak guard), an existing placeholder token consumed whole by the alternation's LAST branch so a value occurrence still wins there, and no redaction below `MIN_REDACTABLE_SECRET_LENGTH` (3) — with the residuals that leaves. | diff --git a/docs/memory/common/repo-placeholders.md b/docs/memory/common/repo-placeholders.md new file mode 100644 index 0000000..78a2e35 --- /dev/null +++ b/docs/memory/common/repo-placeholders.md @@ -0,0 +1,177 @@ +--- +type: memory +description: "`repoPlaceholders.ts` (`packages/common`) — both directions of `${variables.*}`/`${secrets.*}` handling, and the `redactResolvedValue` contract every prompt, report and span write crosses: unanchored substring matching sorted longest-value-first (the substring-leak guard), an existing placeholder token consumed whole by the alternation's LAST branch so a value occurrence still wins there, and no redaction below `MIN_REDACTABLE_SECRET_LENGTH` (3) — with the residuals that leaves." +--- +# Repo Placeholders (common) + +**Domain**: common + +## Overview + +`packages/common/src/repoPlaceholders.ts` is the repo's whole handling of +`${variables.KEY}` / `${secrets.KEY}` tokens, in both directions: + +- `resolveRuntimePlaceholders(value, bindings)` — forward: substitutes each token with its bound + value (`String(...)` for a variable, the raw string for a secret) and re-emits the literal token + for a key absent from the bindings. +- `containsSecretPlaceholder(value)` — a non-global `${secrets.KEY}` test. +- `redactResolvedValue(value, bindings)` — reverse: rewrites resolved secret *values* back to their + `${secrets.KEY}` placeholder. Only this direction is security-relevant, and it is the only one + with non-obvious semantics. + +Three call sites consume `redactResolvedValue` through the `@finalrun/common` barrel, and one +algorithm serves all of them — so every bound and residual below holds equally for prompts, report +artifacts and spans: + +- `AIAgent._redactPromptText` / `_redactPromptElements` (`packages/goal-executor/src/ai/AIAgent.ts`) + — the LLM prompt path ([/cli/test-compiler.md](/cli/test-compiler.md)). +- `ReportWriter` (`packages/cli/src/reportWriter.ts`) — every string written beneath a run directory, + including the copied device log ([/cli/report-writer.md](/cli/report-writer.md)). +- `ActionExecutor._redactRuntimeString` (`packages/goal-executor/src/ActionExecutor.ts`) — trace-span + `detail` strings and the messages of `TimedActionPhaseFailure`. + +## Requirements + +### Requirement: Secret values match unanchored, longest value first +`redactResolvedValue` MUST match every eligible secret value through a **single** regex alternation +whose value branches are ordered by the longest-value-first sort +(`.sort(([, left], [, right]) => right.length - left.length)`), and the ordering MUST be preserved: +because the alternation picks the first branch that matches at a position, a shorter secret that is a +substring of a longer one would otherwise match inside the longer one's occurrence and leave the rest +of the longer secret raw in the output. That is the substring-leak guard, and +`packages/cli/src/test/testRunner.test.ts` pins it with an `abc`/`abcd` fixture. The source comment +cites that test by path — as the fixture that also fixes `MIN_REDACTABLE_SECRET_LENGTH` — so +relocating the test carries the citation with it. + +Matching MUST stay **unanchored**. `\b`-style word boundaries would stop matching a secret embedded +in concatenated text (`user=xabcd1234y`), which is the same leak class the sort order guards against; +`packages/common/src/test/repoPlaceholders.test.ts` pins the embedded case so a later "cleanup" +cannot silently anchor the pattern. + +Two lesser contract details hold at the boundaries: a falsy `value` (including `undefined` and `''`) +is returned as-is, and when two keys share one secret value the first key in longest-value-first +order owns the placeholder (`placeholderBySecretValue` is filled only where the value is absent). + +#### Scenario: the longer of two overlapping secrets wins +- **GIVEN** bindings `{ secrets: { short: 'abc', long: 'abcd' } }` +- **WHEN** `redactResolvedValue('primary=abcd secondary=abc', bindings)` runs +- **THEN** the result is `primary=${secrets.long} secondary=${secrets.short}` — neither occurrence + leaves a partial value behind + +#### Scenario: an embedded value is still redacted +- **GIVEN** bindings `{ secrets: { KEY: 'abcd1234' } }` +- **WHEN** `redactResolvedValue('x=zabcd1234q', bindings)` runs +- **THEN** the result is `x=z${secrets.KEY}q` + +### Requirement: An existing placeholder token is consumed whole, and a value occurrence still wins +`PLACEHOLDER_TOKEN_ALTERNATIVE` — a module-level, non-capturing mirror of `PLACEHOLDER_PATTERN`, +declared directly beside it — MUST be the **last** branch of the redaction alternation, after every +secret-value branch. Two properties follow, and both are load-bearing: + +- **Tokens survive.** A secret value occurring strictly *inside* a literal `${secrets.KEY}` / + `${variables.KEY}` token cannot match at the token's start position, so absent a value match there + the token branch consumes the token whole and its interior is never exposed to the value branches. + No nested, invalid `${secrets.${secrets.KEY}}` token can be produced. This matters because + placeholder tokens are *expected* content on every consuming path: `testObjective` carries them by + design (the compile-time guard leaves `${secrets.*}` unsubstituted — + [/cli/test-compiler.md](/cli/test-compiler.md)), and report artifacts carry them from authored test + steps such as `Enter ${secrets.email}`. +- **Value occurrences take precedence.** Where a secret value and a token both match at the same + position, the value branch wins — leak-safety over token cosmetics. A secret value that *begins + with* a literal token (e.g. `${secrets.BAR}hunter2`) is therefore redacted whole instead of having + its raw tail left in the output. + +The two patterns MUST be kept in sync by hand rather than derived from `PLACEHOLDER_PATTERN.source`: +that pattern captures, and splicing capturing groups into a composed alternation shifts group +numbering for any future branch that captures. + +Two accepted edges follow from the replacer body (`placeholderBySecretValue.get(match) ?? match`): +a secret whose value is *exactly* a well-formed token is matched by its own value branch and rewritten +placeholder-to-placeholder through the map — harmless, since nothing raw appears on either side; and +consequently a token matched by the *token* branch corresponds to no secret value, so the `?? match` +fallthrough passes it through verbatim. + +#### Scenario: a secret value equal to its own key name leaves the token intact +- **GIVEN** bindings `{ secrets: { PASSWORD: 'PASSWORD' } }` +- **WHEN** `redactResolvedValue('Use ${secrets.PASSWORD} then type PASSWORD manually', bindings)` runs +- **THEN** the result is `Use ${secrets.PASSWORD} then type ${secrets.PASSWORD} manually` — the token + untouched, the prose occurrence redacted, and no nested token + +#### Scenario: a token-prefixed secret value is redacted whole +- **GIVEN** bindings `{ secrets: { FOO: '${secrets.BAR}hunter2' } }` +- **WHEN** `redactResolvedValue('creds=${secrets.BAR}hunter2 end', bindings)` runs +- **THEN** the result is `creds=${secrets.FOO} end` — token protection does not preempt the value + +### Requirement: Values shorter than MIN_REDACTABLE_SECRET_LENGTH are never redacted +The `replacements` filter MUST drop any secret value shorter than the named constant +`MIN_REDACTABLE_SECRET_LENGTH` (3) alongside falsy values, so such a value contributes no branch to +the alternation. A 1–2 character value collides with ordinary text near-certainly: substituting it +rewrites every incidental occurrence of that character sequence and destroys the surrounding string — +in the prompt case, turning model input into garbage. When only short-valued secrets are bound, +`replacements` is empty and the existing early return skips replacement entirely, so the input is +returned byte-identical. + +#### Scenario: a single-character secret value leaves prose alone +- **GIVEN** bindings `{ secrets: { TOKEN: 's' } }` +- **WHEN** `redactResolvedValue('assemble the secrets list', bindings)` runs +- **THEN** the input is returned unchanged — every `s`, and the word `secrets` inside other tokens, + survives + +### Requirement: The residual exposures are stated, not closed +Callers MUST treat prompts, provider-side logs and report artifacts as secret-bearing. Three +residuals are accepted and deliberate: + +- **Short values reach every sink raw.** A 1–2 character secret value is never redacted anywhere — + prompts, report artifacts, spans, error strings. That is a real (documented) exposure, traded + against corrupting arbitrary text; a value that short is also close to inference-proof by nature. +- **Long-enough prose collisions still corrupt.** A value of 3 characters or more that happens to be + a common substring of surrounding prose still rewrites that prose. This is inherent to value-based + substring redaction, now bounded to lengths where a collision indicates a genuine + (mis)configuration rather than a statistical certainty. +- **A value inside a token is skipped.** Where a secret-value occurrence starts strictly inside a + literal placeholder token and no value branch matches at the token's start, that occurrence is + consumed as token protection instead of being redacted. + +Redaction is also exact-match only, so an app-rendered transformation of a value (truncated, +reformatted, partially masked) passes through, and it operates on strings alone — screenshots are +outside this path entirely ([/cli/test-compiler.md](/cli/test-compiler.md), +[/cli/report-writer.md](/cli/report-writer.md)). + +## Design Decisions + +### Redaction semantics live in one shared function, never per call site +**Decision**: Both guards — token pass-through and the minimum redactable length — sit inside +`redactResolvedValue` itself, so all three consumers (prompt path, report writes, spans and error +strings) get identical semantics. +**Why**: The corruption these guards prevent is a property of the matching algorithm, not of any one +call site: every consumer feeds the function text that legitimately contains placeholder tokens, and +every consumer is equally exposed to a short or prose-colliding value. Forking the semantics would +leave two security-relevant implementations of the same rewrite to keep in sync, and the report path +would keep corrupting silently because reports are read by humans rather than parsed. +**Rejected**: Prompt-call-site-only wrappers — they leave the report and span paths producing nested +invalid tokens, and duplicate a security-relevant function. +*Introduced by*: 260731-gz13-fix-redaction-prompt-corruption + +### The placeholder-token branch is last, not first +**Decision**: `PLACEHOLDER_TOKEN_ALTERNATIVE` is appended after every secret-value branch, so value +branches (longest first) are tried before it at each position. +**Why**: Token protection must never preempt redaction of a real secret-value occurrence. With the +token branch first, a value that begins with a literal token (`${secrets.BAR}hunter2`) is consumed as +token protection at the `$` position and its raw tail survives into the output — worse than having no +token protection at all. Placing it last costs nothing for the case token protection exists to fix, +because a value occurring strictly inside a token cannot match at the token's start position anyway. +**Rejected**: Token-branch-first — reads as the natural "protect tokens before touching anything" +ordering, and leaks the tail of every token-prefixed value. +*Introduced by*: 260731-gz13-fix-redaction-prompt-corruption + +### The minimum redactable length is 3, fixed by the pinned fixtures +**Decision**: `MIN_REDACTABLE_SECRET_LENGTH = 3`. +**Why**: 3 is the largest threshold that leaves every pinned fixture byte-untouched — the 3-char +`abc` overlap-ordering secret in `packages/cli/src/test/testRunner.test.ts` and the 4-char `1080` +structural-JSON secret in `packages/goal-executor/src/ai/test/AIAgent.test.ts` — while still +excluding the 1–2 character values that make substitution destructive. The threshold is a named +constant precisely because its value is argued from those fixtures rather than from first principles. +**Rejected**: A threshold of 4 — it would force an edit to the pinned overlap-ordering fixture, which +the constitution's Test Integrity rule forbids; and 1 or 2, which leave the prose-destruction failure +mode open for exactly the values most likely to trigger it. +*Introduced by*: 260731-gz13-fix-redaction-prompt-corruption diff --git a/docs/memory/index.md b/docs/memory/index.md index 9504028..9772329 100644 --- a/docs/memory/index.md +++ b/docs/memory/index.md @@ -20,7 +20,7 @@ fkf_version: "0.1" | [ci](ci/index.md) | PR quality gate (`npm ci` → build → typecheck → test → lint, with no `--if-present` on any workspace fan-out) and its enforcement — a completed verdict per push (no `concurrency` block) with the `test` check required to merge via ruleset 14531661 — plus the path-filtered additive compile-only drivers.yml gate for the Kotlin/Swift drivers, the committed-lockfile reproducible-install contract shared with release.yml, and the explicit-discovery test-runner contract for the Node-20.19 floor | | [cli](cli/index.md) | CLI orchestration, report writing, HTML generation, and the host preflight environment checks | | [cloud-core](cloud-core/index.md) | Cloud submission from packages/cloud-core — the submit pipeline's environment contract and the temp-artifact lifecycle: what each step acquires, where each cleanup scope opens, and who owns release once a path is returned | -| [common](common/index.md) | `packages/common` — the base of the dependency graph, imported by every other package: the UI-hierarchy parse contract that planner and grounder prompts are built from (two deliberately non-equivalent parse paths, presence-based alias resolution, two bounds forms, unvalidated field casts), and the `CliEnv` environment-loading contract (dotenv/OS precedence, falsy-value traps, and where model and reasoning-level validation lives) | +| [common](common/index.md) | `packages/common` — the base of the dependency graph, imported by every other package: the UI-hierarchy parse contract planner and grounder prompts are built from (two non-equivalent parse paths, presence-based alias resolution, two bounds forms, unvalidated casts), the `${variables.*}`/`${secrets.*}` placeholder contract in both directions (with the bounds and residuals of secret redaction), and the `CliEnv` environment-loading contract (dotenv/OS precedence, falsy-value traps) | | [device-node](device-node/index.md) | Device interaction via gRPC: lifecycle, recording, log capture, and the Android/iOS mirror — what the two platform branches share and what stays per-platform | | [drivers](drivers/index.md) | The native on-device drivers under `drivers/` (Kotlin instrumentation host, Swift XCUITest runner) and the `proto/finalrun/driver.proto` gRPC contract they share with the TypeScript client: the unit and default conventions all three implementations must agree on, the per-platform divergences that are deliberate, and the compile-only verification ceiling native code ships under. | | [report-web](report-web/index.md) | Vite React report viewer SPA + importable UI library for test run results | diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl new file mode 100644 index 0000000..cd0bc4b --- /dev/null +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl @@ -0,0 +1,17 @@ +{"action":"enter","driver":"fab-new","event":"stage-transition","stage":"intake","ts":"2026-07-31T17:56:36Z"} +{"args":"Fix verified prompt-corruption bug in secret redaction: redactResolvedValue unanchored substring matching corrupts prompt text (nested tokens when value equals key name; single-char values mangle prose). Decide shared-function vs call-site fix; add regression tests; fix two CodeRabbit doc findings on change 260731-gzzl intake/plan.","cmd":"fab-new","event":"command","ts":"2026-07-31T17:56:36Z"} +{"delta":"+4.2","event":"confidence","score":4.2,"trigger":"calc-score","ts":"2026-07-31T17:59:03Z"} +{"delta":"+0.0","event":"confidence","score":4.2,"trigger":"calc-score","ts":"2026-07-31T18:00:32Z"} +{"cmd":"fab-fff","event":"command","ts":"2026-07-31T18:01:11Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-31T18:01:27Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-31T18:02:34Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-31T18:11:16Z"} +{"event":"review","result":"failed","ts":"2026-07-31T18:22:41Z"} +{"action":"re-entry","driver":"fab-fff","event":"stage-transition","stage":"apply","ts":"2026-07-31T18:22:42Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-31T18:27:51Z"} +{"action":"re-entry","driver":"fab-fff","event":"stage-transition","stage":"review","ts":"2026-07-31T18:35:36Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-31T18:36:28Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"hydrate","ts":"2026-07-31T18:47:52Z"} +{"event":"review","result":"passed","ts":"2026-07-31T18:47:52Z"} +{"cmd":"fab-continue","event":"command","ts":"2026-07-31T18:48:41Z"} +{"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-31T18:55:03Z"} diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml new file mode 100644 index 0000000..8a6acbd --- /dev/null +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml @@ -0,0 +1,51 @@ +id: gz13 +name: 260731-gz13-fix-redaction-prompt-corruption +created: 2026-07-31T17:56:36Z +created_by: ashish-noon +change_type: fix +issues: [] +progress: + intake: done + apply: done + review: done + hydrate: done + ship: active + review-pr: pending +plan: + generated: true + task_count: 11 + acceptance_count: 18 + acceptance_completed: 18 +confidence: + certain: 3 + confident: 5 + tentative: 0 + unresolved: 0 + score: 4.2 + fuzzy: true + dimensions: + signal: 71.3 + reversibility: 80.0 + competence: 81.9 + disambiguation: 72.5 +stage_metrics: + intake: {started_at: "2026-07-31T17:56:36Z", driver: fab-new, iterations: 1, completed_at: "2026-07-31T18:01:27Z"} + apply: {started_at: "2026-07-31T18:22:42Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T18:35:36Z"} + review: {started_at: "2026-07-31T18:35:36Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T18:47:52Z"} + hydrate: {started_at: "2026-07-31T18:47:52Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T18:55:03Z"} + ship: {started_at: "2026-07-31T18:55:03Z", driver: fab-fff, iterations: 1} +prs: [] +change_type_source: explicit +true_impact: + added: 0 + deleted: 0 + net: 0 + tests: + added: 0 + deleted: 0 + net: 0 + computed_at: "2026-07-31T18:55:03Z" + computed_at_stage: hydrate +summary: 'Bound redactResolvedValue''s substring matching: placeholder tokens pass through whole and secret values under 3 chars are never redacted, documented across common/cli memory' +# true_impact: lazily created on first stage-finish that computes it (no placeholder here). +last_updated: 2026-07-31T18:55:03Z diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md new file mode 100644 index 0000000..bcc16df --- /dev/null +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md @@ -0,0 +1,159 @@ +# Intake: Fix Redaction Prompt Corruption + +**Change**: 260731-gz13-fix-redaction-prompt-corruption +**Created**: 2026-07-31 + +## Origin + +One-shot `/fab-new` invocation. The bug was found by CodeRabbit on PR #175 (change `260731-gzzl-fix-secret-prompt-leak-comment`) after it merged, and independently reproduced by the operator against the built dist. User's raw input: + +> Fix a verified prompt-corruption bug in secret redaction, found by CodeRabbit on PR #175 after it merged and independently reproduced by the operator against the built dist. redactResolvedValue in packages/common/src/repoPlaceholders.ts replaces secret VALUES using UNANCHORED substring matching. PR #175 newly pointed it at the LLM prompt path (AIAgent._redactPromptText and _redactPromptElements), including testObjective which deliberately contains literal secrets placeholder tokens. Repro A: secret PASSWORD with value PASSWORD turns the literal token secrets.PASSWORD into a nested invalid token. Repro B, worse and not what CodeRabbit described: secret TOKEN with value s causes every letter s in ordinary prose to be substituted, mangling the whole string including the word secrets inside other tokens. So a single-character or common-substring secret value destroys arbitrary prompt text. A normal long value redacts correctly, so the flaw is specifically the unanchored match. CodeRabbit rated it Minor; operator assessment is must-fix because it can turn model input into garbage. JUDGEMENT REQUIRED: the same shared function is used by packages/cli/src/reportWriter.ts and by ActionExecutor for spans and error strings, where this has existed all along, so decide whether to fix the shared function (changes those paths too, probably correct) or only the prompt call sites, and justify. Do not reintroduce the substring-leak hazard the existing comment describes -- longest-value-first ordering is load-bearing and a test pins it. Add regression tests for value-equals-its-own-key-name, a single-character value, and a value that is a substring of prose. Do not weaken existing tests (constitution Test Integrity). Also fix two CodeRabbit doc findings on that change: intake.md line 16 wrongly says toPromptElementsForPlanner emits node.text (verify what it emits; keep node.text to the Grounder variant), and plan.md line 128 acceptance A-002 wrongly says redaction applies to the assembled text when AIAgent redacts each input BEFORE assembly. Do NOT commit fab/backlog.md. + +Key decisions from the invocation: the corruption fix is must-fix (operator overrode CodeRabbit's Minor rating); the shared-vs-call-site scope decision is judgment-delegated with an explicit lean toward the shared function ("probably correct"); the longest-value-first ordering and its pinning test are load-bearing and must survive; three specific regression tests are required; existing tests must not be weakened (constitution Test Integrity); `fab/backlog.md` must never be committed. + +## Why + +**The pain point.** `redactResolvedValue` (`packages/common/src/repoPlaceholders.ts:27-65`) rewrites every occurrence of every secret *value* to its `${secrets.KEY}` placeholder using a single unanchored regex alternation over the raw values. PR #175 (change `260731-gzzl`) newly wired this into the LLM prompt path — `AIAgent._redactPromptText` (`packages/goal-executor/src/ai/AIAgent.ts:259-262`) and `_redactPromptElements` (`:265-278`) — including `testObjective`, which *deliberately* contains literal `${secrets.KEY}` placeholder tokens (the compile-time guard in `testCompiler.ts` leaves them unsubstituted by design). Two verified failure modes: + +- **Repro A (token nesting):** secret `PASSWORD` with value `PASSWORD`. The unanchored match finds `PASSWORD` *inside* the literal token `${secrets.PASSWORD}` and rewrites it to `${secrets.${secrets.PASSWORD}}` — a nested, invalid token the model has never been taught. +- **Repro B (prose destruction — worse, and not what CodeRabbit described):** secret `TOKEN` with value `s`. Every letter `s` in ordinary prose is substituted with `${secrets.TOKEN}`, mangling the entire string — including the word `secrets` inside *other* placeholder tokens. + +A normal long value redacts correctly, so the flaw is specifically the unanchored substring match applied to (a) text that legitimately contains placeholder tokens and (b) secret values short/common enough to occur incidentally in prose. + +**The consequence of not fixing.** A single short or self-referential secret value silently turns the planner/grounder prompt into garbage — the model receives corrupted objectives, history, and UI-element text, and every downstream action degrades. CodeRabbit rated this Minor; the operator assessment is must-fix precisely because it corrupts *model input*, not just display output. The same corruption has existed all along on the report/span paths (`reportWriter.ts`, `ActionExecutor._redactRuntimeString`), where report artifacts also contain literal placeholder tokens (e.g. test steps like `Enter ${secrets.email}`), so Repro A applies there today too — it just went unnoticed because reports are read by humans, not fed back to a model. + +**Why this approach.** Fix the shared function, not the prompt call sites: the corruption is inherent to the algorithm, not the call site; the report/span paths exhibit the identical defect; and forking redaction semantics per call site would create two divergent security-relevant implementations to keep in sync. The fix must be surgical: protect existing placeholder tokens from rewriting (fixes Repro A at any value length) and stop attempting redaction of values too short to be meaningfully redactable (fixes Repro B), while leaving unanchored matching in place for real-length values — anchoring (e.g. `\b` word boundaries) would *miss* secrets embedded in concatenated text (`x{secret}y`) and reopen the leak the redaction exists to close. + +## What Changes + +### 1. `redactResolvedValue` — protect placeholder tokens from rewriting (fixes Repro A) + +`packages/common/src/repoPlaceholders.ts`. Current pattern construction (lines 55-64): + +```ts +const secretPattern = new RegExp( + replacements + .map(([, secretValue]) => escapeRegExp(secretValue)) + .join('|'), + 'g', +); + +return value.replace(secretPattern, (match) => { + return placeholderBySecretValue.get(match) ?? match; +}); +``` + +Append a non-capturing placeholder-token alternative as the **last** branch of the alternation *(revised in review cycle 1 — the token-FIRST mechanism originally specified here preempted redaction of a secret value that begins with a literal token, leaking its raw tail)*, so the value alternatives are tried first at every position and an existing `${secrets.KEY}` / `${variables.KEY}` token is consumed whole when no value matches there: + +```ts +const secretPattern = new RegExp( + [ + ...replacements.map(([, secretValue]) => escapeRegExp(secretValue)), + PLACEHOLDER_TOKEN_ALTERNATIVE, + ].join('|'), + 'g', +); +``` + +The replacer body is **unchanged**. A secret value occurring strictly *inside* a token (the Repro-A shape) cannot match at the token's start position, so absent a value match there the token branch consumes the token whole and its interior is never visible to the value alternatives; a value occurrence that *does* match at the same position wins over token protection — leak-safety over token cosmetics. Longest-value-first ordering among the value alternatives is untouched — the substring-leak guard and its pinning test (`packages/cli/src/test/testRunner.test.ts:144-154`, `redactResolvedValue preserves complete placeholders when secrets overlap`) remain byte-identical. + +Edge cases (accepted, document in the comment; verified against the shipped code): a secret whose *value* is *exactly* a well-formed token is matched by its own value alternative and rewritten placeholder→placeholder via the map — harmless (nothing raw on either side) and identical to pre-change behavior; consequently a token matched by the token branch matches no secret value, so the replacer's `placeholderBySecretValue.get(match) ?? match` falls through and passes it through verbatim. + +The token alternative mirrors `PLACEHOLDER_PATTERN` (line 3) but non-capturing; keep them adjacent and cross-reference in a comment rather than deriving one from the other with string surgery on `.source` (the capture groups differ). + +### 2. `redactResolvedValue` — minimum redactable value length (fixes Repro B) + +Extend the existing filter (line 42) to skip secret values shorter than **3 characters**: + +```ts +const MIN_REDACTABLE_SECRET_LENGTH = 3; +// ... +const replacements = Object.entries(bindings.secrets) + .filter( + ([, secretValue]) => + Boolean(secretValue) && secretValue.length >= MIN_REDACTABLE_SECRET_LENGTH, + ) + .sort(([, left], [, right]) => right.length - left.length); +``` + +Rationale for the threshold value: a 1–2 character "secret" occurs incidentally throughout arbitrary text, so substituting it destroys the string while providing no protection (the value remains trivially inferable). Threshold 3 is the **largest value that leaves every existing pinned test byte-untouched**: the substring-leak test uses a 3-char secret (`abc`, `testRunner.test.ts:147`) and the structural-JSON test uses a 4-char secret (`1080`, `AIAgent.test.ts:650`). Raising the threshold to 4 would require editing the pinned overlap-ordering fixture, which the instruction ("do not weaken existing tests") and constitution Test Integrity both counsel against. Residual (accepted, must be documented in the comment): a ≥3-char value that happens to be a common substring of prose still corrupts that prose — inherent to value-based substring redaction and now bounded to values long enough that a collision is a genuine (mis)configuration rather than a statistical certainty. + +Behavioral consequence to document: secrets with 1–2 char values are now **never redacted anywhere** (prompts, report artifacts, spans, error strings). Previously they were "redacted" by mangling every occurrence of that character sequence in the surrounding text — strictly worse. With only short-valued secrets bound, `replacements` is empty and the existing early return (lines 44-46) skips replacement entirely. + +### 3. No word-boundary anchoring for surviving values + +Deliberate non-change: values ≥3 chars keep **unanchored** matching. Adding `\b`-style anchors would stop matching secrets embedded in concatenated text (`user=xabcd1234y` with secret `abcd1234`) — a leak regression on the report path, the exact hazard class the existing comment warns about. Add a pinning test (see §4) so a future "cleanup" cannot silently anchor the pattern. + +### 4. Regression tests + +New unit-test file `packages/common/src/test/repoPlaceholders.test.ts` (node:test + `node:assert/strict`, matching sibling tests in that directory), covering the shared function directly: + +1. **Value equals its own key name** (Repro A): secrets `{ PASSWORD: 'PASSWORD' }`, input `Use ${secrets.PASSWORD} then type PASSWORD manually` → `Use ${secrets.PASSWORD} then type ${secrets.PASSWORD} manually` — the existing token is untouched; the prose occurrence of the value is still redacted. +2. **Single-character value** (Repro B): secrets `{ TOKEN: 's' }`, prose input (e.g. `assemble the secrets list`) is returned **unchanged**. +3. **Value that is a substring of prose**: a 2-char value (e.g. `{ AB: 'es' }`), input `these tests pass` returned unchanged. +4. **Embedded long value still redacted** (anti-anchoring pin): secret `{ KEY: 'abcd1234' }`, input `x=zabcd1234q` → `x=z${secrets.KEY}q`. +5. **Token protection composes with overlap ordering**: overlapping secrets (≥3 chars each) plus a literal token in the same input — longest wins outside the token, token survives verbatim. + +Prompt-path integration tests appended to `packages/goal-executor/src/ai/test/AIAgent.test.ts` (existing redaction block starts at line 526): + +6. `testObjective` containing a literal `${secrets.PASSWORD}` token with secret value `PASSWORD` reaches the planner prompt with the token intact (no nesting). +7. A single-char secret value leaves the assembled planner prompt text byte-identical to the unredacted assembly. + +All existing tests pass **unmodified** — no fixture edits, no assertion changes (constitution Test Integrity; explicit instruction). + +### 5. Comment updates falsified by the fix + +Per the descriptive-claims discipline (dominant defect class in this repo), update the comments whose claims the fix changes — and only those: + +- `packages/common/src/repoPlaceholders.ts:35-40` (substring-leak guard comment): still true, but extend to state the two new behaviors — existing placeholder tokens are protected by the trailing token branch (value occurrences win at the same position — see §1 as revised in review cycle 1), and values under `MIN_REDACTABLE_SECRET_LENGTH` are never redacted (with the why). +- `packages/goal-executor/src/ai/AIAgent.ts:233-258` (`_redactPromptText` doc): the "Exact-match is also the ceiling" paragraph gains the second ceiling — values shorter than 3 chars are never redacted, because substituting them corrupts arbitrary text without protecting anything. +- `packages/cli/src/testCompiler.ts:31` area ("redactResolvedValue also guards the write paths…"): verify at apply whether its claims survive; amend only if the min-length skip falsifies a stated guarantee. + +### 6. Two doc corrections on change `260731-gzzl` artifacts (CodeRabbit findings, verified at intake) + +Both verified against current code: + +- **`fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md:16`** claims "`Hierarchy.toPromptElementsForGrounder` … and `toPromptElementsForPlanner` emit `node.text` verbatim." False for the planner: `toPromptElementsForPlanner` (`packages/common/src/models/Hierarchy.ts:180-200`) emits only `index`, `contentDesc` (from `accessibilityText`), `class`, and `bounds` — no `text` field. Only the Grounder variant (`:213-230`, specifically `:216`) emits `node.text`. Correct the sentence to attribute `node.text` emission to the Grounder variant alone (the planner's slim payload can still echo a value via `contentDesc`, but that is a different field and the line must not claim `node.text`). +- **`fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md:127`, acceptance A-002** (CodeRabbit cited line 128; the verified current location is 127) says redaction covers "element fields pre-serialization **plus the assembled text**." False: `AIAgent` redacts each input individually *before* assembly, and the shipped doc comment (`AIAgent.ts:244-246`) states there is deliberately NO pass over the assembled prompt text. Rewrite the parenthetical to "(element fields pre-serialization and each free-text input before assembly; deliberately no pass over the assembled text)". + +These are text-only corrections to a completed change's artifacts (same class as PR #176's false-claim corrections); checkbox states and everything else stay untouched. + +### Non-change: `resolveRuntimePlaceholders` and `containsSecretPlaceholder` + +Both untouched — the bug is exclusively in the value→placeholder reverse direction. + +## Affected Memory + +- `common/repo-placeholders`: (new) the `redactResolvedValue` contract — unanchored substring semantics, longest-value-first substring-leak guard, placeholder-token pass-through, minimum redactable length and its residuals (short values never redacted anywhere; ≥3-char prose collisions still corrupt) +- `cli/report-writer`: (modify) the secret-redaction contract it documents gains the two new bounds (token pass-through, min-length skip applies to report artifacts/spans too) +- `cli/test-compiler`: (modify) its description of prompt-path redaction residuals gains the min-length exclusion alongside screenshots/re-rendered values/INFO output + +## Impact + +- `packages/common/src/repoPlaceholders.ts` — the fix (filter + pattern; replacer and sort untouched) +- `packages/common/src/test/repoPlaceholders.test.ts` — new unit-test file +- `packages/goal-executor/src/ai/test/AIAgent.test.ts` — added prompt-path regression tests +- `packages/goal-executor/src/ai/AIAgent.ts`, `packages/cli/src/testCompiler.ts` — comment-only updates +- `fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md`, `plan.md` — two text-only corrections +- Behavior shift for **all** `redactResolvedValue` consumers (AIAgent prompts, reportWriter artifacts, ActionExecutor spans/errors): inputs containing literal placeholder tokens no longer corrupt; 1–2 char secret values are no longer redacted (previously they mangled the text instead) +- No API/signature changes; no changes to placeholder resolution +- `fab/backlog.md` is never staged or committed + +## Open Questions + +*(none — the invocation resolved scope, constraints, and required tests; the delegated judgment calls are recorded as graded assumptions below)* + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Confident | Fix the shared `redactResolvedValue`, not just the prompt call sites | Judgment delegated with an explicit lean ("probably correct"); verified the identical defect exists on report/span paths (report content carries literal `${secrets.*}` tokens from test steps), and forking redaction semantics would duplicate a security-relevant function | S:85 R:60 A:80 D:75 | +| 2 | Confident | Fix Repro A via a placeholder-token alternative in the redaction regex (revised to the LAST branch in review cycle 1 — token-FIRST leaked the raw tail of token-prefixed values); replacer unchanged | Minimal, mechanism-precise: a value strictly inside a token cannot match at the token's start, so the trailing token branch consumes tokens whole; preserves longest-first ordering and the pinned overlap test byte-identically | S:70 R:75 A:85 D:70 | +| 3 | Confident | Fix Repro B via `MIN_REDACTABLE_SECRET_LENGTH = 3` (skip 1–2 char values) | Threshold chosen as the largest value leaving pinned fixtures (`abc` 3-char, `1080` 4-char) untouched per Test Integrity; 1–2 char values are unprotectable by substitution; residual ≥3-char prose collision documented and accepted | S:50 R:80 A:55 D:40 | +| 4 | Certain | Doc-fix targets verified: gzzl `intake.md:16` (planner emits `contentDesc`/`class`/`bounds`, never `text`) and `plan.md:127` A-002 (redaction is per-input pre-assembly; CodeRabbit's cited line 128 is off by one) | Both claims re-verified against `Hierarchy.ts:180-230` and `AIAgent.ts:244-246` at intake | S:90 R:90 A:95 D:90 | +| 5 | Confident | New unit tests live in `packages/common/src/test/repoPlaceholders.test.ts`; prompt-path tests in `AIAgent.test.ts`; existing tests untouched | The function is owned by `packages/common` (sibling test dir exists); the lone existing unit test sits in cli's `testRunner.test.ts` and stays there untouched | S:45 R:90 A:75 D:60 | +| 6 | Confident | No word-boundary anchoring for values ≥3 chars; add an embedded-value pinning test | Anchoring would miss secrets embedded in concatenated text — a leak regression of the hazard class the load-bearing comment warns about | S:60 R:70 A:80 D:65 | +| 7 | Certain | `fab/backlog.md` is never committed or staged | Explicit instruction, repeated across recent changes | S:100 R:90 A:100 D:100 | +| 8 | Certain | Update the shipped comments whose claims the fix falsifies (repoPlaceholders guard comment, AIAgent redaction doc, testCompiler write-path note if affected) | Descriptive-claims rot is this repo's dominant defect class; the fix changes the documented redaction contract | S:70 R:85 A:85 D:80 | + +8 assumptions (3 certain, 5 confident, 0 tentative, 0 unresolved). diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md new file mode 100644 index 0000000..635bf8f --- /dev/null +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md @@ -0,0 +1,193 @@ +# Plan: Fix Redaction Prompt Corruption + +**Change**: 260731-gz13-fix-redaction-prompt-corruption +**Intake**: `intake.md` + +## Requirements + +### common: `redactResolvedValue` corruption fix + +#### R1: Existing placeholder tokens survive redaction verbatim (Repro A) +`redactResolvedValue` (`packages/common/src/repoPlaceholders.ts`) MUST NOT rewrite the interior of an existing `${secrets.KEY}` / `${variables.KEY}` placeholder token. Implemented by appending the non-capturing alternative `\$\{(?:variables|secrets)\.[A-Za-z0-9_-]+\}` as the **LAST** branch of the redaction regex alternation, hoisted to a module-level constant declared beside `PLACEHOLDER_PATTERN` (adjacency makes the keep-in-sync instruction self-enforcing); the replacer body stays unchanged. The token alternative mirrors `PLACEHOLDER_PATTERN` but MUST NOT be derived from it via `.source` string surgery (the capture groups differ). + +Branch-order rationale (the FIRST-branch variant shipped in rework cycle 0 and was rejected by review as a redaction regression): with the token branch tried **last**, a secret value whose text *begins with* a literal token (e.g. value `${secrets.BAR}hunter2`) is still matched by its own earlier value alternative and redacted whole; token-first preempts it at the `$` position and the raw tail leaks. Repro A stays fixed either way: a value occurring strictly *inside* a token cannot match at the token's start position, so the token branch consumes the token whole before the scan ever reaches the interior. Precedence rule (documented in the comment): a secret-value occurrence always wins over token protection when both match at the same position — leak-safety over token cosmetics. + +Accepted edges (documented in the comment, claims verified against the shipped code): (a) a secret whose value is *exactly* a well-formed token is rewritten placeholder→placeholder via the map lookup — harmless (nothing raw on either side) and identical to pre-change behavior; (b) a token matched by the token branch never corresponds to any secret value (those matched earlier), so the replacer's `get(match) ?? match` falls through to `match` and passes it through verbatim. + +- **GIVEN** bindings `{ secrets: { PASSWORD: 'PASSWORD' } }` +- **WHEN** `redactResolvedValue('Use ${secrets.PASSWORD} then type PASSWORD manually', bindings)` runs +- **THEN** the result is `Use ${secrets.PASSWORD} then type ${secrets.PASSWORD} manually` — the existing token untouched, the prose occurrence still redacted, no nested `${secrets.${secrets.PASSWORD}}` token + +- **GIVEN** bindings `{ secrets: { FOO: '${secrets.BAR}hunter2' } }` +- **WHEN** `redactResolvedValue('creds=${secrets.BAR}hunter2 end', bindings)` runs +- **THEN** the result is `creds=${secrets.FOO} end` — the token-prefixed value is redacted whole, not preempted by token protection + +#### R2: Secret values shorter than 3 characters are never redacted (Repro B) +`redactResolvedValue` MUST skip secret values shorter than a named constant `MIN_REDACTABLE_SECRET_LENGTH = 3` in its `replacements` filter. Rationale (documented in the comment, worded accurately — a 2-char secret is still a real secret): a 1–2 char value collides with ordinary text near-certainly, so substituting it corrupts arbitrary strings; the trade accepted here is that short secret values now reach prompts, report artifacts, and spans raw — an accepted, documented residual exposure, not "protecting nothing". 3 is the largest threshold that leaves every pinned fixture byte-untouched (3-char `abc` in `testRunner.test.ts`, 4-char `1080` in `AIAgent.test.ts`). Behavioral consequence (documented): 1–2 char secret values are now never redacted anywhere — prompts, report artifacts, spans, error strings. With only short-valued secrets bound, `replacements` is empty and the existing early return skips replacement entirely. Residual (accepted, documented): a ≥3-char value that is a common substring of prose still corrupts that prose. + +- **GIVEN** bindings `{ secrets: { TOKEN: 's' } }` +- **WHEN** `redactResolvedValue('assemble the secrets list', bindings)` runs +- **THEN** the input is returned unchanged + +#### R3: Unanchored matching and longest-value-first ordering are preserved +Values ≥ 3 chars MUST keep unanchored substring matching — no `\b`-style anchoring (anchors would miss secrets embedded in concatenated text like `x=zabcd1234q`, a leak regression of the hazard class the existing guard comment warns about). The longest-value-first `.sort(([, left], [, right]) => right.length - left.length)` MUST remain byte-identical, and the pinned overlap test (`packages/cli/src/test/testRunner.test.ts:144-154`) MUST pass unmodified. A new pinning test MUST cover the embedded-long-value case so a future "cleanup" cannot silently anchor the pattern. + +- **GIVEN** bindings `{ secrets: { KEY: 'abcd1234' } }` +- **WHEN** `redactResolvedValue('x=zabcd1234q', bindings)` runs +- **THEN** the result is `x=z${secrets.KEY}q` + +### common: unit-test coverage of the shared function + +#### R4: New unit-test file pins the fixed contract +A new test file `packages/common/src/test/repoPlaceholders.test.ts` (node:test + `node:assert/strict`, matching sibling tests in that directory) MUST cover the shared function directly with six cases: (1) value-equals-its-own-key-name (R1 scenario), (2) single-character value returns prose unchanged (R2 scenario), (3) a 2-char value that is a substring of prose returns input unchanged, (4) embedded long value still redacted (R3 anti-anchoring pin), (5) token protection composes with overlap ordering — overlapping ≥3-char secrets plus a literal token in one input, where the token MUST be **discriminating**: its interior must contain one of the overlapping values (e.g. token `${secrets.abc}` with secrets `abc`/`abcd`), so the case fails against the pre-fix implementation (pre-fix → nested `${secrets.${secrets.…}}`, post-fix → token intact, longest still wins outside the token), (6) token-prefix regression pin (R1 second scenario): a secret value beginning with a literal token (`${secrets.BAR}hunter2`) is redacted whole, never left raw. + +- **GIVEN** the new test file +- **WHEN** the `packages/common` suite runs +- **THEN** all six cases pass against the fixed implementation; cases (1) and (5) fail against the pre-change implementation (nested-token corruption), and case (6) fails against the rejected token-FIRST variant (raw-tail leak) + +### goal-executor: prompt-path integration coverage + +#### R5: Prompt-path regression tests in AIAgent.test.ts +Two tests appended to the existing redaction block in `packages/goal-executor/src/ai/test/AIAgent.test.ts` MUST verify the fix end-to-end at the prompt seam: (6) a `testObjective` containing a literal `${secrets.PASSWORD}` token with secret value `PASSWORD` reaches the planner prompt with the token intact (no nesting), and (7) a single-char secret value leaves the assembled planner prompt text byte-identical to the unredacted assembly. All existing tests pass unmodified — no fixture edits, no assertion changes (constitution Test Integrity). + +- **GIVEN** an `AIAgent` with bindings `{ secrets: { PASSWORD: 'PASSWORD' } }` and a `testObjective` containing `${secrets.PASSWORD}` +- **WHEN** the planner prompt is built +- **THEN** the prompt contains the literal token and never `${secrets.${secrets.PASSWORD}}` + +### docs: comments falsified by the fix + +#### R6: Update exactly the comments the fix falsifies +Three comment sites, and only those: (a) the substring-leak guard comment in `repoPlaceholders.ts` gains the two new behaviors — the token branch tried last (with the precedence rule and both accepted edges from R1, exactly as verified: token-valued secrets rewrite placeholder→placeholder; only tokens matching no secret value fall through `?? match`) and values under `MIN_REDACTABLE_SECRET_LENGTH` never redacted (with R2's accurately-worded residual-exposure rationale — no "protecting nothing" absolutes); (b) the `_redactPromptText` doc in `packages/goal-executor/src/ai/AIAgent.ts` gains the second ceiling in its "Exact-match is also the ceiling" paragraph — values shorter than 3 chars are never redacted and reach the provider raw, the accepted trade for not corrupting arbitrary text; (c) the `packages/cli/src/testCompiler.ts:31` area is amended only if the min-length skip actually falsifies a stated guarantee (verified at apply: it does — one residual clause added). Every comment claim states what the code cannot show (deletion test) and none is falsified by the shipped code — this is the criterion review cycle 1 failed on. + +- **GIVEN** the fixed implementation +- **WHEN** each updated comment is read against the shipped code +- **THEN** no claim is falsified by the fix and no unrelated comment is touched + +#### R7: Two text-only corrections in change `260731-gzzl`'s artifacts +(a) `fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md:16` MUST attribute `node.text` emission to the Grounder variant only — `toPromptElementsForPlanner` emits `index`/`contentDesc`/`class`/`bounds`, no `text` field — and the same sentence's citation `Hierarchy.ts:214-215` MUST be corrected to `:216`, where `node.text` is actually emitted (review nice-to-have 2: the corrected sentence must not keep a stale line number). (b) `fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md:127` acceptance A-002 MUST state that redaction is per-input BEFORE assembly with deliberately no pass over the assembled text. Nothing else in those files changes — checkbox states stay. (c) This change's own `intake.md` accepted-edge sentence (the "nothing raw leaks" claim in § What Changes item 1, ~line 60) MUST be corrected to the verified R1 accepted edges — the intake is the state-transfer document and must not carry a claim review proved false. + +- **GIVEN** the two corrected lines +- **WHEN** compared against `Hierarchy.ts:180-230` and `AIAgent.ts:244-246` +- **THEN** both claims are true of the shipped code and the rest of both files is byte-identical + +### Non-Goals + +- No changes to `resolveRuntimePlaceholders` or `containsSecretPlaceholder` — the bug is exclusively in the value→placeholder reverse direction +- No word-boundary anchoring — a leak regression, deliberately excluded (R3) +- No modification of any existing test (constitution Test Integrity) +- No commits during apply; `fab/backlog.md` is never staged or committed + +### Design Decisions + +#### Fix the shared function, not the prompt call sites +**Decision**: Both fixes land in the shared `redactResolvedValue`, changing behavior for all consumers (AIAgent prompts, reportWriter artifacts, ActionExecutor spans/errors). +**Why**: The corruption is inherent to the algorithm, not the call site; the report/span paths exhibit the identical Repro-A defect today (report artifacts carry literal `${secrets.*}` tokens from test steps); forking redaction semantics per call site would create two divergent security-relevant implementations. +**Rejected**: Prompt-call-site-only wrappers — leaves the report path corrupting and duplicates a security-relevant function. +*Introduced by*: 260731-gz13-fix-redaction-prompt-corruption + +#### Token branch last, not first +**Decision**: The placeholder-token alternative is the LAST branch of the redaction alternation; secret-value alternatives (longest-first) are tried before it at every position. +**Why**: Token protection must never preempt redaction of a real secret-value occurrence. A value beginning with a literal token (`${secrets.BAR}hunter2`) matches its own alternative before the token branch and is redacted whole; Repro A is unaffected because a value strictly inside a token cannot match at the token's start position, so the token branch still consumes the token whole. Leak-safety over token cosmetics. +**Rejected**: Token-FIRST (rework cycle 0's implementation) — reviewed and rejected: it left the raw tail of token-prefixed values in the output, a redaction regression relative to the pre-change code, and forced comment claims the code falsifies. +*Introduced by*: 260731-gz13-fix-redaction-prompt-corruption + +#### Threshold 3, chosen by the pinned fixtures +**Decision**: `MIN_REDACTABLE_SECRET_LENGTH = 3`. +**Why**: The largest value that leaves every existing pinned test byte-untouched (3-char `abc` overlap fixture, 4-char `1080` structural-JSON fixture); 1–2 char values are unprotectable by substitution anyway. +**Rejected**: Threshold 4 — would require editing the pinned overlap-ordering fixture, prohibited by Test Integrity and the explicit do-not-weaken instruction. +*Introduced by*: 260731-gz13-fix-redaction-prompt-corruption + +## Tasks + +### Phase 1: Core Implementation + +- [x] T001 Add `MIN_REDACTABLE_SECRET_LENGTH = 3` and extend the `replacements` filter in `packages/common/src/repoPlaceholders.ts` to skip shorter values; sort stays byte-identical +- [x] T002 Move the non-capturing placeholder-token alternative `\$\{(?:variables|secrets)\.[A-Za-z0-9_-]+\}` to the LAST branch of `secretPattern` in `packages/common/src/repoPlaceholders.ts` and hoist it to a module-level constant beside `PLACEHOLDER_PATTERN`; replacer body unchanged +- [x] T003 Rewrite the substring-leak guard comment in `packages/common/src/repoPlaceholders.ts` per revised R6(a): token branch last with the precedence rule and the two verified accepted edges, min-length skip with the residual-exposure wording; delete the falsified "never a key of placeholderBySecretValue" and "nothing raw leaks" claims + +### Phase 2: Tests + +- [x] T004 [P] Update `packages/common/src/test/repoPlaceholders.test.ts` to the six unit cases of revised R4: make case 5's token discriminating (`${secrets.abc}` — interior contains an overlapping value) and add case 6 (token-prefixed value `${secrets.BAR}hunter2` redacted whole) +- [x] T005 [P] Append the two prompt-path regression tests (token-intact objective; single-char value byte-identical assembly) to the redaction block in `packages/goal-executor/src/ai/test/AIAgent.test.ts` + +### Phase 3: Comment & Doc Corrections + +- [x] T006 [P] Add the min-length second ceiling to the `_redactPromptText` doc "Exact-match is also the ceiling" paragraph in `packages/goal-executor/src/ai/AIAgent.ts` +- [x] T007 [P] Verify the `packages/cli/src/testCompiler.ts:31` area claims against the fixed code; amend only the clause the min-length skip falsifies +- [x] T008 [P] Correct `fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md:16` — attribute `node.text` emission to the Grounder variant only (planner emits `index`/`contentDesc`/`class`/`bounds`, no `text`) and fix the same sentence's citation `Hierarchy.ts:214-215` → `:216`; touch nothing else +- [x] T009 [P] Correct `fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md:127` A-002 — redaction is per-input before assembly, deliberately no pass over the assembled text; checkbox state stays +- [x] T011 [P] Correct this change's own `intake.md` accepted-edge sentence (§ What Changes item 1, ~line 60): replace the falsified "passes through unredacted — nothing raw leaks" claim with the verified R1 accepted edges (exact-token value rewrites placeholder→placeholder; token-prefixed value redacted whole under token-LAST) + +### Phase 4: Verification + +- [x] T010 Build + typecheck the workspaces and run the `packages/common`, `packages/cli`, and `packages/goal-executor` test suites; confirm every pre-existing test passes unmodified and `fab/backlog.md` is unstaged + +## Execution Order + +- T001 and T002 block T004/T005 (tests exercise the fixed function) and T003 (the comment describes the final shipped behavior) +- T010 runs last + +## Acceptance + +### Functional Completeness + +- [x] A-001 R1: The redaction regex's **last** alternation branch is the non-capturing placeholder-token pattern, hoisted to a module-level constant beside `PLACEHOLDER_PATTERN`; an input containing a literal `${secrets.KEY}` token is never rewritten inside the token; a secret value beginning with a literal token is redacted whole; and the replacer body is unchanged +- [x] A-002 R2: `MIN_REDACTABLE_SECRET_LENGTH = 3` exists as a named constant and the `replacements` filter skips shorter values; a binding set containing only short values takes the existing empty-`replacements` early return +- [x] A-003 R4: `packages/common/src/test/repoPlaceholders.test.ts` exists with the six specified cases, all passing; case 5's token is discriminating (fails pre-fix) and case 6 pins the token-prefix leak (fails token-FIRST) +- [x] A-004 R5: The two new AIAgent prompt-path tests exist in the existing redaction block and pass +- [x] A-005 R7: The gzzl `intake.md:16` sentence attributes `node.text` to the Grounder variant only with the citation corrected to `Hierarchy.ts:216`, and gzzl `plan.md:127` A-002 describes per-input pre-assembly redaction with no assembled-text pass; this change's own `intake.md` accepted-edge sentence states the verified edges; no other line in any of the three files changed + +### Behavioral Correctness + +- [x] A-006 R1: `redactResolvedValue('Use ${secrets.PASSWORD} then type PASSWORD manually', { secrets: { PASSWORD: 'PASSWORD' }, variables: {} })` returns `Use ${secrets.PASSWORD} then type ${secrets.PASSWORD} manually` +- [x] A-007 R2: A single-char and a 2-char secret value leave arbitrary prose byte-identical through `redactResolvedValue` +- [x] A-008 R3: The longest-value-first sort is byte-identical to before the change and the pinned overlap test (`testRunner.test.ts:144-154`) passes unmodified + +### Scenario Coverage + +- [x] A-009 R5: A `testObjective` carrying a literal `${secrets.PASSWORD}` token with secret value `PASSWORD` reaches the planner prompt token-intact (no `${secrets.${secrets.` nesting), proven by test +- [x] A-010 R5: With a single-char secret bound, the assembled planner prompt is byte-identical to a bindings-less agent's assembly of the same request, proven by test + +### Edge Cases & Error Handling + +- [x] A-011 R3: An embedded long value (`x=zabcd1234q`, secret `abcd1234`) is still redacted — the anti-anchoring pin exists as a test +- [x] A-012 R1: Token protection composes with overlap ordering — overlapping ≥3-char secrets plus a **discriminating** literal token (`${secrets.abc}`, interior contains an overlapping value) in one input: longest wins outside the token, token verbatim, and the case fails against the pre-fix implementation (rework of cycle-1 should-fix 1: the original `${secrets.short}` fixture passed pre-fix byte-identically and pinned nothing) + +### Code Quality + +- [x] A-013 Pattern consistency: New test file matches sibling `packages/common/src/test` conventions (node:test, `node:assert/strict`, `.js` import specifiers); implementation matches surrounding style +- [x] A-014 No unnecessary duplication: The token alternative is a module-level constant declared directly beside `PLACEHOLDER_PATTERN` (physical adjacency, not just a comment pointer — rework of the cycle-1 nice-to-have), not derived via `.source` surgery, and no redaction logic is duplicated per call site +- [x] A-015 No magic numbers: The length threshold is the named constant `MIN_REDACTABLE_SECRET_LENGTH` +- [x] A-016 Comment content: Every added/updated comment passes the deletion test (rationale, cross-file couplings, accepted residuals — nothing restating adjacent code); no existing rationale claim was deleted; no comment claim is falsified by the shipped code (cycle-1 failure criterion: the "never a key of placeholderBySecretValue" / "nothing raw leaks" claims — both must be gone, replaced by the verified R1 edges and R2's residual-exposure wording) + +### Security + +- [x] A-017 R3: No word-boundary anchoring was introduced; unanchored matching for ≥3-char values is preserved and pinned; the cycle-1 token-prefix leak (value `${secrets.OLD}tail` surviving raw under token-FIRST) is gone under token-LAST and pinned by unit case 6; and no redaction regression exists beyond the two intended R1/R2 trades — 1–2 char values pass through raw (R2's accepted residual), and a value occurrence starting strictly inside a literal token is skipped when no value alternative matches at that position (R1's token protection working as specified) — *cycle-1 re-review*: verified by 26-case differential probe + 200k-input fuzz against the pre-change implementation; wording narrowed from "no regression of any kind" per that review's should-fix 3 + +### Removal Verification + +- [x] A-018 **N/A**: No requirements are deprecated by 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 + +- `packages/cli/src/test/testRunner.test.ts:144-154` (`redactResolvedValue preserves complete placeholders when secrets overlap`) — the overlap-ordering pin now has an equivalent in the owning package (`packages/common/src/test/repoPlaceholders.test.ts:44-57`, which composes the same `abc`/`abcd` fixture with a discriminating literal token), so the cli copy is a cross-package test of a `@finalrun/common` function. **Do not delete in this change**: constitution Test Integrity forbids touching it, and `repoPlaceholders.ts:37-38` cites it by path as the fixture that fixes `MIN_REDACTABLE_SECRET_LENGTH = 3` — any relocation must move that citation too. +- No production code became redundant: both fixes are additive guards inside `redactResolvedValue`; the replacer body, the sort, `resolveRuntimePlaceholders`, and `containsSecretPlaceholder` are all still reached. + +## Assumptions + +| # | Grade | Decision | Rationale | Scores | +|---|-------|----------|-----------|--------| +| 1 | Confident | The `testCompiler.ts:31`-area comment IS amended: its claim that the prompt seam "redacts exact occurrences of resolved secret values" is falsified by the min-length skip (a 1–2 char occurrence is an exact match yet now passes through), so a minimal residual clause is added | The comment's own design decision (test-compiler memory) sets its accuracy standard as the highest in the file; the amendment is one clause, not a rewrite | S:70 R:85 A:80 D:70 | +| 2 | Confident | Test 7's "byte-identical to the unredacted assembly" is asserted by comparing the redacting agent's `textPrompt` against a bindings-less agent's `textPrompt` for the same request | Strongest available formulation — pins the whole assembled text, not selected substrings; prompt assembly is deterministic for a fixed request | S:60 R:90 A:80 D:70 | +| 3 | Certain | The two new AIAgent tests are appended at the end of the existing "Prompt-path secret redaction" block (before the retry section), reusing `makeAgent`/`buildPlannerPrompt` helpers | Purely structural; the intake names the block and the helpers exist | S:75 R:95 A:95 D:90 | +| 4 | Confident | Unit test 5 composes the pinned overlap fixture's secret pair (`abc`/`abcd`) with a literal token in one input | Reuses the exact overlap semantics the load-bearing cli test pins, making the composition test directly comparable to it | S:55 R:90 A:80 D:70 | +| 5 | Certain | A-005's "no other line changed" is read as: the two gzzl files change exactly one line each (verified via diff), while this change's own intake additionally updates the § What Changes item-1 mechanism paragraph and snippet — the explicitly mandated cycle-1 revision, not an "other" change | The rework dispatch orders the snippet update by name; both gzzl diffs are +1/−1 | S:75 R:90 A:85 D:80 | +| 6 | Certain | The plan's Deletion Candidates citation `repoPlaceholders.test.ts:44-54` is updated to `:44-57` (case 5 grew by its discriminating-token comment) | Descriptive-claims discipline: the rework shifted the cited block's line span; leaving it stale reintroduces the repo's dominant defect class | S:65 R:95 A:90 D:85 | + +6 assumptions (3 certain, 3 confident, 0 tentative). 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 index 6c94d4b..47c93c8 100644 --- a/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md +++ b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md @@ -13,7 +13,7 @@ Key decisions from the invocation: the comment rewrite is REQUIRED and must ship ## 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 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:216`) emits `node.text` verbatim (`toPromptElementsForPlanner` emits only `index`/`contentDesc`/`class`/`bounds` — no `text` field); `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. 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 index ba00c9b..ca05edc 100644 --- a/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md +++ b/fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md @@ -124,7 +124,7 @@ This change MUST NOT `git add` or commit `fab/backlog.md` at any stage. Apply co ### 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-002 R3: `AIAgent` accepts optional bindings and redacts exact resolved-secret occurrences from grounder and planner prompt text (element fields pre-serialization and each free-text input before assembly; deliberately no pass over 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 diff --git a/packages/cli/src/testCompiler.ts b/packages/cli/src/testCompiler.ts index f3deab8..6825749 100644 --- a/packages/cli/src/testCompiler.ts +++ b/packages/cli/src/testCompiler.ts @@ -27,7 +27,10 @@ import type { TestDefinition, RuntimeBindings } from '@finalrun/common'; // 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. +// reformatted, partially masked by the app) passes through — as does any +// secret value shorter than MIN_REDACTABLE_SECRET_LENGTH (3 chars, in +// repoPlaceholders.ts): redaction skips 1-2 char values everywhere rather +// than mangle every incidental occurrence in the surrounding text. // 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 diff --git a/packages/common/src/repoPlaceholders.ts b/packages/common/src/repoPlaceholders.ts index 8e38308..2f15797 100644 --- a/packages/common/src/repoPlaceholders.ts +++ b/packages/common/src/repoPlaceholders.ts @@ -2,6 +2,14 @@ import type { RuntimeBindings } from './models/Environment.js'; const PLACEHOLDER_PATTERN = /\$\{(variables|secrets)\.([A-Za-z0-9_-]+)\}/g; +// Non-capturing mirror of PLACEHOLDER_PATTERN above, used as the final branch +// of redactResolvedValue's alternation. Keep the two in sync by hand rather +// than deriving this from PLACEHOLDER_PATTERN.source: that pattern's capture +// groups happen to be harmless today (the replacer ignores its group +// arguments), but splicing capturing groups into a composed alternation +// shifts group numbering for any future branch that does capture. +const PLACEHOLDER_TOKEN_ALTERNATIVE = '\\$\\{(?:variables|secrets)\\.[A-Za-z0-9_-]+\\}'; + export function resolveRuntimePlaceholders( value: string, bindings: RuntimeBindings, @@ -24,6 +32,14 @@ export function containsSecretPlaceholder(value: string): boolean { return /\$\{secrets\.[A-Za-z0-9_-]+\}/.test(value); } +// A 1-2 character secret value collides with ordinary text near-certainly, so +// substituting it corrupts arbitrary strings. Values below this length are +// therefore never redacted anywhere — the accepted trade is that such short +// secret values reach prompts, report artifacts, spans, and error strings raw. +// 3 is the largest threshold that leaves the pinned overlap-ordering fixture +// (3-char 'abc' in packages/cli/src/test/testRunner.test.ts) untouched. +const MIN_REDACTABLE_SECRET_LENGTH = 3; + export function redactResolvedValue( value: string | undefined, bindings: RuntimeBindings, @@ -39,7 +55,10 @@ export function redactResolvedValue( // match the short value inside the longer one's occurrence, leaving the rest // of the longer secret unredacted in the output. const replacements = Object.entries(bindings.secrets) - .filter(([, secretValue]) => Boolean(secretValue)) + .filter( + ([, secretValue]) => + Boolean(secretValue) && secretValue.length >= MIN_REDACTABLE_SECRET_LENGTH, + ) .sort(([, left], [, right]) => right.length - left.length); if (replacements.length === 0) { return value; @@ -52,10 +71,32 @@ export function redactResolvedValue( } } + // The placeholder-token alternative is deliberately the LAST branch: every + // secret-value alternative (longest first) is tried before it at each + // position, so a secret-value occurrence always wins over token protection + // when both match at the same position — leak-safety over token cosmetics. + // Token-first would preempt a value that BEGINS with a literal token (e.g. + // value ${secrets.BAR}hunter2), leaving its raw tail in the output. Token + // protection still holds where it matters: a value occurring strictly + // INSIDE a token (e.g. value PASSWORD inside ${secrets.PASSWORD}, common in + // testObjective and report text) cannot match at the token's start + // position, so absent a value match there the token branch consumes the + // token whole and its interior is never exposed to the value alternatives — + // no nested, invalid token. Accepted edges: a secret whose value is EXACTLY + // a well-formed token is matched by its own value alternative and rewritten + // placeholder-to-placeholder via the map — harmless (nothing raw on either + // side) and identical to pre-change behavior; consequently a token matched + // by the token branch matches no secret value, so the replacer's `?? match` + // passes it through verbatim. Value matching stays UNANCHORED on purpose: + // word boundaries would miss a secret embedded in concatenated text + // (user=xabcd1234y), the same leak class the sort order above guards + // against. Residual: a value >= 3 chars that is a common substring of prose + // still rewrites that prose. const secretPattern = new RegExp( - replacements - .map(([, secretValue]) => escapeRegExp(secretValue)) - .join('|'), + [ + ...replacements.map(([, secretValue]) => escapeRegExp(secretValue)), + PLACEHOLDER_TOKEN_ALTERNATIVE, + ].join('|'), 'g', ); diff --git a/packages/common/src/test/repoPlaceholders.test.ts b/packages/common/src/test/repoPlaceholders.test.ts new file mode 100644 index 0000000..234800f --- /dev/null +++ b/packages/common/src/test/repoPlaceholders.test.ts @@ -0,0 +1,70 @@ +import assert from 'node:assert/strict'; +import test from 'node:test'; +import type { RuntimeBindings } from '../models/Environment.js'; +import { redactResolvedValue } from '../repoPlaceholders.js'; + +function bindings(secrets: Record): RuntimeBindings { + return { secrets, variables: {} }; +} + +test('redactResolvedValue keeps an existing placeholder token intact when a secret value equals its own key name', () => { + // Regression (unanchored-match corruption): the value alternative used to + // match PASSWORD inside the literal ${secrets.PASSWORD} token, producing + // the nested, invalid ${secrets.${secrets.PASSWORD}}. + const redacted = redactResolvedValue( + 'Use ${secrets.PASSWORD} then type PASSWORD manually', + bindings({ PASSWORD: 'PASSWORD' }), + ); + + assert.equal(redacted, 'Use ${secrets.PASSWORD} then type ${secrets.PASSWORD} manually'); +}); + +test('redactResolvedValue leaves prose unchanged for a single-character secret value', () => { + // Regression (prose destruction): a 1-char value used to rewrite every + // occurrence of that character, mangling the entire string. + const redacted = redactResolvedValue('assemble the secrets list', bindings({ TOKEN: 's' })); + + assert.equal(redacted, 'assemble the secrets list'); +}); + +test('redactResolvedValue leaves prose unchanged for a two-character secret value that is a substring of it', () => { + const redacted = redactResolvedValue('these tests pass', bindings({ AB: 'es' })); + + assert.equal(redacted, 'these tests pass'); +}); + +test('redactResolvedValue still redacts a secret embedded in concatenated text', () => { + // Pins the deliberate absence of word-boundary anchoring: anchors would + // miss embedded values like this one and leak them. + const redacted = redactResolvedValue('x=zabcd1234q', bindings({ KEY: 'abcd1234' })); + + assert.equal(redacted, 'x=z${secrets.KEY}q'); +}); + +test('redactResolvedValue token protection composes with longest-value-first overlap ordering', () => { + // The ${secrets.abc} token is discriminating: its interior contains the + // overlapping value abc, so the pre-fix implementation (no token branch) + // nests it into ${secrets.${secrets.short}} and fails this case. + const redacted = redactResolvedValue( + 'keep ${secrets.abc} primary=abcd secondary=abc', + bindings({ short: 'abc', long: 'abcd' }), + ); + + assert.equal( + redacted, + 'keep ${secrets.abc} primary=${secrets.long} secondary=${secrets.short}', + ); +}); + +test('redactResolvedValue redacts a secret value that begins with a literal placeholder token', () => { + // Regression (token-FIRST leak): with the token alternative as the first + // branch, ${secrets.BAR} was consumed as token protection before the value + // alternative could match, leaving the raw hunter2 tail in the output. The + // token branch is last so the value alternative wins at the $ position. + const redacted = redactResolvedValue( + 'creds=${secrets.BAR}hunter2 end', + bindings({ FOO: '${secrets.BAR}hunter2' }), + ); + + assert.equal(redacted, 'creds=${secrets.FOO} end'); +}); diff --git a/packages/goal-executor/src/ai/AIAgent.ts b/packages/goal-executor/src/ai/AIAgent.ts index 4fcfeb7..feacfe4 100644 --- a/packages/goal-executor/src/ai/AIAgent.ts +++ b/packages/goal-executor/src/ai/AIAgent.ts @@ -254,7 +254,11 @@ export class AIAgent { * 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. + * app re-renders (truncated, reformatted, partially masked) is not caught, + * and values shorter than MIN_REDACTABLE_SECRET_LENGTH (3 chars, in + * repoPlaceholders.ts) are never redacted and reach the provider raw — + * the accepted trade for not corrupting arbitrary prompt text, since a + * 1-2 char value collides with ordinary text near-certainly. */ private _redactPromptText(value: string): string { if (!this._bindings) return value; diff --git a/packages/goal-executor/src/ai/test/AIAgent.test.ts b/packages/goal-executor/src/ai/test/AIAgent.test.ts index ad3e2a8..9785d32 100644 --- a/packages/goal-executor/src/ai/test/AIAgent.test.ts +++ b/packages/goal-executor/src/ai/test/AIAgent.test.ts @@ -698,6 +698,48 @@ test('AIAgent without bindings assembles prompts unredacted', () => { assert.ok(!text.includes('${secrets.PASSWORD}')); }); +test('AIAgent keeps a literal placeholder token in the objective intact when a secret value equals its key name', () => { + // Regression: the unanchored value match used to find PASSWORD inside the + // literal ${secrets.PASSWORD} token that testObjective deliberately carries + // and nest it into ${secrets.${secrets.PASSWORD}} — a token the model was + // never taught. + const agent = makeAgent({ + bindings: { secrets: { PASSWORD: 'PASSWORD' }, variables: {} }, + }); + + const { textPrompt } = buildPlannerPrompt(agent, { + testObjective: 'Type ${secrets.PASSWORD} into the field, then type PASSWORD again', + platform: 'android', + }); + + assert.ok(!textPrompt.includes('${secrets.${secrets.PASSWORD}}')); + assert.ok( + textPrompt.includes( + 'Type ${secrets.PASSWORD} into the field, then type ${secrets.PASSWORD} again', + ), + ); +}); + +test('AIAgent with a single-character secret value assembles the planner prompt byte-identically', () => { + // Regression: a 1-char value used to rewrite every occurrence of that + // character, mangling the whole prompt — including the word "secrets" + // inside other placeholder tokens. + const request: PlannerRequest = { + testObjective: 'Press the submit button and assemble the secrets list', + platform: 'android', + history: '1. [tap] Tapped submit → SUCCESS\n', + }; + const redactingAgent = makeAgent({ + bindings: { secrets: { TOKEN: 's' }, variables: {} }, + }); + const plainAgent = makeAgent(); + + assert.equal( + buildPlannerPrompt(redactingAgent, request).textPrompt, + buildPlannerPrompt(plainAgent, request).textPrompt, + ); +}); + // ---------------------------------------------------------------------------- // Retry behavior for plan() and ground() // ---------------------------------------------------------------------------- From 30d3c7595aecd40417d183a124a82dd8dd3dd5b8 Mon Sep 17 00:00:00 2001 From: droid-ash Date: Fri, 31 Jul 2026 18:57:34 +0000 Subject: [PATCH 2/3] Update ship status and record PR URL --- .../.history.jsonl | 1 + .../.status.yaml | 26 ++++++++++--------- 2 files changed, 15 insertions(+), 12 deletions(-) diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl index cd0bc4b..ba02179 100644 --- a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl @@ -15,3 +15,4 @@ {"event":"review","result":"passed","ts":"2026-07-31T18:47:52Z"} {"cmd":"fab-continue","event":"command","ts":"2026-07-31T18:48:41Z"} {"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-31T18:55:03Z"} +{"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-07-31T18:57:30Z"} diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml index 8a6acbd..874e8f4 100644 --- a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml @@ -9,8 +9,8 @@ progress: apply: done review: done hydrate: done - ship: active - review-pr: pending + ship: done + review-pr: active plan: generated: true task_count: 11 @@ -33,19 +33,21 @@ stage_metrics: apply: {started_at: "2026-07-31T18:22:42Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T18:35:36Z"} review: {started_at: "2026-07-31T18:35:36Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T18:47:52Z"} hydrate: {started_at: "2026-07-31T18:47:52Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T18:55:03Z"} - ship: {started_at: "2026-07-31T18:55:03Z", driver: fab-fff, iterations: 1} -prs: [] + ship: {started_at: "2026-07-31T18:55:03Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T18:57:30Z"} + review-pr: {started_at: "2026-07-31T18:57:30Z", driver: git-pr, iterations: 1} +prs: + - https://github.com/droid-ash/finalrun-agent/pull/178 change_type_source: explicit true_impact: - added: 0 - deleted: 0 - net: 0 + added: 803 + deleted: 20 + net: 783 tests: - added: 0 + added: 112 deleted: 0 - net: 0 - computed_at: "2026-07-31T18:55:03Z" - computed_at_stage: hydrate + net: 112 + computed_at: "2026-07-31T18:57:30Z" + computed_at_stage: ship summary: 'Bound redactResolvedValue''s substring matching: placeholder tokens pass through whole and secret values under 3 chars are never redacted, documented across common/cli memory' # true_impact: lazily created on first stage-finish that computes it (no placeholder here). -last_updated: 2026-07-31T18:55:03Z +last_updated: 2026-07-31T18:57:30Z From 872d2192bb618e9a344d016b7f170de1a0d8f2d4 Mon Sep 17 00:00:00 2001 From: droid-ash Date: Fri, 31 Jul 2026 19:14:40 +0000 Subject: [PATCH 3/3] Update review-pr status --- .../.history.jsonl | 1 + .../.status.yaml | 6 +++--- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl index ba02179..a6d347e 100644 --- a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonl @@ -16,3 +16,4 @@ {"cmd":"fab-continue","event":"command","ts":"2026-07-31T18:48:41Z"} {"action":"enter","driver":"fab-fff","event":"stage-transition","stage":"ship","ts":"2026-07-31T18:55:03Z"} {"action":"enter","driver":"git-pr","event":"stage-transition","stage":"review-pr","ts":"2026-07-31T18:57:30Z"} +{"event":"review","result":"passed","ts":"2026-07-31T19:14:33Z"} diff --git a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml index 874e8f4..dd30abb 100644 --- a/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml +++ b/fab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yaml @@ -10,7 +10,7 @@ progress: review: done hydrate: done ship: done - review-pr: active + review-pr: done plan: generated: true task_count: 11 @@ -34,7 +34,7 @@ stage_metrics: review: {started_at: "2026-07-31T18:35:36Z", driver: fab-fff, iterations: 2, completed_at: "2026-07-31T18:47:52Z"} hydrate: {started_at: "2026-07-31T18:47:52Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T18:55:03Z"} ship: {started_at: "2026-07-31T18:55:03Z", driver: fab-fff, iterations: 1, completed_at: "2026-07-31T18:57:30Z"} - review-pr: {started_at: "2026-07-31T18:57:30Z", driver: git-pr, iterations: 1} + review-pr: {started_at: "2026-07-31T18:57:30Z", driver: git-pr, iterations: 1, completed_at: "2026-07-31T19:14:33Z"} prs: - https://github.com/droid-ash/finalrun-agent/pull/178 change_type_source: explicit @@ -50,4 +50,4 @@ true_impact: computed_at_stage: ship summary: 'Bound redactResolvedValue''s substring matching: placeholder tokens pass through whole and secret values under 3 chars are never redacted, documented across common/cli memory' # true_impact: lazily created on first stage-finish that computes it (no placeholder here). -last_updated: 2026-07-31T18:57:30Z +last_updated: 2026-07-31T19:14:33Z