fix(runner): restore operator-path credential fetch broken by alpha migration#1439
Merged
mergify[bot] merged 2 commits intomainfrom Apr 23, 2026
Merged
Conversation
✅ Deploy Preview for cheerful-kitten-f556a0 canceled.
|
Contributor
📝 WalkthroughWalkthroughCredential fetch/population now derives project from Changes
Sequence Diagram(s)sequenceDiagram
participant Runner as Ambient Runner
participant Backend as Control-plane / Operator API
participant Workspace as Session Workspace (env/files)
Runner->>Runner: determine project (PROJECT_NAME or AGENTIC_SESSION_NAMESPACE)
alt context.session_id present
Runner->>Backend: GET /projects/{project}/agentic-sessions/{session}/credentials/{credential_type}
else CREDENTIAL_IDS present
Runner->>Backend: GET /api/ambient/v1/.../credentials/{credential_id}
end
Backend-->>Runner: credential response ({token} or {accessToken, refreshToken, expiresAt,...} or {apiToken})
alt Google accessToken/refreshToken
Runner->>Workspace: write workspace-MCP JSON to _GOOGLE_WORKSPACE_CREDS_FILE (0600)
else Jira apiToken
Runner->>Workspace: set JIRA_API_TOKEN env
else legacy token
Runner->>Workspace: set provider-specific env or credentials file (existing behavior)
end
🚥 Pre-merge checks | ✅ 7 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (7 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
✨ Simplify code
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/runners/ambient-runner/ambient_runner/platform/auth.py (1)
432-433: Consider logging a warning if OAuth client env vars are missing.Empty string defaults for
GOOGLE_OAUTH_CLIENT_IDandGOOGLE_OAUTH_CLIENT_SECRETwill produce a credentials file with blank values. While these are validated asREQUIRED_ENV_VARSat Gemini CLI session startup (persession.py), a warning here would surface misconfigurations earlier.🔧 Optional defensive logging
+ client_id = os.getenv("GOOGLE_OAUTH_CLIENT_ID", "") + client_secret = os.getenv("GOOGLE_OAUTH_CLIENT_SECRET", "") + if not client_id or not client_secret: + logger.warning( + "GOOGLE_OAUTH_CLIENT_ID/SECRET not set; workspace-mcp may fail" + ) creds_data = { "token": google_creds.get("accessToken"), "refresh_token": google_creds.get("refreshToken", ""), "token_uri": "https://oauth2.googleapis.com/token", - "client_id": os.getenv("GOOGLE_OAUTH_CLIENT_ID", ""), - "client_secret": os.getenv("GOOGLE_OAUTH_CLIENT_SECRET", ""), + "client_id": client_id, + "client_secret": client_secret,🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/ambient_runner/platform/auth.py` around lines 432 - 433, Add a defensive warning when building the Google OAuth credentials dict if the environment variables GOOGLE_OAUTH_CLIENT_ID or GOOGLE_OAUTH_CLIENT_SECRET are empty: check os.getenv("GOOGLE_OAUTH_CLIENT_ID", "") and os.getenv("GOOGLE_OAUTH_CLIENT_SECRET", "") before creating the credentials dict (the block that sets "client_id" and "client_secret") and emit a warning via the module logger if either is blank so misconfiguration surfaces earlier (this complements the REQUIRED_ENV_VARS validation done at CLI startup).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/runners/ambient-runner/ambient_runner/platform/auth.py`:
- Around line 432-433: Add a defensive warning when building the Google OAuth
credentials dict if the environment variables GOOGLE_OAUTH_CLIENT_ID or
GOOGLE_OAUTH_CLIENT_SECRET are empty: check os.getenv("GOOGLE_OAUTH_CLIENT_ID",
"") and os.getenv("GOOGLE_OAUTH_CLIENT_SECRET", "") before creating the
credentials dict (the block that sets "client_id" and "client_secret") and emit
a warning via the module logger if either is blank so misconfiguration surfaces
earlier (this complements the REQUIRED_ENV_VARS validation done at CLI startup).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1296c77-864e-47de-be69-7d539ce93986
📒 Files selected for processing (1)
components/runners/ambient-runner/ambient_runner/platform/auth.py
markturansky
force-pushed
the
fix/runner-credential-fetch-dual-path
branch
from
ab8ef10 to
b757b52
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
components/runners/ambient-runner/tests/test_shared_session_credentials.py (2)
561-564: Avoid fixedhandle_request()loop counts to reduce test flakinessUsing
server.handle_request()with a hard-coded count (7) makes these tests fragile if provider fetch count changes. Preferserve_forever()and explicitshutdown()infinallyso tests are deterministic and don’t rely on request cardinality.Suggested refactor
- thread = Thread( - target=lambda: [server.handle_request() for _ in range(7)], - daemon=True, - ) + thread = Thread(target=server.serve_forever, daemon=True) thread.start() @@ finally: + server.shutdown() server.server_close() thread.join(timeout=2)Also applies to: 630-633, 700-702
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py` around lines 561 - 564, The test uses a Thread that calls server.handle_request() in a fixed-count loop (target lambda with range(7)), which is flaky; change each such Thread to call server.serve_forever() instead and ensure the test invokes server.shutdown() in a finally block (or teardown) so the server stops deterministically; update the Thread creation that references server (and any other similar Thread targets in this file) to use daemon=True with target=server.serve_forever and add a try/finally around the request-driving code to call server.shutdown().
664-668: Add permission assertion for Google credentials fileThis test verifies content but not secure mode. Add an assertion for
0600to prevent regressions that could expose OAuth tokens via world/group-readable file permissions.Suggested assertion
assert tmp_creds.exists() written = json.loads(tmp_creds.read_text()) assert written["token"] == "ya29.access" assert written["refresh_token"] == "1//refresh" + assert (tmp_creds.stat().st_mode & 0o777) == 0o600🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py` around lines 664 - 668, The test currently checks the file exists and its JSON contents (using tmp_creds and written), but doesn't verify file permissions; update the test (the block using tmp_creds.exists() / tmp_creds.read_text()) to assert the credential file is created with mode 0o600 by checking tmp_creds.stat().st_mode & 0o777 == 0o600 so the file is not world/group-readable.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py`:
- Around line 561-564: The test uses a Thread that calls server.handle_request()
in a fixed-count loop (target lambda with range(7)), which is flaky; change each
such Thread to call server.serve_forever() instead and ensure the test invokes
server.shutdown() in a finally block (or teardown) so the server stops
deterministically; update the Thread creation that references server (and any
other similar Thread targets in this file) to use daemon=True with
target=server.serve_forever and add a try/finally around the request-driving
code to call server.shutdown().
- Around line 664-668: The test currently checks the file exists and its JSON
contents (using tmp_creds and written), but doesn't verify file permissions;
update the test (the block using tmp_creds.exists() / tmp_creds.read_text()) to
assert the credential file is created with mode 0o600 by checking
tmp_creds.stat().st_mode & 0o777 == 0o600 so the file is not
world/group-readable.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 9117c8c1-0dbc-4d3d-aa75-54cce61d5626
📒 Files selected for processing (2)
components/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/tests/test_shared_session_credentials.py
🚧 Files skipped from review as they are similar to previous changes (1)
- components/runners/ambient-runner/ambient_runner/platform/auth.py
…igration The alpha migration (1aa8b42) rewrote _fetch_credential() to require CREDENTIAL_IDS env var and use the control-plane API path. The operator does not inject CREDENTIAL_IDS, so all credential fetches silently returned empty — breaking GitHub, Jira, Google, and GitLab auth in operator-based deployments. Restore the session-scoped backend URL as fallback when CREDENTIAL_IDS is absent, and accept both old (accessToken/apiToken) and new (token) response field formats. Fixes #1438 🤖 Generated with [Claude Code](https://claude.ai/code) Co-Authored-By: Claude <noreply@anthropic.com>
markturansky
force-pushed
the
fix/runner-credential-fetch-dual-path
branch
from
643f567 to
d75ea24
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
components/runners/ambient-runner/tests/test_shared_session_credentials.py (1)
561-564: Make test HTTP server lifecycle deterministic (avoid fixedhandle_request()loops).Using a fixed count of
handle_request()calls can leave daemon threads blocked when request counts change, which makes this suite brittle. Preferserve_forever()+ explicitshutdown()infinally.Suggested refactor pattern
- thread = Thread( - target=lambda: [server.handle_request() for _ in range(7)], - daemon=True, - ) + thread = Thread(target=server.serve_forever, daemon=True) thread.start() try: ... finally: + server.shutdown() server.server_close() thread.join(timeout=2)As per coding guidelines, "components/runners/ambient-runner/**/*.py: Check subprocess handling, timeout management, and that secrets are not logged."
Also applies to: 630-633, 699-702
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py` around lines 561 - 564, The test starts a daemon Thread that runs a fixed loop calling server.handle_request(), which can hang or be misaligned with actual request counts; replace this pattern by starting the server with server.serve_forever() on a non-daemon Thread (Thread(target=server.serve_forever)) and ensure you call server.shutdown() and thread.join() in a finally block to deterministically stop the server; update the instances around the Thread creation and the server.handle_request() loops (including locations referenced by the tests) to use serve_forever()/shutdown() lifecycle management and ensure the thread is joined to avoid leaked daemon threads.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@components/runners/ambient-runner/tests/test_shared_session_credentials.py`:
- Around line 561-564: The test starts a daemon Thread that runs a fixed loop
calling server.handle_request(), which can hang or be misaligned with actual
request counts; replace this pattern by starting the server with
server.serve_forever() on a non-daemon Thread
(Thread(target=server.serve_forever)) and ensure you call server.shutdown() and
thread.join() in a finally block to deterministically stop the server; update
the instances around the Thread creation and the server.handle_request() loops
(including locations referenced by the tests) to use serve_forever()/shutdown()
lifecycle management and ensure the thread is joined to avoid leaked daemon
threads.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 199a485f-2fb6-44f9-bcf4-d84040efbf73
📒 Files selected for processing (3)
components/runners/ambient-runner/ambient_runner/platform/auth.pycomponents/runners/ambient-runner/tests/test_shared_session_credentials.pye2e/cypress/e2e/screenshots.cy.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- components/runners/ambient-runner/ambient_runner/platform/auth.py
Contributor
Merge Queue Status
This pull request spent 12 seconds in the queue, including 1 second running CI. Required conditions to merge |
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
1aa8b428) rewrote_fetch_credential()to requireCREDENTIAL_IDSenv var and use the control-plane API path (/api/ambient/v1/projects/{id}/credentials/{id}/token). The operator does not injectCREDENTIAL_IDS, so all credential fetches silently returned empty — breaking GitHub, Jira, Google, and GitLab auth in operator-based deployments./projects/{project}/agentic-sessions/{session}/credentials/{type}) as fallback whenCREDENTIAL_IDSis absent. WhenCREDENTIAL_IDSis present (control-plane deployments), the new CP path is used.accessToken/apiToken) and new (token) response field formats for Google and Jira credentials, since the backend still returns the old format.Fixes #1438
Test plan
ruff checkpassesruff format --checkpassespython -m pytest tests/— 713 passed, 11 skippedGITHUB_TOKENis available in runner sessiongh auth statusworks in a session🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests