mirror of
https://github.com/stablyai/orca.git
synced 2026-09-30 00:03:15 +00:00
fix(windows): assert the rebuilt addon, and install the patch for real in tests
Three follow-ups from review. **The packaged binary had no check.** The relay build asserts its own artifact and ensure-native-runtime asserts what it loads, but nothing looked at the addon copied into the packaged app -- so a rebuild that silently produced the upstream reader shipped. `rebuild-native-deps.mjs` now asserts `clean` on it after `rebuild()`. This is also the caller D4's tri-state was missing: every existing site branches on `=== 'unpatched'`, so `missing` still behaved exactly like `clean` everywhere, which was the thing making it a state rather than a boolean. Here both non-clean states fail, and they fail differently: after a rebuild that reported success, an absent binary is a broken build, not an absence to shrug at. The fake `rebuild()` had to start producing a binary for that to mean anything, so it now emits stand-in bytes and takes `addon: 'clean' | 'unpatched' | 'none'`. Verified by deletion: with the assertion removed both new cases pass. **The frozen-install case could not see a patch at all.** `--lockfile-only` resolves and never applies one, so its coverage stops at hash consistency. Added a case that installs `@vscode/windows-process-tree@0.8.0` for real with the patch and asserts the materialized `src/process_commandline.cc` carries the marker and no longer carries `ReadProcessMemory` -- about 1.5s for the pair. Correcting the brief on that one: it does **not** catch the `git apply` breakage from the previous commit. Measured -- with `-c core.autocrlf=input` removed it passes cleanly, because `pnpm install` uses pnpm's own patch applier and never runs our repair script. What it does catch is a patch pnpm can no longer apply: corrupting one pre-image line fails both cases. The repair path stays covered by the CRLF fixture in rebuild-native-deps-node-pty.test.mjs. Worth recording, since it decides whether the LF normalization was safe at all: pnpm applies the LF patch to the CRLF tarball sources without complaint, and materializes them as LF with the marker present and `ReadProcessMemory` absent. The primary install path was never affected -- only the `git apply` fallback was. **Dead timeout.** The frozen-install case passed `timeoutMs: 300_000` to the spawn while vitest capped the case itself at 30s, so on a cold runner vitest would have killed it first. Both cases now declare the budget they use.
This commit is contained in:
@@ -1,4 +1,12 @@
|
||||
import { cpSync, copyFileSync, existsSync, mkdirSync, mkdtempSync } from 'node:fs'
|
||||
import {
|
||||
cpSync,
|
||||
copyFileSync,
|
||||
existsSync,
|
||||
mkdirSync,
|
||||
mkdtempSync,
|
||||
readFileSync,
|
||||
writeFileSync
|
||||
} from 'node:fs'
|
||||
import { tmpdir } from 'node:os'
|
||||
import { delimiter, join, resolve } from 'node:path'
|
||||
import { describe, expect, it } from 'vitest'
|
||||
@@ -20,6 +28,7 @@ import { removeTreeSync } from '../../src/shared/windows-transient-lock-removal.
|
||||
* with no node_modules and no native builds.
|
||||
*/
|
||||
const PROJECT_DIR = resolve(import.meta.dirname, '../..')
|
||||
const WINDOWS_PROCESS_TREE_PATCH = '@vscode__windows-process-tree@0.8.0.patch'
|
||||
|
||||
/** runProcessSync wants an absolute program on Windows, where pnpm is a `.cmd` shim. */
|
||||
function resolvePnpmProgram() {
|
||||
@@ -63,5 +72,73 @@ describe('patched dependencies', () => {
|
||||
} finally {
|
||||
removeTreeSync(scratch)
|
||||
}
|
||||
})
|
||||
// The 300s spawn budget is only reachable if the case is allowed to take it;
|
||||
// config/vitest.config.ts caps every case at 30s by default.
|
||||
}, 300_000)
|
||||
|
||||
/**
|
||||
* `--lockfile-only` resolves; it never applies a patch. So the case above is
|
||||
* bounded to hash consistency, and the actual question -- can pnpm still put
|
||||
* the patched reader on disk? -- had nothing covering it.
|
||||
*
|
||||
* One package, patch applied for real, assert the marker landed. Scoped to the
|
||||
* single dependency so it stays a ~2s check rather than a full install.
|
||||
*/
|
||||
it('materializes the patched command-line reader on a real install', () => {
|
||||
const pnpm = resolvePnpmProgram()
|
||||
expect(pnpm, 'pnpm must be on PATH; it is the only thing that can check this').not.toBeNull()
|
||||
|
||||
const scratch = mkdtempSync(join(tmpdir(), 'orca-patch-apply-'))
|
||||
try {
|
||||
mkdirSync(join(scratch, 'config', 'patches'), { recursive: true })
|
||||
copyFileSync(
|
||||
join(PROJECT_DIR, 'config', 'patches', WINDOWS_PROCESS_TREE_PATCH),
|
||||
join(scratch, 'config', 'patches', WINDOWS_PROCESS_TREE_PATCH)
|
||||
)
|
||||
writeFileSync(
|
||||
join(scratch, 'package.json'),
|
||||
`${JSON.stringify(
|
||||
{
|
||||
name: 'orca-patch-apply-probe',
|
||||
version: '1.0.0',
|
||||
dependencies: { '@vscode/windows-process-tree': '0.8.0' }
|
||||
},
|
||||
null,
|
||||
2
|
||||
)}\n`
|
||||
)
|
||||
writeFileSync(
|
||||
join(scratch, 'pnpm-workspace.yaml'),
|
||||
'packages: []\n' +
|
||||
'patchedDependencies:\n' +
|
||||
` '@vscode/windows-process-tree@0.8.0': config/patches/${WINDOWS_PROCESS_TREE_PATCH}\n`
|
||||
)
|
||||
|
||||
const result = runProcessSync({
|
||||
program: pnpm,
|
||||
args: ['install', '--no-frozen-lockfile', '--ignore-scripts'],
|
||||
cwd: scratch,
|
||||
timeoutMs: 300_000
|
||||
})
|
||||
expect(result.code, `${result.stdout}\n${result.stderr}`).toBe(0)
|
||||
|
||||
const materialized = readFileSync(
|
||||
join(
|
||||
scratch,
|
||||
'node_modules',
|
||||
'@vscode',
|
||||
'windows-process-tree',
|
||||
'src',
|
||||
'process_commandline.cc'
|
||||
),
|
||||
'utf8'
|
||||
)
|
||||
expect(materialized).toContain('kProcessCommandLineInformation')
|
||||
// The whole point of the patch: the upstream reader is gone, not merely
|
||||
// supplemented.
|
||||
expect(materialized).not.toContain('ReadProcessMemory')
|
||||
} finally {
|
||||
removeTreeSync(scratch)
|
||||
}
|
||||
}, 300_000)
|
||||
})
|
||||
|
||||
@@ -344,4 +344,37 @@ describe('rebuild-native-deps patched node-pty rebuild', () => {
|
||||
}
|
||||
}
|
||||
)
|
||||
|
||||
// The binary this step produces is the one copied into the packaged app. The
|
||||
// relay build checks its own artifact and ensure-native-runtime checks what it
|
||||
// loads; nothing checked this one, so a rebuild that quietly emitted the
|
||||
// upstream reader shipped. Both non-clean states have to fail, which is the
|
||||
// caller the tri-state was missing: after a rebuild that reported success, an
|
||||
// absent binary is a broken build, not an absence to shrug at.
|
||||
for (const [addon, expected] of [
|
||||
['unpatched', 'still imports ReadProcessMemory'],
|
||||
['none', 'is not there']
|
||||
]) {
|
||||
it(`fails a Windows rebuild that leaves ${addon} windows-process-tree bytes`, () => {
|
||||
const projectDir = mkTempProject()
|
||||
|
||||
try {
|
||||
writeFakeUsableElectronPackage(projectDir, { platform: 'win32' })
|
||||
writeFakeElectronRebuild(projectDir, { addon })
|
||||
writeFakeNodePtyConptyPayload(projectDir, 'x64')
|
||||
writeFakeWindowsProcessTreeWithNodeAddonApi(projectDir)
|
||||
|
||||
const result = runRebuildScript(
|
||||
projectDir,
|
||||
{ npm_config_platform: 'win32', npm_config_arch: 'x64' },
|
||||
['--platform=win32', '--arch=x64', '--force']
|
||||
)
|
||||
|
||||
expect(result.status).not.toBe(0)
|
||||
expect(result.stderr).toContain(expected)
|
||||
} finally {
|
||||
removeTreeSync(projectDir)
|
||||
}
|
||||
})
|
||||
}
|
||||
})
|
||||
|
||||
@@ -212,17 +212,46 @@ if (${JSON.stringify(createExecutable)}) {
|
||||
)
|
||||
}
|
||||
|
||||
export function writeFakeElectronRebuild(projectDir, { logPathEnv = null } = {}) {
|
||||
/** Bytes that stand in for a compiled addon's import table. */
|
||||
const FAKE_ADDON_BYTES = {
|
||||
clean: 'MZ\0ntdll.dll\0NtQueryInformationProcess\0',
|
||||
unpatched: 'MZ\0KERNEL32.dll\0ReadProcessMemory\0'
|
||||
}
|
||||
|
||||
/**
|
||||
* A rebuild that produces nothing leaves no addon to inspect, and the script now
|
||||
* asserts the binary it just built is a patched one. Emit a stand-in so the
|
||||
* fixture models a rebuild that actually succeeded. `addon` picks which kind,
|
||||
* because "produced the upstream reader" and "produced nothing" are both real
|
||||
* outcomes that assertion has to tell apart.
|
||||
*/
|
||||
export function writeFakeElectronRebuild(projectDir, { logPathEnv = null, addon = 'clean' } = {}) {
|
||||
const rebuildDir = join(projectDir, 'node_modules', '@electron', 'rebuild')
|
||||
mkdirSync(rebuildDir, { recursive: true })
|
||||
writeFileSync(join(rebuildDir, 'package.json'), JSON.stringify({ type: 'module' }))
|
||||
const emitAddon =
|
||||
addon === 'none'
|
||||
? ''
|
||||
: `
|
||||
const packageDir = join('node_modules', '@vscode', 'windows-process-tree')
|
||||
if (existsSync(join(packageDir, 'package.json'))) {
|
||||
mkdirSync(join(packageDir, 'build', 'Release'), { recursive: true })
|
||||
writeFileSync(
|
||||
join(packageDir, 'build', 'Release', 'windows_process_tree.node'),
|
||||
${JSON.stringify(FAKE_ADDON_BYTES[addon])}
|
||||
)
|
||||
}`
|
||||
const emitImports =
|
||||
addon === 'none'
|
||||
? ''
|
||||
: "import { existsSync, mkdirSync, writeFileSync } from 'node:fs'\nimport { join } from 'node:path'\n"
|
||||
writeFileSync(
|
||||
join(rebuildDir, 'index.js'),
|
||||
logPathEnv
|
||||
? `
|
||||
import { appendFileSync } from 'node:fs'
|
||||
|
||||
export async function rebuild(options) {
|
||||
${emitImports}
|
||||
export async function rebuild(options) {${emitAddon}
|
||||
const logPath = process.env[${JSON.stringify(logPathEnv)}]
|
||||
if (!logPath) {
|
||||
return
|
||||
@@ -240,7 +269,10 @@ export async function rebuild(options) {
|
||||
)
|
||||
}
|
||||
`
|
||||
: 'export async function rebuild() {}\n'
|
||||
: `${emitImports}
|
||||
export async function rebuild() {${emitAddon}
|
||||
}
|
||||
`
|
||||
)
|
||||
}
|
||||
|
||||
|
||||
@@ -22,7 +22,9 @@ import { rebuild } from '@electron/rebuild'
|
||||
import { execFileSync, spawnSync } from 'node:child_process'
|
||||
import {
|
||||
ensureWindowsProcessTreeCommandLinePatch,
|
||||
stageWindowsProcessTreeNodeAddonApiHeaders
|
||||
inspectWindowsProcessTreeAddon,
|
||||
stageWindowsProcessTreeNodeAddonApiHeaders,
|
||||
windowsProcessTreeAddonPath
|
||||
} from './windows-process-tree-gyp-rebuild.mjs'
|
||||
import {
|
||||
copyFileSync,
|
||||
@@ -174,6 +176,7 @@ try {
|
||||
force: true
|
||||
})
|
||||
restoreNodePtyWindowsConptyRuntime()
|
||||
assertWindowsProcessTreeAddonIsPatched()
|
||||
} catch (/** @type {any} */ err) {
|
||||
console.error('[rebuild] Native module rebuild failed:', err?.message ?? err)
|
||||
if (isWindowsNativeLockError(err)) {
|
||||
@@ -193,6 +196,40 @@ try {
|
||||
process.exit(1)
|
||||
}
|
||||
|
||||
/**
|
||||
* The binary this rebuild just produced is the one the packaged app ships.
|
||||
*
|
||||
* The relay build asserts its own artifact and `ensure-native-runtime.mjs`
|
||||
* asserts what it loads, but nothing checked the addon that gets copied into the
|
||||
* packaged `node_modules` -- so a rebuild that silently produced the upstream
|
||||
* reader would reach users. Anything but `clean` fails: after a rebuild that
|
||||
* reported success the binary must exist, so `missing` is a broken build, not an
|
||||
* absence to shrug at. This is the caller that needs the state to be a state and
|
||||
* not a boolean.
|
||||
*/
|
||||
function assertWindowsProcessTreeAddonIsPatched() {
|
||||
if (
|
||||
rebuildPlatform !== 'win32' ||
|
||||
!modulesToRebuild.includes('@vscode/windows-process-tree') ||
|
||||
!existsSync(join(projectDir, 'node_modules', '@vscode', 'windows-process-tree', 'package.json'))
|
||||
) {
|
||||
return
|
||||
}
|
||||
const addonPath = windowsProcessTreeAddonPath()
|
||||
const state = inspectWindowsProcessTreeAddon(addonPath)
|
||||
if (state === 'clean') {
|
||||
return
|
||||
}
|
||||
throw new Error(
|
||||
state === 'missing'
|
||||
? `the rebuild reported success but ${addonPath} is not there, so the packaged app would ` +
|
||||
'ship no windows-process-tree addon at all.'
|
||||
: `${addonPath} still imports ReadProcessMemory, so it was not built from the patched ` +
|
||||
'command-line reader. The packaged app would carry the primitive MDE scores as ' +
|
||||
'credential dumping.'
|
||||
)
|
||||
}
|
||||
|
||||
function restoreNodePtyWindowsConptyRuntime() {
|
||||
if (rebuildPlatform !== 'win32' || !onlyModules.includes('node-pty')) {
|
||||
return
|
||||
|
||||
Reference in New Issue
Block a user