werkator build succeeded after 01:14
werkrevisor 0.11.0: exact token count before the model call, and a focused style review. - werkrevisor counts the exact prompt PR-Agent sends, with o200k_base and `[model] token_count_factor` (default 1.15), instead of estimating chars/3.5. - The model no longer reports line length or sentence counts; werkrevisor checks them itself. Details: the PR document under `docs/prs/`. 🤖 Generated with [Claude Code](https://claude.com/claude-code) Reviewed-on: #9 Co-authored-by: claude-code <claude-code@javagil.de>
242 lines
12 KiB
Markdown
242 lines
12 KiB
Markdown
# PR#9: Exact token count and a focused style review
|
|
|
|
> **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.10.0 estimates the input tokens before the model call as characters / 3.5,
|
|
with a fixed prompt overhead and an expansion factor for the diff.
|
|
The estimate decides whether a PR gets an AI review at all: the input limit, the worst-case cost and the YELLOW reason.
|
|
It is much too low:
|
|
|
|
| PR | Estimate | Prompt tokens reported by the model |
|
|
|---|---|---|
|
|
| hsadmin-ng PR#305 | about 168000 | 268735 |
|
|
| werkrevisor PR#7 | below 80000 | 87212 to 88369 |
|
|
|
|
A PR above the limit therefore reaches the model, and a review can cost more than `[budget] max_usd_per_review`.
|
|
|
|
The example configs tell the model to judge documentation against the writing style in `AGENTS.md`.
|
|
The model then reports the countable rules, and it counts wrongly.
|
|
On PR#8 it claimed that a 120-character line and a sentence wrapped after a comma violate the rules,
|
|
although the limit is 128 characters and a wrap after a comma is explicitly allowed.
|
|
werkrevisor 0.9.0 already checks these rules deterministically in `[writing]`.
|
|
|
|
## Non-Goals
|
|
|
|
- No tokenizer per model: the image has only tiktoken with `o200k_base`;
|
|
Anthropic and other providers count differently, so a configurable factor covers the difference.
|
|
- No call to a token-count API of a provider: it would be a network call with the key before the budget check.
|
|
- No change to the deterministic writing rules.
|
|
|
|
## The Scenarios
|
|
|
|
### Feature: the token count decides about the AI review
|
|
|
|
#### Scenario#9.01: werkrevisor counts the prompt that PR-Agent sends
|
|
|
|
So that the input limit applies to the real prompt, not to an estimate.
|
|
|
|
- **Given** PR-Agent has rendered the system and the user prompt, with the diff, the full files and the repository context files
|
|
- **When** PR-Agent is about to call the model
|
|
- **Then** werkrevisor counts both prompts with the tokenizer `o200k_base`
|
|
- **and** multiplies the count with `[model] token_count_factor`, rounded up
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.TokenCountTest.test_count_uses_the_tokenizer_and_the_factor](../../tests/test_token_count.py)
|
|
- [test_token_count.TokenCountTest.test_real_o200k_base_tokenizer_loads_offline](../../tests/test_token_count.py)
|
|
- [test_pr_agent_run.PrAgentRunTest.test_admit_sees_the_exact_prompt_before_the_model_call](../../tests/test_pr_agent_run.py)
|
|
|
|
#### Scenario#9.02: a prompt above the input limit gets no AI review
|
|
|
|
So that the model never gets a prompt that it reviews badly or that costs more than the budget.
|
|
|
|
- **Given** the effective input limit is `<limit>`
|
|
- **When** the counted prompt has more tokens than the limit
|
|
- **Then** werkrevisor does not call the model
|
|
- **and** it does not query the key budget
|
|
- **and** the verdict is YELLOW with a reason `too_large_tokens` that names the count, the factor and `<limit>`
|
|
|
|
| limit |
|
|
|---|
|
|
| `[model] max_context_tokens = 1000` |
|
|
| `the limit of 250 derived from [budget] max_usd_per_review = 0.081` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.TokenCountFlowTest.test_limit_is_decided_on_the_count_times_the_factor](../../tests/test_token_count.py)
|
|
- [test_flow.BudgetFlowTest.test_too_long_for_the_model_is_yellow_without_llm_call](../../tests/test_flow.py)
|
|
- [test_flow.BudgetFlowTest.test_too_expensive_is_yellow_without_llm_call](../../tests/test_flow.py)
|
|
- [test_pr_agent_run.PrAgentRunTest.test_too_long_prompt_never_reaches_the_model](../../tests/test_pr_agent_run.py)
|
|
|
|
#### Scenario#9.03: a prompt that PR-Agent had to shorten gets no AI review
|
|
|
|
So that a PR far above the limit is YELLOW, not an ERROR of a clipped review.
|
|
|
|
- **Given** the prompt does not fit PR-Agent's window of twice the input limit plus `[model] max_output_tokens`
|
|
- **When** PR-Agent prunes the diff or refuses the prompt, in the first or in a later attempt
|
|
- **Then** werkrevisor does not call the model with this prompt
|
|
- **and** the verdict is YELLOW with a reason `too_large_tokens` that names the window
|
|
- **and** an error that PR-Agent raises after a model call of the same attempt stays an ERROR, e.g. an invalid answer
|
|
|
|
##### Verified by
|
|
|
|
- [test_pr_agent_run.PrAgentRunTest.test_pruned_prompt_never_reaches_the_model](../../tests/test_pr_agent_run.py)
|
|
- [test_pr_agent_run.PrAgentRunTest.test_prompt_refused_by_pr_agent_never_reaches_the_model](../../tests/test_pr_agent_run.py)
|
|
- [test_pr_agent_run.RefusalTrackingTest.test_refusal_after_an_earlier_call_is_still_a_refusal](../../tests/test_pr_agent_run.py)
|
|
- [test_pr_agent_run.RefusalTrackingTest.test_eligible_error_after_a_call_stays_a_model_error](../../tests/test_pr_agent_run.py)
|
|
|
|
#### Scenario#9.04: without the tokenizer werkrevisor falls back to the estimate and says so
|
|
|
|
So that a broken image does not hide that the decision rests on an estimate.
|
|
|
|
- **Given** the tokenizer `o200k_base` cannot load
|
|
- **When** werkrevisor decides about the model call
|
|
- **Then** it uses the estimate of 0.10.0
|
|
- **and** the reason and the comment say `token count estimated`
|
|
- **and** the usage record has `token_count_method = "estimated"`
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.TokenCountTest.test_fallback_to_the_estimate_is_flagged](../../tests/test_token_count.py)
|
|
- [test_token_count.TokenCountFlowTest.test_estimated_count_is_named_in_reason_comment_and_usage](../../tests/test_token_count.py)
|
|
|
|
#### Scenario#9.05: the usage record holds the count and the reported prompt tokens
|
|
|
|
So that the operator can calibrate `[model] token_count_factor` from real reviews.
|
|
|
|
- **Given** a review with a model call
|
|
- **When** werkrevisor writes the usage record
|
|
- **Then** it has `counted_input_tokens`, `counted_raw_tokens`, `token_count_factor`, `token_count_method` and `worst_case_usd`
|
|
- **and** `prompt_tokens` as the model reported it
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.TokenCountFlowTest.test_usage_record_has_count_and_reported_prompt_tokens](../../tests/test_token_count.py)
|
|
|
|
#### Scenario#9.06: only the host sets `[model] token_count_factor`, and it is positive
|
|
|
|
So that a PR cannot lower the count of its own review.
|
|
|
|
- **Given** `<config>`
|
|
- **When** werkrevisor builds the config
|
|
- **Then** it refuses the config with `<message>`
|
|
|
|
| config | message |
|
|
|---|---|
|
|
| host `token_count_factor = 0` | `[model] token_count_factor must be a positive number` |
|
|
| host `token_count_factor = true` | `[model] token_count_factor must be a positive number` |
|
|
| `.werkrevisor.toml` with `token_count_factor = 0.5` | `must not set [model] token_count_factor: only the host config sets it` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.TokenCountConfigTest.test_factor_must_be_positive](../../tests/test_token_count.py)
|
|
- [test_token_count.TokenCountConfigTest.test_factor_is_host_only](../../tests/test_token_count.py)
|
|
|
|
#### Scenario#9.08: a retry of the model call uses the decision of the first call
|
|
|
|
So that a retry never stops a review whose first call already cost money, because of a second key query.
|
|
|
|
- **Given** werkrevisor admitted the first model call of a review
|
|
- **When** PR-Agent calls the model again with a prompt of `<size>`
|
|
- **Then** werkrevisor does not query the key budget again
|
|
- **and** `<result>`
|
|
|
|
| size | result |
|
|
|---|---|
|
|
| the same or fewer tokens | the call goes ahead; `budget_remaining_before_usd` stays the value of the first query |
|
|
| more tokens, above the input limit | the review stops with YELLOW `too_large_tokens` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.RetryTest.test_retry_with_the_same_prompt_queries_the_key_only_once](../../tests/test_token_count.py)
|
|
- [test_token_count.RetryTest.test_retry_with_a_smaller_prompt_keeps_the_first_decision](../../tests/test_token_count.py)
|
|
- [test_token_count.RetryTest.test_retry_with_a_grown_prompt_above_the_limit_stops_and_keeps_the_usage](../../tests/test_token_count.py)
|
|
|
|
#### Scenario#9.09: a stop after an earlier model call keeps its usage
|
|
|
|
So that the comment and the usage record never hide the cost of a call that happened.
|
|
|
|
- **Given** a review stops before a model call
|
|
- **When** an earlier model call of this review `<happened>`
|
|
- **Then** the usage shows `<usage>`
|
|
|
|
| happened | usage |
|
|
|---|---|
|
|
| happened | the reported calls, tokens and cost, or `cost unknown` |
|
|
| did not happen | `no model call, cost 0 USD` |
|
|
|
|
##### Verified by
|
|
|
|
- [test_token_count.RetryTest.test_retry_with_a_grown_prompt_above_the_limit_stops_and_keeps_the_usage](../../tests/test_token_count.py)
|
|
- [test_token_count.RetryTest.test_stop_without_an_earlier_call_costs_nothing](../../tests/test_token_count.py)
|
|
- [test_pr_agent_run.PrAgentRunTest.test_stop_carries_the_run_details_of_pr_agent](../../tests/test_pr_agent_run.py)
|
|
|
|
### Feature: a focused style review
|
|
|
|
#### Scenario#9.07: the model does not report the countable writing rules
|
|
|
|
So that the model reports only what werkrevisor cannot count.
|
|
|
|
- **Given** the example configs and `.werkrevisor.toml`
|
|
- **When** werkrevisor builds the review instructions
|
|
- **Then** they tell the model not to report line length, sentences per line and sentences per paragraph
|
|
- **and** to report bloated or mannered prose, unclear statements and contradictions between the PR document and the code
|
|
|
|
##### Verified by
|
|
|
|
- [test_config_budget_redact.ConfigTest.test_examples_leave_the_countable_writing_rules_to_werkrevisor](../../tests/test_config_budget_redact.py)
|
|
|
|
## The Solution
|
|
|
|
- werkrevisor wraps the AI handler of PR-Agent.
|
|
Before each model call, it counts the system and the user prompt that PR-Agent passes to the handler,
|
|
with tiktoken `o200k_base` from litellm's bundled tokenizer data.
|
|
The count times `[model] token_count_factor` decides about the input limit and the worst-case cost.
|
|
Only then werkrevisor queries the key budget.
|
|
- New host key `[model] token_count_factor`, default 1.15,
|
|
because Anthropic and other tokenizers count more tokens than `o200k_base`.
|
|
- PR-Agent gets a window of twice the input limit plus `max_output_tokens`, unless `[model] context_tokens` is set.
|
|
Thus a prompt reaches the count unshortened, and the reason names the exact count.
|
|
A prompt that PR-Agent still has to prune or refuse is YELLOW, also in a later attempt;
|
|
the usage of an earlier call stays, see Scenario#9.09.
|
|
- If the tokenizer cannot load, werkrevisor uses the estimate of 0.10.0 and names it `token count estimated`.
|
|
`[budget] chars_per_token`, `prompt_overhead_chars` and `diff_expansion_factor` apply only to this fallback.
|
|
- The usage record replaces `estimated_input_tokens` and `estimated_worst_case_usd` with the fields of Scenario#9.05.
|
|
- The review instructions in `.werkrevisor.toml`, `config/werkprobe.toml` and `config/hsadmin-ng.toml`
|
|
leave the countable writing rules to werkrevisor.
|
|
`.werkrevisor.toml` is a risky path, and the review reads it from the base commit,
|
|
so the new instructions apply to the PRs of werkrevisor only after the merge of this PR.
|
|
- werkrevisor 0.11.0; the README and `docs/betrieb-git.javagil.de.md` describe the change.
|
|
|
|
## Open Questions
|
|
|
|
- [x] Does werkrevisor count before PR-Agent builds the prompt?
|
|
No: only PR-Agent knows its rendered templates, the diff with context lines and the repository context files.
|
|
werkrevisor counts inside the run, in the AI handler, before the model call; nothing reaches the model without the count.
|
|
- [x] Does a dry run without `--llm` still report a token count?
|
|
No: without `--llm` werkrevisor starts no PR-Agent run, so there is no prompt to count; the usage fields are empty.
|
|
`tools/dry-run … --llm` reports the count.
|
|
- [x] Does the key budget query still come before the token check?
|
|
No: the count comes first, so a too long PR is YELLOW without a query of the key.
|
|
|
|
## Additional Changes
|
|
|
|
- The test of the version skew reason uses the installed version instead of a fixed `0.10.0`.
|
|
- A prompt that PR-Agent prunes is YELLOW now; before, it was an ERROR `clipped` after the model call.
|
|
A pruned prompt never reaches the model, so the ERROR reasons `clipped` and `PR-Agent left out files` are removed.
|
|
A changed file missing in the prompt still gives ERROR `skipped_files`.
|
|
- The coverage check reported a file as unreadable
|
|
when the text `This file could not be read.` appeared anywhere in its diff section,
|
|
also in a diff line or in the appended full files.
|
|
The self-review of this PR got a false ERROR for `werkrevisor/review.py` and this document,
|
|
because `review.py` contains that text.
|
|
Now only the notice line that PR-Agent writes directly below the `## File:` header counts.
|
|
|
|
## Follow-up PRs
|
|
|
|
- Calibrate `[model] token_count_factor` from the usage records: `prompt_tokens` / `counted_raw_tokens` of real reviews.
|