fix(kanban): honor all toolset aliases - #35588
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Changes
tools/kanban_tools.py: Gate check_profile_has_kanban_toolset()now treats"all"and"*"as kanban-capable in addition to"kanban"tests/tools/test_kanban_tools.py: Parametrized existing regression test across["kanban", "all", "*"]
Review
✅ Correctness
- Fix is minimal:
"kanban" in toolsets→any(name in {"kanban", "all", "*"} for name in toolsets) toolsets.resolve_toolset()documentsall/*as aliases for every registered toolset — this brings the Kanban gate in line with that contract- Only the
check_fnis changed; the actual tool registration logic is untouched
✅ Testing
- Parametrized the existing regression test across all 3 values — canonical approach
- 83 passed, 1 warning reported by the author (pre-existing)
✅ Code Quality
- 2-file change, 5 additions/4 deletions — extremely focused
- Set literal
{"kanban", "all", "*"}for O(1) lookup - Clear bug description: dispatcher-spawned workers masked the bug via
HERMES_KANBAN_TASK, but non-worker profiles using"all"/"*"silently lost Kanban tools
This only addresses the Kanban gate for top-level toolsets config. The PR description correctly scopes platform-specific/composite toolset filtering issues (like #35527) as out of scope.
Summary
Clean, targeted fix. Matches the Kanban tool gate to the documented all/* alias contract. Well-tested.
Reviewed by Hermes Agent (cron job)
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Review
A clean one-line fix for _profile_has_kanban_toolset() to honor all and * toolset aliases, with parametrized test coverage.
✅ Looks Good
- Correct fix: Uses
any(name in {"kanban", "all", "*"} for name in toolsets)instead of literal"kanban" in toolsets. - Parametrized test: The existing test is now parametrized across
kanban,all,*. - Minimal: 1 line production change, 1 test change.
Reviewed by Hermes Agent (cron job)
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Approved ✅
Reviewed Changes
- tools/kanban_tools.py —
_profile_has_kanban_toolset()now recognizes"all"and"*"as kanban-capable toolset aliases. - tests/tools/test_kanban_tools.py — Parameterized test to cover
"kanban","all", and"*"toolset names.
✅ Looks Good
- Correctness: Fixes issue #35581 — profiles using
toolsets: ["all"]ortoolsets: ["*"]now correctly expose kanban tools. The fix usesany(name in {"kanban", "all", "*"} for name in toolsets)which is straightforward and unambiguous. - Test coverage: The existing test is parameterized to validate all three alias variants. Good regression coverage.
- No security concerns: No secrets or injection vectors.
- Minimal change: +5/-4 lines across 2 files. Clean.
Reviewed by Hermes Agent
|
Closing this as superseded by #35729. That PR clarifies that the current kanban carve-out for all/* is intentional, so this code change would move behavior in the wrong direction. Happy to revisit if maintainers want the runtime behavior changed later. |
Summary
toolsets: ["all"]ortoolsets: ["*"]expose the same Kanban tools astoolsets: ["kanban"].Why
toolsets.resolve_toolset()documentsall/*as aliases for every registered toolset, buttools/kanban_tools.pyperformed a literal"kanban" in toolsetscheck after registry expansion.HERMES_KANBAN_TASK.Changes
tools/kanban_tools.py: treatskanban,all, and*as Kanban-capable profile toolset values in the Kanban gate.tests/tools/test_kanban_tools.py: parametrizes the orchestrator visibility regression test acrosskanban,all, and*.Validation
python -m pytest tests/tools/test_kanban_tools.py -q -o 'addopts='(83 passed, 1 warning)python -m ruff check tools/kanban_tools.py tests/tools/test_kanban_tools.pygit diff --checkScope
toolsetsconfig aliases.