|
| 1 | +--- |
| 2 | +name: invforge-review |
| 3 | +description: Mandatory pre-push review for InvForge (Python). Runs ruff/mypy/pytest gates and reviews changed files against docs/coding-standards.md. Must be run and must pass before any `git push` in this repo. |
| 4 | +--- |
| 5 | + |
| 6 | +# InvForge pre-push review |
| 7 | + |
| 8 | +This is a **mandatory gate**, not an optional lint pass. No commit in this |
| 9 | +repo gets pushed without this skill having been run against it and having |
| 10 | +reported a clean result. If you are about to `git push` in this repo and |
| 11 | +haven't run this skill against the commits being pushed, run it first. |
| 12 | + |
| 13 | +## 1. Scope the review |
| 14 | + |
| 15 | +Determine what's changed and being pushed: |
| 16 | + |
| 17 | +```sh |
| 18 | +git status --short |
| 19 | +git diff --stat $(git merge-base HEAD origin/main 2>/dev/null || echo HEAD)..HEAD |
| 20 | +``` |
| 21 | + |
| 22 | +If there's no `origin/main` yet (new repo, nothing pushed), review |
| 23 | +everything staged/committed so far instead. |
| 24 | + |
| 25 | +## 2. Mechanical gates — all must pass with zero errors/warnings |
| 26 | + |
| 27 | +Run from the repo root, using the project venv: |
| 28 | + |
| 29 | +```sh |
| 30 | +.venv/bin/ruff format --check invforge tests |
| 31 | +.venv/bin/ruff check invforge tests |
| 32 | +.venv/bin/mypy |
| 33 | +.venv/bin/python -m pytest tests/unit -q |
| 34 | +``` |
| 35 | + |
| 36 | +`tests/unit/` is fast and network-free — always run it. If the change |
| 37 | +under review touches anything in `invforge/core/connectivity.py`, |
| 38 | +`invforge/core/server.py`, `invforge/core/control_api.py`, a scenario |
| 39 | +fixture, or the Dockerfile/compose file, also run the integration suite |
| 40 | +(needs Docker): |
| 41 | + |
| 42 | +```sh |
| 43 | +scripts/integration-test.sh |
| 44 | +``` |
| 45 | + |
| 46 | +Any failure here is an automatic FAIL — fix it (or, for `ruff format`, |
| 47 | +just run `ruff format invforge tests` and re-check) before doing the |
| 48 | +semantic review below. Don't hand-wave a mypy or ruff error as |
| 49 | +"pre-existing" without confirming it actually predates the change under |
| 50 | +review — the whole codebase is expected to be clean at all times, see |
| 51 | +`docs/coding-standards.md`. |
| 52 | + |
| 53 | +## 3. Semantic review |
| 54 | + |
| 55 | +Read `docs/coding-standards.md` in full, then walk every changed file |
| 56 | +against it. In particular: |
| 57 | + |
| 58 | +- **Correctness against real fixtures**, not just synthetic ones — does |
| 59 | + the change still work against `scenarios/recorded/*.yaml`, not only |
| 60 | + invented scenarios? |
| 61 | +- **Boundary conditions**: empty scenario, register at a unit's block |
| 62 | + edge, malformed/partial YAML, an HTTP request with a missing/wrong-typed |
| 63 | + field. |
| 64 | +- **Resource cleanup** on every exit path — threads, sockets, the |
| 65 | + ticker's stop_event. |
| 66 | +- **Type-hint accuracy** — does the annotation actually match runtime |
| 67 | + behavior, or did mypy only pass because of an `Any` escape hatch |
| 68 | + (`# type: ignore`, unannotated `object`, etc.) that hides a real gap? |
| 69 | + Every `# type: ignore` in this codebase should have a comment |
| 70 | + explaining why (see `core/server.py`'s pymodbus-subclassing comment as |
| 71 | + the reference example) — a bare, unexplained one is itself a finding. |
| 72 | +- **Test coverage** for the behavior actually changed — a new code path |
| 73 | + with no corresponding test is a finding, not just a suggestion. |
| 74 | +- **Consistency with `invforge/core/`'s conventions** — did a |
| 75 | + vendor-specific quirk leak into `core/` instead of staying inside its |
| 76 | + `invforge/profiles/<vendor>/`? That's an architecture violation, not a |
| 77 | + style nit. |
| 78 | +- **New dependency scrutiny** — anything added to `requirements.txt` |
| 79 | + that the stdlib or an existing dependency already covers is a finding. |
| 80 | + |
| 81 | +## 4. Report |
| 82 | + |
| 83 | +Use the `ReportFindings` tool with the verified findings, most severe |
| 84 | +first. If nothing survives review, call it with an empty findings list — |
| 85 | +don't skip the call. |
| 86 | + |
| 87 | +## 5. Verdict |
| 88 | + |
| 89 | +State explicitly, in plain text after the findings: |
| 90 | + |
| 91 | +- **PASS — safe to push** if all mechanical gates passed and no |
| 92 | + CONFIRMED correctness finding remains, or |
| 93 | +- **FAIL — do not push** otherwise, naming exactly what has to change |
| 94 | + first. |
| 95 | + |
| 96 | +Do not push on a FAIL verdict, even if the user seems to be in a hurry — |
| 97 | +say so and let them explicitly override if they choose to. |
0 commit comments