feat(dotnet): add approval chain parity#3363
Conversation
Signed-off-by: Carlos Hernandez <carloshvp@gmail.com>
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
PR Review Summary
Verdict: AI review comments are untrusted advisory output. The summary reports workflow-generated completion status only, not model-authored pass/fail claims. |
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
imran-siddique
left a comment
There was a problem hiding this comment.
Reviewed the .NET slice against the Python reference coordinator. Strong work. Every ambiguous or unexpected path fails closed: ResolveCoreAsync denies on null transport, timeout, transport protocol error, and generic exceptions, and ValidateForExecution rebinds the action digest, policy version, and chain version, consumes exactly once (verified by the 24-thread concurrency test), and verifies the hash-linked entry chain. LLM entries are correctly advisory-only. Tests meaningfully exercise deny, timeout, transport failure, tamper, expiry, cancel, and unverified-webhook paths. Scope stays within agent-governance-dotnet, no secrets.
Notes before this is relied on in production:
- WebhookApprover.ValidateEndpoint blocks cloud metadata and link-local hosts but not localhost, 127.0.0.0/8, or private ranges, and does not guard against DNS rebinding. Since this is advertised as an SSRF control, please tighten it or document the limits clearly.
- ResolveAsync eagerly consumes the approval, so a caller that later calls ValidateForExecution is denied as approval_consumed. Consider documenting ResolveAsync as terminal, or not consuming there.
- The no_required_approval_stage divergence from Python vacuous-allow is deliberate, documented, and matches Go. Good call.
Approving on the logic; item 1 is worth a follow-up.
|
Filed the SSRF hardening follow-up from my review as #3369 so it does not get lost. Not a blocker for this PR. |
Description
Adds the .NET language-parity slice for action-bound
require_approvalapproval chains tracked in #3083.The new
AgentGovernance.Approvalssurface provides:The zero-required-non-advisory-stage path intentionally denies with
no_required_approval_stage. This avoids the current Python vacuous-allow behavior and matches the safer Go parity implementation in #3242. The behavior is documented in the .NET README and covered by regression tests.Type of Change
Package(s) Affected
Scope:
agent-governance-dotnet/only.Checklist
ruffis not applicable; scopeddotnet format --verify-no-changespassed)dotnet test AgentGovernance.sln)Additional validation:
dotnet format AgentGovernance.sln --no-restore --verify-no-changes --include src/AgentGovernance/Approvals tests/AgentGovernance.Tests/ApprovalProtocolTests.csdotnet build AgentGovernance.slnsucceedsdotnet test AgentGovernance.slnpasses 806 testsAttribution & Prior Art
Prior art / related projects:
AI Assistance
OpenAI Codex assisted with implementation, regression tests, and validation. The draft remains open for contributor and maintainer review.
IP, Patents, and Licensing
Related Issues
Part of #3083.