From a8ce5962d3f4ec0069400afa17e9916cde949706 Mon Sep 17 00:00:00 2001 From: Paul Bakaus Date: Tue, 8 Sep 2026 19:20:16 -0700 Subject: [PATCH] Fix silent launcher setup failures (#788) * Fix silent launcher setup failures Report cache creation, cache write, and download failures with recovery guidance while preserving lazy engine downloads. AI-assisted by Codex. * Fix Windows staging-write failure handling Branch directly on redirection failure and reject staging directories before cleanup. Add coverage for an existing read-only staging file. AI-assisted by Codex. --- skill/scripts/impeccable | 21 ++++++++++- skill/scripts/impeccable.cmd | 39 +++++++++++++++++-- tests/launcher-download.test.mjs | 64 ++++++++++++++++++++++++++++++-- 3 files changed, 116 insertions(+), 8 deletions(-) diff --git a/skill/scripts/impeccable b/skill/scripts/impeccable index a25e42302..eb60f8667 100755 --- a/skill/scripts/impeccable +++ b/skill/scripts/impeccable @@ -87,6 +87,11 @@ fi # Last resort: fetch this version's binary for the current platform from the # public release channel into the user cache. Needs network; sandboxes without # egress preinstall the binary on PATH instead. +setup_help() { + echo "Engine $version setup needs network access and write permission to $cache_root/bin/$version." >&2 + echo "Run this launcher ($0) with engine-probe in a terminal that has those permissions, then retry the original command." >&2 + echo "Alternatively, set IMPECCABLE_HOME to a writable cache location, or IMPECCABLE_BIN to a preinstalled engine binary." >&2 +} fetch_url() { if command -v curl >/dev/null 2>&1; then curl -fsSL --retry 2 -o "$tmp" "$1" 2>/dev/null @@ -119,7 +124,18 @@ if [ -n "$version" ] && [ "$os" != unknown ] && [ "$arch" != unknown ]; then [ "$os" = windows ] && asset="$asset.exe" url="$base/engine-v$version/$asset" tmp="$cache_root/bin/$version/.impeccable.part.$$" - mkdir -p "$cache_root/bin/$version" 2>/dev/null + if ! mkdir -p "$cache_root/bin/$version" 2>/dev/null; then + echo "impeccable: engine $version is not installed; cannot create cache directory: $cache_root/bin/$version" >&2 + setup_help + exit 127 + fi + # Check the actual staging file, not just directory existence: a cache from + # an earlier run can be readable but no longer writable inside a sandbox. + if ! (umask 077; : > "$tmp") 2>/dev/null; then + echo "impeccable: engine $version is not installed; cannot write to cache directory: $cache_root/bin/$version" >&2 + setup_help + exit 127 + fi fetched=0 if fetch_url "$url"; then fetched=1 @@ -180,6 +196,9 @@ if [ -n "$version" ] && [ "$os" != unknown ] && [ "$arch" != unknown ]; then exec "$cached" "$@" fi rm -f "$tmp" 2>/dev/null + echo "impeccable: could not download engine $version from $url; check network access, the release URL, and curl or wget availability." >&2 + setup_help + exit 127 fi echo "impeccable: no engine binary for $os-$arch found (looked in $bin, $cached, PATH)." >&2 diff --git a/skill/scripts/impeccable.cmd b/skill/scripts/impeccable.cmd index 9c285c910..7ebe419bf 100644 --- a/skill/scripts/impeccable.cmd +++ b/skill/scripts/impeccable.cmd @@ -68,18 +68,27 @@ rem another launcher's probe: fail fast and quiet instead. if defined IMPECCABLE_LAUNCHER_PROBE exit /b 127 if not defined version goto fail where curl.exe >nul 2>nul -if errorlevel 1 goto fail +if errorlevel 1 goto curl_missing if not defined IMPECCABLE_DOWNLOAD_BASE set "IMPECCABLE_DOWNLOAD_BASE=https://github.com/pbakaus/impeccable/releases/download" -if not exist "%IMPECCABLE_HOME%\bin\%version%" mkdir "%IMPECCABLE_HOME%\bin\%version%" >nul 2>nul +if exist "%IMPECCABLE_HOME%\bin\%version%\" goto cache_ready +mkdir "%IMPECCABLE_HOME%\bin\%version%" >nul 2>nul +if errorlevel 1 goto cache_directory_failed +:cache_ready +rem Check the staging file too: an existing directory may be read-only. +rem Redirection failures do not reliably update ERRORLEVEL in cmd.exe; +rem branch on the command's failure directly. Never treat a directory as a +rem staging file (later del cleanup would prompt to delete its contents). +if exist "%cached%.part\" goto cache_write_failed +(type nul >"%cached%.part") 2>nul || goto cache_write_failed set "asset=impeccable-windows-%arch%.exe" set "url=%IMPECCABLE_DOWNLOAD_BASE%/engine-v%version%/%asset%" curl.exe -fsSL -o "%cached%.part" "%url%" >nul 2>nul if not errorlevel 1 goto verify -if not "%arch%"=="arm64" goto fail +if not "%arch%"=="arm64" goto download_failed set "asset=impeccable-windows-x64.exe" set "url=%IMPECCABLE_DOWNLOAD_BASE%/engine-v%version%/%asset%" curl.exe -fsSL -o "%cached%.part" "%url%" >nul 2>nul -if errorlevel 1 goto fail +if errorlevel 1 goto download_failed :verify call :check_download @@ -176,6 +185,28 @@ if not errorlevel 1 set "probe_ok=1" del "%probe_tmp%" >nul 2>nul exit /b 0 +:cache_directory_failed +echo impeccable: engine %version% is not installed; cannot create cache directory: "%IMPECCABLE_HOME%\bin\%version%" 1>&2 +goto setup_failed + +:cache_write_failed +echo impeccable: engine %version% is not installed; cannot write to cache directory: "%IMPECCABLE_HOME%\bin\%version%" 1>&2 +goto setup_failed + +:curl_missing +echo impeccable: cannot download engine %version%; curl.exe is unavailable. 1>&2 +goto setup_failed + +:download_failed +del "%cached%.part" >nul 2>nul +echo impeccable: could not download engine %version% from %url%; check network access and the release URL. 1>&2 + +:setup_failed +echo Engine %version% setup needs network access and write permission to "%IMPECCABLE_HOME%\bin\%version%". 1>&2 +echo Run this launcher ("%~f0") with engine-probe in a terminal that has those permissions, then retry the original command. 1>&2 +echo Alternatively, set IMPECCABLE_HOME to a writable cache location, or IMPECCABLE_BIN to a preinstalled engine binary. 1>&2 +exit /b 127 + :fail del "%cached%.part" >nul 2>nul echo impeccable: no engine binary found (looked in %bin%, %cached%, PATH). 1>&2 diff --git a/tests/launcher-download.test.mjs b/tests/launcher-download.test.mjs index 2e4a8759f..862731075 100644 --- a/tests/launcher-download.test.mjs +++ b/tests/launcher-download.test.mjs @@ -29,8 +29,29 @@ async function exercise(t, scenario) { const launcher = path.join(scripts, name); fs.copyFileSync(path.join(ROOT, 'skill/scripts', name), launcher); const cacheDir = path.join(cache, 'bin', '0.0.0-test'); + if (scenario === 'cache-directory-failure') { + // A file where the cache parent belongs makes mkdir fail on every OS, + // including privileged test runners where chmod cannot deny writes. + fs.mkdirSync(cache); + fs.writeFileSync(path.join(cache, 'bin'), 'blocked'); + } + if (scenario === 'cache-write-failure') { + fs.mkdirSync(cacheDir, { recursive: true }); + if (WINDOWS) fs.mkdirSync(path.join(cacheDir, 'impeccable.exe.part')); + } + if (scenario === 'cache-readonly-file') { + fs.mkdirSync(cacheDir, { recursive: true }); + const staging = path.join(cacheDir, 'impeccable.exe.part'); + fs.writeFileSync(staging, 'read-only'); + fs.chmodSync(staging, 0o444); + } const tools = path.join(root, 'tools'); fs.mkdirSync(tools); + if (!WINDOWS && scenario === 'cache-write-failure') { + // The POSIX staging name contains the launcher's PID, so intercept the + // preceding mkdir to place a directory at precisely that file path. + fs.writeFileSync(path.join(tools, 'mkdir'), '#!/bin/sh\n/bin/mkdir "$@" || exit $?\n/bin/mkdir "$IMPECCABLE_HOME/bin/0.0.0-test/.impeccable.part.$PPID"\n', { mode: 0o755 }); + } if (!WINDOWS && ['hash-failure', 'removed-during-hash'].includes(scenario)) { fs.writeFileSync(path.join(tools, 'shasum'), `#!/bin/sh\n${scenario === 'removed-during-hash' ? 'rm -f "$3"\n' : ''}printf '%s %s\\n' '${HASH}' "$3"\nexit ${scenario === 'hash-failure' ? 1 : 0}\n`, @@ -71,6 +92,10 @@ async function exercise(t, scenario) { const requests = []; const server = http.createServer((req, res) => { requests.push(req.url); + if (scenario === 'transport-failure') { + req.socket.destroy(); + return; + } if (req.url.endsWith('.sha256')) { const part = fs.readdirSync(cacheDir).find(file => file.includes('.part')); if (scenario === 'removed') fs.unlinkSync(path.join(cacheDir, part)); @@ -78,6 +103,7 @@ async function exercise(t, scenario) { res.writeHead(scenario === 'no-sidecar' ? 404 : 200); res.end(scenario === 'empty-sidecar' ? '' : `${scenario === 'mismatch' ? '0'.repeat(64) : HASH} engine\n`); } else { + if (scenario === 'download-failure') res.writeHead(404); res.end(scenario === 'empty-download' ? '' : PAYLOAD); } }); @@ -92,7 +118,7 @@ async function exercise(t, scenario) { IMPECCABLE_DOWNLOAD_BASE: `http://127.0.0.1:${server.address().port}`, ...(WINDOWS ? { SystemRoot: process.env.SystemRoot, ComSpec: COMSPEC, PROCESSOR_ARCHITECTURE: 'AMD64' } : {}), }; - const result = await new Promise((resolve, reject) => { + const run = () => new Promise((resolve, reject) => { const child = WINDOWS ? spawn(COMSPEC, ['/d', '/s', '/c', `""${launcher}" /d /c echo verified-engine"`], { env, cwd: root, windowsVerbatimArguments: true, timeout: 20000 }) : spawn('/bin/sh', [launcher], { env, cwd: root, timeout: 20000 }); @@ -103,9 +129,41 @@ async function exercise(t, scenario) { child.on('error', reject); child.on('close', (status, signal) => resolve({ status, signal, stdout, stderr })); }); + const result = await run(); + if (scenario === 'valid') { + const requestCount = requests.length; + const cachedResult = await run(); + assert.equal(cachedResult.status, 0, cachedResult.stderr); + assert.match(cachedResult.stdout, /verified-engine/); + assert.equal(cachedResult.stderr, '', 'cached execution stays quiet'); + assert.equal(requests.length, requestCount, 'subsequent runs use the cached engine without network'); + } assert.equal(result.signal, null, JSON.stringify(result)); - assert.equal(requests.filter(url => !url.endsWith('.sha256')).length, 1, 'one binary download, no verification retry loop'); - return { ...result, files: fs.readdirSync(cacheDir), requests }; + const cacheFailure = scenario.startsWith('cache-'); + assert.equal(requests.filter(url => !url.endsWith('.sha256')).length, cacheFailure ? 0 : 1, + 'cache failures do not attempt a download; other scenarios download once'); + return { ...result, files: fs.existsSync(cacheDir) ? fs.readdirSync(cacheDir) : [], requests, cacheDir }; +} + +for (const scenario of ['cache-directory-failure', 'cache-write-failure', 'download-failure', 'transport-failure', ...(WINDOWS ? ['cache-readonly-file'] : [])]) { + test(`launcher explains ${scenario} and how to retry setup`, async t => { + const result = await exercise(t, scenario); + assert.equal(result.status, 127, JSON.stringify(result)); + assert.doesNotMatch(result.stdout, /verified-engine/); + assert.match(result.stderr, /engine 0\.0\.0-test/); + assert.ok(result.stderr.includes(result.cacheDir), result.stderr); + assert.match(result.stderr, /engine-probe/); + assert.match(result.stderr, /IMPECCABLE_HOME/); + assert.match(result.stderr, /IMPECCABLE_BIN/); + if (['download-failure', 'transport-failure'].includes(scenario)) { + assert.match(result.stderr, /could not download/); + assert.match(result.stderr, /network/); + assert.deepEqual(result.files, [], 'failed downloads leave no staging files'); + assert.equal(result.requests.length, 1, 'no verification without a download'); + } else { + assert.match(result.stderr, scenario === 'cache-directory-failure' ? /cannot create/ : /cannot write/); + } + }); } test('launcher downloads and runs a verified executable', async t => {