From f65997209c2d61d846db968bef7bf544e4694718 Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Thu, 12 Mar 2026 19:16:16 -0700 Subject: [PATCH 1/2] agents|fix: Clean up PR and review output conventions Add no-automated-tests-in-test-plan rule to summarize-change and prepare-pr skills. Add PR comment labeling rules to review-criteria (finding IDs map to descriptive labels like `fixme:`, `warning:`). Add finding-IDs-out-of-context entry to common-mistakes. --- .../agents/content/skills/common-mistakes/SKILL.md | 5 +++++ packages/agents/content/skills/prepare-pr/SKILL.md | 1 + .../agents/content/skills/review-criteria/SKILL.md | 12 ++++++++++++ .../agents/content/skills/summarize-change/SKILL.md | 1 + 4 files changed, 19 insertions(+) diff --git a/packages/agents/content/skills/common-mistakes/SKILL.md b/packages/agents/content/skills/common-mistakes/SKILL.md index abf1a8df..ef0a93df 100644 --- a/packages/agents/content/skills/common-mistakes/SKILL.md +++ b/packages/agents/content/skills/common-mistakes/SKILL.md @@ -70,3 +70,8 @@ These mistakes span multiple categories: - Conditional expects that can silently pass - Tests that pass coincidentally despite logical flaws - Missing edge case coverage + +### Finding IDs out of context + +- Finding IDs (`F1`, `W2`, `T3`, etc.) belong only in review documents. Never use them in commit messages, PR descriptions, tickets, or plans. +- When posting findings as PR comments, use descriptive labels (`fixme:`, `warning:`, `todo:`, `suggestion:`, `recommendation:`) instead. See `review-criteria` skill for the full mapping. diff --git a/packages/agents/content/skills/prepare-pr/SKILL.md b/packages/agents/content/skills/prepare-pr/SKILL.md index 77600efa..3a350497 100644 --- a/packages/agents/content/skills/prepare-pr/SKILL.md +++ b/packages/agents/content/skills/prepare-pr/SKILL.md @@ -52,3 +52,4 @@ Example: `20250121-1530Z_auto-share-exception_pull-request.md` - The PR description file is a copy of the change summary - This separation allows for PR-specific modifications if needed - The change summary serves as the source of truth for branch work +- Never list automated checks (formatting, linting, typechecking, unit tests) in a test plan. They run automatically in CI. diff --git a/packages/agents/content/skills/review-criteria/SKILL.md b/packages/agents/content/skills/review-criteria/SKILL.md index f20d6b16..6ceb3b33 100644 --- a/packages/agents/content/skills/review-criteria/SKILL.md +++ b/packages/agents/content/skills/review-criteria/SKILL.md @@ -99,3 +99,15 @@ Used by review-producing skills and agents for structured code review findings. ### Re-review severity escalation `S → R → T → W → F`. L findings are never escalated. + +## PR comment format + +When posting findings as PR comments (e.g., inline comments on a pull request), use a descriptive label instead of the raw finding ID. Finding IDs like `F1` or `W2` are internal to review documents and have no meaning to readers in PR context. + +| Finding prefix | PR comment label | +| -------------- | ----------------- | +| `F` | `fixme:` | +| `W` | `warning:` | +| `T` | `todo:` | +| `S` | `suggestion:` | +| `R` | `recommendation:` | diff --git a/packages/agents/content/skills/summarize-change/SKILL.md b/packages/agents/content/skills/summarize-change/SKILL.md index 7ccbc574..33dc3ef1 100644 --- a/packages/agents/content/skills/summarize-change/SKILL.md +++ b/packages/agents/content/skills/summarize-change/SKILL.md @@ -80,6 +80,7 @@ Generated by: {Agent name} (model: {model}) - Omit inapplicable Details subsections - Order Details subsections per `work-types.md` priority: Primary → Secondary → Tertiary - `## What` and `## Why` are required; Details subsections are optional +- Never list automated checks (formatting, linting, typechecking, unit tests) in a test plan. They run automatically in CI. ## As a PR description From 55b5ad055c93820ad8e0e6b0be6291ae888d1649 Mon Sep 17 00:00:00 2001 From: William Thorsen Date: Thu, 12 Mar 2026 19:19:10 -0700 Subject: [PATCH 2/2] agents|fix: Strengthen no-hard-line-breaks rules in commit and common-mistakes skills Add bold inline rule to commit skill body guidelines instead of relying on a cross-reference. Expand common-mistakes entry with concrete wrong/right example and explanation of why manual wrapping is harmful. --- packages/agents/content/skills/commit/SKILL.md | 2 ++ .../content/skills/common-mistakes/SKILL.md | 18 +++++++++++++++++- 2 files changed, 19 insertions(+), 1 deletion(-) diff --git a/packages/agents/content/skills/commit/SKILL.md b/packages/agents/content/skills/commit/SKILL.md index 457f1d4e..aafe2aa1 100644 --- a/packages/agents/content/skills/commit/SKILL.md +++ b/packages/agents/content/skills/commit/SKILL.md @@ -41,6 +41,8 @@ Do not include the ticket ID in the commit title. The branch name carries it. In ## Body guidelines +**No hard line breaks.** Write each paragraph or list item as a single long line. Do not insert newlines to wrap at a column width. Every tool that renders commit messages handles wrapping; manual breaks produce ragged text. + See `../_data/commit-format.md` for body formatting rules (punctuation, backtick formatting, paragraph structure, and what to omit). ## Changes touching multiple workspaces diff --git a/packages/agents/content/skills/common-mistakes/SKILL.md b/packages/agents/content/skills/common-mistakes/SKILL.md index ef0a93df..49c17fac 100644 --- a/packages/agents/content/skills/common-mistakes/SKILL.md +++ b/packages/agents/content/skills/common-mistakes/SKILL.md @@ -41,7 +41,23 @@ Commit titles must describe the **code change**, not the process. Titles like "A ### Hard line breaks in commit body -Do not insert newlines to wrap the commit body at a fixed column width. Write naturally as continuous text. Let the terminal or viewer handle wrapping. This applies to both prose paragraphs and list items. +This is a frequently repeated mistake. Do not insert newlines to wrap the commit body at a fixed column width. Every tool that displays commit messages (GitHub, Bitbucket, terminal pagers) handles wrapping. Manually broken lines create ragged text, produce unnecessary diffs when reflowed, and make commit messages harder to search. + +Write each paragraph or list item as a single unwrapped line. Let the viewer handle wrapping. + +Wrong: + +``` +Add no-automated-tests-in-test-plan rule to summarize-change and +prepare-pr skills. Add PR comment labeling rules to review-criteria +(finding IDs map to descriptive labels like `fixme:`, `warning:`). +``` + +Right: + +``` +Add no-automated-tests-in-test-plan rule to summarize-change and prepare-pr skills. Add PR comment labeling rules to review-criteria (finding IDs map to descriptive labels like `fixme:`, `warning:`). +``` ## Pull requests