fix(installer): port install.sh fixes to install.ps1 (Windows parity) - #13
Merged
Conversation
Brings install.ps1 into parity with the merged install.sh after the DEV-1417 install-fixes series. Previously Windows users hit four silent papercuts that macOS/Linux users were already protected from. Ported fixes: - ST-dir verification now requires BOTH server.js AND package.json (OR logic, matching install.sh); previously the PS1 check only errored if BOTH were missing, silently installing in the wrong dir when one was present. - BUG-4: generate config.yaml by starting SillyTavern briefly when missing. Uses Start-Process + taskkill /F /T to manage the npm -> node process tree (Windows analogue of install.sh's exec npm + kill $!). - enableServerPlugins idempotent auto-flip in config.yaml via PowerShell -replace with (?m) multiline regex, matching install.sh's sed semantics. Writes with [System.IO.File]::WriteAllText to avoid Windows PowerShell 5.1's UTF-8 BOM injection. - BUG-5: resolvable apiKey probe through the plugin's fallback chain (hosts.sillytavern.apiKey -> root apiKey) using PowerShell's native ConvertFrom-Json + null-propagation — cleaner than install.sh's python3 probe since PS .NET JSON parsing + null chaining needs no external runtime. Additional: - Junction failure message now names both remediations (Administrator PowerShell OR Windows Developer Mode); default Azure Windows Server images ship Developer Mode off, making this distinction relevant for any automated VM-based verification. References merged install.sh commits: 3b09f32 (config.yaml + flip), 6ef2018 (apiKey probe), 32a90d3 (install.sh hardening), 161c3ae (usage comment drift). No new dependencies. Pure port — behavior is a strict superset of the prior install.ps1 (same happy paths, now with the four additional guards). Not yet verified on a real Windows host; see PR body for verification plan.
Drop rationale comments that restate what the code already expresses via named cmdlets, guards, and error messages. Removed: #Requires directive explanation, ST-dir verify preamble, junction-no-admin-needed preamble (same content already in the error message), New-Item-vs-mklink JSDoc, npm install guard rationale, Start-Process redirection novel, HasExited poll rationale, taskkill null-guard rationale, config.yaml bootstrap multi-line block, regex explanation multi-line block, BOM note multi-line block, resolvable-key probe multi-line block. Kept: one-line WHY where the invariant isn't visible in code — the [ \t]* vs \s* .NET asymmetry, UTF-8 BOM behavior on PS 5.1 Set-Content, the config.yaml first-npm-start invariant (BUG-4 crux), the false-promise probe rationale, and the malformed-JSON catch note. Mirrors Eri's cleanup pass on PR #7 (eef27be).
fix(installer): port install.sh fixes to install.ps1 (Windows parity)
WalkthroughThe Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as install.ps1
participant FS as Filesystem
participant NPM as npm.exe
participant ST as SillyTavern (process)
User->>Installer: run install.ps1
Installer->>Installer: check PowerShell >= 5.0
Installer->>FS: verify `server.js` or `package.json` present
Installer->>FS: create junction via New-Item
alt junction fails
Installer->>User: print diagnostics (cross-volume, network, permissions)
Installer-->>User: exit
end
Installer->>NPM: resolve `npm.cmd`
alt npm missing or install fails
Installer->>User: print actionable npm troubleshooting and verbose rerun hint
Installer-->>User: exit on nonzero
end
Installer->>FS: check `config.yaml`
alt config missing
Installer->>ST: start SillyTavern (background)
Installer->>FS: poll for `config.yaml` up to 60s (capture stdout/stderr to temp logs)
alt file appears
Installer->>ST: kill process if needed
else
Installer->>User: show temp logs and exit
end
end
Installer->>FS: rewrite YAML key `enableServerPlugins: true` (UTF-8 no BOM)
Installer->>FS: validate YAML change
alt validation fails
Installer->>User: provide manual YAML edit instructions
end
Installer->>FS: parse `.honcho/config.json`
alt malformed JSON
Installer->>User: error and exit
else if auto-key not resolvable
Installer->>User: prompt to enter API key manually after restart
end
Installer-->>User: finish
Estimated Code Review Effort🎯 4 (Complex) | ⏱️ ~45 minutes Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.ps1`:
- Around line 130-145: The install.ps1 block currently treats a JSON parse error
the same as a valid config without keys; update the try/catch to track parse
success (e.g., set a $parseSucceeded or $malformedJson flag) so $resolvableKey
is only evaluated when ConvertFrom-Json succeeded; in the catch set the
malformed flag and later branch on that flag (instead of just $resolvableKey) to
print a distinct message when $cfg parsing failed for $HONCHO_CONFIG and avoid
implying the config simply lacks an apiKey—refer to the variables
$HONCHO_CONFIG, $cfg, $hostKey, $rootKey, and $resolvableKey when locating and
changing the logic.
- Around line 63-85: The npm install step can run when npm isn't on PATH because
$LASTEXITCODE can be stale; before calling npm (the npm install block that sets
$null = npm install --silent and reads $LASTEXITCODE) run a command availability
check using Get-Command -Name npm.cmd -ErrorAction SilentlyContinue (or npm) and
if it returns $null write a clear error and exit 1; additionally after
attempting the install check the automatic success flag ($?) as a fallback and
treat a false value the same as a non-zero $LASTEXITCODE so you don't proceed to
the Start-Process bootstrap (the Start-Process -FilePath "npm.cmd" ... block)
when npm is missing or failed.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
- Preflight Get-Command npm.cmd before invoking, with $? fallback to catch command-not-found cases that don't set $LASTEXITCODE. - Treat malformed ~/.honcho/config.json as a hard error rather than collapsing it into the missing-key branch (current behavior risks silent settings wipe when the plugin recreates a minimal config). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@install.ps1`:
- Around line 138-154: The probe logic incorrectly treats
hosts.sillytavern.apiKey (hostKey) as sufficient for "resolvable" even though
the plugin runtime first consults the SecretManager and only falls back to the
root apiKey; update the check that sets resolvableKey so it mirrors the runtime
fallback: consider a SecretManager entry as the highest-priority resolvable
source and only mark resolvableKey true if a SecretManager config exists or the
root-level apiKey (rootKey) exists, rather than relying on
hosts.sillytavern.apiKey alone; adjust the conditional that examines
hostKey/rootKey (and any secret manager field) so the Write-Host message about
auto-population for HONCHO_CONFIG accurately reflects the runtime behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+138
to
+154
| $hostKey = $cfg.hosts.sillytavern.apiKey | ||
| $rootKey = $cfg.apiKey | ||
| if ($hostKey -or $rootKey) { $resolvableKey = $true } | ||
| } catch { | ||
| $parseFailed = $true | ||
| } | ||
| if ($parseFailed) { | ||
| Write-Host "[!] Found malformed Honcho config at $HONCHO_CONFIG" | ||
| Write-Host " Fix or remove it before re-running, or the plugin may recreate a minimal config and wipe existing settings." | ||
| exit 1 | ||
| } elseif ($resolvableKey) { | ||
| Write-Host "[*] Found global Honcho config with resolvable apiKey at $HONCHO_CONFIG" | ||
| Write-Host " API key, workspace, and peer name will be auto-populated." | ||
| } else { | ||
| Write-Host "[*] Found $HONCHO_CONFIG but no resolvable apiKey." | ||
| Write-Host " (plugin checks hosts.sillytavern.apiKey, then root apiKey.)" | ||
| Write-Host " Enter your Honcho API key via the Extensions panel after restart." |
There was a problem hiding this comment.
Align apiKey probe with actual plugin fallback chain.
At Line 138 and Line 153, the script treats hosts.sillytavern.apiKey as resolvable. But the provided plugin runtime paths (plugin/index.js:88-110, plugin/index.js:201-230) use SecretManager first, then root-level apiKey. This can report “auto-populated” when runtime auth still fails.
Suggested fix
- $hostKey = $cfg.hosts.sillytavern.apiKey
$rootKey = $cfg.apiKey
- if ($hostKey -or $rootKey) { $resolvableKey = $true }
+ if ($rootKey) { $resolvableKey = $true }
...
- Write-Host " (plugin checks hosts.sillytavern.apiKey, then root apiKey.)"
+ Write-Host " (plugin checks SillyTavern SecretManager first, then root apiKey.)"📝 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.
Suggested change
| $hostKey = $cfg.hosts.sillytavern.apiKey | |
| $rootKey = $cfg.apiKey | |
| if ($hostKey -or $rootKey) { $resolvableKey = $true } | |
| } catch { | |
| $parseFailed = $true | |
| } | |
| if ($parseFailed) { | |
| Write-Host "[!] Found malformed Honcho config at $HONCHO_CONFIG" | |
| Write-Host " Fix or remove it before re-running, or the plugin may recreate a minimal config and wipe existing settings." | |
| exit 1 | |
| } elseif ($resolvableKey) { | |
| Write-Host "[*] Found global Honcho config with resolvable apiKey at $HONCHO_CONFIG" | |
| Write-Host " API key, workspace, and peer name will be auto-populated." | |
| } else { | |
| Write-Host "[*] Found $HONCHO_CONFIG but no resolvable apiKey." | |
| Write-Host " (plugin checks hosts.sillytavern.apiKey, then root apiKey.)" | |
| Write-Host " Enter your Honcho API key via the Extensions panel after restart." | |
| $rootKey = $cfg.apiKey | |
| if ($rootKey) { $resolvableKey = $true } | |
| } catch { | |
| $parseFailed = $true | |
| } | |
| if ($parseFailed) { | |
| Write-Host "[!] Found malformed Honcho config at $HONCHO_CONFIG" | |
| Write-Host " Fix or remove it before re-running, or the plugin may recreate a minimal config and wipe existing settings." | |
| exit 1 | |
| } elseif ($resolvableKey) { | |
| Write-Host "[*] Found global Honcho config with resolvable apiKey at $HONCHO_CONFIG" | |
| Write-Host " API key, workspace, and peer name will be auto-populated." | |
| } else { | |
| Write-Host "[*] Found $HONCHO_CONFIG but no resolvable apiKey." | |
| Write-Host " (plugin checks SillyTavern SecretManager first, then root apiKey.)" | |
| Write-Host " Enter your Honcho API key via the Extensions panel after restart." |
🧰 Tools
🪛 PSScriptAnalyzer (1.25.0)
[warning] Missing BOM encoding for non-ASCII encoded file 'install.ps1'
(PSUseBOMForUnicodeEncodedFile)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@install.ps1` around lines 138 - 154, The probe logic incorrectly treats
hosts.sillytavern.apiKey (hostKey) as sufficient for "resolvable" even though
the plugin runtime first consults the SecretManager and only falls back to the
root apiKey; update the check that sets resolvableKey so it mirrors the runtime
fallback: consider a SecretManager entry as the highest-priority resolvable
source and only mark resolvableKey true if a SecretManager config exists or the
root-level apiKey (rootKey) exists, rather than relying on
hosts.sillytavern.apiKey alone; adjust the conditional that examines
hostKey/rootKey (and any secret manager field) so the Write-Host message about
auto-population for HONCHO_CONFIG accurately reflects the runtime behavior.
Contributor
Author
There was a problem hiding this comment.
Not fixing since SecretManager path is dead — SECRET_KEYS.HONCHO isn't in ST's enum, so readSecret(undefined) always returns ''. Real runtime fallback is hosts.sillytavern.apiKey || globalConfig.apiKey. PR #14 removes the dead branch and codifies globalConfig as SSoT.
There was a problem hiding this comment.
@lowyelling, understood — thanks for the clarification. Since SECRET_KEYS.HONCHO is absent from ST's enum, readSecret(undefined) is a no-op and the effective fallback chain is hosts.sillytavern.apiKey → globalConfig.apiKey, which is exactly what the current probe logic reflects. My suggestion was based on an incorrect read of the runtime path. Makes sense to keep hostKey in the check until PR #14 lands.
✏️ Learnings added
Learnt from: lowyelling
Repo: plastic-labs/sillytavern-honcho PR: 13
File: install.ps1:138-154
Timestamp: 2026-05-01T19:59:36.570Z
Learning: In the sillytavern-honcho plugin (install.ps1 and plugin/index.js), the SecretManager path for the Honcho API key is dead code: `SECRET_KEYS.HONCHO` is not in SillyTavern's enum, so `readSecret(undefined)` always returns an empty string. The real runtime apiKey fallback chain is `hosts.sillytavern.apiKey` (from `~/.honcho/config.json`) first, then root-level `apiKey` (`globalConfig.apiKey`). PR `#14` removes the dead SecretManager branch and codifies globalConfig as the Single Source of Truth.
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.
ajspig
approved these changes
May 11, 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.
Backports the install.sh portability + UX fixes that landed in PR #4 to install.ps1, so Windows users get parity with macOS/Linux. Three commits, all mine — they were sitting on
eri/dev-1417after PR #8 merged into that branch instead of being promoted to main. Re-branched cleanly underlily/for ownership clarity.Summary
Verification
Follow-up
None.
Linear
DEV-1417
Summary by CodeRabbit