Make NodePilot installable from outside, and cut 1.0.1 - #103
Merged
Conversation
The service spawns PowerShell child processes and runs generated scripts out of %TEMP%; without agreed exceptions, endpoint protection blocks individual steps or the install-directory swap during an upgrade. Neither the deployment guide nor the operator README said so. Adds a hand-off document for a security team - folders, processes, temp-file patterns and behaviour rules, each with its rationale and residual risk, split by role (server / desktop / shared PowerShell execution). Deliberately product-neutral: no Add-MpPreference lines, the implementation stays with the AV department. Linked from the prerequisite lists that a reader hits before installing, plus the docs site under Deployment.
Building a release meant running two scripts that did not know about each other: Build-Artifact.ps1 wrote the server zip to out\, Build-DesktopInstaller.ps1 wrote the Electron installer to deploy\desktop\out\, each with its own hand-typed -Version. The drift was already on disk - NodePilot-1.0.0-lab3.zip next to NodePilot-Desktop-Setup-1.0.3.exe, built from the same tree. Build-Artifact.ps1 now takes -IncludeDesktopInstaller (plus -PgBinariesPath and -IsccPath, passed through) and chains the desktop build, so one invocation produces both artifacts under one version in one directory, with a SHA256SUMS file covering everything it produced. The default version comes from Directory.Build.props instead of a timestamp, which is what makes "one version" hold. The desktop build is invoked with -SkipSpaBuild because the SPA was just built; the two dotnet publishes stay separate on purpose, since the server zip is framework-dependent and the desktop payload is not. Missing Inno Setup or Postgres binaries downgrade to a skip with a warning rather than failing the run, and the check happens in pre-flight rather than after a ten-minute publish: a machine without Inno Setup must still be able to build the server artifact. Alongside, three things a stranger hits that are the same class of problem: - launchSettings.json bound port 5068 while every doc and the Vite proxy said 5000, so a bare `dotnet run` produced a backend the frontend never talks to. - appsettings.Development.json carried "SourceCodeRootPath": "e:\NodePilot" - one checkout path published to everyone. It never crashed; the source-knowledge reader just reported itself unavailable, so the feature was quietly dead in every other clone. Machine paths belong in the gitignored appsettings.runtime.json. - LocalLoginMode was BreakGlassOnly in every environment. The bootstrap admin carries that flag so first login works, but any further local user a developer created was rejected at login with no visible cause. Development now sets Enabled; production keeps BreakGlassOnly. An unreachable database was an unhandled provider exception plus stack trace, which reads as a crash rather than as "start Postgres first". MigrationBootstrapper now re-throws DatabaseUnreachableException naming provider, server and database, built from DataSource/Database so the password in the connection string cannot leak into it. The predicate re-probes with CanConnect instead of matching exception types, so it stays provider-agnostic and never swallows a real migration error. global.json and engines fields pin the toolchain. The Node floor was stated three different ways (README 22.22+, CONTRIBUTING 20+, deployment docs "LTS"); react-router 8.3.0 requires >=22.22.0, so that is now declared in the manifests and the docs can stop disagreeing. Two deployment-script fixes found while reading them: the desktop icon generator is launched through powershell.exe rather than dot-called, because it is built on GDI+ and this script's `#requires -Version 5.1` also admits PowerShell 7; and the installer now warns when the host is not domain-joined, where the Domain-profile firewall rules apply to no active profile and the service is reachable locally but not from the network. Tests: contract checks for the release build in Test-DeploymentTemplates.ps1 (mutation-verified), AppSettingsHygieneTests for the two config defects, and an unreachable-database test asserting the message names the target and omits the password.
README, docs/mcp-server.md and the docs site all told operators to install `np` and the MCP server with `dotnet pack` + `dotnet tool install -g`. Running it: error NETSDK1146: PackAsTool does not support specifying TargetPlatformIdentifier Both projects inherit net10.0-windows from Directory.Build.props, and PackAsTool rejects a platform TFM, so `dotnet pack` fails outright for both. docs/roadmap.md already recorded this under "Bewusst nicht auf der Roadmap" - the ruled-out entry was right and the install instructions were wrong, for as long as they have existed. Every surface now documents `dotnet publish` plus a PATH entry, which is verified working: np.exe answers --version, nodepilot-mcp.exe is produced, and the .mcp.json examples point at the executable instead of a tool command that can never resolve. The dead PackAsTool / ToolCommandName / PackageId properties are removed from both csproj files with a comment naming the reason. They are what made the wrong instructions look correct; leaving them in place invites the same doc drift back. AssemblyName stays, so the produced executables keep their names. Also drops the pgAdmin4 filtering item from the roadmap: it was fixed in 103d491, which is why Build-DesktopInstaller.ps1 copies only bin, lib and share.
The documentation site presents the three supported shapes as one comparison table. The README, which is what a stranger reads first, never did: "Quick Start", "Desktop app" and "Production Deployment" sat as three unrelated sections, and building from source was never named as one of three options at all. Someone arriving here could not tell which route was theirs, and each route had at least one instruction that does not work. The README now opens with a three-column table - desktop app / Windows service / from source - followed by one complete section per path. Along the way, the instructions that were wrong: - Both production examples omitted a [Parameter(Mandatory)]. Build-Artifact.ps1 without -SigningCertificateThumbprint stalls on a prompt, and Install-NodePilot.ps1 without -TrustedArtifactSignerThumbprint cannot run at all. The signing requirement is now stated rather than implied. - The setup token was described as landing "in the working directory". It lands in the content root, which with the README's own `dotnet run --project` invocation is a different folder. Readers were looking in the wrong place. - The source path told you to write a password into the tracked appsettings.Development.json, and dropped SSL Mode=Disable while doing so. It now uses an environment variable, and says outright that neither shipped connection string carries a password - previously the step that actually blocks a first run was missing. - There was no CREATE ROLE / CREATE DATABASE step anywhere in README or CONTRIBUTING, and both pointed at C:\NodePilot-Postgres - a cluster that exists on one machine and that nothing in this repository creates. - The Grafana quickstart could not work as printed: compose refuses to start without a .env, and "admin / admin by default" was the only default-credential claim in the repo and was false. The Prometheus exporter needs three environment variables, not two; without the third the dashboards stay empty. - Quick Start said "SQL Server 2022" where the installer enforces CU1+, and "Windows Server 2019+" where the supported matrix is 2022/2025. - The desktop section called the app "a single signed .exe" and "ships as", with no download anywhere in the repository and deploy/desktop/README.md stating plainly that the installer is unsigned. It now points at the release assets, and the self-build path carries its real prerequisites (Inno Setup 6, the EDB Postgres binaries) and its real signing status. - CONTRIBUTING and CLAUDE.md both claimed the first login creates the Admin account without mentioning the setup token, so following either one alone stranded you at the login screen. Smaller corrections: deploy/README.md listed a "Default" for a mandatory parameter with no default, which read as a built-in pinned publisher; the deployment guide pointed at deploy/README.md for the desktop installer, which does not document it; the desktop README showed port 47000 as fixed where the provisioner picks the first free port from 47000 up; and Sync-DesktopApp.ps1's -Component list was missing `shell`. src/nodepilot-desktop/ had no README even though deploy/desktop/README.md sends contributors there, and the project was missing from the README's structure listing. Both fixed; the new file's claims are checked against main.ts, config.ts and skins.ts.
The product version was declared independently in Directory.Build.props, both client csproj files, three package.json files, and as a literal in the CLI's Program.cs. They had already diverged: `np --version` answered 1.0.0 while the assembly it was compiled into carried a different number, and nothing would have caught it. Directory.Build.props is now the only declaration for the .NET side - the redundant <Version> elements are gone from NodePilot.Cli and NodePilot.Mcp, which inherit it - and the CLI reads its version off the assembly instead of a string, with the SDK's "+<commit>" source-revision suffix trimmed. A test binds `np --version` back to Directory.Build.props so the two cannot drift again. Bumped to 1.0.1 across Directory.Build.props and the three package.json files, with the lockfiles regenerated so `npm ci` stays in sync - both the version and the engines field added earlier had to land there or the release build and CI would fail on an out-of-sync lockfile.
Build-DesktopInstaller.ps1 defaulted -IsccPath to the machine-wide Program Files (x86) location. Inno Setup installs there only when its own installer runs elevated; a normal double-click install lands in %LOCALAPPDATA%\Programs. On a machine with a working per-user installation the build therefore refused to start, and after the pre-flight added to Build-Artifact.ps1 it would have quietly skipped the desktop installer with a message telling you to install software you already had. Both scripts now resolve the compiler through one shared probe list (deploy/desktop/Resolve-IsccPath.ps1) so the pre-flight and the build cannot disagree about where it lives. An explicitly passed -IsccPath is still used verbatim and is not silently replaced when it is wrong.
Every deployment surface assumed you build the artifact yourself, because until now there was nothing to download. With assets attached to the release, the server path has two entry points and they need different instructions: a self-built artifact is signed with your own certificate, a downloaded one is signed with the project's, and the installer demands that you name the publisher you trust either way. docs/deployment-guide.md step 1 is now split into download and build, including how to verify the checksums and the publisher thumbprint before importing anything into LocalMachine\Root - importing a code-signing certificate there trusts that publisher for the whole machine, which is not a step to hand someone without the verification around it. deploy/README.md and the documentation site carry the same split. The desktop page said a signed release artifact 'must be produced by the respective release process' - a process that did not exist anywhere in this repository. It now points at the release assets and is explicit that a self-built installer is unsigned. Also drops the hard-coded -Version from the build examples: it defaults to the product version now, and repeating a literal in six documents is how the examples drifted to four different values in the first place.
Signing rewrites the .exe. The build wrote SHA256SUMS.txt and then told the operator to run signtool - so following the printed instructions produced a checksum file declaring the shipped installer corrupt. I hit this by doing exactly what the script said. -DesktopSigningCertificateThumbprint now Authenticode-signs the installer between producing it and hashing it, verifies afterwards that the signature really carries the requested certificate rather than trusting signtool's exit code, and the closing message only tells you the installer is unsigned when it actually is. A contract check pins the ordering. It anchors on the signtool invocation and the checksum computation rather than on the words 'Authenticode-sign', which also appear in the parameter help at the top of the file - anchoring on the prose made the check pass regardless of where the signing step sat, which a mutation run exposed.
Same defect the root README had: the README claimed anonymous Viewer access plus admin/admin, while docker-compose.yml declares GF_SECURITY_ADMIN_PASSWORD and MSSQL_SA_PASSWORD as required and aborts without a .env, and anonymous access defaults to off. The .env.example that carries both was never mentioned.
Picked out of the open docs-sync PR (#93), which also carried two regressions that are deliberately left behind: it doubled the backslashes in the credential curl example (CONTOSO\svc-winrm renders as two literal backslashes, not one), and it stripped the trailing newline from the file. Only the endpoint addition is correct - GET /api/admin/settings/effective-sizing exists in AdminSettingsController.
A 52-node manual-trigger workflow that exercises every activity with a remote code path - runScript, folder/file/textFileEdit, fileHash, zip, registry, wmiQuery, service, startProgram, scheduledTask, waitForCondition, powerManagement - across two target machines, joined at a junction and closed with returnData. Unlike every other workflow under scripts/, this one carries real targetMachineId GUIDs instead of "localhost". That is deliberate and unavoidable: localhost executes in-process and never touches WinRM, so a localhost variant would test the opposite of what this file is for. Anyone importing it has to repoint both machines first.
![github-advanced-security[bot]](https://avatars.github.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+63
to
+66
| catch | ||
| { | ||
| return true; | ||
| } |
Comment on lines
+84
to
+88
| catch | ||
| { | ||
| // A malformed connection string cannot be projected. The generic message below is | ||
| // still far better than the raw provider exception. | ||
| } |
| [Fact] | ||
| public void TrackedAppSettings_DoNotCarryMachineSpecificPaths() | ||
| { | ||
| var apiDirectory = Path.Combine(FindRepoRoot(), "src", "NodePilot.Api"); |
| var repoRoot = FindRepoRoot(); | ||
|
|
||
| var launchSettings = JsonDocument.Parse(File.ReadAllText( | ||
| Path.Combine(repoRoot, "src", "NodePilot.Api", "Properties", "launchSettings.json"))); |
| var launchPort = LocalhostPort.Match(applicationUrl ?? string.Empty); | ||
| launchPort.Success.Should().BeTrue($"launchSettings applicationUrl '{applicationUrl}' should be an http://localhost:<port> URL"); | ||
|
|
||
| var viteConfig = File.ReadAllText(Path.Combine(repoRoot, "src", "nodepilot-ui", "vite.config.ts")); |
| private static string FindRepoRoot() | ||
| { | ||
| var directory = new DirectoryInfo(AppContext.BaseDirectory); | ||
| while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "NodePilot.slnx"))) |
| [Fact] | ||
| public void Current_MatchesTheProductVersionInDirectoryBuildProps() | ||
| { | ||
| var props = File.ReadAllText(Path.Combine(FindRepoRoot(), "Directory.Build.props")); |
| private static string FindRepoRoot() | ||
| { | ||
| var directory = new DirectoryInfo(AppContext.BaseDirectory); | ||
| while (directory is not null && !File.Exists(Path.Combine(directory.FullName, "NodePilot.slnx"))) |
This was referenced Aug 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nobody outside this repository can install NodePilot today. This branch fixes the three reasons why, and cuts 1.0.1.
Why
v1.0.0release has zero assets, there is no release workflow, and no script in the repo ever uploads anything. Both product install paths start with "build it yourself".Build-Artifact.ps1writes the server zip toout/,Build-DesktopInstaller.ps1writes the installer todeploy/desktop/out/, each with its own hand-typed-Version. They had already drifted:NodePilot-1.0.0-lab3.zipsat next toNodePilot-Desktop-Setup-1.0.3.exe, built from the same tree.CONTRIBUTING.md,CLAUDE.md,deploy/README.md,docs/deployment-guide.md,deploy/desktop/README.mdand the docs site there were 26 documented inconsistencies — including two production commands missing a[Parameter(Mandatory)], a wrong path for the setup token, three different Node floors, and a Grafana line inventing default credentials.Packaging
Build-Artifact.ps1gains-IncludeDesktopInstaller(plus-PgBinariesPath,-IsccPath,-DesktopSigningCertificateThumbprint) and chains the desktop build, so one command produces both shipping targets under one version in one directory, with aSHA256SUMS.txtover everything it produced. Missing Inno Setup or Postgres binaries downgrade to a skip with a warning — a machine without Inno Setup must still be able to build the server artifact — and that is decided in pre-flight rather than after a ten-minute publish.Two things found by actually running it:
signtool. Signing rewrites the.exe, so following the printed instructions produced a checksum file declaring the shipped installer corrupt. A contract check now pins the ordering.Program Files (x86), but a non-elevated Inno Setup install lands in%LOCALAPPDATA%\Programs. On a machine with a working installation the build refused to start. Both scripts now share one probe list.The product version had five independent declarations and had already diverged —
np --versionanswered1.0.0regardless.Directory.Build.propsis now the only source; the CLI reads it off the assembly, and a test binds the two together.Onboarding
The README now opens with a three-column table (desktop app / Windows service / from source) followed by one complete section per path. Each path's instructions were verified against the scripts they invoke. Fixed along the way:
dotnet runbinds port 5000.launchSettings.jsonsaid 5068 while every doc and the Vite proxy assumed 5000, so a plaindotnet runproduced a backend the frontend never talked to.DataSource/Databaseso the connection password cannot leak into it.appsettings.Development.jsonno longer ships a developer's checkout path ("SourceCodeRootPath": "e:\\NodePilot"). It never crashed; the source-knowledge reader just reported itself unavailable, so the feature was quietly dead in every other clone.BreakGlassOnlyin every environment silently rejected every additional local user a developer created.global.jsonandenginesfields pin the toolchain, which settles the Node floor at its real value (react-router8.3.0 requires>=22.22.0) instead of restating it differently in four documents.dotnet tool install -gnever worked for the CLI and the MCP server.PackAsToolrejects thenet10.0-windowstarget both projects inherit, sodotnet packfails with NETSDK1146 — whichdocs/roadmap.mdalready recorded as ruled-out while the README and docs site kept documenting it as the install path. Both now usedotnet publishplus aPATHentry, verified working, and the dead csproj properties that made the wrong instructions look plausible are removed.Release
docs/deployment-guide.mdstep 1 is split into download and build, including verifying checksums and the publisher thumbprint before importing a certificate intoLocalMachine\Root.Version bumped to 1.0.1. Artifacts are built and signed; the tag and release follow after this merge.
Tests
AppSettingsHygieneTests(no machine paths in tracked config, dev port matches the Vite proxy),CliVersionTests, an unreachable-database test, and release-build contracts inTest-DeploymentTemplates.ps1— all mutation-verified.Notes for review
effective-sizingendpoint row). The rest of that PR doubled the backslashes in a credential example and stripped a trailing newline, so docs: sync after #88 (add effective-sizing to docs-site API table) #93 can be closed.scripts/test-winrm-remote-sweep.jsonis the one workflow underscripts/carrying real machine GUIDs instead oflocalhost, because localhost runs in-process and never exercises WinRM.deploy/desktop/README.md.