Fix purge_expired_grants to preserve audit trail before deleting grants#261
Conversation
Before deleting expired grants, null out grant_id on any AuditLog rows that reference those grants so the forensic audit trail is preserved as soft references (null grant_id). This prevents orphaned foreign-key references in the audit log. Fixes #250 Signed-off-by: Saffron <263493777+itsmiso-ai@users.noreply.github.com>
There was a problem hiding this comment.
AI Automated Review
Full PR review.
Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) — escalated (fast_low_confidence)
Recommendation: Approve
This PR correctly implements the fix for issue PR 250 by nullifying grant_id on AuditLog rows before hard-deleting expired grants, preserving the forensic audit trail as soft-references.
Change-by-Change Findings
kubetix-api/kubetix_api/cleanup.py
Before: purge_expired_grants() ran a direct DELETE on Grant rows filtered by expires_at < now, leaving AuditLog rows with dangling grant_id references.
After: The function now:
- Queries and collects expired grant IDs first
- Nulls out
grant_idon anyAuditLogrows referencing those IDs viaupdate({AuditLog.grant_id: None}) - Proceeds with the original
DELETE
This is the acceptance-criteria option (a) from issue PR 250: "audit log entries for deleted grants are preserved as soft-references (null grant_id)." The implementation is correct and minimal.
kubetix-api/tests/test_cleanup.py
test_purge_deletes_expired_grants_and_commits: Updated fromassert_called_once()→assert_called()to accommodate the new AuditLog query call in the call chain.test_purge_returns_zero_when_no_expired_grants: Fixed — the early-return path (if not expired_grants: return 0) correctly skips both theupdate()andcommit()call. The old assertionsession.commit.assert_called_once()was wrong for the no-op path and would have failed with the early-return logic.test_purge_nulls_audit_log_grant_id_before_delete: New regression test verifying AuditLog is queried and updated with{grant_id: None}before grants are deleted.
tests/unit/test_cleanup.py
test_purge_expired_grants_removes_only_expired: UpdatedFakeSessionandFakeQueryto support the new.all()call (to collect expired grant IDs) and the.update()call (to null AuditLog references), while keeping the original deletion behavior intact.test_purge_expired_grants_preserves_audit_trail: New end-to-end unit test withFakeGrant,FakeAuditLog, andFakeSessionthat verifies afterpurge_expired_grants():AuditLog.grant_idis nulled for expired grantsGrantrows for expired grants are deleted- Non-expired grants and audit entries remain intact
Standards Compliance
- Audit logging:
AuditLogtable in API models is correctly preserved per the project's audit trail requirements (AGENTS.md §"audit log of all access", §"audit log entry for every state-changing action"). - Database models: Uses existing
GrantandAuditLogSQLAlchemy models — no new models introduced. - Tests: Follow the existing pattern of
FakeSession/FakeQuerymocking used throughouttests/unit/test_cleanup.py.
Linked Issue Fit
Issue PR 250 acceptance criteria: "Either (a) audit log entries for deleted grants are preserved as soft-references (null grant_id), or (b) expired grants are soft-deleted instead of hard-deleted."
This PR implements option (a) exactly as described. The evidence cited in the issue (cleanup.py:44-47) is fully addressed: the hard delete still happens, but only after AuditLog references are nulled. The forensic record (who had access, when, what action) is preserved in AuditLog rows with grant_id = NULL.
Tool Harness Findings
No tool harness output in this corpus; reviewing the delta diff directly.
CI Check Results
All 9 CI checks passed (e2e-tests, security, legacy-test, test, lint, directory-check, helm-check, frontend-build, build-and-publish). The test and legacy-test checks cover the two test files modified by this PR.
Unknowns / Needs Verification
None.
What
Before deleting expired grants in
purge_expired_grants(), null outgrant_idon anyAuditLogrows that reference those expired grants, preserving the forensic audit trail as soft-references.Why
purge_expired_grants()was hard-deleting expired grants witho…Fixes #250
Opened by foreman on review GO (workload wl-misospace-kubetix-250).