[codex] Add CLI path visibility guidance#1053
Closed
TabishB wants to merge 1 commit into
Closed
Conversation
Contributor
📝 WalkthroughWalkthroughThis PR enhances OpenSpec with PATH visibility diagnostics that detect when the OpenSpec CLI bin directory is not on the system PATH (especially during global installations), generate troubleshooting guidance in the postinstall script and skill generation, and update onboard workflow templates to check CLI visibility via shell environment variables. ChangesPATH Visibility Detection & Guidance
Sequence DiagramsequenceDiagram
actor User
participant NPM Install
participant Postinstall Script
participant Skill Generator
participant Shell
participant User Shell
User->>NPM Install: npm install -g `@fission-ai/openspec`
NPM Install->>Postinstall Script: Trigger postinstall hook
Postinstall Script->>Postinstall Script: Detect if global install
Postinstall Script->>Postinstall Script: Gather npm/pnpm/bun bin directories
Postinstall Script->>Postinstall Script: Check if bin dirs exist<br/>and are missing from PATH
Postinstall Script->>User: Print PATH visibility hint<br/>(if missing dirs detected)
Note over Skill Generator,User Shell: Later: During skill generation
Skill Generator->>Skill Generator: Check if compatibility<br/>mentions "openspec cli"
Skill Generator->>Skill Generator: Prepend CLI visibility<br/>guidance to YAML
Skill Generator->>User: Generated skill with<br/>PATH troubleshooting
User->>User Shell: Execute generated skill
User Shell->>Shell: Check OPENSPEC_BIN<br/>or openspec via command -v
Shell-->>User Shell: CLI visibility status
User Shell->>User: Print CLI_NOT_ON_PATH<br/>if not found
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
scripts/test-postinstall.sh (1)
29-47: ⚡ Quick winAdd an assertion for the expected PATH hint output.
Line 29 introduces a new scenario, but it currently does not verify that the hint text is actually emitted. That makes this test a smoke run instead of a regression check.
Suggested test hardening
TMP_PREFIX="$(mktemp -d)" TMP_HOME="$(mktemp -d)" mkdir -p "$TMP_PREFIX/bin" printf '#!/bin/sh\nexit 0\n' > "$TMP_PREFIX/bin/openspec" chmod +x "$TMP_PREFIX/bin/openspec" unset CI unset OPENSPEC_NO_COMPLETIONS export npm_config_global=true export npm_config_prefix="$TMP_PREFIX" -HOME="$TMP_HOME" PNPM_HOME="$TMP_HOME/no-pnpm" PATH="/usr/bin:/bin" "$NODE_BIN" scripts/postinstall.js +OUTPUT="$(HOME="$TMP_HOME" PNPM_HOME="$TMP_HOME/no-pnpm" PATH="/usr/bin:/bin" "$NODE_BIN" scripts/postinstall.js)" +echo "$OUTPUT" +if [[ "$OUTPUT" != *"not on PATH"* ]]; then + echo "Expected PATH visibility hint was not printed" + exit 1 +fi rm -rf "$TMP_PREFIX" rm -rf "$TMP_HOME" unset npm_config_global unset npm_config_prefix export PATH="$ORIGINAL_PATH"🤖 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 `@scripts/test-postinstall.sh` around lines 29 - 47, The new Test 2 runs the postinstall.js scenario but doesn't assert that the PATH hint was printed; capture the command output when invoking HOME="$TMP_HOME" PNPM_HOME="$TMP_HOME/no-pnpm" PATH="/usr/bin:/bin" "$NODE_BIN" scripts/postinstall.js (e.g., redirect stdout/stderr to a temp file or variable) and add an assertion that the output contains the expected PATH hint text emitted by scripts/postinstall.js (search for the specific hint substring your postinstall prints, e.g., "add to your PATH" or similar) so the test fails if the hint is missing; keep teardown (rm -rf and env unsets) unchanged.
🤖 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/core/templates/workflows/onboard.ts`:
- Around line 31-33: The check combining command -v and [ -x "$OPENSPEC_CMD" ]
can yield false positives when OPENSPEC_CMD is a bare name (e.g., "openspec")
that is executable only in the current directory; change the condition to only
use -x when OPENSPEC_CMD is a path (contains a slash) and otherwise rely on
command -v. Concretely, update the if that references OPENSPEC_CMD to: if
command -v "$OPENSPEC_CMD" >/dev/null 2>&1 || { [[ "$OPENSPEC_CMD" == */* ]] &&
[ -x "$OPENSPEC_CMD" ]; }; then ... so the script emits the intended
CLI_NOT_ON_PATH when a bare command name isn't on PATH.
---
Nitpick comments:
In `@scripts/test-postinstall.sh`:
- Around line 29-47: The new Test 2 runs the postinstall.js scenario but doesn't
assert that the PATH hint was printed; capture the command output when invoking
HOME="$TMP_HOME" PNPM_HOME="$TMP_HOME/no-pnpm" PATH="/usr/bin:/bin" "$NODE_BIN"
scripts/postinstall.js (e.g., redirect stdout/stderr to a temp file or variable)
and add an assertion that the output contains the expected PATH hint text
emitted by scripts/postinstall.js (search for the specific hint substring your
postinstall prints, e.g., "add to your PATH" or similar) so the test fails if
the hint is missing; keep teardown (rm -rf and env unsets) 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d995b2eb-3b4b-44e7-84ca-329246896758
📒 Files selected for processing (8)
.changeset/clear-path-hints.mddocs/installation.mdscripts/postinstall.jsscripts/test-postinstall.shsrc/core/shared/skill-generation.tssrc/core/templates/workflows/onboard.tstest/core/shared/skill-generation.test.tstest/core/templates/skill-templates-parity.test.ts
Comment on lines
+31
to
+33
| OPENSPEC_CMD="\${OPENSPEC_BIN:-openspec}" | ||
| if command -v "$OPENSPEC_CMD" >/dev/null 2>&1 || [ -x "$OPENSPEC_CMD" ]; then | ||
| "$OPENSPEC_CMD" --version |
Contributor
There was a problem hiding this comment.
Avoid false positives from -x on bare command names.
Line 32 can incorrectly pass when openspec exists only in the current directory (not on PATH). That can cause Line 33 to fail instead of emitting CLI_NOT_ON_PATH.
Suggested fix
OPENSPEC_CMD="${OPENSPEC_BIN:-openspec}"
-if command -v "$OPENSPEC_CMD" >/dev/null 2>&1 || [ -x "$OPENSPEC_CMD" ]; then
+if command -v "$OPENSPEC_CMD" >/dev/null 2>&1; then
+ "$OPENSPEC_CMD" --version
+elif [ "${OPENSPEC_CMD#*/}" != "$OPENSPEC_CMD" ] && [ -x "$OPENSPEC_CMD" ]; then
"$OPENSPEC_CMD" --version
else
echo "CLI_NOT_ON_PATH"
fi📝 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
| OPENSPEC_CMD="\${OPENSPEC_BIN:-openspec}" | |
| if command -v "$OPENSPEC_CMD" >/dev/null 2>&1 || [ -x "$OPENSPEC_CMD" ]; then | |
| "$OPENSPEC_CMD" --version | |
| OPENSPEC_CMD="${OPENSPEC_BIN:-openspec}" | |
| if command -v "$OPENSPEC_CMD" >/dev/null 2>&1; then | |
| "$OPENSPEC_CMD" --version | |
| elif [ "${OPENSPEC_CMD#*/}" != "$OPENSPEC_CMD" ] && [ -x "$OPENSPEC_CMD" ]; then | |
| "$OPENSPEC_CMD" --version | |
| else | |
| echo "CLI_NOT_ON_PATH" | |
| fi |
🤖 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/core/templates/workflows/onboard.ts` around lines 31 - 33, The check
combining command -v and [ -x "$OPENSPEC_CMD" ] can yield false positives when
OPENSPEC_CMD is a bare name (e.g., "openspec") that is executable only in the
current directory; change the condition to only use -x when OPENSPEC_CMD is a
path (contains a slash) and otherwise rely on command -v. Concretely, update the
if that references OPENSPEC_CMD to: if command -v "$OPENSPEC_CMD" >/dev/null
2>&1 || { [[ "$OPENSPEC_CMD" == */* ]] && [ -x "$OPENSPEC_CMD" ]; }; then ... so
the script emits the intended CLI_NOT_ON_PATH when a bare command name isn't on
PATH.
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
1 participant
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.
Summary
PATH, with a troubleshooting link.OPENSPEC_BINor an absolute executable path.Why
OpenSpec can be installed correctly while still being unavailable to an agent or editor shell because that process inherits a different
PATHthan the user's terminal. The fix is to explain the package-manager-bin pattern generally and make generated workflows less likely to treatcommand not foundas proof that OpenSpec is not installed.Validation
pnpm lintpnpm testpnpm buildbash scripts/test-postinstall.shpnpm vitest run test/core/shared/skill-generation.test.ts test/core/templates/skill-templates-parity.test.tspnpm exec changeset status --since=origin/mainSummary by CodeRabbit
Release Notes
New Features
OPENSPEC_BINenvironment variable or absolute paths as workaroundsDocumentation