diff --git a/.changeset/patch-fix-logs-command-filtering.md b/.changeset/patch-fix-logs-command-filtering.md new file mode 100644 index 0000000000..93f92cd1b5 --- /dev/null +++ b/.changeset/patch-fix-logs-command-filtering.md @@ -0,0 +1,5 @@ +--- +"gh-aw": patch +--- + +Fixed logs command not returning the requested number of workflow runs when no workflow name is specified diff --git a/LOGS_FILTERING_FIX.md b/LOGS_FILTERING_FIX.md new file mode 100644 index 0000000000..a1866dd147 --- /dev/null +++ b/LOGS_FILTERING_FIX.md @@ -0,0 +1,82 @@ +# Logs Command Filtering Fix + +## Problem Description + +The `logs` command was not finding all workflow runs when no workflow name was specified. + +**Reproduction:** +```bash +./gh-aw logs tidy -c 10 # Returns 10 runs +./gh-aw logs -c 10 # Returns fewer than 10 runs (inconsistent) +``` + +## Root Cause + +The issue was in the pagination logic in `pkg/cli/logs.go`: + +1. `listWorkflowRunsWithPagination()` fetches workflow runs from GitHub API +2. When no workflow name is specified, it filters results to only agentic workflows +3. The iteration loop checked if `len(filteredRuns) < batchSize` to detect end of data +4. This caused premature termination when few agentic workflows were in a batch + +**Example scenario:** +- Request 10 agentic workflow runs +- First batch: Fetch 250 runs from API → Only 5 are agentic workflows after filtering +- **Bug**: `len(filteredRuns)=5 < batchSize=250` → Stop iteration ❌ +- **Expected**: Continue iterating to find more agentic workflows ✓ + +## Solution + +Modified `listWorkflowRunsWithPagination()` to return two values: +1. Filtered workflow runs (agentic only when no workflow name specified) +2. Total count fetched from GitHub API (before filtering) + +Changed the end-of-data check from: +```go +if len(runs) < batchSize { // WRONG: uses filtered count + break +} +``` + +To: +```go +if totalFetched < batchSize { // CORRECT: uses API response count + break +} +``` + +This ensures iteration continues until: +- We have enough agentic workflow runs, OR +- We truly reach the end of GitHub data (API returns fewer than requested) + +## Files Changed + +1. **pkg/cli/logs.go** + - Modified `listWorkflowRunsWithPagination()` signature to return `([]WorkflowRun, int, error)` + - Added `totalFetched` tracking before filtering + - Updated end-of-data check to use `totalFetched` + - Added comprehensive comments explaining the fix + +2. **pkg/cli/logs_test.go** + - Updated test to handle new return value from `listWorkflowRunsWithPagination()` + +3. **pkg/cli/logs_filtering_test.go** (new) + - Added documentation tests explaining the expected behavior + - Tests are skipped (network-dependent) but serve as documentation + +## Testing + +All existing unit tests pass: +```bash +make test-unit # ✓ All tests pass +make lint # ✓ No issues +make build # ✓ Compiles successfully +``` + +## Impact + +This fix ensures consistent behavior: +- `./gh-aw logs -c 10` now returns 10 agentic workflow runs (not fewer) +- `./gh-aw logs tidy -c 10` behavior unchanged (still returns 10 runs) +- No performance impact (still uses efficient batch fetching) +- No breaking changes (CLI interface unchanged) diff --git a/pkg/cli/logs.go b/pkg/cli/logs.go index 34a3c19a02..9abc0cc3ce 100644 --- a/pkg/cli/logs.go +++ b/pkg/cli/logs.go @@ -450,7 +450,7 @@ func DownloadWorkflowLogs(workflowName string, count int, startDate, endDate, ou } } - runs, err := listWorkflowRunsWithPagination(workflowName, batchSize, startDate, endDate, beforeDate, branch, beforeRunID, afterRunID, verbose) + runs, totalFetched, err := listWorkflowRunsWithPagination(workflowName, batchSize, startDate, endDate, beforeDate, branch, beforeRunID, afterRunID, verbose) if err != nil { return err } @@ -613,7 +613,13 @@ func DownloadWorkflowLogs(workflowName string, count int, startDate, endDate, ou } // If we got fewer runs than requested in this batch, we've likely hit the end - if len(runs) < batchSize { + // IMPORTANT: Use totalFetched (API response size before filtering) not len(runs) (after filtering) + // to detect end. When workflowName is empty, runs are filtered to only agentic workflows, + // so len(runs) may be much smaller than totalFetched even when more data is available from GitHub. + // Example: API returns 250 total runs, but only 5 are agentic workflows after filtering. + // Old buggy logic: len(runs)=5 < batchSize=250, stop iteration (WRONG - misses more agentic workflows!) + // Fixed logic: totalFetched=250 < batchSize=250 is false, continue iteration (CORRECT) + if totalFetched < batchSize { if verbose { fmt.Fprintln(os.Stderr, console.FormatInfoMessage("Received fewer runs than requested, likely reached end of available runs")) } @@ -773,7 +779,14 @@ func downloadRunArtifactsConcurrent(runs []WorkflowRun, outputDir string, verbos } // listWorkflowRunsWithPagination fetches workflow runs from GitHub with pagination support -func listWorkflowRunsWithPagination(workflowName string, count int, startDate, endDate, beforeDate, branch string, beforeRunID, afterRunID int64, verbose bool) ([]WorkflowRun, error) { +// Returns: +// - []WorkflowRun: filtered workflow runs (agentic workflows only when workflowName is empty) +// - int: total count of runs fetched from GitHub API before filtering +// - error: any error that occurred +// +// The totalFetched count is critical for pagination - it indicates whether more data is available +// from GitHub, whereas the filtered runs count may be much smaller after filtering for agentic workflows. +func listWorkflowRunsWithPagination(workflowName string, count int, startDate, endDate, beforeDate, branch string, beforeRunID, afterRunID int64, verbose bool) ([]WorkflowRun, int, error) { args := []string{"run", "list", "--json", "databaseId,number,url,status,conclusion,workflowName,createdAt,startedAt,updatedAt,event,headBranch,headSha,displayTitle"} // Add filters @@ -829,19 +842,22 @@ func listWorkflowRunsWithPagination(workflowName string, count int, startDate, e strings.Contains(combinedMsg, "To use GitHub CLI in a GitHub Actions workflow") || strings.Contains(combinedMsg, "authentication required") || strings.Contains(outputMsg, "gh auth login") { - return nil, fmt.Errorf("GitHub CLI authentication required. Run 'gh auth login' first") + return nil, 0, fmt.Errorf("GitHub CLI authentication required. Run 'gh auth login' first") } if len(output) > 0 { - return nil, fmt.Errorf("failed to list workflow runs: %s", string(output)) + return nil, 0, fmt.Errorf("failed to list workflow runs: %s", string(output)) } - return nil, fmt.Errorf("failed to list workflow runs: %w", err) + return nil, 0, fmt.Errorf("failed to list workflow runs: %w", err) } var runs []WorkflowRun if err := json.Unmarshal(output, &runs); err != nil { - return nil, fmt.Errorf("failed to parse workflow runs: %w", err) + return nil, 0, fmt.Errorf("failed to parse workflow runs: %w", err) } + // Store the total count fetched from API before filtering + totalFetched := len(runs) + // Filter only agentic workflow runs when no specific workflow is specified // If a workflow name was specified, we already filtered by it in the API call var agenticRuns []WorkflowRun @@ -850,7 +866,7 @@ func listWorkflowRunsWithPagination(workflowName string, count int, startDate, e // Get the list of agentic workflow names from .lock.yml files agenticWorkflowNames, err := getAgenticWorkflowNames(verbose) if err != nil { - return nil, fmt.Errorf("failed to get agentic workflow names: %w", err) + return nil, 0, fmt.Errorf("failed to get agentic workflow names: %w", err) } for _, run := range runs { @@ -880,7 +896,7 @@ func listWorkflowRunsWithPagination(workflowName string, count int, startDate, e agenticRuns = filteredRuns } - return agenticRuns, nil + return agenticRuns, totalFetched, nil } // flattenSingleFileArtifacts applies the artifact unfold rule to downloaded artifacts diff --git a/pkg/cli/logs_filtering_test.go b/pkg/cli/logs_filtering_test.go new file mode 100644 index 0000000000..73875ac716 --- /dev/null +++ b/pkg/cli/logs_filtering_test.go @@ -0,0 +1,41 @@ +package cli + +import ( + "testing" +) + +// TestListWorkflowRunsWithPagination_ReturnsTotalFetchedCount verifies that +// the function returns both the filtered runs and the total count fetched from API +func TestListWorkflowRunsWithPagination_ReturnsTotalFetchedCount(t *testing.T) { + t.Skip("Skipping network-dependent test - this verifies the fix for filtering issue") + + // This test would require actual GitHub CLI access to work properly + // The key insight is that the function should return: + // 1. Filtered runs (e.g., 5 agentic workflows) + // 2. Total fetched count (e.g., 250 total runs from API) + // + // This allows the caller to properly detect when it has reached the end + // of available runs by checking totalFetched < batchSize, not len(runs) < batchSize + + // Example scenario that would fail with old code: + // - Request 250 runs from GitHub API + // - API returns 250 runs (mix of agentic and non-agentic) + // - Only 5 are agentic workflows after filtering + // - Old code: checks len(runs)=5 < batchSize=250, stops iteration incorrectly + // - New code: checks totalFetched=250 < batchSize=250 is false, continues iteration +} + +// TestDownloadWorkflowLogs_IteratesUntilEnoughRuns demonstrates the fixed behavior +func TestDownloadWorkflowLogs_IteratesUntilEnoughRuns(t *testing.T) { + t.Skip("Skipping network-dependent test - this would verify end-to-end behavior") + + // This test would verify that when calling: + // ./gh-aw logs -c 10 (no workflow name) + // + // The function: + // 1. Fetches batches of runs until it has 10 agentic workflow runs + // 2. Continues iterating if first batch has few agentic workflows + // 3. Only stops when totalFetched < batchSize (reached end of GitHub data) + // 4. Returns the same number of results as: + // ./gh-aw logs tidy -c 10 (specific workflow name) +} diff --git a/pkg/cli/logs_test.go b/pkg/cli/logs_test.go index 12b5cb4ced..55c5a4cd39 100644 --- a/pkg/cli/logs_test.go +++ b/pkg/cli/logs_test.go @@ -361,7 +361,7 @@ func TestListWorkflowRunsWithPagination(t *testing.T) { // This should fail with authentication error (if not authenticated) // or succeed with empty results (if authenticated but no workflows match) - runs, err := listWorkflowRunsWithPagination("nonexistent-workflow", 5, "", "", "2024-01-01T00:00:00Z", "", 0, 0, false) + runs, _, err := listWorkflowRunsWithPagination("nonexistent-workflow", 5, "", "", "2024-01-01T00:00:00Z", "", 0, 0, false) if err != nil { // If there's an error, it should be an authentication error or workflow not found