fix(grafana): drop shadowing _strip_promql_comments + correct stale #440 assertion

[stefans-elastic]

Closes #455.

Two independent problems, one PR

The issue correctly identifies two separate failures on main since #444. Both are fixed here; they are in different commits so the independence is verifiable.

---

Problem 1 — ruff F811 is an active correctness bug

panels.py:94 imported _strip_promql_comments from .promql, then panels.py:1556 redefined it at module level, shadowing the import across all five in-module call sites.

The issue's fuzzing conclusion ("behaviourally identical, cosmetic") is wrong. The two bodies diverge on backslash-inside-backquoted-string: the local copy treated backticks as raw (no escapes), so it left string state at an escaped backtick and ate the rest of the line — including real expression text — as a comment.

Concrete harm, verified on the parent of commit 1:

expr = ('sum by (device) (metric{path=`a\`b#c`})'
        ' / on(device) sum by (device) (other)')

panels._sanitize_promql_structure(expr)
# -> 'sum by (device) (metric{path=`a\`b'   # matcher GONE
panels.can_use_native_promql(expr, runtime_features={})   # -> True  <-- BUG

The local scanner ate #c`} / on(device) … as a comment. The gate then saw a plain aggregation and offered native PROMQL to an incapable target — exactly the hiding #440 exists to prevent.

Why the canonical copy: promql_parser.parse('foo{path=/a`}') raises *unterminated quoted string*, so the parser every downstream gate uses treats `` \ `` as an escape — the canonical copy matches it, the local one did not. The canonical copy is also the only one under test (the #443 suite imports from `promql.py`).

Honest trade-off noted in the promql.py docstring: following promql-parser's escape semantics rather than Prometheus's raw-string rule means a real # comment after an escaped-backtick value can survive into the cleaned text on that class of input. That is the lesser error — deleting the string removes real query structure — and only arises on inputs the parser rejects as unterminated. A follow-up issue will cover this and the missing parse gate in can_use_native_promql.

Commit 1 removes the local definition and folds its #440 rationale into the promql.py docstring. It also adds two regression tests:

• tests/test_grafana_issue_440_vector_matching.py: pins that the matcher survives sanitization (and both targets correctly decline) for the escaped-backtick case
• tests/test_grafana_issue_443_promql_comments.py: asserts panels._strip_promql_comments is promql._strip_promql_comments — catches the re-duplication shape ruff cannot see

---

Problem 2 — the #440 test assertion is stale, not ambiguous

test_grafana_issue_440_vector_matching.py:391 asserted False for a capable target on an expression with a comment before the matcher parenthesis. The inline rationale: "_clean_promql_for_native flattens the expression … would fold the operand after the comment into it."

That ordering predates #444. #444 strips comments before the flatten, so _clean_promql_for_native now produces:

sum by (device) (rate(…)) / on (device) sum by (device) (rate(…))

— valid PromQL with the matcher intact and both sides sum by (device), the documented native-eligible shape. The comment-free equivalent is already True under CAPABLE (line 306 of the same file). Keeping line 391 would make a comment alone flip the routing decision, contradicting:

• docs/sources/grafana.md:995 (written by fix: strip PromQL comments before they are read as query structure #444): "Comment text never decides routing either"
• tests/test_grafana_issue_443_promql_comments.py:317-321: test_comment_does_not_change_the_routing_decision

#440's actual bug — the matcher must not be hidden so an incapable target declines — is preserved. Only the belt-and-braces capable-target line is wrong.

Commit 2 flips the assertion to assertTrue, rewrites the stale rationale, adds an equivalence assertion (both feature states; guards both directions), and adds a sentence to docs/sources/grafana.md making the 'comment inside an otherwise native-eligible expression' case explicit.

---

Why four PRs landed on red main

The main push runs for #442–#450 were cancelled, not failed. tests.yml sets concurrency: cancel-in-progress: true keyed on workflow+ref, and those merges landed within ~3 minutes, cancelling each preceding run. Only #451's run completed. There is also no required-status-check ruleset on main (the org rulesets cover force-push, require-PR, and restrict-deletions). Both need repo/org admin rights. The most pragmatic code change — excluding main push runs from the concurrency group in .github/workflows/tests.yml — would let each merge produce a completed signal rather than a cancelled one, but it does not replace required status checks and is left for the maintainer to decide.

---

Verification

make lint && make typecheck && make test
# and for a full non-stopping count:
.venv/bin/python -m pytest tests -o addopts="" -q --maxfail=500
# Result: 6660 passed, 61 skipped, 0 failures (baseline: 6658 passed, 2 failed)

🤖 Generated with Claude Code