templates: surface real govulncheck errors, swallow only known x/tools generics panic

[bborbe]

Summary

Improve Makefile.service and Makefile.library vulncheck pattern after maintainer-watcher bot review (raised on cron PR #12, backup PR #11) flagged the prior 2>/dev/null pattern as a critical false-negative risk.

Old pattern: silenced ALL stderr → real govulncheck failures (network errors, bad args, permissions) silently produced "No unignored vulnerabilities found" with exit 0.

New pattern:

• Capture stderr to a temp file
• Check govulncheck exit code separately
• Swallow ONLY when stderr matches the known panic signature (ForEachElement called on type containing *types.TypeParam — the golang.org/x/tools v0.46.0 SSA-on-generics bug)
• Any other failure surfaces its stderr + propagates exit code

Test plan

Validated end-to-end on cron (PR #12 commit dd3ee67):

• Normal case (no findings): make vulncheck → "No unignored vulnerabilities found"
• Real error case (bad args injected): pattern surfaces govulncheck stderr + propagates exit 1
• Panic case (simulated stderr with the known signature): pattern swallows + reports no findings

Downstream

Open Makefile-modernization PRs will be re-synced to this pattern:

• cron docs(ast-grep): propagate counter-total-suffix syntax findings #12, backup feat(prometheus): ast-grep YAML for counter-total-suffix #11, ping bootstrap rules for go-error-wrapping-guide.md (5 rules) #2, git-rest bootstrap rules for go-security-linting.md (4 rules) #3, beactive clarify CQRS handler-error semantics in go-cqrs.md #1, ip bootstrap rules for go-security-linting.md (4 rules) #3, mqtt-kafka-connector bootstrap rules for go-security-linting.md (4 rules) #3, task-watcher clarify CQRS handler-error semantics in go-cqrs.md #1, git-sync bootstrap rules for go-security-linting.md (4 rules) #3, teamvault-utils docs: trim go-testing-guide to rules-only (1207 → 428 lines) #7
• go-skeleton (separate PR — also has Makefile.precommit using the old pattern)
• 35+ already-canonical repos will pick up the pattern naturally via future updater runs

🤖 Addresses: bot reviews on cron #12, backup #11

[ben-s-pull-request-reviewer]

Based on my analysis of the changed files, here is my review:

PR Review: templates: surface real govulncheck errors, swallow only known x/tools generics panic

Changes Analyzed

Both templates/Makefile.library and templates/Makefile.service were modified with identical changes to the vulncheck target. The change fixes a bug where govulncheck failures were incorrectly suppressed.

Security Fix (Correctness)

Old logic (broken):

OUT=$(govulncheck ... 2>ERR); RC=$?; \
if [ $$RC -ne 0 ] || ! grep -q "ForEachElement" $$ERR; then

The || meant: if govulncheck failed AND grep didn't match the panic, exit. But if govulncheck succeeded (RC=0), the grep was never evaluated — and worse, if RC was non-zero but grep matched, the code path continued without properly handling the partial JSON case. The empty OUT/partial output case wasn't handled.

New logic (correct):

OUT=$(govulncheck ... 2>ERR); RC=$?; \
if [ $$RC -ne 0 ] && ! grep -q "ForEachElement" $$ERR; then
    echo "govulncheck failed (exit $$RC):" >&2; cat $$ERR >&2; rm -f $$ERR; exit $$RC
fi
rm -f $$ERR
# then process OUT...

With &&, the error path only triggers for REAL failures (network, bad args, permissions). The x/tools generics panic (which emits real JSON to stdout before crashing) is now correctly distinguished from actual tool failures.

Minor Observations

1. Temp file on early exit: The rm -f $$ERR before exit $$RC correctly cleans up the temp file on the error path. However, if the process were killed by a signal before reaching that line, the temp file would leak. This is theoretical — the panic message is reliably written to ERR before process death.

2. grep pattern case-sensitivity: The pattern "ForEachElement called on type containing" is case-sensitive. Given the comment cites this as the exact panic message from golang.org/x/tools v0.46.0, this is correct.

3. Empty OUT handling: The jq pipeline (jq -rs) handles empty input gracefully — an empty string produces [] which yields no remaining vulnerabilities.

Rule Coverage

The changed files are Makefiles (not Go source), so no Go-language rules apply. The change introduces no new patterns that trigger any indexed rules. No security, architecture, or quality violations.

---

{
  "verdict": "approve",
  "summary": "Bugfix correctly distinguishes x/tools generics panic (govulncheck RC=1 with partial JSON output) from real tool failures (network, permissions, bad args). The old `||` logic silently swallowed all non-zero exits, producing false \"No unignored vulnerabilities found\" even when govulncheck itself errored. The new `&&` logic surfaces real failures while correctly treating the known panic as benign.",
  "comments": [],
  "concerns_addressed": [
    "correctness: grep pattern 'ForEachElement called on type containing' is case-sensitive — confirmed intentional, matches actual panic message from golang.org/x/tools v0.46.0",
    "correctness: temp file ERR rm -f $$ERR runs before exit on the error path — correctly handles cleanup for non-panic failures",
    "correctness: jq pipeline handles empty OUT gracefully via jq -rs producing []",
    "security: old pattern produced false negatives (exit 0 with 'No findings') when govulncheck failed due to real errors; new pattern correctly propagates real failures"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Based on my manual review of the diff (shellcheck requires approval in this environment), here is my analysis:

Summary of Changes

Both templates/Makefile.library and templates/Makefile.service modify the vulncheck target to:

1. Replace 2>/dev/null stderr suppression with explicit error capture via mktemp + trap cleanup
2. Add selective stderr filtering: only swallow the known golang.org/x/tools generics panic (ForEachElement called on type containing), surfacing all other govulncheck failures (network, permissions, bad args)

Manual Code Review Findings

Should Fix — Fragile substring match (templates/Makefile.library:71, templates/Makefile.service:78):

! grep -q "ForEachElement called on type containing" "$$ERR"

This greps for a literal error message substring. If golang.org/x/tools ever revises this panic message, the check silently stops working and real failures would be swallowed (regressing to the old 2>/dev/null behavior). A more robust approach would be to grep for a versioned panic prefix or panic type indicator (e.g., the goroutine stack frame function name), rather than free-text message content.

Should Fix — Subshell variable scoping (templates/Makefile.library:67-70, templates/Makefile.service:74-77):

ERR=$$(mktemp); \
trap 'rm -f "$$ERR"' EXIT INT TERM; \
OUT=$$(go run golang.org/x/vuln/cmd/govulncheck@$(GOVULNCHECK_VERSION) -format json $$PKGS 2>$$ERR); \
RC=$$?; \

The ERR temp file path is local to the shell invocation. If the rule is invoked multiple times in parallel (e.g., make -j2 vulncheck), both invocations will share the same temp file path via $$ expansion — though in practice Makefile's serial recipe execution avoids this for single-target runs. For defensive correctness, consider ERR=$$(mktemp -t govulncheck.XXXXXX) with a unique suffix.

Correct — Trap on temp file (templates/Makefile.library:68, templates/Makefile.service:75):

trap 'rm -f "$$ERR"' EXIT INT TERM

Correctly handles the Ctrl-C (INT) and termination (TERM) signals between mktemp and any subsequent rm. The $$ expansion ensures the trap runs in the same shell that created ERR.

Correct — Error surfacing intentional (templates/Makefile.library:71-74, templates/Makefile.service:78-81):
The change from 2>/dev/null to explicit error capture with selective suppression is a net improvement in debuggability. Real govulncheck failures (bad module cache, permissions, malformed args) now surface with exit codes instead of silently failing.

Note: Both templates/Makefile.library and templates/Makefile.service are identical changes. The findings above apply to both.

---

{
  "verdict": "request-changes",
  "summary": "The PR correctly surfaces real govulncheck failures instead of blanket-stderr suppression, but the grep-based panic filter is a fragile substring match that could silently regress if the x/tools error message changes. Two minor shell-pattern concerns noted below.",
  "comments": [
    {
      "file": "templates/Makefile.library",
      "line": 71,
      "severity": "major",
      "message": "Should Fix: grep pattern `'ForEachElement called on type containing'` is a free-text substring match — fragile if golang.org/x/tools revises this panic message. If the message changes, the filter silently stops working and real failures regress to being swallowed (old behavior: `2>/dev/null`). Consider matching on a versioned panic indicator or goroutine symbol name instead of free-text message content."
    },
    {
      "file": "templates/Makefile.library",
      "line": 67,
      "severity": "major",
      "message": "Should Fix: `ERR=$$(mktemp)` uses a non-unique temp file path. If the rule is ever invoked multiple times in parallel (e.g., `make -j2 vulncheck`), both invocations share the same temp file, causing a race condition on the error capture. Use `mktemp -t govulncheck.XXXXXX` or similar with a unique suffix pattern to avoid collisions."
    },
    {
      "file": "templates/Makefile.service",
      "line": 78,
      "severity": "major",
      "message": "Should Fix: grep pattern `'ForEachElement called on type containing'` is a free-text substring match — fragile if golang.org/x/tools revises this panic message. If the message changes, the filter silently stops working and real failures regress to being swallowed (old behavior: `2>/dev/null`). Consider matching on a versioned panic indicator or goroutine symbol name instead of free-text message content."
    },
    {
      "file": "templates/Makefile.service",
      "line": 74,
      "severity": "major",
      "message": "Should Fix: `ERR=$$(mktemp)` uses a non-unique temp file path. If the rule is ever invoked multiple times in parallel (e.g., `make -j2 vulncheck`), both invocations share the same temp file, causing a race condition on the error capture. Use `mktemp -t govulncheck.XXXXXX` or similar with a unique suffix pattern to avoid collisions."
    }
  ],
  "concerns_addressed": [
    "correctness: grep pattern fragility — raised as Should Fix (major)",
    "correctness: mktemp temp file race condition — raised as Should Fix (major)",
    "security: stderr no longer silenced for real govulncheck failures — confirmed correct as designed"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

I've already obtained the diff. The sandbox is blocking access to the plugin rules/scripts. Let me review the diff directly since this is a small 2-file Makefile change.

Diff Review:

templates/Makefile.library & templates/Makefile.service

Security Fix (Must Fix addressed):

• Old: govulncheck ... 2>/dev/null — silences ALL stderr, causing false "No vulnerabilities found" on real failures (network, bad args, permissions)
• New: Captures stderr to temp file, checks exit code, and only swallows the known typesinternal.ForEachElement panic pattern; all other failures surface properly with echo "govulncheck failed (exit $RC):" >&2

Trap Pattern Analysis:

ERR=$(mktemp)
trap 'rm -f "$ERR"' EXIT INT TERM
OUT=$(govulncheck ... 2>$ERR)
RC=$?
if [ $RC -ne 0 ] && ! grep -qE "typesinternal\.ForEachElement|ForEachElement called on type containing" "$ERR"; then
    echo "govulncheck failed (exit $RC):" >&2
    cat "$ERR" >&2
    exit $RC
fi
REMAIN=$(printf '%s' "$OUT" | jq ...)

• ERR is set by mktemp before use — if mktemp fails, shell would exit before trap is set. Correct.
• trap 'rm -f "$ERR"' EXIT INT TERM — cleans temp file on any exit path. Correct.
• grep -qE pattern matches the known x/tools v0.46.0 SSA generics panic. Correct per upstream issue.

Additional Improvements:

• Added GO-2026-5037/5038/5039 (Go 1.26.3 stdlib crypto vulns) to ignore list with explanation comment
• Updated comments to explain the benign failure mode logic
• Both files updated identically — in sync ✓

No issues found. The fix correctly surfaces real govulncheck errors while only swallowing the known x/tools generics panic.

---

{
  "verdict": "approve",
  "summary": "Makefile vulncheck targets correctly fixed: old pattern silently discarded all stderr causing false security results; new pattern surfaces real failures (network, permissions, bad args) while only suppressing the known golang.org/x/tools v0.46.0 generics panic. Trap pattern is correct, files are in sync, and Go 1.26.3 stdlib vulns are properly added to ignore list.",
  "comments": [],
  "concerns_addressed": [
    "security: Old pattern silenced ALL stderr — real govulncheck failures now properly surfaced in templates/Makefile.library and templates/Makefile.service",
    "correctness: trap cleanup verified — ERR set by mktemp before use, trap on EXIT INT TERM, no early-exit path bypass",
    "correctness: grep -qE pattern matches typesinternal.ForEachElement panic from golang.org/x/tools v0.46.0 SSA generics bug",
    "correctness: OUT captured before jq; if govulncheck panics, OUT may be empty but RC is non-zero, and the failure is surfaced via stderr check",
    "security: Both Makefile.library and Makefile.service updated identically — confirmed in sync",
    "correctness: Same trap/ERR pattern applied to both files — verified consistent"
  ]
}