From 1accf9366538cebdd8b9886abc939e5d241fa929 Mon Sep 17 00:00:00 2001 From: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> Date: Tue, 27 Oct 2026 02:47:37 +0800 Subject: [PATCH] fix(review): support readable bilingual review bodies Signed-off-by: LoopX Agent <337587101+loopx-agent@users.noreply.github.com> --- examples/pr-review-command-smoke.py | 23 +++++++++-- loopx/capabilities/pr_review_queue/README.md | 22 +++++++--- .../pr_review_queue/review_body.py | 23 ++++++++--- .../pr_review_queue/review_contract.py | 14 +++++-- skills/loopx-pr-review/SKILL.md | 26 +++++++++--- skills/loopx-pr-review/agents/openai.yaml | 2 +- tests/capabilities/test_pr_review_body.py | 41 +++++++++++++++++++ tests/capabilities/test_pr_review_contract.py | 14 +++++++ 8 files changed, 140 insertions(+), 25 deletions(-) diff --git a/examples/pr-review-command-smoke.py b/examples/pr-review-command-smoke.py index 255a042d9c..c4f0c48f07 100644 --- a/examples/pr-review-command-smoke.py +++ b/examples/pr-review-command-smoke.py @@ -18,6 +18,7 @@ from loopx.capabilities.project_skill_delivery.core import ( # noqa: E402 discover_project_scoped_skill_ids, ) +from loopx.capabilities.pr_review_queue.review_body import check_review_body # noqa: E402 from loopx.doctor import REQUIRED_INSTALLED_SKILL_PHRASES # noqa: E402 from loopx.pr_review import ( # noqa: E402 _github_search_date, @@ -107,9 +108,6 @@ def main() -> int: "Full PR Review And Bilingual Format", "findings-only or blocker-only body is incomplete", "Cover every changed surface and key symbols", - "详细中文评审", - "英文简短结论", - "complete Chinese five-block review plus one concise English verdict", "review_execution_contract.verdict_policy", "validation_matrix", "validation_source", @@ -128,7 +126,10 @@ def main() -> int: "--target-exact-head NUMBER@HEAD_OID", ): assert phrase in skill_text, phrase - assert len(skill_source.splitlines()) <= 180, len(skill_source.splitlines()) + # The adapter grows from 165/11,818 to 181/13,018 lines/UTF-8 bytes to + # preserve body-edit authority and compatibility obligations. Keep it thin + # without removing those clauses to fit the former 180-line budget. + assert len(skill_source.splitlines()) <= 190, len(skill_source.splitlines()) for duplicated_contract_heading in ( "Per-PR Evidence And Depth Gate", "Motivation Causal Chain", @@ -637,6 +638,20 @@ def approved_open_head( assert section["agent_instruction"], section assert "quota.py" not in section["agent_instruction"], section assert all(section["minimum_prose_characters"] > 0 for section in template["sections"]) + preferred_titles = ["动机 / Motivation", "改动思路 / Approach", "具体改动 / Changes", + "对主干的风险 / Risks", "我的整体评价 / Verdict"] + for section, title in zip(template["sections"], preferred_titles, strict=True): + assert title in section["agent_instruction"], section + body_head = "a" * 40 + canonical_body = (REPO_ROOT / "examples/fixtures/pr-review.body.md").read_text( + encoding="utf-8").replace("HEAD_OID", body_head).replace("VERDICT", "APPROVE") + bilingual_body = canonical_body + for label, title in zip(labels, preferred_titles, strict=True): + bilingual_body = bilingual_body.replace(f"## {label}\n", f"## {title}\n") + canonical_check = check_review_body(canonical_body, head_oid=body_head, behavior_bearing=True) + assert canonical_check["valid"], canonical_check + assert check_review_body(bilingual_body, head_oid=body_head, + behavior_bearing=True) == canonical_check concrete_change = next( section for section in template["sections"] if section["label"] == "具体改动" ) diff --git a/loopx/capabilities/pr_review_queue/README.md b/loopx/capabilities/pr_review_queue/README.md index 55649ceec1..e5a22d1ab8 100644 --- a/loopx/capabilities/pr_review_queue/README.md +++ b/loopx/capabilities/pr_review_queue/README.md @@ -773,13 +773,23 @@ starts with `/loopx-pr-review`, words such as `open`, `closed`, `merged`, skip the review. Downgrade only for explicit opt-out phrases such as `只统计`, `只列出`, `stats only`, `list only`, `不要 review`, or `不用分析`. -The published review is a full-PR bilingual review: one complete Chinese -five-block review covering every changed surface, key symbol, positive and -negative path, and validation, plus one concise English machine verdict +The published review is a full-PR bilingual review: five complete sections with +English structure and concise Chinese explanations covering every changed +surface, key symbol, positive and negative path, and validation, plus one concise English machine verdict (`APPROVE`, `REQUEST_CHANGES`, or the author-owned `COMMENTED` fallback). The -Chinese review carries the depth and evidence; the English verdict carries the -machine-readable state and validation summary. A findings-only or blocker-only -body is not a complete PR review. +body carries the depth and evidence; the English verdict carries the +machine-readable state and validation summary. Prefer `动机 / Motivation`, +`改动思路 / Approach`, `具体改动 / Changes`, `对主干的风险 / Risks` and +`我的整体评价 / Verdict`. These map to the same Chinese canonical section ids; +existing headings and evidence floors remain valid. Older installed parsers can +use Chinese headings with English subheadings until upgraded. + +Use an informative English finding title, followed by short Problem / Evidence / +Impact / Fix paragraphs. Mark validation PASS / FAIL / UNVERIFIED, using compact +tables for actual comparisons. Keep symbols, commands and standard technical +terms in English. Do not mechanically translate each sentence, impose a language +ratio or delete evidence to make the text shorter. This is writing guidance, +not a new language gate. A findings-only or blocker-only body is incomplete. The opening `动机` must stand alone for a reader unfamiliar with the repository: who encounters the problem, one triggering task and concrete before/after diff --git a/loopx/capabilities/pr_review_queue/review_body.py b/loopx/capabilities/pr_review_queue/review_body.py index f643f1f24c..48f4b395ed 100644 --- a/loopx/capabilities/pr_review_queue/review_body.py +++ b/loopx/capabilities/pr_review_queue/review_body.py @@ -6,7 +6,16 @@ from collections.abc import Iterator from typing import Any -REQUIRED_FINAL_SECTIONS = ["动机", "改动思路", "具体改动", "对主干的风险", "我的整体评价"] +# Presentation aliases share the existing canonical section ids. They do not +# broaden evidence matching to arbitrary headings that contain a Chinese word. +REVIEW_SECTION_TITLES = { + "动机": "动机 / Motivation", + "改动思路": "改动思路 / Approach", + "具体改动": "具体改动 / Changes", + "对主干的风险": "对主干的风险 / Risks", + "我的整体评价": "我的整体评价 / Verdict", +} +REQUIRED_FINAL_SECTIONS = list(REVIEW_SECTION_TITLES) FENCE_OPEN = re.compile(r"^ {0,3}(`{3,}|~{3,})(.*)$") FENCE_CLOSE = re.compile(r"^ {0,3}(`{3,}|~{3,})[ \t]*$") AUTHOR_OWNED_APPROVAL_FALLBACK_TITLE = ( @@ -122,11 +131,13 @@ def _review_sections(body: str) -> tuple[dict[str, list[str]], list[str]]: heading = re.match(r"^(#{1,6})\s+(.+?)\s*#*\s*$", stripped) if heading: level, label = len(heading[1]), heading[2] - if label in REQUIRED_FINAL_SECTIONS: - if label in sections: - reasons.append(f"duplicate_section:{label}") - sections.setdefault(label, []) - current, section_level = label, level + canonical = next((section for section, title in REVIEW_SECTION_TITLES.items() + if label in (section, title)), None) + if canonical is not None: + if canonical in sections: + reasons.append(f"duplicate_section:{canonical}") + sections.setdefault(canonical, []) + current, section_level = canonical, level elif level <= section_level: current = None continue diff --git a/loopx/capabilities/pr_review_queue/review_contract.py b/loopx/capabilities/pr_review_queue/review_contract.py index 995e05fda8..950a9f6187 100644 --- a/loopx/capabilities/pr_review_queue/review_contract.py +++ b/loopx/capabilities/pr_review_queue/review_contract.py @@ -8,6 +8,7 @@ AUTHOR_OWNED_APPROVAL_FALLBACK_TITLE, AUTHOR_OWNED_REQUEST_CHANGES_FALLBACK_TITLE, REQUIRED_FINAL_SECTIONS, + REVIEW_SECTION_TITLES, review_body_requirements, ) from .approval_closeout import approval_closeout_contract @@ -15,7 +16,7 @@ from .visual_assessment import VISUAL_ASSESSMENT # Increment when review requirements change without changing the packet shape. -REVIEW_POLICY_REVISION = 24 +REVIEW_POLICY_REVISION = 25 # Agent-consumed prose is observable behavior even when JSON shape is stable. # Reuse the existing semantic verdicts; this is evidence within the same review. @@ -68,7 +69,7 @@ "Keep labels public-safe and readable; inspect the rendered diagram before publication, " "or use the prose/table fallback and disclose that rendering was not verified. " "A diagram is optional presentation guidance, never a new required result field, evidence " - "receipt, approval condition or substitute for findings, validation, the complete Chinese " + "receipt, approval condition or substitute for findings, validation, the complete bilingual " "five-section review and English verdict. Code blocks still do not satisfy explanatory " "section floors. Reuse the existing Markdown surface; do not create an image service, " "upload private artifacts or infer publication/merge authority." @@ -403,7 +404,7 @@ def _section(label: str, minimum: int, instruction: str) -> dict[str, Any]: "word_hint": f"至少 {minimum} 个正文字符；不含标题、链接地址、代码和重复行", "minimum_prose_characters": minimum, "content": "", - "agent_instruction": instruction, + "agent_instruction": f"Prefer the heading `## {REVIEW_SECTION_TITLES[label]}`. " + instruction, } @@ -451,6 +452,13 @@ def build_review_template(item: Mapping[str, Any]) -> dict[str, Any]: "Render the verified structured result using the five sections. " "Open the body with one `Reviewer:` line (reviewer_declaration.body_marker) " "carrying result.reviewer's actor_kind, model and provider. " + "Use English throughout the body as useful structure, with short Chinese explanations: " + "bilingual section headings, an informative English finding title, and Problem / Evidence / " + "Impact / Fix within each finding. Keep symbols, commands and standard technical terms in " + "English; explain unfamiliar terms once. Use PASS / FAIL / UNVERIFIED for validation, and " + "a compact table only for genuinely comparable observations. Do not translate every sentence " + "twice, impose a language ratio, repeat conclusions or remove evidence to reduce density. " + "This is writing guidance, not a new language quota or an exception to any evidence gate. " "The capability-owned review_execution_contract is the evidence and completeness authority. Save the exact final Markdown in result.review_body before check-result; publish that checked body and read it back. Section floors reject empty shells, not certify reasoning. Explain concrete paths and counterexamples; do not pad or duplicate evidence to meet a floor." " Follow visual_explanation when a diagram or table improves understanding; retain the complete prose review." ), diff --git a/skills/loopx-pr-review/SKILL.md b/skills/loopx-pr-review/SKILL.md index f966818fe7..d4401b0072 100644 --- a/skills/loopx-pr-review/SKILL.md +++ b/skills/loopx-pr-review/SKILL.md @@ -1,6 +1,6 @@ --- name: loopx-pr-review -description: Use for `/loopx-pr-review` or evidence-backed PR queue review. Run `loopx pr-review` first, execute the capability-owned review plan for each selected exact head, then publish full bilingual PR reviews (complete Chinese five-block review plus one concise English verdict) that match the verified findings. Use `loopx-pr-merge` for approval or merge actions. +description: Use for `/loopx-pr-review` or evidence-backed PR queue review. Run `loopx pr-review` first, execute the capability-owned review plan for each selected exact head, then publish complete reviews with English structure, concise Chinese explanation and an English verdict matching the verified findings. Use `loopx-pr-merge` for approval or merge actions. --- # LoopX PR Review @@ -59,7 +59,17 @@ names the contract the change is judged against. - Review inside that frame; cite the document and check its rows, gates, or exit criteria one by one at the exact head, attaching each finding to its row. - Name unsatisfied rows as missing frame items with their own evidence; keep the declared boundary (preview versus promotion, milestone entry versus merge). -- Publish the frame-aligned conclusion as a PR comment citing that guidance, since a published review body cannot be edited. +- Publish the frame-aligned conclusion through the current exact-head workflow, citing that guidance. + +For a writing-only correction to your own published summary, GitHub supports +[updating the review body](https://docs.github.com/en/rest/pulls/reviews#update-a-review-for-a-pull-request). +Preserve the original body and evidence first, recheck its unchanged commit and +current head, and check the revised complete body. Update only `body` on the +existing review, then read back its id, commit, state and exact text and rerun +check-result. This preserves the review instead of posting a duplicate formal +conclusion for layout. It grants no authority to edit another review, change a +verdict, dismiss findings or treat an old-head finding as current. A changed head +or substantive conclusion follows the normal review workflow. ## Execute One Review Plan @@ -131,9 +141,15 @@ bypass never overrides this gate, and author fallback needs user authority. Use each PR's current `review_template`, `reviewer_declaration` and `completion_gate.required_final_sections` to render a standalone full-PR review. -The **详细中文评审** covers the entire PR in five sections (`动机`, `改动思路`, -`具体改动`, `对主干的风险`, `我的整体评价`); a findings-only or blocker-only body is -incomplete. Cover every changed surface and key symbols according to the current +Use the packet's bilingual headings (`动机 / Motivation`, `改动思路 / Approach`, +`具体改动 / Changes`, `对主干的风险 / Risks`, `我的整体评价 / Verdict`), with short +Chinese explanations and useful English structure throughout the body. Existing +Chinese-only anchors remain valid; older installed checkers need those anchors +with English subheadings until the shared parser is upgraded. Keep informative +English finding titles and Problem / Evidence / Impact / Fix paragraphs, and +PASS / FAIL / UNVERIFIED validation states. Avoid mechanical sentence-by-sentence +translation, language ratios, dense paragraphs and repeated conclusions. +A findings-only or blocker-only body is incomplete. Cover every changed surface and key symbols according to the current plan, not just the main finding. Open with the visible `body_marker` matching `result.reviewer`. The **英文简短结论** starts with `English verdict:` and the bare token `APPROVE` or `REQUEST_CHANGES`, followed by the head, finding and validation. diff --git a/skills/loopx-pr-review/agents/openai.yaml b/skills/loopx-pr-review/agents/openai.yaml index 8dabb96d17..0a2dcb6058 100644 --- a/skills/loopx-pr-review/agents/openai.yaml +++ b/skills/loopx-pr-review/agents/openai.yaml @@ -1,4 +1,4 @@ interface: display_name: "LoopX PR Review" short_description: "Review LoopX PRs with deep key-code explanations" - default_prompt: "Use $loopx-pr-review to obtain and execute the current capability-owned exact-head review packet, publish the checked complete Chinese five-block review plus English verdict, and read back the published result." + default_prompt: "Use $loopx-pr-review to execute the current exact-head review packet, publish the checked complete review with English structure and concise Chinese explanations throughout all five sections plus an English verdict, and read back the result." diff --git a/tests/capabilities/test_pr_review_body.py b/tests/capabilities/test_pr_review_body.py index 879c9161c0..262d664060 100644 --- a/tests/capabilities/test_pr_review_body.py +++ b/tests/capabilities/test_pr_review_body.py @@ -107,6 +107,47 @@ def test_duplicate_sections_are_not_merged_into_a_passing_review(): assert "duplicate_section:动机" in result["invalid_reasons"] +def test_bilingual_headings_keep_canonical_evidence_sections_and_prose_floors(): + from loopx.capabilities.pr_review_queue.review_body import review_section_text + + body = review_body() + headings = [("动机", "Motivation"), ("改动思路", "Approach"), ("具体改动", "Changes"), + ("对主干的风险", "Risks"), ("我的整体评价", "Verdict")] + for chinese, english in headings: + body = body.replace(f"## {chinese}\n", f"## {chinese} / {english}\n") + original = check_review_body(review_body(), head_oid=HEAD, behavior_bearing=True) + translated = check_review_body(body, head_oid=HEAD, behavior_bearing=True) + assert translated == original + for chinese, _ in headings: + assert review_section_text(body, chinese) == review_section_text(review_body(), chinese) + + +@pytest.mark.parametrize("heading", ["动机 / Other", "动机 / Motivation / Extra", "Motivation"]) +def test_bilingual_aliases_do_not_accept_arbitrary_or_english_only_headings(heading): + body = review_body().replace("## 动机\n", f"## {heading}\n") + result = check_review_body(body, head_oid=HEAD, behavior_bearing=True) + assert "missing_section:动机" in result["invalid_reasons"] + + +def test_chinese_and_bilingual_alias_cannot_hide_duplicate_or_empty_sections(): + result = check_review_body(review_body() + "\n## 动机 / Motivation\n重复。", + head_oid=HEAD, behavior_bearing=True) + assert "duplicate_section:动机" in result["invalid_reasons"] + body = review_body() + start, end = body.index("## 对主干的风险"), body.index("## 我的整体评价") + body = body[:start] + "## 对主干的风险 / Risks\nFAIL\n" + body[end:] + result = check_review_body(body, head_oid=HEAD, behavior_bearing=True) + assert any(reason.startswith("section_too_short:对主干的风险") + for reason in result["invalid_reasons"]) + + +@pytest.mark.parametrize("wrapper", ["```\n{}\n```", ""]) +def test_bilingual_heading_remains_hidden_in_code_or_comment(wrapper): + body = review_body().replace("## 动机\n", "## 动机 / Motivation\n") + result = check_review_body(wrapper.format(body), head_oid=HEAD, behavior_bearing=True) + assert "missing_section:动机" in result["invalid_reasons"] + + def test_multiple_english_verdict_lines_cannot_hide_a_contradiction(): result = check_review_body(review_body() + "\nEnglish verdict: REQUEST_CHANGES", head_oid=HEAD, behavior_bearing=True) diff --git a/tests/capabilities/test_pr_review_contract.py b/tests/capabilities/test_pr_review_contract.py index 80fbfdc384..6f7980125a 100644 --- a/tests/capabilities/test_pr_review_contract.py +++ b/tests/capabilities/test_pr_review_contract.py @@ -281,6 +281,20 @@ def test_docs_plan_does_not_invent_code_symbols() -> None: assert template["review_order"] == ["docs/design.md"] +def test_bilingual_presentation_keeps_canonical_evidence_and_validation_requirements() -> None: + template = build_review_template(_item(areas={"product_runtime": 1})) + assert [section["label"] for section in template["sections"]] == [ + "动机", "改动思路", "具体改动", "对主干的风险", "我的整体评价"] + assert [section["minimum_prose_characters"] for section in template["sections"]] == [40, 80, 180, 120, 60] + assert "## 动机 / Motivation" in template["sections"][0]["agent_instruction"] + assert "Problem / Evidence / " in template["output_hint"] + assert "PASS / FAIL / UNVERIFIED" in template["output_hint"] + assert "not a new language quota" in template["output_hint"] + # Style is carried by the existing writing hint, not a new evidence bypass. + assert "check-result" in template["output_hint"] + assert "read it back" in template["output_hint"] + + def test_agent_instruction_plan_requires_default_off_user_experience_evidence() -> None: item = _item(areas={"agent_instruction_surface": 1}) item["key_files"] = [