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

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

_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>

* test(extensions): assert via public should_execute_hook, not private 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>

* fix(extensions): ignore malformed env var names in _get_env_config

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>

---------

Co-authored-by: Claude Fable 5 <noreply@anthropic.com>

Author: jawwad-ali

src/specify_cli/extensions/__init__.py

@@ -2755,18 +2755,32 @@ def _get_env_config(self) -> Dict[str, Any]:
             if not key.startswith(prefix):
                 continue
 
-            # Remove prefix and split into parts
-            config_path = key[len(prefix) :].lower().split("_")
+            # Remove prefix and split into parts. Drop empty components from a
+            # malformed name (e.g. ``SPECKIT_<EXT>_`` with no key, or
+            # consecutive underscores ``SPECKIT_X__Y``) so we never create an
+            # entry under an empty key.
+            config_path = [p for p in key[len(prefix) :].lower().split("_") if p]
+            if not config_path:
+                continue
 
-            # Build nested dict
+            # Build nested dict. Two env vars can collide on a prefix, e.g.
+            # SPECKIT_X_CONNECTION=a and SPECKIT_X_CONNECTION_URL=b. Guard the
+            # walk so a colliding scalar is replaced by a dict (deeper/more
+            # specific vars win) instead of being indexed into — which raised
+            # TypeError ('str' object does not support item assignment) — and
+            # guard the leaf so a scalar processed after the nested var does
+            # not clobber the nested dict. Order-independent: both insertion
+            # orders yield {'connection': {'url': ...}}. Nested-wins mirrors
+            # _merge_configs' dict-preserving semantics.
             current = env_config
             for part in config_path[:-1]:
-                if part not in current:
+                if not isinstance(current.get(part), dict):
                     current[part] = {}
                 current = current[part]
 
-            # Set the final value
-            current[config_path[-1]] = value
+            # Set the final value, unless a nested dict already occupies it.
+            if not isinstance(current.get(config_path[-1]), dict):
+                current[config_path[-1]] = value
 
         return env_config
 

tests/test_extensions.py

@@ -7628,3 +7628,56 @@ def test_hook_condition_returns_false_without_raising(self, tmp_path):
         (ext_dir / "jira-config.yml").write_text("just a string\n", encoding="utf-8")
         executor = HookExecutor(tmp_path)
         assert executor._evaluate_condition("config.x is set", "jira") is False
+
+
+class TestConfigManagerEnvPrefixCollision:
+    """Prefix-colliding env vars must not crash or clobber nested config."""
+
+    def test_scalar_then_nested_yields_nested(self, tmp_path, monkeypatch):
+        """SPECKIT_X_CONNECTION=x then SPECKIT_X_CONNECTION_URL=y.
+
+        The scalar-first order previously raised TypeError ('str' object
+        does not support item assignment) when the walk indexed into 'x'.
+        """
+        monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION", "x")
+        monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION_URL", "y")
+        cm = ConfigManager(tmp_path, "testext")
+        assert cm._get_env_config() == {"connection": {"url": "y"}}
+
+    def test_nested_then_scalar_does_not_clobber(self, tmp_path, monkeypatch):
+        """Reverse order previously returned {'connection': 'x'}, losing url."""
+        monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION_URL", "y")
+        monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION", "x")
+        cm = ConfigManager(tmp_path, "testext")
+        assert cm._get_env_config() == {"connection": {"url": "y"}}
+
+    def test_colliding_env_does_not_disable_hook_condition(self, tmp_path, monkeypatch):
+        """`config.connection.url is set` must stay True under colliding env.
+
+        Before the fix the TypeError propagated into should_execute_hook's
+        blanket `except Exception: return False`, silently disabling the hook.
+        """
+        ext_dir = tmp_path / ".specify" / "extensions" / "testext"
+        ext_dir.mkdir(parents=True)
+        (ext_dir / "testext-config.yml").write_text(
+            "connection:\n  url: https://example.com\n", encoding="utf-8"
+        )
+        monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION", "x")
+        monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION_URL", "y")
+        executor = HookExecutor(tmp_path)
+        # Exercise the public API: before the fix the TypeError was swallowed
+        # by should_execute_hook's `except Exception: return False`, so the
+        # hook was silently disabled (False); after the fix it returns True.
+        assert executor.should_execute_hook(
+            {"condition": "config.connection.url is set", "extension": "testext"}
+        ) is True
+
+    def test_malformed_env_names_ignored(self, tmp_path, monkeypatch):
+        """A name with no key (SPECKIT_X_) or empty parts (consecutive
+        underscores) must not create an entry under an empty key."""
+        monkeypatch.setenv("SPECKIT_TESTEXT_", "orphan")  # no key at all
+        monkeypatch.setenv("SPECKIT_TESTEXT_A__B", "z")   # empty middle part
+        cm = ConfigManager(tmp_path, "testext")
+        cfg = cm._get_env_config()
+        assert "" not in cfg
+        assert cfg == {"a": {"b": "z"}}