Skip to content

feat: add isolated RVT connection reader runtime - #480

Open
pawellisowski wants to merge 83 commits into
mainfrom
codex/xeorvt-aware-rvt-reader
Open

pawellisowski wants to merge 83 commits into
mainfrom
codex/xeorvt-aware-rvt-reader

Conversation

@pawellisowski

Copy link
Copy Markdown
Contributor

Summary

  • add the collision-isolated RVT connection-reader/runtime path required by the private xeoRVT distribution
  • make the Windows reproducible builder fail closed with sparse source extraction, immutable npm/Cargo inputs, private caches, and explicit vendored Cargo replacement
  • retain complete manifests, receipts, and bounded build evidence

Verification

  • builder G and builder H produced byte-identical declared artifacts from commit ab69de0 / tree 1672d49676897458492bb8e4726726e83b43e685
  • aware.exe: a24074d0cdace2ef4ec3f2d8260eca58fbd24f7a11e80cfbb2bb8fbaf9774e22
  • aware-connection-reader.exe: 75c08f76d66f338e49a831ff966b5271794a21693868976dce1389c0d6cccceb
  • builder comparison evidence SHA-256: 2343e4251b19371ffe77fac3f9fa59a28a49c8b459575f4249e329415a12ff9

No default branch was merged and no release was published.

# Conflicts:
#	.github/workflows/ci.yml
#	20-agents/aeco/engineering/model-reference-reader/manifest.yaml
#	CLAUDE.md
#	cli-connection-reader/model-cache.mjs
#	cli-connection-reader/model-cache.test.mjs
#	cli-connection-reader/model-contract.mjs
#	cli-connection-reader/model-contract.test.mjs
#	cli-connection-reader/model-dispatcher.mjs
#	cli-connection-reader/model-dispatcher.test.mjs
#	cli-connection-reader/model-host-client.mjs
#	cli-connection-reader/model-host-client.test.mjs
#	cli-connection-reader/model-provider.mjs
#	cli-connection-reader/model-provider.test.mjs
#	cli-connection-reader/model-reader.mjs
#	cli-connection-reader/model-reader.test.mjs
#	cli-connection-reader/model-snapshot.mjs
#	cli-connection-reader/model-snapshot.test.mjs
#	cli-connection-reader/model-windows-harness.mjs
#	cli-connection-reader/package-lock.json
#	cli-connection-reader/package.json
#	cli-connection-reader/revit-glb.mjs
#	cli-connection-reader/revit-glb.test.mjs
#	cli-connection-reader/revit-metadata.mjs
#	cli-connection-reader/revit-metadata.test.mjs
#	cli-connection-reader/test-fixtures/model-provider-fixture.mjs
#	cli/Cargo.toml
#	cli/src/commands/model_reader_host.rs
#	cli/src/runtime/invoker.rs
#	cli/src/runtime/pidfile.rs
#	docs/superpowers/specs/2026-08-23-aware-rvt-reader-plan.md
#	docs/superpowers/specs/2026-08-23-aware-rvt-reader-review-log.md
@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

The Windows compiler audit compared paths produced by Node against paths
produced by Windows and .NET, and the two disagree about 8.3 short names:
`join`/`win32.resolve` keep whatever spelling they were given, while
`GetFinalPathNameByHandleW` and .NET `Path.GetFullPath` return the expanded
form once the path exists on disk. Any work root under a short temp directory
therefore broke three checks in sequence.

The auditor validates its own startup policy with `DosPath(path) == path`, so a
policy built from the short spelling failed as "Invalid private telemetry
policy" before any compilation began. Past that, image attribution compared the
debugger's canonical observation against a short root, so the compiler's own
build script read as "an image outside its authority". Past that, the test's
cleanup guard compared its canonical root against a short `tmpdir()` and
concluded it had escaped its own parent.

Canonicalize at the three points where these roots are established, which is
the idiom this file already uses for closure inventories and the PowerShell
inbox. `build-windows-internal-repro.mjs` was already immune because it
canonicalizes both sides of its comparison.

This only reproduces where the temporary directory has a short component, which
is why it went unseen locally and failed every run on a GitHub Windows runner,
whose TEMP is C:/Users/RUNNER~1/AppData/Local/Temp. Verified by constructing
that shape deliberately: with TEMP set to an 8.3 alias, the native gate now
reports two byte-identical executables and its two red mutations, exit 0.
…ns it

The scanner test derived its fixture from `tmpdir()`, but this suite runs on
ubuntu in CI. A POSIX path holds no backslashes, so `JSON.stringify(root)` is
the identity and the needle collapsed to `root.toLowerCase()` -- which the
unfixed scanner already emitted. The guard against re-regression was therefore
green with and without the fix on the only runner that executes it.

Derive it from a synthetic Windows root instead. Those backslashes are ordinary
characters on POSIX, so they survive `resolve()` and the fold-then-escape
combination is genuinely absent before the fix on every platform. Asserting on
`forbiddenEncodings` directly also names the missing needle when it fails.

Two related corrections. The caller-side schema check in `model-reader.mjs` is
now unreachable -- the normalizer refuses a mismatch before it -- but it was
re-parsing `propertiesBytes` to re-derive a constant, which can reach 32 MB on
every read; assert on the coverage already returned instead, keeping the
defence in depth at no cost. And a `Math.max(2, 2, 2) < 4096` assertion in the
snapshot test was a constant expression that documented intent while proving
nothing, so it is gone; the load-bearing assertion beside it does fail without
its fix.
@pawellisowski

Copy link
Copy Markdown
Contributor Author

@codex review

Head is now 0974c690c. Two commits since your round-1 findings were addressed at 676e9aeab.

2e8e76292 — fixes the CI failure, which was real and predated the fix commits. The connection-reader packaged RVT/IFC harness job had been red since 4591c220e (green at 2e1083971). Root cause: Node's join/win32.resolve never expand 8.3 short components, while GetFinalPathNameByHandleW and .NET Path.GetFullPath both do once the path exists. A GitHub Windows runner's TEMP is C:/Users/RUNNER~1/AppData/Local/Temp, so every canonical-vs-short comparison failed there and nowhere else. It broke three checks in sequence:

  1. DosPath(denied.path) == denied.path in the auditor's startup-policy validation → Invalid private telemetry policy before any compilation
  2. image attribution comparing the debugger's canonical observation against a short root → the compiler's own build script read as an image outside its authority
  3. the probe's cleanup guard comparing its canonical root against a short tmpdir()test cleanup escaped its temporary parent

Canonicalized at the three points where those roots are established, using the realpathSync.native idiom already used five times in that file. build-windows-internal-repro.mjs was already immune — it canonicalizes both sides at line 452.

Verified by reproducing CI's shape deliberately rather than on a convenient path: with TEMP set to an 8.3 alias whose expansion is safe, the native gate reports 2 byte-identical executables, both path spellings, spaced paths, two red mutations, old-runner/new-bundle refusal, exit 0. Digest 9d0df714…, matching the clean-path run.

0974c690c — corrections to my own round-1 fixes. The forbidden-root regression test derived its fixture from tmpdir(), but that suite runs on ubuntu, where a POSIX path has no backslashes, so JSON.stringify(root) was the identity and the needle collapsed to root.toLowerCase() — which the unfixed scanner already emitted. It was green with and without the fix on the only runner that runs it. Now derived from a synthetic Windows root, so it fails pre-fix on every platform. Also made the now-unreachable caller-side schema check cheap (it was re-parsing up to 32 MB per read to re-derive a constant) and removed a constant-expression assertion that proved nothing.

Local gates on this head: reader 300/300, Windows repro suites 38/38 (+1 win32-gated skip), cargo fmt/clippy -D warnings clean, 54 Rust suites ok.

Review rounds: 3 (self-feeding: 1) — rounds: 1 Codex CLI (4 findings, all fixed); 2 Codex CLI hit its usage limit mid-run, local fallback reviewer found my scanner test was vacuous in CI (self-feeding); 3 requested here. Per repo policy this PR is NOT self-merging: it needs a Codex review of this final commit with CI green on the same commit.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

The packaged RVT/IFC harness asserts that a provider run in a deliberately
stripped environment reproduces the Node SEA CSPRNG abort (exit 134). It timed
out instead (exit 124) on every run since this job gained its compiler-audit
steps.

The harness itself is unchanged -- this assertion has been here since #468, and
the job passed while it ran on a fresh runner with a 15-minute budget. What
changed is the work that now precedes it: two fresh compiler/SDK closure copies,
a native vendor-repro build that compiles Rust twice under a debugger, a full
cargo build and SEA staging. The call then launches a freshly written .exe for
the FIRST time, paying a cold Defender scan on a runner that has been working
hard for ten minutes, and 10s expired before the provider reached its abort.

10s was also tighter than this file's own `run()` default of 30s for invoking
the same binaries, and it was the only timeoutMs here.

Raise it to 60s. This does not weaken the check: its meaning is carried by the
`exitCode === 134` assertion, which a hang still fails -- the budget only sets
how long a hang takes to surface, well inside the job's 45-minute limit.

The alternative is to split the compiler audits into their own job so the
packaged harness gets an unworked runner. That is the better shape long term but
restructures CI on this branch's terms, so it belongs to whoever owns this work.
@pawellisowski

Copy link
Copy Markdown
Contributor Author

@codex review

Head is 29050ab1d and all 14 CI checks are green — including connection-reader packaged RVT/IFC harness, which had been red since 4591c220e (last green: 2e1083971).

Three commits since your round-1 findings were addressed:

2e8e76292 — 8.3 short-path canonicalization. Node's join/win32.resolve never expand short components; GetFinalPathNameByHandleW and .NET Path.GetFullPath both do once the path exists. A runner's TEMP is C:/Users/RUNNER~1/AppData/Local/Temp, so this broke three checks in sequence: the auditor's DosPath(denied.path) == denied.path startup-policy validation (Invalid private telemetry policy), image attribution against a short root (an image outside its authority), and the probe's cleanup guard against a short tmpdir(). Canonicalized at the three points those roots are established, using the realpathSync.native idiom already used five times in that file. build-windows-internal-repro.mjs was already immune (it canonicalizes both sides at line 452). Confirmed on the runner: the four compiler-authority tests now pass where they previously aborted the job.

0974c690c — corrections to my own earlier fixes. The forbidden-root regression test derived its fixture from tmpdir(), but that suite runs on ubuntu, where a POSIX path has no backslashes, so JSON.stringify(root) was the identity and the needle collapsed to root.toLowerCase() — which the unfixed scanner already emitted. It was green with and without the fix on the only runner that runs it. Now derived from a synthetic Windows root so it fails pre-fix everywhere. Also made the now-unreachable caller-side schema check cheap (it re-parsed up to 32 MB per read to re-derive a constant) and dropped a constant-expression assertion.

29050ab1d — the packaged-provider launch budget. The harness asserts a stripped environment reproduces the Node SEA CSPRNG abort (exit 134); it was timing out (124) instead. The harness is byte-identical to what passed at 2e1083971 and the assertion dates from #468 — what changed is that this job now runs two compiler/SDK closure copies, a native vendor-repro build, cargo build and SEA staging first, so the FIRST launch of a freshly written .exe pays a cold Defender scan on a worked runner. 10s was also tighter than this file's own run() default of 30s for the same binaries. Raised to 60s; the check's meaning is carried by exitCode === 134, which a genuine hang still fails.

Local gates: reader 300/300, Windows repro suites 38/38 (+1 win32-gated skip), cargo fmt/clippy -D warnings clean, 54 Rust suites ok. The native #487 gate passes under a deliberately CI-shaped 8.3 temp — 2 byte-identical executables, two red mutations, exit 0, digest 9d0df714….

Review rounds: 4 (self-feeding: 1) — 1 Codex CLI (4 findings, all fixed); 2 Codex CLI hit its usage limit mid-run, local fallback reviewer found my scanner test was vacuous in CI (self-feeding); 3 CI root-cause and fix; 4 requested here.

Not self-merging. Per repo policy this needs a Codex review of this final commit; the CLI is rate-limited until Sep 7.

@chatgpt-codex-connector

Copy link
Copy Markdown

You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard.
To continue using code reviews, add credits to your account and enable them for code reviews in your settings.

@pawellisowski

Copy link
Copy Markdown
Contributor Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 96f64e22bd

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment on lines +428 to +430
const commit = run(authority.tools.git, ['rev-parse', 'HEAD'], { cwd: source, env: gitEnv }).trim();
const tree = run(authority.tools.git, ['rev-parse', 'HEAD^{tree}'], { cwd: source, env: gitEnv }).trim();
if (commit !== manifest.source.commit || tree !== manifest.source.tree) throw new Error('extracted source identity mismatch');

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Authenticate the checked-out source bytes consumed by the builds

A concurrent or malicious modification to a checked-out Rust or reader source file after checkout is compiled into the outputs while these rev-parse calls still report the manifest's committed tree, because they inspect Git objects rather than the worktree. verifyExtractedInputs later covers only the lockfiles and four helper scripts, and there is no full worktree verification after either build, so the resulting receipt can attribute attacker-controlled binary bytes to the authenticated source commit. Materialize an immutable source closure or compare every sparse-worktree file with the committed tree before and after both builds.

Useful? React with 👍 / 👎.

…t-reader

# Conflicts:
#	.github/workflows/ci.yml
# Conflicts:
#	cli/src/error.rs
#	cli/src/runtime/invoker.rs
#	cli/src/runtime/provenance.rs
# Conflicts:
#	cli-connection-reader/revit-metadata.mjs
#	cli-connection-reader/revit-metadata.test.mjs
# Conflicts:
#	cli-connection-reader/model-contract.mjs
#	cli-connection-reader/model-reader.mjs
#	cli-connection-reader/model-reader.test.mjs
#	cli-connection-reader/revit-glb.mjs
#	cli-connection-reader/revit-glb.test.mjs
@pawellisowski

Copy link
Copy Markdown
Contributor Author

@codex review

Please audit exact current head e4500f1 for production regressions and security issues before merge and release. All exact-head CI checks are green. Do not approve if any material regression remains.

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: e4500f1404

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

throw new Error('wrapper must run under the pinned Windows x64 Node');
}
const toolIds = ['git', 'node', 'npm-cli', 'postject', 'web-ifc-wasm', 'powershell'];
const tools = Object.fromEntries(toolIds.map((id) => [id, verifyFileRecord(id, manifest, locator)]));

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

P1 Badge Materialize verified tools before invoking them

In the private Windows build, this verification retains the locator's original mutable paths, and Git is not invoked until later during source extraction. If git.exe is replaced after its digest is checked, the replacement can both materialize attacker-controlled source and forge the subsequent rev-parse results; because only the lockfiles and four helper scripts are checked independently, the final receipt can still claim the authenticated tool and source identities. The same check-then-use window affects npm/postject. Copy these tools from verified bytes into the private work root (and verify the copies) before consuming them.

Useful? React with 👍 / 👎.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant