Skip to content

fix(workflows): state the equal-priority tie-break in workflow resolve output - #4542

Open
jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/resolve-layers-tiebreak-label
Open

jawwad-ali wants to merge 1 commit into
github:mainfrom
jawwad-ali:fix/resolve-layers-tiebreak-label

Conversation

@jawwad-ali

Copy link
Copy Markdown
Contributor

Problem

workflow resolve prints the layer list under a bare header:

console.print("Layers (highest precedence first):")

collect_all_layers sorts by (priority, source) ascending, which puts the winning layer first only while priorities differ. On a tie the sort is alphabetical by source, while the merge gives the conflict to the last id — exactly as documented:

Equal-priority overlays are applied alphabetically by ID, with the last ID winning conflicts. — docs/reference/workflows.md:126

So on a tie the header states the opposite of the outcome printed directly beneath it, in the one command whose documented purpose is explaining which overlay contributed or overrode a step.

Reproduction on current main (c173bf1)

DIFFERENT priorities (alpha=5, beta=10)
   listed first : project:alpha
   actual winner: echo FROM-ALPHA
   header correct? YES

EQUAL priorities (both 10)
   listed first : project:alpha
   actual winner: echo FROM-BETA
   header correct? NO  <-- inverted

The full output contradicted itself two lines apart:

Layers (highest precedence first):
  • [project-overlay] project:alpha (priority=10)
  • [project-overlay] project:beta (priority=10)
Step attribution:
  • build: project:beta          <-- beta won, alpha was labelled highest

Fix — the label, not the order

The ordering is deliberate: test_workflow_resolve_equal_priority_layers_sort_by_source pins it, and its own comment acknowledges that the alphabetically-last layer is the one that wins. Reordering the list would fight a decision the maintainers made on purpose, so this spells the tie-break out instead:

Layers (highest precedence first; on equal priority the last ID wins):
  • [project-overlay] project:alpha (priority=10)
  • [project-overlay] project:beta (priority=10)
Step attribution:
  • build: project:beta

Now self-consistent, and the behaviour is untouched.

Verification

  • Fail-before / pass-after: 1 new-vs-baseline failure with the source reverted to upstream/main26 passed with the fix.
  • The new test asserts the header and the attribution together, so it pins the label against the real outcome rather than against a hard-coded string alone.
  • No existing test asserts the old header text (checked: git grep "highest precedence first" -- tests/ is empty), and test_workflow_resolve_equal_priority_layers_sort_by_source passes unchanged.
  • uvx ruff@0.15.0 check src tests → clean

No behaviour change — output text only.

Note on overlap: this touches overlays/_commands.py, as does my #4141, but a different function (workflow_resolve vs workflow_overlay_add). Happy to rebase whichever lands second.


Written with assistance from Claude Code. Bug found, reproduced, and verified by me on current main.

🤖 Generated with Claude Code

…ve' output

`workflow resolve` prints the layer list under a bare header:

    console.print("Layers (highest precedence first):")

`collect_all_layers` sorts by `(priority, source)` ascending, which puts the
winning layer first only while priorities DIFFER. On a tie the sort is
alphabetical by source, while the merge gives the conflict to the LAST id
(docs/reference/workflows.md:126: "Equal-priority overlays are applied
alphabetically by ID, with the last ID winning conflicts").

So for a tie the header stated the opposite of the outcome printed directly
beneath it, in the one command whose job is explaining which overlay won:

    DIFFERENT priorities (alpha=5, beta=10)
       listed first : project:alpha
       actual winner: echo FROM-ALPHA        header correct? YES

    EQUAL priorities (both 10)
       listed first : project:alpha
       actual winner: echo FROM-BETA         header correct? NO

The ordering itself is deliberate and pinned by
`test_workflow_resolve_equal_priority_layers_sort_by_source`, so this spells
the tie-break out rather than reordering the list. Output is now
self-consistent:

    Layers (highest precedence first; on equal priority the last ID wins):
      - [project-overlay] project:alpha (priority=10)
      - [project-overlay] project:beta (priority=10)
    Step attribution:
      - build: project:beta

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
@jawwad-ali
jawwad-ali requested a review from mnriem as a code owner September 11, 2026 18:01
@mnriem mnriem added author-over-cap Over the 3-open-PR cap or repetitive batch submissions — please consolidate triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review labels Sep 12, 2026
@mnriem
mnriem requested a balanced review from Copilot September 18, 2026 18:43

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🟢 Approval recommended

The output-only fix is focused and regression-tested; the remaining wording precision note is non-blocking.

Pull request overview

Clarifies equal-priority overlay resolution without changing behavior.

Changes:

  • Expands the layer-order header with the tie-break rule.
  • Adds regression coverage linking the header to actual attribution.
File summaries
File Description
tests/workflows/test_overlay_commands.py Tests equal-priority header and attribution consistency.
src/specify_cli/workflows/overlays/_commands.py Clarifies equal-priority resolution output.
Review details

Suppressed comments (1)

src/specify_cli/workflows/overlays/_commands.py:436

  • The tie-break is still underspecified here: “last ID” does not say that equal-priority IDs are applied alphabetically, and “wins” can imply that the last overlay replaces earlier overlays rather than only winning conflicting edits. Match the documented contract at docs/reference/workflows.md:126, for example: “on equal priority, IDs are applied alphabetically and the last ID wins conflicts,” and update the test assertion to match.
        "Layers (highest precedence first; on equal priority the last ID wins):"
  • Files reviewed: 2/2 changed files
  • Comments generated: 0
  • Review effort level: Balanced

💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

@mnriem mnriem left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please resolve conflicts

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

author-over-cap Over the 3-open-PR cap or repetitive batch submissions — please consolidate triage-nice-to-have Verdict: evidence-backed fix or greenlit feature — land after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants