feat(scenarios): add 004-findings-exist-path against maintainer#2; promote to active

[bborbe]

Summary

Closes the gap that scenarios 001-003 leave open: none of them walks the load-bearing path the bot executes on every real Go PR with violations (Step 4a funnel surfaces findings → Step 4b dispatches per-Owner agents → Step 5 reports the violations under Must Fix).

Stable test target: bborbe/maintainer#2 (branch `delete-this-pr-never`, title `test: delete-this-pr-never`). The companion fixture `pkg/scenarios-test-fixture/violations.go` was committed to that branch in maintainer commit `536a6e7`; the PR stays open in perpetuity so the scenario can re-walk against the same SHA on every dispatcher change.

Fixture violations (5)

Rule	Trigger in fixture
`go-architecture/constructor-returns-interface`	`NewService1` returns `*Service1`
`go-architecture/no-globals-or-singletons`	`sharedService1` at package scope
`go-time/no-time-now-direct`	bare `time.Now()`
`go-concurrency/no-raw-go-func`	bare `go func(){...}()`
`go-errors/no-fmt-errorf`	`fmt.Errorf(...)` — does NOT currently fire (separate YAML parse-ambiguity bug)

Walk result — 6/6 PASS

• findings_count: 4 (target ≥ 4) — 4 of 5 violations surfaced
• distinct_rule_ids: 4 (target ≥ 3)
• distinct_owners: 2 — go-architecture-assistant + go-time-assistant (target ≥ 1)
• agent_presence: 0 missing (target 0) — Step 4b can resolve every Owner's agent file
• hallucinated_rules: 0 (target 0) — citation discipline holds
• funnel_wall_ms: 20 (target ≤ 5000)

On the missing 5th violation

The fixture's `fmt.Errorf` doesn't fire because `rules/go/no-fmt-errorf.yml` uses pattern `fmt.Errorf($$$ARGS)` which tree-sitter Go grammar parses as `type_conversion_expression` rather than `call_expression` — so the pattern matches no real call sites. The fix is to rewrite the pattern as `kind: call_expression` + selector regex, similar to the glog/cobra YAMLs.

Tracked as a separate rule bug; the scenario floor is set to ≥ 4 so this PR can promote 004 to `active` independent of that fix. Lift the floor to ≥ 5 when the YAML is corrected.

Aftermath

All 4 scenarios on master will be `active`. Coverage layout:

Scenario	Path
001	toolchain-absent failure
002	zero-findings happy path (`/coding:code-review master`)
003	mechanical funnel scaling
004	findings-exist path (`/coding:pr-review` against maintainer#2)

Test plan

• `make precommit` clean (131 rules, 28 mechanical YAMLs, no drift)
• First successful walk against maintainer#2 sha 536a6e7, evidence in commit message
• Bot review

[ben-s-pull-request-reviewer]

{
  "verdict": "request-changes",
  "summary": "The scenario content is well-structured and accurately documents the 4-of-5 baseline limitation for the go-errors/no-fmt-errorf rule. However, the PR introduces new content (scenario 004) without updating README.md or llms.txt, violating the plugin's own documentation update requirements.",
  "comments": [
    {
      "file": "README.md",
      "line": 1,
      "severity": "major",
      "message": "scenarios/004-findings-exist-path.md is not referenced in README.md. Per CLAUDE.md 'Adding a new guide: Add to README.md in appropriate table' — new scenarios should be documented in README for discoverability."
    },
    {
      "file": "llms.txt",
      "line": 1,
      "severity": "major",
      "message": "scenarios/004-findings-exist-path.md is not referenced in llms.txt. Per CLAUDE.md 'Update llms.txt' when adding new content — AI agents relying on llms.txt will not discover this scenario."
    }
  ],
  "concerns_addressed": [
    "correctness: The go-errors/no-fmt-errorf limitation IS clearly documented in the scenario's Expected section ('at least 4 of the 5 deliberate violations surface... The 5th violation... does not currently fire because the YAML pattern is parsed as type_conversion_expression') — concern adequately addressed",
    "tests: The threshold values (findings_count≥4, distinct_rule_ids≥3, distinct_owners≥1) are explicitly stated in the Expected section and appear appropriately calibrated for the described fixture"
  ]
}

superseded by new automated review

[ben-s-pull-request-reviewer]

Based on my comprehensive review of the PR changes, here is my assessment:

Review Summary

Files Changed: README.md (+11), llms.txt (+7), scenarios/004-findings-exist-path.md (new +53)

Nature of Changes: Pure documentation — addition of acceptance scenario 004 and table entries in README/llms.txt.

Automated Checks

• ✅ ast-grep available at /usr/bin/ast-grep
• ✅ plugin.json valid JSON (version 0.14.0)
• ✅ marketplace.json valid JSON (metadata.version: 0.14.0, plugins[0].version: 0.14.0)
• ✅ All 5 rule_ids referenced in scenario fixture exist in rules/index.json:
  • go-architecture/constructor-returns-interface ✓
  • go-architecture/no-globals-or-singletons ✓
  • go-time/no-time-now-direct ✓
  • go-concurrency/no-raw-go-func ✓
  • go-errors/no-fmt-errorf ✓

Scenario 004 Quality Assessment

• Purpose: Validates /coding:pr-review end-to-end against a real external PR (bborbe/maintainer#2)
• Fixture: 5 deliberate violations, correctly documents the known 5th violation limitation
• Expected floor: ≥4 findings (documented rationale for accepting 4-of-5 baseline)
• Citations: All rule_ids verified against rules/index.json
• Setup/Action/Expected/Cleanup: Complete checklist structure

Minor Observation (Not a Blocker)

The plan noted the fixture relies on SHA 536a6e7 of bborbe/maintainer#2 — I cannot verify from this review whether that external SHA is still intact, but that's expected for a review of this repo's changes.

---

{
  "verdict": "approve",
  "summary": "PR adds a well-structured acceptance scenario (004) validating the pr-review pipeline against a real external PR fixture, plus corresponding README/llms.txt entries. All referenced rule_ids exist in rules/index.json. Changes are documentation-only with no code impact.",
  "comments": [],
  "concerns_addressed": [
    "correctness: scenario correctly references all 5 rule_ids against rules/index.json — no hallucinated rule citations",
    "tests: scenario documents the known 5th violation limitation (fmt.Errorf YAML pattern bug) and sets floor at ≥4 findings accordingly"
  ]
}