fix(kanban): harden reliability and sensitive execution boundaries - #93021
fix(kanban): harden reliability and sensitive execution boundaries#93021coloradeo wants to merge 9 commits into
Conversation
This PR currently contains no diff against its base branch (0 changed files, +0/-0), so there is no code content to review. If changes were expected here, the branch may be pointed at the wrong base, the commits may already be merged upstream, or a force-push may have emptied it — worth re-checking the compare target before marking ready. |
Launch sensitive workers and their subprocesses with deny-by-default environments, resolve model credentials through process-private scope, and block unmediated execution capabilities before they can escape policy. Add synthetic-canary startup, provider-auth, normal-worker, and indirect-expansion regressions.
andrexibiza
left a comment
There was a problem hiding this comment.
Reviewed exact head 765f2556adcd6240fd54e4bb6520cf415e1fb2ab (base recorded by the PR: f293e7206b4ddd66042329442c6afebc19a8808d; current main at review: 654d537088aa499e14b2a859534652dc80b16e7f). I inspected the sensitive-worker spawn/env boundary, kanban_sensitive validator, all three tool-dispatch paths, plugin hook semantics, artifact redaction/preservation, workspace preflight/lease fences, the focused sensitive tests, existing discussion, and the related credential-containment graph.
There is one security blocker in the new policy composition.
hermes_cli.plugins._get_pre_tool_call_directive_details() still preserves the existing “first valid block/approve directive wins” behavior. This PR adds the kanban_sensitive validator to that same ordered result stream, but the function returns immediately when it sees an earlier approve directive. That early return bypasses the later sensitive-mode check that requires a policy == "kanban_sensitive" validator. _resolve_block_from_details() can then turn that earlier approve into None after human approval, and _dispatch_pre_tool_call_hooks() only runs validators carried in the already-returned directive. If the sensitive validator was registered later, there are none. In other words, an ordinary supported pre-tool plugin can accidentally convert the sensitive floor into an order-dependent policy: [approve, kanban_sensitive validate] + approval permits a call that validate_final_tool_args() was specifically meant to forbid, including arbitrary terminal commands or another unmediated capability. A preceding block remains safe; approve is the bypass.
The current regression set covers [modify, validate], missing-validator fail-closed, and hook exceptions, but not this composition. Please make the sensitive validator non-shadowable by block/approve ordering: collect/require the sensitive validator independently of the first directive (or move the mandatory sensitive floor outside the plugin result race), then run it over final post-modification args whenever a call would otherwise proceed. Add adversarial tests for at least [approve, validate] and [modify, approve, validate] with the approval gate mocked to approve, proving the fixed runner remains the only terminal path and unmediated tools still cannot execute.
Topology/provenance: this is broader than, but same defect-class lineage as #55600 by @necoweb3 and the Kanban half of #82936 by @neo-wanderer; those objects established the parent/default-profile credential-inheritance failure. #83565 is the canonical child-process credential-inheritance tracker and should treat this as the wider Kanban successor/interlock rather than a parallel untracked fix. #82591 is complementary, not duplicate: it defines the eventual zero-authority/whole-worker containment architecture. This PR is valuable host-worker hardening, but because the worker still authenticates models in-process and loads the normal plugin runtime, it should not be treated as satisfying #82591's networkless/zero-authority boundary.
Repository-state gate: exact-head CI, Docker, and Nix are all action_required with no executed hosted jobs at this SHA. The PR body says to keep this draft until independent QA/CI, but the live PR is currently non-draft. Also, current main is 185 commits ahead of the recorded base and has changed several touched authority surfaces including agent/agent_runtime_helpers.py, agent/tool_executor.py, hermes_cli/plugins.py, hermes_cli/config*.py, plugins/kanban/dashboard/plugin_api.py, and tools/environments/local.py. After fixing the policy-composition hole, rebase onto current main and rerun the exact-head sensitive/ordinary-worker controls before treating this as merge-ready.
I found no additional blocker in the deny-by-default sensitive subprocess environment, fixed-runner argv/resource validation, exact-secret log/artifact redaction, atomic containment, or the new Git/workspace preflight fences on this head.
Remediated immutable candidate
765f2556adcd6240fd54e4bb6520cf415e1fb2abc9fd7dca141a1ddb83a99e55a5221801946cef7e5cbe438511f5059a2b6e54679fa9a5f5a4e3bc15/ tree655237207f7d58188f21b5cef736309540ef4d54.coloradeo:epic/kanban-reliability-hardening-20260823→NousResearch:main.Scope
Synthetic-canary regression evidence
test_sensitive_worker_spawn_uses_output_redaction_wrapperfailed becauseCANARY_PROVIDER_API_KEYremained in the captured_default_spawnenvironment. Only a synthetic canary was used; no real credential was read or printed.git diff --checkpassed.npm run checkwas attempted but unavailable in this checkout because JavaScript dependencies are not installed (tsc/esbuildnot found, exit 127); it is not reported as passed or as a candidate regression.Required next gate
Fresh independent QA must inspect commit
765f2556adcd6240fd54e4bb6520cf415e1fb2aband treec9fd7dca141a1ddb83a99e55a5221801946cef7edirectly, re-run the synthetic-canary and normal-worker controls, and confirm local/fork/PR parity. Keep the PR draft until that QA and subsequent CI/update gates pass.