feat(report): show the weekly trend of Doctor reports and flag a sharp rise by volen-silo · Pull Request #775 · ROCm/rocm-cli

volen-silo · 2026-10-28T09:14:17Z

cargo xtask report-rollup counts exported Doctor report mail at every level of the group key, but only as one total, so it cannot say whether a problem is steady or new. This adds the weekly trend and the weeks in which a known problem rose sharply (ROCMAI-745). It is the missing piece of the proposal's step 2, "Group the reports and count them. Report the trend. Send nothing.", and the manual form of the rate signal in ROCMAI-498. Nothing is sent or filed.

What changes

The trend (crates/rocm-core/src/report_trend.rs, new; pure, like the rollup). Reports per week for each catalog entry, for the newest --weeks weeks (8). A week is named by the Monday it starts on, and numbered from a Monday, so a baseline reaches back across New Year with plain arithmetic.

A rise is a week in which a known problem's count is at least --rise-factor (3) times the average of the --baseline-weeks (4) weeks before it, with at least --min-reports (5). The defaults are the proposal's own "triple in the week after a ROCm release"; they are flags because there is no data yet to set them from. Each rise names the entry, the week, the count against the average, and the ROCm releases that week's reports came from, so a rise is tied to a release without a table of release dates. Rules that keep it from naming a rise that is not one:

Only a matched catalog entry can rise. unrecognised mixes every cause the catalog does not know; it is in the table and never flagged.
A problem with nothing in its baseline is new, not risen.
The whole baseline has to lie inside the weeks the export covers, and the export's first week is never part of one: an export that begins mid-week holds only the end of that week, which would understate a baseline and make the next week look like a rise. So a rise needs --baseline-weeks plus two weeks of reports.
A quiet week inside the baseline counts as zero.

When the export is too short to judge, or the rule could not mean a rise (a factor under 2, a baseline of 0), the output says "not evaluated" and why, instead of "none found", and --json carries span_weeks and rises_evaluated. A problem with nothing in the weeks the table shows has no row, though a rise is judged over every week.

Where the week comes from. A report has no timestamp, by design, so it comes from the mail's Date header. The adapter converts it to UTC and reduces it to its week inside one function (week_of in xtask/src/report_mailbox.rs); the time of day, the zone and the day never leave it, and Message has no field that could hold them. The address takes mail from anyone, so a date is not evidence of when a report was written: a report whose date is missing, unreadable, before 2026-10-05 (the day the code that writes reports first landed) or more than a day ahead is counted and is in no week, and the output says how many. Raw .json bodies have no mail around them, so they are always undated.

This reverses an earlier decision. The mail adapter from #751 dropped Date altogether, because the zone it carries identifies a sender. It is kept now, as a week and nothing finer, because the trend needs a time axis and a report has no timestamp of its own. The module comment in xtask/src/report_mailbox.rs says so.

A date inside the plausible range is still the sender's claim, so one sender can choose a week inside it. That is acceptable while a person reads the output, docs/testing.md says so, and it is why a rise is not acted on automatically yet (ROCMAI-498).

The command gains --weeks, --rise-factor, --baseline-weeks and --min-reports, defined in report_rollup::Args beside the code that reads them, with their defaults read from the library. Values that could not mean anything are refused before anything is read, each with its own message. The trend and rises are in the text output and in --json.

Docs: docs/testing.md "Counting received reports" gains the trend, the flags and the rules above, and its privacy paragraph now says what is kept of Date. docs/architecture.md lists the new module.

Public API of rocm-core: Rollup::add, Rollup::of, Rollup::render_text() and Rollup::to_json() are #[cfg(test)] now, so they are no longer part of its public API. Production hands every report to Rollup::add_in_week (with None when it has no date) and renders with render_text_with and to_json_with; nothing in the workspace outside the tests called the removed forms. Trend::dated is gone for the same reason.

New dependency: chrono 0.4 is pinned in [workspace.dependencies] (default-features = false, std only) and used by xtask to parse the RFC 5322 date, and by a rocm-core test only. It is already in Cargo.lock through the dashboard crates, so no new crate enters the tree. rocm-core itself needs no date library: a test checks its small civil-date function against chrono for every day from 2000 to 2100.

Tests

rocm-core: week arithmetic across New Year and a leap day; counts per entry per week with their releases; a rise at the threshold and not below it; each of the three flags and the baseline length; unrecognised, a new problem, a baseline that starts before the export, a partial first week, and a week under the floor are not rises; a rule that cannot mean a rise, or an export too short to judge, says "not evaluated"; rises and table rows come in a fixed order and the JSON carries every rise field; a quiet week counts as zero; the wiring from the rollup into the trend.
xtask: Date in several zones and spellings (a trailing or nested comment included, an unclosed one undated), the week turning over at midnight UTC and not where it was sent, the week spanning New Year; a missing, malformed, wrong-weekday, pre-floor, future or too-far-ahead date is undated; a Date inside a MIME part is not the message's date; a marker test that an .eml dated Thu, 29 Oct 2026 23:59:59 +0530 leaks no time, zone or day into the text, --verbose or JSON output; the flags parsed from real command lines, each reaching its own field, refused before anything is read, accepted at their smallest valid value, and changing what is printed; and an in-process run over six weeks of mail built from the CLI's own link, with a planted rise, asserting the table, the rise and its release.
To check the tests catch what they should, 24 single mutations of the production code were run, each restored afterwards (the first-week rule, the rise sort, the baseline multiplier, JSON fields including undated and rises_evaluated, table order, each flag's validation and wiring, the date floor, the min_reports boundary, and both ways today() can go wrong). 22 are caught by a named test. The two that survive are equivalent mutants: the tie-breaks in the table's sort and in the releases sort. Their sources are BTreeMaps and the sorts are stable, so equal counts come out in name order either way; tests pin the resulting order, and a comment says why the clauses are stated.
No Gherkin scenario and no docs/manual-testing.md change: this is maintainer tooling under cargo xtask, not rocm behaviour.

Run locally (Windows 11)

A real export (a report sent with rocm diagnose --report --send and fetched as .eml through Graph): its Date, Tue, 27 Oct 2026 10:49:57 +0000, lands in one week, and no time, zone or day appears in either output.
A synthetic export over the four weeks that can exist today (the earliest plausible date is 2026-10-05), with 1/4/4/12 reports for one problem, a steady second problem, a flat unrecognised, one message dated before the floor and one with no date: by default it says "not evaluated: the reports cover 4 weeks, and a rise needs 6"; with --baseline-weeks 1 it flags the rise with its release, and both undated messages are counted and said to be in no week.
cargo clippy -p rocm-core -p xtask --all-targets -- -D warnings, cargo fmt --all --check, and the report tests of both crates pass on this head.
On the previous head the full gate also ran: cargo clippy --workspace --all-targets -- -D warnings, cargo xtask check-crate-edges, cargo xtask manifest --check, python scripts/smoke_local.py, and cargo test --workspace --all-targets --no-fail-fast, where all pass except two e2e-cucumber shared_runtime symlink tests that need Windows symlink privilege (OS error 1314) and are unrelated to this change. The pre-push hook runs the workspace tests again for this head.

Not run locally

cargo xtask coverage: aborted before the floors were checked, because ring_accumulates_snapshots_for_replay in rocm-dash-daemon failed under the instrumented build ("ring never reached 3 snapshots; len=2"). It passes in the normal test run and is unrelated to this change; CI's coverage job is authoritative for the rocm-core and xtask floors.
cargo xtask tpn --check: cargo-about is not installed. chrono is already in the dependency tree, so no notice should change.

CI

The self-hosted E2E lanes were red on an earlier head of this PR, and none of it came from this change, which touches only rocm-core, xtask and docs: the Windows lane hit a 403 downloading therock-dist-windows-gfx1151-7.14.1.tar.gz during the Lemonade backend install (fixed by #754), and the Strix Halo Ubuntu lanes failed on the dash-37 step added by #758 (fixed by #779). With both on main, the Strix Halo Windows, WSL2 and Ubuntu (release and nightly) lanes passed on an earlier head. On the current base, the Ubuntu lanes are cancelled at their 45-minute limit on every PR and on main: the two AIM serving scenarios added by #708 (serve-26, serve-27) take about 12 minutes each. That is the same on main and is unrelated to this change.

jussielo-amd

Review of ROCMAI-745's weekly trend / rise detection (full review, read blind against the diff before this description)

No code-level blocking findings. Nothing here is a reason to hold this up — posting as a comment rather than a change request.

CI note (not a code issue)

E2E tests (Strix Halo, Windows, current) is currently failing. Checked the job log directly: it fails during environment pre-warm with HTTP error 403 downloading therock-dist-windows-gfx1151-7.14.1.tar.gz from repo.amd.com, after compiling and starting cleanly. Nothing in the log touches report_trend/report_rollup/report_mailbox/chrono — this reads as an external download flake unrelated to this diff. Worth a rerun rather than a code change.

Non-blocking

Stale doc comment: "four headers" → should say "five" (inline below, xtask/src/report_mailbox.rs:16). Headers now has five fields since this PR adds date; the sentence predates the change and is off by one. This block specifically documents the module's privacy-retention design, so worth a quick fix even though it's cosmetic.

Worth confirming was deliberate (not blocking, just flagging for the record)

Reinstating the mail Date header reverses a decision made one commit earlier. The immediately preceding commit's module doc explicitly called out Date's timezone as sender-identifying and deliberately did not keep it; this PR adds it back as a fifth kept header (inline comment on xtask/src/report_mailbox.rs:164). The reversal looks well-mitigated to me — week_of() reduces straight to a UTC week before returning anything, and two tests (the_sender_never_leaves_the_parser, a_message_date_reaches_no_output_finer_than_its_week) assert the raw zone/time/weekday never reach output. Flagging only because the connection to the prior commit's stated rationale isn't visible from the diff alone — might be worth a sentence in the commit history making the reversal explicit for future readers.
Once ROCMAI-498 automates on this signal, a forged/missing Date is an input an attacker could use to manufacture or suppress a rise. Today's blast radius is low (phase-0, human-read-only, nothing filed or sent per the ticket's own scope), but worth carrying into that follow-up's design.

Minor, your call entirely

Trend::ranked() duplicates Group::ranked()'s sort/tie-break pattern (inline below, crates/rocm-core/src/report_trend.rs:285) — the underlying collections differ enough that unifying needs a generic helper; leaving it duplicated is a defensible call.
The baseline average in render_text truncates rather than rounds (inline below, crates/rocm-core/src/report_trend.rs:351) — purely cosmetic display precision.
A catalog entry with a single non-zero baseline report can be flagged "risen" even if that report was the problem's first-ever occurrence a few weeks earlier (inline below, crates/rocm-core/src/report_trend.rs:221) — within the three documented exclusion rules, just flagging as a possible future refinement to the rise criteria.

What stood out positively

the_sender_never_leaves_the_parser and a_message_date_reaches_no_output_finer_than_its_week assert the absence of leaked zone/time/weekday markers, not just the presence of expected values — meaningfully harder to accidentally pass than a typical test, and it's exactly the property the privacy guarantee depends on.
The civil-date/week arithmetic checks out against the real Gregorian calendar by hand, plus a dedicated cross-check against chrono for every day 2000–2100 — unusually rigorous boundary coverage.
All four of ROCMAI-745's Gherkin acceptance scenarios (trend per entry/week, a rise naming entry/rate/week/releases, undated reports counted but unplaced, and no finer-than-week leakage) check out directly against the code, not just against the description.

Scope: full read of all 12 changed files, plus the ticket (ROCMAI-745) and the prior report_rollup feature (#751) for convention-following and history. Didn't re-read GitHub comments/discussion before reaching these findings (first independent pass).

jussielo-amd · 2026-10-28T09:48:56Z

+/// `None` for a header that is missing, malformed, before
+/// [`EARLIEST_PLAUSIBLE_DAY`], or after `today` plus a day of slack for clocks
+/// that disagree.
+fn week_of(date: &str, today: i64) -> Option<ReportWeek> {


Worth noting for the record: the immediately preceding commit's module doc explicitly listed Date's timezone as sender-identifying info and deliberately didn't keep it. This reinstates it as a fifth kept header. Looks well-mitigated to me — this function reduces straight to a UTC week before returning, and the_sender_never_leaves_the_parser / a_message_date_reaches_no_output_finer_than_its_week both assert the raw zone/time/weekday never reach output. Just flagging that the connection back to that prior commit's rationale isn't visible from the diff alone; might be worth a sentence making the reversal explicit somewhere for future readers.

Made explicit in the module comment of xtask/src/report_mailbox.rs (updated head 522f3331): it now lists the five headers the parser keeps, says Date is the one that is itself sender-identifying, that an earlier version dropped it for that reason, why it is kept now (the only place a report's time is recorded, and the trend needs a time axis), and that only the week leaves. The PR description says the same.

jussielo-amd · 2026-10-28T09:48:56Z

+    ///   Otherwise the first weeks of any export would read as rises against
+    ///   weeks before collection began.
+    #[must_use]
+    pub fn rises(&self, rule: &RiseRule) -> Vec<Rise> {


A catalog entry with even a single non-zero report in its baseline window is eligible to be flagged "risen," even if that single report was the problem's own first-ever occurrence a few weeks earlier. That's within the three documented exclusion rules (only a fully-zero or export-spanning-incomplete baseline is excluded) — not a defect, just flagging as a possible future refinement to the rise criteria.

Left as it is. A baseline of one report can make a rise: the rule is a non-empty baseline, the factor and the floor, all three of which are flags, so a stricter one can be set when there is data to set it from. A refinement that ignores a problem's first-ever occurrence is noted for ROCMAI-498, where the signal gets automated.

jussielo-amd · 2026-10-28T09:48:56Z

+    }
+
+    /// Problems by their reports in all weeks, most first.
+    fn ranked(&self) -> Vec<(&String, &BTreeMap<Week, Cell>)> {


Minor: this duplicates Group::ranked()'s "sort by count desc, tie-break by key asc" pattern in report_rollup.rs. The underlying collections differ enough (BTreeMap<Week, Cell> vs BTreeMap<String, Group>) that sharing it would need a generic helper/closure — leaving it duplicated seems like a reasonable call, not pushing for a change.

Left duplicated. The two collections differ (BTreeMap<Week, Cell> against BTreeMap<String, Group>), so sharing the three-line sort would take a generic helper that costs more than it saves.

jussielo-amd · 2026-10-28T09:48:56Z

+            out.push_str("  none found\n");
+        }
+        for rise in &rises {
+            let tenths = rise.baseline_reports * 10 / rise.baseline_weeks;


Nit: this truncates rather than rounds (e.g. an average of 4.25/week prints as "4.2"). Purely cosmetic — the one test exercising it uses an exact multiple so it isn't exposed as wrong, just slightly under-precise in the general case.

Fixed in 522f3331: the average now rounds to the nearest tenth, halves up, in integers, so 17 reports over 4 weeks prints 4.3 a week. Test: the_average_in_a_rise_is_rounded_to_the_nearest_tenth_not_truncated.

nowycondro

🔴 Automated review · pr-review-watcher · 497b289

Review — needs work

Full review of the whole change (single PR, two commits, not stacked — base is main's current tip). Implements ROCMAI-745 ("report the weekly trend of Doctor reports and flag a sharp rise for a catalog entry") in full, including all four of its acceptance scenarios; also gives ROCMAI-498 ("signal a sharp rise... after a ROCm release") its explicitly-scoped manual, phase-0 form, without claiming to close either (both tickets remain "In Progress" and the PR does not claim completion of either Epic-level item).

Blocking

Privacy paragraph in docs/testing.md contradicts itself and the feature it documents — docs/testing.md:2236-2242
The updated paragraph lists "the week of the Date" among what the reader keeps, then states "...so nothing from a report's mail headers or signature reaches any output." The week is exactly what the new trend output's entire default (non---verbose) text table and --json output show for every entry (confirmed in crates/rocm-core/src/report_trend.rs's render_text/to_json, and its own test reports_are_placed_in_the_week_they_were_sent_and_a_rise_is_found asserting "2026-10-26" appears in text and "week": "2026-10-26" in JSON). The two sentences immediately after ("The Date is converted to UTC and reduced to its week... so the trend cannot place a sender in time more finely than a week") implicitly concede the week does reach output, leaving the paragraph internally inconsistent. The underlying code is correct and matches ROCMAI-745's own acceptance scenario 4 ("only the week appears... the time of day and the time zone appear nowhere") — this is a documentation-accuracy defect, not a privacy bug in the implementation, but it misstates a specific, meaningful privacy claim and a reader auditing what this tool discloses would come away with a false belief.
Confidence 80 · logic · Fix: qualify the sentence, e.g. "...so nothing from a report's mail headers or signature reaches any output except the week derived from the Date," keeping the clarifying sentences that already follow it.
3 of 5 self-hosted hardware E2E check lanes are failing on this PR's head commit (497b289e), and 2 more are still queued/incomplete — CI (E2E self-hosted workflow)
E2E tests (Strix Halo, Ubuntu, release, current), E2E tests (Strix Halo, Ubuntu, nightly, current), and E2E tests (Strix Halo, Windows, current) all report FAILURE; E2E tests (MI300X, current) and E2E tests (W7900, current) remain QUEUED as of this review. The job logs were not retrievable from this environment (the Actions API and job-log endpoints returned 403/were proxy-blocked), so the specific failure cause could not be inspected directly — reported at face value per gh pr checks/gh pr view --json statusCheckRollup. The change touches only crates/rocm-core, xtask, and docs, with no edits to apps/rocm/apps/rocmd or the engines these hardware lanes exercise, which is circumstantial evidence this is an infrastructure flake rather than a regression from this change — but that is a guess, not a verified exemption, and the project's own contribution workflow is explicit that "a red check means not ready until resolved" and flakes need to be re-run or escalated with evidence, not assumed away.
Confidence 90 (status taken directly from GitHub's own check rollup) · architectural · Fix: investigate/re-run the three failing self-hosted lanes and let the two queued ones complete before merge; if they are confirmed unrelated infrastructure flakes, say so in the PR with the evidence (failure logs or a rerun showing green) rather than leaving it implicit.

Non-blocking

None

Decisions for the author

The Rollup → Trend wiring (add_report calling self.trend.add on the vocabulary branch) has no direct unit test inside rocm-core itself — non-blocking-improvement
Coverage exists end-to-end through xtask's integration test (reports_are_placed_in_the_week_they_were_sent_and_a_rise_is_found) and through report_trend.rs's own tests calling Trend::add directly, so the wiring is exercised, just not by cargo test -p rocm-core in isolation. A small test in report_rollup.rs exercising add_report with a dated report and asserting the resulting Trend would let the core crate catch a broken wire-up on its own.
options_that_could_not_mean_anything_are_refused_before_anything_is_read (xtask/src/report_rollup.rs:769) checks the validation error content, not the ordering its name claims — non-blocking-improvement
It runs validation against an already-valid directory, so it would pass identically even if validation ran after collect(). The "before anything is read" property is real (confirmed by reading run() top to bottom) but isn't pinned down by this specific test; a fixture directory containing something that would fail during collect() (e.g. an unreadable file) alongside an invalid flag value would let the test actually prove the ordering.

Positive signals

Production code stays dependency-free for date arithmetic (civil_from_days, a hand-ported Howard Hinnant algorithm) while a chrono-based test exhaustively cross-checks it for every day 2000–2100, deliberately spanning both a /400 leap year and a /100-but-not-/400 non-leap year.
Privacy is enforced structurally, not just procedurally: time-of-day, zone and weekday are discarded (div_euclid(86_400)) before a Week value can even be constructed, with leak tests covering text, --verbose, --json, and Debug output of every relevant struct.
The "a rise's baseline must lie wholly inside the weeks the export covers" guard is a real, non-obvious correctness safeguard against false rises in the first weeks of any export, and it's tested in both directions (rejected when it reaches outside the window, accepted when it doesn't).
The RFC 5322 date parser correctly works around a known chrono limitation (trailing (...) comments after the timezone, chronotope/chrono#732) by stripping the comment before parsing, rather than working around it accidentally or silently misparsing.

Deployment notes

None required. This is maintainer-only tooling behind cargo xtask, not a change to the shipped rocm/rocmd binaries or their behavior. The new chrono workspace dependency (default-features = false, std only) is a real dependency only of the dev-only xtask binary and a [dev-dependencies]-only addition to rocm-core (test-only); it does not enter the product binaries' dependency tree, and Cargo.lock already carried chrono via the existing rocm-dash-* crates.

What this covered

Every file in the diff, read completely, plus surrounding code needed to judge it in place: crates/rocm-core/src/report.rs (the Report/catalog types the trend consumes), the full current crates/rocm-core/src/report_rollup.rs and xtask/src/report_rollup.rs/xtask/src/main.rs, and the pre-existing docs/testing.md "Counting received reports" section the diff extends. Commit range d59f596f6e18e6fd34fc58d05e3e44d5a08aa1fb...497b289e6e18e6fd34fc58d05e3e44d5a08aa1fb (base confirmed as the current tip of main, i.e. not stacked). Reviewed via three independent fan-out workers grouped by coupling (core trend/rollup crate; xtask mailbox+CLI+dependency; docs-vs-implementation accuracy), each reading its full file set, plus this driver performing consolidation, the agent-instruction-adherence pass (against this repo's AGENTS.md: DCO sign-off and commit signing both verified green in CI, no internal-keyword leaks in the diff, Jira-style ticket IDs correctly left unflagged, markdown-link conventions followed, no Gherkin-scenario gap since this is maintainer xtask tooling rather than rocm-user-observable behavior, coverage floors unaffected per the passing ratcheted coverage gate), the history pass (via git log/the two preceding commits on these files), and a code-comments pass (folded into the fan-out workers' review of each file).

Did not run: the "prior changes" pass (review comments on earlier changes to these files) — this sandbox has no access to this PR's or prior PRs' review-comment threads, and per this method's independence rule that discussion is explicitly out of scope for a blind findings pass regardless. CI job logs for the three failing E2E checks could not be fetched (API access blocked in this environment); their conclusions are reported at face value from GitHub's check rollup, not from inspecting the failure cause. Retrieved and read both named tickets (ROCMAI-745, ROCMAI-498) directly from Jira; no other ticket or discussion was consulted. Not reconciled against any review discussion on this PR — this report was produced blind to it.

volen-silo · 2026-10-28T12:44:18Z

Thanks, both reviews are addressed in the updated head (two commits, rebased on current main).

Fixed

Privacy paragraph in docs/testing.md: it now says nothing from a report's mail headers or signature reaches any output, except the week derived from the Date, which is what the trend shows. The clarifying sentences after it are kept.
"four headers" in report_mailbox.rs: the module comment now lists the five headers the parser keeps (Subject, Message-ID, the two content headers, Date) and says Date is kept only as a week. It also says plainly that an earlier version dropped Date for the zone it carries, why it is kept now (it is the only place a report's time is recorded, and the trend needs a time axis), and that only the week leaves. The PR description says the same.
Baseline average: it now rounds to the nearest tenth, halves up, in integers (17 reports over 4 weeks is 4.3 a week, not 4.2). Test: the_average_in_a_rise_is_rounded_to_the_nearest_tenth_not_truncated.
Rollup to Trend wiring: a_report_is_placed_in_the_week_it_is_given_and_one_without_a_week_in_none in rocm-core checks that a dated report lands in its week, an undated one in none, and a report outside the vocabulary in neither the groups nor a week.
Options ordering test: options_that_could_not_mean_anything_are_refused_before_anything_is_read now runs against a path that cannot be read, with a control call showing that valid options fail on reading that path. Validation running after the read would now surface as the wrong error.

Left as it is

Trend::ranked stays separate from Group::ranked: the collections differ (BTreeMap<Week, Cell> against BTreeMap<String, Group>), and sharing the three-line sort would cost a generic helper.
A baseline of one report can still make a rise. That is the rule as documented (a non-empty baseline, the factor and the floor), all three of which are flags, and I have noted the refinement for ROCMAI-498.
Carried to ROCMAI-498: once the signal is automated, a forged or missing Date is an input that could manufacture or suppress a rise. Today it is read by a person and nothing is sent or filed.

The red E2E lanes are not from this change. The diff touches only rocm-core, xtask and docs, and I read the failing job logs:

Strix Halo, Windows: the Lemonade backend install in the pre-warm step gets HTTP error 403 on therock-dist-windows-gfx1151-7.14.1.tar.gz and fails before any scenario runs. The lane has hit this on main pushes and in the merge queue, and fix(lemonade): restore packaged backend pins when a kept alignment pin stops installing #754 is the fix.
Strix Halo, Ubuntu (release and nightly): the only unexpected failure is the dash-37 step added by feat(diagnose): say a Doctor report mail carries the sender's address #758 ("the send review does not say the mail carries the user's address"). It has failed on every run of these lanes since that merge, on main, in the merge queue and on other PRs. The step joins the screen into one line while the review is drawn over the dashboard panels, so panel text lands inside the wrapped notice. fix(e2e): unblock the Strix Halo Ubuntu and WSL2 lanes on main #779 fixes it, and its Ubuntu lanes pass. The other failures in these lanes are the expected xfails (fix(e2e): unblock the Strix Halo Ubuntu and WSL2 lanes on main #779 says the same).
MI300X and W7900: still queued.

I will re-run the lanes once #779 and #754 are on main, and report the result here.

jussielo-amd

🔴 Automated review · pr-review-watcher · 522f333

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change at 522f3331 (the PR was rebased since the last round, so nothing was narrowed). All findings below are new this round; no change request was standing from the earlier round.
The ticket named in the description could not be retrieved from here; the description is judged on its own.

Blocking

A partial first export week is used as a baseline, so it can produce false rises (new) — crates/rocm-core/src/report_trend.rs:235, doc at :217-219
first is the earliest week with any report across all entries. The guard week.offset(-back) < first lets a baseline start on first. If the export begins mid-week, that week understates the baseline. The doc and the CLI help warn only that the latest week is partial, while the doc says the rules keep this "from naming a rise that is not one" — the same artifact appears one week later than the rule guards against.
Run on a scratch copy: weeks 11–13 at 6 reports each and week 14 at 15. With a week-10 count of 1 a rise is flagged (15 against a baseline of 19); with 6 there is none.
Confidence 85 · logic · Fix: require the baseline to start strictly after first (<=), or document the partial first week and exclude it from baselines.
Dead code: the no-date wrappers in rocm-core lost their only production caller (new) — crates/rocm-core/src/report_rollup.rs:80 (Rollup::of), :70 (add), :157 (render_text()), :214 (to_json()); report_trend.rs:195 (Trend::dated); lib.rs:166-170 (re-exports ReportRise, ReportTrend)
At the base, xtask/src/report_rollup.rs:123 called ReportRollup::of. This change replaces that with Collected::rollup() and add_in_week. A grep of apps, crates and xtask finds only test callers. xtask's own equivalents were made #[cfg(test)]; the rocm-core ones stayed pub. As a result the seven older rocm-core rollup tests now exercise only the undated route, not the one production uses.
Confidence 85 · mechanical · Fix: delete them, or mark them #[cfg(test)] and drive the tests through add_in_week and render_text_with.
The four new CLI options' wiring and validation boundaries are untested (new) — xtask/src/main.rs:289-304, 396-413; xtask/src/report_rollup.rs:165-190; crates/rocm-core/src/report_rollup.rs:208, 222
run() is only tested on its error paths, and every test that reaches the output uses TrendView::default(). Single-mutation runs left the suite green (rocm-core 47 passed, xtask 41 passed) for:
- the two render calls given TrendView::default();
- weeks: options.weeks → 8;
- rise: options.rise.clone() → RiseRule::default();
- baseline_weeks and min_reports swapped in main.rs (same usize type, so it compiles);
- factor < 2 → < 3, baseline_weeks < 1 → < 2, weeks < 1 → < 2 (each wrongly rejects a valid value; docs/testing.md itself uses --rise-factor 2).
  The validation test only drives the invalid side, and the --baseline-weeks and --weeks checks share one error string, so it cannot tell them apart.
  Confidence 100 · logic · Fix: parse flags with Cli::parse_from, run to completion with non-default options, and add valid-boundary cases (factor 2, baseline 1, weeks 1).
Tests in report_trend.rs leave output order, JSON rise fields and the baseline multiplier unpinned (new) — crates/rocm-core/src/report_trend.rs (the rises sort and the ranked() tie-breaks, the rise JSON, the baseline_weeks multiplier at :250-252)
Single-mutation runs (cargo test -p rocm-core --lib report_ -j 4, 47 passed each time) stayed green for: removing the sort of rises; the entry tie-break in the rises sort; reversing ranked(); the tie-break in ranked(); the tie-break in the releases sort; rises[].week replaced by count in the JSON; baseline_reports and baseline_weeks swapped in the JSON; and the multiplier replaced with saturating_mul(4) (the baseline_weeks: 2 case averages 4 either way). The tests also assert one entry only, and none has two or more rises or entries, or any ties. The output is documented as deterministic, and nothing pins it.
Confidence 100 · logic · Fix: add a case with two entries and two rises, with tied counts, asserting the exact text and JSON; add a baseline case where the baseline length changes the result.
The report_rollup.rs module doc still says no mail header reaches any output (new) — xtask/src/report_rollup.rs:18-20, against :29-32, docs/testing.md (privacy paragraph), xtask/src/report_mailbox.rs:25-31
Line 19 reads "Nothing from a report's own mail headers reaches any output; the adapter drops it while parsing." The week derived from Date now reaches both the text and --json output, and a_message_date_reaches_no_output_finer_than_its_week asserts that 2026-10-26 appears. The code and docs/testing.md govern: the latter states the exception explicitly and the test pins it. This is the privacy sentence the rest of that paragraph relies on ("so it is safe to pass on"), and the same class of statement already fixed in the other two places.
Confidence 85 · mechanical · Fix: add the exception to line 19, or fold lines 29-32 into that sentence.

Non-blocking

A rule that cannot be evaluated prints the same "none found" as one that was evaluated and found nothing (new) — crates/rocm-core/src/report_trend.rs:225-228, 235, 347-349
Run: a 3-week export with the default 4-week baseline, and --baseline-weeks 18446744073709551615 on a 5-week export, both print "Rises: … none found", the absurd value echoed in the heading. run rejects a baseline of 0 but nothing bounds it against the span. The only explanation is in docs/testing.md, not in the output.
Confidence 70 · logic · Fix: when no week has a whole baseline inside the span, say the export is too short and how many weeks it needs.
Week::short's doc says "the year is stated once", but the table never states a year (new) — report_trend.rs:59, header at :309-313
The header says only "(MM-DD)". --weeks has no upper bound, so a wide table can cross New Year with nothing saying which year a column is. Rises and --json do carry full dates.
Confidence 85 · mechanical · Fix: state the newest week's year in the header, or drop the claim.
The Date floor's doc claim is inaccurate, and one sender-chosen date reshapes the output (new) — xtask/src/report_mailbox.rs:45-51, :174-176; docs/testing.md ("must lie inside the weeks the export covers")
EARLIEST_PLAUSIBLE_DAY is documented as "the earliest day a report can have been sent", but it is 2025-01-01, well before the reporting feature existed in this repository's history. Run: on a 3-week export that printed "none found", adding one report dated Feb 2025 widened the table to 8 columns (5 empty) and produced a rise (fix-6-path: week of 2026-10-26, 12 reports against 2.0 a week).
Confidence 70 · logic · Fix: derive the floor from a real date (the feature's first release) or correct the comment, and say in docs/testing.md that the span comes from sender-asserted dates. See the first decision below.
The trend table prints rows with nothing in the shown window, and rises are judged over all weeks (new) — report_trend.rs:285-290, 319, 382
With weeks: 2, an entry whose only reports were older prints as fix-1-arch . ., and the JSON carries all-zero counts rows. A printed rise can name a week that is not in the table.
Confidence 80 · logic · Fix: skip entries with no count in the shown columns, or say the rows are ranked over all weeks.
The (-stripping in week_of adds almost nothing, and its test cases cannot fail on it (new) — xtask/src/report_mailbox.rs:166-169; tests at :720-733
chrono 0.4.45 already parses trailing comments, including nested ones (parse_from_rfc2822 on "… +0200 (CEST)" and a nested form both returned Ok). Deleting the line left all 41 report_ tests green. The only input it changes is an unclosed comment, which chrono rejects and the strip accepts. The test claims "however the header is spelled" but its (UTC) and (Central European Summer Time) cases cannot fail on this branch.
Confidence 70 · mechanical · Fix: delete the strip, or keep it and add an unclosed-comment case that pins why.
a_date_inside_a_mime_part_is_not_the_date_of_the_message passes with the Date reading removed (new) — xtask/src/report_mailbox.rs:792
Removing the Date reading entirely did not fail it, because the assertion is "no date". It can only fail if part headers were merged into the top level, which the current structure does not allow. This is a negative guard, not a pinned behaviour.
Confidence 70 · mechanical · Fix: assert that a top-level Date and a part Date together yield the top-level week.

Decisions for the author

The sender's Date is the time axis, and an outlier decides what is reported — tradeoff
Date is what the sender's client claims. Received is more trustworthy but identifies the sender, which is what the earlier decision avoided. A forward carries the forwarder's Date, not the original's. The cost is shown above: one dated report changed both the table width and whether a rise appears. The author has already noted that a forged or missing Date matters once the signal is automated; this run shows the effect exists today in the human-read output. Confirm that is acceptable until automation arrives, and whether the span should come from configured bounds rather than from the export.
Validity rules and defaults live in the CLI adapter, not the library — non-blocking-improvement
RiseRule and TrendView are public with raw usize fields. factor: 0 with min_reports: 0 makes every week with a non-zero baseline a rise (probe: rises=1 on 4,4,4,4,5). weeks: 0 is clamped to 1 in the library but rejected in xtask. The library handles baseline_weeks == 0 itself. The defaults 8/3/4/5 are written twice, in report_trend.rs Default impls and as clap default_value_t in main.rs, and nothing ties them together. A RiseRule::new, or clap reading the core Default, would fix both.
Baseline coverage is global, not per entry — tradeoff
The "export covers" bound uses the first week across all entries, so one early report in any entry unlocks baselines for every other entry. The alternative, a per-entry bound, would suppress more rises for newer problems. Confirm the global bound was the intent.

Positive signals

options_that_could_not_mean_anything_are_refused_before_anything_is_read now runs against an unreadable path with a control call that fails on reading. A validation that moved after the read would surface as the wrong error — a stronger premise than the usual "returns Err".
the_week_turns_over_at_midnight_utc… kills the local-day-instead-of-UTC mutation, and the_civil_date_agrees_with_chrono… checks every day from 2000 to 2100.
the_sender_never_leaves_the_parser and a_message_date_reaches_no_output_finer_than_its_week assert the absence of zone and time markers, not only the presence of the week.

Deployment notes

chrono 0.4 (default-features = false, std) is added to [workspace.dependencies], xtask and, as a dev-dependency only, rocm-core. Cargo.lock gains the two edges; the description says it is already in the tree via the dashboard crates. This is maintainer tooling (cargo xtask report-rollup) only, with no user-facing rocm behaviour change. The --json output gains a trend object and the command gains four flags.

What this covered

Every changed file was read completely by two independent review passes, split by contract: the rocm-core side (report_trend.rs, report_rollup.rs, the lib.rs and Cargo.toml hunks, plus the Report / group_key region they depend on) and the xtask side (report_mailbox.rs, report_rollup.rs, main.rs, the docs/testing.md section). AGENTS.md and the quality standards were read by both. The first-week guard, the dead wrappers and their former caller, the stale module doc, the short() year claim and the week_of code were re-verified directly against the source. Reviewed against merge base 965b94cf…, head 522f3331….

Mutation runs were made on scratch copies of the tree (about 15 on xtask, about 15 on rocm-core), each restored afterwards, plus a chrono probe and the xtask binary against synthetic exports. The full workspace suite was not run. CI shows build-and-test, clippy, Test (affected crates), coverage, signatures and the E2E lanes successful; E2E tests (W7900, current) was still queued when checked. The description's CI section (three red lanes) is out of date relative to that.

Did not run: the prior-changes pass (no review comments on earlier PRs to these files were looked up); the ticket check; any real-mailbox run. The review passes saw only paths and operational facts, with nothing from the earlier review or the discussion. The history pass ran in the xtask pass only. The description was read after the findings were settled and shows no blocking mismatch with the change; the first blocking finding concerns its claim that the first weeks of an export are never rises.

Reconciliation with the earlier round (read after the blind review; it adds and changes no finding). The earlier round's items are answered: the Windows E2E 403 flake (now passing), the "four headers" doc (now lists five), making the Date reversal explicit (done in the module comment and description), and the rounding of the baseline average (fixed, with a test that fails on truncation). The ranked duplication and the single-baseline-report rise were left by the author with reasons, which stand as documented rules. Everything above is new to this round, not a regression from those fixes.

jussielo-amd · 2026-10-29T14:04:13Z

+        );
+        let rises = self.rises(rule);
+        if rises.is_empty() {
+            out.push_str("  none found\n");


Non-blocking: when no week has its whole baseline inside the span, this prints the same none found as a rule that was evaluated and found nothing. Also at :225-228 and :235.

Run: a 3-week export with the default 4-week baseline, and --baseline-weeks 18446744073709551615 on a 5-week export, both print "Rises: … none found" (the huge value is echoed in the heading). run rejects a baseline of 0 but nothing bounds it against the span, and the only explanation is in docs/testing.md, not in the output.

Suggestion: say the export is too short and how many weeks it needs. Full detail is in the review body.

Fixed in 52de03be. When no week can be judged the output now says "not evaluated: the reports cover N weeks, and a rise needs M", and --json carries span_weeks and rises_evaluated. A rule that cannot mean a rise (a factor under 2, a baseline of 0) says so too. Tests: an_export_too_short_to_judge_says_so_and_does_not_say_none_found and a_rule_that_cannot_mean_a_rise_is_not_evaluated.

jussielo-amd · 2026-10-29T14:04:15Z

+        self.0.saturating_mul(7).saturating_add(4)
+    }
+
+    /// The Monday as `MM-DD`, for a column heading where the year is stated once.


Non-blocking: this says the year is stated once, but the table header (:309-313) says only (MM-DD) and never states a year. --weeks has no upper bound, so a wide table can cross New Year with nothing saying which year a column is (rises and --json do carry full dates).

Suggestion: state the newest week's year in the header, or drop the claim.

Fixed in 52de03be. The table header now names the first and last Monday in full, so a table that crosses New Year still says which year a column is in, and the Week::short doc says so.

jussielo-amd · 2026-10-29T14:04:16Z

+/// The report address takes mail from anyone, so a `Date` is not evidence of
+/// when a report was written. One before this, or one in the future, reads as
+/// undated instead of inventing a week that never happened.
+const EARLIEST_PLAUSIBLE_DAY: i64 = 20_089;


Non-blocking: the doc calls this "the earliest day a report can have been sent", but it is 2025-01-01, well before the reporting feature existed in this repository's history. It also lets one sender-chosen date reshape the output.

Run: on a 3-week export that printed "none found", adding one report dated Feb 2025 widened the table to 8 columns (5 empty) and produced a rise (fix-6-path: week of 2026-10-26, 12 reports against 2.0 a week).

Suggestion: derive the floor from a real date (the feature's first release) or correct the comment, and say in docs/testing.md that the span comes from sender-asserted dates. See the first decision in the review body.

Fixed in 52de03be. The floor is now 2026-10-05, the day the code that writes reports first landed, and the comment says so. Your February 2025 example is now undated. docs/testing.md says the table span and every baseline come from sender-asserted dates, and that a date inside the range is still the sender's claim. That is acceptable while a person reads the output, and I carried it to ROCMAI-498, where the signal gets automated.

jussielo-amd · 2026-10-29T14:04:18Z

+    }
+
+    /// Problems by their reports in all weeks, most first.
+    fn ranked(&self) -> Vec<(&String, &BTreeMap<Week, Cell>)> {


Non-blocking: ranked() returns every entry, so the table prints rows with nothing in the shown window (with weeks: 2, an entry whose only reports were older prints as fix-1-arch . .), and the JSON carries all-zero counts rows. Rises are judged over all weeks, so a printed rise can name a week that is not in the table. See also :319 and :382.

Suggestion: skip entries with no count in the shown columns, or say the rows are ranked over all weeks.

Fixed in 52de03be. An entry with nothing in the weeks shown has no row, in the text and in --json. docs/testing.md says a rise is judged over every week, so it can name a week older than the table shows. Test: the_table_shows_the_newest_weeks_and_a_dot_for_a_quiet_one.

jussielo-amd · 2026-10-29T14:04:20Z

+fn week_of(date: &str, today: i64) -> Option<ReportWeek> {
+    // A trailing comment such as `(CEST)` is allowed by the format and is not
+    // part of the date.
+    let date = date.split('(').next().unwrap_or(date).trim();


Non-blocking: chrono's parse_from_rfc2822 already accepts trailing comments, including nested ones ("… +0200 (CEST)" and a nested form both returned Ok). Deleting this line left all 41 report_ tests green. The only input it changes is an unclosed comment, which chrono rejects and this accepts.

The tests at :720-733 claim "however the header is spelled", but their (UTC) and (Central European Summer Time) cases cannot fail on this branch.

Suggestion: delete the strip, or keep it and add an unclosed-comment case that pins why.

Removed in 52de03be. I checked chrono 0.4.45 directly: it reads trailing and nested comments and rejects only an unclosed one. An unclosed comment is now a tested case for "undated", and the date cases pin chrono's behaviour.

jussielo-amd · 2026-10-29T14:04:22Z

+    /// Only the top-level `Date` counts. A part of the message may carry its own
+    /// headers, and those are not when the message was sent.
+    #[test]
+    fn a_date_inside_a_mime_part_is_not_the_date_of_the_message() {


Non-blocking: this test passes with the Date reading removed entirely, because its assertion is "no date". It can only fail if part headers were merged into the top level, which the current structure does not allow, so it is a negative guard rather than a pinned behaviour.

Suggestion: also assert that a top-level Date and a part Date together yield the top-level week.

Fixed in 52de03be. The test now asserts that a top-level Date and a part Date together give the top-level week, and that a part's date does not stand in for a missing top-level one. Test: a_date_inside_a_mime_part_is_not_the_date_of_the_message.

volen-silo · 2026-10-29T15:20:46Z

Thanks, this was a careful review. I checked each finding against the code before changing anything, and reproduced the first one. The fixes are in the updated head (two commits, rebased on current main).

Blocking

Partial first week as a baseline: reproduced with your numbers (a first week of 1 report flags a rise, one of 6 does not). A baseline now has to start strictly after the export's first week, which may be partial. Test: the_exports_first_week_is_never_part_of_a_baseline. The fixtures start a partial week earlier, docs/testing.md and the --help text say so, and a rise now needs --baseline-weeks plus two weeks of reports.
Dead code: Rollup::add, Rollup::of and the two no-argument renders are #[cfg(test)] now, Trend::dated is gone (its callers count understood - undated), and the ReportRise and ReportTrend re-exports are dropped. The older rocm-core rollup tests still go through the undated route. The dated one is covered by a_report_is_placed_in_the_week_it_is_given_and_one_without_a_week_in_none, the report_trend tests, and the xtask run over real mail.
Untested option wiring: the flags now live in report_rollup::Args, flattened into the command, so there is no field mapping in main.rs to mutate. The defaults are read from the library's Default instead of being written twice. render returns the output, and the tests parse real command lines: each flag reaches its own field (all values different, so a swap shows), each refusal has its own message and is checked before anything is read (with a control call showing the path really fails on reading), the smallest valid values (--rise-factor 2, --baseline-weeks 1, --weeks 1) are accepted, and non-default flags change what is printed.
report_trend tests: added tests for output order, every JSON rise field, the baseline length, the table order, a rule that cannot mean a rise, and a too-short export.
Module doc: the exception for the week derived from Date is stated in xtask/src/report_rollup.rs.

To check the tests do what you found missing, I ran 17 single mutations, including the ones you listed, restoring the files after each. All your listed ones are now caught, each by a named test: the removed sort, the multiplier fixed at 4, the swapped and wrong JSON fields, the reversed table order, factor < 2 to < 3, baseline_weeks < 1 to < 2, weeks < 1 to < 2, both renders given the default view, weeks ignored, and baseline_weeks swapped with min_reports. One survives: removing the tie-break in the table's sort, which is an equivalent mutant. The source is a BTreeMap, already in name order, and the sort is stable, so equal totals come out in name order either way. The code keeps the explicit tie-break and says so in a comment. The same holds for the entry tie-break in rises.

Non-blocking

"none found" for a rule that could not be evaluated: the output now says "not evaluated: the reports cover N weeks, and a rise needs M", and --json carries span_weeks and rises_evaluated. The same holds for a factor under 2 or a baseline of 0.
Week::short and the year: the table header names the first and last Monday in full.
The date floor: it is now 2026-10-05, the day the report code first landed, and the comment says so. Your February 2025 example is undated. docs/testing.md says the table's span and every baseline come from sender-asserted dates.
Rows with nothing in view: skipped in the text and in --json. The docs say a rise is judged over every week, so it can name a week older than the table shows.
The (-stripping: you are right. I checked chrono 0.4.45 directly: it reads trailing and nested comments and rejects only an unclosed one. The line is gone, an unclosed comment is a tested case for "undated", and the earlier note that it worked around a chrono limitation was wrong for this version.
MIME part Date test: it now asserts that a top-level Date and a part's Date together give the top-level week, and that a part's date does not stand in for a missing one.

Decisions

The sender's Date as the time axis: acceptable while a person reads the output, and docs/testing.md now says so, along with the forwarder's-date caveat. Received identifies the sender, which is why it is not read. I carried the forged-date point to ROCMAI-498, where the signal gets automated. Bounds set by configuration are not worth adding before then.
Validity in the library: RiseRule::is_meaningful now decides it, so a rule that cannot mean a rise finds none and says so. The defaults have one source. A --weeks of 0 is clamped in the library and refused by the command, which I left as it is.
A global baseline bound: intended, and documented. A per-entry bound would suppress rises for newer problems. The partial-first-week rule is what covers the case you found.

The description's CI section is updated: the Windows and Ubuntu lanes pass now that #754 and #779 landed.

All points addressed in 52de03b: the five blocking findings are fixed with tests (the partial first week, the dead code, the option wiring, the ordering and JSON tests, the module doc), and each non-blocking point and decision has a fix or a stated reason. Details: #775 (comment)

jussielo-amd

Thanks for the rework. The first-week-never-in-a-baseline rule, and the "not evaluated" versus "none found" distinction, make this much easier to trust, and the tests around the date handling are thorough. I'm requesting changes for one thing: a couple of the new --json fields aren't pinned by any test, and I'd like them to be before this lands since a script is the only consumer of them. Details are inline.

I checked this by mutating the code in a scratch copy rather than only reading it. Flipping the first-week guard (<= to <) is caught by the_exports_first_week_is_never_part_of_a_baseline, as the PR says. These ones are not caught, and the PR text doesn't mention them:

forcing "undated" to 0 in to_json leaves all 98 report tests in rocm-core and xtask green
dropping the is_meaningful() conjunct from rises_evaluated leaves the rocm-core report tests green
changing / 86_400 to / 3_600 in today() leaves everything green

A few smaller things, none of which need to block:

the message of the second commit says dates "before 2025" are undated, but the code, docs/testing.md and the PR body all say 2026-10-05 (which is right: report.rs first landed that day). Worth fixing if you rewrite the commits again.
in rises, the .then_with(|| a.0.cmp(&b.0)) tie-break on the ROCm releases isn't pinned either, since every fixture has distinct counts. Either add a tied-count case or drop the clause.
the doc on civil_from_days says it is exact over the whole i64 range, but saturating_add(719_468) isn't exact near i64::MAX. It can't be reached from mail input because week_of bounds the day, so a rewording is enough.
making Rollup::add, of, render_text and to_json #[cfg(test)] removes them from rocm-core's public API. I found no non-test user in the workspace, so a line in the PR description would cover it.

jussielo-amd · 2026-10-30T07:51:40Z

+        json!({
+            "weeks": columns.iter().map(ToString::to_string).collect::<Vec<_>>(),
+            "entries": entries,
+            "undated": self.undated,


This is the only place a script learns that reports were left out of every week, but nothing pins it above zero. If I change this to "undated": 0 every test still passes: the only JSON assertion is assert_eq!(value["undated"], 0) in the_json_carries_the_weeks_the_counts_and_the_rises, and the undated-date test in xtask checks the text output only. Could you assert a non-zero value here, for example by adding an undated report in the JSON test and checking value["undated"]?

Fixed in 640b3842. The JSON test now adds two undated reports and asserts undated == 2, so forcing the field to 0 fails the_json_carries_the_weeks_the_counts_and_the_rises. I ran that mutation to check.

jussielo-amd · 2026-10-30T07:51:40Z

+            "rises_evaluated": view.rise.is_meaningful()
+                && self.span_weeks() >= view.rise.weeks_needed(),


I removed the view.rise.is_meaningful() && part and the rocm-core report tests still pass. a_rule_that_cannot_mean_a_rise_is_not_evaluated only looks at render_text, and the JSON tests use the default rule. Without that conjunct, baseline_weeks: 0 gives weeks_needed() == 2, so the JSON would say rises_evaluated: true for a rule that can't be evaluated. The CLI's Args::check hides this today, but TrendView is public. Adding assert_eq!(trend.to_json(&view(rule))["rises_evaluated"], false) inside that loop would pin it.

Fixed in 640b3842. a_rule_that_cannot_mean_a_rise_is_not_evaluated now asserts rises_evaluated == false for each rule that cannot mean a rise, for the JSON as well as the text. Dropping the is_meaningful() conjunct fails it, which I checked by running that mutation.

jussielo-amd · 2026-10-30T07:51:40Z

+/// Today, as days since 1970-01-01 in UTC. A clock before 1970 reads as day 0,
+/// which makes every date look like the future, so every report reads as
+/// undated: the output says so instead of placing reports in the wrong weeks.
+fn today() -> i64 {


Nothing exercises this: render takes today as a parameter, so the tests never reach the real clock. I changed / 86_400 to / 3_600 and everything stayed green, which would quietly switch off the "more than a day ahead" check in production (and returning 0 would make every report undated). A small test that today() is at or after EARLIEST_PLAUSIBLE_DAY and below some sane upper bound would cover it. Not blocking on its own.

Fixed in 640b3842. today_is_a_day_number_in_a_sane_range checks that today() falls between 2026-10-05 and 2100-01-01. Both mutations you describe, dividing by 3,600 and returning 0, now fail it; I ran both.

jussielo-amd · 2026-10-30T07:51:40Z

+    /// Also list each message skipped as not a report, by file and subject.
+    #[arg(long)]
+    pub verbose: bool,
+    /// How many of the newest weeks the trend table shows.


Small docs nit: --rise-factor and --baseline-weeks say what the minimum is in both the help and docs/testing.md, but --weeks doesn't, even though check() refuses 0. Saying "(at least 1)" here and in the docs would keep them in step.

Fixed in 640b3842. The --weeks help now says "(at least 1)", and docs/testing.md says --rise-factor must be at least 2 and --baseline-weeks and --weeks at least 1.

nowycondro

🔴 Automated review · pr-review-watcher · 52de03b

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change at head 52de03be1b6c0db9db0581414b3c4d0f7f19bc73 (unchanged since dispatch). Implements ROCMAI-745 ("report the weekly trend of Doctor reports and flag a sharp rise for a catalog entry") in full — all four of its acceptance scenarios check out directly against the code — and gives ROCMAI-498 ("signal a sharp rise... after a ROCm release") its explicitly-scoped manual, phase-0 form, without claiming to close either (both tickets remain "In Progress" and the PR does not claim completion of either).

Blocking

Two self-hosted hardware E2E lanes did not complete successfully, and a third has been stuck queued for over 12 hours, all at this head — CI (E2E self-hosted workflow, run 40900539451)
E2E tests (Strix Halo, Ubuntu, release, current) and E2E tests (Strix Halo, Ubuntu, nightly, current) both report cancelled (their "Run E2E tests on Strix Halo" step was itself cancelled mid-run, after ~45 minutes). E2E tests (W7900, current) has been queued since 2026-10-29T19:17 with no runner picking it up as of this review. Every other check (build-and-test, clippy, Test (affected crates), coverage, commit signatures, CodeQL, windows-build-and-test, the non-hardware E2E suite, docs build) is green, and the change touches only crates/rocm-core, xtask, and docs — circumstantial evidence this is self-hosted runner-pool flakiness rather than a regression from this diff, consistent with this exact class of lane being flagged as flaky in earlier rounds of this PR's own CI history. That is a plausible read, not a verified exemption: a cancelled or indefinitely-queued required lane is not a pass, taken at face value from the check rollup.
Confidence 90 (status read directly from the GitHub Checks/Actions API) · architectural · Fix: re-run the two cancelled lanes and chase why W7900 never got a runner; if confirmed unrelated infra flakiness, say so on the PR with the rerun evidence.

Non-blocking

None

Decisions for the author

The min_reports floor's exact-equality boundary is untested (crates/rocm-core/src/report_trend.rs:257) — non-blocking-improvement
cell.count < rule.min_reports correctly implements "at least min_reports" (count equal to the floor is accepted, matching the documented rule), and this is hand-traceable and currently correct. But no existing test constructs a candidate rise with count exactly equal to min_reports (default 5) — every test either sits well above the floor or is deliberately below a raised floor (e.g. 12 against a floor of 13) to prove rejection. A one-character mutation (< → <=) here would pass the whole suite unnoticed. A case with count == min_reports exactly, asserting it still counts as a rise candidate, would close this.
unrecognised reports can never be flagged as a rise, by design (crates/rocm-core/src/report_trend.rs:253, doc at :228, pinned by the_unrecognised_entry_is_shown_and_never_flagged) — tradeoff
This matches ROCMAI-745's own design decision ("only a matched catalog entry is considered") and is deliberately tested, not a gap. The consequence: a genuinely new, previously-uncatalogued problem that spikes sharply will show its raw count in the unrecognised row but will never trigger the "flag a sharp rise" behavior this feature is named for — exactly the kind of sudden, unclassified problem a maintainer might most want an automatic flag on. A human skimming the table can still notice the raw number today (phase 0, human-read-only), so the risk is scoped, but worth confirming it stays acceptable as this feeds ROCMAI-498's eventual automation.

Positive signals

The civil-date arithmetic (crates/rocm-core/src/report_trend.rs) is hand-verified against a ported Howard Hinnant algorithm and cross-checked against chrono for every day from 2000 to 2100, rather than spot-checked.
The privacy claim — that only the ISO week of a report's Date, never the time, zone, or weekday, reaches any output — was independently re-verified as self-consistent across the code, both documentation locations, and the test suite by this round, correcting the self-contradiction an earlier round of review on this same PR found in docs/testing.md.
each_flag_reaches_its_own_part_of_the_view (xtask) deliberately uses four distinct non-default values across same-typed usize CLI fields, specifically so a field-swap mutation between --baseline-weeks and --min-reports would be caught rather than silently pass.

Deployment notes

None beyond what's already on the record: this is maintainer-only cargo xtask tooling with no change to the shipped rocm/rocmd binaries. chrono is a dev-dependency only for rocm-core, so it does not enter the product binaries' dependency tree. Worth noting for precision: in a full cargo test --workspace build, Cargo's feature unification means chrono still resolves with the broader feature set already pulled in by the rocm-dash-* crates regardless of this PR's default-features = false request — not a defect, since the claim in the PR body ("no new crate enters the tree") is accurate, just a nuance on what the minimal-feature framing does and doesn't control.

What this covered

Every file in the diff (Cargo.lock, Cargo.toml, crates/rocm-core/Cargo.toml, crates/rocm-core/src/lib.rs, crates/rocm-core/src/report_rollup.rs, crates/rocm-core/src/report_trend.rs, docs/architecture.md, docs/testing.md, xtask/Cargo.toml, xtask/src/main.rs, xtask/src/report_mailbox.rs, xtask/src/report_rollup.rs) read completely, by three independent blind fan-out workers grouped by coupling (rocm-core core logic; xtask CLI/mailbox wiring; docs-vs-implementation accuracy), each also reading the surrounding Report/group_key code, production callers (to rule out dead code), and this repo's AGENTS.md and git history for the files in its scope. Commit range f8dcff5052f912c5fa10b68097d34db0cd470bfe...52de03be1b6c0db9db0581414b3c4d0f7f19bc73 (base confirmed as the current tip of main; not stacked). I personally re-verified the one surviving finding against the source and pulled the live CI check/job detail directly.

Did not run: no Rust toolchain was available to any of the three workers or to me, so no cargo test/clippy/mutation run was actually executed here — all boundary reasoning is from direct code tracing against the existing tests' literal assertions, not from an executed run. The "prior changes" pass (review comments on earlier PRs to these files) did not run. I checked this PR's existing review list only to confirm no review from this automation's identity already existed at this exact head (required before posting); none of that discussion's content informed the findings above, which were reached blind, and it is not reconciled here. Retrieved and read both named tickets (ROCMAI-745, ROCMAI-498) directly from Jira.

…lem rose The rollup counts reports at every level of the group key but only as one total, so it cannot say whether a problem is steady or new. Adds the same reports counted per week, and the weeks in which a known problem's count is at least three times the average of the four weeks before it, with a floor of five reports. Pure counting, like the rollup: it is handed a `Week`, which is all a date reduces to, so nothing finer than a week can reach a count or an output. A week is a plain number counted from a Monday, so a baseline reaches back across New Year without knowing how long the last year was. A test checks the hand-written calendar against chrono for every day from 2000 to 2100. Only matched catalog entries can rise: `unrecognised` mixes every cause the catalog does not know. A problem with nothing in its baseline is new, not risen, and a baseline that starts before the export does is not used, so the first weeks of an export are never rises. Nothing is sent or filed. Reading dates out of mail follows. Signed-off-by: Eugene Volen <eugene.volen@amd.com>

`cargo xtask report-rollup` now places each report in the week its mail was sent and prints the trend from the previous commit: a table of reports per week for each entry, and the weeks in which a known problem rose, with the ROCm releases that week's reports came from. `--weeks`, `--rise-factor`, `--baseline-weeks` and `--min-reports` set the view and the rule, and values that could not mean anything are refused. A report has no timestamp, by design, so the week comes from the mail's `Date`. It is converted to UTC and reduced to its week inside one function; the time of day, the zone and the day never leave it, and `Message` has no field that could hold them. A report whose date is missing, unreadable, before 2026-10-05 (the day the code that writes reports first landed) or more than a day ahead is counted and is in no week, and the output says how many: the address takes mail from anyone, so a date is not evidence of when a report was written. Sends and files nothing. This is maintainer tooling under `cargo xtask`, so it is covered by unit tests and an in-process run over mail built from the CLI's own link, not by a scenario. Signed-off-by: Eugene Volen <eugene.volen@amd.com>

volen-silo · 2026-10-30T09:00:07Z

Thanks. I reproduced each of the three by mutating the code, and all three were as you describe. They are fixed in the updated head (two commits, rebased on current main), and I ran the same mutations again to check that a test now fails for each.

The three unpinned fields

"undated" in --json: the JSON test now adds two undated reports and asserts undated == 2, so forcing it to 0 fails the_json_carries_the_weeks_the_counts_and_the_rises.
rises_evaluated: a_rule_that_cannot_mean_a_rise_is_not_evaluated now asserts rises_evaluated == false for each rule that cannot mean a rise, as well as the text. Dropping the is_meaningful() conjunct fails it.
today(): a test now checks that it is a day number between 2026-10-05 and 2100-01-01. Dividing by 3,600 and returning 0 both fail today_is_a_day_number_in_a_sane_range.

Smaller points

The commit message: it said dates "before 2025"; it now says 2026-10-05, the day the report code first landed, as the code, the docs and the description do.
The release tie-break in rises: removing it is an equivalent mutant. cell.rocm is a BTreeMap and the sort is stable, so equal counts come out by name either way, and no test can tell the two apart. I added releases_with_equal_counts_are_listed_by_name, a tied-count case that pins the resulting order, and a comment saying why the clause is stated. The same holds for the tie-break in the table's sort, which the earlier round covered.
civil_from_days: the doc now says it is exact for every day count whose shift by 719,468 does not overflow, and that saturating_add only keeps the arithmetic from panicking beyond that.
The #[cfg(test)] wrappers: the description now says that Rollup::add, of, render_text() and to_json() are no longer public API, and that nothing outside the tests used them.
The --weeks minimum: the help and docs/testing.md now say it is at least 1.

A correction to my own earlier count. The previous reply said 17 mutations ran and 16 were caught. Recounting, 18 valid mutants ran and 17 were caught; one entry in that run added a match arm that can never fire, so it was not a valid mutant and should not have been counted. With the six from this round (five caught, plus the release tie-break, equivalent) it is 24 mutants, 22 caught, and 2 equivalent survivors. The description now says that.

The automated review filed at 08:27

The min_reports boundary: it was untested. each_part_of_the_rule_can_be_changed now has a count exactly at the floor (12 against a floor of 12) and expects a rise, so changing < to <= fails it. I ran that mutation to check.
unrecognised is never flagged: that is by design, since it mixes every cause the catalog does not know, and the ticket says only a matched entry is considered. It is in the table with its raw count, which a person reading the output can see. I have noted it for ROCMAI-498, where an automated signal might want a separate rule for a spike in unrecognised.
The chrono feature note: agreed. Feature unification means it resolves with the dashboard crates' features in a workspace build, and "no new crate enters the tree" stays accurate.

CI. The Ubuntu release and nightly lanes show as failed on this head, but both were cancelled at the workflow's 45-minute limit. That is not specific to this PR: of the last 78 finished Ubuntu lane jobs, 72 were cancelled at that limit, 4 failed and 2 passed, on main pushes, in the merge queue and on every PR. The two AIM scenarios added by #708 (serve-26 and serve-27) take about 12 minutes each, and the last Ubuntu pass was on a release branch that does not have them. The previous head (52de03be) passed in the MI300X, Windows and WSL2 lanes, and the W7900 lane was still queued. This change touches only rocm-core, xtask and docs.

All points addressed in 640b384: the three unpinned fields (undated, rises_evaluated, today) are now covered by tests that fail under the mutations you described, and each smaller point and decision has a fix or a stated reason. Details: #775 (comment)

nowycondro

🔴 Automated review · pr-review-watcher · 640b384

This automation never files a GitHub approval, so no approving review will appear here whatever the outcome — the merge decision stays with a human reviewer.

Review — needs work

Full review of the whole change at head 640b3842d48f61c0e1cd0856089e67d73c850b49 (all 12 files this PR touches relative to main, each read completely by independent workers — not limited to the diff hunks since the last round). Implements ROCMAI-745 ("report the weekly trend of Doctor reports and flag a sharp rise for a catalog entry") in full — all four of its acceptance scenarios check out directly against the code — and gives ROCMAI-498 its explicitly-scoped manual, phase-0 form, without claiming to close either (both tickets remain "In Progress," matching the PR's own framing).

Blocking

The self-hosted hardware E2E workflow has not run at this head — CI (E2E self-hosted workflow, run 41030185109)
At head 640b3842, this workflow's status is pending with zero dispatched jobs, unchanged since its creation timestamp at the point of this review. Every other check (build-and-test, windows-build-and-test, clippy, Test (affected crates), the coverage-floors job, commit signatures, CodeQL, the regular E2E tests job and its consolidated report, docs/license/link checks) is green at this exact head. This is the same lane that blocked each of the three preceding rounds on this PR (a Windows 403 download flake, then two cancelled Ubuntu runs plus an indefinitely queued W7900 run) — a chronic, change-independent infrastructure pattern, not something introduced by this diff (which touches only crates/rocm-core, xtask, and docs). That is circumstantial evidence it is unrelated to the code, not a verified exemption: a required lane that never dispatches is not a pass.
Confidence 90 (status read directly from the GitHub Actions API) · architectural · Fix: find why this run is stuck in pending/undispatched (runner-pool availability), re-run it, and record the evidence (or a green re-run) on the PR before merge.

Non-blocking

A tie-break clause in Trend::rises's final sort is unobservable, and the test that exercises it asserts the opposite of what's true — crates/rocm-core/src/report_trend.rs:283 (found.sort_by(|a, b| a.week.cmp(&b.week).then_with(|| a.entry.cmp(&b.entry)))), test at :1008-1040
self.weeks is a BTreeMap<String, _>, so rises()'s outer loop always visits entries in alphabetical order; for any two entries sharing a week, their Rises are pushed into found already in alphabetical order, and Vec::sort_by is stable, so dropping the .then_with(...) entry tie-break produces an identical result. I hand-traced the exact fixture in rises_are_listed_oldest_week_first_with_their_counts_and_baselines and confirmed this: the test's own assertion message — "oldest week first, then by entry; not the order the entries are stored in" — is factually backwards; the resulting order is exactly the BTreeMap storage order, preserved by sort stability. Two sibling tie-breaks in the same file (ranked()'s entry tie-break, and the rocm.sort_by release tie-break) are correctly disclosed in their own comments as "unobservable to a test"; this third one isn't, and the PR description's mutation-sweep count ("22 are caught... the two that survive are equivalent mutants: the tie-breaks in the table's sort and in the releases sort") doesn't mention it, suggesting it was missed by that sweep rather than deliberately scoped out.
Confidence 85 · mechanical · Fix: either drop the clause and say so (matching the other two disclosed cases), or add a disclosing comment like the siblings' and correct the test's assertion message.
The "weeks" JSON field and the table header's per-column week labels are untested beyond length — crates/rocm-core/src/report_trend.rs:470 ("weeks": columns.iter().map(ToString::to_string)...), :353-356 (header-building loop using week.short())
The only test touching value["weeks"] (the_json_carries_the_weeks_the_counts_and_the_rises) checks .as_array().unwrap().len() and nothing else; no test matches on the actual week strings in that array or in the rendered header row. Week::short() is unit-tested in isolation (report_trend.rs:575) but never through this integration path. A mutation substituting a wrong Week value into the JSON array, or swapping week.short() for a different format in the header loop, would pass the full suite.
Confidence 75 · mechanical · Fix: add a content assertion on value["weeks"] and/or the header row text in one of the existing table-rendering tests.

Decisions for the author

xtask's flag validation reimplements the library's is_meaningful() by hand — xtask/src/report_rollup.rs:198-213 vs crates/rocm-core/src/report_trend.rs:171-172 — non-blocking-improvement
Args::check() hand-codes rise_factor < 2 and baseline_weeks < 1 instead of calling RiseRule::is_meaningful() (factor >= 2 && baseline_weeks >= 1), which states the identical rule. The values match today, so nothing is wrong, but the duplication means the CLI's validation can silently drift from the library's own definition of "meaningful" if that definition changes later (the library would then happily compute rises() as empty with no error, rather than the CLI refusing up front). Calling the library function (with per-field messages added at the call site if needed) would make that drift structurally impossible.
Two items flagged in the preceding nowycondro round (at 52de03be) are resolved, re-verified by reading the code rather than trusting the prior report: the min_reports exact-equality boundary now has a dedicated test (report_trend.rs, a count of exactly 12 against a floor of 12), and the unrecognised-is-never-flagged tradeoff is unchanged and still holds as documented — informational, not reopened as findings.

Positive signals

Boundary-exact tests throughout (the min_reports floor, the baseline-length multiplier, weeks_needed) pin the exact boundary value, not just an interior case — closing precisely the gap the previous review round raised.
The privacy guarantee is enforced structurally, not just by test: Week/ReportWeek is a bare day-number value with nowhere to hold a time of day or zone, and this is independently checked by marker-absence tests in both report_trend.rs and xtask/src/report_mailbox.rs (asserting the absence of zone/time/weekday substrings, not just the presence of the expected week).
Defaults can't drift across three surfaces: clap's default_value_t reads ReportTrendView::default()/ReportRiseRule::default() directly rather than repeating the literals, and docs/testing.md's stated defaults were independently confirmed to match.
The RFC 5322 comment-stripping workaround flagged as dead weight in the prior round was simply deleted (rather than patched or left as noted debt), and the module doc now correctly attributes trailing-comment parsing to chrono itself.

Deployment notes

None beyond what's already on record: maintainer-only cargo xtask report-rollup tooling, no change to shipped rocm/rocmd binaries. chrono 0.4.45 was already in Cargo.lock via the rocm-dash-* crates before this PR (independently confirmed), so no new crate enters the dependency tree; it's a real dependency of xtask and a [dev-dependencies]-only addition to rocm-core. --json output gains a trend object; the command gains four new flags.

What this covered

Every file in the diff — Cargo.lock, Cargo.toml, crates/rocm-core/Cargo.toml, crates/rocm-core/src/lib.rs, crates/rocm-core/src/report_rollup.rs, crates/rocm-core/src/report_trend.rs, docs/architecture.md, docs/testing.md, xtask/Cargo.toml, xtask/src/main.rs, xtask/src/report_mailbox.rs, xtask/src/report_rollup.rs — read completely by three independent fan-out workers grouped by coupling (rocm-core core logic; xtask CLI/mailbox wiring; docs-vs-implementation accuracy + history), each also reading crates/rocm-core/src/report.rs and this repo's AGENTS.md. I independently re-verified every worker finding directly against the source myself rather than accepting it at face value: hand-traced the tie-break (confirmed vacuous by simulating the exact test fixture), grepped the whole tree for non-test callers of the now-#[cfg(test)] Rollup/Trend methods (none found), independently recomputed EARLIEST_PLAUSIBLE_DAY (20,731 = 2026-10-05) and the default-value wiring, and confirmed chrono predates this PR in Cargo.lock. Commit range 8c23a91abed959df3233651346a379feea3fa0e2 (confirmed current tip of main) … 640b3842d48f61c0e1cd0856089e67d73c850b49. Retrieved and read both named tickets (ROCMAI-745, ROCMAI-498) directly from Jira.

Narrowing note: given the extensive prior review history, the delta actually introduced since the last nowycondro round (at 52de03be) was small — test additions pinning the min_reports boundary, the undated/rises_evaluated JSON fields and the release tie-break, a new today() sanity test, one doc-comment accuracy fix, and a commit-message date correction — all confirmed present and correct by reading the code, not by trusting the delta's own framing. Workers nonetheless read every file in the PR's scope in full, which is why this is reported as a full review rather than a narrowed one.

Did not run: no Rust toolchain was available in this environment, so no cargo test/clippy/mutation run was executed; all findings are from direct code tracing, cross-checked by hand where noted above. CI status was read directly from the GitHub Actions/Checks API, not re-derived from workflow definitions. The "prior changes" pass did not re-read GitHub review-thread discussion beyond deriving the code delta via git diff between the last-reviewed and current commits; this report was reached blind to that discussion and is not reconciled against it.

volen-silo requested a review from a team as a code owner October 28, 2026 09:14

volen-silo requested a review from fredespi October 28, 2026 09:14

jussielo-amd reviewed Oct 28, 2026

View reviewed changes

nowycondro reviewed Oct 28, 2026

View reviewed changes

volen-silo force-pushed the rocmai-745-report-trend branch from 497b289 to 522f333 Compare October 28, 2026 12:43

jussielo-amd previously requested changes Oct 29, 2026

View reviewed changes

volen-silo force-pushed the rocmai-745-report-trend branch from 522f333 to 52de03b Compare October 29, 2026 15:20

jussielo-amd previously requested changes Oct 30, 2026

View reviewed changes

nowycondro reviewed Oct 30, 2026

View reviewed changes

volen-silo added 2 commits October 30, 2026 10:49

volen-silo force-pushed the rocmai-745-report-trend branch from 52de03b to 640b384 Compare October 30, 2026 08:59

nowycondro reviewed Oct 30, 2026

View reviewed changes

		"rises_evaluated": view.rise.is_meaningful()
		&& self.span_weeks() >= view.rise.weeks_needed(),

Conversation

volen-silo commented Oct 28, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What changes

Tests

Run locally (Windows 11)

Not run locally

CI

Uh oh!

jussielo-amd left a comment

Choose a reason for hiding this comment

Review of ROCMAI-745's weekly trend / rise detection (full review, read blind against the diff before this description)

CI note (not a code issue)

Non-blocking

Worth confirming was deliberate (not blocking, just flagging for the record)

Minor, your call entirely

What stood out positively

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nowycondro left a comment

Choose a reason for hiding this comment

Review — needs work

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Uh oh!

volen-silo commented Oct 28, 2026

Uh oh!

jussielo-amd left a comment

Choose a reason for hiding this comment

Review — needs work

Blocking

Non-blocking

Decisions for the author

Positive signals

Deployment notes

What this covered

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

volen-silo commented Oct 28, 2026 •

edited

Loading