[Nikolaj Jakobi Nøttrup (Nikolaj1407)]

tools/Test-ReviewFixtures.ps1 removes every full-line comment from a sample before it goes to the model. That is intended and documented (evaluation/README.md: preparation "removes full-line sample comments so neither the article slug, domain, nor expected outcome reveals the answer"). It makes two articles impossible to evaluate, because their rule is about the comments themselves:

• microsoft/knowledge/testing/test-feature-scenario-tags.md - the [FEATURE]/[SCENARIO]/[GIVEN]/[WHEN]/[THEN] tags are full-line comments, so its good sample reaches the model with none of them.
• microsoft/knowledge/style/al-comments-must-not-restate-what-code-already-shows.md - its bad sample consists of comments and reaches the model with none, so the rule can never fire there.

Change

An optional keepComments list in evaluation/review-fixtures.json (each entry an article path and a non-empty reason). Preparation keeps the full-line comments of that article's two samples; the Good/Bad token neutralisation still applies. Both articles are listed. No other case changes. The README documents the key, and says to prefer coverageWaivers when keeping the comments would reveal the expected outcome.

The bad sample of test-feature-scenario-tags had a full-line comment that stated the expected finding ("no FEATURE/SCENARIO/GIVEN/WHEN/THEN tags"). With keepComments it would reach the model, so it is removed; the code shows the missing tags and the article's ## Anti Pattern section already explains them.

Checks

Rebased on main (a4593a1); the existing coverageWaivers and fixture registrations are kept alongside keepComments. Test-ReviewFixtures.ps1 -Root . -PrepareDirectory passes (292 cases, 146/367 paired articles, 21 leaf domains), as do Test-KnowledgeIndex.ps1, Test-SkillIndex.ps1 and Test-ReviewContract.ps1. In the prepared set, exactly three samples keep comments - the tags in test-feature-scenario-tags.good.al and both samples of al-comments-must-not-restate-what-code-already-shows; test-feature-scenario-tags.bad.al now has none. All other samples are unchanged.

🤖 Generated with Claude Code

[Jesper Schulz-Wedde (JesperSchulz)]

Revised — this approval is withdrawn; see my changes-requested review on the same head.

Reviewed 1d27535dca56a17eb1c55a841b8fc66919e17fcb. The targeted comment-preservation mechanism fixes the actual evaluation proxy: comment-based rules must see their comment inputs. Both selected entries have explicit reasons, unused/duplicate entries are rejected, and Good/Bad object-name neutralization remains applied. Existing fixture/contract, skill/schema and frontmatter checks pass.

However, the kept comments of test-feature-scenario-tags.bad.al reveal the expected finding to the model (details in the follow-up review), which the README text added by this PR itself says to avoid.

[Jesper Schulz-Wedde (JesperSchulz)]

Requesting changes on 1d27535dca56a17eb1c55a841b8fc66919e17fcb. This supersedes my earlier approval.

Merge-critical: the kept comments leak the expected outcome for one case

microsoft/knowledge/testing/test-feature-scenario-tags.bad.al:21 is a full-line comment:

// setup mixed with assertions, no clear layers, no FEATURE/SCENARIO/GIVEN/WHEN/THEN tags

With keepComments, it now reaches the model verbatim. It states the expected finding, so the bad case measures whether the model can read the hint, not whether it detects the defect. That contradicts:

• the README rule added in this PR ("prefer coverageWaivers when keeping them would reveal the expected outcome"), and
• the new comment in Test-ReviewFixtures.ps1 ("its samples carry no answer-revealing labels").

You flagged this caveat in the description. Please resolve it in one of these ways:

• Remove the comment from the bad sample, or rephrase it so it doesn't describe the defect. The sample code itself shows the missing tags. If the explanation matters to human readers, move it into the article's ## Anti Pattern text. Keep keepComments for both articles.
• Or list test-feature-scenario-tags under coverageWaivers instead of keepComments, as you offered.

I checked the other three kept samples, and none reveals the answer:

• test-feature-scenario-tags.good.al: its tags are the evidence the rule looks for.
• al-comments-must-not-restate-what-code-already-shows.bad.al: the restating comments are the evidence.
• al-comments-must-not-restate-what-code-already-shows.good.al: its comment explains why, which is the compliant shape.

[Jesper Schulz-Wedde (JesperSchulz)]

Nikolaj Jakobi Nøttrup (@Nikolaj1407) Please refresh this branch against the latest main and resolve the merge conflicts, retaining the current fixture registrations and preparation behavior alongside the comment-preservation change.

This is close to moving forward. Please also address the answer-revealing comment identified in the existing review, so the refreshed fixtures preserve the evidence without giving away the expected finding. We can then review the updated head for merge.

[Nikolaj Jakobi Nøttrup (Nikolaj1407)]

Jesper Schulz-Wedde (@JesperSchulz) Thanks - both done on the new head 12a59ca:

• Rebased on the latest main (a4593a1). The conflicts in evaluation/README.md and evaluation/review-fixtures.json are resolved keeping the current coverageWaivers, fixture registrations and preparation text alongside keepComments.
• Removed the answer-revealing comment from test-feature-scenario-tags.bad.al (separate commit). keepComments stays for both articles; the article's ## Anti Pattern text already explains the missing tags.

Test-ReviewFixtures.ps1 -Root . -PrepareDirectory passes (292 cases, 21 leaf domains). In the prepared set only three samples keep comments: the tags in test-feature-scenario-tags.good.al and both samples of al-comments-must-not-restate-what-code-already-shows.