Skip to content

Conversation

@TabishB
Copy link
Contributor

@TabishB TabishB commented Dec 22, 2025

Summary

Root Cause

The config command (added in #382) reintroduced the pre-commit hook hang issue that was originally 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 like:

openspec validate --specs --strict --no-interactive

When stdin is piped (as pre-commit does), these listeners prevent the Node.js process from exiting cleanly - even when --no-interactive is passed and prompts are never actually called.

Fix

  1. Dynamic import - Convert the static import to a dynamic import that only loads inquirer when the config reset command is actually used interactively:
- import { confirm } from '@inquirer/prompts';

  if (!options.yes) {
+   const { confirm } = await import('@inquirer/prompts');
    const confirmed = await confirm({
  1. ESLint rule - Add no-restricted-imports rule that prevents static @inquirer/* imports with a helpful error message:
error  '@inquirer/prompts' import is restricted from being used by a pattern. 
       Use dynamic import() for @inquirer modules to prevent pre-commit hook hangs. See #367.

Test plan

  • Build passes
  • All 488 tests pass
  • pnpm lint passes
  • Verified ESLint rule catches static @inquirer imports
  • Tested piped stdin scenario: echo "" | node dist/cli/index.js validate --specs --strict --no-interactive completes without hanging

Fixes #367

🤖 Generated with Claude Code

Summary by CodeRabbit

  • Chores
    • Established ESLint configuration with TypeScript support for enhanced code quality.
    • Integrated automated linting into CI pipeline to enforce code standards before builds.

✏️ Tip: You can customize this high-level summary in your review settings.

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
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Dec 22, 2025

Caution

Review failed

The pull request is closed.

Walkthrough

Replaces a top-level interactive import with a conditional dynamic import in the config reset path, adds ESLint configuration and dependencies, and inserts a CI lint step and package.json lint script to run ESLint during CI and development.

Changes

Cohort / File(s) Change Summary
Interactive import optimization
src/commands/config.ts
Removed top-level confirm import from @inquirer/prompts; now dynamically imports confirm inside the reset command path and only when --yes is not provided.
CI workflow
.github/workflows/ci.yml
Added a Lint step to the existing CI lint job to run pnpm lint (executes after type-check).
ESLint configuration
eslint.config.js
Added new TypeScript-aware ESLint config (default export) with rules, an ignore list, and a no-restricted-imports rule discouraging static @inquirer/* imports (override for src/core/init.ts).
Package metadata & scripts
package.json
Added lint script (eslint src/) and devDependencies for eslint and typescript-eslint.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Pre-merge checks and finishing touches

✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately describes the main change: converting @inquirer/prompts from static to dynamic import in the config command to fix a hanging pre-commit hook issue.
Linked Issues check ✅ Passed The PR successfully addresses issue #367 by replacing the static import that caused stdin listeners to register at module load time with a dynamic import that only loads the prompts module when the interactive reset command runs.
Out of Scope Changes check ✅ Passed All changes are directly scoped to the objective of converting the static @inquirer/prompts import to dynamic in the config command to prevent stdin listener registration.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.

📜 Recent review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between ce79ec0 and 313afbb.

⛔ Files ignored due to path filters (1)
  • pnpm-lock.yaml is excluded by !**/pnpm-lock.yaml
📒 Files selected for processing (3)
  • .github/workflows/ci.yml
  • eslint.config.js
  • package.json

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

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.
Run `pnpm lint` in CI to enforce the no-restricted-imports rule
that prevents static @InQuirer imports.
@TabishB TabishB merged commit 6d84924 into main Dec 22, 2025
6 of 7 checks passed
appboypov added a commit to appboypov/OpenSplx that referenced this pull request Dec 24, 2025
* 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 <tabishbidiwale@gmail.com>

* 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 <tabishbidiwale@gmail.com>

* 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 <tabishbidiwale@gmail.com>

* 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 <30385142+TabishB@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: Tabish Bidiwale <tabishbidiwale@gmail.com>
Co-authored-by: Eunsong-Park <111448985+smileeunsong@users.noreply.github.com>
appboypov pushed a commit to appboypov/OpenSplx that referenced this pull request Dec 24, 2025
…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.
appboypov pushed a commit to appboypov/OpenSplx that referenced this pull request Dec 24, 2025
…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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

openspec validate hangs when runnings as pre-commit hook

2 participants