Conversation
sj26
added a commit
that referenced
this pull request
Jul 6, 2025
- Invert TTY detection logic to run action directly when no TTY available - Add comprehensive test coverage for SpinWhile function - Prevents crashes in CI/CD and non-interactive environments Fixes logic from PR #499 Amp-Thread: https://ampcode.com/threads/T-c5d06564-9dec-49ca-9134-0733756f1e98 Co-authored-by: Amp <amp@ampcode.com>
Merged
sj26
added a commit
that referenced
this pull request
Jul 7, 2025
- Invert TTY detection logic to run action directly when no TTY available - Add comprehensive test coverage for SpinWhile function - Prevents crashes in CI/CD and non-interactive environments Fixes logic from PR #499 Amp-Thread: https://ampcode.com/threads/T-c5d06564-9dec-49ca-9134-0733756f1e98 Co-authored-by: Amp <amp@ampcode.com>
sj26
added a commit
that referenced
this pull request
Jul 7, 2025
- Replace spinner.New() calls in 13 files across the codebase - All spinners now use SpinWhile for TTY-aware behavior - Add comprehensive test coverage for TTY/non-TTY scenarios - Ensures CLI works reliably in both interactive and automated environments - Follows patterns from PR #499 for consistent spinner behavior Files updated: - pkg/cmd/job/retry.go - pkg/cmd/build/rebuild.go - pkg/cmd/build/cancel.go - pkg/cmd/build/view.go - pkg/cmd/build/new.go - pkg/cmd/agent/view.go - pkg/cmd/artifacts/download.go - pkg/cmd/build/download.go - pkg/cmd/artifacts/list.go - pkg/cmd/job/unblock.go - pkg/cmd/pipeline/create.go - pkg/cmd/cluster/view.go - internal/pipeline/resolver/repository.go - internal/io/spinner_test.go (enhanced test coverage) Amp-Thread: https://ampcode.com/threads/T-69a131f0-814d-4d1d-b0fc-f7f61f34b1a6 Co-authored-by: Amp <amp@ampcode.com>
Contributor
|
@sj26 this looks good, did you want to rebase so we can merge in your PR? |
sj26
added a commit
that referenced
this pull request
Jul 8, 2025
- Replace spinner.New() calls in 13 files across the codebase - All spinners now use SpinWhile for TTY-aware behavior - Add comprehensive test coverage for TTY/non-TTY scenarios - Ensures CLI works reliably in both interactive and automated environments - Follows patterns from PR #499 for consistent spinner behavior Files updated: - pkg/cmd/job/retry.go - pkg/cmd/build/rebuild.go - pkg/cmd/build/cancel.go - pkg/cmd/build/view.go - pkg/cmd/build/new.go - pkg/cmd/agent/view.go - pkg/cmd/artifacts/download.go - pkg/cmd/build/download.go - pkg/cmd/artifacts/list.go - pkg/cmd/job/unblock.go - pkg/cmd/pipeline/create.go - pkg/cmd/cluster/view.go - internal/pipeline/resolver/repository.go - internal/io/spinner_test.go (enhanced test coverage) Amp-Thread: https://ampcode.com/threads/T-69a131f0-814d-4d1d-b0fc-f7f61f34b1a6 Co-authored-by: Amp <amp@ampcode.com>
- Fix SpinWhile logic to properly handle non-TTY environments - Add TTY detection to Confirm and PromptForOne functions - Replace all spinner.New() calls with TTY-aware SpinWhile() calls - Add non-TTY fallback for agent stop command using direct API calls - Add comprehensive test coverage for TTY/non-TTY behavior - All spinner operations now gracefully degrade when no TTY is available Fixes #496 Amp-Thread: https://ampcode.com/threads/T-04774b89-5ae4-4839-830d-67c16e65acb6 Co-authored-by: Amp <amp@ampcode.com>
- Fix HasDataAvailable to use Len() instead of Size() for strings.Reader - Restructure agent stop command to check TTY before consuming stdin - Split agent stop logic into separate interactive/non-interactive functions - Fix error output destination to match test expectations All tests now pass including agent stop command tests. Amp-Thread: https://ampcode.com/threads/T-04774b89-5ae4-4839-830d-67c16e65acb6 Co-authored-by: Amp <amp@ampcode.com>
Member
Author
|
I've rebased this, but I think it needs a little bit more. I like the idea of making non-interaction confirmations reject by default, then adding a And the selection thingo and stop interactive/non-interactive also feels odd. Feels like when we need tty and no-tty versions we need a little abstraction, like SpinWhile, which does something sensible and intention-based in both scenarios. |
Member
Author
|
Oh man, I missed that it already has |
Member
Author
|
Extracted another little bit - just the clearer message when unconfirmed: #503 |
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
2 participants
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.
I prompted Amp to remove the dependency on the TTY from the CLI. It's done a reasonable job!
It fixed a bug from my attempt to make spinners work out of TTY: #409
I'm not sold on its decision to always confirm in non-TTY. I think apt style "-y" which fails without confirmation by default might be better.
I also don't think it's done enough testing. But it's a great starting point!
Amp:
Fixes #496
Amp-Thread: https://ampcode.com/threads/T-04774b89-5ae4-4839-830d-67c16e65acb6