Skip to content

feat(review): explore the change once per round - #514

Merged
moq-bot[bot] merged 8 commits into
mainfrom
quest/m0/token-waste/review-single-pass
Nov 7, 2026
Merged

moq-bot[bot] merged 8 commits into
mainfrom
quest/m0/token-waste/review-single-pass

Conversation

@kixelated

@kixelated kixelated commented Nov 5, 2026 •

Copy link
Copy Markdown
Collaborator

Implements single-pass review, part of the token waste epic, and deletes that quest.

Design change

At the round-3 checkpoint, the maintainer picked option 1 (comment): a session resumes, checking only the delta, when nothing but the head changed. A changed model, effort, base ref, uncovered range, instruction file, or REVIEW.md starts a fresh session that reviews the whole change. The prompt branches that patched individual cases are gone, and later prompts no longer resend REVIEW.md, since a resumed session already has it. Review then restarted with quest review --new.

Summary

  • Round 1 explores once. The review turn (codex exec review with custom instructions on stdin, or /code-review <effort> <merge-base>...HEAD "<text>") now gets the range, REVIEW.md, the severity scale, the score request, and any earlier findings and responses up front. The follow-up turn only reports that review in the JSON schema, and it is told to read no files and run no commands. Before this change, the follow-up re-applied REVIEW.md and re-explored the change from scratch.
  • Later rounds check only the delta: what changed since the last reviewed head (git diff <previous> <head>, minus merged base commits) and the open findings. This applies only when nothing but the head changed: the same model, effort, and base ref, a merge base that the last round's covers (a base that moved on is fine), and the same instruction files and REVIEW.md. Any other change starts a fresh session with a full review that still weighs --responses. Each round stores its merge base for this.
  • Unchanged head: a round that would resume on the head the last round passed (no findings left to settle) runs no reviewer. It repeats that round with an unchanged line, and --json shows unchanged: true. A round with findings still runs on an unchanged head, since a refutation needs no new commit.
  • Reviewer context: Codex runs with -c skills.include_instructions=false. Every turn's instructions say that the base's files and REVIEW.md are all the review needs. They tell the reviewer not to reread or search for instruction files, to disregard other agent instructions such as the user's global AGENTS.md, and to run no builds, tests, or quest commands.
  • ## Models record: without flags, quest review reads the implementing models from the open PR body's Implementer: lines and the size from its Size: line. Order of precedence: --size, then the quest on the base, then the record, then M. --exclude-model replaces the record's models. If the record has an unknown model or no record exists, the running harness's models are excluded too, as before. A record also applies with --harness. A malformed field the round reads (a blank, invalid, or repeated Size:, or an Implementer: line with no model) exits 2, unless the flag that replaces it is given. A PR lookup GitHub refuses exits 1.
  • Review efforts: Sol now reviews XS low, S medium, M medium, L high, XL xhigh, replacing M high and L xhigh from moq-dev/quest#493. Opus is unchanged. The m0 model check Plan now asks its audit to compare findings by effort, and m1's Plan names the new table.
  • Round budget: assets/CONTEXT.md Reviews and /quest-review now say that a round whose findings are all low severity needs no further round. The author fixes or refutes each and adds a line after the record naming the new head's full SHA, which the land gate counts. Fixing those findings is added to the review exemptions. The prompts now define high, medium, and low. The round-3 design checkpoint is unchanged.
  • quest review --help, the README's review section, and the models.ts header path (quest/m1 to quest/m0/model-check.md) are updated.

Evidence (probes on this host, codex-cli 0.166.0, Claude Code 2.1.316)

  • codex exec review rejects a custom prompt alongside --base, and ignores --output-schema in review mode, so Codex needs the follow-up turn.
  • /code-review with --json-schema prints Command completed and reviews nothing, so Claude needs the follow-up turn too. Text after the target, as one double-quoted argument with \" and \\ escapes, reaches the review intact.
  • With skills.include_instructions=false, the Codex rollout has no skills catalog.
  • The global AGENTS.md still loads. Codex reads $CODEX_HOME/AGENTS.override.md or AGENTS.md (codex-rs/codex-home/src/instructions/mod.rs) and has no setting to skip it. In a probe with it loaded, the review mode ran quest context on its own.
  • Claude's reviewer (--setting-sources "") reported no CLAUDE.md, memory, or skills in its context.

Choices made without asking

  • Two turns, not one. Neither built-in review returns a schema, so the quest's preferred single turn is not possible without dropping the built-in review. I kept the tuned built-in review and made the second turn report-only.
  • The diff is named by range, not embedded. Both built-in reviews fetch it themselves, and an embedded diff has no natural bound.
  • Precedence when both exist: for a quest branch, the quest on the base still wins over the record's Size:, since the record is author-written. This keeps moq-dev/quest#493's rule that a change cannot lower its own review.
  • Global AGENTS.md: the only lever is a separate CODEX_HOME. That would need a copy of auth.json, and Codex saves a refreshed token by rename (codex-rs/login/src/auth/storage.rs), so the user's own login would keep a rotated-out refresh token and be logged out. The prompt tells the reviewer to disregard the global file instead.
  • The low-severity rule is prose plus a hand-added record line, not automated, since quest review cannot tell low-finding fixes from other changes.

Public API

quest review's behavior and output change:

  • An unchanged line, and unchanged in --json.
  • The size source can be ## Models.
  • Sol efforts for M and L.
  • A GitHub lookup before the round, to read ## Models.

No flags were added or removed. The persisted session state gains last; older state files simply never skip.

Validation

  • just check: passed.
  • just test: passed at bfe8ad8 (vitest: 1094 passed, 5 skipped; worker: 539 passed). just check passed there too.
  • Real rounds on this PR (Sol medium), from the Codex rollouts:
    • Round 1: the review sub-thread ran 6 read-only commands (git diff, sed, nl, cat CONTRIBUTING.md), with no quest commands, builds, tests, or skills catalog. It used 207k input tokens, 135k of them cached. The report turn ran no commands and used 16.5k input tokens.
    • Round 2 used 102k input tokens, and round 3 used 402k (306k cached).
  • New tests cover:
    • the unchanged-head repeat, and the refutation round on an unchanged head
    • the later-round delta prompt
    • size and exclusions from ## Models, including flag overrides, unknown models, and a malformed Size: or Implementer: line
    • the record parser
    • the Claude quoting

Suggested follow-ups

  • Plan: an upstream Codex setting to skip $CODEX_HOME/AGENTS.md for one run, or a --ignore-user-config that also covers it. Posting to openai/codex needs maintainer approval, so this is a Waiting quest.
  • Drop: the runner's (kix) review loop does not apply the low-severity budget. It re-reviews until clean, and its check run is the land gate there. Changing it is worth doing only if runner review cost shows up in quest usage.

Models

  • Size: M
  • Implementer: Claude Code, claude-opus-5-5, high effort

(Written by Claude Opus 5.5 with high effort)

🤖 Generated with Claude Code

Review

mechanical fix by codex, gpt-6.1-sol, medium effort: Merged base preserving both import additions in src/cli/main.ts and repaired two links to the completed quest; ran just install, just check, and just test successfully. (head 419ff84)

Rewritten by quest review after each round.

  • Reviewer: Codex, gpt-6.1-sol, medium effort
  • Session: 01a1adf9-7c69-7a30-9b45-f40bbbaa7924, round 3
  • Head: acc81f3
  • Base: origin/main at 4b07dd3
  • Findings: none
  • Responses this round: the online review's medium finding on bfe8ad8 (src/github/models.ts:37, review), fixed in acc81f3: every Size: item's whole value is kept, and a blank or multi-token value or a malformed duplicate now exits 2 unless --size is given.

@pullfrog

pullfrog Bot commented Nov 5, 2026

Copy link
Copy Markdown

Pullfrog  | Review this ➔

Round 1 gives the reviewer's own review the range, REVIEW.md, and any
responses up front, and its follow-up turn only reports the result in the
schema. Later rounds check only the changes since the reviewed head and
the open findings, and a round on an unchanged head with nothing to
settle repeats the last one without a reviewer. Codex drops its skills
catalog, and the prompt forbids reloading rules or running builds,
tests, or quest commands.

quest review reads the size and implementing models from the PR body's
## Models record. Sol reviews M at medium and L at high. A round whose
findings are all low severity ends the loop once they are handled.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated kixelated changed the title Single-pass review feat(review): explore the change once per round Nov 5, 2026
kixelated and others added 2 commits November 5, 2026 14:56
…he change under new rules

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…VIEW.md is removed

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

Copy link
Copy Markdown
Collaborator Author

Design change: at the round-3 checkpoint the maintainer picked option 1. A later round resumes the reviewer's session and checks only what changed when nothing but the head changed: the same harness, model, effort, base ref, a merge base the last round's covers, and the same instruction files and REVIEW.md. Any other change starts a fresh session with a full review that still weighs --responses. The prompt branches that patched individual cases (rulesChanged, rangeGrew) are removed. The next round starts with quest review --new.

(Written by Claude Opus 5.5 with high effort)

kixelated and others added 2 commits November 5, 2026 15:09
…nged

Any other change, to the model, effort, base, range, instruction files,
or REVIEW.md, starts a new session that reviews the whole change, so the
resumed prompt no longer patches each case.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@kixelated
kixelated marked this pull request as ready for review November 5, 2026 23:14
@coderabbitai

coderabbitai Bot commented Nov 5, 2026 •

Copy link
Copy Markdown

Important

  • Trigger review

This repository does not receive automatic reviews because it has fewer than 10 stars.

Run configuration

  • Source mode: Auto
  • Repository: Unavailable
  • Branch: Unavailable
  • File: No configuration file from this repository applied
  • Commit: Unavailable
  • Layers: Organization UI
  • Review profile: CHILL
  • Plan: Advanced
  • Run ID: af9abf1b-f2bb-4158-ad0f-22d0bbfc7540
  • Autofix · Keep fixing CodeRabbit findings and required CI, and resolving merge conflicts

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

Share

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed head: bfe8ad8

  • Medium: preserve malformed Size fields for validation (src/github/models.ts:37). The regex drops Size: and Size: L extra entirely, so recordedSize() sees no size and a non-quest branch silently uses M rather than the documented exit-2 usage error. A malformed second Size line also escapes duplicate detection. Collect every Size: field, then validate its whole value only when --size is absent. Add blank, multi-token, and malformed-duplicate cases alongside the existing XXL/override tests.

Direction: retain the single exploration/report split and conservative session reset logic; tighten this parser boundary.

Verification: static GitHub review of the changed code, documentation, and tests. No local tests or CLI probes run. The author reports checks/tests passing; current Check/Queue runs are skipped and Release remains in progress.

…ed one

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

Copy link
Copy Markdown
Collaborator Author

Agreed and fixed in acc81f3: recorded() now keeps every Size: item's whole trimmed value, and recordedSize() rejects a blank value, a multi-token one such as L extra, and any count above one (a malformed duplicate included), all only when --size is absent. New cases in test/review.test.ts cover blank, multi-token, a malformed duplicate, and a blank line overridden by --size. just check and just test pass; the local Sol medium review of the new head (round 3) reported no findings.

(Written by Claude Opus 5.5)

@kixelated
kixelated added this pull request to the merge queue Nov 7, 2026

@kixelated kixelated left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

Automated review by review (OpenAI)

Reviewed head: acc81f3
PR base and merge base: 73e0caf
Follow-up scope: changes since bfe8ad8, with the current base-to-head diff checked for context.

The previous medium finding is fixed. recorded() now preserves every Size field's whole trimmed value. recordedSize() rejects blank, multi-token, and duplicate values while keeping the --size override. The added regression cases cover the reported inputs and the override.

No new actionable findings in this follow-up. The delta contains only the parser correction, validation message, and tests.

Direction: keep the existing design; this addresses the outstanding parser boundary without expanding it.

Verification: static GitHub-only inspection. I did not run tests, builds, or CLI probes. The author reports just check and just test passing on this head. GitHub shows the release build and four platform checks succeeded; the PR check/lifecycle and Queue jobs are skipped, so those are not independent evidence of a full test pass.

(written by review (OpenAI), model unknown)

@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to a conflict with the base branch Nov 7, 2026
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

quest land watch: the merge queue removed this pull request at acc81f3 because it conflicts with main (merge_conflict).

The land fixer works on it in the background, and the watcher re-enqueues it at the back of the queue once it is fixed or judged flaky, or hands it back here with what it needs.

(written by quest land watch)

@moq-bot
moq-bot Bot enabled auto-merge November 7, 2026 09:11
@moq-bot

moq-bot Bot commented Nov 7, 2026

Copy link
Copy Markdown
Contributor

quest land watch: armed auto-merge on this pull request at 419ff84, so it joins the back of the merge queue once its required checks pass, with the land fixer's fix (codex, gpt-6.1-sol, medium effort): Merged base preserving both import additions in src/cli/main.ts and repaired two links to the completed quest; ran just install, just check, and just test successfully..

(written by quest land watch)

@moq-bot
moq-bot Bot added this pull request to the merge queue Nov 7, 2026
Merged via the queue into main with commit f24a614 Nov 7, 2026
17 checks passed
@moq-bot
moq-bot Bot deleted the quest/m0/token-waste/review-single-pass branch November 7, 2026 09:32
kixelated added a commit that referenced this pull request Nov 7, 2026
Main's `quest setup` (env.BASH_MAX_TIMEOUT_MS) and this branch's
(Quest's hooks) become one command: it validates the whole settings
file, env and hooks, before writing once. `--remove` takes out Quest's
hooks and the timeout while it still holds Quest's value.

The context's Reviews prose this branch moved gains main's #514 changes
in /quest-review and /quest-merge, and `quest review` prints
`next finish` for a round whose findings are all low severity. The
context stays under the 10,000-character hook cap: main's coordinator
lines move to a guide Coordination section that /quest-spawn and
/quest-complete include, and the rest is tightened without dropping a
rule.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
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