docs(spec): add registry reconciliation contract#257
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
Warning Rate limit exceeded
You’ve run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (1)
✨ 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 |
|
Droid finished @EffortlessSteven's task —— View job Droid review completed for PR #257 (docs/reconcile-spec). Validation Summary: All 4 candidate comments validated and posted as a batched review.
No security issues identified. |
| - `cargo test -p shipper-cli --test bdd_publish` | ||
| - targeted resume tests for all three reconciliation outcomes | ||
| - policy/doc gates: | ||
| - `cargo xtask check-doc-contracts --mode advisory` |
There was a problem hiding this comment.
[P1] Test mapping references non-existent reconciliation tests in shipper-types
The Test Mapping section specifies cargo test -p shipper-types reconciliation as expected implementation proof, but shipper-types is a types/data-schema crate containing only snapshot/integration tests for schemas and release levels. The ReconciliationOutcome type is defined in shipper-types (with documentation referencing the reconciliation flow), but there are no reconciliation-specific tests in that crate — those tests reside in shipper-core. Running cargo test -p shipper-types reconciliation would find zero tests, making the spec's test mapping inaccurate. The spec should either remove this bullet or clarify that shipper-types holds only the type definitions while actual reconciliation tests live in shipper-core.
| # SHIPPER-SPEC-0003: Registry Reconciliation | ||
|
|
||
| Status: proposed | ||
| Owner: EffortlessMetrics | ||
| Created: 2026-05-13 |
There was a problem hiding this comment.
[P2] Empty linked-artifact header fields create ambiguity about acceptance criteria
The spec header contains several empty fields: Linked specs:, Linked ADRs:, Linked plan:, Linked PRs:. While the proposed status may intend these to be filled as the lane progresses, the Implementation Mapping section explicitly mentions an ADR that should record registry-truth-over-Cargo-output decision, creating an inconsistency — that ADR is referenced in prose but not linked in the header. The companion proposal SHIPPER-PROP-0002 has identical empty fields, suggesting this is the established pattern, but the spec should either populate the ADR link or clarify that the ADR is pending and tracked via open questions rather than a header link.
| happened from the operator. | ||
|
|
||
| Shipper must close ambiguous publish outcomes against registry evidence before | ||
| retrying or resuming. | ||
|
|
||
| ## Behavior Contract | ||
|
|
||
| When a cargo publish attempt exits with an ambiguous class, Shipper must: | ||
|
|
||
| 1. Persist that the package outcome is ambiguous before taking follow-up action. | ||
| 2. Emit a `publish_reconciling` event before querying registry truth. | ||
| 3. Query registry truth using bounded sparse-index and/or registry API evidence, | ||
| honoring the configured readiness method where applicable. | ||
| 4. Produce exactly one reconciliation outcome: | ||
| - `Published` | ||
| - `NotPublished` | ||
| - `StillUnknown` | ||
| 5. Emit a `publish_reconciled` event with the outcome and structured evidence. | ||
| 6. Persist the reconciliation outcome in execution state. | ||
| 7. Apply the outcome: | ||
| - `Published`: mark the package complete and do not re-upload. | ||
| - `NotPublished`: allow retry policy to continue. |
There was a problem hiding this comment.
[P3] Events-as-truth invariant not explicitly stated in Behavior Contract ordering
The Required Evidence section requires State and events remain consistent with the events-as-truth invariant, and publish_reconciling/publish_reconciled events are referenced, but the Behavior Contract steps (1-8) do not explicitly mandate that both events must be written to events.jsonl before the corresponding state update. The spec describes what actions must happen but not the critical ordering constraint (events first, state second) that the events-as-truth invariant enforces. Implementers reading only the Behavior Contract could miss this requirement. The spec should clarify that steps 2 and 5 (event emission) must complete and be flushed to events.jsonl before steps 6 and 7 (state persistence) execute.
| reconciliation outcome is `Published`. | ||
| - Cargo exits with a timeout, bounded registry checks complete and the version | ||
| remains absent: reconciliation outcome is `NotPublished`. | ||
| - Cargo exits with a network failure, and both registry evidence paths are | ||
| unavailable or inconclusive: reconciliation outcome is `StillUnknown`. | ||
| - Resume sees a persisted `Published` reconciliation outcome: the package is | ||
| skipped without another `cargo publish`. | ||
| - Resume sees a persisted `StillUnknown` reconciliation outcome: Shipper stops | ||
| for operator action instead of retrying. |
There was a problem hiding this comment.
[P3] StillUnknown acceptance example omits the halt-for-operator-action consequence
The Acceptance Examples section states that StillUnknown results when registry evidence paths are unavailable or inconclusive, but unlike the other examples it does not state the consequence: that Shipper must stop before blind retry and require operator action (which Behavior Contract step 7 explicitly requires). The other two outcome examples describe consequences (Published → skip upload; NotPublished → retry-eligible), but StillUnknown only describes the trigger condition. An implementer relying solely on the acceptance examples would not know that StillUnknown is a terminal state requiring manual intervention.
Summary:
Validation:
Non-goals: