-
Notifications
You must be signed in to change notification settings - Fork 0
fix(kanban): reject unknown assignee at create time (no silent dispatcher drop) #49
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8520,6 +8520,51 @@ def list_profiles_on_disk() -> list[str]: | |
| return sorted(names) | ||
|
|
||
|
|
||
| # Assignee values that mean "intentionally unassigned" rather than naming a | ||
| # profile. Mirrors the tool surface's ``_normalize_profile`` (none/-/null) so a | ||
| # deliberately-unassigned or triage card stays legal. Matched case-folded. | ||
| _UNASSIGNED_SENTINELS = frozenset({"none", "-", "null"}) | ||
|
|
||
|
|
||
| def validate_assignee(assignee: Optional[str]) -> Optional[str]: | ||
| """Validate a create-time assignee against the known profile set. | ||
|
|
||
| Returns an error string when ``assignee`` names a lane that does not | ||
| exist (a typo'd or never-built profile), or ``None`` when it is valid. | ||
|
|
||
| The dispatcher only spawns a ready task whose assignee is a real profile | ||
| on disk; an unknown assignee is accepted onto the board and then silently | ||
| never spawns. Validating here makes a bad route fail loud at create time | ||
| so nobody has to pre-probe the profile list. | ||
|
|
||
| "Unassigned" is valid: ``None``, empty, and the ``none``/``-``/``null`` | ||
| sentinels all mean "no owner yet" (a triage/unassigned card) and pass. | ||
| Only a *named* assignee that is neither a known profile nor a sentinel | ||
| is rejected. Reuses :func:`list_profiles_on_disk` — no new enumeration. | ||
| """ | ||
| if assignee is None: | ||
| return None | ||
| text = str(assignee).strip() | ||
| if not text or text.casefold() in _UNASSIGNED_SENTINELS: | ||
| return None | ||
|
Comment on lines
+8548
to
+8549
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This new validator accepts and the error text recommends Useful? React with 👍 / 👎. |
||
| try: | ||
| from hermes_cli.profiles import normalize_profile_name | ||
|
|
||
| canon = normalize_profile_name(text) | ||
| except Exception: | ||
| canon = text | ||
| known = set(list_profiles_on_disk()) | ||
| if canon in known: | ||
|
Comment on lines
+8550
to
+8557
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To ensure robust and defensive validation, we should address two potential issues here:
try:
raw_known = list_profiles_on_disk()
except Exception:
raw_known = []
try:
from hermes_cli.profiles import normalize_profile_name
canon = normalize_profile_name(text)
known = {normalize_profile_name(p) for p in raw_known}
except Exception:
canon = text
known = set(raw_known)
if canon in known: |
||
| return None | ||
|
Comment on lines
+8556
to
+8558
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
This makes Useful? React with 👍 / 👎. |
||
| valid = ", ".join(sorted(known)) if known else "(none found on disk)" | ||
| return ( | ||
| f"assignee {assignee!r} is not a known profile — the dispatcher only " | ||
| f"spawns tasks whose assignee names a profile under " | ||
| f"~/.hermes/profiles/, so this card would be accepted but never run. " | ||
| f"Valid profiles: {valid}. (Use 'none' to create an unassigned card.)" | ||
| ) | ||
|
|
||
|
|
||
| def known_assignees(conn: sqlite3.Connection) -> list[dict]: | ||
| """Return every assignee name known to the board or on disk. | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,142 @@ | ||
| """Tests for create-time assignee validation (issue: silent dispatcher drop). | ||
|
|
||
| ``kanban_create`` (both the model tool and the ``hermes kanban create`` CLI) | ||
| now rejects an assignee that names a lane which doesn't exist, instead of | ||
| accepting a card the dispatcher will then silently never spawn. The check is a | ||
| single shared validator, ``kanban_db.validate_assignee``, so the two surfaces | ||
| can't drift. | ||
|
|
||
| Covers: | ||
| - validate_assignee() directly: real profile / unknown / sentinels / default. | ||
| - tool _handle_create: real assignee OK, unknown rejected (no row created), | ||
| 'none' sentinel OK. | ||
| - CLI _cmd_create / run_slash: unknown --assignee fails loud (no row created). | ||
| """ | ||
| from __future__ import annotations | ||
|
|
||
| import json | ||
| from pathlib import Path | ||
|
|
||
| import pytest | ||
|
|
||
| from hermes_cli import kanban as kc | ||
| from hermes_cli import kanban_db as kb | ||
|
|
||
|
|
||
| def _seed_profiles(home: Path, *names: str) -> None: | ||
| for name in names: | ||
| pdir = home / "profiles" / name | ||
| pdir.mkdir(parents=True, exist_ok=True) | ||
| (pdir / "config.yaml").write_text("model: {}\n") | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def kanban_home(tmp_path, monkeypatch): | ||
| """Isolated HERMES_HOME with two real profiles on disk (researcher, | ||
| writer) plus the implicit ``default``.""" | ||
| home = tmp_path / ".hermes" | ||
| home.mkdir() | ||
| monkeypatch.setenv("HERMES_HOME", str(home)) | ||
| monkeypatch.setattr(Path, "home", lambda: tmp_path) | ||
| monkeypatch.delenv("HERMES_KANBAN_TASK", raising=False) | ||
| _seed_profiles(home, "researcher", "writer") | ||
| kb._INITIALIZED_PATHS.clear() | ||
| kb.init_db() | ||
| return home | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # validate_assignee() unit cases | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def test_validate_assignee_accepts_real_profile(kanban_home): | ||
| assert kb.validate_assignee("researcher") is None | ||
| assert kb.validate_assignee("writer") is None | ||
|
|
||
|
|
||
| def test_validate_assignee_accepts_default(kanban_home): | ||
| # `default` is always a valid profile when the default root exists. | ||
| assert kb.validate_assignee("default") is None | ||
|
|
||
|
|
||
| def test_validate_assignee_normalizes_case(kanban_home): | ||
| # create_task canonicalizes via normalize_profile_name; the validator must | ||
| # accept the same case-folded form so a title-cased label still passes. | ||
| assert kb.validate_assignee("Researcher") is None | ||
|
|
||
|
|
||
| def test_validate_assignee_rejects_unknown(kanban_home): | ||
| err = kb.validate_assignee("equity-analyzer") | ||
| assert err is not None | ||
| assert "equity-analyzer" in err | ||
| # Lists the valid profiles so the model can self-correct. | ||
| assert "researcher" in err and "writer" in err | ||
|
|
||
|
|
||
| @pytest.mark.parametrize("sentinel", [None, "", " ", "none", "None", "-", "null"]) | ||
| def test_validate_assignee_accepts_unassigned_sentinels(kanban_home, sentinel): | ||
| # A deliberately-unassigned / triage card is valid — don't over-tighten. | ||
| assert kb.validate_assignee(sentinel) is None | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # Tool surface: _handle_create | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def _board_task_count() -> int: | ||
| conn = kb.connect() | ||
| try: | ||
| row = conn.execute("SELECT COUNT(*) AS n FROM tasks").fetchone() | ||
| return int(row["n"]) | ||
| finally: | ||
| conn.close() | ||
|
|
||
|
|
||
| def test_tool_create_real_assignee_succeeds(kanban_home): | ||
| from tools import kanban_tools as kt | ||
|
|
||
| out = json.loads(kt._handle_create({"title": "do work", "assignee": "researcher"})) | ||
| assert out["ok"] is True | ||
| assert out["task_id"] | ||
|
|
||
|
|
||
| def test_tool_create_unknown_assignee_rejected_no_row(kanban_home): | ||
| from tools import kanban_tools as kt | ||
|
|
||
| before = _board_task_count() | ||
| out = json.loads(kt._handle_create({"title": "bad route", "assignee": "equity-analyzer"})) | ||
| assert "error" in out | ||
| assert "equity-analyzer" in out["error"] | ||
| # The board must be unchanged: no row was created for the bad assignee. | ||
| assert _board_task_count() == before | ||
|
|
||
|
|
||
| def test_tool_create_unassigned_sentinel_succeeds(kanban_home): | ||
| """An explicit 'none' assignee is a recognized unassign sentinel and must | ||
| still pass the validator — guards against over-tightening. (The validator | ||
| deliberately only gates 'names a lane that doesn't exist'; it does not | ||
| coerce how the sentinel is stored — that's out of scope.)""" | ||
| from tools import kanban_tools as kt | ||
|
|
||
| out = json.loads(kt._handle_create({"title": "park it", "assignee": "none"})) | ||
| assert out["ok"] is True | ||
| assert out["task_id"] | ||
|
|
||
|
|
||
| # --------------------------------------------------------------------------- | ||
| # CLI surface: hermes kanban create | ||
| # --------------------------------------------------------------------------- | ||
|
|
||
| def test_cli_create_real_assignee_succeeds(kanban_home): | ||
| out = kc.run_slash("create 'cli ok' --assignee writer") | ||
| assert "Created" in out | ||
|
|
||
|
|
||
| def test_cli_create_unknown_assignee_rejected_no_row(kanban_home): | ||
| before = _board_task_count() | ||
| out = kc.run_slash("create 'cli bad' --assignee equity-analyzer") | ||
| assert "Created" not in out | ||
| assert "equity-analyzer" in out | ||
| assert "not a known profile" in out | ||
| # No row created for the bad assignee. | ||
| assert _board_task_count() == before |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When a create request is retried with an
--idempotency-keyfor an existing non-archived task whose assignee profile was later renamed or deleted, this preflight rejects the retry beforekb.create_taskcan reach its existing idempotency lookup and return the original task id. That turns an idempotent retry into a hard failure; the tool path has the same ordering, so webhook/orchestrator retries can break after a profile cleanup even though no new row would be inserted.Useful? React with 👍 / 👎.