feat: smart Postgres detection and context-aware installer menu#127
feat: smart Postgres detection and context-aware installer menu#127AntTheLimey wants to merge 24 commits into
Conversation
…etection TcpListener binding to 127.0.0.1 missed Postgres instances listening on 0.0.0.0 or ::1. Get-NetTCPConnection reads the kernel TCP table directly and catches all listeners.
Scans ports 5432-5436 for running Postgres using pg_isready with fallbacks to lsof and ss. Populates DETECTED_PORTS and DETECTED_CONFIRMED arrays for the context-aware menu.
Scans ports 5432-5436 for running Postgres using pg_isready with fallback to Get-NetTCPConnection. Returns array of objects with Port and Confirmed properties.
Adds try_passwordless_auth, list_databases, connect_existing_instance, and prompt_credentials_and_list functions. Implements the four-tier dependency fallback chain: full psql, pg_isready only, port check only, and no tools.
Rewrites choose_database to detect running Postgres instances before presenting the menu. Shows detected instances with option to connect. Adds --detect flag for non-interactive auto-connection via passwordless auth.
Adds Test-PasswordlessAuth, Get-UserDatabases, Connect-ExistingInstance, and Invoke-CredentialPrompt functions. Mirrors the bash implementation with PowerShell idioms and secure password handling.
Rewrites Select-Database to detect running Postgres instances before presenting the menu. Adds -Detect parameter for non-interactive auto-connection. Mirrors the bash implementation.
Updates changelog with port detection fix, smart instance detection, --detect flag, and context-aware database menu.
Use the label variable in port display output. Remove unused demo_num variable assignment.
Fix Find-FreePort to filter by -State Listen so TIME_WAIT connections don't falsely mark ports as in use. Restructure Test-PasswordlessAuth with a single try/finally for reliable ErrorActionPreference restore. Clear $env:PGPASSWORD after use in all PowerShell functions. Validate --db-name database exists before accepting it in --detect mode. Restructure changelog to use Keep a Changelog categories.
|
Caution Review failedFailed to post review comments WalkthroughAdds interactive and non-interactive PostgreSQL detection to the claude-try-it installers (Bash + PowerShell): port scan (5432–5436), passwordless auth probing, per-instance database enumeration, auto-connect (--detect), existing-install/version detection, process cleanup, and updated README/changelog entries. ChangesInstaller + Docs
Sequence Diagram(s)sequenceDiagram
participant User
participant Installer as Installer Script
participant Scanner as Port Scanner
participant PG as PostgreSQL
participant Auth as Auth Probe
participant DB as DB Enumerator
participant Menu as Selection UI
User->>Installer: Run installer (--detect or interactive)
Installer->>Scanner: Scan ports 5432-5436
Scanner->>PG: Check listener (pg_isready / OS listener)
PG-->>Scanner: reachable / not reachable
Scanner-->>Installer: Detected instances
Installer->>Auth: Attempt passwordless auth per instance
Auth->>PG: psql connection attempt
PG-->>Auth: auth success / fail
Auth-->>Installer: auth result
alt Auth success
Installer->>DB: Enumerate databases for instance
DB->>PG: query database list
PG-->>DB: databases
DB-->>Installer: db list
else Auth fail
Installer->>Menu: Prompt for credentials
Menu-->>User: request password
User-->>Menu: provide credentials
Menu-->>Installer: credentials
Installer->>PG: authenticate with provided creds
end
Installer->>Menu: Present selection or auto-select
Menu-->>User: selection (or auto-connect)
User-->>Installer: chosen target
Installer->>Installer: configure DB connection / continue install or reconfigure
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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. Comment |
Up to standards ✅🟢 Issues
|
| Category | Results |
|---|---|
| BestPractice | 3 medium (3 false positives) |
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/claude-try-it/install.ps1`:
- Around line 508-534: The code overwrites $env:PGPASSWORD and always restores
it to $null, which clears any pre-existing session value; update the affected
helpers (the shown block plus Get-UserDatabases and Invoke-CredentialPrompt) to
save the original $env:PGPASSWORD into a local variable (e.g. $prevPGPASSWORD)
before setting it for the check, then in the finally block restore
$env:PGPASSWORD = $prevPGPASSWORD (not $null); ensure you still preserve and
restore $ErrorActionPreference as currently done and apply this save/restore
pattern in all three functions.
- Around line 513-550: The psql calls (e.g., in the auth-check function that
sets $script:AuthUser and in Get-UserDatabases) rely on default DB resolution
and also always exclude the 'postgres' DB; update those invocations to pass an
explicit database via -d using the configured DbName (or default to 'postgres'
when DbName is empty) so psql won't fail when a same-named DB doesn't exist, and
change the query in Get-UserDatabases to only exclude 'postgres' when the
requested DbName is not 'postgres' (i.e., make the filter conditional based on
the provided DbName parameter) so validation works for clusters containing only
the default database.
- Around line 579-580: Get-UserDatabases may return a scalar when there's only
one DB, causing $dbs[0] to grab a character; wrap each call to Get-UserDatabases
in array subexpression syntax @(...) to force array context. Specifically,
update the assignments to $dbs (used where you check .Count and foreach, where
you use index-based access $dbs[$i], where you use -contains, and where you
directly index $dbs[0] to set $script:DbName) so each is set as $dbs =
@(...Get-UserDatabases...), ensuring subsequent .Count, foreach, -contains and
indexing operate on an array rather than a scalar string.
In `@examples/claude-try-it/install.sh`:
- Around line 499-533: The psql authentication checks use the default database
(username) which can fail if a same-named DB doesn't exist, and list_databases
explicitly excludes 'postgres' which prevents detecting fresh clusters; update
the three psql calls in the detection/auth block to include -d postgres so they
always connect to the postgres database, and modify the list_databases function
to stop excluding 'postgres' (remove the "AND datname NOT IN ('postgres')"
filter) or make that exclusion conditional so --db-name=postgres is honored;
reference the psql invocations in the detection/auth block and the
list_databases function when making these changes.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0d3df874-8ef1-4305-8813-7a9096e94a32
📒 Files selected for processing (3)
docs/changelog.mdexamples/claude-try-it/install.ps1examples/claude-try-it/install.sh
The installer now checks for a previous installation before downloading. If the binary is current, it offers to reconfigure the database connection. If a newer version is available, it offers to update and kills stale MCP server processes first.
Add -d postgres to all psql calls so auth checks don't fail when a same-named database doesn't exist. Preserve existing PGPASSWORD in PowerShell instead of clearing to null. Wrap Get-UserDatabases calls in @() to prevent scalar unwrapping when only one database is returned.
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 `@examples/claude-try-it/install.ps1`:
- Around line 598-610: The code builds an "auth" option even when $hasPsql is
false, causing Invoke-CredentialPrompt to fail; fix by not adding a Type="auth"
PSCustomObject when $hasPsql is $false and instead add a different option (e.g.,
Type="manual" or Type="no-psql") that preserves the detected Port (InstIndex
null) so the UI routes to manual credential entry for that specific port; update
the prompt flow consumer (Invoke-CredentialPrompt call) to only be invoked for
options with Type="auth" and ensure Set-OwnDatabase reads and uses any prefilled
$script:DbHost and $script:DbPort values as the default values in its prompts so
manual entry uses the detected host/port.
- Around line 1245-1293: When an existing install is detected (installedVersion
-eq $script:Version) honor explicit DB flags and avoid overwriting configs:
before the early return in that branch, check for the explicit database flags
(e.g. -Detect, -Demo, -OwnDb) and call Select-Database non-interactively so
those flags can configure $script:DbConfigured; only call Set-ClaudeCodeConfig,
Set-ClaudeDesktopConfig and Write-Summary if $script:DbConfigured is $true. For
the interactive reconfigure path (Test-Interactive + Read-Prompt reconfigure),
after Select-Database also gate calls to
Set-ClaudeCodeConfig/Set-ClaudeDesktopConfig on $script:DbConfigured to avoid
writing placeholder configs when Select-Database leaves $script:DbConfigured
$false.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a11d7079-34da-4c30-82af-a1749833aacb
📒 Files selected for processing (3)
docs/changelog.mdexamples/claude-try-it/install.ps1examples/claude-try-it/install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/claude-try-it/install.sh
Route no-psql detected ports to manual entry instead of auth prompt. Honor explicit flags (-Detect, -Demo, -OwnDb / --detect, --demo, --own-db) in the existing-install flow instead of short-circuiting. Gate config writes on DbConfigured to avoid overwriting working configs with placeholders.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
examples/claude-try-it/install.ps1 (1)
1353-1356:⚠️ Potential issue | 🟠 MajorGuard config writes on
DbConfiguredin fresh-install path.Line 1354 and Line 1355 still run even when
Select-Databaseleaves$script:DbConfigured = $false, which can overwrite an existing working Claude config with placeholders.🐛 Proposed fix
Write-Host "" Select-Database - Write-Host "" - Set-ClaudeCodeConfig - Set-ClaudeDesktopConfig + Write-Host "" + if ($script:DbConfigured) { + Set-ClaudeCodeConfig + Set-ClaudeDesktopConfig + } else { + Write-Warn "Skipping Claude config update because database is not configured." + } Write-Summary🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.ps1` around lines 1353 - 1356, The installer currently calls Set-ClaudeCodeConfig and Set-ClaudeDesktopConfig unconditionally after Select-Database, which can overwrite a working config when Select-Database sets $script:DbConfigured = $false; update the fresh-install path to check the $script:DbConfigured flag returned/modified by Select-Database and only invoke Set-ClaudeCodeConfig and Set-ClaudeDesktopConfig when $script:DbConfigured -eq $true (i.e., guard the config write operations based on the Select-Database outcome).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/claude-try-it/install.ps1`:
- Around line 815-820: The DETECT_AUTH_FAILED output is too generic for
non-interactive runs; modify the detection branch that writes DETECT_AUTH_FAILED
(the block that sets $script:DbConfigured = $false and emits
"DETECT_AUTH_FAILED") to include a machine-readable, explicit failure reason
(e.g., a short token or the underlying error message) so automation can
distinguish cases like missing psql vs no matching port; populate and emit a new
variable/token such as $script:DetectFailureReason (or include
$Error[0].Exception.Message) alongside the existing message and ensure the
non-interactive path prints that token/message so callers can parse it.
---
Outside diff comments:
In `@examples/claude-try-it/install.ps1`:
- Around line 1353-1356: The installer currently calls Set-ClaudeCodeConfig and
Set-ClaudeDesktopConfig unconditionally after Select-Database, which can
overwrite a working config when Select-Database sets $script:DbConfigured =
$false; update the fresh-install path to check the $script:DbConfigured flag
returned/modified by Select-Database and only invoke Set-ClaudeCodeConfig and
Set-ClaudeDesktopConfig when $script:DbConfigured -eq $true (i.e., guard the
config write operations based on the Select-Database outcome).
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: a49402a7-f39f-4500-9857-8d279267cb0d
📒 Files selected for processing (2)
examples/claude-try-it/install.ps1examples/claude-try-it/install.sh
🚧 Files skipped from review as they are similar to previous changes (1)
- examples/claude-try-it/install.sh
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
examples/claude-try-it/install.sh (1)
1374-1378:⚠️ Potential issue | 🟠 MajorDon’t write Claude configs when DB selection failed.
When
choose_databasereturns withDB_CONFIGURED=false,mainstill writes configs using placeholder database values. This affects no-choice non-interactive runs and failed--detect/ Docker flows.Proposed fix
echo "" choose_database - echo "" - configure_claude_code - configure_claude_desktop + if [ "$DB_CONFIGURED" = true ]; then + echo "" + configure_claude_code + configure_claude_desktop + fi print_summary🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.sh` around lines 1374 - 1378, After calling choose_database in main, check the DB_CONFIGURED flag and do not proceed to write Claude configs if it is false; specifically, guard the calls to configure_claude_code and configure_claude_desktop (and any placeholder DB writes performed in main) behind a conditional that only runs when DB_CONFIGURED=true so non-interactive no-choice and failed --detect/Docker flows do not create configs with placeholder database values.examples/claude-try-it/install.ps1 (1)
1385-1390:⚠️ Potential issue | 🟠 MajorGate config writes after database selection.
If
Select-Databaseleaves$script:DbConfigured = $false—for example-Detectfinds nothing, Docker is unavailable, or non-interactive mode asks for a choice—Mainstill writes Claude configs with placeholder DB values.Proposed fix
Select-Database - Write-Host "" - Set-ClaudeCodeConfig - Set-ClaudeDesktopConfig + if ($script:DbConfigured) { + Write-Host "" + Set-ClaudeCodeConfig + Set-ClaudeDesktopConfig + } Write-Summary🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.ps1` around lines 1385 - 1390, Main currently calls Set-ClaudeCodeConfig and Set-ClaudeDesktopConfig unconditionally after Select-Database, which can leave placeholder DB values when Select-Database sets $script:DbConfigured = $false; change the flow so that after calling Select-Database you check $script:DbConfigured and only invoke Set-ClaudeCodeConfig and Set-ClaudeDesktopConfig when $script:DbConfigured -eq $true (or otherwise handle the non-configured path, e.g., abort or prompt), ensuring Select-Database, Set-ClaudeCodeConfig, and Set-ClaudeDesktopConfig are referenced when adding the conditional guard.
♻️ Duplicate comments (2)
examples/claude-try-it/install.sh (1)
1041-1045:⚠️ Potential issue | 🟠 MajorKeep detected host/port as manual-entry defaults.
manualPortpreloadsDB_HOST=localhostandDB_PORT=<detected port>, but these prompts still default tolocalhost:5432, so users can accidentally lose the selected port by pressing Enter.Proposed fix
- ask " Host [localhost]: " DB_HOST - DB_HOST="${DB_HOST:-localhost}" + local default_host="${DB_HOST:-localhost}" + ask " Host [$default_host]: " DB_HOST + DB_HOST="${DB_HOST:-$default_host}" - ask " Port [5432]: " DB_PORT - DB_PORT="${DB_PORT:-5432}" + local default_port="${DB_PORT:-5432}" + ask " Port [$default_port]: " DB_PORT + DB_PORT="${DB_PORT:-$default_port}"🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.sh` around lines 1041 - 1045, The prompts always fall back to hardcoded defaults ("localhost" and "5432") which can overwrite values preloaded by manualPort; update the prompt logic so the ask calls use the current DB_HOST and DB_PORT variables as their displayed/default values (i.e., pass DB_HOST and DB_PORT into the prompts instead of literal "localhost" and "5432") so existing detected values are preserved when the user presses Enter; locate the ask invocations around the DB_HOST/DB_PORT variables and ensure you only set DB_HOST="${DB_HOST:-localhost}" and DB_PORT="${DB_PORT:-5432}" before prompting or use the variables directly in the prompt text to avoid losing preloaded values.examples/claude-try-it/install.ps1 (1)
1052-1053:⚠️ Potential issue | 🟠 MajorPreserve prefilled manual connection defaults.
The
manualPortflow sets$script:DbPortto the selected detected port, butSet-OwnDatabasestill prompts with and defaults to5432, so pressing Enter silently discards the selected port.Proposed fix
- $script:DbHost = Read-Prompt " Host [localhost]" "localhost" - $script:DbPort = Read-Prompt " Port [5432]" "5432" + $defaultHost = if ($script:DbHost) { $script:DbHost } else { "localhost" } + $defaultPort = if ($script:DbPort) { $script:DbPort } else { "5432" } + $script:DbHost = Read-Prompt " Host [$defaultHost]" $defaultHost + $script:DbPort = Read-Prompt " Port [$defaultPort]" $defaultPort🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.ps1` around lines 1052 - 1053, Set-OwnDatabase is prompting with a hardcoded default "5432" which overrides the previously-detected manual port stored in $script:DbPort; change the Read-Prompt call in Set-OwnDatabase to use $script:DbPort as the default (i.e., pass $script:DbPort as the second argument to Read-Prompt for the Port prompt) so that pressing Enter preserves the detected/selected manual port value from the manualPort flow.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/claude-try-it/install.ps1`:
- Around line 1273-1274: The Test-ExistingInstall function sets
$explicitReconfigure without considering $InstallDocker, so calling the script
with -InstallDocker on an existing install is ignored; update the assignment of
$explicitReconfigure in Test-ExistingInstall (where $explicitReconfigure is
currently set) to include $InstallDocker (or otherwise OR in the $InstallDocker
flag) so that Select-Database and downstream logic will respect -InstallDocker
for existing installations; ensure this change is applied where
$explicitReconfigure is defined and any code paths that branch on it (e.g., the
path that calls Select-Database / Docker setup) will now execute when
-InstallDocker is provided.
In `@examples/claude-try-it/install.sh`:
- Around line 1284-1317: Replace the interactive TTY checks that use the test "[
-t 0 ]" in the existing-install flow with the project helper function "has_tty"
so prompts behave correctly when stdin is redirected; specifically, update the
conditional branches that guard the reconfigure prompt and the later info block
(the "elif [ -t 0 ]" and the later "if [ -t 0 ]" surrounding the choose_database
/ configure_claude_code / configure_claude_desktop / print_summary logic) to
call has_tty instead and keep the existing branches and behavior intact when
has_tty returns false.
- Around line 794-797: The grep call that checks for the requested database name
treats DB_NAME as a regex which breaks names containing regex metacharacters;
update the literal match to use fixed-string mode by changing the grep
invocation that tests dbs for DB_NAME (the line with echo "$dbs" | grep -qx
"$DB_NAME") so it uses the -F flag (i.e., grep -Fqx) to ensure list_databases
results are matched literally; keep the surrounding logic with DB_NAME and the
dbs variable intact.
- Around line 1248-1254: In stop_stale_processes replace full-command matching
flags by removing -f from pgrep and pkill so they match the command name only
(or use pgrep -xc for exact-count matching); update the pgrep invocation
(currently using pgrep -fc pgedge-postgres-mcp) to use name-only matching (e.g.,
pgrep -c or pgrep -xc) and remove -f from pkill -f pgedge-postgres-mcp so pkill
targets the command basename instead of the entire command line.
---
Outside diff comments:
In `@examples/claude-try-it/install.ps1`:
- Around line 1385-1390: Main currently calls Set-ClaudeCodeConfig and
Set-ClaudeDesktopConfig unconditionally after Select-Database, which can leave
placeholder DB values when Select-Database sets $script:DbConfigured = $false;
change the flow so that after calling Select-Database you check
$script:DbConfigured and only invoke Set-ClaudeCodeConfig and
Set-ClaudeDesktopConfig when $script:DbConfigured -eq $true (or otherwise handle
the non-configured path, e.g., abort or prompt), ensuring Select-Database,
Set-ClaudeCodeConfig, and Set-ClaudeDesktopConfig are referenced when adding the
conditional guard.
In `@examples/claude-try-it/install.sh`:
- Around line 1374-1378: After calling choose_database in main, check the
DB_CONFIGURED flag and do not proceed to write Claude configs if it is false;
specifically, guard the calls to configure_claude_code and
configure_claude_desktop (and any placeholder DB writes performed in main)
behind a conditional that only runs when DB_CONFIGURED=true so non-interactive
no-choice and failed --detect/Docker flows do not create configs with
placeholder database values.
---
Duplicate comments:
In `@examples/claude-try-it/install.ps1`:
- Around line 1052-1053: Set-OwnDatabase is prompting with a hardcoded default
"5432" which overrides the previously-detected manual port stored in
$script:DbPort; change the Read-Prompt call in Set-OwnDatabase to use
$script:DbPort as the default (i.e., pass $script:DbPort as the second argument
to Read-Prompt for the Port prompt) so that pressing Enter preserves the
detected/selected manual port value from the manualPort flow.
In `@examples/claude-try-it/install.sh`:
- Around line 1041-1045: The prompts always fall back to hardcoded defaults
("localhost" and "5432") which can overwrite values preloaded by manualPort;
update the prompt logic so the ask calls use the current DB_HOST and DB_PORT
variables as their displayed/default values (i.e., pass DB_HOST and DB_PORT into
the prompts instead of literal "localhost" and "5432") so existing detected
values are preserved when the user presses Enter; locate the ask invocations
around the DB_HOST/DB_PORT variables and ensure you only set
DB_HOST="${DB_HOST:-localhost}" and DB_PORT="${DB_PORT:-5432}" before prompting
or use the variables directly in the prompt text to avoid losing preloaded
values.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b75097d7-4419-4f36-a997-160c5053438d
📒 Files selected for processing (2)
examples/claude-try-it/install.ps1examples/claude-try-it/install.sh
- Add -InstallDocker to $explicitReconfigure in install.ps1 - Use grep -Fqx for literal DB_NAME matching in install.sh - Anchor pgrep/pkill patterns to avoid path false matches - Replace [ -t 0 ] with has_tty in existing-install flow
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
examples/claude-try-it/install.ps1 (1)
458-458: Nit:Detect-PostgresInstancesuses an unapproved PowerShell verb.PSScriptAnalyzer flags
Detect-(PSUseApprovedVerbs). The idiomatic approved verbs for this kind of discovery areFind-orGet-. A rename (e.g.,Find-PostgresInstance, singular per PSUseSingularNouns) silences the warning and reads as more canonical PowerShell — but this is purely cosmetic and purely internal to the script, so feel free to defer.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.ps1` at line 458, Rename the PowerShell function Detect-PostgresInstances to use an approved verb and singular noun (e.g., Find-PostgresInstance or Get-PostgresInstance) to satisfy PSUseApprovedVerbs/PSUseSingularNouns; update the function declaration name and all internal references/call sites that invoke Detect-PostgresInstances (search for that symbol) so callers match the new name, and run PSScriptAnalyzer to confirm the warning is resolved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@examples/claude-try-it/install.ps1`:
- Line 1337: Write-UpdateSummary currently runs unconditionally and prints an
"existing database unchanged" message even when explicit reconfiguration is
requested; change the flow to avoid double/conflicting summaries by gating the
Write-UpdateSummary call on -not $explicitReconfigure (or otherwise
consolidating with the reconfigure path). Specifically, in the install.ps1 flow
where Write-UpdateSummary is invoked, wrap that call in a conditional such as if
(-not $explicitReconfigure) { Write-UpdateSummary } so that when Select-Database
and Write-Summary run (the explicit reconfigure branch) only the new
configuration summary is printed; update the analogous occurrence at the second
location (lines around 1347–1357) as well.
- Around line 1279-1283: The installer crashes when .version is empty because
(Get-Content -Raw) can return $null and calling .Trim() throws; change the logic
around the $versionFile read (the $versionFile / $installedVersion block) to
capture the raw content into a temp variable, check for $null (or coerce to
string) before calling .Trim(), and set $installedVersion to "" when the read
returns $null so the installer continues to Install-Binary; ensure you reference
the existing variables ($versionFile and $installedVersion) and the Get-Content
-Raw call when making the fix.
- Around line 1259-1269: Stop-StaleProcesses should avoid scanning all processes
and reading .Path (which can throw access errors) and must match the binary name
exactly; replace the Get-Process | Where-Object approach in Stop-StaleProcesses
with a direct Get-Process -Name 'pgedge-postgres-mcp' (use the process name
without .exe so the match is exact/not a substring) and supply -ErrorAction
SilentlyContinue (or wrap the call in try/catch) so failures are swallowed; then
use the returned $procs (no .Path access) to compute the count and call
Stop-Process -Force -ErrorAction SilentlyContinue on those PIDs.
In `@examples/claude-try-it/install.sh`:
- Around line 1322-1348: When an update is followed by an explicit reconfigure
the script currently calls download_binary then print_update_summary, then
choose_database/configure_claude_* and print_summary, producing contradictory
back-to-back summaries; change the flow so print_update_summary is suppressed
when $explicit_reconfigure is true (i.e. only call print_update_summary in the
branches where no reconfigure will occur), and make the same change in the
PowerShell logic by suppressing Write-UpdateSummary in Test-ExistingInstall when
a reconfiguration path will call Write-Summary (use the flags/variables
explicit_reconfigure and the equivalent detection in Test-ExistingInstall to
decide whether to emit the update summary).
---
Nitpick comments:
In `@examples/claude-try-it/install.ps1`:
- Line 458: Rename the PowerShell function Detect-PostgresInstances to use an
approved verb and singular noun (e.g., Find-PostgresInstance or
Get-PostgresInstance) to satisfy PSUseApprovedVerbs/PSUseSingularNouns; update
the function declaration name and all internal references/call sites that invoke
Detect-PostgresInstances (search for that symbol) so callers match the new name,
and run PSScriptAnalyzer to confirm the warning is resolved.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 188d11eb-2fbb-449a-a441-37bfca8a0f2d
📒 Files selected for processing (2)
examples/claude-try-it/install.ps1examples/claude-try-it/install.sh
- Gate config writes on DB_CONFIGURED/DbConfigured in main - Suppress update summary when explicit reconfigure follows - Use preloaded DB_HOST/DB_PORT as manual entry defaults - Use exact process name matching in stop_stale_processes - Null-safe .version file reading in Test-ExistingInstall - Rename Detect-PostgresInstances to Find-PostgresInstance
- Exit/return when user declines update instead of falling through to explicit_reconfigure block - Move Write-Summary inside DbConfigured guard in PS1 Main - Return 0 from connect_existing_instance on empty list to avoid set -eo pipefail terminating the script
- Add empty-instance guard in PS1 Connect-ExistingInstance
matching bash's early return when no instances match
- Align update prompt default to "y" in bash (was silently
declining on Enter; PS1 already defaulted to "y")
- Make reconfigure prompt default explicit ("n") in bash
- Back up invalid Claude Desktop JSON in PS1 instead of
silently skipping config write (matches bash and PS1's
own Claude Code behavior)
- Add confirmed/unconfirmed label on port display in PS1
to match bash's "(PostgreSQL)" vs "(service)" labels
- Remove double blank line in PS1 Write-UpdateSummary
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
♻️ Duplicate comments (1)
examples/claude-try-it/install.sh (1)
1251-1259:⚠️ Potential issue | 🟠 MajorFix
pgrep -x/pkill -xto actually match the MCP process on Linux.Without
-f,pgrep/pkillmatch against the process name from/proc/PID/stat, which the Linux kernel truncates to 15 characters (TASK_COMM_LEN). The binary namepgedge-postgres-mcpis 19 characters—its storedcommispgedge-postgres. Combined with-x(exact match), the patternpgedge-postgres-mcpwill never match any running process on Linux, sostop_stale_processesbecomes a silent no-op during upgrades and old server processes aren't stopped before the binary is replaced.Use
-fto match against the full command line instead, which is not truncated:Proposed fix
stop_stale_processes() { local count - count=$(pgrep -xc pgedge-postgres-mcp 2>/dev/null || true) + count=$(pgrep -fc '(^|/)pgedge-postgres-mcp( |$)' 2>/dev/null || true) if [ "$count" -gt 0 ] 2>/dev/null; then info "Stopping $count running MCP server process(es)..." - pkill -x pgedge-postgres-mcp 2>/dev/null || true + pkill -f '(^|/)pgedge-postgres-mcp( |$)' 2>/dev/null || true sleep 1 fi }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@examples/claude-try-it/install.sh` around lines 1251 - 1259, stop_stale_processes currently uses pgrep -x / pkill -x which match the truncated comm on Linux and will never find "pgedge-postgres-mcp"; change those calls to use the full-command-line matcher (-f) so the 19-char binary name is matched: replace pgrep -xc pgedge-postgres-mcp with pgrep -c -f pgedge-postgres-mcp (or pgrep -fc -f pgedge-postgres-mcp) to count matches and replace pkill -x pgedge-postgres-mcp with pkill -f pgedge-postgres-mcp to actually kill the running MCP processes in stop_stale_processes.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@examples/claude-try-it/install.sh`:
- Around line 1251-1259: stop_stale_processes currently uses pgrep -x / pkill -x
which match the truncated comm on Linux and will never find
"pgedge-postgres-mcp"; change those calls to use the full-command-line matcher
(-f) so the 19-char binary name is matched: replace pgrep -xc
pgedge-postgres-mcp with pgrep -c -f pgedge-postgres-mcp (or pgrep -fc -f
pgedge-postgres-mcp) to count matches and replace pkill -x pgedge-postgres-mcp
with pkill -f pgedge-postgres-mcp to actually kill the running MCP processes in
stop_stale_processes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 16e1a916-489e-4324-ac78-5bbe179bd9d6
📒 Files selected for processing (3)
examples/claude-try-it/README.mdexamples/claude-try-it/install.ps1examples/claude-try-it/install.sh
✅ Files skipped from review due to trivial changes (1)
- examples/claude-try-it/README.md
Linux truncates process comm to 15 chars (TASK_COMM_LEN), so pgrep -x never matches "pgedge-postgres-mcp" (19 chars). Switch to pgrep -f with an anchored regex to match the full command line.
There was a problem hiding this comment.
Review Summary
Substantial enhancement (1,447 additions): instance discovery, context-aware menu with passwordless-auth + database-listing, non-interactive --detect/-Detect flag, existing-install update/reconfigure flows, and a Windows port-detection fix. The PR has been through 4 rounds of CodeRabbit review with all 15 review threads resolved, all 12 CI checks passing, and Codacy reporting "Up to standards" — it's in good shape. My findings are all minor refinements plus one merge-process concern.
I read both scripts end-to-end and audited the new flows: detection, passwordless auth, credential prompt, --detect auto-connect, existing-install update path, and JSON config writers. The error handling is careful (sentinel tokens for non-interactive output, .bak backups on invalid JSON, env-var-based python3 invocation to avoid injection, 4-tier graceful degradation when pg_isready/psql/lsof are missing).
Findings
🟡 Should Fix — Branch is 57 commits behind main; changelog will conflict at merge
The PR branch is 57 commits behind main. The diff currently shows main's ### Fixed entry for #117 (Database switching via select_database_connection) being deleted, because the branch never had it. Main has also acquired entries for #126 and #142 since this branch was cut. A simple GitHub merge will not resolve this cleanly — the [Unreleased] section needs a manual reconcile. Recommend rebasing onto main (or merging main in) before merge. The conflict zone is small (just docs/changelog.md [Unreleased]); the install scripts themselves shouldn't conflict with anything on main.
🟡 Should Fix — Cross-platform parity: case-sensitive --db-name/-DbName match
examples/claude-try-it/install.ps1:797:
if ($dbs -contains $script:DbName) {PowerShell's -contains operator is case-insensitive for strings by default. The bash counterpart at examples/claude-try-it/install.sh:798 uses echo "$dbs" | grep -Fqx -- "$DB_NAME", which is case-sensitive.
PostgreSQL identifiers are case-sensitive when not folded — a database created as "MyDB" is distinct from mydb. If a user runs install.ps1 -Detect -DbName MyDB and only mydb exists, this branch silently configures Claude with DbName=MyDB. The Postgres connection then fails at use time with a confusing database "MyDB" does not exist error from psql.
One-character fix: -ccontains (the case-sensitive variant).
🟢 Suggestion — find_free_port doesn't fall back when lsof is missing
examples/claude-try-it/install.sh:434-445:
find_free_port() {
for port in 5432 5433 5434 5435 5436; do
if ! lsof -i ":$port" >/dev/null 2>&1; then
echo "$port"; return
fi
done
...
}If lsof isn't installed (some minimal Linux containers, alpine without lsof), lsof -i ":$port" exits non-zero, the if ! triggers, and every port is treated as free. The demo container then tries to bind 5432 on top of an existing Postgres and fails with a port-bind error in the docker-compose log.
The detection function detect_postgres_instances (lines 459-486) already implements a 3-tier fallback: pg_isready → lsof → ss. find_free_port should mirror at least the ss step. Suggested:
port_in_use() {
local p="$1"
if command -v lsof &>/dev/null; then
lsof -i ":$p" >/dev/null 2>&1
elif command -v ss &>/dev/null; then
ss -tlnH "sport = :$p" 2>/dev/null | grep -q .
else
(echo >/dev/tcp/localhost/"$p") 2>/dev/null
fi
}The PowerShell counterpart Find-FreePort uses Get-NetTCPConnection, which is always present on supported Windows versions, so it doesn't have this issue.
ℹ️ Note — README doesn't mention demo container cleanup wipes prior demos
cleanup_old_demos (install.sh:839-847) / Remove-OldDemos (install.ps1:843-854) removes every pgedge-demo-* container with docker rm -v (volumes too). Intentional and correct given the unique-timestamp naming for new containers, but a user who's been experimenting with multiple demo runs may not expect prior data to be wiped. Worth a one-liner in examples/claude-try-it/README.md, e.g.:
Each demo run replaces any previous demo container (
pgedge-demo-*); previous demo data in those containers is removed.
ℹ️ Note — 120s readiness budget may be tight on first-run demos
start_demo_postgres waits up to 24×5s = 120s for pg_isready inside the container. On a fresh machine this has to cover docker image pull + Northwind SQL download + import. The script handles the timeout gracefully (DB_CONFIGURED=true with a warning) so the user gets a configured Claude — but the first query may hit a "database not ready" error on slow networks. Not a blocker, just noting.
Things I checked that are correct
- ✅
PGPASSWORDis set per-process in bash; saved/restored viatry/finallyin PS. Never leaks to other processes. - ✅ JSON config writes use python3 with env-var-based value passing (no shell or python injection); manual fallback uses a robust
json_escape. PS usesConvertTo-Json. - ✅ Invalid pre-existing JSON configs are backed up to
.bakinstead of silently overwritten (matches PR hardening notes). - ✅
stop_stale_processesuses anchored regex(^|/)pgedge-postgres-mcp( |$); PS uses exactGet-Process -Name. Both avoid false positives likepgedge-postgres-mcp-foo. - ✅ Sentinel tokens (
DETECT_NO_INSTANCES,DETECT_AUTH_FAILED,DOCKER_NOT_RUNNING, etc.) are consistent across both scripts — useful integration surface for Claude Code. - ✅
DB_CONFIGUREDgating prevents placeholder values (your_database,your_user) from being written; traced every code path. - ✅
check_existing_install/Test-ExistingInstallshort-circuit cleanly viaexit 0/ earlyreturn— no double download. - ✅
bash -nsyntax check passes. - ✅ All 12 CI checks green; CodeRabbit complete; Codacy "Up to standards".
Checklist
- Correctness — flows trace cleanly
- Security — credentials, JSON writes, and process-stop logic are safe
- Cross-platform parity — see
install.ps1:797(case-sensitivity) - Robustness — see
install.sh:434-445(find_free_portfallback) - Tests — manual test plan in PR description is comprehensive
- Documentation — README missing demo-cleanup note
- Branch state — 57 commits behind main; rebase before merging
Review generated with Claude Code.
- Use case-sensitive -ccontains for database name matching in install.ps1 to match the case-sensitive grep -Fqx in install.sh - Extract port_in_use() helper with lsof/ss/dev-tcp fallback so find_free_port works on systems without lsof - Add README note that demo runs remove previous pgedge-demo-* containers and their data
AntTheLimey
left a comment
AntTheLimey
left a comment
There was a problem hiding this comment.
Thanks for the thorough review, Dave. All four actionable items addressed in f5bab72:
Case-sensitive -ccontains -- Fixed. Changed -contains to -ccontains at install.ps1:797 so database name matching is case-sensitive, consistent with the grep -Fqx in install.sh.
find_free_port lsof fallback -- Fixed. Extracted a port_in_use() helper with 3-tier fallback (lsof -> ss -> /dev/tcp), mirroring the degradation strategy already used in detect_postgres_instances.
README cleanup note -- Added a line in the Install Location section noting that each demo run removes previous pgedge-demo-* containers and their data.
57 commits behind main -- Merged origin/main into the branch. The changelog auto-merged cleanly with all three existing entries (#117, #126, #142) preserved.
Re the 120s readiness budget note -- agreed, the timeout is tight on cold pulls but the graceful degradation path handles it. Worth revisiting if we get reports.
2 participants
Summary
Adds Postgres instance discovery on startup (ports 5432-5436)
using
pg_isreadywith platform-native fallbacks (lsof,ss,Get-NetTCPConnection).Replaces the static two-option menu with a context-aware menu
that adapts based on detected instances: connect to existing,
start demo on a non-conflicting port, or enter details manually.
Adds a connect-to-existing flow with passwordless auth attempt,
credential prompting, and automatic database listing.
Adds
--detect(bash) /-Detect(PowerShell) flag fornon-interactive auto-connection to detected instances.
Detects previous installations and offers to update the binary
or reconfigure the database connection instead of re-running
the full install flow.
Fixes Windows port detection (
TcpListener->Get-NetTCPConnection) to reliably detect Postgres on allnetwork addresses.
Graceful four-tier degradation when
pg_isreadyorpsqlare not available.
Adds a README for the try-it-now installer scripts documenting
flags, detection behavior, and install location.
Hardening (review rounds)
values are never written to Claude Code or Claude Desktop configs.
through to the reconfigure block.
avoid contradictory double summaries.
detected values survive the prompt flow.
file.
Get-Process -Name) to avoid false positives.
skipping the config write (matches bash behavior).
bash's early return when no instances match.
defaulted to "y").
bash's "(PostgreSQL)" vs "(service)" output.
(PSScriptAnalyzer approved verb).
Files changed
examples/claude-try-it/install.sh-- instance detection,context-aware menu, connect-to-existing flow, existing-install
detection,
--detectflag, hardening fixesexamples/claude-try-it/install.ps1-- same changes plusFind-FreePortfix and PS1-specific hardeningexamples/claude-try-it/README.md-- new; documents flags,detection behavior, and install location
docs/changelog.md-- Added/Fixed entries for installer changesTest plan
install.shon macOS with Postgres on 5432 -- verifydetection, context-aware menu, database listing
install.shon macOS with no Postgres -- verify fallbackto original 2-option menu
install.sh --detect-- verify auto-connectioninstall.sh --detect --db-name=nonexistent-- verifyerror message about database not found
install.ps1on Windows with Postgres on 5432 -- verifydetection and
Find-FreePortpicks a non-conflicting portinstall.ps1 -Detecton Windows -- verify auto-connectioninstall.shwithoutpg_isreadyinstalled -- verifyTier 3 fallback (port check only, no confirmation)
install.shwithoutpg_isreadyorpsql-- verifyTier 4 fallback (original menu)
"already up to date" flow with reconfigure prompt
update prompt, then reconfigure if flags provided
exit with no database prompts
install.sh --demoover existing install -- verifyupdate + reconfigure to demo in one pass
Summary by CodeRabbit
New Features
Bug Fixes
Documentation