feat(cmd): add no-review cmd

[Syt3s]

Description

Adds --no-filter flag to ocr review, allowing users to skip the REVIEW_FILTER_TASK post-processing phase — eliminating one LLM call per reviewed file and reducing token consumption.

The REVIEW_FILTER_TASK runs after each file's main review loop to filter out false-positive comments. Every reviewed file triggers exactly one additional LLM call for this phase. While effective for trimming low-quality comments, this per-file call adds up quickly — a 20-file PR costs 20 extra LLM round-trips even when the filter produces no actionable removals.

ocr scan already provides --no-plan, --no-dedup, and --no-summary flags to skip optional LLM stages, but ocr review had no equivalent. This PR closes the gap by following the same established pattern.

Design: mirrors the existing --no-dedup pattern from internal/scan/agent.go. The skip check lives inside executeReviewFilter, placed after the ReviewFilterTask == nil guard (so users aren't spammed with misleading skip messages when the template doesn't configure a filter task) and after the telemetry span is created (so the skip is still observable in traces via the skipped=true span attribute). When the flag is set, a log line [ocr] Review filter skipped for <file> (--no-filter) is emitted and the function returns immediately.

Files changed (4 files, 6 code locations):

File	Change
internal/agent/agent.go	Added SkipFilter bool to Args struct with doc comment
internal/agent/agent.go (executeReviewFilter)	Added early-return check after the nil-template guard and span setup; sets skipped=true span attr
cmd/opencodereview/review_cmd.go	Added noFilter bool to reviewOptions; wired to Args.SkipFilter
cmd/opencodereview/shared_flags.go	Registered --no-filter bool flag (default false) in registerReviewFlags
internal/agent/coverage_test.go	Added TestExecuteReviewFilter_SkipFilter with 5 sub-tests

No changes to internal/scan/, internal/llmloop/, internal/tool/, internal/diff/, or internal/config/. Zero new third-party Go dependencies.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change (fix or feature that would cause existing functionality to change)
• Refactoring (no functional changes)
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

All tests use fakeAgentClient (an in-package mock with a calls counter) — no real LLM configuration needed. The counter-argument test (AC-3, no flag) proves the mock is correctly invoked when the filter runs, so the zero-call assertions in AC-1/2/4/5 are meaningful.

New tests (5 sub-tests in TestExecuteReviewFilter_SkipFilter):

Sub-test	Verifies
AC-1: SkipFilter disables the filter	With SkipFilter=true + ReviewFilterTask present + comments exist → client.calls == 0, comments untouched
AC-2: All comments preserved when skipped	3 comments added → all 3 remain after skip
AC-3: Default (no SkipFilter) still runs filter	SkipFilter zero-value (false) → client.calls > 0, comments filtered normally (backward compat)
AC-4: SkipFilter is reached when ReviewFilterTask is non-nil	SkipFilter=true + non-nil ReviewFilterTask + 1 comment → client.calls == 0, comment preserved (exercises the skip branch, not the nil-template guard)
AC-5: Skip takes priority over no comments	SkipFilter=true + zero comments → client.calls == 0

Regression: all 6 existing TestExecuteReviewFilter_* tests pass unchanged (NoFilterTask, NoComments, RemovesComments, LLMError, WithTimeout, plus the existing timeout variant).

• make test passes locally
• Manual testing (describe below)

Checklist

• My code follows the project's coding style (go fmt, go vet)
• I have performed a self-review of my code
• I have added tests that prove my fix is effective or my feature works
• New and existing unit tests pass locally with my changes
• I have updated the documentation accordingly (if applicable)
• I have signed the CLA

Related Issues

closes #833

[github-actions]

🔍 OpenCodeReview found 2 issue(s) in this PR.

• ✅ Successfully posted inline: 1 comment(s)
• ❌ Failed to post inline: 1 comment(s)

---

[bug · low]

📄 internal/agent/agent.go (L1240-L1249)

⚠️ GitHub could not post this as an inline comment: Lines 1240-1249 could not be resolved (outside PR diff hunks)

The SkipReviewFilter check is placed before the nil/empty check for ReviewFilterTask. When the template has no REVIEW_FILTER_TASK defined (ft == nil), the function would previously return silently. With this ordering, it will instead print a misleading "Review filter skipped" message for every file, even though no filter was ever going to run. Move this block after the ft == nil || len(ft.Messages) == 0 check so the skip message is only emitted when an actual filter task is being bypassed.

💡 Suggested Change

Before:

	if a.args.SkipReviewFilter {
		telemetry.SetAttr(span, "skipped", true)
		fmt.Fprintf(stdout.Writer(), "[ocr] Review filter skipped for %s (--no-review-filter)\n", newPath)
		return
	}

	ft := a.args.Template.ReviewFilterTask
	if ft == nil || len(ft.Messages) == 0 {
		return
	}

After:

	ft := a.args.Template.ReviewFilterTask
	if ft == nil || len(ft.Messages) == 0 {
		return
	}

	if a.args.SkipReviewFilter {
		telemetry.SetAttr(span, "skipped", true)
		fmt.Fprintf(stdout.Writer(), "[ocr] Review filter skipped for %s (--no-review-filter)\n", newPath)
		return
	}

[github-actions]

[maintainability · low]
The skip check happens before validating whether a review filter task actually exists. This means users will see "Review filter skipped for " messages for every file even when REVIEW_FILTER_TASK is not configured in the template. Consider moving the SkipReviewFilter check after the ft == nil || len(ft.Messages) == 0 validation to avoid spamming users with misleading skip messages when there was nothing to skip.

[wu21-web]

Great job there, no issues!

[lizhengfeng101]

Hey, nice PR! One small naming suggestion:

The existing skip flags follow a pattern of --no- + the core keyword from the task name:

• PLAN_TASK → --no-plan / SkipPlan
• DEDUP_TASK → --no-dedup / SkipDedup
• PROJECT_SUMMARY_TASK → --no-summary / SkipSummary

For REVIEW_FILTER_TASK, following the same convention would give us --no-filter / noFilter / SkipFilter rather than --no-review-filter / noReviewFilter / SkipReviewFilter.

The review- prefix feels redundant here since this flag only exists on the ocr review subcommand anyway — there's no ambiguity about which filter we're skipping. Shorter is also easier to type :)

Would you mind renaming to --no-filter for consistency with the rest of the family?

[Syt3s]

Hi @lizhengfeng101, thanks for the review and the great naming suggestion! 🙏

That makes total sense — keeping the skip flags consistent with the existing --no-* convention keeps things cleaner and easier to remember. I've renamed --no-review-filter to --no-filter across the flag name, variable name, and the corresponding SkipFilter helper. Please take another look when you have a moment.

[lizhengfeng101]

@Syt3s CI / test (pull_request)Failing

[Syt3s]

@Syt3s CI / test (pull_request)Failing

fixed

[lizhengfeng101]

Hey @Syt3s, nice work addressing the earlier feedback — the check ordering and naming are both solid now. A few things I'd like cleaned up before we merge:

1. PR description is stale

The body still references --no-review-filter and SkipReviewFilter in several places (the design section, the files-changed table, etc.), but the code now uses --no-filter / SkipFilter. Would you mind updating the description to match? Once this merges, the PR becomes the historical record — future readers shouldn't have to diff the description against the code to figure out what actually landed.

2. AC-4 test name contradicts the implementation

t.Run("AC-4: Skip takes priority over nil ReviewFilterTask", ...)

After you moved the SkipFilter check below the nil-template guard (the right call per the bot's feedback), it's actually the nil check that returns first — SkipFilter is never even evaluated in this path. The test still passes, but the name claims the opposite causality. Something like "AC-4: No LLM call when SkipFilter=true and ReviewFilterTask is nil" would accurately describe what's being asserted.

3. AC-4 doesn't exercise the SkipFilter path

Related to the above — because ReviewFilterTask is nil, the function bails out before reaching the SkipFilter branch. You could set SkipFilter to false and the test would still pass identically. So this sub-test isn't really adding coverage for the skip logic; it's a duplicate of the existing TestExecuteReviewFilter_NoFilterTask. Consider either removing it or giving it a non-nil ReviewFilterTask (so the skip check is actually reached and meaningfully tested).

---

Everything else looks good. Thanks for iterating on this!

[lizhengfeng101]

All three issues addressed — description is accurate, AC-4 now exercises the actual SkipFilter branch with a meaningful assertion, and the naming is consistent throughout. LGTM, thanks for the quick turnaround!