staged: fix/reconciler-followups-20260606 -> staged-2c3b33832330-1780719283#690
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. 🗂️ Base branches to auto review (1)
Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
DSO-Review-Cycle: 1 pr_number=690 commit_sha=b3e20bd01e0934378b2b9e58dd1119305705fab9 findings_hash=c1031aa28e5af70a tuples=[["plugins/dso/scripts/dso_reconciler/applier.py", "", "verification"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "maintainability"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "verification"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "verification"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "verification"]] |
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| @@ -477,6 +512,63 @@ def test_apply_honours_suppress_pair_drops_subsequent_inbound_via_computed_form( | |||
| assert "SHOULD-BE-DROPPED" not in titles | |||
There was a problem hiding this comment.
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.
| assert "SHOULD-BE-DROPPED" not in titles | ||
|
|
||
|
|
||
| def test_inbound_create_dedups_against_binding_store(applier, mut_mod, fixture_repo): |
There was a problem hiding this comment.
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.
| assert "SHOULD-BE-DROPPED" not in titles | ||
|
|
||
|
|
||
| def test_inbound_create_dedups_against_binding_store(applier, mut_mod, fixture_repo): |
There was a problem hiding this comment.
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.
| @@ -1681,17 +1709,24 @@ def _apply_typed(mutation, *, client=None, repo_root=None) -> ApplyResult: | |||
|
|
|||
| try: | |||
| sig = _inspect.signature(handler) | |||
There was a problem hiding this comment.
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.
…apply() tests (549c)
test_apply_honours_suppress_pair_drops_subsequent_inbound{,_via_computed_form}
called applier.apply() without stubbing the lazy module loaders, so apply()
loaded the real acli-integration.py whose 'from dso_reconciler.adf import ...'
is unresolvable under this file's spec_from_file_location scheme (modules live
under plugins.dso.scripts.dso_reconciler, not a bare dso_reconciler package) —
raising ModuleNotFoundError before any inbound dispatch. Mirror test_e2e_dedup_pass:
add _patch_apply_deps() stubbing _load_acli (offline client) and _load_concurrency
(no git in tmp_path). The suppress_pair LOGIC was already correct — file now 24/24.
A test-harness gap, not the inbound-dispatch defect originally suspected.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…re (1577) The snapshot differ stands down for an already-bound issue by recognising its dso-id:<local_id> label (bug 4354) — the primary production dedup. This adds a cheap applier-level safety net for the narrow transient where the fetched snapshot predates the label write-back but bindings.json already holds the binding: the differ then mis-emits an inbound CREATE that would materialise a phantom local ticket. _apply_inbound_create now consults binding_store (reverse-index get_local_id(target), no Jira round-trip); on a hit it writes the mapping and returns dedup_skipped without materialising. binding_store is threaded through _apply_typed (both apply() call sites); leaves opt in by declaring the param, so other leaves are unaffected. RED->GREEN test added; inbound_leaf_bodies 25/25, differ/dedup/comment-bootstrap suites green. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…7af) e2e_field_validation_probe.sh run_reconciler used mktemp -t recon-probe.XXXXXX.log; the -t flag has divergent macOS/GNU semantics and is prohibited by CLAUDE.md rule:mktemp-tmp. Switch to the template form mktemp "/tmp/recon-probe.XXXXXX.log" (production/orchestrator context — no per-test TMPDIR isolation contract). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
![gitar-bot[bot]](https://avatars.githubusercontent.com/in/827041?s=60&v=4){kind=small}
| # 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") |
There was a problem hiding this comment.
💡 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 👍 / 👎
joeoakhart
force-pushed
the
fix/reconciler-followups-20260606
branch
from
b9f2b32 to
7a7f1fa
Compare
|
DSO-Review-Cycle: 1 pr_number=690 commit_sha=b52dc7d9f482178f2a70297f6b3eab0c4eca08a2 findings_hash=77f1b2182406dd49 tuples=[["plugins/dso/scripts/dso_reconciler/applier.py", "", "correctness"], ["plugins/dso/scripts/dso_reconciler/applier.py", "", "correctness"], ["plugins/dso/scripts/dso_reconciler/applier.py", "", "correctness"], ["plugins/dso/scripts/dso_reconciler/e2e_field_validation_probe.sh", "", "maintainability"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "verification"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "verification"], ["tests/unit/dso_reconciler/test_inbound_leaf_bodies.py", "", "verification"]] |
| # 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) |
There was a problem hiding this comment.
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.
| # 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) | ||
| if bound_local_id: |
There was a problem hiding this comment.
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.
| bound_local_id = binding_store.get_local_id(jira_key) | ||
| if bound_local_id: | ||
| if repo_root is None: | ||
| repo_root = Path(__file__).parents[4] |
There was a problem hiding this comment.
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.
| @@ -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") | |||
There was a problem hiding this comment.
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.
| def test_inbound_create_dedups_against_binding_store(applier, mut_mod, fixture_repo): | ||
| """ticket 1577: an inbound CREATE for a Jira key already bound in the binding | ||
| store is deduped — mapping recorded, no phantom local ticket materialised. | ||
|
|
There was a problem hiding this comment.
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.
| result = applier._apply_typed( | ||
| mutation, repo_root=fixture_repo, binding_store=_FakeBindingStore() | ||
| ) | ||
|
|
There was a problem hiding this comment.
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.
CI failed: The build failed because the automated LLM-based code review identified 1 critical and 5 important issues in the PR, triggering a blocking exit status.OverviewThe CI pipeline was explicitly terminated by the LLM-based review tool because it detected 1 critical and 5 important code quality issues. This is a policy-driven failure rather than an infrastructure or build execution error. FailuresLLM Review Policy Violation (confidence: high)
Summary
Code Review 👍 Approved with suggestions 0 resolved / 1 findingsAdds robust binding deduplication to the reconciler and improves test isolation with dependency stubbing. The mktemp template requires a suffix cleanup to ensure cross-platform compatibility. 💡 Quality: mktemp template has trailing suffix after X's (non-portable)📄 plugins/dso/scripts/dso_reconciler/e2e_field_validation_probe.sh:122-125 The replacement 🤖 Prompt for agentsTip Comment OptionsAuto-apply is off → Gitar will not commit updates to this branch. Comment with these commands to change:
Was this helpful? React with 👍 / 👎 | Gitar |
JoeOakhartNava
merged commit ca015ac
into
staged-2c3b33832330-1780719283
2 participants
Staged promotion (PR-C two-PR flow)
This is the first PR of a two-PR session→main promotion. The worktree branch
fix/reconciler-followups-20260606is being merged into the staged refstaged-2c3b33832330-1780719283, which was created fromorigin/mainHEAD.The sub-PR ruleset's
required_workflowsrule requiresreview-sub-prto run successfully on this PR before merge. Once this PR merges, the merge-to-main script will open PR2 fromstaged-2c3b33832330-1780719283→main.🤖 Generated by merge-to-main-pr.sh
Summary by Gitar
_apply_inbound_createto prevent duplicate ticket materialization when a binding already exists._apply_typedandapply()signatures to propagatebinding_storeto relevant handlers.mktemp -twith a literal/tmptemplate ine2e_field_validation_probe.shto resolve platform-specific command issues._patch_apply_depsintest_inbound_leaf_bodies.pyto stub external dependencies for offline testing.test_inbound_create_dedups_against_binding_storeto verify mapping creation and deduplication logic.This will update automatically on new commits.