Skip to content

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

Merged
mnriem merged 3 commits into
github:mainfrom
jawwad-ali:fix/config-env-prefix-collision
Jul 13, 2026
Merged

fix(extensions): handle prefix-colliding env vars in _get_env_config#3350
mnriem merged 3 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 jawwad-ali force-pushed the fix/config-env-prefix-collision branch from f785916 to ea0dacd Compare July 7, 2026 17:42
@mnriem mnriem requested a review from Copilot July 8, 2026 12:40

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.

Review details

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

Comment thread src/specify_cli/extensions/__init__.py Outdated
mnriem
mnriem previously approved these changes Jul 9, 2026

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Address Copilot feedback

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.

Review details

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

@mnriem

mnriem commented Jul 10, 2026

Copy link
Copy Markdown
Collaborator

Please address test & lint errors

jawwad-ali and others added 3 commits July 10, 2026 21:20
_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>
Per review: a name like SPECKIT_<EXT>_ (no key) or with consecutive
underscores produced empty path components, creating surprising entries
under an empty key (env_config[''] = ...). Filter out empty parts and
skip the variable entirely when nothing remains.

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@jawwad-ali jawwad-ali force-pushed the fix/config-env-prefix-collision branch from fc58c8f to e48715c Compare July 10, 2026 16:23
@mnriem mnriem requested a review from Copilot July 13, 2026 12:21

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.

Review details

  • Files reviewed: 2/2 changed files
  • Comments generated: 0 new
  • Review effort level: Medium

@mnriem mnriem merged commit e59da78 into github:main Jul 13, 2026
12 checks passed
@mnriem

mnriem commented Jul 13, 2026

Copy link
Copy Markdown
Collaborator

Thank you!

mnriem pushed a commit that referenced this pull request Jul 14, 2026
…extension IDs (#3497)

* fix(extensions): stop env-var config leaking across prefix-colliding IDs (#3494)

Because ``_`` doubles as both the separator between an extension ID and
its config path AND the substitute for ``-`` inside an extension ID, an
env var like ``SPECKIT_GIT_HOOKS_URL`` starts with *both* the
``SPECKIT_GIT_`` prefix of the ``git`` extension and the
``SPECKIT_GIT_HOOKS_`` prefix of a co-installed ``git-hooks`` extension.
``ConfigManager._get_env_config`` matched only on the shorter prefix,
so the same env var silently surfaced inside both extensions' configs
(as ``{'hooks': {'url': ...}}`` for ``git`` and ``{'url': ...}`` for
``git-hooks``).

Impact: config intended for one extension leaked into another and, worse,
could flip ``config.<field> is set`` hook conditions on the wrong
extension.

Route the env var to the extension whose normalized ID is the longest
match — the more specific one. When another installed sibling's
normalized ID + ``_`` claims the remainder, skip the var here. The
sibling scan reads ``.specify/extensions/`` directly and degrades to a
no-op if the dir is missing (fresh project / ad-hoc harness), so the
pre-fix single-extension behaviour is unchanged when there is no
collision.

Distinct from #3350 (intra-extension prefix collision between two keys
of the same extension) — this fixes the cross-extension case.

Fixes #3494

* fix(extensions): source sibling scan from registry, not directory

Address Copilot review on #3497: ``ExtensionManager.remove(...,
keep_config=True)`` preserves the extension directory but drops the
registry entry, so the previous directory-scan approach would treat a
config-only leftover as an installed sibling and silently discard
``SPECKIT_<sibling>_*`` env vars into no owner. Sourced the sibling
list from ``ExtensionRegistry.keys()`` — the registry is the source of
truth for "installed" — and kept the same graceful ``[]`` fallback so
the fresh-project / ad-hoc harness path is unaffected. Updated the
``TestConfigManagerCrossExtensionEnvLeak`` ``_install`` helper to
register its fake installations and added
``test_config_only_leftover_not_treated_as_sibling`` to lock in the
new behaviour for the ``keep_config=True`` scenario.

Full suite: 3978 passed, 110 skipped.

* fix(extensions): swallow non-UTF-8 registry in sibling scan

Address Copilot follow-up on #3497: ``ExtensionRegistry._load()`` catches
``JSONDecodeError`` / ``FileNotFoundError`` but not decode failures — a
registry file with invalid text encoding would surface a
``UnicodeDecodeError`` out of ``_sibling_extension_ids`` and break every
config read instead of degrading to the documented pre-fix behaviour.
Extend the fallback in ``_sibling_extension_ids`` to also catch
``UnicodeError`` and add ``test_non_utf8_registry_does_not_crash`` as a
regression pin (kept ``_load()`` itself out of scope — that broader
hardening belongs in a separate PR since it affects all readers).

Full suite: 3979 passed, 110 skipped.
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