Retry Windows VM RDP credentials

[qualvey]

Summary

When saved RDP credentials are rejected, prompt for the Windows username and password again, save each newly entered pair atomically, and retry until the user connects successfully or cancels.

The VM remains running between attempts. Cancelling or leaving a field empty follows the existing cleanup path.

Testing

• bash test/shell.d/windows-vm-test.sh
• bash test/shell.d/windows-vm-compose-test.sh

Follow-up to #14088.

[greptile-apps]

Confidence Score: 4/5

[Medium risk] Adds credential retry logic to Windows VM RDP connection.

Fix the undefined variable in windows-vm-test.sh before merging so the shell test suite can pass.

Findings

1.  Undefined variable breaks shell tests ▶
2.  Retry behavior goes untested ▶

Summary

Adds credential prompts and retries after selected FreeRDP login failures. Newly entered credentials are saved atomically, and the VM stays running between attempts.

• Adds tests for retrying saved credentials and closing RDP without another prompt.
• Updates directory creation in the compose test.
• The new opacity check breaks the shell test because windows_vm_rules is undefined.

Diagram

%%{init: {'theme': 'neutral'}}%%
flowchart TD
  A[Start VM] --> B[Connect with saved credentials]
  B --> C{Retryable login failure?}
  C -->|Yes| D[Prompt for username and password]
  D -->|Credentials entered| E[Save credentials atomically]
  E --> B
  D -->|Cancel or empty field| F[Existing cleanup path]
  C -->|No| F
  F --> G{Keep alive?}
  G -->|Yes| H[Leave VM running]
  G -->|No| I[Stop VM]

Loading

_{Reviews (5) · Last reviewed commit: "Prepare Windows VM test mount directorie..."}

[greptile-apps]

Retry behavior goes untested

These checks only search for strings; they never run the retry loop. Changing the success check to ((RDP_STATUS != 0)) && break would still pass all three checks while breaking retries. Add a test with stubbed xfreerdp3 and gum calls that checks failure followed by success, saved credentials, and cancellation reaching cleanup.

[camdencarr84-jpg]

You fixed it? Thank you so much!

[camdencarr84-jpg]

I set my agent on adding this PR.

[camdencarr84-jpg]

To success, my VM is fixed, thanks @qualvey

[omarchybot]

Following the merge of #12323, The setgid hardening portion is covered by the merged fix. Re-entering and saving rejected RDP credentials is separate behavior and remains unmerged. Keeping this PR open for that remainder; remove the superseded chmod change when rebasing. This is a scope disposition, not a claim that the retry implementation is verified or ready.

Scope checked by GPT-6 in Codex and Codex Medium against the discussion and landed change; review independence is not guaranteed. No tests were rerun for this cleanup.

Omabot on behalf of DHH

[jandrusk]

Thanks for picking this up! One blocking issue with the retry loop:

It re-prompts on any non-zero xfreerdp3 exit, not just credential failures. FreeRDP 3 returns non-zero for plenty of ordinary endings: 1 disconnect, 2 logoff, 11 disconnect by user, 131 connection failed, 141/147 transport failures, and so on. So logging off Windows from the Start menu, or the VM still booting, now asks for the username and password again and overwrites the saved credentials with whatever gets typed. Only the auth and logon codes should loop:

    printf '%s\n' "${RDP_ARGS[@]}" | xfreerdp3 /args-from:stdin
    RDP_STATUS=$?
    case $RDP_STATUS in
      132 | 134 | 154 | 159) ;;  # AUTH_FAILURE, LOGON_FAILURE, WRONG_PASSWORD, NO_OR_MISSING_CREDENTIALS
      *) break ;;
    esac

Also:

• As @omarchybot noted, the chmod 00700 hunk is already covered by Clear setgid when hardening Windows VM directories #12323. Please drop it (and the related compose-test changes, including the removed prepare_user_mount_sources call) when you rebase.
• The new test only greps for strings. A stubbed xfreerdp3 that exits 132 once, then 0, plus a stubbed gum, would actually exercise the loop, including a check that exit 2 does not re-prompt.

[greptile-apps]

Undefined variable breaks shell tests

windows_vm_rules is never defined in this test or its shared helper. With set -u, the first new opacity check exits with an unbound-variable error. This breaks windows-vm-test.sh and the shell test suite. Define the rule-file path before these checks.

[jandrusk]

Thanks, the retry loop now only re-prompts on the auth/logon exit codes, which fixes my blocking point. A few things are still open:

• The prepare_user_mount_sources / mount-source hunks are still in the diff. They overlap Clear setgid when hardening Windows VM directories #12323, and Greptile's P1 notes fresh installs can fail with "Failed to write the Windows VM configuration." Could you drop them and leave that to Clear setgid when hardening Windows VM directories #12323?
• test/shell.d/windows-vm-test.sh uses $windows_vm_rules, but it's never defined, so the test exits under set -u.
• The gum call counter in the retry test runs in a subshell, so it can't catch an unexpected prompt. Recording calls to a file would fix that.

[jandrusk]

Thanks, the retry loop now only re-prompts on 132/134/154/159, and the new test counts prompts through a file, so it would catch an ordinary exit that re-prompts. That part looks good.

The mount-source changes are still in the PR, though, and they've become a regression since #12323 merged (10-04):

• install_windows and migrate_legacy_compose no longer call prepare_user_mount_sources. That function is where Clear setgid when hardening Windows VM directories #12323 clears setgid (chmod u=rwx,go=,a-s) and where the ownership and "storage and shared must be different directories" checks run. The install -d -m 0700 added to write_compose doesn't replace those checks. GNU install/chmod with a numeric mode also keeps an existing setgid bit on a directory, so the omarchy-windows-vm launch fails silently after the first run — setgid on the shared mount breaks an exact == 700 mode check #14196 / omarchy windows vm: shared-folder setgid blocks relaunch after first run; KVM boot crash on Meteor Lake #11879 case comes back.
• test/shell.d/windows-vm-test.sh now reads $windows_vm_rules, which is never defined. With set -u, the suite stops at the first opacity check. Those opacity assertions look unrelated to RDP retries anyway.

Could you rebase onto current quattro and keep only the RDP retry loop and windows-vm-rdp-retry-test.sh? That means dropping the prepare_user_mount_sources removals, the write_compose install -d, the compose-test edits and the opacity checks. Then this is a small, easy-to-review change.

[omarchybot]

Reviewed at cc739b5. Classification is unsettled, so nothing was run on a worker yet. I read it as a bug fix: saved credentials that Windows rejects currently leave no way to connect. Codex Medium, classifying from the pull request alone, read it as an enhancement: a new prompt-and-retry flow. Both readings are reasonable, and which one applies decides how it gets verified, so the maintainer settles it.

From reading the diff against current quattro, the two points @jandrusk raised on 10-07 still hold at this head:

• bin/omarchy-windows-vm drops the prepare_user_mount_sources calls from install_windows and migrate_legacy_compose. On quattro that function runs the mount-source preflight, refuses ~/.windows and ~/Windows when they are the same directory, and clears the setgid bit with chmod u=rwx,go=,a-s. The install -d -m 0700 added to write_compose does none of that, so this reverts part of Clear setgid when hardening Windows VM directories #12323.
• test/shell.d/windows-vm-test.sh greps "$windows_vm_rules", which is never defined, so under set -u the test stops at the first opacity check. Those opacity assertions are unrelated to RDP retries.

The retry loop itself (re-prompting only on exits 132/134/154/159) and test/shell.d/windows-vm-rdp-retry-test.sh are the part this pull request is about. Rebased onto quattro with only those two kept, it becomes a small change.

Waiting on the maintainer for the classification and on @qualvey for the rebase.