From 0bf2dd30ef5a035ae25d1d9dcfdb2e21ddbf0af0 Mon Sep 17 00:00:00 2001
From: Bryan Helmkamp <bryan@brynary.com>
Date: Thu, 23 Apr 2026 22:45:29 -0400
Subject: [PATCH] refactor(server): collapse 409 conflict into standard
 ApiError
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace hand-built 409 response that inlined {errors,pull_request} with
a regular ApiError::with_code(CONFLICT, ..., "pull_request_exists") and
drop the optional pull_request field that had been added to the
ErrorResponse OpenAPI schema solely to carry the existing record.

Clients receiving a 409 can GET /runs/{id}/pull_request to retrieve the
stored record when they need it — the detail string still includes the
existing html_url, which is the field most clients branch on.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
---
 docs/api-reference/fabro-api.yaml             | 12 ++-----
 lib/crates/fabro-server/src/server.rs         | 31 +++++++------------
 .../src/models/error-response.ts              |  7 -----
 3 files changed, 13 insertions(+), 37 deletions(-)

diff --git a/docs/api-reference/fabro-api.yaml b/docs/api-reference/fabro-api.yaml
index 05f539e16..dd4e8ac27 100644
--- a/docs/api-reference/fabro-api.yaml
+++ b/docs/api-reference/fabro-api.yaml
@@ -949,9 +949,8 @@ paths:
                 $ref: "#/components/schemas/ErrorResponse"
         "409":
           description: >-
-            Pull request already exists for this run. The `pull_request` field
-            contains the existing record so clients can recover its URL and
-            number without an extra call.
+            Pull request already exists for this run. Clients can GET
+            /runs/{id}/pull_request to retrieve the stored record.
           content:
             application/json:
               schema:
@@ -3801,13 +3800,6 @@ components:
             failure responses only.
           items:
             type: string
-        pull_request:
-          allOf:
-            - $ref: "#/components/schemas/PullRequestRecord"
-          description: >-
-            Optional existing pull request record. Populated by `POST
-            /runs/{id}/pull_request` 409 responses so clients can recover the
-            stored record without a follow-up request.
 
     ActorKind:
       description: High-level category of an event actor.
diff --git a/lib/crates/fabro-server/src/server.rs b/lib/crates/fabro-server/src/server.rs
index ac67fff15..258fc0531 100644
--- a/lib/crates/fabro-server/src/server.rs
+++ b/lib/crates/fabro-server/src/server.rs
@@ -5069,22 +5069,12 @@ fn github_pull_request_not_found_error(record: &PullRequestRecord) -> ApiError {
     )
 }
 
-fn pull_request_already_exists_response(record: &PullRequestRecord) -> Response {
-    let status = StatusCode::CONFLICT;
-    let title = status.canonical_reason().unwrap_or("Conflict").to_string();
-    (
-        status,
-        Json(serde_json::json!({
-            "errors": [{
-                "status": status.as_u16().to_string(),
-                "title": title,
-                "detail": format!("Pull request already exists at {}", record.html_url),
-                "code": "pull_request_exists",
-            }],
-            "pull_request": record,
-        })),
+fn pull_request_already_exists_error(record: &PullRequestRecord) -> ApiError {
+    ApiError::with_code(
+        StatusCode::CONFLICT,
+        format!("Pull request already exists at {}", record.html_url),
+        "pull_request_exists",
     )
-        .into_response()
 }
 
 fn missing_repo_origin_error() -> ApiError {
@@ -5196,7 +5186,7 @@ async fn create_run_pull_request(
     };
 
     if let Some(record) = run_state.pull_request.as_ref() {
-        return pull_request_already_exists_response(record);
+        return pull_request_already_exists_error(record).into_response();
     }
 
     let Some(run_spec) = run_state.spec.as_ref() else {
@@ -9662,10 +9652,11 @@ slug = "fabro"
         let body = response_json!(response, StatusCode::CONFLICT).await;
 
         assert_eq!(body["errors"][0]["code"], "pull_request_exists");
-        assert_eq!(body["pull_request"]["number"], 42);
-        assert_eq!(
-            body["pull_request"]["html_url"],
-            "https://github.com/acme/widgets/pull/42"
+        assert!(
+            body["errors"][0]["detail"]
+                .as_str()
+                .unwrap()
+                .contains("https://github.com/acme/widgets/pull/42")
         );
     }
 
diff --git a/lib/packages/fabro-api-client/src/models/error-response.ts b/lib/packages/fabro-api-client/src/models/error-response.ts
index a7c8d2bae..ee0d45ad1 100644
--- a/lib/packages/fabro-api-client/src/models/error-response.ts
+++ b/lib/packages/fabro-api-client/src/models/error-response.ts
@@ -16,9 +16,6 @@
 // May contain unused imports in some cases
 // @ts-ignore
 import type { ErrorResponseEntry } from './error-response-entry';
-// May contain unused imports in some cases
-// @ts-ignore
-import type { PullRequestRecord } from './pull-request-record';
 
 /**
  * Standard error response containing one or more error entries.
@@ -36,9 +33,5 @@ export interface ErrorResponse {
      * Optional list of runtime env keys that were actually removed before an install failure. Currently populated by `POST /install/finish` failure responses only.
      */
     'removed_env_keys'?: Array<string>;
-    /**
-     * Optional existing pull request record. Populated by `POST /runs/{id}/pull_request` 409 responses so clients can recover the stored record without a follow-up request.
-     */
-    'pull_request'?: PullRequestRecord;
 }