fix(agent-os): add opt-in PII redaction to CredentialRedactor.redact

[PratikDhanave (PratikDhanave)]

Summary

CredentialRedactor documents itself for cleaning data "before persisting audit payloads or returning tool output to callers", but redact() (and the redact_data_structure/redact_mapping/redact_dictionary helpers) only iterated PATTERNS (secrets). PII detected by find_pii_matches() — email, phone, SSN, credit card, IP — was detected but never removed. A caller reading the docstring could reasonably assume PII was scrubbed when persisting or returning data, when in fact it flowed through untouched.

Fixes #3239

Change

• Add a keyword-only redact_pii flag (default False, fully backwards compatible) to redact(), redact_data_structure(), redact_mapping(), and redact_dictionary().
• When redact_pii=True, PII spans are merged with secret spans and removed in the same single pass, so an overlapping secret/PII region is redacted exactly once (reusing the existing span-merge logic).
• Clarify the class and redact() docstrings to state explicitly that redaction is secrets-only unless redact_pii=True — resolving the "docs imply PII is scrubbed" trap directly.

Keeping PII opt-in (rather than always-on) preserves current behavior for every existing caller and matches the governance model where PII handling is policy-driven (report vs. block vs. scrub).

The SSN-pattern-divergence half of #3239 was already resolved separately by #3531, so this PR only addresses the redaction-coverage gap.

Testing

Added tests covering the new behavior in tests/test_credential_redactor.py:

• redact() leaves each PII sample (email/SSN/phone/credit-card/IP) untouched by default, and confirms each sample is detected-as-PII-but-not-a-secret.
• redact(..., redact_pii=True) scrubs each PII sample.
• redact_pii=True still removes secrets, and the default still removes secrets while preserving PII (mixed secret+PII string).
• redact_data_structure(..., redact_pii=True) scrubs PII recursively at every depth; redact_dictionary/redact_mapping accept the flag.

Full test_credential_redactor.py suite passes (87 tests). Changes are lint/format-clean against the repo's ruff config (verified the added lines produce no ruff format diff and stay within the 100-char limit).

[github-actions]

Welcome to the Agent Governance Toolkit! Thanks for your first pull request.
Please ensure tests pass, code follows style (ruff check), and you have signed the CLA.
See our Contributing Guide.

[azure-pipelines]

Azure Pipelines:
There may be pipelines that require an authorized user to comment /azp run to run.

[github-actions]

PR Review Summary

Check	Status	Details
🔍 Code Review	⚠️ Missing	No current-run comment
🛡️ Security Scan	⚠️ Missing	No current-run comment
🔄 Breaking Changes	⚠️ Missing	No current-run comment
📝 Docs Sync	⚠️ Missing	No current-run comment
🧪 Test Coverage	⚠️ Missing	No current-run comment

Verdict: ⚠️ AI review incomplete; ready for human review

AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims.

[Copilot]

🟢 Ready to approve

Core behavior and API changes are correct and tested; only minor test/comment cleanups remain.

This review doesn't count toward merge requirements. Sign up for the private preview to control whether Copilot approvals count.

Pull request overview

TL;DR: 0 blockers, 2 warnings. Safe to merge; the warnings are fine as follow-ups.

#	Sev	Issue	Where
1	Warn	“reserved-range” wording is inaccurate for some samples	tests/test_credential_redactor.py
2	Warn	New assertions use is True / is False boolean identity checks	tests/test_credential_redactor.py

Adds an opt-in redact_pii capability to the Agent OS CredentialRedactor so callers can explicitly scrub PII/CRI in addition to secrets, while keeping the default behavior secrets-only (backwards compatible) and clarifying docs accordingly.

Changes:

• Add keyword-only redact_pii: bool = False to redact() and the structure/mapping helper APIs.
• When enabled, merge PII spans with credential spans and redact in a single merged-span pass.
• Add tests validating default (secrets-only) behavior and opt-in PII scrubbing across strings and nested structures.

File summaries

File	Description
agent-governance-python/agent-os/src/agent_os/credential_redactor.py	Adds opt-in redact_pii plumbing + doc clarifications for secrets-only default.
agent-governance-python/agent-os/tests/test_credential_redactor.py	Adds coverage for default vs opt-in PII redaction and recursive structure handling.

Review details

• Files reviewed: 2/2 changed files
• Comments generated: 2
• Review effort level: Lite

We're testing this review assessment. Please use 👍 or 👎 to tell us if it's correct.

[liamcrumm]

redact, redact_mapping, redact_dictionary and redact_data_structure all take redact_pii, but scan_and_redact does not, and its docstring describes it as the one call a host should use to clean text before returning it to a model. So the recommended entry point cannot opt in. Merging this before #3575 also exposes the quadratic email scan to redact(), around 9.7 seconds on 48 KB.

[Copilot]

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Suppressed comments (1)

agent-governance-python/agent-os/tests/test_credential_redactor.py:421

• Avoid is True / is False boolean comparisons in assertions; they can behave unexpectedly with non-bool truthy/falsey values and are disallowed by this repo’s Python guidelines. Use direct truthiness / not instead.

    assert CredentialRedactor.contains_pii(sample) is True
    assert CredentialRedactor.contains_credentials(sample) is False

[Imran Siddique (imran-siddique)]

The gap is real and the shape of the fix is right.

CredentialRedactor describes itself as being for cleaning data before persisting audit payloads or returning tool output to callers, and redact() iterated PATTERNS only. So PII that find_pii_matches() had already located was detected and then handed back in the output. A detector whose findings are discarded at the point of use is worse than no detector, because the class name and docstring both promise the opposite.

Three things you got right that a smaller fix would have missed.

redact_pii: bool = False keyword-only. Every existing caller keeps its exact behaviour, so this cannot silently change what lands in an audit payload someone is already parsing. For a redactor that is the correct default, however tempting the opposite is: changing what gets scrubbed underneath existing consumers is a data-shape change, not an improvement.

Merging overlapping secret and PII spans. A credential containing an email, or the reverse, would otherwise produce nested or doubled placeholders and a mangled output. That is the detail that turns up in production rather than in review.

Justifying the default rather than only stating it. "PII handling is often policy-driven (report vs. block vs. scrub), so callers choose when to strip it" is the right reason, and it is why this can land without settling PII policy for anyone. It makes the capability reachable, which is what find_pii_matches existing without a redaction path already implied.

One thing to be aware of that neither PR mentions. #3706 by Abhinav Tiwari (@erensh27) documents the current limitation in the same file, adding docstrings that say PII "is not removed by redact". If this merges first, that text becomes wrong and needs to describe the opt-in instead. If that merges first, this PR rewrites text just added. Whoever takes either should take both. I have said the same on that PR.

Your only red check is Developer Certificate of Origin: the commit needs a sign-off. git commit --amend -s then force-push clears it.

No approve bit on this repo, so a comment, but once signed off I would merge it.

[Imran Siddique (imran-siddique)]

I filed #3239 and this is the right answer to it. Read the diff rather than the summary.

The defect was real and this closes it. On main at b5705588, redact() was (cls, value: str | None) -> str and iterated PATTERNS only, while PII_PATTERNS at line 159 was reachable solely through the detection path at 217. So find_pii_matches() reported email, phone, SSN, credit card and IP, and redact() left every one of them in the output. The class documents itself for cleaning data "before persisting audit payloads or returning tool output to callers", which is precisely the read that gets someone into trouble.

Keyword-only and defaulting to False is the correct shape. *, redact_pii: bool = False cannot be passed positionally by accident, and no existing caller changes behaviour. On a redaction API, a flag that silently starts removing more than it did is as much of a hazard as one that removes too little, because callers downstream may be matching on the output.

Threading it through all four entry points is what makes it usable. redact, redact_mapping, redact_dictionary and redact_data_structure each take it and pass it down through the recursion. A version that added the flag to redact() only would have looked complete and silently failed on any nested payload, which is the shape audit data actually arrives in.

Two notes, neither blocking:

The docstring change matters as much as the code. #3706 by Abhinav Tiwari (@erensh27) is open and does the documentation half on its own. Once this merges, that PR's premise is no longer true, and the wording here supersedes it. Worth landing this one first and closing that with credit.

The SSN half of #3239 is already done, in #3531, so this PR correctly does not touch it.

Approving. I have push but not merge on this repository, so this needs a maintainer.

[MohammadHaroonAbuomar]

The only CI run on this head is from mid-August and its required checks have expired, so the merge gate cannot see them. Please rebase onto current main and force-push to trigger a fresh run. The content is already reviewed and approved; nothing else is needed.

[MohammadHaroonAbuomar]

Verified: opt-in redact_pii passthrough on CredentialRedactor.redact, byte-identical rebase re-checked, adversarial pass clean on current main. Two docstring nits noted for a follow-up, not blocking.

[MohammadHaroonAbuomar]

Re-approving on head 6785b12: a mechanical merge of main over the already verified content (tree hash checked), CI green.

[MohammadHaroonAbuomar]

Approved and green, but the branch protection requires the most recent push to be approved by someone other than the person who pushed it, and the last update here was made from the maintainer side. One push from you (a rebase onto current main, or an empty commit) resets that and lets the existing approval land the PR. Nothing else is needed.

[MohammadHaroonAbuomar]

The merge of #3955 touched agent-governance-python/agent-os/tests/test_credential_redactor.py and this branch now conflicts with main on that file. Please rebase onto main and push; the approvals stand and auto-merge is armed, so it will merge once a co-maintainer re-approves the pushed head.

[PratikDhanave (PratikDhanave)]

Rebased onto the current main (was CONFLICTING after the branch fell behind). The only conflict was in tests/test_credential_redactor.py, where main and this branch had each appended an independent test block (the #3933 credential-underscore tests upstream, the #3239 PII tests here) — resolved as a union, no logic changed. src/agent_os/credential_redactor.py merged cleanly.

Verified on Python 3.11 against the new tip: pytest tests/test_credential_redactor.py → 120 passed. Commit is DCO-signed.

Imran Siddique (@imran-siddique) — since the rebase rewrote the commit, your approval dropped off the new head; would you mind re-reviewing when you get a chance? Nothing changed in the fix itself.

[MohammadHaroonAbuomar]

Re-approving at 7c3c672: a faithful rebase of the twice-approved content onto current main. The source patch is byte-identical; the test additions are identical apart from their placement after the #3955 tests and two blank lines at the seam. 120 redactor tests pass, spell check clean, single signed-off commit by the author.