feat(rules): 2 mechanical ast-grep YAMLs — no-raw-go-func + cobra-not-stdlib-flag

[bborbe]

Summary

Two new mechanical ast-grep YAMLs for high-leverage MUST rules previously in judgment-only mode. Each smoke-tested against fixtures before push.

`rules/index.json`: 124 rules unchanged. Mechanical YAML count: 15 → 17.

Rules promoted to mechanical

rule_id	Pattern shape	Verified
`go-concurrency/no-raw-go-func`	`kind: go_statement` (Go tree-sitter grammar's node for `go expr()`)	Catches both `go func() {...}()` AND `go doWork()` shapes
`go-cli/cobra-not-stdlib-flag`	`any:` over import_spec `"flag"` + call_expression patterns for flag.Parse / flag.String / flag.Bool / flag.Int + their *Var variants	Matches stdlib flag usage; clean on cobra-using fixture

Test plan

• Fixture smoke: bad case fires, good case clean
• `make precommit` clean (check-coverage now reports 17 mechanical YAMLs)
• Enforcement field in docs updated from "judgment (ast-grep follow-up: ...)" to the YAML path
• Bot review

[ben-s-pull-request-reviewer]

---

PR Review: feat(rules): 2 mechanical ast-grep YAMLs — no-raw-go-func + cobra-not-stdlib-flag

Scope

5 files changed:

• docs/go-cli-guide.md — enforcement updated from judgment → mechanical rule
• docs/go-concurrency-patterns.md — enforcement updated from judgment → mechanical rule
• rules/go/cobra-not-stdlib-flag.yml — new rule
• rules/go/no-raw-go-func.yml — new rule
• rules/index.json — enforcement strings updated

---

Must Fix (Critical)

None.

---

Should Fix (Important)

1. cobra-not-stdlib-flag.yml: Missing common flag.* function variants

The rule covers Parse, String, Bool, Int, StringVar, BoolVar, IntVar but misses other stdlib flag functions. The most significant gap is NewFlagSet — developers who know about flag.CommandLine pollution sometimes use flag.NewFlagSet as a workaround. It would slip through this rule despite being a violation of the doc's intent.

Also missing: Float64, Float64Var, Duration, DurationVar, Int64, Int64Var, Uint, UintVar, Uint64, Uint64Var, Var, CommandLine.

Recommendation: Expand call-site patterns to cover the full flag.* family, especially NewFlagSet.

---

2. no-raw-go-func.yml: No suppression mechanism for documented edge cases

The doc (go-concurrency-patterns.md) demonstrates a signal-listener goroutine pattern in pkg/cli/ (see go-cli-guide.md:95-98). This pattern is a legitimate go func() that runs for the process lifetime — it does not fit run.CancelOnFirstErrorWait etc. The rule's ignores cover main.go and cmd/** but not pkg/cli/**. A project following the doc's recommended CLI structure would get a false positive.

Recommendation: Add //nolint:go-concurrency/no-raw-go-func // <reason> as a documented suppression convention in the rule's message:, and add **/pkg/cli/** to ignores if that's the project's CLI composition root convention.

---

Nice to Have (Optional)

3. no-raw-go-func.yml: Redundant entries in ignores

The ignores block has both main.go and **/main.go — the bare main.go is redundant since **/main.go already matches at any depth. Same for cmd/** + **/cmd/**. This is consistent with other rules in the repo, so it's not a defect — just cleanup if the rule is touched.

4. no-raw-go-func.yml: Consider narrowing with metavariable constraints

kind: go_statement is blunt — it catches every go statement. Using tree-sitter metavariables to specifically match go func() { ... }() (closure spawn) and go <method>(...) (method-call spawn) would reduce false positives from legitimate but uncommon go patterns. This is a follow-up improvement, not a blocker.

---

Manual Review Notes

• Both YAML files are valid ast-grep 0.43.0 syntax and follow repo conventions
• The message: fields correctly cite the doc path and rule ID
• rules/index.json enforcement strings are consistent with the new mechanical rules
• Both rules correctly ignore vendor/**, mocks/**, and *_test.go

---

{
  "verdict": "request-changes",
  "summary": "Two new mechanical ast-grep rules correctly implement the judgment-tier enforcement from the docs, but both have coverage gaps: cobra-not-stdlib-flag misses the full flag.* family (especially NewFlagSet), and no-raw-go-func lacks a suppression mechanism for the documented signal-listener edge case in pkg/cli/. Neither gap is a security or correctness defect, but both will produce false negatives that undermine the rule's intent.",
  "comments": [
    {
      "file": "rules/go/cobra-not-stdlib-flag.yml",
      "line": 15,
      "severity": "major",
      "message": "Should Fix: Missing common flag functions — Float64, Float64Var, Duration, DurationVar, Int64, Int64Var, Uint, UintVar, Uint64, Uint64Var, Var, NewFlagSet, and CommandLine. The NewFlagSet gap is most significant since it's a known workaround for flag.CommandLine pollution that should be flagged."
    },
    {
      "file": "rules/go/no-raw-go-func.yml",
      "line": 15,
      "severity": "major",
      "message": "Should Fix: No suppression mechanism for documented edge cases. The signal-listener pattern in pkg/cli/ (demonstrated in go-cli-guide.md:95-98) is a legitimate go func() that doesn't fit run.* primitives. Without a //nolint:go-concurrency/no-raw-go-func convention, CI will false-positive on this documented pattern."
    },
    {
      "file": "rules/go/no-raw-go-func.yml",
      "line": 16,
      "severity": "minor",
      "message": "Nice to Have: Add **/pkg/cli/** to ignores if that's the project's CLI composition root convention (per go-cli-guide.md pattern)."
    },
    {
      "file": "rules/go/no-raw-go-func.yml",
      "line": 18,
      "severity": "minor",
      "message": "Nice to Have: Redundant ignores entries — main.go + **/main.go and cmd/** + **/cmd/**. The bare forms are unnecessary since the ** prefixed forms match at all depths."
    }
  ],
  "concerns_addressed": [
    "correctness: regex pattern '^\"flag\"$' in cobra-not-stdlib-flag — CORRECT, anchors are properly applied to the import_spec path field",
    "correctness: multiple flag.*($$$ARGS) call-site patterns in cobra-not-stdlib-flag — PARTIALLY ADDRESSED, missing Float64/Duration/NewFlagSet family",
    "correctness: kind: go_statement in no-raw-go-func — CORRECT but blunt; path ignores sufficient for main.go/cmd/ but miss pkg/cli/",
    "tests: cobra-not-stdlib-flag no fixture — NOTED, no test files added; rule is well-structured but untested",
    "tests: no-raw-go-func no fixture — NOTED, rule follows established patterns but untested"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

I now have a complete picture of the PR. Let me compile the final report.

{
  "verdict": "request-changes",
  "summary": "PR adds two mechanical ast-grep YAML rules with correct structural shape but contains three correctness issues: (1) `cobra-not-stdlib-flag` YAML catches stdlib `flag` usage in any non-test Go file while its `applies_when` text scopes it to CLI binaries only — scope mismatch; (2) `cobra-not-stdlib-flag` misses `flag.Func` (Go 1.16+ custom-flag-type API); (3) the `pkg/cli/**` exemption in `no-raw-go-func` is not mentioned in the doc's `applies_when` text. Additionally, the `commands/code-review.md` simplification removes the ast-grep runner and citation validation steps without a corresponding `rules/index.json` update to reflect the new agent calling convention.",
  "comments": [
    {
      "file": "rules/go/cobra-not-stdlib-flag.yml",
      "line": 1,
      "severity": "major",
      "message": "[go-cli/cobra-not-stdlib-flag] Scope mismatch: the `applies_when` text in `docs/go-cli-guide.md:8` and `rules/index.json:476` says the rule applies to 'a Go CLI binary's `main.go` / `pkg/cli/...`', but the YAML's `ignores` block only exempts `**/*_test.go`, `vendor/**`, and `**/mocks/**` — it matches `flag` usage in any non-test Go file. The `Why` paragraph correctly describes the global-pollution problem (a library calling `flag.String()` in `init()` pollutes every importing binary), which applies beyond CLI entry points. Fix: broaden `applies_when` text to 'any non-test Go file imports the stdlib flag package or calls flag.* functions', OR add `main.go`/`**/main.go`/`pkg/cli/**` path exemptions to the YAML ignores block to match the stated scope."
    },
    {
      "file": "rules/go/cobra-not-stdlib-flag.yml",
      "line": 19,
      "severity": "minor",
      "message": "[go-cli/cobra-not-stdlib-flag] `flag.Func($$$ARGS)` is missing from the pattern list. `flag.Func` (Go 1.16+) registers a flag with a custom value-setter function — it is a legitimate and increasingly common way to define custom flag types and would bypass this rule. Add `- pattern: 'flag.Func($$$ARGS)'` to close the gap."
    },
    {
      "file": "rules/go/no-raw-go-func.yml",
      "line": 21,
      "severity": "major",
      "message": "[go-concurrency/no-raw-go-func] The `pkg/cli/**` path exemption is not mentioned in the doc's `applies_when` text (`docs/go-concurrency-patterns.md:10` says only 'outside `main.go` / top-level entry points'). A reader comparing the doc to the YAML will find a discrepancy. Additionally, the exemption is potentially over-broad: CLI subcommand `RunE` functions may legitimately fan out goroutines for concurrent sub-tasks (e.g. streaming while waiting on a stop signal). Either: (a) remove `pkg/cli/**` from ignores and rely only on `main.go`/`cmd/**` exemptions, OR (b) add a comment in the YAML explaining the rationale and update `applies_when` to list the exemption."
    },
    {
      "file": "docs/go-cli-guide.md",
      "line": 8,
      "severity": "minor",
      "message": "[go-cli/cobra-not-stdlib-flag] `applies_when` text is inconsistent with the mechanical YAML scope — see finding for `rules/go/cobra-not-stdlib-flag.yml`."
    },
    {
      "file": "rules/index.json",
      "line": 476,
      "severity": "minor",
      "message": "[go-cli/cobra-not-stdlib-flag] `applies_when` text mismatch with YAML scope — same issue as `docs/go-cli-guide.md:8`."
    },
    {
      "file": "docs/go-concurrency-patterns.md",
      "line": 10,
      "severity": "minor",
      "message": "[go-concurrency/no-raw-go-func] `applies_when` does not mention the `pkg/cli/**` exemption present in the YAML ignores block — creates a doc/YAML discrepancy."
    },
    {
      "file": "commands/code-review.md",
      "line": 240,
      "severity": "major",
      "message": "[agent-cmd/command-thin] `commands/code-review.md` was significantly simplified: the dispatcher/ast-grep-runner pipeline (Step 4a-4d) was replaced with a direct agent-list invocation. This is a substantive architecture change to the review pipeline that was not flagged in the PR concerns. The change removes: (a) mechanical pre-filtering via `ast-grep-runner`, (b) per-owner LLM adjudication of pre-filtered findings, (c) citation validation via `scripts/validate-citations.sh`. The new approach calls agents directly on the diff without the mechanical funnel. This is acceptable as a simplification if intentional, but should be reviewed against the `agent-cmd/command-thin` rule (thin-wrapper requirement) and the dispatcher refactor rationale from PR #27."
    }
  ],
  "concerns_addressed": [
    "correctness: ast-grep YAML rules have no test files to validate pattern matching — not directly addressed, but static analysis confirms structural validity",
    "correctness: cobra-not-stdlib-flag YAML pattern list expanded in fix commit (f6c2e44) to cover full flag.* family including NewFlagSet and CommandLine — addressed",
    "correctness: index.json enforcement field correctly points to YAML files — verified consistent",
    "correctness: pkg/cli exemption added to no-raw-go-func in fix commit (f6c2e44) — addressed, but new concern raised: exemption not documented in applies_when",
    "correctness: scope mismatch between cobra-not-stdlib-flag applies_when text and YAML matching scope — raised as major finding",
    "correctness: flag.Func gap in cobra-not-stdlib-flag pattern list — raised as minor finding"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Based on my comprehensive review, here is my assessment:

Summary: This PR adds two new mechanical ast-grep rules (no-raw-go-func and cobra-not-stdlib-flag) with matching doc updates. Both adjudication agents confirmed the rules are correctly structured — the kind: go_statement correctly matches all go expr() forms per tree-sitter grammar, and the import_spec regex correctly handles both single and grouped import forms. The scope broadening for cobra-not-stdlib-flag is well-justified by the transitive flag.init() pollution concern. Minor nits about redundant patterns/globs were identified but are harmless.

{
  "verdict": "approve",
  "summary": "PR #29 adds two well-structured mechanical ast-grep rules. Both go-quality-assistant and go-architecture-assistant confirmed the rules are correctly designed: the tree-sitter kind matching is sound, the import_spec regex handles both import forms, and the scope expansions (any non-test Go file for cobra; pkg/cli/ exemption for no-raw-go-func) are justified by the documented failure modes. Two minor nits (redundant flag.Parse pattern, redundant glob pair) are harmless. Rule IDs, enforcement paths, and applies_when texts are all consistent between rules/index.json and the YAML/doc sources.",
  "comments": [
    {
      "file": "rules/go/cobra-not-stdlib-flag.yml",
      "line": 18,
      "severity": "nit",
      "message": "go-quality-assistant: The explicit `flag.Parse()` pattern (line 18) is redundant with `flag.Parse($$$ARGS)` which already matches zero-argument calls. Harmless (ast-grep deduplicates `any` matches), but unnecessary."
    },
    {
      "file": "rules/go/no-raw-go-func.yml",
      "line": 28,
      "severity": "nit",
      "message": "go-architecture-assistant: The ignores contain both `pkg/cli/**` and `**/pkg/cli/**`. The latter is a strict superset of the former, making `pkg/cli/**` redundant. If only nested matching is needed, a single `**/pkg/cli/**` would suffice. Harmless but inconsistent with how other ignores are structured."
    }
  ],
  "concerns_addressed": [
    "correctness: import_spec regex confirmed to match both single-line `import \"flag\"` and grouped `import (\"flag\")` forms per ast-grep 0.43.0 tree-sitter grammar — addressed by go-quality-assistant adjudication",
    "correctness: flag.* call patterns (now 20 total) comprehensively cover the stdlib flag API surface — addressed by go-quality-assistant adjudication",
    "correctness: kind: go_statement confirmed to match all go expr() forms — addressed by go-architecture-assistant adjudication",
    "correctness: pkg/cli/** exemption confirmed to match the documented signal-listener pattern — addressed by go-architecture-assistant adjudication",
    "correctness: scope broadening to 'any non-test Go file' is correct and justified by transitive flag.init() pollution prevention — addressed by go-quality-assistant adjudication",
    "tests: no test fixtures for new rules — not applicable (plugin repo has no Go test suite; rules are verified mechanically via ast-grep when installed)"
  ]
}