joshtronic/igor

Watch

feat(review): escalating effort ladder -- reviewer flat->max, worker climbs (Closes #308) #312

Merged

joshtronic merged 2 commits from agent/308-review-effort-ladder into master

2026-07-01 20:47:02 +00:00

igor commented

2026-07-01 20:13:59 +00:00

Collaborator

What this does

Implements the RFC (#308): reasoning effort now escalates through the review + rework flow. The harness set no effort before; the model stays Opus, escalation is pure effort.

Reviewer axis (the "final boss")

reviewer_effort(rework_rounds): flat high during the loop -- a stable bar the worker can converge on (a reviewer that got pickier every round would move the goalposts and the PR would never land) -- then max for the last review before it escalates to the human (rounds >= 3). Rare, and the alternative is your time, so max is the cheap bet.

Worker axis

worker_effort(rework_rounds): the bot`s PR rework climbs each round -- high -> xhigh -> max -- trying harder as the problem proves hard.

Plumbing

claude_call learns an optional model:effort suffix (claude-opus-4-8:high -> --effort high). Model ids carry no colon, so every existing bare-model caller is unchanged. The rework claude_run_with_cost passes --effort directly.

Audit-trail seed (for #310)

The shadow-review comment now records (effort: <level>) -- the first thread of the per-ticket audit trail #310 will build on. This PR is the one we dogfood #310 through.

Safety

Verified claude -p --effort is accepted (not just interactive) and a real claude_call with a model:effort model returned cleanly -- so the critical review path (which gates every merge) is proven before wiring.
reviewer_effort / worker_effort are hardcoded thresholds (opinionated, not knobs). Bare-model callers unaffected. check-sync green, bash -n clean.

Test plan

claude -p --effort low returns a clean result (no unknown-option error)
real claude_call "claude-opus-4-8:low" ... -> ok (full path works)
reviewer_effort: 0/1/2 -> high, 3+ -> max ; worker_effort: 0/1 -> high, 2 -> xhigh, 3+ -> max
bin/check-sync.sh green
Reviewer: merge, then we build #310 (audit trail) and watch it get reviewed by this laddered reviewer

Closes #308.

## What this does Implements the RFC (#308): reasoning effort now escalates through the review + rework flow. The harness set no effort before; the model stays **Opus**, escalation is pure **effort**. ### Reviewer axis (the "final boss") `reviewer_effort(rework_rounds)`: **flat `high`** during the loop -- a stable bar the worker can converge on (a reviewer that got pickier every round would move the goalposts and the PR would never land) -- then **`max`** for the last review before it escalates to the human (rounds >= 3). Rare, and the alternative is your time, so max is the cheap bet. ### Worker axis `worker_effort(rework_rounds)`: the bot`s PR rework **climbs** each round -- **high -> xhigh -> max** -- trying harder as the problem proves hard. ### Plumbing `claude_call` learns an optional **`model:effort`** suffix (`claude-opus-4-8:high` -> `--effort high`). Model ids carry no colon, so every existing bare-model caller is unchanged. The rework `claude_run_with_cost` passes `--effort` directly. ### Audit-trail seed (for #310) The shadow-review comment now records **`(effort: <level>)`** -- the first thread of the per-ticket audit trail #310 will build on. This PR is the one we dogfood #310 through. ## Safety - **Verified `claude -p --effort` is accepted** (not just interactive) and a **real `claude_call` with a `model:effort` model returned cleanly** -- so the critical review path (which gates every merge) is proven before wiring. - `reviewer_effort` / `worker_effort` are **hardcoded thresholds** (opinionated, not knobs). Bare-model callers unaffected. `check-sync` green, `bash -n` clean. ## Test plan - [x] `claude -p --effort low` returns a clean result (no unknown-option error) - [x] real `claude_call "claude-opus-4-8:low" ...` -> `ok` (full path works) - [x] `reviewer_effort`: 0/1/2 -> high, 3+ -> max ; `worker_effort`: 0/1 -> high, 2 -> xhigh, 3+ -> max - [x] `bin/check-sync.sh` green - [ ] Reviewer: merge, then we build #310 (audit trail) and watch it get reviewed by this laddered reviewer Closes #308.

igor added 1 commit

2026-07-01 20:13:59 +00:00

feat(review): escalating effort ladder -- reviewer flat->max, worker climbs (igor#308)

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

288333d460

Wire reasoning effort into the review + rework flow (the harness set none
before). Verdict model stays Opus; escalation is pure effort, per the RFC:

- claude_call learns an optional `model:effort` suffix (e.g.
  "claude-opus-4-8:high") -> `--effort`. Model ids carry no colon, so a
  bare model is unchanged.
- Reviewer effort is FLAT ("high") during the rework loop -- a stable bar
  the worker can converge on -- then "max" for the final look before the
  reviewer escalates to the human (the "final boss": rare, and the
  alternative is Josh's time).
- Worker rework effort CLIMBS each round: high -> xhigh -> max. Try harder
  as the problem proves hard.
- The review comment now records the effort used (seed for the #310
  audit trail we dogfood next).

Verified `claude -p --effort` is accepted and a real claude_call with a
`model:effort` model returns cleanly, so the critical review path is safe.
reviewer_effort / worker_effort are hardcoded thresholds -- opinionated,
not knobs.

Closes #308.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_011KXPafSYJPY6XkWHHk1Lfs

igor requested review from joshtronic

2026-07-01 20:13:59 +00:00

igor commented

2026-07-01 20:18:35 +00:00

Author

Collaborator

🤖 Review — `REQUEST_CHANGES` (automated)

CI for 288333d4: success

Summary: Adds an escalating reasoning-effort ladder — reviewer flat high→max, worker high→xhigh→max — via a new model:effort suffix in claude_call and a direct --effort on the rework path. Small, focused (~60 lines), in scope, no CI-config touches, no security concerns. The code and the description's threshold claims match the diff. One concrete gap blocks it.

Blocking

The effort tokens actually shipped were never verified to be accepted values. The test plan proves only claude -p --effort low and claude_call "...:low" — but low is never used in production. The values that are deployed are high (every normal shadow review), xhigh (worker rework round 2), and max (final review + rework round ≥3). None of these three were verified. If the CLI validates --effort against an enum and any of these tokens is rejected:
- high failing breaks every shadow review (bin/tick.sh ~L2632, ${AGENT_MODEL_REVIEW}:${rev_effort}) — i.e. the merge gate silently stops producing reviews after both retry attempts fail.
- xhigh failing breaks worker rework at round 2 (bin/tick.sh L3361, --effort "$PR_REWORK_EFFORT").
- max failing breaks the final escalation review.
Testing low proves the flag parses, not that the tokens you ship are valid — and choosing low for the test while deploying high/xhigh/max is exactly the mismatch worth closing. CI (check-sync + bash -n) does not exercise a live claude call, so success doesn't cover this. Fix: run each of --effort high, --effort xhigh, --effort max once and confirm a clean result (no unknown-value error), and add them to the test plan. Cheap to do; if they all pass this is a one-line re-check.

Non-blocking nits

bin/tick.sh L3359–L3360: review_rework_rounds "$REVIEW_KEY" is computed three times (once for PR_REWORK_EFFORT, twice in the log line). Harmless but worth caching in a local, since the round can in principle move between calls.

Notes (looks correct)

claude.sh colon-split (${model##*:} / ${model%:*}) is unambiguous given colon-free model ids; the case *:* guard leaves all bare-model callers untouched. Good.
reviewer_effort (≥3→max, else high) and worker_effort (0|1→high, 2→xhigh, *→max) match the described thresholds exactly.
Audit-trail (effort: ${rev_effort}) addition to the review comment is a benign one-liner.

Once the three deployed effort tokens are confirmed accepted, I'd have no further reservations.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `REQUEST_CHANGES` _(automated)_ CI for `288333d4`: **success** **Summary:** Adds an escalating reasoning-effort ladder — reviewer flat `high`→`max`, worker `high`→`xhigh`→`max` — via a new `model:effort` suffix in `claude_call` and a direct `--effort` on the rework path. Small, focused (~60 lines), in scope, no CI-config touches, no security concerns. The code and the description's threshold claims match the diff. One concrete gap blocks it. ### Blocking - **The effort tokens actually shipped were never verified to be accepted values.** The test plan proves only `claude -p --effort low` and `claude_call "...:low"` — but `low` is never used in production. The values that *are* deployed are `high` (every normal shadow review), `xhigh` (worker rework round 2), and `max` (final review + rework round ≥3). None of these three were verified. If the CLI validates `--effort` against an enum and any of these tokens is rejected: - `high` failing breaks **every shadow review** (`bin/tick.sh` ~L2632, `${AGENT_MODEL_REVIEW}:${rev_effort}`) — i.e. the merge gate silently stops producing reviews after both retry attempts fail. - `xhigh` failing breaks **worker rework at round 2** (`bin/tick.sh` L3361, `--effort "$PR_REWORK_EFFORT"`). - `max` failing breaks the final escalation review. Testing `low` proves the *flag parses*, not that the *tokens you ship* are valid — and choosing `low` for the test while deploying `high/xhigh/max` is exactly the mismatch worth closing. CI (`check-sync` + `bash -n`) does not exercise a live `claude` call, so `success` doesn't cover this. **Fix:** run each of `--effort high`, `--effort xhigh`, `--effort max` once and confirm a clean result (no unknown-value error), and add them to the test plan. Cheap to do; if they all pass this is a one-line re-check. ### Non-blocking nits - `bin/tick.sh` L3359–L3360: `review_rework_rounds "$REVIEW_KEY"` is computed three times (once for `PR_REWORK_EFFORT`, twice in the log line). Harmless but worth caching in a local, since the round can in principle move between calls. ### Notes (looks correct) - `claude.sh` colon-split (`${model##*:}` / `${model%:*}`) is unambiguous given colon-free model ids; the `case *:*` guard leaves all bare-model callers untouched. Good. - `reviewer_effort` (≥3→`max`, else `high`) and `worker_effort` (0|1→`high`, 2→`xhigh`, *→`max`) match the described thresholds exactly. - Audit-trail `(effort: ${rev_effort})` addition to the review comment is a benign one-liner. Once the three deployed effort tokens are confirmed accepted, I'd have no further reservations. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-01 20:18:36 +00:00

1 minute 40 seconds

igor self-assigned this

2026-07-01 20:18:36 +00:00

igor added 1 commit

2026-07-01 20:25:44 +00:00

docs: document effort enum values and cache rework round

Lint / check-sync (push) Successful in 5s

Details

Lint / check-sync (pull_request) Successful in 4s

Details

bea4d1323a

igor removed their assignment

2026-07-01 20:25:44 +00:00

igor added spent time

2026-07-01 20:25:45 +00:00

4 minutes 37 seconds

igor commented

2026-07-01 20:29:31 +00:00

Author

Collaborator

🤖 Review — `COMMENT` (automated)

CI for bea4d132: success

Small, well-scoped change: adds two pure effort-ladder helpers (reviewer_effort, worker_effort), teaches claude_call an optional model:effort suffix, and threads effort into the shadow review + PR-rework calls. Logic reads correctly and the checklist matches the code. CI is green. A few things I'd want a human to weigh before merge, hence COMMENT rather than APPROVE.

Verified against the diff (positive)

Thresholds match the stated behavior exactly: reviewer_effort → high for rounds 0/1/2, max for ≥3 (tick.sh:1013); worker_effort → high (0/1), xhigh (2), max (≥3) (tick.sh:1024). Checklist item is honest.
model:effort split is unambiguous: ${model%:*} / ${model##*:} handles a single colon correctly; bare model (no colon) falls through the case untouched, so the "bare callers unaffected" claim holds structurally (claude.sh:283).
No CI-workflow changes, no secrets/injection surface (effort values come from hardcoded functions, never user input). Scope is tight (~60 lines).

Things to weigh

Empty-array expansion under set -u (claude.sh:288). "${effort_args[@]}" when the array is empty (the bare-model path — i.e. every other caller of claude_call) errors with "unbound variable" on bash < 4.4 (notably macOS's default 3.2). The :- defaults sprinkled through the new code suggest set -u may be active. On modern Linux bash (4.4+/5.x) this is a non-issue, and the author's manual verification exercised the effort path — but the empty-array path (the critical bare-model review/other calls) is the one that would break, and neither bash -n nor check-sync catches a runtime unbound-variable error. Worth a one-line confirmation of the runtime bash version, or defensively guard with ${effort_args[@]+"${effort_args[@]}"}.
No automated tests added. reviewer_effort/worker_effort are trivially unit-testable pure functions, but the contract's "add/adjust tests" is satisfied only by manual test-plan bullets here. If the repo has a bash test suite, these belong in it; if it doesn't, that's understandable but worth stating.
Unverifiable runtime claims. "claude -p --effort accepted (not just interactive)" and the live model:effort call returning ok can't be confirmed from the diff — CI green is consistent but doesn't exercise a real claude invocation. Also, the reviewer-maxes-at-round-3 design depends on review_rework_rounds and the REQUEST_CHANGES/escalation handler, neither of which is in the diff; the shown logic is internally consistent but I can't confirm the review at round ≥3 actually fires before escalation.

None of these are hard blockers, but the empty-array edge case on the bare-model path is the one I'd want eyes on before merge, since it touches the merge-gating review path for all existing callers.

_{Independent review by the harness on claude-opus-4-8. The human reviewer is requested once Igor has reviewed; a human still merges.}

### 🤖 Review — `COMMENT` _(automated)_ CI for `bea4d132`: **success** Small, well-scoped change: adds two pure effort-ladder helpers (`reviewer_effort`, `worker_effort`), teaches `claude_call` an optional `model:effort` suffix, and threads effort into the shadow review + PR-rework calls. Logic reads correctly and the checklist matches the code. CI is green. A few things I'd want a human to weigh before merge, hence COMMENT rather than APPROVE. **Verified against the diff (positive)** - Thresholds match the stated behavior exactly: `reviewer_effort` → `high` for rounds 0/1/2, `max` for ≥3 (`tick.sh:1013`); `worker_effort` → `high` (0/1), `xhigh` (2), `max` (≥3) (`tick.sh:1024`). Checklist item is honest. - `model:effort` split is unambiguous: `${model%:*}` / `${model##*:}` handles a single colon correctly; bare model (no colon) falls through the `case` untouched, so the "bare callers unaffected" claim holds structurally (`claude.sh:283`). - No CI-workflow changes, no secrets/injection surface (effort values come from hardcoded functions, never user input). Scope is tight (~60 lines). **Things to weigh** - **Empty-array expansion under `set -u` (`claude.sh:288`).** `"${effort_args[@]}"` when the array is empty (the bare-model path — i.e. every *other* caller of `claude_call`) errors with "unbound variable" on bash < 4.4 (notably macOS's default 3.2). The `:-` defaults sprinkled through the new code suggest `set -u` may be active. On modern Linux bash (4.4+/5.x) this is a non-issue, and the author's manual verification exercised the *effort* path — but the empty-array path (the critical bare-model review/other calls) is the one that would break, and neither `bash -n` nor `check-sync` catches a runtime unbound-variable error. Worth a one-line confirmation of the runtime bash version, or defensively guard with `${effort_args[@]+"${effort_args[@]}"}`. - **No automated tests added.** `reviewer_effort`/`worker_effort` are trivially unit-testable pure functions, but the contract's "add/adjust tests" is satisfied only by manual test-plan bullets here. If the repo has a bash test suite, these belong in it; if it doesn't, that's understandable but worth stating. - **Unverifiable runtime claims.** "`claude -p --effort` accepted (not just interactive)" and the live `model:effort` call returning `ok` can't be confirmed from the diff — CI green is consistent but doesn't exercise a real `claude` invocation. Also, the reviewer-maxes-at-round-3 design depends on `review_rework_rounds` and the REQUEST_CHANGES/escalation handler, neither of which is in the diff; the shown logic is internally consistent but I can't confirm the review at round ≥3 actually fires before escalation. None of these are hard blockers, but the empty-array edge case on the bare-model path is the one I'd want eyes on before merge, since it touches the merge-gating review path for all existing callers. --- <sub>Independent review by the harness on `claude-opus-4-8`. The human reviewer is requested once Igor has reviewed; a human still merges.</sub>

igor added spent time

2026-07-01 20:29:31 +00:00

1 minute 36 seconds