sledtools/pika branch #61

pika-orch-incus-cleanup-10

Centralize pika-news pikaci run loading

Target branch: master

Merge Commit: 2713b7b255295c631b3125d610927bb9198f6a12

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 #82 success

2 passed

head 43bd9f958fccde7a25c46dbbe7170c54158f0a99 · queued 2026-03-25 20:43:18 · 2 lane(s)

queued 1m 06s · ran 24s

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

Summary

This branch introduces a dedicated PikaciRunStore abstraction that centralizes all pikaci run-record, log, and bundle loading behind a single struct. Previously, every call site in the web module independently resolved the forge repo configuration, computed the state root path, and called raw pikaci loading functions. The new PikaciRunStore is constructed once from Config during application startup, stored in AppState, and threaded through to all handler helpers. This eliminates duplicated config-to-path resolution logic, makes the dependency on a configured forge repo explicit via Option<PikaciRunStore>, and simplifies each handler to a one-liner delegation.

Tutorial Steps

Create the PikaciRunStore module

Intent: Introduce a new module that encapsulates the state-root derivation and all pikaci data-loading operations (run records, logs, run bundles) behind a clean struct interface, removing the need for callers to manually resolve forge repo config and compute paths.

Affected files: crates/pika-news/src/pikaci_store.rs, crates/pika-news/src/main.rs

Evidence

@@ -0,0 +1,46 @@
+use std::path::{Path, PathBuf};
+
+use anyhow::{anyhow, Result};
+use pikaci::{load_logs, load_run_bundle, load_run_record, LogKind, Logs, RunBundle, RunRecord};
+
+use crate::config::{Config, ForgeRepoConfig};
+use crate::forge;
+
+#[derive(Clone, Debug)]
+pub struct PikaciRunStore {
+    state_root: PathBuf,
+}
+
+impl PikaciRunStore {
+    pub fn from_config(config: &Config) -> Option<Self> {
+    pub fn from_forge_repo(repo: &ForgeRepoConfig) -> Self {
+    pub fn state_root(&self) -> &Path {
+    pub fn load_run(&self, run_id: &str) -> Result<RunRecord> {
+    pub fn load_logs(&self, run_id: &str, job_id: Option<&str>, kind: LogKind) -> Result<Logs> {
+    pub fn load_run_bundle(&self, run_id: &str) -> Result<RunBundle> {
+}
+
+pub fn require_pikaci_run_store(store: Option<&PikaciRunStore>) -> Result<&PikaciRunStore> {
+    store.ok_or_else(|| anyhow!("forge repo is not configured"))
+}

@@ -13,6 +13,7 @@ mod live;
 mod local;
 mod mirror;
 mod model;
+mod pikaci_store;
 mod poller;

The new crates/pika-news/src/pikaci_store.rs file defines PikaciRunStore, a small wrapper around a PathBuf state root. It provides two constructors:

from_config(config: &Config) -> Option<Self> — returns None when no forge repo is configured, making the optionality explicit at construction time rather than at every call site.
from_forge_repo(repo: &ForgeRepoConfig) -> Self — for cases where a ForgeRepoConfig is already available.

The struct exposes three data-loading methods (load_run, load_logs, load_run_bundle) that delegate to the raw pikaci crate functions, always passing self.state_root as the base path.

A free function require_pikaci_run_store converts Option<&PikaciRunStore> into a Result, producing a consistent error message when the store is absent.

The module is registered in main.rs via mod pikaci_store;.

Add PikaciRunStore to AppState and initialize at startup

Intent: Store the pre-built PikaciRunStore in the shared application state so every request handler can access it without re-deriving the forge repo path from config on each call.

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

Evidence

@@ -52,6 +49,7 @@ use crate::pikaci_store::{require_pikaci_run_store, PikaciRunStore};
 struct AppState {
     store: Store,
     config: Config,
+    pikaci_run_store: Option<PikaciRunStore>,
     auth: Arc<AuthState>,

@@ -695,6 +693,7 @@ pub async fn serve(
     let state = Arc::new(AppState {
         store,
         config: config.clone(),
+        pikaci_run_store: PikaciRunStore::from_config(&config),
         auth,

AppState gains a new field pikaci_run_store: Option<PikaciRunStore>. It is initialized once in the serve function via PikaciRunStore::from_config(&config), which returns None when no forge repo is configured. This moves the config lookup from per-request to once-at-startup.

Refactor handler helpers to use PikaciRunStore

Intent: Replace the repeated pattern of resolving the forge repo from config and computing the state root in each handler helper with a single delegation to the PikaciRunStore methods.

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

Evidence

@@ -2709,36 +2710,24 @@ async fn api_forge_pikaci_prepared_outputs_handler(
-fn load_pikaci_run(config: &Config, run_id: &str) -> anyhow::Result<RunRecord> {
-    let forge_repo = config
-        .effective_forge_repo()
-        .ok_or_else(|| anyhow::anyhow!("forge repo is not configured"))?;
-    let state_root = forge::pikaci_state_root(&forge_repo);
-    load_run_record(&state_root, run_id)
+fn load_pikaci_run(store: Option<&PikaciRunStore>, run_id: &str) -> anyhow::Result<RunRecord> {
+    require_pikaci_run_store(store)?.load_run(run_id)
 }

@@ -2709,36 +2710,24 @@
 fn load_pikaci_logs(
-    config: &Config,
+    store: Option<&PikaciRunStore>,
     run_id: &str,
     job_id: Option<&str>,
     kind: LogKind,
 ) -> anyhow::Result<pikaci::Logs> {
-    let forge_repo = config
-        .effective_forge_repo()
-        .ok_or_else(|| anyhow::anyhow!("forge repo is not configured"))?;
-    let state_root = forge::pikaci_state_root(&forge_repo);
-    load_logs(&state_root, run_id, job_id, kind)
+    require_pikaci_run_store(store)?.load_logs(run_id, job_id, kind)
 }

@@ -2709,36 +2710,24 @@
 fn load_pikaci_run_bundle(
-    config: &Config,
+    store: Option<&PikaciRunStore>,
     run_id: &str,
 ) -> anyhow::Result<(RunRecord, RunLogsMetadata, Option<PreparedOutputsRecord>)> {
-    let forge_repo = config
-        .effective_forge_repo()
-        .ok_or_else(|| anyhow::anyhow!("forge repo is not configured"))?;
-    let state_root = forge::pikaci_state_root(&forge_repo);
-    let bundle = load_run_bundle(&state_root, run_id)?;
+    let bundle = require_pikaci_run_store(store)?.load_run_bundle(run_id)?;
     Ok((bundle.run, bundle.logs, bundle.prepared_outputs))
 }

Three private helper functions in web.rs — load_pikaci_run, load_pikaci_logs, and load_pikaci_run_bundle — each previously contained identical boilerplate:

Call config.effective_forge_repo() and map None to an error.
Compute forge::pikaci_state_root(&forge_repo).
Call the raw pikaci loading function.

All three are now replaced with a single-line pattern: require_pikaci_run_store(store)?.load_*(...). Their first parameter changes from &Config to Option<&PikaciRunStore>, and the crate::forge import is removed entirely from web.rs since it is no longer needed there.

Update handler call sites to pass the store

Intent: Thread the new PikaciRunStore from AppState through to each handler that loads pikaci data, replacing the previous config references.

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

Evidence

@@ -2610,7 +2609,9 @@ async fn api_forge_branch_logs_handler(
-                .and_then(|pikaci_run_id| load_pikaci_run_bundle(&state.config, pikaci_run_id).ok())
+                .and_then(|pikaci_run_id| {
+                    load_pikaci_run_bundle(state.pikaci_run_store.as_ref(), pikaci_run_id).ok()
+                })

@@ -2642,7 +2643,7 @@ async fn api_forge_pikaci_run_handler(
-    match load_pikaci_run(&state.config, &run_id) {
+    match load_pikaci_run(state.pikaci_run_store.as_ref(), &run_id) {

@@ -2662,7 +2663,7 @@ async fn api_forge_pikaci_logs_handler(
     match load_pikaci_logs(
-        &state.config,
+        state.pikaci_run_store.as_ref(),

@@ -2690,7 +2691,7 @@ async fn api_forge_pikaci_prepared_outputs_handler(
-    match load_pikaci_run_bundle(&state.config, &run_id) {
+    match load_pikaci_run_bundle(state.pikaci_run_store.as_ref(), &run_id) {

Four handler call sites are updated to pass state.pikaci_run_store.as_ref() instead of &state.config:

api_forge_branch_logs_handler — loads the run bundle for each lane's pikaci run.
api_forge_pikaci_run_handler — loads a single run record by ID.
api_forge_pikaci_logs_handler — loads logs for a run/job.
api_forge_pikaci_prepared_outputs_handler — loads the full bundle to extract prepared outputs.

The conversion from Option<PikaciRunStore> to Option<&PikaciRunStore> via .as_ref() avoids cloning the store on every request.

Clean up imports

Intent: Remove now-unused imports from web.rs since the forge module and raw pikaci loading functions are no longer referenced directly.

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

Evidence

@@ -16,10 +16,7 @@ use axum::Router;
-use pikaci::{
-    load_logs, load_run_bundle, load_run_record, LogKind, PreparedOutputsRecord, RunLogsMetadata,
-    RunRecord,
-};
+use pikaci::{LogKind, PreparedOutputsRecord, RunLogsMetadata, RunRecord};

@@ -34,7 +31,6 @@ use crate::ci_state::{
-use crate::forge;

The pikaci import is trimmed from four items (load_logs, load_run_bundle, load_run_record, LogKind) down to just the type re-exports (LogKind, PreparedOutputsRecord, RunLogsMetadata, RunRecord) since the loading functions are now called through PikaciRunStore. The use crate::forge line is removed entirely — web.rs no longer needs to know about state-root path computation.

Update test infrastructure to use PikaciRunStore

Intent: Align the test helpers and fixtures with the new abstraction so tests construct and use PikaciRunStore rather than manually computing paths from config.

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

Evidence

@@ -4232,6 +4221,8 @@ mod tests {
+    use crate::pikaci_store::PikaciRunStore;

@@ -4334,14 +4325,18 @@ mod tests {
     fn write_pikaci_run_fixture(config: &Config, run_id: &str) {
-        let forge_repo = config.effective_forge_repo().expect("forge repo");
-        let state_root = crate::forge::pikaci_state_root(&forge_repo);
+        let state_root = PikaciRunStore::from_config(config)
+            .expect("pikaci run store")
+            .state_root()
+            .to_path_buf();

@@ -4374,6 +4369,7 @@ mod tests {
+        let pikaci_run_store = PikaciRunStore::from_config(&config);
         Arc::new(AppState {
+            pikaci_run_store,

@@ -5764,8 +5761,9 @@ mod tests {
-        let forge_repo = config.effective_forge_repo().expect("forge repo");
-        let prepared_outputs_path = crate::forge::pikaci_state_root(&forge_repo)
+        let prepared_outputs_path = PikaciRunStore::from_config(&config)
+            .expect("pikaci run store")
+            .state_root()

The test module receives three changes:

write_pikaci_run_fixture now obtains the state root via PikaciRunStore::from_config(config).expect("pikaci run store").state_root() instead of manually calling config.effective_forge_repo() and forge::pikaci_state_root(). It also updates the guest log path to artifacts/guest.log with the necessary create_dir_all for the parent directory.
AppState construction in tests adds the pikaci_run_store field initialized from config, matching the production serve function.
Invalid prepared-outputs test uses PikaciRunStore to derive the path to the fixture file instead of the old manual approach.

These changes ensure tests exercise the same code paths as production, validating the new abstraction end-to-end.

pika-orch-incus-cleanup-10

CI: success

Summary

Tutorial Steps

Create the PikaciRunStore module

Add PikaciRunStore to AppState and initialize at startup

Refactor handler helpers to use PikaciRunStore

Update handler call sites to pass the store

Clean up imports

Update test infrastructure to use PikaciRunStore

Diff