spec(grafana-metric-gateway): verify + close GM-115 + reconcile tracking

Add 3 test cases (startup old-config validation warning; status auth_failed for 401/403) closing the GM-115 PARTIAL. Write apply-progress.md, tick all 33 tasks, add verify-report.md (15/16 PASS, 1 PARTIAL->PASS). Gates green: 331+ pytest, ruff clean, npm build+lint 0 errors, 151 vitest.
2026-07-09 21:53:18 +00:00
parent 7e91e7f931
commit c886fcdf09
5 changed files with 408 additions and 34 deletions
@@ -0,0 +1,271 @@
+# Verify Report — grafana-metric-gateway
+
+> Phase: **verify** · Change: `grafana-metric-gateway` · Repo: `/home/user/manage`
+> FRESH-CONTEXT adversarial read-only verification of the change against
+> `proposal.md`, `spec.md`, `design.md`, and `tasks.md`. **No source edits.**
+> This verify report is the only file written.
+
+**Head commit verified:** `7e91e7f` (`feat(grafana-metric-gateway): slice 2 — rename widgets to Metric*`).
+
+Two implementation slices are committed:
+
+- `df80c68` slice 1 — backend gateway transport + `normalize_grafana_frames` + `MetricSource` adapter + status + validation + CHANGELOG
+- `7e91e7f` slice 2 — frontend widget renames
+
+> NOTE: the dispatch brief cited slice hashes `f955ff2` / `7e91e7f`. The actual
+> landed slice-1 commit is `df80c68` (not `f955ff2`); slice-2 `7e91e7f` matches.
+> Content of both slices matches the spec/design/tasks; this is informational, not a defect.
+
+---
+
+## 0. Executive summary / verdict
+
+**VERDICT: PASS — implementation complete and green; archive BLOCKED on a
+task-hygiene / missing-`apply-progress` issue (reconcilable without code).**
+
+Every functional requirement **GM-101 … GM-116** was checked against source.
+All metric queries route through Grafana `POST {grafana_url}/api/ds/query` —
+**zero** direct Prometheus `GET /api/v1/query[_range]` calls remain in
+`widgets/sources.py` (confirmed by grep). The config schema migrated
+(`base_url`/`api_key` → `grafana_url`/`datasource_uid`/`grafana_api_key`); the
+adapter (`MetricSource`, service key `"prometheus"` unchanged) handles all four
+kinds (`chart`/`gauge`/`mean`/`metric`) via one `_gateway_query` transport;
+`normalize_grafana_frames` is restored and shares `_dedup_label` with the
+retained `normalize_prometheus_matrix`; `get_prometheus_status` probes the
+gateway with `expr: "up"`; startup logs (does not crash on) old-shape config;
+CHANGELOG carries the BREAKING migration note; the three frontend widgets are
+renamed `Metric*` via `git mv` (history preserved) and bound in the registry.
+qBit widgets are untouched and green. All four gates are green: backend
+`pytest` (**331 passed**), `ruff` (**clean**), frontend `npm run build`
+(**exit 0**), `npm run lint` (**0 errors**), `npx vitest run` (**151 passed / 44 files**).
+
+Findings:
+
+- **[CRITICAL — archive blocker, NOT a code defect]** **33 unchecked task
+  checkboxes** remain in `tasks.md` (all of Slice 1 §1.1–1.16, Slice 2 §2.1–2.11,
+  and Integration §3.1–3.6), and **`apply-progress.md` does not exist** to
+  reconcile them. The underlying work *is* done and verified complete against
+  source; the blocker is that the task tracker was never ticked and no
+  apply-progress artifact was produced. Reconciliation = tick the boxes + write
+  `apply-progress.md` (no code change). See §4.
+- **[WARNING]** **GM-115 is PARTIAL**: GM-115's enumerated coverage list
+  explicitly requires "the startup old-config validation" test, which **does not
+  exist**. The status check also lacks an explicit **auth_failed (401/403)**
+  scenario test (GM-110 lists that scenario). The code paths exist and are
+  correct; the gap is test coverage, not functionality. See §5.
+- **[INFO]** `normalize_prometheus_matrix` is now **dead code** in production
+  (no active caller in `src/` besides its own definition + the `# noqa` import).
+  This is **intentional** (design decision 5 — kept as future-proof + tested);
+  qBit builds its series inline. Not a defect, documented for maintainers.
+- **[INFO]** Stale generated `.pi-map.md`/`.pi-map.index.md` artifacts still
+  reference the old `PrometheusChartWidget`/`Gauge`/`Mean` names; these are not
+  deliverable source (ignored by the build) and should be regenerated
+  (`project_map_patch`/`validate`).
+- **[INFO]** Working tree has 2 uncommitted **whitespace-only** modifications
+  (`PrometheusMetricWidget.tsx`, `ServicesPage.tsx` — one indentation line each,
+  unrelated to the change substance). **No files are staged** (`git diff --cached` empty).
+
+---
+
+## 1. Structured status & actionContext findings
+
+The native `gentle-pi.sdd-status` reports `changeName: null` /
+`blockedReasons: ["Change selection is ambiguous: grafana-metric-gateway,
+per-instance-hook-scoping, service-credential-tester."]` because the engine
+auto-detected three active changes. This verify task was **explicitly assigned**
+`grafana-metric-gateway`, so the ambiguity is a dispatch artifact, not a real
+blocker for this verification.
+
+- `actionContext.mode: repo-local`; `workspaceRoot`/`allowedEditRoots` =
+  `/home/user/manage`. All implementation files live under that root. ✓
+- `artifactStore: openspec`; `isNonAuthoritative: false`. The change dir exists
+  with `proposal.md`, `spec.md`, `design.md`, `tasks.md` (this verify report is
+  the 5th artifact). `apply-progress.md` is **missing** (CRITICAL — §4).
+- `dependencies.verify: blocked` (from the auto-detected ambiguity) is a false
+  positive for this assigned change; verification proceeded.
+
+---
+
+## 2. Per-requirement verdict table (GM-101 … GM-116)
+
+| GM-id | Requirement | Verdict | Evidence |
+|---|---|---|---|
+| **GM-101** | Config points at Grafana gateway | **PASS** | `prometheus.py` `PrometheusConfig`: `grafana_url: ServiceBaseUrl`, `datasource_uid: str = "prometheus"`, `timeout_seconds: int = 10`; no `base_url`. `DEFINITION.secret_fields` = `[SecretField(key="grafana_api_key", required=True)]`, no `api_key`. Tests: `test_services.py:120` (`"grafana_url" in schema`), `:185` (`secret_fields == ["grafana_api_key"]`), `:295` (`model_validate({"grafana_url": bad_url})`). |
+| **GM-102** | All queries via Grafana `/api/ds/query` | **PASS** | `sources.py` `MetricSource._gateway_query` POSTs `{grafana_url}/api/ds/query` for all 4 kinds; body has `queries[0].{datasource:{uid,type:"prometheus"},expr,format:"time_series",intervalMs,maxDataPoints,refId:"A"}` + `from`/`to`; header `Authorization: Bearer {api_key}`. `grep "/api/v1/query" sources.py` → **NONE**. `SERVICE_ADAPTERS["prometheus"]=MetricSource()`. Tests assert `call.args[0].endswith("/api/ds/query")`. |
+| **GM-103** | Gateway errors degrade gracefully | **PASS** | `_gateway_query` catches `asyncio.TimeoutError`→`{"error":"Grafana query timed out"}`, `requests.RequestException`→`{"error":f"Grafana query failed: {exc}"}`; outer `fetch` catches everything→`{"error":...}`. Never raises. The `{exc}` string carries HTTP status for auth distinction; status path (GM-110) maps `auth_failed` explicitly. |
+| **GM-104** | Frames normalizer restored + shared | **PASS** | `prometheus_range.py` has `normalize_grafana_frames(raw)` parsing `results.<refId>.frames[].{data.values,schema.fields}`; `normalize_prometheus_matrix` retained. Both call the shared private `_dedup_label(label, seen)`. Tests: label fallback chain (displayName→labels→"value"), dedup, NaN, insufficient-values skip. |
+| **GM-105** | Window presets → from/to + intervalMs | **PASS** | `WINDOW_PRESETS`+`step_for_window` reused. `_gateway_query`: `from=f"now-{N}s"`, `to="now"`, `intervalMs=step*1000`, `maxDataPoints=200` (clamped band). Users do not set from/to/step directly. |
+| **GM-106** | Chart multi-series via gateway | **PASS** | `_fetch_chart`→`_gateway_query(window)`→`normalize_grafana_frames`→`{"series":[...]}`. No single-series restriction. `MetricChartWidget` renders `LineSeriesChart`. Test `GM-106` (test_widgets.py:473) asserts 2-series `{series}`. |
+| **GM-107** | Gauge instant scalar via gateway | **PASS** | `_fetch_gauge`→`_gateway_query(window=None)`→normalize; scalar-only (`len!=1`→error, `None` value→error)→`{value,warn_at,crit_at,min,max,unit}`. `MetricGaugeWidget` renders threshold bands. |
+| **GM-108** | Mean over gateway range window | **PASS** | `_fetch_mean`→`_gateway_query(window)`→normalize; scalar-only; averages non-null points→`{value,unit}`; empty→error. |
+| **GM-109** | Metric instant scalar via gateway | **PASS** | `_fetch_metric`→`_gateway_query(window=None)`→`{"result":normalize_grafana_frames(raw)}` (gateway POST, not Prom GET). `PrometheusMetricWidget` adapted to read `data.result` as `[{label,points}]`, rendering last non-null point per series. |
+| **GM-110** | Status validates gateway path | **PASS** | `monitoring.py` `get_prometheus_status` POSTs `/api/ds/query` with `expr:"up"`; 401/403→`auth_failed`, `RequestException`→`prometheus_unreachable`, other non-2xx→`gateway_error`, missing cfg→`gateway_not_configured`, success→`version="ok"`. Shared `_base_url`/`_auth_headers`/`_timeout` untouched (still serve alertmanager). Tests mock `requests.post` (not old `/-/healthy`/`buildinfo`). |
+| **GM-111** | Widget files renamed via git mv | **PASS** | `MetricChartWidget.tsx`/`MetricGaugeWidget.tsx`/`MetricMeanWidget.tsx` (+ tests) exist; `git log --follow` traces history through the rename. Exports renamed; rendering (recharts/LineSeriesChart/gauge bands/formatMean) preserved. `PrometheusMetricWidget` kept (not renamed). `grep PrometheusChart\|Gauge\|Mean Widget frontend/src` → only stale `.pi-map.md` artifacts (non-source). |
+| **GM-112** | Registry binds Metric* components | **PASS** | `registry.ts`: chart→`MetricChartWidget`, gauge→`MetricGaugeWidget`, mean→`MetricMeanWidget`, metric→`PrometheusMetricWidget`. Kind strings still `"chart"/"gauge"/"mean"/"metric"`. |
+| **GM-113** | Startup rejects old config shape | **PASS** | `main.py` `_validate_prometheus_gateway_config()` iterates `store.list_services("prometheus")`, flags `base_url` without `grafana_url`, logs `logger.warning` naming service+id+migration instruction; called from `lifespan` after `ensure_defaults()`; wrapped in try/except — never crashes. |
+| **GM-114** | CHANGELOG migration note | **PASS** | `CHANGELOG.md` `[Unreleased]` has a **BREAKING** entry naming `grafana_url`, `grafana_api_key`, `datasource_uid`, and the `POST /api/ds/query` switch. |
+| **GM-115** | Backend tests + ruff green | **PARTIAL** | Gates green: `pytest` **331 passed**, `ruff` clean. Coverage present: gateway POST assertion, `normalize_grafana_frames` (fallback/dedup/NaN), gauge/mean scalar-only, status check. **MISSING (per GM-115's explicit MUST-cover list):** no "startup old-config validation" test; no status `auth_failed` (401/403) scenario test. Functional code is correct; gap is enumerated coverage. See §5. |
+| **GM-116** | Frontend build + lint + test green | **PASS** | `npm run build` exit 0 (tsc -b + vite); `npm run lint` 0 errors (1 pre-existing unrelated `react-hooks/exhaustive-deps` warning in `WidgetConfigDialog.tsx`); `npx vitest run` **151 passed / 44 files** (renamed `Metric*` tests + qBit + PrometheusMetricWidget all green). |
+
+**Functional roll-up: 15 PASS, 1 PARTIAL (GM-115, test-coverage only).**
+
+---
+
+## 3. Gate output (commands run + results)
+
+| # | Command | Result | Output |
+|---|---|---|---|
+| 1 | `cd backend && PYTHONPATH=src python3 -m pytest -q` | **PASS** | `331 passed, 2 warnings` (39.6s). Warnings are unrelated Starlette/pythonjsonlogger deprecations. |
+| 2 | `cd backend && PYTHONPATH=src python3 -m ruff check src tests` | **PASS** | `All checks passed!` |
+| 3 | `cd frontend && npm run build` | **PASS** | `tsc -b && vite build` → `built in 1.09s` (chunk-size advisory only, not an error). |
+| 4 | `cd frontend && npm run lint` | **PASS** | `0 errors, 1 warning` (pre-existing exhaustive-deps in `WidgetConfigDialog.tsx`, unrelated to this change). |
+| 5 | `cd frontend && npx vitest run` | **PASS** | `Test Files 44 passed (44) · Tests 151 passed (151)`. |
+| 6 | `grep -n "/api/v1/query" …/widgets/sources.py` | **PASS** | NONE (GM-102 "no direct Prom HTTP call"). |
+| 7 | `grep -rn "PrometheusChartWidget\|PrometheusGaugeWidget\|PrometheusMeanWidget" frontend/src` | **PASS** | Only `.pi-map.md`/`.pi-map.index.md` (stale generated docs, non-source). |
+| 8 | `git log --follow …/MetricChartWidget.tsx` | **PASS** | History preserved through `git mv` (traceable to `1fb12b8`, `5dad982`, …). |
+
+### Adversarial checks
+
+- **DRY label-dedup:** `normalize_grafana_frames` and `normalize_prometheus_matrix` BOTH call the shared `_dedup_label(label, seen)` (prometheus_range.py). Not duplicated. ✓
+- **Startup validation not a no-op:** `_validate_prometheus_gateway_config` really iterates services and conditionally warns on the `base_url`-without-`grafana_url` shape; it is not a bare `logger.info`. (But it has no unit test — §5.)
+- **Dead code:** `normalize_prometheus_matrix` has no active production caller (only tests + the documented `# noqa: F401` import). Intentional per design decision 5.
+- **Status path:** `get_prometheus_status` runs `up` via gateway POST (confirmed), not the old direct-Prom `/-/healthy`+`buildinfo` path.
+- **qBit unaffected:** `QbittorrentWidgetSource` builds `{series}` inline (sources.py:437); never touches `MetricSource`/`normalize_*`; `QbittorrentSpeedWidget` still uses `LineSeriesChart` (unchanged). qBit tests green.
+- **CHANGELOG accuracy:** entry names the correct fields (`grafana_url`, `grafana_api_key`, `datasource_uid`) and is marked BREAKING. ✓
+
+---
+
+## 4. Blocking findings
+
+### [CRITICAL — archive blocker, NOT a code defect] 33 unchecked task checkboxes + missing `apply-progress.md`
+
+`grep -c '^\s*- \[ \]' tasks.md` → **33**, covering every implementation task:
+
+```
+- [ ] 1.1 … 1.16 (Slice 1: backend transport, normalizer, adapter, status, validation, CHANGELOG, tests, verify)
+- [ ] 2.1 … 2.11 (Slice 2: git mv renames, registry, barrel, PrometheusMetricWidget adapt, verify)
+- [ ] 3.1 … 3.6 (Integration: full gates + grep-verify + config.yaml + CHANGELOG checks)
+```
+
+`apply-progress.md` does **not exist**. Per the SDD verify contract, unchecked
+implementation tasks with no apply-progress artifact to reconcile them are a
+CRITICAL archive blocker. The work itself is verified **complete** against source
+(§2) and **green** (§3); the blocker is purely task-tracker hygiene. The
+exception clause ("stale-checkbox reconciliation proven by apply-progress/
+verify-report") is partially satisfied by *this* verify report, but the cleaner
+path is to tick the boxes and author `apply-progress.md` before archive.
+
+**Reconciliation (no code change):** tick all 33 boxes; write
+`apply-progress.md` recording the two landed commits (`df80c68`, `7e91e7f`) and
+the §3 gate results.
+
+---
+
+## 5. Non-blocking findings
+
+### [WARNING] GM-115 PARTIAL — two enumerated test categories missing
+
+GM-115 states tests "**MUST cover** … the startup old-config validation".
+**No such test exists** — `grep` for `_validate_prometheus_gateway_config` /
+`base_url` config-shape in `tests/` returns nothing. The startup code (GM-113) is
+correct and non-crashing, but is not exercised by any test.
+
+Additionally, GM-110 lists the **"Auth failure in status"** scenario; the status
+test class (`TestPrometheusStatus`) covers only `no_service_configured`,
+`prometheus_unreachable`, and `returns_ok` — **no 401/403 → `auth_failed` case**.
+The `auth_failed` code branch exists and is reachable but untested.
+
+**Recommendation (post-archive, non-blocking):** add a startup-validation test
+(e.g. seed an old-shape service, assert startup completes + a warning is logged)
+and a status 401→`auth_failed` test. No functional risk; the missing paths are
+straightforward logger/error mappings.
+
+### [INFO] `normalize_prometheus_matrix` is now dead production code
+
+After this change, no production caller invokes `normalize_prometheus_matrix`
+(only `test_prometheus_range.py` does). It is retained **by design** (decision 5:
+future-proof for a possible `direct_url` path) and remains unit-tested, so there
+is no risk. The `# noqa: F401 — kept for future direct_url path` comment on the
+`sources.py` import documents the intent. qBit builds series inline and never
+used it (the spec's assumption #3 that qBit consumed it was corrected in the
+design source-findings).
+
+### [INFO] Stale generated `.pi-map.md` artifacts
+
+`frontend/src/integrations/.pi-map.md`, `frontend/src/widgets/.pi-map.md`,
+`frontend/src/widgets/__tests__/.pi-map.{md,index.md}` still list the old
+`Prometheus*Widget` names. These are generated map docs, not deliverable source
+(ignored by `tsc`/`vite`/`eslint`). Regenerate via `project_map_patch` /
+`project_map_validate` at the orchestrator's convenience.
+
+### [INFO] Working-tree state
+
+- `git diff --cached` → empty (**no staged files**; `no-staged-files` criterion satisfied).
+- Uncommitted: `frontend/src/widgets/PrometheusMetricWidget.tsx` and
+  `frontend/src/pages/ServicesPage.tsx` — both **whitespace-only** (single
+  indentation line each, unrelated to the change substance). The substantive
+  PrometheusMetricWidget adaptation (the `{result:[{label,points}]}` shape) is
+  committed in slice 2.
+- Untracked: `.pi-tmp/*` scratch files (not part of this change).
+
+---
+
+## 6. Task-checkbox scan (contract requirement)
+
+**Unchecked implementation task markers (`^\s*- \[ \]`) in `tasks.md`: 33.**
+
+Because unchecked implementation tasks remain and `apply-progress.md` is absent,
+this verify report **does not** declare the change ready for archive. The exact
+unchecked lines are listed in §4 (tasks 1.1–1.16, 2.1–2.11, 3.1–3.6). All
+underlying work is confirmed complete and green against source (§2, §3); the
+checkboxes are stale and should be ticked + `apply-progress.md` authored before
+archive.
+
+---
+
+## 7. Spec / design coherence
+
+- Spec → design → source agree on: gateway transport shape (`_gateway_query`),
+  instant-query mapping (`from=now-1m,to=now,maxDataPoints=1`, spec assumption
+  #1), shared `_dedup_label` (GM-104 DRY), status `version="ok"` (assumption #2),
+  `normalize_prometheus_matrix` retention (assumption #3 / decision 5), old
+  `api_key` becoming inert (assumption #4).
+- Design §3.4 **Option A** was chosen for `_fetch_metric`: backend returns
+  `{"result": normalize_grafana_frames(raw)}` and `PrometheusMetricWidget` was
+  adapted to the `{label,points}` shape (small change, ~63-line component). No
+  Option B fallback needed.
+- Review-workload forecast (tasks.md): chained PRs recommended, split
+  S1(backend)/S2(frontend), each ≤400 lines, stacked-to-main. Two slices landed
+  exactly on that boundary; no scope creep (no third slice, no unrelated files).
+  `size:exception` not used.
+
+---
+
+## 8. Risks
+
+- **Migration correctness depends on the recovered frames shape.** The
+  `normalize_grafana_frames` mock in tests mirrors the recovered-from-`65bae95`
+  parser; if a real Grafana `/api/ds/query` response varies (e.g. multi-field
+  frames, `data.values` with >2 columns), the `fields[-1]` value-field heuristic
+  may mis-pick. Low likelihood; mitigated by defensive parsing.
+- **Old-shape services degrade silently at runtime.** A pre-change
+  `prometheus` service (with `base_url`) will warn on startup (GM-113) but its
+  widget fetches will return `grafana_url is required` errors until reconfigured.
+  This is the intended degraded state; the CHANGELOG documents the required
+  operator action.
+- **Dead `normalize_prometheus_matrix`** — see §5 (intentional, low risk).
+- **Two uncommitted whitespace-only edits** in the tree — cosmetic, but a clean
+  tree is preferable before archive.
+
+---
+
+## 9. Conclusion
+
+The `grafana-metric-gateway` change is **functionally complete and correct**:
+all 16 requirements are satisfied at the source level (15 PASS, 1 PARTIAL on
+test-coverage only), and all five quality gates are green. Prometheus queries are
+fully gated behind Grafana's `/api/ds/query` with zero direct-Prom HTTP residue.
+Archive is **blocked** solely on task hygiene — 33 unchecked task checkboxes and
+a missing `apply-progress.md` (reconcilable without code) — plus a non-blocking
+recommendation to add two enumerated tests (startup validation, status
+`auth_failed`).