From 6780d2a562f8c640c1174c0dbfa2f9c324c66edf Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Mon, 5 Oct 2026 21:47:17 +0500 Subject: [PATCH 1/2] fix(bundler): resolve the active integration like the canonical reader `active_integration`'s own comment says it matches the canonical reader in `integration_state`, but it diverged from it in three ways: 1. It only checked `isinstance(value, str) and value`, while the canonical reader runs every value through `clean_integration_key`. A whitespace-only key is truthy, so it was returned as a real integration and suppressed the "not determinable" fallback; a padded key was returned verbatim and matches no registered integration. 2. Normalizing only the first truthy raw field loses a valid legacy key behind a blank `default_integration`: {"default_integration": " ", "integration": "copilot"} raw-then-clean -> None canonical -> 'copilot' Each candidate is now cleaned before it is selected. 3. Installed-only state (`installed_integrations` populated, no default recorded) returned None, while `normalize_integration_state` promotes `installed_integrations[0]`. None tells `bundle install` / `bundle update` the integration "cannot be determined", which lets an explicit `--integration` bypass the FR-019 integration-clash guard. The fallback is consulted last, so no marker that already resolved changes. Tests live beside the existing `active_integration` tests in tests/specify_cli/bundles/test_security_paths.py; the installed-only cases assert agreement with `default_integration_key(normalize_integration_state())`. Rebased onto current main (files moved in the workflow/bundler restructure). Co-Authored-By: Claude Opus 5.5 (1M context) --- src/specify_cli/bundles/project.py | 45 +++++-- .../bundles/test_security_paths.py | 119 ++++++++++++++++++ 2 files changed, 154 insertions(+), 10 deletions(-) diff --git a/src/specify_cli/bundles/project.py b/src/specify_cli/bundles/project.py index 6476b5c054..eb96e3f973 100644 --- a/src/specify_cli/bundles/project.py +++ b/src/specify_cli/bundles/project.py @@ -4,6 +4,7 @@ from pathlib import Path from .._project import _resolve_init_dir_override +from ..integration_state import clean_integration_key, dedupe_integration_keys from . import BundlerError from .yamlio import ensure_within, load_json @@ -82,8 +83,8 @@ def active_integration(project_root: Path) -> str | None: except BundlerError: return None if isinstance(data, dict): - # ``default_integration`` first, matching the canonical reader in - # ``integration_state`` (line 199): + # ``default_integration`` first, matching the canonical reader + # (``integration_state.default_integration_key``): # ``state.get("default_integration") or state.get("integration")``. # ``write_integration_json`` writes both keys, so a marker produced by # the current CLI already resolved through the ``integration`` alias -- @@ -91,12 +92,36 @@ def active_integration(project_root: Path) -> str | None: # about resolving a marker that carries only ``default_integration`` # (hand-edited, or written by anything that follows the canonical # reader's shape). ``integration``/``id``/``active`` stay as fallbacks. - value = ( - data.get("default_integration") - or data.get("integration") - or data.get("id") - or data.get("active") - ) - if isinstance(value, str) and value: - return value + # Clean EACH candidate before selecting it, rather than picking the + # first truthy raw value and normalizing only that one. A raw ``or`` + # chain selects a whitespace-only ``default_integration`` (truthy) and + # then normalizes it to ``None``, losing the valid legacy key behind + # it -- whereas ``normalize_integration_state`` does + # ``clean_integration_key(data.get("default_integration")) or + # legacy_key`` and falls through: + # {"default_integration": " ", "integration": "copilot"} + # raw-then-clean -> None canonical -> 'copilot' + # + # Normalizing through the shared helper also fixes the original + # divergence: ``isinstance(value, str) and value`` accepted a + # whitespace-only key as real -- truthy, so it suppressed the "not + # determinable" fallback -- and returned a padded key verbatim, which + # matches no registered integration. + for field in ("default_integration", "integration", "id", "active"): + cleaned = clean_integration_key(data.get(field)) + if cleaned: + return cleaned + # Installed-only state -- ``installed_integrations`` populated but no + # default recorded -- still has an active integration: the canonical + # ``normalize_integration_state`` promotes ``installed_integrations[0]`` + # to the default. Returning None here instead told callers the + # integration "cannot be determined", which lets an explicit + # ``--integration`` bypass the FR-019 integration-clash guard in + # ``bundle install`` / ``bundle update``. Checked last, so a marker + # that already resolved through the fields above is unaffected. + installed = data.get("installed_integrations") + if isinstance(installed, list): + installed_keys = dedupe_integration_keys(installed) + if installed_keys: + return installed_keys[0] return None diff --git a/tests/specify_cli/bundles/test_security_paths.py b/tests/specify_cli/bundles/test_security_paths.py index 807650a5ce..c96cec8974 100644 --- a/tests/specify_cli/bundles/test_security_paths.py +++ b/tests/specify_cli/bundles/test_security_paths.py @@ -5,6 +5,7 @@ """ from __future__ import annotations +import json import os from pathlib import Path @@ -170,6 +171,124 @@ def test_active_integration_still_reads_legacy_alias(tmp_path: Path): assert active_integration(project) == "copilot" +@pytest.mark.parametrize( + "recorded,expected", + [ + ("copilot", "copilot"), + (" copilot ", "copilot"), # padded: previously returned verbatim + (" ", None), # whitespace-only: previously truthy + ("\t\n", None), + ("", None), + (None, None), + (5, None), + ], + ids=["plain", "padded", "spaces", "tabs", "empty", "null", "non_string"], +) +def test_active_integration_matches_the_canonical_key_reader( + tmp_path: Path, recorded, expected +): + """`active_integration` must normalize the way the canonical reader does. + + That reader runs every value through `clean_integration_key`, while this + one only checked `isinstance(value, str) and value`. A whitespace-only key + is truthy, so it was returned as a real integration *and* suppressed the + "not determinable" fallback; a padded key was returned verbatim and + matches no registered integration. + """ + from specify_cli.bundles.project import active_integration + + project = _write_marker(tmp_path, json.dumps({"default_integration": recorded})) + assert active_integration(project) == expected + + +@pytest.mark.parametrize( + "recorded,expected", + [ + ({"default_integration": " ", "integration": "copilot"}, "copilot"), + ({"default_integration": "\t\n", "integration": " copilot "}, "copilot"), + ({"default_integration": 5, "integration": "copilot"}, "copilot"), + ({"integration": " ", "id": "claude"}, "claude"), + ({"default_integration": " ", "integration": " "}, None), + ({"default_integration": "cursor", "integration": "copilot"}, "cursor"), + ], + ids=[ + "blank_default", + "blank_default_padded_legacy", + "non_string_default", + "blank_legacy_falls_to_id", + "all_blank", + "precedence_kept", + ], +) +def test_active_integration_cleans_each_candidate_before_selecting( + tmp_path: Path, recorded, expected +): + """Each candidate must be cleaned before selection, not just the winner. + + A raw `or` chain selects a whitespace-only `default_integration` (truthy) + and then normalizes it to None, losing the valid legacy key behind it -- + while `normalize_integration_state` does + `clean_integration_key(default) or legacy_key` and falls through. + """ + from specify_cli.bundles.project import active_integration + + project = _write_marker(tmp_path, json.dumps(recorded)) + assert active_integration(project) == expected + + +@pytest.mark.parametrize( + "recorded,expected", + [ + ({"installed_integrations": ["claude", "copilot"]}, "claude"), + ({"installed_integrations": [" ", " claude "]}, "claude"), + ({"installed_integrations": []}, None), + ({"installed_integrations": "claude"}, None), + ], + ids=["installed_only", "blank_first_entry", "empty_list", "non_list"], +) +def test_active_integration_resolves_installed_only_state( + tmp_path: Path, recorded, expected +): + """Installed-only state resolves exactly as the canonical reader does. + + With ``installed_integrations`` populated but no default recorded, + ``normalize_integration_state`` promotes the first installed key to the + default. ``active_integration`` returned None instead -- "cannot be + determined" -- which lets an explicit ``--integration`` bypass the FR-019 + integration-clash guard in ``bundle install`` / ``bundle update``. + """ + from specify_cli.bundles.project import active_integration + from specify_cli.integration_state import ( + default_integration_key, + normalize_integration_state, + ) + + project = _write_marker(tmp_path, json.dumps(recorded)) + assert active_integration(project) == expected + assert expected == default_integration_key(normalize_integration_state(recorded)) + + +@pytest.mark.parametrize( + "recorded,expected", + [ + ({"integration": "copilot", "installed_integrations": ["claude"]}, "copilot"), + ({"id": "cursor", "installed_integrations": ["claude"]}, "cursor"), + ], + ids=["recorded_default_wins", "legacy_id_still_wins"], +) +def test_active_integration_installed_fallback_is_checked_last( + tmp_path: Path, recorded, expected +): + """The installed-only fallback must not change any marker that already + resolved: it is consulted only after every recorded field, so it can turn + a None into a key but never replace a key this function already returned. + """ + from specify_cli.bundles.project import active_integration + + project = _write_marker(tmp_path, json.dumps(recorded)) + assert active_integration(project) == expected + + def test_read_catalog_config_refuses_symlinked_specify_escape(tmp_path: Path): from specify_cli.bundles import catalog_config as cc From e192c48093604a7a094508589c6e52663ff257b7 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Thu, 8 Oct 2026 00:39:59 +0500 Subject: [PATCH 2/2] docs(bundler): name the full canonical path active_integration matches The comment called `integration_state.default_integration_key` the canonical reader, but that helper expects normalized state: on a raw marker its `state.get("default_integration") or state.get("integration")` picks a whitespace-only default over a valid legacy key, and it ignores `installed_integrations`. What `active_integration` matches is `normalize_integration_state` followed by `default_integration_key`: raw dk(raw) dk(normalize(raw)) {"default_integration": " ", "integration": "copilot"} None 'copilot' {"installed_integrations": ["claude"]} None 'claude' Say so in the comment and in the two test docstrings that referred to "the canonical reader". Comment/docstring-only change. Co-Authored-By: Claude Opus 5.5 --- src/specify_cli/bundles/project.py | 29 +++++++++---------- .../bundles/test_security_paths.py | 9 ++++-- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/src/specify_cli/bundles/project.py b/src/specify_cli/bundles/project.py index eb96e3f973..6838adf3f4 100644 --- a/src/specify_cli/bundles/project.py +++ b/src/specify_cli/bundles/project.py @@ -83,22 +83,21 @@ def active_integration(project_root: Path) -> str | None: except BundlerError: return None if isinstance(data, dict): - # ``default_integration`` first, matching the canonical reader - # (``integration_state.default_integration_key``): - # ``state.get("default_integration") or state.get("integration")``. - # ``write_integration_json`` writes both keys, so a marker produced by - # the current CLI already resolved through the ``integration`` alias -- - # this is about which field is authoritative when they disagree, and - # about resolving a marker that carries only ``default_integration`` - # (hand-edited, or written by anything that follows the canonical - # reader's shape). ``integration``/``id``/``active`` stay as fallbacks. - # Clean EACH candidate before selecting it, rather than picking the - # first truthy raw value and normalizing only that one. A raw ``or`` - # chain selects a whitespace-only ``default_integration`` (truthy) and - # then normalizes it to ``None``, losing the valid legacy key behind - # it -- whereas ``normalize_integration_state`` does + # Resolve the way the canonical path does: normalize first + # (``normalize_integration_state``), then read the default + # (``default_integration_key``). ``default_integration_key`` alone is + # not that reader -- it expects *normalized* state, and on a raw marker + # its ``state.get("default_integration") or state.get("integration")`` + # picks a whitespace-only default (truthy) over a valid legacy key + # behind it. + # + # ``default_integration`` is authoritative; ``integration`` (the legacy + # alias ``write_integration_json`` also writes), then ``id``/``active``, + # are fallbacks. Clean EACH candidate before selecting it, as + # ``normalize_integration_state`` does with # ``clean_integration_key(data.get("default_integration")) or - # legacy_key`` and falls through: + # legacy_key``, rather than picking the first truthy raw value and + # normalizing only that one: # {"default_integration": " ", "integration": "copilot"} # raw-then-clean -> None canonical -> 'copilot' # diff --git a/tests/specify_cli/bundles/test_security_paths.py b/tests/specify_cli/bundles/test_security_paths.py index c96cec8974..9f250a9142 100644 --- a/tests/specify_cli/bundles/test_security_paths.py +++ b/tests/specify_cli/bundles/test_security_paths.py @@ -189,8 +189,10 @@ def test_active_integration_matches_the_canonical_key_reader( ): """`active_integration` must normalize the way the canonical reader does. - That reader runs every value through `clean_integration_key`, while this - one only checked `isinstance(value, str) and value`. A whitespace-only key + The canonical path -- `normalize_integration_state`, then + `default_integration_key` -- runs every candidate through + `clean_integration_key`, while this one only checked + `isinstance(value, str) and value`. A whitespace-only key is truthy, so it was returned as a real integration *and* suppressed the "not determinable" fallback; a padded key was returned verbatim and matches no registered integration. @@ -249,7 +251,8 @@ def test_active_integration_cleans_each_candidate_before_selecting( def test_active_integration_resolves_installed_only_state( tmp_path: Path, recorded, expected ): - """Installed-only state resolves exactly as the canonical reader does. + """Installed-only state resolves exactly as the canonical reader does + (``normalize_integration_state``, then ``default_integration_key``). With ``installed_integrations`` populated but no default recorded, ``normalize_integration_state`` promotes the first installed key to the