feat(config): cap reviewer effort per model tier by zzwong · Pull Request #552 · open-cli-collective/codereview-cli

zzwong · 2026-08-05T19:14:07Z

Why

Agent catalogs declare an absolute effort that becomes the provider's reasoning-effort setting (--thinking on the Pi RPC adapter). A deployment wanting to bound spend on an expensive tier previously had one lever: edit the shared catalog. That changes the agent author's declared intent for every consumer, including ones who want the deeper review and are happy to pay for it.

cr already separates portable intent from deployment choice for models — an agent declares model_tier, the profile maps tiers to concrete models and sets a reviewer_model_tier baseline. Effort had no equivalent layer.

What

llm.max_effort, a per-tier ceiling that sits next to model_map:

llm_runtimes:
  pi-codex-monit-reviewer:
    model_map:
      large: openai-codex/gpt-5.6-sol
      medium: openai-codex/gpt-5.6-terra
    max_effort:
      large: medium

A tier absent from the map is uncapped. It is a ceiling only — an agent declaring low under a medium ceiling still runs at low.

The clamp lives in stagemodel.ResolveStageModel, which docs/architecture.md designates as the single path from profile preferences to a concrete model and effort, explicitly so "token costs and profile-level tier floors can be added without touching individual review stages."

Notable

A latent trap this surfaced. resolveReviewerRuntimeConfig discarded the resolver's effort and passed agent.Effort straight through. A clamp in the resolver alone would have silently missed reviewer agents — the only path that ever reaches the large tier, and the entire point of the feature. The resolved effort is now threaded through reviewerRuntimeResolution.

Init preservation. There is no init editor for max_effort, so hand-editing config.yml is the only way in. The runtime round trip (initLLMRuntimeDraftFromConfig → exportConfig) rebuilds LLMConfig field by field, so without this the first later cr init touching the runtime would silently discard the ceiling — config parses fine, cap just gone. identityKey now includes the map too, so two runtimes differing only by ceiling no longer collide.

Deliberate bypasses. Four paths ignore the ceiling because each is an explicit selection of a concrete model or effort, and all four are documented in the README:

Path	Why
`--reviewer-effort`	operator override applied after resolution
`--reviewer-model`	`ModelOverride` returns before the clamp
agent `model_id`	no tier to key a tier-scoped ceiling on
`cr benchmark run`	`stages.reviewers.effort` is required so candidates stay comparable

The model_id case is the one structural hole worth knowing about: a catalog pinning both model_id and effort: high is outside a deployment's control entirely.

Testing

go build ./..., golangci-lint run, and go test ./... (2998 passed, 71 packages) all clean.

New coverage: the clamp, uncapped tiers, the never-raise rule, model_id staying uncapped with a ceiling configured (the pre-existing exact-model tests use profiles without MaxEffort, so they would have passed either way), config validation, MinEffort, and the init round trip.

Also exercised end to end against a real PR — a large-tier agent declaring effort: high resolved to gpt-5.6-sol at medium while medium-tier agents were untouched, in a live run that posted to GitHub.

Follow-ups, not in this PR

--reviewer-effort / --reviewer-model vs. the ceiling: escape hatch or absolute? Needs a contract decision, and --selection-effort is currently asymmetric (it feeds into the resolver and is capped).
max_effort support in the cr init wizard.
ModelMapRow JSON parity — text mode shows the ceiling, --json does not — plus extending the architecture guardrail test to cover config.ResolveMaxEffort.
rebaseReviewerCohort's incompatibility message should name the field that drifted; enabling a cap trips it on the first run against a saved cohort.
LLMConfig.normalized() and empty() do not know about MaxEffort; extending normalized() would let ResolveMaxEffort index directly instead of scanning.

Agent catalogs declare an absolute effort that becomes the provider's reasoning-effort setting. A deployment that wants to bound spend on an expensive tier previously had only one lever: editing the shared catalog, which changes the agent's declared intent for every consumer. Add llm.max_effort, a per-tier ceiling resolved alongside model_map: max_effort: large: medium A tier absent from the map is uncapped. The cap is a ceiling only, so an agent declaring low under a medium ceiling still runs at low. The clamp lives in stagemodel.ResolveStageModel, the single documented path from profile preferences to a concrete model and effort, so every stage picks it up without per-stage changes. Reviewer resolution previously discarded the resolver's effort and passed agent.Effort straight through, which would have left reviewers - the only path that reaches the large tier - silently uncapped. The resolved effort is now threaded through reviewerRuntimeResolution. cr init has no editor for max_effort, so the runtime round trip is extended to preserve it: dropping the field would silently discard a hand-written ceiling on any later init pass. identityKey now includes the map so two runtimes differing only by ceiling no longer collide. Four paths intentionally bypass the ceiling because each is an explicit selection of a concrete model or effort: --reviewer-effort, --reviewer-model, agent model_id, and benchmark suites, where stages.reviewers.effort is required so candidates stay comparable. README and docs/architecture.md name all four.

rianjs · 2026-08-10T16:03:46Z

PR #552 — pre-fix TDD assessment

Blocker

No findings.

Major

internal/stagemodel/resolver.go deliberately clamps using the post-floor resolved tier, but the new cap tests only request large directly. None uses a small baseline/agent tier raised to large with only max_effort.large set. Capping the pre-floor tier would pass all tests. Add one resolver test for that matrix; a pipeline dry-run variant is optional.
The data-preservation claim is not exercised through a real config/init lifecycle. The implementation has independent non-interactive and interactive preservation paths, while the new tests call only draft helpers. Removing the non-interactive copy, YAML persistence, or save/load behavior would still pass. Add one real cr init --non-interactive test starting from config YAML containing max_effort, then load the saved file and assert the ceiling remains.

Minor

The documented --reviewer-effort and --reviewer-model escape hatches have no cap-present regression test. Add cap-present assertions that reviewer effort and reviewer model overrides retain their requested effort.
The text config view adds [max effort: medium], but no test asserts it. Add one expectation; JSON row parity is explicitly deferred.

Nit

No over-mocking concern in the new resolver/reviewer tests: they call production resolution functions and assert concrete model/effort values.
The existing static-smoke failure is unrelated: TestPiRPCReviewerHelperStaysInParentProcessGroup timed out once, and no PR file touches that process-group path.

…ceiling

rianjs · 2026-08-10T17:36:06Z

PR #552 post-fix TDD / coverage assessment

Assessed at e27c3d8a612627e03b118836e5edc2d6312cd6fd (feat/max-effort-tier-ceiling).

Verdict: PASS, with one targeted non-blocking test gap

The resolver is the single precedence point, and the focused suite passed:
go test ./internal/config ./internal/modelprefs ./internal/stagemodel ./internal/pipeline ./internal/cmd/initcmd ./internal/cmd/benchmarkcmd.

P2 — worthwhile follow-up

Add one dry-run pipeline case with a configured ceiling, --reviewer-model, and
--reviewer-effort. Assert the reviewer LLM request, ledger session, and
agent-sources.json.reviewer_runtime all report the exact model and uncapped
explicit effort. Current tests prove the pieces separately: generic exact-model
resolution under a cap, agent model_id under a cap, and request/session/artifact
agreement for tier-based reviewer effort overrides. The combined override path is
the small remaining regression seam introduced by the provenance change.

Covered convincingly

Reviewer-only tier floors and final-tier caps: resolver floor/cap tests and
dry-run baseline/tier-override tests exercise the final effective tier.
Explicit effort overrides win: resolver and pipeline tests cover a lower cap
followed by high reviewer/selection effort overrides.
Exact selection/reviewer model and agent model_id paths bypass caps; benchmark
recipes are independently verified to forward those same model/effort flags.
Validation, normalization/deep-copy, runtime identity, inline projection, and
interactive/non-interactive init preservation all have focused tests.
Runtime, sessions, and reviewer provenance agree for capped and explicit-effort
tier resolution; provenance now records resolved_effort.
README and architecture/init docs accurately describe canonical runtime storage,
final-tier scope, override precedence, bypasses, and init's preservation-only
behavior.

Low-value expansion to skip

Do not add a full benchmark subprocess/config-ceiling matrix: benchmark code only
forwards recipe fields to the already-covered review flags, and its argument tests
plus pipeline tests cover that composition. Avoid duplicating all cap combinations
across selection, synthesis, thread analysis, and every reviewer tier.

rianjs-bot

Automated PR Review

Reviewed commit: ba724ecef2a2
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]

Summary

Reviewer	Findings
go:implementation-tests	1
policies:conventions	1
structure:repo-health	1

go:implementation-tests (1 finding)

Major - `internal/stagemodel/resolver.go:97`

Applying EffortOverride after applyMaxEffort makes --selection-effort bypass llm.max_effort. That override is routed through this resolver but is not among the documented bypasses, so a deployment that caps the medium tier at low can still run selection at high (the new test explicitly codifies that escape). Apply the override before clamping, or clamp the override as well; retain the reviewer-specific post-resolution override only if that explicit escape hatch is intended.

policies:conventions (1 finding)

Minor - `docs/architecture.md:61`

The new ceiling is part of the documented single resolver boundary, but the existing architecture guardrail only rejects direct config.ResolveModelTier calls. It does not cover the newly added config.ResolveMaxEffort, so a future runtime stage can apply or bypass the ceiling outside internal/stagemodel without the check catching it. Extend internal/architecture/model_resolution_test.go to reject direct calls to both resolver helpers outside its approved packages.

structure:repo-health (1 finding)

Minor - `internal/view/config.go:270`

max_effort is rendered only as a text suffix; ModelMapRow—used by cr config llm models list --json—has no ceiling field. Programmatic config inspection therefore cannot determine the effective effort constraint for a returned tier. Add a JSON max_effort field to ModelMapRow, populate it from the normalized/effective configuration, and cover both capped and uncapped JSON rows.

Reviewer Coverage

go:implementation-tests — complete (broad); inspected 15 assigned files (18 inspected across reviewers): internal/cmd/initcmd/initcmd.go, internal/cmd/initcmd/initcmd_max_effort_test.go, internal/cmd/initcmd/initcmd_test.go, internal/config/config.go, internal/config/config_max_effort_test.go, internal/modelprefs/modelprefs.go, internal/modelprefs/modelprefs_effort_test.go, internal/pipeline/artifacts.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go, internal/stagemodel/resolver.go, internal/stagemodel/resolver_test.go, internal/view/config.go, internal/view/config_test.go; skipped: none; constraints: Review scope limited to assigned Go implementation and tests. Regular CGO-enabled tests cannot compile from the workspace path because clang mishandles its spaces; CGO-disabled focused tests passed for config, modelprefs, and stagemodel.
policies:conventions — complete (broad); inspected 7 assigned files (18 inspected across reviewers): README.md, docs/architecture.md, docs/init-config-surface.md, internal/cmd/initcmd/initcmd.go, internal/config/config.go, internal/pipeline/pipeline.go, internal/view/config.go; skipped: none; constraints: Shared CLI standards were not available as a local convenience copy; review used repo-local conventions and the supplied review context. Targeted Go tests could not build because the sandbox worktree path contains spaces that clang treated as split path arguments.
structure:repo-health — complete (broad); inspected 9 assigned files (18 inspected across reviewers): docs/architecture.md, internal/cmd/initcmd/initcmd.go, internal/config/config.go, internal/modelprefs/modelprefs.go, internal/pipeline/artifacts.go, internal/pipeline/pipeline.go, internal/pipeline/prompts.go, internal/stagemodel/resolver.go, internal/view/config.go; skipped: none; constraints: Review limited to the nine assigned files and their directly relevant tests/guardrail. Targeted Go tests could not build because the sandboxed workspace path contains spaces that the C toolchain treated as split paths.

Inspected files (18)

README.md
docs/architecture.md
docs/init-config-surface.md
internal/cmd/initcmd/initcmd.go
internal/cmd/initcmd/initcmd_max_effort_test.go
internal/cmd/initcmd/initcmd_test.go
internal/config/config.go
internal/config/config_max_effort_test.go
internal/modelprefs/modelprefs.go
internal/modelprefs/modelprefs_effort_test.go
internal/pipeline/artifacts.go
internal/pipeline/pipeline.go
internal/pipeline/pipeline_test.go
internal/pipeline/prompts.go
internal/stagemodel/resolver.go
internal/stagemodel/resolver_test.go
internal/view/config.go
internal/view/config_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 2m 49s | gpt-5.6-terra | cr 0.10.284

Field	Value
Model	gpt-5.6-terra
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health
Engine	codex_cli · gpt-5.6-terra
Reviewed by	cr · rianjs-bot[bot]
Duration	2m 49s wall · 4m 59s compute
Cost	unavailable
Tokens	1.8M in / 14.5k out

Per-workstream usage

orchestrator-selection — gpt-5.6-terra
- In: 18.0k
- Out: 475
- Cache read: 11.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 11s
go:implementation-tests — gpt-5.6-terra
- In: 915.1k
- Out: 5.1k
- Cache read: 825.6k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 56s
policies:conventions — gpt-5.6-terra
- In: 421.2k
- Out: 3.4k
- Cache read: 357.1k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 05s
structure:repo-health — gpt-5.6-terra
- In: 414.3k
- Out: 4.7k
- Cache read: 353.0k
- Cache create: unavailable
- Cost: unavailable
- Duration: 1m 30s
orchestrator-rollup — gpt-5.6-terra
- In: 39.0k
- Out: 751
- Cache read: 28.2k
- Cache create: unavailable
- Cost: unavailable
- Duration: 15s

rianjs-bot

Automated PR Review

Reviewed commit: 70f9200785e1
Profile: codex-rianjs-bot - Posting as: rianjs-bot[bot]

Summary

Reviewer	Findings
go:implementation-tests	0
policies:conventions	1
structure:repo-health	0

policies:conventions (1 finding)

Nits - `docs/architecture.md:61`

The architecture guardrail was extended to reject direct config.ResolveMaxEffort calls as well, but this section still documents only ResolveModelTier as restricted and checked. Update the two guardrail sentences to name both helpers, keeping the durable model-and-effort resolution contract aligned with its enforcement.

Reviewer Coverage

go:implementation-tests — complete (constrained); inspected 16 assigned files (19 inspected across reviewers): internal/architecture/model_resolution_test.go, internal/cmd/initcmd/initcmd.go, internal/cmd/initcmd/initcmd_max_effort_test.go, internal/cmd/initcmd/initcmd_test.go, internal/config/config.go, internal/config/config_max_effort_test.go, internal/modelprefs/modelprefs.go, internal/modelprefs/modelprefs_effort_test.go, internal/pipeline/artifacts.go, internal/pipeline/pipeline.go, internal/pipeline/pipeline_test.go, internal/pipeline/prompts.go, internal/stagemodel/resolver.go, internal/stagemodel/resolver_test.go, internal/view/config.go, internal/view/config_test.go; skipped: none; constraints: Focused Go tests could not run because this read-only workbench prevents Go from creating its build-cache directories.
policies:conventions — complete (constrained); inspected 7 assigned files (19 inspected across reviewers): README.md, docs/architecture.md, docs/init-config-surface.md, internal/cmd/initcmd/initcmd.go, internal/config/config.go, internal/pipeline/pipeline.go, internal/view/config.go; skipped: none; constraints: Shared CLI standards were not available as a local convenience copy; review used repo-local conventions and the supplied review context.
structure:repo-health — complete (constrained); inspected 9 assigned files (19 inspected across reviewers): docs/architecture.md, internal/cmd/initcmd/initcmd.go, internal/config/config.go, internal/modelprefs/modelprefs.go, internal/pipeline/artifacts.go, internal/pipeline/pipeline.go, internal/pipeline/prompts.go, internal/stagemodel/resolver.go, internal/view/config.go; skipped: none; constraints: Review scoped to the assigned files and the updated architecture guardrail context.

Inspected files (19)

README.md
docs/architecture.md
docs/init-config-surface.md
internal/architecture/model_resolution_test.go
internal/cmd/initcmd/initcmd.go
internal/cmd/initcmd/initcmd_max_effort_test.go
internal/cmd/initcmd/initcmd_test.go
internal/config/config.go
internal/config/config_max_effort_test.go
internal/modelprefs/modelprefs.go
internal/modelprefs/modelprefs_effort_test.go
internal/pipeline/artifacts.go
internal/pipeline/pipeline.go
internal/pipeline/pipeline_test.go
internal/pipeline/prompts.go
internal/stagemodel/resolver.go
internal/stagemodel/resolver_test.go
internal/view/config.go
internal/view/config_test.go

0 PR discussion threads considered. 0 summarized; 0 resolved.

Completed in 54s | gpt-5.6-terra | cr 0.10.284

Field	Value
Model	gpt-5.6-terra
Reviewers	go:implementation-tests, policies:conventions, structure:repo-health
Engine	codex_cli · gpt-5.6-terra
Reviewed by	cr · rianjs-bot[bot]
Duration	54s wall · 1m 17s compute
Cost	unavailable
Tokens	2.7M in / 17.8k out

Per-workstream usage

go:implementation-tests — gpt-5.6-terra
- In: 1.4M
- Out: 6.6k
- Cache read: 1.2M
- Cache create: unavailable
- Cost: unavailable
- Duration: 31s
policies:conventions — gpt-5.6-terra
- In: 577.9k
- Out: 4.6k
- Cache read: 497.9k
- Cache create: unavailable
- Cost: unavailable
- Duration: 18s
structure:repo-health — gpt-5.6-terra
- In: 657.0k
- Out: 5.8k
- Cache read: 571.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 20s
orchestrator-rollup — gpt-5.6-terra
- In: 62.2k
- Out: 813
- Cache read: 48.4k
- Cache create: unavailable
- Cost: unavailable
- Duration: 6s

rianjs added 4 commits August 10, 2026 12:04

Merge remote-tracking branch 'origin/main' into feat/max-effort-tier-…

f7efda8

…ceiling

fix: enforce max effort ceilings consistently

d08ca40

docs: document max effort precedence

c7f8896

docs: clarify effort ceiling scope

e27c3d8

test: cover exact reviewer override provenance

ba724ec

rianjs marked this pull request as ready for review August 10, 2026 18:05

rianjs-bot Bot suggested changes Aug 10, 2026

View reviewed changes

Comment thread internal/stagemodel/resolver.go

Comment thread docs/architecture.md

Comment thread internal/view/config.go

test: guard effort ceiling resolution boundary

70f9200

rianjs-bot Bot approved these changes Aug 10, 2026

View reviewed changes

Comment thread docs/architecture.md

docs: align model resolution guardrail

07cd403

rianjs merged commit 3b7004f into main Aug 10, 2026
10 checks passed

rianjs deleted the feat/max-effort-tier-ceiling branch August 10, 2026 19:20

Navigation Menu

Uh oh!

feat(config): cap reviewer effort per model tier - #552

feat(config): cap reviewer effort per model tier#552
rianjs merged 8 commits into
mainfrom
feat/max-effort-tier-ceiling

zzwong commented Aug 5, 2026

Uh oh!

rianjs commented Aug 10, 2026

Uh oh!

rianjs commented Aug 10, 2026

Uh oh!

rianjs-bot Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rianjs-bot Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

zzwong commented Aug 5, 2026

Why

What

Notable

Testing

Follow-ups, not in this PR

Uh oh!

rianjs commented Aug 10, 2026

PR #552 — pre-fix TDD assessment

Blocker

Major

Minor

Nit

Uh oh!

rianjs commented Aug 10, 2026

PR #552 post-fix TDD / coverage assessment

Verdict: PASS, with one targeted non-blocking test gap

P2 — worthwhile follow-up

Covered convincingly

Low-value expansion to skip

Uh oh!

rianjs-bot Bot left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Major - internal/stagemodel/resolver.go:97

Minor - docs/architecture.md:61

Minor - internal/view/config.go:270

Reviewer Coverage

Uh oh!

Uh oh!

Uh oh!

Uh oh!

rianjs-bot Bot left a comment

Choose a reason for hiding this comment

Automated PR Review

Summary

Nits - docs/architecture.md:61

Reviewer Coverage

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Major - `internal/stagemodel/resolver.go:97`

Minor - `docs/architecture.md:61`

Minor - `internal/view/config.go:270`

Nits - `docs/architecture.md:61`