Skip to content

fix(agt): validate Guardian decision responses - #249

Open
mjatin2007 wants to merge 2 commits into
GenAI-Security-Project:integrationfrom
mjatin2007:fix/validate-guardian-response-envelope
Open

mjatin2007 wants to merge 2 commits into
GenAI-Security-Project:integrationfrom
mjatin2007:fix/validate-guardian-response-envelope

Conversation

@mjatin2007

@mjatin2007 mjatin2007 commented Oct 10, 2026 •

Copy link
Copy Markdown

What changed

Validates parsed, correlated Guardian decision responses against the canonical response-envelope.json schema and its method-specific AcsResult branch before the host adapter accepts them. A canonical-schema or method-result violation is audited and denied even under on_decision_failure: proceed; inability to run the local validator remains a host-side failure and follows the negotiated posture.

This follows the accepted issue's fail-closed direction, but under proceed it deliberately differs from the literal current §6.4 treatment of a malformed response. Validation covers the complete canonical response envelope, not only deny.reasoning. This draft should not be treated as conformant until §6.4 is clarified, and it should not merge ahead of that clarification unless maintainers direct otherwise. Response-signature verification is separate and remains pending in #229.

Which issue does this implement

Part of #182

  • This is an editorial correction (typo, grammar, link, formatting) with no change in
    meaning, so it needs no issue

Base branch

  • integration, because this touches the specification, schemas, a reference
    implementation, an adapter, tests, or CI
  • main, because every changed path is on the documentation lane allowlist in
    CONTRIBUTING.md

Type of change

  • Specification change (schema, hooks, events, AgBOM)
  • Reference implementation or adapter
  • Documentation
  • Tooling or CI
  • Governance (licensing, security policy, contributor docs)

I tested this

  • I synced my branch with the base branch before opening this
  • uv run pytest -v passes on my machine
  • uv run mkdocs build --strict passes on my machine

The AGT validation also passes on the repository's pinned Bun 1.3.11: TypeScript succeeds, and the complete Bun suite reports 1,123 passed, 1 skipped, 0 failed across 68 files.

Checklist

  • Commits are signed off with git commit -s (required by the DCO)
  • Prose follows STYLE.md
  • No secrets, tokens, or internal URLs in the diff

Security

  • This change has no security impact

Security impact: schema-invalid or method-invalid Guardian responses are blocked and audited. Local validator unavailability remains posture-governed and is attributed to the host rather than the Guardian.

@acs-board-reconciler acs-board-reconciler Bot moved this from Needs triage to In progress in ACS Project Tracker Oct 11, 2026
@mjatin2007

Copy link
Copy Markdown
Author

Hi @shivang-sec and @President, may I check how you’d prefer to sequence this? Section 6.4 on integration still treats a malformed response as a decision failure governed by on_decision_failure, while this draft intentionally denies a parsed schema-invalid response under either posture. Would you prefer that I open a Discussion and propose the specification clarification first, or keep this draft open for implementation review in parallel? I’m happy to follow either route.

@shivang-sec

shivang-sec commented Oct 23, 2026 •

Copy link
Copy Markdown

Thanks for checking, @mjatin2007 . I’d lean toward opening a Discussion for the Section 6.4 clarification first, while keeping this draft open for implementation review in parallel. That should let us resolve the intended behavior for parseable but schema-invalid Guardian responses before merging a change that deliberately differs from the current wording.

I agree with keeping protocol violations distinct from technical failures, with the exact fail-closed behavior and its relationship to on_decision_failure made explicit in the spec. For #182, I think validating the incoming Guardian response against the canonical response-envelope.json schema at the host/client boundary is the right approach, rather than duplicating field checks in validateDecision().

I’ll defer to @President on the preferred sequencing before we treat the behavior as settled.

@President

Copy link
Copy Markdown

@mjatin2007 @shivang-sec - I think no new Discussion needed: same question is already open in #32 and in Rock's #308.

In #308 I proposed that a reply that breaks the spec, e.g. a missing required field, is DENY, not a decision failure. That is what this draft already does.

Until the spec's §6.4 text changes, I'd note in the host adapter's README that this is a stricter reading of §6.4. #182, the issue this draft fixes, is already accepted. Once #229 adds response signatures, I'd check the signature first (a bad one is DENY), then the schema.

@mjatin2007

Copy link
Copy Markdown
Author

@President @shivang-sec I added the requested README clarification that the adapter intentionally takes a stricter reading of the current §6.4. I left signature-before-schema validation for #229. The update is in 3f4debc. Thanks!

@rocklambros

Copy link
Copy Markdown
Collaborator

Hi Jatin, OWASP requires a sign-off on every commit, and 3f4debc on this PR doesn't have one. The dco check we're adding in PR #407 will fail it.

Please run these commands:

git checkout fix/validate-guardian-response-envelope
git fetch https://github.com/GenAI-Security-Project/agent-control-standard.git integration
git rebase --signoff FETCH_HEAD
git push --force-with-lease

They add a Signed-off-by: line to each commit and move your branch onto the latest integration. On a copy of your branch they ran with no conflicts. Thanks!

Signed-off-by: Jatin Mannepalli <mjatin2007@users.noreply.github.com>
Signed-off-by: Jatin Mannepalli <mjatin2007@users.noreply.github.com>
@mjatin2007
mjatin2007 force-pushed the fix/validate-guardian-response-envelope branch from 3f4debc to 625efb4 Compare November 1, 2026 01:03
@mjatin2007

Copy link
Copy Markdown
Author

Thanks, Rock. I rebased the branch onto the current integration head with --signoff and updated it using an exact --force-with-lease. Both PR commits now include my Signed-off-by trailer.

@President

Copy link
Copy Markdown

@mjatin2007 thanks, the README note is exactly what I meant. Looks good to me.

@drk-ram drk-ram left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Security peer review of commit 625efb48f91276239a7961e30a6cfb96b6be9df0.

No exploitable security vulnerability found in the changed code. Decision results are checked against both the canonical response envelope and the method-specific result contract. Schema-invalid replies and recognized refusals take the deny path regardless of the negotiated delivery-failure posture; local validator unavailability remains a separately audited host fault.

Validation at that exact commit:

  • Directly changed decision-client and failure-posture suites: 91 passed, 0 failed.
  • Broader host-adapter and affected host suites: 393 passed, 53 failed across 446 tests. The 91 focused tests are included in this broader count, not additional tests.
  • Comparison against the exact pre-change base bac52e1c63ba479e7193b9a33d6b40ca5c4e046a: 382 passed, 53 failed across 435 tests on the same Windows host. The broader run therefore is not a clean pass, and the same failure count is present without this change.

I am leaving a non-blocking comment rather than an approval because the broader affected suite is not green in this environment. I found no security issue in this change that warrants a request for changes.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: In progress

Development

Successfully merging this pull request may close these issues.

5 participants