feat(temporal): enable TLS for all Temporal Cloud connections#12
Merged
feat(temporal): enable TLS for all Temporal Cloud connections#12
Conversation
Temporal Cloud requires TLS on all connections. Previously, TLS was only configured when mTLS client certificates were provided, causing API key auth without certs to fail. Now TLS is enabled automatically for any Temporal Cloud connection (detected via is_cloud), regardless of the authentication method.
📝 WalkthroughWalkthroughEnables TLS by default for Temporal Cloud when unset, adds API-key authentication by injecting a Bearer token into rpc_metadata and propagates cloud_namespace as temporal-namespace; preserves existing client-cert and non-cloud behaviors. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches
🧪 Generate unit tests (beta)
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}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
olive/temporal/worker.py (1)
74-82: Use the SDK's built-inapi_keyparameter for Temporal Cloud authentication.The temporalio Python SDK provides a dedicated
api_keyparameter onClient.connect()that automatically sets theAuthorization: Bearerheader. This is the officially documented pattern for Temporal Cloud and eliminates the need for manualrpc_metadatapopulation.♻️ Suggested refactor
# Temporal Cloud API key auth if temporal_config.is_cloud and temporal_config.cloud_api_key: - connect_kwargs.setdefault("rpc_metadata", {}) - connect_kwargs["rpc_metadata"]["authorization"] = f"Bearer {temporal_config.cloud_api_key}" + connect_kwargs["api_key"] = temporal_config.cloud_api_key if temporal_config.cloud_namespace: connect_kwargs.setdefault("rpc_metadata", {}) - connect_kwargs["rpc_metadata"].setdefault("temporal-namespace", temporal_config.cloud_namespace) + connect_kwargs["rpc_metadata"]["temporal-namespace"] = temporal_config.cloud_namespace elif temporal_config.namespace_endpoint:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@olive/temporal/worker.py` around lines 74 - 82, Replace the manual rpc_metadata auth with the SDK api_key parameter: when using Temporal Cloud (temporal_config.is_cloud and temporal_config.cloud_api_key) pass temporal_config.cloud_api_key to Client.connect via the api_key argument instead of setting connect_kwargs["rpc_metadata"]["authorization"]; likewise pass the namespace via the Client.connect namespace argument (temporal_config.cloud_namespace) rather than setting "temporal-namespace" in rpc_metadata. For the non-cloud path that uses temporal_config.namespace_endpoint/namespace, keep setting connect_kwargs but ensure you populate the Client.connect namespace argument from temporal_config.namespace instead of manually injecting "temporal-namespace" into rpc_metadata. Use the existing temporal_config and connect_kwargs symbols and update the code paths around Client.connect accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@olive/temporal/worker.py`:
- Around line 74-82: Replace the manual rpc_metadata auth with the SDK api_key
parameter: when using Temporal Cloud (temporal_config.is_cloud and
temporal_config.cloud_api_key) pass temporal_config.cloud_api_key to
Client.connect via the api_key argument instead of setting
connect_kwargs["rpc_metadata"]["authorization"]; likewise pass the namespace via
the Client.connect namespace argument (temporal_config.cloud_namespace) rather
than setting "temporal-namespace" in rpc_metadata. For the non-cloud path that
uses temporal_config.namespace_endpoint/namespace, keep setting connect_kwargs
but ensure you populate the Client.connect namespace argument from
temporal_config.namespace instead of manually injecting "temporal-namespace"
into rpc_metadata. Use the existing temporal_config and connect_kwargs symbols
and update the code paths around Client.connect accordingly.
TAOGenna
previously approved these changes
Feb 17, 2026
Collaborator
TAOGenna
left a comment
TAOGenna
left a comment
There was a problem hiding this comment.
solves potential security issue.
LGTM
- Add timeout-minutes: 10 to prevent 6-hour zombie runs - Disable anyio pytest plugin (-p no:anyio) which conflicts with pytest-asyncio auto mode and causes deadlocks on CI runners - Remove unused --cov-report=html (only XML needed for Codecov) Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In @.github/workflows/tests.yml:
- Line 12: The job-level timeout (timeout-minutes: 10) is killing whole workflow
runs on cold caches; either increase the job timeout to a safer value (e.g.,
timeout-minutes: 15) or remove the job-level timeout and instead add a
step-level timeout only around the test step (reference the timeout-minutes key
and the test step name used in the workflow) so setup/install/lint/typecheck can
complete while still bounding the test execution time; update the
.github/workflows/tests.yml accordingly.
- Disable langsmith pytest plugin that may phone home and block - Set LANGSMITH_TRACING=false to prevent any tracing - Add -v for verbose output to see exactly which test hangs Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
The test_main_module test spawns a subprocess that imports temporalio, which loads gRPC. On Linux CI runners, gRPC starts non-daemon threads that prevent the subprocess from exiting, causing subprocess.run to block forever. Add timeout=10 to prevent the hang. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
CLI tests calling dev() and serve() with config_file=None loaded .olive.yaml from the repo root, which has all-default temporal config (temporal.enabled=False). This caused tests to skip the temporal check block and reach an unmocked uvicorn.run(), starting a real server that blocks forever on Linux CI runners. Fix: mock OliveConfig.from_file to return a config with temporal explicitly enabled, so the tests exercise the intended code paths. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
🧹 Nitpick comments (2)
tests/test_cli.py (2)
86-91: MoveTemporalConfigimport to the module-level imports.
TemporalConfigis imported lazily inside_temporal_enabled_configwhileOliveConfigis already imported at the module level (line 22). There's no circular-import or conditional-availability reason to defer it here.♻️ Proposed refactor
-from olive.config import OliveConfig +from olive.config import OliveConfig, TemporalConfigdef _temporal_enabled_config(**overrides): """Return an OliveConfig with temporal explicitly enabled.""" - from olive.config import TemporalConfig - temporal_kwargs = {"enabled": True, **overrides} return OliveConfig(temporal=TemporalConfig(**temporal_kwargs))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 86 - 91, Move the TemporalConfig import out of the function and into the module-level imports alongside OliveConfig: remove the lazy import inside _temporal_enabled_config and add a top-level "from ... import TemporalConfig" so _temporal_enabled_config simply constructs TemporalConfig(**temporal_kwargs) and returns OliveConfig(temporal=...), keeping the function signature and behavior unchanged.
290-322:test_main_py_detectionis the onlydev()-exercising test that does not mockOliveConfig.from_file.All other updated
devtests now mockOliveConfig.from_filefor isolation. This test callsdev(config_file=None), which triggers a real.olive.yamlfilesystem probe in the process's working directory. If the project root has such a file (or if CI is run from a directory that does), the loaded config could differ from the expected default (e.g.,temporal.enabled=True), making the test environment-sensitive.♻️ Proposed fix — add the same mock for consistency
def test_main_py_detection(): """Test that CLI detects main.py vs module import.""" with ( mock.patch("pathlib.Path.exists") as mock_exists, + mock.patch("olive.cli.OliveConfig.from_file", return_value=_temporal_enabled_config()), mock.patch("olive.cli.check_temporal_running", return_value=True), mock.patch("olive.cli.TemporalWorker"), mock.patch("uvicorn.run") as mock_uvicorn, ):🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 290 - 322, test_main_py_detection calls dev(config_file=None) and leaks a real .olive.yaml probe because OliveConfig.from_file is not mocked; wrap the test's with(...) context to also mock OliveConfig.from_file (the same symbol used in other dev tests) and have it return a deterministic default OliveConfig (e.g., an instance representing default settings with temporal.disabled or the same default used by other tests) so the dev(host=..., config_file=None) invocation is isolated and deterministic.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_cli.py`:
- Around line 86-91: Move the TemporalConfig import out of the function and into
the module-level imports alongside OliveConfig: remove the lazy import inside
_temporal_enabled_config and add a top-level "from ... import TemporalConfig" so
_temporal_enabled_config simply constructs TemporalConfig(**temporal_kwargs) and
returns OliveConfig(temporal=...), keeping the function signature and behavior
unchanged.
- Around line 290-322: test_main_py_detection calls dev(config_file=None) and
leaks a real .olive.yaml probe because OliveConfig.from_file is not mocked; wrap
the test's with(...) context to also mock OliveConfig.from_file (the same symbol
used in other dev tests) and have it return a deterministic default OliveConfig
(e.g., an instance representing default settings with temporal.disabled or the
same default used by other tests) so the dev(host=..., config_file=None)
invocation is isolated and deterministic.
The Temporal dev server database was accidentally committed. Added it to .gitignore and moved the TemporalConfig import to the top of test_cli.py. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/test_cli.py (1)
288-320:⚠️ Potential issue | 🟡 Minor
test_main_py_detectionis fragile — globalpathlib.Path.existspatch allows actual file I/O inOliveConfig.from_file.
mock.patch("pathlib.Path.exists")mocks the existence check but not the subsequent file read. Whenfrom_file(None)is called, it checksPath.cwd() / ".olive.yaml"with the mockedpath.exists(), and if that returnsTrue, it proceeds to execute the unmockedwith open(path)call. Since.olive.yamlactually exists in the repo root, the test reads and parses the real configuration file, making test behavior dependent on that file's contents.All other dev tests in this PR patch
OliveConfig.from_filedirectly (e.g., line 95, 119, 146), preventing this issue.Add
mock.patch("olive.cli.OliveConfig.from_file", return_value=_temporal_enabled_config())alongside the existing mocks to match the pattern used elsewhere and ensure the test is independent of the filesystem.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_cli.py` around lines 288 - 320, The test test_main_py_detection currently patches pathlib.Path.exists but allows OliveConfig.from_file to perform real file I/O; update the test's mock context to also patch olive.cli.OliveConfig.from_file returning _temporal_enabled_config() (i.e., add mock.patch("olive.cli.OliveConfig.from_file", return_value=_temporal_enabled_config()) to the with(...) block) so dev(...) uses the mocked config and the test no longer depends on the real .olive.yaml file.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@tests/test_cli.py`:
- Around line 288-320: The test test_main_py_detection currently patches
pathlib.Path.exists but allows OliveConfig.from_file to perform real file I/O;
update the test's mock context to also patch olive.cli.OliveConfig.from_file
returning _temporal_enabled_config() (i.e., add
mock.patch("olive.cli.OliveConfig.from_file",
return_value=_temporal_enabled_config()) to the with(...) block) so dev(...)
uses the mocked config and the test no longer depends on the real .olive.yaml
file.
TAOGenna
approved these changes
Feb 18, 2026
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
2 participants
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.
Temporal Cloud requires TLS on all connections. Previously, TLS was only configured when mTLS client certificates were provided, causing API key auth without certs to fail. Now TLS is enabled automatically for any Temporal Cloud connection (detected via is_cloud), regardless of the authentication method.
Additional troubleshooting of PR tests:
Fixes applied:
temporal.enabled=True
hardening
Summary by CodeRabbit
New Features
Tests / CI
Chores