feat: Improve error handling

[kroese]

No description provided.

[Copilot]

Pull request overview

This PR adds direct metadata detection for standalone Microsoft .esd downloads (before extraction) and refactors several image/answer-file detection paths to return status codes instead of exiting, improving how callers can decide between fallback behaviors vs. hard failures.

Changes:

• Add detectESDImage() to parse ESD XML metadata, select an installable image, and run the existing detection/configuration flow.
• Update installer flow to call ESD detection early and tighten reuse of the original bootable ISO to direct-image sources only.
• Adjust several detection/configuration functions to return (instead of exit) and normalize some error messaging.

Reviewed changes

Copilot reviewed 4 out of 4 changed files in this pull request and generated 3 comments.

File	Description
src/mido.sh	Adjusts checksum-mismatch reporting behavior/message during download verification.
src/install.sh	Adds ESD pre-detection branch and tightens original-ISO reuse conditions.
src/image.sh	Refactors detection functions to return status codes; adds ESD metadata detection and improves ISO metadata error reporting.
src/answer.sh	Converts a hard exit to a return code for better caller-controlled fallback behavior; minor pipeline formatting.

---

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (2)

src/mido.sh:1121

• This warning is misleading: the checksum isn't "unknown", it's a mismatch, and the punctuation/spacing makes it harder to read.

  warn "the downloaded file has an unknown $algo checksum: $hash , as the expected value was: $check. Please report this at $SUPPORT/issues"

src/install.sh:85

• In the .esd path, a failed detectESDImage() currently returns 76 immediately, which prevents falling back to the extraction-based path (extractImage() supports .esd via extractESD). This makes ESD handling brittle: any metadata parsing hiccup blocks the only viable route to proceed.

  elif [[ "${iso,,}" == *.esd ]]; then

    detectESDImage "$iso" && return 0
    return 76

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/mido.sh:1125

• Checksum mismatch is a deterministic validation failure (see downloadFile() treating verifyFile status 1 as invalid download). Logging it as warn can reduce visibility in CI/console output, and the message has inconsistent capitalization and spacing before the comma.

  warn "the downloaded file has an unknown $algo checksum: $hash , as the expected value was: $check. Please report this at $SUPPORT/issues"

src/mido.sh:226

• Similar to SKU parsing above: converting jq failures into an empty link can hide malformed responses and makes the error message less accurate. Consider explicitly detecting jq failures and treating them as a parsing error while still logging the raw response.

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

  if [ -z "$link" ] || [[ "${link,,}" == "null" ]]; then
    error "Microsoft server provided us no download link to our request for an automated download!"
    info "Response: $linkJson"
    return 1
  fi

src/mido.sh:187

• This treats any jq failure the same as an empty SKU ID (because stderr is suppressed and the failure is converted to an empty string). That can misreport a malformed/blocked API response as “No download in …”. Consider handling jq parse failures explicitly and logging the response for debugging.

This issue also appears on line 220 of the same file.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

  if [ -z "$skuId" ] || [[ "${skuId,,}" == "null" ]]; then
    if [[ "${lang,,}" != "en" && "${lang,,}" != "en-"* ]]; then
      language=$(getLanguage "$lang" "desc")
      error "No download in the $language language available for $desc!"
    else
      error "Microsoft server provided us no SKU ID in response to our request!"
      info "Response: $skuJson"
    fi

src/image.sh:1402

• The selected INDEX is interpolated into xmlstarlet XPath expressions below. Validate it is a positive integer before using it to avoid unexpected XPath behavior if detectVersion ever returns a non-numeric index.

  mapfile -t detected <<< "$output"
  index="${detected[1]:-}"

  if [ -z "$index" ]; then
    error "Failed to select an installation image based on the ESD metadata!"
    return 1
  fi

  # extractESD removes every other image, leaving the selected edition at
  # index 1. Detect against that final layout so the generated answer file
  # already references the index that will exist after extraction.
  if ! image_info=$(xmlstarlet ed \
      -d "/WIM/IMAGE[number(@INDEX) != $index]" \
      -u "/WIM/IMAGE[@INDEX='$index']/@INDEX" -v '1' \
      <<< "$install_info" 2>/dev/null); then
    error "Cannot prepare ESD image information!"
    return 2
  fi

src/image.sh:1365

• detectESDImage() reads full ESD XML metadata via wimlib-imagex+iconv, but extractESD() later re-reads the same metadata (image.sh:1505-1510). For solid-compressed ESDs this can be expensive; consider caching/passing the already-read XML into extractESD to avoid a second scan.

  image_info=$(wimlib-imagex info "$iso" --xml 2>/dev/null |
    iconv -f UTF-16LE -t UTF-8 2>/dev/null) || {
    rc=$?
    error "Cannot read ESD file information (status $rc)." >&2
    return 2
  }

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/mido.sh:1125

• The checksum failure message says “unknown … checksum” (it’s actually a mismatch) and includes inconsistent capitalization/spacing ($hash ,). This is a hard verification failure (return 1), so the message should be clear and consistently formatted.

  warn "the downloaded file has an unknown $algo checksum: $hash , as the expected value was: $check. Please report this at $SUPPORT/issues"

src/mido.sh:178

• jq parse failures are currently swallowed via || skuId="", which can turn malformed/blocked API responses into a misleading “No download in the language…” error. Handle parse failures explicitly (and optionally log the raw response) before treating empty output as “not found”.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

src/mido.sh:220

• jq failures are suppressed via || link="", which can hide malformed connector responses and produce a generic “no download link” error. Consider failing fast on parse errors so the diagnostic is accurate.

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

src/image.sh:1359

• output is declared twice (local ... output ... and later local output). This is redundant and makes the function harder to scan.

  local image_info install_info output index rc
  local -a detected=()

  image_info=$(wimlib-imagex info "$iso" --xml 2>/dev/null |
    iconv -f UTF-16LE -t UTF-8 2>/dev/null) || {
    rc=$?
    error "Cannot read ESD file information (status $rc)." >&2
    return 2
  }

  # Microsoft download ESDs use images 1-3 for setup media, WinPE, and Windows
  # Setup; images 4 and higher contain installable editions.
  if ! install_info=$(xmlstarlet ed \
      -d '/WIM/IMAGE[number(@INDEX) < 4]' \
      <<< "$image_info" 2>/dev/null); then
    error "Cannot read installable images from ESD file!"
    return 2
  fi

  checkPlatform "$image_info" || return 2

  local output
  output=$(detectVersion "$install_info") || {

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (7)

src/mido.sh:220

• Similarly, jq parse failures for the download link are silenced and converted into an empty string, which makes malformed responses look like a normal “missing link” case. Returning a parse-specific error here will make debugging much easier.

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

src/mido.sh:1125

• Checksum mismatch causes the function to fail (return 1), but it now logs as a lowercase warn with awkward punctuation ($hash ,). This should be an error (or at least consistently capitalized) with cleaner formatting so failures are obvious in logs.

  warn "the downloaded file has an unknown $algo checksum: $hash , as the expected value was: $check. Please report this at $SUPPORT/issues"

src/answer.sh:2820

• disableAutoReboot ignores grep errors (exit 2) and also ignores failures from sed/unix2dos (|| :), but still returns success. This can silently leave AutoReboot enabled even when the edit failed (e.g., permissions, I/O error, bad regex). Handle grep status 2 explicitly and propagate edit failures.

      '    If Left(sid, 9) = "S-1-5-21-" And Right(sid, 4) = "-500" Then' \
      '      LocalAdminADsPath = DomainItem.ADsPath' \
      '      Exit For' \
      '    End If' '  End If' 'Next' '' "Call Domain.MoveHere(LocalAdminADsPath, \"$username\")" ''

src/answer.sh:2452

• The write to setupp.ini is currently best-effort (|| :), so failures to update the PID are silently ignored. Since the caller treats setLegacyKey failures as fatal, it’s better to propagate the write error here (or at least warn and return non-zero).

This issue also appears on line 2533 of the same file.

  copyStorageDriver "$dir" "$target" "$driver" "$arch" "$drivers" || return 1
  addNetworkDriver "$dir" "$driver" "$arch" "$drivers" || return 1

src/answer.sh:2535

• Same issue here: ignoring failures to write back to setupp.ini can hide real I/O/permission problems and lead to confusing downstream install failures. Propagate the error so the caller can stop early with a clear message.

    fi

    if [ -n "$key" ]; then

src/mido.sh:178

• jq parse failures are currently silenced (2>/dev/null) and treated as an empty SKU ID, which can misreport a malformed Microsoft response as “no SKU ID”. Consider failing fast on jq errors and surfacing the response for debugging.

This issue also appears on line 220 of the same file.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

src/image.sh:279

• When xmlstarlet fails, this path logs a warning but returns success (return 0). That makes metadata parse failures indistinguishable from a valid “language not detected” case and can let later steps proceed with bad inputs. Propagate a non-zero status here so callers can treat it as a real inspection failure.

    if ! lang=$(xmlstarlet sel -T -t -v "normalize-space(string(($path)[1]))" - 2>/dev/null <<< "$xml"); then
      warn "failed to read language metadata from Windows image!"
      return 0
    fi

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/answer.sh:2571

• setLegacyKey also ignores failures when writing the updated PID suffix (echo ... || :). If this write fails, the function continues as if it prepared the media, but the installation may still prompt for an unexpected key type. Return non-zero on write failure.

  if [ "${#pid}" -ge 3 ]; then
    echo "${pid::-3}000" > "$setup" || :
  fi

src/answer.sh:2858

• disableAutoReboot swallows grep/sed/unix2dos failures (using || :) and returns success even when updating HIVESYS.INF fails. With set -e, this also makes it harder to distinguish “pattern not found” (rc=1) from real errors (rc=2). Propagate failures and handle unexpected grep exit codes explicitly so the caller can abort instead of silently producing an unchanged image.

  case "$rc" in
    0 )
      sed -i -E "s|(${pattern})[[:space:]]*[^,;[:space:]]+|\\1 0|I" "$file" || :
      ;;
    1 )

src/answer.sh:2489

• setLegacyKey currently ignores failures when writing back to setupp.ini (echo ... || :). If the write fails (read-only media, permissions, full disk), the function still returns success and later steps may behave inconsistently. Propagate the failure so the install can stop with a clear error.

This issue also appears on line 2569 of the same file.

  if [[ "$driver" == "2k" ]]; then
    [ "${#pid}" -ge 3 ] || return 0
    echo "${pid::-3}270" > "$setup" || :
    return 0

src/mido.sh:1125

• verifyFile reports a checksum mismatch as an “unknown checksum” and logs it with warn-level severity, even though the function returns failure. This message is misleading (the checksum is known; it just doesn’t match) and the severity should be error for a verification failure.

  warn "the downloaded file has an unknown $algo checksum: $hash , as the expected value was: $check. Please report this at $SUPPORT/issues"

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/mido.sh:220

• Similar to the SKU parsing above: treating jq parse failures as an empty link makes malformed API responses look like a normal missing-link response. Handle jq failure explicitly and return an error so callers don’t continue under a misleading diagnosis.

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

src/answer.sh:2571

• Same issue as above: the final setupp.ini write is now best-effort (|| :), which can silently fail and still return success. It should return non-zero if updating the PID marker fails so legacy installs don’t proceed with a partially patched source.

  if [ "${#pid}" -ge 3 ]; then
    echo "${pid::-3}000" > "$setup" || :
  fi

src/mido.sh:178

• jq parse failures are being converted into an empty skuId, which can misclassify malformed/blocked API responses as a normal “missing language SKU” case (especially for non-English). It’s better to treat jq failures as a hard error and return non-zero with the raw response logged.

This issue also appears on line 220 of the same file.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

src/answer.sh:2856

• disableAutoReboot now ignores failures from sed/unix2dos and also treats grep errors (rc=2) as success. This can report success even when the file couldn’t be modified, despite callers relying on a non-zero return to abort legacy image preparation.

  grep -Eqi "${pattern}[[:space:]]*[^,;[:space:]]+" "$file" || rc=$?

  case "$rc" in
    0 )
      sed -i -E "s|(${pattern})[[:space:]]*[^,;[:space:]]+|\\1 0|I" "$file" || :

src/answer.sh:2488

• setLegacyKey now ignores failures when writing setupp.ini (|| :). If the file is read-only or the write fails, the function still returns success, but subsequent installation steps may rely on the PID change having been applied. Propagate write failures (or at least return non-zero) so callers can abort consistently.

This issue also appears on line 2569 of the same file.

  if [[ "$driver" == "2k" ]]; then
    [ "${#pid}" -ge 3 ] || return 0
    echo "${pid::-3}270" > "$setup" || :

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.

Suppressed comments (5)

src/mido.sh:220

• Like the SKU parsing above, jq failures are currently coerced to an empty link, which collapses parse/format errors into the “no download link” path and hides the real failure mode.

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

src/answer.sh:2571

• setLegacyKey also ignores failures when writing the updated PID (echo ... || :). If the file is not writable, the function still returns success and the installation proceeds with inconsistent state.

  esac

  if [ "${#pid}" -ge 3 ]; then

src/mido.sh:178

• This change turns a jq parse failure into an empty skuId (and stderr is suppressed), which can misreport malformed/blocked API responses as “no download in language …”. Parse failures should be treated as an error distinct from a legitimate empty result.

This issue also appears on line 220 of the same file.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

src/answer.sh:2489

• setLegacyKey ignores failures when updating setupp.ini (echo ... || :). Since the caller (legacyInstall) treats setLegacyKey failures as fatal, silently continuing here can lead to later failures (or incorrect behavior) without the true cause.

This issue also appears on line 2569 of the same file.

  if [[ "$driver" == "2k" ]]; then
    [ "${#pid}" -ge 3 ] || return 0

src/answer.sh:2856

• disableAutoReboot treats grep read errors (rc=2) as success (falls through the case) and also suppresses failures from sed/unix2dos (|| :). Since callers rely on this returning non-zero on failure, this can silently skip disabling AutoReboot or ignore I/O errors.

  # Keep setup crashes visible instead of immediately rebooting into an
  # opaque installation loop.
  grep -Eqi "${pattern}[[:space:]]*[^,;[:space:]]+" "$file" || rc=$?

  case "$rc" in

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (4)

src/mido.sh:1125

• This checksum mismatch makes verifyFile fail (return 1), but it is currently logged as a warning and the message has awkward capitalization/spacing ("the... $hash ,"). Using error here will align severity with the failure and improve readability.

  warn "the downloaded file has an unknown $algo checksum: $hash , as the expected value was: $check. Please report this at $SUPPORT/issues"

src/mido.sh:220

• jq failures while parsing the download-link response are being swallowed (|| link=""), which makes JSON parse/connector failures indistinguishable from a legitimate “missing link” response. Consider failing explicitly on parse errors and logging the response.

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

src/mido.sh:178

• jq failures while parsing the SKU response are being swallowed (|| skuId=""), which can misreport malformed/blocked API responses as a normal “no download” condition. It would be more accurate to treat parse failures as an error and include the response for debugging.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

src/answer.sh:2858

• disableAutoReboot currently ignores grep errors (rc > 1) and also ignores failures from sed/unix2dos, but still returns success. This can silently leave AutoReboot enabled when the file can’t be inspected or updated.

  grep -Eqi "${pattern}[[:space:]]*[^,;[:space:]]+" "$file" || rc=$?

  case "$rc" in
    0 )
      sed -i -E "s|(${pattern})[[:space:]]*[^,;[:space:]]+|\\1 0|I" "$file" || :

[Copilot]

Pull request overview

Copilot reviewed 4 out of 4 changed files in this pull request and generated no new comments.

Suppressed comments (5)

src/mido.sh:224

• jq failures are swallowed (|| link=""), which can misreport a parsing/dependency problem as "no download link". Consider treating parse failures as an error and returning non-zero (with the response for debugging).

  link=$(printf '%s\n' "$linkJson" | jq --argjson TYPE "$type" -r 'first(.ProductDownloadOptions[]? | select(.DownloadType == $TYPE) | .Uri) // empty') 2>/dev/null || link=""

  if [ -z "$link" ] || [[ "${link,,}" == "null" ]]; then
    error "Microsoft server provided us no download link to our request for an automated download!"
    info "Response: $linkJson"

src/answer.sh:2573

• Write failures to setupp.ini are ignored here (|| :), but the function still returns 0. If this update is required for legacy installs, failures should be surfaced to the caller.

  if [ "${#pid}" -ge 3 ]; then
    echo "${pid::-3}000" > "$setup" || :
  fi

src/mido.sh:178

• jq parsing errors are being swallowed (|| skuId=""), which can turn real failures (e.g., malformed JSON or missing jq) into the misleading "no SKU ID" path. It would be better to treat parse failures as an error and return non-zero with context.

This issue also appears on line 220 of the same file.

  skuId=$(printf '%s\n' "$skuJson" | jq --arg LANG "$language" -r 'first(.Skus[]? | select(.Language == $LANG) | .Id) // empty') 2>/dev/null || skuId=""

  if [ -z "$skuId" ] || [[ "${skuId,,}" == "null" ]]; then
    if [[ "${lang,,}" != "en" && "${lang,,}" != "en-"* ]]; then
      language=$(getLanguage "$lang" "desc")

src/answer.sh:2864

• This function now ignores failures from sed and unix2dos (both are followed by || :) and also ignores grep errors (exit >1). That can silently leave AutoReboot enabled while still returning success.

  grep -Eqi "${pattern}[[:space:]]*[^,;[:space:]]+" "$file" || rc=$?

  case "$rc" in
    0 )
      sed -i -E "s|(${pattern})[[:space:]]*[^,;[:space:]]+|\\1 0|I" "$file" || :
      ;;
    1 )
      printf '%s\n' \
        'HKLM,"SYSTEM\CurrentControlSet\Control\CrashControl","AutoReboot",0x00010001,0' |
        unix2dos >> "$file" || :
      ;;

src/answer.sh:2490

• Write failures to setupp.ini are currently ignored (|| :), but setLegacyKey still returns success. If the write fails, later steps may assume the PID was updated when it wasn’t.

This issue also appears on line 2571 of the same file.

  if [[ "$driver" == "2k" ]]; then
    [ "${#pid}" -ge 3 ] || return 0
    echo "${pid::-3}270" > "$setup" || :
    return 0