fix(e2e): redact replay evidence and clean up compose resources - #1208
Conversation
- redact credentials across all replay evidence sinks - preserve the normal live replay schema - clean up Compose resources after success or failure - preserve original build, startup, and harness exit codes
There was a problem hiding this comment.
Pull request overview
This PR hardens the Bub e2e replay harness by (1) redacting credentials at the evidence “sink” (so secrets can’t leak into persisted artifacts regardless of where they originated) and (2) ensuring Docker Compose resources are reliably cleaned up via process-lifecycle traps.
Changes:
- Add an
EvidenceRedactorand enforce redaction before writingreplay.json,eval-report.json, andreport.md. - Introduce Compose
EXIT/INT/TERMtraps so containers/networks/volumes are removed on both success and failure without masking the original exit code. - Add regression tests for both evidence redaction and Compose lifecycle cleanup; run these tests under
make harness-check.
Reviewed changes
Copilot reviewed 8 out of 9 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| Makefile | Runs the Bub harness pytest suite as part of harness-check. |
| e2e/bub/uv.lock | Locks new dev dependency metadata/packages for the harness tests (pytest + deps). |
| e2e/bub/tests/test_run_script.py | Adds regression tests asserting Compose cleanup and exit-code behavior across failure modes. |
| e2e/bub/tests/test_evidence_redaction.py | Adds regression tests ensuring all evidence sinks are redacted and “normal” replays remain schema/JSON-equivalent. |
| e2e/bub/src/powercontext_e2e/runner.py | Routes artifact writing through the redactor and redacts Markdown output at the sink. |
| e2e/bub/src/powercontext_e2e/redaction.py | Introduces the unified evidence redactor with env-derived secrets + structural credential detection. |
| e2e/bub/run.sh | Adds lifecycle traps to always run docker compose down and preserve failure status. |
| e2e/bub/README.md | Documents the new “always cleanup” Compose behavior. |
| e2e/bub/pyproject.toml | Adds pytest as a dev dependency group and ruff per-file ignore for test asserts. |
Suppressed comments (2)
e2e/bub/src/powercontext_e2e/redaction.py:87
- The free-text credential assignment redaction pattern omits
token,refresh_token, andproxy_authorization. As a result, evidence liketoken=.../refresh_token: .../proxy_authorization=...can leak unless the exact secret value was also discovered from the environment.
_CREDENTIAL_ASSIGNMENT = re.compile(
r"(?i)(\b(?:access[_-]?token|api[_-]?key|auth[_-]?token|authorization|client[_-]?secret|password|secret)"
r"\b\s*[:=]\s*)(?:(?:basic|bearer)\s+)?[^\s,;&\"'`]+"
)
e2e/bub/src/powercontext_e2e/redaction.py:91
- The CLI option redaction pattern doesn't include
--refresh-tokenor--proxy-authorization, even though the corresponding field names are treated as sensitive. If these flags appear in logs/errors included in evidence, their values could leak.
_CREDENTIAL_OPTION = re.compile(
r"(?i)(--(?:access-token|api-key|auth-token|client-secret|password|token)\s+)"
r"[^\s,;\"'`]+"
)
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
e2e/bub/src/powercontext_e2e/runner.py:395
_redact()currently callsEvidenceRedactor.from_environment()on every invocation, which rescans all environment variables each time._redact()is called inside the per-session loop (e.g., foroutputand exception strings), so this can add avoidable overhead on scenarios with many sessions.
Consider caching the computed redactor for the lifetime of the process (the environment is effectively static during a run).
def _redact(value: str) -> str:
return EvidenceRedactor.from_environment().redact_text(value)
…ext into fix/evidence-redaction
PsiACE
left a comment
There was a problem hiding this comment.
I suggest simplifying this: redact known runtime secrets at the evidence sink, then use TruffleHog before publication. If scanning fails, skip summary and artifact upload without failing CI. Keep tests on observable behavior, not regex coverage, field locations, or Docker command order.
Got it, I'll adjust things in that direction. |
|
Thanks for catching this. After looking more closely at the CI path, I think live replay should stay local for now because uploaded evidence may leak credentials. Contributors should still run it when changing this flow. This does not block the fixes in this PR. What do you think? |
👌,I fully agree with this approach. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated 1 comment.
Suppressed comments (2)
Makefile:51
pytestis declared only in thedevdependency group ine2e/bub/pyproject.toml, but this target runs it without enabling that group.uv run --project e2e/bub python -m pytest ...is likely to fail becausepytestwon't be installed in the project environment by default.
@uv run --project e2e/bub python -m pytest e2e/bub/tests
e2e/bub/src/powercontext_e2e/runner.py:52
- The PR description states the evidence redactor recognizes patterns (bearer creds, authorization fields, URL userinfo/query params, DB URLs) and recursively processes structured data. The implementation here only collects a fixed allowlist of environment values and does substring replacement, so it won't redact credentials that are derived from those env vars or appear only as structured auth headers/URLs not matching the exact env value.
_EVIDENCE_SECRET_ENVIRONMENT_NAMES = (
"ANTHROPIC_API_KEY",
"BUB_API_KEY",
"DEEPSEEK_API_KEY",
"OPENAI_API_KEY",
"OPENROUTER_API_KEY",
"POWERCONTEXT_CLIENT_API_TOKEN",
"POWERCONTEXT_SERVER_AUTH_TOKEN",
"POWERCONTEXT_SERVER_DATABASE_URL",
)
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
e2e/bub/src/powercontext_e2e/runner.py:467
- Evidence redaction currently only performs literal replacement of known environment values. This does not implement the PR’s stated pattern-based protections (e.g., Bearer tokens, URL userinfo, sensitive query parameters) and can still leak credentials that are not exact env var values (or appear only in derived forms) into replay.json/eval-report.json/report.md.
def _write_evidence(path: Path, content: str, secrets: tuple[str, ...]) -> None:
for secret in secrets:
content = content.replace(secret, _REDACTED)
content = content.replace(json.dumps(secret, ensure_ascii=False)[1:-1], _REDACTED)
path.write_text(content, encoding="utf-8")
Comment out the live replay job and its associated steps in the e2e-harness workflow.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 8 out of 9 changed files in this pull request and generated no new comments.
Suppressed comments (1)
e2e/bub/src/powercontext_e2e/runner.py:467
- The PR description says the evidence redactor recognizes bearer credentials, authorization fields, URL userinfo, and sensitive query parameters. In the current implementation,
_write_evidenceonly replaces exact environment-secret values (and their JSON-escaped form), so credentials that are URL-encoded/embedded in URLs or appear asBearer <token>without an exact env-value match can still leak intoreplay.json/eval-report.json/report.md. Consider extending_write_evidenceto also redact common credential patterns and URL-encoded variants to match the stated behavior.
def _write_evidence(path: Path, content: str, secrets: tuple[str, ...]) -> None:
for secret in secrets:
content = content.replace(secret, _REDACTED)
content = content.replace(json.dumps(secret, ensure_ascii=False)[1:-1], _REDACTED)
path.write_text(content, encoding="utf-8")
…nbase#1208) * fix(e2e): redact replay evidence and clean up compose resources - redact credentials across all replay evidence sinks - preserve the normal live replay schema - clean up Compose resources after success or failure - preserve original build, startup, and harness exit codes * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * fix: copilot comment * improve:simplify the method for handling evidence * Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> * Enable inclusion of hidden files in artifact upload * Comment out live replay job in e2e-harness.yml Comment out the live replay job and its associated steps in the e2e-harness workflow. --------- Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Which issue or RFC does this PR close?
Closes #.
Rationale for this change
The Bub replay harness previously redacted credentials only at selected upstream call sites. Judge exceptions, span attributes, evaluation reasons, failures, and other values could bypass those scattered checks and persist provider credentials or authorization tokens in
replay.json,eval-report.json, orreport.md.The Compose runner also used
set -ewhile callingdocker compose downonly after the successful execution path. A failed image build, Server startup, or harness execution could therefore leave containers, networks, and database volumes behind. Cleanup failures could also obscure the original failure status.This change enforces credential redaction at the evidence sink and Compose cleanup at the process lifecycle boundary, giving every execution path the same guarantees.
What changes are included in this PR?
replay.jsoneval-report.jsonreport.mdEXIT,INT, andTERMtraps before the Compose build:make harness-check.Are there any user-facing changes?
NONE
How was this change tested?
The added regression coverage verifies that:
31,32, and33;1, while Compose output confirms that the container, volume, and network were removed;Commands run:
Results:
AI usage statement