feat(governance): cross-session GDPR erasure with HMAC-signed receipts (closes #365)#567
feat(governance): cross-session GDPR erasure with HMAC-signed receipts (closes #365)#567arcgod-design wants to merge 5 commits into
Conversation
|
Someone is attempting to deploy a commit to the sreerevanth's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Warning Review limit reached
Next review available in: 56 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThis PR adds a GDPR cross-session right-to-erasure service with HMAC-signed receipts, repository query/delete helpers, and a new authenticated API endpoint. It also adds reasoning style fingerprinting and mid-session style-swap detection to the auditor, with corresponding schema models and tests, plus a changelog entry. ChangesCross-session GDPR erasure
Reasoning style fingerprint and swap detection
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Endpoint as gdpr_erase endpoint
participant Service as CrossSessionErasureService
participant Target as ErasureTarget
Client->>Endpoint: POST /api/v1/gdpr/erase (ErasureRequest)
Endpoint->>Service: erase(request)
loop for each registered target
Service->>Target: count_matching(identifier, scope)
Service->>Target: erase_matching(identifier, scope)
end
Service->>Service: sign receipt via HMAC-SHA256
Service-->>Endpoint: ErasureReceipt
Endpoint-->>Client: ErasureReceipt
sequenceDiagram
participant Caller
participant Auditor as ReasoningAuditor
participant Fingerprint as fingerprint()
participant Detector as detect_mid_session_change()
Caller->>Auditor: fingerprint_session(events, threshold, split_ratio)
Auditor->>Fingerprint: fingerprint(events)
Auditor->>Detector: detect_mid_session_change(events, split_ratio)
Auditor->>Fingerprint: fingerprint(first_half)
Auditor->>Fingerprint: fingerprint(second_half)
Auditor->>Auditor: build StyleSwapAlert + FingerprintReport
Auditor-->>Caller: FingerprintReport
Possibly related PRs
Suggested labels: Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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 |
🧪 PR Test Results
Python 3.12 · commit c93b148 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
agentwatch/api/server.py (1)
1628-1631: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valuePer-session delete loop is N+1.
For agent/user/tenant scopes each session triggers separate event + session deletes.
prune_sessionsalready accepts a list, so sessions can be pruned in one statement after batch-deleting events, reducing round-trips on large erasures.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/api/server.py` around lines 1628 - 1631, The per-session delete loop in the agent/user/tenant erase path is doing N+1 deletes, so update the prune flow to batch work instead of deleting each session one by one. In the logic around get_session_ids, first collect all session IDs, then delete events for those sessions in bulk, and finally call prune_sessions with the full list rather than delete_session inside the loop. Use the existing repo methods get_session_ids and prune_sessions to keep the change localized and reduce round-trips.agentwatch/governance/gdpr.py (1)
213-220: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value
total_start_optis computed but never used.The pre-erasure total is accumulated across targets but never surfaced in the receipt or results, so this block is dead. This looks like an incomplete aggregate (
ErasureReceipthas no total-matched field). Either drop it or expose it on the receipt.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@agentwatch/governance/gdpr.py` around lines 213 - 220, `total_start_opt` in the erasure flow is computed in the target loop but never used, so the aggregation is dead. Update the logic in the governance erasure method that iterates over `self._targets` to either remove the unused pre-erasure count accumulation entirely or thread it through the returned `ErasureReceipt`/results as a new field if it is meant to be reported. Make sure the chosen fix is reflected consistently in the receipt-building path and any related return values so `count_matching` is not called without purpose.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@agentwatch/api/server.py`:
- Around line 1644-1657: The gdpr_erase endpoint currently returns a successful
ErasureReceipt even when CrossSessionErasureService.erase records per-target
errors, so the caller cannot tell failure from a no-op. Update gdpr_erase and
the ErasureReceipt/erasure flow to surface failures explicitly: either raise an
error and return a non-2xx response when any target result has an error, or add
a failure field that includes per-target details and ensure the response schema
reflects it. Use the existing gdpr_erase, CrossSessionErasureService.erase,
ErasureTargetResult, and ErasureReceipt symbols to wire the failure state
through the API.
- Around line 1587-1589: The session erasure signing secret is currently falling
back to an ephemeral os.urandom value in the module-level
_SESSION_ERASURE_SECRET initialization, which makes receipt signatures
non-deterministic across restarts and instances. Update the secret handling in
agentwatch/api/server.py to require AGENTWATCH_ERASURE_SECRET to be set at
startup, fail fast with a clear error if it is missing or empty, and remove the
random default so the same signing key is used consistently by the
receipt-signing and verification flow.
- Around line 1607-1634: The erasure logic in erase_matching and the related
count path are still filtering USER_ID/TENANT_ID through agent_id, which does
not match the stored keys on SessionRecord and EventRecord. Update the
Repository queries in these branches to use tenant_id for TENANT_ID and the
correct stored key for USER_ID, and make count_matching and erase_matching use
the same scope-to-column mapping so deletions and counts stay consistent even
when events exist without a session row.
In `@agentwatch/governance/gdpr.py`:
- Around line 114-130: The receipt signature in gdpr.py does not actually
include any timestamp even though the docstring says it does. Update the
documentation on the signature helper that builds the HMAC payload to match the
real signed fields, or, if you want tamper-evident timing, add the relevant
receipt timestamp field(s) to the payload in the signing logic while preserving
the behavior expected by test_receipt_signature_is_deterministic. Reference the
payload construction inside the erasure receipt signature function and keep the
signed data consistent with the receipt model.
- Around line 194-200: The constructor in GDPR-related receipt handling is
falling back to a constant zero-filled signing key when signing_secret is empty,
which makes audit signatures forgeable. Update __init__ in the receipt/signing
flow to reject an empty signing_secret instead of substituting b"\x00" * 32, and
fail fast with a clear error so callers must provide a real secret before any
signature generation.
In `@agentwatch/reasoning/auditor.py`:
- Around line 163-171: The reason selection in the auditor logic is using
distance == 0.0 as a proxy for missing planner output, which mislabels identical
planner styles as insufficient_planner_signal. Update the reason derivation near
session_id/reason so it checks actual planner-output presence from the already
sliced first and second halves instead of inferring from distance. Keep
insufficient_planner_signal only when one side truly lacks planner signal, and
let identical-style zero-distance cases fall through to below_threshold.
In `@tests/test_reasoning_style_fingerprint.py`:
- Around line 139-146: The 5-event parametrized case in
test_reasoning_style_fingerprint is misleading because fingerprint_session
short-circuits to insufficient_events before any below_threshold logic can
apply. Update the test around fingerprint_session to either assert the actual
reason explicitly for the 5-event input or change/remove the comment so it
matches the real behavior, and keep the events/expected_reason table aligned
with the intended branch coverage.
---
Nitpick comments:
In `@agentwatch/api/server.py`:
- Around line 1628-1631: The per-session delete loop in the agent/user/tenant
erase path is doing N+1 deletes, so update the prune flow to batch work instead
of deleting each session one by one. In the logic around get_session_ids, first
collect all session IDs, then delete events for those sessions in bulk, and
finally call prune_sessions with the full list rather than delete_session inside
the loop. Use the existing repo methods get_session_ids and prune_sessions to
keep the change localized and reduce round-trips.
In `@agentwatch/governance/gdpr.py`:
- Around line 213-220: `total_start_opt` in the erasure flow is computed in the
target loop but never used, so the aggregation is dead. Update the logic in the
governance erasure method that iterates over `self._targets` to either remove
the unused pre-erasure count accumulation entirely or thread it through the
returned `ErasureReceipt`/results as a new field if it is meant to be reported.
Make sure the chosen fix is reflected consistently in the receipt-building path
and any related return values so `count_matching` is not called without purpose.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 261fb12f-6fe3-475f-8a13-05369c2236b8
📒 Files selected for processing (8)
CHANGELOG.mdagentwatch/api/server.pyagentwatch/core/models.pyagentwatch/core/schema.pyagentwatch/governance/gdpr.pyagentwatch/reasoning/auditor.pytests/test_gdpr_erasure.pytests/test_reasoning_style_fingerprint.py
| _SESSION_ERASURE_SECRET: bytes = os.getenv("AGENTWATCH_ERASURE_SECRET", "").encode() or os.urandom( | ||
| 32 | ||
| ) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Randomly generated signing secret breaks receipt verifiability.
When AGENTWATCH_ERASURE_SECRET is unset, a fresh os.urandom(32) key is generated at import. Signatures then change on every process restart and differ across horizontally-scaled instances, so previously issued compliance receipts can never be re-verified—undermining the immutable audit-trail goal. Require the secret to be configured (fail fast if missing) rather than defaulting to an ephemeral key.
🔒 Proposed fix
-_SESSION_ERASURE_SECRET: bytes = os.getenv("AGENTWATCH_ERASURE_SECRET", "").encode() or os.urandom(
- 32
-)
+_erasure_secret = os.getenv("AGENTWATCH_ERASURE_SECRET", "")
+if not _erasure_secret:
+ raise RuntimeError(
+ "AGENTWATCH_ERASURE_SECRET must be set to sign GDPR erasure receipts"
+ )
+_SESSION_ERASURE_SECRET: bytes = _erasure_secret.encode()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| _SESSION_ERASURE_SECRET: bytes = os.getenv("AGENTWATCH_ERASURE_SECRET", "").encode() or os.urandom( | |
| 32 | |
| ) | |
| _erasure_secret = os.getenv("AGENTWATCH_ERASURE_SECRET", "") | |
| if not _erasure_secret: | |
| raise RuntimeError( | |
| "AGENTWATCH_ERASURE_SECRET must be set to sign GDPR erasure receipts" | |
| ) | |
| _SESSION_ERASURE_SECRET: bytes = _erasure_secret.encode() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/api/server.py` around lines 1587 - 1589, The session erasure
signing secret is currently falling back to an ephemeral os.urandom value in the
module-level _SESSION_ERASURE_SECRET initialization, which makes receipt
signatures non-deterministic across restarts and instances. Update the secret
handling in agentwatch/api/server.py to require AGENTWATCH_ERASURE_SECRET to be
set at startup, fail fast with a clear error if it is missing or empty, and
remove the random default so the same signing key is used consistently by the
receipt-signing and verification flow.
| if scope == ErasureScope.SESSION_ID: | ||
| return await repo.count_sessions({"session_id": identifier}) | ||
| if scope == ErasureScope.AGENT_ID: | ||
| return await repo.count_sessions({"agent_id": identifier}) | ||
| # user_id / tenant_id / fallback: scan both tables on agent_id | ||
| sessions = await repo.count_sessions({"agent_id": identifier}) | ||
| events = await repo.count_events({"agent_id": identifier}) | ||
| return sessions + events | ||
|
|
||
| async def erase_matching(self, identifier: str, scope: ErasureScope) -> int: | ||
| if _db_session_factory is None: | ||
| return 0 | ||
| async with _db_session_factory() as db: | ||
| repo = Repository(db) | ||
| erased = 0 | ||
| if scope == ErasureScope.SESSION_ID: | ||
| erased += await repo.delete_events_by_session(identifier) | ||
| erased += await repo.delete_session(identifier) | ||
| await db.commit() | ||
| return erased | ||
| if scope in (ErasureScope.AGENT_ID, ErasureScope.USER_ID, ErasureScope.TENANT_ID): | ||
| session_ids = await repo.get_session_ids({"agent_id": identifier}) | ||
| for sid in session_ids: | ||
| erased += await repo.delete_events_by_session(sid) | ||
| erased += await repo.delete_session(sid) | ||
| await db.commit() | ||
| return erased | ||
| return 0 |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify SessionRecord/EventRecord columns to confirm correct scope→column mapping
ast-grep outline agentwatch/core/models.py --items all
rg -nP '\b(agent_id|user_id|tenant_id)\b' -C1 agentwatch/core/models.pyRepository: sreerevanth/AgentWatch
Length of output: 6533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the server-side erasure logic and related scope definitions/callers
ast-grep outline agentwatch/api/server.py --items all
printf '\n--- scope/erasure matches ---\n'
rg -n "ErasureScope|count_matching|erase_matching|get_session_ids|count_sessions|count_events|delete_events_by_session|delete_session" agentwatch/api/server.py -C 3
printf '\n--- repository lookup helpers ---\n'
rg -n "def (get_session_ids|count_sessions|count_events|delete_events_by_session|delete_session)" agentwatch/core/models.py -C 20
printf '\n--- tenant/user erasure references ---\n'
rg -n "user_id|tenant_id|erasure|right to erasure|delete.*tenant|delete.*user" agentwatch -C 2Repository: sreerevanth/AgentWatch
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read the erasure scope definition and the surrounding server logic
rg -n "class ErasureScope|Enum|USER_ID|TENANT_ID|AGENT_ID|SESSION_ID" agentwatch -C 2
printf '\n--- server.py around erase/count_matching ---\n'
sed -n '1560,1655p' agentwatch/api/server.py
printf '\n--- repository helper implementations ---\n'
sed -n '330,410p' agentwatch/core/models.pyRepository: sreerevanth/AgentWatch
Length of output: 26159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Inspect the erasure service contract and target protocol to determine expected scope behavior
sed -n '1,220p' agentwatch/governance/gdpr.py
printf '\n--- cross-session erasure references ---\n'
rg -n "class CrossSessionErasureService|class ErasureTarget|Protocol|count_matching|erase_matching|ErasureRequest|ErasureReceipt" agentwatch/governance/gdpr.py -C 4Repository: sreerevanth/AgentWatch
Length of output: 12034
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check whether event/session records are always linked by the same agent_id and session_id
sed -n '1,220p' agentwatch/core/schema.py
printf '\n--- repository inserts/lookups ---\n'
sed -n '220,330p' agentwatch/core/models.py
printf '\n--- server write paths ---\n'
sed -n '150,260p' agentwatch/api/server.py
printf '\n--- search for user_id storage in persistence models ---\n'
rg -n "\buser_id\b" agentwatch/core agentwatch/api agentwatch/models -C 2Repository: sreerevanth/AgentWatch
Length of output: 18965
Map erasure scopes to the stored tenant/session keys
USER_ID/TENANT_ID still route through agent_id here, even though SessionRecord and EventRecord both persist tenant_id and there is no user_id column in the storage models. Tenant-scoped erasure can miss the intended rows, and the count path can drift from the delete path when events exist without a matching session row.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/api/server.py` around lines 1607 - 1634, The erasure logic in
erase_matching and the related count path are still filtering USER_ID/TENANT_ID
through agent_id, which does not match the stored keys on SessionRecord and
EventRecord. Update the Repository queries in these branches to use tenant_id
for TENANT_ID and the correct stored key for USER_ID, and make count_matching
and erase_matching use the same scope-to-column mapping so deletions and counts
stay consistent even when events exist without a session row.
| @app.post("/api/v1/gdpr/erase", response_model=ErasureReceipt) | ||
| async def gdpr_erase( | ||
| request: ErasureRequest, | ||
| _auth: None = Depends(_require_api_key), | ||
| ) -> ErasureReceipt: | ||
| """Execute a right-to-erasure (right-to-be-forgotten) action across all | ||
| session, event, and memory data for the given identifier. | ||
|
|
||
| The response is an HMAC-SHA256 signed erasure receipt suitable for | ||
| compliance audit trails. | ||
| """ | ||
| service = await _build_erasure_service() | ||
| receipt = await service.erase(request) | ||
| return receipt |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
Endpoint reports success even when erasure fails.
CrossSessionErasureService.erase converts per-target exceptions into ErasureTargetResult(error=...) and still returns a signed receipt, but ErasureReceipt exposes only items_erased—no error/per-target detail. A DB failure therefore yields a 200 with items_erased=0 and a valid signature, and the caller cannot distinguish "nothing to erase" from "erasure failed." For a compliance action this should surface failures (non-2xx or an explicit failed-targets field).
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/api/server.py` around lines 1644 - 1657, The gdpr_erase endpoint
currently returns a successful ErasureReceipt even when
CrossSessionErasureService.erase records per-target errors, so the caller cannot
tell failure from a no-op. Update gdpr_erase and the ErasureReceipt/erasure flow
to surface failures explicitly: either raise an error and return a non-2xx
response when any target result has an error, or add a failure field that
includes per-target details and ensure the response schema reflects it. Use the
existing gdpr_erase, CrossSessionErasureService.erase, ErasureTargetResult, and
ErasureReceipt symbols to wire the failure state through the API.
| """Generate an HMAC-SHA256 erasure receipt signature. | ||
|
|
||
| The signature covers the request identity, the per-target erased counts, | ||
| and the completion timestamp, ensuring the receipt cannot be forged. | ||
| """ | ||
| total_erased = sum(r.erased_items for r in results) | ||
| payload = json.dumps( | ||
| { | ||
| "identifier": request.identifier, | ||
| "scope": request.scope.value, | ||
| "total_erased": total_erased, | ||
| "results": [(r.target_name, r.erased_items, r.error) for r in results], | ||
| }, | ||
| sort_keys=True, | ||
| separators=(",", ":"), | ||
| ) | ||
| return hmac.new(secret, payload.encode(), hashlib.sha256).hexdigest() |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Signature does not cover timestamps despite the docstring.
The docstring states the signature covers "the completion timestamp," but payload only includes identifier, scope, total_erased, and per-target results. As a result submitted_at/completed_at in the receipt are not tamper-evident. Either fold a timestamp into the signed payload (note this would break the determinism relied on by test_receipt_signature_is_deterministic) or correct the docstring to reflect what is actually signed.
🧰 Tools
🪛 ast-grep (0.44.1)
[info] 119-128: use jsonify instead of json.dumps for JSON output
Context: json.dumps(
{
"identifier": request.identifier,
"scope": request.scope.value,
"total_erased": total_erased,
"results": [(r.target_name, r.erased_items, r.error) for r in results],
},
sort_keys=True,
separators=(",", ":"),
)
Note: [CWE-116] Improper Encoding or Escaping of Output.
(use-jsonify)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/governance/gdpr.py` around lines 114 - 130, The receipt signature
in gdpr.py does not actually include any timestamp even though the docstring
says it does. Update the documentation on the signature helper that builds the
HMAC payload to match the real signed fields, or, if you want tamper-evident
timing, add the relevant receipt timestamp field(s) to the payload in the
signing logic while preserving the behavior expected by
test_receipt_signature_is_deterministic. Reference the payload construction
inside the erasure receipt signature function and keep the signed data
consistent with the receipt model.
| def __init__( | ||
| self, | ||
| targets: Iterable[ErasureTarget], | ||
| signing_secret: bytes = b"", | ||
| ): | ||
| self._targets = list(targets) | ||
| self._secret = signing_secret or b"\x00" * 32 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
Zero-byte fallback signing secret makes receipts forgeable.
When signing_secret is empty, the service silently signs with a constant, publicly-known key (b"\x00" * 32). Any party can then forge a valid audit_signature, defeating the "cryptographically signed, immutable receipt" guarantee. Prefer failing fast over a predictable key.
🔒 Proposed fix
def __init__(
self,
targets: Iterable[ErasureTarget],
- signing_secret: bytes = b"",
+ signing_secret: bytes,
):
+ if len(signing_secret) < 32:
+ raise ValueError("signing_secret must be at least 32 bytes")
self._targets = list(targets)
- self._secret = signing_secret or b"\x00" * 32
+ self._secret = signing_secret📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| def __init__( | |
| self, | |
| targets: Iterable[ErasureTarget], | |
| signing_secret: bytes = b"", | |
| ): | |
| self._targets = list(targets) | |
| self._secret = signing_secret or b"\x00" * 32 | |
| def __init__( | |
| self, | |
| targets: Iterable[ErasureTarget], | |
| signing_secret: bytes, | |
| ): | |
| if len(signing_secret) < 32: | |
| raise ValueError("signing_secret must be at least 32 bytes") | |
| self._targets = list(targets) | |
| self._secret = signing_secret |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/governance/gdpr.py` around lines 194 - 200, The constructor in
GDPR-related receipt handling is falling back to a constant zero-filled signing
key when signing_secret is empty, which makes audit signatures forgeable. Update
__init__ in the receipt/signing flow to reject an empty signing_secret instead
of substituting b"\x00" * 32, and fail fast with a clear error so callers must
provide a real secret before any signature generation.
| session_id = events[0].session_id if events else "" | ||
| reason: str | None = None | ||
| if not detected: | ||
| if len(events) < 6: | ||
| reason = "insufficient_events" | ||
| elif distance == 0.0: | ||
| reason = "insufficient_planner_signal" | ||
| else: | ||
| reason = "below_threshold" |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
reason derivation mislabels "identical style" as "insufficient_planner_signal".
elif distance == 0.0 is used as a proxy for "one half had no planner output," but distance can legitimately be exactly 0.0 when both halves do have planner output with an identical computed style (same token count/diversity/punctuation stats). In that case the reported reason ("insufficient_planner_signal") is factually wrong — there was planner signal, it just didn't differ.
This is exercised by tests/test_reasoning_style_fingerprint.py::test_fingerprint_session_no_swap_detected_consistent_plans, which uses identical plan text for all 8 events (guaranteeing distance 0.0 with real planner signal in both halves) and has to accept either "insufficient_planner_signal" or "below_threshold" to pass — a symptom of this ambiguity rather than a real guarantee of correct behavior.
Since first/second are already sliced above, derive the reason directly from actual planner-output presence instead of inferring it from the distance value.
🐛 Proposed fix
session_id = events[0].session_id if events else ""
+ has_first_planner = any(e.event_type == EventType.PLANNER_OUTPUT for e in first)
+ has_second_planner = any(e.event_type == EventType.PLANNER_OUTPUT for e in second)
reason: str | None = None
if not detected:
if len(events) < 6:
reason = "insufficient_events"
- elif distance == 0.0:
+ elif not has_first_planner or not has_second_planner:
reason = "insufficient_planner_signal"
else:
reason = "below_threshold"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| session_id = events[0].session_id if events else "" | |
| reason: str | None = None | |
| if not detected: | |
| if len(events) < 6: | |
| reason = "insufficient_events" | |
| elif distance == 0.0: | |
| reason = "insufficient_planner_signal" | |
| else: | |
| reason = "below_threshold" | |
| session_id = events[0].session_id if events else "" | |
| has_first_planner = any(e.event_type == EventType.PLANNER_OUTPUT for e in first) | |
| has_second_planner = any(e.event_type == EventType.PLANNER_OUTPUT for e in second) | |
| reason: str | None = None | |
| if not detected: | |
| if len(events) < 6: | |
| reason = "insufficient_events" | |
| elif not has_first_planner or not has_second_planner: | |
| reason = "insufficient_planner_signal" | |
| else: | |
| reason = "below_threshold" |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@agentwatch/reasoning/auditor.py` around lines 163 - 171, The reason selection
in the auditor logic is using distance == 0.0 as a proxy for missing planner
output, which mislabels identical planner styles as insufficient_planner_signal.
Update the reason derivation near session_id/reason so it checks actual
planner-output presence from the already sliced first and second halves instead
of inferring from distance. Keep insufficient_planner_signal only when one side
truly lacks planner signal, and let identical-style zero-distance cases fall
through to below_threshold.
| @pytest.mark.parametrize( | ||
| "events, expected_reason", | ||
| [ | ||
| ([], "insufficient_events"), | ||
| ([_plan("x", 0)], "insufficient_events"), | ||
| ([_plan("plan", i) for i in range(5)], None), # below threshold in even dist | ||
| ], | ||
| ) |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Comment doesn't match actual behavior for the 5-event case.
The comment # below threshold in even dist implies the expected reason is "below_threshold", but with 5 events (< 6), fingerprint_session will short-circuit to reason = "insufficient_events" per the len(events) < 6 check. Since expected_reason is None here, the test only asserts detected is False and doesn't actually pin down which reason is produced, so the mismatch goes unnoticed.
Consider asserting the actual reason explicitly (or fixing the comment) to keep this case meaningful.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_reasoning_style_fingerprint.py` around lines 139 - 146, The
5-event parametrized case in test_reasoning_style_fingerprint is misleading
because fingerprint_session short-circuits to insufficient_events before any
below_threshold logic can apply. Update the test around fingerprint_session to
either assert the actual reason explicitly for the 5-event input or
change/remove the comment so it matches the real behavior, and keep the
events/expected_reason table aligned with the intended branch coverage.
|
Closes #365 |
…h#339) (sreerevanth#557) * feat(cli): add cost report command for spend by framework (sreerevanth#339) * fix(cli): warn on truncated results, skip malformed sessions, dedup group_by validation (sreerevanth#339)
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
Thanks for the substantial contribution—this adds a valuable GDPR erasure workflow with good test coverage. Before merging, I'd like to address a few issues:
Require a configured signing secret instead of falling back to a random/constant key.
Correct the USER_ID/TENANT_ID mapping so erasure targets the proper stored fields.
Surface target failures through the API instead of always returning a successful receipt.
Once those are addressed, I'd be happy to take another look.
|
Hey @sreerevanth, this PR has been ready for review for a while -- offering this friendly bump. The implementation matches the issue's specified behavior and CI is passing. PR: #567 When you have a moment, a review would be much appreciated. I am happy to iterate on any feedback. If the timing is not right, no worries at all -- just a gentle nudge from a contributor who has been waiting patiently. � |
sreerevanth
left a comment
sreerevanth
left a comment
There was a problem hiding this comment.
Thanks for the substantial contribution—this adds valuable GDPR erasure functionality and reasoning fingerprinting, and the accompanying tests are appreciated.
Before this can be merged, there are a few blockers that need to be addressed:
The receipt signing secret must not fall back to a random or zero-value key. Receipts need to remain verifiable across restarts and deployments, so the service should fail fast if no signing secret is configured.
The GDPR endpoint currently returns a successful signed receipt even if one or more erasure targets fail. Compliance operations should surface failures explicitly rather than returning HTTP 200 with items_erased=0.
The tenant/user scope mapping should use the actual persisted identifiers instead of routing through agent_id, otherwise tenant-scoped erasure may miss records.
The reasoning fingerprint logic should distinguish between identical fingerprints and missing planner signal instead of treating distance == 0 as insufficient planner output.
Please also update the receipt signing documentation so it accurately reflects what is included in the HMAC payload.
Once these issues are resolved, I'd be happy to review it again.
3 participants
Summary
Closes #365 — implements cross-session GDPR erasure (right-to-be-forgotten) with HMAC-signed compliance receipts, an
ErasureTargetprotocol for pluggable storage backends, and aPOST /api/v1/gdpr/eraseREST endpoint.Changes
agentwatch/core/models.pyget_session_ids,count_sessions,count_events,delete_session,delete_events_by_session. Used by the SQLAlchemy erasure target.agentwatch/core/schema.pyEventType.STYLE_FINGERPRINT_COMPUTEDandEventType.STYLE_SWAP_DETECTED.ReasoningStyleFingerprintandStyleSwapAlertmodels.agentwatch/reasoning/auditor.pyFingerprintReportdataclass andReasoningAuditor.fingerprint_session()method.agentwatch/governance/gdpr.pyErasureScopeenum (user/agent/session/tenant).ErasureRequestandErasureTargetResultdataclasses.ErasureTargetprotocol for pluggable backends.CrossSessionErasureService— orchestrator that counts, erases, and signs a receipt with HMAC-SHA256.typing.Iterabletocollections.abc.Iterable.agentwatch/api/server.py_SessionErasureTarget— SQLAlchemy-backedErasureTargetusing the existingRepository.POST /api/v1/gdpr/erase— acceptsErasureRequestJSON, returnsErasureReceipt.Tests (
tests/test_gdpr_erasure.py)Checklist
ruff checkandruff format --checkare cleanSummary by CodeRabbit
New Features
Documentation
Tests