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

@@ -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

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

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 => {