fix(runner,backend,control-plane): workflow dir creation + jira-write flag propagation#1702
Open
jwm4 wants to merge 1 commit into
Open
fix(runner,backend,control-plane): workflow dir creation + jira-write flag propagation#1702jwm4 wants to merge 1 commit into
jwm4 wants to merge 1 commit into
Conversation
…flag propagation Three bugs fixed across runner, backend, and ambient-control-plane. ## fix(runner): create /workspace/workflows before shutil.move (#1493) clone_workflow_at_runtime in workflow.py called shutil.move(temp_dir, workflow_final) without first creating workflow_final.parent (/workspace/workflows). If no prior workflow had been cloned the directory did not exist, causing a FileNotFoundError that was silently swallowed by the outer try/except. The workflow then appeared to load but nothing was placed in the session context. The same missing mkdir existed for the subpath-not-found fallback path. Fixes both paths: - no-subpath (primary): add workflow_final.parent.mkdir(parents=True, exist_ok=True) - subpath-not-found fallback: same fix Added tests in tests/test_workflow_endpoint.py covering: - no-subpath path creates parent directory when absent - no-subpath path works when parent already exists - subpath-not-found fallback creates parent directory - git clone failure returns (False, '') without raising ## fix(runner): create /workspace/workflows in hydrate.sh (#1493) Identical gap in the init-container shell script. The subpath case already ran mkdir -p "$(dirname "$WORKFLOW_FINAL")" but the no-subpath path and the subpath-not-found fallback path both called mv without creating the parent first, silently failing under set +e. Added mkdir -p before mv in both shell paths. ## fix(backend): use service-account client for workspace feature-flag ConfigMap (#1506) getWorkspaceOverrides was called with reqK8s (the user's bearer token) rather than K8sClient (the backend service account). End-users may not have configmaps.get permission in the project namespace, causing the RBAC check to fail intermittently. When it failed the code fell back to the global Unleash flag; if jira-write was only workspace-enabled (not globally enabled) the fallback returned false and JIRA_READ_ONLY_MODE was never set to "false", so mcp-atlassian started in read-only mode and write tools (jira_add_comment, etc.) were not registered. Switch to K8sClient so the ConfigMap read always uses a principal that has the necessary permissions. ## fix(control-plane): propagate JIRA_READ_ONLY_MODE to Jira credential sidecar (#1506) In the ambient-control-plane path, credential sidecars are separate containers that each run the MCP server binary (e.g. uvx mcp-atlassian). JIRA_READ_ONLY_MODE was set on the runner container env but the Jira sidecar container env was built independently and never received this variable, so mcp-atlassian always started in read-only mode regardless of the jira-write flag. Changes: - Add parseSessionEnvVars() helper to deserialise the JSON-encoded EnvironmentVariables field from the Session SDK type. - Pass sessionEnvVars to buildCredentialSidecars(). - In buildCredentialSidecars, inject JIRA_READ_ONLY_MODE into the Jira sidecar env when present in the session env — and only for the Jira sidecar (GitHub/K8s/Google sidecars are unaffected). - Update all existing test callsites to pass the new parameter. - Add new tests: JIRA_READ_ONLY_MODE set to false, not set by default, not propagated to GitHub sidecar, and parseSessionEnvVars unit tests.
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughThree independent bug fixes: the Go reconciler now parses session environment variables and propagates ChangesJira JIRA_READ_ONLY_MODE Propagation to Credential Sidecar
Backend K8s Client Fix for Workspace Overrides
Workflow Clone Parent Directory Creation Fix
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 docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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_test.go`:
- Around line 201-334: The test suite lacks a regression test for a critical
security boundary: when jira-write is disabled at the workspace level, a user
should not be able to bypass this by providing JIRA_READ_ONLY_MODE=false in the
sessionEnv. Add a new test function following the pattern of
TestBuildCredentialSidecars_JiraReadOnlyMode_SetToFalse that creates a
SimpleKubeReconciler without jira-write capability enabled, passes a sessionEnv
containing JIRA_READ_ONLY_MODE=false as untrusted user input, calls
buildCredentialSidecars, and asserts that JIRA_READ_ONLY_MODE is not injected
into the Jira sidecar (or remains in the safe read-only state), thereby
validating that workspace-level authorization controls cannot be overridden by
user-provided environment variables.
In `@components/ambient-control-plane/internal/reconciler/kube_reconciler.go`:
- Around line 1205-1207: The parseSessionEnvVars function silently returns an
empty map when JSON unmarshaling fails, which suppresses error details and makes
debugging difficult. Modify parseSessionEnvVars to return both the output map
and an error value. Update the unmarshal error handling to return the error
instead of silently returning an empty map. Then in the ensurePod method (which
calls parseSessionEnvVars), capture the returned error and add contextual
logging to make the failure explicit, ensuring errors are never silently
discarded. Apply this same fix to any other similar error handling patterns in
the file where environment variable parsing might fail.
- Around line 1265-1268: The code at the jira provider check block forwards
JIRA_READ_ONLY_MODE directly from sessionEnvVars which contains untrusted
user-provided environment variables. This allows users to bypass authorization
by setting JIRA_READ_ONLY_MODE=false to enable Jira write tools without
workspace permission. Remove the direct forwarding of JIRA_READ_ONLY_MODE from
sessionEnvVars in the if provider == "jira" block and instead set it exclusively
based on trusted server-side configuration such as workspace settings or
authorization checks. Only propagate JIRA_READ_ONLY_MODE to the environment if
it comes from an authorized server-controlled source, not from user input.
In `@components/runners/ambient-runner/tests/test_workflow_endpoint.py`:
- Line 98: In the async tests, the variable `path` is unpacked from the return
value of `clone_workflow_at_runtime()` at three locations (lines 98, 152, and
193) but is never used thereafter. Rename the unused `path` variable to `_path`
(or `_`) in all three occurrences where `clone_workflow_at_runtime()` is called
to follow Python conventions for unused variables and satisfy Ruff linting
requirements.
🪄 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: a0a6896d-7a62-488f-9ad5-20e52b3dba9c
📒 Files selected for processing (6)
components/ambient-control-plane/internal/reconciler/kube_reconciler.gocomponents/ambient-control-plane/internal/reconciler/kube_reconciler_test.gocomponents/backend/handlers/sessions.gocomponents/runners/ambient-runner/ambient_runner/endpoints/workflow.pycomponents/runners/ambient-runner/tests/test_workflow_endpoint.pycomponents/runners/state-sync/hydrate.sh
Comment on lines
+201
to
+334
| func TestBuildCredentialSidecars_JiraReadOnlyMode_SetToFalse(t *testing.T) { | ||
| // When jira-write is enabled, JIRA_READ_ONLY_MODE=false must be passed to | ||
| // the Jira sidecar so mcp-atlassian exposes write tools. | ||
| r := &SimpleKubeReconciler{ | ||
| cfg: KubeReconcilerConfig{ | ||
| JiraMCPImage: "jira-mcp:latest", | ||
| MCPAPIServerURL: "http://api.svc:8000", | ||
| CPTokenURL: "http://cp.svc:8080", | ||
| CPTokenPublicKey: "test-key", | ||
| }, | ||
| } | ||
| r.logger = r.logger.With().Logger() | ||
|
|
||
| credentialIDs := map[string]string{"jira": "cred-42"} | ||
| sessionEnv := map[string]string{"JIRA_READ_ONLY_MODE": "false"} | ||
|
|
||
| sidecars, _, _ := r.buildCredentialSidecars("test-session", "test-ns", credentialIDs, sessionEnv) | ||
|
|
||
| if len(sidecars) != 1 { | ||
| t.Fatalf("expected 1 sidecar, got %d", len(sidecars)) | ||
| } | ||
|
|
||
| sidecar := sidecars[0].(map[string]interface{}) | ||
| env := sidecar["env"].([]interface{}) | ||
|
|
||
| val, found := findEnvVar(env, "JIRA_READ_ONLY_MODE") | ||
| if !found { | ||
| t.Fatal("JIRA_READ_ONLY_MODE not found in Jira sidecar env") | ||
| } | ||
| if val != "false" { | ||
| t.Errorf("expected JIRA_READ_ONLY_MODE=false, got %q", val) | ||
| } | ||
| } | ||
|
|
||
| func TestBuildCredentialSidecars_JiraReadOnlyMode_NotSetByDefault(t *testing.T) { | ||
| // When jira-write is not enabled, JIRA_READ_ONLY_MODE must NOT be injected | ||
| // (mcp-atlassian defaults to read-only, which is the safe default). | ||
| r := &SimpleKubeReconciler{ | ||
| cfg: KubeReconcilerConfig{ | ||
| JiraMCPImage: "jira-mcp:latest", | ||
| MCPAPIServerURL: "http://api.svc:8000", | ||
| CPTokenURL: "http://cp.svc:8080", | ||
| CPTokenPublicKey: "test-key", | ||
| }, | ||
| } | ||
| r.logger = r.logger.With().Logger() | ||
|
|
||
| credentialIDs := map[string]string{"jira": "cred-42"} | ||
|
|
||
| sidecars, _, _ := r.buildCredentialSidecars("test-session", "test-ns", credentialIDs, noSessionEnv) | ||
|
|
||
| if len(sidecars) != 1 { | ||
| t.Fatalf("expected 1 sidecar, got %d", len(sidecars)) | ||
| } | ||
|
|
||
| sidecar := sidecars[0].(map[string]interface{}) | ||
| env := sidecar["env"].([]interface{}) | ||
|
|
||
| _, found := findEnvVar(env, "JIRA_READ_ONLY_MODE") | ||
| if found { | ||
| t.Error("JIRA_READ_ONLY_MODE should not be present when jira-write is disabled") | ||
| } | ||
| } | ||
|
|
||
| func TestBuildCredentialSidecars_JiraReadOnly_NotPropagatedToGitHub(t *testing.T) { | ||
| // JIRA_READ_ONLY_MODE must only be injected into the Jira sidecar, never | ||
| // into other provider sidecars (e.g. GitHub). | ||
| r := &SimpleKubeReconciler{ | ||
| cfg: KubeReconcilerConfig{ | ||
| GitHubMCPImage: "github-mcp:latest", | ||
| JiraMCPImage: "jira-mcp:latest", | ||
| MCPAPIServerURL: "http://api.svc:8000", | ||
| CPTokenURL: "http://cp.svc:8080", | ||
| CPTokenPublicKey: "test-key", | ||
| }, | ||
| } | ||
| r.logger = r.logger.With().Logger() | ||
|
|
||
| credentialIDs := map[string]string{"github": "cred-1", "jira": "cred-2"} | ||
| sessionEnv := map[string]string{"JIRA_READ_ONLY_MODE": "false"} | ||
|
|
||
| sidecars, _, _ := r.buildCredentialSidecars("test-session", "test-ns", credentialIDs, sessionEnv) | ||
|
|
||
| if len(sidecars) != 2 { | ||
| t.Fatalf("expected 2 sidecars, got %d", len(sidecars)) | ||
| } | ||
|
|
||
| for _, item := range sidecars { | ||
| sidecar := item.(map[string]interface{}) | ||
| name := sidecar["name"].(string) | ||
| if name == "credential-github" { | ||
| env := sidecar["env"].([]interface{}) | ||
| if _, found := findEnvVar(env, "JIRA_READ_ONLY_MODE"); found { | ||
| t.Errorf("JIRA_READ_ONLY_MODE must not be injected into GitHub sidecar") | ||
| } | ||
| } | ||
| } | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // Tests for parseSessionEnvVars | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| func TestParseSessionEnvVars_ValidJSON(t *testing.T) { | ||
| raw := `{"JIRA_READ_ONLY_MODE":"false","FOO":"bar"}` | ||
| got := parseSessionEnvVars(raw) | ||
| if got["JIRA_READ_ONLY_MODE"] != "false" { | ||
| t.Errorf("expected false, got %q", got["JIRA_READ_ONLY_MODE"]) | ||
| } | ||
| if got["FOO"] != "bar" { | ||
| t.Errorf("expected bar, got %q", got["FOO"]) | ||
| } | ||
| } | ||
|
|
||
| func TestParseSessionEnvVars_Empty(t *testing.T) { | ||
| got := parseSessionEnvVars("") | ||
| if len(got) != 0 { | ||
| t.Errorf("expected empty map, got %v", got) | ||
| } | ||
| } | ||
|
|
||
| func TestParseSessionEnvVars_InvalidJSON(t *testing.T) { | ||
| got := parseSessionEnvVars("not-json") | ||
| if len(got) != 0 { | ||
| t.Errorf("expected empty map on parse error, got %v", got) | ||
| } | ||
| } | ||
|
|
||
| func TestParseSessionEnvVars_EmptyObject(t *testing.T) { | ||
| got := parseSessionEnvVars("{}") | ||
| if len(got) != 0 { | ||
| t.Errorf("expected empty map, got %v", got) | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
Missing regression test for unauthorized Jira write-mode enablement
Current tests validate positive propagation behavior, but they don’t protect against the critical case where JIRA_READ_ONLY_MODE=false comes from user-provided session env while workspace jira-write is disabled. Add a negative test that asserts write mode is not enabled from untrusted env input.
🤖 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_test.go`
around lines 201 - 334, The test suite lacks a regression test for a critical
security boundary: when jira-write is disabled at the workspace level, a user
should not be able to bypass this by providing JIRA_READ_ONLY_MODE=false in the
sessionEnv. Add a new test function following the pattern of
TestBuildCredentialSidecars_JiraReadOnlyMode_SetToFalse that creates a
SimpleKubeReconciler without jira-write capability enabled, passes a sessionEnv
containing JIRA_READ_ONLY_MODE=false as untrusted user input, calls
buildCredentialSidecars, and asserts that JIRA_READ_ONLY_MODE is not injected
into the Jira sidecar (or remains in the safe read-only state), thereby
validating that workspace-level authorization controls cannot be overridden by
user-provided environment variables.
Comment on lines
+1205
to
+1207
| if err := json.Unmarshal([]byte(raw), &out); err != nil { | ||
| return map[string]string{} | ||
| } |
Contributor
There was a problem hiding this comment.
Don’t silently drop malformed session env JSON
parseSessionEnvVars returns {} on unmarshal failure without logging or error propagation. That silently disables sidecar env propagation (including JIRA_READ_ONLY_MODE) and makes failures hard to diagnose.
Please return an error from parseSessionEnvVars and handle it in ensurePod with contextual logging (without logging env values), so the failure path is explicit.
As per coding guidelines, "Never silently swallow partial failures; every error path must propagate or be explicitly collected, never discarded".
Also applies to: 587-588
🤖 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 1205 - 1207, The parseSessionEnvVars function silently returns an
empty map when JSON unmarshaling fails, which suppresses error details and makes
debugging difficult. Modify parseSessionEnvVars to return both the output map
and an error value. Update the unmarshal error handling to return the error
instead of silently returning an empty map. Then in the ensurePod method (which
calls parseSessionEnvVars), capture the returned error and add contextual
logging to make the failure explicit, ensuring errors are never silently
discarded. Apply this same fix to any other similar error handling patterns in
the file where environment variable parsing might fail.
Source: Coding guidelines
Comment on lines
+1265
to
+1268
| if provider == "jira" { | ||
| if v, ok := sessionEnvVars["JIRA_READ_ONLY_MODE"]; ok { | ||
| env = append(env, envVar("JIRA_READ_ONLY_MODE", v)) | ||
| } |
Contributor
There was a problem hiding this comment.
Untrusted session env can bypass Jira write-mode authorization
JIRA_READ_ONLY_MODE is forwarded directly from sessionEnvVars, but upstream session env includes user-provided req.EnvironmentVariables. A user can set JIRA_READ_ONLY_MODE=false even when jira-write is disabled, which enables Jira write tools in the sidecar without workspace authorization.
This key must be server-controlled only (e.g., strip/ignore user-provided JIRA_READ_ONLY_MODE in backend and set it exclusively from workspace override, or gate propagation on a trusted server-side marker).
🤖 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 1265 - 1268, The code at the jira provider check block forwards
JIRA_READ_ONLY_MODE directly from sessionEnvVars which contains untrusted
user-provided environment variables. This allows users to bypass authorization
by setting JIRA_READ_ONLY_MODE=false to enable Jira write tools without
workspace permission. Remove the direct forwarding of JIRA_READ_ONLY_MODE from
sessionEnvVars in the if provider == "jira" block and instead set it exclusively
based on trusted server-side configuration such as workspace settings or
authorization checks. Only propagate JIRA_READ_ONLY_MODE to the environment if
it comes from an authorized server-controlled source, not from user input.
| ), patch( | ||
| "os.getenv", side_effect=lambda k, d="": str(workspace) if k == "WORKSPACE_PATH" else d | ||
| ): | ||
| success, path = await clone_workflow_at_runtime(git_url, "main", "") |
Contributor
There was a problem hiding this comment.
Remove unused path bindings in async tests.
At Line 98, Line 152, and Line 193, path is unpacked but unused. Rename to _path (or _) to keep Ruff clean and avoid CI noise.
Suggested patch
- success, path = await clone_workflow_at_runtime(git_url, "main", "")
+ success, _path = await clone_workflow_at_runtime(git_url, "main", "")
@@
- success, path = await clone_workflow_at_runtime(
+ success, _path = await clone_workflow_at_runtime(
git_url, "main", "does-not-exist"
)
@@
- success, path = await clone_workflow_at_runtime(
+ success, _path = await clone_workflow_at_runtime(
git_url, "main", "workflows/my-workflow"
)Also applies to: 152-152, 193-193
🧰 Tools
🪛 Ruff (0.15.17)
[warning] 98-98: Unpacked variable path is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
🤖 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/tests/test_workflow_endpoint.py` at line
98, In the async tests, the variable `path` is unpacked from the return value of
`clone_workflow_at_runtime()` at three locations (lines 98, 152, and 193) but is
never used thereafter. Rename the unused `path` variable to `_path` (or `_`) in
all three occurrences where `clone_workflow_at_runtime()` is called to follow
Python conventions for unused variables and satisfy Ruff linting requirements.
Source: Linters/SAST tools
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
Three related bugs fixed across the runner, backend, and ambient-control-plane.
Bug 1 — Custom workflow fails to load when
/workspace/workflowsdoesn't exist (fixes #1493)Root cause —
workflow.pyclone_workflow_at_runtimecalledshutil.move(temp_dir, workflow_final)without first creatingworkflow_final.parent(/workspace/workflows). If no prior workflow had been cloned in this session the directory did not exist, causing aFileNotFoundErrorsilently swallowed by the outertry/except. The workflow appeared to load but nothing was placed in the session context.The same missing
mkdirwas present in the subpath-not-found fallback path.Fix: add
workflow_final.parent.mkdir(parents=True, exist_ok=True)before bothshutil.movecallsites.Root cause —
hydrate.sh(init-container)Identical gap in the shell script. The subpath-found path already ran
mkdir -p "$(dirname "$WORKFLOW_FINAL")"but the no-subpath path and the subpath-not-found fallback both calledmvwithout creating the parent first, silently failing underset +e.Fix: add
mkdir -pbeforemvin both shell paths.Bug 2 — Intermittent jira-write failure in backend/operator path (fixes #1506)
Root cause
getWorkspaceOverrideswas called withreqK8s(the user's bearer token) rather thanK8sClient(the backend service account). End-users may not haveconfigmaps.getpermission in the project namespace, causing the lookup to fail intermittently. When it failed the code fell back to the global Unleash flag; ifjira-writewas only workspace-enabled (not globally enabled in Unleash) the fallback returnedfalseandJIRA_READ_ONLY_MODEwas never set to"false", somcp-atlassianstarted in read-only mode and write tools (jira_add_comment, etc.) were absent.Fix: switch to
K8sClient(backend SA) for the ConfigMap read — the SA has the necessary permissions and this is a server-side concern.Bug 3 —
JIRA_READ_ONLY_MODEnever reached Jira credential sidecar in control-plane path (fixes #1506)Root cause
In the ambient-control-plane deployment, Jira credentials are served by a dedicated sidecar container that runs
uvx mcp-atlassian.JIRA_READ_ONLY_MODEwas injected into the runner container's env butbuildCredentialSidecarsbuilt each sidecar's env independently and never received this variable. As a resultmcp-atlassianalways started in read-only mode regardless of thejira-writeflag.Fix:
parseSessionEnvVars()to deserialise the JSON-encodedEnvironmentVariablesfield from the Session SDK type.sessionEnvVarsthrough tobuildCredentialSidecars.JIRA_READ_ONLY_MODEinto the Jira sidecar env when present — and only for the Jira sidecar (GitHub/K8s/Google sidecars are unaffected).Files changed
components/runners/ambient-runner/ambient_runner/endpoints/workflow.pymkdirbeforeshutil.move(2 callsites)components/runners/state-sync/hydrate.shmkdir -pbeforemv(2 callsites)components/backend/handlers/sessions.goK8sClientinstead ofreqK8sfor ConfigMap readcomponents/ambient-control-plane/internal/reconciler/kube_reconciler.goparseSessionEnvVars, propagateJIRA_READ_ONLY_MODEto Jira sidecarcomponents/ambient-control-plane/internal/reconciler/kube_reconciler_test.gocomponents/runners/ambient-runner/tests/test_workflow_endpoint.pyclone_workflow_at_runtimeTesting
cd components/ambient-control-plane && go test ./internal/reconciler/...cd components/runners/ambient-runner && python -m pytest tests/test_workflow_endpoint.py -vSummary by CodeRabbit
New Features
Bug Fixes
Tests