feat: add --exclude flag to logs command and MCP tool to skip specified workflows#28477
feat: add --exclude flag to logs command and MCP tool to skip specified workflows#28477
Conversation
…ed workflows Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c7d0d9fa-9662-4e6c-87cb-405514757d34 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/c7d0d9fa-9662-4e6c-87cb-405514757d34 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 gh aw logs (and the MCP logs tool) so specified workflows can be skipped before artifact download, reducing unnecessary API calls and network traffic.
Changes:
- Added
--exclude(StringSlice) togh aw logsand plumbed it through a newDownloadWorkflowLogsOptionsstruct-based entrypoint. - Implemented exclude resolution + matching helpers and applied filtering prior to artifact downloads.
- Extended the MCP privileged
logstool withexclude_workflowsand added unit tests covering the new behavior.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_command.go | Adds --exclude flag and passes it via DownloadWorkflowLogsOptions. |
| pkg/cli/logs_orchestrator.go | Introduces DownloadWorkflowLogsOptions / DownloadWorkflowLogsWithOptions and applies pre-download exclusion filtering. |
| pkg/cli/logs_utils.go | Adds helpers to resolve excludes against local workflows and perform case-insensitive/slug matching. |
| pkg/cli/mcp_tools_privileged.go | Adds exclude_workflows to MCP logs tool args and forwards it to the CLI via --exclude. |
| pkg/cli/logs_command_test.go | Adds tests for --exclude flag registration and isWorkflowExcluded matching behavior. |
| pkg/cli/logs_json_stderr_order_test.go | Updates calls to include the new exclude argument. |
| pkg/cli/logs_ci_scenario_test.go | Updates calls to include the new exclude argument. |
| pkg/cli/context_cancellation_test.go | Updates calls to include the new exclude argument. |
| pkg/cli/logs_download_test.go | Updates calls to include the new exclude argument. |
| docs/adr/28452-workflow-exclusion-filter-for-logs-command.md | Adds ADR documenting the workflow exclusion decision/spec. |
Copilot's findings
Tip
Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comments suppressed due to low confidence (1)
docs/adr/28452-workflow-exclusion-filter-for-logs-command.md:60
- The normative spec says implementations MUST pass the exclude list to
DownloadWorkflowLogsvia anexcludeWorkflowsparameter, but the CLI implementation callsDownloadWorkflowLogsWithOptionsdirectly. Update the spec to match the actual public entrypoint(s) (e.g., require populatingDownloadWorkflowLogsOptions.ExcludeWorkflowsand/or define which function is canonical).
### Exclusion Flag and Parameter
1. The `logs` CLI command **MUST** expose an `--exclude` flag of type `StringSlice` that accepts one or more workflow names or IDs, either comma-separated or via repeated flags.
2. The MCP `logs` tool **MUST** expose an `exclude_workflows` array field in its argument struct with a JSON schema description.
3. Implementations **MUST** pass the exclude list to `DownloadWorkflowLogs` as the `excludeWorkflows []string` parameter.
- Files reviewed: 10/10 changed files
- Comments generated: 2
| } | ||
|
|
||
| // DownloadWorkflowLogs downloads and analyzes workflow logs with metrics. | ||
| // It is a thin wrapper around DownloadWorkflowLogsWithOptions for backward compatibility. |
There was a problem hiding this comment.
The comment says this wrapper is for backward compatibility, but the function signature itself has changed by adding excludeWorkflows []string, which is a breaking change for any external callers. Either keep the old DownloadWorkflowLogs signature and route new functionality through DownloadWorkflowLogsWithOptions, or update the comment to avoid claiming backward compatibility.
| // It is a thin wrapper around DownloadWorkflowLogsWithOptions for backward compatibility. | |
| // It is a thin wrapper around DownloadWorkflowLogsWithOptions that maps the | |
| // positional arguments into DownloadWorkflowLogsOptions. |
| #### Negative | ||
| - `DownloadWorkflowLogs` gains yet another parameter, worsening an already long function signature. This may be a motivation for future refactoring of the signature into a struct. | ||
| - If lock files are absent or out of date, `resolveExcludeWorkflows` silently falls back to raw-value matching, which may produce unexpected results without a clear error to the user. |
There was a problem hiding this comment.
This ADR notes that adding excludeWorkflows worsens the long DownloadWorkflowLogs signature and suggests a future refactor into a struct, but this PR already introduces DownloadWorkflowLogsOptions. The ADR should be updated to reflect the implemented design (options struct as primary API) to avoid misleading future readers.
This issue also appears on line 55 of the same file.
✅ Design Decision Gate — ADR VerifiedThe implementation in this PR aligns with the stated Architecture Decision Record. ADR reviewed: ADR-28452: Pre-Download Workflow Exclusion Filter for the Logs Command Verification SummaryAll normative requirements from the ADR are satisfied:
📋 Scope note: Options struct refactoringThe ADR lists the long The design decision has been recorded and the implementation follows it. Great work! 🏗️
|
🧪 Test Quality Sentinel ReportTest Quality Score: 65/100
Test Classification Details
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 (threshold: 30%). TestLogsCommandExcludeFlag only verifies flag registration metadata — it doesn't test that the --exclude flag actually filters workflows. Please add a behavioral end-to-end test before merging. See the comment above for a detailed breakdown and suggested improvement.
|
@copilot delete adr file, recompile Add tests |
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/79e0cf8d-045f-40b2-b9e0-b67b09a39cf3 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in 81b4a3f:
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, fix conflicts, recompile |
…ges-from-28452 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Done in d6f4dba — merged main (no conflicts), rebuilt the binary, and recompiled all 202 workflow files successfully. 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
Recreates the changes from #28452.
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 wrapper that maps positional arguments intoDownloadWorkflowLogsOptions. 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 excludesTestResolveExcludeWorkflows: table-driven tests for nil/empty input, blank-entry trimming, unknown workflow fallback, and whitespace stripping