# PR#7: Change kind "feature with tests", harmless files, DB schema and config version skew

> **WARNING:** This document describes only the change applied in this PR.
> It may already be outdated once the next PR is merged.
> Historic PR documentation is not maintained along with new PRs; treat it as a snapshot.

## The Problem

A typical feature PR changes main code and adds tests.
werkrevisor 0.9.0 calls this change kind `mixed`, so every such PR needs a human, even with an empty model finding list.

The test rule of 0.9.0 detects only removed lines with `@Test`, `assert`, `should` or `expect`.
A changed line in an existing test that does not contain these words is not detected.

Every file outside the categories is `other` and makes the PR YELLOW,
also a file that a repository considers harmless, e.g. an editor config.

DB schema changes are risky in every repository.
They get YELLOW only if each repository lists the paths in `[hard_yellow] paths`.

A changed `.werkrevisor.toml` is checked against the installed werkrevisor version.
A section or key that only a newer werkrevisor knows gives RED, although the file is correct:
PR#6 added `[writing]` while 0.8.0 was installed.

## Non-Goals

- No parser for test languages; the test rule reads only the diff lines.
- No second tier of critical paths; see Follow-up PRs.
- No change to the verdicts of the model or the classifier.

## The Scenarios

### Feature: AI-approvable change kinds

#### Background

- The config is `config/werkprobe.toml`: `src/main/**` is main code, `src/test/**` are tests, `doc/**` are docs.

#### Scenario#7.01: the change kind follows from the changed files

So that only simple change kinds can become GREEN without a human.

- **Given** a PR that changes `<files>`
- **When** werkrevisor applies the rules
- **Then** the change kind is `<kind>`
  - **and** it is AI-approvable: `<approvable>`

| files | kind | approvable |
|---|---|---|
| docs | `docs_only` | yes |
| added tests | `tests_only_added` | yes |
| docs and added tests | `docs_and_tests` | yes |
| main code, optionally docs | `refactoring` | yes |
| main code and added tests, optionally docs | `feature_with_tests` | yes |
| tests with a changed line | `tests_modified` | no |
| main code and tests with a changed line | `feature_with_modified_tests` | no |
| main code and an API spec | `mixed` | no |

##### Verified by

- [test_change_kinds.ChangeKindTest.test_change_kind_matrix](../../tests/test_change_kinds.py)

#### Scenario#7.02: a feature with purely additive tests can become GREEN

So that a typical feature PR merges without a human when the model finds no issue.

- **Given** a PR that changes `src/main/java/A.java` and adds a test method to `src/test/java/ATest.java`
- **When** werkrevisor reviews the PR
  - **and** the model recommends `safe_to_merge` without findings
- **Then** the verdict is GREEN
  - **and** the model gets the instruction to check that the added tests cover the changed behavior

##### Verified by

- [test_change_kinds.FeatureWithTestsFlowTest.test_feature_with_additive_tests_is_green](../../tests/test_change_kinds.py)

#### Scenario#7.03: a new assert line in an existing test method counts as added

So that a PR can make an existing test stricter.

- **Given** a PR that adds the line `assertThat(a.y()).isEqualTo(2);` to an existing test method
  - **and** removes no line of the test file
- **When** werkrevisor applies the rules
- **Then** the change kind is `feature_with_tests`
  - **and** there is no reason `tests_weakened`

##### Verified by

- [test_change_kinds.TestWeakeningTest.test_new_assert_line_in_an_existing_method_is_additive](../../tests/test_change_kinds.py)

#### Scenario#7.04: a changed or removed test line gives YELLOW

So that a PR cannot weaken a test unnoticed.

- **Given** a PR that changes main code
  - **and** `<change>` in `src/test/java/ATest.java`
- **When** werkrevisor applies the rules
- **Then** the verdict is YELLOW
  - **and** the reason is `test changes are not purely additive: <finding>`

| change | finding |
|---|---|
| replaces `assertThat(x).isEqualTo(1);` with `assertThat(x).isNotNull();` | `src/test/java/ATest.java:1 removes a test or assertion` |
| replaces `var a = new A(1);` with `var a = new A(2);` | `src/test/java/ATest.java:1 changes an existing test line` |
| deletes the file | `src/test/java/ATest.java deleted` |

##### Verified by

- [test_change_kinds.TestWeakeningTest.test_changed_or_removed_test_line_is_yellow](../../tests/test_change_kinds.py)

#### Scenario#7.05: a line that only moves stays additive

So that a test appended at the end of a file is not a change, when the diff re-adds an unchanged line.

- **Given** a test file diff that removes the line `}` and adds it again after a new test method
- **When** werkrevisor applies the rules
- **Then** there is no reason `tests_weakened`
  - **and** a removed assert line that is added again elsewhere still gives YELLOW
  - **and** a removed line with content, e.g. `var a = new A(1);`, that is added again elsewhere still gives YELLOW

##### Verified by

- [test_change_kinds.TestWeakeningTest.test_re_added_unchanged_line_is_additive_except_an_assertion](../../tests/test_change_kinds.py)
- [test_change_kinds.TestWeakeningTest.test_a_moved_line_with_content_counts_as_changed](../../tests/test_change_kinds.py)

#### Scenario#7.06: a disabled test gives YELLOW

So that a PR cannot switch off a failing test.

- **Given** a PR that adds the line `<line>` to a test file
- **When** werkrevisor applies the rules
- **Then** the verdict is YELLOW
  - **and** the reason names the line as `disables a test`
  - **and** the same marker inside a string literal, e.g. `("@Disabled", "@Ignore"),`, does not count

| line |
|---|
| `@Disabled` |
| `@Ignore` |
| `@pytest.mark.skip` |
| `@unittest.skip("flaky")` |
| `xit("works", () => {})` |
| `describe.skip("all", () => {})` |
| `it.only("this one", () => {})` |

##### Verified by

- [test_change_kinds.TestWeakeningTest.test_disabled_test_is_yellow](../../tests/test_change_kinds.py)
- [test_change_kinds.TestWeakeningTest.test_skip_markers_inside_string_literals_are_test_data](../../tests/test_change_kinds.py)

### Feature: harmless files

#### Scenario#7.07: a file that matches `[categories] harmless` does not spoil the change kind

So that a repository can declare its own harmless files, e.g. an editor config.

- **Given** `[categories] harmless = [".editorconfig"]`
- **When** a PR changes `src/main/java/A.java` and `<file>`
- **Then** the change kind is `<kind>`

| file | kind |
|---|---|
| `.editorconfig` | `refactoring` |
| `Makefile` | `mixed` |

##### Verified by

- [test_change_kinds.HarmlessTest.test_harmless_file_does_not_spoil_the_change_kind](../../tests/test_change_kinds.py)

#### Scenario#7.08: a PR of harmless files only is AI-approvable

So that a change of harmless files alone needs no human.

- **Given** `[categories] harmless = [".editorconfig"]`
- **When** a PR changes only `.editorconfig`
- **Then** the change kind is `harmless_only`
  - **and** it is AI-approvable

##### Verified by

- [test_change_kinds.HarmlessTest.test_harmless_only_is_approvable](../../tests/test_change_kinds.py)

#### Scenario#7.09: harmless files never hide risky paths

So that `harmless` cannot weaken a hard rule.

- **Given** `[categories] harmless = ["**"]`
- **When** a PR changes `.gitea/workflows/ci.yaml`
- **Then** the verdict is YELLOW with the reason `risky paths changed: .gitea/workflows/ci.yaml (.gitea/**)`
  - **and** a file of the main code is still main code

##### Verified by

- [test_change_kinds.HarmlessTest.test_harmless_never_hides_risky_paths_or_main_code](../../tests/test_change_kinds.py)

#### Scenario#7.10: the host config and `.werkrevisor.toml` may set `harmless`

So that the repository keeps its harmless files next to its code.

- **Given** the host config sets no `harmless`
  - **and** `.werkrevisor.toml` sets `[categories] harmless = [".editorconfig"]`
- **When** werkrevisor builds the config
- **Then** `harmless` is `[".editorconfig"]`
  - **and** without both layers it is empty

##### Verified by

- [test_change_kinds.HarmlessTest.test_harmless_comes_from_host_or_repository_and_defaults_to_empty](../../tests/test_change_kinds.py)

### Feature: DB schema changes need a human

#### Scenario#7.11: a DB schema change gives YELLOW

So that a human always checks a DB schema change.

- **Given** no `[hard_yellow] paths`
- **When** a PR changes `<path>`
- **Then** the verdict is YELLOW with the reason `DB schema changed: <path> (built in)`

| path |
|---|
| `src/main/resources/db/changelog/1-create.sql` |
| `app/migrations/0002_add_field.py` |
| `src/main/resources/db/migration/V2__x.java` |
| `src/main/resources/liquibase/master.xml` |
| `src/main/resources/flyway/V3.sql` |
| `db/schema.changelog.yaml` |
| `src/main/resources/db.changelog-master.yaml` |

##### Verified by

- [test_change_kinds.DbSchemaTest.test_db_schema_change_is_yellow](../../tests/test_change_kinds.py)

#### Scenario#7.12: no config layer removes the DB schema paths

So that a repository config cannot switch off the DB schema rule.

- **Given** `[hard_yellow] paths = []`, `[categories] docs = ["**"]` and `[categories] harmless = ["**"]`
- **When** a PR changes `src/main/resources/db/changelog/1-create.sql`
- **Then** the verdict is YELLOW with the reason `DB schema changed: src/main/resources/db/changelog/1-create.sql (built in)`
  - **and** a Markdown file below a migration directory is no DB schema change

##### Verified by

- [test_change_kinds.DbSchemaTest.test_no_layer_removes_the_db_schema_paths](../../tests/test_change_kinds.py)

### Feature: a changed `.werkrevisor.toml` for a newer werkrevisor

#### Scenario#7.13: a section or key unknown to the installed version gives YELLOW

So that a PR can prepare the config for the next werkrevisor version.

- **Given** the installed werkrevisor is 0.10.0
  - **and** the PR adds `<setting>` to `.werkrevisor.toml`
- **When** werkrevisor reviews the PR
- **Then** the verdict is YELLOW
  - **and** the reason names `<shown>` as unknown to the installed werkrevisor 0.10.0
  - **and** the reason asks to deploy a version that knows it right after the merge
  - **and** the model reviews the PR

| setting | shown |
|---|---|
| a section `[future]` | `[future]` |
| a key `max_hunks` in `[size]` | `[size] max_hunks` |

##### Verified by

- [test_config_layers.ChangedRepositoryConfigTest.test_setting_unknown_to_the_installed_version_is_yellow](../../tests/test_config_layers.py)

#### Scenario#7.14: an invalid `.werkrevisor.toml` stays RED

So that a broken config never reaches the base branch.

- **Given** the PR changes `.werkrevisor.toml` with `<error>`
- **When** werkrevisor reviews the PR
- **Then** the verdict is RED with the reason `repository_config_invalid`

| error |
|---|
| a TOML syntax error |
| a value of the wrong type |
| a host key, e.g. `[model] id` |
| an unknown key together with a value of the wrong type |

##### Verified by

- [test_config_layers.ChangedRepositoryConfigTest.test_invalid_change_is_red_without_model_call](../../tests/test_config_layers.py)
- [test_config_layers.ChangedRepositoryConfigTest.test_wrong_value_type_is_red](../../tests/test_config_layers.py)
- [test_config_layers.ChangedRepositoryConfigTest.test_syntax_error_and_unknown_with_invalid_settings_stay_red](../../tests/test_config_layers.py)

## The Solution

- New change kind `feature_with_tests`: main code and tests, optionally docs, with purely additive test changes.
  The defaults and the example host configs list it in `auto_approvable`.
  The model gets the instruction to check that the added tests cover the changed behavior.
- The test rule counts every removed or changed non-blank line of a test file,
  so a replaced assert line is a change, not an addition.
  Only a line without content of its own may move, e.g. a closing brace; the self-review found that any re-added line passed.
  The rule detects more skip markers, e.g. `pytest.mark.skip`, `xit(` and `.only(`;
  a marker inside a string literal is test data and does not count.
  A weakened test makes the kind `tests_modified` or `feature_with_modified_tests`.
- New key `[categories] harmless`, default empty, allowed in the host config and in `.werkrevisor.toml`.
  It applies only to files outside the other categories.
  Harmless files come along with every change kind; alone they are the change kind `harmless_only`.
- Built-in DB schema paths give YELLOW with the reason `DB schema changed: <path> (built in)`.
  No config layer can remove them; Markdown files do not count.
- A changed `.werkrevisor.toml` with a section or key unknown to the installed version gives YELLOW,
  if the rest of the file is valid.
  Syntax errors, wrong types and host keys stay RED.
- werkrevisor 0.10.0; the README and the German guides describe the change.

## Open Questions

- [x] Does a PR of harmless files alone get a change kind of its own?
  Yes: `harmless_only`, in the default `auto_approvable`; without `harmless` globs it never occurs.
- [x] Does a removed line outside a test method count as a change?
  Yes: the rule reads only the diff and cannot see method borders.
  Helpers and fixtures also influence the tests, so the strict rule is the safe one.
- [x] Does this PR add `feature_with_tests` to the `auto_approvable` of `.werkrevisor.toml`?
  No: the installed werkrevisor 0.9.0 does not know the value, so the check of the changed file would give RED.
  A follow-up PR adds it after 0.10.0 is deployed.

## Additional Changes

- The YELLOW examples of the gate tests use main code with a `Makefile`, because main code with added tests can now be GREEN.

## Follow-up PRs

- Critical paths in two tiers, configurable per repository: `[hard_yellow] paths` and a new `[hard_orange] paths`.
- Add `feature_with_tests` to `[categories] auto_approvable` in `.werkrevisor.toml`, after 0.10.0 is deployed.