fix: Fix Redaction Prompt Corruption - #178
Merged
Merged
Conversation
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.
📝 WalkthroughWalkthroughThe change updates shared secret redaction to preserve placeholder tokens and exclude secrets shorter than three characters. It adds regression tests and documents prompt, report, span, screenshot, and provider exposure boundaries. ChangesSecret redaction contract
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
droid-ash
marked this pull request as ready for review
August 1, 2026 02:33
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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 `@docs/memory/cli/report-writer.md`:
- Around line 20-26: Update the guarantee in the opening paragraph to say that
every textual value reaching a file crosses redactResolvedValue, rather than
claiming every value is redacted. Preserve the existing explanation that binary
screenshot bytes are outside this textual redaction path and do not broaden the
guarantee beyond textual file contents.
- Line 24: Update the authored `${secrets.KEY}` token statement in the
report-writer documentation to qualify verbatim pass-through: it applies only
when no eligible secret-value branch matches at the token’s start. Preserve the
existing explanation that the authored-token alternation branch consumes and
emits the token unchanged in cases where that value-branch condition is not met.
In `@docs/memory/common/repo-placeholders.md`:
- Around line 124-126: Update the short-secret documentation in
repo-placeholders.md to remove the claim that 1–2 character values are
inference-proof, and state only that leaving them raw is an accepted residual
risk to avoid corrupting arbitrary text.
- Around line 146-150: Update the rationale in the paragraph beginning “Why” to
replace the claim that reports are only read by humans with wording that covers
both machine consumers, including the report viewer and runIndex, and human
readers. Preserve the existing explanation about shared matching semantics and
security-relevant implementations.
In `@packages/common/src/repoPlaceholders.ts`:
- Around line 85-90: Correct the token-branch explanations: in
packages/common/src/repoPlaceholders.ts:85-90, state that no secret-value
alternative wins at the token’s start position, that the full token is not a
placeholderBySecretValue key, and qualify the memory claim by this precedence
rule. Apply the corresponding accepted-edge rationale corrections in
fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md:60 and
plan.md:15, update the R6 requirement in plan.md:60, and state in
docs/memory/cli/test-compiler.md:63-68 that token protection applies when a
value alternative does not match at the token start.
🪄 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: a8c25f43-5f55-4d64-a5cc-00766b44e472
📒 Files selected for processing (17)
docs/memory/cli/index.mddocs/memory/cli/report-writer.mddocs/memory/cli/test-compiler.mddocs/memory/common/index.mddocs/memory/common/repo-placeholders.mddocs/memory/index.mdfab/changes/260731-gz13-fix-redaction-prompt-corruption/.history.jsonlfab/changes/260731-gz13-fix-redaction-prompt-corruption/.status.yamlfab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.mdfab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.mdfab/changes/260731-gzzl-fix-secret-prompt-leak-comment/intake.mdfab/changes/260731-gzzl-fix-secret-prompt-leak-comment/plan.mdpackages/cli/src/testCompiler.tspackages/common/src/repoPlaceholders.tspackages/common/src/test/repoPlaceholders.test.tspackages/goal-executor/src/ai/AIAgent.tspackages/goal-executor/src/ai/test/AIAgent.test.ts
Comment on lines
+20
to
+26
| 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.<key>}` 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/<testId>/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/<testId>/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. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Limit the guarantee to textual values.
Line 20 says every value that reaches a file crosses redactResolvedValue, but Line 26 says binary screenshot bytes are outside this path. Use “textual value” for the guarantee so readers do not interpret the whole run directory as redacted.
Proposed wording
-Every value that reaches a file crosses `redactResolvedValue(value, bindings)` ...
+Every textual value that reaches a file crosses `redactResolvedValue(value, bindings)` ...
-The sweep is a byte-level check on emitted files ...
+This is a string-redaction guarantee. The sweep is a byte-level check on emitted files ...📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| 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.<key>}` 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/<testId>/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/<testId>/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. | |
| Every textual 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.<key>}` 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 **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. | |
| "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)). This is a string-redaction guarantee. The sweep is a **byte-level** check on emitted files, so `tests/<testId>/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. |
🤖 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 `@docs/memory/cli/report-writer.md` around lines 20 - 26, Update the guarantee
in the opening paragraph to say that every textual value reaching a file crosses
redactResolvedValue, rather than claiming every value is redacted. Preserve the
existing explanation that binary screenshot bytes are outside this textual
redaction path and do not broaden the guarantee beyond textual file contents.
| 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/<testId>/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. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Qualify the authored-token pass-through claim.
Line 24 promises verbatim output for every authored token. docs/memory/common/repo-placeholders.md Lines 88-92 document an exact-token secret-value case where the value branch rewrites the token to another placeholder. Restrict “emitted verbatim” to cases where no eligible value branch matches at the token start.
Proposed wording
-... is consumed whole by the redaction alternation's last branch and emitted verbatim, never rewritten into a nested token.
+... is passed through verbatim when no eligible value branch matches at the token start; an exact-token secret value may be rewritten placeholder-to-placeholder, but never into a nested token.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| "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. | |
| "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 passed through verbatim when no eligible value branch matches at the token start; an exact-token secret value may be rewritten placeholder-to-placeholder, but never into a nested token. |
🤖 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 `@docs/memory/cli/report-writer.md` at line 24, Update the authored
`${secrets.KEY}` token statement in the report-writer documentation to qualify
verbatim pass-through: it applies only when no eligible secret-value branch
matches at the token’s start. Preserve the existing explanation that the
authored-token alternation branch consumes and emits the token unchanged in
cases where that value-branch condition is not met.
Comment on lines
+124
to
+126
| - **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. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Remove the inference-proof claim for short secrets.
A one- or two-character secret can still be a PIN, access code, or password fragment. The implementation leaves these values raw by design, so this wording gives unsafe assurance. State that the residual is accepted to avoid text corruption, not because the value is safe to expose.
Proposed wording
-a value that short is also close to inference-proof by nature.
+short values can still be sensitive; this trade-off does not make them safe to expose.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| - **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. | |
| - **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; short values can still be sensitive; this trade-off does not make them safe to expose. |
🤖 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 `@docs/memory/common/repo-placeholders.md` around lines 124 - 126, Update the
short-secret documentation in repo-placeholders.md to remove the claim that 1–2
character values are inference-proof, and state only that leaving them raw is an
accepted residual risk to avoid corrupting arbitrary text.
Comment on lines
+146
to
+150
| **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. |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Correct the report-consumer rationale.
Line 150 says reports are read by humans rather than parsed. docs/memory/cli/report-writer.md Line 67 states that the report viewer and runIndex consume the emitted files. Update the rationale to cover machine consumers and human readers.
Proposed wording
-report path would keep corrupting silently because reports are read by humans rather than parsed.
+report path would keep corrupting silently because report files are parsed by the report viewer and `runIndex` and then read by humans.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| **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. | |
| **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 report files are parsed by the report viewer and `runIndex` and then read by humans. |
🤖 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 `@docs/memory/common/repo-placeholders.md` around lines 146 - 150, Update the
rationale in the paragraph beginning “Why” to replace the claim that reports are
only read by humans with wording that covers both machine consumers, including
the report viewer and runIndex, and human readers. Preserve the existing
explanation about shared matching semantics and security-relevant
implementations.
Comment on lines
+85
to
+90
| // 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: |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the token-branch explanation.
A token selected at $ can still contain a configured secret value. For example, ${secrets.PASSWORD} contains PASSWORD. The token branch wins because it starts earlier, not because no secret value exists.
State that no secret-value alternative wins at the token start position. State that the full token is not a placeholderBySecretValue key. Qualify the memory claim with the same precedence rule.
packages/common/src/repoPlaceholders.ts#L85-L90: Replace the “matches no secret value” explanation with the token-start precedence and map-key explanation.fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md#L60-L60: Correct the accepted-edge claim to match the implementation.fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md#L15-L15: Correct the accepted-edge rationale.fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md#L60-L60: Correct the R6 comment requirement.docs/memory/cli/test-compiler.md#L63-L68: State that token protection applies when a value alternative does not match at the token start.
📍 Affects 4 files
packages/common/src/repoPlaceholders.ts#L85-L90(this comment)fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md#L60-L60fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md#L15-L15fab/changes/260731-gz13-fix-redaction-prompt-corruption/plan.md#L60-L60docs/memory/cli/test-compiler.md#L63-L68
🤖 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/common/src/repoPlaceholders.ts` around lines 85 - 90, Correct the
token-branch explanations: in packages/common/src/repoPlaceholders.ts:85-90,
state that no secret-value alternative wins at the token’s start position, that
the full token is not a placeholderBySecretValue key, and qualify the memory
claim by this precedence rule. Apply the corresponding accepted-edge rationale
corrections in
fab/changes/260731-gz13-fix-redaction-prompt-corruption/intake.md:60 and
plan.md:15, update the R6 requirement in plan.md:60, and state in
docs/memory/cli/test-compiler.md:63-68 that token protection applies when a
value alternative does not match at the token start.
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
gz13generated by fab-kit v2.16.8
Pipeline: intake ✓ → apply ✓ → review ✓ → hydrate ✓ → ship → review-pr
Summary
redactResolvedValue(packages/common/src/repoPlaceholders.ts) rewrote secret values via unanchored substring matching, which PR #175 newly wired into the LLM prompt path. A secret value equal to its own key name nested into an invalid${secrets.${secrets.KEY}}token, and a short/common secret value could mangle arbitrary prose in the prompt — both verified reproductions from CodeRabbit's review of PR #175 and independent operator testing against the built dist. This fixes the shared function so existing placeholder tokens survive redaction and values under 3 characters are never redacted, since substituting them corrupts text without protecting anything.Changes
redactResolvedValue— protect placeholder tokens from rewriting (fixes token-nesting corruption)redactResolvedValue— minimum redactable value length of 3 characters (fixes prose-destruction corruption from short values)packages/common/src/test/repoPlaceholders.test.ts,packages/goal-executor/src/ai/test/AIAgent.test.ts)repoPlaceholders.ts,AIAgent.ts,testCompiler.ts)260731-gzzl-fix-secret-prompt-leak-comment's artifacts (CodeRabbit findings, verified at intake)Summary by CodeRabbit
Bug Fixes
${secrets.KEY}placeholders during redaction.Documentation
Tests