grm/REVIEW_CHECKLIST.md

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