Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
55 changes: 47 additions & 8 deletions plugins/dso/scripts/dso_reconciler/applier.py
Original file line number Diff line number Diff line change
Expand Up @@ -753,7 +753,9 @@ def _normalize_adf_body(body: Any) -> str:
return str(body) if body is not None else ""


def _apply_inbound_create(mutation, *, client=None, repo_root=None) -> ApplyResult:
def _apply_inbound_create(
mutation, *, client=None, repo_root=None, binding_store=None
) -> ApplyResult:
"""Materialise a remote Jira issue as a local jira-* ticket.

Writes a CREATE event (title, ticket_type, priority, description, tags
Expand All @@ -770,6 +772,32 @@ def _apply_inbound_create(mutation, *, client=None, repo_root=None) -> ApplyResu
fields = payload.get("fields") or payload
jira_key = mutation.target
local_id = _jira_key_to_local_id(jira_key)

# Defense-in-depth inbound dedup (ticket 1577). The snapshot differ normally
# stands down for an already-bound issue by recognising its dso-id:<local_id>
# label in the fetched fields (bug 4354) — that is the primary production
# dedup. This guard covers the narrow transient where the snapshot predates
# the label write-back yet the binding already exists in bindings.json: the
# differ then mis-emits an inbound CREATE which would materialise a duplicate
# local ticket. If the target Jira key is already bound, record the mapping
# and skip materialisation. Cheap (local reverse-index lookup, no Jira GET).
if binding_store is not None:
bound_local_id = binding_store.get_local_id(jira_key)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 1/6

[critical] plugins/dso/scripts/dso_reconciler/applier.py:785 plugins/dso/scripts/dso_reconciler/applier.py:2606 (correctness)

Unverified binding_store interface assumption creates runtime AttributeError risk. At line 785, the code calls binding_store.get_local_id(jira_key) without verifying that binding_store has this method. Sonnet A's finding is valid: if binding_store is passed from apply() without interface validation, and lacks get_local_id (e.g., a partially initialized or wrong-type object), the code raises AttributeError at runtime. The production call paths at lines 2606 and 2777 pass binding_store from apply() without pre-validation. The test mocks with _FakeBindingStore, but production has no defensive check. This violates the defensive programming pattern established elsewhere in the codebase (e.g., signature introspection at line 1712). Verify binding_store is either (1) type-hinted and validated at apply() entry, or (2) guarded with hasattr() before method call.

Posted by dso_ci_review.runner; resolve this comment when addressed.

if bound_local_id:

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 2/6

[important] plugins/dso/scripts/dso_reconciler/applier.py:786 (correctness)

Fragile truthiness check on binding_store.get_local_id() return value. At line 786, `if bound_local_id:` skips dedup if the result is falsy. The test at line 520 returns truthy strings ('uuid-bound-7') or None, but the interface contract does not enforce this. If a production binding_store implementation returns '' (empty string), 0, False, or any other falsy-but-not-None value to indicate a bound key, the dedup guard silently fails and allows duplicate ticket materialization. This is a silent correctness bug — the binding exists but is not consulted. The fix is to explicitly check `is not None` rather than truthiness: `if bound_local_id is not None:`.

Posted by dso_ci_review.runner; resolve this comment when addressed.

if repo_root is None:
repo_root = Path(__file__).parents[4]

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 3/6

[important] plugins/dso/scripts/dso_reconciler/applier.py:788 plugins/dso/scripts/dso_reconciler/applier.py:791 (correctness)

Missing directory existence guard before _write_mapping_atomic(). At lines 788-791, if repo_root is None, it is inferred from __file__, and then mapping_path is constructed as repo_root / 'bridge_state' / 'mapping.json'. The code then calls _write_mapping_atomic() without verifying the 'bridge_state' directory exists. If the directory does not exist (first-run, post-deletion, or misconfiguration), _write_mapping_atomic() will fail with FileNotFoundError or similar, converting a success case (dedup skip) into a failure. The dedup should be resilient to missing directories — either create the directory first or handle the exception gracefully.

Posted by dso_ci_review.runner; resolve this comment when addressed.

mapping_path = repo_root / "bridge_state" / "mapping.json"
_write_mapping_atomic(mapping_path, bound_local_id, jira_key)
return ApplyResult(
mutation.direction,
mutation.action,
{
"local_id": bound_local_id,
"jira_key": jira_key,
"dedup_skipped": True,
},
)

issuetype = _extract_name(fields.get("issuetype"), "Task")
ticket_type = _JIRA_TYPE_MAP.get(issuetype, "task")

Expand Down Expand Up @@ -1649,7 +1677,7 @@ def __init__(self, batch_mutation: dict) -> None:
}


def _apply_typed(mutation, *, client=None, repo_root=None) -> ApplyResult:
def _apply_typed(mutation, *, client=None, repo_root=None, binding_store=None) -> ApplyResult:
"""Typed-mutation dispatch via _LEAVES.

Looks up (mutation.direction, mutation.action) in _LEAVES and invokes the
Expand Down Expand Up @@ -1681,17 +1709,24 @@ def _apply_typed(mutation, *, client=None, repo_root=None) -> ApplyResult:

try:
sig = _inspect.signature(handler)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 4/4

[important] plugins/dso/scripts/dso_reconciler/applier.py:1711 plugins/dso/scripts/dso_reconciler/applier.py:1712 plugins/dso/scripts/dso_reconciler/applier.py:1713 plugins/dso/scripts/dso_reconciler/applier.py:1714 plugins/dso/scripts/dso_reconciler/applier.py:1715 plugins/dso/scripts/dso_reconciler/applier.py:1716 plugins/dso/scripts/dso_reconciler/applier.py:1717 plugins/dso/scripts/dso_reconciler/applier.py:1718 plugins/dso/scripts/dso_reconciler/applier.py:1719 plugins/dso/scripts/dso_reconciler/applier.py:1720 (verification)

[Upward synthesis of Sonnet A finding on C-extension handler discovery contract] Sonnet A flagged (appropriately as `important`) that the C-extension fallback at applier.py:1713-1720 sets accepts_binding_store = False, creating a fragile contract where C-extension handlers cannot request binding_store even if they need it. The comment acknowledges this is intentional ('only leaves that explicitly declare it'), but this is architecturally fragile: if a future handler is added as a C-extension that needs binding_store support, the introspection will fail silently and the handler will receive None, bypassing the dedup guard. Sonnet A's recommendation to add defensive registration is sound. Additionally, the test suite has NO coverage of any handler that would benefit from binding_store—the test only exercises Python functions via _apply_typed. No test verifies that a real handler (e.g., a leaf function for inbound CREATE) can access binding_store correctly. The combination of a fragile discovery contract + zero test coverage of binding_store propagation to actual handlers creates a systemic risk: the dedup feature works in the test but could fail silently in production if a handler is ever added that the introspection cannot inspect.

Posted by dso_ci_review.runner; resolve this comment when addressed.

accepts_repo_root = "repo_root" in sig.parameters or any(
_has_var_kw = any(
p.kind is _inspect.Parameter.VAR_KEYWORD for p in sig.parameters.values()
)
accepts_repo_root = "repo_root" in sig.parameters or _has_var_kw
accepts_binding_store = "binding_store" in sig.parameters or _has_var_kw
except (TypeError, ValueError):
# Builtins / C-extensions don't expose signatures: fall back to passing
# repo_root (legacy behaviour).
# repo_root (legacy behaviour) but NOT binding_store (only leaves that
# explicitly declare it consume it — ticket 1577).
accepts_repo_root = True
accepts_binding_store = False

_leaf_kwargs: dict[str, Any] = {"client": client}
if accepts_repo_root:
return handler(mutation, client=client, repo_root=repo_root)
return handler(mutation, client=client)
_leaf_kwargs["repo_root"] = repo_root
if accepts_binding_store:
_leaf_kwargs["binding_store"] = binding_store
return handler(mutation, **_leaf_kwargs)


# Exit code signalling that the caller should reschedule this pass.
Expand Down Expand Up @@ -2568,7 +2603,9 @@ def apply(
and hasattr(mutations, "direction")
and hasattr(mutations, "action")
):
return _apply_typed(mutations, client=client, repo_root=repo_root)
return _apply_typed(
mutations, client=client, repo_root=repo_root, binding_store=binding_store
)

# Legacy batch path requires pass_id.
if pass_id is None:
Expand Down Expand Up @@ -2737,7 +2774,9 @@ def _record_suppression(local_id: str, jira_key: str) -> None:
for mut in inbound_typed:
if _is_suppressed(getattr(mut, "target", "")):
continue
result = _apply_typed(mut, client=client, repo_root=repo_root)
result = _apply_typed(
mut, client=client, repo_root=repo_root, binding_store=binding_store
)
result_payload = (
getattr(result, "payload", None) if result is not None else None
)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -119,7 +119,10 @@ trap restore_snapshot EXIT
LAST_RECONCILER_LOG=""
run_reconciler() {
local output
LAST_RECONCILER_LOG=$(mktemp -t recon-probe.XXXXXX.log)
# Template form (not `-t`): the -t flag has divergent macOS/GNU semantics
# and is prohibited by CLAUDE.md rule:mktemp-tmp. Production/orchestrator
# context, so a literal /tmp template is fine (no per-test TMPDIR contract).
LAST_RECONCILER_LOG=$(mktemp "/tmp/recon-probe.XXXXXX.log")
Comment on lines +122 to +125

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Quality: mktemp template has trailing suffix after X's (non-portable)

The replacement mktemp "recon-probe.XXXXXX.log" places the XXXXXX placeholder before a .log suffix. GNU coreutils mktemp accepts a suffix after the X run, but BSD/macOS mktemp requires the X's to be trailing and errors with "too few X's in template" otherwise. This is the same kind of macOS/GNU divergence the change set out to eliminate by dropping -t. The accompanying comment acknowledges this is acceptable because the probe runs only in a Linux orchestrator context, so impact is limited; flagging only so the portability caveat is explicit. If macOS execution is ever desired, use a trailing-X form such as mktemp recon-probe-XXXXXX (optionally renaming afterward to add .log).

Was this helpful? React with 👍 / 👎

output=$(cd "$RECONCILER_DIR" && python -m dso_reconciler "$@" 2>&1) || true
printf '%s\n' "$output" > "$LAST_RECONCILER_LOG"
echo "$output"
Expand Down
92 changes: 92 additions & 0 deletions tests/unit/dso_reconciler/test_inbound_leaf_bodies.py
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,7 @@
import importlib.util
import json
import sys
import types
from pathlib import Path
from unittest.mock import MagicMock

Expand Down Expand Up @@ -91,6 +92,38 @@ def _make_mutation(
)


def _patch_apply_deps(applier, monkeypatch):
"""Stub applier.apply()'s lazy module loaders for an offline, all-inbound run.

apply() calls ``_load_acli()`` unconditionally to construct the Jira client
(applier.py ~2716). The real acli-integration.py does
``from dso_reconciler.adf import text_to_adf`` at import time, which is
unresolvable under this file's spec_from_file_location loading scheme
(modules live under the ``plugins.dso.scripts.dso_reconciler`` package, not a
bare ``dso_reconciler`` package) — so the real load raises ModuleNotFoundError
before any inbound dispatch happens. Mirror test_e2e_dedup_pass: stub
``_load_acli`` (offline client) and ``_load_concurrency`` (no git in tmp_path).
"""
fake_acli = types.ModuleType("acli_integration_stub")
fake_acli.AcliClient = lambda **_: MagicMock() # type: ignore[attr-defined]
monkeypatch.setattr(applier, "_load_acli", lambda: fake_acli)

fake_conc = types.ModuleType("concurrency_stub")
fake_conc.snapshot_head = lambda _repo_root: "deadbeef" * 5 # type: ignore[attr-defined]

class _Result:
ok = True
event = None
value = None

def _rebase_retry(_repo_root, write_fn, **_kwargs):
write_fn()
return _Result()

fake_conc.rebase_retry = _rebase_retry # type: ignore[attr-defined]
monkeypatch.setattr(applier, "_load_concurrency", lambda: fake_conc)


def _event_files(tracker_dir: Path, local_id: str) -> list[Path]:
return sorted((tracker_dir / local_id).glob("*.json"))

Expand Down Expand Up @@ -377,6 +410,7 @@ def test_apply_honours_suppress_pair_drops_subsequent_inbound(
"""

monkeypatch.setattr(applier, "_file_conflict_bug_ticket", lambda *a, **k: "bug-1")

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 4/6

[important] tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:412 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:414 (verification)

Test test_inbound_create_dedups_against_binding_store does not call _patch_apply_deps(), creating risk of ModuleNotFoundError in CI. The test at line 414 calls applier._apply_typed() which dispatches to _apply_inbound_create as the handler. However, unlike test_apply_honours_suppress_pair_drops_subsequent_inbound (line 412-413), this test does not stub _load_acli and _load_concurrency. If _apply_inbound_create invokes any downstream function that depends on these lazy loaders (e.g., via _write_mapping_atomic or ticket-file operations), the test will fail in CI when the real acli_integration or concurrency modules cannot be imported under the spec_from_file_location scheme. The test passes on developer machines with different import paths but fails in the isolated CI environment. Add `_patch_apply_deps(applier, monkeypatch)` at line 413.

Posted by dso_ci_review.runner; resolve this comment when addressed.

_patch_apply_deps(applier, monkeypatch)

# Seed the ticket dir so updates would otherwise apply.
create = _make_mutation(
Expand Down Expand Up @@ -435,6 +469,7 @@ def test_apply_honours_suppress_pair_drops_subsequent_inbound_via_computed_form(
third match-arm, the later inbound update sneaks past.
"""
monkeypatch.setattr(applier, "_file_conflict_bug_ticket", lambda *a, **k: "bug-1")
_patch_apply_deps(applier, monkeypatch)

# Seed the ticket dir so an EDIT could otherwise be written.
create = _make_mutation(
Expand Down Expand Up @@ -477,6 +512,63 @@ def test_apply_honours_suppress_pair_drops_subsequent_inbound_via_computed_form(
assert "SHOULD-BE-DROPPED" not in titles

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 1/4

[critical] tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:512 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:526 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:554 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:566 (verification)

The test test_inbound_create_dedups_against_binding_store() will fail immediately on first execution due to missing fixture setup. At line 526 (applier._apply_typed call), the code path invokes _apply_inbound_create which calls _write_mapping_atomic(mapping_path, ...) at applier.py:788. The mapping_path is fixture_repo / 'bridge_state' / 'mapping.json'. The parent directory bridge_state/ does not exist in fixture_repo—it is never created by the fixture or the test setup. When _write_mapping_atomic attempts to write to a non-existent parent, it will raise FileNotFoundError. Similarly, the unbound-create regression check at line 554 calls applier._apply_typed(unbound, ...) which writes a ticket to .tickets-tracker/<local_id>/, but .tickets-tracker is never created by fixture_repo setup, causing another FileNotFoundError. The test cannot run to completion without pre-creating both directories.

Posted by dso_ci_review.runner; resolve this comment when addressed.



def test_inbound_create_dedups_against_binding_store(applier, mut_mod, fixture_repo):

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 2/4

[important] tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:515 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:540 (verification)

[Tautological test — cannot detect regressions in guard logic] The test_inbound_create_dedups_against_binding_store() test asserts that when passed a _FakeBindingStore that hardcodes get_local_id('DIG-77') = 'uuid-bound-7', the result echoes back 'uuid-bound-7' and 'dedup_skipped' = True. This assertion verifies the test setup, not the dedup guard. The test would pass identically even if the dedup guard code (lines 783–790 in applier.py) were completely removed or inverted, as long as the code echoes back the values the test fixture provides. The test cannot detect regressions such as: (1) binding_store.get_local_id() is never called, (2) the returned bound_local_id is ignored, (3) the guard is bypassed due to a logic error in the `if binding_store is not None:` or `if bound_local_id:` conditions. A proper behavioral test must verify the observable side-effect: that when a Jira key is bound, NO phantom local ticket is materialized in the filesystem (the assertion at lines 435–437 for the bound case IS correct; the problem is the redundant payload assertions at lines 540–542 couple to mock-defined values).

Posted by dso_ci_review.runner; resolve this comment when addressed.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 3/4

[important] tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:515 (verification)

The _FakeBindingStore class in the test only exercises the happy path (returns 'uuid-bound-7' for DIG-77, None for others). Missing edge-case coverage: (1) falsy-but-not-None return values (empty string '', 0, False)—the guard at applier.py:785 checks `if bound_local_id:`, so an empty-string or zero return from a malformed binding store would incorrectly skip the dedup and materialize a phantom ticket; the test does not verify this scenario is handled safely. (2) Exception cases—the test does not verify that if binding_store.get_local_id() raises an exception (KeyError, AttributeError, etc.), the exception is either caught and logged, or propagates correctly without corrupting state. These edge cases are unexercised, leaving the dedup guard's robustness unverified.

Posted by dso_ci_review.runner; resolve this comment when addressed.

"""ticket 1577: an inbound CREATE for a Jira key already bound in the binding
store is deduped — mapping recorded, no phantom local ticket materialised.

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 5/6

[important] tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:518 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:530 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:531 (verification)

Test exhibits tautological assertion pattern: it verifies that mock return values round-trip without validating applier logic. At lines 530-531, the test asserts `result.payload.get('dedup_skipped') is True` and `result.payload.get('local_id') == 'uuid-bound-7'` — both values come directly from _FakeBindingStore defined at line 518 (`return 'uuid-bound-7' if jira_key == 'DIG-77'`). The test verifies the mock works, not that the applier correctly uses it. If the applier code at lines 785-797 were broken (e.g., ignored binding_store result, inverted the condition, returned wrong payload field), this test would still pass because it only checks that the hardcoded mock values appear in output. The test needs to verify the behavioral contract independently: when a bound key is detected, dedup should occur. A better approach: assert on observable side effects that don't depend on the mock (e.g., no ticket directory created) rather than asserting the exact payload content the test itself set up.

Posted by dso_ci_review.runner; resolve this comment when addressed.

Covers the narrow transient the snapshot differ's 4354 label stand-down
cannot: the fetched snapshot predates the dso-id:<local_id> label write-back,
so the differ mis-emits an inbound CREATE, but bindings.json already records
the binding. The applier-level guard catches it.
"""

class _FakeBindingStore:
def get_local_id(self, jira_key):
return "uuid-bound-7" if jira_key == "DIG-77" else None

mutation = _make_mutation(
mut_mod,
direction=mut_mod.MutationDirection.inbound,
action=mut_mod.MutationAction.create,
target="DIG-77",
payload={"fields": {"summary": "should NOT materialise", "issuetype": "Task"}},
)
result = applier._apply_typed(
mutation, repo_root=fixture_repo, binding_store=_FakeBindingStore()
)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DSO llm-review — finding 6/6

[important] tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:539 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:542 tests/unit/dso_reconciler/test_inbound_leaf_bodies.py:547 (verification)

Test is implementation-detail coupled: assertions on file path and JSON structure detect refactoring, not behavioral regressions. Lines 539-543 assert the mapping.json file exists and contains `mapping.get('uuid-bound-7') == 'DIG-77'`. These assertions break if the applier refactors how the mapping is persisted (e.g., uses a database, changes JSON keys, defers the write, or uses a different location) even if the observable behavior (no duplicate ticket) remains correct. The test conflates two concerns: (1) the dedup guard works (no phantom ticket created), and (2) the mapping persistence implementation matches the current file-based approach. The test should focus on (1) — the behavioral contract — and allow (2) to vary with implementation. The assertions at lines 547-549 (no ticket directory exists) already verify the core behavior; the file-write assertions are scaffolding that couples the test to internal design choices.

Posted by dso_ci_review.runner; resolve this comment when addressed.

# 1. Result signals a dedup skip bound to the pre-existing local id.
assert result.payload.get("dedup_skipped") is True
assert result.payload.get("local_id") == "uuid-bound-7"

# 2. mapping.json records uuid-bound-7 -> DIG-77.
mapping_file = fixture_repo / "bridge_state" / "mapping.json"
assert mapping_file.exists(), "dedup guard must write mapping.json"
mapping = json.loads(mapping_file.read_text())
assert mapping.get("uuid-bound-7") == "DIG-77"

# 3. No phantom local ticket materialised under the jira-key-derived id.
assert not (fixture_repo / ".tickets-tracker" / "jira-dig-77").exists(), (
"must not materialise a phantom local ticket when already bound"
)

# Regression guard: an UNBOUND inbound create still materialises normally —
# the stand-down is scoped to bound keys.
unbound = _make_mutation(
mut_mod,
direction=mut_mod.MutationDirection.inbound,
action=mut_mod.MutationAction.create,
target="DIG-88",
payload={"fields": {"summary": "brand new", "issuetype": "Task"}},
)
applier._apply_typed(
unbound, repo_root=fixture_repo, binding_store=_FakeBindingStore()
)
assert (fixture_repo / ".tickets-tracker" / "jira-dig-88").exists(), (
"an unbound inbound create must still materialise a local ticket"
)


# ---------------------------------------------------------------------------
# 6. Payload shape tolerance (Defect 1)
# ---------------------------------------------------------------------------
Expand Down
Loading