feat: add isolated RVT connection reader runtime - #480
Conversation
# Conflicts: # CLAUDE.md
# 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
Bind the heap opt-out into the compiler environment, reject ambient overrides, and prove audited child inheritance with an authenticated negative control. Preserve all audit and provenance gates. Fixes #493.
Brings v0.134.0 (search corpus reporting, nuspec license guard) and the builder-nuget coverage onto the branch so the Windows repro work is reviewed against current main.
…be bypassed `normalizeRevitMetadata` accepts metadata schema 1 or 2, and the caller compared the result against the REQUESTED reader schema only after normalization had finished. A v1 response to a v2 request was therefore expanded under v1's much larger allowance, ignoring the v2 row and byte ceilings the request asked for: a compact document could allocate millions of property rows before anyone noticed the mismatch. The normalizer now takes the expected schema and refuses a mismatch immediately after parsing, before any table is expanded. `invalid()` already raises `reference-metadata-invalid` at stage `normalize-metadata`, the same code and stage the caller used, so the observable error contract is unchanged; the caller's check stays as defence in depth. The test pins WHICH error fires — the schema mismatch rather than an expansion ceiling — because that is what proves nothing was expanded first.
…isions Windows environment names are case-insensitive, but the inner reader's exact poison lookup was case-sensitive while its prefix check already folded case. An ambient `esbuild_binary_path` therefore passed the guard, and `process.env.ESBUILD_BINARY_PATH` still resolved it — letting esbuild execute an undeclared binary inside a supposedly closed build. Fold case before the exact lookup and reject names that collide only by case, matching what the outer builder in `cli/build-windows-internal-repro.mjs` already does. Which spelling a child process reads is not ours to decide, so two spellings of one name is itself the error. `Path` remains allowed: it is authority the reader needs and is absent from this file's poison set.
`forbiddenEncodings` derived each transform from the raw spelling only, so it produced a lowercased path and a JSON-escaped path but never a lowercased JSON-escaped one. `scanForbiddenRoots` lowercases file CONTENT before matching, so an artifact carrying a serialized Windows path with altered casing — `c:\users\alice` — matched no needle, and the URI form with spaces had the same combination gap. That is a hole in the proof itself: this scanner is what certifies that no physical builder root leaked into a compared artifact, so a gap here lets the reproducibility gate pass on artifacts that still contain a forbidden root. Fold case first, then escape, so each separator spelling contributes its raw, lowercased, escaped, lowercased-escaped, URI and lowercased-URI forms.
…figuration For reader v2 the number of property records is bounded by the request's own `propertyExpansionLimits`, but `packageConfiguration` only ever saw the ordinary model limits. `maximumShardRecords` was therefore `max(maxEntities, maxParameters, maxRelationships)`, which a lowered request can drive below the expansion ceiling: with model limits of 2 and two elements, groups and parameters, a valid document publishes eight property rows while its SIGNED configuration advertises two, and downstream package validation rejects reader-produced output. Pass the normalized expansion limits in and include `maxExpandedPropertyRows` in the v2 ceiling. v1 is untouched — it has no independent expansion bound. The test lowers the model ceilings beneath the expansion ceiling, which is the only shape where the two disagree; without the fix it reports 2 against 4096.
|
@codex review Please review the current head Since your round-1 review of
Local gates on this head: Review rounds: 2 (self-feeding: 0) — rounds: 1 CLI (4 findings, all fixed), 2 CLI interrupted by usage limit then requested here. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
|
@codex review Head is now
Canonicalized at the three points where those roots are established, using the 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
Local gates on this head: reader 300/300, Windows repro suites 38/38 (+1 win32-gated skip), 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
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.
|
@codex review Head is Three commits since your round-1 findings were addressed:
Local gates: reader 300/300, Windows repro suites 38/38 (+1 win32-gated skip), 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. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 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".
| 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'); |
There was a problem hiding this comment.
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 👍 / 👎.
1 participant
Summary
Verification
No default branch was merged and no release was published.