refactor: SDK changes to support new API#746
refactor: SDK changes to support new API#746markturansky wants to merge 1 commit intoambient-code:mainfrom
Conversation
markturansky
changed the title
This comment has been minimized.
This comment has been minimized.
Claude Code ReviewSummaryThis PR refactors the Ambient SDK to update the API base path from Issues by Severity🚫 Blocker Issues1. Unresolved merge conflict in Lines 9 and 22 in the diff contain raw The build is broken as-is. The old version ( 2. Python gRPC watch imports a non-existent module
from . import _grpc_stubs # This would be generatedThe comment itself admits this module does not exist. Any call to the gRPC watch path will fail at runtime with 3. Binary executables committed to git Two compiled binaries are tracked:
The PR description says 🔴 Critical Issues4. Immediate context cancellation bug in Go if opts.Timeout > 0 {
var cancel context.CancelFunc
streamCtx, cancel = context.WithTimeout(streamCtx, opts.Timeout)
defer cancel() // fires when Watch() returns — not when the stream ends
}
stream, err := client.WatchSessions(streamCtx, &ambient_v1.WatchSessionsRequest{})
// ...
return watcher, nil
// defer cancel() fires HERE, immediately cancelling streamCtxThe gRPC stream is created with Fix: store the cancel in the 5. watch(opts?: WatchOptions): SessionWatcher {
return new SessionWatcher(this.config); // opts is dropped entirely
}
6.
this.eventSource = new EventSource(url);The Use 🟡 Major Issues7. Busy-wait polling loop in TypeScript ( while (!finished && !this.closed) {
if (eventQueue.length > 0) {
yield eventQueue.shift()!;
} else {
await new Promise(resolve => setTimeout(resolve, 10)); // polls every 10ms
}
}This spins at 100 iterations per second when idle. For a long-running watch (default 30 min), that is millions of wasted microtask evaluations. Use a proper Promise-based notification mechanism (e.g., a Deferred or ReadableStream backed by the event listener). 8. Duplicate class definitions across Python watch modules Both 9. Missing newlines at end of file (pre-commit will block) The pre-commit
10. Deprecated gRPC dial API ( conn, err := grpc.DialContext(ctx, grpcAddr,
grpc.WithTransportCredentials(creds),
grpc.WithBlock(),
grpc.FailOnNonTempDialError(true),
)
11. Go module path inconsistency in The module path is 🔵 Minor Issues12. function handleWatchEvent(event: any) {Violates the zero- 13. Monkey-patching def add_watch_to_session_api():
"""Monkey patch to add watch method to SessionAPI."""
from ._session_api import SessionAPI
SessionAPI.watch = watchThis function is defined but never called. The watch method will not be available on 14. Empty TLS config in Go gRPC connection
15. if !strings.Contains(addr, ":") {
addr += ":4434"
}IPv6 addresses like Positive Highlights
Recommendations
🤖 Generated with Claude Code 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
markturansky
force-pushed
the
feat/ambient-sdk
branch
2 times, most recently
from
e46d11a to
6fae61f
Compare
|
test comment |
Ambient Code PlatformKubernetes-native AI automation platform that orchestrates agentic sessions through containerized microservices. Built with Go (backend, operator), NextJS + Shadcn (frontend), Python (runner), and Kubernetes CRDs.
Structure
Key Files
Session FlowCommandsmake build-all # Build all container images
make deploy # Deploy to cluster
make test # Run tests
make lint # Lint code
make kind-up # Start local Kind cluster
make test-e2e-local # Run E2E tests against KindPer-Component# Backend / Operator (Go)
cd components/backend && gofmt -l . && go vet ./... && golangci-lint run
cd components/operator && gofmt -l . && go vet ./... && golangci-lint run
# Frontend
cd components/frontend && npm run build # Must pass with 0 errors, 0 warnings
# Runner (Python)
cd components/runners/claude-code-runner && uv venv && uv pip install -e .
# Docs
cd docs && npm run dev # http://localhost:4321Critical Context
Pre-commit HooksThe project uses the pre-commit framework to run linters locally before every commit. Configuration lives in Installmake setup-hooksWhat RunsOn every
On every
Run Manuallymake lint # All hooks, all files
pre-commit run gofmt-check --all-files # Single hook
pre-commit run --files path/to/file.go # Single fileSkip Hooksgit commit --no-verify # Skip pre-commit hooks
git push --no-verify # Skip pre-push hooksNotes
More InfoSee BOOKMARKS.md for architecture decisions, development context, code patterns, and component-specific guides. |
|
Claude Code Review -- PR #746 BLOCKER 1: TypeScript any violations (explicit project rule breach) BLOCKER 2: Committed binary artifacts CRITICAL 3: Python gRPC watch stub is unresolvable CRITICAL 4: go-sdk/go.mod replace directive breaks external consumers MAJOR 5: No tests for session watch MAJOR 6: Dual Python implementations, no guidance MAJOR 7: Monkey-patching SessionAPI MINOR 8: createGRPCConnection() hardcodes port 4434 -- make it configurable via WatchOptions. Positives: Path prefix cleanup is complete and consistent. TRex v0.0.14 bump is clean. Go session watcher has good channel-based design (Events/Errors/Done/Stop with context.WithCancel). SSE Python watcher provides sync+async variants with context manager support. TypeScript example covers Node.js and browser. Summary: Items 1-4 are blocking. Items 5-6 are strongly recommended before merge. |
Claude Code ReviewSummaryThis PR refactors SDK clients to use the new Issues by SeverityBlocker Issues1. Unresolved git conflict marker in
2. Module path mismatch breaks
3.
4. Missing newline at end of The diff shows Critical Issues5. Timeout context cancelled immediately on The 6. Incomplete Python gRPC implementation — imports non-existent module
7. Python
8. Duplicate Both 9. Binary file committed to the repository
Major Issues10.
11.
12. watch(opts?: WatchOptions): SessionWatcher {
return new SessionWatcher(this.config); // opts silently dropped
}
13.
14. This PR introduces gRPC v1.75.1 where Minor Issues15. Multiple new files missing trailing newline — failing
16. Go 17. 18. Busy-wait in 19. Positive Highlights
Recommendations
Generated with Claude Code 🔍 View AI decision process (logs available for 90 days) 📋 View memory system files loaded (click to expand)What Amber Loaded for Code ReviewAmber automatically loaded these repository standards from the memory system:
Impact: This review used your repository's specific code quality standards, security patterns, and best practices from the memory system (PRs #359, #360) - not just generic code review guidelines. |
Amber Code Review — PR #746
SummaryThis PR has good intent — fixing the API path prefix ( 🔴 Blocking Issues1. Go SDK: Invalid module path in
|
markturansky
force-pushed
the
feat/ambient-sdk
branch
2 times, most recently
from
6b283fc to
dc9e83b
Compare
- Generator: fix path prefix (/api/ambient/v1) for correct delete/update method detection - Generator: regenerate all SDK clients (Go, Python, TypeScript) from OpenAPI spec - Go SDK: add session watch via gRPC streaming (session_watch.go, types/session_watch.go) - Go SDK: NewClientFromEnv requires explicit project param (remove AMBIENT_PROJECT env var) - Go SDK: fix default API port 8080→8000 - Go SDK: fix examples/go.mod missing ambient-api-server replace directive - Go SDK: fix examples project lookup on conflict (List search instead of Get-by-name) - Python/TS SDK: add session watch via SSE streaming - All SDK clients: default port corrected to localhost:8000 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
feat/ambient-sdk
branch
from
dc9e83b to
2c45ca9
Compare
Merge Readiness — Blockers Found
|
1 participant
Summary
Motivation
Improve SDK usability by making project/namespace specification more explicit and reducing reliance on environment variables. Synchronize SDK with latest API dependency.
Test Plan
\ud83e\udd16 Generated with Claude Code