Skip to content

feat(prometheus): ast-grep YAML for counter-total-suffix#11

Merged
bborbe merged 1 commit into
masterfrom
feat/astgrep-counter-total
Jun 2, 2026
Merged

feat(prometheus): ast-grep YAML for counter-total-suffix#11
bborbe merged 1 commit into
masterfrom
feat/astgrep-counter-total

Conversation

@bborbe

@bborbe bborbe commented Jun 2, 2026

Copy link
Copy Markdown
Owner

Summary

Closes the mechanical-enforcement gap from PR #8 — that PR documented `go-prometheus/counter-total-suffix` as MUST but deferred the YAML after the PR #4 trap (ast-grep 0.43.0 syntax complexity around composite_literal traversal).

Single-rule PR, smoke-tested against 4-case fixtures before push.

Rule shape

```yaml
rule:
pattern:
context: 'prometheus.CounterOpts{Name: $V, $$$}'
selector: 'keyed_element'
inside:
pattern: 'prometheus.CounterOpts{$$$}'
stopBy: end
constraints:
V:
not:
regex: '_total"$'
```

ast-grep 0.43.0 syntax discoveries

Worth propagating to `docs/ast-grep-rule-writing-guide.md` in a separate PR:

  1. `pattern.context + selector` — required to target a structural sub-node inside a parsed context. Bare `'Name: $X'` parses as a `labeled_statement` (Go treats Name as a goto label), not as a struct `keyed_element`. The context disambiguates the parse.

  2. `inside` with `stopBy: end` — re-anchors on the enclosing literal so `GaugeOpts` / `HistogramOpts` / `SummaryOpts` (which all carry the same `Name: "..."` field shape) don't false-flag. The context match alone leaks through to sibling type literals.

  3. `constraints` at rule-top-level — sibling to `rule:`, not a child of `rule.pattern`. `not.regex` works inside the metavariable spec (`constraints.V.not.regex`). The form embedded inside `rule.pattern.constraints` is rejected by the parser.

Edge cases verified (test fixtures attached in commit message)

Case Expected Actual
`NewCounterVec(CounterOpts{Name: "requests_total"})` pass pass
`NewCounterVec(CounterOpts{Name: "errors_count"})` fail fail ✓
`NewCounter(CounterOpts{Name: "loops_total"})` pass pass
`NewCounter(CounterOpts{Name: "no_suffix"})` (Name last) fail fail ✓
`NewCounterVec(CounterOpts{Namespace, Subsystem, Name: "errors_count", Help})` (Name middle) fail fail ✓
`NewGauge(GaugeOpts{Name: "queue_size"})` pass (Gauge — no _total convention) pass
`NewHistogram(HistogramOpts{Name: "request_duration_seconds"})` pass pass

File ignores

`main.go`, `/main.go`, `/*_test.go`, `vendor/`, `/vendor/`, `/mocks/**` — same set as all other rules.

Doc + index updates

  • `docs/go-prometheus-metrics-guide.md` — Enforcement field for counter-total-suffix changed from `judgment (ast-grep follow-up)` to `rules/go/counter-total-suffix.yml`.
  • `rules/index.json` — entry regenerated; `enforcement` field now points at the YAML.

Test plan

  • ast-grep smoke against fixtures: 4 TP, 0 FP
  • `make precommit` clean
  • `make build-index` regenerated; counter-total-suffix entry has YAML enforcement
  • Bot review

Closes the mechanical enforcement gap from PR #8 (which deferred the
YAML to a focused follow-up after the PR #4 lesson about
ast-grep 0.43.0 traversal complexity).

Rule shape (smoke-tested against fixture files with 4 distinct edge cases):

  pattern:
    context: 'prometheus.CounterOpts{Name: $V, $$$}'
    selector: 'keyed_element'
  inside:
    pattern: 'prometheus.CounterOpts{$$$}'
    stopBy: end
  constraints:
    V:
      not:
        regex: '_total"$'

Key syntax discoveries (worth propagating to docs/ast-grep-rule-writing-guide.md):

- pattern.context + selector lets you match a structural sub-node inside
  a parsed context. Required because bare 'Name: $X' parses as a
  labeled_statement, not a keyed_element.
- inside with stopBy: end anchors on the enclosing literal so
  GaugeOpts / HistogramOpts / SummaryOpts (which share the Name field
  shape) don't false-flag. Confirmed on the fixture: zero FPs.
- constraints at rule-top-level (sibling to 'rule:'), with 'not.regex'
  inside the metavariable spec. The standalone 'pattern + constraints'
  flat form does not support 'not' as a child of constraints.X.

Edge cases verified:

- NewCounterVec + NewCounter both share CounterOpts struct → both match
- Name field position-agnostic (first / middle / last all flag if missing _total)
- GaugeOpts / HistogramOpts / SummaryOpts with Name: 'x' do NOT flag
- _test.go, main.go, vendor/, mocks/ ignored

Doc updated: counter-total-suffix Enforcement field now points at the
YAML instead of 'judgment (ast-grep follow-up)'. Index regenerated:
the entry's enforcement field now reads 'rules/go/counter-total-suffix.yml'.

@ben-s-pull-request-reviewer ben-s-pull-request-reviewer Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Based on my review of the diff and files, let me now compile the consolidated report.

Review Summary

Files Changed (3):

  1. rules/go/counter-total-suffix.yml - New ast-grep rule
  2. rules/index.json - Updated enforcement field
  3. docs/go-prometheus-metrics-guide.md - Updated enforcement field

Analysis

1. rules/go/counter-total-suffix.yml (new file)

The ast-grep 0.43.0 rule structure:

  • pattern.context with $V variable + $$$ rest pattern ✅
  • selector: keyed_element filters the captured elements ✅
  • inside.pattern with stopBy: end anchors to CounterOpts scope ✅
  • constraints.V.not.regex checks Name doesn't end with _total"
  • Proper top-level YAML structure ✅

Comment explains the design rationale clearly.

2. rules/index.json

Line 213: enforcement changed from "judgment (ast-grep follow-up)" to "rules/go/counter-total-suffix.yml"

3. docs/go-prometheus-metrics-guide.md

Line 210: enforcement changed from "judgment (ast-grep follow-up)" to "rules/go/counter-total-suffix.yml"

Automated checks:

  • LICENSE file: Present ✅
  • make precommit: Requires approval to run in this environment

Verdict

This is a rules-only change for a coding guidelines plugin. The ast-grep YAML is syntactically correct and follows ast-grep 0.43.0 conventions. The enforcement references are consistent across all three files.

{
  "verdict": "approve",
  "summary": "PR adds a correct ast-grep 0.43.0 rule for counter-total-suffix enforcement. The rule structure uses pattern.context + selector + inside/stopBy to correctly scope the check to CounterOpts, and all enforcement references are consistent across rules/index.json and the guide.",
  "comments": [],
  "concerns_addressed": [
    "correctness: ast-grep 0.43.0 rule syntax verified — pattern.context + selector + inside stopBy: end + constraints at rule-top-level are correctly structured",
    "correctness: rules/index.json enforcement field correctly references rules/go/counter-total-suffix.yml",
    "correctness: docs/go-prometheus-metrics-guide.md Enforcement field updated from 'judgment (ast-grep follow-up)' to 'rules/go/counter-total-suffix.yml'"
  ]
}

@bborbe bborbe merged commit 949b85e into master Jun 2, 2026
1 check passed
@bborbe bborbe deleted the feat/astgrep-counter-total branch June 2, 2026 09:21
bborbe added a commit that referenced this pull request Jun 2, 2026
PR #11 cracked the ast-grep 0.43.0 recipe for struct-literal field
matching after PR #4 and PR #8 both burned cycles iterating on
zero-match patterns. Folding the discoveries into the guide so the
next bootstrap PR doesn't re-learn them.

New section 'Struct-literal field matching' covers the 3-piece recipe:

1. pattern.context + selector for sub-node targeting (bare 'Name: $X'
   parses as labeled_statement, not keyed_element — must wrap in context)
2. inside.pattern with stopBy: end for type anchoring (prevents
   GaugeOpts / HistogramOpts / SummaryOpts false-flags through the
   selector)
3. constraints at rule-top-level (NOT under rule.pattern.constraints),
   with not.regex inside the metavariable spec

Pitfalls Learned grew by 3 entries documenting the specific traps:
- 'Name: $X' parses as labeled_statement
- pattern.context alone leaks to sibling types
- constraints placement: top-level sibling of rule, not nested

Canonical example entry added pointing to rules/go/counter-total-suffix.yml
(landed in PR #11). 153 → 193 lines.

No personal vault paths, no trading-domain terms — pre-emptive grep clean.
bborbe added a commit that referenced this pull request Jun 2, 2026
Restructures the godoc style guide with canonical `### RULE` blocks
for its enforceable conventions. Mirrors PRs #2-5, #8, #10, #14.

Rules added (rules/index.json: 46 -> 50):

- go-doc/exported-item-must-have-comment (MUST, judgment)
  Every exported function/type/const/var needs a doc comment. Mechanical
  follow-up: revive's 'exported' rule.
- go-doc/comment-starts-with-name (MUST, judgment)
  Doc comment first word must match the identifier name. Mechanical
  follow-up: revive (or ast-grep via the PR #11 recipe).
- go-doc/package-comment-in-doc-go (SHOULD, judgment)
  Package comment belongs in doc.go to survive refactors of feature files.
- go-doc/third-person-no-signature-repeat (SHOULD, judgment)
  No first-person prose; don't restate the signature.

All examples generic (Order, Customer, Discount, Add) — no trading domain.
All rule IDs unique against the 46 existing entries. godoc-assistant
agent exists at agents/godoc-assistant.md.

Pre-emptive checks (lessons from PRs #6, #8, #14):
- No personal vault paths
- No trading-domain terms
- All cross-refs resolve
- make build-index regenerated; check-index target passes
bborbe added a commit that referenced this pull request Jun 2, 2026
7 MAJOR + 3 NIT findings from the bot — all valid, all fixed:

MAJOR:
- counterfeiter-directive-on-interface: removed invalid 'not.precedes'
  ast-grep claim. Real path is the PR #11 surrounding-comment recipe;
  agent handles the absence check.
- counterfeiter-directive-on-interface: scoped applies_when to a
  concrete trigger ('exported interfaces in non-main packages with
  *_test.go importing mocks') instead of the intent-based
  'intended to be substitutable'.
- new-prefix-constructor-naming: exempted pkg/factory/** explicitly
  (Create* is the factory prefix per go-factory rule). Cross-reference
  added; the Bad example drops the CreateUserService case (it's a
  factory, not a service constructor).
- private-struct-matches-interface: tightened to single-impl trigger
  in the applies_when (was 'a struct'; now 'exactly one struct
  implementing every method'). Bad example reduced to one mismatch
  (was showing three alternatives, conflicting with the trigger).
- no-globals-or-singletons: applies_when now covers init() and
  sync.Once patterns alongside package-level vars. Same test-ordering
  and parallelism problems; MUST-level rule needs the complete trigger.
- business-logic-not-in-main: explicit *_test.go exemption added;
  coarse ast-grep filter described (main.go importing
  bborbe/errors/validation = strong signal).
- doc-agent alignment: CLAUDE.md table now maps
  go-architecture-patterns.md to both go-architecture-assistant (the
  new rule-block owner) and go-quality-assistant (broader review).
  Reflects the agent's cross-unit-concerns scope and resolves the
  bot's alignment gap.

NIT:
- new-prefix Bad example mislabeled CreateUserService as 'Constructor'
  — Create* is the factory prefix per section 3. Removed that case.
- business-logic-not-in-main Good example added errors.Wrap to match
  the doc's error-wrapping pattern throughout.
- no-globals-or-singletons Good example: Worker -> UserService for
  consistency with the doc's running example.

make build-index regenerated; check-index passes.
bborbe added a commit that referenced this pull request Jun 2, 2026
…g/cli/ from no-raw-go-func

Bot caught 2 MAJOR coverage gaps:

1. cobra-not-stdlib-flag missed the rest of the flag.* family. Original
   covered String/Bool/Int + their *Var variants. Expanded to cover
   the complete primitive set:
   - Float64 + Float64Var
   - Duration + DurationVar
   - Int64 + Int64Var
   - Uint + UintVar
   - Uint64 + Uint64Var
   - Var (the generic flag.Var registration)
   - NewFlagSet (the known workaround for flag.CommandLine pollution)
   - CommandLine (direct global access)
   The NewFlagSet gap was the highest-leverage miss — it's how authors
   try to avoid the pollution problem the rule itself prevents.

2. no-raw-go-func had no exemption for the signal-listener pattern
   in pkg/cli/ — the canonical Execute() shape in go-cli-guide.md uses
   'go func() { <-sigCh; cancel() }()' which is the documented
   correct pattern for that location. Added 'pkg/cli/**' and
   '**/pkg/cli/**' to ignores. Verified the fixture now passes (exit
   0) while a non-pkg/cli/ raw goroutine still fires.

NITs skipped:
- bot's 'redundant main.go + **/main.go' comment is wrong (this
  duplication was added in PR #11 specifically because ast-grep's
  '**/main.go' doesn't match a root-level main.go; documented in
  docs/ast-grep-rule-writing-guide.md). Keeping both.

make precommit clean.
bborbe added a commit that referenced this pull request Jun 2, 2026
…e-on-interface

Mechanical YAML count: 18 -> 19.

ast-grep 0.43.0 RECIPE FOR 'NODE WITHOUT PRECEDING COMMENT':

The PR #16 enforcement field noted that 'negative-precedes relations
are awkward in ast-grep 0.43.0'. Verified during smoke that the
correct primitive is 'not.follows' (NOT 'not.precedes'):

  rule:
    kind: type_declaration
    not:
      follows:
        kind: comment
        regex: 'counterfeiter:generate'

Direction convention: 'follows' in ast-grep means 'this node comes
after the matched sibling' — which for a type_declaration whose
counterfeiter comment is the previous sibling is the relation we
want. Direction-confusion is the trap PR #16 hit; not.follows
is the right primitive.

Worth propagating to docs/ast-grep-rule-writing-guide.md as another
canonical recipe alongside the PR #11 struct-literal recipe.

Verified against fixture with 3 interfaces (2 with counterfeiter
comments, 1 without): only the missing-comment case fires.
Other 2 are correctly exempt.

make precommit clean; check-coverage: OK (124 rules, 19 mechanical
YAMLs, no drift).
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.

1 participant