RELEASE_CHECKLIST_v0.2.0.md (1)

63-67: ⚠️ Potential issue | 🟡 Minor

Publish commands don't cover the new workspace crates.

Multiple new crates introduced in this PR (shipper-types, shipper-environment, shipper-events, shipper-storage, shipper-registry, shipper-retry, shipper-webhook, etc.) each declare a documentation = "https://docs.rs/..." URL, indicating intent to publish. They are absent from the publish block. If these crates are meant to be published as part of v0.2.0, the block needs to be expanded — and in dependency order (leaves first, e.g. shipper-types before any crate that depends on it).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@RELEASE_CHECKLIST_v0.2.0.md` around lines 63 - 67, The publish checklist only
lists shipper and shipper-cli but omits the new workspace crates; update the
"Publish to crates.io" block to include all new crates (at least shipper-types,
shipper-environment, shipper-events, shipper-storage, shipper-registry,
shipper-retry, shipper-webhook, etc.) in the correct dependency order (publish
leaf crates like shipper-types first, then crates that depend on them, ending
with shipper and shipper-cli); ensure each crate appears as a separate cargo
publish -p <crate-name> entry and order them so dependents come after their
dependencies.

crates/shipper/src/config.rs (1)

864-1221: 🧹 Nitpick | 🔵 Trivial

Tests use plain assertions instead of insta snapshot testing.

The coding guidelines recommend using insta for snapshot testing. While the current assertions are correct, consider snapshot-testing the TOML template output and the RuntimeOptions produced by build_runtime_options to improve regression detection as the config surface grows.

As per coding guidelines, "Use snapshot testing with insta for test assertions."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper/src/config.rs` around lines 864 - 1221, Replace plain
assert-based checks in the tests (notably test_parse_toml_config,
test_parse_toml_with_registry, test_parse_toml_with_parallel,
test_parse_toml_with_partial_*, and the test_build_runtime_options* tests) with
insta snapshot assertions: serialize the parsed ShipperConfig/TOML output and
the RuntimeOptions returned by ShipperConfig::build_runtime_options (use
serde_json or Debug) and call insta::assert_snapshot! (or assert_json_snapshot!)
to capture expected output; update test names/fixtures accordingly and
add/modify the test module to initialize insta (if needed) so snapshots are
created/checked instead of using multiple assert_eq!/assert! calls.

crates/shipper-cli/tests/cli_e2e.rs (2)

906-941: 🧹 Nitpick | 🔵 Trivial

Redundant parentheses in vec! literal.

vec![(404), (404)] — the parens around the integer literals are unnecessary.

Nit

     let registry = spawn_registry(
-        vec![
-            (404), // crate check
-            (404), // version check
-        ],
+        vec![404, 404],
         2,
     );

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper-cli/tests/cli_e2e.rs` around lines 906 - 941, In the test
preflight_command_with_new_crates replace the vec![(404), (404)] with a plain
vec![404, 404] (remove the unnecessary parentheses around the integer literals)
so the registry mock invocation uses a normal vec! literal; update the
occurrence in the spawn_registry(...) call to reference vec![404, 404]
accordingly.

---

129-149: ⚠️ Potential issue | 🟡 Minor

spawn_registry server thread can hang if a test exits early.

The server thread blocks on server.recv() for exactly expected_requests iterations. If a test assertion fails or panics before all requests are sent, registry.join() (or the implicit thread destructor) will hang. Consider adding a short receive timeout or switching to a try_recv-style pattern with a shutdown signal.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper-cli/tests/cli_e2e.rs` around lines 129 - 149, The spawned
server thread in spawn_registry blocks forever on server.recv() if the test
panics or exits early; change the request loop to use a timed receive (e.g.,
server.recv_timeout(Duration::from_millis(...)) or a try_recv pattern) and
handle the timeout by breaking out of the loop and exiting the thread so
TestRegistry.handle.join() cannot hang; update spawn_registry to treat a timeout
as a signal to stop (while still honoring expected_requests when requests
arrive) and ensure any Err from recv_timeout is handled gracefully so the thread
always terminates.

crates/shipper/src/cargo.rs (1)

100-146: 🧹 Nitpick | 🔵 Trivial

Minor: CargoOutput.duration is measured independently of CommandOutput.duration.

start.elapsed() at line 137 captures wall-clock time from the caller's perspective, while process::run_command_with_timeout also records its own duration internally (which is discarded here). This is fine if the intent is caller-perspective timing, but it does mean the duration includes the overhead of building the args vector and context wrapping. If you ever need process-only duration, consider using output.duration instead.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper/src/cargo.rs` around lines 100 - 146, The CargoOutput.duration
in cargo_publish is currently set from start.elapsed() (measuring caller-side
wall time) while process::run_command_with_timeout already returns a
process-only duration on the returned CommandOutput; swap to use the returned
output.duration (from the value returned by process::run_command_with_timeout)
when constructing CargoOutput so CargoOutput.duration reflects the process
execution time, or explicitly document/choose which timing is desired in
cargo_publish.

crates/shipper/src/plan.rs (1)

324-377: 🧹 Nitpick | 🔵 Trivial

group_by_levels is duplicated between plan.rs and shipper-types.

This implementation (lines 324–377) is nearly identical to the one in crates/shipper-types/src/lib.rs:695-732. The #[cfg(not(feature = "micro-types"))] gate ensures only one is compiled, but having two copies to maintain is a code smell. Consider extracting the algorithm into a shared free function that both sites call, or consolidating it in one location with a re-export.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper/src/plan.rs` around lines 324 - 377, The
ReleasePlan::group_by_levels implementation is duplicated; extract the leveling
algorithm into a single shared free function (e.g., group_packages_by_levels)
that takes the plan inputs (packages: &[Package], dependencies: &HashMap<String,
Vec<String>>) and returns Vec<crate::types::PublishLevel>, then replace the
ReleasePlan::group_by_levels definition to call that shared function; ensure the
shared function lives in a common crate (preferably shipper-types) and is
re-exported or called from both crates so only one implementation of the
algorithm remains and ReleasePlan (and any other callers) delegate to it.

crates/shipper/src/engine_parallel.rs (1)

694-715: 🛠️ Refactor suggestion | 🟠 Major

Unnecessary double-locking: SendReporter fields are Mutex<Vec<String>> but the struct is already behind Arc<Mutex<…>>.

The outer Mutex grants exclusive &mut self access. The inner Mutex on each field is redundant and adds unnecessary synchronization overhead.

Proposed fix

     struct SendReporter {
-        infos: Mutex<Vec<String>>,
-        warns: Mutex<Vec<String>>,
-        errors: Mutex<Vec<String>>,
+        infos: Vec<String>,
+        warns: Vec<String>,
+        errors: Vec<String>,
     }
     impl Reporter for SendReporter {
         fn info(&mut self, msg: &str) {
-            self.infos.lock().unwrap().push(msg.to_string());
+            self.infos.push(msg.to_string());
         }
         fn warn(&mut self, msg: &str) {
-            self.warns.lock().unwrap().push(msg.to_string());
+            self.warns.push(msg.to_string());
         }
         fn error(&mut self, msg: &str) {
-            self.errors.lock().unwrap().push(msg.to_string());
+            self.errors.push(msg.to_string());
         }
     }
 
     let send_reporter = Arc::new(Mutex::new(SendReporter {
-        infos: Mutex::new(Vec::new()),
-        warns: Mutex::new(Vec::new()),
-        errors: Mutex::new(Vec::new()),
+        infos: Vec::new(),
+        warns: Vec::new(),
+        errors: Vec::new(),
     }));

Also update the replay section:

     {
         let sr = send_reporter.lock().unwrap();
-        for msg in sr.infos.lock().unwrap().iter() {
+        for msg in sr.infos.iter() {
             reporter.info(msg);
         }
-        for msg in sr.warns.lock().unwrap().iter() {
+        for msg in sr.warns.iter() {
             reporter.warn(msg);
         }
-        for msg in sr.errors.lock().unwrap().iter() {
+        for msg in sr.errors.iter() {
             reporter.error(msg);
         }
     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper/src/engine_parallel.rs` around lines 694 - 715, The
SendReporter struct currently wraps its Vec<String> fields in Mutex while the
entire struct is already stored in an Arc<Mutex<...>>, causing redundant
locking; change SendReporter::infos, warns, errors from Mutex<Vec<String>> to
plain Vec<String>, update the impl Reporter methods (info/warn/error) to push
directly on &mut self (no inner locks), and update the send_reporter
construction to initialize plain Vecs inside Arc::new(Mutex::new(SendReporter {
infos: Vec::new(), warns: Vec::new(), errors: Vec::new() })); also propagate the
same removal of inner Mutexes to the replay section mentioned so usages assume
outer Mutex protection.

crates/shipper-cli/src/main.rs (1)

426-445: ⚠️ Potential issue | 🟡 Minor

ProgressReporter updates are incomplete—only the first package is displayed, with no progress tracking during the publish loop.

ProgressReporter is created and set_package() is called once for the first package before engine::run_publish() executes. However, engine::run_publish() receives the CliReporter (which implements the Reporter trait), not ProgressReporter. The progress object is then abandoned during the publish loop and finish() is called after completion. The finish_package() method exists but is never invoked in production code—it appears only in unit tests.

To properly track progress, either:

• Pass progress to the engine and call progress.set_package() and progress.finish_package() for each package in the publish loop, or
• Implement Reporter on ProgressReporter and use it instead of CliReporter

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper-cli/src/main.rs` around lines 426 - 445, Progress reporting is
incomplete because a CliReporter is passed into engine::run_publish while
ProgressReporter is created but never used; implement the Reporter trait for
ProgressReporter (implementing the same callbacks used by the engine such as
package start/finish/status updates) and then pass &mut progress into
engine::run_publish instead of &mut reporter so the publish loop drives
ProgressReporter.set_package(...) and finish_package(...) automatically; keep
the final progress.finish() call and remove or consolidate the manual initial
set_package() if that becomes redundant once Reporter methods are wired up.

crates/shipper/src/engine.rs (2)

2409-2525: ⚠️ Potential issue | 🟡 Minor

Add #[serial] to filesystem-mutating tests.

These tests create tempdirs and write state files but aren’t serialized, which can introduce cross-test interference on shared FS state.

✅ Suggested fix (apply to all listed tests)

 #[test]
+#[serial]
 fn run_publish_errors_on_plan_mismatch_without_force_resume() {

 #[test]
+#[serial]
 fn run_publish_allows_forced_resume_with_plan_mismatch() {

 #[test]
+#[serial]
 fn run_resume_errors_when_state_is_missing() {

 #[test]
+#[serial]
 fn run_resume_runs_publish_when_state_exists() {

As per coding guidelines, Use #[serial] attribute from serial_test for tests that mutate global environment or filesystem state.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper/src/engine.rs` around lines 2409 - 2525, Add the #[serial]
attribute (from the serial_test crate) to each filesystem-mutating test to
prevent cross-test interference: apply it to the test functions
run_publish_errors_on_plan_mismatch_without_force_resume,
run_publish_allows_forced_resume_with_plan_mismatch,
run_resume_errors_when_state_is_missing, and
run_resume_runs_publish_when_state_exists; also ensure the serial_test
dependency is in Cargo.toml and the serial attribute is imported (e.g., use
serial_test::serial) where these tests live.

---

670-678: ⚠️ Potential issue | 🟠 Major

Record success when the registry confirms publish after a cargo failure.

When cargo publish fails but version_exists returns true, the package is marked Published without a PackagePublished event or PublishSucceeded webhook, leaving audit/webhook consumers blind to the success.

✅ Suggested fix

                     if reg.version_exists(&p.name, &p.version)? {
                         reporter.info(&format!(
                             "{}@{}: version is present on registry; treating as published",
                             p.name, p.version
                         ));
                         update_state(&mut st, &state_dir, &key, PackageState::Published)?;
+
+                        event_log.record(PublishEvent {
+                            timestamp: Utc::now(),
+                            event_type: EventType::PackagePublished {
+                                duration_ms: start_instant.elapsed().as_millis() as u64,
+                            },
+                            package: pkg_label.clone(),
+                        });
+                        event_log.write_to_file(&events_path)?;
+                        event_log.clear();
+
+                        webhook::maybe_send_event(
+                            &opts.webhook,
+                            WebhookEvent::PublishSucceeded {
+                                plan_id: ws.plan.plan_id.clone(),
+                                package_name: p.name.clone(),
+                                package_version: p.version.clone(),
+                                duration_ms: start_instant.elapsed().as_millis() as u64,
+                            },
+                        );
+
                         last_err = None;
                         break;
                     }

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@crates/shipper/src/engine.rs` around lines 670 - 678, When
reg.version_exists(&p.name, &p.version) returns true after a failed cargo
publish, we currently call update_state(&mut st, &state_dir, &key,
PackageState::Published) and clear last_err but do not emit the PackagePublished
event or trigger the PublishSucceeded webhook; modify the branch handling that
condition to replicate the successful-publish path: invoke the same event
emission (e.g., emit or record a PackagePublished event) and call the
PublishSucceeded webhook handler used elsewhere so audit logs and webhook
consumers see the success, keeping update_state, clearing last_err, and
preserving any retry/metrics logic around functions like reg.version_exists,
update_state, PackageState::Published, last_err, and whatever function emits
webhooks/events.