docs/design/review-tone.md
review.attributionA review posted by /review --comment is written in a template voice rather
than a reviewer's voice. The worst offender is the inline comment body format
dictated by the skill: a — Failure scenario: <trigger> → <wrong outcome>
clause with a label and arrow notation no human would type. Template
scaffolding costs every reader time — the failure scenario is information,
but the label and the arrows around it are not.
Two other artifacts look like tells but are not: LGTM! ✅ and the ⚠️
glyph are things human reviewers type constantly, and they aid scanning.
Readability — not concealment — is the criterion, and by that criterion they
stay.
review.attributionThe posted text splits into two layers, and they get different treatment:
Failure scenario: label, the <trigger> → <wrong outcome> arrow notation, the section-header voice — is template
scaffolding. Plain sentences carry the same information more readably for
every audience, including the openly-attributed posts on this
repository's own PRs. Phrasing goes plain unconditionally, in both
attribution modes. No setting, no register branch: the model writes one
style. The evidence rule is unchanged — the concrete trigger and wrong
outcome must be in the sentences; the scaffolding is gone, the evidence
is not.**[Critical]**/**[Suggestion]** prefixes and the
footer — are machine-readable signals, not prose style:
qwen-autofix.yml's Critical-only mode greps posted bodies for
contains("**[Critical]**") in a dozen places, and the prefix lets a
human triage blockers at a glance. They stay when attribution is on and
are stripped when it is off — attribution already decides whether the
post identifies itself, so it decides whether the post carries the
machine contract too.No new setting. review.attribution: false (#8994) now means "post without
VISIBLE AI attribution": no footer, no visible severity markers. The
machine contract moves to an invisible severity marker
(<!-- qwen-review critical|suggestion -->) that every unattributed comment
carries — presubmit dedup and the blocker re-promotion read it — so the
mode is not signal-free, and that is load-bearing, not an oversight.
Rationale over a separate review.tone: two registers would double the
prompt and test surface for a phrasing that is strictly worse; the only
honest axis is whether the post carries machine-readable markers, and that
is exactly what attribution already governs.
| Layer | What shapes the posted text | File |
|---|---|---|
| Finding fields (internal state) | FINDING_FORMAT — File/Anchor/Issue/Failure scenario/… | packages/cli/src/commands/review/agent-prompt.ts |
| Inline comment body (model-written) | Body format spec | packages/core/src/skills/bundled/review/SKILL.md (Step 7) |
| Comment normalization at post time | Strips forged footers; appends canonical footer (attribution on) | packages/cli/src/commands/review/submit.ts |
| Severity counting | submit counts **[Critical]** / **[Suggestion]** prefixes off the attached comments | packages/cli/src/commands/review/lib/inline-counts.ts |
| Review body (deterministic) | Fixed bilingual copy, <details> fold | packages/cli/src/commands/review/compose-review.ts |
| Settings resolution | operatorReviewSettings() — operator scopes only | packages/cli/src/commands/review/lib/review-settings.ts (#8994) |
Two constraints discovered during investigation:
submit derives the Critical/Suggestion counts from the comment
prefixes (the skill forbids the caller from supplying the counts).
De-prefixing must happen after counting, at the final post transform.agent-prompt.ts needs no change. Its structured format is internal
state; only the orchestrator-composed comment bodies and the composed
review body reach GitHub.attribution already flows into submit and compose-review (#8994 wires
it). The markers key off that same boolean; the phrasing stops being a
template at all — no new plumbing anywhere.
submit.ts — when attribution is off, the posted comment bodies lose
the leading **[Critical]** / **[Suggestion]** prefix. The strip
happens in the final post object only: the payload keeps its canonical
marked shape, so severity counting, the unmarked-comment gate, and the
ledger all ran on the marked comments before the transform.compose-review.ts — body Criticals and the cannot-tell list keep
their **[Critical]** marker when attribution is on (autofix greps it)
and lose it when off. All other fixed copy is unchanged — LGTM! ✅
and the ⚠️ clauses stay in both modes.<!-- qwen-review-ledger … -->) still
rides posted review bodies — invisible when rendered, but present in the
markdown source. It is how the next review round recovers this round's
findings; dropping it would break multi-round re-reviews. It stays.<!-- qwen-review critical --> / <!-- qwen-review suggestion -->) for
the same reason: it is the one signal that survives the prefix strip and
the footer removal. presubmit's duplicate detection matches it only
together with authorship by the reviewing account — the string is public
and renders invisibly, so an ungated match would let a PR author plant it
on a line they expect a blocker on and have the next round silently
withhold that blocker. The "other accounts escape dedup" limitation from
#8994 therefore stands. pr-context's blocker promotion reads the
marker's severity, so an unresolved Critical re-enters the re-check
section every round even without the visible prefix.qwen-autofix's Critical-only mode (engaged after round 5, or earlier when a counting window's diff-growth budget trips) greps posted bodies
for **[Critical]**; attribution-off findings no longer match and are
deferred as non-Critical. Disclosed in the setting's description. A fix
(the workflow parsing the severity marker instead) is possible follow-up,
not this PR.Failure scenario: label, no → notation; state the
problem, when it bites, and the fix in ordinary sentences. The evidence
rule is unchanged (the concrete trigger and wrong outcome must be in the
sentences). ```suggestion blocks stay (human reviewers use them).
The payload still carries the canonical prefixed shape — the prefix
is the pipeline's counting signal and stripping it is the code's job —
so the machine-checkable contract is identical in both modes, and a
model that ignores the prose instruction degrades to a prefixed comment,
not a miscounted verdict.LGTM! ✅, the ⚠️ clauses, the bilingual
<details>中文说明</details> fold — humans type the first two, and the
fold is language policy.```suggestion blocks.qwen-autofix.yml keys off prefix + footer, and
this repository's CI reviews run with attribution on, so every string the
workflow greps for still appears in its posts.parse-args verdict shape: prose style is not conditional, so the
orchestrator has nothing to branch on.| File | Change |
|---|---|
packages/cli/src/commands/review/submit.ts | Prefix strip in the attribution-off post transform |
packages/cli/src/commands/review/compose-review.ts | Body-list markers follow attribution |
packages/cli/src/commands/review/lib/inline-counts.ts | stripSeverityPrefix beside severityOf |
packages/core/src/skills/bundled/review/SKILL.md | Plain-prose body format as the only register |
docs/users/configuration/settings.md, docs/users/features/code-review.md | Widen review.attribution description |
packages/cli/src/config/settingsSchema.ts + regenerated IDE schema | Attribution description widened (no new key) |
Collocated *.test.ts | Pin both modes; fixed copy identical in each |
Base branch: pr-8994 (the setting this couples to exists only there).
review.attribution description is
widened); no settingsSchema shape change.