fix(kanban): auto-decomposed subtrees are dispatchable and the orchestrator worker can see the board - #92206
Conversation
Duplicate of #80427 for the decompose-triage child-priority inheritance repair: both propagate the root priority into newly created children. #80427 is the earlier open implementation. This PR also includes separate orchestrator-worker and desktop/profile-session changes, which should be split for focused review. |
andrexibiza
left a comment
There was a problem hiding this comment.
Blocking review on exact head e86208ef062c09a20985db7627f5b541c305a1c3 (GitHub does not permit this identity to submit REQUEST_CHANGES without explicit repository review access).
The priority inheritance change is mechanically sound in isolation, and the orchestrator-worker exception is aimed at a real asymmetry. Two things block this exact object from being merge-safe, though.
- The new board-routing grant is built on a fail-open ownership predicate.
_is_orchestrator_worker() now grants the orchestrator-only surface when HERMES_PROFILE matches kanban.orchestrator_profile, HERMES_KANBAN_TASK is present, and _is_dispatcher_owned_worker() returns true. That last helper is not an authority proof: today it does
try:
from agent.delegation_context import is_dispatcher_owned_worker_context
return is_dispatcher_owned_worker_context()
except Exception:
return Truewhile _is_delegated_child_context() likewise returns False on any exception. So if the context module/import/probe is unavailable or broken, the exact pair of safety checks this PR relies on both fail in the permissive direction. A process carrying the orchestrator profile/task env then gets _check_kanban_orchestrator_mode() == True, and _require_orchestrator_tool() also admits it. That surface includes kanban_unblock, i.e. this is not merely visibility of a read-only helper; the failure mode crosses from one-task lifecycle authority into board-wide mutation authority.
This cuts directly against the architecture landed by #79657: that change introduced is_dispatcher_owned_worker_context() specifically as the single context-local predicate to consult before trusting inherited HERMES_KANBAN_* values, because process-global env is not dispatcher ownership for delegate/cron executions. #79657 preserved @gfriesen1's #78961 diagnosis/credit while replacing the unsafe env-mutation approach with ContextVar ownership. This PR should compose with that boundary, not let an exception collapse back to HERMES_PROFILE + HERMES_KANBAN_TASK as sufficient authority.
Required repair: make the elevated orchestrator-worker decision fail closed when the canonical dispatcher-ownership/delegation verdict cannot be obtained. This can be local to _is_orchestrator_worker() if changing the older wrapper globally has broader compatibility implications. Add a production-path regression where is_dispatcher_owned_worker_context import/call fails and prove both schema gating and _require_orchestrator_tool("kanban_unblock") refuse the worker. Keep the existing delegated-child and cron isolation witnesses from #79657 intact.
The new tests currently monkeypatch _is_dispatcher_owned_worker to True, so they prove the happy-path profile match but never exercise the authority-failure side of the shape.
- The PR object contains a second, unrelated commit and would merge an unadvertised Desktop/profile API change.
The exact ancestry is not just a stale diff artifact. Current main is 4a6b362178ab2445e8310cc55a49fa2816b7aad0; this head is 2 unique commits ahead of merge-base 2584b7c4eca82ada05f16eba08936d157b483329 and 133 commits behind current main. The intended Kanban commit e86208e touches only the four Kanban source/test files, but its parent 5e45a0d10417fe65ea93b174fbf6ef7c2a856cce is fix(profiles): report session history from state db and changes:
tui_gateway/methods_profiles.py(profiles.listgains authoritative all-sourcesession_countand separate queries),apps/desktop/src/plugins/hermes-bots/plugin.js(BotRow preview changes to “N saved sessions”),tests/tui_gateway/test_profiles_list_worker_session.py.
That is real behavior, not part of this PR body. It also sits directly on the #90268/#90359 lane: #90359 by @teknium1 already merged the worker-activity worker_session contract as d604ba6585b856d90c02232ed08ab06f5fdc78f3. Any further session-count/preview work needs its own review and provenance, not an implicit ride inside a Kanban decomposition/authority PR.
Required repair: rebase/cherry-pick the intended Kanban commit onto current main (or otherwise drop 5e45a0d from this PR) before further verification. Do not silently fold the profile-history follow-up into this branch.
Topology / duplicate boundary
The existing triage comment is correct that the priority-inheritance slice overlaps earlier open #80427 by @MarkWin91. #80427 is broader and currently non-mergeable, so that does not make the orchestrator-worker fix a duplicate, nor does it automatically make #80427 the better landing object. But only one priority implementation should survive. If this smaller/current implementation becomes the chosen owner, preserve @MarkWin91's prior-art credit explicitly; if #80427 is repaired and retained, drop the duplicate priority slice here. The orchestrator-worker authority change remains a distinct contribution.
Verification state
- No submitted reviews or inline threads existed when I inspected this head; there is one AI triage comment identifying the duplicate/split issue.
- PR-local evidence reports 41 focused tests passing and the same 15 suite-order Kanban failures on branch/base.
- Exact-head GitHub CI (
32568138770), Docker (32568138147), and Nix (32568138179) are allaction_required; no hosted exact-head jobs executed. That is workflow-approval state, not a code failure, but it is not a green receipt. - GitHub currently reports the PR mergeable, but after removing the unrelated parent and repairing the authority gate, the resulting head needs fresh exact-object tests/CI against current main.
So: keep the useful decomposition fix and the orchestrator-worker concept, but bind board-wide authority to a fail-closed dispatcher-owned context and clean the branch topology before treating this as a merge candidate.
Both defects are real and well-chosen:
Two notes:
|
…trator worker can see the board Two independent gaps make autonomous board orchestration look like it never fires. Rebased onto current main; the unrelated profile-history commit that rode along on the previous head is dropped. 1. Auto-decomposed children were inserted without a priority column, so they landed at the SQL default 0. Dispatch is strictly highest-priority-first with a per-profile in-flight cap, so on a board whose live band sits in the 90s every auto-decomposed subtree queued below the floor and never visibly moved. Children now inherit the root's priority; a child dict may still override with its own value. 2. A dispatched worker run of kanban.orchestrator_profile got a NARROWER toolset than the same profile's interactive session (no kanban_list), so the card meant to sweep the board could only read the ids handed to it and had to route a read-only inventory card to another profile just to enumerate the board. That worker run is now the board-routing surface, in both the schema gate and the call-time guard, which are kept in lockstep. The elevation in (2) is the only path that hands a dispatched worker board-WIDE mutation authority (kanban_unblock), so it fails CLOSED: _is_orchestrator_worker() consults the canonical dispatcher-ownership predicate from #79657 directly with default False, instead of inheriting _is_dispatcher_owned_worker()'s permissive True. Process env (HERMES_PROFILE + HERMES_KANBAN_TASK) is not an authority proof. The older wrapper keeps its permissive default for broad compatibility. Delegated-child and cron isolation from #79657 stay intact and are re-witnessed at both the schema gate and the call-time guard. Priority-inheritance prior art: #80427 by @MarkWin91. That PR is broader and currently non-mergeable; if it is repaired and retained, drop the duplicate slice here. Tests: production-path regression where is_dispatcher_owned_worker_context raises and both the schema gate and _require_orchestrator_tool ("kanban_unblock") refuse the worker; priority inheritance, per-child override, and a genuine zero-priority root. 48 passed locally.
e86208e to
2e43fa7
Compare
|
Both blocking items are repaired on the new head 1. Fail-open ownership predicate — fixed, fails closed now.
# Fail closed, unlike _is_dispatcher_owned_worker()'s permissive default.
return _delegation_ctx("is_dispatcher_owned_worker_context", False)The repair is local to the elevation, as you suggested: the older wrapper keeps its permissive default so nothing else changes behaviour. Rationale is in the docstring — this elevation is the only path that hands a dispatched worker board-wide mutation authority, so process env is not an authority proof. The call-time guard New production-path regression, with no monkeypatch of The #79657 delegated-child witness is intact and now also asserts the call-time refusal of 2. Branch topology — cleaned.
Rebase note: Duplicate boundary. Acknowledged: the priority slice overlaps #80427 by @MarkWin91, and that prior art is credited in the commit message. If #80427 is repaired and retained, drop the slice here; the orchestrator-worker authority change stands independently either way. Verification. 48 tests pass locally against the exact head ( |
Re-triaged: keeping #80427 as related rather than duplicate - it covers only the child-priority inheritance half; the orchestrator-worker |
Auto-decompose fires correctly — the engine works. Two defects make every
decomposed subtree invisible and leave the board-sweeping card unable to see
the board.
Defect 1 — decomposed children are emitted at priority 0
kanban_db.decompose_triage_task()inserts children with an INSERT that omitsthe
prioritycolumn entirely, so every child gets the SQL default0,regardless of the root's priority.
Dispatch is strictly
ORDER BY priority DESC, created_at ASCwithkanban.max_in_progress_per_profile, so on any board whose live band is above0 an auto-decomposed subtree queues behind every hand-created card and never
runs. This is not one starved card: the decomposer systematically emits below
the floor, which is exactly what makes autonomous orchestration look like it
never fires.
Observed on a live board: root at priority 1, its five children all at 0, while
the board's live band was 94-99.
Fix: children inherit the ROOT's priority. A child dict may still set its
own
priorityto deprioritize one leaf, and a root genuinely at 0 still yieldschildren at 0 — this is inheritance, not a hardcoded floor.
Defect 2 — an orchestrator WORKER run has no
kanban_list_check_kanban_orchestrator_mode()returnedFalsefor anydispatcher-spawned worker:
That is right for ordinary workers — close your own card, do not enumerate or
unblock the board — but it also strips the board-routing tools from a worker run
of
kanban.orchestrator_profile, the profile whose entire job is sweeping theboard. The same profile has
kanban_listin an interactive session, so adispatched orchestrator gets a strictly narrower toolset than its own chat.
Consequence, quoted verbatim from a real orchestrator card's completion
metadata:
It could read only the handful of ids handed to it and had to route a read-only
inventory card to another profile just to see the board.
Fix: a dispatched worker run of the configured
kanban.orchestrator_profilekeeps the board-routing surface. Every otherprofile's worker stays scoped to its own card, unchanged. The dispatcher already
exports
HERMES_PROFILEfor the assignee it spawns (kanban_db._worker_env),so the check needs no new state.
_require_orchestrator_tool(), the belt-and-suspenders runtime guard, mirrorsthe same condition. Without that half the tool would be registered in the
schema and then refuse at call time.
With
kanban.orchestrator_profileunset, nothing changes for anyone: no workeris treated as the board agent. A
delegate_taskchild stays denied even whenits parent is the orchestrator, since it shares the parent's process and its
inherited
HERMES_*env is not proof of ownership.Tests
tests/hermes_cli/test_kanban_decompose_db.pyprioritystill winstests/tools/test_kanban_tools.pyagrees with the schema gate
orchestrator_profiledoes not fall opendelegate_taskchild stays deniedBoth headline tests are proven to fail on the unpatched tree:
and pass with it:
41 passedacross both files.tests/hermes_cli -k kanbanshows 15 failures both with and without thischange (identical set, byte for byte) — pre-existing suite-order pollution,
untouched here.
tests/hermes_cli/test_kanban_decompose.pypasses in isolation.