feat: add --exclude flag to logs command and MCP tool to skip specified workflows#28452
feat: add --exclude flag to logs command and MCP tool to skip specified workflows#28452
Conversation
…ed workflows Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4cf7448d-21ec-4500-b93d-fdbe9cddd4f4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/4cf7448d-21ec-4500-b93d-fdbe9cddd4f4 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds workflow-exclusion support to the gh aw logs command and the MCP logs tool so users can skip specified workflows before downloading artifacts, reducing unnecessary API calls and network traffic.
Changes:
- Introduces
--exclude(StringSlice) togh aw logsand threads the value through to the log download orchestrator. - Adds exclude resolution + matching helpers and applies the exclude filter before artifact downloads.
- Extends the MCP privileged
logstool schema withexclude_workflowsand forwards it to the CLI.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_command.go | Adds --exclude flag, examples, and passes excludes into DownloadWorkflowLogs. |
| pkg/cli/logs_orchestrator.go | Adds excludeWorkflows parameter and filters runs before downloading artifacts. |
| pkg/cli/logs_utils.go | Adds helpers to resolve exclude inputs and check exclusion matches. |
| pkg/cli/mcp_tools_privileged.go | Adds exclude_workflows to MCP schema and forwards as --exclude to CLI subprocess. |
| pkg/cli/logs_command_test.go | Adds tests for --exclude flag registration and isWorkflowExcluded. |
| pkg/cli/logs_json_stderr_order_test.go | Updates DownloadWorkflowLogs calls for new parameter. |
| pkg/cli/logs_ci_scenario_test.go | Updates DownloadWorkflowLogs calls for new parameter. |
| pkg/cli/context_cancellation_test.go | Updates DownloadWorkflowLogs calls for new parameter. |
| pkg/cli/logs_download_test.go | Updates DownloadWorkflowLogs calls for new parameter. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Files reviewed: 9/9 changed files
- Comments generated: 3
| func isWorkflowExcluded(workflowName string, excludes []string) bool { | ||
| lowerName := strings.ToLower(workflowName) | ||
| for _, ex := range excludes { | ||
| if strings.ToLower(ex) == lowerName { | ||
| return true |
There was a problem hiding this comment.
isWorkflowExcluded only compares the run's WorkflowName to the exclude list. If resolveExcludeWorkflows fails to resolve an ID (e.g., when running with --repo and no local .lock.yml files), the raw ID (like weekly-research) will never match WorkflowRun.WorkflowName (which is a display name), so --exclude <id> won't work in the exact scenario the helper claims to support. Consider matching excludes against both the display name and a normalized workflow ID derived from WorkflowRun.WorkflowPath (e.g., basename normalized by stripping .lock.yml/.md), and/or normalizing exclude inputs similarly.
| resolved := make([]string, 0, len(excludes)) | ||
| for _, name := range excludes { | ||
| name = strings.TrimSpace(name) | ||
| if name == "" { | ||
| continue | ||
| } | ||
| // Try to resolve to canonical display name via lock files. | ||
| displayName, err := workflow.FindWorkflowName(name) | ||
| if err == nil && displayName != "" { | ||
| logsUtilsLog.Printf("Resolved exclude workflow '%s' -> '%s'", name, displayName) | ||
| resolved = append(resolved, displayName) |
There was a problem hiding this comment.
resolveExcludeWorkflows calls workflow.FindWorkflowName inside the loop, and FindWorkflowName scans/parses workflow lock files (glob + read) when resolving. With multiple --exclude entries this becomes repeated filesystem work. Consider loading workflows once (e.g., via workflow.GetAllWorkflows) and resolving all excludes against an in-memory map, or adding a batch-resolve helper in pkg/workflow to avoid re-scanning files per exclude value.
| func DownloadWorkflowLogs(ctx context.Context, workflowName string, count int, startDate, endDate, outputDir, engine, ref string, beforeRunID, afterRunID int64, repoOverride string, verbose bool, toolGraph bool, noStaged bool, firewallOnly bool, noFirewall bool, parse bool, jsonOutput bool, timeout int, summaryFile string, safeOutputType string, filteredIntegrity bool, train bool, format string, artifactSets []string, excludeWorkflows []string) error { | ||
| logsOrchestratorLog.Printf("Starting workflow log download: workflow=%s, count=%d, startDate=%s, endDate=%s, outputDir=%s, summaryFile=%s, safeOutputType=%s, filteredIntegrity=%v, train=%v, format=%s, artifactSets=%v, excludeWorkflows=%v", workflowName, count, startDate, endDate, outputDir, summaryFile, safeOutputType, filteredIntegrity, train, format, artifactSets, excludeWorkflows) |
There was a problem hiding this comment.
DownloadWorkflowLogs already has a very large parameter list, and adding excludeWorkflows increases call-site churn and makes the API harder to use correctly. Consider introducing an options struct (e.g., DownloadWorkflowLogsOptions) and passing a single argument, keeping the current signature as a thin backward-compatible wrapper if needed.
See below for a potential fix:
// DownloadWorkflowLogsOptions groups the configuration for downloading and analyzing workflow logs.
type DownloadWorkflowLogsOptions struct {
WorkflowName string
Count int
StartDate string
EndDate string
OutputDir string
Engine string
Ref string
BeforeRunID int64
AfterRunID int64
RepoOverride string
Verbose bool
ToolGraph bool
NoStaged bool
FirewallOnly bool
NoFirewall bool
Parse bool
JSONOutput bool
Timeout int
SummaryFile string
SafeOutputType string
FilteredIntegrity bool
Train bool
Format string
ArtifactSets []string
ExcludeWorkflows []string
}
// DownloadWorkflowLogs downloads and analyzes workflow logs with metrics.
// This wrapper preserves the existing call signature for backward compatibility.
func DownloadWorkflowLogs(ctx context.Context, workflowName string, count int, startDate, endDate, outputDir, engine, ref string, beforeRunID, afterRunID int64, repoOverride string, verbose bool, toolGraph bool, noStaged bool, firewallOnly bool, noFirewall bool, parse bool, jsonOutput bool, timeout int, summaryFile string, safeOutputType string, filteredIntegrity bool, train bool, format string, artifactSets []string, excludeWorkflows []string) error {
return DownloadWorkflowLogsWithOptions(ctx, DownloadWorkflowLogsOptions{
WorkflowName: workflowName,
Count: count,
StartDate: startDate,
EndDate: endDate,
OutputDir: outputDir,
Engine: engine,
Ref: ref,
BeforeRunID: beforeRunID,
AfterRunID: afterRunID,
RepoOverride: repoOverride,
Verbose: verbose,
ToolGraph: toolGraph,
NoStaged: noStaged,
FirewallOnly: firewallOnly,
NoFirewall: noFirewall,
Parse: parse,
JSONOutput: jsonOutput,
Timeout: timeout,
SummaryFile: summaryFile,
SafeOutputType: safeOutputType,
FilteredIntegrity: filteredIntegrity,
Train: train,
Format: format,
ArtifactSets: artifactSets,
ExcludeWorkflows: excludeWorkflows,
})
}
// DownloadWorkflowLogsWithOptions downloads and analyzes workflow logs with metrics using an options struct.
func DownloadWorkflowLogsWithOptions(ctx context.Context, opts DownloadWorkflowLogsOptions) error {
workflowName := opts.WorkflowName
count := opts.Count
startDate := opts.StartDate
endDate := opts.EndDate
outputDir := opts.OutputDir
engine := opts.Engine
ref := opts.Ref
beforeRunID := opts.BeforeRunID
afterRunID := opts.AfterRunID
repoOverride := opts.RepoOverride
verbose := opts.Verbose
toolGraph := opts.ToolGraph
noStaged := opts.NoStaged
firewallOnly := opts.FirewallOnly
noFirewall := opts.NoFirewall
parse := opts.Parse
jsonOutput := opts.JSONOutput
timeout := opts.Timeout
summaryFile := opts.SummaryFile
safeOutputType := opts.SafeOutputType
filteredIntegrity := opts.FilteredIntegrity
train := opts.Train
format := opts.Format
artifactSets := opts.ArtifactSets
excludeWorkflows := opts.ExcludeWorkflows
🧪 Test Quality Sentinel ReportTest Quality Score: 65/100
Test Classification DetailsView All Test Classifications
Flagged Tests — Requires Review
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
❌ Test Quality Sentinel: 65/100. 50% of new tests are classified as low-value implementation tests, exceeding the 30% threshold. TestLogsCommandExcludeFlag only verifies flag registration metadata — please add a test that exercises the actual exclude filtering behavior (e.g., passing --exclude values and asserting that matching workflows are skipped). See the comment above for a suggested improvement.
…command Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
|
Commit pushed:
|
🏗️ Design Decision Gate — ADR RequiredThis PR makes significant changes to core business logic (159 new lines in AI has analyzed the PR diff and generated a draft ADR to help you get started: 📄 Draft ADR: What to do next
Once an ADR is linked in the PR body, this gate will re-run and verify the implementation matches the decision. Why ADRs Matter
ADRs create a searchable, permanent record of why the codebase looks the way it does. Future contributors (and your future self) will thank you. 📋 Draft ADR PreviewTitle: ADR-28452: Pre-Download Workflow Exclusion Filter for the Logs Command Key decision: Add a Alternatives captured in the draft:
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §24930818842
|
|
@copilot merge main and recompile Review all comments |
…ommand-exclude-workflows # Please enter a commit message to explain why this merge is necessary, # especially if it merges an updated upstream into a topic branch. # # Lines starting with '#' will be ignored, and an empty message aborts # the commit. Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…ug matching, merge main Agent-Logs-Url: https://github.com/github/gh-aw/sessions/ec5f3e7a-405a-4d8f-9118-d136c60250b0 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done (commit
Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
|
@copilot merge main and recompile |
…ommand-exclude-workflows Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done (commit Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
Summary
Adds a new
--excludeflag to thegh aw logscommand and a correspondingexclude_workflowsparameter to the MCPlogstool, allowing users to skip specified workflows when downloading logs.Excluded runs are filtered before artifact download, which avoids unnecessary GitHub API requests and artifact download network traffic for the excluded workflows.
Changes
pkg/cli/logs_command.go: New--exclude(StringSlice) flag. Supports comma-separated values or repeated--excludeflags. CallsDownloadWorkflowLogsWithOptionsdirectly with named fields for clarity.pkg/cli/logs_orchestrator.go: IntroducesDownloadWorkflowLogsOptionsstruct grouping all parameters.DownloadWorkflowLogsWithOptionsis the primary implementation;DownloadWorkflowLogsis a thin backward-compatible wrapper. The resolved exclude list is applied to each batch of runs beforedownloadRunArtifactsConcurrentis called.pkg/cli/logs_utils.go: Two helpers:resolveExcludeWorkflows: loadsworkflow.GetAllWorkflows()once and builds an in-memory map (keyed by display name and workflow ID, case-insensitive) to batch-resolve all excludes without repeated filesystem scans. Falls back to keeping the raw value when resolution fails.isWorkflowExcluded: case-insensitive comparison against both the display name and a slugified form (spaces→hyphens), so--exclude weekly-researchcorrectly matches a run named"Weekly Research"even when no local lock files are present.pkg/cli/mcp_tools_privileged.go: Newexclude_workflows []stringfield in thelogsArgsstruct with JSON schema description. Passes--excludeto the CLI subprocess.Usage
Tests
TestLogsCommandExcludeFlag: verifies the flag is registered with correct type and defaultTestIsWorkflowExcluded: table-driven tests for exact match, case-insensitive match, slug match (ID matching display name), no match, empty excludes, and multiple excludes