Exe download priority, after migration users validation, before logoff#270
Conversation
📝 WalkthroughWalkthroughThe script reorders local executable validation to occur after determining migration users, adding an early exit when no eligible users exist. Executable pre-check and GUI path retrieval move into a new exeValidation section, the logoffUsers region marker is relocated, and the migration section reuses the previously computed path instead of re-fetching it. ChangesExecution Order Rework in ADMU Invoke Script
Estimated code review effort: 2 (Simple) | ~10 minutes Sequence Diagram(s)sequenceDiagram
participant Script as 3_ADMU_Invoke.ps1
participant MigrationLookup as Get-MigrationUser
participant ExeValidation as exeValidation
participant GuiExe as Get-LatestADMUGUIExe
participant Migration as Invoke-UserMigrationBatch
Script->>MigrationLookup: request UsersToMigrate
MigrationLookup-->>Script: return UsersToMigrate
alt UsersToMigrate not present
Script->>Script: exit
else UsersToMigrate present
Script->>ExeValidation: run local executable pre-check
ExeValidation->>GuiExe: retrieve latest gui_jcadmu.exe
GuiExe-->>ExeValidation: guiJcadmuPath
Script->>Migration: invoke migration using guiJcadmuPath
end
Related issues: None referenced in the provided context. Related PRs: None referenced in the provided context. Suggested labels: None Suggested reviewers: None 🐰 A rabbit hopped the script's course, 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@jumpcloud-ADMU-Advanced-Deployment/InvokeFromJCAgent/3_ADMU_Invoke.ps1`:
- Around line 706-719: The early-exit in the $UsersToMigrate check currently
treats a benign “no users to migrate” state as a failure. Update the exit path
near the initial data import logic so the no-op case returns a success code
instead of failing the JumpCloud run, and keep exit 1 only for real errors in
the surrounding migration flow. Use the $UsersToMigrate check and the existing
exeValidation block as anchors when making the change.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 9c5035e4-54f2-4763-89d5-5f023706feb1
📒 Files selected for processing (1)
jumpcloud-ADMU-Advanced-Deployment/InvokeFromJCAgent/3_ADMU_Invoke.ps1
| if (-not $UsersToMigrate) { Write-Host "[status] No users to migrate, exiting..."; exit 1 } | ||
| #endregion dataImport | ||
|
|
||
| #region exeValidation | ||
| try { | ||
| $localExeValidation = Test-RequiredLocalExeFiles -localEXEs $localEXEs | ||
| if ($localExeValidation) { Write-Host "[STATUS] Local executable pre-check completed successfully." } | ||
| } catch { | ||
| Write-Host "[ERROR] Local executable validation failed: $_" | ||
| exit 1 | ||
| } | ||
| $guiJcadmuPath = Get-LatestADMUGUIExe -useLocalEXEs $localEXEs -BypassValidation $bypassExeValidation | ||
| #endregion exeValidation | ||
|
|
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Early-exit uses failure exit code for a benign "no users" state.
The new early exit at Line 706 calls exit 1 when $UsersToMigrate is empty. For the Description data source, Get-MgUserFromDesc returns $null simply when there are no pending users to migrate (e.g., all users already migrated, or a routine check-in) — this is not necessarily an error condition. Exiting with code 1 will likely be recorded as a failed JumpCloud command run, which can generate false-positive failure alerts for otherwise-healthy systems.
This also appears to be genuinely new behavior (per the AI summary), which conflicts with the PR description's statement that no new behavior beyond reordering is expected.
Consider using exit 0 for this no-op case, or reserve exit 1 for genuine failures.
💡 Proposed fix
-if (-not $UsersToMigrate) { Write-Host "[status] No users to migrate, exiting..."; exit 1 }
+if (-not $UsersToMigrate) { Write-Host "[status] No users to migrate, exiting..."; exit 0 }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (-not $UsersToMigrate) { Write-Host "[status] No users to migrate, exiting..."; exit 1 } | |
| #endregion dataImport | |
| #region exeValidation | |
| try { | |
| $localExeValidation = Test-RequiredLocalExeFiles -localEXEs $localEXEs | |
| if ($localExeValidation) { Write-Host "[STATUS] Local executable pre-check completed successfully." } | |
| } catch { | |
| Write-Host "[ERROR] Local executable validation failed: $_" | |
| exit 1 | |
| } | |
| $guiJcadmuPath = Get-LatestADMUGUIExe -useLocalEXEs $localEXEs -BypassValidation $bypassExeValidation | |
| #endregion exeValidation | |
| if (-not $UsersToMigrate) { Write-Host "[status] No users to migrate, exiting..."; exit 0 } | |
| `#endregion` dataImport | |
| `#region` exeValidation | |
| try { | |
| $localExeValidation = Test-RequiredLocalExeFiles -localEXEs $localEXEs | |
| if ($localExeValidation) { Write-Host "[STATUS] Local executable pre-check completed successfully." } | |
| } catch { | |
| Write-Host "[ERROR] Local executable validation failed: $_" | |
| exit 1 | |
| } | |
| $guiJcadmuPath = Get-LatestADMUGUIExe -useLocalEXEs $localEXEs -BypassValidation $bypassExeValidation | |
| `#endregion` exeValidation |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@jumpcloud-ADMU-Advanced-Deployment/InvokeFromJCAgent/3_ADMU_Invoke.ps1`
around lines 706 - 719, The early-exit in the $UsersToMigrate check currently
treats a benign “no users to migrate” state as a failure. Update the exit path
near the initial data import logic so the no-op case returns a success code
instead of failing the JumpCloud run, and keep exit 1 only for real errors in
the surrounding migration flow. Use the $UsersToMigrate check and the existing
exeValidation block as anchors when making the change.
There was a problem hiding this comment.
This is correct, if there are no users to migrate, we do want to throw an exit 1 to indicate something happened that we didn't expect. In this context an exit code 1 is good enough to denote there was an error with data prep. If no users are able to migrated I want an exit code 1.
An alternative would be to define a table of new exit codes:
1 error
2 no users to migrate
3 data validation error etc
The team can take this back and work as a separate card but for this change it's fine to keep it as is.
There was a problem hiding this comment.
@jworkmanjc, understood — thanks for the clarification. Keeping exit 1 for the "no users to migrate" case makes sense given the current convention of using exit 1 to denote any unexpected/error condition in this script. The idea of a structured exit code table (e.g., 1=error, 2=no users, 3=data validation) is a nice enhancement for more granular alerting/triage, but agreed that's reasonable to defer as a separate follow-up card rather than bundling it into this reorder-focused PR.
Let me know if you'd like me to open a follow-up issue to track the structured exit code idea.
✏️ Learnings added
Learnt from: jworkmanjc
Repo: TheJumpCloud/jumpcloud-ADMU PR: 270
File: jumpcloud-ADMU-Advanced-Deployment/InvokeFromJCAgent/3_ADMU_Invoke.ps1:706-719
Timestamp: 2026-07-01T17:19:18.338Z
Learning: In `jumpcloud-ADMU-Advanced-Deployment/InvokeFromJCAgent/3_ADMU_Invoke.ps1` (JumpCloud ADMU Invoke Script), when `$UsersToMigrate` is empty/null after the data import step, the script intentionally exits with `exit 1` (not `exit 0`), even though this can represent a benign "no pending users" state (e.g., via `Get-MgUserFromDesc`). This is a deliberate design choice by the team (per maintainer jworkmanjc): exit code 1 is used broadly to denote "something happened that we didn't expect" including data prep issues, not just hard failures. A future enhancement being considered (not yet planned) is a structured exit code table (e.g., 1=general error, 2=no users to migrate, 3=data validation error) to allow finer-grained alerting, but this exit-code granularity work is out of scope for reorder-focused PRs and would be tracked as a separate card.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
3 participants
Update Invoke Script EXE download priority order.
Issues
What does this solve?
This minimal change re-structures the order in which the EXE is downloaded. The goal here is to cache all required assets before migration is invoked and before the logoff task occurs. The ADMU, if prompted to via parameter, will block accounts form logging in during migration. This action should occur as soon as possible after logoff.
Previously the migration user was logged off, then the production exe downloaded to the device. This change downloads the EXE before logoff to ensure account login blocking is triggered as soon as possible.
Is there anything particularly tricky?
NO
How should this be tested?
Run the new invoke script. There should be not new behavior here, just order of operations has changed.
Screenshots
Note
Low Risk
Same operations in a different order with no new migration parameters or logic; failure modes are unchanged aside from failing before logoff if EXE download fails.
Overview
Reorders the ADMU invoke script so
gui_jcadmu.exeis validated and obtained (local check +Get-LatestADMUGUIExe) after migration users are loaded and beforequser/logoff and the migration batch.Local EXE pre-check moves out of the parameter validation block into a dedicated
#region exeValidation;Get-LatestADMUGUIExeis no longer called at the start of#region migration(path is still passed viaguiJcadmuPath). Region comments fordataImportandlogoffUsersare cleaned up.Intent: when
BlockAccountLoginis enabled, the production binary is already on disk right after logoff instead of downloading while users are being logged off.Reviewed by Cursor Bugbot for commit 6f522eb. Bugbot is set up for automated code reviews on this repo. Configure here.
Summary by CodeRabbit