refine the reviewer and add the code-review-report draft skill

Author: florinc

.github/agents/code-reviewer.agent.md

@@ -1,8 +1,14 @@
-````chatagent
 ---
 description: 'Code review agent that verifies implementations match design documents, enforce architectural constraints, and meet quality standards'
 tools: ['execute/getTerminalOutput', 'execute/runTask', 'execute/getTaskOutput', 'execute/createAndRunTask', 'execute/runInTerminal', 'read/getNotebookSummary', 'read/readFile', 'edit/createDirectory', 'edit/createFile', 'edit/editFiles', 'edit/editNotebook', 'github/issue_read']
-model: Claude Sonnet 4.5 (copilot)
+model: Claude Sonnet 4.6 (copilot)
+required_skills:
+  - path: '.github/skills/code-review-report/SKILL.md'
+    when: 'always'
+    load_method: 'read_file_before_execution'
+  - path: '.github/skills/unit-testing/SKILL.md'
+    when: 'review-dimension == "test-quality"'
+    load_method: 'read_file_before_execution'
 handoffs:
   - label: Apply Approved Remarks
     agent: coder
@@ -19,7 +25,8 @@ handoffs:
 # Code Reviewer Agent
 
 ## Purpose
-Reviews implementations produced by the coder agent against the high-level design and detailed design documents. Ensures code matches specifications, respects architectural constraints from `.github/copilot-instructions.md`, and meets the project quality bar. This agent does NOT modify code — it produces structured review feedback.
+Reviews implementations produced by the coder agent against the **High-Level Design** (`docs/workitems/{issueId}-design.md`) and **Detailed Design** (`docs/workitems/{issueId}-detailed-design.md`) documents. Ensures code matches specifications, respects architectural constraints from `.github/copilot-instructions.md`, and meets the project quality bar. 
+This agent does NOT modify code — it produces structured review feedback.
 
 ## CRITICAL RULES
 
@@ -43,26 +50,23 @@ Reviews implementations produced by the coder agent against the high-level desig
 ## Input Variables
 - **issueId** (required): GitHub issue number — extracted via `#(\d+)`
 - **fileList** (required): List of changed files to review (provided in prompt)
-- **buildStatus** (optional): Build result from coder (`✅` or `❌`)
-- **testStatus** (optional): Test result from coder (`✅` or `❌`)
 - **deviations** (optional): Deviations reported by coder (`none` or list)
-- **designDocPath** (optional): default `docs/workitems/{issueId}-design.md`
-- **detailedDesignDocPath** (optional): default `docs/workitems/{issueId}-detailed-design.md`
 
 ## Prepared Prompts
 
 ```
 @code-reviewer Review the implementation for issue #[NUMBER]
   Files: [FILE-LIST]
+  Deviations: [DEVIATIONS-OR-NONE]
 ```
 
 ```
-@code-reviewer Review the implementation for issue #[NUMBER]
-  Context: Mode=[MODE], Files=[FILE-LIST], Build=[STATUS], Tests=[STATUS], Deviations=[LIST-OR-NONE]
+@code-reviewer Review only architecture compliance for issue #[NUMBER]
+  Files: [FILE-LIST]
 ```
 
 ```
-@code-reviewer Review only architecture compliance for issue #[NUMBER]
+@code-reviewer Review only unit tests for issue #[NUMBER]
   Files: [FILE-LIST]
 ```
 
@@ -85,31 +89,39 @@ Compare implementation against **both** design documents:
 ### Dimension 2: Architecture Compliance
 Enforce rules from `.github/copilot-instructions.md`:
 - **Dependency Rules**: `Contracts` has zero deps; `*.Services` references only `Contracts`, `DataAccess`, `*.DataModel`; no cross-module refs
+- **Contracts** has zero logic — pure interfaces and DTOs only
 - **Service Registration**: All services use `[Service(typeof(IInterface), ServiceLifetime)]`; registered by interface, not concrete
 - **Data Access**: Services use `IRepository`/`IUnitOfWork` only — never `DbContext` directly
-- **Async**: No `.Result`/`.Wait()` — `async`/`await` end-to-end
 - **Nullability**: `<Nullable>enable</Nullable>` — no suppression (`!`) unless justified
-- **Access Modifiers**: Services are `internal` unless exported via `Contracts`
+- **Access Modifiers**: Implementations are `internal` unless clear justification for `public` (e.g., shared via `Contracts`)
 - **Primary Constructors**: Used for DI injection
 - **No AutoMapper**: Mapping is explicit
 - **Protected Areas**: `Infra/**` and `*.DbContext/**` not modified
 
 ### Dimension 3: Code Quality
 - **Single Responsibility**: Each class/method has one clear purpose
 - **Naming**: Follows conventions (interfaces prefixed `I`, async methods suffixed `Async`)
+- **Comments**: No unnecessary comments — code is self-documenting
 - **Dead Code**: No commented-out code, unused usings, or unreachable paths
 - **Duplication**: No copy-paste across services or methods
 - **Complexity**: Methods are not overly long or deeply nested
 - **Error Handling**: Exceptions are meaningful, not swallowed or generic
+- **Namespaces**: Match folder structure (excluding `Modules`, `Infra`, `UI` prefixes)
+- **Unrelated Files**: No unrelated files modified beyond the declared scope
+- **Deviations**: Coder-declared deviations are justified and acceptable
 
 ### Dimension 4: Test Quality
-If unit tests are in scope (Mode 2 or mixed):
+
+> Use `unit-testing` skill as the standard for test quality assessment
+
 - **Coverage**: Are all behaviors from the detailed design test strategy covered?
 - **Naming**: `{Method}_{Scenario}_{Expected}` convention
 - **AAA Structure**: Arrange / Act / Assert clearly separated
 - **Isolation**: Dependencies faked via NSubstitute; no real I/O
 - **Assertions**: Use FluentAssertions; assert behavior, not implementation
 - **Edge Cases**: Null inputs, empty collections, boundary values tested
+- **Duplication**: No duplicated code in a test class
+- **Coupling**: No coupling between test classes; each tests a single service or behavior
 
 ### Dimension 5: Scope & Hygiene
 - **No Unrelated Changes**: Only files relevant to the issue are modified
@@ -132,12 +144,14 @@ If unit tests are in scope (Mode 2 or mixed):
 - Fetch GitHub issue via `github/issue_read` for requirements and acceptance criteria
 - Read high-level design: `docs/workitems/{issueId}-design.md`
 - Read detailed design: `docs/workitems/{issueId}-detailed-design.md`
+- Build the solution to verify build status 
+- Run the unit  test to verify results (if provided by coder)
 - If a design document is missing, note it as a 🔴 BLOCKER and proceed with available documents
 
 ### 2. Read Implementation
 - Read every file in the provided file list
 - For each file, understand its role (contract, service, entity, interceptor, test, etc.)
-- If build/test status was provided, note failures upfront
+- If build/test status failed, note failures upfront
 
 ### 3. Cross-Reference Design vs. Implementation
 Walk through the detailed design section by section:
@@ -158,6 +172,10 @@ Output structured report (see Response Format below).
 
 ## Response Format
 
+### Summary
+
+Output a summary as following:
+
 ```
 # Code Review: Issue #{id} — {title}
 
@@ -166,64 +184,32 @@ Output structured report (see Response Format below).
 - Detailed: docs/workitems/{id}-detailed-design.md — {found|MISSING}
 
 **Build:** {✅|❌} | **Tests:** {✅|❌}
+**Blockers:** {count} | **Warnings:** {count} | **Suggestions:** {count} | **Notes:** {count}
+
+## Verdict
+  {APPROVE | REQUEST CHANGES | APPROVE WITH SUGGESTIONS}
 
 ## Summary
 {2-4 sentence overview: is the implementation faithful to the design? What are the main concerns?}
 
-## Design Conformance
-
-### Contracts
-{Remarks on interface/DTO/exception conformance}
-
-### Services
-{Remarks on service implementation vs. design}
-
-### Data Model
-{Remarks on entity conformance}
-
-### Interceptors
-{Remarks or "As designed ✅"}
-
-### Error Handling
-{Remarks on fault contracts and exception handling}
-
-### Test Strategy Coverage
-{Remarks on which tests from the design are present/missing}
-
-### Missing or Extra Components
-{Any components in the design but not implemented, or vice versa}
-
-## Architecture Compliance
-{Remarks on dependency rules, service registration, data access patterns, async, nullability, access modifiers}
-
-## Code Quality
-{Remarks on SRP, naming, dead code, duplication, complexity}
+```
 
-## Test Quality
-{Remarks on naming, AAA, isolation, assertions — or "No tests in scope"}
+### Review Report
 
-## Scope & Hygiene
-{Remarks on unrelated changes, commit size, formatting}
+Output a structured review report in markdown format, and save it at `docs/code-reviews/{issueId}-code-reviewer_{timestamp}.md`.
 
-## Remarks Summary
+Use the `code-review-report` skill to generate the report.
+In case of not being able to use the skill, report a error and produce a simple markdown report.
 
-| # | Severity | Dimension | File | Remark |
-|---|----------|-----------|------|--------|
-| 1 | 🔴 | Design | path/File.cs | {short description} |
-| 2 | 🟡 | Architecture | path/File.cs | {short description} |
-| ... | ... | ... | ... | ... |
+## Error Recovery
 
-**Blockers:** {count} | **Warnings:** {count} | **Suggestions:** {count} | **Notes:** {count}
+**Missing design document:** Flag as 🔴 BLOCKER. Review what is possible with available documents and note reduced confidence.
 
-## Verdict
-{APPROVE | REQUEST CHANGES | APPROVE WITH SUGGESTIONS}
+**File in list not found:** Flag as 🔴 BLOCKER. Note the missing file and proceed with remaining files.
 
-{If REQUEST CHANGES: list the blockers that must be resolved}
-{If APPROVE WITH SUGGESTIONS: list the suggestions worth considering}
+**Ambiguous design specification:** Flag as 🟡 WARNING. State the ambiguity and how the implementation interpreted it.
 
-## Recommended Next Step
-{e.g., "Hand off to @coder to apply approved remarks" or "Ready for merge"}
-```
+**No file list provided:** Ask for the file list. Do not guess.
 
 ## What This Agent Does NOT Do
 - Does NOT modify source code or project files
@@ -246,5 +232,4 @@ Output structured report (see Response Format below).
 ## Progress Reporting
 - Announce each major step: "Reading design documents...", "Reviewing contracts...", "Checking architecture compliance..."
 - Report per-dimension progress for large reviews
-- If review requires clarification, ask ONE specific question before proceeding
-````
+- If review requires clarification, ask ONE specific question before proceeding

.github/copilot-instructions.md

@@ -212,6 +212,7 @@ Domain-specific guidance documents located in `.github/skills/`. When a task mat
 |-------|---------|
 | **add-module-plugin** | Step-by-step guide for adding new modules: folder structure, plugin registration, `IModule` initialization, build-order dependencies in `.sln` |
 | **unit-testing** | Test patterns using xUnit, NSubstitute, FluentAssertions: AAA structure, fake naming (`Stub`/`Mock`), collection assertions, `GetTarget` helpers |
+| **code-review-report** | Structured code review report template for PR-importable Markdown output (GitHub / Azure DevOps compatible) |
 
 Usage pattern in this file: `> For [topic], use the **skill-name** skill.`