Developer
98bf496a98
Strengthen test_secrets_not_logged (N-2): now sends real-looking secrets through prometheus + qbittorrent test_callables (mocked at network boundary), asserts no fragments leak into caplog, verified non-vacuous. Write apply-progress.md, tick all 29 tasks, add verify-report.md (21/21 PASS). Gates green: 362+ pytest, ruff clean, npm build+lint 0 errors, 158 vitest.
260 lines
16 KiB
Markdown
260 lines
16 KiB
Markdown
# 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.
|