Improve Code Reviewer agent instructions to catch template rendering issues

[oocx]

Background

During the review of Feature 068 (Parent-Child Resource Grouping), the Code Reviewer agent missed a critical issue: the Azure AD group template was missing the {{ include "/_child_resources.sbn" }} directive, causing ALL member tables to be absent from the generated output.

The issue was only caught during UAT when the maintainer visually inspected the rendered markdown in GitHub/Azure DevOps PRs.

Root cause of the miss: The reviewer focused on complex configuration reference matching logic and trusted that existing snapshot tests proved the feature worked, without manually generating and inspecting the actual rendered output.

Proposed Instructional Improvements

Based on the root cause analysis in code-review-post-uat-fixes.md, the following 5 improvements would help catch similar issues in the future:

---

1. Template Verification Checklist (CRITICAL)

Add to the "Review Checklist" section in .github/modes/code-reviewer.md:

### Template Verification (for features modifying rendering)
- [ ] All provider-specific templates include required shared template includes
- [ ] For parent-child features: Verify `{{ include "/_child_resources.sbn" }}` is present in parent templates
- [ ] Compare template structure against architectural design (e.g., architecture.md Section 4.2 "Template Changes")
- [ ] If child resources should render, grep the generated artifact for the expected child heading (e.g., `grep "#### Members"`)

Why this helps: Forces direct verification of template files rather than assuming they're correct based on test setup.

---

2. Mandatory Manual Artifact Generation (CRITICAL)

Add to the "Review Approach" section:

## Review Approach

2. **Generate test artifacts manually** - Before trusting snapshot tests:
   ```bash
   # Generate a simple test case for the feature
   dotnet run --project src/Oocx.TfPlan2Md -- [simple-test-plan].json --output test-output.md
   
   # Verify the feature-specific output is present
   grep "[expected-pattern]" test-output.md || echo "FEATURE NOT RENDERING"

Do NOT assume snapshot tests are correct just because they exist.

• Snapshots may have been generated before the feature was complete
• Snapshots may have been approved with SNAPSHOT_UPDATE_OK despite being incorrect
• Always verify the actual rendered output matches the specification examples

**Why this helps:** Prevents false confidence from seeing that "snapshot tests exist and pass." The reviewer would have immediately seen that the generated output lacked member tables.

---

### 3. Simplest Possible Test Case First (HIGH PRIORITY)

Add to the "Adversarial Testing" section:

```markdown
## Adversarial Testing

**Start with the simplest possible test case:**
1. For rendering features, create the minimal example (e.g., 1 parent + 1 child)
2. Generate the artifact manually
3. Verify the core feature works before testing edge cases
4. If the simple case fails, diagnose before reviewing complex scenarios

Example for parent-child rendering:
- 1 azuread_group with 1 inline member (CREATE action)
- Generate markdown
- Verify "#### Members" heading and 1-row table exist
- **If this fails, the feature is fundamentally broken regardless of edge case coverage**

Why this helps: Focuses attention on core functionality first. The complexity of configuration reference matching distracted from the basic rendering requirement.

---

4. Line-by-Line Specification-to-Implementation Comparison (HIGH PRIORITY)

Enhance the existing instruction:

## Line-by-line specification comparison

For each acceptance criterion:
1. Read the criterion
2. **For rendering features:** Find the relevant example in `rendering-examples.md`
3. **Generate an artifact that should match that example** (create test data if needed)
4. **Compare the generated output to the example character-by-character**
5. Find the implementing code
6. Find the corresponding test(s)
7. Verify the behavior matches the spec exactly

**Red Flag:** If you cannot find a way to generate output that matches the spec examples, the feature may not be implemented correctly.

Why this helps: The specification had rendering-examples.md with clear examples showing "#### Members" tables. A character-by-character comparison would have immediately revealed the missing tables.

---

5. Distinguish Test Data Issues from Implementation Issues (MEDIUM)

Add to "Critical Questions for Every Review":

## When test data has `(known after apply)` values:

Ask yourself:
1. Is the feature supposed to work WITHOUT the fallback logic? (Can I test it with known values first?)
2. Is the test data designed to test the fallback, or is it incidental?
3. Can I create simpler test data with known values to isolate the core functionality?

**Anti-pattern:** Assuming a feature doesn't work because of missing edge case handling when the core functionality itself is broken.

Why this helps: The reviewer saw (known after apply) in some test data and assumed that was the blocker, without first verifying whether the feature worked for the simpler known-value case.

---

Implementation Priority

Priority	Improvement	Impact
CRITICAL	Template verification checklist	Forces inspection of template files
CRITICAL	Mandatory manual artifact generation	Prevents trusting snapshots blindly
HIGH	Simplest test case first	Focuses on core before edge cases
HIGH	Spec-to-output comparison	Ensures examples match reality
MEDIUM	Test data vs implementation distinction	Avoids misdiagnosis

Recommended Next Steps

1. Update .github/modes/code-reviewer.md with all 5 improvements
2. Add examples/templates for "simplest test case" to feature template
3. Consider adding a "rendering feature checklist" to the feature specification template

Related

• Feature: refactor: rename agents for clarity and consistency #68 Parent-Child Resource Grouping
• Root cause analysis: code-review-post-uat-fixes.md
• UAT failure report: uat-report.md