Skip to content

fix(agt): support Windows UCRT/Win32 OPA path publishing and repo-relative path normalization - #337

Open
drk-ram wants to merge 2 commits into
GenAI-Security-Project:integrationfrom
drk-ram:refimpl/windows-opa-and-path-portability-336
Open

drk-ram wants to merge 2 commits into
GenAI-Security-Project:integrationfrom
drk-ram:refimpl/windows-opa-and-path-portability-336

Conversation

@drk-ram

@drk-ram drk-ram commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

What changed

Publishes ACS_OPA_PATH on Windows (win32) in reference-implementations/agt/packages/agt-bridge/src/opa-path.ts via a process.env write, which reaches the real Windows environment under Bun so the bundled agent-control-specification-opa-win32-x64 binary works under Bun without pre-exporting ACS_OPA_PATH, normalizes backslashes in the repository-relative path remainder stripped by toRepoRelativeMessage in packages/guardian/src/server.ts, and uses URL .href and plain environment maps in packages/guardian/test/otel-trace-exporter.test.ts.

Which issue does this implement

Part of #72
Part of #92
Part of #110
Fixes #336

  • 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

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

@rocklambros rocklambros 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.

Raghuvar, your backslash change works on the message in issue #336. I ran your pattern on that message with a C:\ root, and it gives specification/v0.1.0/. On integration 19b1197 the backslashes stay. Every suite passes on the merged tree with the same counts as integration, including 1287 Bun tests.

The win32 block is unverified. Linux and macOS can't load ucrtbase.dll, and CI uses ubuntu-latest only, so nobody ran this code in this review. I ran its logic on macOS with a fake bun:ffi, and that shows the logic only. It shows nothing about Windows.

Your report in issue #336 is the only Windows evidence so far.

This PR carries the win32 block, and PR #344 deletes it (ask 4 on PR #344). This PR is the one about Windows. PR #344 is a security fix that waits on a lead answer, and Windows support should not wait with it.

The two blocks are byte-identical, and the merge gives one block. PR #337 needs to land before PR #344, because PR #344 closes issue #72 and issue #72 names this Windows defect.

I need the changes below. Paths are under reference-implementations/agt/packages/. I ran asks 1 to 3 in a scratch copy, and the suites pass with them in.

  1. guardian/src/server.ts:147-150 lets the repo path through. The new group [^'"\\s]* reads up to the next space or quote, so it also takes a second copy of the root, and the g flag never sees that copy. The message search path <root>/a:<root>/b comes back as search path a:<root>/b. On integration it comes back as search path a:b. A colon, comma, semicolon, equals sign or backtick between the paths all leak the same way. This function exists to keep that path out of client errors and the envelope log. I found no throw site today that puts two paths in one token, so nothing leaks this way yet. The fix is small, so I want it in. The comment at lines 137-146 also describes a lookahead that is gone. Replace lines 137-150 with:

    // The root must be followed by a path separator or the end of the string.
    // Without that rule, a *sibling* directory whose name merely extends the root
    // (`<root>_old`) matched too: the literal text of REPO_ROOT is a prefix of
    // that name, so it stripped, leaving a misleading
    // `_old/reference-implementations/agt` behind -- not a disclosure of this
    // tree's own location, since it's a different directory entirely, but a
    // diagnostic that then reads as if it were one. A message naming the sibling
    // is left alone, in full.
    //
    // The group captures the repository-relative remainder, so the replacer in
    // toRepoRelativeMessage can change its backslashes to slashes. The group stops
    // before the next copy of the root, because a list such as `<root>/a:<root>/b`
    // is one token, and the `g` flag must still find the second copy.
    const REPO_ROOT_SOURCE = REPO_ROOT.replace(/[.*+?^${}()|[\]\\]/g, "\\$&");
    const REPO_ROOT_PATTERN = new RegExp(`${REPO_ROOT_SOURCE}(?:[/\\\\]((?:(?!${REPO_ROOT_SOURCE})[^'"\\s])*)|$)`, "g");

    Test: ask 2.

  2. No test covers your change to this function on Linux or macOS. I removed the backslash replace, and all tests still passed. I put integration's pattern back, and all tests still passed. Add this test directly under line 961 of guardian/test/server.test.ts:

    it("strips the root from every path in one token, and writes the remainder with slashes", () => {
      const root = fileURLToPath(new URL("../../../../../", import.meta.url)).replace(/[/\\]+$/, "");
      expect(toRepoRelativeMessage(`search path ${root}/a:${root}/b`)).toBe("search path a:b");
      expect(toRepoRelativeMessage(`tried ${root}/a,${root}/b;${root}/c`)).toBe("tried a,b;c");
      expect(toRepoRelativeMessage(`scandir '${root}\\specification\\v0.1.0\\'`)).toBe("scandir 'specification/v0.1.0/'");
    });

    Test: run bun test packages/guardian/test/server.test.ts from reference-implementations/agt. It fails on your PR today, and it passes with ask 1.

  3. The five keepLoopbackOffProxy tests at guardian/test/otel-trace-exporter.test.ts:1491 moved to plain objects, and neither this PR nor issue #336 says why. The change is right. process.env ignores case on Windows, so NO_PROXY and no_proxy are one variable there, and the old assertions could never pass. Coverage held too: I broke keepLoopbackOffProxy five ways, and your tests and the old tests failed in the same five. A future reader can't see why the tests moved, so add this above line 1491:

    // These tests pass a plain object, not process.env. On Windows process.env
    // ignores case, so NO_PROXY and no_proxy are one variable there, and a test
    // that one exists without the other could never pass.

    Test: git grep -n "ignores case" reference-implementations/agt/packages/guardian/test/otel-trace-exporter.test.ts prints the new line.

  4. Your closing lines are in the right form. Part of #92 leaves issue #92 open and Fixes #336 closes issue #336. tools/closing_choice.py passes them, and GitHub links only issue #336. Please add two lines to the issue section, so the PR names the accepted issues it feeds:

    Part of #72
    Part of #110
    

    Test: closing-choice stays green, and gh pr view 337 --json closingIssuesReferences still lists only issue #336.

  5. Please run bun test on Windows from reference-implementations/agt on your final head, with Bun 1.3.11 (CI's pin) and with Bun 1.4.4 (the version in issue #336). You need both, because the two tests you changed to .href skip below Bun 1.3.14. Paste the pass, skip and fail counts and the names of the failing tests. Issue #110 lists other Windows failures, so I don't expect zero.

    Then please try one more thing. Bun's source says a process.env write on Windows calls SetEnvironmentVariableW (src/bun.js/node/node_process.zig:345 at bun-v1.3.11). So the existing line at agt-bridge/src/opa-path.ts:106 may already do the job of this block. I read this and did not run it. Delete lines 111-138, add this line directly under line 81, and run bun test again on both versions:

    if (process.platform === "win32") process.env[OPA_PATH_ENV] = resolved;

    If the counts match, please keep this one line, and this PR then needs no Windows FFI code.

    Test: the two sets of counts.

@bar-capsule @afogel, this PR needs one triage answer from you on issue #336. It has no status:accepted, no scope: label and no milestone, so this PR can't close it yet. Two of its three defects are already in accepted issue #110: the win32 C library failure and the backslash path (R7 there). The third is the .pathname problem from R6, in a file issue #110 does not list. Please do one of these:

  1. Accept issue #336, with labels and a milestone. Fixes #336 stays.
  2. Close it as a duplicate of issue #110. Fixes #336 goes, and this PR stays Part of #110.

Either answer settles the closing line. The PR then still needs a Windows run by someone other than the author.

@Premmethuku75, you measured Windows 11 in issue #110. If you still have that machine, a bun test on this PR's final head would be that run.

@drk-ram

drk-ram commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

@rocklambros Addressed all five asks in 0944df8:

  1. Updated REPO_ROOT_SOURCE and REPO_ROOT_PATTERN (and the comment at lines 137–150) in packages/guardian/src/server.ts so multiple copies of <root> inside a single token are all stripped.
  2. Added strips the root from every path in one token, and writes the remainder with slashes in packages/guardian/test/server.test.ts (53 pass, 0 fail).
  3. Added the 3-line comment above packages/guardian/test/otel-trace-exporter.test.ts:1491 explaining why the five keepLoopbackOffProxy tests pass a plain object instead of process.env.
  4. Added Part of #72 and Part of #110 to the issue section of the PR description (and commit message); closingIssuesReferences still lists only #336.
  5. Windows bun test comparison (FFI block vs. 1-line if (process.platform === "win32") process.env[OPA_PATH_ENV] = resolved;):
    • Your reading of src/bun.js/node/node_process.zig:345 is confirmed on Windows 11: assigning process.env[OPA_PATH_ENV] = resolved on win32 sets the real Windows process environment (SetEnvironmentVariableW), openLibc() returns undefined, and the existing if (process.env[OPA_PATH_ENV] === resolved) return resolved; early return succeeds with zero FFI code.
    • packages/agt-bridge/test/opa-path.test.ts (3 pass, 0 fail) and packages/guardian/test/server.test.ts (53 pass, 0 fail) pass identically on both Bun 1.3.11 and Bun 1.4.4 with the 1-line process.env assignment, so I kept the 1-line version and removed the win32 FFI block.
    • Full bun test counts from reference-implementations/agt on Windows 11:
      • Bun 1.3.11 (af24e281): 1172 pass, 4 skip, 101 fail across 75 files (1277 tests).
      • Bun 1.4.4 (38afee6c8): 1171 pass, 1 skip, 105 fail across 75 files (1277 tests; With the 28-line FFI block it was 1170 pass, 1 skip, 106 fail).
    • The remaining Windows failures (tracked under #110) are in: acs-hook / the wire shape this host writes to stdout (Claude Code & Cursor hook shims spawning .ts / shell paths), AcsPlugin's load-time gate, agt-bridge — the optional annotator dispatcher, fetchUpstreamSurfaces / the schema leg / readSurfaces / checkPolicyInputSchemaAt / runUpstreamWatch / renderToolsRegistrySection / the entry point's exit status, createOtelTraceExporter > creates the file 0600 and its directory 0700 (POSIX mode bits 0666 vs 0700), call_id: the real hookmaps, and the derivation pinned, tailAuditLog, tailEnvelopeLog, and (on Bun 1.4.4, where the 3 version-gated tests un-skip) buildServerHello — the zero-argument path the Guardian actually calls and main.ts applies the loopback proxy exemption.

@rocklambros rocklambros 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.

Raghuvar, all five asks are done. Your Windows run answered the question I could not run here, so thank you for that.

I ran your head 0944df8 merged on integration 1b8f970. A second copy of the root in one token no longer leaks. I broke the pattern on purpose in 6 ways, and a test failed each time. Asks 3 and 4 are in the PR.

For ask 5, the one line replaced 28 lines that nobody here could run. Your counts are slightly better with it: 1171 pass and 105 fail on Bun 1.4.4, against 1170 and 106. Those counts are your own report. I can't run Windows here, so this PR still needs a Windows run by someone other than you.

Every suite passes on macOS: pytest 924 passed and 1 skipped, and 1288 Bun tests with 0 failures on Bun 1.3.11, 1.3.14 and 1.4.4.

I need two small changes. The path below is under reference-implementations/agt/.

  1. packages/agt-bridge/src/opa-path.ts:79: the header of this file says a Bun process.env write changes nothing the native side can see. That is true on Linux and macOS, and your new line depends on Windows being different. A future reader will think the line is a bug. Add this above line 79:

    // On Windows a Bun `process.env` write calls SetEnvironmentVariableW, so it
    // reaches the real environment and this platform needs no C library. Linux
    // and macOS keep the JavaScript-side copy the header describes.

    Test: git grep -n SetEnvironmentVariableW reference-implementations/agt/packages/agt-bridge/src/opa-path.ts prints the new line.

  2. The first paragraph of the PR description still says ucrtbase.dll, _putenv_s and SetEnvironmentVariableA, and that code is gone. Replace those words with "a process.env write, which reaches the real Windows environment under Bun".

    Test: gh pr view 337 --json body no longer prints ucrtbase.

This PR and PR #344 now change the same line of opa-path.ts, so they conflict. My review of PR #344 has the fix, and that PR takes the rebase, so you don't need to change anything here for it.

@bar-capsule @afogel, this PR still needs the triage answer from my last review. Please accept issue #336 with labels and a milestone, or close it as a duplicate of issue #110. Either answer settles the closing line.

…normalization

Publish ACS_OPA_PATH on Windows via process.env[OPA_PATH_ENV] = resolved
(which calls SetEnvironmentVariableW in Bun on Windows) in
packages/agt-bridge/src/opa-path.ts, normalize stripped repo-relative
path separators and multi-path tokens in toRepoRelativeMessage, and use
file URL hrefs for child-process dynamic imports in
otel-trace-exporter.test.ts.

Part of GenAI-Security-Project#72
Part of GenAI-Security-Project#92
Part of GenAI-Security-Project#110
Fixes GenAI-Security-Project#336

Signed-off-by: Raghuvar Karthik Durga <177662415+drk-ram@users.noreply.github.com>
@drk-ram
drk-ram force-pushed the refimpl/windows-opa-and-path-portability-336 branch from 0944df8 to 82141a4 Compare October 31, 2026 23:37
@drk-ram

drk-ram commented Oct 31, 2026

Copy link
Copy Markdown
Collaborator Author

@rocklambros Addressed both asks in 82141a4: added the SetEnvironmentVariableW comment above line 79 of packages/agt-bridge/src/opa-path.ts and updated the PR description to remove the old ucrtbase.dll wording.

Split on the repository root instead of matching the whole token, so a
long token cannot end the match and hand the root back unredacted, and
a root followed by a quote, colon, comma, space, or newline is stripped.

On Windows, match the root in any case and in the forward-slash,
doubled-backslash, /C:/, file:/// and %20 spellings. Rewrite backslashes
only in the path remainder and only for a Windows root, so a regex after
the path or another drive's path keeps its text, and a POSIX file name
holding a backslash stays the same file. A space stays in the remainder
when a quote closes the path.

Return early on win32 so no libc probe runs there, and say in the code
and the bridge README that Windows has no readback and that a lost write
denies every evaluation on policy_invocation_failed.

Use fileURLToPath in the two remaining tests that read URL.pathname.

Part of GenAI-Security-Project#110

Signed-off-by: rocklambros <rock@rockcyber.com>
@rocklambros

Copy link
Copy Markdown
Collaborator

Raghuvar, both of your last asks are done, and they work. The review after that found a bug that came from my own ask. The pattern I told you to use stops matching on a very long token, and then the absolute path leaks with no error. That's on me, so I fixed it and pushed 15facac on top of your commit. You don't need to do another round. Please pull before you push again.

What changed:

  1. Redaction now splits on the root, so a long token can't leak it. A 30 MB message takes under a quarter of a second.
  2. On Windows it now matches the root in any case, and in the C:/, /C:/, file:///, doubled-backslash and %20 spellings.
  3. Backslashes change only in the path itself, and only for a Windows root. A regex after the path stays as it was.
  4. On Windows, publishOpaPath returns early, so the libc probe no longer runs. The code and the README now say Windows has no readback.
  5. The two tests that still read URL.pathname now use fileURLToPath.

All 48 deliberate breaks were caught, and every suite passes on the merged tree, except posture.test.ts, which also fails on integration. PR #404 fixes that one. This PR still lands before PR #344.

Two gaps remain. Other percent-encoded characters are not covered, and a path with a space and no quotes stops at the space.

@Premmethuku75, could you run this on Windows, on Bun 1.3.11 and 1.4.4? From reference-implementations/agt:

bun test packages/agt-bridge/test/opa-path.test.ts

Nobody except Raghuvar has run this on Windows yet, and the PR needs that before it merges.

I pushed last, so my approval doesn't count here. @bar-capsule @afogel, one of you needs to approve this one.

@drk-ram

drk-ram commented Nov 2, 2026

Copy link
Copy Markdown
Collaborator Author

@rocklambros Pulled 15facac83ff56c1416baed580d22ffde61980c35 locally and verified packages/agt-bridge/test/opa-path.test.ts and packages/guardian/test/server.test.ts on Windows 11 (Windows_NT 10.0.26300 x64) across both Bun 1.3.11 (af24e281) and Bun 1.4.4 (38afee6c8):

1. Bun 1.3.11 (af24e281)

$ bun test packages/agt-bridge/test/opa-path.test.ts
bun test v1.3.11 (af24e281)

packages\agt-bridge\test\opa-path.test.ts:
(pass) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > a bridge built in a process whose environment names no opa at all still evaluates policy [344.00ms]
(skip) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > on win32 publishes through process.env and leaves the C library alone
(pass) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > answers the path of an opa that actually runs [78.00ms]
(pass) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > refuses to build a bridge when an explicit ACS_OPA_PATH names nothing, rather than one that denies every step

 3 pass
 1 skip
 0 fail
 4 expect() calls
Ran 4 tests across 1 file. [527.00ms]
$ bun test packages/guardian/test/server.test.ts
bun test v1.3.11 (af24e281)

 63 pass
 0 fail
 246 expect() calls
Ran 63 tests across 1 file. [7.59s]

2. Bun 1.4.4 (38afee6c8)

$ bun test packages/agt-bridge/test/opa-path.test.ts
bun test v1.4.4 (38afee6c8)

packages\agt-bridge\test\opa-path.test.ts:
(pass) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > a bridge built in a process whose environment names no opa at all still evaluates policy [214.06ms]
(skip) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > on win32 publishes through process.env and leaves the C library alone
(pass) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > answers the path of an opa that actually runs [78.97ms]
(pass) publishOpaPath -- the opa AGT's native core spawns, made reachable from a Bun process > refuses to build a bridge when an explicit ACS_OPA_PATH names nothing, rather than one that denies every step [0.94ms]

 3 pass
 1 skip
 0 fail
 4 expect() calls
Ran 4 tests across 1 file. [433.00ms]
$ bun test packages/guardian/test/server.test.ts
bun test v1.4.4 (38afee6c8)

 63 pass
 0 fail
 246 expect() calls
Ran 63 tests across 1 file. [6.00s]

Leaving 15facac untouched as the branch head so @bar-capsule / @afogel can review and approve directly (#349 worktree also pulled to 137c773).

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.

[Conformance] Windows x64 dogfooding at d1ad294: opa-path.ts lacks Win32/UCRT env publishing and toRepoRelativeMessage leaves backslash paths

2 participants