Skip to content

refactor: extract getStdinFlags helper for Windows stdin transport#395

Merged
pedramamini merged 1 commit intomainfrom
refactor-stdin-flags
Feb 17, 2026
Merged

refactor: extract getStdinFlags helper for Windows stdin transport#395
pedramamini merged 1 commit intomainfrom
refactor-stdin-flags

Conversation

@pedramamini
Copy link
Copy Markdown
Collaborator

Summary

Changes

  • New: src/renderer/utils/spawnHelpers.ts — single source of truth for sendPromptViaStdin / sendPromptViaStdinRaw computation
  • Updated: useAgentExecution.ts, useInputProcessing.ts, inlineWizardConversation.ts — replaced 4 inline copies with getStdinFlags() calls
  • New: src/__tests__/renderer/utils/spawnHelpers.test.ts — 5 tests covering all flag combinations

Context

PR #376 added stdin transport flags to the batch spawn paths in useAgentExecution. This brought the total to 4 copies of the same 6-line computation across the renderer. This PR consolidates them and makes SSH exclusion consistent everywhere (the spawner's if-else chain already made this safe, but the explicit guard is clearer).

Test plan

  • npm run lint passes
  • All 71 affected tests pass (spawnHelpers, useAgentExecution, useInputProcessing, inlineWizardConversation)
  • Windows Auto Run behavior unchanged (stdin transport still active for local runs)
  • SSH sessions unaffected (flags correctly disabled)

…ogic

Deduplicates the Windows stdin transport flag computation from 4 call
sites into a single getStdinFlags() utility. Also adds SSH session
exclusion to useInputProcessing and inlineWizardConversation paths
that were previously missing it (the spawner's if-else chain made
this safe, but the explicit guard is clearer).
@claude
Copy link
Copy Markdown

claude bot commented Feb 16, 2026

Code Review - PR #395

Summary

This is an excellent refactoring that consolidates duplicated Windows stdin transport logic into a single, well-tested utility function. The PR successfully addresses code duplication while improving consistency and maintainability.


✅ Strengths

  1. Clean Abstraction

    • The getStdinFlags() utility in src/renderer/utils/spawnHelpers.ts:11-22 provides a single source of truth for stdin transport logic
    • Clear JSDoc explaining the Windows cmd.exe limitation and SSH exclusion rationale
    • Simple, focused interface with explicit input parameters
  2. Comprehensive Test Coverage

    • All 5 test cases cover the complete decision matrix:
      • Non-Windows platforms
      • Windows with/without stream-json support
      • SSH session exclusions (2 test cases)
    • Tests use proper mocking of navigator.platform
  3. Bug Fix: SSH Session Handling

    • Previously, useInputProcessing.ts and inlineWizardConversation.ts were missing SSH session checks
    • Now all 4 call sites consistently exclude SSH sessions from stdin transport
    • This prevents potential conflicts with SSH's dedicated stdin-script handling
  4. Consistent Implementation

    • All refactored sites (useAgentExecution.ts, useInputProcessing.ts, inlineWizardConversation.ts) now follow the same pattern
    • Reduced from ~6 lines per site to a single function call with clear parameters

🔍 Code Quality Observations

Good:

  • ✅ Follows DRY principle effectively
  • ✅ Maintains backward compatibility (behavior unchanged for existing flows)
  • ✅ Clear variable naming (sendPromptViaStdin vs sendPromptViaStdinRaw)
  • ✅ Proper SSH exclusion handling per [[CLAUDE-PATTERNS.md]]

Minor Notes:

  • The navigator.platform check is renderer-specific (correct for this context, but worth noting this utility is renderer-only)
  • Consider future enhancement: platform detection could potentially be abstracted further if main process ever needs similar logic (not needed now)

🔒 Security & Performance

Security: ✅ No concerns

  • Stdin transport is a safer alternative to command-line arguments (avoids shell injection via special characters)
  • SSH exclusion properly maintained

Performance: ✅ No concerns

  • Negligible overhead (simple boolean checks)
  • No new IPC calls or async operations

🧪 Testing

Test Coverage: ✅ Excellent

  • New test file with 5 comprehensive test cases
  • All edge cases covered (platform × SSH × stream-json support)
  • Proper cleanup with afterEach restore

Suggested Test Verification:

npm run test -- src/__tests__/renderer/utils/spawnHelpers.test.ts
npm run lint

📋 Recommendations

Must Do (Before Merge):

  • Verify all existing tests pass (71 tests mentioned in test plan)
  • Confirm npm run lint passes
  • Manual smoke test: Windows Auto Run behavior unchanged
  • Manual smoke test: SSH sessions still work correctly

Nice to Have (Future):

  • Consider adding a comment in CLAUDE-PLATFORM.md referencing this utility for future Windows platform work
  • If more renderer utilities emerge, consider creating a src/renderer/utils/platform.ts module

✅ Verdict

LGTM with approval pending test verification.

This PR demonstrates excellent software engineering:

  • Identifies and eliminates code duplication
  • Adds tests for the new abstraction
  • Fixes previously missed SSH exclusions
  • Maintains clean, readable code

The refactoring is low-risk (pure consolidation) and high-value (improves maintainability and fixes edge cases).

Risk Assessment: 🟢 Low

  • No behavior changes for existing paths
  • New utility is well-tested
  • Follows established patterns per CLAUDE.md

Once tests pass, this is ready to merge.


Review conducted per CLAUDE.md guidelines, focusing on code quality, security, performance, and test coverage.

@pedramamini pedramamini merged commit 8cf3bae into main Feb 17, 2026
1 check passed
@pedramamini pedramamini deleted the refactor-stdin-flags branch March 18, 2026 22:07
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.

1 participant