feat(specs): credential sidecar isolation architecture#1599
feat(specs): credential sidecar isolation architecture#1599markturansky wants to merge 9 commits into
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
|
| File | Component | Mode |
|---|---|---|
components/runners/ambient-runner/.mcp.json |
runner | warn |
components/runners/ambient-runner/Dockerfile |
runner | warn |
No action required — these components are in warn mode. Consider using the component's agent workflow for future changes.
📖 Specs: Runner Spec · Runner Constitution
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Enterprise Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
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:
📝 WalkthroughWalkthroughThree spec documents are updated to move integration credentials into CP-injected MCP sidecars with SSE endpoints, sidecar-managed refresh and a github-mcp git-credential proxy; runner no longer holds integration tokens, and security invariants/lifecycle are updated accordingly. ChangesSidecar Credential Isolation
Possibly Related PRs
Important Pre-merge checks failedPlease resolve all errors before merging. Addressing warnings is optional. ❌ Failed checks (1 error, 2 warnings)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
markturansky
force-pushed
the
credential-mcp-sidecar
branch
from
7dac1d1 to
cb9305b
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
specs/agents/runner.spec.md (1)
431-432:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMissing environment variable documentation.
The pod layout in
mcp-server.spec.md(line 1010) introducesCREDENTIAL_MCP_URLS={"github":"http://localhost:8091", ...}but this variable is not documented in the runner's environment variables table.📝 Proposed addition to environment variables table
| `CREDENTIAL_IDS` | JSON map `{provider: id}` — resolved credential IDs for this session | | `AMBIENT_MCP_URL` | Ambient MCP sidecar URL (SSE transport) | +| `CREDENTIAL_MCP_URLS` | JSON map `{provider: url}` — localhost URLs for credential MCP sidecars (e.g. `{"github":"http://localhost:8091"}`) | | `REPOS_JSON` | JSON array of `{url, branch, autoPush}` repo configs |🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/agents/runner.spec.md` around lines 431 - 432, Add a new environment variable row for CREDENTIAL_MCP_URLS to the runner's environment variables table (near the entries for CREDENTIAL_IDS and AMBIENT_MCP_URL) documenting that it is a JSON map of provider to MCP URL (e.g., {"github":"http://localhost:8091"}) used to resolve credential MCP endpoints for this session; mention its format, that values are MCP sidecar URLs (SSE transport) and how it relates to CREDENTIAL_IDS (provider→id mapping) so readers understand both mappings together.
🧹 Nitpick comments (1)
components/ambient-cli/cmd/acpctl/agent/cmd_test.go (1)
359-396: ⚡ Quick winAdd partial-failure tests for
--allstart/stop flows.At Line [359] and Line [465], tests only cover all-success behavior. Please add cases where one agent/session operation fails and assert the command returns/aggregates that failure instead of silently succeeding.
As per coding guidelines
**/*.go: Never silently swallow partial failures; every error path must propagate or be explicitly collected, never discarded.Also applies to: 465-501
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@components/ambient-cli/cmd/acpctl/agent/cmd_test.go` around lines 359 - 396, Add tests that cover partial-failure cases for the --all start/stop flows: update TestStartAgent_AllFlag (and the analogous stop test around the other block) to include at least one agent handler that returns an error status (e.g., 4xx/5xx) or an error StartResponse and assert that running Cmd with "start --all" (and "stop --all") returns a non-nil error or non-zero exit (result.Err) and that the command output/stderr contains the failing agent/session ID and error message instead of silently succeeding; ensure you reference the existing test helpers (testhelper.NewServer, testhelper.Run, sampleAgent) and assert both successful session IDs and the reported failure are present so partial failures are collected and propagated.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ambient-cli/cmd/acpctl/credential/cmd_test.go`:
- Around line 327-344: The test TestTokenCredential_Success currently asserts
the raw token is present in stdout (via sampleTokenResponse and invoking Cmd
"token"), which exposes secrets; update the test to assert the CLI redacts or
omits the token instead — for example, verify that stdout does not contain the
raw token value and that it contains a redaction marker or only the token
length/metadata (e.g., "token: [REDACTED]" or "token length: 12"); apply the
same change to the related token-output tests that reference
sampleTokenResponse/Cmd so they check for redaction/length or generic messages
rather than the literal token string.
---
Outside diff comments:
In `@specs/agents/runner.spec.md`:
- Around line 431-432: Add a new environment variable row for
CREDENTIAL_MCP_URLS to the runner's environment variables table (near the
entries for CREDENTIAL_IDS and AMBIENT_MCP_URL) documenting that it is a JSON
map of provider to MCP URL (e.g., {"github":"http://localhost:8091"}) used to
resolve credential MCP endpoints for this session; mention its format, that
values are MCP sidecar URLs (SSE transport) and how it relates to CREDENTIAL_IDS
(provider→id mapping) so readers understand both mappings together.
---
Nitpick comments:
In `@components/ambient-cli/cmd/acpctl/agent/cmd_test.go`:
- Around line 359-396: Add tests that cover partial-failure cases for the --all
start/stop flows: update TestStartAgent_AllFlag (and the analogous stop test
around the other block) to include at least one agent handler that returns an
error status (e.g., 4xx/5xx) or an error StartResponse and assert that running
Cmd with "start --all" (and "stop --all") returns a non-nil error or non-zero
exit (result.Err) and that the command output/stderr contains the failing
agent/session ID and error message instead of silently succeeding; ensure you
reference the existing test helpers (testhelper.NewServer, testhelper.Run,
sampleAgent) and assert both successful session IDs and the reported failure are
present so partial failures are collected and propagated.
🪄 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: Enterprise
Run ID: 5c4632b0-3e71-4a03-ac13-33121d907708
📒 Files selected for processing (5)
components/ambient-cli/cmd/acpctl/agent/cmd_test.gocomponents/ambient-cli/cmd/acpctl/credential/cmd_test.gospecs/agents/runner.spec.mdspecs/integrations/mcp-server.spec.mdspecs/security/security.spec.md
| func TestTokenCredential_Success(t *testing.T) { | ||
| srv := testhelper.NewServer(t) | ||
| srv.Handle("/api/ambient/v1/credentials/cred-t1/token", func(w http.ResponseWriter, r *http.Request) { | ||
| if r.Method != http.MethodGet { | ||
| t.Errorf("expected GET, got %s", r.Method) | ||
| } | ||
| srv.RespondJSON(t, w, http.StatusOK, sampleTokenResponse("cred-t1", "github", "test-value-gh")) | ||
| }) | ||
|
|
||
| testhelper.Configure(t, srv.URL) | ||
| result := testhelper.Run(t, Cmd, "token", "cred-t1") | ||
| if result.Err != nil { | ||
| t.Fatalf("unexpected error: %v\nstdout: %s\nstderr: %s", result.Err, result.Stdout, result.Stderr) | ||
| } | ||
| if !strings.Contains(result.Stdout, "test-value-gh") { | ||
| t.Errorf("expected raw token in output, got: %s", result.Stdout) | ||
| } | ||
| } |
There was a problem hiding this comment.
Stop enforcing plaintext token output in CLI tests.
At Line [341] and Line [360], the tests require raw token values in stdout/JSON. That bakes sensitive-token exposure into the CLI contract. Assert redacted/omitted token output (or metadata such as length) instead.
[suggested change]
Adjust assertions to block raw token disclosure
- if !strings.Contains(result.Stdout, "test-value-gh") {
- t.Errorf("expected raw token in output, got: %s", result.Stdout)
- }
+ if strings.Contains(result.Stdout, "test-value-gh") {
+ t.Errorf("raw token must not be printed, got: %s", result.Stdout)
+ }
...
- if !strings.Contains(result.Stdout, `"test-value-gl"`) {
- t.Errorf("expected JSON with token, got: %s", result.Stdout)
- }
+ if strings.Contains(result.Stdout, `"test-value-gl"`) {
+ t.Errorf("raw token must not be present in JSON output, got: %s", result.Stdout)
+ }As per coding guidelines **/*.go: Never log, error, or return tokens directly in responses; use len(token) for logging and provide generic messages to users.
Also applies to: 346-362
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-cli/cmd/acpctl/credential/cmd_test.go` around lines 327 -
344, The test TestTokenCredential_Success currently asserts the raw token is
present in stdout (via sampleTokenResponse and invoking Cmd "token"), which
exposes secrets; update the test to assert the CLI redacts or omits the token
instead — for example, verify that stdout does not contain the raw token value
and that it contains a redaction marker or only the token length/metadata (e.g.,
"token: [REDACTED]" or "token length: 12"); apply the same change to the related
token-output tests that reference sampleTokenResponse/Cmd so they check for
redaction/length or generic messages rather than the literal token string.
cb9305b to
7fd65b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/agents/runner.spec.md`:
- Around line 352-357: The docs reference a git credential HTTP endpoint on the
github-mcp sidecar but the MCP server spec only documents stdio and SSE
transports; update mcp-server.spec.md to either (A) formally document the git
credential HTTP endpoint and its request/response contract used by the git
credential helper (describe endpoint path, HTTP method, request fields and
credential-protocol formatted response), or (B) explicitly state that git
credential retrieval uses the existing stdio MCP transport (tool invocation over
stdio) and remove/adjust references to a separate HTTP endpoint in
security.spec.md and runner.spec.md; ensure mentions of "github-mcp", "git
credential helper", "git credential protocol", "stdio transport", and "SSE
transport" are consistent across the three specs.
In `@specs/integrations/mcp-server.spec.md`:
- Around line 987-1031: The credential sidecars (e.g., github-mcp, jira-mcp,
k8s-mcp, google-mcp) lack the backend auth env vars required for them to
re-fetch tokens; add AMBIENT_API_URL, AMBIENT_CP_TOKEN_URL,
AMBIENT_CP_TOKEN_PUBLIC_KEY and SESSION_ID to each credential sidecar's env var
list in the credential provider table and the Pod Layout (same format used for
ambient-mcp) so github-mcp, jira-mcp, k8s-mcp, google-mcp, etc. can authenticate
to the backend and perform their own token refresh cycles as described.
🪄 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: Enterprise
Run ID: dc0c2ac1-063c-4dce-92bf-14b5838e51df
📒 Files selected for processing (3)
specs/agents/runner.spec.mdspecs/integrations/mcp-server.spec.mdspecs/security/security.spec.md
markturansky
changed the title
Claude Code Review - PR #1599SummaryThis PR documents a significant architectural shift: moving from in-process credential management to sidecar-based credential isolation. Three spec files are updated to reflect that integration credentials (GitHub, GitLab, Jira, Google, kubeconfig) SHALL NOT be visible to the agent process, while LLM provider credentials (Anthropic/Vertex) remain in the runner container. The specs are well-written, internally consistent, and represent a positive security enhancement. FindingsBlockerNone Critical1. PR scope mismatch (specs vs. implementation) The test plan mentions: This implies implementation code exists, but it's not in this PR diff. Violated Standard: CLAUDE.md - "Verify contracts and references" Suggested Fix:
Major1. CodeRabbit credential provider not in sidecar table Violated Standard: Cross-cutting convention - "Verify contracts and references. After moving anything, grep scripts, workflows, manifests, and configs" Suggested Fix:
2. Removed Violated Standard: While not explicitly a standards violation, this is a breaking architectural change that affects existing functionality. Suggested Fix:
Minor1. GitLab sidecar image marked as TBD Impact: Low - this is acceptable for a spec document, but signals incomplete design. Suggested Fix: If the image is known, specify it. Otherwise, add a note that this will be determined during implementation. 2. Incomplete test plan checkboxes - [ ] Verify spec changes are consistent across all three documents
- [ ] `cd components/ambient-cli && go test ./cmd/acpctl/credential/ ./cmd/acpctl/agent/`Suggested Fix: Complete the checklist before requesting review or merge. If tests haven't been run, the PR may be premature. Positive Highlights
RecommendationsPriority order:
The specs themselves are high-quality and represent sound architectural thinking. Once the scope/consistency issues are resolved, this will be a strong foundation for the sidecar credential isolation implementation. |
markturansky
force-pushed
the
credential-mcp-sidecar
branch
from
7fd65b8 to
0d895f5
Compare
markturansky
changed the title
Update security, MCP server, and runner specs to define the credential sidecar isolation architecture: integration tokens live exclusively in sidecar containers, never in the runner environment. LLM credentials (Anthropic/Vertex) are exempt. Key additions: - Agent Credential Isolation requirement with testable scenarios - Integration Credential Sidecars table with auth env vars for refresh - Git Credential Proxy endpoint contract (GET /git-credentials) - Migration note: acp in-process MCP server replaced by ambient-mcp SSE sidecar 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
credential-mcp-sidecar
branch
from
0d895f5 to
288d275
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/agents/runner.spec.md`:
- Around line 370-377: Add a new row for the gitlab MCP sidecar to the MCP
Servers table: insert the line for `gitlab-mcp` with SSE port `:8092`,
description "GitLab API tools (repos, MRs, pipelines)" and policy "CP-injected
sidecar, only if `gitlab` credential bound"; ensure the entry uses the same
table formatting and aligns with the existing rows for `github-mcp`, `jira-mcp`,
and `google-mcp` so `gitlab-mcp` is included in the table alongside `ambient`,
`session`, `rubric`, and `corrections`.
🪄 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: Enterprise
Run ID: f71a554b-aeb2-4e39-8c39-f65f9b7d295c
📒 Files selected for processing (3)
specs/agents/runner.spec.mdspecs/integrations/mcp-server.spec.mdspecs/security/security.spec.md
| | `ambient` | SSE (`AMBIENT_MCP_URL`) | 16 platform tools (sessions, agents, projects) | CP-injected sidecar | | ||
| | `github-mcp` | SSE (`:8091`) | GitHub API tools (repos, issues, PRs, actions) | CP-injected sidecar, only if `github` credential bound | | ||
| | `jira-mcp` | SSE (`:8093`) | Jira API tools (issues, search, transitions) | CP-injected sidecar, only if `jira` credential bound | | ||
| | `k8s-mcp` | SSE (`:8094`) | Kubernetes tools (kubectl via MCP) | CP-injected sidecar, only if `kubeconfig` credential bound | | ||
| | `google-mcp` | SSE (`:8095`) | Google Workspace tools (Gmail, Drive) | CP-injected sidecar, only if `google` credential bound | | ||
| | `session` | in-process | `refresh_credentials` | always registered | | ||
| | `rubric` | in-process | `evaluate_rubric` | registered if `.ambient/rubric.md` found | | ||
| | `corrections` | in-process | `log_correction` | always registered | |
There was a problem hiding this comment.
Add gitlab-mcp to the MCP Servers table.
The security spec (line 171) includes GitLab in the list of integration credentials requiring sidecar isolation, and the MCP server spec (lines 987-994) defines a gitlab-mcp sidecar. This table should include:
| `gitlab-mcp` | SSE (`:8092`) | GitLab API tools (repos, MRs, pipelines) | CP-injected sidecar, only if `gitlab` credential bound |Cross-file consistency issue: all three specs must align on the complete credential sidecar set.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~371-~371: The official name of this software platform is spelled with a capital “H”.
Context: ...actions) | CP-injected sidecar, only if github credential bound | | jira-mcp | SSE ...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/agents/runner.spec.md` around lines 370 - 377, Add a new row for the
gitlab MCP sidecar to the MCP Servers table: insert the line for `gitlab-mcp`
with SSE port `:8092`, description "GitLab API tools (repos, MRs, pipelines)"
and policy "CP-injected sidecar, only if `gitlab` credential bound"; ensure the
entry uses the same table formatting and aligns with the existing rows for
`github-mcp`, `jira-mcp`, and `google-mcp` so `gitlab-mcp` is included in the
table alongside `ambient`, `session`, `rubric`, and `corrections`.
Review Update - PR #1599Changes Since Initial ReviewExcellent progress! The PR author has addressed 2 of 5 findings from the initial review and added valuable clarifications. ✅ Issues Addressed1. Migration Path for
|
| Finding | Severity | Status |
|---|---|---|
Migration path for acp tools |
MAJOR | ✅ Resolved |
| CodeRabbit discrepancy | MAJOR | ✅ Resolved (removed from specs) |
| PR scope mismatch | CRITICAL | |
| GitLab TBD | MINOR | ✅ Improved |
| Test plan incomplete | MINOR |
New Review Verdict
2 blockers remain, both are PR metadata issues (not spec content):
- PR title mentions
clibut contains only specs - Clarify scope - Test plan checklist unchecked - Complete or explain status
The spec content itself is now in excellent shape:
- ✅ Comprehensive migration documentation
- ✅ Clear git operations constraints
- ✅ Detailed sidecar token refresh mechanism
- ✅ Testable security scenarios
- ✅ Consistent credential provider lists across all three specs
Recommendation
For merge readiness:
- Update PR title to
feat(specs): credential sidecar isolation specs(removecli) - Check test plan boxes or note if this is specs-only PR (implementation TBD)
- Once metadata fixed: LGTM for merge 🎉
The architectural design is sound and well-documented. Excellent work addressing the review feedback!
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
specs/agents/runner.spec.md (1)
375-379:⚠️ Potential issue | 🟠 Major | ⚡ Quick winAdd
gitlab-mcpto the runner MCP server inventory
gitlab-mcpis defined in the MCP sidecar spec but missing here, so the server inventory is inconsistent across specs. Add the missing row (port:8092, conditional ongitlabbinding).🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@specs/agents/runner.spec.md` around lines 375 - 379, The runner MCP server inventory table is missing the `gitlab-mcp` row; add a new table row mirroring the other MCP entries with name `gitlab-mcp`, SSE URL `:8092`, a short description like "GitLab API tools (repos, issues, MRs, CI)" and note "CP-injected sidecar, only if `gitlab` credential bound" so it matches the pattern used by `github-mcp`, `jira-mcp`, etc.; ensure the row formatting aligns with the existing Markdown table columns (`name | SSE | tools | injection/condition`) and keep the port `:8092`.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@specs/agents/runner.spec.md`:
- Around line 324-349: Update the legacy runner credential-flow text that still
describes populating/clearing credentials inside the runner so it matches the
sidecar-isolation model: remove or rewrite references that say the runner
environment or filesystem is populated/cleared and instead describe CP adding
per-credential sidecar containers (sidecar environment contains only its own
credential), the runner connecting to sidecars via SSE (localhost:{port}/sse),
and the `refresh_credentials` MCP tool (registered under the `session` MCP
server) signaling sidecars to refresh tokens; also ensure the credential-free
fallback section consistently states that no sidecars are added when a project
has no bound credentials.
In `@specs/integrations/mcp-server.spec.md`:
- Around line 987-1001: The spec claims credential sidecars use the same
RSA-OAEP token exchange as ambient-mcp but the env var
AMBIENT_CP_TOKEN_PUBLIC_KEY is missing; add AMBIENT_CP_TOKEN_PUBLIC_KEY to each
credential provider row in the provider table (github-mcp, gitlab-mcp, jira-mcp,
k8s-mcp, google-mcp) and include AMBIENT_CP_TOKEN_PUBLIC_KEY in the pod
layout/env-vars block for credential sidecars so they can perform the RSA-OAEP
token verification alongside AMBIENT_API_URL, AMBIENT_CP_TOKEN_URL, and
SESSION_ID.
- Around line 970-995: The docs conflict: this section says sidecars use SSE
("runner connects to each sidecar as an SSE MCP client on
`http://localhost:{port}/sse`") while earlier text describes stdio/default-stdio
transport; pick one transport (prefer SSE for sidecar HTTP ports) and make all
references consistent: update the earlier stdio/default-stdio mentions to
describe SSE sidecar connectivity, adjust any examples and the "ambient-mcp" /
"Integration Credential Sidecars" descriptions to reference the SSE endpoint and
ports, and remove or clearly scope any remaining stdio text (or conversely, if
you choose stdio, change the `/sse` sentence and port table entries
accordingly).
---
Duplicate comments:
In `@specs/agents/runner.spec.md`:
- Around line 375-379: The runner MCP server inventory table is missing the
`gitlab-mcp` row; add a new table row mirroring the other MCP entries with name
`gitlab-mcp`, SSE URL `:8092`, a short description like "GitLab API tools
(repos, issues, MRs, CI)" and note "CP-injected sidecar, only if `gitlab`
credential bound" so it matches the pattern used by `github-mcp`, `jira-mcp`,
etc.; ensure the row formatting aligns with the existing Markdown table columns
(`name | SSE | tools | injection/condition`) and keep the port `:8092`.
🪄 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: Enterprise
Run ID: f418611b-21d2-45fe-b020-63c93f5fcdc5
📒 Files selected for processing (3)
specs/agents/runner.spec.mdspecs/integrations/mcp-server.spec.mdspecs/security/security.spec.md
| Integration credentials are **isolated in sidecar containers**. The runner container | ||
| has no integration tokens in its environment or filesystem. Each credential-bearing | ||
| MCP sidecar holds only its own credentials and exposes tools via SSE on a localhost | ||
| port. | ||
|
|
||
| LLM provider credentials (Anthropic API key, Vertex AI service account) remain in | ||
| the runner container — they are necessary for inference. | ||
|
|
||
| ### Sidecar Credential Flow | ||
|
|
||
| ``` | ||
| populate_runtime_credentials(context): | ||
| concurrent asyncio.gather: | ||
| _fetch_credential("github") → GITHUB_TOKEN, /tmp/.ambient_github_token | ||
| _fetch_credential("gitlab") → GITLAB_TOKEN, /tmp/.ambient_gitlab_token | ||
| _fetch_credential("google") → GOOGLE_APPLICATION_CREDENTIALS, credentials.json | ||
| _fetch_credential("jira") → JIRA_URL, JIRA_API_TOKEN, JIRA_EMAIL | ||
|
|
||
| clear_runtime_credentials(): | ||
| unset all env vars + delete all temp files | ||
| CP resolves CREDENTIAL_IDS for the Project | ||
| → For each bound credential: | ||
| CP adds a sidecar container to the pod spec | ||
| Sidecar environment contains only its own credential | ||
| Sidecar exposes MCP tools on localhost:{port}/sse | ||
| → Runner connects to sidecars as SSE MCP clients | ||
| → Agent calls MCP tools — never sees raw tokens | ||
| ``` | ||
|
|
||
| The credential fetch uses `context.caller_token` (the user's bearer from `x-caller-token` header) so each user can only access their own credentials. The `BACKEND_API_URL` is validated to be a cluster-local hostname before any request is made (prevents token exfiltration to external hosts). | ||
| Credential sidecars manage their own token refresh cycles. The `refresh_credentials` | ||
| MCP tool (registered under the `session` MCP server) signals sidecars to re-fetch | ||
| tokens from the backend API. Rate-limited to once per 30 seconds. | ||
|
|
||
| The credential-free fallback: Projects with no bound credentials get no credential | ||
| sidecars. The runner operates without integration credentials. |
There was a problem hiding this comment.
Update legacy runner credential flow references to match sidecar isolation
The new model says integration credentials never enter the runner, but the same document still instructs credential population/clearing in-runner (Line 153, Line 238, Line 241). Please align those sections so there is a single authoritative flow.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/agents/runner.spec.md` around lines 324 - 349, Update the legacy runner
credential-flow text that still describes populating/clearing credentials inside
the runner so it matches the sidecar-isolation model: remove or rewrite
references that say the runner environment or filesystem is populated/cleared
and instead describe CP adding per-credential sidecar containers (sidecar
environment contains only its own credential), the runner connecting to sidecars
via SSE (localhost:{port}/sse), and the `refresh_credentials` MCP tool
(registered under the `session` MCP server) signaling sidecars to refresh
tokens; also ensure the credential-free fallback section consistently states
that no sidecars are added when a project has no bound credentials.
| ### Platform MCP Sidecar (`ambient-mcp`) | ||
|
|
||
| Sessions opt into the MCP sidecar by setting the annotation: | ||
| Sessions opt into the platform MCP sidecar by setting the annotation: | ||
|
|
||
| ``` | ||
| ambient-code.io/mcp-sidecar: "true" | ||
| ``` | ||
|
|
||
| This annotation is set on the Session resource at creation time. The operator reads it and injects the `ambient-mcp` container into the runner Job pod. | ||
| This annotation is set on the Session resource at creation time. The CP reads it and injects the `ambient-mcp` container into the runner Job pod. | ||
|
|
||
| ### Integration Credential Sidecars | ||
|
|
||
| For each credential bound to the session's Project (via `CREDENTIAL_IDS`), the CP | ||
| injects an additional sidecar container running the corresponding MCP server. Each | ||
| sidecar has its own isolated environment containing only its credential. The runner | ||
| container has **no** integration credential tokens in its environment or filesystem. | ||
|
|
||
| | Credential Provider | Sidecar Name | Image | Port | Env Vars Injected | | ||
| |---|---|---|---|---| | ||
| | `github` | `github-mcp` | `ghcr.io/github/github-mcp-server` | `:8091` | `GITHUB_PERSONAL_ACCESS_TOKEN`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `gitlab` | `gitlab-mcp` | TBD — no official MCP server exists yet; will require a community or custom image | `:8092` | `GITLAB_TOKEN`, `GITLAB_HOST`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `jira` | `jira-mcp` | `uvx mcp-atlassian` (init + run) | `:8093` | `JIRA_URL`, `JIRA_API_TOKEN`, `JIRA_EMAIL`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `kubeconfig` | `k8s-mcp` | `uvx kubernetes-mcp-server` (init + run) | `:8094` | `KUBECONFIG` (file mount), `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `google` | `google-mcp` | `uvx workspace-mcp` (init + run) | `:8095` | `GOOGLE_OAUTH_*`, `USER_GOOGLE_EMAIL`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
|
|
||
| The runner connects to each sidecar as an SSE MCP client on `http://localhost:{port}/sse`. |
There was a problem hiding this comment.
Sidecar transport mode conflicts with earlier MCP transport definition
This section defines sidecar connectivity as SSE (/sse on localhost), but earlier sections still define sidecar mode as stdio/default stdio. Please reconcile to one transport contract; right now implementation guidance is contradictory.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~989-~989: The official name of this software platform is spelled with a capital “H”.
Context: ...Vars Injected | |---|---|---|---|---| | github | github-mcp | `ghcr.io/github/githu...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/integrations/mcp-server.spec.md` around lines 970 - 995, The docs
conflict: this section says sidecars use SSE ("runner connects to each sidecar
as an SSE MCP client on `http://localhost:{port}/sse`") while earlier text
describes stdio/default-stdio transport; pick one transport (prefer SSE for
sidecar HTTP ports) and make all references consistent: update the earlier
stdio/default-stdio mentions to describe SSE sidecar connectivity, adjust any
examples and the "ambient-mcp" / "Integration Credential Sidecars" descriptions
to reference the SSE endpoint and ports, and remove or clearly scope any
remaining stdio text (or conversely, if you choose stdio, change the `/sse`
sentence and port table entries accordingly).
| | Credential Provider | Sidecar Name | Image | Port | Env Vars Injected | | ||
| |---|---|---|---|---| | ||
| | `github` | `github-mcp` | `ghcr.io/github/github-mcp-server` | `:8091` | `GITHUB_PERSONAL_ACCESS_TOKEN`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `gitlab` | `gitlab-mcp` | TBD — no official MCP server exists yet; will require a community or custom image | `:8092` | `GITLAB_TOKEN`, `GITLAB_HOST`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `jira` | `jira-mcp` | `uvx mcp-atlassian` (init + run) | `:8093` | `JIRA_URL`, `JIRA_API_TOKEN`, `JIRA_EMAIL`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `kubeconfig` | `k8s-mcp` | `uvx kubernetes-mcp-server` (init + run) | `:8094` | `KUBECONFIG` (file mount), `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
| | `google` | `google-mcp` | `uvx workspace-mcp` (init + run) | `:8095` | `GOOGLE_OAUTH_*`, `USER_GOOGLE_EMAIL`, `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, `SESSION_ID` | | ||
|
|
||
| The runner connects to each sidecar as an SSE MCP client on `http://localhost:{port}/sse`. | ||
|
|
||
| Each credential sidecar receives `AMBIENT_API_URL`, `AMBIENT_CP_TOKEN_URL`, and | ||
| `SESSION_ID` so it can re-fetch tokens from the backend API when credentials | ||
| approach expiry. The sidecar authenticates to the backend using the same | ||
| RSA-OAEP token exchange mechanism as the `ambient-mcp` sidecar. | ||
|
|
There was a problem hiding this comment.
Credential sidecar auth contract is incomplete for RSA-OAEP flow
You state sidecars use the same RSA-OAEP exchange as ambient-mcp, but credential sidecars are missing AMBIENT_CP_TOKEN_PUBLIC_KEY in both the provider table and pod layout (e.g., Line 989–Line 993, Line 1043–Line 1049). Without it, the documented exchange cannot be executed.
Also applies to: 1043-1049
🧰 Tools
🪛 LanguageTool
[uncategorized] ~989-~989: The official name of this software platform is spelled with a capital “H”.
Context: ...Vars Injected | |---|---|---|---|---| | github | github-mcp | `ghcr.io/github/githu...
(GITHUB)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@specs/integrations/mcp-server.spec.md` around lines 987 - 1001, The spec
claims credential sidecars use the same RSA-OAEP token exchange as ambient-mcp
but the env var AMBIENT_CP_TOKEN_PUBLIC_KEY is missing; add
AMBIENT_CP_TOKEN_PUBLIC_KEY to each credential provider row in the provider
table (github-mcp, gitlab-mcp, jira-mcp, k8s-mcp, google-mcp) and include
AMBIENT_CP_TOKEN_PUBLIC_KEY in the pod layout/env-vars block for credential
sidecars so they can perform the RSA-OAEP token verification alongside
AMBIENT_API_URL, AMBIENT_CP_TOKEN_URL, and SESSION_ID.
Wave 1 of credential sidecar isolation. Adds buildCredentialSidecars() to inject per-provider MCP sidecar containers (GitHub, Jira, K8s, Google) into session pods. Each sidecar runs on a dedicated port (8091-8094) and the runner receives CREDENTIAL_MCP_URLS env var mapping providers to localhost sidecar URLs. Sidecars are only injected when the corresponding image env var is configured. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Wave 2 of credential sidecar isolation. When CREDENTIAL_MCP_URLS is set (injected by CP from Wave 1), the runner connects to credential sidecars via SSE instead of spawning subprocess MCP servers. Integration tokens (GitHub, GitLab, Jira, kubeconfig, Google) are no longer injected into the runner environment in sidecar mode. Git identity (user name/email) is still extracted for commit attribution. Falls back to subprocess mode when sidecars are not configured. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Wave 3 of credential sidecar isolation. When credential sidecars are active, system prompts instruct the agent to use mcp__github__push_files and mcp__github__create_pull_request instead of git push/gh pr create. Token visibility prompts are replaced with MCP tool documentation. Falls back to legacy token-based prompts when sidecars are not configured. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Wave 4 of credential sidecar isolation. Adds Dockerfiles for four credential sidecars: - GitHub: wraps official github-mcp-server with streamable-http transport - Jira: mcp-atlassian via mcp-proxy SSE bridge - K8s: kubernetes-mcp-server via mcp-proxy SSE bridge - Google: workspace-mcp via mcp-proxy SSE bridge Adds Makefile targets (build-credential-sidecars and per-provider). Updates runner sidecar registry with per-provider transport types (streamable_http for GitHub, sse for mcp-proxy-based sidecars). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Wave 5 of credential sidecar isolation. Adds comprehensive tests for buildCredentialSidecars covering: empty credentials, missing image config, single sidecar, multiple sidecars, unknown providers, localhost image pull policy, and CREDENTIAL_MCP_URLS JSON round-trip. Also fixes GitHub sidecar Dockerfile binary path (/server/github-mcp-server). 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Wave 6 of credential sidecar isolation. Adds a shared Go entrypoint binary (credential-entrypoint) that handles RSA-OAEP token exchange with the CP token server, fetches integration credentials from the backend API, injects them as env vars, then exec's the actual MCP server binary. Includes background token refresh. Validates API URL hostname to prevent token exfiltration. Updates GitHub sidecar Dockerfile to use the entrypoint wrapper with multi-stage build. Updates Makefile to use repo root as build context. 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Wave 7 of credential sidecar isolation. Removes the credential-entrypoint binary from git tracking and adds it to .gitignore. All tests pass: - CP: go vet + go test (6 new reconciler tests + 3 existing) - Runner: 714 tests pass, 11 skipped 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
Claude Code Review - PR #1599 (Implementation)SummaryThis PR implements the credential sidecar isolation architecture across the entire stack: Control Plane (Go), Runner (Python), sidecar Dockerfiles, and shared entrypoint. The implementation quality is excellent with strong test coverage (6 new unit tests), proper security contexts, and comprehensive backward compatibility. The code follows platform conventions closely. Minor issues exist but none are blockers. FindingsBlockerNone CriticalNone Major1. Hardcoded placeholder credential token in sidecar environment ProviderEnvs: map[string]string{
"GITHUB_PERSONAL_ACCESS_TOKEN": "__CREDENTIAL_TOKEN__",
},This injects a literal Problems:
Violated Standard: Security standards - "No tokens in logs/errors/responses" (indirect: placeholder could leak in error messages) Suggested Fix: Option A (Preferred): Don't inject placeholder tokens. Let entrypoint fetch and set env vars before exec: // Remove ProviderEnvs from registry (except for Jira multi-field case)
// Entrypoint reads provider from CREDENTIAL_PROVIDER and sets appropriate env varsOption B: Document the placeholder contract clearly:
Recommendation: Option A - Remove placeholder injection. The entrypoint already knows the provider ( 2. Missing credential token injection for Jira/K8s/Google sidecars "jira": {
Name: "credential-jira",
ImageField: "JiraMCPImage",
Port: 8092,
// ProviderEnvs missing - no token injection!
},Impact: These sidecars will receive Violated Standard: Cross-cutting convention - "Verify contracts and references" Suggested Fix: Since the entrypoint sets credentials based on
Looking at Downgrade to MINOR after re-examining - the entrypoint owns credential injection, not the CP. The GitHub placeholder is vestigial and should be removed for consistency. Minor1. Test plan incomplete - [ ] Full kind cluster E2E with credential sidecars injected
- [ ] Verify MCP tool connectivity from runner to sidecar
- [ ] Verify token refresh cycle in sidecar entrypointSuggested Fix: Check these boxes if tests passed, or document that E2E testing is in progress / to be completed before merge. 2. Credential URL validation uses string suffix matching if !strings.HasSuffix(hostname, ".svc.cluster.local") &&
!strings.HasSuffix(hostname, ".svc") &&
hostname != "localhost" &&
hostname != "127.0.0.1" {
return fmt.Errorf("refusing to send credentials to external host: %s", hostname)
}Problems:
Violated Standard: Security standards - "Input validation" Suggested Fix: // More robust cluster-internal validation
hostname := parsed.Hostname()
// Exact match for localhost
if hostname == "localhost" || hostname == "127.0.0.1" || hostname == "::1" {
// OK
} else if strings.HasSuffix(hostname, ".svc.cluster.local") && !strings.Contains(hostname, "..") {
// OK - cluster-internal service DNS
} else if strings.HasSuffix(hostname, ".svc") && !strings.Contains(hostname, "..") {
// OK - short service DNS
} else {
return fmt.Errorf("refusing to send credentials to external host: %s", hostname)
}Better: use 3. No "securityContext": map[string]interface{}{
"allowPrivilegeEscalation": false,
"capabilities": map[string]interface{}{
"drop": []interface{}{"ALL"},
},
},Missing Violated Standard: Operator conventions - "Restricted SecurityContext on all containers" Suggested Fix: "securityContext": map[string]interface{}{
"allowPrivilegeEscalation": false,
"readOnlyRootFilesystem": true, // Add this
"capabilities": map[string]interface{}{
"drop": []interface{}{"ALL"},
},
},Note: Only add if sidecars don't need temp file writes. The GitHub MCP server likely needs 4. Runner MCP connection transport autodetection logic try:
server_config = _build_server_config_streamable_http(
base_url=sidecar_url, server_name=server_name
)
except Exception as e:
logger.warning(
"Failed to build SSE config for %s at %s: %s — falling back to subprocess",
server_name,
sidecar_url,
e,
)
server_config = _build_server_config_subprocess(...)Problem: This silently falls back to subprocess mode on any SSE config error, which could hide configuration issues. The sidecar URL is explicitly provided in Violated Standard: Error handling conventions - "Never silently swallow partial failures" Suggested Fix: if sidecar_url:
# Explicit sidecar mode - SSE only, no subprocess fallback
try:
server_config = _build_server_config_streamable_http(...)
except Exception as e:
logger.error(f"Failed to connect to {server_name} sidecar at {sidecar_url}: {e}")
raise # Don't fall back - this violates isolation
else:
# Legacy subprocess mode
server_config = _build_server_config_subprocess(...)5. Dockerfile entrypoint could use ENTRYPOINT ["credential-entrypoint", "github-mcp-server"]
CMD ["http", "--port", "8091"]If someone overrides CMD with arguments that look like flags (e.g., Suggested Fix: ENTRYPOINT ["credential-entrypoint"]
CMD ["github-mcp-server", "http", "--port", "8091"]Or use ENTRYPOINT ["credential-entrypoint", "--"]
CMD ["github-mcp-server", "http", "--port", "8091"]Impact: Very low - CMD is unlikely to be overridden in practice. 6. Missing Violated Standard: CLAUDE.md - "Restricted SecurityContext on all containers: Suggested Fix: "securityContext": map[string]interface{}{
"allowPrivilegeEscalation": false,
"runAsNonRoot": true, // Add this
"capabilities": map[string]interface{}{
"drop": []interface{}{"ALL"},
},
},Positive Highlights
RecommendationsPriority order:
Architecture ReviewThe sidecar isolation architecture is sound and well-executed: ✅ Security: Integration tokens never touch the runner container The implementation successfully achieves the spec's goals without breaking existing functionality. Code Quality Assessment
Overall: 4.8/5 - Excellent implementation with minor polish needed. Final VerdictRecommend: Approve with minor changes The implementation is production-ready with the following minor fixes:
All other findings are optional improvements. The core architecture is solid, tests are comprehensive, and backward compatibility is preserved. Great work! |
There was a problem hiding this comment.
Actionable comments posted: 9
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@components/ambient-control-plane/internal/reconciler/kube_reconciler.go`:
- Around line 491-502: The code currently injects credential sidecars and sets
CREDENTIAL_MCP_URLS unconditionally; change the logic in the block that calls
buildCredentialSidecars (around function/method kube_reconciler.go where
buildCredentialSidecars, envVar and containers are used) to first verify
token-exchange prerequisites (non-empty CPTokenURL and CPTokenPublicKey on the
session/config) and only proceed to append credSidecars and set the
CREDENTIAL_MCP_URLS env var when both are present; if either is missing, skip
injecting the sidecars/env and emit a clear log entry (r.logger.Warn/Info)
indicating token-exchange is not configured for session.ID. Also handle
json.Marshal error explicitly instead of swallowing it: if json.Marshal returns
an error, log the error with context (including session.ID and credMCPURLs) and
do not mutate containers. Ensure references to buildCredentialSidecars,
CREDENTIAL_MCP_URLS, envVar and containers remain consistent.
In `@components/credential-sidecars/entrypoint/main.go`:
- Around line 118-120: The error currently returns the raw response body
(string(body)) which may leak secrets; update the error returned from the
credential fetch logic to omit the body contents and instead include only
non-sensitive metadata (e.g., resp.StatusCode and len(body)) and a generic
message. Locate the fmt.Errorf call that references resp.StatusCode and body in
main.go (the credential fetch/HTTP response handling) and replace the detailed
body string with a sanitized message that reports status and body length or a
generic failure text while keeping the fmt.Errorf usage for context.
- Around line 30-34: The current check for tokenURL, publicKey, and sessionID in
main (using the variables tokenURL, publicKey, sessionID) prints an error then
calls execCommand(os.Args[1:]) which allows the process to "fail open"; instead,
make the path fail closed by stopping startup when any required bootstrap env
var is missing: replace the execCommand call with an immediate non-zero exit
(e.g., log to stderr or via process logger and call os.Exit(1) or log.Fatalf) so
the sidecar does not continue running without valid auth configuration; update
the conditional in main.go accordingly and remove the execCommand invocation for
this error path.
In `@components/credential-sidecars/k8s/Dockerfile`:
- Around line 10-11: The image starts kubernetes_mcp_server with --kubeconfig
/tmp/.ambient_kubeconfig but never runs credential-entrypoint (which creates and
refreshes that file), so add the credential bootstrap wrapper to the container
start sequence: run credential-entrypoint before starting the existing
ENTRYPOINT/CMD so it writes /tmp/.ambient_kubeconfig and runs token refresh,
then exec into the current processes (mcp-proxy and python -m
kubernetes_mcp_server). Update the Dockerfile ENTRYPOINT/CMD invocation (and
mirror the same change in google/Dockerfile) to invoke credential-entrypoint as
the wrapper that then starts mcp-proxy and kubernetes_mcp_server.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`:
- Around line 227-243: In _build_sidecar_mcp_servers:
json.loads(credential_mcp_urls_raw) can produce non-dict values and entries can
contain non-string URLs which cause .items() or .rstrip('/') to raise; update
the function to validate that the parsed credential_mcp_urls is a dict and
iterate only its items where provider is a string and url is a string (or coerce
to str safely) and skip/ log any malformed entries, before looking up
_CREDENTIAL_SIDECAR_REGISTRY and building the servers dict so only well-formed
server_name, transport_type and url values are added to servers.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 386-387: The helper _using_credential_sidecars currently returns
true for any non-empty CREDENTIAL_MCP_URLS string which lets malformed config
enable sidecar mode and skip legacy token setup; change
_using_credential_sidecars to parse and validate CREDENTIAL_MCP_URLS (split by
comma/whitespace, trim entries, verify each is a well-formed URL/host:port as
your app expects) and only return True when at least one valid URL is present;
if parsing/validation fails, return False (and optionally log the invalid value)
so the code paths that set up legacy tokens/helpers still run.
In `@components/runners/ambient-runner/ambient_runner/platform/prompts.py`:
- Around line 244-247: The prompt currently chooses GIT_PUSH_MCP_STEPS whenever
credential_mcp_urls is non-empty; instead, validate that credential_mcp_urls can
be parsed and contains a usable GitHub entry before gating MCP instructions.
Update the logic around credential_mcp_urls (the variable) to: try to
parse/normalize it, ensure it contains a "github" entry or an entry whose
type/host == "github" (handle JSON parsing errors and missing keys), and only
append GIT_PUSH_MCP_STEPS.format(branch=push_branch) when that validation
succeeds; otherwise append GIT_PUSH_STEPS.format(branch=push_branch). Apply the
same validation/guarded decision to the other occurrence covering lines
~256-264.
In `@Makefile`:
- Around line 228-254: Add the new Makefile targets to the .PHONY declaration so
their recipes always run: include build-credential-sidecars,
build-credential-github, build-credential-jira, build-credential-k8s, and
build-credential-google in the existing .PHONY list (update the .PHONY line near
the top where other phony targets are declared) to prevent name collisions with
files of the same names.
- Around line 228-254: The new credential sidecar targets
(build-credential-github, build-credential-jira, build-credential-k8s,
build-credential-google) are not wired into the main flows; update the primary
meta targets build-all, push-all and the _kind-load-images target to include
these sidecar targets (or include a sidecar variable list) so that running make
build-all, make push-all, or make _kind-load-images (and make kind-up
LOCAL_IMAGES=true) will build/push/load the credential sidecars; reference the
targets by name (build-credential-github, build-credential-jira,
build-credential-k8s, build-credential-google) when adding them as dependencies
or appending them to the appropriate image lists used by build-all, push-all and
_kind-load-images.
🪄 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: Enterprise
Run ID: d6ecc95e-bbe0-4be7-b8b6-b0697cb20328
📒 Files selected for processing (15)
.gitignoreMakefilecomponents/ambient-control-plane/cmd/ambient-control-plane/main.gocomponents/ambient-control-plane/internal/config/config.gocomponents/ambient-control-plane/internal/reconciler/kube_reconciler.gocomponents/ambient-control-plane/internal/reconciler/kube_reconciler_test.gocomponents/credential-sidecars/entrypoint/go.modcomponents/credential-sidecars/entrypoint/main.gocomponents/credential-sidecars/github/Dockerfilecomponents/credential-sidecars/google/Dockerfilecomponents/credential-sidecars/jira/Dockerfilecomponents/credential-sidecars/k8s/Dockerfilecomponents/runners/ambient-runner/ambient_runner/bridges/claude/mcp.pycomponents/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/ambient_runner/platform/prompts.py
✅ Files skipped from review due to trivial changes (3)
- .gitignore
- components/credential-sidecars/jira/Dockerfile
- components/credential-sidecars/entrypoint/go.mod
| credSidecars, credMCPURLs := r.buildCredentialSidecars(session.ID, credentialIDs) | ||
| containers = append(containers, credSidecars...) | ||
| if len(credMCPURLs) > 0 { | ||
| raw, err := json.Marshal(credMCPURLs) | ||
| if err == nil { | ||
| containers[0].(map[string]interface{})["env"] = append( | ||
| containers[0].(map[string]interface{})["env"].([]interface{}), | ||
| envVar("CREDENTIAL_MCP_URLS", string(raw)), | ||
| ) | ||
| } | ||
| r.logger.Info().Int("count", len(credSidecars)).Str("session_id", session.ID).Msg("credential sidecars injected") | ||
| } |
There was a problem hiding this comment.
Gate credential sidecar injection on token-exchange prerequisites.
This path injects sidecars and CREDENTIAL_MCP_URLS without validating CPTokenURL and CPTokenPublicKey. If those are empty, runner still gets endpoints that cannot authenticate/fetch credentials.
Suggested fix
- credSidecars, credMCPURLs := r.buildCredentialSidecars(session.ID, credentialIDs)
- containers = append(containers, credSidecars...)
- if len(credMCPURLs) > 0 {
+ canUseCredentialSidecars := r.cfg.CPTokenURL != "" && r.cfg.CPTokenPublicKey != ""
+ if !canUseCredentialSidecars && len(credentialIDs) > 0 {
+ r.logger.Warn().
+ Str("session_id", session.ID).
+ Msg("credential sidecars disabled: CP token exchange config missing")
+ }
+ credSidecars, credMCPURLs := []interface{}{}, map[string]string{}
+ if canUseCredentialSidecars {
+ credSidecars, credMCPURLs = r.buildCredentialSidecars(session.ID, credentialIDs)
+ containers = append(containers, credSidecars...)
+ }
+ if len(credMCPURLs) > 0 {
raw, err := json.Marshal(credMCPURLs)
if err == nil {
containers[0].(map[string]interface{})["env"] = append(
containers[0].(map[string]interface{})["env"].([]interface{}),
envVar("CREDENTIAL_MCP_URLS", string(raw)),
)
}
r.logger.Info().Int("count", len(credSidecars)).Str("session_id", session.ID).Msg("credential sidecars injected")
}As per coding guidelines "Never silently swallow partial failures; every error path must propagate or be explicitly collected, never discarded".
📝 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.
| credSidecars, credMCPURLs := r.buildCredentialSidecars(session.ID, credentialIDs) | |
| containers = append(containers, credSidecars...) | |
| if len(credMCPURLs) > 0 { | |
| raw, err := json.Marshal(credMCPURLs) | |
| if err == nil { | |
| containers[0].(map[string]interface{})["env"] = append( | |
| containers[0].(map[string]interface{})["env"].([]interface{}), | |
| envVar("CREDENTIAL_MCP_URLS", string(raw)), | |
| ) | |
| } | |
| r.logger.Info().Int("count", len(credSidecars)).Str("session_id", session.ID).Msg("credential sidecars injected") | |
| } | |
| canUseCredentialSidecars := r.cfg.CPTokenURL != "" && r.cfg.CPTokenPublicKey != "" | |
| if !canUseCredentialSidecars && len(credentialIDs) > 0 { | |
| r.logger.Warn(). | |
| Str("session_id", session.ID). | |
| Msg("credential sidecars disabled: CP token exchange config missing") | |
| } | |
| credSidecars, credMCPURLs := []interface{}{}, map[string]string{} | |
| if canUseCredentialSidecars { | |
| credSidecars, credMCPURLs = r.buildCredentialSidecars(session.ID, credentialIDs) | |
| containers = append(containers, credSidecars...) | |
| } | |
| if len(credMCPURLs) > 0 { | |
| raw, err := json.Marshal(credMCPURLs) | |
| if err == nil { | |
| containers[0].(map[string]interface{})["env"] = append( | |
| containers[0].(map[string]interface{})["env"].([]interface{}), | |
| envVar("CREDENTIAL_MCP_URLS", string(raw)), | |
| ) | |
| } | |
| r.logger.Info().Int("count", len(credSidecars)).Str("session_id", session.ID).Msg("credential sidecars injected") | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/ambient-control-plane/internal/reconciler/kube_reconciler.go`
around lines 491 - 502, The code currently injects credential sidecars and sets
CREDENTIAL_MCP_URLS unconditionally; change the logic in the block that calls
buildCredentialSidecars (around function/method kube_reconciler.go where
buildCredentialSidecars, envVar and containers are used) to first verify
token-exchange prerequisites (non-empty CPTokenURL and CPTokenPublicKey on the
session/config) and only proceed to append credSidecars and set the
CREDENTIAL_MCP_URLS env var when both are present; if either is missing, skip
injecting the sidecars/env and emit a clear log entry (r.logger.Warn/Info)
indicating token-exchange is not configured for session.ID. Also handle
json.Marshal error explicitly instead of swallowing it: if json.Marshal returns
an error, log the error with context (including session.ID and credMCPURLs) and
do not mutate containers. Ensure references to buildCredentialSidecars,
CREDENTIAL_MCP_URLS, envVar and containers remain consistent.
| if tokenURL == "" || publicKey == "" || sessionID == "" { | ||
| fmt.Fprintf(os.Stderr, "AMBIENT_CP_TOKEN_URL, AMBIENT_CP_TOKEN_PUBLIC_KEY, SESSION_ID required\n") | ||
| execCommand(os.Args[1:]) | ||
| return | ||
| } |
There was a problem hiding this comment.
Do not fail open when required bootstrap env vars are missing.
When required vars are absent, this currently logs and continues to exec the server. That hides a broken auth path and starts a non-functional sidecar.
Suggested fix
if tokenURL == "" || publicKey == "" || sessionID == "" {
fmt.Fprintf(os.Stderr, "AMBIENT_CP_TOKEN_URL, AMBIENT_CP_TOKEN_PUBLIC_KEY, SESSION_ID required\n")
- execCommand(os.Args[1:])
- return
+ os.Exit(1)
}As per coding guidelines "Never silently swallow partial failures; every error path must propagate or be explicitly collected, never discarded".
📝 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.
| if tokenURL == "" || publicKey == "" || sessionID == "" { | |
| fmt.Fprintf(os.Stderr, "AMBIENT_CP_TOKEN_URL, AMBIENT_CP_TOKEN_PUBLIC_KEY, SESSION_ID required\n") | |
| execCommand(os.Args[1:]) | |
| return | |
| } | |
| if tokenURL == "" || publicKey == "" || sessionID == "" { | |
| fmt.Fprintf(os.Stderr, "AMBIENT_CP_TOKEN_URL, AMBIENT_CP_TOKEN_PUBLIC_KEY, SESSION_ID required\n") | |
| os.Exit(1) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/credential-sidecars/entrypoint/main.go` around lines 30 - 34, The
current check for tokenURL, publicKey, and sessionID in main (using the
variables tokenURL, publicKey, sessionID) prints an error then calls
execCommand(os.Args[1:]) which allows the process to "fail open"; instead, make
the path fail closed by stopping startup when any required bootstrap env var is
missing: replace the execCommand call with an immediate non-zero exit (e.g., log
to stderr or via process logger and call os.Exit(1) or log.Fatalf) so the
sidecar does not continue running without valid auth configuration; update the
conditional in main.go accordingly and remove the execCommand invocation for
this error path.
| if resp.StatusCode != http.StatusOK { | ||
| return fmt.Errorf("credential fetch HTTP %d: %s", resp.StatusCode, string(body)) | ||
| } |
There was a problem hiding this comment.
Avoid including raw credential API response bodies in errors.
Returning string(body) in the error can leak sensitive backend details into stderr/log sinks.
Suggested fix
if resp.StatusCode != http.StatusOK {
- return fmt.Errorf("credential fetch HTTP %d: %s", resp.StatusCode, string(body))
+ return fmt.Errorf("credential fetch HTTP %d (body_len=%d)", resp.StatusCode, len(body))
}As per coding guidelines "Never log, error, or return tokens directly in responses; use len(token) for logging and provide generic messages to users".
📝 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.
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("credential fetch HTTP %d: %s", resp.StatusCode, string(body)) | |
| } | |
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("credential fetch HTTP %d (body_len=%d)", resp.StatusCode, len(body)) | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/credential-sidecars/entrypoint/main.go` around lines 118 - 120,
The error currently returns the raw response body (string(body)) which may leak
secrets; update the error returned from the credential fetch logic to omit the
body contents and instead include only non-sensitive metadata (e.g.,
resp.StatusCode and len(body)) and a generic message. Locate the fmt.Errorf call
that references resp.StatusCode and body in main.go (the credential fetch/HTTP
response handling) and replace the detailed body string with a sanitized message
that reports status and body length or a generic failure text while keeping the
fmt.Errorf usage for context.
| ENTRYPOINT ["mcp-proxy", "--sse-port", "8093", "--"] | ||
| CMD ["python", "-m", "kubernetes_mcp_server", "--kubeconfig", "/tmp/.ambient_kubeconfig", "--disable-multi-cluster"] |
There was a problem hiding this comment.
Credential bootstrap wrapper is missing for this sidecar flow (also applies to google/Dockerfile).
kubernetes_mcp_server is started with --kubeconfig /tmp/.ambient_kubeconfig, but this image never runs credential-entrypoint (the component that writes that file and handles token refresh). This breaks credential hydration at runtime.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/credential-sidecars/k8s/Dockerfile` around lines 10 - 11, The
image starts kubernetes_mcp_server with --kubeconfig /tmp/.ambient_kubeconfig
but never runs credential-entrypoint (which creates and refreshes that file), so
add the credential bootstrap wrapper to the container start sequence: run
credential-entrypoint before starting the existing ENTRYPOINT/CMD so it writes
/tmp/.ambient_kubeconfig and runs token refresh, then exec into the current
processes (mcp-proxy and python -m kubernetes_mcp_server). Update the Dockerfile
ENTRYPOINT/CMD invocation (and mirror the same change in google/Dockerfile) to
invoke credential-entrypoint as the wrapper that then starts mcp-proxy and
kubernetes_mcp_server.
| def _build_sidecar_mcp_servers(credential_mcp_urls_raw: str) -> dict: | ||
| try: | ||
| credential_mcp_urls = json.loads(credential_mcp_urls_raw) | ||
| except (json.JSONDecodeError, TypeError): | ||
| logger.warning("Failed to parse CREDENTIAL_MCP_URLS — skipping credential MCP servers") | ||
| return {} | ||
|
|
||
| servers: dict = {} | ||
| for provider, url in credential_mcp_urls.items(): | ||
| spec = _CREDENTIAL_SIDECAR_REGISTRY.get(provider, {}) | ||
| server_name = spec.get("server_name", provider) | ||
| transport_type = spec.get("type", "sse") | ||
| path = spec.get("path", "/sse") | ||
| servers[server_name] = { | ||
| "type": transport_type, | ||
| "url": f"{url.rstrip('/')}{path}", | ||
| } |
There was a problem hiding this comment.
Harden sidecar URL parsing to avoid runtime crashes on malformed config.
json.loads() may return non-dicts, and url values may be non-strings; current logic can fail on .items()/.rstrip('/') and break credential MCP initialization. Validate shape and skip bad entries.
Suggested fix
def _build_sidecar_mcp_servers(credential_mcp_urls_raw: str) -> dict:
try:
credential_mcp_urls = json.loads(credential_mcp_urls_raw)
except (json.JSONDecodeError, TypeError):
logger.warning("Failed to parse CREDENTIAL_MCP_URLS — skipping credential MCP servers")
return {}
+ if not isinstance(credential_mcp_urls, dict):
+ logger.warning("CREDENTIAL_MCP_URLS must be a JSON object: provider -> base URL")
+ return {}
servers: dict = {}
for provider, url in credential_mcp_urls.items():
+ if not isinstance(provider, str) or not isinstance(url, str) or not url.strip():
+ logger.warning("Skipping invalid credential sidecar entry for provider=%r", provider)
+ continue
spec = _CREDENTIAL_SIDECAR_REGISTRY.get(provider, {})
server_name = spec.get("server_name", provider)
transport_type = spec.get("type", "sse")
path = spec.get("path", "/sse")
servers[server_name] = {
"type": transport_type,
"url": f"{url.rstrip('/')}{path}",
}As per coding guidelines, "Handle errors and edge cases explicitly rather than ignoring them".
📝 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.
| def _build_sidecar_mcp_servers(credential_mcp_urls_raw: str) -> dict: | |
| try: | |
| credential_mcp_urls = json.loads(credential_mcp_urls_raw) | |
| except (json.JSONDecodeError, TypeError): | |
| logger.warning("Failed to parse CREDENTIAL_MCP_URLS — skipping credential MCP servers") | |
| return {} | |
| servers: dict = {} | |
| for provider, url in credential_mcp_urls.items(): | |
| spec = _CREDENTIAL_SIDECAR_REGISTRY.get(provider, {}) | |
| server_name = spec.get("server_name", provider) | |
| transport_type = spec.get("type", "sse") | |
| path = spec.get("path", "/sse") | |
| servers[server_name] = { | |
| "type": transport_type, | |
| "url": f"{url.rstrip('/')}{path}", | |
| } | |
| def _build_sidecar_mcp_servers(credential_mcp_urls_raw: str) -> dict: | |
| try: | |
| credential_mcp_urls = json.loads(credential_mcp_urls_raw) | |
| except (json.JSONDecodeError, TypeError): | |
| logger.warning("Failed to parse CREDENTIAL_MCP_URLS — skipping credential MCP servers") | |
| return {} | |
| if not isinstance(credential_mcp_urls, dict): | |
| logger.warning("CREDENTIAL_MCP_URLS must be a JSON object: provider -> base URL") | |
| return {} | |
| servers: dict = {} | |
| for provider, url in credential_mcp_urls.items(): | |
| if not isinstance(provider, str) or not isinstance(url, str) or not url.strip(): | |
| logger.warning("Skipping invalid credential sidecar entry for provider=%r", provider) | |
| continue | |
| spec = _CREDENTIAL_SIDECAR_REGISTRY.get(provider, {}) | |
| server_name = spec.get("server_name", provider) | |
| transport_type = spec.get("type", "sse") | |
| path = spec.get("path", "/sse") | |
| servers[server_name] = { | |
| "type": transport_type, | |
| "url": f"{url.rstrip('/')}{path}", | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/runners/ambient-runner/ambient_runner/bridges/claude/mcp.py`
around lines 227 - 243, In _build_sidecar_mcp_servers:
json.loads(credential_mcp_urls_raw) can produce non-dict values and entries can
contain non-string URLs which cause .items() or .rstrip('/') to raise; update
the function to validate that the parsed credential_mcp_urls is a dict and
iterate only its items where provider is a string and url is a string (or coerce
to str safely) and skip/ log any malformed entries, before looking up
_CREDENTIAL_SIDECAR_REGISTRY and building the servers dict so only well-formed
server_name, transport_type and url values are added to servers.
| def _using_credential_sidecars() -> bool: | ||
| return bool(os.getenv("CREDENTIAL_MCP_URLS", "").strip()) |
There was a problem hiding this comment.
Don’t activate sidecar mode from a non-empty string alone.
Current detection treats malformed CREDENTIAL_MCP_URLS as enabled, then skips legacy token/helper setup. If sidecar config is invalid, credentials are unavailable in both paths.
Suggested fix
def _using_credential_sidecars() -> bool:
- return bool(os.getenv("CREDENTIAL_MCP_URLS", "").strip())
+ raw = os.getenv("CREDENTIAL_MCP_URLS", "").strip()
+ if not raw:
+ return False
+ try:
+ parsed = _json.loads(raw)
+ except (ValueError, TypeError):
+ logger.warning("Invalid CREDENTIAL_MCP_URLS; falling back to legacy credential mode")
+ return False
+ return isinstance(parsed, dict) and any(
+ isinstance(v, str) and v.strip() for v in parsed.values()
+ )As per coding guidelines, "Handle errors and edge cases explicitly rather than ignoring them".
Also applies to: 400-404
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around
lines 386 - 387, The helper _using_credential_sidecars currently returns true
for any non-empty CREDENTIAL_MCP_URLS string which lets malformed config enable
sidecar mode and skip legacy token setup; change _using_credential_sidecars to
parse and validate CREDENTIAL_MCP_URLS (split by comma/whitespace, trim entries,
verify each is a well-formed URL/host:port as your app expects) and only return
True when at least one valid URL is present; if parsing/validation fails, return
False (and optionally log the invalid value) so the code paths that set up
legacy tokens/helpers still run.
| if credential_mcp_urls: | ||
| prompt += GIT_PUSH_MCP_STEPS.format(branch=push_branch) | ||
| else: | ||
| prompt += GIT_PUSH_STEPS.format(branch=push_branch) |
There was a problem hiding this comment.
Gate GitHub MCP push instructions on a valid GitHub sidecar entry.
The prompt currently switches to MCP push steps on any non-empty CREDENTIAL_MCP_URLS, even if JSON is invalid or lacks "github". That can direct the agent to unavailable tools.
Suggested fix
- credential_mcp_urls = os.getenv("CREDENTIAL_MCP_URLS", "").strip()
+ credential_mcp_urls = os.getenv("CREDENTIAL_MCP_URLS", "").strip()
+ github_mcp_enabled = False
+ if credential_mcp_urls:
+ try:
+ import json as _json
+ parsed_urls = _json.loads(credential_mcp_urls)
+ github_mcp_enabled = (
+ isinstance(parsed_urls, dict)
+ and isinstance(parsed_urls.get("github"), str)
+ and bool(parsed_urls.get("github", "").strip())
+ )
+ except (ValueError, TypeError):
+ logger.warning("Invalid CREDENTIAL_MCP_URLS in prompt builder; using legacy git instructions")
@@
- if credential_mcp_urls:
+ if github_mcp_enabled:
prompt += GIT_PUSH_MCP_STEPS.format(branch=push_branch)
else:
prompt += GIT_PUSH_STEPS.format(branch=push_branch)
@@
- if credential_mcp_urls:
- try:
- import json as _json
- urls = _json.loads(credential_mcp_urls)
- if "github" in urls:
- prompt += GITHUB_MCP_PROMPT
- except (ValueError, TypeError):
- pass
+ if github_mcp_enabled:
+ prompt += GITHUB_MCP_PROMPTAs per coding guidelines, "Handle errors and edge cases explicitly rather than ignoring them".
Also applies to: 256-264
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@components/runners/ambient-runner/ambient_runner/platform/prompts.py` around
lines 244 - 247, The prompt currently chooses GIT_PUSH_MCP_STEPS whenever
credential_mcp_urls is non-empty; instead, validate that credential_mcp_urls can
be parsed and contains a usable GitHub entry before gating MCP instructions.
Update the logic around credential_mcp_urls (the variable) to: try to
parse/normalize it, ensure it contains a "github" entry or an entry whose
type/host == "github" (handle JSON parsing errors and missing keys), and only
append GIT_PUSH_MCP_STEPS.format(branch=push_branch) when that validation
succeeds; otherwise append GIT_PUSH_STEPS.format(branch=push_branch). Apply the
same validation/guarded decision to the other occurrence covering lines
~256-264.
| build-credential-sidecars: build-credential-github build-credential-jira build-credential-k8s build-credential-google ## Build all credential sidecar images | ||
|
|
||
| build-credential-github: ## Build GitHub credential sidecar image | ||
| @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Building GitHub credential sidecar with $(CONTAINER_ENGINE)..." | ||
| @$(CONTAINER_ENGINE) build $(PLATFORM_FLAG) $(BUILD_FLAGS) \ | ||
| -f components/credential-sidecars/github/Dockerfile \ | ||
| -t $(GITHUB_MCP_IMAGE) . | ||
| @echo "$(COLOR_GREEN)✓$(COLOR_RESET) GitHub credential sidecar built: $(GITHUB_MCP_IMAGE)" | ||
|
|
||
| build-credential-jira: ## Build Jira credential sidecar image | ||
| @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Building Jira credential sidecar with $(CONTAINER_ENGINE)..." | ||
| @cd components/credential-sidecars/jira && $(CONTAINER_ENGINE) build $(PLATFORM_FLAG) $(BUILD_FLAGS) \ | ||
| -t $(JIRA_MCP_IMAGE) . | ||
| @echo "$(COLOR_GREEN)✓$(COLOR_RESET) Jira credential sidecar built: $(JIRA_MCP_IMAGE)" | ||
|
|
||
| build-credential-k8s: ## Build K8s credential sidecar image | ||
| @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Building K8s credential sidecar with $(CONTAINER_ENGINE)..." | ||
| @cd components/credential-sidecars/k8s && $(CONTAINER_ENGINE) build $(PLATFORM_FLAG) $(BUILD_FLAGS) \ | ||
| -t $(K8S_MCP_IMAGE) . | ||
| @echo "$(COLOR_GREEN)✓$(COLOR_RESET) K8s credential sidecar built: $(K8S_MCP_IMAGE)" | ||
|
|
||
| build-credential-google: ## Build Google credential sidecar image | ||
| @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Building Google credential sidecar with $(CONTAINER_ENGINE)..." | ||
| @cd components/credential-sidecars/google && $(CONTAINER_ENGINE) build $(PLATFORM_FLAG) $(BUILD_FLAGS) \ | ||
| -t $(GOOGLE_MCP_IMAGE) . | ||
| @echo "$(COLOR_GREEN)✓$(COLOR_RESET) Google credential sidecar built: $(GOOGLE_MCP_IMAGE)" | ||
|
|
There was a problem hiding this comment.
Declare new credential-sidecar targets as .PHONY.
The targets added at Lines 228, 230, 237, 243, and 249 are missing from .PHONY (Lines 1-13). If same-named files appear, recipes may not run.
Suggested patch
-.PHONY: _create-operator-config _auto-port-forward _show-access-info _kind-load-images
+.PHONY: _create-operator-config _auto-port-forward _show-access-info _kind-load-images
+.PHONY: build-credential-sidecars build-credential-github build-credential-jira build-credential-k8s build-credential-google🧰 Tools
🪛 checkmake (0.3.2)
[warning] 228-228: Target "build-credential-sidecars" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 228 - 254, Add the new Makefile targets to the .PHONY
declaration so their recipes always run: include build-credential-sidecars,
build-credential-github, build-credential-jira, build-credential-k8s, and
build-credential-google in the existing .PHONY list (update the .PHONY line near
the top where other phony targets are declared) to prevent name collisions with
files of the same names.
Credential sidecar build targets are currently orphaned from primary workflows.
At Line 228 you add sidecar build targets, but build-all (Line 170), push-all (Line 299), and _kind-load-images (Line 1263) still omit these images. This means make kind-up LOCAL_IMAGES=true and make push-all can skip sidecar images even when sidecars are required.
Suggested patch
-build-all: build-frontend build-backend build-operator build-runner build-state-sync build-public-api build-api-server build-observability-dashboard ## Build all container images
+build-all: build-frontend build-backend build-operator build-runner build-state-sync build-public-api build-api-server build-observability-dashboard build-credential-sidecars ## Build all container images
@@
- `@for` image in $(FRONTEND_IMAGE) $(BACKEND_IMAGE) $(OPERATOR_IMAGE) $(RUNNER_IMAGE) $(STATE_SYNC_IMAGE) $(PUBLIC_API_IMAGE) $(API_SERVER_IMAGE) $(OBSERVABILITY_DASHBOARD_IMAGE); do \
+ `@for` image in $(FRONTEND_IMAGE) $(BACKEND_IMAGE) $(OPERATOR_IMAGE) $(RUNNER_IMAGE) $(STATE_SYNC_IMAGE) $(PUBLIC_API_IMAGE) $(API_SERVER_IMAGE) $(OBSERVABILITY_DASHBOARD_IMAGE) $(GITHUB_MCP_IMAGE) $(JIRA_MCP_IMAGE) $(K8S_MCP_IMAGE) $(GOOGLE_MCP_IMAGE); do \
@@
- `@for` img in $(BACKEND_IMAGE) $(FRONTEND_IMAGE) $(OPERATOR_IMAGE) $(RUNNER_IMAGE) $(STATE_SYNC_IMAGE) $(PUBLIC_API_IMAGE) $(API_SERVER_IMAGE) $(OBSERVABILITY_DASHBOARD_IMAGE); do \
+ `@for` img in $(BACKEND_IMAGE) $(FRONTEND_IMAGE) $(OPERATOR_IMAGE) $(RUNNER_IMAGE) $(STATE_SYNC_IMAGE) $(PUBLIC_API_IMAGE) $(API_SERVER_IMAGE) $(OBSERVABILITY_DASHBOARD_IMAGE) $(GITHUB_MCP_IMAGE) $(JIRA_MCP_IMAGE) $(K8S_MCP_IMAGE) $(GOOGLE_MCP_IMAGE); do \🧰 Tools
🪛 checkmake (0.3.2)
[warning] 228-228: Target "build-credential-sidecars" should be declared PHONY.
(phonydeclared)
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@Makefile` around lines 228 - 254, The new credential sidecar targets
(build-credential-github, build-credential-jira, build-credential-k8s,
build-credential-google) are not wired into the main flows; update the primary
meta targets build-all, push-all and the _kind-load-images target to include
these sidecar targets (or include a sidecar variable list) so that running make
build-all, make push-all, or make _kind-load-images (and make kind-up
LOCAL_IMAGES=true) will build/push/load the credential sidecars; reference the
targets by name (build-credential-github, build-credential-jira,
build-credential-k8s, build-credential-google) when adding them as dependencies
or appending them to the appropriate image lists used by build-all, push-all and
_kind-load-images.
Address PR review findings:
- Fail-closed entrypoint: exit(1) when bootstrap env vars missing instead of
starting MCP server unauthenticated
- Prevent credential response body leak in error logs (use len(body))
- Add multi-stage Dockerfile builds for jira/k8s/google sidecars (include
credential-entrypoint Go binary)
- Gate sidecar injection on CPTokenURL/CPTokenPublicKey configuration
- Fix credential token API URL: /api/ambient/v1/credentials/{id}/token
- Add CREDENTIAL_IDS and AGENTIC_SESSION_NAMESPACE env vars to sidecars
- Add input validation in runner mcp.py, auth.py, prompts.py
- Fix Makefile: .PHONY declarations, repo-root build context for sidecars
- Handle json.Marshal error in credential MCP URL serialization
Verified end-to-end on kind cluster:
- Sidecar injection (2 containers: runner + credential-github)
- Fail-closed behavior (exit 1 without env vars)
- Credential fetch via CP token exchange → API server → token set in env
- SecurityContext (drop ALL capabilities, no privilege escalation)
🤖 Generated with [Claude Code](https://claude.ai/code)
Co-Authored-By: Claude <noreply@anthropic.com>
1 participant
Summary
Implements credential sidecar isolation architecture across 7 waves:
CREDENTIAL_MCP_URLSenv varpush_files,create_pull_request) instead ofgit push/gh pr createwhen sidecars activebuildCredentialSidecars(6 test cases covering all edge cases)credential-entrypointGo binary for RSA-OAEP token exchange and credential fetchingIntegration tokens (GitHub, GitLab, Jira, kubeconfig, Google) are isolated in sidecar containers — the runner container has zero integration tokens in sidecar mode. LLM credentials remain in the runner. Full backward compatibility via fallback to subprocess mode.
Files changed
< /dev/null | Component | Files | Purpose |
|---|---|---|
| Control Plane |
config.go,kube_reconciler.go,main.go| Sidecar injection, config, wiring || Runner |
mcp.py,auth.py,prompts.py| SSE transport, token gating, MCP prompts || Sidecars |
credential-sidecars/{github,jira,k8s,google}/Dockerfile| Container images || Entrypoint |
credential-sidecars/entrypoint/main.go| Token exchange wrapper || Tests |
kube_reconciler_test.go| 6 unit tests || Build |
Makefile|build-credential-sidecarstarget |Test plan
go vet ./...cleango test ./...— all pass (6 new + 3 existing)pytest— 714 passed, 11 skippedpodman build+podman run)🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Documentation