Announce run report path and delta baseline - #3874
Conversation
|
@codex review |
|
@claude review |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a520718bef
ℹ️ 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".
| Start = summary.Start, | ||
| End = summary.End, | ||
| TotalDuration = summary.TotalDuration, | ||
| PreviousEnd = previousTotalDuration is null ? null : previousReport!.End, |
There was a problem hiding this comment.
In runs where the previous or current pipeline is not fully successful but individual modules are successful, CreateModuleReport still computes module DurationDelta values from previousReport, so the results table will show the new Δ previous column. Because PreviousEnd is only populated when previousTotalDuration exists, those module-only delta runs have no baseline timestamp and the new footer is omitted even though a previous run was used; this is the same path covered by the failed-run history test where total delta is null but a successful module has PreviousDuration. Populate PreviousEnd from the previous report whenever it can be the module delta baseline, or otherwise keep the caption logic consistent with the deltas being shown.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Fixed in 0403b73. PreviousEnd is now populated whenever either the total duration or any module duration has a delta, so module-only delta tables identify their baseline run. Extended the existing failed-run history regression to assert the previous report end. Exact regression 1/1, SpectreResultsPrinterTests 4/4, core Release build 0 warnings/errors.
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
There was a problem hiding this comment.
Review: #3874 — Announce run report path and delta baseline
Implements #3750 cleanly: the write-path log line, the PreviousEnd field threaded through the run report, and the dim caption on the results table. Docs are updated, and both new tests pass as claimed.
Finding: baseline footer is gated more strictly than the delta column it explains
showDeltas (src/ModularPipelines/Helpers/SpectreResultsPrinter.cs:150-151) is true if either the pipeline-level TotalDurationDelta or any individual module's DurationDelta has a value. But the new footer only renders when PreviousEnd is non-null:
if (showDeltas && pipelineSummary.RunReport?.PreviousEnd is { } previousEnd) { ... }and PreviousEnd is only populated when previousTotalDuration is non-null (src/ModularPipelines/Engine/PipelineRunReportFactory.cs:73-86):
TimeSpan? previousTotalDuration = status == Status.Successful
&& previousReport?.Status == Status.Successful
? previousReport.TotalDuration
: null;
...
PreviousEnd = previousTotalDuration is null ? null : previousReport!.End,That condition requires the whole pipeline to have succeeded both times. Per-module deltas don't have that requirement — GetPreviousDuration (PipelineRunReportFactory.cs:200-208) only checks that the individual module succeeded in both runs, independent of overall pipeline status. So it's entirely possible for a run to fail overall (one module broke) while another module succeeded in both the current and previous run: showDeltas is true, the Δ previous column and a +Ns/-Ns value render on that module's row, but the footer identifying which run that delta is against is silently omitted — even though previousReport.End is available.
This also drifts from the issue's own acceptance criteria: #3750 asks for the footer "when deltas are shown," and calls previousReport.End "trivially available" — not conditioned on the pipeline-level delta being present.
Suggested fix: decouple PreviousEnd from previousTotalDuration/overall-success matching and set it directly whenever a previous report exists:
PreviousEnd = previousReport?.End,This also removes the previousReport! null-forgiving operator, since PreviousEnd no longer depends on previousTotalDuration's null-check to prove non-nullness — a small correctness/clarity win on top of fixing the gap. Worth adding a test for the "module delta present, total delta absent" case (e.g. a failed pipeline with one successful, previously-successful module) to lock in the fix, since neither of the two new tests exercises it.
Everything else
- Log line placement, message, and test (
SuccessfulReportWriteLogsFullPath) look correct — logged only after the atomic write succeeds, with the resolved full path. CultureInfo.InvariantCulture+ fixed"yyyy-MM-dd HH:mm 'UTC'"format is a good choice for a machine/CI-facing log-adjacent string — avoids locale-dependent surprises.- Docs changes accurately describe the new behavior.
No other concerns.
GitHub Actions
Failure
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 0403b73cde
ℹ️ 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".
| /// <summary> | ||
| /// Gets when the previous retained run used as the delta baseline finished. | ||
| /// </summary> | ||
| public DateTimeOffset? PreviousEnd { get; init; } |
There was a problem hiding this comment.
Adding PreviousEnd changes the serialized, schema-versioned run-report JSON while CurrentSchemaVersion still remains at 2, so downstream consumers cannot distinguish earlier v2 reports that lack the baseline timestamp from new reports where the baseline is intentionally absent. Since the previous report-shape addition bumped this version and the history store uses it for compatibility decisions, please advance the schema version and update the related compatibility/docs expectations with this new field.
Useful? React with 👍 / 👎.
1 participant
Closes #3750.
Logs the resolved run-report path after a successful atomic write. Carries the previous successful run's finish time into the report and displays it as a dim UTC footer whenever duration deltas are shown.
Validation:
RunReportTests: 69/69 passedSpectreResultsPrinterTests: 4/4 passed