From 2bcef054a50ee70f1bf9b22bb1ba526370d20a5e Mon Sep 17 00:00:00 2001 From: Jahvon Dockery Date: Thu, 27 Aug 2026 00:51:12 -0400 Subject: [PATCH] fix(io): emit valid GitHub log groups, and only on GitHub MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three problems with CI log grouping, all of which put output where a reader cannot see it. Workflow commands were gated on isCI, which is true for any provider. ::group:: is GitHub syntax, so every other CI printed literal "::group::name" lines instead of a header. They are now gated on GITHUB_ACTIONS, and everything else gets the styled header that local runs already got. isCI had no other callers, so it is removed rather than left dead. Nested groups are not supported by GitHub: a second ::group:: before the first closes swallows the rest of the log. Depth is now tracked so only the outermost pair is emitted, and a nested caller degrades to no group rather than a broken one. An unmatched EndGroup emitted a stray ::endgroup::, which closes whatever GitHub had open around it. It is now a no-op, so a caller can skip BeginGroup conditionally without mirroring that condition on the way out — which is what lets flow suppress the group for single-task runs, where wrapping the only output in a collapsed group hides it for no benefit. Group names are escaped per GitHub's spec; an unescaped newline previously ended the command early and leaked the remainder as ordinary log lines. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01R328pa3FUUfga4gYah1iQi --- io/ci.go | 33 ++++++++++-- io/ci_group_test.go | 119 ++++++++++++++++++++++++++++++++++++++++++++ io/logger.go | 44 ++++++++++++---- 3 files changed, 183 insertions(+), 13 deletions(-) create mode 100644 io/ci_group_test.go 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::") } }