Repository navigation
fix(e2e): normalize agent smoke status to lowercase "pass"/"fail" - #491
Conversation
chao-xu-spec
left a comment
There was a problem hiding this comment.
Verdict: the fix is correct and complete, with no regression. I'd like two small changes before merge.
What I validated (MI355X ×8, GLM-5.2-MXFP4, sglang TP8)
| Check | main (cfd5b58) |
this PR (695c895) |
|---|---|---|
| Probe: extract agents on real GLM-5.2 kernel tasks, native Claude Workflow runtime | 21 samples, all exactly pass/fail, 0 prose |
21 samples, all exactly pass/fail |
| Probe: same tasks, standalone runtime (codex backend) | 23 samples, all exactly pass/fail, 0 prose |
23 samples, all exactly pass/fail |
Full e2e, phases=head, fast mode, same carried setup state |
dense GEMM head pass → bakeoff author_recommended → author; MoE head fail (real infra blocker) → dropped |
identical behavior; agents also filled smoke_detail |
| Schema rejections seen in e2e transcripts | n/a | 0 |
- The enum works as intended. In a probe where I pushed the agent to answer in prose, its first StructuredOutput was
"PASS (exit 0) on GPU 4". The runtime rejected it with/unittest_smoke: must be equal to one of the allowed values: ["pass","fail"], and the agent re-answered"pass"in the same session. The gate then let it through. - Gate coverage is complete. Smoke status only comes from
EXTRACT_OP_SCHEMA.smokeandEXTRACT_SCHEMA.unittest_smoke, and every consumer compares against'pass'(L1545, L3695, L4060, L4268, L4586). A schema-valid"pass"can no longer be read as a failure. - I could not reproduce the original bug. On main, none of the 44 probe samples and none of the extraction results in the full e2e run were in prose form. The commit message cites the 0924/0925 runs. Could you link the log lines or journal entries where
smokewas prose such as"PASS (exit 0) ..."? That would show how often this actually happens. It doesn't block the PR, since the change is a cheap and correct hardening either way.
1. Failure reasons move into smoke_detail, which nothing reads
On main, every extraction-failure log line builds its reason as notes || smoke. This PR keeps that unchanged:
- L3696:
const why = ext ? ext.notes || ext.smoke : 'none'; - L4082:
const why = p.ext ? p.ext.notes || p.ext.smoke : 'none'; - L4269:
const why = ext ? ext.notes || ext.smoke : 'none'; - L4587:
reason: `extraction failed/non-editable (${ext ? ext.notes || ext.unittest_smoke : 'none'})`
The PR changes where the agent writes the reason, though:
- The status field can now only hold
"fail". The fallbackext.smokeused to carry text like"fail: gpu_lock refused ...", and now it yields just"fail".notesis not inrequired, so it can be empty. - The new contract (kernel_extractor.md L82-83) tells the agent to put the command, exit code and caveats in
smoke_detail.smoke_detailis defined in the schema (L845, L867) but nothing reads it.
Observed on real hardware. The task was glm52_dsa_indexer_topk, the standalone runtime used the codex backend, and in both runs gpu_lock refused to run because GPU 2 was busy:
# main: the cause is in notes, so it appears in the skip reason
unittest_smoke: "fail"
notes: "The prescribed smoke command exited 1 before unittest.py ran because gpu_lock
reported that GPU 2 had foreign work running or VRAM above the permitted threshold. ..."
# this PR: the cause is only in smoke_detail; notes is unrelated, so the skip reason loses it
unittest_smoke: "fail"
notes: "The task uses synthesized fixed-seed inputs and has no reference_io.pt. ...
No files under TASK were created, regenerated, captured, or edited."
smoke_detail: "Ran `... gpu_lock.sh 2 python3 unittest.py`. Exit code 1: GPU 2 had foreign work
running above the lock's busy/VRAM threshold, so the lock refused measurement ..."
In the full e2e run the MoE notes happened to include the blocker, so the reason survived there. That was luck, not something the contract guarantees.
Suggested fix: apply this to all four sites, or factor it into a small helper.
- const why = ext ? ext.notes || ext.smoke : 'none';
+ const why = ext ? [ext.smoke_detail, ext.notes].filter(Boolean).join(' | ') || ext.smoke : 'none';A test assertion that each extraction-failure reason includes smoke_detail would keep this from regressing.
2. The new contract contradicts the exit-3 rule
The new section (kernel_extractor.md L81-82) says:
"pass"means the smoke run exited 0 AND correctness passed; anything else is"fail".
The existing rule (L491-496) says:
On smoke exit 3 ... REGENERATE the UT ... Do NOT record
unittest_smoke:"fail"or drop the head for exit 3 ... Only after 3 failed regenerations setunittest_smoke:"fail"
An agent that reads "anything else is fail" first may report "fail" on exit 3 and skip the regeneration. Suggested wording:
-passed; anything else is `"fail"`.
+passed; a genuine failure is `"fail"` (exit 3 / `UT_HARNESS_INCOMPLETE` is NOT a failure yet:
+regenerate the UT first, see PHASE=extract step 6).Summary: the change is good to go once these two items are addressed.
Review of AMD-AGI#491: 1. With smoke/unittest_smoke pinned to "pass"/"fail", the failure prose moved to smoke_detail, but every extraction-failure site still built its reason as `notes || smoke`, so a cause that was only in smoke_detail (e.g. a gpu_lock refusal while notes was unrelated) collapsed to a bare "fail". Add extractFailWhy() (smoke_detail, then notes, then the status) and use it at all four sites. When extractWithBaseline overrides an agent "pass" with a forced "fail", it now also sets smoke_detail to the workflow's reason, so the reason never opens with the agent's stale "CORRECTNESS: PASS". 2. The new contract said "anything else is fail", contradicting the exit-3 rule (regenerate the UT first). Reword it to defer to PHASE=extract step 6, and have step 6 put its give-up reason (harness_incomplete_unrecoverable) in smoke_detail instead of a `reason` field the workflow never reads. test_smoke_status_contract.js runs extractFailWhy from the shipped source (gpu_lock and forced-fail cases), forbids `notes || status` reasons, and pins the exit-3 wording. Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
EXTRACT_OP_SCHEMA.smoke and EXTRACT_SCHEMA.unittest_smoke were free strings, while every extraction gate tests `=== 'pass'`. Agents answered things like "PASS (exit 0) on GPU 1 ...", so a passing unittest was recorded as "op extraction failed" and the head's bake-off / kernel lane was skipped (0924/0925: MoE sorting, topk gating, CK blockscale / MoE GEMM, router fusion). - Both fields are now enum ['pass', 'fail']; StructuredOutput rejects anything else so the agent must re-answer. Free text moves to a new smoke_detail field. - kernel_extractor.md states the contract and its output examples use "pass" + smoke_detail instead of the ambiguous "pass|fail" placeholder. - test_smoke_status_contract.js extracts the shipped schemas and checks the enum, rejects the 7 real-world strings, and pins the gates + role doc; wired into CI L0.
Review of AMD-AGI#491: 1. With smoke/unittest_smoke pinned to "pass"/"fail", the failure prose moved to smoke_detail, but every extraction-failure site still built its reason as `notes || smoke`, so a cause that was only in smoke_detail (e.g. a gpu_lock refusal while notes was unrelated) collapsed to a bare "fail". Add extractFailWhy() (smoke_detail, then notes, then the status) and use it at all four sites. When extractWithBaseline overrides an agent "pass" with a forced "fail", it now also sets smoke_detail to the workflow's reason, so the reason never opens with the agent's stale "CORRECTNESS: PASS". 2. The new contract said "anything else is fail", contradicting the exit-3 rule (regenerate the UT first). Reword it to defer to PHASE=extract step 6, and have step 6 put its give-up reason (harness_incomplete_unrecoverable) in smoke_detail instead of a `reason` field the workflow never reads. test_smoke_status_contract.js runs extractFailWhy from the shipped source (gpu_lock and forced-fail cases), forbids `notes || status` reasons, and pins the exit-3 wording.
6e0fcb1 to
8b276e2
Compare
|
Addressed both review comments in 8b276e2:
|
Summary
Extraction gates check
smoke === 'pass', but the schema accepted any string. Agents often returned text like"PASS (exit 0) ...", so a passing unittest was treated as a failed extraction and the kernel was skipped for optimization.Changes
smoke/unittest_smoketo"pass"or"fail"via schemaenum; other values are rejected and the agent must re-answer.smoke_detailfield for free-text details.kernel_extractor.md.test_smoke_status_contract.jsto CI L0.Test
main.