docs: Add code review skill for enhanced verification process - #685
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #685 +/- ##
=======================================
Coverage 98.74% 98.74%
=======================================
Files 12 12
Lines 3977 3977
Branches 457 457
=======================================
Hits 3927 3927
Misses 50 50 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Copilot review overview
🟡 Changes recommended
Core workflow rules contain contradictory or inaccurate guidance, and one evaluation fixture uses a nonexistent dependency version.
Review effort: Balanced
Findings: 8
Open (8)
What changed in this PR
Adds repository-specific guidance for consistent, verified code reviews.
Changes:
- Adds a review workflow covering verification and repository conventions.
- Documents complexity rules and review checks.
- Adds five evaluation fixtures and contributor guidance.
| File | Description |
|---|---|
CONTRIBUTING.md |
Links the review skill. |
.github/copilot-instructions.md |
Documents skill directories. |
.agents/skills/code-review/SKILL.md |
Defines the review workflow. |
references/review-checklist.md |
Adds rule-specific checks. |
references/cognitive-complexity-rules.md |
Documents analyzer scoring behavior. |
evals/README.md |
Explains fixture usage. |
evals/01-else-nesting-regression/input.diff |
Adds an else-nesting regression case. |
evals/01-else-nesting-regression/expected.md |
Defines expected findings. |
evals/02-fix-with-sibling-drift/input.diff |
Adds a parity-drift case. |
evals/02-fix-with-sibling-drift/expected.md |
Defines expected findings. |
evals/03-dependabot-native-bump/input.diff |
Adds a dependency-bump case. |
evals/03-dependabot-native-bump/expected.md |
Defines expected findings. |
evals/04-setting-default-drift/input.diff |
Adds a configuration-drift case. |
evals/04-setting-default-drift/expected.md |
Defines expected findings. |
evals/05-generated-file-hand-edit/input.diff |
Adds a generated-file case. |
evals/05-generated-file-hand-edit/expected.md |
Defines expected findings. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
|
||
| | Class (how to detect) | Check only this | Then | | ||
| |-----------------------|-----------------|------| | ||
| | dependabot — `author.login` is `dependabot[bot]`, title `build(deps…)` / `ci(deps)` | `package.json` and `package-lock.json` agree on the new version; a `tree-sitter*` bump updates the matching `allowScripts` key `name@version` (npm 12 skips the native build otherwise); a pinned action SHA keeps its `# vX.Y.Z` comment; `statusCheckRollup` is green | Verdict from these checks only. Skip Steps 3–5. | |
| | Class (how to detect) | Check only this | Then | | ||
| |-----------------------|-----------------|------| | ||
| | dependabot — `author.login` is `dependabot[bot]`, title `build(deps…)` / `ci(deps)` | `package.json` and `package-lock.json` agree on the new version; a `tree-sitter*` bump updates the matching `allowScripts` key `name@version` (npm 12 skips the native build otherwise); a pinned action SHA keeps its `# vX.Y.Z` comment; `statusCheckRollup` is green | Verdict from these checks only. Skip Steps 3–5. | | ||
| | release-please — title `chore(main): release x.y.z` | `package.json` version, `.github/.release-please-manifest.json` and the top `CHANGELOG.md` entry agree; **no other file** changed | `COMMENT`; never edit its files | |
| Run these in the worktree that contains the change. Never infer a result from reading. | ||
|
|
||
| ```bash | ||
| npm ci --no-audit --no-fund # only if node_modules is missing or package-lock.json is in the diff |
| |---|-----------------|-------|-----| | ||
| | R1 | `src/metricsAnalyzer/languages/*Analyzer.ts` | Increments follow `references/cognitive-complexity-rules.md`: structural constructs `1 + nesting`; `else`/`else if`, ternary, boolean-operator *runs* and labeled jumps flat `+1`; an else-if body is visited without a second nesting bump; same-operator chains counted once via `isOutermostInSameOperatorChain`; new nesting constructs added to `NESTING_TYPES` | 🔴 | | ||
| | R2 | Any change to how one analyzer scores a construct | Probe that construct in all seven analyzers (Step 5). Disagreement **introduced** by the diff → 🟠. Pre-existing disagreement left untouched → 🟡 informational, named as follow-up, never blocking | 🟠/🟡 | | ||
| | R3 | Runtime behavior change under `src/**` | For `src/metricsAnalyzer/**` and `src/lruCache.ts`: an exact-complexity assertion (`assert.strictEqual(result.complexity, N)` plus `details` reasons) is added in `src/unit/unit.test.ts` — tests only in `src/test/**` do not count toward the c8 gate. For `src/extension.ts`, `src/configuration.ts`, `src/providers/**` (c8-excluded, need the `vscode` module): the test belongs in `src/test/**` instead. A new `/* c8 ignore */` carries a comment saying why the branch is unreachable | 🟠 | |
| | R5 | A `codeMetrics.*` setting or its default | Identical in `package.json` `contributes.configuration`, `src/configuration.ts` (default and validation) and the README "Extension Settings" list. The `package.json` value is what users get at runtime; `DEFAULT_CONFIG` is only a fallback, so a change made there alone does nothing and breaks `src/test/configuration.test.ts` | 🟠 | | ||
| | R6 | `CHANGELOG.md`, `.github/.release-please-manifest.json`, `.github/workflows/*.lock.yml` | Hand edits outside a release-please PR are wrong: the first two come from release-please, `.lock.yml` from `gh aw compile` of its sibling `.md`. A `.lock.yml` change without its `.md` (or the reverse) is drift | 🟠 | | ||
| | R7 | PR title | Type matches content: `fix:` changes runtime behavior in `src/` and adds a regression test; `feat:` adds a user-visible capability and updates README; `refactor:`/`perf:` leave exact-complexity assertions unchanged; `test:`/`docs:`/`ci:`/`build:`/`chore:` touch no `src/` runtime file. `feat` bumps minor, `fix`/`perf` patch, `!` major; `docs`/`test`/`ci`/`build`/`style` are hidden from the changelog | 🟠 | | ||
| | R8 | Any TypeScript | Generic correctness with the local traps: `line`/`column` are 0-based; `node.children` allocates in hot loops (use `childCount`/`child(i)`); `substring` where `node.type` suffices; eslint `eqeqeq`, `curly`, `semi` are **warn-only** so CI stays green — you are the gate; glob or regex built from user settings must escape metacharacters | 🟡 | |
Comment on lines
+23
to
+24
| - `1 + this.acc.nesting` on something the baseline lists as flat (`else`, `else if`, ternary, | ||
| boolean run, labeled jump) → 🔴. |
Comment on lines
+73
to
+74
| Worked example: #682 added eviction coverage for `pruneAnalysisCacheForDocument` purely to keep | ||
| the branch threshold green — that is the standard. |
Comment on lines
+161
to
+162
| - `line`/`column` in `MetricsDetail` are 0-based; anything that adds `+1` for display must do so | ||
| only at the VS Code boundary in `src/providers/**`. |
6 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This pull request introduces a new skill for code review located in the
.agents/skills/code-reviewdirectory. The skill aims to improve the review process by incorporating specific checks and guidelines tailored to the repository's needs.Changes made:
SKILL.mddetailing the workflow and rules for code review.evals/directory to test various scenarios, including cognitive complexity and cross-analyzer parity.cognitive-complexity-rules.mdandreview-checklist.md, to provide guidelines and examples for reviewers.CONTRIBUTING.mdandcopilot-instructions.mdto include pointers to the new skill and its usage.Key Features:
This addition is expected to streamline the code review process and enhance the quality of code submissions.