feat(rev): persist immutable review packets (REV-03B) - #460
Conversation
|
Navigate logical layers of code changes, visualize relationships, and explore their blast radius. No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Advanced Run ID: 📒 Files selected for processing (12)
Included review availability: This review used your included allowance. Your plan provides up to 1 included review per hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe pull request adds immutable, normalized review packet storage tied to exact leases, queue entries, submissions, guide snapshots, and live guide ingests. It adds database validation and repository operations, updates tests and ownership records, and marks REV-03B delivered with REV-04A next. ChangesReview packet storage
Priority: ➖ Normal Estimated code review effort: 4 (Complex) | ~60 minutes Change: Feature Sequence Diagram(s)sequenceDiagram
participant ApplicationCaller
participant ReviewPacketRepository
participant PostgreSQL
ApplicationCaller->>ReviewPacketRepository: store lease and membership
ReviewPacketRepository->>PostgreSQL: lock lease and queue entry
PostgreSQL-->>ReviewPacketRepository: current lease and queue state
ReviewPacketRepository->>PostgreSQL: insert packet manifest and guide items
PostgreSQL-->>ReviewPacketRepository: validated packet records
ReviewPacketRepository-->>ApplicationCaller: stored packet without commit
Merge Risk: ⚪ Minimal · up to No actionable packet-storage issue remains identified; the change is mergeable after normal checks. Security Architecture ReviewSecurity architecture risk: 🔵 Low · up to The new review records have layered ownership, expiry and immutability checks, without activating a new user-facing access path. No concrete security regression was established, but deployment compatibility and correction-test results remain unverified. Retained concerns Security review detailsSecurity Blast Radius
Trust Boundaries and Controls
Resilience and Maintainability Implications
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Full details: Docstring CoverageExplanation Docstring coverage is 16.67% which is insufficient. The required threshold is 80.00%. Docstring coverage is scoped to functions touched by this diff. Analyzed 60 functions across 24 files. (3 skipped: 3 unsupported.)
✨ Finishing Touches 💡 1📝 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 |
2 participants
Change
REV-03B — immutable normalized reviewer packet persistence.
Goal
Freeze the exact Submission ZIP and complete guide document set for one review lease. This completes the packet-storage prerequisite before complete REV-04A Review storage and shared FinalAcceptance.
Intent And Planning Context
Change record defines scope, custody rules, named proof and the next boundary. The approved sequence still permits automated acceptance as the first complete runtime path; this PR activates no human-review workflow.
What Changed
packet_manifest_idname with no alias.guide_binding_idpacket field with liveingest_id. The old target is retained extraction evidence whose writes are sealed. No alias or revived path; no retained data deletion.Design Chosen
One header plus normalized guide members; the required ZIP is represented by non-null header fields. Existing canonical hashing and immutable-fact guards are reused. ORM references preserve owner representations; detached contracts use native UUIDs. Metadata grants no authority or artifact access.
Scope Control
Allowed implementation, proof and documentation paths are listed in the change record. No public route, resolver, provider I/O, claim activation, Review, acceptance effects, worker or CI-policy change. Identifier and lane inventories add only the new owned entries. This cohesive L1 schema change exceeds the usual size preference because migration custody, direct-SQL proof and current navigation belong together.
Product Behavior
Evidence
Current clean candidate:
49be77efe361823b08fe380fd97639d3ae634e5a, base7754703f.The human-review corrections enforce the lease deadline independently in the repository and PostgreSQL INSERT guard, preserve exact replay after expiry/closure, compute substituted digests from final altered fields, require named rejection boundaries, and exercise child insertion while its parent is uncommitted and later rolls back.
Focused real-PostgreSQL deadline, replay, owner-substitution and concurrency cases pass. Removing only the deadline or result-owner predicate makes the expected rejection assertion fail. The parent-visibility test proves PostgreSQL’s immediate FK denial in the second session while the first still owns the uncommitted parent, followed by rollback and no retained rows.
The complete packet/contract batch passed on clean
49be77ef: 52 tests, zero skips/deselections, including the existing migration-head and truncate regressions. All required hosted checks passed: 8,099 tests completed, zero skips or deselections. The tested merge tree exactly matches this candidate, and the downloaded lane evidence passed the canonical validator. Ruff, module boundaries, ownership, Commitrail, Markdown links and stale wording checks pass. The schema fingerprint was measured from the new migration; no validation was weakened.Acceptance Criteria Proof
The record's named test inventory covers canonical membership, direct-SQL rejection, database-owned time, lease generation, exact replay, rollback, concurrent writers/closure, migration preservation and historical lineage. Guard-removal probes must fail at the intended assertion, not fixture setup. Namespace substitution is rejected by ART's existing namespace FK before packet validation.
Test Delta
New PostgreSQL storage/repository/migration tests; existing ART contract tests replace the obsolete field and explicitly reject it. Identifier, schema-reset, lane and ownership inventories are extended. No tests removed or skipped; no coverage gate introduced or weakened.
Impact-Routed Reviewer Results
Security, QA/test-delta, architecture/reuse and documentation/product operations passed on exact
49be77ef. No findings remain. Security independently reran the deadline and altered-result owner cases: 2 passed. Final CI-integrity review passed with no active findings: exact-tree hosted completeness, evidence digests, all nine lane inventories, and PostgreSQL/MinIO cleanup were verified. The remaining low risk is timing headroom, described below.CodeRabbit completed a substantive review of
49be77ef, with no actionable findings and no unresolved threads. Its docstring-percentage advisory is not a repository gate.Risks And Human Review Focus
Verify canonical stored lineage and the absence of byte/claim authority. Live packet resolution, exact authorization, complete Review storage and both acceptance triggers remain later work. Local tests use real PostgreSQL and canonical owner fixtures; they do not claim live reviewer endpoints or new provider execution.
The final hosted run took 22m03s; its slowest lane used 1,144.9s of the unchanged 1,200s limit. Timing remains above the advisory target, with about 55 seconds of lane headroom. No timeout or gate was relaxed.
Roadmap impact is reflected in the same PR, including the diagram, remaining gates and current initiative navigation. No local spreadsheet exports are present.