fix(agent): make the functional-test report say what it actually tested - #3524
Merged
Merged
Conversation
A registry entry only ever describes a break the revision under test did not cause, but annotate added a comment and left the status at `fail`, which the report reads as "this PR broke it". apply_baseline was covering for that by accident: it downgrades a failure the baseline also hit, and the baseline was red for the same reason. The moment a fix existed and the baseline went green, three innocent PRs were reported as breaking the suite with a link to the fix printed directly underneath. Downgrade on match instead. Carry unmerged out-of-repo fixes automatically while we are here. A NeMo-RL fix reaches later runs on the integration branch; a Megatron-Bridge one cannot ride a NeMo-RL branch, so it had to be remembered as --bridge-ref on every submit for as long as review took. run_suite.sh now asks the registry, via a new pending-fix-ref query, and refuses to guess when two fixes are pending at once. Signed-off-by: shanmugamr1992 <shanmugamr1992@gmail.com>
The header carried four bare shas, retyped by hand into --meta on every post. Nothing checked them against the run, so a stale copy-paste read exactly like a result, and the refs that give a sha its meaning were dropped on the way. Which mattered more once runs stopped being uniform. Megatron-Bridge is either the sha NeMo-RL pins or a fix branch carried in its place, and those are different claims about what a green table proves. NeMo-RL is normally a fork branch, so its sha links nowhere the reader can check. megatron-core `main` and a PR head are indistinguishable once written as hex. Render the table from the prep block the run already recorded instead, naming ref and commit together and linking each to the repo it was fetched from. The clone URLs are now recorded so those links can be built at all. The watchdog issue imports the same renderer rather than keeping its own, which had begun asserting a bare Bridge sha that is no longer necessarily the pin. Signed-off-by: shanmugamr1992 <shanmugamr1992@gmail.com>
shanmugamr1992
marked this pull request as ready for review
August 6, 2026 20:12
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Follow-up to #3522, from continuing to pull on the same sweep. Two commits, each
fixing a way the functional-test report could mislead the person reading it.
1. Known breaks were reported as PR-caused failures
known_issues.py annotatelabelled a matched failure with its diagnosis and fixlink but left
status: fail, which the report vocabulary defines as "genuinefailure caused by the PR". Nothing looked wrong while
apply_baseline.pyhappened to cover for it: it downgrades any failure the cached baseline also
hit, and the baseline was red for the same reason.
That cover disappears exactly when a fix is raised. The baseline gets re-taken on
a stack carrying the fix, goes green, stops matching — so the next sweep reports
the same known break as
failagainst whichever PRs are under test, with alink to the fix printed directly underneath.
An entry only ever exists for a break the PR did not cause, so
annotatenowdowngrades to
fail (pre-existing)on match. It deliberately does not readfirst_seen_megatron_pras a culprit: that field records which run firstsurfaced the break, usually neither the cause nor even the same repository — the
Bridge break in the registry was first seen on Megatron-LM#5382 and caused by
#5865, merged weeks earlier.
Carrying a fix the integration branch cannot reach.
sync_integration.shcherry-picks open NeMo-RL fixes onto the branch under test, so those reach later
runs by themselves. A Megatron-Bridge or Megatron-LM fix is checked out inside
the container instead, so it had to be passed as
--bridge-refon every singlesubmit until it merged; forgetting it produces a confident wrong answer rather
than a missing one.
run_suite.shnow asks the registry through a newpending-fix-refquery, and errors rather than guessing when two fixes arepending in one repo. This makes
--fix-branchload-bearing atrecordtime,which the triage and fix-delivery skills now say.
2. The report did not say which revisions it ran
The header carried four bare shas, retyped by hand into
--metaon every post.Nothing checked them against the run, so a stale copy-paste read exactly like a
result, and the refs that give a sha its meaning were dropped on the way.
That got worse once runs stopped being uniform. Megatron-Bridge is now either the
sha NeMo-RL pins or a fix branch carried in its place, and those are different
claims about what a green table proves. NeMo-RL is normally a fork branch, so its
sha links nowhere a reader can check. megatron-core
mainand a PR head areindistinguishable once written as hex.
The report now builds this from the
prepblock the run already recorded:eb01b689mcore-5382-fix— an override, not the Bridge NeMo-RL pins2940a635nrlta/integration86774472Clone URLs are recorded in
prepso those links can be built at all. Thewatchdog issue imports the same renderer rather than keeping its own, which had
begun asserting a bare Bridge sha that is no longer necessarily the pin.
Test plan
test_known_issues.sh— 18 checks, extended with the downgrade, thesuspect-pass and unmatched-failure cases that must not move, the
stale-entry case that keeps its
fail, andpending-fix-refon none /one / two pending fixes
test_report_stack.sh— new; 17 checks covering all three Bridge refshapes, the fork link, PR-head vs
main, header de-duplication, resultspredating the URL record,
--state running, and the watchdog headingrun_suite.sh --dry-runpicks upmcore-5382-fixfrom the live registrytopp_topkruff check/ruff format