feat: add Gerrit integration connector for code review workflows#1078
Open
sbauza wants to merge 5 commits intoambient-code:mainfrom
Open
feat: add Gerrit integration connector for code review workflows#1078sbauza wants to merge 5 commits intoambient-code:mainfrom
sbauza wants to merge 5 commits intoambient-code:mainfrom
Conversation
Contributor
|
Warning Rate limit exceeded
Your organization is not enrolled in usage-based pricing. Contact your admin to enable usage-based pricing to continue reviews beyond the rate limit, or try again in 1 minutes and 57 seconds. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (10)
WalkthroughAdds multi-instance Gerrit integration: backend connect/test/list/status/disconnect handlers with per-user Kubernetes Secret storage and SSRF-safe validation; frontend UI, API client, and React Query hooks; runner bundling of a Gerrit MCP server and per-session config generation; plus docs, specs, and tests. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant Kubernetes as K8s
participant Runner
User->>Frontend: Open Integrations UI / Submit Connect payload
Frontend->>Backend: POST /api/auth/gerrit/connect (forwarded headers + body)
Backend->>Backend: validateGerritURL, validateGerritToken
alt valid
Backend->>Kubernetes: Create/Update Secret `gerrit-credentials-<userID>` (JSON key per instance)
Kubernetes-->>Backend: Secret persisted
Backend-->>Frontend: 200 OK (connected)
Frontend-->>User: success UI update
else invalid
Backend-->>Frontend: 4xx/401 error
Frontend-->>User: show error
end
Note over Runner,K8s: At session startup
Runner->>Backend: GET /api/projects/:p/agentic-sessions/:s/credentials/gerrit
Backend->>Kubernetes: Read Secret `gerrit-credentials-<effectiveUserID>`
Kubernetes-->>Backend: Secret data (instances JSON)
Backend-->>Runner: instances array
Runner->>Runner: generate_gerrit_config(instances) -> write gerrit_config.json + gitcookies file
Runner->>Runner: Launch gerrit-mcp-server with GERRIT_CONFIG_PATH
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 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 |
7 tasks
![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: 17
🤖 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 277-279: The test currently only checks that status (from
httpUtils.GetResponseRecorder().Code) is not http.StatusBadRequest which is too
weak; update the assertion to require the exact expected success status (e.g.,
Expect(status).To(Equal(http.StatusOK), "Should accept valid http_basic
credentials")) so failures like 401/500 fail the test; modify the Expect call
and message around the status variable and httpUtils.GetResponseRecorder() usage
accordingly.
- Line 365: The test currently allows two status codes by using
Expect(status).To(BeElementOf(http.StatusOK, http.StatusNotFound)); change this
to assert a single canonical response code (e.g.,
Expect(status).To(Equal(http.StatusOK)) or
Expect(status).To(Equal(http.StatusNotFound))) consistent with the API contract
you intend to enforce. Update the assertion in gerrit_auth_test.go (the
Expect(status) assertion) to use Equal(...) for the chosen canonical code and
adjust any test setup or comments to reflect that single expected outcome.
In `@components/backend/handlers/gerrit_auth.go`:
- Around line 48-82: validateGerritURL currently does DNS resolution separately
from the HTTP request, making it vulnerable to DNS-rebinding TOCTOU; update the
implementation so the same trusted IP is enforced when the credential HTTP call
is made (referencing validateGerritURL and validateGerritTokenFn). Fix by
resolving the hostname once and either: (A) pass the resolved IP into the HTTP
request (pin the IP and set Host header to original hostname) so the request
goes to that IP, or (B) create a custom http.Transport with a DialContext that
performs the IP validation (reuse the same validation logic from
validateGerritURL) immediately before dialing and rejects connections to
loopback/private/metadata addresses; ensure the chosen approach blocks
169.254.169.254 and private ranges and uses HTTPS scheme checks as before.
- Around line 199-203: Handlers GetGerritStatus, DisconnectGerrit, and
ListGerritInstances retrieve userID from the context but don't validate its
format; follow the same pattern used in ConnectGerrit by calling
isValidUserID(userID) immediately after userID := c.GetString("userID"),
returning c.JSON(http.StatusUnauthorized, gin.H{"error":"User authentication
required"}) and early return if validation fails. Update each handler
(GetGerritStatus, DisconnectGerrit, ListGerritInstances) to perform this check
to ensure consistent userID validation across the code paths.
In `@components/backend/handlers/integration_validation.go`:
- Around line 194-203: The current validation in the function that checks
resp.StatusCode (the Gerrit /a/accounts/self validation) treats any non-401/403
status as valid, which allows 5xx or other errors to be accepted; change the
logic so only http.StatusOK (200) returns true, and all other responses
(including 5xx, 3xx, etc.) return false (fail closed) while preserving error
handling for the HTTP request and returning nil error for authentication
failures; update the block that currently checks resp.StatusCode to explicitly
return false, nil for any status != http.StatusOK and keep the existing true,
nil for http.StatusOK.
In `@components/backend/handlers/runtime_credentials.go`:
- Around line 514-530: The response is non-deterministic because the loop over
instances (returned by listGerritCredentials) preserves map iteration order;
before building result in the runtime_credentials.go handler (the block that
constructs instance := gin.H{...} and appends to result), sort the instances
slice by a stable key (e.g., creds.InstanceName) using sort.SliceStable to
guarantee deterministic ordering, then iterate the sorted slice to build result
and return it via c.JSON; this ensures consistent Gerrit instance ordering
across requests.
In `@components/frontend/src/app/api/auth/gerrit/test/route.ts`:
- Around line 8-15: The fetch to `${BACKEND_URL}/auth/gerrit/test` is unbounded
and uncaught; wrap the call in an AbortController with a short timeout (e.g.,
5s) and pass signal to fetch, then handle errors and non-OK responses
explicitly: catch abort/timeouts and network errors and return a JSON Response
with an appropriate upstream-failure status (504 for timeout, 502 for other
upstream/network failures) and a JSON body { error: "..."}; also check resp.ok
and, if false, read resp.text() or resp.json() and return it wrapped as JSON
with status 502 so the route returns controlled JSON errors instead of framework
500s. Use the existing BACKEND_URL reference, the fetch call site, and the final
new Response(...) creation to locate where to add the AbortController,
try/catch, resp.ok handling, and appropriate status mapping.
In `@components/frontend/src/services/api/gerrit-auth.ts`:
- Around line 5-20: Replace the flat optional-field shapes GerritConnectRequest
and GerritTestRequest with discriminated-union types keyed by authMethod so
TypeScript enforces required fields per method (e.g., for GerritConnectRequest
and GerritTestRequest provide variants for each GerritAuthMethod such as
'http_basic' that requires username, 'http_token' that requires httpToken, and
'git_cookies' that requires gitcookiesContent); follow the contract in
specs/001-gerrit-integration/contracts/frontend-types.ts to mirror backend
validation and update any usages to accept the new union types.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 105-106: generate_gerrit_config currently only sets
os.environ["GERRIT_CONFIG_PATH"] in the parent process but may be invoked after
the Gerrit MCP subprocess (adapter/MCP) has already been started, so the child
keeps an old/empty value; update the flow to ensure the config is available to
the MCP process by either calling generate_gerrit_config() before creating the
MCP/adapter subprocess or detect changes to GERRIT_CONFIG_PATH and gracefully
restart the Gerrit MCP subprocess when generate_gerrit_config updates the path;
specifically, modify the MCP startup sequence that spawns the Gerrit adapter/MCP
to depend on generate_gerrit_config (or add a watcher that compares the new
config path produced by generate_gerrit_config and triggers a restart of the
Gerrit MCP process) so children always inherit the current GERRIT_CONFIG_PATH.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 421-429: When backend returns an empty list for gerrit_instances
the old Gerrit config can persist; update the conditional after fetching
gerrit_instances to handle the empty-list case by clearing stale configs: in the
branch that currently checks gerrit_instances and calls generate_gerrit_config,
add an else branch that either calls generate_gerrit_config([]) to regenerate an
empty config or explicitly remove the file at GERRIT_CONFIG_PATH (or call the
generator/cleanup helper) so no old config files remain; reference
gerrit_instances, generate_gerrit_config, and GERRIT_CONFIG_PATH when making the
change.
In `@docs/internal/integrations/gerrit-integration.md`:
- Line 442: The credential key format in the line stating "A: Yes. Instance
names are scoped per user..." conflicts with the Security section; replace the
compound key `instanceName.userID` with the canonical model described in
Security: store credentials in per-user Secrets keyed by `instanceName` only,
and update that sentence to clarify that different users can have the same
`instanceName` because each user's Secret holds its own `instanceName` entry;
ensure any other mentions in this paragraph reference the per-user Secret +
`instanceName` key scheme instead of `instanceName.userID`.
In `@specs/001-gerrit-integration/contracts/frontend-types.ts`:
- Around line 10-23: The GerritConnectRequest union models the connect payload
and incorrectly forces instanceName for the `/test` flow; add a new dedicated
type (e.g., GerritTestRequest) that mirrors the authMethod branches of
GerritConnectRequest but omits instanceName (keep the same branch discriminants
authMethod: 'http_basic' and 'git_cookies' and their fields like
username/httpToken and gitcookiesContent), and update any callers of the `/test`
endpoint to use GerritTestRequest instead of GerritConnectRequest.
In `@specs/001-gerrit-integration/contracts/gerrit-api.yaml`:
- Around line 90-100: The contract reuses GerritConnectRequest for POST
/api/auth/gerrit/test (operationId testGerritConnection) which wrongly requires
instanceName and allows ambiguous combos of
username/httpToken/gitcookiesContent; create a new GerritTestRequest schema that
requires only url and the authentication discriminator, then redefine both
GerritTestRequest and GerritConnectRequest to model authentication via a oneOf
with discriminator "authMethod" and separate branches for the username+httpToken
and gitcookiesContent variants (ensuring each branch enforces required fields),
and update the /api/auth/gerrit/test requestBody to reference GerritTestRequest
while leaving the connect endpoint referencing GerritConnectRequest.
In `@specs/001-gerrit-integration/plan.md`:
- Around line 107-110: Update the plan text to match the implemented storage
model: replace the single shared secret approach (`gerrit-credentials` with
`{instanceName}:{userID}` keys) with the per-user secret naming pattern used in
code (`gerrit-credentials-{userID}`) and describe that each per-user secret
contains per-instance entries; also adjust the "Why" rationale to reflect
per-user secret benefits (scoped access, simpler rotation/cleanup) and remove
references to scanning keys ending with `:userID`.
In `@specs/001-gerrit-integration/quickstart.md`:
- Around line 39-40: The quickstart clones Gerrit MCP server and builds
immediately, which is non-reproducible; change the instructions to pin a known
revision before running build-gerrit.sh by adding a step to checkout a specific
commit hash, tag, or release (e.g., run git fetch --all && git checkout
<COMMIT_HASH_OR_TAG> or use git clone --branch <TAG> --single-branch) and verify
the checked-out revision (git rev-parse HEAD) before invoking ./build-gerrit.sh
so the build uses a fixed, reviewed revision.
In `@specs/001-gerrit-integration/spec.md`:
- Around line 67-69: User Story 4 - Connect Multiple Gerrit Instances currently
contradicts itself (saying users can "specify which one to use" while a
clarification and the acceptance scenario state all connected instances are
automatically available); pick one model (either per-session selection or global
auto-availability) and make the spec consistent by updating the story
description, the clarification paragraph (currently at Line 14), and the
acceptance scenario text so they all describe the same behavior; ensure the
chosen model is reflected in the title/summary ("Connect Multiple Gerrit
Instances"), the acceptance criteria, and any session/UX notes so future UI work
targets the correct behavior.
In `@specs/001-gerrit-integration/tasks.md`:
- Around line 39-40: The task spec T003 incorrectly describes a shared Secret
layout; update the description and any helper guidance to the per-user Secret
design used in code: use per-user K8s Secrets named with the pattern
`gerrit-credentials-{userID}` that store keys by `instanceName` (not
`{instanceName}:{userID}`), and adjust the K8s secret helper behaviors in
components/backend/handlers/gerrit_auth.go (GerritCredentials struct,
store/get/delete/list helpers) to read/write per-user Secrets with label
`ambient-code.io/provider: gerrit`; also ensure the five handlers
(ConnectGerrit, TestGerritConnection, GetGerritStatus, DisconnectGerrit,
ListGerritInstances) follow the per-user secret conventions and the same
conflict-retry and error-handling patterns as jira_auth.go, and update the T004
description for GetGerritCredentialsForSession in
components/backend/handlers/runtime_credentials.go to state it fetches all
instance keys from the `gerrit-credentials-{userID}` Secret and returns
instances with fields instanceName, url, authMethod, username, httpToken,
gitcookiesContent.
🪄 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: 02132bdb-dedf-42f5-acc6-b2eb7051bd3f
📒 Files selected for processing (34)
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/backend/tests/constants/labels.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/__tests__/integrations-panel.test.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
+48
to
+82
| // validateGerritURL validates a Gerrit URL for SSRF protection. | ||
| // It ensures the scheme is HTTPS and the host does not resolve to | ||
| // loopback, private, link-local, or metadata-service addresses. | ||
| func validateGerritURL(rawURL string) error { | ||
| parsed, err := url.Parse(rawURL) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid URL: %w", err) | ||
| } | ||
| if parsed.Scheme != "https" { | ||
| return fmt.Errorf("URL must use HTTPS scheme") | ||
| } | ||
| host := parsed.Hostname() | ||
| if host == "" { | ||
| return fmt.Errorf("URL must include a hostname") | ||
| } | ||
|
|
||
| ips, err := net.LookupHost(host) | ||
| if err != nil { | ||
| return fmt.Errorf("cannot resolve host %q: %w", host, err) | ||
| } | ||
| for _, ipStr := range ips { | ||
| ip := net.ParseIP(ipStr) | ||
| if ip == nil { | ||
| continue | ||
| } | ||
| if ip.IsLoopback() || ip.IsPrivate() || ip.IsLinkLocalUnicast() || ip.IsLinkLocalMulticast() { | ||
| return fmt.Errorf("URL must not resolve to a private or loopback address") | ||
| } | ||
| // Block cloud metadata endpoints (169.254.169.254) | ||
| if ip.Equal(net.ParseIP("169.254.169.254")) { | ||
| return fmt.Errorf("URL must not resolve to a metadata service address") | ||
| } | ||
| } | ||
| return nil | ||
| } |
Contributor
There was a problem hiding this comment.
SSRF protection vulnerable to DNS rebinding (TOCTOU).
DNS resolution at validation time (line 64) and the actual HTTP request in validateGerritTokenFn occur at different times. An attacker controlling a DNS server can return a public IP during validation, then a private/metadata IP when the credential validation request is made.
Mitigation options:
- Resolve DNS once and pass the resolved IP to the HTTP client (pin the IP).
- Use a custom
http.Transportwith aDialContextthat re-validates the resolved IP before connecting. - Perform the IP check in the HTTP client's dial function rather than as a separate step.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/gerrit_auth.go` around lines 48 - 82,
validateGerritURL currently does DNS resolution separately from the HTTP
request, making it vulnerable to DNS-rebinding TOCTOU; update the implementation
so the same trusted IP is enforced when the credential HTTP call is made
(referencing validateGerritURL and validateGerritTokenFn). Fix by resolving the
hostname once and either: (A) pass the resolved IP into the HTTP request (pin
the IP and set Host header to original hostname) so the request goes to that IP,
or (B) create a custom http.Transport with a DialContext that performs the IP
validation (reuse the same validation logic from validateGerritURL) immediately
before dialing and rejects connections to loopback/private/metadata addresses;
ensure the chosen approach blocks 169.254.169.254 and private ranges and uses
HTTPS scheme checks as before.
Comment on lines
+194
to
+203
| // 200 = valid, 401/403 = invalid | ||
| if resp.StatusCode == http.StatusOK { | ||
| return true, nil | ||
| } | ||
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { | ||
| return false, nil | ||
| } | ||
|
|
||
| // Other status codes - can't validate, assume valid to avoid false negatives | ||
| return true, nil |
Contributor
There was a problem hiding this comment.
Don’t treat unexpected Gerrit statuses as valid credentials.
Anything other than 401/403 currently falls through to success, so a wrong base URL, reverse-proxy error page, or server-side 5xx will be stored as a valid connection. Since /a/accounts/self is the validation contract, fail closed on non-200 responses.
Suggested fix
- // 200 = valid, 401/403 = invalid
- if resp.StatusCode == http.StatusOK {
- return true, nil
- }
- if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden {
- return false, nil
- }
-
- // Other status codes - can't validate, assume valid to avoid false negatives
- return true, nil
+ switch resp.StatusCode {
+ case http.StatusOK:
+ return true, nil
+ case http.StatusUnauthorized, http.StatusForbidden:
+ return false, nil
+ default:
+ return false, fmt.Errorf("unexpected Gerrit validation status: %s", resp.Status)
+ }📝 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
| // 200 = valid, 401/403 = invalid | |
| if resp.StatusCode == http.StatusOK { | |
| return true, nil | |
| } | |
| if resp.StatusCode == http.StatusUnauthorized || resp.StatusCode == http.StatusForbidden { | |
| return false, nil | |
| } | |
| // Other status codes - can't validate, assume valid to avoid false negatives | |
| return true, nil | |
| switch resp.StatusCode { | |
| case http.StatusOK: | |
| return true, nil | |
| case http.StatusUnauthorized, http.StatusForbidden: | |
| return false, nil | |
| default: | |
| return false, fmt.Errorf("unexpected Gerrit validation status: %s", resp.Status) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/integration_validation.go` around lines 194 -
203, The current validation in the function that checks resp.StatusCode (the
Gerrit /a/accounts/self validation) treats any non-401/403 status as valid,
which allows 5xx or other errors to be accepted; change the logic so only
http.StatusOK (200) returns true, and all other responses (including 5xx, 3xx,
etc.) return false (fail closed) while preserving error handling for the HTTP
request and returning nil error for authentication failures; update the block
that currently checks resp.StatusCode to explicitly return false, nil for any
status != http.StatusOK and keep the existing true, nil for http.StatusOK.
Contributor
Review Queue Status
Action needed: Resolve merge conflicts
|
sbauza
force-pushed
the
001-gerrit-integration
branch
from
68c04bd to
fc49c9a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
components/frontend/src/app/api/auth/gerrit/test/route.ts (1)
8-20:⚠️ Potential issue | 🟠 MajorStill normalize non-2xx upstream responses to JSON.
The timeout/catch helps, but a 401/500/HTML error from the backend is still passed through verbatim while forcing
Content-Type: application/json. That leaves the caller parsing invalid JSON instead of getting a stable{ valid: false, error }payload.Suggested fix
try { const resp = await fetch(`${BACKEND_URL}/auth/gerrit/test`, { method: 'POST', headers, body, signal: AbortSignal.timeout(15_000), }) const data = await resp.text() + if (!resp.ok) { + return Response.json( + { valid: false, error: data || 'Backend request failed' }, + { status: resp.status }, + ) + } return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } }) } catch (err) { - const message = err instanceof Error ? err.message : 'Backend request failed' - return new Response(JSON.stringify({ valid: false, error: message }), { - status: 502, - headers: { 'Content-Type': 'application/json' }, - }) + const isTimeout = err instanceof Error && err.name === 'TimeoutError' + return Response.json( + { valid: false, error: isTimeout ? 'Backend request timed out' : 'Backend request failed' }, + { status: isTimeout ? 504 : 502 }, + ) } }🤖 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/test/route.ts` around lines 8 - 20, The handler currently forwards non-2xx upstream responses verbatim while forcing Content-Type: application/json, causing JSON parse errors; inside the try after the fetch (the block that uses resp from the fetch call), check resp.ok and if false read resp.text() (or resp.json() guarded by try/catch), then return a JSON Response like { valid: false, error: <message/or-body> } with Content-Type: application/json and an appropriate status (use resp.status or a mapped status) instead of returning the raw HTML/body; update the error-path logic around the const resp = await fetch(...) and the subsequent return to normalize all non-2xx to the stable JSON payload.
🤖 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/integration_validation.go`:
- Around line 224-228: The cookie selection currently always allows suffix
matching; update the logic in the function that parses the Netscape cookie lines
(used by ValidateGerritToken) to respect the second column (the domain flag).
Read fields[1] (the include-subdomains flag), treat it case-insensitively
("TRUE" means domain cookie allowing subdomain suffix matches, "FALSE" means
host-only), and only perform strings.HasSuffix(host, "."+cookieHost) when the
flag indicates subdomain inclusion; for host-only cookies require exact
cookieHost == host before returning fields[5]+"="+fields[6].
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 354-367: The code attaches BOT_TOKEN to requests built from
BACKEND_API_URL without validating the URL; parse base (the BACKEND_API_URL
value) with urllib.parse.urlparse and reject/skip adding the Authorization
header (or abort the probe) unless the parsed URL has an expected cluster-local
host (e.g., scheme in {"http","https"} and hostname is localhost/127.0.0.1 or
endswith(".svc") or endswith(".cluster.local") or otherwise matches your cluster
host pattern); update the block that sets base/project/session_id and the logic
around bot/req.add_header so you only call req.add_header("Authorization", ...)
and perform _urllib_request.urlopen(req, ...) when the parsed base passes this
cluster-host validation.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 424-425: The Gerrit branch currently only logs PermissionError
from fetch_gerrit_credentials() (gerrit_instances) but should mirror other
providers by recording the failure for the post-refresh fail-fast logic; replace
the lone logger.warning with code that appends the error to the shared
auth_failures collection (e.g. auth_failures.append(("gerrit",
gerrit_instances))) and ensure Gerrit remains disabled (e.g. leave
gerrit_instances as exception/None) so the existing "abort after refresh" logic
will handle the failure rather than allowing the run to continue silently.
In `@specs/001-gerrit-integration/contracts/frontend-types.ts`:
- Around line 59-61: The spec defines the response type as
GerritInstancesListResponse but the implementation in gerrit-auth.ts uses
GerritInstancesResponse; update one to match the other for consistency—either
rename the implementation type GerritInstancesResponse to
GerritInstancesListResponse in gerrit-auth.ts (and all references) or rename the
spec type to GerritInstancesResponse so both the spec and the code use the same
identifier (ensure you update imports/usages of GerritInstancesResponse or
GerritInstancesListResponse accordingly).
In `@specs/001-gerrit-integration/contracts/gerrit-api.yaml`:
- Around line 180-194: The oneOf branches for authMethod (http_basic vs
git_cookies) allow extra fields from the other branch; update the schema so each
branch explicitly forbids unrelated fields—either add additionalProperties:
false to both branch schemas (the branches that set authMethod: http_basic and
authMethod: git_cookies) so http_basic cannot include gitcookiesContent and
git_cookies cannot include username/httpToken, or introduce a discriminator on
authMethod with explicit mapping to separate schemas to enforce exclusive
validation for username, httpToken and gitcookiesContent.
In `@specs/001-gerrit-integration/spec.md`:
- Line 125: Update the spec to replace the outdated generic MCP server
credential storage assumption (`mcp-server-credentials` with `serverName:userID`
keys) with the implemented per-user Secret model (`gerrit-credentials-{userID}`
Secrets that use `instanceName` keys); remove or replace the sentence at line
125 that references `mcp-server-credentials`/`serverName:userID`, and ensure all
references in the document and examples point to `gerrit-credentials-{userID}`
and `instanceName` naming/keys so follow-up work uses the per-user persistence
design.
---
Duplicate comments:
In `@components/frontend/src/app/api/auth/gerrit/test/route.ts`:
- Around line 8-20: The handler currently forwards non-2xx upstream responses
verbatim while forcing Content-Type: application/json, causing JSON parse
errors; inside the try after the fetch (the block that uses resp from the fetch
call), check resp.ok and if false read resp.text() (or resp.json() guarded by
try/catch), then return a JSON Response like { valid: false, error:
<message/or-body> } with Content-Type: application/json and an appropriate
status (use resp.status or a mapped status) instead of returning the raw
HTML/body; update the error-path logic around the const resp = await fetch(...)
and the subsequent return to normalize all non-2xx to the stable JSON payload.
🪄 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: 0164cb21-2a4d-4e4c-be3a-cb22263ac7ac
📒 Files selected for processing (14)
components/backend/handlers/gerrit_auth.gocomponents/backend/handlers/gerrit_auth_test.gocomponents/backend/handlers/integration_validation.gocomponents/frontend/src/app/api/auth/gerrit/test/route.tscomponents/frontend/src/services/api/gerrit-auth.tscomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pydocs/internal/integrations/gerrit-integration.mdspecs/001-gerrit-integration/contracts/frontend-types.tsspecs/001-gerrit-integration/contracts/gerrit-api.yamlspecs/001-gerrit-integration/plan.mdspecs/001-gerrit-integration/quickstart.mdspecs/001-gerrit-integration/spec.mdspecs/001-gerrit-integration/tasks.md
components/runners/ambient-runner/ambient_runner/platform/auth.py
Outdated
Show resolved
Hide resolved
Comment on lines
+59
to
+61
| export interface GerritInstancesListResponse { | ||
| instances: GerritInstanceStatus[] | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Minor naming inconsistency with implementation.
The spec defines GerritInstancesListResponse but the implementation in gerrit-auth.ts uses GerritInstancesResponse. While the structures are identical, consider aligning the names for consistency between spec and implementation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/001-gerrit-integration/contracts/frontend-types.ts` around lines 59 -
61, The spec defines the response type as GerritInstancesListResponse but the
implementation in gerrit-auth.ts uses GerritInstancesResponse; update one to
match the other for consistency—either rename the implementation type
GerritInstancesResponse to GerritInstancesListResponse in gerrit-auth.ts (and
all references) or rename the spec type to GerritInstancesResponse so both the
spec and the code use the same identifier (ensure you update imports/usages of
GerritInstancesResponse or GerritInstancesListResponse accordingly).
Comment on lines
+180
to
+194
| oneOf: | ||
| - properties: | ||
| authMethod: | ||
| const: http_basic | ||
| username: | ||
| type: string | ||
| httpToken: | ||
| type: string | ||
| required: [username, httpToken] | ||
| - properties: | ||
| authMethod: | ||
| const: git_cookies | ||
| gitcookiesContent: | ||
| type: string | ||
| required: [gitcookiesContent] |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
The oneOf branches should explicitly exclude the other auth method's fields.
The oneOf pattern defines required fields per branch but doesn't prevent extra properties. A request with authMethod: 'http_basic' could still include gitcookiesContent at the schema level (though backend validation will reject it). Consider adding additionalProperties: false or using the discriminator keyword for stricter OpenAPI validation.
Example using discriminator
oneOf:
- properties:
authMethod:
const: http_basic
username:
type: string
httpToken:
type: string
required: [username, httpToken]
- properties:
authMethod:
const: git_cookies
gitcookiesContent:
type: string
required: [gitcookiesContent]
+ discriminator:
+ propertyName: authMethod
+ mapping:
+ http_basic: '#/components/schemas/GerritHttpBasicAuth'
+ git_cookies: '#/components/schemas/GerritGitCookiesAuth'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/001-gerrit-integration/contracts/gerrit-api.yaml` around lines 180 -
194, The oneOf branches for authMethod (http_basic vs git_cookies) allow extra
fields from the other branch; update the schema so each branch explicitly
forbids unrelated fields—either add additionalProperties: false to both branch
schemas (the branches that set authMethod: http_basic and authMethod:
git_cookies) so http_basic cannot include gitcookiesContent and git_cookies
cannot include username/httpToken, or introduce a discriminator on authMethod
with explicit mapping to separate schemas to enforce exclusive validation for
username, httpToken and gitcookiesContent.
Add Gerrit as a native integration in the Ambient Code Platform,
allowing users to connect Gerrit instances for code review workflows.
Backend:
- Add Gerrit auth handlers (connect, disconnect, status, list instances)
- Per-user K8s Secrets for credential storage (gerrit-credentials-{userID})
- SSRF protection with URL validation (scheme, DNS, IP range checks)
- Mixed auth rejection (discriminated union enforcement)
- Gerrit credential validation against /a/accounts/self endpoint
- Support for HTTP Basic and git_cookies authentication methods
- Deterministic API responses via sorted instance listing
- Integration status endpoint for Gerrit instances
- Credential test endpoint (POST /api/auth/gerrit/test)
Runner:
- Generate gerrit_config.json from fetched credentials at session startup
- Clean up stale config on credential refresh
- Support multiple Gerrit instances in MCP server configuration
Frontend:
- Add Gerrit integration card to Integrations page
- Support connecting/disconnecting Gerrit instances
- Display connection status and instance details
Tests:
- Comprehensive Ginkgo test suite for Gerrit auth handlers
- SSRF protection tests, mixed auth rejection tests
- Per-user secret isolation tests
Documentation:
- Internal integration guide for Gerrit setup
- Data model specification
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Fix SSRF DNS rebinding (TOCTOU): add ssrfSafeTransport with custom dialer that validates resolved IPs at connection time, preventing DNS rebinding attacks between validation and HTTP request - Fail closed on non-200 Gerrit responses in ValidateGerritToken instead of assuming valid (prevents storing invalid connections) - Add isValidUserID() check to GetGerritStatus, DisconnectGerrit, and ListGerritInstances for consistency with ConnectGerrit - Tighten test assertions: assert exact HTTP 200 instead of NotTo(Equal(400)) or BeElementOf(200, 404) - Add timeout (15s) and error handling to frontend test/route.ts - Use discriminated union types for GerritConnectRequest and GerritTestRequest in frontend API types - Fix stale Gerrit config: call generate_gerrit_config() even when backend returns empty instances list, to clean up old config - Add docstring clarifying MCP config regeneration timing - Fix documentation: per-user Secret naming, pinned MCP server revision, multi-instance UX consistency, dedicated test request schema in OpenAPI contract Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
- Honor gitcookie subdomain flag (column 2) in parseGitcookies: only allow subdomain matching when flag is TRUE, preventing credential scope widening on mistyped URLs - Add cluster URL validation to Gerrit MCP auth check fallback to prevent BOT_TOKEN exfiltration via user-overridden env vars - Propagate Gerrit PermissionError into auth_failures list for consistent fail-fast behavior with other providers - Align spec type name GerritInstancesListResponse → GerritInstancesResponse to match implementation - Update spec assumption to reflect per-user Secret model - Add discriminator keyword to OpenAPI oneOf schemas Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
sbauza
force-pushed
the
001-gerrit-integration
branch
from
6733fb4 to
7a32187
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (6)
components/backend/handlers/runtime_credentials.go (1)
514-530:⚠️ Potential issue | 🟠 MajorSort Gerrit instances for deterministic API responses.
The response order depends on iteration order from
listGerritCredentials, which may be non-deterministic. This contrasts withgetGerritStatusForUserinintegrations_status.go(lines 145-147) which sorts instances byInstanceNamebefore returning.Add sorting for consistency:
Proposed fix
+ "sort" "strings" @@ if len(instances) == 0 { c.JSON(http.StatusNotFound, gin.H{"error": "Gerrit credentials not configured"}) return } + sort.Slice(instances, func(i, j int) bool { + return instances[i].InstanceName < instances[j].InstanceName + }) + result := make([]gin.H, 0, len(instances))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/runtime_credentials.go` around lines 514 - 530, The response builds the instances slice from listGerritCredentials without a deterministic order; sort the instances by InstanceName before constructing result to match getGerritStatusForUser behavior. After obtaining instances (the variable named instances from listGerritCredentials), perform a stable sort on instances by creds.InstanceName, then iterate and build the result as shown (preserving the existing fields and authMethod branches). This ensures deterministic API responses for the handler in runtime_credentials.go.components/runners/ambient-runner/ambient_runner/platform/auth.py (1)
424-426:⚠️ Potential issue | 🟠 MajorPropagate Gerrit permission failures into the shared fail-fast path.
On Line 424, Gerrit auth exceptions are only logged.
PermissionErrorshould also be appended toauth_failureslike the other providers so refresh fails consistently instead of silently continuing with Gerrit disabled.Proposed fix
if isinstance(gerrit_instances, Exception): logger.warning(f"Failed to fetch Gerrit credentials: {gerrit_instances}") + if isinstance(gerrit_instances, PermissionError): + auth_failures.append(str(gerrit_instances)) else: try: from ambient_runner.bridges.claude.mcp import generate_gerrit_configAs per coding guidelines: "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around lines 424 - 426, The Gerrit auth failure path currently only logs exceptions (the block handling gerrit_instances) and must also propagate permission failures into the shared fail-fast list; update the block that checks isinstance(gerrit_instances, Exception) to detect PermissionError (or treat all Exception cases used by other providers) and append the PermissionError instance or a matching sentinel to auth_failures (the same list other providers use) before logging, so the refresh path fails consistently when Gerrit permissions are missing; refer to the variables gerrit_instances, auth_failures and the logger.warning call to locate and mirror the behavior used for other providers.specs/001-gerrit-integration/spec.md (1)
120-126:⚠️ Potential issue | 🟠 MajorUpdate the storage assumption to the per-user Secret model.
This still says Gerrit extends the generic
mcp-server-credentials/serverName:userIDscheme, but the rest of this PR implements per-usergerrit-credentials-{userID}Secrets keyed byinstanceName. Keeping both models in the spec will push future work toward the wrong persistence design.Suggested fix
-- The platform's existing generic MCP server credential storage pattern (`mcp-server-credentials` secret with `serverName:userID` keying) can be extended to support Gerrit. +- Gerrit uses a dedicated per-user Secret (`gerrit-credentials-{userID}`) with one entry per `instanceName`, instead of the generic `mcp-server-credentials` keying scheme.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-gerrit-integration/spec.md` around lines 120 - 126, The spec's storage assumption incorrectly states Gerrit will extend the generic `mcp-server-credentials` (`serverName:userID`) scheme; update it to state Gerrit uses the per-user Secret model instead: per-user Secrets named `gerrit-credentials-{userID}` with keys keyed by `instanceName`. Replace the bullet that references `mcp-server-credentials`/`serverName:userID` with a sentence describing the `gerrit-credentials-{userID}` Secret format and how `instanceName` is used as the key, so the spec matches the implemented persistence design.components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py (1)
354-367:⚠️ Potential issue | 🔴 CriticalValidate
BACKEND_API_URLbefore attaching the bot token.This fallback probe still builds a request from raw
BACKEND_API_URLand addsAuthorization: Bearer ..., so a user-controlled env override can exfiltrate the runner token during a Gerrit auth-status check.Suggested fix
+from urllib.parse import urlparse + ... import urllib.request as _urllib_request base = os.getenv("BACKEND_API_URL", "").rstrip("/") + parsed = urlparse(base) + if parsed.scheme not in {"http", "https"} or not parsed.hostname or not ( + parsed.hostname.endswith(".svc.cluster.local") + or parsed.hostname in {"localhost", "127.0.0.1"} + ): + logger.debug( + "Skipping Gerrit credential probe for non-cluster BACKEND_API_URL: %s", + base, + ) + return False, "Gerrit not configured - connect on Integrations page" project = os.getenv("PROJECT_NAME") or os.getenv( "AGENTIC_SESSION_NAMESPACE", "" ) session_id = os.getenv("SESSION_ID", "") if base and project and session_id: url = f"{base}/projects/{project.strip()}/agentic-sessions/{session_id}/credentials/gerrit" req = _urllib_request.Request(url, method="GET") - bot = (os.getenv("BOT_TOKEN") or "").strip() + bot = get_bot_token() if bot: req.add_header("Authorization", f"Bearer {bot}")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py` around lines 354 - 367, The code builds a request from BACKEND_API_URL and unconditionally attaches the BOT_TOKEN Authorization header; validate BACKEND_API_URL (the local variable base) before adding credentials by parsing it (e.g., with urllib.parse.urlparse) and ensuring scheme is http/https and netloc matches an allowed/trusted host or a configured ALLOWED_BACKEND_HOSTS list; if the check fails, do not add the Authorization header and skip the probe (or bail out), and only call req.add_header("Authorization", f"Bearer {bot}") when the parsed URL is trusted. Ensure you update the logic around base, bot, req, and req.add_header so the token is never sent to user-controlled BACKEND_API_URL values.specs/001-gerrit-integration/contracts/gerrit-api.yaml (1)
162-225:⚠️ Potential issue | 🟠 MajorModel the auth union with OpenAPI 3.0-compatible schemas.
This contract is declared as OpenAPI 3.0.3, but these
oneOfbranches rely onconst, which 3.0 tooling does not handle reliably. The branches also stay open, so mixed-auth payloads can still validate. As written, the schema does not enforce the backend'shttp_basicvsgit_cookiesexclusivity.Does OpenAPI 3.0.3 support the `const` keyword in Schema Objects, and what is the recommended way to model an exclusive discriminated union for `authMethod` in OpenAPI 3.0?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-gerrit-integration/contracts/gerrit-api.yaml` around lines 162 - 225, The schemas GerritConnectRequest and GerritTestRequest use oneOf with const (unreliable in OpenAPI 3.0) and leave branches open, so auth-method exclusivity isn't enforced; fix by extracting two concrete component schemas (e.g., GerritHttpBasic and GerritGitCookies) that set authMethod as a single-value enum ('http_basic' and 'git_cookies'), define their required fields (username/httpToken or gitcookiesContent), set additionalProperties: false to close the branches, then reference those components in GerritConnectRequest and GerritTestRequest via oneOf/$ref and add a discriminator on authMethod mapping to those component names to ensure deterministic, exclusive validation (use the component schema names in the discriminator mapping).components/backend/handlers/integration_validation.go (1)
241-245:⚠️ Potential issue | 🟠 MajorHonor the cookie-domain flag when matching gitcookies.
Line 244 always allows suffix matching, but the second Netscape-cookie column controls whether subdomains are allowed. That can send a host-only cookie to
sub.example.comand incorrectly validate the wrong Gerrit URL.Suggested fix
fields := strings.Split(line, "\t") if len(fields) >= 7 { cookieHost := strings.TrimPrefix(fields[0], ".") - if cookieHost == host || strings.HasSuffix(host, "."+cookieHost) { + includeSubdomains := strings.EqualFold(fields[1], "TRUE") + if cookieHost == host || (includeSubdomains && strings.HasSuffix(host, "."+cookieHost)) { return fields[5] + "=" + fields[6] } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/integration_validation.go` around lines 241 - 245, The code in integration_validation.go currently allows suffix matching for cookies regardless of the Netscape "flag" column; update the match logic that uses fields, cookieHost and host so that subdomain/suffix matching is allowed only when the cookie's include-subdomains flag (fields[1]) is set (e.g., "TRUE"), otherwise require an exact host match; perform a case-insensitive check for the flag and keep returning fields[5]+"="+fields[6] when the correct match is found.
🤖 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 37-39: The package-global mutable variable validateGerritTokenFn
should be removed and the validator injected into the handler that needs it: add
a field (e.g., GerritValidator or ValidateGerritToken func type) on the handler
struct used in gerrit_auth.go, replace all uses of validateGerritTokenFn with
that struct field (or pass it as an explicit parameter to ValidateGerritToken
where the handler calls it), update the handler constructor/initialization sites
to accept the validator dependency (defaulting to the real ValidateGerritToken
in production), and change tests (and integration setup) to provide a stub
validator via the handler constructor instead of reassigning the package
variable; apply the same refactor pattern to the validateGitLabConnectivityFn in
gitlab_auth.go.
- Around line 48-54: isPrivateOrBlocked() is too narrow and misses several IANA
special-use ranges (CGNAT 100.64.0.0/10, benchmark 198.18.0.0/15, full multicast
224.0.0.0/4, unspecified/documentation ranges, IPv6 special ranges, etc.), so
update isPrivateOrBlocked to explicitly reject all IANA special-use networks
instead of relying solely on net.IP helpers: create a list of net.IPNet
constants (0.0.0.0/8, 100.64.0.0/10, 127.0.0.0/8, 169.254.0.0/16, 192.0.0.0/24,
192.0.2.0/24, 198.18.0.0/15, 198.51.100.0/24, 203.0.113.0/24, 224.0.0.0/4,
255.255.255.255, and the IPv6 equivalents like ::/128, ::1/128, fc00::/7,
fe80::/10, ff00::/8, etc.), then check membership with Contains(ip) and return
true if any match; ensure validateGerritURL() and ssrfSafeTransport() use this
updated isPrivateOrBlocked for every resolved IP (both A/AAAA answers) so
hostnames resolving to these ranges are rejected.
In `@components/frontend/src/app/api/auth/gerrit/test/route.ts`:
- Around line 19-22: The handler currently returns raw err.message in the JSON
response (variable message) which can leak internal details; update the catch
block around the upstream call (the err handling that builds the Response with
JSON.stringify({ valid: false, error: message })) to instead return a fixed,
client-safe error string (e.g., "Upstream service error") and map specific error
types to appropriate status codes (detect timeouts or network errors on err and
return 504 for timeouts, 502 for generic upstream failures) while logging the
original err.message/server stack only to server logs (not to the client).
In `@components/frontend/src/components/gerrit-connection-card.tsx`:
- Around line 256-264: The icon-only toggle Button that calls
setShowToken(!showToken) lacks an accessible name; update the Button (the
component rendering the Eye/EyeOff icons, which uses showToken, setShowToken and
connectMutation.isPending) to include a dynamic accessible label (e.g.,
aria-label and/or title) that reflects its action such as aria-label={showToken
? "Hide token" : "Show token"} (or use aria-pressed semantics if preferred) so
screen reader users can understand the control while keeping the existing
disabled logic.
In `@components/frontend/src/services/api/gerrit-auth.ts`:
- Around line 31-40: GerritInstanceStatus is currently defined as a single
object with required fields but the backend sometimes returns a disconnected
shape { connected: false } only; update the TypeScript types to model a
discriminated union (e.g. a connected variant with instanceName, url,
authMethod, updatedAt and connected: true, and a disconnected variant with only
connected: false) and update GerritInstancesResponse to use
GerritInstanceStatus[]; locate and change the GerritInstanceStatus and
GerritInstancesResponse type declarations in gerrit-auth.ts and ensure any code
handling getGerritInstanceStatus() checks the discriminant (connected) before
accessing instanceName/url/authMethod/updatedAt.
In `@specs/001-gerrit-integration/contracts/frontend-types.ts`:
- Around line 1-61: The implementation currently redeclares the spec types
(GerritAuthMethod, GerritConnectRequest, GerritTestRequest,
GerritConnectResponse, GerritInstanceStatus, GerritTestResponse,
GerritInstancesListResponse) causing drift; fix by removing the duplicate type
declarations from the implementation (gerrit-auth.ts) and importing these named
types from this spec file instead, updating all local references to use the
imported names, exporting any types from the spec if the implementation needs to
re-export them, and adjust any import paths/tsconfig path aliases so the spec
file is the single source of truth for these types.
In `@specs/001-gerrit-integration/research.md`:
- Line 61: Update the spec to use per-user secret names to match the PR
objectives: change the documented Secret name from the fixed
`gerrit-credentials` to the per-user pattern `gerrit-credentials-{userID}` (or
similar variable placeholder) and confirm the label `ambient-code.io/provider:
gerrit` remains applied to each per-user secret; ensure any examples or
references (e.g., in the "Secret name" line) are updated to the `{userID}`
pattern so the spec and implementation are consistent.
---
Duplicate comments:
In `@components/backend/handlers/integration_validation.go`:
- Around line 241-245: The code in integration_validation.go currently allows
suffix matching for cookies regardless of the Netscape "flag" column; update the
match logic that uses fields, cookieHost and host so that subdomain/suffix
matching is allowed only when the cookie's include-subdomains flag (fields[1])
is set (e.g., "TRUE"), otherwise require an exact host match; perform a
case-insensitive check for the flag and keep returning fields[5]+"="+fields[6]
when the correct match is found.
In `@components/backend/handlers/runtime_credentials.go`:
- Around line 514-530: The response builds the instances slice from
listGerritCredentials without a deterministic order; sort the instances by
InstanceName before constructing result to match getGerritStatusForUser
behavior. After obtaining instances (the variable named instances from
listGerritCredentials), perform a stable sort on instances by
creds.InstanceName, then iterate and build the result as shown (preserving the
existing fields and authMethod branches). This ensures deterministic API
responses for the handler in runtime_credentials.go.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 354-367: The code builds a request from BACKEND_API_URL and
unconditionally attaches the BOT_TOKEN Authorization header; validate
BACKEND_API_URL (the local variable base) before adding credentials by parsing
it (e.g., with urllib.parse.urlparse) and ensuring scheme is http/https and
netloc matches an allowed/trusted host or a configured ALLOWED_BACKEND_HOSTS
list; if the check fails, do not add the Authorization header and skip the probe
(or bail out), and only call req.add_header("Authorization", f"Bearer {bot}")
when the parsed URL is trusted. Ensure you update the logic around base, bot,
req, and req.add_header so the token is never sent to user-controlled
BACKEND_API_URL values.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 424-426: The Gerrit auth failure path currently only logs
exceptions (the block handling gerrit_instances) and must also propagate
permission failures into the shared fail-fast list; update the block that checks
isinstance(gerrit_instances, Exception) to detect PermissionError (or treat all
Exception cases used by other providers) and append the PermissionError instance
or a matching sentinel to auth_failures (the same list other providers use)
before logging, so the refresh path fails consistently when Gerrit permissions
are missing; refer to the variables gerrit_instances, auth_failures and the
logger.warning call to locate and mirror the behavior used for other providers.
In `@specs/001-gerrit-integration/contracts/gerrit-api.yaml`:
- Around line 162-225: The schemas GerritConnectRequest and GerritTestRequest
use oneOf with const (unreliable in OpenAPI 3.0) and leave branches open, so
auth-method exclusivity isn't enforced; fix by extracting two concrete component
schemas (e.g., GerritHttpBasic and GerritGitCookies) that set authMethod as a
single-value enum ('http_basic' and 'git_cookies'), define their required fields
(username/httpToken or gitcookiesContent), set additionalProperties: false to
close the branches, then reference those components in GerritConnectRequest and
GerritTestRequest via oneOf/$ref and add a discriminator on authMethod mapping
to those component names to ensure deterministic, exclusive validation (use the
component schema names in the discriminator mapping).
In `@specs/001-gerrit-integration/spec.md`:
- Around line 120-126: The spec's storage assumption incorrectly states Gerrit
will extend the generic `mcp-server-credentials` (`serverName:userID`) scheme;
update it to state Gerrit uses the per-user Secret model instead: per-user
Secrets named `gerrit-credentials-{userID}` with keys keyed by `instanceName`.
Replace the bullet that references `mcp-server-credentials`/`serverName:userID`
with a sentence describing the `gerrit-credentials-{userID}` Secret format and
how `instanceName` is used as the key, so the spec matches the implemented
persistence design.
🪄 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: 82f50eae-f50a-47d4-83fc-d3d8e25e76cb
📒 Files selected for processing (34)
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/backend/tests/constants/labels.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/__tests__/integrations-panel.test.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
+37
to
+39
| // validateGerritTokenFn is the function used to validate Gerrit credentials. | ||
| // It is a variable so tests can replace it with a stub. | ||
| var validateGerritTokenFn = ValidateGerritToken |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "gerrit_auth.go" -o -name "gerrit_auth_test.go" -o -name "integration_validation.go" | head -20Repository: ambient-code/platform
Length of output: 215
🏁 Script executed:
echo "=== gerrit_auth.go (lines 30-50) ===" && sed -n '30,50p' ./components/backend/handlers/gerrit_auth.go && echo "" && echo "=== gerrit_auth_test.go (lines 25-50) ===" && sed -n '25,50p' ./components/backend/handlers/gerrit_auth_test.go && echo "" && echo "=== integration_validation.go (lines 270-280) ===" && sed -n '270,280p' ./components/backend/handlers/integration_validation.goRepository: ambient-code/platform
Length of output: 2247
🏁 Script executed:
rg "validateGerritTokenFn" -n --context 3Repository: ambient-code/platform
Length of output: 4641
🏁 Script executed:
head -30 ./components/backend/handlers/gerrit_auth_test.go && echo "..." && grep -n "import\|parallel\|Describe\|Context" ./components/backend/handlers/gerrit_auth_test.go | head -20Repository: ambient-code/platform
Length of output: 2706
🏁 Script executed:
find . -name "ginkgo.yml" -o -name ".ginkgo-config" -o -name "ginkgo_config.go" 2>/dev/null && grep -r "parallel\|concurrency" . --include="*.go" --include="*.yml" --include="*.yaml" 2>/dev/null | grep -i "ginkgo\|test" | head -10Repository: ambient-code/platform
Length of output: 364
🏁 Script executed:
grep -A 10 "unit-tests.yml" ./.github/workflows/unit-tests.yml 2>/dev/null | head -40Repository: ambient-code/platform
Length of output: 647
🏁 Script executed:
grep -n "go test\|ginkgo run\|ginkgo.*parallel" ./.github/workflows/unit-tests.ymlRepository: ambient-code/platform
Length of output: 128
🏁 Script executed:
find . -path ./node_modules -prune -o -name "Makefile" -o -name "main_test.go" -o -name "*_suite_test.go" | grep -E "\.(go|Makefile)$" | xargs grep -l "ginkgo\|SetupSuite" 2>/dev/null | head -5Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
ls -la ./components/backend/handlers/ | grep test && echo "---" && grep -n "parallel\|SetupTest\|SetupSuite" ./components/backend/handlers/gerrit_auth_test.go ./components/backend/handlers/integration_validation_test.go 2>/dev/null | head -20Repository: ambient-code/platform
Length of output: 1879
🏁 Script executed:
cat ./components/backend/handlers/backend_unit_test.goRepository: ambient-code/platform
Length of output: 5101
🏁 Script executed:
rg "var \w+ = \w+\s*$" ./components/backend/handlers/ --type go -B 1 | grep -A 1 "^--$\|func" | head -30Repository: ambient-code/platform
Length of output: 113
🏁 Script executed:
rg "^var \w+ = " ./components/backend/handlers/*.go | grep -v "regex\|Namespace\|Prefix\|timeout" | head -20Repository: ambient-code/platform
Length of output: 3434
🏁 Script executed:
rg "Fn\s*=" ./components/backend/handlers/*.go --type go -B 2 -A 2Repository: ambient-code/platform
Length of output: 4574
Eliminate the package-global validator override seam via dependency injection.
Lines 37–39 declare validateGerritTokenFn as a mutable package variable that test code reassigns in gerrit_auth_test.go (lines 36–40, 73) and integration_validation.go (line 282) invokes from the handler path. While the test properly saves and restores the original in AfterEach (line 73), the pattern remains fragile: it couples test setup directly to production code, assumes sequential execution (which would break if specs ran in parallel), and could leak state if an exception occurs between save and restore. A similar pattern exists with validateGitLabConnectivityFn in gitlab_auth.go. Inject the validator as a dependency into the handler or test fixture instead, decoupling test stubs from package-level mutation.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/gerrit_auth.go` around lines 37 - 39, The
package-global mutable variable validateGerritTokenFn should be removed and the
validator injected into the handler that needs it: add a field (e.g.,
GerritValidator or ValidateGerritToken func type) on the handler struct used in
gerrit_auth.go, replace all uses of validateGerritTokenFn with that struct field
(or pass it as an explicit parameter to ValidateGerritToken where the handler
calls it), update the handler constructor/initialization sites to accept the
validator dependency (defaulting to the real ValidateGerritToken in production),
and change tests (and integration setup) to provide a stub validator via the
handler constructor instead of reassigning the package variable; apply the same
refactor pattern to the validateGitLabConnectivityFn in gitlab_auth.go.
Comment on lines
+31
to
+40
| export type GerritInstanceStatus = { | ||
| connected: boolean | ||
| instanceName: string | ||
| url: string | ||
| authMethod: GerritAuthMethod | ||
| updatedAt: string | ||
| } | ||
|
|
||
| export type GerritInstancesResponse = { | ||
| instances: GerritInstanceStatus[] |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find the backend handler file
find . -name "gerrit_auth.go" -type fRepository: ambient-code/platform
Length of output: 109
🏁 Script executed:
# Find the frontend gerrit-auth.ts file
find . -name "gerrit-auth.ts" -type fRepository: ambient-code/platform
Length of output: 118
🏁 Script executed:
# Inspect frontend type definitions
cat -n components/frontend/src/services/api/gerrit-auth.ts | head -50Repository: ambient-code/platform
Length of output: 1732
🏁 Script executed:
# Inspect backend response at lines 257 and 266
cat -n components/backend/handlers/gerrit_auth.go | sed -n '250,275p'Repository: ambient-code/platform
Length of output: 1031
Model the status payload as a connected/disconnected union.
getGerritInstanceStatus() returns only { connected: false } when the backend has no stored credentials (lines 257 and 266 in components/backend/handlers/gerrit_auth.go), but GerritInstanceStatus currently requires instanceName, url, authMethod, and updatedAt unconditionally. This makes the client type unsound and hides the disconnected branch from the compiler.
Suggested type shape
+export type GerritConnectedInstance = {
+ connected: true
+ instanceName: string
+ url: string
+ authMethod: GerritAuthMethod
+ updatedAt: string
+}
+
-export type GerritInstanceStatus = {
- connected: boolean
- instanceName: string
- url: string
- authMethod: GerritAuthMethod
- updatedAt: string
-}
+export type GerritInstanceStatus = { connected: false } | GerritConnectedInstance
export type GerritInstancesResponse = {
- instances: GerritInstanceStatus[]
+ instances: GerritConnectedInstance[]
}📝 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
| export type GerritInstanceStatus = { | |
| connected: boolean | |
| instanceName: string | |
| url: string | |
| authMethod: GerritAuthMethod | |
| updatedAt: string | |
| } | |
| export type GerritInstancesResponse = { | |
| instances: GerritInstanceStatus[] | |
| export type GerritConnectedInstance = { | |
| connected: true | |
| instanceName: string | |
| url: string | |
| authMethod: GerritAuthMethod | |
| updatedAt: string | |
| } | |
| export type GerritInstanceStatus = { connected: false } | GerritConnectedInstance | |
| export type GerritInstancesResponse = { | |
| instances: GerritConnectedInstance[] | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/services/api/gerrit-auth.ts` around lines 31 - 40,
GerritInstanceStatus is currently defined as a single object with required
fields but the backend sometimes returns a disconnected shape { connected: false
} only; update the TypeScript types to model a discriminated union (e.g. a
connected variant with instanceName, url, authMethod, updatedAt and connected:
true, and a disconnected variant with only connected: false) and update
GerritInstancesResponse to use GerritInstanceStatus[]; locate and change the
GerritInstanceStatus and GerritInstancesResponse type declarations in
gerrit-auth.ts and ensure any code handling getGerritInstanceStatus() checks the
discriminant (connected) before accessing instanceName/url/authMethod/updatedAt.
Comment on lines
+1
to
+61
| /** | ||
| * Frontend TypeScript types for the Gerrit Integration. | ||
| * These extend the existing IntegrationsStatus type. | ||
| */ | ||
|
|
||
| // --- API Request/Response Types --- | ||
|
|
||
| export type GerritAuthMethod = 'http_basic' | 'git_cookies' | ||
|
|
||
| export type GerritConnectRequest = | ||
| | { | ||
| instanceName: string | ||
| url: string | ||
| authMethod: 'http_basic' | ||
| username: string | ||
| httpToken: string | ||
| } | ||
| | { | ||
| instanceName: string | ||
| url: string | ||
| authMethod: 'git_cookies' | ||
| gitcookiesContent: string | ||
| } | ||
|
|
||
| export type GerritTestRequest = | ||
| | { | ||
| url: string | ||
| authMethod: 'http_basic' | ||
| username: string | ||
| httpToken: string | ||
| } | ||
| | { | ||
| url: string | ||
| authMethod: 'git_cookies' | ||
| gitcookiesContent: string | ||
| } | ||
|
|
||
| export interface GerritConnectResponse { | ||
| message: string | ||
| instanceName: string | ||
| url: string | ||
| authMethod: GerritAuthMethod | ||
| } | ||
|
|
||
| export interface GerritInstanceStatus { | ||
| connected: boolean | ||
| instanceName: string | ||
| url: string | ||
| authMethod: GerritAuthMethod | ||
| updatedAt: string | ||
| } | ||
|
|
||
| export interface GerritTestResponse { | ||
| valid: boolean | ||
| message?: string | ||
| error?: string | ||
| } | ||
|
|
||
| export interface GerritInstancesListResponse { | ||
| instances: GerritInstanceStatus[] | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Spec types are not imported by implementation - drift risk.
The implementation in gerrit-auth.ts redeclares all these types independently rather than importing from this spec file. This creates a maintenance burden where changes must be synchronized manually between the spec and implementation, with no compile-time enforcement. Consider either:
- Having
gerrit-auth.tsimport types from this spec file, or - Removing this spec file and treating
gerrit-auth.tsas the source of truth.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@specs/001-gerrit-integration/contracts/frontend-types.ts` around lines 1 -
61, The implementation currently redeclares the spec types (GerritAuthMethod,
GerritConnectRequest, GerritTestRequest, GerritConnectResponse,
GerritInstanceStatus, GerritTestResponse, GerritInstancesListResponse) causing
drift; fix by removing the duplicate type declarations from the implementation
(gerrit-auth.ts) and importing these named types from this spec file instead,
updating all local references to use the imported names, exporting any types
from the spec if the implementation needs to re-export them, and adjust any
import paths/tsconfig path aliases so the spec file is the single source of
truth for these types.
- Expand isPrivateOrBlocked() to cover all IANA special-use IP ranges (CGNAT, benchmarking, documentation, multicast, reserved, unspecified) - Fix TypeScript build error: use `as const` assertions for discriminated union narrowing in gerrit-connection-card.tsx - Sanitize error messages in gerrit test route (don't expose raw upstream errors) - Add aria-label to password toggle button for accessibility - Fix secret naming in research.md to match per-user implementation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (4)
specs/001-gerrit-integration/research.md (1)
59-65:⚠️ Potential issue | 🟠 MajorAlign both storage-model sections with the implemented per-user Secret design.
These sections still describe a shared
gerrit-credentialssecret keyed byinstanceName:userID, but the code stores per-user secrets namedgerrit-credentials-{userID}with per-instance keys. Leaving both models here will misdirect follow-on cleanup, RBAC, and test work. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."Also applies to: 128-132
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-gerrit-integration/research.md` around lines 59 - 65, Update the storage-model documentation to match the implemented per-user Secret design: replace any mention of a shared secret keyed by "instanceName:userID" with the per-user Secret name pattern "gerrit-credentials-{userID}" and document that each Secret contains per-instance keys (instanceName -> credential payload) corresponding to the GerritCredentials struct (fields: UserID, URL, AuthMethod, Username, HTTPToken, GitcookiesContent, UpdatedAt). Ensure the three endpoints (ConnectGerrit, GetGerritStatus, DisconnectGerrit) and TestGerritConnection reference the per-user secret lookup, and reflect the session-scoped fetch path GET /api/projects/:project/agentic-sessions/:session/credentials/gerrit when describing how credentials are resolved; remove the obsolete shared-secret wording to avoid confusion for RBAC and tests.components/frontend/src/components/gerrit-connection-card.tsx (1)
256-264:⚠️ Potential issue | 🟠 MajorGive the password visibility toggle an accessible name.
This icon-only button still has no
aria-label, so screen-reader users cannot tell whether it will show or hide the HTTP password. Add a dynamic label, and ideallyaria-pressed, on the toggle. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/components/gerrit-connection-card.tsx` around lines 256 - 264, The password visibility toggle Button (uses showToken state and setShowToken, renders Eye/EyeOff, disabled by connectMutation.isPending) lacks accessible name—add a dynamic aria-label like "Show HTTP password" when showToken is false and "Hide HTTP password" when true, and add aria-pressed={showToken} so screen readers announce state; ensure the label updates in the onClick toggle and remains present even when disabled.components/backend/handlers/runtime_credentials.go (1)
514-528:⚠️ Potential issue | 🟠 MajorSort the returned Gerrit instances before serializing them.
This endpoint still emits whatever order
listGerritCredentials()returns, so identical requests can produce differentinstancesarrays and churn the generated runner config. Sort by a stable key before buildingresult. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/runtime_credentials.go` around lines 514 - 528, The returned Gerrit instances must be sorted deterministically before building the result; modify the handler to sort the slice returned by listGerritCredentials() (the variable `instances`) by a stable key such as `InstanceName` (use sort.SliceStable with a comparator using creds.InstanceName) before the for-loop that constructs `result`, so the `result` slice is emitted in a stable, reproducible order.components/frontend/src/services/api/gerrit-auth.ts (1)
31-37: 🧹 Nitpick | 🔵 TrivialConsider modeling disconnected state in
GerritInstanceStatus.The backend's status endpoint can return
{ connected: false }without other fields when credentials don't exist. The current type requires all fields unconditionally, which may cause TypeScript errors if the UI accessesinstanceName/url/etc. on a disconnected instance without first checkingconnected.This is a defensive typing improvement rather than a runtime bug, assuming the UI guards access.
Suggested type refinement
-export type GerritInstanceStatus = { - connected: boolean - instanceName: string - url: string - authMethod: GerritAuthMethod - updatedAt: string -} +export type GerritInstanceStatus = + | { connected: false } + | { + connected: true + instanceName: string + url: string + authMethod: GerritAuthMethod + updatedAt: string + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/services/api/gerrit-auth.ts` around lines 31 - 37, The GerritInstanceStatus type currently requires instanceName, url, authMethod and updatedAt unconditionally which breaks when backend returns only { connected: false }; change the type to model a disconnected vs connected union (e.g. a discriminated union like { connected: false } | { connected: true; instanceName: string; url: string; authMethod: GerritAuthMethod; updatedAt: string }) or make those fields optional and update call sites to check connected before accessing them; update the GerritInstanceStatus definition and any consumers of GerritInstanceStatus (e.g., UI components reading instanceName/url/authMethod) to handle the disconnected variant safely.
🤖 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 434-466: The function listGerritCredentials returns (nil, nil)
when the Kubernetes Secret is not found; document this explicit contract in the
listGerritCredentials comment (mention that callers must treat nil,nil as "not
found" and not an internal error) and update callers that conflate "not found"
with errors—specifically change integrations_status.go (which currently checks
if err != nil || len(instances) == 0) so it distinguishes err != nil (500) from
err == nil && len(instances) == 0 (404). Also add a short unit test for
listGerritCredentials to assert the nil,nil behavior and update any other
callers to follow the documented contract.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsx:
- Around line 10-11: Update the test to use the shared IntegrationsStatus type
and provide a complete mock for integrationsStatus so the component can access
all expected fields; specifically import IntegrationsStatus in
integrations-panel.test.tsx and replace the underspecified mock with an object
whose gerrit.instances include url, authMethod, updatedAt (in addition to
connected and instanceName) so calls like
integrationsStatus?.gerrit?.instances?.map((i) => i.url) and any rendering that
relies on authMethod/updatedAt will behave the same as production. Ensure the
mock variable name used by the test matches the component prop (e.g.,
integrationsStatus) and cast or construct it using the shared IntegrationsStatus
type to keep the shape strict.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 429-438: The try/except around generate_gerrit_config(...)
swallows failures—replace the current except block so that when
generate_gerrit_config (from ambient_runner.bridges.claude.mcp) raises, you
either re-raise the exception or explicitly mark the Gerrit refresh as failed
after clearing any stale config; i.e., ensure that on error you still clear
stale config (if gerrit_instances is empty) and then raise the original
exception (or call a failure handler) instead of only logging via
logger.warning, referencing generate_gerrit_config, gerrit_instances and the
surrounding Gerrit MCP config generation block.
- Around line 287-297: fetch_gerrit_credentials currently treats backend/network
failures (where _fetch_credential returns {}) as "no instances" by using
data.get("instances", []), which masks errors; update fetch_gerrit_credentials
to check the raw return from _fetch_credential for falsy or missing "instances"
and distinguish error responses (e.g., None or empty dict) from a valid empty
instances list by surfacing or raising/logging the backend error before
returning [], and apply the same pattern to the other Gerrit helper in the
421-436 range; reference _fetch_credential and RunnerContext while making sure
valid [] (explicit empty list from backend) is still returned unchanged but real
failures are logged/propagated.
In `@specs/001-gerrit-integration/data-model.md`:
- Around line 93-97: The fenced code blocks containing the ASCII diagrams (the
state transitions with "[Not Connected] --connect()--> [Connected]" and the ER
diagram lines "User (1) ---has many---> GerritCredentials (N)" /
"gerrit_config.json ... consumed by---> Gerrit MCP Server") must include a
language tag to satisfy markdownlint MD040; update both triple-backtick blocks
to start with ```text (i.e., change ``` to ```text) so the blocks containing
those diagrams are explicitly marked as text.
In `@specs/001-gerrit-integration/tasks.md`:
- Line 76: Update the task wording to remove ambiguity about .gitcookies
handling: in generate_gerrit_config(instances) specify that you must create
/tmp/gerrit-mcp/.gitcookies once and append (concatenate) the gitcookies content
from every instance that uses git_cookies auth (do not overwrite per-instance),
ensuring entries are separated appropriately; keep building gerrit_config.json
with gerrit_hosts mapping for each instance (name, external_url, authentication
with type/username/auth_token or gitcookies_path), write the final config to
/tmp/gerrit-mcp/gerrit_config.json, set GERRIT_CONFIG_PATH, and call
generate_gerrit_config from build_mcp_servers() before loading MCP config so the
concatenated .gitcookies and config are available.
- Line 6: Remove the phrase "Test tasks are omitted" from the specs text and
replace it with a requirement that appropriate tests must be included for
security-sensitive areas (auth, SSRF, secret isolation); update the sentence
around the Tests section to state that test tasks are required for
security-sensitive changes and reference the need for unit/integration/security
tests as applicable so reviewers and implementers must include test plans
alongside the feature work.
---
Duplicate comments:
In `@components/backend/handlers/runtime_credentials.go`:
- Around line 514-528: The returned Gerrit instances must be sorted
deterministically before building the result; modify the handler to sort the
slice returned by listGerritCredentials() (the variable `instances`) by a stable
key such as `InstanceName` (use sort.SliceStable with a comparator using
creds.InstanceName) before the for-loop that constructs `result`, so the
`result` slice is emitted in a stable, reproducible order.
In `@components/frontend/src/components/gerrit-connection-card.tsx`:
- Around line 256-264: The password visibility toggle Button (uses showToken
state and setShowToken, renders Eye/EyeOff, disabled by
connectMutation.isPending) lacks accessible name—add a dynamic aria-label like
"Show HTTP password" when showToken is false and "Hide HTTP password" when true,
and add aria-pressed={showToken} so screen readers announce state; ensure the
label updates in the onClick toggle and remains present even when disabled.
In `@components/frontend/src/services/api/gerrit-auth.ts`:
- Around line 31-37: The GerritInstanceStatus type currently requires
instanceName, url, authMethod and updatedAt unconditionally which breaks when
backend returns only { connected: false }; change the type to model a
disconnected vs connected union (e.g. a discriminated union like { connected:
false } | { connected: true; instanceName: string; url: string; authMethod:
GerritAuthMethod; updatedAt: string }) or make those fields optional and update
call sites to check connected before accessing them; update the
GerritInstanceStatus definition and any consumers of GerritInstanceStatus (e.g.,
UI components reading instanceName/url/authMethod) to handle the disconnected
variant safely.
In `@specs/001-gerrit-integration/research.md`:
- Around line 59-65: Update the storage-model documentation to match the
implemented per-user Secret design: replace any mention of a shared secret keyed
by "instanceName:userID" with the per-user Secret name pattern
"gerrit-credentials-{userID}" and document that each Secret contains
per-instance keys (instanceName -> credential payload) corresponding to the
GerritCredentials struct (fields: UserID, URL, AuthMethod, Username, HTTPToken,
GitcookiesContent, UpdatedAt). Ensure the three endpoints (ConnectGerrit,
GetGerritStatus, DisconnectGerrit) and TestGerritConnection reference the
per-user secret lookup, and reflect the session-scoped fetch path GET
/api/projects/:project/agentic-sessions/:session/credentials/gerrit when
describing how credentials are resolved; remove the obsolete shared-secret
wording to avoid confusion for RBAC and tests.
🪄 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: 58c2ee84-0609-43a0-85c4-921012ac6a74
📒 Files selected for processing (34)
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/backend/tests/constants/labels.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/__tests__/integrations-panel.test.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
+434
to
+466
| // listGerritCredentials returns all Gerrit instances for a user. | ||
| // Each user has their own Secret, so all entries belong to the user. | ||
| // Results are sorted by instance name for deterministic ordering. | ||
| func listGerritCredentials(ctx context.Context, userID string) ([]*GerritCredentials, error) { | ||
| if userID == "" { | ||
| return nil, fmt.Errorf("userID is required") | ||
| } | ||
|
|
||
| secretName := gerritSecretName(userID) | ||
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, secretName, v1.GetOptions{}) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| return nil, nil | ||
| } | ||
| return nil, err | ||
| } | ||
|
|
||
| var result []*GerritCredentials | ||
| for key, val := range secret.Data { | ||
| var creds GerritCredentials | ||
| if err := json.Unmarshal(val, &creds); err != nil { | ||
| log.Printf("Failed to parse Gerrit credentials for key %s: %v", key, err) | ||
| continue | ||
| } | ||
| result = append(result, &creds) | ||
| } | ||
|
|
||
| sort.Slice(result, func(i, j int) bool { | ||
| return result[i].InstanceName < result[j].InstanceName | ||
| }) | ||
|
|
||
| return result, nil | ||
| } |
Contributor
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
listGerritCredentials contract: (nil, nil) for "not found" is consistent but requires careful caller handling.
The function returns (nil, nil) when the secret doesn't exist (lines 445-446), which differs from a genuine error. Per the relevant code snippets:
runtime_credentials.gocorrectly distinguisheserr != nil(500) fromlen(instances) == 0(404)integrations_status.goconflates both withif err != nil || len(instances) == 0
This is intentional for the status endpoint, but document the contract clearly if other callers are added.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/handlers/gerrit_auth.go` around lines 434 - 466, The
function listGerritCredentials returns (nil, nil) when the Kubernetes Secret is
not found; document this explicit contract in the listGerritCredentials comment
(mention that callers must treat nil,nil as "not found" and not an internal
error) and update callers that conflate "not found" with errors—specifically
change integrations_status.go (which currently checks if err != nil ||
len(instances) == 0) so it distinguishes err != nil (500) from err == nil &&
len(instances) == 0 (404). Also add a short unit test for listGerritCredentials
to assert the nil,nil behavior and update any other callers to follow the
documented contract.
Comment on lines
10
to
11
| gerrit: { instances: Array<{ connected: boolean; instanceName: string }> }; | ||
| } | null; |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify whether local test-only integrations status shapes diverge from the shared API type.
rg -n --type=ts "type\\s+.*Integrations.*=\\s*\\{" components/frontend/src
rg -n --type=ts "export type IntegrationsStatus" components/frontend/src/services/api/integrations.tsRepository: ambient-code/platform
Length of output: 507
🏁 Script executed:
cat -n components/frontend/src/services/api/integrations.ts | head -30Repository: ambient-code/platform
Length of output: 838
🏁 Script executed:
cat -n components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsx | head -50Repository: ambient-code/platform
Length of output: 2104
🏁 Script executed:
cat -n components/frontend/src/services/api/integrations.ts | head -100Repository: ambient-code/platform
Length of output: 1571
🏁 Script executed:
cat -n components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsx | head -100Repository: ambient-code/platform
Length of output: 3745
Use the shared IntegrationsStatus type in tests to prevent rendering bugs.
The test mock significantly under-specifies the schema. Critically, the gerrit instances lack the url field that the component explicitly accesses on line 23 (integrationsStatus?.gerrit?.instances?.map((i) => i.url)), and gerrit instances are also missing authMethod and updatedAt fields. The test would pass with this incomplete structure while production data includes them, masking potential rendering issues as the code evolves.
Suggested fix
+import type { IntegrationsStatus } from '@/services/api/integrations';
@@
-type IntegrationsData = {
- github: { active: string | null };
- gitlab: { connected: boolean };
- jira: { connected: boolean };
- google: { connected: boolean };
- gerrit: { instances: Array<{ connected: boolean; instanceName: string }> };
-} | null;
+type IntegrationsData = IntegrationsStatus | null;📝 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
| gerrit: { instances: Array<{ connected: boolean; instanceName: string }> }; | |
| } | null; | |
| import type { IntegrationsStatus } from '@/services/api/integrations'; | |
| // ... other imports and code ... | |
| type IntegrationsData = IntegrationsStatus | null; |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsx
around lines 10 - 11, Update the test to use the shared IntegrationsStatus type
and provide a complete mock for integrationsStatus so the component can access
all expected fields; specifically import IntegrationsStatus in
integrations-panel.test.tsx and replace the underspecified mock with an object
whose gerrit.instances include url, authMethod, updatedAt (in addition to
connected and instanceName) so calls like
integrationsStatus?.gerrit?.instances?.map((i) => i.url) and any rendering that
relies on authMethod/updatedAt will behave the same as production. Ensure the
mock variable name used by the test matches the component prop (e.g.,
integrationsStatus) and cast or construct it using the shared IntegrationsStatus
type to keep the shape strict.
Comment on lines
+429
to
+438
| try: | ||
| from ambient_runner.bridges.claude.mcp import generate_gerrit_config | ||
|
|
||
| generate_gerrit_config(gerrit_instances if gerrit_instances else []) | ||
| if gerrit_instances: | ||
| logger.info("Generated Gerrit MCP config from backend credentials") | ||
| else: | ||
| logger.info("Cleared stale Gerrit MCP config (no instances)") | ||
| except Exception as e: | ||
| logger.warning(f"Failed to generate Gerrit config: {e}") |
Contributor
There was a problem hiding this comment.
Don’t swallow Gerrit config generation failures.
If generate_gerrit_config() throws, the runner only logs and continues. That leaves Gerrit in a stale or missing state with no surfaced failure, and this integration has no env-var fallback to recover from that. Bubble the error up, or mark the refresh as failed after clearing any stale config. As per coding guidelines, "Focus on major issues impacting performance, readability, maintainability and security. Avoid nitpicks and avoid verbosity."
🧰 Tools
🪛 Ruff (0.15.7)
[warning] 437-437: Do not catch blind exception: Exception
(BLE001)
[warning] 438-438: Logging statement uses f-string
(G004)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around
lines 429 - 438, The try/except around generate_gerrit_config(...) swallows
failures—replace the current except block so that when generate_gerrit_config
(from ambient_runner.bridges.claude.mcp) raises, you either re-raise the
exception or explicitly mark the Gerrit refresh as failed after clearing any
stale config; i.e., ensure that on error you still clear stale config (if
gerrit_instances is empty) and then raise the original exception (or call a
failure handler) instead of only logging via logger.warning, referencing
generate_gerrit_config, gerrit_instances and the surrounding Gerrit MCP config
generation block.
- Document listGerritCredentials nil,nil contract for callers
- Log errors separately in getGerritStatusForUser (don't conflate
errors with empty results)
- Use shared IntegrationsStatus type in integrations-panel tests with
complete mock fields (url, authMethod, updatedAt)
- Distinguish backend failures from empty Gerrit instances in runner:
raise RuntimeError when _fetch_credential returns {}, so callers
don't accidentally clear config when backend is unreachable
- Don't swallow generate_gerrit_config failures: surface through
auth_failures instead of just logging; catch specific exceptions
(OSError, ValueError) instead of bare Exception
- On non-auth Gerrit fetch failures, preserve existing config instead
of clearing it
- Fix checklist to acknowledge spec contains implementation details
in Assumptions section
- Add language tags to fenced code blocks in data-model.md (MD040)
- Remove "tests are omitted" from tasks.md for security-sensitive scope
- Clarify gitcookies concatenation behavior in tasks.md
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
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.
Summary
Add Gerrit as a native integration in the Ambient Code Platform, allowing users to connect Gerrit instances for code review workflows.
Backend
gerrit-credentials-{userID})/a/accounts/selfendpointRunner
gerrit_config.jsonfrom fetched credentials at session startupFrontend
Tests
Documentation
Test plan
cd components/backend && make test)gerrit_config.jsonfrom credentialsCo-Authored-By: Claude Opus 4.6 noreply@anthropic.com