bootstrap rules for go-factory-pattern.md (4 rules) - #4
Merged
Conversation
Daemon's first pass invented invalid ast-grep 0.43.0 syntax: rule-level `regex` matches the whole node text (not a specific field), and the multi-line `pattern:` blocks failed to fire at all. All three rules produced zero matches even on synthetic Bad cases — caught locally via `scripts/scan.sh` against /tmp/factory-test/sample.go before opening the PR (lesson from PR #3). Rewrote each rule using field-based `has` clauses against the function_declaration's `name` + `result` fields: - factory-no-error-return: match name=^Create + result=~error - factory-no-conditional-in-body: match name=^Create + body contains if/switch/for via stopBy:end - factory-no-cleanup-return: match name=^Create + result=~func\(\) Local smoke against 9-function synthetic file: - 4 expected Bad hits on rule 1 (CreateBadA/B/D/E) - 1 expected Bad hit on rule 2 (CreateBadC with the if) - 2 expected Bad hits on rule 3 (CreateBadD/E with func()) - 0 false positives on NewServiceX (constructor) or CreateGood* Index unchanged (only YAML internals changed).
There was a problem hiding this comment.
{
"verdict": "approve",
"summary": "The PR correctly bootstraps 4 new RULE blocks into go-factory-pattern.md (no-error-return, no-conditional-in-body, no-cleanup-return, no-impl-in-factory-pkg) with matching ast-grep YAML detectors and index.json entries. All three rule YAMLs are syntactically and semantically correct for ast-grep 0.43.0; the field-based `has` clauses are properly constructed. The index.json entries are well-structured with id, level (all MUST), owner, enforcement, applies_when, doc_path, and anchor fields. The RULE blocks have consistent owner, enforcement, why, and good/bad code examples. One environmental note: `make precommit` (link validation + JSON syntax check) could not be executed in this sandbox environment — it should be run locally before merging.",
"comments": [],
"concerns_addressed": [
"correctness: ast-grep YAML rules validated — field-based has-clauses are correct for ast-grep 0.43.0",
"correctness: index.json entries properly structured with id, level, owner, enforcement, applies_when, doc_path, anchor",
"correctness: RULE block IDs match filenames (factory-no-cleanup-return.yml → go-factory/no-cleanup-return, etc.)",
"correctness: docs/go-factory-pattern.md has 4 new RULE blocks with good/bad examples and enforcement paths",
"tests: no CI for ast-grep rules — documented as relying on local smoke test"
]
}Uh oh!
There was an error while loading. Please reload this page.
6 tasks
5 tasks
bborbe added a commit
that referenced
this pull request
Jun 2, 2026
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'.
4 tasks
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.
This was referenced Jun 2, 2026
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
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
Rules added
Scope: doc has ~10 distinct rules in the Checklist; this PR extracts 4 mechanically-tractable + 1 judgment. Skipped naming (Create*/New*), inline-business-logic overlap, boot-time validation, singletons, split-files — judgment-heavy or file-system checks.
Test plan