fix(kanban): enforce safe worker execution and authority - #94009
Closed
benperry6 wants to merge 24 commits into
Closed
fix(kanban): enforce safe worker execution and authority#94009benperry6 wants to merge 24 commits into
benperry6 wants to merge 24 commits into
Conversation
Co-Authored-By: Hermes Agent / OpenAI Codex / GPT-5.6 Sol <noreply@agents.invalid>
`hermes auth reset <provider>` printed "Reset status on N credentials" and left the pool exactly as it was. Measured on a live install: an entry benched with 402 / Insufficient Balance kept `last_status: exhausted` and the same `last_status_at`, read back in a fresh process immediately after the reset, and the CLI went on refusing the provider with "No usable credentials found" long after the account had been topped up. Two causes, one per file. write_credential_pool keeps a newer on-disk cooldown over the caller's snapshot so one process cannot resurrect a key another has just rate-limited. That merge compares `last_status_at`, and clearing sets it to None, which parses as epoch 0 — older than any real timestamp. So a deliberate operator reset was indistinguishable from the stale snapshot the merge exists to reject, and the cooldown was copied straight back over the cleared fields. The failure profile made it hard to see: `_merge_disk_cooldown_state` returns early for a cooldown that has already expired, so a reset appeared to work whenever it did not matter and silently did nothing whenever it did. reset_statuses also never cleared `failure_reason`. It lives in `extra` rather than as a dataclass field, so `replace()` could not reach it, and an entry came out of a reset with no status and no error code but still classified `billing` — which `hermes auth list` renders as though it were current. The fix threads the caller's intent through, mirroring the `removed_ids` parameter that already exists one concern over for the same reason: an id in `status_cleared_ids` says "I have seen the newer status and I am dropping it", as opposed to "I have not seen it". Anything that does not declare the intent keeps the old, protective behaviour. Tests: three in tests/agent/test_credential_pool.py. The cooldown in the two regression tests is deliberately recent, because an expired one passes with or without the fix. Verified by reverting the source change with the tests kept — both regression tests fail and the third keeps passing, so it is known to pin the concurrency guard rather than the new flag. The guard test exists because "skip the merge always" would have made these two pass while reintroducing the lost update the merge was written to prevent. 204 passed, 2 skipped across tests/agent/test_credential_pool.py and tests/hermes_cli/test_auth*.py.
Preserve the five carried fixes while adopting current upstream behavior.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
…mands Kanban-dispatched worker subprocesses fall through every approval-context check in tools/approval.py and silently auto-approve any non-hardline dangerous command, with only a logger.warning() left behind. Same bug class as NousResearch#30882 (P0, fixed in NousResearch#34497) -- an execution surface the approval system doesn't recognize defaults to trusted-by-omission instead of trusted-by-explicit-config. Unlike NousResearch#30882 this is a plain omission, not a regression: kanban workers are separate subprocesses (not threads sharing contextvars), and the approval-context question was simply never wired in when kanban dispatch was built. hermes_cli/kanban_db.py's worker-spawn env construction sets 15+ env vars (HERMES_KANBAN_TASK, HERMES_KANBAN_WORKSPACE, etc.) but never any of the four flags tools/approval.py checks for a non-interactive context (HERMES_CRON_SESSION, HERMES_GATEWAY_SESSION, HERMES_SESSION_PLATFORM, HERMES_INTERACTIVE), so all three approval-gate functions (check_dangerous_command, check_all_command_guards, check_execute_code_guard) fall through to bare auto-approve. Mirrors the proven cron_mode pattern exactly: - HERMES_KANBAN_SESSION env flag set at worker spawn time - approvals.kanban_mode config (default deny, matching cron_mode) - _get_kanban_approval_mode() mirrors _get_cron_approval_mode() verbatim - A parallel deny-by-default branch added to all three call sites, alongside the existing cron branch -- not replacing it approvals.kanban_mode: approve remains available for users who've deliberately decided to trust their kanban workers, same as cron_mode. Verified against the real, unmodified functions (not mocked): captured the actual env _default_spawn() builds for a real task in a scratch kanban DB (subprocess.Popen intercepted, not launched), confirmed all four approval flags were absent, then fed that exact env into a fresh process calling the real check_dangerous_command() -- confirmed auto-approved pre-fix, blocked post-fix, and confirmed kanban_mode: approve restores opt-in pass-through. kanban_mode lives under approvals (not the kanban: config block) -- that block has a known, separately-filed duplicate-key bug (NousResearch#55779) that silently drops the first of two "kanban" keys in the same DEFAULT_CONFIG dict literal. Fixes NousResearch#55945
…er path The kanban policy branches were dead code for real workers: cli.main exports HERMES_INTERACTIVE=1, so a spawned `hermes ... chat -q` worker took the interactive path before ever reaching them, and _default_spawn copied the dispatcher's full environment, letting inherited gateway/ask/cron markers reroute worker approvals into prompts and pending queues no one watches (NousResearch#63183). Three coordinated changes: HERMES_KANBAN_SESSION is decided before all ambient context inference in the three gates, with both arms returning; the spawn env scrubs the seven ambient markers alongside the existing HERMES_TUI pop; worker startup no longer self-marks interactive when the kanban marker is set. tests/tools/test_kanban_worker_real_path.py builds the worker env with the real _default_spawn and drives the real gate entry points: 12 of 14 cases fail on the previous code, all pass now. Existing approval suites pass unchanged. Also fixes three pre-existing test bugs surfaced by the full sweep, each verified pre-existing at ef3ff3f15: a /tmp symlink hardcode, a missing darwin zombie probe in a _pid_alive test mirror, and a timing flake under parallel load. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PnMCvi2vXqfs996AjVeF2F
… env-isolation gate The completeness test added upstream force-classifies every dispatcher kanban env var as identity (scrubbed from delegated children) or behaviour-only (inherited). The unattended-session marker must be inherited: a delegate_task child of a kanban worker is still unattended, so scrubbing the marker would reopen the dangerous-command auto-approve gap one fork deeper.
Treat in-process cron jobs as independently owned approval contexts while keeping delegated children inside the parent worker policy. Cover the scheduler ContextVar thread hop in both policy directions. Co-Authored-By: GPT-5.6 SOL <noreply@openai.com>
The new mixed-context test called clear_session_vars in its finally block. That pins every session var to "" which is the "explicitly cleared" state, and get_session_env treats that as authoritative and stops falling back to os.environ for the rest of the process. test_cron_session_unaffected_by_kanban_changes sets HERMES_CRON_SESSION as a plain env var, so once the mixed-context test had run, _is_cron_approval_context returned False there and the dangerous command auto-approved. Passed alone, failed when the two files ran together. reset_session_vars restores the _UNSET sentinel instead, which is what a test teardown wants: it leaves the context looking like the vars were never bound, so the env fallback works again.
benjaminperry6@yahoo.fr -> benperry6, who authored the cron-policy follow-up on this PR. The attribution check fails on any commit email it cannot resolve to a GitHub account, and that address is not public on his profile.
Retain the contributor's protocol-violation DB foundation while the current-run receipt implementation follows separately. Generated Paperclip evidence and stale file snapshots are excluded.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
Persist one metadata-only tool receipt per current task run so both native Hermes and Codex app-server workers can prove successful material work before completion. Reject failed results, preserve the running task, and close the cross-process notification race with a bounded read. Co-Authored-By: GPT-5.6 SOL <noreply@openai.com>
Track one deterministic progress lease independently from stream/activity liveness. Claims, verified tool executions, and durable board transitions renew progress; model output and heartbeat commentary do not. An expired lease is detection-only in this commit: retain the exact task, claim, PID, and run, emit a bounded no_progress_deferred receipt, and never signal, requeue, or count failure without a separate settlement authority. Propagate the timeout and passive receipt through dispatcher, CLI, gateway, dashboard, Desktop/TUI notifications, diagnostics, config, and docs.
Nested hermes chat --source tool from a Kanban worker inherited HERMES_KANBAN_TASK and could kanban_complete the parent card. Strip lifecycle ownership from subprocess env and drop it at chat startup unless the process is the dispatcher worker.
Port only the exact Linux birth/group identity, retained fences, process-group quiescence, full-state CAS, stale-attempt refusal, and dashboard writer guard required to prevent bypass. Exclude stale parser, config, main, TUI, broad dashboard, and generated snapshots.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
Model the dispatcher lifecycle in tool tests by supplying synthetic Linux worker identity, releasing quiesced fences, and recomputing successors on the next simulated tick.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
Keep the progress implementation from 95fe96f while supplying exact synthetic worker identity to lifecycle tests and explicitly clearing it for custom/remote spawn cases.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
Route lifecycle fixtures through exact synthetic identity, retain complete authority tuples during reclaim, and assert fail-closed behavior for forged dashboard claims.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
Pass the one-shot worker nonce with --kanban-worker-launch=<value> so urlsafe values beginning with '-' cannot be misparsed as another option.\n\nCo-Authored-By: GPT-5.6 SOL <noreply@openai.com>
alt-glitch
added
type/bug
alt-glitch
added
comp/plugins
Contributor
Diff too large for automated batch review (95 changed files, +11063/-890) — recommending manual human review. |
Contributor
Author
|
Closing this draft because it bundled several independent Kanban fixes together with unrelated carried commits, which made the change impractical to review. The relevant work remains available on the existing issue-specific PRs:
No maintainer action is requested on this superseded draft. Future follow-ups will stay narrowly scoped to the corresponding source PR. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Integration
057dcdf236f8a6a26721c10fcc6ccb72726e272a9617282e,49f1d789,e19ff6c4,95fe96f6,aea87a4d, and507539d6semantically via their cherry-picked equivalentsExplicit exclusions
.paperclip_artifactsor generated files6c5bf42d3eand no stale parser/config/main/TUI/dashboard snapshots fromfc26d964Verification
All Python tests used the canonical
scripts/run_tests.shwrapper.git diff --check origin/main...HEAD: passedOne broad run exposed a leading-dash nonce argparse flake; the source was fixed by binding the nonce as
--kanban-worker-launch=<value>, then both the focused regression and the full broad sweep passed with retries disabled.Not run
No live activation or live Kanban canary was run in this worktree.