131 lines
6.9 KiB
Markdown
131 lines
6.9 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
|
|
- [ ] **No `Any` type without justification** — document why if used
|
|
- [ ] **Error handling complete** — all failure paths handled, no silent failures
|
|
- [ ] **Cleanup in error paths** — files closed, connections released, temp files removed
|
|
|
|
## 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.
|
|
- [ ] **File paths validated** — no path traversal (use `Path.resolve()`, check boundaries)
|
|
|
|
## 4. Internationalization (i18n) [auto + 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, None/Optional
|
|
- [ ] **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 — use generators for large datasets,
|
|
avoid reading entire files into memory
|
|
- [ ] **Correct data structures** — O(1) lookups use `set`/`dict`, not `list`;
|
|
`dict` for key-value, `set` for membership, `list` for ordered iteration
|
|
- [ ] **No N+1 query patterns** in API calls or file I/O — batch operations where possible
|
|
- [ ] **No blocking I/O on hot paths** without justification — CLI startup, command execution
|
|
|
|
## 7. User Experience [manual]
|
|
|
|
- [ ] **Clear error messages** — tell the user what went wrong and how to fix it.
|
|
Example: "Error: Config file not found at /etc/grm.conf. Create it with: grm config init"
|
|
- [ ] **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.
|
|
New behavior via new functions/classes, not by modifying existing ones
|
|
- [ ] **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
|
|
|
|
## 11. Resource Management [auto + manual]
|
|
|
|
- [ ] **File handles closed** — use `with` statements or explicit `close()` in `finally`
|
|
- [ ] **Subprocess resources cleaned up** — call `.wait()` or `.communicate()`
|
|
- [ ] **Temporary files deleted** — use `tempfile.TemporaryDirectory()` or cleanup in `finally`
|
|
- [ ] **No resource leaks in error paths** — `try/finally` or context managers for cleanup
|
|
|
|
## 12. Backwards Compatibility [manual]
|
|
|
|
- [ ] **No breaking changes to public API** — or documented as major version bump
|
|
- [ ] **Removed functions deprecated first** — with `DeprecationWarning` and removal timeline
|
|
- [ ] **Default values added** instead of new required arguments
|
|
- [ ] **Return types stable** — no changes without major version bump
|
|
- [ ] **Behavior changes documented** — no silent behavior changes in existing functions
|
|
|
|
## 13. Logging and Observability [manual]
|
|
|
|
- [ ] **No sensitive data in logs** — tokens, passwords, PII excluded
|
|
- [ ] **Sufficient detail for debugging** — context, state, values logged at DEBUG level
|
|
- [ ] **Log levels appropriate** — DEBUG for internals, INFO for user actions, WARNING for recoverable issues
|
|
- [ ] **No log spam** — loops don't log per iteration, use DEBUG for high-frequency events
|