Closed
Conversation
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
✅ Files skipped from review due to trivial changes (6)
📝 WalkthroughWalkthroughThis PR adds C#/.NET language support across documentation: updates README/install examples and rule counts, adds a Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~15 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment Tip CodeRabbit can use TruffleHog to scan for secrets in your code with verification capabilities.Add a TruffleHog config file (e.g. trufflehog-config.yml, trufflehog.yml) to your project to customize detectors and scanning behavior. The tool runs only when a config file is present. |
Contributor
Greptile SummaryThis PR adds full C# / .NET 8+ language support to the plugin, introducing 5 rule files ( The content is generally high quality and technically accurate. Issues flagged in a prior review (HttpClient anti-pattern, null-forgiving operator on the JWT signing key) have both been addressed in this revision.
Confidence Score: 4/5
Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[C# Language Support] --> B[rules/csharp/]
A --> C[skills/]
B --> B1[coding-style.md\nPascalCase, nullability, dotnet format]
B --> B2[hooks.md\ndotnet format + build PostToolUse]
B --> B3[patterns.md\nRepository, DI, async, LINQ]
B --> B4[security.md\nSecrets, validation, SQLi, XSS]
B --> B5[testing.md\nxUnit, Coverlet, NSubstitute]
C --> C1[csharp-patterns/SKILL.md\nRecords, nullable, pattern matching,\nDI, async, LINQ, error handling,\nMinimal API, configuration]
C --> C2[csharp-testing/SKILL.md\nTDD workflow, xUnit, FluentAssertions,\nNSubstitute, WebApplicationFactory,\nTestContainers, Coverlet]
C --> C3[csharp-security/SKILL.md\nJWT auth, input validation, SQLi,\nXSS, CSRF, secrets, headers,\nrate limiting, OWASP Top 10]
C --> C4[csharp-data-access/SKILL.md\nEF Core, Dapper, DbContext lifecycle,\nmigrations, transactions,\nrepository + Unit of Work]
B1 -.->|references| C1
B3 -.->|references| C1
B4 -.->|references| C3
B5 -.->|references| C2
Last reviewed commit: "fix: address PR revi..." |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
skills/csharp-testing/SKILL.md (1)
734-741: Make the coverage-report open command cross-platform.Line 740 uses
open, which is macOS-specific. Add Linux/Windows alternatives (or a short OS note) to avoid broken copy-paste flows.Suggested docs tweak
-open coveragereport/index.html +# macOS +open coveragereport/index.html +# Linux +xdg-open coveragereport/index.html +# Windows (PowerShell) +start coveragereport/index.html🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@skills/csharp-testing/SKILL.md` around lines 734 - 741, The README uses a macOS-only command ("open coveragereport/index.html"); make this cross-platform by replacing that single-line instruction with platform alternatives or a short note: list "open coveragereport/index.html" for macOS, "xdg-open coveragereport/index.html" for Linux, and "start coveragereport\\index.html" (or PowerShell "Invoke-Item coveragereport\\index.html") for Windows, and mention the user can run the appropriate command for their OS so copy-pasting won't break; update the line containing "open coveragereport/index.html" accordingly.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@skills/csharp-security/SKILL.md`:
- Around line 286-293: The CSP example in the Program.cs middleware (the app.Use
block that calls context.Response.Headers.Append for "Content-Security-Policy")
should not include 'unsafe-inline' for style-src; update the header to remove
'unsafe-inline' and instead recommend using a nonce or hash-based approach for
inline styles (or add a comment/documentation line explaining 'unsafe-inline' is
only a temporary compatibility fallback). Modify the middleware that sets the
header so it emits a policy that uses nonce/hash-based style-src directives and
ensure the header string generation is updated consistently where
context.Response.Headers.Append("Content-Security-Policy", ...) is set.
- Around line 11-19: Rename the top-level heading "When to Activate" to the
required "When to Use", move the current bullet list under that new "When to
Use" heading, and add two new top-level headings "How It Works" and "Examples"
(each with at least one descriptive sentence or short paragraph) so the skill
Markdown follows the repository format; update any internal references if they
rely on the old "When to Activate" heading and ensure the three headings appear
as clear top-level sections in the document.
In `@skills/csharp-testing/SKILL.md`:
- Around line 11-18: Replace the non-standard "When to Activate" heading in
SKILL.md with the exact required section headings "When to Use", "How It Works",
and "Examples"; move the existing bullet list under "When to Use", add a brief
descriptive paragraph under "How It Works" explaining the skill's behavior in C#
testing workflows, and include at least one concrete usage snippet or scenario
under "Examples". Also apply the same section-heading restructuring to the other
referenced skill (19-902) to match the skills/**/*.md schema.
---
Nitpick comments:
In `@skills/csharp-testing/SKILL.md`:
- Around line 734-741: The README uses a macOS-only command ("open
coveragereport/index.html"); make this cross-platform by replacing that
single-line instruction with platform alternatives or a short note: list "open
coveragereport/index.html" for macOS, "xdg-open coveragereport/index.html" for
Linux, and "start coveragereport\\index.html" (or PowerShell "Invoke-Item
coveragereport\\index.html") for Windows, and mention the user can run the
appropriate command for their OS so copy-pasting won't break; update the line
containing "open coveragereport/index.html" accordingly.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 324a54b6-267c-4ad0-a09d-dd85a768a212
📒 Files selected for processing (10)
README.mdrules/README.mdrules/csharp/coding-style.mdrules/csharp/hooks.mdrules/csharp/patterns.mdrules/csharp/security.mdrules/csharp/testing.mdskills/csharp-patterns/SKILL.mdskills/csharp-security/SKILL.mdskills/csharp-testing/SKILL.md
skills/csharp-security/SKILL.md
Outdated
Comment on lines
+11
to
+19
| ## When to Activate | ||
|
|
||
| - Handling user input in ASP.NET Core applications | ||
| - Configuring authentication and authorization | ||
| - Reviewing C# code for security vulnerabilities | ||
| - Building REST APIs with ASP.NET Core | ||
| - Managing secrets and configuration | ||
| - Setting up HTTP security headers | ||
|
|
Contributor
There was a problem hiding this comment.
Add explicit required skill sections (When to Use, How It Works, Examples).
Line 11 currently uses When to Activate, and there are no explicit top-level How It Works / Examples sections. Please add those headings and reorganize existing content under them to match repository skill format requirements.
As per coding guidelines "skills/**/*.md: Skills should be formatted as Markdown with clear sections for When to Use, How It Works, and Examples".
Also applies to: 90-640
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/csharp-security/SKILL.md` around lines 11 - 19, Rename the top-level
heading "When to Activate" to the required "When to Use", move the current
bullet list under that new "When to Use" heading, and add two new top-level
headings "How It Works" and "Examples" (each with at least one descriptive
sentence or short paragraph) so the skill Markdown follows the repository
format; update any internal references if they rely on the old "When to
Activate" heading and ensure the three headings appear as clear top-level
sections in the document.
skills/csharp-testing/SKILL.md
Outdated
Comment on lines
+11
to
+18
| ## When to Activate | ||
|
|
||
| - Writing new C# code (follow TDD: red, green, refactor) | ||
| - Designing test suites for .NET applications | ||
| - Reviewing C# test coverage | ||
| - Setting up .NET testing infrastructure | ||
| - Debugging failing C# tests | ||
|
|
Contributor
There was a problem hiding this comment.
Restructure to include explicit When to Use, How It Works, and Examples sections.
Line 11 uses When to Activate, but the required skill-section schema is not explicitly present. Please add the exact section headings and nest current content under them.
As per coding guidelines "skills/**/*.md: Skills should be formatted as Markdown with clear sections for When to Use, How It Works, and Examples".
Also applies to: 19-902
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@skills/csharp-testing/SKILL.md` around lines 11 - 18, Replace the
non-standard "When to Activate" heading in SKILL.md with the exact required
section headings "When to Use", "How It Works", and "Examples"; move the
existing bullet list under "When to Use", add a brief descriptive paragraph
under "How It Works" explaining the skill's behavior in C# testing workflows,
and include at least one concrete usage snippet or scenario under "Examples".
Also apply the same section-heading restructuring to the other referenced skill
(19-902) to match the skills/**/*.md schema.
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
6 issues found across 10 files
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/csharp-patterns/SKILL.md">
<violation number="1" location="skills/csharp-patterns/SKILL.md:29">
P2: The comment claims record structs have “no heap allocation,” but value types can still allocate on the heap when boxed (e.g., cast to object or interface). This absolute statement can mislead performance guidance.</violation>
<violation number="2" location="skills/csharp-patterns/SKILL.md:284">
P2: The “Good” async example creates and disposes a new HttpClient per call. Microsoft’s guidelines warn this can exhaust sockets/ports under load; best practice is to reuse a long‑lived HttpClient or use IHttpClientFactory. As written, this doc promotes a known anti‑pattern.</violation>
</file>
<file name="skills/csharp-testing/SKILL.md">
<violation number="1" location="skills/csharp-testing/SKILL.md:231">
P2: Example test is guaranteed to fail because QueryUsers returns an empty list while the test asserts NotBeEmpty, making the sample internally inconsistent.</violation>
<violation number="2" location="skills/csharp-testing/SKILL.md:492">
P2: The sample uses a fixed in-memory database name. EF Core’s in-memory provider reuses databases by name, so a shared WebApplicationFactory/IClassFixture can leak state between tests and cause flaky, order-dependent failures. Use a unique name per test (or an InMemoryDatabaseRoot scoped per test) to preserve isolation.</violation>
</file>
<file name="skills/csharp-security/SKILL.md">
<violation number="1" location="skills/csharp-security/SKILL.md:336">
P2: JWT CSRF guidance is too broad. Disabling antiforgery is only safe when JWTs are sent via Authorization headers; if JWTs are stored in cookies, browsers auto-send them and CSRF protections are still needed.</violation>
</file>
<file name="rules/csharp/coding-style.md">
<violation number="1" location="rules/csharp/coding-style.md:14">
P2: Recommending `<LangVersion>latest</LangVersion>` makes language version selection SDK-dependent, which can change across machines/CI and break reproducible builds. Prefer an explicit version (e.g., `12.0` for .NET 8) or `latestMajor` if you want stable major targeting.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
- Replace HttpClient anti-pattern with IHttpClientFactory in async example - Fix misleading "no heap allocation" claim for record structs - Replace null-forgiving operator with fail-fast pattern for Jwt:Key config - Remove unsafe-inline from CSP example to prevent XSS - Clarify JWT CSRF guidance applies only when using Authorization header - Use unique in-memory database names for test isolation - Fix internally inconsistent test stub (empty list vs NotBeEmpty assert) - Rename "When to Activate" to "When to Use" per skill format convention
Contributor
|
Contributor
Analysis Failed
Troubleshooting
Retry: |
Contributor
There was a problem hiding this comment.
6 issues found across 6 files (changes from recent commits).
Prompt for AI agents (unresolved issues)
Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.
<file name="skills/csharp-data-access/SKILL.md">
<violation number="1" location="skills/csharp-data-access/SKILL.md:306">
P2: The example is labeled “Bulk insert” but Dapper’s ExecuteAsync with an IEnumerable executes one insert per item, not a true bulk protocol. This can mislead readers into using it for high-volume writes and hit performance issues.</violation>
</file>
<file name="skills/csharp-patterns/SKILL.md">
<violation number="1" location="skills/csharp-patterns/SKILL.md:193">
P2: The DI example now references `SmtpSettings.SectionName` without defining `SmtpSettings` anywhere in the skill file, leaving the snippet incomplete for copy/paste guidance.</violation>
</file>
<file name="AGENTS.md">
<violation number="1" location="AGENTS.md:3">
P2: AGENTS.md has conflicting hardcoded skill counts (112 vs 102), creating inconsistent project metadata.</violation>
</file>
<file name="skills/csharp-security/SKILL.md">
<violation number="1" location="skills/csharp-security/SKILL.md:275">
P2: The snippet labels HSTS as “production only” but applies `app.UseHsts()` unconditionally, contradicting the guidance and potentially causing cached HSTS issues in development. Gate HSTS with an environment check to match the comment and best practices.</violation>
<violation number="2" location="skills/csharp-security/SKILL.md:275">
P1: Custom security headers middleware is documented but not registered, so the advertised headers will not be applied.</violation>
</file>
<file name="README.md">
<violation number="1" location="README.md:195">
P3: README now reports 112 skills in updated sections but still lists 102 skills in the Cross-Tool Feature Parity table, creating contradictory counts in the same document.</violation>
</file>
Reply with feedback, questions, or to request a fix. Tag @cubic-dev-ai to re-run a review.
Owner
|
Thanks for the comprehensive C# language support! The skills and rules follow existing conventions well. This PR now has merge conflicts (likely from recent merges updating AGENTS.md/README.md counts). Could you rebase on the latest |
3 tasks
Owner
|
Superseded by rebased PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What Changed
Full C# / .NET 8+ language support following existing patterns (Perl as template):
Rules (
rules/csharp/) — 5 files with**/*.cs,**/*.csproj,**/*.sln,**/*.razorpath globs:coding-style.md— PascalCase naming, records, nullability,dotnet formathooks.md— PostToolUse fordotnet format/dotnet build, Console.WriteLine warningpatterns.md— Repository pattern with EF Core, DI, async/await, LINQsecurity.md— User Secrets, EF Core params, Razor encoding, Roslyn analyzerstesting.md— xUnit, Coverlet, NSubstitute, FluentAssertionsSkills — 3 comprehensive SKILL.md files:
csharp-patterns/(691 lines) — Records, nullable types, pattern matching, DI, async, LINQ, errorhandling, project organization, Minimal API vs Controllers, configuration
csharp-testing/(902 lines) — TDD workflow, xUnit, FluentAssertions, NSubstitute,WebApplicationFactory, TestContainers, Coverlet
csharp-security/(640 lines) — Auth, validation, SQLi, XSS, CSRF, secrets, headers, rate limiting,OWASP mapping
Documentation updates:
Why This Change
C# was listed in the README roadmap as a planned addition. This brings full rule + skill coverage following
the established patterns, consistent with other supported languages (TypeScript, Python, Go, Swift, Perl,
PHP).
Testing Done
node tests/run-all.js) — 1390/1402 pass, 12 failures are pre-existingupstream issues (sql.js, package-manager, formatter, hooks, validators) unrelated to this change
./install.shlistscsharpin available languages../common/relative links resolve correctlyawaitonThrowAsyncand inconsistent dash styleType of Change
fix:Bug fixfeat:New featurerefactor:Code refactoringdocs:Documentationtest:Testschore:Maintenance/toolingci:CI/CD changesSecurity & Quality Checklist
Documentation
Summary by cubic
Adds full C# (.NET 8+) language support with 5 rule files and 4 skills (patterns, testing, security, data-access), plus installer and README updates. Increases rules 34→39 and skills 108→112, and wires
dotnetformat/build hooks.New Features
rules/csharp/):coding-style.md,hooks.md,patterns.md,security.md,testing.mdwith globs for**/*.cs,**/*.csproj,**/*.sln,**/*.razor; PostToolUse fordotnet format/dotnet build; warn onConsole.WriteLineoutsideProgram.cs.csharp-patterns,csharp-testing,csharp-security,csharp-data-access(EF Core/Dapper patterns).csharp; updated counts and tables (rules 39, skills 112);rules/README.mdincludescsharp.Bug Fixes
IHttpClientFactoryinstead ofHttpClient; fail-fast for missingJwt:Key; stricter CSP (removedunsafe-inline); clarified JWT/CSRF note (Authorization header); corrected record struct allocation note.awaitonThrowAsync.Written for commit 7a01e6f. Summary will update on new commits.
Summary by CodeRabbit
New Features
Documentation