Skip to content

fix(cli): preserve retry command arguments - #359

Merged
mohanagy merged 2 commits into
developmentfrom
fix/issue-347-posix-retry-quoting
Aug 9, 2026
Merged

fix(cli): preserve retry command arguments#359
mohanagy merged 2 commits into
developmentfrom
fix/issue-347-posix-retry-quoting

Conversation

@mohanagy

@mohanagy mohanagy commented Aug 9, 2026

Copy link
Copy Markdown
Owner

Summary

  • reuse one platform-aware shell-command formatter for setup and diagnostics
  • preserve exact argv through a real POSIX shell, including spaces, apostrophes, and expansion syntax
  • render and label Windows commands for PowerShell, with deterministic warning coverage and a Windows-only PowerShell argv round-trip
  • keep installed-package output contracts platform-aware

Validation

  • npm test -- tests/cli-error-output.test.ts tests/setup-completion.test.ts tests/mcp-wrapper.test.ts tests/package-contract.test.ts (145 passed; 1 Windows-only skip locally)
  • npm test (1,831 passed; 33 platform skips)
  • npm run lint
  • npm run typecheck

Closes #347

Summary by CodeRabbit

  • Bug Fixes
    • Corrected shell argument escaping for apostrophes in generated CLI commands.
    • Ensured retry commands preserve configuration and profile arguments across POSIX shells and PowerShell.
    • Improved CLI error and startup-warning guidance with platform-appropriate retry instructions.
    • Standardized command wording for diagnostic and resource subscription capability warnings.
    • Improved reliability of generated retry commands on Windows and non-Windows platforms.

@coderabbitai

coderabbitai Bot commented Aug 9, 2026

Copy link
Copy Markdown

Review Change Stack

📝 Walkthrough

Walkthrough

The change centralizes shell argument quoting and command instructions. CLI retry output, MCP warnings, startup diagnostics, and setup completion now use platform-specific formatting. Tests verify POSIX and PowerShell escaping and argument round-trips.

Changes

Diagnostic command formatting

Layer / File(s) Summary
Shared shell command helpers
src/utils/shell-command.ts, src/upstream/startup-diagnostic.ts, src/setup/setup-completion.ts
The shared utilities quote arguments for POSIX shells and PowerShell. Startup diagnostics and setup completion use these helpers.
CLI and MCP warning instructions
src/cli/error-output.ts, src/mcp/server/miftah-server.ts
CLI retry output and MCP resource subscription warnings use shell-aware command instructions.
Platform-specific command validation
tests/cli-error-output.test.ts, tests/mcp-wrapper.test.ts, tests/package-contract.test.ts
Tests verify apostrophe escaping, POSIX and PowerShell command round-trips, and platform-specific retry prefixes.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Possibly related PRs

  • mohanagy/miftah#348: Modifies shared startup diagnostics and shell-safe retry command formatting.

Poem

A rabbit checks each quoted trail,
Through POSIX shell and PowerShell.
Paths and profiles keep their shape,
While retry commands make escape.
“Hop!” says the test, “the args prevail.”


Caution

Pre-merge checks failed

Please resolve all errors before merging. Addressing warnings is optional.

  • Ignore

❌ Failed checks (2 errors, 1 warning)

Check name Status Explanation Resolution
Linked Issues check ❌ Error The changes address platform-specific retry commands and regression tests [#347], but do not show bounded redaction, structured metadata, JSON fields, or uvx pinning support. Add or reference implementation and tests for bounded redacted diagnostics, structured metadata, JSON fields, uvx recognition, and the remaining issue acceptance criteria.
Docstring Coverage ❌ Error Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
Description check ⚠️ Warning The description includes a focused summary and validation results but omits the required Security impact section and several validation checklist items. Add the Security impact section and document each required validation command, checklist result, and applicable documentation, dependency, fixture, and vulnerability review.
✅ Passed checks (2 passed)
Check name Status Explanation
Out of Scope Changes check ✅ Passed The utility, diagnostic, completion, server-warning, and test changes all support platform-aware retry command formatting within the stated objectives.
Title check ✅ Passed The title clearly summarizes the main change: preserving retry command arguments in the CLI.
✨ Finishing Touches 💡 1
📝 Generate docstrings 💡
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch fix/issue-347-posix-retry-quoting

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

@mohanagy
mohanagy merged commit 898d6d9 into development Aug 9, 2026
12 checks passed
@mohanagy
mohanagy deleted the fix/issue-347-posix-retry-quoting branch August 9, 2026 16:00
@mohanagy mohanagy mentioned this pull request Aug 9, 2026
10 tasks
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.

Make upstream initialization failures actionable without exposing secrets

1 participant