Retain cached JSONL history within requested date ranges - #60361
Conversation
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
There was a problem hiding this comment.
🟡 Changes recommended
Stdin mode bypasses filtering, and unknown-schema run records can be incorrectly deleted.
Get a fresh assessment by requesting another Copilot review.
Pull request overview
Adds post-collection date-range pruning for cached JSONL history while retaining unrelated records and file permissions.
Changes:
- Filters cached run records after single- and multi-target collection.
- Implements inclusive date bounds and atomic rewriting.
- Updates CLI help and adds cache-filtering tests.
File summaries
| File | Description |
|---|---|
pkg/cli/logs_orchestrator.go |
Filters single-target caches after collection. |
pkg/cli/logs_multi.go |
Filters multi-target caches after collection. |
pkg/cli/logs_command.go |
Documents date-range cache retention. |
pkg/cli/logs_cached_json.go |
Implements atomic date filtering. |
pkg/cli/logs_cached_json_test.go |
Tests filtering, preservation, and permissions. |
Review details
- Files reviewed: 5/5 changed files
- Comments generated: 2
- Review effort level: Balanced
💡 Add a code-review agent skill for context-aware, tailored reviews. Learn more in the docs.
| if err := json.Unmarshal(trimmed, &record); err != nil || | ||
| record.Kind != cachedLogsJSONLKindRun || | ||
| record.Run == nil || | ||
| record.Run.CreatedAt.IsZero() { | ||
| filtered = append(filtered, line...) |
There was a problem hiding this comment.
Fixed in 4e5... filterDateRange now requires record.SchemaVersion == cachedLogsJSONLSchemaVersion for run records, so unknown/future-schema run records are preserved. Added a regression test with a schema_version: 99 run record in the excluded range that stays in the file.
| artifacts again. New results are appended immediately as JSON Lines. When a date range is specified, | ||
| cached run records outside that range are removed after collection; other record types are retained. |
There was a problem hiding this comment.
Fixed in a9707d7: StartDate/EndDate are now plumbed through StdinLogsOptions, and DownloadWorkflowLogsFromStdin defers a call to cachedJSONLWriter.filterDateRange mirroring the discovery-mode path. Added a regression test exercising --stdin with a cached JSONL and date range.
|
✅ Ponytail Reviewer completed successfully! 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.
|
|
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.
|
|
|
🧠 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
|
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
There was a problem hiding this comment.
Ponytail review: focused on over-engineering only.
net: -0 lines possible.
Warning
Firewall blocked 1 domain
The following domain was blocked by the firewall during workflow execution:
ab.chatgpt.com
To allow these domains, add them to the network.allowed list in your workflow frontmatter:
network:
allowed:
- defaults
- "ab.chatgpt.com"See Network Configuration for more information.
Generated by ✂️ Ponytail Reviewer for #60361 · codex · gpt53codex · 4.32 AIC · ⌖ 3.98 AIC · ⊞ 12.8K
Comment /ponytail to run again
ADR RequiredI could not find an existing ADR in the PR body or on the PR branch that covers this change using the required Michael Nygard sections (Context, Decision, Alternatives Considered, Consequences). Evidence used:
I added a draft ADR at Next action: review and refine that ADR, then keep it with the PR so this decision is explicit before merge.
|
There was a problem hiding this comment.
Skills-Based Review 🧠
Applied /diagnosing-bugs — the core append/filter lifecycle logic and test are solid, but two edge cases undermine the correctness guarantee this PR advertises.
📋 Key Themes & Highlights
Key Themes
--stdinpath is silently unguarded:loadStdinLogsOptionsdropsStartDate/EndDate, andDownloadWorkflowLogsFromStdinnever callsfilterDateRange. The updated--cached-jsonldoc claims cached run records outside the range are always removed after collection — that's false for--stdin. This was also flagged by an earlier bot review comment on the same line.- Schema-version blind spot in
filterDateRange: the preservation check treats any record that fails to unmarshal cleanly into the current run shape as safe-to-keep viaCreatedAt.IsZero(), but a future-schema run record with a validcreated_atwould be misidentified and could be incorrectly dropped or kept for the wrong reason. Also flagged by an earlier bot comment.
Positive Highlights
- ✅ Atomic filter-then-write via
writeFileAtomically, with a no-op short-circuit (bytes.Equal) when nothing changed — avoids unnecessary rewrites. - ✅ Good regression test (
TestCachedLogsJSONLWriterFiltersAppendedContentByDateRange) exercising boundary dates, undated records, workflow-list/rate-limit/unknown record preservation, and file permission retention. - ✅
defer+errors.Joinpattern inlogs_multi.go/logs_orchestrator.gocleanly ties filtering to the collection lifecycle without obscuring the original error.
Requesting changes to close the --stdin gap (or explicitly document/reject it) and to make the schema-version guard explicit with a regression test, per the inline comments.
@copilot please address the review comments above.
🧠 Reviewed using Matt Pocock's skills by Matt Pocock Skills Reviewer · copilot · sonnet50 · 64 AIC · ⌖ 14.1 AIC · ⊞ 10.4K
Comment /matt to run again
| record.Kind != cachedLogsJSONLKindRun || | ||
| record.Run == nil || | ||
| record.Run.CreatedAt.IsZero() { | ||
| filtered = append(filtered, line...) |
There was a problem hiding this comment.
[/diagnosing-bugs] filterDateRange only pattern-matches on Kind == cachedLogsJSONLKindRun, ignoring SchemaVersion. A future-schema run record (e.g. schema_version: 99) will unmarshal into the current struct with zero/garbage fields, likely leaving CreatedAt zero, so it's preserved by this check — but it's silently mis-parsed rather than genuinely 'unknown and safe to keep'. This mirrors a bot review comment already on this line.
💡 Suggested fix
Guard on record.SchemaVersion != cachedLogsJSONLSchemaVersion explicitly (treat unrecognized/newer schema versions as opaque/preserved, the same way unknown kinds are handled), rather than relying on CreatedAt.IsZero() as an implicit signal. Add a regression test with a run record at a future schema_version containing a valid created_at in the excluded range, and assert it is still preserved (proving the schema check, not an accidental zero-value, is what protects it).
@copilot please address this.
There was a problem hiding this comment.
Fixed in a9707d7: filterDateRange now requires record.SchemaVersion == cachedLogsJSONLSchemaVersion for run records before pruning, so future-schema run records are preserved. Added a regression test with a schema_version: 99 run record in the excluded range.
| artifacts again. New results are appended immediately as JSON Lines. Aggregate analysis may be approximate when compact | ||
| cached records omit detailed data. | ||
| artifacts again. New results are appended immediately as JSON Lines. When a date range is specified, | ||
| cached run records outside that range are removed after collection; other record types are retained. |
There was a problem hiding this comment.
[/diagnosing-bugs] This doc says cached run records outside the requested range are always removed, but loadStdinLogsOptions (line ~225) drops StartDate/EndDate when building StdinLogsOptions, and DownloadWorkflowLogsFromStdin never calls filterDateRange at all. Using --stdin --cached-jsonl --start-date/--end-date silently skips pruning, contradicting this doc and the PR description's stated guarantee. Flagged previously by another reviewer on this same line.
💡 Suggested fix
Either (a) plumb StartDate/EndDate through to StdinLogsOptions and call cachedJSONLWriter.filterDateRange in DownloadWorkflowLogsFromStdin (mirroring the defer pattern added in logs_multi.go/logs_orchestrator.go), or (b) explicitly document/reject the combination for the stdin path. Add a regression test exercising --stdin with a cached JSONL and date range to lock in whichever behavior is chosen — right now there's no test covering this path at all.
@copilot please address this.
There was a problem hiding this comment.
Fixed in a9707d7: StartDate/EndDate now flow through StdinLogsOptions and DownloadWorkflowLogsFromStdin filters the cache on completion, matching the discovery-mode guarantee described in the docs. Added a regression test.
|
@copilot run pr-finisher skill |
…iltering Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
Co-authored-by: pelikhan <4175913+pelikhan@users.noreply.github.com>
|
🎉 This pull request is included in a new release. Release: |
logs --cached-jsonlappended new records without pruning cached runs outside the requested time range.Cache lifecycle
Filtering
--start-dateand--end-datebounds.