fix(no-panic): first burn-down — 3 indexing sites in shipper-core (#190)#235
Conversation
…#190) Refactors three safe-by-construction indexing operations to their idiomatic Rust equivalents, removing them from the no-panic baseline. All three were already provably safe — the panic surface was statically unreachable — but the new forms make that obvious and let the check recognise them as resolved. ops/cargo/mod.rs is_valid_package_name: replace the `name.is_empty()` guard + `chars: Vec<char>` collection + `chars[0]` indexing with a direct `chars.next()` returning Option<char>. Drops the Vec allocation as a small bonus. runtime/environment/mod.rs normalize_version: replace `.collect::<Vec<_>>()` + `.len() >= 2` guard + `[1]` with `.nth(1)`. Reduces a five-line function to one expression. plan/chunking/mod.rs chunk_by_max_concurrent: replace the hand-rolled index/while loop with the standard `slice::chunks(batch_size)`. Drops the `if items.is_empty()` guard since `[T]::chunks` on an empty slice is already a no-op. Baseline state: Before: 28 entries, 59 production sites by family: expect=4 index=7 unwrap=48 After: 25 entries, 55 production sites by family: expect=4 index=3 unwrap=48 `cargo xtask no-panic check` against the prior baseline correctly reported `resolved=3`, identifying each site by exact identity (path, family, selector_kind, selector_callee, snippet). After regenerating the baseline, `check` reports clean. This is the first burn-down validating the no-panic loop end-to-end. The issue's chosen target (shipper-duration) had zero production debt — all 53 sites flagged in the issue body were in test code, which the detector already excludes from the production baseline via the `*_tests.rs` / `#[cfg(test)]` filtering landed in #228. The real debt lives in shipper-core, so the burn-down moved there. Tests: cargo test -p shipper-core --lib passes (1900/1900).
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
|
Droid finished @EffortlessSteven's task —— View job Droid Review Complete — PR #235 ✅ Status: approved Assessment: All three refactoring sites are behaviorally equivalent to their pre-refactor forms and make the no-panic guarantee obvious to the reader. Inspected surfaces:
Security review: No findings. Why no comments: No actionable findings emitted — this is a pure refactoring PR that maintains behavioral equivalence while improving static safety guarantees. The no-panic check loop was validated end-to-end in #229 and correctly identifies all three resolved sites. |
|
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 (5)
✨ 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.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 82ee7d944f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| } | ||
| } | ||
| } | ||
| //! Cargo metadata loading + `cargo publish` invocation. |
There was a problem hiding this comment.
Restore LF line endings in Rust sources
This commit rewrites this Rust source with CRLF line endings, so git diff --check 2642fb8^ 2642fb8 -- crates/shipper-core/src/ops/cargo/mod.rs crates/shipper-core/src/plan/chunking/mod.rs crates/shipper-core/src/runtime/environment/mod.rs reports every added line as trailing whitespace; the same line-ending conversion is present in plan/chunking/mod.rs and runtime/environment/mod.rs. That makes whitespace validation fail/noisy for this PR and leaves future edits likely to churn entire files, obscuring the intended no-panic refactors. Regenerate these files with LF line endings while keeping only the semantic changes. Confidence: high.
Useful? React with 👍 / 👎.
In #235 (first no-panic burn-down), three shipper-core source files flipped from LF to CRLF in their committed form because: - The repo standard is LF (verified against pre-#235 storage). - The Windows working tree had CRLF (historical autocrlf side-effect from before this checkout). - `core.autocrlf` is `false` in this clone, so git stored whatever bytes were in the working tree. - The Edit tool preserved the working-tree CRLF when it rewrote the files, and the commit therefore stored CRLF. The diff in #235 showed ~4,000 line changes that were actually 15 lines of real code change plus full-file line-ending flips. Unreviewable. This PR locks the policy down: .gitattributes: `* text=auto eol=lf` — every text file is stored with LF regardless of platform. Working-tree files may still be CRLF on Windows; only storage matters. The existing `linguist-generated=true` rules for `policy/no-panic-baseline.json` and `badges/*.json` are preserved. crates/shipper-core/src/ops/cargo/mod.rs crates/shipper-core/src/plan/chunking/mod.rs crates/shipper-core/src/runtime/environment/mod.rs Renormalized back to LF via `dos2unix`. `git diff -w HEAD` against the prior commit produces zero non-whitespace differences — the file content is byte-identical to the pre-#235 state plus the three small semantic refactors #235 actually intended. After this lands, future Edit operations on these files will store LF on commit even when the working tree is CRLF; the .gitattributes rule makes git do the conversion at index time.
Summary
First no-panic burn-down. Refactors three safe-by-construction indexing operations to their idiomatic Rust equivalents, removing them from
policy/no-panic-baseline.json. End-to-end validation of the no-panic check loop landed in #229.What changed
ops/cargo/mod.rsis_valid_package_namename.is_empty()guard +chars: Vec<char>+chars[0]indexingchars.next()returningOption<char>runtime/environment/mod.rsnormalize_version.collect::<Vec<_>>()+.len() >= 2guard +[1].nth(1)plan/chunking/mod.rschunk_by_max_concurrentslice::chunks(batch_size)All three were provably safe before — the panic surface was statically unreachable — but the new forms make that obvious and the no-panic detector recognises them as resolved.
Baseline state
cargo xtask no-panic checkagainst the prior baseline correctly reportedresolved=3, identifying each site by exact identity (path, family, selector_kind, selector_callee, snippet). After regenerating the baseline,checkreports clean.Why not
shipper-duration(the issue's chosen target)?#190's investigation section recommended
shipper-durationbecause it had 53 panic-family items in a small crate. But that count was a grep over all source files — including test code. The detector landed in #228 correctly excludes*_tests.rsfiles and#[cfg(test)]modules, soshipper-durationshows zero entries in the production baseline. The real production debt is inshipper-core(28 entries pre-burndown), so the burn-down moved there.The "narrow lane" framing still applies — I picked the three smallest, lowest-risk indexing sites for the first PR rather than the heavier
.lock().unwrap()patterns inengine/parallel/(~35 sites; want a separate scoped PR for the Mutex poisoning posture).Test plan
cargo test -p shipper-core --lib— 1900/1900 passingcargo clippy --workspace --all-targets -- -D warnings— cleancargo fmt --all -- --check— cleancargo xtask no-panic checkagainst prior baseline reportsresolved=3cargo xtask no-panic baselineregenerated;cargo xtask no-panic checkagainst new baseline reports cleanFollow-ups (not in this PR)
engine/parallel/.lock().unwrap()Mutex-poisoning posture (~35 sites) — its own scoped PR; the right fix is policy-level (parking_lotvsexpect("invariant: ...")vs handlingPoisonError).state/execution_state/mod.rsreceipt["..."] = ...JSON indexing (3 sites) — relies onReceiptalways being a JSON object.engine/mod.rstoken.as_deref().unwrap()(2 sites) — outer Some-guarantee can be hoisted into the call site.Refs #190.