-
Notifications
You must be signed in to change notification settings - Fork 1.1k
fix(cli): prevent hang in pre-commit hooks by using dynamic imports #380
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
Fixes #367 The CLI was hanging when run as a pre-commit hook because @inquirer/prompts was statically imported at module load time. Even when prompts were never called (e.g., `openspec validate --specs --no-interactive`), the import itself could set up stdin references that prevented clean process exit when stdin was piped. Changes: - Convert all static `@inquirer/prompts` imports to dynamic imports - Dynamically import `InitCommand` (which uses `@inquirer/core`) - Update `isInteractive()` to accept options object with both `noInteractive` and Commander's negated `interactive` property - Handle empty validation queue with proper exit code Now when running in non-interactive mode, the inquirer modules are never loaded, allowing the process to exit cleanly after completion.
WalkthroughThis pull request refactors how inquirer prompt modules are imported and how interactive mode is detected across the codebase. Static top-level imports of Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Areas requiring extra attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🧰 Additional context used🧬 Code graph analysis (4)src/commands/change.ts (2)
src/commands/spec.ts (2)
src/commands/show.ts (1)
src/commands/validate.ts (1)
🔇 Additional comments (17)
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 |
* fix(global-config): respect XDG_CONFIG_HOME on all platforms (Fission-AI#378) Prioritize XDG_CONFIG_HOME on Windows to fix test environment overrides. Previously, Windows would always use APPDATA regardless of XDG_CONFIG_HOME, causing tests to fail. Now XDG_CONFIG_HOME is checked first on all platforms before falling back to platform-specific defaults. Also update the Windows APPDATA test to explicitly clear XDG_CONFIG_HOME when testing the fallback behavior. * fix(cli): prevent hang in pre-commit hooks by using dynamic imports (Fission-AI#380) Fixes Fission-AI#367 The CLI was hanging when run as a pre-commit hook because @inquirer/prompts was statically imported at module load time. Even when prompts were never called (e.g., `openspec validate --specs --no-interactive`), the import itself could set up stdin references that prevented clean process exit when stdin was piped. Changes: - Convert all static `@inquirer/prompts` imports to dynamic imports - Dynamically import `InitCommand` (which uses `@inquirer/core`) - Update `isInteractive()` to accept options object with both `noInteractive` and Commander's negated `interactive` property - Handle empty validation queue with proper exit code Now when running in non-interactive mode, the inquirer modules are never loaded, allowing the process to exit cleanly after completion. * feat(cli): add plx command alias and rebrand as OpenSplx (OpenSplx-#1) - Add `plx` as an alias command alongside `openspec` - Create OpenSplx pixel art logo assets (light/dark themes) - Update README with fork notice and Quick Start section - Make CLI command name dynamic based on invocation - Update completion system to support both command names - Add command-name utility for detecting invoked command * feat(cli): add external issue tracking support - Add YAML frontmatter parsing for tracked issues in proposal.md - Display issue identifiers in `openspec list` output - Include trackedIssues in `openspec show --json` output - Report tracked issues when archiving changes - Add External Issue Tracking section to AGENTS.md template - Add TrackedIssue schema and type exports * fix(cli): address PR review feedback and archive external issue tracking - Fix list.ts alignment to include issue display in width calculation - Fix command-name.ts to handle Windows extensions and cross-platform paths - Fix postinstall.js to install completions for both openspec and plx - Fix change.ts issue display format (after title in long format) - Add comprehensive unit tests for all fixes - Archive add-external-issue-tracking change with spec updates --------- Co-authored-by: Tabish Bidiwale <[email protected]>
* fix(global-config): respect XDG_CONFIG_HOME on all platforms (Fission-AI#378) Prioritize XDG_CONFIG_HOME on Windows to fix test environment overrides. Previously, Windows would always use APPDATA regardless of XDG_CONFIG_HOME, causing tests to fail. Now XDG_CONFIG_HOME is checked first on all platforms before falling back to platform-specific defaults. Also update the Windows APPDATA test to explicitly clear XDG_CONFIG_HOME when testing the fallback behavior. * fix(cli): prevent hang in pre-commit hooks by using dynamic imports (Fission-AI#380) Fixes Fission-AI#367 The CLI was hanging when run as a pre-commit hook because @inquirer/prompts was statically imported at module load time. Even when prompts were never called (e.g., `openspec validate --specs --no-interactive`), the import itself could set up stdin references that prevented clean process exit when stdin was piped. Changes: - Convert all static `@inquirer/prompts` imports to dynamic imports - Dynamically import `InitCommand` (which uses `@inquirer/core`) - Update `isInteractive()` to accept options object with both `noInteractive` and Commander's negated `interactive` property - Handle empty validation queue with proper exit code Now when running in non-interactive mode, the inquirer modules are never loaded, allowing the process to exit cleanly after completion. * feat(cli): add plx command alias and rebrand as OpenSplx (OpenSplx-#1) - Add `plx` as an alias command alongside `openspec` - Create OpenSplx pixel art logo assets (light/dark themes) - Update README with fork notice and Quick Start section - Make CLI command name dynamic based on invocation - Update completion system to support both command names - Add command-name utility for detecting invoked command * feat(cli): add external issue tracking support - Add YAML frontmatter parsing for tracked issues in proposal.md - Display issue identifiers in `openspec list` output - Include trackedIssues in `openspec show --json` output - Report tracked issues when archiving changes - Add External Issue Tracking section to AGENTS.md template - Add TrackedIssue schema and type exports * fix(cli): address PR review feedback and archive external issue tracking - Fix list.ts alignment to include issue display in width calculation - Fix command-name.ts to handle Windows extensions and cross-platform paths - Fix postinstall.js to install completions for both openspec and plx - Fix change.ts issue display format (after title in long format) - Add comprehensive unit tests for all fixes - Archive add-external-issue-tracking change with spec updates --------- Co-authored-by: Tabish Bidiwale <[email protected]>
The config command (added in #382) reintroduced the pre-commit hook hang issue that was fixed in #380. The static import of @inquirer/prompts at module load time causes stdin event listeners to be registered even when running non-interactive commands, preventing clean process exit when stdin is piped (as pre-commit does). Convert the static import to a dynamic import that only loads inquirer when the `config reset` command is actually used interactively. Fixes #367
…392) * fix(cli): use dynamic import for @inquirer/prompts in config command The config command (added in #382) reintroduced the pre-commit hook hang issue that was fixed in #380. The static import of @inquirer/prompts at module load time causes stdin event listeners to be registered even when running non-interactive commands, preventing clean process exit when stdin is piped (as pre-commit does). Convert the static import to a dynamic import that only loads inquirer when the `config reset` command is actually used interactively. Fixes #367 * chore: add ESLint with no-restricted-imports rule for @InQuirer Add ESLint configuration that prevents static imports of @inquirer/* modules. This prevents future regressions of the pre-commit hook hang issue fixed in this PR. The rule shows a helpful error message pointing to issue #367 for context. init.ts is exempted since it's already dynamically imported from the CLI. * ci: add ESLint step to lint job Run `pnpm lint` in CI to enforce the no-restricted-imports rule that prevents static @InQuirer imports.
* feat(cli): add openspec config command for global configuration management (Fission-AI#382) * feat(cli): add openspec config command for global configuration management Implements the `openspec config` command with subcommands: - `path`: Show config file location - `list [--json]`: Show all current settings - `get <key>`: Get a specific value (raw output for scripting) - `set <key> <value> [--string]`: Set a value with auto type coercion - `unset <key>`: Remove a key (revert to default) - `reset --all [-y]`: Reset configuration to defaults - `edit`: Open config in $EDITOR/$VISUAL Key features: - Dot notation for nested key access (e.g., featureFlags.someFlag) - Auto type coercion (true/false → boolean, numbers → number) - --string flag to force string storage - Zod schema validation with unknown field passthrough - Reserved --scope flag for future project-local config - Windows-compatible editor spawning with proper path quoting - Shell completion registry integration * test(config): add additional unit tests for validation and coercion - Add tests for unknown fields with various types - Add test to verify error message path for featureFlags - Add test for number values rejection in featureFlags - Add config set simulation tests to verify full coerce → set → validate flow * fix(config): avoid shell parsing in config edit to handle paths with spaces Use spawn with shell: false and pass configPath as an argument instead of building a shell command string. This correctly handles spaces in both the EDITOR path and config file path on all platforms. * chore(openspec): archive add-config-command and create cli-config spec Move completed change to archive and apply spec deltas to create the cli-config specification documenting the config command interface. * Validate config keys on set * Add changeset for config command and shell completions (Fission-AI#388) * chore(release): version packages (Fission-AI#389) * Version Packages * chore: trigger CI --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tabish Bidiwale <[email protected]> * feat(ci): migrate to npm OIDC trusted publishing (Fission-AI#390) Replace classic npm token authentication with OIDC trusted publishing: - Add `id-token: write` permission for OIDC token generation - Upgrade to Node 24 (includes npm 11.5.1+ required for OIDC) - Remove NPM_TOKEN/NODE_AUTH_TOKEN env vars (OIDC replaces them) This eliminates the need for rotating npm access tokens and provides cryptographically verified publisher identity with automatic provenance attestation. Requires configuring trusted publisher on npmjs.com: - Organization: Fission-AI - Repository: OpenSpec - Workflow: release-prepare.yml * fix(cli): use dynamic import for @inquirer/prompts in config command (Fission-AI#392) * fix(cli): use dynamic import for @inquirer/prompts in config command The config command (added in Fission-AI#382) reintroduced the pre-commit hook hang issue that was fixed in Fission-AI#380. The static import of @inquirer/prompts at module load time causes stdin event listeners to be registered even when running non-interactive commands, preventing clean process exit when stdin is piped (as pre-commit does). Convert the static import to a dynamic import that only loads inquirer when the `config reset` command is actually used interactively. Fixes Fission-AI#367 * chore: add ESLint with no-restricted-imports rule for @InQuirer Add ESLint configuration that prevents static imports of @inquirer/* modules. This prevents future regressions of the pre-commit hook hang issue fixed in this PR. The rule shows a helpful error message pointing to issue Fission-AI#367 for context. init.ts is exempted since it's already dynamically imported from the CLI. * ci: add ESLint step to lint job Run `pnpm lint` in CI to enforce the no-restricted-imports rule that prevents static @InQuirer imports. * Add changeset for config command dynamic import fix (Fission-AI#393) * chore(release): version packages (Fission-AI#394) * Version Packages * chore: trigger CI --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tabish Bidiwale <[email protected]> * fix(cli): respect --no-interactive flag in validate command (Fission-AI#395) * fix(cli): respect --no-interactive flag in validate command The validate command's spinner was starting regardless of the --no-interactive flag, causing hangs in pre-commit hooks. Changes: - Pass noInteractive option to runBulkValidation - Handle Commander.js --no-* flag syntax (sets interactive=false) - Only start ora spinner when in interactive mode - Add CI environment variable check to isInteractive() for industry standard compliance * test: add unit tests for interactive utilities and CLI flag - Export resolveNoInteractive() helper for reuse - Add InteractiveOptions type export for testing - Refactor validate.ts to use resolveNoInteractive() - Add 17 unit tests for isInteractive() and resolveNoInteractive() - Add CLI integration test for --no-interactive flag This prevents future regressions where Commander.js --no-* flag parsing is not properly handled. * Add changeset for --no-interactive flag fix (Fission-AI#396) * chore(release): version packages (Fission-AI#397) * Version Packages * chore: trigger CI --------- Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tabish Bidiwale <[email protected]> * docs: add artifact POC analysis document (Fission-AI#398) Add internal documentation for the artifact-based approach to OpenSpec core. This document outlines design decisions, terminology, and the philosophy behind treating dependencies as enablers rather than gates. * fix(archive): allow REMOVED requirements when creating new spec files (Fission-AI#403) (Fission-AI#404) When creating a new spec file, REMOVED requirements are now ignored with a warning instead of causing archive to fail. This enables refactoring scenarios where old fields are removed while documenting a capability for the first time. Fixes Fission-AI#403 --------- Co-authored-by: Tabish Bidiwale <[email protected]> Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> Co-authored-by: Tabish Bidiwale <[email protected]> Co-authored-by: Eunsong-Park <[email protected]>
…ission-AI#392) * fix(cli): use dynamic import for @inquirer/prompts in config command The config command (added in Fission-AI#382) reintroduced the pre-commit hook hang issue that was fixed in Fission-AI#380. The static import of @inquirer/prompts at module load time causes stdin event listeners to be registered even when running non-interactive commands, preventing clean process exit when stdin is piped (as pre-commit does). Convert the static import to a dynamic import that only loads inquirer when the `config reset` command is actually used interactively. Fixes Fission-AI#367 * chore: add ESLint with no-restricted-imports rule for @InQuirer Add ESLint configuration that prevents static imports of @inquirer/* modules. This prevents future regressions of the pre-commit hook hang issue fixed in this PR. The rule shows a helpful error message pointing to issue Fission-AI#367 for context. init.ts is exempted since it's already dynamically imported from the CLI. * ci: add ESLint step to lint job Run `pnpm lint` in CI to enforce the no-restricted-imports rule that prevents static @InQuirer imports.
…ission-AI#392) * fix(cli): use dynamic import for @inquirer/prompts in config command The config command (added in Fission-AI#382) reintroduced the pre-commit hook hang issue that was fixed in Fission-AI#380. The static import of @inquirer/prompts at module load time causes stdin event listeners to be registered even when running non-interactive commands, preventing clean process exit when stdin is piped (as pre-commit does). Convert the static import to a dynamic import that only loads inquirer when the `config reset` command is actually used interactively. Fixes Fission-AI#367 * chore: add ESLint with no-restricted-imports rule for @InQuirer Add ESLint configuration that prevents static imports of @inquirer/* modules. This prevents future regressions of the pre-commit hook hang issue fixed in this PR. The rule shows a helpful error message pointing to issue Fission-AI#367 for context. init.ts is exempted since it's already dynamically imported from the CLI. * ci: add ESLint step to lint job Run `pnpm lint` in CI to enforce the no-restricted-imports rule that prevents static @InQuirer imports.
…ission-AI#380) Fixes Fission-AI#367 The CLI was hanging when run as a pre-commit hook because @inquirer/prompts was statically imported at module load time. Even when prompts were never called (e.g., `openspec validate --specs --no-interactive`), the import itself could set up stdin references that prevented clean process exit when stdin was piped. Changes: - Convert all static `@inquirer/prompts` imports to dynamic imports - Dynamically import `InitCommand` (which uses `@inquirer/core`) - Update `isInteractive()` to accept options object with both `noInteractive` and Commander's negated `interactive` property - Handle empty validation queue with proper exit code Now when running in non-interactive mode, the inquirer modules are never loaded, allowing the process to exit cleanly after completion.
…ission-AI#392) * fix(cli): use dynamic import for @inquirer/prompts in config command The config command (added in Fission-AI#382) reintroduced the pre-commit hook hang issue that was fixed in Fission-AI#380. The static import of @inquirer/prompts at module load time causes stdin event listeners to be registered even when running non-interactive commands, preventing clean process exit when stdin is piped (as pre-commit does). Convert the static import to a dynamic import that only loads inquirer when the `config reset` command is actually used interactively. Fixes Fission-AI#367 * chore: add ESLint with no-restricted-imports rule for @InQuirer Add ESLint configuration that prevents static imports of @inquirer/* modules. This prevents future regressions of the pre-commit hook hang issue fixed in this PR. The rule shows a helpful error message pointing to issue Fission-AI#367 for context. init.ts is exempted since it's already dynamically imported from the CLI. * ci: add ESLint step to lint job Run `pnpm lint` in CI to enforce the no-restricted-imports rule that prevents static @InQuirer imports.
Summary
Fixes #367
@inquirer/promptsimports to dynamic imports, loaded only when interactive prompts are actually neededInitCommand(which uses@inquirer/core) to prevent loading at startupisInteractive()to accept options object supporting bothnoInteractiveand Commander's negatedinteractivepropertyRoot Cause
The CLI hung when run as a pre-commit hook because
@inquirer/promptswas statically imported at module load time. Even when prompts were never called (e.g.,openspec validate --specs --no-interactive), the import could set up stdin references that prevented clean process exit when stdin was piped.The workaround discovered by @greenkiwi (
0<&-to close stdin) confirmed the issue was stdin-related.Test plan
openspec validate --specs --strict --no-interactivein a pre-commit hook - should complete without hanging🤖 Generated with Claude Code
Summary by CodeRabbit
Release Notes
Refactor
New Features
Bug Fixes
✏️ Tip: You can customize this high-level summary in your review settings.