docs(pages): correct FAQ JSON output shape and plan-threshold cost lever

[Qiyuanqiii]

Description

Summary

• Correct two factual errors in the FAQ across all five maintained locales (en, zh, ja, ko, ru).
• Entry 1 — the FAQ incorrectly described review --format json as returning a bare [] for a successful zero-finding review, put files_reviewed at the wrong level, and suggested distinguishing "nothing to review" from "reviewed with no findings" by JSON shape.
• Entry 2 — the plan-phase cost guidance had the threshold direction reversed and did not account for the two thresholds' asymmetric behavior at zero.
• Tighten the corrected JSON wording so it distinguishes manifest-backed review output from the manifest-less no-files path, requires callers to tolerate both summary and manifest being absent, and documents tool_calls as unconditional while llm / trace_id remain optional.
• Documentation only: five Markdown files. No Go, TypeScript, config, workflow, or dependency changes.

Drafted against main @ e967f3f4cf40e2f1e1154117763382b24bd608e4.

Full investigation, code citations, measured output, and the original problem report are in #1189.

Why the old text was wrong

1. review --format json never emits a bare array

The old FAQ said that a normal review with zero comments produces a regular empty array:

[]

That output shape is not produced by the normal review JSON path.

outputJSONWithWarnings serializes a jsonOutput struct. comments is a field of that object, not the JSON document itself.

The existing tests already pin this behavior:

• TestEmitRunResult_JSONHasNoReportText decodes stdout as JSON and verifies that exactly one JSON document is present.
• TestOutputJSON_NoComments unmarshals zero-comment output into jsonOutput, which would fail if stdout were a bare array.

So review --format json writes one JSON object to stdout, with comments: [] when there are no findings.

2. files_reviewed is not a top-level field

The old FAQ heading suggests:

{
  "files_reviewed": 0,
  "comments": []
}

On manifest-backed review output, files_reviewed lives under summary:

summary.files_reviewed

For a manifest-backed review where nothing is eligible, measured output contains:

• status: "skipped"
• summary.files_reviewed: 0
• comments: []
• message: "Review skipped: no items were selected."
• manifest.terminal_state: "skipped"
• empty manifest coverage arrays

The manifest-less no-files path described below omits both summary and manifest, so callers must tolerate both fields being absent.

The heading is intentionally left unchanged for compatibility; see "Compatibility constraints" below.

3. "Nothing to review" and "reviewed with no findings" are semantic states, not array-vs-object shapes

A manifest-backed review that actually examines files and produces zero findings also returns a JSON object with:

comments: []

but its semantic values differ from a manifest-backed review where nothing was eligible:

• summary.files_reviewed reflects the files actually reviewed
• status is "complete"
• manifest.terminal_state is "complete"
• message is "Review complete: 0 finding(s) across N selected item(s)."

The first draft of this correction described these two cases as having "the same object shape". That was still too strong.

jsonOutput has optional top-level fields such as:

• groups
• warnings
• project_summary
• resume
• retry_report
• trace_id

and their presence can differ between runs.

In particular, a no-files review exits before grouping, while a review that actually dispatches files can populate groups.

The stable contract is therefore:

• review --format json writes one JSON object, never a bare array;
• manifest-backed skipped and zero-finding review output can both contain comments: [];
• on manifest-backed output, callers can distinguish those states using semantic fields such as summary.files_reviewed, status, or manifest.terminal_state;
• callers must tolerate both summary and manifest being absent on the manifest-less no-files path; and
• callers should not assume identical sets of optional keys.

4. The leaner output is the manifest-less no-files path

outputJSONNoFiles is reached when the output pipeline has no run manifest and the no-files guard matches.

ocr scan is always manifest-less because the scan agent deliberately returns nil from RunManifest(). ocr review normally produces a run manifest, but it can also reach this path when manifest construction fails and the no-files guard matches.

Measured on the manifest-less no-files path:

• status is "skipped"
• message is "No supported files changed."
• comments is []
• both summary and manifest are absent
• tool_calls is always present
• optional metadata such as llm or trace_id may be present

The corrected FAQ describes these field semantics without presenting a selected field subset as the exact complete JSON shape.

5. The plan-threshold cost lever was reversed

The old FAQ said:

Lowering the thresholds reduces cost.

Template.PlanRequired does the opposite for positive thresholds: planning is enabled when the changed-line count reaches a threshold.

Conceptually:

if maxFileChanged >= PLAN_MODE_LINE_THRESHOLD {
    return true
}

if fileCount >= 2 &&
   PLAN_MODE_GROUP_LINE_THRESHOLD > 0 &&
   totalChanged >= PLAN_MODE_GROUP_LINE_THRESHOLD {
    return true
}

Therefore:

• raising a positive threshold makes the condition harder to satisfy, so fewer groups take the extra plan LLM call;
• lowering a positive threshold makes planning trigger more often and generally costs more.

This also fixes a contradiction with the existing "Plan phase took forever and the file is small" FAQ entry, which already describes planning as switching on when a threshold is reached.

6. The two plan thresholds are asymmetric at zero

This could not be fixed by merely swapping "lowering" and "raising".

PLAN_MODE_LINE_THRESHOLD <= 0 hits the early return and means:

always plan

That is the most expensive setting for this gate.

By contrast:

PLAN_MODE_GROUP_LINE_THRESHOLD == 0

fails the group's > 0 guard and disables the group-total trigger.

That can save a plan call when the group-total gate would otherwise have been the only trigger.

It does not necessarily save a call when the per-file threshold is already satisfied.

Both zero cases are covered by named tests:

• "per-file threshold zero means always plan"
• "group threshold zero disables group gate"

What Changed

English: JSON output entry

The corrected entry now distinguishes manifest-backed review output from the manifest-less no-files path:

Nothing was eligible to review. `files_reviewed` is not a top-level
field — it sits under `summary`, where it reads `0` on this path, while
`comments` is the top-level `[]`. The same object carries
`"status": "skipped"`, `"message": "Review skipped: no items were
selected."`, and a `manifest` whose `terminal_state` is `"skipped"` with
every `coverage` array empty.

A review that examined files and found nothing also returns **a JSON
object with `comments: []`**: `summary.files_reviewed` counts the files
actually reviewed, `status` is `"complete"`, and `message` reads
`"Review complete: 0 finding(s) across N selected item(s)."`. Optional
top-level fields can differ between runs, so do not distinguish these
states by object shape or by the presence of an optional key. On
manifest-backed review output, use `summary.files_reviewed` or
`manifest.terminal_state` instead. Callers must still tolerate both
`summary` and `manifest` being absent on the manifest-less path described
below. `review --format json` always writes exactly one JSON object to
stdout, never a bare array.

The manifest-less no-files path is leaner: it omits both `summary` and
`manifest`, while still reporting `"status": "skipped"`, `"message": "No
supported files changed."`, and `"comments": []`. `tool_calls` is always
present. `ocr scan` is always manifest-less; when its no-files guard
matches it uses this path. `ocr review` can also reach the same path when
manifest construction fails and the no-files guard matches. Optional
metadata such as `llm` or `trace_id` may be present.

The same contract is stated across all five maintained locales.

English: plan-cost entry

The corrected cost guidance says:

Plan phase is on for groups whose largest file is ≥ 50 lines, or whose
2+ files total ≥ 100 lines. It costs an extra LLM call per group, so
raising those thresholds is what makes a run cheaper; lowering them
sends more groups through planning and costs more. The two do not
behave alike at zero. `PLAN_MODE_LINE_THRESHOLD` at `0` or below means
always plan — the dearest setting available — whereas
`PLAN_MODE_GROUP_LINE_THRESHOLD` at `0` turns the group gate off. That
can save the plan call when the group gate would otherwise be the only
trigger. See "Plan phase took forever and the file is small" above for
the trigger rules.

The same corrections are applied in translation to zh, ja, ko, and ru.

Each locale keeps its existing style rather than being mechanically back-translated from English.

Compatibility Constraints

Headings are intentionally unchanged

The affected FAQ headings remain byte-for-byte unchanged in all five locales.

The Korean entry has an explicit anchor:

{#json-output-is-filesreviewed-0-comments}

The other locales derive their heading IDs from the heading text through the Pages heading-ID machinery.

Renaming those headings would silently change existing fragment URLs and could break inbound links. The heading therefore still names the historical/mistaken JSON shape even though the body immediately explains the real structure.

This cannot be repaired later by simply adding an equivalent legacy {#...} alias: the explicit heading-ID parser accepts only ASCII identifiers, while generated fragments for the existing Chinese and Japanese headings preserve CJK characters. A safe rename needs broader heading-ID support or a fragment-redirect mechanism before the localized headings can change.

The plan-phase cross-reference remains textual

The cost entry refers to the earlier "Plan phase took forever and the file is small" entry by name instead of adding a relative #... link.

The localized headings generate different non-ASCII fragments. Pages also has special handling because marked percent-encodes non-ASCII hrefs while generated heading IDs remain raw and DocsPage.tsx decodes the fragment before lookup.

Keeping the cross-reference textual avoids introducing five additional locale-specific anchor cases into this documentation fix.

Type of Change

• Bug fix (non-breaking change that fixes an issue)
• New feature (non-breaking change that adds functionality)
• Breaking change
• Refactoring
• Documentation update
• CI / Build / Tooling

How Has This Been Tested?

Validation Environment

Initial full local validation was performed against:

• Host: Windows 11 Pro, amd64
• Node.js: v24.14.0
• npm: 11.9.0
• Go: go1.26.5 windows/amd64
• Baseline: e967f3f4cf40e2f1e1154117763382b24bd608e4 (origin/main)
• Existing pages/node_modules dependency tree, unchanged

Subsequent commits only tightened wording in the same five Markdown files after additional review found over-broad descriptions of the JSON contract. They introduce no executable, configuration, or dependency changes.

Pages validation

The following Pages gates were run locally on the initial documentation change:

• npm run lint — exit 0

  • two pre-existing react-hooks/exhaustive-deps warnings in HeroSection.tsx and MarkdownRenderer.tsx
  • neither file is touched here

• npm test — 8 files, 38 tests, all passing

• npm run typecheck — exit 0

• production webpack build — exit 0

• npm run size — 95.13 kB brotli against the 150 kB limit

The production build was run as:

NODE_ENV=production npx webpack --mode production

under Git Bash.

The repository's npm run build command itself is POSIX-oriented and does not run directly in the same Windows environment. That separate build-script portability issue is documented in #1181 and is intentionally not addressed here.

Go behavior validation

The packages whose behavior is described by this documentation were tested:

go test ./cmd/opencodereview/... ./internal/config/template/...

Both passed.

These cover the two relevant implementation areas:

• CLI JSON output behavior
• Template.PlanRequired

No Go file is modified by this PR.

Render verification

The FAQ is rendered through the Pages Markdown pipeline:

.md
  -> asset/source
  -> marked
  -> MarkdownRenderer
  -> DOMPurify

rather than being compiled as MDX.

The rewritten sections were rendered through the project parser to verify that:

• inline JSON renders inside <code>;
• Markdown emphasis renders correctly;
• the affected heading text remains unchanged;
• the plan-cost paragraph remains a single list item;
• the prose-only follow-up corrections introduce no new Markdown construct.

Behavioral evidence

The documentation was checked against runtime output rather than relying only on source inspection.

Manifest-backed ocr review with nothing eligible

A freshly built binary was run in a repository whose only staged change was a Markdown file, which OCR excludes.

ocr review --format json --audience agent produced:

• exit code 0
• exactly one JSON document on stdout
• summary.files_reviewed: 0
• comments: []
• status: "skipped"
• message: "Review skipped: no items were selected."
• manifest.terminal_state: "skipped"

Manifest-less no-files output

ocr scan --format json --audience agent in the same repository produced the manifest-less no-files path:

• status: "skipped"
• message: "No supported files changed."
• comments: []
• tool_calls present
• no summary
• no manifest

ocr scan is always manifest-less. ocr review can also reach this path when manifest construction fails and the no-files guard matches. tool_calls remains unconditional; only metadata such as llm or trace_id is optional.

Review with files examined and zero findings

The manifest-backed review path reports:

status: "complete"
comments: []
message: "Review complete: 0 finding(s) across N selected item(s)."

rather than the legacy "success" plus "No comments generated. Looks good to me." pairing.

outputJSONWithWarnings initially constructs Status: "success" but replaces it with manifest.TerminalState when a manifest is present.

The review agent normally has a manifest; the "success" pairing survives only on manifest-less legacy paths.

Threshold evidence

The cost guidance was checked against both Template.PlanRequired and its existing tests.

The important cases are:

PLAN_MODE_LINE_THRESHOLD <= 0
=> always plan

and:

PLAN_MODE_GROUP_LINE_THRESHOLD == 0
=> disable the group-total trigger

The second case can avoid the plan call only when the group-total gate would otherwise have been the sole trigger.

OCR Coverage

The required OCR self-review was run as instructed by AGENTS.md.

It selected no files:

[ocr] Summary: 0 file(s) reviewed, 0 comment(s), ~0 token(s) used, 0s elapsed
[ocr] Session: 78b35da2-c943-4f15-a067-d9e61a0aa51a
Review skipped: no items were selected.

ocr review --preview explained why: all five changed FAQ files were classified as unsupported_ext.

The zero-comment OCR result is therefore not treated as evidence that this PR is correct.

The substitute verification is the runtime measurement and existing implementation tests described above.

Incidentally, the skipped self-review printed:

Review skipped: no items were selected.

which is the exact message documented by the corrected FAQ.

Final Validation Notes

• The diff remains limited to five localized FAQ Markdown files.
• No Go, TypeScript, configuration, workflow, or dependency files are modified.
• Heading text is preserved, so existing fragment IDs do not change.
• The corrections are synchronized across all five maintained locales.
• Automated PR CI reruns after each push and is the authoritative validation of the final head.

make check was deliberately not run locally.

It includes repository-wide mutating steps such as go mod tidy and formatting operations over Go files that this documentation-only PR does not touch. Targeted Go tests and the Pages validation were used instead.

license-check is not applicable to the changed Markdown files.

english-check does not scan .md, which is what permits the localized FAQ files to contain their respective languages.

Screenshots

FAQ JSON output

Before:

After:

Plan-threshold cost guidance

Before:

After:

Desktop viewport. The five maintained locales use the same documentation
layout; this PR changes localized prose only, with no component, styling,
navigation, or responsive-layout changes.

Interaction With Other Open PRs

Several other open PRs currently touch the same FAQ files but not the lines changed here:

• fix(review): honor lower max-tools overrides #1118 touches the --max-tools documentation.
• fix(encoding): decode non-UTF-8 source files before review (#987) #1028 and fix(agent): make --preview apply the same selection the review applies #801 both touch the exclusion-reasons table and primarily overlap with each other rather than this PR.
• fix(cmd): return non-zero for partial operational failures #1030 touches exit-code documentation.

These may cause line-number drift when rebasing, but no textual overlap with the sections changed here is currently expected.

Checklist

• Documentation-only change
• Scope is limited to one logical correction
• All five maintained locales are updated
• JSON behavior is checked against runtime output and implementation
• Threshold behavior is checked against implementation and existing tests
• Existing headings and fragment IDs are preserved
• Contributor authorship is preserved
• CLA is signed
• Required before/after Pages screenshots are attached

The current PR's CLA check is authoritative.

For historical context, the same contributor account also passed license/cla on the previously merged #1091.

Known Limitations

• The FAQ headings still display the historical JSON shape even though the body explains that it is not the literal output structure. They are intentionally preserved to avoid silently breaking inbound fragment links. A safe rename for the localized headings needs broader heading-ID support or fragment redirects because explicit IDs are ASCII-only while existing Chinese and Japanese generated fragments preserve CJK.
• The concrete skipped and zero-finding review examples describe manifest-backed output. Callers must also tolerate the manifest-less no-files fallback, where both summary and manifest are absent.
• The plan-phase cross-reference is textual rather than an anchor link. If that entry is renamed later, all five localized references will need updating.
• No executable change is proposed. outputJSONNoFiles, outputJSONWithWarnings, manifestMessage, and PlanRequired already implement the intended behavior; only the prose describing them was wrong.

Related Issues

Closes #1189.

[github-actions]

✅ OpenCodeReview: Review skipped: no items were selected.

[Qiyuanqiii]

还在检查和修改

[lizhengfeng101]

Nice piece of work — I checked every claim against the source and they all hold up: jsonOutput is a struct so a bare array is impossible, files_reviewed really does live under summary, and PlanRequired in internal/config/template/template.go:69 confirms the threshold direction was backwards (plus the asymmetry at zero). Three things before this goes in:

1. ru/faq.md lost its trailing newline. The last line of the "Related" section came back byte-identical but without the final \n:

-- [Телеметрия](../telemetry/) — использование токенов и метрики LLM.
+- [Телеметрия](../telemetry/) — использование токенов и метрики LLM.
\ No newline at end of file

Unrelated to the correction, and nothing in CI will catch it. Worth restoring so the diff really is limited to the two entries.

2. The lean no-files path isn't scan-specific. All five locales attribute it to ocr scan, but the guard in cmd/opencodereview/shared.go:707 keys off manifest == nil, and Agent.RunManifest() documents itself as returning nil "when manifest construction failed" — so ocr review --format json can land on outputJSONNoFiles too, with neither summary nor manifest. That undercuts the advice in the same entry to key off summary.files_reviewed or manifest.terminal_state. Suggest phrasing it as the manifest-less path (always the case for scan, and for review when the manifest couldn't be built) and noting that callers should treat both fields as possibly absent.

3. tool_calls is always present, and only ru says so. ToolCalls has no omitempty and newJSONToolCalls(nil, nil) never returns nil — it backfills the map and slice. The Russian text draws the line correctly ("Также присутствуют обычные поля вроде tool_calls, а необязательные метаданные … могут присутствовать"), while en/zh/ja/ko put tool_calls under the same "may also be present" hedge as llm/trace_id. Given that precision is the whole point of this PR, worth aligning the other four with ru.

For what it's worth, I also checked the heading-anchor argument and I think you're right to leave the headings alone — parseExplicitHeadingId in pages/src/utils/headingId.ts:7 only accepts ASCII ids, while generateHeadingId preserves CJK, so the zh/ja anchors simply can't be pinned with {#...}. That's less of a "future work" item than the description suggests.

[Qiyuanqiii]

2026年9月6日 22：58

** 立正风101 **
评论
2026年9月6日

Thanks for the careful pass — all three points make sense. I'll restore the RU EOF newline, reframe the lean output as the manifest-less path rather than scan-specific, and make tool_calls unconditional in the other four locales. I'll also tighten the heading-anchor note in the PR description.

[Qiyuanqiii]

Thanks for the careful pass — addressed all three points in bb240456:

1. Restored the trailing newline in ru/faq.md.
2. Reframed the lean output as the manifest-less no-files path, including the review fallback when manifest construction fails, and made callers tolerate both summary and manifest being absent.
3. Made tool_calls explicitly unconditional across all five locales, with only llm / trace_id described as optional.

I also kept the existing headings unchanged, per the fragment-compatibility constraint you pointed out.

[lizhengfeng101]

LGTM