bug(ci): both nightlies post a false ✅ "clean" to PRs whose suite never ran

[obasilakis]

Summary

Both nightly sweeps post a ✅ clean sticky comment to a PR whose suite never ran. When the test job fails before a verdict is written, Write status JSON still runs (if: always()), the unset output stringifies to false, and the comment script's final else branch renders the green tick.

This is the #1941 defect one job downstream. #1941 was "a non-zero exit is not proof of a content conflict"; this is "a job that failed is not proof that nothing regressed." Both come from a verdict being inferred from an absent signal rather than from evidence.

The direction matters: #1941 produced a false alarm (annoying, self-limiting — someone eventually investigates). This produces a false all-clear on a regression detector, which nothing corrects.

Affected

Workflow	State	Severity
backend-unit-nightly.yml	on dev today	latent — reachable when the checkout or PR-head fetch fails
backend-unit-nightly.yml (PR #1952)	in review	adds a deliberate exit 1 branch that routes into it
integration-nightly.yml (PR #1963)	in review	same, and creates a new comment rather than only updating one

#1963 is the worst case: the unit nightly guards creation with } else if (status.regression || status.merge_conflict), so a false clean can only overwrite an existing sticky. The integration nightly's } else { calls createComment unconditionally, and it is a new workflow where no PR has a sticky yet — so the first bad night posts a fresh green tick to up to 20 PRs.

Mechanism

- name: Write status JSON
  if: always()                                    # runs even when the job failed
  run: |
    merge_conflict='${{ steps.merge.outputs.merge_conflict }}'   # unset -> ''
    regression='${{ steps.diff.outputs.regression }}'            # unset -> ''
    if [ -z "$regression" ]; then regression="false"; fi
    jq -n ... '{merge_conflict: ($merge_conflict == "true"),     # '' == "true" -> false
                regression:     ($regression == "true")}'        # false

{merge_conflict: false, regression: false} is indistinguishable from a genuine clean run, so the comment job renders:

✅ Nightly unit-suite clean when this PR is merged into dev

The comment job is if: always() with needs: [discover, test] and never inspects needs.test.result, so a failed test job is invisible to it.

Reachable paths: base checkout fails · PR-head fetch fails · the merge fails for a non-conflict reason (#1952's new branch) · the stack never boots or writes no JUnit (#1963's :274-279). #1963's :271-273 explicitly claims the comment step "posts nothing for this PR rather than something false" — it posts something false.

Suggested fix

Absence of a verdict is its own state and must not collapse into "clean". Refuse to write the status file:

- name: Write status JSON
  if: always()
  run: |
    merge_conflict='${{ steps.merge.outputs.merge_conflict }}'
    if [ -z "$merge_conflict" ]; then
      echo "::warning::merge verdict unknown — no status written, sticky left untouched"
      exit 0
    fi

Both comment jobs enumerate status-pr*.json, so a missing file means that PR is skipped and any existing sticky is left alone. Uploads are already if-no-files-found: warn.

Applies to the regression side too: regression defaulting to false is correct only when the merge conflicted (the diff legitimately never ran) — not when the diff step was skipped because the job died.

A stronger belt, if wanted: have the comment job consult needs.test.result and refuse to render a clean verdict when the matrix did not succeed.

Acceptance criteria

• A test job that fails before producing a verdict results in no sticky comment change for that PR — neither created nor updated
• Verified for each reachable path: checkout failure, PR-head fetch failure, non-conflict merge failure, missing JUnit XML
• regression=false is only written when a diff actually ran, or when the merge legitimately conflicted
• A guard test covers the unknown-verdict path in each workflow — the existing suites assert the happy verdicts and have no coverage of the absent one
• Applied to backend-unit-nightly.yml and integration-nightly.yml (coordinate with fix(ci): give the nightly's merge check a common ancestor (#1941) #1952 / ci: give the live-instance integration suite a nightly CI home (#1896) #1963 to avoid conflicting edits)

Related

• bug(ci): nightly unit-suite reports a false merge conflict on every PR — shallow fetch on both sides leaves no common ancestor #1941 — the same inference error one job upstream (if ! git merge collapsing every failure into "conflict"); its docs/memory/learnings.md entry generalises the rule this violates: "anything else must fail the job loudly rather than post a confident wrong answer." The job does fail loudly. The comment job does not hear about it.
• test(ci): frontend-e2e only runs on ui-labeled PRs — specs rot silently and the suite sits red on dev #1526 — frontend-e2e rotting because its signal was never trusted. Same family: a check whose output is decoupled from its input.
• fix(ci): give the nightly's merge check a common ancestor (#1941) #1952, ci: give the live-instance integration suite a nightly CI home (#1896) #1963 — the two PRs in review; both carry this and both are one small edit from not carrying it.

[dolho]

/claim

[github-actions]

Claimed by @dolho. Label status-in-progress added.

Next steps:

1. Create branch: feature/2029-<short-slug>
2. Implement the feature
3. Open a PR with Fixes #2029

[dolho]

Handled, in the two PRs that own the files rather than a third one.

integration-nightly.yml does not exist on dev — it arrives with #1963 — and backend-unit-nightly.yml is already modified by #1952. A separate PR could not have touched the first and would have conflicted with the second, so the fix went where the AC asked: #1952 (2942102b) and #1963 (3405002f).

The fix. Write status JSON refuses to write when the merge verdict is unknown, and regression only defaults to false when the merge actually conflicted — the one case where the diff legitimately never ran. Both comment jobs enumerate the status files that exist, so a missing one means that PR is skipped and its sticky is left exactly as it was.

I did not apply the coarser belt, and that is a decision rather than an omission. Gating the comment step on needs.test.result == 'success' looked right until I checked what it does on a matrix job: the result is failure when any single leg fails, so one PR's infrastructure hiccup would suppress the report for every other PR in the sweep. That trades a false green for a silence that is equally wrong and hits PRs that were fine. The per-PR guard is precise and complete on its own — a leg with no verdict writes no file, and the all-legs-failed case falls out of the same mechanism (no files, no comments). A test pins the decision so the next reader sees it was considered.

One thing worth recording: the issue notes that #1963's comment claims the step "posts nothing for this PR rather than something false". That was my comment, and it was untrue when I wrote it. I corrected it in place rather than deleting it — a comment asserting a property the code does not have is more useful as a recorded mistake than as a tidy line.

7 guard tests across the two workflows (4 + 3), every one mutation-verified: removing the unknown-verdict guard, defaulting regression blindly, dropping if: always(), and adding the matrix gate each fail exactly their own test. The backend assertions run through that file's existing _commands() helper, which strips comments — this fix's own explanation contains the strings being asserted, which is the trap that helper exists for.

Closes when both PRs land; leaving this open until then.