Conversation
…n a date delta Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e0abd69b-b722-427e-a51a-de1fc42c8f62 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
…nupCutoff helper Agent-Logs-Url: https://github.com/github/gh-aw/sessions/e0abd69b-b722-427e-a51a-de1fc42c8f62 Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Adds a new --after flag to gh aw logs to clean up cached run-* folders older than a user-provided cutoff before downloading logs, enabling a rolling cache window.
Changes:
- Introduce
parseCleanupCutoffandcleanupOldRunFolderscache-maintenance helpers. - Wire
--afterthrough the logs command into the orchestrator to run cleanup pre-download. - Add/update unit tests to cover the new behavior and update call sites for the new parameter.
Show a summary per file
| File | Description |
|---|---|
| pkg/cli/logs_cache.go | Adds cutoff parsing and cache folder cleanup logic. |
| pkg/cli/logs_orchestrator.go | Adds after parameter and invokes cache cleanup before downloads. |
| pkg/cli/logs_command.go | Registers --after flag, updates help examples, passes flag to orchestrator. |
| pkg/cli/logs_cache_test.go | New unit tests for cleanupOldRunFolders. |
| pkg/cli/logs_command_test.go | Verifies the after flag is registered. |
| pkg/cli/logs_json_stderr_order_test.go | Updates orchestrator call sites with the new after argument. |
| pkg/cli/logs_download_test.go | Updates orchestrator call sites with the new after argument. |
| pkg/cli/logs_ci_scenario_test.go | Updates orchestrator call sites with the new after argument. |
| pkg/cli/context_cancellation_test.go | Updates orchestrator call sites with the new after argument. |
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
| // Clean up cached run folders older than the --after cutoff, if specified. | ||
| if after != "" { | ||
| cutoff, parseErr := parseCleanupCutoff(after) | ||
| if parseErr != nil { | ||
| return parseErr |
There was a problem hiding this comment.
--after cleanup runs before the initial context-cancellation check, so a cancelled context can still trigger potentially expensive directory scanning/deletion. To preserve the existing “check context cancellation at the start” behavior, consider moving the cancellation check above the cleanup block, or pass ctx into cleanupOldRunFolders and bail out early during iteration when ctx.Done() is closed.
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to clean up old run folders: %v", cleanErr))) | ||
| } else if removed > 0 { | ||
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Removed %d cached run folder(s) older than %s", removed, after))) | ||
| } else if verbose { |
There was a problem hiding this comment.
When --json is enabled, this cleanup block can write to stderr (success/info/warn) before the JSON payload is written to stdout (JSON is rendered much later, or in the no-runs path). This can break consumers that redirect stdout+stderr and expect the combined stream to start with valid JSON (see logs_json_stderr_order_test.go). Consider suppressing all console/stderr cleanup messages when jsonOutput is true, or buffering/defer-printing them until after JSON has been emitted.
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to clean up old run folders: %v", cleanErr))) | |
| } else if removed > 0 { | |
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Removed %d cached run folder(s) older than %s", removed, after))) | |
| } else if verbose { | |
| if !jsonOutput { | |
| fmt.Fprintln(os.Stderr, console.FormatWarningMessage(fmt.Sprintf("Failed to clean up old run folders: %v", cleanErr))) | |
| } | |
| } else if removed > 0 { | |
| if !jsonOutput { | |
| fmt.Fprintln(os.Stderr, console.FormatSuccessMessage(fmt.Sprintf("Removed %d cached run folder(s) older than %s", removed, after))) | |
| } | |
| } else if verbose && !jsonOutput { |
| if !strings.HasPrefix(entry.Name(), "run-") { | ||
| continue | ||
| } | ||
|
|
There was a problem hiding this comment.
The PR description says cleanup should only target directories matching the run-{ID} pattern, but the current check only enforces the run- prefix. This can accidentally delete unrelated directories like run-backup or run-temp. Consider validating that the suffix is a valid integer (e.g., strconv.ParseInt(strings.TrimPrefix(name, "run-"), 10, 64)), and skip entries that don’t parse cleanly.
…after flag 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 (284 new lines across The Design Decision Gate 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. 📋 Draft ADR preview — key sectionsDecision captured from the diff:
Alternatives the draft considers:
Trade-offs flagged:
📋 Michael Nygard ADR Format ReferenceAn ADR must contain these four sections to be considered complete:
All ADRs are stored in
References: §25090240769
|
🧪 Test Quality Sentinel ReportTest Quality Score: 80/100✅ Excellent test quality
Test Classification DetailsView all test classifications (3 tests)
Highlights
Minor Suggestions
Language SupportTests analyzed:
Verdict
📖 Understanding Test ClassificationsDesign Tests (High Value) verify what the system does:
Implementation Tests (Low Value) verify how the system does it:
Goal: Shift toward tests that describe the system's behavioral contract — the promises it makes to its users and collaborators. References: §25090240791
|
![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: 80/100. Test quality is excellent — 0% of new tests are implementation tests (threshold: 30%). The new logs_cache_test.go is particularly strong with 7 well-structured table-driven cases covering the full behavioral contract of the --after cleanup feature.
|
@copilot review all comments |
…, validate run-{int} suffix
Agent-Logs-Url: https://github.com/github/gh-aw/sessions/70e1d806-6d60-4d13-b35c-c5cdc7d0f1df
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
All three review comments addressed in commit
|
Summary
Adds a new
--afterflag togh aw logsthat deletes cached run folders in the output directory whose run creation date is older than the specified cutoff. This allows maintaining a rolling window of cached logs on a shared storage location.Usage
The flag accepts the same date/time delta formats as
--start-dateand--end-date(e.g.-1d,-1w,-1mo) as well as absolute dates (YYYY-MM-DD).How it works
run-{ID}naming pattern inside the output directory are considered.run_summary.jsoninside each folder.run_summary.jsonis absent (e.g. incomplete download), the directory's modification time is used as a fallback.Changes
pkg/cli/logs_cache.goparseCleanupCutoffhelper andcleanupOldRunFoldersfunctionpkg/cli/logs_command.go--afterflag definition, examples in help text, pass-through to orchestratorpkg/cli/logs_orchestrator.goDownloadWorkflowLogssignature; callscleanupOldRunFolderswhen--afteris setpkg/cli/logs_cache_test.gocleanupOldRunFolders(7 table-driven cases + verbose test)pkg/cli/logs_command_test.go--afterflag is registered*_test.gofilesafterparameter