fix(cli): add local_development.redis_enabled manifest opt-out (MAY-1086)#365
Open
IdkwhatImD0ing wants to merge 3 commits into
Open
fix(cli): add local_development.redis_enabled manifest opt-out (MAY-1086)#365IdkwhatImD0ing wants to merge 3 commits into
IdkwhatImD0ing wants to merge 3 commits into
Conversation
…086) agentex agents run unconditionally sets REDIS_URL=redis://localhost:6379 in the agent process env. Combined with the module-level RedisStreamRepository instantiated by `from agentex.lib import adk`, this causes silent request hangs for agents that don't use adk.messages/adk.streaming when no local Redis is reachable (e.g. Temporal-direct async agents in restricted dev pods). The lazy redis.asyncio client only fails on first publish, so there is no startup error to point at the misconfiguration. Add an explicit `local_development.redis_enabled: bool = true` flag. Default true preserves existing behavior; users with no local Redis can set false to skip the REDIS_URL default. acp_type is not a reliable discriminator here — the codebase has acp_type=async tutorials that legitimately call adk.messages.create. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Address staff-engineer review on #365: - Pop REDIS_URL from the inherited env when redis_enabled is false. The previous implementation only avoided seeding a default, so a stale REDIS_URL exported by the parent shell still leaked into the agent process and reproduced the same silent hang the opt-out was meant to prevent. - Drop the unreachable `local_development is None` short-circuit; the dict literal above already dereferences manifest.local_development with a # type: ignore[union-attr], so the None branch can never run. - Add tests/lib/cli/test_run_handlers.py covering the three branches: default seeds REDIS_URL, opt-out with clean parent env clears it, opt-out with a parent-shell REDIS_URL still clears it. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Contributor
Author
|
Follow-up commit
Deferring the other review points:
|
Use a tmp_path YAML fixture rendered from a minimal inline template instead of reading examples/tutorials/.../manifest.yaml, so the test survives tutorial renames or removals while still exercising the real AgentManifest.from_yaml loader. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
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
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
agentex agents rununconditionally setsREDIS_URL=redis://localhost:6379in the agent process env. Combined with the module-levelRedisStreamRepositoryfromfrom agentex.lib import adk, agents that don't actually useadk.messages/adk.streamingsee silent request hangs when no local Redis is reachable (e.g. Temporal-direct async agents running in restricted dev pods). The lazyredis.asyncioclient only fails on first publish, so there's no startup error to surface.local_development.redis_enabled: bool = true(default) flag on the agent manifest. Default preserves existing behavior; users with no local Redis can setfalseto opt out of theREDIS_URLdefault.acp_typeis not a reliable discriminator:examples/tutorials/10_async/00_base/110_pydantic_aiand100_langgraphareacp_type: asyncand legitimately calladk.messages.create(...); severalacp_type: synctutorials also useadk.messages. Hence the explicit per-manifest opt-out instead of anacp_type-based rule.Files
src/agentex/lib/sdk/config/local_development_config.py— newredis_enabled: bool = Truefield onLocalDevelopmentConfig.src/agentex/lib/cli/handlers/run_handlers.py—create_agent_environmentskips theREDIS_URLdefault whenlocal_development.redis_enabledisfalse.Example manifest opt-out
Test plan
rye run pyrighton modified files — 0 errors.rye run ruff checkon modified files — clean.REDIS_URLset;redis_enabled: false→REDIS_URLabsent (verified againstexamples/tutorials/10_async/00_base/110_pydantic_ai/manifest.yaml).tests/...matchinglocal_development|run_handlers|manifest) passes.Linear
MAY-1086
🤖 Generated with Claude Code
Greptile Summary
This PR adds a
local_development.redis_enabled: bool = Truefield toLocalDevelopmentConfigand gates the automaticREDIS_URL=redis://localhost:6379injection increate_agent_environmenton that flag. The fix addresses silent request hangs in dev pods where no local Redis is reachable.local_development_config.py: Addsredis_enabledfield withdefault=Trueto preserve existing behavior; agents that don't useadk.messages/adk.streamingcan set it tofalse.run_handlers.py: The conditional now setsREDIS_URLinenv_varswhen opted in, and actively pops any inherited parent-shellREDIS_URLwhen opted out — ensuring the opt-out is clean even when the shell exports the variable.test_run_handlers.py: Three parametric scenarios cover the default, opt-out-clean, and opt-out-with-stale-parent-env cases.Confidence Score: 5/5
Safe to merge; the change is an additive opt-out flag with a backward-compatible default, and the core env-mutation logic is correctly ordered relative to the final env.update call.
The new field defaults to True, so all existing manifests see no behavior change. The opt-out path pops REDIS_URL from the os.environ snapshot before env.update(env_vars) is called, so the removal is durable. Tests cover the three relevant scenarios. No other code paths are affected.
No files require special attention.
Important Files Changed
Flowchart
%%{init: {'theme': 'neutral'}}%% flowchart TD A[create_agent_environment] --> B["env = dict(os.environ)"] B --> C["Build env_vars: TEMPORAL_ADDRESS, AGENT_NAME, ACP_TYPE, ACP_URL, ACP_PORT"] C --> D{"manifest.local_development.redis_enabled?"} D -- "True (default)" --> E["env_vars['REDIS_URL'] = 'redis://localhost:6379'"] D -- "False" --> F["env.pop('REDIS_URL', None)"] E --> G["Apply agent_config.env overrides to env_vars"] F --> G G --> H["env.update(env_vars)"] H --> I["return env"]Reviews (3): Last reviewed commit: "test(cli): decouple run_handlers tests f..." | Re-trigger Greptile