feat: add Gerrit integration connector for code review workflows#1349
Open
jeremyeder wants to merge 18 commits intoambient-code:mainfrom
Open
feat: add Gerrit integration connector for code review workflows#1349jeremyeder wants to merge 18 commits intoambient-code:mainfrom
jeremyeder wants to merge 18 commits intoambient-code:mainfrom
Conversation
Spec, plan, and tasks extracted from PR ambient-code#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>
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughAdds a Gerrit integration connector behind Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant Frontend
participant FrontendAPI
participant BackendAPI
participant Validator
participant K8sSecret
User->>Frontend: Enter instance name, URL, auth
Frontend->>FrontendAPI: POST /api/auth/gerrit/test
FrontendAPI->>BackendAPI: Forward /auth/gerrit/test
BackendAPI->>Validator: ValidateGerritToken (SSRF checks, /a/accounts/self)
Validator-->>BackendAPI: valid true/false
BackendAPI-->>FrontendAPI: { valid, message }
FrontendAPI-->>Frontend: Response
User->>Frontend: POST Save Instance
Frontend->>FrontendAPI: POST /api/auth/gerrit/connect
FrontendAPI->>BackendAPI: Forward /auth/gerrit/connect
BackendAPI->>Validator: ValidateGerritToken
BackendAPI->>K8sSecret: Store credentials (retry-on-conflict)
K8sSecret-->>BackendAPI: Stored
BackendAPI-->>FrontendAPI: 200
FrontendAPI-->>Frontend: Instance connected
sequenceDiagram
participant Session
participant BackendAPI
participant K8sSecret
participant Runner
participant MCPServer
Session->>BackendAPI: GET /agentic-sessions/:session/credentials/gerrit
BackendAPI->>K8sSecret: listGerritCredentials for user
K8sSecret-->>BackendAPI: instances[]
BackendAPI-->>Session: instances[]
Session->>Runner: start with instances[]
Runner->>Runner: generate_gerrit_config(instances)
alt any git_cookies
Runner->>Runner: write combined .gitcookies file
end
Runner->>Runner: set GERRIT_CONFIG_PATH and register MCP entry
Runner->>MCPServer: start with GERRIT_CONFIG_PATH
MCPServer-->>Runner: ready
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (3 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
components/frontend/src/components/gerrit-connection-card.tsx (2)
21-34: Unusedstatusprop.The
statusprop is defined inPropstype but not destructured or used in the component. Either remove it from the type or wire it up (e.g., for initial state beforeuseGerritInstancesloads).♻️ Remove unused prop
type Props = { - status?: { - connected: boolean - instances?: Array<{ - instanceName: string - url: string - authMethod: string - connected: boolean - }> - } onRefresh?: () => void } -export function GerritConnectionCard({ onRefresh }: Props) { +export function GerritConnectionCard({ onRefresh }: Props) {🤖 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 21 - 34, The Props type declares a status prop that GerritConnectionCard doesn't use; either remove status from Props or wire it into the component: update the Props type to drop status if not needed, or destructure status in GerritConnectionCard and use it to seed local state or render initial UI (e.g., before useGerritInstances loads) and replace any hardcoded/empty initial state with status.instances and status.connected; ensure references to status.instanceName/url/authMethod/connected align with existing useGerritInstances usage so behavior and types remain consistent.
95-113: Consider validating auth-specific fields before test.
handleTestonly checks!urlbut doesn't validate that required auth fields are filled. User could click "Test Connection" with empty username/token and get a less helpful error from the backend.♻️ Add auth field validation to handleTest
const handleTest = () => { if (!url) { toast.error('Please enter a Gerrit URL') return } + if (authMethod === 'http_basic' && (!username || !httpToken)) { + toast.error('Please enter username and HTTP token') + return + } + if (authMethod === 'git_cookies' && !gitcookiesContent) { + toast.error('Please enter gitcookies content') + return + } testMutation.mutate(buildTestPayload(), {🤖 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 95 - 113, handleTest currently only checks url and can call testMutation.mutate with missing auth fields, causing poor backend errors; before calling testMutation.mutate (in handleTest) validate the auth-specific inputs (e.g., username and token/password or selected authType) are present and non-empty, surface user-friendly toast.error messages for the missing fields, and only call buildTestPayload()/testMutation.mutate when all required auth fields are filled so the UI-side validation prevents unnecessary backend calls.
🤖 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 148-153: ConnectGerrit (and the other handlers
getGerritCredentials, listGerritCredentials, deleteGerritCredentials) obtain a
request-scoped Kubernetes client via GetK8sClientsForRequest(c) but then call
Secret helper functions (e.g., storeGerritCredentials and other secret helpers)
that use the global K8sClient; update each handler to pass the obtained reqK8s
into those helpers, and change the helper function signatures
(storeGerritCredentials and related secret helpers) to accept and use the
passed-in K8s client instead of the global K8sClient so all user-facing API ops
use the request-scoped client.
In `@components/backend/routes.go`:
- Around line 171-176: Register server-side enforcement for the gerrit feature
flag by gating all Gerrit endpoints (ConnectGerrit, TestGerritConnection,
ListGerritInstances, GetGerritStatus, DisconnectGerrit) behind the
`gerrit.enabled` check—either by wrapping the route registrations with a
middleware that reads the `gerrit.enabled` flag or by adding an early guard
inside each handler to return 404/403 when the flag is off; also ensure
TestGerritConnection requires user authentication the same way as the other
handlers (apply the same auth middleware used for
ConnectGerrit/ListGerritInstances/etc. or add an auth check at the top of
TestGerritConnection).
In `@components/frontend/src/app/api/auth/gerrit/instances/route.ts`:
- Around line 7-14: The current proxy call using fetch to
`${BACKEND_URL}/auth/gerrit/instances` (resp/resp.text()) lacks a timeout and
error handling; wrap the fetch in an AbortController with a short hard timeout,
await resp.ok then parse body, and surround the fetch/response logic with
try/catch to return appropriate non-500 responses on network failures (e.g.,
504/502 and a JSON error message) instead of letting the framework throw; update
the code paths that create Response (the return new Response(...) block) to use
the caught error/status and ensure 'Content-Type: application/json' is
preserved, and apply this same pattern to all new Gerrit proxy route handlers.
In `@components/frontend/src/services/api/gerrit-auth.ts`:
- Around line 28-38: The TypeScript type GerritInstanceStatus requires connected
but the backend ListGerritInstances (gerrit_auth.go) does not return it; update
the frontend type to make connected optional (connected?: boolean) in
GerritInstanceStatus or alternatively update the ListGerritInstances response to
include a connected boolean so it matches GetGerritStatus — change whichever
side you prefer (edit GerritInstanceStatus in
components/frontend/src/services/api/gerrit-auth.ts or add connected in the
ListGerritInstances response in gerrit_auth.go) so both shapes align.
---
Nitpick comments:
In `@components/frontend/src/components/gerrit-connection-card.tsx`:
- Around line 21-34: The Props type declares a status prop that
GerritConnectionCard doesn't use; either remove status from Props or wire it
into the component: update the Props type to drop status if not needed, or
destructure status in GerritConnectionCard and use it to seed local state or
render initial UI (e.g., before useGerritInstances loads) and replace any
hardcoded/empty initial state with status.instances and status.connected; ensure
references to status.instanceName/url/authMethod/connected align with existing
useGerritInstances usage so behavior and types remain consistent.
- Around line 95-113: handleTest currently only checks url and can call
testMutation.mutate with missing auth fields, causing poor backend errors;
before calling testMutation.mutate (in handleTest) validate the auth-specific
inputs (e.g., username and token/password or selected authType) are present and
non-empty, surface user-friendly toast.error messages for the missing fields,
and only call buildTestPayload()/testMutation.mutate when all required auth
fields are filled so the UI-side validation prevents unnecessary backend calls.
🪄 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: 68398905-5e79-4c72-ac5c-61475894b6df
📒 Files selected for processing (22)
.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/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/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/__init__.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.py
Comment on lines
+148
to
+153
| func ConnectGerrit(c *gin.Context) { | ||
| reqK8s, _ := GetK8sClientsForRequest(c) | ||
| if reqK8s == nil { | ||
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"}) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Critical: Secret operations use backend service account instead of user's K8s client.
ConnectGerrit correctly calls GetK8sClientsForRequest(c) but then storeGerritCredentials (and all other Secret helpers) use the global K8sClient. Per coding guidelines, user-facing API ops must use the request-scoped client.
Same issue in getGerritCredentials, listGerritCredentials, and deleteGerritCredentials.
🔧 Proposed fix: Pass K8s client to helpers
-func storeGerritCredentials(ctx context.Context, creds *GerritCredentials) error {
+func storeGerritCredentials(ctx context.Context, k8sClient kubernetes.Interface, creds *GerritCredentials) error {
// ... use k8sClient instead of K8sClient ...
-func getGerritCredentials(ctx context.Context, userID, instanceName string) (*GerritCredentials, error) {
+func getGerritCredentials(ctx context.Context, k8sClient kubernetes.Interface, userID, instanceName string) (*GerritCredentials, error) {
-func listGerritCredentials(ctx context.Context, userID string) ([]GerritCredentials, error) {
+func listGerritCredentials(ctx context.Context, k8sClient kubernetes.Interface, userID string) ([]GerritCredentials, error) {
-func deleteGerritCredentials(ctx context.Context, userID, instanceName string) error {
+func deleteGerritCredentials(ctx context.Context, k8sClient kubernetes.Interface, userID, instanceName string) error {Then in handlers:
func ConnectGerrit(c *gin.Context) {
reqK8s, _ := GetK8sClientsForRequest(c)
// ...
- if err := storeGerritCredentials(c.Request.Context(), creds); err != nil {
+ if err := storeGerritCredentials(c.Request.Context(), reqK8s, creds); err != nil {As per coding guidelines: "User-facing API ops MUST use GetK8sClientsForRequest(c), never the backend service account."
Also applies to: 372-429
🤖 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 148 - 153,
ConnectGerrit (and the other handlers getGerritCredentials,
listGerritCredentials, deleteGerritCredentials) obtain a request-scoped
Kubernetes client via GetK8sClientsForRequest(c) but then call Secret helper
functions (e.g., storeGerritCredentials and other secret helpers) that use the
global K8sClient; update each handler to pass the obtained reqK8s into those
helpers, and change the helper function signatures (storeGerritCredentials and
related secret helpers) to accept and use the passed-in K8s client instead of
the global K8sClient so all user-facing API ops use the request-scoped client.
Comment on lines
+171
to
+176
| // Cluster-level Gerrit (user-scoped, multi-instance) | ||
| api.POST("/auth/gerrit/connect", handlers.ConnectGerrit) | ||
| api.POST("/auth/gerrit/test", handlers.TestGerritConnection) | ||
| api.GET("/auth/gerrit/instances", handlers.ListGerritInstances) | ||
| api.GET("/auth/gerrit/:instanceName/status", handlers.GetGerritStatus) | ||
| api.DELETE("/auth/gerrit/:instanceName/disconnect", handlers.DisconnectGerrit) |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# 1) Find where gerrit.enabled is evaluated in backend code
rg -n --type=go -C3 'gerrit\.enabled|feature.?flag|EvaluateFeatureFlag|GetFeatureFlag' components/backend
# 2) Inspect Gerrit handler entry points for explicit flag checks
rg -n --type=go -C6 'func (ConnectGerrit|TestGerritConnection|ListGerritInstances|GetGerritStatus|DisconnectGerrit)\(' components/backend/handlersRepository: ambient-code/platform
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Find Gerrit-related handler files
find components/backend -name "*gerrit*" -type f
# Search for gerrit in handler files (broader search)
rg -l gerrit components/backend/handlers/ 2>/dev/null || echo "No gerrit files found in handlers"
# Look for any gerrit-related code in entire backend
rg -n "ConnectGerrit|TestGerritConnection|ListGerritInstances|GetGerritStatus|DisconnectGerrit" components/backend/ -t go --max-count=50Repository: ambient-code/platform
Length of output: 1379
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read gerrit_auth.go to inspect the handler implementations for flag checks
echo "=== gerrit_auth.go ==="
wc -l components/backend/handlers/gerrit_auth.go
# Read the first part and look for flag checks
head -200 components/backend/handlers/gerrit_auth.go
echo ""
echo "=== Searching for flag checks in gerrit_auth.go ==="
rg -n "gerrit\.enabled|FeatureEnabledForRequest|EvaluateFeatureFlag|flags\.|feature" components/backend/handlers/gerrit_auth.goRepository: ambient-code/platform
Length of output: 5889
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read more of gerrit_auth.go starting from ConnectGerrit function (line 148)
sed -n '148,300p' components/backend/handlers/gerrit_auth.go
echo ""
echo "=== TestGerritConnection from integration_validation.go ==="
# Find and read TestGerritConnection function
rg -n "func TestGerritConnection" components/backend/handlers/integration_validation.go -A 50Repository: ambient-code/platform
Length of output: 7036
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Read remaining Gerrit handlers
echo "=== ListGerritInstances and DisconnectGerrit ==="
sed -n '299,400p' components/backend/handlers/gerrit_auth.go
echo ""
echo "=== Check routes.go for middleware on Gerrit endpoints ==="
# Look at context around the Gerrit routes to see if middleware is applied
sed -n '160,185p' components/backend/routes.goRepository: ambient-code/platform
Length of output: 4871
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Check broader routes context to confirm no middleware at route group level
echo "=== Full route group context ==="
sed -n '100,190p' components/backend/routes.go | head -100
echo ""
echo "=== Check if there's any global middleware that checks gerrit.enabled ==="
rg -n "gerrit\.enabled" components/backend/ -t goRepository: ambient-code/platform
Length of output: 5446
Add server-side feature-flag enforcement for Gerrit endpoints.
Lines 171-176 register Gerrit routes without any gerrit.enabled gate. All five handlers (ConnectGerrit, TestGerritConnection, ListGerritInstances, GetGerritStatus, DisconnectGerrit) lack server-side flag checks, allowing direct API bypass of workspace-scoped rollout control. Additionally, TestGerritConnection is unauthenticated—it should require user authentication like the other handlers.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@components/backend/routes.go` around lines 171 - 176, Register server-side
enforcement for the gerrit feature flag by gating all Gerrit endpoints
(ConnectGerrit, TestGerritConnection, ListGerritInstances, GetGerritStatus,
DisconnectGerrit) behind the `gerrit.enabled` check—either by wrapping the route
registrations with a middleware that reads the `gerrit.enabled` flag or by
adding an early guard inside each handler to return 404/403 when the flag is
off; also ensure TestGerritConnection requires user authentication the same way
as the other handlers (apply the same auth middleware used for
ConnectGerrit/ListGerritInstances/etc. or add an auth check at the top of
TestGerritConnection).
Comment on lines
+7
to
+14
| const resp = await fetch(`${BACKEND_URL}/auth/gerrit/instances`, { | ||
| 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 + network-failure handling for backend proxy calls.
Line 7 currently has no timeout, and fetch failures are unhandled, so transient backend/network issues can produce hanging requests or framework-level 500s. Please apply the same hardening to all new Gerrit proxy routes.
Suggested fix
export async function GET(request: Request) {
const headers = await buildForwardHeadersAsync(request)
-
- const resp = await fetch(`${BACKEND_URL}/auth/gerrit/instances`, {
- 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/instances`, {
+ method: 'GET',
+ headers,
+ signal: AbortSignal.timeout(15000),
+ })
+
+ const data = await resp.text()
+ return new Response(data, {
+ status: resp.status,
+ headers: { 'Content-Type': 'application/json' },
+ })
+ } catch {
+ return Response.json({ error: 'Failed to reach backend service' }, { 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/instances`, { | |
| 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/instances`, { | |
| method: 'GET', | |
| headers, | |
| signal: AbortSignal.timeout(15000), | |
| }) | |
| const data = await resp.text() | |
| return new Response(data, { | |
| status: resp.status, | |
| headers: { 'Content-Type': 'application/json' }, | |
| }) | |
| } catch { | |
| return Response.json({ error: 'Failed to reach backend service' }, { 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/instances/route.ts` around lines
7 - 14, The current proxy call using fetch to
`${BACKEND_URL}/auth/gerrit/instances` (resp/resp.text()) lacks a timeout and
error handling; wrap the fetch in an AbortController with a short hard timeout,
await resp.ok then parse body, and surround the fetch/response logic with
try/catch to return appropriate non-500 responses on network failures (e.g.,
504/502 and a JSON error message) instead of letting the framework throw; update
the code paths that create Response (the return new Response(...) block) to use
the caught error/status and ensure 'Content-Type: application/json' is
preserved, and apply this same pattern to all new Gerrit proxy route handlers.
Comment on lines
+28
to
+38
| 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:
#!/bin/bash
# Verify backend list response structure
rg -n 'result = append.*gin.H' components/backend/handlers/gerrit_auth.go -A5 -B2Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Find the ListGerritInstances function definition
rg -n "func.*ListGerritInstances" components/backend/ -A 30Repository: ambient-code/platform
Length of output: 2494
🏁 Script executed:
# Search for where instances are appended in the backend
rg -n "instances.*append\|instances\s*=" components/backend/handlers/gerrit_auth.go -B 2 -A 3Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
# Check the frontend file to see how GerritInstanceStatus is used
cat -n components/frontend/src/services/api/gerrit-auth.ts | head -60Repository: ambient-code/platform
Length of output: 2097
🏁 Script executed:
# Get the complete ListGerritInstances function
sed -n '328,370p' components/backend/handlers/gerrit_auth.goRepository: ambient-code/platform
Length of output: 1369
🏁 Script executed:
# Also check if there's any connection status logic or connected field being set
rg -n "connected" components/backend/handlers/gerrit_auth.go -B 2 -A 2Repository: ambient-code/platform
Length of output: 984
Type mismatch: connected field missing from list response.
GerritInstanceStatus requires connected: boolean (line 29), but ListGerritInstances in gerrit_auth.go (lines 353-362) returns only instanceName, url, authMethod, updatedAt — no connected field.
The GetGerritStatus endpoint includes connected, but the list endpoint doesn't. Either add connected to the list response or make it optional in the type.
🤖 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 28 - 38,
The TypeScript type GerritInstanceStatus requires connected but the backend
ListGerritInstances (gerrit_auth.go) does not return it; update the frontend
type to make connected optional (connected?: boolean) in GerritInstanceStatus or
alternatively update the ListGerritInstances response to include a connected
boolean so it matches GetGerritStatus — change whichever side you prefer (edit
GerritInstanceStatus in components/frontend/src/services/api/gerrit-auth.ts or
add connected in the ListGerritInstances response in gerrit_auth.go) so both
shapes align.
3 tasks
Contributor
Author
|
Follow-up: #1350 adds Gerrit workflow skills (review, submit, create) that teach agents how to compose the MCP server tools this PR enables. |
Contributor
Author
jeremyeder
added a commit
that referenced
this pull request
Apr 20, 2026
## Summary - Three Claude Code plugin skills teaching agents Gerrit-specific workflows - **gerrit-review**: fetch changes, read diffs, post inline comments, vote with labels - **gerrit-submit**: check submit requirements, rebase, submit changes - **gerrit-create**: push code for review, update patch sets, manage metadata - References the Gerrit MCP server tools (`fetch_gerrit_change`, `fetch_patchset_diff`, `submit_gerrit_review`) - Complements #1349 (Gerrit integration connector) — skills provide workflow judgment, connector provides plumbing Depends on #1349 ## Test plan - [ ] Skills discovered by Claude Code plugin system - [ ] Skill descriptions trigger on relevant user prompts - [ ] Skill content references correct MCP tool names 🤖 Generated with [Claude Code](https://claude.com/claude-code) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added Gerrit skills plugin providing comprehensive guides for creating Gerrit changes, reviewing code with voting and inline comments, and submitting changes for merge, including troubleshooting for common workflow issues. <!-- end of auto-generated comment: release notes by coderabbit.ai --> Co-authored-by: Ambient Code Bot <bot@ambient-code.local> Co-authored-by: Claude Opus 4.6 (1M context) <noreply@anthropic.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>
7 tasks
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
components/backend/routes.go (1)
102-102:⚠️ Potential issue | 🟠 MajorEnforce
gerrit.enabledserver-side for Gerrit endpoints (including runtime credential fetch).Line 102 and Lines 177-183 expose Gerrit API surfaces without any visible route-level feature-flag guard. If handlers also lack a guard, this allows direct API access even when workspace rollout is disabled in UI. Add a server-side
gerrit.enabledcheck (middleware or early handler return with 403/404) for these endpoints.As per coding guidelines, "Prioritize Critical and Major severity issues. Minimize Minor and Trivial findings." and "Flag bugs, security vulnerabilities, logic errors, data loss risks, and meaningful refactoring opportunities."
Also applies to: 177-183
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/routes.go` at line 102, The Gerrit routes (e.g., projectGroup.GET("/agentic-sessions/:sessionName/credentials/gerrit", handlers.GetGerritCredentialsForSession)) are exposed without a server-side feature flag check; add a guard so requests are blocked when gerrit.enabled is false by either creating a small middleware that reads the server/workspace config and returns 403 (or 404) when disabled and applying it to all Gerrit-related routes, or update each handler (e.g., handlers.GetGerritCredentialsForSession and the other Gerrit handlers referenced in the diff) to perform an early check of gerrit.enabled and return the appropriate HTTP error before doing any work.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@components/backend/routes.go`:
- Line 102: The Gerrit routes (e.g.,
projectGroup.GET("/agentic-sessions/:sessionName/credentials/gerrit",
handlers.GetGerritCredentialsForSession)) are exposed without a server-side
feature flag check; add a guard so requests are blocked when gerrit.enabled is
false by either creating a small middleware that reads the server/workspace
config and returns 403 (or 404) when disabled and applying it to all
Gerrit-related routes, or update each handler (e.g.,
handlers.GetGerritCredentialsForSession and the other Gerrit handlers referenced
in the diff) to perform an early check of gerrit.enabled and return the
appropriate HTTP error before doing any work.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: b633fbff-32b2-47dd-87df-6732149a0dab
📒 Files selected for processing (2)
components/backend/routes.gocomponents/runners/ambient-runner/ambient_runner/platform/auth.py
🚧 Files skipped from review as they are similar to previous changes (1)
- components/runners/ambient-runner/ambient_runner/platform/auth.py
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
.mcp.json)gerrit.enabledwithscope:workspacetagSupersedes #1078
Test plan
cd components/backend && make test— backend compiles and vets cleancd components/frontend && npm run build— zero errors, zero warningscd components/runners/ambient-runner && python -m pytest tests/— runner tests passmake lint— all pre-commit hooks pass🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes / Security
Chores