fix(eval): make install preflight path test portable on macOS - #3283
Conversation
Astro-Han
left a comment
There was a problem hiding this comment.
Thanks — the diagnosis is right and the fix is the minimal one. Reviewed exact head 263c91753b759ca77665b316aece3ace6b10ee6b.
os.tmpdir() returning /var/folders/... while realpath() resolves /private/var/folders/... is exactly why this failed only on macOS, and asserting against the canonical path matches the production contract rather than working around it. The file is written earlier in the test, so realpath has something to resolve.
One point about how much the assertion protects, inline. No findings above P3, and no CI checks are currently reported for this head.
Review disclosure: this review was prepared with Claude Code, which read the diff at this head and checked the surrounding test for consistency. The human contributor reviewed it before posting.
| assert.equal( | ||
| calls[0]?.environment.MAKA_EVAL_NETWORK_POLICY_PATH, | ||
| join(egress, 'network-policy.json'), | ||
| await realpath(join(egress, 'network-policy.json')), |
There was a problem hiding this comment.
[P3] Make the canonicalization contract observable on every platform, not just macOS. The expected value is now derived with the same primitive production uses, so where tmpdir() is already canonical — Linux CI, which is where this will usually run — realpath(join(egress, ...)) equals join(egress, ...) and the assertion silently degrades to what it was before. A regression that dropped the canonicalization would then pass CI and fail only on a contributor's Mac, which is the situation this PR is fixing. Placing the egress directory behind a symlink the test creates itself makes the two spellings differ everywhere, so the assertion tests the contract rather than the platform.
Astro-Han
left a comment
There was a problem hiding this comment.
Approving at 263c91753b759ca77665b316aece3ace6b10ee6b. My review of this head found nothing above P3, so nothing here should hold the merge — the inline note stands as a suggestion, to take or leave.
The review disclosure on my earlier comment applies: it was prepared with Claude Code, and I reviewed the diff and the findings myself before posting. This approval is my own judgment.
Astro-Han
left a comment
There was a problem hiding this comment.
Re-reviewed at e897fdf5 after the force-push. No findings.
The revision fixes the failure at its cause rather than working around it. install-preflight.ts deliberately resolves the egress paths through resolveRealPathWithinRoot, so canonicalization is the production contract — the old assertion comparing against a non-canonical join(egress, …) was simply asserting the wrong thing, and only passed on Linux because tmpdir() there happens not to be a symlink. Asserting against realpath is correct, and it is now correct for the same reason on every platform instead of by accident on one.
Two things I want to note in the change's favour, since they are easy to miss on a ten-line diff:
- Introducing an explicit
egress -> egress-targetsymlink makes the test exercise symlink resolution deterministically, rather than depending on whether the host's temp directory happens to be one. That is strictly more coverage than before, not less. assert.notEqual(canonicalNetworkPolicyPath, networkPolicyPath)guards the fixture itself: if the symlink ever stops being created, the test fails loudly instead of silently degrading into a tautology. That is the right shape for this kind of fix.
I also checked whether the same non-portable assumption exists elsewhere in the package. It does not — lifecycle-boundaries.test.ts is the only other test combining mkdtemp(tmpdir()) with a join-based assertion, and that one compares a stat mode rather than a path string, so it is unaffected. The Windows 'junction' branch is correct; directory junctions do not require elevation.
Reviewed with Claude Opus as an analysis assistant. I verified the production canonicalization path and the sibling-test sweep by reading source at this head; I did not execute the suite on macOS or Windows, so the portability claim rests on the code path rather than on a run.
Not marking this approved only because my current standing authorization in this review campaign is comment-only; nothing here blocks it from my side.
Astro-Han
left a comment
There was a problem hiding this comment.
Reviewed exact head e897fdf52f2875d5e992b925b48f1bda5614336a.
No actionable findings. The new revision closes the earlier portability gap by creating its own symlink fixture and proving that the input spelling differs from the canonical path on every supported platform. The assertion now directly protects the production resolveRealPathWithinRoot contract instead of depending on macOS /var behavior.
This is a focused test-only correction with no production or protected-area effect. git diff --check passed. The required test check is currently running and must pass before merge.
Review disclosure: Codex assisted with the exact-head diff, production-contract, thread, and check-state analysis. I reviewed the final evidence and own this approval.
Summary
Fix the Eval install preflight test on macOS by comparing the propagated network policy path with its canonical filesystem path.
On macOS,
os.tmpdir()can return/var/folders/..., whilerealpath()resolves the same location as/private/var/folders/.... Production intentionally returns the canonical path to support symlink-containment checks, so the test now reflects that contract instead of expecting the original path spelling.Verification
npm run build:test— passednode --test packages/eval/dist/__tests__/install-preflight.test.js— 8 passed, 0 failedgit diff --check— passedtest:distwere not run successfully because the local Python 3.9.6 interpreter does not support the suite'sint | Nonesyntax.AI use
Select exactly one:
Tool(s) and scope: OpenAI Codex diagnosed the macOS path canonicalization mismatch, updated the test expectation, and ran the relevant build and test checks.
Checklist
Does this PR entail a change in behavior?