Skip to content

fix(extensions): handle prefix-colliding env vars in _get_env_config#3350

Open
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/config-env-prefix-collision
Open

fix(extensions): handle prefix-colliding env vars in _get_env_config#3350
jawwad-ali wants to merge 2 commits into
github:mainfrom
jawwad-ali:fix/config-env-prefix-collision

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Description

ConfigManager._get_env_config maps SPECKIT_{EXT}_{SECTION}_{KEY} env vars into a nested dict:

current = env_config
for part in config_path[:-1]:
    if part not in current:
        current[part] = {}
    current = current[part]
current[config_path[-1]] = value

Two env vars can collide on a prefix — e.g. SPECKIT_JIRA_CONNECTION=x and SPECKIT_JIRA_CONNECTION_URL=y. Behavior then depends on os.environ iteration order:

  • scalar first → the walk for CONNECTION_URL does current = current['connection'] (a str) then current['url'] = 'y'TypeError: 'str' object does not support item assignment;
  • scalar lastcurrent['connection'] = 'x' overwrites the nested dict → returns {'connection': 'x'}, silently losing connection.url.

Both reproduced on main @ bba473c. Worse: the TypeError propagates through get_config()/has_value() into should_execute_hook's blanket except Exception: return False, so a colliding env var silently disables every config.* hook condition for that extension.

Fix

Guard the walk (if not isinstance(current.get(part), dict): current[part] = {}) and the leaf assignment (skip when a nested dict already occupies it). A colliding scalar yields to the more-specific nested var, so the result is order-independent{'connection': {'url': ...}} either way — matching _merge_configs' dict-preserving semantics.

Design note: nested-wins was chosen for order-independence and consistency with _merge_configs. An alternative (skip/warn on the colliding scalar) is possible; happy to adjust if you'd prefer.

Testing

New TestConfigManagerEnvPrefixCollision (3 tests, all fail-before / pass-after via source-stash):

  • scalar-first and scalar-last both yield {'connection': {'url': 'y'}};
  • e2e: HookExecutor._evaluate_condition('config.connection.url is set', ...) stays True under colliding env (was silently False).

uvx ruff check clean.

AI Disclosure

  • I did use AI assistance (describe below)

Found and fixed with Claude Code (Claude Fable 5) under my direction. AI found the order-dependent crash/clobber and traced it into the swallowed hook exception; I reproduced both modes, verified fail-before/pass-after, and reviewed the diff.

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR makes ConfigManager._get_env_config robust to prefix-colliding environment variables (e.g., SPECKIT_X_CONNECTION vs SPECKIT_X_CONNECTION_URL) so config parsing becomes order-independent and no longer crashes/clobbers nested configuration, preventing hook conditions from being inadvertently disabled.

Changes:

  • Harden _get_env_config to (1) replace colliding scalars with dicts during path walking, and (2) prevent later scalar assignments from overwriting an existing nested dict at the leaf.
  • Add regression tests covering both env insertion orders and a hook-condition scenario under colliding env vars.
Show a summary per file
File Description
src/specify_cli/extensions/__init__.py Guards env-config path walking and leaf assignment to make env parsing order-independent under prefix collisions.
tests/test_extensions.py Adds regression tests ensuring prefix-colliding env vars don’t crash/clobber nested config and don’t break hook condition evaluation.

Review details

Tip

Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

  • Files reviewed: 2/2 changed files
  • Comments generated: 1
  • Review effort level: Low

Comment thread tests/test_extensions.py
jawwad-ali and others added 2 commits July 7, 2026 22:41
_get_env_config built the nested dict with 'if part not in current:
current[part] = {}' and an unconditional leaf assignment. Two env vars
that collide on a prefix — e.g. SPECKIT_X_CONNECTION and
SPECKIT_X_CONNECTION_URL — then either crash (scalar processed first:
the walk indexes into a str -> TypeError 'str object does not support
item assignment') or silently clobber the nested dict (scalar processed
last). Via should_execute_hook's blanket except, the crash silently
disables every config-based hook for the extension. Guard the walk and
the leaf assignment with isinstance checks so a colliding scalar yields
to the nested dict; result is order-independent
({'connection': {'url': ...}} either way), matching _merge_configs'
dict-preserving semantics.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…helper

Per review: the colliding-env hook test described should_execute_hook
swallowing the TypeError, but asserted on the private _evaluate_condition.
Assert on the public should_execute_hook instead — it returns False
(silently disabled) before the fix and True after, matching the
real-world failure mode and not coupling to a private helper.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jawwad-ali jawwad-ali force-pushed the fix/config-env-prefix-collision branch from f785916 to ea0dacd Compare July 7, 2026 17:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants