feat: OpenAPI Migration with Generated TypeScript Types#486
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughAdds OpenAPI/utoipa annotations across the Rust API, generates an OpenAPI spec and Swagger UI, introduces a typed TypeScript API surface (centralized schema types and a typed client), widens several frontend prop types to accept null, adds TTS/audio hook stubs, and makes memory/agent workspace type and DB-binding adjustments. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![tembo[bot]](https://avatars.githubusercontent.com/in/1223475?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 20
🧹 Nitpick comments (3)
src/main.rs (1)
1542-1553: Factor theAgentInfoconversion into one helper.The same field-by-field mapping now lives in two places. Since
workspaceformatting just changed, these call sites can drift again; a single constructor/helper would keep the API shape consistent.Also applies to: 2842-2853
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main.rs` around lines 1542 - 1553, Create a single helper to construct spacebot::api::AgentInfo from the internal Agent struct (e.g., implement From<&Agent> for spacebot::api::AgentInfo or add a function like agent_to_agent_info(agent: &Agent) -> spacebot::api::AgentInfo) and move the field-by-field mapping (id, display_name, role, gradient_start, gradient_end, workspace formatting via to_string_lossy().to_string(), context_window, max_turns, max_concurrent_branches, max_concurrent_workers) into that helper; then replace the two duplicated map closures that build AgentInfo with calls to this helper to keep the conversion logic centralized and consistent (update both places where AgentInfo is constructed).src/lib.rs (1)
40-42: Avoid panicking from this public helper.
openapi_json()is part of the library surface now, so thisexpectturns a serialization failure into a process abort for any caller. Returning aResult<String>here and letting theopenapi-specbinary decide whether toexpectwould keep the failure mode local.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/lib.rs` around lines 40 - 42, The public helper openapi_json currently calls api::api_router().split_for_parts() and then api.to_json().expect(...), which panics on serialization failure; change openapi_json's signature to return a Result<String, E> (propagate the specific error type from api.to_json, e.g., the openapi-spec serde/error type or a boxed std::error::Error) and replace the expect with a ?-based propagation so callers handle failures; keep the body using api::api_router() and api.to_json() but return the Result instead of aborting the process.src/api/messaging.rs (1)
921-930: Consider using typed response structs instead ofserde_json::Value.The
disconnect_platformandtoggle_platformendpoints documentbody = serde_json::Valueas the response type. While this works, it provides less schema information to API consumers. The handlers already return a JSON object withsuccessandmessagefields — consider defining a typed response struct (similar toMessagingInstanceActionResponse) for better OpenAPI documentation.Also applies to: 1113-1122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/messaging.rs` around lines 921 - 930, The OpenAPI docs for the disconnect_platform and toggle_platform endpoints use serde_json::Value as the response body which loses schema details; define a typed response struct (e.g., MessagingInstanceActionResponse or a new DisconnectToggleResponse with fields success: bool and message: String) and update the utoipa::path attributes for disconnect_platform and toggle_platform to use that struct as body, and ensure the handlers return/serialize that struct instead of a raw serde_json::Value so the generated docs include a proper schema.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/api/client-typed.ts`:
- Around line 10-14: Client requests are getting a double "/api" when backend
paths already include that prefix; update getClient to match the chosen backend
convention by (a) deciding whether backend Utoipa paths include "/api" or not,
and then (b) making createClient's baseUrl construction in getClient respect
that choice: if backend paths include "/api" remove the hardcoded "/api" suffix
from the baseUrl logic, otherwise keep or ensure "/api" is present; adjust the
baseUrl expression near createClient<paths> and ensure getAuthHeaders usage
remains unchanged.
- Line 1: The import createClient from "openapi-fetch" is failing because the
package is not declared as a dependency; add "openapi-fetch" to
interface/package.json dependencies (or devDependencies as appropriate) and run
bun install to update the lockfile (interface/bun.lock), then commit the updated
package.json and lockfile so the import in client-typed.ts resolves.
In `@interface/src/api/client.ts`:
- Around line 195-200: AgentMessageEvent lacks a discriminant so it can’t be
matched as part of the ApiEvent union; add a literal type field (e.g. type:
"agent.message") to the AgentMessageEvent interface and update the ApiEvent
union to include that literal variant so it becomes reachable; apply the same
fix to the other event interfaces referenced in the same area (the block around
the other missing discriminants) by giving each a unique type literal and
ensuring each is included in the ApiEvent union.
- Around line 819-826: The local GlobalSettingsUpdate interface in client.ts is
missing the new ssh_enabled field and should be replaced with the canonical
schema type; remove the local export of GlobalSettingsUpdate and re-export or
alias the generated schema type (Types.GlobalSettingsUpdate) instead (e.g.,
export type GlobalSettingsUpdate = Types.GlobalSettingsUpdate) so callers
receive the up-to-date shape including ssh_enabled and any future additions;
update any references in this file that referred to the local interface to use
the re-exported GlobalSettingsUpdate.
- Around line 1925-1928: The registrySkillContent function is using the wrong
response type (it currently promises the installed-skill payload); update its
fetchJson generic to the lightweight registry response type used by
/skills/registry/content (e.g., replace SkillContentResponse with the registry
content type such as RegistrySkillContentResponse or the appropriate name
exported from your types) so the returned shape matches { source, skill_id,
content } and not the installed-skill fields; ensure the symbol
registrySkillContent and the fetchJson call are updated accordingly.
In `@interface/src/routes/AgentDetail.tsx`:
- Around line 212-217: The code is suppressing a type mismatch by casting
overviewData.recent_cortex_events to CortexEvent[] when the API schema lacks
agent_id; instead fix it by either (A) updating the generated schema to include
agent_id, (B) changing the client CortexEvent interface to the API shape, or (C)
map overviewData.recent_cortex_events to the client shape before passing to
CortexEventsSection — e.g., transform each item from
overviewData.recent_cortex_events (from overviewData) into an object matching
CortexEvent (adding agent_id from agentId) and pass that array to
CortexEventsSection rather than using the as CortexEvent[] cast.
In `@interface/src/routes/ChannelDetail.tsx`:
- Around line 274-275: Remove the redundant type assertions on TimelineItem:
delete the "as TimelineBranchRun" and "as TimelineWorkerRun" casts and pass the
item directly to the components (e.g., LiveBranchRunItem, BranchRunItem and the
corresponding worker-run components) so TypeScript can use the
discriminated-union narrowing by item.type; update the JSX in ChannelDetail.tsx
to supply item without casting.
In `@src/api/agents.rs`:
- Around line 1101-1112: Handlers update_agent, delete_agent, and upload_avatar
are returning 200 OK with a JSON {success: false} for validation errors, which
conflicts with the OpenAPI spec that declares 400 responses; update these
handlers (update_agent, delete_agent, upload_avatar) to detect input/validation
failures (empty IDs, unsupported image types, etc.) and return proper 400 Bad
Request responses (e.g., HttpResponse::BadRequest or
actix_web::error::ErrorBadRequest) with a clear error body instead of encoding
success: false under 200, and ensure any corresponding utoipa annotations remain
accurate for the 400 case.
In `@src/api/bindings.rs`:
- Around line 701-711: The OpenAPI annotation for the PUT /bindings endpoint
(utoipa::path using UpdateBindingRequest and UpdateBindingResponse) disagrees
with the handler behavior: handlers return 200 with { success: false } when a
binding/config is missing but the spec advertises 404. Fix by making them
consistent — either update the annotation to document the actual 200 response
for "not found" (add a 200 response description clarifying { success: false }
for missing binding) or change the handler(s) that currently return 200 with {
success: false } to return a proper 404 status when a binding/config is absent;
locate the binding update handler(s) in bindings.rs that construct
UpdateBindingResponse and adjust the response code or the utoipa::path responses
accordingly.
In `@src/api/cortex.rs`:
- Around line 75-89: The Utoipa route annotations in src/api/cortex.rs use
"/cortex/chat/*" but the shipped client expects "/cortex-chat/*"; update the
path strings in the utoipa::path attributes for the endpoint definitions (the
annotated items that handle chat messages, send, threads, and thread
deletion—look for the utoipa::path blocks around the handler functions for
messages, send, threads, and delete_thread) to match the frontend routes (e.g.,
"/cortex-chat/messages", "/cortex-chat/send", "/cortex-chat/threads", "DELETE
/cortex-chat/thread"); apply the same change to the other annotated blocks noted
(around the blocks currently at 130-141, 211-223, 241-251) so the generated
OpenAPI client targets the shipped route table.
In `@src/api/cron.rs`:
- Around line 303-314: The OpenAPI spec defines PUT /agents/cron and POST
/agents/cron/toggle, but the handwritten client uses the opposite verbs; update
the server annotation and/or the handwritten typed client so both agree: ensure
the route annotated with "/agents/cron" (utoipa::path on the
CreateCronRequest/CronActionResponse handler) uses PUT, and ensure the client
method that invokes the cron creation/mutation (the function in the client that
currently calls "/agents/cron") issues a PUT request; likewise ensure the client
method that toggles cron (the function calling "/agents/cron/toggle") issues a
POST request so the handwritten client verbs match the OpenAPI spec and the
generated client.
In `@src/api/factory.rs`:
- Around line 25-39: The documented response currently declares a bare Preset
but the handler get_preset returns JSON with an envelope { "preset": Preset },
so update the utoipa::path responses to match that envelope: define or reuse a
response wrapper type (e.g., PresetResponse with a field preset:
crate::factory::presets::Preset) or use an inline schema, then change the
(status = 200, body = ...) entry to reference that wrapper type; ensure the
annotation and any imports reflect the new PresetResponse and keep the handler
(get_preset and PresetRegistry::load) unchanged.
- Around line 11-21: The OpenAPI schema and returned JSON are inconsistent:
list_presets currently returns Json({ "presets": [...] }) but the utoipa::path
body is declared as Vec<PresetMeta>; fix by aligning them — either change
list_presets to return the raw array (Json(presets)) or define a response DTO
struct (e.g., PresetsResponse with a presets: Vec<PresetMeta> field) and update
the utoipa::path body to that DTO; update the function return and the annotation
accordingly (referencing list_presets, PresetRegistry::list, and PresetMeta).
In `@src/api/providers.rs`:
- Around line 536-545: The documented responses (400/404) in the utoipa::path
macro for endpoints like the OpenAi browser-oauth start (request type
OpenAiOAuthBrowserStartRequest, response OpenAiOAuthBrowserStartResponse) don't
match the implementation which returns 200 with success: false; update the
handlers to return proper HTTP errors (e.g., return
Err(axum::response::IntoResponse with StatusCode::BAD_REQUEST/NOT_FOUND and an
error body) from the handler functions) so the runtime matches the documented
400/404 responses, or alternatively change the utoipa annotations and response
types to explicitly model a 200 body that contains success:false for
validation/lookup failures; make the same change consistently for the other
routes called out (the handlers referenced around lines 731-742, 796-805,
885-894, 980-992) so spec and implementation are aligned.
In `@src/api/server.rs`:
- Around line 271-281: The opencode proxy routes and Swagger UI are mounted
directly on the top-level app and bypass the authentication middleware; move or
wrap the proxy and docs routes so they pass through the existing
api_auth_middleware rather than being attached outside of it. Concretely, ensure
the routes using opencode_proxy::opencode_proxy and the
utoipa_swagger_ui::SwaggerUi::new("/api/docs")/"/api/openapi.json" are nested
under the same middleware stack as api_routes (or apply api_auth_middleware to
their scope) so that api_auth_middleware is invoked for these endpoints and the
docs/schema and /api/opencode/{...} require the same bearer auth as the rest of
the API.
- Around line 32-41: Extend the ApiDoc OpenApi derive to advertise bearer token
auth: update the #[openapi(...)] on the ApiDoc struct to add a components entry
that defines a "bearerAuth" security scheme of type HTTP with scheme "bearer"
(bearer format JWT) and add a global security requirement referencing
"bearerAuth" so protected endpoints are marked as requiring Authorization:
Bearer <token>; modify the ApiDoc (struct ApiDoc, #[derive(OpenApi)] /
#[openapi(...)] attributes) accordingly.
- Around line 43-46: The OpenAPI spec is generated without the "/api" base path
so documented routes don't match the live server; after building the OpenAPI
object from ApiDoc::openapi() (used in api_router() and in start_http_server()),
add a server entry or set the basePath to "/api" on that OpenAPI object before
passing it into OpenApiRouter::with_openapi() (or return it from
ApiDoc::openapi()); update the code that constructs the spec (reference
ApiDoc::openapi(), api_router(), and OpenApiRouter::with_openapi) to inject a
Server with url "/api" (or equivalent basePath setting) so generated paths
become "/api/..." and match the nested router.
In `@src/api/settings.rs`:
- Around line 88-96: The OpenAPI annotations hardcode an "/api" prefix causing
mismatched paths; update the utoipa::path attributes in this module (e.g., the
attribute currently annotating the handler returning GlobalSettingsResponse and
the ones documenting update-check, update-apply, settings/raw) to remove the
"/api" prefix and use the normalized routes the client expects (e.g.,
"/settings", "/update/check", "/update/apply", "/config/raw"); apply the same
change to the other annotated blocks mentioned (around the regions covering
lines 245-254 and 453-567) so all documented paths are consistent with
getApiBase()-style base URLs.
In `@src/api/skills.rs`:
- Around line 263-274: The OpenAPI annotation for the "/agents/skills/remove"
route currently uses post in the utoipa::path attribute but the frontend client
(interface/src/api/client.ts) calls DELETE; make them consistent by changing the
utoipa::path HTTP verb from post to delete so the generated client matches the
existing DELETE request for "/agents/skills/remove" (update the attribute on the
macro that declares the path in src/api/skills.rs).
In `@src/api/ssh.rs`:
- Around line 49-59: The route paths in the utoipa::path attributes include an
extra "/api" prefix causing double nesting (e.g. path =
"/api/ssh/authorized-key" and path = "/api/ssh/status"); update those attributes
in src/api/ssh.rs to remove the prefix so they read "/ssh/authorized-key" and
"/ssh/status" respectively so the documented paths match the main router nesting
under /api.
---
Nitpick comments:
In `@src/api/messaging.rs`:
- Around line 921-930: The OpenAPI docs for the disconnect_platform and
toggle_platform endpoints use serde_json::Value as the response body which loses
schema details; define a typed response struct (e.g.,
MessagingInstanceActionResponse or a new DisconnectToggleResponse with fields
success: bool and message: String) and update the utoipa::path attributes for
disconnect_platform and toggle_platform to use that struct as body, and ensure
the handlers return/serialize that struct instead of a raw serde_json::Value so
the generated docs include a proper schema.
In `@src/lib.rs`:
- Around line 40-42: The public helper openapi_json currently calls
api::api_router().split_for_parts() and then api.to_json().expect(...), which
panics on serialization failure; change openapi_json's signature to return a
Result<String, E> (propagate the specific error type from api.to_json, e.g., the
openapi-spec serde/error type or a boxed std::error::Error) and replace the
expect with a ?-based propagation so callers handle failures; keep the body
using api::api_router() and api.to_json() but return the Result instead of
aborting the process.
In `@src/main.rs`:
- Around line 1542-1553: Create a single helper to construct
spacebot::api::AgentInfo from the internal Agent struct (e.g., implement
From<&Agent> for spacebot::api::AgentInfo or add a function like
agent_to_agent_info(agent: &Agent) -> spacebot::api::AgentInfo) and move the
field-by-field mapping (id, display_name, role, gradient_start, gradient_end,
workspace formatting via to_string_lossy().to_string(), context_window,
max_turns, max_concurrent_branches, max_concurrent_workers) into that helper;
then replace the two duplicated map closures that build AgentInfo with calls to
this helper to keep the conversion logic centralized and consistent (update both
places where AgentInfo is constructed).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 17539c3b-264d-4619-abd2-160a2f882579
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomlinterface/bun.lockis excluded by!**/*.lock,!**/*.lockinterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (62)
interface/src/api/client-typed.tsinterface/src/api/client.tsinterface/src/api/schema.d.tsinterface/src/api/types.tsinterface/src/components/ProfileAvatar.tsxinterface/src/components/Sidebar.tsxinterface/src/components/TaskBoard.tsxinterface/src/components/TopologyGraph.tsxinterface/src/hooks/useAudioRecorder.tsinterface/src/hooks/useTtsPlayback.tsinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentWorkers.tsxinterface/src/routes/ChannelDetail.tsxinterface/src/routes/GlobalTasks.tsxinterface/src/routes/Orchestrate.tsxjustfilesrc/agent/cortex.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/bindings.rssrc/api/channels.rssrc/api/config.rssrc/api/cortex.rssrc/api/cron.rssrc/api/factory.rssrc/api/ingest.rssrc/api/links.rssrc/api/mcp.rssrc/api/memories.rssrc/api/messaging.rssrc/api/models.rssrc/api/projects.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/skills.rssrc/api/ssh.rssrc/api/state.rssrc/api/system.rssrc/api/tasks.rssrc/api/tools.rssrc/api/webchat.rssrc/api/workers.rssrc/bin/openapi_spec.rssrc/config/types.rssrc/conversation/history.rssrc/conversation/worker_transcript.rssrc/cron/store.rssrc/factory/presets.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/memory/store.rssrc/memory/types.rssrc/opencode/types.rssrc/projects/store.rssrc/secrets/store.rssrc/tasks/store.rssrc/tools/memory_save.rssrc/update.rs
👮 Files not reviewed due to content moderation or server errors (8)
- src/config/types.rs
- src/api/secrets.rs
- src/api/models.rs
- src/api/system.rs
- src/api/links.rs
- src/bin/openapi_spec.rs
- src/tools/memory_save.rs
- src/cron/store.rs
jamiepine
force-pushed
the
feat/openapi-migration
branch
2 times, most recently
from
b84f6aa to
d9ff5ec
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/agent/cortex_chat.rs (1)
34-36:⚠️ Potential issue | 🟡 MinorThe
tool_callsdescription now documents the wrong wire format.Line 34 says this is a “Serialized JSON array”, but the public API model exposes
Option<Vec<CortexChatToolCall>>. WithToSchemaadded, that misleading description will now show up in the generated docs.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/agent/cortex_chat.rs` around lines 34 - 36, Update the doc comment for the CortexChat struct field tool_calls to reflect the actual wire/API format (it is an Option<Vec<CortexChatToolCall>>), not a "Serialized JSON array"; change the description to something like "Optional list of CortexChatToolCall objects for assistant messages" so the generated docs from ToSchema are accurate, and ensure the comment is attached to the pub tool_calls: Option<Vec<CortexChatToolCall>> field.
♻️ Duplicate comments (1)
interface/src/api/client.ts (1)
819-826:⚠️ Potential issue | 🟠 MajorReplace the local
GlobalSettingsUpdatewith the schema type.The
updateGlobalSettingsmethod at line 1817 acceptsTypes.GlobalSettingsUpdate, but this exported interface is a stale local copy. Callers importingGlobalSettingsUpdatefrom this file won't see new fields (e.g.,ssh_enabled) that exist in the schema type.Suggested fix
-export interface GlobalSettingsUpdate { - brave_search_key?: string | null; - api_enabled?: boolean; - api_port?: number; - api_bind?: string; - worker_log_mode?: string; - opencode?: OpenCodeSettingsUpdate; -} +export type GlobalSettingsUpdate = Types.GlobalSettingsUpdate;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 819 - 826, The exported local interface GlobalSettingsUpdate is a stale copy and should be replaced with the schema type so callers get up-to-date fields (e.g., ssh_enabled); remove the local interface declaration and instead re-export or alias the schema type used by updateGlobalSettings (use Types.GlobalSettingsUpdate) so all imports of GlobalSettingsUpdate from this file point to the canonical schema type.
🧹 Nitpick comments (5)
src/tools/memory_save.rs (1)
399-402: Consider usingas_deref().map(Arc::from)for cleaner conversion.The current code is correct, but can be simplified to match the idiomatic pattern used elsewhere in this PR:
♻️ Suggested simplification
let event = ProcessEvent::MemorySaved { agent_id: event_context.agent_id.clone(), memory_id: memory.id.clone(), - channel_id: memory - .channel_id - .as_ref() - .map(|s| std::sync::Arc::from(s.as_str())), + channel_id: memory.channel_id.as_deref().map(Arc::from), memory_type: memory.memory_type,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/tools/memory_save.rs` around lines 399 - 402, Replace the verbose conversion for channel_id by using the idiomatic as_deref() -> map(Arc::from) pattern: change the expression that builds channel_id from memory.channel_id.as_ref().map(|s| std::sync::Arc::from(s.as_str())) to use memory.channel_id.as_deref().map(std::sync::Arc::from) so the field construction for channel_id is simpler and consistent with other code; keep references to memory.channel_id and Arc::from unchanged.src/api/system.rs (1)
271-279: Consider documenting 500 responses for completeness.The
backup_exportandbackup_restorehandlers can return500 INTERNAL_SERVER_ERROR(e.g., whenspawn_blockingfails or backup/restore operations fail), but this status is not documented in the OpenAPI annotations. While omitting 500s is a common convention (since they're "unexpected"), explicitly documenting them would improve API clarity for consumers.📝 Optional: Add 500 response documentation
#[utoipa::path( get, path = "/system/backup/export", responses( (status = 200, description = "Backup archive", content_type = "application/zip"), + (status = 500, description = "Internal error during backup generation"), (status = 503, description = "No runtime config available"), ), tag = "system", )]#[utoipa::path( post, path = "/system/backup/restore", request_body = Vec<u8>, responses( (status = 200, description = "Backup restored successfully"), (status = 400, description = "Empty payload"), + (status = 500, description = "Internal error during restore"), (status = 503, description = "No runtime config available"), ), tag = "system", )]Also applies to: 320-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/system.rs` around lines 271 - 279, Add explicit 500 response documentation to the OpenAPI annotations for the backup endpoints: update the utoipa::path attributes on backup_export and backup_restore to include a responses entry like (status = 500, description = "Internal Server Error") (optionally with a content_type if you want a body schema). This ensures the handlers backup_export and backup_restore document possible INTERNAL_SERVER_ERROR returns (e.g., from spawn_blocking or operation failures).src/api/agents.rs (1)
317-327: Define concrete response DTOs instead of usingserde_json::Valuefor OpenAPI spec accuracy.The endpoints at lines 317–327, 519–530, 1101–1112, 1238–1250, 2053–2067, and 2125–2135 still use opaque
serde_json::Valuein their response annotations. This prevents the OpenAPI spec from describing the actual success payloads (success,message,agent_id, etc.), so the frontend cannot generate precise response types for these endpoints. Define concrete response DTOs (or at least document the schema structure) for each endpoint instead of relying on generic JSON values.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 317 - 327, The OpenAPI annotations use opaque serde_json::Value for responses (e.g., the #[utoipa::path(... request_body = ReconnectMcpRequest ...)] entry for POST "/agents/mcp/reconnect"), which prevents accurate schema generation; define concrete response DTO structs (e.g., ReconnectMcpResponse with fields like success: bool, message: String, agent_id: Option<String>) and derive utoipa::ToSchema/serde::Serialize for them, then replace serde_json::Value in the responses(...) tuples with those DTO types; apply the same pattern to the other annotated endpoints referenced (lines 519–530, 1101–1112, 1238–1250, 2053–2067, 2125–2135) so each path uses a concrete response struct in its utoipa::path responses.interface/src/api/client.ts (2)
1802-1812: Consider re-exportingDeleteMessagingInstanceRequestfor consistency.
CreateMessagingInstanceRequestis re-exported (line 81), butDeleteMessagingInstanceRequestused at line 1802 is not. Callers constructing delete requests would need to import directly fromtypes.ts.Suggested addition to re-exports
CreateMessagingInstanceRequest, MessagingInstanceActionResponse, + DeleteMessagingInstanceRequest, } from "./types";🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 1802 - 1812, The file uses Types.DeleteMessagingInstanceRequest in deleteMessagingInstance but that type isn't re-exported for consistency with CreateMessagingInstanceRequest; add a re-export for DeleteMessagingInstanceRequest alongside the existing re-exports (the same place CreateMessagingInstanceRequest is exported) so callers can import it from this module instead of from types.ts directly; specifically add DeleteMessagingInstanceRequest to the export list where CreateMessagingInstanceRequest (and related Messaging types) are re-exported.
1451-1462: Identity types should be added to the OpenAPI schema for consistency.These methods use inline types while similar methods like
agentProfileuseTypes.AgentProfileResponsefrom the generated schema. Identity types (soul,identity,roleresponse) and identity update request type are not currently in the generated schema. Consider adding these to the OpenAPI schema definition so thatagentIdentityandupdateIdentitycan follow the established pattern of usingTypes.*exports.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 1451 - 1462, The inline response/request types used by agentIdentity and updateIdentity are missing from the generated Types schema; add OpenAPI definitions for the identity response (soul, identity, role) and for the updateIdentity request payload, regenerate the client types, and then replace the inline types in client.ts by referencing the new generated Types exports (e.g. Types.AgentIdentityResponse and Types.UpdateIdentityRequest) in the agentIdentity and updateIdentity function signatures and return types to match the pattern used by agentProfile.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/api/client.ts`:
- Around line 1925-1928: The registrySkillContent endpoint is typed
incorrectly—change the generic on fetchJson in the registrySkillContent function
from SkillContentResponse to RegistrySkillContentResponse so the returned shape
matches the OpenAPI schema (content, skill_id, source only); also update any
nearby import or type references to ensure RegistrySkillContentResponse is
imported/available where registrySkillContent is defined.
In `@src/api/bindings.rs`:
- Around line 101-106: The CreateBindingResponse.restart_required field is
always false but should reflect whether platform credentials were added/changed
or a hot-start of the adapter failed; update the create_binding handler to
compute a boolean (e.g., credential_changed || !hot_start_success) and set
CreateBindingResponse.restart_required to that value instead of hardcoding
false, ensuring any code paths that add/update credentials (the logic that
currently logs hot-start failures) return or expose that
credential_changed/hot_start_success status so create_binding can use it when
constructing the response.
In `@src/api/projects.rs`:
- Around line 671-683: The OpenAPI annotations for the POST endpoints handled by
create_repo and create_worktree are missing a 400 Bad Request response for
invalid path/worktree input; update the utoipa::path attributes for the
endpoints (the ones annotating create_repo and create_worktree) to add a
responses entry like (status = 400, description = "Invalid path/worktree:
sanitize_relative_path or sanitize_segment failed") so that invalid inputs
flagged by sanitize_relative_path and sanitize_segment are documented; locate
the annotations surrounding the create_repo and create_worktree functions and
add the 400 response to their responses(...) lists.
- Around line 573-585: The OpenAPI spec for the scan endpoint is missing the 422
response that scan_project can return; update the utoipa attribute on the POST
handler (the #[utoipa::path(...)] block that documents path =
"/agents/projects/{id}/scan", params including AgentQuery and response
ProjectResponse) to add a (status = 422, description = "Unprocessable Entity")
(or use the existing error body type if you have one) so the generated clients
match the runtime behavior where scan_project returns UNPROCESSABLE_ENTITY.
In `@src/api/secrets.rs`:
- Around line 41-49: Add a 500 response to the utoipa::path responses for each
handler that can return StatusCode::INTERNAL_SERVER_ERROR: update the
annotations for secrets_status, list_secrets, put_secret, delete_secret,
unlock_secrets, migrate_secrets, export_secrets, and import_secrets to include
(status = 500, body = serde_json::Value) (or a suitable description) so the
OpenAPI contract accurately reflects internal server errors returned by those
handlers.
In `@src/api/ssh.rs`:
- Around line 104-113: The utoipa annotation for the ssh_status endpoint
declares 400 and 500 responses but the ssh_status handler always returns 200 due
to fallback handling; either remove the unused 400/500 response entries from the
#[utoipa::path] macro or change the ssh_status function to return a
Result/Response that propagates failures as proper HTTP errors (map internal
errors to 500 and bad input to 400) and update the handler signature and
error-to-response mapping accordingly so the documented responses match runtime
behavior. Ensure you modify the annotation near #[utoipa::path(...)] and the
ssh_status function/method where the fallback currently forces a 200.
- Around line 49-59: The OpenAPI annotation declares a 400 response but the
handler set_authorized_key currently returns HTTP 200 for invalid keys; update
the handler so that when input validation fails it returns an HTTP 400 (Bad
Request) response instead of Ok(Json(...)) to match the #[utoipa::path(...,
responses(... (status = 400 ...)))] contract — modify set_authorized_key's
invalid-key branch (and any helper like validate_authorized_key if present) to
produce a 400 response using your framework's error/Result type (e.g., return
Err((StatusCode::BAD_REQUEST, Json(...))) or otherwise map to a BadRequest
response) and ensure the response body shape still matches AuthorizedKeyResponse
or adjust the utoipa entry if you choose to remove the 400 case instead.
---
Outside diff comments:
In `@src/agent/cortex_chat.rs`:
- Around line 34-36: Update the doc comment for the CortexChat struct field
tool_calls to reflect the actual wire/API format (it is an
Option<Vec<CortexChatToolCall>>), not a "Serialized JSON array"; change the
description to something like "Optional list of CortexChatToolCall objects for
assistant messages" so the generated docs from ToSchema are accurate, and ensure
the comment is attached to the pub tool_calls: Option<Vec<CortexChatToolCall>>
field.
---
Duplicate comments:
In `@interface/src/api/client.ts`:
- Around line 819-826: The exported local interface GlobalSettingsUpdate is a
stale copy and should be replaced with the schema type so callers get up-to-date
fields (e.g., ssh_enabled); remove the local interface declaration and instead
re-export or alias the schema type used by updateGlobalSettings (use
Types.GlobalSettingsUpdate) so all imports of GlobalSettingsUpdate from this
file point to the canonical schema type.
---
Nitpick comments:
In `@interface/src/api/client.ts`:
- Around line 1802-1812: The file uses Types.DeleteMessagingInstanceRequest in
deleteMessagingInstance but that type isn't re-exported for consistency with
CreateMessagingInstanceRequest; add a re-export for
DeleteMessagingInstanceRequest alongside the existing re-exports (the same place
CreateMessagingInstanceRequest is exported) so callers can import it from this
module instead of from types.ts directly; specifically add
DeleteMessagingInstanceRequest to the export list where
CreateMessagingInstanceRequest (and related Messaging types) are re-exported.
- Around line 1451-1462: The inline response/request types used by agentIdentity
and updateIdentity are missing from the generated Types schema; add OpenAPI
definitions for the identity response (soul, identity, role) and for the
updateIdentity request payload, regenerate the client types, and then replace
the inline types in client.ts by referencing the new generated Types exports
(e.g. Types.AgentIdentityResponse and Types.UpdateIdentityRequest) in the
agentIdentity and updateIdentity function signatures and return types to match
the pattern used by agentProfile.
In `@src/api/agents.rs`:
- Around line 317-327: The OpenAPI annotations use opaque serde_json::Value for
responses (e.g., the #[utoipa::path(... request_body = ReconnectMcpRequest ...)]
entry for POST "/agents/mcp/reconnect"), which prevents accurate schema
generation; define concrete response DTO structs (e.g., ReconnectMcpResponse
with fields like success: bool, message: String, agent_id: Option<String>) and
derive utoipa::ToSchema/serde::Serialize for them, then replace
serde_json::Value in the responses(...) tuples with those DTO types; apply the
same pattern to the other annotated endpoints referenced (lines 519–530,
1101–1112, 1238–1250, 2053–2067, 2125–2135) so each path uses a concrete
response struct in its utoipa::path responses.
In `@src/api/system.rs`:
- Around line 271-279: Add explicit 500 response documentation to the OpenAPI
annotations for the backup endpoints: update the utoipa::path attributes on
backup_export and backup_restore to include a responses entry like (status =
500, description = "Internal Server Error") (optionally with a content_type if
you want a body schema). This ensures the handlers backup_export and
backup_restore document possible INTERNAL_SERVER_ERROR returns (e.g., from
spawn_blocking or operation failures).
In `@src/tools/memory_save.rs`:
- Around line 399-402: Replace the verbose conversion for channel_id by using
the idiomatic as_deref() -> map(Arc::from) pattern: change the expression that
builds channel_id from memory.channel_id.as_ref().map(|s|
std::sync::Arc::from(s.as_str())) to use
memory.channel_id.as_deref().map(std::sync::Arc::from) so the field construction
for channel_id is simpler and consistent with other code; keep references to
memory.channel_id and Arc::from unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: cf4d80a9-8b04-46d2-881f-b591b16e8a55
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomlinterface/bun.lockis excluded by!**/*.lock,!**/*.lockinterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (62)
interface/src/api/client-typed.tsinterface/src/api/client.tsinterface/src/api/schema.d.tsinterface/src/api/types.tsinterface/src/components/ProfileAvatar.tsxinterface/src/components/Sidebar.tsxinterface/src/components/TaskBoard.tsxinterface/src/components/TopologyGraph.tsxinterface/src/hooks/useAudioRecorder.tsinterface/src/hooks/useTtsPlayback.tsinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentWorkers.tsxinterface/src/routes/ChannelDetail.tsxinterface/src/routes/GlobalTasks.tsxinterface/src/routes/Orchestrate.tsxjustfilesrc/agent/cortex.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/bindings.rssrc/api/channels.rssrc/api/config.rssrc/api/cortex.rssrc/api/cron.rssrc/api/factory.rssrc/api/ingest.rssrc/api/links.rssrc/api/mcp.rssrc/api/memories.rssrc/api/messaging.rssrc/api/models.rssrc/api/projects.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/skills.rssrc/api/ssh.rssrc/api/state.rssrc/api/system.rssrc/api/tasks.rssrc/api/tools.rssrc/api/webchat.rssrc/api/workers.rssrc/bin/openapi_spec.rssrc/config/types.rssrc/conversation/history.rssrc/conversation/worker_transcript.rssrc/cron/store.rssrc/factory/presets.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/memory/store.rssrc/memory/types.rssrc/opencode/types.rssrc/projects/store.rssrc/secrets/store.rssrc/tasks/store.rssrc/tools/memory_save.rssrc/update.rs
✅ Files skipped from review due to trivial changes (28)
- src/api.rs
- src/cron/store.rs
- src/conversation/history.rs
- src/factory/presets.rs
- src/main.rs
- src/conversation/worker_transcript.rs
- src/opencode/types.rs
- src/config/types.rs
- src/tasks/store.rs
- src/api/workers.rs
- src/api/factory.rs
- src/api/webchat.rs
- src/api/cron.rs
- src/secrets/store.rs
- interface/src/hooks/useTtsPlayback.ts
- src/api/config.rs
- src/agent/cortex.rs
- src/api/cortex.rs
- interface/src/hooks/useAudioRecorder.ts
- src/mcp.rs
- src/api/ingest.rs
- src/api/models.rs
- src/api/mcp.rs
- src/api/settings.rs
- src/api/links.rs
- src/api/memories.rs
- src/api/messaging.rs
- interface/src/api/types.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- interface/src/routes/Orchestrate.tsx
- interface/src/routes/GlobalTasks.tsx
- src/bin/openapi_spec.rs
- src/lib.rs
- interface/src/components/ProfileAvatar.tsx
- interface/src/components/TaskBoard.tsx
- interface/src/components/TopologyGraph.tsx
- justfile
- src/projects/store.rs
- src/update.rs
- interface/src/api/client-typed.ts
- src/memory/types.rs
- interface/src/routes/ChannelDetail.tsx
- src/api/server.rs
- src/api/providers.rs
- src/api/channels.rs
- src/api/tasks.rs
- src/api/skills.rs
Comment on lines
1925
to
1928
| registrySkillContent: (source: string, skillId: string) => | ||
| fetchJson<RegistrySkillContentResponse>( | ||
| fetchJson<SkillContentResponse>( | ||
| `/skills/registry/content?source=${encodeURIComponent(source)}&skill_id=${encodeURIComponent(skillId)}`, | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check what Types are exported for registry skill content
rg -n "RegistrySkillContent" interface/src/api/types.ts
# Check the backend endpoint response structure
ast-grep --pattern 'struct RegistrySkillContentResponse {
$$$
}'
# Also check if SkillContentResponse in schema matches local definition
rg -n "SkillContentResponse" interface/src/api/types.ts -A 10Repository: spacedriveapp/spacebot
Length of output: 1433
🏁 Script executed:
# Find the OpenAPI schema file
fd -e yaml -e json -e ts -n "openapi\|schema\|spec" interface/ | head -20
# Also search for where these schemas are defined
rg -l "RegistrySkillContentResponse\|SkillContentResponse" interface/ --type json --type yaml | head -10Repository: spacedriveapp/spacebot
Length of output: 296
🏁 Script executed:
# Look at the types file to see the actual schema definitions
# First, let's check if there's a generated schema or types file
fd "openapi\|schema\|generated" interface/ -e json -e yaml -e tsRepository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check the actual request at the endpoint location to see what types are in use elsewhere
rg -n "registrySkillContent" interface/src/ -B 5 -A 5Repository: spacedriveapp/spacebot
Length of output: 1621
🏁 Script executed:
# See what fields are accessed from the registrySkillContent response
sed -n '120,200p' interface/src/routes/AgentSkills.tsx | cat -nRepository: spacedriveapp/spacebot
Length of output: 2964
🏁 Script executed:
# Also check the types.ts file to understand the schema imports
sed -n '240,260p' interface/src/api/types.ts | cat -nRepository: spacedriveapp/spacebot
Length of output: 1355
🏁 Script executed:
# Look for the OpenAPI spec or generated types to understand the schema definitions
fd "openapi" interface/ -e yaml -e json | head -5Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check if there are comments or API documentation about what these endpoints return
rg -A 20 "skills/registry/content" interface/ --type ts --type tsxRepository: spacedriveapp/spacebot
Length of output: 93
🏁 Script executed:
# Look for what fields are accessed from the data variable in AgentSkills.tsx
sed -n '75,200p' interface/src/routes/AgentSkills.tsx | cat -nRepository: spacedriveapp/spacebot
Length of output: 4665
🏁 Script executed:
# Search for where the content/data from registrySkillContent is actually rendered
rg -A 30 "data\?" interface/src/routes/AgentSkills.tsx | head -60Repository: spacedriveapp/spacebot
Length of output: 839
🏁 Script executed:
# Check if there's an OpenAPI spec file
find interface/ -name "*.yaml" -o -name "*.yml" -o -name "*.json" | grep -i "openapi\|spec\|schema" | head -10Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check for local definitions of SkillContentResponse in client.ts
sed -n '880,900p' interface/src/api/client.ts | cat -nRepository: spacedriveapp/spacebot
Length of output: 558
🏁 Script executed:
# Search for any local type definitions of these response types
rg -n "type SkillContentResponse\|interface SkillContentResponse" interface/src/api/client.ts -A 10Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Check if RegistrySkillContentResponse is used anywhere in the codebase
rg "RegistrySkillContentResponse" interface/src/ --type tsRepository: spacedriveapp/spacebot
Length of output: 385
🏁 Script executed:
# Check what RegistrySkillContentResponse contains in the schema
sed -n '1,100p' interface/src/api/schema.d.ts | grep -A 30 "RegistrySkillContentResponse:" | head -40Repository: spacedriveapp/spacebot
Length of output: 48
🏁 Script executed:
# Also look for the full definition in schema.d.ts
rg -A 15 "RegistrySkillContentResponse:" interface/src/api/schema.d.ts | head -50Repository: spacedriveapp/spacebot
Length of output: 591
Use RegistrySkillContentResponse instead of SkillContentResponse for this endpoint.
The OpenAPI schema defines the /skills/registry/content endpoint to return RegistrySkillContentResponse (containing only content, skill_id, and source), but the code declares it as SkillContentResponse. The current type includes fields like source_repo and base_dir that don't exist in the actual endpoint response, causing type mismatches and potential runtime errors.
Current vs. Correct Type
RegistrySkillContentResponse (actual schema):
content?: string | null
skill_id: string
source: string
SkillContentResponse (currently used):
name: string
description: string
content: string
file_path: string
base_dir: string
source: string
source_repo?: string
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@interface/src/api/client.ts` around lines 1925 - 1928, The
registrySkillContent endpoint is typed incorrectly—change the generic on
fetchJson in the registrySkillContent function from SkillContentResponse to
RegistrySkillContentResponse so the returned shape matches the OpenAPI schema
(content, skill_id, source only); also update any nearby import or type
references to ensure RegistrySkillContentResponse is imported/available where
registrySkillContent is defined.
Comment on lines
+101
to
106
| #[derive(Serialize, utoipa::ToSchema)] | ||
| pub(super) struct CreateBindingResponse { | ||
| success: bool, | ||
| /// True if platform credentials were added/changed (adapter needs restart). | ||
| restart_required: bool, | ||
| message: String, |
Contributor
There was a problem hiding this comment.
restart_required never reflects the real outcome.
Line 104 documents this as the signal that credential changes need a restart, but create_binding currently hardcodes restart_required: false and only logs hot-start failures later in the handler. Clients can be told the binding is active when the adapter still needs intervention.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/bindings.rs` around lines 101 - 106, The
CreateBindingResponse.restart_required field is always false but should reflect
whether platform credentials were added/changed or a hot-start of the adapter
failed; update the create_binding handler to compute a boolean (e.g.,
credential_changed || !hot_start_success) and set
CreateBindingResponse.restart_required to that value instead of hardcoding
false, ensuring any code paths that add/update credentials (the logic that
currently logs hot-start failures) return or expose that
credential_changed/hot_start_success status so create_binding can use it when
constructing the response.
Comment on lines
+671
to
+683
| #[utoipa::path( | ||
| post, | ||
| path = "/agents/projects/{id}/repos", | ||
| params( | ||
| ("id" = String, Path, description = "Project ID"), | ||
| ), | ||
| request_body = CreateRepoRequest, | ||
| responses( | ||
| (status = 200, body = RepoResponse), | ||
| (status = 404, description = "Agent or project not found"), | ||
| ), | ||
| tag = "projects", | ||
| )] |
Contributor
There was a problem hiding this comment.
Document 400 Bad Request for invalid path/worktree input.
create_repo can return BAD_REQUEST from sanitize_relative_path (Line 703), and create_worktree can return BAD_REQUEST from sanitize_segment (Line 829). These are currently undocumented in the OpenAPI responses.
Suggested OpenAPI fix
#[utoipa::path(
post,
path = "/agents/projects/{id}/repos",
@@
responses(
(status = 200, body = RepoResponse),
(status = 404, description = "Agent or project not found"),
+ (status = 400, description = "Invalid repository path"),
),
tag = "projects",
)] #[utoipa::path(
post,
path = "/agents/projects/{id}/worktrees",
@@
responses(
(status = 200, body = WorktreeResponse),
(status = 404, description = "Agent, project, or repo not found"),
+ (status = 400, description = "Invalid worktree name"),
),
tag = "projects",
)]Also applies to: 776-788
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/projects.rs` around lines 671 - 683, The OpenAPI annotations for the
POST endpoints handled by create_repo and create_worktree are missing a 400 Bad
Request response for invalid path/worktree input; update the utoipa::path
attributes for the endpoints (the ones annotating create_repo and
create_worktree) to add a responses entry like (status = 400, description =
"Invalid path/worktree: sanitize_relative_path or sanitize_segment failed") so
that invalid inputs flagged by sanitize_relative_path and sanitize_segment are
documented; locate the annotations surrounding the create_repo and
create_worktree functions and add the 400 response to their responses(...)
lists.
Comment on lines
+41
to
+49
| #[utoipa::path( | ||
| get, | ||
| path = "/secrets/status", | ||
| responses( | ||
| (status = 200, body = serde_json::Value), | ||
| (status = 503, description = "Secrets store not initialized"), | ||
| ), | ||
| tag = "secrets", | ||
| )] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python - <<'PY'
import re
from pathlib import Path
p = Path("src/api/secrets.rs")
text = p.read_text()
pattern = re.compile(
r'(?s)#\[utoipa::path\((?P<anno>.*?)\)\]\s*pub async fn (?P<name>\w+)\b(?P<body>.*?)(?=\n#\[utoipa::path\(|\nfn try_migrate_field|\Z)'
)
missing = []
for m in pattern.finditer(text):
name = m.group("name")
anno = m.group("anno")
body = m.group("body")
returns_500 = "StatusCode::INTERNAL_SERVER_ERROR" in body
has_500_doc = re.search(r'status\s*=\s*500\b', anno) is not None
if returns_500 and not has_500_doc:
missing.append(name)
if missing:
print("Handlers returning 500 but missing 500 in utoipa responses:")
for n in missing:
print(f" - {n}")
else:
print("No 500 response documentation gaps found.")
PYRepository: spacedriveapp/spacebot
Length of output: 263
Add missing status = 500 responses to OpenAPI annotations.
Multiple handlers return StatusCode::INTERNAL_SERVER_ERROR but their #[utoipa::path(...responses(...))] blocks omit status = 500. This creates inaccurate API contracts for generated TypeScript clients.
Pattern to apply across affected handlers
#[utoipa::path(
get,
path = "/secrets/status",
responses(
(status = 200, body = serde_json::Value),
+ (status = 500, description = "Internal server error"),
(status = 503, description = "Secrets store not initialized"),
),
tag = "secrets",
)]Affected handlers: secrets_status (lines 41–49), list_secrets (83–91), put_secret (136–149), delete_secret (203–215), unlock_secrets (348–359), migrate_secrets (509–518), export_secrets (602–611), import_secrets (660–670).
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| #[utoipa::path( | |
| get, | |
| path = "/secrets/status", | |
| responses( | |
| (status = 200, body = serde_json::Value), | |
| (status = 503, description = "Secrets store not initialized"), | |
| ), | |
| tag = "secrets", | |
| )] | |
| #[utoipa::path( | |
| get, | |
| path = "/secrets/status", | |
| responses( | |
| (status = 200, body = serde_json::Value), | |
| (status = 500, description = "Internal server error"), | |
| (status = 503, description = "Secrets store not initialized"), | |
| ), | |
| tag = "secrets", | |
| )] |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/secrets.rs` around lines 41 - 49, Add a 500 response to the
utoipa::path responses for each handler that can return
StatusCode::INTERNAL_SERVER_ERROR: update the annotations for secrets_status,
list_secrets, put_secret, delete_secret, unlock_secrets, migrate_secrets,
export_secrets, and import_secrets to include (status = 500, body =
serde_json::Value) (or a suitable description) so the OpenAPI contract
accurately reflects internal server errors returned by those handlers.
Comment on lines
+49
to
+59
| #[utoipa::path( | ||
| put, | ||
| path = "/ssh/authorized-key", | ||
| request_body = AuthorizedKeyRequest, | ||
| responses( | ||
| (status = 200, body = AuthorizedKeyResponse), | ||
| (status = 400, description = "Invalid request"), | ||
| (status = 500, description = "Internal server error"), | ||
| ), | ||
| tag = "ssh", | ||
| )] |
Contributor
There was a problem hiding this comment.
set_authorized_key response contract is mismatched for invalid input.
Line 55 documents HTTP 400, but invalid keys at Line 65 currently return HTTP 200 (Ok(Json(...))). Please make status behavior and OpenAPI responses consistent (either return BAD_REQUEST for invalid input or remove the 400 response entry).
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/ssh.rs` around lines 49 - 59, The OpenAPI annotation declares a 400
response but the handler set_authorized_key currently returns HTTP 200 for
invalid keys; update the handler so that when input validation fails it returns
an HTTP 400 (Bad Request) response instead of Ok(Json(...)) to match the
#[utoipa::path(..., responses(... (status = 400 ...)))] contract — modify
set_authorized_key's invalid-key branch (and any helper like
validate_authorized_key if present) to produce a 400 response using your
framework's error/Result type (e.g., return Err((StatusCode::BAD_REQUEST,
Json(...))) or otherwise map to a BadRequest response) and ensure the response
body shape still matches AuthorizedKeyResponse or adjust the utoipa entry if you
choose to remove the 400 case instead.
d9ff5ec to
4368fca
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/api/ingest.rs (2)
197-207:⚠️ Potential issue | 🟠 MajorHandle ingestion metadata write failures; don't silently discard the SQL Result.
Lines 197-207 ignore the Result from
sqlx::query()withlet _ =. Failed metadata inserts will be silently ignored while the API reports success, leaving uploaded files and ingestion DB state inconsistent. Store the result and either log the error, propagate it, or return it to the caller.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ingest.rs` around lines 197 - 207, The INSERT OR IGNORE into ingestion_files currently discards the sqlx Result with `let _ = ...execute(pool).await`; instead capture the result (e.g., `let res = ...execute(pool).await`) and handle errors — at minimum propagate or return the error to the caller or log it (using your request/handler error path) so failed metadata writes don't report success; update the surrounding function to return a Result/Error if needed and use the captured variables (`hash`, `safe_name`, `file_size`, `pool`) to build the logged or returned error context.
145-145:⚠️ Potential issue | 🟠 MajorAddress error handling in multipart loop and database insert.
Line 145 uses
while let Ok(Some(field))which exits the loop silently onErrfromnext_field(), allowing the function to return200 OKeven when multipart iteration fails—potentially masking partial uploads. Return an error on iteration failure or add error handling/logging.Lines 197–207 explicitly discard the database insert result with
let _ = sqlx::query(...). Per repo guidelines, handle, log, or propagate errors instead of silently discarding them.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ingest.rs` at line 145, The multipart loop currently uses `while let Ok(Some(field)) = multipart.next_field().await` which silently stops on `Err` — change this to explicitly match the `multipart.next_field().await` result inside the loop (e.g., `match multipart.next_field().await`) and on `Err` log the error and return or propagate an appropriate failure response instead of continuing silently; likewise, replace the discarded `let _ = sqlx::query(...).execute(...).await` with proper error handling: await the query, check the `Result`, and on `Err` log the error and return or propagate it (or convert to your handler's error type) so database insert failures are not ignored (refer to the `multipart.next_field()` call and the `sqlx::query(...).execute(...)` invocation to locate the spots).src/api/secrets.rs (1)
286-291:⚠️ Potential issue | 🟡 MinorError type is discarded; all failures appear as 404.
The wildcard
Err(_)converts any store error (including internal failures) into a 404 response. Ifget_metadatafails due to corruption, I/O errors, or decryption issues, the client sees "not found" instead of an actionable error.Proposed fix to distinguish not-found from internal errors
match store.get_metadata(&name) { Ok(meta) => Json(SecretInfoResponse { name, category: meta.category, created_at: meta.created_at, updated_at: meta.updated_at, }) .into_response(), - Err(_) => ( - StatusCode::NOT_FOUND, - Json(serde_json::json!({"error": format!("secret '{name}' not found")})), - ) - .into_response(), + Err(error) => { + let error_str = error.to_string(); + if error_str.contains("not found") || error_str.contains("NotFound") { + ( + StatusCode::NOT_FOUND, + Json(serde_json::json!({"error": format!("secret '{name}' not found")})), + ) + .into_response() + } else { + ( + StatusCode::INTERNAL_SERVER_ERROR, + Json(serde_json::json!({"error": error_str})), + ) + .into_response() + } + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/secrets.rs` around lines 286 - 291, The current catch-all Err(_) in the get_metadata handling masks all store errors as 404; change the match to inspect the error from get_metadata (e.g., match get_metadata(...)) and handle the store's NotFound variant by returning StatusCode::NOT_FOUND with the existing message for secret '{name}', while mapping all other variants to StatusCode::INTERNAL_SERVER_ERROR (optionally logging the full error) and returning a generic/internal error JSON to the client; specifically update the match arm that currently uses Err(_) to pattern-match the store error enum (or use if matches!(err, StoreError::NotFound)) so only NotFound yields 404 and other errors yield 500.src/api/cortex.rs (1)
29-36:⚠️ Potential issue | 🟠 MajorClamp
limitandoffsetbefore they reach storage.The handlers only cap with
.min(200), so negative values still pass straight through. A request likelimit=-1bypasses the advertised cap, and negative offsets are left to downstream SQL semantics. Add lower bounds here and mirror them in the schema.Also applies to: 50-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/cortex.rs` around lines 29 - 36, CortexChatMessagesQuery currently allows negative values for limit (and the similar pagination struct around lines 50-59 allows negative offsets) so negative inputs bypass the handler caps; update CortexChatMessagesQuery (and the corresponding pagination struct) to enforce non-negative lower bounds by clamping/validating the deserialized fields (e.g., make limit and offset unsigned or add a custom deserializer/validator that enforces min 0 and then apply the existing .min(200) cap), update default_cortex_chat_limit if needed, and adjust the schema annotations (utoipa/ToSchema) to reflect the non-negative range so storage never receives negative limit/offset values.
♻️ Duplicate comments (7)
interface/src/routes/AgentDetail.tsx (1)
212-218:⚠️ Potential issue | 🟠 MajorRemove the unsafe
as CortexEvent[]cast at Line 215.This reintroduces type suppression in the typed-client migration path. Please align shapes instead of asserting. A safe local fix is to map events into the expected
CortexEventshape before passing toCortexEventsSection.💡 Proposed fix
{overviewData && overviewData.recent_cortex_events.length > 0 && ( <CortexEventsSection agentId={agentId} - events={overviewData.recent_cortex_events as CortexEvent[]} + events={overviewData.recent_cortex_events.map((event) => ({ + ...event, + agent_id: agentId, + }))} lastBulletinAt={overviewData.last_bulletin_at ?? null} /> )}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/routes/AgentDetail.tsx` around lines 212 - 218, The code is using an unsafe type assertion "as CortexEvent[]" when passing overviewData.recent_cortex_events into CortexEventsSection; replace the cast by mapping overviewData.recent_cortex_events into the exact CortexEvent shape before passing it in. In the render branch around overviewData and recent_cortex_events, create a local mappedEvents = (overviewData.recent_cortex_events || []).map(ev => ({ /* ensure properties match CortexEvent: id, type, timestamp, details, etc. */ })) and pass mappedEvents to CortexEventsSection along with agentId and last_bulletin_at, ensuring any missing fields are filled or defaulted to the correct types rather than asserting.src/api/bindings.rs (2)
693-696:⚠️ Potential issue | 🟠 Major
restart_requiredremains incorrect in the response.Line [695] is still hardcoded to
false, so credential changes / hot-start failures are not reflected to clients.Suggested fix
- let mut new_discord_token: Option<String> = None; + let mut restart_required = false; + let mut new_discord_token: Option<String> = None; @@ discord["enabled"] = toml_edit::value(true); discord["token"] = toml_edit::value(token.as_str()); new_discord_token = Some(token.clone()); + restart_required = true; @@ if let Err(error) = manager.register_and_start(adapter).await { tracing::error!(%error, "failed to hot-start discord adapter"); + restart_required = true; } @@ - restart_required: false, + restart_required,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 693 - 696, The response is hardcoding restart_required to false; change the CreateBindingResponse construction to use the actual restart-required boolean computed earlier (e.g., the local restart_required variable or the value from the binding creation result) instead of false so clients receive accurate restart_required status; locate the CreateBindingResponse instantiation in bindings.rs and replace the literal false with the appropriate symbol (restart_required or result.restart_required) that reflects the real hot-start/restart requirement.
701-711:⚠️ Potential issue | 🟠 MajorOpenAPI status codes still don’t match runtime behavior.
These specs document
404, but not-found paths return200with{ success: false }(Line [801] and Line [1028]). This keeps generated clients out of sync.Suggested fix (spec-aligned with current handler behavior)
#[utoipa::path( @@ responses( (status = 200, body = UpdateBindingResponse), - (status = 404, description = "Binding not found or config not found"), + (status = 200, description = "No matching binding found (success=false)", body = UpdateBindingResponse), (status = 500, description = "Internal server error"), ), @@ #[utoipa::path( @@ responses( (status = 200, body = DeleteBindingResponse), - (status = 404, description = "Binding not found or config not found"), + (status = 200, description = "No matching binding found (success=false)", body = DeleteBindingResponse), (status = 500, description = "Internal server error"), ),#!/bin/bash # Verify mismatch between declared 404 responses and 200 not-found handler branches. rg -n -C2 'status = 404|No matching binding found|success: false' src/api/bindings.rsAlso applies to: 928-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 701 - 711, The OpenAPI annotations for the PUT "/bindings" utoipa::path (and the similar blocks around the other binding endpoints) declare a 404 but the runtime handlers return 200 with a payload indicating failure ({ success: false }); update the utoipa::path responses for the PUT "/bindings" entry (and the other binding endpoint annotations mentioned) to remove the 404 response and instead document the 200 response to include both success and not-found cases (reference the UpdateBindingRequest and UpdateBindingResponse types used in the attribute) and add a description noting that not-found/config-missing cases return 200 with success: false so generated clients match runtime behavior.interface/src/api/client.ts (2)
1925-1928:⚠️ Potential issue | 🔴 CriticalUse the registry-content response type here.
/skills/registry/contentdoes not return the richerSkillContentResponse. Typing it that way lets callers assume fields likename,description,base_dir, andfile_pathexist when the server only returns the registry payload shape.Suggested fix
- fetchJson<SkillContentResponse>( + fetchJson<Types.RegistrySkillContentResponse>(🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 1925 - 1928, The registrySkillContent client method is incorrectly typed as returning SkillContentResponse; update its generic on fetchJson in registrySkillContent to use the proper registry-content response type (the server's registry payload type) instead of SkillContentResponse so callers don't assume fields like name/description/base_dir/file_path exist; locate the registrySkillContent function and replace SkillContentResponse with the correct registry response interface (and update any imports/types where necessary).
195-200:⚠️ Potential issue | 🟡 MinorThis doesn't model either emitted agent-link event yet.
The SSE backend emits
agent_message_sentandagent_message_received, but this interface has no literaltypeand never participates inApiEvent. Consumers still can't discriminate or even see these events in the exported union.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 195 - 200, The AgentMessageEvent is missing a discriminant and thus can't be used in the ApiEvent union; update the AgentMessageEvent interface to include a literal type field (e.g. type: 'agent_message_sent' | 'agent_message_received') and ensure any existing ApiEvent union (or exported event union) includes AgentMessageEvent so consumers can discriminate on event.type and see these events in the exported union; adjust any downstream uses of AgentMessageEvent or ApiEvent to handle the new type field.src/api/cortex.rs (1)
75-89:⚠️ Potential issue | 🟠 MajorKeep the documented Cortex chat URLs aligned with the shipped client.
These annotations publish
/cortex/chat/*, whileinterface/src/api/client.tsstill calls/cortex-chat/messages,/cortex-chat/send,/cortex-chat/threads, andDELETE /cortex-chat/thread. The generated OpenAPI client will otherwise target a different route table than the app uses today.Also applies to: 130-141, 211-223, 241-251
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/cortex.rs` around lines 75 - 89, The OpenAPI path annotations in the utoipa attributes use "/cortex/chat/..." but the shipped client expects "/cortex-chat/..."; update the path strings in the utoipa::path attributes for all Cortex chat endpoints (the annotations shown and the other occurrences around the given diff ranges) to use "/cortex-chat/..." so the generated client matches the runtime routes (i.e., change "/cortex/chat/messages" -> "/cortex-chat/messages" and similarly for send, threads, and thread DELETE annotations).src/api/agents.rs (1)
1101-1112:⚠️ Potential issue | 🟠 MajorThese 4xx responses are still spec-only.
update_agent,delete_agent, andupload_avatarstill encode empty IDs, missing agents, and unsupported image types as200 OKJSON payloads (success: false). Until the handlers return transport-level 4xx statuses, this contract will keep disagreeing with the actual API.Also applies to: 1238-1250, 2053-2067
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 1101 - 1112, The handlers update_agent, delete_agent, and upload_avatar currently return 200 OK with a JSON { success:false } for client errors, causing the OpenAPI annotations to disagree with runtime behavior; change each handler to return proper transport-level responses (e.g., HttpResponse::BadRequest for invalid input/empty IDs, HttpResponse::NotFound when an agent is missing, HttpResponse::UnsupportedMediaType for unsupported image types) instead of 200, and update their Result/Response types accordingly so utoipa annotations (the #[utoipa::path(...)] blocks) match actual status codes — ensure the logic inside update_agent, delete_agent, and upload_avatar returns the matching HttpResponse variants and error handling paths propagate those status codes.
🧹 Nitpick comments (2)
src/api/secrets.rs (1)
256-268: OpenAPI annotation should include 500 if internal errors are possible.While the current implementation maps all errors to 404, if you apply the fix to distinguish error types, the annotation should also document the 500 response. This is a heads-up for consistency once the error handling is updated.
Future-proof annotation
#[utoipa::path( get, path = "/secrets/{name}/info", params( ("name" = String, Path, description = "Secret name"), ), responses( (status = 200, body = SecretInfoResponse), (status = 404, description = "Secret not found"), + (status = 500, description = "Internal server error"), (status = 503, description = "Secrets store not initialized"), ), tag = "secrets", )]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/secrets.rs` around lines 256 - 268, Update the OpenAPI annotation on the utoipa::path attribute for the "/secrets/{name}/info" endpoint to include a 500 response entry for internal server errors; modify the responses list (currently containing 200, 404, 503) to add (status = 500, description = "Internal server error") so the documentation matches the planned error handling changes for the handler that serves secret info (the attribute block above the secret info handler).src/api/skills.rs (1)
131-137: Use theIntoParamsquery types instead of re-declaring these params.
RegistryBrowseQueryandRegistrySearchQueryalready deriveIntoParams, and utoipa documents thatIntoParamsreflectsserde(default)plus the same required/optional rules asToSchema. Repeatingview,page, andlimitas bare tuple params here drops that metadata and makes the generated OpenAPI easier to drift away from the handler defaults. Preferparams(RegistryBrowseQuery)/params(RegistrySearchQuery)so typegen stays aligned. (docs.rs)Also applies to: 143-148, 470-476, 536-542
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 131 - 137, The OpenAPI annotations re-declare individual query params (view, page, limit) instead of using the existing IntoParams types, which loses serde(default) and other metadata; update the endpoint attribute usage to reference the derived param types (e.g., use params(RegistryBrowseQuery) and params(RegistrySearchQuery)) rather than listing bare tuple/query params so the generated docs stay in sync with RegistryBrowseQuery and RegistrySearchQuery and preserve defaults and optional/required rules.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/api/client.ts`:
- Around line 1339-1340: The overview helper currently calls the wrong path;
update the overview field so that overview: () =>
fetchJson<Types.InstanceOverviewResponse>(...) uses the backend path
"/agents/instance" instead of "/overview" — locate the overview function in
interface/src/api/client.ts and modify the fetchJson call accordingly while
keeping Types.InstanceOverviewResponse unchanged.
In `@src/api/agents.rs`:
- Around line 400-409: The OpenAPI annotation for the POST
/agents/warmup/trigger (the utoipa::path on the trigger_warmup endpoint) is
missing a 500 response; update the attribute to include (status = 500,
description = "Internal Server Error") so generated clients know server failures
are possible — this covers cases where resolve_warmup_agent_ids() can return
INTERNAL_SERVER_ERROR when runtime state (memory search, mcp manager, or sqlite
pool) is half-initialized; locate the utoipa::path on trigger_warmup and add the
500 response entry alongside the existing 200/404/503 entries.
In `@src/api/messaging.rs`:
- Around line 921-930: The OpenAPI annotation for the POST /messaging/disconnect
currently uses a generic serde_json::Value as the 200 response which weakens
generated typed clients; update the #[utoipa::path(... responses(...))]
annotation for disconnect_platform (and similarly for toggle_platform) to
reference a concrete response schema type (e.g., the specific response struct
returned by disconnect_platform/toggle_platform or a shared SuccessResponse
type) instead of serde_json::Value, and if that response struct does not yet
implement or appear in the docs, add/derive the appropriate utoipa::ToSchema on
the response struct so the generated TypeScript types are fully typed.
In `@src/api/skills.rs`:
- Around line 364-377: The utoipa::path macro for the POST /agents/skills/upload
is missing a request_body describing the multipart/form-data payload; define a
form struct (e.g., UploadSkillForm) that derives ToSchema and annotate the file
field with #[schema(format = Binary, value_type = String)] and any other fields,
then update the #[utoipa::path(...)] on the handler that accepts
axum::extract::Multipart to include request_body(content = UploadSkillForm,
content_type = "multipart/form-data") so the OpenAPI schema documents the
multipart upload alongside the existing responses (UploadSkillResponse).
In `@src/api/system.rs`:
- Around line 174-182: The OpenAPI path attributes for storage_status,
backup_export, and backup_restore currently document 200 and 503 but omit the
500 responses that those handlers return on task/join/IO failures; update the
utoipa::path responses for the functions storage_status, backup_export, and
backup_restore to include a (status = 500, description = "Internal Server
Error") response (or a defined error body type if you have one) so the generated
client models INTERNAL_SERVER_ERROR as a possible outcome.
In `@src/api/workers.rs`:
- Around line 93-101: The OpenAPI params for the /workers endpoint in the
#[utoipa::path] macro incorrectly declare limit and offset as required i64
tuples; fix it by removing the manual tuple param declarations and let the
existing WorkerListQuery (which derives IntoParams and uses serde defaults) be
the single source of truth—replace the explicit params list with a params entry
that references WorkerListQuery (or omit manual params so utoipa/IntoParams
generates them), ensuring the generated spec correctly marks limit and offset as
optional.
---
Outside diff comments:
In `@src/api/cortex.rs`:
- Around line 29-36: CortexChatMessagesQuery currently allows negative values
for limit (and the similar pagination struct around lines 50-59 allows negative
offsets) so negative inputs bypass the handler caps; update
CortexChatMessagesQuery (and the corresponding pagination struct) to enforce
non-negative lower bounds by clamping/validating the deserialized fields (e.g.,
make limit and offset unsigned or add a custom deserializer/validator that
enforces min 0 and then apply the existing .min(200) cap), update
default_cortex_chat_limit if needed, and adjust the schema annotations
(utoipa/ToSchema) to reflect the non-negative range so storage never receives
negative limit/offset values.
In `@src/api/ingest.rs`:
- Around line 197-207: The INSERT OR IGNORE into ingestion_files currently
discards the sqlx Result with `let _ = ...execute(pool).await`; instead capture
the result (e.g., `let res = ...execute(pool).await`) and handle errors — at
minimum propagate or return the error to the caller or log it (using your
request/handler error path) so failed metadata writes don't report success;
update the surrounding function to return a Result/Error if needed and use the
captured variables (`hash`, `safe_name`, `file_size`, `pool`) to build the
logged or returned error context.
- Line 145: The multipart loop currently uses `while let Ok(Some(field)) =
multipart.next_field().await` which silently stops on `Err` — change this to
explicitly match the `multipart.next_field().await` result inside the loop
(e.g., `match multipart.next_field().await`) and on `Err` log the error and
return or propagate an appropriate failure response instead of continuing
silently; likewise, replace the discarded `let _ =
sqlx::query(...).execute(...).await` with proper error handling: await the
query, check the `Result`, and on `Err` log the error and return or propagate it
(or convert to your handler's error type) so database insert failures are not
ignored (refer to the `multipart.next_field()` call and the
`sqlx::query(...).execute(...)` invocation to locate the spots).
In `@src/api/secrets.rs`:
- Around line 286-291: The current catch-all Err(_) in the get_metadata handling
masks all store errors as 404; change the match to inspect the error from
get_metadata (e.g., match get_metadata(...)) and handle the store's NotFound
variant by returning StatusCode::NOT_FOUND with the existing message for secret
'{name}', while mapping all other variants to StatusCode::INTERNAL_SERVER_ERROR
(optionally logging the full error) and returning a generic/internal error JSON
to the client; specifically update the match arm that currently uses Err(_) to
pattern-match the store error enum (or use if matches!(err,
StoreError::NotFound)) so only NotFound yields 404 and other errors yield 500.
---
Duplicate comments:
In `@interface/src/api/client.ts`:
- Around line 1925-1928: The registrySkillContent client method is incorrectly
typed as returning SkillContentResponse; update its generic on fetchJson in
registrySkillContent to use the proper registry-content response type (the
server's registry payload type) instead of SkillContentResponse so callers don't
assume fields like name/description/base_dir/file_path exist; locate the
registrySkillContent function and replace SkillContentResponse with the correct
registry response interface (and update any imports/types where necessary).
- Around line 195-200: The AgentMessageEvent is missing a discriminant and thus
can't be used in the ApiEvent union; update the AgentMessageEvent interface to
include a literal type field (e.g. type: 'agent_message_sent' |
'agent_message_received') and ensure any existing ApiEvent union (or exported
event union) includes AgentMessageEvent so consumers can discriminate on
event.type and see these events in the exported union; adjust any downstream
uses of AgentMessageEvent or ApiEvent to handle the new type field.
In `@interface/src/routes/AgentDetail.tsx`:
- Around line 212-218: The code is using an unsafe type assertion "as
CortexEvent[]" when passing overviewData.recent_cortex_events into
CortexEventsSection; replace the cast by mapping
overviewData.recent_cortex_events into the exact CortexEvent shape before
passing it in. In the render branch around overviewData and
recent_cortex_events, create a local mappedEvents =
(overviewData.recent_cortex_events || []).map(ev => ({ /* ensure properties
match CortexEvent: id, type, timestamp, details, etc. */ })) and pass
mappedEvents to CortexEventsSection along with agentId and last_bulletin_at,
ensuring any missing fields are filled or defaulted to the correct types rather
than asserting.
In `@src/api/agents.rs`:
- Around line 1101-1112: The handlers update_agent, delete_agent, and
upload_avatar currently return 200 OK with a JSON { success:false } for client
errors, causing the OpenAPI annotations to disagree with runtime behavior;
change each handler to return proper transport-level responses (e.g.,
HttpResponse::BadRequest for invalid input/empty IDs, HttpResponse::NotFound
when an agent is missing, HttpResponse::UnsupportedMediaType for unsupported
image types) instead of 200, and update their Result/Response types accordingly
so utoipa annotations (the #[utoipa::path(...)] blocks) match actual status
codes — ensure the logic inside update_agent, delete_agent, and upload_avatar
returns the matching HttpResponse variants and error handling paths propagate
those status codes.
In `@src/api/bindings.rs`:
- Around line 693-696: The response is hardcoding restart_required to false;
change the CreateBindingResponse construction to use the actual restart-required
boolean computed earlier (e.g., the local restart_required variable or the value
from the binding creation result) instead of false so clients receive accurate
restart_required status; locate the CreateBindingResponse instantiation in
bindings.rs and replace the literal false with the appropriate symbol
(restart_required or result.restart_required) that reflects the real
hot-start/restart requirement.
- Around line 701-711: The OpenAPI annotations for the PUT "/bindings"
utoipa::path (and the similar blocks around the other binding endpoints) declare
a 404 but the runtime handlers return 200 with a payload indicating failure ({
success: false }); update the utoipa::path responses for the PUT "/bindings"
entry (and the other binding endpoint annotations mentioned) to remove the 404
response and instead document the 200 response to include both success and
not-found cases (reference the UpdateBindingRequest and UpdateBindingResponse
types used in the attribute) and add a description noting that
not-found/config-missing cases return 200 with success: false so generated
clients match runtime behavior.
In `@src/api/cortex.rs`:
- Around line 75-89: The OpenAPI path annotations in the utoipa attributes use
"/cortex/chat/..." but the shipped client expects "/cortex-chat/..."; update the
path strings in the utoipa::path attributes for all Cortex chat endpoints (the
annotations shown and the other occurrences around the given diff ranges) to use
"/cortex-chat/..." so the generated client matches the runtime routes (i.e.,
change "/cortex/chat/messages" -> "/cortex-chat/messages" and similarly for
send, threads, and thread DELETE annotations).
---
Nitpick comments:
In `@src/api/secrets.rs`:
- Around line 256-268: Update the OpenAPI annotation on the utoipa::path
attribute for the "/secrets/{name}/info" endpoint to include a 500 response
entry for internal server errors; modify the responses list (currently
containing 200, 404, 503) to add (status = 500, description = "Internal server
error") so the documentation matches the planned error handling changes for the
handler that serves secret info (the attribute block above the secret info
handler).
In `@src/api/skills.rs`:
- Around line 131-137: The OpenAPI annotations re-declare individual query
params (view, page, limit) instead of using the existing IntoParams types, which
loses serde(default) and other metadata; update the endpoint attribute usage to
reference the derived param types (e.g., use params(RegistryBrowseQuery) and
params(RegistrySearchQuery)) rather than listing bare tuple/query params so the
generated docs stay in sync with RegistryBrowseQuery and RegistrySearchQuery and
preserve defaults and optional/required rules.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: e7bfcf1a-1dc0-4589-a68d-5f366644cd49
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomlinterface/bun.lockis excluded by!**/*.lock,!**/*.lockinterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (62)
interface/src/api/client-typed.tsinterface/src/api/client.tsinterface/src/api/schema.d.tsinterface/src/api/types.tsinterface/src/components/ProfileAvatar.tsxinterface/src/components/Sidebar.tsxinterface/src/components/TaskBoard.tsxinterface/src/components/TopologyGraph.tsxinterface/src/hooks/useAudioRecorder.tsinterface/src/hooks/useTtsPlayback.tsinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentWorkers.tsxinterface/src/routes/ChannelDetail.tsxinterface/src/routes/GlobalTasks.tsxinterface/src/routes/Orchestrate.tsxjustfilesrc/agent/cortex.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/bindings.rssrc/api/channels.rssrc/api/config.rssrc/api/cortex.rssrc/api/cron.rssrc/api/factory.rssrc/api/ingest.rssrc/api/links.rssrc/api/mcp.rssrc/api/memories.rssrc/api/messaging.rssrc/api/models.rssrc/api/projects.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/skills.rssrc/api/ssh.rssrc/api/state.rssrc/api/system.rssrc/api/tasks.rssrc/api/tools.rssrc/api/webchat.rssrc/api/workers.rssrc/bin/openapi_spec.rssrc/config/types.rssrc/conversation/history.rssrc/conversation/worker_transcript.rssrc/cron/store.rssrc/factory/presets.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/memory/store.rssrc/memory/types.rssrc/opencode/types.rssrc/projects/store.rssrc/secrets/store.rssrc/tasks/store.rssrc/tools/memory_save.rssrc/update.rs
✅ Files skipped from review due to trivial changes (27)
- src/cron/store.rs
- interface/src/routes/GlobalTasks.tsx
- interface/src/components/ProfileAvatar.tsx
- src/projects/store.rs
- src/api/tools.rs
- src/tasks/store.rs
- interface/src/hooks/useAudioRecorder.ts
- src/opencode/types.rs
- src/api/links.rs
- src/factory/presets.rs
- src/mcp.rs
- src/update.rs
- src/api/webchat.rs
- interface/src/components/TopologyGraph.tsx
- src/conversation/history.rs
- src/agent/cortex_chat.rs
- interface/src/hooks/useTtsPlayback.ts
- src/main.rs
- src/conversation/worker_transcript.rs
- src/agent/cortex.rs
- src/api/mcp.rs
- src/api/models.rs
- src/api/memories.rs
- src/api/projects.rs
- src/api/channels.rs
- src/api/providers.rs
- interface/src/api/types.ts
🚧 Files skipped from review as they are similar to previous changes (19)
- src/tools/memory_save.rs
- src/lib.rs
- interface/src/routes/ChannelDetail.tsx
- interface/src/components/Sidebar.tsx
- src/api/factory.rs
- interface/src/components/TaskBoard.tsx
- src/api.rs
- src/bin/openapi_spec.rs
- interface/src/routes/Orchestrate.tsx
- src/api/server.rs
- interface/src/api/client-typed.ts
- justfile
- src/api/settings.rs
- src/secrets/store.rs
- src/api/ssh.rs
- src/config/types.rs
- src/api/tasks.rs
- src/api/cron.rs
- src/api/config.rs
Comment on lines
+400
to
+409
| #[utoipa::path( | ||
| post, | ||
| path = "/agents/warmup/trigger", | ||
| request_body = WarmupTriggerRequest, | ||
| responses( | ||
| (status = 200, body = WarmupTriggerResponse), | ||
| (status = 503, description = "LLM manager not available"), | ||
| (status = 404, description = "Agent not found"), | ||
| ), | ||
| tag = "agents", |
Contributor
There was a problem hiding this comment.
Document the 500 path from trigger_warmup.
resolve_warmup_agent_ids() can still return INTERNAL_SERVER_ERROR for a single-agent trigger when runtime state is half-initialized (missing memory search, MCP manager, or SQLite pool). This annotation only advertises 200/404/503, so the generated client will treat a real server failure as impossible.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/agents.rs` around lines 400 - 409, The OpenAPI annotation for the
POST /agents/warmup/trigger (the utoipa::path on the trigger_warmup endpoint) is
missing a 500 response; update the attribute to include (status = 500,
description = "Internal Server Error") so generated clients know server failures
are possible — this covers cases where resolve_warmup_agent_ids() can return
INTERNAL_SERVER_ERROR when runtime state (memory search, mcp manager, or sqlite
pool) is half-initialized; locate the utoipa::path on trigger_warmup and add the
500 response entry alongside the existing 200/404/503 entries.
Comment on lines
+921
to
+930
| #[utoipa::path( | ||
| post, | ||
| path = "/messaging/disconnect", | ||
| request_body = DisconnectPlatformRequest, | ||
| responses( | ||
| (status = 200, body = serde_json::Value), | ||
| (status = 500, description = "Internal server error"), | ||
| ), | ||
| tag = "messaging", | ||
| )] |
Contributor
There was a problem hiding this comment.
Use concrete response schemas instead of serde_json::Value for typed-client parity.
disconnect_platform and toggle_platform document 200 as serde_json::Value, which weakens generated TypeScript typing for these endpoints versus the rest of this migration.
Suggested change
#[utoipa::path(
post,
path = "/messaging/disconnect",
request_body = DisconnectPlatformRequest,
responses(
- (status = 200, body = serde_json::Value),
+ (status = 200, body = MessagingInstanceActionResponse),
(status = 500, description = "Internal server error"),
),
tag = "messaging",
)]
pub(super) async fn disconnect_platform(
State(state): State<Arc<ApiState>>,
Json(request): Json<DisconnectPlatformRequest>,
-) -> Result<Json<serde_json::Value>, StatusCode> {
+) -> Result<Json<MessagingInstanceActionResponse>, StatusCode> {
...
- Ok(Json(serde_json::json!({
- "success": true,
- "message": format!("{label} disconnected")
- })))
+ Ok(Json(MessagingInstanceActionResponse {
+ success: true,
+ message: format!("{label} disconnected"),
+ }))
}
#[utoipa::path(
post,
path = "/messaging/toggle",
request_body = TogglePlatformRequest,
responses(
- (status = 200, body = serde_json::Value),
+ (status = 200, body = MessagingInstanceActionResponse),
(status = 500, description = "Internal server error"),
),
tag = "messaging",
)]
pub(super) async fn toggle_platform(
State(state): State<Arc<ApiState>>,
Json(request): Json<TogglePlatformRequest>,
-) -> Result<Json<serde_json::Value>, StatusCode> {
+) -> Result<Json<MessagingInstanceActionResponse>, StatusCode> {
...
- Ok(Json(serde_json::json!({
- "success": true,
- "message": format!("{platform} {action}")
- })))
+ Ok(Json(MessagingInstanceActionResponse {
+ success: true,
+ message: format!("{platform} {action}"),
+ }))
}Also applies to: 1113-1122
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/messaging.rs` around lines 921 - 930, The OpenAPI annotation for the
POST /messaging/disconnect currently uses a generic serde_json::Value as the 200
response which weakens generated typed clients; update the #[utoipa::path(...
responses(...))] annotation for disconnect_platform (and similarly for
toggle_platform) to reference a concrete response schema type (e.g., the
specific response struct returned by disconnect_platform/toggle_platform or a
shared SuccessResponse type) instead of serde_json::Value, and if that response
struct does not yet implement or appear in the docs, add/derive the appropriate
utoipa::ToSchema on the response struct so the generated TypeScript types are
fully typed.
Comment on lines
+364
to
+377
| #[utoipa::path( | ||
| post, | ||
| path = "/agents/skills/upload", | ||
| params( | ||
| ("agent_id" = String, Query, description = "Agent ID"), | ||
| ), | ||
| responses( | ||
| (status = 200, body = UploadSkillResponse), | ||
| (status = 400, description = "Invalid request"), | ||
| (status = 404, description = "Agent not found"), | ||
| (status = 500, description = "Internal server error"), | ||
| ), | ||
| tag = "skills", | ||
| )] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, examine the file around the specified lines
head -n 400 src/api/skills.rs | tail -n 50Repository: spacedriveapp/spacebot
Length of output: 1820
🏁 Script executed:
# Get the full handler function to understand the multipart handling
sed -n '364,430p' src/api/skills.rsRepository: spacedriveapp/spacebot
Length of output: 2367
🏁 Script executed:
# Search for how multipart is handled in the handler
rg -A 20 "upload_skill" src/api/skills.rsRepository: spacedriveapp/spacebot
Length of output: 783
🏁 Script executed:
# Check if there are other multipart examples in the codebase using utoipa
rg -B 5 -A 5 'multipart/form-data' src/ --type rsRepository: spacedriveapp/spacebot
Length of output: 92
🏁 Script executed:
# Check utoipa imports and see if request_body is used elsewhere
rg 'request_body|utoipa::path' src/api/ --type rs -A 3 -B 1 | head -n 100Repository: spacedriveapp/spacebot
Length of output: 92
🏁 Script executed:
# Search for other utoipa request_body usage in the codebase
rg 'request_body' src/ -B 3 -A 3Repository: spacedriveapp/spacebot
Length of output: 22043
🏁 Script executed:
# Check if there are version specifications for utoipa in Cargo.toml
cat Cargo.toml | grep -A 2 'utoipa'Repository: spacedriveapp/spacebot
Length of output: 263
🏁 Script executed:
# Look for any utoipa documentation or comments about multipart in the codebase
rg -i 'utoipa|multipart' src/ --type rs | grep -i 'doc\|comment\|//'Repository: spacedriveapp/spacebot
Length of output: 92
🌐 Web query:
utoipa multipart form-data request body documentation example
💡 Result:
To document a multipart/form-data request body in Utoipa for OpenAPI/Swagger UI (commonly used with Axum or Actix-web), define a struct deriving ToSchema with fields annotated for binary file data using #[schema(format = Binary, value_type = String)] on Vec fields, then reference the struct in the #[utoipa::path] macro's request_body with content_type = "multipart/form-data". Example for Axum: rust use utoipa::ToSchema; #[derive(ToSchema)] struct UploadForm { #[schema(format = Binary, value_type = String)] file: Vec, name: String, // optional text field } #[utoipa::path( post, path = "/upload", request_body( content = UploadForm, content_type = "multipart/form-data" ), responses( (status = 200, description = "File uploaded") ) )] async fn upload(mut multipart: Multipart) -> Result<Json, StatusCode> { // handler implementation using axum::extract::Multipart Ok(Json()) } This generates a Swagger UI with file upload input. For proper Swagger file picker display, the Binary format and value_type=String are crucial. Official examples are in the utoipa repo at examples/axum-multipart (added Dec 2024) and utoipa 5.0 PR #1113. Similar for Actix-web using MultipartForm where T derives ToSchema with the same schema attributes.
Citations:
- 1: Support for MultiPart Form Data Request Data for File Upload juhaku/utoipa#1055
- 2: How to add multipart to swagger? Nested routers error juhaku/utoipa#1230
- 3: Make implementation for actix MultipartForm juhaku/utoipa#740
- 4: https://stackoverflow.com/questions/79187416/how-to-show-multipart-form-data-with-a-file-upload-field-in-swagger-ui-with-rust
- 5: example on file uploads juhaku/utoipa#753
Add explicit multipart form-data documentation to the OpenAPI schema.
The #[utoipa::path] annotation is missing a request_body parameter for the multipart upload. Without it, the generated OpenAPI spec will not document the multipart/form-data payload, even though the handler accepts axum::extract::Multipart.
For utoipa 5.x, this requires defining a struct that derives ToSchema with appropriate field annotations (using #[schema(format = Binary, value_type = String)] for file fields), then referencing it in the annotation: request_body(content = YourFormStruct, content_type = "multipart/form-data").
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/skills.rs` around lines 364 - 377, The utoipa::path macro for the
POST /agents/skills/upload is missing a request_body describing the
multipart/form-data payload; define a form struct (e.g., UploadSkillForm) that
derives ToSchema and annotate the file field with #[schema(format = Binary,
value_type = String)] and any other fields, then update the #[utoipa::path(...)]
on the handler that accepts axum::extract::Multipart to include
request_body(content = UploadSkillForm, content_type = "multipart/form-data") so
the OpenAPI schema documents the multipart upload alongside the existing
responses (UploadSkillResponse).
Comment on lines
+174
to
+182
| #[utoipa::path( | ||
| get, | ||
| path = "/system/storage", | ||
| responses( | ||
| (status = 200, body = StorageStatus), | ||
| (status = 503, description = "No runtime config available"), | ||
| ), | ||
| tag = "system", | ||
| )] |
Contributor
There was a problem hiding this comment.
The new OpenAPI blocks omit 500s these handlers already emit.
storage_status, backup_export, and backup_restore all map task/join/IO failures to INTERNAL_SERVER_ERROR, but those responses are not documented here. The generated client will therefore model legitimate server failures as impossible.
Also applies to: 271-330
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/system.rs` around lines 174 - 182, The OpenAPI path attributes for
storage_status, backup_export, and backup_restore currently document 200 and 503
but omit the 500 responses that those handlers return on task/join/IO failures;
update the utoipa::path responses for the functions storage_status,
backup_export, and backup_restore to include a (status = 500, description =
"Internal Server Error") response (or a defined error body type if you have one)
so the generated client models INTERNAL_SERVER_ERROR as a possible outcome.
Comment on lines
+93
to
+101
| #[utoipa::path( | ||
| get, | ||
| path = "/workers", | ||
| params( | ||
| ("agent_id" = String, Query, description = "Agent ID"), | ||
| ("limit" = i64, Query, description = "Maximum number of results to return"), | ||
| ("offset" = i64, Query, description = "Number of results to skip"), | ||
| ("status" = Option<String>, Query, description = "Filter by worker status"), | ||
| ), |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
spec_file="$(fd -HI 'openapi|swagger|spec' | rg -n '\.json$' | head -n1 | cut -d: -f1 || true)"
if [ -z "${spec_file:-}" ]; then
echo "Could not find an OpenAPI JSON artifact in the repo. Generate it first (e.g., just typegen) and rerun."
exit 1
fi
python - "$spec_file" <<'PY'
import json, sys
path = sys.argv[1]
with open(path, "r", encoding="utf-8") as f:
data = json.load(f)
def show(endpoint):
get_op = data.get("paths", {}).get(endpoint, {}).get("get", {})
params = get_op.get("parameters", [])
print(f"\n{endpoint} parameters:")
for p in params:
name = p.get("name")
required = p.get("required")
default = p.get("schema", {}).get("default", None)
print(f" - {name}: required={required}, default={default}")
show("/workers")
show("/workers/detail")
PYRepository: spacedriveapp/spacebot
Length of output: 168
🏁 Script executed:
cat -n src/api/workers.rs | head -120Repository: spacedriveapp/spacebot
Length of output: 4820
🏁 Script executed:
cat -n src/api/workers.rs | sed -n '180,200p'Repository: spacedriveapp/spacebot
Length of output: 899
OpenAPI query contract mismatch on /workers endpoint
The limit and offset parameters in the #[utoipa::path] macro (lines 98–99) are declared as required (i64 in tuple form), but WorkerListQuery (line 14) marks them as optional via serde defaults (#[serde(default)] and #[serde(default = "default_limit")]). This causes the generated OpenAPI spec to incorrectly require these parameters, forcing generated TypeScript clients to supply them even though the server accepts omitted values.
Use the IntoParams derive (already present on WorkerListQuery) as the single source of truth by removing the manual parameter tuple declarations and letting utoipa generate the spec from the struct definition.
Note: This concern does not apply to /workers/detail (line 184+), where both required parameters (agent_id, worker_id) match the struct definition with no defaults.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/workers.rs` around lines 93 - 101, The OpenAPI params for the
/workers endpoint in the #[utoipa::path] macro incorrectly declare limit and
offset as required i64 tuples; fix it by removing the manual tuple param
declarations and let the existing WorkerListQuery (which derives IntoParams and
uses serde defaults) be the single source of truth—replace the explicit params
list with a params entry that references WorkerListQuery (or omit manual params
so utoipa/IntoParams generates them), ensuring the generated spec correctly
marks limit and offset as optional.
4368fca to
1709e19
Compare
- Add utoipa for OpenAPI spec generation with axum integration - Create openapi-spec binary for spec export - Add Swagger UI served at /api/docs - Generate TypeScript types from OpenAPI spec using openapi-typescript - Migrate all 211 hand-written TypeScript interfaces to schema-generated types - Create types.ts re-export wrapper for clean imports - Add openapi-fetch client for fully typed API calls - Add typegen and check-typegen just recipes for CI - Update all components to use new schema types - Fix type mismatches (string | null | undefined vs old types) - Add voice overlay hooks as stubs for future implementation Breaking changes: None - all types backward compatible via re-exports
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
src/api/webchat.rs (1)
31-77:⚠️ Potential issue | 🟠 Major
webchat_sendannotation missing 500 response status.Line 74–77 maps
inject_messagefailures toStatusCode::INTERNAL_SERVER_ERROR, but the#[utoipa::path]macro only documents 200/400/503. Add(status = 500, description = "Failed to inject message")to the responses to match the actual implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/webchat.rs` around lines 31 - 77, The utoipa OpenAPI annotation on the webchat_send handler is missing the 500 response that the code can return when manager.inject_message(...) fails; update the #[utoipa::path(...)] attribute on the webchat_send function to include responses((status = 500, description = "Failed to inject message")) so the documented responses match the actual behavior where inject_message maps errors to StatusCode::INTERNAL_SERVER_ERROR.src/api/cron.rs (1)
303-341:⚠️ Potential issue | 🟠 MajorAdd
body = CronActionResponseto error response statuses in the OpenAPI schema.The handler returns
Json<CronActionResponse>withsuccessandmessagefields on all error paths (400, 404, 500), but the#[utoipa::path]responses block only specifies a body for status 200. This causes the generated client to lack those fields on error responses. Update the responses metadata to document the error body types.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/cron.rs` around lines 303 - 341, The OpenAPI attribute on create_or_update_cron declares responses but only sets body = CronActionResponse for status 200; update the #[utoipa::path(... responses(...))] on the create_or_update_cron handler to include body = CronActionResponse for the error statuses (400, 404, 500) so that the generated schema reflects the Json<CronActionResponse> error payloads returned by the function; ensure the responses tuple entries for 400, 404, and 500 each include body = CronActionResponse while keeping their descriptions.
♻️ Duplicate comments (7)
src/api/server.rs (1)
32-44:⚠️ Potential issue | 🟠 MajorThe OpenAPI document still doesn't advertise bearer auth.
Lines 275-278 put
protected_routesbehindapi_auth_middleware, butApiDoconly definesinfo(...)andservers(...). Swagger UI and generated clients won't know that a bearer token may be required unless the spec declares the bearer scheme and marks protected operations accordingly. If you make it global, exempt/api/healthexplicitly.Also applies to: 261-278
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/server.rs` around lines 32 - 44, ApiDoc's OpenApi derive doesn't declare a bearer security scheme so clients/Swagger won't know protected_routes require tokens; update the ApiDoc OpenApi configuration (the ApiDoc struct with #[derive(OpenApi)] and #[openapi(...)] ) to add a bearer authentication scheme (e.g., security_schemes with type http scheme="bearer" bearer_format="JWT") and apply that scheme globally (security = (("bearerAuth", []), ...)) or annotate protected endpoints accordingly; ensure the /api/health route is excluded from the global requirement (either omit it from the annotated list or add an explicit no-security attribute) so api_auth_middleware-protected_routes behavior matches the OpenAPI spec.src/api/ssh.rs (1)
49-69:⚠️ Potential issue | 🟠 MajorThe SSH response docs still don't match the handlers.
Line 65 returns
Ok(...)for invalid keys even though Line 55 documents HTTP 400, andssh_statusonly buildsOk(Json(...))despite Lines 109-110 advertising 400/500. Since this spec now feeds generated clients, either emit those statuses at runtime or remove them from the annotations.Also applies to: 104-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ssh.rs` around lines 49 - 69, The OpenAPI annotations for set_authorized_key/ssh endpoints declare 400 and 500 responses but the handler always returns Ok(Json(...)), so update the runtime to match the spec or vice-versa: either (A) change set_authorized_key (and the ssh_status helper) to return Err(StatusCode::BAD_REQUEST) for invalid keys and Err(StatusCode::INTERNAL_SERVER_ERROR) on failures (return Result::Err instead of Ok(Json(...))) so the HTTP status codes declared by the utoipa::path attribute are actually emitted, or (B) remove the 400/500 response entries from the utoipa::path attribute above set_authorized_key so the docs match the current behavior; pick one approach and apply it consistently to set_authorized_key and the ssh_status-related functions.src/api/agents.rs (2)
400-409:⚠️ Potential issue | 🟠 MajorAdd the missing 500 response to
trigger_warmup.
resolve_warmup_agent_ids()can still returnStatusCode::INTERNAL_SERVER_ERRORfor a single-agent trigger when runtime state is only partially initialized, but this annotation only advertises200/404/503. The generated client will treat a real server failure as impossible until the spec includes500.🛠️ Suggested annotation change
responses( (status = 200, body = WarmupTriggerResponse), + (status = 500, description = "Internal server error"), (status = 503, description = "LLM manager not available"), (status = 404, description = "Agent not found"), ),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 400 - 409, Update the utoipa path annotation for the trigger_warmup endpoint to include a 500 response so the OpenAPI spec reflects server errors; specifically, in the attribute block that references WarmupTriggerRequest and WarmupTriggerResponse (the same block used by the trigger_warmup handler), add a responses entry for (status = 500, description = "Internal server error") because resolve_warmup_agent_ids can return StatusCode::INTERNAL_SERVER_ERROR during partial runtime initialization and the generated client must be aware this outcome is possible.
1101-1112:⚠️ Potential issue | 🟠 MajorThese specs still don't match the HTTP behavior.
update_agent,delete_agent, andupload_avatarstill encode validation / lookup failures as200 OKJSON bodies on Lines 1119-1122, 1257-1270, and 2094-2097, while these annotations now advertise400/404. Generated clients will misclassify those paths until the handlers return real HTTP errors or the spec models them as200.Also applies to: 1238-1250, 2053-2067
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/agents.rs` around lines 1101 - 1112, The OpenAPI specs and handler behavior diverge: the handlers update_agent, delete_agent, and upload_avatar currently return 200 with JSON error payloads for validation/lookup failures but the utoipa annotations declare 400/404 responses; fix this by making the handlers return proper HTTP error responses (e.g., return actix_web::HttpResponse::BadRequest()/NotFound() or propagate a typed error (ApiError::BadRequest/NotFound) so the framework emits the correct status) and/or update the utoipa::path responses to reflect the actual 200-with-error behavior; ensure the handler signatures and return types (e.g., Result<HttpResponse, actix_web::Error> or Result<impl Responder, ApiError>) are adjusted so validation and lookup failures produce real 400/404 status codes consistent with the annotations.src/api/providers.rs (1)
536-545:⚠️ Potential issue | 🟠 MajorThese response annotations still don’t match the actual failure path.
The handlers behind these routes return
200 OKbodies withsuccess: false/found: falsefor validation and lookup failures, not the documented400/404. That still leaves the generated client with the wrong success/error typing for these flows.Also applies to: 731-742, 796-805, 885-894, 980-992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 536 - 545, The documented responses on the utoipa::path attributes for the provider endpoints (e.g. the attribute for POST "/providers/openai/browser-oauth/start" and the other similar attributes at the other ranges) are incorrect: the handlers always return HTTP 200 with a response body containing success:false/found:false on validation/lookup failures. Update those utoipa::path response annotations to reflect the actual behavior by removing the 400/404 variants and only documenting the 200 response (or alternately add separate 200 response entries representing both success and failure body shapes using the existing response body types like OpenAiOAuthBrowserStartResponse), and apply the same change to the other affected route attributes mentioned in the comment so the generated client types match runtime behavior.src/api/bindings.rs (2)
101-106:⚠️ Potential issue | 🟠 Major
restart_requiredis still misleading.
create_bindingstill returnsrestart_required: falseunconditionally at Line 695, even on the branches that write new platform credentials or later log hot-start failures. Either compute this from the actual outcome or drop the field until it can be truthful.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 101 - 106, The CreateBindingResponse struct's restart_required field is misleading because create_binding currently always returns restart_required: false; update create_binding to set restart_required = true on any path that writes or updates platform credentials (the branch that persists credentials) or on branches that later log hot-start failures, and set false otherwise, or alternatively remove restart_required from CreateBindingResponse and all call sites if you cannot reliably determine it; modify the return construction in create_binding and the CreateBindingResponse definition accordingly so the field reflects the actual outcome.
701-711:⚠️ Potential issue | 🟠 MajorThe 404 contract still disagrees with missing bindings.
On Lines 801-806 and 1028-1033,
update_bindinganddelete_bindingreturn200 OKwithsuccess: falsewhen no binding matches, but the new annotations advertise that case as404. Generated clients will get the wrong success/error typing until the handler or spec is aligned.Also applies to: 928-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 701 - 711, The OpenAPI/utoipa annotations declare a 404 for "binding not found" but the handlers update_binding and delete_binding currently return 200 with success:false when no binding matches; update the handlers (update_binding, delete_binding and any similar handlers returning a 200+success:false pattern such as the one referenced at the other spot) to return an actual 404 HTTP response when the binding/config is missing (use the UpdateBindingResponse/DeleteBindingResponse error variant or convert the handler to return a Result with an appropriate HttpResponse::NotFound and the same error body), or alternatively change the utoipa #[utoipa::path(...)] annotations to remove the 404 entry if you prefer keeping 200+success:false; make sure the runtime status and the declared responses (UpdateBindingResponse/DeleteBindingResponse) are consistent.
🧹 Nitpick comments (1)
src/api/links.rs (1)
14-21:serde_json::Valuehere throws away the generated response types.These endpoints all return stable wrapper shapes like
{"links": ...},{"group": ...}, and{"human": ...}, but the spec advertises them as arbitrary JSON. That undercuts the generated-type goal of this PR and forces consumers back to manual casting.Also applies to: 28-38, 188-199, 305-319, 486-510, 581-595, 781-788, 841-851, 983-995
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/links.rs` around lines 14 - 21, The utoipa path attributes currently use serde_json::Value which discards generated response schemas; replace those with concrete response types that model your stable wrapper shapes (e.g., define and use LinkListResponse { links: Vec<LinkDto> }, GroupResponse { group: GroupDto }, HumanResponse { human: HumanDto } or similar) and update each utoipa::path attribute (including the one with get path="/links" and the other ranges referenced) to use body = LinkListResponse / GroupResponse / HumanResponse as appropriate so the OpenAPI generator emits precise schemas; ensure the DTO types (LinkDto, GroupDto, HumanDto) are #[derive(Serialize, ToSchema)] (or map to existing domain types) and import them where the path attributes are declared.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/api/agents.rs`:
- Around line 317-327: The utoipa::path for POST "/agents/mcp/reconnect"
currently declares anonymous JSON responses (serde_json::Value); create concrete
response DTO structs (e.g., ReconnectMcpResponse with fields like success: bool,
agent_id: String, message: Option<String>, server_name: Option<String>, path:
Option<String>) and replace the anonymous bodies in the responses attribute with
those DTO types, then update the handler to return those structs (or
Result<ReconnectMcpResponse, ErrorDto>) so the generated TypeScript client has
stable field-level types; apply the same pattern for the other endpoints you
noted (the blocks around the other ranges) by defining specific response DTOs
and referencing them in their utoipa::path responses.
In `@src/api/bindings.rs`:
- Around line 220-230: The create_binding, update_binding, and delete_binding
handlers perform an unguarded read-parse-write on config.toml which can lead to
races; wrap the entire mutation sequence (read file, parse, modify, serialize,
write back) with the shared write lock used elsewhere (state.config_write_mutex)
to ensure exclusive access. Specifically, in the functions create_binding,
update_binding, and delete_binding acquire
state.config_write_mutex.write().await before reading and parsing the config and
only release after the file has been successfully written (and handle errors
while holding/releasing the lock properly). Follow the same locking pattern used
in src/api/agents.rs so concurrent agent/provider endpoints and binding mutators
cannot overwrite each other.
In `@src/api/links.rs`:
- Around line 188-199: The OpenAPI annotations for mutating endpoints currently
omit a 500 response and use body = serde_json::Value; update each affected
#[utoipa::path(...)] for the create/update/delete handlers (e.g., create_link,
update_link, delete_link and the equivalent group/human handlers) to add (status
= 500, description = "Internal server error") to responses and replace body =
serde_json::Value with concrete response schema types (e.g., CreateLinkResponse,
UpdateLinkResponse, DeleteLinkResponse, CreateGroupResponse, etc.); then add
small response structs (derive Serialize, ToSchema) that wrap the stable shapes
returned (e.g., struct CreateLinkResponse { link: Link }, struct
CreateGroupResponse { group: Group }, struct CreateHumanResponse { human: Human
}, etc.) so the annotations reference those types and the OpenAPI contract
matches actual responses.
In `@src/api/providers.rs`:
- Around line 796-805: The utoipa annotations for the provider handlers don't
include a 500 response while the handlers (notably update_provider and
delete_provider) map config read/parse/write failures to
StatusCode::INTERNAL_SERVER_ERROR; update the #[utoipa::path(...)] for
update_provider and delete_provider to add a (status = 500, description =
"Internal server error") response entry (and likewise add a 500 response to any
other provider handler annotation in this file that currently lists only
200/400[/404]) so the OpenAPI contract matches the code's persistence-failure
behavior.
- Around line 980-992: The delete-provider handler currently writes the config
but does not refresh runtime defaults or notify the provider setup workflow,
leaving the process using the removed provider; update the delete handler (the
same function that performs the config write) to call
refresh_defaults_config(&state) after the write and then notify
provider_setup_tx in the same way update_provider and finalize_openai_oauth do
(i.e., clone the sender/tx and send the same message/enum used by those
handlers) so the running service immediately picks up the removal.
---
Outside diff comments:
In `@src/api/cron.rs`:
- Around line 303-341: The OpenAPI attribute on create_or_update_cron declares
responses but only sets body = CronActionResponse for status 200; update the
#[utoipa::path(... responses(...))] on the create_or_update_cron handler to
include body = CronActionResponse for the error statuses (400, 404, 500) so that
the generated schema reflects the Json<CronActionResponse> error payloads
returned by the function; ensure the responses tuple entries for 400, 404, and
500 each include body = CronActionResponse while keeping their descriptions.
In `@src/api/webchat.rs`:
- Around line 31-77: The utoipa OpenAPI annotation on the webchat_send handler
is missing the 500 response that the code can return when
manager.inject_message(...) fails; update the #[utoipa::path(...)] attribute on
the webchat_send function to include responses((status = 500, description =
"Failed to inject message")) so the documented responses match the actual
behavior where inject_message maps errors to StatusCode::INTERNAL_SERVER_ERROR.
---
Duplicate comments:
In `@src/api/agents.rs`:
- Around line 400-409: Update the utoipa path annotation for the trigger_warmup
endpoint to include a 500 response so the OpenAPI spec reflects server errors;
specifically, in the attribute block that references WarmupTriggerRequest and
WarmupTriggerResponse (the same block used by the trigger_warmup handler), add a
responses entry for (status = 500, description = "Internal server error")
because resolve_warmup_agent_ids can return StatusCode::INTERNAL_SERVER_ERROR
during partial runtime initialization and the generated client must be aware
this outcome is possible.
- Around line 1101-1112: The OpenAPI specs and handler behavior diverge: the
handlers update_agent, delete_agent, and upload_avatar currently return 200 with
JSON error payloads for validation/lookup failures but the utoipa annotations
declare 400/404 responses; fix this by making the handlers return proper HTTP
error responses (e.g., return actix_web::HttpResponse::BadRequest()/NotFound()
or propagate a typed error (ApiError::BadRequest/NotFound) so the framework
emits the correct status) and/or update the utoipa::path responses to reflect
the actual 200-with-error behavior; ensure the handler signatures and return
types (e.g., Result<HttpResponse, actix_web::Error> or Result<impl Responder,
ApiError>) are adjusted so validation and lookup failures produce real 400/404
status codes consistent with the annotations.
In `@src/api/bindings.rs`:
- Around line 101-106: The CreateBindingResponse struct's restart_required field
is misleading because create_binding currently always returns restart_required:
false; update create_binding to set restart_required = true on any path that
writes or updates platform credentials (the branch that persists credentials) or
on branches that later log hot-start failures, and set false otherwise, or
alternatively remove restart_required from CreateBindingResponse and all call
sites if you cannot reliably determine it; modify the return construction in
create_binding and the CreateBindingResponse definition accordingly so the field
reflects the actual outcome.
- Around line 701-711: The OpenAPI/utoipa annotations declare a 404 for "binding
not found" but the handlers update_binding and delete_binding currently return
200 with success:false when no binding matches; update the handlers
(update_binding, delete_binding and any similar handlers returning a
200+success:false pattern such as the one referenced at the other spot) to
return an actual 404 HTTP response when the binding/config is missing (use the
UpdateBindingResponse/DeleteBindingResponse error variant or convert the handler
to return a Result with an appropriate HttpResponse::NotFound and the same error
body), or alternatively change the utoipa #[utoipa::path(...)] annotations to
remove the 404 entry if you prefer keeping 200+success:false; make sure the
runtime status and the declared responses
(UpdateBindingResponse/DeleteBindingResponse) are consistent.
In `@src/api/providers.rs`:
- Around line 536-545: The documented responses on the utoipa::path attributes
for the provider endpoints (e.g. the attribute for POST
"/providers/openai/browser-oauth/start" and the other similar attributes at the
other ranges) are incorrect: the handlers always return HTTP 200 with a response
body containing success:false/found:false on validation/lookup failures. Update
those utoipa::path response annotations to reflect the actual behavior by
removing the 400/404 variants and only documenting the 200 response (or
alternately add separate 200 response entries representing both success and
failure body shapes using the existing response body types like
OpenAiOAuthBrowserStartResponse), and apply the same change to the other
affected route attributes mentioned in the comment so the generated client types
match runtime behavior.
In `@src/api/server.rs`:
- Around line 32-44: ApiDoc's OpenApi derive doesn't declare a bearer security
scheme so clients/Swagger won't know protected_routes require tokens; update the
ApiDoc OpenApi configuration (the ApiDoc struct with #[derive(OpenApi)] and
#[openapi(...)] ) to add a bearer authentication scheme (e.g., security_schemes
with type http scheme="bearer" bearer_format="JWT") and apply that scheme
globally (security = (("bearerAuth", []), ...)) or annotate protected endpoints
accordingly; ensure the /api/health route is excluded from the global
requirement (either omit it from the annotated list or add an explicit
no-security attribute) so api_auth_middleware-protected_routes behavior matches
the OpenAPI spec.
In `@src/api/ssh.rs`:
- Around line 49-69: The OpenAPI annotations for set_authorized_key/ssh
endpoints declare 400 and 500 responses but the handler always returns
Ok(Json(...)), so update the runtime to match the spec or vice-versa: either (A)
change set_authorized_key (and the ssh_status helper) to return
Err(StatusCode::BAD_REQUEST) for invalid keys and
Err(StatusCode::INTERNAL_SERVER_ERROR) on failures (return Result::Err instead
of Ok(Json(...))) so the HTTP status codes declared by the utoipa::path
attribute are actually emitted, or (B) remove the 400/500 response entries from
the utoipa::path attribute above set_authorized_key so the docs match the
current behavior; pick one approach and apply it consistently to
set_authorized_key and the ssh_status-related functions.
---
Nitpick comments:
In `@src/api/links.rs`:
- Around line 14-21: The utoipa path attributes currently use serde_json::Value
which discards generated response schemas; replace those with concrete response
types that model your stable wrapper shapes (e.g., define and use
LinkListResponse { links: Vec<LinkDto> }, GroupResponse { group: GroupDto },
HumanResponse { human: HumanDto } or similar) and update each utoipa::path
attribute (including the one with get path="/links" and the other ranges
referenced) to use body = LinkListResponse / GroupResponse / HumanResponse as
appropriate so the OpenAPI generator emits precise schemas; ensure the DTO types
(LinkDto, GroupDto, HumanDto) are #[derive(Serialize, ToSchema)] (or map to
existing domain types) and import them where the path attributes are declared.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 72bd0e4c-734f-4970-80a6-df536fd1726a
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomlinterface/bun.lockis excluded by!**/*.lock,!**/*.lockinterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (62)
interface/src/api/client-typed.tsinterface/src/api/client.tsinterface/src/api/schema.d.tsinterface/src/api/types.tsinterface/src/components/ProfileAvatar.tsxinterface/src/components/Sidebar.tsxinterface/src/components/TaskBoard.tsxinterface/src/components/TopologyGraph.tsxinterface/src/hooks/useAudioRecorder.tsinterface/src/hooks/useTtsPlayback.tsinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentWorkers.tsxinterface/src/routes/ChannelDetail.tsxinterface/src/routes/GlobalTasks.tsxinterface/src/routes/Orchestrate.tsxjustfilesrc/agent/cortex.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/bindings.rssrc/api/channels.rssrc/api/config.rssrc/api/cortex.rssrc/api/cron.rssrc/api/factory.rssrc/api/ingest.rssrc/api/links.rssrc/api/mcp.rssrc/api/memories.rssrc/api/messaging.rssrc/api/models.rssrc/api/projects.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/skills.rssrc/api/ssh.rssrc/api/state.rssrc/api/system.rssrc/api/tasks.rssrc/api/tools.rssrc/api/webchat.rssrc/api/workers.rssrc/bin/openapi_spec.rssrc/config/types.rssrc/conversation/history.rssrc/conversation/worker_transcript.rssrc/cron/store.rssrc/factory/presets.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/memory/store.rssrc/memory/types.rssrc/opencode/types.rssrc/projects/store.rssrc/secrets/store.rssrc/tasks/store.rssrc/tools/memory_save.rssrc/update.rs
✅ Files skipped from review due to trivial changes (27)
- interface/src/routes/Orchestrate.tsx
- src/bin/openapi_spec.rs
- interface/src/routes/GlobalTasks.tsx
- interface/src/components/TopologyGraph.tsx
- interface/src/components/ProfileAvatar.tsx
- src/conversation/history.rs
- src/conversation/worker_transcript.rs
- src/cron/store.rs
- src/factory/presets.rs
- src/mcp.rs
- src/update.rs
- src/api/factory.rs
- src/agent/cortex_chat.rs
- interface/src/hooks/useTtsPlayback.ts
- src/projects/store.rs
- src/opencode/types.rs
- src/config/types.rs
- src/agent/cortex.rs
- src/api/system.rs
- src/api/workers.rs
- interface/src/hooks/useAudioRecorder.ts
- src/api/secrets.rs
- src/api/ingest.rs
- src/api/cortex.rs
- src/api/channels.rs
- src/api/tasks.rs
- interface/src/api/types.ts
🚧 Files skipped from review as they are similar to previous changes (18)
- src/api.rs
- interface/src/routes/ChannelDetail.tsx
- src/api/messaging.rs
- interface/src/components/TaskBoard.tsx
- src/secrets/store.rs
- src/main.rs
- justfile
- interface/src/api/client-typed.ts
- src/api/tools.rs
- src/api/mcp.rs
- src/memory/types.rs
- src/tasks/store.rs
- src/api/models.rs
- src/api/projects.rs
- src/api/config.rs
- src/api/settings.rs
- interface/src/api/client.ts
- src/api/skills.rs
Comment on lines
+317
to
+327
| #[utoipa::path( | ||
| post, | ||
| path = "/agents/mcp/reconnect", | ||
| request_body = ReconnectMcpRequest, | ||
| responses( | ||
| (status = 200, body = serde_json::Value), | ||
| (status = 404, description = "Agent not found"), | ||
| (status = 400, description = "Failed to reconnect"), | ||
| ), | ||
| tag = "agents", | ||
| )] |
Contributor
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace these free-form JSON bodies with concrete DTOs.
These endpoints still expose anonymous object responses, so the generated TypeScript client will not get stable field-level types for fields like success, agent_id, message, server_name, or path. That leaves visible gaps in the “full type safety” goal of this migration.
Also applies to: 519-530, 1101-1112, 1238-1250, 2053-2067, 2125-2136
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/agents.rs` around lines 317 - 327, The utoipa::path for POST
"/agents/mcp/reconnect" currently declares anonymous JSON responses
(serde_json::Value); create concrete response DTO structs (e.g.,
ReconnectMcpResponse with fields like success: bool, agent_id: String, message:
Option<String>, server_name: Option<String>, path: Option<String>) and replace
the anonymous bodies in the responses attribute with those DTO types, then
update the handler to return those structs (or Result<ReconnectMcpResponse,
ErrorDto>) so the generated TypeScript client has stable field-level types;
apply the same pattern for the other endpoints you noted (the blocks around the
other ranges) by defining specific response DTOs and referencing them in their
utoipa::path responses.
Comment on lines
+220
to
+230
| #[utoipa::path( | ||
| post, | ||
| path = "/bindings", | ||
| request_body = CreateBindingRequest, | ||
| responses( | ||
| (status = 200, body = CreateBindingResponse), | ||
| (status = 400, description = "Invalid request"), | ||
| (status = 500, description = "Internal server error"), | ||
| ), | ||
| tag = "bindings", | ||
| )] |
Contributor
There was a problem hiding this comment.
Guard these config mutations with the shared write mutex.
create_binding, update_binding, and delete_binding all do an unlocked read-parse-write cycle on config.toml. Two concurrent mutations can overwrite each other, especially alongside the agent/provider endpoints that touch the same file. src/api/agents.rs already uses state.config_write_mutex for this exact pattern.
🔒 Minimal pattern to apply in each binding mutator
pub(super) async fn create_binding(
State(state): State<Arc<ApiState>>,
axum::Json(request): axum::Json<CreateBindingRequest>,
) -> Result<Json<CreateBindingResponse>, StatusCode> {
+ let _config_guard = state.config_write_mutex.lock().await;
let config_path = state.config_path.read().await.clone();
...Also applies to: 701-711, 928-938
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/bindings.rs` around lines 220 - 230, The create_binding,
update_binding, and delete_binding handlers perform an unguarded
read-parse-write on config.toml which can lead to races; wrap the entire
mutation sequence (read file, parse, modify, serialize, write back) with the
shared write lock used elsewhere (state.config_write_mutex) to ensure exclusive
access. Specifically, in the functions create_binding, update_binding, and
delete_binding acquire state.config_write_mutex.write().await before reading and
parsing the config and only release after the file has been successfully written
(and handle errors while holding/releasing the lock properly). Follow the same
locking pattern used in src/api/agents.rs so concurrent agent/provider endpoints
and binding mutators cannot overwrite each other.
Comment on lines
+188
to
+199
| #[utoipa::path( | ||
| post, | ||
| path = "/links", | ||
| request_body = CreateLinkRequest, | ||
| responses( | ||
| (status = 201, body = serde_json::Value), | ||
| (status = 400, description = "Invalid request"), | ||
| (status = 404, description = "Agent or human not found"), | ||
| (status = 409, description = "Link already exists"), | ||
| ), | ||
| tag = "links", | ||
| )] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
wc -l src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 87
🏁 Script executed:
sed -n '188,199p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 359
🏁 Script executed:
sed -n '200,220p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 769
🏁 Script executed:
sed -n '305,330p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 757
🏁 Script executed:
sed -n '391,410p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 602
🏁 Script executed:
sed -n '200,280p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 3126
🏁 Script executed:
sed -n '280,310p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 859
🏁 Script executed:
sed -n '330,380p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 2115
🏁 Script executed:
sed -n '391,420p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 913
🏁 Script executed:
sed -n '500,530p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 952
🏁 Script executed:
sed -n '581,610p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 908
🏁 Script executed:
rg -n "StatusCode::INTERNAL_SERVER_ERROR" src/api/links.rs | head -20Repository: spacedriveapp/spacebot
Length of output: 1053
🏁 Script executed:
sed -n '684,710p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 736
🏁 Script executed:
sed -n '841,870p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 1035
🏁 Script executed:
sed -n '983,1020p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 1123
🏁 Script executed:
sed -n '413,450p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 1458
🏁 Script executed:
sed -n '628,665p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 1448
🏁 Script executed:
sed -n '1020,1060p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 1190
🏁 Script executed:
sed -n '1167,1200p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 1009
🏁 Script executed:
sed -n '737,765p' src/api/links.rsRepository: spacedriveapp/spacebot
Length of output: 765
Add 500 responses to all mutating link/group/human endpoint annotations.
Each handler (create/update/delete link, group, and human) performs config.toml read, parse, and write operations that can fail with INTERNAL_SERVER_ERROR, but the responses(...) blocks omit (status = 500). This creates a contract mismatch in the generated OpenAPI schema.
Additionally, all these endpoints document body = serde_json::Value but return stable shapes ({ "link": ... }, { "group": ... }, { "human": ... }). Define proper response schemas for better client code generation.
Affected annotations at lines 188–199, 305–319, 391–403, 500–510, 581–595, 684–695, 841–851, 983–995, 1167–1178.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/links.rs` around lines 188 - 199, The OpenAPI annotations for
mutating endpoints currently omit a 500 response and use body =
serde_json::Value; update each affected #[utoipa::path(...)] for the
create/update/delete handlers (e.g., create_link, update_link, delete_link and
the equivalent group/human handlers) to add (status = 500, description =
"Internal server error") to responses and replace body = serde_json::Value with
concrete response schema types (e.g., CreateLinkResponse, UpdateLinkResponse,
DeleteLinkResponse, CreateGroupResponse, etc.); then add small response structs
(derive Serialize, ToSchema) that wrap the stable shapes returned (e.g., struct
CreateLinkResponse { link: Link }, struct CreateGroupResponse { group: Group },
struct CreateHumanResponse { human: Human }, etc.) so the annotations reference
those types and the OpenAPI contract matches actual responses.
Comment on lines
+796
to
+805
| #[utoipa::path( | ||
| post, | ||
| path = "/providers", | ||
| request_body = ProviderUpdateRequest, | ||
| responses( | ||
| (status = 200, body = ProviderUpdateResponse), | ||
| (status = 400, description = "Invalid request"), | ||
| ), | ||
| tag = "providers", | ||
| )] |
Contributor
There was a problem hiding this comment.
update_provider and delete_provider are missing their real 500 path.
Both handlers map config read/parse/write failures to StatusCode::INTERNAL_SERVER_ERROR on Lines 846-866 and 1004-1063, but these annotations only list 200/400 and 200/400/404. The contract should include 500 for persistence failures.
🛠️ Suggested annotation updates
#[utoipa::path(
post,
path = "/providers",
request_body = ProviderUpdateRequest,
responses(
(status = 200, body = ProviderUpdateResponse),
(status = 400, description = "Invalid request"),
+ (status = 500, description = "Internal server error"),
),
tag = "providers",
)] #[utoipa::path(
delete,
path = "/providers/{provider}",
params(
("provider" = String, Path, description = "Provider name to delete"),
),
responses(
(status = 200, body = ProviderUpdateResponse),
(status = 400, description = "Invalid request"),
(status = 404, description = "Provider not found"),
+ (status = 500, description = "Internal server error"),
),
tag = "providers",
)]Also applies to: 980-992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/providers.rs` around lines 796 - 805, The utoipa annotations for the
provider handlers don't include a 500 response while the handlers (notably
update_provider and delete_provider) map config read/parse/write failures to
StatusCode::INTERNAL_SERVER_ERROR; update the #[utoipa::path(...)] for
update_provider and delete_provider to add a (status = 500, description =
"Internal server error") response entry (and likewise add a 500 response to any
other provider handler annotation in this file that currently lists only
200/400[/404]) so the OpenAPI contract matches the code's persistence-failure
behavior.
Comment on lines
+980
to
+992
| #[utoipa::path( | ||
| delete, | ||
| path = "/providers/{provider}", | ||
| params( | ||
| ("provider" = String, Path, description = "Provider name to delete"), | ||
| ), | ||
| responses( | ||
| (status = 200, body = ProviderUpdateResponse), | ||
| (status = 400, description = "Invalid request"), | ||
| (status = 404, description = "Provider not found"), | ||
| ), | ||
| tag = "providers", | ||
| )] |
Contributor
There was a problem hiding this comment.
Deleting a provider leaves the live config stale.
After the config write on Lines 1061-1063, this handler returns success without calling refresh_defaults_config(&state) or notifying provider_setup_tx, unlike update_provider and finalize_openai_oauth. The process can keep using the removed provider until restart even though the API says it is gone.
🔄 Minimal follow-up after the write
tokio::fs::write(&config_path, doc.to_string())
.await
.map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?;
+
+ refresh_defaults_config(&state).await;
+ state
+ .provider_setup_tx
+ .try_send(crate::ProviderSetupEvent::ProvidersConfigured)
+ .ok();
Ok(Json(ProviderUpdateResponse {
success: true,
message: format!("Provider '{}' removed", provider),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/providers.rs` around lines 980 - 992, The delete-provider handler
currently writes the config but does not refresh runtime defaults or notify the
provider setup workflow, leaving the process using the removed provider; update
the delete handler (the same function that performs the config write) to call
refresh_defaults_config(&state) after the write and then notify
provider_setup_tx in the same way update_provider and finalize_openai_oauth do
(i.e., clone the sender/tx and send the same message/enum used by those
handlers) so the running service immediately picks up the removal.
1709e19 to
8b8bf2c
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/api/config.rs (1)
283-290:⚠️ Potential issue | 🟠 MajorDocument the existing 404 path on config updates.
update_agent_config()persists the TOML and then callsget_agent_config()to build the response. If the agent is absent fromstate.runtime_configs, that helper returnsStatusCode::NOT_FOUND, so this endpoint can already emit 404 even though the annotation only advertises 200/400/500.📝 Annotation update
#[utoipa::path( put, path = "/agents/config", request_body = AgentConfigUpdateRequest, responses( (status = 200, body = AgentConfigResponse), (status = 400, description = "Invalid request"), + (status = 404, description = "Agent not found"), (status = 500, description = "Internal server error"), ), tag = "config", )]Also applies to: 412-422, 541-548
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/config.rs` around lines 283 - 290, The endpoint documentation/response annotations for update_agent_config (and any other handlers that call get_agent_config to build responses) are missing the 404 response; update the OpenAPI/route annotations for update_agent_config and the other annotated handlers that later call get_agent_config so they include StatusCode::NOT_FOUND/HTTP 404 as a possible response alongside 200/400/500 to reflect that get_agent_config (the function that looks up state.runtime_configs and returns NOT_FOUND when the agent key is missing) can produce a 404; ensure the annotations reference the same AgentConfigResponse/error schema used for other statuses and adjust any doc comments to mention the 404 case.interface/src/api/client.ts (2)
1659-1674:⚠️ Potential issue | 🔴 CriticalUpload still posts to the removed ingest route.
The backend upload handler is documented and registered at
POST /agents/ingest/files, but this helper still posts to/agents/ingest/upload. List/delete were already moved to/agents/ingest/files, so uploads will hit a dead path.📤 Path fix
const response = await fetch( - `${getApiBase()}/agents/ingest/upload?agent_id=${encodeURIComponent(agentId)}`, + `${getApiBase()}/agents/ingest/files?agent_id=${encodeURIComponent(agentId)}`, { method: "POST", body: formData }, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 1659 - 1674, The uploadIngestFiles helper is posting to the removed path `/agents/ingest/upload`; update the fetch URL in uploadIngestFiles to use the documented/registered path `/agents/ingest/files` (keep the same query param encoding of agentId and method POST and FormData body), so it matches ingestFiles and the backend handler; modify the URL string in uploadIngestFiles accordingly.
1585-1628:⚠️ Potential issue | 🔴 CriticalThese provider helpers still call the pre-migration routes.
src/api/providers.rsnow exposesPOST /providers,POST /providers/test-model, and/providers/openai/browser-oauth/{start,status}. The helpers here still usePUT /providers,/providers/test, and/providers/openai/oauth/browser/..., so provider setup and OAuth flows will 404/405 against the new router.🛠️ Route/method alignment
updateProvider: async (provider: string, apiKey: string, model: string) => { const response = await fetch(`${getApiBase()}/providers`, { - method: "PUT", + method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ provider, api_key: apiKey, model }), }); @@ testProviderModel: async (provider: string, apiKey: string, model: string) => { - const response = await fetch(`${getApiBase()}/providers/test`, { + const response = await fetch(`${getApiBase()}/providers/test-model`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ provider, api_key: apiKey, model }), }); @@ startOpenAiOAuthBrowser: async (params: {model: string}) => { - const response = await fetch(`${getApiBase()}/providers/openai/oauth/browser/start`, { + const response = await fetch(`${getApiBase()}/providers/openai/browser-oauth/start`, { method: "POST", headers: { "Content-Type": "application/json" }, body: JSON.stringify({ model: params.model, }), }); @@ openAiOAuthBrowserStatus: async (state: string) => { const response = await fetch( - `${getApiBase()}/providers/openai/oauth/browser/status?state=${encodeURIComponent(state)}`, + `${getApiBase()}/providers/openai/browser-oauth/status?state=${encodeURIComponent(state)}`, );🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 1585 - 1628, The client helpers updateProvider, testProviderModel, startOpenAiOAuthBrowser, and openAiOAuthBrowserStatus call the old routes/methods and must be changed to match the new server endpoints: change updateProvider to POST to /providers (not PUT), change testProviderModel to POST to /providers/test-model (not /providers/test), and change startOpenAiOAuthBrowser and openAiOAuthBrowserStatus to use the new paths /providers/openai/browser-oauth/start and /providers/openai/browser-oauth/status (matching the server’s path segment ordering) while preserving JSON bodies and response typing (Types.ProviderUpdateResponse, Types.ProviderModelTestResponse, Types.OpenAiOAuthBrowserStartResponse, Types.OpenAiOAuthBrowserStatusResponse) so the functions continue to return the same promise types.
♻️ Duplicate comments (10)
src/api/bindings.rs (3)
231-254:⚠️ Potential issue | 🟠 MajorConfig mutation handlers are still unguarded against concurrent write races.
create_binding,update_binding, anddelete_bindingall do read-parse-modify-write cycles onconfig.tomlwithout acquiringstate.config_write_mutex, so concurrent requests can clobber each other.Also applies to: 496-501, 716-732, 880-885, 943-959, 1037-1042
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 231 - 254, The create_binding, update_binding, and delete_binding handlers perform read-parse-modify-write cycles on config.toml without acquiring the shared write mutex, causing races; fix each handler (create_binding, update_binding, delete_binding and the other similar blocks referenced) by acquiring state.config_write_mutex.lock().await before starting the read/parse/modify/write sequence and holding that lock until after the file has been written and state updated so the entire critical section (tokio::fs::read_to_string, parsing into toml_edit::DocumentMut, applying changes, writing the file, and any in-memory state updates) is protected; ensure you release the lock only after the write and any state updates succeed or after handling/propagating errors.
101-106:⚠️ Potential issue | 🟠 Major
restart_requiredis still hardcoded and does not reflect actual outcome.Line 104 describes semantic meaning, but response construction still returns
restart_required: falseunconditionally at Line 695.Also applies to: 693-696
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 101 - 106, CreateBindingResponse.restart_required is currently hardcoded false; update the code paths that construct CreateBindingResponse (where the response is returned after creating/updating a binding) to compute restart_required from the actual outcome (e.g., a boolean like credentials_changed / platform_credentials_modified or adapter_restart_required that you determine when saving/updating the binding). Locate the response construction that instantiates CreateBindingResponse and replace the literal false with that computed boolean, and ensure both create and update branches set that flag consistently (derive the boolean from the logic that detects added/changed platform credentials).
701-711:⚠️ Potential issue | 🟠 Major
404docs still don’t match update/delete binding behavior.
update_bindinganddelete_bindingadvertise 404, but not-found cases currently return HTTP 200 with{ success: false }(Line 801 and Line 1028). This keeps the OpenAPI contract out of sync with runtime behavior.Also applies to: 801-806, 928-938, 1028-1033
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/bindings.rs` around lines 701 - 711, The OpenAPI doc for the PUT /bindings endpoint claims a 404 for missing binding/config, but the handlers return HTTP 200 with { success: false }; update the runtime to match the docs by returning an actual 404 when a binding or its config is not found in the update_binding and delete_binding handlers (and any other binding handlers referenced), or alternatively change the utoipa response declarations to reflect the current 200/{ success: false } behavior; modify the logic inside update_binding and delete_binding to call HttpResponse::NotFound (or return Err(HttpError::NotFound) per your error type) with an appropriate error body where you currently return success: false so the OpenAPI contract and runtime behavior are consistent.src/api/skills.rs (1)
364-377:⚠️ Potential issue | 🟠 Major
upload_skillOpenAPI docs still omit multipart request body schema.Line 378 accepts
axum::extract::Multipart, but the#[utoipa::path]block still lacks a multipartrequest_body(...). That leaves the generated spec incomplete for this endpoint.For utoipa (v5) with axum Multipart handlers, what is the recommended `#[utoipa::path]` syntax to document `multipart/form-data` request bodies, including binary file fields?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/skills.rs` around lines 364 - 377, The OpenAPI attrs for upload_skill need a multipart/form-data request_body so the axum::extract::Multipart handler is represented: update the #[utoipa::path] on upload_skill to include request_body(...) with content for "multipart/form-data" that defines a schema for the form object (e.g., a schema or inline object with a "file" property type: string, format: binary and any other form fields your handler expects), and reference that schema name in the attribute so the generated spec documents the binary file field alongside the existing UploadSkillResponse, keeping agent_id as a Query param.src/api/ssh.rs (1)
49-57:⚠️ Potential issue | 🟠 MajorSSH response status contracts are still inconsistent with handler behavior.
set_authorized_key: Line 55 documents 400, but Line 66 returns HTTP 200 for invalid input.ssh_status: Line 109-110 documents 400/500, but handler always returns 200.Please align annotations and actual status codes so generated clients don’t mis-handle error paths.
Also applies to: 65-69, 104-111, 114-127
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/ssh.rs` around lines 49 - 57, The OpenAPI annotations (utoipa::path) for the SSH endpoints do not match actual handler behavior—update either the handlers or the annotations so they align: for set_authorized_key (function set_authorized_key, request/response types AuthorizedKeyRequest/AuthorizedKeyResponse) and ssh_status (function ssh_status) decide whether invalid input should produce a 400/500 and implement that in the handler by returning proper HTTP responses (e.g., return Err(HttpResponse::BadRequest(...)) or HttpResponse::InternalServerError(...) where appropriate), or alternatively remove the 400/500 entries from the utoipa::path response list so the attribute only documents the statuses the handlers actually return (e.g., 200); make the same change for all related utoipa::path blocks referenced in the diff.src/api/links.rs (1)
200-211:⚠️ Potential issue | 🟠 MajorOpenAPI mutator contracts still miss real error/shape behavior.
These annotations still omit documented
500responses even though handler code can returnINTERNAL_SERVER_ERRORon config read/parse/write failures. They also keepbody = serde_json::Valuefor stable wrapper payloads ({ link: ... },{ group: ... },{ human: ... }), which weakens generated client typing.Also applies to: 317-331, 403-415, 512-522, 593-607, 696-707, 853-863, 995-1007, 1179-1190
src/api/server.rs (1)
32-44:⚠️ Potential issue | 🟠 MajorThe generated spec still marks protected operations as anonymous.
api_auth_middlewareguards the nested/apirouter, butApiDoconly declaresinfoandservers. Swagger UI and any OpenAPI consumer will treat those routes as public unless the document also defines the bearer scheme and applies it to the protected operations.Also applies to: 261-278, 311-340
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/server.rs` around lines 32 - 44, ApiDoc currently only declares info and servers which leaves protected routes marked as anonymous; update the OpenApi derive on the ApiDoc struct to declare a bearer authentication scheme in components (e.g. a security_scheme named "bearerAuth" of type http scheme "bearer" with bearerFormat "JWT") and add a global security requirement referencing that scheme (or apply it to specific operations) so that the protected endpoints guarded by api_auth_middleware are marked as requiring bearerAuth in the generated spec; modify the #[openapi(...)] attributes on ApiDoc accordingly to include components.security_schemes and security entries.interface/src/api/client.ts (1)
206-211:⚠️ Potential issue | 🟡 MinorGive
AgentMessageEventthe same discriminator treatment as the other events.Without a literal
typeand a slot inApiEvent, this interface cannot participate in the same narrowing pattern as the rest of the SSE model.Also applies to: 249-264
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@interface/src/api/client.ts` around lines 206 - 211, AgentMessageEvent is missing a discriminant so it can't be narrowed like the other SSE events; add a readonly literal property type: "agent_message" (or the chosen event name consistent with other events) to the AgentMessageEvent interface and then add AgentMessageEvent to the ApiEvent union (where ApiEvent discriminates on the type field) so code can use switch/type-narrowing on ApiEvent.type; also apply the same change to the other similar event interfaces referenced (the block around the other event definitions) so they all expose a consistent type discriminator.src/api/providers.rs (2)
980-992:⚠️ Potential issue | 🟠 MajorRefresh runtime provider state after deletions.
After the config write, this handler returns success without calling
refresh_defaults_config(&state)or notifyingprovider_setup_tx, unlikeupdate_provider()andfinalize_openai_oauth(). The process can keep serving a removed provider until restart.🔄 Minimal follow-up after the write
tokio::fs::write(&config_path, doc.to_string()) .await .map_err(|_| StatusCode::INTERNAL_SERVER_ERROR)?; + + refresh_defaults_config(&state).await; + state + .provider_setup_tx + .try_send(crate::ProviderSetupEvent::ProvidersConfigured) + .ok(); Ok(Json(ProviderUpdateResponse { success: true, message: format!("Provider '{}' removed", provider),🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 980 - 992, The delete-provider handler in providers.rs writes the config but never refreshes runtime state or signals the provider setup channel, so removed providers remain active until restart; after the config write in the delete handler, call refresh_defaults_config(&state) (same as update_provider() and finalize_openai_oauth()) and send/notify via provider_setup_tx (e.g., provider_setup_tx.send(()) or the existing notification mechanism) so runtime state is updated before returning success.
536-545:⚠️ Potential issue | 🟠 MajorThe documented provider error statuses still don't match the handlers.
These annotations now drive the generated client, but the handlers still return
200 OKwithsuccess: falsefor validation and lookup failures such as empty model/API key, unknown provider, missing OAuth state, or missing config.update_provider()anddelete_provider()also surface real500s that are not declared here. Either return actual 4xx/5xx responses or model those failures as200bodies consistently.Also applies to: 731-742, 796-805, 885-894, 980-992
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/api/providers.rs` around lines 536 - 545, The documented response statuses in the utoipa annotations (e.g., the OpenAiOAuthBrowserStart endpoint annotated with OpenAiOAuthBrowserStartRequest/OpenAiOAuthBrowserStartResponse) do not match the actual handler behavior (handlers currently return 200 with success: false for validation/lookup failures and real 500s from update_provider()/delete_provider()); update either the handlers or the annotations to be consistent: either modify the handlers (functions like update_provider, delete_provider and the OAuth start/complete handlers) to return proper HTTP error responses (4xx for validation/missing/unknown provider, 500 for internal errors) or change the annotations for each affected endpoint (the OpenAi OAuth endpoints and the endpoints around the other noted ranges) to document 200 responses carrying error payloads, and ensure the generated client and response types (e.g., OpenAiOAuthBrowserStartResponse) reflect the chosen approach.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@interface/src/api/client.ts`:
- Around line 23-25: The local config types used by agentConfig() and
updateAgentConfig() are stale; replace the handwritten shapes by re-exporting
the schema-backed types AgentConfigResponse and AgentConfigUpdateRequest (or
import and use them directly) so the client API matches src/api/config.rs;
update the function signatures for agentConfig() and updateAgentConfig() to
accept/return those schema types and remove or reconcile any local properties
like *_thinking_effort, warmup, sandbox.passthrough_env, and cortex maintenance
fields to rely on the canonical schema types (also update any other local
re-exports that mirror config types).
In `@src/api/ingest.rs`:
- Around line 111-124: The upload handler currently swallows
multipart.next_field() errors and discards the Result from the DB insert,
causing 200 responses on partial/failed uploads; update the upload handler to
properly handle errors from multipart.next_field() inside the processing loop
(log the error and return a 400/500 response or propagate the error) and remove
the "let _ =" discard for the insert into ingest history—check the Result from
the insert call, log failures with context (e.g., file name/agent_id) and return
an appropriate error response instead of treating it as success; ensure no
Result is silently ignored anywhere in the multipart processing or the insert
logic so failures are surfaced to the caller.
---
Outside diff comments:
In `@interface/src/api/client.ts`:
- Around line 1659-1674: The uploadIngestFiles helper is posting to the removed
path `/agents/ingest/upload`; update the fetch URL in uploadIngestFiles to use
the documented/registered path `/agents/ingest/files` (keep the same query param
encoding of agentId and method POST and FormData body), so it matches
ingestFiles and the backend handler; modify the URL string in uploadIngestFiles
accordingly.
- Around line 1585-1628: The client helpers updateProvider, testProviderModel,
startOpenAiOAuthBrowser, and openAiOAuthBrowserStatus call the old
routes/methods and must be changed to match the new server endpoints: change
updateProvider to POST to /providers (not PUT), change testProviderModel to POST
to /providers/test-model (not /providers/test), and change
startOpenAiOAuthBrowser and openAiOAuthBrowserStatus to use the new paths
/providers/openai/browser-oauth/start and /providers/openai/browser-oauth/status
(matching the server’s path segment ordering) while preserving JSON bodies and
response typing (Types.ProviderUpdateResponse, Types.ProviderModelTestResponse,
Types.OpenAiOAuthBrowserStartResponse, Types.OpenAiOAuthBrowserStatusResponse)
so the functions continue to return the same promise types.
In `@src/api/config.rs`:
- Around line 283-290: The endpoint documentation/response annotations for
update_agent_config (and any other handlers that call get_agent_config to build
responses) are missing the 404 response; update the OpenAPI/route annotations
for update_agent_config and the other annotated handlers that later call
get_agent_config so they include StatusCode::NOT_FOUND/HTTP 404 as a possible
response alongside 200/400/500 to reflect that get_agent_config (the function
that looks up state.runtime_configs and returns NOT_FOUND when the agent key is
missing) can produce a 404; ensure the annotations reference the same
AgentConfigResponse/error schema used for other statuses and adjust any doc
comments to mention the 404 case.
---
Duplicate comments:
In `@interface/src/api/client.ts`:
- Around line 206-211: AgentMessageEvent is missing a discriminant so it can't
be narrowed like the other SSE events; add a readonly literal property type:
"agent_message" (or the chosen event name consistent with other events) to the
AgentMessageEvent interface and then add AgentMessageEvent to the ApiEvent union
(where ApiEvent discriminates on the type field) so code can use
switch/type-narrowing on ApiEvent.type; also apply the same change to the other
similar event interfaces referenced (the block around the other event
definitions) so they all expose a consistent type discriminator.
In `@src/api/bindings.rs`:
- Around line 231-254: The create_binding, update_binding, and delete_binding
handlers perform read-parse-modify-write cycles on config.toml without acquiring
the shared write mutex, causing races; fix each handler (create_binding,
update_binding, delete_binding and the other similar blocks referenced) by
acquiring state.config_write_mutex.lock().await before starting the
read/parse/modify/write sequence and holding that lock until after the file has
been written and state updated so the entire critical section
(tokio::fs::read_to_string, parsing into toml_edit::DocumentMut, applying
changes, writing the file, and any in-memory state updates) is protected; ensure
you release the lock only after the write and any state updates succeed or after
handling/propagating errors.
- Around line 101-106: CreateBindingResponse.restart_required is currently
hardcoded false; update the code paths that construct CreateBindingResponse
(where the response is returned after creating/updating a binding) to compute
restart_required from the actual outcome (e.g., a boolean like
credentials_changed / platform_credentials_modified or adapter_restart_required
that you determine when saving/updating the binding). Locate the response
construction that instantiates CreateBindingResponse and replace the literal
false with that computed boolean, and ensure both create and update branches set
that flag consistently (derive the boolean from the logic that detects
added/changed platform credentials).
- Around line 701-711: The OpenAPI doc for the PUT /bindings endpoint claims a
404 for missing binding/config, but the handlers return HTTP 200 with { success:
false }; update the runtime to match the docs by returning an actual 404 when a
binding or its config is not found in the update_binding and delete_binding
handlers (and any other binding handlers referenced), or alternatively change
the utoipa response declarations to reflect the current 200/{ success: false }
behavior; modify the logic inside update_binding and delete_binding to call
HttpResponse::NotFound (or return Err(HttpError::NotFound) per your error type)
with an appropriate error body where you currently return success: false so the
OpenAPI contract and runtime behavior are consistent.
In `@src/api/providers.rs`:
- Around line 980-992: The delete-provider handler in providers.rs writes the
config but never refreshes runtime state or signals the provider setup channel,
so removed providers remain active until restart; after the config write in the
delete handler, call refresh_defaults_config(&state) (same as update_provider()
and finalize_openai_oauth()) and send/notify via provider_setup_tx (e.g.,
provider_setup_tx.send(()) or the existing notification mechanism) so runtime
state is updated before returning success.
- Around line 536-545: The documented response statuses in the utoipa
annotations (e.g., the OpenAiOAuthBrowserStart endpoint annotated with
OpenAiOAuthBrowserStartRequest/OpenAiOAuthBrowserStartResponse) do not match the
actual handler behavior (handlers currently return 200 with success: false for
validation/lookup failures and real 500s from
update_provider()/delete_provider()); update either the handlers or the
annotations to be consistent: either modify the handlers (functions like
update_provider, delete_provider and the OAuth start/complete handlers) to
return proper HTTP error responses (4xx for validation/missing/unknown provider,
500 for internal errors) or change the annotations for each affected endpoint
(the OpenAi OAuth endpoints and the endpoints around the other noted ranges) to
document 200 responses carrying error payloads, and ensure the generated client
and response types (e.g., OpenAiOAuthBrowserStartResponse) reflect the chosen
approach.
In `@src/api/server.rs`:
- Around line 32-44: ApiDoc currently only declares info and servers which
leaves protected routes marked as anonymous; update the OpenApi derive on the
ApiDoc struct to declare a bearer authentication scheme in components (e.g. a
security_scheme named "bearerAuth" of type http scheme "bearer" with
bearerFormat "JWT") and add a global security requirement referencing that
scheme (or apply it to specific operations) so that the protected endpoints
guarded by api_auth_middleware are marked as requiring bearerAuth in the
generated spec; modify the #[openapi(...)] attributes on ApiDoc accordingly to
include components.security_schemes and security entries.
In `@src/api/skills.rs`:
- Around line 364-377: The OpenAPI attrs for upload_skill need a
multipart/form-data request_body so the axum::extract::Multipart handler is
represented: update the #[utoipa::path] on upload_skill to include
request_body(...) with content for "multipart/form-data" that defines a schema
for the form object (e.g., a schema or inline object with a "file" property
type: string, format: binary and any other form fields your handler expects),
and reference that schema name in the attribute so the generated spec documents
the binary file field alongside the existing UploadSkillResponse, keeping
agent_id as a Query param.
In `@src/api/ssh.rs`:
- Around line 49-57: The OpenAPI annotations (utoipa::path) for the SSH
endpoints do not match actual handler behavior—update either the handlers or the
annotations so they align: for set_authorized_key (function set_authorized_key,
request/response types AuthorizedKeyRequest/AuthorizedKeyResponse) and
ssh_status (function ssh_status) decide whether invalid input should produce a
400/500 and implement that in the handler by returning proper HTTP responses
(e.g., return Err(HttpResponse::BadRequest(...)) or
HttpResponse::InternalServerError(...) where appropriate), or alternatively
remove the 400/500 entries from the utoipa::path response list so the attribute
only documents the statuses the handlers actually return (e.g., 200); make the
same change for all related utoipa::path blocks referenced in the diff.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 437618c8-a859-47b3-9769-d17cf71d7da5
⛔ Files ignored due to path filters (4)
Cargo.lockis excluded by!**/*.lock,!**/*.lockCargo.tomlis excluded by!**/*.tomlinterface/bun.lockis excluded by!**/*.lock,!**/*.lockinterface/package.jsonis excluded by!**/*.json
📒 Files selected for processing (62)
interface/src/api/client-typed.tsinterface/src/api/client.tsinterface/src/api/schema.d.tsinterface/src/api/types.tsinterface/src/components/ProfileAvatar.tsxinterface/src/components/Sidebar.tsxinterface/src/components/TaskBoard.tsxinterface/src/components/TopologyGraph.tsxinterface/src/hooks/useAudioRecorder.tsinterface/src/hooks/useTtsPlayback.tsinterface/src/routes/AgentDetail.tsxinterface/src/routes/AgentWorkers.tsxinterface/src/routes/ChannelDetail.tsxinterface/src/routes/GlobalTasks.tsxinterface/src/routes/Orchestrate.tsxjustfilesrc/agent/cortex.rssrc/agent/cortex_chat.rssrc/api.rssrc/api/agents.rssrc/api/bindings.rssrc/api/channels.rssrc/api/config.rssrc/api/cortex.rssrc/api/cron.rssrc/api/factory.rssrc/api/ingest.rssrc/api/links.rssrc/api/mcp.rssrc/api/memories.rssrc/api/messaging.rssrc/api/models.rssrc/api/projects.rssrc/api/providers.rssrc/api/secrets.rssrc/api/server.rssrc/api/settings.rssrc/api/skills.rssrc/api/ssh.rssrc/api/state.rssrc/api/system.rssrc/api/tasks.rssrc/api/tools.rssrc/api/webchat.rssrc/api/workers.rssrc/bin/openapi_spec.rssrc/config/types.rssrc/conversation/history.rssrc/conversation/worker_transcript.rssrc/cron/store.rssrc/factory/presets.rssrc/lib.rssrc/main.rssrc/mcp.rssrc/memory/store.rssrc/memory/types.rssrc/opencode/types.rssrc/projects/store.rssrc/secrets/store.rssrc/tasks/store.rssrc/tools/memory_save.rssrc/update.rs
✅ Files skipped from review due to trivial changes (24)
- interface/src/routes/ChannelDetail.tsx
- src/cron/store.rs
- interface/src/routes/Orchestrate.tsx
- src/factory/presets.rs
- src/projects/store.rs
- interface/src/components/ProfileAvatar.tsx
- src/mcp.rs
- src/update.rs
- src/conversation/worker_transcript.rs
- src/opencode/types.rs
- src/config/types.rs
- src/agent/cortex_chat.rs
- src/api/secrets.rs
- src/agent/cortex.rs
- src/conversation/history.rs
- interface/src/hooks/useAudioRecorder.ts
- src/api/workers.rs
- interface/src/hooks/useTtsPlayback.ts
- interface/src/api/client-typed.ts
- src/api/settings.rs
- src/api/cron.rs
- src/api/channels.rs
- src/api/projects.rs
- src/api/agents.rs
🚧 Files skipped from review as they are similar to previous changes (22)
- interface/src/routes/GlobalTasks.tsx
- src/api.rs
- src/memory/store.rs
- src/bin/openapi_spec.rs
- interface/src/components/Sidebar.tsx
- interface/src/routes/AgentWorkers.tsx
- src/secrets/store.rs
- src/main.rs
- src/api/factory.rs
- src/api/tasks.rs
- src/tasks/store.rs
- interface/src/components/TopologyGraph.tsx
- src/api/models.rs
- src/api/webchat.rs
- src/api/messaging.rs
- src/memory/types.rs
- src/api/system.rs
- src/api/cortex.rs
- src/api/mcp.rs
- src/api/memories.rs
- src/api/tools.rs
- interface/src/api/types.ts
Comment on lines
+111
to
+124
| #[utoipa::path( | ||
| post, | ||
| path = "/agents/ingest/files", | ||
| params( | ||
| ("agent_id" = String, Query, description = "Agent ID"), | ||
| ), | ||
| responses( | ||
| (status = 200, body = IngestUploadResponse), | ||
| (status = 400, description = "Invalid request"), | ||
| (status = 404, description = "Agent not found"), | ||
| (status = 500, description = "Internal server error"), | ||
| ), | ||
| tag = "ingest", | ||
| )] |
Contributor
There was a problem hiding this comment.
The upload handler still masks failure cases as success.
multipart.next_field() errors fall out of the loop on Line 145, and the insert on Lines 197-207 discards its Result, so this endpoint can return 200 with uploaded entries even when the request was not fully processed or the file never reached ingest history. Please surface or log those failures instead of treating them as success. As per coding guidelines "Don't silently discard errors. No let _ = on Results. Handle them, log them, or propagate them. The only exception is .ok() on channel sends where the receiver may be dropped."
Also applies to: 145-145, 197-207
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/api/ingest.rs` around lines 111 - 124, The upload handler currently
swallows multipart.next_field() errors and discards the Result from the DB
insert, causing 200 responses on partial/failed uploads; update the upload
handler to properly handle errors from multipart.next_field() inside the
processing loop (log the error and return a 400/500 response or propagate the
error) and remove the "let _ =" discard for the insert into ingest history—check
the Result from the insert call, log failures with context (e.g., file
name/agent_id) and return an appropriate error response instead of treating it
as success; ensure no Result is silently ignored anywhere in the multipart
processing or the insert logic so failures are surfaced to the caller.
This was referenced Mar 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Complete migration from hand-written TypeScript types to OpenAPI-generated types with full type safety.
Changes
Rust Backend
TypeScript Frontend
Build & CI
Gate Status
Verification
just typegen # Generate types
just check-typegen # Check types are up to date
Breaking Changes
None - all types backward compatible via re-exports from client.ts.
Residual