werkrevisor 0.20.6: the usage record never ends a review, and the statistics bound the cost sums #37
No Assignees
Notifications
Due Date
No due date set.
Dependencies
No dependencies set.
Reference: mi/werkrevisor#37
Reference in New Issue
Block a user
See the PR doc.
🤖 Generated with Claude Code
🟢 werkrevisor GREEN for
085823a, superseded by the result for33e46ad🟢 werkrevisor: GREEN / PASS
Gate: approved by werkrevisor; merge waits for 2 open hint(s): reply to each line comment and resolve it, then restart the review or push a commit.
Change kind:
mixed.Reasons
Findings
[medium] Stats report skipped when the record cannot be built at
werkrevisor/cli.py:710-714, hint, as a line comment[low] Unclear sum bound for the Jev costs at
docs/prs/2026-10-06-PR#37-usage-record-never-ends-the-review.md:63, hint, as a line commentClassifier
6 of 6 units passed all questions (1 PR document(s)).
Usage: model `openrouter/anthropic/claude-sonnet-5.5`, LLM done, tokens counted before the call 65420, calls 1, tokens in 65662 / out 438 (reasoning included in out), duration 19.8 s, cost 0.1357 USD (reported), classifier `jev-1.13.0` calls 6, tokens in 4698, cost 0.0002 USD, finding judge `jev-1.13.0` calls 1, tokens in 1402, cost 0.0001 USD, build file judge `jev-1.13.0` calls 1, tokens in 560, cost 0.0000 USD, config: defaults + host + repository (.werkrevisor.toml at base1 answer ignored because of a confidence below [classifier] min_confidence.
227e5d8).Reviewed head `085823ad44e50cc8238fc2695e5e67ed4efb009e` against base `227e5d89285d1fe30ea46a99712bb7932b4325e0`, werkrevisor 0.20.5.
werkrevisor: GREEN / PASS for
085823ad44e50cc8238fc2695e5e67ed4efb009e.The result comment of werkrevisor holds the details.
@@ -0,0 +60,4 @@- werkrevisor writes the usage record inside a guard: an error goes to stderr,and the review keeps its verdict and exit code.- The statistics service checks the LLM cost and the Jev cost of a record against the bound after the sum.[low] Unclear sum bound for the Jev costs (werkrevisor)
hint
The solution says the service checks costs "against the bound after the sum". The code applies the bound to the LLM sum only when
cost_usdis present, so with an unknowncost_usdtheexcerpt_cost_usdis not summed or bounded. The document does not state this.Clarified in
33e46ad: the doc names both sums, and that withoutcost_usdthe review cost stays unknown, soexcerpt_cost_usdis only bounded on its own.@@ -709,2 +709,2 @@record = report.usage_record(result, pr, _now())report.write_usage(record, redactor, deps.out, deps.usage_path)# The review is done and published; its statistics must not change its exit code.try:[medium] Stats report skipped when the record cannot be built (werkrevisor)
hint; Jev: real 0.43, severity 2.1 of 3
If
report.usage_recordraises,executereturns before the stats call, so nothing is reported. This path is intended and the PR document does not describe it. No test covers a failingusage_record. The document and README only say a failed write keeps the result.Fixed in
33e46ad:test_a_usage_record_that_cannot_be_built_keeps_the_resultcovers it (red without the guard), and the PR doc states that such a record is neither written nor reported: there is nothing to report.New commits pushed, approval review dismissed automatically according to repository settings
🟢 werkrevisor GREEN for
33e46ad, superseded by the result fore3078d1🟢 werkrevisor: GREEN / PASS
Gate: approved by werkrevisor; merge waits for 2 open hint(s): reply to each line comment and resolve it, then restart the review or push a commit.
Change kind:
mixed.Reasons
Findings
[medium] Usage record built-failure skips stats report at
werkrevisor/cli.py:709-721, hint, as a line comment[low] excerpt_cost_usd is not validated without cost_usd at
statistics/src/main/kotlin/de/hoennig/werkrevisor/statistics/UsageApiController.kt:196-199, hint, as a line commentClassifier
6 of 6 units passed all questions (1 PR document(s)).
Usage: model `openrouter/anthropic/claude-sonnet-5.5`, LLM done, tokens counted before the call 65742, calls 1, tokens in 66048 / out 561 (reasoning included in out), duration 11.4 s, cost 0.1377 USD (reported), classifier `jev-1.13.0` calls 6, tokens in 4760, cost 0.0002 USD, finding judge `jev-1.13.0` calls 1, tokens in 1599, cost 0.0001 USD, build file judge `jev-1.13.0` calls 1, tokens in 560, cost 0.0000 USD, config: defaults + host + repository (.werkrevisor.toml at base227e5d8).Reviewed head `33e46ad7e01eb5d744ddc9638e9281cdaecb1384` against base `227e5d89285d1fe30ea46a99712bb7932b4325e0`, werkrevisor 0.20.5.
werkrevisor: GREEN / PASS for
33e46ad7e01eb5d744ddc9638e9281cdaecb1384.The result comment of werkrevisor holds the details.
@@ -184,3 +194,3 @@local = local,// The excerpt selection is an LLM call via OpenRouter too; an unknown review cost stays unknown.costUsd = body.number("cost_usd")?.plus(body.number("excerpt_cost_usd") ?: 0.0),costUsd =[low] excerpt_cost_usd is not validated without cost_usd (werkrevisor)
hint
When
cost_usdis absent,body.number("excerpt_cost_usd")is never called, so an invalidexcerpt_cost_usd(a string or a negative value) is accepted. Before the PR the same happened, because?.plusshort-circuited the call, so this is not a regression. The PR doc states it ("only bounded on its own"), but the claim is misleading, because it is then not bounded at all.Fixed in
e3078d1:excerpt_cost_usdis read and checked also withoutcost_usd; the Scenario#37.02 test sends a negative, a string and a too large value withcost_usdnull, red without the fix. The doc no longer claims a bound that did not exist.@@ -708,3 +708,2 @@file=deps.out, flush=True)record = report.usage_record(result, pr, _now())report.write_usage(record, redactor, deps.out, deps.usage_path)# The review is done and published; its statistics must not change its exit code.[medium] Usage record built-failure skips stats report (werkrevisor)
hint; Jev: real 0.61, severity 2.2 of 3
If
report.usage_recordraises,executereturns beforedeps.statsis reached, so nothing is reported to the statistics service. This matches the PR doc. However, the stderr line is not guarded against a failingredactor, which could still raise. The main residual risk is that a failing stats write cannot raise, but aprintto a closed stderr could. This is a narrow case, but the guard's goal is that the usage record never changes the exit code.Not changed: the same
redactorandprintto stderr run in every other log line of the run, so a failing one would have ended the review long before; guarding this one line would protect nothing.Answer to the review of
085823a(GREEN, merge held for 2 hints):werkrevisor/cli.py:710-714(no report when the record cannot be built): test and doc in33e46ad, resolved.33e46ad, resolved.statistics/build.gradle.kts: only the version bump.Answer to the review of
33e46ad(GREEN, merge held for 2 hints):UsageApiController.kt:196-199(excerpt_cost_usdunchecked withoutcost_usd): fixed ine3078d1, resolved.werkrevisor/cli.py:709-721(unguarded log line): not changed, reasons in the thread, resolved.statistics/build.gradle.kts: only the version bump.New commits pushed, approval review dismissed automatically according to repository settings
🟢 werkrevisor GREEN for
e3078d1, superseded by the result forc875881🟢 werkrevisor: GREEN / PASS
Gate: approved by werkrevisor; merge waits for 2 open hint(s): reply to each line comment and resolve it, then restart the review or push a commit.
Change kind:
mixed.Reasons
Findings
[medium] Failed usage write may leave stdout record unflagged at
werkrevisor/cli.py:715-718, hint, as a line comment[low] Scenario 37.01 does not match the second test at
docs/prs/2026-10-06-PR#37-usage-record-never-ends-the-review.md:27-39, hint, as a line commentClassifier
6 of 6 units passed all questions (1 PR document(s)).
Usage: model `openrouter/anthropic/claude-sonnet-5.5`, LLM done, tokens counted before the call 66528, calls 1, tokens in 66839 / out 464 (reasoning included in out), duration 10.9 s, cost 0.1383 USD (reported), classifier `jev-1.13.0` calls 6, tokens in 4767, cost 0.0002 USD, finding judge `jev-1.13.0` calls 1, tokens in 1431, cost 0.0001 USD, build file judge `jev-1.13.0` calls 1, tokens in 560, cost 0.0000 USD, config: defaults + host + repository (.werkrevisor.toml at base1 answer ignored because of a confidence below [classifier] min_confidence.
227e5d8).Reviewed head `e3078d1cd6d487735767040a842068a7a03f239b` against base `227e5d89285d1fe30ea46a99712bb7932b4325e0`, werkrevisor 0.20.5.
werkrevisor: GREEN / PASS for
e3078d1cd6d487735767040a842068a7a03f239b.The result comment of werkrevisor holds the details.
@@ -0,0 +24,4 @@### Feature: the usage record never ends a review#### Scenario#37.01: a failed write of the usage record keeps the result of the review[low] Scenario 37.01 does not match the second test (werkrevisor)
hint
Scenario#37.01 only covers a failed write. The second "Verified by" test covers a record that cannot be built, where the record is neither written nor reported. This contradicts the "Then" clause "still reports the usage record to the statistics service". The scenario should state the build-failure case separately.
Fixed in
c875881: the record that cannot be built has its own Scenario#37.02; the cost bound is now Scenario#37.03.@@ -711,0 +712,4 @@except Exception as e:print(redactor(f"werkrevisor: usage record not built: {type(e).__name__}: {e}"), file=sys.stderr)return outcometry:[medium] Failed usage write may leave stdout record unflagged (werkrevisor)
hint; Jev: real 0.44, severity 2.0 of 3
write_usagewrites todeps.outand then to the file. If the file write fails, the stdout line is already printed, but the PR document says a record "that cannot be written" is only logged. This is fine for the exit code. However, the test for the failed write does not check that stderr gets the message, so the logging path is untested.Fixed in
c875881: the failed-write test checks the stderr line. The stdout line before the failed file write is intended: Werkator keeps it in the build log, so the record is not lost.New commits pushed, approval review dismissed automatically according to repository settings
Answer to the review of
e3078d1(GREEN, merge held for 2 hints):werkrevisor/cli.py:715-718(stderr not tested): fixed inc875881, resolved.c875881, resolved.statistics/build.gradle.kts: only the version bump.🟢 werkrevisor GREEN for
c875881, superseded by the result forf00192c🟢 werkrevisor: GREEN / PASS
Gate: approved by werkrevisor; merge waits for 2 open hint(s): reply to each line comment and resolve it, then restart the review or push a commit.
Change kind:
mixed.Reasons
Findings
[medium] Scenario#37.02 contradicts the code at
docs/prs/2026-10-06-PR#37-usage-record-never-ends-the-review.md:44-47, hint, as a line comment[low] Jev cost sum is not bounded per record in the total at
statistics/src/main/kotlin/de/hoennig/werkrevisor/statistics/UsageApiController.kt:198-208, hint, as a line commentClassifier
7 of 7 units passed all questions (1 PR document(s)).
Usage: model `openrouter/anthropic/claude-sonnet-5.5`, LLM done, tokens counted before the call 66877, calls 1, tokens in 67190 / out 523 (reasoning included in out), duration 11.7 s, cost 0.1396 USD (reported), classifier `jev-1.13.0` calls 7, tokens in 5698, cost 0.0002 USD, finding judge `jev-1.13.0` calls 1, tokens in 1382, cost 0.0001 USD, build file judge `jev-1.13.0` calls 1, tokens in 560, cost 0.0000 USD, config: defaults + host + repository (.werkrevisor.toml at base1 answer ignored because of a confidence below [classifier] min_confidence.
227e5d8).Reviewed head `c875881cdf362c3726d52f7b8882bf390030266b` against base `227e5d89285d1fe30ea46a99712bb7932b4325e0`, werkrevisor 0.20.5.
werkrevisor: GREEN / PASS for
c875881cdf362c3726d52f7b8882bf390030266b.The result comment of werkrevisor holds the details.
@@ -0,0 +41,4 @@So that a defect in the statistics code does not fail the gate check of a finished review.- **Given** the usage record of a run cannot be built[medium] Scenario#37.02 contradicts the code (werkrevisor)
hint; Jev: real 0.68, severity 2.1 of 3
Scenario#37.02 says that werkrevisor neither writes nor reports a usage record when the record cannot be built, and the Solution section repeats it. The code matches this, because it returns early on a build error. The README, however, says only that a failed write is logged. A build failure also skips the stats report, which the README does not mention.
Fixed in
f00192c: the README says that a record that cannot be built is not reported either.@@ -184,3 +196,3 @@local = local,// The excerpt selection is an LLM call via OpenRouter too; an unknown review cost stays unknown.costUsd = body.number("cost_usd")?.plus(body.number("excerpt_cost_usd") ?: 0.0),costUsd = body.number("cost_usd")?.let { boundedSum("cost_usd and excerpt_cost_usd", listOf(it, excerptCost)) },[low] Jev cost sum is not bounded per record in the total (werkrevisor)
hint
The bound applies to the LLM sum (
cost_usdplusexcerpt_cost_usd) and to the Jev sum separately. The stored review cost can still reach 200 USD (100 USD each). The PR document states that the sums are bounded, but not the combined total, so the wording "refuses costs above 100 USD, also as sum" in the README can mislead.Clarified in
f00192c: the README names the two bounds, the LLM cost and the Jev cost, each also as sum of its parts. Bounding their total would change the meaning of the bound; both stay far above any real review.Answer to the review of
c875881(GREEN, merge held for 2 hints):f00192c, resolved.f00192c, resolved.statistics/build.gradle.kts: only the version bump.New commits pushed, approval review dismissed automatically according to repository settings
🟢 werkrevisor: GREEN / PASS
Gate: approved by werkrevisor; merge waits for 2 open hint(s): reply to each line comment and resolve it, then restart the review or push a commit.
Change kind:
mixed.Reasons
Findings
[medium] Usage record write error skips stats reporting? No: build failure skips it; stdout write errors not covered by test at
werkrevisor/cli.py:715-718, hint, as a line comment[low] Solution section narrates the implementation at
docs/prs/2026-10-06-PR#37-usage-record-never-ends-the-review.md:74-80, hint, as a line commentClassifier
7 of 7 units passed all questions (1 PR document(s)).
Usage: model `openrouter/anthropic/claude-sonnet-5.5`, LLM done, tokens counted before the call 67010, calls 1, tokens in 67301 / out 485 (reasoning included in out), duration 13.4 s, cost 0.1395 USD (reported), classifier `jev-1.13.0` calls 7, tokens in 5698, cost 0.0002 USD, finding judge `jev-1.13.0` calls 1, tokens in 1457, cost 0.0001 USD, build file judge `jev-1.13.0` calls 1, tokens in 560, cost 0.0000 USD, config: defaults + host + repository (.werkrevisor.toml at base2 answers ignored because of a confidence below [classifier] min_confidence.
227e5d8).Reviewed head `f00192c658cb34520b470f674b3a0174e86e8c1b` against base `227e5d89285d1fe30ea46a99712bb7932b4325e0`, werkrevisor 0.20.5.
werkrevisor: GREEN / PASS for
f00192c658cb34520b470f674b3a0174e86e8c1b.The result comment of werkrevisor holds the details.
@@ -0,0 +71,4 @@## The Solution- werkrevisor builds and writes the usage record inside a guard: an error goes to stderr,[low] Solution section narrates the implementation (werkrevisor)
hint
The "The Solution" bullets restate the code behavior in detail (which sums are checked, "which it was not before"). This narrates the implementation and duplicates the scenarios. Shorten it to one sentence per change.
Not changed: the bullets name the two decisions a reader needs, that the guard keeps the exit code and which sums are bounded; the earlier review rounds asked for exactly this precision.
@@ -711,0 +712,4 @@except Exception as e:print(redactor(f"werkrevisor: usage record not built: {type(e).__name__}: {e}"), file=sys.stderr)return outcometry:[medium] Usage record write error skips stats reporting? No: build failure skips it; stdout write errors not covered by test (werkrevisor)
hint; Jev: real 0.20, severity 2.0 of 3
If
write_usagefails after printing to stdout, the failure is logged and the record is still reported, which matches the PR document. However,write_usageis not atomic: a partial write (stdout printed, file failed) is not distinguishable from a full failure. The scenario text "usage file cannot be written" is only tested for the file path, not for a brokendeps.out. This is a minor gap, not a defect.Not changed: a broken stdout fails in the same guard as a broken file and is logged the same way; the test of the file path covers that guard. A partial write needs no distinction, since nothing reads the result back.
Answer to the review of
f00192c(GREEN, merge held for 2 hints): both hints answered in their threads without a change and resolved. Restarting the review to release the merge.werkrevisor, review restarted for
f00192c658cb34520b470f674b3a0174e86e8c1b: approved by werkrevisor; merge scheduled for when the checks succeed.