Add secrets detection tool pre-invoke hook

[lucarlig]

Summary

• add tool_pre_invoke to the secrets detection manifest, Python shim, and Rust core
• scan canonical CPEX ToolPreInvokePayload.args before tool execution and block/redact with existing secrets violation behavior
• share prompt/tool scan result handling to reduce duplicate hook policy code
• add detection-only clean-path scanning for prompt/tool hooks, building redacted copies only when findings exist and redaction is needed
• bump cpex-secrets-detection to 0.3.2
• add direct hook, plugin-manager dispatch, manifest, block+redact, and clean-container coverage

Fixes #93

Tests

• RED: focused tests failed for canonical args, block+redact coverage, and clean nested container identity
• GREEN: focused command passed: 54 passed
• real installed CPEX check: ToolPreInvokePayload(args=...) blocked with SECRETS_DETECTED
• make install
• make test-integration: 81 passed
• make check-all: fmt-check, clippy, and 52 Rust tests passed
• make verify-stubs

Review follow-up

Addressed detailed review findings: canonical args contract, block+redact coverage, clean-path scanner behavior, and duplicated hook result construction.

[msureshkumar88]

Review findings

1. Double-scan on dirty path — document the tradeoff (plugin.rs scan_payload_attr)

When redact=true and secrets exist, the payload is scanned twice:

let (mut count, mut findings) = scan_container_findings(py, &value, &self.config)?; // scan 1
if has_findings(count) {
    let (redacted_count, redacted, redacted_findings) =
        scan_container(py, &value, &self.config)?; // scan 2

Old code scanned once. This trades dirty-path performance for clean-path allocation savings. Reasonable only if clean payloads dominate — add a one-line comment making the intent explicit, e.g.:

// Two-pass: skip copy allocation on clean payloads; dirty path (secrets present) scans twice.

2. Unusual bool-tuple pattern match (plugin.rs)

if let (true, true) = (self.config.redact, has_findings(count)) {

Idiomatic Rust: if self.config.redact && has_findings(count). No bug, but the tuple form is surprising. Appears in both scan_payload_attr and resource_post_fetch.

3. Missing clean-path test for tool_pre_invoke

No test covers tool_pre_invoke with a payload containing no secrets. All other hooks have a clean-path case. Add one asserting continue_processing=True, violation=None, modified_payload=None.

---

Not blocking (follow-up OK):

• resource_post_fetch still uses old single-scan path — doesn't benefit from the scan_container_findings clean-path optimisation. Consistent with the refactor goal but worth a TODO.
• has_findings(count) wraps count != 0 — trivial helper adds indirection without clarity gain; count > 0 inline is equally readable.

[lucarlig]

Addressed review 4272190414 in commit 417d46b.

Changes:

• Documented the two-pass dirty-path tradeoff in scan_payload_attr.
• Replaced tuple bool matches with direct boolean conditions.
• Added a Python integration clean-path test for tool_pre_invoke asserting continue_processing=True, violation=None, modified_payload=None, and empty metadata.

Verified locally:

cargo fmt --check -p secrets_detection
cargo nextest run -p secrets_detection tool_pre_invoke
cargo nextest run -p secrets_detection
make test-integration
make check-all

[msureshkumar88]

Review follow-up — all requested changes verified

Requested changes: done

Finding	Status	Location
Two-pass comment in scan_payload_attr	✅ Done	plugin.rs:158
Tuple bool → direct bool (if let (true, true))	✅ Done	0 matches remain
Clean-path test for tool_pre_invoke	✅ Done	test_plugin_hooks.py:72 — test_tool_pre_invoke_leaves_clean_payload_unmodified

Clean-path test asserts continue_processing=True, violation=None, modified_payload=None, metadata={} — matches the spec from the review.

New issues from the fix: none

scan_findings_inner (pass 1) uses seen: HashSet only; scan_container_inner (pass 2) uses seen + memo: HashMap. Difference is intentional — memo tracks redacted copies for shared subgraphs, not needed for count-only traversal. Finding counts between the two passes are equivalent.

Still open (pre-existing, not-blocking per original review)

• resource_post_fetch not refactored to scan_payload_attr — different structure (content.text nesting), so exclusion is correct
• has_findings helper wrapping count != 0 still present

Approving.

[lucarlig]

@msureshkumar88 the requested changes from review 4272190414 are addressed on head 2b3ed36.

Summary:

• Added the two-pass dirty-path comment in scan_payload_attr.
• Replaced both tuple bool checks with direct && conditions.
• Added clean-path coverage for tool_pre_invoke.
• Added one extra Rust unit test after mutation testing found a surviving resource redaction-branch mutant.

Current verification:

make check-all
# 62 Rust tests passed

make test-integration
# 82 passed

PYO3_PYTHON=$(command -v python3) cargo mutants -p secrets_detection --in-diff cargo-mutants.diff
# 243 mutants tested, 23 caught, 220 unviable, 0 missed

Could you re-review or submit the approval review? GitHub still shows CHANGES_REQUESTED because the follow-up approval was posted as an issue comment, not a PR review.

[msureshkumar88]

Good progress on sharing the hook result construction across all four hooks — the refactor is clean and the two-pass comment (addressing the previous review) reads well.

One blocking issue surfaced during integration testing against a live cpex gateway that needs to be resolved before this ships.

---

Blocking: cast::<PyDict>() rejects Python dict subclasses — secrets go undetected

What's happening

At runtime, the cpex framework's _isolate_payload wraps ToolPreInvokePayload.args in a CopyOnWriteDict (a dict subclass, not a bare dict) before dispatching to plugins. In PyO3 0.28, Bound<PyAny>::cast::<PyDict>() is a raw pointer cast that only succeeds for exact PyDict instances — it rejects subclasses. So when scan_findings_inner (and the existing scan_container_inner) encounters a CopyOnWriteDict, the dict branch is skipped entirely and the scan returns zero findings. No violation is raised, and secrets pass through silently.

This affects tool_pre_invoke specifically because it's the first hook where the framework applies the copy-on-write isolation. prompt_pre_fetch was already affected (pre-existing), and this PR inherits that behaviour in scan_container_inner without addressing it.

Confirmed in production

We reproduced this end-to-end: an AWS_ACCESS_KEY_ID=AKIA... payload passed through the installed 0.3.2 wheel without triggering a violation. Adding type(payload.args) is not dict → model_copy(args=dict(payload.args)) in the Python shim restored correct blocking. The Rust tests pass because they construct payloads with a plain-Python class ToolPayload using a bare dict — the framework's CopyOnWriteDict is never exercised in the test harness.

Fix

Replace cast with downcast at the three container-type checks in scan_findings_inner (new in this PR) and the three matching checks in scan_container_inner (pre-existing, same file):

// Before
if let Ok(dict) = container.cast::<PyDict>() {

// After
if let Ok(dict) = container.downcast::<PyDict>() {

downcast calls Python's isinstance() internally, so it accepts dict subclasses. The same applies to PyList and PyTuple casts in both functions — worth auditing those too, though CopyOnWriteDict is the one the framework actually uses today.

Test gap

The in-process Rust tests use a stub ToolPayload with a plain dict. A test that passes a CopyOnWriteDict (or any dict subclass) as args and asserts the secret is still detected would have caught this. Worth adding one that imports or mocks CopyOnWriteDict — or at minimum uses type('CowDict', (dict,), {})({'message': 'AKIA...'}) as a minimal subclass to verify the downcast path.

---

Everything else (two-pass tradeoff, clean-path allocation skip, the follow-up clean-path test) looks good. Happy to iterate quickly on this one.

[lucarlig]

@msureshkumar88 confirmed the blocking behavior is real and fixed it in df70f11.

One detail: cast::<PyDict>() itself is not exact-only in PyO3 0.28.3; PyDict uses PyDict_Check, not PyDict_CheckExact. The actual failure is that CopyOnWriteDict passes the PyDict type check, but PyDict iteration reads the subclass's empty underlying dict and bypasses its overridden items().

Fix:

• Exact dict still uses fast PyDict iteration.
• dict subclasses now scan through Python items(), so CopyOnWriteDict exposes original values correctly.
• Applied this to both scan_findings_inner and scan_container_inner.

Coverage added:

• Rust detection-only regression for CopyOnWriteDict-style dict subclasses.
• Rust redaction regression for the same shape.
• Python tool_pre_invoke hook regression using CopyOnWriteDict args.

Verified:

make install
make check-all
# 64 Rust tests passed

make test-integration
# 83 passed

PYO3_PYTHON=$(command -v python3) cargo mutants -p secrets_detection --in-diff cargo-mutants.diff
# 344 mutants tested, 27 caught, 317 unviable, 0 missed

[msureshkumar88]

Requested changes addressed in df70f11 — approving.

The fix is right. The clarification on the actual failure mode is helpful: PyDict_Check (not PyDict_CheckExact) does accept the subclass, but PyO3's fast PyDict iterator reads the underlying C-level dict state, which is empty for CopyOnWriteDict since the subclass stores values in a shadow layer and exposes them only through items(). The two-branch pattern — exact instance on the fast path, isinstance + call_method0("items") for subclasses — handles both correctly and avoids introducing Python-level overhead for the common plain-dict case.

Regression coverage (Rust detection + redaction for dict-subclass shape, Python tool_pre_invoke integration with CopyOnWriteDict args) closes the test gap. CI is still running on the new commit; no failures so far, and the earlier run was 34/35 passed.