fix(cli): switch copilot and amp providers to stdin prompt delivery - #76
Open
Caldalis wants to merge 2 commits into
Open
fix(cli): switch copilot and amp providers to stdin prompt delivery#76Caldalis wants to merge 2 commits into
Caldalis wants to merge 2 commits into
Conversation
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.
copilotandampwere the last two CLI providers passing the prompt through argv (copilot -p "<prompt>",amp -x "<prompt>"). The prompt carries the full task context JSON and the output JSON schema — peer context alone can reach60 KB — so it can exceed the OS per-argument limit (128 KB on Linux). Same class of failure as the opencode fix in #65.
Both CLIs accept the prompt on stdin, so this routes it there.
The two subtleties
copilot:
-phas to be removed entirely, not emptied. Copilot resolves the prompt asopts.prompt ? opts.prompt : readStdin(), so any-pshort-circuits the stdin path. Its own error text names both sources:amp:
-xstays. It selects execute mode rather than being a prompt flag — bare-xreads the prompt from stdin.-x, --execute [message]takes an optional value, so I checked that it does not swallow the following--flagas its message (see verification below).With that, no provider puts the prompt in argv, so
usesStdinPromptand the now-unusedpromptparameter ofbuildBaseArgsare removed — the invariant is enforced by the signature instead of by convention.Why the EPIPE fix is in this PR
Sending a large prompt to these two providers exposed a latent crash in
runCommand. A CLI that exits before draining stdin — failing auth is the common case — makes the write fail withEPIPE, which Node raises as an'error'event onchild.stdinthat is fatal when unhandled.Empty stdin never opened that window, so copilot and amp were previously immune while the other seven providers were not:
""(copilot/amp before)Without it, this PR would turn a clear
No authentication information foundinto a process crash for any misconfigured copilot user. Ignoring the write error preserves the diagnostic from theclosehandler and turns a whole-run crash back into a single failed agent. It also fixes the pre-existing exposure for the other seven providers.Happy to split this into its own commit or PR if you'd prefer — the only constraint is that it must not land after the stdin switch.
Tests
cliTypederived fromCliProviderSchemaand fails if the task payload appears in argv, so a newly added provider has to decide its prompt delivery deliberately rather than inheriting a defaultBoth new tests were confirmed to fail when their fix is reverted — the argv one reports
expected [ 'copilot' ] to deeply equal [], and the EPIPE one surfacesUnhandled Errors: write EPIPEwith a non-zero exit.Verification
Both CLIs were installed locally and probed directly:
--helpdocuments stdin prompts ("either as argument, or via stdin");amp -x --definitely-not-a-flagstill reportsunknown option, proving-xdoes not consume the following--flagas its value; and--dangerously-allow-allpasses argument parsing (it is accepted although not listed in--help).-pwas also confirmed to stay non-interactive and exit rather than opening a TUI.Checks
npm run format:check,npm run lint,npm run ci, andnpm run smoke:packall pass. 352 tests (+2).