feat(api,sdk,cli): add unauthenticated version endpoint#1602
Merged
Conversation
Add GET /api/ambient/v1/version returning build metadata (git SHA, build time, git tag) without requiring authentication. Uses pre-auth middleware pattern from the proxy plugin with a pre-marshaled JSON response for zero per-request allocation. Full-stack implementation: - ambient-api-server: version plugin with RegisterPreAuthMiddleware - Makefile/Dockerfile: inject git_tag via ldflags and build-args - Go SDK: Client.ServerVersion() + standalone FetchServerVersion() - Python SDK: fetch_server_version() with ServerVersion dataclass - acpctl: version command now shows client and server version Closes #1598 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Spec covering the unauthenticated /version endpoint design: pre-auth middleware pattern, pre-marshaled response, full-stack implementation units (api-server, Makefile, Dockerfile, Go SDK, Python SDK, CLI). Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
📝 WalkthroughWalkthroughThis PR implements an unauthenticated ChangesVersion Endpoint Implementation
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
✅ Deploy Preview for cheerful-kitten-f556a0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
![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: 3
🧹 Nitpick comments (1)
components/ambient-sdk/python-sdk/ambient_platform/_version_api.py (1)
16-21: ⚡ Quick winMake
from_dict’s input type more precise (API contract)
from_dictcurrently acceptsdata: dict, which drops key/value types. Use a typed mapping (e.g.,Mapping[str, Any]) and adjust imports.Proposed fix
-from typing import Optional +from typing import Any, Mapping @@ - def from_dict(cls, data: dict) -> ServerVersion: + def from_dict(cls, data: Mapping[str, Any]) -> ServerVersion:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-sdk/python-sdk/ambient_platform/_version_api.py` around lines 16 - 21, Change the from_dict signature to accept a precise typed mapping instead of plain dict: update ServerVersion.from_dict to def from_dict(cls, data: Mapping[str, Any]) -> ServerVersion and add the required imports (Mapping, Any) from typing; keep the body unchanged but rely on the typed input to reflect the API contract.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ambient-api-server/plugins/version/plugin.go`:
- Around line 17-27: The code currently discards errors from json.Marshal when
building responseBytes and from w.Write when serving the version endpoint;
update the versionResponse creation and the handler returned by
pkgserver.RegisterPreAuthMiddleware to check and handle both errors: capture the
error returned by json.Marshal(versionResponse{...}) (e.g., log it and set
responseBytes to a safe fallback or abort startup) and capture the error from
w.Write(responseBytes) inside the handler and log/handle it and, if appropriate,
write an HTTP 500 or stop further writes; modify the symbols responseBytes,
json.Marshal call, and the http.HandlerFunc returned by
pkgserver.RegisterPreAuthMiddleware (the handler that checks r.Method and
versionPath) to perform these error checks and logging.
In `@components/ambient-cli/cmd/acpctl/version/cmd.go`:
- Around line 24-30: The current code silently returns when err != nil and when
apiURL == "" which discards the failure context; update the error paths in the
version command handler (the function containing the err check and apiURL :=
cfg.GetAPIUrl()) to return or propagate a descriptive error instead of a naked
return (e.g., wrap and return the original err when cfg retrieval fails, and
return a specific error when apiURL is empty), so callers see why server-version
lookup failed and can log or surface the cause.
- Line 37: Replace the user-facing printf that prints the raw error (the
fmt.Fprintf call writing to cmd.OutOrStdout() with "Server: unavailable (%v)\n",
err) with a generic message such as "Server: unavailable\n" for stdout, and send
the detailed err to a non-user-facing sink (e.g., cmd.ErrOrStderr() or existing
logger) so internal/transport details are not leaked; keep the generic message
in the output and log the full error separately for debugging.
---
Nitpick comments:
In `@components/ambient-sdk/python-sdk/ambient_platform/_version_api.py`:
- Around line 16-21: Change the from_dict signature to accept a precise typed
mapping instead of plain dict: update ServerVersion.from_dict to def
from_dict(cls, data: Mapping[str, Any]) -> ServerVersion and add the required
imports (Mapping, Any) from typing; keep the body unchanged but rely on the
typed input to reflect the API contract.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Enterprise
Run ID: b63dfead-2645-4231-a8b3-a3b3e5cb723a
📒 Files selected for processing (9)
components/ambient-api-server/Dockerfilecomponents/ambient-api-server/Makefilecomponents/ambient-api-server/cmd/ambient-api-server/main.gocomponents/ambient-api-server/pkg/api/api.gocomponents/ambient-api-server/plugins/version/plugin.gocomponents/ambient-cli/cmd/acpctl/version/cmd.gocomponents/ambient-sdk/go-sdk/client/version_api.gocomponents/ambient-sdk/python-sdk/ambient_platform/_version_api.pydocs/plans/2026-05-20-001-feat-version-endpoint-plan.md
Comment on lines
+17
to
+27
| responseBytes, _ = json.Marshal(versionResponse{ | ||
| Version: localapi.Version, | ||
| BuildTime: localapi.BuildTime, | ||
| GitTag: localapi.GitTag, | ||
| }) | ||
|
|
||
| pkgserver.RegisterPreAuthMiddleware(func(next http.Handler) http.Handler { | ||
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method == http.MethodGet && strings.TrimSuffix(r.URL.Path, "/") == versionPath { | ||
| w.Header().Set("Content-Type", "application/json") | ||
| _, _ = w.Write(responseBytes) |
Contributor
There was a problem hiding this comment.
Handle the marshal/write error paths instead of discarding them.
Line 17 and Line 27 both ignore errors. Please collect/log these explicitly so failures are not silent in this unauthenticated path.
Suggested fix
import (
"encoding/json"
+ "log"
"net/http"
"strings"
@@
func init() {
- responseBytes, _ = json.Marshal(versionResponse{
+ var err error
+ responseBytes, err = json.Marshal(versionResponse{
Version: localapi.Version,
BuildTime: localapi.BuildTime,
GitTag: localapi.GitTag,
})
+ if err != nil {
+ log.Printf("version plugin: failed to marshal version payload: %v", err)
+ responseBytes = []byte(`{"version":"","build_time":"","git_tag":""}`)
+ }
@@
- _, _ = w.Write(responseBytes)
+ if _, err := w.Write(responseBytes); err != nil {
+ log.Printf("version plugin: failed to write response: %v", err)
+ }
return
}As per coding guidelines, "Never silently swallow partial failures; every error path must propagate or be explicitly collected, never discarded".
📝 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
| responseBytes, _ = json.Marshal(versionResponse{ | |
| Version: localapi.Version, | |
| BuildTime: localapi.BuildTime, | |
| GitTag: localapi.GitTag, | |
| }) | |
| pkgserver.RegisterPreAuthMiddleware(func(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if r.Method == http.MethodGet && strings.TrimSuffix(r.URL.Path, "/") == versionPath { | |
| w.Header().Set("Content-Type", "application/json") | |
| _, _ = w.Write(responseBytes) | |
| var err error | |
| responseBytes, err = json.Marshal(versionResponse{ | |
| Version: localapi.Version, | |
| BuildTime: localapi.BuildTime, | |
| GitTag: localapi.GitTag, | |
| }) | |
| if err != nil { | |
| log.Printf("version plugin: failed to marshal version payload: %v", err) | |
| responseBytes = []byte(`{"version":"","build_time":"","git_tag":""}`) | |
| } | |
| pkgserver.RegisterPreAuthMiddleware(func(next http.Handler) http.Handler { | |
| return http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { | |
| if r.Method == http.MethodGet && strings.TrimSuffix(r.URL.Path, "/") == versionPath { | |
| w.Header().Set("Content-Type", "application/json") | |
| if _, err := w.Write(responseBytes); err != nil { | |
| log.Printf("version plugin: failed to write response: %v", err) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-api-server/plugins/version/plugin.go` around lines 17 -
27, The code currently discards errors from json.Marshal when building
responseBytes and from w.Write when serving the version endpoint; update the
versionResponse creation and the handler returned by
pkgserver.RegisterPreAuthMiddleware to check and handle both errors: capture the
error returned by json.Marshal(versionResponse{...}) (e.g., log it and set
responseBytes to a safe fallback or abort startup) and capture the error from
w.Write(responseBytes) inside the handler and log/handle it and, if appropriate,
write an HTTP 500 or stop further writes; modify the symbols responseBytes,
json.Marshal call, and the http.HandlerFunc returned by
pkgserver.RegisterPreAuthMiddleware (the handler that checks r.Method and
versionPath) to perform these error checks and logging.
Comment on lines
+24
to
+30
| if err != nil { | ||
| return | ||
| } | ||
| apiURL := cfg.GetAPIUrl() | ||
| if apiURL == "" { | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Do not silently drop server-version failure states
Line 24 and Line 28 return without surfacing why server version is missing. This hides partial failures and makes troubleshooting harder.
Suggested fix
cfg, err := config.Load()
if err != nil {
+ fmt.Fprintln(cmd.OutOrStdout(), "Server: unavailable (config not loaded)")
return
}
apiURL := cfg.GetAPIUrl()
if apiURL == "" {
+ fmt.Fprintln(cmd.OutOrStdout(), "Server: unavailable (API URL not configured)")
return
}As per coding guidelines, "**/*.go: Never silently swallow partial failures; every error path must propagate or be explicitly collected, never discarded".
📝 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
| if err != nil { | |
| return | |
| } | |
| apiURL := cfg.GetAPIUrl() | |
| if apiURL == "" { | |
| return | |
| } | |
| if err != nil { | |
| fmt.Fprintln(cmd.OutOrStdout(), "Server: unavailable (config not loaded)") | |
| return | |
| } | |
| apiURL := cfg.GetAPIUrl() | |
| if apiURL == "" { | |
| fmt.Fprintln(cmd.OutOrStdout(), "Server: unavailable (API URL not configured)") | |
| return | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-cli/cmd/acpctl/version/cmd.go` around lines 24 - 30, The
current code silently returns when err != nil and when apiURL == "" which
discards the failure context; update the error paths in the version command
handler (the function containing the err check and apiURL := cfg.GetAPIUrl()) to
return or propagate a descriptive error instead of a naked return (e.g., wrap
and return the original err when cfg retrieval fails, and return a specific
error when apiURL is empty), so callers see why server-version lookup failed and
can log or surface the cause.
|
|
||
| sv, err := sdkclient.FetchServerVersion(ctx, apiURL, cfg.InsecureTLSVerify) | ||
| if err != nil { | ||
| fmt.Fprintf(cmd.OutOrStdout(), "Server: unavailable (%v)\n", err) |
Contributor
There was a problem hiding this comment.
Avoid printing raw errors in user-facing version output
Line 37 includes %v from the fetch error directly in stdout. That can leak endpoint/internal transport details; prefer a generic message for end users.
Suggested fix
- fmt.Fprintf(cmd.OutOrStdout(), "Server: unavailable (%v)\n", err)
+ fmt.Fprintln(cmd.OutOrStdout(), "Server: unavailable")
returnAs per coding guidelines, "**/*.go: Never log, error, or return tokens directly in responses; use len(token) for logging and provide generic messages to users".
📝 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
| fmt.Fprintf(cmd.OutOrStdout(), "Server: unavailable (%v)\n", err) | |
| fmt.Fprintln(cmd.OutOrStdout(), "Server: unavailable") | |
| return |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-cli/cmd/acpctl/version/cmd.go` at line 37, Replace the
user-facing printf that prints the raw error (the fmt.Fprintf call writing to
cmd.OutOrStdout() with "Server: unavailable (%v)\n", err) with a generic message
such as "Server: unavailable\n" for stdout, and send the detailed err to a
non-user-facing sink (e.g., cmd.ErrOrStderr() or existing logger) so
internal/transport details are not leaked; keep the generic message in the
output and log the full error separately for debugging.
Contributor
Merge Queue Status
This pull request spent 57 seconds in the queue, including 15 seconds running CI. Required conditions to merge |
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.
This PR uses the spec I generated off #1598
Summary
GET /api/ambient/v1/versionreturning build metadata (version,build_time,git_tag) without authenticationacpctl versionclient+server outputdocs/plans/2026-05-20-001-feat-version-endpoint-plan.mdTest plan
cd components/ambient-api-server && go build ./... && go vet ./...cd components/ambient-sdk/go-sdk && go build ./...cd components/ambient-cli && go build ./...python3 -c "from ambient_platform._version_api import fetch_server_version"curl http://localhost:8000/api/ambient/v1/versionreturns JSON without authacpctl versionshows both client and server linesCloses #1598
🤖 Generated with Claude Code
Summary by CodeRabbit
/api/ambient/v1/versionendpoint that exposes version, build time, and git tag information as JSONacpctl versioncommand to fetch and display both client and server version information with build metadata