|
| 1 | +# Design: readable posted reviews — plain prose; markers follow `review.attribution` |
| 2 | + |
| 3 | +## Problem statement |
| 4 | + |
| 5 | +A review posted by `/review --comment` is written in a template voice rather |
| 6 | +than a reviewer's voice. The worst offender is the inline comment body format |
| 7 | +dictated by the skill: a `— Failure scenario: <trigger> → <wrong outcome>` |
| 8 | +clause with a label and arrow notation no human would type. Template |
| 9 | +scaffolding costs every reader time — the failure scenario is information, |
| 10 | +but the label and the arrows around it are not. |
| 11 | + |
| 12 | +Two other artifacts look like tells but are not: `LGTM! ✅` and the `⚠️` |
| 13 | +glyph are things human reviewers type constantly, and they aid scanning. |
| 14 | +Readability — not concealment — is the criterion, and by that criterion they |
| 15 | +stay. |
| 16 | + |
| 17 | +## Decision: plain prose unconditionally; machine-readable markers follow `review.attribution` |
| 18 | + |
| 19 | +The posted text splits into two layers, and they get different treatment: |
| 20 | + |
| 21 | +- **Phrasing — the `Failure scenario:` label, the `<trigger> → <wrong |
| 22 | +outcome>` arrow notation, the section-header voice** — is template |
| 23 | + scaffolding. Plain sentences carry the same information more readably for |
| 24 | + _every_ audience, including the openly-attributed posts on this |
| 25 | + repository's own PRs. Phrasing goes plain **unconditionally**, in both |
| 26 | + attribution modes. No setting, no register branch: the model writes one |
| 27 | + style. The evidence rule is unchanged — the concrete trigger and wrong |
| 28 | + outcome must be in the sentences; the scaffolding is gone, the evidence |
| 29 | + is not. |
| 30 | +- **Markers — the `**[Critical]**`/`**[Suggestion]**` prefixes and the |
| 31 | + footer** — are machine-readable signals, not prose style: |
| 32 | + `qwen-autofix.yml`'s Critical-only mode greps posted bodies for |
| 33 | + `contains("**[Critical]**")` in a dozen places, and the prefix lets a |
| 34 | + human triage blockers at a glance. They stay when attribution is on and |
| 35 | + are stripped when it is off — attribution already decides whether the |
| 36 | + post identifies itself, so it decides whether the post carries the |
| 37 | + machine contract too. |
| 38 | + |
| 39 | +No new setting. `review.attribution: false` (#8994) now means "post without |
| 40 | +VISIBLE AI attribution": no footer, no visible severity markers. The |
| 41 | +machine contract moves to an invisible severity marker |
| 42 | +(`<!-- qwen-review critical|suggestion -->`) that every unattributed comment |
| 43 | +carries — presubmit dedup and the blocker re-promotion read it — so the |
| 44 | +mode is not signal-free, and that is load-bearing, not an oversight. |
| 45 | + |
| 46 | +Rationale over a separate `review.tone`: two registers would double the |
| 47 | +prompt and test surface for a phrasing that is strictly worse; the only |
| 48 | +honest axis is whether the post carries machine-readable markers, and that |
| 49 | +is exactly what attribution already governs. |
| 50 | + |
| 51 | +## Current state |
| 52 | + |
| 53 | +| Layer | What shapes the posted text | File | |
| 54 | +| ----------------------------------- | ---------------------------------------------------------------------------------------- | ----------------------------------------------------------------- | |
| 55 | +| Finding fields (internal state) | `FINDING_FORMAT` — `File/Anchor/Issue/Failure scenario/…` | `packages/cli/src/commands/review/agent-prompt.ts` | |
| 56 | +| Inline comment body (model-written) | Body format spec | `packages/core/src/skills/bundled/review/SKILL.md` (Step 7) | |
| 57 | +| Comment normalization at post time | Strips forged footers; appends canonical footer (attribution on) | `packages/cli/src/commands/review/submit.ts` | |
| 58 | +| Severity counting | `submit` counts `**[Critical]**` / `**[Suggestion]**` prefixes off the attached comments | `packages/cli/src/commands/review/lib/inline-counts.ts` | |
| 59 | +| Review body (deterministic) | Fixed bilingual copy, `<details>` fold | `packages/cli/src/commands/review/compose-review.ts` | |
| 60 | +| Settings resolution | `operatorReviewSettings()` — operator scopes only | `packages/cli/src/commands/review/lib/review-settings.ts` (#8994) | |
| 61 | + |
| 62 | +Two constraints discovered during investigation: |
| 63 | + |
| 64 | +- **The severity prefix is load-bearing inside the pipeline, not just for |
| 65 | + autofix.** `submit` derives the Critical/Suggestion counts from the comment |
| 66 | + prefixes (the skill forbids the caller from supplying the counts). |
| 67 | + De-prefixing must happen _after_ counting, at the final post transform. |
| 68 | +- **`agent-prompt.ts` needs no change.** Its structured format is internal |
| 69 | + state; only the orchestrator-composed comment bodies and the composed |
| 70 | + review body reach GitHub. |
| 71 | + |
| 72 | +## Proposed changes |
| 73 | + |
| 74 | +`attribution` already flows into `submit` and `compose-review` (#8994 wires |
| 75 | +it). The markers key off that same boolean; the phrasing stops being a |
| 76 | +template at all — no new plumbing anywhere. |
| 77 | + |
| 78 | +### Deterministic (code, unit-tested) |
| 79 | + |
| 80 | +1. **`submit.ts`** — when attribution is off, the posted comment bodies lose |
| 81 | + the leading `**[Critical]**` / `**[Suggestion]**` prefix. The strip |
| 82 | + happens in the final `post` object only: the payload keeps its canonical |
| 83 | + marked shape, so severity counting, the unmarked-comment gate, and the |
| 84 | + ledger all ran on the marked comments before the transform. |
| 85 | +2. **`compose-review.ts`** — body Criticals and the cannot-tell list keep |
| 86 | + their `**[Critical]**` marker when attribution is on (autofix greps it) |
| 87 | + and lose it when off. All other fixed copy is unchanged — `LGTM! ✅` |
| 88 | + and the `⚠️` clauses stay in both modes. |
| 89 | + |
| 90 | +### Known tradeoffs (disclosed, accepted) |
| 91 | + |
| 92 | +- The HTML-comment ledger marker (`<!-- qwen-review-ledger … -->`) still |
| 93 | + rides posted review bodies — invisible when rendered, but present in the |
| 94 | + markdown source. It is how the next review round recovers this round's |
| 95 | + findings; dropping it would break multi-round re-reviews. It stays. |
| 96 | +- Attribution-off inline comments carry an invisible severity marker |
| 97 | + (`<!-- qwen-review critical -->` / `<!-- qwen-review suggestion -->`) for |
| 98 | + the same reason: it is the one signal that survives the prefix strip and |
| 99 | + the footer removal. `presubmit`'s duplicate detection matches it only |
| 100 | + together with authorship by the reviewing account — the string is public |
| 101 | + and renders invisibly, so an ungated match would let a PR author plant it |
| 102 | + on a line they expect a blocker on and have the next round silently |
| 103 | + withhold that blocker. The "other accounts escape dedup" limitation from |
| 104 | + #8994 therefore stands. `pr-context`'s blocker promotion reads the |
| 105 | + marker's severity, so an unresolved Critical re-enters the re-check |
| 106 | + section every round even without the visible prefix. |
| 107 | +- `qwen-autofix`'s Critical-only mode (engaged after round 5, or earlier when a counting window's diff-growth budget trips) greps posted bodies |
| 108 | + for `**[Critical]**`; attribution-off findings no longer match and are |
| 109 | + deferred as non-Critical. Disclosed in the setting's description. A fix |
| 110 | + (the workflow parsing the severity marker instead) is possible follow-up, |
| 111 | + not this PR. |
| 112 | + |
| 113 | +### Prompt layer (SKILL.md, dogfooded) |
| 114 | + |
| 115 | +3. Step 7's comment-body paragraph drops the labelled template **as the |
| 116 | + only register**: write each description as plain reviewer prose in the |
| 117 | + PR's language — no `Failure scenario:` label, no `→` notation; state the |
| 118 | + problem, when it bites, and the fix in ordinary sentences. The evidence |
| 119 | + rule is unchanged (the concrete trigger and wrong outcome must be in the |
| 120 | + sentences). ` ```suggestion ` blocks stay (human reviewers use them). |
| 121 | + **The payload still carries the canonical prefixed shape** — the prefix |
| 122 | + is the pipeline's counting signal and stripping it is the code's job — |
| 123 | + so the machine-checkable contract is identical in both modes, and a |
| 124 | + model that ignores the prose instruction degrades to a prefixed comment, |
| 125 | + not a miscounted verdict. |
| 126 | + |
| 127 | +## What does not change |
| 128 | + |
| 129 | +- Verdict semantics, severity definitions, exclusion criteria, the reverse |
| 130 | + audit, presubmit, authorization. Presentation only. |
| 131 | +- The fixed review-body copy: `LGTM! ✅`, the `⚠️` clauses, the bilingual |
| 132 | + `<details>中文说明</details>` fold — humans type the first two, and the |
| 133 | + fold is language policy. |
| 134 | +- ` ```suggestion ` blocks. |
| 135 | +- qwen-code's own autofix: `qwen-autofix.yml` keys off prefix + footer, and |
| 136 | + this repository's CI reviews run with attribution on, so every string the |
| 137 | + workflow greps for still appears in its posts. |
| 138 | +- The `parse-args` verdict shape: prose style is not conditional, so the |
| 139 | + orchestrator has nothing to branch on. |
| 140 | + |
| 141 | +## Files affected |
| 142 | + |
| 143 | +| File | Change | |
| 144 | +| ---------------------------------------------------------------------------- | -------------------------------------------------- | |
| 145 | +| `packages/cli/src/commands/review/submit.ts` | Prefix strip in the attribution-off post transform | |
| 146 | +| `packages/cli/src/commands/review/compose-review.ts` | Body-list markers follow attribution | |
| 147 | +| `packages/cli/src/commands/review/lib/inline-counts.ts` | `stripSeverityPrefix` beside `severityOf` | |
| 148 | +| `packages/core/src/skills/bundled/review/SKILL.md` | Plain-prose body format as the only register | |
| 149 | +| `docs/users/configuration/settings.md`, `docs/users/features/code-review.md` | Widen `review.attribution` description | |
| 150 | +| `packages/cli/src/config/settingsSchema.ts` + regenerated IDE schema | Attribution description widened (no new key) | |
| 151 | +| Collocated `*.test.ts` | Pin both modes; fixed copy identical in each | |
| 152 | + |
| 153 | +Base branch: `pr-8994` (the setting this couples to exists only there). |
| 154 | + |
| 155 | +## Scope boundaries |
| 156 | + |
| 157 | +- No change to finding _content_ policy — only to how posted text reads. |
| 158 | +- No new settings key (the existing `review.attribution` description is |
| 159 | + widened); no settingsSchema shape change. |
| 160 | +- Attribution-off posts from other accounts remain undetectable to |
| 161 | + presubmit dedup (already documented in #8994); prefix stripping does not |
| 162 | + change that. |
| 163 | + |
| 164 | +## Open questions |
| 165 | + |
| 166 | +- None blocking. |
0 commit comments