Add System.Security.Cryptography Copilot instructions#131006
Add System.Security.Cryptography Copilot instructions#131006PranavSenthilnathan wants to merge 4 commits into
Conversation
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42bda83a-f457-4dcc-a426-e12aa6e4d47e
|
Azure Pipelines: Successfully started running 1 pipeline(s). 15 pipeline(s) were filtered out due to trigger conditions. There may be pipelines that require an authorized user to comment /azp run to run. |
|
Tagging subscribers to this area: @bartonjs, @vcsjones, @dotnet/area-system-security |
There was a problem hiding this comment.
Pull request overview
Adds a new folder-scoped GitHub Copilot instruction file for src/libraries/System.Security.Cryptography/** to guide generated contributions toward established crypto coding and testing patterns.
Changes:
- Introduces System.Security.Cryptography-specific guidance covering code style, correctness checks for
Try*APIs, and sensitive-data handling. - Adds security recommendations around constant-time comparisons and buffer clearing/returning.
- Adds test-skipping guidance intended to reduce noisy skips (but needs adjustment to match existing patterns/semantics).
|
|
||
| ## Tests | ||
|
|
||
| - Avoid throwing `SkipTestException`, which creates noisy test output. Prefer `[ConditionalFact]` or `[ConditionalTheory]` with a condition; when that is not possible, return early from the test. |
There was a problem hiding this comment.
Hopefully these comments go away with the new instructions.
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42bda83a-f457-4dcc-a426-e12aa6e4d47e
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42bda83a-f457-4dcc-a426-e12aa6e4d47e
Co-authored-by: Copilot App <223556219+Copilot@users.noreply.github.com> Copilot-Session: 42bda83a-f457-4dcc-a426-e12aa6e4d47e
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
.github/instructions/system-security-cryptography.instructions.md:24
- Returning early from an xUnit test reports the test as passed, not skipped, so this guidance can hide missing coverage. Also, System.Security.Cryptography tests already use
SkipTestExceptionwidely (e.g.,CryptoConfigTests.cs:158andSkipTestException.ThrowUnless(...)inECDsaCngImportExportTests.cs:56), so advising against it unconditionally doesn’t match established practice. Suggest: prefer[ConditionalFact]/[ConditionalTheory]when possible, otherwise useSkipTestExceptionso the test is reported as skipped.
- Avoid throwing `SkipTestException`, which creates noisy test output. Prefer `[ConditionalFact]` or `[ConditionalTheory]` with a condition; when that is not possible, return early from the test.
|
Workflow state for the Holistic Review Orchestrator. {
"version": 5,
"last_dispatched_commit": "2c80f423f3bc384c55ade497eb395ff3a7fb727f",
"last_dispatched_base_ref": "main",
"last_dispatched_base_sha": "189c884f91b51313b41de40c3bf306c97c9c73eb",
"last_reviewed_commit": "2c80f423f3bc384c55ade497eb395ff3a7fb727f",
"last_reviewed_base_ref": "main",
"last_reviewed_base_sha": "189c884f91b51313b41de40c3bf306c97c9c73eb",
"last_recorded_worker_run_id": "29687178549",
"review_attempt_commit": "",
"review_attempt_base_ref": "",
"review_attempt_count": 0,
"max_review_attempts": 5,
"review_history_format": "holistic-review-disclosure-v1",
"review_history": [
{
"commit": "2c80f423f3bc384c55ade497eb395ff3a7fb727f",
"review_id": 4730767818
}
]
} |
There was a problem hiding this comment.
Holistic Review
Motivation: Justified. The repo already has path-specific .github/instructions/*.instructions.md files for several src/libraries areas (Net.Quic, Compression, Extensions, etc.), and adding equivalent guidance for System.Security.Cryptography fills a real gap for both the built-in Copilot reviewer and the code-review skill.
Approach: Correct. The file follows the established sibling pattern exactly — YAML frontmatter with an applyTo glob scoped to src/libraries/System.Security.Cryptography/**, followed by a # ... — Folder-Specific Guidance heading and topical sections. It codifies existing conventions rather than inventing new ones.
Summary: ✅ LGTM. Documentation-only change (24 additions, one new file, no code/tests affected). I verified each concrete reference against the codebase: csharp_prefer_simple_using_statement = false:none exists in .editorconfig (line 92); CryptoPool.Rent/Return, CryptoPoolLease, PinAndClear.Track, and CryptographicOperations.FixedTimeEquals/ZeroMemory are all real, in-use helpers under src/libraries/Common/src/System/Security/Cryptography and the product source. The guidance is accurate, internally consistent, and matches the actual patterns enforced in the area. No actionable findings.
Detailed Findings
✅ Documentation accuracy — All referenced conventions and helpers verified
- Style guidance on scoped
usingstatements correctly cites the real.editorconfigsettingcsharp_prefer_simple_using_statement = false:none. - Security guidance references helpers that exist and are actively used:
CryptographicOperations.ZeroMemory,CryptoPoolLease,CryptoPool.Rent/Return,PinAndClear.Track(e.g.CngHelpers.cs,CompositeMLDsaManaged.*.cs), andCryptographicOperations.FixedTimeEquals. - Frontmatter shape and
applyToscoping match sibling instruction files (e.g.system-net-quic.instructions.md,compression.instructions.md).
✅ Scope and risk — Minimal and self-contained
Additive, docs-only, no build or test impact. As a guidance document its recommendations are advisory to future reviewers/authors and carry no runtime risk.
Note
This review was generated by this repository's Holistic Review agentic workflow to complement the built-in Copilot review.
Generated by Holistic Review · 53.1 AIC · ⌖ 10.5 AIC · ⊞ 10K
Adds path-specific GitHub Copilot guidance for
System.Security.Cryptography, covering established code style, correctness, sensitive-data handling, constant-time comparisons, and test skipping conventions.Documentation-only change; no build or tests were run.
Note
This pull request description was generated by GitHub Copilot.