diff --git a/.claude-plugin/marketplace.json b/.claude-plugin/marketplace.json index f72f737..80a9655 100644 --- a/.claude-plugin/marketplace.json +++ b/.claude-plugin/marketplace.json @@ -43,8 +43,8 @@ { "name": "bitwarden-software-engineer", "source": "./plugins/bitwarden-software-engineer", - "version": "0.3.3", - "description": "Full-stack software engineering assistant with skills for Bitwarden client, server, and database development patterns." + "version": "0.4.0", + "description": "Comprehensive full-stack software engineering assistant proficient in modern software development at Bitwarden." }, { "name": "bitwarden-atlassian-tools", diff --git a/README.md b/README.md index 1922b9c..007a0d6 100644 --- a/README.md +++ b/README.md @@ -13,7 +13,7 @@ A curated collection of plugins for AI-assisted development at Bitwarden. Enable | [bitwarden-init](plugins/bitwarden-init/) | 1.1.0 | Initialize and enhance CLAUDE.md files with Bitwarden's standardized template format | | [bitwarden-product-analyst](plugins/bitwarden-product-analyst/) | 0.1.5 | Product analyst agent for creating comprehensive Bitwarden requirements documents from multiple sources | | [bitwarden-security-engineer](plugins/bitwarden-security-engineer/) | 1.0.0 | Application security engineering: vulnerability triage, threat modeling, and secure code analysis | -| [bitwarden-software-engineer](plugins/bitwarden-software-engineer/) | 0.3.3 | Full-stack engineering assistant for Bitwarden client, server, and database development patterns | +| [bitwarden-software-engineer](plugins/bitwarden-software-engineer/) | 0.4.0 | Comprehensive full-stack software engineering assistant proficient in modern software development at Bitwarden. | | [claude-config-validator](plugins/claude-config-validator/) | 1.1.1 | Validates Claude Code configuration files for security, structure, and quality | | [claude-retrospective](plugins/claude-retrospective/) | 1.1.1 | Analyze Claude Code sessions to identify successful patterns and improvement opportunities | diff --git a/plugins/bitwarden-software-engineer/.claude-plugin/plugin.json b/plugins/bitwarden-software-engineer/.claude-plugin/plugin.json index 671295b..293f93a 100644 --- a/plugins/bitwarden-software-engineer/.claude-plugin/plugin.json +++ b/plugins/bitwarden-software-engineer/.claude-plugin/plugin.json @@ -1,6 +1,6 @@ { "name": "bitwarden-software-engineer", - "version": "0.3.3", + "version": "0.4.0", "description": "Comprehensive full-stack software engineering assistant proficient in modern software development at Bitwarden.", "author": { "name": "Bitwarden", diff --git a/plugins/bitwarden-software-engineer/CHANGELOG.md b/plugins/bitwarden-software-engineer/CHANGELOG.md index be86bb8..2948729 100644 --- a/plugins/bitwarden-software-engineer/CHANGELOG.md +++ b/plugins/bitwarden-software-engineer/CHANGELOG.md @@ -5,6 +5,16 @@ All notable changes to the `bitwarden-software-engineer` plugin will be document The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/), and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0.html). +## [0.4.0] - 2026-04-21 + +### Changed + +- Aligned description in the README.md with the plugin.json. + +### Removed + +- Removed the repo specific skills and migrated them to their respective repos. Claude Code uses progressive disclosure to use them when needed. + ## [0.3.3] - 2026-04-15 ### Changed diff --git a/plugins/bitwarden-software-engineer/README.md b/plugins/bitwarden-software-engineer/README.md index e57ab6a..a84b6f1 100644 --- a/plugins/bitwarden-software-engineer/README.md +++ b/plugins/bitwarden-software-engineer/README.md @@ -1,3 +1,15 @@ # Bitwarden Software Engineer Plugin Claude Code skills for Bitwarden development patterns. Generic AI coding assistance doesn't know our conventions, architecture decisions, or anti-patterns we've learned the hard way. These skills keep Claude focused on how we build software here. + +## Overview + +The plugin provides a software engineer agent for software engineering at Bitwarden. + +## Usage + +Install the plugin and invoke the agent: + +``` +Use the bitwarden-software-engineer agent to implement Jira story PM-12345. +``` diff --git a/plugins/bitwarden-software-engineer/agents/bitwarden-software-engineer.md b/plugins/bitwarden-software-engineer/agents/bitwarden-software-engineer.md index 0ed1ffa..4512c0f 100644 --- a/plugins/bitwarden-software-engineer/agents/bitwarden-software-engineer.md +++ b/plugins/bitwarden-software-engineer/agents/bitwarden-software-engineer.md @@ -1,16 +1,12 @@ --- name: bitwarden-software-engineer -description: Full-stack software engineer specializing in C#, JavaScript, TypeScript, and SQL. Coordinates complex development tasks across languages. Use for feature implementation, and cross-language refactoring. +description: Comprehensive full-stack software engineering assistant proficient in modern software development at Bitwarden. Use for feature implementation, and cross-language refactoring. model: opus tools: Read, Write, Edit, Bash, Glob, Grep -skills: - - writing-client-code - - writing-server-code - - writing-database-queries color: blue --- -You are a senior full-stack software engineer with expertise across C#, JavaScript, TypeScript, and SQL. You're an engineer working with the team, not just executing commands. Focus intently on code quality **over** code quantity. You avoid over-engineering because you focus on what's needed, not what might be needed. +You are a senior full-stack software engineer. You're an engineer working with the team, not just executing commands. Focus intently on code quality **over** code quantity. You avoid over-engineering because you focus on what's needed, not what might be needed. ## Purpose @@ -25,7 +21,7 @@ Coordinate complex software development tasks that span multiple languages, arch ## Skill Routing -The architectural skills (`writing-client-code`, `writing-server-code`, `writing-database-queries`) are preloaded. For implementation tasks, activate the appropriate skill: +For implementation tasks, activate the appropriate skill: - **Dapper/stored procedure work** (creating SPs, MSSQL migrations, Dapper repository methods) → activate `implementing-dapper-queries` - **EF Core work** (EF repositories, EF migrations, PostgreSQL/MySQL/SQLite) → activate `implementing-ef-core` diff --git a/plugins/bitwarden-software-engineer/skills/implementing-dapper-queries/SKILL.md b/plugins/bitwarden-software-engineer/skills/implementing-dapper-queries/SKILL.md deleted file mode 100644 index 23dfee7..0000000 --- a/plugins/bitwarden-software-engineer/skills/implementing-dapper-queries/SKILL.md +++ /dev/null @@ -1,157 +0,0 @@ ---- -name: implementing-dapper-queries -description: Implementing Dapper repository methods and stored procedures for MSSQL at Bitwarden. Use when creating or modifying Dapper repositories, writing stored procedures, or working with MSSQL-specific data access in the server repo. ---- - -## Repository Pattern - -All Dapper implementations live in `src/Infrastructure/Dapper/Repositories/`. Each repository class implements an interface from `src/Core/` and uses stored procedures for all database operations. The repository method is intentionally thin — it maps C# parameters to SQL parameters and maps result sets back to domain objects. - -### Stored procedures over inline SQL - -The default pattern is stored procedures for all Dapper database operations. Some exceptions exist where inline SQL is used — these are provided automatically by the repository base class and parent patterns, not written ad-hoc in individual repository methods. - -## Workflow - -1. **Define/update the stored procedure** in `src/Sql/dbo/Stored Procedures/` — use plain `CREATE PROCEDURE` (SSDT syntax) -2. **Create a migration script** in `util/Migrator/DbScripts/` that deploys it — use `CREATE OR ALTER PROCEDURE` (idempotent) -3. **Implement the repository method** in `src/Infrastructure/Dapper/Repositories/` using `DapperServiceProvider` to call the procedure -4. **Write integration tests** using `[DatabaseData]` attribute - -The stored procedure is the source of truth for MSSQL query behavior. The Dapper repository method is thin — it maps parameters and results. - -### Stored procedure naming convention - -Procedures follow `{Entity}_{Action}` pattern: `User_Create`, `Cipher_ReadManyByUserId`, `Organization_DeleteById`. Tooling and code generation rely on this convention to map repository methods to their procedures. - -## Key Decisions That Trip Up AI Assistants - -### `CREATE OR ALTER` vs `CREATE PROCEDURE` — depends on file location - -Bitwarden maintains two copies of every stored procedure in different contexts with different toolchain constraints: - -| Context | Location | Required syntax | -| ---------------------- | -------------------------------- | --------------------------- | -| **SSDT schema source** | `src/Sql/dbo/Stored Procedures/` | `CREATE PROCEDURE` (plain) | -| **Migration script** | `util/Migrator/DbScripts/` | `CREATE OR ALTER PROCEDURE` | - -**Why they differ:** - -- **SSDT projects** do not support `CREATE OR ALTER` — using it produces build errors. SSDT manages object lifecycle through its own deployment model, so each source file must contain a bare `CREATE PROCEDURE`. -- **Migration scripts** must be idempotent because they may be re-run. `CREATE OR ALTER` works whether the procedure exists or not. Never use bare `CREATE PROCEDURE` in a migration. - -### SSDT table files require `GO` batch separators - -In `src/Sql/dbo/Tables/`, SSDT requires a `GO` batch separator between `CREATE TABLE` and any subsequent `CREATE INDEX` or `CREATE NONCLUSTERED INDEX` statements. - -```sql --- CORRECT — GO separates DDL statements for SSDT -CREATE TABLE [dbo].[Example] ( - [Id] UNIQUEIDENTIFIER NOT NULL, - [Name] NVARCHAR(256) NOT NULL, - CONSTRAINT [PK_Example] PRIMARY KEY CLUSTERED ([Id] ASC) -) -GO - -CREATE NONCLUSTERED INDEX [IX_Example_Name] - ON [dbo].[Example] ([Name] ASC) -GO -``` - -### New parameters must be nullable with defaults - -When adding parameters to existing stored procedures, always use `@NewParam DATATYPE = NULL`. Existing callers don't pass the new parameter — without a default, they break. - -### NOT NULL columns: use inline defaults, not ALTER-UPDATE-ALTER - -Adding a NOT NULL column by first adding it nullable, updating all rows, then altering to NOT NULL causes a full table scan. Instead, use `ADD [Column] INT NOT NULL CONSTRAINT DF_Table_Column DEFAULT 0` — this is a metadata-only operation in SQL Server. **This is the single most common mistake AI assistants make with Bitwarden migrations.** - -### Never create indexes on large tables in migration scripts - -Creating indexes on `dbo.Cipher`, `dbo.OrganizationUser`, or other large tables in migration scripts can cause outages. Never specify `ONLINE = ON` in scripts — production handles this automatically, and the option fails on unsupported SQL Server editions. Large index operations belong in `DbScripts_manual`. - -### Use defaults only for numeric types - -Use defaults for `BIT`, `TINYINT`, `INT`, `BIGINT`. Never use defaults for `VARCHAR`, `NVARCHAR`, or MAX types. SQL Server handles these differently and defaults on strings create unexpected behavior with EF Core migrations. - -### Views require metadata refresh - -After modifying a table, any views that reference it have stale metadata. Call `sp_refreshview` on affected views. After altering views, call `sp_refreshsqlmodule` on dependent procedures. This is the most frequently forgotten step. - -### GUID columns use `UNIQUEIDENTIFIER` - -All entity IDs are `UNIQUEIDENTIFIER` populated by `CoreHelpers.GenerateComb()` in application code, not by SQL Server. Never use `NEWID()` or `NEWSEQUENTIALID()` in stored procedures. - -## EF Parity Requirement - -Every stored procedure's behavior must be exactly replicated in the EF Core implementation. When writing a new stored procedure, think about how the EF implementation will reproduce the same filtering, ordering, and side effects. If a stored procedure does something complex (e.g., conditional updates, multi-table operations), document the expected behavior clearly so the EF implementation can match it. - -## Critical Rules - -These are the most frequently violated conventions. Claude cannot fetch the linked docs at runtime, so these are inlined here: - -- **`SET NOCOUNT ON`** at the start of every stored procedure -- **Parameter naming:** `@ParamName` in PascalCase, matching C# property names -- **Migration scripts must be idempotent** — use `CREATE OR ALTER` in `util/Migrator/DbScripts/`; use plain `CREATE PROCEDURE` in SSDT source (`src/Sql/dbo/`) -- **Constraint naming:** `PK_TableName`, `FK_Child_Parent`, `IX_Table_Column`, `DF_Table_Column` -- **Stored procedure file naming:** one procedure per file, named `{Entity}_{Action}.sql` - -## Examples - -### Stored procedure creation — SSDT source vs migration script - -```sql --- SSDT source file: src/Sql/dbo/Stored Procedures/User_ReadById.sql --- Use plain CREATE PROCEDURE (SSDT does not support CREATE OR ALTER) -CREATE PROCEDURE [dbo].[User_ReadById] - @Id UNIQUEIDENTIFIER -AS -BEGIN - SET NOCOUNT ON - SELECT * FROM [dbo].[User] WHERE [Id] = @Id -END -``` - -```sql --- Migration script: util/Migrator/DbScripts/YYYY-MM-DD_00_AddUser_ReadById.sql --- Use CREATE OR ALTER for idempotency -CREATE OR ALTER PROCEDURE [dbo].[User_ReadById] - @Id UNIQUEIDENTIFIER -AS -BEGIN - SET NOCOUNT ON - SELECT * FROM [dbo].[User] WHERE [Id] = @Id -END -``` - -### Adding a NOT NULL column - -```sql --- CORRECT — metadata-only operation, no table scan -ALTER TABLE [dbo].[Organization] - ADD [UseCustomPermissions] BIT NOT NULL CONSTRAINT DF_Organization_UseCustomPermissions DEFAULT 0 - --- WRONG — causes full table scan on large tables -ALTER TABLE [dbo].[Organization] ADD [UseCustomPermissions] BIT NULL -UPDATE [dbo].[Organization] SET [UseCustomPermissions] = 0 -ALTER TABLE [dbo].[Organization] ALTER COLUMN [UseCustomPermissions] BIT NOT NULL -``` - -### Adding parameters to existing procedures - -```sql --- CORRECT — existing callers won't break -CREATE OR ALTER PROCEDURE [dbo].[Cipher_Create] - @Id UNIQUEIDENTIFIER, - @NewField NVARCHAR(MAX) = NULL -- default protects existing callers - --- WRONG — breaks all existing callers immediately -CREATE OR ALTER PROCEDURE [dbo].[Cipher_Create] - @Id UNIQUEIDENTIFIER, - @NewField NVARCHAR(MAX) -- no default = required parameter -``` - -## Further Reading - -- [SQL code style](https://contributing.bitwarden.com/contributing/code-style/sql/) -- [Database migrations (MSSQL)](https://contributing.bitwarden.com/contributing/database-migrations/mssql) diff --git a/plugins/bitwarden-software-engineer/skills/implementing-ef-core/SKILL.md b/plugins/bitwarden-software-engineer/skills/implementing-ef-core/SKILL.md deleted file mode 100644 index b643e0f..0000000 --- a/plugins/bitwarden-software-engineer/skills/implementing-ef-core/SKILL.md +++ /dev/null @@ -1,113 +0,0 @@ ---- -name: implementing-ef-core -description: Implementing Entity Framework Core repositories and migrations for PostgreSQL, MySQL, and SQLite at Bitwarden. Use when creating or modifying EF repositories, generating EF migrations, or working with non-MSSQL data access in the server repo. ---- - -## Repository Pattern - -EF implementations live in `src/Infrastructure/EntityFramework/Repositories/`. Each class implements the same interface as its Dapper counterpart. The EF repository uses `DbContext` and LINQ queries instead of stored procedures, but must produce identical behavior. - -### Why behavior must match stored procedures exactly - -Bitwarden self-hosted runs on the customer's choice of database. If `CipherRepository.GetManyByUserId()` returns results in a different order on PostgreSQL than the stored procedure returns on MSSQL, or filters differently, or handles nulls differently — that's a bug. Users switching databases or comparing behavior across environments will see inconsistencies. - -The `[DatabaseData]` integration test attribute runs the same test against all configured databases. This is the primary safety net for parity. - -### Cross-database considerations - -EF Core's LINQ-to-SQL translation varies by provider. Patterns that work on one database may fail on another: - -- **PostgreSQL** is stricter about types — operations like `Min()` on booleans or implicit string/int conversions that MySQL allows will throw -- **SQLite** has limited ALTER TABLE support — some migrations that work elsewhere fail on SQLite -- **Case sensitivity** depends on database collation, not on C# code — don't assume case-insensitive string comparison - -The pragmatic approach: write clean LINQ, run `[DatabaseData]` tests, and fix provider-specific failures as they surface rather than trying to predict every edge case. - -## Migration Generation - -### Workflow - -Run `pwsh ef_migrate.ps1 ` to generate migrations for all EF targets simultaneously. This creates migration files for each provider (PostgreSQL, MySQL, SQLite). - -### Why the migration name matters - -The EF migration class name must exactly match the MSSQL migration name portion (from the `YYYY-MM-DD_##_MigrationName.sql` filename). This convention keeps migration history aligned across ORMs and makes it easy to trace which EF migration corresponds to which SQL script. - -### Always review generated migrations - -EF's migration generator makes mechanical decisions that aren't always optimal: - -- It may drop and recreate indexes instead of renaming them -- It may generate unnecessary column modifications when model annotations change -- It doesn't know about Bitwarden's large table concerns (never add indexes to `Cipher`, `OrganizationUser` etc. without careful review) - -Review the generated `Up()` and `Down()` methods to ensure they align with the stored procedure migration's intent. - -## Key Decisions That Trip Up AI Assistants - -### Don't add navigation properties casually - -EF navigation properties (e.g., `public virtual Organization Organization { get; set; }`) affect query generation and lazy loading behavior. Only add them when the stored procedure equivalent also joins those tables. Unnecessary navigation properties cause N+1 queries that don't match the stored procedure's behavior. - -### DbContext configuration lives in `EntityTypeConfiguration` classes - -Don't configure entities inline in `OnModelCreating`. Each entity has a configuration class that defines table mapping, relationships, and constraints. This keeps the DbContext clean and each entity's configuration self-contained. - -### Respect the same GUID generation strategy - -Entity IDs are generated in application code via `CoreHelpers.GenerateComb()`, not by the database. Don't configure `ValueGeneratedOnAdd()` or database-generated defaults for ID columns in EF configuration. - -## Critical Rules - -These are the most frequently violated conventions. Claude cannot fetch the linked docs at runtime, so these are inlined here: - -- **One `EntityTypeConfiguration` class per entity** — never configure inline in `OnModelCreating` -- **Migration name must match MSSQL migration name** from `YYYY-MM-DD_##_MigrationName.sql` -- **Run `pwsh ef_migrate.ps1 `** to generate migrations for all providers simultaneously -- **Review `Up()` and `Down()` methods** in every generated migration before committing -- **No `ValueGeneratedOnAdd()` on ID columns** — IDs come from `CoreHelpers.GenerateComb()` in app code - -## Examples - -### GUID configuration - -```csharp -// CORRECT — ID generated in application code -public void Configure(EntityTypeBuilder builder) -{ - builder.HasKey(c => c.Id); - // No ValueGeneratedOnAdd — CoreHelpers.GenerateComb() handles this -} - -// WRONG — lets database generate IDs, breaks MSSQL parity -public void Configure(EntityTypeBuilder builder) -{ - builder.HasKey(c => c.Id); - builder.Property(c => c.Id).ValueGeneratedOnAdd(); -} -``` - -### Navigation properties - -```csharp -// CORRECT — only add when the SP also joins this table -public class Cipher -{ - public Guid Id { get; set; } - public Guid OrganizationId { get; set; } - // No navigation property — the SP doesn't JOIN Organization -} - -// WRONG — causes N+1 queries that don't match SP behavior -public class Cipher -{ - public Guid Id { get; set; } - public Guid OrganizationId { get; set; } - public virtual Organization Organization { get; set; } -} -``` - -## Further Reading - -- [Database migrations (EF)](https://contributing.bitwarden.com/contributing/database-migrations/ef) -- [SQL code style](https://contributing.bitwarden.com/contributing/code-style/sql/) diff --git a/plugins/bitwarden-software-engineer/skills/writing-client-code/SKILL.md b/plugins/bitwarden-software-engineer/skills/writing-client-code/SKILL.md deleted file mode 100644 index fbab2ac..0000000 --- a/plugins/bitwarden-software-engineer/skills/writing-client-code/SKILL.md +++ /dev/null @@ -1,114 +0,0 @@ ---- -name: writing-client-code -description: Bitwarden client code conventions for Angular and TypeScript. Use when working in the clients mono-repo, creating components, services, or modifying web/browser/desktop apps. ---- - -## Repository Orientation - -The `clients` mono-repo contains: - -- `apps/web`, `apps/browser`, `apps/desktop`, `apps/cli` — client applications -- `libs/common` — shared code for ALL clients including CLI (**no Angular dependencies** — CLI uses Node, not Angular DI) -- `libs/angular` — Angular-specific code for visual clients only -- `libs/components` — Angular Component Library (CL) - -### Why `libs/common` cannot import Angular - -CLI is a first-class client. Any code in `libs/common` must work without Angular's dependency injection, decorators, or lifecycle hooks. This is why cross-client services use abstract classes as interfaces — the concrete implementations (`Default*`, `Web*`, `Browser*`, `Desktop*`, `Cli*`) live in their respective apps. - -## Architectural Rationale - -### Thin components - -Components contain only view logic. Business logic belongs in services. This keeps components testable, reusable, and prevents Angular lifecycle coupling from leaking into domain logic. - -### Composition over inheritance - -Avoid extending components across clients. Compose using shared child components instead. Inheritance creates tight coupling between client-specific UI and shared behavior — when one client's needs diverge, inherited components become hard to change safely. - -### Don't modernize existing code unless asked - -The codebase contains both legacy and modern Angular patterns. When modifying an existing file, **follow the patterns already in that file**. Don't migrate any of these unless explicitly asked: - -- `*ngIf` → `@if`, `*ngFor` → `@for` -- `@Input()` / `@Output()` → `input()` / `output()` signals -- Constructor injection → `inject()` -- Default change detection → `OnPush` -- NgModule declarations → standalone components - -If asked to modernize, follow this order (per the Angular migration guide): standalone → control flow → input/output signals → view queries → signals → computed → OnPush (last, only after full signal migration). - -### State management: Signals vs RxJS - -- **Component local state and Angular-only services:** Use Signals -- **Cross-client services (`libs/common`):** Use RxJS (because CLI has no Angular Signals support) - -Avoid manual subscriptions. Prefer `| async` pipe. When subscriptions are necessary, pipe through `takeUntilDestroyed()` — enforced by the `prefer-takeUntil` lint rule. - -### No TypeScript enums (ADR-0025) - -Use frozen const objects with `Object.freeze()` and `as const`, plus a companion type alias. Enums have runtime behavior that creates subtle bugs with tree-shaking. - -## Critical Rules for New Code - -These rules apply **strictly to new files and components**. For existing code, follow the patterns already in the file. - -- **New components must use `ChangeDetectionStrategy.OnPush`** and be `standalone: true`. `NgModules` are permitted only for grouping related standalone components -- **Prefer `inject()` function** for DI in Angular primitives (components, pipes, directives). Use constructor injection for code shared with non-Angular clients (CLI) -- **New templates must use control flow syntax** (`@if`, `@for`, `@switch`), not structural directives -- **Use `host` property** in component decorators, not `@HostBinding` / `@HostListener` -- **Use Reactive Forms exclusively** — not template-driven forms -- **File naming:** `kebab-case.component.ts`, `.service.ts`, `.pipe.ts`, `.directive.ts`. Also: `.request.ts`, `.response.ts`, `.view.ts`, `.data.ts` for models (ADR-0012) -- **All Tailwind classes require `tw-` prefix** — `tw-flex`, `tw-mt-2`, not `flex`, `mt-2` -- **Testing with Jest** — use `jest-mock-extended` for mocking services. `describe`/`it` blocks, not `test()` -- **Imports from `@bitwarden/common`** must not pull in Angular-specific code (breaks CLI) - -## Examples - -### Dependency injection (new Angular code) - -```typescript -// CORRECT — inject() for Angular primitives -export class VaultComponent { - private vaultService = inject(VaultService); -} - -// ALSO CORRECT — constructor injection for code shared with CLI -export class CryptoService { - constructor(private stateService: StateService) {} -} -``` - -### Tailwind prefix - -```html - -
- -
-
-``` - -### Const objects over enums (ADR-0025) - -```typescript -// CORRECT — with companion type alias -export const CipherType = Object.freeze({ - Login: 1, - SecureNote: 2, -} as const); -export type CipherType = (typeof CipherType)[keyof typeof CipherType]; - -// WRONG — TypeScript enums have runtime side effects -export enum CipherType { - Login = 1, - SecureNote = 2, -} -``` - -## Further Reading - -- [Angular conventions](https://contributing.bitwarden.com/contributing/code-style/web/angular) -- [Angular migration guide](https://contributing.bitwarden.com/contributing/code-style/web/angular-migration-guide) -- [TypeScript style](https://contributing.bitwarden.com/contributing/code-style/typescript/) -- [Component Library](https://contributing.bitwarden.com/contributing/ui/component-library/) diff --git a/plugins/bitwarden-software-engineer/skills/writing-database-queries/SKILL.md b/plugins/bitwarden-software-engineer/skills/writing-database-queries/SKILL.md deleted file mode 100644 index d83f9f3..0000000 --- a/plugins/bitwarden-software-engineer/skills/writing-database-queries/SKILL.md +++ /dev/null @@ -1,67 +0,0 @@ ---- -name: writing-database-queries -description: Bitwarden database architecture, migrations, and dual-ORM strategy. Use when working with .sql files, stored procedures, EF migrations, or database schema changes. ---- - -## Dual-ORM Architecture - -Bitwarden maintains two data access implementations, split by database provider: - -- **MSSQL:** Dapper with stored procedures -- **PostgreSQL, MySQL, SQLite:** Entity Framework Core - -These implementations are **mutually exclusive at runtime** — SQL Server uses only Dapper, while the other providers use only EF Core. Both implementations conform to the same repository interfaces. - -- When **adding new repository functionality**, implement it in **both** Dapper and EF Core (unless the feature is explicitly EF-only). -- When **modifying an existing stored procedure** in a backwards-compatible way (for example, adding a new parameter with a default), **EF Core changes are not required**. -- Some commercial features (for example, **Secrets Manager**) are **EF Core only**. - -## Evolutionary Database Design (EDD) - -Bitwarden Cloud uses a **no-rollback** approach to database deployments. The key implication: **server deployments can be rolled back, but database migrations cannot**, so migrations must be designed to avoid being a source of downtime. - -All MSSQL migrations live in `util/Migrator/DbScripts/` and execute in chronological order based on the migration filename (`YYYY-MM-DD_##_Description.sql`). - -> Note: You may see `util/Migrator/DbScripts_transition/` and `util/Migrator/DbScripts_finalization/` folders. These are not currently used; ignore them for now. - -Simple additive changes (new nullable column, new table, new stored procedure) typically require only a single migration script in `util/Migrator/DbScripts/`. - -### Stored procedure compatibility - -Stored procedure changes fall into two categories: - -- **Non-breaking (DEFAULT parameters):** Adding a parameter with a default value (e.g., `@NewParam BIT = NULL`) is backwards-compatible. Existing callers keep working; no `_V2` is needed. -- **Breaking (`_V2` versioning):** Required when result-set structure changes, calling patterns change (e.g., single result → multiple result sets), required parameters are added without defaults, or query semantics differ. Implement this by creating `ProcedureName_V2` while retaining the original procedure for backwards compatibility. - -Table-level breaking changes (removing columns, changing types) typically cascade into stored procedure changes and often require the `_V2` pattern. - -**Always defer to the developer on migration strategy.** The approach is complex and context-dependent. When a database change is needed, write the migration script and ask the developer whether `_V2` versioning or additional steps are required. - -## Key locations - -- `src/Sql/dbo` — Master schema source of truth -- `util/Migrator/DbScripts` — All migrations (single folder, chronological) - -## ORM-Specific Implementation - -When implementing Dapper repository methods, stored procedures, or MSSQL migration scripts, activate the `implementing-dapper-queries` skill. - -When implementing EF Core repositories, generating EF migrations, or working with PostgreSQL/MySQL/SQLite, activate the `implementing-ef-core` skill. - -## Critical Rules - -These are the most frequently violated conventions. Claude cannot fetch the linked docs at runtime, so these are inlined here: - -- **Migration file naming:** `YYYY-MM-DD_##_Description.sql` (e.g., `2025-06-15_00_AddVaultColumn.sql`) -- **All schema objects use `dbo` schema** — never create objects in other schemas -- **Constraint naming:** `PK_TableName` (primary key), `FK_Child_Parent` (foreign key), `IX_Table_Column` (index), `DF_Table_Column` (default) -- **Idempotent scripts:** Use `IF NOT EXISTS` / `IF COL_LENGTH(...)` guards before schema changes in migration scripts -- **New repository functionality requires both Dapper and EF Core implementations** — unless the feature is explicitly EF-only or the change is a backwards-compatible stored procedure modification -- **Integration tests use `[DatabaseData]` attribute** — this runs the test against all configured database providers - -## Further Reading - -- [SQL code style](https://contributing.bitwarden.com/contributing/code-style/sql/) -- [Database migrations (MSSQL)](https://contributing.bitwarden.com/contributing/database-migrations/mssql) -- [Database migrations (EF)](https://contributing.bitwarden.com/contributing/database-migrations/ef) -- [Evolutionary Database Design](https://contributing.bitwarden.com/contributing/database-migrations/edd) diff --git a/plugins/bitwarden-software-engineer/skills/writing-server-code/SKILL.md b/plugins/bitwarden-software-engineer/skills/writing-server-code/SKILL.md deleted file mode 100644 index 394af15..0000000 --- a/plugins/bitwarden-software-engineer/skills/writing-server-code/SKILL.md +++ /dev/null @@ -1,88 +0,0 @@ ---- -name: writing-server-code -description: Bitwarden server code conventions for C# and .NET. Use when working in the server repo, creating commands, queries, services, or API endpoints. ---- - -## Repository Orientation - -The `server` repo contains: - -- `src/Api` — REST API endpoints -- `src/Identity` — Authentication/identity service -- `src/Core` — Business logic, commands, queries, services -- `src/Infrastructure` — Data access, repositories - -## Architectural Rationale - -### Command Query Separation (CQS) - -New features should use the CQS pattern — discrete action classes instead of large entity-focused services. See [ADR-0008](https://contributing.bitwarden.com/architecture/adr/server-CQRS-pattern). - -**Why CQS matters at Bitwarden:** The codebase historically grew around entity-focused services (e.g., `CipherService`) that accumulated hundreds of methods. CQS breaks these into single-responsibility classes (`CreateCipherCommand`, `GetOrganizationApiKeyQuery`), making code easier to test, reason about, and modify without unintended side effects. - -**Commands** = write operations. Change state, may return result. Named after the action: `RotateOrganizationApiKeyCommand`. - -**Queries** = read operations. Return data, never change state. - -**When NOT to use CQS:** When modifying existing service-based code, follow the patterns already in the file. Don't refactor to CQS unless explicitly asked. If asked to refactor, apply the pattern only to the scope requested. - -### Caching - -When caching is needed, follow the conventions in [CACHING.md](https://github.com/bitwarden/server/blob/main/src/Core/Utilities/CACHING.md). Use `IFusionCache` instead of `IDistributedCache`. - -**Don't implement caching unless requested.** If a user describes a performance problem where caching might help, suggest it — but don't implement without confirmation. - -### GUID Generation - -Always use `CoreHelpers.GenerateComb()` for entity IDs — never `Guid.NewGuid()`. Sequential COMBs prevent SQL Server index fragmentation that random GUIDs cause on clustered indexes, which is critical for Bitwarden's database performance at scale. - -## Critical Rules - -These are the most frequently violated conventions. Claude cannot fetch the linked docs at runtime, so these are inlined here: - -- **Use `TryAdd*` for DI registration** (`TryAddScoped`, `TryAddTransient`) — prevents duplicate registrations when multiple modules register the same service -- **File-scoped namespaces** — `namespace Bit.Core.Vault;` not `namespace Bit.Core.Vault { ... }` -- **Nullable reference types are enabled** (ADR-0024) — use `!` (null-forgiving) when you know a value isn't null; use `required` modifier for properties that must be set during construction -- **`Async` suffix on all async methods** — `CreateAsync`, not `Create`, when the method returns `Task` -- **Controller actions return `ActionResult`** — not `IActionResult` or bare `T` -- **Testing with xUnit** — use `[Theory, BitAutoData]` (not `[AutoData]`), `SutProvider` for automatic SUT wiring, and `Substitute.For()` from NSubstitute for mocking - -## Examples - -### GUID generation - -```csharp -// CORRECT — sequential COMB prevents index fragmentation -var id = CoreHelpers.GenerateComb(); - -// WRONG — random GUIDs fragment clustered indexes -var id = Guid.NewGuid(); -``` - -### DI registration - -```csharp -// CORRECT — idempotent, won't duplicate -services.TryAddScoped(); - -// WRONG — silently duplicates registration, last-wins causes subtle bugs -services.AddScoped(); -``` - -### Namespace style - -```csharp -// CORRECT — file-scoped -namespace Bit.Core.Vault.Commands; - -// WRONG — block-scoped -namespace Bit.Core.Vault.Commands -{ - // ... -} -``` - -## Further Reading - -- [C# code style](https://contributing.bitwarden.com/contributing/code-style/csharp/) -- [Server architecture](https://contributing.bitwarden.com/architecture/server/)