fix(bundler): resolve the active integration like the canonical reader (#4541)

* 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) <noreply@anthropic.com>

* 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 <noreply@anthropic.com>

---------

Co-authored-by: Claude Opus 5.5 (1M context) <noreply@anthropic.com>

Author: jawwad-ali

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,21 +83,44 @@ 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):
-        # ``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.
-        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
+        # 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``, rather than picking the first truthy raw value and
+        # normalizing only that one:
+        #     {"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

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,127 @@ 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.
+
+    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.
+    """
+    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
+    (``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
+    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