Skip to content

Commit 457f64c

Browse files
sbauzaclaude
andcommitted
fix: address CodeRabbit review feedback
- Sort Gerrit instances by name for deterministic API responses - Register missing /version route in backend - Encode instanceName in frontend proxy routes to prevent path injection - Clean up stale Gerrit config when instance list is empty - Add debug logging to bare except blocks in MCP credential checks - Update integrations-panel tests for 5th integration (Gerrit) - Fix markdown heading spacing (MD022) - Clarify K8s Secrets encryption-at-rest requires cluster config - Align contract types with implementation (required fields) - Fix instanceName char range in spec (2-63, not 1-63) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
1 parent 9367dd6 commit 457f64c

10 files changed

Lines changed: 33 additions & 16 deletions

File tree

components/backend/handlers/integrations_status.go

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -4,6 +4,7 @@ import (
44
"context"
55
"log"
66
"net/http"
7+
"sort"
78

89
"github.com/gin-gonic/gin"
910
)
@@ -141,6 +142,10 @@ func getGerritStatusForUser(ctx context.Context, userID string) gin.H {
141142
return gin.H{"instances": []gin.H{}}
142143
}
143144

145+
sort.Slice(instances, func(i, j int) bool {
146+
return instances[i].InstanceName < instances[j].InstanceName
147+
})
148+
144149
result := make([]gin.H, 0, len(instances))
145150
for _, creds := range instances {
146151
result = append(result, gin.H{

components/backend/routes.go

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ func registerRoutes(r *gin.Engine) {
1212
api := r.Group("/api")
1313
{
1414
// Public endpoints (no auth required)
15+
api.GET("/version", handlers.GetVersion)
1516
api.GET("/workflows/ootb", handlers.ListOOTBWorkflows)
1617
// Global runner-types endpoint (no workspace overrides — for admin pages)
1718
api.GET("/runner-types", handlers.GetRunnerTypesGlobal)

components/frontend/src/app/api/auth/gerrit/[instanceName]/disconnect/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@ export async function DELETE(
66
{ params }: { params: Promise<{ instanceName: string }> }
77
) {
88
const { instanceName } = await params
9+
const safeInstanceName = encodeURIComponent(instanceName)
910
const headers = await buildForwardHeadersAsync(request)
1011

11-
const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${instanceName}/disconnect`, {
12+
const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${safeInstanceName}/disconnect`, {
1213
method: 'DELETE',
1314
headers,
1415
})

components/frontend/src/app/api/auth/gerrit/[instanceName]/status/route.ts

Lines changed: 2 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,9 +6,10 @@ export async function GET(
66
{ params }: { params: Promise<{ instanceName: string }> }
77
) {
88
const { instanceName } = await params
9+
const safeInstanceName = encodeURIComponent(instanceName)
910
const headers = await buildForwardHeadersAsync(request)
1011

11-
const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${instanceName}/status`, {
12+
const resp = await fetch(`${BACKEND_URL}/auth/gerrit/${safeInstanceName}/status`, {
1213
method: 'GET',
1314
headers,
1415
})

components/frontend/src/app/projects/[name]/sessions/[sessionName]/components/settings/__tests__/integrations-panel.test.tsx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -7,6 +7,7 @@ type IntegrationsData = {
77
gitlab: { connected: boolean };
88
jira: { connected: boolean };
99
google: { connected: boolean };
10+
gerrit: { instances: Array<{ connected: boolean; instanceName: string }> };
1011
} | null;
1112

1213
const mockUseIntegrationsStatus = vi.fn((): { data: IntegrationsData; isPending: boolean } => ({
@@ -36,13 +37,14 @@ describe('IntegrationsPanel', () => {
3637
expect(skeletons.length).toBeGreaterThan(0);
3738
});
3839

39-
it('renders integration cards (GitHub, GitLab, Google Workspace, Jira)', () => {
40+
it('renders integration cards (GitHub, GitLab, Google Workspace, Jira, Gerrit)', () => {
4041
mockUseIntegrationsStatus.mockReturnValue({
4142
data: {
4243
github: { active: null },
4344
gitlab: { connected: false },
4445
jira: { connected: false },
4546
google: { connected: false },
47+
gerrit: { instances: [] },
4648
},
4749
isPending: false,
4850
});
@@ -51,6 +53,7 @@ describe('IntegrationsPanel', () => {
5153
expect(screen.getByText('GitLab')).toBeDefined();
5254
expect(screen.getByText('Google Workspace')).toBeDefined();
5355
expect(screen.getByText('Jira')).toBeDefined();
56+
expect(screen.getByText('Gerrit')).toBeDefined();
5457
});
5558

5659
it('shows connected status for configured integrations', () => {
@@ -60,11 +63,12 @@ describe('IntegrationsPanel', () => {
6063
gitlab: { connected: true },
6164
jira: { connected: true },
6265
google: { connected: false },
66+
gerrit: { instances: [] },
6367
},
6468
isPending: false,
6569
});
6670
render(<IntegrationsPanel />);
67-
// 3 out of 4 configured: badge should show 3/4
68-
expect(screen.getByText('3/4')).toBeDefined();
71+
// 3 out of 5 configured: badge should show 3/5
72+
expect(screen.getByText('3/5')).toBeDefined();
6973
});
7074
});

components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py

Lines changed: 9 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -39,10 +39,13 @@ def generate_gerrit_config(instances: list[dict]) -> None:
3939
4040
Sets GERRIT_CONFIG_PATH env var to point to the generated config.
4141
"""
42+
config_dir = Path("/tmp/gerrit-mcp")
43+
if config_dir.exists():
44+
import shutil
45+
shutil.rmtree(config_dir)
4246
if not instances:
47+
os.environ.pop("GERRIT_CONFIG_PATH", None)
4348
return
44-
45-
config_dir = Path("/tmp/gerrit-mcp")
4649
config_dir.mkdir(parents=True, exist_ok=True)
4750

4851
gerrit_hosts = []
@@ -323,10 +326,10 @@ def check_mcp_authentication(server_name: str) -> tuple[bool | None, str | None]
323326
True,
324327
"Jira credentials available (not yet loaded in session)",
325328
)
326-
except Exception:
327-
pass
328-
except Exception:
329-
pass
329+
except Exception as e:
330+
logger.debug(f"Jira credential probe failed: {e}")
331+
except Exception as e:
332+
logger.debug(f"Jira credential check setup failed: {e}")
330333

331334
return False, "Jira not configured - connect on Integrations page"
332335

docs/internal/integrations/README.md

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -63,6 +63,7 @@ Documentation for integrating the Ambient Code Platform with external services.
6363
- **Instance URL** - Support for self-hosted
6464

6565
### Gerrit
66+
6667
- **HTTP Basic** - Username + HTTP password
6768
- **Gitcookies** - Cookie-based authentication
6869
- **Multi-instance** - Connect multiple Gerrit servers
@@ -92,6 +93,7 @@ All integrations are configured per-project via:
9293
- [GitLab API Endpoints](../api/gitlab-endpoints.md) - API reference
9394

9495
### Gerrit
96+
9597
- [Gerrit Integration](gerrit-integration.md) - Setup and usage guide
9698

9799
### Google Workspace

docs/internal/integrations/gerrit-integration.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -207,7 +207,7 @@ Both instances will be available to the MCP server during AgenticSessions.
207207

208208
- Credentials are stored in a Kubernetes Secret named `gerrit-credentials`
209209
- Each credential entry uses a compound key: `instanceName.userID`
210-
- Secrets are encrypted at rest (standard Kubernetes Secret encryption)
210+
- Stored in Kubernetes Secrets (encrypted at rest when cluster-level encryption is configured)
211211
- Credentials are never logged in plaintext
212212
- Credentials are not exposed in API responses after storage
213213

specs/001-gerrit-integration/contracts/frontend-types.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -26,9 +26,9 @@ export interface GerritConnectResponse {
2626
export interface GerritInstanceStatus {
2727
connected: boolean
2828
instanceName: string
29-
url?: string
30-
authMethod?: GerritAuthMethod
31-
updatedAt?: string
29+
url: string
30+
authMethod: GerritAuthMethod
31+
updatedAt: string
3232
}
3333

3434
export interface GerritTestResponse {

specs/001-gerrit-integration/data-model.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ Represents a user's connection to a single Gerrit instance.
2323

2424
**Validation rules**:
2525
- `url` must be a valid HTTPS URL (HTTP allowed for local development only)
26-
- `instanceName` must match `^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$` (lowercase, alphanumeric with hyphens, 1-63 chars)
26+
- `instanceName` must match `^[a-z0-9][a-z0-9-]{0,61}[a-z0-9]$` (lowercase, alphanumeric with hyphens, 2-63 chars)
2727
- When `authMethod` is `http_basic`: both `username` and `httpToken` must be non-empty
2828
- When `authMethod` is `git_cookies`: `gitcookiesContent` must be non-empty and contain at least one cookie line for the target host
2929

0 commit comments

Comments
 (0)