fix: Fix Secret-Leak Guarantee Comment and Assess the Prompt-Path Secret Leak - #175
Merged
Merged
Conversation
testCompiler.ts overstated its guarantee — secrets never enter the
compiled objective, but resolved values still reach the model via the
post-typing accessibility hierarchy and screenshots. Comment rewritten
to scope the claim correctly and document the redaction wiring.
AIAgent now accepts optional RuntimeBindings (wired from sessionRunner)
and redacts exact resolved-secret values to ${secrets.KEY} placeholders
per-input before assembly/serialization, across every free-text input
and element/availableApps string field. No-bindings path is a no-op.
Reworked once: an initial whole-text redaction pass corrupted
serialized element JSON when a numeric secret matched a coordinate and
missed JSON-escaped secrets in remember entries. Replaced with
per-input pre-serialization redaction; 2 regression tests added.
📝 WalkthroughWalkthroughThis change documents secret handling during test objective compilation and runtime execution. It passes runtime bindings into ChangesSecret exposure contract and workflow records
Runtime secret redaction
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant sessionRunner
participant AIAgent
participant Planner
participant Grounder
sessionRunner->>AIAgent: pass config.runtimeBindings
AIAgent->>AIAgent: redact resolved secret values
AIAgent->>Planner: send redacted objective and hierarchy
AIAgent->>Grounder: send redacted action, hierarchy, and app records
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
No external review was obtained: Copilot code review is not enabled on this repository (request polled 10 minutes, zero reviews landed) and the CodeRabbit check's pass state is a rate-limit decline, not an approval. Stage finished per the user's standing directive to not block on external bots and treat the operator-side pipeline review as the substitute — that review caught and fixed two must-fix redaction defects in one rework cycle before the PR was opened. Includes the pending review-pr telemetry line so main records the change as completed (PR #170 precedent: unpushed finishing commit left it permanently in-flight). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
droid-ash
marked this pull request as ready for review
July 31, 2026 17:14
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md`:
- Line 16: Correct the description of Hierarchy.toPromptElementsForPlanner to
state that eligible image or button nodes contribute contentDesc, not node.text.
Keep node.text attribution only for Hierarchy.toPromptElementsForGrounder,
without changing the surrounding security analysis.
In `@fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md`:
- Around line 126-128: Update the A-002 acceptance-record wording to describe
redaction only on individual grounder and planner inputs before assembly,
including element fields where applicable; remove the claim that assembled
prompt text is redacted, while preserving the existing placeholder and reused
redactResolvedValue details.
In `@packages/goal-executor/src/ai/AIAgent.ts`:
- Around line 259-262: Update shared redactResolvedValue logic used by
_redactPromptText to preserve valid ${secrets.KEY} placeholders before matching
or replacing resolved values, preventing secret key/value collisions from
producing nested tokens; add a regression test covering this collision and
verify the original placeholder reaches the model unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: a8137235-f510-48f4-b9b3-ac64a848daae
📒 Files selected for processing (12)
docs/memory/cli/index.mddocs/memory/cli/report-writer.mddocs/memory/cli/test-compiler.mddocs/memory/common/hierarchy.mdfab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.history.jsonlfab/changes/260731-gzzl-fix-secret-prompt-leak-comment/.status.yamlfab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.mdfab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.mdpackages/cli/src/sessionRunner.tspackages/cli/src/testCompiler.tspackages/goal-executor/src/ai/AIAgent.tspackages/goal-executor/src/ai/test/AIAgent.test.ts
|
|
||
| ## 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. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the planner hierarchy description.
Line 16 says toPromptElementsForPlanner emits node.text. It emits contentDesc only for eligible image or button nodes. Keep node.text attribution limited to toPromptElementsForGrounder.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.md` at line 16,
Correct the description of Hierarchy.toPromptElementsForPlanner to state that
eligible image or button nodes contribute contentDesc, not node.text. Keep
node.text attribution only for Hierarchy.toPromptElementsForGrounder, without
changing the surrounding security analysis.
Comment on lines
+126
to
+128
| - [x] A-001 R1: The rewritten `testCompiler.ts` comment scopes the guarantee to the compiled objective, states the runtime hierarchy + screenshot exposure with the concrete file chain, states where redaction is and is not wired, warns against enabling full prompt logging, and keeps the pattern-widening warning | ||
| - [x] A-002 R3: `AIAgent` accepts optional bindings and redacts exact resolved-secret occurrences from grounder and planner prompt text (element fields pre-serialization plus the assembled text), replacing them with `${secrets.KEY}` placeholders via the reused `redactResolvedValue` | ||
| - [x] A-003 R3: `sessionRunner.createSessionExecutor` passes `config.runtimeBindings` to the `AIAgent`, and constructing `AIAgent` without bindings leaves prompt assembly byte-identical to the pre-change behavior |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Correct A-002 to exclude assembled-text redaction.
Line 127 states that redaction also applies to “the assembled text.” AIAgent intentionally redacts each input before assembly. Replace this phrase with the per-input behavior so the acceptance record does not endorse a JSON-corrupting post-serialization pass.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@fab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.md` around lines
126 - 128, Update the A-002 acceptance-record wording to describe redaction only
on individual grounder and planner inputs before assembly, including element
fields where applicable; remove the claim that assembled prompt text is
redacted, while preserving the existing placeholder and reused
redactResolvedValue details.
Comment on lines
+259
to
+262
| private _redactPromptText(value: string): string { | ||
| if (!this._bindings) return value; | ||
| return redactResolvedValue(value, this._bindings) ?? value; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Preserve existing secret placeholders during redaction.
Line 261 redacts testObjective, which intentionally contains ${secrets.KEY}. If bindings.secrets.PASSWORD is PASSWORD, redactResolvedValue rewrites ${secrets.PASSWORD} into ${secrets.${secrets.PASSWORD}}. The model then receives an invalid logical token.
Protect valid ${secrets.KEY} tokens before matching resolved values, preferably in shared redactResolvedValue. Add a regression test for a secret key/value collision.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@packages/goal-executor/src/ai/AIAgent.ts` around lines 259 - 262, Update
shared redactResolvedValue logic used by _redactPromptText to preserve valid
${secrets.KEY} placeholders before matching or replacing resolved values,
preventing secret key/value collisions from producing nested tokens; add a
regression test covering this collision and verify the original placeholder
reaches the model unchanged.
droid-ash
added a commit
that referenced
this pull request
Aug 1, 2026
* 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. * Update ship status and record PR URL * Update review-pr status --------- Co-authored-by: droid-ash <droid-ash@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Meta
gzzlgenerated by fab-kit v2.16.8
Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr
Summary
packages/cli/src/testCompiler.tsclaimed secret values never enter the LLM prompt, provider logs, or compiled artifacts — true only for the compiled objective. The comment is rewritten to scope the guarantee correctly and to state plainly that resolved secret values DO reach the model provider via the post-typing accessibility hierarchy (grounder-legtext/hintText/error) and via screenshots. A partial mitigation is also implemented:AIAgentnow redacts exact resolved-secret values to${secrets.KEY}placeholders per-input, before assembly/serialization, gated on grounding proof (existing + 2 new regression tests).Changes
packages/cli/src/testCompiler.tsto scope the guarantee to placeholder substitution in the compiled objective, describe the runtime leak chain and the redaction wiring accurately, and warn against enabling full prompt logging.AIAgent(packages/goal-executor) accepts optionalRuntimeBindings(wired fromsessionRunner) and redacts exact resolved-secret values to${secrets.KEY}placeholders per-input before assembly/serialization — every free-text input (objective, act, history, remember entries, preContext, appKnowledge) and every element/availableApps string field. ReusesredactResolvedValue(longest-first). Hierarchy structure (index/bounds/id/class) is never mutated; the no-bindings path is a strict no-op.rememberentries. Replaced with per-input pre-serialization redaction; 2 regression tests added.docs/memory/cli/test-compiler.md): screenshots still carry secrets to the provider and report artifacts and cannot be redacted cheaply without blinding the planner; app-rendered transformations (truncation/reformat/masking) can escape exact-match redaction; exact-match redaction can over-match structural strings (degrades grounding text, never JSON structure);formatPlannerReasoningstill prints planner thought/action/reason raw to stdout — a known should-fix from review, deliberately not addressed in this change.docs/memory/cli/test-compiler.md(exposure contract + 4 design decisions); carve-out added todocs/memory/cli/report-writer.md; downstream-redaction pointer added todocs/memory/common/hierarchy.md; indexes regenerated.Origin: independent adversarial review of PRs #168–#172.
Summary by CodeRabbit
Security Improvements
Documentation