fix(detector): sort previous JSON dirs in descending order for diff#2555
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull request overview
Fixes report --diff-plus selecting an older “previous” scan by ensuring detector.ListValidJSONDirs returns result directories in descending (newest-first) order, matching the documented behavior and reporter.ListValidJSONDirs.
Changes:
- Sort
detector.ListValidJSONDirsresults in descending order usingslices.SortFunc+cmp.Compare. - Add a unit test ensuring the returned directory list is newest-first and ignores non-timestamp dirs.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated no comments.
| File | Description |
|---|---|
| detector/util.go | Changes directory sorting to newest-first so diff logic selects the correct prior scan. |
| detector/util_test.go | Adds coverage verifying descending sort order and filtering of invalid directory names. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
ListValidJSONDirs in detector was switched from a descending sort.Slice to slices.Sort in #2415, which sorts ascending. The doc comment and loadPrevious both assume descending order — loadPrevious takes dirs[1:] expecting the second-newest dir as the previous scan, but with the ascending sort it picks the second-oldest, so report --diff-plus compares against an ancient scan instead of the latest prior one. Match reporter/util.go's ListValidJSONDirs by sorting descending. Reported in #2536 Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Reproduces the scenario from discussion #2536 — when many timestamped result dirs exist, the newest must come first so loadPrevious picks the second-newest as the previous scan. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
07a16b2 to
7df1c49
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes #2536 —
report --diff-pluskeeps re-sending the same unfixed alerts because it compares the current scan against an ancient one (the second-oldest result dir) instead of the most recent previous scan.Root cause
detector.ListValidJSONDirssorts ascending, sodirs[0]is the oldest dir.loadPreviousthen iteratesdirs[1:]and breaks on the first matching JSON, which makes it pick the second-oldest as "previous". The function's own doc comment says the opposite ("recent directories are at the head"), and the siblingreporter.ListValidJSONDirssorts descending.This is a regression from #2415 (Go 1.26 bump), where the original
was modernized to
Fix
Match
reporter/util.go's descending sort:Verification — before / after
Added
TestListValidJSONDirs_SortedDescendingthat recreates the directory list from the discussion and assertsdirs[0]is the newest. Running the same test against master vs. this branch:Before (on master, broken sort):
Note that
dirs[1] = 2026-04-25T08-27-25+0000— exactly the path the discussion reporter saw inPrevious json found: ....After (this branch):
Test plan
go build ./...go test ./detector/ -run TestListValidJSONDirs_SortedDescending -v— PASS on this branch, FAIL on master (see above)🤖 Generated with Claude Code
References