chore: remove tools.go, migrate CLI tools to tools.env pattern

[bborbe]

Resolves the pre-existing CRITICAL flagged on PR #10 (go-tools-versioning/no-tools-go-for-clis).

tools.go pinned 6 CLI tools (counterfeiter, addlicense, ginkgo, golines, goimports, govulncheck) that the Makefile already invokes via go run pkg@$(VERSION) from tools.env — so tools.go only polluted go.mod with tool-only deps. Deleting it + go mod tidy drops 19 requires / 61 go.sum lines.

Also pins the two remaining bare counterfeiter //go:generate directives (metrics.go, pr_commenter.go) to @v6.12.2, matching the other 10, so go generate resolves without the go.mod dependency.

make precommit green — all 9 mocks regenerate via go generate, tests/lint/gosec/vulncheck pass.

[ben-s-pull-request-reviewer]

Now I have all the context I need. Let me compile the consolidated review findings based on the mechanical funnel results (already provided) and the code I've read.

Review Summary:

This PR removes tools.go and migrates CLI tool versions to tools.env, with counterfeiter directives now explicitly pinning v6.12.2. The mechanical funnel found extensive violations.

Key observations:

• tools.env correctly defines COUNTERFEITER_VERSION ?= v6.12.2
• Both metrics.go and pr_commenter.go now use //go:generate go run github.com/maxbrunsfeld/counterfeiter/v6@v6.12.2 -generate - consistent with tools.env
• go generate is invoked via Makefile.precommit with go generate -mod=mod ./...

The mechanical funnel findings (already computed) are extensive across 5 rule owners:

Must Fix (from ast-grep):

• 57 total findings
• Missing //counterfeiter:generate on Metrics and PRCommenter interfaces
• 12 instances of package-level promauto globals (violates no-globals rule)
• 28 instances of package function calls in business logic (errors.Errorf, json.Marshal, http.NewRequestWithContext, etc.)
• 6 untyped string constants (should be typed enum)
• Missing log on c.httpClient.Do(req) HTTP call

Should Fix:

• 5 missing ctx.Done() checks in init() loops

Correctness verification: The counterfeiter version v6.12.2 is consistent between tools.env and the go:generate directives - this concern is addressed.

---

{
  "verdict": "request-changes",
  "summary": "PR correctly removes tools.go and migrates to tools.env pattern with pinned counterfeiter v6.12.2, but the ast-grep mechanical funnel surfaced 57 MUST-tier violations spanning missing counterfeiter directives, package-level global metrics variables, direct package function calls in business logic, untyped string enums, and a missing log on an HTTP boundary call. These must be addressed before merge.",
  "comments": [
    {
      "file": "pkg/metrics/metrics.go",
      "line": 16,
      "severity": "critical",
      "message": "MUST: go-testing/counterfeiter-mocks-required — Metrics interface missing //counterfeiter:generate directive. Add '//counterfeiter:generate -o ../../mocks/metrics.go --fake-name Metrics . Metrics' above the interface declaration so go generate regenerates the fake when the interface drifts."
    },
    {
      "file": "pkg/prcomment/pr_commenter.go",
      "line": 32,
      "severity": "critical",
      "message": "MUST: go-testing/counterfeiter-mocks-required — PRCommenter interface missing //counterfeiter:generate directive. Add '//counterfeiter:generate -o ../../mocks/pr_commenter.go --fake-name PRCommenter . PRCommenter' above the interface declaration."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 75,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var ScanCyclesTotal declared as package-level 'var X = promauto.NewCounterVec(...)'. Inject via constructor instead. Globals break test parallelism and hide the dependency graph."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 84,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var TasksPublishedTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 95,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var PlanningRetryTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 104,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var ResultsWrittenTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 113,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var GitPushTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 122,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var ConflictResolutionsTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 133,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var FrontmatterCommandsTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 142,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var GitRestCallsTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 152,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var KafkaConsumePausedTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 172,
      "severity": "critical",
      "message": "MUST: go-architecture/no-globals-or-singletons — var SkippedFilesTotal is a package-level global. Inject via constructor instead."
    },
    {
      "file": "pkg/prcomment/pr_commenter.go",
      "line": 72,
      "severity": "critical",
      "message": "MUST: go-composition/no-package-function-calls-in-business-logic — errors.Errorf(...) called directly in business logic. Wrap each capability in a small interface, inject via constructor, call through injected field."
    },
    {
      "file": "pkg/prcomment/pr_commenter.go",
      "line": 87,
      "severity": "critical",
      "message": "MUST: go-composition/no-package-function-calls-in-business-logic — json.Marshal(...) called directly. Inject a marshaller interface via constructor."
    },
    {
      "file": "pkg/prcomment/pr_commenter.go",
      "line": 92,
      "severity": "critical",
      "message": "MUST: go-composition/no-package-function-calls-in-business-logic — http.NewRequestWithContext(...) called directly. Inject an HTTP client interface via constructor."
    },
    {
      "file": "pkg/prcomment/pr_commenter.go",
      "line": 106,
      "severity": "critical",
      "message": "MUST: go-composition/no-package-function-calls-in-business-logic — io.Copy(io.Discard, io.LimitReader(...)) called directly. Inject or use a bounded reader interface."
    },
    {
      "file": "pkg/prcomment/pr_commenter.go",
      "line": 101,
      "severity": "critical",
      "message": "MUST: go-logging/external-call-logs-response — c.httpClient.Do(req) HTTP call found without a log statement summarising the call outcome. Boundary calls are the audit trail — log method + path + status + latency; add error message on failure."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 158,
      "severity": "major",
      "message": "MUST: go-enum-type/typed-constants-with-collection — ReasonInvalidFrontmatter is an untyped string constant. Declare 'type SkipReason string' + 'var AvailableSkipReasons' so the closed set is type-checked and self-describing."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 159,
      "severity": "major",
      "message": "MUST: go-enum-type/typed-constants-with-collection — ReasonDuplicateFrontmatterInvalid untyped string constant. Use typed enum pattern."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 160,
      "severity": "major",
      "message": "MUST: go-enum-type/typed-constants-with-collection — ReasonEmptyStatus untyped string constant. Use typed enum pattern."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 161,
      "severity": "major",
      "message": "MUST: go-enum-type/typed-constants-with-collection — ReasonInjectTaskIdentifierFailed untyped string constant. Use typed enum pattern."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 162,
      "severity": "major",
      "message": "MUST: go-enum-type/typed-constants-with-collection — ReasonReadFailed untyped string constant. Use typed enum pattern."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 163,
      "severity": "major",
      "message": "MUST: go-enum-type/typed-constants-with-collection — ReasonAutoInjectDisabled untyped string constant. Use typed enum pattern."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 204,
      "severity": "major",
      "message": "SHOULD: go-context/cancel-check-in-loop — for _, op := range ... loop should check ctx.Done() each iteration. Add non-blocking select { case <-ctx.Done(): ...; default: } at loop top."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 210,
      "severity": "major",
      "message": "SHOULD: go-context/cancel-check-in-loop — for _, status := range ... loop should check ctx.Done() each iteration."
    },
    {
      "file": "pkg/metrics/metrics.go",
      "line": 215,
      "severity": "major",
      "message": "SHOULD: go-context/cancel-check-in-loop — for _, reason := range ... loop should check ctx.Done() each iteration."
    }
  ],
  "concerns_addressed": [
    "correctness: counterfeiter directive version pinning — tools.env defines COUNTERFEITER_VERSION=v6.12.2, and both go:generate directives now use @v6.12.2 — verified consistent",
    "correctness: go generate still produces mocks — the generate target in Makefile.precommit runs 'go generate -mod=mod ./...' which will use the pinned version in go:generate directives",
    "tests: tools.go deletion — tools.go is deleted in this PR (file not found at HEAD), confirming the migration is complete"
  ]
}

[bborbe]

Merging with --admin over the bot's CHANGES_REQUESTED, documented waiver:

The 57 MUST findings are pre-existing — this PR's actual diff to metrics.go and pr_commenter.go is 2 lines total (+@v6.12.2 on the counterfeiter directive). The globals/DI/typed-enum/logging findings are the existing contents of those files, surfaced only because the review funnel scans whole touched files rather than the diff. None are introduced here.

This PR is a clean, minimal fix that removes debt (deletes tools.go, drops 19 go.mod requires / 61 go.sum lines) and resolves the go-tools-versioning/no-tools-go-for-clis CRITICAL flagged on #10. CI green; make precommit clean (9 mocks regenerate).

The pre-existing metrics/pr_commenter DI debt is captured as a separate refactor task and will be addressed on its own. Also noted: the reviewer bot should baseline pre-existing findings (only flag changed lines) to avoid blocking minimal PRs — tracked separately.