refactor(test/e2e): route fixture redaction through canonical entry#5022
Merged
Conversation
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
laitingsheng
added
area: e2e
Contributor
E2E Advisor RecommendationRequired E2E: Dispatch hint: Full advisor summaryE2E Recommendation AdvisorBase: Required E2E
Optional E2E
New E2E recommendations
Dispatch hint
|
Contributor
E2E Scenario Advisor RecommendationRequired scenario E2E: Dispatch required scenario E2E:
Full scenario advisor summaryE2E Scenario AdvisorBase: Required scenario E2E
Optional scenario E2E
Relevant changed files
|
Contributor
📝 WalkthroughWalkthroughUnify redaction by extending redactString(text, explicitValues?) to handle explicit literals longest-first, route SecretStore and ShellProbe through it (removing redactText), and add Vitest contract tests verifying explicit vs canonical redaction behavior and edge cases. ChangesRedaction Entry Point Consolidation
Sequence DiagramsequenceDiagram
participant TestSuite as Test Suite
participant SecretStore as SecretStore.redact()
participant ShellProbe as ShellProbe.redactProbeText()
participant redactString as redactString()
participant ExplicitPass as Explicit Value Replacement
participant CanonicalPass as Canonical Pattern Redaction
TestSuite->>SecretStore: redact(text, extraValues)
TestSuite->>ShellProbe: redactProbeText(text)
SecretStore->>redactString: redactString(text, env + extraValues)
ShellProbe->>redactString: redactString(text, redactionValues)
redactString->>ExplicitPass: deduplicate & sort longest-first
ExplicitPass->>CanonicalPass: continue with regex patterns
CanonicalPass->>redactString: return redacted text
redactString->>SecretStore: return redacted text
redactString->>ShellProbe: return redacted text
SecretStore->>TestSuite: return redacted text
ShellProbe->>TestSuite: return redacted text
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
PR Review AdvisorFindings: 0 needs attention, 0 worth checking, 0 nice ideas This is an automated advisory review. A human maintainer must make the final merge decision. |
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@test/e2e-scenario/framework/shell-probe.ts`:
- Around line 140-151: enforcedValues should be applied locally before
delegating to this.redact and in canonical explicit-value order to avoid
partial-overlap leaks; change enforceLocalRedaction/redactProbeText so
enforcedValues = redactionValues.filter(...).sort((a,b)=>b.length-a.length) and
redactProbeText first runs enforceLocalRedaction(text) (replacing exact matches
with "[REDACTED]") then passes that result into this.redact(...,
redactionValues), falling back to this.redact(text, redactionValues) when no
enforcedValues.
🪄 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: Enterprise
Run ID: d08f3654-84c2-4c2a-b8c2-481677e992d1
📒 Files selected for processing (2)
test/e2e-scenario/framework-tests/e2e-fixture-context.test.tstest/e2e-scenario/framework/shell-probe.ts
Signed-off-by: Tinson Lai <tinsonl@nvidia.com>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
test/e2e-scenario/framework-tests/e2e-fixture-context.test.ts (1)
165-205: ⚡ Quick winConsider adding positive assertions for
[REDACTED]to confirm redaction occurred.The test correctly verifies that neither the full
longersecret nor the-beta-gamma-deltasuffix appear in the output, confirming longest-first ordering. However, the test uses only negative assertions (.not.toContain). If the subprocess produced no output at all, the test would still pass.Adding positive assertions that
[REDACTED]appears inresult.stdoutandresult.stderrwould increase confidence that:
- The subprocess successfully output the secret.
- Redaction actually occurred (vs. empty/missing output).
The previous test at lines 152-153 includes these positive checks, so adding them here would maintain consistency.
✨ Suggested enhancement
expect(result.exitCode).toBe(0); + expect(result.stdout).toContain("[REDACTED]"); + expect(result.stderr).toContain("[REDACTED]"); expect(result.stdout).not.toContain(longer); expect(result.stdout).not.toContain("-beta-gamma-delta");🤖 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 `@test/e2e-scenario/framework-tests/e2e-fixture-context.test.ts` around lines 165 - 205, Add positive assertions to confirm redaction occurred: after obtaining result from ShellProbe.run in the "shell probe scrubs overlapping..." test, assert that result.stdout and result.stderr contain the literal "[REDACTED]"; likewise assert the serialized result file content (variable written) and the artifact files read via artifacts.pathFor("shell/overlap-shorter-first.stdout.txt") and "...stderr.txt" contain "[REDACTED]". Keep these checks alongside the existing negative assertions to ensure the subprocess emitted the secret and it was actually redacted.
🤖 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.
Nitpick comments:
In `@test/e2e-scenario/framework-tests/e2e-fixture-context.test.ts`:
- Around line 165-205: Add positive assertions to confirm redaction occurred:
after obtaining result from ShellProbe.run in the "shell probe scrubs
overlapping..." test, assert that result.stdout and result.stderr contain the
literal "[REDACTED]"; likewise assert the serialized result file content
(variable written) and the artifact files read via
artifacts.pathFor("shell/overlap-shorter-first.stdout.txt") and "...stderr.txt"
contain "[REDACTED]". Keep these checks alongside the existing negative
assertions to ensure the subprocess emitted the secret and it was actually
redacted.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: 9f47d269-aded-4516-9537-77430d4c80d1
📒 Files selected for processing (2)
test/e2e-scenario/framework-tests/e2e-fixture-context.test.tstest/e2e-scenario/framework/shell-probe.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- test/e2e-scenario/framework/shell-probe.ts
cv
approved these changes
Jun 9, 2026
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
2 participants
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.
Summary
Collapse the two redaction entry points in the E2E fixture stack so per-test explicit secret values and canonical secret-shape matches share a single
redactStringcall. The fixture bridge no longer carries its own redaction pattern source;SecretStoreonly supplies the explicit values it knows about and delegates.Related Issue
Closes #4989
Changes
test/e2e-scenario/scenarios/orchestrators/redaction.ts:redactStringnow accepts an optionalexplicitValuesiterable and applies them longest first with the[REDACTED]sentinel before the canonical regex passes run with<REDACTED>. The two sentinels stay distinct so explicit-value hits remain visually separable from regex hits in artifacts.test/e2e-scenario/framework/secrets.ts: drops the standaloneredactTextexport.SecretStore.redactis now a thin delegate that hands its env-derived plus caller-supplied values toredactStringin one call.test/e2e-scenario/framework/shell-probe.ts: drops theredactTextimport and the double-pass throughredactText+this.redact; the probe now calls the injectedredact(text, redactionValues)once.test/e2e-scenario/framework-tests/e2e-redaction-entry.test.ts(new): asserts the single-entry contract — both sentinels coexist, longest-first ordering protects nested values, empty values are ignored, andSecretStore.redactunions env-derived and caller-supplied values through the canonical entry.Canonical secret-shape regex parity with
src/lib/security/secret-patterns.ts(e2e-redaction-parity.test.ts) stays unchanged; no second product secret-pattern mirror is introduced.Type of Change
Verification
npx prek run --all-filespassesnpm testpassesnpm run docsbuilds without warnings (doc changes only)Signed-off-by: Tinson Lai tinsonl@nvidia.com
Summary by CodeRabbit
New Features
Tests
Chores