Repository navigation
Tighten forward compat for 0.x versions - #255
aaronormerod wants to merge 2 commits into
Conversation
Closes GenAI-Security-Project#196 Signed-off-by: aaronormerod <137795541+aaronormerod@users.noreply.github.com>
|
This pull request's description says it closes #196, which is on the project roadmap. When it merges, GitHub closes that issue and the roadmap counts it as delivered. Please confirm the change delivers the whole issue before merging. |
acs-board-reconciler
Bot
moved this from Needs triage
to In progress
in ACS Project Tracker
The embedded schemas follow integration after GenAI-Security-Project#63, GenAI-Security-Project#258 and GenAI-Security-Project#261. The parity recording stores the git object IDs of the deciding TypeScript paths instead of a commit, so a squash merge no longer makes TestRecordingIsCurrent fail. The paths include agt.lock and the agt-bridge package.json, which pin the AGT commit and SDK. The recorder and the test read them from one list; the test fails when the recording names other paths or was made from another corpus, and the recorder refuses uncommitted changes to them. A test checks that the Go workflow runs on every deciding path. agt/README.md tells a TypeScript contributor when to re-record. A test requires agt-native.lock and agt.lock to pin the same AGT repo and commit. The native build fails when AGT at that commit changes ffi.rs, which agteval/acs.h is written from, or fetches another opa than agt-native.lock pins. Under the agt evaluator, policy.opa_timeout must be shorter than protocol.decision_timeout; guardian.yaml ships 4s. Otherwise a slow opa lets the agent stop waiting before AGT's deny arrives (GenAI-Security-Project#269). make load measures either evaluator under concurrent clients, selected by EVALUATOR like the other targets. Under agt each decision runs one opa process; on a 20-core host decisions stop rising near 16 concurrent ones, and at 256 they reach the 5s timeout. guardian.yaml now ships limits.max_engine_calls 32, and docs/load.md records the results. The load run found that an engine call gave its slot back only after its answer was sent, so a client's next step could be refused engine_saturated with no call running; callEngine now releases the slot first, also after a panic. When the host adapter's audit log cannot be written, the record goes to stderr as one JSON line with the final disposition, and the refusal's reasoning says whether the record went to stderr or nowhere. The audit file's close error now counts. The Codex and OpenCode hooks pass their own stderr. The §6.3 row says that ALLOW at a hook that permits no DENY is an interpretation (GenAI-Security-Project#270), and the docs say only the adapter library reaches those hooks. Dependabot proposes Go updates, one pull request per dependency across both modules. The Go workflow runs weekly, so govulncheck sees advisories for the toolchain that Dependabot does not move, triggers on the agt-bridge package.json, and no longer fetches full history. A test checks that the method table permits DENY exactly where hooks.md does, and fails on a Decision line it cannot classify. The coverage floor counts every package except commands, examples and test helpers. The Guardian meets the expectations GenAI-Security-Project#256 sets for reference implementations, mapped to tests in docs/conformance.md. audit.max_file_bytes and audit.max_files rotate each audit file; guardian.yaml keeps at most 1 GiB per log; max_file_bytes needs max_files of at least 1, so a rotation never deletes the live log. make load can set the rotation and fails if the log drops, splits or reorders a record or exceeds its bound; docs/load.md records a run and the cost of long sessions. storage.provider names the session store. The HMAC secret file holds base64 of at least 32 bytes. key_id is a configured label with no default, an unknown key_id is refused (GenAI-Security-Project#118, GenAI-Security-Project#272), and the hooks and the conformance harness take it as a flag. A toolCallResult whose request_id_ref matches no request is recorded (GenAI-Security-Project#272). Decisions from other pull requests that the Go code contradicted: - A shell command's URL ends only at ASCII whitespace; U+FEFF or U+00A0 in the userinfo no longer hides the host curl reaches (GenAI-Security-Project#268). - Versions are compatible when they share X.Y under 0.x; the handshake negotiates only such a version, and a step must match it (GenAI-Security-Project#196, GenAI-Security-Project#255). - A policy target that is not a string is denied evaluation_failed; the TypeScript fix is in GenAI-Security-Project#229, so two recorded cases are listed as ahead of the recording until it is re-recorded. - The host adapter records every decision failure under either posture, with its class, handshake failures included (GenAI-Security-Project#37). - The host adapter denies under any posture a signed error other than -32603, an unverified answer and a malformed answer; the docs mark this as an interpretation of §6.4 ahead of GenAI-Security-Project#32, which is open. The version rule is marked the same way against §3. - The audit directory is 0700; a symlinked audit path or an audit file open to group or others is refused (GenAI-Security-Project#71). Claude-Session: https://claude.ai/code/session_01DFdYnoBitHhPcUSq7wMWs1 Signed-off-by: Konstantin Trunin <konstantin@trunin.com>
rocklambros
left a comment
rocklambros
left a comment
There was a problem hiding this comment.
Aaron, your new row is the rule issue #196 suggests, and PR #348 and PR #169's Go Guardian already follow it in code. I ran the merged tree on integration 19b1197. Pytest, mkdocs build --strict, typecheck and bun test on Bun 1.3.11 and 1.3.14 give the same counts as integration.
@bar-capsule @afogel, this PR waits on item 5 on issue #32. Your answer decides if it merges as written.
It also sets what PR #229, PR #348 and PR #22 change to. PR #183 belongs on that list too, because its Python Guardian follows the old row and negotiates 0.2.0. I ran each of those PRs, and the row leaves three things open that your answer should cover:
-
Which number does the rule compare? Item 5 used 0.1.4 as its example, but that's a release number.
acs_versionhas stayed0.1.0through every release, andCHANGELOG.md:7says "the handshake cannot tell the old rules from the new". So this row refuses0.2.0and can't see a breaking change that ships under0.1.0. Does a breaking spec change have to moveX.Y? -
Does the rule apply to
params.acs_versionon each step, or only at hello? §17.1 defines-32001only "at handshake". With0.2.0on a step after a0.1.0hello, PR #229 allows the step, and PR #348 and PR #169 answer-32001. Your answer decides what is left of PR #348 and whether PR #229 can close issue #139. -
What does a
0.1.0Guardian put innegotiated_versionfor a client that offers only0.1.1?handshake.json:75says it selects from the client's list. PR #169 and PR #183 send0.1.1. Integration, PR #348 and PR #22 send0.1.0, and PR #22's test requires that. PR #229 refuses the client.
Once you answer, Aaron can push and one of you can approve and merge.
Aaron, please wait for that answer, so you only push once. If they accept your rule, I need one change:
-
specification/v0.1.0/handshake.json:75, thenegotiated_versiondescription, still says "MUST match the client's major version." After your row, §3 refuses a0.2.0client and this line accepts it, so the problem in issue #196 stays for anyone who reads the schema. Replace that sentence with:MUST satisfy the Forward compat rule in Specification §3.The rule then lives in one place, your row. I tried this on a scratch copy and every suite gave the same counts. No test reads this text, so check it with
git grep -n "client's major version" docs specification, which should print nothing.
If Bar or Ariel answer differently, change the text to match their answer.
Please also comment on issue #196, as I asked on PR #253, so I can assign it to you.
drk-ram
left a comment
drk-ram
left a comment
There was a problem hiding this comment.
I checked 99b5908 against #196. The 0.x exception now excludes different minors while preserving patch compatibility and the existing post-0.x rule.
The three specification guard files pass: 10 tests. Reverting the row leaves those guards green, so they do not enforce this wording. A focused text assertion catches the reversion.
This clarifies the contract; it does not implement runtime negotiation.
I could not build the documentation locally because mkdocs is unavailable. I stopped the broader test run after errors; I am not claiming full-suite success.
No blocker in the changed rule.
|
@rocklambros @aaronormerod, following my answer on #196:
@aaronormerod, with that, please make Rock's handshake.json change and the §17.1 row, and this is ready for approval. cc: @afogel |
negotiated_version now defers to the Forward compat rule instead of repeating a major-version match. The -32001 row also covers a step whose params.acs_version is not the session's negotiated_version. Signed-off-by: aaronormerod <137795541+aaronormerod@users.noreply.github.com> (cherry picked from commit 2beeb549317f756fef717a439ce4823691b3bc5f)
|
Done. handshake.json now defers to §3, and the -32001 row covers per-step params.acs_version. git grep for "client's major version" is empty. |
What changed
Narrow the §3 forward compat row: while the major version is 0, require a matching
X.Yand accept any patch.negotiated_versioninhandshake.jsonnow defers to that row. The §17.1-32001row also covers a step whoseparams.acs_versionis not the session'snegotiated_version.Which issue does this implement
Closes #196
Base branch
integration, because this touchesdocs/spec/Type of change
Specification changes
No structural schema change. One description in
handshake.jsonnow points at §3. Matches the usual semver reading for 0.x, and stops a 0.1 implementation from evaluating 0.2 messages under 0.1 rules.Breaking for implementers? Only for one that accepted other 0.y versions, or a step whose version differs from the negotiated one. Both now get
-32001.I tested this
uv run pytest -vpasses on my machineuv run mkdocs build --strictpasses on my machineChecklist
git commit -s(required by the DCO)Security