Skip to content

Review through Converse; default to Sonnet 5 by measurement - #6

Merged
bbertucc merged 2 commits into
mainfrom
review-models
Sep 24, 2026
Merged

bbertucc merged 2 commits into
mainfrom
review-models

Conversation

@bbertucc

Copy link
Copy Markdown
Member

Chooses the review model by measurement instead of by vendor.

  • Transport. review on Bedrock now uses the Converse API. It takes images and tools the same way for every vendor, so --model accepts any Bedrock model that reads images and calls tools. toConverse and fromConverse map to and from the Messages shape, so Send and the retry logic don't change.
  • Default: Sonnet 5, replacing Opus 5.5.
    • It tied the leaders on catching seeded defects, finding the tagger's real problems, and false positives.
    • It has the lowest published price among them: $1.93 per 100 pages, against $2.62 for Opus 5.5 and $3.31 for Kimi K3.
  • Budget option: GPT-5.6 luna, at $0.13 per 100 pages. It caught 28 of 30 defects.
  • Cost. cost() now knows AWS's rates for luna, Kimi K3 and Mistral Large 3. The 10% regional uplift applies to Claude only.
  • Docs. docs/models.md has the full table and the method: 26 models from 10 vendors screened, three draws for the leaders.
    • The GPT-5.5, GPT-5.6 sol and GPT-6 models tie on quality, but AWS publishes no price for them, so they aren't recommended yet.

While measuring, the reviewers flagged real tagger bugs on real pages. I'll handle these separately:

  • stray OCR fragments tagged as paragraphs;
  • TD Headers pointing at a header ID that doesn't exist, where the source has an empty <th>;
  • a footnote back-link with no text;
  • a title tagged twice.

Tests: 114 pass. Live check: review on Bedrock with Sonnet 5 (default) and with --model us.openai.gpt-5.6-luna.

🤖 Generated with Claude Code

…ment

Converse takes images and tools the same way for every vendor, so --model
accepts any Bedrock model. Measured 26 models from 10 vendors on seeded and
real pages (docs/models.md): Sonnet 5 ties the leaders at the lowest published
price; GPT-5.6 luna is the budget choice at about a fifteenth of the cost.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All five checks pass (114 tests, 11 skipped only because veraPDF is absent). The Converse mapping is sound: stopReason values (tool_use, max_tokens) line up with what reported() and findings() expect, usage.inputTokens/outputTokens map to the Messages names, blob bytes is base64 as --cli-input-json requires, and moving --model-id out of argv into the JSON file removes it from the command line. No blocking issue found. Nothing here touches the tagger, so the output PDF is unaffected.

Non-blocking notes

1. docs/models.md contradicts PRICES on the budget model's cost. The table recommends GPT-5.6 luna at $0.13 per 100 pages, but the rate the code will actually use is

[/gpt-5\.6-luna/, 0.264, 1.584],

and the same document says luna uses "about 2,000 [input] and 1,000 [output] tokens a page". Output alone is then 1000 × 1.584 / 1e6 = $0.00158 a page, i.e. $0.158 per 100 pages before any input tokens; with input it is about $0.21. So $0.13 cannot hold given the other two figures — one of the three is wrong. It matters beyond the doc because cost() feeds estimatedCostUsd, which the CLI prints (src/cli.ts:83) and writes to --report. Sonnet 5's row reconciles ( (4000×2 + 1000×10) × 1.1 / 1e6 = $0.0198 a page ≈ $1.93/100 ), so it is specifically the luna row or its rate that needs rechecking.

2. Stale reason for tool_choice. src/review/review.ts:155:

// tool_choice stays auto: Opus 5.5 rejects a forced tool.

and the same claim at test/review.test.ts:34. Opus 5.5 is no longer the default, and --model now accepts any Bedrock model, so the stated reason no longer explains the current configuration. Either restate it for Sonnet 5 / arbitrary vendors or drop it.

3. The new CLI invocation is untested. The tests cover toConverse/fromConverse directly and stub send, so nothing exercises bedrock() itself (src/review/review.ts:172-185) — the converse subcommand, the file:// prefix (a stray fileb:// would silently base64 the whole request), or reading the reply from stdout instead of an out-file. A test that stubs aws on PATH and asserts the argv plus the parsed reply would catch a regression in the part of this PR that live-testing was the only check on.

4. The second ask may be unusable on Converse when the first reply has no text. src/review/review.ts:113-117 drops tool_use blocks and appends the nudge:

const said = ((res as Reply).content ?? []).filter((c) => c.type === "text" && c.text);
const messages = [...(req.messages as unknown[]), ...(said.length ? [{ role: "assistant", content: said }] : []),
  { role: "user", content: `Answer by calling ${TOOL}.` }];

When said is empty the request carries two consecutive user turns. Converse requires a conversation to alternate roles, so that retry would come back as a validation error rather than a real second chance, and the page fails with a confusing message instead of the did not report findings one. Reachable with a model that returns only a malformed toolUse (wrong tool name, or findings not an array) and no text — which is the failure mode docs/models.md reports for several of the screened vendors. The message-building logic predates this PR; the transport change is what makes the alternation rule apply, so it is worth confirming against Converse before widening --model to more vendors.

Accessibility impact: none on the tagged PDF — this changes only the optional review transport and default model, and Sonnet 5 is documented to catch the same seeded defects as the model it replaces.

… luna's token figures

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

@claude claude Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All five checks pass. The four notes from my review of 0ae8c8a are addressed, and I verified each fix:

  1. luna cost contradiction — fixed, by correcting the measured token averages rather than the rate (docs/models.md:64). The three figures now reconcile: 3800×2 + 1000×10 ×1.1 = $1.936/100 against the table's $1.93, and 1800×0.264 + 500×1.584 = $0.127/100 against $0.13. Opus 5.5's $2.62 implies ~431 output tokens a page, consistent with the previous doc's "Sonnet … writes about twice as many output tokens".
  2. Stale tool_choice reason — fixed (src/review/review.ts:157, test/review.test.ts:37).
  3. Untested CLI invocation — fixed. test/review.test.ts:166-184 stubs aws on PATH, asserts the argv is bedrock-runtime converse --cli-input-json file://… --output json, that the file (not fileb://) holds the Converse request with a real PNG, and that the reply is read from stdout.
  4. Two consecutive user turns on the retry — fixed. src/review/review.ts:126-128:
const messages = said.length ? [first, { role: "assistant", content: said }, { role: "user", content: nudge }]
  : [{ ...first, content: [...first.content, { type: "text", text: nudge }] }];

With no text the nudge joins the first user turn, so the retry stays alternating; toConverse maps the string-valued content of the said branch correctly, and both paths have a test.

No blocking issue found.

Non-blocking notes

1. docs/models.md prices three models that cost() does not know. The table gives per-100-page figures for GPT-5.4 ($1.10), GPT-5.6 terra ($1.03) and Kimi K2.5 ($0.29), but PRICES (src/review/review.ts:37-40) has only gpt-5.6-luna, kimi-k3 and mistral-large-3. --model us.openai.gpt-5.4 therefore prints and reports estimatedCostUsd: null for a model the docs price. That is the documented behaviour for an unknown model, and the PR scopes the added rates deliberately, so it is only a gap between the table and what the tool will estimate.

Accessibility impact: none on the tagged PDF — this changes only the optional review transport and its default model, which docs/models.md measures as catching every seeded defect and the same real tagger problems as the model it replaces.

@bbertucc
bbertucc merged commit 62d7342 into main Sep 24, 2026
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant