werkator build succeeded after 01:57
werkrevisor 0.8.0: repository config layer, clear context reasons, docs only as diff. - Config layers: built-in defaults < host < `.werkrevisor.toml` read from the PR base; the repository may only tighten cost and model limits, and `.werkrevisor.toml` is always a risky path. - One clear reason per cause instead of one line per file. - Documentation files go to the model only as diff and are judged against the writing style in AGENTS.md. - AGENTS.md with the shared writing style. Details: the PR document under `docs/prs/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed-on: #5 Co-authored-by: claude-code <claude-code@javagil.de>
303 lines
13 KiB
Markdown
303 lines
13 KiB
Markdown
# PR#5: Repository config and clear context reasons
|
|
|
|
> **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
|
|
|
|
The operator config is one complete TOML on the host.
|
|
It mixes host matters (model, prices, budget, gate) with repository matters (paths, categories, review instructions).
|
|
A repository cannot keep its own review rules next to its code, and every change needs the host operator.
|
|
|
|
The full-file context gives one reason per left-out file.
|
|
A large PR showed 20 such lines, and the text "full-file context incomplete" did not say what it means for the review.
|
|
|
|
Documentation files also go into the full-file context.
|
|
They crowd out code files, although the model reviews docs for their wording, not against the rest of the file.
|
|
|
|
## Non-Goals
|
|
|
|
- No repository config for hsadmin-ng in this PR; `config/hsadmin-ng.toml` stays complete.
|
|
- No change to the verdict colors or to the gate.
|
|
- No change to the werkrevisor installation on mih09.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: a repository config on top of the host config
|
|
|
|
#### Background
|
|
|
|
- The host config is `config/werkprobe.toml`.
|
|
- The repository has a `.werkrevisor.toml` at its root, unless a scenario says otherwise.
|
|
|
|
#### Scenario#5.01: a later layer wins per key
|
|
|
|
So that a repository can keep its own review rules next to its code.
|
|
|
|
- **Given** no layer sets `[review] max_findings`, the host sets `[size] max_files = 30`
|
|
- **and** `.werkrevisor.toml` sets `[review] max_findings = 7` and `[size] max_files = 12`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** `max_findings` is 7 and `max_files` is 12
|
|
- **and** `max_changed_lines` keeps the host value 800
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.LayeringTest.test_repository_wins_over_host_and_host_over_defaults](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.02: the repository config is read at the base commit
|
|
|
|
So that a PR cannot change its own review.
|
|
|
|
- **Given** `.werkrevisor.toml` sets `max_files = 11` at the base commit and `max_files = 99` at the head commit
|
|
- **When** werkrevisor builds the config
|
|
- **Then** `max_files` is 11
|
|
- **and** werkrevisor requested the file only at the base commit
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.RepositoryReadTest.test_reads_the_base_not_the_head](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.03: without the file there is no repository layer
|
|
|
|
So that repositories without a `.werkrevisor.toml` keep working.
|
|
|
|
- **Given** the base commit has no `.werkrevisor.toml`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** the config is the host config
|
|
- **and** the config sources are `defaults + host`
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.RepositoryReadTest.test_missing_file_means_no_repository_layer](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.04: a host key in the repository config is an error
|
|
|
|
So that model, money and merge rights stay with the host operator.
|
|
|
|
- **Given** `.werkrevisor.toml` sets `<key>`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** the run ends with ERROR (exit 30) and the message `must not set <key>: only the host config sets it`
|
|
|
|
| key |
|
|
|---|
|
|
| `[model] id` |
|
|
| `[model] input_price_usd_per_mtok` |
|
|
| `[model] reasoning_effort` |
|
|
| `[budget] min_key_remaining_usd` |
|
|
| `[gate] yellow_gate` |
|
|
| `[gate] human_reviewers` |
|
|
| `[engine] mode` |
|
|
| `[classifier] endpoint` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.LayeringTest.test_host_only_keys_are_refused](../../tests/test_config_layers.py)
|
|
- [test_config_layers.MainRepositoryConfigTest.test_forbidden_repository_key_is_error_exit_30](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.05: a repository config may only lower a cost or context limit
|
|
|
|
So that a repository cannot make a review more expensive than the host allows.
|
|
|
|
- **Given** `.werkrevisor.toml` sets `<key>` above the host value
|
|
- **When** werkrevisor builds the config
|
|
- **Then** the run ends with ERROR and the message `exceeds the host value`
|
|
|
|
| key |
|
|
|---|
|
|
| `[model] max_context_tokens` |
|
|
| `[model] max_output_tokens` |
|
|
| `[budget] max_usd_per_review` |
|
|
| `[review] max_full_file_chars` |
|
|
| `[review] max_full_files_chars` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.LayeringTest.test_limits_may_only_be_lowered](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.06: repository risky paths are added to the host paths
|
|
|
|
So that a repository can mark more files as risky, but never fewer.
|
|
|
|
- **Given** the host risky paths contain `.gitea/**`
|
|
- **and** `.werkrevisor.toml` sets `[hard_yellow] paths = ["werkrevisor/config.py"]`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** the risky paths contain both
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.LayeringTest.test_hard_yellow_paths_are_added](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.07: repository review instructions are appended to the host instructions
|
|
|
|
So that the host rules stay in force.
|
|
|
|
- **Given** the host instructions `Host rule.` and the repository instructions `Repository rule.`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** the instructions are `Host rule.`, a blank line, and `Repository rule.`
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.LayeringTest.test_extra_instructions_are_appended](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.08: a change of the repository config needs a human
|
|
|
|
So that nobody changes the review rules without a human approval.
|
|
|
|
- **Given** a PR that changes `.werkrevisor.toml`, while no layer lists it as a risky path
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is at least YELLOW with the reason `risky paths changed: .werkrevisor.toml (built in)`
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardYellowTest.test_repository_config_is_always_a_risky_path](../../tests/test_rules.py)
|
|
- [test_config_layers.ChangedRepositoryConfigTest.test_valid_change_needs_a_human](../../tests/test_config_layers.py)
|
|
|
|
#### Scenario#5.09: an invalid change of the repository config is RED
|
|
|
|
So that a broken file cannot make every later review end with ERROR.
|
|
|
|
- **Given** a PR that sets `[model] id` in `.werkrevisor.toml`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is RED with the reason `the changed .werkrevisor.toml is invalid: …`
|
|
- **and** no model call is made
|
|
|
|
##### 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)
|
|
|
|
#### Scenario#5.10: the comment names the config sources
|
|
|
|
So that a reviewer sees which config judged the PR.
|
|
|
|
- **Given** a repository config at the base commit `bbbbbbb`
|
|
- **When** werkrevisor writes its comment
|
|
- **Then** the usage line contains `config: defaults + host + repository (.werkrevisor.toml at base bbbbbbb)`
|
|
- **and** the usage record has the same text as `config_source`
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_layers.ChangedRepositoryConfigTest.test_comment_names_the_config_sources](../../tests/test_config_layers.py)
|
|
|
|
### Feature: clear and short reasons
|
|
|
|
#### Scenario#5.11: files left out of the full-file context give one reason
|
|
|
|
So that the reviewer reads one line that says what it means for the review.
|
|
|
|
- **Given** 21 changed code files of which only the first fits into `[review] max_full_files_chars = 200`
|
|
- **When** werkrevisor collects the full files
|
|
- **Then** the only reason is `20 changed files were given to the AI only as diff, not in full,
|
|
so it could not check them against the rest of their file
|
|
(all full files together would exceed [review] max_full_files_chars = 200): src/f01.py, src/f02.py, src/f03.py and 17 more`
|
|
|
|
##### Verified by
|
|
|
|
- [test_fullfiles.CollectTest.test_files_left_out_for_one_cause_give_one_reason](../../tests/test_fullfiles.py)
|
|
|
|
#### Scenario#5.12: different causes give one reason each
|
|
|
|
So that the operator knows which limit to change.
|
|
|
|
- **Given** two binary files and one file above `[review] max_full_file_chars`
|
|
- **When** werkrevisor collects the full files
|
|
- **Then** one reason names the 2 files that are not UTF-8 text
|
|
- **and** one reason names the file above `[review] max_full_file_chars`
|
|
|
|
##### Verified by
|
|
|
|
- [test_fullfiles.CollectTest.test_different_causes_give_one_reason_each](../../tests/test_fullfiles.py)
|
|
- [test_fullfiles.CollectTest.test_per_file_and_total_limits](../../tests/test_fullfiles.py)
|
|
|
|
#### Scenario#5.13: other reasons about many files show at most 3 examples
|
|
|
|
So that every reason stays one line.
|
|
|
|
- **Given** a PR that changes 5 risky files
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the reason lists 3 files and `and 2 more`
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardYellowTest.test_long_lists_show_three_examples](../../tests/test_rules.py)
|
|
- [test_classifier.JudgeUnitsTest.test_oversized_unit_is_incomplete_and_not_sent](../../tests/test_classifier.py)
|
|
|
|
### Feature: documentation only as diff
|
|
|
|
#### Scenario#5.14: documentation is not sent in full
|
|
|
|
So that full docs do not crowd out code files.
|
|
|
|
- **Given** a PR that changes a file of the `docs` category and a code file
|
|
- **When** werkrevisor collects the full files
|
|
- **Then** only the code file is in the full-file section
|
|
- **and** the doc file does not count against the full-file limits
|
|
|
|
##### Verified by
|
|
|
|
- [test_fullfiles.CollectTest.test_docs_are_sent_only_as_diff](../../tests/test_fullfiles.py)
|
|
- [test_fullfiles.FullFilesFlowTest.test_docs_reach_the_model_only_as_diff](../../tests/test_fullfiles.py)
|
|
|
|
#### Scenario#5.15: the model judges documentation against the writing style
|
|
|
|
So that bloated or mannered prose becomes a finding.
|
|
|
|
- **Given** the example configs `<config>`
|
|
- **When** werkrevisor builds the review instructions
|
|
- **Then** they tell the model to judge documentation against the writing style
|
|
and to report bloated or mannered prose
|
|
|
|
| config |
|
|
|---|
|
|
| `config/werkprobe.toml` |
|
|
| `config/hsadmin-ng.toml` |
|
|
| `config/werkrevisor.toml` with `.werkrevisor.toml` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_budget_redact.ConfigTest.test_examples_tell_the_model_to_judge_docs_against_the_writing_style](../../tests/test_config_budget_redact.py)
|
|
|
|
## The Solution
|
|
|
|
- Three config layers, a later one wins per key: built-in defaults, host config, `.werkrevisor.toml`.
|
|
- werkrevisor reads `.werkrevisor.toml` through the Gitea raw-file API at the base commit, never at the head.
|
|
A missing file means no repository layer; other read errors are ERROR.
|
|
- The repository layer may set `[size]`, `[categories]`, `[pr_doc]`, most of `[review]`,
|
|
the classifier questions and thresholds, and some limits.
|
|
It adds risky paths and appends review instructions instead of replacing them.
|
|
It can only lower `[model] max_context_tokens`, `max_output_tokens`, `[budget] max_usd_per_review`
|
|
and the full-file limits, and cannot switch off the secret scan.
|
|
Every other key is a host key; setting it is ERROR with the key in the message.
|
|
- After the merge of both layers, every required section must be set, as before for the host config alone.
|
|
- `.werkrevisor.toml` is a built-in risky path, so a change needs a human approval.
|
|
werkrevisor also validates the changed file against the host config; an invalid file is RED,
|
|
because the review reads the base version and would only fail after the merge.
|
|
- The usage line names the config sources; the usage record has `config_source` and `host_config`.
|
|
- `config/werkrevisor.toml` keeps only the host part; `.werkrevisor.toml` holds the review rules of werkrevisor,
|
|
with `repo_context_files = ["AGENTS.md"]`.
|
|
- One reason per cause for files left out of the full-file context, with the count and at most 3 examples.
|
|
The same applies to the classifier context and to the coverage and rule reasons.
|
|
- Files of the `docs` category reach the model only as diff:
|
|
the model reviews them for their wording, and full docs would crowd out code.
|
|
- The review instructions tell the model to judge documentation against the writing style
|
|
from `AGENTS.md` and to report bloated or mannered prose.
|
|
- werkrevisor 0.8.0; the README and the German guides describe the layers.
|
|
|
|
## Open Questions
|
|
|
|
- [x] May the repository layer raise `[size]` limits? Yes: they decide only whether a human must look, not cost or model.
|
|
- [x] Must the host config still be complete? No: the merged config must be complete; a missing section is still ERROR.
|
|
- [x] Does the installed werkrevisor 0.7.0 need a change for this PR? No: it still reads its complete config;
|
|
install 0.8.0 with the reduced `config/werkrevisor.toml` only after this PR is merged.
|
|
|
|
## Additional Changes
|
|
|
|
- `AGENTS.md` for werkrevisor: project, build and test, conventions and the writing style.
|
|
- `GiteaError` carries the HTTP status, so a missing file (404) differs from a failed read.
|
|
|
|
## Follow-up PRs
|
|
|
|
- A `.werkrevisor.toml` for hsadmin-ng and a reduced `config/hsadmin-ng.toml`.
|
|
- `tools/jev-eval` with an optional repository config.
|