Back to feed

sledtools/pika branch #66

pika-orch-incus-cleanup-12

Simplify pika-news RunBundle loading

Target branch: master

Merge Commit: adb0227668539ee6d22336fbef4e0692a487c19d

branch: merged tutorial: ready ci: success
Open CI Details

Continuous Integration

CI: success

Compact status on the review page, with full logs on the CI page.

Open CI Details

Latest run #85 success

2 passed

head 0234a4f10cd6b18c627d61a414c7b785d523bae7 · queued 2026-03-25 20:55:44 · 2 lane(s)

queued 34s · ran 29s

check-notifications · success check-agent-contracts · success

Summary

This branch simplifies pika-news RunBundle loading by removing three thin wrapper functions (load_pikaci_run, load_pikaci_logs, load_pikaci_run_bundle) that did nothing beyond delegating to require_pikaci_run_store and then calling methods on the store. Each call site in the handler functions is refactored to invoke require_pikaci_run_store and the store methods directly via .and_then() chains, and the load_pikaci_run_bundle return type is replaced with direct struct field access on the RunBundle, eliminating a destructuring tuple that obscured intent.

Tutorial Steps

Inline RunBundle loading in the branch-logs handler

Intent: Replace the `load_pikaci_run_bundle` wrapper with a direct call to `store.load_run_bundle()`, and destructure the resulting `RunBundle` struct by field name instead of a positional tuple, improving readability.

Affected files: crates/pika-news/src/web.rs

Evidence 
@@ -2606,15 +2606,16 @@
-            let (pikaci_run, pikaci_log_metadata, pikaci_prepared_outputs) = lane
-                .pikaci_run_id
-                .as_deref()
-                .and_then(|pikaci_run_id| {
-                    load_pikaci_run_bundle(state.pikaci_run_store.as_ref(), pikaci_run_id).ok()
-                })
-                .map_or((None, None, None), |(run, logs, prepared_outputs)| {
-                    (Some(run), Some(logs), prepared_outputs)
-                });
+            let bundle = lane.pikaci_run_id.as_deref().and_then(|pikaci_run_id| {
+                state
+                    .pikaci_run_store
+                    .as_ref()
+                    .and_then(|store| store.load_run_bundle(pikaci_run_id).ok())
+            });
+            let (pikaci_run, pikaci_log_metadata, pikaci_prepared_outputs) = match bundle {
+                Some(bundle) => (Some(bundle.run), Some(bundle.logs), bundle.prepared_outputs),
+                None => (None, None, None),
+            };

The old code called load_pikaci_run_bundle(state.pikaci_run_store.as_ref(), pikaci_run_id) which internally called require_pikaci_run_store(store)? and then destructured the bundle into a tuple (RunRecord, RunLogsMetadata, Option<PreparedOutputsRecord>).

The new code chains directly:

  1. state.pikaci_run_store.as_ref() — gets the Option<&PikaciRunStore>.
  2. .and_then(|store| store.load_run_bundle(pikaci_run_id).ok()) — calls the store method and converts the Result to Option.

The subsequent match on bundle uses named fields (bundle.run, bundle.logs, bundle.prepared_outputs) instead of positional tuple access, making it immediately clear what each value represents.

Inline run loading in the single-run handler

Intent: Replace `load_pikaci_run` with a direct `require_pikaci_run_store` + `store.load_run` chain, removing one layer of indirection.

Affected files: crates/pika-news/src/web.rs

Evidence 
@@ -2643,7 +2644,9 @@
-    match load_pikaci_run(state.pikaci_run_store.as_ref(), &run_id) {
+    match require_pikaci_run_store(state.pikaci_run_store.as_ref())
+        .and_then(|store| store.load_run(&run_id))
+    {

The api_forge_pikaci_run_handler previously delegated to load_pikaci_run(store, &run_id), a three-line function whose only job was require_pikaci_run_store(store)?.load_run(run_id). The call site now expresses this directly with an .and_then() combinator, keeping the same error-propagation semantics while removing the wrapper.

Inline log loading in the logs handler

Intent: Replace `load_pikaci_logs` with a direct `require_pikaci_run_store` + `store.load_logs` chain.

Affected files: crates/pika-news/src/web.rs

Evidence 
@@ -2662,12 +2665,13 @@
-    match load_pikaci_logs(
-        state.pikaci_run_store.as_ref(),
-        &run_id,
-        query.job.as_deref(),
-        map_forge_pikaci_log_kind(query.kind),
-    ) {
+    match require_pikaci_run_store(state.pikaci_run_store.as_ref()).and_then(|store| {
+        store.load_logs(
+            &run_id,
+            query.job.as_deref(),
+            map_forge_pikaci_log_kind(query.kind),
+        )
+    }) {

Same pattern as the run handler: load_pikaci_logs accepted the Option<&PikaciRunStore> plus the query parameters and forwarded them. The handler now calls require_pikaci_run_store itself and chains store.load_logs(...) directly, preserving identical behavior.

Inline bundle loading in the prepared-outputs handler and use struct fields

Intent: Replace `load_pikaci_run_bundle` with a direct store call and switch from tuple pattern matching to named struct field access, also improving the nested match for the optional prepared outputs.

Affected files: crates/pika-news/src/web.rs

Evidence 
@@ -2691,17 +2695,23 @@
-    match load_pikaci_run_bundle(state.pikaci_run_store.as_ref(), &run_id) {
-        Ok((_, _, Some(prepared_outputs))) => Json(ForgePikaciPreparedOutputsResponse {
+    match require_pikaci_run_store(state.pikaci_run_store.as_ref())
+        .and_then(|store| store.load_run_bundle(&run_id))
+    {
+        Ok(bundle) => match bundle.prepared_outputs {
+            Some(prepared_outputs) => Json(ForgePikaciPreparedOutputsResponse {

The old code matched on Ok((_, _, Some(prepared_outputs))) and Ok((_, _, None)), discarding the first two tuple elements with underscores. The refactored version matches Ok(bundle) and then destructures bundle.prepared_outputs in a nested match, which:

  • Makes it explicit that only prepared_outputs is used.
  • Avoids the opaque (_, _, ...) positional pattern.
  • Reads naturally as "load the bundle, then check if prepared outputs exist."

Delete the now-unused wrapper functions

Intent: Remove the three indirection functions that are no longer called anywhere, reducing the module's surface area.

Affected files: crates/pika-news/src/web.rs

Evidence 
@@ -2710,27 +2720,6 @@
-fn load_pikaci_run(store: Option<&PikaciRunStore>, run_id: &str) -> anyhow::Result<RunRecord> {
-    require_pikaci_run_store(store)?.load_run(run_id)
-}
-
-fn load_pikaci_logs(
-    store: Option<&PikaciRunStore>,
-    run_id: &str,
-    job_id: Option<&str>,
-    kind: LogKind,
-) -> anyhow::Result<pikaci::Logs> {
-    require_pikaci_run_store(store)?.load_logs(run_id, job_id, kind)
-}
-
-fn load_pikaci_run_bundle(
-    store: Option<&PikaciRunStore>,
-    run_id: &str,
-) -> anyhow::Result<(RunRecord, RunLogsMetadata, Option<PreparedOutputsRecord>)> {
-    let bundle = require_pikaci_run_store(store)?.load_run_bundle(run_id)?;
-    Ok((bundle.run, bundle.logs, bundle.prepared_outputs))
-}

With every call site inlined, the three functions load_pikaci_run, load_pikaci_logs, and load_pikaci_run_bundle are dead code. Deleting them removes 21 lines and eliminates the tuple-based API surface that load_pikaci_run_bundle imposed (returning (RunRecord, RunLogsMetadata, Option<PreparedOutputsRecord>) instead of the natural RunBundle struct). The remaining require_pikaci_run_store helper is still used by all handlers and is retained.

Diff