Repository navigation
kanban/delegation: bind completion to run evidence and add read-only status - #102390
Rook-CodeVolt wants to merge 6 commits into
Conversation
…hanical completion verification
Part A — kanban_db.py:
- Add HallucinatedResultError (ValueError subclass, mirrors HallucinatedCardsError
fail-closed style) with .task_id, .pattern, .actual fields
- Add optional expected_result_pattern parameter to complete_task()
- When supplied and result/summary does not match (re.search), block completion
with an auditable 'completion_blocked_hallucination' event and raise
HallucinatedResultError instead of writing status='done'
- When omitted (default), behavior is byte-identical to existing callers
Part B — tools/async_delegation.py + hermes_cli/kanban.py:
- Add query_delegation_status(delegation_id) to async_delegation.py
Reads async_delegations table directly from state.db, bypasses all
in-memory _records (LLM self-report path), returns parsed result_json
as dict. Returns None for unknown ids. Thread-safe read-only query.
- Add _set_state_db_path_for_tests() for hermetic unit testing
- Add 'hermes kanban delegation-status <id> [--json]' CLI subcommand
that wraps query_delegation_status so parent orchestrators can call
it via terminal tool without any new model-facing core tool surface
Design choice for Part B: CLI subcommand ('hermes kanban delegation-status')
is the smallest footprint option per AGENTS.md Footprint Ladder:
- Terminal tool is already in scope for kanban workers
- Zero new model-tool-schema surface added
- Reuses existing kanban CLI infra (build_parser + handlers dict)
- Parents can call 'hermes kanban delegation-status <id> --json' and
parse the JSON to get ground-truth durable state
Tests: 14 new behavior-contract tests (9 for Part A, 5 for Part B),
all following RED-then-GREEN TDD cycle. No regressions in existing
kanban_db / async_delegation / kanban CLI suites.
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 9502ffce6f0c17abff1bcffb3b47a17cf0ab73c8 against exact base/current main@63279301bcbdc185c1b07b98a9312eb0c862f26d. I read the five-file patch, the durable delegation query path, the Kanban completion gate and CLI contract, the new tests, exact-head workflow state, and the live FILE-LIST collisions with #102117 and #102406/#102445.
There are two correctness blockers before this can be treated as mechanical completion verification, plus one structural landing blocker.
BLOCKER 1 — the new completion gate still verifies the worker's own prose, not independent evidence
complete_task(..., expected_result_pattern=...) runs re.search() directly against summary or result — the same worker-supplied payload that this PR says must not be trusted. A child that did nothing can return text containing the expected phrase and the task transitions to done; no artifact, receipt, acceptance authority, or externally observed state is consulted. The test suite proves matching/non-matching strings, not completed/non-completed work.
The default also remains expected_result_pattern=None, and this patch does not wire a production caller that supplies it. So the existing completion path the PR identifies as unsafe remains byte-identical and still unblocks dependents from unverified self-report.
Please bind completion to evidence independent of the worker narrative. A string pattern can be a claim-shape check, but not the acceptance proof. Add an inverse regression where the worker returns a perfectly matching success phrase while the required evidence is absent/wrong and prove complete_task() refuses to unblock dependents; then a positive case with independently verified evidence should pass. The evidence also needs to be bound to the exact task/run (expected_run_id or equivalent) so stale proof cannot satisfy a later retry.
BLOCKER 2 — delegation-status collapses read failures into not found
query_delegation_status() catches every exception and returns None. _cmd_delegation_status() interprets None as an absent ID and returns exit code 1, even though the public CLI contract says 2=error. A corrupt DB, permission failure, schema failure, or other read error therefore becomes a false "not found" result. That is the opposite of a ground-truth verifier: uncertainty is converted into absence.
The helper also calls _initialize_schema(conn) on the purported read-only verification path, so observation can perform DDL/create-or-migrate state before answering. A verifier should not mutate the object it is verifying.
Please make the durable read actually read-only and preserve a typed error/exception separately from not found; the CLI should return 2 on verification failure. Add regressions for a missing ID versus a corrupt/unreadable DB and assert they cannot collapse to the same outcome.
BLOCKER 3 — current patch lands new behavior into two >2K owners already being decomposed
This PR edits hermes_cli/kanban.py at ~3.4k lines and hermes_cli/kanban_db.py at ~5.4k lines. #102117 is the active decomposition on this same base and directly owns hermes_cli/kanban.py, hermes_cli/kanban_db.py, and tools/async_delegation.py; #102406/#102445 also collide on hermes_cli/kanban_db.py while defining profile-delegation completion semantics. This cannot safely land as independent growth to those facades. Preserve the verification semantics, but compose them into the bounded owners from #102117 (or forward-port #102117 explicitly if this lands first), and reconcile the completion contract with #102406/#102445 rather than letting merge order decide which semantics survive.
Exact-head verification state
The sole surviving commit is the exact PR head. Hosted CI 33788394581, Docker 33788393749, and Nix 33788393744 are all action_required, so there is no exact-object green receipt yet.
The useful part here is the move toward a durable observer and an explicit acceptance gate. The current implementation just stops one layer too early: it mechanically checks prose about work rather than mechanically checking the work/evidence itself, and its verifier fails open from read uncertainty to not found.
alt-glitch
added
type/feature
Replace prose-pattern matching with exact-task/run verification receipts. Make delegation-status reads non-mutating and distinguish read errors from missing IDs, while isolating the observer in a bounded module.
Rook-CodeVolt
changed the title
andrexibiza
left a comment
andrexibiza
left a comment
There was a problem hiding this comment.
Follow-up on exact head 69bbe0311b8db5f94af8ccd90d6a50f649f049e5 against current main@63279301bcbdc185c1b07b98a9312eb0c862f26d.
The prior correctness blockers are materially fixed on this object: completion no longer accepts worker prose as proof, and the delegation observer is now a true mode=ro read with typed read failures distinct from an absent id. The inverse evidence tests cover wrong task/run/status and stale receipts.
One correctness blocker remains in the production wiring.
BLOCKER — incomplete dispatcher identity disables the evidence gate
_worker_completion_evidence() returns (False, None) when HERMES_KANBAN_TASK matches the task but any of HERMES_SESSION_ID, HERMES_KANBAN_WORKSPACE, or HERMES_KANBAN_RUN_ID is missing/unparseable. _handle_complete() interprets that first boolean as require_completion_evidence=False, so a dispatcher-owned worker with partial/malformed trusted metadata falls straight back to the legacy prose-only completion path.
That is a fail-open transition at exactly the boundary this patch is meant to harden. The tool surface itself does not require all of those fields: dispatcher worker mode is admitted from HERMES_KANBAN_TASK plus the ownership context. Once this is the worker's own task, inability to establish the session/workspace/run needed to query verification is uncertainty, not evidence that verification is not_applicable.
Please make missing or invalid worker evidence context fail closed: for a dispatcher-owned call on its own task, return require=True with no receipt (or an equivalent typed failure) whenever the session/workspace/run identity cannot be established. The legitimate opt-out should be the verifier explicitly returning not_applicable, not missing identity. Add inverse regressions for missing session id, missing workspace, and missing/invalid run id and prove kanban_complete cannot transition the task to done in each case.
Structural landing gate remains
The observer extraction is better, but this exact patch still grows hermes_cli/kanban.py (~3.4k) and hermes_cli/kanban_db.py (~5.6k). #102117 already decomposes those surfaces into bounded owners including hermes_cli/kanban_parser.py and the hermes_cli/kanban_db_* modules, and #102406/#102445 overlap hermes_cli/kanban_db.py with profile-delegation completion semantics. Preserve this evidence contract through that composition; merge order cannot be the integration strategy.
Exact-object proof
This PR now has two surviving commits, 9502ffce6f0c17abff1bcffb3b47a17cf0ab73c8 and 69bbe0311b8db5f94af8ccd90d6a50f649f049e5. Neither has a hosted-green matrix: the former's CI 33788394581, Docker 33788393749, and Nix 33788393744 are action_required; the current head's CI 33815619278, Docker 33815618954, and Nix 33815618615 are also action_required. I attempted the exact-head CI rerun and GitHub returned 403 Resource not accessible by integration.
So the prior two logic findings are closed, but this exact object is not yet completion-safe or every-commit-green.
… gate When HERMES_KANBAN_TASK matches the task (dispatcher-owned call) but any of HERMES_SESSION_ID, HERMES_KANBAN_WORKSPACE, or HERMES_KANBAN_RUN_ID is missing or unparseable, _worker_completion_evidence previously returned (False, None) — silently falling back to the legacy prose-only completion path. This was a fail-open transition at exactly the boundary the completion evidence gate is meant to harden. Fix: return (True, None) — "evidence required, no receipt" — whenever the task matches but identity is incomplete. The legitimate opt-out path remains the verifier explicitly returning not_applicable, never missing identity variables. Update three existing tests (test_complete_happy_path, test_complete_retry_with_empty_created_cards_succeeds, test_worker_lifecycle_through_tools, and test_notifier_artifact_delivery_skips_missing_files) to supply the required identity env vars + a not_applicable verifier mock, consistent with how the no-code workspace path is supposed to flow. Add three inverse regression tests per the reviewer's requirement: - test_missing_session_id_requires_evidence_no_receipt - test_missing_workspace_requires_evidence_no_receipt - test_missing_invalid_run_id_requires_evidence_no_receipt Each proves both the low-level return value and that kanban_complete cannot transition the task to done through the missing-identity path. Fixes: PR NousResearch#102390 third review blocker (andrexibiza@, 2026-09-03)
Third review blocker addressed — 5894c31Commit 5894c31 fixes the fail-open dispatcher identity gap identified in review PRR_kwDOPRF1G88AAAABMHMaJA. What was wrong: Fix: Return TDD evidence (RED confirmed before GREEN):
Tests watched fail (returned False, not True) before the one-line fix; all three pass after. Collateral test updates (not regressions): Four existing tests that called Local test result: 38 passed in tests/tools/test_kanban_tools.py; 18 pre-existing failures in the broader kanban suite unchanged (all from hermes_cli layer, none in the tools layer touched by this PR). Still requiring upstream maintainer action (no action taken on these):
|
Fail-open dispatcher identity gap — re-verified, still resolved on exact head 5894c31Re-investigated this PR from a fresh clone against current Blocker 2 (fail-open identity) — confirmed fixed and regression-tested on exact head
Blocker 3 (structural landing gate) — not yet addressed, and the drift has grown substantially.
CI — still Net status: the specific security-relevant fail-open gap this review thread was tracking is fixed and regression-tested on the exact PR head. The PR is not yet mergeable due to the structural drift above; requesting independent security review of the fail-open fix now per process, separate from the merge-readiness question. |
…plete Independent security review of PR NousResearch#102390 (t_69a24e88, Maya) found a second, unaddressed fail-open path alongside the fix in 5894c31: the completion-evidence gate in tools/kanban_tools.py's _worker_completion_evidence() was only enforced by the model-tool wrapper _handle_complete(). The CLI surface hermes_cli/kanban.py::_cmd_complete() (hermes kanban complete <id>) called kb.complete_task() with require_completion_evidence defaulting to False and never consulted _worker_completion_evidence() at all. Every dispatcher-spawned worker has a terminal tool, and TERMINAL_ENV defaults to "local", so a worker could defeat the entire evidence-hardening effort with a single shell command from its own terminal tool instead of calling the kanban_complete tool -- a confused-deputy bypass, since a worker process shares the same OS identity/env as the trusted-human CLI caller the docstring assumed. Fix: hermes_cli/kanban.py::_cmd_complete() now calls a new _cli_worker_completion_evidence() helper that delegates to the single source of truth (tools.kanban_tools._worker_completion_evidence) and passes its result into kb.complete_task(require_completion_evidence=..., completion_evidence=...), exactly mirroring the tool path. The helper is a no-op (False, None) for every caller that is not itself the dispatcher-owned worker for the exact target task id (using the same HERMES_KANBAN_TASK == task_id predicate _worker_run_id_for() already uses in this file), so ordinary human/CLI completion is unaffected. Import failure fails closed (evidence required, no receipt), matching the tool path's own defensive posture. Added 3 regression tests in tests/hermes_cli/test_kanban_core_functionality.py: - test_cli_complete_blocks_worker_bypass_without_evidence: worker identity + unverified evidence -> CLI complete is rejected, task stays non-done. - test_cli_complete_allows_worker_with_verified_evidence: worker identity + a real passing verification receipt -> CLI complete succeeds (gate blocks unverified self-report, not legitimate work). - test_cli_complete_unaffected_for_non_worker_caller: no HERMES_KANBAN_TASK identity match -> CLI complete behaves exactly as before. Verified: tests/hermes_cli/test_kanban_core_functionality.py, tests/hermes_cli/test_kanban_cli.py, tests/tools/test_kanban_tools.py all pass (68 passed, 1 skipped) including the original 38 tool-path tests. Ran the full tests/hermes_cli/ -k kanban suite before and after this change: the same 15 pre-existing failures (write-guard/hook/ decompose isolation issues unrelated to completion) occur at the unmodified PR head, confirming no regression introduced here.
|
Pushed cf2d39b to close the parallel fail-open path found in independent review (t_69a24e88, Maya): Fix: Added 3 regression tests in tests/hermes_cli/test_kanban_core_functionality.py covering: worker-identity + unverified evidence (blocked), worker-identity + real passing receipt (allowed), and non-worker caller (unaffected). Full suite: tests/hermes_cli/test_kanban_core_functionality.py + test_kanban_cli.py + tests/tools/test_kanban_tools.py all pass (68 passed, 1 skipped). Also ran tests/hermes_cli/ -k kanban before/after — same 15 pre-existing unrelated failures (write-guard/hook/decompose isolation issues) at both the old and new head, confirming no regression. Re-requesting review — this addresses the HIGH-severity finding from t_69a24e88. |
|
Update: pushed a follow-up fix (commit cf2d39b) closing the fail-open dispatcher-identity gap flagged in review — when session/workspace/run-id identity is missing or malformed, the CLI completion path now requires evidence (fails closed) instead of silently falling back to legacy prose-only completion. Added 3 regression tests proving both the RED (old behavior) and GREEN (fixed behavior) cases. Independently re-reviewed and approved internally with a 5-scenario repro against the real CLI entry point, full test suite (68 passed/1 skipped), and confirmation the CLI helper delegates to the single source of truth in tools/kanban_tools.py rather than reimplementing (no drift risk), failing closed on import failure. Ready for maintainer re-review. |
…tion-mechanical-verification # Conflicts: # hermes_cli/kanban.py # hermes_cli/kanban_db.py # tests/hermes_cli/test_kanban_notify.py # tests/tools/test_kanban_tools.py # tools/kanban_tools.py
…arch#102117 modules Forward-ports PR NousResearch#102390's fail-open dispatcher-identity fix (fix a worker's completion claim being trusted without independent verification evidence onto the current decomposed kanban.py/kanban_db.py/kanban_tools.py module structure (post-NousResearch#102117). The original PR's base predates that 9,455-commit decomposition; this reconciles the same logic against the current layout rather than mechanically merging. - tools/kanban_tools.py: _worker_completion_evidence() binds a verification_evidence receipt to the current task_id/run_id, treats the verify_on_stop disabled status the same as not_applicable (the ledger isn't running, so absence of a receipt proves nothing -- this status value did not exist when the original PR was authored), and fails closed (evidence required, no receipt) on any malformed/missing identity vars rather than silently falling open. - hermes_cli/kanban_db.py: complete_task() gains require_completion_evidence/completion_evidence params, validated and audited (completion_blocked_missing_evidence event) BEFORE the existing kanban_pr_acceptance_store prepare_acceptance/record_acceptance calls -- the two gates are independent and additive (PR-publication acceptance vs. worker-claimed-done-without-proof), not conflicting. - hermes_cli/kanban.py: _cli_worker_completion_evidence() closes the CLI confused-deputy bypass (a worker shelling out to directly to dodge the tool-path gate); delegates to the single source of truth in kanban_tools rather than reimplementing. Also wires the new delegation-status command (_cmd_delegation_status) into _HANDLERS. - hermes_cli/kanban_parser.py: delegation-status re-expressed as a _cmd(...) record in the post-NousResearch#102117 data-driven _SPECS table (the original PR's imperative add_parser call has no home there). - hermes_cli/delegation_status.py (new): read-only bounded SQLite observer for async_delegations, used by both the new CLI verb and tools/async_delegation.py::query_delegation_status. - Test files ported/extended to current fixture conventions (kbc.connect() from hermes_cli.kanban_db_connect, not the old kb.connect() re-export removed by the decomposition): test_kanban_completion_evidence.py (new), test_check_delegation_status.py (new), test_kanban_core_functionality.py, test_kanban_notify.py, test_kanban_tools.py (6 new fail-open regression tests), test_kanban_descendant_scope.py (fixed a regression this change introduced -- the shared _worker_board fixture now sets the identity env vars and mocks the verifier so the gate doesn't block its own coverage). Verified: full source tree py_compile clean; targeted kanban test files all green; full tests/hermes_cli + tests/tools run diffed against unmodified upstream/main to confirm every remaining failure is pre-existing/ environmental (sandbox tmp-path realpath differences, flaky timing tests, etc.), not a regression from this change. Supersedes PR NousResearch#102390 (same fix, forward-ported past the NousResearch#102117 refactor). Maya's independent security review required before merge (auth/ completion-integrity trust-boundary code). EOF ) # Conflicts: # hermes_cli/kanban.py # hermes_cli/kanban_db.py # tests/hermes_cli/test_kanban_notify.py # tools/kanban_tools.py
3 participants
Problem
A delegated worker's completion prose is a claim, not acceptance evidence. The prior implementation still allowed matching prose to unblock dependants, and the durable delegation observer converted database failures into “not found” while initializing schema on a purported read path.
Changes
Run-bound completion evidence
expected_result_pattern/HallucinatedResultErrorwith an independent receipt gate.kanban_completenow reads the existing terminal verification ledger for code workspaces. A receipt must bepassed, produced after the current run started, and is bound by the trusted wrapper to the exact task and dispatcher run.complete_task()rejects missing, failed, wrong-task, wrong-run, or untrusted receipts before settingstatus='done'.completion_blocked_missing_evidencewithout copying worker prose. Successful completion events retain the bounded receipt fields for audit.Read-only durable delegation observer
hermes_cli/delegation_status.py.state.dbwith SQLitemode=ro; it never creates directories, initializes schema, or migrates the database.Noneonly when the delegation ID is absent from a readable compatible table.DelegationStatusReadError; the CLI returns exit code 2, distinct from exit code 1 for not found.hermes kanban delegation-status <id> [--json]as the zero-new-tool-schema interface.Tests
TDD regressions cover:
Verification on head
69bbe0311b8db5f94af8ccd90d6a50f649f049e5before push:uv run pytest -q tests/hermes_cli/test_kanban_completion_evidence.py tests/tools/test_check_delegation_status.py tests/tools/test_kanban_tools.py— 52 passeduv run pytest -q tests/hermes_cli/test_kanban_db.py tests/tools/test_async_delegation.py— 60 passed, 1 skippeduv run ruff check ...across all eight changed Python paths — passedgit diff --check— passedRelated PR reconciliation
Live heads inspected:
e293872ac5cc79a3ce74f5e01a6f28b9d946946e830543e7c3c8696797119497c4fc0cb6c5ea9a9d38e7b0070741bf3eb33745a45250f1f11d415705This patch no longer puts the durable read implementation in
tools/async_delegation.py; that file is now a compatibility wrapper over the bounded observer. The CLI still needs small parser/dispatch wiring inhermes_cli/kanban.py, and the completion kernel necessarily toucheshermes_cli/kanban_db.py. If #102117 lands first, those two small integrations should be forward-ported into itskanban_parser/ bounded command owner rather than restoring facade growth. #102406/#102445 define separate profile-delegation tables and reconciliation; this patch does not adopt or overwrite those schemas or completion states.Documentation impact
No standalone documentation change: the public CLI contract is documented in argparse help and the bounded observer/DB APIs have updated docstrings. No new environment variables, dependencies, migrations, model-facing tools, or deployment steps.