Skip to content

revert: back to Opus 4.8 and single-pass review (#108, #110) - #112

Merged
MounirDhahri merged 3 commits into
mainfrom
chore/use-opus-4.8-again
Aug 6, 2026
Merged

revert: back to Opus 4.8 and single-pass review (#108, #110)#112
MounirDhahri merged 3 commits into
mainfrom
chore/use-opus-4.8-again

Conversation

@MounirDhahri

@MounirDhahri MounirDhahri commented Aug 6, 2026

Copy link
Copy Markdown
Member

Summary

Test plan

  • Confirm claude-review.yml default model is claude-opus-4-8
  • Confirm two-pass review code, tests, and template flag are gone
  • Trigger a Claude review on a test PR and confirm single-pass behavior

Assisted-by: Claude:Sonnet-5

@MounirDhahri MounirDhahri self-assigned this Aug 6, 2026
@github-actions

github-actions Bot commented Aug 6, 2026

Copy link
Copy Markdown

Detected new or modified files in .github/workflows/. Please ensure you update the relevant documentation alongside these workflow changes.

@claude

claude Bot commented Aug 6, 2026

Copy link
Copy Markdown
Contributor

Summary

Reverts #108 (model default back to claude-opus-4-8) and #110 (removes the two-pass find/filter review, its compare-two-pass-review input, and the finder prompt/tests), plus a docs fix pointing the actions.md model example at the real default.

I diffed the result against 866a1456 (the tree right after #108, before two-pass landed): scripts/build-review-prompt.ts, scripts/build-review-prompt.test.ts, and templates/run-claude-review.yml come back byte-identical, and the only remaining delta in claude-review.yml is the intended claude-opus-5 -> claude-opus-4-8. No two-pass remnants survive in tracked files (--mode=, pass1, FINDER_PROMPT all grep clean; the only hits are in the untracked .tooling/ checkout, which is pulled from main at runtime). path and fs are both still used in the trimmed script, and every symbol the test file imports is still exported.

Issues Found

🟢 Suggestion - templates/run-claude-review.yml:14 still shows the old pinned ID: model: "claude-opus-4-20250514". Commit 19abbe3 deliberately synced docs/actions.md:204 to claude-opus-4-8 (the actual workflow default; the docs had said claude-opus-4-20250514 since before #108). The template is now the last place carrying the stale value, and this PR already edits the line right below it. One-line change while you are in the file.

Areas Reviewed

  • Version skew: claude-review.yml checks out tooling with ref: main, so scripts always come from main while the workflow file comes from the caller's ref. Dropping --mode=legacy from the workflow and from the script in the same commit keeps those in sync, and the template pins @main. No skew window.
  • Testing: the deleted tests only covered deleted code. Remaining loadRepoConfig / buildPrompt / DEFAULT_PROMPT coverage is untouched.
  • Docs: docs/actions.md drops the flag from both the example block and the inputs list, and the stated timeout-minutes: 30 default matches claude-review.yml.

Questions for Author

  1. Did any consumer repo add compare-two-pass-review: true to its caller workflow while feat: two-pass Claude review (find, then filter) to cut false positives - behind feature flag #110 was on main? Removing the input means those callers now pass something the reusable workflow no longer declares. Given feat: two-pass Claude review (find, then filter) to cut false positives - behind feature flag #110 merged and was reverted the same day this is probably moot, but it is a cheap check.
  2. Is the two-pass approach coming back in some form, or is this a full retreat? The prompts and tests are recoverable from 1ab8ec1 either way, so asking to get the intent on record, not to keep the code around.

@MounirDhahri
MounirDhahri merged commit bd791c7 into main Aug 6, 2026
23 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