fix(kanban): make diagnostics --severity an "at or above" threshold#26384
Closed
briandevans wants to merge 2 commits into
Closed
fix(kanban): make diagnostics --severity an "at or above" threshold#26384briandevans wants to merge 2 commits into
diagnostics --severity an "at or above" threshold#26384briandevans wants to merge 2 commits into
Conversation
The `--severity` flag for `hermes kanban diagnostics` is documented (and
its argparse help text says) "Only show diagnostics at or above this
severity", but the filter was implemented as an exact-match equality:
kept = [d for d in diags_by_task[tid] if d.severity == sev]
This silently dropped higher-severity diagnostics, so `--severity warning`
returned 0 diagnostics on a board with two `error`-severity findings.
Operators relying on `--severity warning` as a catch-all watchdog threshold
got false-confidence silent runs while real `error`/`critical` issues went
unsurfaced.
Fix: compare via `kanban_diagnostics.SEVERITY_ORDER` index so a request at
level X also keeps diagnostics with severity > X. `argparse(choices=...)`
already constrains the flag to a valid level, and the `severity in
SEVERITY_ORDER` guard tolerates any future unknown rule-produced severity
without crashing the filter.
Regression test: parametrized over no-filter / warning / error / critical,
asserting the kept set matches the threshold semantics. Also verifies
tasks whose diagnostics are all below threshold are dropped from output
(unchanged behavior). With the old exact-match code, the `warning` and
`error` cases fail; the new threshold makes them pass.
Fixes NousResearch#26379
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Fixes a bug in /kanban diagnostics --severity where the filter performed exact-match instead of the documented "at or above" threshold semantics.
Changes:
- Replace equality check with index-based threshold comparison against
kd.SEVERITY_ORDER. - Add parametrized regression tests covering threshold behavior across severity levels.
- Add test verifying tasks with no matching diagnostics are dropped.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| hermes_cli/kanban.py | Switches severity filter from exact equality to threshold using SEVERITY_ORDER.index. |
| tests/hermes_cli/test_kanban_cli.py | Adds regression tests for threshold filtering behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+1410
to
+1414
| kept = [ | ||
| d for d in diags_by_task[tid] | ||
| if d.severity in kd.SEVERITY_ORDER | ||
| and kd.SEVERITY_ORDER.index(d.severity) >= threshold | ||
| ] |
Comment on lines
+457
to
+459
| if not expected_severities: | ||
| assert payload == [] | ||
| return |
Comment on lines
+420
to
+421
| import re | ||
| tid = re.search(r"(t_[a-f0-9]+)", out).group(1) |
Comment on lines
+472
to
+473
| import re | ||
| tid = re.search(r"(t_[a-f0-9]+)", out).group(1) |
19 tasks
- Precompute a {severity: rank} dict once instead of repeated
O(n) `kd.SEVERITY_ORDER.index()` calls per diagnostic; use
`severity_rank.get(d.severity, -1)` so unknown severities are
handled explicitly rather than via membership pre-check.
- Move `import re` and `from hermes_cli import kanban_diagnostics as kd`
to the top of the test module instead of inline inside helpers.
- Remove the unreachable `if not expected_severities` branch in
`test_diagnostics_severity_filter_is_threshold_not_exact_match` —
no parametrized case has an empty `expected_severities` set, and
the empty-set behavior is already covered by
`test_diagnostics_severity_filter_drops_task_when_no_match`.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Contributor
Author
|
@copilot All findings addressed in commit 08d09c1:
|
Contributor
Author
|
CI audit — all 7 test failures are pre-existing baselines on clean
All seven reproduce on clean |
Contributor
Author
|
Closing — superseded by @shellybotmoyer's #26661, which landed as 1eadb06 on 2026-05-17 with the same documented "at or above" semantics fix. Their fix is in main; mine is no longer needed. Thanks! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
hermes kanban diagnostics --severity Xis documented as "at or above X" but the filter was implemented as an exact-match equality, silently dropping higher-severity diagnostics. Replace with aSEVERITY_ORDER-index threshold comparison so the implementation matches the documented (and intuitive) semantics.Fixes #26379.
The bug
hermes_cli/kanban.py:1406(before):Argparse help text (
hermes_cli/kanban.py:360) and the issue reporter's expectation:Live reproduction from the issue, on a board with two
error-severity diagnostics:--severity warning--severity error--severity criticalwarningreturned 0 — the silent failure mode. Watchdog cron scripts that use--severity warningas a catch-all gate get false-confidence runs while realerror/criticalissues go undetected.The fix
hermes_cli/kanban_diagnostics.pyalready defines the ordering and uses it for sort/highest:The filter now uses the same constant to compute a threshold index.
argparse(choices=["warning", "error", "critical"])already constrains the flag, and the innerd.severity in SEVERITY_ORDERguard tolerates any future rule-produced severity that isn't in the canonical tuple without crashing the filter (same pattern used byseverity_of_highest()).Test plan
tests/hermes_cli/test_kanban_cli.py::test_diagnostics_severity_filter_is_threshold_not_exact_match— parametrized over(no filter | warning | error | critical), asserts kept severities match the threshold semantics.test_diagnostics_severity_filter_drops_task_when_no_match— task whose only diagnostic is below the requested threshold is dropped from output (preserves the existing "no match → drop task" behavior).--severity warningand--severity errorcases fail (because they expect higher-severity diags to be included). With the threshold fix, all 5 pass.tests/hermes_cli/test_kanban_cli.py+tests/hermes_cli/test_kanban_diagnostics.py(73 tests). 72 pass; 1 unrelated failure (test_run_slash_missing_required_arg_friendly_error) reproduces on cleanorigin/mainand is not in the touched code.Notes for reviewers
Single-file production change (~10 lines including a why-comment) plus regression tests. The
severity in SEVERITY_ORDERguard inside the comprehension is intentional defense — rule authors could in principle ship aDiagnostic(severity="info")and that should silently skip a--severity warningthreshold rather than ValueError-crash the CLI.