Skip to content

Commit 4dbc5a1

Browse files
rafaelgilnRafaelclaude
authored
fix(migration): stop the report claiming PASSED when the red step is outside every phase (#1141) (#1142)
#1120 taught the report to reconcile the runner's `steps.<id>.outcome` against what each phase recorded, so a phase that crashed before writing its verdict can no longer render as `Result: PASSED`. Only three steps carry an `id`, so only three steps were reconciled. The steps BETWEEN them carry none: resolving the nightly version from PyPI, installing it, starting it, and waiting out the alembic migration. A failure there stops the job, so phases 2 and 3 never run and their outcomes arrive empty — `declared_outcomes()` filters those out, the state file still holds a fully-passing `latest` phase, and the report printed `Result: PASSED` into an issue titled "Langflow Migration Test Failed". The nightly failing to boot against a migrated database is exactly what this workflow exists to catch, and it landed in that gap. The workflow now also hands over `JOB_STATUS` (`job.status`, already used by the summary step below it). A job the runner reports as `failure`/`cancelled`, whose report found no failure and no integrity problem, is itself an integrity problem: the failure is real and lives outside every phase the report can see. It is only raised when nothing else attributes the failure — the phase-level message says it better and duplicating it would be noise. Reconciling the job status covers the four steps that exist today and any added later, which per-step `id`s would not. Also: `## Unaccounted phases` is now `## Unaccounted failures`, since the section can hold an entry that belongs to no phase; the header states the runner's verdict next to the report's, the pair that contradicted each other; and `__pycache__/` is ignored — running the Python tests locally left untracked noise in every subsequent diff. Co-authored-by: Rafael <rafael@oriontech.me> Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>
1 parent 56d522b commit 4dbc5a1

4 files changed

Lines changed: 161 additions & 6 deletions

File tree

.github/workflows/migration-test.yml

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -219,12 +219,19 @@ jobs:
219219
# opened an issue titled "Failed" whose body said PASSED for exactly that
220220
# reason — `test_05_execute_flow_ui` died on a Playwright timeout before it
221221
# could write its own verdict.
222+
#
223+
# `JOB_STATUS` closes the other half (#1141): only these three steps carry an
224+
# `id`, so a failure in the steps BETWEEN them — resolving/installing/booting
225+
# the nightly, or the alembic migration timing out — left phases 2 and 3
226+
# unrun, their outcomes empty, and the report saying PASSED on a red job. The
227+
# job's own status covers those steps, and any step added here later.
222228
- name: Generate report
223229
if: always()
224230
env:
225231
PHASE_OUTCOME_latest: ${{ steps.phase_latest.outcome }}
226232
PHASE_OUTCOME_nightly_api: ${{ steps.phase_nightly_api.outcome }}
227233
PHASE_OUTCOME_nightly_ui: ${{ steps.phase_nightly_ui.outcome }}
234+
JOB_STATUS: ${{ job.status }}
228235
run: python tests/github-workflows/migration/generate_report.py
229236

230237
- name: Print report

.gitignore

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -44,3 +44,9 @@ all-liveness/
4444
# run's commit id and the machine's system data, so a committed copy is stale on
4545
# arrival. Same category as playwright-report/ and blob-report/ above.
4646
flakiness-report/
47+
48+
# Python bytecode from the migration tests (tests/github-workflows/migration/).
49+
# Those tests arrived with #1139's python-units lane; running them locally leaves
50+
# __pycache__ behind, which showed up as untracked noise on every subsequent diff.
51+
__pycache__/
52+
*.pyc

tests/github-workflows/migration/generate_report.py

Lines changed: 60 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,22 @@
2323
recorded no failure, is reported as a crash — never as a pass. A phase that was
2424
supposed to run and is missing from the state entirely is reported as incomplete.
2525
Nothing here can conclude "passed" from missing data.
26+
27+
## Why the job's own status is reconciled too (#1141)
28+
29+
Only three steps carry an `id`, so `PHASE_OUTCOME_*` covers only the three
30+
verification phases. The steps **between** them — resolving, installing, booting
31+
the nightly, and waiting out the alembic migration — carry none, and a failure
32+
there reproduced the #1120 symptom by a different route: the job goes red, phases 2
33+
and 3 never run (so their outcomes arrive empty and are not declared), the state
34+
file holds a fully-passing `latest` phase, and the report printed `Result: PASSED`
35+
into an issue titled *"Failed"*. A migration that never completes is precisely what
36+
this workflow exists to catch, and it landed in that gap.
37+
38+
So the workflow also hands over `JOB_STATUS` (`job.status`). A red job whose report
39+
found nothing wrong is itself an integrity problem: the failure is real and lives
40+
outside every phase this report can see. Reconciling the job status covers the
41+
steps that exist today *and* any added later, which per-step `id`s would not.
2642
"""
2743

2844
import json
@@ -47,6 +63,13 @@
4763
# `steps.<id>.outcome` vocabulary, passed straight through.
4864
PHASE_OUTCOME_PREFIX = "PHASE_OUTCOME_"
4965

66+
# The job's own status at the time the report is generated, from `job.status`
67+
# (`success|failure|cancelled`). The report step runs under `if: always()`, so this
68+
# is the runner's verdict on everything that happened before it — including the
69+
# steps no `PHASE_OUTCOME_*` covers.
70+
JOB_STATUS_VAR = "JOB_STATUS"
71+
JOB_STATUS_NOT_OK = {"failure", "cancelled"}
72+
5073
RESULT_FAILED = "FAILED"
5174
RESULT_PASSED = "PASSED"
5275
RESULT_PASSED_WARN = "PASSED (with warnings)"
@@ -81,13 +104,24 @@ def declared_outcomes(environ=None) -> dict:
81104
return out
82105

83106

84-
def assess(state: dict, declared: dict) -> dict:
107+
def job_status(environ=None) -> Optional[str]:
108+
"""The runner's verdict on the job so far, from `JOB_STATUS`. `None` if unset."""
109+
env = os.environ if environ is None else environ
110+
value = (env.get(JOB_STATUS_VAR) or "").strip().lower()
111+
return value or None
112+
113+
114+
def assess(state: dict, declared: dict, job: Optional[str] = None) -> dict:
85115
"""Decide the overall result, and surface anything that makes the report untrustworthy.
86116
87117
Returns `{"result", "failures", "warnings", "integrity"}`. `integrity` holds
88118
mismatches between what the runner observed and what the phase recorded — the
89119
#1120 class. Those count as failures: a report that cannot account for a phase
90120
must not claim that phase passed.
121+
122+
`job` is the runner's verdict on the whole job (`job.status`). It catches the
123+
#1141 case: a failure in a step no phase covers, which otherwise left the report
124+
saying `PASSED` on a red run.
91125
"""
92126
phases = state.get("phases", {}) or {}
93127

@@ -132,6 +166,18 @@ def assess(state: dict, declared: dict) -> dict:
132166
"verified nothing. Treated as a failure rather than an empty pass."
133167
)
134168

169+
# #1141: the job is red but nothing above accounts for it — the failing step is
170+
# one this report does not cover. Only reported when there is no attribution yet;
171+
# a phase that already owns the failure says it better than this can.
172+
if job in JOB_STATUS_NOT_OK and not failures and not integrity:
173+
integrity.append(
174+
f"- **(outside every phase)**: the runner reports this job `{job}`, but no "
175+
f"verification phase recorded or was declared a failure — so the cause is a "
176+
f"step this report does not cover (resolving, installing or booting the "
177+
f"nightly, or the alembic migration timing out). Read the job log; this "
178+
f"report cannot attribute it."
179+
)
180+
135181
if failures or integrity:
136182
result = RESULT_FAILED
137183
elif warnings:
@@ -160,21 +206,31 @@ def format_step(name: str, step: dict) -> str:
160206
return line
161207

162208

163-
def generate_report(state: dict, declared: Optional[dict] = None) -> str:
209+
def generate_report(
210+
state: dict, declared: Optional[dict] = None, job: Optional[str] = None
211+
) -> str:
164212
if declared is None:
165213
declared = declared_outcomes()
214+
if job is None:
215+
job = job_status()
166216
now = datetime.now(timezone.utc).strftime("%Y-%m-%d %H:%M UTC")
167217
latest_digest = load_digest("/tmp/latest-digest.txt")
168218
nightly_digest = load_digest("/tmp/nightly-digest.txt")
169219

170-
verdict = assess(state, declared)
220+
verdict = assess(state, declared, job)
171221

172222
lines = [
173223
"# Langflow Migration Test Report",
174224
f"**Date:** {now}",
175225
f"**Flow:** {state.get('flow_name', 'N/A')} (`{state.get('flow_id', 'N/A')}`)",
176226
f"**Latest digest:** `{latest_digest[-20:]}`",
177227
f"**Nightly digest:** `{nightly_digest[-20:]}`",
228+
]
229+
# Stated in the header so a reader comparing the issue title to the body can see
230+
# both verdicts at once — the pair that contradicted each other in #1120/#1141.
231+
if job:
232+
lines.append(f"**Job status (runner):** `{job}`")
233+
lines += [
178234
"",
179235
f"## Result: {verdict['result']}",
180236
"",
@@ -184,7 +240,7 @@ def generate_report(state: dict, declared: Optional[dict] = None) -> str:
184240
# trusted at face value — burying it below the per-step tables is how run #115
185241
# read as a pass.
186242
if verdict["integrity"]:
187-
lines.append("## Unaccounted phases")
243+
lines.append("## Unaccounted failures")
188244
lines.append("")
189245
lines.extend(verdict["integrity"])
190246
lines.append("")

tests/github-workflows/migration/test_generate_report.py

Lines changed: 88 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -163,12 +163,98 @@ def test_the_rendered_report_leads_with_the_unaccounted_phase():
163163
)
164164

165165
assert "## Result: FAILED" in body
166-
assert "## Unaccounted phases" in body
167-
assert body.index("## Unaccounted phases") < body.index("### Phase: latest")
166+
assert "## Unaccounted failures" in body
167+
assert body.index("## Unaccounted failures") < body.index("### Phase: latest")
168168
# The runner's own verdict is shown next to the phase it belongs to.
169169
assert "runner outcome: `failure`" in body
170170

171171

172+
# ── The job-status half of the same defect (#1141) ────────────────────────────
173+
#
174+
# The exact shape of a run whose nightly never came up: `latest` fully recorded and
175+
# passing, phases 2 and 3 never reached, so the runner hands over empty outcomes for
176+
# them. Before the fix this rendered `Result: PASSED` into an issue titled "Failed".
177+
NIGHTLY_NEVER_BOOTED_STATE = {
178+
"phases": {
179+
"latest": {
180+
"steps": {"auth": {"status": "pass"}, "execute_flow": {"status": "pass"}}
181+
}
182+
}
183+
}
184+
185+
186+
def test_a_red_job_with_no_phase_failure_is_not_a_pass():
187+
"""The #1141 regression: the failing step is one no phase covers."""
188+
verdict = gr.assess(
189+
NIGHTLY_NEVER_BOOTED_STATE, {"latest": "success"}, job="failure"
190+
)
191+
192+
assert verdict["result"] == gr.RESULT_FAILED, (
193+
"a red job whose report found nothing wrong must not print PASSED — the "
194+
"failure is real and lives outside every phase this report can see"
195+
)
196+
assert len(verdict["integrity"]) == 1
197+
assert "outside every phase" in verdict["integrity"][0]
198+
assert "Read the job log" in verdict["integrity"][0]
199+
200+
201+
def test_the_same_state_still_passes_when_the_job_is_green():
202+
"""Guards against the fix reddening healthy runs: only the job status differs."""
203+
for job in ("success", None):
204+
verdict = gr.assess(
205+
NIGHTLY_NEVER_BOOTED_STATE, {"latest": "success"}, job=job
206+
)
207+
assert verdict["result"] == gr.RESULT_PASSED, job
208+
assert verdict["integrity"] == []
209+
210+
211+
def test_a_cancelled_job_is_not_a_pass():
212+
verdict = gr.assess(
213+
NIGHTLY_NEVER_BOOTED_STATE, {"latest": "success"}, job="cancelled"
214+
)
215+
216+
assert verdict["result"] == gr.RESULT_FAILED
217+
assert "`cancelled`" in verdict["integrity"][0]
218+
219+
220+
def test_a_red_job_is_not_reported_twice_when_a_phase_already_owns_the_failure():
221+
"""The phase-level message attributes the failure; this one cannot. Don't add noise."""
222+
recorded = {"phases": {"nightly_api": {"steps": {"flow": {"status": "fail"}}}}}
223+
verdict = gr.assess(recorded, {"nightly_api": "failure"}, job="failure")
224+
225+
assert verdict["result"] == gr.RESULT_FAILED
226+
assert verdict["integrity"] == []
227+
assert len(verdict["failures"]) == 1
228+
229+
# Same for an unaccounted phase: run #115's job was red too.
230+
verdict = gr.assess(
231+
RUN_115_STATE,
232+
{"latest": "success", "nightly_api": "success", "nightly_ui": "failure"},
233+
job="failure",
234+
)
235+
assert len(verdict["integrity"]) == 1
236+
assert "nightly_ui" in verdict["integrity"][0]
237+
238+
239+
def test_job_status_is_parsed_from_the_env():
240+
assert gr.job_status({"JOB_STATUS": "Failure"}) == "failure" # GitHub casing varies
241+
assert gr.job_status({"JOB_STATUS": " success "}) == "success"
242+
# An unset or blank value must read as "unknown", never as a status.
243+
assert gr.job_status({}) is None
244+
assert gr.job_status({"JOB_STATUS": " "}) is None
245+
246+
247+
def test_the_rendered_report_states_both_verdicts():
248+
"""The pair that contradicted each other in the issue body, side by side."""
249+
body = gr.generate_report(
250+
NIGHTLY_NEVER_BOOTED_STATE, {"latest": "success"}, job="failure"
251+
)
252+
253+
assert "**Job status (runner):** `failure`" in body
254+
assert "## Result: FAILED" in body
255+
assert "## Unaccounted failures" in body
256+
257+
172258
def test_the_rendered_report_still_lists_every_step():
173259
body = gr.generate_report(RUN_115_STATE, {})
174260

0 commit comments

Comments
 (0)