From 1149ba941cf7066f9a9e6478e320f4370a30a191 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sun, 5 Jul 2026 19:38:54 +0500 Subject: [PATCH 1/3] fix(extensions): handle prefix-colliding env vars in _get_env_config MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit _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 --- src/specify_cli/extensions/__init__.py | 17 ++++++++--- tests/test_extensions.py | 40 ++++++++++++++++++++++++++ 2 files changed, 53 insertions(+), 4 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index b84b836545..856f13d124 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2758,15 +2758,24 @@ def _get_env_config(self) -> Dict[str, Any]: # Remove prefix and split into parts config_path = key[len(prefix) :].lower().split("_") - # 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 diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 2885180360..9cf8e50604 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7628,3 +7628,43 @@ 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) + assert executor._evaluate_condition( + "config.connection.url is set", "testext" + ) is True From ff85586cab3cc54346603f6c84b51ec8114742a3 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Tue, 7 Jul 2026 22:17:25 +0500 Subject: [PATCH 2/3] test(extensions): assert via public should_execute_hook, not private helper MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- tests/test_extensions.py | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 9cf8e50604..06de54ccef 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7665,6 +7665,9 @@ def test_colliding_env_does_not_disable_hook_condition(self, tmp_path, monkeypat monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION", "x") monkeypatch.setenv("SPECKIT_TESTEXT_CONNECTION_URL", "y") executor = HookExecutor(tmp_path) - assert executor._evaluate_condition( - "config.connection.url is set", "testext" + # 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 From e48715c679b1a998f515d286933a4ceb6d903187 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 9 Jul 2026 19:33:08 +0500 Subject: [PATCH 3/3] fix(extensions): ignore malformed env var names in _get_env_config Per review: a name like SPECKIT__ (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 --- src/specify_cli/extensions/__init__.py | 9 +++++++-- tests/test_extensions.py | 10 ++++++++++ 2 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/specify_cli/extensions/__init__.py b/src/specify_cli/extensions/__init__.py index 856f13d124..1f898c5f78 100644 --- a/src/specify_cli/extensions/__init__.py +++ b/src/specify_cli/extensions/__init__.py @@ -2755,8 +2755,13 @@ 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__`` 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. Two env vars can collide on a prefix, e.g. # SPECKIT_X_CONNECTION=a and SPECKIT_X_CONNECTION_URL=b. Guard the diff --git a/tests/test_extensions.py b/tests/test_extensions.py index 06de54ccef..cb3120f492 100644 --- a/tests/test_extensions.py +++ b/tests/test_extensions.py @@ -7671,3 +7671,13 @@ def test_colliding_env_does_not_disable_hook_condition(self, tmp_path, monkeypat 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"}}