From c70cd70070b72dba2b320fe6decda397a9177756 Mon Sep 17 00:00:00 2001 From: Mike Swanson Date: Fri, 29 May 2026 17:24:53 -0700 Subject: [PATCH] feat(skills): add gc-audit skill for GuruConnect end-to-end audit Modeled on rmm-audit but adapted to GuruConnect's architecture: 7 passes (6 parallel + sequential CI/CD), protobuf 4-way wire-drift matrix, sqlx compile-time macros allowed (GC norm), Gitea Actions pipeline + deploy host checks, reconciles docs/FEATURE_ROADMAP.md + TECHNICAL_DEBT.md. Invoke via /gc-audit; --pass= for a single pass. Co-Authored-By: Claude Opus 4.8 (1M context) --- .claude/skills/gc-audit/SKILL.md | 627 +++++++++++++++++++++++++++++++ 1 file changed, 627 insertions(+) create mode 100644 .claude/skills/gc-audit/SKILL.md diff --git a/.claude/skills/gc-audit/SKILL.md b/.claude/skills/gc-audit/SKILL.md new file mode 100644 index 0000000..0ae7b3d --- /dev/null +++ b/.claude/skills/gc-audit/SKILL.md @@ -0,0 +1,627 @@ +--- +name: gc-audit +description: | + Periodic end-to-end verification of the GuruConnect codebase and CI/CD + infrastructure. Runs 6 parallel audit passes: (1) API/route & surface + inventory, (2) Rust code quality & standards, (3) TypeScript/dashboard + quality, (4) protocol & wire-format integrity (proto <-> prost <-> manual TS + decode), (5) security & remote-session integrity, (6) docs/roadmap + reconciliation. A 7th sequential pass audits CI/CD pipeline health (Gitea + Actions workflows, runner registration, clippy/audit gates, deploy host). + Produces a timestamped audit report and updates the living docs + (FEATURE_ROADMAP.md, TECHNICAL_DEBT.md). Takes 10-20 minutes. + + Invoke explicitly only — no auto-trigger. Use /gc-audit for a full audit. + Optional arg: --pass= to run a single pass + (api, rust, ts, protocol, security, docs, pipeline). + The docs pass reconciles FEATURE_ROADMAP.md / TECHNICAL_DEBT.md against the + code and cleans up stale entries. +--- + +# GuruConnect End-to-End Audit + +Periodic full-stack sanity check for **GuruConnect** — the standalone MSP remote-desktop +product (Rust/Axum relay server + Windows Rust agent + React/TS viewer component library, +Protobuf-over-WebSocket transport). Read-only by default — findings are written to a report +file and living docs are updated. No production code is changed. + +> **This is GuruConnect, not GuruRMM.** GC diverges from the RMM audit in ways that matter — +> do NOT copy RMM assumptions. The biggest traps, called out where they apply below: +> - **sqlx compile-time macros (`sqlx::query!` / `query_as!`) are the GC NORM and are allowed.** +> RMM bans them; GC does not. Do not flag them as violations. +> - **Wire format is Protobuf**, not RMM's JSON `AgentMessage`/`ServerMessage` enums. The +> integrity pass chases drift across four artifacts: `proto/guruconnect.proto` → +> prost-generated agent code → prost-generated server code → **hand-written binary decode in +> `dashboard/src/lib/protobuf.ts` (hardcoded message IDs)**. That manual TS decoder is GC's +> signature failure mode. +> - **The dashboard is a React component library + server-served static HTML** +> (`server/static/{login,dashboard,viewer}.html`), NOT a React Router SPA. There is no +> `App.tsx` route tree and no `dashboard/src/api/client.ts`. The surface pass adapts. +> - **CI is Gitea Actions** (`.gitea/workflows/*.yml`) with a Windows runner on Pluto — not +> RMM's `webhook-handler.py` + `build-shared.sh` pipeline. + +--- + +## Execution Overview + +``` +Phase 0: Context load (coordinator reads key files) +Phase 1: Spawn 6 parallel audit agents (codebase passes A-F) +Phase 2: Run CI/CD pipeline audit (Agent G — sequential; SSH to deploy host) +Phase 3: Collect findings, aggregate, score +Phase 4: Write report + update living docs +Phase 5: Present summary to user +``` + +The audit is orchestrated here (Claude coordinator). All codebase passes run in parallel +subagents. The pipeline pass runs sequentially after (it touches live state via SSH / HTTP). +Each agent returns structured findings; the coordinator aggregates and writes the final report. + +### Model (MANDATORY) + +**Always run every audit pass on Opus.** Spawn each agent with `model: "opus"` (the `opus` +alias resolves to the latest Opus). This overrides default complexity-based routing — do NOT +downgrade any pass to Sonnet or Haiku, even the lower-stakes ones (API surface, TypeScript). +The coordinator orchestration also runs on Opus. + +### Repo root + +All paths below are relative to `projects/msp-tools/guru-connect/` unless prefixed otherwise. +GuruConnect is a **separate Gitea repo** (`azcomputerguru/guru-connect`), not a submodule of +the monorepo in the RMM sense — it lives at `projects/msp-tools/guru-connect/` with its own +`.git`. Deploy host: **172.16.3.30:3002**, systemd service `guruconnect.service`. + +--- + +## Phase 0: Context Load (Coordinator Reads These) + +Before spawning agents, read these yourself: + +1. `CLAUDE.md` — GC project instructions, coding standards, ports, auth model +2. `docs/FEATURE_ROADMAP.md` — planned features (`[ ]`/`[~]`/`[x]` + P1-P3) +3. `TECHNICAL_DEBT.md` — living debt backlog + "Completed Items" section (repo root, NOT docs/) +4. `docs/ARCHITECTURE_DECISIONS.md` — ADR-001 (RMM↔GC contract), ADR-002 (release eng) +5. `docs/specs/SPEC-001-operational-tooling-parity.md` — release-engineering deliverables +6. `.claude/CODING_GUIDELINES.md` (repo root) — shared standards GC inherits + +Capture from `server/src/api/mod.rs` (+ `server/src/main.rs` route registration) the complete +route list — every `.route(...)` plus the two WebSocket upgrade endpoints in +`server/src/relay/mod.rs` (`agent_ws_handler`, `viewer_ws_handler`). This becomes the +**authority route list** passed to Agents A and E. + +Also extract every checkbox line from `FEATURE_ROADMAP.md` (with section + priority) into a +**roadmap claims list** — passed to Agent F for reconciliation against the code. + +--- + +## Phase 1: Parallel Audit Agents + +Spawn all six parallel agents (A, B, C, D, E, F) simultaneously in a single message. Each +agent receives the full context it needs inline — do not assume they share context. (Agent G — +Pipeline — runs sequentially afterward; it SSHes the live deploy host and hits HTTP endpoints.) + +--- + +### Agent A — API Surface & Route Inventory + +**Goal:** Find server endpoints with no UI/consumer exposure, handlers that are dead code, and +references in the dashboard/static HTML to routes that don't exist. + +**Instructions for agent:** + +1. Read `server/src/api/mod.rs` and `server/src/main.rs` — extract every `.route(path, method)` + into a list grouped by resource (auth, users, sessions, downloads, releases, changelog, + health, metrics). Include the two WS endpoints from `server/src/relay/mod.rs`. + +2. **The dashboard is NOT a routed SPA.** Instead cross-reference route usage against: + - `server/static/login.html`, `dashboard.html`, `viewer.html` — grep for `fetch(`, `/api/`, + and `ws`/`wss` URLs the static pages call. + - `dashboard/src/hooks/useRemoteSession.ts` — the WS connection URLs it opens. + - `dashboard/src/components/*.tsx` — any REST calls. + +3. Classify: + - Server route never referenced by any static page / component / hook → **ORPHANED ROUTE** + (dead code vs. intentionally external/integration — distinguish by context, e.g. routes the + RMM integration calls per ADR-001 are not dead). + - A `fetch`/WS URL in the frontend that matches no server route → **BROKEN CALL**. + - Method mismatch (frontend POSTs to a GET route) → **METHOD MISMATCH**. + +4. For each API resource group, assess whether the UI (static pages + viewer component) exposes + the major operations. Flag resources with full server CRUD but no UI surface. + +5. Return structured findings: `[SEVERITY] Description — file:line`. + +--- + +### Agent B — Rust Code Quality & Standards + +**Goal:** Find violations of GC's Rust standards and common quality issues across server + agent. + +**Instructions for agent:** + +Read `CLAUDE.md` (GC standards section) and `.claude/CODING_GUIDELINES.md` first. GC standards: +`tracing` for logging (not `println!`/`log`), `anyhow` in binaries, `thiserror` in libraries, +`async`/`await` preferred, clippy clean. + +**Compliance checks:** +- `.unwrap()` / `.expect()` outside `#[cfg(test)]` — panic in production. Flag each with context. + `.expect("invariant reason")` on a truly-impossible path is acceptable if the reason is clear. +- `todo!()` / `unimplemented!()` in non-test production paths. +- `println!` / `eprintln!` used for logging instead of `tracing::` macros. +- `format!()` used to build SQL strings (injection risk — parameterize instead). +- **DO NOT flag `sqlx::query!` / `sqlx::query_as!` compile-time macros.** They are the GC + convention (the codebase relies on compile-time query checking). This is the inverse of the + RMM rule — flagging them here is a false positive. + +**Auth coverage (server):** +- Read `server/src/api/mod.rs` + `server/src/auth/mod.rs`. Identify which route groups go through + the `AuthenticatedUser` extractor / auth middleware vs. which are public (login, health, + downloads, metrics may be intentionally public). +- Verify the **agent plane and viewer/admin plane are separated**: the agent WS + (`agent_ws_handler`, auth via `support_code` OR `api_key`) and the viewer WS + (`viewer_ws_handler`, auth via JWT) must not accept each other's credentials. Any non-public + admin route reachable without JWT → `[CRITICAL]`. +- Confirm JWT secret + min-length enforcement at startup (`server/src/main.rs`) and the logout + token blacklist (`server/src/auth/token_blacklist.rs`) is actually consulted on each request. + +**Logging hygiene:** +- Grep `tracing::` calls that include JWT secrets, API keys, support codes, passwords, or PII + (emails, usernames). These must be redacted. + +**Error handling:** +- HTTP handlers returning 500 with raw `e.to_string()` — leaks internals to callers. Log + server-side, return a generic message. + +**Search paths:** `server/src/` and `agent/src/`. Exclude generated proto (`OUT_DIR`) and +`server/src/db/mod.rs` re-exports. + +Return structured findings with file:line references. + +--- + +### Agent C — TypeScript / Dashboard Quality + +**Goal:** Find dashboard quality issues, with special focus on the hand-written protobuf decoder +(GC has no schema library on the TS side — correctness is manual). + +**Instructions for agent:** + +The dashboard (`dashboard/`) is a **React component library** (peer-dep React, no app router, +no bundler in-repo). Main artifacts: `components/RemoteViewer.tsx`, `components/SessionControls.tsx`, +`hooks/useRemoteSession.ts`, `lib/protobuf.ts`, `types/protocol.ts`. + +**TypeScript quality:** +- `any` annotations in `dashboard/src/` — each is a type-safety gap (the binary/canvas code is + exactly where `any` hides bugs). +- `@ts-ignore` / `@ts-expect-error` — note reason and whether still needed. +- `console.log` / `console.error` left in non-dev paths. +- Hardcoded server URLs instead of a passed `serverUrl` prop / config. + +**Manual protobuf decoder correctness (`lib/protobuf.ts`) — HIGH PRIORITY:** +- The decoder parses binary frames with `DataView` and **hardcoded message IDs** (e.g. + `MSG_VIDEO_FRAME`, `MSG_MOUSE_EVENT`, `MSG_KEY_EVENT`). Verify these IDs and field offsets + match `proto/guruconnect.proto` field numbers / wire layout. A mismatch silently corrupts + frames or events. (Agent D cross-checks the same surface from the proto side — coordinate via + the report; this side checks the TS implementation correctness.) +- Bounds checking: does the decoder validate buffer length before reading offsets? A short/hostile + frame should not throw an unhandled error or read OOB. + +**Component patterns:** +- `RemoteViewer.tsx` canvas rendering: BGRA→RGBA conversion correctness; is the canvas sized to + the frame? Are out-of-order frames (the `sequence` field) handled or dropped? +- `useRemoteSession.ts`: WebSocket lifecycle — reconnect handling, cleanup on unmount, error + surfacing (a dropped session should not silently hang). +- Icon-only buttons without `aria-label`/`title`; inputs without `