feat(plugins): redaction pattern registry — vendor token formats as plugins - #58550
feat(plugins): redaction pattern registry — vendor token formats as plugins#58550webdevtodayjason wants to merge 4 commits into
Conversation
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating the redaction-format gap behind a generic plugin seam. Current main still keeps vendor token formats in agent/redact.py:71-113, so the underlying gap is real.
Problems
plugins/nvapi-redaction/plugin.yaml:1adds a vendor-specific NVIDIA plugin to the core tree.AGENTS.md:797-813requires new third-party vendor integrations to ship as standalone plugin repositories rather than bundled directories.- In commit
8ba64514, the validation inagent/redact.pydoes not uphold its stated no-redact-everything guarantee:ab|.*compiles and has the acceptedabliteral prefix, while the.*branch remains unprefixed._extract_literal_prefix()stops at|(agent/redact.py:756-768on main). The added tests do not cover this shape.
Suggested changes
- Publish
nvapi-redactionindependently and keep this PR focused on the generic surface. - Restrict accepted regex structure or reject alternation/unprefixed branches, with a regression test for
ab|.*.
This is an automated hermes-sweeper review.
| @@ -0,0 +1,4 @@ | |||
| name: nvapi-redaction | |||
There was a problem hiding this comment.
This introduces an NVIDIA-specific vendor integration under plugins/. AGENTS.md:797-813 requires new third-party integrations to be published as standalone plugins instead; please remove this bundled reference plugin and publish it independently.
|
Both fixed in 165e0e68f. The alternation bypass was a real hole, good catch. register_redaction_patterns now rejects any pattern with a top-level alternation ('ab|.' and 'ab|cd' both refused with a warning that points authors at the grouped form). Grouped alternation after the prefix (ab(?:x|y)), escaped pipes, and character-class pipes remain accepted, each pinned by its own test, and the regression test covers your exact 'ab|.' shape. The bundled nvapi-redaction plugin is removed per the AGENTS.md standalone-repo policy; I'll publish it independently. The end-to-end register() coverage that used it now runs through a synthetic plugin written at test time, so the PR stays focused on the generic surface. Suites: tests/test_redaction_registry.py 15 passed, tests/agent/test_redact.py unchanged and green (148 total across both). |
53f4ace to
b30afb7
Compare
…lugins Every new vendor token format has required a core PR appending to _PREFIX_PATTERNS in agent/redact.py (fw_, retaindb_, hsk-, mem0_, brv_ all landed that way; NousResearch#58466/NousResearch#58501 are the latest of the class). This adds an additive-only registry so provider plugins own their format: - agent/redact.py: register_redaction_patterns(patterns, source) — validates each pattern (must compile; must start with >=2 literal characters so the pre-screen substring gate keeps working and redact-everything patterns like `.*` are structurally impossible), dedupes against built-ins and prior registrations, then atomically rebuilds _PREFIX_RE and _PREFIX_SUBSTRINGS. Registered patterns get identical treatment to built-ins everywhere: same head/tail masking, same non-reusable «redacted:label…» sentinel on file_read, same security.redact_secrets operator opt-out. Additive-only by design — a plugin can extend masking, never weaken it. Includes a test/teardown reset helper. - hermes_cli/plugins.py: PluginContext.register_redaction_patterns() delegating with per-plugin attribution; warns and returns 0 on any failure so a broken plugin can never break startup. - Bundled reference plugin `nvapi-redaction` (opt-in): masks NVIDIA API keys (nvapi-, used by NIM / build.nvidia.com) — a real format missing from core, shipped as the one-liner plugin that previously would have been a one-line core PR. 13 new tests: baseline gap, masking + built-ins unaffected, invalid regex / no-literal-prefix / dedupe / non-string rejection, file_read sentinel labeling, reset semantics, PluginContext wiring incl. exception isolation, and a no-mocks end-to-end through the demo plugin. Existing redaction suites (tests/agent/test_redact.py, tests/tools/test_kanban_redaction.py) pass untouched. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FWMcB7RPSYUpsXDfBgwjzM
…undle demo plugin 'ab|.*' compiled and carried the accepted 'ab' literal prefix while its '.*' branch stayed unprefixed, escaping the no-redact-everything guarantee (_extract_literal_prefix stops at '|'). Registration now rejects top-level alternation with a regression test for exactly that shape; grouped alternation after the prefix, escaped pipes, and character-class pipes remain accepted. The bundled nvapi-redaction reference plugin is removed per repo policy (vendor integrations ship as standalone plugin repos); the end-to-end register() coverage now uses a synthetic plugin written at test time. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnMCvi2vXqfs996AjVeF2F
Nested unbounded quantifiers ((a+)+, (?:x*)*, (a{2,})+) backtrack
catastrophically, and registered patterns run against every log line
and tool output, so a pathological pattern from a buggy plugin would
stall the host process. Registration now rejects the structural
nesting shape with a logged warning, same fail-soft contract as the
other validators.
Detection is a hand-rolled scanner matching the top-level-alternation
check's idiom: escapes and character classes skipped, group stack
tracks whether each group body contains an unbounded repeat, reject
when such a group closes into an unbounded quantifier. Overlapping
alternation ambiguity ((a|aa)+) is documented as out of scope.
Also refreshes the test module docstring left stale by the demo-plugin
unbundling.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnMCvi2vXqfs996AjVeF2F
…ebuild proof Two review items raised on NousResearch#65449 (thanks @hansai-art): 1. Explicit test that post-module-load registration REBUILDS the _PREFIX_SUBSTRINGS pre-screen tuple — plugin patterns flow through the same fast path as built-ins, never around it. This was covered implicitly by the masking tests; now it is asserted directly. 2. Plugin patterns are now stored keyed by registration source, giving the NousResearch#64229 lifecycle/ownership-ledger work a clean seam to drop one plugin's patterns on unload. No public removal API is added — additive-only stands; unload remains a host-owned lifecycle concern. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnMCvi2vXqfs996AjVeF2F
b30afb7 to
02f729b
Compare
|
Merged via #84927 with your commit intact (rebase-merge, authorship preserved) — ctx.register_redaction_patterns() is on main with the over-redact-only construction, plus salvage hardening (top-level alternation + ReDoS-shape rejection, per-source attribution). Per repo policy the nvapi reference plugin moved out-of-tree (the e2e test builds a synthetic plugin instead). Thanks @webdevtodayjason! Closing this original. (#65449) |
Tracking home: #65449 (Phase 1 of the plugin-interface expansion tracker #64182). Rebased onto current main; the safety properties below are the review bar adopted there.
What does this PR do?
Third piece of the plugin-interface expansion work (with #58524 and #58538): an additive-only redaction pattern registry, so provider plugins own their vendor's token format instead of patching core.
Every new vendor token format has historically been a one-line core PR appending to
_PREFIX_PATTERNSinagent/redact.py. That is howfw_,retaindb_,hsk-,mem0_, andbrv_got there, with #58466/#58501 the latest of the class. This makes that class plugin territory:agent/redact.py:register_redaction_patterns(patterns, source)validates each pattern, dedupes against built-ins and prior registrations, then atomically rebuilds_PREFIX_RE/_PREFIX_SUBSTRINGS. Registered patterns get identical treatment to built-ins everywhere they apply: same head/tail masking, same non-reusable«redacted:label…»sentinel onfile_readcontent ([Bug]: redact_sensitive_text corrupts API keys in config files when read via read_file/search_files, causing 401 #35519 semantics), samesecurity.redact_secretsoperator opt-out..*structurally impossible); must not contain a top-level alternation (ab|.*would escape the literal-prefix guarantee through its unprefixed branch); and must not nest unbounded quantifiers ((a+)+-style ReDoS shapes are rejected at registration, since registered patterns run against every log line and tool output). Invalid entries are warned and skipped, never raised, so a broken plugin cannot break startup.hermes_cli/plugins.py:PluginContext.register_redaction_patterns()with per-plugin attribution; warns and returns 0 on any failure.nvapi-redactionreference plugin was removed from this PR. End-to-endregister()coverage runs through a synthetic plugin written at test time;nvapi-redaction(NVIDIAnvapi-keys, used by NIM / build.nvidia.com endpoints) is ready to publish as a standalone plugin repo once this lands.Zero behavior change without plugins
With no plugin registered,
_PREFIX_RE/_PREFIX_SUBSTRINGSare byte-identical to today's. Existing redaction suites (tests/agent/test_redact.py,tests/tools/test_kanban_redaction.py) pass untouched.Testing
17 tests in
tests/test_redaction_registry.py: baseline gap documentation, masking + built-ins unaffected, invalid-regex / no-literal-prefix / top-level-alternation / nested-unbounded-quantifier / dedupe / non-string rejection, accepted-shape coverage (grouped alternation, literal pipes, bounded quantifiers inside unbounded groups, quantifier chars in character classes),file_readsentinel labeling, reset semantics,PluginContextwiring incl. exception isolation, and a no-mocks end-to-end through a synthetic plugin written at test time.ruff checkclean.🤖 Generated with Claude Code
https://claude.ai/code/session_01FWMcB7RPSYUpsXDfBgwjzM