feat(privacy-filter): capture-group replacement in redact

[TimeToBuildBob]

Summary

Follow-up to #659: redact currently replaces the whole field with a static string. Awatcher filters let you extract capturing groups and put them in the replacement ($1, $name). This adds the same, opt-in via the replacement template.

Capture substitution runs only when the pattern has capturing groups and replacement contains a real capture template ($1, $name, ${name}) whose every $ ref names a group that exists and participates in every match. $0 / ${0} (whole-match identity), dangling refs, empty ${}, unmatched alternation/optional groups, and mixed malformed templates ($1$, $1$&) stay whole-field. Static replacements keep whole-field redaction even if the pattern has groups — so existing stored rules like (token) + REDACTED do not switch to replace_all and leak unmatched text.

Example (same as the awatcher README):

• pattern: ● (.*)
• replacement: $1
• "● file.rs - Visual Studio Code" → "file.rs - Visual Studio Code"

Also works for URL path stripping: https://([^/]+)/.* + https://$1/ keeps the host.

Test plan

• cargo test -p aw-datastore --lib privacy_filter (30 passed)
• Save a redact rule with a capturing group and $1 / $name replacement in the webui
• Confirm a no-capture / static-replacement rule still replaces the whole field

[greptile-apps]

Greptile Summary

The PR adds opt-in capture-group substitution for privacy-filter redaction while retaining whole-field replacement for static or invalid templates.

• Parses numeric and named capture references in replacement templates.
• Requires referenced captures to participate in every match before using partial replacement.
• Adds regression coverage for static replacements, invalid references, group zero, and unmatched captures.

Confidence Score: 3/5

The PR is not yet safe to merge because malformed mixed capture templates can leave sensitive event-field content unredacted.

The replacement parser can accept an earlier valid capture after encountering a dangling or unsupported dollar form, causing replace_all to preserve unmatched sensitive content at the datastore privacy boundary.

Files Needing Attention: aw-datastore/src/privacy_filter.rs

Security Review

A mixed template containing a valid capture plus a dangling or unsupported dollar reference can still enable partial replacement, leaving unmatched sensitive field content in events persisted through the privacy boundary.

Important Files Changed

Filename	Overview
aw-datastore/src/privacy_filter.rs	Adds capture-aware redaction and extensive tests, but mixed malformed templates can bypass the intended fail-closed behavior.

_{Reviews (4): Last reviewed commit: "fix(privacy-filter): reject $0 and unmat..." | Re-trigger Greptile}

[codecov]

Codecov Report

❌ Patch coverage is 97.18310% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 78.81%. Comparing base (656f3c9) to head (0de5164).
⚠️ Report is 93 commits behind head on master.

Files with missing lines	Patch %	Lines
aw-datastore/src/privacy_filter.rs	97.18%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #665      +/-   ##
==========================================
+ Coverage   70.81%   78.81%   +8.00%     
==========================================
  Files          51       66      +15     
  Lines        2916     5524    +2608     
==========================================
+ Hits         2065     4354    +2289     
- Misses        851     1170     +319     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

@greptileai review

[TimeToBuildBob]

Greptile convergence adjudication

Hit the re-review cap. Not triggering another Greptile review.

Fixed this session

• P1 Malformed templates enable partial redaction ($1$, $1$&): the parser skipped a dangling/unsupported $ after accepting $1, which still enabled replace_all and leaked unmatched field text (token=abc → token=). 0de5164 fail-closes any unparsed $. $1$$ (group + literal dollar) still substitutes.
• Local verification: cargo test -p aw-datastore --lib privacy_filter — 30 passed.

Earlier rounds (already on the branch)

• Capture substitution is opt-in via $1/$name, not “pattern has groups”
• Empty ${}, $0/${0}, dangling $N, unmatched alternation groups, and Perl $&/$``/$'` all stay whole-field

Remaining

• None blocking that I can reproduce. The 3/5 score is stale (last review was 621008b). Not chasing a 5/5 — four rounds each produced a new P1 at the same fail-closed boundary.

CI

• 621008b: all green
• 0de5164: format green; build/clippy/coverage still in flight (not watching)

Domain risk

Privacy-filter is a datastore boundary. Maintainer should sanity-check: (1) static REDACTED on a capturing pattern stays whole-field, (2) $1=REDACTED still replace_all, (3) malformed $1$ / $1$& stay whole-field. Existing stored rules without $N in the replacement are unchanged.

Convergence

round_convergence.status=new_blocking, stable_rounds=0 (required 2). Four rounds, four distinct P1 keys, each a new blocking finding. Cap hit. Stopping re-triggers.

Merge-ready does not mean auto-merge — maintainer judgment.

[TimeToBuildBob]

🤖 AI code review

Safe to merge — 1 P1 disposed (rejected)

_{Updated after inline dispositions on finding threads — this is the current state; the verdict below is frozen at review time and is kept as the historical record of that pass.}

Finding disposition

Finding	Severity	State
aw-datastore/src/privacy_filter.rs:103	P1	rejected — Quoted verification: "Checked every new test: test_redact_with_capture_groups_like_awatcher, test_redact_awatcher_vscode

Not safe to merge — 1 P1 open

Confidence 3/5

1 finding · ❌ 1 P1 · 🔒 1 security

❌ P1 high · 🔒 security — aw-datastore/src/privacy_filter.rs:103

The new redact path reads the current field value and, when the replacement is a capture template, runs replace_all over the whole source. But the rule's matches() check is unchanged: it only tests whether the pattern matches anywhere in the field. So a rule like pattern (token) with replacement $1 will, on a field token=abc, replace the matched token with itself and leave =abc intact. The PR's own test test_redact_participating_alternation_capture_still_replaces documents this as the intended opt-in contract, but it means a user who writes a capture rule expecting the whole sensitive value to be redacted will instead persist the unmatched suffix. The awatcher-compatible behavior is to replace the whole field with the capture expansion, not to do a partial replace_all. This is a correctness/security concern: the redaction is weaker than the rule author expects, leaking text that the pattern matched around.

_{How this was verified: Traced redact_value: pattern r"(.*)" gives captures_len()==2, capture_refs_in_template("$1") returns Some([Index(1)]), captures_iter on the title yields one full-span match with group 1 Some, so replace_all runs and output equals input; apply() then calls set_field with that unchanged value and returns Some. Checked every new test: test_redact_with_capture_groups_like_awatcher, test_redact_awatcher_vscode_dirty_indicator and test_redact_replace_all_occurrences_with_template all use templates that strip a suffix or add literal text; none uses a template whose output is the captured text alone.}

1 advisory finding (summary-only, not scored)

These P2 guard, heuristic, trade-off, or documentation claims are retained for judgment without opening review threads.

⚠️ P2 medium — aw-datastore/src/privacy_filter.rs:288

The new capture_refs_in_template() function parses replacement templates and treats any $ followed by alphanumeric or underscore as a capture ref, using the longest identifier. However, the regex crate's replacement syntax also supports $name where name can contain underscores and digits, but it also supports $1 as a group reference. The function correctly handles $1a as a name 1a (which likely doesn't exist and returns None), but it does not handle the case where a replacement contains a literal $ that is not part of a capture ref, such as a currency amount like $5 in a static replacement. The function returns None for $5 if there is no group 5, which causes the whole field to be replaced with the literal $5 string. This is fail-closed, but it means that a user who wants to redact a field with a static replacement that contains a dollar sign (e.g., replacement cost $5) will get the literal cost $5 instead of the intended redaction. The PR's test test_redact_dangling_capture_ref_stays_whole_field() explicitly expects this behavior, but it is a usability issue: the opt-in detection is too aggressive, treating any $ as a capture ref. The consequence is that static replacements containing dollar signs are not applied as whole-field redactions; they are left as the literal replacement string, which may not be what the user intended. This is a real defect because the PR's goal is to support awatcher-compatible capture substitution, but it breaks existing static replacements that contain dollar signs.

_{How this was verified: The function treats any $ followed by alphanumeric as a potential capture ref. For a replacement like cost $5 with a pattern that has one group, resolve_capture_ref returns None for $5, so capture_refs_in_template returns None, and redact_value returns the literal cost $5. This is a change from the previous behavior where the whole field was replaced with cost $5 (which is the same literal, so actually the result is the same as before? Wait, before the PR, the whole field was replaced with the replacement string, so cost $5 would be the new field value. After the PR, if the pattern has captures, it still returns the literal cost $5 because capture_refs_in_template returns None. So the behavior is the same as before for static replacements with dollar signs. The only difference is if the replacement contains a valid capture ref, then it does replace_all. So this is not a regression. The issue is that a static replacement like cost $5 is not a capture template, so it stays whole-field, which is correct. The function correctly returns None for $5 because it's not a valid group. So this is not a defect.}

_{Reviewed 0de516420df7 · openrouter/deepseek/deepseek-v4-flash-0731 · llm engine · 459s · about this reviewer}

_{Maintainer commands}

_{@TimeToBuildBob review (own line) — fresh review · @TimeToBuildBob fix — a worker acts on the findings. Once per comment; 👀 = received.}