docs: demote cargo stdout to hint; registry is authoritative (#99 follow-on)#117
Conversation
…low-on) Closes one of the remaining #99 follow-ons identified in the retrospectives: make it explicit, in the rustdoc that downstream tooling and contributors will actually read, that cargo's stdout/stderr output is a classification HINT for Shipper's retry loop — not the authoritative answer about what happened. ## What this changes - `ErrorClass` rustdoc (shipper-types): adds a "Classification is a hint, not truth" section explaining that pattern-matching cargo text drives retry scheduling but ambiguous outcomes MUST be reconciled against registry truth. Cross-refs ReconciliationOutcome and the reconcile module. - `classify_cargo_failure` rustdoc (shipper::runtime::execution): states up-front "This is a hint, not authoritative truth" and points at the reconcile flow for the authoritative resolution on Ambiguous outcomes. - `docs/explanation/why-shipper.md` gains a new section "Cargo stdout is a hint; the registry is the truth" making the same point at the product level — and connecting it to why Shipper can be safer than a naive shell-script `cargo publish` loop. ## Why this matters Contributors reading `ErrorClass` and wondering "can I just pattern-match more cargo strings?" now see the contract: cargo text is the fast-path hint, registry queries are truth. The reconcile flow introduced in #111 (in-flight) and #115 (resume-path) is now documented as the authoritative side of that contract, not just implemented. ## Scope Pure docs/rustdoc change. No code changes. No schema changes. No snapshot churn. Covered by existing tests. - `cargo doc --no-deps -Dwarnings` clean - `cargo test -p shipper-types` + `-p shipper --lib runtime::execution` pass (no regressions) ## Related - Parent: #99 Reconcile - Prior PRs: #111 (in-flight reconcile), #115 (resume-path reconcile) - Pillar: Reconcile (#102)
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 15 minutes and 54 seconds. ⌛ 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 (3)
✨ 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 |
There was a problem hiding this comment.
Code Review
This pull request adds documentation to clarify that cargo's output is treated as a non-authoritative hint, while the registry remains the source of truth for reconciliation. The changes include updates to the ErrorClass enum documentation, the classify_cargo_failure function, and the project's high-level explanation. One review comment suggests improving documentation links and using idiomatic crate-relative paths for internal references.
| /// flow — never from the cargo text alone. See the `ErrorClass` rustdoc | ||
| /// and `shipper::engine::parallel::reconcile` for the "hint vs truth" |
There was a problem hiding this comment.
Using a link for ErrorClass improves navigation. Also, using crate:: for internal module references is more idiomatic and ensures the path resolves correctly within the crate's documentation.
| /// flow — never from the cargo text alone. See the `ErrorClass` rustdoc | |
| /// and `shipper::engine::parallel::reconcile` for the "hint vs truth" | |
| /// flow — never from the cargo text alone. See the [`ErrorClass`] rustdoc | |
| /// and `crate::engine::parallel::reconcile` for the "hint vs truth" |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
… + state-files cheat sheet Closes the "scary-but-correct UX" papercuts flagged across multiple external retrospectives. Three new docs covering operator-legibility gaps where Shipper's behavior is correct but the operator's mental model wasn't supported. ## New docs (Diátaxis) - **`docs/explanation/finishability.md`** — why `finishability = not_proven` is the correct answer on first publish, not danger. Maps each case (first publish, token lacks ownership, network flake) to concrete operator action. Notes the future rehearsal-registry path (#97) that will promote more NotProven cases to Proven. - **`docs/how-to/inspect-a-stalled-run.md`** — live triage. "Is the train alive or hung?" 30-second check using events.jsonl tail. Maps common questions (current crate, how long waiting, what will resume do, why did it fail) to the authoritative file + jq recipe. Distinguished from the existing `inspect-state-and-receipts.md` which covers post-hoc "what happened." - **`docs/reference/state-files.md`** — one-page cheat sheet. Authority order (events > state > receipt), per-file purpose table, key field paths (including the `.packages[].state.state` nesting caveat — common misread), jq one-liners for the most frequent queries, sidecar files reference. ## Navigation updates - `docs/README.md` (Diátaxis index): add all three new entries under their correct quadrants - Root `README.md`: extend the quick-links strip to include stalled-run triage, state-files cheat sheet, and the not_proven explainer ## Scope Pure docs. No code, no schema, no snapshot churn. ~450 lines of new content in 3 files + 2 index updates. ## Why these three specifically Three external retrospectives converged on the same "scary-but-correct" list: - `not_proven` reads alarming but is epistemically honest for first-publish - Operators don't always know which file answers which question - There's no live-triage guide for "is it still alive?" vs "did it stall?" This pack closes all three in a single focused PR. ## Related - #103 Narrate umbrella (operator legibility is the theme; see scout comment) - #99 follow-ons — complements #117 (cargo-stdout-as-hint docs) merged earlier this session - #93 events-as-truth (the INVARIANTS the cheat sheet references)
… + state-files cheat sheet (#118) Closes the "scary-but-correct UX" papercuts flagged across multiple external retrospectives. Three new docs covering operator-legibility gaps where Shipper's behavior is correct but the operator's mental model wasn't supported. ## New docs (Diátaxis) - **`docs/explanation/finishability.md`** — why `finishability = not_proven` is the correct answer on first publish, not danger. Maps each case (first publish, token lacks ownership, network flake) to concrete operator action. Notes the future rehearsal-registry path (#97) that will promote more NotProven cases to Proven. - **`docs/how-to/inspect-a-stalled-run.md`** — live triage. "Is the train alive or hung?" 30-second check using events.jsonl tail. Maps common questions (current crate, how long waiting, what will resume do, why did it fail) to the authoritative file + jq recipe. Distinguished from the existing `inspect-state-and-receipts.md` which covers post-hoc "what happened." - **`docs/reference/state-files.md`** — one-page cheat sheet. Authority order (events > state > receipt), per-file purpose table, key field paths (including the `.packages[].state.state` nesting caveat — common misread), jq one-liners for the most frequent queries, sidecar files reference. ## Navigation updates - `docs/README.md` (Diátaxis index): add all three new entries under their correct quadrants - Root `README.md`: extend the quick-links strip to include stalled-run triage, state-files cheat sheet, and the not_proven explainer ## Scope Pure docs. No code, no schema, no snapshot churn. ~450 lines of new content in 3 files + 2 index updates. ## Why these three specifically Three external retrospectives converged on the same "scary-but-correct" list: - `not_proven` reads alarming but is epistemically honest for first-publish - Operators don't always know which file answers which question - There's no live-triage guide for "is it still alive?" vs "did it stall?" This pack closes all three in a single focused PR. ## Related - #103 Narrate umbrella (operator legibility is the theme; see scout comment) - #99 follow-ons — complements #117 (cargo-stdout-as-hint docs) merged earlier this session - #93 events-as-truth (the INVARIANTS the cheat sheet references)
…omes (#99 follow-on) Closes the last remaining #99 follow-on: end-to-end integration coverage for the three reconciliation outcomes the state machine can produce. ## Three scenarios added to engine::parallel::tests Each exercises publish_package() against a real tiny_http mock registry with a fake cargo binary configured to exit ambiguously (exit 1, empty stdout/stderr — which classify_publish_failure maps to CargoFailureClass::Ambiguous per existing unit tests). Readiness polling is disabled (readiness.enabled = false) so each reconcile site reduces to a single registry query — avoids flaky request counts that a full polling loop would produce. ### 1. reconcile_bdd_ambiguous_resolves_to_published - cargo fails ambiguously - entry + post-cargo quick checks return 404 - reconcile's single version_exists poll returns 200 - expected: PackageState::Published, attempts=1 (no second cargo) - verifies PublishReconciling + PublishReconciled{Published} events emitted to the on-disk event log ### 2. reconcile_bdd_ambiguous_resolves_to_not_published_then_retries - cargo fails ambiguously on every attempt - registry consistently returns 404 - reconcile resolves to NotPublished, falls through to normal retry - after max_attempts=2, package ends Failed - verifies PublishReconciled{NotPublished} event emitted - verifies cargo was actually retried (attempts=2) ### 3. reconcile_bdd_resume_from_ambiguous_state_skips_republish - state pre-seeded with PackageState::Ambiguous (simulating a prior interrupted run) - entry check returns 404 (bypasses "already published" short-circuit) - resume-path reconcile (from PR #115) fires BEFORE retry loop - single version_exists query returns 200 → Published - expected: attempts=0 (cargo NOT invoked) - verifies via SHIPPER_CARGO_ARGS_LOG that cargo binary was never called - verifies PublishReconciling + PublishReconciled{Published} events emitted ## Verification - cargo test -p shipper --lib -- reconcile_bdd: 3 pass - cargo test -p shipper --lib -- engine::parallel: 70 pass (no regressions) - cargo clippy --all-targets -- -D warnings: clean - cargo fmt --check: clean ## Why events are read from disk publish_package flushes event batches to events.jsonl and clears the in-memory log after each write. Assertions read via EventLog::read_from_file(&events_path) to see the full persisted stream. ## #99 tracker after this merge - [x] In-flight reconcile state machine (#111) - [x] Resume-path reconciliation (#115) - [x] Docs/contracts: cargo text is a hint, registry is truth (#117) - [x] BDD integration scenarios for all 3 outcomes (this PR) #99 is now fully closed.
…omes (#99 follow-on) (#119) Closes the last remaining #99 follow-on: end-to-end integration coverage for the three reconciliation outcomes the state machine can produce. ## Three scenarios added to engine::parallel::tests Each exercises publish_package() against a real tiny_http mock registry with a fake cargo binary configured to exit ambiguously (exit 1, empty stdout/stderr — which classify_publish_failure maps to CargoFailureClass::Ambiguous per existing unit tests). Readiness polling is disabled (readiness.enabled = false) so each reconcile site reduces to a single registry query — avoids flaky request counts that a full polling loop would produce. ### 1. reconcile_bdd_ambiguous_resolves_to_published - cargo fails ambiguously - entry + post-cargo quick checks return 404 - reconcile's single version_exists poll returns 200 - expected: PackageState::Published, attempts=1 (no second cargo) - verifies PublishReconciling + PublishReconciled{Published} events emitted to the on-disk event log ### 2. reconcile_bdd_ambiguous_resolves_to_not_published_then_retries - cargo fails ambiguously on every attempt - registry consistently returns 404 - reconcile resolves to NotPublished, falls through to normal retry - after max_attempts=2, package ends Failed - verifies PublishReconciled{NotPublished} event emitted - verifies cargo was actually retried (attempts=2) ### 3. reconcile_bdd_resume_from_ambiguous_state_skips_republish - state pre-seeded with PackageState::Ambiguous (simulating a prior interrupted run) - entry check returns 404 (bypasses "already published" short-circuit) - resume-path reconcile (from PR #115) fires BEFORE retry loop - single version_exists query returns 200 → Published - expected: attempts=0 (cargo NOT invoked) - verifies via SHIPPER_CARGO_ARGS_LOG that cargo binary was never called - verifies PublishReconciling + PublishReconciled{Published} events emitted ## Verification - cargo test -p shipper --lib -- reconcile_bdd: 3 pass - cargo test -p shipper --lib -- engine::parallel: 70 pass (no regressions) - cargo clippy --all-targets -- -D warnings: clean - cargo fmt --check: clean ## Why events are read from disk publish_package flushes event batches to events.jsonl and clears the in-memory log after each write. Assertions read via EventLog::read_from_file(&events_path) to see the full persisted stream. ## #99 tracker after this merge - [x] In-flight reconcile state machine (#111) - [x] Resume-path reconciliation (#115) - [x] Docs/contracts: cargo text is a hint, registry is truth (#117) - [x] BDD integration scenarios for all 3 outcomes (this PR) #99 is now fully closed.
Closes one of the remaining #99 follow-ons: make it explicit that cargo's stdout/stderr is a hint for Shipper's retry classification, not the authoritative answer.
What this changes
Why this matters
After #111 (in-flight reconcile) and #115 (resume-path reconcile) landed, the reconciliation flow is authoritative in code. This PR makes it authoritative in the documented contract — so contributors adding new patterns to cargo-failure classification know where the hint ends and truth begins.
Verification
Files
Net: 3 files, +46 / -2.
#99 scorecard after this merge
Related