fix(security): force-redact context compression summaries - #20998
fix(security): force-redact context compression summaries#20998qWaitCrypto wants to merge 1 commit into
Conversation
egilewski
left a comment
There was a problem hiding this comment.
Recommendation: request changes
I reviewed this against current GitHub main 36ae958473b8530ffb1a395c4944b8cdbcae82fe, PR base 49c3c2e0d37c96dc593a807a5e81fdf4f0aa3d85, and PR head 81a54d1a31f1b4789866eb5fb74704efcd16d8d2.
Validation:
gh pr view 20998 --repo NousResearch/hermes-agent --json mergeable,mergeStateStatus,baseRefOid,headRefOid: fails mergeability withCONFLICTING/DIRTYon the same PR head.git fetch --no-write-fetch-head --no-tags upstream main:refs/remotes/upstream/main +pull/20998/head:refs/remotes/upstream/pr/20998: fetched current main and PR head successfully.git rev-list --left-right --count refs/remotes/upstream/main...refs/remotes/upstream/pr/20998: reports4068 1, so this branch is far behind current main.git merge-tree --write-tree refs/remotes/upstream/main refs/remotes/upstream/pr/20998: fails with a content conflict inagent/context_compressor.py.
Finding:
The PR cannot currently be reviewed or merged safely against current main because the security change conflicts in agent/context_compressor.py. Please rebase or port the patch onto current main, then rerun the focused context-compressor tests from the PR (python -m pytest tests/agent/test_context_compressor.py -q) so the forced-redaction behavior can be validated on the actual merge candidate.
Signed: GPT-5.5-xhigh in Codex
|
Thanks for the careful write-up and tests — but we're going to pass on this one. The premise doesn't hold up: if a value is genuinely a secret, it's already redacted upstream when it first flows through tool output / logs (redaction is on by default). By the time That's the other half: No behavior change needed on main. Appreciate the contribution regardless. |
What does this PR do?
This fixes a security boundary gap in context compaction summary generation.
ContextCompressor._serialize_for_summary()prepares conversation history for the auxiliary summarizer, and the resulting summary may persist across later compactions. The method docstring already states that content is redacted before serialization so secrets do not leak into the auxiliary model or into compacted summaries. In practice, the summary path was callingredact_sensitive_text()withoutforce=True, which meant default installations with global redaction disabled could still send raw secrets through this path.This PR makes context-compression summaries always enforce redaction at that boundary:
This keeps the existing user-facing redaction preference unchanged everywhere else while ensuring the compaction summary path never returns raw secrets.
Related Issue
No tracked issue.
This fixes a source-level security mismatch between the documented behavior of
ContextCompressorand the actual summary-serialization path.Type of Change
Changes Made
agent/context_compressor.pyto callredact_sensitive_text(..., force=True)when serializing message content for summary generationagent/context_compressor.pyto callredact_sensitive_text(..., force=True)when serializing assistant tool-call arguments for summary generationagent/context_compressor.pyto force-redact auxiliary summarizer output before storing the compacted summarytests/agent/test_context_compressor.pycovering:How to Test
python -m pytest tests/agent/test_context_compressor.py -qTestSummarySerializationRedactioncoverage_serialize_for_summary()/_generate_summary()no longer retain raw secrets in serialized summary contentChecklist
Code
fix(scope):,feat(scope):, etc.)pytest tests/ -qand all tests passDocumentation & Housekeeping
docs/, docstrings) — or N/Acli-config.yaml.exampleif I added/changed config keys — or N/ACONTRIBUTING.mdorAGENTS.mdif I changed architecture or workflows — or N/AScreenshots / Logs
Validation run:
python -m pytest tests/agent/test_context_compressor.py -q 71 passed, 71 warnings in 68.73sManual verification with global redaction disabled confirmed the fix closes both leak paths:
_serialize_for_summary()no longer retains the original secret value in serialized message content, tool results, or tool-call arguments_generate_summary()no longer retains the original secret value when the auxiliary summarizer echoes it back