werkator build succeeded after 01:14
werkrevisor 0.13.0: mechanical file changes skip the model. - A file is mechanical only if every changed line is a pure rename, a whitespace change (not in whitespace-significant files) or an import line of a known language; it goes neither into the diff nor into the full files for the model. - A PR of only mechanical files is GREEN-eligible without a model call; hard rules still apply. - Additional changes: the example host configs match mih09 (`mode = "combined"`, `token_count_factor = 1.55`); an unknown change kind in a changed `.werkrevisor.toml` gives YELLOW, not RED. Details: the PR document under `docs/prs/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed-on: #11 Co-authored-by: claude-code <claude-code@javagil.de>
271 lines
12 KiB
Markdown
271 lines
12 KiB
Markdown
# PR#11: mechanical file changes skip the model
|
||
|
||
> **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
|
||
|
||
werkrevisor 0.12.0 sends the diff and the full content of every changed file to the model.
|
||
Many changed files carry no logic change: changed imports, a new indentation, a renamed file.
|
||
These files cost tokens, and the model has nothing to judge in them.
|
||
A PR of such files alone needs a model call to become GREEN, although deterministic rules can judge it.
|
||
|
||
## Non-Goals
|
||
|
||
- No Jev-based risk filter per file; it stays a measured follow-up, see `docs/konzept-jev-classifier.md`.
|
||
- No reformatting rule beyond whitespace, e.g. changed line breaks or spaces around operators.
|
||
- No change of the hard rules: risky paths, DB schema, open questions and writing rules apply to mechanical files too.
|
||
|
||
## The Scenarios
|
||
|
||
### Feature: mechanical file changes skip the model
|
||
|
||
#### Background
|
||
|
||
- The config is `config/werkprobe.toml` with `[mechanical] enabled = true`.
|
||
- A file is mechanical if the rules `imports`, `whitespace` or `renamed` explain every changed line.
|
||
|
||
#### Scenario#11.01: a file with only import changes is not sent to the model
|
||
|
||
So that the model reads only files with something to judge.
|
||
|
||
- **Given** a PR changes `A.java` with logic and `B.java` with imports only
|
||
- **When** werkrevisor reviews the PR
|
||
- **Then** the model prompt contains neither the diff nor the full content of `B.java`
|
||
- **and** the comment says `1 file changed only mechanically and was not reviewed by the model: B.java (imports).`
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.FlowTest.test_mechanical_files_stay_out_of_the_prompt_and_the_coverage_check](../../tests/test_mechanical.py)
|
||
- [test_pr_agent_run.PrAgentRunTest.test_excluded_files_never_reach_the_prompt](../../tests/test_pr_agent_run.py)
|
||
|
||
#### Scenario#11.02: a mechanical file missing in the prompt is no skipped file
|
||
|
||
So that leaving out a mechanical file does not end the review with ERROR.
|
||
|
||
- **Given** a PR changes `A.java` with logic and `B.java` with imports only
|
||
- **When** the model reviews the PR without `B.java` in its prompt
|
||
- **Then** there is no reason `skipped_files`
|
||
- **and** the verdict is GREEN
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.FlowTest.test_mechanical_files_stay_out_of_the_prompt_and_the_coverage_check](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.03: the comment lists many mechanical files in one line
|
||
|
||
So that a large package move does not fill the comment.
|
||
|
||
- **Given** a PR changes 4 Java files with imports only and `a.c` with whitespace only
|
||
- **When** werkrevisor reviews the PR
|
||
- **Then** the comment says `5 files changed only mechanically and were not reviewed by the model:`
|
||
followed by 3 files with their rules and `and 2 more.`
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.FlowTest.test_comment_lists_many_mechanical_files_in_one_line](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.04: an added or removed import line is mechanical only for the languages with an import rule
|
||
|
||
So that only a line without effect at runtime skips the model.
|
||
|
||
- **Given** a file with the extension `<extension>` changes only in the line `<line>`
|
||
- **When** werkrevisor checks the file
|
||
- **Then** the file is `<result>`
|
||
|
||
| extension | line | result |
|
||
|---|---|---|
|
||
| `java`, `kt`, `kts` | `import java.util.Map;`, `package net.example;` | mechanical (imports) |
|
||
| `ts`, `tsx`, `js`, `jsx`, `mjs` | `import { a, b } from './x';` | mechanical (imports) |
|
||
| `ts` | `import './polyfill';` | not mechanical |
|
||
| `ts` | ` b,` inside an `import { … } from './x';` that the hunk shows completely | mechanical (imports) |
|
||
| `ts` | ` b,` inside an import whose first line is outside the hunk | not mechanical |
|
||
| `go` | `"os"` inside an `import ( … )` block that the hunk shows completely | mechanical (imports) |
|
||
| `go` | `_ "embed"` or `. "math"` | not mechanical |
|
||
| `rs` | `use std::io::{Read, Write};` | mechanical (imports) |
|
||
| `rs` | `mod helpers;` | not mechanical |
|
||
| `py` | `import os` | not mechanical |
|
||
| `scala` | `import a.b.C` | not mechanical |
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.ImportRuleTest](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.05: one changed line without a rule makes the file not mechanical
|
||
|
||
So that a logic change never hides between import changes.
|
||
|
||
- **Given** a Java file changes 3 import lines and adds the line `int x = 1;`
|
||
- **When** werkrevisor checks the file
|
||
- **Then** the file is not mechanical
|
||
- **and** the model gets its diff and its full content
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.ImportRuleTest.test_one_non_import_line_makes_the_file_not_mechanical](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.06: a whitespace change is mechanical except in whitespace-significant files
|
||
|
||
So that a new indentation skips the model only where indentation has no meaning.
|
||
|
||
- **Given** the file `<file>` changes only the indentation of one line
|
||
- **When** werkrevisor checks the file
|
||
- **Then** the file is `<result>`
|
||
|
||
| file | result |
|
||
|---|---|
|
||
| `src/a.c`, `src/A.java` | mechanical (whitespace) |
|
||
| `app/a.py`, `ci.yml`, `x.yaml`, `Makefile`, `rules.mk`, `README.md` | not mechanical |
|
||
| `v.haml`, `v.pug`, `a.coffee`, `a.nim`, `a.fs` | not mechanical |
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.WhitespaceRuleTest.test_indentation_trailing_whitespace_and_blank_lines](../../tests/test_mechanical.py)
|
||
- [test_mechanical.WhitespaceRuleTest.test_whitespace_significant_files_are_never_whitespace_mechanical](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.07: a renamed file is mechanical only with identical content
|
||
|
||
So that a moved file with changes still gets a review.
|
||
|
||
- **Given** Gitea reports `new/A.java` as renamed from `old/A.java` with `<added>` added and `<removed>` removed lines
|
||
- **When** werkrevisor checks the file
|
||
- **Then** the file is `<result>`
|
||
|
||
| added | removed | result |
|
||
|---|---|---|
|
||
| 0 | 0 | mechanical (renamed) |
|
||
| 1 | 1 | not mechanical |
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.RenameRuleTest.test_rename_with_identical_content](../../tests/test_mechanical.py)
|
||
- [test_mechanical.RenameRuleTest.test_rename_with_changes_is_not_mechanical](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.08: a PR of mechanical files only is GREEN without a model call
|
||
|
||
So that such a PR costs no tokens.
|
||
|
||
- **Given** a PR changes `B.java` with imports only and `a.c` with whitespace only
|
||
- **When** werkrevisor reviews the PR
|
||
- **Then** the change kind is `mechanical_only`
|
||
- **and** the verdict is GREEN
|
||
- **and** neither Jev nor the model is called
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.FlowTest.test_all_mechanical_pr_is_green_without_model_call](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.09: a mechanical change of a risky path stays YELLOW
|
||
|
||
So that the hard rules keep their effect.
|
||
|
||
- **Given** a PR changes `src/main/java/net/example/auth/Login.java` with imports only
|
||
- **When** werkrevisor reviews the PR
|
||
- **Then** the verdict is YELLOW with the reason `risky_paths`
|
||
- **and** the model is not called
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.FlowTest.test_mechanical_change_of_a_risky_path_stays_yellow](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.10: changed imports in a test file are no weakened test
|
||
|
||
So that a feature PR that also cleans up test imports can still become GREEN.
|
||
|
||
- **Given** a PR changes main code and replaces an import in `ATest.java`
|
||
- **When** werkrevisor applies the rules
|
||
- **Then** there is no reason `tests_weakened`
|
||
- **and** the change kind is `refactoring`
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.RulesTest.test_import_only_change_in_a_test_file_is_not_test_weakening](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.11: mechanical files do not count for the size limits
|
||
|
||
So that a package move across many files does not need a human only because of its size.
|
||
|
||
- **Given** `[size] max_files = 30`
|
||
- **and** a PR changes 31 Java files with import lines only and one file with logic
|
||
- **When** werkrevisor applies the rules
|
||
- **Then** the size check counts 1 file
|
||
- **and** the PR does not become ORANGE because of its size
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.RulesTest.test_mechanical_files_do_not_count_for_the_size_limits](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.12: an invalid import regex is a config error
|
||
|
||
So that a typo in a regex cannot silently switch a rule off.
|
||
|
||
- **Given** a config layer sets `[mechanical.imports] java = ["(unclosed"]`
|
||
- **When** werkrevisor loads the config
|
||
- **Then** the result is ERROR with `[mechanical] imports.java has an invalid regex '(unclosed'`
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.ConfigTest.test_invalid_regex_is_a_config_error](../../tests/test_mechanical.py)
|
||
|
||
#### Scenario#11.13: the repository config replaces the import regexes per extension
|
||
|
||
So that a repository can add a language without repeating the built-in ones.
|
||
|
||
- **Given** the host config sets `[mechanical.imports] scala = [...]`
|
||
- **and** `.werkrevisor.toml` sets `[mechanical.imports] java = []`
|
||
- **When** werkrevisor merges the layers
|
||
- **Then** Scala imports are mechanical, Java imports are not, and TypeScript keeps its built-in rule
|
||
|
||
##### Verified by
|
||
|
||
- [test_mechanical.ConfigTest.test_layers_merge_the_imports_per_extension](../../tests/test_mechanical.py)
|
||
|
||
## The Solution
|
||
|
||
- `werkrevisor/mechanical.py` decides per changed file which rules explain every changed line.
|
||
It works on the hunks of the Gitea diff, which `diff.py` now keeps with their context lines.
|
||
- A multi-line import counts only if the whole statement is in one hunk, so missing context gives "not mechanical".
|
||
- The whitespace rule compares the changed lines like `git diff -b`:
|
||
an added space inside a line, e.g. in a string literal, is a change.
|
||
- A file whose diff has fewer lines than the counts of Gitea is not mechanical, because the missing lines could hold anything.
|
||
- `evaluate_rules` leaves the mechanical files out of the change kind, the test weakening check and the size limits;
|
||
a PR of mechanical files only gets the new change kind `mechanical_only`.
|
||
- `pr_agent_settings` passes the mechanical paths as exact `ignore.regex` entries,
|
||
so PR-Agent leaves them out of the diff it sends; `fullfiles.collect` leaves out their full content.
|
||
- `coverage_reasons` does not miss them, and the comment and the usage record (`mechanical_files`) list them.
|
||
- The new section `[mechanical]` is required in every merged config: `enabled`, `whitespace_significant` and `imports`.
|
||
The repository layer replaces `enabled` and `whitespace_significant`, and the regexes per extension of `imports`.
|
||
- `mechanical_only` is in the default `auto_approvable` and in the example configs.
|
||
`.werkrevisor.toml` stays unchanged; the host config on mih09 gets `[mechanical]` with the deploy of 0.13.0.
|
||
- README, `docs/konzept-jev-classifier.md` and `docs/betrieb-git.javagil.de.md` describe the change; werkrevisor 0.13.0.
|
||
|
||
## Open Questions
|
||
|
||
- [x] Is a renamed file with changed imports, e.g. a moved Java class with a new `package` line, mechanical?
|
||
No: a renamed file is mechanical only with identical content, so the model still reviews a move with changes.
|
||
- [x] Do mechanical files count for the `[size]` limits?
|
||
No: they need no AI judgement, as `docs/konzept-jev-classifier.md` intends for a package move across many files.
|
||
- [x] Does a changed space inside a line count as whitespace only, like `git diff -w`?
|
||
No: the rule follows `git diff -b`, because a space inside a string literal can change the behavior.
|
||
- [x] Is an added or deleted file with only import lines mechanical?
|
||
No: only a file that exists before and after the PR can be mechanical.
|
||
|
||
## Additional Changes
|
||
|
||
- The example host configs follow the configuration on mih09:
|
||
`config/werkprobe.toml`, `config/werkrevisor.toml` and `config/hsadmin-ng.toml` use `[engine] mode = "combined"`
|
||
and `[model] token_count_factor = 1.55`.
|
||
On werkrevisor PR#10 the factor 1.15 gave 49841 tokens, and Anthropic reported 65792 prompt tokens for Sonnet;
|
||
so `o200k_base` counts about 35 % too low, and 1.15 × 65792 / 49841 ≈ 1.52 is rounded up to 1.55.
|
||
- The test helper `config()` sets `[engine] mode = "llm"`, because most tests check the LLM path;
|
||
the classifier tests set their mode explicitly.
|
||
- The tests that depend on the factor 1.15 set it explicitly.
|
||
- A change kind in `[categories] auto_approvable` of a changed `.werkrevisor.toml` that the installed werkrevisor
|
||
does not know gives YELLOW like an unknown key, not RED:
|
||
a newer version may know it, e.g. `mechanical_only` while 0.12.0 is installed.
|
||
|
||
## Follow-up PRs
|
||
|
||
- After the deploy of 0.13.0, `.werkrevisor.toml` adds `mechanical_only` to `[categories] auto_approvable`, like PR#8.
|