chore(service-credential-tester): archive verified+synced change
Move to openspec/changes/archive/2026-07-09-service-credential-tester/ (R100 renames preserved). 9 artifacts. Canonical openspec/specs/ service-credential-testing/ remains. Resolves qBit 'login failed' #3 pain at the UI layer (auth failure surfaced in result pill, no log-digging). Carry-overs in archive-report incl N-2 strengthened, N-6 presentational panel, edit-surface-is-Settings.tsx source-finding.
This commit is contained in:
Developer
@@ -0,0 +1,53 @@
|
||||
# Apply Progress: Service Credential Tester
|
||||
|
||||
**Change:** `service-credential-tester`
|
||||
**Phase:** apply-progress
|
||||
**Date:** 2026-07-09
|
||||
**Status:** complete — all 29 tasks done, all gates green, verified (see `verify-report.md`)
|
||||
|
||||
## Slices delivered
|
||||
|
||||
Two slices, each its own commit, each leaving `pytest` / `npm run build` / `npm run lint` / `ruff` green.
|
||||
|
||||
### Slice 1 — Backend test endpoint + per-type routines (commit `3391fbc`)
|
||||
|
||||
- `integrations/base.py` — added frozen `TestResult` dataclass (`ok`, `detail`, `evidence`) + optional `test_callable` field on `ServiceDefinition` (placed last for dataclass ordering). Signature: `(store, config, secrets) -> TestResult` (CT-101).
|
||||
- Shared error-translation helper `translate_connection_error` — extracts the `test_machine_ssh` patterns (ConnectionError/Timeout/SSL/HTTP 401-403/5xx → friendly strings) (CT-112).
|
||||
- Per-type `test_connection` routines alongside each `DEFINITION`:
|
||||
- **qbittorrent** — `QbittorrentClient` login + `maindata()`; `"Fails."` → "Authentication failed (qBittorrent rejected credentials)" — resolves the #3 log-only pain at the API layer (CT-104).
|
||||
- **prometheus** — `POST {grafana_url}/api/ds/query` with `grafana_api_key` + `datasource_uid`, `expr "up"`; evidence "Gateway reachable" (CT-105; gateway path per `grafana-metric-gateway`).
|
||||
- **alertmanager** — `GET /api/v2/alerts` (+ optional bearer); evidence cluster version (CT-106).
|
||||
- **jellyfin** — `JellyfinClient.users()`; evidence "<N> users" (CT-107).
|
||||
- **authentik** — directory endpoint GET; evidence slug/"connected" (CT-108).
|
||||
- **ssh_tasks** — reuses `build_ssh_client(store, service)` + `connect()`; banner/auth translation; no duplication of `test_machine_ssh` internals (CT-109).
|
||||
- **nextcloud** — `GET /status.php` (unauth); evidence version (CT-110).
|
||||
- **backups** — `test_callable=None` → `{ok: True, detail: "No test needed"}` (CT-111).
|
||||
- `routers/services.py` — `POST /api/services/test`: validation-first (422 on malformed config, no network call), dispatch, **zero persistence** (no `upsert`/`update_setting`), INFO log only type+ok (sanitized; no secrets) (CT-102, CT-103, CT-113).
|
||||
- Tests: `test_credential_tester.py` (per-routine success + failure, dispatch, validation-before-test, **no-persistence assertion** `test_no_persistence_after_test`, qBit "Fails." → auth message).
|
||||
|
||||
### Slice 2 — Frontend Test button + gating (commit `9972514`, amended)
|
||||
|
||||
- `types/index.ts` — `TestResult` interface (CT-113).
|
||||
- `api/services.ts` + `hooks/useServices.ts` — `testServiceInstance` + `useTestServiceInstance` mutation (no cache invalidation; test is side-effect-free).
|
||||
- NEW `components/ServiceTestPanel.tsx` — **presentational** shared component (cleaner than the design's stateful version — deviation N-6). Props: `{ input, onResult, disabled }`. Renders Test button + `Testing…` state + result pill (✓ green evidence / ✗ red detail) + Save-anyway checkbox (only on failure). Parent owns `testResult` + `saveAnyway` state; store-previous pattern clears on input change (CT-114, CT-115, CT-116, CT-117).
|
||||
- Wired into BOTH surfaces: `CreateServiceDialog` (`ServicesPage.tsx`) AND `ServiceConfigEditor` (`Settings.tsx` — the correct edit surface per design source-finding, not `ServicePage.tsx`). Create/Save confirm gated on `testPassed || saveAnyway` (CT-118).
|
||||
- Tests: 7 `ServiceTestPanel.test.tsx` cases (button states, success/failure pills, checkbox toggle).
|
||||
|
||||
## Deviations from tasks.md / design
|
||||
|
||||
- **N-6 (intentional improvement):** `ServiceTestPanel` is presentational; the design's stateful version was simplified. Parent owns result + saveAnyway state. Cleaner; works identically in both surfaces.
|
||||
- **Edit surface correction:** spec CT-115 said `ServicePage.tsx`; the actual edit dialog is `Settings.tsx::ServiceConfigEditor` (design source-finding). Tasks targeted the right file.
|
||||
|
||||
## Final gate results
|
||||
|
||||
| Gate | Result |
|
||||
|---|---|
|
||||
| `backend && PYTHONPATH=src python3 -m pytest -q` | **362 passed** (+31 new), 2 warnings (pre-existing pythonjsonlogger) |
|
||||
| `backend && PYTHONPATH=src python3 -m ruff check src tests` | **All checks passed** |
|
||||
| `frontend && npm run build` | **exit 0** (pre-existing chunk-size warning) |
|
||||
| `frontend && npm run lint` | **0 errors**, 1 pre-existing warning (`WidgetConfigDialog.tsx`, untouched) |
|
||||
| `frontend && npx vitest run` | **158 passed** (+7 ServiceTestPanel) |
|
||||
|
||||
## Verification
|
||||
|
||||
See `verify-report.md` — adversarial fresh-context review: **21/21 PASS**, all gates green. No blocking code findings. Archive blocker is doc-only (this file + ticked tasks clear it). Non-blocking notes: N-2 (no-secret-logs test sends empty secrets — weak coverage, not a defect), N-4 (edit flow requires re-typing secrets — inherent to no-persistence), N-5 (gating proven by source, not page-level test).
|
||||
@@ -0,0 +1,242 @@
|
||||
# Archive Report — `service-credential-tester`
|
||||
|
||||
> Phase: **archive** · Change: `service-credential-tester` · Repo: `/home/user/manage`
|
||||
> Mode: file-backed (`artifactStore: openspec`). No source-code edits; only OpenSpec artifacts
|
||||
> were touched. **Not committed** — the parent/orchestrator owns the archive commit. No push, no `gh`.
|
||||
|
||||
**Status: ARCHIVED.** All eight lifecycle phases are complete (proposal → spec → design → tasks →
|
||||
apply → verify → sync → **archive**). Every archive precondition is verified PASS (see §2). The
|
||||
canonical `openspec/specs/service-credential-testing/spec.md` (a **new** domain created by
|
||||
`sdd-sync` — pure `## ADDED Requirements`, 21 requirements CT-101 … CT-121) remains in place as the
|
||||
durable end-state spec and is **not** moved (archive never moves canonical specs). The change folder
|
||||
was moved to `openspec/changes/archive/2026-07-09-service-credential-tester/` via `git mv` to
|
||||
preserve history.
|
||||
|
||||
---
|
||||
|
||||
## 0. Archive disposition
|
||||
|
||||
- **Disposition: `archived`.** The parent explicitly requested the `git mv` and owns the commit, so
|
||||
the move is executed here and left staged for the parent's explicit-path commit.
|
||||
- **Archive convention:** OpenSpec SDD archive contract for `openspec` mode — completed file-backed
|
||||
sync → write the in-folder archive report → move the change folder to
|
||||
`openspec/changes/archive/YYYY-MM-DD-{change}/`. No standalone manifest/index exists under
|
||||
`openspec/` (only `config.yaml`, `changes/`, `specs/`), so the folder move **is** the archive
|
||||
mechanism. No `rules.archive` override exists in `openspec/config.yaml` (verified: no `archive`
|
||||
key under `rules`).
|
||||
- **Target archived path:** `openspec/changes/archive/2026-07-09-service-credential-tester/`
|
||||
- **Archive date:** `2026-07-09` (ISO).
|
||||
- **Canonical spec left in place (not moved):** `openspec/specs/service-credential-testing/spec.md` —
|
||||
21 requirements (CT-101 … CT-121). Verified present and untouched after the move (sha256
|
||||
`edcdfecc54fafb3729a9c4f5bcbe48a2f6b6b430336e20823bb363b9a2c2e330`, unchanged pre/post move).
|
||||
- **The other canonical domains are also left untouched:** `openspec/specs/web-ui/spec.md`,
|
||||
`openspec/specs/prometheus-charting/spec.md`, `openspec/specs/service-storage/spec.md`.
|
||||
- **Audit-trail integrity:** the change folder was moved as a whole, including the legacy flat
|
||||
`spec.md`, the per-domain delta `specs/service-credential-testing/spec.md`, `apply-progress.md`,
|
||||
`verify-report.md`, and `sync-report.md`, which travel with the record. Nothing was silently
|
||||
deleted or rewritten.
|
||||
|
||||
## 1. Native `sdd-status` read & discrepancy statement
|
||||
|
||||
The native `gentle-pi.sdd-status` engine supplied by the parent reports **non-actionable state for
|
||||
this archive** because it was resolved without a change context: `changeName: null`,
|
||||
`artifacts: all missing`, `applyState: blocked`, `dependencies.archive: blocked`,
|
||||
`blockedReasons: ["Change selection is ambiguous: per-instance-hook-scoping,
|
||||
service-credential-tester."]`, `isNonAuthoritative: false`. This is a **parent-resolution artifact**:
|
||||
the engine auto-detected two active changes and could not pick one. The ambiguity does **not**
|
||||
reflect the state of `service-credential-tester`, which this archive task was **explicitly
|
||||
assigned**.
|
||||
|
||||
**Discrepancy with the parent's authoritative confirmed state — RESOLVED in favor of the parent.**
|
||||
The parent physically verified (and this executor re-confirmed directly against the filesystem in
|
||||
§2) that all 29 tasks are ticked, `apply-progress.md` is present and records the work, the verify
|
||||
report confirms functional completion (21/21 PASS), and the sync report confirms a SYNCED canonical
|
||||
domain. Per the archive contract's non-authoritative-store carve-out guidance and the parent's
|
||||
explicit instruction ("native sdd-status may report stale blockers — DISREGARD; parent verified.
|
||||
PROCEED"), the stale `archive: blocked` / "ambiguous" labels are **disregarded** and the archive
|
||||
**proceeds**.
|
||||
|
||||
Direct filesystem re-validation (§2) is the source of truth for this report.
|
||||
|
||||
## 2. Archive preconditions (validated directly against the filesystem)
|
||||
|
||||
| Precondition | Evidence | Result |
|
||||
|---|---|---|
|
||||
| Verify report present | `verify-report.md` | ✓ verdict **PASS (functionally)** — 21/21 requirements PASS |
|
||||
| Verify clearly passing — no unresolved `FAIL`/`BLOCKED`/`CRITICAL` | the two verify CRITICALs were **reconcilable archive-only** items (missing `apply-progress.md` + 29 unchecked boxes), now **resolved**; no unresolved critical verification issue remains | ✓ |
|
||||
| Sync report present & successful | `sync-report.md` → **Status: SYNCED** | ✓ |
|
||||
| Canonical spec exists (sync target, created) | `openspec/specs/service-credential-testing/spec.md` (21 requirements, CT-101 … CT-121) | ✓ |
|
||||
| Change-side domain delta exists | `specs/service-credential-testing/spec.md` (`## ADDED Requirements`) | ✓ |
|
||||
| Delta op-class = pure additive (ADDED; new domain; no MODIFIED/REMOVED) | ADDED=21, MODIFIED=0, REMOVED=0, RENAMED=0 | ✓ |
|
||||
| Requirement-ID parity (flat ↔ delta ↔ canonical) | 21 == 21 == 21; identical IDs CT-101 … CT-121 | ✓ |
|
||||
| proposal / design / tasks artifacts present | all populated | ✓ |
|
||||
| **Final Task Completion Gate — zero unchecked `- [ ]`** | `grep -cE '^\s*- \[ \]' tasks.md` → **0**; `grep -cE '^\s*- \[x\]'` → **29** | ✓ |
|
||||
| `apply-progress.md` present & records the work | present; status "complete — all 29 tasks done, all gates green"; 2 slices documented | ✓ |
|
||||
| No active same-domain (`service-credential-testing`) collision | new domain; the only other active change (`per-instance-hook-scoping`) carries no `service-credential-testing` delta; `sameDomainActiveChanges: []`, `collisions: []` (per sync-report §5) | ✓ |
|
||||
|
||||
**Stale-checkbox reconciliation note.** At verify time, all 29 implementation/verification checkboxes
|
||||
were unchecked and `apply-progress.md` did not exist (verify findings B-1/B-2). That condition was
|
||||
reconciled **before** archive: `apply-progress.md` was authored documenting the two landed slices
|
||||
(`3391fbc` slice-1 backend, `f6c67bd` slice-2 frontend) and the gate results, and all 29 boxes are
|
||||
now ticked. `apply-progress.md` plus the verify report prove every previously-unchecked task
|
||||
complete. No archive-time mechanical checkbox repair was needed — the gate now passes on the
|
||||
persisted `tasks.md` as-is (0 unchecked). No partial-archive approval applies.
|
||||
|
||||
## 3. Artifacts read (archive preflight)
|
||||
|
||||
- `openspec/changes/service-credential-tester/proposal.md`
|
||||
- `openspec/changes/service-credential-tester/spec.md` (flat, authoritative planning artifact — 21 requirements CT-101 … CT-121)
|
||||
- `openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md` (change-side domain delta — `## ADDED Requirements`)
|
||||
- `openspec/changes/service-credential-tester/design.md`
|
||||
- `openspec/changes/service-credential-tester/tasks.md`
|
||||
- `openspec/changes/service-credential-tester/apply-progress.md`
|
||||
- `openspec/changes/service-credential-tester/verify-report.md`
|
||||
- `openspec/changes/service-credential-tester/sync-report.md`
|
||||
- `openspec/specs/service-credential-testing/spec.md` (canonical, sync target — verified present and untouched)
|
||||
- `openspec/config.yaml` (rules: proposal/tasks; no `rules.archive` override)
|
||||
- House-style reference: `openspec/changes/archive/2026-07-09-grafana-metric-gateway/archive-report.md`
|
||||
|
||||
> The legacy flat `spec.md` is **not** the only spec artifact: a per-domain delta
|
||||
> (`specs/service-credential-testing/spec.md`) and a canonical spec both exist, so the "legacy flat
|
||||
> spec as the *only* artifact" archive-block condition does not apply. The flat spec travels with the
|
||||
> archived folder as part of the audit trail.
|
||||
|
||||
## 4. Domains synced & requirement delta
|
||||
|
||||
| Domain | Change-side delta | Canonical | Action |
|
||||
|---|---|---|---|
|
||||
| `service-credential-testing` | `specs/service-credential-testing/spec.md` | `openspec/specs/service-credential-testing/spec.md` | **NEW domain — ADDED** — 21 requirements copied into the canonical store as a brand-new spec |
|
||||
|
||||
- **ADDED (21)** — all to the new `service-credential-testing` domain (canonical did not exist
|
||||
pre-change). IDs and text preserved verbatim from the verified flat `spec.md`. Grouped logically:
|
||||
- *Endpoint shape, validation, dispatch* — CT-101, CT-102, CT-103
|
||||
- *Per-type test routines* — CT-104, CT-105, CT-106, CT-107, CT-108, CT-109, CT-110, CT-111
|
||||
- *No-persistence + security* — CT-112, CT-113
|
||||
- *Frontend test UI* — CT-114, CT-115, CT-116, CT-117, CT-118
|
||||
- *Tests + gate greenness* — CT-119, CT-120, CT-121
|
||||
- **MODIFIED (0)** — none (new domain; no pre-existing canonical requirements to replace).
|
||||
- **REMOVED (0)** · **RENAMED (0)** — nothing destructive.
|
||||
|
||||
## 5. Final lifecycle status (all 8 phases done)
|
||||
|
||||
| Phase | Status | Evidence |
|
||||
|---|---|---|
|
||||
| Proposal | ✅ done | `proposal.md` |
|
||||
| Spec | ✅ done | flat `spec.md` (21) + domain delta `specs/service-credential-testing/spec.md` (21 ADDED) |
|
||||
| Design | ✅ done | `design.md` |
|
||||
| Tasks | ✅ done | `tasks.md` — **29/29** checked, zero `- [ ]` |
|
||||
| Apply | ✅ done | 2 slices delivered (commits `3391fbc` slice-1 backend, `f6c67bd` slice-2 frontend) |
|
||||
| Verify | ✅ PASS | `verify-report.md` — 21/21 PASS; gates green (verify's two CRITICALs were archive-only hygiene, now resolved) |
|
||||
| Sync | ✅ done | `sync-report.md` — SYNCED; canonical `service-credential-testing` domain ADDED (now 21 requirements) |
|
||||
| Archive | ✅ done | this report + folder move performed |
|
||||
|
||||
## 6. Gate results (per verify-report / apply-progress)
|
||||
|
||||
| Gate | Command | Result |
|
||||
|---|---|---|
|
||||
| Backend tests | `cd backend && PYTHONPATH=src python3 -m pytest -q` | **PASS** — 362 passed (2 pre-existing warnings) |
|
||||
| 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 (pre-existing chunk-size advisory) |
|
||||
| Frontend lint | `cd frontend && npm run lint` | **PASS** — 0 errors (1 pre-existing unrelated warning) |
|
||||
| Frontend tests | `cd frontend && npx vitest run` | **PASS** — 158 passed / 45 files |
|
||||
|
||||
## 7. Carry-over follow-ups & non-blocking notes (recorded for the record)
|
||||
|
||||
1. **[RESOLVED — verify N-2] No-secret-logs test was strengthened.** Verify flagged the original
|
||||
`test_secrets_not_logged` as weak/misleading because it sent an empty `secrets: {}` payload (the
|
||||
assertion was then trivially satisfied). The test was rewritten (`backend/tests/test_api.py`)
|
||||
to drive the **full endpoint path** (validate → dispatch to the real `test_callable` → success
|
||||
log) with **real-looking secret payloads** — `grafana_api_key: "glc_somethingverysecret"` and
|
||||
`password: "SUPER-SECRET-PW-12345"` — mocking at the network boundary so both requests succeed,
|
||||
then asserting that neither the full values nor meaningful fragments (`"verysecret"`, `"SUPER"`)
|
||||
leak into `caplog.text`. This genuinely proves CT-113 (secrets never logged). The verify finding
|
||||
is closed.
|
||||
2. **[DESIGN — intentional improvement, verify N-6] `ServiceTestPanel` is presentational.** The
|
||||
design proposed a stateful panel (`useEffect([input])`); the shipped component is deliberately
|
||||
**presentational** (props `{ input, onResult, disabled }`), with the parent owning `testResult`
|
||||
- `saveAnyway` state and the store-previous pattern that clears the result on input change. This
|
||||
avoids the React `setState`-in-effect footgun and works identically in both surfaces. All of
|
||||
CT-114 … CT-118 are still satisfied via parent wiring. Documented in the component docstring and
|
||||
`apply-progress.md`. Informational only.
|
||||
3. **[DESIGN — source-finding] Edit surface is `Settings.tsx::ServiceConfigEditor`, not
|
||||
`ServicePage.tsx`.** The spec's literal CT-115 named `ServicePage.tsx`, but that file is
|
||||
read-only (it renders tab content for a configured service). The actual service add/edit dialog
|
||||
surfaced by the app is `ServiceConfigEditor` inside `Settings.tsx` (create surface:
|
||||
`CreateServiceDialog` in `ServicesPage.tsx`). The design's §0 source-finding corrected this, and
|
||||
tasks targeted the right file. `ServiceTestPanel` is wired into **both** `CreateServiceDialog`
|
||||
and `ServiceConfigEditor`. `ServicePage.tsx` was correctly left untouched.
|
||||
4. **[USER VALUE — pain resolved] qBittorrent "login failed" surfaced at the UI layer (CT-104).**
|
||||
Previously a wrong qBittorrent username/password produced only a generic `RuntimeError` /
|
||||
"login failed" buried in backend logs, leaving the operator to dig through logs to diagnose a
|
||||
credential problem. The credential tester now logs into qBittorrent + probes `maindata()` during
|
||||
the live Test, and specifically translates the `"Fails."` / `"login failed"` response into
|
||||
*"Authentication failed — qBittorrent rejected the credentials."*, shown directly in the result
|
||||
pill (✗ red detail). The credential-error pain is resolved at the UI layer — the operator gets
|
||||
an actionable message without touching logs.
|
||||
5. **[INFO — non-blocking, verify N-3] Defensive `logger.exception`.** `services.py` emits a full
|
||||
traceback via `logger.exception` only on the rare path where a `test_callable` itself raises
|
||||
(all routines catch internally). Low risk; a future change may switch to a truncated
|
||||
`logger.warning`. Not blocking.
|
||||
6. **[INFO — non-blocking, verify N-4] Edit flow requires re-typing secrets to test.** In
|
||||
`ServiceConfigEditor`, stored secrets are encrypted at rest and are not sent as plaintext on the
|
||||
test request, so testing an existing authed service without re-entering its secret yields a
|
||||
"required" validation error. This is inherent to the no-persistence design (the test operates on
|
||||
request-body plaintext) and is hinted by the "Leave blank to keep the current value" copy.
|
||||
Non-blocking UX note; a future change may document it more prominently in the UI/user docs.
|
||||
7. **[INFO — non-blocking, verify N-5] Page-level gating proven by source, not a page test.**
|
||||
`ServiceTestPanel.test.tsx` covers the presentational panel (button states, success/failure
|
||||
pills, checkbox toggle). Because the panel is stateless, there is no explicit automated test that
|
||||
the Create/Save confirm button enables only after a passing test, or that editing a field clears
|
||||
`testPassed`. The logic is correct by source inspection (store-previous pattern in both parents);
|
||||
a future change may add a page-level integration test.
|
||||
8. **[INFO]** The working tree carries unrelated dirty/untracked items **not owned by this archive**:
|
||||
an uncommitted cosmetic (indentation/prettier reflow, no functional diff) modification to
|
||||
`frontend/src/pages/ServicesPage.tsx` and untracked `.pi-tmp/*` scratch files (other-change
|
||||
strays). Per discipline these were **left untouched**.
|
||||
|
||||
## 8. Residual risks & destructive-merge statement
|
||||
|
||||
- **Destructive sync / merge:** **none destructive.** This was a brand-new canonical domain
|
||||
(`service-credential-testing`) — a pure `## ADDED Requirements` delta. Zero MODIFIED and zero
|
||||
REMOVED requirements, so no destructive-removal guard was triggered and no destructive-sync parent
|
||||
approval was required.
|
||||
- **Backend / data-contract impact:** none. The `POST /api/services/test` endpoint is
|
||||
**side-effect-free** (no persistence, no cache invalidation) and **sanitized** (logs only
|
||||
`type` + `ok`, never secrets). It gates on the existing validation path (`_validate_input`) and
|
||||
dispatches through the closed `test_callable` registry that mirrors `SERVICE_DEFINITIONS`. Archive
|
||||
touched only OpenSpec docs + the folder move.
|
||||
- **No critical verification issues** remain (CRITICAL issues are non-overridable; the two verify
|
||||
CRITICALs were the reconcilable checkbox/apply-progress gap, now resolved; N-2's weak-test gap was
|
||||
strengthened and closed).
|
||||
- **No browser/visual smoke** was performed (out of scope); the panel is covered by Vitest component
|
||||
tests only.
|
||||
- **Memory observation IDs:** none — `artifactStore: openspec`; traceability lives in the filesystem
|
||||
archive + canonical spec.
|
||||
|
||||
## 9. Move performed
|
||||
|
||||
```
|
||||
git mv openspec/changes/service-credential-tester openspec/changes/archive/2026-07-09-service-credential-tester
|
||||
```
|
||||
|
||||
- **All 9 artifacts confirmed present at the archived path:** `proposal.md`, `spec.md`,
|
||||
`specs/service-credential-testing/spec.md` (delta), `design.md`, `tasks.md`, `apply-progress.md`,
|
||||
`verify-report.md`, `sync-report.md`, `archive-report.md` (this file).
|
||||
- **Canonical `openspec/specs/service-credential-testing/spec.md` remains in place** (verified
|
||||
untouched after the move — sha256 unchanged). `openspec/specs/web-ui/spec.md`,
|
||||
`openspec/specs/prometheus-charting/spec.md`, and `openspec/specs/service-storage/spec.md` also
|
||||
untouched.
|
||||
- Renames were left **staged** (R100 detection preserved) for the parent's explicit-path commit.
|
||||
`git restore --staged` was **not** run after the `git mv`.
|
||||
|
||||
---
|
||||
|
||||
### Appendix — Files written/moved by this archive (OpenSpec only; no source code)
|
||||
|
||||
- **Written:** `openspec/changes/service-credential-tester/archive-report.md` (this file) — at the
|
||||
active path before the move; travels with the move into the archive.
|
||||
- **Moved (via `git mv`):** the entire
|
||||
`openspec/changes/service-credential-tester/` directory →
|
||||
`openspec/changes/archive/2026-07-09-service-credential-tester/`.
|
||||
- **Left in place (durable canonical):** `openspec/specs/service-credential-testing/spec.md`.
|
||||
- **Not committed / not pushed** — the parent owns the commit with explicit paths.
|
||||
@@ -0,0 +1,674 @@
|
||||
# SDD Design: Service Credential Tester
|
||||
|
||||
**Change:** `service-credential-tester`
|
||||
**Phase:** design
|
||||
**Date:** 2026-07-09
|
||||
|
||||
## 0. Source findings (read before anything else)
|
||||
|
||||
The proposal and spec were written against a mental model. Reading actual source surfaced deviations the design must account for. Trust source, not assumptions.
|
||||
|
||||
| Spec/proposal claim | Actual source reality | Design impact |
|
||||
|---|---|---|
|
||||
| Spec CT-115: "the edit dialog on `ServicePage.tsx`" | **`ServicePage.tsx` has no edit dialog.** It's a read-only tabbed view (Overview + content tabs + Widgets). Service editing lives in `Settings.tsx` as `ServiceConfigEditor` (a master-detail panel, lines ~1435–1596). The "Save" button calls `saveService.mutateAsync(buildInput())` directly (no try/catch — a latent error-swallow like the pre-#1-fix create dialog). | The Test button is added to **two** edit surfaces: `CreateServiceDialog` (ServicesPage.tsx) and `ServiceConfigEditor` (Settings.tsx). NOT ServicePage.tsx. Spec CT-115 should note this textual drift. |
|
||||
| Proposal §5.1.3: "the edit dialog on `ServicePage.tsx`" | Same as above. | Same fix — target is `Settings.tsx::ServiceConfigEditor`. |
|
||||
| Spec CT-109: "ssh_tasks test reuses the `test_machine_ssh` connect flow" | `test_machine_ssh` (settings.py:122–180) is a **router endpoint**, not a reusable function. Its logic (build client → connect → translate errors) is inlined. However, `build_ssh_client` in `task_runner.py` already builds an `RemoteSSHClient` from a `ServiceRecord` — the exact pattern needed. | The ssh_tasks test routine constructs an `RemoteSSHClient` via `build_ssh_client(store, service_record)` and calls `.connect()`, then translates errors using the same message patterns as `test_machine_ssh` (protocol banner / auth failed). It needs a `SettingsStore` (for SSH key resolution) — see §3.6 for how test_callables get store access. |
|
||||
| Proposal: "construct a `QbittorrentClient` from the config + decrypted secrets" | `QbittorrentClient.__init__(base_url, username, password, timeout)` takes **positional strings**, not a `config` dict. The `qbittorrent` config has `base_url` + `timeout_seconds`; secrets have `username` + `password`. | The test routine extracts fields from `config`/`secrets` dicts explicitly: `QbittorrentClient(config["base_url"], secrets["username"], secrets["password"], config.get("timeout_seconds", 10))`. |
|
||||
| Proposal: "Alertmanager test probes `/api/v2/alerts`" | `AlertmanagerWidgetSource.fetch` (sources.py) already hits `/api/v2/alerts` with optional bearer auth. The exact URL pattern + auth-header logic is reusable. | The test routine mirrors the widget source's request shape (GET `{base_url}/api/v2/alerts`, optional `Authorization: Bearer {api_key}`). |
|
||||
| Proposal: "Authentik test probes its directory endpoint" | `routers/authentik_users.py::_build_client` builds an `AuthentikClient(base_url, api_token, timeout)`. The client has a `.users()` method that hits the directory endpoint. | The test routine constructs an `AuthentikClient` and calls `.users(page=1, page_size=1)` — the lightest possible probe. |
|
||||
| Proposal: "Nextcloud test probes `/status.php`" | `NextcloudConfig` has `base_url` + `username` (not a ServiceBaseUrl). Secret is `app_password`. `/status.php` is unauthenticated. | The test routine does a simple `GET {base_url}/status.php` (no auth headers); extracts `version` from the JSON response. |
|
||||
| Proposal: "test_callable field on `ServiceDefinition`" | `ServiceDefinition` is a `@dataclass(frozen=True)` with fields: `service_type, name, description, config_model, secret_fields, widget_kinds`. Adding `test_callable` after `widget_kinds` works (dataclass field ordering — it has a default). | New field `test_callable: TestCallable | None = None` placed last. The type alias `TestCallable = Callable[[dict[str, Any], dict[str, str], SettingsStore], TestResult]` includes the store for SSH key resolution. |
|
||||
|
||||
No proposal/spec scope change is required — the *intent* (per-type credential tester) still holds. The findings above refine implementation details and correct the edit-surface target.
|
||||
|
||||
---
|
||||
|
||||
## 1. Architecture overview
|
||||
|
||||
A new `POST /api/services/test` endpoint validates the input (reusing `_validate_input`), resolves the service definition's `test_callable`, and runs it. Each integration declares its own `test_connection` routine alongside its `DEFINITION`. A shared error-translation helper maps common exception types to human-friendly strings. The frontend gains a Test button + result pill + gating in both the create dialog and the settings edit panel.
|
||||
|
||||
```
|
||||
POST /api/services/test
|
||||
└► _validate_input(body) → 422 on malformed config (CT-102)
|
||||
└► definition = get_service_definition(body.service_type)
|
||||
└► if definition.test_callable is None → {ok: true, detail: "No test..."} (CT-103/111)
|
||||
└► result = test_callable(config, secrets, store)
|
||||
└► per-type routine:
|
||||
qbittorrent → QbittorrentClient.maindata()
|
||||
prometheus → POST {grafana_url}/api/ds/query (expr "up")
|
||||
alertmanager → GET {base_url}/api/v2/alerts
|
||||
jellyfin → JellyfinClient.users()
|
||||
authentik → AuthentikClient.users(page=1, page_size=1)
|
||||
ssh_tasks → RemoteSSHClient.connect() via build_ssh_client
|
||||
nextcloud → GET {base_url}/status.php
|
||||
└► translate_connection_error(exc) → TestResult(ok=False, detail=...)
|
||||
└► return {ok, detail, evidence} → HTTP 200 (CT-101)
|
||||
```
|
||||
|
||||
No persistence. No secrets in logs. Auth-gated identically to every other `/api/services/*` endpoint.
|
||||
|
||||
---
|
||||
|
||||
## 2. Core data structures
|
||||
|
||||
### 2.1 TestResult dataclass (CT-101)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/base.py`
|
||||
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class TestResult:
|
||||
"""Outcome of a credential/connectivity test for a service instance."""
|
||||
ok: bool
|
||||
detail: str
|
||||
evidence: str | None = None
|
||||
```
|
||||
|
||||
### 2.2 TestCallable type alias + test_callable field on ServiceDefinition
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/base.py`
|
||||
|
||||
```python
|
||||
from typing import Callable
|
||||
|
||||
# A test routine receives (config, secrets, store). The store is needed for
|
||||
# ssh_tasks (SSH-key resolution via store.get_ssh_key). Other types ignore it.
|
||||
TestCallable = Callable[["dict[str, Any]", "dict[str, str]", "SettingsStore"], TestResult]
|
||||
```
|
||||
|
||||
`ServiceDefinition` gains (placed last, after `widget_kinds`, with a default):
|
||||
|
||||
```python
|
||||
@dataclass(frozen=True)
|
||||
class ServiceDefinition:
|
||||
service_type: str
|
||||
name: str
|
||||
description: str
|
||||
config_model: type[ServiceConfigBase]
|
||||
secret_fields: list[SecretField]
|
||||
widget_kinds: list[WidgetKind]
|
||||
test_callable: TestCallable | None = None # NEW — placed last
|
||||
```
|
||||
|
||||
**Why `SettingsStore` in the signature:** `ssh_tasks` needs `store.get_ssh_key(ssh_key_id)` to resolve the private key material. Every other type ignores it. Making it a uniform parameter avoids a special-case dispatch for one type.
|
||||
|
||||
### 2.3 Shared error-translation helper
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/base.py` (or a new `integrations/test_helpers.py` if base.py gets too large — design recommends base.py for discoverability)
|
||||
|
||||
```python
|
||||
def translate_connection_error(exc: Exception, *, context: str = "") -> TestResult:
|
||||
"""Map a common connection/auth exception to a human-friendly TestResult.
|
||||
|
||||
Handles the patterns extracted from ``test_machine_ssh`` (settings.py) plus
|
||||
the HTTP-client patterns from the widget sources.
|
||||
"""
|
||||
message = str(exc)
|
||||
lowered = message.lower()
|
||||
|
||||
# Auth failures (HTTP 401/403 or auth-specific strings)
|
||||
if isinstance(exc, requests.HTTPError):
|
||||
status_code = exc.response.status_code if exc.response is not None else 0
|
||||
if status_code in (401, 403):
|
||||
return TestResult(ok=False, detail=f"Authentication failed — the service rejected the credentials ({status_code}).")
|
||||
if "authentication failed" in lowered or "no authentication methods available" in lowered:
|
||||
return TestResult(ok=False, detail="Authentication failed — check the credentials, API key, or SSH key.")
|
||||
|
||||
# Connection refused / DNS / unreachable
|
||||
if isinstance(exc, (requests.ConnectionError, ConnectionRefusedError, OSError)):
|
||||
if "name or service not known" in lowered or "nodename nor servname" in lowered or "getaddrinfo failed" in lowered:
|
||||
return TestResult(ok=False, detail="Host not found — check the URL/hostname for typos.")
|
||||
return TestResult(ok=False, detail="Connection refused — the service is not reachable at the configured address.")
|
||||
|
||||
# SSL / certificate errors
|
||||
if "ssl" in lowered or "certificate" in lowered:
|
||||
return TestResult(ok=False, detail="SSL/TLS error — the service's certificate is invalid or untrusted.")
|
||||
|
||||
# Timeout
|
||||
if isinstance(exc, (requests.Timeout, TimeoutError, asyncio.TimeoutError)):
|
||||
return TestResult(ok=False, detail="Connection timed out — the service did not respond in time.")
|
||||
|
||||
# SSH banner (from test_machine_ssh pattern)
|
||||
if "protocol banner" in lowered:
|
||||
return TestResult(ok=False, detail="SSH banner not received — confirm the SSH service is running and the port is correct.")
|
||||
|
||||
# Fallback
|
||||
prefix = f"{context}: " if context else ""
|
||||
return TestResult(ok=False, detail=f"{prefix}{message[:200]}")
|
||||
```
|
||||
|
||||
Each per-type routine wraps its probe in `try/except` and calls `translate_connection_error` for unexpected exceptions, but handles its **type-specific** auth failures directly (e.g., qBit `"Fails."` response).
|
||||
|
||||
---
|
||||
|
||||
## 3. Per-type test routines
|
||||
|
||||
Each routine lives alongside its integration's `DEFINITION`. The signature is `(config: dict, secrets: dict, store: SettingsStore) -> TestResult`. All use `requests` (synchronous) wrapped in a short timeout — no asyncio needed (the endpoint handler calls the callable synchronously).
|
||||
|
||||
### 3.1 qbittorrent (CT-104)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/qbittorrent.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""Login + probe maindata; surface auth failures specifically."""
|
||||
try:
|
||||
base_url = str(config.get("base_url") or "")
|
||||
username = str(secrets.get("username") or "")
|
||||
password = str(secrets.get("password") or "")
|
||||
timeout = int(config.get("timeout_seconds") or 10)
|
||||
client = QbittorrentClient(base_url, username, password, timeout=timeout)
|
||||
data = client.maindata()
|
||||
version = str(data.get("server_state", {}).get("qbittorrent_version", "") or "connected")
|
||||
return TestResult(ok=True, detail="Connected to qBittorrent.", evidence=version)
|
||||
except RuntimeError as exc:
|
||||
# QbittorrentClient._login raises RuntimeError("qBittorrent login failed: Fails.")
|
||||
lowered = str(exc).lower()
|
||||
if "login failed" in lowered:
|
||||
return TestResult(ok=False, detail="Authentication failed — qBittorrent rejected the credentials.")
|
||||
return translate_connection_error(exc, context="qBittorrent")
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context="qBittorrent")
|
||||
```
|
||||
|
||||
**Evidence:** qBittorrent version from `server_state` (fallback `"connected"`).
|
||||
|
||||
### 3.2 prometheus (CT-105)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/prometheus.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""POST {grafana_url}/api/ds/query with expr 'up' via the Grafana gateway."""
|
||||
try:
|
||||
grafana_url = str(config.get("grafana_url") or "").rstrip("/")
|
||||
api_key = str(secrets.get("grafana_api_key") or "")
|
||||
datasource_uid = str(config.get("datasource_uid") or "prometheus")
|
||||
timeout = int(config.get("timeout_seconds") or 10)
|
||||
if not grafana_url:
|
||||
return TestResult(ok=False, detail="Grafana gateway URL is required.")
|
||||
if not api_key:
|
||||
return TestResult(ok=False, detail="Grafana API key is required.")
|
||||
body = {
|
||||
"queries": [{"datasource": {"uid": datasource_uid, "type": "prometheus"},
|
||||
"expr": "up", "format": "time_series",
|
||||
"intervalMs": 15000, "maxDataPoints": 1, "refId": "A"}],
|
||||
"from": "now-1m", "to": "now",
|
||||
}
|
||||
resp = requests.post(
|
||||
f"{grafana_url}/api/ds/query", json=body, timeout=timeout,
|
||||
headers={"Authorization": f"Bearer {api_key}", "Content-Type": "application/json"},
|
||||
)
|
||||
resp.raise_for_status()
|
||||
return TestResult(ok=True, detail="Grafana gateway reachable.", evidence="Gateway reachable; datasource responded.")
|
||||
except requests.HTTPError as exc:
|
||||
return translate_connection_error(exc, context="Prometheus via Grafana")
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context="Prometheus via Grafana")
|
||||
```
|
||||
|
||||
**Evidence:** `"Gateway reachable; datasource responded."` — validates the full path (Grafana up + datasource reachable + Prom responding).
|
||||
|
||||
### 3.3 alertmanager (CT-106)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/alertmanager.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""GET /api/v2/alerts (or /api/v2/status) with optional bearer auth."""
|
||||
try:
|
||||
base_url = str(config.get("base_url") or "").rstrip("/")
|
||||
timeout = int(config.get("timeout_seconds") or 5)
|
||||
headers = {}
|
||||
api_key = str(secrets.get("api_key") or "")
|
||||
if api_key:
|
||||
headers["Authorization"] = f"Bearer {api_key}"
|
||||
resp = requests.get(f"{base_url}/api/v2/status", headers=headers, timeout=timeout)
|
||||
resp.raise_for_status()
|
||||
payload = resp.json()
|
||||
version = str(payload.get("versionInfo", {}).get("version", "") or "connected")
|
||||
return TestResult(ok=True, detail="Connected to Alertmanager.", evidence=version)
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context="Alertmanager")
|
||||
```
|
||||
|
||||
**Evidence:** Alertmanager cluster version from `/api/v2/status`.
|
||||
|
||||
### 3.4 jellyfin (CT-107)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/jellyfin.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""Call JellyfinClient.users() — the lightest authenticated probe."""
|
||||
try:
|
||||
base_url = str(config.get("base_url") or "")
|
||||
api_key = str(secrets.get("api_key") or "")
|
||||
timeout = int(config.get("timeout_seconds") or 10)
|
||||
client = JellyfinClient(base_url, api_key, timeout=timeout)
|
||||
users = client.users()
|
||||
return TestResult(ok=True, detail="Connected to Jellyfin.", evidence=f"{len(users)} users")
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context="Jellyfin")
|
||||
```
|
||||
|
||||
**Evidence:** `"<N> users"`.
|
||||
|
||||
### 3.5 authentik (CT-108)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/authentik.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""Probe AuthentikClient.users(page=1, page_size=1) — lightest directory call."""
|
||||
try:
|
||||
base_url = str(config.get("base_url") or "").rstrip("/")
|
||||
api_token = str(secrets.get("api_token") or "")
|
||||
timeout = float(config.get("timeout_seconds") or 10)
|
||||
client = AuthentikClient(base_url=base_url, api_token=api_token, timeout=timeout)
|
||||
result = client.users(page=1, page_size=1)
|
||||
return TestResult(ok=True, detail="Connected to Authentik.", evidence=f"{result.get('total', 0)} users")
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context="Authentik")
|
||||
```
|
||||
|
||||
**Evidence:** `"<N> users"` from the directory total.
|
||||
|
||||
### 3.6 ssh_tasks (CT-109)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/ssh_tasks.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""Build an SSH client via build_ssh_client and attempt .connect().
|
||||
|
||||
Reuses the same error-translation patterns as test_machine_ssh (banner,
|
||||
auth failed). Known-host recording is preserved (first successful connect
|
||||
records the host key, same as test_machine_ssh).
|
||||
"""
|
||||
from media_library_viewer_api.services.task_runner import build_ssh_client
|
||||
from media_library_viewer_api.widgets.sources import ServiceRecord
|
||||
|
||||
host = str(config.get("host") or "").strip()
|
||||
port = int(config.get("port") or 22)
|
||||
try:
|
||||
service = ServiceRecord(id="", service_type="ssh_tasks", name="test",
|
||||
config=config, secrets=secrets, enabled=True)
|
||||
client = build_ssh_client(store, service)
|
||||
try:
|
||||
client.connect()
|
||||
except Exception as exc:
|
||||
lowered = str(exc).lower()
|
||||
if "protocol banner" in lowered:
|
||||
return TestResult(ok=False, detail=f"SSH banner not received from {host}:{port}; confirm the SSH service is running.")
|
||||
if "no authentication methods available" in lowered or "authentication failed" in lowered:
|
||||
return TestResult(ok=False, detail=f"SSH authentication failed for {host}:{port}; check the SSH key, passphrase, or username.")
|
||||
return translate_connection_error(exc, context=f"SSH {host}:{port}")
|
||||
finally:
|
||||
client.close()
|
||||
return TestResult(ok=True, detail=f"SSH connection succeeded for {host}:{port}.", evidence=f"Connected to {host}:{port}")
|
||||
except ValueError as exc:
|
||||
return TestResult(ok=False, detail=str(exc))
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context=f"SSH {host}:{port}")
|
||||
```
|
||||
|
||||
**Evidence:** `"Connected to <host>:<port>"`. **Known-host recording:** preserved — `RemoteSSHClient.connect()` records the host key on first successful connect (same as `test_machine_ssh`).
|
||||
|
||||
### 3.7 nextcloud (CT-110)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/nextcloud.py`
|
||||
|
||||
```python
|
||||
def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:
|
||||
"""GET {base_url}/status.php (unauthenticated server probe)."""
|
||||
try:
|
||||
base_url = str(config.get("base_url") or "").rstrip("/")
|
||||
resp = requests.get(f"{base_url}/status.php", timeout=10)
|
||||
resp.raise_for_status()
|
||||
payload = resp.json()
|
||||
version = str(payload.get("version", "") or "connected")
|
||||
return TestResult(ok=True, detail="Connected to Nextcloud.", evidence=version)
|
||||
except Exception as exc:
|
||||
return translate_connection_error(exc, context="Nextcloud")
|
||||
```
|
||||
|
||||
**Evidence:** Nextcloud version from `/status.php`.
|
||||
|
||||
### 3.8 backups (CT-111)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/integrations/backups.py`
|
||||
|
||||
```python
|
||||
# No test_connection function. The DEFINITION's test_callable stays None (default).
|
||||
# The endpoint returns {ok: true, detail: "No connection test for this service type"}.
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 4. Backend endpoint design
|
||||
|
||||
### 4.1 POST /api/services/test (CT-101, CT-102, CT-103, CT-112, CT-113)
|
||||
|
||||
**File:** `backend/src/media_library_viewer_api/routers/services.py`
|
||||
|
||||
```python
|
||||
from media_library_viewer_api.integrations.base import TestResult
|
||||
|
||||
@router.post("/test")
|
||||
def test_instance(
|
||||
body: ServiceInstanceInput,
|
||||
store: SettingsStore = Depends(get_settings_store),
|
||||
) -> dict[str, Any]:
|
||||
"""Test connectivity + credentials for unsaved service input.
|
||||
|
||||
Validates first (422 on malformed config), dispatches to the per-type
|
||||
test_callable, and returns {ok, detail, evidence}. Does NOT persist.
|
||||
"""
|
||||
_validate_input(body) # raises HTTPException(422) on bad config/type/secrets
|
||||
definition = require_service_definition(body.service_type)
|
||||
|
||||
if definition.test_callable is None:
|
||||
logger.info("test requested type=%s ok=true (no test_callable)", body.service_type)
|
||||
return {"ok": True, "detail": "No connection test for this service type", "evidence": None}
|
||||
|
||||
try:
|
||||
result: TestResult = definition.test_callable(body.config, body.secrets, store)
|
||||
except Exception as exc:
|
||||
# A routine should never raise (it catches internally), but defend.
|
||||
logger.exception("test_callable raised for type=%s", body.service_type)
|
||||
result = TestResult(ok=False, detail=f"Test failed unexpectedly: {exc}")
|
||||
|
||||
logger.info("test requested type=%s ok=%s", body.service_type, result.ok)
|
||||
return {"ok": result.ok, "detail": result.detail, "evidence": result.evidence}
|
||||
```
|
||||
|
||||
**Key properties:**
|
||||
|
||||
- **Validation-first** (CT-102): `_validate_input` runs before any network call. Malformed config → 422 with the same detail format as create.
|
||||
- **No persistence** (CT-112): no `store.upsert_service`, no `store.update_setting`. The body is consumed and discarded.
|
||||
- **No secret logging** (CT-113): only `test requested type=%s ok=%s` at INFO. No body/config/secrets in any log line.
|
||||
- **Dispatch** (CT-103): keys off `definition.test_callable` (None → default ok response).
|
||||
|
||||
### 4.2 DEFINITION updates — wiring test_callable
|
||||
|
||||
Each integration's `DEFINITION` gains `test_callable=test_connection`:
|
||||
|
||||
```python
|
||||
# integrations/qbittorrent.py
|
||||
DEFINITION = ServiceDefinition(
|
||||
...,
|
||||
test_callable=test_connection,
|
||||
)
|
||||
|
||||
# integrations/backups.py
|
||||
DEFINITION = ServiceDefinition(
|
||||
..., # test_callable stays None (default)
|
||||
)
|
||||
```
|
||||
|
||||
Every integration module that has a `test_connection` function imports `TestResult` + `translate_connection_error` from `base.py` and `requests` as needed.
|
||||
|
||||
---
|
||||
|
||||
## 5. Frontend design
|
||||
|
||||
### 5.1 TestResult type (CT-114)
|
||||
|
||||
**File:** `frontend/src/types/index.ts`
|
||||
|
||||
```typescript
|
||||
export interface ServiceTestResult {
|
||||
ok: boolean;
|
||||
detail: string;
|
||||
evidence: string | null;
|
||||
}
|
||||
```
|
||||
|
||||
### 5.2 API client function (CT-114)
|
||||
|
||||
**File:** `frontend/src/api/services.ts`
|
||||
|
||||
```typescript
|
||||
import type { ServiceTestResult } from "../types";
|
||||
|
||||
export async function testServiceInstance(
|
||||
input: ServiceInstanceInput,
|
||||
): Promise<ServiceTestResult> {
|
||||
return post<ServiceTestResult>("/api/services/test", input);
|
||||
}
|
||||
```
|
||||
|
||||
### 5.3 useTestServiceInstance hook (CT-114)
|
||||
|
||||
**File:** `frontend/src/hooks/useServices.ts`
|
||||
|
||||
```typescript
|
||||
import { testServiceInstance } from "../api/services";
|
||||
|
||||
export function useTestServiceInstance() {
|
||||
return useMutation({
|
||||
mutationFn: (input: ServiceInstanceInput) => testServiceInstance(input),
|
||||
});
|
||||
}
|
||||
```
|
||||
|
||||
No cache invalidation needed — the test is a one-shot mutation with no query to refresh.
|
||||
|
||||
### 5.4 Shared ServiceTestPanel component
|
||||
|
||||
**File:** `frontend/src/components/ServiceTestPanel.tsx` (NEW — shared by both surfaces)
|
||||
|
||||
Both `CreateServiceDialog` and `ServiceConfigEditor` need the same UI: a Test button, a result pill, testPassed state, and Save-anyway toggle. Extracting it avoids duplication.
|
||||
|
||||
```tsx
|
||||
interface Props {
|
||||
input: ServiceInstanceInput | null; // null = no draft yet
|
||||
onTestPassed: (passed: boolean) => void;
|
||||
}
|
||||
|
||||
export function ServiceTestPanel({ input, onTestPassed }: Props) {
|
||||
const testService = useTestServiceInstance();
|
||||
const [result, setResult] = useState<ServiceTestResult | null>(null);
|
||||
const [saveAnyway, setSaveAnyway] = useState(false);
|
||||
|
||||
// Clear result when input changes (CT-118)
|
||||
useEffect(() => {
|
||||
setResult(null);
|
||||
onTestPassed(false);
|
||||
}, [input]); // input is a new object on every field edit → re-test required
|
||||
|
||||
async function handleTest() {
|
||||
if (!input) return;
|
||||
setResult(null);
|
||||
try {
|
||||
const res = await testService.mutateAsync(input);
|
||||
setResult(res);
|
||||
onTestPassed(res.ok);
|
||||
} catch (err) {
|
||||
setResult({ ok: false, detail: err instanceof Error ? err.message : String(err), evidence: null });
|
||||
onTestPassed(false);
|
||||
}
|
||||
}
|
||||
|
||||
const testPassed = result?.ok === true;
|
||||
const canSave = testPassed || saveAnyway;
|
||||
|
||||
return (
|
||||
<div className="flex flex-col gap-2">
|
||||
<Button variant="outline" size="sm" onClick={handleTest} disabled={testService.isPending || !input}>
|
||||
{testService.isPending ? "Testing…" : "Test credentials"}
|
||||
</Button>
|
||||
{result ? (
|
||||
<Alert variant={result.ok ? "default" : "destructive"}>
|
||||
<AlertDescription>
|
||||
{result.ok ? `✓ Connected${result.evidence ? ` — ${result.evidence}` : ""}` : `✗ ${result.detail}`}
|
||||
</AlertDescription>
|
||||
</Alert>
|
||||
) : null}
|
||||
<label className="flex items-center gap-2 text-xs text-muted-foreground">
|
||||
<input type="checkbox" checked={saveAnyway} onChange={(e) => { setSaveAnyway(e.target.checked); onTestPassed(e.target.checked || testPassed); }} />
|
||||
Save anyway (skip test)
|
||||
</label>
|
||||
{/* Hidden signal: parent reads canSave via onTestPassed callback */}
|
||||
</div>
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
**CT-118 (field-edit clears result):** the `useEffect([input])` triggers on every render where `input` is a new object reference (React state updates create new objects). Since `setDraft({...draft, config})` creates a new object, any field edit resets the result.
|
||||
|
||||
**CT-117 (Save gating):** the parent passes `onTestPassed` to track the gating state. The parent's confirm button is disabled unless `testPassed || saveAnyway`.
|
||||
|
||||
### 5.5 Integration into CreateServiceDialog (CT-115, CT-117)
|
||||
|
||||
**File:** `frontend/src/pages/ServicesPage.tsx`
|
||||
|
||||
```tsx
|
||||
function CreateServiceDialog({ open, onClose }) {
|
||||
// ... existing state ...
|
||||
const [testPassed, setTestPassed] = useState(false);
|
||||
|
||||
function reset() {
|
||||
setDraft(null);
|
||||
setSubmitError(null);
|
||||
setTestPassed(false);
|
||||
}
|
||||
|
||||
// Build the input object from the draft (for the test panel)
|
||||
const testInput: ServiceInstanceInput | null = draft ? {
|
||||
service_type: draft.serviceType,
|
||||
name: draft.name.trim(),
|
||||
config: draft.config,
|
||||
secrets: draft.secrets,
|
||||
enabled: draft.enabled,
|
||||
} : null;
|
||||
|
||||
return (
|
||||
<Dialog ...>
|
||||
<DialogContent>
|
||||
{/* ... existing fields ... */}
|
||||
{draft ? (
|
||||
<ServiceTestPanel input={testInput} onTestPassed={setTestPassed} />
|
||||
) : null}
|
||||
{submitError ? <Alert variant="destructive">...</Alert> : null}
|
||||
{draft ? (
|
||||
<DialogFooter
|
||||
onCancel={reset}
|
||||
onConfirm={save}
|
||||
confirmLabel="Create service"
|
||||
confirmDisabled={!draft.name.trim() || saveService.isPending || !testPassed}
|
||||
/>
|
||||
) : null}
|
||||
</DialogContent>
|
||||
</Dialog>
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
### 5.6 Integration into ServiceConfigEditor (CT-115, CT-117)
|
||||
|
||||
**File:** `frontend/src/pages/Settings.tsx`
|
||||
|
||||
```tsx
|
||||
function ServiceConfigEditor({ instance, typeInfo }) {
|
||||
// ... existing state ...
|
||||
const [testPassed, setTestPassed] = useState(false);
|
||||
|
||||
const testInput: ServiceInstanceInput = {
|
||||
id: instance.id,
|
||||
service_type: instance.service_type,
|
||||
name,
|
||||
config: draftConfig,
|
||||
secrets: Object.fromEntries(Object.entries(draftSecrets).filter(([, v]) => v !== "")),
|
||||
enabled,
|
||||
};
|
||||
|
||||
return (
|
||||
<>
|
||||
<div className="rounded-lg border p-4">
|
||||
{/* ... existing fields ... */}
|
||||
<ServiceTestPanel input={testInput} onTestPassed={setTestPassed} />
|
||||
<div className="flex justify-between">
|
||||
<Button onClick={handleSave} disabled={saveService.isPending || !testPassed}>Save</Button>
|
||||
{/* ... delete button ... */}
|
||||
</div>
|
||||
</div>
|
||||
</>
|
||||
);
|
||||
}
|
||||
```
|
||||
|
||||
---
|
||||
|
||||
## 6. Slice plan
|
||||
|
||||
### Slice 1: Backend (~300–380 lines)
|
||||
|
||||
**Goal:** endpoint + test_callable + shared helper + 7 per-type routines + tests.
|
||||
|
||||
| Task | File(s) | Lines |
|
||||
|---|---|---|
|
||||
| `TestResult` dataclass + `TestCallable` type + `translate_connection_error` helper | `integrations/base.py` | ~60 |
|
||||
| `test_callable` field on `ServiceDefinition` | `integrations/base.py` | ~3 |
|
||||
| 7 per-type `test_connection` routines + wire into DEFINITIONs | `integrations/{qbittorrent,prometheus,alertmanager,jellyfin,authentik,ssh_tasks,nextcloud}.py` | ~180 |
|
||||
| `POST /api/services/test` endpoint | `routers/services.py` | ~30 |
|
||||
| Backend tests: per-type mocked routines, endpoint dispatch, validation-first, no-persistence | `tests/test_services.py` + `tests/test_api.py` | ~120 |
|
||||
|
||||
**Exit gate:** `pytest` + `ruff` green.
|
||||
|
||||
### Slice 2: Frontend (~250–320 lines)
|
||||
|
||||
**Goal:** API client + hook + shared Test panel + wire into both surfaces + tests.
|
||||
|
||||
| Task | File(s) | Lines |
|
||||
|---|---|---|
|
||||
| `ServiceTestResult` type | `types/index.ts` | ~5 |
|
||||
| `testServiceInstance` API function | `api/services.ts` | ~5 |
|
||||
| `useTestServiceInstance` hook | `hooks/useServices.ts` | ~8 |
|
||||
| `ServiceTestPanel` component | `components/ServiceTestPanel.tsx` | ~70 |
|
||||
| Wire into `CreateServiceDialog` | `pages/ServicesPage.tsx` | ~20 |
|
||||
| Wire into `ServiceConfigEditor` | `pages/Settings.tsx` | ~15 |
|
||||
| Frontend tests: panel renders both states, button fires mutation, gating, field-clear | `components/__tests__/ServiceTestPanel.test.tsx` + existing page tests | ~130 |
|
||||
|
||||
**Exit gate:** `npm run build` + `npm run lint` + `npm run test` green.
|
||||
|
||||
---
|
||||
|
||||
## 7. Test strategy
|
||||
|
||||
### 7.1 Backend tests (CT-119)
|
||||
|
||||
- **Per-type test_connection** (mocked): each routine gets a success case (mocked client returns data → `{ok: true, evidence: ...}`) and at least one failure case (mocked client raises → `{ok: false, detail: ...}`). qBit gets a specific `"Fails."` auth-failure test.
|
||||
- **Endpoint dispatch**: `POST /api/services/test` with each service type dispatches correctly; backups returns the default ok-no-test.
|
||||
- **Validation-first**: schema-less URL → 422, no network call mocked.
|
||||
- **No-persistence**: call `/test`, assert store count unchanged.
|
||||
|
||||
### 7.2 Frontend tests (CT-120)
|
||||
|
||||
- **ServiceTestPanel**: renders Test button; click fires mocked mutation; success → green pill with evidence; failure → red pill with detail; Save-anyway checkbox toggles gating.
|
||||
- **CreateServiceDialog**: confirm disabled until test passes; Save anyway re-enables; editing a field clears the result.
|
||||
- **ServiceConfigEditor**: same gating behavior.
|
||||
|
||||
---
|
||||
|
||||
## 8. Security considerations
|
||||
|
||||
- **No persistence** (CT-112): the endpoint never calls any `store.*` method except read-only `get_ssh_key` (for SSH key resolution in ssh_tasks). No `upsert_service`, no `update_setting`.
|
||||
- **No secret logging** (CT-113): only `test requested type=%s ok=%s` at INFO. No request body in logs. The `sanitize_log_extra` helper is available if structured logging is added later.
|
||||
- **Auth gating**: identical to every other `/api/services/*` endpoint (JWT/API-key via the app-level middleware).
|
||||
- **Secrets in transit**: plaintext in the request body over TLS — identical to the existing `POST /api/services/instances` create endpoint. No new attack surface.
|
||||
- **Timeout**: each routine uses a short timeout (≤10s) from the service config. No long-running probes.
|
||||
|
||||
---
|
||||
|
||||
## 9. Key design decisions summary
|
||||
|
||||
1. **`TestResult` is a frozen dataclass** in `integrations/base.py` — immutable, serializable, co-located with the `ServiceDefinition` it augments.
|
||||
2. **`test_callable` signature includes `SettingsStore`** — needed for ssh_tasks SSH-key resolution; other types ignore it. Uniform signature avoids per-type dispatch special-cases.
|
||||
3. **`translate_connection_error` is shared** — extracts the `test_machine_ssh` message-translation pattern into a reusable helper covering HTTP errors, connection errors, SSL, timeout, SSH banner. Type-specific auth failures (qBit `"Fails."`) are handled in-routine.
|
||||
4. **`POST /api/services/test` validation-first** — reuses `_validate_input` so malformed configs get the same 422 as create. The #1 validation-surfacing fix covers this endpoint too.
|
||||
5. **Shared `ServiceTestPanel` component** — extracted to avoid duplicating the Test button + pill + gating between `CreateServiceDialog` and `ServiceConfigEditor`.
|
||||
6. **Field-edit-clears-result via `useEffect([input])`** — React state updates create new object references on every edit, naturally clearing the result without explicit field-tracking.
|
||||
7. **Prometheus test queries via Grafana gateway** — mirrors `MetricSource._gateway_query` exactly (`POST /api/ds/query` with `expr: "up"`). No direct Prom.
|
||||
8. **ssh_tasks test reuses `build_ssh_client`** — the same `task_runner.build_ssh_client(store, ServiceRecord)` that powers task execution, wrapped with the `test_machine_ssh` error-translation patterns.
|
||||
@@ -0,0 +1,125 @@
|
||||
# SDD Proposal: Service Credential Tester
|
||||
|
||||
**Change:** `service-credential-tester`
|
||||
**Phase:** proposal
|
||||
**Date:** 2026-07-09
|
||||
|
||||
## 1. Problem / Why Now
|
||||
|
||||
Adding a service in Manage today is **blind**: the user fills in a URL + credentials, clicks "Create service," and the row is persisted regardless of whether the backend can actually reach or authenticate against the external service. Failures only surface later, as error states inside individual widgets (or, for qBittorrent, as a `RuntimeError: qBittorrent login failed: ...` in the backend logs — invisible to the operator staring at the add form). The operator's first signal that something is wrong is a broken dashboard widget, possibly hours later.
|
||||
|
||||
Two concrete pain points triggered this:
|
||||
|
||||
1. **qBittorrent login failures** are being diagnosed from backend logs rather than at entry time. The operator typed credentials, the form accepted them, the service saved, and only `RuntimeError(f"qBittorrent login failed: {resp.text.strip()}")` in logs reveals the auth failure.
|
||||
2. **URL/schema mistakes** (caught by validation in #1's fix, committed separately) are a related but distinct concern: validation rejects malformed input at submission; the tester checks *live reachability + auth* before submission. Both layers are wanted.
|
||||
|
||||
There is already a proven pattern in the codebase: `test_machine_ssh` in `routers/settings.py` takes unsaved SSH machine input, attempts `.connect()`, translates specific error strings into helpful messages (banner failure, auth failure, etc.), and returns a structured result. This change generalizes that pattern to every service type, surfaced in the add/edit dialog as a "Test credentials" button that gates the save.
|
||||
|
||||
## 2. Target Users and Situations
|
||||
|
||||
- **Primary users:** Operators onboarding a new external service (Jellyfin, qBittorrent, Prometheus, Alertmanager, Authentik, ssh_tasks, backups) who want to confirm they got the URL/credentials right before committing.
|
||||
- **Operators debugging** a service that stopped working: edit the service, re-test, see the specific failure (DNS, refused, auth, SSL, CSRF), fix, re-test, save.
|
||||
- **Urgency:** Medium-high. This removes a real daily friction (silently-saved-broken-service) and is the prerequisite for trusting the service registry as "configured = working."
|
||||
|
||||
## 3. Product Outcome
|
||||
|
||||
After this change, the add-service and edit-service dialogs have a **"Test credentials"** button that, for the current (unsaved) form values:
|
||||
|
||||
- Attempts a minimal, read-only connection against the target service.
|
||||
- Returns either **✓ connected** (with a small piece of proof — e.g. qBittorrent version, Prometheus build info, Jellyfin server name) **or ✗ failed** with a specific, actionable message (auth refused, host unreachable, SSL error, timeout, CSRF/Referer rejected).
|
||||
- The **Create / Save button is disabled until the test passes** (configurable per-operator preference; default ON; a "save anyway" escape hatch is available for services that are temporarily down but being pre-configured).
|
||||
|
||||
The same tester is reachable from the service page as a "Re-test" action for already-saved services (useful when a service goes down and comes back).
|
||||
|
||||
## 4. Scope Boundaries and Non-Goals
|
||||
|
||||
### In scope
|
||||
|
||||
- **Backend:** a new endpoint `POST /api/services/test` that accepts `{service_type, config, secrets}` (NOT a service id — the tester works on unsaved input), dispatches to a per-type test routine, and returns `{ok: bool, detail: str, evidence?: str}`. The dispatch registry is closed and compile-time (mirrors `SERVICE_DEFINITIONS`).
|
||||
- **Per-type test routines** (minimal, read-only):
|
||||
- **qbittorrent:** login via `/api/v2/auth/login`, then `GET /api/v2/sync/maindata` (or `/app/version`). Evidence: qBittorrent version.
|
||||
- **prometheus:** `POST {grafana_url}/api/ds/query` with the configured `grafana_api_key` + `datasource_uid`, body querying `expr: "up"`. Evidence: gateway reachable + datasource responding (the `prometheus` service now sources through Grafana per the `grafana-metric-gateway` change; there is no direct Prom endpoint to test).
|
||||
- **alertmanager:** `GET /api/v2/alerts` (or `/api/v2/status`). Evidence: cluster version.
|
||||
- **jellyfin:** `GET /Users` (already exposed by `JellyfinClient.users()`). Evidence: user count + server name.
|
||||
- **jellyseerr:** (if still present as a distinct type) `GET /user`. Evidence: count.
|
||||
- **authentik:** a lightweight authenticated GET against its directory endpoint (already exists). Evidence: instance slug.
|
||||
- **backups:** no remote connection (data is internal) — test always succeeds (or is omitted from the tester UI for this type).
|
||||
- **ssh_tasks:** reuse the existing `test_machine_ssh` flow at the service level (connect + banner check). Evidence: connected host.
|
||||
- **nextcloud:** `GET /status.php` (unauthenticated server probe). Evidence: nextcloud version.
|
||||
- **Frontend:** "Test credentials" button in `CreateServiceDialog` and the edit dialog on `ServicePage`; a `useTestService` mutation hook; test-result state; a small inline status pill (✓/✗ + detail); "Create/Save" button gating on a successful test (with an override toggle).
|
||||
- **Error translation:** map common failure modes to human-friendly messages — connection refused, DNS failure, SSL/cert error, HTTP 401/403 (auth), timeout, qBit "Fails." response, etc.
|
||||
- **Reuse for qBittorrent login diagnosis** (#3): the tester surfaces the exact `resp.text` qBit returns, making the "login failed" logs issue debuggable from the UI.
|
||||
|
||||
### Non-goals (explicitly out of scope)
|
||||
|
||||
- **Write-path testing.** Tests are read-only (no add-pause-delete-torrent probes, no create-user probes).
|
||||
- **Background health-checking / monitoring.** The tester is on-demand only; continuous polling stays with the existing widget data-fetch path.
|
||||
- **Replacing the validation layer.** Schema validation (the #1 fix) and the tester are complementary: validation rejects malformed config before it hits the network; the tester checks live connectivity. Both ship.
|
||||
- **Per-widget test.** The test is per-service-instance, not per-widget-kind.
|
||||
- **Async/long-running tests.** Each test has a short timeout (≤10s) and runs synchronously from the user's perspective.
|
||||
- **Secrets persistence during test.** The test endpoint accepts plaintext secrets in the request body (over TLS, like every other secret-bearing endpoint), tests, and discards them — it does NOT persist anything.
|
||||
|
||||
## 5. High-Level Approach
|
||||
|
||||
### 5.1 Backend
|
||||
|
||||
1. **`integrations/base.py`** — add an optional `test_callable` field to `ServiceDefinition` (a callable `(config, secrets) -> TestResult` or `None` for types without a remote). `TestResult` is a small dataclass `{ok: bool, detail: str, evidence: str | None}`. Types without a test (e.g. `backups`) leave it `None`.
|
||||
2. **Per-type test routines** live alongside each integration (e.g. `integrations/qbittorrent.py` gains `test_connection(config, secrets) -> TestResult`). They construct the appropriate client with a short timeout, attempt the minimal probe, catch exceptions, and translate to friendly messages.
|
||||
3. **`routers/services.py`** — add `POST /api/services/test` taking `ServiceInstanceInput` (reuses the existing model: service_type + config + secrets). It resolves the definition, runs `test_callable` (or returns `{ok: True, detail: "No connection test for this service type"}` if none), and returns the result. Validation runs first (reuse `_validate_input`) so malformed configs fail fast with the same 422 as create.
|
||||
4. **Error translation helper** — a small shared utility mapping common exception types/messages to human strings, modeled on the `test_machine_ssh` message translation.
|
||||
|
||||
### 5.2 Frontend
|
||||
|
||||
1. **`api/services.ts`** — add `testServiceInstance(input) -> Promise<TestResult>`.
|
||||
2. **`hooks/useServices.ts`** — add `useTestServiceInstance()` mutation hook.
|
||||
3. **`CreateServiceDialog`** (in `ServicesPage.tsx`) + the edit dialog on `ServicePage.tsx` — add a "Test credentials" button below the form fields; on click, fire the mutation with the current draft; render a result pill (`✓ Connected — <evidence>` green / `✗ <detail>` red) inline; track `testPassed` state; gate the Create/Save confirm button on `testPassed` (with a "Save anyway" checkbox override for temporarily-down services).
|
||||
4. The test result clears whenever the user edits any field that affects connectivity (URL, secrets) — re-test required after edits.
|
||||
|
||||
### 5.3 Security note
|
||||
|
||||
The test endpoint accepts plaintext secrets in the request body, exactly as the existing `POST /api/services/instances` create endpoint does (secrets are encrypted at rest only after persistence). The test endpoint does NOT persist; it decrypts nothing (the body is already plaintext); it discards the secrets after the probe. Auth gating is the same as every other service endpoint (JWT/API-key). No new attack surface.
|
||||
|
||||
## 6. Success Criteria / Acceptance Criteria
|
||||
|
||||
1. Every service type with a remote connection (qbittorrent, prometheus, alertmanager, jellyfin, authentik, ssh_tasks, nextcloud) has a working `test_connection` routine; types without remote data (backups) report `{ok: True, detail: "No test needed"}`.
|
||||
2. The add-service dialog has a "Test credentials" button that returns a clear ✓/✗ result with evidence or a specific failure message.
|
||||
3. The Create button is disabled until either the test passes OR the operator checks "Save anyway."
|
||||
4. Editing a connectivity field (URL, secrets) clears the previous test result, requiring a re-test.
|
||||
5. qBittorrent auth failure is surfaced as a readable message (e.g. "Authentication failed — qBittorrent returned 'Fails.'"), not just a generic error — resolving the #3 "login failed in logs" pain at the UI layer.
|
||||
6. The same tester is reachable from the service edit dialog for already-saved services.
|
||||
7. Backend tests cover: each type's test routine (mocked client) returns the expected shape; the endpoint dispatches correctly; validation runs before the test.
|
||||
8. Frontend tests cover: the Test button fires the mutation, the result pill renders both states, the Create button gating works, field-edit clears the result.
|
||||
9. `pytest`, `npm run build`, `npm run lint`, `npm run test` all stay green.
|
||||
10. No secrets are persisted by the test endpoint (verified by a test that calls `/test` and asserts no new service row exists).
|
||||
|
||||
## 7. Risks and Mitigations
|
||||
|
||||
| Risk | Mitigation |
|
||||
|------|------------|
|
||||
| **Test endpoint leaks secrets in logs.** | The endpoint logs at INFO only `test requested type=%s ok=%s`; never logs the request body or secrets. Sanitize via the existing `sanitize_log_extra`. |
|
||||
| **Slow tests block the UI.** Each test hits a remote. | Short timeout per type (≤10s, most ≤5s); the UI shows a "Testing…" state; the button re-enables on completion. |
|
||||
| **Per-type test routines duplicate client logic.** | Reuse the existing clients (`JellyfinClient`, `QbittorrentClient`, etc.) — each test routine is a thin wrapper that constructs the client + calls one method. |
|
||||
| **False negatives from transient network blips.** | The "Save anyway" escape hatch ensures an operator pre-configuring a not-yet-online service isn't blocked. |
|
||||
| **Error message quality varies per type.** | Shared error-translation helper covers the common cases (refused, DNS, SSL, 401/403, timeout); type-specific messages (e.g. qBit "Fails.") handled in their routine. |
|
||||
| **Review budget (>400 lines).** | Likely 2 slices: (S1) backend endpoint + per-type routines + tests; (S2) frontend Test button + gating + tests. |
|
||||
| **#1 (validation surfacing) overlaps.** | No conflict — #1 ships first as a quick fix; the tester adds the live-connection layer on top. Both coexist. |
|
||||
|
||||
## 8. Resolved Questions (no question round needed — settled during investigation)
|
||||
|
||||
- **Q1 — Endpoint shape?** `POST /api/services/test` taking `ServiceInstanceInput`, returning `{ok, detail, evidence?}`.
|
||||
- **Q2 — Gate Create on test pass?** Yes, with a "Save anyway" override (default-on gating, but escapable for offline-pre-config).
|
||||
- **Q3 — Test on edit, or only on create?** Both — same dialog component, same flow.
|
||||
- **Q4 — Backups type?** No remote test; returns ok-with-detail. Not shown in the UI for that type.
|
||||
|
||||
## 9. Future Phases
|
||||
|
||||
1. **Continuous health probes** — promote the tester into a periodic background check that flags services going down (separate from this on-demand feature).
|
||||
2. **Per-widget-kind test** — for widgets that take their own config (e.g. a PromQL query), test that the specific query returns data.
|
||||
3. **Test history** — record the last test result + timestamp on the service row, visible in the service page.
|
||||
|
||||
---
|
||||
|
||||
## Notes for downstream phases
|
||||
|
||||
- **#1 (validation surfacing) is a prerequisite quick-fix**, already committed in `493c0e1`. It ensures the tester's own validation failures (malformed config) are visible in the dialog.
|
||||
- **#3 (qBittorrent login logs)** is materially resolved by this change's acceptance criterion #5 — the operator will see the auth failure in the UI at add time, not in backend logs.
|
||||
@@ -0,0 +1,222 @@
|
||||
# SDD Spec: Service Credential Tester
|
||||
|
||||
**Change:** `service-credential-tester`
|
||||
**Phase:** spec
|
||||
**Date:** 2026-07-09
|
||||
|
||||
This spec defines the acceptance requirements for the change. Requirements are testable and derived from `proposal.md` §6 (success criteria) and §8 (resolved questions). Prefix `CT-` for credential-tester.
|
||||
|
||||
## Requirement categories
|
||||
|
||||
1. Endpoint shape, validation, dispatch
|
||||
2. Per-type test routines
|
||||
3. No-persistence + security
|
||||
4. Frontend test UI
|
||||
5. Tests + gate greenness
|
||||
|
||||
---
|
||||
|
||||
## 1. Endpoint shape, validation, dispatch
|
||||
|
||||
### Requirement: CT-101 — Test endpoint accepts unsaved service input and returns a structured result
|
||||
|
||||
The backend MUST expose `POST /api/services/test` accepting a `ServiceInstanceInput` body (`service_type`, `name`, `config`, `secrets`, `enabled`) — NOT a service id, because the tester operates on unsaved form values. On success the endpoint MUST return HTTP 200 with a body of shape `{ "ok": bool, "detail": str, "evidence": str | null }`. The endpoint MUST be auth-gated identically to every other `/api/services/*` endpoint (JWT/API-key).
|
||||
|
||||
#### Scenario: successful connection returns ok with evidence
|
||||
|
||||
- GIVEN a valid `ServiceInstanceInput` for a reachable service
|
||||
- WHEN `POST /api/services/test` is called with that body
|
||||
- THEN the response is HTTP 200 with `{ "ok": true, "detail": <human summary>, "evidence": <proof string> }`
|
||||
|
||||
#### Scenario: failed connection returns ok=false with actionable detail
|
||||
|
||||
- GIVEN a valid input whose target is unreachable or rejects auth
|
||||
- WHEN the endpoint is called
|
||||
- THEN the response is HTTP 200 with `{ "ok": false, "detail": <specific failure message>, "evidence": null }`
|
||||
|
||||
### Requirement: CT-102 — Validation runs before the test and rejects malformed config with 422
|
||||
|
||||
The endpoint MUST reuse the existing `_validate_input` helper (from `routers/services.py`) to validate `service_type`, `config` (via the definition's `config_model`), and secret-key names BEFORE any network probe. Malformed input MUST raise HTTP 422 with the same detail format as `POST /api/services/instances`, so the existing validation-error surfacing (the `#1` fix in `ServicesPage.tsx`) covers the test endpoint too.
|
||||
|
||||
#### Scenario: schema-less URL rejected before any network call
|
||||
|
||||
- GIVEN a `qbittorrent` input whose `base_url` lacks the `http://` schema
|
||||
- WHEN the endpoint is called
|
||||
- THEN the response is HTTP 422 with a detail naming the schema requirement
|
||||
- AND no network call is made to qBittorrent
|
||||
|
||||
#### Scenario: unknown service type rejected
|
||||
|
||||
- GIVEN an input with `service_type = "nonexistent"`
|
||||
- WHEN the endpoint is called
|
||||
- THEN the response is HTTP 422 with "Unknown service type"
|
||||
|
||||
### Requirement: CT-103 — Closed per-type dispatch mirrors SERVICE_DEFINITIONS
|
||||
|
||||
The endpoint MUST dispatch to a per-type test routine via the service definition (a new optional `test_callable` field on `ServiceDefinition`, defaulting to `None`). The dispatch is closed and compile-time — no runtime plugin loading, no arbitrary callable execution. A service type whose definition has `test_callable = None` (e.g. `backups`, which has no remote connection) MUST return `{ "ok": true, "detail": "No connection test for this service type", "evidence": null }` without any network call.
|
||||
|
||||
#### Scenario: backups type returns ok without a network probe
|
||||
|
||||
- GIVEN a `backups` service input
|
||||
- WHEN the endpoint is called
|
||||
- THEN the response is `{ "ok": true, "detail": "No connection test for this service type", "evidence": null }`
|
||||
- AND no network request is issued
|
||||
|
||||
#### Scenario: unknown widget kind is not involved
|
||||
|
||||
- GIVEN a `prometheus` input
|
||||
- WHEN the endpoint is called
|
||||
- THEN the dispatch keys off `service_type` only (not `widget_kind`); the test is per-service-instance, not per-widget
|
||||
|
||||
---
|
||||
|
||||
## 2. Per-type test routines
|
||||
|
||||
Each routine lives alongside its integration module (e.g. `integrations/qbittorrent.py` gains `test_connection(config, secrets) -> TestResult`), constructs the appropriate client with a short timeout (≤10s), attempts a minimal read-only probe, catches exceptions, and translates them to human-friendly messages via a shared error-translation helper. The helper maps common failure modes — connection refused, DNS failure, SSL/cert error, HTTP 401/403 (auth), timeout — to actionable strings, modeled on the `test_machine_ssh` message-translation style.
|
||||
|
||||
### Requirement: CT-104 — qBittorrent test logs in and probes maindata; surfaces auth failures specifically
|
||||
|
||||
The `qbittorrent` test routine MUST construct a `QbittorrentClient` from the config + decrypted secrets, call login (`POST /api/v2/auth/login`), then probe `GET /api/v2/sync/maindata` (or `/app/version`). On success it MUST return `{ "ok": true, "evidence": <qBittorrent version or "connected"> }`. When qBittorrent's login returns anything other than `"Ok."` (e.g. the literal `"Fails."`), the routine MUST return `{ "ok": false, "detail": "Authentication failed — qBittorrent rejected the credentials." }` — surfacing the specific auth failure at the UI layer, not just in backend logs.
|
||||
|
||||
#### Scenario: correct credentials succeed with version evidence
|
||||
|
||||
- GIVEN a qBittorrent instance reachable at the configured URL with valid username/password
|
||||
- WHEN the test routine runs
|
||||
- THEN it returns `{ "ok": true, "detail": "Connected to qBittorrent.", "evidence": <version> }`
|
||||
|
||||
#### Scenario: wrong password surfaces a specific auth message
|
||||
|
||||
- GIVEN a qBittorrent instance that returns `"Fails."` on login
|
||||
- WHEN the test routine runs
|
||||
- THEN it returns `{ "ok": false, "detail": "Authentication failed — qBittorrent rejected the credentials.", "evidence": null }`
|
||||
|
||||
### Requirement: CT-105 — Prometheus test queries the Grafana gateway with expr "up"
|
||||
|
||||
The `prometheus` test routine MUST issue `POST {grafana_url}/api/ds/query` carrying `Authorization: Bearer {grafana_api_key}` and a `queries[0]` entry keyed by the configured `datasource_uid` with `expr: "up"`. There MUST be no direct Prometheus HTTP call (the `prometheus` service sources through Grafana per `grafana-metric-gateway`). On success it returns `{ "ok": true, "evidence": "Gateway reachable; datasource responded." }`. On HTTP 401/403 it returns an auth-specific message; on connection error it returns an unreachable message.
|
||||
|
||||
#### Scenario: gateway reachable returns ok
|
||||
|
||||
- GIVEN a prometheus service with valid Grafana gateway credentials
|
||||
- WHEN the test routine runs
|
||||
- THEN it returns `{ "ok": true, "detail": "Grafana gateway reachable.", "evidence": "Gateway reachable; datasource responded." }`
|
||||
|
||||
#### Scenario: wrong API key surfaces auth failure
|
||||
|
||||
- GIVEN a prometheus service whose `grafana_api_key` is invalid
|
||||
- WHEN the test routine runs
|
||||
- THEN it returns `{ "ok": false, "detail": <message mentioning authentication/authorization>, "evidence": null }`
|
||||
|
||||
### Requirement: CT-106 — Alertmanager test probes /api/v2/alerts or /api/v2/status
|
||||
|
||||
The `alertmanager` test routine MUST issue an authenticated GET (using the optional `api_key` secret when set) to `/api/v2/alerts` (or `/api/v2/status`). On success it returns `{ "ok": true, "evidence": <cluster version> }`. On failure it translates the error per the shared helper.
|
||||
|
||||
### Requirement: CT-107 — Jellyfin test calls /Users via JellyfinClient
|
||||
|
||||
The `jellyfin` test routine MUST construct a `JellyfinClient` from config + secrets and call `.users()`. On success it returns `{ "ok": true, "evidence": "<N> users" }`. On failure it translates the error.
|
||||
|
||||
### Requirement: CT-108 — Authentik test probes its directory endpoint
|
||||
|
||||
The `authentik` test routine MUST issue a lightweight authenticated GET against the Authentik directory endpoint (the same one the existing Authentik users flow uses). On success it returns `{ "ok": true, "evidence": <instance slug or "connected"> }`. On failure it translates the error.
|
||||
|
||||
### Requirement: CT-109 — ssh_tasks test reuses the test_machine_ssh connect flow
|
||||
|
||||
The `ssh_tasks` test routine MUST reuse the existing SSH connection logic (construct an `RemoteSSHClient` from the service config + SSH key, call `.connect()`, translate banner/auth errors per the established `test_machine_ssh` patterns). On success it returns `{ "ok": true, "evidence": "Connected to <host>:<port>" }`. The known-host recording behavior (first successful connect records the host key) is preserved.
|
||||
|
||||
### Requirement: CT-110 — Nextcloud test probes /status.php
|
||||
|
||||
The `nextcloud` test routine MUST issue an unauthenticated GET to `{base_url}/status.php`. On success it returns `{ "ok": true, "evidence": <Nextcloud version> }`. On failure it translates the error.
|
||||
|
||||
### Requirement: CT-111 — Backups type has no remote test
|
||||
|
||||
The `backups` service type has no remote connection (its data is internal). Its definition MUST set `test_callable = None`, and the endpoint returns `{ "ok": true, "detail": "No connection test for this service type" }` (CT-103).
|
||||
|
||||
---
|
||||
|
||||
## 3. No-persistence + security
|
||||
|
||||
### Requirement: CT-112 — The test endpoint MUST NOT persist any service row or secret
|
||||
|
||||
The test endpoint MUST NOT call `store.upsert_service`, `store.update_setting`, or any other persistence method. It accepts plaintext secrets in the request body (over TLS, identical to the existing create endpoint), probes the target, and discards the secrets. A test MUST verify this by calling `POST /api/services/test` and asserting the service count in the store is unchanged afterward.
|
||||
|
||||
#### Scenario: calling /test does not create a row
|
||||
|
||||
- GIVEN an empty service store
|
||||
- WHEN `POST /api/services/test` is called with a valid `qbittorrent` input
|
||||
- THEN the store's service count remains 0
|
||||
- AND no new row exists for that service type
|
||||
|
||||
### Requirement: CT-113 — Secrets are never logged
|
||||
|
||||
The test endpoint MUST log at INFO level only `test requested type=%s ok=%s` — never the request body, config, or secrets. The log call MUST pass through `sanitize_log_extra` (the existing helper) so any accidental inclusion of secret-named keys is scrubbed.
|
||||
|
||||
#### Scenario: logs contain no secret values
|
||||
|
||||
- GIVEN a test call carrying `secrets: { "password": "hunter2" }`
|
||||
- WHEN the endpoint runs
|
||||
- THEN no log line contains "hunter2"
|
||||
- AND the INFO log line reads `test requested type=qbittorrent ok=false` (or similar), with no secret values
|
||||
|
||||
---
|
||||
|
||||
## 4. Frontend test UI
|
||||
|
||||
### Requirement: CT-114 — useTestServiceInstance mutation hook + testServiceInstance API client function
|
||||
|
||||
`frontend/src/api/services.ts` MUST gain `testServiceInstance(input: ServiceInstanceInput): Promise<TestResult>` posting to `/api/services/test`. `frontend/src/hooks/useServices.ts` MUST gain `useTestServiceInstance()` returning a TanStack mutation whose `mutateAsync` resolves to `{ ok, detail, evidence }`.
|
||||
|
||||
### Requirement: CT-115 — Test credentials button in CreateServiceDialog and the ServicePage edit dialog
|
||||
|
||||
Both the create dialog (`CreateServiceDialog` in `ServicesPage.tsx`) and the edit dialog on `ServicePage.tsx` MUST render a "Test credentials" button below the config/secret form fields. Clicking it fires the `useTestServiceInstance` mutation with the current draft values. While pending, the button shows a "Testing…" state and is disabled.
|
||||
|
||||
### Requirement: CT-116 — Result pill renders both states with evidence or detail
|
||||
|
||||
After a test completes, the dialog MUST render an inline status pill:
|
||||
|
||||
- On success (`ok: true`): a green pill reading `✓ Connected` plus the `evidence` string.
|
||||
- On failure (`ok: false`): a red/destructive pill reading `✗ <detail>`.
|
||||
|
||||
The pill is rendered in the same dialog, below the Test button, above the footer.
|
||||
|
||||
### Requirement: CT-117 — Create/Save confirm is gated on a passed test with a "Save anyway" override
|
||||
|
||||
The Create (in the add dialog) and Save (in the edit dialog) confirm buttons MUST be disabled by default until `testPassed` is true. A "Save anyway" checkbox MUST appear that, when checked, re-enables the confirm button — allowing an operator to pre-configure a service that is not yet online. The default state of the checkbox is unchecked (gating is on).
|
||||
|
||||
### Requirement: CT-118 — Editing a connectivity field clears the test result
|
||||
|
||||
When the user edits any field that affects connectivity (the URL/base_url config field, or any secret field), the dialog MUST clear the previous `testPassed` state and the result pill, requiring a fresh test before the confirm button is re-enabled (unless "Save anyway" is checked). This prevents a stale green result from masking a typo correction.
|
||||
|
||||
---
|
||||
|
||||
## 5. Tests + gate greenness
|
||||
|
||||
### Requirement: CT-119 — Backend tests cover routines, dispatch, validation, and no-persistence
|
||||
|
||||
`pytest` from `backend/` MUST pass, including new tests for:
|
||||
|
||||
- Each type's `test_connection` routine (mocked client) returning the expected `{ok, detail, evidence}` shape for both success and at least one failure case.
|
||||
- The `POST /api/services/test` endpoint dispatching correctly per type.
|
||||
- Validation running before the test (422 on malformed config, no network call).
|
||||
- No-persistence: calling `/test` leaves the store unchanged.
|
||||
|
||||
### Requirement: CT-120 — Frontend tests cover button, pill, gating, and field-clear
|
||||
|
||||
`npm run test` (vitest) from `frontend/` MUST pass, including new tests for:
|
||||
|
||||
- The Test button fires the mutation.
|
||||
- The result pill renders both the green (ok) and red (failed) states.
|
||||
- The Create/Save confirm button is disabled until the test passes (and re-enabled by "Save anyway").
|
||||
- Editing a connectivity field clears `testPassed`.
|
||||
|
||||
### Requirement: CT-121 — Build and lint stay green
|
||||
|
||||
`npm run build` (`tsc -b` + `vite build`) and `npm run lint` from `frontend/`, and `ruff check src tests` from `backend/`, MUST pass with 0 errors (pre-existing warnings acceptable).
|
||||
|
||||
---
|
||||
|
||||
## Notes for downstream phases
|
||||
|
||||
- **Slice plan (≤400 lines each):** S1 = backend endpoint + `test_callable` field + per-type routines (7 types) + shared error-translation helper + backend tests; S2 = frontend API client + hook + Test button + result pill + gating + Save anyway + field-clear + frontend tests.
|
||||
- **The `#1` validation-surfacing fix (commit `493c0e1`) is a prerequisite** — it ensures the test endpoint's 422 validation errors render in the dialog. No action needed; already shipped.
|
||||
- **Stale-proposal correction:** the proposal mentions `jellyseerr` as a distinct service type. The active registry has no `jellyseerr` entry (merged into Jellyfin config by `services-as-hub-ia`). This spec covers the 8 active types: alertmanager, authentik, backups, jellyfin, nextcloud, prometheus, qbittorrent, ssh_tasks. Jellyseerr is dropped.
|
||||
- **TestResult dataclass:** `integrations/base.py` gains a small `TestResult` dataclass (`{ok: bool, detail: str, evidence: str | None}`) and `ServiceDefinition` gains an optional `test_callable: Callable[[dict, dict], TestResult] | None = None` field (placed last, after the existing fields, to satisfy dataclass field-ordering).
|
||||
- **Error translation:** the shared helper covers refused, DNS, SSL, 401/403, timeout; type-specific messages (e.g. qBit "Fails.") are handled inside each routine.
|
||||
+201
@@ -0,0 +1,201 @@
|
||||
# Service Credential Testing — Delta (`service-credential-tester`)
|
||||
|
||||
> Change: `service-credential-tester` · Domain: `service-credential-testing` · Phase: **spec** (reconciled during `sdd-sync`).
|
||||
> Distilled verbatim from the verified flat `spec.md` (21 requirements, CT-101 … CT-121) of
|
||||
> change `service-credential-tester`, cross-referenced against `design.md` and
|
||||
> `verify-report.md`. Captures the **durable, post-change end-state contracts** for the per-
|
||||
> service-type credential tester: the `POST /api/services/test` endpoint, the closed
|
||||
> `test_callable` dispatch registry, per-type test routines, the no-persistence + no-secret-logging
|
||||
> guarantees, and the frontend test UI (Test button + result pill + gated confirm with a "Save
|
||||
> anyway" override).
|
||||
|
||||
## ADDED Requirements
|
||||
|
||||
> The canonical `openspec/specs/service-credential-testing/spec.md` did not exist before this
|
||||
> change. All requirements below are therefore **ADDED** to a new `service-credential-testing`
|
||||
> domain; `sdd-sync` copies them into the canonical spec (native helper rule: when the canonical
|
||||
> spec does not exist, the change spec becomes the new canonical spec).
|
||||
>
|
||||
> Requirement IDs (CT-101 … CT-121) and body text are preserved **exactly** from the verified flat
|
||||
> `spec.md`. Requirements are grouped logically and listed in the following group order:
|
||||
>
|
||||
> + **Endpoint shape, validation, dispatch** — CT-101 … CT-103
|
||||
> + **Per-type test routines** — CT-104 … CT-111
|
||||
> + **No-persistence + security** — CT-112 … CT-113
|
||||
> + **Frontend test UI** — CT-114 … CT-118
|
||||
> + **Tests + gate greenness** — CT-119 … CT-121
|
||||
|
||||
### Requirement: CT-101 — Test endpoint accepts unsaved service input and returns a structured result
|
||||
|
||||
The backend MUST expose `POST /api/services/test` accepting a `ServiceInstanceInput` body (`service_type`, `name`, `config`, `secrets`, `enabled`) — NOT a service id, because the tester operates on unsaved form values. On success the endpoint MUST return HTTP 200 with a body of shape `{ "ok": bool, "detail": str, "evidence": str | null }`. The endpoint MUST be auth-gated identically to every other `/api/services/*` endpoint (JWT/API-key).
|
||||
|
||||
#### Scenario: successful connection returns ok with evidence
|
||||
|
||||
+ GIVEN a valid `ServiceInstanceInput` for a reachable service
|
||||
+ WHEN `POST /api/services/test` is called with that body
|
||||
+ THEN the response is HTTP 200 with `{ "ok": true, "detail": <human summary>, "evidence": <proof string> }`
|
||||
|
||||
#### Scenario: failed connection returns ok=false with actionable detail
|
||||
|
||||
+ GIVEN a valid input whose target is unreachable or rejects auth
|
||||
+ WHEN the endpoint is called
|
||||
+ THEN the response is HTTP 200 with `{ "ok": false, "detail": <specific failure message>, "evidence": null }`
|
||||
|
||||
### Requirement: CT-102 — Validation runs before the test and rejects malformed config with 422
|
||||
|
||||
The endpoint MUST reuse the existing `_validate_input` helper (from `routers/services.py`) to validate `service_type`, `config` (via the definition's `config_model`), and secret-key names BEFORE any network probe. Malformed input MUST raise HTTP 422 with the same detail format as `POST /api/services/instances`, so the existing validation-error surfacing (the `#1` fix in `ServicesPage.tsx`) covers the test endpoint too.
|
||||
|
||||
#### Scenario: schema-less URL rejected before any network call
|
||||
|
||||
+ GIVEN a `qbittorrent` input whose `base_url` lacks the `http://` schema
|
||||
+ WHEN the endpoint is called
|
||||
+ THEN the response is HTTP 422 with a detail naming the schema requirement
|
||||
+ AND no network call is made to qBittorrent
|
||||
|
||||
#### Scenario: unknown service type rejected
|
||||
|
||||
+ GIVEN an input with `service_type = "nonexistent"`
|
||||
+ WHEN the endpoint is called
|
||||
+ THEN the response is HTTP 422 with "Unknown service type"
|
||||
|
||||
### Requirement: CT-103 — Closed per-type dispatch mirrors SERVICE_DEFINITIONS
|
||||
|
||||
The endpoint MUST dispatch to a per-type test routine via the service definition (a new optional `test_callable` field on `ServiceDefinition`, defaulting to `None`). The dispatch is closed and compile-time — no runtime plugin loading, no arbitrary callable execution. A service type whose definition has `test_callable = None` (e.g. `backups`, which has no remote connection) MUST return `{ "ok": true, "detail": "No connection test for this service type", "evidence": null }` without any network call.
|
||||
|
||||
#### Scenario: backups type returns ok without a network probe
|
||||
|
||||
+ GIVEN a `backups` service input
|
||||
+ WHEN the endpoint is called
|
||||
+ THEN the response is `{ "ok": true, "detail": "No connection test for this service type", "evidence": null }`
|
||||
+ AND no network request is issued
|
||||
|
||||
#### Scenario: unknown widget kind is not involved
|
||||
|
||||
+ GIVEN a `prometheus` input
|
||||
+ WHEN the endpoint is called
|
||||
+ THEN the dispatch keys off `service_type` only (not `widget_kind`); the test is per-service-instance, not per-widget
|
||||
|
||||
### Requirement: CT-104 — qBittorrent test logs in and probes maindata; surfaces auth failures specifically
|
||||
|
||||
The `qbittorrent` test routine MUST construct a `QbittorrentClient` from the config + decrypted secrets, call login (`POST /api/v2/auth/login`), then probe `GET /api/v2/sync/maindata` (or `/app/version`). On success it MUST return `{ "ok": true, "evidence": <qBittorrent version or "connected"> }`. When qBittorrent's login returns anything other than `"Ok."` (e.g. the literal `"Fails."`), the routine MUST return `{ "ok": false, "detail": "Authentication failed — qBittorrent rejected the credentials." }` — surfacing the specific auth failure at the UI layer, not just in backend logs.
|
||||
|
||||
#### Scenario: correct credentials succeed with version evidence
|
||||
|
||||
+ GIVEN a qBittorrent instance reachable at the configured URL with valid username/password
|
||||
+ WHEN the test routine runs
|
||||
+ THEN it returns `{ "ok": true, "detail": "Connected to qBittorrent.", "evidence": <version> }`
|
||||
|
||||
#### Scenario: wrong password surfaces a specific auth message
|
||||
|
||||
+ GIVEN a qBittorrent instance that returns `"Fails."` on login
|
||||
+ WHEN the test routine runs
|
||||
+ THEN it returns `{ "ok": false, "detail": "Authentication failed — qBittorrent rejected the credentials.", "evidence": null }`
|
||||
|
||||
### Requirement: CT-105 — Prometheus test queries the Grafana gateway with expr "up"
|
||||
|
||||
The `prometheus` test routine MUST issue `POST {grafana_url}/api/ds/query` carrying `Authorization: Bearer {grafana_api_key}` and a `queries[0]` entry keyed by the configured `datasource_uid` with `expr: "up"`. There MUST be no direct Prometheus HTTP call (the `prometheus` service sources through Grafana per `grafana-metric-gateway`). On success it returns `{ "ok": true, "evidence": "Gateway reachable; datasource responded." }`. On HTTP 401/403 it returns an auth-specific message; on connection error it returns an unreachable message.
|
||||
|
||||
#### Scenario: gateway reachable returns ok
|
||||
|
||||
+ GIVEN a prometheus service with valid Grafana gateway credentials
|
||||
+ WHEN the test routine runs
|
||||
+ THEN it returns `{ "ok": true, "detail": "Grafana gateway reachable.", "evidence": "Gateway reachable; datasource responded." }`
|
||||
|
||||
#### Scenario: wrong API key surfaces auth failure
|
||||
|
||||
+ GIVEN a prometheus service whose `grafana_api_key` is invalid
|
||||
+ WHEN the test routine runs
|
||||
+ THEN it returns `{ "ok": false, "detail": <message mentioning authentication/authorization>, "evidence": null }`
|
||||
|
||||
### Requirement: CT-106 — Alertmanager test probes /api/v2/alerts or /api/v2/status
|
||||
|
||||
The `alertmanager` test routine MUST issue an authenticated GET (using the optional `api_key` secret when set) to `/api/v2/alerts` (or `/api/v2/status`). On success it returns `{ "ok": true, "evidence": <cluster version> }`. On failure it translates the error per the shared helper.
|
||||
|
||||
### Requirement: CT-107 — Jellyfin test calls /Users via JellyfinClient
|
||||
|
||||
The `jellyfin` test routine MUST construct a `JellyfinClient` from config + secrets and call `.users()`. On success it returns `{ "ok": true, "evidence": "<N> users" }`. On failure it translates the error.
|
||||
|
||||
### Requirement: CT-108 — Authentik test probes its directory endpoint
|
||||
|
||||
The `authentik` test routine MUST issue a lightweight authenticated GET against the Authentik directory endpoint (the same one the existing Authentik users flow uses). On success it returns `{ "ok": true, "evidence": <instance slug or "connected"> }`. On failure it translates the error.
|
||||
|
||||
### Requirement: CT-109 — ssh_tasks test reuses the test_machine_ssh connect flow
|
||||
|
||||
The `ssh_tasks` test routine MUST reuse the existing SSH connection logic (construct an `RemoteSSHClient` from the service config + SSH key, call `.connect()`, translate banner/auth errors per the established `test_machine_ssh` patterns). On success it returns `{ "ok": true, "evidence": "Connected to <host>:<port>" }`. The known-host recording behavior (first successful connect records the host key) is preserved.
|
||||
|
||||
### Requirement: CT-110 — Nextcloud test probes /status.php
|
||||
|
||||
The `nextcloud` test routine MUST issue an unauthenticated GET to `{base_url}/status.php`. On success it returns `{ "ok": true, "evidence": <Nextcloud version> }`. On failure it translates the error.
|
||||
|
||||
### Requirement: CT-111 — Backups type has no remote test
|
||||
|
||||
The `backups` service type has no remote connection (its data is internal). Its definition MUST set `test_callable = None`, and the endpoint returns `{ "ok": true, "detail": "No connection test for this service type" }` (CT-103).
|
||||
|
||||
### Requirement: CT-112 — The test endpoint MUST NOT persist any service row or secret
|
||||
|
||||
The test endpoint MUST NOT call `store.upsert_service`, `store.update_setting`, or any other persistence method. It accepts plaintext secrets in the request body (over TLS, identical to the existing create endpoint), probes the target, and discards the secrets. A test MUST verify this by calling `POST /api/services/test` and asserting the service count in the store is unchanged afterward.
|
||||
|
||||
#### Scenario: calling /test does not create a row
|
||||
|
||||
+ GIVEN an empty service store
|
||||
+ WHEN `POST /api/services/test` is called with a valid `qbittorrent` input
|
||||
+ THEN the store's service count remains 0
|
||||
+ AND no new row exists for that service type
|
||||
|
||||
### Requirement: CT-113 — Secrets are never logged
|
||||
|
||||
The test endpoint MUST log at INFO level only `test requested type=%s ok=%s` — never the request body, config, or secrets. The log call MUST pass through `sanitize_log_extra` (the existing helper) so any accidental inclusion of secret-named keys is scrubbed.
|
||||
|
||||
#### Scenario: logs contain no secret values
|
||||
|
||||
+ GIVEN a test call carrying `secrets: { "password": "hunter2" }`
|
||||
+ WHEN the endpoint runs
|
||||
+ THEN no log line contains "hunter2"
|
||||
+ AND the INFO log line reads `test requested type=qbittorrent ok=false` (or similar), with no secret values
|
||||
|
||||
### Requirement: CT-114 — useTestServiceInstance mutation hook + testServiceInstance API client function
|
||||
|
||||
`frontend/src/api/services.ts` MUST gain `testServiceInstance(input: ServiceInstanceInput): Promise<TestResult>` posting to `/api/services/test`. `frontend/src/hooks/useServices.ts` MUST gain `useTestServiceInstance()` returning a TanStack mutation whose `mutateAsync` resolves to `{ ok, detail, evidence }`.
|
||||
|
||||
### Requirement: CT-115 — Test credentials button in CreateServiceDialog and the ServicePage edit dialog
|
||||
|
||||
Both the create dialog (`CreateServiceDialog` in `ServicesPage.tsx`) and the edit dialog on `ServicePage.tsx` MUST render a "Test credentials" button below the config/secret form fields. Clicking it fires the `useTestServiceInstance` mutation with the current draft values. While pending, the button shows a "Testing…" state and is disabled.
|
||||
|
||||
### Requirement: CT-116 — Result pill renders both states with evidence or detail
|
||||
|
||||
After a test completes, the dialog MUST render an inline status pill:
|
||||
|
||||
+ On success (`ok: true`): a green pill reading `✓ Connected` plus the `evidence` string.
|
||||
+ On failure (`ok: false`): a red/destructive pill reading `✗ <detail>`.
|
||||
|
||||
The pill is rendered in the same dialog, below the Test button, above the footer.
|
||||
|
||||
### Requirement: CT-117 — Create/Save confirm is gated on a passed test with a "Save anyway" override
|
||||
|
||||
The Create (in the add dialog) and Save (in the edit dialog) confirm buttons MUST be disabled by default until `testPassed` is true. A "Save anyway" checkbox MUST appear that, when checked, re-enables the confirm button — allowing an operator to pre-configure a service that is not yet online. The default state of the checkbox is unchecked (gating is on).
|
||||
|
||||
### Requirement: CT-118 — Editing a connectivity field clears the test result
|
||||
|
||||
When the user edits any field that affects connectivity (the URL/base_url config field, or any secret field), the dialog MUST clear the previous `testPassed` state and the result pill, requiring a fresh test before the confirm button is re-enabled (unless "Save anyway" is checked). This prevents a stale green result from masking a typo correction.
|
||||
|
||||
### Requirement: CT-119 — Backend tests cover routines, dispatch, validation, and no-persistence
|
||||
|
||||
`pytest` from `backend/` MUST pass, including new tests for:
|
||||
|
||||
+ Each type's `test_connection` routine (mocked client) returning the expected `{ok, detail, evidence}` shape for both success and at least one failure case.
|
||||
+ The `POST /api/services/test` endpoint dispatching correctly per type.
|
||||
+ Validation running before the test (422 on malformed config, no network call).
|
||||
+ No-persistence: calling `/test` leaves the store unchanged.
|
||||
|
||||
### Requirement: CT-120 — Frontend tests cover button, pill, gating, and field-clear
|
||||
|
||||
`npm run test` (vitest) from `frontend/` MUST pass, including new tests for:
|
||||
|
||||
+ The Test button fires the mutation.
|
||||
+ The result pill renders both the green (ok) and red (failed) states.
|
||||
+ The Create/Save confirm button is disabled until the test passes (and re-enabled by "Save anyway").
|
||||
+ Editing a connectivity field clears `testPassed`.
|
||||
|
||||
### Requirement: CT-121 — Build and lint stay green
|
||||
|
||||
`npm run build` (`tsc -b` + `vite build`) and `npm run lint` from `frontend/`, and `ruff check src tests` from `backend/`, MUST pass with 0 errors (pre-existing warnings acceptable).
|
||||
@@ -0,0 +1,183 @@
|
||||
# Sync Report — `service-credential-tester`
|
||||
|
||||
> Phase: **sync** · Change: `service-credential-tester` · Repo: `/home/user/manage`
|
||||
> Mode: file-backed (`artifactStore: openspec`). No source-code edits; only OpenSpec artifacts were
|
||||
> written. Not committed (parent owns the commit). The change folder was **not** moved (that is
|
||||
> `sdd-archive`'s job).
|
||||
|
||||
**Status: SYNCED.** A new canonical domain
|
||||
`openspec/specs/service-credential-testing/spec.md` was created from the verified change, and the
|
||||
change-side domain delta spec that unblocks the native status engine's `sync`/`archive` gates is
|
||||
also in place.
|
||||
|
||||
---
|
||||
|
||||
## 1. Executive summary
|
||||
|
||||
The `service-credential-tester` change shipped a **complete but flat**
|
||||
`openspec/changes/service-credential-tester/spec.md` (21 requirements, CT-101 … CT-121) with **no**
|
||||
per-domain delta spec under `openspec/changes/service-credential-tester/specs/<domain>/`. `sdd-sync`
|
||||
requires a domain delta spec; the flat spec alone does not satisfy the canonical-merge contract.
|
||||
|
||||
Verify already returned a **functional PASS** — verdict in `verify-report.md`: all 21 requirements
|
||||
CT-101 … CT-121 PASS against source, and all five gates are green (backend `pytest` 362 passed,
|
||||
`ruff` clean, frontend `npm run build`, `npm run lint` 0 errors, `npx vitest run` 158 passed). The
|
||||
verify report's two CRITICAL items are **archive-only** blockers (B-1 missing `apply-progress.md`,
|
||||
B-2 unchecked task checkboxes); the parent confirms `apply-progress.md` is now present and
|
||||
reconciled. These are lifecycle-hygiene items, not verification-integrity blockers, and they do not
|
||||
block `sdd-sync` of the green code.
|
||||
|
||||
This sync **reconciles** the flat-spec-vs-domain-spec gap:
|
||||
|
||||
1. Authored the missing **change-side domain delta spec** —
|
||||
`openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md` — using a
|
||||
clean `## ADDED Requirements` structure that preserves the exact requirement IDs (CT-101 …
|
||||
CT-121) and text from the verified flat `spec.md`. This is what flips the native status engine's
|
||||
`specs` artifact from partial → done.
|
||||
2. **Synced** the end-state into the **canonical store** —
|
||||
`openspec/specs/service-credential-testing/spec.md` — the actual sync target. Because the
|
||||
canonical `service-credential-testing` domain did not previously exist, the native helper rule
|
||||
applies: *when the canonical spec does not exist, the change spec becomes the new canonical
|
||||
spec.* The two files therefore carry identical requirement bodies (delta under
|
||||
`## ADDED Requirements`; canonical under `## Requirements`).
|
||||
|
||||
Domain name **`service-credential-testing`** was chosen (per the dispatch brief) because it scopes
|
||||
the full new capability: the `POST /api/services/test` endpoint, the closed `test_callable`
|
||||
dispatch registry, the seven per-type connection-test routines, the no-persistence /
|
||||
no-secret-logging guarantees, and the frontend test UI. It is distinct from the existing canonical
|
||||
domains `web-ui` (MUI→shadcn migration), `prometheus-charting` (direct Prometheus metric
|
||||
visualization), and `service-storage` (per-service data lifecycle layer) — none of which was
|
||||
**touched**.
|
||||
|
||||
## 2. Structured status & actionContext findings
|
||||
|
||||
The native `gentle-pi.sdd-status` passed by the parent reports `changeName: null` with
|
||||
`blockedReasons: ["Change selection is ambiguous: per-instance-hook-scoping,
|
||||
service-credential-tester."]` because the engine auto-detected more than one active change. This
|
||||
sync task was **explicitly assigned** `service-credential-tester`; the ambiguity is a
|
||||
parent-resolution artifact and does not block this phase (`isNonAuthoritative: false`).
|
||||
|
||||
- `artifactStore: openspec`; change root `openspec/changes/service-credential-tester/`.
|
||||
- Artifacts present: `proposal.md`, `spec.md`, `design.md`, `tasks.md`, `apply-progress.md`,
|
||||
`verify-report.md`.
|
||||
- `verify: PASS` (functional verdict; gates green at `f6c67bd`).
|
||||
- `actionContext`: `mode: repo-local`, `workspaceRoot: /home/user/manage`,
|
||||
`allowedEditRoots: ["/home/user/manage"]`, `warnings: []`. All three files written are inside the
|
||||
authoritative workspace / allowed edit roots. ✓
|
||||
- `relationships.sameDomainActiveChanges: []`, `collisions: []` — **no active same-domain
|
||||
collisions**, so no archive/sync ordering decision was required.
|
||||
- The new `service-credential-testing` domain is distinct from the existing `web-ui`,
|
||||
`prometheus-charting`, and `service-storage` canonical domains; all three were left untouched
|
||||
(verified via `git status --porcelain`).
|
||||
|
||||
**Verify verdict nuance (archive-only blockers):** the verify report's verdict is "PASS
|
||||
(functionally) — every requirement CT-101 … CT-121 is met … ARCHIVE IS BLOCKED on a task-hygiene /
|
||||
missing-`apply-progress` issue." The two CRITICAL findings (B-1, B-2) are explicitly **archive**
|
||||
blockers, not verification failures: the code is functionally complete and all gates are green.
|
||||
`sdd-sync`'s stop conditions target *verification integrity* (unresolved FAIL/BLOCKED/CRITICAL
|
||||
**verification** blockers), and these are lifecycle-hygiene items. The parent states
|
||||
`apply-progress.md` is now reconciled. Sync therefore proceeds; the unchecked-tasks item is
|
||||
forwarded to `sdd-archive` (§7).
|
||||
|
||||
**Post-sync structural change:**
|
||||
`openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md` now exists
|
||||
(`hasDomainSpecs` → true), resolving the missing-domain-spec condition that gated sync. The flat
|
||||
`spec.md` is intentionally **left in place** as the authoritative planning artifact the work was
|
||||
built against (the archive convention keeps flat specs too); it no longer triggers the "flat spec
|
||||
without domain specs" condition now that a domain delta sits alongside it.
|
||||
|
||||
## 3. Domains synced & canonical files updated
|
||||
|
||||
| Domain | Change-side delta (source) | Canonical (sync target) | Action |
|
||||
|---|---|---|---|
|
||||
| `service-credential-testing` | `openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md` | `openspec/specs/service-credential-testing/spec.md` | **NEW domain** — `## ADDED Requirements` copied into canonical as a new spec |
|
||||
|
||||
- **Canonical file created:** `openspec/specs/service-credential-testing/spec.md` (21 requirements).
|
||||
- **Change-side delta created:**
|
||||
`openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md`
|
||||
(21 requirements, all `## ADDED Requirements`).
|
||||
|
||||
## 4. Requirement delta (ADDED / MODIFIED / REMOVED)
|
||||
|
||||
- **ADDED (21)** — all to the new `service-credential-testing` domain (canonical did not exist
|
||||
pre-change). IDs and text preserved verbatim from the verified flat `spec.md`. Grouped logically:
|
||||
- *Endpoint shape, validation, dispatch* — CT-101, CT-102, CT-103
|
||||
- *Per-type test routines* — CT-104, CT-105, CT-106, CT-107, CT-108, CT-109, CT-110, CT-111
|
||||
- *No-persistence + security* — CT-112, CT-113
|
||||
- *Frontend test UI* — CT-114, CT-115, CT-116, CT-117, CT-118
|
||||
- *Tests + gate greenness* — CT-119, CT-120, CT-121
|
||||
- **MODIFIED (0)** — none (new domain; no pre-existing canonical requirements to replace).
|
||||
- **REMOVED (0)** — none.
|
||||
- **RENAMED (0)** — none (RENAMED is intentionally unsupported by the native delta helper; not used).
|
||||
|
||||
## 5. Guardrails, approvals & destructive-sync assessment
|
||||
|
||||
- **Same-domain collisions:** none (`sameDomainActiveChanges: []`, `collisions: []`). The new
|
||||
`service-credential-testing` domain does not overlap the existing `web-ui`,
|
||||
`prometheus-charting`, or `service-storage` canonical domains. No ordering decision was needed.
|
||||
- **Destructive sync:** **not applicable.** There are zero REMOVED requirements and zero large
|
||||
MODIFIED blocks (new domain; everything is ADDED). No destructive-sync parent approval was
|
||||
required beyond the explicit reconciliation instruction in the task.
|
||||
- **Legacy flat spec:** detected pre-sync; resolved by adding the domain delta spec alongside it
|
||||
(the block condition is specifically "flat spec *without* domain specs"). The flat spec was left
|
||||
in place as a planning artifact.
|
||||
- **`web-ui` / `prometheus-charting` / `service-storage` canonical isolation:** the existing
|
||||
`openspec/specs/web-ui/spec.md`, `openspec/specs/prometheus-charting/spec.md`, and
|
||||
`openspec/specs/service-storage/spec.md` were **not modified** — verified untouched by
|
||||
`git status --porcelain openspec/specs/web-ui openspec/specs/prometheus-charting
|
||||
openspec/specs/service-storage` (empty). The four domains are independent.
|
||||
|
||||
## 6. Validation / checks performed (file-backed, read-only)
|
||||
|
||||
Run from `/home/user/manage` (no source edits, no test re-runs — those are owned by verify and were
|
||||
already green at `f6c67bd`):
|
||||
|
||||
| Check | Command | Result |
|
||||
|---|---|---|
|
||||
| Canonical store populated | `ls openspec/specs/service-credential-testing/spec.md` | present ✓ |
|
||||
| Change-side domain spec present | `ls openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md` | present ✓ |
|
||||
| Requirement-ID parity (flat ↔ delta ↔ canonical) | `grep -oE 'CT-[0-9]+' … \| sort -u \| wc -l` | **21 == 21 == 21**, identical IDs CT-101…CT-121 ✓ |
|
||||
| ID-set parity (flat ↔ canonical) | `diff <(…flat…) <(…canonical…)` | **PARITY OK** — identical ID sets ✓ |
|
||||
| Body-text parity (delta ↔ canonical) | `### Requirement:` regions compared | identical prose ✓ |
|
||||
| Delta is pure ADDED | count `## ADDED/MODIFIED/REMOVED Requirements` | ADDED=1, MODIFIED=0, REMOVED=0 ✓ (no destructive sync) |
|
||||
| Other canonicals untouched | `git status --porcelain openspec/specs/web-ui openspec/specs/prometheus-charting openspec/specs/service-storage` | empty (not modified) ✓ |
|
||||
| Flat spec left in place | `ls openspec/changes/service-credential-tester/spec.md` | present (not moved/deleted) ✓ |
|
||||
| No edits outside openspec | `git status --porcelain` (filtered) | only `openspec/specs/service-credential-testing/`, `openspec/changes/service-credential-tester/specs/`, and this report added ✓ |
|
||||
| Markdown validity | write-time lint | all three files "Markdown clean" ✓ |
|
||||
|
||||
## 7. Carry-over items for the archive summary
|
||||
|
||||
These verify-phase findings are non-blocking for sync and should land in the archive summary:
|
||||
|
||||
1. **[CRITICAL-process, archive-only] Unchecked task checkboxes.** At verify time, all 29 tasks in
|
||||
`tasks.md` were unchecked (`- [ ]`) and `apply-progress.md` was missing (verify findings B-1/B-2).
|
||||
The parent states `apply-progress.md` is now present and reconciled. `sdd-archive` should re-scan
|
||||
the native status engine to confirm `tasks: done` / `applyProgress: present` and tick any
|
||||
remaining unchecked boxes before moving the change to archive.
|
||||
2. **[INFO] Stale-proposal correction carried into the spec.** The flat spec documents that
|
||||
`jellyseerr` is no longer a distinct service type (merged into Jellyfin config by
|
||||
`services-as-hub-ia`); the 7 remote types + `backups` are the active coverage. No `jellyseerr`
|
||||
requirement exists.
|
||||
3. **[INFO] `prometheus` routes through the Grafana gateway.** CT-105's canonical text (verbatim
|
||||
from the flat spec) references `grafana-metric-gateway`. This is a pre-existing design assumption
|
||||
recorded faithfully; it does not affect sync.
|
||||
4. **[INFO] List-marker normalization.** The flat source uses `-` bullets; the linter normalized
|
||||
list markers in the delta/canonical files (cosmetic only — requirement prose is verbatim and the
|
||||
delta helper matches by `### Requirement:` blocks, not marker style).
|
||||
|
||||
## 8. Next recommended phase
|
||||
|
||||
→ **`sdd-archive`** (clean). Confirm the native status re-scan reports `specs: done` / `sync: ready`
|
||||
/ `archive: ready`, then move the change to
|
||||
`openspec/changes/archive/2026-07-09-service-credential-tester`, carrying over the items in §7 into
|
||||
the archive summary. Do **not** commit or push — the parent owns the commit with explicit paths.
|
||||
|
||||
---
|
||||
|
||||
### Appendix — Files written by this sync (OpenSpec only; no source code)
|
||||
|
||||
- `openspec/changes/service-credential-tester/specs/service-credential-testing/spec.md` —
|
||||
**change-side domain delta (`## ADDED Requirements`), 21 requirements CT-101…CT-121.**
|
||||
- `openspec/specs/service-credential-testing/spec.md` — **canonical spec (new domain), 21
|
||||
requirements.**
|
||||
- `openspec/changes/service-credential-tester/sync-report.md` — this report.
|
||||
@@ -0,0 +1,266 @@
|
||||
# SDD Tasks: Service Credential Tester
|
||||
|
||||
**Change:** `service-credential-tester`
|
||||
**Phase:** tasks
|
||||
**Date:** 2026-07-09
|
||||
|
||||
## Review Workload Forecast
|
||||
|
||||
| Field | Value |
|
||||
|-------|-------|
|
||||
| Estimated changed lines | ~550–700 (sum of two implementation slices) |
|
||||
| 400-line budget risk | Low–Medium |
|
||||
| Chained PRs recommended | Yes |
|
||||
| Suggested split | PR 1: backend endpoint + TestResult/test_callable/translate_connection_error + 7 per-type routines + tests → PR 2: frontend type + API fn + hook + ServiceTestPanel + wire into both surfaces + tests |
|
||||
| Delivery strategy | auto-chain |
|
||||
| Chain strategy | stacked-to-main |
|
||||
|
||||
```text
|
||||
Decision needed before apply: No
|
||||
Chained PRs recommended: Yes
|
||||
Chain strategy: stacked-to-main
|
||||
400-line budget risk: Low–Medium
|
||||
```
|
||||
|
||||
> Each slice individually lands under the 400-line review budget. Slices are ordered S1 → S2; S1 is independently shippable (backend endpoint works, frontend has no Test UI until S2). Per `openspec/config.yaml` rules, each slice leaves `npm run build` (tsc -b + vite build), `npm run lint`, and backend `pytest` green.
|
||||
|
||||
---
|
||||
|
||||
## Slice ordering rationale (critical)
|
||||
|
||||
**Slice 1 builds the backend endpoint + per-type test routines.** After S1:
|
||||
|
||||
- `POST /api/services/test` validates input (reuses `_validate_input`), dispatches to the definition's `test_callable`, and returns `{ok, detail, evidence}` — no persistence.
|
||||
- Each of 7 service types (qbittorrent, prometheus, alertmanager, jellyfin, authentik, ssh_tasks, nextcloud) has a `test_connection` routine; backups has `test_callable = None`.
|
||||
- `TestResult` dataclass + `translate_connection_error` shared helper live in `integrations/base.py`.
|
||||
- All backend tests are green: per-type mocked routines, endpoint dispatch, validation-first, no-persistence.
|
||||
- The frontend has NO Test UI yet — the endpoint is callable via API only. That's fine; S2 adds the UI.
|
||||
|
||||
**Slice 2 adds the frontend Test UI.** A shared `ServiceTestPanel` component (Test button + result pill + Save-anyway override + field-edit-clears-result) is wired into BOTH `CreateServiceDialog` (in `ServicesPage.tsx`) AND `ServiceConfigEditor` (in `Settings.tsx` — per the design source finding, NOT `ServicePage.tsx`). The panel is identical in both surfaces.
|
||||
|
||||
This ordering ensures the backend is proven (endpoint dispatches correctly, validation rejects malformed input, no-persistence holds) before any frontend churn.
|
||||
|
||||
---
|
||||
|
||||
## Source-finding corrections (read before applying)
|
||||
|
||||
The design (§0) surfaced deviations from the proposal/spec. Tasks below incorporate these corrections:
|
||||
|
||||
| Spec/proposal claim | Actual source reality | Correction in tasks |
|
||||
|---|---|---|
|
||||
| CT-115: "the edit dialog on `ServicePage.tsx`" | **`ServicePage.tsx` has no edit dialog.** Service editing lives in `Settings.tsx::ServiceConfigEditor` (master-detail panel, lines ~1435–1596). | Task 2.6 targets `Settings.tsx::ServiceConfigEditor`, NOT `ServicePage.tsx`. |
|
||||
| CT-109: "reuses the `test_machine_ssh` connect flow" | `test_machine_ssh` (settings.py:122–180) is a **router endpoint**, not a reusable function. But `build_ssh_client` in `task_runner.py` builds the client from a `ServiceRecord`. | Task 1.8 uses `build_ssh_client(store, ServiceRecord)` + `.connect()`, then applies the same error-translation patterns inline. It does NOT import or call `test_machine_ssh`. |
|
||||
| Proposal: "construct a `QbittorrentClient` from the config + decrypted secrets" | `QbittorrentClient.__init__` takes positional strings (`base_url, username, password, timeout`), not a config dict. | Task 1.4 extracts fields explicitly from `config`/`secrets` dicts. |
|
||||
| Proposal mentions `jellyseerr` as a distinct type | Active registry has no `jellyseerr` entry (merged into Jellyfin by `services-as-hub-ia`). | No jellyseerr test routine. The 7 remote types are: qbittorrent, prometheus, alertmanager, jellyfin, authentik, ssh_tasks, nextcloud. |
|
||||
|
||||
No proposal/spec scope change is required — the intent (per-type credential tester) holds. These corrections refine implementation details and the edit-surface target.
|
||||
|
||||
---
|
||||
|
||||
## Slice 1: Backend endpoint + test_callable + per-type routines + tests
|
||||
|
||||
**Goal:** Add `POST /api/services/test` that validates unsaved service input, dispatches to a per-type `test_callable`, and returns `{ok, detail, evidence}` without persisting. Each of 7 service types gets a `test_connection` routine; backups has no test.
|
||||
|
||||
**Satisfies:** CT-101, CT-102, CT-103, CT-104, CT-105, CT-106, CT-107, CT-108, CT-109, CT-110, CT-111, CT-112, CT-113, CT-119.
|
||||
|
||||
- [x] **1.1 Add `TestResult` dataclass + `TestCallable` type alias + `test_callable` field on `ServiceDefinition` (CT-101, CT-103)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/base.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: none
|
||||
- Details: Add `@dataclass(frozen=True) class TestResult` with fields `ok: bool`, `detail: str`, `evidence: str | None = None`. Add a type alias `TestCallable = Callable[[dict[str, Any], dict[str, str], "SettingsStore"], TestResult]` (forward-reference SettingsStore to avoid circular import at module level; import it inside the test routines or use a string annotation). Add `test_callable: TestCallable | None = None` to `ServiceDefinition` — placed LAST (after `widget_kinds`) so the dataclass field-ordering with a default works. Import `Callable` from `typing`. Update `TYPE_CHECKING` guard if needed for the SettingsStore forward reference.
|
||||
|
||||
- [x] **1.2 Add `translate_connection_error` shared helper (CT-104..CT-110)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/base.py` (modify, same file)
|
||||
- Lines: ~40
|
||||
- Dependencies: 1.1
|
||||
- Details: Add `def translate_connection_error(exc: Exception, *, context: str = "") -> TestResult:` that maps common exception types to human-friendly messages. Handle: `requests.HTTPError` with status 401/403 → auth message; `requests.ConnectionError` / `ConnectionRefusedError` / `OSError` with DNS keywords (`getaddrinfo`, `name or service not known`) → "Host not found"; generic connection error → "Connection refused"; SSL/certificate keywords → "SSL/TLS error"; `requests.Timeout` / `TimeoutError` / `asyncio.TimeoutError` → "Connection timed out"; SSH "protocol banner" → "SSH banner not received". Fallback: truncate message to 200 chars, prefix with context. The helper returns `TestResult(ok=False, detail=...)` always. Import `requests` and `asyncio` at the top of `base.py` (or inside the function to avoid circular imports — prefer top-level since `base.py` already may need `requests` for the type checks).
|
||||
|
||||
- [x] **1.3 Add `test_connection` for qbittorrent (CT-104)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/qbittorrent.py` (modify)
|
||||
- Lines: ~25
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config: dict[str, Any], secrets: dict[str, str], store: SettingsStore) -> TestResult:` that constructs `QbittorrentClient(config["base_url"], secrets["username"], secrets["password"], timeout=config.get("timeout_seconds", 10))`, calls `client.maindata()`, extracts `server_state` version (fallback `"connected"`), returns `TestResult(ok=True, detail="Connected to qBittorrent.", evidence=version)`. Catch `RuntimeError` with "login failed" in the message → `TestResult(ok=False, detail="Authentication failed — qBittorrent rejected the credentials.")`. All other exceptions → `translate_connection_error(exc, context="qBittorrent")`. Import `QbittorrentClient` from `clients.qbittorrent`, `TestResult` + `translate_connection_error` from `integrations.base`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.4 Add `test_connection` for prometheus via Grafana gateway (CT-105)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/prometheus.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that issues `POST {grafana_url}/api/ds/query` with `Authorization: Bearer {grafana_api_key}`, body `{"queries": [{"datasource": {"uid": datasource_uid, "type": "prometheus"}, "expr": "up", "format": "time_series", "intervalMs": 15000, "maxDataPoints": 1, "refId": "A"}], "from": "now-1m", "to": "now"}`. Extract `grafana_url`, `datasource_uid`, `timeout_seconds` from config; `grafana_api_key` from secrets. Missing URL/key → `TestResult(ok=False, detail="...")` (no network). Success → `TestResult(ok=True, detail="Grafana gateway reachable.", evidence="Gateway reachable; datasource responded.")`. Errors → `translate_connection_error(exc, context="Prometheus via Grafana")`. Mirror `MetricSource._gateway_query` request shape exactly. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.5 Add `test_connection` for alertmanager (CT-106)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/alertmanager.py` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that GETs `{base_url}/api/v2/status` with optional `Authorization: Bearer {api_key}` (when secret present). Extract version from `versionInfo.version` (fallback `"connected"`). Success → `TestResult(ok=True, detail="Connected to Alertmanager.", evidence=version)`. Errors → `translate_connection_error(exc, context="Alertmanager")`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.6 Add `test_connection` for jellyfin (CT-107)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/jellyfin.py` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs `JellyfinClient(config["base_url"], secrets["api_key"], timeout=config.get("timeout_seconds", 10))` and calls `.users()`. Success → `TestResult(ok=True, detail="Connected to Jellyfin.", evidence=f"{len(users)} users")`. Errors → `translate_connection_error(exc, context="Jellyfin")`. Import `JellyfinClient` from `clients.jellyfin`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.7 Add `test_connection` for authentik (CT-108)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/authentik.py` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs `AuthentikClient(base_url=config["base_url"], api_token=secrets["api_token"], timeout=config.get("timeout_seconds", 10))` and calls `.users(page=1, page_size=1)` — the lightest directory probe. Success → `TestResult(ok=True, detail="Connected to Authentik.", evidence=...)`. Errors → `translate_connection_error(exc, context="Authentik")`. Import `AuthentikClient` from its module (check `routers/authentik_users.py::_build_client` for the exact import path + constructor signature). Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.8 Add `test_connection` for ssh_tasks reusing `build_ssh_client` (CT-109)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/ssh_tasks.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that constructs a `ServiceRecord(id="", service_type="ssh_tasks", name="test", config=config, secrets=secrets, enabled=True)`, calls `build_ssh_client(store, service)` (from `services.task_runner`), then `.connect()` inside try/finally with `.close()`. Translate errors inline for SSH-specific patterns: "protocol banner" → `"SSH banner not received from {host}:{port}; confirm the SSH service is running."`; "no authentication methods available" / "authentication failed" → `"SSH authentication failed for {host}:{port}; check the SSH key, passphrase, or username."`. Other exceptions → `translate_connection_error(exc, context=f"SSH {host}:{port}")`. Success → `TestResult(ok=True, detail=f"SSH connection succeeded for {host}:{port}.", evidence=f"Connected to {host}:{port}")`. Import `build_ssh_client` from `media_library_viewer_api.services.task_runner` and `ServiceRecord` from `media_library_viewer_api.widgets.sources` (or define locally to avoid circular import — check). **Do NOT import or call `test_machine_ssh`** — it's a router endpoint, not a reusable function. Known-host recording is preserved automatically by `RemoteSSHClient.connect()`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.9 Add `test_connection` for nextcloud (CT-110)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/nextcloud.py` (modify)
|
||||
- Lines: ~15
|
||||
- Dependencies: 1.1, 1.2
|
||||
- Details: Add `def test_connection(config, secrets, store) -> TestResult:` that GETs `{base_url}/status.php` (unauthenticated — `/status.php` is public). Extract `version` from the JSON response (fallback `"connected"`). Success → `TestResult(ok=True, detail="Connected to Nextcloud.", evidence=version)`. Errors → `translate_connection_error(exc, context="Nextcloud")`. Wire `test_callable=test_connection` into the `DEFINITION`.
|
||||
|
||||
- [x] **1.10 Confirm backups DEFINITION has `test_callable = None` (CT-111)**
|
||||
- Files: `backend/src/media_library_viewer_api/integrations/backups.py` (verify, modify only if the field isn't defaulted)
|
||||
- Lines: ~0 (default applies)
|
||||
- Dependencies: 1.1
|
||||
- Details: `backups` should NOT gain a `test_connection` function. Its `DEFINITION` relies on the default `test_callable=None` from `ServiceDefinition`. Verify by reading the file; no code change expected unless the DEFINITION is constructed with explicit keyword args that omit `test_callable` (the default handles it). The endpoint returns `{ok: true, detail: "No connection test for this service type"}` automatically.
|
||||
|
||||
- [x] **1.11 Add `POST /api/services/test` endpoint (CT-101, CT-102, CT-103, CT-112, CT-113)**
|
||||
- Files: `backend/src/media_library_viewer_api/routers/services.py` (modify)
|
||||
- Lines: ~30
|
||||
- Dependencies: 1.1–1.10
|
||||
- Details: Add `@router.post("/test") def test_instance(body: ServiceInstanceInput, store: SettingsStore = Depends(get_settings_store)) -> dict[str, Any]:` that: (a) calls `_validate_input(body)` — reuses the existing helper, raises HTTPException(422) on malformed config/type/secrets BEFORE any network call (CT-102); (b) resolves `definition = require_service_definition(body.service_type)`; (c) if `definition.test_callable is None` → log `test requested type=%s ok=true` and return `{"ok": True, "detail": "No connection test for this service type", "evidence": None}` (CT-103/111); (d) else call `result = definition.test_callable(body.config, body.secrets, store)` inside a try/except (defensive — routines catch internally, but if one raises, return `TestResult(ok=False, detail=f"Test failed unexpectedly: {exc}")`); (e) log `test requested type=%s ok=%s` at INFO — NEVER log body/config/secrets (CT-113); (f) return `{"ok": result.ok, "detail": result.detail, "evidence": result.evidence}`. NO call to `store.upsert_service`, `store.update_setting`, or any persistence method (CT-112). Import `require_service_definition` from `integrations.registry` (already imported or adjacent).
|
||||
|
||||
- [x] **1.12 Add backend tests for `translate_connection_error` (CT-119)**
|
||||
- Files: `backend/tests/test_services.py` (modify) or a new `backend/tests/test_credential_tester.py` (create)
|
||||
- Lines: ~40
|
||||
- Dependencies: 1.2
|
||||
- Details: Test the shared helper directly: (a) `requests.HTTPError` with a mock response `.status_code = 401` → auth message; (b) `requests.ConnectionError("getaddrinfo failed")` → "Host not found"; (c) `requests.Timeout()` → "timed out"; (d) generic `ValueError("something")` → truncated fallback with context prefix. Assert `ok=False` and the detail string contains expected keywords.
|
||||
|
||||
- [x] **1.13 Add backend tests for per-type `test_connection` routines (CT-119)**
|
||||
- Files: `backend/tests/test_services.py` (modify) or `backend/tests/test_credential_tester.py` (create/modify)
|
||||
- Lines: ~100
|
||||
- Dependencies: 1.3–1.9
|
||||
- Details: For each of the 7 remote types, add at least two tests: (a) SUCCESS — mock the client/request to return valid data, assert `{ok: True, detail: ..., evidence: ...}`; (b) FAILURE — mock the client/request to raise, assert `{ok: False, detail: <readable message>, evidence: None}`. Specific cases:
|
||||
- **qbittorrent**: success returns version; `RuntimeError("qBittorrent login failed: Fails.")` → detail mentions "Authentication failed".
|
||||
- **prometheus**: success returns gateway-reachable evidence; HTTP 401 → auth message.
|
||||
- **alertmanager**: success returns version; connection refused → unreachable message.
|
||||
- **jellyfin**: success returns user count; HTTP 401 → auth message.
|
||||
- **authentik**: success returns user count; connection error → unreachable.
|
||||
- **ssh_tasks**: mock `build_ssh_client` to return a mock client; `.connect()` raises "authentication failed" → SSH auth message; `.connect()` succeeds → connected evidence.
|
||||
- **nextcloud**: success returns version; 404 → connection error.
|
||||
- Use `unittest.mock.patch` to mock at the right boundary (the client constructor or `requests.get/post`).
|
||||
|
||||
- [x] **1.14 Add backend tests for the endpoint: dispatch, validation-first, no-persistence (CT-119)**
|
||||
- Files: `backend/tests/test_api.py` (modify) or `backend/tests/test_credential_tester.py` (create/modify)
|
||||
- Lines: ~50
|
||||
- Dependencies: 1.11
|
||||
- Details: (a) DISPATCH: `POST /api/services/test` with a `backups` body → `{ok: True, detail: "No connection test..."}` and no network call. (b) VALIDATION-FIRST: body with schema-less `base_url` for qbittorrent → HTTP 422 (not 200); assert the test_callable was NOT called (mock it, assert call count 0). (c) NO-PERSISTENCE: call `/test` with a valid qbittorrent body (test_callable mocked to return ok), assert `store.list_services()` count is unchanged before/after. (d) NO-SECRET-LOGS: use `caplog` at INFO level, call `/test` with `secrets: {"password": "hunter2"}`, assert no log line contains "hunter2".
|
||||
|
||||
- [x] **1.15 Verify Slice 1 (pytest + ruff + frontend still builds)**
|
||||
- Run: `cd backend && PYTHONPATH=src python3 -m pytest -q && PYTHONPATH=src python3 -m ruff check src tests`
|
||||
- Run: `cd frontend && npm run build && npm run lint`
|
||||
- Verify: all backend tests pass (per-type routines, endpoint dispatch, validation, no-persistence, no-secret-logs); ruff clean; frontend still builds + lints (no frontend change in S1, so this is a regression check only).
|
||||
- **Risk flag:** the `test_callable` field on `ServiceDefinition` is a new field with a default. Verify that existing DEFINITION construction sites (all 8 integration modules) still compile — they should, since the default is `None`, but a frozen dataclass with field-ordering can surprise if any DEFINITION passes positional args. Check for positional-arg construction (unlikely — the codebase uses keyword args).
|
||||
|
||||
**Slice 1 total:** ~300–380 changed lines.
|
||||
|
||||
---
|
||||
|
||||
## Slice 2: Frontend Test UI — API client + hook + shared panel + wire into both surfaces + tests
|
||||
|
||||
**Goal:** Add a "Test credentials" button + result pill + Create/Save gating to BOTH the add-service dialog (`CreateServiceDialog` in `ServicesPage.tsx`) and the edit panel (`ServiceConfigEditor` in `Settings.tsx`). A shared `ServiceTestPanel` component avoids duplication.
|
||||
|
||||
**Satisfies:** CT-114, CT-115, CT-116, CT-117, CT-118, CT-120, CT-121.
|
||||
|
||||
- [x] **2.1 Add `ServiceTestResult` type (CT-114)**
|
||||
- Files: `frontend/src/types/index.ts` (modify)
|
||||
- Lines: ~5
|
||||
- Dependencies: none
|
||||
- Details: Add `export interface ServiceTestResult { ok: boolean; detail: string; evidence: string | null; }`.
|
||||
|
||||
- [x] **2.2 Add `testServiceInstance` API client function (CT-114)**
|
||||
- Files: `frontend/src/api/services.ts` (modify)
|
||||
- Lines: ~5
|
||||
- Dependencies: 2.1
|
||||
- Details: Add `export async function testServiceInstance(input: ServiceInstanceInput): Promise<ServiceTestResult> { return post<ServiceTestResult>("/api/services/test", input); }`. Import `ServiceTestResult` from `../types`. The existing `post` helper (from `./shared`) handles auth headers + error extraction.
|
||||
|
||||
- [x] **2.3 Add `useTestServiceInstance` hook (CT-114)**
|
||||
- Files: `frontend/src/hooks/useServices.ts` (modify)
|
||||
- Lines: ~8
|
||||
- Dependencies: 2.2
|
||||
- Details: Add `export function useTestServiceInstance() { return useMutation({ mutationFn: (input: ServiceInstanceInput) => testServiceInstance(input) }); }`. Import `useMutation` from `@tanstack/react-query` (already imported in the file). Import `testServiceInstance` from `../api/services`. No cache invalidation needed — the test is a one-shot mutation.
|
||||
|
||||
- [x] **2.4 Create shared `ServiceTestPanel` component (CT-115, CT-116, CT-117, CT-118)**
|
||||
- Files: `frontend/src/components/ServiceTestPanel.tsx` (NEW)
|
||||
- Lines: ~70
|
||||
- Dependencies: 2.3
|
||||
- Details: Create a component with props `{ input: ServiceInstanceInput | null; onTestResult: (passed: boolean) => void; }`. Internal state: `result: ServiceTestResult | null`, `saveAnyway: boolean`. `useEffect([input])` clears `result` and calls `onTestResult(false)` whenever `input` changes (new object reference on every field edit — CT-118). A "Test credentials" `Button` (variant outline, size sm) fires `testService.mutateAsync(input)`; while pending shows "Testing…" and is disabled. On result: render an `Alert` — green/default variant with `✓ Connected — {evidence}` on success, destructive variant with `✗ {detail}` on failure. A "Save anyway (skip test)" checkbox toggles `saveAnyway`; when checked, calls `onTestResult(true)` regardless of test outcome (CT-117). The parent reads the gating signal via the `onTestResult` callback. Import `useTestServiceInstance`, `Alert`/`AlertDescription` from `@/components/ui/alert`, `Button` from `@/components/ui/button`.
|
||||
|
||||
- [x] **2.5 Wire `ServiceTestPanel` into `CreateServiceDialog` (CT-115, CT-117)**
|
||||
- Files: `frontend/src/pages/ServicesPage.tsx` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 2.4
|
||||
- Details: In `CreateServiceDialog`: add `const [testPassed, setTestPassed] = useState(false)`. Reset it in `reset()`. Build `testInput` from the current draft (null if no draft). Render `<ServiceTestPanel input={testInput} onTestResult={setTestPassed} />` below the config/secret fields and above the footer. Update `DialogFooter`'s `confirmDisabled` to include `!testPassed` (i.e. `confirmDisabled={!draft.name.trim() || saveService.isPending || !testPassed}`). Import `ServiceTestPanel` from `../components/ServiceTestPanel`.
|
||||
|
||||
- [x] **2.6 Wire `ServiceTestPanel` into `ServiceConfigEditor` in `Settings.tsx` (CT-115, CT-117)**
|
||||
- Files: `frontend/src/pages/Settings.tsx` (modify)
|
||||
- Lines: ~20
|
||||
- Dependencies: 2.4
|
||||
- Details: In `ServiceConfigEditor` (NOT `ServicePage.tsx` — per design source finding §0, service editing lives in `Settings.tsx`): add `const [testPassed, setTestPassed] = useState(false)`. Build `testInput` from the editor's current draft state (name, config, secrets, enabled). Render `<ServiceTestPanel input={testInput} onTestResult={setTestPassed} />` below the form fields. Gate the Save button on `!testPassed` (disabled until test passes or Save anyway is checked). Reset `testPassed` when switching instances (if the editor has an instance-switch effect). Import `ServiceTestPanel`.
|
||||
- **Risk flag:** the spec CT-115 says "ServicePage.tsx" — that is textual drift. The actual edit surface is `Settings.tsx::ServiceConfigEditor`. This task targets the correct file.
|
||||
|
||||
- [x] **2.7 Add frontend tests for `ServiceTestPanel` (CT-120)**
|
||||
- Files: `frontend/src/components/__tests__/ServiceTestPanel.test.tsx` (NEW)
|
||||
- Lines: ~80
|
||||
- Dependencies: 2.4
|
||||
- Details: Test cases: (a) renders Test button; (b) click fires the mocked `useTestServiceInstance` mutation; (c) success result → green `✓ Connected` pill with evidence text; (d) failure result → red `✗` pill with detail text; (e) "Save anyway" checkbox checked → `onTestResult(true)` called regardless of test state; (f) editing `input` (passing a new object) clears the result and calls `onTestResult(false)`. Mock `useTestServiceInstance` via `vi.mock("../../hooks/useServices", ...)`. Use `@testing-library/react` + `@testing-library/user-event`.
|
||||
|
||||
- [x] **2.8 Update existing dialog tests for gating (CT-120)**
|
||||
- Files: `frontend/src/pages/__tests__/ServicesPage.test.tsx` (modify, if it exists) or confirm coverage via the panel test
|
||||
- Lines: ~20
|
||||
- Dependencies: 2.5
|
||||
- Details: If `ServicesPage.test.tsx` tests the create dialog, add assertions that the Create button is disabled until the test passes. Mock the test mutation to return `{ok: true}` and verify the button enables. If there's no existing ServicesPage test covering the dialog, the panel test (2.7) covers the gating behavior adequately — note the coverage decision.
|
||||
|
||||
- [x] **2.9 Verify Slice 2 (build + lint + test)**
|
||||
- Run: `cd frontend && npm run build && npm run lint && npx vitest run`
|
||||
- Run: `cd backend && PYTHONPATH=src python3 -m pytest -q` (regression: Slice 1 tests still pass)
|
||||
- Verify: frontend typechecks + builds; lint 0 errors; vitest passes (new panel test + existing tests); backend still green.
|
||||
- **Risk flag:** `ServiceConfigEditor` in `Settings.tsx` is a large component (~160 lines). The wiring (state + panel render + button gating) must be surgical — do not refactor the editor. Add the `testPassed` state, the `testInput` build, the panel render, and the button `disabled` prop only.
|
||||
|
||||
**Slice 2 total:** ~250–320 changed lines.
|
||||
|
||||
---
|
||||
|
||||
## Integration verification (post-slice)
|
||||
|
||||
- [x] **3.1 Full backend test run**
|
||||
- Run: `cd backend && PYTHONPATH=src python3 -m pytest -q`
|
||||
- Verify: all tests pass (per-type routines, endpoint, validation, no-persistence, no-secret-logs).
|
||||
|
||||
- [x] **3.2 Full frontend build + lint + test**
|
||||
- Run: `cd frontend && npm run build && npm run lint && npx vitest run`
|
||||
- Verify: 0 errors; panel test covers button/pill/gating/field-clear.
|
||||
|
||||
- [x] **3.3 End-to-end dispatch check**
|
||||
- Verify (by reading source or running a manual API call): `POST /api/services/test` with a `backups` body returns `{ok: true, detail: "No connection test for this service type"}`; with a `qbittorrent` body (mocked client) dispatches to the qbittorrent routine; with an unknown type returns 422.
|
||||
|
||||
- [x] **3.4 No-persistence check**
|
||||
- Verify (by test in 1.14): calling `/test` does not create a service row.
|
||||
|
||||
- [x] **3.5 No-secret-logs check**
|
||||
- Verify (by test in 1.14): no log line contains a secret value.
|
||||
|
||||
---
|
||||
|
||||
## Risk flags summary
|
||||
|
||||
1. **(a) CT-115 spec drift — edit surface is `Settings.tsx::ServiceConfigEditor`, NOT `ServicePage.tsx`.** Task 2.6 targets the correct file. The spec's literal text is stale; the design (§0) corrected it. Do not add a Test button to `ServicePage.tsx` (it's a read-only tabbed view with no edit form).
|
||||
|
||||
2. **(b) ssh_tasks test must NOT duplicate `test_machine_ssh`.** `test_machine_ssh` (settings.py:122–180) is a router endpoint, not a reusable function. Task 1.8 uses `build_ssh_client(store, ServiceRecord)` from `task_runner.py` + `.connect()`, then translates errors inline using the same message patterns. No import of `test_machine_ssh`.
|
||||
|
||||
3. **(c) The shared `ServiceTestPanel` must work identically in both surfaces.** Tasks 2.5 + 2.6 wire the same component into `CreateServiceDialog` and `ServiceConfigEditor`. The panel's `input` prop is an object built from the parent's draft state; `onTestResult` is a callback the parent uses for gating. The panel owns the Test button, result pill, and Save-anyway checkbox; the parent owns the confirm button's `disabled` prop.
|
||||
|
||||
4. **(d) `ServiceDefinition` is a frozen dataclass.** Adding `test_callable` with a default `None` as the LAST field works with dataclass field-ordering. Verify no DEFINITION is constructed with positional args (the codebase uses keyword args, so this should be safe). Task 1.15 checks this.
|
||||
|
||||
5. **(e) Circular import risk for `SettingsStore` in `TestCallable`.** The type alias references `SettingsStore` (from `services.settings_store`). Use a string forward-reference (`"SettingsStore"`) in the type alias to avoid importing `settings_store` into `integrations/base.py` at module level. The actual `SettingsStore` is passed at runtime by the endpoint handler. Task 1.1 handles this.
|
||||
|
||||
6. **(f) `Settings.tsx::ServiceConfigEditor` is a large component.** Task 2.6 wiring must be surgical (add state + input build + panel render + button disabled prop). Do NOT refactor the editor or touch unrelated fields.
|
||||
@@ -0,0 +1,259 @@
|
||||
# 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.
|
||||
Reference in New Issue
Block a user