fix(approvals): bypass approval for background job control - #1817
Merged
Conversation
check_background_job is a shell-coupled tool (ToolAccessPolicy.IsShellCoupledTool) that routes through ShellApprovalMatcher, whose IsFailClosedOnPersonal returns true unconditionally. That forces ToolApprovalMode.Approval for every Personal invocation with no explicit override — including pure read-only status queries (Cancel=false) scoped to the caller's own jobs. On a non-interactive turn (reminder/webhook), nothing can answer the prompt, so the session wedges waiting on an approval that never arrives. This is the repro-only half of a red-green change; the fix follows separately. Three cases: - status query (interactive): must not require approval - status query (non-interactive automation): must not require approval - cancel: MUST keep requiring approval (mutation)
…ries check_background_job is a shell-coupled tool that routed through ShellApprovalMatcher, whose IsFailClosedOnPersonal returns true unconditionally. Every Personal invocation therefore forced ToolApprovalMode.Approval — including read-only status queries scoped to the caller's own jobs. On a non-interactive turn (reminder, webhook, sub-agent without an approval bridge) nothing can answer the prompt, so the session wedged waiting on an approval that never arrives. Add BackgroundJobApprovalMatcher: - status query (Cancel=false): IsFailClosedOnPersonal=false, so it falls through to the profile default (Auto) and runs without a prompt. The job manager enforces an exact SessionId+Audience+Boundary match on QueryBackgroundJob, so a status query cannot read another session's jobs. - cancel (Cancel=true): IsFailClosedOnPersonal=true, stays approval- gated — killing a running process is a mutation. Launching a background job is unaffected: it is a shell_execute call with _background:true, which still routes through ShellApprovalMatcher and the ordinary shell approval gate. Also add a pipeline-level regression test driving a non-interactive, VerifiedAutomation turn through the executor.
…tatus query Drives a non-interactive (InteractiveApproval=Unavailable), Personal, TrustedInstance turn through DispatchingToolExecutor: - status query (Cancel=false) must not throw ToolApprovalRequiredException - cancel (Cancel=true) must still throw Verified red against the pre-fix ToolAccessPolicy and green with BackgroundJobApprovalMatcher.
…st class Adversarial review findings (pr-review-specialist on PR #1817): HIGH: IsCancelRequest accepted a strict subset of the argument shapes the generated tool binding accepts. The binding falls back to case-insensitive and normalized key matching and coerces JsonElement/string values, so {"cancel": true} or {"Cancel": "true"} bound as a real cancel while the matcher treated them as a status query and auto-allowed — cancel fired with no approval. Delegate cancellation detection to ToolArgumentHelper.GetBoolStrict, the same helper the binding uses, so the approval decision and execution decision agree by construction. MED: add a 13-shape cancel value matrix test (CLR bool, JsonElement bool, JsonElement string, CLR string, null, garbage, numeric) asserting matcher parity with the binding, plus key-variant tests (lowercase, normalized, trailing-space) and a ToolOverrides-precedence test proving an operator can still force Approval for status queries. LOW: move the pipeline-level executor regression into a dedicated Akka.Hosting.TestKit-based class (CheckBackgroundJobApprovalTests) instead of an inline ActorSystem + nested actor in DispatchingToolExecutorTests.
Collaborator
Author
|
Closes #1818 |
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
1 participant
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.
Why
closes #1818
shell_executeapproves the process before it creates a background job.check_background_jobcontrols that existing job. It does not create a new shell invocation.The old path sent this tool through the approval system. An approval retry could fail and leave an incomplete tool-call history.
Contract
shell_executeapproval.check_background_job.Implementation
The access policy returns an authorized job-lifecycle decision before the approval gate.
The change removes
SelectShellApprovalMatcher. Real shell commands still useShellApprovalMatcher.The executor test supplies an approval service that throws on every call. Both status and cancellation pass without contact with that service.
Verification