fix(extensions): stop env-var config leaking across prefix-colliding extension IDs by thejesh23 · Pull Request #3497 · github/spec-kit

thejesh23 · 2026-07-13T17:38:59Z

What

ConfigManager._get_env_config reads SPECKIT_<EXT_ID>_... env vars by prefix-match. But because _ doubles as both the separator between the extension ID and the 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.

Repro on main (0.12.13.dev0):

os.environ["SPECKIT_GIT_URL"]       = "value_for_git"
os.environ["SPECKIT_GIT_HOOKS_URL"] = "value_for_git_hooks"
ConfigManager(project, "git")._get_env_config()
# -> {'url': 'value_for_git', 'hooks': {'url': 'value_for_git_hooks'}}
#                             ^^^^^^^^^^^ leaked from git-hooks

Impact:

config intended for one extension surfaces inside another's config namespace,
hook conditions like config.hooks.url is set fire on the wrong extension,
documented env-over-YAML precedence is silently violated for an unrelated extension.

Distinct from #3350, which fixed intra-extension collisions (two keys of the same extension colliding on a prefix). This PR fixes the cross-extension case.

Fix

Route each env var to the extension whose normalized ID is the longest match — the more specific one:

Look up sibling extension IDs from ExtensionRegistry (.specify/extensions/.registry) — the registry is the source of truth for "installed", not the on-disk directory. A bare <sibling>/ directory can be a config-only leftover from ExtensionManager.remove(..., keep_config=True), which preserves the dir but drops the registry entry; treating it as installed would silently discard SPECKIT_<leftover>_* env vars into no owner.
For each sibling whose normalized ID extends the current extension's (GIT_HOOKS starts with GIT_), record the portion of the remainder it claims, including a trailing _ boundary (so sibling hook cannot false-positive on key hooks).
When an env var's remainder starts with any sibling-owned prefix, skip it — it belongs to the sibling.

Fallbacks in _sibling_extension_ids:

Missing registry (fresh project, ad-hoc harness) → ExtensionRegistry(...).keys() returns empty; behaviour is identical to pre-fix.
Corrupted / non-UTF-8 registry → _sibling_extension_ids catches (OSError, UnicodeError) and returns [] so a broken registry degrades to the pre-fix single-extension path rather than aborting every config read (ExtensionRegistry._load() only handles JSONDecodeError / FileNotFoundError, so a non-UTF-8 file would otherwise surface UnicodeDecodeError). Hardening _load() itself is left for a separate PR since it affects every reader.
No colliding sibling installed → var is still absorbed as before (the fix is scoped strictly to the collision case, avoiding surprise for users who set a nested env key without a sibling to disambiguate against).

Tests

New class TestConfigManagerCrossExtensionEnvLeak with 7 tests:

test_sibling_owns_longer_prefix_env — the reproducer above, now correct.
test_no_sibling_installed_keeps_legacy_absorption — no regression when only one extension is installed.
test_non_prefix_sibling_ignored — sibling not-git does not affect git.
test_boundary_prevents_false_positive — sibling git-hook does not eat HOOKS_* keys.
test_missing_extensions_dir_does_not_crash — sibling scan degrades cleanly when .specify/extensions/ is absent (registry lookup returns empty).
test_config_only_leftover_not_treated_as_sibling — a git-hooks/ directory left behind by remove(..., keep_config=True) (dir + config preserved, no registry entry) is not treated as installed; git still absorbs SPECKIT_GIT_HOOKS_*.
test_non_utf8_registry_does_not_crash — a registry file containing invalid UTF-8 does not propagate UnicodeDecodeError out of the sibling scan.

The test helper _install now writes both the directory and an ExtensionRegistry.add(...) entry, matching the registry-as-source-of-truth model.

Adjacent TestConfigManagerEnvPrefixCollision (from #3350) still passes.

Full suite:

$ .venv/bin/python -m pytest tests -q --tb=no
========== 3979 passed, 110 skipped, 82 warnings in 67.58s (0:01:07) ===========

Manual test results

Agent: N/A — this is a config-loader fix; no slash command's behaviour changes. | OS/Shell: macOS 15 / zsh

Command tested	Notes
n/a	Only `src/specify_cli/extensions/__init__.py` (`ConfigManager._get_env_config` + a new private helper `_sibling_extension_ids`) is modified. No `templates/commands/`, no `scripts/bash\|powershell/`, no `src/specify_cli/*` CLI entry point, no extension.yml/manifest surface.

AI disclosure

Per CONTRIBUTING.md#ai-contributions-in-spec-kit: this PR (bug identification, patch, and tests) was prepared with Claude (Anthropic) assistance in an autonomous agent session. Reproducer was executed against a fresh clone of main before and after the change; the full test suite (3,979 tests) was run green before submission. The fix is deliberately narrow — one new private helper (_sibling_extension_ids), one added guard in _get_env_config, and seven focused regression tests. The design intentionally preserves the pre-fix single-extension behaviour when no colliding sibling is installed, so the change is a pure additive-safety fix rather than a policy shift.

🤖 Generated with Claude Code

…IDs (github#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 github#3350 (intra-extension prefix collision between two keys of the same extension) — this fixes the cross-extension case. Fixes github#3494

Copilot

Pull request overview

Prevents environment-variable configuration from leaking between prefix-colliding extension IDs.

Changes:

Filters variables owned by longer sibling extension IDs.
Adds regression and fallback coverage.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File	Description
`src/specify_cli/extensions/__init__.py`	Adds sibling-aware environment-variable routing.
`tests/test_extensions.py`	Tests collisions, boundaries, and fallback behavior.

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

mnriem · 2026-07-13T20:06:40Z

Please address Copilot feedback

Address Copilot review on github#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.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

Address Copilot follow-up on github#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.

Copilot

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated no new comments.

Copilot

Review details

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

thejesh23 · 2026-07-13T22:37:20Z

+        Sourced from ``ExtensionRegistry`` (``.specify/extensions/.registry``)
+        rather than a directory scan: ``ExtensionManager.remove(...,
+        keep_config=True)`` deliberately preserves the extension directory
+        while dropping the registry entry, so a directory scan would treat
+        that config-only leftover as an installed sibling and keep silently
+        absorbing its ``SPECKIT_<sibling>_*`` env vars into no one. The
+        registry is the source of truth for "installed".


Good catch — updated the Fix and Tests sections of the PR description to reflect the registry-sourced sibling scan (ExtensionRegistry.keys() instead of the directory scan) and the current seven-test list, including test_config_only_leftover_not_treated_as_sibling (keep_config=True leftover) and test_non_utf8_registry_does_not_crash (corrupted-encoding fallback). The Fix section also now names the (OSError, UnicodeError) fallback in _sibling_extension_ids and its rationale. No code changes in this update — description-only.

mnriem · 2026-07-13T22:30:23Z

Please address Copilot feedback

thejesh23 · 2026-07-13T22:40:43Z

@mnriem addressed comments

Copilot

Review details

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

rickgonzalez · 2026-07-13T23:25:51Z

An explanation of this PR scanned by a tool I'm working on - Hope that it helps: https://www.lenzon.ai/viewer/cmrjulyib0005m7f49oejcyx5?voice=google-chirp3

Copilot AI review requested due to automatic review settings July 13, 2026 17:39

thejesh23 requested a review from mnriem as a code owner July 13, 2026 17:39

thejesh23 mentioned this pull request Jul 13, 2026

fix(extensions): env-var config leaks across prefix-colliding extension IDs in _get_env_config #3494

Open

Copilot started reviewing on behalf of thejesh23 July 13, 2026 17:39 View session