feat(forensics): support epoch backfill lifecycle rows - #339
Conversation
Sync the canonical MDK v2 audit schema and cover all five epoch-stall backfill lifecycle rows through ingestion and agent-state export. Fixes #338
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (1)
WalkthroughThe audit schema adds operational and epoch-stall backfill events. Fixtures and tests validate peeler outcomes and lifecycle ingestion, group association, field preservation, export shape, and operation ID normalization. ChangesAudit lifecycle support
Estimated code review effort: 3 (Moderate) | ~25 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
forensics/tests.py (2)
5037-5046: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAssert the exported key set to cover the "no additional identifying data" objective.
The loop checks four named keys. It does not fail if the export adds a key such as an account pubkey or a device label. Comparing the full key set turns that objective into an enforced assertion.
🛡️ Proposed addition
for event_type, expected in expected_by_type.items(): with self.subTest(event_type=event_type): exported = exported_by_type[event_type] + self.assertEqual( + set(exported), + {"event_type", "context", "kind", "group_ref", "normalized"}, + ) self.assertEqual(exported["context"], expected["context"])Adjust the expected key set to the full documented export contract if it contains more fields.
🤖 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 `@forensics/tests.py` around lines 5037 - 5046, Update the assertions in the expected_by_type iteration to compare the complete set of keys in each exported record against the documented export contract, rejecting unexpected identifying fields while including every legitimate documented field. Keep the existing value assertions for context, kind, group_ref, and normalized.
4983-4983: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAnchor the fixture path to the repository root.
If the test runner starts outside the repository root,
Path("fixtures/...")cannot find the committed fixture. UsePath(__file__).resolve().parent.parent / "fixtures" / "epoch-stall-backfill-lifecycle.jsonl".🤖 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 `@forensics/tests.py` at line 4983, Update the fixture_path assignment in the test to resolve the fixture from the repository root using __file__. Preserve the existing epoch-stall-backfill-lifecycle.jsonl fixture while constructing the path through the parent directories and the fixtures directory instead of the current working directory.docs/schemas/audit-log-event.v2.schema.json (1)
1704-1786: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueExtract the shared completed/failed payload into a
$def.The
epoch_stall_backfill_completedandepoch_stall_backfill_failedbranches declare the same eight required fields and the same property set. Only thetypeconst and the extraerror_kinddiffer. A shared$defkeeps the two branches from drifting when the producer adds a field.♻️ Sketch of the shared definition
+ "epochBackfillOutcomeFields": { + "retry_ordinal": { "$ref": "`#/`$defs/u64" }, + "duration_ms": { "$ref": "`#/`$defs/u64" }, + "activation_outcome": { "$ref": "`#/`$defs/epochBackfillActivationOutcome" }, + "deliveries": { "$ref": "`#/`$defs/u64" }, + "local_epoch_before": { "$ref": "`#/`$defs/u64" }, + "local_epoch_after": { "$ref": "`#/`$defs/u64" }, + "group_advanced": { "type": "boolean" } + },Then reference it from both branches with
"$ref": "#/$defs/epochBackfillOutcomeFields"insideproperties, keeping the per-branchtypeconst anderror_kind.🤖 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 `@docs/schemas/audit-log-event.v2.schema.json` around lines 1704 - 1786, Extract the shared fields and property definitions from the epoch_stall_backfill_completed and epoch_stall_backfill_failed schema branches into a new $defs.epochBackfillOutcomeFields definition. Replace the duplicated fields in each branch with a reference to that definition inside properties, while retaining each branch’s type const and the failed branch’s error_kind property.
🤖 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 `@docs/schemas/audit-log-event.v2.schema.json`:
- Around line 695-705: Update the ingestion outcome allowlist in
forensics/ingest.py to accept the schema-defined invalid_signature and
wrong_recipient values, then add both outcomes to the corresponding glossary and
ingestion tests. Preserve the existing generic non-success error-tone behavior.
---
Nitpick comments:
In `@docs/schemas/audit-log-event.v2.schema.json`:
- Around line 1704-1786: Extract the shared fields and property definitions from
the epoch_stall_backfill_completed and epoch_stall_backfill_failed schema
branches into a new $defs.epochBackfillOutcomeFields definition. Replace the
duplicated fields in each branch with a reference to that definition inside
properties, while retaining each branch’s type const and the failed branch’s
error_kind property.
In `@forensics/tests.py`:
- Around line 5037-5046: Update the assertions in the expected_by_type iteration
to compare the complete set of keys in each exported record against the
documented export contract, rejecting unexpected identifying fields while
including every legitimate documented field. Keep the existing value assertions
for context, kind, group_ref, and normalized.
- Line 4983: Update the fixture_path assignment in the test to resolve the
fixture from the repository root using __file__. Preserve the existing
epoch-stall-backfill-lifecycle.jsonl fixture while constructing the path through
the parent directories and the fixtures directory instead of the current working
directory.
🪄 Autofix
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: e2f7eecd-d600-474a-a38d-c44e6d2f888d
📒 Files selected for processing (3)
docs/schemas/audit-log-event.v2.schema.jsonfixtures/epoch-stall-backfill-lifecycle.jsonlforensics/tests.py
Adversarial review — changes requiredReviewed exact head Blocking (1)Canonical The copied schema adds both values to Impact: this PR makes Goggles' canonical validator advertise support for producer evidence that the upload path then quarantines and excludes from valid-event exports. This also violates the issue's existing-v2 compatibility requirement because the byte-for-byte schema sync necessarily brings these producer outcomes with it. Required fix: extend the ingestion allowlist to match the schema, add regression coverage proving both rows remain Verified
No additional suggestions or nitpicks from this pass. |
Align ingestion and documentation with the synced v2 schema, and add regression coverage for both newly supported outcomes and the lifecycle export contract.
|
Addressed review feedback in fb54c47: fixed the blocking peeler-outcome ingest mismatch, added valid-ingest regression coverage, updated the glossary, anchored the lifecycle fixture path to the repository, and asserted the complete documented export key set. The suggested schema deduplication was intentionally not applied: this file must remain byte-for-byte identical to the MDK producer schema, and it still matches producer head 773402d3 (SHA-256 8b082e70a41614ac72b6f61f96a912e95af2f8c24cd326a25081e80a07ebba54). Local just check passes 257 tests plus Django/Ruff/migration gates; dependency audit reports no known vulnerabilities. @coderabbitai review |
🧩 Analysis chain🏁 Script executed: #!/bin/bash
set -euo pipefail
printf '%s\n' '== PR changed files =='
git diff --name-status HEAD^ HEAD
printf '%s\n' '== Relevant symbol map =='
ast-grep outline forensics/ingest.py --items all --match 'normalize_kind|peeler' || true
ast-grep outline forensics/tests.py --items all --match 'Peeler|EpochStallBackfillLifecycleTests' || true
printf '%s\n' '== Ingestion and tests =='
rg -n -C 8 'invalid_signature|wrong_recipient|peeler_outcome|normalize_kind' \
forensics/ingest.py forensics/tests.py
printf '%s\n' '== Glossary references =='
rg -n -i -C 5 'peeler outcome|invalid_signature|wrong_recipient' \
docs README.md . -g '!forensics/ingest.py' -g '!forensics/tests.py' \
-g '!docs/schemas/audit-log-event.v2.schema.json' || true
printf '%s\n' '== Focused diff =='
git diff HEAD^ HEAD -- forensics/ingest.py forensics/tests.py docs README.mdLength of output: 19098
The repository-anchored fixture path and exact export-key assertion are also present. The schema deduplication should remain excluded because byte-for-byte producer-schema synchronization is a stated requirement. I did not execute the test suite in this review environment. Static inspection found no remaining issue in this fix. If you found this review helpful, would you consider giving us a shout-out on X? Thank you for using CodeRabbit!
|
Adversarial re-review — APPROVE / LGTMReviewed exact final head The prior blocking schema/ingest mismatch is resolved:
Blocking findings: 0. No new blocker, sensitive path, suggestion, or nitpick found in the final diff. |
1 participant
Summary
773402d3context.operation_id, while account-scoped rows remain free of a fabricated top-levelgroup_refVerification
just check— 256 tests passed; Django check, Ruff lint/format, and migration drift passedjust audit-dependencies— no known vulnerabilitiespython manage.py validate_audit_schema fixtures/*.jsonl— 39 events across 4 fixtures passedjust ci— frozen sync and all 256 SQLite tests passed; local Postgres leg could not start because this worker has no access to/var/run/docker.sock(GitHub CI will run it)Fixes #338
Summary by CodeRabbit
New Features
Documentation
Tests