From 4e27f95333c999b4232b62e5f54057afbcd0f628 Mon Sep 17 00:00:00 2001 From: J3vb <192430104+J3vb@users.noreply.github.com> Date: Mon, 20 Jul 2026 10:48:13 +0200 Subject: [PATCH] docs(audit): record D9 LogAudit best-effort error-handling policy Add decision D9 (dated 2026-07-20) to the audit decisions doc capturing the maintainer-approved policy: best-effort audit writes, never silently discarded, routed through db.WriteAudit. Mark carried-over finding #10 in docs/audit-2026-07-19.md as RESOLVED with the helper adoption. Co-Authored-By: Claude Fable 5 --- docs/audit-2026-07-19.md | 2 +- docs/plans/audit-2026-07-19-decisions.md | 3 ++- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/docs/audit-2026-07-19.md b/docs/audit-2026-07-19.md index 8ee85dbc..cc842082 100644 --- a/docs/audit-2026-07-19.md +++ b/docs/audit-2026-07-19.md @@ -53,7 +53,7 @@ audit's table; details stay in [audit-2026-04-07.md](audit-2026-04-07.md). | 6 | HIGH | `Server/store/` untested | **RESOLVED 2026-07-19 (D3)** — the `store/` package is deleted rather than tested. `SQLiteStore` was a pure pass-through to `*db.DB`; its event/plugin methods moved into `db` (`event_queries.go`, `plugin_queries.go`). Consumers now depend on narrow interfaces `*db.DB` satisfies (`service.Store`, `ws.EventStore`, `plugin.PluginStore`), and the former `MemStore`-based unit tests run against a real in-memory SQLite `db` — so the code paths that were untested through the seam are now exercised directly | | 7 | HIGH | Client unit coverage | Suite is large (157 test files) but currently KNOWN RED and non-blocking — see A-2026-07-04 | | 9 | MEDIUM | auth_handler bypasses service layer | **Confirmed open** — `Server/api/router.go:101` passes `database *db.DB` to `MountAuthRoutes` while sibling mounts receive `svc` | -| 10 | MEDIUM | Audit-trail write failures silently ignored | **Fixed 2026-07-19** at the two flagged backup-handler sites (errors now logged). Wider scope discovered: the `_ = LogAudit` pattern exists at 23 call sites across admin/api/ws/service — appears to be a deliberate best-effort convention; policy decision tracked in the decisions doc (D8 note) | +| 10 | MEDIUM | Audit-trail write failures silently ignored | **RESOLVED 2026-07-20 (D9)** — best-effort audit writes are kept as the convention, but no longer silently discarded: every `LogAudit` call site now routes through the shared `db.WriteAudit` helper (`db/audit.go`), which logs a failed write with actor/action/target context without failing the request. All ~26 call sites across admin/api/ws/service converted from `_ = LogAudit(...)`; pinned by `db/audit_test.go` | | 11 | MEDIUM | E2E not in CI | **Confirmed open** — no Playwright job exists in `.github/workflows/ci.yml` | | W3-4 (remediation plan) | LOW | Contradictory upload cache header | **Fixed 2026-07-19** — now `private, no-cache` per the remediation plan's prescription | diff --git a/docs/plans/audit-2026-07-19-decisions.md b/docs/plans/audit-2026-07-19-decisions.md index 703d5832..e50dda89 100644 --- a/docs/plans/audit-2026-07-19-decisions.md +++ b/docs/plans/audit-2026-07-19-decisions.md @@ -2,7 +2,7 @@ **Date decided:** 2026-07-19 **Decided by:** J3vb -**Status:** decisions recorded; greenlit items (D4, D7, D8) implemented 2026-07-19 — see per-row Status +**Status:** decisions recorded; greenlit items (D4, D7, D8) implemented 2026-07-19; D9 (repo-wide `LogAudit` policy, flagged by the D8 note) decided + implemented 2026-07-20 — see per-row Status **Source:** decision points raised by [docs/audit-2026-07-19.md](../audit-2026-07-19.md) This document records the maintainer's answers to the open decision points from @@ -22,6 +22,7 @@ here (and the audit's closure table) as items land. | D6 | Abandoned SolidJS beachhead + stale `docs/client-architecture.md` | A-2026-07-12 | **Delete it all**: remove `src/components/solid/`, `solidMount`/`solidAdapter`, `vite-plugin-solid`, and Solid test deps; retire `client-architecture.md` in favor of [docs/architecture/client.md](../architecture/client.md). | **Implemented 2026-07-19** — solid/ dir, solidMount/solidAdapter, setup-solid tests, vite-plugin-solid, jsx tsconfig settings, and solid-js/@solidjs deps all removed; client-architecture.md is now a pointer. | | D7 | Spec refresh strategy for api.md / protocol.md / schema.md | A-2026-07-03 | **One refresh PR first**, using the audit's §2 conformance matrix as the checklist; afterwards specs are kept current per-PR (see the maintenance rule in [docs/architecture/README.md](../architecture/README.md)). Announcement channels (D1) later update the *fresh* specs. | **Implemented 2026-07-19** — all three specs refreshed against the code (incl. E2EE protocol section, migrations 001–015, profile/blocks/plugin-admin endpoints); reference tables now point at `protocol-schema.json`. | | D8 | What to implement first | backlog §6 | **Greenlit now: Protocol codegen (D4) + the quick-wins batch** — `LogAudit` error handling (`admin/handlers_backup.go`), contradictory upload `Cache-Control` (`upload_handler.go`), hub inline settings SQL through the data layer (`ws/hub.go`), Hub constructor cleanup (required collaborators into `NewHub`). | **Implemented 2026-07-19** (all four quick wins + D4). Hub cleanup shipped as: race fix — `eventPersister`/`eventStore`/`pluginSink` are now atomic (they were plain fields written by `main.go` after `NewRouter` had already started `Run`); remaining pre-Run setters now reject late calls with an error log instead of racing silently. Note discovered during the work: the discarded-`LogAudit` pattern is repo-wide (23 call sites) — the two tracker-flagged backup handlers are fixed; whether best-effort audit writes stay the convention elsewhere needs a policy decision. | +| D9 | Repo-wide `LogAudit` error-handling policy (the open question the D8 note flagged) | backlog §6 / D8 | **Decided 2026-07-20:** audit writes stay **best-effort** — a `LogAudit` failure must never fail or abort the request — but a failed write must never be silently discarded either. Every call site routes through one small shared helper that logs the failure with request context (actor/action/target). Not a blanket "make audit failures fatal": the request path is unchanged, only the silent `_ =` discard is removed. | **Implemented 2026-07-20:** `db.WriteAudit(auditor, actor, action, targetType, targetID, detail)` helper added in `db/audit.go` (structural `Auditor` interface — both `*db.DB` and the service-layer `Store` satisfy it, so api/admin/ws/service all reach it without an import cycle). All ~26 `LogAudit` call sites converted from `_ = LogAudit(...)` (and the two backup handlers' inline `if err` blocks) to `db.WriteAudit`; the `detail` string is deliberately not logged (may be sensitive). Pinned by `db/audit_test.go`: failure is logged and never propagated, success logs nothing, detail never leaks. | ## Suggested sequencing