session: inherit ANTHROPIC env into spawned Claude sessions #1 #662
Conversation
|
|
There was a problem hiding this comment.
Important
Looks good to me! 👍
Reviewed everything up to 3881710 in 1 minute and 12 seconds. Click for details.
- Reviewed
70lines of code in1files - Skipped
0files when reviewing. - Skipped posting
3draft comments. View those below. - Modify your settings and rules to customize what types of comments Ellipsis leaves. And don't forget to react with 👍 or 👎 to teach Ellipsis.
1. hld/session/manager.go:366
- Draft comment:
Good use of initializing claudeConfig.Env and then conditionally inheriting ANTHROPIC_BASE_URL and ANTHROPIC_API_KEY from the daemon. This properly avoids nil map panics and respects explicit overrides. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
2. hld/session/manager.go:1722
- Draft comment:
Similarly, in ContinueSession the config.Env is initialized and daemon-level ANTHROPIC_* variables are inherited correctly when not explicitly set. This resolves potential nil map issues on resumed sessions. - Reason this comment was not posted:
Confidence changes required:0%<= threshold50%None
3. hld/session/manager.go:1746
- Draft comment:
Consider refactoring the common ANTHROPIC environment variable injection logic into a helper function to avoid duplication between LaunchSession and ContinueSession. - Reason this comment was not posted:
Confidence changes required:33%<= threshold50%None
Workflow ID: wflow_cRsk06FATLXf949O
You can customize by changing your verbosity settings, reacting with 👍 or 👎, replying to comments, or adding code review rules.
|
thanks - okay so it does work from CLI? I think this approach is fine. At a certain point we need a more complete design around env vars, since you may want arbitrary vars to be passed in at a global, per-project, or per-session level |
|
@dexhorthy is there any thing need to change from there please give me in brief |
dexhorthy
left a comment
There was a problem hiding this comment.
this looks like the correct implementation - can you add a unit test for the config.go changes just to prove out some basic happy-path examples?
1fce6d8 to
31adc22
Compare
|
I have tested this with a LINEAR_API_KEY and it works, and the tests are close. One last thing - I'd like us to have coverage/demonstration in the tests on what is the behavior when env vars are set and config.json has values - what takes precedence? e.g.
I think env vars + launching with |
|
@dexhorthy I am developing on ubantu ,I am launching through cd humanlayer-wui && bun dev and also used from tauri |
|
okay thanks - does my question about adding more unit tests for env var override behavior make sense? |
c46775c to
76cecc5
Compare
Add comprehensive unit tests for the config.go Env map functionality: - Test case preservation for environment variable keys (ANTHROPIC_BASE_URL, AWS_REGION, etc.) - Test handling of missing env section - Test empty env object behavior - Test save/load round-trip preservation - Test special characters in env values (URLs, paths, spaces) These tests validate that the case-sensitive environment variable configuration works correctly for Bedrock and other Anthropic-compatible endpoints.
- Properly handle error returns in deferred cleanup functions - Remove unnecessary nil check for cfg.Env (len() is safe on nil maps) - Fixes errcheck and staticcheck linter warnings
6cb3421 to
8bc5fb4
Compare
|
@VAIBHAVSING sorry for the delay here - I've just found that claude supports env vars in see the for now we will advise folks to use this instead of having env var management in the product I super appreciate your contribution and hope that us closing won't dissuade you from contributing in the future. https://docs.claude.com/en/docs/claude-code/settings#available-settings |
This PR initializes session's Env map when resuming sessions and inherits daemon-level ANTHROPIC_BASE_URL and ANTHROPIC_API_KEY into spawned Claude processes when not explicitly set.
Why: Enables running Claude against Anthropic-compatible endpoints such as AWS Bedrock by setting the env vars at the daemon level.
Changes:
Initialize config.Env in resumed session flow to avoid nil map panics\n- Inject ANTHROPIC_BASE_URL and ANTHROPIC_API_KEY from the daemon environment into session Env when missing
Closes #598
Important
Inherit
ANTHROPIC_BASE_URLandANTHROPIC_API_KEYfrom daemon environment into Claude sessions inmanager.go, enabling Anthropic-compatible endpoints.LaunchSessionandContinueSessioninmanager.go, inheritANTHROPIC_BASE_URLandANTHROPIC_API_KEYfrom daemon environment if not set in session config.config.Envmap inContinueSessionto prevent nil map panics.This description was created by
for 3881710. You can customize this summary. It will automatically update as commits are pushed.