fix: show full binary path in user-facing command hints - #616
Conversation
The version-mismatch warning told users to restart the supervisor with `pitchfork supervisor start --force`, relying on whatever `pitchfork` resolves to on PATH. When the PATH binary differs from the one the user just invoked, the hint restarts the wrong binary and the mismatch persists. Use the existing `env::PITCHFORK_BIN` (current_exe, canonicalized) so the hint names the exact CLI binary the user invoked.
📝 WalkthroughWalkthroughIPC connection diagnostics and supervisor guidance now use the configured CLI executable across connection, proxy, TUI, and API surfaces. IPC error variants carry the executable name, and ready-check tests allow additional CI scheduling and output-delivery latency. ChangesExecutable-aware IPC diagnostics
Estimated code review effort: 3 (Moderate) | ~20 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Greptile SummaryThis PR replaces hardcoded
Confidence Score: 5/5Safe to merge — all changes are additive substitutions of a static string with a runtime-resolved path, with no logic changes and no broken pattern-match sites. The change is mechanically straightforward: every hardcoded 'pitchfork' in user-facing hints is replaced with PITCHFORK_BIN.display(), which is already used widely across the codebase. The Timeout/ConnectionClosed enum changes add a struct field but no existing match sites were found. The bats timing relaxation is well-reasoned and preserves the regression detection margin. No logic paths were altered. No files require special attention. Important Files Changed
Reviews (6): Last reviewed commit: "fix(supervisor): keep version-mismatch h..." | Re-trigger Greptile |
![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 `@src/ipc/client.rs`:
- Around line 34-37: Update the restart-hint construction around PITCHFORK_BIN
and the corresponding occurrences to shell-escape the executable path instead of
using raw Path::display() output. Use the project’s platform-appropriate quoting
mechanism, preserve the existing command structure, and add coverage for
executable paths containing spaces.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: c8cd23e0-514f-40e4-9cb3-c7f160164d79
📒 Files selected for processing (1)
src/ipc/client.rs
| // Resolve the running CLI's binary path so the restart hint points at | ||
| // the exact binary the user invoked, not whatever `pitchfork` resolves | ||
| // to on PATH (which may be a different version). | ||
| let exe = env::PITCHFORK_BIN.display(); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Shell-escape the executable path in restart hints.
Path::display() emits the raw path. If the binary is installed under a directory containing spaces or shell metacharacters, copying {exe} supervisor ... will fail or be interpreted incorrectly. Use platform-appropriate quoting/escaping and add coverage for paths containing spaces.
Also applies to: 54-54, 71-71
🤖 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 `@src/ipc/client.rs` around lines 34 - 37, Update the restart-hint construction
around PITCHFORK_BIN and the corresponding occurrences to shell-escape the
executable path instead of using raw Path::display() output. Use the project’s
platform-appropriate quoting mechanism, preserve the existing command structure,
and add coverage for executable paths containing spaces.
…rn tests `pitchfork start` returns as soon as the ready_output pattern matches a line on the daemon's stdout. Under CI, bats runs 16-way parallel (`mise run test:bats` uses `bats --jobs 16`) on a runner shared with the build, rust-tests, and web-ui workflows, so the supervisor's tokio output processing can be starved for a few seconds. This made the tight `[[ $elapsed -lt 3 ]]` bound flake: ci-bats(0) failed "ready output pattern matches and returns early" with ready detected at ~4s while the identical sibling "ready output regex" test passed the same run. Raise the early-return bound to 8s (10s on Windows) for the two ready_output tests. The daemon's trailing `sleep 10` still gives a deterministic regression baseline (~11s on no-match daemon exit), so a regression that skips early return still fails the bound. The timer-based `custom ready delay` test is untouched; its ready_delay fires on a wall-clock timer, not stdout delivery, so it is not subject to this latency.
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 `@test/basic.bats`:
- Around line 330-334: Update the timing comment above max_elapsed in the
parallel-bats scheduling test to state that the expected regression path remains
approximately 11 seconds, preserving the test’s detection margin; leave the
8-second and Windows 10-second thresholds unchanged.
🪄 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: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: b297e06a-8fbb-45ef-b45f-28bbffc54ea8
📒 Files selected for processing (1)
test/basic.bats
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 1 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
|
This PR currently has failing checks. If this continues for 7 days, it will be closed automatically. This is warning day 2 of 7. Please update the PR when you have a chance. Feel free to reopen or create a new PR if it is closed and you'd like to continue working on it. This comment was generated by an automated workflow. |
The Windows threshold (10s) equals the daemon's trailing sleep, so the prior "well under" wording was self-contradictory. Restate the detection basis: the regression path (no early return → daemon exit ~11s) still exceeds the 8s (10s on Windows) bound. Thresholds unchanged.
Several runtime messages told users to run `pitchfork <cmd> ...`, relying
on whatever `pitchfork` resolves to on PATH. When the PATH binary differs
from the one the user invoked (or under sudo, whose PATH differs), following
the hint runs the wrong binary. Use `crate::env::PITCHFORK_BIN`
(current_exe, canonicalized) so every hint names the exact binary.
CLI-side (current_exe = the binary the user just ran, authoritative):
- src/proxy/server.rs: "sudo pitchfork supervisor start" on low-port bind
- src/ipc/client.rs: three "ensure the supervisor is running with:
pitchfork supervisor start" connection-error helps
- src/cli/proxy.rs: "PITCHFORK_PROXY_ENABLE=true pitchfork supervisor start"
- src/proxy/trust.rs: "pitchfork supervisor start" in the missing-CA hint
- src/tui/ui.rs: "Start one with: pitchfork start <name>"
Supervisor-side (current_exe = the running supervisor binary; correct in the
common single-binary / upgraded-in-place case):
- src/supervisor/ipc_handlers.rs: version-mismatch restart hint
- src/web/routes/api/proxies.rs: namespace registration hint
error.rs: the two IPC diagnostic helps (`#[diagnostic(help(...))]`) are
static attribute literals, so they can't interpolate a runtime path directly.
Add a `bin: String` field to `Timeout` and `ConnectionClosed` and
interpolate `{bin}` in the help text, populated from PITCHFORK_BIN at
construction.
CLI help-text / doc examples are intentionally left as `pitchfork` — that's
documentation, not a runtime hint.
gaojunran
changed the title
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/ipc/client.rs (1)
54-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShell-escape the executable path in command hints.
The binary path is inserted directly into the suggested commands without quotes. If
PITCHFORK_BINresolves to a path containing spaces, the suggested command will fail or be misinterpreted when a user copy-pastes it into their shell. Wrap the interpolated path in double quotes (\") at all sites.
src/ipc/client.rs#L54-L54: Add quotes around{exe}.src/ipc/client.rs#L71-L71: Add quotes around{exe}.src/ipc/client.rs#L142-L145: Format aswith: \"{}\" supervisor start.src/ipc/client.rs#L155-L158: Format aswith: \"{}\" supervisor start.src/ipc/client.rs#L168-L169: Format aswith: \"{}\" supervisor start.src/error.rs#L349-L362: Quote{bin}in both theTimeoutandConnectionClosedhelp strings.src/supervisor/ipc_handlers.rs#L55-L56: Format aswith: \"{}\" supervisor start.src/cli/proxy.rs#L183-L186: Format asPITCHFORK_PROXY_ENABLE=true \"{}\" supervisor start.src/proxy/server.rs#L1630-L1634: Format asTry: sudo \"{}\" supervisor start.src/proxy/trust.rs#L140-L146: Format as\"{}\" supervisor start.src/tui/ui.rs#L168-L173: Format aswith: \"{}\" start <name>.src/web/routes/api/proxies.rs#L89-L92: Replace the single quotes wrapping the entire command with double quotes specifically around the executable:Run: \"{}\" supervisor namespace add {} /path/to/dir.🤖 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 `@src/ipc/client.rs` at line 54, Shell-escape executable paths in every listed command hint: update src/ipc/client.rs lines 54, 71, 142-145, 155-158, and 168-169; quote {exe} or format the executable as "{}" before supervisor start. In src/error.rs lines 349-362, quote {bin} in both Timeout and ConnectionClosed help strings. Apply the specified "{}" formatting in src/supervisor/ipc_handlers.rs lines 55-56, src/cli/proxy.rs lines 183-186, src/proxy/server.rs lines 1630-1634, src/proxy/trust.rs lines 140-146, and src/tui/ui.rs lines 168-173. In src/web/routes/api/proxies.rs lines 89-92, remove the command-wide single quotes and quote only the executable in the Run hint.
🤖 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.
Outside diff comments:
In `@src/ipc/client.rs`:
- Line 54: Shell-escape executable paths in every listed command hint: update
src/ipc/client.rs lines 54, 71, 142-145, 155-158, and 168-169; quote {exe} or
format the executable as "{}" before supervisor start. In src/error.rs lines
349-362, quote {bin} in both Timeout and ConnectionClosed help strings. Apply
the specified "{}" formatting in src/supervisor/ipc_handlers.rs lines 55-56,
src/cli/proxy.rs lines 183-186, src/proxy/server.rs lines 1630-1634,
src/proxy/trust.rs lines 140-146, and src/tui/ui.rs lines 168-173. In
src/web/routes/api/proxies.rs lines 89-92, remove the command-wide single quotes
and quote only the executable in the Run hint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Central YAML (base), Organization UI (inherited)
Review profile: CHILL
Plan: Pro Plus
Run ID: 6151cd1e-941b-4d52-8bc8-ca03ee2892fd
📒 Files selected for processing (8)
src/cli/proxy.rssrc/error.rssrc/ipc/client.rssrc/proxy/server.rssrc/proxy/trust.rssrc/supervisor/ipc_handlers.rssrc/tui/ui.rssrc/web/routes/api/proxies.rs
Reverts the supervisor-side version-mismatch hint to bare `pitchfork`. The supervisor resolves env::PITCHFORK_BIN via current_exe(), which is the RUNNING supervisor's binary. In the two-binaries case this PR exists to handle (new CLI vs old supervisor), that path points at the old binary, so "Restart with: <path> supervisor start --force" restarts the wrong (old) executable and the mismatch persists — the very problem this PR fixes, now on the other side. The supervisor cannot know which CLI binary the user invoked (ConnectV2 only carries the version), so leave its hint as bare `pitchfork` (PATH-resolved). The authoritative full-path hint is the CLI-side one in ipc/client.rs, where current_exe is the binary the user just ran. Other supervisor-side hints (proxy/server.rs port-elevation, web/proxies.rs namespace add) are operational, not version-sensitive, so pointing at the supervisor's own known-working binary is correct there.
|
Thanks for the work here, but I don't think this change is worth it. Expanding these hints to the full This comment was generated by Claude Code. |
2 participants
Problem
Several runtime messages tell the user to run
pitchfork <cmd> ..., relying on whateverpitchforkresolves to onPATH. When thePATHbinary differs from the one the user just invoked — e.g. a newer build in~/.cargo/binwhilePATHstill points at an older/usr/bin/pitchfork, or undersudo(whosePATHdiffers) — following the hint runs the wrong binary and the problem persists.Fix
Use the existing
env::PITCHFORK_BIN(current_exe().canonicalize(), with a fallback) so hints name the exact binary:CLI-side hints (authoritative)
current_exeis the binary the user just ran — the right one to point at.src/proxy/server.rs—sudo pitchfork supervisor starton low-port bind (thesudocase is especially important, since sudo'sPATHdiffers).src/ipc/client.rs— threeensure the supervisor is running with: pitchfork supervisor startconnection-error helps; plus the original version-mismatch warning (Restart the supervisor with: <bin> supervisor start --force).src/cli/proxy.rs—PITCHFORK_PROXY_ENABLE=true pitchfork supervisor start.src/proxy/trust.rs—pitchfork supervisor startin the missing-CA hint (install_certis called from the CLI).src/tui/ui.rs—Start one with: pitchfork start <name>.Other supervisor-side hints (operational, not version-sensitive)
current_exeis the running supervisor binary — a known-working binary. Fine for operational hints that aren't about upgrading.src/proxy/server.rs—sudo <bin> supervisor starton low-port bind (elevation, not upgrade).src/web/routes/api/proxies.rs— namespace registration hint.Intentionally NOT converted
src/supervisor/ipc_handlers.rs— the version-mismatch restart hint is left as barepitchfork supervisor start --force. This hint is version-sensitive (its whole purpose is to upgrade the supervisor), and the supervisor resolvesPITCHFORK_BINto its OWN (potentially old) binary. In the two-binaries case this PR handles (new CLI vs old supervisor), a full path there would point at the wrong (old) executable — reintroducing the problem on the other side. The supervisor can't know which CLI binary the user invoked (ConnectV2only carries the version string), so the authoritative full-path hint is the CLI-side one inipc/client.rs.error.rsThe two IPC diagnostic helps (
#[diagnostic(help("..."))]) are static attribute literals, so they can't interpolate a runtime path directly. Added abin: Stringfield toTimeoutandConnectionClosedand interpolate{bin}in the help text, populated fromPITCHFORK_BINat construction (CLI-side, authoritative).Not changed
CLI help text / doc-comment examples (e.g.
pitchfork start api,pitchfork logs ...) are intentionally left aspitchfork— that's documentation, not a runtime hint, and the program name should read aspitchfork.CI flake mitigation
The Linux
ci-batsjob was flaking onready output pattern matches and returns earlybecausemise run test:batsrunsbats --jobs 16on a runner shared with the build/rust-tests/web-ui workflows, so the supervisor's tokio output processing can be starved for a few seconds. The< 3searly-return bound was too tight (one run detected ready at ~4s while the identical sibling "ready output regex" test passed the same run). Raised the bound to8s(10s on Windows); the daemon's trailingsleep 10still gives a deterministic ~11s regression baseline, so a regression that skips early return still fails the bound. The timer-basedcustom ready delaytest is untouched (itsready_delayfires on a wall-clock timer, not stdout delivery).Verification
cargo fmt --check✓cargo clippy --all-targets --all-features -- -D warnings✓cargo nextest run: 476 passedmise run render: no doc diff (no CLI/schema change)This PR description was generated by Claude Code.