From c6cafceae0ec348294c3ec7535412e40660338f2 Mon Sep 17 00:00:00 2001
From: Bryan Helmkamp <bryan@brynary.com>
Date: Mon, 20 Apr 2026 15:04:08 -0400
Subject: [PATCH] test: de-flake pr_list and full_http_lifecycle_cancel

Two CLI/server tests racing against peer state on the shared fabro server
session, surfaced by running the default nextest profile 20 times.

pr_list_missing_github_credentials_errors depended on an empty shared
store; if pr_view_reads_pull_request_from_store_without_pull_request_json
ran first it left a PR record behind and this test hit the
credentials-required branch instead of "No pull requests found." The
snapshot captured the empty path, but the test name promises the error
path. Seed a PullRequestCreated event against the test's own run so the
store is guaranteed non-empty and the credentials-required error fires
deterministically.

full_http_lifecycle_cancel asserted that the cancel response body's
pending_control == "cancel", but that field is re-read from the store
projection after the worker has been signaled. The worker is sitting at
a human gate; on hot CI it can emit a clearing event before the handler
re-reads the projection, yielding a legitimate null. Relax the
assertion to accept "cancel" or null; durable convergence to
failed/cancelled is still asserted below.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 lib/crates/fabro-cli/tests/it/cmd/pr_list.rs  | 59 ++++++++++++++++++-
 .../tests/it/scenario/lifecycle.rs            | 12 +++-
 2 files changed, 67 insertions(+), 4 deletions(-)

diff --git a/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs b/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs
index 66cd60308..9d85918af 100644
--- a/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs
+++ b/lib/crates/fabro-cli/tests/it/cmd/pr_list.rs
@@ -1,4 +1,13 @@
+#![allow(
+    clippy::absolute_paths,
+    reason = "This test module prefers explicit type paths over extra imports."
+)]
+
 use fabro_test::{fabro_snapshot, test_context};
+use fabro_types::run_event::PullRequestCreatedProps;
+use fabro_types::{EventBody, RunEvent, RunId};
+
+use super::support::{server_endpoint, setup_completed_fast_dry_run};
 
 #[test]
 fn help() {
@@ -26,17 +35,61 @@ fn help() {
     ");
 }
 
+// Seed a PR event against this test's own run so the store is guaranteed to
+// have at least one entry; `fabro pr list` then must load GitHub credentials
+// and fail, regardless of what peer tests have left in the shared store.
 #[test]
 fn pr_list_missing_github_credentials_errors() {
     let context = test_context!();
+    let run = setup_completed_fast_dry_run(&context);
+    let run_id: RunId = run.run_id.parse().unwrap();
+
+    let runtime = tokio::runtime::Runtime::new().unwrap();
+    runtime.block_on(async {
+        let (client, base_url) =
+            server_endpoint(&context.storage_dir).expect("server endpoint should exist");
+        let event = RunEvent {
+            id: ulid::Ulid::new().to_string(),
+            ts: chrono::Utc::now(),
+            run_id,
+            node_id: None,
+            node_label: None,
+            stage_id: None,
+            parallel_group_id: None,
+            parallel_branch_id: None,
+            session_id: None,
+            parent_session_id: None,
+            tool_call_id: None,
+            actor: None,
+            body: EventBody::PullRequestCreated(PullRequestCreatedProps {
+                pr_url:      "https://github.com/fabro-sh/fabro/pull/123".to_string(),
+                pr_number:   123,
+                owner:       "fabro-sh".to_string(),
+                repo:        "fabro".to_string(),
+                base_branch: "main".to_string(),
+                head_branch: "fabro/run/demo".to_string(),
+                title:       "Map the constellations".to_string(),
+                draft:       false,
+            }),
+        };
+        client
+            .post(format!("{base_url}/api/v1/runs/{run_id}/events"))
+            .json(&event)
+            .send()
+            .await
+            .unwrap()
+            .error_for_status()
+            .unwrap();
+    });
+
     let mut cmd = context.command();
     cmd.args(["pr", "list"]);
 
     fabro_snapshot!(context.filters(), cmd, @"
-    success: true
-    exit_code: 0
+    success: false
+    exit_code: 1
     ----- stdout -----
-    No pull requests found.
     ----- stderr -----
+    error: GitHub credentials required — run `fabro install` or set GITHUB_TOKEN
     ");
 }
diff --git a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs
index 2ec100f16..f04300075 100644
--- a/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs
+++ b/lib/crates/fabro-server/tests/it/scenario/lifecycle.rs
@@ -249,7 +249,17 @@ async fn full_http_lifecycle_cancel() {
     )
     .await;
     assert_eq!(body["status"], "running");
-    assert_eq!(body["pending_control"], "cancel");
+    // `pending_control` is computed from the store projection after the cancel
+    // event is appended AND the worker is signaled. The worker is sitting at a
+    // human gate; once notified it can emit a clearing event before this
+    // handler re-reads the projection, so the response can legitimately
+    // observe either the still-pending "cancel" or a null where the worker
+    // already consumed it. Durable convergence is asserted below.
+    let pending_control = &body["pending_control"];
+    assert!(
+        pending_control == "cancel" || pending_control.is_null(),
+        "expected pending_control to be \"cancel\" or null, got {pending_control}"
+    );
 
     // Verify the durable store view converges to cancelled failure.
     let body = wait_for_run_state(&app, &run_id, "failed", "cancelled").await;