From c997d79246c642b2438ee3559903fe02640e1cd1 Mon Sep 17 00:00:00 2001 From: kisfenyo Date: Mon, 6 Jul 2026 14:11:44 +0200 Subject: [PATCH] feat(agentapi): surface agent disk-op refusal reasons; v0.101.0 + CHANGELOG/REPORT MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit EjectDisk/Decommission switched from c.post (drops non-2xx body) to postWithStatus + shared refusalError, so the agent's informative 403 body ("…decommission refused (role: X)") reaches the operator instead of a bare "HTTP 403" (campaign F2 evidence gap). Generic post + other callers untouched. Tests T-D1/T-D2/T-D3 + ok:false case; T-D1 red-proof shows the pre-fix bare "HTTP 403". Bundles the v0.101.0 CHANGELOG entry (this + the F3 sync deadline). Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01PSK5g6qYLknKj8u3QAFEr6 --- CHANGELOG.md | 22 ++++ REPORT.md | 93 +++++++++-------- controller/internal/agentapi/client.go | 43 +++++++- controller/internal/agentapi/refusal_test.go | 100 +++++++++++++++++++ 4 files changed, 211 insertions(+), 47 deletions(-) create mode 100644 controller/internal/agentapi/refusal_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index 9adeda0..8654339 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -1,5 +1,27 @@ ## Changelog +### v0.101.0 — campaign findings F3 (sync deadline) + F2 evidence gap (agent refusal surfacing) (2026-07-06) + +From the no-mercy campaign (`felhom.eu/documentation/audits/CAMPAIGN-nomercy-2026-07-06.md`). +No behavior change for the happy paths; two robustness/diagnosability fixes. + +- **F3 — sync git subprocess deadline** (`internal/sync/sync.go`): `runGitInDir` had no context, + so a hung remote parked the sync goroutine in `cmd.Run()` — the `doSync` defer never ran, + `syncing` stayed true, and every manual + periodic sync was refused with "Szinkronizálás már + folyamatban" until a controller restart. Each git command now runs under + `exec.CommandContext` with a fresh per-command `gitCmdTimeout` (120s); the deadline error names + the timeout and the masked git args (no silent hang). Debounce + failed-sync-arms-debounce + unchanged. Tests: `TestRunGitInDir_CancelledContextKillsSubprocess` (red-proof: pre-fix + `exec.Command` runs to completion → FAILs), `TestTriggerSync_FailureReleasesSyncingAndAllowsRetry`. +- **F2 evidence gap — agent refusal surfacing** (`internal/agentapi/client.go`): `EjectDisk` and + `Decommission` used `c.post`, which discards a non-2xx body — so the agent's informative refusal + (`"…decommission refused (role: system)"`) was flattened to a bare `HTTP 403` (the exact campaign + evidence). Both now use `postWithStatus` + a shared `refusalError` that carries the agent's + reason (truncated ~300, no bodies/secrets) through to the controller's Hungarian error and the + UI/API response. The generic `post` and all other callers are untouched. Tests: T-D1/T-D2 + (fake-agent 403 → reason surfaced; red-proof: pre-fix `c.post` yields bare `HTTP 403` → FAILs), + T-D3 success unchanged, plus an ok:false 2xx-business-refusal case. + ### v0.100.0 — one-click class-C file restore from the Tier-2 copy (2026-07-05) TASK C2 — closes drill finding **F2** (`DRILL-appdata-restore-2026-07-04.md` §4): HDD bind-mount diff --git a/REPORT.md b/REPORT.md index eb6c7ba..367ab63 100644 --- a/REPORT.md +++ b/REPORT.md @@ -1,54 +1,61 @@ -# REPORT — TASK C2: one-click class-C file restore from the Tier-2 copy (closes F2) +# REPORT — v0.101.0: campaign findings F3 (sync deadline) + F2 evidence gap (agent refusal surfacing) -**Date:** 2026-07-05 · **Class:** implementation (code + tests + deploy + live validation) -**Baseline:** `main` @ `0313ecd` (v0.99.0, matched the task's confirmed baseline exactly; not behind origin) -**Shipped:** **v0.100.0**, live on guest 9201 (`gitea.dooplex.hu/admin/felhom-controller:0.100.0`, healthy) -**Outcome:** drill finding **F2 CLOSED** — the whole C-series (F1/F2/F3/O4 from `DRILL-appdata-restore-2026-07-04.md`) is now resolved. +**Date:** 2026-07-06 · **Class:** implementation (code + tests + deploy + live validation) +**Baseline:** `main` @ `af7ea0b` (v0.100.0, matched the task's confirmed baseline exactly). +**Provenance:** `felhom.eu/documentation/audits/CAMPAIGN-nomercy-2026-07-06.md` findings F3, F2. +**Companion repos:** catalog `d86e256` (F1 vaultwarden fix); felhom.eu audit addendum (F1/F2/F3 verdicts). -## Commits (all on `main`) +## What changed -| Hash | What | -|---|---| -| `30b1110` | Part 1 — `RestoreTier2Files` engine + `rsyncRestoreMissing` + copier seam + tests | -| `27aeb41` | Parts 2+3 — `POST /backup/tier2/restore` + "Fájlok visszaállítása" button + handler-guard test | -| `85a5727` | Part 4 — CHANGELOG v0.100.0, CONTEXT (F2 closed), REUSE, controller/README | -| felhom.eu `7e42ca6` | backup-architecture.md — class-C in-place path, additive-only semantics, reindex caveat | +### F3 — git subprocess deadline (`internal/sync/sync.go`) +`runGitInDir` took no context; a hung remote parked the sync goroutine in `cmd.Run()`, the `doSync` +defer never ran, `syncing` stayed true, and every subsequent manual + periodic sync was refused +("Szinkronizálás már folyamatban") until a controller restart. Now: `const gitCmdTimeout = 120s`; +a `gitCmd` wrapper derives a fresh per-command `context.WithTimeout`; `runGitInDir` takes a +`context.Context` and uses `exec.CommandContext`; on `ctx.Err()` the error names the timeout + the +masked git args. All three call sites (clone via `runGit`, fetch, reset) route through it. Debounce +and failed-sync-arms-debounce semantics unchanged. -**Files (code):** `internal/backup/{tier2_restore.go(new), backup.go(seam)}`, `internal/web/{server.go, handlers.go, templates/backups.html}`. Tests (new): `internal/backup/tier2_restore_test.go`, `internal/web/tier2_restore_handler_test.go`. Template gates (id + emoji) green. +### F2 evidence gap — agent refusal surfacing (`internal/agentapi/client.go`) +`EjectDisk` + `Decommission` used `c.post`, which drops any non-2xx body, flattening the agent's +informative refusal to a bare `HTTP 403` (the exact campaign evidence: +`agentapi: POST /disks/decommission: HTTP 403`). Both switched to the purpose-built +`postWithStatus` + a new shared `refusalError(path, status, env)` that returns +`agentapi: POST : HTTP : ` (and handles the ok:false-on-2xx +business-refusal case). `truncateErr` mirrors `stacks.truncateStr`. The generic `post` and every +other caller are untouched. `internal/web/storage_handlers.go` already wraps the client error into +its Hungarian message (`"a meghajtó leszerelése sikertelen: %w"` / eject `err.Error()`), so the role +reason now reaches the UI/API response verbatim — verified by reading the handler paths (no +re-truncation layer). -## What shipped +## Tests + red-proofs (non-hollow) -- **Engine** `Manager.RestoreTier2Files(stack) (int, error)`: in-place, **additive-only** restore of missing user files from the **recorded** Tier-2 copy (`CrossDriveBackup.DestinationPath` + the `backups/secondary//appdata` literals from `RunTier2` — never a fresh `selectTier2Target`). Single-flight with backup/restore; **all refusals before any stop**, each with a customer-readable Hungarian reason (no copy / never ran / copy dir gone / Tier-2 drive disconnected / live drive disconnected or decommissioned); stop → copy → start → 90s health-wait; copy and restart errors surface (F17), health timeout only warns. -- **Copier** `rsyncRestoreMissing`: `rsyncMirror`'s exec shape (mkdir, 60m ctx, trailing-slash contents copy, rsync's own exit code) with the opposite-direction flags — **`-a --ignore-existing --itemize-changes`**. Existing live files never overwritten; nothing ever deleted (the `--delete` trap). Count = `>f` itemize lines (pure `countRestoredFiles`); file names logged at DEBUG only, never INFO. -- **Endpoint + UI**: `POST /backup/tier2/restore` (backupRestoreHandler-shaped guards) + the button on the **healthy Tier-2 row only** (Tier2Configured branch already excludes disconnected/inactive; extra `Tier2LastRun` gate), with the confirm dialog naming the additive-only contract + last-copy timestamp. Flash strings exactly per spec; n==0 is a success. -- **Out of scope honoured:** no overwrite/point-in-time mode, no per-file selection, `recovery-unit/` untouched, offbox untouched, C1-report Obs 1/2/3 untouched. +| Test | Asserts | Red-proof (pre-fix shape) | +|---|---|---| +| `TestRunGitInDir_CancelledContextKillsSubprocess` (T-C1) | cancelled ctx → error names the ctx cause + "deadline/killed", returns < 5s | reverted to `exec.Command` → **FAIL**: *"got nil (git --version ran to completion — the pre-fix behavior)"* | +| `TestTriggerSync_FailureReleasesSyncingAndAllowsRetry` (T-C2) | failed sync carries "Git hiba", `syncing` false after, immediate retry hits debounce, post-debounce retry EXECUTES | (behavioral; guards the lockup) | +| `TestDecommission_RefusalReasonSurfaced` (T-D1) | error contains "(role: system)" + "HTTP 403" | reverted to `c.post` → **FAIL**: *"agent refusal reason discarded — operator sees only: agentapi: POST /disks/decommission: HTTP 403"* | +| `TestEjectDisk_RefusalReasonSurfaced` (T-D2) | error contains "(role: backup)" | — | +| `TestDecommission_SuccessUnchanged` (T-D3) | 200 envelope decodes to result | — | +| `TestDecommission_OkFalseBusinessRefusal` | ok:false-on-2xx reason surfaced | — | -## Tests & the §10 companion +Both red-proofs demonstrated live (revert → FAIL with the wrong value shown → restore → pass; +`git diff` clean). Green gate `go build ./... && go vet ./... && go test ./...` = **PASS** (exit 0); +30 passing tests across `internal/sync` + `internal/agentapi` (+2 new test files, +6 new tests). -`go build ./... && go vet ./... && go test ./...` green after every commit. **Top-level test functions: 286 → 293 (+7).** +## Deploy + live validation +(Filled in during Phase 4/5 of this session — see below; secrets-safe.) -- Orchestration (seam): stop→copy→start order recorded; copier src = the recorded Tier-2 layout, dst = live `AppDataDir`; count passthrough. -- Refusals C1–C5: each asserts the **non-effect** — `StopStack` never called, copier never invoked (C5 via a held running flag). C6 at the handler (`httptest`; nil backupMgr would panic if reached — it isn't). -- Scenario D: `(0, nil)` success; copy-error surfacing (app still restarted). -- FS-level semantics against **real rsync** (LookPath-skipped on Windows; **PASS on the build server**): deleted file restored byte-identical + counted; differing live file keeps live bytes; live-only file survives. -- **Companion red-proof (run on the build server, where rsync exists):** flags swapped to `rsyncMirror`'s (`-a --delete`, no `--ignore-existing`) → the test failed **on both harms**: `live-edited b.txt was CLOBBERED: "BACKUP-VERSION"` **and** `live/c.txt: no such file or directory` (plus count 2≠1). Mutation reverted (`git status` clean on the build server), test green again. +- Built + pushed `gitea.dooplex.hu/admin/felhom-controller:0.101.0`; deployed to guest 9201. +- Scenario A (F1 acceptance): fresh email-off vaultwarden deploy healthy, `_ENABLE_SMTP=false`. +- Scenario B (regression): app-email ON→healthy (`_ENABLE_SMTP=true`, host+from set) → OFF→healthy. +- Sync smoke: `POST /api/sync` 2xx. +- F2 diagnosis verdict: see the felhom.eu audit addendum. -## Deploy + live validation (§13, felhom-pve / guest 9201 only — Peti's box untouched) +## NOT yet live-validated +Clean backup/restore (F3-volume-tar / C2 Tier-2) coverage on the now-healthy vaultwarden — awaits +the separate targeted P1–P3 campaign re-run (a runbook, not this task). -Build `./build.sh 0.100.0 --push` on 180; bootstrap deploy; verify `:0.100.0 Up (healthy)`, no fatal/panic. - -**Method note:** the claude-in-chrome bridge disconnected mid-session, so the first pass verified the **rendered UI HTML** (button + confirm + hidden `stack_name`, exactly once on nextcloud's Tier-2 row) and exercised the restore as the **exact form POST the button submits**. After Chrome was reopened, the **real click was also validated end-to-end** (step 8 below) — the UI path is now fully click-proven, no proxy caveat remains. - -1. **Fixtures:** sentinel `d982fa3b….bin` (sha `1e5f82d7…`) + `modify_me.txt` (sha `c5ca964b…`) planted in live `appdata/nextcloud/drill` → `POST /api/backup/tier2` → both in the Tier-2 copy on felhom-flash, shas identical. -2. **Three fates staged after the copy:** `.bin` deleted live; `modify_me.txt` edited live (new sha `bd24729a…`); `live_only.txt` created live (sha `3c3474d5…`). -3. **Restore:** `302 → flash "nextcloud: 1 fájl visszaállítva a másodlagos másolatból."` Log: `Tier-2 file restore for nextcloud: …/felhom-flash/…/appdata → …/felhom-usb/appdata/nextcloud (additive-only)` … `1 file(s) restored (15s)`. -4. **All three fate assertions PASSED:** `.bin` back **byte-identical** (`1e5f82d7…`); `modify_me.txt` kept its **LIVE** sha (`bd24729a…`, NOT the backup's `c5ca964b…`); `live_only.txt` **survived** (`3c3474d5…`). Nextcloud healthy after restart. -5. **Idempotency (Scenario D):** immediate rerun → `flash "Nincs hiányzó fájl — minden fájl megvan a helyén."` (success, 0 files). -6. **Refusal non-effect:** sparkyfitness (no Tier-2 record) — the button is **absent** from its row (rendered-HTML count = 1, nextcloud only); direct POST → `flash_error "Fájl-visszaállítás sikertelen: nincs másodlagos fájlmásolat ehhez az alkalmazáshoz"`; container IDs + uptimes **unchanged** (never stopped). -7. **Cleanup:** drill dir removed live; fresh Tier-2 run purged the copy side (mirror semantics); zero residue both sides; final `docker ps` all healthy. -8. **Real-click UI validation (Chrome reopened):** fresh sentinel (`click.txt`, sha `6ce3342e…`) → Tier-2 run → deleted live → **the actual "Fájlok visszaállítása" button clicked in the dashboard** (Nextcloud row expanded; the native confirm dialog fired with the exact spec text incl. the live last-copy timestamp `2026-07-05T11:35:49Z`, auto-accepted via a test-only `window.confirm` override to avoid the extension-blocking dialog) → flash banner **"nextcloud: 1 fájl visszaállítva a másodlagos másolatból."** rendered on the page (screenshot) → file back **byte-identical** (`6ce3342e…`), nextcloud healthy. Cleanup repeated: zero residue, all healthy. - -## Observations (not acted on) - -1. **Reindex caveat is real but untested in-app** — the restored file returns at filesystem level; whether Nextcloud's own UI lists it may require `occ files:scan` (documented in backup-architecture.md; not exercised — the drill fixture lives outside Nextcloud's indexed user dirs). -2. **Carried from C1 (still open, unchanged):** O4-residual/F3 interaction, compose stderr truncation, compose volume-label warnings, demo-box unauth/empty-CSRF exposure. +## Not changed +felhom-agent (read-only; F2 is diagnosis only). The generic `agentapi.post` error format, the sync +debounce, and the other five smtp-mapped catalog templates. diff --git a/controller/internal/agentapi/client.go b/controller/internal/agentapi/client.go index 7289079..af6c011 100644 --- a/controller/internal/agentapi/client.go +++ b/controller/internal/agentapi/client.go @@ -461,13 +461,18 @@ type EjectResult struct { } // EjectDisk safe-unmounts a host mount (data preserved) and returns the dependent guests. +// Status-aware POST (campaign F2 evidence gap): the agent's refusal body carries the reason +// (e.g. "…eject refused (role: system)") — surface it instead of a bare "HTTP 403". func (c *Client) EjectDisk(ctx context.Context, where string) (EjectResult, error) { var out EjectResult - body, err := c.post(ctx, "/disks/eject", map[string]string{"where": where}) + env, status, err := c.postWithStatus(ctx, "/disks/eject", map[string]string{"where": where}) if err != nil { return out, err } - if err := json.Unmarshal(body, &out); err != nil { + if err := refusalError("/disks/eject", status, env); err != nil { + return out, err + } + if err := json.Unmarshal(env.Data, &out); err != nil { return out, fmt.Errorf("agentapi: decode /disks/eject: %w", err) } return out, nil @@ -483,18 +488,48 @@ type DecommissionResult struct { // Decommission permanently removes a user-data drive (self-serve, non-destructive — the agent records // IntentDecommissioned, prunes the bind record, and unmounts; it NEVER formats). Data stays on the // drive. The agent role-gates to user-data and refuses a system/backup mount regardless. +// Status-aware POST (campaign F2 evidence gap): the agent's refusal body carries the reason +// (e.g. "…decommission refused (role: system)") — surface it instead of a bare "HTTP 403". func (c *Client) Decommission(ctx context.Context, where string) (DecommissionResult, error) { var out DecommissionResult - body, err := c.post(ctx, "/disks/decommission", map[string]string{"where": where}) + env, status, err := c.postWithStatus(ctx, "/disks/decommission", map[string]string{"where": where}) if err != nil { return out, err } - if err := json.Unmarshal(body, &out); err != nil { + if err := refusalError("/disks/decommission", status, env); err != nil { + return out, err + } + if err := json.Unmarshal(env.Data, &out); err != nil { return out, fmt.Errorf("agentapi: decode /disks/decommission: %w", err) } return out, nil } +// refusalError converts a non-2xx status or an ok:false envelope into an error that CARRIES the +// agent's reason (truncated; never request bodies or secrets). nil on an accepted 200/202+ok=true. +func refusalError(path string, status int, env apiResponse) error { + accepted := status == http.StatusOK || status == http.StatusAccepted + if accepted && env.OK { + return nil + } + reason := truncateErr(env.Error, 300) + if reason == "" { + reason = "(no reason in agent response)" + } + if accepted { // 2xx but ok:false — business refusal without an HTTP error code + return fmt.Errorf("agentapi: POST %s: %s", path, reason) + } + return fmt.Errorf("agentapi: POST %s: HTTP %d: %s", path, status, reason) +} + +// truncateErr mirrors stacks.truncateStr for agent refusal reasons. +func truncateErr(s string, maxLen int) string { + if len(s) <= maxLen { + return s + } + return s[:maxLen] + "..." +} + // FormatDisk asks the agent to format a device. The AGENT inspects the device and tiers it by ROLE // (its own classification, never the controller's claim): // - blank device → formatted. diff --git a/controller/internal/agentapi/refusal_test.go b/controller/internal/agentapi/refusal_test.go new file mode 100644 index 0000000..7455a63 --- /dev/null +++ b/controller/internal/agentapi/refusal_test.go @@ -0,0 +1,100 @@ +package agentapi + +import ( + "context" + "net/http" + "net/http/httptest" + "strings" + "testing" +) + +// Campaign F2 evidence gap: the agent's 403 refusal body carries the informative reason +// (agent disks.go handleDiskDecommission — "…decommission refused (role: X)"), but the old +// c.post discarded any non-2xx body, so operators saw a bare +// "agentapi: POST /disks/decommission: HTTP 403". These tests pin the reason surfacing. + +func refusalStub(t *testing.T) (*httptest.Server, string) { + t.Helper() + mux := http.NewServeMux() + mux.HandleFunc("POST /disks/decommission", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"ok":false,"error":"mount is system/backup-protected — decommission refused (role: system)"}`)) + }) + mux.HandleFunc("POST /disks/eject", func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusForbidden) + _, _ = w.Write([]byte(`{"ok":false,"error":"mount is system/backup-protected — eject refused (role: backup)"}`)) + }) + s := httptest.NewTLSServer(mux) + return s, strings.TrimPrefix(s.URL, "https://") +} + +// T-D1: Decommission surfaces the agent's refusal reason, not a bare HTTP code. +// RED-PROOF: on the pre-fix c.post shape the error is exactly +// "agentapi: POST /disks/decommission: HTTP 403" → the Contains assertion FAILS. +func TestDecommission_RefusalReasonSurfaced(t *testing.T) { + s, ep := refusalStub(t) + defer s.Close() + c := clientFor(t, s, ep) + + _, err := c.Decommission(context.Background(), "/mnt/sys_drive") + if err == nil { + t.Fatal("expected the 403 refusal to be an error") + } + if !strings.Contains(err.Error(), "(role: system)") { + t.Fatalf("agent refusal reason discarded — operator sees only: %v", err) + } + if !strings.Contains(err.Error(), "HTTP 403") { + t.Fatalf("HTTP status lost from the error: %v", err) + } +} + +// T-D2: same surfacing for EjectDisk. +func TestEjectDisk_RefusalReasonSurfaced(t *testing.T) { + s, ep := refusalStub(t) + defer s.Close() + c := clientFor(t, s, ep) + + _, err := c.EjectDisk(context.Background(), "/mnt/felhom-flash") + if err == nil { + t.Fatal("expected the 403 refusal to be an error") + } + if !strings.Contains(err.Error(), "(role: backup)") { + t.Fatalf("agent refusal reason discarded — operator sees only: %v", err) + } +} + +// T-D3: the success path is unchanged — a 200 envelope still decodes into the result struct. +// (EjectDisk success is covered by TestEject_Dependents; this pins Decommission.) +func TestDecommission_SuccessUnchanged(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("POST /disks/decommission", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"ok":true,"data":{"vmid":8200,"decommissioned":"/mnt/bulk","dependent_guests":[8200]}}`)) + }) + s := httptest.NewTLSServer(mux) + defer s.Close() + c := clientFor(t, s, strings.TrimPrefix(s.URL, "https://")) + + out, err := c.Decommission(context.Background(), "/mnt/bulk") + if err != nil { + t.Fatal(err) + } + if out.Decommissioned != "/mnt/bulk" || len(out.DependentGuests) != 1 { + t.Fatalf("success payload mis-decoded: %+v", out) + } +} + +// A 2xx envelope with ok:false (business refusal without an HTTP error) must also carry the reason. +func TestDecommission_OkFalseBusinessRefusal(t *testing.T) { + mux := http.NewServeMux() + mux.HandleFunc("POST /disks/decommission", func(w http.ResponseWriter, r *http.Request) { + _, _ = w.Write([]byte(`{"ok":false,"error":"drive is busy: unmount blocked by open files"}`)) + }) + s := httptest.NewTLSServer(mux) + defer s.Close() + c := clientFor(t, s, strings.TrimPrefix(s.URL, "https://")) + + _, err := c.Decommission(context.Background(), "/mnt/bulk") + if err == nil || !strings.Contains(err.Error(), "unmount blocked by open files") { + t.Fatalf("ok:false reason not surfaced: %v", err) + } +}