feat: add Structured Logging and CQRS standards#6
Merged
Conversation
Add two new organization-wide sections with agentic-friendly directives: - Structured Logging: JSON format, canonical fields, correlation/tracing, log levels, what to log/not log, Go (slog) and TypeScript (pino) patterns - CQRS: when to apply, command/query/event naming conventions, separation rules, idempotency, eventual consistency, GraphQL integration, testing Both sections include numbered "Agentic Directives" blocks with deterministic rules that AI coding agents can follow without ambiguity. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR expands the organization-wide AGENTS.md standards by introducing deterministic, agent-followable guidance for Structured Logging and CQRS, aiming to improve observability consistency and architectural correctness across services.
Changes:
- Adds a structured logging standard (JSON schema, canonical fields, correlation/tracing, level guidance, do/don’t lists, and language-specific patterns).
- Adds a CQRS standard (when to apply, separation rules, naming conventions, idempotency/outbox/event guidance, GraphQL mapping, and testing strategies).
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
📝 WalkthroughWalkthroughAdded organization-wide guidelines to Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes Possibly related PRs
🚥 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)
Comment |
…oke tests Add comprehensive E2E testing section to org-wide AGENTS.md that enforces testing real business outcomes through the full stack. Key additions: - Philosophy: every E2E test must answer "what would break for a real user?" - Forbidden patterns table: smoke tests disguised as E2E, UI-only assertions, mocked backends, status-code-only checks, arbitrary sleeps, happy-path-only - Required test structure: Arrange → Act → Assert → Verify → Cleanup - Multi-layer assertions: UI + API response + database state - GraphQL E2E: mutation→query round trips, auth on every resolver, pagination - Go backend E2E: testcontainers for real databases, migration testing, concurrency/idempotency testing - Mobile E2E: Detox/Maestro patterns, offline/online, testID selectors - 12 agentic directives for deterministic agent behavior Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 741-744: Update the "CQRS — Command Query Responsibility
Segregation" section to explicitly distinguish CQRS from event sourcing: add a
brief note under that header stating that CQRS only separates read/write models
and does not mandate how writes are persisted (stateful persistence or event
sourcing are both valid), and that event sourcing is a separate pattern which
records state as a sequence of events and can be used with or without CQRS;
include two concise bullet points reflecting these distinctions so readers of
the "CQRS — Command Query Responsibility Segregation" section won't conflate the
patterns.
- Around line 724-730: Add an explicit example showing how to attach a
request-scoped slog logger to a context in middleware so downstream code can use
slog.InfoContext; implement a LoggingMiddleware (referenced as
LoggingMiddleware) that generates or reads a request ID (requestID /
uuid.New().String()), creates a logger via slog.Default().With("request_id",
requestID), stores it on the request context using context.WithValue and a
loggerKey, and then calls next.ServeHTTP with r.WithContext(ctx) so handlers can
extract the logger and use slog.InfoContext(ctx, ...) or a helper to retrieve
the logger.
- Around line 796-802: The Outbox pattern mention is too brief; expand the
“Outbox pattern” section by adding a short implementation overview describing
writing events to an outbox table in the same DB transaction as the aggregate
update, an async publisher that polls/streams the outbox and publishes events,
and how to mark events as dispatched (idempotency/visibility); include one or
two authoritative references (e.g., Martin Fowler, transactional outbox patterns
or Debezium) and add a small “Language-specific examples” subsection (mirroring
the Structured Logging section) with brief notes for at least two ecosystems
(e.g., Java/Spring and Node.js) and pointers to implementation patterns (DB
transactions, polling workers, or CDC). Ensure you reference the existing terms
“Outbox pattern”, “outbox table”, “transactional outbox”, and “publish from the
outbox asynchronously” so reviewers can locate the new content.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 113-130: The fenced code block in the "Multi-Layer Assertion
Example" is opened without a language tag and without surrounding blank lines,
causing MD031/MD040 lint failures; fix by adding a language tag (e.g., ```ts) to
the opening fence and ensure there is a blank line before the opening ``` and a
blank line after the closing ``` so the block is properly separated from
surrounding text (apply to the triple-backtick fenced block shown in the
example).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
…contracts Add "Breaking Changes — Human Approval Required" subsection to Coding Standards. Technology-agnostic rules covering all layers: - What constitutes a breaking change (API, database, frontend, backend, shared contracts) with concrete examples table - Tests as the primary detection mechanism — existing tests encode contracts, never modify a test to accommodate a breaking change - Mandatory human approval gate: stop, describe, list impact, propose non-breaking alternative, wait for explicit approval - Non-breaking alternatives in priority order: additive changes, deprecation, feature flags, adapters, staged database migrations - 9 agentic directives (deterministic always/never rules) Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
♻️ Duplicate comments (1)
AGENTS.md (1)
114-130:⚠️ Potential issue | 🟡 MinorFix fenced code block markdown compliance.
The code block at line 114 violates MD031 (missing blank lines around fence) and MD040 (missing language identifier).
🔧 Proposed fix
**Multi-Layer Assertion Example:** + -``` +```ts // WRONG — only checks UI await page.click("#submit-order");🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@AGENTS.md` around lines 114 - 130, The fenced code block must include a language identifier and have blank lines before and after the triple backticks to satisfy MD040 and MD031; update the offending fence around the example (the block containing "await page.click..." / "expect(await page.getText...") to start with ```ts and ensure there is an empty line above the opening fence and an empty line below the closing fence so the block is properly separated from surrounding text.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@AGENTS.md`:
- Around line 114-130: The fenced code block must include a language identifier
and have blank lines before and after the triple backticks to satisfy MD040 and
MD031; update the offending fence around the example (the block containing
"await page.click..." / "expect(await page.getText...") to start with ```ts and
ensure there is an empty line above the opening fence and an empty line below
the closing fence so the block is properly separated from surrounding text.
…fic refs, address review comments E2E Testing section: - Replace "workflow" with "functional requirement" throughout - Remove all technology-specific references (Playwright, Detox, Maestro, testcontainers-go, httptest, errgroup, React Native, GraphQL) - Generalize subsections: "GraphQL E2E" → "API E2E", "Go Backend E2E" → "Backend E2E", "Mobile / React Native E2E" → "Frontend E2E (Web and Mobile)" - Use generic terms: "frontend", "backend", "database", "test-ID attributes" - Fix code block: add language identifier (pseudocode) for MD031/MD040 Logging section (addressing Copilot + CodeRabbit review comments): - Add logger initialization guidance for baseline fields (timestamp, service, version) — addresses Copilot comment on line 826 - Add correlation_id, causation_id to canonical field names with explicit relationship definitions linking to CQRS — addresses comments on lines 832, 962 - Clarify error_message vs err object serialization — addresses line 853 - Narrow sensitive field name matching from substring "key" to explicit suffixes (api_key, private_key, etc.) — addresses line 882 CQRS section: - Add "CQRS is not Event Sourcing" clarification — addresses CodeRabbit nitpick - Cross-reference correlation_id/causation_id back to Structured Logging Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Around line 884-885: Harden the sensitive-field detection rule by making the
match case-insensitive and matching across naming styles (snake_case, camelCase,
PascalCase, kebab-case) so names like apiKey, accessToken, authHeader,
refreshToken, cardNumber, api-key, api_key, etc. are caught; update the rule
that currently lists exact or ends-with names (password, secret, token, api_key,
private_key, secret_key, access_key, authorization, cookie, ssn, credit_card,
card_number) to instead check for those tokens as whole-word or substring
matches in a case-insensitive manner and include common camelCase variants and
hyphenated forms, and ensure any detection logic (used when deciding whether to
log a field) omits or redacts the field rather than logging it.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
Comment on lines
+884
to
+885
| 6. NEVER log any variable whose name exactly matches or ends with: `password`, `secret`, `token`, `api_key`, `private_key`, `secret_key`, `access_key`, `authorization`, `cookie`, `ssn`, `credit_card`, `card_number`. When in doubt, omit the field rather than risk leaking sensitive data. | ||
| 7. When adding retry logic, log each retry at WARN with `attempt_number` and `max_retries`. |
There was a problem hiding this comment.
Harden sensitive-field detection beyond snake_case suffixes.
The current “exact or ends with” list is too narrow and can miss common names like apiKey, accessToken, authHeader, or refreshToken, which weakens the no-secrets logging rule.
🔐 Suggested doc patch
-6. NEVER log any variable whose name exactly matches or ends with: `password`, `secret`, `token`, `api_key`, `private_key`, `secret_key`, `access_key`, `authorization`, `cookie`, `ssn`, `credit_card`, `card_number`. When in doubt, omit the field rather than risk leaking sensitive data.
+6. NEVER log any variable whose normalized field name indicates sensitive data.
+ Normalize names to lowercase and treat snake_case/camelCase/PascalCase/kebab-case equivalently.
+ Block fields that exactly match or end with: `password`, `secret`, `token`, `api_key`, `apikey`, `private_key`, `secret_key`, `access_key`, `authorization`, `auth_header`, `cookie`, `ssn`, `credit_card`, `card_number`.
+ When in doubt, omit the field rather than risk leaking sensitive data.As per coding guidelines "Never commit secrets; use environment variables, GitHub Actions secrets, or external secret managers instead".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` around lines 884 - 885, Harden the sensitive-field detection rule
by making the match case-insensitive and matching across naming styles
(snake_case, camelCase, PascalCase, kebab-case) so names like apiKey,
accessToken, authHeader, refreshToken, cardNumber, api-key, api_key, etc. are
caught; update the rule that currently lists exact or ends-with names (password,
secret, token, api_key, private_key, secret_key, access_key, authorization,
cookie, ssn, credit_card, card_number) to instead check for those tokens as
whole-word or substring matches in a case-insensitive manner and include common
camelCase variants and hyphenated forms, and ensure any detection logic (used
when deciding whether to log a field) omits or redacts the field rather than
logging it.
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
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.
Summary
Sections added
Structured Logging (~95 lines)
CQRS (~95 lines)
Test plan
🤖 Generated with Claude Code
Summary by CodeRabbit