feat: add shellInteractive flag for PTY-based shell commands#326
Merged
feat: add shellInteractive flag for PTY-based shell commands#326
Conversation
- Add `shellInteractive` YAML key and `interactive` parameter to shellCommand install actions, enabling commands that need terminal access (e.g. sudo prompts) to run via forkpty() with a real PTY - Thread interactive flag through ShellRunning protocol, all sync strategies, PackInstaller, and ManifestBuilder export - Show "may prompt for your password" message before interactive commands; suppress empty stderr warnings for interactive failures
- Document shellInteractive in CLAUDE.md, techpack-schema.md, creating-tech-packs.md, and architecture.md - Add manifest decode tests for interactive flag (verbose + shorthand) - Add adapter passthrough test for interactive flag - Add lifecycle integration tests for shellCommand components
There was a problem hiding this comment.
Pull request overview
Adds an interactive flag to shell-based install actions so tech pack shell components can optionally run inside a PTY (via forkpty) to support terminal-driven prompts such as sudo, along with YAML/docs/test updates.
Changes:
- Extend
.shellCommandactions (internal + external/YAML) withinteractive: Bool = falseand addshellInteractiveYAML shorthand. - Thread the interactive flag through sync/install/export paths and implement PTY-backed execution in
ShellRunner. - Update docs and add/adjust tests for decoding and lifecycle behavior.
Reviewed changes
Copilot reviewed 17 out of 17 changed files in this pull request and generated 6 comments.
Show a summary per file
| File | Description |
|---|---|
| Tests/MCSTests/TestHelpers.swift | Updates MockShellRunner to match the expanded ShellRunning API. |
| Tests/MCSTests/LifecycleIntegrationTests.swift | Adds lifecycle coverage for shellCommand components and the new interactive flag. |
| Tests/MCSTests/ExternalPackManifestTests.swift | Adds/updates YAML decode tests for interactive and shellInteractive. |
| Tests/MCSTests/ExternalPackAdapterTests.swift | Verifies adapter passthrough of the interactive flag. |
| Sources/mcs/TechPack/Component.swift | Extends ComponentInstallAction.shellCommand with interactive. |
| Sources/mcs/Sync/ProjectSyncStrategy.swift | Threads interactive execution through project sync shell commands. |
| Sources/mcs/Sync/PackInstaller.swift | Threads interactive execution through dependency installer shell commands. |
| Sources/mcs/Sync/GlobalSyncStrategy.swift | Adds interactive messaging and routes interactive shell commands through the runner. |
| Sources/mcs/ExternalPack/PackTrustManager.swift | Updates trust manager pattern-matching for the new shellCommand associated value shape. |
| Sources/mcs/ExternalPack/ExternalPackManifest.swift | Adds shellInteractive shorthand + verbose interactive decoding/encoding for shellCommand. |
| Sources/mcs/ExternalPack/ExternalPackAdapter.swift | Maps external shellCommand interactive flag into internal component actions. |
| Sources/mcs/Export/ManifestBuilder.swift | Exports shellInteractive: true when an action is interactive. |
| Sources/mcs/Core/ShellRunner.swift | Implements PTY-backed interactive execution and updates ShellRunning API. |
| docs/techpack-schema.md | Documents shellInteractive and the verbose interactive field. |
| docs/creating-tech-packs.md | Adds authoring guidance/examples for shellInteractive: true. |
| docs/architecture.md | Updates architecture docs for the new shellCommand signature. |
| CLAUDE.md | Updates external pack shorthand documentation to mention shellInteractive. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Unify interactive failure messaging: all three call sites now show "failed (see output above)" instead of silent suppression or misleading "requires manual installation" - Add writeAll helper to handle partial writes and EINTR in I/O bridge - Handle stdin EOF by removing stdin from select set - Re-enable ISIG in raw mode so Ctrl-C/Ctrl-Z can interrupt stuck commands - Add chdir/execve failure diagnostics in child process - Retry waitpid on EINTR, retry tcsetattr on failure - Fix misleading ShorthandKeys comment and stale docstrings
- Replace select() + fd_set helpers with poll() — removes 25 lines of manual bit manipulation and eliminates FD_SETSIZE bounds concern - Fix fork safety: use only async-signal-safe C calls in child process (no Swift String interpolation or ARC after forkpty) - Record interactive flag in MockShellRunner RunCall/ShellCall structs - Remove unnecessary inout on writeAll buf parameter - Simplify terminal restore to single TCSADRAIN call
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 17 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…branch - Pre-convert workingDirectory to a C string before forkpty() so the child process doesn't invoke Swift's String-to-CString bridge - Remove unreachable else-if POLLHUP branch (already covered by the POLLIN|POLLHUP check above)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds a
shellInteractiveYAML key for tech packshell:components that need terminal access (e.g. install scripts requiringsudo). When enabled, mcs usesforkpty()to allocate a real pseudo-terminal so commands can prompt for passwords securely with proper echo control.Changes
interactive: Boolassociated value toComponentInstallAction.shellCommandandExternalInstallAction.shellCommand(defaults tofalse— fully backward-compatible)shellInteractiveshorthand key toExternalComponentDefinitionfor YAML packsinteractivethroughShellRunningprotocol, all sync strategies,PackInstaller, andManifestBuilderexportrunInteractive()toShellRunnerusingforkpty()+select()I/O bridge with raw-mode terminal, EINTR handling, properWIFEXITED/WIFSIGNALEDexit status, and defer-based terminal restorationTest plan
swift testpasses locally (978 tests)swiftformat --lint .andswiftlintpass without violationsmcs sync --globalwith a pack usingshellInteractive: true— sudo prompt works, password is hidden, install completes successfullyinteractive: false)mcs doctordoes not trigger side effects from interactive componentsChecklist for engine changes
.shellCommandcomponents havesupplementaryDoctorChecksdefined (deriveDoctorCheck()returnsnilfor shell actions)CLAUDE.md,docs/,techpack.yamlschema inExternalPackManifest.swift)Tech pack YAML syntax