Repository navigation
feat(apply): implement deterministic reconciliation - #62
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThe PR implements deterministic, durable Apply processing with ordered child dispatch, authenticated local records, Google quota preference writes, unknown-outcome reconciliation, plan invalidation and recovery, audit deduplication, expanded validation, and crash-safety coverage. ChangesApply lifecycle
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
Comment |
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
There was a problem hiding this comment.
CodeQL found more than 20 potential problems in the proposed changes. Check the Files changed tab for more details.
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
| Filename | Overview |
|---|---|
| src/cqmgr/application/operations/apply.py | Adds the ordered Apply state machine, durable recovery paths, terminal projection, and fail-closed reconciliation. |
| src/cqmgr/adapters/persistence/apply_records.py | Adds authenticated, atomic Apply-record and unknown-resolution persistence. |
| src/cqmgr/adapters/persistence/plans.py | Extends plan persistence with recoverable dispatch and consumption transitions. |
| src/cqmgr/adapters/google/quota_preference_writes.py | Adds deterministic Cloud Quotas writes and read-after-unknown reconciliation with generic retries disabled. |
| src/cqmgr/domain/apply_records.py | Defines Apply record state, child dispositions, transition invariants, and terminalization behavior. |
| tests/application/test_apply_operations.py | Exercises Apply ordering, persistence failures, restart recovery, and crash boundaries. |
Flowchart
%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Load immutable plan] --> B[Revalidate complete bundle]
B --> C[Acquire plan lease]
C --> D[Persist Apply intent]
D --> E[Mark plan dispatched]
E --> F[Dispatch children in order]
F --> G{Child outcome}
G -->|Accepted| F
G -->|Failed| H[Mark remaining children unattempted]
G -->|Unknown| I[Persist uncertainty and reconcile]
F -->|All accepted| J[Finalize accepted record]
H --> K[Finalize failed record]
I --> L[Finalize or retain unknown state]
J --> M[Consume plan]
K --> M
M --> N[Project durable terminal result on retry]
Reviews (3): Last reviewed commit: "fix(apply/recovery): return critical aud..." | Re-trigger Greptile
3023ba1 to
a0b913a
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/cqmgr/adapters/persistence/plans.py (1)
1000-1006: 🩺 Stability & Availability | 🟡 Minor | ⚡ Quick winRestore the
parent == candidateguard in_ensure_private_directory.runtime_paths()still allows relativeexpanduser()results, so a missing working directory can leavePath(".")non-existent and the loop spinning indefinitely.🤖 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 `@src/cqmgr/adapters/persistence/plans.py` around lines 1000 - 1006, Restore the parent-equality termination guard in _ensure_private_directory’s loop so traversal stops when candidate.parent is candidate, including for relative paths whose working directory is missing. Preserve the existing missing-path collection and private mkdir behavior.
🤖 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 `@tests/domain/test_apply_records.py`:
- Around line 85-89: Update all three type-check suppressions on the replace
calls in the parameterized test to use mypy’s actual arg-type error code instead
of bad-argument-type, or remove them if those calls no longer produce errors.
Keep the test behavior and replace invocation unchanged.
---
Outside diff comments:
In `@src/cqmgr/adapters/persistence/plans.py`:
- Around line 1000-1006: Restore the parent-equality termination guard in
_ensure_private_directory’s loop so traversal stops when candidate.parent is
candidate, including for relative paths whose working directory is missing.
Preserve the existing missing-path collection and private mkdir behavior.
🪄 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: ASSERTIVE
Plan: Pro Plus
Run ID: 7ef35569-21bc-4e7d-ab47-233685f46bb9
📒 Files selected for processing (24)
.github/workflows/python.ymlscripts/verify_distribution.pysrc/cqmgr/adapters/google/quota_preference_writes.pysrc/cqmgr/adapters/persistence/apply_records.pysrc/cqmgr/adapters/persistence/audit.pysrc/cqmgr/adapters/persistence/plans.pysrc/cqmgr/application/operations/apply.pysrc/cqmgr/application/operations/plans.pysrc/cqmgr/application/ports/apply.pysrc/cqmgr/application/ports/apply_records.pysrc/cqmgr/application/ports/audit.pysrc/cqmgr/application/ports/plans.pysrc/cqmgr/application/ports/provider_writes.pysrc/cqmgr/domain/apply_records.pysrc/cqmgr/domain/audit.pysrc/cqmgr/domain/plan_consumption.pysrc/cqmgr/domain/plans.pytests/adapters/test_google_quota_preference_writes.pytests/application/test_apply_operations.pytests/domain/test_apply_records.pytests/plans/test_apply_record_repository.pytests/plans/test_plan_codec.pytests/plans/test_plan_consumption.pytests/plans/test_plan_repository.py
src/cqmgr/adapters/google/quota_preference_writes.pysrc/cqmgr/adapters/persistence/apply_records.pysrc/cqmgr/adapters/persistence/audit.pysrc/cqmgr/adapters/persistence/plans.pysrc/cqmgr/application/operations/apply.pysrc/cqmgr/application/operations/plans.pysrc/cqmgr/application/ports/apply.pysrc/cqmgr/application/ports/apply_records.pysrc/cqmgr/application/ports/audit.pysrc/cqmgr/application/ports/plans.pysrc/cqmgr/application/ports/provider_writes.pysrc/cqmgr/domain/apply_records.pysrc/cqmgr/domain/audit.pysrc/cqmgr/domain/plan_consumption.pysrc/cqmgr/domain/plans.py.github/workflows/python.ymlscripts/verify_distribution.pytests/adapters/test_google_quota_preference_writes.pytests/application/test_apply_operations.pytests/domain/test_apply_records.pytests/plans/test_apply_record_repository.pytests/plans/test_plan_codec.pytests/plans/test_plan_consumption.pytests/plans/test_plan_repository.pySummary
Review path
src/cqmgr/application/operations/apply.py.src/cqmgr/application/ports/apply.py,src/cqmgr/domain/apply_records.py, andsrc/cqmgr/adapters/persistence/apply_records.py.src/cqmgr/adapters/persistence/plans.py,src/cqmgr/domain/plan_consumption.py, and the related ports.src/cqmgr/adapters/google/quota_preference_writes.py, then the adversarial crash/retry tests.Changes
NotFoundand nonmatching observations unresolved until rejection is proven, while retaining exact reconciliation identity even when containment persistence fails.retry=None,validate_only=False,allow_missing=False, current etag use, input-only contact semantics, and no generic retry loop.Safety
validateOnlycall was performed.Verification
uv run --locked ruff check .— passed.uv run --locked ruff format --check .— passed.uv run --locked pyrefly check— 0 errors.uv run --locked lint-imports— 5 contracts kept.uv run --locked pytest -q— 1,363 passed, 3 skipped; 91.78% branch coverage.uv build --clear --no-sourcesand distribution verification — passed.git diff --check origin/main..HEAD— passed.Closes #38