From 292d14553238f40418d7b32c954c567e4eed4ae6 Mon Sep 17 00:00:00 2001 From: John Smith Date: Sun, 26 Jul 2026 00:23:15 -0400 Subject: [PATCH 01/23] remediation: Phase 1 config + Phase 2 quick fixes (hyperplan audit) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 1 — Config hygiene: - Pin vue/vue-router from 'latest' to concrete versions (server, desktop, base) - Dependabot npm interval: weekly → daily - Add fallow.txt/fallow.json to .gitignore - Remove redundant server/.editorconfig (subset of root) - Remove commented-out arktype generator in schema.prisma - Remove 7 commented-out code blocks across server/ - Fix CLAUDE.md:35 (pre-commit falsehood: says pnpm test, actual is typecheck) - Fix CLAUDE.md:79 (dead path reference to server/.husky/pre-commit) - Create fallow.toml with Nuxt path excludes - Switch CodeQL build-mode: none → autobuild for JS/TS + Rust Phase 2 — Correctness quick fixes: - Fix torrential download.rs:57 double unwrap chain (P0) - Fix torrential server/mod.rs:134 inner unwrap defeating error return (P0) - Fix Promise boolean at session/index.ts:195: add await (P1) - Fix 3 pre-existing type errors (settings InputEvent, IGDBID, igdb ratingCoverUrl) --- .github/dependabot.yml | 3 +- .github/workflows/codeql.yml | 4 +- .gitignore | 4 +- .omo/plans/remediation-plan.md | 241 ++++++++++ .omo/review/agent-hooks-audit.md | 336 ++++++++++++++ .omo/review/ci-cd-audit.md | 317 ++++++++++++++ .omo/review/code-quality-audit.md | 535 +++++++++++++++++++++++ .omo/review/cross-attack-high-effort.md | 363 +++++++++++++++ .omo/review/documentation-audit.md | 365 ++++++++++++++++ .omo/review/sonarqube-audit.md | 265 +++++++++++ .omo/review/test-coverage-audit.md | 378 ++++++++++++++++ AGENTS.md | 27 ++ CLAUDE.md | 8 +- desktop/main/package.json | 2 +- fallow.toml | 13 + libraries/base/package.json | 4 +- pnpm-lock.yaml | 371 ++++++++-------- server/.editorconfig | 10 - server/components/UserFooter.vue | 2 - server/package.json | 4 +- server/pages/account/security.vue | 1 - server/pages/admin/settings.vue | 5 - server/pages/admin/settings/index.vue | 4 +- server/pages/library/game/[id]/index.vue | 15 - server/pages/store/[id]/index.vue | 1 - server/prisma/schema.prisma | 9 - server/server/internal/metadata/igdb.ts | 6 +- server/server/internal/session/index.ts | 2 +- server/server/internal/tasks/index.ts | 26 -- server/server/plugins/ca.ts | 2 - torrential/src/downloads/download.rs | 5 +- torrential/src/server/mod.rs | 5 +- 32 files changed, 3056 insertions(+), 277 deletions(-) create mode 100644 .omo/plans/remediation-plan.md create mode 100644 .omo/review/agent-hooks-audit.md create mode 100644 .omo/review/ci-cd-audit.md create mode 100644 .omo/review/code-quality-audit.md create mode 100644 .omo/review/cross-attack-high-effort.md create mode 100644 .omo/review/documentation-audit.md create mode 100644 .omo/review/sonarqube-audit.md create mode 100644 .omo/review/test-coverage-audit.md create mode 100644 fallow.toml delete mode 100644 server/.editorconfig diff --git a/.github/dependabot.yml b/.github/dependabot.yml index bf4a60970..9befd953d 100644 --- a/.github/dependabot.yml +++ b/.github/dependabot.yml @@ -13,8 +13,7 @@ updates: - package-ecosystem: "npm" directory: "/" schedule: - interval: "weekly" - day: "monday" + interval: "daily" time: "06:00" timezone: "America/New_York" open-pull-requests-limit: 10 diff --git a/.github/workflows/codeql.yml b/.github/workflows/codeql.yml index a66ec4950..ecaa23fbc 100644 --- a/.github/workflows/codeql.yml +++ b/.github/workflows/codeql.yml @@ -48,9 +48,9 @@ jobs: - language: go build-mode: autobuild - language: javascript-typescript - build-mode: none + build-mode: autobuild - language: rust - build-mode: none + build-mode: autobuild # No Swift project in repo — remove entry. Re-add when Swift files need analysis. # - language: swift # build-mode: autobuild diff --git a/.gitignore b/.gitignore index deb5ce88f..2b34e68c7 100644 --- a/.gitignore +++ b/.gitignore @@ -1,3 +1,5 @@ dist/ node_modules/ -.omo/run-continuation/ \ No newline at end of file +.omo/run-continuation/ +fallow.txt +fallow.json \ No newline at end of file diff --git a/.omo/plans/remediation-plan.md b/.omo/plans/remediation-plan.md new file mode 100644 index 000000000..c3cfce631 --- /dev/null +++ b/.omo/plans/remediation-plan.md @@ -0,0 +1,241 @@ +# Drop Monorepo — Remediation Plan +**Source**: Hyperplan adversarial review (4 critics: low-effort, artistry, high-effort, ultrabrain — 3 rounds) +**Generated**: 2026-07-26 +**Scope**: Items with cross-critic adversarial consensus only (items without consensus dropped) + +--- + +## 1. Executive Summary + +Six audits produced 200+ findings across Drop's codebase, CI/CD, documentation, and tooling. After 3 rounds of adversarial cross-critique (independent analysis → cross-attack → defend/refine/concede), ~30 items survived consensus into this plan. The plan has 3 phases: **Foundation** (Day 1, ~6h — config-only, zero code logic changes, highest ROI), **Correctness** (Days 2-3, ~8h — production bugs, DB integrity), and **Structural** (Days 4-10, ~22h — CI, process gates, test infrastructure, Rust unsafety). Total: ~36 hours over 2 weeks for a solo developer. 4 P0 items (SonarQube exclusion, CodeQL autobuild, 2 torrential unwrap panics), 11 P1 items, 4 process changes, and 9 config hygiene items. 9 items deferred with explicit trigger conditions. + +--- + +## 2. Phasing Strategy + +### Phase 1 — Foundation (Day 1) +Config-only and zero code-logic changes. Every item is independent — all can run in parallel. Highest-ROI item in the entire plan (P0-1: 15 minutes to turn SonarQube gate green). No risk of regressions because no logic is touched. + +### Phase 2 — Correctness (Days 2-3) +Production runtime bugs + DB integrity. Torrential unwrap fixes (P0-3, P0-4) must be done sequentially (same crate). All other items (P1-1 through P1-5) are independent and can parallelize. These carry some regression risk — each fix has a verification gate. + +### Phase 3 — Structural (Days 4-10) +Heavier items: CI workflows for uncovered workspaces, dependency migrations, process gate setup, and the testing-trap-breaking reference test. Some items block others (PROC-3 must precede P1-4 verification; PROC-2 must precede soft-delete suppressions cleanup). + +--- + +## 3. Per-Phase Tasks + +### Phase 1 — Foundation (Day 1, ~6 hours) + +| ID | Task | Files | Effort | Verification | Dependencies | +|----|------|-------|--------|-------------|--------------| +| P0-1 | Exclude `prisma/migrations/` from SonarQube analysis | `sonar-project.properties` (create if absent) | 15m | Run SonarCloud analysis → 8 BLOCKERs disappear, QG turns GREEN | None | +| P0-2 | Switch CodeQL to `build-mode: autobuild` for JS/TS + Rust | `.github/workflows/codeql.yml` | 4h | Trigger CodeQL workflow on test PR → taint-tracking queries execute (verify in SARIF output) | None | +| PROC-1 | Create `fallow.toml` with Nuxt path excludes | New `fallow.toml` at repo root | 1h | Run `fallow audit --format json` → issue count drops from 671 to ~240 | None | +| PROC-5a | Fix CLAUDE.md:35 (pre-commit behavior falsehood) | `CLAUDE.md` | 10s | Line 35 accurately states `lint-staged + typecheck` (not `pnpm test`) | None | +| PROC-5b | Fix CLAUDE.md:79 (dead path reference) | `CLAUDE.md` | 10s | Line 79 no longer references `server/.husky/pre-commit` | None | +| CONF-1 | Pin `vue`/`vue-router` from `"latest"` to lockfile-resolved version | `server/package.json` | 30s | `pnpm ls vue` shows concrete version (e.g., `3.4.x`) | None | +| CONF-2 | Pin `vue-router` from `"latest"` | `desktop/main/package.json` | 30s | `pnpm ls vue-router` shows concrete version | None | +| CONF-3 | Add `fallow.txt` + `fallow.json` to `.gitignore` | `.gitignore` | 30s | `git status` no longer shows these files as untracked | None | +| CONF-4 | Remove `server/.editorconfig` (redundant subset of root) | `server/.editorconfig` | 3s | File deleted. `pnpm --filter drop lint` still passes | None | +| CONF-5 | Remove commented-out arktype generator | `server/prisma/schema.prisma` lines 13-20 | 30s | `pnpm --filter drop exec prisma validate` passes | None | +| CONF-6 | Remove 7 commented-out code blocks | See 7 files below | 15m | All 7 blocks removed. `pnpm --filter drop typecheck` passes | None | +| CONF-7 | Set Dependabot `schedule.interval: "daily"` for npm | `.github/dependabot.yml` | 2m | Dependabot config valid. Dependabot runs daily check on next cycle | None | +| CONF-8 | Generate root `CHANGELOG.md` | New `CHANGELOG.md` | 10s | File exists at repo root | None | +| CONF-9 | Create PR + issue templates | `.github/PULL_REQUEST_TEMPLATE.md`, `.github/ISSUE_TEMPLATE/bug.yml`, `.github/ISSUE_TEMPLATE/feature.yml` | 30m | 3 template files exist. `gh` recognizes them | None | + +**CONF-6 files**: `server/server/internal/tasks/index.ts:524-548`, `server/pages/library/game/[id]/index.vue:144-154`, `server/server/plugins/ca.ts:12`, `server/pages/admin/settings.vue:76-77`, `server/pages/account/security.vue:246`, `server/pages/store/[id]/index.vue:307`, `server/components/UserFooter.vue:134` + +--- + +### Phase 2 — Correctness (Days 2-3, ~8 hours) + +| ID | Task | Files | Effort | Verification | Dependencies | +|----|------|-------|--------|-------------|--------------| +| P0-3 | Fix `download.rs:57` double unwrap chain | `torrential/src/downloads/download.rs:57` | 30m | Add test with malformed config → expect `Err` not panic. `cargo test --manifest-path torrential/Cargo.toml` | None (independent) | +| P0-4 | Fix `server/mod.rs:134` inner unwrap defeating error return | `torrential/src/server/mod.rs:134` | 15m | Add test with invalid UTF-8 → expect graceful error, not panic. `cargo test` | None (independent; same crate as P0-3 but different module) | +| P1-1 | Fix Promise boolean at `session/index.ts:195` | `server/server/internal/session/index.ts:195` | 30m | Add unit test: mock `removeSession` to return false → assert `signout` returns false AND cookie NOT cleared. `pnpm --filter drop test` | None | +| P1-2 | Verify Prisma migration DELETE-without-WHERE status | `server/prisma/migrations/20251210231153_move_to_version_id/migration.sql:18` | 1h | SQL: `SELECT migration_name, finished_at FROM _prisma_migrations WHERE migration_name = '20251210231153_move_to_version_id'` | Requires production DB access | +| P1-3 | Add `@@index([userId])` on Client + Session, `@@index([expiresAt])` on Session | `server/prisma/schema.prisma` | 15m + migrate | `EXPLAIN ANALYZE` on user lookup query → index scan not seq scan. `prisma migrate dev --name add_user_session_indexes` | P1-2 (verify migration safety first) | +| P1-5 | Replace `console.log`/`console.error` with pino logger in 5 files | `oidc/index.ts` (×4), `error-handler.ts:3`, `webauthn/finish.post.ts:48`, `desktop/plugins/global-error-handler.ts:6`, `desktop/composables/game.ts:10` | 2h | Trigger an OIDC failure in dev → assert structured log entry, not bare stderr | None | + +--- + +### Phase 3 — Structural (Days 4-10, ~22 hours) + +| ID | Task | Files | Effort | Verification | Dependencies | +|----|------|-------|--------|-------------|--------------| +| P1-4 | Fix N+1 query in `objects.ts:149-161` | `server/server/internal/tasks/registry/objects.ts` | 2h | Run task with 100 objects × 5 models → Prisma query count ≤10 (was 500+) | None | +| P1-6 | Add CI for `sites/promo` + `sites/docs` | New `.github/workflows/sites-ci.yml` | 2h | Push PR touching `sites/promo/` → workflow runs typecheck + lint + build | None | +| P1-7 | Add CI for `desktop/main/` (path-filtered for stable files) | New `.github/workflows/desktop-main-ci.yml` | 2h | Push PR touching stable file in `desktop/main/` → workflow runs | Pre-req: audit `desktop/main/` to identify stable dirs | +| P1-8 | Migrate `jsonwebtoken` → `jose` | `server/package.json` + usage in `server/server/internal/auth/` | 2h | `npm ls jsonwebtoken` shows zero usages. `pnpm --filter drop test` passes | None | +| P1-9 | Pin 7 Tauri plugins from `"*"` to explicit versions | `desktop/src-tauri/Cargo.toml` | 30m | `cargo check` passes. `Cargo.lock` shows concrete versions for all 7 | None | +| P1-10 | Fix `@ts-ignore` with no reason (`users.ts:21, news.ts:35`) | `server/composables/users.ts:21`, `server/composables/news.ts:35` | 1h | No `@ts-ignore` remains without `@ts-expect-error` + documented reason | None | +| P1-11 | Fix recursive `read_block()` in libarchive → loop | `libraries/libarchive/src/reader.rs:63` | 1h | Add test with deeply nested archive → assert no stack overflow | None | +| PROC-2 | Narrow `drop/no-prisma-delete` to entity allowlist | `server/rules/no-prisma-delete.mts` | 1h | 6 join-table files no longer need eslint-disable. The 10 remaining suppressions remain but now have documented rationale | None | +| PROC-3 | Write ONE h3 factory reference test for auth route | New `server/test/unit/auth/route-template-reference.test.ts` | 4h | Test passes. A new developer can write the next auth route test by following the reference without inventing new infrastructure | None | +| PROC-4 | Add pre-commit Rust fmt + fallow gate | `.husky/pre-commit` | 1h | Make formatting error in `.rs` → pre-commit fails. Introduce new fallow finding → pre-commit fails | None | + +--- + +## 4. Execution Order (Solo Developer) + +### Day 1 — Foundation (6h) + +All items in Phase 1 are independent. Execute in any order. Suggested sequence for minimal context-switching: + +``` +CONF-3 (gitignore: 30s) +CONF-4 (remove editorconfig: 3s) +CONF-8 (changelog: 10s) +CONF-5 (remove arktype comment: 30s) +CONF-1 (pin vue server: 30s) +CONF-2 (pin vue desktop: 30s) +CONF-7 (dependabot daily: 2m) +PROC-5a (claude.md line 35: 10s) +PROC-5b (claude.md line 79: 10s) +CONF-6 (7 commented-out blocks: 15m) +CONF-9 (templates: 30m) +PROC-1 (fallow.toml: 1h) +P0-1 (sonarqube exclusion: 15m) +P0-2 (codeql autobuild: 4h) +``` + +Total: ~6 hours. All config/CI work — no production code touched. + +### Day 2 — Correctness I (3h) + +``` +P1-2 (verify migration status: 1h) — requires production DB +P0-3 (torrential download.rs: 30m) +P0-4 (torrential server/mod.rs: 15m) +P1-1 (Promise boolean: 30m) +``` + +P1-2 is in parallel with everything else (requires production access, no code change). P0-3 and P0-4 are in same crate but different modules — can be done sequentially in one session. + +### Day 3 — Correctness II (3h) + +``` +P1-3 (add prisma indexes: 15m + migrate) — blocks on P1-2 verification +P1-5 (structured logger: 2h) +``` + +P1-5 runs independently of everything else. + +### Day 5 — Structural I (6h) + +``` +P1-9 (tauri plugin pins: 30m) — independent +P1-11 (libarchive recursion → loop: 1h) — independent +P1-10 (@ts-ignore fixes: 1h) — independent +PROC-4 (pre-commit rust fmt + fallow gate: 1h) — independent +PROC-2 (narrow eslint rule: 1h) — independent +P1-6 (sites CI: 2h) — independent +``` + +All 6 items are independent. Can batch: 3 quick items (P1-9, P1-10, P1-11, PROC-4, PROC-2) then 2 larger ones (P1-6). + +### Day 10 — Structural II (8h) + +``` +P1-7 (desktop/main CI: 2h) — requires prereq audit of stable dirs +P1-8 (jsonwebtoken → jose: 2h) — independent +P1-4 (N+1 query fix: 2h) — independent +PROC-3 (reference test: 4h) — independent +``` + +PROC-3 is the heaviest single item (4h). Do it when fresh. P1-7 requires a directory audit first (30m). P1-8 and P1-4 are straightforward. + +--- + +## 5. Verification Gates + +### Gate 1 — End of Phase 1 +- [ ] `pnpm --filter drop typecheck && pnpm --filter drop lint` passes (CLAUDE.md changes don't affect code, config changes don't break anything) +- [ ] SonarCloud Quality Gate is GREEN (verify in SonarCloud dashboard) +- [ ] CodeQL workflow completes on test PR (verify in Actions tab) +- [ ] `fallow audit --format json` shows ~240 findings (was 671) +- [ ] All 14 config/template/changelog files exist in correct locations + +### Gate 2 — End of Phase 2 +- [ ] `cargo test --all-features --all && cargo clippy --all-targets --all-features -- -D warnings && cargo fmt --all -- --check` passes (torrential + libarchive) +- [ ] `pnpm --filter drop test` passes (Promise boolean fix + structured logger) +- [ ] `pnpm --filter drop typecheck` passes (no TS regressions) +- [ ] `SELECT migration_name, finished_at FROM _prisma_migrations WHERE migration_name = '20251210231153_move_to_version_id'` confirms migration status +- [ ] `EXPLAIN ANALYZE` on user lookup query shows index scan (P1-3) + +### Gate 3 — End of Phase 3 +- [ ] All 4 new CI workflows pass (sites-ci.yml, desktop-main-ci.yml) +- [ ] `npm ls jsonwebtoken` shows zero usages +- [ ] Pre-commit hooks work: formatting error → fails, fallow finding → fails +- [ ] N+1 query resolved: Prisma query count ≤10 for 100-object × 5-model test +- [ ] Reference test passes: auth route handler test covers sign-in flow +- [ ] Tauri plugins pinned: `Cargo.lock` shows concrete versions +- [ ] ESLint rule narrowed: 6 join-table suppressions removed + +--- + +## 6. Risk & Mitigation + +### Phase 1 Risks + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| SonarQube exclusion doesn't work on free tier | Medium | High (8 BLOCKERs persist) | Test with one exclusion first. Fallback: use `sonar.issue.ignore.multicriteria` to suppress specific rule IDs instead of path exclusion | +| CodeQL autobuild fails for Nuxt/Nitro project | Medium | Medium (P0-2 delayed) | Fall back to `build-mode: manual` with explicit `pnpm run build` step. Nuxt has documented CodeQL setup | +| `vue` pin breaks Nuxt compatibility | Low | Low | Pin to lockfile-resolved version (same version already installed). Nuxt peer-dep constrains to Vue 3.x | +| `fallow.toml` excludes too much, hiding real dead code | Low | Low | Start with conservative ignores (`.nuxt/`, `node_modules/`, `dist/` only). Add Nuxt routing dirs only after verifying remaining findings | + +### Phase 2 Risks + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| Torrential unwrap fix changes behavior | Low | Medium | Tests verify malformed config returns `Err`, not panic. No behavior change for valid configs | +| Promise boolean fix (adding `await`) changes signout behavior | Low | Medium | Verify intent first. If silent-swallow is intentional, add `await` + explicit error logging (not just `if (!await ...)`). The cookie-is-cleared behavior is correct either way | +| Migration DELETE-without-WHERE is unapplied in production | Low | Critical | Verify BEFORE any other migration work. If unapplied, manually review the 2025-12-10 migration's intent before applying | +| Adding indexes locks tables on large DB | Low | Low | PostgreSQL 12+ supports `CREATE INDEX CONCURRENTLY`. Add indexes with `CONCURRENTLY` flag in migration SQL | + +### Phase 3 Risks + +| Risk | Likelihood | Impact | Mitigation | +|------|-----------|--------|------------| +| `jsonwebtoken` → `jose` breaks existing JWT tokens | Medium | High | Token format differs between libraries. Must support CURRENT tokens (verify) while issuing NEW tokens via `jose`. Staged rollout: issue via `jose`, verify both `jsonwebtoken` AND `jose` can decode for one release cycle | +| Desktop/main CI on Nuxt 4 migration is perpetually red | Medium | Medium | Path-filter to exclude directories known to be in migration flux. Start with CI on `composables/`, `utils/`, config files only | +| N+1 query fix changes query semantics | Low | Medium | Verify: `findUnreferencedStrings` must return the SAME results. Compare output before and after fix on a test DB | +| Reference test (PROC-3) is too coupled to current auth implementation | Medium | Low | Use the h3 factory pattern (already proven in `test/utils/h3.ts`). If auth handler has too many dependencies, extract a pure function first | + +--- + +## 7. Out-of-Scope Confirmation + +These items were **dropped or deferred** by adversarial consensus: + +| Item | Trigger for Reassessment | Reason for Deferral | +|------|-------------------------|---------------------| +| **Coverage > 30%** (test-coverage-audit) | Coverage crosses 30% OR test infrastructure enables cost-effective testing | Matches AGENTS.md deferred work. 1.17% → 30% requires route refactoring (200-300h). Not feasible without breaking Cycle 1 first (PROC-3) | +| **`verbatimModuleSyntax` enable** | After 30+ latent errors from `noUncheckedIndexedAccess` are fixed | Codemod touches 100+ files; must be isolated PR, not mixed with fixes | +| **Full JSDoc/TSDoc sprint** | First external contributor submits PR against undocumented code | Zero JSDoc is emergent convention, not deliberate policy. Breaks at first contributor contact | +| **Soft-delete violations (10 entity suppressions)** | When actual soft-delete implementation is planned | 6 of 16 are join-table false positives (fixed in PROC-2). Remaining 10 require proper soft-delete implemention (~4h, deferred) | +| **Full route test suite (200-300h)** | After PROC-3 (reference test) and 3+ routes follow the template | Cannot refactor 100 routes without proven pattern | +| **`noUncheckedIndexedAccess` enable** | After 30+ latent TS errors fixed per AGENTS.md | 30+ errors tracked. Fix per-site before enabling globally | +| **husky v10 migration** | husky v10 actual release | Not yet released | +| **commitlint** | Team > 1 | Solo dev → zero value | +| **SonarCloud C rating fix** | Auth available to view findings | Cannot fix what cannot be seen | +| **P0-7 (no DB migration in release)** | N/A — FALSE POSITIVE | `launch.sh:5` runs `prisma migrate deploy` at container startup. Runtime-migration pattern is valid | +| **Codecov thresholds** | Coverage > 30% | At 1.17%, any threshold blocks every PR | +| **Architecture ADRs** | Second contributor onboarded | Single-dev consensus is implicit | + +--- + +## 8. Final Verification Checklist + +Before the remediation is declared complete: + +- [ ] **Phase 1 complete**: All 14 Foundation items done. SonarQube QG green. CodeQL does dataflow analysis. Fallow reports ~240 findings. CLAUDE.md accurate. All config hygiene applied. +- [ ] **Phase 2 complete**: Torrential has zero panic-on-malformed-input paths (verified by test). Session cleanup works correctly (verified by test). Prisma migration status verified. Proper indexes on Client + Session. Production errors go to structured logger. +- [ ] **Phase 3 complete**: All 4 new CI workflows exist and pass. `jsonwebtoken` fully migrated to `jose`. Tauri plugins pinned. N+1 query resolved. Testing trap broken (reference test exists). Pre-commit covers Rust + fallow. ESLint rule narrowed to entity allowlist. +- [ ] **Verification gates passed**: All verification steps in Section 5 pass. +- [ ] **No regressions**: `pnpm --filter drop test` passes (same or more tests than before). `pnpm --filter drop typecheck` passes. `cargo test --all-features --all` passes. +- [ ] **Deferred items documented**: DROP/DEFER items logged in AGENTS.md or equivalent tracking. diff --git a/.omo/review/agent-hooks-audit.md b/.omo/review/agent-hooks-audit.md new file mode 100644 index 000000000..33f27eb59 --- /dev/null +++ b/.omo/review/agent-hooks-audit.md @@ -0,0 +1,336 @@ +# Agent Hooks & Configuration Audit Report + +**Date:** 2026-07-25 +**Auditor:** Agent Hooks & Configuration Auditor (deep-audit-team) +**Repo:** Drop Monorepo (`/home/john/Projects/drop`) + +--- + +## 1. Complete Configuration Inventory + +### 1.1 Root Configuration Files + +| File | Path | Status | Purpose | +|------|------|--------|---------| +| `AGENTS.md` | `/AGENTS.md` | ✅ Present, 234 lines | Primary agent technical reference | +| `CLAUDE.md` | `/CLAUDE.md` | ✅ Present, 81 lines | Cross-tool behavioral rules (Claude Code, Cursor, Codex, OpenCode) | +| `fallow.txt` | `/fallow.txt` | ✅ Present (output) | Fallow audit output — **not a config file**, it's the rendered report | +| `fallow.json` | `/fallow.json` | ✅ Present | Fallow JSON output — **not a config file**, it's the serialized audit result (3.6.0, 671 issues) | +| `fallow.toml` | `/fallow.toml` | ❌ **MISSING** | Fallow configuration file — does not exist anywhere | +| `package.json` | `/package.json` | ✅ Present | Root package manager config | +| `.editorconfig` | `/.editorconfig` | ✅ Present, 32 lines | Root editor settings | + +### 1.2 `.opencode/` Directory (OpenCode Agent Runtime) + +| Path | Type | Status | +|------|------|--------| +| `.opencode/package.json` | File | ✅ Has `@opencode-ai/plugin@1.18.3` dependency | +| `.opencode/skills/test-runner/SKILL.md` | File | ✅ Project-level skill for running tests | +| `.opencode/plans/hyperplan-dep-tdd-coverage.md` | File | ✅ Detailed 8-PR execution plan | +| `.opencode/opencode.json` | File | ❌ **MISSING** — No OpenCode config file | +| `.opencode/opencode.jsonc` | File | ❌ **MISSING** — No OpenCode config file (comment variant) | +| `.opencode/agents/` | Dir | ❌ **MISSING** — No custom agent definitions | +| `.opencode/mcp.json` | File | ❌ **MISSING** — No MCP server configs | +| `.opencode/permissions.json` | File | ❌ **MISSING** — No permission rules | + +### 1.3 `.claude/` Directory (Claude Code Configuration) + +| Path | Type | Status | +|------|------|--------| +| `.claude/settings.json` | File | ✅ Present — PreToolUse hook for fallow gate | +| `.claude/hooks/fallow-gate.sh` | File | ✅ Present — 103-line bash script | +| `.claude/skills/generate-tests/SKILL.md` | File | ✅ Present — Java-focused test generation (186 lines) | +| `.claude/skills/generate-tests/rules/` | Dir | ✅ 25+ rule files covering Java unit tests, general test principles | +| `.claude/skills/generate-test-cases/SKILL.md` | File | ✅ Present — Test case listing only (134 lines) | +| `.claude/skills/generate-test-cases/rules/` | Dir | ✅ 14 rule files covering general test case strategy | + +### 1.4 Git Hooks (`.husky/`) + +| Path | Status | Content | +|------|--------|---------| +| `.husky/pre-commit` | ✅ **ACTIVE** | `pnpm --filter drop lint-staged && pnpm --filter drop typecheck` | +| `.husky/pre-push` | ✅ **ACTIVE** | `pnpm --filter drop test` | +| `.husky/_/commit-msg` | ✅ Present | Shell wrapper (sources `h`) | +| `.husky/_/husky.sh` | ⚠️ **DEPRECATED** | Shows husky v10 deprecation warning | +| `.husky/_/.gitignore` | ✅ Present | Ignores all files (husky internal pattern) | + +### 1.5 CI/Config Files (`.github/`) + +| File | Status | Notes | +|------|--------|-------| +| `.github/CODEOWNERS` | ✅ Present | 17 lines — BillyOutlast as sole owner | +| `.github/dependabot.yml` | ✅ Present | 159 lines — 7 ecosystems configured | +| `.github/workflows/ci.yml` | ✅ Present | Main CI | +| `.github/workflows/server-ci.yml` | ✅ Present | Server-only CI | +| `.github/workflows/cli-ci.yml` | ✅ Present | CLI CI | +| `.github/workflows/desktop-ci.yml` | ✅ Present | Desktop CI | +| `.github/workflows/droplet-ci.yml` | ✅ Present | Droplet lib CI | +| `.github/workflows/pages.yml` | ✅ Present | Promo + docs site builds | +| `.github/workflows/codeql.yml` | ✅ Present | Security scanning | +| `.github/workflows/osv-scanner.yml` | ✅ Present | Vulnerability scanning | +| `.github/workflows/editorconfig-ci.yml` | ✅ Present | Editorconfig enforcement | +| `.github/workflows/e2e.yml` | ✅ Present | E2E tests | +| `.github/workflows/stale.yml` | ✅ Present | Stale issue management | +| `.github/workflows/client-release.yml` | ✅ Present | Client releases | +| `.github/workflows/server-release.yml` | ✅ Present | Server releases | +| `.github/workflows/dependabot-auto-merge.yml` | ✅ Present | Auto-merge for dep PRs | +| `.github/scripts/` | ✅ Present | Helper scripts | + +### 1.6 Lint & Format Configs + +| File | Status | Notes | +|------|--------|-------| +| `.editorconfig` (root) | ✅ Present | 2-space indent, LF, UTF-8, .rs=4-space | +| `server/.editorconfig` | ✅ Present | Subset of root — LF, UTF-8, 2-space | +| `server/.prettierrc.json` | ✅ Present | JSON sort plugin | +| `server/eslint.config.mjs` | ✅ Present | Flat config, vue-i18n, custom no-prisma-delete rule | +| `libraries/base/eslint.config.js` | ✅ Present | Base library eslint | +| `sites/promo/eslint.config.mjs` | ✅ Present | Promo site eslint | +| `desktop/main/.prettierrc.json` | ✅ Present | Desktop prettier | +| `sites/promo/.prettierrc.json` | ✅ Present | Promo prettier | +| `sites/docs/.prettierrc.json` | ✅ Present | Docs prettier | +| `libraries/base/.prettierrc.json` | ✅ Present | Base lib prettier | +| Root `.prettierrc` | ❌ **MISSING** | No root prettier config (individual workspaces have their own) | +| `libraries/base/.editorconfig` | ✅ Present | Base lib editorconfig | + +### 1.7 MCP Server Configuration + +| Location | Status | +|----------|--------| +| Root MCP config | ❌ No MCP config file found anywhere in repo | +| `.opencode/` | ❌ No MCP configurations | + +**Note:** MCP servers appear to be configured externally (via OpenCode/Claude desktop configs or IDE-level settings), not in the repo itself. This is acceptable for agent tooling but means there's no portable, check-in-able MCP configuration. + +--- + +## 2. Fallow Configuration Analysis + +### 2.1 Fallow Configuration Files + +| Config File | Status | Verdict | +|-------------|--------|---------| +| `fallow.toml` | ❌ **MISSING** | No actual fallow configuration file exists | +| `fallow.txt` | ✅ Present | Rendered audit output (generated, not config) | +| `fallow.json` | ✅ Present | JSON audit result (generated, not config) | + +**ISSUE: `fallow.toml` does not exist.** The fallow audit output (`fallow.txt`) is present and shows fallow v3.6.0 analysis, but there is no configuration file to customize: + +- No `ignorePatterns` to suppress known false positives +- No `gate` configuration (defaults to `new-only`) +- No per-workspace configuration +- No entry point overrides (uses auto-detected 124 entry points) + +### 2.2 Fallow Rules In Effect (from AGENTS.md) + +The `AGENTS.md` references a fallow task map and gate: + +| Rule | Command | Enforced? | +|------|---------|-----------| +| Pre-commit/pre-push gate | `fallow audit --format json --quiet --explain --gate-marker agent` | ⚠️ Partially — embedded in AGENTS.md as instruction to human/agent | +| Dead code trace before deletion | `fallow dead-code --trace :` | ⚠️ Manual only | +| Dead dependency trace | `fallow dead-code --trace-dependency ` | ⚠️ Manual only | +| Health check before refactoring | `fallow health --hotspots --targets` | ⚠️ Manual only | +| Ownership check | `fallow health --ownership` | ⚠️ Manual only | +| Coverage gaps | `fallow health --coverage-gaps` | ⚠️ Manual only | +| Duplication trace | `fallow dupes --trace dup:` | ⚠️ Manual only | +| Flag detection | `fallow flags` | ⚠️ Manual only | +| Architecture guard check | `fallow guard ` | ⚠️ Manual only | +| Security surface scan | `fallow security` | ⚠️ Manual only | + +The fallow audit gate is embedded in `AGENTS.md` (via `` markers) AND in the `.claude/hooks/fallow-gate.sh` script, which fires on git commit/push in Claude Code sessions. However: + +- **No automated CI enforcement** — no `.github/workflows/` workflow runs fallow audit +- **OpenCode does NOT have a similar hook** — only Claude Code has the PreToolUse hook +- **The gate is advisory** — it runs as a Claude Code PreToolUse hook, not a pre-commit hook + +### 2.3 Fallow Issues Summary (from fallow.txt) + +| Category | Count | Key Concerns | +|----------|-------|-------------| +| Unused files | 385 | 64.3% dead files in server workspace | +| Unused exports | 126 | 21 exports + 105 in already-reported files | +| Unused type exports | 14 | 7 primary + 7 in dead files | +| Unused enum members | 4 | All in `desktop/main/types.ts` | +| Unused class members | 33 | Across objectHandler, session, OIDC, CA, metadata | +| Unused dependencies | 9 | Across desktop/main, sites/docs, sites/promo | +| Unused devDependencies | 1 | `eslint-config-next` in sites/promo | +| Unresolved imports | 44 | Prisma client imports failing resolution | +| Unlisted dependencies | 47 | Packages imported but not in package.json | +| Circular dependencies | 6 | Tasks index.ts (5 cycles) + library/index.ts | +| Duplicates | 124 clone groups | Significant code duplication across Vue components | +| Large functions | 168 total | 10 shown — 990-line template in Metadata.vue | +| High complexity | 227 findings | CRITICAL/HIGH in templates and TS backends | +| File health issues | 499 files | Most server files 100% dead with 0 fan-in | + +**Metrics:** 53,270 LOC · maintainability 79.7 (moderate) · 1 churn hotspot + +--- + +## 3. Agent Instruction Quality Assessment + +### 3.1 AGENTS.md Quality + +**Strengths:** +- ✅ Dense, technical, minimal fluff — matches the "caveman" style instructed +- ✅ Accurate workspace map with language/framework/entry points +- ✅ Clear pnpm policy with version pinning +- ✅ Nuxt double-nesting confusion point explicitly documented +- ✅ Commands table per workspace (build/test/lint) +- ✅ CI workflow map with file paths +- ✅ Pre-commit behavior documented with lint-staged patterns +- ✅ Gotchas section (libpng, tailwindcss recursion, noUncheckedIndexedAccess) +- ✅ Edit protocol (formatter commands) +- ✅ Test state snapshot with coverage baseline +- ✅ Deferred work backlog with triggers and rationale +- ✅ Self-verification instructions for out-of-date facts + +**Weaknesses:** +- ⚠️ 234 lines — exceeds the stated "keep under 150 lines" limit +- ⚠️ Skills section in AGENTS.md is unusual — skills are typically in SKILL.md files +- ⚠️ `` markers embed HTML comments in markdown — render clean but visually noisy +- ⚠️ Test state is outdated (mentions 32 tests from July 24, 2026) +- ⚠️ Deferred work was captured at PR #22 close-out but repo issues are disabled — no way to track + +### 3.2 CLAUDE.md Quality + +**Strengths:** +- ✅ Clear behavioral rules (format after edit, verify before completion) +- ✅ Explicit do-not-commit list +- ✅ Package manager enforcement +- ✅ Edit loop detection +- ✅ Cross-tool compatibility (4 agent platforms listed) + +**Weaknesses:** +- ⚠️ Line 35 states "pre-commit hook runs lint-staged + `pnpm test` automatically" — **INCORRECT.** The actual pre-commit hook runs `pnpm --filter drop lint-staged && pnpm --filter drop typecheck`, NOT tests. The pre-push hook runs tests. This is a factual error. +- ⚠️ Line 79 mentions `server/.husky/pre-commit` as dead code but git hooks live at root `.husky/` — this is confusing + +### 3.3 Conflict Analysis + +| Conflict | Files | Severity | +|----------|-------|----------| +| Pre-commit behavior | CLAUDE.md says "runs lint-staged + pnpm test", but `.husky/pre-commit` runs `lint-staged && typecheck` (no test) | **HIGH** — misleading agents | +| Formatter commands | AGENTS.md says `pnpm --filter drop exec prettier --write `; CLAUDE.md says same | ✅ Consistent | +| Package policy | Both say "ALWAYS pnpm" | ✅ Consistent | +| Skill instructions | AGENTS.md says use `npx openskills read`; OpenCode may use different mechanism | ⚠️ **MEDIUM** — skill invocation instructions differ | + +### 3.4 Outdated Instructions + +| Item | File | Issue | +|------|------|-------| +| Test count "32 vitest + 1 skipped" | AGENTS.md:159 | Snapshot from 2026-07-24, may be stale | +| Pre-commit hook behavior | CLAUDE.md:35 | Factually wrong (no test in pre-commit) | +| `server/.husky/pre-commit` dead | CLAUDE.md:79 | Confusing — no such file exists at that path | +| Coverage baseline 1.17% | AGENTS.md:168 | Snapshot value, may have changed | + +--- + +## 4. Hook Configuration and Gaps + +### 4.1 Active Hooks + +| Hook | File | Effect | Scope | +|------|------|--------|-------| +| pre-commit | `.husky/pre-commit` | `lint-staged` (format + eslint) + `typecheck` | **Server only** (pnpm --filter drop) | +| pre-push | `.husky/pre-push` | `pnpm --filter drop test` | **Server only** (pnpm --filter drop) | +| Claude PreToolUse (Bash) | `.claude/settings.json` + `.claude/hooks/fallow-gate.sh` | Blocks git commit/push if fallow audit fails | **Claude Code only** | + +### 4.2 Missing Hooks + +| Hook | Missing? | Impact | +|------|----------|--------| +| `commit-msg` | ✅ Present (but minimal — just sources `h`) | No commit message validation | +| **Rust pre-commit** | ❌ Missing | `cargo fmt` is covered by lint-staged for `*.rs` but `cargo clippy` is not run | +| **Desktop pre-commit** | ❌ Missing | No hooks for `desktop/src-tauri/` Rust code changes | +| **CLI pre-commit** | ❌ Missing | No hooks for `cli/` Rust code changes | +| **Sites pre-commit** | ❌ Missing | No hooks for `sites/promo/` or `sites/docs/` changes | +| **OpenCode hook** | ❌ Missing | No fallow gate or equivalent for OpenCode agent sessions | +| **CI-level fallow gate** | ❌ Missing | No GitHub Action runs fallow audit | + +### 4.3 Hook Quality Issues + +1. **Pre-commit only covers server workspace.** Running `pnpm --filter drop lint-staged` only lints the `server/` workspace. Rust workspaces (`cli/`, `desktop/src-tauri/`, `libraries/`) are not verified on commit. + +2. **No test on pre-commit.** Tests only run on pre-push, meaning an agent can commit code that breaks existing tests and only discover the failure when pushing. + +3. **CLAUDE.md contradiction.** States tests run in pre-commit, but they don't. This will cause agents to trust incorrect information. + +4. **Fallow gate only in Claude Code.** The `.claude/hooks/fallow-gate.sh` is tied to Claude Code's PreToolUse hook system. OpenCode, Cursor, and Codex agents do not have this protection. + +5. **husky v10 deprecation.** The `.husky/_/husky.sh` file shows a deprecation warning for husky v10. Current husky is v9.1.7 (from root `package.json`). When v10 releases, hooks may break. + +### 4.4 CODEOWNERS Analysis + +**Status:** ✅ Present, but minimal + +- Only `@BillyOutlast` as owner for all files +- Security-sensitive paths covered (auth, metadata, Nitro core) +- Agent config files covered (AGENTS.md, CLAUDE.md) +- NOT covered: desktop, CLI, libraries, sites/promo, sites/docs + +**Recommendation:** Add more granular ownership as the team grows. + +--- + +## 5. Specific Issues and Recommendations + +### 5.1 Critical Issues + +| # | Issue | Severity | Recommendation | +|---|-------|----------|---------------| +| 1 | CLAUDE.md says pre-commit runs tests — FALSE | **HIGH** | Fix CLAUDE.md:35 to match actual `.husky/pre-commit` behavior — lint-staged + typecheck, NOT test | +| 2 | No CI-level fallow enforcement | **HIGH** | Add a `.github/workflows/fallow-audit.yml` that runs `fallow audit` on PRs to develop/main. Gate on `--gate-marker agent` | +| 3 | Pre-commit only covers server workspace | **HIGH** | Expand pre-commit to run `cargo fmt --check && cargo clippy -- -D warnings` for changed Rust workspaces, or add per-workspace lint-staged configs | +| 4 | No MCP config in repo | **HIGH** | Consider adding `.opencode/mcp.json` or `.claude/mcp.json` to make MCP server configuration portable and reviewable | + +### 5.2 Medium Issues + +| # | Issue | Severity | Recommendation | +|---|-------|----------|---------------| +| 5 | No fallow.toml exists | **MEDIUM** | Create `fallow.toml` with ignore patterns for known false positives (e.g., Prisma client imports, Nuxt generated dirs) | +| 6 | Test assertion gap | **MEDIUM** | Move `pnpm --filter drop test` to pre-commit (replacing or supplementing pre-push). Tests take ~30s, not slow enough to justify pre-push-only | +| 7 | No commit-msg validation | **MEDIUM** | Add commitlint or a simple commit-msg hook when team grows >1. Documented in deferred backlog already | +| 8 | OpenCode not configured | **MEDIUM** | Create `.opencode/opencode.json` with skill paths, agent definitions, and hooks mirroring the Claude Code setup | +| 9 | Skill invocation divergence | **MEDIUM** | AGENTS.md says `npx openskills read` for skill loading but OpenCode/Claude use different systems. Standardize or document both | +| 10 | CLAUDE.md references dead path | **MEDIUM** | Remove `server/.husky/pre-commit` dead code reference — confusing and the file doesn't exist | + +### 5.3 Low Issues + +| # | Issue | Severity | Recommendation | +|---|-------|----------|---------------| +| 11 | AGENTS.md exceeds stated line limit | **LOW** | Trim or remove the deferred-work backlog and skills section to stay under 150 lines | +| 12 | Test state snapshot dated | **LOW** | Update test counts when tests change | +| 13 | husky v10 deprecation pending | **LOW** | When upgrading husky, migrate away from `.husky/_/husky.sh` pattern | +| 14 | CODEOWNERS incomplete | **LOW** | Add coverage for desktop, CLI, libraries, sites when contributors join | +| 15 | `server/.editorconfig` is subset of root | **LOW** | Consider removing redundant `.editorconfig` in favor of root-only | +| 16 | fallow.txt committed to repo | **LOW** | Add `fallow.txt` and `fallow.json` to `.gitignore` — they are generated output, not configuration | + +### 5.4 Positive Findings (Non-Issues) + +- ✅ Agent instructions are technically accurate and dense — good for AI agents +- ✅ CLAUDE.md and AGEMENTS.md serve different purposes (behavior vs. reference) — good separation +- ✅ Fallow pre-commit gate for Claude Code is well-implemented (fail-open on errors, version floor check, jq dependency check) +- ✅ Husky hooks are correctly wired (`.husky/pre-commit` is the active hook, `_/` directory is internal) +- ✅ Prettier and ESLint configs are modern (flat config, plugins, vue-i18n integration) +- ✅ Dependabot covers 7 ecosystems comprehensively +- ✅ 14 CI workflows provide good coverage for security, linting, building, and releasing +- ✅ CODEOWNERS correctly marks security-critical paths +- ✅ Custom ESLint rule (`no-prisma-delete`) properly enforces soft-delete policy +- ✅ The hyperplan in `.opencode/plans/` is detailed and well-structured (dependency graph, parallel waves, PR-by-PR breakdown, risk table, stop conditions) + +--- + +## 6. Summary + +| Area | Grade | Key Action | +|------|-------|------------| +| Agent instructions | B+ | Fix pre-commit falsehood in CLAUDE.md; trim AGENTS.md | +| Fallow configuration | D | Create `fallow.toml`; add CI-based gate | +| Git hooks | B- | Add per-workspace Rust hooks; move test to pre-commit | +| Claude Code config | B | Good hook implementation; needs OpenCode equivalent | +| OpenCode config | F | No `opencode.json`, no MCP config, no agents config | +| Lint/format | A | Modern flat configs, custom rules, good plugin support | +| CI/CD | A- | 14 workflows comprehensive; missing fallow gate | +| CODEOWNERS | C+ | Covers security paths; misses half the codebase | + +**Total issues found: 16 (4 critical, 6 medium, 6 low)** diff --git a/.omo/review/ci-cd-audit.md b/.omo/review/ci-cd-audit.md new file mode 100644 index 000000000..480ebccbd --- /dev/null +++ b/.omo/review/ci-cd-audit.md @@ -0,0 +1,317 @@ +# CI/CD & Automation Audit — Drop Monorepo + +**Date**: 2026-07-25 +**Auditor**: CI/CD & Automation Auditor + +--- + +## 1. Workflow Inventory + +### Core CI + +| Workflow | Triggers | Jobs | Caching | Notes | +|----------|----------|------|---------|-------| +| **ci.yml** | push/PR main, develop | validate (actionlint + risk register), dependency-review, secrets (gitleaks), typecheck, lint & format, test + coverage, SonarCloud, dockerfile lint (hadolint), shellcheck | pnpm cache | Main quality gate — runs on ALL changes to main/develop. Contains 10 jobs running in parallel. | +| **server-ci.yml** | push/PR develop — paths: server/**, libraries/base/** | typecheck, lint, test | pnpm cache | Narrower trigger than ci.yml. Redundant with ci.yml on develop. | +| **droplet-ci.yml** | push/PR develop — paths: libraries/droplet, droplet_types, libarchive | Build, Test, Lint (fmt, clippy, test, coverage, audit) | Rust cache (swatinem) | Covers Rust libraries. Has `workflow_dispatch`. | +| **desktop-ci.yml** | push/PR develop — paths: desktop/src-tauri/** | fmt, check (cargo check), test (continue-on-error), coverage, audit | Rust cache | No clippy — `cargo check` not `clippy`. Tests on `continue-on-error`. | +| **cli-ci.yml** | push/PR develop — paths: cli/** | fmt, clippy, test, coverage, audit | Rust cache | Has `workflow_dispatch`. | +| **e2e.yml** | push/PR develop — paths: server/** | Playwright E2E | pnpm cache | Single job, no matrix. Installs chromium only. | +| **editorconfig-ci.yml** | push/PR develop | editorconfig-checker | pnpm cache | Validates .editorconfig compliance. | + +### Security + +| Workflow | Triggers | Analysis | +|----------|----------|----------| +| **codeql.yml** | push/PR develop, schedule (weekly Sun) | CodeQL Advanced — 4 languages (actions, go, javascript-typescript, rust). Uses `build-mode: none` for most — no actual build analysis. | +| **osv-scanner.yml** | push/PR develop, merge_group, schedule (weekly Fri) | OSV-Scanner — recursive scan for known vulnerabilities. Reusable workflow. | + +### Release + +| Workflow | Triggers | Build & Deploy | +|----------|----------|----------------| +| **server-release.yml** | workflow_dispatch, release published, schedule (2 AM daily) | Multi-arch Docker build (linux/amd64 + linux/arm64) with digest + manifest merge. Pushes to ghcr.io/drop-oss/drop. Builds nightly + release images. SBOM + provenance enabled. | +| **client-release.yml** | workflow_dispatch (with tagName input), release published | Tauri build across 5 platforms (macOS arm64 + x64, Ubuntu 22.04 x64 + arm64, Windows x64). Apple code signing. Uploads to GitHub release. | + +### Automation + +| Workflow | Triggers | Purpose | +|----------|----------|---------| +| **pages.yml** | push develop — paths: sites/promo/**, sites/docs/** | Builds promo site (Next.js) + docs site (Astro) → nests docs at /docs → deploys to GitHub Pages. Concurrency group: "pages". | +| **dependabot-auto-merge.yml** | PR opened/synchronize/reopened | Auto-merges non-major npm Dependabot PRs after CI passes. Cargo + major npm require human review. | +| **stale.yml** | schedule (weekly Mon) | Closes issues after 90 days stale + 14 day grace. Exempts priority/p0, priority/p1. | + +--- + +## 2. Configuration Audit + +### Dependabot (`.github/dependabot.yml`) + +**Comprehensive coverage — 8 update streams:** +1. Root npm (weekly, grouped minor/patch, limit 10) +2. Desktop/main npm separate workspace (weekly, limit 5) +3. CLI cargo (weekly, limit 5) +4. Droplet cargo (weekly, limit 5) +5. native_model cargo (weekly, limit 5) +6. Desktop src-tauri cargo (weekly, limit 5) +7. Dockerfile root (weekly, limit 5) +8. GitHub Actions (weekly, grouped minor/patch, limit 10) + +**Strengths**: Groups reduce PR spam. Rebase strategy auto. Separate reviewer for root npm. Registry config for GitHub Packages. + +**Weaknesses**: No `schedule.interval: "daily"` for security-critical deps. All at `weekly` — vulnerability fixes sit for up to 7 days. + +### Pre-commit Hook (`.husky/pre-commit`) + +``` +pnpm --filter drop lint-staged && pnpm --filter drop typecheck +``` + +**Runs on**: `server/` workspace only (the `drop` package). Runs lint-staged (files staged in git) then typecheck. + +### Pre-push Hook (`.husky/pre-push`) + +``` +pnpm --filter drop test +``` + +**Runs on push**: Vitest tests for server workspace. + +**Gap**: No Rust workspace hooks — `cargo fmt --check` or `cargo clippy` not running pre-commit for Rust changes. + +### Lint-Staged (in `server/package.json`) + +| Pattern | Commands | +|---------|----------| +| `*.{ts,vue}` | eslint --fix, prettier --write | +| `*.{json,css,scss}` | prettier --write | +| `*.{yaml,yml,md}` | prettier --write | +| `*.{mjs,cjs}` | eslint --fix, prettier --write | +| `*.rs` | cargo fmt -- | + +**Note**: `.rs` rule exists but only fires when running inside `server/` workspace via `pnpm --filter drop lint-staged`. Rust files in `cli/`, `libraries/`, `desktop/src-tauri/` are NOT covered by pre-commit hooks. + +### Renovate + +Found only in `libraries/native_model/renovate.json` — independent config since native_model is a standalone external crate. Not part of main monorepo. + +### Risk Register (`security/risk-register.yaml`) + +13 entries (RISK-001 through RISK-013). Covered: decompress, lodash, SVGO, request, file-type, Hono/node-server (x2), uuid, Astro (x3), esbuild, Valibot. All have `review_by: 2025-10-24` dates. CI enforcement in `ci.yml` validates that every `pnpm audit --ignore GHSA-...` maps to a risk register entry. + +### CODEOWNERS + +Covers: security-sensitive auth routes, metadata providers, Nitro server core, build/CI config, AGENTS.md, CONTRIBUTING.md. All owned by @BillyOutlast. Single bus factor — no backup reviewer. + +--- + +## 3. Gaps & Missing Automation + +### Missing Workspace CI + +| Workspace | CI Exists? | Notes | +|-----------|-----------|-------| +| server/ | ✅ ci.yml, server-ci.yml, e2e.yml | Double coverage on develop | +| cli/ | ✅ cli-ci.yml | | +| desktop/src-tauri/ | ✅ desktop-ci.yml | No clippy | +| libraries/droplet, droplet_types, libarchive | ✅ droplet-ci.yml | | +| libraries/base (TS) | ❌ **MISSING** | Only triggered indirectly via server-ci.yml path filter. No dedicated workflow. | +| libraries/native_model | ❌ **MISSING** | External project with own CI — not run here. Acceptable. | +| sites/promo (Next.js) | ❌ **MISSING** | Only built in pages.yml (deploy). No typecheck/lint on PR. | +| sites/docs (Astro) | ❌ **MISSING** | Only built in pages.yml (deploy). No typecheck/lint on PR. | +| desktop/main/ (Nuxt 4) | ❌ **MISSING** | No CI whatsoever. Runs Nuxt 4 — could break silently. | + +### Missing Scheduled Jobs + +| Type | Status | Priority | +|------|--------|----------| +| Dependency updates | ✅ Dependabot (weekly) | — | +| Security scan (weekly) | ✅ CodeQL (Sun), OSV (Fri) | — | +| Secret scan (scheduled) | ❌ **MISSING** — gitleaks only runs on push/PR | Medium | +| Image vulnerability scan | ❌ **MISSING** — no Trivy or Grype scan on production images | High | +| License compliance scan | ❌ **MISSING** | Low | +| Docker image rebuild (nightly) | ✅ server-release.yml (2 AM) | — | +| Nightly desktop build | ❌ **MISSING** — client-release.yml schedule is commented out | Medium | +| Database migration checks | ❌ **MISSING** — no Prisma migration validation in CI | Medium | +| Coverage regression tracking | ❌ **MISSING** — coverage uploaded but no thresholds or badges | Low | +| E2E scheduled smoke test | ❌ **MISSING** — e2e.yml only runs on push/PR | Low | + +### Missing Deployment Automation + +| Deploy Target | Status | Priority | +|---------------|--------|----------| +| Production Docker image | ✅ server-release.yml | — | +| Desktop client release | ✅ client-release.yml | — | +| GitHub Pages (promo + docs) | ✅ pages.yml | — | +| Staging/preview environments | ❌ **MISSING** — no PR preview deployments | Medium | +| DB migration automation | ❌ **MISSING** — no `prisma migrate deploy` in release pipeline | High | +| Rollback mechanism | ❌ **MISSING** — no documented rollback strategy | Medium | + +### Missing Release Automation + +| Artifact | Status | +|----------|--------| +| Auto-generated changelog | ❌ No changelog generation | +| Semantic release | ❌ No semantic-release or release-please | +| Version bump automation | ❌ Manual version bumps in `server/package.json` | +| Git tag automation | ❌ Tags are manual — `server-release.yml` references `package.json` version | + +### Missing Templates + +| Template | Status | +|----------|--------| +| PR template | ❌ MISSING | +| Issue templates | ❌ MISSING | +| Bug report template | ❌ MISSING | +| Feature request template | ❌ MISSING | + +--- + +## 4. Performance Issues + +### CI Parallelism — GOOD + +- **ci.yml**: 10 parallel jobs — excellent parallelism. All jobs are independent. +- **server-ci.yml**: 3 parallel jobs — fine. +- Cross-workflow: All workspace CI workflows run independently — they can run in parallel. + +### Caching — MOSTLY GOOD + +- **pnpm**: `setup-node cache: pnpm` used everywhere. Good. +- **Rust (swatinem/rust-cache)**: Used in droplet-ci, desktop-ci, cli-ci, client-release. Good. +- **Next.js cache** in pages.yml: Caches `.next/cache` with hash of lockfile + source. Good. +- **Docker layer caching**: NOT used in server-release.yml. `build-push-action` pushes but doesn't use `cache-from`/`cache-to`. Could significantly speed up multi-arch builds. + +### Redundant Runs + +- **ci.yml + server-ci.yml overlap**: On develop push/PR to `server/**`, BOTH workflows run. server-ci.yml is a subset of ci.yml. This wastes 3-5 minutes of runner time. Consider removing server-ci.yml or making ci.yml the only full gate. + +### Wait Time + +- **cargo-install in CI**: droplet-ci, desktop-ci, cli-ci all run `cargo install cargo-llvm-cov --locked` and `cargo install cargo-audit --locked` per run. This takes ~2-3 minutes each. Consider pre-building a Docker image with these tools, or using `actions/cache` for cargo install binaries. +- **Ubuntu apt-get update every time**: Every Node.js job runs `sudo apt-get update && sudo apt-get install -y libpng-dev`. This is ~30s per job. Cache the apt sources or pre-build a runner image. + +### Matrix Strategy — GOOD + +- **server-release.yml**: Dual-platform matrix (amd64 + arm64). Good. +- **client-release.yml**: 5-platform matrix. Good. +- **codeql.yml**: Language matrix (4 languages). Good. + +--- + +## 5. Security Concerns + +### Current Security Measures (GOOD) + +- ✅ CodeQL Advanced — 4 languages +- ✅ OSV-Scanner — dependency vulnerability scan +- ✅ Gitleaks — secret scanning +- ✅ Dependency review action on PRs (fails on critical) +- ✅ Hadolint — Dockerfile linting +- ✅ Risk register — audit trail for ignored advisories +- ✅ SBOM + provenance in Docker builds +- ✅ `pnpm audit` in CI +- ✅ `cargo audit` (continue-on-error) in all Rust workflows + +### Issues + +1. **Gitleaks v2 — needs v3 migration** (documented in deferred work, pre-Sept 2026) +2. **cargo audit is `continue-on-error: true`** in ALL three Rust workflows. Fixes won't block CI. +3. **CodeQL `build-mode: none` for JavaScript/TypeScript** — no actual build analysis. This means CodeQL can't do dataflow analysis for JS/TS — only structural queries. Need build-mode: autobuild with proper setup. +4. **CodeQL `build-mode: none` for Rust** — same problem. Rust analysis without build can't track data flow. +5. **`pnpm audit` only runs on server** — not for desktop/main/ workspace, sites, or other npm workspaces. +6. **No scheduled gitleaks scan** — only on push/PR. An accidental secret merge between PR runs isn't caught. +7. **No container image vulnerability scanning** — builds push to ghcr.io but never scan with Trivy/Grype. +8. **`fail_ci_if_error: false` on ALL Codecov uploads** — coverage upload failures are silently ignored. + +--- + +## 6. Branch Protection + +No GitHub branch protection rules documented in the repo. No `CODE_OF_CONDUCT.md`. Based on workflows: +- `develop` has required CI checks (ci.yml blocks PRs) +- `main` presumably has stricter protection + +**Recommended protections for `main`**: +- Require status checks: ci.yml all jobs, codeql, osv-scanner +- Require PR review (at least 1) +- Require up-to-date branches +- Require signed commits +- No direct pushes +- Require CODEOWNERS review + +**Recommended protections for `develop`**: +- Require status checks: ci.yml, path-matched CI +- Require PR review +- Require up-to-date branches + +--- + +## 7. Recommendations (Priority Ordered) + +### Critical + +| # | Issue | Recommendation | +|---|-------|---------------| +| R1 | **No staging/preview environments** | Add PR preview deployment for server (e.g., ephemeral Docker or preview URLs) | +| R2 | **No DB migration in release** | Add `prisma migrate deploy` step to server-release.yml before image build | +| R3 | **Site workspaces have no CI** | Add `sites-ci.yml` with typecheck + lint for sites/promo and sites/docs on PRs | +| R4 | **desktop/main has no CI** | Add desktop-main-ci.yml with typecheck + lint for Nuxt 4 app | + +### High + +| # | Issue | Recommendation | +|---|-------|---------------| +| R5 | **ci.yml + server-ci.yml redundant** | Remove server-ci.yml or make ci.yml use path filters and skip server-ci.yml on server changes | +| R6 | **Docker builds have no cache** | Add `cache-from`/`cache-to` with `type=gha` in server-release.yml build step | +| R7 | **Nightly desktop build is commented out** | Uncomment and fix the scheduled trigger in client-release.yml | +| R8 | **CodeQL uses `build-mode: none` for JS/TS and Rust** | Switch to `build-mode: autobuild` or manual for proper data flow analysis. Add node setup for JS/TS. | +| R9 | **cargo-install per CI run** | Cache `~/.cargo/bin` or prebuild tool images for cargo-llvm-cov and cargo-audit | +| R10 | **No gitleaks scheduled scan** | Add `schedule` trigger to the secrets job or a standalone gitleaks scheduled workflow | +| R11 | **No container vulnerability scanning** | Add Trivy/Grype scan to server-release.yml after build | +| R12 | **Nuxt 4 desktop app has zero CI** | Add typecheck + lint for desktop/main/ | + +### Medium + +| # | Issue | Recommendation | +|---|-------|---------------| +| R13 | **cargo audit is continue-on-error everywhere** | After triaging existing vulns, switch to blocking on new findings | +| R14 | **Gitleaks v2 → v3** | Plan migration before Sept 2026 GitHub Node 20 deprecation | +| R15 | **Pre-commit only covers server/ workspace** | Add to root `.husky/pre-commit`: detect Rust changes and run `cargo fmt --check` | +| R16 | **No PR/issue templates** | Add `.github/pull_request_template.md` and `.github/ISSUE_TEMPLATE/` | +| R17 | **Changelog generation missing** | Add `release-please` or `git-cliff` for automatic changelog | +| R18 | **apt-get update on every job** | Cache apt packages or use custom runner image with libpng-dev pre-installed | +| R19 | **Dependabot weekly is too slow for security** | Set `schedule.interval: "daily"` for npm and cargo ecosystems | + +### Low + +| # | Issue | Recommendation | +|---|-------|---------------| +| R20 | **PR deployment previews** | Add deploy previews for docs site (Astro → Cloudflare Pages or Vercel) | +| R21 | **Rollback documentation** | Document rollback procedure for DB migrations and Docker deployments | +| R22 | **Codecov `fail_ci_if_error: false`** | Tighten after confirming Codecov works reliably | +| R23 | **Single bus factor in CODEOWNERS** | Add backup reviewer for CI/workflows | +| R24 | **No issue templates** | Add bug report + feature request templates | +| R25 | **No license compliance scan** | Add FOSSA or askalono for license compliance | + +--- + +## 8. Summary + +**Strengths**: +- Comprehensive workflow coverage for main server, CLI, desktop, and Rust libraries +- Multi-arch Docker builds with SBOM/provenance +- Good caching strategy (pnpm, Rust, Next.js) +- Strong security scanning (CodeQL, OSV, gitleaks, dependency review) +- Excellent risk register discipline with CI enforcement +- Dependabot covers all 8 ecosystems thoroughly +- Parallel job structure in all workflows + +**Critical Gaps**: +- `desktop/main/`, `sites/promo`, `sites/docs`, `libraries/base` have NO dedicated CI +- No database migration in release pipeline +- No staging/preview deployment environments +- Nightly desktop builds are commented out +- Redundant CI between ci.yml and server-ci.yml wasting runner time + +**Risk Level**: Medium — gaps exist in coverage for non-server workspaces, but the core server CI is solid. The biggest risk is unreviewed breakage in the Nuxt 4 desktop app and static sites. diff --git a/.omo/review/code-quality-audit.md b/.omo/review/code-quality-audit.md new file mode 100644 index 000000000..c20d52bf6 --- /dev/null +++ b/.omo/review/code-quality-audit.md @@ -0,0 +1,535 @@ +# Code Quality & Technical Debt Audit + +**Date:** 2026-07-25 +**Codebase:** Drop Monorepo (344 TS, 181 RS files) +**Auditor:** Code Quality Auditor + +--- + +## Technical Debt Score Estimate: **MODERATE (32/100)** + +| Category | Score | Weight | +|---|---|---| +| TypeScript anti-patterns | 35 | 20% | +| Rust anti-patterns | 40 | 20% | +| Error handling | 30 | 15% | +| Architecture & layering | 25 | 15% | +| Dependency health | 30 | 10% | +| Performance | 40 | 10% | +| Code organization | 35 | 10% | + +**Interpretation:** 32/100 (higher = cleaner). Codebase has pragmatic debt — suppressed type errors, unwrap-heavy Rust, and pattern bypasses are concentrated in specific areas, but the overall structure is sound. + +--- + +## 1. TypeScript Anti-Patterns + +### 1.1 `@ts-ignore` / `@ts-expect-error` // SEVERITY: HIGH + +**14 occurrences** across 9 files. Files with `@ts-ignore`: + +| File | Line | Excuse | Risk | +|---|---|---|---| +| `server/composables/users.ts` | 21 | "forget why this ignor exists" | HIGH — unknown suppression | +| `server/composables/request.ts` | 22, 32 | No comment | MEDIUM | +| `server/composables/request.ts` | 54 | "Excessive stack depth" | LOW — known TS limitation | +| `server/composables/news.ts` | 35 | "forget why this ignor exists" | HIGH — unknown suppression | +| `server/server/internal/services/torrential/index.ts` | 112 | No comment | MEDIUM | +| `server/server/internal/services/services/nginx.ts` | 20 | No comment | LOW — env var access | + +Files with `@ts-expect-error`: + +| File | Line | Comment | Risk | +|---|---|---|---| +| `server/composables/collection.ts` | 13, 36 | valid pattern | LOW | +| `server/server/internal/tasks/registry/objects.ts` | 68, 75, 132 | "im not dealing with this", "not typing this mess omg" | **HIGH** | +| `server/server/internal/saves/index.ts` | 54 | "Not sure how to get this to be typed" | MEDIUM | +| `server/server/api/v1/games/[id]/index.get.ts` | 82 | "value exists at runtime" | MEDIUM | + +**Recommendation:** Fix `users.ts:21` and `news.ts:35` unknowns ASAP. Refactor `objects.ts` to use proper Prisma types instead of dynamic reflection. + +### 1.2 `as any` Casts // SEVERITY: HIGH + +**11 occurrences** across 10 files. **Most concerning:** + +| File | Line | Context | +|---|---|---| +| `server/server/internal/services/torrential/droplet-interface.ts` | 228, 265 | Callback typecasting bypass | +| `server/server/internal/auth/index.ts` | 30 | Dynamic provider registration | +| `server/server/internal/auth/oidc/index.ts` | 486 | Prisma JSON type coercion | +| `server/server/api/v1/admin/library/index.get.ts` | 111 | Filter passthrough to Prisma — **injection surface** | +| `server/server/api/v1/user/mfa/webauthn/index.delete.ts` | 46 | Credential typecast | +| `desktop/main/composables/game.ts` | 37 | Event payload destructure | +| `server/nuxt.config.ts` | 89 | Tailwind plugin cast | +| `server/test/unit/acls/confused-deputy.test.ts` | 41 | Test — acceptable | + +**Recommendation:** `library/index.get.ts:111` is the highest risk — passing `filters as any` to Prisma `count()` bypasses type-checked where clauses. Use `Prisma.GameCountArgs` instead. + +### 1.3 `console.log`/`console.error` in Production // SEVERITY: MEDIUM + +**17 occurrences** in 11 files: + +- **`server/composables/task.ts`** (lines 41, 75) — console.log in composable used at runtime +- **`server/server/internal/session/db.ts`** (line 132) — commented-out console.log (code smell) +- **`server/server/internal/auth/oidc/index.ts`** (lines 156, 511, 517, 520) — console.warn/error with no structured logger +- **`server/plugins/error-handler.ts`** (line 3) — console.error instead of logger +- **`server/server/api/v1/user/mfa/webauthn/finish.post.ts`** (line 48) — console.error instead of logger +- **`desktop/main/plugins/global-error-handler.ts`** (line 6) — console.error +- **`desktop/main/composables/game.ts`** (line 10) — console.log in production composable +- **`server/nuxt.config.ts`** (lines 35, 293, 301) — build-time logging, acceptable +- **`server/i18n/scripts/rewrite-keys.ts`** (line 54), **`detect-keys.ts`** (line 26) — script-only, acceptable + +**Recommendation:** Replace runtime console calls with structured logger (pino). At minimum, `auth/oidc/index.ts`, `error-handler.ts`, `webauthn/finish.post.ts`. + +### 1.4 `no-explicit-any` Suppressions // SEVERITY: LOW + +**16 eslint-disable-next-line `@typescript-eslint/no-explicit-any`** across 12 files. Concentrated in: +- `server/server/internal/services/torrential/droplet-interface.ts` — 2 (generic callback infrastructure) +- `server/server/internal/session/types.d.ts` — 2 (type declaration files) +- `server/pages/admin/library/index.vue` — 2 (admin panel) +- `server/components/GameEditor/Metadata.vue` — 1 (990-line behemoth) +- `server/components/StoreView.vue` — 1 (524-line component) + +**Recommendation:** Acceptable for type decl files and generic infrastructure. The large Vue components (Metadata.vue, StoreView.vue) should be refactored to remove need for any. + +### 1.5 Commented-Out Code Blocks // SEVERITY: LOW + +**7 significant blocks:** + +| File | Lines | Content | +|---|---|---| +| `server/server/internal/tasks/index.ts` | 524-548 | Entire `msgWithTimestamp()` function commented out | +| `server/pages/library/game/[id]/index.vue` | 144-154 | Carousel navigation functions | +| `server/server/plugins/ca.ts` | 12 | `fsCertificateStore()` import | +| `server/pages/admin/settings.vue` | 76-77 | Notification composable | +| `server/pages/account/security.vue` | 246 | Auth fetch call | +| `server/pages/store/[id]/index.vue` | 307 | Rating calc | +| `server/components/UserFooter.vue` | 134 | API link | + +**Recommendation:** Remove dead code. Git history preserves it. + +--- + +## 2. Rust Anti-Patterns + +### 2.1 `.unwrap()` Usage // SEVERITY: CRITICAL + +**50+ calls** across 12 files. **Production code (non-test) offenders:** + +| File | Count | Risk | +|---|---|---| +| `torrential/src/downloads/download.rs` | 5 | Multiple unwrap chains — will panic | +| `torrential/src/server/mod.rs` | 3 | Message type unwrap | +| `torrential/src/droplet/mod.rs` | 1 | enum_value unwrap | +| `torrential/src/downloads/serve.rs` | 4 | Semaphore, header, cache unwrap | +| `torrential/src/downloads/handlers.rs` | 1 | Header parse unwrap | +| `torrential/src/conversions.rs` | 2 | TryInto unwrap | +| `torrential/build.rs` | 10 | Build script — acceptable | +| `desktop/src-tauri/tailscale/src/lib.rs` | 0 | Uses proper pattern matching | + +**Critical path:** `download.rs:57` — double unwrap chain: +```rust +let base_path = base_path.get("baseDir").unwrap().as_str().unwrap(); +``` +This panics if `baseDir` is missing or not a string. + +**Recommendation:** Replace with `context()` from anyhow or proper match/if-let. **torrential/ is the highest-priority target for unwrap removal.** + +### 2.2 `.expect()` Usage // SEVERITY: HIGH + +**30 calls** in 12 files. **Production offenders:** + +| File | Line | Message | +|---|---|---| +| `torrential/src/main.rs` | 29, 37, 73, 92, 101 | All startup — acceptable on fail | +| `torrential/src/server/mod.rs` | 60, 104 | Runtime operations | +| `torrential/src/downloads/serve.rs` | 68, 128 | Runtime semaphore operations | +| `libraries/droplet/src/versions/archive_backend.rs` | 138 | "file not found" | +| `cli/src/commands/connect/config.rs` | 44, 52, 67, 104 | Config operations | +| `libraries/droplet/src/manifest.rs` | 207 | Writer acquisition | + +**Recommendation:** `serve.rs:68` — `LazyLock::new(|| file_open_limit::get().expect(...))` panics on init if syscall fails. Use fallback value. + +### 2.3 `eprintln!()` for Error Logging // SEVERITY: MEDIUM + +**7 occurrences** in 2 files: + +| File | Line | Context | +|---|---|---| +| `desktop/src-tauri/tailscale/src/lib.rs` | 104, 114, 124 | Drop impls — acceptable (no logger available in Drop) | +| `libraries/droplet/tests/pipeline_test.rs` | 95, 190, 235, 282 | Tests — acceptable | + +**Recommendation:** Tailscale Drop impls are borderline acceptable — consider `log::error!` if logger is initialized before drop. + +### 2.4 `#[allow(dead_code)]` / `#[allow(clippy::*)]` // SEVERITY: MEDIUM + +**10 suppressions** across 7 files: + +| File | Line | Suppression | +|---|---|---| +| `desktop/src-tauri/process/src/process_handlers.rs` | 46 | `dead_code` | +| `desktop/src-tauri/process/src/process_handlers.rs` | 478, 479 | `unreachable_code`, `unused_variables` | +| `desktop/src-tauri/games/src/downloads/download_agent.rs` | 450 | `dead_code` | +| `desktop/src-tauri/games/src/downloads/download_logic.rs` | 31 | `clippy::too_many_arguments` | +| `desktop/src-tauri/download_manager/src/error.rs` | 42 | `dead_code` | +| `desktop/src-tauri/download_manager/src/util/queue.rs` | 14 | `dead_code` | +| `desktop/src-tauri/download_manager/src/download_manager_frontend.rs` | 95 | `dead_code` | +| `desktop/src-tauri/database/src/platform.rs` | 7 | `non_camel_case_types` | +| `desktop/src-tauri/cloud_saves/src/normalise.rs` | 57 | `clippy::single_element_loop` | + +**Recommendation:** `dead_code` suppressions indicate unused functions/enums. Either remove the code or add `#[expect(dead_code)]` (Rust 2024). `clippy::too_many_arguments` on `download_logic.rs:31` should be fixed by introducing a config struct. + +### 2.5 TODO/FIXME/HACK // SEVERITY: LOW + +Only **1 TODO** found: +- `torrential/src/droplet/manifest.rs:24` — "re-write the droplet interface so it takes a 'static reference instead of an arc" + +Low volume is good, but this remaining TODO is a correctness concern. + +--- + +## 3. Architecture Violations + +### 3.1 `drop/no-prisma-delete` Bypass // SEVERITY: HIGH + +**16 eslint suppressions** across 16 files — the soft-delete rule is systematically bypassed. + +| File | Line | Entity Deleted | +|---|---|---| +| `server/server/internal/screenshots/index.ts` | 59 | Screenshot | +| `server/server/internal/news/index.ts` | 132 | Article | +| `server/server/internal/clients/handler.ts` | 191 | Client | +| `server/server/internal/library/index.ts` | 654 | Library | +| `server/server/internal/tasks/registry/check-integrity.ts` | 79 | Integrity check | +| `server/server/api/v1/user/mfa/webauthn/index.delete.ts` | 35 | WebAuthn credential | +| `server/server/api/v1/user/mfa/webauthn/finish.post.ts` | 96 | WebAuthn credential | +| `server/server/api/v1/user/mfa/totp/start.post.ts` | 30 | TOTP secret | +| `server/server/api/v1/user/mfa/totp/finish.post.ts` | 47 | TOTP secret | +| `server/server/api/v1/auth/passkey/finish.post.ts` | 92 | Passkey | +| `server/server/api/v1/auth/mfa/webauthn/finish.post.ts` | 94 | WebAuthn | +| `server/server/api/v1/admin/users/[id]/index.delete.ts` | 30 | User | +| `server/server/api/v1/admin/company/[id]/game.post.ts` | 54 | Company-game relation | +| `server/server/api/v1/admin/company/[id]/game.patch.ts` | 27 | Company-game relation | +| `server/server/api/v1/admin/company/[id]/game.delete.ts` | 26 | Company-game relation | +| `server/server/api/v1/admin/game/[id]/tags.patch.ts` | 33 | Game-tag relation | + +**Recommendation:** For join-table deletions (company-game, game-tag), deletion is correct — no need for soft-delete on relations. For entities (screenshots, articles, clients), either implement soft-delete or update the eslint rule to allow deletion of non-core entities. + +### 3.2 TypeScript Strictness Gaps // SEVERITY: MEDIUM + +- `verbatimModuleSyntax: false` — disables a key TS 5.x best practice. Fixing this would require adding `type` imports everywhere. +- `noUncheckedIndexedAccess` — not enabled (30+ latent errors, tracked in AGENTS.md) +- `strictNullChecks: true` ✓ (good) +- `exactOptionalPropertyTypes: true` ✓ (good) + +**Recommendation:** Enable `verbatimModuleSyntax` with a codemod. Fix the `noUncheckedIndexedAccess` deferred items. + +### 3.3 Layer Violation Potential // SEVERITY: LOW + +The codebase generally respects layering (Nitro backend ↔ Nuxt frontend ↔ Prisma). The server uses `~/server/internal/` for business logic, which is correct. No direct DB calls from Vue components observed. + +**Minor concern:** `server/composables/task.ts` has a WebSocket connection utility that's shared between frontend and backend layers — could create coupling. + +--- + +## 4. Code Duplication + +### 4.1 Droplet Callback Processors // SEVERITY: MEDIUM + +`server/server/internal/services/torrential/droplet-interface.ts` (362 lines) has **8 nearly identical** callback processor definitions (lines 77-184). Each follows the same pattern: +```typescript +const XProcessor = this.defineDropletCallbackProcessor({ + queryType: DropBoundType.X, + callbackType: "x", + run: async (message, callbacks) => { + const messageData = fromBinary(XSchema, message.data); + callbacks.resolve(messageData.x); + this.callbacks.delete(message.messageId); + }, +}); +``` + +**Recommendation:** Create a factory function or data-driven configuration that reduces 8 definitions to 1 loop over a config array. + +### 4.2 `prisma.delete()` Pattern // SEVERITY: LOW + +16 files follow the same pattern — find-or-fail, then delete, then cleanup. This could be extracted into a generic `safeDelete(model, id, cleanupFn?)` utility. + +### 4.3 Metadata Provider Patterns // SEVERITY: LOW + +Providers (steam.ts: 1115, igdb.ts: 682, pcgamingwiki.ts: 504, giantbomb.ts: 426) share common HTTP fetch, caching, and image URL patterns. The metadata index (397 lines) already provides the chain — shared HTTP utilities would reduce duplication. + +--- + +## 5. Dependency Health + +### 5.1 Wildcard / Floating Dependencies // SEVERITY: HIGH + +| Dependency | File | Version | Risk | +|---|---|---|---| +| `vue` | server/package.json | `"latest"` | **CRITICAL** — breaks builds | +| `vue-router` | server/package.json | `"latest"` | **CRITICAL** — breaks builds | +| `wry` | desktop/Cargo.toml | `"*"` | HIGH — unpinned | +| `tauri-build` | desktop/Cargo.toml | `"*"` | HIGH — unpinned | +| `tauri-plugin-autostart` | desktop/Cargo.toml | `"*"` | HIGH | +| `tauri-plugin-deep-link` | desktop/Cargo.toml | `"*"` | HIGH | +| `tauri-plugin-dialog` | desktop/Cargo.toml | `"*"` | HIGH | +| `tauri-plugin-opener` | desktop/Cargo.toml | `"*"` | HIGH | +| `tauri-plugin-os` | desktop/Cargo.toml | `"*"` | HIGH | +| `tauri-plugin-shell` | desktop/Cargo.toml | `"*"` | HIGH | +| `libarchive-drop` | libraries/droplet/Cargo.toml | `"*"` | MEDIUM | +| `serde_json` | desktop/Cargo.toml | `"1"` (no minor) | LOW | +| `serde` | desktop/Cargo.toml | `"1"` (no minor) | LOW | +| `rustbreak` | desktop/Cargo.toml | `"2"` (no patch) | LOW | + +**Recommendation:** Pin `vue` and `vue-router` to explicit versions. Pin Tauri plugins to current compatible versions. Use `cargo upgrade` for workspace deps. + +### 5.2 Outdated Dependencies // SEVERITY: LOW + +| Dependency | Current | Notes | +|---|---|---| +| `otp-io` | `^1.2.7` | Low-maintenance OTP lib | +| `@lobomfz/prismark` | `0.0.3` | Early-stage, no updates | +| `libloading` | `0.7` (linux-only) | 0.8 available (not a direct risk) | + +### 5.3 Dependency Security // SEVERITY: LOW + +- `jsonwebtoken` with `jsonwebtoken` types — `jsonwebtoken` has known CVEs (CVE-2022-23529). The `jose` library is also present, suggesting partial migration. +- `argon2` — actively maintained, low risk. +- `tar` at `0.4.46` — symlink traversal, but only used for archive extraction. + +**Recommendation:** Complete migration from `jsonwebtoken` to `jose` (already in deps). + +--- + +## 6. Performance Anti-Patterns + +### 6.1 N+1 Query // SEVERITY: HIGH + +`server/server/internal/tasks/registry/objects.ts` — `findUnreferencedStrings()` (lines 149-161): +```typescript +for (const obj of objects) { + const isRef = await isReferencedInModelFields(obj, fieldRefMap); + // ... +} +``` +This issues **1 query per object per model** — O(n*m) database calls. If there are 1000 objects and 20 models, that's 20,000 queries. + +**Recommendation:** Batch-check using `IN` operator. Collect all object IDs into a single query per model: +```typescript +const found = await model.findMany({ + where: { OR: fields.map(f => ({ [f]: { in: objectIds } })) }, + select: { [fields[0]]: true }, +}); +``` + +### 6.2 Missing Indexes // SEVERITY: MEDIUM + +**Prisma schema analysis:** + +| Model | Missing | Impact | +|---|---|---| +| `Client` | `@@index([userId])` | All client lookups by user do full scan | +| `Session` | `@@index([userId])` | Session cleanup by user | +| `Notification` | `@@index([userId, read])` | Unread notification queries | +| `LinkedAuthMec` | `@@index([userId])` | Redundant with composite key, but fine | +| `GameVersion` | `@@index([gameId])` | Already covered by relation | +| `SaveSlot` | `@@index([userId])` | Already present ✓ | +| `Screenshot` | `@@index([gameId, userId])` | Already present ✓ | + +**Recommendation:** Add `@@index([userId])` to Client and Session models. + +### 6.3 Unbounded Queries // SEVERITY: MEDIUM + +- `objects.ts:22` — `objectHandler.listAll()` returns ALL objects with no pagination. For large deployments, this will OOM. +- `SaveSlot` stores full `historyObjectIds String[]` and `historyChecksums String[]` — no limit enforcement beyond `saveSlotHistoryLimit`. + +### 6.4 Recursive Read in Archive Reader // SEVERITY: LOW + +`libraries/libarchive/src/reader.rs:63`: +```rust +return self.read_block(); +``` +Recursive retry on null buffer — could stack overflow on repeated failures. Should use loop instead. + +--- + +## 7. Code Organization + +### 7.1 Large Files (Exceeds 500 Lines) // SEVERITY: MEDIUM + +| File | Lines | Issue | +|---|---|---| +| `server/components/GameEditor/Metadata.vue` | 990 | Mixed template/script/styles | +| `desktop/main/pages/library/[id]/index.vue` | 866 | Feature sprawl | +| `server/pages/admin/library/index.vue` | 805 | Complex admin panel | +| `server/server/internal/library/index.ts` | 712 | Monolithic service | +| `server/server/internal/metadata/steam.ts` | 1115 | Largest single file | +| `server/server/internal/metadata/igdb.ts` | 682 | Large provider | +| `desktop/src-tauri/process/src/process_manager.rs` | 644 | Large | +| `desktop/src-tauri/games/src/downloads/download_agent.rs` | 629 | Large | + +**Recommendation:** Set a 500-line soft limit. Break `steam.ts` into sub-modules (search, details, images). Extract `library/index.ts` into sub-services. + +### 7.2 Vue Component Complexity // SEVERITY: MEDIUM + +Several Vue components mix too many concerns: +- **`GameEditor/Metadata.vue`** (990 lines) — editor, preview, search, image management +- **`StoreView.vue`** (524 lines) — store grid, filtering, search, pagination +- **`admin/library/index.vue`** (805 lines) — CRUD, import, search, filtering, mass actions + +**Recommendation:** Extract composables for data fetching, separate presentational sub-components. + +### 7.3 Module Boundary Clarity // SEVERITY: LOW + +The `server/server/internal/` structure is well-organized by domain (auth, metadata, tasks, objects, library, etc.). Each domain has clear entry points. The metadata provider pattern with `PriorityListIndexed` is clean. + +**Minor:** The `screenshots` and `notifications` services are relatively thin wrappers — consider whether they justify separate service files. + +--- + +## 8. Prisma Schema Issues + +### 8.1 Missing Indexes // SEVERITY: MEDIUM + +```prisma +model Client { + id String @id @default(uuid()) + userId String + user User @relation(fields: [userId], references: [id], onDelete: Cascade) + // MISSING: @@index([userId]) +} +``` + +```prisma +model Session { + token String @id + expiresAt DateTime + userId String? + user User? @relation(fields: [userId], references: [id], onDelete: Cascade) + // MISSING: @@index([userId]) + // MISSING: @@index([expiresAt]) for cleanup +} +``` + +### 8.2 Schema Design Issues // SEVERITY: MEDIUM + +1. **`Notification.nonce` is optional with `@@unique([userId, nonce])`** — unique constraint fails if `nonce` is null, since NULL != NULL in PostgreSQL. Use a sentinel value instead. + +2. **`Task` model lacks defaults:** + ```prisma + model Task { + started DateTime // No @default(now()) + ended DateTime // No @default(now()) + } + ``` + +3. **`UnimportedGameVersion.fileList String[]`** — could grow unbounded. Consider a separate table. + +4. **`ObjectHash.hash String`** — no index on hash field for lookups. + +5. **`ApplicationSettings` uses `timestamp DateTime @id`** — each settings change creates a new row. Consider a single-row pattern with upsert instead. + +### 8.3 Commented-Out Generator // SEVERITY: LOW + +Lines 13-20 of `schema.prisma` have a commented-out `arktype` generator. Either uncomment and fix, or remove. + +--- + +## 9. Error Handling Patterns + +### 9.1 Structured Logging Gaps // SEVERITY: MEDIUM + +Server uses `pino` as logger, but: +- Auth OIDC handler uses `console.error` directly (4 occurrences) +- Error handler plugin uses `console.error` +- WebAuthn finish endpoint uses `console.error` + +**Recommendation:** Use `logger.error(...)` consistently. The OIDC handler imported logger exists but is bypassed. + +### 9.2 Rust Error Handling // SEVERITY: HIGH + +**Pattern:** `torrential/src/downloads/download.rs:57`: +```rust +let base_path = base_path.get("baseDir").unwrap().as_str().unwrap(); +``` +Two unwraps without context — panics on malformed config. Replace with: +```rust +let base_path = base_path + .get("baseDir") + .and_then(|v| v.as_str()) + .context("Missing baseDir in config")?; +``` + +**Pattern:** Tailscale Drop impls use `eprintln!` — acceptable because logger may not be initialized during Drop. + +**Pattern:** `torrential/src/server/mod.rs:134`: +```rust +Err(anyhow!(String::from_utf8(message.data.clone()).unwrap())) +``` +Inner unwrap defeats the error handling. Use `.map_err()` or `context()`. + +### 9.3 Frontend Error Handling // SEVERITY: LOW + +WebSocket composable (`composables/task.ts`) has minimal error handling — disconnection and reconnection logging uses console.log rather than user-facing notifications. + +--- + +## 10. Dead Code Inventory + +| Location | Type | Impact | +|---|---|---| +| `server/server/internal/tasks/index.ts:529-548` | Commented function | Low | +| `server/pages/library/game/[id]/index.vue:144-154` | Commented code | Low | +| `server/server/plugins/ca.ts:12` | Commented import | Low | +| `server/prisma/schema.prisma:13-20` | Commented generator | Low | +| `desktop/.../download_agent.rs` | 5 `#[allow(dead_code)]` markers | **High** — real dead code | +| `desktop/.../download_manager_frontend.rs` | `#[allow(dead_code)]` | **High** | +| `desktop/.../error.rs` | `#[allow(dead_code)]` | **Medium** | +| `desktop/.../queue.rs` | `#[allow(dead_code)]` | **Medium** | +| `desktop/.../process_handlers.rs` | `#[allow(dead_code)]` | **Medium** | + +**Recommendation:** Run `cargo clippy -- -D warnings` on desktop workspace and fix or remove dead_code items. + +--- + +## Remediation Priority Matrix + +| Priority | Area | Effort | Impact | +|---|---|---|---| +| **P0** | Fix `.unwrap()` chains in torrential (6 files) | 1 day | Prevents production panics | +| **P0** | Pin `vue`/`vue-router` from `"latest"` | 5 min | Prevents build breakage | +| **P1** | Fix `objects.ts` N+1 query | 2 hours | Prevents DB meltdown at scale | +| **P1** | Add missing `@@index([userId])` on Client/Session | 10 min | Query perf | +| **P1** | Fix `library/index.get.ts` `as any` filter | 1 hour | Security | +| **P1** | Replace `console.error` with logger (3 files) | 2 hours | Observability | +| **P2** | Refactor droplet-interface.ts callback duplication | 3 hours | Maintainability | +| **P2** | Fix `Notification.nonce` nullable unique | 1 hour | Data integrity | +| **P2** | Pin Tauri plugin `*` deps | 1 hour | Reproducible builds | +| **P2** | Remove dead_code items in desktop Rust | 2 hours | Cleanliness | +| **P3** | Fix `@ts-ignore` unknowns in users.ts, news.ts | 1 hour | Type safety | +| **P3** | Break up steam.ts (1115 lines) | 4 hours | Maintainability | +| **P3** | Fix recursive `read_block()` in libarchive | 1 hour | Stack safety | + +--- + +## Summary + +**Strengths:** +- Good overall architecture with clear domain separation +- No circular dependencies detected +- Strong Prisma schema with proper relations and cascades +- Low TODO/FIXME density (only 1 found) +- Consistent use of metadata provider pattern +- TypeScript strict mode partially enabled + +**Weaknesses:** +- **torrential** Rust code has pervasive unwrap/expect — highest risk area +- 16 `drop/no-prisma-delete` suppressions indicate policy is not enforced +- Pin dependencies from wildcards (`"latest"`, `"*"`, `"1"`) +- `objects.ts` cleanup task will cause N+1 DB meltdown at scale +- Missing indexes on Client and Session tables +- console.error bypasses structured logging in 5 runtime files +- 3 Vue components exceed 750 lines +- Commented-out code blocks scattered across codebase +- Recursive call in libarchive reader risk stack overflow diff --git a/.omo/review/cross-attack-high-effort.md b/.omo/review/cross-attack-high-effort.md new file mode 100644 index 000000000..4649d2665 --- /dev/null +++ b/.omo/review/cross-attack-high-effort.md @@ -0,0 +1,363 @@ +# Cross-Attack Report: High-Effort Critic (Round 2) + +**Position:** Rigorous, effort realist, prioritization hard-liner +**Targets:** critic-low (in-team), critic-artistry (ses_0639a2f96ffemN7u63TfNc4rQJ), critic-ultrabrain (ses_0639a0d52ffe1Z57n6hC7w7HSh) +**Verified against:** actual source code, workflow files, audit content (fact-checked inline) + +--- + +## A: Counter-Attacks on critic-low + +*critic-low reported: 13 lowest-effort wins, 14 false positives, 8 redundant clusters, 5 mutually exclusive pairs, 10 hard-to-justify items, 5-day sequencing* + +### A1: "13 lowest-effort wins" — effort inflation detected + +**Claim:** 13 items can be fixed in minimal time. + +**Counter:** +- Several of these "easy wins" have hidden dependency chains. Pinning `vue`/`vue-router` from `"latest"` affects **both** `server/package.json` AND `desktop/main/package.json` (verified: desktop/main line 30 also has `"vue-router": "latest"`). Pinning one without the other leaves the desktop client equally exposed. +- CLAUDE.md line 35 fix ("pre-commit runs test" → "pre-commit runs typecheck") seems trivial (10s) but is actually coupled to CONTRIBUTING.md accuracy and the broader agent-documentation gap. A partial fix (just line 35) leaves the broader agent/docs drift unaddressed. +- Adding `@@index([userId])` to Prisma schema (listed as 10-min fix) requires: edit schema, generate migration, run migration, verify no downtime impact on live DB. On a production database with 100K+ session rows, adding an index takes Postgres exclusive lock. This is not "10 minutes" for a solo dev managing a live instance. + +**Verdict:** 3 of 13 "easy wins" are deceptive. True quick hits: Promise await fix (line 195), `cargo fmt` dead-code review, Prisma arktype generator comment removal. ~10 of 13 are genuine. + +### A2: "14 false positives" — aggressive dismissal + +**Claim:** 14 audit findings are false positives. + +**Counter:** +- The `@ts-ignore` in `users.ts:21` where the dev wrote "forget why this ignor exists" is **not** a false positive. It's a time bomb. When TypeScript's type resolution changes (strict mode, dep bump), that suppressed error materializes as a runtime bug. The comment itself proves the dev doesn't know the risk. This is a real finding, not a false positive. +- The `console.error` in OIDC auth (code-quality §1.3) — critic-low likely calls this "dev artifact, works in prod." Counter: OIDC auth failures in Docker land in stdout/stderr streams that Docker doesn't persist. If someone brute-forces the OIDC provider or has a setup error, the admin has zero signal. Missing structured logging in auth = blind incident response. +- The `ApplicationSettings` timestamp-as-id (every settings update adds a row) — likely dismissed as "works fine." Counter: game distribution instances can accumulate settings rows on every admin action. No cleanup mechanism. Over years, this is unbounded table growth. The Postgres autovacuum won't help with dead rows that aren't actually dead. +- The `Notification.nonce` nullable unique constraint with `@@unique([userId, nonce])` — nullable + unique in Postgres allows multiple NULLs but the semantic intent was likely "nonce must be unique per user." If the column has NULLs and the code assumes uniqueness, you get duplicate notifications. + +**Verdict:** At least 5 of "14 false positives" are real findings being dismissed. Particularly egregious: the `@ts-ignore` comments where the AUTHOR says they don't know why. + +### A3: "8 redundant clusters" — misses nuanced differences + +**Counter:** The "redundant" items in CI configuration (multiple `fail_ci_if_error: false` across 4 files, multiple `continue-on-error: true`) are **separate configuration drift** — they need to be evaluated independently. The fact that they look similar doesn't make them redundant. Each file could be independently modified, and fixing one doesn't fix the others. The pattern IS the finding: 4 CI workflows all have the same misconfiguration, which suggests a copy-paste error that spread. + +### A4: "5 mutually exclusive pairs" — false conflicts + +**Counter:** Critic-low likely claims items like "desktop/main CI" and "nuxt 4 migration completion" are mutually exclusive. They're not — you can add CI that runs `nuxt typecheck` and ignores failures on known-error files via a path filter (as ultrabrain suggested in Leverage 4). That's not mutual exclusion; it's phased rollout. + +### A5: "10 hard-to-justify items" — justification exists + +**Counter:** Items like "remove 7 commented-out code blocks" (code-quality §1.5) are NOT hard to justify. They're in `tasks/index.ts:524-548` — a ~25-line commented-out function. The justification: the function either does something (uncomment it) or doesn't (delete it). Keeping it commented is the worst option: it confuses readers, it won't compile-stay-current. + +### A6: "5-day sequencing" — misses external dependencies + +**Counter:** A 5-day sequencing plan assumes all work happens consecutively on a single machine. Real bottlenecks: +- Cargo audit triage (P1-5) requires understanding each vulnerability's reachability — this could take 2+ days if the project has 20+ dependency trees across 7 Rust workspaces +- Prisma index migration requires DB schema locks — can't batch all index changes into one migration without careful ordering +- SonarQube path exclusion for Prisma migrations requires understanding whether SonarCloud free tier supports it — could be blocked on support answer + +**Verdict:** The 5-day sequencing is optimistic by 40-60%. True sequential-critical path is closer to 7-9 days of wall-clock time accounting for real-world blockers. + +--- + +## B: Counter-Attacks on critic-artistry + +### B1: "Promise trap is 'most impactful fix'" — overclaim + +**Artistry Claim (§A, Lever #1):** `session/index.ts:195` Promise in boolean is "The audits' single most impactful fix" and a "runtime auth bypass vector." + +**Counter:** +- This is factually wrong. The code at session/index.ts:192-198: + ```ts + async signout(h3: H3Event) { + const token = this.getSessionToken(h3); + if (!token) return false; + if (!this.signoutByToken(token)) return false; // BUG: Promise always truthy + deleteCookie(h3, dropTokenCookieName); + return true; + } + ``` + Because `!Promise` is always `false`, the `if` **never triggers**, meaning `deleteCookie` ALWAYS runs and the function ALWAYS returns `true`. The cookie IS cleared. The user IS logged out. The bug is that `removeSession()` failure is silently swallowed — the DB row persists but the user gets logged out anyway. + + **This is NOT an auth bypass.** The user cannot stay authenticated after this function runs. It's a session cleanup failure (orphaned rows, blind ops), which I correctly classified as P0-4 but NOT as an "auth bypass." + + Fix is indeed 1 line (`await this.signoutByToken(token)`), but calling it "the single most impactful fix" ignores that: + - A torrential panic in production crashes the server (bigger impact) + - A migration DELETE without WHERE can destroy data (bigger impact) + - A release pipeline without `prisma migrate deploy` can cause a total outage (bigger impact) + +**Verdict:** Overhyped. Correctly identified bug, wrong severity framing. + +### B2: "One canonical API route test unlocks everything" — unverified ROI + +**Artistry Claim (§A, Lever #2):** Writing ONE reference auth route test "changes the psychology" and makes marginal cost of remaining 99 routes "hours to minutes." + +**Counter:** +- This assumes all 100 routes have similar complexity. They don't: + - Auth routes (12): simple request/response patterns, cheap to test + - Admin routes (66): complex authorization chains, file uploads, multi-step mutations. Each test requires fixture data for models, user roles, permissions + - Client routes (15): native h3 event expectations, different auth context +- The reference test works for auth routes but gives diminishing returns for admin routes where each endpoint has unique validation logic +- The h3 factory in `test/utils/h3.ts` is already built but unused — the constraint isn't "no template," it's **no time to refactor 100 routes**. A reference test doesn't reduce the 2-3 hours per route if the actual work is extracting the handler from the closure, not copying test boilerplate + +**Verdict:** The "reference test" idea is good but oversold by 5-10x. Real ROI: reference test saves ~15 min per route (template boilerplate). For 100 routes, ~25 hours saved from ~200-300 total hours. Useful, not transformative. + +### B3: "Solid Ground" release scope — undercounts effort + +**Artistry Claim (§G):** "~60 issues collapsed into a weekend's work." + +**Counter:** Let's audit the 15 items listed and add realistic effort: + +| Listed Item | Claimed Effort | Real Effort | Gap Source | +|---|---|---|---| +| Promise fix | 5s | 5s + 30min code review to verify intended behavior | Must verify `signoutByToken` isn't being called elsewhere unawaited (OIDC line 544 does this) | +| Pin vue/vue-router | 30s | 10min × 2 packages = 20min | Must test that Nuxt 3 doesn't conflict with pinned Vue version | +| Add @@index schema | 2min | 1h | Must generate migration, run on staging, verify no lock contention | +| Issue/PR templates | 10min | 10min ✓ | — | +| fallow.toml | 5min | 1h | Must identify all path patterns for exclusion, test fallow behavior | +| CLAUDE.md fix | 10s | 10s ✓ | — | +| OpenAPI spec | 2min | 30min | Requires dev server running with populated data | +| CHANGELOG.md | 10s | 10s ✓ | — | +| Remove server-ci.yml | 5s | 5s ✓ | — | +| Remove server/.editorconfig | 3s | 3s ✓ | — | +| Gitignore fallow artifacts | 30s | 30s ✓ | — | +| Remove commented code | 10min | 1h | 7 files across workspace, must verify each | +| CodeQL autobuild | 5min | 4h (P1-7 in my report) | Requires pnpm install for node_modules; affects CI time | +| Prisma migrate deploy | 5min | 2h | Must add to release workflow, test on staging, handle failure rollback | +| Docker layer caching | 5min | 1h | Requires testing cache key invalidation, multi-arch nuances | +| **TOTAL** | **~45min** | **~11h** | **15x undercount** | + +**Verdict:** The "Solid Ground" release is 2-3 days, not "a weekend." The artistry critic's external fixer table (section C) is the weakest part of the report — almost every item is undercounted by 2-10x. + +### B4: "Zero JSDoc is intentional policy" — speculation framed as fact + +**Artistry Claim (§E, JSDoc row):** "0 JSDoc across the codebase is a POLICY, not an oversight." + +**Counter:** +- Documentation-audit §5.1 says ZERO JSDoc across entire TS codebase. The audits don't call this a policy — it's an observed absence. +- The project HAS comments in some places (CLAUDEMD, AGENTS.md, inline comments like "forget why this ignor exists"). It's not uniformly anti-documentation. +- The documented pattern is: document what AI agents need (AGENTS.md), not what humans need (JSDoc). But calling this a "policy" implies deliberate choice with upheld reasoning. More likely: the solo dev never had external contributors, never needed to document for others, and the absence is accidental, not principled. +- If it IS a policy, it should be documented as such. Currently there's no decision record or style guide saying "don't write JSDoc." + +**Verdict:** Plausible hypothesis presented as fact. Zero evidence of an actual policy decision. + +### B5: "Torrential quarantine" — ignores production dependency + +**Artistry Claim (§D):** Delete torrential from active codebase, move to experimental branch or feature flag. + +**Counter:** +- Verified: `server/server/internal/services/torrential/droplet-interface.ts` is imported by `library/providers/flat.ts`, `library/providers/filesystem.ts`, and `clients/ca.ts`. The `06.service-spinup.ts` plugin imports `TORRENTIAL_SERVICE`. +- Torrential is NOT an optional plugin — it's integrated into the game library system. Removing it would disable game file management and the CA client. +- A feature flag requires: wrapping ALL imports in conditional logic, creating fallback providers for non-torrential mode, testing both paths. That's 1-2 weeks of work, not a "delete" action. +- The "move to experimental branch" option would cause merge conflicts on every server change that touches services/torrential. It's not practical for active development. + +**Verdict:** Good creative thinking, ignores the engineering reality of the dependency graph. Correctly identifies the problem (torrential is half-baked), wrong solution. + +### B6: "Zero TODOs = data loss event" — intellectually interesting, wrong conclusion + +**Artistry Claim (§F, point 5):** "Zero TODOs in a codebase this active means the debt is invisible, not absent." + +**Counter:** +- This assumes the dev(s) encounter issues and deliberately suppress them. Alternative: the dev works in short bursts, fixes issues immediately when found, and the absence of TODOs means **issues get fixed or ignored, not annotated**. +- The project uses fallow for structural debt tracking (unused code, complexity, missing tests). The decision to route debt tracking to fallow (tool) rather than TODO comments (inline) is a deliberate choice, not suppression. +- 671 fallow issues exist — the debt IS visible, just not in TODOs. The problem is fallow output is noise, not that TODOs are missing. + +**Verdict:** Interesting frame, wrong conclusion. The debt tracking mechanism exists (fallow); it's just poor. Fix fallow.toml, don't start writing TODOs. + +--- + +## C: Counter-Attacks on critic-ultrabrain + +### C1: Leverage points presented without ROI calculation + +**Ultrabrain Claim (§E):** Five leverage points listed with effort estimates but no ROI ratio (effort vs. findings resolved). + +**Counter:** + +| Leverage | Claimed Effort | Findings Resolved | ROI Ratio | Assessment | +|---|---|---|---|---| +| 1: Narrow `drop/no-prisma-delete` | 1h | 6 false suppressions (verification: need to confirm 6 is correct) | 6:1 | Good. But artistry correctly notes the rule needs entity-allowlist, which is custom ESLint logic. Effort may be 2-3h with testing. | +| 2: Exclude Prisma migrations from SonarQube | 15min | 8 BLOCKER issues cleared | 32:1 | **Best ROI in entire report.** But hidden constraint: SonarCloud free tier may not support per-path exclusion. Need to verify before claiming. | +| 3: Apply h3 factory to auth routes | 2-3 days | 12 routes testable | 4-6 routes/day | **Oversold.** At 2-3 days for 12 routes, that's 4-6 routes/day. At this rate, all 100 routes = 16-25 days. Not a leverage point — it's a significant time investment with no immediate production impact. More like P1-P2 work. | +| 4: Add desktop/main CI with path filter | 2h | 1 workspace covered | 0.5 workspaces/hour | Good if Nuxt 4 migration is close to stable. If not, CI is perpetually red. Dependency on migration status not evaluated. | +| 5: Fix CLAUDE.md | 1min | 1 doc fix | 60/hr | Trivial. But calling it a "leverage point" inflates the term. It's a one-line fix — not a cascade driver. | + +**Verdict:** Leverage 2 (SonarQube exclusion) is genuinely high-ROI and was underplayed. Leverage 3 (auth routes) is overinvested — the cascade effect is weaker than claimed because admin routes are 5x more numerous and individually more complex. + +### C2: Architectural debt realism — classification inflation + +**Ultrabrain Claim (§F):** 7 architectural debts, including Nuxt 3 vs Nuxt 4 split and missing CI for 3 workspaces. + +**Counter:** +- **Nuxt 3 vs Nuxt 4 split as "architectural debt":** Inflation. A migration-in-progress is not debt — it's work in progress. Two Nuxt versions in the repo is a temporary condition. Labeling it architectural debt implies it requires architectural intervention, when the fix is "finish the migration." Using a more precise label: "transient version divergence." +- **Missing CI for 3 workspaces (4-6 hours):** This is tactical debt, not architectural. Adding CI workflows doesn't change the architecture — it adds checks. Ultrabrain's own Leverage 4 estimate is 2h. A 2-4h fix resolving "no CI" is tactical. +- **No ADRs (4-8 hours):** Not debt at all for a solo dev. ADRs are coordination tools for teams, not engineering artifacts. No team = no ADR need. Labeling this "architectural" inflates the category. + +**Verdict:** Ultrabrain correctly identifies 3 genuinely architectural debts (defineEventHandler closure, metadata provider monolith, torrential → server dependency) and inflates 4 tactical items into the same category. Dilutes the term. + +### C3: Hidden Constraints — straw man claims + +**Ultrabrain Claim (§C):** "Adding CI for desktop/main/ → Nuxt 4 migration incomplete → CI would be perpetually red → ignored → worse than absent." + +**Counter:** +- This assumes all-or-nothing CI. Alternative: add a workflow that runs `pnpm typecheck` only on specified stable file lists. Use `paths-ignore` for known-unstable directories. +- The same constraint analysis says blocking CodeQL on build-mode:none "would double CI time." Counter: CI already takes 5-10 minutes. Adding `pnpm install` for JS/TS CodeQL would add ~2 min. That's a 20-40% increase, not "doubling." +- The SonarQube exclusion constraint: "SonarCloud might not support per-path exclusion in free tier." This is speculative. The ultrabrain didn't verify. If it IS supported, this constraint disappears entirely. + +**Verdict:** Some hidden constraints are realistic (torrential producer-consumer entanglement, defineEventHandler closure barrier), others are speculative worst-casing (CI doubling, SonarCloud limitation). + +### C4: System dynamics — over-engineered graphs + +**Ultrabrain Claim (§D):** 5 vicious cycles described as causal loops. + +**Counter:** +- **Cycle 1 (Testing Trap):** Correct structure, well-described. The 200-300 hour breaking condition is accurate. +- **Cycle 2 (Documentation Debt Spiral):** Premature. The AGENTS.md is 150 lines — trimming it to "what agents actually need" could create new agent confusion (information loss). The cycle also assumes JSDoc would help — in practice, JSDoc for functions no one reads doesn't improve anything. +- **Cycle 3 (Solo-Dev Quality Ceiling):** **Most important cycle in the report.** "The project needs contributors to fix quality, but quality scares contributors away" is the key insight. This IS the meta-finding. +- **Cycle 4 (Experimental Code Entanglement):** Correct but the breaking condition is wrong. You don't need to "cut the dependency or productionize it." You can: (a) add try-catch around the TORRENTIAL_SERVICE import, (b) add a health check with graceful degradation, (c) feature-flag the dependency. These partial fixes don't require the full 1-2 week sprint. +- **Cycle 5 (Rule Enforcement Paradox):** Minor cycle — affects only 16 suppressions. Not system-level. + +**Verdict:** Cycle 3 is the report's crown jewel. Cycles 2 and 5 overreach. Cycle 4's breaking condition is unnecessarily binary. + +--- + +## D: My P0s Defended + +### P0-1: torrential double unwrap chain (download.rs:57) +**Attacked by:** Critic-low (likely counts as "experimental code, skip"); Critic-artistry (wants to torrential quarantine) +**Defense:** Verified torrential IS wired into production (06.service-spinup.ts, 4 internal files import droplet-interface). A panic in the download handler crashes the entire server process. The fix is 0.5h and prevents a potential production crash on every game download. **P0 survives.** + +### P0-2: torrential inner unwrap defeats error return (server/mod.rs:134) +**Attacked by:** Same as P0-1 +**Defense:** `Err(anyhow!(String::from_utf8(...).unwrap()))` — the unwrap executes BEFORE Err, so the function panics instead of returning an error. This means ALL torrential server message parsing can crash on malformed input. 0.25h fix. **P0 survives.** + +### P0-3: Migration DELETE without WHERE (migration.sql:18) +**Attacked by:** Critic-artistry (considers all Prisma migration SQL "auto-generated, not fixable") +**Defense:** Artistry's counter-narrative says "a migration that runs exactly once — intentionally dropping a table column." This is WRONG. Migration file `20251210231153_move_to_version_id/migration.sql:18` has `DELETE FROM "Object"` with no WHERE clause. This is NOT auto-generated Prisma output — it's a raw SQL migration step that was manually added (Prisma generates ALTER TABLE, not full-table DELETE). If this migration re-runs or was never applied, ALL Object rows are deleted. **P0 survives, severity unchanged.** + +### P0-4: Promise in boolean conditional (session/index.ts:195) +**Attacked by:** Critic-artistry (over-hypes as "auth bypass," then potential under-reaction when that's debunked) +**Defense:** Artistry's "auth bypass" claim is wrong (I proved this in B1). But the actual impact is still P0-worthy: `removeSession()` failures are silently swallowed, session rows persist as orphans, and the OIDC logout path (line 544) pushes a meaningless Promise to the task queue. OIDC logout is broken silently. 0.25h fix. **P0 survives, with corrected rationale: session cleanup failure, not auth bypass.** + +### P0-5: vue/vue-router at "latest" (server/package.json) +**Attacked by:** Critic-low (likely says "lockfile pins it, so it's fine"); Ultrabrain (§B8) says the same +**Defense:** Ultrabrain's argument that "the lockfile freezes it" is **technically correct but practically wrong.** Dependabot/dependabot treats `"latest"` as "no update available" — it won't suggest version bumps. When the lockfile is regenerated (pnpm install --frozen-lockfile failure, CI cache miss, new developer setup), it resolves to the LATEST at that moment, which could be Vue 4 after a major release. 72 Vue components break simultaneously. The fix is labeling `"^3.5.0"` in the package.json while letting the lockfile stay as-is — zero behavioral change, massive risk reduction. **P0 survives. Ultrabrain's counterargument is wrong about Dependabot behavior.** + +**Also noted:** `desktop/main/package.json` line 30 also has `"vue-router": "latest"`. The same problem exists in TWO workspaces. This strengthens the P0 classification. + +### P0-6: desktop/main/ zero CI +**Attacked by:** Ultrabrain (§C: "migration incomplete — CI would be perpetually red") +**Defense:** The Nuxt 4 migration status IS a constraint, but the solution isn't "no CI." A minimal workflow checking only stable files (verified: package.json scripts include `typecheck` and `lint`), with a path filter excluding known-unstable migration artifacts, provides a net positive safety value. Even a fraction of feedback is better than zero. The current state — 0 tests, 0 CI, 24 pages — means every agent or human edit to desktop/main/ is blind. **P0 survives, with condition: CI must use path filtering for stability.** + +### P0-7: No DB migration in release pipeline +**No critic attacked this directly.** +**Defense:** This is the single most operationally dangerous finding. Without `prisma migrate deploy` in `server-release.yml`, a deploy with schema changes causes Prisma client/schema mismatch — every DB query returns 500. Having this NOT in the release pipeline means every release risks a full outage. 1h fix. **P0 survives uncontested.** + +### Summary: All 7 P0s survive adversarial review with adjusted rationales. +- P0-1, P0-2, P0-3, P0-7: Unchanged +- P0-4: Survives with corrected justification (session cleanup, not auth bypass) +- P0-5: Survives, strengthened (affects BOTH workspaces) +- P0-6: Survives with modified approach (path-filtered CI) + +--- + +## E: Hidden P0s I Missed Round 1 + +After re-scanning all 6 audits and the 3 other critics' reports, here's what I underrated or missed: + +### E1: `desktop/main/` also has `"vue-router": "latest"` [NEW P0] +**File:** `desktop/main/package.json:30` +**Why I missed it:** I only checked `server/package.json` for `vue` and `vue-router` pins. The desktop Nuxt 4 app has the SAME problem. +**Impact:** Desktop client frontend instability on major framework upgrade. +**Action:** Merge with P0-5 — pin in both workspaces. + +### E2: OIDC callback token leakage over HTTP [UPGRADE from P2 to P1] +**File:** SonarQube §3.4 item (OIDC uses HTTP) +**Why I underrated it:** I assumed TLS termination at reverse proxy. But the SonarQube finding flags the code path itself uses `http://`, not `https://` — meaning even behind a proxy, if the proxy-to-app connection is HTTP, the token is in cleartext on the internal network. For a self-hosted Docker instance, this IS the default deployment pattern. +**Impact:** OIDC authorization codes transmitted in cleartext within the Docker network. +**Reclassification:** P2-13 → **P1** (not P0 because it requires local network access, but higher than P2). + +### E3: `@ts-ignore` in `users.ts:21` — author explicitly doesn't know why [UPGRADE from P1 to borderline P0] +**File:** `users.ts:21` (comment: "forget why this ignor exists") +**Why I underrated it:** I had this at P1-11 as a pair with `news.ts:35`. The "forget why" comment elevates the risk — the author suppressed a type error they don't understand. If the underlying type shifts, the suppressed error produces runtime behavior the author couldn't have predicted. +**Impact:** Unknown — the suppressed type could mask anything from a harmless null check to a data corruption path. +**Reclassification:** P1-11 → **P1, borderline P0** (needs investigation to determine actual suppressed type). + +### E4: 50+ unwrap calls in torrential (systemic, not per-call) [UPGRADE latent risk] +**File:** Entire `torrential/src/` (code-quality §2.1: "50+ unwrap calls") +**Why I underrated it:** I classified only 2 specific unwrap chains as P0. The remaining 48+ unwraps are latent panics. The SYSTEMIC risk — that ANY of these could crash the server process — is itself a P0-class concern even though individual calls may be safe. +**Impact:** Each unwrap is a panic-on-bad-input point. With 50+ in a crate wired into production, the probability that at least one is reachable with bad input approaches certainty. +**Reclassification:** The systemic risk should be a separate P0 finding — "torrential crate: 50+ unwrap calls create unacceptable production crash surface." Individual P0-1 and P0-2 stand, but the systemic risk is the real P0. + +### E5: `server/server/plugins/06.service-spinup.ts` imports TORRENTIAL_SERVICE — startup crash risk [NEW P0] +**File:** `06.service-spinup.ts:3` +**Why I missed it:** I focused on runtime crashes (downloads, server messages) but missed the STARTUP crash risk. If torrential service initialization panics, the ENTIRE server never becomes ready. +**Impact:** Server fails to boot. Zero availability. Affects every deployment. +**Severity:** **P0** — this is the most impactful torrential-related risk. +**Action:** Wrap TORRENTIAL_SERVICE import in error boundary with fallback or graceful degradation. + +### E6: `fail_ci_if_error: false` across 4 CI workflows ensures coverage NEVER enforces [Artistry's insight, upgraded] +**Artistry flagged (§B bury-the-lede):** Codecov `fail_ci_if_error: false` means "the only feedback loop is disabled." +**Why I underrated it:** I called it "correct given 1.17% coverage" in my P4 list. But artistry's framing is more accurate: this setting ensures coverage DOESN'T enforce, which means it WILL stay at 1.17%. It's a self-fulfilling cycle. Should be P1, not P4 — making it fail_ci_if_error: true with a LOW threshold (e.g., 1%) would at least prevent coverage from going DOWN further. +**Reclassification:** P4 → **P1** (set fail_ci_if_error: true with minimum threshold to prevent regression). + +--- + +## F: Reframed Priority List (Authoritative After Cross-Attack) + +### P0 — Must-Fix Before Next Release (8 items, was 7) + +| # | Finding | File | Effort | Verdict vs Round 1 | +|---|---|---|---|---| +| P0-1 | torrential double unwrap chain | download.rs:57 | 0.5h | ✓ Unchanged | +| P0-2 | torrential inner unwrap defeats error return | server/mod.rs:134 | 0.25h | ✓ Unchanged | +| P0-3 | Migration DELETE without WHERE | migration.sql:18 | 0.5h | ✓ Unchanged (artistry's "auto-generated" claim refuted) | +| P0-4 | Promise in boolean conditional | session/index.ts:195 | 0.25h | ✓ Survives, rationale corrected | +| P0-5 | vue/vue-router "latest" (server + desktop) | server + desktop/main package.json | 0.2h | ✓ Strengthened (desktop also affected) | +| P0-6 | desktop/main/ zero CI | (missing workflow) | 2h | ✓ Survives, conditional on path-filtered approach | +| P0-7 | No DB migration in release pipeline | server-release.yml | 1h | ✓ Unchanged, uncontested | +| **P0-8** | **torrential startup crash risk** | `06.service-spinup.ts:3` + 50+ unwraps | **4h** | **NEW** — systemic unwrap risk + startup path | + +**Total P0 effort:** ~8.7 dev-days (was 4.6 — P0-8 nearly doubles it) + +### P1 — Must-Fix This Quarter (17 items, was 15) + +Updated additions: +- **P1-16** (new): Set `fail_ci_if_error: true` with >1% threshold — prevents coverage regression (from P4) +- **P1-17** (upgrade from P2): OIDC HTTP token leakage — escalate from P2-13 +- **P1-11 reclassified**: `@ts-ignore` in users.ts:21 — marked as P1-borderline-P0, needs investigation + +**Updated P1 effort:** ~20 dev-days (was 16.5) + +### Key Adjustments from Round 1 + +| Change | From | To | Driver | +|---|---|---|---| +| P0 count | 7 | 8 | Systemic torrential risk + startup path | +| P4→P1 | fail_ci_if_error | Coverage regression | Artistry's "no feedback loop" insight | +| P2→P1 | OIDC HTTP | Token leakage in Docker network | Re-examined deployment architecture | +| P1 rationale | "unknown ts-ignore" | "author doesn't know why" | Elevates from routine to investigative | +| P0-4 narrative | "auth bypass" | "session cleanup failure" | Corrected from artistry's overclaim | +| P0-5 scope | server/ only | server/ + desktop/main/ | Cross-workspace verification | + +### What I Maintain (Disagreements with Other Critics) + +| Other Critic's Claim | My Position | Reasoning | +|---|---|---| +| Artistry: "Promise = most impactful fix" | Torrential startup crash is more impactful | Process-wide crash > silent cleanup failure | +| Artistry: "Solid Ground is a weekend" | 2-3 days minimum, 15x undercount | Verified each item's real effort | +| Ultrabrain: leverage 3 (auth routes in 2-3d) | ~25h saved, not transformative | Admin routes 5x more numerous + complex | +| Ultrabrain: Nuxt 3/4 = architectural debt | Work in progress, not debt | Temporary conditions ≠ structural flaws | +| Critic-low: 14 false positives | At least 5 are real | `@ts-ignore` knowledge gap alone proves it | +| Artistry: "zero JSDoc = policy" | Absence, not policy | No evidence of intentional decision | +| Artistry: "torrential quarantine" | Impractical due to dependency graph | 4 production files import it directly | + +### Adversarial Self-Correction + +The cross-attack validates my Round 1 framework (P0 classification based on crash/data-loss/blocker criteria) but shows three systematic blind spots: + +1. **Systemic risks from individual findings**: I evaluated each torrential unwrap independently but missed the startup-path risk and the 50+ unwrap systemic probability +2. **Self-fulfilling CI configurations**: I accepted `fail_ci_if_error: false` as pragmatic without seeing the feedback-loop dynamic +3. **Cross-workspace vulnerability**: I checked `server/package.json` but not `desktop/main/package.json` for the same "latest" problem — a 2-minute verification I skipped + +These blind spots all share a root cause: **individual finding analysis without system dynamics thinking.** Ultrabrain's system dynamics (though over-engineered in places) is the right tool for catching these. My next analysis will add a "cross-cutting dependency check" step before finalizing any finding severity. + +--- + +**End of Cross-Attack Report — High-Effort Critic, Round 2** diff --git a/.omo/review/documentation-audit.md b/.omo/review/documentation-audit.md new file mode 100644 index 000000000..ae45bedd2 --- /dev/null +++ b/.omo/review/documentation-audit.md @@ -0,0 +1,365 @@ +# Documentation Audit Report — Drop Monorepo + +**Date:** 2026-07-25 +**Auditor:** documentation-auditor (deep-audit-team) +**Scope:** Entire Drop monorepo — root docs, sites/docs, inline code docs, API docs, architecture docs, GitHub templates, changelogs, technical debt markers + +--- + +## 1. Documentation Inventory + +### 1.1 Root Documentation + +| Document | Status | Lines | Quality | +|----------|--------|-------|---------| +| `README.md` | ✅ Present | 37 | **Good** | +| `AGENTS.md` | ✅ Present | 234 | **Excellent** | +| `CLAUDE.md` | ✅ Present | 81 | **Good** | +| `CONTRIBUTING.md` | ✅ Present | 27 | **Needs Work** | +| `SECURITY.md` | ✅ Present | 65 | **Excellent** | +| `LICENSE` (AGPL-3.0 root) | ✅ Present | — | Standard | +| `.env.example` | ✅ Present | 5 | **Minimal** | +| `.editorconfig` | ✅ Present | 32 | **Good** | +| `fallow.json` | ✅ Present | — | Tool config | + +### 1.2 `docs/` Directory + +| File | Status | Quality | +|------|--------|---------| +| `docs/coverage-baseline-2026-07-24.md` | ✅ | **Good** (snapshot) | +| Everything else | ❌ **Missing** | — | + +**Verdict:** The `docs/` directory is effectively empty (1 file). No architecture docs, no ADRs, no system design docs, no diagrams, no developer guides. + +### 1.3 `sites/docs/` — Astro Starlight Documentation Site + +**Total pages:** 40 (19 .md + 18 .mdx + 3 images) +**Framework:** Astro 6 + Starlight with plugins (theme-rapide, links-validator, image-zoom) +**Structure:** + +``` +User (7 pages) +├── Getting Started (index.md) +├── Install/ +│ ├── Windows, macOS, Ubuntu, Debian, Fedora, Arch Linux, +│ │ Steam Deck, Bazzite (8 platform guides) +│ └── ...with screenshots for Ubuntu/Fedora/Debian store installs +└── Usage/ + └── Proton (mdx) + +Admin (19 pages) +├── Quickstart (compose.yaml deployment) +├── Guides/ +│ ├── Exposing, Creating Library, Import Game, Import Version, +│ │ Migrating +│ └── ...with screenshots (version-import-wizard) +├── Going Further/ +│ ├── Setting Up OIDC, Importing Update, Emulators +├── Metadata/ +│ ├── IGDB, Steam, GiantBomb, PCGamingWiki, Manual +├── Authentication/ +│ ├── Simple, OIDC, MFA + +Reference (6 pages) +├── Build Server, Build Client, Downloads, Library Sources, +│ Command Parsing, Update Mode +``` + +**Quality: Good overall** — well-structured, good Starlight configuration, useful content for users and admins. + +**Gaps:** +- ❌ No API reference documentation +- ❌ No developer/contributor documentation section +- ❌ No CLI (`downpour`) documentation +- ❌ No desktop client documentation +- ❌ No library (`droplet`, `native_model`) usage docs +- ❌ No architecture overview or system design +- ❌ No FAQ or troubleshooting section +- ❌ Some pages are very short (e.g., Steam metadata: 1 paragraph) + +### 1.4 Workspace README Files + +| README | Status | Quality | +|--------|--------|---------| +| `server/README.md` | ✅ | **Minimal** (1 sentence) | +| `cli/README.md` | ✅ | **Minimal** (1 sentence) | +| `desktop/README.md` | ✅ | **Minimal** (1 sentence) | +| `sites/promo/README.md` | ✅ | **Minimal** (1 paragraph) | +| `sites/docs/README.md` | ✅ | **Minimal** ("The docs for Drop.") | +| `libraries/base/README.md` | ✅ | **Minimal** (1 sentence) | +| `libraries/droplet/README.md` | ✅ | **Needs Work** (brief overview) | +| `libraries/native_model/README.md` | ✅ | **Excellent** (327 lines, full docs + examples) | +| `libraries/libarchive/README.md` | ✅ | **Stale** (forked upstream README, points to Chef org) | +| `torrential/README.md` | ❓ | Not checked (experimental) | + +**Pattern:** Most workspace READMEs are one-liners. Only `native_model` has thorough documentation. + +### 1.5 Inline Code Documentation + +#### TypeScript (`server/`) + +| Metric | Count | +|--------|-------| +| JSDoc/TSDoc comments (`/**`) | **0** | +| `defineRouteMeta` with OpenAPI | **5 endpoints** out of 100+ | +| Total API route handlers | 100+ in `server/server/api/v1/` | +| Internal modules (`server/server/internal/`) | 24 subdirectories | + +**Verdict: Missing.** Zero JSDoc/TSDoc in the TypeScript codebase. API documentation via `defineRouteMeta` is used on only 5 of 100+ endpoints. Internal modules have no documentation at module level. + +#### Rust (`cli/`, `desktop/src-tauri/`, `libraries/`) + +| Crate | `///` doc comments | `//!` crate docs | Quality | +|-------|-------------------|-------------------|---------| +| `native_model` | 170 in 8 files | 30 in 6 files | **Excellent** | +| `cli/` (downpour) | 7 in 1 file | 3 in 1 file | **Minimal** | +| `desktop/src-tauri/tailscale` | 48 in 1 file | 0 | **Moderate** | +| `desktop/src-tauri/download_manager` | 25 in 2 files | 0 | **Moderate** | +| `desktop/src-tauri/database` | 0 | 4 in 1 file | **Minimal** | +| `desktop/src-tauri/cloud_saves` | 1 | 0 | **None** | +| `desktop/src-tauri/src` (main) | 1 | 0 | **None** | +| `droplet` | 0 | 0 | **None** | +| `droplet_types` | 0 | 0 | **None** | +| `libarchive` | 0 | 0 | **None** (forked) | + +**Verdict:** Highly uneven. `native_model` has excellent docs. The rest ranges from minimal to none. The critical business logic in `droplet`, `droplet_types`, and most of `desktop/src-tauri` is undocumented. + +#### Vue Components + +| Metric | Count | +|--------|-------| +| Total `.vue` files in `server/components/` | **72** | +| Files with JSDoc or HTML comments | **13** (36 matches total) | +| Files with NO documentation | **59 (~82%)** | + +**Verdict:** Most Vue components (82%) have no inline documentation. Props, events, slots, and usage patterns are not documented. + +### 1.6 API Documentation + +| Item | Status | Details | +|------|--------|---------| +| Nitro OpenAPI generation | ✅ Enabled | `nitro.experimental.openAPI: true` in nuxt.config.ts | +| OpenAPI spec committed | ❌ Missing | Only available at runtime `/api/_openapi.json` | +| Route metadata (OpenAPI tags) | ❌ Minimal | Only 5 endpoints use `defineRouteMeta` | +| Request/response types documented | ❌ Missing | arktype schemas exist but aren't exported as docs | +| API route handler comments | ❌ Missing | Zero JSDoc/TSDoc in any route handler | + +**Verdict:** The Nitro OpenAPI infrastructure is in place but largely unused. Of 100+ API endpoints, only 5 have any OpenAPI metadata. No committed OpenAPI spec for external consumers. The client SDK (`libraries/droplet`) has no documented API contract. + +### 1.7 Architecture Documentation + +| Item | Status | +|------|--------| +| ADRs (Architecture Decision Records) | ❌ **Missing** | +| Diagrams (.drawio, .puml, .mermaid) | ❌ **Missing** | +| System design docs | ❌ **Missing** | +| Architecture overview | ⚠️ **De facto** (AGENTS.md sections) | +| Data flow diagrams | ❌ **Missing** | +| Deployment architecture | ⚠️ **Partial** (docker-compose in quickstart docs) | +| Security architecture | ✅ **Good** (SECURITY.md + risk-register.yaml) | + +**Verdict:** No formal architecture documentation exists. AGENTS.md acts as the de facto architecture reference but only covers CI, plugin ordering, metadata providers, and Prisma workflow. + +### 1.8 Changelog / Release Notes + +| Item | Status | +|------|--------| +| Root `CHANGELOG.md` | ❌ **Missing** | +| `desktop/changelog.md` | ✅ Present (v0.1.0-beta, v0.2.0-beta) | +| GitHub Releases | ✅ Uses GitHub releases | + +**Verdict:** No root-level changelog. Desktop has an auto-generated changelog (via go-conventional-commits). Server, CLI, and libraries have no changelogs. + +### 1.9 GitHub Templates + +| Item | Status | +|------|--------| +| `ISSUE_TEMPLATE/` directory | ❌ **Missing** | +| `PULL_REQUEST_TEMPLATE/` directory | ❌ **Missing** | +| `pull_request_template.md` | ❌ **Missing** | +| `CODEOWNERS` | ✅ Present (17 rules) | +| `dependabot.yml` | ✅ Present | +| `coderabbit.yaml` | ✅ Present | +| CI workflows | ✅ **Comprehensive** (14 workflow files) | + +**Verdict:** No issue or PR templates. This means contributors get no guidance on what information to include. CODEOWNERS and CI workflows are well-configured. + +### 1.10 Technical Debt Markers (TODO/FIXME/HACK) + +| Search Scope | Matches | +|-------------|---------| +| `server/server/api/` (all .ts) | **0** | +| `cli/` (all .rs) | **0** | +| `desktop/src-tauri/` (all .rs) | **0** | +| `libraries/` (all .rs) | **0** | + +**Verdict:** Zero TODO/FIXME/HACK markers found anywhere in the codebase. This is either excellent discipline or indicates that technical debt is being silently accumulated without tracking. + +**Note:** The `fallow.json` audit reveals **671 total issues** (385 unused files, 126 unused exports, 47 unlisted dependencies, 44 unresolved imports, etc.) — these represent significant undocumented technical debt, but none are tracked as TODO comments in code. + +--- + +## 2. Quality Assessment Per Doc + +| Document | Quality | Key Issues | +|----------|---------|------------| +| `README.md` | 🟢 Good | Clear, concise. Could add install instructions directly. | +| `AGENTS.md` | 🟢 Excellent | Dense, accurate, covers workspaces, builds, CI, gotchas, deferred work. Shows maintenance date. | +| `CLAUDE.md` | 🟢 Good | Clear behavioral rules. Some duplication with AGENTS.md. | +| `CONTRIBUTING.md` | 🟡 Needs Work | Self-describes as "stub — full guide being developed." Lacks detailed setup, coding standards, review process. | +| `SECURITY.md` | 🟢 Excellent | Full disclosure policy, response timeline, scope, risk register, supported versions. | +| `.env.example` (root) | 🔴 Minimal | Only 3 vars. Points to server/.env.example but doesn't document all vars. | +| `.env.example` (server/) | 🟡 Needs Work | 11 vars, no descriptions for most. Missing vars like OIDC_, DISABLE_SIMPLE_AUTH, TORRENTIAL_PATH. | +| `docs/` directory | 🔴 Missing | Single file. No architecture, design, or developer docs. | +| `sites/docs/` | 🟢 Good | Well-structured Starlight site. Missing API ref, developer docs, CLI docs. | +| Server README | 🔴 Minimal | One sentence. | +| CLI README | 🔴 Minimal | One sentence. | +| Desktop README | 🔴 Minimal | One sentence. | +| All workspace READMEs | 🔴 Minimal | Most are 1-sentence stubs. | +| `native_model` docs | 🟢 Excellent | Full crate docs, README with examples, performance benchmarks. | +| `droplet` docs | 🔴 Minimal | Brief README, no inline docs, no API contract docs. | +| `libarchive` README | 🔴 Stale | Forked from Chef org, points to Travis CI (dead). | +| TypeScript JSDoc/TSDoc | 🔴 Missing | Zero across entire server codebase. | +| Rust doc comments (most crates) | 🔴 Minimal | Only native_model has good coverage. | +| Vue component docs | 🔴 Minimal | 82% of components undocumented. | +| API OpenAPI annotations | 🔴 Minimal | 5 of 100+ endpoints annotated. | +| Architecture docs | 🔴 Missing | No ADRs, no diagrams, no system design. | +| CHANGELOG (root) | 🔴 Missing | No root changelog. Desktop has one per-release. | +| GitHub issue/PR templates | 🔴 Missing | No templates to guide contributors. | +| TODO/FIXME/HACK in code | 🟢 Clean | Zero found. | + +--- + +## 3. Specific Gaps with Recommendations + +### P0 — Critical Gaps (block contributor onboarding & API consumers) + +| Gap | Recommendation | Effort | +|-----|---------------|--------| +| **No API documentation** | Generate OpenAPI spec from Nitro, commit `openapi.yaml` to repo. Add `defineRouteMeta` to all 100+ endpoints. Expose arktype schemas. | **Large** | +| **No CONTRIBUTING detail** | Expand CONTRIBUTING.md: full local dev setup, Docker workflow, test running, coding conventions, review process. | **Medium** | +| **No issue/PR templates** | Create `.github/ISSUE_TEMPLATE/bug.yml`, `feature.yml`, and `.github/PULL_REQUEST_TEMPLATE.md`. | **Small** | +| **Workspace READMEs are stubs** | Expand each workspace README to include purpose, setup, build commands, and usage examples. | **Medium** | + +### P1 — High Priority (developer experience & maintainability) + +| Gap | Recommendation | Effort | +|-----|---------------|--------| +| **No ADRs** | Start an `docs/adr/` directory. Record key decisions: metadata provider chain, double-nested server structure, Nuxt 3 vs 4 split. | **Medium** | +| **No architecture diagrams** | Add Mermaid diagrams for: system architecture, deployment, auth flow, metadata provider chain, data models. | **Medium** | +| **Zero JSDoc/TSDoc in TypeScript** | Add `@param` and `@returns` JSDoc to all internal module exports. Add module-level `@packageDocumentation` to `server/server/internal/*/index.ts`. | **Large** | +| **Missing CLI (`downpour`) docs** | Document commands, flags, examples in both code (`///`) and docs site. | **Medium** | +| **Missing desktop client docs** | Document Tauri commands, download manager architecture, auth protocol on docs site. | **Medium** | + +### P2 — Medium Priority + +| Gap | Recommendation | Effort | +|-----|---------------|--------| +| **docs/ directory sparse** | Create `docs/architecture.md`, `docs/development.md`, `docs/deployment.md`. | **Medium** | +| **Vue component docs missing** | Add prop/event/slot documentation to all components. Use Vue's `defineProps`/`defineEmits` with JSDoc annotations. | **Medium** | +| **Rust doc comments (non-native_model)** | Add `///` docs to public API surfaces in droplet, droplet_types, and desktop crates. | **Medium** | +| **No CHANGELOG at root** | Generate root `CHANGELOG.md` from git history using conventional commits. | **Small** | +| **`.env.example` incomplete** | Document all env vars with descriptions and defaults. | **Small** | + +### P3 — Nice to Have + +| Gap | Recommendation | Effort | +|-----|---------------|--------| +| **sites/docs lacks API reference** | Add auto-generated API reference from OpenAPI spec. | **Large** | +| **No FAQ section in docs** | Create FAQ from common Discord/forum questions. | **Small** | +| **libarchive README is stale** | Replace with Drop-maintained version (not Chef fork). | **Small** | +| **No developer docs section** | Add "For Developers" section to Starlight: architecture, contributing, building, testing. | **Medium** | + +--- + +## 4. Priority Ranking + +``` +P0: ████████████████▌ 1. API documentation (OpenAPI + route metadata) + 2. CONTRIBUTING.md expansion + 3. Issue/PR templates + 4. Workspace README improvements + +P1: ██████████████ 1. ADRs (architecture decision records) + 2. Architecture diagrams + 3. JSDoc/TSDoc for TypeScript + 4. CLI & Desktop documentation + +P2: ████████████ 1. docs/ directory expansion + 2. Vue component documentation + 3. Rust doc comments (droplet, desktop) + 4. Root CHANGELOG.md + 5. .env.example completion + +P3: ██████ 1. API reference in docs site + 2. FAQ section + 3. libarchive README fix + 4. Developer docs section +``` + +--- + +## 5. Strengths (What's Done Well) + +1. **AGENTS.md is exceptional.** Dense, accurate, well-maintained, covers workspaces, builds, CI, gotchas, deferred work log. Serves as the project's best single documentation artifact. + +2. **sites/docs is well-configured.** Astro Starlight with good plugins (links-validator, image-zoom), clean sidebar navigation, useful deployment quickstart, platform install guides, metadata provider docs. + +3. **native_model has excellent documentation.** Crate-level docs, inline doc comments on all public APIs, extensive README with examples, performance benchmarks. + +4. **SECURITY.md is comprehensive.** Full disclosure policy, response timeline, scope definition, risk register with review dates. + +5. **CI workflows are documented** in AGENTS.md with a complete CI workflow map. + +6. **Deferred work is tracked** in AGENTS.md with triggers and rationale (6 items documented). + +7. **Zero TODO/FIXME/HACK** in code — no undocumented technical debt markers. + +--- + +## 6. Documentation Freshness Assessment + +| Document | Freshness | Notes | +|----------|-----------|-------| +| AGENTS.md | 🟢 Fresh | Last updated 2026-07-24 (test state, PR #22) | +| CLAUDE.md | 🟢 Fresh | References current tooling | +| SECURITY.md | 🟢 Fresh | Current risk register with review dates | +| docs/coverage-baseline.md | 🟢 Fresh | Dated 2026-07-24 | +| sites/docs content | 🟡 Mostly fresh | Docker tag `0.4.0-rc-3` in quickstart — may be outdated | +| libarchive/README.md | 🔴 Stale | Forked from Chef/libarchive-rust, references Travis CI | +| droplet/README.md | 🟡 Needs update | Brief, lacks current API surface | +| Workspace READMEs | 🔴 Stale | All one-liners, no details | + +--- + +## 7. Documentation by the Numbers + +| Category | Total | Documented | Coverage | +|----------|-------|-----------|----------| +| API route handlers | 100+ | 5 (OpenAPI) | **5%** | +| Internal modules | 24 dirs | 0 (module docs) | **0%** | +| Vue components | 72 | 13 (partial) | **18%** | +| Rust crates | 10+ | 1 (native_model) | **10%** | +| Cli commands | TBD | 0 | **0%** | +| Tauri commands | TBD | 0 | **0%** | +| Workspace READMEs | 10 | 1 (native_model) | **10%** | + +--- + +## 8. Summary + +**Overall Documentation Health: 🟡 Needs Work** + +The Drop monorepo has strong foundations (AGENTS.md, SECURITY.md, docs site infrastructure) but critical gaps in API documentation, inline code docs, architecture documentation, and contributor onboarding. The gap between the excellent security docs and the missing API/arch docs suggests security was prioritized (correctly), but developer experience and API consumer documentation have been neglected. + +**Quick wins (can be done in <1 hour):** +1. Create ISSUE_TEMPLATE/bug.yml and feature.yml +2. Create PULL_REQUEST_TEMPLATE.md +3. Expand `.env.example` with all vars +4. Generate root CHANGELOG.md from git history + +**Highest impact medium-term work:** +1. Add `defineRouteMeta` to all 100+ API endpoints +2. Create `docs/adr/` with 3-5 initial ADRs +3. Add architecture diagrams to docs site +4. Expand workspace READMEs beyond one-liners diff --git a/.omo/review/sonarqube-audit.md b/.omo/review/sonarqube-audit.md new file mode 100644 index 000000000..4443546dc --- /dev/null +++ b/.omo/review/sonarqube-audit.md @@ -0,0 +1,265 @@ +# SonarQube Audit Report — Drop Project + +**Generated:** 2026-07-25 +**Project Key:** `BillyOutlast_drop` +**Branch:** `develop` (main branch) +**Analysis Date:** 2026-07-26T01:47:37Z + +--- + +## 1. Quality Gate: ❌ ERROR + +| Condition | Status | Threshold | Actual | +|-----------|--------|-----------|--------| +| new_reliability_rating | ❌ ERROR | 1 | **3** | +| new_security_rating | ❌ ERROR | 1 | **3** | +| new_maintainability_rating | ✅ OK | 1 | 1 | +| new_duplicated_lines_density | ✅ OK | 3 | 0.1 | +| new_security_hotspots_reviewed | ✅ OK | 100 | 100.0 | + +**Quality Gate fails** due to reliability and security ratings exceeding threshold. + +--- + +## 2. Project Metrics + +| Metric | Value | +|--------|-------| +| Lines of Code (ncloc) | 72,023 | +| Cyclomatic Complexity | 3,765 | +| Total Violations | 127 | +| Bugs | 10 | +| Vulnerabilities | 13 | +| Code Smells | 104 | +| Security Hotspots | 0 | +| Technical Debt | 1,179 min (~19.7 hours) | +| Duplicated Lines Density | 1.1% | +| Test Coverage | N/A (not reported to SonarCloud) | + +--- + +## 3. Issues by Severity + +### 3.1 BLOCKER (6 issues) + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 1 | `server/prisma/migrations/20260206064926_rename_gametypes/migration.sql` | 15 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | +| 2 | `server/prisma/migrations/20251210231153_move_to_version_id/migration.sql` | 18 | plsql:DeleteOrUpdateWithoutWhereCheck | Ensure WHERE clause is not missing in this DELETE query. | +| 3 | `server/prisma/migrations/20250721053244_update_genre_names/migration.sql` | 13 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | +| 4 | `server/prisma/migrations/20250721061200_remove_genres/migration.sql` | 11 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | +| 5 | `server/prisma/migrations/20250401083942_rename_save_to_cloud_saves/migration.sql` | 14 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | +| 6 | `server/prisma/migrations/20241226065709_rename_custom_to_manual/migration.sql` | 15 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | +| 7 | `server/prisma/migrations/20241105221904_different_client_capabilities/migration.sql` | 13 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | +| 8 | `server/prisma/migrations/20241105222110_trackable_names_for_capabilities/migration.sql` | 13 | plsql:QuotedIdentifiersCheck | Avoid using quoted identifiers. | + +> All BLOCKER issues are in auto-generated Prisma migration SQL — inherited from Prisma's naming conventions. The DELETE without WHERE (item 2) warrants review. + +### 3.2 CRITICAL / HIGH (4 issues — Cognitive Complexity) + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 1 | `server/server/internal/auth/oidc/index.ts` | 154 | ts:S3776 | Cognitive Complexity 24 (limit 15). Refactor function. | +| 2 | `server/server/internal/session/cache.ts` | 50 | ts:S3776 | Cognitive Complexity 23 (limit 15). Refactor function. | +| 3 | `server/server/internal/session/db.ts` | 156 | ts:S3776 | Cognitive Complexity 24 (limit 15). Refactor function. | +| 4 | `server/server/internal/session/memory.ts` | 41 | ts:S3776 | Cognitive Complexity 21 (limit 15). Refactor function. | + +> All CRITICAL issues are high cognitive complexity in core server logic (auth + session management). 3 of 4 in session layer. + +### 3.3 MAJOR / MEDIUM (73 issues — top 30 listed by area) + +#### Accessibility (Web:S6819, Web:InputWithoutLabelCheck, Web:S5255) + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 1 | `server/components/GameEditor/Metadata.vue` | 56 | Web:S6853 | Form label must be associated with a control. | +| 2 | `server/components/GameEditor/Metadata.vue` | 90 | Web:InputWithoutLabelCheck | Input missing id + label. | +| 3 | `server/components/GameEditor/Metadata.vue` | 102 | Web:InputWithoutLabelCheck | Input missing id + label. | +| 4 | `server/components/GameEditor/Metadata.vue` | 218 | Web:S6819 | Use `` instead of status role. | +| 5 | `server/components/Selector/MultiItem.vue` | 96 | Web:S6819 | Use `` instead of status role. | +| 6 | `server/pages/admin/task/[id]/index.vue` | 71 | Web:S6819 | Use `` instead of status role. | +| 7 | `server/pages/admin/library/index.vue` | 463 | Web:S6819 | Use `` instead of status role. | +| 8 | `server/pages/admin/library/import.vue` | 266 | Web:S6819 | Use `` instead of status role. | +| 9 | `server/pages/admin/library/[id]/import.vue` | 321 | Web:S6819 | Use `` instead of status role. | +| 10 | `server/pages/client/authorize/[id].vue` | 69 | Web:InputWithoutLabelCheck | Input missing id + label. | +| 11 | `server/layouts/admin.vue` | 59,101 | Web:S5255 | Add aria-label to nav elements. | +| 12 | `server/components/UserHeader.vue` | 7,146 | Web:S5255 | Add aria-label to nav elements. | +| 13 | `server/pages/store/[id]/index.vue` | 54 | Web:S5256 | Add `` headers to table. | +| 14 | `server/components/StoreView.vue` | 465 | ts:S3358 | Extract nested ternary operation. | +| 15 | `desktop/main/components/LibrarySearch.vue` | 96 | Web:S6819 | Use `` instead of status role. | +| 16 | `desktop/main/pages/library/[id]/index.vue` | 348 | Web:S6819 | Use `` instead of status role. | +| 17 | `desktop/main/components/InitiateAuthModule.vue` | 17 | Web:S6819 | Use `` instead of status role. | +| 18 | `desktop/main/pages/auth/processing.vue` | 4 | Web:S6819 | Use `` instead of status role. | +| 19 | `libraries/base/components/LoadingButton.vue` | 7 | Web:S6819 | Use `` instead of status role. | + +#### Security-Sensitive + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 20 | `.github/workflows/e2e.yml` | 64 | githubactions:S6505 | Omit `--ignore-scripts` allows lifecycle scripts to run. | +| 21 | `Dockerfile` | 37 | docker:S8549 | Unlocked dependency versions (security-sensitive). | +| 22 | `desktop/optimize-appimage.sh` | 17 | shell:S6506 | Not disabling redirects might allow insecure redirects. | + +#### Dependency Locking (text:S8570) + +| # | File | Rule | Message | +|---|------|------|---------| +| 23 | `libraries/libarchive/Cargo.toml` | text:S8570 | Cargo.lock may be missing — versions not predictable. | +| 24 | `libraries/native_model/Cargo.toml` | text:S8570 | Cargo.lock may be missing. | +| 25 | `libraries/native_model/native_model_macro/Cargo.toml` | text:S8570 | Cargo.lock may be missing. | +| 26 | `libraries/native_model/tests_crate/Cargo.toml` | text:S8570 | Cargo.lock may be missing. | + +#### Code Quality (TypeScript) + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 27 | `server/server/internal/metadata/steam.ts` | 646,918,1010,1013,1107 | ts:S8786 | Regex with super-linear backtracking (5 occurrences). | +| 28 | `server/server/internal/metadata/steam.ts` | 449 | ts:S4624 | Nested template literal. | +| 29 | `server/server/internal/metadata/giantbomb.ts` | 361 | ts:S4624 | Nested template literal. | +| 30 | `server/server/internal/metadata/igdb.ts` | 652 | ts:S1751 | Loop body allows only one iteration. | +| 31 | `server/server/internal/metadata/pcgamingwiki.ts` | 340 | ts:S6035 | Replace alternation with character class. | +| 32 | `server/server/internal/session/index.ts` | 195 | ts:S6544 | Expected non-Promise value in boolean conditional. | +| 33 | `server/server/api/v1/client/game/[id]/versions.get.ts` | 11,12 | ts:S4782 | Redundant `undefined` type with `?` specifier. | +| 34 | `server/server/internal/library/manifest/utils.ts` | 3 | ts:S6564 | Redundant type alias for `V2Manifest`. | +| 35 | `server/server/internal/library/index.ts` | 381 | ts:S4043 | Use `toSorted()` instead of in-place `sort()`. | +| 36 | `server/server/internal/utils/prioritylist.ts` | 32 | ts:S4043 | Use `toSorted()` instead of in-place `sort()`. | +| 37 | `server/server/internal/objects/objectHandler.ts` | 24 | ts:S6564 | Redundant type alias for `string`. | +| 38 | `server/server/internal/db/database.ts` | 10 | ts:S2137 | Do not use `globalThis` to declare a variable. | +| 39 | `server/server/routes/auth/oidc.get.ts` | 18 | ts:S4624 | Nested template literal. | +| 40 | `server/composables/current-page-engine.ts` | 15 | ts:S4043 | Use `toSorted()` instead of in-place `sort()`. | +| 41 | `desktop/main/composables/current-page-engine.ts` | 16 | ts:S4043 | Use `toSorted()` instead of in-place `sort()`. | +| 42 | `desktop/main/pages/settings/index.vue` | 45 | ts:S7785 | Prefer top-level await over promise chain. | +| 43 | `desktop/main/pages/setup/server.vue` | 97 | ts:S1854 | Remove useless assignment to `result`. | +| 44 | `server/pages/admin/users/auth/simple/index.vue` | 427,442 | ts:S1121 | Extract assignment from expression. | +| 45 | `server/pages/admin/users/auth/simple/index.vue` | 446 | ts:S8786 | Regex with super-linear backtracking. | + +#### Promo Site + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 46 | `sites/promo/src/components/comparison.tsx` | 54 | ts:S6772 | Ambiguous spacing after img element. | +| 47 | `sites/promo/src/components/comparison.tsx` | 200 | ts:S3358 | Extract nested ternary. | +| 48 | `sites/promo/src/components/comparison.tsx` | 281,310 | ts:S6479 | Do not use Array index in keys. | +| 49 | `sites/promo/src/components/comparison.tsx` | 330 | ts:S7721 | Move function `onlyUnique` to outer scope. | +| 50 | `sites/promo/src/components/comparison.tsx` | 458,463 | ts:S3358 | Extract nested ternary (2x). | +| 51 | `sites/promo/src/components/map.tsx` | 53 | ts:S2137 | Do not use `Map` to declare a function. | +| 52 | `sites/promo/src/components/sponsors.tsx` | 101 | ts:S1763 | Unreachable code (lines 101-116). | +| 53 | `sites/promo/src/components/sponsors.tsx` | 245,262 | ts:S6479 | Do not use Array index in keys. | +| 54 | `sites/promo/src/components/team.tsx` | 137 | ts:S6822 | Redundant explicit `role="list"` on `
    `. | + +### 3.4 MINOR / LOW (45 issues — representative selection) + +#### Security-adjacent + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 1 | `server/server/internal/services/torrential/index.ts` | 62,95 | ts:S4036 | PATH variable may contain writable directories. | +| 2 | `server/server/internal/services/services/nginx.ts` | 16 | ts:S4036 | PATH variable may contain writable directories. | +| 3 | `server/nuxt.config.ts` | 33 | ts:S4036 | PATH variable may contain writable directories. | +| 4 | `server/server/internal/auth/oidc/index.ts` | 105 | ts:S5332 | Using http protocol (use https instead). | +| 5 | `Dockerfile` | 62 | docker:S6471 | "node" image runs as root user. | + +#### Code Quality + +| # | File | Line | Rule | Message | +|---|------|------|------|---------| +| 6 | `server/rules/no-prisma-delete.mts` | 3 | ts:S7776 | Use `Set` instead of array for `blacklistedFunctions`. | +| 7 | `server/server/api/v1/store/index.get.ts` | 90 | ts:S7776 | Use `Set` instead of array for `companyActions`. | +| 8 | `server/server/internal/metadata/pcgamingwiki.ts` | 49 | ts:S4323 | Replace union type with type alias. | +| 9 | `server/server/internal/config/application-configuration.ts` | 15 | ts:S7784 | Prefer `structuredClone` over `JSON.parse(JSON.stringify(...))`. | +| 10 | `server/composables/ws.ts` | 32 | ts:S1301 | Replace switch with if statements. | +| 11 | `server/server/internal/clients/event-handler.ts` | 28 | ts:S1301 | Replace switch with if statements. | +| 12 | `sites/promo/eslint.config.mjs` | 2,3 | js:S7772 | Prefer `node:path` / `node:url` imports. | +| 13 | `server/server/internal/utils/recursivedirs.ts` | 1,2 | ts:S7772 | Prefer `node:fs` / `node:path` imports. | + +> Full list: 45 MINOR/INFO issues — mostly stylistic, modernization, and convention violations. + +--- + +## 4. Security Hotspots + +**Total: 0** — No security hotspots requiring review. + +--- + +## 5. Dependency Risks + +**N/A** — Advanced Security not enabled for this SonarCloud organization. Cannot audit dependency risks via SonarQube. + +--- + +## 6. Duplicated Code + +**Overall: 1.1% duplication density | 1,003 duplicated lines | 40 duplication blocks | 32 files affected** + +### Worst-offending files (>50% duplicated lines) + +| File | Duplicated Lines | Density | Blocks | +|------|-----------------|---------|--------| +| `server/server/internal/acls/descriptions.ts` | 98 | **81.7%** | 5 | +| `server/server/api/v1/collection/[id]/index.get.ts` | 30 | **81.1%** | 1 | +| `server/server/api/v1/client/news/index.get.ts` | 25 | **83.3%** | 1 | +| `server/server/api/v1/client/collection/[id]/index.get.ts` | 25 | **78.1%** | 1 | +| `server/server/api/v1/collection/[id]/index.delete.ts` | 29 | **78.4%** | 1 | +| `server/server/api/v1/client/collection/[id]/index.delete.ts` | 24 | **75.0%** | 1 | +| `server/server/api/v1/admin/news/index.get.ts` | 27 | **73.0%** | 2 | +| `server/server/api/v1/client/saves/[gameid]/index.get.ts` | 28 | **73.7%** | 1 | +| `server/server/api/v1/client/saves/[gameid]/[slotindex]/index.get.ts` | 35 | **62.5%** | 1 | +| `server/server/api/v1/client/saves/[gameid]/[slotindex]/index.delete.ts` | 35 | **67.3%** | 1 | +| `server/server/api/v1/client/saves/[gameid]/[slotindex]/push.post.ts` | 35 | **68.6%** | 1 | +| `server/server/api/v1/collection/[id]/entry.post.ts` | 18 | **72.0%** | 1 | +| `server/server/api/v1/admin/company/[id]/banner.post.ts` | 28 | **53.8%** | 1 | +| `server/server/api/v1/admin/company/[id]/icon.post.ts` | 28 | **52.8%** | 1 | +| `server/composables/current-page-engine.ts` | 15 | **50.0%** | 1 | + +### Cross-workspace duplication + +| File | Duplicated Lines | Density | +|------|-----------------|---------| +| `server/composables/current-page-engine.ts` | 15 | 50.0% | +| `desktop/main/composables/current-page-engine.ts` | 15 | 45.5% | + +| File | Duplicated Lines | Density | +|------|-----------------|---------| +| `server/pages/account/tokens.vue` | 52 | 22.6% | +| `server/pages/admin/settings/tokens.vue` | 52 | 22.2% | + +**Duplication patterns:** +- `acls/descriptions.ts` has 5 duplicate blocks — largest concentration +- Many `server/api/v1/` route handlers share boilerplate patterns (collection, notification, saves routes) +- `server/nuxt.config.ts` has 2 blocks with 54 duplicated lines +- Session implementations (`cache.ts` / `memory.ts`) share 39-40 duplicated lines +- WebAuthn and Passkey `finish.post.ts` share 39 lines + +--- + +## 7. Coverage Gaps + +**N/A** — No coverage data reported to SonarCloud for this project. Test coverage is not tracked in SonarQube. + +Per AGENTS.md: 32 vitest tests (1.17% line coverage), 10 cargo tests, 6 database cargo tests. Coverage baseline at 1.17% lines / 2.09% functions. + +--- + +## 8. Summary + +| Category | Count | Notes | +|----------|-------|-------| +| Violations (total) | 127 | | +| Bugs | 10 | 8 BLOCKER (SQL, auto-generated), 0 actual code bugs | +| Vulnerabilities | 13 | Security-adjacent config issues | +| Code Smells | 104 | Cognitive complexity, regex perf, a11y, duplication | +| Security Hotspots | 0 | None requiring review | +| Dependency Risks | N/A | Advanced Security not enabled | +| Quality Gate | ❌ ERROR | Reliability + Security ratings fail | +| Technical Debt | 1,179 min | ~19.7 hours estimated | +| Duplication | 1.1% | 1,003 lines in 32 files | +| Coverage | N/A | Not reported to SonarCloud | + +**Top priorities:** +1. Fix DELETE without WHERE in `20251210231153_move_to_version_id/migration.sql` +2. Refactor high-complexity functions in session layer (3 files) + OIDC +3. Fix regex backtracking in `steam.ts` (5 occurrences) + `pcgamingwiki.ts` +4. Resolve `Promise`-in-boolean bug in `session/index.ts:195` +5. Security PATH issues in `torrential/index.ts`, `nginx.ts`, `nuxt.config.ts` +6. Unreachable code in `sites/promo/src/components/sponsors.tsx:101-116` diff --git a/.omo/review/test-coverage-audit.md b/.omo/review/test-coverage-audit.md new file mode 100644 index 000000000..8c197c30a --- /dev/null +++ b/.omo/review/test-coverage-audit.md @@ -0,0 +1,378 @@ +# Test Coverage Audit — Drop Monorepo + +> Generated: 2026-07-25 by deep-audit-team/test-auditor + +--- + +## 1. Executive Summary + +| Workspace | Source Files | Test Files | Coverage Ratio | Status | +|-----------|-------------|------------|----------------|--------| +| server/ (Nitro backend) | ~173 TS files | 23 test + 4 mock/2 util | ~0.13:1 (13%) | Critical Gaps | +| server/ (Vue frontend) | ~142 Vue/TS files | 2 component tests | ~0.01:1 (1%) | Near Zero | +| cli/ (Rust) | 16 Rust files | 2 integration tests | ~0.13:1 (13%) | Low | +| desktop/src-tauri/ (Rust) | ~73 Rust files | 2 inline test modules | ~0.03:1 (3%) | Critical | +| desktop/main/ (Nuxt 4) | 24+ Vue files | 0 tests | 0:1 | Zero | +| sites/promo | Unknown (Next.js) | 0 tests | 0:1 | Zero | +| sites/docs | Unknown (Astro) | 0 tests | 0:1 | Zero | +| libraries/droplet | 11 Rust files | 2 test locations | ~0.18:1 (18%) | Low | +| libraries/droplet_types | 1 Rust file | 0 tests | 0:1 | Zero | +| libraries/libarchive | 5 Rust files | 3 test files | ~0.6:1 (60%) | Moderate | +| libraries/native_model | 8 Rust files | ~12 test files | ~1.5:1 (150%) | Good | +| **Total** | **~450+** | **~44 test files** | **~0.1:1 (10%)** | **Critical** | + +--- + +## 2. Test Inventory Per Workspace + +### 2.1 Server (TypeScript/Nitro + Nuxt) + +**Test Framework:** Vitest (unit/integration), Playwright (e2e) +**Mock Framework:** MSW (Mock Service Worker) +**Coverage Scope (vitest config):** `server/server/**/*.ts` only + +| Category | Path | Count | Has Tests? | +|----------|------|-------|------------| +| **Unit tests** | `server/test/unit/` | 15 files | ✅ | +| **Integration tests** | `server/test/integration/` | 5 files | ✅ | +| **Component tests** | `server/test/components/` | 2 files | ✅ | +| **E2E tests** | `server/test/e2e/` | 1 file (1 spec) | ✅ | +| **Mocks** | `server/test/mocks/` | 4 files | ✅ | +| **Utils** | `server/test/utils/` | 2 files | ✅ | +| **Setup** | `server/test/setup.ts` | 1 file | ✅ | + +**Unit test files (15):** +``` +unit/prioritylist.test.ts +unit/plugins/init-order.test.ts +unit/metadata/provider-chain.test.ts +unit/auth/webauthn.test.ts +unit/auth/session-fixation.test.ts +unit/auth/oidc-escalation.test.ts +unit/auth/ca-blacklist.test.ts +unit/auth-totp.test.ts +unit/acls/confused-deputy.test.ts +unit/utils.test.ts +unit/tuple.test.ts +unit/session-memory.test.ts +unit/h3-factory.test.ts +unit/colors.test.ts +unit/array.test.ts +``` + +**Integration test files (5):** +``` +integration/prioritylist.test.ts +integration/password-hash.test.ts +integration/oidc-mocks.test.ts +integration/fs-backend-hash.test.ts +integration/db-helper.test.ts +``` + +### 2.2 CLI (Rust) + +**Test Framework:** `cargo test` (built-in) +**Test location:** `cli/tests/` (integration tests) + +| Source | Files | Tests | +|--------|-------|-------| +| `cli/src/` | 16 .rs files | 0 inline (#[cfg(test)]) | +| `cli/tests/` | 2 files | manifest_test.rs, config_test.rs | + +### 2.3 Desktop (Rust — Tauri v2) + +**Test Framework:** `cargo test` +**7 crates in workspace** + +| Crate | Source Files | Test Files | Source LOC | Test LOC | Status | +|-------|-------------|------------|------------|----------|--------| +| `database/` | 6 + tests.rs | 1 inline | ~300 | ~150 | ✅ Partial | +| `tailscale/` | 2 + test.rs | 1 inline | ~100 | ~50 | ✅ Partial | +| `utils/` | 3 | 0 | ~80 | 0 | ❌ Zero | +| `remote/` | 7 | 0 | ~400 | 0 | ❌ Zero | +| `process/` | 6 | 0 | ~350 | 0 | ❌ Zero | +| `games/` | 11 | 0 | ~600 | 0 | ❌ Zero | +| `download_manager/` | 11 | 0 | ~500 | 0 | ❌ Zero | +| `cloud_saves/` | 11 | 0 | ~500 | 0 | ❌ Zero | +| `client/` | 5 | 0 | ~150 | 0 | ❌ Zero | +| Tauri app root (`src/`) | 11 | 0 | ~400 | 0 | ❌ Zero | + +### 2.4 Desktop (Nuxt 4 — desktop/main/) + +| Source | Files | Tests | +|--------|-------|-------| +| Pages | 24 .vue | 0 | +| Components/Composables | Unknown | 0 | + +### 2.5 Sites + +| Site | Framework | Tests | +|------|-----------|-------| +| `sites/promo/` | Next.js 15 | 0 | +| `sites/docs/` | Astro 6 + Starlight | 0 | + +### 2.6 Rust Libraries + +| Library | Source Files | Test Files | Status | +|---------|-------------|------------|--------| +| `droplet` | 11 | tests.rs + pipeline_test.rs | ✅ Partial | +| `droplet_types` | 1 | 0 | ❌ Zero | +| `libarchive` | 5 | 3 | ✅ Moderate | +| `native_model` | 8 | ~12 | ✅ Good | + +--- + +## 3. Critical Untested Modules + +### 3.1 Server API Routes — 100% UNTESTED + +**All 100 API route handlers have ZERO tests.** No route handler files have corresponding test files. + +| Route Group | Files | Risk Level | Reason | +|-------------|-------|------------|--------| +| `server/server/api/v1/auth/` | 12 routes | 🔴 CRITICAL | Auth bypass, session hijack, MFA bypass | +| `server/server/api/v1/admin/` | 66 routes | 🔴 CRITICAL | Admin privilege escalation, data corruption | +| `server/server/api/v1/client/` | ~15 routes | 🔴 CRITICAL | Game distribution abuse, auth bypass | +| `server/server/api/v1/collection/` | ~8 routes | 🟠 HIGH | Data mutation, ownership bypass | +| `server/server/api/v1/object/` | 4 routes | 🟠 HIGH | Storage abuse, object access control | +| `server/server/api/v1/screenshots/` | 5 routes | 🟠 HIGH | Unauthorized content access | +| `server/server/api/v1/notifications/` | 5 routes | 🟡 MEDIUM | Logic bugs | +| `server/server/api/v1/store/` | ~5 routes | 🟡 MEDIUM | Store display | +| `server/server/api/v1/news/` | 3 routes | 🟡 MEDIUM | Content display | +| `server/server/api/v1/user/` | ~10 routes | 🟠 HIGH | Profile/token management | +| `server/server/api/v1/settings/` | 1 route | 🟡 MEDIUM | Config leaks | +| `server/server/api/v1/task/` | 1 route | 🟡 MEDIUM | Task status | +| Other misc (health, index, token, setup, etc.) | ~5 routes | 🟡 MEDIUM | — | + +### 3.2 Server Internal Business Logic — 60% UNTESTED + +| Module | Path | Files | Has Tests? | Risk | +|--------|------|-------|------------|------| +| **Auth** | `internal/auth/` | 5 files | Partial (webauthn, totp, passwordHash tested; oidc/ UNTESTED) | 🔴 CRITICAL | +| **Objects/Storage** | `internal/objects/` | 3 files | ❌ Zero | 🔴 CRITICAL | +| **Library/Manifest** | `internal/library/` | 5 files | ❌ Zero | 🔴 CRITICAL | +| **Screenshots** | `internal/screenshots/` | 1 file | ❌ Zero | 🟠 HIGH | +| **Saves** | `internal/saves/` | 1 file | ❌ Zero | 🟠 HIGH | +| **Notifications** | `internal/notifications/` | 1 file | ❌ Zero | 🟠 HIGH | +| **News** | `internal/news/` | 1 file | ❌ Zero | 🟠 HIGH | +| **Cache** | `internal/cache/` | 2 files | ❌ Zero | 🟡 MEDIUM | +| **Clients** | `internal/clients/` | 4 files | ❌ Zero (ca-blacklist tested) | 🟠 HIGH | +| **Config** | `internal/config/` | 2 files | ❌ Zero | 🟡 MEDIUM | +| **Tasks** | `internal/tasks/` | 6 files | ❌ Zero | 🟠 HIGH | +| **Services (Torrential)** | `internal/services/` | 4 files | ❌ Zero | 🟠 HIGH | +| **User Stats** | `internal/userstats/` | 1 file | ❌ Zero | 🟡 MEDIUM | +| **User Library** | `internal/userlibrary/` | 1 file | ❌ Zero | 🟠 HIGH | +| **Session (DB/Cache)** | `internal/session/` | 3 files | Partial (only memory tested) | 🟠 HIGH | +| **Database** | `internal/db/` | 1 file | ❌ Zero | 🔴 CRITICAL | +| **System Data** | `internal/system-data/` | 1 file | ❌ Zero | 🟡 MEDIUM | +| **Utilities** | `internal/utils/` | 6 files | Partial (prioritylist, tuple, array, h3 tested; files, query, handlefileupload, parseplatform UNTESTED) | 🟡 MEDIUM | + +### 3.3 Desktop Rust — 90% UNTESTED + +| Crate | Source Files | Tests | Risk | +|-------|-------------|-------|------| +| `download_manager/` | 11 files | 0 | 🔴 CRITICAL — file download, queue, progress | +| `cloud_saves/` | 11 files | 0 | 🔴 CRITICAL — save data integrity | +| `games/` | 11 files | 0 | 🔴 CRITICAL — game library, scanning, state | +| `remote/` | 7 files | 0 | 🔴 CRITICAL — network auth, cache, requests | +| `process/` | 6 files | 0 | 🟠 HIGH — process management | +| `client/` | 5 files | 0 | 🟠 HIGH — user/autostart | +| `Tauri root` | 11 files | 0 | 🔴 CRITICAL — app startup, updates, scheduler | + +### 3.4 CLI — 70% UNTESTED + +| Module | Tests | Risk | +|--------|-------|------| +| `commands/upload/` | 0 | 🔴 CRITICAL — upload pipeline | +| `commands/connect/` | 0 | 🔴 CRITICAL — S3/connect config | +| `logging.rs` | 0 | 🟡 MEDIUM | +| `manifest.rs` | 1 integration test | 🟢 Partial | +| `cli.rs` | 0 | 🟡 MEDIUM | + +### 3.5 Frontend (Server + Desktop) — 99% UNTESTED + +| Area | Files | Tests | Risk | +|------|-------|-------|------| +| Server components | 72 .vue | 2 tests | 🟠 HIGH | +| Server pages | 54 .vue | 0 | 🟠 HIGH | +| Server composables | 16 .ts | 0 (partially covered by unit tests) | 🟡 MEDIUM | +| Desktop (Nuxt) pages | 24 .vue | 0 | 🟡 MEDIUM | +| Sites (promo + docs) | Unknown | 0 | 🟢 LOW (mostly static) | + +--- + +## 4. Missing Test Types + +### 4.1 Server (TypeScript) + +| Test Type | Current | Required For | +|-----------|---------|-------------| +| **Unit tests (business logic)** | 15 files | Internal modules (71 files, ~50 untested) | +| **API route handler tests** | **0** | **All 100 routes** | +| **Integration tests (DB)** | 5 files | Auth flows, object storage, library operations | +| **Component tests (Vue)** | 2 files | 72 components (70 untested) | +| **Page tests (Vue)** | **0** | 54 pages | +| **E2E tests** | 1 spec (health only) | Auth flows, game installation, admin operations | +| **Security tests** | 3 files (auth security) | Injection, CSRF, rate limiting, session management | + +### 4.2 CLI (Rust) + +| Test Type | Current | Required For | +|-----------|---------|-------------| +| **Unit tests** | 0 inline | All 16 src files | +| **Integration tests** | 2 files (manifest, config) | Upload commands, connect flows | +| **CLI argument tests** | 0 | clap argument parsing, subcommands | + +### 4.3 Desktop (Rust) + +| Test Type | Current | Required For | +|-----------|---------|-------------| +| **Unit tests** | ~2 inline modules | 7 crates × 5-11 files each | +| **Integration tests** | 0 | Cross-crate flows (download → write → verify) | +| **State machine tests** | 0 | Games state, download state, process lifecycle | + +--- + +## 5. Specific Recommendations + +### PRIORITY 1 — CRITICAL (Test Immediately) + +1. **Add API route handler tests for ALL auth routes** (12 files in `server/server/api/v1/auth/`) because authentication is the security boundary of the entire platform. Use the existing MSW mock infrastructure and h3 factory pattern already proven in `test/unit/h3-factory.test.ts`. + +2. **Add tests for `server/server/internal/objects/`** (fsBackend, objectHandler, transactional) because these handle ALL game file storage — data loss/corruption here is unrecoverable. + +3. **Add tests for `server/server/internal/db/database.ts`** because it's the Prisma client singleton that every data operation depends on. A misconfigured connection pool affects every route. + +4. **Add tests for `desktop/src-tauri/games/`** (library, scan, state, downloads, collections) because game detection and download management are the core desktop features with zero coverage. + +5. **Add tests for `desktop/src-tauri/cloud_saves/`** (backup_manager, metadata, resolver) because save data loss is user-facing and unrecoverable. + +### PRIORITY 2 — HIGH (Test Soon) + +6. **Add API route handler tests for admin routes** (`server/server/api/v1/admin/` — 66 files) because admin operations mutate critical system data (games, users, settings, library sources). + +7. **Add tests for `server/server/internal/library/`** because library management (manifest, providers, flat/filesystem providers) determines how games are discovered and served. + +8. **Add tests for `server/server/internal/clients/`** (handler, event-handler, capabilities) because client-server protocol correctness affects all desktop ↔ server communication. + +9. **Add tests for `server/server/internal/tasks/`** (registry, group, index) because background tasks handle integrity checks, session cleanup, and invitation processing. + +10. **Add upload command tests for CLI** (`cli/src/commands/upload/`) because file upload is the primary CLI workflow. + +11. **Add S3 connect tests for CLI** (`cli/src/commands/connect/`) because S3 configuration errors block the entire onboarding flow. + +### PRIORITY 3 — MEDIUM (Add When Touching Code) + +12. **Add tests for `server/server/internal/auth/oidc/`** before any OIDC changes — currently completely untested despite being an auth boundary. + +13. **Add tests for `server/server/internal/screenshots/`, `saves/`, `notifications/`, `news/`** when implementing features in these areas. + +14. **Add component tests for Vue components** when refactoring UI. Focus on `Modal/*`, `Auth/*`, `GameEditor/*` first. + +15. **Add tests for `desktop/src-tauri/remote/`** (auth, cache, requests, server_proto) because network errors degrade the entire desktop experience. + +16. **Add tests for `desktop/src-tauri/download_manager/`** because concurrent download queue logic is prone to race conditions. + +17. **Add tests for `desktop/src-tauri/process/`** because process lifecycle management (start/stop/kill game processes) affects system stability. + +### PRIORITY 4 — LOW (Add On Cleanup) + +18. **Add inline unit tests for `cli/src/commands/connect/`** sub-modules (speedtest, interactive, configurable, config_option, s3). + +19. **Add tests for `server/composables/`** utility functions when refactoring frontend code. + +20. **Add tests for `libraries/droplet_types/`** — single file lib, trivial to add basic encoding/decoding tests. + +--- + +## 6. Test Pattern Summary + +### Server Tests (TypeScript) + +| Pattern | How | +|---------|-----| +| **Unit test imports** | Import function directly, mock dependencies with `vi.mock()` | +| **Nitro globals** | Stubbed in `test/setup.ts` (`defineEventHandler`, `getHeader`, `readBody`, etc.) | +| **External HTTP** | MSW (Mock Service Worker) with `setupTestMocks()` / `teardownTestMocks()` | +| **Config** | `systemConfig` mock at top of test file before imports | +| **H3 handler testing** | Use `mockH3Handler()` pattern from `test/utils/h3.ts` | + +### Rust Tests (CLI + Desktop + Libraries) + +| Pattern | How | +|---------|-----| +| **Inline tests** | `#[cfg(test)] mod tests { ... }` in source files | +| **Integration tests** | `tests/` directory at crate root | +| **Test framework** | Built-in `#[test]`, `cargo test` | +| **Mocking** | Manual trait-based or struct mock pattern | + +### E2E Tests (Server) + +| Pattern | How | +|---------|-----| +| **Framework** | Playwright | +| **Location** | `server/test/e2e/` | +| **Dev server** | Auto-started by Playwright config | +| **Port** | 4000 (configurable via `E2E=true pnpm dev`) | +| **Current** | 1 spec (health endpoint only) | + +--- + +## 7. Coverage Gap Heatmap + +``` +server/server/api/v1/auth/ ████████████████████ (12 routes, 0 tests) +server/server/api/v1/admin/ ████████████████████ (66 routes, 0 tests) +server/server/api/v1/client/ ████████████████████ (15 routes, 0 tests) +server/server/api/v1/collection/ ████████████████████ (8 routes, 0 tests) +server/server/internal/objects/ ████████████████████ (3 files, 0 tests) +server/server/internal/library/ ████████████████████ (5 files, 0 tests) +server/server/internal/services/ ████████████████████ (5 files, 0 tests) +server/server/internal/clients/ ████████████████████ (4 files, 0 tests) +server/server/internal/tasks/ ████████████████████ (6 files, 0 tests) +server/server/internal/auth/oidc/ ████████████████████ (1 file, 0 tests) +server/server/internal/db/ ████████████████████ (1 file, 0 tests) +desktop/src-tauri/games/ ████████████████████ (11 files, 0 tests) +desktop/src-tauri/cloud_saves/ ████████████████████ (11 files, 0 tests) +desktop/src-tauri/download_manager/ ████████████████████ (11 files, 0 tests) +desktop/src-tauri/remote/ ████████████████████ (7 files, 0 tests) +desktop/src-tauri/process/ ████████████████████ (6 files, 0 tests) +desktop/src-tauri/client/ ████████████████████ (5 files, 0 tests) +cli/src/commands/upload/ ████████████████████ (2 files, 0 tests) +cli/src/commands/connect/ ████████████████████ (6 files, 0 tests) +server/server/internal/auth/ ████░░░░░░░░░░░░░░░░ (3/5 tested) +server/server/internal/session/ ████░░░░░░░░░░░░░░░░ (1/3 tested) +server/test/unit/ ████████████████████ (15 files, good) +server/test/mocks/ ████████████████████ (MSW setup, good) +``` + +**Legend:** ██ = red (untested), ░░ = green (tested) + +--- + +## 8. What's Working Well + +1. **MSW mock infrastructure** is solid — OIDC + all metadata providers mocked, `onUnhandledRequest: "error"` catches unmocked external calls. +2. **Auth security tests** (webauthn, OIDC escalation, session fixation, CA blacklist) cover important vulnerability patterns. +3. **libarchive** has decent coverage (60%) with 3 test files. +4. **native_model** has excellent coverage (150%+) with a dedicated test crate. +5. **H3 handler test utility** (`test/utils/h3.ts`) provides a pattern for testing API handlers — just needs to be applied. +6. **Nuxt test environment** with fork pool isolation prevents cross-test state leaks. +7. **prioritylist** tests (unit + integration) cover the core data structure well. +8. **Coverage scope** is correctly scoped to Nitro backend only, excluding frontend code. + +--- + +## 9. Summary + +**Overall test coverage: ~10% of codebase has tests.** + +- **Server backend:** ~30% of business logic tested, 0% of API routes tested +- **Server frontend:** ~1% tested +- **CLI:** ~13% tested +- **Desktop Rust:** ~3% tested +- **Desktop Nuxt:** 0% tested +- **Sites:** 0% tested +- **Libraries:** native_model (good), libarchive (moderate), droplet (low), droplet_types (zero) + +**Most urgent: Add API route handler tests utilizing the existing h3 factory + MSW infrastructure.** diff --git a/AGENTS.md b/AGENTS.md index 0508cca0c..c0b95cf73 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -98,6 +98,33 @@ cargo clippy --all-targets --all-features -- -D warnings ## Pre-commit Hooks (ACTUAL BEHAVIOR) + +## Fallow local gate + +Before any `git commit` or `git push`, run `fallow audit --format json --quiet --explain --gate-marker agent`. If the verdict is `fail`, fix the reported findings before retrying. Treat JSON runtime errors like `{ "error": true, ... }` as non-blocking. + +Audit defaults to `gate=new-only`: only findings introduced by the current changeset affect the verdict. Inherited findings on touched files are reported under `attribution` and annotated with `introduced: false`, but do not block the commit. Set `[audit] gate = "all"` in `fallow.toml` to gate every finding in changed files. + +For non-skill agents, treat the task map below as the local onboarding source: run the listed fallow command before destructive edits, before commits, and before pull request handoff. + +## Fallow task map + +| When the agent is about to... | Run | +|---|---| +| delete an "unused" export or file | `fallow dead-code --trace :` | +| delete an "unused" dependency | `fallow dead-code --trace-dependency ` | +| commit or open a PR | `fallow audit --base ` | +| prioritize refactoring | `fallow health --hotspots --targets` | +| ask who owns code | `fallow health --ownership` | +| check untested-but-reachable code | `fallow health --coverage-gaps` | +| consolidate duplication | `fallow dupes --trace dup:` | +| find feature flags | `fallow flags` | +| check which architecture rules apply to a file before changing it | `fallow guard ` | +| surface security candidates | `fallow security` | +| understand a finding | `fallow explain ` | +| scope a monorepo | `--workspace / --changed-workspaces ` (global flags, prefix any command) | + + - `.husky/pre-commit` (root, ACTIVE): runs `pnpm --filter drop lint-staged && pnpm --filter drop typecheck` - `--filter drop` = `server/` (filter targets the `drop` package name; see `server/package.json`). - lint-staged patterns: `*.{ts,vue,json,css,scss,yaml,yml,md,mjs,cjs}` → eslint --fix + prettier --write. `*.rs` → `cargo fmt -- `. diff --git a/CLAUDE.md b/CLAUDE.md index c161378d7..0ed910937 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -1,6 +1,6 @@ # Drop — Agent Behavioral Rules -Cross-tool behavioral rules. Read by Claude Code, Cursor, Codex, OpenCode, and other AI coding agents. This file governs *behavior*; the dense technical reference is in `AGENTS.md`. +Cross-tool behavioral rules. Read by Claude Code, Cursor, Codex, OpenCode, and other AI coding agents. This file governs _behavior_; the dense technical reference is in `AGENTS.md`. ## After editing any file, format it immediately @@ -32,7 +32,7 @@ pnpm --filter drop test pnpm --filter drop format:check ``` -The pre-commit hook runs lint-staged + `pnpm test` automatically. If pre-commit fails, fix the issue, then `git commit --amend --no-edit` (if no new files) or re-stage and commit. +The pre-commit hook runs lint-staged + `pnpm --filter drop typecheck` automatically. Tests run on pre-push via `pnpm --filter drop test`. If pre-commit fails, fix the issue, then `git commit --amend --no-edit` (if no new files) or re-stage and commit. ## Do not commit @@ -50,6 +50,7 @@ ALWAYS pnpm. NEVER yarn or npm. The project has `packageManager: pnpm@11.17.0` e ## Run commands from workspace directory For `cd server && pnpm test`-style commands, either: + - `pnpm --filter drop diff --git a/server/pages/admin/settings/index.vue b/server/pages/admin/settings/index.vue index 4fab0f3f3..21eb5483a 100644 --- a/server/pages/admin/settings/index.vue +++ b/server/pages/admin/settings/index.vue @@ -120,7 +120,7 @@ const mCustomLogoObjectId = ref( settings.value.generalSettings.mLogoObjectId || "", ); -const updateServerName = (event: InputEvent) => { +const updateServerName = (event: Event) => { settings.value.generalSettings.serverName = (event.target as HTMLInputElement)?.value || ""; allowSave.value = true; @@ -168,7 +168,7 @@ function updateLogo(response: { id: string }) { allowSave.value = true; } -const updateFormLogo = (event: InputEvent | null) => { +const updateFormLogo = (event: Event | null) => { settings.value.generalSettings.mLogoObjectId = (event?.target as HTMLInputElement)?.value || null; allowSave.value = true; diff --git a/server/pages/library/game/[id]/index.vue b/server/pages/library/game/[id]/index.vue index 441447c65..03a6f2590 100644 --- a/server/pages/library/game/[id]/index.vue +++ b/server/pages/library/game/[id]/index.vue @@ -140,21 +140,6 @@ const game = computed(() => { const descriptionHTML = computed(() => micromark(game.value.mDescription ?? ""), ); - -// const currentImageIndex = ref(0); - -// function nextImage() { -// if (!game.value?.mImageCarousel) return; -// currentImageIndex.value = -// (currentImageIndex.value + 1) % game.value.mImageCarousel.length; -// } - -// function previousImage() { -// if (!game.value?.mImageCarousel) return; -// currentImageIndex.value = -// (currentImageIndex.value - 1 + game.value.mImageCarousel.length) % -// game.value.mImageCarousel.length; -// }