Skip to content

Add reasoning effort option - #3

Closed
ramshres wants to merge 2 commits into
Ingar30:mainfrom
ramshres:reasoning-effort-option
Closed

ramshres wants to merge 2 commits into
Ingar30:mainfrom
ramshres:reasoning-effort-option

Conversation

@ramshres

Copy link
Copy Markdown

Adds a configurable reasoning effort option across reviewer defaults, scripts, prompts, docs, and tests.

Created from the fork workaround because the authenticated account does not have direct push access to Ingar30/reviewer.

@likekangen likekangen 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.

Review summary for PR #3 — Add reasoning effort option

Thanks for the contribution. I checked out the PR locally and ran the project test/shareability gates against the PR head.

Validation run locally:

  • python -m unittest using the existing project venv: PASS, 53 tests
  • scripts/check_shareable_repo.py --include-untracked: PASS
  • scripts/check_tracked_sensitive_names.py: PASS
  • git diff --check origin/main...HEAD: PASS

Review notes:

  • The codex_exec_command(...) helper is a good consolidation: it preserves search-enabled reviewer behavior while adding the reasoning override in one place.
  • The reasoning-effort argument is threaded through reviewer selection, individual reviewers, parser repair, and editor execution, which is the right scope for run-level consistency.
  • One thing I would explicitly confirm before merge: the PR changes default behavior/cost profile by making full review runs use xhigh and parser repair overlay by default. That may be desirable for quality, but it should be accepted intentionally because it can increase runtime/cost.
  • Documentation was updated to mention the new default and lower-cost override path.

Recommendation: approve if the maintainers intentionally want the higher default reasoning/cost profile and parser-repair-overlay default. Otherwise request a small change to keep the CLI default as “project config unless overridden” and document --reasoning-effort xhigh as an opt-in quality mode.

@likekangen

Copy link
Copy Markdown

Follow-up after checking the current merge state: this PR is now conflicting with main in README.md.

The conflict is not only mechanical. It reflects a default-behavior decision:

  • PR branch text/code: parser repair overlay runs by default, and users pass --parser-repair off to skip it.
  • Current main text: parser repair is optional/off by default, and users opt in with overlay mode.

Recommendation before merge:

  1. Decide the intended default explicitly: quality-first default (overlay) vs cheaper/faster default (off).
  2. Resolve README.md so the command example matches that default.
  3. Re-run the local gates after resolving:
    • python -m unittest
    • python scripts/check_shareable_repo.py --include-untracked
    • python scripts/check_tracked_sensitive_names.py
    • git diff --check

I would keep this unapproved until the default-behavior decision and README conflict are resolved.

@likekangen

Copy link
Copy Markdown

Follow-up recommendation on the current conflict/default decision:

I recommend keeping parser repair off by default and treating --parser-repair overlay as the opt-in quality mode.

Reasoning:

  • origin/main now documents parser repair as optional/off by default, so the conflicting README section should stay aligned with that behavior.
  • Parser repair adds extra LLM runtime/cost and is still described as an experimental/narrow overlay step.
  • The reasoning-effort option from this PR is still useful independently; I would merge that part, but avoid bundling it with a parser-repair default flip.

Concrete resolution I would request before merge:

  1. In scripts/review_paper.py, keep --parser-repair default as off and keep the help text as opt-in.
  2. Keep/add --reasoning-effort with the intended default (xhigh if maintainers accept that quality/cost tradeoff, otherwise leave project config/default behavior unchanged).
  3. Resolve the README conflict using the origin/main parser-repair wording and command example:
    • --parser-repair overlay to enable it
    • not --parser-repair off as the normal/faster path
  4. Re-run:
    • ./.venv/bin/python -m unittest
    • ./.venv/bin/python scripts/check_shareable_repo.py --include-untracked
    • ./.venv/bin/python scripts/check_tracked_sensitive_names.py
    • git diff --check origin/main...HEAD

Until that default decision is reflected in code/docs, I would keep this PR unmerged.

@likekangen

Copy link
Copy Markdown

I prepared the concrete follow-up change for this PR, but I cannot push directly to ramshres/reviewer:reasoning-effort-option from the likekangen account (403 Permission denied).

Branch with the fix:
https://github.com/likekangen/reviewer/tree/fix-pr3-parser-repair-opt-in

Commit:
aa9b9f7 Keep parser repair opt-in

What it changes:

  • keeps the new --reasoning-effort option from this PR
  • reverts --parser-repair default back to off
  • updates README/AGENTS/skill/docs so parser repair is described as opt-in via --parser-repair overlay
  • removes wording that says parser repair overlay runs by default

Validation run on the fixed branch:

  • ./.venv/bin/python -m unittest — PASS, 53 tests
  • ./.venv/bin/python scripts/check_shareable_repo.py --include-untracked — PASS
  • ./.venv/bin/python scripts/check_tracked_sensitive_names.py — PASS
  • git diff --check origin/main...HEAD — PASS

A maintainer or the PR author can cherry-pick aa9b9f7 or apply the same changes to resolve the default-behavior conflict.

@Ingar30

Ingar30 commented Sep 2, 2026

Copy link
Copy Markdown
Owner

Superseded by #8, now merged into main. The final update retains configurable reasoning for controlled tests, uses GPT-5.6 Sol with the quality-first reasoning defaults, and removes the experimental parser-repair layer. Thank you for the original reasoning-effort contribution.

@Ingar30 Ingar30 closed this Sep 2, 2026
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.

3 participants