clarify CQRS handler-error semantics in go-cqrs.md - #1
Merged
Conversation
Add a 'Handler Errors Do Not Cause Kafka Replay' section explaining the result-sender wrapper behavior: handler errors are caught, a single Failure result is emitted, and the offset commits normally. This removes the common misconception that returning err triggers infinite kafka replay. Reframe the err vs ErrCommandObjectSkipped guidance around result-topic noise rather than retry behaviour (both commit the offset; they differ in what they put on the result topic). References bborbe/trading#125 — the 228 visible failures on one actualTrade were 228 distinct kafka messages from a publisher without state pre-filter, not retries of one offset.
Address PR #1 review: replace bborbe/trading#125 / actualtrade / InvalidStateError reference block with a generic order-processing example. Drop trading#125 trailer from the changelog entry. Per CLAUDE.md "General-Purpose Content Only" rule.
Address PR #1 re-review: replace `cdb_command-object-executor-tx-result-sender.go` with the generic "result-sender wrapper" — surfacing an external library's internal filename creates fragile coupling that silently rots on refactor.
Address PR #1 round-3 nits: - Line 94: "the only path where offsets do NOT commit" was too absolute; hedge to "in normal error-handling paths" and call out process-level failures (panic, SIGKILL) as a separate concern. - Line 101: replace "Filled" (trading-domain) with "Completed" per the General-Purpose Content Only rule.
Uh oh!
There was an error while loading. Please reload this page.
This was referenced May 31, 2026
5 tasks
bborbe added a commit
that referenced
this pull request
Jun 2, 2026
…e, patterns) Sixth Go bootstrap batch. 3 small Go docs (108-228 lines), 5 rules. Rules added (rules/index.json: 95 -> 100): go-mod-dependency-fix/* (owner: go-quality-assistant) - tidy-after-get-update (MUST) — always run 'go mod tidy' after 'go get -u'. Skipping it is the #1 cause of broken-build-after-merge. - exclude-over-cross-repo-replace (SHOULD) — for broken transitive upstream pinning, prefer 'exclude' over a cross-repo 'replace'. Cross-references go-mod-replace/no-cross-repo-replace. go-makefile/* (owner: go-quality-assistant) - test-not-build-for-verification (MUST) — use 'make test', never 'go build ./...' as THE verification step. go build catches compile errors only; make test wraps Ginkgo + race + coverage + errcheck. go-patterns/* (owner: go-quality-assistant) - bborbe-collection-ptr-not-helpers (MUST) — use collection.Ptr(), never custom stringPtr/intPtr/boolPtr helpers. One import line collapses 3-5 per-package helpers + scattered &[]T{x}[0] tricks. - switch-over-if-chain-for-dispatch (SHOULD) — use 'switch' with explicit cases + default-error for enum dispatch, not if/else chains. Surfaces missing cases at runtime; documents the closed value set. 100 total rules — round-number milestone for the rule-base. CLAUDE.md doc-agent table updated with 3 new mappings. Generic examples (User/Order/Item) throughout. No personal vault paths, no trading-domain terms. Pre-emptive grep clean. make build-index regenerated; check-index passes.
4 tasks
bborbe added a commit
that referenced
this pull request
Jun 2, 2026
MAJOR resolved: - cookie credential type re-added to no-sensitive-data-in-logs applies_when list (was 'full session cookie' in the original draft; got dropped during the round-2 regex/applies_when expansion). Enforcement regex now includes 'cookie|set[_-]?cookie'. - compound *Secret (clientSecret, webhookSecret, signingSecret) added to enforcement regex via '\b(private|signing|encryption| api|client|webhook)(Key|Secret)\b' alternation. Bot MAJOR #1 not addressed (owner mismatch claim): bot reported 'docs line 127 says Owner: go-quality-assistant'. Verified — doc line 127 is 'Owner: go-security-specialist' (correct), matching index.json. The bot's claim is a hallucination on this round. NITs addressed: - external-call-logs-response Good example: added 'start := time.Now()' + elapsed_ms to both branches per rule's Why paragraph stating latency is part of the minimum payload - Added DB / gRPC / subprocess Good-example variants showing the same one-log-line-per-call shape (replaces the prose 'External Calls' section that was lost during bootstrap) - slog 'connecting' Good example now logs host + db name in addition to dsn_len (operator can identify which database) - Enforcement field clarification: whole-struct %+v / %#v dumps are judgment-tier (ast-grep cannot mechanically correlate the format verb with the argument's struct type — agent reads fields) NIT not addressed: bot's 'add bare secret to regex' — already covered by the new (Key|Secret) compound alternation. Rule count unchanged: 131. make precommit clean.
3 tasks
3 tasks
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
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
*-resulttopic, and the kafka offset commits normally.errvsErrCommandObjectSkippedaround result-topic noise rather than retry semantics. Both commit the offset; they differ only in what they put on the result topic.Why
A 2026-05-14 bug investigation in
core/actualtrade/controllertraced repeated failures on a single trade to a publisher-side issue, not consumer-side replay. The misleading guide language pointed the original investigation at the wrong layer. Source confirmation:cdb_command-object-executor-tx-result-sender.go:31-67returns nil to the outer message handler after sending the Failure result.Test plan
make precommitpasses