From f74746176876cae1a656632c6659e9cc84882b1e Mon Sep 17 00:00:00 2001 From: monkut Date: Wed, 29 Apr 2026 16:16:37 +0900 Subject: [PATCH 1/4] :bug: Add PR already-reviewed guard, reduce redundant issue comments (closes #100) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - REVIEWPR_AGENT_PROMPT: add `Pre-review dedup` block before `Pre-merge guard`. Skips review entirely when ellen-goc's last substantial review (body > 50 chars) postdates the PR's most recent commit. - REVIEWPR_AGENT_PROMPT: remove unconditional linked-issue comment line. The PR review itself is the canonical record. - DEVELOP_AGENT_PROMPT: make the on-completion issue comment conditional — comment only when this session opened a new PR; skip on follow-up commits. - Tests: add `TestReviewprPromptDedupGuard`, `TestReviewprPromptNoLinkedIssueComment`, and `TestDevelopPromptConditionalIssueComment` (literal-substring asserts mirroring the `TestReviewprPromptMergeGuard` precedent). --- askcc/definitions.py | 13 ++++++++++-- tests/test_askcc.py | 49 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 60 insertions(+), 2 deletions(-) diff --git a/askcc/definitions.py b/askcc/definitions.py index d61db8e..5af0c95 100644 --- a/askcc/definitions.py +++ b/askcc/definitions.py @@ -330,7 +330,9 @@ - If no PR exists, open one linked to the issue. - If a PR exists (follow-up changes), review and update its description (see "PR description update") before commenting. - Update the test plan checklist (see "Test plan update"). -- Comment on the issue summarizing what was implemented. +- If this session opened a new PR: comment on the issue summarizing what was implemented. +- If a PR already existed before this session (follow-up commits): skip the issue comment \ +— the PR description update is sufficient. {DEVELOP_PR_DESCRIPTION_UPDATE} @@ -522,6 +524,14 @@ - Check out the PR branch: `gh pr checkout `. - Run the test suite to confirm all tests pass. +Pre-review dedup (skip if already reviewed since last push): +- `gh pr view -R --json reviews,commits \ +--jq '{last_commit: (.commits | sort_by(.committedDate) | last | .committedDate), \ +last_review: ([.reviews[] | select(.author.login == "ellen-goc" and (.body|length > 50)) \ +| .submittedAt] | sort | last // "")}'` +- If `last_review` is non-empty AND `last_review >= last_commit` → skip entirely \ +(no review, no linked-issue comment). Stop immediately. + Pre-merge guard (CHANGES_REQUESTED): - Before merging, check for unresolved CHANGES_REQUESTED reviews: \ `gh pr view -R --json reviews --jq '[.reviews[] | select(.state == "CHANGES_REQUESTED")]'`. @@ -546,7 +556,6 @@ Posting the review: - All pass: `gh pr review -R --approve --body ""` - Any fail: `gh pr review -R --request-changes --body ""` -- Also post a brief summary on the linked issue: `gh issue comment`. Update test plan in PR description: - Read PR body: `gh pr view -R --json body -q .body`. diff --git a/tests/test_askcc.py b/tests/test_askcc.py index 5ed7758..e860f5f 100644 --- a/tests/test_askcc.py +++ b/tests/test_askcc.py @@ -393,6 +393,55 @@ def test_includes_pre_merge_guard_heading(self): assert "Pre-merge guard" in REVIEWPR_AGENT_PROMPT +class TestReviewprPromptDedupGuard: + """The pr-review prompt must skip duplicate reviews when no new commits since last review (issue #100). + + Assertions are literal-substring matches: prompt-wording changes will surface + here intentionally — update both the prompt and the asserted substrings together, + mirroring the TestReviewprPromptMergeGuard precedent. + """ + + def test_includes_pre_review_dedup_heading(self): + assert "Pre-review dedup" in REVIEWPR_AGENT_PROMPT + + def test_includes_dedup_check_command(self): + assert "sort_by(.committedDate)" in REVIEWPR_AGENT_PROMPT + assert '.author.login == "ellen-goc"' in REVIEWPR_AGENT_PROMPT + assert "(.body|length > 50)" in REVIEWPR_AGENT_PROMPT + assert "--json reviews,commits" in REVIEWPR_AGENT_PROMPT + + def test_includes_skip_instruction(self): + assert "skip entirely" in REVIEWPR_AGENT_PROMPT + + def test_dedup_guard_placed_before_pre_merge_guard(self): + assert REVIEWPR_AGENT_PROMPT.index("Pre-review dedup") < REVIEWPR_AGENT_PROMPT.index("Pre-merge guard") + + +class TestReviewprPromptNoLinkedIssueComment: + """The pr-review prompt must not unconditionally post a linked-issue comment (issue #100).""" + + def test_does_not_post_linked_issue_comment(self): + assert "Also post a brief summary on the linked issue" not in REVIEWPR_AGENT_PROMPT + + +class TestDevelopPromptConditionalIssueComment: + """The develop prompt must only comment on the issue when opening a new PR (issue #100). + + Assertions are literal-substring matches: prompt-wording changes will surface + here intentionally — update both the prompt and the asserted substrings together. + """ + + def test_does_not_unconditionally_comment_on_issue(self): + assert "- Comment on the issue summarizing what was implemented." not in DEVELOP_AGENT_PROMPT + + def test_includes_new_pr_branch(self): + assert "If this session opened a new PR" in DEVELOP_AGENT_PROMPT + + def test_includes_existing_pr_skip_branch(self): + assert "PR already existed" in DEVELOP_AGENT_PROMPT + assert "PR description update is sufficient" in DEVELOP_AGENT_PROMPT + + class TestConfigBoundaryConstraints: """Both develop and fix-ci prompts must forbid relaxing linter/type-checker config (issue #89).""" From 0cbc20c5cf066d48290d550291b580723cf5f14f Mon Sep 17 00:00:00 2001 From: monkut Date: Thu, 30 Apr 2026 15:01:51 +0900 Subject: [PATCH 2/4] :bookmark: Bump version to 0.2.11 --- pyproject.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/pyproject.toml b/pyproject.toml index 47bdc41..02e29ac 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -1,6 +1,6 @@ [project] name = "askcc" -version = "0.2.10" +version = "0.2.11" description = "A one-shot cc cli executor" authors = [{ name = "mknt", email = "shane.cousins@gmail.com" }] readme = "README.md" From e4f35d08afd2be9cddd10dc01068014c3606d8e7 Mon Sep 17 00:00:00 2001 From: monkut Date: Thu, 30 Apr 2026 15:05:09 +0900 Subject: [PATCH 3/4] :bug: Add mermaid label-quoting guidance to develop prompt - DEVELOP_AGENT_PROMPT: warn that unquoted labels starting with / or \ are parsed as mermaid parallelogram/trapezoid shape syntax and break rendering with a lexical error. Show quoted form: B["/simplify, ..."] vs broken B[/simplify, ...]. - Tests: add TestDevelopPromptMermaidLabelSafety asserting the heading, quoted example, and shape-collision wording (literal-substring style). - uv.lock: sync to 0.2.11. --- askcc/definitions.py | 4 ++++ tests/test_askcc.py | 17 +++++++++++++++++ uv.lock | 2 +- 3 files changed, 22 insertions(+), 1 deletion(-) diff --git a/askcc/definitions.py b/askcc/definitions.py index 5af0c95..79ea670 100644 --- a/askcc/definitions.py +++ b/askcc/definitions.py @@ -321,6 +321,10 @@ C -- any fail --> E[stay in develop] `` ` ``` +- Mermaid label safety: quote labels containing `/`, `\\`, `(`, `)`, `|`, `:`, or starting \ +with punctuation — e.g. `B["/simplify, commit, push"]`, not `B[/simplify, commit, push]`. \ +Unquoted leading `/` or `\\` is parsed as parallelogram/trapezoid shape syntax and breaks \ +rendering with a "Lexical error / Unrecognized text" message. - Keep diagrams concise — one or two covering the most important flows. \ Skip this section for trivial changes (config-only, docs-only, single-line fix). diff --git a/tests/test_askcc.py b/tests/test_askcc.py index e860f5f..76ef0d7 100644 --- a/tests/test_askcc.py +++ b/tests/test_askcc.py @@ -442,6 +442,23 @@ def test_includes_existing_pr_skip_branch(self): assert "PR description update is sufficient" in DEVELOP_AGENT_PROMPT +class TestDevelopPromptMermaidLabelSafety: + r"""The develop prompt must instruct quoting mermaid labels with shape-reserved chars. + + Without quoting, labels starting with `/` or `\` are parsed as parallelogram/trapezoid + shape syntax (e.g. `B[/simplify]`) and break rendering with a lexical error. + """ + + def test_includes_label_safety_guidance(self): + assert "Mermaid label safety" in DEVELOP_AGENT_PROMPT + + def test_includes_quoted_example(self): + assert 'B["/simplify, commit, push"]' in DEVELOP_AGENT_PROMPT + + def test_warns_against_parallelogram_collision(self): + assert "parallelogram/trapezoid shape syntax" in DEVELOP_AGENT_PROMPT + + class TestConfigBoundaryConstraints: """Both develop and fix-ci prompts must forbid relaxing linter/type-checker config (issue #89).""" diff --git a/uv.lock b/uv.lock index 70fa014..b9cea8a 100644 --- a/uv.lock +++ b/uv.lock @@ -4,7 +4,7 @@ requires-python = "==3.14.*" [[package]] name = "askcc" -version = "0.2.10" +version = "0.2.11" source = { editable = "." } [package.dev-dependencies] From 89115b7d167dd65bad9dea7e565a5eabeae1cff8 Mon Sep 17 00:00:00 2001 From: monkut Date: Thu, 30 Apr 2026 15:27:47 +0900 Subject: [PATCH 4/4] :art: Trim narrative test docstrings, drop stale 'before commenting' - Tests: collapse multi-paragraph docstrings on TestReviewprPromptDedupGuard and TestDevelopPromptConditionalIssueComment to one-liners matching the TestReviewprPromptMergeGuard precedent. - DEVELOP_AGENT_PROMPT: drop 'before commenting' from the existing-PR bullet; the issue comment is now governed by the conditional bullets that follow. --- askcc/definitions.py | 2 +- tests/test_askcc.py | 13 ++----------- 2 files changed, 3 insertions(+), 12 deletions(-) diff --git a/askcc/definitions.py b/askcc/definitions.py index 79ea670..a6d89eb 100644 --- a/askcc/definitions.py +++ b/askcc/definitions.py @@ -332,7 +332,7 @@ - Run /simplify or /refactor to improve the code. - Commit and push the feature branch. - If no PR exists, open one linked to the issue. -- If a PR exists (follow-up changes), review and update its description (see "PR description update") before commenting. +- If a PR exists (follow-up changes), review and update its description (see "PR description update"). - Update the test plan checklist (see "Test plan update"). - If this session opened a new PR: comment on the issue summarizing what was implemented. - If a PR already existed before this session (follow-up commits): skip the issue comment \ diff --git a/tests/test_askcc.py b/tests/test_askcc.py index 76ef0d7..274d5e8 100644 --- a/tests/test_askcc.py +++ b/tests/test_askcc.py @@ -394,12 +394,7 @@ def test_includes_pre_merge_guard_heading(self): class TestReviewprPromptDedupGuard: - """The pr-review prompt must skip duplicate reviews when no new commits since last review (issue #100). - - Assertions are literal-substring matches: prompt-wording changes will surface - here intentionally — update both the prompt and the asserted substrings together, - mirroring the TestReviewprPromptMergeGuard precedent. - """ + """The pr-review prompt must skip duplicate reviews when no new commits since last review (issue #100).""" def test_includes_pre_review_dedup_heading(self): assert "Pre-review dedup" in REVIEWPR_AGENT_PROMPT @@ -425,11 +420,7 @@ def test_does_not_post_linked_issue_comment(self): class TestDevelopPromptConditionalIssueComment: - """The develop prompt must only comment on the issue when opening a new PR (issue #100). - - Assertions are literal-substring matches: prompt-wording changes will surface - here intentionally — update both the prompt and the asserted substrings together. - """ + """The develop prompt must only comment on the issue when opening a new PR (issue #100).""" def test_does_not_unconditionally_comment_on_issue(self): assert "- Comment on the issue summarizing what was implemented." not in DEVELOP_AGENT_PROMPT