grm/docs/tech/contributing.md

# Contributing Guide

## Key Conventions

- Python 3.12+ required (ruff/pyright target `py312`)
- 100% test coverage required (`--cov-fail-under=100`)
- Conventional commits on feature branches (no `GRM-N:` prefix)
- Branch names must include `GRM-N` task ID
- Line length: 120 chars
- Secrets are passed via temp JSON files, never on the command line (CWE-214)
- CI triggers only on `opened` and `synchronize` PR events (not `labeled`)
- No `print()` — use `click.echo()` via `ui.say()` for console output
- No bare `except` — catch specific exceptions
- No `TODO`/`FIXME` comments in committed code
- No functions longer than 50 lines
- No `shell=True` with subprocess
- No `eval()` or `exec()`
- No raw strings in `click.echo()` without `_()` wrapper (i18n)
- No `open()` without `with` statement
- No `Popen()` without cleanup

## Code Style Rules

- **Python version**: 3.12+ (ruff and pyright target `py312`)
- **Line length**: 120 characters
- **Test coverage**: 100% required (`--cov-fail-under=100`)
- **Secrets handling**: Secrets are passed via temp JSON files with `0600` permissions, never on the command line (CWE-214). Extra-vars are written to a temporary JSON file and passed via `--extra-vars @tempfile`, which is deleted after execution. This prevents secrets from being visible in the process list (`ps aux`).
- **Linting**: `make lint-all` runs ruff + pyright + bandit + ansible-lint + checkmake + actionlint
- **Formatting**: `ruff format` with double quotes and space indentation
- **Type checking**: `pyright` in strict mode for `src/grm/`
- **Security scanning**: `bandit -r src/` on every PR
- **Import rules**: `src/grm/` NEVER imports from devx — the GRM tool is self-contained

## Commit Rules

Branch commits use conventional commit format (no `GRM-N:` prefix):

```
feat: add new feature
fix: resolve bug
docs: update README
ci: update workflow
refactor: simplify executor
test: add molecule scenario
chore: update dependencies
```

The pre-commit hook validates that commit messages follow the conventional commit format. Non-conventional commits are rejected.

### Version Bumping Rules

| Commit type | Version bump |
|-------------|-------------|
| `feat:` | minor (0.X.0) |
| `fix:` | patch (0.0.X) |
| `feat!:` or `BREAKING CHANGE` | minor (pre-1.0: major would be 1.0.0) |
| `chore:`, `ci:`, `docs:` | no bump (excluded by cliff.toml) |

## Branch Naming

| What | Format | Example |
|------|--------|---------|
| Branch name | `GRM-N-short-description` | `GRM-33-add-pr-review-step` |
| Branch commits | `<conventional commit>` | `feat: add review script` |
| PR title | `GRM-N: <vikunja task title>` | `GRM-33: Add mandatory PR review step` |
| Merge commit | `GRM-N <conventional commit>` | `GRM-33 feat: add review script` |

## PR Workflow Summary

Every change to master goes through this workflow. No exceptions.

1. **Create Vikunja task** — get a `GRM-N` identifier (Vikunja project 6)
2. **Create branch** — `GRM-N-short-description`
3. **Implement** — write code, tests (100% coverage), update docs
4. **Commit** — conventional commits (no `GRM-N:` prefix on branch)
5. **Push & create PR** — title: `GRM-N: <vikunja task title>`, body: summary + `Closes GRM-N`
6. **Review** — review the full diff focusing on: functional completeness, edge cases, technical excellence (architecture, SRP, deduplication, code smells, best practices, code quality, reusability, clean code, readability, maintainability, extensibility), performance, security, UX, documentation completeness/relevance. Post review comments via `devx.ci.pr_review`.
7. **Address comments** — fix each comment, commit, push, re-review
8. **Approve** — post an `APPROVE` review via `devx.ci.pr_review`
9. **Add `ready-to-merge` label** — auto-merge workflow squash-merges with title `GRM-N <conventional commit message>`, post-merge workflow marks the Vikunja task as done, release workflow automatically versions and tags
### 1. Create Vikunja task

Create a task in Vikunja project 6 to get a `GRM-N` identifier.

### 2. Create branch

```bash
git checkout master && git pull
git checkout -b GRM-N-short-description
```

### 3. Implement changes

- Write code following conventions above
- Write/update tests (100% coverage required)
- Update documentation (CHANGELOG, README, AGENTS.md, docs/ as needed)

### 4. Commit (conventional commits)

Branch commits use conventional commit format (no `GRM-N:` prefix):

```
feat: add new feature
fix: resolve bug
docs: update README
```

### 5. Push and create PR

- **PR title format**: `GRM-N: <vikunja task title>` (must match the Vikunja task title exactly)
- PR body: summary of changes, `Closes GRM-N`
- Add `ready-to-merge` label **only after review is complete**

### 6. Review the PR

Review the full diff (`git diff master...HEAD`) focusing on:

- **Functional completeness**: Does the code do what it claims? Are all requirements met?
- **Edge cases**: Are boundary conditions, empty inputs, error paths handled?
- **Technical excellence**:
  - Architecture compliance and evolution
  - Single Responsibility Principle (SRP)
  - Deduplication (no copy-paste, single source of truth)
  - Code smells detection and removal
  - Best industry practices
  - Industry-grade code quality
  - Reusability
  - Clean code
  - Readability
  - Maintainability
  - Extensibility
- **Performance**: No unnecessary allocations, O(n) vs O(n^2), efficient data structures
- **Security**: No secrets in logs/process list, input validation, no injection vectors
- **User experience**: Clear error messages, intuitive CLI flags, helpful output
- **Documentation**: Completeness and relevance of docs, CHANGELOG entries, AGENTS.md updates

Post review comments using `devx.ci.review_pr`:

```bash
CI_GITEA_TOKEN=<token> python -m devx.ci.review_pr <pr_number> <owner/repo> \
  --event REQUEST_CHANGES \
  --body "Review summary" \
  --comments-json comments.json
```

### 7. Address review comments

Fix each comment one by one, commit, and push. Re-review until satisfied.

### 8. Approve and merge

Once all comments are addressed, post an approval review:

```bash
CI_GITEA_TOKEN=<token> python -m devx.ci.review_pr <pr_number> <owner/repo> \
  --event APPROVE --checklist-confirmed \
  --checklist-categories 1,2,3,4,5,6,7,8,9,10,11,12,13 \
  --body "All 13 checklist categories verified."
```

Then add the `ready-to-merge` label. The auto-merge workflow will:

1. Validate PR title format and match against Vikunja task title
2. Check that at least one APPROVE review exists
3. Wait for all CI checks to pass
4. Squash-merge with title: `GRM-N <conventional commit message>` (space-separated)
5. The post-merge workflow marks the Vikunja task as done
6. The release workflow automatically versions, tags, and publishes

> **IMPORTANT**: Never manually merge PRs via the API. Always use the auto-merge workflow by adding the `ready-to-merge` label. Manual merges bypass the `GRM-N <conventional>` format enforcement.

### Branch Protection (Required Gitea Settings)

Branch protection is automatically configured by `devx.tools.configure_repo` (runs as a `configure-repo` job in the post-merge workflow). The following rules are enforced for `master`:

- **Require pull request**: No direct pushes to master
- **Require approval review**: At least 1 `APPROVE` review before merge
- **Require status checks**: CI quality + molecule tests must pass
- **Block force pushes**: No history rewriting on master

The auto-merge workflow enforces the APPROVE review check programmatically as a defense-in-depth measure, but branch protection is the primary gate.

## Build & Test Commands

```bash
make setup          # Create venv, install deps, set up hooks, install CI tools
make lint-all       # ruff + pyright + bandit + ansible-lint + checkmake + actionlint
make pytest-cov     # Unit tests with 100% coverage enforcement
make test-unit      # Unit tests without coverage
make molecule       # All 6 scenarios on Ubuntu 22.04
make molecule-all   # All 6 scenarios on all 4 supported OSes
make test-all       # pytest-cov + molecule
make workflow-check # Static lint + dry-run of workflow YAML
```

## Ansible Role Conventions

```
main.yml → systemd_check → user_setup → rootless_docker → install_runner → prune → integration_test
```

- `install_runner.yml` handles: download, config, validate, register, service
- `main.yml` handles: prune, integration_test (NOT install_runner — avoids duplicates)
- `systemctl --user` tasks must be guarded by `docker_rootless_setup`
- Template creation tasks are NOT guarded by `docker_rootless_setup` (they just create files)
- `apt` tasks use `cache_valid_time: 3600` to avoid unnecessary cache updates
- `remove-runner.yml` runs `loginctl disable-linger` and removes subuid/subgid entries

## Change Classification

Not all changes require a new release. The project classifies changes using `devx.ci.classify_changes`:

**Workflow-only paths** (no release needed):
- `.gitea/**`, `docs/**`, `tests/**`, `scripts/**`
- `AGENTS.md`, `README.md`, `CHANGELOG.md`, `Makefile`, `cliff.toml`
- Lint config files, `.env.example`, `.gitignore`

**User-facing paths** (release needed):
- `src/grm/**` (except `__init__.py`)
- `ansible/**`
- `pyproject.toml`

When working on workflow/CI/docs-only changes, use `ci:` or `docs:` commit prefixes. Do NOT bump the version or create tags for workflow-only changes.

## Known Issues

- `ansible-lint` may warn about `command-instead-of-module` for `systemctl --user` calls — this is expected (systemd module doesn't support user services) and skipped in `.ansible-lint`
- Molecule Docker driver may print "Event loop is closed" warnings on interrupt — harmless