Read latest run history first

[thomhurst]

Closes #3754

Summary

• order matching history files newest-first using their encoded timestamps
• return after the first compatible report for the requested pipeline identity
• deserialize from an asynchronous FileStream while preserving the forward-schema compatibility gate
• cover early exit and newest-invalid fallback behavior

Validation

• build ModularPipelines.slnx -c Release (0 warnings, 0 errors)
• 11 FileSystemHistoryStore* tests passed
• formatting verification passed for both changed files
• git diff --check passed

The unit-test project currently needs the pending PipelineOptions.Console initializer correction from #3844 to compile; that unrelated temporary local adjustment was removed and is not part of this PR.

[thomhurst]

@claude review

[claude]

Code review

No issues found. Checked for bugs and CLAUDE.md compliance.

[thomhurst]

The shared RunReportTests regression is fixed on current main by merged PR #3844. Refreshed this branch onto current main for clean CI and review.

[thomhurst]

@codex review

[thomhurst]

@claude review

[chatgpt-codex-connector]

Codex Review: Didn't find any major issues. Swish!

Reviewed commit: 73d7dddb6e

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[claude]

Code review

Reviewed commit 73d7dddb6e.

The change is correct and well tested. Ordering by the filename-encoded timestamp (OrderByDescending(GetHistoryTimestamp).ThenByDescending(filename)) and returning on the first schema-compatible, identity-matching report is equivalent to the old "scan everything and keep the max End" approach, because SaveAsync always derives the filename timestamp from report.End — so the newest filename is guaranteed to be the newest report. The switch from File.ReadAllTextAsync + JsonDocument.Parse to a FileStream + JsonDocument.ParseAsync preserves the existing schema-compatibility gate and exception handling (IOException/UnauthorizedAccessException/JsonException still caught and logged per-file). The new FileSystemHistoryStoreStopsAfterNewestCompatibleReport test is a good regression guard for the early-exit behavior, and the updated FileSystemHistoryStoreSkipsInvalidSchemaMetadata test correctly repositions invalid content into the newest files to exercise the new short-circuit path.

Two non-blocking suggestions:

• Duplicated ordering logic: GetLatestAsyncCore and PruneFiles now both contain the identical .OrderByDescending(GetHistoryTimestamp).ThenByDescending(static file => Path.GetFileName(file), StringComparer.Ordinal) chain. Worth extracting into a small shared helper (e.g. GetFilesNewestFirst(directory, searchPattern)) so the "what counts as newest" definition can't drift between the read path and the prune path if one is changed later without the other.
• RunReportJsonSerializer.Deserialize: after this change it's no longer called from production code (only from tests) — FileSystemRunHistoryStore now deserializes directly via document.RootElement.Deserialize(...). Consider either reusing the helper for consistency or removing it if it's no longer needed outside tests.

No CLAUDE.md violations or correctness bugs found.

[thomhurst]

Refreshed onto current main after Ubuntu's core test host ran 17m6s and exited without a failing-test summary; all other jobs passed. Validation on a8da17f: RunReportTests 69/69 and ModularPipelines.slnx Release build 0 warnings/errors. A detached worktree preserved the existing divergent local branch untouched.

[claude]

Code review

Reviewed the current head, a8da17fbe6. It's byte-identical in both changed files (FileSystemRunHistoryStore.cs, RunReportTests.cs) to 73d7dddb6e, which I reviewed in detail earlier on this PR — this update was just a rebase/merge onto main, no new code changes to review.

That earlier review's conclusion stands: the newest-first ordering + early-return logic is correct and equivalent to the old "scan everything, keep max End" behavior, since SaveAsync always derives the filename timestamp from report.End. The FileStream + JsonDocument.ParseAsync switch preserves the schema-compatibility gate and exception handling, and the new/updated tests are solid regression coverage for the early-exit and newest-invalid-fallback paths.

One of the two non-blocking suggestions from before is still outstanding and worth a second look as an architectural point rather than a nitpick:

• Duplicated "newest first" ordering logic. GetLatestAsyncCore (src/ModularPipelines/Engine/FileSystemRunHistoryStore.cs:39-41) and PruneFiles (src/ModularPipelines/Engine/FileSystemRunHistoryStore.cs:174-177) each independently repeat .OrderByDescending(GetHistoryTimestamp).ThenByDescending(static file => Path.GetFileName(file), StringComparer.Ordinal). This isn't just style — it's the single most load-bearing invariant this PR introduces (the whole early-return optimization depends on "newest" meaning the same thing on both the read path and the prune path). Right now that invariant is only true because two independently-maintained LINQ chains happen to match. If a future change adjusts one (e.g. adding a tiebreaker, or switching to a cached/indexed timestamp) without the other, GetLatestAsync and pruning silently disagree on ordering — a bug that unit tests on either method in isolation wouldn't catch, since each would still pass against its own copy of the logic. Extracting a shared IEnumerable<string> GetHistoryFilesNewestFirst(string directory, string searchPattern) used by both call sites would make "newest" a single definition instead of a duplicated convention, and a future test against that one helper would cover both consumers.

The other earlier suggestion (RunReportJsonSerializer.Deserialize now only being exercised by tests, not production code) is unchanged and still just a minor cleanup candidate — not blocking.

No correctness bugs or CLAUDE.md violations found.