Skip to content

docs: require contract change routing#2960

Open
franksong2702 wants to merge 2 commits into
nesquena:masterfrom
franksong2702:franksong2702/fix-2959-contract-review-gate
Open

docs: require contract change routing#2960
franksong2702 wants to merge 2 commits into
nesquena:masterfrom
franksong2702:franksong2702/fix-2959-contract-review-gate

Conversation

@franksong2702
Copy link
Copy Markdown
Contributor

Thinking Path

  • Hermes WebUI uses contributor-facing contracts to keep product, runtime, and review expectations stable across many small PRs.
  • A PR can still change product semantics by editing behavior or contract tests without explicitly saying it is changing a contract.
  • Green CI is not enough when a PR changes the rule that future CI will enforce.
  • This PR makes contract-affecting changes explicit in contributor guidance and pins the new process rule with a small static test.

Fixes #2959.

Contract Routing

Task type: contributor-process guardrail
Touched areas:

  • CONTRIBUTING.md
  • docs/CONTRACTS.md
  • tests/test_contract_review_gate.py
  • CHANGELOG.md
    Relevant public docs:
  • AGENTS.md
  • CONTRIBUTING.md
  • docs/CONTRACTS.md
    Scope boundaries:
  • Adds review guidance and static coverage only.
  • Does not add a GitHub Actions policy engine or PR template.
    Evidence needed before claiming done:
  • Static regression test proves the guidance names Contract Routing, Contract Change, contract tests, corresponding docs, and release-batch callouts.

What Changed

  • Added a Contract-affecting PRs section to CONTRIBUTING.md.
  • Added a Contract changes section to docs/CONTRACTS.md.
  • Updated the PR preparation checklist so contract-affecting PRs require Contract Routing, and intentional contract changes require Contract Change.
  • Added tests/test_contract_review_gate.py to pin the process terms.
  • Added an Unreleased changelog entry.

Why It Matters

Contracts should prevent accidental product-direction changes, not just document them after the fact. When a PR changes product semantics or the tests that encode them, reviewers need an explicit signal that the PR is changing a contract rather than merely fixing a local bug.

Verification

  • RED before docs update:
    • python3 -m pytest tests/test_contract_review_gate.py -q -> failed on missing Contract Routing, Contract Change, contract tests, corresponding-docs language, and release-batch callout.
  • GREEN after docs update:
    • python3 -m pytest tests/test_contract_review_gate.py tests/test_canonical_session_resolution_rfc.py -q -> 4 passed
    • git diff --check

Risks / Follow-ups

  • This is guidance plus static coverage, not an automated GitHub policy gate. A stricter follow-up could add a PR-template or CI check if maintainers want enforcement beyond docs/tests.
  • The rule is intentionally narrow: it targets contract-affecting PRs, not every small bugfix.

Model Used

OpenAI GPT-5 via Codex app agent. Tool use: local repo inspection, GitHub CLI, pytest, git worktree, and git push/PR creation.

@nesquena-hermes
Copy link
Copy Markdown
Collaborator

Read the diff against CONTRIBUTING.md, docs/CONTRACTS.md, the new tests/test_contract_review_gate.py, and the linked issue #2959. The intent — making contract changes visible in the PR body rather than smuggled inside test edits — is reasonable. A few things worth thinking about before this lands.

The static test only checks documentation strings, not behavior

def test_contributing_requires_contract_routing_for_contract_affecting_prs():
    text = CONTRIBUTING.read_text(encoding="utf-8")
    required_terms = [
        "contract-affecting PR",
        "Contract Routing",
        "Contract Change",
        "release batch",
    ]
    missing = [term for term in required_terms if term not in text]
    assert missing == []

This pins specific copy in CONTRIBUTING.md. It will pass forever as long as those four substrings are present somewhere in the file, even if surrounding context is later deleted, contradicted, or moved to a footnote saying "obsolete." A rewording from "release batch" to "release batches" or "release-batch callouts" would actually break this test even though the new text says the same thing.

That's the well-known shape of a static-content regression test, and it's not new in this repo — but for a process contract, this is weaker than the equivalent test for a runtime contract. A runtime contract test fails when the behavior changes. This test fails only when the literal string "Contract Routing" disappears. The actual review-gate behavior — does a PR that changes a contract include this section? — isn't checked by anything here, including CI.

That gap is fine if the PR description is honest that this is advisory coverage, not enforcement. The current framing in docs/CONTRACTS.md ("Contract tests and corresponding docs must move together") reads more like an enforced rule than what the test actually pins. Worth making the gap explicit in the docs so future contributors don't think CI is catching contract violations when it isn't.

"Tests must move with docs" is hard to enforce without tooling

The new section in docs/CONTRACTS.md:114-118 says:

Contract tests and corresponding docs must move together. Tests that encode product semantics must not silently redefine the contract by asserting the opposite behavior without updating the public docs and naming the change in the PR body.

In practice, the way a PR "silently redefines a contract" is by editing an assertion (e.g. assert "compact_mode_hides_interim" in ...assert "compact_mode_preserves_interim" in ...). There is no automated way to detect that without either:

  • a pre-commit hook that warns when files matching tests/test_*contract*.py or tests/test_*regression*.py are modified without a corresponding docs/ or CONTRIBUTING.md change,
  • or a GitHub Actions check that does the same on PR diff.

The PR explicitly excludes both from scope, which is fine, but it means the rule lives entirely as social contract. That's an honest trade-off given the small scope; just worth being clear that without enforcement, this is a norm dressed as a contract. The next contributor to invert a test will not be stopped by any check — only by a reviewer noticing.

A cheap step that would actually catch the failure mode: a pre_release_check.sh (or release.py) augmentation that diffs tests/test_*regression*.py and tests/test_*contract*.py against the previous tag and flags PRs in the range whose body does not contain Contract Routing. That's still not enforcement, but it surfaces the signal at release-batch time, which is exactly where #2959 says it's currently missing.

Smaller things

  • CONTRIBUTING.md:23-34 introduces "contract-affecting PR" as a defined term but doesn't link to the matching section in docs/CONTRACTS.md. Cross-linking would help contributors who land on one document but not the other.
  • The new Contract changes section in docs/CONTRACTS.md:105-122 lives between "PR routing" and "PR preparation checklist". Reasonable placement, but the checklist update at line 141-142 references Contract Routing / Contract Change without re-stating where those sections come from — fine since the doc is short, but a section anchor link would help.
  • The CHANGELOG entry says "Contributor guidance now requires..." which is accurate. Worth confirming the release-notes wording at the next batch doesn't soften this to "encourages" — if it's a requirement, the wording should match what the doc says.

Bigger question

Issue #2959 frames the problem as "CI turns green while product direction silently shifts." This PR's response is "add prose in two docs and a test that pins the prose." That doesn't actually close the failure mode the issue describes — it just makes the rule citeable. If the goal is closing the gap, this should probably be labeled "phase 1: write down the rule" with a follow-up tracked for "phase 2: enforce at release time." If the goal is just documenting the norm, then the issue's framing is over-promising what this PR will deliver, and the issue should be updated to match.

That's a maintainer call, not a blocker on the diff. Diff itself is small, safe, and well-tested for what it pins.

@franksong2702
Copy link
Copy Markdown
Contributor Author

Follow-up pushed in fb2b1ebd for the review notes.

What changed:

  • docs/CONTRACTS.md now explicitly says the static tests are advisory coverage, not an automated GitHub policy gate, and that they do not enforce PR-body content.
  • CONTRIBUTING.md now links directly to the Contract Routing and Contract changes sections in docs/CONTRACTS.md.
  • CHANGELOG.md now reflects that this PR documents a contributor guidance/static-coverage rule rather than shipping enforcement.
  • Added a regression test that pins the advisory/not-enforcement wording so future wording does not overstate what CI catches.

Verification:

  • pytest tests/test_contract_review_gate.py tests/test_canonical_session_resolution_rfc.py -q -> 5 passed
  • git diff --check
  • GitHub Actions is green on Python 3.11, 3.12, and 3.13.

Scope boundary:

  • This remains phase 1: making the rule explicit and citeable. A release-time or CI enforcement check would be a separate follow-up.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Process gap: contract-changing PRs need an explicit review gate

2 participants