diff --git a/docs/adr/50850-migrate-tabwriter-to-console-rendertable.md b/docs/adr/50850-migrate-tabwriter-to-console-rendertable.md new file mode 100644 index 00000000000..382f8891a69 --- /dev/null +++ b/docs/adr/50850-migrate-tabwriter-to-console-rendertable.md @@ -0,0 +1,43 @@ +# ADR-50850: Migrate CLI Table Rendering from tabwriter to console.RenderTable + +**Date**: 2026-08-06 +**Status**: Draft +**Deciders**: pelikhan (PR author), copilot-swe-agent (implementation) + +--- + +### Context + +`pkg/cli/logs_format_compact.go` rendered its two `[runs]` tables using `text/tabwriter`, writing tab-delimited lines directly to the output writer. Every other table in the CLI used the centralized `console.RenderTable` / `console.TableConfig` API, which provides lipgloss-based borders, automatic TTY detection, and consistent plain-text degradation on non-TTY writers (pipes, CI logs). The divergence meant the compact logs formatter had different visual output, bypassed the project's shared styling machinery, and required manual flush-error handling (`tw.Flush()`) not present elsewhere. + +### Decision + +We will migrate both tabwriter-backed tables in `renderLogsCompactToWriter` and `renderLogsCompactVerboseToWriter` to `console.RenderTable(console.TableConfig{...})`. All column headers, ordering, fallback values (`-` for empty fields), filtering of `skipped`/`cancelled` runs, and numeric formatting are preserved; only the alignment mechanism changes from tab-padding to lipgloss borders. + +### Alternatives Considered + +#### Alternative 1: Keep text/tabwriter + +The existing tabwriter approach is simple and has no runtime dependencies beyond the standard library. However, it produces plain tab-aligned output that does not adapt to TTY vs. non-TTY contexts the same way `console.RenderTable` does, creates visible inconsistency with every other CLI table, and requires manual flush-error handling. Accepting this inconsistency indefinitely is a maintenance burden and makes the codebase harder to reason about. + +#### Alternative 2: Write a Custom Formatter Matching console.RenderTable Behavior + +A custom formatter could in theory produce identical output without the `console` package dependency. However, this would duplicate the TTY-detection and border-rendering logic already centralized in `console.RenderTable`, creating two sources of truth for the same behavior and increasing the risk of divergence over time. No material benefit justifies the extra code. + +### Consequences + +#### Positive +- Compact logs tables now use the same bordered rendering style as all other CLI tables, eliminating a visible inconsistency in the tool's output. +- TTY-detection and plain-text degradation are handled automatically by `console.RenderTable`; no manual flush-or-error pattern is needed. +- The `text/tabwriter` import and both `tw.Flush()` error-logging branches are removed, reducing code surface area. + +#### Negative +- Output format changes from whitespace-aligned columns to lipgloss box-drawing characters (`╭─┬─╮`, etc.), which is a breaking change for any downstream consumer parsing the raw text of these tables. +- Box-drawing border glyphs increase per-row character count, raising token density when this output is consumed by LLMs or agents that read compact logs. The PR body explicitly calls this out as a deliberate tradeoff worth monitoring. + +#### Neutral +- A new test file `logs_format_compact_test.go` is added to cover bordered rendering, non-TTY ANSI-escape degradation, and `skipped`/`cancelled` run exclusion — behavior that was previously untested. + +--- + +*ADR created by [adr-writer agent]. Review and finalize before changing status from Draft to Accepted.* diff --git a/pkg/cli/data/agentic_workflows_fallback_aw_files.json b/pkg/cli/data/agentic_workflows_fallback_aw_files.json index b1f63d9fc3c..7d1f03c07d6 100644 --- a/pkg/cli/data/agentic_workflows_fallback_aw_files.json +++ b/pkg/cli/data/agentic_workflows_fallback_aw_files.json @@ -15,6 +15,7 @@ "debug-agentic-workflow.md", "dependabot.md", "deployment-status.md", + "designer-mappings.md", "designer.md", "evals.md", "experiments.md", diff --git a/pkg/cli/logs_format_compact.go b/pkg/cli/logs_format_compact.go index 1eda6c639f0..1f0713ca850 100644 --- a/pkg/cli/logs_format_compact.go +++ b/pkg/cli/logs_format_compact.go @@ -6,8 +6,8 @@ import ( "os" "strconv" "strings" - "text/tabwriter" + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/stringutil" ) @@ -108,10 +108,9 @@ func renderLogsCompactToWriter(w io.Writer, data LogsData) { return } - // [runs] aligned table using tabwriter + // [runs] aligned table fmt.Fprintln(w, "[runs]") - tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) - fmt.Fprintln(tw, "RUNID\tWORKFLOW\tENGINE\tSTATUS\tDUR\tTOKENS\tAIC\tTURNS\tERR\tEVENT\tACTOR\tBRANCH") + rows := make([][]string, 0, len(data.Runs)) for _, r := range data.Runs { status := r.Conclusion @@ -132,14 +131,18 @@ func renderLogsCompactToWriter(w io.Writer, data LogsData) { } wfID := workflowIDFromRun(r.WorkflowPath, r.WorkflowName) - fmt.Fprintf(tw, "%d\t%s\t%s\t%s\t%s\t%d\t%s\t%d\t%d\t%s\t%s\t%s\n", - r.RunID, wfID, r.EngineID, status, dur, - r.TokenUsage, formatCompactAIC(r.AIC), r.Turns, r.ErrorCount, - r.Event, actor, branch) - } - if err := tw.Flush(); err != nil { - logsCompactLog.Printf("flush error: %v", err) + rows = append(rows, []string{ + strconv.FormatInt(r.RunID, 10), wfID, r.EngineID, status, dur, + strconv.Itoa(r.TokenUsage), formatCompactAIC(r.AIC), + strconv.Itoa(r.Turns), strconv.Itoa(r.ErrorCount), + r.Event, actor, branch, + }) } + // RenderTable appends a trailing newline, so following section headers remain separated. + fmt.Fprint(w, console.RenderTable(console.TableConfig{ + Headers: []string{"RUNID", "WORKFLOW", "ENGINE", "STATUS", "DUR", "TOKENS", "AIC", "TURNS", "ERR", "EVENT", "ACTOR", "BRANCH"}, + Rows: rows, + })) // [errors] — aggregated error/warning messages if len(data.ErrorsAndWarnings) > 0 { @@ -283,8 +286,7 @@ func renderLogsCompactVerboseToWriter(w io.Writer, data LogsData) { // [runs] verbose aligned table fmt.Fprintln(w, "[runs]") - tw := tabwriter.NewWriter(w, 0, 0, 2, ' ', 0) - fmt.Fprintln(tw, "RUNID\tWORKFLOW\tENGINE\tSTATUS\tDUR\tTOKENS\tAIC\tTURNS\tERR\tWARN\tEVENT\tACTOR\tTBT\tCLASS\tCREATED\tBRANCH") + rows := make([][]string, 0, len(data.Runs)) for _, r := range data.Runs { status := r.Conclusion @@ -312,16 +314,19 @@ func renderLogsCompactVerboseToWriter(w io.Writer, data LogsData) { } wfID := workflowIDFromRun(r.WorkflowPath, r.WorkflowName) - fmt.Fprintf(tw, "%d\t%s\t%s\t%s\t%s\t%d\t%s\t%d\t%d\t%d\t%s\t%s\t%s\t%s\t%s\t%s\n", - r.RunID, wfID, r.EngineID, status, dur, - r.TokenUsage, formatCompactAIC(r.AIC), - r.Turns, r.ErrorCount, r.WarningCount, + rows = append(rows, []string{ + strconv.FormatInt(r.RunID, 10), wfID, r.EngineID, status, dur, + strconv.Itoa(r.TokenUsage), formatCompactAIC(r.AIC), + strconv.Itoa(r.Turns), strconv.Itoa(r.ErrorCount), strconv.Itoa(r.WarningCount), r.Event, actor, tbt, classification, - r.CreatedAt.Format("01-02 15:04"), r.Branch) - } - if err := tw.Flush(); err != nil { - logsCompactLog.Printf("flush error: %v", err) - } + r.CreatedAt.Format("01-02 15:04"), r.Branch, + }) + } + // RenderTable appends a trailing newline, so following section headers remain separated. + fmt.Fprint(w, console.RenderTable(console.TableConfig{ + Headers: []string{"RUNID", "WORKFLOW", "ENGINE", "STATUS", "DUR", "TOKENS", "AIC", "TURNS", "ERR", "WARN", "EVENT", "ACTOR", "TBT", "CLASS", "CREATED", "BRANCH"}, + Rows: rows, + })) // [errors] if len(data.ErrorsAndWarnings) > 0 { diff --git a/pkg/cli/logs_format_compact_test.go b/pkg/cli/logs_format_compact_test.go new file mode 100644 index 00000000000..eb52af080e9 --- /dev/null +++ b/pkg/cli/logs_format_compact_test.go @@ -0,0 +1,81 @@ +//go:build !integration + +package cli + +import ( + "bytes" + "strings" + "testing" + "time" + + "github.com/stretchr/testify/assert" +) + +func compactTestData() LogsData { + return LogsData{ + Summary: LogsSummary{TotalRuns: 1}, + Runs: []RunData{{ + RunID: 1234567, + WorkflowName: "Logs", + WorkflowPath: ".github/workflows/logs.lock.yml", + EngineID: "copilot", + Status: "completed", + Conclusion: "success", + Duration: "1m2s", + TokenUsage: 1200, + Turns: 4, + Event: "push", + Actor: "octocat", + Branch: "main", + CreatedAt: time.Now(), + }}, + } +} + +func TestRenderLogsCompactRendersRunsTableWithBorders(t *testing.T) { + t.Parallel() + var buf bytes.Buffer + renderLogsCompactToWriter(&buf, compactTestData()) + out := buf.String() + + assert.Contains(t, out, "[runs]") + assert.Contains(t, out, "╭") + assert.Contains(t, out, "RUNID") + assert.Contains(t, out, "1234567") + assert.Contains(t, out, "logs") + assert.NotContains(t, out, "\x1b[", "non-TTY output should degrade to plain text") +} + +func TestRenderLogsCompactVerboseRendersRunsTableWithBorders(t *testing.T) { + t.Parallel() + var buf bytes.Buffer + renderLogsCompactVerboseToWriter(&buf, compactTestData()) + out := buf.String() + + assert.Contains(t, out, "[runs]") + assert.Contains(t, out, "╭") + assert.Contains(t, out, "CLASS") + assert.Contains(t, out, "1234567") + assert.NotContains(t, out, "\x1b[", "non-TTY output should degrade to plain text") +} + +func TestRenderLogsCompactSkipsSkippedAndCancelledRuns(t *testing.T) { + t.Parallel() + data := compactTestData() + data.Runs = append(data.Runs, + RunData{RunID: 222, WorkflowName: "skipped-wf", Status: "skipped", CreatedAt: time.Now()}, + RunData{RunID: 333, WorkflowName: "cancelled-wf", Status: "cancelled", CreatedAt: time.Now()}, + RunData{RunID: 444, WorkflowName: "skipped-c", Conclusion: "skipped", CreatedAt: time.Now()}, + RunData{RunID: 555, WorkflowName: "cancelled-c", Conclusion: "cancelled", CreatedAt: time.Now()}, + ) + + var buf bytes.Buffer + renderLogsCompactToWriter(&buf, data) + out := buf.String() + + assert.NotContains(t, out, "222") + assert.NotContains(t, out, "333") + assert.NotContains(t, out, "444") + assert.NotContains(t, out, "555") + assert.Equal(t, 1, strings.Count(out, "1234567")) +} diff --git a/pkg/workflow/js/exchange_otlp_workload_identity.cjs b/pkg/workflow/js/exchange_otlp_workload_identity.cjs index 49fbc63a218..4502bc65a22 100644 --- a/pkg/workflow/js/exchange_otlp_workload_identity.cjs +++ b/pkg/workflow/js/exchange_otlp_workload_identity.cjs @@ -1,4 +1,5 @@ // @ts-check +// @safe-outputs-exempt SEC-004 — "body" references are HTTP transport payloads for OAuth token exchange, not GitHub content /** * Exchanges a GitHub OIDC token for a Google Cloud access token using * Workload Identity Federation, optionally impersonating a service account.