From 3c5e1b2bcd38df818137e12f64a2a19c560832a8 Mon Sep 17 00:00:00 2001 From: Tobias Fischer Date: Thu, 10 Sep 2026 08:49:13 +1000 Subject: [PATCH] fix: reject duplicate if-block conditions within the same list key Two "- if:" blocks for the same condition under one list key (e.g. two separate "if: win" blocks in packages_select_by_deps) are never wrong on their own, but they're a standing foot-gun downstream: a future addition can land in the "wrong" one by accident, nothing merges the two, and the same package can end up skipped from one angle and not the other depending on which block someone edits later. This showed up for real across all three of RoboStack's humble/jazzy/ rolling vinca.yaml files -- humble alone had accumulated 4 separate duplicate-condition groups across ~19 blocks (mostly "if: not win" and "if: linux") from being extended incrementally across many sessions, none of which the previous adjacent-block fix (#152) would have caught since they weren't adjacent. Raises DuplicateConditionError (exit 1, clean message naming the list key and condition) rather than silently merging the blocks -- merging correctly needs a human to reconcile any per-item comments, since sorting the merged then: list can separate a standalone comment from the item it was meant to describe (the exact hazard #152 fixed for adjacent blocks, but which still applies to a comment sitting above an item inside either of the two blocks being merged by hand). Co-Authored-By: Claude Sonnet 5 --- vinca/sort_vinca_lists.py | 58 +++++++++++++++++++++++++----- vinca/test_sort_vinca_lists.py | 65 +++++++++++++++++++++++++++++++++- 2 files changed, 114 insertions(+), 9 deletions(-) diff --git a/vinca/sort_vinca_lists.py b/vinca/sort_vinca_lists.py index 2db7d4e..8de8dc5 100644 --- a/vinca/sort_vinca_lists.py +++ b/vinca/sort_vinca_lists.py @@ -5,6 +5,15 @@ Conditional blocks (`- if: ... then: [...]`) stay at the end; their inner `then:` lists are also sorted. +Also validates that each condition (e.g. "win", "not win", "linux") appears +in at most one `- if:` block per top-level list key. Two separate blocks for +the same condition are never wrong on their own, but they're a standing +foot-gun: a future addition can land in the "wrong" one by accident, and +nothing merges the two, so the same package can end up skipped from one +angle and not the other depending on which block someone edits. Raises +DuplicateConditionError (exit 1) rather than silently merging them, since +merging by hand needs a human to reconcile any per-item comments correctly. + Usage: vinca-sort-vinca-lists [FILE] vinca-sort-vinca-lists --check [FILE] @@ -27,12 +36,19 @@ RE_SIMPLE_ITEM = re.compile(r"^ - (\S.*)$") # Regex for the start of a conditional block: " - if: ..." RE_IF_BLOCK = re.compile(r"^ - if:") +# Regex capturing the condition text of a " - if: " line +RE_IF_CONDITION = re.compile(r"^ - if:\s*(.+?)\s*$") # Regex for a then-list item inside a conditional block: " - value" RE_THEN_ITEM = re.compile(r"^ - (\S.*)$") # Regex for a top-level key RE_TOP_KEY = re.compile(r"^(\S+):") +class DuplicateConditionError(ValueError): + """Raised when the same `- if:` condition appears in more than one block + under the same top-level list key.""" + + def _sort_key(line: str) -> str: """Extract sortable value from a list item line (lowercase, ignore comments).""" m = RE_SIMPLE_ITEM.match(line) or RE_THEN_ITEM.match(line) @@ -57,6 +73,7 @@ def sort_vinca_lists(path: Path) -> bool: # Check if this line starts a target list key m = RE_TOP_KEY.match(line) if m and m.group(1) in LISTS_TO_SORT: + list_key = m.group(1) result.append(line) i += 1 @@ -136,6 +153,25 @@ def sort_vinca_lists(path: Path) -> bool: if current_if_block is not None: if_blocks.append(current_if_block) + # Reject duplicate conditions: two separate "- if:" blocks for the + # same condition under the same list key. See module docstring. + seen_conditions = {} + for block in if_blocks: + cond_match = RE_IF_CONDITION.match(block[0]) + if not cond_match: + continue + condition = cond_match.group(1) + if condition in seen_conditions: + raise DuplicateConditionError( + f"{list_key}: condition {condition!r} appears in more " + f"than one '- if:' block. Merge them into a single " + f"block (each item may need its own comment moved " + f"inline first, since sorting the merged then: list " + f"can otherwise separate a standalone comment from " + f"the item it was meant to describe)." + ) + seen_conditions[condition] = True + # Sort simple items sorted_simple = sorted(simple_items, key=_sort_key) if sorted_simple != simple_items: @@ -221,14 +257,20 @@ def main(): print(f"ERROR: {args.file} not found", file=sys.stderr) sys.exit(1) - if args.check: - with tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) as tf: - tmp = Path(tf.name) - shutil.copy2(args.file, tmp) - changed = sort_vinca_lists(tmp) - tmp.unlink(missing_ok=True) - else: - changed = sort_vinca_lists(args.file) + try: + if args.check: + with tempfile.NamedTemporaryFile(suffix=".yaml", delete=False) as tf: + tmp = Path(tf.name) + shutil.copy2(args.file, tmp) + try: + changed = sort_vinca_lists(tmp) + finally: + tmp.unlink(missing_ok=True) + else: + changed = sort_vinca_lists(args.file) + except DuplicateConditionError as e: + print(f"ERROR: {args.file}: {e}", file=sys.stderr) + sys.exit(1) status = ("UNSORTED" if args.check else "SORTED") if changed else "OK" print(f"{status}: {args.file}") diff --git a/vinca/test_sort_vinca_lists.py b/vinca/test_sort_vinca_lists.py index 44777c6..3febd5a 100644 --- a/vinca/test_sort_vinca_lists.py +++ b/vinca/test_sort_vinca_lists.py @@ -1,6 +1,8 @@ """Tests for the vinca.yaml list sorter.""" -from vinca.sort_vinca_lists import sort_vinca_lists +import pytest + +from vinca.sort_vinca_lists import DuplicateConditionError, sort_vinca_lists BASE = """packages_select_by_deps: - alpha @@ -96,3 +98,64 @@ def test_adjacent_if_blocks_separated_by_comment_stay_isolated(tmp_path): first_block, second_block = path.read_text().split("- if: linux and not aarch64") assert "- webots_ros2" not in first_block assert "- webots_ros2" in second_block + + +def test_duplicate_condition_in_same_list_raises(tmp_path): + # Two non-adjacent "- if: win" blocks under the same list key, separated + # by unrelated content -- not the adjacency bug from the previous fix, + # just two blocks that should have been one all along. + content = """packages_select_by_deps: + - if: win + then: + - alpha + + - unrelated_package + + - if: win + then: + - bravo +""" + path = tmp_path / "vinca.yaml" + path.write_text(content) + + with pytest.raises( + DuplicateConditionError, match=r"packages_select_by_deps.*'win'" + ): + sort_vinca_lists(path) + + +def test_duplicate_condition_scoped_per_list_key(tmp_path): + # The same condition appearing once in each of two different list keys + # is fine -- duplication is only a problem within a single list key. + content = """packages_skip_by_deps: + - if: win + then: + - alpha + +packages_select_by_deps: + - if: win + then: + - bravo +""" + path = tmp_path / "vinca.yaml" + path.write_text(content) + + # Should not raise. + sort_vinca_lists(path) + + +def test_different_conditions_do_not_raise(tmp_path): + content = """packages_select_by_deps: + - if: win + then: + - alpha + + - if: not win + then: + - bravo +""" + path = tmp_path / "vinca.yaml" + path.write_text(content) + + # Should not raise. + sort_vinca_lists(path)