From acb6b3f9d67c09352927c9357b61ba4c55889e58 Mon Sep 17 00:00:00 2001 From: Bryan Helmkamp Date: Sun, 19 Apr 2026 13:48:31 -0400 Subject: [PATCH] refactor(install): tag GithubAppOwner with discriminated object shape The install GitHub App manifest shape encoded owner as `"personal"` or `"org:"` - a magic string parsed in install-app.tsx, built by install-api.ts, and reparsed server-side. Replace with a tagged object `{ kind: "personal" } | { kind: "org", slug }` in the OpenAPI spec, the progenitor-generated Rust types, and the frontend. Server-side, the internal `GitHubAppOwner` enum keeps its semantic shape but gains a `TryFrom` conversion and emits the tagged JSON via `as_session_value`. Frontend drops `buildGithubOwnerValue` in favor of `buildInstallGithubAppOwner`, and the ready-screen renders the owner through a small helper instead of string concatenation. Co-Authored-By: Claude Opus 4.7 (1M context) --- apps/fabro-web/app/install-api.test.ts | 15 +++-- apps/fabro-web/app/install-api.ts | 16 ++++-- apps/fabro-web/app/install-app.tsx | 20 +++++-- docs/api-reference/fabro-api.yaml | 19 +++++-- lib/crates/fabro-server/src/install.rs | 55 +++++++++++++------ .../fabro-server/tests/it/api/install.rs | 14 +++-- 6 files changed, 93 insertions(+), 46 deletions(-) diff --git a/apps/fabro-web/app/install-api.test.ts b/apps/fabro-web/app/install-api.test.ts index d920beee9..2fbf09518 100644 --- a/apps/fabro-web/app/install-api.test.ts +++ b/apps/fabro-web/app/install-api.test.ts @@ -1,6 +1,6 @@ import { describe, expect, test } from "bun:test"; -import { buildGithubOwnerValue, readInstallError } from "./install-api"; +import { buildInstallGithubAppOwner, readInstallError } from "./install-api"; describe("readInstallError", () => { test("prefers the structured install error payload", async () => { @@ -31,12 +31,15 @@ describe("readInstallError", () => { }); }); -describe("buildGithubOwnerValue", () => { - test("uses personal for personal app installs", () => { - expect(buildGithubOwnerValue("personal", "")).toBe("personal"); +describe("buildInstallGithubAppOwner", () => { + test("uses the personal tag for personal app installs", () => { + expect(buildInstallGithubAppOwner("personal", "")).toEqual({ kind: "personal" }); }); - test("formats organization owners with the expected prefix", () => { - expect(buildGithubOwnerValue("org", " acme ")).toBe("org:acme"); + test("trims organization slugs and tags them with `org`", () => { + expect(buildInstallGithubAppOwner("org", " acme ")).toEqual({ + kind: "org", + slug: "acme", + }); }); }); diff --git a/apps/fabro-web/app/install-api.ts b/apps/fabro-web/app/install-api.ts index 328080d29..1fd326d28 100644 --- a/apps/fabro-web/app/install-api.ts +++ b/apps/fabro-web/app/install-api.ts @@ -1,3 +1,7 @@ +export type InstallGithubAppOwner = + | { kind: "personal" } + | { kind: "org"; slug: string }; + export interface InstallSessionResponse { completed_steps: string[]; llm: @@ -13,7 +17,7 @@ export interface InstallSessionResponse { | { strategy: string; username?: string; - owner?: string; + owner?: InstallGithubAppOwner; app_name?: string; slug?: string; allowed_username?: string; @@ -34,7 +38,7 @@ export interface InstallLlmProviderInput { } export interface InstallGithubAppManifestInput { - owner: string; + owner: InstallGithubAppOwner; app_name: string; allowed_username: string; } @@ -92,13 +96,13 @@ export async function readInstallError( return `${fallback} (${response.status})`; } -export function buildGithubOwnerValue( +export function buildInstallGithubAppOwner( ownerKind: "personal" | "org", organizationSlug: string, -): string { +): InstallGithubAppOwner { return ownerKind === "org" - ? `org:${organizationSlug.trim()}` - : "personal"; + ? { kind: "org", slug: organizationSlug.trim() } + : { kind: "personal" }; } export async function getInstallSession(token: string): Promise { diff --git a/apps/fabro-web/app/install-app.tsx b/apps/fabro-web/app/install-app.tsx index 27836b4cb..12bc7fefc 100644 --- a/apps/fabro-web/app/install-app.tsx +++ b/apps/fabro-web/app/install-app.tsx @@ -4,9 +4,10 @@ import { Link, Navigate, useLocation, useNavigate } from "react-router"; import { type InstallFinishResponse, + type InstallGithubAppOwner, type InstallLlmProviderInput, type InstallSessionResponse, - buildGithubOwnerValue, + buildInstallGithubAppOwner, createInstallGithubAppManifest, finishInstall, getInstallSession, @@ -116,10 +117,10 @@ export default function InstallApp() { ); if (nextSession.github?.strategy === "app") { setGithubStrategy("app"); - const owner = nextSession.github.owner ?? "personal"; - if (owner.startsWith("org:")) { + const owner = nextSession.github.owner ?? { kind: "personal" }; + if (owner.kind === "org") { setGithubOwnerKind("org"); - setGithubOrganization(owner.slice(4)); + setGithubOrganization(owner.slug); } else { setGithubOwnerKind("personal"); setGithubOrganization(""); @@ -389,7 +390,7 @@ export default function InstallApp() { } const manifest = await createInstallGithubAppManifest(installToken, { - owner: buildGithubOwnerValue(githubOwnerKind, githubOrganization), + owner: buildInstallGithubAppOwner(githubOwnerKind, githubOrganization), app_name: githubAppName.trim(), allowed_username: githubAllowedUsername.trim(), }); @@ -970,7 +971,7 @@ function GithubAppDoneScreen({

- + , diff --git a/docs/api-reference/fabro-api.yaml b/docs/api-reference/fabro-api.yaml index 85ad523ed..61568b532 100644 --- a/docs/api-reference/fabro-api.yaml +++ b/docs/api-reference/fabro-api.yaml @@ -2111,14 +2111,25 @@ components: - allowed_username properties: owner: - type: string - description: > - Either `personal` or `org:`. + $ref: "#/components/schemas/InstallGithubAppOwner" app_name: type: string allowed_username: type: string + InstallGithubAppOwner: + description: Owner of the GitHub App being created during browser install. + type: object + required: + - kind + properties: + kind: + type: string + enum: [personal, org] + slug: + type: string + description: Required when `kind` is `org`; the organization slug. + InstallGithubAppManifestResponse: description: Browser handoff payload for the GitHub App creation flow. type: object @@ -2145,7 +2156,7 @@ components: username: type: string owner: - type: string + $ref: "#/components/schemas/InstallGithubAppOwner" app_name: type: string slug: diff --git a/lib/crates/fabro-server/src/install.rs b/lib/crates/fabro-server/src/install.rs index 36c3f6124..34e89e21b 100644 --- a/lib/crates/fabro-server/src/install.rs +++ b/lib/crates/fabro-server/src/install.rs @@ -227,11 +227,24 @@ struct GithubTokenTestInput { #[derive(Clone, Debug, Deserialize)] struct GithubAppManifestInput { - owner: String, + owner: GithubAppOwnerInput, app_name: String, allowed_username: String, } +#[derive(Clone, Debug, Deserialize)] +struct GithubAppOwnerInput { + kind: GithubAppOwnerKind, + slug: Option, +} + +#[derive(Clone, Copy, Debug, Deserialize, PartialEq, Eq)] +#[serde(rename_all = "lowercase")] +enum GithubAppOwnerKind { + Personal, + Org, +} + #[derive(Clone, Debug, Deserialize)] struct GithubAppRedirectQuery { code: Option, @@ -260,18 +273,6 @@ enum GitHubAppOwner { } impl GitHubAppOwner { - fn parse(raw: &str) -> anyhow::Result { - let value = raw.trim(); - if value.eq_ignore_ascii_case("personal") { - return Ok(Self::Personal); - } - if let Some(org) = value.strip_prefix("org:") { - anyhow::ensure!(!org.trim().is_empty(), "organization owner cannot be empty"); - return Ok(Self::Organization(org.trim().to_string())); - } - anyhow::bail!("owner must be 'personal' or 'org:'"); - } - fn manifest_form_action(&self) -> String { match self { Self::Personal => "https://github.com/settings/apps/new".to_string(), @@ -281,10 +282,28 @@ impl GitHubAppOwner { } } - fn as_session_value(&self) -> String { + fn as_session_value(&self) -> serde_json::Value { match self { - Self::Personal => "personal".to_string(), - Self::Organization(org) => format!("org:{org}"), + Self::Personal => serde_json::json!({ "kind": "personal" }), + Self::Organization(org) => serde_json::json!({ "kind": "org", "slug": org }), + } + } +} + +impl TryFrom for GitHubAppOwner { + type Error = String; + + fn try_from(value: GithubAppOwnerInput) -> Result { + match value.kind { + GithubAppOwnerKind::Personal => Ok(Self::Personal), + GithubAppOwnerKind::Org => { + let slug = value.slug.unwrap_or_default(); + let trimmed = slug.trim(); + if trimmed.is_empty() { + return Err("organization owner requires a non-empty slug".to_string()); + } + Ok(Self::Organization(trimmed.to_string())) + } } } } @@ -605,10 +624,10 @@ async fn post_install_github_app_manifest( } observe_operator(&state, &headers); - let owner = match GitHubAppOwner::parse(&input.owner) { + let owner = match GitHubAppOwner::try_from(input.owner) { Ok(owner) => owner, Err(err) => { - return install_error_response(StatusCode::UNPROCESSABLE_ENTITY, err.to_string()); + return install_error_response(StatusCode::UNPROCESSABLE_ENTITY, err); } }; if input.app_name.trim().is_empty() { diff --git a/lib/crates/fabro-server/tests/it/api/install.rs b/lib/crates/fabro-server/tests/it/api/install.rs index f0f21b739..e1aaa7c56 100644 --- a/lib/crates/fabro-server/tests/it/api/install.rs +++ b/lib/crates/fabro-server/tests/it/api/install.rs @@ -197,7 +197,9 @@ async fn install_endpoints_reject_missing_and_wrong_tokens() { ( "POST", "/install/github/app/manifest", - Some(r#"{"owner":"personal","app_name":"Fabro","allowed_username":"octocat"}"#), + Some( + r#"{"owner":{"kind":"personal"},"app_name":"Fabro","allowed_username":"octocat"}"#, + ), ), ("POST", "/install/finish", None), ]; @@ -539,7 +541,7 @@ async fn github_app_manifest_round_trip_updates_install_session() { .header("authorization", "Bearer test-install-token") .header("content-type", "application/json") .body(Body::from( - r#"{"owner":"personal","app_name":"Fabro Test","allowed_username":"octocat"}"#, + r#"{"owner":{"kind":"personal"},"app_name":"Fabro Test","allowed_username":"octocat"}"#, )) .unwrap(), ) @@ -661,7 +663,7 @@ async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_tok .header("authorization", "Bearer test-install-token") .header("content-type", "application/json") .body(Body::from( - r#"{"owner":"personal","app_name":"Fabro Test","allowed_username":"octocat"}"#, + r#"{"owner":{"kind":"personal"},"app_name":"Fabro Test","allowed_username":"octocat"}"#, )) .unwrap(), ) @@ -701,7 +703,7 @@ async fn github_app_manifest_rejects_retry_while_pending_and_preserves_prior_tok .header("authorization", "Bearer test-install-token") .header("content-type", "application/json") .body(Body::from( - r#"{"owner":"personal","app_name":"Fabro Retry","allowed_username":"octocat"}"#, + r#"{"owner":{"kind":"personal"},"app_name":"Fabro Retry","allowed_username":"octocat"}"#, )) .unwrap(), ) @@ -767,7 +769,7 @@ async fn github_app_redirect_rejects_invalid_or_missing_state_without_mutating_s .header("authorization", "Bearer test-install-token") .header("content-type", "application/json") .body(Body::from( - r#"{"owner":"personal","app_name":"Fabro Test","allowed_username":"octocat"}"#, + r#"{"owner":{"kind":"personal"},"app_name":"Fabro Test","allowed_username":"octocat"}"#, )) .unwrap(), ) @@ -907,7 +909,7 @@ async fn github_app_redirect_exchange_failure_returns_to_wizard_and_keeps_pendin .header("authorization", "Bearer test-install-token") .header("content-type", "application/json") .body(Body::from( - r#"{"owner":"personal","app_name":"Fabro Test","allowed_username":"octocat"}"#, + r#"{"owner":{"kind":"personal"},"app_name":"Fabro Test","allowed_username":"octocat"}"#, )) .unwrap(), )