Using to_lowercase().find() and then using the resulting byte offset to slice the original string can lead to a panic if the string contains multi-byte characters where the lowercase representation has a different byte length. While HTTP headers are typically ASCII, making this robust against all UTF-8 strings would be safer. A potential approach is to use a regex with a case-insensitive flag, or iterate character indices to find the correct byte boundaries.

The search for "token" is case-sensitive. This is inconsistent with the case-insensitive handling of Authorization: on line 47. For better robustness, you could consider making this search case-insensitive as well. Note that using to_lowercase().find() can be problematic with non-ASCII characters, as mentioned in another comment.

The val_offset variable is calculated on line 62 but its value is never used, other than to suppress a compiler warning on line 67. This unused code can be removed to improve clarity and maintainability.

            // Check for quoted or unquoted value
            if after_eq.starts_with('"') || after_eq.starts_with('\'') {
                out = format!("{}= \"[REDACTED]\"", &out[..eq_offset]);
            } else if !after_eq.is_empty() {
                out = format!("{}= [REDACTED]", &out[..eq_offset]);
            }

The test expects that resuming from Uploaded state will result in either Published or Skipped state (lines 2800-2806), but the logic at line 401 in engine.rs checks version_exists at the very beginning. Since the test sets up the mock registry to return 200 (version exists), the package will be marked as Skipped with reason "already published" before the Uploaded state handling even runs. This means the test doesn't actually validate the resume-from-Uploaded behavior—it validates the skip-if-already-published behavior instead.

To properly test the Uploaded state resume logic, the test should mock the registry to return 404 initially (version doesn't exist), then return 200 after cargo publish would have completed (simulating successful readiness verification). This would exercise the actual "skip cargo publish but run readiness verification" code path.

When resuming from the Uploaded state in parallel mode, the attempt counter is incremented at line 139 but cargo_succeeded is set to true at line 127. This means that when resuming, the code will:

1. Enter the retry loop with attempt=1
2. Skip cargo publish (because cargo_succeeded=true)
3. Run readiness verification
4. If readiness fails, increment attempt and sleep/retry

However, the attempts counter in the state will be incremented even though no cargo publish is being retried—only the readiness check is being retried. This could be confusing in the receipt, where attempts might show 2 or 3 when only 1 cargo publish was executed (the original one that created the Uploaded state).

Consider either:

1. Not incrementing attempts when resuming from Uploaded (preserve the original attempt count)
2. Or documenting that attempts includes readiness verification retries for resumed publishes

After calling try_wait() and getting Some(status), the code calls wait_with_output() on an already-exited child process. This will fail because try_wait() already reaped the process. The stdout/stderr will not be available after try_wait() returns Some.

Instead, read stdout and stderr from the piped handles before the child exits, or use a different approach like storing the handles and reading them after detecting the process has exited.

The val_offset variable is calculated but never used (note the let _ = val_offset; suppression). This calculation appears unnecessary and could be removed for clarity. If it was intended for a future enhancement, consider adding a comment explaining why it's preserved.

The redaction logic for "token" (line 56) is case-sensitive and only searches for lowercase "token". This means patterns like "Token = value" or "TOKEN = value" won't be redacted. Consider using case-insensitive matching similar to the Authorization header pattern (line 47) to ensure all token patterns are caught.

Do not classify Uploaded as a successful terminal state

Uploaded is introduced as an intermediate checkpoint, but this success predicate treats it as equivalent to Published. In run_publish, a resumed package can remain Uploaded when its persisted attempts is already at max_attempts (the retry loop is skipped), and this branch then marks the whole execution successful without performing readiness verification. That can produce a false-positive successful publish run even though visibility was never confirmed.

Useful? React with 👍 / 👎.

Verify registry before failing timed-out parallel publishes

Parallel mode now passes a per-package timeout into cargo_publish, and timeout expirations surface through this Err(e) path. Returning immediately here skips the registry version_exists reconciliation that non-zero cargo exits get, so a package that uploaded successfully but timed out locally is treated as a hard failure and may be re-attempted on resume (causing duplicate-upload/permanent errors).

Useful? React with 👍 / 👎.

🧹 Nitpick | 🔵 Trivial

Redaction is correctly wired into tails; tighten patterns and remove dead code.

Wiring redact_sensitive() into tail_lines() is the right “defense-in-depth” point (single choke point for receipts/events).

Two concrete cleanups:

• val_offset is computed but unused (then explicitly suppressed). Drop it to reduce confusion.
• Consider extending token assignment matching beyond token = ... (e.g., token:) only if you’ve observed leaks in real cargo output; otherwise current scope is OK.

In `@crates/shipper/src/cargo.rs` around lines 16 - 98, The redact_line function
contains an unused val_offset variable and suppression; remove val_offset and
the unused suppression and simplify the "token" assignment branch in redact_line
to only compute eq_offset and construct the redacted output (no unused locals).
Keep the existing matching behavior for token assignments as-is unless you have
evidence of "token:" style leaks—if you do, add a small extra branch in
redact_line to treat a colon (:) similarly to '=' when trimming after the
"token" key. Ensure references: redact_line, find_cargo_token_env, and
redact_sensitive remain unchanged otherwise.

⚠️ Potential issue | 🟡 Minor

Test env isolation refactor + resume-from-Uploaded coverage look good.

• with_test_env() + fake_program_env_vars() makes env setup consistent and reduces leakage risk.
• Tests that mutate env are consistently #[serial], which avoids cross-test flakiness.

Minor nit: in resume_from_uploaded_skips_cargo_publish_and_reaches_published, the comment/expectation mixes “Published” and “Skipped” (given version_exists returns 200, “Skipped” is the likely state). Consider tightening to the intended invariant (e.g., “must not call cargo publish; must not error; must reach a terminal state”).
As per coding guidelines, "Use #[serial] from serial_test to isolate tests that mutate environment variables or filesystem".

Also applies to: 1350-1578, 1580-2742, 2743-2831

In `@crates/shipper/src/engine.rs` around lines 972 - 993, The test
comment/expectation in
resume_from_uploaded_skips_cargo_publish_and_reaches_published is ambiguous
about the terminal state: when version_exists returns 200 the crate should be
treated as already published ("Skipped") not "Published". Update the test to
assert the concrete invariants — e.g., that cargo publish was not invoked and
the run completes without error and reaches a terminal "Skipped" state — and
update the assertion/messages accordingly; locate the test by the function name
resume_from_uploaded_skips_cargo_publish_and_reaches_published and ensure any
logging/assertion text and expectations are tightened to "Skipped" (or
equivalent terminal enum/state) rather than "Published". Ensure this aligns with
the existing env setup helpers with_test_env and fake_program_env_vars so the
test remains isolated.

🧹 Nitpick | 🔵 Trivial

Dependency validation is the right guardrail; consider reporting all violations at once.

Catching normal/build deps from included publishable crates onto non-publishable workspace members is exactly the “fail fast” you want.

Two improvements that would materially help UX:

• Aggregate all offending edges and report them together (avoid fix-one-run-again loops).
• Include the dependency kind (normal/build) and maybe the workspace registry name in the error message.

In `@crates/shipper/src/plan.rs` around lines 141 - 172, Collect all offending
dependency edges instead of bailing on the first one: walk the same loops (over
resolve.nodes, included, node.deps and checking publishable/workspace_ids and
dep.dep_kinds) but push a formatted violation string into a Vec when
is_normal_or_build is true instead of calling bail immediately; include the
source package name (from pkg_map.get(&node.id)), the dependent package name
(from pkg_map.get(&dep.pkg)), the dependency kind (derive from dep.dep_kinds,
e.g., "normal" or "build") and the workspace/registry identifier (from the
pkg_map/pkg metadata) in each message; after the loops, if the Vec is non-empty,
call bail once with a joined error message listing all violations.

⚠️ Potential issue | 🟡 Minor

Tests cover the new validation well; one test name/fixture doesn’t match what it claims.

build_plan_rejects_publishable_depending_on_non_publishable and the selection test are good coverage.

But build_plan_allows_dev_dep_on_non_publishable doesn’t actually set up a dev-dependency on a non-publishable member (it currently uses alpha dev-depending on a, which is publishable). Either rename it or adjust the fixture to dev-depend on c (publish=false) so it truly asserts the intended “dev-deps are allowed” rule.

Also applies to: 483-498, 528-579

In `@crates/shipper/src/plan.rs` around lines 388 - 407, The test named
build_plan_allows_dev_dep_on_non_publishable is misleading because its fixture
makes alpha dev-depend on a publishable package; update the test fixture so
alpha dev-depends on a non-publishable member (e.g., change the dependency to
point to "c" which has publish = false) or rename the test to match the current
fixture; search for build_plan_allows_dev_dep_on_non_publishable and associated
test fixtures (and the create_workspace/create_workspace_with_npdep helpers) and
modify the dependency entry for alpha to target "c" (or alternatively rename the
test) consistently in the other test occurrences mentioned.

🧹 Nitpick | 🔵 Trivial

Test helper uses to_lowercase() while production code uses to_ascii_lowercase().

calculate_index_path_for_crate in this test helper (Line 928) uses to_lowercase(), but the production calculate_index_path in registry.rs (Line 142) uses to_ascii_lowercase(). These behave differently for non-ASCII input (e.g., Unicode case folding). The proptest regex only generates ASCII, so it won't catch this divergence, but the helper should mirror production exactly.

     fn calculate_index_path_for_crate(crate_name: &str) -> String {
-        let lower = crate_name.to_lowercase();
+        let lower = crate_name.to_ascii_lowercase();
         match lower.len() {

-            let lower = crate_name.to_lowercase();
+            let lower = crate_name.to_ascii_lowercase();

In `@crates/shipper/src/types.rs` around lines 927 - 935, Test helper
calculate_index_path_for_crate is using to_lowercase() which diverges from
production calculate_index_path; change calculate_index_path_for_crate to use
to_ascii_lowercase() instead (so the crate-name canonicalization matches
production), and likewise update index_path_follows_pattern to use
to_ascii_lowercase() where it currently calls to_lowercase(); keep the same
slicing/formatting logic but operate on the ASCII-lowercased string.