samples: Python Guardian agent plus FastMCP client instrumentation - #78
Conversation
Minimal ACS v0.1 Guardian sample (stdlib-only HTTP: handshake/hello, system/ping, steps/toolCallRequest with pluggable policy, per-session hash chain, fail-closed posture incl. policy errors) plus a FastMCP client wrapper enforcing Guardian verdicts pre-execution (allow proceeds; deny/modify/ask/defer/unreachable deny closed). 14 stdlib-only tests (python -m pytest samples/python-guardian-fastmcp -q). Limits stated in README (no signature verification, no trace emission, no modification loop). Signed-off-by: angelnicolasc <angelnicolasc@users.noreply.github.com>
hooks/tool-call-request.json requires every argument to be an object with a value key, so a provenance record can attach per argument. The sample emitted bare scalars, which fails validation against the schema it claims to follow. build_tool_call_envelope now applies the wrapper on the way out, so callers keep passing ordinary mappings. The Guardian rejects a non-conformant arguments object with -32600 rather than handing it to a policy, and unwraps to plain values for the policy layer. Policies may now return reason codes as a third element. The previous code labelled every deny destructive_tool, including path-escape denials, which put a wrong machine-readable code on the wire. Signed-off-by: angelnicolasc <angelnicolasc@users.noreply.github.com>
…nd run them test_sample.py covers behaviour. It could not have caught the argument shape, because a suite that agrees with itself about a wire format proves nothing. test_conformance.py loads specification/v0.1.0/ off disk and validates every envelope the sample emits, so drift fails here rather than in somebody else's integration. One test pins the old broken shape as a regression guard. Those tests assert the response branch before validating it. The response envelope accepts an error alongside a result, so conformance alone passes on a Guardian that never reached its policy, which is the accept-and-ignore shape issue GenAI-Security-Project#31 describes on the Guardian side. Neither suite ran anywhere. testpaths in pyproject.toml scopes the deploy gate to tests/, and CONTRIBUTING.md warns that a suite outside it will 'pass review looking like coverage it does not provide'. samples.yml is path-scoped, holds no permissions beyond contents: read, pins its actions by SHA, and installs jsonschema in a job nothing else depends on rather than adding it to uv.lock. Signed-off-by: angelnicolasc <angelnicolasc@users.noreply.github.com>
Every line opened with a hash, so GitHub rendered the whole file as a stack of headings. Rewritten as prose per STYLE.md, with the argument wrapper documented and the signature gap tied to issue GenAI-Security-Project#70 rather than left as a general disclaimer. Signed-off-by: angelnicolasc <angelnicolasc@users.noreply.github.com>
f1b31e7 to
0d89803
Compare
|
Thank you for this, and for the way it is framed. Acknowledging quickly so you are not waiting on silence, with the caveat that the OWASP relaunch call runs today and considered review will come after it. A few things worth saying plainly, because they are not generic praise. You answered an ask before it was made. The call later today asks for exactly this: the reference implementation runs on Bun, and Python was named as open and unclaimed. You got there first, from reading the repository. You targeted The fail-closed posture is the interesting part. The AGT reference implementation defaults to On your question, which was the right one to ask. The distinction the team has been drawing is that a reference implementation carries policy and demonstrates the contract end to end, while an adapter slots into an existing harness and emits the wire. A Guardian with a pluggable policy plus a client wrapper is the first shape, not the second, so One heads-up on review scope. The pull request adds Two things that would help while this waits. Say a word about what the conformance suite asserts and what it does not, since the project is trying to be careful about the difference between emission conformance and requirement-by-requirement conformance. And if you have run this against a real FastMCP client rather than only the tests, that is worth writing down. Labelled |
|
Cross-linking for visibility: the kick-off onramp issues are now filed, and #86 asks for exactly this work. It went up after your pull request, so this is a record that you got there first rather than a duplicate request. |
What changed
New
samples/python-guardian-fastmcp/: a minimal ACS v0.1 Guardian agent in Python (handshake/hello, system/ping, steps/toolCallRequest with a pluggable policy, per-session hash chain, fail-closed posture) plus a FastMCP client wrapper enforcing Guardian verdicts pre-execution. It fills the v1 roadmap gap with the smallest reviewable thing first.The sample code imports nothing outside the standard library. The tests need pytest, and the conformance suite needs jsonschema.
Still a draft. One question below I would rather have answered before this is worth anyone's review time.
Where should this live?
I created a top-level
samples/, which I now think was presumptuous.reference-implementations/already exists, and #22 is addingadapters/. Three parallel trees for adjacent ideas is a structure decision that belongs to maintainers, not to a first-time contributor. Happy to move it anywhere, including intoreference-implementations/, or to drop it if the tree is already covered.Type of change
.github/workflows/samples.yml)Specification changes
None. Nothing under
specification/ordocs/spec/is touched. The sample validates against those schemas rather than restating them.Breaking for implementers? No.
Tests
test_sample.pycovers behaviour: fail-closed posture, hash chain, verdict handling.test_conformance.pyloadsspecification/v0.1.0/off disk and validates every envelope the sample emits against it, the wayreference-implementations/agtdoes. 26 tests.That second suite is the reason this is worth looking at. The first revision of this branch emitted
"arguments": {"path": "/tmp/x"}, bare scalars, which failshooks/tool-call-request.json— that schema requires each argument to be an object with avaluekey so provenance can attach per argument. The behaviour suite passed the whole time, because a suite that agrees with itself about a wire format proves nothing. The conformance suite now pins the broken shape as a regression guard.Those tests assert the response branch before validating it.
response-envelope.jsonaccepts anerroralongside aresult, so conformance alone still passes on a Guardian that never reached its policy — the accept-and-ignore shape #31 describes on the Guardian side. I hit exactly that while writing them.Neither suite would have run anywhere.
testpathsscopes the deploy gate totests/, and CONTRIBUTING.md warns that a suite outside it will "pass review looking like coverage it does not provide."samples.ymlis modelled onreference-implementation.yml: path-scoped,permissions: {}withcontents: readper job, actions pinned by SHA, and jsonschema installed in a job nothing else depends on rather than added touv.lock.Checklist
git commit -s(required by the DCO)#-prefixed lines that GitHub rendered as all headings; rewritten as prose)uv run mkdocs build --strictpasses (untouched — the sample is outside the site build)integration, per the branching change in Install the contribution governance for the OWASP re-launch #64Security
It has a deliberate one, in the safe direction: the sample denies on policy errors, unknown verdicts, unknown methods, version mismatch, non-conformant arguments, and Guardian-unreachable. The server binds loopback by default.
The limits are in the sample README rather than buried:
modify/ask/deferare treated as deny, there is no trace emission, and request signatures are accepted but not verified. That last one is the same gap #70 tracks against the reference Guardian — no HMAC-SHA256 over the JCS-canonicalized envelope with an HKDF-derived per-session key. I did not want to add a second Guardian carrying that hole silently, so the README names it and points at the issue. If it is worth doing here, say so and I will implement it in this branch rather than shipping the gap twice.