Skip to content

Commit 1c91b62

Browse files
authored
Merge pull request #541 from atomantic/claim/pythonsetup-arch-tests
test([pythonsetup-arch-tests]): cover arch-aware detectPython + /setup/check arch fields
2 parents 7d76a47 + 0cb59ca commit 1c91b62

4 files changed

Lines changed: 293 additions & 3 deletions

File tree

.changelog/NEXT.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22

33
## Changed
44

5+
- **[pythonsetup-arch-tests] Test coverage for the arch-aware Local Python detector.** Internal: locks in the macOS arm64-vs-x86_64 preference behavior and the `/setup/check` arch fields with mocked-platform unit tests, so future edits to interpreter detection can't silently regress mlx wheel loading. No user-visible behavior change.
56
- **[mediajobqueue-resolve-live-params] Media job dispatch internals.** Internal refactor: the live-settings re-resolution step that picks the current Python interpreter before each video / local-image render moves into its own helper, giving future live-resolved fields a single seam. No user-visible behavior change.
67
- **[patch-settings-slice-helper] Nested settings edits no longer risk clobbering sibling fields.** Saving a Python path from Video Gen, or an image-gen config from a pipeline / Writers Room stage, used to round-trip the entire current settings object — leaving unrelated top-level keys exposed to drift if the read raced a concurrent write elsewhere. These edits now scope themselves to the slice being changed.
78
- **[client-use-previous-hook] Shared `usePrevious` hook.** Internal refactor: collapses two ad-hoc previous-render comparisons into one shared hook. No user-visible behavior change.

PLAN.md

Lines changed: 0 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,6 @@ For project goals, see [GOALS.md](./GOALS.md). For completed work, see [.changel
66

77
_Batch-cleared 2026-05-25: 23 Next Up items shipped together via parallel sub-agents (env-file helper, formatBytes migration, lazy voice UI text, voice long-term-memory routing, voice tool expansion + `ui_describe_visually`, three phosphene generate_ltx2.py hardenings, the peer-sync snapshot-coverage P1 refactor + ephemeralize-then-delete, per-record tombstone-ack clamp, MediaLightbox→MediaImage, global vitest peer mock, videoHistory sync category, reverse-sub UI broadcast, palette Health disambiguation, insertXxxWithId resurrection side-effects, importer tombstone detection, TUI finish-rejection note, mortalLoom errno widen, VideoGen stale-model toast, Universes sidebar grandchildren). See `git log` + `.changelog/NEXT.md`._
88

9-
- [ ] [pythonsetup-arch-tests] **Test coverage for arch-aware `detectPython()` + `/setup/check` arch fields.** Added 2026-05-28 with the VideoGen inline Local Python setup fix. New helpers (`probePythonArch`, `isArchMismatch`, `detectArm64Python`, `HOST_ARCH`) and the new `/api/image-gen/setup/check` response fields (`interpreterArch`, `hostArch`, `archMismatch`, `suggestedArm64Python`) have no test coverage — the existing `server/routes/imageGen.test.js` skips all `/setup/*` routes entirely. Worth a `pythonSetup.test.js` that mocks `node:os` + `node:child_process` to verify: (1) on `darwin/arm64`, `detectPython` prefers arm64 candidates over x86_64; (2) `/setup/check` includes the new arch fields and only sets `archMismatch: true` when the host is arm64; (3) `suggestedArm64Python` is null when no viable arm64 candidate exists. Deferred from the fix because the `/setup/*` route family has zero existing test harness and adding one is its own piece of work.
109
- [ ] [mediacard-use-mediaimage-for-syncing-assets] **`MediaCard.jsx` grid thumbnails still use a raw `<img src={previewUrl}>`.** Same peer-sync placeholder/live-swap gap that `[peer-sync-medialightbox-use-mediaimage-for-syncing-assets]` fixed for the lightbox — `client/src/components/media/MediaCard.jsx` (~line 42) doesn't get the "Syncing" placeholder or the `peerSync:asset-arrived` atomic swap. Swap the raw `<img>` for `MediaImage`. Surfaced by that item's cross-check during the batch-clear (2026-05-25); was outside its stated scope.
1110
- [ ] [chrome-canary-followups] **Hardening for the custom-Chrome-binary feature (xhigh code-review 2026-05-25).** _DONE in the v2.10.0 release review: (b) both `spawn()` calls now have `.on('error', …)` listeners; (d) `browser/server.js#loadConfig` now try/catches the `JSON.parse`; (e) `setup-browser.js#loadConfig` now warns + returns `null` and `applyCanaryToConfig` skips the save when the existing config is unreadable; (f) top-level `runCanarySetup()` is now `.catch()`-wrapped. (c) is tracked separately in `[setup-browser-canary-headless]`. Remaining: (a), (g)–(o)._ **High-severity:** (a) On macOS headed mode (the default), `browser/server.js:208` uses `macAppBundle` only and silently ignores `chromePath` — a UI user who fills in `chromePath` for Canary/Chromium/Brave but leaves `macAppBundle` empty gets stock Chrome and the log misreports the binary; couple the two fields in the UI (or auto-derive `macAppBundle` from `chromePath` when the latter is inside a `.app`). (b) Neither `spawn(chromePath, …)` (line 216) nor `spawn('/usr/bin/open', …)` (line 208) has an `.on('error', …)` listener — a typo in `chromePath` emits 'error' with no listener → `uncaughtException` → portos-browser PM2 child crashes and restart-loops. (c) `scripts/setup-browser.js#applyCanaryToConfig` writes `chromePath` + `macAppBundle` but doesn't flip `headless: false`; combined with the seed default of `headless: true` (`data.reference/browser-config.json`), a fresh install accepting Canary runs Canary invisibly. (d) `browser/server.js#loadConfig` does bare `JSON.parse(raw)` with no try/catch — combined with setup-browser's non-atomic `writeFileSync` and the `cachedConfig` race, a partial-file write crashes the supervisor on next start (PM2 restart-loop forever). **Medium-severity:** (e) `setup-browser.js#loadConfig` silently catches all JSON parse errors and returns `{}`, then `applyCanaryToConfig` saves `{chromePath, macAppBundle}` only — wiping every other user-customized key. (f) Top-level `await runCanarySetup()` has no try/catch — any EACCES on data/browser-config.json aborts `npm run setup` / `update.sh` at what was previously a no-op step. (g) Idempotency guard only checks `chromePath`: users who decline get re-prompted on every update, and users who set only `macAppBundle` keep getting re-prompted. (h) `PORTOS_USE_CANARY` only matches literal `'0'`/`'false'` (opt-out) or `'1'`/`'true'` (opt-in) — `'no'`/`'off'`/`'yes'`/`'on'`/`'True'` fall through both branches. (i) `spawnSync(install.cmd, …, { stdio: 'inherit' })` for brew/winget can hang on a sudo password prompt under non-TTY + `PORTOS_USE_CANARY=1` (update.sh stalls). (j) `cachedConfig` in `browserService.js` is stale relative to setup-browser's direct write — GET /api/browser/config returns pre-update values until process restart. (k) `saveConfig` uses bare `writeFileSync` — switch to the canonical `atomicWrite` pattern (`server/lib/fileUtils.js`). **Low-severity:** (l) `spawnSync` failure-status check `result.status !== 0` treats `status: null` (spawn-failure / signal kill) identically to a non-zero exit and never logs `result.error` — masks ENOENT/EPERM/SIGKILL. (m) `optionalPath` Zod schema accepts any string up to 1024 chars; no `.app`/`.exe` sanity check — user pastes the bundle into `chromePath` → spawn() EISDIR. (n) `launchBrowser`'s reuse-existing-Chrome early-return (line 167) fires BEFORE `headlessMode = config.headless === true` (line 170), leaving the module-level default `false` after a PM2 restart that reuses Chrome — /health reports wrong mode (pre-existing, but in a function touched by this change). (o) `detectCanary` on macOS only checks `/Applications/...`; misses per-user `~/Applications/Google Chrome Canary.app` installs (corporate Macs, `HOMEBREW_CASK_OPTS=--appdir=$HOME/Applications`).
1211
- [ ] [strip-malloc-env-ffmpeg-children] **Extend `stripDebugMallocEnv` coverage to ffmpeg child spawns if Pinokio users still see the warning.** The Malloc-strip PR (`[ref-watch-phosphene-strip-malloc-subprocess-env]`) routed every direct Node→Python spawn through `stripDebugMallocEnv`, but ffmpeg children (`server/lib/ffmpeg.js:81`, the ffmpeg/ffprobe calls in `server/services/mediaService.js`, `server/services/videoTimeline/local.js`, and the post-render thumbnail/optimize/upscale paths in `server/services/videoGen/local.js`) still inherit `process.env`. ffmpeg on macOS links libSystem (which includes libmalloc) so it can in principle emit the same `MallocStackLogging: can't turn off…` line — unverified whether it actually does. Trigger: if Pinokio-launched installs still report stderr noise after the Python-spawn fix lands, wrap each ffmpeg spawn `env` the same way. Surfaced by reuse-review subagent during the original PR (2026-05-29).

server/lib/pythonSetup.test.js

Lines changed: 202 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,202 @@
1+
import { describe, it, expect, vi, beforeEach } from 'vitest';
2+
3+
// Mutable state read by the hoisted vi.mock factories below. Each test mutates
4+
// it and then `vi.resetModules() + dynamic import` re-evaluates pythonSetup.js
5+
// with the new platform/arch — `HOST_ARCH`, `IS_DARWIN`, and `PYTHON_CANDIDATES`
6+
// are all computed at module-load time, so the only way to exercise both
7+
// arm64 and x86_64 host paths is a fresh module per scenario.
8+
const mockState = {
9+
arch: 'arm64',
10+
platform: 'darwin',
11+
homedir: '/Users/test',
12+
presentPaths: new Set(),
13+
archByPath: new Map(),
14+
execShouldFail: false,
15+
};
16+
17+
vi.mock('node:os', async () => {
18+
const actual = await vi.importActual('node:os');
19+
return {
20+
...actual,
21+
arch: () => mockState.arch,
22+
platform: () => mockState.platform,
23+
homedir: () => mockState.homedir,
24+
};
25+
});
26+
27+
vi.mock('node:fs', async () => {
28+
const actual = await vi.importActual('node:fs');
29+
return { ...actual, existsSync: (p) => mockState.presentPaths.has(p) };
30+
});
31+
32+
vi.mock('node:child_process', async () => {
33+
const actual = await vi.importActual('node:child_process');
34+
const util = await vi.importActual('node:util');
35+
// `promisify(execFile)` uses execFile's util.promisify.custom symbol so the
36+
// resolved value is `{ stdout, stderr }` (not a bare string). Honor that on
37+
// the mock or `probePythonArch`'s `.stdout` read returns undefined.
38+
const fakeExecFile = () => {};
39+
fakeExecFile[util.promisify.custom] = (bin, args) => new Promise((resolve, reject) => {
40+
if (mockState.execShouldFail) {
41+
reject(new Error('spawn failed'));
42+
return;
43+
}
44+
const probeArg = args?.[1] || '';
45+
if (probeArg.includes('platform.machine')) {
46+
const a = mockState.archByPath.get(bin) ?? mockState.arch;
47+
resolve({ stdout: `${a}\n`, stderr: '' });
48+
} else {
49+
resolve({ stdout: '', stderr: '' });
50+
}
51+
});
52+
return { ...actual, execFile: fakeExecFile };
53+
});
54+
55+
vi.mock('./fileUtils.js', () => ({ PATHS: { data: '/data' } }));
56+
57+
const loadModule = async () => {
58+
vi.resetModules();
59+
return await import('./pythonSetup.js');
60+
};
61+
62+
const resetState = () => {
63+
mockState.arch = 'arm64';
64+
mockState.platform = 'darwin';
65+
mockState.homedir = '/Users/test';
66+
mockState.presentPaths = new Set();
67+
mockState.archByPath = new Map();
68+
mockState.execShouldFail = false;
69+
};
70+
71+
describe('HOST_ARCH', () => {
72+
beforeEach(resetState);
73+
74+
it('reports arm64 on Apple Silicon Node', async () => {
75+
mockState.arch = 'arm64';
76+
const { HOST_ARCH } = await loadModule();
77+
expect(HOST_ARCH).toBe('arm64');
78+
});
79+
80+
it('normalizes os.arch x64 → x86_64 so the Python convention matches', async () => {
81+
mockState.arch = 'x64';
82+
const { HOST_ARCH } = await loadModule();
83+
expect(HOST_ARCH).toBe('x86_64');
84+
});
85+
});
86+
87+
describe('probePythonArch', () => {
88+
beforeEach(resetState);
89+
90+
it('returns the trimmed platform.machine() output', async () => {
91+
mockState.archByPath.set('/x/python3', 'arm64');
92+
const { probePythonArch } = await loadModule();
93+
await expect(probePythonArch('/x/python3')).resolves.toBe('arm64');
94+
});
95+
96+
it('returns null when the subprocess errors', async () => {
97+
mockState.execShouldFail = true;
98+
const { probePythonArch } = await loadModule();
99+
await expect(probePythonArch('/missing/python3')).resolves.toBeNull();
100+
});
101+
});
102+
103+
describe('isArchMismatch', () => {
104+
beforeEach(resetState);
105+
106+
it('returns false on non-darwin (mlx wheels are arm64-only on macOS; other OSes are out of scope)', async () => {
107+
mockState.platform = 'linux';
108+
mockState.archByPath.set('/x/python3', 'x86_64');
109+
const { isArchMismatch } = await loadModule();
110+
await expect(isArchMismatch('/x/python3')).resolves.toBe(false);
111+
});
112+
113+
it('returns false when interpreter arch matches HOST_ARCH on darwin', async () => {
114+
mockState.archByPath.set('/x/python3', 'arm64');
115+
const { isArchMismatch } = await loadModule();
116+
await expect(isArchMismatch('/x/python3')).resolves.toBe(false);
117+
});
118+
119+
it('returns true when interpreter arch differs from HOST_ARCH on darwin', async () => {
120+
mockState.archByPath.set('/x/python3', 'x86_64');
121+
const { isArchMismatch } = await loadModule();
122+
await expect(isArchMismatch('/x/python3')).resolves.toBe(true);
123+
});
124+
125+
it('returns false when the probe fails (no interpreter to compare)', async () => {
126+
mockState.execShouldFail = true;
127+
const { isArchMismatch } = await loadModule();
128+
await expect(isArchMismatch('/x/python3')).resolves.toBe(false);
129+
});
130+
});
131+
132+
describe('detectPython on darwin/arm64', () => {
133+
beforeEach(resetState);
134+
135+
it('prefers an arm64 candidate over an earlier-listed x86_64 candidate', async () => {
136+
// /opt/anaconda3 (index 4 in PYTHON_CANDIDATES) is listed before
137+
// /opt/homebrew (index 9). The arm64-preference branch must override
138+
// first-present-wins so mlx wheels load correctly.
139+
mockState.presentPaths.add('/opt/anaconda3/bin/python3');
140+
mockState.presentPaths.add('/opt/homebrew/bin/python3');
141+
mockState.archByPath.set('/opt/anaconda3/bin/python3', 'x86_64');
142+
mockState.archByPath.set('/opt/homebrew/bin/python3', 'arm64');
143+
const { detectPython } = await loadModule();
144+
await expect(detectPython()).resolves.toBe('/opt/homebrew/bin/python3');
145+
});
146+
147+
it('returns the sole present candidate without paying the arch-probe cost', async () => {
148+
// The `present.length > 1` guard skips the parallel probe when there is
149+
// nothing to choose between.
150+
mockState.presentPaths.add('/opt/anaconda3/bin/python3');
151+
mockState.archByPath.set('/opt/anaconda3/bin/python3', 'x86_64');
152+
const { detectPython } = await loadModule();
153+
await expect(detectPython()).resolves.toBe('/opt/anaconda3/bin/python3');
154+
});
155+
156+
it('falls back to the first present candidate when no candidate is arm64', async () => {
157+
mockState.presentPaths.add('/opt/anaconda3/bin/python3');
158+
mockState.presentPaths.add('/usr/local/bin/python3');
159+
mockState.archByPath.set('/opt/anaconda3/bin/python3', 'x86_64');
160+
mockState.archByPath.set('/usr/local/bin/python3', 'x86_64');
161+
const { detectPython } = await loadModule();
162+
await expect(detectPython()).resolves.toBe('/opt/anaconda3/bin/python3');
163+
});
164+
});
165+
166+
describe('detectArm64Python', () => {
167+
beforeEach(resetState);
168+
169+
it('returns null on non-darwin', async () => {
170+
mockState.platform = 'linux';
171+
mockState.presentPaths.add('/usr/bin/python3');
172+
mockState.archByPath.set('/usr/bin/python3', 'arm64');
173+
const { detectArm64Python } = await loadModule();
174+
await expect(detectArm64Python()).resolves.toBeNull();
175+
});
176+
177+
it('returns null on Intel macs (HOST_ARCH !== arm64)', async () => {
178+
mockState.arch = 'x64';
179+
mockState.presentPaths.add('/usr/local/bin/python3');
180+
mockState.archByPath.set('/usr/local/bin/python3', 'x86_64');
181+
const { detectArm64Python } = await loadModule();
182+
await expect(detectArm64Python()).resolves.toBeNull();
183+
});
184+
185+
it('returns null when no present candidate reports arm64', async () => {
186+
mockState.presentPaths.add('/opt/anaconda3/bin/python3');
187+
mockState.archByPath.set('/opt/anaconda3/bin/python3', 'x86_64');
188+
const { detectArm64Python } = await loadModule();
189+
await expect(detectArm64Python()).resolves.toBeNull();
190+
});
191+
192+
it('returns the first arm64 candidate by PYTHON_CANDIDATES order', async () => {
193+
mockState.presentPaths.add('/opt/anaconda3/bin/python3');
194+
mockState.presentPaths.add('/opt/homebrew/bin/python3');
195+
mockState.presentPaths.add('/usr/local/bin/python3');
196+
mockState.archByPath.set('/opt/anaconda3/bin/python3', 'x86_64');
197+
mockState.archByPath.set('/opt/homebrew/bin/python3', 'arm64');
198+
mockState.archByPath.set('/usr/local/bin/python3', 'arm64');
199+
const { detectArm64Python } = await loadModule();
200+
await expect(detectArm64Python()).resolves.toBe('/opt/homebrew/bin/python3');
201+
});
202+
});

0 commit comments

Comments
 (0)