From aeebdd2f3fb5e5dc1a10e20b38384658b8f61870 Mon Sep 17 00:00:00 2001
From: MechaCat02 <fabian@diekaulbachs.de>
Date: Sun, 12 Jul 2026 19:08:21 +0200
Subject: [PATCH] fix(workflows): close re-review gaps in the pull round-trip +
 partial-failure UI
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

A re-review of eaf5ace surfaced three follow-ups, all in the fixes themselves:

- pull vs an old server: a returned empty `definition` can only mean a server
  predating the definition-in-list field (a real zero-step workflow is rejected
  at apply). Emitting a stepless `[[workflows]]` block would just fail the next
  apply, so `wire_workflow_to_manifest` now warns and skips it instead.
- base_ms/backoff defaults: the pull mapper keyed its default-omission off a
  hardcoded `500` literal. `default_base_ms` is now `pub` + re-exported, and the
  mapper omits whatever equals `default_base_ms()` / `WorkflowBackoff::default()`
  — no drift if a default ever changes.
- dashboard: a run that succeeds with an `on_error = continue` step failure now
  carries a partial-failure note in `run.error` (from the compute_advance
  change); the run-detail view rendered it as a red failure, contradicting the
  green "succeeded" badge. It now renders as a `.notice` (warning) for a
  succeeded run, `.error` only for a failed one.

Verified: fmt + clippy -D warnings clean; 450 lib tests; 5 workflow CLI
journeys (incl. the pull→plan round-trip); dashboard npm run check clean.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
---
 crates/picloud-cli/src/cmds/pull.rs           | 38 ++++++++++++-------
 crates/shared/src/lib.rs                      |  4 +-
 crates/shared/src/workflow.rs                 |  6 ++-
 .../routes/apps/[slug]/workflows/+page.svelte | 10 ++++-
 4 files changed, 40 insertions(+), 18 deletions(-)

diff --git a/crates/picloud-cli/src/cmds/pull.rs b/crates/picloud-cli/src/cmds/pull.rs
index 7993330..1a7d043 100644
--- a/crates/picloud-cli/src/cmds/pull.rs
+++ b/crates/picloud-cli/src/cmds/pull.rs
@@ -253,7 +253,10 @@ pub async fn run(app_ident: &str, dir: &Path, force: bool, mode: OutputMode) ->
         },
         vars: manifest_vars,
         suppress: crate::manifest::ManifestSuppress::default(),
-        workflows: workflows.iter().map(wire_workflow_to_manifest).collect(),
+        workflows: workflows
+            .iter()
+            .filter_map(wire_workflow_to_manifest)
+            .collect(),
     };
 
     std::fs::write(&manifest_path, manifest.to_toml()?)
@@ -280,8 +283,21 @@ fn trigger_count(t: &ManifestTriggers) -> usize {
 /// (the inverse of `plan::workflow_to_wire`). Without this a `pic pull` would
 /// drop the app's workflows, and a subsequent `pic apply --prune` would delete
 /// them from the server.
-fn wire_workflow_to_manifest(w: &crate::client::WorkflowDto) -> ManifestWorkflow {
-    ManifestWorkflow {
+///
+/// Returns `None` (with a warning) when the definition came back empty. A
+/// zero-step workflow is rejected at apply, so an empty `definition` can only
+/// mean the server predates the definition-in-list field (version skew) —
+/// emitting a stepless block would just fail the next apply, so skip it.
+fn wire_workflow_to_manifest(w: &crate::client::WorkflowDto) -> Option<ManifestWorkflow> {
+    if w.definition.steps.is_empty() {
+        eprintln!(
+            "warning: skipping workflow `{}` — the server did not return its \
+             definition (upgrade the server to round-trip workflows via pull)",
+            w.name
+        );
+        return None;
+    }
+    Some(ManifestWorkflow {
         name: w.name.clone(),
         enabled: w.enabled,
         steps: w
@@ -290,7 +306,7 @@ fn wire_workflow_to_manifest(w: &crate::client::WorkflowDto) -> ManifestWorkflow
             .iter()
             .map(wire_step_to_manifest)
             .collect(),
-    }
+    })
 }
 
 fn wire_step_to_manifest(s: &picloud_shared::WorkflowStepDef) -> ManifestWorkflowStep {
@@ -321,16 +337,10 @@ fn wire_step_to_manifest(s: &picloud_shared::WorkflowStepDef) -> ManifestWorkflo
         when: s.when.clone(),
         retry: s.retry.as_ref().map(|r| ManifestWorkflowRetry {
             max_attempts: r.max_attempts,
-            // Exponential is the default → omit; likewise the default base_ms.
-            backoff: match r.backoff {
-                picloud_shared::WorkflowBackoff::Exponential => None,
-                other => Some(other),
-            },
-            base_ms: if r.base_ms == 500 {
-                None
-            } else {
-                Some(r.base_ms)
-            },
+            // Omit whatever equals the server default (so the round-trip is a
+            // no-op), keyed off the shared defaults rather than local literals.
+            backoff: (r.backoff != picloud_shared::WorkflowBackoff::default()).then_some(r.backoff),
+            base_ms: (r.base_ms != picloud_shared::default_base_ms()).then_some(r.base_ms),
         }),
         // Fail is the default → omit; Continue is explicit.
         on_error: match s.on_error {
diff --git a/crates/shared/src/lib.rs b/crates/shared/src/lib.rs
index 21251be..4845fcf 100644
--- a/crates/shared/src/lib.rs
+++ b/crates/shared/src/lib.rs
@@ -121,6 +121,6 @@ pub use validator::{ScriptValidator, ValidatedScript, ValidationError};
 pub use vars::{NoopVarsService, VarsError, VarsService};
 pub use version::{API_VERSION, PRODUCT_VERSION, SDK_VERSION, WIRE_VERSION};
 pub use workflow::{
-    NoopWorkflowService, OnError, RunStatus, StepStatus, WorkflowBackoff, WorkflowDefinition,
-    WorkflowError, WorkflowRetry, WorkflowService, WorkflowStepDef,
+    default_base_ms, NoopWorkflowService, OnError, RunStatus, StepStatus, WorkflowBackoff,
+    WorkflowDefinition, WorkflowError, WorkflowRetry, WorkflowService, WorkflowStepDef,
 };
diff --git a/crates/shared/src/workflow.rs b/crates/shared/src/workflow.rs
index ec83279..92eba2f 100644
--- a/crates/shared/src/workflow.rs
+++ b/crates/shared/src/workflow.rs
@@ -158,7 +158,11 @@ pub enum WorkflowBackoff {
     Exponential,
 }
 
-fn default_base_ms() -> u32 {
+/// Default per-step retry base delay (ms). Public so the CLI can invert the
+/// pull round-trip (omit `base_ms` when it equals this) without duplicating the
+/// literal.
+#[must_use]
+pub fn default_base_ms() -> u32 {
     500
 }
 
diff --git a/dashboard/src/routes/apps/[slug]/workflows/+page.svelte b/dashboard/src/routes/apps/[slug]/workflows/+page.svelte
index 09789a9..253ba21 100644
--- a/dashboard/src/routes/apps/[slug]/workflows/+page.svelte
+++ b/dashboard/src/routes/apps/[slug]/workflows/+page.svelte
@@ -230,7 +230,12 @@
 			<span class="badge st-{detail.run.status}">{detail.run.status}</span>
 		</h3>
 		{#if detail.run.error}
-			<p class="error">{detail.run.error}</p>
+			<!-- A succeeded run can still carry an `error` note when a step failed
+			     under `on_error = continue` — surface it as a warning, not a
+			     red failure, so the green badge and message don't contradict. -->
+			<p class={detail.run.status === 'succeeded' ? 'notice' : 'error'}>
+				{detail.run.error}
+			</p>
 		{/if}
 
 		<h4>DAG</h4>
@@ -341,6 +346,9 @@
 	.error {
 		color: var(--color-danger);
 	}
+	.notice {
+		color: var(--color-warning, var(--text-muted));
+	}
 	.muted {
 		color: var(--text-muted);
 	}