From 7c6e21c0c7b4940a170122fdb70477696078032b Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Mon, 6 Oct 2025 18:40:25 +0200 Subject: [PATCH 1/3] Initial create of a CLAUDE.md and a code review action --- .github/workflows/review-code.yml | 109 ++++++++++++++++++++++++++++++ CLAUDE.md | 30 ++++++++ 2 files changed, 139 insertions(+) create mode 100644 .github/workflows/review-code.yml create mode 100644 CLAUDE.md diff --git a/.github/workflows/review-code.yml b/.github/workflows/review-code.yml new file mode 100644 index 000000000..b49f5cec8 --- /dev/null +++ b/.github/workflows/review-code.yml @@ -0,0 +1,109 @@ +name: Review code + +on: + pull_request: + types: [opened, synchronize, reopened] + +permissions: {} + +jobs: + review: + name: Review + runs-on: ubuntu-24.04 + permissions: + contents: read + id-token: write + pull-requests: write + + steps: + - name: Check out repo + uses: actions/checkout@08c6903cd8c0fde910a37f88322edcfb5dd907a8 # v5.0.0 + with: + fetch-depth: 0 + persist-credentials: false + + - name: Check for Vault team changes + id: check_changes + run: | + # Ensure we have the base branch + git fetch origin ${{ github.base_ref }} + + echo "Comparing changes between origin/${{ github.base_ref }} and HEAD" + CHANGED_FILES=$(git diff --name-only origin/${{ github.base_ref }}...HEAD) + + if [ -z "$CHANGED_FILES" ]; then + echo "Zero files changed" + echo "vault_team_changes=false" >> $GITHUB_OUTPUT + exit 0 + fi + + # Handle variations in spacing and multiple teams + VAULT_PATTERNS=$(grep -E "@bitwarden/team-vault-dev(\s|$)" .github/CODEOWNERS 2>/dev/null | awk '{print $1}') + + if [ -z "$VAULT_PATTERNS" ]; then + echo "⚠️ No patterns found for @bitwarden/team-vault-dev in CODEOWNERS" + echo "vault_team_changes=false" >> $GITHUB_OUTPUT + exit 0 + fi + + vault_team_changes=false + for pattern in $VAULT_PATTERNS; do + echo "Checking pattern: $pattern" + + # Handle **/directory patterns + if [[ "$pattern" == "**/"* ]]; then + # Remove the **/ prefix + dir_pattern="${pattern#\*\*/}" + # Check if any file contains this directory in its path + if echo "$CHANGED_FILES" | grep -qE "(^|/)${dir_pattern}(/|$)"; then + vault_team_changes=true + echo "✅ Found files matching pattern: $pattern" + echo "$CHANGED_FILES" | grep -E "(^|/)${dir_pattern}(/|$)" | sed 's/^/ - /' + break + fi + else + # Handle other patterns (shouldn't happen based on your CODEOWNERS) + if echo "$CHANGED_FILES" | grep -q "$pattern"; then + vault_team_changes=true + echo "✅ Found files matching pattern: $pattern" + echo "$CHANGED_FILES" | grep "$pattern" | sed 's/^/ - /' + break + fi + fi + done + + echo "vault_team_changes=$vault_team_changes" >> $GITHUB_OUTPUT + + if [ "$vault_team_changes" = "true" ]; then + echo "" + echo "✅ Vault team changes detected - proceeding with review" + else + echo "" + echo "❌ No Vault team changes detected - skipping review" + fi + + - name: Review with Claude Code + if: steps.check_changes.outputs.vault_team_changes == 'true' + uses: anthropics/claude-code-action@a5528eec7426a4f0c9c1ac96018daa53ebd05bc4 # v1.0.7 + with: + anthropic_api_key: ${{ secrets.ANTHROPIC_API_KEY }} + track_progress: true + prompt: | + REPO: ${{ github.repository }} + PR NUMBER: ${{ github.event.pull_request.number }} + TITLE: ${{ github.event.pull_request.title }} + BODY: ${{ github.event.pull_request.body }} + AUTHOR: ${{ github.event.pull_request.user.login }} + + Please review this pull request with a focus on: + - Code quality and best practices + - Potential bugs or issues + - Security implications + - Performance considerations + + Note: The PR branch is already checked out in the current working directory. + + Provide detailed feedback using inline comments for specific issues. + + claude_args: | + --allowedTools "mcp__github_inline_comment__create_inline_comment,Bash(gh pr comment:*),Bash(gh pr diff:*),Bash(gh pr view:*)" diff --git a/CLAUDE.md b/CLAUDE.md new file mode 100644 index 000000000..5e25ba1a7 --- /dev/null +++ b/CLAUDE.md @@ -0,0 +1,30 @@ +# Bitwarden Internal SDK + +Rust SDK centralizing business logic **Internal use only** + +## Client Pattern + +PasswordManagerClient ([bitwarden-pm](crates/bitwarden-pm/src/lib.rs)) wraps +[bitwarden_core::Client](crates/bitwarden-core/src/client/client.rs) and exposes sub-clients: +`auth()`, `vault()`, `crypto()`, `sends()`, `generator()`, `exporters()`. + +**Lifecycle**: Init → Lock/Unlock → Logout (drops instance). Memento pattern for state resurrection. + +**Storage**: Consuming apps use `HashMap`. + +## Crate Organization + +- `bitwarden-core` - Core Client struct (avoid editing, use feature crates) +- `bitwarden-crypto` - Crypto primitives (edit with extreme care, multi-team ownership) + - `derive_*` = deterministic key derivation, `make_*` = non-deterministic generation + - Memory zeroed on drop by default +- `bitwarden-{auth,vault,send,generators}` - Domain features +- `bitwarden-uniffi` - Mobile bindings (no lifetimes in FFI types) +- `bitwarden-wasm-internal` - Web bindings (no logic, only conversions) +- `bitwarden-api-*` - Auto-generated (regenerate via `./support/build-api.sh`, revert Cargo.toml) + +## Non-Obvious Constraints + +- Clippy allows `.unwrap()` and `.expect()` in tests only +- `bitwarden-wasm-internal` must remain logic-free (business logic belongs in feature crates) +- Serializable crypto representations must maintain backward compatibility indefinitely From fa8627ff9970b25fbcbb706e13c742ac486987f9 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Tue, 7 Oct 2025 11:26:50 +0200 Subject: [PATCH 2/3] Rewrite the code review directions --- CLAUDE.md | 53 ++++++++++++++++++++++++++++++++++++++--------------- 1 file changed, 38 insertions(+), 15 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 5e25ba1a7..f984eb4e4 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,30 +1,53 @@ # Bitwarden Internal SDK -Rust SDK centralizing business logic **Internal use only** +Rust SDK centralizing business logic. You're reviewing code as a senior Rust engineer mentoring +teammates. ## Client Pattern PasswordManagerClient ([bitwarden-pm](crates/bitwarden-pm/src/lib.rs)) wraps [bitwarden_core::Client](crates/bitwarden-core/src/client/client.rs) and exposes sub-clients: -`auth()`, `vault()`, `crypto()`, `sends()`, `generator()`, `exporters()`. +`auth()`, `vault()`, `crypto()`, `sends()`, `generators()`, `exporters()`. **Lifecycle**: Init → Lock/Unlock → Logout (drops instance). Memento pattern for state resurrection. **Storage**: Consuming apps use `HashMap`. -## Crate Organization +## Issues necessitating comments -- `bitwarden-core` - Core Client struct (avoid editing, use feature crates) -- `bitwarden-crypto` - Crypto primitives (edit with extreme care, multi-team ownership) - - `derive_*` = deterministic key derivation, `make_*` = non-deterministic generation - - Memory zeroed on drop by default -- `bitwarden-{auth,vault,send,generators}` - Domain features -- `bitwarden-uniffi` - Mobile bindings (no lifetimes in FFI types) -- `bitwarden-wasm-internal` - Web bindings (no logic, only conversions) -- `bitwarden-api-*` - Auto-generated (regenerate via `./support/build-api.sh`, revert Cargo.toml) +**Auto-generated code changes:** -## Non-Obvious Constraints +- Changes to `bitwarden-api-api/` or `bitwarden-api-identity/` are generally discouraged. These are + auto-generated from swagger specs. -- Clippy allows `.unwrap()` and `.expect()` in tests only -- `bitwarden-wasm-internal` must remain logic-free (business logic belongs in feature crates) -- Serializable crypto representations must maintain backward compatibility indefinitely +**Secrets in logs/errors:** + +- Do not log keys, passwords, or vault data in logs or error paths. Redact sensitive data. + +**Business logic in WASM:** `bitwarden-wasm-internal` contains only thin bindings. Move business +logic to feature crates. + +**Unsafe without justification:** + +- Any `unsafe` block needs a comment explaining why it's safe and what invariants are being upheld. + +**Changes to `bitwarden-crypto/` core functionality** + +- Generally speaking, this crate should not be modified. Changes need a comment explaining why. + +**New crypto algorithms or key derivation** + +- Detailed description, review and audit trail required. Document algorithm choice rationale and + test vectors. + +**Encryption/decryption modifications** + +- Verify backward compatibility. Existing encrypted data must remain decryptable. + +**Breaking serialization:** + +- Backward compatibility required. Users must decrypt vaults from older versions. + +**Breaking API changes:** + +- Document migration path for clients. From 63df57156ffd7fec22c7f95b988337d0d0039a05 Mon Sep 17 00:00:00 2001 From: Mick Letofsky Date: Tue, 7 Oct 2025 17:02:05 +0200 Subject: [PATCH 3/3] Tidy up the use of colons --- CLAUDE.md | 23 ++++++++++++++--------- 1 file changed, 14 insertions(+), 9 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index f984eb4e4..8cb24ba65 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -9,25 +9,30 @@ PasswordManagerClient ([bitwarden-pm](crates/bitwarden-pm/src/lib.rs)) wraps [bitwarden_core::Client](crates/bitwarden-core/src/client/client.rs) and exposes sub-clients: `auth()`, `vault()`, `crypto()`, `sends()`, `generators()`, `exporters()`. -**Lifecycle**: Init → Lock/Unlock → Logout (drops instance). Memento pattern for state resurrection. +**Lifecycle** -**Storage**: Consuming apps use `HashMap`. +- Init → Lock/Unlock → Logout (drops instance). Memento pattern for state resurrection. + +**Storage** + +- Consuming apps use `HashMap`. ## Issues necessitating comments -**Auto-generated code changes:** +**Auto-generated code changes** - Changes to `bitwarden-api-api/` or `bitwarden-api-identity/` are generally discouraged. These are auto-generated from swagger specs. -**Secrets in logs/errors:** +**Secrets in logs/errors** - Do not log keys, passwords, or vault data in logs or error paths. Redact sensitive data. -**Business logic in WASM:** `bitwarden-wasm-internal` contains only thin bindings. Move business -logic to feature crates. +**Business logic in WASM** + +- `bitwarden-wasm-internal` contains only thin bindings. Move business logic to feature crates. -**Unsafe without justification:** +**Unsafe without justification** - Any `unsafe` block needs a comment explaining why it's safe and what invariants are being upheld. @@ -44,10 +49,10 @@ logic to feature crates. - Verify backward compatibility. Existing encrypted data must remain decryptable. -**Breaking serialization:** +**Breaking serialization** - Backward compatibility required. Users must decrypt vaults from older versions. -**Breaking API changes:** +**Breaking API changes** - Document migration path for clients.