werkator build succeeded after 00:05
werkrevisor 0.7.0: clearer reasons and two size limits. - The reason for change kind `mixed` names the file kinds involved and the AI-approvable change kinds. - Every limit-based reason names its config key and value. - Without a model call the comment says `no model call, cost 0 USD`. - General size (`[size]`, docs not counted) → ORANGE, the model still runs; model limit → YELLOW, derived from `[budget] max_usd_per_review` and the new `[model] max_context_tokens` (replaces `max_input_tokens`). Details: the PR document under `docs/prs/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed-on: #4 Co-authored-by: claude-code <claude-code@javagil.de>
220 lines
10 KiB
Markdown
220 lines
10 KiB
Markdown
# PR#4: Clearer reasons and two size limits
|
|
|
|
> **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 first self-review of PR#3 by werkrevisor showed four weaknesses in its comment:
|
|
|
|
1. The reason `change kind 'mixed' is not AI-approvable` did not say which files made the PR mixed,
|
|
nor which change kinds could pass.
|
|
2. A limit-based reason did not say which config key it came from.
|
|
The limit of 300 changed lines was far too low anyway: a PR document alone has 100 to 200 lines.
|
|
3. The usage line said "cost unknown" although no model call was made.
|
|
4. One size limit had two meanings.
|
|
"Too large in general" means that a human must approve the PR knowingly;
|
|
"too large for the model" means that no complete AI review is possible.
|
|
Both gave YELLOW and skipped the model, so a large PR lost the model findings that would help its human reviewer.
|
|
|
|
In addition, `[model] max_input_tokens` had to be kept consistent with the budget by hand.
|
|
|
|
## Non-Goals
|
|
|
|
- The change-kind rule itself does not change; `mixed` stays not AI-approvable.
|
|
- No change to the werkrevisor installation on mih09; `docs/betrieb-git.javagil.de.md` keeps the version that runs there.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: reasons that a developer can act on
|
|
|
|
#### Scenario#4.01: a mixed PR names its file kinds and the AI-approvable change kinds
|
|
|
|
So that the author sees what to split off to get an AI approval.
|
|
|
|
- **Given** a PR that changes main code, a Makefile and a PR document
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** a reason is `change kind 'mixed' (main code, other files, docs); AI-approvable are only: docs only, tests only added, docs and tests, refactoring with unchanged tests`
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.CategoryTest.test_mixed_reason_names_the_file_kinds_and_the_approvable_change_kinds](../../tests/test_rules.py)
|
|
- [test_rules.CategoryTest.test_mixed_reason_names_api_spec_and_tests](../../tests/test_rules.py)
|
|
|
|
#### Scenario#4.02: a limit-based reason names its config key and value
|
|
|
|
So that the operator knows which value to change.
|
|
|
|
- **Given** a PR that exceeds the limit `<limit>`
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the reason contains `<limit>` with its configured value
|
|
|
|
| limit |
|
|
|---|
|
|
| `[size] max_changed_lines` |
|
|
| `[size] max_files` |
|
|
| `[model] max_context_tokens` |
|
|
| `[budget] max_usd_per_review` |
|
|
| `[budget] min_key_remaining_usd` |
|
|
| `[model] max_output_tokens` |
|
|
| `[review] max_full_file_chars` |
|
|
| `[review] max_full_files_chars` |
|
|
| `[review] red_min_findings` |
|
|
| `[classifier] max_state_chars` |
|
|
| `[classifier] max_units` |
|
|
| `[classifier] min_confidence` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardYellowTest.test_size_limits_are_orange_and_keep_the_model](../../tests/test_rules.py)
|
|
- [test_config_budget_redact.BudgetTest.test_precheck_reasons_name_the_limit](../../tests/test_config_budget_redact.py)
|
|
- [test_config_budget_redact.BudgetTest.test_key_check_names_the_deciding_limit](../../tests/test_config_budget_redact.py)
|
|
- [test_flow.VerdictFlowTest.test_truncated_answer_is_error](../../tests/test_flow.py)
|
|
- [test_fullfiles.CollectTest.test_per_file_and_total_limits](../../tests/test_fullfiles.py)
|
|
- [test_verdict.ModelMappingTest.test_threshold_reason_names_the_config_key](../../tests/test_verdict.py)
|
|
- [test_classifier.JudgeUnitsTest.test_max_units](../../tests/test_classifier.py)
|
|
- [test_classifier.MappingTest.test_rate](../../tests/test_classifier.py)
|
|
|
|
### Feature: the usage line states the cost
|
|
|
|
#### Scenario#4.03: a run without a model call costs nothing
|
|
|
|
So that the cost of a skipped review does not look unknown.
|
|
|
|
- **Given** a PR that werkrevisor reviews without a model call
|
|
- **When** werkrevisor writes its comment and usage record
|
|
- **Then** the comment says `no model call, cost 0 USD`
|
|
- **and** the usage record has the cost 0.0 USD with the cost status `none`
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.PublishAndUsageTest.test_run_without_model_call_costs_nothing](../../tests/test_flow.py)
|
|
|
|
#### Scenario#4.04: a model call without usage data has an unknown cost
|
|
|
|
So that "unknown" means a real gap in the data.
|
|
|
|
- **Given** a model call that reports no usage
|
|
- **When** werkrevisor writes its comment
|
|
- **Then** the comment says `cost unknown`
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.PublishAndUsageTest.test_model_call_without_usage_has_unknown_cost](../../tests/test_flow.py)
|
|
|
|
### Feature: two separate size limits
|
|
|
|
#### Background
|
|
|
|
- The operator config of the repository is `config/werkprobe.toml`: 800 changed lines, 30 files, 0.40 USD per review.
|
|
|
|
#### Scenario#4.05: a PR above the general size limit needs a knowing human approval
|
|
|
|
So that a large PR gets a human review and still the model findings.
|
|
|
|
- **Given** a PR with 900 changed lines of main code
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is ORANGE with the reason `PR too large for an AI-only approval: … exceed [size] max_changed_lines = 800; please review it yourself`
|
|
- **and** the model reviews the PR, and its findings appear in the comment
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.SizeFlowTest.test_large_pr_is_orange_and_the_model_still_reviews_it](../../tests/test_flow.py)
|
|
|
|
#### Scenario#4.06: docs do not count for the general size limit
|
|
|
|
So that a long PR document does not make a small change look large.
|
|
|
|
- **Given** a PR with 2000 changed lines in docs and 800 changed lines of main code
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** no size reason appears
|
|
|
|
##### Verified by
|
|
|
|
- [test_rules.HardYellowTest.test_docs_do_not_count_for_the_size_limits](../../tests/test_rules.py)
|
|
|
|
#### Scenario#4.07: a PR too long for the model gets no AI review
|
|
|
|
So that the model never reviews a prompt that is too long for a good review.
|
|
|
|
- **Given** `[model] max_context_tokens` below the estimated input of the PR
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is YELLOW with the reason `no AI review: the PR needs about … input tokens, more than [model] max_context_tokens = …`
|
|
- **and** no model call is made
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.BudgetFlowTest.test_too_long_for_the_model_is_yellow_without_llm_call](../../tests/test_flow.py)
|
|
|
|
#### Scenario#4.08: a PR too expensive for the budget gets no AI review
|
|
|
|
So that a review never costs more than the budget allows.
|
|
|
|
- **Given** a `[budget] max_usd_per_review` that pays for less input than the PR needs
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is YELLOW with the reason `no AI review: … more than the limit of … derived from [budget] max_usd_per_review = …`
|
|
- **and** no model call is made
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.BudgetFlowTest.test_too_expensive_is_yellow_without_llm_call](../../tests/test_flow.py)
|
|
- [test_config_budget_redact.BudgetTest.test_input_limit_is_the_lower_of_budget_and_context](../../tests/test_config_budget_redact.py)
|
|
|
|
#### Scenario#4.09: a PR above both limits shows both reasons
|
|
|
|
So that the human sees both why the PR needs a knowing approval and why the model did not review it.
|
|
|
|
- **Given** a PR above `[size] max_changed_lines` and above the input limit of the model
|
|
- **When** werkrevisor reviews the PR
|
|
- **Then** the verdict is ORANGE
|
|
- **and** both the size reason and the "no AI review" reason appear
|
|
|
|
##### Verified by
|
|
|
|
- [test_flow.SizeFlowTest.test_large_pr_too_long_for_the_model_is_orange_with_both_reasons](../../tests/test_flow.py)
|
|
|
|
#### Scenario#4.10: a config with the old key `max_input_tokens` is refused
|
|
|
|
So that an old config cannot silently keep a limit that no longer exists.
|
|
|
|
- **Given** an operator config with `[model] max_input_tokens`
|
|
- **When** werkrevisor loads it
|
|
- **Then** loading fails with "unknown keys: max_input_tokens"
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_budget_redact.ConfigTest.test_old_max_input_tokens_is_an_unknown_key](../../tests/test_config_budget_redact.py)
|
|
|
|
## The Solution
|
|
|
|
- The reason for a change kind that is not AI-approvable lists the kinds of the changed files
|
|
(main code, API spec, tests, other files, docs) and the configured AI-approvable change kinds in readable words.
|
|
- Every limit-based reason names its config key and value, e.g. `[size] max_changed_lines = 800`.
|
|
- A run without a model call has `cost_usd` 0.0 and the cost status `none`; the comment says `no model call, cost 0 USD`.
|
|
- `[size]` counts only changed lines and files outside the `docs` category.
|
|
Above it, the verdict is ORANGE, and the model still reviews the PR.
|
|
- `[model] max_input_tokens` is renamed to `max_context_tokens`, a quality cap: 150000 for frontier models, 80000 for cheap ones.
|
|
werkrevisor derives a second limit, the largest input whose worst case stays within `[budget] max_usd_per_review`.
|
|
The lower of both is the effective input limit; above it, the verdict is YELLOW without a model call.
|
|
PR-Agent gets the same effective limit.
|
|
A config whose budget does not pay for `max_output_tokens` is refused.
|
|
- New example values:
|
|
- `config/werkprobe.toml` and `config/werkrevisor.toml`: 800 lines, 30 files, 0.40 USD, which gives 80000 input tokens;
|
|
`max_full_files_chars` 120000, so that the diff of an 800-line PR still fits.
|
|
- `config/hsadmin-ng.toml`: 1000 lines, 40 files, 1.00 USD, limited by `max_context_tokens` to 150000 input tokens;
|
|
`max_full_files_chars` 300000 instead of 400000, for the same reason.
|
|
- The general size limits come from the changed lines outside the docs per merged PR in one month:
|
|
Werkator (92 PRs) median 194, p75 489, p90 865, so 800 lines hit about every tenth PR;
|
|
hsadmin-ng (16 PRs) median 441, p75 974, p90 1098, so 1000 lines hit about every fourth PR,
|
|
just below the hard limit of hsadmin-ng's own `tools/pr-quick-check` (about 1200 to 1400 lines).
|
|
- werkrevisor 0.7.0; the README and the German guides describe the two limits.
|
|
|
|
## Open Questions
|
|
|
|
- [x] Is the change-kind rule 'mixed' too strict, so that GREEN almost never happens? Yes, very likely; it is reconsidered in a follow-up PR.
|
|
|
|
## Follow-up PRs
|
|
|
|
- Revise the change-kind rule, so that a typical PR with main code, tests and a PR document can become GREEN.
|