feat(rules): bootstrap 3 Go doc families (http-service, cqrs, cli)#23
Merged
Conversation
Fourth Go bootstrap batch. 3 docs (118-215 lines after additions),
6 rules. Single bot review cycle.
Rules added (rules/index.json: 84 -> 90):
go-http-service/* (owner: go-http-handler-assistant)
- canonical-admin-endpoints (MUST) — every bborbe service's admin
HTTP server has /healthz, /readiness, /metrics, /setloglevel/{level},
/gc. Missing any one breaks the cross-service contract with
Kubernetes probes, Prometheus scrapes, on-call debugging.
- admin-port-9090 (MUST) — admin port is always 9090. Custom ports
break Prometheus scrape configs, gateway auto-routing, and
operator muscle memory.
go-cqrs/* (owner: go-architecture-assistant)
- auto-tx-wrapper-no-manual-wrap (MUST) — use RunCommandConsumerTx;
never manually wrap executors with NewTransactionMiddleware. Manual
wrapping duplicates the framework's transaction lifecycle and
produces double-rollback panics on closed txns.
- skipped-not-nil-for-non-retryable (MUST) — non-retryable conditions
return ErrCommandObjectSkipped, never nil (publishes false Success)
or arbitrary err (publishes N Failure entries on N duplicate
commands → noisy result topic + N error log lines + N alerts).
go-cli/* (owner: go-quality-assistant)
- cobra-not-stdlib-flag (MUST) — use cobra/pflag for CLI flag parsing.
Stdlib flag uses global flag.CommandLine which transitive deps
(most famously glog's init) pollute with 8+ unwanted flags
(-alsologtostderr, -log_dir, -v, etc).
- slog-not-glog-in-new-projects (MUST) — new CLI binaries use log/slog
(stdlib Go 1.21+). Legacy glog projects exempt — they should not
mix slog and glog. slog is structured by default and has no
flag.init() pollution.
CLAUDE.md doc-agent table updated with all 3 new mappings.
All examples generic (config/verbose flags, Order/User entities, etc).
No personal vault paths, no trading-domain terms. Pre-emptive grep clean.
make build-index regenerated; check-index passes.
![ben-s-pull-request-reviewer[bot]](https://avatars.githubusercontent.com/u/1193284?s=60&v=4){kind=small}
There was a problem hiding this comment.
Consolidated Report
Must Fix (Critical) — None.
Should Fix (Important)
| # | File | Line | Issue |
|---|---|---|---|
| 1 | docs/go-http-service-guide.md |
78 | Code won't compile: The Good example uses router.Handle("/healthz", handler) — gorilla/mux.Router has no Handle(path, handler) method. The doc's own canonical block (line 35) correctly uses router.Path("/healthz").Handler(...). Copy-pasting the Good example breaks the build. |
| 2 | docs/go-cqrs.md |
116 | Wrong function in Bad example: cqrs.RunCommandConsumer(ctx, consumer, wrapped) uses RunCommandConsumer but the rule is about RunCommandConsumerTx. The surrounding prose and the Good example reference RunCommandConsumerTx. Reader cannot reconcile which function is being warned against. |
| 3 | docs/go-cqrs.md |
142–151 | Unreachable code in Bad example: The skipped-not-nil-for-non-retryable Bad code block checks if order.Status == Completed twice in a row — second check is unreachable. Intent is to show two distinct wrong outcomes; structure makes it look like a copy-paste error. |
| 4 | docs/go-http-service-guide.md |
39 vs 78 | Signature mismatch: Canonical block (line 39): log.NewSetLoglevelHandler(ctx, log.NewLogLevelSetter(2, 5*time.Minute)). Good example (line 78): log.NewSetLoglevelHandler(2, 5*time.Minute) — different function arity and type. One of them is wrong against the actual library API. |
| 5 | docs/go-cli-guide.md |
90–103 | Execute() calls os.Exit(1): The Key Rules table (line 168) says "Run(ctx, args) returns error — testable without os.Exit." But the canonical Execute() function (lines 100–103) calls os.Exit(1) directly. A reader following this doc creates untestable code. os.Exit belongs in main(), not Execute(). |
| 6 | docs/go-cqrs.md |
116 | Package alias inconsistency: Bad example uses cqrs.*; all other code examples in the doc use cdb.* (matching the library github.com/bborbe/cqrs with cdb import alias). Pick one and use it consistently. |
Nice to Have (Optional)
| # | File | Line | Issue |
|---|---|---|---|
| 7 | docs/go-http-service-guide.md |
201 | go-skeleton/main.go is a bare backtick path, not a link — inconsistent with other References entries which use [text](path) format. |
| 8 | docs/go-http-service-guide.md |
89 | Endpoint Catalog lists /setloglevel/{level} library as log.NewSetLoglevelHandler but the canonical block shows it needs ctx + LogLevelSetter. Table entry is incomplete as a quick-reference. |
| 9 | docs/go-cli-guide.md |
175 | slog-not-glog-in-new-projects enforcement says "ast-grep partial" but the "new project" gate (module age / git history) cannot be done by ast-grep. Ast-grep would flag every glog import regardless of project age — the semantic exemption for existing projects requires judgment no tool can automate. |
| 10 | rules/index.json |
— | go-cqrs/skipped-not-nil-for-non-retryable (MUST) and go-http-service/canonical-admin-endpoints (MUST, for /gc and /setloglevel) carry high false-positive risk at MUST level. Consider splitting canonical-admin-endpoints into MUST (/healthz, /readiness, /metrics) and SHOULD (/gc, /setloglevel). |
{
"verdict": "request-changes",
"summary": "Three new Go doc families (http-service, cqrs, cli) and 6 corresponding rules added to rules/index.json. The docs are structurally sound and the rule index entries are valid JSON with correct owners. However, five code correctness issues must be fixed before merge: two code examples in go-http-service-guide.md use incorrect gorilla/mux APIs (router.Handle vs router.Path().Handler()), the cqrs.md Bad examples mix wrong function names and unreachable code, and the cli guide's canonical Execute() function calls os.Exit making it untestable against its own stated contract.",
"comments": [
{
"file": "docs/go-http-service-guide.md",
"line": 78,
"severity": "major",
"message": "go-http-service-guide.md | Good example uses `router.Handle(\"/healthz\", handler)` — gorilla/mux.Router has no such method. Use `router.Path(\"/healthz\").Handler(...)` to match the canonical block on line 35."
},
{
"file": "docs/go-http-service-guide.md",
"line": 78,
"severity": "major",
"message": "go-http-service-guide.md | `NewSetLoglevelHandler(2, 5*time.Minute)` signature mismatch: canonical block (line 39) uses `(ctx, NewLogLevelSetter(2, 5*time.Minute))` but the Good example passes `(int, duration)` — reconcile to one correct API."
},
{
"file": "docs/go-cqrs.md",
"line": 116,
"severity": "major",
"message": "go-cqrs.md | Bad example calls `cqrs.RunCommandConsumer(ctx, consumer, wrapped)` but the rule is about `RunCommandConsumerTx`. Change to `cdb.RunCommandConsumerTx` or whichever variant is being manual-wrapped."
},
{
"file": "docs/go-cqrs.md",
"line": 116,
"severity": "major",
"message": "go-cqrs.md | Package alias inconsistency: Bad example uses `cqrs.*` but all other examples in the doc use `cdb.*`. Use `cdb` throughout."
},
{
"file": "docs/go-cqrs.md",
"line": 147,
"severity": "major",
"message": "go-cqrs.md | Bad example has unreachable code: `if order.Status == Completed` appears twice in succession (lines 144 and 147). The second check is dead code — restructure as two separate Bad blocks or use separate conditions."
},
{
"file": "docs/go-cli-guide.md",
"line": 102,
"severity": "major",
"message": "go-cli-guide.md | `Execute()` calls `os.Exit(1)` directly (line 102), making it untestable. The Key Rules table (line 168) states Run() is testable without os.Exit — os.Exit should live in main(), not Execute(). Move os.Exit(1) to main() or a thin wrapper, keep Execute() error-returning."
},
{
"file": "docs/go-http-service-guide.md",
"line": 201,
"severity": "nit",
"message": "go-http-service-guide.md | go-skeleton/main.go reference is a bare backtick path; other References entries use markdown link format. Either link it or note it as an external repo."
},
{
"file": "docs/go-http-service-guide.md",
"line": 89,
"severity": "nit",
"message": "go-http-service-guide.md | Endpoint Catalog table lists `log.NewSetLoglevelHandler` as the library for /setloglevel but doesn't show the required LogLevelSetter wrapper. Quick-reference entry is incomplete."
},
{
"file": "docs/go-cli-guide.md",
"line": 175,
"severity": "nit",
"message": "go-cli-guide.md | slog-not-glog-in-new-projects enforcement says 'ast-grep partial' but the 'new project' distinction requires git history reasoning no tool can automate. The ast-grep partial will flag every glog import regardless of project age — the semantic exemption is agent-only."
},
{
"file": "rules/index.json",
"line": 376,
"severity": "nit",
"message": "rules/index.json | go-http-service/canonical-admin-endpoints marks /gc and /setloglevel as MUST alongside /healthz/readiness/metrics, but those two are service-dependent. Consider splitting into MUST (/healthz, /readiness, /metrics) and SHOULD (/gc, /setloglevel) to reduce false positives."
}
],
"concerns_addressed": [
"correctness: rules/index.json schema — verified valid JSON, all 6 rule IDs unique, owners match CLAUDE.md table, 7-field structure consistent across all 79 entries",
"correctness: http-service cross-service contracts — canonical-admin-endpoints rule is well-specified (5 endpoints, gateway annotations), but code examples have correctness bugs that must be fixed first",
"correctness: cqrs transaction lifecycle — auto-tx-wrapper rule is unambiguous, skipped-not-nil rule is well-explained, but Bad examples contain wrong function name and unreachable code",
"correctness: cli flag namespacing — cobra-not-stdlib-flag rule is clear and enforceable, but canonical Execute() example calls os.Exit making it untestable",
"tests: make precommit — cannot execute due to sandbox restrictions; rules/index.json manually verified as valid JSON with correct structure"
]
}Bot's review caught real API-shape inconsistencies between my new rule
examples and the canonical reference blocks in the same docs. All 5
MAJORs traceable to my examples; bot was right on each.
MAJOR (fixed):
1. go-http-service-guide Bad example used 'router.Handle("/x", h)' but
the canonical block earlier in the same doc uses
'router.Path("/x").Handler(h)'. Both APIs work in gorilla/mux but
the doc's convention is Path().Handler(). Aligned both Bad and Good.
2. go-http-service-guide log.NewSetLoglevelHandler signature: my example
passed (int, duration) but the canonical block uses
(ctx, log.NewLogLevelSetter(2, 5*time.Minute)). Updated to match.
3-4. go-cqrs Bad example used 'cqrs.*' package alias but the rest of
the doc uses 'cdb.*'. Also the Bad function call was
RunCommandConsumer (no Tx) — the rule is about RunCommandConsumerTx
so the example should call the Tx variant to demonstrate the
double-wrap smell. Aligned to cdb.RunCommandConsumerTx in both
the manual-wrap Bad and the Good example.
5. go-cqrs skipped-not-nil Bad example had unreachable code: two
'if order.Status == Completed' branches in sequence (second was
dead code). Split into two distinct '#### Bad (variant A)' and
'#### Bad (variant B)' blocks — one for 'return err' shape
(noisy Failure result), one for 'return nil' shape (lies Success).
Good remains a single block.
NOT fixed (deliberate):
6. Bot flagged go-cli Execute() calling os.Exit(1) as untestable.
Pre-existing canonical pattern in the doc, not introduced by this PR.
The split is intentional: Execute() is the un-testable wrapper that
exits on error, Run() returns the error and IS testable. The doc
explicitly notes this design (line ~116 of original). Out of scope.
NITs (skipped — not blocking):
- go-skeleton bare-backtick reference (cosmetic markdown style)
- Endpoint Catalog incomplete LogLevelSetter mention (covered by the
separate '/setloglevel/{level} — Constructor Args' section below)
- slog-not-glog 'ast-grep partial' vs git-history reasoning (the
caveat is in the enforcement field already — agent makes final call)
- /gc and /setloglevel MUST vs SHOULD split (defensible either way;
the doc's Endpoint Catalog marks all 5 as 'always required', so
MUST matches the doc's own classification)
make build-index regenerated; check-index passes.
ben-s-pull-request-reviewer
Bot
dismissed
their stale review
superseded by new automated review
There was a problem hiding this comment.
Based on my thorough review of the PR #23 diff (5 files, 280 insertions, 3 deletions), here is my consolidated report:
PR Review: feat(rules): bootstrap 3 Go doc families (http-service, cqrs, cli)
Automated Checks
- LICENSE file: Present ✓
- JSON validity (
rules/index.json): Structure is well-formed ✓ check-indexvalidation:build-index.pygeneratesrules/index.jsonfrom### RULEblocks — the 6 new rules have matching### RULEblocks in their respective docs, so index regeneration would be consistent ✓- Link validation: All
doc_pathreferences point to existing files ✓ - CLAUDE.md alignment table: 3 new entries correctly added for
go-http-service-guide.md→go-http-handler-assistant,go-cqrs.md→go-architecture-assistant,go-cli-guide.md→go-quality-assistant✓
Diff Quality Assessment
All 6 new rules follow the canonical ### RULE block structure with Owner, Applies when, Enforcement, Why, and BAD/GOOD examples:
go-cli/cobra-not-stdlib-flag(MUST/go-quality-assistant): Clear anti-pattern description; enforcement path (ast-grep partial) is realistic ✓go-cli/slog-not-glog-in-new-projects(MUST/go-quality-assistant): Enforcement correctly marked as "judgment (semantic)" — distinguishing new vs. existing projects mid-migration is not mechanically derivable; the doc honestly acknowledges this ✓go-cqrs/auto-tx-wrapper-no-manual-wrap(MUST/go-architecture-assistant): Accurately describesRunCommandConsumerTxas the framework's auto-wrapping entry point; the double-wrap risk (double-rollback panic) is a real bug pattern ✓go-cqrs/skipped-not-nil-for-non-retryable(MUST/go-architecture-assistant): Correctly documentsErrCommandObjectSkippedsemantics (silent skip, no result topic emission) vs. returningnil(Success) vs. returningerr(Failure) ✓go-http-service/canonical-admin-endpoints(MUST/go-http-handler-assistant): The doc's canonical server example correctly shows all 5 endpoints (/healthz,/readiness,/metrics,/setloglevel/{level},/gc) ✓go-http-service/admin-port-9090(MUST/go-http-handler-assistant): Correctly mandates:9090as the admin port default; the gateway routing note is appropriately scoped to the doc's security section ✓
Plan Concerns Addressed
| Concern | Resolution |
|---|---|
| JSON schema consistency, unique IDs | All 6 IDs unique (verified by grep count=1 each) ✓ |
go-cqrs/auto-tx-wrapper-no-manual-wrap — RunCommandConsumerTx method accuracy |
Doc accurately describes framework semantics from github.com/bborbe/cqrs ✓ |
go-cqrs/skipped-not-nil-for-non-retryable — ErrCommandObjectSkipped existence |
Doc correctly documents the error's behavior; this is framework documentation, not code ✓ |
go-http-service/canonical-admin-endpoints — 5 endpoints |
Doc's canonical example correctly shows all 5 ✓ |
go-cli/slog-not-glog-in-new-projects — judgment enforcement |
Enforcement honestly marked "judgment (semantic)" — this is accurate and not a deficiency ✓ |
| No test files changed | Expected — this is a docs/rules bootstrap PR; no Go/Python code to test ✓ |
admin-port-9090 gateway routing |
Security boundary is at the gateway (per doc); this is correctly documented ✓ |
No issues found.
{
"verdict": "approve",
"summary": "PR #23 cleanly bootstraps 3 new Go doc families (go-cli-guide, go-cqrs, go-http-service-guide) with 6 new rules (2 CLI, 2 CQRS, 2 HTTP service). All RULE blocks are structurally correct with proper Owner/Applies when/Enforcement/Why/GOOD/BAD fields. rules/index.json is consistent with the doc blocks. CLAUDE.md alignment table is correctly updated. No critical, major, or nit issues identified.",
"comments": [],
"concerns_addressed": [
"correctness: rules/index.json — 6 new rule IDs verified unique (1 occurrence each) and doc_path references point to existing files",
"correctness: docs/go-cqrs.md — auto-tx-wrapper-no-manual-wrap accurately describes RunCommandConsumerTx as the framework's auto-wrapping entry point",
"correctness: docs/go-cqrs.md — skipped-not-nil-for-non-retryable correctly documents ErrCommandObjectSkipped semantics (silent skip, no result topic emission)",
"correctness: docs/go-http-service-guide.md — canonical-admin-endpoints correctly lists all 5 required endpoints with the canonical server example",
"correctness: docs/go-cli-guide.md — slog-not-glog-in-new-projects honestly notes enforcement is 'judgment (semantic)' — accurate given the new-vs-existing project distinction requires git history",
"tests: rules/index.json — no test files changed is expected for a documentation/rules bootstrap PR",
"security: docs/go-http-service-guide.md — admin-port-9090 security model (gateway-level auth, cluster-internal only) is correctly documented"
]
}
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
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
Fourth Go bootstrap batch. 3 docs (118-215 lines), 6 rules.
`rules/index.json`: 84 → 90 entries (3 new families).
Rules added
All 6 rules are MUST-level — these docs are full of cross-service contracts (admin endpoints, CQRS transaction lifecycle, CLI flag namespacing) where deviation has clear production cost.
Pre-emptive checks
Test plan