Skip to content

Conversation

@theMickster
Copy link
Contributor

@theMickster theMickster commented Nov 27, 2025

🎟️ Tracking

📔 Objective

The updates to these various markdowns are the culmination of a number of iterations with Claude Code, Copilot and a few teammates to aid in better Claude Code Reviews. A general consensus is that the information that Claude has given thus far in our Rust SDK needs to improve. I attempted to combine many of the thoughts in the copilot-instructions.md into our CLAUDE.md because there was good information in that document.

I'm no Rust expert so I will need help in double-checking that the suggestions are valid.
Thanks!

… from Claude Code itself where it is getting lost-ish in our code base
@theMickster theMickster requested review from a team as code owners November 27, 2025 14:17
@claude
Copy link

claude bot commented Nov 27, 2025

Claude finished @theMickster's task —— View job


Review Summary

This PR significantly improves AI guidance documentation by consolidating and restructuring critical rules into a more accessible format. The changes transform the root CLAUDE.md from a comment-focused checklist into comprehensive architectural documentation, while new crate-specific CLAUDE.md files provide focused guidance for complex subsystems.


Critical Issues (1)

Finding 1: Duplicate crypto rules at .claude/CLAUDE.md:72-74 (copy/paste error from applying formatting suggestion)


Previous Review Items

Resolved:

  • ✅ WASM test command corrected per dani-garcia's feedback
  • ✅ Module path vs file path corrected (bitwarden_crypto::safe)
  • ✅ WASM derivation pattern includes Serialize/Deserialize
  • ✅ Crate documentation guidance improved

Outstanding from previous reviews:

  • Non-existent await_holding_invalid_type clippy lint references (lines 65, appears in comment threads but not blocking per quexten's approval)

Good Practices Observed

  • Well-structured four-layer architecture clearly delineates responsibilities
  • Crate-specific CLAUDE.md files are appropriately concise for AI guidance (not developer docs)
  • Security requirements section consolidates critical patterns
  • Development workflow commands provide practical validation steps

Action Items

  1. Remove duplicate crypto rules at lines 72-74 (see inline comment)
  2. Address prettier formatting error mentioned by quexten

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2025

Logo
Checkmarx One – Scan Summary & Details675d42fd-05dc-439d-a018-322dc12c7d09

Great job! No new security vulnerabilities introduced in this pull request

@github-actions
Copy link
Contributor

github-actions bot commented Nov 27, 2025

🔍 SDK Breaking Change Detection Results

SDK Version: ai/claude-markdown-suggestions (674cc0d)
Completed: 2025-12-01 14:51:45 UTC
Total Time: 246s

Client Status Details
typescript ✅ No breaking changes detected TypeScript compilation passed with new SDK version - View Details

Breaking change detection completed. View SDK workflow

@theMickster theMickster changed the title Suggested improvements to aid in better Claude Code results Refactor CLAUDE.md documentation to improve AI code review guidance Nov 27, 2025
@codecov
Copy link

codecov bot commented Nov 27, 2025

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 79.46%. Comparing base (f75a58c) to head (674cc0d).
⚠️ Report is 5 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #586      +/-   ##
==========================================
- Coverage   79.51%   79.46%   -0.05%     
==========================================
  Files         302      302              
  Lines       32311    32331      +20     
==========================================
  Hits        25691    25691              
- Misses       6620     6640      +20     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

theMickster and others added 2 commits November 27, 2025 16:15
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Copy link
Contributor

@quexten quexten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some small comments/suggestions on the main claude file. I did not review error-macro / ipc / state / threading since the other reviewers have a better grasp on these.

Also looks like there is a prettier formatting error, but that can be resolved when all other reviewers comments are resolved.

Other than that mostly looks good.

@theMickster theMickster requested review from a team and quexten December 1, 2025 09:36
Co-authored-by: claude[bot] <209825114+claude[bot]@users.noreply.github.com>
Comment on lines 72 to 74
- IMPORTANT: Use constant time equality checks
- Do not expose low-level / hazmat functions from the crypto crate.
- Do not expose key material from the crypto crate, use key references in the key store instead
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Duplicate crypto rules

Lines 72-74 are exact duplicates of lines 69-71. This duplication appears to have occurred when applying quexten's formatting suggestion - the line breaks were added correctly, but the original unformatted text wasn't removed.

Suggested change
- IMPORTANT: Use constant time equality checks
- Do not expose low-level / hazmat functions from the crypto crate.
- Do not expose key material from the crypto crate, use key references in the key store instead
### State Management (bitwarden-state)

This removes the duplicate rules and adds the proper blank line before the next section header.

quexten
quexten previously approved these changes Dec 1, 2025
Copy link
Contributor

@quexten quexten left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good for an initial round. I think we can iterate if we find it not focusing enough or focusing too much on some details.

withinfocus
withinfocus previously approved these changes Dec 1, 2025
Copy link
Contributor

@withinfocus withinfocus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Supporting approval after Bernd's review.

dani-garcia
dani-garcia previously approved these changes Dec 1, 2025
Copy link
Member

@dani-garcia dani-garcia left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me, just a minor nit that will cause WASM tests to fail if not run on the correct directory.


**WASM Testing:**

- `cargo test --target wasm32-unknown-unknown --all-features` - WASM-specific tests
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This command can't be run directly from root, as some crates depend on certain features that aren't available in WASM contexts. We should probably not use --all-features either as that would enable the UniFFI feature of some packages that is also not WASM compatible.

Instead, we have two options:

  • We indicate that this command should be run only in the directories of the relevant crates (at the moment that is bitwarden-error, bitwarden-threading, bitwarden-uuid)
  • We update the command to only run on the crates with wasm-specific tests:
cargo test --target wasm32-unknown-unknown --features wasm -p bitwarden-error -p bitwarden-threading -p bitwarden-uuid

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I'll update it now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated to reflect those changes

@theMickster theMickster dismissed stale reviews from dani-garcia, withinfocus, and quexten via 674cc0d December 1, 2025 14:38
@theMickster theMickster merged commit 868dace into main Dec 1, 2025
75 of 79 checks passed
@theMickster theMickster deleted the ai/claude-markdown-suggestions branch December 1, 2025 15:41
bw-ghapp bot pushed a commit to bitwarden/sdk-swift that referenced this pull request Dec 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants