fix(HYBIM-787): clear bridged GALILEO_* env vars on reset()#82
Conversation
The bridge in _bridge_env_vars() guards against overwriting a GALILEO_* key that is already set in os.environ. After the first get() call every GALILEO_* key is permanently present, so a subsequent reset() followed by a credential change left galileo-core reading the original stale bridged value silently. Fix: extract _BRIDGE to module level and have SplunkAOConfig.reset() pop every GALILEO_* key the bridge may have written before calling super().reset(). The next get() therefore re-bridges from the current SPLUNK_AO_* values. Also adds three regression tests covering: - reset() removes all bridgeable GALILEO_* keys - get() → reset() → credential rotation → get() picks up the new key - documents the trade-off that reset() owns all GALILEO_* keys Co-authored-by: Cursor <cursoragent@cursor.com>
fercor-cisco
left a comment
There was a problem hiding this comment.
🤖 This review was generated by the Astra agent (claude-opus-4-8). It may contain mistakes.
Verdict: approve — Fix correctly clears stale bridged env vars and is well-tested; one design asymmetry worth noting but non-blocking given the documented trade-off.
Follow-ups
Suggested follow-up work that could be tracked as Shortcut stories:
tests/test_config.py:151-158: test_bridge_picks_up_new_credential_after_reset writes GALILEO_API_KEY directly to os.environ and only pops it on the final line. If any earlier assertion fails, the manual cleanup is skipped and GALILEO_API_KEY leaks into subsequent tests. Consider wrapping the direct os.environ writes in a patch.dict(..., clear=False) block (as the other bridge tests do) so cleanup is guaranteed regardless of assertion outcome.
| for _splunk_key, galileo_key in _BRIDGE: | ||
| os.environ.pop(galileo_key, None) |
There was a problem hiding this comment.
🟡 minor (design): reset() unconditionally pops every GALILEO_* key in _BRIDGE, but _bridge_env_vars() deliberately preserves user-set GALILEO_* values ("explicit GALILEO_* overrides win"). So a user who sets GALILEO_API_KEY directly — without a SPLUNK_AO_* equivalent — will have it silently deleted by reset(), and the next get() will fail with no credential to re-bridge. This breaks the documented override invariant. The PR acknowledges this as a trade-off ("the bridge owns all GALILEO_* keys"), but the safer, symmetric fix is to only clear keys the bridge actually wrote: have _bridge_env_vars() record the keys it set (e.g. a class-level set) and have reset() pop only those. That fixes the stale-bridge bug without touching user-provided overrides. Worth confirming the "no consumer sets GALILEO_* directly" assumption holds for downstream SDKs (splunk-ao-adk / a2a) before relying on it.
🤖 Generated by the Astra agent
fercor-cisco
left a comment
There was a problem hiding this comment.
I think the two nits brought up by Claude are acceptable.
Summary
SplunkAOConfig.get()call, everyGALILEO_*env var is permanently written toos.environ. Whenreset()is called,galileo-core's reset only clears the config object's in-memory fields — it never removes those env vars. On the nextget(), the bridge seesGALILEO_API_KEY"already set" and skips the copy, sogalileo-coresilently re-authenticates with the stale original credentials._check_auth_configreadsSPLUNK_AO_*directly._BRIDGEto module level soreset()can reference it, then haveSplunkAOConfig.reset()pop()everyGALILEO_*key before callingsuper().reset(). The nextget()re-bridges cleanly from the currentSPLUNK_AO_*values.Closes HYBIM-787.
Changes
src/splunk_ao/config.py: Extract_BRIDGEto module level; extendSplunkAOConfig.reset()to clear bridgedGALILEO_*env vars.tests/test_config.py: Add three regression tests covering the stale-bridge scenario.Test plan
test_reset_clears_bridged_galileo_env_vars—reset()removes everyGALILEO_*key the bridge may have writtentest_bridge_picks_up_new_credential_after_reset— fullget()→reset()→ credential rotation →get()flow uses the new keytest_reset_removes_all_bridgeable_galileo_vars— documents the trade-off thatreset()owns all bridgeGALILEO_*keystest_config.pytests pass (45 total)splunk-ao-adk: 252 passedsplunk-ao-a2a: 67 passedMade with Cursor