Do not use terminal escape sequences in non-interactive terminals or the Windows command prompt.

[mitchell-as]

DX-1527 Occasional panic errors and continues gibberish during runtime progress on Windows

[Naatan]

Interactivity isn't the issue here. My forgetting to address cmd.exe when I implemented this is. There's a lib I found that should handle it cross-platform:

https://github.com/atomicgo/cursor

BUT it has the same issue as your code; it assumes windows==cmd.exe, whereas in reality windows also runs bash, and it's not an uncommon use-case.

That said; you can probably use the lib above to figure out how to do it with cmd.exe. I realize this is more of a missed functionality issue than a bug though, so if this turns into a timesink we could descope the windows implementation and utilize interactive=false for windows for the bug (but please only if it turns into a timesink).

Regardless of which route we go; it's not the job of the output package to assert interactive or detect shells. So we'll want to find a way to pass that in externally, this seems like a reasonable location:

https://github.com/ActiveState/cli/blob/4e7692e37d5d9602945a34fb352faed519d21c8e/cmd/state/output.go#L59

For shell detection we can use the logic here:

https://github.com/ActiveState/cli/blob/4e7692e37d5d9602945a34fb352faed519d21c8e/internal/subshell/subshell.go#L91

I suggest splitting that out into its own function so we don't have to do subshell.New().

There's a lot in my response here.. happy to jump on zoom to go over things in greater detail.

[mitchell-as]

I ended up using subshell.New() because it doesn't look very expensive and the result is cleaner. I could switch it to subshell.DetectShellBinary() and check the file basename for cmd.exe if you prefer.

[Naatan]

I'd still like to advocate splitting the shell detection logic out. It's true that New() isn't very costly, but it's still doing more than we need it to. Plus we don't know how it will evolve in the future, nor will we be sensitive to this use-case in the future unless we make it explicit.

[Naatan]

Let's send a rollbar error if this happens, since it should never happen.

[mitchell-as]

I thought about that, but then we'll hammer rollbar on every tick in a worst-case scenario.

[Naatan]

Could just only send it the first time. Record it to something like Spinner.recorded = true.

[mitchell-as]

Aye, but is there a chance we'll be creating a large number of these progress objects? We'd still be in the same boat.

[mitchell-as]

Let me know if I missed something extracting the shell detection logic into its own function.

[Naatan]

Please also see my replies to previous comments.

[Naatan]

This should be part of the shell detection.

[Naatan]

Nice, the subshell stuff looks much more readable!