From 5ad338d3be3a092aed08804be4f86ec9a40b5ca0 Mon Sep 17 00:00:00 2001 From: jawwad-ali Date: Sat, 15 Aug 2026 21:10:10 +0500 Subject: [PATCH] fix(workflows): report overlay operation keys in declaration order `_parse_edit` collected the shorthand operation keys by iterating the frozenset: shorthand_keys = [key for key in _SHORTHAND_OPERATION_KEYS if key in edit_raw] `_SHORTHAND_OPERATION_KEYS` is `VALID_OPERATIONS`, a frozenset, so its iteration order depends on per-process string-hash randomization. The two error messages built from that list named the offending keys in a different order on every run for the exact same overlay file: ["Edit at index 0 has multiple operation keys: 'insert_after', 'remove'."] ["Edit at index 0 has multiple operation keys: 'remove', 'insert_after'."] ["Edit at index 0 has multiple operation keys: 'remove', 'insert_after'."] Iterating `edit_raw` instead yields the user's declared order and is deterministic. Dict keys are always hashable, so the membership test is safe in this direction too. Co-Authored-By: Claude Opus 5 (1M context) --- src/specify_cli/workflows/overlays/schema.py | 8 ++- tests/workflows/test_overlay_schema.py | 51 ++++++++++++++++++++ 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/src/specify_cli/workflows/overlays/schema.py b/src/specify_cli/workflows/overlays/schema.py index 0a018b7af0..969bbd94a9 100644 --- a/src/specify_cli/workflows/overlays/schema.py +++ b/src/specify_cli/workflows/overlays/schema.py @@ -60,7 +60,13 @@ def _validate_safe_id( def _parse_edit(edit_raw: dict[str, Any], idx: int) -> tuple[OverlayEdit | None, str | None]: """Parse a single edit dict into an OverlayEdit or an error string.""" - shorthand_keys = [key for key in _SHORTHAND_OPERATION_KEYS if key in edit_raw] + # Iterate ``edit_raw`` rather than ``_SHORTHAND_OPERATION_KEYS``: the latter + # is a frozenset, whose iteration order varies between processes with + # string-hash randomization, so the error messages built from this list + # named the offending keys in a different order on every run for the very + # same overlay file. Dict keys are always hashable, so the membership test + # is safe in this direction too. + shorthand_keys = [key for key in edit_raw if key in _SHORTHAND_OPERATION_KEYS] has_operation = "operation" in edit_raw operation: str | None = None diff --git a/tests/workflows/test_overlay_schema.py b/tests/workflows/test_overlay_schema.py index 77e0432eca..a91efb0a36 100644 --- a/tests/workflows/test_overlay_schema.py +++ b/tests/workflows/test_overlay_schema.py @@ -124,6 +124,57 @@ def test_multiple_operation_fields_rejected(self): assert overlay is None assert any("multiple" in e.lower() for e in errors), errors + @pytest.mark.parametrize( + "first,second", + [("remove", "insert_after"), ("insert_after", "remove")], + ids=["remove-first", "insert_after-first"], + ) + def test_multiple_operation_keys_reported_in_declaration_order( + self, first, second + ): + """The message must name the keys in the order the user wrote them. + + Collecting the keys by iterating the ``_SHORTHAND_OPERATION_KEYS`` + frozenset made the order depend on per-process string-hash + randomization, so the same overlay file produced a different message on + every run. Whatever fixed order a frozenset happens to have in a given + process, one of these two parametrizations contradicts it -- so this + pair fails deterministically without the fix, in every process. + """ + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "edits": [{first: "a", second: "a"}], + } + ) + assert overlay is None + assert errors == [ + f"Edit at index 0 has multiple operation keys: {first!r}, {second!r}." + ] + + @pytest.mark.parametrize( + "first,second", + [("remove", "insert_after"), ("insert_after", "remove")], + ids=["remove-first", "insert_after-first"], + ) + def test_shorthand_mixed_with_operation_names_first_declared_key( + self, first, second + ): + """``shorthand_keys[0]`` must be the first key the user declared.""" + overlay, errors = validate_overlay_yaml( + { + "id": "ov", + "extends": "wf", + "edits": [{first: "a", second: "a", "operation": "replace"}], + } + ) + assert overlay is None + assert errors == [ + f"Edit at index 0 mixes shorthand operation key ({first!r}) " + f"with explicit 'operation' field." + ] + def test_invalid_operation_field_rejected(self): overlay, errors = validate_overlay_yaml( {