diff --git a/io/ci.go b/io/ci.go index 85e00c8..500e7a8 100644 --- a/io/ci.go +++ b/io/ci.go @@ -1,8 +1,33 @@ package io -import "os" +import ( + "os" + "strings" +) -// isCI returns true if running in a CI environment. -func isCI() bool { - return os.Getenv("GITHUB_ACTIONS") == "true" || os.Getenv("CI") == "true" +// envTrue is the value CI providers use for their boolean environment flags. +const envTrue = "true" + +// isGitHubActions reports whether the runner is GitHub Actions specifically. +// +// Workflow commands like ::group:: are GitHub syntax, not a CI convention. Any +// other provider renders them as literal text, so they must be gated on this +// rather than on a general "is this CI" check. +func isGitHubActions() bool { + return os.Getenv("GITHUB_ACTIONS") == envTrue +} + +// workflowCommandEscaper escapes data in a GitHub workflow command. An +// unescaped newline would end the command early and emit the remainder as +// ordinary log lines; percent is escaped first so it cannot double-encode the +// replacements that follow. +var workflowCommandEscaper = strings.NewReplacer( + "%", "%25", + "\r", "%0D", + "\n", "%0A", +) + +// escapeWorkflowData makes a value safe to embed in a workflow command. +func escapeWorkflowData(s string) string { + return workflowCommandEscaper.Replace(s) } diff --git a/io/ci_group_test.go b/io/ci_group_test.go new file mode 100644 index 0000000..098ce9a --- /dev/null +++ b/io/ci_group_test.go @@ -0,0 +1,119 @@ +package io_test + +import ( + "os" + "strings" + "testing" + + "github.com/flowexec/tuikit/io" +) + +// groupOutput runs fn against a logger writing to a temp file and returns what +// was written, with the given CI-related environment applied. +func groupOutput(t *testing.T, env map[string]string, fn func(l *io.StandardLogger)) string { + t.Helper() + for k, v := range env { + t.Setenv(k, v) + } + + f, err := os.CreateTemp(t.TempDir(), "tuikit-group") + if err != nil { + t.Fatalf("temp file: %v", err) + } + defer f.Close() + + logger := io.NewLogger(io.WithMode(io.Text), io.WithOutput(f)) + fn(logger) + + if _, err := f.Seek(0, 0); err != nil { + t.Fatalf("seek: %v", err) + } + buf := make([]byte, 8192) + n, _ := f.Read(buf) + return string(buf[:n]) +} + +func TestBeginGroupEmitsWorkflowCommandOnGitHubActions(t *testing.T) { + out := groupOutput(t, map[string]string{"GITHUB_ACTIONS": "true", "CI": "true"}, + func(l *io.StandardLogger) { + l.BeginGroup("build app") + l.EndGroup() + }) + + if !strings.Contains(out, "::group::build app") { + t.Errorf("output = %q, want a ::group:: command", out) + } + if !strings.Contains(out, "::endgroup::") { + t.Errorf("output = %q, want an ::endgroup:: command", out) + } +} + +// ::group:: is GitHub syntax, not a CI convention. Emitting it on another +// provider prints literal noise into the log rather than collapsing anything. +func TestBeginGroupIsPlainOnNonGitHubCI(t *testing.T) { + out := groupOutput(t, map[string]string{"GITHUB_ACTIONS": "", "CI": "true"}, + func(l *io.StandardLogger) { + l.BeginGroup("build app") + l.EndGroup() + }) + + if strings.Contains(out, "::group::") || strings.Contains(out, "::endgroup::") { + t.Errorf("output = %q, want no GitHub workflow commands outside GitHub Actions", out) + } + if !strings.Contains(out, "build app") { + t.Errorf("output = %q, want the group name in the plain header", out) + } +} + +// GitHub does not support nested groups: a second ::group:: before the first +// closes swallows the remainder of the log. +func TestNestedGroupsEmitOnlyTheOutermost(t *testing.T) { + out := groupOutput(t, map[string]string{"GITHUB_ACTIONS": "true"}, + func(l *io.StandardLogger) { + l.BeginGroup("outer") + l.BeginGroup("inner") + l.EndGroup() + l.EndGroup() + }) + + if got := strings.Count(out, "::group::"); got != 1 { + t.Errorf("::group:: count = %d, want 1; output = %q", got, out) + } + if got := strings.Count(out, "::endgroup::"); got != 1 { + t.Errorf("::endgroup:: count = %d, want 1; output = %q", got, out) + } + if !strings.Contains(out, "::group::outer") { + t.Errorf("output = %q, want the outermost group to be the one emitted", out) + } +} + +// A stray ::endgroup:: would close whatever GitHub had open around it, so an +// unmatched EndGroup must emit nothing. This is what lets a caller skip +// BeginGroup conditionally without having to mirror that condition on the way out. +func TestUnmatchedEndGroupEmitsNothing(t *testing.T) { + out := groupOutput(t, map[string]string{"GITHUB_ACTIONS": "true"}, + func(l *io.StandardLogger) { + l.EndGroup() + }) + + if strings.Contains(out, "::endgroup::") { + t.Errorf("output = %q, want no ::endgroup:: without a matching BeginGroup", out) + } +} + +// An unescaped newline ends the workflow command early and leaks the remainder +// as ordinary log lines. +func TestGroupNameIsEscaped(t *testing.T) { + out := groupOutput(t, map[string]string{"GITHUB_ACTIONS": "true"}, + func(l *io.StandardLogger) { + l.BeginGroup("build\nrm -rf / 100%") + l.EndGroup() + }) + + if !strings.Contains(out, "::group::build%0Arm -rf / 100%25") { + t.Errorf("output = %q, want newline and percent escaped", out) + } + if strings.Count(out, "\n::group::") > 0 && strings.Count(out, "::group::") != 1 { + t.Errorf("output = %q, want a single well-formed group command", out) + } +} diff --git a/io/logger.go b/io/logger.go index d42fb0c..55534d6 100644 --- a/io/logger.go +++ b/io/logger.go @@ -39,6 +39,10 @@ type StandardLogger struct { // writeMu serializes the Std{Out,Err}Writer flip/render/restore sequence so concurrent // writers cannot interleave and render each other's output in the wrong mode. writeMu sync.Mutex + // groupMu guards groupDepth, which tracks open GitHub Actions log groups so + // that nested or unbalanced Begin/EndGroup calls cannot emit invalid output. + groupMu sync.Mutex + groupDepth int } type LoggerOptions func(*StandardLogger) @@ -609,19 +613,41 @@ func (l *StandardLogger) archiveTaskSummary(tasks []*TaskContext) { } func (l *StandardLogger) BeginGroup(name string) { - if isCI() { - _, _ = fmt.Fprintf(l.outFile, "::group::%s\n", name) - } else { - palette := l.theme.ColorPalette() - style := lipgloss.NewStyle(). - Foreground(lipgloss.Color(palette.Secondary)). - Bold(true) - _, _ = fmt.Fprintf(l.outWriter, "\n%s\n", style.Render("--- "+name+" ---")) + if isGitHubActions() { + l.groupMu.Lock() + defer l.groupMu.Unlock() + // GitHub does not support nested groups: a second ::group:: before the + // first closes swallows the rest of the log. Track depth and emit only + // the outermost, so a nested caller degrades to no group rather than a + // broken one. + l.groupDepth++ + if l.groupDepth == 1 { + _, _ = fmt.Fprintf(l.outFile, "::group::%s\n", escapeWorkflowData(name)) + } + return } + + palette := l.theme.ColorPalette() + style := lipgloss.NewStyle(). + Foreground(lipgloss.Color(palette.Secondary)). + Bold(true) + _, _ = fmt.Fprintf(l.outWriter, "\n%s\n", style.Render("--- "+name+" ---")) } func (l *StandardLogger) EndGroup() { - if isCI() { + if !isGitHubActions() { + return + } + + l.groupMu.Lock() + defer l.groupMu.Unlock() + // Without a matching BeginGroup there is nothing to close, and a stray + // ::endgroup:: would collapse whatever GitHub had open around it. + if l.groupDepth == 0 { + return + } + l.groupDepth-- + if l.groupDepth == 0 { _, _ = fmt.Fprintln(l.outFile, "::endgroup::") } }