Skip to content

fix(email): eval-pass follow-ups — ctx-stamped baseline, refresh limit, usage aggregate#2089

Merged
itomek merged 3 commits into
mainfrom
tmi/eval-pass-followups
Jul 15, 2026
Merged

fix(email): eval-pass follow-ups — ctx-stamped baseline, refresh limit, usage aggregate#2089
itomek merged 3 commits into
mainfrom
tmi/eval-pass-followups

Conversation

@itomek

@itomek itomek commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator

Follow-ups from the #1319 consolidated eval pass, cherry-picked onto main after the #2072 train merged (the commits raced the merge). Three small, independent threads:

Test plan

  • tests/unit/eval tests/unit/email: 689 passed (single failure is the known pre-existing test_claude_judge machine-.env artifact — passes in CI)
  • Full widened suite on the identical pre-merge content: 1819 passed · lint clean · YAML validated
  • The baseline number itself is real hardware evidence (299/299 scored at pinned ctx on Strix Halo; run artifacts retained)

Tomasz Iniewicz added 3 commits July 14, 2026 21:48
#1892)

Re-measured on Strix Halo via tests/fixtures/email/score_baseline.py
--ctx-size 16384 (category_accuracy 0.796, 299 scored). The prior 0.8094 was
recorded at the silent 64K Lemonade default, not the #1892 pinned envelope —
0.796 is the honest number for the window the agent actually ships under.
…tx (#2087)

60 emails' fixed ~8K aggregation-turn overhead plus ~190 tokens/email totals
~19.8K tokens, guaranteed context_length_exceeded at the #1892 16384 window.
25 emails (~12.8K) fits with margin. Restore a larger limit once #2087's
chunking lands.
The email benchmark's aggregate scorecard performance block silently
dropped triage_llm_tokens / llm_classified_count / tokens_per_triage even
on a fully-passing run: build_scorecard() is shared across every eval
category and has no notion of these email-specific fields, which only ever
landed in each run's per-scenario performance_summary. summarize_benchmark
already patches ctx_size onto the scorecard after build_scorecard() runs;
this adds the same domain-specific merge for the #1891 triage-usage
fields, backed by a new test that pins the aggregate shape (as opposed to
the existing hermetic test, which only covered the per-scenario one).
@github-actions github-actions Bot added devops DevOps/infrastructure changes eval Evaluation framework changes tests Test changes performance Performance-critical changes labels Jul 15, 2026
@itomek
itomek marked this pull request as ready for review July 15, 2026 02:02
@itomek
itomek requested a review from kovtcharov-amd as a code owner July 15, 2026 02:02
@itomek
itomek enabled auto-merge July 15, 2026 02:02
@itomek itomek self-assigned this Jul 15, 2026
@github-actions

Copy link
Copy Markdown
Contributor

Verdict: Approve

Three small, independent eval-pass follow-ups: an honest baseline re-recorded at the pinned 16K context window, a workflow triage-limit cut from 60 → 25 so it stops overflowing that window, and a fix so the aggregate scorecard stops silently dropping email's per-triage token metrics. Each thread is well-justified, the two code paths are tested for both the present and absent shapes, and the baseline change is real hardware evidence rather than a hand-edit. Nothing blocking.

🔍 Technical details

🟢 Minor / nits

Aggregate means can use mismatched denominators (src/gaia/eval/benchmark.py:453)_aggregate_triage_usage builds llm_tokens_vals, classified_vals, and per_triage_vals as three independent lists, so if one run had llm_classified_count == 0 (no tokens_per_triage emitted) but a non-zero triage_llm_tokens, the reported tokens_per_triage would be averaged over a different run set than the token/count means it should reconcile with. In practice every run scores the identical corpus so this can't realistically fire — noting it only as a latent inconsistency, not a merge blocker. No change required.

Strengths

  • Baseline stamp is generator-produced, not hand-editedctx_size: 16384 matches what score_baseline.py:192 emits from recorded_ctx, and the numbers reconcile (238/299 = 0.796), so the "honest pinned-window figure" claim holds up. tolerance_pp: 5 leaves the gate appropriate headroom.
  • Both shapes tested — the new aggregate path has a presence test and a no-fabrication absence test (test_benchmark.py:319, :346), mirroring the existing per-run coverage. Good regression pinning for a bug that only showed on real, fully-passing hardware runs.
  • Clean reuse of the existing pattern — merging triage usage via post-build_scorecard enrichment follows the same convention ctx_size already uses, rather than reaching into the shared generic scorecard builder. The None-when-absent contract keeps non-email categories untouched.
  • Workflow change is defense-in-depth documented — the 60 → 25 cut is justified inline with the token math (~8K fixed + ~190/email vs the 16K window) and both the input default and the LIMIT env fallback were updated together, so a manual workflow_dispatch can't silently reintroduce the overflow.

@itomek

itomek commented Jul 15, 2026

Copy link
Copy Markdown
Collaborator Author

Acknowledging the denominator note as-is (reviewer marked it non-blocking and it cannot fire while every run scores the identical corpus): the three lists are per-run parallel today, and the case where they'd diverge — a run with classified_count 0 but non-zero triage tokens — would itself indicate broken accounting upstream. Worth revisiting if #2087's chunking changes run composition; noted there implicitly via the shared code path.

@itomek
itomek added this pull request to the merge queue Jul 15, 2026
Merged via the queue into main with commit e65e879 Jul 15, 2026
54 of 56 checks passed
@itomek
itomek deleted the tmi/eval-pass-followups branch July 15, 2026 13:31
itomek pushed a commit that referenced this pull request Jul 15, 2026
…ection

specification.html carried a hand-edited 'Context-window envelope' section
(added in #2072) that spec_html.py never emitted, so the #2082 drift guard
failed and any regeneration silently dropped the section. Move it into the
generator with the bounds introspected from context_budget constants, refresh
the stale 'no committed artifact carries that stamp yet' claim (the baseline
is ctx-stamped since #2089), and regenerate the artifact so check_artifact
passes byte-for-byte.
pull Bot pushed a commit to bhardwajRahul/gaia that referenced this pull request Jul 15, 2026
)

Main's email test suite has been red since amd#2082 landed this morning, so
every email PR now opens onto a failing suite and a release cut from
main inherits it. amd#2082 added a drift guard asserting
\`specification.html\` is a pure render of \`spec_html.py\` — but the
artifact already carried a hand-added "Context-window envelope" section
(from amd#2072) that the generator never emitted, so the guard failed on
its first run and every run since. This makes the generator own that
section (bounds read from the \`context_budget\` constants rather than
hand-typed), which fixes the guard and means a future regeneration can't
silently delete the section — the thing amd#2082 was written to prevent.

Extracted from amd#2088 so the release isn't gated on that PR's hardware
validation.

## Evidence

Before — the guard cannot pass on main, because the generator does not
emit what the artifact contains:

```
$ git show origin/main:.../spec_html.py       | grep -c "Context-window envelope"   → 0
$ git show origin/main:.../specification.html | grep -c "Context-window envelope"   → 1
```

Main CI agrees ([run on
e65e879](https://github.com/amd/gaia/actions/runs/29433041545)):

```
hub/agents/python/email/tests/test_spec_html_artifact.py::test_committed_spec_html_artifact_is_up_to_date FAILED [ 95%]
E   AssertionError: specification.html is stale — it has drifted from spec_html.py.
```

After — on this branch:

```
$ python -m pytest hub/agents/python/email/tests/test_spec_html_artifact.py -q
....                                                                     [100%]
4 passed in 0.72s
```

The regenerated artifact diff is 3 lines: the section is now
generator-emitted, and its stale "no committed artifact carries that
stamp yet" claim is corrected (the baseline has been ctx-stamped since
amd#2089).

## Test plan

- [x] \`pytest
hub/agents/python/email/tests/test_spec_html_artifact.py\` — 4 passed
(was 1 failed on main)
- [x] \`python -m gaia_agent_email.spec_html\` regenerates
byte-identically (guard is self-checking)
- [ ] CI: Email Agent Unit Tests green on this branch (red on main)

Co-authored-by: Tomasz Iniewicz <tomasz@iniewicz.com>
pull Bot pushed a commit to bhardwajRahul/gaia that referenced this pull request Jul 16, 2026
…uard the published basis (amd#2107)

Closes amd#2094

## Why this matters

The email agent's published quality card — the accuracy figure
integrators rely on — is refreshed by an automated workflow that was
supposed to reject any change making quality worse. Two hours ago that
workflow did the opposite: it auto-committed a card claiming **100%
accuracy measured on 25 emails in a single run**, silently replacing the
real figure (83.4%, measured over 249 emails, repeated 3 times). Its
regression check approved the swap *because 1.0 beats 0.834* — it
compares only the number, never the basis the number was measured on, so
a small lucky sample reads as an improvement. That card is sitting on an
open, merge-ready PR right now (flagged in amd#2060); `main` is still
clean.

That incident reframes this issue. amd#2094 was filed because the workflow
had never once completed — a hand-typed `timeout-minutes: 90` against a
~2h workload. amd#2089's interim limit cut (60→25) made it fast enough to
finish, which means the failure mode changed rather than went away: **a
workflow that reliably died became one that reliably publishes a
meaningless number.** The loud failure was the safer bug.

After this PR: a subset-sized run physically cannot commit a card, a run
that would downgrade the published basis (fewer emails, fewer repeats,
or a different context window) is rejected in seconds before spending
any eval time, a sub-bar card can never reach the published surface, and
an overrun fails at a named step instead of a silent job-level
`cancelled`. The per-push trigger — which fired the incident above,
unasked, from an unrelated refactor branch merging `main` — is gone;
refresh is now a deliberate dispatch, and the budget is derived from
measured step times with the arithmetic recorded in the workflow.

## Evidence

**1. The bug this prevents, firing in production** — [run
29457164041](https://github.com/amd/gaia/actions/runs/29457164041) (old
workflow, `main`'s version) auto-committed
[`2e21edb9`](amd@2e21edb9):

| | before | after the auto-commit |
|---|---|---|
| corpus / repeats | 250 / 3 | **25 / 1** |
| within-one accuracy | 0.834 (stdev 0.0116) | **1.0 (stdev 0.0)** |

Under this PR that run is a SUBSET profile: it evaluates, prints `SUBSET
RUN -- card NOT committed`, restores the card, and exits 0.

**2. Basis guard rejects a downgrade before spending eval time** — [run
29458595361](https://github.com/amd/gaia/actions/runs/29458595361),
dispatched `limit=250 experiments=3 ctx_size=16384 rebaseline=false`:

```
##[error]Basis mismatch -- committed card ctx_size=<unstamped> vs requested ctx_size=16384.
Re-dispatch at the committed ctx_size to compare apples-to-apples, or pass rebaseline=true
with a rebaseline_reason to move the basis deliberately.
Re-dispatch command: gh workflow run email_scorecard_refresh.yml --ref ... -f rebaseline=true ...
```

Failed at the **named step** `Resolve version, committed basis, and fail
fast on a basis mismatch` in **6 seconds** — the whole run cost ~2
minutes instead of ~2 hours. This is AC-3 (loud, step-named failure) and
the AC-4 guard in one artifact.

**3. Subset end-to-end + 4. full-corpus terminal step** — dispatched;
posted as a follow-up comment.

<details>
<summary>Budget derivation (AC-2) — measured, not guessed</summary>

Ceilings derive from [run
29433392568](https://github.com/amd/gaia/actions/runs/29433392568) and
are recorded in the workflow header beside the arithmetic. The judge
legs are fixed-size (19/21/11 cases) and don't shrink with `limit`, so
they dominate a subset run: tonight's 50-minute run spent 34:25 of it in
judges.

| step | ceiling (full / subset) |
|---|---|
| benchmark | 620 / 45 |
| drafting · action-item · briefing judges | 35 · 45 · 25 (both
profiles) |
| setup | 35 |
| five short steps @10 | 50 |
| **job** | **840 (sum 810) / 240 (sum 235)** |

The old value, 90, was never derived from a measurement. The job timeout
is now only a backstop — every long step carries its own ceiling so an
overrun names itself.
</details>

## Tests

- [x] Unit: `python -m pytest tests/unit/eval/ -q` → **471 passed, 1
deselected** (pre-existing worktree `.env` failure, reproduced
identically at `origin/main`)
- [x] Judge pin (AC-5): `python -m pytest
tests/unit/eval/test_claude_judge.py -v` → **28 passed**; 18 new tests
cover all 6 `messages.create` sites across 4 public methods. Tests were
authored against the acceptance criteria before the implementation
existed, and pin the judge to `temperature=0.0` — with agent generation
already at temp 0, the judge's own sampling was the last uncontrolled
variance source
- [x] Lint: `python util/lint.py --all` → Black / isort / Pylint /
Flake8 OK
- [x] Static: `actionlint` → 31→30 findings, **zero new** (remainder are
pre-existing PowerShell-parsed-as-bash false positives already on
`main`). Static only — it cannot prove the ternaries evaluate correctly
or that PS 5.1 survives the string encoding; the dispatched runs above
are the real validation
- [x] Real-world: basis-guard fail-fast (evidence 2) ✅
- [ ] Real-world: subset end-to-end, terminal = NOT-COMMITTED
- [ ] Real-world: full-corpus, terminal = commit or loud rejection

## Notes for the reviewer

- **The full-corpus run is expected to *reject*, not commit.** Prior
measurement puts 16K at ~0.796 against the enforced 0.80 floor. That is
the guard working: the card stays at its current basis and the measured
number goes back for a decision (adjacent to amd#2090). The bar does not
move to make the run pass.
- **Known residual gap:** this workflow can only govern what *it*
commits. A human hand-editing `SCORECARD.md` in an unrelated PR bypasses
every guard here.
- **Follow-up owed, deliberately not absorbed:** six sibling workflows
(`test_api`, `test_embeddings`, `test_lemonade_server`, `test_rag`,
`build_cpp`, `test_examples`) share the `stx` runner label *outside* the
`lemonade-eval` concurrency group, and most run `cleanup-lemonade.ps1`,
which force-kills port 13305 — so an unrelated PR can kill a long eval
mid-run. Widening the group is the right fix and needs its own issue.
- **Unrelated anomaly spotted in the incident data:** the briefing judge
finished in 36s against a ~11-15 min estimate — possibly a silent no-op.
Noted on amd#2094, not fixed here.
- `scorecard_gate.py` is touched additively only: `_env_ctx_size` →
public `env_ctx_size` (+4 tests), because the guard must read the ctx
stamp from `recipe.environment`, and re-deriving that path in workflow
PowerShell is how this class of bug gets written twice. No gate
semantics, thresholds, or flags changed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

devops DevOps/infrastructure changes eval Evaluation framework changes performance Performance-critical changes tests Test changes

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants