Path filters plus require_checks can make a docs-only MR unmergeable #172

closed cmc opened this on 2026-09-05 06:27 UTC · milestone v1.14.0

Discussion

cmc 2026-09-05 06:27 UTC

The check gate (internal/control/mr.go:808-829) requires that the head carry statuses and that all of them are green. It does not check for a specific set of job names — an absence of statuses is refused with "requires green checks and none were reported".

Path filters (#169) can produce exactly that absence. A repository where every job carries a paths or paths-ignore rule that excludes the change queues no builds, sets no ci/<job> statuses, and the merge request then cannot be merged at all. A docs-only change is the obvious case, which is unfortunate given docs changes are what path filters were added for.

krz/gitbay is safe by construction — build carries no paths-ignore, so a status is always reported — and require_checks is off here anyway. Neither is something another deployment can be assumed to have arranged.

Options, roughly in order of preference:

  • Treat a job that a filter excluded as satisfied rather than absent: record a ci/<job> status of success with a message naming the filter. Honest, and it keeps CombinedStatus meaningful.
  • Let the gate pass when no job was applicable, distinguishing "filtered out" from "never ran". Needs the gate to know why nothing is there.
  • Document it and leave it, requiring operators to keep one unconditional job. Cheapest, and the least likely to be read in time.

Ref #169.

referenced in commit 9a2a62e575 by cmc: ci: derive a diff base from the merge base on a branch's first push

2026-09-05 07:00 UTC
cmc 2026-09-05 18:40 UTC

Decision, so this is unambiguous when someone picks it up.

Take the first option: a job a filter excluded records a ci/<job> status rather than nothing at all.

The state is skipped, not success. CommitStatus.State is documented as pending | success | failure | error (internal/store/statuses.go:10) but the column is TEXT, so no migration is needed, and CombinedStatus (statuses.go:97-111) already returns failure only for error/failure, pending for pending, and success for everything else. A skipped status therefore satisfies the check gate without touching the reduction rule.

Recording success would also work and is a smaller change, and it is the wrong one: ci/test = success on a commit whose tests never ran is a lie the dashboard repeats forever. skipped says what happened.

Three things that come with it:

  • The description should name the reason, e.g. no files matched paths, so the status explains itself without going to the build list.
  • There is no build behind a skipped status, so ChecksForCommit (statuses.go:122) must tolerate Build == 0 for a ci/ context. Check what it does today before assuming it does.
  • The web checks list and any status badge need to render the new state. It must not fall through to a colour that reads as failure.

Update State's doc comment on CommitStatus when adding it.

cmc 2026-09-05 20:04 UTC

Correction to the decision above: "the column is TEXT, so no migration is needed" was wrong. commit_statuses.state carries CHECK (state IN ('pending','success','failure','error')) from migration 0008, so skipped violates it.

The failure mode is the bad part. queueJobs discards SetCommitStatus's error (internal/control/build.go:636,646, matching the surrounding style), so the write would have failed the constraint silently: the status never lands, the merge request still reports no checks, and the fix looks like it works while changing nothing. That is exactly the bug this issue is about, reintroduced by its own fix.

Migration 0041 widens the constraint. SQLite cannot alter a CHECK in place, so it is a table rebuild — rename, recreate with the widened constraint, copy, drop, recreate the index. Verified before accepting: commit_statuses_sha is the only index on the table and is recreated in both directions, nothing has a foreign key pointing at commit_statuses so the rename orphans no references, and creator_id is ON DELETE SET NULL so no copied row can fail a foreign key. The down migration drops skipped rows, which is the only sane reverse.

Worth remembering generally: a state column here may be constrained, and a constraint violation in a path that discards its error is invisible.

closed by commit 2f49d2d95e by cmc: ci: filtered jobs record a skipped status instead of nothing

2026-09-05 20:32 UTC