diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index 4269b081..f506fbe3 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -44,6 +44,34 @@ An issue, roadmap row, or helper-queue result is a lead. It is not sufficient evidence by itself. If the work has landed, is already claimed, or no longer matches the record, the agent reconciles that state instead of duplicating it. +## Landing a pull request + +Squash-merge it: `gh pr merge --squash`. The merge method is not cosmetic, +because two gates read the commits a push actually lands on `main`, and both +fail *after* the merge, on `main`, not on your PR. + +`scripts/check-role-discipline.py` inspects every commit in the pushed range and +requires each to name either its `row/` branch or its PR as `(#N)` +(`POL-PR-REQUIRED`). A squash-merge writes `(#N)` into the subject for free. A +`--merge` landing does not: the content commit keeps the subject it had on the +branch, which names neither, so the gate reports `reached main without a +reviewed row/* PR` even though a reviewed row PR is exactly where it came from. + +`scripts/check-commit-trailers.py` reads the same range, so the landed message +must itself carry the `FOLLOWING_AGENTS_PROTOCOL` paragraph and the trailers +(`POL-COMMIT-TRAILERS`, `POL-AI-ATTRIBUTION`). Squash bodies are built from the +branch's commit messages, so a correctly trailered commit carries them through; +confirm the composed message in the merge dialog before confirming. A `--merge` +landing fails this too, because GitHub's generated `Merge pull request #N +from ...` message has no marker and no trailers. + +Neither failure is repairable afterwards. Both gates are scoped over +`github.event.before..github.sha`, and each run's `before` is the previous run's +`sha`, so no later run re-covers a range that already went red. The remedies are +rewriting published history or an explicit waiver; getting the merge method +right is much cheaper. A red `main` from this cause does not block your next PR, +which is checked against its own base, but it does hide real regressions. + ## Reference and hardware guardrails Documentation and hardware-independent work can start without a vLLM checkout diff --git a/tests/scripts/test_agent_role.py b/tests/scripts/test_agent_role.py index be64ceb1..913bae30 100644 --- a/tests/scripts/test_agent_role.py +++ b/tests/scripts/test_agent_role.py @@ -486,10 +486,38 @@ def test_exact_pending_pr_range_is_reportable_in_any_checkout(self) -> None: sys.argv = saved def test_landed_detached_commit_remains_strict_without_pending_evidence(self) -> None: + # The subject here is main()'s DECISION: a violation on a commit that + # has landed, with no --pending-pr-head evidence, is strict (1), not a + # REPORT (0). Feed it a fixed violation instead of relying on the real + # HEAD to be one. It did rely on that, and the coupling was live: under + # a pull_request event CI checks out the SYNTHETIC merge, whose + # merged_messages are the PR's own commit bodies, so any PR whose + # message cites an issue or PR number matched PR_REFERENCE, HEAD stopped + # being a violation, main() returned 0, and this test failed for a + # reason that had nothing to do with what it asserts. saved = sys.argv sys.argv = [saved[0], "--commit", "HEAD"] try: - with mock.patch.object(discipline, "has_reached_main", return_value=True): + with mock.patch.object(discipline, "has_reached_main", return_value=True), \ + mock.patch.object(discipline, "enforced", return_value=True), \ + mock.patch.object(discipline, "inspect", return_value=["x: landed without a row PR"]): + self.assertEqual(discipline.main(), 1) + finally: + sys.argv = saved + + def test_a_pr_number_in_the_body_does_not_decide_this_gate(self) -> None: + """Regression: the case that made the test above fail in CI. + + A commit message that merely MENTIONS `#123` must not change main()'s + landed-vs-pending decision. This pins the decision to the inputs it is + about, so a message quoting PR numbers cannot flip the outcome again. + """ + saved = sys.argv + sys.argv = [saved[0], "--commit", "HEAD"] + try: + with mock.patch.object(discipline, "has_reached_main", return_value=True), \ + mock.patch.object(discipline, "enforced", return_value=True), \ + mock.patch.object(discipline, "inspect", return_value=["x: see (#157) and #174"]): self.assertEqual(discipline.main(), 1) finally: sys.argv = saved