diff --git a/.github/workflows/smoke-copilot-aoai-entra.lock.yml b/.github/workflows/smoke-copilot-aoai-entra.lock.yml index 4718430c760..4b991a5c414 100644 --- a/.github/workflows/smoke-copilot-aoai-entra.lock.yml +++ b/.github/workflows/smoke-copilot-aoai-entra.lock.yml @@ -1718,7 +1718,7 @@ jobs: DOCKER_SOCK_GID=$(stat -c '%g' "$DOCKER_SOCK_PATH" 2>/dev/null || echo '0') export MCP_GATEWAY_DOCKER_COMMAND='docker run -i --rm --network host --add-host host.docker.internal:127.0.0.1 --user '"${MCP_GATEWAY_UID}"':'"${MCP_GATEWAY_GID}"' --group-add '"${DOCKER_SOCK_GID}"' -v '"${DOCKER_SOCK_PATH}"':/var/run/docker.sock -e MCP_GATEWAY_PORT -e MCP_GATEWAY_DOMAIN -e MCP_GATEWAY_API_KEY -e MCP_GATEWAY_PAYLOAD_DIR -e MCP_GATEWAY_PAYLOAD_SIZE_THRESHOLD -e DOCKER_HOST=unix:///var/run/docker.sock -e DEBUG -e MCP_GATEWAY_LOG_DIR -e GH_AW_MCP_LOG_DIR -e GH_AW_SAFE_OUTPUTS -e GH_AW_SAFE_OUTPUTS_CONFIG_PATH -e GH_AW_SAFE_OUTPUTS_TOOLS_PATH -e GH_AW_ASSETS_BRANCH -e GH_AW_ASSETS_MAX_SIZE_KB -e GH_AW_ASSETS_ALLOWED_EXTS -e DEFAULT_BRANCH -e GITHUB_MCP_SERVER_TOKEN -e GITHUB_MCP_GUARD_MIN_INTEGRITY -e GITHUB_MCP_GUARD_REPOS -e GITHUB_REPOSITORY -e GITHUB_SERVER_URL -e GITHUB_SHA -e GITHUB_WORKSPACE -e GITHUB_TOKEN -e GITHUB_RUN_ID -e GITHUB_RUN_NUMBER -e GITHUB_RUN_ATTEMPT -e GITHUB_JOB -e GITHUB_ACTION -e GITHUB_EVENT_NAME -e GITHUB_EVENT_PATH -e GITHUB_ACTOR -e GITHUB_ACTOR_ID -e GITHUB_TRIGGERING_ACTOR -e GITHUB_WORKFLOW -e GITHUB_WORKFLOW_REF -e GITHUB_WORKFLOW_SHA -e GITHUB_REF -e GITHUB_REF_NAME -e GITHUB_REF_TYPE -e GITHUB_HEAD_REF -e GITHUB_BASE_REF -e GH_AW_MCP_SCRIPTS_PORT -e GH_AW_MCP_SCRIPTS_API_KEY -e GH_AW_SAFE_OUTPUTS_PORT -e GH_AW_SAFE_OUTPUTS_API_KEY -e GITHUB_AW_OTEL_TRACE_ID -e GITHUB_AW_OTEL_PARENT_SPAN_ID -e OTEL_EXPORTER_OTLP_HEADERS -e GH_TOKEN -v /tmp/gh-aw/mcp-payloads:/tmp/gh-aw/mcp-payloads:rw -v /opt:/opt:ro -v /tmp:/tmp:rw -v '"${GITHUB_WORKSPACE}"':'"${GITHUB_WORKSPACE}"':rw ghcr.io/github/gh-aw-mcpg:v0.3.25' - mkdir -p /home/runner/.copilot + mkdir -p "$HOME/.copilot" GH_AW_NODE=$(which node 2>/dev/null || command -v node 2>/dev/null || echo node) cat << GH_AW_MCP_CONFIG_51d567a2fd4f7ca5_EOF | "$GH_AW_NODE" "${RUNNER_TEMP}/gh-aw/actions/start_mcp_gateway.cjs" { @@ -1849,9 +1849,11 @@ jobs: run: | set -o pipefail printf '%s' "$(date +%s%3N)" > /tmp/gh-aw/agent_cli_start_ms.txt - trap 'rm -f /home/runner/.copilot/settings.json' EXIT - mkdir -p /home/runner/.copilot - printf '%s' '{"builtInAgents":{"rubberDuck":false}}' > /home/runner/.copilot/settings.json + trap 'rm -f "$HOME/.copilot/settings.json"' EXIT + mkdir -p "$HOME/.copilot" + printf '%s' '{"builtInAgents":{"rubberDuck":false}}' > "$HOME/.copilot/settings.json" + export XDG_CONFIG_HOME="$HOME" + export GH_AW_MCP_CONFIG="$HOME/.copilot/mcp-config.json" touch /tmp/gh-aw/agent-step-summary.md GH_AW_NODE_BIN=$(command -v node 2>/dev/null || true) export GH_AW_NODE_BIN @@ -1889,7 +1891,6 @@ jobs: COPILOT_PROVIDER_BASE_URL: ${{ secrets.FOUNDRY_OPENAI_ENDPOINT }} COPILOT_PROVIDER_WIRE_API: responses GH_AW_MAX_TURNS: ${{ vars.GH_AW_DEFAULT_MAX_TURNS || '' }} - GH_AW_MCP_CONFIG: /home/runner/.copilot/mcp-config.json GH_AW_PHASE: agent GH_AW_PROMPT: /tmp/gh-aw/aw-prompts/prompt.txt GH_AW_SAFE_OUTPUTS: ${{ steps.set-runtime-paths.outputs.GH_AW_SAFE_OUTPUTS }} @@ -1910,7 +1911,6 @@ jobs: GIT_COMMITTER_EMAIL: github-actions[bot]@users.noreply.github.com GIT_COMMITTER_NAME: github-actions[bot] RUNNER_TEMP: ${{ runner.temp }} - XDG_CONFIG_HOME: /home/runner - name: Stop CLI Proxy if: always() continue-on-error: true @@ -2452,7 +2452,7 @@ jobs: if: always() && steps.detection_guard.outputs.run_detection == 'true' run: | rm -f "${RUNNER_TEMP}/gh-aw/mcp-config/mcp-servers.json" - rm -f /home/runner/.copilot/mcp-config.json + rm -f "$HOME/.copilot/mcp-config.json" rm -f "$GITHUB_WORKSPACE/.gemini/settings.json" - name: Prepare threat detection files if: always() && steps.detection_guard.outputs.run_detection == 'true' @@ -2510,9 +2510,10 @@ jobs: run: | set -o pipefail printf '%s' "$(date +%s%3N)" > /tmp/gh-aw/agent_cli_start_ms.txt - trap 'rm -f /home/runner/.copilot/settings.json' EXIT - mkdir -p /home/runner/.copilot - printf '%s' '{"builtInAgents":{"rubberDuck":false}}' > /home/runner/.copilot/settings.json + trap 'rm -f "$HOME/.copilot/settings.json"' EXIT + mkdir -p "$HOME/.copilot" + printf '%s' '{"builtInAgents":{"rubberDuck":false}}' > "$HOME/.copilot/settings.json" + export XDG_CONFIG_HOME="$HOME" touch /tmp/gh-aw/agent-step-summary.md GH_AW_NODE_BIN=$(command -v node 2>/dev/null || true) export GH_AW_NODE_BIN @@ -2567,7 +2568,6 @@ jobs: GIT_COMMITTER_EMAIL: github-actions[bot]@users.noreply.github.com GIT_COMMITTER_NAME: github-actions[bot] RUNNER_TEMP: ${{ runner.temp }} - XDG_CONFIG_HOME: /home/runner - name: Parse threat detection token usage for step summary id: parse_detection_token_usage if: always() diff --git a/docs/adr/38776-consolidate-duplicated-logic-into-shared-utilities.md b/docs/adr/38776-consolidate-duplicated-logic-into-shared-utilities.md new file mode 100644 index 00000000000..aa2b3e61d58 --- /dev/null +++ b/docs/adr/38776-consolidate-duplicated-logic-into-shared-utilities.md @@ -0,0 +1,39 @@ +# ADR-38776: Consolidate Duplicated Logic into Shared Utilities and Generics + +**Date**: 2026-06-12 +**Status**: Draft + +## Context + +A semantic function-clustering analysis (#38748) identified several groups of structurally identical code scattered across the `pkg/cli`, `pkg/console`, `pkg/parser`, and `pkg/workflow` packages: four inline string-truncation snippets, two near-identical `abbreviateNumber`/`formatTokens` implementations, a hand-rolled dedup loop duplicating `sliceutil.Deduplicate`, two identical inline-section extraction loops, two validation functions sharing the same "feature set → capability supported → error" shape, and inline GitHub remote-URL parsing that duplicated the GHE-aware `parseGitHubRepoSlugFromURL` helper. This duplication increases maintenance cost and risks the copies drifting apart (for example, the inline URL parser hardcoded `github.com` and was not GitHub Enterprise aware). No non-negotiable behavioral change is required; the goal is to reduce duplication without altering observable output. + +## Decision + +We decided to eliminate the identified duplication by consolidating each cluster into a single shared implementation: route inline truncations through `stringutil.Truncate`, replace both token formatters with one exported `console.FormatTokens`, replace the manual dedup loop with `sliceutil.Deduplicate`, extract a generic `extractInlineSections[T any]` helper used by both `ExtractInlineSkills` and `ExtractInlineSubAgents`, extract a shared `validateCapabilitySupport` helper, and delegate git-remote parsing to the existing GHE-aware `parseGitHubRepoSlugFromURL`. The driver is maintainability and a single source of truth; generics are introduced only where the extraction loops are structurally identical. + +## Alternatives Considered + +### Alternative 1: Leave the duplication in place (status quo) +Each copy is short and already works, so we could defer consolidation indefinitely. Rejected because the duplication was actively causing divergence (the inline URL parser was not GHE-aware while the shared helper was), and the analysis in #38748 had already surfaced the cost. + +### Alternative 2: Detect duplication with tooling instead of manual consolidation +We could add a linter or `dupl`-style check to flag clones rather than refactoring them now. Rejected as insufficient on its own — it would report the duplication but not remove it, and the clusters were already enumerated and small enough to consolidate directly. Tooling remains a complementary option for preventing future regressions. + +## Consequences + +### Positive +- Single source of truth for truncation, token formatting, deduplication, inline-section extraction, capability validation, and GitHub URL parsing. +- The git-remote fallback path is now GitHub Enterprise aware, fixing a latent inconsistency with hardcoded `github.com`. +- Reduced net code and lower future maintenance cost; behavior is preserved (existing tests, e.g. `TestFormatTokens`, were re-pointed at the shared helper). + +### Negative +- Introduces cross-package coupling: callers in `pkg/cli` now depend on `pkg/console`, `pkg/stringutil`, and `pkg/sliceutil` where they previously had local copies. +- The generic `extractInlineSections[T]` adds a small amount of cognitive overhead (a `makeItem` factory callback) compared with two explicit loops. + +### Neutral +- The rename `parseEventsJSONLFile` → `parseEventsJSONLMetrics` clarifies intent and avoids collision with `parseEventsJSONL`, but requires updating all callers and test names. +- A pre-existing `perfsprint` lint warning in `action_resolver.go` was fixed in passing; unrelated to the consolidation theme. + +--- + +*This is a DRAFT ADR generated by the [Design Decision Gate](https://github.com/github/gh-aw/actions/runs/27420151317) workflow. The PR author must review, complete, and finalize this document before the PR can merge.* diff --git a/pkg/cli/audit_cross_run_render.go b/pkg/cli/audit_cross_run_render.go index f564798bfa7..f840ab8ce1b 100644 --- a/pkg/cli/audit_cross_run_render.go +++ b/pkg/cli/audit_cross_run_render.go @@ -185,7 +185,7 @@ func markdownPerRunFields(run PerRunFirewallBreakdown) (string, string, string, } tokenStr := "—" if run.Tokens > 0 { - tokenStr = formatTokens(run.Tokens) + tokenStr = console.FormatTokens(run.Tokens) if run.TokenSpike { tokenStr += " ⚠" } @@ -246,8 +246,8 @@ func renderPrettyTokenTrend(mt MetricsTrendData) { return } fmt.Fprintf(os.Stderr, " Tokens: total=%s avg=%s/run min=%s max=%s\n%s", - formatTokens(mt.TotalTokens), formatTokens(mt.AvgTokens), - formatTokens(mt.MinTokens), formatTokens(mt.MaxTokens), prettySpikeNote("Token", mt.TokenSpikes)) + console.FormatTokens(mt.TotalTokens), console.FormatTokens(mt.AvgTokens), + console.FormatTokens(mt.MinTokens), console.FormatTokens(mt.MaxTokens), prettySpikeNote("Token", mt.TokenSpikes)) } func renderPrettyTurnTrend(mt MetricsTrendData) { @@ -376,7 +376,7 @@ func prettyPerRunOptionalFields(run PerRunFirewallBreakdown) string { } if run.Tokens > 0 { parts.WriteString(" tokens=") - parts.WriteString(formatTokens(run.Tokens)) + parts.WriteString(console.FormatTokens(run.Tokens)) if run.TokenSpike { parts.WriteString("⚠") } diff --git a/pkg/cli/audit_expanded.go b/pkg/cli/audit_expanded.go index 18ca15c3641..0fdbe388b19 100644 --- a/pkg/cli/audit_expanded.go +++ b/pkg/cli/audit_expanded.go @@ -172,7 +172,7 @@ func inferFallbackLogMetrics(logsPath string) (LogMetrics, string) { } if eventsJSONLPath := findEventsJSONLFile(logsPath); eventsJSONLPath != "" { - if metrics, err := parseEventsJSONLFile(eventsJSONLPath, false); err == nil && hasUsefulFallbackMetrics(metrics) { + if metrics, err := parseEventsJSONLMetrics(eventsJSONLPath, false); err == nil && hasUsefulFallbackMetrics(metrics) { return metrics, "copilot" } } diff --git a/pkg/cli/completions.go b/pkg/cli/completions.go index 1bda1d3b7a2..86eac8d762a 100644 --- a/pkg/cli/completions.go +++ b/pkg/cli/completions.go @@ -9,6 +9,7 @@ import ( "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/parser" "github.com/github/gh-aw/pkg/sliceutil" + "github.com/github/gh-aw/pkg/stringutil" "github.com/github/gh-aw/pkg/workflow" "github.com/spf13/cobra" ) @@ -44,21 +45,14 @@ func getWorkflowDescription(filePath string) string { // Look for description field if desc, ok := result.Frontmatter["description"]; ok { if descStr, ok := desc.(string); ok { - // Truncate to 60 characters for better display - if len(descStr) > 60 { - return descStr[:57] + "..." - } - return descStr + return stringutil.Truncate(descStr, 60) } } // Fallback to name field if description not found if name, ok := result.Frontmatter["name"]; ok { if nameStr, ok := name.(string); ok { - if len(nameStr) > 60 { - return nameStr[:57] + "..." - } - return nameStr + return stringutil.Truncate(nameStr, 60) } } diff --git a/pkg/cli/copilot_events_jsonl.go b/pkg/cli/copilot_events_jsonl.go index 26e841f403b..b8a5e751279 100644 --- a/pkg/cli/copilot_events_jsonl.go +++ b/pkg/cli/copilot_events_jsonl.go @@ -159,7 +159,7 @@ func findFileInDir(dir, name string) string { return found } -// parseEventsJSONLFile parses a Copilot events.jsonl file and extracts log metrics. +// parseEventsJSONLMetrics parses a Copilot events.jsonl file and extracts log metrics. // // events.jsonl provides precise, structured data about a Copilot CLI session: // - "session.start": session metadata (sessionId, copilotVersion) @@ -170,7 +170,7 @@ func findFileInDir(dir, name string) string { // // Returns the extracted metrics and nil on success, or empty metrics and an // error if the file cannot be read or contains no recognizable events. -func parseEventsJSONLFile(path string, verbose bool) (workflow.LogMetrics, error) { +func parseEventsJSONLMetrics(path string, verbose bool) (workflow.LogMetrics, error) { copilotEventsJSONLLog.Printf("Parsing events.jsonl from: %s", path) var metrics workflow.LogMetrics diff --git a/pkg/cli/copilot_events_jsonl_test.go b/pkg/cli/copilot_events_jsonl_test.go index f136f6a4ca7..d7ef9915c89 100644 --- a/pkg/cli/copilot_events_jsonl_test.go +++ b/pkg/cli/copilot_events_jsonl_test.go @@ -79,7 +79,7 @@ func realFormatEventsLine(eventType string, dataJSON string) string { return `{"type":"` + eventType + `","data":` + dataJSON + `,"id":"test-id","timestamp":"2026-04-02T04:00:00.000Z"}` } -// TestParseEventsJSONLFile verifies that parseEventsJSONLFile correctly extracts +// TestParseEventsJSONLFile verifies that parseEventsJSONLMetrics correctly extracts // turns, tool calls, tool sequences, and token counts from events.jsonl using // the real Copilot CLI format (nested data object, tool.execution_start events). func TestParseEventsJSONLFile(t *testing.T) { @@ -156,7 +156,7 @@ func TestParseEventsJSONLFile(t *testing.T) { eventsPath := filepath.Join(dir, "events.jsonl") require.NoError(t, os.WriteFile(eventsPath, []byte(tt.content), 0644)) - metrics, err := parseEventsJSONLFile(eventsPath, false) + metrics, err := parseEventsJSONLMetrics(eventsPath, false) if tt.wantErr { assert.Error(t, err, "expected an error") @@ -206,7 +206,7 @@ func TestParseEventsJSONLFile_RealArtifact(t *testing.T) { eventsPath := filepath.Join(dir, "events.jsonl") require.NoError(t, os.WriteFile(eventsPath, []byte(content), 0644)) - metrics, err := parseEventsJSONLFile(eventsPath, false) + metrics, err := parseEventsJSONLMetrics(eventsPath, false) require.NoError(t, err, "should parse without error") assert.Equal(t, 2, metrics.Turns, "should detect 2 turns") @@ -306,7 +306,7 @@ func TestParseEventsJSONLFile_TBT(t *testing.T) { eventsPath := filepath.Join(dir, "events.jsonl") require.NoError(t, os.WriteFile(eventsPath, []byte(content), 0644)) - metrics, err := parseEventsJSONLFile(eventsPath, false) + metrics, err := parseEventsJSONLMetrics(eventsPath, false) require.NoError(t, err, "should parse without error") assert.Equal(t, 3, metrics.Turns, "should detect 3 turns") @@ -322,7 +322,7 @@ func TestParseEventsJSONLFile_TBT(t *testing.T) { eventsPath := filepath.Join(dir, "events.jsonl") require.NoError(t, os.WriteFile(eventsPath, []byte(content), 0644)) - metrics, err := parseEventsJSONLFile(eventsPath, false) + metrics, err := parseEventsJSONLMetrics(eventsPath, false) require.NoError(t, err, "should parse without error") assert.Equal(t, 1, metrics.Turns, "should detect 1 turn") @@ -340,7 +340,7 @@ func TestParseEventsJSONLFile_TBT(t *testing.T) { eventsPath := filepath.Join(dir, "events.jsonl") require.NoError(t, os.WriteFile(eventsPath, []byte(content), 0644)) - metrics, err := parseEventsJSONLFile(eventsPath, false) + metrics, err := parseEventsJSONLMetrics(eventsPath, false) require.NoError(t, err, "should parse without error") assert.Equal(t, 2, metrics.Turns, "should detect 2 turns") diff --git a/pkg/cli/git.go b/pkg/cli/git.go index c54d9de0730..687769e499c 100644 --- a/pkg/cli/git.go +++ b/pkg/cli/git.go @@ -53,8 +53,9 @@ func findGitRootForPath(path string) (string, error) { } // parseGitHubRepoSlugFromURL extracts owner/repo from a GitHub URL -// Supports both HTTPS (https://github.com/owner/repo) and SSH (git@github.com:owner/repo) formats -// Also supports GitHub Enterprise URLs +// Supports HTTPS (https://github.com/owner/repo), SCP-style SSH (git@github.com:owner/repo), +// and SSH URL scheme (ssh://git@github.com/owner/repo) formats. +// Also supports GitHub Enterprise URLs. func parseGitHubRepoSlugFromURL(url string) string { gitLog.Printf("Parsing GitHub repo slug from URL: %s", url) @@ -71,7 +72,7 @@ func parseGitHubRepoSlugFromURL(url string) string { return slug } - // Handle SSH URLs: git@github.com:owner/repo or git@enterprise.github.com:owner/repo + // Handle SCP-style SSH URLs: git@github.com:owner/repo or git@enterprise.github.com:owner/repo sshPrefix := "git@" + githubHostWithoutScheme + ":" if after, ok := strings.CutPrefix(url, sshPrefix); ok { slug := after @@ -79,6 +80,18 @@ func parseGitHubRepoSlugFromURL(url string) string { return slug } + // Handle SSH URL scheme: ssh://git@github.com/owner/repo or ssh://github.com/owner/repo + if after, ok := strings.CutPrefix(url, "ssh://"); ok { + // Strip optional user info (e.g. "git@") + if _, userStripped, hasAt := strings.Cut(after, "@"); hasAt { + after = userStripped + } + if slug, ok := strings.CutPrefix(after, githubHostWithoutScheme+"/"); ok { + gitLog.Printf("Extracted slug from SSH URL scheme: %s", slug) + return slug + } + } + gitLog.Print("Could not extract slug from URL") return "" } diff --git a/pkg/cli/git_helpers_test.go b/pkg/cli/git_helpers_test.go index 9e8a07dfafb..94223709927 100644 --- a/pkg/cli/git_helpers_test.go +++ b/pkg/cli/git_helpers_test.go @@ -32,6 +32,21 @@ func TestParseGitHubRepoSlugFromURL(t *testing.T) { url: "git@github.com:github/gh-aw", expected: "github/gh-aw", }, + { + name: "SSH URL scheme with .git", + url: "ssh://git@github.com/github/gh-aw.git", + expected: "github/gh-aw", + }, + { + name: "SSH URL scheme without .git", + url: "ssh://git@github.com/github/gh-aw", + expected: "github/gh-aw", + }, + { + name: "SSH URL scheme without user info", + url: "ssh://github.com/github/gh-aw.git", + expected: "github/gh-aw", + }, { name: "Invalid URL", url: "not-a-github-url", diff --git a/pkg/cli/health_metrics.go b/pkg/cli/health_metrics.go index 006a50474c2..acf2d1d2bea 100644 --- a/pkg/cli/health_metrics.go +++ b/pkg/cli/health_metrics.go @@ -2,9 +2,9 @@ package cli import ( "fmt" - "strconv" "time" + "github.com/github/gh-aw/pkg/console" "github.com/github/gh-aw/pkg/logger" "github.com/github/gh-aw/pkg/stats" "github.com/github/gh-aw/pkg/timeutil" @@ -107,7 +107,7 @@ func CalculateWorkflowHealth(workflowName string, runs []WorkflowRun, threshold // Format display values displayRate := fmt.Sprintf("%.0f%% (%d/%d)", successRate, successCount, totalRuns) displayDur := timeutil.FormatDuration(avgDuration) - displayTokens := formatTokens(avgTokens) + displayTokens := console.FormatTokens(avgTokens) belowThreshold := successRate < threshold @@ -215,17 +215,3 @@ func GroupRunsByWorkflow(runs []WorkflowRun) map[string][]WorkflowRun { } return grouped } - -// formatTokens formats token count in a human-readable format -func formatTokens(tokens int) string { - if tokens == 0 { - return "-" - } - if tokens < 1000 { - return strconv.Itoa(tokens) - } - if tokens < 1000000 { - return fmt.Sprintf("%.1fK", float64(tokens)/1000) - } - return fmt.Sprintf("%.1fM", float64(tokens)/1000000) -} diff --git a/pkg/cli/health_metrics_test.go b/pkg/cli/health_metrics_test.go index c0289a3cb6a..870a93f5b3c 100644 --- a/pkg/cli/health_metrics_test.go +++ b/pkg/cli/health_metrics_test.go @@ -6,6 +6,7 @@ import ( "testing" "time" + "github.com/github/gh-aw/pkg/console" "github.com/stretchr/testify/assert" ) @@ -251,7 +252,7 @@ func TestFormatTokens(t *testing.T) { for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { - result := formatTokens(tt.tokens) + result := console.FormatTokens(tt.tokens) assert.Equal(t, tt.expected, result, "Formatted tokens should match") }) } diff --git a/pkg/cli/logs_format_compact.go b/pkg/cli/logs_format_compact.go index 2b247d4b0d0..12441dd4e71 100644 --- a/pkg/cli/logs_format_compact.go +++ b/pkg/cli/logs_format_compact.go @@ -8,6 +8,7 @@ import ( "text/tabwriter" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/stringutil" ) var logsCompactLog = logger.New("cli:logs_format_compact") @@ -117,10 +118,7 @@ func renderLogsCompact(data LogsData) { if dur == "" { dur = "-" } - branch := r.Branch - if len(branch) > 30 { - branch = branch[:27] + "..." - } + branch := stringutil.Truncate(r.Branch, 30) actor := r.Actor if actor == "" { actor = "-" @@ -138,10 +136,7 @@ func renderLogsCompact(data LogsData) { if len(data.ErrorsAndWarnings) > 0 { fmt.Fprintln(os.Stdout, "[errors]") for _, ew := range data.ErrorsAndWarnings { - msg := ew.Message - if len(msg) > 120 { - msg = msg[:117] + "..." - } + msg := stringutil.Truncate(ew.Message, 120) fmt.Fprintf(os.Stdout, "%s run=%d count=%d: %s\n", ew.Type, ew.RunID, ew.Count, msg) } } diff --git a/pkg/cli/logs_metrics.go b/pkg/cli/logs_metrics.go index 576b32c775f..0039114b67b 100644 --- a/pkg/cli/logs_metrics.go +++ b/pkg/cli/logs_metrics.go @@ -150,7 +150,7 @@ func extractLogMetrics(logDir string, verbose bool, workflowPath ...string) (Log fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Found events.jsonl, using as primary metrics source")) } } - eventsMetrics, eventsErr := parseEventsJSONLFile(eventsJSONLPath, verbose) + eventsMetrics, eventsErr := parseEventsJSONLMetrics(eventsJSONLPath, verbose) if eventsErr == nil { metrics = eventsMetrics eventsJSONLParsed = true diff --git a/pkg/cli/repo.go b/pkg/cli/repo.go index de4b6a3a449..99655f43ac3 100644 --- a/pkg/cli/repo.go +++ b/pkg/cli/repo.go @@ -47,26 +47,13 @@ func getCurrentRepoSlugUncached() (string, error) { remoteURL := strings.TrimSpace(string(gitOutput)) repoLog.Printf("Parsing git remote URL: %s", remoteURL) - // Parse GitHub repository from remote URL - // Handle both SSH and HTTPS formats - var repoPath string - - // SSH format: git@github.com:owner/repo.git - if after, ok := strings.CutPrefix(remoteURL, "git@github.com:"); ok { - repoPath = after - } else if strings.Contains(remoteURL, "github.com/") { - // HTTPS format: https://github.com/owner/repo.git - parts := strings.Split(remoteURL, "github.com/") - if len(parts) >= 2 { - repoPath = parts[1] - } - } else { + // Delegate to the shared helper which supports both HTTPS and SSH formats, + // including GitHub Enterprise hosts configured via getGitHubHost(). + repoPath := parseGitHubRepoSlugFromURL(remoteURL) + if repoPath == "" { return "", fmt.Errorf("remote URL does not appear to be a GitHub repository: %s", remoteURL) } - // Remove .git suffix if present - repoPath = strings.TrimSuffix(repoPath, ".git") - // Validate format (should be owner/repo) parts := strings.Split(repoPath, "/") if len(parts) != 2 || parts[0] == "" || parts[1] == "" { diff --git a/pkg/console/render.go b/pkg/console/render.go index 1c09f401493..983f8ce9ed7 100644 --- a/pkg/console/render.go +++ b/pkg/console/render.go @@ -10,6 +10,7 @@ import ( "time" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/stringutil" ) var renderLog = logger.New("console:render") @@ -477,12 +478,8 @@ func formatFieldValueWithTag(val reflect.Value, tag consoleTag) string { } // Apply maxlen truncation if specified - if tag.maxLen > 0 && len(baseValue) > tag.maxLen { - if tag.maxLen > 3 { - baseValue = baseValue[:tag.maxLen-3] + "..." - } else { - baseValue = baseValue[:tag.maxLen] - } + if tag.maxLen > 0 { + baseValue = stringutil.Truncate(baseValue, tag.maxLen) } return baseValue @@ -630,6 +627,30 @@ func FormatNumber(n int) string { } } +// FormatTokens formats a token count as a compact human-readable string. +// Zero is rendered as "-"; values below 1000 are rendered as plain integers; +// values in the thousands are rendered with one decimal place and a "K" suffix; +// values in the millions are rendered with one decimal place and an "M" suffix. +// +// Examples: +// +// FormatTokens(0) // "-" +// FormatTokens(500) // "500" +// FormatTokens(1500) // "1.5K" +// FormatTokens(1200000) // "1.2M" +func FormatTokens(tokens int) string { + if tokens == 0 { + return "-" + } + if tokens < 1000 { + return strconv.Itoa(tokens) + } + if tokens < 1_000_000 { + return fmt.Sprintf("%.1fK", float64(tokens)/1000) + } + return fmt.Sprintf("%.1fM", float64(tokens)/1_000_000) +} + // ToRelativePath converts an absolute path to a relative path from the current working directory // If the relative path contains "..", returns the absolute path instead for clarity func ToRelativePath(path string) string { diff --git a/pkg/parser/inline_section_helpers.go b/pkg/parser/inline_section_helpers.go index 67c173a7d96..8dcf015a594 100644 --- a/pkg/parser/inline_section_helpers.go +++ b/pkg/parser/inline_section_helpers.go @@ -40,6 +40,21 @@ func nextH2After(offset int, h2Positions []int, markdownLength int) int { return markdownLength } +// extractInlineSections collects all sections delimited by marker positions in +// markdown. The caller provides allStarts (already validated non-empty), and a +// makeItem factory that converts a (name, content) pair into the desired result +// type T. It returns the trimmed main markdown (the text before the first +// marker) and the collected items. +func extractInlineSections[T any](markdown string, allStarts [][]int, makeItem func(name, content string) T) (mainMarkdown string, items []T) { + mainMarkdown = strings.TrimRight(markdown[:allStarts[0][0]], "\n") + h2Positions := collectH2Positions(markdown) + for _, m := range allStarts { + name, content := extractInlineSection(markdown, m, h2Positions) + items = append(items, makeItem(name, content)) + } + return mainMarkdown, items +} + func validateUniqueInlineSectionNames(markdown string, allStarts [][]int, createDuplicateError func(name string) error) error { seen := make(map[string]struct{}) for _, m := range allStarts { diff --git a/pkg/parser/inline_skill_extractor.go b/pkg/parser/inline_skill_extractor.go index 4542581c0fe..8dfeef814ee 100644 --- a/pkg/parser/inline_skill_extractor.go +++ b/pkg/parser/inline_skill_extractor.go @@ -87,14 +87,10 @@ func ExtractInlineSkills(markdown string) (mainMarkdown string, skills []InlineS return "", nil, err } - mainMarkdown = strings.TrimRight(markdown[:allStarts[0][0]], "\n") - h2Positions := collectH2Positions(markdown) - for _, m := range allStarts { - name, content := extractInlineSection(markdown, m, h2Positions) + mainMarkdown, skills = extractInlineSections(markdown, allStarts, func(name, content string) InlineSkill { inlineSkillLog.Printf("Extracted inline skill %q (content length: %d)", name, len(content)) - skills = append(skills, InlineSkill{Name: name, Content: content}) - } - + return InlineSkill{Name: name, Content: content} + }) inlineSkillLog.Printf("Extraction complete: %d skill(s), main markdown length: %d", len(skills), len(mainMarkdown)) return mainMarkdown, skills, nil } diff --git a/pkg/parser/sub_agent_extractor.go b/pkg/parser/sub_agent_extractor.go index 46bdbb08dc2..df4bcd95da8 100644 --- a/pkg/parser/sub_agent_extractor.go +++ b/pkg/parser/sub_agent_extractor.go @@ -232,14 +232,10 @@ func ExtractInlineSubAgents(markdown string) (mainMarkdown string, agents []Inli return "", nil, err } - mainMarkdown = strings.TrimRight(markdown[:allStarts[0][0]], "\n") - h2Positions := collectH2Positions(markdown) - for _, m := range allStarts { - name, content := extractInlineSection(markdown, m, h2Positions) + mainMarkdown, agents = extractInlineSections(markdown, allStarts, func(name, content string) InlineSubAgent { subAgentLog.Printf("Extracted sub-agent %q (content length: %d)", name, len(content)) - agents = append(agents, InlineSubAgent{Name: name, Content: content}) - } - + return InlineSubAgent{Name: name, Content: content} + }) subAgentLog.Printf("Extraction complete: %d sub-agent(s), main markdown length: %d", len(agents), len(mainMarkdown)) return mainMarkdown, agents, nil } diff --git a/pkg/workflow/action_resolver.go b/pkg/workflow/action_resolver.go index 34df2779e61..118e5e2c7cd 100644 --- a/pkg/workflow/action_resolver.go +++ b/pkg/workflow/action_resolver.go @@ -163,18 +163,18 @@ func ResolveGhAwRef(ctx context.Context, ref string) (string, error) { return ref, nil } resolverLog.Printf("Resolving --gh-aw-ref %q to commit SHA via GitHub API", ref) - apiPath := fmt.Sprintf("/repos/github/gh-aw/commits/%s", ref) + apiPath := "/repos/github/gh-aw/commits/" + ref callCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() -cmd := ExecGHContext(callCtx, "api", apiPath, "--jq", ".sha") -output, err := cmd.CombinedOutput() -if err != nil { - msg := strings.TrimSpace(string(output)) - if msg != "" { - return "", fmt.Errorf("failed to resolve gh-aw ref %q to SHA: %s: %w", ref, msg, err) + cmd := ExecGHContext(callCtx, "api", apiPath, "--jq", ".sha") + output, err := cmd.CombinedOutput() + if err != nil { + msg := strings.TrimSpace(string(output)) + if msg != "" { + return "", fmt.Errorf("failed to resolve gh-aw ref %q to SHA: %s: %w", ref, msg, err) + } + return "", fmt.Errorf("failed to resolve gh-aw ref %q to SHA: %w", ref, err) } - return "", fmt.Errorf("failed to resolve gh-aw ref %q to SHA: %w", ref, err) -} sha := strings.TrimSpace(string(output)) if !gitutil.IsValidFullSHA(sha) { return "", fmt.Errorf("unexpected response resolving gh-aw ref %q: got %q (expected 40-char hex SHA)", ref, sha) diff --git a/pkg/workflow/agent_validation.go b/pkg/workflow/agent_validation.go index 89ee5f52d05..1998c546646 100644 --- a/pkg/workflow/agent_validation.go +++ b/pkg/workflow/agent_validation.go @@ -111,6 +111,20 @@ func (c *Compiler) validateAgentFile(workflowData *WorkflowData, markdownPath st return nil } +// validateCapabilitySupport is a shared helper that checks whether an engine supports a +// required capability. It returns an error with a standard "not supported" message when +// the feature is requested but the engine capability is missing. +// Returns nil when featureSet is false (feature not requested) or when the engine supports it. +func validateCapabilitySupport(featureName string, featureSet bool, capabilitySupported bool, engineID string) error { + if !featureSet { + return nil + } + if !capabilitySupported { + return fmt.Errorf("%s not supported: engine '%s' does not support the %s feature", featureName, engineID, featureName) + } + return nil +} + // validateMaxTurnsSupport validates that max-turns is only used with engines that support this feature func (c *Compiler) validateMaxTurnsSupport(frontmatter map[string]any, engine CodingAgentEngine) error { // Check if max-turns is specified in the engine config @@ -118,23 +132,10 @@ func (c *Compiler) validateMaxTurnsSupport(frontmatter map[string]any, engine Co hasMaxTurns := engineConfig != nil && engineConfig.MaxTurns != "" - if !hasMaxTurns { - // No max-turns specified, no validation needed - return nil - } - - agentValidationLog.Printf("Validating max-turns support: engine=%s, maxTurns=%s", engine.GetID(), engineConfig.MaxTurns) - - // max-turns is specified, check if the engine supports it - if !engine.GetCapabilities().MaxTurns { - agentValidationLog.Printf("Engine %s does not support max-turns feature", engine.GetID()) - return fmt.Errorf("max-turns not supported: engine '%s' does not support the max-turns feature", engine.GetID()) + if hasMaxTurns { + agentValidationLog.Printf("Validating max-turns support: engine=%s", engine.GetID()) } - - // Engine supports max-turns - additional validation could be added here if needed - // For now, we rely on JSON schema validation for format checking - - return nil + return validateCapabilitySupport("max-turns", hasMaxTurns, engine.GetCapabilities().MaxTurns, engine.GetID()) } // validateMaxContinuationsSupport validates that max-continuations is only used with engines that support this feature @@ -142,20 +143,12 @@ func (c *Compiler) validateMaxContinuationsSupport(frontmatter map[string]any, e // Check if max-continuations is specified in the engine config _, engineConfig := c.ExtractEngineConfig(frontmatter) - if engineConfig == nil || engineConfig.MaxContinuations == 0 { - // No max-continuations specified, no validation needed - return nil - } - - agentValidationLog.Printf("Validating max-continuations support: engine=%s, maxContinuations=%d", engine.GetID(), engineConfig.MaxContinuations) + hasMaxContinuations := engineConfig != nil && engineConfig.MaxContinuations != 0 - // max-continuations is specified, check if the engine supports it - if !engine.GetCapabilities().MaxContinuations { - agentValidationLog.Printf("Engine %s does not support max-continuations feature", engine.GetID()) - return fmt.Errorf("max-continuations not supported: engine '%s' does not support the max-continuations feature", engine.GetID()) + if hasMaxContinuations { + agentValidationLog.Printf("Validating max-continuations support: engine=%s", engine.GetID()) } - - return nil + return validateCapabilitySupport("max-continuations", hasMaxContinuations, engine.GetCapabilities().MaxContinuations, engine.GetID()) } // validateMaxToolDenialsSupport validates that max-tool-denials is only used with diff --git a/pkg/workflow/cache_integrity.go b/pkg/workflow/cache_integrity.go index b02743ebe6c..6a74d6008ca 100644 --- a/pkg/workflow/cache_integrity.go +++ b/pkg/workflow/cache_integrity.go @@ -8,6 +8,7 @@ import ( "strings" "github.com/github/gh-aw/pkg/logger" + "github.com/github/gh-aw/pkg/sliceutil" ) var cacheIntegrityLog = logger.New("workflow:cache_integrity") @@ -106,14 +107,7 @@ func canonicalUserList(users []string) string { } // Deduplicate - seen := make(map[string]struct{}, len(normalized)) - deduped := normalized[:0] - for _, u := range normalized { - if _, exists := seen[u]; !exists { - seen[u] = struct{}{} - deduped = append(deduped, u) - } - } + deduped := sliceutil.Deduplicate(normalized) // Sort sort.Strings(deduped)