Repository navigation
fix(kanban): parent-granted self-scope carve-out for delegated child self-report - #119108
0xTidalBore wants to merge 1 commit into
Conversation
…self-report In-process delegate_task children could not comment on or attach to their own card: the path fence denied every mutation before the self-scope check ran, so the writable grant handle from connect() had no usable writer (checklist (e) failed on any file-backed board). Composition (path fence decides WHERE, in-process context the blanket deny, parent grant the sole exception to both for exactly one task id): - agent/delegation_context.py: is_delegated_child_in_process_context (ContextVar-only), delegated_self_scope_grant / self_scope_grant / capture_self_scope_identity / is_self_scoped_write. Grant never crosses scrub_kanban_env, so exec'd descendants stay fully fenced. - hermes_cli/kanban_db.py: gate evaluates self-scoped exemption against BOTH the path fence and the blanket in-process deny; only add_comment / add_attachment forward self_scoped_task. - hermes_cli/kanban_db_connect.py: connect() opens a writable handle for a granted in-process child (busy_timeout preserved); write_txn() forwards self_scoped_task. - tools/delegate_tool_child_run.py: parent captures its dispatcher identity before entering the child context and hands the grant down. - tests: new tests/tools/test_kanban_child_self_scope.py (file-backed, 12 cases); spawn-env tests use claim_task + INVOCATION_ID isolation. Also: tools/bot_mode_dm.py parks an undelivered DM body aside (_park_undelivered) when the recipient turn never ran (see NousResearch#100523), so verbatim content survives the runner's unlink for resend.
andrexibiza
left a comment
There was a problem hiding this comment.
Exact-head review — the self-scope exception is currently forgeable and the DB boundary becomes bypassable
I reviewed exact head 199738f9f4677bd9ff1c308fbbb4a68b938f0fd4: all 9 changed files, the delegation/DB boundary, the new file-backed matrix, current-main topology, exact-head workflow state, and the related ownership lineage (#69837, #105202, #113314/#114793, #87650). There were no existing review submissions or inline threads on this PR, so this is not repeating an unchanged-head review.
The goal is coherent: children need a narrow way to return evidence to the card they were delegated from without regaining worker authority. The file-backed tests and keeping exec'd descendants fenced are useful improvements. I do not think the current capability boundary is merge-safe yet, because the new exception can be self-issued and the connection opened for it is broader than the two sanctioned mutations.
Disposition: not ready to merge. Three authority-boundary blockers plus one interlock/scope split are below.
P1 — the child can mint a grant for any task id
delegated_self_scope_grant() is itself callable from the child and unconditionally overwrites _SELF_SCOPE_GRANT. There is no issuer check, no “already delegated” refusal, and no parent-only installation phase. The new test named test_h_nested_grant_cannot_widen_scope actually demonstrates the opposite property: inside the delegated child it enters delegated_self_scope_grant(foreign, "lock") and then asserts is_self_scoped_write(foreign) is True.
That turns the intended parent capability into ambient authority. The child can do the same and then call add_comment() / store_attachment_bytes() on the foreign id; those helpers pass that id into write_txn, and _assert_not_delegated_child_mutation accepts it because the child just replaced the ContextVar.
Please make issuance one-way: install the grant only from the dispatcher-owned parent frame, before entering delegated_child_context, and refuse grant replacement from an in-process child or when a grant is already present. A regression should attempt the exact nested foreign grant and then the foreign comment/attachment; it must fail without changing either task.
P1 — a valid grant now opens an unrestricted writable SQLite handle
The previous fenced connect() returned mode=ro. This branch returns ordinary _sqlite_connect(path) whenever an in-process child has any grant. The helper-level write_txn guard cannot constrain SQL issued directly on that connection.
That reopens the bypass #69837 explicitly closed. A granted child can import kanban_db_connect, call connect() and execute raw SQL against tasks, task_comments, task_runs, etc., without going through add_comment, add_attachment, or _assert_not_delegated_child_mutation at all. The base implementation's own threat model says children may shell out/import the DB module; the read-only connection was the lower boundary that made that safe.
The carve-out therefore needs a capability-shaped mutation surface, not a generic writable DB connection. Keep child-facing connect() read-only and perform the two allowed writes through a narrow owner/gate path, or otherwise enforce the restriction at the SQLite boundary. Add red-before/green-after controls for direct UPDATE tasks ... and direct INSERT task_comments ... on a foreign task while a valid self-report grant exists.
P1 — the grant captures a claim generation but never consumes it
The grant stores (task_id, claim_lock), but is_self_scoped_write() compares only grant[0], and connect() checks only self_scope_grant() is not None. No mutation verifies the captured claim lock/current run before committing.
So a child granted during claim L1 remains authorized after the parent loses/completes/requeues the task and another worker owns the same task under L2. Its late comment/attachment is then durably attributed to the new task generation. This is exactly the ownership boundary the existing claim CAS machinery is supposed to make explicit.
Bind the self-report permit to the exact board/database + task + current claim/run generation, and validate that tuple inside the same transaction that inserts the comment/attachment. Required regression: grant under L1 → transition/reclaim to L2 → old child write is refused and leaves no row; a current-generation grant still succeeds. Also include a different-board/same-task-id control so task ids are not treated as globally sufficient authority.
Interlock / scope — split the Bot Mode DM preservation hunk
The _park_undelivered change is a different defect class from delegated Kanban authority and should not ride in this commit. #100523 is already closed through the live-owner delivery lineage (#101293, later #113954 and follow-ons); preserving a refused legacy/fallback payload may still be useful, but it is adjacent, not the same ownership repair. Give it its own issue/PR and acceptance surface so Bot Mode delivery can evolve independently and contributor/supersession history stays legible.
On the Kanban side, preserve the existing lineage rather than replacing it: merged #69837 established child isolation and credited the salvaged #65953 work plus #56651; merged #114793 later tightened the heartbeat/owner boundary and explicitly discussed why a context-local self-write grant can widen authority if it is not tied to the dispatcher grant. This PR is a refinement of that machinery, not a fresh ownership model.
Exact-head verification / CI
The PR metadata points at base f14f86dd5ccc568296dcd71a74014eb5a1729cf9; current main is 2c65d5aee60d0a4e2eadc34c319ca3daac203667 (5 commits newer). The head itself has an older merge base (be65eab584a8490d39f7552b93e603d82c929d17); GitHub reports it 304 commits behind the PR's recorded base / 309 behind current main, so refresh it onto current main without rewriting contributor history before final acceptance.
Exact-head CI is not green and did not execute jobs: CI run 35726655236, Nix 35726655313, and Docker 35726655075 all concluded action_required, and each run currently returns an empty job list. That is an approval/execution gap, not a test failure, but it is still no exact-head verification receipt. The PR's local 129 passed, 1 skipped claim is useful author evidence; it does not substitute for current-head upstream CI.
I also checked the new tests against the failure model rather than only their pass claims. The most important missing controls are the three negative cases above: child-forged grant, raw-SQL mutation under a legitimate grant, and stale claim-generation use. Once those are red-before/green-after and the unrelated DM hunk is split, the narrow self-report idea is much easier to trust.
| """ | ||
| tid = (task_id or "").strip() | ||
| token = _SELF_SCOPE_GRANT.set((tid, (claim_lock or "").strip() or None) if tid else None) | ||
| try: |
There was a problem hiding this comment.
P1 — this is not parent-only authority. A delegated child can import this same context manager and call delegated_self_scope_grant(foreign_id, ...); there is no issuer/context check before _SELF_SCOPE_GRANT.set. The new test_h_nested_grant_cannot_widen_scope even proves the dangerous intermediate state by asserting is_self_scoped_write(foreign) is True inside a nested foreign grant. From there add_comment / add_attachment accept the forged id. Install the grant only from the dispatcher-owned parent frame (before entering child context), refuse replacement from a child/already-granted scope, and add the foreign-write regression.
| # instead of instant-locking. Never init_db / migrations here; missing | ||
| # schema fails closed. | ||
| if is_delegated_child_in_process_context() and self_scope_grant() is not None: | ||
| conn = _sqlite_connect(path) |
There was a problem hiding this comment.
P1 — this widens two sanctioned writes into a generic writable database capability. _sqlite_connect(path) returns the child a normal writable SQLite connection. write_txn() can guard helper calls, but it cannot mediate conn.execute(...) / commit() issued directly by child code. That defeats the read-only lower boundary #69837 relied on for direct DB imports. Keep the child connection read-only and put comment/attachment writes behind a narrow gated operation (or enforce equivalent restrictions at the SQLite layer); add direct-SQL UPDATE/INSERT denial tests while a valid grant exists.
| def is_self_scoped_write(task_id: str | None) -> bool: | ||
| """True when ``task_id`` is exactly the dispatcher-provided grant for this context.""" | ||
| grant = _SELF_SCOPE_GRANT.get() | ||
| return bool(grant and task_id and str(task_id).strip() == grant[0]) |
There was a problem hiding this comment.
P1 — the captured claim generation is never validated. The grant stores (task_id, claim_lock), but this predicate checks only grant[0]; connect() likewise checks only that a grant exists. A child granted under claim L1 therefore remains able to self-report after the task is completed/reclaimed under L2. Consume the claim/run generation (and exact board identity) in the same transaction as the allowed write, with a stale-L1 → new-L2 refusal regression.
In-process
delegate_taskchildren cannot self-report: the Kanbanmutation gate path-fences FIRST, so a granted child's
add_comment/add_attachmenton its own card always raises before the self-scopecheck runs. The writable grant handle from
connect()therefore hasno usable writer — checklist (e) fails on any file-backed board
(
:memory:-only tests pass and hide it).Amended composition (path fence decides WHERE, in-process context the
blanket deny, parent grant the sole exception to both, exactly one id):
agent/delegation_context.py(additive):is_delegated_child_in_process_context(ContextVar-only, never true for exec'd descendants),
delegated_self_scope_grant/self_scope_grant/capture_self_scope_identity/is_self_scoped_write. The grant isa ContextVar captured from the dispatcher-owned parent, never read
from the child's env (
scrub_kanban_envstripsHERMES_KANBAN_TASKby construction), and dies at the process boundary so grandchildren
stay fully fenced.
hermes_cli/kanban_db.py: gate exempts the self-scoped write fromBOTH the path fence and the blanket in-process deny; only
add_comment/add_attachmentforwardself_scoped_task.hermes_cli/kanban_db_connect.py:connect()opens a writablehandle (busy_timeout kept) for a granted in-process child;
write_txnforwards
self_scoped_task.tools/delegate_tool_child_run.py: parent captures its identitybefore entering the child context and hands the grant down.
tests/tools/test_kanban_child_self_scope.py(file-backedboards, 12 cases: granted self-report, cross-id/status/claim/complete
denial, read-only non-granted handle, exec'd-descendant fencing,
legacy "1" marker, scratch-root writability, env-spoof resistance);
spawn-env tests fixed to
claim_task+INVOCATION_IDisolation.Included in the same branch (same local-edit lineage):
tools/bot_mode_dm.py_park_undelivered— when a Bot Chat turn never ran (refusal, see #100523),the runner's unconditional unlink destroyed the only verbatim copy; the
body is now parked (0600) and its path returned in the failure payload
so a resend reads verbatim instead of paraphrasing from a summary.
Verification: 129 passed, 1 skipped —
test_kanban_child_self_scope.py(12),test_bot_mode_dm.py,test_kanban_boards.py,test_kanban_core_functionality.py.Related: #105202 (child task-ownership class), #87650 (marker scoping).