feat: add CodeRabbit integration for AI-powered code review#1314
Closed
jeremyeder wants to merge 23 commits intoambient-code:mainfrom
Closed
feat: add CodeRabbit integration for AI-powered code review#1314jeremyeder wants to merge 23 commits intoambient-code:mainfrom
jeremyeder wants to merge 23 commits intoambient-code:mainfrom
Conversation
…view + Mergify Replaces human code review with an automated confidence stack: CI checks, inner-loop review (Amber/CodeRabbit/Codex against GPS governance + repo conventions), and a reconfigured Mergify queue that merges on label + CI green without requiring human approval. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Consolidate convention documentation from .claude/context/ and .claude/patterns/ into component-level files, create per-component review agents, overhaul existing skills to the Anthropic skill-creator standard, and add enforcement hooks. Infrastructure: - Fix broken .claude/agents symlink → real directory - Create .claude/settings.json with 7 PreToolUse/Stop hooks - Create scripts/claude-hooks/stop-review.sh - Delete 9 speckit + acp-compile commands Documentation consolidation: - Move context/pattern docs to component-level locations - Create components/operator/DEVELOPMENT.md - Add Critical Conventions section to CLAUDE.md - Update BOOKMARKS.md with table format and new paths - Add back-references in all component docs Skills (overhauls): - dev-cluster: pushy description, evals - pr-fixer: pre-flight checks, 30-min timeout, evals - unleash-flag: middleware gating, E2E guidance, GA cleanup lifecycle, evals Skills (promoted from commands): - amber-review, cypress-demo, jira-log → proper skills with frontmatter + evals Skills (new): - /align — convention alignment scoring via convention-eval agent - /scaffold — integration/endpoint/flag scaffolding templates - /memory — auto-memory search, audit, prune, add Agents (new): - backend-review, frontend-review, operator-review, runner-review, security-review - convention-eval (dispatched by /align) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Fix regex patterns in backend-review (B3) and operator-review (O1) to match singular resource names - Clarify scoped-run scoring normalization in convention-eval - Broaden S2 RBAC check scope from write-only to all resource access - Remove hardcoded user path in memory skill, use $HOME placeholder - Fix BlockOwnerDeletion doc to match actual operator code (omitted for RBAC) - Fix context propagation doc to reference ctx param, not req.Context() - Detect untracked files in stop-review hook via git status --porcelain - Exclude src/app/api/ from F2 manual fetch check (false positives) - Resolve [scope] placeholders in align skill dispatch prompt - Add MD040 fenced-code language tags (align, cypress-demo, memory, scaffold) - Add reqDyn nil guard in scaffold handler template - Fix relative path to CLAUDE.md in security-standards.md - Replace non-existent requireFeatureFlag() with inline middleware pattern Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…ew agents - B4: replace naive grep with multiline pattern to detect empty error blocks - B7: add word boundaries to err pattern to reduce false positives - F5: use null-delimited find/xargs for path safety - F7: fix spelling "colocated" → "co-located" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- F6: broaden useQuery scan to include components/ directory - S7: extend secret-scan grep to tsx, js, yaml, yml file types - Unleash E2E: replace hardcoded admin token with Cypress.env() - Memory skill: add explicit secrets/PII exclusion rule - Memory evals: add default /memory (no args) test case - PR fixer evals: add negative test for missing PR number Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Use FeatureEnabledForRequest(c, ...) for request-aware flag evaluation - Add CYPRESS_UNLEASH_ADMIN_TOKEN env var setup instructions for E2E tests Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- pr-fixer: fix gh CLI flag from -q to --jq for state extraction - dev-cluster: remove manual MinIO bucket recovery (conflicts with "never manually recover" rule) - dev-cluster: add CONTAINER_ENGINE to build examples for consistency Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- B8: use null-delimited find/xargs for safe file counting - O2: fix terminal phase from "Error" to "Failed" (matches operator code) - amber-review: add operator DEVELOPMENT.md to context, fix heading to "# Claude Code Review" (matches GHA workflow) - cypress-demo: update stale chatbox-demo.cy.ts ref to sessions.cy.ts - pr-fixer: reorder pre-flight to resolve repo before PR state check, fix -q to --jq - unleash-flag: qualify handlers.FeatureEnabledForRequest in routes.go example - scaffold: add verification note for reference files Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- B2: broaden DynamicClient grep to match all member access, not just .Resource - B4: switch to rg -nUP for proper multiline empty-block detection - B5: exclude test files from error exposure check - O2/operator docs: fix terminal phase from "Error" to "Failed" (matches code) - unleash-flag: fix split infinitive "appears to not exist" → "appears not to exist" Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- operator docs: fix return types to ctrl.Result{}, nil/err (matches Reconcile signature)
- operator docs: fix remaining "Error" → "Failed" in key patterns text
- unleash-flag: replace real-looking token placeholder with generic one
- unleash-flag: add ordered cleanup sequence for GA phase
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- Add response status assertions to cy.request calls - Document token source (Unleash UI or deployment secrets) - Add cypress.config.ts alternative for centralized env mapping Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Phase 1 of spec-kit-auto pipeline. Defines user stories, functional requirements, success criteria, and edge cases for full-stack CodeRabbit integration following the established Jira pattern. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Full-stack integration following the Jira pattern and PR ambient-code#1307 conventions: Backend (Go): - CodeRabbit auth handlers (connect/status/disconnect) with K8s Secret storage - API key validation against CodeRabbit health endpoint - Unified integrations status includes CodeRabbit - Runtime credential endpoint with RBAC for session pods - 16 Ginkgo tests (all passing, 461 total suite green) Frontend (Next.js + React): - Connection card with API key input, show/hide toggle, status indicator - API client layer and React Query hooks - 4 Next.js proxy routes - Wired into IntegrationsClient grid and session integrations panel Runner (Python): - Async credential fetch via shared _fetch_credential helper - CODERABBIT_API_KEY injected into session environment - Cleared on turn completion Pre-commit hook: - Runs coderabbit review on staged changes - Skips gracefully when CLI/auth/changes unavailable - Rate limits treated as non-blocking warnings CI: - GHA smoke test validates .coderabbit.yaml, runs live CodeRabbit review, and tests pre-commit hook skip behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Change from func(string) bool to func(context.Context, string) (bool, error) to match ValidateGitHubToken, ValidateGitLabToken, ValidateJiraToken. Enables proper error differentiation (network error vs invalid key) and context-aware cancellation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…tion Shows how each artifact from the Claude Code automation overhaul (scaffold skill, convention docs, hooks, review agents) shaped the CodeRabbit integration implementation. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…vate repos only Public repos get free CodeRabbit reviews via the GitHub App — no configuration needed. The card now shows this as the default state with a green "Active for public repositories" indicator. API key configuration is collapsed under "Private repository access" with a billing warning: using a key on public repos incurs charges for reviews that are otherwise free. Pre-commit hook updated to check both CODERABBIT_API_KEY and cr auth login session, so local devs on public repos don't need an API key at all. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Validates the full stack against a running cluster: - 6 backend API endpoint tests (auth, connect, status, disconnect, test, runtime) - Unified integrations status includes coderabbit - Frontend integrations page loads - Pre-commit hook exits cleanly Run: ./scripts/test-coderabbit-integration.sh --context kind-<name> Optional: CODERABBIT_API_KEY for live API key validation Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Covers public repos (free, no config), private repos (API key), local dev setup, pre-commit hook, session credential flow, .coderabbit.yaml configuration, and integration test usage. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
- gofmt: coderabbit_auth_test.go, labels.go - ruff format: auth.py - end-of-file-fixer: integrations-panel.tsx Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
…' into 001-coderabbit-integration
Contributor
📝 WalkthroughWalkthroughAdds end-to-end CodeRabbit integration (backend, frontend, runner, tests), introduces new Claude agents/skills and settings/hooks for automated convention reviews, restructures review/engineering documentation, and adds CI/pre-commit/integration scripts and an ADR for automated code-review workflows. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Frontend
participant Backend
participant K8s as Kubernetes (Secret Store)
participant CodeRabbit as CodeRabbit API
User->>Frontend: Submit API key
Frontend->>Frontend: Validate non-empty
Frontend->>Backend: POST /auth/coderabbit/connect (forwarded headers + body)
Backend->>CodeRabbit: GET /api/v1/health (Bearer <apiKey>)
alt valid key
CodeRabbit-->>Backend: 200 OK
Backend->>K8s: Create/Update Secret (coderabbit-credentials[userID]=json)
K8s-->>Backend: Stored
Backend-->>Frontend: 200 OK
Frontend->>User: Connected
else invalid
CodeRabbit-->>Backend: 401/403
Backend-->>Frontend: 200 OK {valid:false, error}
Frontend->>User: Invalid key
end
sequenceDiagram
participant Runner as Runner Pod
participant Backend
participant K8s as Kubernetes (Secret Store)
participant Env as Pod Environment
Runner->>Backend: GET /credentials/coderabbit (session token)
Backend->>K8s: Read coderabbit-credentials for user/session
K8s-->>Backend: {apiKey}
Backend-->>Runner: {apiKey}
Runner->>Env: export CODERABBIT_API_KEY
Runner->>Runner: Run pre-commit review (uses env var)
Runner->>CodeRabbit as CodeRabbit API: review --agent (uses apiKey)
Note right of Runner: On completion or skip, unset CODERABBIT_API_KEY and cleanup
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
⚔️ Resolve merge conflicts
✨ 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: 18
🧹 Nitpick comments (3)
.claude/skills/memory/SKILL.md (1)
56-59: Harden search instructions to avoid unsafe query handling.Line 57 is a bit underspecified for user-provided input. Recommend explicitly requiring literal, escaped matching and bounded output.
Proposed wording update
### `/memory search <query>` 1. Read `MEMORY.md` for the index -2. Grep through all memory files for the query term -3. Read matching files and show relevant excerpts +2. Search memory files using **literal, safely escaped** query matching (no shell interpolation of raw input) +3. Read matching files and show bounded relevant excerpts 4. Report: matching files with frontmatter (name, type, description)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/memory/SKILL.md around lines 56 - 59, The search step "Grep through all memory files for the query term" is underspecified and unsafe for arbitrary user input; update the SKILL.md instruction (the numbered step that contains "Grep through all memory files for the query term") to require literal, escaped matching (no untrusted regex evaluation), mandate input sanitization/escaping before running searches, and impose bounds on results (max files, max matches per file, and max excerpt length) as well as a note to avoid executing shell commands directly; explicitly state these constraints and give exact limits (e.g., escape regex, use literal search mode, limit to N files, M excerpts of K characters) so implementers know how to handle user-provided queries safely..claude/skills/memory/evals/evals.json (1)
1-26: Add an eval for/memory add <topic>routing.This fixture validates 4 routes well, but
/memory add(documented in the skill) is currently untested. Add at least one case to prevent regressions in tool-arg parsing for creation flows.Proposed eval addition
[ + { + "input": "/memory add coderabbit integration rollout", + "expected_tool_call": "Skill", + "expected_args": { "skill": "memory", "args": "add coderabbit integration rollout" }, + "description": "Create a new memory entry from explicit add command" + }, { "input": "/memory audit",🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.claude/skills/memory/evals/evals.json around lines 1 - 26, Add a new eval case entry to the evals JSON array for the "/memory add <topic>" route so creation flows are tested: create a JSON object with "input": "/memory add onboarding" (or another topic), "expected_tool_call": "Skill", and "expected_args": { "skill": "memory", "args": "add onboarding" } plus a short "description" like "Create memory add routing"; place it alongside the existing objects that use the "input"/"expected_tool_call"/"expected_args" pattern so the parser will validate tool-arg parsing for add flows.scripts/pre-commit/coderabbit-review.sh (1)
44-46: Intentional word splitting, but quote the capture.The
$AUTH_ARGSexpansion is intentionally unquoted for word splitting. However, theOUTPUTcapture should handle potential special characters safely.Per SC2086, if you want to suppress the warning while keeping intentional splitting, consider using an array:
♻️ Array-based alternative
-AUTH_ARGS="" +AUTH_ARGS=() if [ -n "${CODERABBIT_API_KEY:-}" ]; then - AUTH_ARGS="--api-key $CODERABBIT_API_KEY" + AUTH_ARGS=(--api-key "$CODERABBIT_API_KEY") elif ! "$CR_BIN" auth status 2>&1 | grep -qi "logged in"; then ... -OUTPUT=$(timeout 300 "$CR_BIN" review --agent --type uncommitted $AUTH_ARGS 2>&1) || EXIT_CODE=$? +OUTPUT=$(timeout 300 "$CR_BIN" review --agent --type uncommitted "${AUTH_ARGS[@]}" 2>&1) || EXIT_CODE=$?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@scripts/pre-commit/coderabbit-review.sh` around lines 44 - 46, The OUTPUT capture currently expands $AUTH_ARGS unquoted which risks word-splitting/special-char issues; change AUTH_ARGS to be an array (e.g., declare -a AUTH_ARGS) and invoke the command using the array expansion syntax so splitting is intentional and safe, and keep the OUTPUT capture quoted; specifically replace the unquoted $AUTH_ARGS usage in the timeout invocation with the array expansion form (e.g., "${AUTH_ARGS[@]}") while keeping OUTPUT=$(timeout 300 "$CR_BIN" review --agent --type uncommitted ... 2>&1) || EXIT_CODE=$? so special characters are preserved and intentional splitting remains.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.claude/skills/cypress-demo/SKILL.md:
- Line 50: Update the frontend health-check line in SKILL.md to use the
forwarded frontend port environment variable instead of hardcoding
http://localhost: change the curl check to target http://localhost with the port
taken from $KIND_FWD_FRONTEND_PORT (use that variable in the URL and optionally
provide a sensible default if desired) so the validation works when kind
forwards the frontend on a non-80 port; locate the line that starts "Verify the
frontend is accessible" and replace the hardcoded URL accordingly.
In @.claude/skills/dev-cluster/SKILL.md:
- Line 354: The recovery guidance currently suggests re-running "make kind-down
&& make kind-up" without preserving any previously supplied env flags (e.g.,
LOCAL_IMAGES, LOCAL_VERTEX), which can flip workflows back to default images;
change the instruction to re-run the original "kind-up" invocation with the same
flags used initially (e.g., re-run "make kind-down && [previous env flags] make
kind-up") so the Makefile target kind-up and its init-minio.sh behavior run
under the same environment—ensure the doc text mentions preserving any exported
env vars or CLI-staged flags (LOCAL_IMAGES, LOCAL_VERTEX, etc.) when retrying
the recovery command.
In @.claude/skills/pr-fixer/SKILL.md:
- Around line 3-7: The skill description currently lists natural-language
triggers ("address PR comments", "auto-fix pull request") that are not covered
by the evaluation tests; either add corresponding examples to evals.json to
validate those triggers or remove those phrases from the description and make
the "When to Invoke This Skill" section match the remaining supported phrases.
Update the description block (the description: > text) and the "When to Invoke
This Skill" section so both contain the same, tested trigger examples, and if
choosing to add tests, add entries for "address PR comments" and "auto-fix pull
request" to evals.json to ensure they are supported.
In @.claude/skills/scaffold/SKILL.md:
- Line 64: The SKILL.md scaffold entry for the ambient-runner auth module points
to the wrong module location; update the scaffolding line that currently
references the ambient-runner auth module so it targets the runner's actual auth
module used at runtime (the ambient-runner auth module under the package's
platform directory) and not the non-existent src path, and ensure the scaffolded
change adds a fetch_{provider}_credentials() implementation following the
existing fetch_jira_credentials pattern so the new credential helper is created
in the correct module.
In @.github/workflows/coderabbit-smoke-test.yml:
- Around line 35-42: Replace mutable action tags by pinning actions/checkout@v6
and actions/setup-node@v6 to their specific commit SHAs (use the corresponding
SHA for the v6 releases) and add an explicit permissions: block at the
workflow/job level to declare minimal GITHUB_TOKEN permissions (e.g., read-only
where possible) before the steps. Locate the steps using actions/checkout and
actions/setup-node in the workflow and update their uses fields to the SHA pins
and add a top-level permissions: mapping to scope token access.
- Around line 68-96: The smoke test masks real failures by silencing the
coderabbit CLI exit code with "|| true" when running the coderabbit review
command (OUTPUT=$(coderabbit review ... 2>&1) || true); remove the "|| true" or
instead capture the command's exit code ($?) immediately after invocation and
fail the step if non-zero and not an authentication error: run coderabbit review
into OUTPUT, save EXIT_CODE=$?, print OUTPUT, then if EXIT_CODE != 0 check for
auth messages (using the existing grep) and exit 1 for any other non-auth
failures so network/CLI/5xx errors fail the job.
In `@components/backend/handlers/coderabbit_auth.go`:
- Around line 41-47: The current code treats every non-200 response as an
invalid key; change the response handling after client.Do(req) so that
resp.StatusCode == http.StatusOK returns (true, nil), 401/403 returns (false,
nil) to denote invalid credentials, and retryable or upstream failures (e.g.,
429 or any 5xx or unexpected statuses) return a non-nil error (e.g.,
fmt.Errorf("upstream error: status %d", resp.StatusCode)) so callers can retry;
keep the existing networkError handling for client.Do errors and ensure
resp.Body.Close() is still deferred.
In `@components/backend/handlers/integration_validation.go`:
- Around line 243-245: The handler currently returns raw err.Error() to clients
via c.JSON which can leak internal/provider details; change the response to
return a generic error message (e.g., "invalid token" or "validation failed")
instead of err.Error(), and move the detailed error into server-side logging
only (log the error and, if needed, the token length via len(token) rather than
the token itself). Update the code paths around the c.JSON call in
integration_validation.go (the block invoking c.JSON and using err.Error()) so
clients get the generic message while internal logs retain the full err for
debugging.
In `@components/backend/handlers/integrations_status.go`:
- Around line 42-43: The handler is calling getCodeRabbitStatusForUser which
ultimately uses the global K8sClient (via GetCodeRabbitCredentials) instead of
the request-scoped reqK8s created earlier; update the code so the request-scoped
Kubernetes client is used for all integration credential reads by threading
reqK8s through the helper chain (e.g., change getCodeRabbitStatusForUser
signature to accept reqK8s and update its call sites) or refactor
GetCodeRabbitCredentials to accept a k8s client parameter and replace usages of
the global K8sClient inside GetCodeRabbitCredentials (and analogous helpers for
GitHub, Google, Jira, GitLab) to use the passed-in client so RBAC is enforced
per-request.
In `@components/backend/handlers/runtime_credentials.go`:
- Around line 351-358: The handler currently checks only reqK8s after calling
GetK8sClientsForRequest, then calls enforceCredentialRBAC which uses reqDyn and
can panic if reqDyn is nil; add a nil guard for reqDyn immediately after the
GetK8sClientsForRequest call (same place as the existing reqK8s check) and
return an appropriate HTTP error (e.g., c.JSON(http.StatusUnauthorized, ... )
and return) to avoid calling enforceCredentialRBAC with a nil dynamic client;
also ensure any internal functions that might panic on nil dynamic clients
return explicit errors (fmt.Errorf with context) instead of panicking.
- Around line 362-371: The handler treats any error from
GetCodeRabbitCredentials as a 500; instead detect "not found" errors and return
404 like other credential handlers. Modify the error branch after calling
GetCodeRabbitCredentials(effectiveUserID) to check the error kind (e.g., using
errors.Is(err, sql.ErrNoRows) or errors.Is(err, store.ErrNotFound) or the
package-specific NotFound sentinel) and call c.JSON(http.StatusNotFound, ...)
when it represents missing credentials; only return
http.StatusInternalServerError for other unexpected errors. Keep the existing
creds == nil check as a fallback if applicable.
In `@components/frontend/src/app/api/auth/coderabbit/connect/route.ts`:
- Around line 8-15: Wrap the outbound fetch to
`${BACKEND_URL}/auth/coderabbit/connect` in a try/catch so network errors don't
throw; in the catch return a JSON Response with an appropriate error object and
status (e.g., 502) and set 'Content-Type': 'application/json' to preserve the
route's error contract. Update the same pattern in the other auth route handlers
(disconnect, github/install, etc.) that perform fetches. Locate the fetch call
in this file (the POST route handler in route.ts) and add the try/catch around
the await fetch(...) and ensure any downstream response handling (resp.text() /
new Response(...)) happens only on success; on failure return a consistent JSON
error response.
In `@components/frontend/src/app/api/auth/coderabbit/test/route.ts`:
- Around line 8-16: The proxy route in route.ts currently calls
fetch(`${BACKEND_URL}/auth/coderabbit/test`, ...) and then returns new
Response(...) without catching fetch/network errors; wrap the backend fetch and
subsequent resp.text() in a try/catch and on failure return a well-formed JSON
Response (e.g., { error: "...", detail: err.message }) with a suitable HTTP
status (502/503) and Content-Type: application/json so callers always receive
structured JSON; apply the same pattern to the other proxy handlers that use
fetch (the status and disconnect routes) and preserve resp.status when the
backend responded successfully.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/integrations-panel.tsx:
- Around line 52-60: The CodeRabbit entry in integrations-panel.tsx currently
hardcodes configured: true and uses coderabbitConfigured for the message, which
incorrectly shows CodeRabbit as available even when coderabbit.connected is
false; change the logic for the object with key "coderabbit" so configured is
derived from both coderabbit.connected and coderabbitConfigured (e.g.,
configured = coderabbit.connected && coderabbitConfigured) and update
configuredMessage to reflect three states (connected+configured, connected but
missing API key, and not connected) so the badge/status correctly reflects
coderabbit.connected and coderabbitConfigured.
In `@components/frontend/src/components/coderabbit-connection-card.tsx`:
- Around line 106-116: The raw HTML <button> in coderabbit-connection-card.tsx
should be replaced with the Shadcn Button component: import Button from the UI
library and swap the element used around the ChevronDown/ChevronRight, text
"Private repository access", and the conditional status dot; set variant="ghost"
and pass through the onClick={() => setShowAdvanced(!showAdvanced)} and
type="button", preserve the className (or convert to className+cn) and all child
elements (including the conditional {status?.connected && ...}) so behavior and
styling remain identical.
In `@scripts/pre-commit/coderabbit-review.sh`:
- Around line 28-29: The script currently builds AUTH_ARGS="--api-key
$CODERABBIT_API_KEY" which exposes CODERABBIT_API_KEY in process listings;
remove the branch that sets AUTH_ARGS when CODERABBIT_API_KEY is present and
stop passing AUTH_ARGS to the CodeRabbit CLI so the CLI reads the key from the
environment instead; search for AUTH_ARGS usage in the script (e.g., where the
CLI is invoked) and ensure the invocation is updated to omit the --api-key
option so the env var alone supplies credentials.
In `@scripts/test-coderabbit-integration.sh`:
- Around line 197-213: The test captures the hook output with OUTPUT=$("$HOOK"
2>&1) || true which causes $? to reflect the `true` fallback and makes HOOK_EXIT
always 0; change the capture to record the hook's exit status directly by
running the hook assignment without the `|| true`, then immediately set
HOOK_EXIT=$? (or run the hook and assign both output and exit code using a
construction that preserves the hook's exit status) so the subsequent if [
"$HOOK_EXIT" -eq 0 ] branch can correctly detect non-zero exits; update the
lines referencing OUTPUT and HOOK_EXIT accordingly to ensure failures are
caught.
---
Nitpick comments:
In @.claude/skills/memory/evals/evals.json:
- Around line 1-26: Add a new eval case entry to the evals JSON array for the
"/memory add <topic>" route so creation flows are tested: create a JSON object
with "input": "/memory add onboarding" (or another topic), "expected_tool_call":
"Skill", and "expected_args": { "skill": "memory", "args": "add onboarding" }
plus a short "description" like "Create memory add routing"; place it alongside
the existing objects that use the "input"/"expected_tool_call"/"expected_args"
pattern so the parser will validate tool-arg parsing for add flows.
In @.claude/skills/memory/SKILL.md:
- Around line 56-59: The search step "Grep through all memory files for the
query term" is underspecified and unsafe for arbitrary user input; update the
SKILL.md instruction (the numbered step that contains "Grep through all memory
files for the query term") to require literal, escaped matching (no untrusted
regex evaluation), mandate input sanitization/escaping before running searches,
and impose bounds on results (max files, max matches per file, and max excerpt
length) as well as a note to avoid executing shell commands directly; explicitly
state these constraints and give exact limits (e.g., escape regex, use literal
search mode, limit to N files, M excerpts of K characters) so implementers know
how to handle user-provided queries safely.
In `@scripts/pre-commit/coderabbit-review.sh`:
- Around line 44-46: The OUTPUT capture currently expands $AUTH_ARGS unquoted
which risks word-splitting/special-char issues; change AUTH_ARGS to be an array
(e.g., declare -a AUTH_ARGS) and invoke the command using the array expansion
syntax so splitting is intentional and safe, and keep the OUTPUT capture quoted;
specifically replace the unquoted $AUTH_ARGS usage in the timeout invocation
with the array expansion form (e.g., "${AUTH_ARGS[@]}") while keeping
OUTPUT=$(timeout 300 "$CR_BIN" review --agent --type uncommitted ... 2>&1) ||
EXIT_CODE=$? so special characters are preserved and intentional splitting
remains.
🪄 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: f604fb93-5dc1-406a-8f41-029deb205e86
📒 Files selected for processing (71)
.claude/agents.claude/agents/backend-review.md.claude/agents/convention-eval.md.claude/agents/frontend-review.md.claude/agents/operator-review.md.claude/agents/runner-review.md.claude/agents/security-review.md.claude/commands/acp-compile.md.claude/commands/cypress-demo.md.claude/commands/speckit.analyze.md.claude/commands/speckit.checklist.md.claude/commands/speckit.clarify.md.claude/commands/speckit.constitution.md.claude/commands/speckit.implement.md.claude/commands/speckit.plan.md.claude/commands/speckit.specify.md.claude/commands/speckit.tasks.md.claude/settings.json.claude/skills/align/SKILL.md.claude/skills/align/evals/evals.json.claude/skills/amber-review/SKILL.md.claude/skills/amber-review/evals/evals.json.claude/skills/cypress-demo/SKILL.md.claude/skills/cypress-demo/evals/evals.json.claude/skills/dev-cluster/SKILL.md.claude/skills/dev-cluster/evals/evals.json.claude/skills/memory/SKILL.md.claude/skills/memory/evals/evals.json.claude/skills/pr-fixer/SKILL.md.claude/skills/pr-fixer/evals/evals.json.claude/skills/scaffold/SKILL.md.claude/skills/scaffold/evals/evals.json.claude/skills/unleash-flag/SKILL.md.claude/skills/unleash-flag/evals/evals.json.github/workflows/coderabbit-smoke-test.yml.pre-commit-config.yamlBOOKMARKS.mdCLAUDE.mdcomponents/backend/DEVELOPMENT.mdcomponents/backend/ERROR_PATTERNS.mdcomponents/backend/K8S_CLIENT_PATTERNS.mdcomponents/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/DEVELOPMENT.mdcomponents/frontend/REACT_QUERY_PATTERNS.mdcomponents/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/operator/DEVELOPMENT.mdcomponents/runners/ambient-runner/ambient_runner/platform/auth.pydocs/internal/adr/0008-automate-code-reviews.mddocs/pr-1307-impact-analysis.mddocs/security-standards.mddocs/src/content/docs/features/coderabbit.mdscripts/claude-hooks/stop-review.shscripts/pre-commit/coderabbit-review.shscripts/test-coderabbit-integration.shspecs/001-coderabbit-integration/checklists/requirements.mdspecs/001-coderabbit-integration/spec.md
💤 Files with no reviewable changes (11)
- .claude/agents
- .claude/commands/speckit.constitution.md
- .claude/commands/speckit.checklist.md
- .claude/commands/speckit.analyze.md
- .claude/commands/speckit.plan.md
- .claude/commands/speckit.clarify.md
- .claude/commands/cypress-demo.md
- .claude/commands/speckit.specify.md
- .claude/commands/speckit.implement.md
- .claude/commands/speckit.tasks.md
- .claude/commands/acp-compile.md
| - Verify `e2e/.env.test` or `e2e/.env` exists with `TEST_TOKEN` | ||
| - Check if `ANTHROPIC_API_KEY` is available (needed for Running state) | ||
| - Verify the kind cluster is up: `kubectl get pods -n ambient-code` | ||
| - Verify the frontend is accessible: `curl -s -o /dev/null -w "%{http_code}" http://localhost` |
Contributor
There was a problem hiding this comment.
Use port-aware frontend health check instead of hardcoded http://localhost.
Line 50 assumes port 80, but kind access is often on $KIND_FWD_FRONTEND_PORT; this can incorrectly fail prerequisite validation.
Suggested doc fix
-- Verify the frontend is accessible: `curl -s -o /dev/null -w "%{http_code}" http://localhost`
+- Verify the frontend is accessible on the assigned kind port:
+ `curl -s -o /dev/null -w "%{http_code}" "http://localhost:${KIND_FWD_FRONTEND_PORT}"`📝 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
| - Verify the frontend is accessible: `curl -s -o /dev/null -w "%{http_code}" http://localhost` | |
| - Verify the frontend is accessible on the assigned kind port: | |
| `curl -s -o /dev/null -w "%{http_code}" "http://localhost:${KIND_FWD_FRONTEND_PORT}"` |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/cypress-demo/SKILL.md at line 50, Update the frontend
health-check line in SKILL.md to use the forwarded frontend port environment
variable instead of hardcoding http://localhost: change the curl check to target
http://localhost with the port taken from $KIND_FWD_FRONTEND_PORT (use that
variable in the URL and optionally provide a sensible default if desired) so the
validation works when kind forwards the frontend on a non-80 port; locate the
line that starts "Verify the frontend is accessible" and replace the hardcoded
URL accordingly.
| ``` | ||
|
|
||
| **Prevention:** If `make kind-up` fails, fix the underlying issue and re-run it rather than manually recovering individual steps. The Makefile runs `init-minio.sh` near the end of `kind-up`. | ||
| **Solution:** Fix the underlying issue (e.g., image pull errors) and re-run `make kind-down && make kind-up`. The Makefile runs `init-minio.sh` near the end of `kind-up`, which creates the required buckets. If `make kind-up` completes successfully, the bucket will exist. |
Contributor
There was a problem hiding this comment.
Preserve original kind-up flags in recovery command.
Line 354 retries with plain make kind-up, which can unintentionally switch users from LOCAL_IMAGES=true / LOCAL_VERTEX=true workflows back to default images and invalidate local-change testing.
Suggested doc fix
-**Solution:** Fix the underlying issue (e.g., image pull errors) and re-run `make kind-down && make kind-up`. The Makefile runs `init-minio.sh` near the end of `kind-up`, which creates the required buckets. If `make kind-up` completes successfully, the bucket will exist.
+**Solution:** Fix the underlying issue (e.g., image pull errors) and re-run `make kind-down`, then re-run the **same** `make kind-up` command/flags you were using before failure (for example: `make kind-up LOCAL_IMAGES=true` or `make kind-up LOCAL_IMAGES=true LOCAL_VERTEX=true`). The Makefile runs `init-minio.sh` near the end of `kind-up`, which creates the required buckets. If `kind-up` completes successfully, the bucket will exist.📝 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
| **Solution:** Fix the underlying issue (e.g., image pull errors) and re-run `make kind-down && make kind-up`. The Makefile runs `init-minio.sh` near the end of `kind-up`, which creates the required buckets. If `make kind-up` completes successfully, the bucket will exist. | |
| **Solution:** Fix the underlying issue (e.g., image pull errors) and re-run `make kind-down`, then re-run the **same** `make kind-up` command/flags you were using before failure (for example: `make kind-up LOCAL_IMAGES=true` or `make kind-up LOCAL_IMAGES=true LOCAL_VERTEX=true`). The Makefile runs `init-minio.sh` near the end of `kind-up`, which creates the required buckets. If `kind-up` completes successfully, the bucket will exist. |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/dev-cluster/SKILL.md at line 354, The recovery guidance
currently suggests re-running "make kind-down && make kind-up" without
preserving any previously supplied env flags (e.g., LOCAL_IMAGES, LOCAL_VERTEX),
which can flip workflows back to default images; change the instruction to
re-run the original "kind-up" invocation with the same flags used initially
(e.g., re-run "make kind-down && [previous env flags] make kind-up") so the
Makefile target kind-up and its init-minio.sh behavior run under the same
environment—ensure the doc text mentions preserving any exported env vars or
CLI-staged flags (LOCAL_IMAGES, LOCAL_VERTEX, etc.) when retrying the recovery
command.
Comment on lines
+3
to
+7
| description: > | ||
| Trigger the PR Fixer GitHub Actions workflow to automatically fix a pull request | ||
| (rebase, address review comments, run lints/tests, push fixes). Use when user | ||
| types /pr-fixer <number>, says "fix PR", "run pr-fixer", "address PR comments", | ||
| or "auto-fix pull request". |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check if skill parsing/routing logic handles natural language triggers
# Search for skill invocation or routing logic
rg -n -C5 'skill.*invoke|parse.*skill|route.*skill' --type=go --type=py --type=ts -g '!**/node_modules/**' -g '!**/vendor/**'
# Look for existing pattern matching or NLP trigger detection
rg -n -C3 'trigger.*pattern|natural.*language.*skill' --type=go --type=py --type=ts -g '!**/node_modules/**'Repository: ambient-code/platform
Length of output: 47
🏁 Script executed:
#!/bin/bash
# First, examine the skill file and its context
echo "=== Skill file context ==="
find . -path './.*' -name "SKILL.md" -o -path './.*' -name "*.md" | head -20
# Find where pr-fixer skill files exist
find . -name "*pr-fixer*" -type f 2>/dev/null | head -20
# Look at the directory structure around SKILL.md
if [ -f "./.claude/skills/pr-fixer/SKILL.md" ]; then
ls -la "./.claude/skills/pr-fixer/" 2>/dev/null
echo "=== SKILL.md content ==="
cat -n "./.claude/skills/pr-fixer/SKILL.md"
fiRepository: ambient-code/platform
Length of output: 4866
🏁 Script executed:
#!/bin/bash
# Search for skill invocation/parsing logic more broadly (all languages, all files)
echo "=== Searching for skill parsing logic ==="
rg -n 'skill' --type-list | grep -E 'go|ts|py|js|yaml|json'
rg -n -i 'invoke.*skill|skill.*invoke|parse.*skill|skill.*parse' -g '!**/node_modules/**' -g '!**/vendor/**' -g '!**/.git/**' | head -30
echo "=== Searching for pr-fixer references ==="
rg -n 'pr-fixer' -g '!**/node_modules/**' -g '!**/vendor/**' -g '!**/.git/**' | head -30Repository: ambient-code/platform
Length of output: 722
🏁 Script executed:
#!/bin/bash
# Look for skill dispatch/routing or trigger detection logic
echo "=== Searching for trigger/dispatch logic ==="
rg -n -i 'dispatch|route|trigger' . -g '!**/node_modules/**' -g '!**/vendor/**' -g '!**/.git/**' --max-count=30
# Look for skill evaluation or testing
echo "=== Looking for eval or test files ==="
fd -e 'eval' -o -name '*eval*' -o -name '*test*skill*' | grep -i skill | head -20Repository: ambient-code/platform
Length of output: 50626
🏁 Script executed:
#!/bin/bash
# Look at the pr-fixer evals directory
if [ -d "./.claude/skills/pr-fixer/evals" ]; then
echo "=== pr-fixer/evals files ==="
ls -la "./.claude/skills/pr-fixer/evals/"
echo ""
# List all evals files
find "./.claude/skills/pr-fixer/evals" -type f | head -20
fi
# Look for skill evaluation/invocation patterns
echo ""
echo "=== Skill-related eval files ==="
find "./.claude" -name "*eval*" -o -name "*test*" | grep -i skill
# Look for any agent or skill invocation configuration
echo ""
echo "=== Agent files in .claude ==="
ls -la "./.claude/agents/" 2>/dev/null || echo "No agents dir"
ls -la "./.claude/" 2>/dev/null | grep -E '\.(md|yaml|json|yml)$'Repository: ambient-code/platform
Length of output: 1706
🏁 Script executed:
#!/bin/bash
# Search for skill invocation/dispatch in .claude directory specifically
echo "=== Searching .claude for skill dispatch/routing ==="
rg -l 'skill|invoke|dispatch' "./.claude" --type markdown --type yaml --type json 2>/dev/null | head -20
# Look at agent files to understand how skills are used
if [ -f "./.claude/agents/backend-review.md" ]; then
echo ""
echo "=== Sample: backend-review.md (first 50 lines) ==="
head -50 "./.claude/agents/backend-review.md"
fiRepository: ambient-code/platform
Length of output: 2206
🏁 Script executed:
#!/bin/bash
# Check if there's documentation on skill invocation
echo "=== Searching for skill invocation documentation ==="
rg -C3 'invoke.*skill|skill.*trigger|natural.*language' "./.claude" --type markdown -A2 -B2 2>/dev/null | head -50
# Look at the SKILL.md format across multiple skills to understand the pattern
echo ""
echo "=== Comparing SKILL.md files ==="
for skill_file in ./.claude/skills/*/SKILL.md; do
if [ -f "$skill_file" ]; then
echo "=== $skill_file ==="
head -20 "$skill_file"
echo ""
fi
doneRepository: ambient-code/platform
Length of output: 6818
🏁 Script executed:
#!/bin/bash
# Read the pr-fixer evals.json
echo "=== pr-fixer/evals/evals.json ==="
cat "./.claude/skills/pr-fixer/evals/evals.json"
echo ""
echo "=== Comparing with another skill: align/evals/evals.json ==="
cat "./.claude/skills/align/evals/evals.json" | head -100Repository: ambient-code/platform
Length of output: 1528
🏁 Script executed:
#!/bin/bash
# Search the codebase for how skills/evals are processed
echo "=== Looking for skill evaluation processing ==="
rg -n 'evals\.json|skill.*eval|trigger.*eval' -g '!**/node_modules/**' -g '!**/.git/**' --max-count=30 | head -40
# Look for Claude configuration that might reference skills
echo ""
echo "=== .claude/settings.json ==="
cat "./.claude/settings.json"
# Look for how natural language is mapped to skills
echo ""
echo "=== Searching for skill dispatch/matching logic ==="
rg -n -i 'skill.*match|dispatch.*skill|parse.*skill|natural.*language' -g '!**/node_modules/**' -g '!**/.git/**' --max-count=20Repository: ambient-code/platform
Length of output: 3440
The description overstates support for some natural language trigger patterns. The skill evaluation file (evals.json) confirms that natural language triggers are supported and tested—examples like "fix PR 1234" and "run the pr fixer on PR #987" correctly invoke the skill. However, the patterns "address PR comments" and "auto-fix pull request" mentioned in the description are not covered by the evaluation tests, so they may not be reliably recognized. Consider either:
- Testing those patterns in
evals.jsonif they should be supported, or - Removing them from the description if they aren't implemented.
Also note: the "When to Invoke This Skill" section (lines 70–76) shows different example phrases than the description claims; align these for clarity.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/pr-fixer/SKILL.md around lines 3 - 7, The skill description
currently lists natural-language triggers ("address PR comments", "auto-fix pull
request") that are not covered by the evaluation tests; either add corresponding
examples to evals.json to validate those triggers or remove those phrases from
the description and make the "When to Invoke This Skill" section match the
remaining supported phrases. Update the description block (the description: >
text) and the "When to Invoke This Skill" section so both contain the same,
tested trigger examples, and if choosing to add tests, add entries for "address
PR comments" and "auto-fix pull request" to evals.json to ensure they are
supported.
|
|
||
| | File | Purpose | Template | | ||
| |------|---------|----------| | ||
| | `components/runners/ambient-runner/src/auth.py` | Add `fetch_{provider}_credentials()` | Follow `fetch_jira_credentials` pattern | |
Contributor
There was a problem hiding this comment.
Runner scaffold path is wrong and will scaffold to a non-existent module location.
Line [64] targets components/runners/ambient-runner/src/auth.py, but the runner auth module here is components/runners/ambient-runner/ambient_runner/platform/auth.py. This will misplace generated integration code.
Proposed fix
-| `components/runners/ambient-runner/src/auth.py` | Add `fetch_{provider}_credentials()` | Follow `fetch_jira_credentials` pattern |
+| `components/runners/ambient-runner/ambient_runner/platform/auth.py` | Add `fetch_{provider}_credentials()` | Follow `fetch_jira_credentials` pattern |🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.claude/skills/scaffold/SKILL.md at line 64, The SKILL.md scaffold entry for
the ambient-runner auth module points to the wrong module location; update the
scaffolding line that currently references the ambient-runner auth module so it
targets the runner's actual auth module used at runtime (the ambient-runner auth
module under the package's platform directory) and not the non-existent src
path, and ensure the scaffolded change adds a fetch_{provider}_credentials()
implementation following the existing fetch_jira_credentials pattern so the new
credential helper is created in the correct module.
Comment on lines
+8
to
+16
| const resp = await fetch(`${BACKEND_URL}/auth/coderabbit/test`, { | ||
| method: 'POST', | ||
| headers, | ||
| body, | ||
| }) | ||
|
|
||
| const data = await resp.text() | ||
| return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } }) | ||
| } |
Contributor
There was a problem hiding this comment.
Handle backend fetch failures in the proxy route.
Line 8 can throw on backend/network failure; without a try/catch, callers get an unstructured 500 path instead of JSON, which can break the client flow (same pattern applies to status and disconnect routes).
Suggested fix
export async function POST(request: Request) {
const headers = await buildForwardHeadersAsync(request)
const body = await request.text()
-
- const resp = await fetch(`${BACKEND_URL}/auth/coderabbit/test`, {
- method: 'POST',
- headers,
- body,
- })
-
- 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/coderabbit/test`, {
+ method: 'POST',
+ headers,
+ body,
+ })
+
+ const data = await resp.text()
+ return new Response(data, { status: resp.status, headers: { 'Content-Type': 'application/json' } })
+ } catch {
+ return Response.json({ error: 'CodeRabbit service 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/test/route.ts` around lines 8
- 16, The proxy route in route.ts currently calls
fetch(`${BACKEND_URL}/auth/coderabbit/test`, ...) and then returns new
Response(...) without catching fetch/network errors; wrap the backend fetch and
subsequent resp.text() in a try/catch and on failure return a well-formed JSON
Response (e.g., { error: "...", detail: err.message }) with a suitable HTTP
status (502/503) and Content-Type: application/json so callers always receive
structured JSON; apply the same pattern to the other proxy handlers that use
fetch (the status and disconnect routes) and preserve resp.status when the
backend responded successfully.
Comment on lines
+52
to
+60
| { | ||
| key: "coderabbit", | ||
| name: "CodeRabbit", | ||
| configured: true, | ||
| configuredMessage: | ||
| coderabbitConfigured | ||
| ? "Active. API key configured for private repos." | ||
| : "Active for public repositories. No configuration needed.", | ||
| }, |
Contributor
There was a problem hiding this comment.
Don't report CodeRabbit as configured for every session.
Line 55 hardcodes a green state even when coderabbit.connected is false. For sessions backed by private repos, the runner still needs the API key, so the badge/count and status here can claim CodeRabbit is available when it is not.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@components/frontend/src/app/projects/`[name]/sessions/[sessionName]/components/settings/integrations-panel.tsx
around lines 52 - 60, The CodeRabbit entry in integrations-panel.tsx currently
hardcodes configured: true and uses coderabbitConfigured for the message, which
incorrectly shows CodeRabbit as available even when coderabbit.connected is
false; change the logic for the object with key "coderabbit" so configured is
derived from both coderabbit.connected and coderabbitConfigured (e.g.,
configured = coderabbit.connected && coderabbitConfigured) and update
configuredMessage to reflect three states (connected+configured, connected but
missing API key, and not connected) so the badge/status correctly reflects
coderabbit.connected and coderabbitConfigured.
Backend: - Differentiate 401/403 (invalid key) from 5xx (upstream error) in validator - Don't return raw err.Error() to clients in TestCodeRabbitConnection - Handle NotFound as 404 in GetCodeRabbitCredentialsForSession Frontend: - Replace raw <button> with Shadcn Button in connection card CI: - Pin actions/checkout and actions/setup-node to commit SHAs - Add permissions: contents: read to workflow - Fix || true masking real CLI failures in smoke test Pre-commit hook: - Remove --api-key from CLI args (CLI reads CODERABBIT_API_KEY env directly) - Avoids exposing key in process listings Test script: - Fix HOOK_EXIT always evaluating to 0 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
components/backend/handlers/coderabbit_auth.go (1)
183-298:⚠️ Potential issue | 🔴 CriticalRequest-scoped RBAC is still bypassed during Secret CRUD.
These helpers still read and write
coderabbit-credentialsthrough the package-globalK8sClient, so the user-facing endpoints only authenticate the caller and then perform the actual Secret operations as the backend service account. That breaks per-request authorization and tenant isolation for connect/status/disconnect. Thread the request-scoped client returned byGetK8sClientsForRequest(c)into these helpers and use it for all Secret operations.As per coding guidelines, "
**/*.go: UseGetK8sClientsForRequest(c)for all user-facing API operations; never use the backend service account directly".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/backend/handlers/coderabbit_auth.go` around lines 183 - 298, Update these helpers to use the request-scoped K8s client instead of the package-global K8sClient: change the signatures of storeCodeRabbitCredentials, GetCodeRabbitCredentials, and DeleteCodeRabbitCredentials to accept the client object returned by GetK8sClientsForRequest(c) (or a typed kubernetes client parameter), replace all uses of the global K8sClient.CoreV1().Secrets(...) with the passed-in client's CoreV1().Secrets(...), and then update every caller to obtain the request-scoped client via GetK8sClientsForRequest(c) and pass it through; preserve existing retry/logic and error handling.
🤖 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 72-90: The workflow exposes the secret by passing it on the
coderabbit review command line; remove the explicit --api-key
"$CODERABBIT_API_KEY" argument from the coderabbit review invocation so the CLI
uses the CODERABBIT_API_KEY provided in the env block; keep the existing env:
CODERABBIT_API_KEY export and ensure no other steps echo or print that variable
(refer to the coderabbit review command in the diff).
In `@scripts/test-coderabbit-integration.sh`:
- Around line 170-184: The probe computes FRONTEND_PORT but then ignores it;
update the check so FRONTEND_STATUS uses the discovered FRONTEND_PORT (e.g.,
curl "http://localhost:${FRONTEND_PORT}/integrations") or, alternatively,
implement a kubectl port-forward like the backend probe and point the curl at
the forwarded local port; change the code paths that set and read FRONTEND_PORT
and FRONTEND_STATUS to use the chosen approach and ensure the pass/skip messages
still reference the correct port.
---
Duplicate comments:
In `@components/backend/handlers/coderabbit_auth.go`:
- Around line 183-298: Update these helpers to use the request-scoped K8s client
instead of the package-global K8sClient: change the signatures of
storeCodeRabbitCredentials, GetCodeRabbitCredentials, and
DeleteCodeRabbitCredentials to accept the client object returned by
GetK8sClientsForRequest(c) (or a typed kubernetes client parameter), replace all
uses of the global K8sClient.CoreV1().Secrets(...) with the passed-in client's
CoreV1().Secrets(...), and then update every caller to obtain the request-scoped
client via GetK8sClientsForRequest(c) and pass it through; preserve existing
retry/logic and error handling.
🪄 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: b523b16a-9043-4aea-827a-18dac21ad6d0
📒 Files selected for processing (7)
.github/workflows/coderabbit-smoke-test.ymlcomponents/backend/handlers/coderabbit_auth.gocomponents/backend/handlers/integration_validation.gocomponents/backend/handlers/runtime_credentials.gocomponents/frontend/src/components/coderabbit-connection-card.tsxscripts/pre-commit/coderabbit-review.shscripts/test-coderabbit-integration.sh
🚧 Files skipped from review as they are similar to previous changes (2)
- components/backend/handlers/integration_validation.go
- components/backend/handlers/runtime_credentials.go
Comment on lines
+72
to
+90
| env: | ||
| CODERABBIT_API_KEY: ${{ secrets.CODERABBIT_API_KEY }} | ||
| run: | | ||
| echo "=== Running CodeRabbit review against real API ===" | ||
|
|
||
| # Skip if no API key (fork PRs, missing secret) | ||
| if [ -z "$CODERABBIT_API_KEY" ]; then | ||
| echo "CODERABBIT_API_KEY not set - skipping live review" | ||
| echo "This is expected for fork PRs or when the secret is not configured" | ||
| exit 0 | ||
| fi | ||
|
|
||
| # Review the config file itself using agent mode for structured output | ||
| EXIT_CODE=0 | ||
| OUTPUT=$(coderabbit review \ | ||
| --agent \ | ||
| --files .coderabbit.yaml \ | ||
| --api-key "$CODERABBIT_API_KEY" \ | ||
| 2>&1) || EXIT_CODE=$? |
Contributor
There was a problem hiding this comment.
Don't put CODERABBIT_API_KEY on the process command line.
This step already exports the key in env, but Line 89 passes it again as --api-key, which exposes the secret to process listings on the runner. Let the CLI read the environment variable directly.
Suggested fix
OUTPUT=$(coderabbit review \
--agent \
--files .coderabbit.yaml \
- --api-key "$CODERABBIT_API_KEY" \
2>&1) || EXIT_CODE=$?As per coding guidelines, ".github/workflows/**/*.{yml,yaml}: - Pin action versions to SHA. Verify secrets are not exposed and permissions are scoped."
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In @.github/workflows/coderabbit-smoke-test.yml around lines 72 - 90, The
workflow exposes the secret by passing it on the coderabbit review command line;
remove the explicit --api-key "$CODERABBIT_API_KEY" argument from the coderabbit
review invocation so the CLI uses the CODERABBIT_API_KEY provided in the env
block; keep the existing env: CODERABBIT_API_KEY export and ensure no other
steps echo or print that variable (refer to the coderabbit review command in the
diff).
Comment on lines
+170
to
+184
| FRONTEND_PORT=$($KUBECTL get svc -n "$NAMESPACE" -o json | python3 -c " | ||
| import sys,json | ||
| svcs = json.load(sys.stdin)['items'] | ||
| for s in svcs: | ||
| if 'frontend' in s['metadata']['name']: | ||
| for p in s['spec'].get('ports',[]): | ||
| if p.get('nodePort'): print(p['nodePort']); sys.exit(0) | ||
| print('NONE') | ||
| " 2>/dev/null || echo "NONE") | ||
|
|
||
| if [ "$FRONTEND_PORT" != "NONE" ]; then | ||
| # The frontend is behind the kind NodePort — check container port mapping | ||
| FRONTEND_STATUS=$(curl -s -o /dev/null -w "%{http_code}" "http://localhost:9323/integrations" 2>/dev/null || echo "000") | ||
| if [ "$FRONTEND_STATUS" = "200" ]; then pass "Integrations page loads (HTTP 200)" | ||
| else skip "Integrations page returned $FRONTEND_STATUS (may need auth)"; fi |
Contributor
There was a problem hiding this comment.
The frontend probe ignores the port it just discovered.
This block computes FRONTEND_PORT from the cluster, then Line 182 always curls localhost:9323 anyway. The test will misreport frontend availability anywhere that host mapping differs from that hardcoded port. Use the discovered port, or switch this check to a kubectl port-forward flow like the backend probe.
🤖 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 170 - 184, The probe
computes FRONTEND_PORT but then ignores it; update the check so FRONTEND_STATUS
uses the discovered FRONTEND_PORT (e.g., curl
"http://localhost:${FRONTEND_PORT}/integrations") or, alternatively, implement a
kubectl port-forward like the backend probe and point the curl at the forwarded
local port; change the code paths that set and read FRONTEND_PORT and
FRONTEND_STATUS to use the chosen approach and ensure the pass/skip messages
still reference the correct port.
Contributor
Author
|
Closing — rebased onto main and reopening as a clean single-commit PR. |
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
docs/internal/adr/0008-automate-code-reviews.md)CODERABBIT_API_KEYinjected into session environment viaasyncio.gather, cleared on turn completioncoderabbit review --agent, supports both API key andcr auth login, skips gracefully./scripts/test-coderabbit-integration.sh(9/9 passing against dev cluster)docs/src/content/docs/features/coderabbit.mdPR #1307 impact analysis
This integration was built as a reference implementation of the conventions framework from #1307.
See
docs/pr-1307-impact-analysis.mdfor a detailed before/after comparison.ADR-0008 alignment
ADR-0008 defines an automated inner-loop review using Amber + CodeRabbit + Codex. This PR provides the CodeRabbit credential infrastructure (API key storage, runtime injection, pre-commit hook) that the inner-loop needs to function. See PR #1291 for the full ADR.
Test plan
go build ./...andgo vet ./...passnpx tsc --noEmitandnpm run buildpass (0 errors, 0 warnings)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation