Proposal: repo-specific skills mined from historical PR review feedback

## Background

We want to give AI coding assistants (Claude Code, Cursor, etc.) a set of repo-specific "skills" — small, scoped guidance files placed under `.claude/skills/` — so contributions land closer to reviewer expectations on the first attempt. The merged PR adding the baseline set was kept generic on purpose. This proposal mines this repo's own historical review feedback to identify what would have the highest return as a follow-up, AgodaAnalyzers-tailored set.

The methodology was: read substantive human review/inline comments across ~52 PRs (out of 147 total, August 2017–April 2026), prioritising the recent Playwright / SpecFlow / test-quality wave, and group reviewer corrections into themes. Each theme below has at least three distinct PR examples and a concrete skill proposal that would have prevented the correction.

---

## 1. Per-theme findings

### Theme A — Analyzer scope is too broad / false positives in non-target code

**Frequency**: ~10 review interactions across PRs #226, #228, #229, #230, #235, #220, #221, #219, #136, #98, #67, #117. By far the most recurring class of feedback, and has driven a long tail of follow-up "fix false positive" PRs.

> "Do we use this naming pattern somewhere (starting with `Test`)? Feels prone to false positives, idk things like `TestimonialService` come to mind." *(reviewer, PR #228)*

> "Potential high-noise source: this analyzer runs for all C# code, but the rule intent/docs are test-data specific. As implemented, production code like migrations, seed data, protocol parsing, or fixed historical/business boundary dates will be flagged. Can we gate this to test contexts (test projects / test classes / test methods)?" *(reviewer, PR #226)*

> "This adds `LoadState.NetworkIdle` as a global enum-member blacklist, which means **any** usage of that enum value is flagged (including variable assignment, comparisons, switch cases), not just `WaitForLoadStateAsync(...)`. That can create false positives in non-flaky contexts." *(reviewer, PR #226)*

> "This solution might fix the particular example highlighted, but there might be other edge-cases: global using, fully qualified type references, etc? Can we think of a different approach? E.g. only check variables that actually get passed into a PW locator?" *(reviewer, PR #235)*

**Proposed skill — `analyzer-scope-guard`**
Invoke when authoring a new diagnostic or modifying an existing one's reporting conditions. Core rule: before reporting a diagnostic, answer "what is the narrowest context in which this rule is intended to fire?" Specifically (a) gate test-only rules by class/method test attributes *and* namespace heuristics rather than namespace alone; (b) when blacklisting a method/enum, scope detection to the actual call site (receiver type + invocation context) rather than the bare symbol; (c) before merging, enumerate at least three plausible non-target call sites (logging strings, comments, fully-qualified references, regex literals, generated code, partial-name collisions like `TestimonialService`) and add a negative unit test for each.

---

### Theme B — Test detection misses method-level test attributes

**Frequency**: 4 review pushbacks across PRs #228 and #229, plus a related correction on PR #89 from 2018 — a long-standing trap.

> "`IsInTestContext` does not look at **method-level test attributes** (`[Test]`, `[Fact]`, etc.). So this pattern is currently missed if namespace/class naming does not include `Test(s)` and class has no fixture attribute." *(reviewer, PR #228)*

> "If no xUnit is intentional, then should it be mentioned in docs that it only works with [the supported frameworks]?" *(reviewer, PR #228)*

> "`IsInTestClass` only checks class/base-class attributes (`[TestFixture]`, `[TestClass]`). It misses method-level test attributes and therefore may skip real violations in valid test classes without fixture attributes." *(reviewer, PR #229)*

> "This is close, but not quite there yet. The `TestFixture` attribute is optional. Instead, we need to check if one of the methods in the class is a test case." *(reviewer, PR #89, 2018)*

**Proposed skill — `test-context-detection`**
Invoke when an analyzer's logic includes "only fire inside test code." Core rule: a class is a test class iff *any* of the following holds — (1) the class or a base class carries `[TestFixture]` / `[TestClass]`; (2) **any method** in the class carries `[Test]`, `[Fact]`, `[Theory]`, `[TestMethod]`, or `[TestCase]`; (3) the containing namespace segment matches `^Test$|Tests?$` (avoiding prefix-only "Test*" matches that collide with words like *Testimonial*). The skill should also produce a docs sentence enumerating which test frameworks are recognised, because reviewers consistently ask for that disclosure when it's dropped.

---

### Theme C — Documentation must explain *why* and show Do/Don't pairs

**Frequency**: 7 review interactions across PRs #192, #200, #208, #211, #84, #86, #226 — and the project lead has stated the standard repeatedly.

> "When you make documentation, can you: provide the reason why this is bad, and what the alternatives are to solve the problem that the developer was trying to. Provide example of both non-compliant and compliant code … look at it like this, you are a junior engineer, you see this error for the first time in your IDE, what's the documentation you need to fix your problem? and save you from either A. Escalating in Slack, B. Disabling the rule that you don't understand." *(reviewer, PR #192)*

> "Make the file style consistent with other rules." *(reviewer, PRs #208 and #211, identical comment on two separate rules)*

> "The compliant and non-compliant code examples are the wrong way round." *(reviewer, PR #84)*

> "My only comment would be that the HTML cheat sheet shows explicit 'Don't' and 'Do' examples." *(reviewer, PR #86)*

**Proposed skill — `analyzer-rule-doc-template`**
Invoke when creating or substantially editing a file in `doc/AGxxxx.md` or `src/Agoda.Analyzers/RuleContent/*.html`. Core rule: every rule doc must contain, in order, (1) one-sentence summary of what is flagged, (2) the "why" — the failure mode or business risk this prevents, (3) a "Don't" code block, (4) a "Do" code block showing the recommended replacement, (5) a "Detection scope" section listing exactly which syntactic contexts are inspected, and (6) any framework/version assumptions. The skill should also enforce stylistic uniformity with sibling files (icon usage, heading levels) by reading a neighbouring rule doc as the canonical template before writing.

---

### Theme D — Diagnostic must be tested with negative cases and edge variants

**Frequency**: 8+ across PRs #84, #51, #157, #82, #226, #228, #235, plus the original CONTRIBUTING.md requirement.

> "Could you please add cases for the following: non-enumerable type `public int` (allowed), non-public `private`/`protected`/`internal` (allowed), `public byte[]` (allowed), `public AnythingElse[]` (not allowed), `public string` (allowed)." *(reviewer, PR #84)*

> "Could you add a test case here, please? Optional: if it's not a big ask, could you add `IReadOnlyList` too?" *(reviewer, PR #157)*

> "Wait, is this really what was the intention? … This is bad: `await Task.Delay(1000);` — But this is OK? `var d = 1000; await Task.Delay(d);` — This is OK as well? `await Task.Delay(5*200);` (note: the AST node for `5*200` is not a `NumericLiteralExpression`)." *(reviewer, PR #229)*

> "Also: consider adding test cases for the scenario described in the issue." *(reviewer, PR #235)*

**Proposed skill — `analyzer-test-coverage-matrix`**
Invoke when adding or modifying tests for a Roslyn diagnostic analyzer. Core rule: for every positive case the analyzer fires on, the test file must include matched negative cases that vary along (a) accessibility modifiers, (b) syntactic form (literal vs. constant expression vs. local variable vs. parenthesised vs. binary expression vs. interpolated string vs. fully-qualified reference), (c) generic vs. non-generic, (d) array vs. scalar, and (e) interface vs. class. The skill should produce a "test coverage matrix" comment in the test file enumerating which axes have been covered. It should also explicitly check that any user issue cited in the PR has a regression test added.

---

### Theme E — Performance: analyzer hot paths allocate or do redundant tree walks

**Frequency**: 3 explicit pushes across PRs #226 and #228, plus drive-by mentions in older PRs.

> "Nit, but since rules get evaluated a lot while typing I'll mention: `new[] { '-', '/' }` could be moved into a static field rather than allocating it every time." *(reviewer, PR #228)*

> "Performance note: for each screenshot call, this scans all prior invocations in the entire method (`DescendantNodes().OfType()`), so methods with multiple screenshots become O(N^2)-ish. This can be optimized by restricting to containing block statements and scanning backward from the screenshot statement, or precomputing wait-call spans once per method." *(reviewer, PR #226)*

> "I like that we can quickly skip non-matching identifier for performance." *(reviewer, PR #50 — positive reinforcement of the same principle)*

**Proposed skill — `analyzer-hot-path-perf`**
Invoke when authoring or reviewing the per-syntax-node callback registered by an analyzer. Core rule: code on the analyzer callback runs every keystroke, so it must (1) hoist any array/collection/regex/string-array literal to `private static readonly`, (2) cheap-out early — string/identifier check before semantic-model lookup, (3) avoid `DescendantNodes()` over the entire method when a containing block or single ancestor walk suffices, and (4) prefer `SymbolEqualityComparer` / symbol checks to `ToDisplayString()` comparisons in tight loops. The skill should also surface that `RegisterSyntaxNodeAction(..., SyntaxKind.X)` filtering at registration time is cheaper than filtering inside the callback.

---

### Theme F — Naming conventions: analyzer class names, constants, and helper extraction

**Frequency**: 6 across PRs #51, #84, #86, #82, #120 — concentrated in older PRs but the rules still apply (newer PRs are mostly written by contributors who already know them).

> "This class should be named something like `AG0006RegisteredComponentShouldHaveExactlyOnePublicConstructor`." *(reviewer, PR #51)*

> "Better name: `AG0018PermitOnlyCertainPubliclyExposedEnumerables`." *(reviewer, PR #84)*

> "I know it's silly, but the standards for constants are `DIAGNOSTIC_ID`." *(reviewer, PR #86)*

> "For the other analyzers this method is called `AnalyzeNode`." *(reviewer, PR #120)*

> "This is fantastic, just a couple of things: could it be moved to its own file to make its intended reuse more clear? Could it be named something more obvious like `ForbiddenMethodAnalyzerBase`?" *(reviewer, PR #82)*

**Proposed skill — `analyzer-naming-conventions`**
Invoke when creating a new analyzer file, helper, or constant. Core rule: (1) analyzer class file is `AG0XXX.cs` where the verb phrase reads as a positive prescription or a clear prohibition matching the resx Title; (2) the constant holding the rule ID is `DIAGNOSTIC_ID` (SCREAMING_SNAKE_CASE), not `DiagnosticId`; (3) the syntax-node callback method is named `AnalyzeNode`; (4) reusable helpers must live in `src/Agoda.Analyzers/Helpers/` (e.g. `TestMethodHelpers.cs`, `InvocationRule.cs`) as their own file, not nested private classes. The skill should grep two existing sibling analyzers and mimic their structure before producing new files.

---

### Theme G — Rules must be independent / no cross-rule assumptions

**Frequency**: 3 strong instances on PR #80, plus echoes on PR #229.

> "We disallow dynamics completely in AG0030, so you don't need to test for this." *(contributor's initial reasoning, PR #80)* — followed by the reviewer:

> "I believe our checks shouldn't interact with each other. Otherwise we have to review all our rules on any change. User can disable any rule at any time, so other checks should work properly. The checking is important due to `dynamic` may crash the current rule. So it is a part of the regressions." *(reviewer, PR #80)*

> "I would consider explicitly adding `Thread.Sleep()` into this one, regardless. Rules are independently configurable. It's a stretch, but someone might decide to disable AG0023 for whatever reason." *(reviewer, PR #229)*

**Proposed skill — `independent-rule-design`**
Invoke when an analyzer's design or tests appeal to "another rule will already catch this." Core rule: every analyzer must be self-sufficient — never narrow its detection assuming an adjacent rule is enabled, never skip a test case because another rule covers the input shape, and never share mutable state across analyzers. The skill should rewrite any "AG0NNN already handles X, so we don't need to here" reasoning into either (a) the explicit detection, or (b) a documented coupling decision called out in the PR description.

---

### Theme H — Cite the source: docs links and code-comment hygiene

**Frequency**: 4 across PRs #192, #191, #200, #227.

> "Do these comments really add value? I mean, do we really need to explain what `if (methodName != \"QuerySelectorAsync\")` means?" *(reviewer, PR #191)*

> "Do these comments add value? Personally I think that the code `if (something is MemberAccessExpressionSyntax)` communicates well that we're checking if something is a member access (literally uses the same words)." *(reviewer, PR #192)*

> "It was just copy-paste from GPT, my bad that I didn't clean it up." *(contributor admitting, PR #191)*

> "Consider extending this with link to waitforloadstate docs." *(reviewer, PR #227)*

**Proposed skill — `comment-and-citation-hygiene`**
Invoke when the agent has produced a code or docs change. Core rule: strip restatement comments (`// check if x is a y` above `if (x is Y)`); when generating documentation that references an external API, include a link to the official docs page; never leave AI-generated boilerplate (e.g. emoji headers in PR descriptions, `// As an AI assistant…` lead-ins) in committed content.

---

### Theme I — Code-fix providers must preserve unrelated code

**Frequency**: 2 on PR #86, but the consequences are severe (silent data loss in user code).

> "The code fix has removed the `AssemblyDescription` attribute. It should have left: `[assembly: AssemblyDescription(\"Description\")]`." *(reviewer, PR #86)*

> "Again in your code here the entire attribute list has been removed. `AssemblyDescription` should have survived." *(reviewer, PR #86)*

**Proposed skill — `code-fix-preservation`**
Invoke when authoring a `CodeFixProvider`. Core rule: a code fix must only remove or alter the exact syntax node identified by the diagnostic — when operating on attribute lists, parameter lists, or member lists, the fix must reconstruct the surrounding structure with all sibling elements intact. The skill mandates a test case where the target syntax is one of several siblings, asserting the others survive the fix verbatim.

---

### Theme J — Scope creep / one-PR-one-issue

**Frequency**: 3 across PRs #226, #235, #217.

> "Please rebase, `AG0054` shows up as new addition in the diffs." *(reviewer, PR #235)*

> "This PR has been split into 5 smaller, independently reviewable PRs — one per rule." *(contributor, PR #226, after reviewer flagged scope)*

> The CONTRIBUTING.md states: "Please limit the changes contained in your PR to a single issue."

**Proposed skill — `one-pr-one-rule`**
Invoke before committing or opening a PR. Core rule: each PR introduces or modifies exactly one diagnostic ID. If the agent finds itself touching multiple `AG0NNN*.cs` files, it should stop and either (a) ask whether the work should be split, or (b) verify with the user that a multi-rule PR is acceptable. Also: rebase or branch from a clean base before pushing, so unrelated rules don't show in the diff.

---

## 2. Prioritised list

| Rank | Skill | Why it ranks here |
|------|-------|-------------------|
| 1 | `analyzer-scope-guard` | The single most common reviewer correction, and the one that has spawned the most follow-up "fix false positive" PRs (#136, #98, #117, #219, #220, #221, #235). Catching this pre-review would save the most review cycles and prevent regressions shipping. |
| 2 | `test-context-detection` | Appears in 4 separate PRs and has a clean mechanical rule. High confidence the skill prevents the exact correction reviewers keep making. |
| 3 | `analyzer-test-coverage-matrix` | Reviewers consistently enumerate missing test cases by hand; a skill that derives the matrix from the analyzer's described behaviour replaces that whole class of comments. |
| 4 | `analyzer-rule-doc-template` | The project lead has stated the docs standard explicitly and repeatedly; documentation drift triggers blocking review comments (#192, #208, #211). |
| 5 | `analyzer-hot-path-perf` | Specific, mechanical, and the cost of getting it wrong ships to every IDE user. Lower frequency but high downstream impact. |
| 6 | `code-fix-preservation` | Lower frequency, but each occurrence is a correctness bug that would corrupt user code — disproportionate severity warrants a dedicated guard. |
| 7 | `independent-rule-design` | Reviewers push back on this consistently; the rule is simple to state and easy to violate when the agent reasons globally about the rule set. |
| 8 | `analyzer-naming-conventions` | Mostly an issue with new contributors; once internalised the cost is zero. A short skill prevents avoidable churn. |
| 9 | `comment-and-citation-hygiene` | Catches a recognisable failure mode of LLM-authored PRs (restatement comments, missing doc links, emoji-heavy descriptions). |
| 10 | `one-pr-one-rule` | Process-level; useful as a pre-commit reminder but lower frequency than the technical themes. |

---

## 3. Out-of-scope notes

- **CI / build failures (#185, #177, #215, #145, #176)** — tooling/infra problems (release-mode-first builds, NuGet packaging, AppVeyor config). They don't generalise into a skill; they need fixing in the build scripts themselves.
- **Dependabot bumps (#164–#170 etc.)** — bot-driven, no review feedback to mine.
- **DiagnosticID collisions** ("Manu has already used this ID in his rule. How can we stop these conflicts in the future?" — PR #47) — better solved by a registry file or a CI check than by an agent skill, though a thin "claim-AG0NNN-before-coding" skill could plug into the project board lookup if the workflow is automated.
- **Stylistic preferences on test readability (#184)** — reviewers themselves disagreed (one preferred parameterised test cases, another preferred inlined readable tests). No converged convention to encode.
- **Project-board / issue-tracker process** ("Do not forget to track progress of your task here" — PR #86) — workflow reminder, better handled by a PR template than a skill.
- **Architectural future work** ("Eventually we should have a separate project for Tests" — PR #226) — not a skill-shaped item.
- **Reviewer's open design question about local functions (PR #230)** — can be subsumed under `analyzer-scope-guard` (edge case enumeration) rather than a standalone skill.

---

## Acceptance criteria

- [ ] Discuss which of the 10 proposed skills should be drafted first.
- [ ] For each accepted skill, open a follow-up PR adding `SKILL.md` under `.claude/skills//` (alongside the generic baseline merged in #237).
- [ ] Re-mine in 6 months to check whether shipped skills actually reduced the frequency of the corresponding review correction.