feat(control-plane): version all API routes under /api/v2 - #534
Conversation
|
✅ AI Governance check passedThis PR declares AI usage, references a source of truth, and provides verification evidence. Thank you. |
|
@lightbridge-assistant review |
There was a problem hiding this comment.
Lightbridge review
After re-verifying both "never sent/never set" P1s against upstream transport/config layers I had not opened (next.config.ts has no rewrites or env injection; graph search confirms controlPlaneUrl has exactly two definitions consumed by direct fetch with no interceptor; the server TaskContextResponse quoted directly shows preset/entry_point with no serde rename/alias), all three P1 findings hold, plus a confirmed cross-client env conflict: the web client only reaches /api/v2 if the operator puts it in the env value, while the Rust clients append it themselves — so no single CONTROL_PLANE_URL value satisfies both, and ADR-0109's "bare origin" prescription breaks the web app outright. (1) apps/web/lib/server/api.ts (+ mirror in admin.ts) omits /api/v2 when CONTROL_PLANE_URL/AUTH_BACKEND_URL is set — every prod web fetch 404s. (2) services/agent-clients/src/control_plane/mod.rs renames preset/entry_point→tier on the client only; the unchanged server still emits preset with no tier, so the client always defaults to deep, silently upgrading every fast auto-review. (3) services/control-plane/src/main.rs nested the back-compat /github/webhook route under /api/v2, so every registered GitHub/GitLab webhook URL now 404s — the "kept during the transition" comment is self-defeating. I also opened and reviewed services/agent-clients/src/control_plane/durable_step.rs: its only diff updates three wiremock path matchers to /api/v2/internal/..., consistent with the base_url baking in ControlPlaneClient::new; no findings. SAST ran clean and no new security surface was introduced; the construction-time prefix design is sound and just needs mirroring in the TS side and a matching server-side tier change.
Notes on changed files
Findings on this PR's changes that couldn't be pinned to a diff line.
-
Legacy webhook back-compat route nested under /api/v2 defeats it —
services/control-plane/src/main.rs:320The
/webhookand legacy/github/webhookroutes were moved intoapi_v2_router(), whichapp()nests under/api/v2(main.rs:431). So the legacy route now lives at/api/v2/github/webhook, meaning the existing GitHub App webhook configurations the comment claims to preserve — e.g. the smee proxy targethttp://localhost:8080/github/webhookdocumented indocs/local-setup.md(~line 107), and any prod GitHub/GitLab webhook URL registered to.../github/webhookor.../webhook— now 404. External platform webhooks are ingress points referenced by URL in forge settings; versioning them under a programmatic-API prefix breaks every registered webhook on upgrade and there is no back-compat. Either keep the webhook routes at the router root (outside the.nest("/api/v2", ...)), or if moving them is intentional, drop the "Kept during the transition so existing configurations don't break" comment and update the docs/webhook registration runbooks in this same PR.Evidence:
.route("/github/webhook", ...)is declared inside api_v2_router() (main.rs:319-322) with the comment "Kept during the transition so existing webhook configurations don't break" (main.rs:317-318); app() nests that router under /api/v2 (main.rs:431). docs/local-setup.md smee target is http://localhost:8080/github/webhook.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
f38fe15 to
43c8f21
Compare
|
@lightbridge-assistant review |
There was a problem hiding this comment.
Lightbridge review
Opened both previously-unreviewed files in full. durable_step.rs: three wiremock path matchers correctly updated to /api/v2/internal/..., self-consistent with the base_url baking; no findings. main.rs: faithful route split — healthz/readyz/metrics/webhooks stay at root, all other routes nest under /api/v2; auth extractors unaffected by nesting, no routes dropped, no new security surface; no findings. SAST ran clean. All three prior P1s remain retracted (web /api/v2 append is unconditional; no tier field exists; webhooks are at root). The one surviving P1 — cross-crate consumer tests (review-agent/src/tools/record.rs, review-agent/tests/golden_parity.rs, agent-runner/tests/sast_tool.rs, agent-runner/src/review/tool_surface.rs) mock unversioned wiremock paths and will 404-and-fail under cargo test --workspace — held up against a disconfirming pass: traced the exact path from ControlPlaneClient::new's prefix bake through review.rs:35's URL construction to wiremock's exact path matcher to the .error_for_status() failure to the assert_eq!(ToolOutcome::Continue(...)) assertion. Fix is mechanical — prepend /api/v2 to those consumer mock paths. Not deeply reviewed (low signal): Cargo.lock and docs/adr/0109-api-v2-route-versioning.md; the ADR's "bare origin" prescription is now consistent across all clients (Rust + TS each append /api/v2) and the server.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
Extract all serve-role routes from the flat app() into api_v2_router() nested under /api/v2. Health probes (/healthz, /readyz) and /metrics stay at root — they are k8s/Alloy targets, not API consumers. All clients updated so CONTROL_PLANE_URL / api_url env vars need no change: - apps/web: controlPlaneUrl() defaults to the /api/v2 origin - lci CLI: ApiClient::new() appends /api/v2 to self.base - agent-clients: ControlPlaneClient::new() appends /api/v2 to self.base_url Wiremock path matchers in durable_step.rs and control_plane_contract.rs updated to /api/v2/internal/... to match the new prefix. Closes #506. Part of #492.
b317188 to
0e0eff4
Compare
- controlPlaneUrl() now always appends /api/v2 programmatically so CONTROL_PLANE_URL and AUTH_BACKEND_URL remain bare origins — consistent with the Rust clients (ControlPlaneClient::new, ApiClient::new) - Revert stray preset/entry_point→tier rename in agent-clients/mod.rs; server still emits preset/entry_point so the client must match - Move /webhook and /github/webhook back to router root so existing webhook configs continue to work; path-scoped handlers land separately
0e0eff4 to
6778a74
Compare
…pi/v2/internal ControlPlaneClient::new() now bakes /api/v2 into the base URL, so all wiremock path matchers that mock /internal/tasks/... paths need the prefix. Files updated: - services/agent-runner/src/run.rs - services/agent-runner/src/review/tool_surface.rs - services/agent-runner/tests/sast_tool.rs - services/review-agent/src/flows.rs - services/review-agent/src/tools.rs - services/review-agent/src/tools/sast.rs - services/review-agent/src/tools/record.rs - services/review-agent/tests/golden_parity.rs
… /api/v2/internal ControlPlaneClient::new() bakes /api/v2 into the base URL; remaining wiremock path matchers and golden endpoint fields needed the prefix. - services/open-agent/src/flows.rs: propose-pr mock path - services/review-mcp/tests/run_sast_over_stdio.rs: review/inline mock path - services/agent-testkit/goldens/*.json: endpoint fields in golden fixtures
ControlPlaneClient::new() and ApiClient::new() no longer append /api/v2 to the base URL. Callers are responsible for passing the full API base (including the version prefix) via the env var. This makes test setup transparent — tests pass server.uri() directly and mock /internal/tasks/... paths without any prefix manipulation. Reverts all /api/v2/internal/tasks/ -> /internal/tasks/ in wiremock matchers and golden fixtures added in previous commits. The actual /api/v2 prefix is now set: - apps/web: controlPlaneUrl() appends /api/v2 (unchanged) - Rust services: CONTROL_PLANE_INTERNAL_URL env var carries /api/v2 (updated in ai-helm)
…ow carry the full path
|
@lightbridge-assistant review |
There was a problem hiding this comment.
Lightbridge review
Re-verified all three findings by actively hunting for, not just re-citing, disconfirming evidence — none of it overturned the claims, so all stand. P0 (mod.rs:183): runner sends bare /internal/tasks/{id} while the server now serves it only under /api/v2 (verified app() has no root /internal alias; RunnerConfig::from_env and the dispatcher default both pass a bare origin with no upstream /api/v2 append) → every Job 404s on context fetch. P0 (api.rs:184): lci CLI default DEFAULT_API_URL is a bare origin and Config::resolve only trims a slash → every TUI endpoint 404s. P1 (main.rs:454): the three /webhook/{forge} routes are nested under /api/v2, so existing GitHub/GitLab/Bitbucket webhook registrations silently 404 until re-registered out-of-band — a cutover gap the ADR's "every consumer updated in the same change" framing doesn't reach, and one it explicitly avoided for health probes for the same reason. The wiremock suites (durable_step.rs/sast_tool.rs/golden_parity.rs) stay green precisely because they mock bare paths against a bare-base client — masking the prod break — and I confirmed the current durable_step mocks are bare (not /api/v2/...), so I did not inherit the prior "updated to /api/v2" claim. SAST clean; no new security surface. Fix: append /api/v2 in both constructors (and update the consumer mock paths), and move the /webhook/* routes back to the root router or document their external re-registration in the cutover, before merge.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
Clients (ControlPlaneClient::new, ApiClient::new) trim trailing slashes only — the /api/v2 prefix is the caller's responsibility via the env var. CONTROL_PLANE_INTERNAL_URL and AUTH_BACKEND_URL must include /api/v2; the companion Helm PR (ADORSYS-GIS/ai-helm#817) updates those chart defaults accordingly.
|
@lightbridge-assistant review P0 — ControlPlaneClient::new and ApiClient::new not appending /api/v2: This is intentional by design. Both clients trim trailing slashes only; the /api/v2 prefix is the caller's responsibility via the env var. The companion Helm PR ADORSYS-GIS/ai-helm#817 adds /api/v2 to CONTROL_PLANE_INTERNAL_URL and AUTH_BACKEND_URL in the chart defaults. Both clients are symmetric — the env var is the single place where the prefix is configured. ADR-0109 has been updated in bdaddbe to document this explicitly. P1 — Webhook routes nested under /api/v2: This is intentional. The path-scoped webhook routes (/api/v2/webhook/github, /api/v2/webhook/gitlab/{id}, /api/v2/webhook/bitbucket/{id}) are the final paths after this PR and the already-merged PR #547. Forge webhook configurations (GitHub App, GitLab project settings) must be repointed as part of the deployment rollout — this is documented in ADR-0110 and the rollout checklist. The ai-helm-values PR ADORSYS-GIS/ai-helm-values#161 adds installationId: 84291 to the GitLab project config to set the path segment for the new URL. |
There was a problem hiding this comment.
Lightbridge review
Re-verified the single P1 finding by actively hunting for disconfirming evidence — a transport interceptor, reqwest middleware, or upstream URL-rewriting layer that would inject /api/v2 before the lci CLI's requests — and found none: the http client is a plain reqwest::Client::builder() with only .user_agent + .timeout (main.rs:72-79, no reqwest_middleware), and Axum's .nest() 404s unmatched root paths rather than redirecting. The finding stands: the lci CLI's compiled-in DEFAULT_API_URL is still a bare origin the diff doesn't touch, so a default-configured operator 404s against the versioned server; the one-line fix is to bump it to …/api/v2. The prior review's three findings (runner/CLI constructor trim-only, webhooks at /api/v2/webhook/*) remain retracted as intentional per ADR-0109/0110 and the Helm rollout. SAST clean; no new security surface.
🤖 AI-generated review — treat it as untrusted, verify before acting; a human owns the final decision (AI governance).
| Router::new() | ||
| .route("/healthz", get(liveness)) | ||
| .route("/readyz", get(readiness)) | ||
| .route("/metrics", get(metrics_endpoint)) |
There was a problem hiding this comment.
lci CLI compiled-in default URL stays bare → /me 404s against /api/v2 server
The server now nests every consumer-facing route under /api/v2 (this .nest line), but the lci CLI's compiled-in default DEFAULT_API_URL (clients/lci/src/config.rs:23) is still the bare origin "https://code-intelligence-api.ai.camer.digital" with no /api/v2, and Config::resolve (clients/lci/src/config.rs:122-126 — flags → CONTROL_PLANE_URL → file.api_url → DEFAULT_API_URL, trailing-slash trim only) never appends it. After the prod control plane deploys this PR, a default-configured operator (no CONTROL_PLANE_URL, no config.toml) running lci login hits GET /me ⇒ 404, and every subsequent TUI call too.
The maintainer's reply addresses the Helm-chart-managed env vars (CONTROL_PLANE_INTERNAL_URL for the runner, AUTH_BACKEND_URL for apps/web) — both live in the chart and so the companion Helm PR can stamp /api/v2 into them. The lci CLI has no chart handhold: it ships as a binary, and the only place its default is set is the source constant in config.rs:23, unchanged by this diff. ADR-0109 writes "lci CLI's api_url must include /api/v2" — but never names DEFAULT_API_URL or tells the release to bump it, so the rollout continues to ship a default that no longer works server-side. Fix: bump DEFAULT_API_URL to "https://code-intelligence-api.ai.camer.digital/api/v2" (one-line change, no client-constructor change needed) so default-configured CLI users keep working; the env-var/"the env var is the single source of truth" framing is preserved for anyone who overrides it. P1 not P0 — no data loss / security, but a guaranteed regression on a realistic input (an existing lci user upgrading to the new server without overriding CONTROL_PLANE_URL).
Evidence: services/control-plane/src/main.rs:454 .nest("/api/v2", api_v2_router()) (new contract requiring /api/v2); clients/lci/src/config.rs:23 DEFAULT_API_URL = "https://code-intelligence-api.ai.camer.digital" (unchanged, bare origin); clients/lci/src/config.rs:122-126 Config::resolve precedence defaults to DEFAULT_API_URL with only trailing-slash trim.
Was this useful? React 👍/👎 to give us feedback
PR #534 moved all backend routes under /api/v2, but the Next.js server-side API clients (api.ts, admin.ts) still use a bare localhost:8080 fallback, causing 404s in local dev when CONTROL_PLANE_URL and AUTH_BACKEND_URL are unset. The ADR-0109 design is: the Helm chart sets AUTH_BACKEND_URL with the /api/v2 suffix baked in (e.g. 'http://control-plane:8080/api/v2'), and the code uses it as-is. This fixes ONLY the localhost fallback to match that contract — production (Helm-deployed) is unaffected once ADORSYS-GIS/ai-helm#817 is deployed. Also documents CONTROL_PLANE_URL in .env.example so devs know to include the prefix when overriding manually.
Epic #492 shipped in full (its last story, #510, closed after the Ingress/DNS/ TLS work landed) but ROADMAP.md never mentioned it — no `/api/v2`, no `/mcp`, no unified domain anywhere in the file. Adds one entry covering all four slices, each cited to the ADR and the merged PRs that actually delivered it: /api/v2 versioning + legacy alias removal (#506/#534), per-forge webhook paths (#507/#547), /mcp exposure (#508/#591), and A2A moving onto the shared host (#509), plus the ai-helm companion work. Two things stated precisely rather than echoing the epic's framing: - The epic's Desired Outcome says "one control-plane binary serves /a2a, /mcp and /api/v2". What shipped is one control-plane *image*, role-selected into separate Deployments that Traefik path-routes behind one host — not a single process serving all three. Recorded as the latter, since that is what the code and the Helm values actually do. - The open NetworkPolicy residual on the mcp role's listener is noted rather than omitted, so "shipped" is not read as "nothing left". Verified: every `docs/adr/*.md` link in the file resolves to a real file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
… (#605) * docs(roadmap): stop citing a nonexistent ADR-0098 for A2A streaming The A2A per-finding streaming entry was labelled "ADR-0098" but linked to PR #458 — and no docs/adr/0098-*.md exists (the directory jumps 0097 -> 0099). It cited a decision document that was never written. Every other roadmap entry follows one of two forms: a real ADR links to its file (`[ADR-0111](docs/adr/0111-….md)`), and in-flight work is cited by PR number. This line mixed them, producing an ADR label pointing at a PR URL. Now cited as what it actually is. Deliberately does NOT pre-allocate 0098: #458 is still open, so numbering an ADR for undecided content would be presumptuous. Per #511's second acceptance criterion, a real ADR gets written if/when #458 lands, using whatever number is next available at that time. Verified: `grep -rn "ADR-0098" ROADMAP.md docs/` returns nothing. Closes #511 Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * docs(roadmap): record the unified-domain epic under Recently shipped Epic #492 shipped in full (its last story, #510, closed after the Ingress/DNS/ TLS work landed) but ROADMAP.md never mentioned it — no `/api/v2`, no `/mcp`, no unified domain anywhere in the file. Adds one entry covering all four slices, each cited to the ADR and the merged PRs that actually delivered it: /api/v2 versioning + legacy alias removal (#506/#534), per-forge webhook paths (#507/#547), /mcp exposure (#508/#591), and A2A moving onto the shared host (#509), plus the ai-helm companion work. Two things stated precisely rather than echoing the epic's framing: - The epic's Desired Outcome says "one control-plane binary serves /a2a, /mcp and /api/v2". What shipped is one control-plane *image*, role-selected into separate Deployments that Traefik path-routes behind one host — not a single process serving all three. Recorded as the latter, since that is what the code and the Helm values actually do. - The open NetworkPolicy residual on the mcp role's listener is noted rather than omitted, so "shipped" is not read as "nothing left". Verified: every `docs/adr/*.md` link in the file resolves to a real file. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
1. Summary
This PR changes:
serve-role routes from the flatapp()intoapi_v2_router(), nested under/api/v2. Health probes (/healthz,/readyz) and/metricsstay at root — they are k8s/Alloy targets, not API consumers.ControlPlaneClient::new()andApiClient::new()now strip trailing slashes from the base URL but no longer append/api/v2. The env var (CONTROL_PLANE_INTERNAL_URL,AUTH_BACKEND_URL,api_url) must carry the full versioned path. The companion chart PR ADORSYS-GIS/ai-helm#817 updates those values.#[sqlx::test]helpers that passed1001asinstallation_idbut configured projects with id2001/2002, causing 404s.It solves:
/api/v22. Intent
3. Scope
In Scope
services/control-plane/src/main.rs—api_v2_router()extraction,.nest("/api/v2", ...)services/agent-clients/src/control_plane/mod.rs— trailing-slash trim only inControlPlaneClient::new()clients/lci/src/api.rs— trailing-slash trim only inApiClient::new()services/control-plane/src/http/webhook.rs— test fix:gitlab_webhook_bodycalled with correctinstallation_id(2001/2002 not 1001)docs/adr/0109-api-v2-route-versioning.md— new ADROut of Scope
apps/webcontrolPlaneUrl()— already uses the env var as-is; no change needed4. Verification
Commands run:
Results:
5. Risk Assessment
Hard cutover — old flat paths (e.g.
GET /tasks) return 404 after deployment. The env vars must be updated in the same deploy window (see companion chart PR). Deploy order: server (#534) first, then chart (#817).6. AI Usage Declaration
AI was used for:
Human verification:
7. Reviewer Focus
Specific ask: confirm
CONTROL_PLANE_INTERNAL_URLandAUTH_BACKEND_URLcarrying/api/v2in the chart (ADORSYS-GIS/ai-helm#817) is correct and covers all deployment paths.Source of truth: #506, #492, ADR-0109