Skip to content

fix(kanban): stop claimed children bypassing new dependencies - #113398

Closed
fangliquanflq wants to merge 5 commits into
NousResearch:mainfrom
fangliquanflq:fix/kanban-reject-claimed-child-links
Closed

fangliquanflq wants to merge 5 commits into
NousResearch:mainfrom
fangliquanflq:fix/kanban-reject-claimed-child-links

Conversation

@fangliquanflq

Copy link
Copy Markdown

What does this PR do?

Rejects parent links to children that are already running. Previously, kanban_link reported success and persisted the edge even though the active child run could not be gated, allowing work that was meant to be serialized to continue concurrently.

Symptom

An operator links an unfinished parent to a child after the child has been claimed. The API reports success and records a linked event, but the child remains running.

Impact

Operators receive a false success signal and can unintentionally allow concurrent work against resources that the dependency was intended to protect.

Bug Cause

Trigger: hermes_cli/kanban_db.py:1595 / link_tasks receives a child whose status is already running.

Causal chain:

  1. The dispatcher atomically claims a ready child and changes its status to running.
  2. link_tasks inserts the dependency, but its gating update only changes ready children to todo.
  3. The update affects no row, while the edge and success response are still committed, so the active run ignores the new parent.

Why it is wrong: A dependency added after claim cannot retroactively constrain the active run, so persisting it as a successful link misrepresents the state transition.

Working sibling / contrast: When linking before claim, a ready child is atomically demoted to todo, and the claim path rechecks unfinished parents.

Ruled out: This is not a missing claim-time parent check; claim_task already checks _parents_satisfied. The failure occurs only when the link transaction runs after the claim transaction.

Fix

Within the same immediate write transaction used to add the edge, reject a running child before inserting the link or emitting events. A regression test verifies the clear error and confirms that no ineffective edge or linked event is recorded.

Related Issue

Closes #113374

Type of Change

  • Bug fix (non-breaking change that fixes an issue)

Changes Made

  • hermes_cli/kanban_db.py - reject links to running children atomically before persisting the edge.
  • tests/hermes_cli/test_kanban_db.py - cover the claimed-child rejection and absence of misleading link state.

How to Test

  1. Create an unfinished parent and a ready child.
  2. Claim the child, then call link_tasks for the new dependency.
  3. Confirm the call raises child is already running and no parent edge or linked event exists.
  4. Run scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py -k link_tasks.

Checklist

Code

  • I've read the Contributing Guide
  • My commit messages follow Conventional Commits (fix(scope):, feat(scope):, etc.)
  • I searched for existing PRs to make sure this isn't a duplicate
  • My PR contains only changes related to this fix/feature (no unrelated commits)
  • I've added tests for my changes (required for bug fixes, strongly encouraged for features)

Documentation & Housekeeping

  • I've updated relevant documentation (README, docs/, docstrings) - N/A; the behavior and error are documented by the function contract
  • I've updated cli-config.yaml.example if I added/changed config keys - N/A
  • I've updated CONTRIBUTING.md or AGENTS.md if I changed architecture or workflows - N/A
  • I've considered cross-platform impact (Windows, macOS) per the compatibility guide - SQLite transaction and status handling are platform-independent
  • I've updated tool descriptions/schemas if I changed tool behavior - N/A; the existing schema already describes dependency gating and errors are returned through the existing tool boundary

Screenshots / Logs

N/A - covered by the focused database regression test.

@KeyArgo

KeyArgo commented Sep 16, 2026

Copy link
Copy Markdown

Independent verification of this PR on pristine main (4e9d3c713a3) and on the PR head (e30fd8106db), plus one regression and one freshness note.

What the change does — confirmed. The new test is red on main and green on this branch:

tree command result
main @ 4e9d3c713a3 pytest tests/hermes_cli/test_kanban_db.py -k rejects_running_child 1 failed
this branch @ e30fd8106db same 1 passed

The rejection is atomic (raised inside the write_txn, so no edge and no linked event survive) and the error surfaces cleanly on every surface rather than as a traceback: the tool wrapper converts ValueError to a structured tool error (tools/kanban_tools.py::_kanban_handler), hermes kanban link maps it to kanban: <message> (hermes_cli/kanban.py, the except (ValueError, RuntimeError, PermissionError) around dispatch), and the dashboard returns 400 (plugins/kanban/dashboard/plugin_api.py, _value_error_400).

Regression: refusing a running child breaks the dependency-parking flow. On the PR head:

$ pytest tests/hermes_cli/test_kanban_block_kinds.py::test_dependency_then_parent_done_promotes
E   ValueError: cannot link t_66f77d90 -> t_b068afc0: child is already running
hermes_cli/kanban_db.py:1611
1 failed

The same test on pristine main passes (1 passed). It is not a stale test — it encodes a real sequence: a card is already running, gets linked under a parent, and is then blocked with kind="dependency", which routes it to todo until the parent completes. That is exactly the "park this running card behind that parent" operation, and the refusal makes it unreachable (the caller must now unclaim or wait out the run first). Wider run on the PR head across the kanban files: 1 failed, 128 passed, 1 skipped (tests/hermes_cli/test_kanban_db.py, test_kanban_cli.py, test_kanban_notify.py, test_kanban_transfer.py, test_kanban_block_kinds.py, test_kanban_review_lifecycle_complete.py, test_kanban_worktree_teardown.py); tests/tools/test_kanban_tools.py is green (38 passed). So the only failing path is the refusal itself, not the rest of the change.

Freshness: this branch's merge base is older than current main. Measured git diff 4e9d3c713a3 e30fd8106db = 820 files, +5191/−30438; hermes_cli/kanban_db.py here still lacks _lossy_text and the BLOB-degradation work from 4465b8d7d58, and link_tasks sits at line 1595 vs 1616 on main. A rebase would make the review diff match what the maintainer actually merges.

Design note (not a defect claim): refusal is the half of #113374 that the maintainer explicitly left open — #111468 (merged 2026-09-15) took the visibility half for the sibling case (a ready child under an unfinished parent is demoted to todo with a recorded dependency_wait, and the link/kanban_link/dashboard responses report the gate), while the refusal/force variant stayed with maintainers (#110999, @KoNit-K, still open). A visibility half keeps a post-claim edge recordable (which matters for the parked-card flow above) while still making the ineffective link impossible to mistake for a success. If the maintainers prefer refusal regardless, the regression above needs a carve-out for kind="dependency" parking.

Happy to re-verify any of these numbers after a rebase or a fix — say the word and I'll re-run the same commands.

@alt-glitch alt-glitch added type/bug Something isn't working P3 Low — cosmetic, nice to have comp/cron Cron scheduler and job management labels Sep 16, 2026
@fangliquanflq

Copy link
Copy Markdown
Author

Thanks for the detailed verification. I addressed each point:

  • The dependency-parking regression was valid. An active worker can now link its own running card only when the task ID, current run ID, and pinned board all match; test_dependency_then_parent_done_promotes remains green.
  • Unowned or foreign running children are still rejected atomically, with no edge or linked event. CLI and tool tests cover both same-board foreign children and cross-board task/run ID collisions.
  • The design concern is handled by that narrow ownership carve-out: the legitimate link-then-dependency handoff remains available, while unrelated callers cannot record an ineffective edge.
  • The branch is currently conflict-free (mergeable: true), so I did not merge main solely because it is behind.

Additional review found and fixed a cross-board ownership gap in the first carve-out. The latest tip binds ownership to HERMES_KANBAN_BOARD as well as task/run identity.

Validation with the canonical runner:

  • scripts/run_tests.sh tests/hermes_cli/test_kanban_cli.py tests/hermes_cli/test_kanban_block_kinds.py tests/tools/test_kanban_tools.py — 48 passed
  • scripts/run_tests.sh tests/hermes_cli/test_kanban_db.py -k "rejects_unowned or allows_owning" — 2 passed
  • independent scan-only review of the committed tip — perfect, with no remaining findings

teknium1 added a commit that referenced this pull request Sep 18, 2026
Salvage follow-up on the cherry-picked #113398 (@fangliquanflq) and #113452
(@JoaoMarcos44) commits, trimmed to the salvage bar and widened to the class.

- link: drop the HERMES_KANBAN_BOARD binding on the run-id ownership proof
  (tools._worker_run_id / kanban._worker_run_id_for). complete/block prove
  ownership with the same task-scoped run id and never board-bind it; the
  guarded case is a forged cross-board task-id collision, not a live path.
  Dropped the two cross-board tests and the duplicate db owner test.
- complete: move the open-parents enumeration into kanban_db.unsatisfied_parents
  next to _parents_satisfied so the tool, the CLI (`hermes kanban complete`
  printed "unknown id or terminal state" for the same refusal), kanban_show
  (`unsatisfied_parents`) and diagnostics share one query.
- show: new `running_with_open_parents` diagnostic (hermes kanban show /
  diagnostics / dashboard) for a running card whose parent is not done —
  the "surface unresolved parents on a running card" ask of #113374.
- tests trimmed to invariants; docs for the refusal text, the show field and
  the diagnostic.

Co-authored-by: fangliquanflq <fangliquan@qq.com>
Co-authored-by: joaomarcos <joaomarcosdias444@gmail.com>
@teknium1

Copy link
Copy Markdown
Collaborator

Thanks @fangliquanflq. Your change was salvaged into #114786 with your authorship preserved (cherry-picked); #114786 — fix(kanban): kanban_link refuses a running child and dependency refusals name the open parents (#113374, #113373, salvage #113398, #113452) — is now merged on main at e6a8eb7f95aa, closing issues #113374, #113373. Closing this PR in favour of the landed change; if you see a case it does not cover, please open a fresh issue with the repro and tag it.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

comp/cron Cron scheduler and job management P3 Low — cosmetic, nice to have type/bug Something isn't working

Projects

None yet

4 participants