Skip to content

fix(kanban): sort priority-desc from highest priority first - #29167

Open
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/kanban-sort-priority-desc-inversion
Open

fix(kanban): sort priority-desc from highest priority first#29167
Dusk1e wants to merge 1 commit into
NousResearch:mainfrom
Dusk1e:fix/kanban-sort-priority-desc-inversion

Conversation

@Dusk1e

@Dusk1e Dusk1e commented May 20, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes hermes kanban list --sort priority-desc so it sorts tasks by priority descending, matching the sort name and the default priority ordering.

Previously, priority-desc mapped to priority ASC, which put lower-priority tasks before higher-priority tasks.

Testing

  • uv run ruff check hermes_cli/kanban_db.py tests/hermes_cli/test_kanban_db.py
    • All checks passed!
  • uv run python -m pytest tests/hermes_cli/test_kanban_db.py::test_list_tasks_order_by tests/hermes_cli/test_kanban_db.py::test_list_tasks_order_by_priority_desc_sorts_highest_first -q -o addopts="" -p no:timeout
    • 2 passed in 1.45s

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cli CLI entry point, hermes_cli/, setup wizard labels May 20, 2026

@teknium1 teknium1 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.

Thanks for adding focused coverage. The current change reverses the established contract rather than fixing a defect.

Problems

  • hermes_cli/kanban_db.py:2716-2717 intentionally pairs priority (DESC, highest first) with priority-desc (ASC, lowest first). Changing line 2717 to DESC makes both flags identical.
  • The original sort feature explicitly documented this behavior: PR #25745 defines priority-desc as “Lowest priority first”; it was salvaged by a846e500b0948468fff9bc88edd87b8768c91d6f.

Suggested changes

  • Keep the existing priority-desc mapping and update the regression test to assert lowest-priority-first behavior.

Automated hermes-sweeper review.

Comment thread hermes_cli/kanban_db.py
@@ -1590,7 +1590,7 @@ def get_task(conn: sqlite3.Connection, task_id: str) -> Optional[Task]:
"created": "created_at ASC, id ASC",

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.

This makes priority-desc identical to priority. The original --sort contract defines priority as highest-first and priority-desc as lowest-first (PR #25745, salvaged by a846e50), so this mapping should remain priority ASC, created_at ASC.

@teknium1 teknium1 added sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform labels Jul 13, 2026

@GottZ GottZ left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

This was generated by AI during triage.

Summary

Four PRs address or reference the priority sorting behavior. #25745 introduced the sorting contract, #28427 merged its clean salvage with priority-desc meaning lowest priority first, while #29167 and #29514 both attempt to reverse that established behavior so it duplicates the normal priority order.

Related pull requests

  • #25745 [closed] related — (+113/-2) — superseded by merged #28427: introduced --sort and explicitly mapped priority-desc to ascending priority (lowest first), but its branch also contained unrelated image-generation changes; it remains relevant as the original documented contract and source of preserved authorship.
  • #28427 [merged] related — (+64/-1) — merged reference implementation: cleanly salvaged the kanban-only portion of #25745, preserving priority as highest-first and priority-desc as lowest-first with regression coverage and no new lint diagnostics.
  • #29167 related — (+13/-1) — revise before merge: the production diff changes priority-desc from ASC to DESC, making it identical to priority, and the new test codifies that duplication rather than the established lowest-first contract. Consistent with the keep_open review on #29167, retain the existing mapping and change the focused regression test to assert [low, mid, high].
  • #29514 [closed] duplicate — (+1/-1) — duplicate of #29167 and correctly closed: it makes the same contract-reversing mapping change without adding the regression test; it remains relevant as confirmation that no distinct fix is present.

Duplicates

#29167 and #29514 contain the same one-line production change; #29514 is the smaller, testless duplicate and is already closed.

Suggested consolidation

Merge #29167 only after revising it to keep priority-desc mapped to priority ASC and converting its focused test into regression coverage for lowest-priority-first ordering. This follows the contributor's keep_open review and the merged contract in #28427; keep #29514 closed as a duplicate, and leave #25745 closed as superseded by #28427.

Complex graph

flowchart LR
    classDef open fill:#dbeafe,stroke:#1d4ed8,color:#1e3a8a
    classDef merged fill:#dcfce7,stroke:#15803d,color:#14532d
    classDef closed fill:#e5e7eb,stroke:#6b7280,color:#1f2937
    classDef unverified fill:#f3f4f6,stroke:#9ca3af,color:#374151
    classDef best stroke-width:3px,stroke:#b45309
    classDef target stroke-width:3px,stroke:#4338ca
    subgraph Dup29167 ["PRs duplicating each other"]
        P29167["PR #29167 (open)"]
        P29514["PR #29514 (closed)"]
    end
    class P29167 open
    class P29514 closed
    class P29167 target
    click P29167 "https://github.com/NousResearch/hermes-agent/pull/29167"
    click P29514 "https://github.com/NousResearch/hermes-agent/pull/29514"
Loading

Graph: solid arrow = fixes / best fix, dashed arrow = partial or unverified (see edge label); boxed group = PRs duplicating each other; amber border = best fix; indigo border = target; gray node = closed or no verify verdict yet (state tag in the node label).

Cross-PR triage: Reviewed 4 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 18 kB of PR diffs, 3 kB of issue/PR text, 2 kB of discussion (5 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.

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

Labels

comp/cli CLI entry point, hermes_cli/, setup wizard P3 Low — cosmetic, nice to have sweeper:blast-moderate Sweeper blast radius: moderate — a subsystem or single platform sweeper:risk-compatibility Sweeper risk: may break existing users, config, migrations, defaults, or upgrades type/bug Something isn't working

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants