Amend ADR 8: Clarify CQS Terminology

[eliykat]

🎟️ Tracking

N/A

📔 Objective

ADR 8 adopted CQRS for our server repository. There wasn't much other documentation at the time, so I wrote the server architecture page with examples and rules for this pattern.

However, in practice we have not followed CQRS architecture or the examples and rules that I wrote in the architecture page. This is for various reasons, but mostly, it seems like we never wanted to go full CQRS to begin with. We have previously discussed this in some long threads in the architecture channel and it has mostly caused confusion.

This PR started as a rewrite of the server architecture page to more closely align with our current practices. However, as this is effectively modifying the language of the previous ADR (although not so much the intent), I have paired it with a proposed ADR to record the decision.

These changes focus on what we do in practice today, but leaves more discretion to teams in how exactly they write their commands and queries. The goal is to make it more accurate and useful, while allowing teams to iterate on their own patterns.

⏰ Reminders before review

• Contributor guidelines followed
• All formatters and local linters executed and passed
• Written new unit and / or integration tests where applicable
• Protected functional changes with optionality (feature flags)
• Used internationalization (i18n) for all UI strings
• CI builds passed
• Communicated to DevOps any deployment requirements
• Updated any necessary documentation (Confluence, contributing docs) or informed the documentation
  team

🦮 Reviewer guidelines

• 👍 (:+1:) or similar for great changes
• 📝 (:memo:) or ℹ️ (:information_source:) for notes or general info
• ❓ (:question:) for questions
• 🤔 (:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed
  issue and could potentially benefit from discussion
• 🎨 (:art:) for suggestions / improvements
• ❌ (:x:) or ⚠️ (:warning:) for more significant problems or concerns needing attention
• 🌱 (:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt
• ⛏ (:pick:) for minor or nitpick changes

[claude]

Claude finished @eliykat's task —— View job

---

PR Review: Amend ADR 8 - Clarify CQS Terminology

• Read repository guidelines and changed files
• Analyze PR changes and review comments
• Provide comprehensive review feedback

---

Overview

This PR successfully clarifies the terminology from CQRS (Command Query Responsibility Segregation) to CQS (Command Query Separation), which better reflects the actual implementation in the Bitwarden server codebase. The changes are well-motivated and align the documentation with current practices.

Key Changes

1. ADR-0008 - Updated title and content to use CQS terminology with a clear note explaining the terminology change
2. Server Architecture docs - Significantly rewritten to be less prescriptive and more aligned with current practices
3. Custom words - Added "CQS" to the dictionary
4. Angular guide - Fixed broken ADR reference
5. ADR index - Updated example to reflect CQS terminology

Review Feedback

✅ Strengths

1. Clear terminology note: The note block in ADR-0008 effectively explains the terminology change without creating confusion
2. Simplified guidance: The server architecture page is now more practical and less prescriptive, which should help it stay current
3. Maintains history: Rather than creating a new ADR, consolidating into ADR-0008 preserves the historical context
4. Good cleanup: Fixed the broken ADR-28 reference in docs/contributing/code-style/web/angular.md:215

📝 Observations & Discussion Points

Based on the review thread, I can see several points have been discussed:

Regarding the command/query distinction (line 17):
The current wording about commands returning "operation result only" is reasonable. The response to @theMickster's concern clarifies this aligns with Meyer's pragmatic approach where commands can return information about the write operation (like the updated object or status), not arbitrary queries.

Regarding prescriptiveness:
The response to @theMickster's question about "wide discretion" makes sense - the previous version was too prescriptive and quickly became outdated. The current approach provides guidance without being overly rigid.

Regarding repository methods as queries (line 68):
This is an interesting discussion point. The current text says "the 'query' is just a repository method" for simple cases. @theMickster expressed concern about this, but @eliykat asked for clarification on the alternative. This seems like it could benefit from further discussion:

• Should every repository method be wrapped in a query class for consistency?
• Or is it acceptable to call repository methods directly for simple reads?
• What's the threshold for when a separate query class is needed?

🎨 Minor Suggestions

1. Line numbering in examples (docs/architecture/server/index.md:54): The style guide mentions "Most often, I have seen our numbered lists in the Contributing Docs to follow the 1., 2., 3.." - you might want to verify this follows the established pattern.

2. Single method exposure (docs/architecture/server/index.md:49): @theMickster noted commands should only expose a single method like ExecuteAsync() or Run(). The current example uses RotateApiKeyAsync() which is action-specific. While the text says "only expose public methods that run the complete action," the discrepancy between current practice and the ideal could be noted if you want to address this in a future iteration.

⚠️ Items Requiring Attention

Previously raised issues that appear resolved:

• ✅ The "CQRS vs CQS motivation" wording has been improved
• ✅ The historical note about CQRS is being removed per discussion
• ✅ The sentence about "wide discretion" was removed
• ✅ Return value clarification has been improved

Still under discussion:

• The repository-as-query pattern (line 68) - awaiting further input from @theMickster on preferred approach

Recommendation

The PR is in good shape and represents a meaningful improvement to the documentation. The main remaining discussion point is the repository-as-query pattern, which seems like it might benefit from either:

1. Additional clarification on when to use each approach
2. A follow-up discussion/decision on the preferred pattern going forward

Overall: Approved with minor discussion point remaining ✅

---

[github-actions]

Checkmarx One – Scan Summary & Details – 9e5917ff-8106-4c8f-8951-c1d18e465172

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

[cloudflare-workers-and-pages]

Deploying contributing-docs with    Cloudflare Pages

Latest commit:	6246617
Status:	✅  Deploy successful!
Preview URL:	https://b56223b9.contributing-docs.pages.dev
Branch Preview URL:	https://adr0028-cqs-terminology.contributing-docs.pages.dev

View logs

[withinfocus]

If you made these changes to ADR 8 instead of a new one, could that work? I could be convinced otherwise but maybe it's best just to improve the original ADR given the refinement; it's not really a significant change in direction to me.

[withinfocus]

🎨 Can we improve this at the template level? Similar to #626, just have the details here and in a structured format so it's rendered consistently.

[eliykat]

This ADR is no longer superseded so I don't think this change is needed here; let me know if you disagree.

[eliykat]

Broken reference that was caught as part of this work. There is no ADR 28, this was presumably meant to reference the Signals ADR here: #632 however that has not been merged yet.

[withinfocus]

I'd omit this personally as it's not part of this PR's scope. This mention has been sitting for a bit because the Angular docs are a work-in-progress.

[theMickster]

I'm good with adjust the existing ADR, but I do respectfully request a few changes.

[theMickster]

🎨 I don't think that this sentence is needed because it detracts from the main goal of CQS.

This results in smaller classes with fewer interdependencies that are easier to change and test.

[eliykat]

I think this sentence belongs now, because the paragraph is discussing our motivation rather than the pattern itself. Let me know what you think.

[theMickster]

⚠️ I think the following note block detracts from the purpose of the document which is to direct the engineer as to what pattern to implement. I think it's best to be left in the ADR but removed from here.

[eliykat]

Lots of people have seen/used the CQRS language by now. I was concerned that developers would either be confused about the change in terminology, or miss why it's significant and keep referring to CQRS.

Publicizing it in the #architecture channel can help with this though, and that way we don't have a historical note on this page forever. I'll remove it.

[theMickster]

❓🤔 Curious, why do teams have wide discretion in structuring commands and queries? Shouldn't we strive for more homogenous flow and structure so our application feels like one application instead of many applications?

[eliykat]

"wide" is overstating it. But the first version of this document was very prescriptive, and we ended up breaking all of the rules in practice, and it became out of date very quickly. So my goal here is to sketch out some guidance without being overly prescriptive; that necessarily leaves it to teams to fill in the blanks. We can become more prescriptive, but that needs someone to review our current practices and requirements across teams, and then work with tech leads to align them.

Either way - I do think this sentence is superfluous so I'll remove it.

[eliykat]

@theMickster I've worked through your feedback. Our command/query structure is a bit weird and not necessarily consistent throughout our codebase, so I'm open to discussing it separately if you want to dig into it.

[withinfocus]

Nice clarification. There are a fair number of references to this ADR, especially with AI tooling lately, so while the path is technically incorrect now we should leave it.

One ⛏️ for something out of scope but not truly necessary to resolve.

[withinfocus]

I'd omit this personally as it's not part of this PR's scope. This mention has been sitting for a bit because the Angular docs are a work-in-progress.