Add workflow run ignore list to logs - #59697
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🧠 Matt Pocock Skills Reviewer has completed the skills-based review. ✅
|
|
✅ Test Quality Sentinel completed test quality analysis. Test Quality Sentinel skipped because pre-fetch PR data was unavailable: unable to fetch test file diff
|
|
✅ Ponytail Reviewer completed successfully! Warning Threat Detection Engine Failure — The analysis engine could not complete. This is a tooling failure, not a security finding. What happenedThe threat detection engine failed to produce results. Review the workflow run logs for details. Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
|
|
✅ PR Code Quality Reviewer completed the code quality review. Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "github.com/ghapi"
- "github.com"
- "github.com/ghraw"See Network Configuration for more information.
|
|
✅ Design Decision Gate 🏗️ completed the design decision gate check. See the comment below for the result and any generated ADR draft.
|
There was a problem hiding this comment.
🟡 Changes recommended
The misplaced test function corrupts an existing JSON fixture and causes the test suite to fail.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
Adds workflow-run exclusions to gh aw logs while preserving requested counts and pagination state.
Changes:
- Adds and validates
--ignore-workflow-runs. - Filters ignored runs before artifact processing.
- Persists ignored IDs in continuation data.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_report.go |
Adds ignored IDs to continuation JSON. |
pkg/cli/logs_orchestrator.go |
Propagates exclusions into continuations. |
pkg/cli/logs_orchestrator_types.go |
Extends log collection options. |
pkg/cli/logs_orchestrator_download.go |
Passes exclusions into run fetching. |
pkg/cli/logs_github_api.go |
Filters excluded workflow runs. |
pkg/cli/logs_github_api_test.go |
Adds filtering coverage, but corrupts an existing JSON fixture. |
pkg/cli/logs_command.go |
Adds the flag and input parser. |
pkg/cli/logs_command_test.go |
Tests flag parsing and invalid input. |
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 2
- Review effort level: Balanced
| "updatedAt": "2026-01-01T00:01:00Z" | ||
| } | ||
|
|
||
| func TestFilterIgnoredWorkflowRuns(t *testing.T) { |
| for _, value := range values { | ||
| value = strings.TrimSpace(value) | ||
| if index := strings.LastIndexByte(value, '/'); index >= 0 { | ||
| value = value[index+1:] | ||
| } | ||
| runID, err := strconv.ParseInt(value, 10, 64) | ||
| if err != nil || runID <= 0 { | ||
| return nil, fmt.Errorf("invalid workflow run %q: expected a positive run ID or slug/ID", value) | ||
| } |
There was a problem hiding this comment.
Reviewed via Impeccable-style pass (change_type: mixed_unclear/backend feature, not UI — applied a standard correctness/security review instead). Implementation is solid: parseIgnoredWorkflowRunIDs correctly handles slug/ID parsing, dedup, and validation; filterIgnoredWorkflowRuns follows the existing in-place filter idiom used elsewhere in the same function; the option threads cleanly through orchestrator/continuation/report structs; tests cover both the happy path and the invalid-ID rejection path. No blocking issues found. Minor non-blocking note: docs/src/content/docs/setup/cli.md options list for gh aw logs does not yet mention --ignore-workflow-runs — worth a follow-up doc update, but out of scope for this diff since that file was not touched.
🧵 Reviewed using Impeccable skills by Impeccable Skills Reviewer · copilot · sonnet50 · 23.7 AIC · ⌖ 13.3 AIC · ⊞ 8.3K
🏗️ ADR Required — draft addedI found that this PR requires ADR enforcement because it adds 122 new lines in Evidence reviewed
Decision inferred from the PRThis change introduces Action takenI added a draft ADR to the branch:
Next action for the authorPlease review and refine the ADR so the rationale and trade-offs reflect the intended long-term decision, then keep the implementation aligned with that record.
|
Comment MemoryNote This comment is managed by comment memory.It stores persistent context for this thread in the code block at the top of this comment. Warning Firewall blocked 3 domainsThe following domains were blocked by the firewall during workflow execution:
[!TIP] tools:
github:
mode: gh-proxySee GitHub Tools for more information on To allow these domains, add them to the network:
allowed:
- defaults
- "github.com/ghapi"
- "github.com"
- "github.com/ghraw"See Network Configuration for more information.
|
There was a problem hiding this comment.
Requesting changes
This adds the CLI flag plumbing, but two merge-blocking gaps remain: the resumed MCP logs path drops ignore_workflow_runs, and the only new helper test is accidentally embedded inside a raw JSON fixture, so it never runs.
Blocking themes
- The continuation/report path now advertises
ignore_workflow_runs, butmcp_tools_privileged.gostill cannot accept or forward it. Any MCP client that resumes from the returned cursor silently reprocesses runs that were meant to stay excluded. TestFilterIgnoredWorkflowRunsis insiderawJSON := \[ ... ]``, so it is fixture text, not executable Go. That leaves the new filtering logic effectively untested.
Warning
Firewall blocked 3 domains
The following domains were blocked by the firewall during workflow execution:
github.com/ghapigithub.laiyagushi.comgithub.laiyagushi.com/ghraw
[!TIP]
github.com/ghapi is blocked because GitHub API access uses the built-in GitHub tools by default. Instead of adding github.com/ghapi to network.allowed, use tools.github.mode: gh-proxy for direct pre-authenticated GitHub CLI access without requiring network access to github.com/ghapi:
tools:
github:
mode: gh-proxySee GitHub Tools for more information on gh-proxy mode.
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "github.com/ghapi"
- "github.com"
- "github.com/ghraw"See Network Configuration for more information.
🔎 Code quality review by PR Code Quality Reviewer · copilot · gpt54 · 41.8 AIC · ⌖ 7.49 AIC · ⊞ 23.5K
Comment /review to run again
| "updatedAt": "2026-01-01T00:01:00Z" | ||
| } | ||
|
|
||
| func TestFilterIgnoredWorkflowRuns(t *testing.T) { |
There was a problem hiding this comment.
TestFilterIgnoredWorkflowRuns is inserted inside the raw JSON string in TestWorkflowRunUnmarshal, so it never becomes a Go test and cannot catch regressions in the new ignore filter.
💡 Why this needs to be fixed before merge
In pkg/cli/logs_github_api_test.go, the opening raw string starts on rawJSON := \[` and does not close until after the newly added function. That means lines 35-42 are just fixture text, not executable code. The package test run already passes with zero coverage for the helper you added, which is exactly the kind of false confidence that lets pagination/filter bugs slip through.
Move TestFilterIgnoredWorkflowRuns out of the JSON fixture and keep it as a normal top-level test function. While you are there, add at least one assertion around continuation behavior or count preservation, because the current helper-only check is very shallow for a feature that changes pagination semantics.
| Branch string `json:"branch,omitempty"` | ||
| AfterRunID int64 `json:"after_run_id,omitempty"` | ||
| BeforeRunID int64 `json:"before_run_id,omitempty"` | ||
| IgnoreWorkflowRuns []int64 `json:"ignore_workflow_runs,omitempty"` |
There was a problem hiding this comment.
The continuation payload now persists ignore_workflow_runs, but the MCP logs tool never exposes that field in logsArgs or forwards a --ignore-workflow-runs flag, so any resumed MCP consumer silently drops the ignore list and starts reprocessing the runs this change was supposed to exclude.
💡 Why this blocks the feature
The CLI path wires IgnoreWorkflowRuns through ContinuationData, LogsDownloadOptions, and the paginator, but mcp_tools_privileged.go still defines logsArgs without an ignore_workflow_runs field and appendLogsFilterArgs never appends the new flag. That leaves the public MCP logs API out of sync with the CLI contract: a client that follows the returned continuation cannot preserve exclusions across requests.
A minimal fix is to add an IgnoreWorkflowRuns []int64 field to logsArgs, document it in the schema, and append --ignore-workflow-runs with a comma-joined list in appendLogsFilterArgs. Add a regression test that asserts the subprocess args include the new flag.
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /codebase-design and /tdd — this is a well-scoped feature addition (new --ignore-workflow-runs flag) with solid new-code test coverage (parsing, dedup, filtering, continuation threading). Requesting minor changes on one code-safety issue.
📋 Key Themes & Highlights
Key Themes
- In-place slice mutation risk:
filterIgnoredWorkflowRunsusesruns[:0]to filter in place. In the one call site today this happens to be safe, but it silently aliases/overwrites the caller's slice, which is inconsistent with the neighboring before/after-run-id filter that builds a new slice — flagged as an inline comment. - Slug-scoping ambiguity:
slug/IDparsing discards the owner/repo prefix and matches by run ID only, regardless of which repo is targeted. This may be intentional but is worth documenting explicitly, since the flag help text implies repo-qualified matching. - Good coverage: New unit tests cover flag registration, parsing (dedup + slug forms), invalid input rejection, and the filter helper itself — nice adherence to
/tddpractices for the new surface area. - Continuation correctly threaded:
IgnoreWorkflowRunsis properly persisted throughContinuationDataso a resumed/paginated fetch preserves the ignore list — this is easy to miss and was handled correctly.
Positive Highlights
- ✅ Dedup logic (
slices.Containsbefore append) inparseIgnoredWorkflowRunIDsavoids duplicate IDs cleanly - ✅ Filtering happens before the "no useful agentic data" pruning step, so ignored runs don't affect count semantics unexpectedly
- ✅ Continuation cursor threading is complete across all pagination call sites (
logs_orchestrator.go,logs_orchestrator_download.go)
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 50.5 AIC · ⌖ 14.9 AIC · ⊞ 10.3K
Comment /matt to run again
| @@ -593,6 +597,19 @@ func listWorkflowRunsWithPagination(opts ListWorkflowRunsOptions) ([]WorkflowRun | |||
| return agenticRuns, totalFetched, nil | |||
| } | |||
|
|
|||
There was a problem hiding this comment.
[/codebase-design] filtered := runs[:0] reuses the caller's backing array in place. runs here is agenticRuns, which may itself be an alias of the original runs slice returned by the GitHub API call earlier in this function (when opts.WorkflowName != "", agenticRuns = runs with no copy). Overwriting elements in place is safe for this specific call site today, but it's a fragile invariant future callers of filterIgnoredWorkflowRuns could easily violate (any caller passing a slice they still hold a live reference to will see it silently mutated).
💡 Suggested fix
func filterIgnoredWorkflowRuns(runs []WorkflowRun, ignoredRunIDs []int64) []WorkflowRun {
if len(ignoredRunIDs) == 0 {
return runs
}
filtered := make([]WorkflowRun, 0, len(runs))
for _, run := range runs {
if !slices.Contains(ignoredRunIDs, run.DatabaseID) {
filtered = append(filtered, run)
}
}
return filtered
}The existing before/after-run-id filter a few lines above already follows this safer append-to-new-slice pattern — worth being consistent with it here too.
@copilot please address this.
| } | ||
| runID, err := strconv.ParseInt(value, 10, 64) | ||
| if err != nil || runID <= 0 { | ||
| return nil, fmt.Errorf("invalid workflow run %q: expected a positive run ID or slug/ID", value) |
There was a problem hiding this comment.
[/tdd] parseIgnoredWorkflowRunIDs silently drops the repo/owner portion of a slug/ID value (e.g. github/gh-aw/456 and other-org/other-repo/456 both resolve to run ID 456). Since RepoOverride is a separate flag, a run ID that happens to collide across repos would be ignored even when the user only meant to ignore it in one specific repo.
💡 Suggested test / behavior
Add a test asserting current behavior is intentional (docs say "slug/ID" is accepted but the slug is discarded), or scope the ignore list by repo when --repo differs. At minimum, document in the flag help text that the slug portion is informational only and matching is by numeric run ID alone, to avoid users assuming cross-repo isolation.
@copilot please address this.
|
@copilot run pr-finisher skill |
…or-ignore-workflow-runs # Conflicts: # pkg/cli/logs_github_api.go Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
@copilot Please take the next forward-progress pass on PR #59697. Unresolved review follow-up to address:
Please address the actionable review feedback, run the
Warning Firewall blocked 1 domainThe following domain was blocked by the firewall during workflow execution:
To allow these domains, add them to the network:
allowed:
- defaults
- "github.com"See Network Configuration for more information.
|
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Completed the PR-finisher pass and pushed |
|
🎉 This pull request is included in a new release. Release: |
gh aw logscould not exclude known workflow runs from collection. Add--ignore-workflow-runsto omit specific runs without reducing the requested result count.Input
slug/IDvalues.Collection