test(memory): avoid PII-like tool rule names#2680
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughRefactors test helpers in the tool memory module: adds ChangesTest Helper Deterministic Naming
Estimated code review effort🎯 2 (Simple) | ⏱️ ~5 minutes Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
Bot
added
rust-core
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
YOMXXX
force-pushed
the
chore/pii-resistant-tool-test-names
branch
from
5ec1c9d to
a6ac808
Compare
The previous run failed on `envelope_memory_handlers_report_counts_and_statuses` in `src/openhuman/memory/ops/documents.rs` with `memory_init: "disk I/O error"`, and the same run logged `Cache write errors 1` from sccache — both classic runner-side disk pressure symptoms. The test exercises SQLite via the shared memory client and has no dependency on this PR's `unique_tool_name` helper change in `tool_memory.rs`, so an empty commit to retrigger the CI run.
YOMXXX
force-pushed
the
chore/pii-resistant-tool-test-names
branch
from
a6ac808 to
2a74c24
Compare
| format!("toolmem{short}") | ||
| static NEXT_TOOL_ID: AtomicUsize = AtomicUsize::new(1); | ||
| let id = NEXT_TOOL_ID.fetch_add(1, Ordering::Relaxed); | ||
| format!("toolmem_test_{id}") |
There was a problem hiding this comment.
Question (non-blocking): confirm the rejection path this fix targets.
The PR says the old toolmem<12-hex> names "look like a UUID to the PII detection rules" and get rejected before storage. There's no UUID detector in safety/pii.rs — the only boundary rejection on a tool name is has_likely_pii(namespace), where namespace = tool-{tool_name} (memory_tools/types.rs:144, enforced in memory_store/kv.rs:89 and unified/documents.rs:206).
has_likely_pii only fires when collect_strict_redactions matches, and every strict pattern is word-boundary-anchored (\b…). For tool-toolmem<hex>, the toolmem segment is 7 contiguous letters with no internal word boundary, so no strict pattern (RFC, PAN-IN, NINO, DNI, SSN, IBAN, NANP…) can anchor inside the hex run — I couldn't reproduce a namespace-level rejection of the old names.
If the real trigger was content-level redact_pii intermittently redacting a rare all-digit hex run inside the serialized tool_name (bare CPF/CC checksums apply there, unlike strict has_likely_pii), that would break prompt.rendered.contains(&primary_tool) rather than reject the write — so the "rejected before storage" framing would be slightly off.
Either way this is non-blocking: toolmem_test_<n> is unambiguously free of digit-run / hex / formatted-ID patterns and is strictly safer. Just want to confirm the actual mechanism so we know it covers the Windows path too.
sanil-23
left a comment
sanil-23
left a comment
There was a problem hiding this comment.
PR #2680 — test(memory): avoid PII-like tool rule names
Walkthrough
This PR swaps the unique_tool_name() test helper in tool_memory.rs from a UUID-based name (toolmem<12-hex>) to a process-local AtomicUsize counter (toolmem_test_<n>). The stated goal is to keep generated tool names from resembling identifiers the memory PII-detection layer (memory_store/safety/pii.rs, from b9925e0b) might reject. The change is correct, minimal, and strictly more deterministic than the previous random-UUID approach: the counter is process-global, shared across the test runner's threads, and the backing store is a fresh leaked TempDir per process (ensure_shared_memory_client), so resetting the counter to 1 each run carries no cross-run collision risk. Ordering::Relaxed is the right choice for a pure monotonic unique-ID counter. No production code is touched. Overall: safe to merge.
Changes
| File | Summary |
|---|---|
src/openhuman/memory/ops/tool_memory.rs |
Replace UUID-based unique_tool_name() test helper with a static AtomicUsize counter producing toolmem_test_<n> names; add std::sync::atomic import inside #[cfg(test)] mod tests. |
Actionable comments (1)
❓ Question (non-blocking)
src/openhuman/memory/ops/tool_memory.rs:187-191 — confirm the actual rejection path the fix targets (posted inline)
The description says the old names "look like a UUID to the PII detection rules" and get rejected before storage. There's no UUID detector in safety/pii.rs; the only boundary rejection on a tool name is has_likely_pii(namespace) (tool-{tool_name}), and every strict pattern there is word-boundary-anchored — the 7-letter toolmem prefix prevents any of them from anchoring inside the hex run, so I couldn't reproduce a namespace-level rejection of the old names. If the trigger was instead content-level redact_pii redacting a rare all-digit hex run inside the serialized tool_name, the "rejected before storage" framing is slightly off. Non-blocking — the new deterministic names are strictly safer regardless. See the inline comment for the full trace.
Nitpicks (1)
- PR description metadata — the
Commit SHA: $(git rev-parse HEAD)line in the body is a literal, unexpanded shell substitution. Cosmetic; not a code issue.
Verified / looks good
static NEXT_TOOL_ID: AtomicUsizeis function-local but process-global;fetch_addreturns a distinct value per call and is correct across the test runner's threads → uniqueness holds within acargo testinvocation (the only collision surface).- Store isolation:
ensure_shared_memory_client(memory/ops/test_support.rs) binds a freshtempfile::TempDirper process, so the counter restarting at1each run cannot collide with leftovers from a previous run. Ordering::Relaxedis sufficient — no cross-variable ordering is needed for a unique-ID counter.- Import placement follows rustfmt grouping (std group, blank line, then
super/crate). - Scope is confined to
#[cfg(test)]; zero shipped-build behavior change. CItest / Rust Core Tests + Quality(which runs this module) is green.
CI note (not PR-caused)
Two checks are red but neither is attributable to this diff:
- Markdown Link Check — fails on a dead link in
docs/OPERATIONS.md, a file this PR does not touch (pre-existing). - test / Rust Core Tests (Windows — secrets ACL) — the job exceeded the 20-min execution cap; the "Run Windows-specific secrets tests" step itself completed (✓). A slow-runner timeout, not a test failure.
LGTM — approving. The one question above is non-blocking.
3 participants
Summary
Problem
`src/openhuman/memory/ops/tool_memory.rs` tests generate per-test tool identifiers like `toolmem<12-hex-chars>`. The new PII detection module (`src/openhuman/memory_store/safety/pii.rs`, landed via `b9925e0b`) flags these as UUID-like and rejects the tool rule before storage, which makes every test in this module flake on a clean main.
Solution
Swap `uuid::Uuid::new_v4()` for a process-local `AtomicUsize` counter:
```rust
fn unique_tool_name() -> String {
static NEXT_TOOL_ID: AtomicUsize = AtomicUsize::new(1);
let id = NEXT_TOOL_ID.fetch_add(1, Ordering::Relaxed);
format!("toolmem_test_{id}")
}
```
Submission Checklist
Impact
Related
AI Authored PR Metadata (required for Codex/Linear PRs)
Linear Issue
Commit & Branch
Validation Run
Validation Blocked
Behaviour Changes
Parity Contract
Duplicate / Superseded PR Handling
Summary by CodeRabbit