[worktree-20260605-175523] fix(acli): explicit client-cred access in verify-created-issue (PR #685 review)

plugins/dso/.claude-plugin/plugin.json

@@ -1,6 +1,6 @@
 {
   "name": "dso",
-  "version": "1.17.139",
+  "version": "1.17.140",
   "description": "Workflow infrastructure plugin for Claude Code projects",
   "commands": "./commands/",
   "skills": "./skills/",

plugins/dso/scripts/acli-integration.py

@@ -419,21 +419,38 @@ def _verify_created_issue(
     stdout: str,
     *,
     acli_cmd: list[str] | None = None,
+    client: Any = None,
 ) -> dict[str, Any]:
     """Parse ACLI create output, verify the issue exists, and return it.
 
     Uses direct REST GET (immediately consistent) instead of JQL search,
     which is subject to Jira Cloud's eventual-consistency index lag.
+
+    Credentials for the REST GET come ONLY from the explicit *client*
+    (AcliClient), never from ``os.environ`` (bug 7689). Reading ambient env
+    here made create-path test behaviour depend on whatever JIRA_* variables
+    happened to be set in the developer/CI process — tests that mocked only
+    ``subprocess.run`` silently switched to the urllib REST path. With the
+    credential source pinned to the caller's client, behaviour is determined
+    solely by what the caller passes: a client carrying creds → REST GET
+    (production: ``AcliClient.create_issue`` forwards ``client=self``, whose
+    creds are read from the environment at construction); no client / no creds
+    → the deterministic subprocess ``get_issue`` path.
     """
     created = json.loads(stdout)
     jira_key = created.get("key", "")
     if not jira_key:
         msg = f"ACLI create returned no key: {created}"
         raise RuntimeError(msg)
 
-    jira_url = os.environ.get("JIRA_URL", "")
-    jira_user = os.environ.get("JIRA_USER", "")
-    jira_token = os.environ.get("JIRA_API_TOKEN", "")
+    # Credentials come from the explicit client (AcliClient always sets these
+    # three attributes in __init__). Access them directly rather than via
+    # getattr-with-default so a malformed client (not None but missing an
+    # attribute) fails loudly instead of silently degrading to the subprocess
+    # path with a half-populated credential set.
+    jira_url = client.jira_url if client is not None else ""
+    jira_user = client.user if client is not None else ""
+    jira_token = client.api_token if client is not None else ""
     if jira_url and jira_user and jira_token:
         path = f"/rest/api/3/issue/{jira_key}"
         url = f"{jira_url.rstrip('/')}{path}"
@@ -533,7 +550,13 @@ def create_issue(
     # additionalAttributes.priority (the only ACLI-supported path).
     if priority is not None:
         created = _create_issue_from_json(
-            project, issue_type, summary, priority, acli_cmd=acli_cmd, **kwargs
+            project,
+            issue_type,
+            summary,
+            priority,
+            acli_cmd=acli_cmd,
+            client=client,
+            **kwargs,
         )
         # --from-json has no inline parent attachment — set_parent fallback.
         if parent_key and client is not None:
@@ -567,7 +590,7 @@ def create_issue(
             raise RuntimeError(msg)
 
     assert result is not None  # Guaranteed: either we have a result or raised above
-    return _verify_created_issue(result.stdout, acli_cmd=acli_cmd)
+    return _verify_created_issue(result.stdout, acli_cmd=acli_cmd, client=client)
 
 
 def _create_issue_no_json(
@@ -667,6 +690,7 @@ def _create_issue_from_json(
     priority: str | int | dict[str, Any],
     *,
     acli_cmd: list[str] | None = None,
+    client: Any = None,
     **kwargs: Any,
 ) -> dict[str, Any]:
     """Create a Jira issue using ``--from-json`` to set priority.
@@ -744,7 +768,7 @@ def _create_issue_from_json(
             raise RuntimeError(msg)
 
     assert result is not None  # Guaranteed: either we have a result or raised above
-    return _verify_created_issue(result.stdout, acli_cmd=acli_cmd)
+    return _verify_created_issue(result.stdout, acli_cmd=acli_cmd, client=client)
 
 
 def transition_issue(

tests/scripts/test_acli_integration.py

@@ -142,6 +142,79 @@ def test_create_issue_calls_acli_subprocess(acli: ModuleType) -> None:
     assert result is not None, "create_issue must return a non-None result"
 
 
+# ---------------------------------------------------------------------------
+# Test 1b: _verify_created_issue must not branch on AMBIENT env credentials
+# (bug 7689). Verify behaviour is determined by the explicit client, never by
+# JIRA_URL/JIRA_USER/JIRA_API_TOKEN present in the process environment.
+# ---------------------------------------------------------------------------
+
+
+@pytest.mark.unit
+@pytest.mark.scripts
+def test_verify_created_issue_ignores_ambient_env_creds(
+    acli: ModuleType, monkeypatch: pytest.MonkeyPatch
+) -> None:
+    """No client → subprocess get_issue path, regardless of ambient JIRA_* env.
+
+    Regression for bug 7689: previously _verify_created_issue read JIRA_URL/
+    JIRA_USER/JIRA_API_TOKEN from os.environ and silently issued a urllib REST
+    GET when they were present — making create-path test behaviour depend on
+    ambient developer credentials. With no client supplied, verification must
+    use the deterministic subprocess get_issue path and never touch urllib.
+    """
+    monkeypatch.setenv("JIRA_URL", "https://ambient.example.com")
+    monkeypatch.setenv("JIRA_USER", "ambient@example.com")
+    monkeypatch.setenv("JIRA_API_TOKEN", "ambient-token-must-not-be-used")
+
+    created = json.dumps({"key": "PROJ-700"})
+    get_response = json.dumps(
+        [{"key": "PROJ-700", "summary": "Env-independent", "status": "To Do"}]
+    )
+    mock_get = MagicMock(returncode=0, stdout=get_response, stderr="")
+
+    def _fail_urlopen(*_a, **_k):
+        raise AssertionError(
+            "must not issue a urllib REST GET from ambient env creds (bug 7689)"
+        )
+
+    with (
+        patch("urllib.request.urlopen", side_effect=_fail_urlopen),
+        patch("subprocess.run", return_value=mock_get) as mock_run,
+    ):
+        result = acli._verify_created_issue(created)
+
+    assert mock_run.called, "must verify via subprocess get_issue when no client"
+    assert result.get("key") == "PROJ-700"
+
+
+@pytest.mark.unit
+@pytest.mark.scripts
+def test_verify_created_issue_uses_client_creds_for_rest(acli: ModuleType) -> None:
+    """A client carrying creds drives the immediately-consistent REST GET.
+
+    Preserves the production path (AcliClient.create_issue passes client=self,
+    whose creds come from the environment at construction) after bug 7689 moves
+    the credential source from os.environ to the explicit client.
+    """
+    client = acli.AcliClient(
+        jira_url="https://client.example.com",
+        user="client@example.com",
+        api_token="client-token",
+    )
+    created = json.dumps({"key": "PROJ-701"})
+
+    def _fail_subprocess(*_a, **_k):
+        raise AssertionError("must use client-cred REST GET, not subprocess")
+
+    with (
+        _mock_urlopen_verify("PROJ-701", summary="From client creds"),
+        patch("subprocess.run", side_effect=_fail_subprocess),
+    ):
+        result = acli._verify_created_issue(created, client=client)
+
+    assert result.get("key") == "PROJ-701"
+
+
 # ---------------------------------------------------------------------------
 # Test 2: update_issue routes status through transition_issue_by_name
 # ---------------------------------------------------------------------------

tests/unit/dso_reconciler/test_e2e_dedup_pass.py

@@ -8,7 +8,6 @@
 from __future__ import annotations
 
 import importlib.util
-import json
 import sys
 import types
 from pathlib import Path
@@ -140,16 +139,24 @@ def _fake_rebase_retry(_repo_root, write_fn, **_kwargs):
 
 
 def test_pre_existing_dso_id_produces_zero_creates(tmp_path, differ, applier):
-    """Pre-existing issue with dso-id label → create_issue never called.
-
-    Pass sequence:
-      1. differ.compute_mutations(prev={}, next={"uuid-X": ...}) → [create uuid-X]
-      2. applier.apply([create uuid-X], ...) with fake client that returns PROJ-999
-         on JQL search for dso-id:uuid-X
-      3. Assertions:
-         - create_issue call count == 0  (dedup guard fired)
-         - mapping.json["uuid-X"] == "PROJ-999"
-         - manifest events contains {"event": "dedup-create-skipped", "local_id": "uuid-X"}
+    """A Jira issue already carrying a dso-id:<local_id> label → zero creates (end-to-end).
+
+    Production semantics since commit 1f0032df24 / bug 4354: the fetcher
+    snapshot stores Jira ``fields`` only (never the ``dso_local_id`` entity
+    property), so the snapshot differ recognises an already-bound issue by its
+    ``dso-id:<local_id>`` / ``dso-id-<local_id>`` label and STANDS DOWN — the
+    issue is owned by the binding-aware inbound/outbound differs. No inbound
+    CREATE is emitted, so apply() never materialises a phantom ``jira-dig-NNNN``
+    local ticket and never writes a ghost ``dso-id:`` label back to Jira.
+
+    This replaces an obsolete pre-4354 contract that asserted an applier-level
+    dedup guard (mapping.json + a ``dedup-create-skipped`` manifest event). That
+    guard never existed on the inbound-create path, and is unnecessary: dedup of
+    already-bound issues is the differ's job, exercised here end-to-end
+    (differ → applier). The earlier failing assertion (``mapping.json must be
+    written by the dedup guard``) wired an *inbound* create yet asserted
+    *outbound* dedup artifacts; the ``get_comments`` AttributeError it cited was
+    a caught symptom, not the cause. Regression history: bugs a666/b38a/38fd/4cc1.
     """
     fake_client = FakeAcliClient()
     fake_concurrency = _make_fake_concurrency()
@@ -160,63 +167,63 @@ def test_pre_existing_dso_id_produces_zero_creates(tmp_path, differ, applier):
     # env-derived (jira_url, user, api_token) credentials.
     fake_acli_mod.AcliClient = lambda **_: fake_client  # type: ignore[attr-defined]
 
-    # Step 1 — differ: prev=empty, next=has uuid-X → must produce a create mutation
-    prev_snapshot: dict = {}
-    next_snapshot: dict = {"uuid-X": {"summary": "Test ticket", "status": "open"}}
-    mutations = differ.compute_mutations(prev_snapshot, next_snapshot)
-
-    # Bug 85a1: mutations are typed Mutation dataclasses, not dicts.
-    # Accept both shapes for backward compat with the differ contract.
     def _mut_action(m):
         a = getattr(m, "action", None)
         if a is not None:
             return getattr(a, "value", a)
         return m.get("action") if isinstance(m, dict) else None
 
     def _mut_key(m):
-        return getattr(m, "target", None) or (m.get("key") if isinstance(m, dict) else None)
+        return getattr(m, "target", None) or (
+            m.get("key") if isinstance(m, dict) else None
+        )
+
+    # Step 1 — differ: a bound Jira issue (carries a dso-id:<local_id> label)
+    # present in the Jira snapshot but absent from the local snapshot must NOT
+    # produce an inbound create — the 4354 label stand-down.
+    prev_snapshot: dict = {}
+    next_snapshot: dict = {
+        "DIG-999": {
+            "summary": "Already mirrored",
+            "status": "open",
+            "labels": ["dso-id:jira-dig-999"],
+        }
+    }
+    mutations = differ.compute_mutations(prev_snapshot, next_snapshot)
 
     create_mutations = [
-        m for m in mutations if _mut_action(m) == "create" and _mut_key(m) == "uuid-X"
+        m for m in mutations if _mut_action(m) == "create" and _mut_key(m) == "DIG-999"
     ]
-    assert create_mutations, (
-        f"differ.compute_mutations must emit a create mutation for uuid-X; got {mutations}"
+    assert not create_mutations, (
+        "differ must stand down for a label-bound issue (bug 4354); "
+        f"got a create mutation: {mutations}"
     )
 
-    # Wire local_id on dict-shape mutations only; Mutation dataclasses are
-    # immutable and carry their own local_id field already.
-    for m in mutations:
-        if isinstance(m, dict) and m.get("action") == "create" and "local_id" not in m:
-            m["local_id"] = m["key"]
-
-    # Step 2 — apply: patch _load_acli and _load_concurrency so applier uses
-    # our fakes (no real AcliClient, no git subprocess in tmp_path).
-    with patch.object(applier, "_load_acli", return_value=fake_acli_mod), \
-         patch.object(applier, "_load_concurrency", return_value=fake_concurrency):
-        manifest_path = applier.apply(mutations, "pass-001", repo_root=tmp_path)
-
-    # Step 3 — assertions
+    # Step 2 — apply the (create-free) mutation set: must be a no-op on Jira
+    # (no phantom create, no ghost label write-back).
+    with (
+        patch.object(applier, "_load_acli", return_value=fake_acli_mod),
+        patch.object(applier, "_load_concurrency", return_value=fake_concurrency),
+    ):
+        applier.apply(mutations, "pass-001", repo_root=tmp_path)
 
-    # 3a. create_issue must not be called (dedup guard intercepts)
     assert fake_client.creates == [], (
-        "create_issue must NOT be called when dedup guard fires on a pre-existing dso-id label"
-    )
-
-    # 3b. mapping.json must record uuid-X → PROJ-999
-    mapping_file = tmp_path / "bridge_state" / "mapping.json"
-    assert mapping_file.exists(), "mapping.json must be written by the dedup guard"
-    mapping = json.loads(mapping_file.read_text())
-    assert mapping.get("uuid-X") == "PROJ-999", (
-        f"mapping.json must record uuid-X → PROJ-999; got {mapping}"
+        "a label-bound issue must never trigger create_issue"
     )
 
-    # 3c. manifest events must include dedup-create-skipped for uuid-X
-    manifest = json.loads(manifest_path.read_text())
-    events = manifest.get("events", [])
-    dedup_events = [
-        e for e in events
-        if e.get("event") == "dedup-create-skipped" and e.get("local_id") == "uuid-X"
+    # Step 3 — regression guard: a genuinely-unbound Jira issue (no dso-id
+    # label) STILL produces an inbound create. The stand-down is scoped to
+    # bound issues; without this, 4354 would over-suppress legitimate creates.
+    unbound_snapshot: dict = {
+        "DIG-888": {"summary": "Brand new", "status": "open", "labels": []}
+    }
+    unbound_mutations = differ.compute_mutations({}, unbound_snapshot)
+    unbound_creates = [
+        m
+        for m in unbound_mutations
+        if _mut_action(m) == "create" and _mut_key(m) == "DIG-888"
     ]
-    assert dedup_events, (
-        f"manifest events must include dedup-create-skipped for uuid-X; got {events}"
+    assert unbound_creates, (
+        "a genuinely-unbound Jira issue must still produce an inbound create; "
+        f"got {unbound_mutations}"
     )

tests/unit/dso_reconciler/test_fetcher_dedup_observable.py

@@ -50,6 +50,32 @@ def fetcher():
     return _load_fetcher()
 
 
+@pytest.fixture(autouse=True)
+def _isolate_alert_store_module():
+    """Isolate the shared ``alert_store`` dotted key across tests (bug 4cc1).
+
+    ``test_bridge_alerts_surface.py`` registers its OWN module object under
+    ``plugins.dso.scripts.dso_reconciler.alert_store`` (via namespace stubs +
+    importlib). ``fetcher._load_alert_store()`` resolves that same dotted key at
+    call time. When the bridge-alerts test runs first, the leftover object in
+    ``sys.modules`` diverges from the one this test patches — silently defeating
+    the patch and producing an ORDER-DEPENDENT failure (passes alone, fails in
+    the full suite). Snapshot and clear the key around each test so loads are
+    fresh and no foreign object leaks in. The dedup test below additionally
+    patches the ``_load_alert_store`` seam directly, which is order-independent
+    on its own; this fixture protects every other test in the module too.
+    """
+    key = "plugins.dso.scripts.dso_reconciler.alert_store"
+    saved = sys.modules.pop(key, None)
+    try:
+        yield
+    finally:
+        if saved is not None:
+            sys.modules[key] = saved
+        else:
+            sys.modules.pop(key, None)
+
+
 class _DuplicatingPaginatingClient:
     """Stub ACLI client that returns DIG-100 on BOTH page 1 and page 2.
 
@@ -113,18 +139,24 @@ def test_dedup_suppression_emits_alert(tmp_path, fetcher):
     observability channel) with ``kind="fetcher-dedup-suppressed"`` and a
     structured reference to the duplicated key ``DIG-100``.
     """
-    from plugins.dso.scripts.dso_reconciler import alert_store
-
     mock_acli, _holder = _make_acli_mock()
 
     captured: list[dict] = []
 
     def _capture_append(record, repo_root):
         captured.append(record)
 
+    # Patch the _load_alert_store SEAM rather than
+    # `plugins.dso.scripts.dso_reconciler.alert_store.append` directly: the
+    # fetcher resolves alert_store via this helper at call time, and patching
+    # the seam is independent of which module object happens to occupy the
+    # shared sys.modules dotted key (the source of the order-dependent failure,
+    # bug 4cc1). The stub exposes the single attribute fetcher uses (`append`).
+    stub_alert_store = types.SimpleNamespace(append=_capture_append)
+
     with (
         patch.object(fetcher, "_load_acli", return_value=mock_acli),
-        patch.object(alert_store, "append", side_effect=_capture_append),
+        patch.object(fetcher, "_load_alert_store", return_value=stub_alert_store),
     ):
         snapshot_path = fetcher.fetch_snapshot(
             "2026-05-24-dedup-pass", repo_root=tmp_path