From b9c82372d7d5aab90cef9888634bc08083b259c8 Mon Sep 17 00:00:00 2001 From: William Aaron Cheung Date: Tue, 28 Jul 2026 16:19:49 +0800 Subject: [PATCH] feat(pr-review): collapse answered open questions Publish each open question inside a
block that starts expanded, and collapse it when a later round marks it answered or withdrawn. Once a question is closed its why-it-matters and how-to-verify bullets are only history, so it reads as a single green summary line that still expands to the original question rather than losing it. The in-place rewrite now swaps the two-line details/summary header instead of a single line. It still reproduces the marker, so re-applying stays a no-op, and a question published before this shape existed falls back to the old single-line rewrite. --- .github/actions/README.md | 12 +- .../claude-pr-review/review_pipeline.py | 63 +++++++-- .../claude-pr-review/test_review_pipeline.py | 124 ++++++++++++++++++ 3 files changed, 183 insertions(+), 16 deletions(-) diff --git a/.github/actions/README.md b/.github/actions/README.md index 4d4cf8d..71b6bca 100644 --- a/.github/actions/README.md +++ b/.github/actions/README.md @@ -96,12 +96,16 @@ Rejected candidates and an empty internal analysis remain invisible. Open questions have the same durable lifecycle as findings. Each one gets a stable ID and a hidden marker on its status line, and the manifest records which review asked it. +Each question is published inside a `
` block that starts expanded. A later round dispositions every open question as `open`, `answered`, or `withdrawn`, and the -publisher edits the original review body in place so the question line reads -`โœ… **Answered**` or `๐Ÿšซ **Withdrawn**` with a one-line reason. +publisher edits the original review body in place so the summary line reads +`โœ… **Answered**` or `๐Ÿšซ **Withdrawn**` with a one-line reason, and the block collapses. +The rationale bullets are kept rather than deleted, so an answered question stays one green +line that expands to the original question, why it mattered, and how to verify it. Editing a submitted review body creates no new review and no new notification, and rewriting -reproduces the same marker line, so the update is idempotent and retried on the next round if -GitHub rejects it. +reproduces the same header (marker included), so the update is idempotent and retried on the +next round if GitHub rejects it. +Questions published before the `
` shape existed fall back to a single-line rewrite. A question that is already open is never re-asked; the original stays the copy the author answers. diff --git a/.github/actions/claude-pr-review/review_pipeline.py b/.github/actions/claude-pr-review/review_pipeline.py index de7ed11..3144554 100644 --- a/.github/actions/claude-pr-review/review_pipeline.py +++ b/.github/actions/claude-pr-review/review_pipeline.py @@ -1388,15 +1388,36 @@ def question_headline(question: dict[str, Any]) -> str: return f"{headline} {marker}" +def question_block_header(question: dict[str, Any]) -> str: + """Render the two-line `
` header that opens a question block. + + An unanswered question stays expanded; a closed one collapses to its + summary line, so the rationale bullets stop competing for attention once + they are only history. `annotate_questions` swaps this header in place, + and because the replacement reproduces the same shape (marker included), + re-applying it is a no-op. + """ + expanded = " open" if str(question.get("status") or "open") == "open" else "" + return ( + f"\n" + f"{question_headline(question)}" + ) + + def render_question(question: dict[str, Any]) -> str: text = public_text(question["question"], maximum=500) why = public_text(question["why_it_matters"], maximum=600) verify = public_text(question["verification"], maximum=600) + # Blank lines around the body: GitHub only renders Markdown inside + #
when it is separated from the surrounding HTML tags. return ( - f"{question_headline(question)}\n" + f"{question_block_header(question)}\n" + f"\n" f"- {text}\n" f"- Why it matters: {why}\n" - f"- How to verify: {verify}" + f"- How to verify: {verify}\n" + f"\n" + f"
" ) @@ -1734,6 +1755,9 @@ def compile_review( "question_id": item_id, "review_id": item["review_id"], "headline": question_headline({**item, "question_id": item_id}), + "block_header": question_block_header( + {**item, "question_id": item_id} + ), } for item_id, item in sorted(question_state.items()) if isinstance(item, dict) @@ -2399,20 +2423,35 @@ def annotate_questions( updated = body outcomes: dict[str, str] = {} for annotation in items: - pattern = re.compile( - "^.*" + re.escape(question_marker(annotation["question_id"])) - + ".*$", - re.MULTILINE, - ) - replaced, count = pattern.subn( - lambda _match, line=annotation["headline"]: line, - updated, + marker = re.escape(question_marker(annotation["question_id"])) + # Current shape first, then the pre-collapse single-line shape, so + # a question published by an older pipeline still gets annotated. + candidates = ( + ( + re.compile( + r"^]*>\n.*" + marker + + r".*$", + re.MULTILINE, + ), + annotation.get("block_header") or annotation["headline"], + ), + ( + re.compile("^.*" + marker + ".*$", re.MULTILINE), + annotation["headline"], + ), ) + count = 0 + for pattern, replacement in candidates: + replaced, count = pattern.subn( + lambda _match, line=replacement: line, + updated, + ) + if count: + updated = replaced + break outcomes[annotation["question_id"]] = ( "applied" if count else "unavailable" ) - if count: - updated = replaced if updated != body: try: if before_write is not None: diff --git a/.github/actions/claude-pr-review/test_review_pipeline.py b/.github/actions/claude-pr-review/test_review_pipeline.py index 0696ffe..5f4baaf 100644 --- a/.github/actions/claude-pr-review/test_review_pipeline.py +++ b/.github/actions/claude-pr-review/test_review_pipeline.py @@ -1406,6 +1406,7 @@ def test_answered_question_produces_a_pending_annotation(self): "question_id": question_id_value, "review_id": 42, "headline": pipeline.question_headline(question), + "block_header": pipeline.question_block_header(question), } ], ) @@ -1413,6 +1414,129 @@ def test_answered_question_produces_a_pending_annotation(self): "โœ… **Answered**", payload["question_annotations"][0]["headline"], ) + # Answered means collapsed: no `open` attribute on the details block. + self.assertTrue( + payload["question_annotations"][0]["block_header"].startswith( + "
" + ) + ) + + def test_open_question_renders_expanded(self): + payload = pipeline.compile_review(review_input(), question_output()) + body = payload["review_body"] + + self.assertIn("
", body) + self.assertIn("โ“ **Open question", body) + self.assertIn("
", body) + self.assertNotIn("
", body) + + @mock.patch.object(pipeline, "gh_json") + def test_answering_collapses_the_question_block(self, gh_mock): + value = review_input() + first = pipeline.compile_review(value, question_output()) + question_id_value = next(iter(first["manifest"]["questions"])) + first["manifest"]["questions"][question_id_value]["review_id"] = 42 + value["manifest"] = first["manifest"] + output = clean_output() + output["prior_questions"] = [ + { + "question_id": question_id_value, + "disposition": "answered", + "reason": "Confirmed unique upstream.", + } + ] + payload = pipeline.compile_review(value, output) + gh_mock.side_effect = [{"body": first["review_body"]}, {"id": 42}] + + pipeline.annotate_questions(payload, payload["manifest"]) + + written = gh_mock.call_args.kwargs["input_value"]["body"] + self.assertIn("
\nโœ… **Answered**", written) + self.assertNotIn("
", written) + # The original question survives collapsed, not deleted. + self.assertIn("- Can the upstream return duplicate records?", written) + self.assertIn("- Why it matters:", written) + self.assertEqual( + payload["manifest"]["questions"][question_id_value]["annotation"], + "applied", + ) + + @mock.patch.object(pipeline, "gh_json") + def test_collapse_annotation_is_idempotent(self, gh_mock): + value = review_input() + first = pipeline.compile_review(value, question_output()) + question_id_value = next(iter(first["manifest"]["questions"])) + first["manifest"]["questions"][question_id_value]["review_id"] = 42 + value["manifest"] = first["manifest"] + output = clean_output() + output["prior_questions"] = [ + { + "question_id": question_id_value, + "disposition": "answered", + "reason": "Confirmed unique upstream.", + } + ] + payload = pipeline.compile_review(value, output) + gh_mock.side_effect = [{"body": first["review_body"]}, {"id": 42}] + pipeline.annotate_questions(payload, payload["manifest"]) + once = gh_mock.call_args.kwargs["input_value"]["body"] + + gh_mock.reset_mock() + gh_mock.side_effect = None + gh_mock.return_value = {"body": once} + payload["manifest"]["questions"][question_id_value]["annotation"] = ( + "pending" + ) + pipeline.annotate_questions(payload, payload["manifest"]) + + # Re-applying finds the same marker and produces identical text, so + # there is nothing to write. + self.assertEqual(gh_mock.call_count, 1) + self.assertEqual( + payload["manifest"]["questions"][question_id_value]["annotation"], + "applied", + ) + + @mock.patch.object(pipeline, "gh_json") + def test_legacy_single_line_question_still_annotates(self, gh_mock): + question_id_value = "Q-abc123" + marker = pipeline.question_marker(question_id_value) + legacy = ( + f"โ“ **Open question ยท Medium confidence** {marker}\n" + "- Can the upstream return duplicate records?" + ) + gh_mock.side_effect = [{"body": legacy}, {"id": 42}] + payload = { + "repository": "megaeth-labs/example", + "pull_request": 7, + "question_annotations": [ + { + "question_id": question_id_value, + "review_id": 42, + "headline": f"โœ… **Answered** โ€” confirmed {marker}", + "block_header": ( + f"
\nโœ… **Answered** โ€” confirmed " + f"{marker}" + ), + } + ], + } + manifest = {"questions": {question_id_value: {"annotation": "pending"}}} + + pipeline.annotate_questions(payload, manifest) + + written = gh_mock.call_args.kwargs["input_value"]["body"] + # Falls back to the single-line rewrite rather than injecting an + # unbalanced
into a body that has no closing tag. + self.assertEqual( + written, + f"โœ… **Answered** โ€” confirmed {marker}\n" + "- Can the upstream return duplicate records?", + ) + self.assertEqual( + manifest["questions"][question_id_value]["annotation"], + "applied", + ) def test_closed_question_without_a_review_stops_being_retried(self): value = review_input()