From 5ae1bab4ee0ecf432e2df87894e0394011dcb0e6 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 17 Sep 2026 22:51:07 +0900 Subject: [PATCH 01/64] Improve Objective list and detail display --- web/workspace/deno.json | 2 +- .../lib/workspace/styles/workspace-pages.css | 59 +++++++++++++++-- .../w/[workspaceId]/objectives/+page.svelte | 1 - .../objectives/[objectiveId]/+page.svelte | 48 +++++--------- .../objectives/[objectiveId]/+page.ts | 22 +++---- web/workspace/test/objective-pages.test.ts | 65 +++++++++++++++++++ 6 files changed, 144 insertions(+), 53 deletions(-) create mode 100644 web/workspace/test/objective-pages.test.ts diff --git a/web/workspace/deno.json b/web/workspace/deno.json index 0887fc85e..58759ffb6 100644 --- a/web/workspace/deno.json +++ b/web/workspace/deno.json @@ -6,7 +6,7 @@ "dev": "deno run -A npm:vite@7.2.7 dev", "dev:backend": "cd ../.. && cargo run -p yoi-workspace-server --bin yoi-server -- serve --listen 127.0.0.1:8787", "check": "deno run -A npm:@sveltejs/kit@2.49.4 sync && deno run -A npm:svelte-check@4.3.4 --tsconfig ./tsconfig.json", - "test": "deno test --allow-read=src,test,tests --allow-env=LOG,VSCODE_TEXTMATE_DEBUG,NODE_ENV tests/workspace-model.test.ts tests/workspace-catalog.test.ts tests/profile-api.test.ts tests/skill-api.test.ts src/lib/workspace/auth/model.test.ts tests/auth-api.test.ts src/lib/workspace/api/http.test.ts src/lib/workspace/api/workers.test.ts src/lib/workspace/header/breadcrumb-model.test.ts src/lib/workspace/console/chat-submit.test.ts src/lib/workspace/console/composer-delivery.test.ts test/composer-history.test.ts tests/composer-paste.test.ts src/lib/workspace/console/composer-command.test.ts src/lib/workspace/console/composer-draft.test.ts src/lib/workspace/console/composer-completion.test.ts src/lib/workspace/console/markdown.test.ts test/console/ansi.test.ts src/lib/workspace/console/model.test.ts src/lib/workspace/companion/api.test.ts tests/workdir-api.test.ts src/lib/workspace/console/tasks.test.ts test/ticket-detail-route-reuse.test.ts test/repositories/ui.test.ts src/lib/workspace/console/worker-console.ui.test.ts src/lib/workspace/settings/model.test.ts tests/runtime-connection.test.ts tests/runtime-management.test.ts tests/runtime-management-source.test.ts src/lib/workspace/sidebar/override-stack.test.ts src/lib/workspace/sidebar/workers.test.ts src/lib/workspace/sidebar/workspace-switcher.test.ts src/lib/workspace/sidebar/worker-subscription.test.ts src/lib/workspace/sidebar/worker-launch.test.ts test/sidebar/worker-actions.test.ts src/lib/workspace/tickets/merge-request-resources.test.ts src/lib/workspace/tickets/ticket-panel.test.ts test/merge-request-status.test.ts test/config-source/decodal-grammar.test.ts test/config-source/editor-state.test.ts test/config-source/fixed-schema-wrapper.test.ts test/config-source/toolchain.test.ts test/config-source/wasm-parity.test.ts test/repository-access/api.test.ts test/repository-access/loader.test.ts test/repository-access/ui.test.ts test/repository-ssh-connection-ui.test.ts", + "test": "deno test --allow-read=src,test,tests --allow-env=LOG,VSCODE_TEXTMATE_DEBUG,NODE_ENV tests/workspace-model.test.ts tests/workspace-catalog.test.ts tests/profile-api.test.ts tests/skill-api.test.ts src/lib/workspace/auth/model.test.ts tests/auth-api.test.ts src/lib/workspace/api/http.test.ts src/lib/workspace/api/workers.test.ts src/lib/workspace/header/breadcrumb-model.test.ts src/lib/workspace/console/chat-submit.test.ts src/lib/workspace/console/composer-delivery.test.ts test/composer-history.test.ts tests/composer-paste.test.ts src/lib/workspace/console/composer-command.test.ts src/lib/workspace/console/composer-draft.test.ts src/lib/workspace/console/composer-completion.test.ts src/lib/workspace/console/markdown.test.ts test/console/ansi.test.ts src/lib/workspace/console/model.test.ts src/lib/workspace/companion/api.test.ts tests/workdir-api.test.ts src/lib/workspace/console/tasks.test.ts test/ticket-detail-route-reuse.test.ts test/objective-pages.test.ts test/repositories/ui.test.ts src/lib/workspace/console/worker-console.ui.test.ts src/lib/workspace/settings/model.test.ts tests/runtime-connection.test.ts tests/runtime-management.test.ts tests/runtime-management-source.test.ts src/lib/workspace/sidebar/override-stack.test.ts src/lib/workspace/sidebar/workers.test.ts src/lib/workspace/sidebar/workspace-switcher.test.ts src/lib/workspace/sidebar/worker-subscription.test.ts src/lib/workspace/sidebar/worker-launch.test.ts test/sidebar/worker-actions.test.ts src/lib/workspace/tickets/merge-request-resources.test.ts src/lib/workspace/tickets/ticket-panel.test.ts test/merge-request-status.test.ts test/config-source/decodal-grammar.test.ts test/config-source/editor-state.test.ts test/config-source/fixed-schema-wrapper.test.ts test/config-source/toolchain.test.ts test/config-source/wasm-parity.test.ts test/repository-access/api.test.ts test/repository-access/loader.test.ts test/repository-access/ui.test.ts test/repository-ssh-connection-ui.test.ts", "build": "deno run -A npm:vite@7.2.7 build", "preview": "deno run -A npm:vite@7.2.7 preview" }, diff --git a/web/workspace/src/lib/workspace/styles/workspace-pages.css b/web/workspace/src/lib/workspace/styles/workspace-pages.css index c599c7962..01ff534f8 100644 --- a/web/workspace/src/lib/workspace/styles/workspace-pages.css +++ b/web/workspace/src/lib/workspace/styles/workspace-pages.css @@ -84,8 +84,7 @@ gap: var(--space-2); min-width: 0; } - .ticket-title, - .objective-title { + .ticket-title { min-width: 0; overflow: hidden; color: var(--text-strong); @@ -93,6 +92,14 @@ text-overflow: ellipsis; white-space: nowrap; } + .objective-title { + min-width: 0; + flex: 1 1 auto; + color: var(--text-strong); + font-weight: 700; + overflow-wrap: anywhere; + white-space: normal; + } .state-pill { flex: 0 0 auto; color: var(--success); @@ -101,8 +108,7 @@ letter-spacing: 0.05em; text-transform: uppercase; } - .ticket-summary, - .objective-summary { + .ticket-summary { margin: 0; color: var(--text-muted); font-size: 0.88rem; @@ -133,6 +139,51 @@ white-space: normal; } } + .objective-title-row { + align-items: flex-start; + } + .objective-title-row.detail h3 { + min-width: 0; + margin: 0; + overflow-wrap: anywhere; + } + .objective-detail-card { + display: grid; + gap: var(--space-4); + } + .objective-detail-card > h2 { + margin: 0; + } + .objective-back-link { + width: fit-content; + color: var(--text-muted); + font-size: 0.85rem; + } + .objective-detail-metadata { + padding-bottom: var(--space-4); + border-bottom: 1px solid var(--line); + } + .objective-detail-metadata dd { + overflow-wrap: anywhere; + } + .objective-body { + min-width: 0; + } + .objective-body > :first-child { + margin-top: 0; + } + .objective-body > :last-child { + margin-bottom: 0; + } + @media (max-width: 600px) { + .objective-title-row.detail { + flex-wrap: wrap; + } + .objective-detail-metadata > div { + grid-template-columns: minmax(0, 1fr); + gap: var(--space-1); + } + } .runtime-heading, .detail-heading { display: flex; diff --git a/web/workspace/src/routes/w/[workspaceId]/objectives/+page.svelte b/web/workspace/src/routes/w/[workspaceId]/objectives/+page.svelte index 4b7a14f37..6eef837ea 100644 --- a/web/workspace/src/routes/w/[workspaceId]/objectives/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/objectives/+page.svelte @@ -25,7 +25,6 @@ {objective.title} {objective.state} -

{objective.summary || 'No summary text is available.'}

Updated {objective.updated_at ? formatDate(objective.updated_at) : 'unknown'} diff --git a/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.svelte b/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.svelte index dff8b622a..162928f51 100644 --- a/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.svelte @@ -1,52 +1,28 @@ {data.objective?.title ?? data.objectiveId} · Objective -
-

Objectives

- {#if data.objectives} -
- {#each data.objectives.items as objective (objective.id)} - -
-
- {objective.title} - {objective.state} -
-

{objective.summary || 'No summary text is available.'}

-
-
- Updated {objective.updated_at ? formatDate(objective.updated_at) : 'unknown'} - {objective.resource_key} -
- - {/each} -
- {:else if data.objectivesError} -

{data.objectivesError}

- {:else} -

Loading objectives…

- {/if} -
-
+ ← All Objectives

Objective detail

{#if data.objective}
-
-

{data.objective.title}

-
+

{data.objective.title}

{data.objective.state}
-
+
Created
{data.objective.created_at ? formatDate(data.objective.created_at) : 'unknown'}
@@ -72,7 +48,13 @@
- 
{data.objective.body}
+
+ {#if data.objective.body} + + {:else} +

No Objective body is available.

+ {/if} +
{#if data.objective.body_truncated}

Objective body was truncated by the Backend response limit.

{/if} diff --git a/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.ts b/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.ts index bf4b1592f..e822b5a9a 100644 --- a/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.ts +++ b/web/workspace/src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.ts @@ -4,22 +4,16 @@ import { canonicalResourceReference, resourceKey, } from "$lib/workspace/resource-links"; -import type { - ObjectiveDetail, - ObjectiveListResponse, -} from "$lib/workspace/sidebar/types"; +import type { ObjectiveDetail } from "$lib/workspace/sidebar/types"; import type { PageLoad } from "./$types"; export const load: PageLoad = async ({ fetch, params }) => { const apiPath = (path: string) => workspaceApiPath(params.workspaceId, path); const objectiveId = resourceKey(params.objectiveId); - const [objectives, objective] = await Promise.all([ - loadJson(fetch, apiPath("/objectives")), - loadJson( - fetch, - apiPath(`/objectives/${encodeURIComponent(objectiveId)}`), - ), - ]); + const objective = await loadJson( + fetch, + apiPath(`/objectives/${encodeURIComponent(objectiveId)}`), + ); if (objective.data) { const canonical = canonicalResourceReference( @@ -29,7 +23,9 @@ export const load: PageLoad = async ({ fetch, params }) => { if (params.objectiveId !== canonical) { redirect( 308, - `/w/${encodeURIComponent(params.workspaceId)}/objectives/${encodeURIComponent(canonical)}`, + `/w/${encodeURIComponent(params.workspaceId)}/objectives/${ + encodeURIComponent(canonical) + }`, ); } } @@ -37,8 +33,6 @@ export const load: PageLoad = async ({ fetch, params }) => { return { workspaceId: params.workspaceId, objectiveId, - objectives: objectives.data, - objectivesError: objectives.error, objective: objective.data, objectiveError: objective.error, }; diff --git a/web/workspace/test/objective-pages.test.ts b/web/workspace/test/objective-pages.test.ts new file mode 100644 index 000000000..319b84f81 --- /dev/null +++ b/web/workspace/test/objective-pages.test.ts @@ -0,0 +1,65 @@ +import { + assert, + assertFalse, + assertMatch, + assertStringIncludes, +} from "jsr:@std/assert"; + +const objectivesListSource = await Deno.readTextFile( + new URL( + "../src/routes/w/[workspaceId]/objectives/+page.svelte", + import.meta.url, + ), +); +const objectiveDetailSource = await Deno.readTextFile( + new URL( + "../src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.svelte", + import.meta.url, + ), +); +const objectiveDetailLoaderSource = await Deno.readTextFile( + new URL( + "../src/routes/w/[workspaceId]/objectives/[objectiveId]/+page.ts", + import.meta.url, + ), +); +const workspaceStyles = await Deno.readTextFile( + new URL("../src/lib/workspace/styles/workspace-pages.css", import.meta.url), +); + +Deno.test("Objective list shows complete titles without body summaries", () => { + assertStringIncludes(objectivesListSource, "{objective.title}"); + assertFalse(objectivesListSource.includes("objective.summary")); + assertMatch( + workspaceStyles, + /\.objective-title\s*\{[^}]*overflow-wrap:\s*anywhere;[^}]*white-space:\s*normal;/s, + ); +}); + +Deno.test("Objective detail focuses on one Markdown-rendered Objective", () => { + assertStringIncludes( + objectiveDetailSource, + "", + ); + assertFalse(objectiveDetailSource.includes('class="objective-list')); + assertFalse(objectiveDetailSource.includes("data.objectives")); + assertStringIncludes(objectiveDetailSource, "linked_ticket_summaries"); + assertStringIncludes(objectiveDetailSource, "body_truncated"); +}); + +Deno.test("Objective detail loader fetches only the selected Objective", () => { + assertStringIncludes( + objectiveDetailLoaderSource, + "`/objectives/${encodeURIComponent(objectiveId)}`", + ); + assertFalse( + objectiveDetailLoaderSource.includes('apiPath("/objectives")'), + ); + assertFalse( + objectiveDetailLoaderSource.includes("ObjectiveListResponse"), + ); + assert( + objectiveDetailLoaderSource.includes("canonicalResourceReference"), + "canonical Objective redirects must remain intact", + ); +}); From 9590cf332dc72bca011e2ec3730f37a13479f6e6 Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 17 Sep 2026 23:05:11 +0900 Subject: [PATCH 02/64] feat: add shared API operation metadata macro --- Cargo.lock | 17 + Cargo.toml | 5 + crates/api-macros-impl/Cargo.toml | 14 + crates/api-macros-impl/src/lib.rs | 912 ++++++++++++++++++ crates/api-macros/Cargo.toml | 12 + crates/api-macros/src/lib.rs | 149 +++ crates/api-macros/tests/compile_fail.rs | 5 + crates/api-macros/tests/expansion.rs | 87 ++ crates/api-macros/tests/ui/anonymous_body.rs | 11 + .../api-macros/tests/ui/anonymous_body.stderr | 5 + .../tests/ui/duplicate_operation_id.rs | 14 + .../tests/ui/duplicate_operation_id.stderr | 5 + crates/api-macros/tests/ui/duplicate_route.rs | 14 + .../tests/ui/duplicate_route.stderr | 5 + .../tests/ui/invalid_error_status.rs | 12 + .../tests/ui/invalid_error_status.stderr | 5 + .../tests/ui/invalid_method_body.rs | 12 + .../tests/ui/invalid_method_body.stderr | 5 + .../tests/ui/invalid_success_status.rs | 11 + .../tests/ui/invalid_success_status.stderr | 5 + crates/api-macros/tests/ui/multiple_bodies.rs | 17 + .../tests/ui/multiple_bodies.stderr | 5 + crates/api-macros/tests/ui/path_mismatch.rs | 11 + .../api-macros/tests/ui/path_mismatch.stderr | 5 + crates/api-macros/tests/ui/private_body.rs | 14 + .../api-macros/tests/ui/private_body.stderr | 10 + .../tests/ui/unsupported_wire_kind.rs | 12 + .../tests/ui/unsupported_wire_kind.stderr | 5 + 28 files changed, 1384 insertions(+) create mode 100644 crates/api-macros-impl/Cargo.toml create mode 100644 crates/api-macros-impl/src/lib.rs create mode 100644 crates/api-macros/Cargo.toml create mode 100644 crates/api-macros/src/lib.rs create mode 100644 crates/api-macros/tests/compile_fail.rs create mode 100644 crates/api-macros/tests/expansion.rs create mode 100644 crates/api-macros/tests/ui/anonymous_body.rs create mode 100644 crates/api-macros/tests/ui/anonymous_body.stderr create mode 100644 crates/api-macros/tests/ui/duplicate_operation_id.rs create mode 100644 crates/api-macros/tests/ui/duplicate_operation_id.stderr create mode 100644 crates/api-macros/tests/ui/duplicate_route.rs create mode 100644 crates/api-macros/tests/ui/duplicate_route.stderr create mode 100644 crates/api-macros/tests/ui/invalid_error_status.rs create mode 100644 crates/api-macros/tests/ui/invalid_error_status.stderr create mode 100644 crates/api-macros/tests/ui/invalid_method_body.rs create mode 100644 crates/api-macros/tests/ui/invalid_method_body.stderr create mode 100644 crates/api-macros/tests/ui/invalid_success_status.rs create mode 100644 crates/api-macros/tests/ui/invalid_success_status.stderr create mode 100644 crates/api-macros/tests/ui/multiple_bodies.rs create mode 100644 crates/api-macros/tests/ui/multiple_bodies.stderr create mode 100644 crates/api-macros/tests/ui/path_mismatch.rs create mode 100644 crates/api-macros/tests/ui/path_mismatch.stderr create mode 100644 crates/api-macros/tests/ui/private_body.rs create mode 100644 crates/api-macros/tests/ui/private_body.stderr create mode 100644 crates/api-macros/tests/ui/unsupported_wire_kind.rs create mode 100644 crates/api-macros/tests/ui/unsupported_wire_kind.stderr diff --git a/Cargo.lock b/Cargo.lock index b429be87b..4936a753f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -168,6 +168,23 @@ version = "1.0.102" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7f202df86484c868dbad7eaa557ef785d5c66295e41b460ef922eca0723b842c" +[[package]] +name = "api-macros" +version = "0.1.0" +dependencies = [ + "api-macros-impl", + "trybuild", +] + +[[package]] +name = "api-macros-impl" +version = "0.1.0" +dependencies = [ + "proc-macro2", + "quote", + "syn 2.0.117", +] + [[package]] name = "arc-swap" version = "1.9.1" diff --git a/Cargo.toml b/Cargo.toml index 4b922b316..c7bd87a64 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -4,6 +4,8 @@ members = [ "crates/client", "crates/agen", "crates/agen-macros", + "crates/api-macros", + "crates/api-macros-impl", "crates/session-store", "crates/standalone", "crates/secrets", @@ -36,6 +38,8 @@ default-members = [ "crates/client", "crates/agen", "crates/agen-macros", + "crates/api-macros", + "crates/api-macros-impl", "crates/session-store", "crates/standalone", "crates/secrets", @@ -79,6 +83,7 @@ debug = false client = { path = "crates/client" } agen = { path = "crates/agen", version = "0.2" } agen-macros = { path = "crates/agen-macros", version = "0.2" } +api-macros = { path = "crates/api-macros" } manifest = { path = "crates/manifest" } mcp = { path = "crates/mcp" } lint-common = { path = "crates/lint-common" } diff --git a/crates/api-macros-impl/Cargo.toml b/crates/api-macros-impl/Cargo.toml new file mode 100644 index 000000000..511d03f67 --- /dev/null +++ b/crates/api-macros-impl/Cargo.toml @@ -0,0 +1,14 @@ +[package] +name = "api-macros-impl" +version = "0.1.0" +edition.workspace = true +license.workspace = true +publish = false + +[lib] +proc-macro = true + +[dependencies] +proc-macro2 = "1" +quote = "1" +syn = { version = "2", features = ["full", "extra-traits"] } diff --git a/crates/api-macros-impl/src/lib.rs b/crates/api-macros-impl/src/lib.rs new file mode 100644 index 000000000..60fac1db5 --- /dev/null +++ b/crates/api-macros-impl/src/lib.rs @@ -0,0 +1,912 @@ +//! Implementation of `api-macros`. + +use std::collections::{BTreeMap, BTreeSet}; + +use proc_macro::TokenStream; +use proc_macro2::Span; +use quote::{format_ident, quote}; +use syn::{ + Attribute, FnArg, GenericArgument, Ident, ItemTrait, LitInt, LitStr, Pat, PathArguments, + ReturnType, Token, TraitItem, TraitItemFn, Type, Visibility, + parse::{Parse, ParseStream}, + parse_macro_input, + spanned::Spanned, +}; + +#[proc_macro_attribute] +pub fn api(attr: TokenStream, item: TokenStream) -> TokenStream { + let attr = proc_macro2::TokenStream::from(attr); + let item = parse_macro_input!(item as ItemTrait); + + normalize_api(attr, item) + .and_then(expand_api) + .unwrap_or_else(syn::Error::into_compile_error) + .into() +} + +#[derive(Clone, Copy, Debug, Eq, Ord, PartialEq, PartialOrd)] +enum Method { + Get, + Post, + Put, + Patch, + Delete, + Head, + Options, +} + +impl Method { + fn from_ident(ident: &Ident) -> Option { + match ident.to_string().as_str() { + "get" => Some(Self::Get), + "post" => Some(Self::Post), + "put" => Some(Self::Put), + "patch" => Some(Self::Patch), + "delete" => Some(Self::Delete), + "head" => Some(Self::Head), + "options" => Some(Self::Options), + _ => None, + } + } + + fn tokens(self) -> proc_macro2::TokenStream { + let variant = match self { + Self::Get => quote!(Get), + Self::Post => quote!(Post), + Self::Put => quote!(Put), + Self::Patch => quote!(Patch), + Self::Delete => quote!(Delete), + Self::Head => quote!(Head), + Self::Options => quote!(Options), + }; + quote!(::api_macros::HttpMethod::#variant) + } + + fn permits_request_body(self) -> bool { + matches!(self, Self::Post | Self::Put | Self::Patch | Self::Delete) + } +} + +struct RouteArgs { + path: LitStr, + operation_id: Option, + status: Option, + error_status: Option, +} + +impl Parse for RouteArgs { + fn parse(input: ParseStream<'_>) -> syn::Result { + let path = input.parse()?; + let mut result = Self { + path, + operation_id: None, + status: None, + error_status: None, + }; + + while !input.is_empty() { + input.parse::()?; + if input.is_empty() { + break; + } + let key: Ident = input.parse()?; + input.parse::()?; + match key.to_string().as_str() { + "operation_id" => set_once( + &mut result.operation_id, + input.parse::()?, + &key, + "operation_id", + )?, + "status" => set_once(&mut result.status, input.parse::()?, &key, "status")?, + "error_status" => set_once( + &mut result.error_status, + input.parse::()?, + &key, + "error_status", + )?, + _ => { + return Err(syn::Error::new( + key.span(), + "unsupported route option; expected operation_id, status, or error_status", + )); + } + } + } + + Ok(result) + } +} + +fn set_once(slot: &mut Option, value: T, key: &Ident, display: &str) -> syn::Result<()> { + if slot.replace(value).is_some() { + return Err(syn::Error::new( + key.span(), + format!("duplicate `{display}` route option"), + )); + } + Ok(()) +} + +#[derive(Clone, Copy)] +enum Location { + Path, + Query, + Header, + Body, +} + +impl Location { + fn tokens(self) -> proc_macro2::TokenStream { + let variant = match self { + Self::Path => quote!(Path), + Self::Query => quote!(Query), + Self::Header => quote!(Header), + Self::Body => quote!(Body), + }; + quote!(::api_macros::ParameterLocation::#variant) + } +} + +struct Parameter { + rust_name: String, + wire_name: String, + location: Location, + ty: Type, +} + +struct Operation { + method_ident: Ident, + marker_ident: Ident, + operation_id: String, + method: Method, + path: String, + parameters: Vec, + request_body: Option, + response_body: Option, + error_body: Option, + response_status: u16, + error_status: Option, +} + +struct ApiDefinition { + item: ItemTrait, + metadata_ident: Ident, + operations_module_ident: Ident, + operations: Vec, +} + +fn normalize_api( + attr: proc_macro2::TokenStream, + mut item: ItemTrait, +) -> syn::Result { + if !attr.is_empty() { + return Err(syn::Error::new( + attr.span(), + "api does not accept arguments", + )); + } + if !matches!(item.vis, Visibility::Public(_)) { + return Err(syn::Error::new_spanned( + &item.ident, + "an #[api] trait must be public", + )); + } + if !item.generics.params.is_empty() || item.generics.where_clause.is_some() { + return Err(syn::Error::new_spanned( + &item.generics, + "an #[api] trait cannot be generic", + )); + } + + let trait_ident = item.ident.clone(); + let metadata_ident = format_ident!("{}Metadata", trait_ident); + let operations_module_ident = format_ident!("{}_operations", to_snake_case(&trait_ident)); + let mut operations = Vec::new(); + let mut operation_ids = BTreeMap::::new(); + let mut routes = BTreeMap::<(Method, String), Span>::new(); + + for trait_item in &mut item.items { + let TraitItem::Fn(method) = trait_item else { + return Err(syn::Error::new_spanned( + trait_item, + "an #[api] trait may contain methods only", + )); + }; + let operation = normalize_operation(method)?; + + if operation_ids + .insert(operation.operation_id.clone(), method.sig.ident.span()) + .is_some() + { + return Err(syn::Error::new( + method.sig.ident.span(), + format!("duplicate operation ID `{}`", operation.operation_id), + )); + } + if routes + .insert( + (operation.method, operation.path.clone()), + method.sig.ident.span(), + ) + .is_some() + { + return Err(syn::Error::new( + method.sig.ident.span(), + format!( + "duplicate route for {} `{}`", + method_name(operation.method), + operation.path + ), + )); + } + operations.push(operation); + } + + if operations.is_empty() { + return Err(syn::Error::new_spanned( + &item.ident, + "an #[api] trait must declare at least one operation", + )); + } + + operations.sort_by(|left, right| left.operation_id.cmp(&right.operation_id)); + + Ok(ApiDefinition { + item, + metadata_ident, + operations_module_ident, + operations, + }) +} + +fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { + validate_signature(method)?; + + let mut route = None; + let mut retained_attrs = Vec::new(); + for attr in std::mem::take(&mut method.attrs) { + let maybe_method = attr.path().get_ident().and_then(Method::from_ident); + if let Some(http_method) = maybe_method { + if route.is_some() { + return Err(syn::Error::new_spanned( + attr, + "an API method must have exactly one HTTP method attribute", + )); + } + let args = attr.parse_args::()?; + route = Some((http_method, args)); + } else { + retained_attrs.push(attr); + } + } + method.attrs = retained_attrs; + + let (http_method, route) = route.ok_or_else(|| { + syn::Error::new_spanned( + &method.sig.ident, + "an API method requires one of #[get], #[post], #[put], #[patch], #[delete], #[head], or #[options]", + ) + })?; + + let path = route.path.value(); + let placeholders = parse_path_template(&route.path)?; + let operation_id = route + .operation_id + .as_ref() + .map(LitStr::value) + .unwrap_or_else(|| method.sig.ident.to_string()); + validate_operation_id( + &operation_id, + route.operation_id.as_ref().unwrap_or(&route.path), + )?; + + let (response_body, error_body) = parse_return_type(&method.sig.output)?; + if let Some(ty) = response_body.as_ref() { + validate_named_body_type(ty, "response")?; + } + if let Some(ty) = error_body.as_ref() { + validate_named_body_type(ty, "error response")?; + } + + let response_status = parse_status(route.status.as_ref(), "status")? + .unwrap_or(if response_body.is_some() { 200 } else { 204 }); + if !(200..=299).contains(&response_status) { + return Err(syn::Error::new( + route + .status + .as_ref() + .map_or(route.path.span(), Spanned::span), + "status must be a success status between 200 and 299", + )); + } + let error_status = parse_status(route.error_status.as_ref(), "error_status")?; + if error_status.is_some() && error_body.is_none() { + return Err(syn::Error::new_spanned( + route.error_status, + "error_status requires a public error response type", + )); + } + if let Some(status) = error_status + && status < 400 + { + return Err(syn::Error::new_spanned( + route.error_status, + "error_status must be between 400 and 599", + )); + } + let error_status = error_body.as_ref().map(|_| error_status.unwrap_or(400)); + + if http_method == Method::Head && response_body.is_some() { + return Err(syn::Error::new_spanned( + &method.sig.output, + "HEAD operations cannot declare a response body", + )); + } + if matches!(response_status, 204 | 205) && response_body.is_some() { + return Err(syn::Error::new_spanned( + &method.sig.output, + format!("status {response_status} cannot have a response body"), + )); + } + + let mut parameters = Vec::new(); + let mut body_type = None; + let mut path_parameters = BTreeSet::new(); + for input in &mut method.sig.inputs { + let FnArg::Typed(input) = input else { + continue; + }; + let ident = argument_ident(&input.pat)?; + let rust_name = ident.to_string(); + let explicit = take_location(&mut input.attrs)?; + let (location, wire_name) = match explicit { + Some((Location::Path, wire_name)) => { + if wire_name.is_some() { + return Err(syn::Error::new( + ident.span(), + "#[path] does not accept a wire name", + )); + } + (Location::Path, rust_name.clone()) + } + Some((Location::Query, wire_name)) => { + if wire_name.is_some() { + return Err(syn::Error::new( + ident.span(), + "#[query] does not accept a wire name", + )); + } + (Location::Query, rust_name.clone()) + } + Some((Location::Header, wire_name)) => ( + Location::Header, + wire_name.unwrap_or_else(|| rust_name.clone()), + ), + Some((Location::Body, wire_name)) => { + if wire_name.is_some() { + return Err(syn::Error::new( + ident.span(), + "#[body] does not accept a wire name", + )); + } + (Location::Body, rust_name.clone()) + } + None if placeholders.contains(&rust_name) => (Location::Path, rust_name.clone()), + None => { + return Err(syn::Error::new( + ident.span(), + "API arguments must use #[path], #[query], #[header], or #[body]; path arguments may omit #[path] when their name matches a placeholder", + )); + } + }; + + if matches!(location, Location::Path) { + path_parameters.insert(rust_name.clone()); + } + if matches!(location, Location::Body) { + if body_type.is_some() { + return Err(syn::Error::new( + ident.span(), + "an API operation may declare only one request body", + )); + } + validate_named_body_type(&input.ty, "request")?; + body_type = Some((*input.ty).clone()); + } + parameters.push(Parameter { + rust_name, + wire_name, + location, + ty: (*input.ty).clone(), + }); + } + + let missing: Vec<_> = placeholders.difference(&path_parameters).cloned().collect(); + if !missing.is_empty() { + return Err(syn::Error::new( + route.path.span(), + format!( + "path placeholder(s) have no matching path parameter: {}", + missing.join(", ") + ), + )); + } + let extra: Vec<_> = path_parameters.difference(&placeholders).cloned().collect(); + if !extra.is_empty() { + return Err(syn::Error::new( + route.path.span(), + format!( + "path parameter(s) have no matching placeholder: {}", + extra.join(", ") + ), + )); + } + if body_type.is_some() && !http_method.permits_request_body() { + return Err(syn::Error::new_spanned( + &method.sig.ident, + format!( + "{} operations cannot declare a request body", + method_name(http_method) + ), + )); + } + + Ok(Operation { + marker_ident: format_ident!("{}", to_pascal_case(&method.sig.ident)), + method_ident: method.sig.ident.clone(), + operation_id, + method: http_method, + path, + parameters, + request_body: body_type, + response_body, + error_body, + response_status, + error_status, + }) +} + +fn validate_signature(method: &TraitItemFn) -> syn::Result<()> { + let sig = &method.sig; + if sig.asyncness.is_none() { + return Err(syn::Error::new_spanned( + &sig.fn_token, + "API methods must be async", + )); + } + if sig.unsafety.is_some() + || sig.abi.is_some() + || sig.variadic.is_some() + || !sig.generics.params.is_empty() + || sig.generics.where_clause.is_some() + { + return Err(syn::Error::new_spanned( + sig, + "API methods cannot be unsafe, extern, variadic, or generic", + )); + } + if method.default.is_some() { + return Err(syn::Error::new_spanned( + method, + "API methods cannot provide a default implementation", + )); + } + match sig.inputs.first() { + Some(FnArg::Receiver(receiver)) + if receiver.reference.is_some() && receiver.mutability.is_none() => {} + _ => { + return Err(syn::Error::new_spanned( + &sig.inputs, + "API methods must take `&self` as their first argument", + )); + } + } + Ok(()) +} + +fn take_location(attrs: &mut Vec) -> syn::Result)>> { + let mut result = None; + let mut retained = Vec::new(); + for attr in std::mem::take(attrs) { + let location = match attr.path().get_ident().map(ToString::to_string).as_deref() { + Some("path") => Some(Location::Path), + Some("query") => Some(Location::Query), + Some("header") => Some(Location::Header), + Some("body") => Some(Location::Body), + Some("stream") | Some("multipart") | Some("binary") => { + return Err(syn::Error::new_spanned( + attr, + "this wire kind is explicitly unsupported by the initial API contract", + )); + } + _ => None, + }; + let Some(location) = location else { + retained.push(attr); + continue; + }; + if result.is_some() { + return Err(syn::Error::new_spanned( + attr, + "an API argument may have only one location attribute", + )); + } + let wire_name = if matches!(location, Location::Header) { + if matches!(attr.meta, syn::Meta::Path(_)) { + None + } else { + Some(attr.parse_args::()?.value()) + } + } else if matches!(attr.meta, syn::Meta::Path(_)) { + None + } else { + return Err(syn::Error::new_spanned( + attr, + "only #[header(\"wire-name\")] accepts an argument", + )); + }; + result = Some((location, wire_name)); + } + *attrs = retained; + Ok(result) +} + +fn parse_path_template(path: &LitStr) -> syn::Result> { + let value = path.value(); + if !value.starts_with('/') { + return Err(syn::Error::new( + path.span(), + "API paths must start with `/`", + )); + } + if value.contains('?') || value.contains('#') { + return Err(syn::Error::new( + path.span(), + "API path templates cannot contain a query string or fragment", + )); + } + + let mut placeholders = BTreeSet::new(); + let bytes = value.as_bytes(); + let mut index = 0; + while index < bytes.len() { + match bytes[index] { + b'{' => { + let rest = &value[index + 1..]; + let Some(end) = rest.find('}') else { + return Err(syn::Error::new(path.span(), "unclosed path placeholder")); + }; + let name = &rest[..end]; + if name.is_empty() + || !name.chars().enumerate().all(|(index, ch)| { + ch == '_' + || ch.is_ascii_alphanumeric() && (index > 0 || !ch.is_ascii_digit()) + }) + { + return Err(syn::Error::new( + path.span(), + "path placeholders must be valid Rust identifiers", + )); + } + if !placeholders.insert(name.to_owned()) { + return Err(syn::Error::new( + path.span(), + format!("duplicate path placeholder `{name}`"), + )); + } + index += end + 2; + } + b'}' => { + return Err(syn::Error::new( + path.span(), + "path template contains an unmatched `}`", + )); + } + _ => index += 1, + } + } + Ok(placeholders) +} + +fn validate_operation_id(operation_id: &str, source: &impl Spanned) -> syn::Result<()> { + if operation_id.is_empty() + || !operation_id + .chars() + .all(|ch| ch.is_ascii_alphanumeric() || matches!(ch, '.' | '-' | '_' | ':' | '/')) + { + return Err(syn::Error::new( + source.span(), + "operation_id must be non-empty and contain only ASCII letters, digits, `.`, `-`, `_`, `:`, or `/`", + )); + } + Ok(()) +} + +fn parse_status(value: Option<&LitInt>, name: &str) -> syn::Result> { + let Some(value) = value else { + return Ok(None); + }; + let status = value.base10_parse::()?; + if !(100..=599).contains(&status) { + return Err(syn::Error::new( + value.span(), + format!("{name} must be an HTTP status between 100 and 599"), + )); + } + Ok(Some(status)) +} + +fn parse_return_type(output: &ReturnType) -> syn::Result<(Option, Option)> { + let ReturnType::Type(_, ty) = output else { + return Ok((None, None)); + }; + if is_unit(ty) { + return Ok((None, None)); + } + + if let Type::Path(type_path) = ty.as_ref() + && let Some(last) = type_path.path.segments.last() + && (last.ident == "Result" || last.ident == "ApiResult") + && let PathArguments::AngleBracketed(arguments) = &last.arguments + { + let types: Vec<_> = arguments + .args + .iter() + .filter_map(|argument| match argument { + GenericArgument::Type(ty) => Some(ty.clone()), + _ => None, + }) + .collect(); + return match types.as_slice() { + [response] => Ok((body_type(response), None)), + [response, error] => Ok((body_type(response), body_type(error))), + _ => Err(syn::Error::new_spanned( + arguments, + "API result types must have one response type and at most one error type", + )), + }; + } + + Ok((Some((**ty).clone()), None)) +} + +fn body_type(ty: &Type) -> Option { + (!is_unit(ty)).then(|| ty.clone()) +} + +fn is_unit(ty: &Type) -> bool { + matches!(ty, Type::Tuple(tuple) if tuple.elems.is_empty()) +} + +fn validate_named_body_type(ty: &Type, kind: &str) -> syn::Result<()> { + let Type::Path(path) = ty else { + return Err(syn::Error::new_spanned( + ty, + format!("{kind} bodies must use a named Rust type"), + )); + }; + if path.qself.is_some() { + return Err(syn::Error::new_spanned( + ty, + format!("{kind} bodies must use a direct named Rust type"), + )); + } + let last = path.path.segments.last().expect("type path has a segment"); + let builtin = matches!( + last.ident.to_string().as_str(), + "bool" + | "char" + | "str" + | "String" + | "u8" + | "u16" + | "u32" + | "u64" + | "u128" + | "usize" + | "i8" + | "i16" + | "i32" + | "i64" + | "i128" + | "isize" + | "f32" + | "f64" + | "Vec" + | "Option" + | "Box" + ); + if builtin { + return Err(syn::Error::new_spanned( + ty, + format!("{kind} bodies must use an explicit named DTO type"), + )); + } + Ok(()) +} + +fn argument_ident(pat: &Pat) -> syn::Result<&Ident> { + let Pat::Ident(ident) = pat else { + return Err(syn::Error::new_spanned( + pat, + "API arguments must use simple identifier patterns", + )); + }; + Ok(&ident.ident) +} + +fn expand_api(api: ApiDefinition) -> syn::Result { + let item = api.item; + let metadata_ident = api.metadata_ident; + let module_ident = api.operations_module_ident; + + let marker_items: Vec<_> = api + .operations + .iter() + .map(|operation| { + let marker_ident = &operation.marker_ident; + let method_ident = &operation.method_ident; + quote! { + #[doc = concat!("Typed operation marker for Rust method `", stringify!(#method_ident), "`.")] + #[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)] + pub struct #marker_ident; + } + }) + .collect(); + let operation_impls: Vec<_> = api + .operations + .iter() + .map(|operation| { + let marker_ident = &operation.marker_ident; + let metadata = metadata_tokens(operation); + let parameter_types = operation.parameters.iter().map(|parameter| ¶meter.ty); + let request_type = operation + .request_body + .as_ref() + .map(|ty| quote!(#ty)) + .unwrap_or_else(|| quote!(::api_macros::NoBody)); + let response_type = operation + .response_body + .as_ref() + .map(|ty| quote!(#ty)) + .unwrap_or_else(|| quote!(::api_macros::NoBody)); + let error_type = operation + .error_body + .as_ref() + .map(|ty| quote!(#ty)) + .unwrap_or_else(|| quote!(::api_macros::NoBody)); + quote! { + impl ::api_macros::Operation for #module_ident::#marker_ident { + type Parameters = (#(#parameter_types,)*); + type RequestBody = #request_type; + type ResponseBody = #response_type; + type ErrorBody = #error_type; + + const METADATA: ::api_macros::OperationMetadata = #metadata; + } + } + }) + .collect(); + let inventory: Vec<_> = api.operations.iter().map(metadata_tokens).collect(); + + Ok(quote! { + #item + + #[doc = "Deterministic operation inventory for the API trait."] + #[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)] + pub struct #metadata_ident; + + impl ::api_macros::ApiContract for #metadata_ident { + const OPERATIONS: &'static [::api_macros::OperationMetadata] = &[ + #(#inventory),* + ]; + } + + #[doc = "Typed markers generated for this API trait's operations."] + pub mod #module_ident { + #(#marker_items)* + } + + #(#operation_impls)* + }) +} + +fn metadata_tokens(operation: &Operation) -> proc_macro2::TokenStream { + let operation_id = &operation.operation_id; + let method = operation.method.tokens(); + let path = &operation.path; + let request_kind = if operation.request_body.is_some() { + quote!(::api_macros::WireKind::Json) + } else { + quote!(::api_macros::WireKind::Empty) + }; + let response_kind = if operation.response_body.is_some() { + quote!(::api_macros::WireKind::Json) + } else { + quote!(::api_macros::WireKind::Empty) + }; + let response_status = operation.response_status; + let error = match operation.error_status { + Some(status) => quote! { + ::core::option::Option::Some(::api_macros::ResponseMetadata { + status: #status, + body: ::api_macros::BodyMetadata { + wire_kind: ::api_macros::WireKind::Json, + }, + }) + }, + None => quote!(::core::option::Option::None), + }; + let parameters = operation.parameters.iter().map(|parameter| { + let rust_name = ¶meter.rust_name; + let wire_name = ¶meter.wire_name; + let location = parameter.location.tokens(); + let _type_connection = ¶meter.ty; + quote! { + ::api_macros::ParameterMetadata { + rust_name: #rust_name, + wire_name: #wire_name, + location: #location, + } + } + }); + + quote! { + ::api_macros::OperationMetadata { + operation_id: #operation_id, + method: #method, + path: #path, + parameters: &[#(#parameters),*], + request_body: ::api_macros::BodyMetadata { wire_kind: #request_kind }, + response: ::api_macros::ResponseMetadata { + status: #response_status, + body: ::api_macros::BodyMetadata { wire_kind: #response_kind }, + }, + error_response: #error, + } + } +} + +fn method_name(method: Method) -> &'static str { + match method { + Method::Get => "GET", + Method::Post => "POST", + Method::Put => "PUT", + Method::Patch => "PATCH", + Method::Delete => "DELETE", + Method::Head => "HEAD", + Method::Options => "OPTIONS", + } +} + +fn to_pascal_case(ident: &Ident) -> String { + ident + .to_string() + .split('_') + .filter(|part| !part.is_empty()) + .map(|part| { + let mut chars = part.chars(); + chars + .next() + .map(|first| first.to_ascii_uppercase().to_string() + chars.as_str()) + .unwrap_or_default() + }) + .collect() +} + +fn to_snake_case(ident: &Ident) -> String { + let value = ident.to_string(); + let mut result = String::new(); + for (index, ch) in value.chars().enumerate() { + if ch.is_ascii_uppercase() { + if index > 0 { + result.push('_'); + } + result.push(ch.to_ascii_lowercase()); + } else { + result.push(ch); + } + } + result +} diff --git a/crates/api-macros/Cargo.toml b/crates/api-macros/Cargo.toml new file mode 100644 index 000000000..73515abad --- /dev/null +++ b/crates/api-macros/Cargo.toml @@ -0,0 +1,12 @@ +[package] +name = "api-macros" +version = "0.1.0" +edition.workspace = true +license.workspace = true +publish = false + +[dependencies] +api-macros-impl = { path = "../api-macros-impl" } + +[dev-dependencies] +trybuild = "1" diff --git a/crates/api-macros/src/lib.rs b/crates/api-macros/src/lib.rs new file mode 100644 index 000000000..4d0ea8b7f --- /dev/null +++ b/crates/api-macros/src/lib.rs @@ -0,0 +1,149 @@ +//! Shared API trait declarations for Yoi services. +//! +//! `#[api]` turns a public trait into a single, transport-neutral API definition. The +//! original trait remains usable by Rust code; generated metadata and zero-sized operation +//! markers give adapters and generators a stable view of the same declaration. +//! +//! # Declaration syntax +//! +//! ``` +//! # mod example { +//! use api_macros::api; +//! +//! pub struct CreateWidget; +//! pub struct Widget; +//! pub struct PublicError; +//! +//! #[api] +//! pub trait WidgetApi { +//! #[post( +//! "/widgets", +//! operation_id = "widgets.create", +//! status = 201, +//! error_status = 422 +//! )] +//! async fn create( +//! &self, +//! #[body] request: CreateWidget, +//! #[header("x-request-id")] request_id: String, +//! ) -> Result; +//! +//! #[get("/widgets/{widget_id}", operation_id = "widgets.get")] +//! async fn get(&self, widget_id: u64) -> Result; +//! } +//! # } +//! ``` +//! +//! Supported route attributes are `get`, `post`, `put`, `patch`, `delete`, `head`, and +//! `options`. The first argument is a path template. `operation_id` defaults to the Rust +//! method name but should be set explicitly for contracts which must remain stable while Rust +//! names evolve. Success status defaults to `200`, except an empty (`()`) response defaults to +//! `204`. A public error inferred from `Result` defaults to status `400`. +//! +//! Arguments are classified with `#[body]`, `#[query]`, `#[header]`, or `#[path]`. An +//! unannotated argument whose Rust name occurs in the route template is inferred as a path +//! argument. Header attributes may carry a wire name, as in `#[header("x-request-id")]`. +//! Exactly one JSON body is allowed. JSON request, response, and error bodies must be named +//! Rust types; tuples, references, arrays, and other anonymous structural types are rejected. +//! +//! # Generated names +//! +//! For `WidgetApi`, the macro emits `WidgetApiMetadata`, which implements [`ApiContract`], +//! plus a `widget_api_operations` module. That module contains one marker per Rust method +//! (`Create`, `Get`, ...), each implementing [`Operation`] and connecting metadata to concrete +//! body types at compile time. The `ApiContract::OPERATIONS` inventory is sorted by operation +//! ID, so its ordering is independent of source method order. +//! +//! Only empty and JSON bodies are accepted by this first contract. [`WireKind`] reserves +//! explicit variants for future transport work; accepting one requires a deliberate macro and +//! adapter change rather than silently treating it as JSON. + +pub use api_macros_impl::api; + +/// A transport-neutral HTTP method. +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub enum HttpMethod { + Get, + Post, + Put, + Patch, + Delete, + Head, + Options, +} + +/// Where an operation argument is encoded on the wire. +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +pub enum ParameterLocation { + Path, + Query, + Header, + Body, +} + +/// Body encodings understood or explicitly reserved by the API contract. +#[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] +#[non_exhaustive] +pub enum WireKind { + Empty, + Json, + Streaming, + Multipart, + Binary, +} + +/// Stable metadata for one Rust method argument. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct ParameterMetadata { + pub rust_name: &'static str, + pub wire_name: &'static str, + pub location: ParameterLocation, +} + +/// Stable metadata for a request or response body. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct BodyMetadata { + pub wire_kind: WireKind, +} + +/// Stable metadata for one HTTP response. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct ResponseMetadata { + pub status: u16, + pub body: BodyMetadata, +} + +/// Complete transport metadata for one operation. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub struct OperationMetadata { + pub operation_id: &'static str, + pub method: HttpMethod, + pub path: &'static str, + pub parameters: &'static [ParameterMetadata], + pub request_body: BodyMetadata, + pub response: ResponseMetadata, + pub error_response: Option, +} + +/// Deterministic operation inventory emitted for an `#[api]` trait. +pub trait ApiContract { + const OPERATIONS: &'static [OperationMetadata]; +} + +/// Compile-time connection between operation metadata and its concrete body types. +pub trait Operation { + /// Tuple of all non-receiver parameter types in declaration order. + type Parameters; + /// JSON request body type, or [`NoBody`]. + type RequestBody; + /// JSON success response body type, or [`NoBody`]. + type ResponseBody; + /// JSON public error body type, or [`NoBody`]. + type ErrorBody; + + const METADATA: OperationMetadata; +} + +/// Marker used when an operation has no request, response, or public error body. +#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq)] +pub enum NoBody {} diff --git a/crates/api-macros/tests/compile_fail.rs b/crates/api-macros/tests/compile_fail.rs new file mode 100644 index 000000000..5858355df --- /dev/null +++ b/crates/api-macros/tests/compile_fail.rs @@ -0,0 +1,5 @@ +#[test] +fn invalid_api_declarations_have_stable_diagnostics() { + let tests = trybuild::TestCases::new(); + tests.compile_fail("tests/ui/*.rs"); +} diff --git a/crates/api-macros/tests/expansion.rs b/crates/api-macros/tests/expansion.rs new file mode 100644 index 000000000..47ea93013 --- /dev/null +++ b/crates/api-macros/tests/expansion.rs @@ -0,0 +1,87 @@ +#![allow(async_fn_in_trait)] + +use api_macros::{ApiContract, HttpMethod, NoBody, Operation, ParameterLocation, WireKind, api}; + +pub struct CreateWidget; +pub struct Widget; +pub struct Lookup; +pub struct PublicError; +pub struct SearchResult(std::marker::PhantomData); + +#[api] +pub trait WidgetApi { + #[post( + "/widgets", + operation_id = "widgets.create", + status = 201, + error_status = 422 + )] + async fn create( + &self, + #[body] request: CreateWidget, + #[header("x-request-id")] request_id: String, + ) -> Result; + + #[get("/widgets/{widget_id}", operation_id = "widgets.get")] + async fn lookup(&self, widget_id: u64, #[query] query: Lookup) -> Result; + + #[delete("/widgets/{widget_id}", operation_id = "widgets.delete")] + async fn delete(&self, #[path] widget_id: u64) -> (); +} + +#[api] +pub trait WrapperApi { + #[get("/search", operation_id = "search")] + async fn search(&self) -> SearchResult; +} + +fn assert_operation_types() +where + O: Operation< + Parameters = (CreateWidget, String), + RequestBody = CreateWidget, + ResponseBody = Widget, + ErrorBody = PublicError, + >, +{ +} + +#[test] +fn expansion_exposes_deterministic_metadata_and_type_connections() { + assert_operation_types::(); + + fn assert_named_wrapper>>() {} + assert_named_wrapper::(); + + let operations = ::OPERATIONS; + assert_eq!(operations.len(), 3); + assert_eq!(operations[0].operation_id, "widgets.create"); + assert_eq!(operations[1].operation_id, "widgets.delete"); + assert_eq!(operations[2].operation_id, "widgets.get"); + + let create = ::METADATA; + assert_eq!(operations[0], create); + assert_eq!(create.method, HttpMethod::Post); + assert_eq!(create.path, "/widgets"); + assert_eq!(create.request_body.wire_kind, WireKind::Json); + assert_eq!(create.response.status, 201); + assert_eq!(create.response.body.wire_kind, WireKind::Json); + assert_eq!(create.error_response.unwrap().status, 422); + assert_eq!(create.parameters[0].location, ParameterLocation::Body); + assert_eq!(create.parameters[1].location, ParameterLocation::Header); + assert_eq!(create.parameters[1].wire_name, "x-request-id"); + + let lookup = ::METADATA; + assert_eq!(lookup.parameters[0].location, ParameterLocation::Path); + assert_eq!(lookup.parameters[1].location, ParameterLocation::Query); + + let delete = ::METADATA; + assert_eq!(delete.response.status, 204); + assert_eq!(delete.response.body.wire_kind, WireKind::Empty); + assert_eq!(delete.error_response, None); + + fn assert_empty_request>() {} + fn assert_empty_response>() {} + assert_empty_request::(); + assert_empty_response::(); +} diff --git a/crates/api-macros/tests/ui/anonymous_body.rs b/crates/api-macros/tests/ui/anonymous_body.rs new file mode 100644 index 000000000..708ec9434 --- /dev/null +++ b/crates/api-macros/tests/ui/anonymous_body.rs @@ -0,0 +1,11 @@ +use api_macros::api; + +pub struct Response; + +#[api] +pub trait AnonymousBody { + #[post("/widgets")] + async fn create(&self, #[body] request: (u64, u64)) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/anonymous_body.stderr b/crates/api-macros/tests/ui/anonymous_body.stderr new file mode 100644 index 000000000..251ba1e12 --- /dev/null +++ b/crates/api-macros/tests/ui/anonymous_body.stderr @@ -0,0 +1,5 @@ +error: request bodies must use a named Rust type + --> tests/ui/anonymous_body.rs:8:45 + | +8 | async fn create(&self, #[body] request: (u64, u64)) -> Response; + | ^^^^^^^^^^ diff --git a/crates/api-macros/tests/ui/duplicate_operation_id.rs b/crates/api-macros/tests/ui/duplicate_operation_id.rs new file mode 100644 index 000000000..e75349269 --- /dev/null +++ b/crates/api-macros/tests/ui/duplicate_operation_id.rs @@ -0,0 +1,14 @@ +use api_macros::api; + +pub struct Response; + +#[api] +pub trait DuplicateOperationId { + #[get("/one", operation_id = "duplicate")] + async fn one(&self) -> Response; + + #[get("/two", operation_id = "duplicate")] + async fn two(&self) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/duplicate_operation_id.stderr b/crates/api-macros/tests/ui/duplicate_operation_id.stderr new file mode 100644 index 000000000..59bbd9096 --- /dev/null +++ b/crates/api-macros/tests/ui/duplicate_operation_id.stderr @@ -0,0 +1,5 @@ +error: duplicate operation ID `duplicate` + --> tests/ui/duplicate_operation_id.rs:11:14 + | +11 | async fn two(&self) -> Response; + | ^^^ diff --git a/crates/api-macros/tests/ui/duplicate_route.rs b/crates/api-macros/tests/ui/duplicate_route.rs new file mode 100644 index 000000000..86290c8dd --- /dev/null +++ b/crates/api-macros/tests/ui/duplicate_route.rs @@ -0,0 +1,14 @@ +use api_macros::api; + +pub struct Response; + +#[api] +pub trait DuplicateRoute { + #[get("/widgets", operation_id = "widgets.first")] + async fn first(&self) -> Response; + + #[get("/widgets", operation_id = "widgets.second")] + async fn second(&self) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/duplicate_route.stderr b/crates/api-macros/tests/ui/duplicate_route.stderr new file mode 100644 index 000000000..8f4999be7 --- /dev/null +++ b/crates/api-macros/tests/ui/duplicate_route.stderr @@ -0,0 +1,5 @@ +error: duplicate route for GET `/widgets` + --> tests/ui/duplicate_route.rs:11:14 + | +11 | async fn second(&self) -> Response; + | ^^^^^^ diff --git a/crates/api-macros/tests/ui/invalid_error_status.rs b/crates/api-macros/tests/ui/invalid_error_status.rs new file mode 100644 index 000000000..16e14af8e --- /dev/null +++ b/crates/api-macros/tests/ui/invalid_error_status.rs @@ -0,0 +1,12 @@ +use api_macros::api; + +pub struct Response; +pub struct PublicError; + +#[api] +pub trait InvalidErrorStatus { + #[get("/widgets", error_status = 200)] + async fn list(&self) -> Result; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/invalid_error_status.stderr b/crates/api-macros/tests/ui/invalid_error_status.stderr new file mode 100644 index 000000000..efaaf8604 --- /dev/null +++ b/crates/api-macros/tests/ui/invalid_error_status.stderr @@ -0,0 +1,5 @@ +error: error_status must be between 400 and 599 + --> tests/ui/invalid_error_status.rs:8:38 + | +8 | #[get("/widgets", error_status = 200)] + | ^^^ diff --git a/crates/api-macros/tests/ui/invalid_method_body.rs b/crates/api-macros/tests/ui/invalid_method_body.rs new file mode 100644 index 000000000..0c8321b76 --- /dev/null +++ b/crates/api-macros/tests/ui/invalid_method_body.rs @@ -0,0 +1,12 @@ +use api_macros::api; + +pub struct Request; +pub struct Response; + +#[api] +pub trait InvalidMethodBody { + #[get("/widgets")] + async fn list(&self, #[body] request: Request) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/invalid_method_body.stderr b/crates/api-macros/tests/ui/invalid_method_body.stderr new file mode 100644 index 000000000..e4d47b676 --- /dev/null +++ b/crates/api-macros/tests/ui/invalid_method_body.stderr @@ -0,0 +1,5 @@ +error: GET operations cannot declare a request body + --> tests/ui/invalid_method_body.rs:9:14 + | +9 | async fn list(&self, #[body] request: Request) -> Response; + | ^^^^ diff --git a/crates/api-macros/tests/ui/invalid_success_status.rs b/crates/api-macros/tests/ui/invalid_success_status.rs new file mode 100644 index 000000000..bf80274cf --- /dev/null +++ b/crates/api-macros/tests/ui/invalid_success_status.rs @@ -0,0 +1,11 @@ +use api_macros::api; + +pub struct Response; + +#[api] +pub trait InvalidSuccessStatus { + #[get("/widgets", status = 404)] + async fn list(&self) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/invalid_success_status.stderr b/crates/api-macros/tests/ui/invalid_success_status.stderr new file mode 100644 index 000000000..14e030505 --- /dev/null +++ b/crates/api-macros/tests/ui/invalid_success_status.stderr @@ -0,0 +1,5 @@ +error: status must be a success status between 200 and 299 + --> tests/ui/invalid_success_status.rs:7:32 + | +7 | #[get("/widgets", status = 404)] + | ^^^ diff --git a/crates/api-macros/tests/ui/multiple_bodies.rs b/crates/api-macros/tests/ui/multiple_bodies.rs new file mode 100644 index 000000000..1c8b139c6 --- /dev/null +++ b/crates/api-macros/tests/ui/multiple_bodies.rs @@ -0,0 +1,17 @@ +use api_macros::api; + +pub struct FirstBody; +pub struct SecondBody; +pub struct Response; + +#[api] +pub trait MultipleBodies { + #[post("/widgets")] + async fn create( + &self, + #[body] first: FirstBody, + #[body] second: SecondBody, + ) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/multiple_bodies.stderr b/crates/api-macros/tests/ui/multiple_bodies.stderr new file mode 100644 index 000000000..1b259838b --- /dev/null +++ b/crates/api-macros/tests/ui/multiple_bodies.stderr @@ -0,0 +1,5 @@ +error: an API operation may declare only one request body + --> tests/ui/multiple_bodies.rs:13:17 + | +13 | #[body] second: SecondBody, + | ^^^^^^ diff --git a/crates/api-macros/tests/ui/path_mismatch.rs b/crates/api-macros/tests/ui/path_mismatch.rs new file mode 100644 index 000000000..bec7543ae --- /dev/null +++ b/crates/api-macros/tests/ui/path_mismatch.rs @@ -0,0 +1,11 @@ +use api_macros::api; + +pub struct Response; + +#[api] +pub trait PathMismatch { + #[get("/widgets/{widget_id}")] + async fn get(&self, #[path] other_id: u64) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/path_mismatch.stderr b/crates/api-macros/tests/ui/path_mismatch.stderr new file mode 100644 index 000000000..c7c55cf66 --- /dev/null +++ b/crates/api-macros/tests/ui/path_mismatch.stderr @@ -0,0 +1,5 @@ +error: path placeholder(s) have no matching path parameter: widget_id + --> tests/ui/path_mismatch.rs:7:11 + | +7 | #[get("/widgets/{widget_id}")] + | ^^^^^^^^^^^^^^^^^^^^^^ diff --git a/crates/api-macros/tests/ui/private_body.rs b/crates/api-macros/tests/ui/private_body.rs new file mode 100644 index 000000000..50ee62629 --- /dev/null +++ b/crates/api-macros/tests/ui/private_body.rs @@ -0,0 +1,14 @@ +#![allow(async_fn_in_trait, private_interfaces)] + +use api_macros::api; + +struct PrivateRequest; +pub struct Response; + +#[api] +pub trait PrivateBody { + #[post("/widgets")] + async fn create(&self, #[body] request: PrivateRequest) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/private_body.stderr b/crates/api-macros/tests/ui/private_body.stderr new file mode 100644 index 000000000..c77408cd3 --- /dev/null +++ b/crates/api-macros/tests/ui/private_body.stderr @@ -0,0 +1,10 @@ +error[E0446]: private type `PrivateRequest` in public interface + --> tests/ui/private_body.rs:8:1 + | +5 | struct PrivateRequest; + | --------------------- `PrivateRequest` declared as private +... +8 | #[api] + | ^^^^^^ can't leak private type + | + = note: this error originates in the attribute macro `api` (in Nightly builds, run with -Z macro-backtrace for more info) diff --git a/crates/api-macros/tests/ui/unsupported_wire_kind.rs b/crates/api-macros/tests/ui/unsupported_wire_kind.rs new file mode 100644 index 000000000..bb7f064d3 --- /dev/null +++ b/crates/api-macros/tests/ui/unsupported_wire_kind.rs @@ -0,0 +1,12 @@ +use api_macros::api; + +pub struct StreamRequest; +pub struct Response; + +#[api] +pub trait UnsupportedWireKind { + #[post("/widgets")] + async fn create(&self, #[stream] request: StreamRequest) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/unsupported_wire_kind.stderr b/crates/api-macros/tests/ui/unsupported_wire_kind.stderr new file mode 100644 index 000000000..20da89534 --- /dev/null +++ b/crates/api-macros/tests/ui/unsupported_wire_kind.stderr @@ -0,0 +1,5 @@ +error: this wire kind is explicitly unsupported by the initial API contract + --> tests/ui/unsupported_wire_kind.rs:9:28 + | +9 | async fn create(&self, #[stream] request: StreamRequest) -> Response; + | ^^^^^^^^^ From 633f43530ca036d1f053e68f30c8dd1400182d0a Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 17 Sep 2026 23:21:18 +0900 Subject: [PATCH 03/64] fix: harden API macro expansion hygiene --- Cargo.lock | 25 ++++ crates/api-macros-impl/Cargo.toml | 1 + crates/api-macros-impl/src/lib.rs | 115 ++++++++++++------ crates/api-macros/src/lib.rs | 2 + crates/api-macros/tests/expansion.rs | 8 ++ crates/api-macros/tests/renamed_dependency.rs | 66 ++++++++++ .../api-macros/tests/ui/marker_collision.rs | 14 +++ .../tests/ui/marker_collision.stderr | 11 ++ 8 files changed, 207 insertions(+), 35 deletions(-) create mode 100644 crates/api-macros/tests/renamed_dependency.rs create mode 100644 crates/api-macros/tests/ui/marker_collision.rs create mode 100644 crates/api-macros/tests/ui/marker_collision.stderr diff --git a/Cargo.lock b/Cargo.lock index 4936a753f..aeed48d99 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -180,6 +180,7 @@ dependencies = [ name = "api-macros-impl" version = "0.1.0" dependencies = [ + "proc-macro-crate", "proc-macro2", "quote", "syn 2.0.117", @@ -3255,6 +3256,15 @@ dependencies = [ "elliptic-curve", ] +[[package]] +name = "proc-macro-crate" +version = "3.5.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "e67ba7e9b2b56446f1d419b1d807906278ffa1a658a8a5d8a39dcb1f5a78614f" +dependencies = [ + "toml_edit", +] + [[package]] name = "proc-macro2" version = "1.0.106" @@ -4844,6 +4854,18 @@ dependencies = [ "serde_core", ] +[[package]] +name = "toml_edit" +version = "0.25.13+spec-1.1.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "6975367e4d2ef766d86af01ffad14b622fecc8d4357a998fbc4deb6e9bacaf9b" +dependencies = [ + "indexmap", + "toml_datetime", + "toml_parser", + "winnow", +] + [[package]] name = "toml_parser" version = "1.1.2+spec-1.1.0" @@ -5900,6 +5922,9 @@ name = "winnow" version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "09dac053f1cd375980747450bfc7250c264eaae0583872e845c0c7cd578872b5" +dependencies = [ + "memchr", +] [[package]] name = "wiremock" diff --git a/crates/api-macros-impl/Cargo.toml b/crates/api-macros-impl/Cargo.toml index 511d03f67..52c228754 100644 --- a/crates/api-macros-impl/Cargo.toml +++ b/crates/api-macros-impl/Cargo.toml @@ -9,6 +9,7 @@ publish = false proc-macro = true [dependencies] +proc-macro-crate = "3" proc-macro2 = "1" quote = "1" syn = { version = "2", features = ["full", "extra-traits"] } diff --git a/crates/api-macros-impl/src/lib.rs b/crates/api-macros-impl/src/lib.rs index 60fac1db5..aabbe99f0 100644 --- a/crates/api-macros-impl/src/lib.rs +++ b/crates/api-macros-impl/src/lib.rs @@ -3,6 +3,7 @@ use std::collections::{BTreeMap, BTreeSet}; use proc_macro::TokenStream; +use proc_macro_crate::{FoundCrate, crate_name}; use proc_macro2::Span; use quote::{format_ident, quote}; use syn::{ @@ -49,7 +50,7 @@ impl Method { } } - fn tokens(self) -> proc_macro2::TokenStream { + fn tokens(self, api_crate: &proc_macro2::TokenStream) -> proc_macro2::TokenStream { let variant = match self { Self::Get => quote!(Get), Self::Post => quote!(Post), @@ -59,7 +60,7 @@ impl Method { Self::Head => quote!(Head), Self::Options => quote!(Options), }; - quote!(::api_macros::HttpMethod::#variant) + quote!(#api_crate::HttpMethod::#variant) } fn permits_request_body(self) -> bool { @@ -137,14 +138,14 @@ enum Location { } impl Location { - fn tokens(self) -> proc_macro2::TokenStream { + fn tokens(self, api_crate: &proc_macro2::TokenStream) -> proc_macro2::TokenStream { let variant = match self { Self::Path => quote!(Path), Self::Query => quote!(Query), Self::Header => quote!(Header), Self::Body => quote!(Body), }; - quote!(::api_macros::ParameterLocation::#variant) + quote!(#api_crate::ParameterLocation::#variant) } } @@ -200,11 +201,12 @@ fn normalize_api( } let trait_ident = item.ident.clone(); - let metadata_ident = format_ident!("{}Metadata", trait_ident); + let metadata_ident = format_ident!("{}Metadata", ident_text(&trait_ident)); let operations_module_ident = format_ident!("{}_operations", to_snake_case(&trait_ident)); let mut operations = Vec::new(); let mut operation_ids = BTreeMap::::new(); let mut routes = BTreeMap::<(Method, String), Span>::new(); + let mut marker_names = BTreeMap::::new(); for trait_item in &mut item.items { let TraitItem::Fn(method) = trait_item else { @@ -215,6 +217,24 @@ fn normalize_api( }; let operation = normalize_operation(method)?; + let marker_name = operation.marker_ident.to_string(); + if let Some((first_method, first_span)) = marker_names.insert( + marker_name.clone(), + (ident_text(&method.sig.ident), method.sig.ident.span()), + ) { + let mut error = syn::Error::new( + method.sig.ident.span(), + format!( + "generated operation marker `{marker_name}` collides with method `{first_method}`" + ), + ); + error.combine(syn::Error::new( + first_span, + format!("first method generating marker `{marker_name}`"), + )); + return Err(error); + } + if operation_ids .insert(operation.operation_id.clone(), method.sig.ident.span()) .is_some() @@ -295,7 +315,7 @@ fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { .operation_id .as_ref() .map(LitStr::value) - .unwrap_or_else(|| method.sig.ident.to_string()); + .unwrap_or_else(|| ident_text(&method.sig.ident)); validate_operation_id( &operation_id, route.operation_id.as_ref().unwrap_or(&route.path), @@ -358,7 +378,7 @@ fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { continue; }; let ident = argument_ident(&input.pat)?; - let rust_name = ident.to_string(); + let rust_name = ident_text(ident); let explicit = take_location(&mut input.attrs)?; let (location, wire_name) = match explicit { Some((Location::Path, wire_name)) => { @@ -736,7 +756,22 @@ fn argument_ident(pat: &Pat) -> syn::Result<&Ident> { Ok(&ident.ident) } +fn api_crate_path() -> syn::Result { + match crate_name("api-macros") { + Ok(FoundCrate::Itself) => Ok(quote!(::api_macros)), + Ok(FoundCrate::Name(name)) => { + let ident = Ident::new(&name, Span::call_site()); + Ok(quote!(::#ident)) + } + Err(error) => Err(syn::Error::new( + Span::call_site(), + format!("could not resolve the api-macros support crate: {error}"), + )), + } +} + fn expand_api(api: ApiDefinition) -> syn::Result { + let api_crate = api_crate_path()?; let item = api.item; let metadata_ident = api.metadata_ident; let module_ident = api.operations_module_ident; @@ -759,36 +794,40 @@ fn expand_api(api: ApiDefinition) -> syn::Result { .iter() .map(|operation| { let marker_ident = &operation.marker_ident; - let metadata = metadata_tokens(operation); + let metadata = metadata_tokens(operation, &api_crate); let parameter_types = operation.parameters.iter().map(|parameter| ¶meter.ty); let request_type = operation .request_body .as_ref() .map(|ty| quote!(#ty)) - .unwrap_or_else(|| quote!(::api_macros::NoBody)); + .unwrap_or_else(|| quote!(#api_crate::NoBody)); let response_type = operation .response_body .as_ref() .map(|ty| quote!(#ty)) - .unwrap_or_else(|| quote!(::api_macros::NoBody)); + .unwrap_or_else(|| quote!(#api_crate::NoBody)); let error_type = operation .error_body .as_ref() .map(|ty| quote!(#ty)) - .unwrap_or_else(|| quote!(::api_macros::NoBody)); + .unwrap_or_else(|| quote!(#api_crate::NoBody)); quote! { - impl ::api_macros::Operation for #module_ident::#marker_ident { + impl #api_crate::Operation for #module_ident::#marker_ident { type Parameters = (#(#parameter_types,)*); type RequestBody = #request_type; type ResponseBody = #response_type; type ErrorBody = #error_type; - const METADATA: ::api_macros::OperationMetadata = #metadata; + const METADATA: #api_crate::OperationMetadata = #metadata; } } }) .collect(); - let inventory: Vec<_> = api.operations.iter().map(metadata_tokens).collect(); + let inventory: Vec<_> = api + .operations + .iter() + .map(|operation| metadata_tokens(operation, &api_crate)) + .collect(); Ok(quote! { #item @@ -797,8 +836,8 @@ fn expand_api(api: ApiDefinition) -> syn::Result { #[derive(Clone, Copy, Debug, Default, Eq, Hash, PartialEq)] pub struct #metadata_ident; - impl ::api_macros::ApiContract for #metadata_ident { - const OPERATIONS: &'static [::api_macros::OperationMetadata] = &[ + impl #api_crate::ApiContract for #metadata_ident { + const OPERATIONS: &'static [#api_crate::OperationMetadata] = &[ #(#inventory),* ]; } @@ -812,27 +851,30 @@ fn expand_api(api: ApiDefinition) -> syn::Result { }) } -fn metadata_tokens(operation: &Operation) -> proc_macro2::TokenStream { +fn metadata_tokens( + operation: &Operation, + api_crate: &proc_macro2::TokenStream, +) -> proc_macro2::TokenStream { let operation_id = &operation.operation_id; - let method = operation.method.tokens(); + let method = operation.method.tokens(api_crate); let path = &operation.path; let request_kind = if operation.request_body.is_some() { - quote!(::api_macros::WireKind::Json) + quote!(#api_crate::WireKind::Json) } else { - quote!(::api_macros::WireKind::Empty) + quote!(#api_crate::WireKind::Empty) }; let response_kind = if operation.response_body.is_some() { - quote!(::api_macros::WireKind::Json) + quote!(#api_crate::WireKind::Json) } else { - quote!(::api_macros::WireKind::Empty) + quote!(#api_crate::WireKind::Empty) }; let response_status = operation.response_status; let error = match operation.error_status { Some(status) => quote! { - ::core::option::Option::Some(::api_macros::ResponseMetadata { + ::core::option::Option::Some(#api_crate::ResponseMetadata { status: #status, - body: ::api_macros::BodyMetadata { - wire_kind: ::api_macros::WireKind::Json, + body: #api_crate::BodyMetadata { + wire_kind: #api_crate::WireKind::Json, }, }) }, @@ -841,10 +883,9 @@ fn metadata_tokens(operation: &Operation) -> proc_macro2::TokenStream { let parameters = operation.parameters.iter().map(|parameter| { let rust_name = ¶meter.rust_name; let wire_name = ¶meter.wire_name; - let location = parameter.location.tokens(); - let _type_connection = ¶meter.ty; + let location = parameter.location.tokens(api_crate); quote! { - ::api_macros::ParameterMetadata { + #api_crate::ParameterMetadata { rust_name: #rust_name, wire_name: #wire_name, location: #location, @@ -853,15 +894,15 @@ fn metadata_tokens(operation: &Operation) -> proc_macro2::TokenStream { }); quote! { - ::api_macros::OperationMetadata { + #api_crate::OperationMetadata { operation_id: #operation_id, method: #method, path: #path, parameters: &[#(#parameters),*], - request_body: ::api_macros::BodyMetadata { wire_kind: #request_kind }, - response: ::api_macros::ResponseMetadata { + request_body: #api_crate::BodyMetadata { wire_kind: #request_kind }, + response: #api_crate::ResponseMetadata { status: #response_status, - body: ::api_macros::BodyMetadata { wire_kind: #response_kind }, + body: #api_crate::BodyMetadata { wire_kind: #response_kind }, }, error_response: #error, } @@ -880,9 +921,13 @@ fn method_name(method: Method) -> &'static str { } } +fn ident_text(ident: &Ident) -> String { + let value = ident.to_string(); + value.strip_prefix("r#").unwrap_or(&value).to_owned() +} + fn to_pascal_case(ident: &Ident) -> String { - ident - .to_string() + ident_text(ident) .split('_') .filter(|part| !part.is_empty()) .map(|part| { @@ -896,7 +941,7 @@ fn to_pascal_case(ident: &Ident) -> String { } fn to_snake_case(ident: &Ident) -> String { - let value = ident.to_string(); + let value = ident_text(ident); let mut result = String::new(); for (index, ch) in value.chars().enumerate() { if ch.is_ascii_uppercase() { diff --git a/crates/api-macros/src/lib.rs b/crates/api-macros/src/lib.rs index 4d0ea8b7f..c7431e3b2 100644 --- a/crates/api-macros/src/lib.rs +++ b/crates/api-macros/src/lib.rs @@ -58,6 +58,8 @@ //! explicit variants for future transport work; accepting one requires a deliberate macro and //! adapter change rather than silently treating it as JSON. +extern crate self as api_macros; + pub use api_macros_impl::api; /// A transport-neutral HTTP method. diff --git a/crates/api-macros/tests/expansion.rs b/crates/api-macros/tests/expansion.rs index 47ea93013..3f8c027a3 100644 --- a/crates/api-macros/tests/expansion.rs +++ b/crates/api-macros/tests/expansion.rs @@ -35,6 +35,12 @@ pub trait WrapperApi { async fn search(&self) -> SearchResult; } +#[api] +pub trait RawIdentifierApi { + #[get("/type", operation_id = "raw.type")] + async fn r#type(&self) -> Widget; +} + fn assert_operation_types() where O: Operation< @@ -52,6 +58,8 @@ fn expansion_exposes_deterministic_metadata_and_type_connections() { fn assert_named_wrapper>>() {} assert_named_wrapper::(); + let raw = ::METADATA; + assert_eq!(raw.operation_id, "raw.type"); let operations = ::OPERATIONS; assert_eq!(operations.len(), 3); diff --git a/crates/api-macros/tests/renamed_dependency.rs b/crates/api-macros/tests/renamed_dependency.rs new file mode 100644 index 000000000..6c0c5c9bd --- /dev/null +++ b/crates/api-macros/tests/renamed_dependency.rs @@ -0,0 +1,66 @@ +use std::{fs, path::PathBuf, process::Command}; + +#[test] +fn macro_expansion_resolves_a_renamed_support_dependency() { + let fixture = std::env::temp_dir().join(format!( + "api-macros-renamed-dependency-{}", + std::process::id() + )); + let _ = fs::remove_dir_all(&fixture); + fs::create_dir_all(fixture.join("src")).expect("create fixture source directory"); + + let package_path = PathBuf::from(env!("CARGO_MANIFEST_DIR")); + let package_path = package_path + .to_str() + .expect("package path is UTF-8") + .replace('\\', "\\\\"); + fs::write( + fixture.join("Cargo.toml"), + format!( + r#"[package] +name = "renamed-api-consumer" +version = "0.0.0" +edition = "2024" + +[workspace] + +[dependencies] +yoi-api = {{ package = "api-macros", path = "{package_path}" }} +"#, + ), + ) + .expect("write fixture manifest"); + fs::write( + fixture.join("src/lib.rs"), + r#"use yoi_api::{ApiContract, api}; + +pub struct Output; + +#[api] +pub trait RenamedApi { + #[get("/output", operation_id = "renamed.output")] + async fn output(&self) -> Output; +} + +pub fn operation_count() -> usize { + ::OPERATIONS.len() +} +"#, + ) + .expect("write fixture source"); + + let output = Command::new(env!("CARGO")) + .args(["check", "--quiet", "--offline"]) + .current_dir(&fixture) + .env("CARGO_TARGET_DIR", fixture.join("target")) + .output() + .expect("run cargo check for renamed dependency fixture"); + let _ = fs::remove_dir_all(&fixture); + + assert!( + output.status.success(), + "renamed dependency fixture failed:\nstdout:\n{}\nstderr:\n{}", + String::from_utf8_lossy(&output.stdout), + String::from_utf8_lossy(&output.stderr), + ); +} diff --git a/crates/api-macros/tests/ui/marker_collision.rs b/crates/api-macros/tests/ui/marker_collision.rs new file mode 100644 index 000000000..b6a188f25 --- /dev/null +++ b/crates/api-macros/tests/ui/marker_collision.rs @@ -0,0 +1,14 @@ +use api_macros::api; + +pub struct Response; + +#[api] +pub trait MarkerCollision { + #[get("/first")] + async fn foo_bar(&self) -> Response; + + #[get("/second")] + async fn foo__bar(&self) -> Response; +} + +fn main() {} diff --git a/crates/api-macros/tests/ui/marker_collision.stderr b/crates/api-macros/tests/ui/marker_collision.stderr new file mode 100644 index 000000000..4286e6991 --- /dev/null +++ b/crates/api-macros/tests/ui/marker_collision.stderr @@ -0,0 +1,11 @@ +error: generated operation marker `FooBar` collides with method `foo_bar` + --> tests/ui/marker_collision.rs:11:14 + | +11 | async fn foo__bar(&self) -> Response; + | ^^^^^^^^ + +error: first method generating marker `FooBar` + --> tests/ui/marker_collision.rs:8:14 + | +8 | async fn foo_bar(&self) -> Response; + | ^^^^^^^ From 019ab57e8f58a1c0d54b9abf3f7bd6dfb43004bf Mon Sep 17 00:00:00 2001 From: Hare Date: Thu, 17 Sep 2026 23:28:20 +0900 Subject: [PATCH 04/64] fix: make operation marker naming total --- crates/api-macros-impl/src/lib.rs | 17 ++++++++++++++++- crates/api-macros/src/lib.rs | 5 +++-- crates/api-macros/tests/expansion.rs | 14 ++++++++++++++ 3 files changed, 33 insertions(+), 3 deletions(-) diff --git a/crates/api-macros-impl/src/lib.rs b/crates/api-macros-impl/src/lib.rs index aabbe99f0..66cd9ab0c 100644 --- a/crates/api-macros-impl/src/lib.rs +++ b/crates/api-macros-impl/src/lib.rs @@ -473,7 +473,7 @@ fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { } Ok(Operation { - marker_ident: format_ident!("{}", to_pascal_case(&method.sig.ident)), + marker_ident: operation_marker_ident(&method.sig.ident), method_ident: method.sig.ident.clone(), operation_id, method: http_method, @@ -926,6 +926,21 @@ fn ident_text(ident: &Ident) -> String { value.strip_prefix("r#").unwrap_or(&value).to_owned() } +fn operation_marker_ident(method: &Ident) -> Ident { + let candidate = to_pascal_case(method); + if let Ok(mut marker) = syn::parse_str::(&candidate) { + marker.set_span(method.span()); + return marker; + } + + let encoded = ident_text(method) + .as_bytes() + .iter() + .map(|byte| format!("{byte:02x}")) + .collect::(); + Ident::new(&format!("Operation{encoded}"), method.span()) +} + fn to_pascal_case(ident: &Ident) -> String { ident_text(ident) .split('_') diff --git a/crates/api-macros/src/lib.rs b/crates/api-macros/src/lib.rs index c7431e3b2..006dd6e4b 100644 --- a/crates/api-macros/src/lib.rs +++ b/crates/api-macros/src/lib.rs @@ -51,8 +51,9 @@ //! For `WidgetApi`, the macro emits `WidgetApiMetadata`, which implements [`ApiContract`], //! plus a `widget_api_operations` module. That module contains one marker per Rust method //! (`Create`, `Get`, ...), each implementing [`Operation`] and connecting metadata to concrete -//! body types at compile time. The `ApiContract::OPERATIONS` inventory is sorted by operation -//! ID, so its ordering is independent of source method order. +//! body types at compile time. Unusual method names which have no valid PascalCase form use the +//! deterministic `Operation` fallback. The `ApiContract::OPERATIONS` inventory is +//! sorted by operation ID, so its ordering is independent of source method order. //! //! Only empty and JSON bodies are accepted by this first contract. [`WireKind`] reserves //! explicit variants for future transport work; accepting one requires a deliberate macro and diff --git a/crates/api-macros/tests/expansion.rs b/crates/api-macros/tests/expansion.rs index 3f8c027a3..a52ae4a78 100644 --- a/crates/api-macros/tests/expansion.rs +++ b/crates/api-macros/tests/expansion.rs @@ -41,6 +41,15 @@ pub trait RawIdentifierApi { async fn r#type(&self) -> Widget; } +#[api] +pub trait UnusualIdentifierApi { + #[get("/underscores", operation_id = "unusual.underscores")] + async fn __(&self) -> Widget; + + #[get("/underscore-digit", operation_id = "unusual.underscore_digit")] + async fn _0(&self) -> Widget; +} + fn assert_operation_types() where O: Operation< @@ -60,6 +69,11 @@ fn expansion_exposes_deterministic_metadata_and_type_connections() { assert_named_wrapper::(); let raw = ::METADATA; assert_eq!(raw.operation_id, "raw.type"); + let underscores = ::METADATA; + assert_eq!(underscores.operation_id, "unusual.underscores"); + let underscore_digit = + ::METADATA; + assert_eq!(underscore_digit.operation_id, "unusual.underscore_digit"); let operations = ::OPERATIONS; assert_eq!(operations.len(), 3); From 01f5c560c70c2d88649ee770df7e50427007d436 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 00:01:41 +0900 Subject: [PATCH 05/64] feat(api): generate reqwest and axum adapters --- Cargo.lock | 8 + crates/api-macros-impl/src/lib.rs | 523 ++++++++++++++++++++- crates/api-macros/Cargo.toml | 22 + crates/api-macros/src/axum.rs | 61 +++ crates/api-macros/src/lib.rs | 24 + crates/api-macros/src/reqwest.rs | 563 +++++++++++++++++++++++ crates/api-macros/tests/http_adapters.rs | 277 +++++++++++ 7 files changed, 1465 insertions(+), 13 deletions(-) create mode 100644 crates/api-macros/src/axum.rs create mode 100644 crates/api-macros/src/reqwest.rs create mode 100644 crates/api-macros/tests/http_adapters.rs diff --git a/Cargo.lock b/Cargo.lock index aeed48d99..961baa889 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -173,7 +173,15 @@ name = "api-macros" version = "0.1.0" dependencies = [ "api-macros-impl", + "axum", + "futures", + "reqwest", + "serde", + "serde_json", + "serde_urlencoded", + "tokio", "trybuild", + "url", ] [[package]] diff --git a/crates/api-macros-impl/src/lib.rs b/crates/api-macros-impl/src/lib.rs index 66cd9ab0c..de30af0ae 100644 --- a/crates/api-macros-impl/src/lib.rs +++ b/crates/api-macros-impl/src/lib.rs @@ -11,6 +11,7 @@ use syn::{ ReturnType, Token, TraitItem, TraitItemFn, Type, Visibility, parse::{Parse, ParseStream}, parse_macro_input, + punctuated::Punctuated, spanned::Spanned, }; @@ -36,6 +37,35 @@ enum Method { Options, } +#[derive(Clone, Copy, Default)] +struct AdapterConfig { + reqwest: bool, + axum: bool, +} + +impl Parse for AdapterConfig { + fn parse(input: ParseStream<'_>) -> syn::Result { + let adapters = Punctuated::::parse_terminated(input)?; + let mut config = Self::default(); + for adapter in adapters { + match adapter.to_string().as_str() { + "reqwest" if !config.reqwest => config.reqwest = true, + "axum" if !config.axum => config.axum = true, + "reqwest" | "axum" => { + return Err(syn::Error::new(adapter.span(), "duplicate API adapter")); + } + _ => { + return Err(syn::Error::new( + adapter.span(), + "unsupported API adapter; expected `reqwest` or `axum`", + )); + } + } + } + Ok(config) + } +} + impl Method { fn from_ident(ident: &Ident) -> Option { match ident.to_string().as_str() { @@ -150,6 +180,7 @@ impl Location { } struct Parameter { + rust_ident: Ident, rust_name: String, wire_name: String, location: Location, @@ -166,6 +197,7 @@ struct Operation { request_body: Option, response_body: Option, error_body: Option, + fallible: bool, response_status: u16, error_status: Option, } @@ -175,18 +207,14 @@ struct ApiDefinition { metadata_ident: Ident, operations_module_ident: Ident, operations: Vec, + adapters: AdapterConfig, } fn normalize_api( attr: proc_macro2::TokenStream, mut item: ItemTrait, ) -> syn::Result { - if !attr.is_empty() { - return Err(syn::Error::new( - attr.span(), - "api does not accept arguments", - )); - } + let adapters = syn::parse2::(attr)?; if !matches!(item.vis, Visibility::Public(_)) { return Err(syn::Error::new_spanned( &item.ident, @@ -272,14 +300,36 @@ fn normalize_api( operations.sort_by(|left, right| left.operation_id.cmp(&right.operation_id)); + if adapters.axum || adapters.reqwest { + make_service_futures_send(&mut item)?; + } + Ok(ApiDefinition { item, metadata_ident, operations_module_ident, operations, + adapters, }) } +fn make_service_futures_send(item: &mut ItemTrait) -> syn::Result<()> { + for trait_item in &mut item.items { + let TraitItem::Fn(method) = trait_item else { + continue; + }; + method.sig.asyncness = None; + let output = match &method.sig.output { + ReturnType::Default => quote!(()), + ReturnType::Type(_, ty) => quote!(#ty), + }; + method.sig.output = syn::parse2(quote!( + -> impl ::core::future::Future + Send + ))?; + } + Ok(()) +} + fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { validate_signature(method)?; @@ -321,7 +371,7 @@ fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { route.operation_id.as_ref().unwrap_or(&route.path), )?; - let (response_body, error_body) = parse_return_type(&method.sig.output)?; + let (response_body, error_body, fallible) = parse_return_type(&method.sig.output)?; if let Some(ty) = response_body.as_ref() { validate_named_body_type(ty, "response")?; } @@ -435,6 +485,7 @@ fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { body_type = Some((*input.ty).clone()); } parameters.push(Parameter { + rust_ident: ident.clone(), rust_name, wire_name, location, @@ -482,6 +533,7 @@ fn normalize_operation(method: &mut TraitItemFn) -> syn::Result { request_body: body_type, response_body, error_body, + fallible, response_status, error_status, }) @@ -657,12 +709,12 @@ fn parse_status(value: Option<&LitInt>, name: &str) -> syn::Result> Ok(Some(status)) } -fn parse_return_type(output: &ReturnType) -> syn::Result<(Option, Option)> { +fn parse_return_type(output: &ReturnType) -> syn::Result<(Option, Option, bool)> { let ReturnType::Type(_, ty) = output else { - return Ok((None, None)); + return Ok((None, None, false)); }; if is_unit(ty) { - return Ok((None, None)); + return Ok((None, None, false)); } if let Type::Path(type_path) = ty.as_ref() @@ -679,8 +731,8 @@ fn parse_return_type(output: &ReturnType) -> syn::Result<(Option, Option Ok((body_type(response), None)), - [response, error] => Ok((body_type(response), body_type(error))), + [response] => Ok((body_type(response), None, true)), + [response, error] => Ok((body_type(response), body_type(error), true)), _ => Err(syn::Error::new_spanned( arguments, "API result types must have one response type and at most one error type", @@ -688,7 +740,7 @@ fn parse_return_type(output: &ReturnType) -> syn::Result<(Option, Option Option { @@ -770,8 +822,450 @@ fn api_crate_path() -> syn::Result { } } +fn reqwest_method_tokens( + method: Method, + api_crate: &proc_macro2::TokenStream, +) -> proc_macro2::TokenStream { + let method = Ident::new(method_name(method), Span::call_site()); + quote!(#api_crate::reqwest::framework::Method::#method) +} + +fn reqwest_path_segment_tokens(segment: &str, operation: &Operation) -> proc_macro2::TokenStream { + let mut pieces = Vec::new(); + let mut remaining = segment; + while let Some(start) = remaining.find('{') { + let literal = &remaining[..start]; + if !literal.is_empty() { + pieces.push(quote!(__segment.push_str(#literal);)); + } + let after = &remaining[start + 1..]; + let end = after + .find('}') + .expect("path placeholders are validated before adapter generation"); + let name = &after[..end]; + let parameter = operation + .parameters + .iter() + .find(|parameter| { + matches!(parameter.location, Location::Path) && parameter.rust_name == name + }) + .expect("path parameter names are validated before adapter generation"); + let ident = ¶meter.rust_ident; + pieces.push(quote! { + __segment.push_str(&::std::string::ToString::to_string(&#ident)); + }); + remaining = &after[end + 1..]; + } + if !remaining.is_empty() { + pieces.push(quote!(__segment.push_str(#remaining);)); + } + quote! {{ + let mut __segment = ::std::string::String::new(); + #(#pieces)* + __segment + }} +} + +fn reqwest_adapter_tokens( + api: &ApiDefinition, + api_crate: &proc_macro2::TokenStream, +) -> proc_macro2::TokenStream { + if !api.adapters.reqwest { + return quote!(); + } + let trait_ident = &api.item.ident; + let visibility = &api.item.vis; + let client_ident = format_ident!("{}Client", ident_text(trait_ident)); + let builder_ident = format_ident!("{}ClientBuilder", ident_text(trait_ident)); + let methods = api.operations.iter().map(|operation| { + let method_ident = &operation.method_ident; + let arguments = operation.parameters.iter().map(|parameter| { + let ident = ¶meter.rust_ident; + let ty = ¶meter.ty; + quote!(#ident: #ty) + }); + let path_segments = operation + .path + .split('/') + .skip(1) + .map(|segment| reqwest_path_segment_tokens(segment, operation)); + let queries = operation.parameters.iter().filter_map(|parameter| { + if !matches!(parameter.location, Location::Query) { + return None; + } + let ident = ¶meter.rust_ident; + Some(quote! { + #api_crate::reqwest::append_query(&mut __url, &#ident) + .map_err(#api_crate::reqwest::ClientError::from)?; + }) + }); + let headers = operation.parameters.iter().filter_map(|parameter| { + if !matches!(parameter.location, Location::Header) { + return None; + } + let ident = ¶meter.rust_ident; + let name = ¶meter.wire_name; + Some(quote! { + #api_crate::reqwest::insert_header(&mut __headers, #name, &#ident) + .map_err(#api_crate::reqwest::ClientError::from)?; + }) + }); + let body = operation + .parameters + .iter() + .find(|parameter| matches!(parameter.location, Location::Body)) + .map(|parameter| { + let ident = ¶meter.rust_ident; + quote! { + ::core::option::Option::Some( + #api_crate::reqwest::encode_json(&#ident) + .map_err(#api_crate::reqwest::ClientError::from)?, + ) + } + }) + .unwrap_or_else(|| quote!(::core::option::Option::None)); + let method = reqwest_method_tokens(operation.method, api_crate); + let response_status = operation.response_status; + let response_type = operation + .response_body + .as_ref() + .map(|ty| quote!(#ty)) + .unwrap_or_else(|| quote!(())); + let error_type = operation + .error_body + .as_ref() + .map(|ty| quote!(#ty)) + .unwrap_or_else(|| quote!(#api_crate::NoBody)); + let success = if let Some(response) = operation.response_body.as_ref() { + quote! { + return __response + .decode_json::<#response>(#api_crate::reqwest::DecodeKind::Success) + .map_err(#api_crate::reqwest::ClientError::from); + } + } else { + quote! { + return __response + .require_empty() + .map_err(#api_crate::reqwest::ClientError::from); + } + }; + let public_error = match (operation.error_status, operation.error_body.as_ref()) { + (Some(status), Some(error)) => quote! { + if __actual == #status { + let __error = __response + .decode_json::<#error>(#api_crate::reqwest::DecodeKind::PublicError) + .map_err(#api_crate::reqwest::ClientError::from)?; + return ::core::result::Result::Err( + #api_crate::reqwest::ClientError::public(__response.status(), __error), + ); + } + }, + _ => quote!(), + }; + quote! { + pub async fn #method_ident( + &self, + #(#arguments),* + ) -> ::core::result::Result<#response_type, #api_crate::reqwest::ClientError<#error_type>> { + let __segments = [#(#path_segments),*]; + let mut __url = self.core.endpoint(&__segments) + .map_err(#api_crate::reqwest::ClientError::from)?; + #(#queries)* + let mut __headers = #api_crate::reqwest::framework::header::HeaderMap::new(); + #(#headers)* + let __body = #body; + let __response = self.core.send(#method, __url, __headers, __body) + .await + .map_err(#api_crate::reqwest::ClientError::from)?; + let __actual = __response.status().as_u16(); + if __actual == #response_status { + #success + } + #public_error + ::core::result::Result::Err(#api_crate::reqwest::ClientError::from( + #api_crate::reqwest::ClientFailure::UnexpectedStatus { + expected: #response_status, + actual: __actual, + }, + )) + } + } + }); + + quote! { + #[doc = concat!("Typed Reqwest client for [`", stringify!(#trait_ident), "`].")] + #visibility struct #client_ident { + core: #api_crate::reqwest::ClientCore, + } + + impl #client_ident<#api_crate::reqwest::NoAuthorizer> { + pub fn try_new( + base_url: impl ::core::convert::AsRef, + ) -> ::core::result::Result { + Self::builder(base_url)?.build() + } + + pub fn builder( + base_url: impl ::core::convert::AsRef, + ) -> ::core::result::Result<#builder_ident, #api_crate::reqwest::ClientBuildError> { + Ok(#builder_ident { + inner: #api_crate::reqwest::ClientBuilder::try_new(base_url)?, + }) + } + + pub fn with_client( + base_url: #api_crate::reqwest::BaseUrl, + client: #api_crate::reqwest::framework::Client, + ) -> Self { + #builder_ident { + inner: #api_crate::reqwest::ClientBuilder::from_base_url(base_url).client(client), + } + .build() + .expect("an injected Reqwest client requires no fallible construction") + } + } + + impl #client_ident { + pub fn from_core(core: #api_crate::reqwest::ClientCore) -> Self { + Self { core } + } + } + + impl #client_ident { + #(#methods)* + } + + impl ::core::fmt::Debug for #client_ident { + fn fmt(&self, formatter: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { + formatter + .debug_struct(stringify!(#client_ident)) + .field("core", &self.core) + .finish() + } + } + + #[doc = concat!("Builder for [`", stringify!(#client_ident), "`].")] + #visibility struct #builder_ident { + inner: #api_crate::reqwest::ClientBuilder, + } + + impl #builder_ident { + pub fn client(mut self, client: #api_crate::reqwest::framework::Client) -> Self { + self.inner = self.inner.client(client); + self + } + + pub fn authorizer(self, authorizer: B) -> #builder_ident { + #builder_ident { inner: self.inner.authorizer(authorizer) } + } + + pub fn request_timeout(mut self, timeout: ::std::time::Duration) -> Self { + self.inner = self.inner.request_timeout(timeout); + self + } + + pub fn response_body_limit(mut self, limit: usize) -> Self { + self.inner = self.inner.response_body_limit(limit); + self + } + + pub fn build( + self, + ) -> ::core::result::Result<#client_ident, #api_crate::reqwest::ClientBuildError> { + self.inner.build().map(#client_ident::from_core) + } + } + + impl ::core::fmt::Debug for #builder_ident { + fn fmt(&self, formatter: &mut ::core::fmt::Formatter<'_>) -> ::core::fmt::Result { + formatter + .debug_struct(stringify!(#builder_ident)) + .field("inner", &self.inner) + .finish() + } + } + } +} + +fn axum_routing_tokens( + method: Method, + api_crate: &proc_macro2::TokenStream, +) -> proc_macro2::TokenStream { + let function = match method { + Method::Get => quote!(get), + Method::Post => quote!(post), + Method::Put => quote!(put), + Method::Patch => quote!(patch), + Method::Delete => quote!(delete), + Method::Head => quote!(head), + Method::Options => quote!(options), + }; + quote!(#api_crate::axum::framework::#function) +} + +fn ordered_path_parameters<'a>(operation: &'a Operation) -> Vec<&'a Parameter> { + let mut result = Vec::new(); + let mut remaining = operation.path.as_str(); + while let Some(start) = remaining.find('{') { + let after = &remaining[start + 1..]; + let Some(end) = after.find('}') else { break }; + let name = &after[..end]; + if let Some(parameter) = operation.parameters.iter().find(|parameter| { + matches!(parameter.location, Location::Path) && parameter.rust_name == name + }) { + result.push(parameter); + } + remaining = &after[end + 1..]; + } + result +} + +fn axum_adapter_tokens( + api: &ApiDefinition, + api_crate: &proc_macro2::TokenStream, +) -> proc_macro2::TokenStream { + if !api.adapters.axum { + return quote!(); + } + let trait_ident = &api.item.ident; + let visibility = &api.item.vis; + let adapter_ident = format_ident!("{}Axum", ident_text(trait_ident)); + let module_ident = format_ident!("{}_axum", to_snake_case(trait_ident)); + let route_functions: Vec<_> = api.operations.iter().map(|operation| { + let method_ident = &operation.method_ident; + let handler_ident = format_ident!("{}_handler", ident_text(method_ident)); + let route_path = &operation.path; + let routing = axum_routing_tokens(operation.method, api_crate); + let path_parameters = ordered_path_parameters(operation); + let path_extractor = match path_parameters.as_slice() { + [] => quote!(), + [parameter] => { + let ident = ¶meter.rust_ident; + let ty = ¶meter.ty; + quote!(#api_crate::axum::framework::Path(#ident): #api_crate::axum::framework::Path<#ty>,) + } + parameters => { + let identifiers = parameters.iter().map(|parameter| ¶meter.rust_ident); + let types = parameters.iter().map(|parameter| ¶meter.ty); + quote!(#api_crate::axum::framework::Path((#(#identifiers),*)): #api_crate::axum::framework::Path<(#(#types),*)>,) + } + }; + let query_extractors = operation.parameters.iter().filter_map(|parameter| { + if !matches!(parameter.location, Location::Query) { + return None; + } + let ident = ¶meter.rust_ident; + let ty = ¶meter.ty; + Some(quote!(#api_crate::axum::framework::Query(#ident): #api_crate::axum::framework::Query<#ty>,)) + }); + let has_headers = operation.parameters.iter().any(|parameter| matches!(parameter.location, Location::Header)); + let header_extractor = has_headers.then(|| quote!(__headers: #api_crate::axum::framework::HeaderMap,)); + let header_parsers = operation.parameters.iter().filter_map(|parameter| { + if !matches!(parameter.location, Location::Header) { + return None; + } + let ident = ¶meter.rust_ident; + let ty = ¶meter.ty; + let name = ¶meter.wire_name; + Some(quote! { + let #ident: #ty = match #api_crate::axum::parse_header(&__headers, #name) { + Ok(value) => value, + Err(status) => return #api_crate::axum::rejection(status), + }; + }) + }); + let body_extractor = operation.parameters.iter().find_map(|parameter| { + if !matches!(parameter.location, Location::Body) { + return None; + } + let ident = ¶meter.rust_ident; + let ty = ¶meter.ty; + Some(quote!(#api_crate::axum::framework::Json(#ident): #api_crate::axum::framework::Json<#ty>,)) + }); + let call_arguments = operation.parameters.iter().map(|parameter| ¶meter.rust_ident); + let response_status = operation.response_status; + let success = if operation.response_body.is_some() { + quote!(#api_crate::axum::json_response(#api_crate::axum::status(#response_status), value)) + } else { + quote!({ let _ = value; #api_crate::axum::empty_response(#api_crate::axum::status(#response_status)) }) + }; + let invocation = quote!(#trait_ident::#method_ident(&*__service, #(#call_arguments),*).await); + let mapped = if operation.fallible { + let error = match operation.error_status { + Some(status) => quote!(#api_crate::axum::json_response(#api_crate::axum::status(#status), error)), + None => quote!({ let _ = error; #api_crate::axum::empty_response(#api_crate::axum::framework::StatusCode::INTERNAL_SERVER_ERROR) }), + }; + quote! { + match #invocation { + Ok(value) => #success, + Err(error) => #error, + } + } + } else { + quote! { + let value = #invocation; + #success + } + }; + quote! { + pub fn #method_ident(service: ::std::sync::Arc) -> #api_crate::axum::framework::Router + where + S: super::#trait_ident + Send + Sync + 'static, + { + #api_crate::axum::framework::Router::new() + .route(#route_path, #routing(#handler_ident::)) + .with_state(service) + } + + async fn #handler_ident( + #api_crate::axum::framework::State(__service): #api_crate::axum::framework::State<::std::sync::Arc>, + #path_extractor + #(#query_extractors)* + #header_extractor + #body_extractor + ) -> #api_crate::axum::framework::Response + where + S: super::#trait_ident + Send + Sync + 'static, + { + #(#header_parsers)* + #mapped + } + } + }).collect(); + let operation_merges = api.operations.iter().map(|operation| { + let method_ident = &operation.method_ident; + quote!(__router = __router.merge(#module_ident::#method_ident(__service.clone()));) + }); + + quote! { + #[doc = concat!("Per-operation Axum routers for [`", stringify!(#trait_ident), "`].")] + #visibility mod #module_ident { + use super::*; + #(#route_functions)* + } + + #[doc = concat!("Axum router adapter for [`", stringify!(#trait_ident), "`].")] + #[derive(Clone, Copy, Debug, Default)] + #visibility struct #adapter_ident; + + impl #adapter_ident { + pub fn router(service: S) -> #api_crate::axum::framework::Router + where + S: #trait_ident + Send + Sync + 'static, + { + let __service = ::std::sync::Arc::new(service); + let mut __router = #api_crate::axum::framework::Router::new(); + #(#operation_merges)* + __router + } + } + } +} + fn expand_api(api: ApiDefinition) -> syn::Result { let api_crate = api_crate_path()?; + let reqwest_adapter = reqwest_adapter_tokens(&api, &api_crate); + let axum_adapter = axum_adapter_tokens(&api, &api_crate); let item = api.item; let metadata_ident = api.metadata_ident; let module_ident = api.operations_module_ident; @@ -848,6 +1342,9 @@ fn expand_api(api: ApiDefinition) -> syn::Result { } #(#operation_impls)* + + #reqwest_adapter + #axum_adapter }) } diff --git a/crates/api-macros/Cargo.toml b/crates/api-macros/Cargo.toml index 73515abad..6839cbcb6 100644 --- a/crates/api-macros/Cargo.toml +++ b/crates/api-macros/Cargo.toml @@ -5,8 +5,30 @@ edition.workspace = true license.workspace = true publish = false +[features] +default = [] +axum = ["dep:axum", "dep:serde"] +reqwest = [ + "dep:futures", + "dep:reqwest", + "dep:serde", + "dep:serde_json", + "dep:serde_urlencoded", + "dep:url", +] + [dependencies] api-macros-impl = { path = "../api-macros-impl" } +axum = { workspace = true, optional = true } +futures = { workspace = true, optional = true } +reqwest = { version = "0.13", default-features = false, optional = true, features = ["stream", "rustls"] } +serde = { workspace = true, optional = true } +serde_json = { workspace = true, optional = true } +serde_urlencoded = { version = "0.7", optional = true } +url = { version = "2", optional = true } [dev-dependencies] +serde = { workspace = true, features = ["derive"] } +serde_json = { workspace = true } +tokio = { workspace = true, features = ["net"] } trybuild = "1" diff --git a/crates/api-macros/src/axum.rs b/crates/api-macros/src/axum.rs new file mode 100644 index 000000000..5298324dc --- /dev/null +++ b/crates/api-macros/src/axum.rs @@ -0,0 +1,61 @@ +//! Axum support used by routers generated with `#[api(axum)]`. +//! +//! Generated operation routers contain only transport extraction, typed service invocation, and +//! response mapping. Authentication/authorization context, body-limit policy, timeout policy, and +//! other application state remain explicit outer Axum layers. Each operation is also exposed as a +//! separate router so route-specific layers can be applied before routers are merged. + +use std::str::FromStr; + +use axum::{ + Json, + http::{HeaderMap, StatusCode}, + response::{IntoResponse, Response}, +}; +use serde::Serialize; + +/// Parse one contract header into its declared Rust type. +pub fn parse_header(headers: &HeaderMap, name: &'static str) -> Result +where + T: FromStr, +{ + headers + .get(name) + .ok_or(StatusCode::BAD_REQUEST)? + .to_str() + .map_err(|_| StatusCode::BAD_REQUEST)? + .parse() + .map_err(|_| StatusCode::BAD_REQUEST) +} + +/// Serialize a typed JSON response with the contract status. +pub fn json_response(status: StatusCode, value: T) -> Response { + (status, Json(value)).into_response() +} + +/// Emit an empty response with the contract status. +pub fn empty_response(status: StatusCode) -> Response { + status.into_response() +} + +/// Emit a transport-level rejection without a response body. +pub fn rejection(status: StatusCode) -> Response { + status.into_response() +} + +/// Convert a status constant from generated metadata. +pub fn status(code: u16) -> StatusCode { + StatusCode::from_u16(code).expect("#[api] validates HTTP status constants") +} + +/// Reexports used by generated router code. +pub mod framework { + pub use axum::{ + Json, Router, + extract::{Path, Query, State}, + http::{HeaderMap, StatusCode}, + response::Response, + routing::{delete, get, head, options, patch, post, put}, + serve, + }; +} diff --git a/crates/api-macros/src/lib.rs b/crates/api-macros/src/lib.rs index 006dd6e4b..2826fbf51 100644 --- a/crates/api-macros/src/lib.rs +++ b/crates/api-macros/src/lib.rs @@ -58,11 +58,35 @@ //! Only empty and JSON bodies are accepted by this first contract. [`WireKind`] reserves //! explicit variants for future transport work; accepting one requires a deliberate macro and //! adapter change rather than silently treating it as JSON. +//! +//! # Optional HTTP adapters +//! +//! `#[api(reqwest)]` generates `TraitNameClient` and enables a typed Reqwest client when this +//! crate's `reqwest` feature is enabled. `#[api(axum)]` generates `TraitNameAxum` plus one public +//! router per operation when the `axum` feature is enabled. `#[api(reqwest, axum)]` emits both +//! from the same normalized definition. A plain `#[api]` remains framework-independent and pulls +//! neither framework into DTO/contract-only builds. +//! +//! Generated clients use a fallible `try_new` constructor and builder. The builder validates an +//! absolute credential-free HTTP(S) base URL and configures request timeout and response byte +//! limit. A prevalidated [`reqwest::BaseUrl`] and policy-configured `reqwest::framework::Client` +//! may be injected instead. Request authorizers receive the final method, encoded path/query, and +//! exact serialized body bytes; credential headers and all body content are redacted from normal +//! diagnostics. +//! +//! Generated Axum adapters keep authentication, authorization, application state, body limits, +//! and timeout layers outside the adapter. Callers may layer the complete router or an individual +//! generated operation router before merging it into their application. extern crate self as api_macros; pub use api_macros_impl::api; +#[cfg(feature = "axum")] +pub mod axum; +#[cfg(feature = "reqwest")] +pub mod reqwest; + /// A transport-neutral HTTP method. #[derive(Clone, Copy, Debug, Eq, Hash, Ord, PartialEq, PartialOrd)] pub enum HttpMethod { diff --git a/crates/api-macros/src/reqwest.rs b/crates/api-macros/src/reqwest.rs new file mode 100644 index 000000000..b45406c9c --- /dev/null +++ b/crates/api-macros/src/reqwest.rs @@ -0,0 +1,563 @@ +//! Reqwest transport support used by clients generated with `#[api(reqwest)]`. +//! +//! The generated builder validates the base URL before it can be stored. Callers that enforce +//! stricter egress policy (custom DNS resolution, proxy policy, TLS roots, or connection pinning) +//! can construct their own [`reqwest::Client`] and inject it with the validated [`BaseUrl`]. + +use std::{fmt, time::Duration}; + +use futures::StreamExt as _; +use reqwest::{ + Client, Method, StatusCode, Url, + header::{HeaderMap, HeaderName, HeaderValue}, +}; +use serde::{Serialize, de::DeserializeOwned}; + +/// Default timeout applied to the complete request, including response-body reads. +pub const DEFAULT_REQUEST_TIMEOUT: Duration = Duration::from_secs(30); +/// Default maximum response body size (1 MiB). +pub const DEFAULT_RESPONSE_BODY_LIMIT: usize = 1024 * 1024; + +/// A parsed HTTP(S) base URL without credentials, query, or fragment. +#[derive(Clone, Eq, PartialEq)] +pub struct BaseUrl(Url); + +impl BaseUrl { + /// Access the validated URL. + pub fn as_url(&self) -> &Url { + &self.0 + } +} + +impl fmt::Debug for BaseUrl { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.debug_tuple("BaseUrl").field(&self.0).finish() + } +} + +impl TryFrom<&str> for BaseUrl { + type Error = ClientBuildError; + + fn try_from(value: &str) -> Result { + Url::parse(value) + .map_err(|_| ClientBuildError::InvalidBaseUrl) + .and_then(Self::try_from) + } +} + +impl TryFrom for BaseUrl { + type Error = ClientBuildError; + + fn try_from(value: String) -> Result { + Self::try_from(value.as_str()) + } +} + +impl TryFrom for BaseUrl { + type Error = ClientBuildError; + + fn try_from(mut value: Url) -> Result { + if !matches!(value.scheme(), "http" | "https") + || value.cannot_be_a_base() + || value.host_str().is_none() + || !value.username().is_empty() + || value.password().is_some() + || value.query().is_some() + || value.fragment().is_some() + { + return Err(ClientBuildError::InvalidBaseUrl); + } + if !value.path().ends_with('/') { + let mut path = value.path().to_owned(); + path.push('/'); + value.set_path(&path); + } + Ok(Self(value)) + } +} + +/// Failure while constructing a generated client. +#[derive(Clone, Copy, Eq, PartialEq)] +pub enum ClientBuildError { + /// The base URL was not an absolute credential-free HTTP(S) URL without query or fragment. + InvalidBaseUrl, + /// Reqwest could not construct its default client. + ClientConstruction, +} + +impl fmt::Debug for ClientBuildError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, formatter) + } +} + +impl fmt::Display for ClientBuildError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(match self { + Self::InvalidBaseUrl => "invalid HTTP client base URL", + Self::ClientConstruction => "failed to construct HTTP client", + }) + } +} + +impl std::error::Error for ClientBuildError {} + +/// Exact canonical request material supplied to a [`RequestAuthorizer`]. +/// +/// `body` is the same byte slice subsequently passed to Reqwest. `path_and_query` is taken from +/// the final URL after path-segment and query encoding. +pub struct AuthorizerRequest<'a> { + pub method: &'a Method, + pub path_and_query: &'a str, + pub body: &'a [u8], +} + +impl fmt::Debug for AuthorizerRequest<'_> { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("AuthorizerRequest") + .field("method", self.method) + .field("path_and_query", &"") + .field("body", &"") + .finish() + } +} + +/// Opaque authorizer failure. Secret-bearing authorizer errors are intentionally not retained. +#[derive(Clone, Copy, Default, Eq, PartialEq)] +pub struct AuthorizationError(()); + +impl AuthorizationError { + pub const fn new() -> Self { + Self(()) + } +} + +impl fmt::Debug for AuthorizationError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("AuthorizationError") + } +} + +impl fmt::Display for AuthorizationError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("request authorization failed") + } +} + +impl std::error::Error for AuthorizationError {} + +/// Supplies credential headers for an exact encoded request. +pub trait RequestAuthorizer: Send + Sync { + fn authorize(&self, request: AuthorizerRequest<'_>) -> Result; +} + +/// Authorizer used by default; it adds no headers. +#[derive(Clone, Copy, Default)] +pub struct NoAuthorizer; + +impl RequestAuthorizer for NoAuthorizer { + fn authorize(&self, _request: AuthorizerRequest<'_>) -> Result { + Ok(HeaderMap::new()) + } +} + +impl fmt::Debug for NoAuthorizer { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("NoAuthorizer") + } +} + +/// Builder shared by generated API-specific client builders. +pub struct ClientBuilder { + base_url: BaseUrl, + client: Option, + authorizer: A, + request_timeout: Duration, + response_body_limit: usize, +} + +impl ClientBuilder { + pub fn try_new(base_url: impl AsRef) -> Result { + Ok(Self::from_base_url(BaseUrl::try_from(base_url.as_ref())?)) + } + + pub fn from_base_url(base_url: BaseUrl) -> Self { + Self { + base_url, + client: None, + authorizer: NoAuthorizer, + request_timeout: DEFAULT_REQUEST_TIMEOUT, + response_body_limit: DEFAULT_RESPONSE_BODY_LIMIT, + } + } +} + +impl ClientBuilder { + pub fn client(mut self, client: Client) -> Self { + self.client = Some(client); + self + } + + pub fn authorizer(self, authorizer: B) -> ClientBuilder { + ClientBuilder { + base_url: self.base_url, + client: self.client, + authorizer, + request_timeout: self.request_timeout, + response_body_limit: self.response_body_limit, + } + } + + pub fn request_timeout(mut self, timeout: Duration) -> Self { + self.request_timeout = timeout; + self + } + + pub fn response_body_limit(mut self, limit: usize) -> Self { + self.response_body_limit = limit; + self + } + + pub fn build(self) -> Result, ClientBuildError> { + let client = match self.client { + Some(client) => client, + None => Client::builder() + .build() + .map_err(|_| ClientBuildError::ClientConstruction)?, + }; + Ok(ClientCore { + base_url: self.base_url, + client, + authorizer: self.authorizer, + request_timeout: self.request_timeout, + response_body_limit: self.response_body_limit, + }) + } +} + +impl fmt::Debug for ClientBuilder { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("ClientBuilder") + .field("base_url", &self.base_url) + .field("client", &"") + .field("authorizer", &"") + .field("request_timeout", &self.request_timeout) + .field("response_body_limit", &self.response_body_limit) + .finish() + } +} + +/// Framework support held by each generated API client. +pub struct ClientCore { + base_url: BaseUrl, + client: Client, + authorizer: A, + request_timeout: Duration, + response_body_limit: usize, +} + +impl Clone for ClientCore { + fn clone(&self) -> Self { + Self { + base_url: self.base_url.clone(), + client: self.client.clone(), + authorizer: self.authorizer.clone(), + request_timeout: self.request_timeout, + response_body_limit: self.response_body_limit, + } + } +} + +impl fmt::Debug for ClientCore { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("ClientCore") + .field("base_url", &self.base_url) + .field("client", &"") + .field("authorizer", &"") + .field("request_timeout", &self.request_timeout) + .field("response_body_limit", &self.response_body_limit) + .finish() + } +} + +impl ClientCore { + pub fn from_parts( + base_url: BaseUrl, + client: Client, + authorizer: A, + request_timeout: Duration, + response_body_limit: usize, + ) -> Self { + Self { + base_url, + client, + authorizer, + request_timeout, + response_body_limit, + } + } + + /// Build an endpoint by appending already-separated, unencoded path segments. + pub fn endpoint(&self, segments: &[String]) -> Result { + let mut url = self.base_url.0.clone(); + let mut path = url + .path_segments_mut() + .map_err(|_| ClientFailure::RequestEncoding)?; + path.pop_if_empty(); + for segment in segments { + path.push(segment); + } + drop(path); + Ok(url) + } + + pub fn response_body_limit(&self) -> usize { + self.response_body_limit + } +} + +impl ClientCore { + pub async fn send( + &self, + method: Method, + url: Url, + mut headers: HeaderMap, + body: Option>, + ) -> Result { + let body_bytes = body.as_deref().unwrap_or_default(); + let path_and_query = &url[url::Position::BeforePath..url::Position::AfterQuery]; + let authorization = self + .authorizer + .authorize(AuthorizerRequest { + method: &method, + path_and_query, + body: body_bytes, + }) + .map_err(|_| ClientFailure::Authorization)?; + headers.extend(authorization); + + let mut request = self + .client + .request(method, url) + .headers(headers) + .timeout(self.request_timeout); + if let Some(body) = body { + request = request + .header(reqwest::header::CONTENT_TYPE, "application/json") + .body(body); + } + let response = request.send().await.map_err(|error| { + if error.is_timeout() { + ClientFailure::Timeout + } else { + ClientFailure::Transport + } + })?; + let status = response.status(); + let mut stream = response.bytes_stream(); + let mut bytes = Vec::new(); + while let Some(chunk) = stream.next().await { + let chunk = chunk.map_err(|error| { + if error.is_timeout() { + ClientFailure::Timeout + } else { + ClientFailure::Transport + } + })?; + let Some(size) = bytes.len().checked_add(chunk.len()) else { + return Err(ClientFailure::ResponseTooLarge { + limit: self.response_body_limit, + }); + }; + if size > self.response_body_limit { + return Err(ClientFailure::ResponseTooLarge { + limit: self.response_body_limit, + }); + } + bytes.extend_from_slice(&chunk); + } + Ok(ReceivedResponse { status, bytes }) + } +} + +/// Encode a serializable query DTO and append its pairs without changing existing pairs. +pub fn append_query(url: &mut Url, value: &T) -> Result<(), ClientFailure> { + let encoded = serde_urlencoded::to_string(value).map_err(|_| ClientFailure::RequestEncoding)?; + if encoded.is_empty() { + return Ok(()); + } + let combined = match url.query() { + Some(existing) if !existing.is_empty() => format!("{existing}&{encoded}"), + _ => encoded, + }; + url.set_query(Some(&combined)); + Ok(()) +} + +/// Insert a string-form header value. +pub fn insert_header( + headers: &mut HeaderMap, + name: &str, + value: &T, +) -> Result<(), ClientFailure> { + let name = + HeaderName::from_bytes(name.as_bytes()).map_err(|_| ClientFailure::RequestEncoding)?; + let value = + HeaderValue::from_str(&value.to_string()).map_err(|_| ClientFailure::RequestEncoding)?; + headers.insert(name, value); + Ok(()) +} + +/// Serialize the request body once. These exact bytes are authorized and transmitted. +pub fn encode_json(value: &T) -> Result, ClientFailure> { + serde_json::to_vec(value).map_err(|_| ClientFailure::RequestEncoding) +} + +/// Response received under the configured byte limit. Its content is always redacted in Debug. +pub struct ReceivedResponse { + status: StatusCode, + bytes: Vec, +} + +impl ReceivedResponse { + pub fn status(&self) -> StatusCode { + self.status + } + + pub fn decode_json(&self, kind: DecodeKind) -> Result { + serde_json::from_slice(&self.bytes).map_err(|_| ClientFailure::Decode { kind }) + } + + pub fn require_empty(&self) -> Result<(), ClientFailure> { + if self.bytes.is_empty() { + Ok(()) + } else { + Err(ClientFailure::UnexpectedBody) + } + } +} + +impl fmt::Debug for ReceivedResponse { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter + .debug_struct("ReceivedResponse") + .field("status", &self.status) + .field("bytes", &"") + .finish() + } +} + +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub enum DecodeKind { + Success, + PublicError, +} + +/// Non-public-error failure from a generated client. +#[derive(Clone, Copy, Eq, PartialEq)] +pub enum ClientFailure { + Authorization, + RequestEncoding, + Transport, + Timeout, + ResponseTooLarge { limit: usize }, + UnexpectedStatus { expected: u16, actual: u16 }, + Decode { kind: DecodeKind }, + UnexpectedBody, +} + +impl fmt::Debug for ClientFailure { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + fmt::Display::fmt(self, formatter) + } +} + +impl fmt::Display for ClientFailure { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Authorization => formatter.write_str("request authorization failed"), + Self::RequestEncoding => formatter.write_str("request encoding failed"), + Self::Transport => formatter.write_str("HTTP transport failed"), + Self::Timeout => formatter.write_str("HTTP request timed out"), + Self::ResponseTooLarge { limit } => { + write!(formatter, "response body exceeded {limit} bytes") + } + Self::UnexpectedStatus { expected, actual } => { + write!( + formatter, + "expected HTTP status {expected}, received {actual}" + ) + } + Self::Decode { + kind: DecodeKind::Success, + } => formatter.write_str("success response JSON was malformed"), + Self::Decode { + kind: DecodeKind::PublicError, + } => formatter.write_str("public error response JSON was malformed"), + Self::UnexpectedBody => formatter.write_str("empty response contained a body"), + } + } +} + +impl std::error::Error for ClientFailure {} + +/// Typed error returned by generated client methods. +pub enum ClientError { + Failure(ClientFailure), + Public { status: StatusCode, error: E }, +} + +impl ClientError { + pub fn failure(failure: ClientFailure) -> Self { + Self::Failure(failure) + } + + pub fn public(status: StatusCode, error: E) -> Self { + Self::Public { status, error } + } + + pub fn into_public_error(self) -> Option { + match self { + Self::Public { error, .. } => Some(error), + Self::Failure(_) => None, + } + } +} + +impl From for ClientError { + fn from(value: ClientFailure) -> Self { + Self::Failure(value) + } +} + +impl fmt::Debug for ClientError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Failure(failure) => formatter.debug_tuple("Failure").field(failure).finish(), + Self::Public { status, .. } => formatter + .debug_struct("Public") + .field("status", status) + .field("error", &"") + .finish(), + } + } +} + +impl fmt::Display for ClientError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Failure(failure) => fmt::Display::fmt(failure, formatter), + Self::Public { status, .. } => write!(formatter, "public HTTP error {status}"), + } + } +} + +impl std::error::Error for ClientError {} + +/// Reexports used by generated code and advanced client construction. +pub mod framework { + pub use reqwest::{Client, Method, StatusCode, Url, header}; +} diff --git a/crates/api-macros/tests/http_adapters.rs b/crates/api-macros/tests/http_adapters.rs new file mode 100644 index 000000000..542d2122d --- /dev/null +++ b/crates/api-macros/tests/http_adapters.rs @@ -0,0 +1,277 @@ +#![cfg(all(feature = "reqwest", feature = "axum"))] + +use std::{ + sync::{Arc, Mutex}, + time::Duration, +}; + +use api_macros::{api, reqwest as api_reqwest}; +use serde::{Deserialize, Serialize}; + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct CreateWidget { + name: String, +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct Lookup { + mode: String, +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct Widget { + id: u64, + name: String, + mode: String, +} + +#[derive(Clone, Debug, Deserialize, Eq, PartialEq, Serialize)] +pub struct PublicError { + message: String, +} + +#[api(reqwest, axum)] +pub trait WidgetApi { + #[post( + "/widgets/{widget_id}", + operation_id = "widgets.create", + status = 201, + error_status = 422 + )] + async fn create( + &self, + #[path] widget_id: u64, + #[query] lookup: Lookup, + #[header("authorization")] authorization: String, + #[body] request: CreateWidget, + ) -> Result; + + #[delete("/widgets/{widget_id}", operation_id = "widgets.delete", status = 204)] + async fn delete(&self, #[path] widget_id: u64) -> (); +} + +#[derive(Clone)] +pub struct WidgetService; + +impl WidgetApi for WidgetService { + async fn create( + &self, + widget_id: u64, + lookup: Lookup, + authorization: String, + request: CreateWidget, + ) -> Result { + assert_eq!(authorization, "Bearer top-secret"); + if request.name == "bad" { + return Err(PublicError { + message: "sensitive-public-detail".to_owned(), + }); + } + Ok(Widget { + id: widget_id, + name: request.name, + mode: lookup.mode, + }) + } + + async fn delete(&self, _widget_id: u64) {} +} + +#[api(reqwest)] +pub trait FailureApi { + #[get("/malformed", operation_id = "failure.malformed", status = 200)] + async fn malformed(&self) -> Widget; + + #[get("/oversized", operation_id = "failure.oversized", status = 200)] + async fn oversized(&self) -> Widget; + + #[get("/slow", operation_id = "failure.slow", status = 200)] + async fn slow(&self) -> Widget; +} + +#[derive(Clone, Debug, Eq, PartialEq)] +pub struct AuthorizedRequest { + method: String, + path_and_query: String, + body: Vec, +} + +#[derive(Clone)] +pub struct RecordingAuthorizer { + request: Arc>>, + credential: String, +} + +impl api_reqwest::RequestAuthorizer for RecordingAuthorizer { + fn authorize( + &self, + request: api_reqwest::AuthorizerRequest<'_>, + ) -> Result { + *self.request.lock().expect("recording lock") = Some(AuthorizedRequest { + method: request.method.to_string(), + path_and_query: request.path_and_query.to_owned(), + body: request.body.to_vec(), + }); + let mut headers = api_reqwest::framework::header::HeaderMap::new(); + headers.insert( + api_reqwest::framework::header::AUTHORIZATION, + self.credential.parse().expect("valid fixture credential"), + ); + Ok(headers) + } +} + +async fn malformed() -> &'static str { + "this is not json" +} + +async fn oversized() -> String { + "response-content-that-must-never-appear-in-debug-output".to_owned() +} + +async fn slow() -> &'static str { + tokio::time::sleep(Duration::from_millis(100)).await; + "{}" +} + +#[tokio::test] +async fn generated_client_and_router_follow_the_normalized_contract() { + let app = WidgetApiAxum::router(WidgetService).merge( + api_macros::axum::framework::Router::new() + .route("/malformed", api_macros::axum::framework::get(malformed)) + .route("/oversized", api_macros::axum::framework::get(oversized)) + .route("/slow", api_macros::axum::framework::get(slow)), + ); + let listener = tokio::net::TcpListener::bind("127.0.0.1:0") + .await + .expect("bind fixture server"); + let address = listener.local_addr().expect("fixture address"); + let server = tokio::spawn(async move { + api_macros::axum::framework::serve(listener, app) + .await + .expect("serve fixture API"); + }); + let base_url = format!("http://{address}/"); + + let recorded = Arc::new(Mutex::new(None)); + let client = WidgetApiClient::builder(&base_url) + .expect("validated base URL") + .authorizer(RecordingAuthorizer { + request: recorded.clone(), + credential: "Bearer top-secret".to_owned(), + }) + .build() + .expect("client build"); + + let widget = client + .create( + 7, + Lookup { + mode: "full".to_owned(), + }, + "placeholder-overwritten-by-authorizer".to_owned(), + CreateWidget { + name: "alpha".to_owned(), + }, + ) + .await + .expect("typed success response"); + assert_eq!( + widget, + Widget { + id: 7, + name: "alpha".to_owned(), + mode: "full".to_owned(), + } + ); + assert_eq!( + recorded.lock().expect("recording lock").clone(), + Some(AuthorizedRequest { + method: "POST".to_owned(), + path_and_query: "/widgets/7?mode=full".to_owned(), + body: br#"{"name":"alpha"}"#.to_vec(), + }) + ); + assert!(!format!("{client:?}").contains("top-secret")); + + client.delete(7).await.expect("typed empty response"); + + let public_error = client + .create( + 8, + Lookup { + mode: "full".to_owned(), + }, + "placeholder".to_owned(), + CreateWidget { + name: "bad".to_owned(), + }, + ) + .await + .expect_err("typed public error"); + assert!(!format!("{public_error:?}").contains("sensitive-public-detail")); + assert_eq!( + public_error.into_public_error(), + Some(PublicError { + message: "sensitive-public-detail".to_owned(), + }) + ); + + let malformed_error = FailureApiClient::try_new(&base_url) + .expect("failure client") + .malformed() + .await + .expect_err("malformed JSON must fail"); + assert_eq!( + format!("{malformed_error:?}"), + "Failure(success response JSON was malformed)" + ); + assert!(!format!("{malformed_error:?}").contains("this is not json")); + + let oversized_error = FailureApiClient::builder(&base_url) + .expect("failure client builder") + .response_body_limit(8) + .build() + .expect("failure client") + .oversized() + .await + .expect_err("oversized body must fail"); + assert!(matches!( + oversized_error, + api_reqwest::ClientError::Failure(api_reqwest::ClientFailure::ResponseTooLarge { + limit: 8 + }) + )); + assert!(!format!("{oversized_error:?}").contains("response-content")); + + let timeout_error = FailureApiClient::builder(&base_url) + .expect("failure client builder") + .request_timeout(Duration::from_millis(10)) + .build() + .expect("failure client") + .slow() + .await + .expect_err("slow response must time out"); + assert!(matches!( + timeout_error, + api_reqwest::ClientError::Failure(api_reqwest::ClientFailure::Timeout) + )); + + let _route_specific = widget_api_axum::create(Arc::new(WidgetService)); + server.abort(); +} + +#[test] +fn base_urls_are_strict_and_named_adapter_types_are_public() { + assert!(api_reqwest::BaseUrl::try_from("ftp://example.test/").is_err()); + assert!(api_reqwest::BaseUrl::try_from("https://user@example.test/").is_err()); + assert!(api_reqwest::BaseUrl::try_from("https://example.test/?token=secret").is_err()); + assert!(api_reqwest::BaseUrl::try_from("https://example.test/#fragment").is_err()); + + fn assert_client(_: &WidgetApiClient) {} + fn assert_builder(_: &WidgetApiClientBuilder) {} + let client = WidgetApiClient::try_new("https://example.test/").expect("client"); + let builder = WidgetApiClient::builder("https://example.test/").expect("builder"); + assert_client(&client); + assert_builder(&builder); +} From 1d7cd9339c61b40797ecae4875aab5f8e269cd25 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 00:08:00 +0900 Subject: [PATCH 06/64] Implement transactional Worker restore outcomes --- crates/client/src/backend_runtime.rs | 1 + crates/client/src/lib.rs | 2 +- crates/tui/src/backend_worker_picker.rs | 19 +- crates/worker-runtime/src/execution.rs | 30 + crates/worker-runtime/src/http_server.rs | 37 +- crates/worker-runtime/src/runtime.rs | 522 ++++++++++++++---- crates/worker-runtime/src/worker_backend.rs | 137 ++++- crates/workspace-api/src/lib.rs | 59 +- crates/workspace-server/src/hosts.rs | 88 ++- crates/workspace-server/src/server.rs | 18 +- .../src/lib/workspace/sidebar/types.ts | 13 + 11 files changed, 739 insertions(+), 187 deletions(-) diff --git a/crates/client/src/backend_runtime.rs b/crates/client/src/backend_runtime.rs index 8ec2ea84a..be87e70d4 100644 --- a/crates/client/src/backend_runtime.rs +++ b/crates/client/src/backend_runtime.rs @@ -17,6 +17,7 @@ pub use workspace_api::{ WorkerLaunchProfileCandidate as BackendWorkerLaunchProfileCandidate, WorkerLaunchRuntimeOption as BackendWorkerLaunchRuntimeOption, WorkerOperationState as BackendWorkerOperationState, + WorkerRestoreState as BackendWorkerRestoreState, WorkerRestoreResponse as BackendWorkerRestoreResponse, WorkerRestoreResult as BackendWorkerRestoreResult, WorkerSummary as BackendWorkerSummary, WorkerWorkspaceSummary as BackendWorkerWorkspaceSummary, diff --git a/crates/client/src/lib.rs b/crates/client/src/lib.rs index 814e08cdf..005b126f1 100644 --- a/crates/client/src/lib.rs +++ b/crates/client/src/lib.rs @@ -27,7 +27,7 @@ pub use backend_runtime::{ BackendWorkerCapabilitySummary, BackendWorkerImplementationSummary, BackendWorkerLaunchOptions, BackendWorkerLaunchProfileCandidate, BackendWorkerLaunchRuntimeOption, BackendWorkerLaunchTarget, BackendWorkerOperationState, BackendWorkerRestoreResponse, - BackendWorkerRestoreResult, BackendWorkerSummary, BackendWorkerWorkspaceSummary, + BackendWorkerRestoreResult, BackendWorkerRestoreState, BackendWorkerSummary, BackendWorkerWorkspaceSummary, BackendWorkingDirectorySummary, connect_backend_runtime, create_backend_worker, get_backend_worker_launch_options, list_backend_stopped_workers, list_backend_workers, restore_backend_worker, diff --git a/crates/tui/src/backend_worker_picker.rs b/crates/tui/src/backend_worker_picker.rs index ac7b2a7bb..34ce59560 100644 --- a/crates/tui/src/backend_worker_picker.rs +++ b/crates/tui/src/backend_worker_picker.rs @@ -3,7 +3,7 @@ use std::io; use std::time::Duration; use client::{ - BackendRuntimeListTarget, BackendWorkerOperationState, BackendWorkerRestoreResponse, + BackendRuntimeListTarget, BackendWorkerRestoreResponse, BackendWorkerRestoreState, BackendWorkerSummary, list_backend_stopped_workers, list_backend_workers, restore_backend_worker, }; @@ -110,7 +110,7 @@ pub(crate) async fn run( } fn restored_worker(response: BackendWorkerRestoreResponse) -> Result { - if response.result.state != BackendWorkerOperationState::Accepted { + if response.result.state != BackendWorkerRestoreState::Accepted { let diagnostics = response .result .diagnostics @@ -119,9 +119,10 @@ fn restored_worker(response: BackendWorkerRestoreResponse) -> Result>() .join("; "); let state = match response.result.state { - BackendWorkerOperationState::Accepted => unreachable!(), - BackendWorkerOperationState::Rejected => "rejected", - BackendWorkerOperationState::Unsupported => "unsupported", + BackendWorkerRestoreState::Accepted => unreachable!(), + BackendWorkerRestoreState::Rejected => "rejected", + BackendWorkerRestoreState::RolledBack => "rolled back", + BackendWorkerRestoreState::ReconciliationRequired => "reconciliation required", }; return Err(if diagnostics.is_empty() { format!("restore was {state} without a diagnostic") @@ -494,7 +495,7 @@ mod tests { } fn restore_response( - state: BackendWorkerOperationState, + state: BackendWorkerRestoreState, worker: Option, diagnostics: Vec, ) -> BackendWorkerRestoreResponse { @@ -513,7 +514,7 @@ mod tests { #[test] fn rejected_restore_surfaces_diagnostic_instead_of_attaching_selected_worker() { let error = restored_worker(restore_response( - BackendWorkerOperationState::Rejected, + BackendWorkerRestoreState::Rejected, None, vec![BackendDiagnostic { code: "working_directory_not_found".to_string(), @@ -532,7 +533,7 @@ mod tests { #[test] fn accepted_restore_requires_returned_worker_snapshot() { let error = restored_worker(restore_response( - BackendWorkerOperationState::Accepted, + BackendWorkerRestoreState::Accepted, None, Vec::new(), )) @@ -545,7 +546,7 @@ mod tests { fn accepted_restore_returns_authoritative_worker_snapshot() { let worker = worker("runtime-a", "worker-a", Some("builtin:companion")); let restored = restored_worker(restore_response( - BackendWorkerOperationState::Accepted, + BackendWorkerRestoreState::Accepted, Some(worker.clone()), Vec::new(), )) diff --git a/crates/worker-runtime/src/execution.rs b/crates/worker-runtime/src/execution.rs index 0b0aaae34..dab47f3f3 100644 --- a/crates/worker-runtime/src/execution.rs +++ b/crates/worker-runtime/src/execution.rs @@ -286,7 +286,21 @@ pub enum WorkerExecutionSpawnResult { worker_state: protocol::WorkerStateSnapshot, working_directory: Option, }, + /// A read-only preflight rejected the operation before live work started. Rejected(WorkerExecutionResult), + /// Live work started, failed, and all operation-owned resources were joined/released. + RolledBack(WorkerExecutionResult), + /// The operation crossed the live side-effect boundary and could not prove + /// either commit or complete cleanup. Optional handle/state fields preserve + /// concrete execution evidence for retry reconciliation. + ReconciliationRequired { + result: WorkerExecutionResult, + handle: Option, + worker_state: Option, + working_directory: Option, + }, + /// Legacy spawn failure. Restore implementations must use `RolledBack` or + /// `ReconciliationRequired` after crossing their live side-effect boundary. Errored(WorkerExecutionResult), } @@ -316,6 +330,15 @@ pub trait WorkerExecutionBackend: Send + Sync + 'static { fn spawn_worker(&self, request: WorkerExecutionSpawnRequest) -> WorkerExecutionSpawnResult; + /// Read-only restore validation performed before any live controller, + /// bridge, task, or external registration is created. + fn preflight_restore( + &self, + _request: &WorkerExecutionRestoreRequest, + ) -> Result<(), WorkerExecutionResult> { + Ok(()) + } + fn restore_worker( &self, _request: WorkerExecutionRestoreRequest, @@ -509,6 +532,13 @@ impl WorkerExecutionBackendRef { self.backend.spawn_worker(request) } + pub(crate) fn preflight_restore( + &self, + request: &WorkerExecutionRestoreRequest, + ) -> Result<(), WorkerExecutionResult> { + self.backend.preflight_restore(request) + } + pub(crate) fn restore_worker( &self, request: WorkerExecutionRestoreRequest, diff --git a/crates/worker-runtime/src/http_server.rs b/crates/worker-runtime/src/http_server.rs index 9f631ca3f..d5dc7e296 100644 --- a/crates/worker-runtime/src/http_server.rs +++ b/crates/worker-runtime/src/http_server.rs @@ -60,6 +60,7 @@ use protocol::subscription::{ SubscriptionTerminationCode, }; use serde::{Deserialize, Serialize}; +use workspace_api::WorkerRestoreState; use std::collections::HashMap; use std::fmt; use std::net::SocketAddr; @@ -420,6 +421,20 @@ pub struct RuntimeHttpWorkerResponse { pub worker: WorkerDetail, } +/// Explicit restore outcome. This envelope is returned for every reachable +/// Runtime restore attempt, including preflight rejection and uncertain commit. +#[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] +#[serde(deny_unknown_fields)] +pub struct RuntimeHttpWorkerRestoreResponse { + pub state: WorkerRestoreState, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub worker: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub reason_code: Option, + #[serde(default, skip_serializing_if = "Option::is_none")] + pub message: Option, +} + /// Replace the Workspace API binding for an existing Worker. #[derive(Clone, Debug, PartialEq, Eq, Serialize, Deserialize)] pub struct RuntimeHttpWorkerWorkspaceApiRequest { @@ -1199,14 +1214,21 @@ async fn restore_worker( State(state): State, auth: Option>, Path(worker_id): Path, -) -> RestResult { +) -> RestResult { let worker_ref = worker_ref_for(&state.runtime, worker_id)?; - let worker = match auth_workspace_scope(&state, auth.as_ref())? { - Some(scope) => state.runtime.restore_worker_scoped(&scope, &worker_ref), - None => state.runtime.restore_worker(&worker_ref), + let result = match auth_workspace_scope(&state, auth.as_ref())? { + Some(scope) => state + .runtime + .restore_worker_operation_scoped(&scope, &worker_ref), + None => state.runtime.restore_worker_operation(&worker_ref), } .map_err(RuntimeHttpRestError::runtime)?; - Ok(Json(RuntimeHttpWorkerResponse { worker })) + Ok(Json(RuntimeHttpWorkerRestoreResponse { + state: result.state, + worker: result.worker, + reason_code: result.reason_code, + message: result.message, + })) } #[cfg(feature = "ws-server")] @@ -3322,8 +3344,9 @@ mod tests { ) .await; assert_eq!(response.status(), StatusCode::OK); - let restored: RuntimeHttpWorkerResponse = read_json(response).await; - assert_eq!(restored.worker.status, WorkerStatus::Idle); + let restored: RuntimeHttpWorkerRestoreResponse = read_json(response).await; + assert_eq!(restored.state, WorkerRestoreState::Accepted); + assert_eq!(restored.worker.unwrap().status, WorkerStatus::Idle); let response = authed_empty_request( app.clone(), diff --git a/crates/worker-runtime/src/runtime.rs b/crates/worker-runtime/src/runtime.rs index 18352c690..85406de48 100644 --- a/crates/worker-runtime/src/runtime.rs +++ b/crates/worker-runtime/src/runtime.rs @@ -54,6 +54,7 @@ use std::sync::{Arc, Mutex, MutexGuard, Weak}; use tokio::sync::broadcast; use tokio::sync::mpsc; use uuid::Uuid; +use workspace_api::WorkerRestoreState; /// Workspace-scoped Runtime authorization context supplied by a trusted backend. #[derive(Clone, Debug, Eq, PartialEq)] @@ -83,6 +84,47 @@ enum WorkerRestoreMode { Automatic, } +/// Runtime-internal restore result consumed by both embedded and HTTP +/// providers. Diagnostics are stable and path-free; detailed backend errors +/// remain in Runtime logs rather than crossing the API boundary. +#[derive(Clone, Debug)] +pub struct RuntimeWorkerRestoreResult { + pub state: WorkerRestoreState, + pub worker: Option, + pub reason_code: Option, + pub message: Option, +} + +impl RuntimeWorkerRestoreResult { + fn accepted(worker: WorkerDetail) -> Self { + Self { + state: WorkerRestoreState::Accepted, + worker: Some(worker), + reason_code: None, + message: None, + } + } + + fn failed(state: WorkerRestoreState, reason_code: &'static str, message: &'static str) -> Self { + Self { + state, + worker: None, + reason_code: Some(reason_code.to_string()), + message: Some(message.to_string()), + } + } + + fn into_legacy_result(self) -> Result { + match (self.state, self.worker) { + (WorkerRestoreState::Accepted, Some(worker)) => Ok(worker), + _ => Err(RuntimeError::InvalidRequest( + self.message + .unwrap_or_else(|| "Worker restore did not commit".to_string()), + )), + } + } +} + #[derive(Clone, Debug)] pub struct RuntimeSubscriptionUpdate { pub subject_revision: u64, @@ -1047,6 +1089,7 @@ impl Runtime { working_directory, } => (handle, worker_state, working_directory), WorkerExecutionSpawnResult::Rejected(result) + | WorkerExecutionSpawnResult::RolledBack(result) | WorkerExecutionSpawnResult::Errored(result) => { self.rollback_failed_create(&worker_ref)?; return Err(RuntimeError::WorkerExecutionRejected { @@ -1057,6 +1100,16 @@ impl Runtime { result, }); } + WorkerExecutionSpawnResult::ReconciliationRequired { result, .. } => { + self.rollback_failed_create(&worker_ref)?; + return Err(RuntimeError::WorkerExecutionRejected { + worker_id: worker_ref.worker_id.clone(), + operation: result.operation, + outcome: result.outcome, + message: result.message_or_default(), + result, + }); + } }; if let Some(mut initial_input) = { @@ -1377,7 +1430,16 @@ impl Runtime { scope: &RuntimeWorkspaceScope, worker_ref: &WorkerRef, ) -> Result { - self.restore_worker_with_scope(worker_ref, Some(scope)) + self.restore_worker_operation_scoped(scope, worker_ref)? + .into_legacy_result() + } + + pub fn restore_worker_operation_scoped( + &self, + scope: &RuntimeWorkspaceScope, + worker_ref: &WorkerRef, + ) -> Result { + self.restore_worker_operation_with_scope(worker_ref, Some(scope)) } /// Attach a live execution to a persisted Worker definition. @@ -1387,14 +1449,21 @@ impl Runtime { /// converges on its already-installed execution rather than spawning another /// controller. pub fn restore_worker(&self, worker_ref: &WorkerRef) -> Result { - self.restore_worker_with_scope(worker_ref, None) + self.restore_worker_operation(worker_ref)?.into_legacy_result() } - fn restore_worker_with_scope( + pub fn restore_worker_operation( + &self, + worker_ref: &WorkerRef, + ) -> Result { + self.restore_worker_operation_with_scope(worker_ref, None) + } + + fn restore_worker_operation_with_scope( &self, worker_ref: &WorkerRef, scope: Option<&RuntimeWorkspaceScope>, - ) -> Result { + ) -> Result { let operation_lock = self.worker_operation_lock(worker_ref.worker_id)?; let _operation_guard = operation_lock .lock() @@ -1409,58 +1478,61 @@ impl Runtime { &self, worker_ref: &WorkerRef, mode: WorkerRestoreMode, - ) -> Result { + ) -> Result { let (backend, request) = { - let mut state = self.lock()?; + let state = self.lock()?; state.ensure_running()?; - let (worker_request, previous_working_directory) = { - let worker = state.worker(worker_ref)?; - if worker.execution_handle.is_some() { - if worker.status.is_active() { - return Ok(worker.detail()); - } - return Err(RuntimeError::WorkerExecutionUnavailable { - worker_id: worker_ref.worker_id, - message: "previous execution cleanup has not completed".to_string(), - }); - } - match mode { - WorkerRestoreMode::Explicit - if worker.execution_metadata_available - && worker.status != WorkerStatus::Stopped => - { - return Err(RuntimeError::InvalidRequest(format!( - "worker {} is not stopped", - worker_ref.worker_id - ))); - } - WorkerRestoreMode::Automatic - if !worker.status.is_active() - || worker.restore_intent != WorkerRestoreIntent::Automatic => - { - return Ok(worker.detail()); - } - _ => {} - } - let request = worker.request.clone().ok_or_else(|| { - RuntimeError::WorkerExecutionUnavailable { - worker_id: worker.worker_id, - message: "persisted Worker restore request is unavailable".to_string(), - } - })?; - (request, worker.working_directory.clone()) - }; - let backend = state.execution_backend.clone().ok_or_else(|| { - RuntimeError::WorkerExecutionUnavailable { - worker_id: worker_ref.worker_id, - message: "runtime has no execution backend".to_string(), - } - })?; - { - let worker = state.worker_mut(worker_ref)?; - worker.execution_bound = true; + let worker = state.worker(worker_ref)?; + if worker.execution_bound && !worker.execution_metadata_available { + return Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::ReconciliationRequired, + "worker_restore_reconciliation_pending", + "Worker restore cleanup or reconciliation is still pending", + )); } - state.persist_worker(&worker_ref.worker_id)?; + if worker.execution_handle.is_some() { + if worker.status.is_active() { + return Ok(RuntimeWorkerRestoreResult::accepted(worker.detail())); + } + return Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::ReconciliationRequired, + "worker_restore_cleanup_pending", + "Worker restore cleanup or reconciliation is still pending", + )); + } + match mode { + WorkerRestoreMode::Explicit + if worker.execution_metadata_available + && worker.status != WorkerStatus::Stopped => + { + return Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::Rejected, + "worker_restore_not_stopped", + "Worker restore requires a stopped Worker", + )); + } + WorkerRestoreMode::Automatic + if !worker.status.is_active() + || worker.restore_intent != WorkerRestoreIntent::Automatic => + { + return Ok(RuntimeWorkerRestoreResult::accepted(worker.detail())); + } + _ => {} + } + let Some(worker_request) = worker.request.clone() else { + return Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::Rejected, + "worker_restore_metadata_unavailable", + "Persisted Worker restore metadata is unavailable", + )); + }; + let Some(backend) = state.execution_backend.clone() else { + return Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::Rejected, + "worker_restore_backend_unavailable", + "Runtime has no Worker execution backend", + )); + }; let workspace_scope = worker_request.workspace_api.as_ref().and_then(|api| { state .workspace_owners @@ -1472,13 +1544,21 @@ impl Runtime { request: worker_request, workspace_scope, context: self.execution_context(worker_ref.clone()), - previous_working_directory, + previous_working_directory: worker.working_directory.clone(), working_directory: None, config_bundle: None, }; (backend, request) }; + if let Err(_result) = backend.preflight_restore(&request) { + return Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::Rejected, + "worker_restore_preflight_rejected", + "Worker restore preflight rejected the current request", + )); + } + match backend.restore_worker(request) { WorkerExecutionSpawnResult::Connected { handle, @@ -1488,30 +1568,117 @@ impl Runtime { let commit = self.commit_restored_worker_execution( worker_ref, handle.clone(), - worker_state, + worker_state.clone(), WorkerStatus::Idle, - working_directory, + working_directory.clone(), ); - if let Err(error) = commit { - self.cleanup_failed_restore(&backend, worker_ref, &handle)?; - return Err(error); + match commit { + Ok(worker) => Ok(RuntimeWorkerRestoreResult::accepted(worker)), + Err(error) => match self.cleanup_failed_restore(&backend, worker_ref, &handle) { + Ok(()) => { + tracing::warn!( + worker_id = %worker_ref.worker_id, + error = %error, + "Worker restore commit failed and was rolled back" + ); + Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::RolledBack, + "worker_restore_commit_rolled_back", + "Worker restore commit failed and cleanup completed", + )) + } + Err(cleanup_error) => { + tracing::error!( + worker_id = %worker_ref.worker_id, + error = %error, + cleanup_error = %cleanup_error, + "Worker restore requires reconciliation after commit and cleanup failures" + ); + if let Err(retain_error) = self.retain_restore_execution_evidence( + worker_ref, + handle, + worker_state, + working_directory, + ) { + tracing::error!( + worker_id = %worker_ref.worker_id, + error = %retain_error, + "failed to persist restore execution evidence; in-process reconciliation remains required" + ); + } + Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::ReconciliationRequired, + "worker_restore_reconciliation_required", + "Worker restore result is uncertain; reread or retry the Worker restore", + )) + } + }, } - self.worker_detail(worker_ref) } - WorkerExecutionSpawnResult::Rejected(result) - | WorkerExecutionSpawnResult::Errored(result) => { + WorkerExecutionSpawnResult::Rejected(_result) => { + Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::Rejected, + "worker_restore_rejected", + "Worker restore was rejected before live work started", + )) + } + WorkerExecutionSpawnResult::RolledBack(result) => { #[cfg(feature = "fs-store")] + self.lock()? + .record_restore_failure(worker_ref, result)?; + Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::RolledBack, + "worker_restore_rolled_back", + "Worker restore failed after live work started; cleanup completed", + )) + } + WorkerExecutionSpawnResult::ReconciliationRequired { + result: _result, + handle, + worker_state, + working_directory, + } => { + if let (Some(handle), Some(worker_state)) = (handle, worker_state) { + if let Err(retain_error) = self.retain_restore_execution_evidence( + worker_ref, + handle, + worker_state, + working_directory, + ) { + tracing::error!( + worker_id = %worker_ref.worker_id, + error = %retain_error, + "failed to persist restore execution evidence; in-process reconciliation remains required" + ); + } + } else if let Err(retain_error) = + self.retain_restore_reconciliation_pending(worker_ref) { - self.lock()? - .record_restore_failure(worker_ref, result.clone())?; + tracing::error!( + worker_id = %worker_ref.worker_id, + error = %retain_error, + "failed to persist pending restore reconciliation marker" + ); } - Err(RuntimeError::WorkerExecutionRejected { - worker_id: worker_ref.worker_id, - operation: result.operation, - outcome: result.outcome, - message: result.message_or_default(), - result, - }) + Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::ReconciliationRequired, + "worker_restore_reconciliation_required", + "Worker restore result is uncertain; reread or retry the Worker restore", + )) + } + WorkerExecutionSpawnResult::Errored(_result) => { + if let Err(retain_error) = self.retain_restore_reconciliation_pending(worker_ref) { + tracing::error!( + worker_id = %worker_ref.worker_id, + error = %retain_error, + "failed to persist pending restore reconciliation marker" + ); + } + Ok(RuntimeWorkerRestoreResult::failed( + WorkerRestoreState::ReconciliationRequired, + "worker_restore_backend_outcome_unknown", + "Worker restore backend outcome is uncertain; reread or retry the Worker restore", + )) } } } @@ -2469,27 +2636,83 @@ impl Runtime { worker_state: protocol::WorkerStateSnapshot, status: WorkerStatus, working_directory: Option, - ) -> Result<(), RuntimeError> { + ) -> Result { let mut state = self.lock()?; state.ensure_worker_ref(worker_ref)?; - { - let worker = state.worker_mut(worker_ref)?; - if worker.execution_handle.is_some() { - return Err(RuntimeError::InvalidRequest(format!( - "worker {} already has a current execution", - worker_ref.worker_id - ))); - } - worker.execution_handle = Some(handle); - worker.execution_metadata_available = true; - worker.execution_bound = true; - worker.status = status; - let _ = worker.apply_worker_state(&worker_state); - worker.restore_intent = restore_intent_for_status(worker.status); - worker.working_directory = working_directory; + let mut candidate = state.worker(worker_ref)?.clone(); + if candidate.execution_handle.is_some() { + return Err(RuntimeError::InvalidRequest(format!( + "worker {} already has a current execution", + worker_ref.worker_id + ))); } + candidate.execution_handle = Some(handle); + candidate.execution_metadata_available = true; + candidate.execution_bound = true; + candidate.status = status; + let _ = candidate.apply_worker_state(&worker_state); + candidate.restore_intent = restore_intent_for_status(candidate.status); + candidate.working_directory = working_directory; + + // Validate the exact subscription projection before persistence. The + // subsequent publish uses the same pure projection while the Worker + // operation lock keeps this candidate stable. + let _subscription_projection = state.subscription_worker(&candidate)?; + + // Persist the complete candidate before making it observable. A failed + // write therefore leaves both the in-memory catalog and subscriptions + // on the pre-restore record, so cleanup can produce a true rollback. + state.persist_worker_record(&candidate)?; + let detail = candidate.detail(); + state.workers.insert(worker_ref.worker_id, candidate); + state.publish_worker_upsert(worker_ref.worker_id)?; + if let Err(error) = state.persist_runtime_snapshot() { + tracing::warn!( + worker_id = %worker_ref.worker_id, + error = %error, + "Worker restore committed to its canonical record but Runtime snapshot refresh failed" + ); + } + Ok(detail) + } + + fn retain_restore_execution_evidence( + &self, + worker_ref: &WorkerRef, + handle: WorkerExecutionHandle, + worker_state: protocol::WorkerStateSnapshot, + working_directory: Option, + ) -> Result<(), RuntimeError> { + let mut state = self.lock()?; + let mut candidate = state.worker(worker_ref)?.clone(); + candidate.execution_handle = Some(handle); + candidate.execution_metadata_available = true; + candidate.execution_bound = true; + candidate.status = WorkerStatus::Idle; + let _ = candidate.apply_worker_state(&worker_state); + candidate.restore_intent = WorkerRestoreIntent::Automatic; + candidate.working_directory = working_directory; + state.workers.insert(worker_ref.worker_id, candidate); + state.publish_worker_upsert(worker_ref.worker_id)?; + state.persist_worker(&worker_ref.worker_id)?; + Ok(()) + } + + fn retain_restore_reconciliation_pending( + &self, + worker_ref: &WorkerRef, + ) -> Result<(), RuntimeError> { + let mut state = self.lock()?; + let mut candidate = state.worker(worker_ref)?.clone(); + candidate.execution_handle = None; + candidate.execution_metadata_available = false; + candidate.execution_bound = true; + // Active-without-metadata is the durable pending-reconciliation marker: + // it prevents a false Stopped projection and blocks duplicate restore. + candidate.status = WorkerStatus::Idle; + candidate.restore_intent = WorkerRestoreIntent::Explicit; + state.workers.insert(worker_ref.worker_id, candidate); state.publish_worker_upsert(worker_ref.worker_id)?; - state.persist_runtime_snapshot()?; state.persist_worker(&worker_ref.worker_id)?; Ok(()) } @@ -2513,18 +2736,6 @@ impl Runtime { result, }); } - - let mut state = self.lock()?; - let worker = state.worker_mut(worker_ref)?; - worker.execution_handle = None; - worker.execution_bound = false; - worker.status = WorkerStatus::Stopped; - worker.worker_state = None; - worker.restore_intent = WorkerRestoreIntent::Explicit; - worker.internal_workers.clear(); - state.publish_worker_upsert(worker_ref.worker_id)?; - state.persist_runtime_snapshot()?; - state.persist_worker(&worker_ref.worker_id)?; Ok(()) } @@ -2908,6 +3119,14 @@ impl RuntimeState { Ok(()) } + #[cfg(feature = "fs-store")] + fn persist_worker_record(&self, worker: &WorkerRecord) -> Result<(), RuntimeError> { + if let Some(store) = self.fs_store() { + store.write_worker_record(&worker.persisted_record())?; + } + Ok(()) + } + #[cfg(feature = "fs-store")] fn persist_worker(&self, worker_id: &WorkerId) -> Result<(), RuntimeError> { if let Some(store) = self.fs_store() { @@ -2945,6 +3164,11 @@ impl RuntimeState { Ok(()) } + #[cfg(not(feature = "fs-store"))] + fn persist_worker_record(&self, _worker: &WorkerRecord) -> Result<(), RuntimeError> { + Ok(()) + } + #[cfg(not(feature = "fs-store"))] fn persist_worker(&self, _worker_id: &WorkerId) -> Result<(), RuntimeError> { Ok(()) @@ -3689,7 +3913,7 @@ struct RemovedInternalWorkerActivity { parent_session_id: Option, } -#[derive(Debug, Default)] +#[derive(Debug, Default, Clone)] struct InternalWorkerActivityProjection { workers: BTreeMap, removed_workers: BTreeMap, @@ -3708,7 +3932,7 @@ impl InternalWorkerActivityProjection { } } -#[derive(Debug)] +#[derive(Debug, Clone)] struct WorkerRecord { worker_ref: WorkerRef, worker_id: WorkerId, @@ -4998,6 +5222,7 @@ mod tests { stop_gate: Mutex>>, stop_count: Mutex, restore_result: Mutex>, + preflight_restore_result: Mutex>, restore_gate: Mutex>>, restore_count: Mutex, config_bundles: Mutex>>, @@ -5117,6 +5342,16 @@ mod tests { } } + fn preflight_restore( + &self, + _request: &WorkerExecutionRestoreRequest, + ) -> Result<(), WorkerExecutionResult> { + match self.preflight_restore_result.lock().unwrap().clone() { + Some(result) => Err(result), + None => Ok(()), + } + } + fn restore_worker( &self, request: WorkerExecutionRestoreRequest, @@ -7229,6 +7464,69 @@ mod tests { let _ = std::fs::remove_dir_all(root); } + #[test] + fn restore_preflight_rejection_is_typed_and_side_effect_free() { + let (runtime, backend) = runtime_and_backend(); + runtime.store_config_bundle(test_bundle()).unwrap(); + let created = runtime + .create_worker(task_request("restore preflight")) + .unwrap(); + runtime.stop_worker(&created.worker_ref, None).unwrap(); + let before = runtime.worker_detail(&created.worker_ref).unwrap(); + backend.preflight_restore_result.lock().unwrap().replace( + WorkerExecutionResult::rejected( + WorkerExecutionOperation::Restore, + "profile is no longer permitted", + ), + ); + + let result = runtime + .restore_worker_operation(&created.worker_ref) + .unwrap(); + + assert_eq!(result.state, WorkerRestoreState::Rejected); + assert!(result.worker.is_none()); + assert_eq!(*backend.restore_count.lock().unwrap(), 0); + assert_eq!(runtime.worker_detail(&created.worker_ref).unwrap(), before); + } + + #[test] + fn uncertain_restore_backend_outcome_requires_reconciliation() { + let (runtime, backend) = runtime_and_backend(); + runtime.store_config_bundle(test_bundle()).unwrap(); + let created = runtime + .create_worker(task_request("restore reconciliation")) + .unwrap(); + runtime.stop_worker(&created.worker_ref, None).unwrap(); + backend.restore_result.lock().unwrap().replace( + WorkerExecutionSpawnResult::ReconciliationRequired { + result: WorkerExecutionResult::errored( + WorkerExecutionOperation::Restore, + "controller handshake outcome was not observable", + ), + handle: None, + worker_state: None, + working_directory: None, + }, + ); + + let result = runtime + .restore_worker_operation(&created.worker_ref) + .unwrap(); + + assert_eq!(result.state, WorkerRestoreState::ReconciliationRequired); + assert!(result.worker.is_none()); + assert_eq!( + runtime.worker_detail(&created.worker_ref).unwrap().status, + WorkerStatus::Idle + ); + let retry = runtime + .restore_worker_operation(&created.worker_ref) + .unwrap(); + assert_eq!(retry.state, WorkerRestoreState::ReconciliationRequired); + assert_eq!(*backend.restore_count.lock().unwrap(), 1); + } + #[cfg(feature = "fs-store")] #[test] fn fs_store_automatically_restores_every_active_lifecycle_state() { @@ -7564,7 +7862,7 @@ mod tests { #[cfg(feature = "fs-store")] #[test] - fn fs_store_stops_worker_and_reports_when_execution_restore_fails() { + fn fs_store_marks_worker_for_reconciliation_when_execution_restore_is_uncertain() { let root = fs_store_root("execution-restore-failed"); let runtime = Runtime::with_fs_store_and_execution_backend( crate::fs_store::FsRuntimeStoreOptions { @@ -7598,17 +7896,13 @@ mod tests { assert_eq!(*restoring_backend.restore_count.lock().unwrap(), 1); let restored_worker = restored.worker_detail(&worker.worker_ref).unwrap(); - assert_eq!(restored_worker.status, WorkerStatus::Stopped); - assert!( - restored - .diagnostics() - .unwrap() - .iter() - .any( - |diagnostic| diagnostic.code == "worker_execution_restore_failed" - && diagnostic.worker_ref.as_ref() == Some(&worker.worker_ref) - ) - ); + assert_eq!(restored_worker.status, WorkerStatus::Idle); + assert!(!restored_worker.execution_metadata_available); + let retry = restored + .restore_worker_operation(&worker.worker_ref) + .unwrap(); + assert_eq!(retry.state, WorkerRestoreState::ReconciliationRequired); + assert_eq!(*restoring_backend.restore_count.lock().unwrap(), 1); let err = restored .send_input( &worker.worker_ref, diff --git a/crates/worker-runtime/src/worker_backend.rs b/crates/worker-runtime/src/worker_backend.rs index e17794e82..49f76cd7d 100644 --- a/crates/worker-runtime/src/worker_backend.rs +++ b/crates/worker-runtime/src/worker_backend.rs @@ -1666,15 +1666,34 @@ where }) { Ok(task) => task, Err(message) => { - let cleanup = self - .cleanup_unconnected_controller(&handle, &shutdown, &tasks, &worker_state) - .err() - .map(|cleanup| format!("; controller cleanup failed: {cleanup}")) - .unwrap_or_default(); - return WorkerExecutionSpawnResult::Errored(WorkerExecutionResult::errored( + let cleanup = self.cleanup_unconnected_controller( + &handle, + &shutdown, + &tasks, + &worker_state, + ); + let result = WorkerExecutionResult::errored( operation, - format!("{message}{cleanup}"), - )); + match &cleanup { + Ok(()) => message, + Err(cleanup) => { + format!("{message}; controller cleanup failed: {cleanup}") + } + }, + ); + return if operation == WorkerExecutionOperation::Restore { + match cleanup { + Ok(()) => WorkerExecutionSpawnResult::RolledBack(result), + Err(_) => WorkerExecutionSpawnResult::ReconciliationRequired { + result, + handle: None, + worker_state: None, + working_directory: None, + }, + } + } else { + WorkerExecutionSpawnResult::Errored(result) + }; } }; tasks.push("protocol bridge", bridge_task, true); @@ -1687,28 +1706,66 @@ where let mut workers = match self.workers.lock() { Ok(workers) => workers, Err(_) => { - let cleanup = self - .cleanup_unconnected_controller(&handle, &shutdown, &tasks, &worker_state) - .err() - .map(|cleanup| format!("; controller cleanup failed: {cleanup}")) - .unwrap_or_default(); - return WorkerExecutionSpawnResult::Errored(WorkerExecutionResult::errored( + let cleanup = self.cleanup_unconnected_controller( + &handle, + &shutdown, + &tasks, + &worker_state, + ); + let result = WorkerExecutionResult::errored( operation, - format!("worker adapter registry lock is poisoned{cleanup}"), - )); + match &cleanup { + Ok(()) => "worker adapter registry lock is poisoned".to_string(), + Err(cleanup) => format!( + "worker adapter registry lock is poisoned; controller cleanup failed: {cleanup}" + ), + }, + ); + return if operation == WorkerExecutionOperation::Restore { + match cleanup { + Ok(()) => WorkerExecutionSpawnResult::RolledBack(result), + Err(_) => WorkerExecutionSpawnResult::ReconciliationRequired { + result, + handle: None, + worker_state: None, + working_directory: None, + }, + } + } else { + WorkerExecutionSpawnResult::Errored(result) + }; } }; if workers.contains_key(&worker_ref) { drop(workers); - let cleanup = self - .cleanup_unconnected_controller(&handle, &shutdown, &tasks, &worker_state) - .err() - .map(|cleanup| format!("; controller cleanup failed: {cleanup}")) - .unwrap_or_default(); - return WorkerExecutionSpawnResult::Rejected(WorkerExecutionResult::busy( + let cleanup = self.cleanup_unconnected_controller( + &handle, + &shutdown, + &tasks, + &worker_state, + ); + let result = WorkerExecutionResult::busy( operation, - format!("Worker is already connected to execution backend{cleanup}"), - )); + match &cleanup { + Ok(()) => "Worker is already connected to execution backend".to_string(), + Err(cleanup) => format!( + "Worker is already connected to execution backend; controller cleanup failed: {cleanup}" + ), + }, + ); + return if operation == WorkerExecutionOperation::Restore { + match cleanup { + Ok(()) => WorkerExecutionSpawnResult::RolledBack(result), + Err(_) => WorkerExecutionSpawnResult::ReconciliationRequired { + result, + handle: None, + worker_state: None, + working_directory: None, + }, + } + } else { + WorkerExecutionSpawnResult::Rejected(result) + }; } let connected_worker_state = worker_state .read() @@ -1995,6 +2052,25 @@ where ) } + fn preflight_restore( + &self, + request: &WorkerExecutionRestoreRequest, + ) -> Result<(), WorkerExecutionResult> { + let workers = self.workers.lock().map_err(|_| { + WorkerExecutionResult::errored( + WorkerExecutionOperation::Restore, + "worker adapter registry lock is poisoned", + ) + })?; + if workers.contains_key(&request.worker_ref) { + return Err(WorkerExecutionResult::busy( + WorkerExecutionOperation::Restore, + "Worker is already connected to execution backend", + )); + } + Ok(()) + } + fn restore_worker( &self, mut request: WorkerExecutionRestoreRequest, @@ -2076,10 +2152,15 @@ where let controller = match restore_result { Ok(controller) => controller, Err(message) => { - return WorkerExecutionSpawnResult::Errored(WorkerExecutionResult::errored( - WorkerExecutionOperation::Restore, - message, - )); + return WorkerExecutionSpawnResult::ReconciliationRequired { + result: WorkerExecutionResult::errored( + WorkerExecutionOperation::Restore, + message, + ), + handle: None, + worker_state: None, + working_directory: None, + }; } }; diff --git a/crates/workspace-api/src/lib.rs b/crates/workspace-api/src/lib.rs index c05750ab3..c3452b24f 100644 --- a/crates/workspace-api/src/lib.rs +++ b/crates/workspace-api/src/lib.rs @@ -2327,15 +2327,36 @@ pub enum WorkerOperationState { Rejected, } +/// Cross-layer outcome of an explicit Worker restore operation. +/// +/// `Rejected` is reserved for read-only preflight failures. Once live restore +/// work starts, failure is either a confirmed `RolledBack` operation or a +/// `ReconciliationRequired` result whose commit state must be reread/retried. +#[derive(Debug, Clone, Copy, Serialize, Deserialize, PartialEq, Eq)] +#[cfg_attr(feature = "typescript", derive(ts_rs::TS))] +#[serde(rename_all = "snake_case")] +pub enum WorkerRestoreState { + Accepted, + Rejected, + RolledBack, + ReconciliationRequired, +} + #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[cfg_attr(feature = "typescript", derive(ts_rs::TS))] +#[serde(deny_unknown_fields)] pub struct WorkerRestoreResult { - pub state: WorkerOperationState, + pub state: WorkerRestoreState, + #[serde(default, skip_serializing_if = "Option::is_none")] + #[cfg_attr(feature = "typescript", ts(optional = nullable))] pub worker: Option, #[serde(default)] pub diagnostics: Vec, } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[cfg_attr(feature = "typescript", derive(ts_rs::TS))] +#[serde(deny_unknown_fields)] pub struct WorkerRestoreResponse { pub workspace_id: String, pub runtime_id: String, @@ -3482,6 +3503,42 @@ mod workdir_typescript_tests { mod tests { use super::*; + #[test] + fn worker_restore_state_round_trips_and_rejects_unknown_variants() { + for (state, wire) in [ + (WorkerRestoreState::Accepted, "accepted"), + (WorkerRestoreState::Rejected, "rejected"), + (WorkerRestoreState::RolledBack, "rolled_back"), + ( + WorkerRestoreState::ReconciliationRequired, + "reconciliation_required", + ), + ] { + assert_eq!( + serde_json::to_value(state).unwrap(), + serde_json::json!(wire) + ); + assert_eq!( + serde_json::from_value::(serde_json::json!(wire)).unwrap(), + state + ); + } + assert!( + serde_json::from_value::(serde_json::json!("unknown")).is_err() + ); + } + + #[test] + fn worker_restore_result_rejects_unknown_fields() { + let result = serde_json::from_value::(serde_json::json!({ + "state": "rejected", + "worker": null, + "diagnostics": [], + "unexpected": true + })); + assert!(result.is_err()); + } + #[test] fn historical_http_repository_source_kind_decodes_as_invalid_evidence() { let source: RepositorySource = serde_json::from_value(serde_json::json!({ diff --git a/crates/workspace-server/src/hosts.rs b/crates/workspace-server/src/hosts.rs index 6cd702387..2386c9548 100644 --- a/crates/workspace-server/src/hosts.rs +++ b/crates/workspace-server/src/hosts.rs @@ -52,6 +52,7 @@ use worker_runtime::http_server::{ RuntimeHttpWorkerCompletionsResponse, RuntimeHttpWorkerDeleteResponse, RuntimeHttpWorkerInputResponse, RuntimeHttpWorkerLifecycleRequest, RuntimeHttpWorkerLifecycleResponse, RuntimeHttpWorkerResponse, + RuntimeHttpWorkerRestoreResponse, RuntimeHttpWorkerWorkspaceApiRequest, RuntimeHttpWorkersResponse, RuntimeHttpWorkingDirectoriesResponse, RuntimeHttpWorkingDirectoryResponse, RuntimeHttpWorkspacePromptProjectionRequest, RuntimeHttpWorkspacePromptProjectionResponse, @@ -378,8 +379,9 @@ pub(crate) fn workspace_worker_summary( } #[derive(Debug, Clone, Serialize, Deserialize, PartialEq, Eq)] +#[serde(deny_unknown_fields)] pub struct WorkerRestoreResult { - pub state: WorkerOperationState, + pub state: workspace_api::WorkerRestoreState, #[serde(skip_serializing_if = "Option::is_none")] pub worker: Option, pub diagnostics: Vec, @@ -882,7 +884,7 @@ pub trait WorkspaceWorkerRuntime: Send + Sync { fn restore_worker(&self, worker_id: &str) -> WorkerRestoreResult { WorkerRestoreResult { - state: WorkerOperationState::Unsupported, + state: workspace_api::WorkerRestoreState::Rejected, worker: None, diagnostics: vec![diagnostic( "worker_restore_unsupported", @@ -2336,7 +2338,7 @@ impl WorkspaceWorkerRuntime for EmbeddedWorkerRuntime { fn restore_worker(&self, worker_id: &str) -> WorkerRestoreResult { let Some(worker_ref) = self.worker_ref(worker_id) else { return WorkerRestoreResult { - state: WorkerOperationState::Rejected, + state: workspace_api::WorkerRestoreState::Rejected, worker: None, diagnostics: vec![diagnostic( "embedded_worker_id_invalid", @@ -2345,14 +2347,24 @@ impl WorkspaceWorkerRuntime for EmbeddedWorkerRuntime { )], }; }; - match self.runtime.restore_worker(&worker_ref) { - Ok(detail) => WorkerRestoreResult { - state: WorkerOperationState::Accepted, - worker: Some(self.map_worker_detail(detail)), - diagnostics: Vec::new(), - }, + match self.runtime.restore_worker_operation(&worker_ref) { + Ok(result) => { + let diagnostics = match (result.reason_code, result.message) { + (Some(code), Some(message)) => vec![diagnostic( + code, + DiagnosticSeverity::Warning, + message, + )], + _ => Vec::new(), + }; + WorkerRestoreResult { + state: result.state, + worker: result.worker.map(|detail| self.map_worker_detail(detail)), + diagnostics, + } + } Err(err) => WorkerRestoreResult { - state: WorkerOperationState::Rejected, + state: workspace_api::WorkerRestoreState::ReconciliationRequired, worker: None, diagnostics: vec![embedded_runtime_diagnostic(&err)], }, @@ -4164,17 +4176,27 @@ impl WorkspaceWorkerRuntime for RemoteWorkerRuntime { } fn restore_worker(&self, worker_id: &str) -> WorkerRestoreResult { - match self.post_json::<_, RuntimeHttpWorkerResponse>( + match self.post_json::<_, RuntimeHttpWorkerRestoreResponse>( &format!("/v1/workers/{worker_id}/restore"), &serde_json::json!({}), ) { - Ok(response) => WorkerRestoreResult { - state: WorkerOperationState::Accepted, - worker: Some(self.map_worker_detail(response.worker)), - diagnostics: Vec::new(), - }, + Ok(response) => { + let diagnostics = match (response.reason_code, response.message) { + (Some(code), Some(message)) => { + vec![diagnostic(code, DiagnosticSeverity::Warning, message)] + } + _ => Vec::new(), + }; + WorkerRestoreResult { + state: response.state, + worker: response.worker.map(|worker| self.map_worker_detail(worker)), + diagnostics, + } + } Err(diagnostic) => WorkerRestoreResult { - state: WorkerOperationState::Rejected, + // A transport/protocol failure cannot prove that the Runtime + // rejected before side effects. Preserve uncertainty. + state: workspace_api::WorkerRestoreState::ReconciliationRequired, worker: None, diagnostics: vec![diagnostic], }, @@ -5428,6 +5450,38 @@ mod tests { use std::sync::{Arc, Mutex}; use std::thread; + #[test] + fn remote_restore_transport_failure_requires_reconciliation() { + let provider = RemoteWorkerRuntime::new( + RemoteRuntimeConfig { + runtime_id: "runtime-remote".to_string(), + workspace_id: Some("workspace-a".to_string()), + display_name: "Remote Runtime".to_string(), + base_url: "http://127.0.0.1:9".to_string(), + bearer_token: None, + workspace_authorization: None, + strict_public_egress: false, + cached_worker_creation_available: true, + cached_os: "linux".to_string(), + cached_arch: "x86_64".to_string(), + cached_status: "ready".to_string(), + timeout: Duration::from_millis(100), + }, + "workspace-a".to_string(), + "http://127.0.0.1:3000".to_string(), + ) + .unwrap(); + + let result = provider.restore_worker("worker-transport-uncertain"); + + assert_eq!( + result.state, + workspace_api::WorkerRestoreState::ReconciliationRequired + ); + assert!(result.worker.is_none()); + assert_eq!(result.diagnostics.len(), 1); + } + #[test] fn embedded_delete_persistence_failure_diagnostic_is_bounded_and_path_free() { let diagnostic = diff --git a/crates/workspace-server/src/server.rs b/crates/workspace-server/src/server.rs index c3f56388d..27112fe5f 100644 --- a/crates/workspace-server/src/server.rs +++ b/crates/workspace-server/src/server.rs @@ -2814,7 +2814,7 @@ impl WorkspaceApi { .map_err(|error| error.into_error())?; if binding.state != WorkerOperationState::Accepted { return Ok(WorkerRestoreResult { - state: binding.state, + state: workspace_api::WorkerRestoreState::Rejected, worker: binding.worker, diagnostics: binding.diagnostics, }); @@ -10422,7 +10422,7 @@ async fn scoped_start_workspace_orchestrator( .runtime .restore_worker(&existing.worker) .map_err(|error| error.into_error())?; - if restored.state != WorkerOperationState::Accepted { + if restored.state != workspace_api::WorkerRestoreState::Accepted { return Err(ApiError::with_diagnostics( Error::RuntimeOperationFailed { runtime_id: existing.worker.runtime_id.clone(), @@ -12501,7 +12501,7 @@ async fn scoped_restore_runtime_worker( runtime_id: runtime_id.clone(), worker_id: worker_id.clone(), result: workspace_api::WorkerRestoreResult { - state: workspace_api::WorkerOperationState::Accepted, + state: workspace_api::WorkerRestoreState::Accepted, worker: Some(worker), diagnostics: Vec::new(), }, @@ -15213,7 +15213,7 @@ async fn restore_runtime_worker( runtime_id: runtime_id.clone(), worker_id: worker_id.clone(), result: workspace_api::WorkerRestoreResult { - state: result.state.into(), + state: result.state, worker: projected_worker, diagnostics: result.diagnostics.into_iter().map(Into::into).collect(), }, @@ -20722,7 +20722,7 @@ mod tests { } #[tokio::test] - async fn production_profile_backend_rejects_unrecoverable_pending_orchestrator_restore() { + async fn production_profile_backend_reports_uncertain_pending_orchestrator_restore() { let workspace = tempfile::tempdir().unwrap(); init_clean_git_workspace(workspace.path()); let config = test_server_config(workspace.path()); @@ -20772,11 +20772,9 @@ mod tests { }), ) .await - .expect_err("pending Workspace Orchestrator restore without durable Prompt must fail"); + .expect_err("pending Workspace Orchestrator restore without durable Prompt must require reconciliation"); assert!( - format!("{error:?}").contains( - "pending Workspace Worker restore requires operation-owned launch material" - ), + format!("{error:?}").contains("worker_restore_reconciliation_required"), "unexpected restore error: {error:?}" ); } @@ -25277,7 +25275,7 @@ mod tests { assert_eq!(retried_restore.worker_id, first_worker.worker.worker_id); assert_eq!( retried_restore.result.state, - workspace_api::WorkerOperationState::Accepted + workspace_api::WorkerRestoreState::Accepted ); let restored_assignment = api .store diff --git a/web/workspace/src/lib/workspace/sidebar/types.ts b/web/workspace/src/lib/workspace/sidebar/types.ts index 856076fd2..a6642c2c3 100644 --- a/web/workspace/src/lib/workspace/sidebar/types.ts +++ b/web/workspace/src/lib/workspace/sidebar/types.ts @@ -112,6 +112,19 @@ export type Worker = { export type WorkerOperationState = "accepted" | "unsupported" | "rejected"; +/** Typed result of restore across Runtime, Workspace, Web, and TUI clients. */ +export type WorkerRestoreState = + | "accepted" + | "rejected" + | "rolled_back" + | "reconciliation_required"; + +export type WorkerRestoreResult = { + state: WorkerRestoreState; + worker?: WorkerSummary | null; + diagnostics: Diagnostic[]; +}; + export type WorkerLaunchRuntimeOption = SharedWorkerLaunchRuntimeOption; export type WorkerLaunchProfileCandidate = SharedWorkerLaunchProfileCandidate; export type WorkingDirectoryRepositoryOption = From 8ac8b2d50c3365e238be75439fa08eabcf001588 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 00:08:08 +0900 Subject: [PATCH 07/64] Document missing rustfmt validation tool --- ...026-08-19-coder-toolchain-missing-rustfmt.md | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) create mode 100644 docs/report/2026-08-19-coder-toolchain-missing-rustfmt.md diff --git a/docs/report/2026-08-19-coder-toolchain-missing-rustfmt.md b/docs/report/2026-08-19-coder-toolchain-missing-rustfmt.md new file mode 100644 index 000000000..907ccd749 --- /dev/null +++ b/docs/report/2026-08-19-coder-toolchain-missing-rustfmt.md @@ -0,0 +1,17 @@ +# Ticket validation image lacks `rustfmt` + +While implementing T-613, `cargo fmt --all` failed before reading project sources because the active Rust toolchain does not provide the `rustfmt` component. The standalone `rustfmt` binary is also absent. + +Observed commands: + +```text +$ cargo fmt --all +error: 'cargo-fmt' is not installed for the toolchain '1.90.0-x86_64-unknown-linux-gnu'. + +$ rustfmt --version +bash: rustfmt: command not found +``` + +This prevents Workers from executing the repository-required formatting check or automatically normalizing Rust changes. The Coder can still run compile/tests and `git diff --check`, but formatting evidence remains weaker and review may surface avoidable formatting differences. + +Suggested improvement: include the `rustfmt` component in the standard coding Workdir/toolchain image and verify `cargo fmt --all -- --check` during image qualification. From 6d9c9cdb48e027d0ef621576f4291a4c2460c4e3 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 00:14:34 +0900 Subject: [PATCH 08/64] fix(web): gate compaction status by worker state --- .../lib/workspace/console/run-status.test.ts | 40 +++++++++++++++++++ .../src/lib/workspace/console/run-status.ts | 23 ++++++++++- .../console/worker-console.ui.test.ts | 25 ++++++++++++ .../workers/[workerRef]/console/+page.svelte | 19 ++++----- 4 files changed, 94 insertions(+), 13 deletions(-) diff --git a/web/workspace/src/lib/workspace/console/run-status.test.ts b/web/workspace/src/lib/workspace/console/run-status.test.ts index e0d990471..998009b61 100644 --- a/web/workspace/src/lib/workspace/console/run-status.test.ts +++ b/web/workspace/src/lib/workspace/console/run-status.test.ts @@ -5,6 +5,7 @@ import { formatRunElapsed, formatRunElapsedCompact, formatRunTokens, + visibleCompactionProgress, } from "./run-status.ts"; function assertEquals(actual: unknown, expected: unknown): void { @@ -98,3 +99,42 @@ Deno.test("run status formatting matches the compact TUI shape", () => { assertEquals(formatRunTokens(3_000), "3.0k"); assertEquals(formatRunTokens(999), "999"); }); + +Deno.test("visible compaction requires a matching busy worker state", () => { + const manual = { + phase: "preparing", + started_at_ms: 1_700_000_000_000, + trigger: "manual", + } as const; + const automatic = { + phase: "summarizing", + started_at_ms: 1_700_000_000_000, + trigger: "request_threshold", + } as const; + const maintenance = { + last_command_id: 7, + state: { + kind: "busy", + state: { kind: "maintenance", state: "compacting" }, + }, + } as const; + const run = { + last_command_id: 7, + state: { + kind: "busy", + state: { kind: "run", state: "running" }, + }, + } as const; + const idle = { + last_command_id: 7, + state: { kind: "idle" }, + } as const; + + assertEquals(visibleCompactionProgress(manual, maintenance), manual); + assertEquals(visibleCompactionProgress(automatic, run), automatic); + assertEquals(visibleCompactionProgress(manual, run), null); + assertEquals(visibleCompactionProgress(automatic, maintenance), null); + assertEquals(visibleCompactionProgress(manual, idle), null); + assertEquals(visibleCompactionProgress(manual, null), null); + assertEquals(visibleCompactionProgress(null, maintenance), null); +}); diff --git a/web/workspace/src/lib/workspace/console/run-status.ts b/web/workspace/src/lib/workspace/console/run-status.ts index 37495dc1c..326694e97 100644 --- a/web/workspace/src/lib/workspace/console/run-status.ts +++ b/web/workspace/src/lib/workspace/console/run-status.ts @@ -1,4 +1,8 @@ -import type { Event as ProtocolEvent } from "$lib/generated/protocol"; +import type { + Event as ProtocolEvent, + InFlightCompaction, + WorkerStateSnapshot, +} from "$lib/generated/protocol"; export type RunActivityStats = { startedAtMs: number | null; @@ -71,3 +75,20 @@ export function formatRunTokens(tokens: number): string { if (tokens >= 1_000) return `${(tokens / 1_000).toFixed(1)}k`; return String(tokens); } + +export function visibleCompactionProgress( + compaction: InFlightCompaction | null, + workerState: WorkerStateSnapshot | null, +): InFlightCompaction | null { + if (!compaction || workerState?.state.kind !== "busy") { + return null; + } + + const busyState = workerState.state.state; + const triggerMatches = compaction.trigger === "manual" + ? busyState.kind === "maintenance" && + busyState.state === "compacting" + : busyState.kind === "run"; + + return triggerMatches ? compaction : null; +} diff --git a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts index 3b6629ae9..c39f231a9 100644 --- a/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts +++ b/web/workspace/src/lib/workspace/console/worker-console.ui.test.ts @@ -1116,3 +1116,28 @@ Deno.test("Web Console uses Notify while running and exposes durable pending con "ordinary text must remain Submit instead of being implicitly converted to Notify", ); }); + +Deno.test("Web Console gates compaction progress before rendering run status", async () => { + const consolePage = await Deno.readTextFile( + new URL( + "./../../../routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte", + import.meta.url, + ), + ); + const runStatus = await Deno.readTextFile( + new URL("./WorkerRunStatus.svelte", import.meta.url), + ); + + assert( + consolePage.includes("visibleCompactionProgress(") && + consolePage.includes("consoleProjection.compaction") && + consolePage.includes("consoleProjection.workerState") && + consolePage.includes("compaction={compactionProgress}"), + "Console page should pass only state-consistent compaction progress to the status component", + ); + assert( + runStatus.includes("compaction?.started_at_ms ?? startedAtMs") && + runStatus.includes("Compacting · {compaction.phase}"), + "run status should render the compaction phase and derive elapsed time from the authoritative start timestamp", + ); +}); diff --git a/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte b/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte index 28f3cbd35..b9ba03797 100644 --- a/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte +++ b/web/workspace/src/routes/w/[workspaceId]/workers/[workerRef]/console/+page.svelte @@ -21,6 +21,7 @@ type ComposerCompletionToken, } from "$lib/workspace/console/composer-completion"; import WorkerRunStatus from "$lib/workspace/console/WorkerRunStatus.svelte"; + import { visibleCompactionProgress } from "$lib/workspace/console/run-status"; import { resolveWorkerControlShortcut } from "$lib/workspace/console/worker-control-shortcuts"; import { consoleWorkerViews, @@ -248,18 +249,12 @@ liveWorkerState ?? (worker?.state === "stopped" ? "stopped" : "loading"), ); const workerRunning = $derived(workerState === "running"); - const compactionProgress = $derived.by(() => { - const state = consoleProjection.workerState?.state; - if (!state || typeof state !== "object" || !("busy" in state)) return null; - const progress = consoleProjection.compaction; - if (!progress) return null; - const busy = state.busy; - if (!busy || typeof busy !== "object") return null; - const valid = progress.trigger === "manual" - ? "maintenance" in busy && busy.maintenance === "compacting" - : "run" in busy; - return valid ? progress : null; - }); + const compactionProgress = $derived( + visibleCompactionProgress( + consoleProjection.compaction, + consoleProjection.workerState, + ), + ); const workerPaused = $derived(workerState === "paused"); const composerEditable = $derived(protocolState === "open" && !sending); const draftHasText = $derived(draft.content.trim().length > 0); From 68c59afbe7f3224ec78f580676190734215875bc Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 00:25:56 +0900 Subject: [PATCH 09/64] Preserve restore subscription and cleanup evidence --- crates/worker-runtime/src/runtime.rs | 4 +- crates/worker-runtime/src/worker_backend.rs | 73 ++++++++++++++++++--- 2 files changed, 67 insertions(+), 10 deletions(-) diff --git a/crates/worker-runtime/src/runtime.rs b/crates/worker-runtime/src/runtime.rs index 85406de48..29736744c 100644 --- a/crates/worker-runtime/src/runtime.rs +++ b/crates/worker-runtime/src/runtime.rs @@ -1622,10 +1622,10 @@ impl Runtime { "Worker restore was rejected before live work started", )) } - WorkerExecutionSpawnResult::RolledBack(result) => { + WorkerExecutionSpawnResult::RolledBack(_result) => { #[cfg(feature = "fs-store")] self.lock()? - .record_restore_failure(worker_ref, result)?; + .record_restore_failure(worker_ref, _result)?; Ok(RuntimeWorkerRestoreResult::failed( WorkerRestoreState::RolledBack, "worker_restore_rolled_back", diff --git a/crates/worker-runtime/src/worker_backend.rs b/crates/worker-runtime/src/worker_backend.rs index 49f76cd7d..8447fc726 100644 --- a/crates/worker-runtime/src/worker_backend.rs +++ b/crates/worker-runtime/src/worker_backend.rs @@ -1569,6 +1569,39 @@ where .unwrap_or_else(|message| WorkerExecutionResult::errored(operation, message)) } + fn retain_uncertain_unconnected_controller( + &self, + worker_ref: &crate::identity::WorkerRef, + handle: WorkerHandle, + shutdown: Arc>>, + tasks: RuntimeExecutionTaskScope, + worker_state: Arc>, + workspace_client: Option>, + ) -> Result { + let mut workers = self + .workers + .lock() + .map_err(|_| "worker adapter registry lock is poisoned".to_string())?; + if workers.contains_key(worker_ref) { + return Err("Worker already has a retained execution handle".to_string()); + } + workers.insert( + worker_ref.clone(), + RuntimeWorkerExecution { + handle, + shutdown, + shutdown_requested: Arc::new(AtomicBool::new(false)), + tasks, + worker_state, + workspace_client, + }, + ); + Ok(WorkerExecutionHandle::new( + worker_ref.clone(), + self.backend_id(), + )) + } + fn cleanup_unconnected_controller( &self, handle: &WorkerHandle, @@ -1618,11 +1651,19 @@ where working_directory: Option, workspace_client: Option>, ) -> WorkerExecutionSpawnResult { - let worker_state = Arc::new(RwLock::new(handle.shared_state.snapshot())); + #[cfg(feature = "ws-server")] + let streams = subscribe_worker_protocol_session(&handle); + #[cfg(feature = "ws-server")] + let worker_state_snapshot = match &streams.snapshot_event { + Event::Snapshot { state, .. } => state.clone(), + _ => unreachable!("Worker protocol subscription snapshot must be a snapshot event"), + }; + #[cfg(not(feature = "ws-server"))] + let worker_state_snapshot = handle.shared_state.snapshot(); + let worker_state = Arc::new(RwLock::new(worker_state_snapshot)); let tasks = RuntimeExecutionTaskScope::new(controller_task); #[cfg(feature = "ws-server")] { - let streams = subscribe_worker_protocol_session(&handle); let mut events = streams.events; let mut entry_events = streams.log_entries; let bridge_worker_state = worker_state.clone(); @@ -1684,12 +1725,28 @@ where return if operation == WorkerExecutionOperation::Restore { match cleanup { Ok(()) => WorkerExecutionSpawnResult::RolledBack(result), - Err(_) => WorkerExecutionSpawnResult::ReconciliationRequired { - result, - handle: None, - worker_state: None, - working_directory: None, - }, + Err(_) => { + let retained_state = + worker_state.read().ok().map(|state| state.clone()); + let retained_working_directory = + working_directory.as_ref().map(|binding| binding.status()); + let retained_handle = self + .retain_uncertain_unconnected_controller( + &worker_ref, + handle, + shutdown, + tasks, + worker_state, + workspace_client, + ) + .ok(); + WorkerExecutionSpawnResult::ReconciliationRequired { + result, + handle: retained_handle, + worker_state: retained_state, + working_directory: retained_working_directory, + } + } } } else { WorkerExecutionSpawnResult::Errored(result) From ecc0602f53a9a4fe0332baee704a27e934891b99 Mon Sep 17 00:00:00 2001 From: Hare Date: Fri, 18 Sep 2026 00:26:40 +0900 Subject: [PATCH 10/64] test(web): cover compacting status presentation --- web/workspace/deno.json | 2 +- .../workspace/console/WorkerRunStatus.svelte | 33 +++++++++++++++---- .../lib/workspace/console/run-status.test.ts | 18 +++++++++- .../src/lib/workspace/console/run-status.ts | 15 +++++++++ .../console/worker-console.ui.test.ts | 17 +++++----- .../workers/[workerRef]/console/+page.svelte | 10 ++---- 6 files changed, 70 insertions(+), 25 deletions(-) diff --git a/web/workspace/deno.json b/web/workspace/deno.json index 58759ffb6..674b94556 100644 --- a/web/workspace/deno.json +++ b/web/workspace/deno.json @@ -6,7 +6,7 @@ "dev": "deno run -A npm:vite@7.2.7 dev", "dev:backend": "cd ../.. && cargo run -p yoi-workspace-server --bin yoi-server -- serve --listen 127.0.0.1:8787", "check": "deno run -A npm:@sveltejs/kit@2.49.4 sync && deno run -A npm:svelte-check@4.3.4 --tsconfig ./tsconfig.json", - "test": "deno test --allow-read=src,test,tests --allow-env=LOG,VSCODE_TEXTMATE_DEBUG,NODE_ENV tests/workspace-model.test.ts tests/workspace-catalog.test.ts tests/profile-api.test.ts tests/skill-api.test.ts src/lib/workspace/auth/model.test.ts tests/auth-api.test.ts src/lib/workspace/api/http.test.ts src/lib/workspace/api/workers.test.ts src/lib/workspace/header/breadcrumb-model.test.ts src/lib/workspace/console/chat-submit.test.ts src/lib/workspace/console/composer-delivery.test.ts test/composer-history.test.ts tests/composer-paste.test.ts src/lib/workspace/console/composer-command.test.ts src/lib/workspace/console/composer-draft.test.ts src/lib/workspace/console/composer-completion.test.ts src/lib/workspace/console/markdown.test.ts test/console/ansi.test.ts src/lib/workspace/console/model.test.ts src/lib/workspace/companion/api.test.ts tests/workdir-api.test.ts src/lib/workspace/console/tasks.test.ts test/ticket-detail-route-reuse.test.ts test/objective-pages.test.ts test/repositories/ui.test.ts src/lib/workspace/console/worker-console.ui.test.ts src/lib/workspace/settings/model.test.ts tests/runtime-connection.test.ts tests/runtime-management.test.ts tests/runtime-management-source.test.ts src/lib/workspace/sidebar/override-stack.test.ts src/lib/workspace/sidebar/workers.test.ts src/lib/workspace/sidebar/workspace-switcher.test.ts src/lib/workspace/sidebar/worker-subscription.test.ts src/lib/workspace/sidebar/worker-launch.test.ts test/sidebar/worker-actions.test.ts src/lib/workspace/tickets/merge-request-resources.test.ts src/lib/workspace/tickets/ticket-panel.test.ts test/merge-request-status.test.ts test/config-source/decodal-grammar.test.ts test/config-source/editor-state.test.ts test/config-source/fixed-schema-wrapper.test.ts test/config-source/toolchain.test.ts test/config-source/wasm-parity.test.ts test/repository-access/api.test.ts test/repository-access/loader.test.ts test/repository-access/ui.test.ts test/repository-ssh-connection-ui.test.ts", + "test": "deno test --allow-read=src,test,tests --allow-env=LOG,VSCODE_TEXTMATE_DEBUG,NODE_ENV tests/workspace-model.test.ts tests/workspace-catalog.test.ts tests/profile-api.test.ts tests/skill-api.test.ts src/lib/workspace/auth/model.test.ts tests/auth-api.test.ts src/lib/workspace/api/http.test.ts src/lib/workspace/api/workers.test.ts src/lib/workspace/header/breadcrumb-model.test.ts src/lib/workspace/console/chat-submit.test.ts src/lib/workspace/console/composer-delivery.test.ts test/composer-history.test.ts tests/composer-paste.test.ts src/lib/workspace/console/composer-command.test.ts src/lib/workspace/console/composer-draft.test.ts src/lib/workspace/console/composer-completion.test.ts src/lib/workspace/console/markdown.test.ts test/console/ansi.test.ts src/lib/workspace/console/model.test.ts src/lib/workspace/console/run-status.test.ts src/lib/workspace/companion/api.test.ts tests/workdir-api.test.ts src/lib/workspace/console/tasks.test.ts test/ticket-detail-route-reuse.test.ts test/objective-pages.test.ts test/repositories/ui.test.ts src/lib/workspace/console/worker-console.ui.test.ts src/lib/workspace/settings/model.test.ts tests/runtime-connection.test.ts tests/runtime-management.test.ts tests/runtime-management-source.test.ts src/lib/workspace/sidebar/override-stack.test.ts src/lib/workspace/sidebar/workers.test.ts src/lib/workspace/sidebar/workspace-switcher.test.ts src/lib/workspace/sidebar/worker-subscription.test.ts src/lib/workspace/sidebar/worker-launch.test.ts test/sidebar/worker-actions.test.ts src/lib/workspace/tickets/merge-request-resources.test.ts src/lib/workspace/tickets/ticket-panel.test.ts test/merge-request-status.test.ts test/config-source/decodal-grammar.test.ts test/config-source/editor-state.test.ts test/config-source/fixed-schema-wrapper.test.ts test/config-source/toolchain.test.ts test/config-source/wasm-parity.test.ts test/repository-access/api.test.ts test/repository-access/loader.test.ts test/repository-access/ui.test.ts test/repository-ssh-connection-ui.test.ts", "build": "deno run -A npm:vite@7.2.7 build", "preview": "deno run -A npm:vite@7.2.7 preview" }, diff --git a/web/workspace/src/lib/workspace/console/WorkerRunStatus.svelte b/web/workspace/src/lib/workspace/console/WorkerRunStatus.svelte index b057d9f9d..0d7e2ed63 100644 --- a/web/workspace/src/lib/workspace/console/WorkerRunStatus.svelte +++ b/web/workspace/src/lib/workspace/console/WorkerRunStatus.svelte @@ -1,7 +1,14 @@ + + + {@render children?.()} + diff --git a/web/workspace/src/lib/workspace/console/RichMarkdown.browser.spec.ts b/web/workspace/src/lib/workspace/console/RichMarkdown.browser.spec.ts new file mode 100644 index 000000000..42650f66a --- /dev/null +++ b/web/workspace/src/lib/workspace/console/RichMarkdown.browser.spec.ts @@ -0,0 +1,70 @@ +// @vitest-environment happy-dom + +import { cleanup, render, waitFor } from "@testing-library/svelte"; +import { afterEach, expect, test } from "vitest"; +import RichMarkdown from "./RichMarkdown.svelte"; + +afterEach(cleanup); + +test("preserves safe links, blocks raw HTML, and renders Shiki code", async () => { + const source = [ + "[unsafe](javascript:alert(1)) and [safe](https://example.com)", + "", + '', + "", + "```ts", + "const answer: number = 42;", + "```", + ].join("\n"); + const { container } = render(RichMarkdown, { + text: source, + streamId: "message-security", + }); + + await waitFor(() => { + expect(container.querySelectorAll("a")).toHaveLength(2); + expect(container.querySelector("pre.shiki code span")).not.toBeNull(); + }); + + const [unsafe, safe] = container.querySelectorAll("a"); + expect(unsafe.getAttribute("href")).toBe(""); + expect(safe.getAttribute("href")).toBe("https://example.com"); + expect(safe.getAttribute("target")).toBe("_blank"); + expect(safe.getAttribute("rel")).toBe("noreferrer"); + expect(container.querySelector("img")).toBeNull(); + expect(container.querySelector("script")).toBeNull(); +}); + +test("keeps completed DOM nodes while a later markdown block grows", async () => { + const initial = [ + "# Stable heading", + "", + "This paragraph is already complete.", + "", + "The final paragraph is stream", + ].join("\n"); + const { container, rerender } = render(RichMarkdown, { + text: initial, + streamId: "message-1", + }); + + await waitFor(() => { + expect(container.querySelector("h1")?.textContent).toBe("Stable heading"); + expect(container.querySelectorAll("p")).toHaveLength(2); + }); + const heading = container.querySelector("h1"); + const completedParagraph = container.querySelector("p"); + + await rerender({ + text: `${initial}ed incrementally.`, + streamId: "message-1", + }); + await waitFor(() => { + expect(container.querySelectorAll("p")[1]?.textContent).toBe( + "The final paragraph is streamed incrementally.", + ); + }); + + expect(container.querySelector("h1")).toBe(heading); + expect(container.querySelector("p")).toBe(completedParagraph); +}); diff --git a/web/workspace/src/lib/workspace/console/RichMarkdown.svelte b/web/workspace/src/lib/workspace/console/RichMarkdown.svelte index da585bd38..c609ec121 100644 --- a/web/workspace/src/lib/workspace/console/RichMarkdown.svelte +++ b/web/workspace/src/lib/workspace/console/RichMarkdown.svelte @@ -1,41 +1,49 @@ + + -
- {#if html} - {@html html} - {:else} -

{text}

- {/if} +
+