Skip to content

Fix OpenAPI conversation action paths#760

Open
lloydmak99 wants to merge 2 commits into
mainfrom
fix/openapi-conversation-v1-prefix
Open

Fix OpenAPI conversation action paths#760
lloydmak99 wants to merge 2 commits into
mainfrom
fix/openapi-conversation-v1-prefix

Conversation

@lloydmak99

Copy link
Copy Markdown
Contributor

Problem

Part of #650.

The generated OpenAPI spec documented five conversation action operations without the /v1 prefix:

  • POST /conversations/{conversation_id}/archive
  • DELETE /conversations/{conversation_id}/archive
  • POST /conversations/{conversation_id}/pin
  • DELETE /conversations/{conversation_id}/pin
  • POST /conversations/{conversation_id}/clone

Those unprefixed paths return bare 404s at runtime. The actual router mounts the handlers under /v1, so generated clients call the wrong URLs for archive, unarchive, pin, unpin, and clone.

Change

  • Updated the utoipa::path annotations for the conversation action handlers to document the runtime /v1/conversations/... paths.
  • Added an OpenAPI regression test that asserts:
    • the prefixed action paths are present in the generated spec
    • the unprefixed variants are absent

What Else Is Needed

Tests

  • cargo test -p api --lib openapi
  • cargo fmt --all -- --check
  • cargo clippy --all-targets --all-features -- -D warnings
  • cargo test --lib --bins

@lloydmak99 lloydmak99 temporarily deployed to Cloud API test env June 10, 2026 20:08 — with GitHub Actions Inactive

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request updates several OpenAPI paths in the conversations routes to include the /v1 prefix, specifically for the pin, unpin, archive, unarchive, and clone endpoints. Additionally, a unit test has been added to verify that these paths are correctly prefixed and that the old paths without the prefix are absent. There are no review comments, and I have no feedback to provide.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

@claude

claude Bot commented Jun 10, 2026

Copy link
Copy Markdown

Review — Fix OpenAPI conversation action paths

Focused, correct fix. Verified the claims:

  • All other conversation endpoints in conversations.rs already document the /v1/conversations prefix (create, batch, get, update, delete, items). The five action handlers (pin/unpin/archive/unarchive/clone) were the only outliers using unprefixed paths — confirming the spec/runtime mismatch.
  • The handlers are mounted under the /v1 nest in lib.rs, so the corrected annotations now match the actual runtime routes.
  • The regression test asserts both the presence of prefixed paths and the absence of unprefixed ones, guarding against future regressions.

No critical issues found.

Minor (non-blocking): the test checks three path keys, which is sufficient since the POST/DELETE pairs for archive and pin share the same path string — a one-line comment noting that would aid future readers, but it's not required. Scope is appropriately narrow; the broader #650 cleanup is correctly deferred to separate PRs.

✅ Approved

@lloydmak99 lloydmak99 temporarily deployed to Cloud API test env June 10, 2026 20:16 — with GitHub Actions Inactive

@Evrard-Nil Evrard-Nil left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed thoroughly with a local build + spec dump. LGTM — approving.

Verified the core claim is correct:

  • Action handlers are registered without /v1 in build_conversation_routes() (crates/api/src/lib.rs:1230-1239: .route("/conversations/{conversation_id}/pin", post(pin).delete(unpin)), .../archive, .../clone), and that router is merged under .nest("/v1", …) at lib.rs:930. So the true runtime path is /v1/conversations/{conversation_id}/{archive,clone,pin} — exactly the new annotation strings (param name {conversation_id} matches too). The pre-fix bare /conversations/... annotations did 404 generated clients. ✅

No double-prefix: ApiDoc (openapi.rs) has no servers/nest/path-prefix and its only modifier (SecurityAddon) never touches paths. Dumped the real ApiDoc::openapi() spec — renders single /v1/conversations/..., no /v1/v1. ✅

Consistency: all 12 utoipa annotations in conversations.rs now use /v1/; the other 7 already did, these 5 ops (3 unique path keys) were the last stragglers. Grepped every route file for path = "/ excluding /v1 → zero matches org-wide. Nothing missed. ✅

Build/test: cargo test -p api --lib openapi → 3 passed (incl. the new test_openapi_conversation_action_paths_use_v1_prefix, which builds the real spec and asserts prefixed-present + bare-absent). cargo fmt --all -- --check clean. cargo clippy -p api --all-targets -- -D warnings clean. ✅

Minor (non-blocking, out of scope): the regression test keys on path strings, not HTTP methods — archive+pin each carry POST+DELETE under one key, so all 5 ops are covered, but a hypothetical regression that kept the path key while dropping the DELETE op would slip through. Extremely low risk; fine to leave as-is.

@PierreLeGuen PierreLeGuen left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This PR correctly fixes the five utoipa::path annotations in crates/api/src/routes/conversations.rs (pin, unpin, archive, unarchive, clone) that were missing the /v1 prefix, and adds a regression test.

Correctness verified:

  • The router registers these handlers at relative paths like /conversations/{conversation_id}/pin in build_conversation_routes (crates/api/src/lib.rs:1229-1240), nested under /v1 (crates/api/src/lib.rs:931), so the runtime paths are indeed /v1/conversations/... and the previous spec paths were wrong.
  • The OpenAPI doc sets no servers base URL or path-modifying modifier (crates/api/src/openapi.rs:303; SecurityAddon only adds security schemes), so annotations must carry the full /v1 prefix — no /v1/v1 double-prefix risk.
  • All 12 utoipa path annotations in conversations.rs now consistently use /v1/, and a repo-wide grep confirms no remaining unprefixed path = "..." annotations in the api crate — the fix is complete, not partial.
  • The new test asserts both the presence of the /v1-prefixed path keys and the absence of the unprefixed variants, guarding against regression. Pin/archive each share one path key for POST and DELETE, which the test comment correctly notes.

Scope exclusions (duplicate model list surfaces, 422 envelopes from #650) are documented in the PR body as follow-ups, which is reasonable. No privacy/logging concerns — the change touches only doc annotations and a test.

Local checks run:

  • cargo test -p api --lib openapi — 3 passed, including the new test_openapi_conversation_action_paths_use_v1_prefix
  • cargo test --lib --bins — 742 passed, 1 ignored (pre-existing), 0 failed
  • cargo clippy -p api --all-targets -- -D warnings — clean
  • cargo fmt --all -- --check — clean
  • git diff --check — clean
  • CI on the PR (Lint, Test Suite, security_audit) is green; integration/vLLM paths weren't run locally as they need services/secrets.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants