docs: clarify redact() covers secrets only, PII is detection-only

[Abhinav Tiwari (erensh27)]

Fixes #3239

redact() / redact_data_structure() iterate only PATTERNS (secret-like material); PII/CRI is detected by find_pii_matches() / contains_pii() but never removed by redact(). Clarified this scope in the class docstring, redact(), and redact_data_structure() so callers do not assume PII is scrubbed when persisting or returning data. (The SSN pattern divergence noted in the issue was already reconciled in a prior change.)

[azure-pipelines]

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

[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.

[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.

[github-actions]

🟡 Contributor Check: MEDIUM

Check	Result
Profile	MEDIUM
Credential	LOW
Overall	MEDIUM

Automated check by AGT Contributor Check.

[Imran Siddique (imran-siddique)]

Accurate today, and it conflicts with an older open PR that nobody has linked.

#3565 by PratikDhanave (@PratikDhanave), open since 2026-08-01, adds opt-in PII redaction to the same file: redact(value, *, redact_pii=False), with overlapping secret and PII spans merged. Default behaviour is unchanged, so the secrets-only path you are documenting stays the default either way.

The consequence is just sequencing, but somebody has to absorb it. Your new text says PII "is not removed by redact", which stops being true once that lands; it would need to say PII is not removed by default, and point at redact_pii=True. If yours lands first, that PR rewrites docstrings it just inherited.

I have endorsed #3565 as the one to lead, on the grounds that "we detect PII and return it to you anyway" is a weak resting place for a governance toolkit even when it is documented honestly, and that the opt-in closes it without deciding PII policy for anyone. That is a maintainer call rather than mine.

Concretely, the cheapest path: let #3565 land, then rebase this and adjust the three notes to describe the flag. The docstrings are otherwise the right ones to touch and the wording is good, so it would be a small edit rather than a rewrite.

Separately, worth a look before this can merge regardless: it currently fails Developer Certificate of Origin, gitleaks, ci-complete and test (agent-os, 3.11). The DCO one is an unsigned commit and the gitleaks failures across this repo today were a platform 503, but test (agent-os, 3.11) failing on a docstring-only change is not obviously environmental and I have not diagnosed it. Worth checking whether it reproduces on a rebase before assuming it is noise.

[Imran Siddique (imran-siddique)]

Abhinav Tiwari (@erensh27) thank you for this, and the reading behind it is correct: redact() is secrets-only today and the class docstring does imply otherwise. That is exactly the defect I described in #3239, and documenting it was one of the two remedies I offered there.

I am going with the other one. #3565 adds the opt-in redact_pii path, keyword-only and defaulting to False, threaded through redact, redact_mapping, redact_dictionary and redact_data_structure. I have approved it.

The reason is not that your change is wrong, it is that a caveat leaves the sharp edge in place. Someone who runs find_pii_matches(), sees five hits, calls redact() and persists the result still ships PII. A docstring warning helps the person reading the source and does nothing for the person reading the function name. Given the class exists to clean data before persisting audit payloads, having no way at all to remove PII was the gap worth closing.

Your docstring wording is good and some of it should survive. #3565 rewrites the same block, so rather than a rebase-and-conflict, the useful move is a comment on that PR pointing at any phrasing of yours that says it better. I would rather your words landed than the PR did.

Closing this once #3565 merges, not before, so nothing is lost if that stalls.

[Imran Siddique (imran-siddique)]

Verified the claim by running it rather than by reading the code, because "detection-only" is exactly
the kind of statement that is easy to assert and easy to get backwards. Every category you name
behaves the way the new docstring says:

kind    contains_pii   find_pii_matches   redact() output
email   True           1                  'contact bob@example.com now'
phone   True           1                  'call 415-555-0132'
ssn     True           1                  'ssn 123-45-6789'
cc      True           1                  'card 4111111111111111'
ip      True           1                  'host 192.168.1.7'

All five are detected and none is removed. The gap between contains_pii returning True and
redact returning the input unchanged is the whole reason this note needs to exist, and nothing in
the class said so before.

This is the right kind of docs PR: it is not restating the code, it is closing a false expectation
that the class name actively encourages. CredentialRedactor with a PII_PATTERNS attribute and a
redact() method reads as though calling redact() handles both, and a caller who assumes that ships
PII into a log. Putting the note in three places (the class docstring, redact, and
redact_data_structure) is right, because redact_data_structure is the one most likely to be
reached for when someone is sanitizing a whole payload and is furthest from the class docstring.

Two notes, neither blocking.

There is a real design question sitting under this, which the PR correctly does not try to answer:
should redact() grow an opt-in PII mode, or should the PII helpers grow their own redactor? #3565
proposes the first. Documenting the current boundary is the right move regardless of how that lands,
and it should not wait on it.

credential_redactor.py is busy right now. #3853 and #3855 both change the pattern definitions and
#3565 proposes an opt-in PII path. This PR only touches docstrings in the class header and above two
methods, so it should not conflict with any of them, but it is worth landing early rather than late
so it does not accumulate rebases for no reason.

Nothing blocking.

[Imran Siddique (imran-siddique)]

My review above said "nothing blocking" without diagnosing the four red checks, which was
incomplete. Resolving them properly, because three of the four are not your code and only one needs
anything from you.

Developer Certificate of Origin — genuine, and the only thing you need to do. The gate greps
for a Signed-off-by: trailer and your commit has none:

if ! git log -1 --format='%B' "$SHA" | grep -qiE '^Signed-off-by: .+ <.+>'; then

git commit --amend -s then force-push clears it.

gitleaks — infrastructure, not your change. It failed inside the Install Gitleaks step while
fetching the 8.24.3 binary, before scanning anything.

test (agent-os, 3.11) — infrastructure, and it never ran a test. The failing step is
Install OPA (ACS-backed Python policy runtime), and the log shows the download returning HTTP 502
twice:

curl: (22) The requested URL returned error: 502

Worth being explicit that this is not a test failure on a documentation-only PR. It would have been
easy to read the job name and conclude your change broke the agent-os suite.

ci-complete — aggregator. It is reporting the two above, not a fifth problem.

All three infra failures are from the same run window on 2026-08-12, which is consistent with a
transient GitHub-releases outage rather than anything about this branch. Because they are transient
rather than stale-base, gh run rerun genuinely clears them, unlike the spell-check over-scan
affecting some other PRs in this queue.

So the sequence is: amend with a sign-off, force-push, and the push re-triggers everything. That
should take it to green.

[Ricky Gummadi (Ricky-G)]

Thank you for the original clarification. This PR has been open since August 11 and has been stale for more than a month.

The underlying issue, #3239, is already closed by #3565, and the current API now supports opt-in PII redaction through redact_pii=True. I opened replacement PR #4060 to carry the documentation clarification forward on current main, with the default-versus-opt-in behavior stated accurately and the original issue tracked transparently.

I’m closing this stale predecessor so review and agent-merge tracking can happen in the replacement PR. Thank you again for the useful documentation work here.