Skip to content

fix: delete command must fail loudly, not silently no-op, without -y in non-TTY mode#7

Merged
hummer98 merged 1 commit into
masterfrom
claude/fix-issue-4-delete-non-interactive-y-flag
Jul 8, 2026
Merged

fix: delete command must fail loudly, not silently no-op, without -y in non-TTY mode#7
hummer98 merged 1 commit into
masterfrom
claude/fix-issue-4-delete-non-interactive-y-flag

Conversation

@hummer98

@hummer98 hummer98 commented Jul 8, 2026

Copy link
Copy Markdown
Owner

Summary

  • In non-interactive execution (e.g. execSync, piped stdin — no TTY), running firex delete <path> [--recursive] without -y/--yes previously exited 0 with no output and no deletion, because the confirmation prompt couldn't be displayed and the command silently no-op'd.
  • This caused real damage downstream: a test harness's cleanup step omitted -y, silently did nothing on every run, and stale data accumulated, producing hard-to-diagnose false failures across multiple debugging sessions.
  • Fix: DeleteCommand#run now checks !skipConfirm && !process.stdout.isTTY up front and fails fast with [ERROR] 非対話実行では -y が必要です(確認プロンプトを表示できません) (exit code 1, validation category) instead of proceeding into the no-op confirmation path.
  • The confirmation-prompt safety mechanism itself is unchanged/correct — this only closes the silent-no-op gap when the prompt cannot be shown.
  • Checked src/mcp/tools/delete.ts (MCP tool wrapper): it always auto-confirms (async () => true) regardless of TTY, so it was never affected by this bug and was left unchanged.

Test plan

  • Added tests in src/commands/delete.test.ts covering: non-TTY + no -y → exits with oclif.exit === 1 and the error message, and never reaches initialize() (i.e. no deletion path is entered); non-TTY + -y → proceeds past the guard; TTY + no -y → proceeds past the guard (existing interactive behavior unaffected).
  • npm run typecheck — passes
  • npm test — 1042/1042 tests pass (59 files)
  • npm run build — passes
  • Manual smoke test of the built CLI (node bin/run.js delete "users/doc1" < /dev/null) — exits 1 with the expected error message and no Firestore call attempted; with -y it proceeds past the guard to the (separate, expected) auth error.
  • npm run lintcould not run: this repo currently has no ESLint config file at all (eslint src --ext .ts fails with "ESLint couldn't find a configuration file"). Verified this is pre-existing and unrelated to this change by stashing my diff and running npm run lint on a clean checkout of master (d23a3a5) — same failure. This matches the CI history: the last 5 runs of .github/workflows/ci.yml on master (including the 0.9.0 and 0.9.1 release commits) all fail at the npm ci step in every job (lint/typecheck/test/build) with npm error Invalid: lock file's picomatch@2.3.2 does not satisfy picomatch@4.0.4 — a pre-existing lockfile/CI infra issue unrelated to this fix.

Fixes #4

@hummer98

hummer98 commented Jul 8, 2026

Copy link
Copy Markdown
Owner Author

CI status update: all jobs (lint, typecheck, test, integration-test, build) fail at the npm ci step within ~10s, before any of this PR's code runs:

npm error Invalid: lock file's picomatch@2.3.2 does not satisfy picomatch@4.0.4

This is pre-existing and unrelated to this change. Verified by:

  • Stashing this PR's diff and running npm run lint/npm ci against a clean checkout of master (d23a3a5, the 0.9.1 tag) — same failure.
  • Checking CI history on master: the last 5 runs (spanning the 0.9.0 and 0.9.1 release commits, ~2 months) all fail identically at npm ci.
  • The repo also currently has no ESLint config file at all (eslint.config.* / .eslintrc*), so npm run lint cannot succeed even once dependencies install.

This PR's actual change was fully verified locally instead: npm run typecheck (clean), npm test (1042/1042 passing, including 3 new tests for this fix), npm run build (clean), and a manual smoke test of the built CLI confirming the new non-TTY guard fires correctly (node bin/run.js delete "users/doc1" < /dev/null → exit 1 with the expected error, no Firestore call attempted; adding -y proceeds past the guard).

Leaving this PR open pending a decision on the lockfile/ESLint-config issue (likely worth its own fix) rather than merging with red CI.

…n-TTY mode

Previously, running `firex delete` without --yes in a non-interactive
context (e.g. execSync, piped input) exited 0 with no output and no
deletion, because the confirmation prompt could not be displayed. This
caused a downstream test harness to silently skip cleanup on every
run, accumulating stale data and producing hard-to-diagnose false
failures.

Add a guard at the top of DeleteCommand#run: when stdout is not a TTY
and --yes was not passed, print a clear error and exit non-zero
instead of proceeding into the no-op confirmation path. The MCP
firestore_delete tool already auto-confirms unconditionally and is
unaffected by this bug, so it was left unchanged.

Fixes #4

Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
@hummer98 hummer98 force-pushed the claude/fix-issue-4-delete-non-interactive-y-flag branch from 369005a to 3db1a07 Compare July 8, 2026 11:16
@hummer98 hummer98 merged commit 8547c45 into master Jul 8, 2026
1 of 7 checks passed
@hummer98 hummer98 deleted the claude/fix-issue-4-delete-non-interactive-y-flag branch July 8, 2026 11:16
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.

delete: 非対話実行で -y を省くと何も削除せずに正常終了(exit 0)する

1 participant