feat: add Gerrit integration connector for code review workflows#1027
Closed
sbauza wants to merge 7 commits intoambient-code:mainfrom
Closed
feat: add Gerrit integration connector for code review workflows#1027sbauza wants to merge 7 commits intoambient-code:mainfrom
sbauza wants to merge 7 commits intoambient-code:mainfrom
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 a Gerrit integration spanning backend, frontend, and runner: new backend handlers and validation to persist per-user/per-instance Gerrit credentials in a Kubernetes Secret; frontend UI, API routes, and React Query hooks for connect/test/list/disconnect; and runner-side config generation and MCP server bundling/startup. Changes
Sequence Diagram(s)sequenceDiagram
participant User as User
participant Frontend as Frontend UI
participant Backend as Backend API
participant Gerrit as Gerrit Service
participant K8s as Kubernetes Secret
User->>Frontend: Submit credentials (instance, URL, auth)
Frontend->>Backend: POST /api/auth/gerrit/test
Backend->>Gerrit: GET /a/accounts/self (with chosen auth)
Gerrit-->>Backend: 200 / 401/403 / other
Backend-->>Frontend: { valid: true/false, message? }
Frontend->>Backend: POST /api/auth/gerrit/connect
Backend->>Gerrit: GET /a/accounts/self (validate)
Gerrit-->>Backend: 200 OK
Backend->>K8s: Read/Update `gerrit-credentials` Secret (key: "<instanceName>.<userID>")
K8s-->>Backend: Secret stored/updated
Backend-->>Frontend: Connection confirmed
sequenceDiagram
participant User as User
participant Backend as Backend API
participant K8s as Kubernetes Secret
participant Runner as Runner Process
participant MCP as Gerrit MCP Server
User->>Backend: Start agentic session
Backend->>K8s: listGerritCredentials for user
K8s-->>Backend: credentials instances
Backend-->>Runner: session start / runner requests credentials
Runner->>Backend: GET session credentials endpoint
Backend-->>Runner: credential payloads (instances)
Runner->>Runner: generate_gerrit_config -> write gerrit_config.json + .gitcookies
Runner->>Runner: set GERRIT_CONFIG_PATH
Runner->>MCP: Launch Gerrit MCP Server (STDIO)
MCP-->>Runner: Tools available
User->>MCP: Use code-review tools
MCP-->>User: Responses
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes 🚥 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 |
![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: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/gerrit_auth.go`:
- Around line 73-77: The error message for instance name validation is
inconsistent with the regex used by validInstanceNameRegex (which requires at
least 2 characters due to the start and end character groups); update the
response in the validation block that checks req.InstanceName to either adjust
the regex to allow 1-63 chars or change the JSON error string to correctly state
"2-63 chars" so the message matches the pattern enforced by
validInstanceNameRegex.
In `@components/frontend/src/components/gerrit-connection-card.tsx`:
- Line 178: The frontend allows single-character instance names but the backend
requires at least two characters; update the onChange handling and validation so
instanceName is validated to match the backend regex (start/end with
alphanumeric, middle may be alnum or hyphen, total length 2–63). Specifically,
adjust the onChange transformation that calls setInstanceName(...) and update
isFormValid() to enforce instanceName.length >= 2 (and continue to enforce the
existing character set via the same regex used server-side) so the client-side
form validity matches the backend rule.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 330-364: In the server_name == "gerrit" fallback block, the nested
try/excepts silently swallow errors; update those except handlers to log the
exception at debug level (capture the exception as e) so connectivity/JSON/HTTP
failures are visible during development. Specifically, inside the outer except
and the inner except around _urllib_request.urlopen(req, timeout=3) add a debug
log that includes contextual info (e.g., the constructed url, env vars like
BACKEND_API_URL/PROJECT_NAME/SESSION_ID/BOT_TOKEN) and the exception (use
exc_info=True or logger.exception) — use the module logger (e.g.,
logging.getLogger(__name__) or the existing process logger if present) so the
logs appear in your normal log stream.
In `@components/runners/ambient-runner/Dockerfile`:
- Around line 72-87: The RUN shell block that clones and builds
gerrit-mcp-server should enable pipefail and avoid pip cache: add a shell option
like "set -o pipefail" at the start of the RUN command so failures in pipelines
(e.g., the WHEEL_FILE=$(ls dist/*.whl | head -n 1) and WHEEL_HASH=$(sha256sum
"${WHEEL_FILE}" | awk '{print $1}')) are propagated, and add --no-cache-dir to
the pip install invocation(s) (the pip install in the venv and the uv pip
install -r local-requirements.txt) to prevent caching and reduce image size
while keeping the same sequence of commands (refer to the RUN block, the pip
install call, WHEEL_FILE, and WHEEL_HASH uses to locate where to change).
In `@specs/001-gerrit-integration/contracts/gerrit-api.yaml`:
- Around line 1-6: The OpenAPI YAML is missing authentication metadata; add a
components.securitySchemes entry defining a bearer HTTP scheme (e.g., name it
bearerAuth with type: http, scheme: bearer, bearerFormat: JWT) and then add a
top-level security array (e.g., - bearerAuth: []) so the global security
requirement applies to all operations; update the existing document by inserting
components.securitySchemes and the global security field (use the symbol names
components.securitySchemes and security / bearerAuth to locate where to add the
entries).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: f03918dc-5606-49fe-8ac1-8fd1672e0cc9
📒 Files selected for processing (24)
components/backend/handlers/gerrit_auth.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/integrations_status.gocomponents/backend/handlers/runtime_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsxcomponents/frontend/src/components/gerrit-connection-card.tsxcomponents/frontend/src/services/api/gerrit-auth.tscomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/queries/use-gerrit.tscomponents/runners/ambient-runner/.mcp.jsoncomponents/runners/ambient-runner/Dockerfilecomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pyspecs/001-gerrit-integration/checklists/requirements.mdspecs/001-gerrit-integration/contracts/frontend-types.tsspecs/001-gerrit-integration/contracts/gerrit-api.yamlspecs/001-gerrit-integration/data-model.mdspecs/001-gerrit-integration/plan.mdspecs/001-gerrit-integration/quickstart.mdspecs/001-gerrit-integration/research.mdspecs/001-gerrit-integration/spec.mdspecs/001-gerrit-integration/tasks.md
sbauza
force-pushed
the
001-gerrit-integration
branch
from
dcee98b to
fbf8bb7
Compare
Author
|
@coderabbitai reverify |
Contributor
|
Tip For best results, initiate chat on the files or code changes. 🧩 Analysis chain🏁 Script executed: Length of output: 0 🏁 Script executed: Length of output: 2297 🏁 Script executed: Length of output: 6063 🏁 Script executed: Length of output: 3312
I apologize for the false positives on all five comments. The implementation is correct as-is. [resolve] |
Add a complete Gerrit integration across backend, frontend, and runner components, following the existing integration patterns (Jira, GitLab). Backend (Go): - Gerrit credential CRUD handlers (connect, status, disconnect, list) - Validation against Gerrit /a/accounts/self endpoint - K8s Secret storage with compound key (instanceName.userID) - Session-scoped credential endpoint for runner consumption - Integration status reporting - Unit tests for auth handler input validation Frontend (TypeScript/React): - Connection card with HTTP basic / gitcookies auth toggle - Multi-instance management with individual disconnect - React Query hooks with proper cache invalidation - Next.js API proxy routes for all Gerrit endpoints - Session integrations panel entry Runner (Python): - Gerrit credential fetching from backend API - Runtime config generation (gerrit_config.json + gitcookies file) - MCP server entry in .mcp.json using Python 3.12 venv - Dockerfile changes for Python 3.12 + Gerrit MCP server build - Credential cleanup on session end Documentation: - Gerrit integration guide (docs/internal/integrations/) - Updated integrations README with Gerrit section - OpenAPI contract spec - Design specs and implementation plan Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbauza
force-pushed
the
001-gerrit-integration
branch
from
0ed404e to
8bbe821
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 13
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/gerrit_auth_test.go`:
- Around line 30-40: The test mutates the global Namespace in
BeforeEach/AfterEach causing races under parallel Ginkgo runs; to fix, mark the
test Describe block as Serial (or similarly annotate it) so Ginkgo runs it
single-threaded, e.g., add the Serial decorator to the Describe that wraps these
BeforeEach/AfterEach hooks referencing Namespace and gerrit_auth.go helpers;
alternatively, refactor functions that use the global Namespace to accept an
injected namespace parameter and update the test to pass the test namespace
instead of mutating Namespace.
In `@components/backend/handlers/integrations_status.go`:
- Around line 137-156: The returned Gerrit instances from getGerritStatusForUser
are nondeterministic because listGerritCredentials iterates a map; before
building the result slice, sort the instances slice by a stable key (e.g.,
creds.InstanceName or creds.URL) using sort.Slice so the order is deterministic,
then map the sorted slice into the result (preserve existing fields "connected",
"instanceName", "url", "authMethod", "updatedAt"); this ensures
getGerritStatusForUser always returns a stable instances order.
In `@components/backend/routes.go`:
- Around line 163-169: The /version route is missing from the router
registration so the frontend's calls to GetVersion fail; add a GET route that
wires the existing GetVersion handler into the API router (e.g. add
api.GET("/version", handlers.GetVersion)) alongside the other api routes (uses
the same router variable "api" and existing handlers package), then run a
build/test to ensure the handler is reachable.
In `@components/frontend/src/app/api/auth/gerrit/`[instanceName]/status/route.ts:
- Around line 11-17: The route handler currently interpolates
params.instanceName directly into the upstream URL, allowing crafted values with
"/", "?", or ".." to manipulate the backend path; update the interpolation to
use encodeURIComponent(instanceName) wherever instanceName is used in the fetch
URL (e.g., the fetch call that builds
`${BACKEND_URL}/auth/gerrit/${instanceName}/status`) so the instance name is
safely URL-encoded before forwarding upstream.
In `@components/frontend/src/app/api/auth/gerrit/connect/route.ts`:
- Around line 8-15: The fetch call to BACKEND_URL in the route (the const resp =
await fetch(...) block) needs timeout and network-failure handling: wrap the
fetch with an AbortController, set a timer (e.g., 5–10s) that calls
controller.abort(), clear the timer after fetch resolves, and wrap the await
fetch in try/catch to handle AbortError and other network errors; on
timeout/abort return a Response with an appropriate status (e.g., 504 Gateway
Timeout) and JSON error body, and on other network errors return a 502/500 JSON
response—ensure you still preserve resp.status and Content-Type:
application/json for successful responses.
In `@components/frontend/src/app/api/auth/gerrit/instances/route.ts`:
- Around line 7-13: Wrap the backend fetch in a try-catch and add an
AbortController-based timeout (e.g., 30s) so the call using
fetch(`${BACKEND_URL}/auth/gerrit/instances`, { method: 'GET', headers }) cannot
hang; create an AbortController, pass controller.signal to fetch, set a timer to
call controller.abort() after 30000ms, clear the timer on success, and in the
catch return a sensible Response (e.g., 504 Gateway Timeout for an abort or 500
for other errors) with a JSON error message while preserving appropriate headers
and status handling for resp/status.
In `@components/frontend/src/app/api/auth/gerrit/test/route.ts`:
- Around line 8-15: The proxy POST to `${BACKEND_URL}/auth/gerrit/test` must use
an AbortController with a configurable timeout and proper error handling: create
an AbortController, pass its signal into fetch, start a timer (e.g., 5–10s) that
calls controller.abort(), and clear the timer on successful response; wrap the
fetch/response processing in try/catch to catch AbortError and network errors
and return a proper error Response (e.g., 502 for upstream failure or 504 for
timeout) with a JSON error body and Content-Type header; apply the same pattern
to the gerrit/connect route; reference the existing BACKEND_URL fetch call (resp
variable) to locate where to add the controller/signal, timer, try/catch, and
appropriate error Responses.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/integrations-panel.tsx:
- Around line 22-23: The panel now includes Gerrit (see gerritConfigured and
gerritUrls), so update the integrations-panel tests
(integrations-panel.test.tsx) to reflect five integrations instead of four:
change any "3/4" badge expectations to "3/5", add Gerrit to the test fixture
data so the rendered list includes the new sorted card, and update related
assertions/snapshot checks (the blocks corresponding to the previous 40-69 and
53-60 assertion regions) so they expect the extra card and its sorted position.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 42-47: When the refreshed instance list is empty the code
currently returns early and leaves the old Gerrit config dir
(Path("/tmp/gerrit-mcp")) and GERRIT_CONFIG_PATH in place; change the
early-return branch to remove the existing config directory (recursively) and
unset/clear the GERRIT_CONFIG_PATH environment variable (and any stale
.gitcookies) before returning, so stale credentials can't be reused; apply the
same cleanup logic to the corresponding block around the second
instance-handling section (the other branch in the same file) to ensure both
code paths regenerate from a clean directory.
In `@docs/internal/integrations/gerrit-integration.md`:
- Around line 208-212: The docs currently assert that Kubernetes Secrets are
encrypted at rest; update the bullet list for the `gerrit-credentials` Secret to
instead state that encryption at rest is only available when cluster-level
encryption is configured (e.g., kube-apiserver `--encryption-provider-config`
with a provider such as KMS) and either add a short deployment prerequisite
saying "Ensure kube-apiserver encryption is enabled to encrypt secrets at rest"
or remove the encryption claim entirely; specifically replace the line "Secrets
are encrypted at rest (standard Kubernetes Secret encryption)" with the
conditional wording and, if chosen, add a one-line prerequisite about enabling
encryption in the cluster.
In `@docs/internal/integrations/README.md`:
- Around line 65-69: The Markdown headings "### Gerrit" are missing a blank line
after them which triggers MD022; locate each "### Gerrit" heading in the README
(the two occurrences shown) and insert a single empty line immediately after the
heading to separate it from the following list content so markdownlint MD022 is
satisfied.
In `@specs/001-gerrit-integration/contracts/frontend-types.ts`:
- Around line 26-32: The GerritInstanceStatus contract currently marks url,
authMethod, and updatedAt as optional but the implementation expects them to be
present; update the GerritInstanceStatus interface (the GerritInstanceStatus
declaration) to make url: string, authMethod: GerritAuthMethod, and updatedAt:
string (remove the ?), or alternatively change the implementation in
components/frontend's integration API to accept optional fields—preferably align
the contract by removing the optional modifier so the interface matches the
implementation.
In `@specs/001-gerrit-integration/data-model.md`:
- Around line 24-28: Update the spec text for instanceName to say "2-63 chars"
to match the actual regex `^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$` and the
implementation in gerrit_auth.go; ensure the Validation rules entry referencing
`instanceName` is changed from "1-63 chars" to "2-63 chars" so the spec and code
are consistent.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: c49f2623-be1b-4bc9-8891-f746adda48e6
📒 Files selected for processing (32)
components/backend/handlers/gerrit_auth.gocomponents/backend/handlers/gerrit_auth_test.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/integrations_status.gocomponents/backend/handlers/runtime_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/api/auth/gerrit/[instanceName]/disconnect/route.tscomponents/frontend/src/app/api/auth/gerrit/[instanceName]/status/route.tscomponents/frontend/src/app/api/auth/gerrit/connect/route.tscomponents/frontend/src/app/api/auth/gerrit/instances/route.tscomponents/frontend/src/app/api/auth/gerrit/test/route.tscomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsxcomponents/frontend/src/components/gerrit-connection-card.tsxcomponents/frontend/src/services/api/gerrit-auth.tscomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/queries/use-gerrit.tscomponents/runners/ambient-runner/.mcp.jsoncomponents/runners/ambient-runner/Dockerfilecomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pydocs/internal/integrations/README.mddocs/internal/integrations/gerrit-integration.mdspecs/001-gerrit-integration/checklists/requirements.mdspecs/001-gerrit-integration/contracts/frontend-types.tsspecs/001-gerrit-integration/contracts/gerrit-api.yamlspecs/001-gerrit-integration/data-model.mdspecs/001-gerrit-integration/plan.mdspecs/001-gerrit-integration/quickstart.mdspecs/001-gerrit-integration/research.mdspecs/001-gerrit-integration/spec.mdspecs/001-gerrit-integration/tasks.md
Comment on lines
+30
to
+40
| BeforeEach(func() { | ||
| logger.Log("Setting up Gerrit Auth Handler test") | ||
|
|
||
| originalNamespace = Namespace | ||
|
|
||
| // Use centralized handler dependencies setup | ||
| k8sUtils = test_utils.NewK8sTestUtils(false, *config.TestNamespace) | ||
| SetupHandlerDependencies(k8sUtils) | ||
|
|
||
| // gerrit_auth.go uses Namespace (backend namespace) for secret operations | ||
| Namespace = *config.TestNamespace |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check if other test files in handlers follow the same pattern
rg -n 'Namespace\s*=' --glob '*_test.go' components/backend/handlers/Repository: ambient-code/platform
Length of output: 2355
🏁 Script executed:
# Check for Ginkgo configuration files
find . -name "ginkgo.yml" -o -name ".ginkgo*" -o -name "ginkgo.toml" 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check for Serial decorator usage in test files to see if tests are sequentialized
rg -n "Serial\|Ordered" --glob '*_test.go' components/backend/handlers/ | head -20Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check how tests are invoked (Makefile, go.mod test flags, etc.)
find . -name "Makefile" -o -name "*.mk" -o -name "go.mod" | xargs grep -l "test\|ginkgo" 2>/dev/null | head -10Repository: ambient-code/platform
Length of output: 247
🏁 Script executed:
# Check the go.mod and look for ginkgo version
head -50 go.modRepository: ambient-code/platform
Length of output: 130
🏁 Script executed:
# Check the backend Makefile for test target configuration
cat -n components/backend/Makefile | grep -A 10 -B 5 "test"Repository: ambient-code/platform
Length of output: 7839
🏁 Script executed:
# Check backend go.mod for ginkgo dependency and version
cat components/backend/go.mod | grep -i ginkgoRepository: ambient-code/platform
Length of output: 99
🏁 Script executed:
# Check if any tests use Ordered decorator (alternative to Serial in newer Ginkgo)
rg "Ordered|Serial" components/backend/handlers/gerrit_auth_test.goRepository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look at the full gerrit_auth_test.go structure to see Describe blocks
head -50 components/backend/handlers/gerrit_auth_test.goRepository: ambient-code/platform
Length of output: 1577
🏁 Script executed:
# Search for any comments or documentation about the Namespace mutation pattern
rg -B 2 "originalNamespace = Namespace" components/backend/handlers/ -A 2Repository: ambient-code/platform
Length of output: 2535
🏁 Script executed:
# Check if there are any synchronization primitives (sync.Mutex, channels) in gerrit_auth.go
rg "sync\.|mutex|lock|Mutex" components/backend/handlers/gerrit_auth.goRepository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Look at AfterEach in gerrit_auth_test.go to see full cleanup
rg -A 5 "AfterEach" components/backend/handlers/gerrit_auth_test.goRepository: ambient-code/platform
Length of output: 310
Add Serial decorator to prevent data race when tests run in parallel.
The test mutates the global Namespace variable in BeforeEach (line 40) and restores it in AfterEach (line 68). While the default test invocation is sequential, Ginkgo's parallel mode (-p flag via make test-ginkgo-parallel) would cause concurrent tests to interfere with each other, as gerrit_auth.go functions read Namespace without synchronization.
Add Serial decorator to the Describe block to prevent this, or refactor to inject namespace as a parameter. Note this pattern is used across multiple auth handler test files.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/gerrit_auth_test.go` around lines 30 - 40, The
test mutates the global Namespace in BeforeEach/AfterEach causing races under
parallel Ginkgo runs; to fix, mark the test Describe block as Serial (or
similarly annotate it) so Ginkgo runs it single-threaded, e.g., add the Serial
decorator to the Describe that wraps these BeforeEach/AfterEach hooks
referencing Namespace and gerrit_auth.go helpers; alternatively, refactor
functions that use the global Namespace to accept an injected namespace
parameter and update the test to pass the test namespace instead of mutating
Namespace.
components/frontend/src/app/api/auth/gerrit/[instanceName]/status/route.ts
Outdated
Show resolved
Hide resolved
Comment on lines
+8
to
+15
| const resp = await fetch(`${BACKEND_URL}/auth/gerrit/connect`, { | ||
| method: 'POST', | ||
| headers, | ||
| body, | ||
| }) | ||
|
|
||
| const data = await resp.text() | ||
| return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify Gerrit proxy routes include timeout signal and local catch handling.
rg -n --type=ts -C2 'fetch\(`\$\{BACKEND_URL\}/auth/gerrit' components/frontend/src/app/api/auth/gerrit
rg -n --type=ts -C2 'signal\s*:' components/frontend/src/app/api/auth/gerrit
rg -n --type=ts -C2 'catch\s*\(' components/frontend/src/app/api/auth/gerritRepository: ambient-code/platform
Length of output: 2600
🏁 Script executed:
cat -n components/frontend/src/app/api/auth/gerrit/connect/route.tsRepository: ambient-code/platform
Length of output: 669
🏁 Script executed:
# Check if similar unprotected fetch patterns exist in other auth routes
rg -n --type=ts 'await fetch\(`\$\{BACKEND_URL\}' components/frontend/src/app/api/auth --no-heading | head -20Repository: ambient-code/platform
Length of output: 2626
🏁 Script executed:
# Check for existing timeout patterns or utilities
rg -n --type=ts 'AbortController|setTimeout.*timeout|TIMEOUT|Abort' components/frontend/src/lib --type ts | head -10Repository: ambient-code/platform
Length of output: 149
Add timeout + network-failure handling for the backend proxy call.
Line 8 issues an upstream fetch without timeout/abort handling; a hung backend call can stall this route indefinitely.
Proposed fix
export async function POST(request: Request) {
const headers = await buildForwardHeadersAsync(request)
const body = await request.text()
- const resp = await fetch(`${BACKEND_URL}/auth/gerrit/connect`, {
- method: 'POST',
- headers,
- body,
- })
-
- const data = await resp.text()
- return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+ const controller = new AbortController()
+ const timeout = setTimeout(() => controller.abort(), 10_000)
+ try {
+ const resp = await fetch(`${BACKEND_URL}/auth/gerrit/connect`, {
+ method: 'POST',
+ headers,
+ body,
+ signal: controller.signal,
+ })
+
+ const data = await resp.text()
+ return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+ } catch {
+ return Response.json({ error: 'Upstream Gerrit connect request failed' }, { status: 504 })
+ } finally {
+ clearTimeout(timeout)
+ }
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/app/api/auth/gerrit/connect/route.ts` around lines 8
- 15, The fetch call to BACKEND_URL in the route (the const resp = await
fetch(...) block) needs timeout and network-failure handling: wrap the fetch
with an AbortController, set a timer (e.g., 5–10s) that calls
controller.abort(), clear the timer after fetch resolves, and wrap the await
fetch in try/catch to handle AbortError and other network errors; on
timeout/abort return a Response with an appropriate status (e.g., 504 Gateway
Timeout) and JSON error body, and on other network errors return a 502/500 JSON
response—ensure you still preserve resp.status and Content-Type:
application/json for successful responses.
- Sort Gerrit instances by name for deterministic API responses - Register missing /version route in backend - Encode instanceName in frontend proxy routes to prevent path injection - Clean up stale Gerrit config when instance list is empty - Add debug logging to bare except blocks in MCP credential checks - Update integrations-panel tests for 5th integration (Gerrit) - Fix markdown heading spacing (MD022) - Clarify K8s Secrets encryption-at-rest requires cluster config - Align contract types with implementation (required fields) - Fix instanceName char range in spec (2-63, not 1-63) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@specs/001-gerrit-integration/contracts/frontend-types.ts`:
- Around line 10-17: Replace the loose GerritConnectRequest interface with a
discriminated union keyed on authMethod so TypeScript enforces the conditional
credential fields; specifically, change GerritConnectRequest (and adjust any
usage expecting GerritAuthMethod) into two union members: one with authMethod:
'http_basic' that requires username and httpToken, and one with authMethod:
'git_cookies' that requires gitcookiesContent, ensuring callers and validators
use the new union shape for compile-time safety and IDE autocomplete.
- Around line 34-37: The GerritTestResponse contract is missing the optional
message field present in the implementation; update the GerritTestResponse
interface to include message?: string so it matches the implementation
(reference symbol: GerritTestResponse) and ensure any consumers/types relying on
error?: string and valid: boolean remain unchanged.
In `@specs/001-gerrit-integration/data-model.md`:
- Around line 51-56: The spec currently shows data keys as
`{instanceName}:{userID}` but the code uses a dot separator via
gerritSecretKey(instanceName, userID) which returns instanceName + "." + userID;
update the documentation and examples to use `{instanceName}.{userID}` (dot
separator) and ensure the examples showing secret entries (e.g., keys
`openstack.user123` and `android.user123`) match the GerritCredentials JSON
values and the gerritSecretKey implementation.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 0bdf6e5d-973c-4b8f-8fb6-5a42195cd466
📒 Files selected for processing (10)
components/backend/handlers/integrations_status.gocomponents/backend/routes.gocomponents/frontend/src/app/api/auth/gerrit/[instanceName]/disconnect/route.tscomponents/frontend/src/app/api/auth/gerrit/[instanceName]/status/route.tscomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsxcomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pydocs/internal/integrations/README.mddocs/internal/integrations/gerrit-integration.mdspecs/001-gerrit-integration/contracts/frontend-types.tsspecs/001-gerrit-integration/data-model.md
- Fix Secret key separator in data-model spec: colon to dot (matches code) - Add missing message field to GerritTestResponse contract - Use discriminated union for GerritConnectRequest type safety Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Remove trailing newline in gerrit_auth.go - Fix comment alignment in gerrit_auth_test.go Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@components/backend/handlers/gerrit_auth_test.go`:
- Around line 96-114: The tests named "Should require user authentication" call
ConnectGerrit but never reach the userID check because they omit the
Authorization header and thus hit the initial "Invalid or missing token" guard;
update these specs to set the Authorization header to testToken (use
httpUtils.CreateTestGinContext or add the header on the returned context) while
still omitting the user context so the handler proceeds to the user check, then
assert the HTTP status and JSON message "User authentication required"; apply
the same change to the other failing specs referenced around ConnectGerrit (the
blocks at the indicated ranges) so they all exercise the user-authentication
branch rather than the missing-token guard.
- Around line 207-236: The test ConnectGerrit in gerrit_auth_test.go is making
real HTTP calls because ValidateGerritToken (integration_validation.go) performs
a live request; replace that network dependency by either stubbing/mocking
ValidateGerritToken or creating an httptest.Server and point the requestBody
"url" to that server so ValidateGerritToken hits the test server, then assert
the specific post-validation behavior (e.g., that ConnectGerrit returns the
expected HTTP status/body) rather than a generic status != 400; reference
ConnectGerrit and ValidateGerritToken when implementing the stub/mock or wiring
the httptest server and reset httpUtils between iterations.
In `@components/backend/handlers/gerrit_auth.go`:
- Around line 79-95: The handler currently only validates required fields but
still copies all request credential fields into creds, allowing mixed payloads
(e.g., authMethod "http_basic" with gitcookiesContent) to be stored; update the
switch in the Gerrit auth handler so that for the selected auth method you
either reject requests that include fields for the other method or explicitly
clear the unused credential fields from creds before calling
storeGerritCredentials; locate the credential construction and call to
storeGerritCredentials and ensure unused fields (e.g., GitcookiesContent,
Username, HTTPToken) are zeroed when the other method is chosen, and apply the
same defensive change to the other validation block later in the file that
performs similar authMethod handling.
- Around line 59-65: Parse and validate req.URL before calling
ValidateGerritToken (and similarly where req.URL is used at the other location
around the block at lines 96-99): ensure the scheme is https, resolve the host
to IPs and reject loopback (127.0.0.0/8, ::1), link-local, RFC1918/private
ranges, and known metadata-service addresses, or require the host to be on an
explicit allowlist; only after these checks pass, call ValidateGerritToken with
the original URL. Use the parsed URL.Host/Hostname and DNS resolution to perform
the IP checks and fail the request early with a clear error if the target is
disallowed.
- Line 31: The single shared Secret constant gerritSecretName causes contention
and unbounded reads; change storage to per-user or per-user+instance Secrets
(e.g., derive secret names from userID and optional instanceID) and update all
handlers in gerrit_auth.go that call Get/Update/List/Connect/Disconnect to use
the new per-entity secret name scheme instead of gerritSecretName, adjust
list/filter logic to only target relevant Secrets (or use labels) to avoid
loading unrelated credential blobs, and ensure gitcookiesContent and other
payloads are sharded so individual Secret size stays within Kubernetes limits
and retries/locks no longer hit a single hotspot.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 84996a13-e384-42ec-8aa2-57def018162b
📒 Files selected for processing (2)
components/backend/handlers/gerrit_auth.gocomponents/backend/handlers/gerrit_auth_test.go
Contributor
Review Queue Status
Action needed: Address CodeRabbit Critical security issue — block arbitrary URLs before validation
|
- Add validateGerritURL with SSRF protection: enforce HTTPS, reject loopback/private/link-local/metadata IPs via DNS resolution - Reject mixed auth credentials (e.g., gitcookiesContent with http_basic) - Make ValidateGerritToken mockable via function variable for tests - Add gerrit_auth_test.go with comprehensive unit tests using stubbed validation (no live HTTP calls) - Fix secret key separator from colon to dot for K8s compatibility - Add scalability comment about shared Secret design - Add SSRF protection to TestGerritConnection handler Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Refactor from single shared gerrit-credentials Secret to per-user
Secrets (gerrit-credentials-{userID}) to eliminate cross-user
contention, unbounded Secret growth, and suffix-filtering overhead
- Data key is now just instanceName (no compound key needed)
- Sort Gerrit instances by name in listGerritCredentials for
deterministic API responses
- Clean up old /tmp/gerrit-mcp directory and unset GERRIT_CONFIG_PATH
when instances list is empty (prevents stale credential reuse)
- Update docs and data-model spec to reflect per-user Secret design
- Update test file for per-user secret naming
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
7 tasks
Author
|
ACP deleted my branch for some reason, had to reopen a new PR that you can find here #1078 |
Member
Sorry about that. Do you have a session URI / session link we can share with the team? There might be something we can learn from this failed agent. |
Author
|
@bobbravo2 Sure, here is the session https://ambient-code.apps.rosa.vteam-uat.0ksl.p3.openshiftapps.com/projects/nova/sessions/session-1774372472 |
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
2 participants
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
Add Gerrit as a first-class integration connector in Ambient Code Platform, enabling users to connect their Gerrit code review instances and use AI-assisted code review workflows through the open-source Gerrit MCP server.
/a/accounts/self), session-scoped credential fetch, unified integration statusgerrit_config.jsongeneration, credential lifecycle management (populate/clear)Changes
New Files (5)
components/backend/handlers/gerrit_auth.gocomponents/frontend/src/services/api/gerrit-auth.tscomponents/frontend/src/services/queries/use-gerrit.tscomponents/frontend/src/components/gerrit-connection-card.tsxspecs/001-gerrit-integration/Modified Files (10)
Dockerfileintegration_validation.goValidateGerritToken(),parseGitcookies(),TestGerritConnection()runtime_credentials.goGetGerritCredentialsForSession()handlerintegrations_status.gogetGerritStatusForUser()helperroutes.gointegrations.tsIntegrationsStatustypeIntegrationsClient.tsx.mcp.jsonauth.pyfetch_gerrit_credentials(), populate/clear lifecyclemcp.pygenerate_gerrit_config(), Gerrit auth checkintegrations-panel.tsxAuth Methods
.gitcookiesfile content (used by OpenStack and other communities)Test Plan
🤖 Generated with Claude Code