fix(uh-237): make the new runtime tests platform-neutral so CI can pass - #238
Conversation
The template files moved from docs/specs/templates to specs/templates but the drift test kept reading the old path, leaving the CI job red on two ENOENT failures since the workspace-standards adoption. Also normalize CRLF so a Windows checkout does not produce a false drift.
Extracted from #231. --force already skips the runtime_requirements preflight, but mission run --auto still filtered candidates by unmet requirements, so --auto --force stayed blocked. Pass ignoreRequirements through chooseAdapter, report waived requirements in the decision reason and the --explain matrix, and update the --force help text on run, dry-run, and run-all.
…fixed script The capture host is environment-configured, so the opt-in path now refuses non-http(s) schemes and any host that is loopback, private, or reserved, checked both as a literal and via DNS resolution, with redirects refused. The beacon moved from a node -e string to a compiled fixed script that repeats the same guard before sending, and the test fixture keys are computed rather than literal.
An adapter file symlinked out of the harness directory was being read as a manifest; containment is now checked on the resolved real path.
Extracted from #233: the header linked linear.app/agentic-eng while every entry below cites linear.app/agenticengineering-agency/team/UH/active.
The 1.0 branch was validated on Windows, so its CI job failed on Linux for four platform-dependent assumptions. - src/harness/tool-guard.ts resolved paths with the host path module, so Windows-style targets (out\x, C:\other\x) were a single literal filename on POSIX and the containment checks misjudged them. Paths now normalize to one canonical form independent of the host, which is what the guard needs: it judges commands written for whatever platform the agent targets. - tests/acceptance.test.ts created temp dirs under the hardcoded Windows drive path T:/tmp; they now use os.tmpdir(). The Windows-style workspace and artifact_root strings in the evidence fixtures stay as data. - tests/independent-review.test.ts pointed cli_command at a fixture with no shebang and no exec bit; it is now a real executable on POSIX. - tests/oh-my-pi.test.ts compared the child cwd against the unresolved sandbox root; on macOS /tmp resolves to /private/tmp. Before: 12 failed / 88 passed across these files. After: 1058 passed / 7 skipped / 0 failed across the full suite, with typecheck and build clean.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Tick the box to add this pull request to the merge queue (same as
|
![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: 2be8b88aaf
ℹ️ 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".
| return path.normalize(candidate).toLowerCase(); | ||
| const raw = value.replaceAll("\\", "/"); | ||
| const base = root.replaceAll("\\", "/").replace(/\/+$/, ""); | ||
| const absolute = /^[a-zA-Z]:\//.test(raw) || raw.startsWith("/"); |
There was a problem hiding this comment.
When worker_root is C:\worker and the default write_roots: ["."] applies, a target such as C:..\secret.txt uses drive-relative Windows syntax and resolves to C:\secret.txt. Because this check recognizes only C:/... as absolute, it prefixes the target with the worker root and canonicalizes it beneath c:/worker, allowing direct writes or redirections outside the policy root. Resolve drive-qualified relative paths with Win32 semantics, or deny them when their drive-relative base is unknown.
AGENTS.md reference: AGENTS.md:L1-L3
Useful? React with 👍 / 👎.
| const candidate = path.isAbsolute(value) ? value : path.resolve(root, value); | ||
| return path.normalize(candidate).toLowerCase(); | ||
| const raw = value.replaceAll("\\", "/"); | ||
| const base = root.replaceAll("\\", "/").replace(/\/+$/, ""); |
There was a problem hiding this comment.
When a mission configures the supported absolute write root /—or worker_root is / with the default write_roots: ["."]—removing every trailing slash reduces the root to an empty string. A descendant such as /tmp/x then normalizes to tmp/x, which neither equals the empty base nor starts with /, so the guard rejects every write below the configured root. Preserve a canonical root marker or special-case filesystem-root containment.
AGENTS.md reference: AGENTS.md:L1-L3
Useful? React with 👍 / 👎.
The cancellation regression asserted that process.kill(pid, 0) throws for the killed grandchild. That tests whether init reaped the process, not whether the harness terminated it: a killed orphan is re-parented and stays visible as a zombie until init reaps it, and kill(pid, 0) succeeds for a zombie. In a container whose PID 1 does not reap orphans the zombie persists indefinitely, so the assertion failed on CI while passing on macOS, where launchd reaps promptly. The test now polls until the process is gone or is a zombie on Linux, which is what "terminated" actually means for a process whose parent has already exited. The SIGKILL group termination itself was working; only the assertion was wrong. Verified in a Linux container with bun as PID 1 and node present, matching the runner: the assertion reproduces "expected [Function] to throw an error" before this change and passes after it. Full suite in that same Linux environment: 91/91 files, 1058 passed, 7 skipped, 0 failed, with typecheck and build clean.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e05c14380d
ℹ️ 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".
| function normalized(value: string, root: string): string { | ||
| const candidate = path.isAbsolute(value) ? value : path.resolve(root, value); | ||
| return path.normalize(candidate).toLowerCase(); | ||
| const raw = value.replaceAll("\\", "/"); |
There was a problem hiding this comment.
On a POSIX worker with write_roots: ["out"], unconditional backslash replacement allows writes outside the configured root. For example, the guard normalizes printf x > out\side.txt as a write to out/side.txt and permits it, but a POSIX shell treats the backslash as an escape and writes outside.txt in the worker root. Choose Windows separator semantics only for Windows-style worker roots; otherwise this bypasses the write-root contract.
Useful? React with 👍 / 👎.
| const segments: string[] = []; | ||
| for (const segment of (absolute ? raw : `${base}/${raw}`).split("/")) { | ||
| if (segment === "" || segment === ".") continue; | ||
| if (segment === "..") { segments.pop(); continue; } |
There was a problem hiding this comment.
When an absolute UNC directory is configured as a write root, this generic stack permits .. to pop the share component even though Windows clamps traversal at the share root. For example, with write_roots: ["\\\\server\\share\\allowed"], the target \\server\share\..\share\allowed\x is normalized here to the permitted server/share/allowed/x, while Win32 resolves it to \\server\share\share\allowed\x, outside the configured directory. Preserve the UNC server/share anchor while collapsing parent segments.
Useful? React with 👍 / 👎.
| } catch { | ||
| return true; // Exited between the existence probe and the state read. |
There was a problem hiding this comment.
On Linux systems where /proc is absent or access to /proc/<pid>/stat is restricted, process.kill(pid, 0) can confirm that the descendant still exists and the subsequent read can fail, but this catch reports the process as terminated. The cancellation test then passes without verifying process-tree cleanup, potentially masking a live-child regression; only an ENOENT race should count as exit, while other errors should fail or fall back to polling.
Useful? React with 👍 / 👎.
…imeout assertion Two tests decided whether a killed process was gone by asking whether its pid still exists, which only proves that init reaped it. The cancellation regression hit this on CI; the verify timeout assertion had the same latent flake — it polled process.kill(pid, 0) and would have hung until its poll timeout if the child were ever left as an unreaped zombie rather than reaped. Extracts the zombie-aware probe into tests/process-state.ts and uses it for both assertions, so the "a zombie has stopped executing" reasoning lives in one place. The rest of the suite was audited for the same pattern: - tests/runtime-process.test.ts:94 is inside a Windows-only test, skipped on Linux, and Windows has no zombies, so it needs no change. - the remaining `.pid` matches in tests are fixture data, not liveness checks; the tui-suspend kill is an injected stub. Verified in a Linux container matching the runner (bun 1.3.14): full suite 91/91 files, 1058 passed, 7 skipped, 0 failed, with typecheck and build clean. Probe discrimination checked directly there — a live process reads as not terminated, and an orphaned grandchild that process.kill(pid, 0) still reports as present reads as terminated.
Ported from f9cef36 on `dev` (UH-140, #227), which never reached main because the v0.10.0 line carrying it was never cut. Main's "Typecheck + tests + build" job has been failing here since the spec-templates path fix stopped masking it. Reproduced on main with the CI pins before this change (Python 3.12, pytest 8.3.4, pytest-asyncio 0.24.0): 1 failed, 89 passed, 17 errors — the same numbers CI reports, and the same repro the original commit documented. - conftest.py: guard the watchdog/eviction task cancels against an already closed event loop. pytest-asyncio closes the per-test loop before the sync fixture's finally runs, so task.cancel() scheduled onto a dead loop and raised RuntimeError: Event loop is closed (the 17 teardown errors). Also snapshot and reset the module-level _active_sse_tails counter per test so the disconnect assertion measures only its own tails instead of inheriting a leak from a test that broke out of a stream early. - plugin_api.py: thread the Starlette Request into the SSE generator and stop tailing once await request.is_disconnected() is true, so the finally block releases the tail slot. Under httpx.ASGITransport the streaming generator is not promptly aclose()d when the consumer stops reading, so the counter previously leaked and test_disconnect_stops_server_generator saw 1 instead of 0. After: 90 passed, 0 errors, stable across consecutive runs, with no pending-task warnings. bun run plugin:typecheck and bun run plugin:build clean; the TypeScript suite is unchanged at 882 passed.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 37a0680b9f
ℹ️ 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".
| if (refused(parsed.hostname)) return; | ||
| try { | ||
| const dns = await import("node:dns"); | ||
| const records = await dns.promises.lookup(parsed.hostname, { all: true }); |
There was a problem hiding this comment.
When telemetry is enabled and DNS resolution stalls (for example, with an unreachable resolver or a misconfigured UH_POSTHOG_HOST), this lookup runs before the two-second abort timer is created. The detached beacon can therefore remain alive far beyond the advertised bound, and repeated CLI invocations can accumulate stuck child processes; apply the timeout to resolution as well as fetch (the in-process resolver in telemetry.ts has the same gap).
Useful? React with 👍 / 👎.
| if (url.protocol !== "https:" && url.protocol !== "http:") return null; | ||
| if (isRefusedCaptureHost(url.hostname)) return null; | ||
| try { | ||
| const records = await lookupImpl(url.hostname, { all: true }); |
There was a problem hiding this comment.
When UH_POSTHOG_HOST uses a literal IPv6 address, URL.hostname retains its brackets (for example, [2606:4700::1111]), but dns.lookup() expects the unbracketed address and returns ENOTFOUND. Consequently every literal-IPv6 PostHog endpoint is silently discarded before fetch, even though it is a valid HTTP(S) URL; normalize the hostname before lookup while retaining the bracketed URL for the request. The standalone beacon repeats the same lookup behavior.
Useful? React with 👍 / 👎.
| if (isRefusedCaptureHost(url.hostname)) return; | ||
| const body = JSON.stringify(buildCapturePayload(config, outcome)); | ||
| const child = spawn(process.execPath, ["-e", BEACON_SOURCE], { | ||
| const child = spawn("node", [beaconPath], { |
There was a problem hiding this comment.
When the built CLI is launched through an absolute Node path in an environment where node is absent from PATH (common in minimal service environments), or where PATH resolves a different/older Node installation, this detached spawn cannot run the beacon even though the CLI itself is running on a supported runtime. The previous implementation used process.execPath; retaining that for the fixed script avoids silently dropping every opted-in telemetry event in these environments.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b9b8899ea4
ℹ️ 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 records = await Promise.race([ | ||
| lookupPromise, | ||
| new Promise<never>((_, reject) => setTimeout(() => reject(new Error("DNS timeout")), timeoutMs)), | ||
| ]); |
There was a problem hiding this comment.
For any normal endpoint whose DNS lookup succeeds promptly, Promise.race resolves but the timeout scheduled in the losing branch is neither cleared nor unref'ed. That ref'd timer keeps every detached telemetry beacon alive for timeoutMs (two seconds) even after the POST finishes; frequent opted-in CLI calls therefore accumulate unnecessary live child processes. Retain and clear the timer in a finally block (and do the same in resolveCaptureEndpoint).
Useful? React with 👍 / 👎.
| const lookupPromise = dns.promises.lookup(lookupHost, { all: true }); | ||
| const records = await Promise.race([ | ||
| lookupPromise, | ||
| new Promise<never>((_, reject) => setTimeout(() => reject(new Error("DNS timeout")), timeoutMs)), | ||
| ]); |
There was a problem hiding this comment.
For an attacker-controlled UH_POSTHOG_HOST that DNS-rebinds, this lookup can return a public address and pass the check, but the later fetch(parsed) performs a separate hostname resolution and can then connect to a private address such as a metadata service. The request body includes the PostHog API key, so the address validation is bypassed in exactly the case it is intended to prevent; connect through a dispatcher pinned to the validated address while preserving the Host/TLS name, or otherwise prevent the second resolution.
AGENTS.md reference: AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
| const mapped = h.match(/^::ffff:(\d{1,3}(?:\.\d{1,3}){3})$/); | ||
| if (mapped) return refused(mapped[1]); | ||
| if (h.includes(":")) { | ||
| // IPv6 unique-local (fc00::/7) and link-local (fe80::/10) ranges. | ||
| return /^f[cd][0-9a-f]{2}:/.test(h) || /^fe[89ab][0-9a-f]:/.test(h); |
There was a problem hiding this comment.
A configured host such as http://[::ffff:7f00:1]:8080 reaches IPv4 loopback, but this pattern recognizes only mapped addresses with a dotted-decimal tail. The remaining IPv6 checks do not match ::ffff:7f00:1, so both the literal-host and DNS-result checks allow it before fetch connects locally; hexadecimal mapped forms can likewise target ::ffff:a9fe:a9fe for the metadata address. Parse IPv6 addresses rather than matching only dotted mapped notation, or reject mapped and compatible IPv6 forms.
AGENTS.md reference: AGENTS.md:L25-L26
Useful? React with 👍 / 👎.
2 participants
Why
Typecheck + tests + buildis red on this branch, but not because of its substance. The branch was validated on Windows, and six platform-dependent assumptions fail on Linux. I reproduced every one and fixed them:src/harness/tool-guard.tsnormalized()resolved paths with the hostpathmodule, so Windows-style targets (out\x.txt,Remove-Item out\a.csv,out\b.csv,Copy-Item ... -Destination out\x.csv) were a single literal filename on POSIX.inside()/protectedRoot()then reportedwrite_outside/delete_outsidewhere the expectation tables sayundefined/protected_root. Green on Windows, red everywhere else.tests/acceptance.test.tsmkdtemp("T:/tmp/...")hardcoded a Windows drive path and failed withENOENToff Windows.tests/independent-review.test.tsreviewer.cjswith no shebang and no exec bit, then pointedcli_commandat it, so POSIXspawnfailed andexecuteFixturesawfailedinstead ofpassed.tests/oh-my-pi.test.ts/tmp/...root; on macOS/tmpresolves to/private/tmp, so the stream inspector threw and the run was recorded as failed.tests/oh-my-pi.test.tsprocess.kill(pid, 0)throws for the killed grandchild — which tests whether init reaped it, not whether the harness terminated it. Details below.tests/verify.test.tsThe pid-existence assertions
A killed process whose parent has already exited is re-parented and remains visible as a zombie until init reaps it, and
process.kill(pid, 0)succeeds for a zombie. In a container whose PID 1 does not reap orphans, that persists indefinitely — so the assertion failed on CI while passing on macOS, where launchd reaps promptly. The process-group SIGKILL termination itself was working correctly; only the assertion was wrong.Confirmed by direct observation in a Linux container: after the group SIGKILL, the direct child reports
gone (ESRCH)while the grandchild reportsstill visible, state=Z.tests/process-state.tsnow holds the zombie-aware probe both assertions use, and its discrimination was checked directly — a live process reads as not terminated, and an orphaned grandchild thatkill(pid, 0)still reports reads as terminated.The rest of the suite was audited for the same pattern:
tests/runtime-process.test.ts:94is inside a Windows-only test (skipped on Linux, and Windows has no zombies), and the remaining.pidmatches are fixture data rather than liveness checks.What changed
src/harness/tool-guard.ts— paths normalize to one canonical comparison form that does not depend on the host running UH: separators unified,..collapsed, absolute targets detected by drive letter or leading slash, case folded. This is the fix that matters beyond CI, because the guard exists to judge commands written for whatever platform the agent targets, and it was judging them by the platform UH happened to run on.inside()/protectedRoot()compare with a plain/separator instead of the hostpath.sep.tests/acceptance.test.ts— the three temp dirs useos.tmpdir(). The Windows-styleworkspace/artifact_rootstrings in the evidence fixtures are deliberately left as-is, since those are data rather than filesystem calls.tests/independent-review.test.ts— the reviewer fixture gets a#!/usr/bin/env nodeshebang and the exec bit.tests/oh-my-pi.test.ts— the cwd assertion compares againstawait realpath(sandboxRoot), and the cancellation assertion waits for actual termination.tests/verify.test.ts+tests/process-state.ts— one shared termination probe, used by both assertions.Evidence
Reproduced and verified on Linux in a container matching the runner (node present, bun 1.3.14, bun as PID 1 so orphan reaping behaves as on CI):
AssertionError: expected [Function] to throw an error.bun run typecheckandbun run buildclean.Inherited: the plugin pytest suite (arrives via merging
main)Once the TypeScript suite passed, CI advanced to the Plugin tests step and failed there:
That is not this branch's —
mainfails identically, and this branch does not touchapps/hermes-plugin/. It is the UH-140 fix from #227 stranded ondev, because the v0.10.0 line carrying it was never cut. I reproduced it onmainwith the CI pins (Python 3.12, pytest 8.3.4, pytest-asyncio 0.24.0) — same numbers — and portedf9cef36ontomain, where it now gives 90 passed, 0 errors. Mergingmaininto this branch brings that fix in with it.Scope
Tests plus the one guard fix on this branch; no change to the 1.0 runtime behaviour, contracts, or docs. The plugin repair lands on
mainseparately, as it is anmaindefect rather than part of this branch's work.Known remaining gap (Windows)
The
independent-reviewfixture is now a real executable on POSIX, but a bare.cjsstill cannot be spawned directly on Windows. That was already failing there before this change, so nothing regresses — making it cross-platform would need a.cmdlauncher alongside the script, the same patternacceptance/support/cmdc.cmdalready uses. I did not add that blind, having no Windows host to verify it on.