From b1c1c3bde486003af0684f47f8432b68fa521d88 Mon Sep 17 00:00:00 2001 From: Marcin Polewski Date: Fri, 30 Oct 2026 22:15:42 -0600 Subject: [PATCH 1/4] Give a learned guidance rule only to reviews of the files its instruction-file sources apply to Training records each instruction file's applyTo with its extracted rules and tags a guidance bullet learned only from instruction files with their globs, generalized to any repository (src/**/*.cs becomes **/*.cs). The engine drops tagged bullets that match no changed file, and each part of a large diff gets only the bullets for its files. node training/run.ts --retag rewrites the tags without a model. Refs #36 --- CHANGELOG.md | 4 + README.md | 2 + src/context.ts | 12 +- src/gather.ts | 50 +++++-- src/review.ts | 39 ++++-- test/engine.test.ts | 67 ++++++++- .../instructions/csharp-1.json | 3 + .../instructions/csharp-production-1.json | 3 + .../instructions/csharp-production-2.json | 3 + .../instructions/csharp-production-3.json | 3 + .../instructions/csharp-production-4.json | 3 + .../instructions/csharp-production-5.json | 3 + .../instructions/csharp-testing-1.json | 3 + .../instructions/csharp-testing-2.json | 3 + .../instructions/project-template-1.json | 3 + .../instructions/repository-1.json | 3 + training/run.ts | 129 +++++++++++++++--- 17 files changed, 291 insertions(+), 42 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f12b923..6637189 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), - Consumers pin the action by a release's full commit SHA, with the tag in a comment, as the README now says: CodeQL's `extended` suite reports a tag pin, even an immutable release's, as an unpinned action. This repository's own workflow pins v0.3.1 that way. - Each review posts its own summary comment, at the bottom of the conversation, and the previous summary collapses to one line that links to it, instead of the first summary being edited in place, so a `/review` or re-review shows its verdict and findings where it ran ([#21](https://github.com/NCodeGroup/code-review/issues/21)). +### Fixed + +- A learned guidance rule that came only from instruction files, such as Identity's C# conventions, is tagged with the files they apply to (`` *Applies to:* `**/*.cs` ``) and reaches only reviews, and parts of a diff, that change such a file, so a C# rule no longer reaches a review of TypeScript. `npm run train` writes the tags, and `npm run train -- --retag` rewrites them without a model ([#36](https://github.com/NCodeGroup/code-review/issues/36)). + ## [0.3.1] - 2026-10-31 ### Changed diff --git a/README.md b/README.md index 872f9b5..11a04a2 100644 --- a/README.md +++ b/README.md @@ -16,6 +16,8 @@ repository under review. Results and the trace are scanned for secrets before th Besides the reviewers, **rule checks** go through the rules the context stage picked for the pull request, twelve at a time, on Sonnet: each call checks its rules one by one against every part of the diff, searching the changed files for what the rule's check names. They report as `rule-check` and run on every review, outside the reviewer roster, so selection can't skip them. In the evals they caught about one more expected finding in 55, at about 70% more cost. `CODE_REVIEW_RULE_CHECKS=off` turns them off. +A rule in the learned guidance (`guidance/`) that came only from instruction files, such as C# conventions, carries their files as a tag, `` *Applies to:* `**/*.cs` ``, and reaches a review only when the pull request changes such a file, and in a diff reviewed in parts only the parts that change one. Rules without a tag apply to every file. `npm run train` writes the tags, and `npm run train -- --retag` rewrites them in the committed guidance from each rule's sources without calling a model. + ## Using it ```yaml diff --git a/src/context.ts b/src/context.ts index a8bd787..ff6eff0 100644 --- a/src/context.ts +++ b/src/context.ts @@ -6,7 +6,7 @@ import { existsSync, readdirSync, readFileSync } from 'node:fs'; import path from 'node:path'; import { runAgent, type AgentRun, type AgentResult } from './agent.ts'; -import type { Gathered, RuleFile } from './gather.ts'; +import { bulletGlobs, matchesAnyFile, type Gathered, type RuleFile } from './gather.ts'; export interface IndexedRule { id: string; @@ -14,6 +14,8 @@ export interface IndexedRule { reviewers: string[]; summary: string; text: string; + // The files a learned rule applies to, from its "*Applies to:*" tag; absent when it applies to every file. + globs?: string[]; } export interface Decision { @@ -64,12 +66,18 @@ export function guidanceRules(files: RuleFile[]): IndexedRule[] { const topic = file.name.replace(/\.md$/, ''); const reviewers = reviewersOfTopic(topic); file.text.split('\n').filter((line) => line.startsWith('- **')).forEach((line, i) => { - rules.push({ id: `G-${topic}-${i + 1}`, reviewers, summary: summarize(line), text: line }); + const globs = bulletGlobs(line); + rules.push({ id: `G-${topic}-${i + 1}`, reviewers, summary: summarize(line), text: line, ...(globs.length ? { globs } : {}) }); }); } return rules; } +// The rules that apply to any of these files: an untagged rule applies to every file. +export function rulesForFiles(rules: IndexedRule[], files: string[]): IndexedRule[] { + return rules.filter((rule) => !rule.globs || matchesAnyFile(rule.globs, files)); +} + // One rule per top-level review-only bullet in the repository's conventions, with its continuation lines. They go to // the conventions reviewer. export function repositoryRules(files: RuleFile[]): IndexedRule[] { diff --git a/src/gather.ts b/src/gather.ts index 78fa5b9..45f9027 100644 --- a/src/gather.ts +++ b/src/gather.ts @@ -183,6 +183,11 @@ export function globToRegExp(glob: string): RegExp { return new RegExp(`^${pattern}$`); } +export function matchesAnyFile(globs: string[], files: string[]): boolean { + const patterns = globs.map(globToRegExp); + return files.some((file) => patterns.some((p) => p.test(file))); +} + function frontmatter(text: string): { fields: Map; body: string } { const fields = new Map(); const match = /^---\r?\n([\s\S]*?)\r?\n---\r?\n?/.exec(text); @@ -194,6 +199,32 @@ function frontmatter(text: string): { fields: Map; body: string return { fields, body: text.slice(match[0].length) }; } +// The globs an instruction file's frontmatter applyTo lists. +export function applyToGlobs(text: string): string[] { + return (frontmatter(text).fields.get('applyTo') ?? '').split(',').map((g) => g.trim()).filter(Boolean); +} + +// A learned guidance bullet names the files it applies to in an "*Applies to:*" field, from the instruction files it +// was learned from, such as "*Applies to:* `**/*.cs`". An empty list means the bullet applies to every file. +export const appliesToLabel = '*Applies to:*'; + +export function bulletGlobs(line: string): string[] { + const field = / \*Applies to:\* (.*?)(?: .*<\/sub>)?$/.exec(line)?.[1] ?? ''; + return [...field.matchAll(/`([^`]+)`/g)].map((m) => m[1]!); +} + +// Drops the bullets tagged for files this pull request doesn't change. Untagged bullets and other lines stay. +export function keepBulletsFor(text: string, changed: string[]): string { + return text + .split('\n') + .filter((line) => { + if (!line.startsWith('- **')) return true; + const globs = bulletGlobs(line); + return !globs.length || matchesAnyFile(globs, changed); + }) + .join('\n'); +} + // Drops rules marked ✅ (tool-enforced: the build already fails on them) with their continuation lines. Rules marked // 👁 (review-only) and ⏳ (rolling out) stay, because only a reviewer catches them. export function dropToolEnforced(text: string): string { @@ -228,20 +259,23 @@ export function readReviewRules(basePath: string, changed: string[]): RuleFile[] return readRuleFolder(path.join(basePath, '.github', 'instructions'), '.instructions.md', changed, '.github/instructions'); } -// NCodeGroup-wide rules the training learned (guidance/*.md in the engine), for the files this pull request changes. +// NCodeGroup-wide rules the training learned (guidance/*.md in the engine), for the files this pull request changes: +// a topic file whose applyTo matches a changed file, without the bullets tagged for files the pull request doesn't +// change, so a rule learned from C# conventions never reaches a review of TypeScript. export function readGuidance(guidanceDir: string, changed: string[]): RuleFile[] { - return readRuleFolder(guidanceDir, '.md', changed, 'NCodeGroup guidance'); + return readRuleFolder(guidanceDir, '.md', changed, 'NCodeGroup guidance', (body) => keepBulletsFor(body, changed)) + .filter((file) => file.text.split('\n').some((line) => line.startsWith('- **'))); } -function readRuleFolder(folder: string, suffix: string, changed: string[], label: string): RuleFile[] { +function readRuleFolder(folder: string, suffix: string, changed: string[], label: string, filter = (body: string) => body): RuleFile[] { if (!existsSync(folder)) return []; const files: RuleFile[] = []; for (const name of readdirSync(folder).filter((n) => n.endsWith(suffix)).sort()) { - const { fields, body } = frontmatter(readFileSync(path.join(folder, name), 'utf8')); - const globs = (fields.get('applyTo') ?? '').split(',').map((g) => g.trim()).filter(Boolean); - const patterns = globs.map(globToRegExp); - if (!changed.some((file) => patterns.some((p) => p.test(file)))) continue; - files.push({ name, globs, text: `From ${label}/${name} (applies to ${globs.join(', ')}):\n${dropToolEnforced(body).trim()}` }); + const text = readFileSync(path.join(folder, name), 'utf8'); + const { body } = frontmatter(text); + const globs = applyToGlobs(text); + if (!matchesAnyFile(globs, changed)) continue; + files.push({ name, globs, text: `From ${label}/${name} (applies to ${globs.join(', ')}):\n${filter(dropToolEnforced(body)).trim()}` }); } const general = (f: RuleFile) => (f.globs.every((g) => g === '**') ? 1 : 0); return files.sort((a, b) => general(a) - general(b)); diff --git a/src/review.ts b/src/review.ts index 96935d4..c22bf1d 100644 --- a/src/review.ts +++ b/src/review.ts @@ -5,7 +5,7 @@ import { tmpdir } from 'node:os'; import path from 'node:path'; import { runAgent, type AgentRun, type CallRecord } from './agent.ts'; import { budgets, findingsPerExtraChunk, maxFindings, maxFindingsCap, maxTurns, minConfidence, reviewerTiers, ruleChecksOn, rulesPerCheck, type ModelTier } from './config.ts'; -import { guidanceRules, repositoryRules, selectContext, type Decision, type IndexedRule } from './context.ts'; +import { guidanceRules, repositoryRules, rulesForFiles, selectContext, type Decision, type IndexedRule } from './context.ts'; import { splitDiff, type ChangedFile, type Gathered } from './gather.ts'; export type Severity = 'blocker' | 'should-fix' | 'nit'; @@ -118,6 +118,11 @@ export function ruleBatches(catalog: IndexedRule[], rulesFor: Map !f.generated && !f.shown).map((f) => f.path)]; +} + export function loadCharters(root: string): Charters { const reviewers = new Map(); for (const name of readdirSync(path.join(root, 'reviewers')).filter((n) => n.endsWith('.md')).sort()) { @@ -311,30 +316,38 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte } log(`Reviewers: ${[...selected].join(', ')}${selection.error ? ` (selection failed: ${selection.error})` : ''}`); - // Specialists, each with the picked rules for its charter, and each part of the diff with its own set of them. The - // part of the prompt they share is its first block; one reviewer per model and part goes first and caches it, and the - // rest read the cache instead of each writing it again. + // Specialists, each with the picked rules for its charter, and each part of the diff with its own set of them: a rule + // learned for some files reaches only the parts that change them. The part of the prompt they share is its first + // block; one reviewer per model and part goes first and caches it, and the rest read the cache instead of each + // writing it again. const tiers = reviewerTiers(); const parts = gathered.chunks.length ? gathered.chunks : [gathered.diff]; const reviewRun: AgentRun = { ...run, workdir: mkdtempSync(path.join(tmpdir(), 'code-review-work-')) }; const batches = ruleChecksOn() ? ruleBatches(catalog, context.rulesFor, rulesPerCheck) : []; const jobs = parts.flatMap((diff, part) => { const sharedBlock = `${sharedContext(gathered, context.decisions, diff, part, parts.length)}\n\nThe checkout is at: ${run.checkout}`; + const files = partFiles(gathered.files, diff); + const scoped = (rules: IndexedRule[] | undefined) => rulesForFiles(rules ?? [], files); return [ ...[...selected].map((reviewer) => ({ name: reviewer, part, label: '', tier: tiers[reviewer] ?? ('haiku' as ModelTier), - prompt: [sharedBlock, `${rulesBlock(context.rulesFor.get(reviewer)).trimStart()}\n\nYour charter:\n${charters.reviewers.get(reviewer)!}`], - })), - ...batches.map((batch, i) => ({ - name: ruleCheckName, - part, - label: `rules ${i * rulesPerCheck + 1}-${i * rulesPerCheck + batch.length}`, - tier: 'sonnet' as ModelTier, - prompt: [sharedBlock, `${rulesBlock(batch).trimStart()}\n\nYour charter:\n${charters.ruleCheckPrompt}`], + prompt: [sharedBlock, `${rulesBlock(scoped(context.rulesFor.get(reviewer))).trimStart()}\n\nYour charter:\n${charters.reviewers.get(reviewer)!}`], })), + ...batches.flatMap((batch, i) => { + const rules = scoped(batch); + return rules.length + ? [{ + name: ruleCheckName, + part, + label: `rules ${i * rulesPerCheck + 1}-${i * rulesPerCheck + batch.length}`, + tier: 'sonnet' as ModelTier, + prompt: [sharedBlock, `${rulesBlock(rules).trimStart()}\n\nYour charter:\n${charters.ruleCheckPrompt}`], + }] + : []; + }), ]; }); const first = jobs.filter((job, i) => jobs.findIndex((other) => other.tier === job.tier && other.part === job.part) === i); @@ -361,7 +374,7 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte ...(await mapWithConcurrency(jobs.filter((job) => !first.includes(job)), concurrency, reviewOne)), ]; // One outcome per reviewer across the parts and rule batches: it ran if any of its calls ran. - const outcomes: ReviewerOutcome[] = [...selected, ...(batches.length ? [ruleCheckName] : [])].map((name) => { + const outcomes: ReviewerOutcome[] = [...selected, ...(jobs.some((job) => job.name === ruleCheckName) ? [ruleCheckName] : [])].map((name) => { const mine = perJob.filter((o) => o.name === name); const errors = mine.flatMap((o) => (o.error ? [o.error] : [])); return { diff --git a/test/engine.test.ts b/test/engine.test.ts index b43d6ad..5a85961 100644 --- a/test/engine.test.ts +++ b/test/engine.test.ts @@ -1,5 +1,5 @@ import assert from 'node:assert/strict'; -import { mkdirSync, mkdtempSync, symlinkSync, writeFileSync } from 'node:fs'; +import { mkdirSync, mkdtempSync, readdirSync, readFileSync, symlinkSync, writeFileSync } from 'node:fs'; import { tmpdir } from 'node:os'; import path from 'node:path'; import { test } from 'node:test'; @@ -147,6 +147,71 @@ test('holdoutLabels names the reviews and decision records the eval cases draw o assert.deepEqual([...labels].sort(), ['Identity ADR-0266', 'Identity review of #309', 'Identity review of #341', 'Identity review of #420']); }); +test('a guidance bullet learned from C# instruction files reaches a review of .cs files and not one of .ts files', async () => { + const { readGuidance } = await import('../src/gather.ts'); + const { guidanceRules } = await import('../src/context.ts'); + const dir = mkdtempSync(path.join(tmpdir(), 'guidance-')); + writeFileSync(path.join(dir, 'code-organization.md'), [ + '---', 'description: "x"', 'applyTo: "**"', '---', '', '# Code organization', '', + '- **Keep functions to three parameters.** Why. *Check:* Count them. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md', + '- **Name things by purpose.** Why. *Check:* Look. Identity ADR-0001', + '', + ].join('\n')); + const statements = (changed: string[]) => guidanceRules(readGuidance(dir, changed)).map((r) => r.summary); + assert.deepEqual(statements(['src/comments.ts']), ['Name things by purpose.']); + assert.deepEqual(statements(['src/Comments.cs']), ['Keep functions to three parameters.', 'Name things by purpose.']); + assert.deepEqual(statements(['src/comments.ts', 'src/Comments.cs']), ['Keep functions to three parameters.', 'Name things by purpose.']); + assert.deepEqual(guidanceRules(readGuidance(dir, ['src/Comments.cs']))[0]!.globs, ['**/*.cs']); +}); + +test('each part of a large diff gets only the guidance rules tagged for its files', async () => { + const { partFiles } = await import('../src/review.ts'); + const { rulesForFiles } = await import('../src/context.ts'); + const csharp = { id: 'G-a-1', reviewers: ['conventions'], summary: 'cs', text: 'cs', globs: ['**/*.cs'] }; + const anywhere = { id: 'G-a-2', reviewers: ['conventions'], summary: 'any', text: 'any' }; + const files: ChangedFile[] = [ + { path: 'src/a.ts', additions: 1, deletions: 0, generated: false, shown: true }, + { path: 'src/A.cs', additions: 1, deletions: 0, generated: false, shown: true }, + ]; + const tsPart = 'diff --git a/src/a.ts b/src/a.ts\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1 @@\n+x\n'; + assert.deepEqual(rulesForFiles([csharp, anywhere], partFiles(files, tsPart)).map((r) => r.id), ['G-a-2']); + // A changed file no part shows may be read by any part's reviewers, so its rules reach every part. + files[1]!.shown = false; + assert.deepEqual(rulesForFiles([csharp, anywhere], partFiles(files, tsPart)).map((r) => r.id), ['G-a-1', 'G-a-2']); +}); + +test('training tags a guidance rule with the files its instruction-file sources apply to', async () => { + const { formatBullet, generalGlob, parseBullet, retagGuidance, ruleScope, sourceScopes } = await import('../training/run.ts'); + assert.equal(generalGlob('src/**/*.cs'), '**/*.cs'); + assert.equal(generalGlob('tests/**/*.cs'), '**/*.cs'); + assert.equal(generalGlob('**'), '**'); + const scopes = sourceScopes([ + { label: 'Identity csharp-production.instructions.md', applyTo: ['src/**/*.cs'] }, + { label: 'Identity repository.instructions.md', applyTo: ['**'] }, + { label: 'Identity ADR-0001' }, + ]); + assert.deepEqual(ruleScope(['Identity csharp-production.instructions.md'], scopes), ['**/*.cs']); + // Any source that applies to every file, or that isn't an instruction file, leaves the rule for every file. + assert.deepEqual(ruleScope(['Identity csharp-production.instructions.md', 'Identity repository.instructions.md'], scopes), []); + assert.deepEqual(ruleScope(['Identity csharp-production.instructions.md', 'Identity ADR-0001'], scopes), []); + const rule = { statement: 'Keep functions small.', why: 'Why.', check: 'Look.', note: 'Note.', sources: ['Identity csharp-production.instructions.md'] }; + const tagged = formatBullet(rule, ['**/*.cs']); + assert.equal(tagged, '- **Keep functions small.** Why. *Check:* Look. *Note:* Note. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md'); + assert.deepEqual(parseBullet(tagged), rule); + assert.equal(retagGuidance(formatBullet(rule, []), scopes), tagged); +}); + +test('the committed guidance carries the tags its sources give it', async () => { + const { retagGuidance, sourceScopes } = await import('../training/run.ts'); + const scopes = sourceScopes(); + assert.deepEqual(scopes.get('Identity csharp-production.instructions.md'), ['**/*.cs']); + const dir = path.resolve(import.meta.dirname, '..', 'guidance'); + for (const name of readdirSync(dir)) { + const text = readFileSync(path.join(dir, name), 'utf8'); + assert.equal(retagGuidance(text, scopes), text, `${name} is out of date: run node training/run.ts --retag`); + } +}); + test('findingLimit grows with the parts of a large diff', () => { assert.equal(findingLimit(1), 40); assert.equal(findingLimit(3), 80); diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-1.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-1.json index 7715ae7..9808584 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-1.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-1.json @@ -2,6 +2,9 @@ "hash": "fd282c7ea1dc68e5", "promptHash": "358791b74fd66fcf", "label": "Identity csharp.instructions.md", + "applyTo": [ + "**/*.cs" + ], "rules": [ { "statement": "Place each test file in the same relative folder as the code under test, and give it the production namespace with a `.Tests` suffix.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-1.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-1.json index f82c509..1e4d35b 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-1.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-1.json @@ -2,6 +2,9 @@ "hash": "da34a067269577be", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-production.instructions.md", + "applyTo": [ + "src/**/*.cs" + ], "rules": [ { "statement": "Async methods return ValueTask/ValueTask, carry an Async suffix, and take a CancellationToken as the last parameter; use Task only where a framework contract you don't own requires it, such as middleware InvokeAsync, minimal-API handlers or mediator handlers.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-2.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-2.json index fa0c9f3..a1b7bbc 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-2.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-2.json @@ -2,6 +2,9 @@ "hash": "dd0ee657c251e90f", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-production.instructions.md", + "applyTo": [ + "src/**/*.cs" + ], "rules": [ { "statement": "Declare one type per file, so an interface and its implementation go in separate files; the only exception is a generic interface and its non-generic sibling of the same name (IFoo and IFoo), which may share a file.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-3.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-3.json index b7136b6..e63dd46 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-3.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-3.json @@ -2,6 +2,9 @@ "hash": "82693c3239dfe867", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-production.instructions.md", + "applyTo": [ + "src/**/*.cs" + ], "rules": [ { "statement": "Keep a host's Program.cs composition-only: a flat list of single `AddX`/`UseX`/`MapX` calls with no inline `if`, loops, policy builders, `new`-ed options, configuration-section parsing, consts or dev/test helpers; move such logic into the owning package as an extension.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-4.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-4.json index 8ef2071..2d144b7 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-4.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-4.json @@ -2,6 +2,9 @@ "hash": "ea280e28b2931949", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-production.instructions.md", + "applyTo": [ + "src/**/*.cs" + ], "rules": [ { "statement": "Never disable warnings-as-errors (`TreatWarningsAsErrors=false`) in any project, tests and hosts included; relax a single rule with a justified `.editorconfig` entry or a narrowly scoped `#pragma`.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-5.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-5.json index 73f6a11..a6c176c 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-production-5.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-production-5.json @@ -2,6 +2,9 @@ "hash": "4b0c3a118f93ad45", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-production.instructions.md", + "applyTo": [ + "src/**/*.cs" + ], "rules": [ { "statement": "Give every type or member you add, or whose signature or behavior you change, a `///` XML doc comment, but do not add doc comments to undocumented code you are not otherwise touching.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-1.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-1.json index 73d40c1..226d2f2 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-1.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-1.json @@ -2,6 +2,9 @@ "hash": "7d7f9c27b5e8070b", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-testing.instructions.md", + "applyTo": [ + "tests/**/*.cs" + ], "rules": [ { "statement": "Mirror the folder structure and namespace of the code under test in test files, adding a `.Tests` suffix to the namespace and a `Tests` suffix to the class name.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-2.json b/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-2.json index 82ef8a7..13b30ee 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-2.json +++ b/training/extracted/NCodeGroup-Identity/instructions/csharp-testing-2.json @@ -2,6 +2,9 @@ "hash": "b485de1d78d99618", "promptHash": "358791b74fd66fcf", "label": "Identity csharp-testing.instructions.md", + "applyTo": [ + "tests/**/*.cs" + ], "rules": [ { "statement": "Test persistence behavior that depends on SaveChanges side effects (concurrency tokens, cascade deletes, SaveChanges interceptors, value converters) against a real EF Core DbContext provider, not only through a mocked store.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/project-template-1.json b/training/extracted/NCodeGroup-Identity/instructions/project-template-1.json index 3d799a3..db90980 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/project-template-1.json +++ b/training/extracted/NCodeGroup-Identity/instructions/project-template-1.json @@ -2,6 +2,9 @@ "hash": "79629ad9cfbe64d6", "promptHash": "358791b74fd66fcf", "label": "Identity project-template.instructions.md", + "applyTo": [ + "**" + ], "rules": [ { "statement": "Never commit `PLAN.md`. It is a local, gitignored handoff file between the planner and the executor.", diff --git a/training/extracted/NCodeGroup-Identity/instructions/repository-1.json b/training/extracted/NCodeGroup-Identity/instructions/repository-1.json index 59550c7..822a6b4 100644 --- a/training/extracted/NCodeGroup-Identity/instructions/repository-1.json +++ b/training/extracted/NCodeGroup-Identity/instructions/repository-1.json @@ -2,6 +2,9 @@ "hash": "437c4352b1ed2497", "promptHash": "358791b74fd66fcf", "label": "Identity repository.instructions.md", + "applyTo": [ + "**" + ], "rules": [ { "statement": "In pre-release code with no shipped consumers, make the change as a clean cutover (rename, move, remove or change signatures directly) instead of adding compatibility shims, obsolete aliases or duplicate settings that keep the old shape.", diff --git a/training/run.ts b/training/run.ts index 1f3bca0..183c5b5 100644 --- a/training/run.ts +++ b/training/run.ts @@ -3,13 +3,16 @@ // // node training/run.ts --repo NCodeGroup/Identity --checkout C:\GitHub\NCode\Identity [--maintainer polewskm] // node training/run.ts --generalize-only [--holdout-only] +// node training/run.ts --retag // // 1. Extract: Opus reads each source and lists the rules a reviewer could check, phrased in general terms. The result // is saved under training/extracted// with the source's content hash, so a rerun reads only sources that -// changed. +// changed. An instruction file's rules also keep the file's applyTo globs. // 2. Generalize: Opus merges every repository's extracted rules, topic by topic, into guidance/.md, which the // engine gives the conventions reviewer in every repository. training/feedback.md holds the maintainer's -// corrections, and every generalization follows them. +// corrections, and every generalization follows them. Each rule learned only from instruction files is tagged with +// the files they apply to ("*Applies to:* `**/*.cs`"), and the engine gives it only to reviews that change such a +// file. --retag rewrites those tags in guidance/ and guidance-holdout/ from the rules' sources, without a model. // 3. Hold out: the same generalization, without the review comments and decision records of the eval cases' pull // requests, into guidance-holdout/. The evals use it, so a catch shows what the bot learned in general rather than // the answer to that case. Production uses guidance/. @@ -22,6 +25,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { runAgent, type AgentRun, type CallRecord } from '../src/agent.ts'; +import { appliesToLabel, applyToGlobs, bulletGlobs } from '../src/gather.ts'; import { startProxy } from '../src/proxy.ts'; import { mapWithConcurrency } from '../src/review.ts'; @@ -32,7 +36,8 @@ const topics = [ 'dependency-injection', 'persistence', 'code-organization', 'documentation', 'performance', ] as const; -interface Source { kind: string; id: string; label: string; text: string } +// applyTo: the globs of the instruction file a source comes from, as its frontmatter lists them. +interface Source { kind: string; id: string; label: string; text: string; applyTo?: string[] } interface Rule { statement: string; why: string; @@ -43,7 +48,7 @@ interface Rule { applies_to: string[]; enforced_by_tools: boolean; } -interface Extracted { hash: string; promptHash: string; label: string; rules: Rule[] } +interface Extracted { hash: string; promptHash: string; label: string; applyTo?: string[]; rules: Rule[] } function arg(name: string): string | undefined { const index = process.argv.indexOf(`--${name}`); @@ -103,8 +108,10 @@ function conventionFiles(checkout: string, repoName: string): Source[] { const instructions = path.join(checkout, '.github', 'instructions'); if (existsSync(instructions)) { for (const name of readdirSync(instructions).filter((n) => n.endsWith('.instructions.md')).sort()) { - chunkByHeading(readFileSync(path.join(instructions, name), 'utf8')).forEach((text, i) => - sources.push({ kind: 'instructions', id: `${name.replace('.instructions.md', '')}-${i + 1}`, label: `${repoName} ${name}`, text }), + const file = readFileSync(path.join(instructions, name), 'utf8'); + const applyTo = applyToGlobs(file); + chunkByHeading(file).forEach((text, i) => + sources.push({ kind: 'instructions', id: `${name.replace('.instructions.md', '')}-${i + 1}`, label: `${repoName} ${name}`, text, ...(applyTo.length ? { applyTo } : {}) }), ); } } @@ -193,6 +200,12 @@ const guidanceSchema = { additionalProperties: false, }; +function saveExtracted(file: string, { hash, promptHash, label, applyTo, rules }: Extracted): void { + mkdirSync(path.dirname(file), { recursive: true }); + const saved: Extracted = { hash, promptHash, label, ...(applyTo?.length ? { applyTo } : {}), rules }; + writeFileSync(file, `${JSON.stringify(saved, null, 2)}\n`); +} + async function extract(run: AgentRun, sources: Source[], outDir: string, calls: CallRecord[]): Promise { const prompt = readFileSync(path.join(root, 'training', 'prompts', 'extract.md'), 'utf8'); const promptHash = sha(prompt); @@ -202,7 +215,12 @@ async function extract(run: AgentRun, sources: Source[], outDir: string, calls: const hash = sha(source.text); if (existsSync(file)) { const saved = JSON.parse(readFileSync(file, 'utf8')) as Extracted; - if (saved.hash === hash && saved.promptHash === promptHash) { reused++; return; } + if (saved.hash === hash && saved.promptHash === promptHash) { + // An instruction file whose applyTo changed and whose text didn't keeps its rules with the new globs. + if ((saved.applyTo ?? []).join(',') !== (source.applyTo ?? []).join(',')) saveExtracted(file, { ...saved, applyTo: source.applyTo ?? [] }); + reused++; + return; + } } const result = await runAgent<{ rules: Rule[] }>(run, { role: `extract:${source.kind}`, @@ -216,31 +234,101 @@ async function extract(run: AgentRun, sources: Source[], outDir: string, calls: }); calls.push(result.record); if (!result.output) { failed++; console.log(` ${source.label}: failed (${result.error})`); return; } - mkdirSync(path.dirname(file), { recursive: true }); - const saved: Extracted = { hash, promptHash, label: source.label, rules: result.output.rules }; - writeFileSync(file, `${JSON.stringify(saved, null, 2)}\n`); + saveExtracted(file, { hash, promptHash, label: source.label, ...(source.applyTo ? { applyTo: source.applyTo } : {}), rules: result.output.rules }); console.log(` ${source.label}: ${result.output.rules.length} rules, $${result.record.costUsd.toFixed(3)}`); }); console.log(`Extracted ${sources.length - reused - failed} sources, reused ${reused}, failed ${failed}.`); } -function loadExtracted(): { rule: Rule; source: string }[] { - const all: { rule: Rule; source: string }[] = []; +function readExtracted(): Extracted[] { + const all: Extracted[] = []; const base = path.join(root, 'training', 'extracted'); const walk = (dir: string): void => { for (const entry of readdirSync(dir, { withFileTypes: true })) { const full = path.join(dir, entry.name); if (entry.isDirectory()) walk(full); - else if (entry.name.endsWith('.json')) { - const saved = JSON.parse(readFileSync(full, 'utf8')) as Extracted; - for (const rule of saved.rules) all.push({ rule, source: saved.label }); - } + else if (entry.name.endsWith('.json')) all.push(JSON.parse(readFileSync(full, 'utf8')) as Extracted); } }; if (existsSync(base)) walk(base); return all; } +function loadExtracted(): { rule: Rule; source: string }[] { + return readExtracted().flatMap((saved) => saved.rules.map((rule) => ({ rule, source: saved.label }))); +} + +// The files each instruction-file source applies to, by its label, as globs for any NCodeGroup repository. +export function sourceScopes(extracted: Pick[] = readExtracted()): Map { + const scopes = new Map(); + for (const { label, applyTo } of extracted) if (applyTo?.length) scopes.set(label, applyTo.map(generalGlob)); + return scopes; +} + +// One repository's folders don't carry over to another, so a glob keeps only its file pattern: src/**/*.cs and +// tests/**/*.cs both become **/*.cs. +export function generalGlob(glob: string): string { + const name = glob.split('/').pop()!; + return name === '**' || name === '' ? '**' : `**/${name}`; +} + +// The globs a learned rule applies to: those of the instruction files it was learned from. A rule with any source that +// applies to every file, or isn't an instruction file (a decision record, a review, a document), applies to every file, +// and gets no globs. +export function ruleScope(sources: string[], scopes: Map): string[] { + const globs = new Set(); + for (const source of sources) { + const scope = scopes.get(source); + if (!scope || scope.includes('**')) return []; + for (const glob of scope) globs.add(glob); + } + return [...globs].sort(); +} + +export interface GuidanceRule { statement: string; why: string; check: string; sources: string[]; note?: string } + +// One guidance bullet: "- **statement** why *Check:* check *Note:* note *Applies to:* `glob` sources". +export function formatBullet(rule: GuidanceRule, globs: string[]): string { + const scope = globs.length ? ` ${appliesToLabel} ${globs.map((g) => `\`${g}\``).join(', ')}` : ''; + return `- **${rule.statement}** ${rule.why} *Check:* ${rule.check}${rule.note ? ` *Note:* ${rule.note}` : ''}${scope} ${rule.sources.join('; ')}`; +} + +export function parseBullet(line: string): GuidanceRule | undefined { + const match = /^- \*\*(.+?)\*\* (.*?) \*Check:\* (.*?)(?: \*Note:\* (.*?))?(?: \*Applies to:\* (?:`[^`]+`(?:, )?)+)? (.*)<\/sub>$/.exec(line); + if (!match) return undefined; + const [, statement, why, check, note, sources] = match; + return { statement: statement!, why: why!, check: check!, ...(note ? { note } : {}), sources: sources!.split('; ') }; +} + +// Rewrites each bullet's "*Applies to:*" tag in a guidance file from its sources, with the same formatter a training +// run uses, and changes nothing else. Refuses a bullet the formatter wouldn't write back exactly as it was. +export function retagGuidance(text: string, scopes: Map): string { + return text + .split('\n') + .map((line) => { + if (!line.startsWith('- **')) return line; + const rule = parseBullet(line); + if (!rule || formatBullet(rule, bulletGlobs(line)) !== line) throw new Error(`Can't retag this bullet: ${line.slice(0, 120)}`); + return formatBullet(rule, ruleScope(rule.sources, scopes)); + }) + .join('\n'); +} + +function retag(folders: string[]): void { + const scopes = sourceScopes(); + for (const folder of folders) { + const dir = path.join(root, folder); + if (!existsSync(dir)) continue; + for (const name of readdirSync(dir).filter((n) => n.endsWith('.md')).sort()) { + const file = path.join(dir, name); + const text = readFileSync(file, 'utf8'); + const retagged = retagGuidance(text, scopes); + if (retagged !== text) writeFileSync(file, retagged); + console.log(` ${folder}/${name}: ${retagged.split('\n').filter((l) => l.startsWith('- **') && bulletGlobs(l).length).length} tagged rules`); + } + } +} + const titles: Record = { 'allocations-and-buffers': 'Allocations and buffers', 'text-and-encoding': 'Text and encoding', 'async-and-concurrency': 'Async and concurrency', 'public-api': 'Public API', 'errors-and-validation': 'Errors and validation', 'security-and-crypto': 'Security and cryptography', @@ -254,6 +342,7 @@ async function generalize(run: AgentRun, calls: CallRecord[], folder: string, ex const feedbackFile = path.join(root, 'training', 'feedback.md'); const feedback = existsSync(feedbackFile) ? readFileSync(feedbackFile, 'utf8') : ''; const usable = loadExtracted().filter(({ rule, source }) => rule.scope !== 'repository' && !rule.enforced_by_tools && !exclude.has(source)); + const scopes = sourceScopes(); console.log(`${folder}/: ${usable.length} rules${exclude.size ? `, holding out ${exclude.size} sources` : ''}`); const out = path.join(root, folder); rmSync(out, { recursive: true, force: true }); @@ -262,7 +351,7 @@ async function generalize(run: AgentRun, calls: CallRecord[], folder: string, ex const rules = usable.filter(({ rule }) => rule.topic === topic); if (!rules.length) return; const input = rules.map(({ rule, source }) => ({ ...rule, source })); - const result = await runAgent<{ description: string; applies_to: string[]; rules: { statement: string; why: string; check: string; sources: string[]; note?: string }[] }>(run, { + const result = await runAgent<{ description: string; applies_to: string[]; rules: GuidanceRule[] }>(run, { role: `generalize:${topic}`, tier: 'opus', systemPrompt: prompt, @@ -285,7 +374,7 @@ async function generalize(run: AgentRun, calls: CallRecord[], folder: string, ex '', '', '', - ...final.map((r) => `- **${r.statement}** ${r.why} *Check:* ${r.check}${r.note ? ` *Note:* ${r.note}` : ''} ${r.sources.join('; ')}`), + ...final.map((r) => formatBullet(r, ruleScope(r.sources, scopes))), '', ]; writeFileSync(path.join(out, `${topic}.md`), lines.join('\n')); @@ -294,6 +383,10 @@ async function generalize(run: AgentRun, calls: CallRecord[], folder: string, ex } async function main(): Promise { + if (process.argv.includes('--retag')) { + retag(['guidance', 'guidance-holdout']); + return; + } const key = process.env['CODE_REVIEW_BEDROCK_KEY'] ?? process.env['AWS_BEARER_TOKEN_BEDROCK']; if (!key) throw new Error('Set AWS_BEARER_TOKEN_BEDROCK to train on Bedrock.'); const proxy = await startProxy({ bedrockKey: key }); From df3325ba35b5625a992b7d075a2f0209c9992fc3 Mon Sep 17 00:00:00 2001 From: Marcin Polewski Date: Fri, 30 Oct 2026 22:15:42 -0600 Subject: [PATCH 2/4] Retag the learned guidance from its sources Generated by node training/run.ts --retag: rules learned only from Identity's C# instruction files now apply to **/*.cs. Refs #36 --- guidance/allocations-and-buffers.md | 6 +-- guidance/async-and-concurrency.md | 6 +-- guidance/code-organization.md | 16 +++--- guidance/dependency-injection.md | 6 +-- guidance/documentation.md | 22 ++++---- guidance/errors-and-validation.md | 4 +- guidance/logging-and-telemetry.md | 2 +- guidance/performance.md | 12 ++--- guidance/persistence.md | 4 +- guidance/protocols.md | 2 +- guidance/public-api.md | 10 ++-- guidance/security-and-crypto.md | 2 +- guidance/testing.md | 78 ++++++++++++++--------------- guidance/text-and-encoding.md | 4 +- 14 files changed, 87 insertions(+), 87 deletions(-) diff --git a/guidance/allocations-and-buffers.md b/guidance/allocations-and-buffers.md index 01a4f2a..3368b12 100644 --- a/guidance/allocations-and-buffers.md +++ b/guidance/allocations-and-buffers.md @@ -9,7 +9,7 @@ applyTo: "**/*.cs" - **Cap the size of every response from an untrusted outbound fetch twice: reject on the advertised Content-Length, and enforce a hard byte limit while streaming the body.** A Content-Length that is missing or false must not be able to exhaust memory. *Check:* Flag untrusted responses read with ReadAsStringAsync, ReadAsByteArrayAsync or an unbounded stream copy. Also flag code that trusts Content-Length alone and does not count bytes as it reads. Identity ADR-0095 - **Keep secret material (passwords, keys, tokens, decrypted plaintext, and their decoded or re-encoded intermediate copies) in sensitive buffers from NCode.Buffers, such as `BufferFactory.CreatePooledBufferWriter(isSensitive: true)` or `BufferFactory.Rent(..., isSensitive: true, ...)`, which are pinned and zeroed on release. Never hold secrets in a `string`, and dispose the buffers as soon as they are used. Only an encrypted envelope may be a `string`.** A string, a plain array or an ordinary pooled buffer cannot be wiped reliably, so secret bytes stay in managed memory. *Check:* Flag secrets returned or held as `string`/`byte[]`, passed through `Encoding.GetString` or array-returning `Encoding.GetBytes`, or rented from `ArrayPool`, `MemoryPool.Shared` or a non-sensitive writer. Also flag sensitive rentals that are not disposed right after use. Identity ADR-0230; Identity ADR-0235; Identity ADR-0238; Identity ADR-0265; Identity review of #420; Identity csharp-production.instructions.md -- **Hold a secret that is stored in a field or kept across an `await` as a sensitive `BufferFactory.Rent(..., isSensitive: true, out Memory)` lease, never as a bare array or a `CreatePinnedArray` ref struct.** `CreatePinnedArray` returns a ref struct that only works synchronously inside one method. The lease keeps the secret pinned and zeroes it when it is returned. *Check:* Flag secrets kept in fields or used across awaits as plain arrays, and `CreatePinnedArray` results that would need to escape the method. Identity csharp-production.instructions.md +- **Hold a secret that is stored in a field or kept across an `await` as a sensitive `BufferFactory.Rent(..., isSensitive: true, out Memory)` lease, never as a bare array or a `CreatePinnedArray` ref struct.** `CreatePinnedArray` returns a ref struct that only works synchronously inside one method. The lease keeps the secret pinned and zeroes it when it is returned. *Check:* Flag secrets kept in fields or used across awaits as plain arrays, and `CreatePinnedArray` results that would need to escape the method. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Encode public, non-secret material such as SPKI public keys with a non-sensitive pooled writer (`BufferFactory.CreatePooledBufferWriter(isSensitive: false)`), and keep the sensitive secure pool for secret material.** Public data does not need the more expensive pinned secure pool, while private data must never go through the non-sensitive pool. *Check:* Flag public-key or other non-secret encoding that uses a sensitive buffer, and private-key material written to a non-sensitive writer. Identity ADR-0115 - **When an operation produces output whose length is known only after it runs (decrypt, unwrap, a remote response), write it to a caller-supplied `IBufferWriter`, call `Advance` only after the operation succeeds, and zero anything already written if it fails. Use a `Span` destination with `out int bytesWritten` only when the size is known in advance.** Callers who cannot size a buffer end up guessing, which hides real errors as 'destination too small'. Secret bytes from a partly finished failed operation must not stay in the caller's buffer. *Check:* Flag unwrap/decrypt APIs that return a new `byte[]` or take a fixed Span/Memory destination for output of unknown length. Flag `Advance` called before success, failure paths that leave partial output uncleared, and callers that over-rent (for example 2x) to guess a size. Identity ADR-0279; Identity review of #370 - **When decoding (for example base64url) into a buffer, size the buffer from an upper bound or the decoder's own byte-count API, then use or verify the `bytesWritten` the decoder returns. Never trust a length you computed yourself.** A hand-computed length can be wrong for some input lengths (for example lengths that are not multiples of three), which gives a mis-sized buffer or trailing garbage. *Check:* Flag decode calls whose output length is computed ahead of time and used without checking the returned `bytesWritten` or decoded length. *Note:* The production instructions size JWS/JWE segment buffers with `Base64Url.GetByteCountForDecode` and treat slicing by bytesWritten as unnecessary. Even with an exact count API, still check that bytesWritten matches. Identity ADR-0092; Identity csharp-production.instructions.md @@ -22,10 +22,10 @@ applyTo: "**/*.cs" - **Give APIs that produce binary data (encode, export, serialize, key/DER material, JSON records, stream-body reads) `ReadOnlySpan`/`ReadOnlyMemory` inputs, and have them write to a caller-supplied generic `ref TWriter destination` where `TWriter : IBufferWriter, allows ref struct`, or to a `Utf8JsonWriter`, instead of returning a new `byte[]` or `string`.** This avoids per-call allocations, lets callers choose pooled or sensitive storage, accepts ref struct writers without boxing, and lets readers parse straight from a pooled `ReadOnlySequence` with no copy. *Check:* Flag new or changed methods that return `byte[]` or take `byte[]` parameters for binary data. Flag a plain `IBufferWriter` parameter where the generic `allows ref struct` form would work; when the plain form is needed (for example to feed a Utf8JsonWriter), the reason should be documented. Flag body-read helpers that return arrays, and callers that call `ToArray()` before a parse that accepts a sequence. When a ref struct writer is the destination of `Utf8JsonWriter` output, stage the output in a pooled buffer and copy it. Identity ADR-0111; Identity ADR-0115; Identity ADR-0116; Identity ADR-0283; Identity csharp-production.instructions.md; Identity review of #382; Identity review of #420 - **Give each public producer that writes to a caller-supplied span or writer a public sizing method on the same type (`GetXxxLength`/`GetByteCount`). When a public encode or decode method takes a `Span` destination, also give it an `IBufferWriter` (`ref TWriter`) overload.** Callers can then size destinations exactly, or skip sizing and allocating altogether. *Check:* Flag new span- or writer-based Write/Wrap/Encode/Decode methods that have no matching length method, and span-only public encode/decode methods that have no buffer-writer overload. Identity ADR-0279; Identity ADR-0235; Identity review of #375; Identity review of #420 - **Get a contiguous span from a multi-segment `Sequence`/`ReadOnlySequence` with the shared `GetSpanLease(isSensitive:)` helper, never with `.ToArray()` or by concatenating segments by hand.** The helper leases a contiguous view without allocating, and zeroes it when the data is sensitive. *Check:* Flag `.ToArray()` on sequences and hand-written loops that copy segments to flatten a sequence. Identity ADR-0116; Identity csharp-production.instructions.md -- **Use the `out Span` overload of `BufferFactory.Rent` by default, use `out Memory` only for buffers kept in a field or used across an `await`, and never slice the returned buffer back to the requested length.** The out buffer is already exactly the requested length, and Span is cheaper for local, synchronous use. *Check:* Flag `.Slice(0, length)` or range slices on a buffer returned by `Rent`, and `out Memory` used only locally and synchronously. Identity csharp-production.instructions.md +- **Use the `out Span` overload of `BufferFactory.Rent` by default, use `out Memory` only for buffers kept in a field or used across an `await`, and never slice the returned buffer back to the requested length.** The out buffer is already exactly the requested length, and Span is cheaper for local, synchronous use. *Check:* Flag `.Slice(0, length)` or range slices on a buffer returned by `Rent`, and `out Memory` used only locally and synchronously. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **When a serializer has to hand back transient bytes, return a disposable rented-buffer lease that the caller disposes with `using`, not a new `byte[]`, and document that consumers must copy the value before their write completes.** This removes the allocation, and the buffer can go back to the pool safely once the consumer has copied it. *Check:* Flag serialize methods that return a `byte[]` which is written to a store and then dropped. Identity review of #420 - **Hash without heap allocations. Use the span overloads (`SHA256.HashData(source, destination)`) into a `stackalloc` destination of the hash size. Feed multi-part input through `IncrementalHash.AppendData` instead of concatenating it. Compute thumbprints from raw DER bytes rather than building an `X509Certificate2` per candidate. When the same fixed-size hash runs several times in a row, allocate the output buffer once in the caller and pass it in as a `Span`.** Hash outputs are small and fixed in size. Concatenating input, creating certificate objects and allocating per call all add allocations and native resources that a hash or thumbprint check does not need. *Check:* Flag array-returning `HashData`/`ComputeHash` whose result is only encoded or compared, prefix+message concatenation done only to hash it, `X509Certificate2` created in loops just for a thumbprint, and repeated hashing helpers that each allocate their own output. Identity ADR-0107; Identity ADR-0116; Identity ADR-0273; Identity review of #341; Identity review of #420 - **Back immutable JSON-wrapping models, and records you read back, with a detached `JsonElement` from `JsonElement.Parse`/`JsonElement.ParseValue`, rather than a mutable `JsonObject` that needs defensive `DeepClone` calls or a `JsonDocument` kept alive.** A detached JsonElement is immutable and owns no pooled `JsonDocument`, so there is nothing to clone or dispose. *Check:* Flag immutable models that store a `JsonObject` and DeepClone it, `JsonDocument` instances held just to keep an element, and `JsonDocument.Parse` used to read back stored records. Identity ADR-0119; Identity ADR-0283 -- **Produce a `string` only once, at a storage or wire boundary, with `string.Create`. Pass span inputs through a readonly ref struct state and check the written count against the length. Do not fill a rented char buffer and then call `new string(span)`.** `string.Create` writes in place with no extra copy, and the count check stops a '\0' remainder from being persisted. *Check:* Flag `new string(span)` after filling a rented buffer, and `string.Create` callbacks that do not check the written count. Identity csharp-production.instructions.md +- **Produce a `string` only once, at a storage or wire boundary, with `string.Create`. Pass span inputs through a readonly ref struct state and check the written count against the length. Do not fill a rented char buffer and then call `new string(span)`.** `string.Create` writes in place with no extra copy, and the count check stops a '\0' remainder from being persisted. *Check:* Flag `new string(span)` after filling a rented buffer, and `string.Create` callbacks that do not check the written count. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Read and parse text as `ReadOnlySpan`. Give helpers that only read text a span parameter, and keep `string` on public contract types. Slice instead of calling `Substring`, split with `MemoryExtensions.Split` into a stack `Range` buffer instead of `string.Split`, and compare with span `StartsWith`/`Equals`. For compact JWS/JWE tokens, allocate `segmentCount + 1` ranges, reject any other segment count, and take the signing input as a slice of the original text.** Span parameters accept existing string callers and slices with no allocation. Range splitting allocates no substrings, and the spare range slot catches extra segments. *Check:* Flag `string.Split`, `Substring`, read-only helpers that take a `string`, Range buffers with no spare slot, and JWS signing input built by concatenation. Identity review of #341; Identity review of #420; Identity csharp-production.instructions.md - **Encode strings into a caller-provided span (stackalloc when small and bounded, otherwise rented) with `Encoding.GetBytes(string, Span)`, and declare fixed protocol or crypto labels (HKDF info, purposes) as `ReadOnlySpan` `"label"u8` properties instead of `const string` values encoded on every call.** The array-returning overload allocates on every call, and u8 literals need no encoding or allocation at runtime. *Check:* Flag `Encoding.*.GetBytes(str)` whose `byte[]` result is used only briefly, and `const string` labels passed through `GetBytes` before HKDF, HMAC or hashing. Identity review of #420; Identity review of #341 diff --git a/guidance/async-and-concurrency.md b/guidance/async-and-concurrency.md index 2c7f180..5dd86be 100644 --- a/guidance/async-and-concurrency.md +++ b/guidance/async-and-concurrency.md @@ -14,7 +14,7 @@ applyTo: "**/*.cs, **/*.ts, **/*.tsx, **/charts/**, build/**/*.ps1" - **Present a rotating refresh token at most once across the cluster. Serialize refreshes of a session locally, and admit one refresher across replicas through a lease keyed by the session key plus a SHA-256 fingerprint of the refresh token, not by a record version. A caller that loses the lease polls with backoff for the winner's result and fails with 503 at the lease duration. It never retries with the refresh token it read earlier.** Presenting a rotated refresh token twice counts as reuse, and the provider revokes the whole token family (RFC 9700 §4.14.2). A lease keyed by version is unsafe because unrelated writes change the version. *Check:* Look for refresh logic that has only an in-process lock or no coordination, a lease keyed by version or timestamp, or a timeout or store-error path that calls the token endpoint outside the lease or resends the old refresh token. Identity ADR-0291; Identity docs/backend-for-frontend.md - **Pass `CancellationToken.None` to work that must finish once a change is committed or a point of no return is passed. This covers security effects after a commit (ending sessions or revoking tokens after a password change, disable or block), recording failed authentication attempts, removing the local session record and its cookie on logout (before any remote revocation call), and a non-idempotent remote operation run under a won lease, such as a refresh-token rotation, together with its write-back. Bound that work by its own timeout. Steps that run before the commit keep the request token.** A client that disconnects must not leave a committed change without its security effect, skip a failed-attempt count, leave its session behind, or lose a rotated token so that the next refresh counts as reuse. *Check:* Look for `HttpContext.RequestAborted` or the caller's token passed to post-commit follow-ups, failed-attempt recording, session removal, or token requests and write-backs inside a lease. Also look for pre-commit steps that pass `CancellationToken.None`, and for logout that removes the session only after remote calls. Expect a test that cancels partway through. Identity ADR-0226; Identity ADR-0237; Identity ADR-0291; Identity ADR-0293; Identity docs/backend-for-frontend.md; Identity csharp-production.instructions.md; Identity review of #420 - **Enforce a limit or keep a count that all replicas must agree on with an atomic increment on a shared expiring counter store. Increment before judging the request, and count what was requested rather than what later succeeded. Do not use in-process `System.Threading.RateLimiting` limiters or `AddRateLimiter` for limits that must hold across replicas.** A read followed by a later write loses concurrent increments. An in-process limiter admits N times its limit on N replicas. *Check:* Look for counters that are read, incremented in memory and written back, counters incremented only on success, and `FixedWindowRateLimiter`/`SlidingWindowRateLimiter`/`TokenBucketRateLimiter` used for tenant or client limits in a multi-replica service. Identity ADR-0196; Identity ADR-0198; Identity csharp-production.instructions.md -- **Don't let correctness depend on process-local state. State that must outlive a request or be visible to another replica belongs in the database or a shared-state store. In-memory caches hold only data that can be rebuilt on a miss. Never write durable data to the local filesystem.** Replicas are interchangeable and disposable, and any one of them may serve the next request. *Check:* Look for static or singleton fields, or `IMemoryCache` entries, that hold authoritative state, and for file writes of durable data. Identity csharp-production.instructions.md +- **Don't let correctness depend on process-local state. State that must outlive a request or be visible to another replica belongs in the database or a shared-state store. In-memory caches hold only data that can be rebuilt on a miss. Never write durable data to the local filesystem.** Replicas are interchangeable and disposable, and any one of them may serve the next request. *Check:* Look for static or singleton fields, or `IMemoryCache` entries, that hold authoritative state, and for file writes of durable data. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **In a background loop, treat an `OperationCanceledException` as shutdown only when the loop's own stopping token is cancelled. Catch a failed unit of work with `when (!stoppingToken.IsCancellationRequested)` and handle any other cancellation, such as an `HttpClient` timeout, as an ordinary failure.** A timeout throws `TaskCanceledException` while the stopping token is untouched. A loop that exits on every cancellation ends there, and under `BackgroundServiceExceptionBehavior.StopHost` the whole host stops. *Check:* Look for `catch (OperationCanceledException)`/`catch (TaskCanceledException)` that breaks or returns without checking the stopping token, and for filters such as `exception is not OperationCanceledException`. Identity ADR-0232; Identity csharp-production.instructions.md - **When a timeout is enforced with a `CancellationTokenSource` linked to the caller's token, for example on outbound fetches of untrusted URLs (which must always have a configurable timeout), convert only the timeout's cancellation into a domain or validation error. Let the caller's own cancellation propagate unchanged.** Without a timeout, a slow remote host ties up the server. Converting every cancellation hides real caller cancellation as a fetch failure. *Check:* Look for untrusted outbound fetches with no timeout or with a timeout not linked to the caller's token, and for `catch (OperationCanceledException)` that converts every cancellation without checking whether the caller's token requested it. Identity ADR-0095; Identity ADR-0111 - **Work deferred past the request (background handlers, event and audit sinks) must use a snapshot of the request context (correlation or trace id, request id, tenant, actor, client, source IP) taken when the work is enqueued or the event is published. It must run in a fresh `IServiceScopeFactory` scope and use an application-lifetime token, never the request token.** By the time background work runs, `HttpContext`, the request scope and `Activity.Current` are gone, and the request token is already cancelled. *Check:* Look for background handlers or sinks that read `HttpContext`, `IHttpContextAccessor` or `Activity.Current`, resolve services from the request scope, or receive the request `CancellationToken`. Identity ADR-0047; Identity ADR-0283; Identity docs/audit-records.md @@ -32,7 +32,7 @@ applyTo: "**/*.cs, **/*.ts, **/*.tsx, **/charts/**, build/**/*.ps1" - **When concurrent callers need the same expensive remote resource, share one in-flight retrieval: a `ConcurrentDictionary` of `Lazy>` keyed by the resource, with the entry removed when the retrieval ends. Run the retrieval with `CancellationToken.None`, bounded by its own timeout, and have each caller await it with `WaitAsync(cancellationToken)`. Give it its own copies of disposable inputs (for example, a fresh `X509Certificate2` loaded from the raw data), and remember a failure for a configured time.** Otherwise a burst of requests repeats the download, one caller's cancellation or disposal breaks the retrieval for everyone else, and an unreachable source costs every request a timeout. *Check:* Look for cache-miss paths where each concurrent miss starts its own fetch, shared tasks started with the first caller's token or awaited without `WaitAsync`, shared tasks that capture caller-owned `IDisposable`s, and failing sources retried on every request. Identity ADR-0252; Identity csharp-production.instructions.md - **Publish a lazily created shared client with `Volatile.Read` and `Interlocked.CompareExchange`, and don't cache a failed creation, so the next call tries again.** A transient failure during creation must not break the component permanently. *Check:* Look for `Lazy` or cached `Task` fields that keep a faulted creation, and for unsynchronized lazy initialization of shared clients. Identity ADR-0260 - **In a concurrent cache, publish related data as one atomic entry, or publish the primary item only after the data it depends on is stored.** A concurrent reader can otherwise see the item without its dependents, for example a certificate without its issuer chain, and build and cache a broken result. *Check:* Look for writes to separate dictionaries where the primary entry is added before the data that depends on it. Identity review of #420 -- **Announce a change that other replicas must see through a replica broadcast. Apply the change locally first and publish afterwards. Once the write has committed, log a failed publish rather than throwing.** In-process caches on other replicas go stale otherwise, and a failed publish must not fail a write that has already committed. *Check:* Look for caches invalidated only locally, a publish that happens before the local apply, or a publish failure that is thrown after the commit. Identity csharp-production.instructions.md +- **Announce a change that other replicas must see through a replica broadcast. Apply the change locally first and publish afterwards. Once the write has committed, log a failed publish rather than throwing.** In-process caches on other replicas go stale otherwise, and a failed publish must not fail a write that has already committed. *Check:* Look for caches invalidated only locally, a publish that happens before the local apply, or a publish failure that is thrown after the commit. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Make broadcast and pub/sub handlers idempotent and independent of ordering. Whenever a subscription is first established or restored after a reconnect, assume messages were missed and evict or re-read everything they could have invalidated.** Delivery is best effort, at most once and unordered; the publisher's own replica also receives its message; and messages sent while a replica was disconnected never arrive. *Check:* Look for handlers that assume a single, ordered delivery or do non-idempotent work (increments, appends, one-time effects), and subscribers with no invalidate-all path on subscribe or connection restore. Identity ADR-0205; Identity ADR-0219; Identity csharp-production.instructions.md - **Hand each incoming pub/sub message to its handlers on a thread-pool thread, not inline on the connection's receive callback.** A slow handler must not stop the connection from receiving other messages. *Check:* Look for subscription callbacks that run or await handler work synchronously inside the receive callback. Identity ADR-0205 - **Process a queue of independent jobs with a bounded number of concurrent consumers, for example several loops reading one `Channel` created with `SingleReader = false`. Don't use a single serial loop or `Parallel.ForEachAsync`.** In a single loop, one slow endpoint holds up every job. In `Parallel.ForEachAsync`, one exception cancels every running job. *Check:* Look for dispatch workers with a single reader loop, `Parallel.ForEachAsync` over a channel, or several consumers reading a channel created with `SingleReader = true`. Identity ADR-0232 @@ -42,7 +42,7 @@ applyTo: "**/*.cs, **/*.ts, **/*.tsx, **/charts/**, build/**/*.ps1" - **Keep ambient context that must reach data contexts created by `IDbContextFactory` from the root provider, such as the current tenant, in an `AsyncLocal`, not in a scoped service or an `HttpContext` item.** Factory-created contexts live outside the request's DI scope, so a scoped holder would leave them without a tenant. *Check:* Look for ambient tenant accessors that keep the current value in a scoped-service field or in `HttpContext.Items`. Identity ADR-0018 - **Do the asynchronous step of loading or unwrapping key material, which may call a remote KMS, once through an async acquisition method, and keep the resulting protector's cryptography synchronous. When caching unwrapped keys, don't store a load that an eviction overtook, and don't zero key memory on eviction; zero it only when the owning cache is disposed.** Only loading master keys should reach the KMS. A protector handed out earlier may still be using an evicted key, and caching a load that raced an eviction brings back stale keys. *Check:* Look for async encrypt/decrypt methods or KMS calls on the hot path, sync-over-async KMS calls, eviction code that clears key buffers, and load-then-store code that doesn't check for an eviction during the load. Identity ADR-0235 - **In a React effect that runs an async load, list the loader as a dependency, abort the in-flight load in the cleanup, and ignore any result or error that settles after it is no longer the current load. Tie error state to the loader that produced it, so that replacing the loader clears the error.** Only the current load may report state. A late result, or a StrictMode remount, must not overwrite newer state or leave a stale error visible. *Check:* Look for async effects that leave the loader out of the dependencies, have no `AbortController` or cleanup, or call `setState`/`dispatch` without checking that the load is still current. Also look for error state that persists after the loader changes. Identity ADR-0156 -- **Use `Task.Run` for fire-and-forget offloading, and only when the work really must leave the calling thread. Never use `Task.Factory.StartNew` with an async delegate.** `StartNew` with an async lambda returns an unobserved `Task` and drops the inner task's exceptions. *Check:* Look for `Task.Factory.StartNew` with an async lambda, and for offloading that isn't needed. Identity csharp-production.instructions.md +- **Use `Task.Run` for fire-and-forget offloading, and only when the work really must leave the calling thread. Never use `Task.Factory.StartNew` with an async delegate.** `StartNew` with an async lambda returns an unobserved `Task` and drops the inner task's exceptions. *Check:* Look for `Task.Factory.StartNew` with an async lambda, and for offloading that isn't needed. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Make async methods return `ValueTask`/`ValueTask`, end their names in `Async`, and take a `CancellationToken` as the last parameter. Return `Task` only where a framework contract you don't own requires it (middleware `InvokeAsync`, minimal-API handlers, mediator handlers). Don't add `ConfigureAwait(false)` in server-side code.** This is the house async shape. Server hosts have no `SynchronizationContext`, so `ConfigureAwait(false)` only adds noise. *Check:* Look for new async methods returning `Task` outside framework boundaries, missing `Async` suffixes, a `CancellationToken` that isn't the last parameter, and added `.ConfigureAwait(false)`. Identity csharp-production.instructions.md; Identity ADR-0285 - **In Helm charts for replicas that keep single-use state in-process with no shared store, use the `Recreate` deployment strategy and refuse more than one replica. Use `RollingUpdate` with `maxSurge: 1` and `maxUnavailable: 0` only when that state is shared.** A rolling update, even with `maxSurge: 0`, can start the new pod while the old one is still serving, so their separate single-use state overlaps. *Check:* Look for `RollingUpdate` (including `maxSurge: 0`), or allowing `replicas > 1`, when no shared state store is configured. Identity ADR-0179 - **When Aspire runs JavaScript resources from a shared pnpm workspace, call `WithPnpm(install: false)` and install the workspace once, serially, from a single bootstrap script. Never add a per-app `pnpm install`.** Concurrent `pnpm install` runs against the shared, symlinked `node_modules` collide with `ERR_PNPM_EBUSY` and fail the dependent resource. *Check:* Look for `WithPnpm()` without `install: false`, and for per-app `pnpm install` steps in orchestration or repair scripts. Identity ADR-0071 diff --git a/guidance/code-organization.md b/guidance/code-organization.md index a680327..ef2e234 100644 --- a/guidance/code-organization.md +++ b/guidance/code-organization.md @@ -76,21 +76,21 @@ applyTo: "**/*.cs, **/*.csproj, **/*.props, **/version.json, **/.editorconfig, . - **Name the single canonical DI implementation of `IFoo` `DefaultFoo`. Do not use `Default*` for one of several variants selected at runtime; name each variant for what makes it specific. Name implementations of framework interfaces by the framework's own pattern (for example `Validator` for `IValidateOptions`).** `Default*` signals the one replaceable default, following the ASP.NET Core idiom, and leaves the bare concept name free for consumers. Using it on variants or adapters misleads readers. *Check:* Look for sole implementations named `FooImpl`, `FooService`, `ConcreteFoo` or the bare name, for `Default*` used among runtime-selected variants, or for `Default*` used on framework adapters. Identity ADR-0006 - **Declare a library's public registration marker (`IRegistrationMarker<>`) in its `.Abstractions` package, and make sub-scope markers `internal` unless outside consumers need them. Name each folder's registration class `DefaultRegistration` (one per folder), and put a second extension receiver shape in a sibling `…Registration` class.** Dependent packages can then name `IServiceBuilder` without referencing the implementation, registration naming stays uniform, and one static class holds only one `extension(...)` receiver block without CA1708. *Check:* Look for public markers in non-Abstractions projects, unused public sub-markers, registration classes named after a feature, several `DefaultRegistration` classes in one folder, or two receiver blocks in one class. Identity ADR-0069 - **Make a package's root namespace match its package id, and give internal types in a subfolder the namespace of that folder. Put generic, domain-agnostic infrastructure in a cross-cutting package whose name and namespace match its purpose, and name vendor adapter packages `.`.** Namespaces then show which package owns a type, which prevents accidental cross-package placement, and generic plumbing can be reused. *Check:* Look for types whose namespace does not start with the package id, internal subfolder types in the root namespace, generic plumbing in a domain package or with domain names (for example `MapIdentityEndpoints`), or adapters named after the wrong area. Identity ADR-0052; Identity ADR-0043; Identity ADR-0279; Identity csharp-production.instructions.md -- **Organize folders by purpose. Do not create catch-all folders or types (Logic, Helpers, Utils, Misc, Common, `*Manager`, `*Helper`). Keep a contract, its DTOs and its `Default*` implementation in one purpose namespace across the abstractions/implementation pair, keep headline public entry points at the package root, keep a single-purpose package flat, and place a service used by only one feature next to that feature.** Catch-all buckets grow without cohesion, and readers cannot find related types by purpose. *Check:* Look for new catch-all folders, namespaces or type names, unrelated types added to such folders, contracts split from their implementations, or headline types buried in subfolders. Identity csharp-production.instructions.md +- **Organize folders by purpose. Do not create catch-all folders or types (Logic, Helpers, Utils, Misc, Common, `*Manager`, `*Helper`). Keep a contract, its DTOs and its `Default*` implementation in one purpose namespace across the abstractions/implementation pair, keep headline public entry points at the package root, keep a single-purpose package flat, and place a service used by only one feature next to that feature.** Catch-all buckets grow without cohesion, and readers cannot find related types by purpose. *Check:* Look for new catch-all folders, namespaces or type names, unrelated types added to such folders, contracts split from their implementations, or headline types buried in subfolders. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Put general-purpose helpers in their dependency-free `NCode.*` home: `JsonElement` helpers in `NCode.Json`'s `JsonElements`, and time helpers in `NCode.Time`. Do not add local copies in feature packages.** A single dependency-free home lets every package use the helper without keeping copies. *Check:* Look for local `JsonElementExtensions` or DateTime/DateTimeOffset/TimeProvider helpers added outside those packages. Identity ADR-0056; Identity ADR-0188 - **Leave out a noun in a type name when the enclosing namespace already supplies it. Keep it only on types referenced across slices, where the short name would be ambiguous or collide.** The repeated noun is redundant and collides with shared types. *Check:* Look for types in a feature namespace that repeat its noun and shadow a shared type with the same meaning. Identity ADR-0030 - **Parse JSON with `JsonElement.Parse` and assign the result directly, instead of `JsonDocument.Parse(...).RootElement.Clone()`. Name the element for what it holds, or `jsonElement`, never `document`.** A detached element needs no disposal or clone, and `document` wrongly suggests a `JsonDocument`. *Check:* Flag `JsonDocument.Parse` followed by `.RootElement.Clone()`, needless intermediate locals, or `JsonElement` variables named `document` or `doc`. Identity review of #420; Identity csharp-production.instructions.md -- **Name `IServiceCollection` parameters, locals and receivers `serviceCollection`, and builder properties `ServiceCollection`, never `services`, including in tests.** This keeps names consistent with the `NCode.Registration` builder shape. *Check:* Look for an `IServiceCollection` named `services` or `service`. Identity csharp-production.instructions.md +- **Name `IServiceCollection` parameters, locals and receivers `serviceCollection`, and builder properties `ServiceCollection`, never `services`, including in tests.** This keeps names consistent with the `NCode.Registration` builder shape. *Check:* Look for an `IServiceCollection` named `services` or `service`. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Compare integer HTTP status codes with `StatusCodes.StatusXxx` constants, not `(int)HttpStatusCode.X` casts or numeric literals.** The framework already provides int constants for every status code. *Check:* Look for `(int)HttpStatusCode.` or bare numbers like `200` compared against status codes. Identity review of #320 - **Use string literals of the `Name` values as dictionary or switch keys for `HashAlgorithmName` lookups, not `HashAlgorithmName.X.Name`.** `Name` is `string?`, so using it as a key brings in nullability problems. *Check:* Look for keys written as `HashAlgorithmName.SHA256.Name` or similar. Identity ADR-0281 - **Do not leave TODO, FIXME or HACK comments, `NotImplementedException` stubs or placeholder logic in a finished change. Finish the work or file a follow-up.** Leftover placeholders mean the step was done with shortcuts instead of as written. *Check:* Search added lines for TODO, FIXME, HACK, `NotImplementedException` or placeholder logic that stands in for required behavior. Identity .claude/commands/review-plan-diff.md; Identity .github/agents/copilot-reviewer.agent.md - **Replace magic numbers and literal strings in logic with named constants or bound configuration options.** Unexplained values make code fragile and hard to understand. *Check:* Look for unexplained literals in conditionals, timeouts, sizes or limits. NCodeGroup maintainer: principal-engineer-review -- **Model a concept that carries invariants as its own type (for example a readonly record struct for small immutable ids, keys, commands and dispositions) rather than a bare `string`, `bool` or `int`.** Primitive obsession spreads validation around and lets invalid values through, while record structs give value equality and immutability for free. *Check:* Look for bare primitives with implied invariants passed through many APIs, or small value-like types declared as classes or mutable structs. Identity csharp-production.instructions.md -- **Keep functions small, doing one thing at one level of abstraction, with at most 2–3 data parameters (a trailing `CancellationToken` or Try-pattern `out` does not count). Turn recurring parameter groups into a type, and follow Command–Query Separation: a query never changes state.** Small, focused functions are easier to read and test, and mixing commands with queries hides side effects. *Check:* Look for long multi-purpose methods, more than 3 data parameters, repeated parameter groups, or query/getter methods that change state. Identity csharp-production.instructions.md -- **Talk only to immediate collaborators, and do not nest a call inside another call's arguments. Assign the inner result to a local with a name that says what it is.** Long member chains couple code to distant structure, and named steps are easier to read and debug. *Check:* Look for `a.B().C().D()` chains that reach through several objects, or calls like `Process(GetValue(), ...)`. Identity csharp-production.instructions.md -- **Prefer auto-properties over fields (a field only for ref, Interlocked, fixed or stackalloc targets; a `Lock` as a get-only auto-property). Give each property the narrowest accessors (`{ get; }`, `{ get; init; }`, `{ get; set; }` only when mutation is needed), and use primary constructors when a constructor only assigns parameters, moving the `` docs onto the type.** This is the house style. It limits mutable state and removes boilerplate. *Check:* Look for new private fields that could be auto-properties, public setters on state never changed after construction, or constructors that only assign parameters. Identity csharp-production.instructions.md -- **Write extension methods as C# 14 `extension(ReceiverType r) { … }` blocks, not classic `this`-parameter static methods.** It keeps extensions uniform with the composition pattern. *Check:* Look for new `static ... Foo(this T x, ...)` extension methods. Identity csharp-production.instructions.md -- **Do not declare nested types, except a private nested type that must close over the outer type's generic parameter, or a nested static class of const fields grouping a related constant vocabulary. Put constants and static members before instance members.** Top-level types in their own files are easy to find, and a consistent member order keeps files predictable. *Check:* Look for other nested class, struct, record, interface or enum declarations, or static/const members declared after instance members. Identity csharp-production.instructions.md +- **Model a concept that carries invariants as its own type (for example a readonly record struct for small immutable ids, keys, commands and dispositions) rather than a bare `string`, `bool` or `int`.** Primitive obsession spreads validation around and lets invalid values through, while record structs give value equality and immutability for free. *Check:* Look for bare primitives with implied invariants passed through many APIs, or small value-like types declared as classes or mutable structs. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Keep functions small, doing one thing at one level of abstraction, with at most 2–3 data parameters (a trailing `CancellationToken` or Try-pattern `out` does not count). Turn recurring parameter groups into a type, and follow Command–Query Separation: a query never changes state.** Small, focused functions are easier to read and test, and mixing commands with queries hides side effects. *Check:* Look for long multi-purpose methods, more than 3 data parameters, repeated parameter groups, or query/getter methods that change state. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Talk only to immediate collaborators, and do not nest a call inside another call's arguments. Assign the inner result to a local with a name that says what it is.** Long member chains couple code to distant structure, and named steps are easier to read and debug. *Check:* Look for `a.B().C().D()` chains that reach through several objects, or calls like `Process(GetValue(), ...)`. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Prefer auto-properties over fields (a field only for ref, Interlocked, fixed or stackalloc targets; a `Lock` as a get-only auto-property). Give each property the narrowest accessors (`{ get; }`, `{ get; init; }`, `{ get; set; }` only when mutation is needed), and use primary constructors when a constructor only assigns parameters, moving the `` docs onto the type.** This is the house style. It limits mutable state and removes boilerplate. *Check:* Look for new private fields that could be auto-properties, public setters on state never changed after construction, or constructors that only assign parameters. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Write extension methods as C# 14 `extension(ReceiverType r) { … }` blocks, not classic `this`-parameter static methods.** It keeps extensions uniform with the composition pattern. *Check:* Look for new `static ... Foo(this T x, ...)` extension methods. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Do not declare nested types, except a private nested type that must close over the outer type's generic parameter, or a nested static class of const fields grouping a related constant vocabulary. Put constants and static members before instance members.** Top-level types in their own files are easy to find, and a consistent member order keeps files predictable. *Check:* Look for other nested class, struct, record, interface or enum declarations, or static/const members declared after instance members. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Keep presentational UI components and the presentational package stateless and driven by props: no Redux store access, RTK Query hooks, fetches or side-effecting effects (local state only when the control needs it). Keep state and config packages free of UI imports, and put stateful containers in the shell package or the app.** Presentational components must render in Storybook and React Testing Library without a store, and a UI dependency would pull a UI framework into a UI-free layer. *Check:* Look for `useSelector`/`useDispatch`, RTK Query hooks, store providers or fetches in presentational components, or React/MUI imports in store or config packages. Identity ADR-0064; Identity ADR-0156 - **Put state and side effects in a container at the top of each screen that wires Redux and RTK Query and passes props down. Render its loading and error views through stateless presentational components that have stories. Move a container into the shared shell package once a second app needs it, and keep a container used by one app in that app.** State wiring in one place keeps each layer testable, and copies of a container drift apart. *Check:* Look for store wiring spread through nested components, loading or error markup built inline in containers, new screens without stories, or a container duplicated between apps. Identity ADR-0064; Identity ADR-0156 - **Keep protocol orchestration and security-audit logic (OAuth flow steps, token handling, metadata audits) in UI-free modules with their own unit tests, not inside React components.** Logic outside the UI can be unit-tested and built from shared primitives. *Check:* Look for flow, token or audit logic inside components or views with no matching UI-free module and tests. Identity ADR-0082 diff --git a/guidance/dependency-injection.md b/guidance/dependency-injection.md index 2ddd12a..97ecc01 100644 --- a/guidance/dependency-injection.md +++ b/guidance/dependency-injection.md @@ -9,7 +9,7 @@ applyTo: "**/*.cs, **/*.csproj" - **Do not cache resolved handler instances in a singleton, and do not resolve scoped services from the root provider. Resolve them from the current scope at the point of use, on every dispatch. Caching reflection results such as closed generic service types is fine.** A cached instance or a root-provider resolution turns scoped dependencies such as a DbContext or tenant persistence into captive dependencies. They then get shared across requests and tenants. *Check:* Look for singletons that keep resolved handlers or services in fields, dictionaries or lazies, and for `GetService`/`GetServices` calls on a root `IServiceProvider` that return scoped services. Identity ADR-0047; Identity csharp-production.instructions.md - **Run each unit of background work (a job, work item, retry attempt or tenant pass) in its own scope. Create it with `await using var scope = scopeFactory.CreateAsyncScope()` and resolve every handler for that unit from that scope. Dispose any context you create before you dispose the scope. In a loop over tenants, use one scope per tenant, and catch and log each tenant's failure so the remaining tenants still run.** Background work has no request scope. Sharing one scope across items or tenants, or resolving from the root, leaks scoped state across work items and tenants. One tenant's exception must not abort the work for the others. *Check:* Look for hosted or background services that inject scoped services directly, use one `IServiceScope` for many items or tenants, call the synchronous `CreateScope()` in async code, skip `await using`, dispose the scope before its context, or let an exception from one tenant escape the loop. Identity ADR-0090; Identity ADR-0223; Identity ADR-0232; Identity csharp-production.instructions.md -- **Never store a captured `HttpContext`, a request-scoped `IServiceProvider` or a `ClaimsPrincipal` in a field or property. Use a live view over `IHttpContextAccessor`, or pass the value as a method parameter.** Captured request state goes stale and causes use-after-request bugs. *Check:* Look for fields or properties of type `HttpContext`, `IServiceProvider` (scoped) or `ClaimsPrincipal` on long-lived or reusable types. Identity csharp-production.instructions.md +- **Never store a captured `HttpContext`, a request-scoped `IServiceProvider` or a `ClaimsPrincipal` in a field or property. Use a live view over `IHttpContextAccessor`, or pass the value as a method parameter.** Captured request state goes stale and causes use-after-request bugs. *Check:* Look for fields or properties of type `HttpContext`, `IServiceProvider` (scoped) or `ClaimsPrincipal` on long-lived or reusable types. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Give `IValidateOptions` implementations no constructor dependencies, and register each one next to the services that read its options. Never resolve services that have their own dependency graphs inside options validation or in any catalog or data source a validator reads. Feed validators from dependency-free descriptors registered beside each strategy.** If constructing a validator's dependencies reads `IOptions.Value`, host composition deadlocks. The container cannot detect the cycle because its last edge is a runtime `GetServices` call. *Check:* Look for validator constructors that take parameters, especially `IOptions`/`IOptionsMonitor`, `IServiceProvider` or `IEnumerable`, and for catalogs used by validators that resolve handlers just to read a property such as a name. Identity ADR-0207; Identity ADR-0221; Identity csharp-production.instructions.md - **Do not rely on generic variance when resolving from Microsoft.Extensions.DependencyInjection. To reach handlers registered for base types, explicitly close the open generic over each type in the hierarchy.** MS.DI ignores variance, so `GetServices>()` does not return handlers registered as `IHandler`. *Check:* Look for code that expects resolving a derived closed generic to also return handlers registered for a base or interface type argument. Identity ADR-0047 - **Make a DI decorator extension work in either registration order. Replace the last non-keyed descriptor in place with one of the same lifetime that wraps its instance, its factory or its implementation type. If there is no descriptor yet, register the decorator around the default implementation so the framework's later `TryAdd` leaves it alone. Never wrap an inner service that is already an instance of the same decorator.** The host may register its implementation before or after the library call. Wrapping twice applies the decorator's transformation, such as normalization, twice. *Check:* Look for decorators that only append a descriptor, change the lifetime, handle only implementation-type descriptors, break when a `TryAdd` runs after them, or wrap without an `inner is TDecorator` guard. Identity ADR-0243 @@ -41,7 +41,7 @@ applyTo: "**/*.cs, **/*.csproj" - **Constructor-inject every collaborator. Do not `new` services and do not use a service locator, except in a singleton that resolves scoped or generically selected services at the point of use. Resolve pluggable mechanisms such as chain validators and revocation checkers from DI rather than constructing them in handlers.** Injection keeps code testable and lets deployments replace mechanisms without editing handlers or extending policy enums. *Check:* Look for `new` of service collaborators inside methods, `GetService` calls where injection was possible, and new policy enum values added only to support a different mechanism. Identity ADR-0173; Identity csharp-production.instructions.md - **Expose a policy that replaceable public services reuse as a public interface in the abstractions package, keep its default implementation internal, and register that default with `TryAdd`.** A public concrete class fixes the policy's dependencies into the public API. An internal-only policy forces every replacement to reimplement it. *Check:* Look for replaceable policies exposed as public concrete classes, or replacements that would need internal types to behave like the default. Identity ADR-0181 - **Declare delegates the host can replace on a shared environment abstraction in `*.Abstractions`, wired from the core options delegate. Consumers call the hook through that abstraction, not by injecting the core `IOptions`.** Packages that depend only on abstractions cannot inject the core options, so they end up writing their own versions of the hook, and the versions drift apart. *Check:* Look for non-core packages that inject a core `IOptions<...Options>` just to call a behavioral delegate, and new delegates reachable only through the options type. Identity ADR-0053 -- **Register stateless capabilities as singletons behind interfaces instead of static utility classes, keep no static mutable or ambient state, and host long-running loops with `AddHostedService` over a `BackgroundService`, with registration kept outside the worker type.** A singleton costs about the same as a static and stays mockable, while static state hurts testability. *Check:* Look for new static classes with logic that has dependencies, mutable static fields, and worker types that register themselves or resolve their own services. Identity csharp-production.instructions.md +- **Register stateless capabilities as singletons behind interfaces instead of static utility classes, keep no static mutable or ambient state, and host long-running loops with `AddHostedService` over a `BackgroundService`, with registration kept outside the worker type.** A singleton costs about the same as a static and stays mockable, while static state hurts testability. *Check:* Look for new static classes with logic that has dependencies, mutable static fields, and worker types that register themselves or resolve their own services. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Make an endpoint depend on as few services as possible, ideally only client authentication, and reach the remaining steps through the mediator.** A thin endpoint holds no protocol logic, and its steps stay replaceable. *Check:* Look for endpoint providers whose constructors inject stores, issuers or other protocol services. Identity ADR-0086 - **Expose a lazily built service to handlers and hosted services through a singleton registry or accessor that has no dependencies and waits until the service exists. Do not have them depend on the services that build it.** Consumers then reach the service without pulling in its construction graph or creating cycles. *Check:* Look for hosted services or handlers that inject a provider or factory only to get an already-built instance, or that build it themselves. Identity ADR-0219 -- **In tests that compose services needing an unrelated dependency, register a strict mock that is never called (`new Mock(MockBehavior.Strict).Object`). Do not disable DI validation or wire up a real backend.** DI validation stays on while the configuration under test stays isolated. *Check:* Look for tests that set `ValidateOnBuild` or `ValidateScopes` to false, or that wire a real database, only to satisfy composition. Identity csharp-testing.instructions.md +- **In tests that compose services needing an unrelated dependency, register a strict mock that is never called (`new Mock(MockBehavior.Strict).Object`). Do not disable DI validation or wire up a real backend.** DI validation stays on while the configuration under test stays isolated. *Check:* Look for tests that set `ValidateOnBuild` or `ValidateScopes` to false, or that wire a real database, only to satisfy composition. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md diff --git a/guidance/documentation.md b/guidance/documentation.md index 77a02fd..1ea5220 100644 --- a/guidance/documentation.md +++ b/guidance/documentation.md @@ -10,10 +10,10 @@ applyTo: "**/*.cs, **/*.md, **/*.csproj, **/*.ts, **/*.tsx, **/*.js, **/*.tf, ** - **When a change renames, re-signatures or changes the behavior or design of a type, member or contract, update every place that refers to it in the same change: callers, overloads, tests, XML docs, code comments, README and other Markdown docs, and decision records.** References that fall out of sync leave contradictions between files. Docs and comments that still describe an old or insecure design mislead the next reader, and the fix has to live in the repository, not only in chat history. *Check:* For each renamed, re-signatured or behavior-changed item in the diff, search for callers, tests, `///` and `//` comments, Markdown and ADRs that still use the old name or describe the old behavior. NCodeGroup maintainer: gap-hunting-review; Identity repository.instructions.md - **When a change adds or renames a field, field-name constant or event type in a documented record or wire contract, such as an audit record or event catalog, update the contract document or shipped catalog in the same change, and name new event actions according to the documented taxonomy.** The contract document is the reviewed contract that log pipelines, sinks, dashboards, alerts and runbooks are built against, so the event set has to change on purpose and must not drift from the code. *Check:* Look for new or renamed record fields, field-name constants, event types or action names with no matching edit to the contract doc or catalog, and for action names that don't follow the documented naming scheme. *Note:* In NCode Identity the event-action taxonomy is dotted, for example `token.issued` or `auth.failed`. Identity docs/audit-records.md; Identity ADR-0047 - **When a change adds or renames a meter, instrument, span, attribute or attribute value, update the package's telemetry/metrics catalog documentation in the same change. A new span should name or add the histogram it mirrors, and the catalog should document both using the same vocabulary.** The catalog is the reviewed contract that dashboards and alerts are built against, and it describes traces and metrics with one vocabulary. *Check:* Look for diffs that add or change `Meter`/`Create*` calls, `ActivitySource` span names, or metric tag names or values without a matching edit to the metrics catalog doc. Identity docs/metrics.md; Identity csharp-production.instructions.md; Identity ADR-0298 -- **Declare every non-200 response of a minimal-API endpoint with `.Produces(StatusCodes.StatusNNN)` or `.Produces()`. Use `` XML docs only to describe a response that is already declared.** A `` XML tag does not add a response to the OpenAPI document, so a response documented only that way never appears in it. *Check:* Look for `` or other non-200 response tags that have no matching `.Produces` call on the endpoint. Identity csharp-production.instructions.md -- **For a minimal-API handler's XML docs to reach its OpenAPI operation, the defining project must reference Microsoft.AspNetCore.OpenApi and call `AddOpenApi`, the handler must be `internal` or more visible, and the endpoint must not also use `.WithSummary()` or `.WithDescription()`. Use either fluent metadata or XML docs for an operation, never both.** The XML-comment source generator silently ignores private handlers and skips operations that already carry fluent metadata. *Check:* Look for private handlers with `///` docs, endpoints that combine `.WithSummary`/`.WithDescription` with XML docs, and handler projects that have docs but no OpenApi package reference or `AddOpenApi` call. Identity csharp-production.instructions.md +- **Declare every non-200 response of a minimal-API endpoint with `.Produces(StatusCodes.StatusNNN)` or `.Produces()`. Use `` XML docs only to describe a response that is already declared.** A `` XML tag does not add a response to the OpenAPI document, so a response documented only that way never appears in it. *Check:* Look for `` or other non-200 response tags that have no matching `.Produces` call on the endpoint. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **For a minimal-API handler's XML docs to reach its OpenAPI operation, the defining project must reference Microsoft.AspNetCore.OpenApi and call `AddOpenApi`, the handler must be `internal` or more visible, and the endpoint must not also use `.WithSummary()` or `.WithDescription()`. Use either fluent metadata or XML docs for an operation, never both.** The XML-comment source generator silently ignores private handlers and skips operations that already carry fluent metadata. *Check:* Look for private handlers with `///` docs, endpoints that combine `.WithSummary`/`.WithDescription` with XML docs, and handler projects that have docs but no OpenApi package reference or `AddOpenApi` call. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Declare the bearer security scheme in the OpenAPI document, plus the OAuth2 flow when one applies, and reference it from every authenticated operation.** API reference renderers such as Scalar need the scheme to show an authentication control and attach the token to requests. *Check:* Look for new authenticated endpoints, or OpenAPI configuration, that do not declare or reference a security scheme. Identity ADR-0044 -- **Keep internal references out of `///` doc comments: no ADR numbers, no relative links to repository docs, no persistence column names, no internal implementation type names. Put any ADR reference in a `//` comment instead.** `///` text ships to consumers through IntelliSense XML and OpenAPI schema descriptions, where internal references mean nothing or are broken links. *Check:* Look for `///` comments containing 'ADR-', `Identity csharp-production.instructions.md - **Add a CHANGELOG.md entry under `## [Unreleased]`, in the same pull request, for every consumer-visible change to public API, behavior or configuration.** Consumers rely on the changelog to learn what changed, and that knowledge has to live in versioned repository content. *Check:* Look for public API, behavior or configuration changes with no matching CHANGELOG.md update in the diff, and for entries added outside `## [Unreleased]`. Identity csharp-production.instructions.md; Identity repository.instructions.md - **Add each CHANGELOG entry exactly once, in the correct section, and make sure scripted or automated insertions did not copy it under every heading with the same name.** Inserting before a heading text that appears many times has copied the same entries into every `### Changed` section. *Check:* Look in CHANGELOG diffs for the same entry text repeated across several sections or releases. Identity review of #420 - **Track deferred work and follow-ups as GitHub issues. In code comments, refer to the issue as `issue #123`. In Markdown, use a full link of the form `[#123](https://github.com///issues/123)`. Never use hand-allocated IDs or untracked TODO, FIXME or 'remaining' notes.** GitHub issues are the single source of truth for pending work. A reference by issue number stays valid and connects the item to the pull request that completes it. *Check:* Look for new TODO, FIXME, 'deferred', 'follow-up' or 'remaining' comments or bullets with no issue reference, references that use a home-grown ID scheme, and Markdown that gives a bare `#N` without an issue link. Identity ADR-0264; Identity repository.instructions.md @@ -23,12 +23,12 @@ applyTo: "**/*.cs, **/*.md, **/*.csproj, **/*.ts, **/*.tsx, **/*.js, **/*.tf, ** - **When a project turns off a source generator or analyzer as a workaround, for example `EnableRequestDelegateGenerator=false`, add a comment next to the setting that explains why and names the guard test and the decision record or tracking issue for turning it back on.** The opt-out is meant to be temporary, for example until the Request Delegate Generator honors antiforgery validation, so the setting must point to the tests that make re-enabling safe and to the issue that tracks it. *Check:* Look for `EnableRequestDelegateGenerator=false`, or a similar opt-out in a .csproj, with no nearby comment naming the guard test and the ADR or issue. Identity ADR-0262 - **When a change alters infrastructure, required manual settings, token permissions or steps that an operational runbook describes, update the runbook in the same change.** The runbook must stay accurate for anyone rebuilding the platform from scratch, so it has to change together with what it describes. *Check:* Look for diffs to bootstrap infrastructure code, workflow permissions or required settings with no matching edit to the runbook that documents them. Identity docs/platform-bootstrap.md - **When a change introduces or relies on a new convention, a non-obvious design decision or a recurring tooling gotcha, record it in the repository's versioned docs in the same change: scoped instruction files for conventions, an ADR for the reasoning, the AGENTS/CONTRIBUTING guide for build or orientation knowledge. Chat, a wiki, personal notes or agent memory are not enough.** Knowledge kept outside the repository isn't versioned or reviewed, so it drifts from the code and has to be learned again and again. *Check:* Look for diffs that set up a new pattern or work around a quirk in a non-obvious way without updating instruction files, ADRs or AGENTS/CONTRIBUTING docs, and for PR text that cites a rule that exists only in chat, a wiki or memory. Identity ADR-0008 -- **Give every type or member you add, or whose signature or behavior you change, a `///` XML doc comment. Do not add doc comments to undocumented code the change doesn't otherwise touch.** New and changed API must be documented. Existing undocumented code is grandfathered so that diffs stay focused. *Check:* Look for new or modified public/internal types or members without `///` docs, and for diffs that add docs to members they don't otherwise touch. Identity csharp-production.instructions.md -- **Mark a type that is public only for cross-assembly infrastructure with `[EditorBrowsable(EditorBrowsableState.Never)]`, and open its summary with the standard 'supports the infrastructure and is not intended to be used directly from your code' boilerplate.** This follows the EF Core and ASP.NET Core pattern and keeps infrastructure types out of what application code sees. *Check:* Look for public types used only by infrastructure that lack the attribute or the boilerplate summary. Identity csharp-production.instructions.md -- **Write `///` and `//` comments and ADR prose in the timeless present. Avoid change-relative words such as 'now', 'new', 'previously', 'no longer', 'renamed' or 'moved', and never name a removed or renamed identifier.** History belongs in Git and the CHANGELOG. The text must read correctly to someone who never saw the earlier version. *Check:* Look for comments with temporal or change-relative wording, and for comments that refer to a deleted or superseded type or member by its old name. Identity csharp-production.instructions.md -- **Write a substantive `` that explains purpose, behavior and context, and use ``, `` and `` where they add clarity.** A one-line summary that only repeats the member name gives consumers no useful context. *Check:* Look for summaries that only restate the member name, and for missing param or returns docs where they would help. Identity csharp-production.instructions.md -- **Start a property summary with 'Gets or sets', 'Gets' or 'Sets' to match its accessors, and use 'Gets' for an `init`-only property.** This is the Microsoft convention, and an init-only property is read-only after construction. *Check:* Look for `{ get; init; }` or get-only properties documented as 'Gets or sets', and for property summaries that don't use these openings. Identity csharp-production.instructions.md -- **Write comments that explain why, not comments that restate what the code does, and do not leave commented-out code.** Comments that restate the code add noise, and Git already keeps the history of removed code. *Check:* Look for comments that paraphrase the next line of code, and for blocks of commented-out code. Identity csharp-production.instructions.md -- **In comment prose, spell protocol terms the way their specifications do, for example 'JSON Web Token (JWT)', JSON, JOSE, OAuth and OpenID, not in the Pascal casing of a .NET type name. cref targets and identifiers keep their real casing.** Documentation should use the specifications' standard terminology. *Check:* Look for `///` or `//` prose that writes 'JsonWebToken', 'Json', 'Jose', 'Oauth' or 'OpenId' as a plain word. Identity csharp-production.instructions.md +- **Give every type or member you add, or whose signature or behavior you change, a `///` XML doc comment. Do not add doc comments to undocumented code the change doesn't otherwise touch.** New and changed API must be documented. Existing undocumented code is grandfathered so that diffs stay focused. *Check:* Look for new or modified public/internal types or members without `///` docs, and for diffs that add docs to members they don't otherwise touch. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Mark a type that is public only for cross-assembly infrastructure with `[EditorBrowsable(EditorBrowsableState.Never)]`, and open its summary with the standard 'supports the infrastructure and is not intended to be used directly from your code' boilerplate.** This follows the EF Core and ASP.NET Core pattern and keeps infrastructure types out of what application code sees. *Check:* Look for public types used only by infrastructure that lack the attribute or the boilerplate summary. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Write `///` and `//` comments and ADR prose in the timeless present. Avoid change-relative words such as 'now', 'new', 'previously', 'no longer', 'renamed' or 'moved', and never name a removed or renamed identifier.** History belongs in Git and the CHANGELOG. The text must read correctly to someone who never saw the earlier version. *Check:* Look for comments with temporal or change-relative wording, and for comments that refer to a deleted or superseded type or member by its old name. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Write a substantive `` that explains purpose, behavior and context, and use ``, `` and `` where they add clarity.** A one-line summary that only repeats the member name gives consumers no useful context. *Check:* Look for summaries that only restate the member name, and for missing param or returns docs where they would help. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Start a property summary with 'Gets or sets', 'Gets' or 'Sets' to match its accessors, and use 'Gets' for an `init`-only property.** This is the Microsoft convention, and an init-only property is read-only after construction. *Check:* Look for `{ get; init; }` or get-only properties documented as 'Gets or sets', and for property summaries that don't use these openings. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **Write comments that explain why, not comments that restate what the code does, and do not leave commented-out code.** Comments that restate the code add noise, and Git already keeps the history of removed code. *Check:* Look for comments that paraphrase the next line of code, and for blocks of commented-out code. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **In comment prose, spell protocol terms the way their specifications do, for example 'JSON Web Token (JWT)', JSON, JOSE, OAuth and OpenID, not in the Pascal casing of a .NET type name. cref targets and identifiers keep their real casing.** Documentation should use the specifications' standard terminology. *Check:* Look for `///` or `//` prose that writes 'JsonWebToken', 'Json', 'Jose', 'Oauth' or 'OpenId' as a plain word. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **To have an API reference renderer pre-check and pre-fill an `application/x-www-form-urlencoded` request field, mark the field `required` and set its schema `Default` in the OpenAPI document. Do not rely on named examples being applied to form fields.** Renderers such as Scalar build form bodies from schema properties. Optional properties render unchecked and are left out of the request, and named examples aren't applied, so the form ends up empty. *Check:* Look for OpenAPI transformers that try to pre-fill form-urlencoded fields only through named examples, or that leave those fields optional. Identity ADR-0046 -- **Put the `` opening tag, its content and its closing tag on separate lines, even when the summary is short.** This keeps doc comment formatting consistent. *Check:* Look for a single-line `/// ...`. Identity csharp-production.instructions.md +- **Put the `` opening tag, its content and its closing tag on separate lines, even when the summary is short.** This keeps doc comment formatting consistent. *Check:* Look for a single-line `/// ...`. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md diff --git a/guidance/errors-and-validation.md b/guidance/errors-and-validation.md index 4ef3e99..5106dba 100644 --- a/guidance/errors-and-validation.md +++ b/guidance/errors-and-validation.md @@ -14,7 +14,7 @@ applyTo: "**/*.cs, build/**, **/*.ps1, **/*.sh, **/charts/**" - **Treat a malformed security-relevant header as a protocol error, never as an unhandled exception or as a missing value. An undecodable client credential is `invalid_client`. A forwarded client-certificate header that is present but cannot be decoded is `invalid_request` (400), with a warning that names the header and the format. Decode such values all-or-nothing: reject the whole value if it has text around PEM blocks, empty entries, non-CERTIFICATE blocks or repeated keys. Do not hand callers a tri-state Decoded/Absent/Malformed result.** Reading malformed input as 'absent' hides proxy misconfiguration and can silently issue unbound tokens. Lenient partial parsing can accept smuggled values. *Check:* Look for credential or certificate-header parsing that throws, asserts, returns null, or falls through to the no-certificate path when decoding fails. Look for decoders that skip bad entries or let a repeated key overwrite an earlier one, and for status enums passed to consumers. Identity ADR-0092; Identity ADR-0172; Identity docs/hosting-analysis.md - **Decode untrusted text with a strict encoding and never substitute or strip invalid characters. Catch `DecoderFallbackException` and rethrow it as the exception type the API documents, or on inbound paths map it to the protocol's error response. Check values that must be ASCII, such as tokens, explicitly (for example with `Ascii.IsValid`).** Substituted characters turn malformed input into different, valid-looking input. An uncaught DecoderFallbackException escapes the documented contract and surfaces as a 500. *Check:* Look for strict `GetString` calls on request or response bodies without DecoderFallbackException handling, catch blocks that continue with replacement characters, and ASCII-only values that are never validated. Identity ADR-0266; Identity review of #420 - **Before reading members of JSON parsed from wire input, require `ValueKind == JsonValueKind.Object`. `TryGet…` helpers on `JsonElement` must check `ValueKind` first and return false for any unacceptable value; they must never throw. Refuse a whole-number value that has a non-zero fraction, is out of range, or arrives as a string. Do not round, truncate or parse it.** `TryGetProperty` throws on a non-object, so hostile input can fault the caller. Rounding or parsing a numeric string invents a value the remote party never sent. *Check:* Look for property reads on parsed bodies, token headers or payloads with no object-kind check; `TryGet*` helpers that call throwing `Get*` methods; and `Math.Round`, `Math.Truncate`, `(int)` casts or `int.Parse` on protocol integers. Identity csharp-production.instructions.md; Identity ADR-0277 -- **Anchor validating regular expressions with `\z`, never `$`.** In .NET, `$` also matches before a trailing newline, so `^[a-z]+$` accepts "acme\n". *Check:* Look for validation regexes that end with `$`. Identity csharp-production.instructions.md +- **Anchor validating regular expressions with `\z`, never `$`.** In .NET, `$` also matches before a trailing newline, so `^[a-z]+$` accepts "acme\n". *Check:* Look for validation regexes that end with `$`. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Validate a value in exactly the form its consumer reads it. Use the same parser the runtime uses, reject anything that round-trips to something other than what was written, and require canonical form for values compared verbatim (for example the host as `Uri.Host` reports it, or a media type without parameters). For URIs, require an explicit scheme, reject whitespace, and check the scheme the runtime needs (for example https for a URI that will be fetched).** Lenient parsers turn typos into different values: `10.0.0` becomes `10.0.0.0`, and `/path` becomes a file URI on Linux. A value stored in a different form is accepted but never matches or works at run time. *Check:* Look for validation that relies only on `IPAddress.TryParse`, `IPNetwork.TryParse`, `Uri.TryCreate` or ad hoc regexes that differ from the consuming parser, with no exact-form or scheme check. Identity ADR-0140; Identity ADR-0173; Identity ADR-0194; Identity csharp-production.instructions.md - **Let caller and request cancellation propagate as `OperationCanceledException`. At endpoint or middleware boundaries, rethrow cancellation caused by `HttpContext.RequestAborted`, and never turn it into an error response, a retry or an audit write. A validator that returns failures as results must still rethrow cancellation, including cancellation an inner component caught and wrapped.** Cancellation means the client went away; it is not an invalid input or a server error. Writes made with the aborted token would fail anyway. *Check:* Look for catch blocks that convert request-aborted or caller cancellation into a failed result, an error status, a retry or a logged audit record, or that wrap it in a provider exception. *Note:* A best-effort call with its own bounded timeout (such as logout-time token revocation) may log its own timeout cancellation without failing the operation. Identity csharp-production.instructions.md; Identity ADR-0167; Identity ADR-0270; Identity ADR-0273; Identity ADR-0274 - **When a write loses an optimistic-concurrency race, translate only the concurrency failure (such as `DbUpdateConcurrencyException` or the store's own stale-token check) into one store-neutral concurrency exception, and let every other save exception propagate unchanged. Management endpoints answer that exception with `412 Precondition Failed` and a problem body telling the caller to re-read and retry, never 409 or 500.** Callers should handle a lost race the same way for every store, and other failures must not be disguised as a lost race. A lost race is the same condition a stale If-Match reports. *Check:* Look for `DbUpdateConcurrencyException` leaking out of persistence, catches of broad `DbUpdateException`/`Exception` mapped to the concurrency exception, and endpoints where the concurrency exception becomes 500, 409 or another status. Identity ADR-0012; Identity ADR-0208; Identity ADR-0214 @@ -49,7 +49,7 @@ applyTo: "**/*.cs, build/**, **/*.ps1, **/*.sh, **/charts/**" - **Reject with 400 a request where a security-relevant single-valued query or form parameter (a guard, a return URL, a logout_token) appears more than once.** Repeated parameters are ambiguous and can be used to slip past validation. *Check:* Look for handlers that read such parameters by first value or joined value without checking that the count is exactly one, or at most one. Identity ADR-0293 - **Under a `where T : notnull` (or unconstrained) generic, never detect presence by testing a `T?` field or parameter against null. Track presence with an explicit bool, or constrain T to `struct` and use `Nullable`.** For value types, `T?` there is just `T`, so the null check always reports a value and `default(T)` is used by mistake. *Check:* Look for generic types that use `x is null`/`is not null`/`!= null` on a `T?` member to decide whether a value exists. Identity ADR-0010; Identity csharp-production.instructions.md - **Never use the null-forgiving operator `!`, even with a justification comment. Restructure so the compiler proves the value is non-null (`?? throw`, `[MemberNotNullWhen]`, `is { } x`, early return). Declare locals that receive `[MaybeNullWhen(false)]` out arguments as nullable, and in tests narrow a maybe-null value with `Assert.NotNull` before dereferencing it.** Nullable warnings are errors, and `!` suppresses the check instead of fixing the cause. *Check:* Look for any postfix `!` (including `null!` and ones annotated `// !`), non-nullable locals receiving MaybeNullWhen out values, and test dereferences without a prior Assert.NotNull. Identity csharp-production.instructions.md; Identity ADR-0127 -- **Null-guard the receiver of an extension member with `ArgumentNullException.ThrowIfNull` or an equivalent guard. When throwing an `ArgumentException` about the receiver, pass `nameof(receiver)` as paramName and suppress CA2208 with a tight `#pragma warning disable/restore` and a one-line reason.** Extension members can be invoked on null, and CA2208 does not yet treat an extension-block receiver as a parameter. *Check:* Look for extension members that dereference the receiver unguarded, receiver exceptions with no paramName, and broad or unexplained CA2208 suppressions. Identity csharp-production.instructions.md +- **Null-guard the receiver of an extension member with `ArgumentNullException.ThrowIfNull` or an equivalent guard. When throwing an `ArgumentException` about the receiver, pass `nameof(receiver)` as paramName and suppress CA2208 with a tight `#pragma warning disable/restore` and a one-line reason.** Extension members can be invoked on null, and CA2208 does not yet treat an extension-block receiver as a parameter. *Check:* Look for extension members that dereference the receiver unguarded, receiver exceptions with no paramName, and broad or unexplained CA2208 suppressions. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Do not fall back to `DateTime` parsing when a `DateTimeOffset` parse fails. When adding a margin or skew to an externally supplied instant, use saturating arithmetic that clamps at `DateTimeOffset.MaxValue`/`MinValue`.** The fallback silently produces a wrong, machine-local instant, and plain addition near the end of the range overflows into a server error. *Check:* Look for DateTimeOffset-then-DateTime parse chains, and `+`/`Add` of a TimeSpan to token- or request-derived times without clamping. Identity ADR-0121; Identity ADR-0123 - **Validate numeric bounds explicitly. Throw `ArgumentOutOfRangeException` for a required lifetime or timeout `TimeSpan` that is not positive. Reject negative retention values but accept zero when it means immediate removal.** Unbounded sessions or nonsensical retention must not be possible, while legitimate edge values stay usable. *Check:* Look for lifetime or timeout parameters that accept zero or negative values, and retention validators that allow negative values or reject zero. Identity ADR-0220; Identity ADR-0221 - **When treating a value as a list by testing for `IEnumerable`, exclude `string` first, and compare elements with default (ordinal) equality.** A string is an `IEnumerable` and must be compared as a scalar. *Check:* Look for `is IEnumerable` checks without a preceding string exclusion, and culture-sensitive or case-insensitive element comparison. Identity ADR-0240 diff --git a/guidance/logging-and-telemetry.md b/guidance/logging-and-telemetry.md index 1cfb755..3908b87 100644 --- a/guidance/logging-and-telemetry.md +++ b/guidance/logging-and-telemetry.md @@ -26,7 +26,7 @@ applyTo: "**/*.cs, **/*.csproj, **/Program.cs, **/Dockerfile, **/*.Dockerfile, * - **Choose levels for handled conditions on purpose. Log an expected, client-recoverable condition (such as a concurrency conflict turned into 412) at Debug. Log a known failure mapped to an error response once, at Warning. Log a skipped unreadable stored document at Warning with its owning identifiers and the exception Message. Each of these gets its own event ID.** Expected conditions must not raise alarms, and operators need the same identifiers in the log that the caller or the diagnostic report received. *Check:* Look for handled, expected conditions logged at Warning or Error, known failures mapped to responses with no log, a missing or reused event ID, or a log that leaves out the identifying fields. Identity ADR-0214; Identity ADR-0245; Identity ADR-0241 - **When a change that cannot be refused (configuration reload, deployment) makes stored entities invalid, log a warning that names each affected entity and its violations, and keep starting up.** Refusing to start would let one entity take down every replica and tenant, and without a report the broken entities would go unnoticed. *Check:* Look for configuration-change paths that silently leave stored entities invalid, or that throw at startup because of one invalid entity. Identity ADR-0219 - **Log through source-generated `[LoggerMessage]` extension methods kept in a per-package `internal static partial class Log` marked `[ExcludeFromCodeCoverage]`. A direct `ILogger.Log*` call is allowed only as an approved exception, and needs a `//` comment at the call that cites the decision permitting it.** Generated logging is allocation-free, strongly typed and structured. The generator does not check the keys of a direct call, so each exception has to be visible and justified. *Check:* Look for direct logger.LogInformation/LogWarning/Log calls without a comment citing the decision, and for a Log class missing [ExcludeFromCodeCoverage]. Identity csharp-production.instructions.md; Identity docs/backend-for-frontend.md; Identity ADR-0283 -- **Make log message templates constants with named placeholders, never interpolated or concatenated strings, and pass each value as its own typed argument instead of a pre-built ToString().** This keeps logs structured, and formatting happens only when the level is enabled. *Check:* Look for `$"..."` or `+` in templates, and for `.ToString()` on log arguments. Identity csharp-production.instructions.md +- **Make log message templates constants with named placeholders, never interpolated or concatenated strings, and pass each value as its own typed argument instead of a pre-built ToString().** This keeps logs structured, and formatting happens only when the level is enabled. *Check:* Look for `$"..."` or `+` in templates, and for `.ToString()` on log arguments. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Treat log event IDs as append-only. Define them as named constants in a per-package `internal static class EventIds` (a private `Base` plus an offset, each named like its `[LoggerMessage]` method), keep each runtime package inside its own disjoint 1000-wide band, and never renumber a shipped ID or reuse a retired or reserved one. Abstractions packages log nothing.** Stable, globally unique IDs keep log queries and alerts reliable. A reused ID would make existing filters match an unrelated event. *Check:* Look for `EventId = 1234` literals, constants whose names don't match their method, changed or reused IDs, IDs outside the package's band, or logging added to an Abstractions package. Identity ADR-0167; Identity ADR-0246; Identity ADR-0283; Identity csharp-production.instructions.md - **Log a structured record as flattened key/value attributes, with numbers kept as numbers, not as one serialized JSON string. Flatten only one level, write arrays and nested values as raw JSON text, and leave out null members.** A collector cannot index fields inside an opaque string, and flattening only one level keeps the number of index fields an event can create bounded. *Check:* Look for JsonSerializer.Serialize output passed as a single log property, flattening that expands arrays element by element or recurses without limit, and null-valued keys being emitted. Identity ADR-0283; Identity docs/audit-records.md - **Declare telemetry names once, as public constants: the meter name, the activity source name, and the field and attribute names. Use the same name for one concept in the audit record, the log state, the span tags and the metric attributes, with only a namespace prefix changing.** Hosts and dashboards subscribe and filter by these names, and operators can move from a log entry to its trace and metrics only when the names match. *Check:* Look for inline string literals for meter, source, tag or field names, and for one concept named differently across signals. Identity ADR-0283; Identity docs/audit-records.md; Identity docs/metrics.md diff --git a/guidance/performance.md b/guidance/performance.md index 320a488..4c34dfc 100644 --- a/guidance/performance.md +++ b/guidance/performance.md @@ -7,17 +7,17 @@ applyTo: "**/*.cs, **/*.csproj, **/*.props, **/.editorconfig, **/vite.config.*, -- **Optimize cryptographic and numeric hot-path primitives as much as you like (AggressiveInlining/AggressiveOptimization, unrolling, stackalloc, `MemoryMarshal`/`Unsafe` zero-copy access, bounds-check elision), but never add a data-dependent branch or memory access to constant-time code, and keep test vectors and differential tests covering every optimized path.** These primitives are performance-critical, but speed must never cost constant-time behavior or correctness. *Check:* Look for data-dependent branches, early exits or secret-indexed lookups added to constant-time code, and for optimizations that no test vector or differential test covers. Identity csharp-production.instructions.md +- **Optimize cryptographic and numeric hot-path primitives as much as you like (AggressiveInlining/AggressiveOptimization, unrolling, stackalloc, `MemoryMarshal`/`Unsafe` zero-copy access, bounds-check elision), but never add a data-dependent branch or memory access to constant-time code, and keep test vectors and differential tests covering every optimized path.** These primitives are performance-critical, but speed must never cost constant-time behavior or correctness. *Check:* Look for data-dependent branches, early exits or secret-indexed lookups added to constant-time code, and for optimizations that no test vector or differential test covers. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Do not cache authorization-relevant data (roles, permissions, server-owned metadata that ends up in tokens) in a per-node `IMemoryCache` or an in-process dictionary. Read it fresh from the store by default. If it must be cached, add an opt-in, replaceable decorator over `IDistributedCache` that is invalidated when the security stamp changes.** In a cluster, a per-node cache lets different nodes make different authorization decisions for the same principal. Reading fresh keeps the store as the single source of truth. *Check:* Look for `IMemoryCache` or static/in-process dictionaries holding data that drives authorization or token contents, or for a built-in freshness setting for that data where an opt-in distributed-cache decorator belongs. Identity ADR-0055 - **Cache revocation data only after it is parsed, verified and current. Key a CRL by its location and the issuer's SHA-256 thumbprint, key an OCSP response by location, issuer and serial number, and expire each entry at the earlier of its `nextUpdate` and a configured maximum cache duration.** Revocation data must never be served past its `nextUpdate`. The cap bounds staleness, and including the issuer in the key stops one issuer's data from being reused for another. *Check:* Look for revocation cache entries with no expiry, an expiry past `nextUpdate` or no maximum cap; CRL keys made of the URL alone or using a SHA-1 thumbprint; OCSP keys missing the issuer or serial; or data cached before it is verified. *Note:* ADR-0259 describes CRL keys as per location only. ADR-0173's more specific requirement to also key by the issuer's SHA-256 thumbprint is kept. Identity ADR-0173; Identity ADR-0259 - **Cache remote protocol metadata and key sets (such as `jwks_uri`) per URI with an absolute expiration bounded by the configured duration. On a failed refresh, the cache may serve its last good value within a bounded stale lifetime, but it must never relax validation to do so. It must collapse concurrent misses into one fetch, rate-limit forced refreshes (for example on an unknown `kid`), and throw when the first fetch fails.** Caching keeps verification from fetching on every request and still allows key rotation. It keeps the client available during brief provider outages without weakening security or hammering the provider. *Check:* Look for metadata or key fetches made on every validation; caches with no absolute expiration or not keyed by URI; validation skipped for stale values; concurrent misses that each fetch; unlimited forced refreshes; or a fabricated value when the first fetch fails. Identity ADR-0106; Identity ADR-0270 - **Treat a cache-invalidation broadcast as a hint only. Every cache it invalidates must still have a bounded absolute lifetime or a polling fallback.** Broadcasts can be lost, so a replica that missed one must still converge. A sliding expiration alone can keep a busy entry stale forever. *Check:* Look for caches that rely only on broadcast or pub/sub invalidation, or that use only a sliding expiration with no absolute lifetime or periodic refresh. Identity ADR-0205 - **Fetch remote revocation data (CRLs, OCSP) only from http/https locations, through a guarded named `HttpClient` with a fetch timeout and a maximum response size. Share concurrent retrievals of the same key, and remember failed retrievals and failing responders in the in-replica cache for a failure duration.** Revocation checks run inside client authentication, so each retrieval must stay bounded and shared, and an outage should cost one timeout per replica and responder, not one per request. *Check:* Look for revocation HTTP calls without the named client, timeout or size limit; redirects or schemes other than http/https being followed; duplicate concurrent fetches; failures that are not remembered; or this cache shared across replicas. Identity ADR-0259 - **When several counters must be checked in order and processing must stop at the first refusal, run them as one atomic server-side script, not as sequential calls, a pipeline, or increment-then-compensate decrements.** Sequential calls cost a round trip each, and a pipeline cannot stop at the first refusal. Compensating decrements leave over-counts that refuse other callers and that remain if a replica stops partway. *Check:* Look for sequential awaits to the counter store for each limit in one request, pipelined increments of dependent limits, or decrement-to-undo logic after a refusal. Identity ADR-0246 -- **Never run work that grows with the size of the deployment (a pass over every tenant, client or other unbounded set) inside a request or on a bounded in-process job dispatcher. Run it as a background reconciliation over an untracked, secret-free projection that skips entities the change cannot affect.** Such passes make request latency unbounded and tie up the dispatcher's limited workers. *Check:* Look for request handlers or queued jobs that loop over every tenant, client or other unbounded entity set. Identity csharp-production.instructions.md +- **Never run work that grows with the size of the deployment (a pass over every tenant, client or other unbounded set) inside a request or on a bounded in-process job dispatcher. Run it as a background reconciliation over an untracked, secret-free projection that skips entities the change cannot affect.** Such passes make request latency unbounded and tie up the dispatcher's limited workers. *Check:* Look for request handlers or queued jobs that loop over every tenant, client or other unbounded entity set. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **When a list endpoint filters by a predicate that must be evaluated in memory, make each request scan one bounded page of the underlying rows and return a continuation token, even when the filtered page is empty. Never scan the whole set in one request.** Otherwise the cost of each request grows with the size of the collection. Callers keep paging until the token is null. *Check:* Look for list handlers that load every row of a collection, filter in memory and return everything at once, or that stop returning a continuation token as soon as a filtered page is empty. Identity ADR-0233 - **Back in-memory background work queues with a bounded channel, and log whenever a full queue drops an item.** Bounded back-pressure keeps memory in check, and logging each dropped item makes the loss visible to operators. *Check:* Look for `Channel.CreateUnbounded` or other unbounded queues in background processing, or overflow paths that drop items without logging. Identity ADR-0090 -- **Return buffered collections by default. Return lazy `IEnumerable`/`IAsyncEnumerable` only for large, I/O-backed or usually short-circuited sequences, and never as a deferred sequence that captures request or ambient context or that callers will enumerate more than once.** A deferred sequence goes stale once the request ends and re-runs its work each time it is enumerated. *Check:* Look for `yield` or deferred LINQ return values that capture request state (HttpContext, scoped services, DbContext) or that callers enumerate more than once. Identity csharp-production.instructions.md +- **Return buffered collections by default. Return lazy `IEnumerable`/`IAsyncEnumerable` only for large, I/O-backed or usually short-circuited sequences, and never as a deferred sequence that captures request or ambient context or that callers will enumerate more than once.** A deferred sequence goes stale once the request ends and re-runs its work each time it is enumerated. *Check:* Look for `yield` or deferred LINQ return values that capture request state (HttpContext, scoped services, DbContext) or that callers enumerate more than once. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Mark code paths that build generic types at runtime (`MakeGenericType` and similar reflection) with `[RequiresDynamicCode]` and `[RequiresUnreferencedCode]`, and give AOT or trimmed hosts a switch that falls back to a non-reflective path.** Building generic types at runtime is not safe under Native AOT or trimming. *Check:* Look for `MakeGenericType`, `MakeGenericMethod` or similar reflection that lacks those attributes or an AOT fallback. Identity ADR-0047 - **Serve content-hashed static files only from a restricted asset root, with `Cache-Control: public, max-age=31536000, immutable` and no per-tenant resolution. Keep the `ETag`/`Last-Modified` validators and range support, and send no cache lifetime on a `404`.** A hashed name always refers to the same content, so the file can be cached forever. Validators alone make the browser revalidate every file on every page load. The restricted root keeps files such as index.html from being served this way, and a 404 must not be cached. *Check:* Look for hashed-asset routes that omit the immutable header, drop validators or range handling, cache not-found responses, resolve a tenant, or serve files outside the asset root. Identity ADR-0254; Identity csharp-production.instructions.md - **Do not write to the database on every request for high-frequency bookkeeping. Record counts as an in-memory increment that a background service writes in batches, and throttle activity timestamps (such as last-activity or touch fields) to a minimum write interval.** Writing a row per request puts log-level write load on the database. Batching and throttling cut it to one small unit of work per replica per interval. *Check:* Look for request handlers or filters that insert or update a row, or publish an event, on every request just to count usage or advance a timestamp, without comparing against a minimum interval. Identity ADR-0199; Identity ADR-0220 @@ -38,6 +38,6 @@ applyTo: "**/*.cs, **/*.csproj, **/*.props, **/.editorconfig, **/vite.config.*, - **Configure vendor chunk splitting with Rolldown's `output.codeSplitting.groups`, not the deprecated `manualChunks`, and keep vendor packages in stable groups apart from application code.** `manualChunks` is deprecated in Vite 8. A vendor chunk that changes only when its packages are upgraded stays cached across app releases. *Check:* Look for new uses of `manualChunks`, or chunking rules that mix application modules into vendor groups. Identity ADR-0175 - **Reinterpret byte buffers as spans of blittable values with zero-copy `MemoryMarshal` APIs (`Cast`, `AsBytes`, `CreateSpan` with `Unsafe.As`), guarded by `BitConverter.IsLittleEndian` with a `BinaryPrimitives.ReverseEndianness` fallback. Do not use per-element copy loops or `BitConverter` array allocations for this.** Reinterpreting avoids copies and allocations, and the endianness guard, which the JIT treats as a constant, keeps the code correct on every architecture. *Check:* Look for per-element `BinaryPrimitives`/`BitConverter.GetBytes` loops over whole buffers, or `MemoryMarshal.Cast`/`AsBytes` on multi-byte values with no endianness guard and fallback. Identity ADR-0116 - **Load a large embedded lookup resource once, lazily on first use, into a frozen collection (for example `FrozenSet`).** This avoids re-reading and re-parsing the resource on each call, and frozen collections are optimized for repeated lookups. *Check:* Look for embedded-resource lookups that re-read or re-parse the resource on each call, or that keep it in a mutable collection. Identity ADR-0236 -- **Pass a capacity to collection constructors when the final or maximum count is known, return shared empty instances (`[]`, `FrozenSet.Empty`), write `list ?? (IReadOnlyCollection)[]` rather than `list ?? []`, and create collections lazily when they may stay empty.** This avoids resize allocations and empty-collection allocations. `list ?? []` on a `List` allocates a new list. *Check:* Look for `new List()`, `new HashSet()` or `new Dictionary<,>()` without a capacity when the count is known, `?? []` on a list-typed value, or a newly allocated empty collection being returned. Identity csharp-production.instructions.md -- **On per-request paths, use `foreach` loops instead of LINQ (`Any`, `All`, `Where(...).ToHashSet()`), and test `Count == 0` first to avoid creating a boxed enumerator.** Each LINQ call allocates a delegate and an enumerator. *Check:* Look for LINQ calls in request-handling code. Identity csharp-production.instructions.md -- **On hot paths, transform identifiers into rented or stack-allocated char spans (for example `MemoryExtensions.ToLowerInvariant(source, destination)`) instead of allocating new strings.** This avoids a string allocation on every call. *Check:* Look for `string.ToLowerInvariant()`, `ToUpperInvariant()` or similar transforms that allocate a string on per-request paths. Identity csharp-production.instructions.md +- **Pass a capacity to collection constructors when the final or maximum count is known, return shared empty instances (`[]`, `FrozenSet.Empty`), write `list ?? (IReadOnlyCollection)[]` rather than `list ?? []`, and create collections lazily when they may stay empty.** This avoids resize allocations and empty-collection allocations. `list ?? []` on a `List` allocates a new list. *Check:* Look for `new List()`, `new HashSet()` or `new Dictionary<,>()` without a capacity when the count is known, `?? []` on a list-typed value, or a newly allocated empty collection being returned. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **On per-request paths, use `foreach` loops instead of LINQ (`Any`, `All`, `Where(...).ToHashSet()`), and test `Count == 0` first to avoid creating a boxed enumerator.** Each LINQ call allocates a delegate and an enumerator. *Check:* Look for LINQ calls in request-handling code. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **On hot paths, transform identifiers into rented or stack-allocated char spans (for example `MemoryExtensions.ToLowerInvariant(source, destination)`) instead of allocating new strings.** This avoids a string allocation on every call. *Check:* Look for `string.ToLowerInvariant()`, `ToUpperInvariant()` or similar transforms that allocate a string on per-request paths. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md diff --git a/guidance/persistence.md b/guidance/persistence.md index 1292eab..675f884 100644 --- a/guidance/persistence.md +++ b/guidance/persistence.md @@ -49,7 +49,7 @@ applyTo: "**/*.cs, **/*.csproj, **/Migrations/**/*.cs, **/*.sql, build/**/*.ps1, - **Use EF Core migrations, never `EnsureCreated()`, for relational databases (keep `EnsureCreated` for the in-memory provider only). When startup finds tables but no migrations history, fail fast with an actionable message, and never delete the database automatically.** `EnsureCreated` never evolves a schema and leaves no history, so new entities fail at runtime. Deleting the database automatically loses data. *Check:* Look for `EnsureCreated(Async)` against SQLite, SQL Server or PostgreSQL, and for `EnsureDeleted()` in startup initialization. Identity ADR-0041 - **Keep migrations and host-only tables out of packable libraries. A published persistence library ships only its model, the host owns provider-specific migrations, and host concerns such as Data Protection keys live in a separate host-owned DbContext.** Migrations are provider-specific and tied to each consumer's schema lifecycle, and host tables in a library context are forced on every consumer. *Check:* Look for Migrations folders or ModelSnapshot files, `IDataProtectionKeyContext` or `DbSet` added to a packable library project. Identity ADR-0041; Identity ADR-0073 - **For each DbContext with migrations: keep a separate migration set per database provider, provide an `IDesignTimeDbContextFactory` in the migrations project and set `MigrationsAssembly` in its registration, give it its own migrations-history table when it shares a database, and add it to the `dotnet ef migrations has-pending-model-changes` drift check.** Migrations are provider-specific SQL, `Migrate()` must be able to find them, contexts must not collide on history, and a context outside the drift gate can gain unmigrated model changes. *Check:* Look for one migration set used for several providers, migrations without a design-time factory or `MigrationsAssembly`, a second context on the same connection without `MigrationsHistoryTable(...)`, or a new context missing from the drift-check script. Identity ADR-0041; Identity ADR-0073 -- **Put identifiers in double quotes inside hand-written SQL fragments in the EF model (`HasFilter`, `HasComputedColumnSql`, `HasDefaultValueSql` and similar).** PostgreSQL folds unquoted identifiers to lower case, so PascalCase columns fail at runtime even though SQLite tests and the drift gate pass. *Check:* Look for EF SQL string fragments that reference column or table names without escaped double quotes. Identity csharp-production.instructions.md +- **Put identifiers in double quotes inside hand-written SQL fragments in the EF model (`HasFilter`, `HasComputedColumnSql`, `HasDefaultValueSql` and similar).** PostgreSQL folds unquoted identifiers to lower case, so PascalCase columns fail at runtime even though SQLite tests and the drift gate pass. *Check:* Look for EF SQL string fragments that reference column or table names without escaped double quotes. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Keep a shared, extensible DbContext schema-agnostic. Add no typed `DbSet` properties for domain entities and no entity-specific configuration in its OnModelCreating. Register entities and their indexes through the owning package's model contributor and use `context.Set()`.** Typed sets or entity configuration in the shared context couple the framework back to one schema. *Check:* Look for new `DbSet<...>` properties on the shared framework context, or entity-specific configuration added to its OnModelCreating. Identity ADR-0052 - **Read and write payloads persisted as JSON only through the single schema-versioned payload serializer, never with bare `JsonSerializer`/`JsonElement.Deserialize` calls on stored columns.** A bare round-trip has no version marker for migrations to hook into, and deserializing the raw envelope (`$ver`, `$payload`) as the payload type fails for every stored row. *Check:* Look for direct System.Text.Json serialize or deserialize calls on stored grant, settings or payload columns in store or service code. Identity ADR-0113; Identity ADR-0220; Identity csharp-production.instructions.md - **Change persisted or exported record formats only additively. New members are optional (nullable or defaulted, never `required`), and no member or enum value is renamed, removed or retyped in place. A non-additive change bumps the payload's single schema version (with no per-event versions) and ships a single-step upcaster, and new fields on versioned export formats bump the minor version.** System.Text.Json enforces `required` on read, so older rows throw, and unversioned non-additive changes silently misread old data on mixed-version nodes. *Check:* Look for new `required` or non-defaulted members, renamed, removed or retyped properties, or removed enum members on persisted payload types without a version bump and upcaster, and new export fields without a schema-version change. Identity ADR-0113; Identity ADR-0283; Identity ADR-0289; Identity csharp-production.instructions.md @@ -88,6 +88,6 @@ applyTo: "**/*.cs, **/*.csproj, **/Migrations/**/*.cs, **/*.sql, build/**/*.ps1, - **In Valkey/Redis, do any compare-then-write or multi-step update (an increment plus its expiry, a prune plus a TTL extension, a version check plus a set) in one server-side Lua script, or in a single atomic command such as `SET NX PX` or `GETDEL`. Set an expiry only when the key is created, and give all keys a script touches the same `{...}` hash tag.** Separate commands let replicas interleave, so counters can outlive or extend their window, and clustered Valkey rejects scripts across slots. *Check:* Look for a GET followed by a dependent SET/EXPIRE/ZADD, `StringIncrementAsync` followed by `KeyExpireAsync`, expiry reset on every increment, and multi-key scripts or batches whose keys lack a shared hash tag or mix tenants. Identity ADR-0196; Identity ADR-0267; Identity ADR-0246 - **Bound shared-store keys that collect members (sets, sorted sets, indexes): prune expired members and keep the key's TTL no longer than its longest-lived member. Compute TTLs from the local injected `TimeProvider` and round them up to whole units rather than relying on the server's clock.** Indexes must not grow without bound or outlive their members, and truncated TTLs expire keys early. *Check:* Look for Valkey/Redis set adds without pruning or a key TTL, TTL calculations that truncate, and absolute expiry timestamps that the remote server interprets. Identity ADR-0267; Identity ADR-0164 - **Give caches that rely on at-most-once change notifications an absolute expiration, not a sliding one. Validate existence and enabled state on the cache-miss load before inserting, evict the entry before any follow-up work that rebuilds it after a write, and read just-committed state from the store, not from a polled in-memory copy.** A missed notification under sliding expiration keeps disabled entities in service indefinitely, invalid entities must never be cached, and polled copies can lag minutes behind the commit even on the replica that wrote it. *Check:* Look for `SlidingExpiration` on broadcast-invalidated caches, cache inserts before validation, writes that don't evict or that rebuild before evicting, and post-write logic that reads a cached or polled provider. Identity ADR-0219; Identity ADR-0037; Identity ADR-0169; Identity ADR-0204 -- **Keep state derived from settings in sync by reconciliation (read the current state, repair differences, durably record the input reconciled against), not by queuing a one-off job for each change.** A restart loses queued jobs, while an interrupted reconciliation can simply run again. *Check:* Look for derived state kept in sync only by enqueuing a job on change, with no durable reconciled-input record or rerunnable pass. Identity csharp-production.instructions.md +- **Keep state derived from settings in sync by reconciliation (read the current state, repair differences, durably record the input reconciled against), not by queuing a one-off job for each change.** A restart loses queued jobs, while an interrupted reconciliation can simply run again. *Check:* Look for derived state kept in sync only by enqueuing a job on change, with no durable reconciled-input record or rerunnable pass. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **In frontend data caches, scope tags to both the owner and the administrative scope, URI-encode every id embedded in a composite tag key, and invalidate only the mutating owner's tags.** Unencoded delimiters such as ':' can make one entity's tag equal another's, and broad or unscoped tags refetch or invalidate other owners' data. *Check:* Look for `providesTags`/`invalidatesTags` using bare tag types or unscoped ids, and tag keys built by joining raw ids with a separator. Identity ADR-0257 - **Configure Terraform/OpenTofu state buckets with versioning, default encryption, a public-access block and a TLS-only bucket policy, and use the S3 backend with `use_lockfile = true`.** State needs locking and history and must never be exposed. *Check:* Look for state bucket definitions missing versioning, SSE, the public access block or an `aws:SecureTransport` deny, and for S3 backends without `use_lockfile`. Identity ADR-0132 diff --git a/guidance/protocols.md b/guidance/protocols.md index 050cbd7..45f3604 100644 --- a/guidance/protocols.md +++ b/guidance/protocols.md @@ -145,5 +145,5 @@ applyTo: "**/*.cs, **/*.ts, **/*.tsx, **/*.cshtml, **/*.html" - **When building a query string by hand, percent-encode each name and value with `Uri.EscapeDataString`, and join to the endpoint with `?`, or with `&` when it already has a query.** This is correct RFC 3986 encoding, and existing queries are preserved. *Check:* Look for unencoded concatenation, `HttpUtility`/`WebUtility.UrlEncode`, or an unconditional `?`. Identity ADR-0285 - **Load protocol endpoint requests into strongly-typed request models backed by known parameter definitions instead of hand-rolled `form.TryGetValue`/`Request.Form[...]` reads, and keep endpoints thin: guard the verb and content type, authenticate the client through the shared service, run validation steps, and render.** Typed requests give consistent parsing, extensibility and OpenAPI metadata, and keep protocol steps replaceable. *Check:* Look for direct form or query reads, or inline validation in endpoint code. Identity ADR-0086; Identity csharp-production.instructions.md - **Refer to OAuth/OIDC parameter names and values, and HTTP status codes, through shared named constants (status codes named like ASP.NET Core `StatusCodes`) instead of inline string literals, `(int)HttpStatusCode.X` casts or bare numbers.** One vocabulary prevents typos and client/server mismatches. *Check:* Look for literal protocol field names in builders or parsers, and for enum casts or magic numbers in status comparisons. Identity review of #350; Identity ADR-0285 -- **Give a JSON-deserialized wire DTO a `[JsonExtensionData] Dictionary? ExtensionData` member.** Unrecognized members then round-trip instead of being dropped. *Check:* Look for new deserialized DTOs without an extension data bag. Identity csharp-production.instructions.md +- **Give a JSON-deserialized wire DTO a `[JsonExtensionData] Dictionary? ExtensionData` member.** Unrecognized members then round-trip instead of being dropped. *Check:* Look for new deserialized DTOs without an extension data bag. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **When an API-reference renderer (Scalar/OpenAPI) logs in with client credentials, send them where the client's registered auth method expects (for example the Authorization header for `client_secret_basic`). Pre-fill form fields by marking them `required` with a schema `Default`, not with examples.** A client may only authenticate with its registered method, and renderers ignore optional properties and named examples. *Check:* Look for a credentials location that does not match the client's method, or transformers that use `example` to pre-fill form fields. Identity ADR-0046; Identity csharp-production.instructions.md diff --git a/guidance/public-api.md b/guidance/public-api.md index 489633c..d5ab493 100644 --- a/guidance/public-api.md +++ b/guidance/public-api.md @@ -17,7 +17,7 @@ applyTo: "**/*.cs, **/*.csproj, **/*.props, **/*.targets, **/PublicAPI.Shipped.t - **Make an extensibility interface async from the start if a plausible replacement implementation would need IO.** Changing a synchronous signature to async later is a breaking change. *Check:* Look for new replaceable policy or resolver interfaces with synchronous methods that real implementations would back with a database or remote lookup. Identity ADR-0091 - **Before a package's first stable release, keep PublicAPI.Shipped.txt empty and list the whole surface in PublicAPI.Unshipped.txt. Make breaking changes as a clean cutover with no shims, `[Obsolete]` forwarders, legacy aliases or dual code paths, and remove broken public values (such as discriminators no handler can round-trip) outright. Move the surface to Shipped.txt at the first stable release, after which the compatibility rules apply in full.** With no released consumers, deprecation cycles buy nothing, and a half-migrated design with two sources of truth is the worse outcome. *Check:* In a pre-1.0 package, look for entries added to Shipped.txt, compatibility shims or fallback branches that read both old and new shapes, and public constants or discriminators with no handler. Do not object to a pre-release rename or reshaped contract only because it breaks compatibility; judge its design instead. In a stable release, flag surface still only listed in Unshipped.txt. *Note:* This overrides the Obsolete-first and additive-only rules for pre-1.0 packages only. Identity ADR-0005; Identity ADR-0025; Identity ADR-0105 - **Every packable project must reference Microsoft.CodeAnalysis.PublicApiAnalyzers and keep a PublicAPI.Shipped.txt/PublicAPI.Unshipped.txt pair. Record every public addition, including new public abstract or virtual hooks and both the container and member lines of a C# extension member, in Unshipped.txt. Mark removals with a `*REMOVED*` prefix instead of deleting lines, remove stale entries, keep the files LF, and edit Shipped.txt only at release time.** The analyzer makes every surface change show up in the diff, so accidental API changes get caught. Shipped.txt is the baseline for the next release. *Check:* Look for new packable projects without the files or that opt out of the analyzer, public changes without a matching Unshipped.txt diff, deleted lines in place of `*REMOVED*`, stale entries, CRLF rewrites, and Shipped.txt edits outside a release. Identity ADR-0005; Identity ADR-0009; Identity ADR-0053; Identity docs/api-compatibility.md; Identity csharp-production.instructions.md -- **Give every type and member the narrowest visibility that works. Make something public only for a stated consumer-facing reason, never for tests, and never add `public virtual` or leave a public type unsealed just so it can be mocked.** An accidental public member is a SemVer contract that only a major-version obsoletion cycle can remove. A public virtual member is also an override point that a subclass could use to weaken security controls. *Check:* Look for new public types or members without a consumer-facing reason, members widened to public, protected or virtual only for tests, and public concrete classes that are unsealed but not designed for subclassing. *Note:* Use `internal virtual` for one-off override seams, and leave internal implementation classes unsealed so they can be mocked. Identity csharp-production.instructions.md; Identity csharp-testing.instructions.md +- **Give every type and member the narrowest visibility that works. Make something public only for a stated consumer-facing reason, never for tests, and never add `public virtual` or leave a public type unsealed just so it can be mocked.** An accidental public member is a SemVer contract that only a major-version obsoletion cycle can remove. A public virtual member is also an override point that a subclass could use to weaken security controls. *Check:* Look for new public types or members without a consumer-facing reason, members widened to public, protected or virtual only for tests, and public concrete classes that are unsealed but not designed for subclassing. *Note:* Use `internal virtual` for one-off override seams, and leave internal implementation classes unsealed so they can be mocked. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md; Identity csharp-testing.instructions.md - **Give tests access to internals through `InternalsVisibleTo` for the test assembly (plus `DynamicProxyGenAssembly2` for Moq) only. Never use `InternalsVisibleTo` to let one production assembly use another's internals; make packages collaborate through designed public contracts.** Cross-assembly internals access is hidden coupling that PublicApiAnalyzers does not track, so a refactor that is safe in one assembly can break another. *Check:* Look for `InternalsVisibleTo` attributes or csproj items that target a non-test production assembly. Identity ADR-0004; Identity csharp-testing.instructions.md; Identity csharp-production.instructions.md - **Mark a type or member that is public only for infrastructure collaboration between sibling packages with `[EditorBrowsable(EditorBrowsableState.Never)]`, and start its `` with "This API supports the ... infrastructure and is not intended to be used directly from your code."** Following the EF Core / ASP.NET Core pattern, the collaboration surface stays explicit and version-tracked without showing up in application IntelliSense. *Check:* Look for new public members that exist only for sibling packages but lack the attribute or the standard summary sentence. Identity ADR-0004 - **Expose a replaceable service as a public interface and make its single canonical DI-resolved implementation an `internal` class named `DefaultFoo` for interface `IFoo`, reached only through the interface.** Integrators extend the library by implementing the interface, not by depending on or subclassing the default. This follows the ASP.NET Core idiom and keeps the surface small. *Check:* Look for a public `Default*` class, or for a sole DI-registered implementation of `IFoo` named `FooService`, `FooImpl` or plain `Foo`. Identity ADR-0006; Identity ADR-0015; Identity docs/backend-for-frontend.md; Identity csharp-production.instructions.md @@ -55,17 +55,17 @@ applyTo: "**/*.cs, **/*.csproj, **/*.props, **/*.targets, **/PublicAPI.Shipped.t - **Use public constructors on model types to adopt an in-memory value (`JsonElement`, `JsonObject`, a collection), and a static `Parse(string)` method to deserialize text.** A consistent verb split tells callers whether input is adopted or parsed. *Check:* Look for constructors that take raw JSON text, and for `Parse`/`From*` factories that only wrap an in-memory node or collection. Identity ADR-0119 - **Make a wrapper or overload that claims to behave like an existing API honor every option it accepts, or throw `NotSupportedException` for options it cannot honor. Never ignore an option silently.** A JSON wrapper that dropped `AllowDuplicateProperties` accepted duplicate members even though its documentation said it would not. *Check:* Look for option objects copied field by field with fields left out. Identity review of #420 - **Make data, options, context and event types sealed, with init or get-only members and concrete buffered collection properties (`List`, not `IList`). Give options classes `IValidateOptions` validators, and add interfaces only for replaceable DI seams or read-only slices.** This is AOT-friendly, cheap to re-enumerate, and free of deferred-execution surprises, and options are checked before use. *Check:* Look for unsealed DTOs or options, settable members, interface-typed collection properties on data types, and options without a validator. Identity csharp-production.instructions.md; Identity docs/backend-for-frontend.md -- **Type parameters and computed return values that do not transfer ownership as the narrowest read-only interface (`IReadOnlyList`, `IReadOnlyCollection`, `IReadOnlySet`). Prefer `params IEnumerable` over `params T[]` and buffer inside the method. Return empty collections, not `null`, where an empty form exists.** Callers are not coupled to concrete types, and null-for-empty forces null checks on every caller and invites NullReferenceExceptions. *Check:* Look for concrete collection parameters, new `params T[]`, and methods that return or accept `null` where an empty value would do. Identity csharp-production.instructions.md +- **Type parameters and computed return values that do not transfer ownership as the narrowest read-only interface (`IReadOnlyList`, `IReadOnlyCollection`, `IReadOnlySet`). Prefer `params IEnumerable` over `params T[]` and buffer inside the method. Return empty collections, not `null`, where an empty form exists.** Callers are not coupled to concrete types, and null-for-empty forces null checks on every caller and invites NullReferenceExceptions. *Check:* Look for concrete collection parameters, new `params T[]`, and methods that return or accept `null` where an empty value would do. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Have interface methods that produce output write into a `Span`/`Memory` destination and return a count, or into a generic `ref TWriter ... allows ref struct` writer with a sizing method beside it, instead of returning a new array. Put the input or source parameter first and the destination last.** This avoids allocation, lets callers size buffers up front, and matches existing source-first APIs such as `CopyPlaintext(source, destination)`. *Check:* Look for new interface methods that return `byte[]`, and for methods whose first parameter is a writer, buffer writer or destination span. Identity csharp-production.instructions.md; Identity review of #382 - **Represent HTTP status codes in public models and response wrappers as `int`, not `System.Net.HttpStatusCode`.** The enum cannot name every status a server may send (such as 499), so values would not be preserved faithfully. *Check:* Look for `HttpStatusCode`-typed properties, parameters or fields in new models, except code that builds or reads `HttpResponseMessage` directly. Identity review of #320 - **Do not add a parameter that only duplicates information already available from another argument. For example, read endpoint metadata from `HttpContext.GetEndpoint()` instead of taking an endpoint name next to the `HttpContext`.** A redundant parameter can disagree with the authoritative source. *Check:* Look for methods that take both an `HttpContext` and an endpoint name or endpoint argument derivable from it. Identity review of #382 - **Do not add bool or enum parameters whose only job is to switch a method between behaviors; split it into intention-revealing methods. Likewise, use a separate validator contract for each token type with different required claims or `typ`, instead of a mode flag.** Flag parameters hide intent at call sites and produce conditional rules. *Check:* Look for flag parameters that select between branches, and for token-kind parameters or conditional claim requirements added to an existing validator. Identity csharp-production.instructions.md; Identity ADR-0274 - **Do not add speculative public surface: no settings that control nothing that matters (such as body-size limits on a call whose body is discarded), and no public type shared between two independently versioned packages. Keep such a type in one owning package or duplicate it deliberately.** Unused settings are permanent surface, and shared types across independently versioned packages create diamond dependency conflicts. *Check:* Look for new options properties on paths where they have no effect, and for new shared public types used in the public surface of several independently versioned packages. Identity ADR-0111; Identity docs/api-compatibility.md - **In key- or secret-describing contracts, include `kid` as a required, non-nullable field already resolved to its fallback.** A nullable `kid` makes every caller repeat the fallback logic, and keys are selected by `kid` either way. *Check:* Look for key or secret response DTOs that omit `kid` or declare it nullable or optional. Identity ADR-0186 -- **Treat a public type's namespace as a stable contract. Do not change it when files move, and keep public types flat under the package root namespace, except options, which go in a `.Options` sub-namespace.** Reorganizing folders must not cause breaking namespace churn for consumers. *Check:* Look for public types whose namespace changed only because the file moved, and for public types in folder-based sub-namespaces other than `.Options`. Identity csharp-production.instructions.md +- **Treat a public type's namespace as a stable contract. Do not change it when files move, and keep public types flat under the package root namespace, except options, which go in a `.Options` sub-namespace.** Reorganizing folders must not cause breaking namespace churn for consumers. *Check:* Look for public types whose namespace changed only because the file moved, and for public types in folder-based sub-namespaces other than `.Options`. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Name interfaces for their role and let the namespace supply the brand. Add a qualifier only when the bare role noun would collide with a well-known BCL or framework type or is too generic. Use established acronym casing consistently (for example `DPoP`, never `Dpop`).** Role names are short and clear, and consistent casing keeps identifiers matching the existing protocol constants. *Check:* Look for needless brand prefixes, bare names that collide with common BCL types, and identifiers containing `Dpop`. Identity csharp-production.instructions.md; Identity ADR-0286 -- **Annotate public types that are consumed by reflection, DI, the serializer or the options binder with JetBrains `[PublicAPI]`, and never apply it to internal types.** It states the intent and silences unused-member analyzers without pragmas. On an internal type it would wrongly present the type as public surface. *Check:* Look for serializer- or DI-consumed public types without `[PublicAPI]`, and for internal types that have it. Identity csharp-production.instructions.md -- **When a concrete type implements two unrelated receiver interfaces (such as `IDictionary<,>` and `IReadOnlyDictionary<,>`), write one extension method on their common base and fast-path the specific interfaces inside it, not one overload per interface.** Parallel overloads make every call on the concrete type ambiguous (CS0121), and moving them into separate classes does not fix it. *Check:* Look for parallel extension overloads on `IDictionary<,>` and `IReadOnlyDictionary<,>` or similar pairs. Identity csharp-production.instructions.md +- **Annotate public types that are consumed by reflection, DI, the serializer or the options binder with JetBrains `[PublicAPI]`, and never apply it to internal types.** It states the intent and silences unused-member analyzers without pragmas. On an internal type it would wrongly present the type as public surface. *Check:* Look for serializer- or DI-consumed public types without `[PublicAPI]`, and for internal types that have it. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md +- **When a concrete type implements two unrelated receiver interfaces (such as `IDictionary<,>` and `IReadOnlyDictionary<,>`), write one extension method on their common base and fast-path the specific interfaces inside it, not one overload per interface.** Parallel overloads make every call on the concrete type ambiguous (CS0121), and moving them into separate classes does not fix it. *Check:* Look for parallel extension overloads on `IDictionary<,>` and `IReadOnlyDictionary<,>` or similar pairs. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **For a TypeScript string vocabulary that the backend treats as open, type it as the known literals plus `(string & {})`, not as a closed union, unless it is provably closed.** A closed union turns every new value into a library edit, while the open form still autocompletes the known names. *Check:* Look for string literal unions for surfaces, kinds or types that the backend maps through an open dictionary and that lack `| (string & {})`. Identity ADR-0156 - **Keep element ids, field names and `autocomplete` values identical between server-rendered pages and the client components that replace them, and treat them as a contract.** Client bundles, automation and browser autofill all depend on these names whether or not the bundle has loaded. *Check:* Look for changes to ids, names or `autocomplete` values on only one side of a server page and its client component. Identity ADR-0253 - **Treat an infrastructure module's variables and outputs as its public API. Removing or renaming a variable or output, adding a required input, or forcing resource replacement is a breaking change that needs migration notes. When a refactor changes resource addresses, add `moved` or `removed` blocks.** Consumers must act on such changes, and forced replacement destroys live resources. *Check:* Look for removed or renamed variables or outputs, new variables without defaults, and renamed resources, module moves or changed `count`/`for_each` keys with no `moved`/`removed` block or breaking-change note. Identity ADR-0133 diff --git a/guidance/security-and-crypto.md b/guidance/security-and-crypto.md index 0479519..d1578ab 100644 --- a/guidance/security-and-crypto.md +++ b/guidance/security-and-crypto.md @@ -101,7 +101,7 @@ applyTo: "**/*.cs, **/*.csproj, **/*.ts, **/*.tsx, **/*.cshtml, **/*.razor, **/* - **Take cloud and KMS credentials from the vendor SDK's keyless credential chain. Wrap long-lived keys with RSA-OAEP-256 on keys of 3072 bits or more, bind owner and key id as AAD or encryption context, record and verify the versioned key the response reports, and send and verify CRC32C (via BitOperations) for Cloud KMS.** Stored cloud credentials are ruled out, and without binding and integrity checks a wrapped key could be misused or silently corrupted. *Check:* Secrets or key files in cloud adapter options, RSA1_5 or SHA-1 OAEP wraps, missing AAD, versionless key ids stored, or KMS calls without checksums. Identity ADR-0235; Identity ADR-0260 - **Persist the Data Protection key ring beside the data it protects. Never auto-revoke undecryptable keys outside Development, and skip revoked or expired keys before reading Descriptor.** Split storage makes secrets undecryptable, and revocation in production hides a lost master key permanently. *Check:* PersistKeysToFileSystem for database-backed data, RevokeKey on failure without a Development or configured-id guard, or Descriptor read before filtering. Identity ADR-0073; Identity ADR-0247 - **Rate-limit by IPv6 /64 with IPv4-mapped addresses normalized, count rejected requests too, check per-address limits before tenant limits, key per-client limits only after confidential-client authentication, and exempt anonymous discovery and JWKS from tenant quotas.** Otherwise attackers spread across addresses, retry without backing off, or spend other clients' or tenants' allowances. *Check:* Raw RemoteIpAddress keys, increments only on admit, tenant counters incremented before or despite an address refusal, client_id limits before authentication, or metered metadata endpoints. Identity ADR-0196; Identity ADR-0198; Identity ADR-0200; Identity ADR-0246 -- **Never cache per-request state such as authentication results in a singleton's fields; keep it on HttpContext.Items or a request context.** A shared field leaks one request's answer to every later request. *Check:* `_field ??= await ...` or mutable fields set during a request in singleton types. Identity csharp-production.instructions.md +- **Never cache per-request state such as authentication results in a singleton's fields; keep it on HttpContext.Items or a request context.** A shared field leaks one request's answer to every later request. *Check:* `_field ??= await ...` or mutable fields set during a request in singleton types. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Enforce security middleware ordering with per-request checks that fail closed, and make extension hooks inside security checks able only to reject.** A misordered pipeline or a permissive hook must never serve a guarded endpoint unprotected. *Check:* Middleware that silently no-ops when misordered, or gate interfaces that can return success to skip built-in checks. Identity ADR-0292; Identity docs/backend-for-frontend.md - **Make a forwarding proxy send requests only to static, allow-listed destinations and methods. Normalize and reject dot segments, encoded slashes and backslashes, strip inbound Cookie/Authorization/anti-forgery/DPoP headers and upstream Set-Cookie, and never follow redirects.** This prevents SSRF, path traversal and credential leakage downstream. *Check:* Destinations built from request data, copied header collections, or AllowAutoRedirect left on. Identity docs/backend-for-frontend.md - **Serve static files through a PhysicalFileProvider rooted at the public asset directory only, answering 404 for traversal, hidden files and unknown content types.** Only intended assets may be exposed. *Check:* Request paths joined onto filesystem paths by hand, providers rooted above the asset folder, or unknown types served. Identity ADR-0193; Identity ADR-0254 diff --git a/guidance/testing.md b/guidance/testing.md index 2baa2ce..2a70de4 100644 --- a/guidance/testing.md +++ b/guidance/testing.md @@ -9,61 +9,61 @@ applyTo: "**/*Tests/**/*.cs, **/*.Tests/**/*.cs, **/*Tests/**/*.csproj, **/*.cs, - **Do not skip, disable, delete or weaken existing tests or assertions to make a change pass. Fix the code, or update the test to match the new correct behavior and say why.** Skipped or weakened tests hide broken behavior while the checks stay green. *Check:* Look for newly added Skip attributes, tests that are commented out or deleted, removed assertions, or loosened expectations (an exact check replaced by NotNull or a wider range) with no behavior change that justifies them. Identity .claude/commands/review-plan-diff.md; Identity .github/agents/copilot-reviewer.agent.md - **Make a negative test fail only because of the rule under test. Make the refused path otherwise viable: register a working implementation of the denied algorithm or item (including a working `none` algorithm when testing a `none` guard), and arrange any refused fallback so it would succeed. Pair each with a control test where the same path succeeds because the rule doesn't apply.** If the denied item isn't registered or the fallback can't work anyway, the refusal happens for some other reason and the test still passes with the rule removed. *Check:* Look for deny or refuse tests where the denied item is not registered, the fallback is not viable, or no permitted control test exists alongside them. Identity ADR-0282; Identity ADR-0295; Identity csharp-testing.instructions.md -- **Give a negative test input that every earlier or built-in check accepts, so only the rule under test refuses it. This includes a key size the algorithm accepts when testing curve binding, values the default rules don't already reject when testing an extension's rule, a request the old code accepted and deferred when testing a refusal moved earlier, and a seeded client registered for the exact authentication method being tested. Assert the specific error text when another check could refuse the same input with a different message.** When an earlier check, a built-in rule or a mismatched registration refuses the input, the test passes even with the rule under test removed. *Check:* Look for negative tests whose input an earlier validation step would also refuse, for example a wrong-curve key of the wrong size, standard names the defaults already reject, input the old code also refused, or a client whose token_endpoint_auth_method differs from the one used. Also look for tests that assert only failure where the message is what tells the causes apart. Identity csharp-testing.instructions.md -- **When a test seeds data that is meant to be invalid or unreadable, first assert that the system actually rejects it, then assert the downstream behavior.** Without that precondition the test can pass on data the server reads without trouble. *Check:* Look for tests that seed corrupt or incoherent data and go straight to downstream assertions, with no check that the data is rejected. Identity csharp-testing.instructions.md +- **Give a negative test input that every earlier or built-in check accepts, so only the rule under test refuses it. This includes a key size the algorithm accepts when testing curve binding, values the default rules don't already reject when testing an extension's rule, a request the old code accepted and deferred when testing a refusal moved earlier, and a seeded client registered for the exact authentication method being tested. Assert the specific error text when another check could refuse the same input with a different message.** When an earlier check, a built-in rule or a mismatched registration refuses the input, the test passes even with the rule under test removed. *Check:* Look for negative tests whose input an earlier validation step would also refuse, for example a wrong-curve key of the wrong size, standard names the defaults already reject, input the old code also refused, or a client whose token_endpoint_auth_method differs from the one used. Also look for tests that assert only failure where the message is what tells the causes apart. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **When a test seeds data that is meant to be invalid or unreadable, first assert that the system actually rejects it, then assert the downstream behavior.** Without that precondition the test can pass on data the server reads without trouble. *Check:* Look for tests that seed corrupt or incoherent data and go straight to downstream assertions, with no check that the data is rejected. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **When the code under test swallows exceptions, make the test fail through mock verification (for example `.Verifiable(Times.Never())`) or by asserting that the error logger was not called. Don't rely on a strict mock throwing.** The code swallows the strict mock's `MockException`, so the test passes whatever the code does. *Check:* Look for tests of code that catches exceptions broadly where the tests depend only on strict mocks throwing and never verify. Identity review of #420 - **Cover the edge cases and failure modes of a new or changed capability, not only the success path.** An untested branch, validation or error path is a gap the review is meant to catch. *Check:* For each new or changed branch, validation or error path in production code, confirm that some test exercises it. Flag test changes that cover only the happy path. NCodeGroup maintainer: gap-hunting-review - **Test optimistic-concurrency conflicts, unique and filtered-unique indexes, and foreign-key enforcement against a relational provider such as SQLite in-memory (one shared open SqliteConnection plus EnsureCreated), never EF InMemory. Use two DbContexts as the racing writers.** EF InMemory leaves concurrency tokens out of the UPDATE WHERE clause and enforces no unique index or foreign key, so these tests pass without exercising the constraint. *Check:* Look for tests that expect DbUpdateConcurrencyException (or the exception it is translated into), a duplicate-key DbUpdateException or a dependent-row failure while running on UseInMemoryDatabase. *Note:* The reverse applies to queries that filter or order by DateTimeOffset: EF Core's SQLite provider can't translate those, so test them on EF InMemory (see the DateTimeOffset rule). Identity ADR-0012; Identity csharp-testing.instructions.md -- **Test persistence behavior that depends on SaveChanges side effects (concurrency tokens, cascade deletes, SaveChanges interceptors, value converters) against a real EF Core DbContext, and cover a handler that writes through more than one store with an end-to-end test through the host, not only with mocked stores.** Mocked stores can't observe interceptors or the ordering constraints between stores. In one case an interceptor hooked only to the synchronous SavingChanges never ran, and in another a handler failed on every call while its unit tests passed. *Check:* Look for new or changed EF interceptors, converters, concurrency tokens, cascade configuration, or multi-store handlers whose only tests mock the store. Identity csharp-testing.instructions.md -- **Test an adapter over ASP.NET Core Identity against a real UserManager (AddIdentityCore plus EF stores) as well as a mocked one.** A mocked UserManager hides real behavior such as password validators on CreateAsync, and rehashing and security-stamp rotation on CheckPasswordAsync. *Check:* Look for Identity adapter changes that are tested only against a mocked UserManager. Identity csharp-testing.instructions.md -- **Build stored-payload fixtures with the production serializer or protector, never with a bare JsonSerializer call.** Stored rows are protected, versioned envelopes, so a test that hands the code a bare payload passes while the code fails on every real row. *Check:* Look for tests that seed persisted payloads with JsonSerializer.Serialize instead of the production serializer or protector. Identity csharp-testing.instructions.md -- **In a test of a global query filter, call ChangeTracker.Clear() or use a fresh DbContext after seeding, so the assertion runs a real database query.** HasQueryFilter is not applied again to entities returned from the change tracker, so the test passes without the filter running. *Check:* Look for query-filter tests that seed and then query through the same context without clearing tracked entities. Identity csharp-testing.instructions.md -- **Generate an EF InMemory database name once, in a local outside the AddDbContext options lambda, and close over it.** The options lambda runs for every context, so a Guid.NewGuid() inside it gives each scope a separate empty database and the test's writes vanish in the next scope. *Check:* Look for UseInMemoryDatabase(Guid.NewGuid()...) inside an AddDbContext options lambda. Identity csharp-testing.instructions.md -- **Call EnableServiceProviderCaching(false) in a test that adds entities or model configuration to an existing DbContext type.** EF Core caches the model per context type across service providers, so the test may get a model built without its entity. *Check:* Look for tests that contribute extra entities to a shared DbContext type without disabling service provider caching. Identity csharp-testing.instructions.md -- **Test store queries that filter on or order by a DateTimeOffset on EF InMemory, not on the EF Core SQLite provider.** EF Core's SQLite provider can't compare or order DateTimeOffset values. *Check:* Look for SQLite-based tests of queries that compare or order DateTimeOffset columns. Identity csharp-testing.instructions.md +- **Test persistence behavior that depends on SaveChanges side effects (concurrency tokens, cascade deletes, SaveChanges interceptors, value converters) against a real EF Core DbContext, and cover a handler that writes through more than one store with an end-to-end test through the host, not only with mocked stores.** Mocked stores can't observe interceptors or the ordering constraints between stores. In one case an interceptor hooked only to the synchronous SavingChanges never ran, and in another a handler failed on every call while its unit tests passed. *Check:* Look for new or changed EF interceptors, converters, concurrency tokens, cascade configuration, or multi-store handlers whose only tests mock the store. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Test an adapter over ASP.NET Core Identity against a real UserManager (AddIdentityCore plus EF stores) as well as a mocked one.** A mocked UserManager hides real behavior such as password validators on CreateAsync, and rehashing and security-stamp rotation on CheckPasswordAsync. *Check:* Look for Identity adapter changes that are tested only against a mocked UserManager. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Build stored-payload fixtures with the production serializer or protector, never with a bare JsonSerializer call.** Stored rows are protected, versioned envelopes, so a test that hands the code a bare payload passes while the code fails on every real row. *Check:* Look for tests that seed persisted payloads with JsonSerializer.Serialize instead of the production serializer or protector. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **In a test of a global query filter, call ChangeTracker.Clear() or use a fresh DbContext after seeding, so the assertion runs a real database query.** HasQueryFilter is not applied again to entities returned from the change tracker, so the test passes without the filter running. *Check:* Look for query-filter tests that seed and then query through the same context without clearing tracked entities. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Generate an EF InMemory database name once, in a local outside the AddDbContext options lambda, and close over it.** The options lambda runs for every context, so a Guid.NewGuid() inside it gives each scope a separate empty database and the test's writes vanish in the next scope. *Check:* Look for UseInMemoryDatabase(Guid.NewGuid()...) inside an AddDbContext options lambda. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Call EnableServiceProviderCaching(false) in a test that adds entities or model configuration to an existing DbContext type.** EF Core caches the model per context type across service providers, so the test may get a model built without its entity. *Check:* Look for tests that contribute extra entities to a shared DbContext type without disabling service provider caching. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Test store queries that filter on or order by a DateTimeOffset on EF InMemory, not on the EF Core SQLite provider.** EF Core's SQLite provider can't compare or order DateTimeOffset values. *Check:* Look for SQLite-based tests of queries that compare or order DateTimeOffset columns. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **Keep unit tests hermetic: no network, no ambient host, no machine state. Use DefaultHttpContext with test RequestServices, in-memory fakes and constructed principals. Serve remote data such as JWKS, CRLs and OCSP responses through stub HttpMessageHandlers or IHttpClientFactory stubs, and declare a single-use stub as a `private sealed` type in the test file.** Tests have to be isolated and deterministic. Network and machine state make them flaky. *Check:* Look for real HTTP calls or external URLs, host startup, file-system or other external resources in unit tests, and for one-off stubs that are public or placed in shared files. Identity docs/backend-for-frontend.md; Identity csharp-testing.instructions.md - **Give every test host its own Data Protection key ring (an in-memory XmlRepository or a per-factory in-memory store), never the machine's default key directory. Give each integration test that protects or unprotects data its own WebApplicationFactory (`using var factory = ...`), not a shared IClassFixture.** Keys left by earlier runs under a different master key leak into tests, and shared hosts leak key-ring and serializer state between tests, so later runs fail. *Check:* Look for WebApplicationFactory or test-host setup without its own Data Protection key repository, and for Data Protection-dependent tests that use a class-fixture factory. Identity ADR-0247; Identity csharp-testing.instructions.md - **Use ephemeral in-memory signing keys in tests, with nothing written to a database or the file system, and never enable the persistent developer-key mode in a test host.** Tests need keys that are created fresh on every run, which is the opposite of what persistent local development needs. *Check:* Look for test hosts or fixtures that enable persistent developer keys, write key rings to disk, or depend on a persisted signing key. Identity ADR-0021 -- **Never assert a host startup failure through WebApplicationFactory. Build the host directly (for example Host.CreateEmptyApplicationBuilder, then the registration under test, then StartAsync) and assert there.** Under parallel load the disposed service provider can throw an ObjectDisposedException that hides the real error, so the test passes alone and fails in the gate. *Check:* Look for tests that expect an exception from WebApplicationFactory.CreateClient, Server or Services. Identity csharp-testing.instructions.md -- **Never call StopAsync or Dispose on a WebApplicationFactory's host. Dispose the factory instead.** Stopping the host while its own run loop also stops it disposes the service provider under in-flight work, and random tests fail during disposal. *Check:* Look for test code calling StopAsync or Dispose on an IHost obtained from a factory. Identity csharp-testing.instructions.md -- **Supply configuration that the composition root reads while registering services through builder.UseSetting(key, value) in the test factory, not ConfigureAppConfiguration, and assert an effect that only that composition produces. Configure every value an integration test depends on in the factory, because WebApplicationFactory never loads launchSettings.json.** A ConfigureAppConfiguration overlay reaches options bound later but not values read during registration, and launch-profile values are missing in tests, so the test can quietly run the wrong composition. *Check:* Look for test factories that set registration-time switches through ConfigureAppConfiguration, tests that never show the setting took effect, and tests that assume values defined only in launch profiles. Identity csharp-testing.instructions.md -- **When a test needs a key or setting that a host or tenant reads once at first use, provision it before the first request, and assert an effect that only that key or setting produces (for example, that the token is a five-segment JWE).** The value is read once at first use, and the assertion proves it was actually picked up. *Check:* Look for tests that seed keys or settings after the first request, or that never assert the resulting effect. Identity csharp-testing.instructions.md -- **Change configuration at runtime in a test by setting the key on the host's IConfiguration cast to IConfigurationRoot and then calling Reload().** The test's in-memory overlay is the last provider, so it keeps the value through the reload and every reload token fires. *Check:* Look for runtime-reconfiguration tests that change configuration without calling Reload, or that rebuild the host instead. Identity csharp-testing.instructions.md +- **Never assert a host startup failure through WebApplicationFactory. Build the host directly (for example Host.CreateEmptyApplicationBuilder, then the registration under test, then StartAsync) and assert there.** Under parallel load the disposed service provider can throw an ObjectDisposedException that hides the real error, so the test passes alone and fails in the gate. *Check:* Look for tests that expect an exception from WebApplicationFactory.CreateClient, Server or Services. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Never call StopAsync or Dispose on a WebApplicationFactory's host. Dispose the factory instead.** Stopping the host while its own run loop also stops it disposes the service provider under in-flight work, and random tests fail during disposal. *Check:* Look for test code calling StopAsync or Dispose on an IHost obtained from a factory. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Supply configuration that the composition root reads while registering services through builder.UseSetting(key, value) in the test factory, not ConfigureAppConfiguration, and assert an effect that only that composition produces. Configure every value an integration test depends on in the factory, because WebApplicationFactory never loads launchSettings.json.** A ConfigureAppConfiguration overlay reaches options bound later but not values read during registration, and launch-profile values are missing in tests, so the test can quietly run the wrong composition. *Check:* Look for test factories that set registration-time switches through ConfigureAppConfiguration, tests that never show the setting took effect, and tests that assume values defined only in launch profiles. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **When a test needs a key or setting that a host or tenant reads once at first use, provision it before the first request, and assert an effect that only that key or setting produces (for example, that the token is a five-segment JWE).** The value is read once at first use, and the assertion proves it was actually picked up. *Check:* Look for tests that seed keys or settings after the first request, or that never assert the resulting effect. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Change configuration at runtime in a test by setting the key on the host's IConfiguration cast to IConfigurationRoot and then calling Reload().** The test's in-memory overlay is the last provider, so it keeps the value through the reload and every reload token fires. *Check:* Look for runtime-reconfiguration tests that change configuration without calling Reload, or that rebuild the host instead. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **Use fixed instants or a fake TimeProvider in tests of time logic, never the wall clock. In production code, reach the clock, Guid.NewGuid(), the crypto RNG, Activity and ambient environment (environment variables, file-system roots, Process.Start) through thin seams: an internal interface facade registered in DI, or `internal virtual` passthrough members. Mark the seams `[ExcludeFromCodeCoverage]` and keep logic out of them.** Tests that read the current time or the real machine depend on when and where they run, and code without a seam can't be unit tested. *Check:* Look for DateTime.Now/UtcNow, DateTimeOffset.UtcNow or TimeProvider.System in tests, direct clock, Guid or RNG calls, Environment.GetEnvironmentVariable, Process.Start or fixed paths in production logic, and seams that contain logic beyond passthrough. Identity ADR-0188; Identity ADR-0287; Identity csharp-production.instructions.md - **Prove constant-time or equal-time verification by recording that the hasher or verifier ran on the dummy path (with a fake or spy), never by measuring elapsed time.** Elapsed-time assertions are flaky when tests run in parallel. *Check:* Look for tests that use Stopwatch or duration thresholds to show resistance to timing attacks. Identity ADR-0239; Identity csharp-testing.instructions.md -- **Make concurrency tests deterministic with TaskCompletionSource gates, never sleeps or timing. To prove single-flight retrieval, start two calls against a gated stub, release the gate, assert a single request, and cancel the first caller to show the work runs on its own token. To prove parallelism, assert a second job finishes while the first waits. To prove a limit of one, assert the second doesn't finish until the first is released.** Gates make the overlap certain, so the test proves the behavior without guessing at timing. *Check:* Look for concurrency, deduplication or worker-limit tests that rely on Task.Delay, Thread.Sleep or timing, or that don't cover cancellation of the first caller. Identity csharp-testing.instructions.md -- **Assert the effect of a background job by polling until a deadline (for example 100 ms steps up to about 15 s), never with a fixed delay followed by one assertion.** Fixed delays are both flaky and slow. *Check:* Look for Task.Delay or Thread.Sleep followed by a single assertion on asynchronous work. Identity csharp-testing.instructions.md -- **Make a test handler that is invoked asynchronously or through reflection fail by returning ValueTask.FromException or Task.FromException, not by throwing synchronously.** Reflection wraps a synchronous throw in TargetInvocationException, so the code under test never sees the exception type an async handler would produce. *Check:* Look for test job or handler fakes that throw synchronously to simulate failure or cancellation. Identity csharp-testing.instructions.md +- **Make concurrency tests deterministic with TaskCompletionSource gates, never sleeps or timing. To prove single-flight retrieval, start two calls against a gated stub, release the gate, assert a single request, and cancel the first caller to show the work runs on its own token. To prove parallelism, assert a second job finishes while the first waits. To prove a limit of one, assert the second doesn't finish until the first is released.** Gates make the overlap certain, so the test proves the behavior without guessing at timing. *Check:* Look for concurrency, deduplication or worker-limit tests that rely on Task.Delay, Thread.Sleep or timing, or that don't cover cancellation of the first caller. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Assert the effect of a background job by polling until a deadline (for example 100 ms steps up to about 15 s), never with a fixed delay followed by one assertion.** Fixed delays are both flaky and slow. *Check:* Look for Task.Delay or Thread.Sleep followed by a single assertion on asynchronous work. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Make a test handler that is invoked asynchronously or through reflection fail by returning ValueTask.FromException or Task.FromException, not by throwing synchronously.** Reflection wraps a synchronous throw in TargetInvocationException, so the code under test never sees the exception type an async handler would produce. *Check:* Look for test job or handler fakes that throw synchronously to simulate failure or cancellation. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **When a test asserts on spans from a shared static ActivitySource, use an ActivityListener that collects only activities under a parent activity the test started itself. When it listens to a shared or framework meter, assert the property on every collected point and find the test's own point with Assert.Contains, never Assert.Single.** Every test in the process shares these sources and instruments, so unfiltered listeners and Assert.Single pick up measurements from tests running at the same time. *Check:* Look for ActivityListeners not filtered by the test's own parent activity or trace id, and for Assert.Single over measurements from a shared meter. Identity ADR-0298; Identity csharp-production.instructions.md; Identity csharp-testing.instructions.md - **Give every new or changed metric instrument a unit test that reads it through `MetricCollector` and asserts its name, unit and attributes, and update the metrics catalog document in the same change.** Dashboards and alerts are built on instruments as a reviewed contract, so adding or renaming one has to be deliberate. *Check:* Look for a new or renamed instrument or attribute with no MetricCollector test or no catalog update. Identity ADR-0197; Identity csharp-production.instructions.md - **Test that code emits a domain event by registering a recording event handler and asserting on the captured event and its envelope, not by mocking the publisher or reading log output. Give every new event such a test.** Recording sinks are hermetic, don't depend on background-delivery timing, and pin the event's contract. *Check:* Look for tests that mock the event publisher or assert on logs to confirm an event was emitted, and for new events with no recording-sink test. Identity ADR-0047 - **Verify self-implemented cryptography and protocol encoders or parsers against an independent source: prefer the specification's own test vectors, otherwise use byte-for-byte differential tests against an oracle known to conform, or fixtures produced by an independent reference tool with the generating commands kept in a comment. When the oracle library has both a platform-native and a managed path, use the managed, conforming path. Reference the oracle only from test projects, and never edit fixtures by hand.** A parser and a test builder that share a misreading of the spec both pass, and a non-conforming oracle once hid a defect behind a green suite. *Check:* Look for new or changed crypto or protocol code tested only with the repository's own builder, oracles that use platform-native paths (for example CNG/CngKey), fixtures with no generating commands, and oracle packages referenced from non-test projects. Identity ADR-0117; Identity ADR-0126; Identity csharp-testing.instructions.md - **Make a conformance test's expected values the same on every OS. Don't branch the expected result or the oracle on the platform, and don't restrict inputs to avoid known failures on some platforms.** A test whose expectation changes with the platform can't express a conformance requirement, and it keeps asserting known-broken behavior. *Check:* Look for OperatingSystem.IsWindows() or similar branches that pick different expected crypto results or oracles, and for assertions narrowed to a subset of inputs (for example P-256 only). Identity ADR-0126 -- **When the concrete exception type depends on the platform or provider, assert the documented base type with `Assert.ThrowsAny`. Keep `Assert.Throws` for cases where the exact type is the contract.** `Assert.Throws` requires an exact match and fails where the platform throws a subclass, for example OpenSSL's subclass of CryptographicException. *Check:* Look for `Assert.Throws` or another exception type that varies by platform, asserted exactly. Identity csharp-testing.instructions.md +- **When the concrete exception type depends on the platform or provider, assert the documented base type with `Assert.ThrowsAny`. Keep `Assert.Throws` for cases where the exact type is the contract.** `Assert.Throws` requires an exact match and fails where the platform throws a subclass, for example OpenSSL's subclass of CryptographicException. *Check:* Look for `Assert.Throws` or another exception type that varies by platform, asserted exactly. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **Make tests that use OS-dependent cryptographic primitives (for example MLDsa) do nothing when the primitive's IsSupported is false.** This keeps the tests passing on CI runners whose OS lacks the primitive. *Check:* Look for tests that use OS-dependent crypto types without checking IsSupported first, so they would throw PlatformNotSupportedException on some runners. Identity ADR-0118 - **Prove that TLS-level behavior such as presenting a client certificate reaches the wire with a real handshake against a server bound to loopback on an ephemeral port, not only with TestServer or mocks.** TestServer terminates no TLS. A loopback server on an ephemeral port stays hermetic while exercising the real handshake. *Check:* Look for TLS or mTLS features tested only through TestServer or mocks, and for test listeners on fixed ports or non-loopback addresses. Identity ADR-0288 - **Give every form-bound endpoint an integration test that posts the form without the antiforgery token and asserts both a 4xx response and that no state changed.** A missing token can surface as a 500 instead of a 400, and a test that checks only the status can miss a state change. *Check:* When a diff adds a form-posting endpoint, look for a test that sends no antiforgery token, expects a 4xx status, and checks that nothing changed. *Note:* In the source ADR, these tests also decide when the Request Delegate Generator may be turned back on for a project with form endpoints. Identity ADR-0262 -- **Read response security headers (Content-Security-Policy, X-Frame-Options and similar) from HttpResponseMessage.Headers, not Content.Headers, and also test a returning browser by sending back the cookies the first response set. Test headers that depend on the forwarded scheme with an http request carrying X-Forwarded-Proto through a host that trusts the proxy, paired with a control host that trusts no proxy.** Some headers are added only on later requests. A test over https alone still passes when the header is written before forwarded headers are processed. *Check:* Look for security-header assertions on Content.Headers, tests of only a first request without cookies, and HSTS or scheme-dependent header tests that use only https. Identity csharp-testing.instructions.md -- **Assert a URI the server must send exactly as written on Headers.Location.OriginalString, not on AbsoluteUri or ToString().** System.Uri normalizes the value (for example by adding a trailing slash), so the assertion passes while the server sends a different string. *Check:* Look for exact redirect assertions on Location.ToString() or AbsoluteUri. Identity csharp-testing.instructions.md +- **Read response security headers (Content-Security-Policy, X-Frame-Options and similar) from HttpResponseMessage.Headers, not Content.Headers, and also test a returning browser by sending back the cookies the first response set. Test headers that depend on the forwarded scheme with an http request carrying X-Forwarded-Proto through a host that trusts the proxy, paired with a control host that trusts no proxy.** Some headers are added only on later requests. A test over https alone still passes when the header is written before forwarded headers are processed. *Check:* Look for security-header assertions on Content.Headers, tests of only a first request without cookies, and HSTS or scheme-dependent header tests that use only https. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Assert a URI the server must send exactly as written on Headers.Location.OriginalString, not on AbsoluteUri or ToString().** System.Uri normalizes the value (for example by adding a trailing slash), so the assertion passes while the server sends a different string. *Check:* Look for exact redirect assertions on Location.ToString() or AbsoluteUri. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **In tests of published URLs or Host-spoofing resistance, configure an explicit issuer or public origin that differs from the request host.** Without a pinned issuer, the value derived from the request and the spoofed value are the same, so the test passes without proving anything. *Check:* Look for tests asserting discovery or endpoint URLs, or spoofed-Host behavior, that don't configure an explicit issuer different from the request host. Identity ADR-0129 -- **In OAuth/OIDC integration tests, follow the protocol rules the server enforces by default. Send an S256 PKCE challenge and redeem the code with the matching verifier. Write private-use redirect URIs as reverse domain names (for example com.example.app:/callback). In DPoP tests, compare cnf.jkt with an RFC 7638 thumbprint computed in the test, and get a fresh authorization code for each case expected to be refused.** Otherwise the test exercises a refusal instead of the flow: codes are bound to PKCE, arbitrary schemes are refused, and a code is used up before dpop_jkt is checked, so reusing one hides the check. *Check:* Look for authorization-code tests without code_challenge/code_verifier, success-path redirect URIs like myapp://callback, and DPoP tests that reuse a code or take the expected thumbprint from the server. Identity csharp-testing.instructions.md -- **Build test JWTs that need exact, missing or malformed time claims with the NCode.Jose `IJoseSerializer.CreateEncoder(...).Encode` path, not with a JWT encoder that enforces token lifetime.** A lifetime-enforcing encoder adds default iat/nbf/exp and overwrites a non-integer iat, so lifetime-validation tests wouldn't test what they claim. *Check:* Look for tests of absent, malformed or far-off time claims that build tokens through an IJwtEncoder. Identity csharp-testing.instructions.md -- **Build test certificate subjects with X500DistinguishedNameBuilder, choosing each value's string type explicitly.** Subject DNs are matched by RFC 4517 rules, not byte comparison, so the string type affects the result. *Check:* Look for certificate-matching tests that build subjects from raw DN strings. Identity csharp-testing.instructions.md +- **In OAuth/OIDC integration tests, follow the protocol rules the server enforces by default. Send an S256 PKCE challenge and redeem the code with the matching verifier. Write private-use redirect URIs as reverse domain names (for example com.example.app:/callback). In DPoP tests, compare cnf.jkt with an RFC 7638 thumbprint computed in the test, and get a fresh authorization code for each case expected to be refused.** Otherwise the test exercises a refusal instead of the flow: codes are bound to PKCE, arbitrary schemes are refused, and a code is used up before dpop_jkt is checked, so reusing one hides the check. *Check:* Look for authorization-code tests without code_challenge/code_verifier, success-path redirect URIs like myapp://callback, and DPoP tests that reuse a code or take the expected thumbprint from the server. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Build test JWTs that need exact, missing or malformed time claims with the NCode.Jose `IJoseSerializer.CreateEncoder(...).Encode` path, not with a JWT encoder that enforces token lifetime.** A lifetime-enforcing encoder adds default iat/nbf/exp and overwrites a non-integer iat, so lifetime-validation tests wouldn't test what they claim. *Check:* Look for tests of absent, malformed or far-off time claims that build tokens through an IJwtEncoder. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Build test certificate subjects with X500DistinguishedNameBuilder, choosing each value's string type explicitly.** Subject DNs are matched by RFC 4517 rules, not byte comparison, so the string type affects the result. *Check:* Look for certificate-matching tests that build subjects from raw DN strings. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **When server code parses or transforms frontend build output, add a test that runs against the real built artifact, skipped when an environment variable doesn't point to it, and run that test in the CI job that builds the frontend.** Hand-written fixtures and checked-in copies drift from what the build produces, so a breaking build change would still pass the .NET gate. *Check:* Look for code that consumes build output and is tested only against handcrafted HTML or a committed copy, with no gated test of the real artifact. Identity ADR-0254 - **Use `MockBehavior.Strict` mocks created from a `MockRepository(MockBehavior.Strict)` that the test base verifies on dispose. End every setup with `.Verifiable()`, name the lambda parameter `x`, and don't call `mock.Verify(...)` by hand. If a shared-fixture setup is used on only some paths, move it into the tests that use it rather than dropping `.Verifiable()`. Use Loose only through shared helpers, and only for partial mocks of abstract base classes or value-provider stubs the test never asserts on.** With strict, verified mocks, an unexpected call or an unmet setup fails the test automatically. *Check:* Look for `new Mock()` outside the repository, `MockBehavior.Loose` outside those two cases, setups without `.Verifiable()`, a lambda parameter other than `x`, manual Verify calls, and `.Verifiable()` removed from shared setups. Identity docs/backend-for-frontend.md; Identity csharp.instructions.md; Identity csharp-testing.instructions.md -- **Write true unit tests that mock external collaborators, but drive the composition through the real DI container with shared builders when the wiring itself (registration, dispatch, serialization) is under test.** The unit stays isolated, while wiring is tested end to end where the wiring is the unit. *Check:* Look for unit tests that use real external collaborators, tests that mock the DI wiring under test, and tests that rebuild the container each time without a shared builder. Identity csharp-testing.instructions.md -- **To prove that composition or options validation constructs no instance of a service, register a factory for that service that throws, then trigger the code path.** The throwing registration fails the test the moment the service is constructed. *Check:* Look for tests that claim lazy construction or no construction but register no throwing factory. Identity csharp-testing.instructions.md -- **Make a test class that owns a real service provider with `IAsyncDisposable`-only services implement `IAsyncLifetime` and await `DisposeAsync()`.** A synchronous `ServiceProvider.Dispose()` throws InvalidOperationException when a service implements only IAsyncDisposable. *Check:* Look for test classes that build a real container and dispose it through IDisposable. Identity csharp-testing.instructions.md -- **Test read-modify-write and retry logic against realistic store behavior. Capture the update delegate passed to the store mock and invoke it with the stored state the scenario means (such as one a concurrent writer changed). In retry loops, return a new object on each read (for example SetupSequence(...).ReturnsAsync(first).ReturnsAsync(second)).** Checking only that the store was called never exercises the merge logic, and one shared instance lets the second read see the first attempt's own write. *Check:* Look for read-modify-write tests that never invoke the captured update function, and for retry-loop mocks that return the same instance on every read. Identity csharp-testing.instructions.md -- **Exercise code that calls a collaborator member taking `Span`/`ReadOnlySpan` through an integration test against the real collaborator, rather than changing the signature to make it mockable. Compute expected setup arguments in a local outside the `Setup` expression.** Moq can't match span arguments, and expression trees can't contain ref structs or expanded `params ReadOnlySpan` calls. *Check:* Look for production signatures changed from span to array only for mocking, and for complex value construction inside a `Setup(x => ...)` lambda. Identity csharp-testing.instructions.md -- **Match a `JsonElement` mock argument by its content through a static helper called in `It.Is`, and match collection arguments by sequence equality, not by reference.** `JsonElement` equality compares document references, matcher expression trees can't contain `out _`, and collections passed by reference rarely match the expected instance. *Check:* Look for a JsonElement or collection matched by equality or by a specific instance in a setup. Identity csharp-testing.instructions.md -- **When code enumerates a mocked collection with `OfType()` or `Cast()`, also set up the non-generic `IEnumerable.GetEnumerator()` through `mock.As()`.** These operators call the non-generic enumerator, so a strict mock with only the generic setup throws. *Check:* Look for collection mocks used through OfType or Cast that have only a generic GetEnumerator setup. Identity csharp-testing.instructions.md -- **When mocking a disposable abstract type whose non-virtual DisposeAsync calls a protected DisposeAsyncCore, set up and verify DisposeAsyncCore through Moq's `Protected()` API.** DisposeAsync can't be intercepted, so without this a strict mock fails or disposal goes unverified. *Check:* Look for strict mocks of disposable abstract types that the code under test disposes, with no `Protected().Setup("DisposeAsyncCore")`. Identity csharp-testing.instructions.md -- **Use a loose mock for a source-generated `ILogger` unless the log entry is the behavior under test. In that case, use a strict mock with `IsEnabled` returning true and a verifiable `Log` setup that uses `It.IsAnyType`.** `[LoggerMessage]` methods check IsEnabled first, so a loose mock never logs and needs no setup. *Check:* Look for strict logger mocks with setups they don't need, and for tests that claim something is logged without a verifiable Log setup. Identity csharp-testing.instructions.md -- **Use Moq's own mechanisms to steer strict mocks. Capture an intermediate command in the setup's Callback and return `ValueTask.FromException(...)` to stop the unit there. Run a callback argument inside `.Returns` through a static async helper, not an async lambda. Fail one specific call by adding a narrower `.Throws` setup after the general one. Drive events with `SetupAdd(...).Callback`, and add `SetupRemove` when the unit detaches.** These keep strict tests deterministic and small, without mocking the whole downstream flow, call counters or loose mocks. *Check:* Look for tests that mock an entire downstream flow just to inspect one command, async lambdas in `.Returns` for ValueTask members, call-counter workarounds, and event subscriptions on strict mocks without SetupAdd or SetupRemove. Identity csharp-testing.instructions.md -- **Make a test's change-token producer swap in a new CancellationTokenSource before cancelling the old one, so it returns a fresh token after each change.** `ChangeToken.OnChange` asks for a new token after each callback, so returning the cancelled token again makes it fire forever. *Check:* Look for test producers that return the same token after cancelling it. Identity csharp-testing.instructions.md -- **Test a component that consumes a catalog of data with the test's own mocked entries, and test the built-in data only in its data source's own tests.** This keeps the unit isolated from changes to the shipped data. *Check:* Look for component tests that depend on a built-in default table. Identity csharp-testing.instructions.md -- **Test database migration rules with synthetic `private sealed` Migration subclasses that need no database, plus a theory over the host's real migrations that shows each one is classified.** The rule stays hermetic while the real migrations stay covered. *Check:* Look for migration-rule tests that need a database, or that leave the real migrations uncovered. Identity csharp-testing.instructions.md +- **Write true unit tests that mock external collaborators, but drive the composition through the real DI container with shared builders when the wiring itself (registration, dispatch, serialization) is under test.** The unit stays isolated, while wiring is tested end to end where the wiring is the unit. *Check:* Look for unit tests that use real external collaborators, tests that mock the DI wiring under test, and tests that rebuild the container each time without a shared builder. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **To prove that composition or options validation constructs no instance of a service, register a factory for that service that throws, then trigger the code path.** The throwing registration fails the test the moment the service is constructed. *Check:* Look for tests that claim lazy construction or no construction but register no throwing factory. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Make a test class that owns a real service provider with `IAsyncDisposable`-only services implement `IAsyncLifetime` and await `DisposeAsync()`.** A synchronous `ServiceProvider.Dispose()` throws InvalidOperationException when a service implements only IAsyncDisposable. *Check:* Look for test classes that build a real container and dispose it through IDisposable. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Test read-modify-write and retry logic against realistic store behavior. Capture the update delegate passed to the store mock and invoke it with the stored state the scenario means (such as one a concurrent writer changed). In retry loops, return a new object on each read (for example SetupSequence(...).ReturnsAsync(first).ReturnsAsync(second)).** Checking only that the store was called never exercises the merge logic, and one shared instance lets the second read see the first attempt's own write. *Check:* Look for read-modify-write tests that never invoke the captured update function, and for retry-loop mocks that return the same instance on every read. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Exercise code that calls a collaborator member taking `Span`/`ReadOnlySpan` through an integration test against the real collaborator, rather than changing the signature to make it mockable. Compute expected setup arguments in a local outside the `Setup` expression.** Moq can't match span arguments, and expression trees can't contain ref structs or expanded `params ReadOnlySpan` calls. *Check:* Look for production signatures changed from span to array only for mocking, and for complex value construction inside a `Setup(x => ...)` lambda. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Match a `JsonElement` mock argument by its content through a static helper called in `It.Is`, and match collection arguments by sequence equality, not by reference.** `JsonElement` equality compares document references, matcher expression trees can't contain `out _`, and collections passed by reference rarely match the expected instance. *Check:* Look for a JsonElement or collection matched by equality or by a specific instance in a setup. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **When code enumerates a mocked collection with `OfType()` or `Cast()`, also set up the non-generic `IEnumerable.GetEnumerator()` through `mock.As()`.** These operators call the non-generic enumerator, so a strict mock with only the generic setup throws. *Check:* Look for collection mocks used through OfType or Cast that have only a generic GetEnumerator setup. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **When mocking a disposable abstract type whose non-virtual DisposeAsync calls a protected DisposeAsyncCore, set up and verify DisposeAsyncCore through Moq's `Protected()` API.** DisposeAsync can't be intercepted, so without this a strict mock fails or disposal goes unverified. *Check:* Look for strict mocks of disposable abstract types that the code under test disposes, with no `Protected().Setup("DisposeAsyncCore")`. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Use a loose mock for a source-generated `ILogger` unless the log entry is the behavior under test. In that case, use a strict mock with `IsEnabled` returning true and a verifiable `Log` setup that uses `It.IsAnyType`.** `[LoggerMessage]` methods check IsEnabled first, so a loose mock never logs and needs no setup. *Check:* Look for strict logger mocks with setups they don't need, and for tests that claim something is logged without a verifiable Log setup. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Use Moq's own mechanisms to steer strict mocks. Capture an intermediate command in the setup's Callback and return `ValueTask.FromException(...)` to stop the unit there. Run a callback argument inside `.Returns` through a static async helper, not an async lambda. Fail one specific call by adding a narrower `.Throws` setup after the general one. Drive events with `SetupAdd(...).Callback`, and add `SetupRemove` when the unit detaches.** These keep strict tests deterministic and small, without mocking the whole downstream flow, call counters or loose mocks. *Check:* Look for tests that mock an entire downstream flow just to inspect one command, async lambdas in `.Returns` for ValueTask members, call-counter workarounds, and event subscriptions on strict mocks without SetupAdd or SetupRemove. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Make a test's change-token producer swap in a new CancellationTokenSource before cancelling the old one, so it returns a fresh token after each change.** `ChangeToken.OnChange` asks for a new token after each callback, so returning the cancelled token again makes it fire forever. *Check:* Look for test producers that return the same token after cancelling it. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Test a component that consumes a catalog of data with the test's own mocked entries, and test the built-in data only in its data source's own tests.** This keeps the unit isolated from changes to the shipped data. *Check:* Look for component tests that depend on a built-in default table. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Test database migration rules with synthetic `private sealed` Migration subclasses that need no database, plus a theory over the host's real migrations that shows each one is classified.** The rule stays hermetic while the real migrations stay covered. *Check:* Look for migration-rule tests that need a database, or that leave the real migrations uncovered. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md - **Test units through their public surface and never widen it for tests. Expose internals with InternalsVisibleTo granted only to the assembly's own *.Tests project (with the correct name) and to DynamicProxyGenAssembly2 so Moq can proxy internal types. Delete private branches tests can't reach, or extract complex private logic into a collaborator.** Private methods aren't seams, unreachable branches are dead code, and any other InternalsVisibleTo target creates a hidden dependency across the package boundary. *Check:* Look for members made public only for tests, reflection used to call private members, InternalsVisibleTo targets other than the matching *.Tests assembly or DynamicProxyGenAssembly2, wrong test assembly names, and tested internal types without the DynamicProxyGenAssembly2 grant. Identity ADR-0004; Identity csharp.instructions.md; Identity csharp-production.instructions.md - **Cover every type and member with tests or mark it `[ExcludeFromCodeCoverage]`, and exclude only thin passthroughs, DI wiring and framework adapters. Keep hand-written shipped code, including host and deployment projects, in coverage scope.** Code with behavior must be tested, and excluding shipped code only to raise the figure hides real gaps. *Check:* Look for new untested behavioral code, `[ExcludeFromCodeCoverage]` on code with logic, and new assembly filters or file globs aimed at hand-written production code. Identity ADR-0185; Identity csharp-production.instructions.md - **Exclude generated code from coverage where coverage is collected (coverlet runsettings `ExcludeByFile`/`ExcludeByAttribute`), not in ReportGenerator filters or a `codecov.yml` `ignore` section, and never exclude `[CompilerGenerated]`.** A single collection-time list feeds every report, while separate lists in different syntaxes drift. The compiler puts `[CompilerGenerated]` on async state machines, lambdas and iterators, so excluding it drops hand-written code. *Check:* Look for exclusions added to ReportGenerator `-filefilters`/`-classfilters` or `codecov.yml` `ignore`, and for CompilerGeneratedAttribute in any exclusion list. Identity ADR-0185 @@ -71,9 +71,9 @@ applyTo: "**/*Tests/**/*.cs, **/*.Tests/**/*.cs, **/*Tests/**/*.csproj, **/*.cs, - **Write test results and coverage to a fixed output directory that is cleared before each run, and don't merge results with recursive globs over per-project `TestResults/` folders.** Results that build up across runs blend every past run into the reported coverage. *Check:* Look for merges over `**/coverage.cobertura.xml` or `TestResults/`, and for output directories that aren't cleared before tests run. Identity ADR-0143 - **Present CI test and coverage results through job summaries (`$GITHUB_STEP_SUMMARY`) and `::error` annotations, not check-run or PR-comment actions that need `checks: write` or `pull-requests: write`.** Pull requests from forks don't get those write permissions, while summaries and annotations need none. *Check:* Look for actions such as `dorny/test-reporter` or `publish-unit-test-result-action`, and for new write permissions added for reporting. Identity ADR-0143 - **Skip coverage uploads on `merge_group` runs, and set `parsers.cobertura.partials_as_hits: true` in codecov.yml.** The push to main already uploads coverage for the same merged result. Without the setting, Codecov counts partially covered lines as misses and disagrees with ReportGenerator. *Check:* Look for upload steps without an `if: github.event_name != 'merge_group'` guard, and for codecov.yml files that leave out or disable `partials_as_hits`. Identity ADR-0185; Identity ADR-0263 -- **Don't set `TreatWarningsAsErrors=false` or a project-level `NoWarn` in test projects. Relax a noisy rule in the `.editorconfig` tests block with a one-line justification, or with a tight `#pragma warning disable/restore` that gives a reason. Fix nullable warnings properly instead of using `!`: give a `[MaybeNullWhen(false)]` out argument a nullable local, and narrow results with `Assert.NotNull`.** Analyzer warnings, such as those from xunit.analyzers, report real defects in tests, and a real fix is better than a suppression. *Check:* Look for TreatWarningsAsErrors set to false or NoWarn in a test csproj, suppressions with no justification, pragmas with no reason or no restore, and `!`/`null!` used to silence nullable warnings in tests. Identity csharp-testing.instructions.md -- **Use precise xUnit assertions: `Assert.Empty` for a count of 0, `Assert.Single` for a count of 1, and `Assert.Equal(expected, actual)` with an explicit element type when comparing an `ImmutableArray`.** Size-specific assertions give clearer failures. `ImmutableArray` equality compares the backing array by reference, so a collection-expression expected value fails even when the elements match. *Check:* Look for `Assert.Equal(0|1, x.Count/Length/Count())`, and `Assert.Equal([..], immutableArray)` without the element type argument. Identity csharp.instructions.md; Identity csharp-testing.instructions.md -- **Test one behavior per test, separate the sections with blank lines instead of `// Arrange`/`// Act`/`// Assert` comments, and apply the production code style in tests (collection expressions, `const` literals, UTF-8 literals like `"x"u8.ToArray()`). Literal arrays passed straight to assertion helpers may stay inline.** This keeps tests short, focused and consistent with production code. *Check:* Look for Arrange/Act/Assert comments, tests that assert several unrelated behaviors, `new byte[] { ... }` where a collection expression fits, and `Encoding.UTF8.GetBytes("literal")`. Identity csharp.instructions.md; Identity csharp-testing.instructions.md +- **Don't set `TreatWarningsAsErrors=false` or a project-level `NoWarn` in test projects. Relax a noisy rule in the `.editorconfig` tests block with a one-line justification, or with a tight `#pragma warning disable/restore` that gives a reason. Fix nullable warnings properly instead of using `!`: give a `[MaybeNullWhen(false)]` out argument a nullable local, and narrow results with `Assert.NotNull`.** Analyzer warnings, such as those from xunit.analyzers, report real defects in tests, and a real fix is better than a suppression. *Check:* Look for TreatWarningsAsErrors set to false or NoWarn in a test csproj, suppressions with no justification, pragmas with no reason or no restore, and `!`/`null!` used to silence nullable warnings in tests. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Use precise xUnit assertions: `Assert.Empty` for a count of 0, `Assert.Single` for a count of 1, and `Assert.Equal(expected, actual)` with an explicit element type when comparing an `ImmutableArray`.** Size-specific assertions give clearer failures. `ImmutableArray` equality compares the backing array by reference, so a collection-expression expected value fails even when the elements match. *Check:* Look for `Assert.Equal(0|1, x.Count/Length/Count())`, and `Assert.Equal([..], immutableArray)` without the element type argument. *Applies to:* `**/*.cs` Identity csharp.instructions.md; Identity csharp-testing.instructions.md +- **Test one behavior per test, separate the sections with blank lines instead of `// Arrange`/`// Act`/`// Assert` comments, and apply the production code style in tests (collection expressions, `const` literals, UTF-8 literals like `"x"u8.ToArray()`). Literal arrays passed straight to assertion helpers may stay inline.** This keeps tests short, focused and consistent with production code. *Check:* Look for Arrange/Act/Assert comments, tests that assert several unrelated behaviors, `new byte[] { ... }` where a collection expression fits, and `Encoding.UTF8.GetBytes("literal")`. *Applies to:* `**/*.cs` Identity csharp.instructions.md; Identity csharp-testing.instructions.md - **Mirror the production folder and namespace in test files, adding a `.Tests` namespace suffix and a `Tests` class suffix. Name test methods `Method_State_Expected`, and group tests in `#region` blocks named `Constructor Tests`, `{MethodName} Tests`, `{PropertyName} Property Tests` or `Generic Type Tests`.** Consistent placement and naming make the tests for any member easy to find, and each name records the member, the condition and the expected outcome. *Check:* Look for test files whose path or namespace doesn't match the production type, test names like `TestEncode` or `EncodeWorks`, and tests outside a region or in regions with other names. Identity docs/backend-for-frontend.md; Identity csharp.instructions.md; Identity csharp-testing.instructions.md - **Add a conformance expected-failure or expected-skip baseline entry only for a documented, intentional deviation, with a comment explaining why and a link to an issue or ADR.** The baseline exists to keep the gate green for known non-support. Unexplained entries hide regressions. *Check:* Look for new baseline entries with no comment, a vague comment, or no issue or ADR link, and for entries added to silence a failure that should be fixed. Identity docs/conformance-testing.md - **Set the stable ids and names that browser automation depends on (for example username, password, login, consent-allow, consent-deny) explicitly on login and consent elements, define them in one place, and never rely on ids a UI library generates.** Conformance automation finds fields by these ids. Generated ids aren't stable, and renaming one without updating the automation config silently breaks it. *Check:* Look for login or consent markup where these ids are missing, generated, renamed or duplicated without a matching update to the automation browser config. Identity ADR-0062; Identity ADR-0065 diff --git a/guidance/text-and-encoding.md b/guidance/text-and-encoding.md index 5a998ab..1fc823f 100644 --- a/guidance/text-and-encoding.md +++ b/guidance/text-and-encoding.md @@ -24,6 +24,6 @@ applyTo: "**/*.cs, **/*.ts, **/*.tsx, **/Migrations/**/*.cs, **/*Tests/**/*.cs, - **Format non-string values embedded in validation, problem or diagnostic messages with the invariant culture, for example with `string.Create(CultureInfo.InvariantCulture, ...)` or `IFormattable.ToString(null, CultureInfo.InvariantCulture)`.** Messages name values such as `TimeSpan`s, numbers and dates, and they have to read the same whatever culture the server runs in. *Check:* Look for string interpolation, `ToString()` or `string.Format` without an `IFormatProvider` that puts non-string values into validation or problem messages. Identity ADR-0240 - **Create a `string` only for a non-secret value at a storage or wire boundary (such as an envelope in a text column, a wrapped key or a KMS encryption context), and build it once, in place, with `string.Create`.** Building the string in place avoids intermediate allocations. Limiting strings to boundaries keeps secret material out of immutable managed strings that cannot be wiped. *Check:* Flag concatenation, `StringBuilder` or intermediate `ToString` calls used to build encoded envelope or key text, and any string that holds secret material. Identity ADR-0235 - **Write constant bytes that are valid UTF-8 text as `u8` literals (`ReadOnlySpan`), not with `Encoding.UTF8.GetBytes("...")` or hand-written byte arrays. Keep explicit byte arrays only for invalid UTF-8 or error-condition tests.** `u8` literals are compile-time constants that do not allocate and are less error-prone than encoding at runtime or spelling out bytes. *Check:* Look for `Encoding.UTF8.GetBytes("literal")` stored in a field or computed per call, and for hand-written byte arrays that spell valid UTF-8 text. Identity ADR-0116; Identity csharp.instructions.md -- **In tests, write strings that are not NFC or are otherwise sensitive to normalization with Unicode escapes (for example `"café"`), never as pasted characters.** An editor can normalize pasted characters and silently change the test input. *Check:* Look for normalization tests whose string literals contain combining or composed characters directly instead of escapes. Identity csharp-testing.instructions.md -- **Remove the UTF-8 BOM from generated EF Core migration files, and revert any model snapshot whose only change is a BOM.** Repository files are BOM-less, but `dotnet ef migrations add` writes BOMs and re-saves the snapshot even when nothing changed. *Check:* Look for new migration files that start with a BOM, and snapshot diffs whose only change is the BOM. Identity csharp-production.instructions.md +- **In tests, write strings that are not NFC or are otherwise sensitive to normalization with Unicode escapes (for example `"café"`), never as pasted characters.** An editor can normalize pasted characters and silently change the test input. *Check:* Look for normalization tests whose string literals contain combining or composed characters directly instead of escapes. *Applies to:* `**/*.cs` Identity csharp-testing.instructions.md +- **Remove the UTF-8 BOM from generated EF Core migration files, and revert any model snapshot whose only change is a BOM.** Repository files are BOM-less, but `dotnet ef migrations add` writes BOMs and re-saves the snapshot even when nothing changed. *Check:* Look for new migration files that start with a BOM, and snapshot diffs whose only change is the BOM. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md - **Save text files as UTF-8 without a BOM and with LF line endings.** This is the shared encoding convention (`charset = utf-8` in `.editorconfig`). BOMs and CRLF cause noisy diffs. *Check:* Flag changed files that start with a UTF-8 BOM or contain CRLF line endings. Identity project-template.instructions.md From 00418a45bdd0e5f0a4a907575f38c042051b0454 Mon Sep 17 00:00:00 2001 From: Marcin Polewski Date: Fri, 30 Oct 2026 22:55:45 -0600 Subject: [PATCH 3/4] Test per-part rule scoping and extraction reuse, and keep the bullet format in one module review() builds its reviewer calls with reviewJobs and names its outcomes with outcomeNames, and extract() decides reuse with reuseExtraction, so tests cover them without a model: each part gets only its files' rules, an emptied rule batch doesn't run or report, and a changed or removed applyTo is saved without re-extracting. Also tests multi-glob tags, a topic with no bullets left, and retag refusing a malformed bullet. formatBullet, parseBullet and bulletGlobs move to src/guidance.ts and share one pattern for the Applies to field, so training and the engine can't drift apart on the layout. --- src/context.ts | 3 +- src/gather.ts | 10 +------ src/guidance.ts | 32 ++++++++++++++++++++ src/review.ts | 66 +++++++++++++++++++++++------------------ test/engine.test.ts | 72 ++++++++++++++++++++++++++++++++++++++++++++- training/run.ts | 43 ++++++++++++--------------- 6 files changed, 162 insertions(+), 64 deletions(-) create mode 100644 src/guidance.ts diff --git a/src/context.ts b/src/context.ts index ff6eff0..5db6c00 100644 --- a/src/context.ts +++ b/src/context.ts @@ -6,7 +6,8 @@ import { existsSync, readdirSync, readFileSync } from 'node:fs'; import path from 'node:path'; import { runAgent, type AgentRun, type AgentResult } from './agent.ts'; -import { bulletGlobs, matchesAnyFile, type Gathered, type RuleFile } from './gather.ts'; +import { matchesAnyFile, type Gathered, type RuleFile } from './gather.ts'; +import { bulletGlobs } from './guidance.ts'; export interface IndexedRule { id: string; diff --git a/src/gather.ts b/src/gather.ts index 45f9027..292e850 100644 --- a/src/gather.ts +++ b/src/gather.ts @@ -5,6 +5,7 @@ import { existsSync, readdirSync, readFileSync } from 'node:fs'; import path from 'node:path'; import { maxChunkChars, maxChunks, maxConventionChars, maxPullRequestTextChars } from './config.ts'; import { readDecisions, readProposedDecisions, type Decision } from './context.ts'; +import { bulletGlobs } from './guidance.ts'; export { maxIssueChars } from './config.ts'; export interface PullRequest { @@ -204,15 +205,6 @@ export function applyToGlobs(text: string): string[] { return (frontmatter(text).fields.get('applyTo') ?? '').split(',').map((g) => g.trim()).filter(Boolean); } -// A learned guidance bullet names the files it applies to in an "*Applies to:*" field, from the instruction files it -// was learned from, such as "*Applies to:* `**/*.cs`". An empty list means the bullet applies to every file. -export const appliesToLabel = '*Applies to:*'; - -export function bulletGlobs(line: string): string[] { - const field = / \*Applies to:\* (.*?)(?: .*<\/sub>)?$/.exec(line)?.[1] ?? ''; - return [...field.matchAll(/`([^`]+)`/g)].map((m) => m[1]!); -} - // Drops the bullets tagged for files this pull request doesn't change. Untagged bullets and other lines stay. export function keepBulletsFor(text: string, changed: string[]): string { return text diff --git a/src/guidance.ts b/src/guidance.ts new file mode 100644 index 0000000..16bf1cc --- /dev/null +++ b/src/guidance.ts @@ -0,0 +1,32 @@ +// The layout of one learned guidance bullet, in guidance/*.md: training writes it, and the engine reads its tag. +// +// - **statement** why *Check:* check *Note:* note *Applies to:* `glob`, `glob` source; source +// +// The "*Applies to:*" field names the files the bullet applies to, from the instruction files it was learned from. +// A bullet without it applies to every file. + +export const appliesToLabel = '*Applies to:*'; + +export interface GuidanceRule { statement: string; why: string; check: string; sources: string[]; note?: string } + +const appliesTo = String.raw` \*Applies to:\* ((?:\`[^\`]+\`(?:, )?)+)`; +const bullet = new RegExp(String.raw`^- \*\*(.+?)\*\* (.*?) \*Check:\* (.*?)(?: \*Note:\* (.*?))?(?:${appliesTo})? (.*)<\/sub>$`); +const tag = new RegExp(`${appliesTo}(?: .*)?$`); + +export function formatBullet(rule: GuidanceRule, globs: string[]): string { + const scope = globs.length ? ` ${appliesToLabel} ${globs.map((g) => `\`${g}\``).join(', ')}` : ''; + return `- **${rule.statement}** ${rule.why} *Check:* ${rule.check}${rule.note ? ` *Note:* ${rule.note}` : ''}${scope} ${rule.sources.join('; ')}`; +} + +export function parseBullet(line: string): GuidanceRule | undefined { + const match = bullet.exec(line); + if (!match) return undefined; + const [, statement, why, check, note, , sources] = match; + return { statement: statement!, why: why!, check: check!, ...(note ? { note } : {}), sources: sources!.split('; ') }; +} + +// The globs a bullet's "*Applies to:*" field lists; none when it has no such field. +export function bulletGlobs(line: string): string[] { + const field = tag.exec(line)?.[1] ?? ''; + return [...field.matchAll(/`([^`]+)`/g)].map((m) => m[1]!); +} diff --git a/src/review.ts b/src/review.ts index 8291644..64873e5 100644 --- a/src/review.ts +++ b/src/review.ts @@ -123,6 +123,38 @@ export function partFiles(files: ChangedFile[], diff: string): string[] { return [...splitDiff(diff).keys(), ...files.filter((f) => !f.generated && !f.shown).map((f) => f.path)]; } +export interface ReviewJob { name: string; part: number; label: string; tier: ModelTier; rules: IndexedRule[] } + +// The reviewer calls for each part of the diff: every selected reviewer with its picked rules, and each rule batch. +// A rule learned for some files reaches only the parts that change them, and a batch left with no rules for a part +// doesn't run on it. +export function reviewJobs( + parts: string[], + files: ChangedFile[], + selected: string[], + rulesFor: Map, + batches: IndexedRule[][], + tiers: Record, +): ReviewJob[] { + return parts.flatMap((diff, part) => { + const shown = partFiles(files, diff); + const scoped = (rules: IndexedRule[] | undefined) => rulesForFiles(rules ?? [], shown); + return [ + ...selected.map((reviewer) => ({ name: reviewer, part, label: '', tier: tiers[reviewer] ?? ('haiku' as ModelTier), rules: scoped(rulesFor.get(reviewer)) })), + ...batches.flatMap((batch, i) => { + const rules = scoped(batch); + const label = `rules ${i * rulesPerCheck + 1}-${i * rulesPerCheck + batch.length}`; + return rules.length ? [{ name: ruleCheckName, part, label, tier: 'sonnet' as ModelTier, rules }] : []; + }), + ]; + }); +} + +// The reviewers that get an outcome: every selected one, and the rule check when any of its calls is planned. +export function outcomeNames(selected: string[], jobs: Pick[]): string[] { + return [...selected, ...(jobs.some((job) => job.name === ruleCheckName) ? [ruleCheckName] : [])]; +} + export function loadCharters(root: string): Charters { const reviewers = new Map(); for (const name of readdirSync(path.join(root, 'reviewers')).filter((n) => n.endsWith('.md')).sort()) { @@ -320,36 +352,14 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte // learned for some files reaches only the parts that change them. The part of the prompt they share is its first // block; one reviewer per model and part goes first and caches it, and the rest read the cache instead of each // writing it again. - const tiers = reviewerTiers(); const parts = gathered.chunks.length ? gathered.chunks : [gathered.diff]; const reviewRun: AgentRun = { ...run, workdir: mkdtempSync(path.join(tmpdir(), 'code-review-work-')) }; const batches = ruleChecksOn() ? ruleBatches(catalog, context.rulesFor, rulesPerCheck) : []; - const jobs = parts.flatMap((diff, part) => { - const sharedBlock = `${sharedContext(gathered, context.decisions, diff, part, parts.length)}\n\nThe checkout is at: ${run.checkout}`; - const files = partFiles(gathered.files, diff); - const scoped = (rules: IndexedRule[] | undefined) => rulesForFiles(rules ?? [], files); - return [ - ...[...selected].map((reviewer) => ({ - name: reviewer, - part, - label: '', - tier: tiers[reviewer] ?? ('haiku' as ModelTier), - prompt: [sharedBlock, `${rulesBlock(scoped(context.rulesFor.get(reviewer))).trimStart()}\n\nYour charter:\n${charters.reviewers.get(reviewer)!}`], - })), - ...batches.flatMap((batch, i) => { - const rules = scoped(batch); - return rules.length - ? [{ - name: ruleCheckName, - part, - label: `rules ${i * rulesPerCheck + 1}-${i * rulesPerCheck + batch.length}`, - tier: 'sonnet' as ModelTier, - prompt: [sharedBlock, `${rulesBlock(rules).trimStart()}\n\nYour charter:\n${charters.ruleCheckPrompt}`], - }] - : []; - }), - ]; - }); + const sharedBlocks = parts.map((diff, part) => `${sharedContext(gathered, context.decisions, diff, part, parts.length)}\n\nThe checkout is at: ${run.checkout}`); + const jobs = reviewJobs(parts, gathered.files, [...selected], context.rulesFor, batches, reviewerTiers()).map((job) => ({ + ...job, + prompt: [sharedBlocks[job.part]!, `${rulesBlock(job.rules).trimStart()}\n\nYour charter:\n${job.name === ruleCheckName ? charters.ruleCheckPrompt : charters.reviewers.get(job.name)!}`], + })); const first = jobs.filter((job, i) => jobs.findIndex((other) => other.tier === job.tier && other.part === job.part) === i); const reviewOne = async ({ name, part, label, tier, prompt }: (typeof jobs)[number]): Promise => { const result = await runAgent<{ findings: Finding[] }>(reviewRun, { @@ -374,7 +384,7 @@ export async function review(run: AgentRun, gathered: Gathered, charters: Charte ...(await mapWithConcurrency(jobs.filter((job) => !first.includes(job)), concurrency, reviewOne)), ]; // One outcome per reviewer across the parts and rule batches: it ran if any of its calls ran. - const outcomes: ReviewerOutcome[] = [...selected, ...(jobs.some((job) => job.name === ruleCheckName) ? [ruleCheckName] : [])].map((name) => { + const outcomes: ReviewerOutcome[] = outcomeNames([...selected], jobs).map((name) => { const mine = perJob.filter((o) => o.name === name); const errors = mine.flatMap((o) => (o.error ? [o.error] : [])); return { diff --git a/test/engine.test.ts b/test/engine.test.ts index 69c00f7..3439036 100644 --- a/test/engine.test.ts +++ b/test/engine.test.ts @@ -180,8 +180,54 @@ test('each part of a large diff gets only the guidance rules tagged for its file assert.deepEqual(rulesForFiles([csharp, anywhere], partFiles(files, tsPart)).map((r) => r.id), ['G-a-1', 'G-a-2']); }); +test('a guidance bullet tagged with several globs reaches a review that changes a file any of them matches', async () => { + const { readGuidance } = await import('../src/gather.ts'); + const { guidanceRules } = await import('../src/context.ts'); + const dir = mkdtempSync(path.join(tmpdir(), 'guidance-')); + writeFileSync(path.join(dir, 'public-api.md'), [ + '---', 'description: "x"', 'applyTo: "**"', '---', '', '# Public API', '', + '- **Pin package versions.** Why. *Check:* Look. *Applies to:* `**/*.cs`, `**/*.csproj` Identity csharp-production.instructions.md', + '', + ].join('\n')); + const statements = (changed: string[]) => guidanceRules(readGuidance(dir, changed)).map((r) => r.summary); + assert.deepEqual(statements(['src/Identity.csproj']), ['Pin package versions.']); + assert.deepEqual(statements(['src/Comments.cs']), ['Pin package versions.']); + // A topic whose bullets are all tagged for other files is left out, rather than given to the reviewer as a bare header. + assert.deepEqual(readGuidance(dir, ['src/comments.ts']), []); +}); + +test('review jobs give each part of the diff only the rules tagged for its files, and skip a rule batch left empty', async () => { + const { outcomeNames, reviewJobs, ruleCheckName } = await import('../src/review.ts'); + const csharp = { id: 'G-a-1', reviewers: ['conventions'], summary: 'cs', text: 'cs', globs: ['**/*.cs'] }; + const anywhere = { id: 'G-a-2', reviewers: ['conventions'], summary: 'any', text: 'any' }; + const files: ChangedFile[] = [ + { path: 'src/a.ts', additions: 1, deletions: 0, generated: false, shown: true }, + { path: 'src/A.cs', additions: 1, deletions: 0, generated: false, shown: true }, + ]; + const tsPart = 'diff --git a/src/a.ts b/src/a.ts\n--- a/src/a.ts\n+++ b/src/a.ts\n@@ -1 +1 @@\n+x\n'; + const csPart = 'diff --git a/src/A.cs b/src/A.cs\n--- a/src/A.cs\n+++ b/src/A.cs\n@@ -1 +1 @@\n+x\n'; + const rulesFor = new Map([['conventions', [csharp, anywhere]]]); + const jobs = reviewJobs([tsPart, csPart], files, ['conventions', 'tests'], rulesFor, [[csharp], [anywhere]], { conventions: 'sonnet' }); + const plan = jobs.map((job) => `${job.part} ${job.name} ${job.tier}: ${job.rules.map((r) => r.id).join(',')}`); + assert.deepEqual(plan, [ + '0 conventions sonnet: G-a-2', + '0 tests haiku: ', + `0 ${ruleCheckName} sonnet: G-a-2`, + '1 conventions sonnet: G-a-1,G-a-2', + '1 tests haiku: ', + `1 ${ruleCheckName} sonnet: G-a-1`, + `1 ${ruleCheckName} sonnet: G-a-2`, + ]); + assert.deepEqual(outcomeNames(['conventions'], jobs), ['conventions', ruleCheckName]); + // A pull request that changes no C# file runs no rule check for a batch of C# rules, and reports none. + const tsOnly = reviewJobs([tsPart], files.slice(0, 1), ['conventions'], rulesFor, [[csharp]], {}); + assert.deepEqual(tsOnly.map((job) => `${job.name}: ${job.rules.map((r) => r.id).join(',')}`), ['conventions: G-a-2']); + assert.deepEqual(outcomeNames(['conventions'], tsOnly), ['conventions']); +}); + test('training tags a guidance rule with the files its instruction-file sources apply to', async () => { - const { formatBullet, generalGlob, parseBullet, retagGuidance, ruleScope, sourceScopes } = await import('../training/run.ts'); + const { generalGlob, retagGuidance, ruleScope, sourceScopes } = await import('../training/run.ts'); + const { formatBullet, parseBullet } = await import('../src/guidance.ts'); assert.equal(generalGlob('src/**/*.cs'), '**/*.cs'); assert.equal(generalGlob('tests/**/*.cs'), '**/*.cs'); assert.equal(generalGlob('**'), '**'); @@ -201,6 +247,30 @@ test('training tags a guidance rule with the files its instruction-file sources assert.equal(retagGuidance(formatBullet(rule, []), scopes), tagged); }); +test('retagging refuses a guidance bullet the formatter would not write back as it was', async () => { + const { retagGuidance } = await import('../training/run.ts'); + assert.throws(() => retagGuidance('- **No sources.** Why. *Check:* Look.', new Map()), /Can't retag this bullet/); + assert.throws(() => retagGuidance('- **Empty note.** Why. *Check:* Look. *Note:* Identity ADR-0001', new Map()), /Can't retag this bullet/); +}); + +test('a rerun of training keeps a saved extraction whose source is unchanged, with the source\'s current applyTo', async () => { + const { reuseExtraction, saveExtracted } = await import('../training/run.ts'); + const saved = { hash: 'h', promptHash: 'p', label: 'Identity csharp.instructions.md', applyTo: ['src/**/*.cs'], rules: [] }; + assert.equal(reuseExtraction(undefined, 'h', 'p', undefined), undefined); + assert.equal(reuseExtraction(saved, 'changed', 'p', ['src/**/*.cs']), undefined); + assert.equal(reuseExtraction(saved, 'h', 'changed', ['src/**/*.cs']), undefined); + assert.deepEqual(reuseExtraction(saved, 'h', 'p', ['src/**/*.cs']), { saved, changed: false }); + const widened = reuseExtraction(saved, 'h', 'p', ['src/**/*.cs', 'tests/**/*.cs'])!; + assert.deepEqual(widened, { saved: { ...saved, applyTo: ['src/**/*.cs', 'tests/**/*.cs'] }, changed: true }); + const dropped = reuseExtraction(saved, 'h', 'p', undefined)!; + assert.equal(dropped.changed, true); + const file = path.join(mkdtempSync(path.join(tmpdir(), 'extracted-')), 'instructions', 'csharp-1.json'); + saveExtracted(file, widened.saved); + assert.deepEqual(JSON.parse(readFileSync(file, 'utf8')).applyTo, ['src/**/*.cs', 'tests/**/*.cs']); + saveExtracted(file, dropped.saved); + assert.ok(!('applyTo' in JSON.parse(readFileSync(file, 'utf8')))); +}); + test('the committed guidance carries the tags its sources give it', async () => { const { retagGuidance, sourceScopes } = await import('../training/run.ts'); const scopes = sourceScopes(); diff --git a/training/run.ts b/training/run.ts index f073cbf..d8ea993 100644 --- a/training/run.ts +++ b/training/run.ts @@ -25,7 +25,8 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, rmSync, writeFileSync import path from 'node:path'; import { fileURLToPath } from 'node:url'; import { runAgent, type AgentRun, type CallRecord } from '../src/agent.ts'; -import { appliesToLabel, applyToGlobs, bulletGlobs } from '../src/gather.ts'; +import { applyToGlobs } from '../src/gather.ts'; +import { bulletGlobs, formatBullet, parseBullet, type GuidanceRule } from '../src/guidance.ts'; import { startProxy } from '../src/proxy.ts'; import { mapWithConcurrency } from '../src/review.ts'; @@ -200,12 +201,22 @@ const guidanceSchema = { additionalProperties: false, }; -function saveExtracted(file: string, { hash, promptHash, label, applyTo, rules }: Extracted): void { +export function saveExtracted(file: string, { hash, promptHash, label, applyTo, rules }: Extracted): void { mkdirSync(path.dirname(file), { recursive: true }); const saved: Extracted = { hash, promptHash, label, ...(applyTo?.length ? { applyTo } : {}), rules }; writeFileSync(file, `${JSON.stringify(saved, null, 2)}\n`); } +// A saved extraction a rerun reuses instead of calling the model: one whose source text and prompt are unchanged. It +// takes the source's current applyTo, so an instruction file whose globs changed and whose text didn't keeps its rules +// with the new globs; changed says it differs from what was saved. Undefined when the source must be extracted again. +export function reuseExtraction(saved: Extracted | undefined, hash: string, promptHash: string, applyTo: string[] | undefined): { saved: Extracted; changed: boolean } | undefined { + if (!saved || saved.hash !== hash || saved.promptHash !== promptHash) return undefined; + const { applyTo: before, ...rest } = saved; + const changed = (before ?? []).join(',') !== (applyTo ?? []).join(','); + return { saved: { ...rest, ...(applyTo?.length ? { applyTo } : {}) }, changed }; +} + async function extract(run: AgentRun, sources: Source[], outDir: string, calls: CallRecord[]): Promise { const prompt = readFileSync(path.join(root, 'training', 'prompts', 'extract.md'), 'utf8'); const promptHash = sha(prompt); @@ -216,14 +227,11 @@ async function extract(run: AgentRun, sources: Source[], outDir: string, calls: // Read it directly rather than checking first, so the file can't change between the check and the read. let text: string | undefined; try { text = readFileSync(file, 'utf8'); } catch (error) { if ((error as NodeJS.ErrnoException).code !== 'ENOENT') throw error; } - if (text !== undefined) { - const saved = JSON.parse(text) as Extracted; - if (saved.hash === hash && saved.promptHash === promptHash) { - // An instruction file whose applyTo changed and whose text didn't keeps its rules with the new globs. - if ((saved.applyTo ?? []).join(',') !== (source.applyTo ?? []).join(',')) saveExtracted(file, { ...saved, applyTo: source.applyTo ?? [] }); - reused++; - return; - } + const reuse = reuseExtraction(text === undefined ? undefined : (JSON.parse(text) as Extracted), hash, promptHash, source.applyTo); + if (reuse) { + if (reuse.changed) saveExtracted(file, reuse.saved); + reused++; + return; } const result = await runAgent<{ rules: Rule[] }>(run, { role: `extract:${source.kind}`, @@ -288,21 +296,6 @@ export function ruleScope(sources: string[], scopes: Map): str return [...globs].sort(); } -export interface GuidanceRule { statement: string; why: string; check: string; sources: string[]; note?: string } - -// One guidance bullet: "- **statement** why *Check:* check *Note:* note *Applies to:* `glob` sources". -export function formatBullet(rule: GuidanceRule, globs: string[]): string { - const scope = globs.length ? ` ${appliesToLabel} ${globs.map((g) => `\`${g}\``).join(', ')}` : ''; - return `- **${rule.statement}** ${rule.why} *Check:* ${rule.check}${rule.note ? ` *Note:* ${rule.note}` : ''}${scope} ${rule.sources.join('; ')}`; -} - -export function parseBullet(line: string): GuidanceRule | undefined { - const match = /^- \*\*(.+?)\*\* (.*?) \*Check:\* (.*?)(?: \*Note:\* (.*?))?(?: \*Applies to:\* (?:`[^`]+`(?:, )?)+)? (.*)<\/sub>$/.exec(line); - if (!match) return undefined; - const [, statement, why, check, note, sources] = match; - return { statement: statement!, why: why!, check: check!, ...(note ? { note } : {}), sources: sources!.split('; ') }; -} - // Rewrites each bullet's "*Applies to:*" tag in a guidance file from its sources, with the same formatter a training // run uses, and changes nothing else. Refuses a bullet the formatter wouldn't write back exactly as it was. export function retagGuidance(text: string, scopes: Map): string { From 2cd3885d6cb4ec59b32ee8e5b9571714b883d0d6 Mon Sep 17 00:00:00 2001 From: Marcin Polewski Date: Sat, 31 Oct 2026 00:25:35 -0600 Subject: [PATCH 4/4] Test that a generated file's rules stay out of other parts, and that bulletGlobs reads a tag after a Note --- test/engine.test.ts | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/test/engine.test.ts b/test/engine.test.ts index 3439036..42fa2fb 100644 --- a/test/engine.test.ts +++ b/test/engine.test.ts @@ -178,6 +178,9 @@ test('each part of a large diff gets only the guidance rules tagged for its file // A changed file no part shows may be read by any part's reviewers, so its rules reach every part. files[1]!.shown = false; assert.deepEqual(rulesForFiles([csharp, anywhere], partFiles(files, tsPart)).map((r) => r.id), ['G-a-1', 'G-a-2']); + // A generated file isn't reviewed, so its rules don't reach the other parts. + files[1]!.generated = true; + assert.deepEqual(rulesForFiles([csharp, anywhere], partFiles(files, tsPart)).map((r) => r.id), ['G-a-2']); }); test('a guidance bullet tagged with several globs reaches a review that changes a file any of them matches', async () => { @@ -227,7 +230,7 @@ test('review jobs give each part of the diff only the rules tagged for its files test('training tags a guidance rule with the files its instruction-file sources apply to', async () => { const { generalGlob, retagGuidance, ruleScope, sourceScopes } = await import('../training/run.ts'); - const { formatBullet, parseBullet } = await import('../src/guidance.ts'); + const { bulletGlobs, formatBullet, parseBullet } = await import('../src/guidance.ts'); assert.equal(generalGlob('src/**/*.cs'), '**/*.cs'); assert.equal(generalGlob('tests/**/*.cs'), '**/*.cs'); assert.equal(generalGlob('**'), '**'); @@ -244,6 +247,7 @@ test('training tags a guidance rule with the files its instruction-file sources const tagged = formatBullet(rule, ['**/*.cs']); assert.equal(tagged, '- **Keep functions small.** Why. *Check:* Look. *Note:* Note. *Applies to:* `**/*.cs` Identity csharp-production.instructions.md'); assert.deepEqual(parseBullet(tagged), rule); + assert.deepEqual(bulletGlobs(tagged), ['**/*.cs']); assert.equal(retagGuidance(formatBullet(rule, []), scopes), tagged); });