Fix Kanban worker archived-skill detection and diagnostics - #42859
Fix Kanban worker archived-skill detection and diagnostics#42859sdgdllc wants to merge 2 commits into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review: Approved. Fixes two issues: 1) Kanban worker now ignores archived/hidden skill copies when checking availability. 2) Task-level max_retries is now properly honored in repeated_failures diagnostics. Clean fixes with good tests.
|
Thanks for the focused diagnostics coverage. One half of this PR needs updating before it can be salvaged onto current main. Problems
Suggested changes
Automated hermes-sweeper review. |
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Summary
Six PRs touch related Kanban dispatch and diagnostics symptoms, but they do not share a single root cause: #34578, #61035, and #65996 expose active-PR respawn guards; #62393 lets an explicit unblock bypass such guards once; #51386 distinguishes interactive claims from reclaimable workers; and #42859 combines obsolete skill-preload detection with task-level retry diagnostics.
Related pull requests
- #34578 [closed]
related— (+138/-0) — closed, rejected/superseded: Adds dispatch output and a dedicated warning diagnostic for active-PR guards, but its recorded closure decision says it was superseded by the distinct sticky blocked-task promotion fix ondeveloper/kanban-blocked-promotion-fix, not by #65996; it remains relevant as an earlier observability design reference. - #42859
related— (+59/-1) — salvage diagnostics only: The archived-skill fix no longer applies because current main removed automatickanban-workerpreloading, while the task-levelmax_retrieschange still addresses a real diagnostic threshold mismatch. Consistent with the keep_open review on #42859, retain only the retry work and make the rendered detail distinguish a task-level cap from the dispatcher circuit breaker. - #51386
related— (+352/-10) — keep open, incomplete: Introduces explicit non-expiring external claims to prevent live interactive sessions from being reclaimed and redispatched, which is distinct from respawn-guard observability. The keep_open review identifies a remaining correctness gap:detect_stale_runningcan still reclaim these holds, and the public--externalsyntax also needs documentation and reconciliation with current typed-block cleanup paths before merge. - #61035 [closed]
related— (+99/-9) — closed, wrong upstream: Exposesrespawn_guardedin JSON, CLI, daemon, and gateway logs, excludes guarded-only queues from stuck telemetry, and documents review handoffs. It remains relevant as a tested reference implementation, but it was explicitly abandoned because it targeted the wrong upstream. - #62393
related— (+307/-22) — keep open: Adds a one-shot, evidence-bound override so an explicit unblock permits exactly one guarded claim, then reapplies the guard after that claim, while also fixing guarded-queue health signals and exposing JSON reasons. This is a distinct control-flow fix rather than an observability duplicate; the documentation requested by the keep_open review was subsequently added in commit110120e6. - #65996
related— (+252/-32) — merge candidate: Provides the current-main active-PR observability fix by exposing dispatch results, replacing falsestranded_in_readyreports with a bounded info diagnostic, and supporting that severity in CLI and dashboard surfaces. This aligns with the keep_open review on #65996, which verified the affected current-main paths and event-order assumptions.
Duplicates
#61035 and #65996 substantially overlap on active-PR dispatch visibility, with #65996 being the current-main implementation and #61035 already closed as wrong-upstream. #34578 overlaps in observability but should not be relabeled as their duplicate because its recorded supersession was the separate sticky blocked-task promotion fix; #62393, #51386, and the retry-diagnostic portion of #42859 address distinct mechanisms.
Suggested consolidation
Merge #65996 for the current-main active-PR observability path; #61035 can remain closed as the wrong-upstream predecessor. Keep #62393 as a separate one-shot-unblock fix, and keep #51386 open until the detect_stale_running, documentation, and cleanup gaps from its contributor review are addressed. Do not merge #42859 as-is: drop the obsolete archived-skill half and salvage the task-level retry diagnostic with source-aware wording. Do not reopen #34578 or treat it as superseded by #65996; preserve its recorded rejection in favor of developer/kanban-blocked-promotion-fix.
Cross-PR triage: Reviewed 6 pull requests and 0 issues in this complex. Each diff was read against this issue; Assessment working set: 96 kB of PR diffs, 8 kB of issue/PR text, 7 kB of discussion (15 comments), 2 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
GottZ
left a comment
There was a problem hiding this comment.
This was generated by AI during triage.
Delta since our previous triage comment
@rosie-skypad reports that #62393 was rebased onto current main at head 13758e0, with the documentation follow-up retained and focused tests passing. This updates the branch-status evidence, but the delta includes no diff for the new head, so the reported retention of the one-shot override, five focused regressions, and exclusion of pruned legacy tests cannot yet be independently confirmed from the diff.
Changed pull requests
- #62393
related— (+314/-22) — updated head, pending diff verification: The author reports that the rebase resolved conflicts, retained the documented one-shot unblock override, and passed focused validation without restoring pruned tests; because no current-head diff is available in the delta, these remain author-reported rather than diff-verified. This does not conflict with the existing keep_open review, which treated the mechanism as salvageable and requested the documentation update that the author says remains included.
Suggested consolidation
The previous consolidation recommendation is unchanged: keep #62393 as a separate one-shot-unblock fix pending current-head diff verification.
Complex graph unchanged since our previous triage comment.
Cross-PR triage: Reviewed 6 pull requests and 0 issues in this complex. Diffs were read for 5 of 6 PRs (rest unavailable); Assessment working set: 75 kB of PR diffs, 8 kB of issue/PR text, 5 kB of discussion (12 comments), 0 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch.
Summary
kanban-workerskill copies when deciding whether to inject--skills kanban-workermax_retriescap is lower than the dispatcher defaultValidation
C:\Users\m_t_c\.hermes-sdgd\venv\Scripts\python.exe -m pytest -p no:timeout -o addopts='' tests\\hermes_cli\\test_kanban_cli_dispatch_passthrough.py -qC:\Users\m_t_c\.hermes-sdgd\venv\Scripts\python.exe -m pytest -p no:timeout -o addopts='' tests\\hermes_cli\\test_kanban_diagnostics.py -qContext
This was extracted from SDGD issue
SundayGundayLLC/sdgd#807, where Foreman Kanban worker pickups crashed because an archivedskills/.archive/kanban-worker/SKILL.mdmade Hermes preload a missing skill name.