feat: add CodeRabbit integration for AI-powered code review#1315
Open
jeremyeder wants to merge 5 commits intoambient-code:mainfrom
Open
feat: add CodeRabbit integration for AI-powered code review#1315jeremyeder wants to merge 5 commits intoambient-code:mainfrom
jeremyeder wants to merge 5 commits intoambient-code:mainfrom
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a CodeRabbit integration: backend auth/status/test handlers with K8s Secret-backed per-user storage, session credential delivery, frontend proxy routes/UI/hooks, runner credential injection, CLI/hooks/scripts, docs/specs/ADR, and CI/test workflows. Changes
Sequence DiagramssequenceDiagram
participant User as User/<br/>Frontend
participant FrontendAPI as Frontend<br/>API Routes
participant Backend as Backend<br/>Handlers
participant K8s as Kubernetes<br/>Secret Store
participant CodeRabbit as CodeRabbit<br/>API
User->>FrontendAPI: POST /api/auth/coderabbit/connect<br/>{apiKey}
FrontendAPI->>Backend: POST /auth/coderabbit/connect<br/>(forwarded headers + body)
Backend->>CodeRabbit: GET /api/v1/health<br/>(Authorization: Bearer <apiKey>)
CodeRabbit-->>Backend: 200 OK / 401/403 / other
alt Key Valid (200)
Backend->>K8s: Create/Update Secret<br/>(coderabbit-credentials per-user entry)
K8s-->>Backend: Secret stored
Backend-->>FrontendAPI: 200 OK {message}
FrontendAPI-->>User: Success
else Key Invalid (401/403)
Backend-->>FrontendAPI: 400 Bad Request {error}
FrontendAPI-->>User: Error
else Upstream Error
Backend-->>FrontendAPI: 502 Bad Gateway {error}
FrontendAPI-->>User: Error
end
sequenceDiagram
participant SessionRunner as Session Runner<br/>(ambient)
participant Backend as Backend<br/>Handlers
participant K8s as Kubernetes<br/>Secret Store
SessionRunner->>Backend: GET /api/projects/:project/.../credentials/coderabbit<br/>(with RBAC)
Backend->>K8s: Read Secret coderabbit-credentials
K8s-->>Backend: Creds for user or not found / error
alt Credentials Found
Backend-->>SessionRunner: 200 OK {apiKey}
SessionRunner->>SessionRunner: export CODERABBIT_API_KEY=<apiKey>
SessionRunner->>SessionRunner: run agentic turn (uses CLI)
SessionRunner->>SessionRunner: unset CODERABBIT_API_KEY (on completion)
else Not Found
Backend-->>SessionRunner: 404 Not Found {error}
SessionRunner->>SessionRunner: skip CodeRabbit usage
else Read Error
Backend-->>SessionRunner: 500 Internal Server Error {error}
SessionRunner->>SessionRunner: log and skip/fail per runner policy
end
sequenceDiagram
participant Dev as Developer
participant Hook as PreToolUse Hook<br/>(coderabbit-review-gate.sh)
participant CLI as CodeRabbit CLI
participant CodeRabbit as CodeRabbit API
Dev->>Hook: gh pr create (hook intercepts)
Hook->>CLI: coderabbit review --agent --base main
alt CLI missing
Hook-->>Dev: prints install instructions, exit 2 (block)
else CLI present
CLI->>CodeRabbit: POST /review (staged/branch)
CodeRabbit-->>CLI: JSON lines (findings or errors)
CLI-->>Hook: output
alt rate_limit/auth error types present
Hook-->>Dev: warn and allow PR (exit 0)
else findings with severity=error
Hook-->>Dev: BLOCKED message and exit 2
else no blocking findings
Hook-->>Dev: Passed and exit 0
end
end
🚥 Pre-merge checks | ✅ 4 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ 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: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
components/frontend/src/app/integrations/IntegrationsClient.tsx (1)
15-16:⚠️ Potential issue | 🟠 MajorHandle query errors explicitly before rendering integration cards.
If
useIntegrationsStatus()fails, this component drops into the non-loading branch and renders cards with undefined statuses, masking transport/auth failures as "disconnected." The hook returnserrorandisErrorfields (via React Query'suseQuery), but you're not destructuring them. Add anisErrorbranch with a clear failure UI before the grid render (lines 32–37).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/integrations/IntegrationsClient.tsx` around lines 15 - 16, The component currently only destructures { data: integrations, isLoading, refetch } from useIntegrationsStatus() and will render integration cards with undefined statuses when the query errors; update the destructuring to also grab isError and error from useIntegrationsStatus(), add an early-return branch that checks isError and renders a clear failure UI (e.g., a message/button to retry using refetch) before the grid of cards is rendered, and ensure any places referencing integrations handle the absence of data safely; look for useIntegrationsStatus, integrations, isLoading, isError, error, refetch and the grid render to implement this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/coderabbit-smoke-test.yml:
- Around line 86-90: The workflow currently passes the secret via the --api-key
flag in the coderabbit review invocation (the command that builds OUTPUT and
EXIT_CODE), which exposes it in process args; remove the --api-key
"$CODERABBIT_API_KEY" argument and rely on the CODERABBIT_API_KEY environment
variable being available to the CLI instead (ensure the step exports or sets
CODERABBIT_API_KEY in the environment or uses GitHub Actions secrets mapping so
coderabbit reads it from process.env rather than from a command-line flag).
In `@components/backend/handlers/integration_validation.go`:
- Around line 232-241: The TestCodeRabbitConnection handler currently begins
validation without establishing request-scoped auth; call
GetK8sClientsForRequest(c) at the start of TestCodeRabbitConnection, check its
returned error (and any nil client), and if it fails return the appropriate HTTP
error JSON and stop processing before binding/validating the API key—this
ensures the endpoint uses the request-scoped k8s client instead of the backend
service account and follows the same auth pattern as other user-facing
endpoints.
In `@components/frontend/src/app/api/auth/coderabbit/status/route.ts`:
- Around line 4-14: The GET handler currently throws on upstream fetch failures
and forces 'application/json'; wrap the fetch in try/catch, and when the fetch
succeeds read the upstream body (e.g., resp.text()) and return new
Response(body, { status: resp.status, headers: { 'Content-Type':
resp.headers.get('content-type') || 'text/plain' } }); if resp.ok is false still
forward the upstream status and content-type (fall back to 'text/plain' if
missing); in the catch block return a 502 Response with the error.message and
'text/plain' content-type. Apply the same pattern to the sibling handlers
(disconnect, test, connect and other integration routes) so they preserve
upstream content-type and handle network/fetch errors gracefully.
In `@scripts/test-coderabbit-integration.sh`:
- Around line 180-187: The health check is using a hardcoded port 9323 instead
of the discovered FRONTEND_PORT; update the curl invocation that sets
FRONTEND_STATUS to use the FRONTEND_PORT variable (e.g.
"http://localhost:${FRONTEND_PORT}/integrations") and keep the same error
fallback (|| echo "000"), preserving the surrounding conditional that checks
FRONTEND_PORT != "NONE" and the pass/skip calls (FRONTEND_STATUS, pass, skip).
---
Outside diff comments:
In `@components/frontend/src/app/integrations/IntegrationsClient.tsx`:
- Around line 15-16: The component currently only destructures { data:
integrations, isLoading, refetch } from useIntegrationsStatus() and will render
integration cards with undefined statuses when the query errors; update the
destructuring to also grab isError and error from useIntegrationsStatus(), add
an early-return branch that checks isError and renders a clear failure UI (e.g.,
a message/button to retry using refetch) before the grid of cards is rendered,
and ensure any places referencing integrations handle the absence of data
safely; look for useIntegrationsStatus, integrations, isLoading, isError, error,
refetch and the grid render to implement this change.
🪄 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: 72b2e4da-a160-49a3-a5f6-cd8a48769aa9
📒 Files selected for processing (27)
.github/workflows/coderabbit-smoke-test.yml.pre-commit-config.yamlcomponents/backend/handlers/coderabbit_auth.gocomponents/backend/handlers/coderabbit_auth_test.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/integrations_status.gocomponents/backend/handlers/runtime_credentials.gocomponents/backend/routes.gocomponents/backend/tests/constants/labels.gocomponents/frontend/src/app/api/auth/coderabbit/connect/route.tscomponents/frontend/src/app/api/auth/coderabbit/disconnect/route.tscomponents/frontend/src/app/api/auth/coderabbit/status/route.tscomponents/frontend/src/app/api/auth/coderabbit/test/route.tscomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsxcomponents/frontend/src/components/coderabbit-connection-card.tsxcomponents/frontend/src/services/api/coderabbit-auth.tscomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/queries/use-coderabbit.tscomponents/runners/ambient-runner/ambient_runner/platform/auth.pydocs/internal/adr/0008-automate-code-reviews.mddocs/pr-1307-impact-analysis.mddocs/src/content/docs/features/coderabbit.mdscripts/pre-commit/coderabbit-review.shscripts/test-coderabbit-integration.shspecs/001-coderabbit-integration/checklists/requirements.mdspecs/001-coderabbit-integration/spec.md
Comment on lines
+232
to
+241
| // TestCodeRabbitConnection handles POST /api/auth/coderabbit/test | ||
| func TestCodeRabbitConnection(c *gin.Context) { | ||
| var req struct { | ||
| APIKey string `json:"apiKey" binding:"required"` | ||
| } | ||
|
|
||
| if err := c.ShouldBindJSON(&req); err != nil { | ||
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | ||
| return | ||
| } |
Contributor
There was a problem hiding this comment.
Add the request-scoped auth gate before validating CodeRabbit keys.
Line 233 begins request processing without the GetK8sClientsForRequest(c) check used by other user-facing auth endpoints. This leaves the test endpoint outside the required request-auth pattern.
🔧 Proposed fix
func TestCodeRabbitConnection(c *gin.Context) {
+ reqK8s, _ := GetK8sClientsForRequest(c)
+ if reqK8s == nil {
+ c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"})
+ return
+ }
+
+ userID := c.GetString("userID")
+ if userID == "" {
+ c.JSON(http.StatusUnauthorized, gin.H{"error": "User authentication required"})
+ return
+ }
+
var req struct {
APIKey string `json:"apiKey" binding:"required"`
}As per coding guidelines, User-facing API ops MUST use GetK8sClientsForRequest(c), never the backend service account.
📝 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
| // TestCodeRabbitConnection handles POST /api/auth/coderabbit/test | |
| func TestCodeRabbitConnection(c *gin.Context) { | |
| var req struct { | |
| APIKey string `json:"apiKey" binding:"required"` | |
| } | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | |
| return | |
| } | |
| func TestCodeRabbitConnection(c *gin.Context) { | |
| reqK8s, _ := GetK8sClientsForRequest(c) | |
| if reqK8s == nil { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "Invalid or missing token"}) | |
| return | |
| } | |
| userID := c.GetString("userID") | |
| if userID == "" { | |
| c.JSON(http.StatusUnauthorized, gin.H{"error": "User authentication required"}) | |
| return | |
| } | |
| var req struct { | |
| APIKey string `json:"apiKey" binding:"required"` | |
| } | |
| if err := c.ShouldBindJSON(&req); err != nil { | |
| c.JSON(http.StatusBadRequest, gin.H{"error": err.Error()}) | |
| return | |
| } |
🤖 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 232 -
241, The TestCodeRabbitConnection handler currently begins validation without
establishing request-scoped auth; call GetK8sClientsForRequest(c) at the start
of TestCodeRabbitConnection, check its returned error (and any nil client), and
if it fails return the appropriate HTTP error JSON and stop processing before
binding/validating the API key—this ensures the endpoint uses the request-scoped
k8s client instead of the backend service account and follows the same auth
pattern as other user-facing endpoints.
Comment on lines
+4
to
+14
| export async function GET(request: Request) { | ||
| const headers = await buildForwardHeadersAsync(request) | ||
|
|
||
| const resp = await fetch(`${BACKEND_URL}/auth/coderabbit/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.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Verify the same proxy pattern (uncaught fetch + forced JSON content-type) across CodeRabbit routes.
rg -n --type=ts -C2 'export async function (GET|POST|DELETE)\(request: Request\)|fetch\(|new Response\(data, \{ status: resp.status, headers: \{ '\''Content-Type'\'': '\''application/json'\'' \} \}\)' components/frontend/src/app/api/auth/coderabbit
rg -n --type=ts -C1 'try\s*\{' components/frontend/src/app/api/auth/coderabbitRepository: ambient-code/platform
Length of output: 5123
🏁 Script executed:
#!/bin/bash
# Check buildForwardHeadersAsync implementation
rg -n "buildForwardHeadersAsync" components/frontend/src/lib/auth --type=ts -A 10Repository: ambient-code/platform
Length of output: 137
🏁 Script executed:
#!/bin/bash
# Check if there are other proxy routes that handle errors correctly
find components/frontend/src/app/api -name "route.ts" -type f | head -20 | xargs grep -l "try\s*{" 2>/dev/null || echo "No routes with try/catch found"Repository: ambient-code/platform
Length of output: 249
🏁 Script executed:
#!/bin/bash
# Check the structure of a few other API routes to see error handling patterns
fd "route.ts" components/frontend/src/app/api --max-results=5 | xargs head -20Repository: ambient-code/platform
Length of output: 2796
🏁 Script executed:
#!/bin/bash
# Find buildForwardHeadersAsync implementation
rg -n "buildForwardHeadersAsync" components/frontend/src --type=ts -B2 -A8 | head -50Repository: ambient-code/platform
Length of output: 4485
🏁 Script executed:
#!/bin/bash
# Get full Google status route to see correct error handling pattern
cat -n components/frontend/src/app/api/auth/google/status/route.tsRepository: ambient-code/platform
Length of output: 1697
🏁 Script executed:
#!/bin/bash
# List all routes with the broken pattern (no try/catch)
rg -l "new Response\(data, \{ status: resp.status, headers: \{ 'Content-Type': 'application/json'" components/frontend/src/app/api --type=tsRepository: ambient-code/platform
Length of output: 1894
Guard upstream fetch failures and preserve upstream content-type.
This proxy throws on upstream failure (no catch) and forces JSON content-type even when upstream returns non-JSON error bodies, breaking the API contract under outage conditions. Same pattern affects the sibling disconnect, test, and connect routes, plus 20+ other routes across Jira, GitLab, GitHub, and other integrations. The Google OAuth status route shows the correct pattern with try/catch and proper error handling.
Proposed fix (apply same pattern to disconnect/test/connect routes)
export async function GET(request: Request) {
- const headers = await buildForwardHeadersAsync(request)
-
- const resp = await fetch(`${BACKEND_URL}/auth/coderabbit/status`, {
- method: 'GET',
- headers,
- })
-
- const data = await resp.text()
- return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+ try {
+ const headers = await buildForwardHeadersAsync(request)
+ const resp = await fetch(`${BACKEND_URL}/auth/coderabbit/status`, {
+ method: 'GET',
+ headers,
+ })
+
+ 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: 'CodeRabbit 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/coderabbit/status/route.ts` around lines
4 - 14, The GET handler currently throws on upstream fetch failures and forces
'application/json'; wrap the fetch in try/catch, and when the fetch succeeds
read the upstream body (e.g., resp.text()) and return new Response(body, {
status: resp.status, headers: { 'Content-Type': resp.headers.get('content-type')
|| 'text/plain' } }); if resp.ok is false still forward the upstream status and
content-type (fall back to 'text/plain' if missing); in the catch block return a
502 Response with the error.message and 'text/plain' content-type. Apply the
same pattern to the sibling handlers (disconnect, test, connect and other
integration routes) so they preserve upstream content-type and handle
network/fetch errors gracefully.
Full-stack integration following the Jira pattern and PR ambient-code#1307 conventions. Implements infrastructure for ADR-0008 (automated inner-loop review). Backend (Go): - Auth handlers: connect, status, disconnect, test (K8s Secret storage) - API key validation against CodeRabbit health API with error differentiation (401/403 = invalid key, 5xx = upstream error) - Runtime credential endpoint with RBAC for session pods - Unified integrations status includes CodeRabbit - 16 Ginkgo tests Frontend (Next.js + React): - Informational-first connection card: public repos free via GitHub App, API key collapsed under "Private repository access" with billing warning - API client layer + React Query hooks with dual cache invalidation - 4 Next.js proxy routes - Wired into IntegrationsClient grid and session integrations panel Runner (Python): - fetch_coderabbit_credentials via shared _fetch_credential helper - CODERABBIT_API_KEY injected into session env via asyncio.gather - Cleared on turn completion Pre-commit hook: - Runs coderabbit review --agent on staged changes - Supports both CODERABBIT_API_KEY env and cr auth login session - CLI reads env var directly (no --api-key in process listing) - Skips gracefully when CLI/auth/changes unavailable CI + Testing: - GHA smoke test: validates config, runs live review, tests hook behavior (actions pinned to SHAs, permissions scoped) - Integration test script: 9/9 passing against dev cluster Docs: - Starlight guide: public vs private repos, local dev, session flow - ADR-0008: automated code reviews via inner-loop + Mergify - PR ambient-code#1307 impact analysis Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
The pre-commit CodeRabbit hook ran on every commit, reviewed partial work, and never blocked anything. Replace it with a PreToolUse command hook that intercepts `gh pr create` and blocks until CodeRabbit passes. This enforces the inner-loop review from ADR-0008: the agent fixes findings locally before the PR exists. Works in both Claude Code CLI and ACP sessions (via the bundled CLI subprocess). The script also runs standalone for CI use. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
jeremyeder
force-pushed
the
coderabbit-integration-clean
branch
from
c0b7f62 to
c2f0560
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
components/frontend/src/app/api/auth/coderabbit/status/route.ts (1)
4-14:⚠️ Potential issue | 🟠 MajorMissing try/catch for upstream fetch failure.
The route throws on network errors (no catch). This was flagged in a previous review. The Google OAuth status route shows the correct pattern with try/catch.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/frontend/src/app/api/auth/coderabbit/status/route.ts` around lines 4 - 14, The GET handler lacks a try/catch around the upstream fetch which lets network errors bubble up; wrap the buildForwardHeadersAsync + fetch call inside a try block and catch errors to return a controlled Response (e.g., 502 Bad Gateway) with a JSON error body and 'Content-Type': 'application/json'. Use the same error-handling pattern as the Google OAuth status route: catch any thrown error, log or include a concise message, and return a Response with appropriate status and JSON payload; update function GET and keep buildForwardHeadersAsync and BACKEND_URL usage unchanged.scripts/test-coderabbit-integration.sh (1)
180-187:⚠️ Potential issue | 🟡 MinorHardcoded port ignores discovered
FRONTEND_PORT.Line 182 uses
localhost:9323instead of theFRONTEND_PORTvariable discovered on lines 170-178.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/test-coderabbit-integration.sh` around lines 180 - 187, The check for the frontend uses a hardcoded port (localhost:9323) instead of the discovered FRONTEND_PORT; update the curl call that sets FRONTEND_STATUS in the block that runs when [ "$FRONTEND_PORT" != "NONE" ] to use the FRONTEND_PORT variable (e.g., "http://localhost:${FRONTEND_PORT}/integrations") so the script actually probes the discovered port, and keep the existing behavior of setting FRONTEND_STATUS and the pass/skip messages unchanged.
🧹 Nitpick comments (1)
specs/001-coderabbit-integration/spec.md (1)
121-122: Spec assumption may be stale.Line 121 references
review --type uncommitted --prompt-onlybut the implemented gate script usesreview --agent --base main. Consider updating the spec to match the implementation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@specs/001-coderabbit-integration/spec.md` around lines 121 - 122, The spec text mentions the CLI usage "review --type uncommitted --prompt-only" which is inconsistent with the implemented gate script that invokes "review --agent --base main"; update the spec entry to match the implementation by replacing or adding the actual command signature used by the gate script (e.g., "review --agent --base main" and mention that "coderabbit" (with "cr" fallback) is the binary), or alternatively change the gate script to call the spec'd flags—locate references to the CLI invocation in the spec (the line mentioning review flags) and update it to the canonical, implemented command form ("review --agent --base main") so documentation and implementation are aligned.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@scripts/test-coderabbit-integration.sh`:
- Around line 130-132: The test in scripts/test-coderabbit-integration.sh
expects "Session not found" but the backend runtime credentials handler
(runtime_credentials.go handler that returns "CodeRabbit credentials not
configured") returns a different message; update the assert_json call that
checks BODY to expect "CodeRabbit credentials not configured" (or broaden the
assertion to accept either message) so the test matches the backend's
runtime_credentials handler behavior — modify the assert_json invocation that
currently asserts "Session not found" to assert the backend's actual message.
---
Duplicate comments:
In `@components/frontend/src/app/api/auth/coderabbit/status/route.ts`:
- Around line 4-14: The GET handler lacks a try/catch around the upstream fetch
which lets network errors bubble up; wrap the buildForwardHeadersAsync + fetch
call inside a try block and catch errors to return a controlled Response (e.g.,
502 Bad Gateway) with a JSON error body and 'Content-Type': 'application/json'.
Use the same error-handling pattern as the Google OAuth status route: catch any
thrown error, log or include a concise message, and return a Response with
appropriate status and JSON payload; update function GET and keep
buildForwardHeadersAsync and BACKEND_URL usage unchanged.
In `@scripts/test-coderabbit-integration.sh`:
- Around line 180-187: The check for the frontend uses a hardcoded port
(localhost:9323) instead of the discovered FRONTEND_PORT; update the curl call
that sets FRONTEND_STATUS in the block that runs when [ "$FRONTEND_PORT" !=
"NONE" ] to use the FRONTEND_PORT variable (e.g.,
"http://localhost:${FRONTEND_PORT}/integrations") so the script actually probes
the discovered port, and keep the existing behavior of setting FRONTEND_STATUS
and the pass/skip messages unchanged.
---
Nitpick comments:
In `@specs/001-coderabbit-integration/spec.md`:
- Around line 121-122: The spec text mentions the CLI usage "review --type
uncommitted --prompt-only" which is inconsistent with the implemented gate
script that invokes "review --agent --base main"; update the spec entry to match
the implementation by replacing or adding the actual command signature used by
the gate script (e.g., "review --agent --base main" and mention that
"coderabbit" (with "cr" fallback) is the binary), or alternatively change the
gate script to call the spec'd flags—locate references to the CLI invocation in
the spec (the line mentioning review flags) and update it to the canonical,
implemented command form ("review --agent --base main") so documentation and
implementation are aligned.
🪄 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: fe8365da-30fe-4256-87a9-d13485f2520e
📒 Files selected for processing (27)
.claude/settings.json.github/workflows/coderabbit-smoke-test.ymlcomponents/backend/handlers/coderabbit_auth.gocomponents/backend/handlers/coderabbit_auth_test.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/integrations_status.gocomponents/backend/handlers/runtime_credentials.gocomponents/backend/routes.gocomponents/backend/tests/constants/labels.gocomponents/frontend/src/app/api/auth/coderabbit/connect/route.tscomponents/frontend/src/app/api/auth/coderabbit/disconnect/route.tscomponents/frontend/src/app/api/auth/coderabbit/status/route.tscomponents/frontend/src/app/api/auth/coderabbit/test/route.tscomponents/frontend/src/app/integrations/IntegrationsClient.tsxcomponents/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/integrations-panel.tsxcomponents/frontend/src/components/coderabbit-connection-card.tsxcomponents/frontend/src/services/api/coderabbit-auth.tscomponents/frontend/src/services/api/integrations.tscomponents/frontend/src/services/queries/use-coderabbit.tscomponents/runners/ambient-runner/ambient_runner/platform/auth.pydocs/internal/adr/0008-automate-code-reviews.mddocs/pr-1307-impact-analysis.mddocs/src/content/docs/features/coderabbit.mdscripts/hooks/coderabbit-review-gate.shscripts/test-coderabbit-integration.shspecs/001-coderabbit-integration/checklists/requirements.mdspecs/001-coderabbit-integration/spec.md
✅ Files skipped from review due to trivial changes (9)
- specs/001-coderabbit-integration/checklists/requirements.md
- components/frontend/src/services/api/integrations.ts
- components/frontend/src/app/api/auth/coderabbit/disconnect/route.ts
- components/frontend/src/app/api/auth/coderabbit/connect/route.ts
- components/frontend/src/app/api/auth/coderabbit/test/route.ts
- docs/internal/adr/0008-automate-code-reviews.md
- docs/pr-1307-impact-analysis.md
- components/frontend/src/services/api/coderabbit-auth.ts
- components/backend/handlers/coderabbit_auth_test.go
🚧 Files skipped from review as they are similar to previous changes (11)
- components/backend/tests/constants/labels.go
- components/frontend/src/app/integrations/IntegrationsClient.tsx
- components/runners/ambient-runner/ambient_runner/platform/auth.py
- components/backend/routes.go
- components/backend/handlers/integrations_status.go
- components/frontend/src/services/queries/use-coderabbit.ts
- components/backend/handlers/runtime_credentials.go
- components/backend/handlers/integration_validation.go
- .github/workflows/coderabbit-smoke-test.yml
- docs/src/content/docs/features/coderabbit.md
- components/backend/handlers/coderabbit_auth.go
8 tasks
- Remove --api-key CLI arg from smoke test (secret leak via process args) - Add try/catch to all 4 proxy routes (502 on upstream failure) - Use tempfile.mkdtemp for gh wrapper (no predictable /tmp path) - Fix test assertion to match backend error message - Use discovered FRONTEND_PORT instead of hardcoded 9323 - Update spec to match implemented CLI flags - Remove dead exports (testCodeRabbitConnection, useCodeRabbitStatus) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add ADR-0008 to architecture decisions in BOOKMARKS.md - Add CodeRabbit integration guide to component guides in BOOKMARKS.md - Update docs to replace pre-commit hook with review gate section Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Replace references to deleted pre-commit hook with the review gate script. Both now exercise the same coderabbit review --agent --base main path used in the inner-loop hook. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 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 `@scripts/test-coderabbit-integration.sh`:
- Around line 130-133: The test currently only inspects BODY for the error
message but doesn't verify the HTTP status; update the
runtime-creds-for-nonexistent-session block to capture the HTTP status from curl
(alongside BODY) and assert that the status equals 404; reuse the existing BODY
variable and the test's assertion helper (or add an assert for STATUS) so the
block that queries
"/api/projects/default/agentic-sessions/nonexistent/credentials/coderabbit"
verifies both the response body contains "CodeRabbit credentials not configured"
and the HTTP status is 404.
- Around line 26-30: The script reads CONTEXT from "$2" in the case for
--context without validating that a value exists, which under set -u can cause
an unbound-variable exit; update the --context branch in the while/case block to
check the remaining argument count before accessing $2 (e.g., if [ "$#" -lt 2 ]
then print a clear "missing value for --context" error and exit 1), otherwise
assign CONTEXT="$2" and shift 2; reference the existing CONTEXT variable and the
case branch handling --context when making this change.
🪄 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: 51595450-fac3-4a03-8fcc-4878144039e4
📒 Files selected for processing (2)
.github/workflows/coderabbit-smoke-test.ymlscripts/test-coderabbit-integration.sh
✅ Files skipped from review due to trivial changes (1)
- .github/workflows/coderabbit-smoke-test.yml
Comment on lines
+26
to
+30
| while [[ $# -gt 0 ]]; do | ||
| case $1 in | ||
| --context) CONTEXT="$2"; shift 2 ;; | ||
| *) echo "Unknown arg: $1"; exit 1 ;; | ||
| esac |
Contributor
There was a problem hiding this comment.
--context parsing can crash on missing value under set -u.
At Line 28, CONTEXT="$2" is accessed without checking argument count. Running with --context and no value causes an unbound-variable exit instead of a clean error message.
Proposed fix
while [[ $# -gt 0 ]]; do
case $1 in
- --context) CONTEXT="$2"; shift 2 ;;
+ --context)
+ if [[ $# -lt 2 || -z "${2:-}" ]]; then
+ echo "ERROR: --context requires a value"
+ exit 1
+ fi
+ CONTEXT="$2"; shift 2 ;;
*) echo "Unknown arg: $1"; exit 1 ;;
esac
done📝 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
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| --context) CONTEXT="$2"; shift 2 ;; | |
| *) echo "Unknown arg: $1"; exit 1 ;; | |
| esac | |
| while [[ $# -gt 0 ]]; do | |
| case $1 in | |
| --context) | |
| if [[ $# -lt 2 || -z "${2:-}" ]]; then | |
| echo "ERROR: --context requires a value" | |
| exit 1 | |
| fi | |
| CONTEXT="$2"; shift 2 ;; | |
| *) echo "Unknown arg: $1"; exit 1 ;; | |
| esac | |
| done |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-coderabbit-integration.sh` around lines 26 - 30, The script
reads CONTEXT from "$2" in the case for --context without validating that a
value exists, which under set -u can cause an unbound-variable exit; update the
--context branch in the while/case block to check the remaining argument count
before accessing $2 (e.g., if [ "$#" -lt 2 ] then print a clear "missing value
for --context" error and exit 1), otherwise assign CONTEXT="$2" and shift 2;
reference the existing CONTEXT variable and the case branch handling --context
when making this change.
Comment on lines
+130
to
+133
| # 1f. Runtime creds for nonexistent session → 404 | ||
| BODY=$(curl -s "${AUTH[@]}" "$BACKEND/api/projects/default/agentic-sessions/nonexistent/credentials/coderabbit") | ||
| assert_json "$BODY" "error" "CodeRabbit credentials not configured" "Runtime creds for missing session returns 404" | ||
|
|
Contributor
There was a problem hiding this comment.
Missing HTTP status assertion for the 404 runtime-credentials case.
Line 130/Line 132 says this scenario is “returns 404”, but the test only checks the error field in the body. This can pass even if the endpoint regresses to a non-404 status.
Proposed fix
# 1f. Runtime creds for nonexistent session → 404
-BODY=$(curl -s "${AUTH[@]}" "$BACKEND/api/projects/default/agentic-sessions/nonexistent/credentials/coderabbit")
-assert_json "$BODY" "error" "CodeRabbit credentials not configured" "Runtime creds for missing session returns 404"
+STATUS=$(curl -s -o /dev/null -w "%{http_code}" "${AUTH[@]}" "$BACKEND/api/projects/default/agentic-sessions/nonexistent/credentials/coderabbit")
+BODY=$(curl -s "${AUTH[@]}" "$BACKEND/api/projects/default/agentic-sessions/nonexistent/credentials/coderabbit")
+assert_status "$STATUS" "404" "Runtime creds for missing session returns 404"
+assert_json "$BODY" "error" "CodeRabbit credentials not configured" "Runtime creds error message matches backend contract"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@scripts/test-coderabbit-integration.sh` around lines 130 - 133, The test
currently only inspects BODY for the error message but doesn't verify the HTTP
status; update the runtime-creds-for-nonexistent-session block to capture the
HTTP status from curl (alongside BODY) and assert that the status equals 404;
reuse the existing BODY variable and the test's assertion helper (or add an
assert for STATUS) so the block that queries
"/api/projects/default/agentic-sessions/nonexistent/credentials/coderabbit"
verifies both the response body contains "CodeRabbit credentials not configured"
and the HTTP status is 404.
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.
Automated Inner-Loop Review Stack
Adds CodeRabbit as the enforcement layer for code review before PR creation. The agent reviews its own code locally, fixes findings, and retries — the PR arrives clean. No post-merge review noise.
How it works
The hook is a command-type
PreToolUseentry in.claude/settings.jsonthat fires on everyBashtool call. Non-gh pr createcommands pass through instantly (exit 0).Where it runs
.claude/settings.jsonhooks loaded directly--setting-sources project→ same hooksbash scripts/hooks/coderabbit-review-gate.sh(noCLAUDE_TOOL_INPUT→ runs review directly)Same script, same CodeRabbit CLI, same flags in all three contexts.
Circuit breakers
What's in the PR
Review gate (the point of this PR)
scripts/hooks/coderabbit-review-gate.sh— dual-mode gate script.claude/settings.json— PreToolUse Bash command hook entrydocs/internal/adr/0008-automate-code-reviews.md— architecture decisionCodeRabbit credential plumbing (infrastructure for private repos)
CODERABBIT_API_KEYinjected viaasyncio.gather, cleared on turn completionTesting & docs
scripts/test-coderabbit-integration.sh)Models / tools
coderabbit review --agent --base main) — CodeRabbit's AI model analyzes the diffImplements ADR-0008. Supersedes #1145.
🤖 Generated with Claude Code