101 lines
5.0 KiB
Markdown
101 lines
5.0 KiB
Markdown
# Review Checklist
|
|
|
|
This checklist is **mandatory** for every PR. The automated `pr-review` CI
|
|
job checks items marked **[auto]**. The agent must verify all items
|
|
marked **[manual]** before posting an APPROVE review.
|
|
|
|
The `review_pr.py` script requires `--checklist-confirmed` for APPROVE
|
|
events. This flag attests that every category below has been reviewed.
|
|
|
|
---
|
|
|
|
## 1. Architecture Compliance [auto + manual]
|
|
|
|
- [ ] **No business logic in CLI** (`cli.py`): no `subprocess`, no
|
|
`os.system`, no `ansible-playbook` — delegate to `executor.py`
|
|
- [ ] **No hardcoded URLs or config values** that belong in `config.py`
|
|
with env var overrides
|
|
- [ ] **Layer boundaries respected**: CLI → runner_manager → executor →
|
|
subprocess/Ansible. No skipping layers.
|
|
- [ ] **Single Responsibility**: each module/function has one reason to
|
|
change. If a function does two things, split it.
|
|
- [ ] **No circular imports** introduced
|
|
|
|
## 2. Code Quality and Best Practices [auto + manual]
|
|
|
|
- [ ] **No `print()`** in `src/` — use `click.echo()` for user output
|
|
- [ ] **No bare `except:`** — catch specific exceptions
|
|
- [ ] **No broad `except Exception:`** without justification
|
|
- [ ] **No `TODO`/`FIXME`/`HACK`/`XXX`** left in merged code
|
|
- [ ] **No functions > 50 lines** (excluding docstrings and decorators)
|
|
- [ ] **No dead code** — unused imports, unreachable branches, commented-out code
|
|
- [ ] **No copy-paste duplication** — extract shared logic into a helper
|
|
- [ ] **Idiomatic Python** — use comprehensions, context managers, dataclasses
|
|
- [ ] **Type hints** on all public functions
|
|
|
|
## 3. Security [auto + manual]
|
|
|
|
- [ ] **No hardcoded secrets** (tokens, passwords, keys in string literals)
|
|
- [ ] **No `shell=True`** in subprocess calls — use argument lists
|
|
- [ ] **No `eval()` or `exec()`** — use `ast.literal_eval` if parsing literals
|
|
- [ ] **No secrets in logs or process arguments** — pass via env vars or files
|
|
- [ ] **Input validation** on all external inputs (CLI args, API responses, file contents)
|
|
- [ ] **No injection vectors** — parameterize subprocess args, SQL queries, etc.
|
|
|
|
## 4. Internationalization (i18n) [manual]
|
|
|
|
- [ ] **All user-facing strings wrapped in `_()`** — `click.echo(_("..."))`,
|
|
error messages, help text, prompts
|
|
- [ ] **No raw English strings** in `click.echo()`, `click.ClickException()`,
|
|
or `raise` messages visible to users
|
|
- [ ] **String interpolation uses named placeholders**: `_("Hello {name}", name=x)`
|
|
not `f"Hello {x}"` for translatable strings
|
|
|
|
## 5. Testability and Test Coverage [auto + manual]
|
|
|
|
- [ ] **Source file changes include corresponding test updates**
|
|
- [ ] **100% coverage maintained** (enforced by `pytest-cov`)
|
|
- [ ] **Tests are fast** (< 10 seconds total, enforced by `check_test_speed.py`)
|
|
- [ ] **Edge cases tested**: empty inputs, boundary values, error paths
|
|
- [ ] **No flaky tests** — no `sleep()`, no race conditions, no external dependencies
|
|
- [ ] **Test names describe the scenario**: `test_<condition>_<expected_result>`
|
|
|
|
## 6. Performance [manual]
|
|
|
|
- [ ] **No unnecessary allocations** in hot paths (list comprehensions vs generators)
|
|
- [ ] **Correct data structures** — O(1) lookups use `set`/`dict`, not `list`
|
|
- [ ] **No N+1 query patterns** in API calls or file I/O
|
|
- [ ] **No blocking I/O on hot paths** without justification
|
|
|
|
## 7. User Experience [manual]
|
|
|
|
- [ ] **Clear error messages** — tell the user what went wrong and how to fix it
|
|
- [ ] **Consistent CLI flag naming** — `--long-name` with `--short` aliases
|
|
- [ ] **Help text on all commands and options** — `--help` should be useful
|
|
- [ ] **No silent failures** — if something fails, the user should know
|
|
- [ ] **Output is actionable** — not just "Error" but "Error: X failed because Y. Try Z."
|
|
|
|
## 8. Documentation [auto + manual]
|
|
|
|
- [ ] **Source changes include doc updates** — README, wiki, AGENTS.md as needed
|
|
- [ ] **New functions/classes have docstrings** — Google style
|
|
- [ ] **Public API changes documented** in CHANGELOG (auto-generated by git-cliff)
|
|
- [ ] **AGENTS.md updated** if workflow, conventions, or processes changed
|
|
- [ ] **No stale documentation** — if code changed, docs must reflect it
|
|
|
|
## 9. Workflow Compliance [manual]
|
|
|
|
- [ ] **PR title matches Vikunja task title** (`GRM-N: <task title>`)
|
|
- [ ] **Commit messages follow conventional format** (`type: description`)
|
|
- [ ] **No force-push after review** — creates new commits and re-trigger CI
|
|
- [ ] **Branch is up to date** with master before merging
|
|
- [ ] **No merge commits** in the PR branch — use squash merge via auto-merge
|
|
|
|
## 10. Extensibility and Maintainability [manual]
|
|
|
|
- [ ] **Open/Closed Principle** — code is open for extension, closed for modification
|
|
- [ ] **No magic numbers** — constants are named and documented
|
|
- [ ] **Configuration over hardcoding** — use `config.py` with env var overrides
|
|
- [ ] **Future-proof error handling** — don't catch specific error messages that may change
|
|
- [ ] **Dependencies are justified** — no new dependency without rationale
|