feat: add Gerrit integration connector for code review workflows#1387
Merged
mergify[bot] merged 13 commits intomainfrom Apr 21, 2026
Merged
feat: add Gerrit integration connector for code review workflows#1387mergify[bot] merged 13 commits intomainfrom
mergify[bot] merged 13 commits intomainfrom
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
✅ Files skipped from review due to trivial changes (1)
📝 WalkthroughWalkthroughAdds a Gerrit integration: backend APIs (connect/test/list/status/disconnect) with SSRF protections and per-user K8s Secret storage, frontend UI/hooks and proxy routes, runner credential fetch and MCP config generation, feature-flag gating, docs/specs/tasks, and CI/installer/script updates. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Frontend
participant Backend as Backend
participant K8s as Kubernetes
participant Gerrit as Gerrit
Client->>Frontend: User tests credentials (url, auth)
Frontend->>Backend: POST /auth/gerrit/test
Backend->>Backend: validateGerritURL() (HTTPS, DNS/IP checks)
Backend->>Gerrit: GET /a/accounts/self (Basic or Cookie)
Gerrit-->>Backend: 200 / 401/403 / error
Backend-->>Frontend: {valid: true/false, message?}
alt valid
Frontend->>Backend: POST /auth/gerrit/connect (instanceName, creds)
Backend->>K8s: Read secret, merge entry, write (retry up to 3)
K8s-->>Backend: Secret updated
Backend-->>Frontend: 200 OK
else invalid
Backend-->>Frontend: validation error
end
sequenceDiagram
participant Runner as AmbientRunner
participant Backend as Backend
participant K8s as Kubernetes
participant MCP as MCPServer
Runner->>Backend: GET /projects/:p/.../credentials/gerrit
Backend->>K8s: Read user's Gerrit Secret
K8s-->>Backend: instances list
Backend-->>Runner: 200 {instances: [...]}
Runner->>Runner: generate_gerrit_config(instances) -> write /tmp/gerrit-mcp (gerrit_config.json, .gitcookies with 0o600)
Runner->>Runner: set GERRIT_CONFIG_PATH
Runner->>MCP: build_mcp_servers() includes gerrit when config exists
Runner->>MCP: check_mcp_authentication('gerrit')
MCP-->>Runner: auth OK if config present
🚥 Pre-merge checks | ✅ 6 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
jeremyeder
force-pushed
the
001-gerrit-integration
branch
from
dffb510 to
e91029f
Compare
![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: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
scripts/hooks/coderabbit-review-gate.sh (2)
43-61:⚠️ Potential issue | 🟠 MajorBlock PR on unhandled CodeRabbit CLI failures; current gate silently passes on crashes.
Line 43 suppresses exit status with
|| true, and onlyrate_limit/autherrors are handled. If coderabbit crashes (timeout, runtime panic, missing dependency), it exits non-zero with non-JSON output, which grep and jq silently ignore—the script then reaches line 83 and passes anyway.Capture the exit code and block on non-zero exits not explicitly allowed:
Proposed fix
-CR_OUTPUT=$(cd "$REPO_ROOT" && coderabbit review --agent --base "$BASE_BRANCH" 2>&1 || true) +CR_EXIT=0 +CR_OUTPUT=$(cd "$REPO_ROOT" && coderabbit review --agent --base "$BASE_BRANCH" 2>&1) || CR_EXIT=$? # Filter to valid JSON lines (--agent emits NDJSON plus non-JSON diagnostic lines) @@ -60,6 +62,11 @@ if [ "$CR_ERROR_TYPE" = "auth" ]; then exit 0 fi +if [ "$CR_EXIT" -ne 0 ] && [ "$CR_ERROR_TYPE" != "rate_limit" ] && [ "$CR_ERROR_TYPE" != "auth" ]; then + echo "Review Gate: CodeRabbit failed (exit $CR_EXIT); blocking to avoid false pass" >&2 + echo "$CR_OUTPUT" >&2 + exit 2 +fi + # Check for blocking findings🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/coderabbit-review-gate.sh` around lines 43 - 61, The script currently silences the coderabbit command with "|| true" and only checks CR_ERROR_TYPE for rate_limit/auth, letting other failures pass; replace that by capturing the coderabbit exit code and output (remove "|| true"), e.g., run CR_OUTPUT=$(cd "$REPO_ROOT" && coderabbit review --agent --base "$BASE_BRANCH" 2>&1); CR_EXIT_CODE=$?; then if CR_EXIT_CODE != 0 and CR_ERROR_TYPE is not "rate_limit" or "auth" (and/or CR_JSON is empty) print a clear error using CR_OUTPUT/CR_JSON and exit non-zero to block the PR; keep the existing special-case handling for CR_ERROR_TYPE="rate_limit" and "auth" to allow creation.
27-32:⚠️ Potential issue | 🟠 MajorFail closed when git history is unavailable; current logic bypasses the gate in shallow clones.
Lines 27–31: Both diffs can fail in shallow CI clones (missing BASE_BRANCH or HEAD~1), leaving
CHANGED_FILESempty. The|| truemasks both failures, causing exit 0 (pass) when you should fail closed. In this ambiguous state, the gate cannot verify coverage yet silently approves the PR.Change exit 0 to exit 2 and add robust fallbacks to detect changes even in shallow clones:
Proposed fix
-# Skip if no changed files (gracefully handle shallow CI clones where HEAD~1 or base may not exist) -CHANGED_FILES=$(git diff "$BASE_BRANCH"...HEAD --name-only 2>/dev/null || git diff HEAD~1 --name-only 2>/dev/null || true) +# Determine changed files with resilient fallbacks +CHANGED_FILES=$(git diff "$BASE_BRANCH"...HEAD --name-only 2>/dev/null || \ + git diff HEAD~1..HEAD --name-only 2>/dev/null || \ + git show --pretty="" --name-only HEAD 2>/dev/null || true) if [ -z "$CHANGED_FILES" ]; then - echo "Review Gate: no changed files, allowing PR creation" >&2 - exit 0 + echo "Review Gate: unable to determine changed files (missing refs/shallow clone); blocking" >&2 + exit 2 fi🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/hooks/coderabbit-review-gate.sh` around lines 27 - 32, The current logic assigns CHANGED_FILES using two git diff fallbacks and then exits 0 when CHANGED_FILES is empty, which lets shallow CI clones bypass the gate; change the behavior to fail closed: replace the final exit 0 with exit 2 and add robust fallbacks before computing CHANGED_FILES—attempt to fetch the base branch and unshallow history (e.g., git fetch --no-tags --prune --depth=50 origin "$BASE_BRANCH" || git fetch --unshallow || true), verify HEAD and BASE_BRANCH exist with git rev-parse, and only then run the git diff commands (still capturing errors); ensure CHANGED_FILES remains empty only when there truly are no changes, otherwise treat any inability to determine changes as an error and exit 2.
🤖 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 214-230: The handler currently only rejects the
httpToken+gitcookiesContent mix but still allows other invalid mixes (e.g.,
authMethod "git_cookies" with a username), causing inconsistent validation
between connect and test; extract a shared validator (e.g.,
validateGerritAuth(req *GerritAuthRequest) error) that enforces exactly one
valid auth shape: for gerritAuthHTTPBasic require username AND HTTPToken and
ensure GitcookiesContent is empty; for gerritAuthGitCookies require
GitcookiesContent and ensure username and HTTPToken are empty; return
descriptive errors on violations and call this validator from both the Gerrit
connect handler and the test endpoint (and from integration_validation.go) so
both agree on valid payloads.
- Around line 405-461: storeGerritCredentials (and the other Secret helper
functions in this file) still use the global K8sClient; change them to accept
the request-scoped Kubernetes client instead and use that for all Secret
operations. Specifically, update storeGerritCredentials signature to take a k8s
client (e.g., kubernetes.Interface or the same type returned by
GetK8sClientsForRequest), replace all K8sClient.CoreV1() calls with the injected
client.CoreV1(), and update callers in the handlers to call
GetK8sClientsForRequest(c) and pass the returned client through; do the same for
the other Secret helpers referenced (the functions around the other ranges) so
no Secret read/write uses the global K8sClient.
- Around line 587-606: Normalize cookieHost by trimming any leading '.'
characters before performing the host match: in the block using cookieHost (the
variables cookieHost, subdomainFlag and matched in gerrit_auth.go), call a trim
(e.g. strings.TrimPrefix or strings.TrimLeft) to remove a leading '.' from
cookieHost, then use that normalized value for both the exact equality check and
the subdomain suffix check (strings.HasSuffix(targetHost,
"."+normalizedCookieHost)); return the cookie using the original
cookieName/cookieValue as before.
In `@components/frontend/src/app/api/auth/gerrit/`[instanceName]/status/route.ts:
- Around line 11-17: The proxy route currently awaits
fetch(`${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`)
and directly returns resp.text(), which can hang or throw on slow/unreachable
backends; wrap the fetch in a cancellable timeout (e.g., AbortController with a
short timeout), catch fetch/network errors and non-2xx responses, and on any
failure return a JSON Response with status 502 and an explanatory body
(Content-Type: application/json). Update the code around the fetch call and the
final Response creation (references: the fetch invocation, resp, and the
returned Response) to ensure timeouts and exceptions produce a controlled 502
JSON response instead of propagating errors.
In `@components/frontend/src/app/api/auth/gerrit/connect/route.ts`:
- Around line 8-12: The fetch in the Gerrit connect handler (the call assigning
to resp) lacks a timeout signal and can hang; update the fetch options to
include signal: AbortSignal.timeout(15000) (same pattern as test/route.ts) and
add controlled error handling for an abort/timeout (detect the AbortError and
return an appropriate error response) so the handler returns promptly on
upstream stalls; apply the same change to the other Gerrit proxy routes noted
(`instances/route.ts`, `[instanceName]/disconnect/route.ts`,
`[instanceName]/status/route.ts`) ensuring each fetch call includes
AbortSignal.timeout(15000) and catches/handles abort errors.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsx:
- Around line 71-72: The test in integrations-panel.test.tsx hardcodes the old
denominator and aggregate count ('4/5') which ignores the newly added Gerrit
integration; update the expectation to the correct aggregate string ('5/6') and
add/assert the Gerrit card/presence in the same test (replace
screen.getByText('4/5') with screen.getByText('5/6') and include an assertion
that the Gerrit integration label/card is rendered) so the suite validates the
new denominator and Gerrit visibility.
In `@components/frontend/src/services/api/integrations.ts`:
- Around line 42-44: The frontend type declares gerrit.connected as required but
the backend getGerritStatusForUser returns only instances, so change the model
to avoid the runtime contract gap: update the type that defines gerrit (in
integrations.ts) to make connected optional (connected?: boolean) or remove the
field and compute it where needed (e.g., derive connected = (instances &&
instances.length > 0) inside the function/component that reads
getGerritStatusForUser); also search for usages of gerrit.connected and adjust
them to handle undefined (use the derived boolean or optional chaining/defaults)
to ensure UI state is computed safely.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 304-314: Replace the fixed config directory usage (config_dir,
config_file, gitcookies_file) that points to "/tmp/gerrit-mcp" with a secure
per-run temp directory (e.g., use tempfile.mkdtemp or
tempfile.TemporaryDirectory) created with restrictive permissions, set
GERRIT_CONFIG_PATH to that per-run path, write credentials there, and ensure
deterministic cleanup of only that temp directory (do not rmtree a global path).
Also ensure code paths that early-return when not instances remove the per-run
env var without touching unrelated filesystem locations.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 454-467: The current logic preserves an old Gerrit config on
non-auth errors which can leak another user's credentials; when gerrit_creds is
an Exception always clear the stored config. Change the branch handling for
gerrit_creds so that on any Exception you call generate_gerrit_config([]) to
wipe config, while still appending to auth_failures only when
isinstance(gerrit_creds, PermissionError); keep
generate_gerrit_config(gerrit_creds) in the else branch for successful fetches.
Ensure you reference gerrit_creds, auth_failures, and generate_gerrit_config in
the updated logic.
- Around line 281-296: The function fetch_gerrit_credentials currently treats a
dict response as a single Gerrit instance, but _fetch_credential returns an
object with an "instances" key; update fetch_gerrit_credentials to check for a
dict containing "instances" and return data["instances"] (ensuring it's a list
of dicts), keep the existing branch for when data is already a list, and return
[] for any other shapes; reference the fetch_gerrit_credentials and
_fetch_credential symbols when making the change.
In `@components/runners/ambient-runner/Dockerfile`:
- Around line 46-47: The Dockerfile currently runs the remote installer directly
via the RUN that pipes https://cli.coderabbit.ai/install.sh into sh (using
CODERABBIT_INSTALL_DIR=/usr/local/bin), which is a supply-chain risk; instead
pin a specific CodeRabbit release (introduce a
CODERABBIT_VERSION/CODERABBIT_SHA256), download the corresponding installer
artifact (or the released binary) to /tmp with curl, fetch the matching checksum
file, verify the checksum (sha256sum -c or equivalent) and fail the build if it
doesn't match, then install the verified artifact into CODERABBIT_INSTALL_DIR
(or move the binary into /usr/local/bin) and remove temporary files; update the
existing RUN that references CODERABBIT_INSTALL_DIR and the remote install.sh to
perform these steps.
---
Outside diff comments:
In `@scripts/hooks/coderabbit-review-gate.sh`:
- Around line 43-61: The script currently silences the coderabbit command with
"|| true" and only checks CR_ERROR_TYPE for rate_limit/auth, letting other
failures pass; replace that by capturing the coderabbit exit code and output
(remove "|| true"), e.g., run CR_OUTPUT=$(cd "$REPO_ROOT" && coderabbit review
--agent --base "$BASE_BRANCH" 2>&1); CR_EXIT_CODE=$?; then if CR_EXIT_CODE != 0
and CR_ERROR_TYPE is not "rate_limit" or "auth" (and/or CR_JSON is empty) print
a clear error using CR_OUTPUT/CR_JSON and exit non-zero to block the PR; keep
the existing special-case handling for CR_ERROR_TYPE="rate_limit" and "auth" to
allow creation.
- Around line 27-32: The current logic assigns CHANGED_FILES using two git diff
fallbacks and then exits 0 when CHANGED_FILES is empty, which lets shallow CI
clones bypass the gate; change the behavior to fail closed: replace the final
exit 0 with exit 2 and add robust fallbacks before computing
CHANGED_FILES—attempt to fetch the base branch and unshallow history (e.g., git
fetch --no-tags --prune --depth=50 origin "$BASE_BRANCH" || git fetch
--unshallow || true), verify HEAD and BASE_BRANCH exist with git rev-parse, and
only then run the git diff commands (still capturing errors); ensure
CHANGED_FILES remains empty only when there truly are no changes, otherwise
treat any inability to determine changes as an error and exit 2.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 0af0986e-4165-4c17-9c18-8c6a703852ae
📒 Files selected for processing (27)
.github/workflows/coderabbit-smoke-test.yml.specify/specs/gerrit-integration/plan.md.specify/specs/gerrit-integration/spec.md.specify/specs/gerrit-integration/tasks.mdcomponents/backend/handlers/gerrit_auth.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/integrations_status.gocomponents/backend/handlers/runtime_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/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/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/manifests/base/core/flags.jsoncomponents/runners/ambient-runner/Dockerfilecomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/__init__.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/tests/test_shared_session_credentials.pyscripts/hooks/coderabbit-review-gate.sh
Comment on lines
+405
to
+461
| // storeGerritCredentials stores Gerrit credentials for a single instance in the user's Secret | ||
| func storeGerritCredentials(ctx context.Context, creds *GerritCredentials) error { | ||
| if creds == nil || creds.UserID == "" || creds.InstanceName == "" { | ||
| return fmt.Errorf("invalid credentials payload") | ||
| } | ||
|
|
||
| secretName := gerritSecretName(creds.UserID) | ||
|
|
||
| for i := 0; i < 3; i++ { // retry on conflict | ||
| secret, err := K8sClient.CoreV1().Secrets(Namespace).Get(ctx, secretName, v1.GetOptions{}) | ||
| if err != nil { | ||
| if errors.IsNotFound(err) { | ||
| // Create Secret | ||
| secret = &corev1.Secret{ | ||
| ObjectMeta: v1.ObjectMeta{ | ||
| Name: secretName, | ||
| Namespace: Namespace, | ||
| Labels: map[string]string{ | ||
| "app": "ambient-code", | ||
| "ambient-code.io/provider": "gerrit", | ||
| }, | ||
| }, | ||
| Type: corev1.SecretTypeOpaque, | ||
| Data: map[string][]byte{}, | ||
| } | ||
| created, cerr := K8sClient.CoreV1().Secrets(Namespace).Create(ctx, secret, v1.CreateOptions{}) | ||
| if cerr != nil { | ||
| if errors.IsAlreadyExists(cerr) { | ||
| continue // retry — concurrent create | ||
| } | ||
| return fmt.Errorf("failed to create Secret: %w", cerr) | ||
| } | ||
| secret = created | ||
| } else { | ||
| return fmt.Errorf("failed to get Secret: %w", err) | ||
| } | ||
| } | ||
|
|
||
| if secret.Data == nil { | ||
| secret.Data = map[string][]byte{} | ||
| } | ||
|
|
||
| b, err := json.Marshal(creds) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to marshal credentials: %w", err) | ||
| } | ||
| secret.Data[creds.InstanceName] = b | ||
|
|
||
| if _, uerr := K8sClient.CoreV1().Secrets(Namespace).Update(ctx, secret, v1.UpdateOptions{}); uerr != nil { | ||
| if errors.IsConflict(uerr) { | ||
| continue // retry | ||
| } | ||
| return fmt.Errorf("failed to update Secret: %w", uerr) | ||
| } | ||
| return nil | ||
| } | ||
| return fmt.Errorf("failed to update Secret after retries") |
Contributor
There was a problem hiding this comment.
Thread the request-scoped K8s client into these Secret helpers.
The handlers do call GetK8sClientsForRequest(c), but every Secret read/write still executes through the global K8sClient. That means all user-facing Gerrit API operations are ultimately running with backend service-account privileges instead of the request-scoped client.
As per coding guidelines, User-facing API ops MUST use GetK8sClientsForRequest(c), never the backend service account.
Also applies to: 465-519, 523-562
🤖 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 405 - 461,
storeGerritCredentials (and the other Secret helper functions in this file)
still use the global K8sClient; change them to accept the request-scoped
Kubernetes client instead and use that for all Secret operations. Specifically,
update storeGerritCredentials signature to take a k8s client (e.g.,
kubernetes.Interface or the same type returned by GetK8sClientsForRequest),
replace all K8sClient.CoreV1() calls with the injected client.CoreV1(), and
update callers in the handlers to call GetK8sClientsForRequest(c) and pass the
returned client through; do the same for the other Secret helpers referenced
(the functions around the other ranges) so no Secret read/write uses the global
K8sClient.
Comment on lines
+587
to
+606
| cookieHost := fields[0] | ||
| subdomainFlag := strings.ToUpper(fields[1]) | ||
| cookieName := fields[5] | ||
| cookieValue := fields[6] | ||
|
|
||
| var matched bool | ||
| if subdomainFlag == "TRUE" { | ||
| // Wildcard: target host must be the cookie host or a subdomain of it | ||
| matched = targetHost == cookieHost || strings.HasSuffix(targetHost, "."+cookieHost) | ||
| } else { | ||
| // Exact match | ||
| matched = targetHost == cookieHost | ||
| } | ||
|
|
||
| if matched { | ||
| return fmt.Sprintf("%s=%s", cookieName, cookieValue), nil | ||
| } | ||
| } | ||
|
|
||
| return "", fmt.Errorf("no matching cookie found for host %s", targetHost) |
Contributor
There was a problem hiding this comment.
Normalize leading-dot cookie domains before matching.
Common Netscape-format gitcookies use domains like .googlesource.com. With the current logic, strings.HasSuffix(targetHost, "."+cookieHost) becomes ..googlesource.com, so valid wildcard cookies never match and standard Gerrit gitcookies auth fails.
Suggested fix
- cookieHost := fields[0]
+ cookieHost := strings.TrimPrefix(fields[0], ".")
subdomainFlag := strings.ToUpper(fields[1])
cookieName := fields[5]
cookieValue := fields[6]📝 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
| cookieHost := fields[0] | |
| subdomainFlag := strings.ToUpper(fields[1]) | |
| cookieName := fields[5] | |
| cookieValue := fields[6] | |
| var matched bool | |
| if subdomainFlag == "TRUE" { | |
| // Wildcard: target host must be the cookie host or a subdomain of it | |
| matched = targetHost == cookieHost || strings.HasSuffix(targetHost, "."+cookieHost) | |
| } else { | |
| // Exact match | |
| matched = targetHost == cookieHost | |
| } | |
| if matched { | |
| return fmt.Sprintf("%s=%s", cookieName, cookieValue), nil | |
| } | |
| } | |
| return "", fmt.Errorf("no matching cookie found for host %s", targetHost) | |
| cookieHost := strings.TrimPrefix(fields[0], ".") | |
| subdomainFlag := strings.ToUpper(fields[1]) | |
| cookieName := fields[5] | |
| cookieValue := fields[6] | |
| var matched bool | |
| if subdomainFlag == "TRUE" { | |
| // Wildcard: target host must be the cookie host or a subdomain of it | |
| matched = targetHost == cookieHost || strings.HasSuffix(targetHost, "."+cookieHost) | |
| } else { | |
| // Exact match | |
| matched = targetHost == cookieHost | |
| } | |
| if matched { | |
| return fmt.Sprintf("%s=%s", cookieName, cookieValue), nil | |
| } | |
| } | |
| return "", fmt.Errorf("no matching cookie found for host %s", targetHost) |
🤖 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 587 - 606, Normalize
cookieHost by trimming any leading '.' characters before performing the host
match: in the block using cookieHost (the variables cookieHost, subdomainFlag
and matched in gerrit_auth.go), call a trim (e.g. strings.TrimPrefix or
strings.TrimLeft) to remove a leading '.' from cookieHost, then use that
normalized value for both the exact equality check and the subdomain suffix
check (strings.HasSuffix(targetHost, "."+normalizedCookieHost)); return the
cookie using the original cookieName/cookieValue as before.
Comment on lines
+11
to
+17
| const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`, { | ||
| method: 'GET', | ||
| headers, | ||
| }) | ||
|
|
||
| const data = await resp.text() | ||
| return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } }) |
Contributor
There was a problem hiding this comment.
Add timeout + upstream failure handling in the proxy route
If the backend is slow/unreachable, this path can hang or throw an uncaught exception, which breaks the API contract for the UI. Return a controlled 502 JSON response on failure.
Proposed fix
export async function GET(
request: Request,
{ params }: { params: Promise<{ instanceName: string }> }
) {
const { instanceName } = await params
const headers = await buildForwardHeadersAsync(request)
- const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`, {
- method: 'GET',
- headers,
- })
-
- const data = await resp.text()
- return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+ try {
+ const resp = await fetch(
+ `${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`,
+ {
+ method: 'GET',
+ headers,
+ signal: AbortSignal.timeout(15000),
+ }
+ )
+
+ const data = await resp.text()
+ return new Response(data, {
+ status: resp.status,
+ headers: { 'Content-Type': resp.headers.get('content-type') ?? 'application/json' },
+ })
+ } catch {
+ return Response.json({ error: 'Gerrit upstream unavailable' }, { status: 502 })
+ }
}📝 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
| const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`, { | |
| method: 'GET', | |
| headers, | |
| }) | |
| const data = await resp.text() | |
| return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } }) | |
| try { | |
| const resp = await fetch( | |
| `${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`, | |
| { | |
| method: 'GET', | |
| headers, | |
| signal: AbortSignal.timeout(15000), | |
| } | |
| ) | |
| const data = await resp.text() | |
| return new Response(data, { | |
| status: resp.status, | |
| headers: { 'Content-Type': resp.headers.get('content-type') ?? 'application/json' }, | |
| }) | |
| } catch { | |
| return Response.json({ error: 'Gerrit upstream unavailable' }, { status: 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/`[instanceName]/status/route.ts
around lines 11 - 17, The proxy route currently awaits
fetch(`${BACKEND_URL}/auth/gerrit/${encodeURIComponent(instanceName)}/status`)
and directly returns resp.text(), which can hang or throw on slow/unreachable
backends; wrap the fetch in a cancellable timeout (e.g., AbortController with a
short timeout), catch fetch/network errors and non-2xx responses, and on any
failure return a JSON Response with status 502 and an explanatory body
(Content-Type: application/json). Update the code around the fetch call and the
final Response creation (references: the fetch invocation, resp, and the
returned Response) to ensure timeouts and exceptions produce a controlled 502
JSON response instead of propagating errors.
Comment on lines
+8
to
+12
| const resp = await fetch(`${BACKEND_URL}/auth/gerrit/connect`, { | ||
| method: 'POST', | ||
| headers, | ||
| body, | ||
| }) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Verify Gerrit proxy routes that call fetch without a signal timeout.
fd -t f route.ts components/frontend/src/app/api/auth/gerrit | while read -r f; do
echo "=== $f ==="
rg -n "fetch\\(" "$f" -A8 -B2 || true
if rg -n "signal\\s*:" "$f" >/dev/null; then
echo "signal: present"
else
echo "signal: MISSING"
fi
echo
doneRepository: ambient-code/platform
Length of output: 2263
Add explicit timeout to all Gerrit proxy fetch calls
Missing AbortSignal.timeout() on fetch calls causes handlers to hang indefinitely if upstream stalls. Affects 4 routes:
components/frontend/src/app/api/auth/gerrit/connect/route.ts(line 8)components/frontend/src/app/api/auth/gerrit/instances/route.ts(line 7)components/frontend/src/app/api/auth/gerrit/[instanceName]/disconnect/route.ts(line 11)components/frontend/src/app/api/auth/gerrit/[instanceName]/status/route.ts(line 11)
test/route.ts correctly implements signal: AbortSignal.timeout(15000)—apply the same pattern to remaining routes and add controlled error handling.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/app/api/auth/gerrit/connect/route.ts` around lines 8
- 12, The fetch in the Gerrit connect handler (the call assigning to resp) lacks
a timeout signal and can hang; update the fetch options to include signal:
AbortSignal.timeout(15000) (same pattern as test/route.ts) and add controlled
error handling for an abort/timeout (detect the AbortError and return an
appropriate error response) so the handler returns promptly on upstream stalls;
apply the same change to the other Gerrit proxy routes noted
(`instances/route.ts`, `[instanceName]/disconnect/route.ts`,
`[instanceName]/status/route.ts`) ensuring each fetch call includes
AbortSignal.timeout(15000) and catches/handles abort errors.
Comment on lines
+42
to
+44
| gerrit: { | ||
| connected: boolean | ||
| instances?: Array<{ |
Contributor
There was a problem hiding this comment.
gerrit.connected is required in type but not guaranteed by backend payload
Line 43 makes connected mandatory, but backend getGerritStatusForUser currently returns only instances (no root connected). This creates a runtime contract gap and can miscompute UI state when connected is read.
Please align one side: either include connected in the backend response or make this field optional/derived in the frontend model.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/frontend/src/services/api/integrations.ts` around lines 42 - 44,
The frontend type declares gerrit.connected as required but the backend
getGerritStatusForUser returns only instances, so change the model to avoid the
runtime contract gap: update the type that defines gerrit (in integrations.ts)
to make connected optional (connected?: boolean) or remove the field and compute
it where needed (e.g., derive connected = (instances && instances.length > 0)
inside the function/component that reads getGerritStatusForUser); also search
for usages of gerrit.connected and adjust them to handle undefined (use the
derived boolean or optional chaining/defaults) to ensure UI state is computed
safely.
Comment on lines
+304
to
+314
| config_dir = Path("/tmp/gerrit-mcp") | ||
| config_file = config_dir / "gerrit_config.json" | ||
| gitcookies_file = config_dir / ".gitcookies" | ||
|
|
||
| # Clean up stale config | ||
| if config_dir.exists(): | ||
| shutil.rmtree(config_dir) | ||
|
|
||
| if not instances: | ||
| os.environ.pop("GERRIT_CONFIG_PATH", None) | ||
| return |
Contributor
There was a problem hiding this comment.
Use an isolated per-run secure temp directory for Gerrit secrets
Using a fixed /tmp/gerrit-mcp for credentials and deleting it recursively creates cross-run collision/leak risk for sensitive auth material.
Proposed fix
import json
import logging
import os
import shutil
+import tempfile
@@
def generate_gerrit_config(instances: list[dict]) -> None:
@@
- config_dir = Path("/tmp/gerrit-mcp")
+ old_config = os.environ.get("GERRIT_CONFIG_PATH", "")
+ if old_config:
+ old_dir = Path(old_config).parent
+ if old_dir.name.startswith("ambient-gerrit-"):
+ shutil.rmtree(old_dir, ignore_errors=True)
+
+ config_dir = Path(tempfile.mkdtemp(prefix="ambient-gerrit-"))
+ config_dir.chmod(0o700)🧰 Tools
🪛 Ruff (0.15.10)
[error] 304-304: Probable insecure usage of temporary file or directory: "/tmp/gerrit-mcp"
(S108)
🤖 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 304 - 314, Replace the fixed config directory usage (config_dir,
config_file, gitcookies_file) that points to "/tmp/gerrit-mcp" with a secure
per-run temp directory (e.g., use tempfile.mkdtemp or
tempfile.TemporaryDirectory) created with restrictive permissions, set
GERRIT_CONFIG_PATH to that per-run path, write credentials there, and ensure
deterministic cleanup of only that temp directory (do not rmtree a global path).
Also ensure code paths that early-return when not instances remove the per-run
env var without touching unrelated filesystem locations.
Comment on lines
+281
to
+296
| async def fetch_gerrit_credentials(context: RunnerContext) -> list[dict]: | ||
| """Fetch Gerrit credentials from backend API. | ||
|
|
||
| Returns list of instance dicts with: instanceName, url, authMethod, | ||
| username, httpToken, gitcookiesContent (depending on auth method). | ||
| """ | ||
| data = await _fetch_credential(context, "gerrit") | ||
| # The endpoint returns a list (multiple instances), not a single dict | ||
| if isinstance(data, list): | ||
| logger.info(f"Fetched {len(data)} Gerrit instance(s)") | ||
| return data | ||
| # If single instance returned (shouldn't happen but handle gracefully) | ||
| if isinstance(data, dict) and data: | ||
| logger.info("Fetched 1 Gerrit instance") | ||
| return [data] | ||
| return [] |
Contributor
There was a problem hiding this comment.
Fix Gerrit response-shape parsing (currently breaks instance extraction)
_fetch_credential(..., "gerrit") receives an object with instances, but this code wraps the entire dict as a single instance, so downstream config generation gets invalid fields.
Proposed fix
async def fetch_gerrit_credentials(context: RunnerContext) -> list[dict]:
@@
data = await _fetch_credential(context, "gerrit")
- # The endpoint returns a list (multiple instances), not a single dict
+ # Preferred shape: {"instances": [...]}
+ if isinstance(data, dict):
+ instances = data.get("instances", [])
+ if isinstance(instances, list):
+ return [inst for inst in instances if isinstance(inst, dict)]
+ if data:
+ # Backward-compatible fallback for legacy single-instance payloads
+ return [data]
+ return []
if isinstance(data, list):
logger.info(f"Fetched {len(data)} Gerrit instance(s)")
return data
- # If single instance returned (shouldn't happen but handle gracefully)
- if isinstance(data, dict) and data:
- logger.info("Fetched 1 Gerrit instance")
- return [data]
return []🤖 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 281 - 296, The function fetch_gerrit_credentials currently treats a dict
response as a single Gerrit instance, but _fetch_credential returns an object
with an "instances" key; update fetch_gerrit_credentials to check for a dict
containing "instances" and return data["instances"] (ensuring it's a list of
dicts), keep the existing branch for when data is already a list, and return []
for any other shapes; reference the fetch_gerrit_credentials and
_fetch_credential symbols when making the change.
Comment on lines
+454
to
+467
| # Gerrit credentials | ||
| if isinstance(gerrit_creds, Exception): | ||
| logger.warning(f"Failed to fetch Gerrit credentials: {gerrit_creds}") | ||
| if isinstance(gerrit_creds, PermissionError): | ||
| auth_failures.append(str(gerrit_creds)) | ||
| # Clear config on auth failure | ||
| from ambient_runner.bridges.claude.mcp import generate_gerrit_config | ||
|
|
||
| generate_gerrit_config([]) | ||
| # On network error, preserve existing config (don't clear) | ||
| else: | ||
| from ambient_runner.bridges.claude.mcp import generate_gerrit_config | ||
|
|
||
| generate_gerrit_config(gerrit_creds) |
Contributor
There was a problem hiding this comment.
Do not keep stale Gerrit config after fetch failures
At Line 463 the code intentionally preserves old config on non-auth errors. In shared-session/multi-user runs, that can reuse a previous user’s Gerrit credentials after a transient fetch failure.
Proposed fix
- if isinstance(gerrit_creds, Exception):
+ if isinstance(gerrit_creds, Exception):
logger.warning(f"Failed to fetch Gerrit credentials: {gerrit_creds}")
- if isinstance(gerrit_creds, PermissionError):
- auth_failures.append(str(gerrit_creds))
- # Clear config on auth failure
- from ambient_runner.bridges.claude.mcp import generate_gerrit_config
-
- generate_gerrit_config([])
- # On network error, preserve existing config (don't clear)
+ from ambient_runner.bridges.claude.mcp import generate_gerrit_config
+ generate_gerrit_config([]) # always clear to avoid stale credential reuse
+ if isinstance(gerrit_creds, PermissionError):
+ auth_failures.append(str(gerrit_creds))
else:
from ambient_runner.bridges.claude.mcp import generate_gerrit_config🤖 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 454 - 467, The current logic preserves an old Gerrit config on non-auth
errors which can leak another user's credentials; when gerrit_creds is an
Exception always clear the stored config. Change the branch handling for
gerrit_creds so that on any Exception you call generate_gerrit_config([]) to
wipe config, while still appending to auth_failures only when
isinstance(gerrit_creds, PermissionError); keep
generate_gerrit_config(gerrit_creds) in the else branch for successful fetches.
Ensure you reference gerrit_creds, auth_failures, and generate_gerrit_config in
the updated logic.
Comment on lines
+46
to
+47
| # Install CodeRabbit CLI (official install script, binary for current arch) | ||
| RUN curl -fsSL https://cli.coderabbit.ai/install.sh | CODERABBIT_INSTALL_DIR=/usr/local/bin sh |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's locate and examine the Dockerfile in question
find . -type f -path "*/components/runners/ambient-runner/Dockerfile" | head -1Repository: ambient-code/platform
Length of output: 111
🏁 Script executed:
# Read the Dockerfile to verify the exact content around lines 46-47
cat -n components/runners/ambient-runner/Dockerfile | sed -n '40,55p'Repository: ambient-code/platform
Length of output: 716
🏁 Script executed:
# Check the full context of the Dockerfile to understand the build strategy
cat -n components/runners/ambient-runner/DockerfileRepository: ambient-code/platform
Length of output: 3773
🏁 Script executed:
# Look for similar install patterns in other changed files/Dockerfiles in the repo
find . -type f -name "Dockerfile*" | head -20Repository: ambient-code/platform
Length of output: 500
Pin and verify the CodeRabbit installer before execution.
Line 47 executes a remote script as root without integrity verification, which is inconsistent with the version-pinned approach used for all other tools in this Dockerfile (GitHub CLI, GitLab CLI, npm/pip packages). This creates a supply-chain risk and breaks build reproducibility.
Recommend downloading, checksum-verifying, and executing a pinned installer artifact to match the pattern established for other dependencies:
Proposed hardening
+ARG CODERABBIT_INSTALLER_SHA256=<expected_sha256>
# Install CodeRabbit CLI (official install script, binary for current arch)
-RUN curl -fsSL https://cli.coderabbit.ai/install.sh | CODERABBIT_INSTALL_DIR=/usr/local/bin sh
+RUN curl -fsSLo /tmp/coderabbit-install.sh https://cli.coderabbit.ai/install.sh && \
+ echo "${CODERABBIT_INSTALLER_SHA256} /tmp/coderabbit-install.sh" | sha256sum -c - && \
+ CODERABBIT_INSTALL_DIR=/usr/local/bin sh /tmp/coderabbit-install.sh && \
+ rm -f /tmp/coderabbit-install.sh🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/runners/ambient-runner/Dockerfile` around lines 46 - 47, The
Dockerfile currently runs the remote installer directly via the RUN that pipes
https://cli.coderabbit.ai/install.sh into sh (using
CODERABBIT_INSTALL_DIR=/usr/local/bin), which is a supply-chain risk; instead
pin a specific CodeRabbit release (introduce a
CODERABBIT_VERSION/CODERABBIT_SHA256), download the corresponding installer
artifact (or the released binary) to /tmp with curl, fetch the matching checksum
file, verify the checksum (sha256sum -c or equivalent) and fail the build if it
doesn't match, then install the verified artifact into CODERABBIT_INSTALL_DIR
(or move the binary into /usr/local/bin) and remove temporary files; update the
existing RUN that references CODERABBIT_INSTALL_DIR and the remote install.sh to
perform these steps.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
| return false, fmt.Errorf("unsupported auth method: %s", authMethod) | ||
| } | ||
|
|
||
| resp, err := client.Do(req) |
jeremyeder
force-pushed
the
001-gerrit-integration
branch
from
304af89 to
3dd0837
Compare
Contributor
|
Caution Failed to replace (edit) comment. This is likely due to insufficient permissions or the comment being deleted. Error details |
Spec, plan, and tasks extracted from PR #1078's design, targeting reimplementation with current harness conventions. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Multi-instance Gerrit support with HTTP Basic and gitcookies auth, SSRF protection, per-user K8s Secret storage, and dynamic MCP server injection when credentials are configured. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…comments - Hoist blocked CIDR ranges to package-level var (avoid per-call alloc) - Add gerritAuthHTTPBasic/gerritAuthGitCookies constants - Use Create return value directly instead of re-fetching Secret - Remove WHAT comments, keep only non-obvious WHY Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add `connected: true` to ListGerritInstances response to match GerritInstanceStatus type (frontend rendered XCircle for all instances) - Gate ConnectGerrit, GetGerritStatus, DisconnectGerrit, ListGerritInstances, and TestGerritConnection behind gerrit.enabled feature flag server-side - Remove unused `status` prop from GerritConnectionCard Props type - Validate auth-specific fields in handleTest before calling mutate Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…t card - ssrfSafeTransport: capture the first allowed resolved IP and dial it directly via net.JoinHostPort(allowedIP, port) instead of re-dialing the original hostname, eliminating the DNS rebinding window between validation and connection - GerritConnectionCard: destructure isLoading/isError from useGerritInstances and render dedicated loading spinner, error message with retry button; guard instance list and empty state behind !isLoading && !isError Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
- integrations-panel.test.tsx: panel now has 5 integrations (CodeRabbit
added, always configured=true); update badge assertion from 3/4 to 4/5
and add CodeRabbit to card render test
- test_shared_session_credentials.py: _GH_WRAPPER_PATH/_GH_WRAPPER_DIR
were imported by value at module load (always ""), causing _cleanup()
to attempt unlink on Path("."); read live values from the module object
instead of the stale import-time bindings
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ckage `npm install -g coderabbit` installs an unrelated package that provides no `coderabbit` binary. The official CLI is distributed from https://cli.coderabbit.ai/install.sh (a Bun-based single binary). - smoke test: swap npm install for the official install script - Dockerfile: add CodeRabbit CLI install step (was missing from PR #1315) - review-gate.sh: update the install hint to match Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
In shallow clones (fetch-depth: 1), neither `main` nor `HEAD~1` exist, causing the script to crash with exit 128. Add `2>/dev/null || true` to the fallback so the gate exits 0 (no changed files) gracefully. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The CodeRabbit install script requires unzip to extract the binary. UBI10 does not include it by default. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
PR #1294 reintroduced context: components/runners, but PR #1260 had already changed the Dockerfile to COPY . (expecting the context to be the ambient-runner subdirectory). This caused pip3 install to fail with "Neither setup.py nor pyproject.toml found". Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
jeremyeder
force-pushed
the
001-gerrit-integration
branch
from
744b767 to
c54926a
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
components/runners/ambient-runner/Dockerfile (1)
46-47:⚠️ Potential issue | 🟠 MajorPin and verify the CodeRabbit installer before executing it.
This still pipes a remote script straight into
shas root. That breaks reproducibility and leaves the runner image exposed to installer-side compromise.Proposed hardening
+ARG CODERABBIT_INSTALLER_SHA256=<expected_sha256> # Install CodeRabbit CLI (official install script, binary for current arch) -RUN curl -fsSL https://cli.coderabbit.ai/install.sh | CODERABBIT_INSTALL_DIR=/usr/local/bin sh +RUN curl -fsSLo /tmp/coderabbit-install.sh https://cli.coderabbit.ai/install.sh && \ + echo "${CODERABBIT_INSTALLER_SHA256} /tmp/coderabbit-install.sh" | sha256sum -c - && \ + CODERABBIT_INSTALL_DIR=/usr/local/bin sh /tmp/coderabbit-install.sh && \ + rm -f /tmp/coderabbit-install.sh#!/bin/bash set -euo pipefail echo "Inspecting current CodeRabbit install step..." sed -n '42,50p' components/runners/ambient-runner/Dockerfile echo echo "Looking for checksum/signature verification near the installer..." rg -n 'coderabbit|install\.sh|sha256|checksum|gpg' components/runners/ambient-runner/Dockerfile🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/Dockerfile` around lines 46 - 47, Replace the inline "RUN curl ... | CODERABBIT_INSTALL_DIR=/usr/local/bin sh" installer step with a reproducible, verified download flow: download a specific CodeRabbit release artifact (pin a version) to a temporary file, fetch and verify its SHA256 checksum (or GPG signature) before executing, set CODERABBIT_INSTALL_DIR=/usr/local/bin and run the installer binary only after verification, and remove the installer file afterward; target the RUN step in the Dockerfile that contains the curl pipe and ensure the new sequence fails the build if checksum/signature verification does not match.components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py (1)
302-316:⚠️ Potential issue | 🟠 MajorUse a per-run private temp directory for Gerrit credential files.
/tmp/gerrit-mcpis process-global, and the unconditionalrmtree()means one refresh can wipe another in-flight session's credentials. That's a real cross-session leak/corruption risk for secret material.Proposed hardening
+import tempfile + def generate_gerrit_config(instances: list[dict]) -> None: """Generate Gerrit MCP server configuration from credential instances.""" - config_dir = Path("/tmp/gerrit-mcp") + old_config = os.environ.pop("GERRIT_CONFIG_PATH", "") + if old_config: + old_dir = Path(old_config).parent + if old_dir.name.startswith("ambient-gerrit-"): + shutil.rmtree(old_dir, ignore_errors=True) + + if not instances: + return + + config_dir = Path(tempfile.mkdtemp(prefix="ambient-gerrit-")) + config_dir.chmod(0o700) config_file = config_dir / "gerrit_config.json" gitcookies_file = config_dir / ".gitcookies" - - # Clean up stale config - if config_dir.exists(): - shutil.rmtree(config_dir) - - if not instances: - os.environ.pop("GERRIT_CONFIG_PATH", None) - return - - config_dir.mkdir(parents=True, exist_ok=True)🤖 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 302 - 316, The function generate_gerrit_config currently uses a process-global /tmp/gerrit-mcp and unconditionally rmtree() which can delete another in-flight session's credentials; change it to create a unique per-run temporary directory (e.g., via tempfile.mkdtemp or tempfile.TemporaryDirectory) instead of Path("/tmp/gerrit-mcp"), use that tempdir for config_dir/config_file/gitcookies_file, remove the global unconditional shutil.rmtree() and only clean up the directory owned by this invocation (or rely on the OS temp cleanup), and ensure any environment variable (GERRIT_CONFIG_PATH) is set to the new config_file; locate and update the generate_gerrit_config function and its config_dir/config_file/gitcookies_file references to implement this.
🤖 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 242-245: The client.Do(req) error is exposing raw dial/resolve
details via networkError(err) back to callers (used by TestGerritConnection);
change the behavior to avoid leaking resolver/transport info by returning a
sanitized error message (e.g., "request failed: connection error") to the client
and log the full original error internally instead. Update the code path around
resp, err := client.Do(req) (and/or adjust networkError) so that
transport/network errors like *net.OpError/*net.DNSError are not returned
verbatim but are replaced with a generic error for the response while retaining
the original error in internal logs.
- Around line 266-285: TestGerritConnection accepts mixed auth fields; before
calling validateGerritTokenFn you must explicitly reject payloads that include
fields from the non-selected auth mode: inspect req.AuthMethod and if it's the
HTTP/token mode ensure GitcookiesContent is empty, and if it's gitcookies mode
ensure Username and HTTPToken are empty (and respond with c.JSON(http.StatusOK,
gin.H{"valid": false, "error": "mixed auth fields not allowed"}) or similar).
Implement this check right after the validateGerritURL call and before invoking
validateGerritTokenFn; use the existing req struct field names (AuthMethod,
Username, HTTPToken, GitcookiesContent) to detect the invalid combinations.
---
Duplicate comments:
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 302-316: The function generate_gerrit_config currently uses a
process-global /tmp/gerrit-mcp and unconditionally rmtree() which can delete
another in-flight session's credentials; change it to create a unique per-run
temporary directory (e.g., via tempfile.mkdtemp or tempfile.TemporaryDirectory)
instead of Path("/tmp/gerrit-mcp"), use that tempdir for
config_dir/config_file/gitcookies_file, remove the global unconditional
shutil.rmtree() and only clean up the directory owned by this invocation (or
rely on the OS temp cleanup), and ensure any environment variable
(GERRIT_CONFIG_PATH) is set to the new config_file; locate and update the
generate_gerrit_config function and its config_dir/config_file/gitcookies_file
references to implement this.
In `@components/runners/ambient-runner/Dockerfile`:
- Around line 46-47: Replace the inline "RUN curl ... |
CODERABBIT_INSTALL_DIR=/usr/local/bin sh" installer step with a reproducible,
verified download flow: download a specific CodeRabbit release artifact (pin a
version) to a temporary file, fetch and verify its SHA256 checksum (or GPG
signature) before executing, set CODERABBIT_INSTALL_DIR=/usr/local/bin and run
the installer binary only after verification, and remove the installer file
afterward; target the RUN step in the Dockerfile that contains the curl pipe and
ensure the new sequence fails the build if checksum/signature verification does
not match.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 53ee01b6-839a-4cd3-b903-7a1f6d97b538
📒 Files selected for processing (28)
.github/workflows/coderabbit-smoke-test.yml.github/workflows/e2e.yml.specify/specs/gerrit-integration/plan.md.specify/specs/gerrit-integration/spec.md.specify/specs/gerrit-integration/tasks.mdcomponents/backend/handlers/gerrit_auth.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/integrations_status.gocomponents/backend/handlers/runtime_credentials.gocomponents/backend/routes.gocomponents/frontend/src/app/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/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/manifests/base/core/flags.jsoncomponents/runners/ambient-runner/Dockerfilecomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/__init__.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/tests/test_shared_session_credentials.pyscripts/hooks/coderabbit-review-gate.sh
✅ Files skipped from review due to trivial changes (6)
- components/manifests/base/core/flags.json
- components/runners/ambient-runner/ambient_runner/platform/init.py
- components/frontend/src/app/api/auth/gerrit/[instanceName]/status/route.ts
- components/frontend/src/app/api/auth/gerrit/test/route.ts
- components/frontend/src/services/queries/use-gerrit.ts
- components/frontend/src/components/gerrit-connection-card.tsx
🚧 Files skipped from review as they are similar to previous changes (13)
- components/frontend/src/app/integrations/IntegrationsClient.tsx
- components/backend/routes.go
- .github/workflows/coderabbit-smoke-test.yml
- components/frontend/src/app/api/auth/gerrit/instances/route.ts
- components/frontend/src/services/api/integrations.ts
- scripts/hooks/coderabbit-review-gate.sh
- components/frontend/src/app/api/auth/gerrit/connect/route.ts
- components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/tests/integrations-panel.test.tsx
- components/backend/handlers/integrations_status.go
- components/backend/handlers/runtime_credentials.go
- components/runners/ambient-runner/ambient_runner/platform/auth.py
- components/backend/handlers/gerrit_auth.go
- .specify/specs/gerrit-integration/tasks.md
Comment on lines
+242
to
+245
| resp, err := client.Do(req) | ||
| if err != nil { | ||
| return false, fmt.Errorf("request failed: %w", networkError(err)) | ||
| } |
Contributor
There was a problem hiding this comment.
Do not bubble raw dial/resolve errors back to the client.
This error is returned verbatim and then exposed by TestGerritConnection on Lines 286-287. Resolver and transport errors commonly include hostnames and internal resolver details, which turns the test endpoint into an information leak.
Proposed fix
resp, err := client.Do(req)
if err != nil {
- return false, fmt.Errorf("request failed: %w", networkError(err))
+ return false, fmt.Errorf("failed to contact Gerrit server")
}📝 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
| resp, err := client.Do(req) | |
| if err != nil { | |
| return false, fmt.Errorf("request failed: %w", networkError(err)) | |
| } | |
| resp, err := client.Do(req) | |
| if err != nil { | |
| return false, fmt.Errorf("failed to contact Gerrit server") | |
| } |
🤖 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 242 -
245, The client.Do(req) error is exposing raw dial/resolve details via
networkError(err) back to callers (used by TestGerritConnection); change the
behavior to avoid leaking resolver/transport info by returning a sanitized error
message (e.g., "request failed: connection error") to the client and log the
full original error internally instead. Update the code path around resp, err :=
client.Do(req) (and/or adjust networkError) so that transport/network errors
like *net.OpError/*net.DNSError are not returned verbatim but are replaced with
a generic error for the response while retaining the original error in internal
logs.
Comment on lines
+266
to
+285
| var req struct { | ||
| URL string `json:"url" binding:"required"` | ||
| AuthMethod string `json:"authMethod" binding:"required"` | ||
| Username string `json:"username"` | ||
| HTTPToken string `json:"httpToken"` | ||
| GitcookiesContent string `json:"gitcookiesContent"` | ||
| } | ||
|
|
||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| return | ||
| } | ||
|
|
||
| // Validate URL (SSRF protection) | ||
| if err := validateGerritURL(req.URL); err != nil { | ||
| c.JSON(http.StatusOK, gin.H{"valid": false, "error": fmt.Sprintf("Invalid URL: %s", err.Error())}) | ||
| return | ||
| } | ||
|
|
||
| valid, err := validateGerritTokenFn(c.Request.Context(), req.URL, req.AuthMethod, req.Username, req.HTTPToken, req.GitcookiesContent) |
Contributor
There was a problem hiding this comment.
Reject mixed auth fields in the test endpoint too.
Right now TestGerritConnection only branches on authMethod; it doesn't reject extra fields from the other auth mode. That lets a mixed payload test as valid here even though the feature contract requires mixed credentials to be rejected.
Proposed fix
if err := c.ShouldBindJSON(&req); err != nil {
c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()})
return
}
+
+ switch req.AuthMethod {
+ case gerritAuthHTTPBasic:
+ if req.GitcookiesContent != "" {
+ c.JSON(http.StatusOK, gin.H{"valid": false, "error": "Mixed credentials are not allowed"})
+ return
+ }
+ case gerritAuthGitCookies:
+ if req.Username != "" || req.HTTPToken != "" {
+ c.JSON(http.StatusOK, gin.H{"valid": false, "error": "Mixed credentials are not allowed"})
+ return
+ }
+ default:
+ c.JSON(http.StatusOK, gin.H{"valid": false, "error": "Unsupported auth method"})
+ return
+ }
// Validate URL (SSRF protection)
if err := validateGerritURL(req.URL); err != nil {📝 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
| var req struct { | |
| URL string `json:"url" binding:"required"` | |
| AuthMethod string `json:"authMethod" binding:"required"` | |
| Username string `json:"username"` | |
| HTTPToken string `json:"httpToken"` | |
| GitcookiesContent string `json:"gitcookiesContent"` | |
| } | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | |
| return | |
| } | |
| // Validate URL (SSRF protection) | |
| if err := validateGerritURL(req.URL); err != nil { | |
| c.JSON(http.StatusOK, gin.H{"valid": false, "error": fmt.Sprintf("Invalid URL: %s", err.Error())}) | |
| return | |
| } | |
| valid, err := validateGerritTokenFn(c.Request.Context(), req.URL, req.AuthMethod, req.Username, req.HTTPToken, req.GitcookiesContent) | |
| var req struct { | |
| URL string `json:"url" binding:"required"` | |
| AuthMethod string `json:"authMethod" binding:"required"` | |
| Username string `json:"username"` | |
| HTTPToken string `json:"httpToken"` | |
| GitcookiesContent string `json:"gitcookiesContent"` | |
| } | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | |
| return | |
| } | |
| switch req.AuthMethod { | |
| case gerritAuthHTTPBasic: | |
| if req.GitcookiesContent != "" { | |
| c.JSON(http.StatusOK, gin.H{"valid": false, "error": "Mixed credentials are not allowed"}) | |
| return | |
| } | |
| case gerritAuthGitCookies: | |
| if req.Username != "" || req.HTTPToken != "" { | |
| c.JSON(http.StatusOK, gin.H{"valid": false, "error": "Mixed credentials are not allowed"}) | |
| return | |
| } | |
| default: | |
| c.JSON(http.StatusOK, gin.H{"valid": false, "error": "Unsupported auth method"}) | |
| return | |
| } | |
| // Validate URL (SSRF protection) | |
| if err := validateGerritURL(req.URL); err != nil { | |
| c.JSON(http.StatusOK, gin.H{"valid": false, "error": fmt.Sprintf("Invalid URL: %s", err.Error())}) | |
| return | |
| } | |
| valid, err := validateGerritTokenFn(c.Request.Context(), req.URL, req.AuthMethod, req.Username, req.HTTPToken, req.GitcookiesContent) |
🤖 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 266 -
285, TestGerritConnection accepts mixed auth fields; before calling
validateGerritTokenFn you must explicitly reject payloads that include fields
from the non-selected auth mode: inspect req.AuthMethod and if it's the
HTTP/token mode ensure GitcookiesContent is empty, and if it's gitcookies mode
ensure Username and HTTPToken are empty (and respond with c.JSON(http.StatusOK,
gin.H{"valid": false, "error": "mixed auth fields not allowed"}) or similar).
Implement this check right after the validateGerritURL call and before invoking
validateGerritTokenFn; use the existing req struct field names (AuthMethod,
Username, HTTPToken, GitcookiesContent) to detect the invalid combinations.
- Remove { force: true } from trigger click; instead assert visibility
before clicking so Radix registers the open event correctly
- Increase menu-item timeout from 5s to 10s and add should('be.visible')
to wait for the dropdown animation to complete before clicking
Fixes intermittent "Expected to find [aria-label='Switch to light theme']"
failures in screenshots.cy.ts (introduced by PR #1322).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
The .should('be.visible').first() chain in the previous attempt caused
a detachment error on pages with live DOM updates (session-list polls
for sessions). Revert to the original chain structure but raise the
cy.get() timeout from 5000 to 10000 ms — slow CI runners need more
than 5 s for Radix to mount the DropdownMenuContent.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Contributor
Merge Queue Status
This pull request spent 10 seconds in the queue, including 1 second running CI. Required conditions to merge |
Contributor
|
| File | Component | Mode |
|---|---|---|
components/runners/ambient-runner/Dockerfile |
runner | warn |
No action required — these components are in warn mode. Consider using the component's agent workflow for future changes.
📖 Specs: Runner Spec · Runner Constitution
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
ssrfSafeTransport()with DNS rebinding TOCTOU fixgerrit.enabledGerritConnectionCardwith loading/error statesFixes applied during review
ListGerritInstancesresponse ("connected": true)TestGerritConnectionGerritConnectionCard; removed unusedstatuspropcli.coderabbit.ai/install.sh(not wrong npm pkg)coderabbit-review-gate.sh(HEAD~1unavailable)3/4→4/5, stale_GH_WRAPPER_PATHimportCloses #1349 (fork PR — reopened as same-repo PR so E2E tests can run)
Follow-up: #1350 adds Gerrit workflow skills.
Summary by CodeRabbit
New Features
Improvements
Tests