# Verify Report — service-credential-tester

> Phase: **verify** · Change: `service-credential-tester` · 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:** `f6c67bd` (`feat(service-credential-tester): slice 2 — Test button + gating (shared ServiceTestPanel)`).

Two implementation slices are committed underneath it:

- `3391fbc` slice 1 — backend test endpoint + per-type routines
- `f6c67bd` slice 2 — Test button + gating (shared `ServiceTestPanel`)  *(the task brief cited
  `9972514` for slice 2; the actual landed commit is `f6c67bd`. Content matches the spec/design;
  informational, not a defect.)*

There is one uncommitted working-tree change: `frontend/src/pages/ServicesPage.tsx` — purely a
cosmetic JSX reflow (indentation/prettier), no functional diff. See finding N-1.

---

## 0. Executive summary / verdict

**VERDICT: PASS (functionally) — every requirement CT-101 … CT-121 is met in source and all
gates are green. ARCHIVE IS BLOCKED on a task-hygiene / missing-`apply-progress` issue
(reconcilable without code changes).**

The credential tester is implemented end-to-end: `POST /api/services/test` validates first
(reusing `_validate_input`), dispatches through the closed `test_callable` registry, returns
`{ok, detail, evidence}`, persists nothing, and logs no secrets. All 7 remote types have a
`test_connection` routine wired into their `DEFINITION`; `backups` correctly has
`test_callable = None`. The shared `ServiceTestPanel` (Test button + result pill + Save-anyway
override) is wired into **both** `CreateServiceDialog` (ServicesPage.tsx) **and**
`ServiceConfigEditor` (Settings.tsx — the correct edit surface per the design source-finding,
**not** the read-only `ServicePage.tsx`). Create/Save are gated on `testPassed`, with a
documented store-previous pattern that clears the result on any connectivity-field edit.

All five gates are green: backend `pytest` (362 passed), `ruff` (clean), frontend `npm run build`,
`npm run lint` (0 errors; 1 pre-existing unrelated warning), and `npx vitest run` (45 files / 158
tests).

**Blocking issues** are purely lifecycle: `apply-progress.md` is absent and **all 29 tasks in
`tasks.md` remain unchecked (`- [ ]`)** — the apply phase never reconciled the checklist. The
implementation is proven complete by source reading + passing gates, so this is stale-checkbox
reconciliation, but the contract requires no unchecked implementation tasks and a present
`apply-progress.md` before archive.

**Per the strict verify contract, this cannot return a clean archive-ready PASS while unchecked
implementation tasks and the missing apply-progress persist.** Functional verification is PASS;
archive readiness is BLOCKED.

---

## 1. Per-requirement verdict table (CT-101 … CT-121)

| Req | Description | Verdict | Evidence |
|-----|-------------|---------|----------|
| CT-101 | `POST /api/services/test` accepts `ServiceInstanceInput`, returns `{ok,detail,evidence}`, auth-gated | ✅ PASS | `routers/services.py:189` `test_instance`; returns 200 dict; auth-gated via app middleware (`path.startswith("/api")`, not in `EXEMPT_PATHS`) |
| CT-102 | Validation before test → 422 on malformed config | ✅ PASS | `_validate_input(body)` called first (`services.py:196`); `test_validation_first_rejects_malformed_config` asserts 422 for schema-less `base_url` |
| CT-103 | Closed per-type dispatch via `test_callable`; `None` → default ok | ✅ PASS | `base.py:125` field `test_callable: TestCallable \| None = None`; `None` branch returns `{ok:true, "No connection test…"}` |
| CT-104 | qBittorrent: login + maindata; `"Fails."` → auth message | ✅ PASS | `qbittorrent.py:test_connection`; `RuntimeError("login failed")` → `"Authentication failed — qBittorrent rejected the credentials."`; `test_login_failed_translates_to_auth_message` confirms |
| CT-105 | Prometheus via Grafana gateway `POST /api/ds/query`, `expr:"up"` | ✅ PASS | `prometheus.py:test_connection` POSTs `{grafana_url}/api/ds/query` with `Bearer` + `queries[0].expr="up"`; **no direct Prom call** |
| CT-106 | Alertmanager probes `/api/v2/alerts` or `/api/v2/status` | ✅ PASS | `alertmanager.py:test_connection` GETs `{base_url}/api/v2/status` (optional bearer), returns `versionInfo.version` |
| CT-107 | Jellyfin calls `.users()` | ✅ PASS | `jellyfin.py:test_connection` → `JellyfinClient(...).users()` → `"<N> users"` |
| CT-108 | Authentik probes directory endpoint | ✅ PASS | `authentik.py:test_connection` → `AuthentikClient(...).users(page=1, page_size=1)` → `"<N> users"` |
| CT-109 | ssh_tasks reuses `build_ssh_client` (no duplicated SSH logic) | ✅ PASS | `ssh_tasks.py:test_connection` imports `build_ssh_client` from `task_runner` + `.connect()`; **does NOT call `test_machine_ssh`** |
| CT-110 | Nextcloud probes `/status.php` | ✅ PASS | `nextcloud.py:test_connection` GETs `{base_url}/status.php`, returns `version` |
| CT-111 | Backups `test_callable = None` | ✅ PASS | `backups.py` `DEFINITION` omits `test_callable` → default `None`; returns no-test response |
| CT-112 | No persistence (no `upsert_service`/`update_setting`) | ✅ PASS | Endpoint has zero persistence calls; `test_no_persistence_after_test` asserts store count unchanged |
| CT-113 | Secrets never logged | ✅ PASS | Only logs `test requested type=%s ok=%s`; no body/config/secrets. ⚠ test is weak (see N-2) |
| CT-114 | API client fn + hook + type | ✅ PASS | `api/services.ts:testServiceInstance`, `hooks/useServices.ts:useTestServiceInstance`, `types/index.ts:ServiceTestResult` |
| CT-115 | Test button in both create + edit dialogs | ✅ PASS | `ServiceTestPanel` in `ServicesPage.tsx::CreateServiceDialog` AND `Settings.tsx::ServiceConfigEditor` |
| CT-116 | Result pill renders both states w/ evidence/detail | ✅ PASS | `ServiceTestPanel` renders `✓ Connected — {evidence}` (default variant) / `✗ {detail}` (destructive) |
| CT-117 | Create/Save gated on `testPassed` + Save-anyway override | ✅ PASS | `confirmDisabled` includes `!testPassed`; `testPassed = (testResult?.ok) \|\| saveAnyway`; override verified |
| CT-118 | Editing a connectivity field clears result | ✅ PASS | store-previous pattern in both parents (ServicesPage on `draft`; Settings on `testInput` via JSON.stringify) |
| CT-119 | Backend tests: routines/dispatch/validation/no-persistence | ✅ PASS | `test_credential_tester.py` (helper + 7 types) + `test_api.py::TestServiceTestEndpoint` (4 tests); 362 passed |
| CT-120 | Frontend tests: button/pill/gating/field-clear | ✅ PASS (w/ gap) | `ServiceTestPanel.test.tsx` (7 cases); page-level gating/field-clear not explicitly tested (see N-5) |
| CT-121 | Build + lint green | ✅ PASS | `npm run build` ✓; `npm run lint` 0 errors (1 pre-existing unrelated warning); `ruff` clean |

---

## 2. Gate outputs (exact)

```
$ cd backend && PYTHONPATH=src python3 -m pytest -q
362 passed, 2 warnings in 40.87s
(warnings: pre-existing StarletteTestClient + pythonjsonlogger deprecation notices — unrelated)

$ cd backend && PYTHONPATH=src python3 -m ruff check src tests
All checks passed!

$ cd frontend && npm run build
vite v8.0.10 building … ✓ 2548 modules transformed.
dist/assets/index-1Qjmq9l.js   1,107.90 kB
✓ built in 990ms
(tsc -b clean; one chunk-size advisory — pre-existing, unrelated)

$ cd frontend && npm run lint
src/components/WidgetConfigDialog.tsx
  370:8  warning  react-hooks/exhaustive-deps  (pre-existing, unrelated to this change)
✖ 1 problem (0 errors, 1 warning)

$ cd frontend && npx vitest run
Test Files  45 passed (45)
     Tests  158 passed (158)
```

---

## 3. Adversarial / special-attention checks

**CT-103 no-persistence** — confirmed: `routers/services.py::test_instance` contains no
`store.upsert_service`, `store.update_setting`, or any write method. The only `store` access is
the injected object passed to `test_callable` (read-only `get_ssh_key` for ssh_tasks). The test
`test_no_persistence_after_test` asserts `before == after` count. ✅

**CT-104 qBit "Fails."** — confirmed: `qbittorrent.py` catches `RuntimeError` and, when
`"login failed"` is in the message, returns `"Authentication failed — qBittorrent rejected the
credentials."`. `QbittorrentClient._login` raises `RuntimeError("qBittorrent login failed: Fails.")`
on the literal `"Fails."` response. Test `test_login_failed_translates_to_auth_message` covers it. ✅

**CT-105 prometheus uses Grafana gateway** — confirmed: the routine POSTs to
`{grafana_url}/api/ds/query` (NOT `/api/v1/query` to a direct Prom). The body mirrors the gateway
query shape (`queries[0]` keyed by `datasource_uid`, `expr:"up"`). ✅

**CT-109 ssh_tasks reuses `build_ssh_client`** — confirmed: `ssh_tasks.py` does
`from media_library_viewer_api.services.task_runner import build_ssh_client` and calls
`build_ssh_client(store, service).connect()`. No `test_machine_ssh` import; no duplicated SSH
connection logic. ✅

**CT-117 store-previous / field-edit-clears-result** — confirmed in both surfaces:

- `ServicesPage.tsx`: `prevDraft` ref comparison clears `testResult`+`saveAnyway` on any draft change.
- `Settings.tsx`: `prevTestInput` with `JSON.stringify` deep-compare clears on content change.
Both are the React-recommended "store previous prop during render" pattern (no `useEffect` sync
loops). ✅

**CT-118 both surfaces wired** — confirmed: `ServiceTestPanel` rendered in `CreateServiceDialog`
(ServicesPage.tsx:310) AND `ServiceConfigEditor` (Settings.tsx:1610). The spec's literal
"ServicePage.tsx" is correctly treated as stale per the design §0 source-finding; `ServicePage.tsx`
is read-only and correctly left untouched. ✅

**Secrets never logged** — confirmed: the only INFO logs are
`test requested type=%s ok=true (no test_callable)` / `test requested type=%s ok=%s`. No request
body, config, or secrets are interpolated. ✅ (see N-2 for the weak test)

**Error-translation coverage** — the shared `translate_connection_error` handles:
HTTP 401/403 (auth), `requests.ConnectionError`/`OSError` (refused), DNS keywords (host not found),
SSL/certificate strings, `requests.Timeout`/`TimeoutError`/`asyncio.TimeoutError` (timed out), and
SSH "protocol banner". The `Timeout` check is correctly ordered before the `OSError` check (since
`requests.Timeout` subclasses `OSError`). ✅

**test_callable presence** — all 7 remote types wired (`qbittorrent`, `prometheus`,
`alertmanager`, `jellyfin`, `authentik`, `ssh_tasks`, `nextcloud`); `backups` is the only `None`.
✅

**Save-anyway checkbox** — it appears **always** (not only on failure). The spec CT-117 requires it
to "appear" and override the gate; it does not mandate "only on failure." The implementation's
always-visible choice is acceptable UX and the override works (`testPassed = ok || saveAnyway`).
Informational, not a defect.

---

## 4. Structured status & actionContext findings

The native SDD status engine reported change selection as **ambiguous**
(`per-instance-hook-scoping, service-credential-tester`) and all dependencies as `blocked` —
because `changeName` was `null` at orchestrator resolution time. This verify phase was scoped by
the parent prompt to `service-credential-tester` explicitly, so the ambiguity does not block this
read-only verification. `actionContext.mode` is `repo-local` with `allowedEditRoots` covering
`/home/user/manage`; all edited files are inside the workspace. No workspace/boundary violation.

---

## 5. Review workload / PR-boundary findings

The `tasks.md` Review Workload Forecast recommended chained PRs (stacked-to-main), ~550–700 total
lines across two ≤400-line slices. The implementation delivered exactly two slices (`3391fbc` S1
backend, `f6c67bd` S2 frontend) matching the forecast boundary. No scope creep beyond the assigned
task list. No `size:exception` was recorded or needed. ✅

---

## 6. Findings

### Blocking (archive blockers)

**B-1: `apply-progress.md` is MISSING.** The required `apply-progress` input artifact does not exist
at `openspec/changes/service-credential-tester/apply-progress.md`. The verify contract requires it
as an input. (No TDD cycle is active for this change, so strict-TDD evidence is not applicable.)

**B-2: All 29 tasks in `tasks.md` are unchecked.** Every implementation task (1.1–1.15, 2.1–2.9,
3.1–3.5) remains `- [ ]`; 0 are `- [x]`. Per the verify contract, unchecked implementation tasks are
a CRITICAL completeness issue and an archive blocker. The implementation is **functionally
complete** (proven by source reading + 362 backend / 158 frontend passing tests), so this is
stale-checkbox reconciliation rather than missing work — but the contract does not permit a clean
archive-ready PASS while unchecked implementation tasks remain. The unchecked lines are all
task-list entries (e.g. `- [ ] **1.1 Add \`TestResult\` dataclass …**`); reconciliation = mark
completed tasks`[x]` + add `apply-progress.md`.

### Non-blocking

**N-1: Uncommitted cosmetic diff in working tree.** `frontend/src/pages/ServicesPage.tsx` has an
uncommitted change that is purely JSX indentation/prettier reflow (no functional diff). Should be
committed or discarded; does not affect verification.

**N-2: `test_secrets_not_logged` (CT-113) is weak/misleading.** The test sends `secrets: {}`
(empty) then asserts an arbitrary string (`"super-secret-hunter2"`) is absent from `caplog.text`.
Because the secret was never sent, the assertion is trivially satisfied and does **not** actually
exercise the property that secrets present in the request body are not logged. The
**implementation is correct** (the endpoint logs only `type`/`ok`), but the test should send a
real secret in the body (e.g. `secrets: {"password": "hunter2"}`) to genuinely verify CT-113.

**N-3: Defensive `logger.exception` could log a traceback.** `services.py:205`
`logger.exception("test_callable raised for type=%s", ...)` emits a full traceback if a
`test_callable` ever raises. All routines catch internally, so this is a rare defensive path, but a
traceback could theoretically surface sensitive data embedded in an exception message. Low risk;
consider `logger.warning` with a truncated message instead.

**N-4: Edit-flow test needs re-typed secrets.** In `ServiceConfigEditor`, `testInput.secrets`
comes from `draftSecrets` which starts empty for existing services. Testing an existing authed
service (e.g. Prometheus) without re-entering its API key fails the test
("Grafana API key is required") because the stored encrypted secret is not sent. This is an
inherent consequence of the no-persistence design (test operates on request-body plaintext;
secrets are encrypted at rest). The "Leave blank to keep the current value" hint implies retyping
is expected. Non-blocking UX note.

**N-5: CT-120 page-level gating/field-clear not explicitly tested.** `ServiceTestPanel.test.tsx`
covers the presentational panel (button, pill states, callbacks). Because the panel is stateless
(the parent owns `testResult`/`saveAnyway` and the gating/field-clear logic), there is no explicit
test that (a) the Create/Save button is disabled until the test passes, or (b) editing a field
clears `testPassed` at the integration level. The logic is correct by source inspection, but the
test coverage is panel-only. Recommend a page-level test asserting the confirm button enables only
after a passing test.

**N-6: Documented design deviation (improvement).** `ServiceTestPanel` is **presentational**
(parent owns state) rather than the design's stateful component (which used `useEffect([input])`).
This is a deliberate, docstring-documented change to avoid the React `setState`-in-effect footgun.
All of CT-114..CT-118 are still satisfied via parent wiring. Informational only.

---

## 7. Residual risks

- The edit-flow "must retype secrets to test" behavior (N-4) may surprise operators; document it
  in the UI or user docs.
- The weak no-secret-logs test (N-2) gives false confidence in CT-113 coverage.
- Frontend integration-level gating is proven by source, not by an automated test (N-5).

---

## 8. Conclusion

The `service-credential-tester` change is **functionally complete and correct**: all 21
requirements pass against source, and all five gates are green. The only blockers to archive are
lifecycle hygiene — the missing `apply-progress.md` and the 29 unchecked tasks in `tasks.md`. Once
the task checklist is reconciled to reflect the (verified-done) work and `apply-progress.md` is
added, the change is archive-ready. No code fixes are required; the non-blocking findings are
test-strength and UX-doc improvements.
