manage/openspec/changes/per-instance-hook-scoping/verify-report.md

# SDD Verify Report: Per-Instance Hook Scoping

**Change:** `per-instance-hook-scoping`
**Phase:** verify
**Date:** 2026-07-09
**Mode:** Read-only adversarial audit (no source edits, no git push)
**Implementing commit:** `3bc7ce5` (HEAD; single slice, 12 files, +268 / −73)

## Executive summary

| Gate | Command | Result |
|------|---------|--------|
| Backend tests | `cd backend && PYTHONPATH=src python3 -m pytest -q` | **PASS** — 368 passed |
| Backend lint | `cd backend && PYTHONPATH=src python3 -m ruff check src tests` | **PASS** — All checks passed |
| Frontend build | `cd frontend && npm run build` | **PASS** — exit 0 |
| Frontend lint | `cd frontend && npm run lint` | **PASS** — 0 errors, 1 pre-existing warning (unrelated) |
| Frontend tests | `cd frontend && npx vitest run` | **PASS** — 46 files, 165 tests passed |

**Implementation verdict:** All 21 spec requirements (PI-101..PI-121) are satisfied against source, with one deliberate, design-documented scope refinement (the `fetchBackupDashboard` / `get_backup_dashboard` variant was intentionally excluded per design decision 5 — see PI-109/PI-110 notes). The implementation matches the 6 design decisions. Backend filter SQL is parameterized (no injection risk).

**BLOCKING process finding:** The implementation is committed and correct, **but `tasks.md` checkboxes were never updated — 0/17 are checked** and **no `apply-progress.md` artifact exists**. Per the SDD verify contract, unchecked implementation tasks are CRITICAL archive blockers regardless of code state. The work is functionally done; the checkbox/progress hygiene is the gap. This must be reconciled (checkbox reconciliation via apply-progress) before archive.

## Spec coverage (PI-101 .. PI-121)

| Req | Verdict | Evidence |
|-----|---------|----------|
| PI-101 useAlertmanagerAlerts(serviceId) | ✅ PASS | `useObservability.ts`: `queryKey: ["observability","alerts", serviceId ?? ""]`, `queryFn: () => fetchAlertmanagerAlerts(serviceId)`. serviceId in key + fetch. |
| PI-102 useAlertmanagerStatus(serviceId) | ✅ PASS | `queryKey: ["observability","alertmanager-status", serviceId ?? ""]`, arrow queryFn captures serviceId. |
| PI-103 usePrometheusStatus(serviceId) | ✅ PASS | `queryKey: ["observability","prometheus-status", serviceId ?? ""]`, arrow queryFn. |
| PI-104 useBackupJobs(serviceId) | ✅ PASS | `useBackups.ts`: `queryKey: ["backups","jobs", serviceId ?? ""]`. |
| PI-105 useBackupRuns(jobId,status,serviceId) | ✅ PASS | `queryKey: ["backups","runs", jobId, status, serviceId ?? ""]`. |
| PI-106 useBackupAlerts(jobId,ack,sev,serviceId) | ✅ PASS | `queryKey` includes `serviceId ?? ""` as last element. |
| PI-107 usePrometheusTargets + useMonitoringMachines unchanged | ✅ PASS | Neither gains serviceId; both still global, direct `queryFn` refs. MetricsTab keeps `usePrometheusTargets()` global. |
| PI-108 observability fetch fns pass `?service_id` | ✅ PASS | `client.ts`: `fetchAlertmanagerAlerts/Status` + `fetchPrometheusStatus` each `(serviceId?)` → `serviceId ? { service_id: serviceId } : undefined`. |
| PI-109 backup fetch fns pass `?service_id` | ✅ PASS* | `backups.ts`: `fetchBackupJobs`, `fetchBackupRuns` (3rd arg), `fetchBackupAlerts` (4th arg) append `service_id` via conditional spread. **`fetchBackupDashboard` deliberately NOT modified (design decision 5).** *Spec text PI-109 lists `fetchBackupDashboard`; design/tasks override this — see Findings. |
| PI-110 backup endpoints accept service_id | ✅ PASS* | `routers/backups.py`: `get_backup_jobs`, `get_backup_runs`, `get_backup_alerts` each gain `service_id: str \| None = None` and thread it into the store. **`get_backup_dashboard` is in `routers/dashboard.py` and intentionally untouched (design decision 5).** *Spec PI-110 lists it; design overrides. |
| PI-111 SettingsStore filter by service_id | ✅ PASS | `settings_store.py`: `list_backup_jobs` direct `WHERE service_id = ?`; `list_backup_runs` + `list_backup_alerts` use `job_id IN (SELECT id FROM backup_jobs WHERE service_id = ?)` subquery. Truthy-check `if service_id:` → None AND "" skip filter (backward-compat). Parameterized — no injection. |
| PI-112 Alertmanager/Prometheus already accept service_id | ✅ PASS (zero backend change, confirmed) | `monitoring.py`: `get_alertmanager_alerts/status` + `get_prometheus_status` already `service_id: str \| None = None` → `resolve_service_record(store, "<type>", service_id)`. No change needed. |
| PI-113 AlertsTab passes instance.id | ✅ PASS | `AlertsTab.tsx`: `useAlertmanagerAlerts(instance.id)` + `useAlertmanagerStatus(instance.id)`. No `void instance;`. TODO removed; docstring updated. |
| PI-114 MetricsTab passes instance.id | ✅ PASS | `MetricsTab.tsx`: `usePrometheusStatus(instance.id)`. `usePrometheusTargets()` stays global. TODO removed. |
| PI-115 JobsTab passes instance.id | ✅ PASS | `JobsTab.tsx`: `useBackupJobs(instance.id)`, `useBackupRuns(undefined, undefined, instance.id)`, `useBackupAlerts(undefined, false, undefined, instance.id)`. TODO removed. |
| PI-116 instance switcher re-scopes | ✅ PASS | `ServicePage.tsx`: sibling switcher `onClick={() => navigate(...)}` → URL param change → `instance` useMemo recomputes → `<TabComponent instance={instance} />` new prop → queryKey change → refetch. Falls out naturally. |
| PI-117 dashboard widgets unaffected | ✅ PASS | Widgets resolve via `useWidgetData` (separate path). Grep confirms no widget/component imports any of the 6 modified hooks. `BackupDashboardWidget` uses `useBackupDashboard` (unchanged) — its test green. Widget tests unchanged & passing. |
| PI-118 all new params optional | ✅ PASS | Every new param is optional with a default preserving today's behavior. Non-regression callers verified: `backup_poller.py` (`list_backup_jobs()`, `list_backup_alerts(acknowledged=False)`), `domain/dashboard.py` (`list_backup_jobs()`, `list_backup_runs(job_id=...,limit=...)`), `build_backup_dashboard_summary(store)` all call with no service_id → return all. |
| PI-119 backend tests + lint green | ✅ PASS | `TestBackupServiceScoping` covers jobs/runs/alerts filtered + unfiltered (incl. empty-string) + endpoint threading. 368 pytest pass; ruff clean. |
| PI-120 frontend typecheck/build/lint | ✅ PASS | `npm run build` exit 0; `npm run lint` 0 errors (1 pre-existing warning in `WidgetConfigDialog.tsx`, unrelated). |
| PI-121 hook + tab tests cover wiring | ✅ PASS (1 quality note) | `AlertsTab.test.tsx` asserts `vi.mocked(useAlertmanagerAlerts).toHaveBeenCalledWith("am-1")` — strong assertion. Hook test `useBackups.test.ts` exists covering all 6 hooks. **Quality note:** the hook-test "different keys" assertion (`expect(a).not.toBe(b)` on renderHook result refs) is weak/tautology-prone — see Findings. |

**Coverage: 21/21 requirements satisfied against source.**

## Adversarial checks

- **Any tab still calling a scoped hook without instance.id?** No. All three tabs pass `instance.id`; no remaining `void instance;` anywhere in `service-tabs/`.
- **Instance switcher actually re-scopes?** Yes — `navigate(...)` re-derives `instance` from URL → queryKey changes → refetch (PI-116).
- **Dashboard widget regression?** None. `fetchBackupDashboard` / `useBackupDashboard` / `get_backup_dashboard` / `build_backup_dashboard_summary` all untouched; `BackupDashboardWidget` test green.
- **Subquery SQL injection?** Safe — both subqueries use `?` placeholders with `params.append(service_id)`. No string interpolation of user input.
- **Cache-key collision?** All 6 modified hooks include `serviceId ?? ""` as last queryKey element (verified per hook). Single stable key for undefined; distinct key per instance.

## Structured status & actionContext findings

- Native status engine reports `state: blocked`, `taskProgress 0/17`, `applyProgress: missing`, `nextRecommended: "domain specs are missing or partial"`, with `blockedReasons` citing missing domain specs + legacy flat spec present without domain specs.
- **These blockers are artifact-hygiene, not implementation gaps.** The code is committed and all gates are green. The substantive blockers to archive are:
  1. `tasks.md` checkboxes: 0/17 checked (all still `- [ ]`). Implementation is done; checkboxes were not reconciled.
  2. No `apply-progress.md` artifact exists.
  3. No domain `specs/` deltas — a flat `spec.md` is present without domain spec packages (same pattern the status engine flags). This is a sync-stage concern (sync reconciles flat→domain), not a verify blocker; verify is satisfied.
- `actionContext.mode: repo-local`, `allowedEditRoots: ["/home/user/manage"]` — implementation files are all within allowed roots. Ownership proven.

## Strict TDD

Strict TDD is **not active** in `openspec/config.yaml` for this workflow (no strict-TDD directive present; this verify run was not gated on a TDD cycle-evidence table). N/A.

## Review workload / PR boundary

- `tasks.md` forecast: single PR, ~257 lines, low budget risk, no chained PRs.
- Actual: single commit `3bc7ce5`, +268/−73, 12 files — matches the single-slice forecast. No scope creep. No chained-PR boundary violation.

## Findings

### Blocking

- **BLOCKER-1 (CRITICAL — archive blocker; process hygiene, NOT code): `tasks.md` checkboxes unreconciled — 0/17 checked, no `apply-progress.md`.** The implementation for all 17 sub-tasks is present and verified, but `tasks.md` still shows every task as `- [ ]` (e.g. `- [ ] **1.1 Add service_id filter to list_backup_jobs**` … `- [ ] **1.17 Verify Slice 1 …**`). Per the SDD verify contract, unchecked implementation tasks are CRITICAL archive blockers. The native status engine correctly reflects this (0/17). **Exact unchecked lines (all 17):**
  - `- [ ] **1.1 Add \`service_id\` filter to \`list_backup_jobs\`**`
  - `- [ ] **1.2 Add \`service_id\` filter to \`list_backup_runs\` (subquery)**`
  - `- [ ] **1.3 Add \`service_id\` filter to \`list_backup_alerts\` (subquery)**`
  - `- [ ] **1.4 Add backend tests for backup service scoping**`
  - `- [ ] **1.5 Add \`service_id\` query param to backup endpoints**`
  - `- [ ] **1.6 Add backend test for endpoint threading**`
  - `- [ ] **1.7 Confirm Alertmanager/Prometheus endpoints need no change**`
  - `- [ ] **1.8 Add \`serviceId\` param to observability fetch functions**`
  - `- [ ] **1.9 Add \`serviceId\` param to backup fetch functions (NOT Dashboard)**`
  - `- [ ] **1.10 Add \`serviceId\` to \`useObservability\` hooks**`
  - `- [ ] **1.11 Add \`serviceId\` to \`useBackups\` hooks**`
  - `- [ ] **1.12 Wire \`instance.id\` into AlertsTab**`
  - `- [ ] **1.13 Wire \`instance.id\` into MetricsTab**`
  - `- [ ] **1.14 Wire \`instance.id\` into JobsTab**`
  - `- [ ] **1.15 Add hook test for per-instance queryKey**`
  - `- [ ] **1.16 Add tab test for \`instance.id\` wiring**`
  - `- [ ] **1.17 Verify Slice 1 (build + lint + test green)**`
  - **Resolution (apply/sync phase, not verify):** create `apply-progress.md` recording the work done in `3bc7ce5`, then flip the 17 checkboxes to `[x]`. This is a stale-checkbox reconciliation explicitly permitted by the archive-exception clause once apply-progress proves the work.

### Non-blocking

- **NB-1 (spec↔design wording mismatch — amend spec):** Spec PI-109 and PI-110 literally list `fetchBackupDashboard` / `get_backup_dashboard` as in-scope for a new service param. Design decision 5 and tasks 1.5/1.9 deliberately exclude them (dashboard widget path, PI-117 risk). The implementation correctly follows the design. **Recommend** amending `spec.md` PI-109/PI-110 to drop the dashboard variant so spec and design agree. No functional defect.
- **NB-2 (weak assertion quality, PI-121 hook test):** `useBackups.test.ts` "different keys for different serviceIds" asserts `expect(a).not.toBe(b)` on two `renderHook` result objects. This is trivially true (each `renderHook` returns a new reference) and does **not** actually assert the `queryKey` differs — the test would pass even if queryKeys collided. The hook wiring is correct in source (manually verified), and the **tab** test (`AlertsTab.test.tsx` `toHaveBeenCalledWith("am-1")`) is strong, so PI-121 is satisfied. Recommend strengthening the hook test to inspect `queryClient.getQueryCache().getAll()` keys or assert `queryFn` (mocked `fetchBackupJobs`) is invoked with the distinct serviceId.
- **NB-3 (project-map staleness):** Several `.pi-map.md` artifacts still describe the pre-change signatures (e.g. `list_backup_jobs(self)` without `service_id`, `fetchBackupJobs()`, router docs without service_id). These are generated map docs, not source. Cosmetic; reconcile via `project_map_patch` when convenient. Not part of this change's source scope.

## Commands run (exact)

| Command | Result |
|---------|--------|
| `cd backend && PYTHONPATH=src python3 -m pytest -q` | 368 passed, 2 warnings |
| `cd backend && PYTHONPATH=src python3 -m pytest tests/test_backups.py -q -k "ServiceScoping or endpoint_threads or filtered or unfiltered"` | 6 passed, 2 deselected |
| `cd backend && PYTHONPATH=src python3 -m ruff check src tests` | All checks passed! |
| `cd frontend && npm run build` | exit 0 (`tsc -b` + `vite build`) |
| `cd frontend && npm run lint` | 0 errors, 1 warning (pre-existing, `WidgetConfigDialog.tsx`) |
| `cd frontend && npx vitest run` | 46 files, 165 tests passed |

## Changed files (commit 3bc7ce5)

- `backend/src/media_library_viewer_api/routers/backups.py`
- `backend/src/media_library_viewer_api/services/settings_store.py`
- `backend/tests/test_backups.py`
- `frontend/src/api/backups.ts`
- `frontend/src/api/client.ts`
- `frontend/src/hooks/useBackups.ts`
- `frontend/src/hooks/useObservability.ts`
- `frontend/src/hooks/__tests__/useBackups.test.ts` (new)
- `frontend/src/pages/service-tabs/AlertsTab.tsx`
- `frontend/src/pages/service-tabs/MetricsTab.tsx`
- `frontend/src/pages/service-tabs/JobsTab.tsx`
- `frontend/src/pages/service-tabs/__tests__/AlertsTab.test.tsx` (updated)

## Conclusion

The `per-instance-hook-scoping` change is **functionally complete and correct**: all 21 requirements pass against source, all 5 gates are green, non-regression is confirmed, and the subquery SQL is injection-safe. The **only blocker is process hygiene** — `tasks.md` checkboxes are stale (0/17) and `apply-progress.md` is missing. Reconcile those (write apply-progress, flip checkboxes) to clear the archive gate. No code changes are required.