fix(kanban): guard dependency block after successor creation - #66632
fix(kanban): guard dependency block after successor creation#66632JeffreyDavidson wants to merge 1 commit into
Conversation
tonydwb
left a comment
There was a problem hiding this comment.
{
"event": "APPROVE",
Code Review Summary
Verdict: Approved
Prevents block_task(kind=\"dependency\") on a parent task that already has a created successor. Trying to park a parent with a dependency block after a successor exists would leave the successor orphaned. The fix checks for existing child links and raises a clear ValueError with the task IDs. Regression test covers the exact scenario.
Looks Good
- Check happens before any state mutation
- Error message includes both task IDs for easy debugging
- Test verifies parent remains
runningand child becomesreadyafter parent completes
Reviewed by Hermes Agent",
"comments": []
}
tonydwb
left a comment
There was a problem hiding this comment.
Code Review Summary
Verdict: Comment
+33/-0 kanban fix to guard dependency block after successor creation. Small, targeted fix. No concerns.
Reviewed by Hermes Agent
teknium1
left a comment
There was a problem hiding this comment.
Thanks for isolating a real lifecycle hole: current block_task routes a dependency-blocked task to todo without considering an already-created successor (hermes_cli/kanban_db.py:4936-4976).
Problems
- The new guard is too broad.
link_taskspermits an unfinished parent to be linked to an already-running task (hermes_cli/kanban_db.py:2814-2837), so a valid graph can beP -> A -> C;Amust still be allowed to dependency-wait forP. The dependency contract is based on incoming parents (hermes_cli/kanban_db.py:3441-3475), not whetherAhas descendants. - The new
ValueErrorwould escapehermes kanban block:_cmd_blockwrites a BLOCKED comment and directly callsblock_taskwithout handling that exception (hermes_cli/kanban.py:2047-2057).
Suggested changes
- Reject only when there is no unsatisfied incoming parent dependency, rather than whenever a successor exists.
- Add the
P -> A -> Cregression and a CLI rejection-path test that confirms no misleading comment is written.
This is an automated hermes-sweeper review.
| # here (rather than ``blocked``) is what keeps a cron from ever seeing | ||
| # a dependency-wait as something to "unblock". | ||
| if kind == "dependency": | ||
| child_row = conn.execute( |
There was a problem hiding this comment.
Checking only for an outgoing child rejects a valid P -> A -> C graph when running A acquires or already has an unfinished incoming parent P. link_tasks permits adding that parent edge to a running task; gate this on unsatisfied incoming parents (or otherwise preserve that dependency-wait path), not descendant existence alone.
SummaryThirteen PRs address or reference #59333/#66909: #59367, #59536, and #59645 implement the requested global Related pull requests
Duplicates#59536 and #59645 are later implementations of #59367; #61372 overlaps the review-reblock portion of #58695, while #61366 and #64585 address the same parentless-dependency cause with different terminal states. Suggested consolidationAuthor action: rebase #59367 onto current main, preserving its boolean-rejection tests and validated routing/event behavior; close #59536 and #59645 as duplicates of #59367 despite their keep_open reviews because their visible diffs retain the validation and default-registration defects above, and close #66909 as a duplicate of #59333. Keep #68027 open with its durable-fingerprint, audited-rearm, and automatic-path-guard salvage path; keep #59906, #61366/#64585, and #63723 in separately scoped salvage lanes addressing their recorded reviews, re-scope #60476 away from #59333, narrow #58695 to the review-reblock exemption unless an authorized ordered verdict source is added, and retain #61372 only as the closed reference. Cross-PR triage: Reviewed 13 pull requests and 2 issues in this complex. Each diff was read against this issue; Assessment working set: 301 kB of PR diffs, 27 kB of issue/PR text, 17 kB of discussion (19 comments), 10 verify verdicts. verdicts reflect diff content, not PR titles. Part of an automated triage batch. |
Summary
todoScope
hermes_cli/kanban_db.pytests/hermes_cli/test_kanban_block_kinds.pyVerification
git verify-commit HEADgit diff --check origin/main...HEADscripts/run_tests.sh tests/hermes_cli/test_kanban_block_kinds.py -k test_dependency_block_refuses_after_successor_creationUpstream QA acceptance was recorded by Merlin for exact signed commit
81fa11a5c0fa434e0ec65f0990d697072154a5a9.