Skip to content

bug(tests): lint baseline rebase race — #791 CI passed against pre-#783 dev, post-merge dev broken #802

Description

@AndriiPasternak31

Summary

tests/lint_sys_modules.py (introduced in #791) has a structural blind
spot: a PR's lint job can pass against pre-existing dev, then merge after
a sibling PR that violates the lint, leaving dev red with no signal to
either author. This is the failure mode that caused #606's PR to inherit
a CI failure it didn't introduce.

Concrete instance

Timeline (from gh pr view 791 --json):

Time (UTC) Event
2026-05-11 15:45 #791 created. CI runs: lint (sys.modules pollution check) ✅ PASS — against synthetic merge of #791 + dev-at-15:45
2026-05-11 16:39 #783 (fix(cleanup): force-fail orphan when agent unreachable during re-verify) merges to dev, adding tests/unit/test_cleanup_unreachable_orphan.py with 3 import-time sys.modules writes
2026-05-11 20:50 #791 merges to dev (4h after its CI). No re-run triggered. Baseline file from #791 lands without an entry for the new file
Post-merge python tests/lint_sys_modules.py on dev reports tests/unit/test_cleanup_unreachable_orphan.py: 0 → 3 (+3) and exits 1

Why the safeguards didn't fire:

  1. test_committed_baseline_matches_current_repo_state (added in same commit as the lint) correctly passed in fix(tests): conftest sys.modules baseline + tests-side lint (closes #762) #791's CI — at that point the local tree matched the local baseline. It would have failed if re-run after merge, but the workflow at .github/workflows/backend-unit-test.yml only triggers on pull_request, never on push to dev or main.
  2. The two PRs touched disjoint paths (tests/lint_sys_modules_baseline.txt vs tests/unit/test_cleanup_unreachable_orphan.py), so GitHub's merge had no content conflict to surface — the silent semantic conflict was invisible to git.
  3. There's no "require branches to be up to date before merging" branch-protection rule on dev (verified empirically — fix(tests): conftest sys.modules baseline + tests-side lint (closes #762) #791 merged 4h after its last CI run).

Why this matters

This isn't unique to the lint job. Any baseline-style invariant test (snapshot tests, license inventories, schema drift checks, fixture audits) has the same failure mode: passing CI proves the PR is consistent with its base-at-CI-run, not with the actual merge state.

#606's PR was the immediate victim — inherited a red CI from a state it didn't author. Worked around by extending the escape hatch onto test_cleanup_unreachable_orphan.py in commit ea3a1291 (PR for #606). But the next contributor branching off dev will hit the same pattern with the next baseline-introducing PR.

Options

A) Enable "Require branches to be up to date before merging" on dev (and main). One-click branch-protection setting. Forces a CI re-run when the base has moved since last green. Highest impact for lowest effort. Adds latency to PR landing (re-run the unit suite per rebase), so worth measuring against velocity preference.

B) Add a push trigger to backend-unit-test.yml. Run the lint + unit suite on every push to dev and main. Catches the stale state post-merge instead of preventing it. Creates a signal but doesn't block.

C) Make the lint baseline-stale check a separate, faster job. Run python tests/lint_sys_modules.py --regenerate-baseline and diff against committed; fail if non-empty. Gives a clean, fast pre-merge signal — but still needs (A) or (B) to enforce.

Recommendation

(A) is the right primary fix — covers this lint, every other baseline-style check we add later, and is the default GitHub-recommended rule for PR workflows of this size. (B) as a backup safety net is also worth doing — gives a fast post-merge alarm even if (A) is bypassed (admin merges, force-pushes, branch-protection misconfigurations).

Acceptance criteria

  • dev and main have "Require branches to be up to date before merging" enabled.
  • backend-unit-test.yml either gains a push trigger for dev/main, OR the lint-sys-modules job is documented as relying solely on (A).
  • Document the chosen rule in DEVELOPMENT_WORKFLOW.md so contributors know rebase-then-merge is required for branches with baseline-style assertions.

Refs

Activity

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Type

No type

Projects

No projects

    Milestone

    No milestone

    Relationships

    None yet

    Development

    No branches or pull requests

    Issue actions