docs(ci): document mypy as hard gate; clarify CI gate policy#31
Conversation
…IBUTING Follow-up to PR #30 review feedback from @copilot-pull-request-reviewer: the previous workflow had 'mypy app/ || true' (warn-only), and #30 silently removed the '|| true'. The change wasn't called out in #30's description. This PR makes the rationale explicit: - ci.yml: rename the step 'Mypy (strict, blocking)' and add a comment next to it explaining the choice (strict = true is configured in pyproject.toml; warn-only would mask regressions). - CONTRIBUTING.md 'Local CI checks': clarify that ruff/mypy/pytest are ALL hard gates in CI. Document the strict-mode mypy + 80% coverage floor. - CONTRIBUTING.md 'Code review': add two new bullets: 1. Wait for AI reviewers even on small PRs — they take ~1-2 minutes. Merging with --admin before they comment forfeits the review value. 2. Side-effects must be in the PR description; reviewers shouldn't discover changes by line-by-line diff reading. The mypy-as-hard-gate decision stands — pyproject.toml already declares strict mode, having CI accept type errors silently is inconsistent. Refs PR #30 review comment.
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request serves as a documentation update to formalize the project's CI gate policies and improve the quality of the code review process. By explicitly documenting the strict requirements for CI checks and establishing clearer expectations for PR submissions, the changes aim to prevent future oversights regarding type-safety and review transparency. Highlights
Ignored Files
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
There was a problem hiding this comment.
Pull request overview
This PR makes the CI policy around type-checking and other gates explicit by documenting that mypy is intended to be blocking (strict) and by updating contributor guidance to avoid “silent” CI behavior changes.
Changes:
- Updated
.github/workflows/ci.ymlto label the mypy step as strict + blocking and document the rationale in-line. - Updated
CONTRIBUTING.mdto clarify that local pre-PR checks mirror CI and that ruff/mypy/pytest are intended to be hard gates. - Added code review workflow rules to ensure side-effects are described in PR descriptions and AI reviews are not skipped.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| CONTRIBUTING.md | Clarifies CI gate expectations and adds review workflow rules; mentions strict mypy + coverage floor. |
| .github/workflows/ci.yml | Renames and comments the mypy step to explicitly document it as strict/blocking. |
| The CI workflow runs the same. **All four are hard gates** — no warn-only: | ||
|
|
||
| - `ruff check` and `ruff format --check` reject style/lint issues | ||
| - `mypy` runs in strict mode (configured in `pyproject.toml` `[tool.mypy] strict = true`); type-check failures fail the build | ||
| - `pytest` requires all tests pass; coverage floor is 80% (configured in `pyproject.toml`) |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request updates the CONTRIBUTING.md file to establish new protocols for AI-assisted reviews and clarifies that CI checks for linting, strict type-checking, and 80% test coverage are mandatory gates. Feedback includes a request to include missing ci.yml changes mentioned in the PR description and a suggestion to update the local pytest command to include coverage flags to align with the project's requirements.
|
|
||
| **Wait for the AI reviewers, even on small PRs.** Gemini and Copilot post within ~1-2 minutes. Merging a small "trivial" PR with `--admin` before they've commented forfeits the review value and makes follow-up comments awkward to address. If a PR is *truly* time-critical (security fix, broken main), call it out explicitly in the PR description and own that you're skipping the review window — and address any post-merge comments in a follow-up PR. | ||
|
|
||
| **Side-effects must be in the PR description.** If a PR primarily fixes A but also changes B, list both in the description. Reviewers shouldn't have to discover changes by reading the diff line by line. |
There was a problem hiding this comment.
The PR description mentions changes to ci.yml (renaming the mypy step and adding a rationale comment), but these changes are not present in the current diff. Given the new rule added in this line regarding documenting side-effects, please ensure the ci.yml modifications are included in the PR to maintain consistency.
| ruff check . # linting (blocking in CI) | ||
| ruff format --check . # formatting (blocking in CI) | ||
| mypy app/ # type checking (blocking in CI — strict mode) | ||
| pytest # tests (blocking in CI) |
There was a problem hiding this comment.
To align the local check with the CI 'hard gate' for coverage (80% floor), the command should include the coverage flag. Running pytest alone will not trigger the coverage enforcement defined in pyproject.toml.
| pytest # tests (blocking in CI) | |
| pytest --cov # tests + coverage (blocking in CI) |
References
- Behaviour changes without tests get pushed back. CI enforces an 80% coverage floor. (link)
## 0.1.0 (2026-05-10) * docs: refactor — ADRs in docs/decisions/, policies in docs/policies/, slim CLAUDE.md ([51c2cf1](51c2cf1)), closes [#1](#1) * docs(ci): document mypy as hard gate; clarify CI gate policy in CONTRIBUTING (#31) ([e1e6f18](e1e6f18)), closes [#31](#31) [#30](#30) [#30](#30) [#30](#30) * docs(decisions): ADR 0006 — PT vs QL ESC i S behaviour from Phase-0 hardware test ([0d12c63](0d12c63)), closes [#12](#12) [#11](#11) * docs(decisions): ADR 0012 — layout management; clarify integration push/pull capabilities ([effcbdf](effcbdf)), closes [#17](#17) [#19](#19) * docs(decisions): ADR 0013 + cart UI spec + AI-review workflow ([c72dc85](c72dc85)), closes [#26](#26) [#27](#27) [#28](#28) * docs(docker): document image tag scheme (latest, 1.0.0, 1.0, 1) ([1f72396](1f72396)) * docs(examples): sample compose files for standalone/Traefik/Pangolin/Caddy ([a2f6f3d](a2f6f3d)) * docs(learnings): add code-review-patterns + reference from CLAUDE/CONTRIBUTING/AI configs (#33) ([0fc61d2](0fc61d2)), closes [#33](#33) [#29](#29) [#30](#30) [#32](#32) [#6](#6) * ci: make Python lint/test job branch-tolerant for dependabot PRs (#30) ([6ee8e16](6ee8e16)), closes [#30](#30) [2-#9](https://github.com/2-/issues/9) [#2](#2) [#3](#3) [#4](#4) [#6](#6) [#7](#7) [#8](#8) [#9](#9) * ci(deps): bump the actions-all group across 1 directory with 14 updates (#9) ([a88a027](a88a027)), closes [#9](#9) * ci(release): trigger releases via cron + workflow_dispatch only (#32) ([9941958](9941958)), closes [#32](#32) [#32](#32) [#32](#32) * chore(deps-dev): bump @commitlint/cli from 19.8.1 to 21.0.0 (#3) ([2642b26](2642b26)), closes [#3](#3) * chore(deps-dev): bump @commitlint/config-conventional (#4) ([44be5f9](44be5f9)), closes [#4](#4) * chore(deps-dev): bump @semantic-release/exec from 6.0.3 to 7.1.0 (#2) ([a8097b0](a8097b0)), closes [#2](#2) * chore(deps-dev): bump @semantic-release/github from 11.0.6 to 12.0.8 (#8) ([098e766](098e766)), closes [#8](#8) * chore(deps-dev): bump conventional-changelog-conventionalcommits (#7) ([9f5370f](9f5370f)), closes [#7](#7) * chore(deps-dev): bump semantic-release from 24.2.9 to 25.0.3 (#6) ([306010f](306010f)), closes [#6](#6) * chore(release): 1.0.0 ([15abeb3](15abeb3)) * feat(status): add Brother 32-byte status block parser (#29) ([ced0ff8](ced0ff8)), closes [#29](#29) [#11](#11) [#19](#19) [#29](#29) [skip ci]
Summary
Follow-up to PR #30. @copilot-pull-request-reviewer pointed out that #30 silently removed `|| true` from the mypy step (turning it from warn-only into a hard gate) without calling that out in the PR description. They were right — that should have been part of the original PR.
This PR makes the decision explicit and tightens the CONTRIBUTING workflow so the same kind of reviewer-skip doesn't happen again.
Changes
`ci.yml` (mypy step)
`CONTRIBUTING.md` (Local CI checks)
`CONTRIBUTING.md` (Code review section §7)
Two new rules learned from this incident:
Linked issue
Refs the review comment on #30
Type of change
Hardware tested on
Test coverage
Checklist