Give learned guidance rules only to reviews of the files they apply to

CHANGELOG.md

@@ -25,6 +25,7 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/),
 
 ### 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)).
 - A review no longer posts a finding that says a file the pull request changes isn't changed or isn't in the diff. Synthesis saw only the diffs of the files the reviewers' findings named, and no list of changed files, so on [#22](https://github.com/NCodeGroup/code-review/pull/22) it wrote a finding of its own saying `src/command.ts` had no changes. It now gets the list of changed files and every file's diff that fits its budget, the reviewers' and synthesis's prompts say the list is complete, and a finding that still makes such a claim in its title, problem or consequence is dropped before the finding limit applies. The summary comment says how many were dropped and how severe, the check run and the trace list them, and a verdict that rested on them is settled on the findings kept ([#32](https://github.com/NCodeGroup/code-review/issues/32)).
 - A request for the earlier summary comments that fails outright, such as on a DNS failure or a reset connection, no longer fails the review step: the run reviews the pull request again, as it does when GitHub refuses the request ([#39](https://github.com/NCodeGroup/code-review/issues/39)).
 - `tools/Enable-CodeReview.ps1 -SkipBedrock` skips only prompting for and setting the Bedrock key: the opted-in repositories still get access to the existing `AWS_BEARER_TOKEN_BEDROCK` secret, instead of failing every review once the organization's API credits run out. `-Repo` takes a comma-separated list as one value, as `pwsh -File` passes it, and `-Repo a, b` there fails before any change instead of binding `b` to another parameter. The help says which changes need the `admin:org` scope. A failure to read an organization secret, other than its not existing, stops the script instead of counting as a missing secret, and a failed grant or `secret set` shows gh's exit code ([#34](https://github.com/NCodeGroup/code-review/issues/34)).

README.md

@@ -17,6 +17,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.
+
 Each model call has its own cost cap, and a whole review has a **cost ceiling** that guards against a runaway review only: $8 plus $4 for each part of the diff, so $12 for most pull requests and $32 for one reviewed in six parts, far above the $0.60 to $2.10 a normal review costs and the $6 to $7 of a 2 MB pull request. Once the calls so far reach it, the reviewer and rule-check calls that haven't started are skipped, each such reviewer shows `skipped: run budget` in the check run, synthesis, and verification when it's on, still run on what was found, and the summary warns that the review is incomplete and doesn't approve. To change it, set `CODE_REVIEW_MAX_RUN_USD` to a positive number of dollars in the calling job's `env`; any other value warns and keeps the default.
 
 **Verification** is off by default. `CODE_REVIEW_VERIFICATION=on` adds a stage after synthesis: each blocker and should-fix it merged goes to a Sonnet call with Read, Grep and Glob in the checkout, behind the same path guard as the reviewers, with its diff hunk and its reviewers' charters, a file's findings per call at up to $0.50 each. The call proves or refutes the finding with a concrete code path. A confirmed finding stays, with the path added to its evidence; a refuted one is dropped and listed in the check run under "Refuted by verification", with the reason; one it can neither prove nor refute stays as a question, one severity lower. Nits aren't verified. The summary, which synthesis wrote before verification, gets a sentence naming the findings verification refuted and counting the ones it made questions. Verification never makes a review an approval: a request for changes with no blocker or should-fix left becomes a comment, since a verifier misled by the change's own code can refute a real finding. The calls show as `verification` in the check run's cost table and the trace records each one. It stays off until an eval shows it catches as much at fewer blockers and should-fixes, for what it costs ([#41](https://github.com/NCodeGroup/code-review/issues/41)).

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. <sub>Identity ADR-0095</sub>
 - **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. <sub>Identity ADR-0230; Identity ADR-0235; Identity ADR-0238; Identity ADR-0265; Identity review of #420; Identity csharp-production.instructions.md</sub>
-- **Hold a secret that is stored in a field or kept across an `await` as a sensitive `BufferFactory.Rent(..., isSensitive: true, out Memory<byte>)` 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. <sub>Identity csharp-production.instructions.md</sub>
+- **Hold a secret that is stored in a field or kept across an `await` as a sensitive `BufferFactory.Rent(..., isSensitive: true, out Memory<byte>)` 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` <sub>Identity csharp-production.instructions.md</sub>
 - **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. <sub>Identity ADR-0115</sub>
 - **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<byte>`, call `Advance` only after the operation succeeds, and zero anything already written if it fails. Use a `Span<byte>` 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. <sub>Identity ADR-0279; Identity review of #370</sub>
 - **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. <sub>Identity ADR-0092; Identity csharp-production.instructions.md</sub>
@@ -22,10 +22,10 @@ applyTo: "**/*.cs"
 - **Give APIs that produce binary data (encode, export, serialize, key/DER material, JSON records, stream-body reads) `ReadOnlySpan<T>`/`ReadOnlyMemory<T>` inputs, and have them write to a caller-supplied generic `ref TWriter destination` where `TWriter : IBufferWriter<byte>, 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<byte>` with no copy. *Check:* Flag new or changed methods that return `byte[]` or take `byte[]` parameters for binary data. Flag a plain `IBufferWriter<byte>` 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. <sub>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</sub>
 - **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<byte>` destination, also give it an `IBufferWriter<byte>` (`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. <sub>Identity ADR-0279; Identity ADR-0235; Identity review of #375; Identity review of #420</sub>
 - **Get a contiguous span from a multi-segment `Sequence<byte>`/`ReadOnlySequence<byte>` 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. <sub>Identity ADR-0116; Identity csharp-production.instructions.md</sub>
-- **Use the `out Span<T>` overload of `BufferFactory.Rent` by default, use `out Memory<T>` 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<T>` used only locally and synchronously. <sub>Identity csharp-production.instructions.md</sub>
+- **Use the `out Span<T>` overload of `BufferFactory.Rent` by default, use `out Memory<T>` 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<T>` used only locally and synchronously. *Applies to:* `**/*.cs` <sub>Identity csharp-production.instructions.md</sub>
 - **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. <sub>Identity review of #420</sub>
 - **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<byte>`.** 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. <sub>Identity ADR-0107; Identity ADR-0116; Identity ADR-0273; Identity review of #341; Identity review of #420</sub>
 - **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. <sub>Identity ADR-0119; Identity ADR-0283</sub>
-- **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. <sub>Identity csharp-production.instructions.md</sub>
+- **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` <sub>Identity csharp-production.instructions.md</sub>
 - **Read and parse text as `ReadOnlySpan<char>`. 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. <sub>Identity review of #341; Identity review of #420; Identity csharp-production.instructions.md</sub>
 - **Encode strings into a caller-provided span (stackalloc when small and bounded, otherwise rented) with `Encoding.GetBytes(string, Span<byte>)`, and declare fixed protocol or crypto labels (HKDF info, purposes) as `ReadOnlySpan<byte>` `"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. <sub>Identity review of #420; Identity review of #341</sub>