[Arham-Begani]

Summary

First stage of the Rust port for #88: a Rust AGT Guardian at reference-implementations/agt-rust/ using the existing AGT manifest and policy, mapping.yaml, and ACS v0.1.0 schemas. The existing TypeScript Claude Code and OpenCode shims can send requests to this server. No policy or schema copy is introduced.

The modules separate HTTP/JSON-RPC dispatch, schema validation, SDK integration and egress annotation, verdict mapping, session state, and local audit logs.

Review updates

• Sync the branch with current integration.
• Require the pinned OPA version 0.70.0 at startup. Reject mismatched, missing, malformed and non-UTF-8 version output. CI now installs through the existing frozen Bun lockfile, using its platform package integrity, instead of an unverified npm install. The hoisted linker keeps the OPA package at the documented job path.
• Advertise the SDK's actual OPA evaluation timeout plus 1000 ms for request handling and delivery. The default host deadline is 6000 ms; ACS_OPA_TIMEOUT_MS=30000 produces 31000 ms. Reject an unrepresentable deadline.
• Make all four remaining server logging writes non-panicking, keep the Rust README under the existing Apache licensing catch-all, and add weekly Cargo Dependabot coverage with the existing seven-day cooldown.
• Narrow the parity claims and refresh test counts. The Unicode-aware Rust egress regex is unchanged.

Tested behavior and limits

The checked-in comparison covers nine cases against live Rust and TypeScript Guardians with the same policy: allow, deny, request and result redaction, egress denial, WebFetch allowance, invalid payload, an undispatched method, and handshake. Eight compare complete responses. The handshake compares everything except the intentionally different timeout_config.default_ms, whose positive integer shape is still checked.

This is evidence for those cases, not general parity. Non-object arguments, handshake validation error data, alternative UUID/timestamp formats, warn/escalate verdicts, and evaluation failures are not covered. The Rust SDK collapses absent result_labels and an empty array to the same vector; the Guardian conservatively retains labels for an empty vector. That IFC limitation remains documented.

Validation of this update

• cargo fmt --all -- --check: passed.
• cargo check --workspace --locked: passed.
• cargo clippy --workspace --all-targets --all-features --locked -- -D warnings: passed.
• ACS_OPA_PATH=<locked AGT OPA 0.70.0> ACS_OPA_TIMEOUT_MS=30000 cargo test --workspace --all-features --locked: 6 unit tests and 1 HTTP integration test passed. These include exact-version and deadline regression tests; the integration test checks the negotiated deadline.
• bun tests/compare-guardians.mjs against both live servers: all nine cases passed, with only the documented handshake deadline excluded from equality.
• bun run typecheck in the TypeScript reference: passed.
• uv run mkdocs build --strict: passed on Windows.

The full Python guard suite does not pass on this Windows environment. A local harness outside the contribution shortens oversized test display IDs and selects Git Bash without changing test bodies or parameter values. It completed with 908 passed, 10 failed, 1 skipped. Failures involve Unix executable/shebang stubs, a missing python3 in test-provided shell environments, and Windows symlink privileges. An unadapted run also encounters Windows' environment-variable length limit. WSL cannot start because virtualization is disabled. These are local platform limitations. The subsequent upstream Linux CI passed on this update: Reference implementation (Rust formatting/check/clippy/tests/live comparison and TypeScript checks), Python guards and documentation build, and workflow security scan. Site deployment is skipped for the pull request.

Remaining scope

The Rust host adapter, Inspector, and conformance runner are outside this stage. Only the two tool gates supported by the TypeScript reference are evaluated; no ACS-Core conformance claim is made.

AI assistance: Codex helped implement and test this update. The contributor personally reviewed the focused patch, certified the project's DCO, and authorized the human sign-off on the review-fix commit.

Part of #88.

[shivang-sec]

I completed an independent substantive review of the first-stage Rust Guardian against the ACS v0.1.0 schemas and existing TypeScript AGT behavior. I checked handshake/method declaration and dispatch, failure handling, AGT→ACS verdict mapping, session/hash-chain handling, Rust↔TS parity coverage, and the documented limitations around duplicate request_id, wire chain_hash, signatures, ASK/ask_details, and DEFER.
The known gaps are explicitly documented as first-stage PoC limitations, and the PR does not claim ACS-Core conformance. I also checked the PR CI; all checks are passing.
I did not identify a new substantive finding.
Conclusion: no new finding from my review.

[Mayur021]

I read this stage against the TypeScript Guardian it ports, concentrating on the session store, since that is where both implementations hold state a policy decision actually reads.

The chain behaviour is faithful and worth saying before the finding. The 1,024 retained-session cap matches DEFAULT_MAX_SESSIONS, the ["public"] floor matches emptySessionState, and an evicted session restarting at genesis with seq 1 is the documented upstream choice rather than a port slip, so none of that needs raising. The result_labels empty-vector note in the README is the right call as well.

replace_labels can drop a verdict's labels and still report success.

session.rs reaches for the session with get_mut, so when the id is not present the body is skipped and the function returns Ok(()). The TypeScript counterpart, replaceIfcLabels, goes through ensure, which recreates an absent session and writes the labels into it. The same call therefore persists in one implementation and discards in the other.

The window is real because server.rs releases the store lock between the two calls: append on line 168 hands back the labels, OPA runs as a subprocess, and replace_labels on line 222 takes the lock again. A burst of 1,024 distinct session ids inside that window evicts the session whose verdict is still being mapped. session_id is read straight off params.metadata.session_id and the wire is unauthenticated, which the README already discloses, so distinct ids cost an attacker nothing. It does need the burst, so this is not a one-request trick, but the direction is the unsafe one: labels the IFC gate did propagate are never stored, and the next step on that session reads the floor instead.

The cause looks structural rather than local. This store keeps recency in recent and state in sessions, and the upstream module doc rules that out in one line: "There is no second structure that could fall out of step with this one." Once there are two, replace_labels has to choose between entry().or_default(), which creates a session with no recent entry and so one the cap can never evict, and get_mut, which drops the write. Routing it through the same insert-and-reorder path append already uses, or holding recency in one structure, removes the choice.

Smaller, same file. The eviction line reports the cap but not what it causes. Upstream's message reads "A further step on this session starts a new chain at seq 1, which the Inspector renders as a chain break", and its doc gives the reason: the eviction and the chain break it causes are otherwise two unexplained facts in two different places. Upstream reports eviction twice over, and the second report is checkSessionChainLink in packages/inspector/src/render.ts, which the parity table marks Not yet ported. That leaves this stderr line as the only eviction signal the stage has. Either carry the consequence in the message, or note in the README that the existing Inspector can tail session-context-rust.jsonl, since the entry shape matches.

Neither tests/guardian.rs nor the session.rs test module exercises the cap today. A test that appends 1,025 ids and then asserts the evicted session's labels after a replace_labels on it would pin both points, and would fail on the first as it stands.

[Arham-Begani]

Thanks for the review. I addressed the session-store finding.

append and replace_labels now use the same ensure/touch/evict path, so an IFC verdict can no longer silently lose its labels if the session is evicted while policy evaluation is in flight.

I also added the requested regression coverage using 1,025 sessions, verified the recreated session retains the propagated labels and restarts at seq 1, and expanded the eviction warning to explain the resulting chain break.

I additionally moved the Rust↔TypeScript live parity comparison into CI. All current checks, including Rust tests, clippy, formatting, TypeScript tests, and the live parity test, are passing.

Would appreciate a final review when you get a chance.

[Mayur021]

That is the right shape. Routing replace_labels through ensure_session registers a recreated session in recent as well as in sessions, so it cannot become the one entry the cap can never reach, and funnelling both writers through a single verb is what keeps the two structures in step.

The if to while change is worth having on its own. The eviction line now also says what a reader of the session log will see next, which was the half that was missing.

The regression test pins it at the right place: against the previous commit it fails on assert_eq!(labels, propagated), not on anything about the chain, since the restart at seq 1 was always the intended behaviour.

Since you asked for a final pass, one more parity observation, explicitly not a request for changes. The TypeScript store wraps its eviction reporter in a try/catch, and its own comment says why: a reporter that throws would surface as a Guardian failure on whichever unrelated step happened to overflow the cap, and a Guardian-side failure under the default posture runs the tool ungoverned, so a broken reporter must not cause the thing the cap exists to prevent. The bare eprintln! here panics if the stderr write fails, which poisons the Mutex and makes every later append return Err.

I would not hold the PR for it. The direction is fail-closed rather than fail-open, since a poisoned lock sends evaluate down deny_on_failure, and it needs stderr itself to break. But it is the one place the port still differs from the reference on a point the reference documents deliberately, so it is worth knowing about rather than finding later.

Nothing else from me, and the finding is closed. One thing worth saying plainly so you are not waiting on the wrong signal: I am not a code owner here and this is a comment rather than a review, so it does not move the merge. That still needs one of the names in CODEOWNERS.

[rocklambros]

@Arham-Begani, the session-store fix holds up. replace_labels now goes through ensure_session, and the 1,025-session test passes in CI. I also read eb609e44, which landed just after Mayur's last comment, and the eviction warning can't panic anymore. Most of what I have left is small, but three things need to change before this merges.

1. The Guardian runs whatever opa it finds. policy.rs:18-33 falls back to opa on PATH and only checks that opa version exits 0. The TS Guardian runs the OPA binary pinned in agt/bun.lock by sha512. manifest.yaml:11-14 already records an OPA loader quirk that drops data.json and answers allow for everything, so a developer with a different Homebrew opa can end up with an allow-all Guardian and nothing would notice. Please parse the opa version output and refuse to start unless it says 0.70.0. In the same spirit, the CI job installs OPA with npm install ... agent-control-specification-opa-linux-x64@0.3.1-beta.0, with no integrity check. You can move bun install --frozen-lockfile in reference-implementations/agt above the Rust test step, set ACS_OPA_PATH in the job env: to the binary under agt/node_modules/agent-control-specification-opa-linux-x64/bin/opa, and drop the npm step.

2. The advertised deadline ties with the OPA deadline. server.rs:129 sends "default_ms": 5000, and the pinned SDK's OPA timeout is also 5s. The host starts its timer before OPA even spawns, so on a slow OPA the host gives up first, and with the default posture (main.rs:17, proceed) the tool runs. The README's ACS_OPA_TIMEOUT_MS=30000 makes that gap 25 seconds. Please either advertise the OPA timeout plus about a second, or refuse to start when the OPA timeout isn't below default_ms, and add a unit test. The TS Guardian has the same pair, so I'll file an issue for that side.

3. The parity wording promises more than the nine cases check. README.md:45 says cross-language drift "fails the Reference implementation workflow", and the PR body says all five AGT verdict kinds are mapped. The nine cases use one session, valid ids, and one invalid payload. I compared a few inputs: a non-object argument gets different error text from each Guardian, and a handshake error has data.pointer in TS but no data in Rust (server.rs:119). A urn:uuid: session id or a space-separated timestamp gets allowed by TS and denied by Rust. Warn, escalate, and evaluation_failed aren't covered at all. Adding cases is the better fix, but it's also fine to narrow the wording to "nine cases" and list what isn't compared. Please also refresh the Testing section, since CI now runs 4 tests, not 3 plus 1.

These aren't blockers:

• Four eprintln! calls are still left in server.rs (lines 101, 247, 259, 386). Two of them run while a log mutex is held, so a failed stderr write can poison the lock and stop the audit logs with no message. Using the same let _ = writeln!(io::stderr().lock(), ...) pattern you used in session.rs fixes all four.
• The new LICENSING.md row puts the Rust README under CC BY-SA, but the TS reference's READMEs fall under the Apache catch-all (LICENSING.md:55-57). Unless you meant that, it's simplest to drop the row.
• Nothing will ever propose updates for the new Cargo.lock (301 crates), because .github/dependabot.yml only covers github-actions and uv. Please add a cargo entry for /reference-implementations/agt-rust, copying the existing weekly schedule and cooldown: default-days: 7. It's a .github/ change, so I'll approve that part.
• policy.rs:74 uses the same regex text as the TS annotator, but Rust's \b and \s are Unicode-aware, so a few inputs match differently. Please don't "fix" Rust to match TS on U+FEFF, because the TS side is the one that's wrong there. I'll handle that separately.

I'll re-run CI against current integration once you push, since the last run merged against 770f1e0.

[Arham-Begani]

Review follow-up is pushed in 208698c, synchronized with integration at 8a45952.

• Exact OPA 0.70.0 startup guard and frozen-lockfile CI install; hoisted Bun layout preserves the requested platform-package path.
• SDK OPA deadline plus 1000 ms advertised to hosts, with overflow rejection and regression coverage.
• Nine-case comparison claims narrowed, including the intentional handshake deadline difference and uncovered inputs.
• Remaining server logging writes made non-panicking; README licensing corrected; weekly Cargo Dependabot coverage added.

All current checks pass, including Linux Rust tests, live Rust/TypeScript comparison, TypeScript checks, Python guards, strict documentation build, and zizmor. Local Rust results are 6 unit tests plus 1 HTTP integration test. The PR description records the separate Windows Python-suite limitations and links the successful Linux runs.

This update was prepared with Codex assistance. The contributor personally reviewed the focused patch and full PR, certified the DCO, and explicitly authorized sign-offs on the new commit and six older follow-up commits. The metadata rewrite preserves every code tree; every PR commit now carries a human sign-off. The Unicode-aware Rust regex remains unchanged as requested.

[Arham-Begani]

@rocklambros Could you re-review 208698c when you have time? Your October 12 changes-requested review is still active. The follow-up implements the exact OPA version guard, frozen-lockfile install, host deadline margin with regression coverage, narrower parity claims, and the logging/licensing/Dependabot changes described above. All reported checks on this head pass; every commit has the contributor-authorized DCO sign-off.

A formal re-review request was attempted but GitHub rejected it for insufficient account permissions, so I am recording the request here. No review approval or merge is being inferred from the green checks. This follow-up was prepared with OpenAI Codex assistance.

[Arham-Begani]

@rocklambros, could you re-review 208698c when convenient? The corrections from your review are in this head: the OPA pin and frozen install, deadline margin, and narrower parity claims. The Reference implementation workflow and the other reported Actions workflows pass.

The source is unchanged since the October 19 update. Please let me know if there is a remaining blocker.

This follow-up was prepared with AI assistance.

[rocklambros]

@Arham-Begani thanks for the follow-up. Everything from my October 12 review is in 208698c: the exact OPA 0.70.0 guard, the frozen-lockfile install with ACS_OPA_PATH, the 1000 ms margin with the overflow check and its tests, the narrowed nine-case claim, the stderr writes that no longer panic, the licensing row, and the Dependabot entry.

Your last CI run was from October 18, and PRs #272, #273 and #309 changed the TypeScript Guardian since then, so I closed and reopened the PR to rerun the workflow against current integration. Both jobs pass.

Two small things, neither a blocker. The Dependabot entry labels rust, and that label doesn't exist in the repo, so Dependabot will warn the way it already does for python. Once PR #229 lands, the TypeScript Guardian will check signatures, replay and timestamps and this port won't yet, so the README's limits sentence will need a follow-up then.

Approving.

[Arham-Begani]

Thanks for the re-review and for rerunning against current integration. I verified that both jobs in the new Reference implementation run executed and passed, including the live Rust/TypeScript comparison on the merge of 208698c into 65813c7.

I've recorded the two non-blocking follow-ups: the missing rust label in Dependabot, and updating the README's parity limits after #229 lands. That PR is still open. I'll preserve this approved patch rather than add another revision for those follow-ups.

This response and verification were prepared with AI assistance.

[artmaro]

@Arham-Begani, thanks for your work here.

Since this PR opened, the project merged shared expectations for Guardian reference implementations in reference-implementations/README.md. A few points where this port does not meet them yet:

• Bounded logging. When the envelope or session log fails to open or write, the Guardian disables it and keeps answering, so decisions go unaudited. The README asks for deny when an audit record cannot be kept, and for a size cap with rotation.
• Signing and verification. No §10 HMAC signing, key_id handling, or §10.3 replay protection.

Could you check the PR against the rest of that README too? Thank you

@rocklambros cc for visibility

[Arham-Begani]

Thanks, @artmaro. Commit 754dce0 adds bounded, fail-closed audit logging and removes the invalid Dependabot label. Current-head upstream CI passes.

Signing, key_id handling and replay protection remain outstanding; no full ACS-Core conformance is claimed. Could a lead assign issue #88 to me and confirm whether the remaining shared requirements must land in this PR or can be handled as agreed follow-ups?