fix: surface kanban completion evidence and validate skills - #28752
fix: surface kanban completion evidence and validate skills#28752jiayuxiaochaoren wants to merge 3 commits into
Conversation
|
The index migration fix in this PR overlaps with #28461, #28602, #28741, and #28754 (all fixing #28464). Note: this PR only moves the |
|
Thanks for the context — that matches what I saw locally. I’m happy to treat the index migration hunk in this PR as incidental/overlapping with #28461/#28602/#28741/#28754. If #28754 is the preferred migration fix since it covers all three optional-column indexes ( The parts I’d like to keep unique in this PR are:
I can also split those unique pieces into a smaller follow-up PR if that would be easier to review. |
teknium1
left a comment
There was a problem hiding this comment.
Thanks for the cleanup here. The unique parts are worth salvaging, but I found two correctness issues to fix before this should be carried forward.
Problems
plugins/kanban/dashboard/plugin_api.py:669-681buildscompletion_evidencefrom the samerunslist returned to the drawer. That list is already filtered byrun_state_type/run_state_name, so filtering the drawer to failed runs also makes the evidence rollup lose completed-run counts and latest completion data. Evidence should use an unfiltered all-runs query; only the visible run-history list should be filtered.hermes_cli/kanban_db.py:1426rejects skill names that contain/, but current main's loader accepts categorized skill paths viaskill_view()direct-path lookup (tools/skills_tool.py:869-870,tools/skills_tool.py:1020-1024). That means valid per-task skill identifiers likedevops/kanban-workerwould be rejected along with natural-language labels.
Suggested changes
- Keep the completion rollup, but compute it from all runs for the task and pass the filtered runs only to the existing
runsresponse field. - Validate per-task skills against the existing skill-loading grammar/resolution path, or at minimum allow categorized path identifiers while still rejecting whitespace/comma labels like
ppt skill. - Drop the index-migration hunk during salvage; current main already moved additive indexes after column migrations in
hermes_cli/kanban_db.py:1696-1709.
Automated hermes-sweeper review.
| ) | ||
| ], | ||
| "runs": [_run_dict(r) for r in runs], | ||
| "completion_evidence": _completion_evidence(conn, task, runs), |
There was a problem hiding this comment.
This passes the run-filtered runs list into the evidence rollup, so ?run_state_type=outcome&run_state_name=failed makes completion evidence report zero completed runs even if the task has completed attempts. Fetch an unfiltered run list for _completion_evidence and keep this filtered list only for the drawer's run history.
| f"skill name cannot contain comma: {name!r} " | ||
| f"(pass a list of separate names instead of a comma-joined string)" | ||
| ) | ||
| if not _VALID_TASK_SKILL_RE.match(name): |
There was a problem hiding this comment.
This rejects /, but the existing skill loader accepts categorized skill paths such as devops/kanban-worker through skill_view() direct-path lookup. The validation should match the loader's accepted identifier grammar, otherwise valid per-task skills get blocked.
Summary
tasks.session_idis missing by creating the session_id index after additive migrationsppt skillfail fast instead of causing worker startup crash loopsContext
A legacy Kanban board upgraded to the newer schema can fail loading the dashboard with:
The root cause is that
SCHEMA_SQLcreatedidx_tasks_session_idbefore_migrate_add_optional_columns()had a chance to addtasks.session_idto older DBs.A separate but related operational issue: if a task stores a natural-language per-task skill such as
ppt skill, workers are spawned with--skills "ppt skill"and exit before doing work with:The dashboard had the raw run history/logs, but completion evidence was not summarized above the fold, making it hard to tell whether dependency tasks completed and why the root task kept failing.
Test Plan
./venv/bin/python -m py_compile hermes_cli/kanban_db.py plugins/kanban/dashboard/plugin_api.py./venv/bin/python -m pytest tests/hermes_cli/test_kanban_db_init.py -q./venv/bin/python -m pytest tests/hermes_cli/test_kanban_db.py::test_session_id_index_exists tests/hermes_cli/test_kanban_db.py::test_create_task_rejects_natural_language_skill_names tests/hermes_cli/test_kanban_db.py::test_create_task_accepts_slug_like_skill_names -q./venv/bin/python -m pytest tests/plugins/test_kanban_dashboard_plugin.py::test_task_detail_completion_evidence_rolls_up_dependencies tests/plugins/test_kanban_dashboard_plugin.py::test_dashboard_bundle_renders_completion_evidence_section -qNote: I also ran the broader Kanban DB/dashboard plugin tests locally. Three unrelated home-channel tests failed because my local real
~/.hermeshas a Weixin home channel configured, which leaks into those tests' expectations.