refactor: overengineering-audit cleanup (dead code + honest GA apply + telemetry)

decisions/2026-06-06-decommission-backend-guild-automation-execution-service.md

@@ -0,0 +1,83 @@
+# ADR 2026-06-06 — Decommission backend GuildAutomationExecutionService
+
+**Status:** Accepted
+**Via:** `/research-and-decide` (critic adjudicated delete-vs-retain; landed on delete — no flip)
+**Relates to:** [2026-05-19-guild-automation-module-executors](2026-05-19-guild-automation-module-executors.md) (the migration this file is being superseded by)
+
+## Context
+
+The overengineering audit (2026-06-06) flagged `packages/backend/src/services/GuildAutomationExecutionService.ts`
+(1,333 LOC) as the single largest removable file in the repo. Investigation made the call
+non-trivial, so it went through research-and-decide.
+
+Verified current state:
+
+- **Zero production callers.** The only reference anywhere is its own unit test. The backend
+  route `routes/guildAutomation.ts` (`/automation/plan`, `/automation/apply`, `/capture`) now
+  imports the **shared** `guildAutomationService` → `GuildAutomationOrchestrator`, not this file.
+  The bot already deleted its equivalent.
+- **The migration that supersedes it is incomplete.** The shared package wires **3 of 7** module
+  executors (autoMessages, moderation, reactionRoles). The other 4 — Roles, Channels, Onboarding,
+  CommandAccess — have no shared executor yet.
+- This old file holds the **only** full implementations of those 4 modules' apply/remap logic
+  (`applyRolesAndChannels`, `applyOnboardingModule`, `pruneStaleRoles/Channels`,
+  `remapRolesSection`, `remapOnboardingSection`, `remapCommandAccessSection`, …). It also
+  instantiates the 3 new shared executors — i.e. it was the migration's bridge/staging ground,
+  now stranded with no caller.
+- Its unit test still runs in CI, producing **green coverage for code nothing calls**.
+- Solo maintainer; git history is intact and the file is recoverable via `git show <sha>:<path>`.
+
+## Decision
+
+**Delete the file now** (do not retain until the migration completes), specifically:
+
+1. Delete `packages/backend/src/services/GuildAutomationExecutionService.ts` and its test
+   `packages/backend/tests/unit/services/GuildAutomationExecutionService.test.ts` in one PR
+   (per the repo's "remove the feature, sweep its tests in the same PR" convention).
+2. Record the decommission here and cross-link from the migration ADR. The 4 un-migrated
+   modules' reference logic lives immutably in git; future executor PRs recover it via
+   `git show 71805799:packages/backend/src/services/GuildAutomationExecutionService.ts`
+   rather than from stranded live code.
+
+Rationale: the file is **already** dead (no callers), so deletion is runtime-safe — it changes
+nothing at execution time. Remaining executors are ported **fresh per module**, not copy-pasted
+from this file, so it is a _reference at best_, and git serves that better than drift-prone live
+code. "Retain until migration completes" is a weak contract that historically becomes "retain
+forever" on this repo (cf. the per-guild-toggle orphan that sat dead for 2 weeks —
+[2026-05-19-retire-per-guild-feature-toggles](2026-05-19-retire-per-guild-feature-toggles.md)),
+and the passing test is a false coverage signal that invites future devs to treat dead code as
+load-bearing.
+
+## Alternatives considered
+
+- **Retain untouched until the migration reaches 7/7, then delete** — rejected. No runtime
+  benefit (already uncalled); high risk of becoming permanent if the migration stalls; the
+  stranded file drifts from the orchestrator and misleads. Git already preserves the reference.
+- **Retain but quarantine** (rename `*.DEPRECATED.ts`, add `@deprecated` header, exclude its
+  test from CI) — rejected as a worse version of delete: keeps 1,333 LOC of inert code and the
+  confusion surface for a benefit (reference) that git already provides. Acceptable fallback
+  only if the revisit-trigger below fires.
+- **Extract the 4 un-migrated modules' logic into a holding module now** — rejected as
+  premature; that work belongs in each module's executor PR, not a speculative pre-extraction.
+
+## Consequences
+
+- **Positive:** −1,333 LOC (+ its ~2,174-LOC test); removes a false coverage signal; removes a
+  drift/confusion source; signals "this path is obsolete, read the migration ADR + executor PRs."
+- **Negative:** Porting the remaining 4 executors references git history instead of live code —
+  one extra `git show` per module (<5 min). Mitigated by recording the pre-deletion SHA here.
+- **Neutral / out of scope:** Whether the 4 un-migrated modules currently apply at all through
+  the new orchestrator is a **separate possible functional gap** — this file's deletion neither
+  causes nor worsens it (the file has no callers). Tracked as a follow-up, not part of this PR.
+
+## Revisit when
+
+Re-open (and prefer the quarantine fallback) if **any** of:
+
+1. **No module-executor PR merges by 2026-06-20** → the migration has stalled; a live reference
+   may regain value over git archaeology.
+2. A Roles/Channels/Onboarding/CommandAccess executor PR copies **2+ functions verbatim** from
+   the old file → it was a template, not just a reference; reconsider keeping it until 7/7.
+3. The separate "do the 4 modules apply through the orchestrator at all?" follow-up reveals the
+   old service was the _de facto_ live apply path for them after all (contradicting the
+   zero-caller finding) → halt deletion, re-investigate.

decisions/2026-06-06-guild-automation-migration-freeze-and-instrument.md

@@ -0,0 +1,81 @@
+# ADR 2026-06-06 — Freeze the Guild Automation executor migration; instrument usage before completing or descoping
+
+**Status:** Accepted
+**Via:** `/research-and-decide` (critic adjudicated complete vs freeze vs descope vs remove; landed on freeze-as-holding-state + measure — no flip)
+**Relates to / umbrella for:** PRD #1059 · [2026-05-19-guild-automation-module-executors](2026-05-19-guild-automation-module-executors.md) · [2026-06-06-decommission-backend-guild-automation-execution-service](2026-06-06-decommission-backend-guild-automation-execution-service.md) · [2026-06-06-web-guild-automation-apply-plan-only](2026-06-06-web-guild-automation-apply-plan-only.md)
+
+## Context
+
+Two prior decisions this session (decommission the dead 1,333-LOC backend service; make web
+"apply" honest/plan-only) exposed a bigger question: **is the Guild Automation Module Executor
+migration (PRD #1059) worth completing at all?**
+
+Verified state:
+
+- **Footprint ≈ 4,900 hand-written LOC** (≈5.5% of the repo): shared core 2,084
+  (manifest/diff/3-of-7 executors/orchestrator/repository); backend 1,531 (incl. the 1,333-LOC
+  monolith already decided for deletion + a 198-LOC route); bot apply 548; frontend page+api 726.
+- **Migration stalled at 3 of 7 executors**, all DB-only. The `DiscordWriteAdapter` /
+  `DiscordRestAdapter` seam — the linchpin that lets the _backend_ apply Discord-writing modules
+  and that collapses the bot/backend duplication — was **never built**. The 4 remaining modules
+  (Roles, Channels, Onboarding, CommandAccess) are mostly Discord-writing and need it. ADR estimate
+  to finish: "≥4 PRs"; the adapter design carries flagged coupling risk and has had **no design review**.
+- A **working apply path exists**: the bot `/guildconfig apply` command. Only the web/backend path
+  is hollow (now being made plan-only).
+- The subsystem has been a recurring **maintenance sink** (a 1,333-LOC dead service; a P1
+  misleading-apply bug; a stalled migration).
+- **Usage is UNMEASURED.** No telemetry on automation apply/plan. The web page is prominent in the
+  sidebar (discoverable) but we do not know how many guilds use it. "Near-zero usage" is an
+  **assumption**, and the choice between completing (A) and descoping/removing (C/D) hinges on it.
+
+## Decision
+
+**Freeze the migration as the holding state, fix the accumulated debt, and instrument usage. Defer
+the A-vs-C/D choice to explicit data/design gates — do not make a large irreversible bet blind.**
+
+Holding-state actions (small, mostly already decided):
+
+1. Implement [2026-06-06-decommission-backend-guild-automation-execution-service] — delete the dead
+   1,333-LOC monolith + its test.
+2. Implement [2026-06-06-web-guild-automation-apply-plan-only] — honest web UX + plan-only run record
+   (kills the false "completed"/autoApplied audit trail).
+3. **NEW — instrument usage:** add lightweight per-guild counters on the web `/automation/plan` and
+   `/automation/apply` attempts and the bot `/guildconfig apply` subcommand (count + guildId, no PII).
+   This is the missing fact that should drive A-vs-C/D. Cheap (≈ middleware + one log/metric).
+4. Reflect the freeze on PRD #1059 (status: frozen pending the gates below); do not open further
+   executor PRs until a gate fires.
+
+This explicitly does **not** complete the migration (A) now, nor descope/remove (C/D) now.
+
+## Alternatives considered
+
+- **A — Complete the migration now.** Rejected as the immediate move: ≥4 PRs of speculative effort
+  (incl. designing the never-built adapter, with flagged coupling risk and no design review) for a
+  feature of **unknown demand**, in a solo-maintainer context. Kept as the destination if a gate fires.
+- **C — Descope/simplify (rip out web+backend orchestration + parity/cutover, keep bot path).**
+  Rejected _now_: parity/cutover is entangled (non-trivial surgery), and removing a shipped,
+  discoverable web feature on an **assumed**-low usage could destroy real value. Becomes the likely
+  path if telemetry shows the web path is unused.
+- **D — Remove the whole feature (bot + web).** Rejected: the bot `/guildconfig apply` is a working,
+  shipped feature; removing it blind is user-hostile. Only reconsider if telemetry shows ~zero usage
+  of _both_ paths.
+
+## Consequences
+
+- **Positive:** stops the bleeding cheaply (delete dead code, honest UX, telemetry) without betting
+  4+ PRs on unknown demand; converts a speculative A-vs-C/D argument into a data-driven one; keeps the
+  working bot path intact.
+- **Negative:** the subsystem stays half-built — web apply is plan-only and ~3,500 LOC of frozen
+  machinery is carried until a gate fires. "Freeze" can decay into "permanent" (the explicit gates +
+  the 2026-07-06 sunset trigger below exist to prevent that).
+- **Neutral:** completing the migration remains fully possible; nothing here forecloses A.
+
+## Revisit when (the gates that resolve the deferred A-vs-C/D)
+
+1. **Usage telemetry shows >5% of active guilds use the web apply/plan path in any 7-day window** →
+   escalate **A**: do the `DiscordWriteAdapter` design review, then build the seam + 4 executors + wire
+   backend/web apply.
+2. **A `DiscordWriteAdapter` design review (pre-mortem + adapter test plan) is completed and passes** →
+   confidence is high enough to proceed with **A** without waiting on usage data.
+3. **Neither gate fires by 2026-07-06 (30 days)** → close PRD #1059 as deferred and run a focused
+   **C-vs-D** decision (descope vs remove) using the telemetry collected by then.

decisions/2026-06-06-web-guild-automation-apply-plan-only.md

@@ -0,0 +1,82 @@
+# ADR 2026-06-06 — Web Guild Automation "apply" is plan-only until the executor migration lands
+
+**Status:** Accepted
+**Via:** `/research-and-decide` (critic adjudicated A vs B; landed on B + a data-integrity fix — no flip)
+**Relates to:** [2026-05-19-guild-automation-module-executors](2026-05-19-guild-automation-module-executors.md) · PRD #1059 (complete the executors) · [2026-06-06-decommission-backend-guild-automation-execution-service](2026-06-06-decommission-backend-guild-automation-execution-service.md)
+
+## Context
+
+Confirmed P1, found while investigating the decommission ADR's "possible functional gap":
+
+The web dashboard's Guild Automation page has **Apply** + **Reconcile** buttons. `Apply`
+calls `POST /automation/apply` → `GuildAutomationOrchestrator.createApplyRun()`, which:
+computes a plan, writes a `GuildAutomationRun` with **`status: 'completed'`** and a populated
+**`autoAppliedOperations`** list, and returns — **without invoking any executor or mutating
+the Discord guild / settings tables** (verified: zero `.apply()` calls anywhere in
+`shared/services/guildAutomation/`; the repository does only Prisma bookkeeping). The web UI
+then toasts **"Changes applied."**
+
+So the web Apply is a **no-op that reports success**, and the run record is a **false audit
+trail**. The only code that actually mutates a guild is the bot's `/guildconfig apply` slash
+command (`bot/utils/guildAutomation/applyPlan.ts`, direct discord.js writes + 3 shared
+executors); there is **no bridge** from a backend run to bot application.
+
+Root cause: the Module Executor migration replaced the backend's executing
+`GuildAutomationExecutionService` with a **plan-only** orchestrator and relocated execution to
+the bot, but the **web UI was never updated**. The intended end-state (ADR 2026-05-19) _does_
+have the backend applying via a `DiscordWriteAdapter` → `DiscordRestAdapter` seam — but that
+seam **does not exist yet** (never built), and 4 of 7 executors (Roles, Channels, Onboarding,
+CommandAccess — the Discord-writing ones that need the adapter) are unbuilt. ADR estimate to
+finish: "≥4 PRs" (PRD #1059).
+
+## Decision
+
+**B — make the web UX honest now; defer real web apply (A) to the executor migration.**
+
+Immediate fix (one small PR):
+
+1. **Frontend:** relabel the web **Apply** action away from claiming application (e.g. "Record
+   plan" / present it as a dry-run + parity record); replace the `"Changes applied."` toast with
+   one that states a plan was recorded and that applying happens via `/guildconfig apply` in
+   Discord. Same for **Reconcile**.
+2. **Backend/shared (data-integrity, do regardless of A/B):** stop recording a **false
+   completion** — `createApplyRun` must not mark the run `completed` with `autoAppliedOperations`
+   populated when nothing executed. Record it as **plan-only / not-applied** (a planned/queued
+   status or an explicit `applied: false`, per the run model) with `autoAppliedOperations: []`.
+3. Verify the bot `/guildconfig apply` path and `getStatus`/`listRuns` displays are unaffected.
+
+Real backend-driven web apply (**Option A**) is the _correct end-state_ and is **deferred to
+PRD #1059**: it requires building the `DiscordWriteAdapter` + backend `DiscordRestAdapter` and
+wiring `createApplyRun` to invoke executors. When that lands, re-enable the web Apply label/toast.
+
+## Alternatives considered
+
+- **A — make web apply real now.** Rejected as the _immediate_ fix: it means finishing a
+  multi-PR migration (the adapter seam + 4 executors, "≥4 PRs") just to correct a misleading
+  toast, for a niche feature with ~near-zero web usage. It is the right _destination_ (kept as
+  the deferred end-state), not a P1 hotfix. Violates the no-big-bang-mid-hotfix instinct.
+- **Hybrid — wire only the 3 built DB-only executors into backend apply now.** Rejected, worse
+  than the status quo: web apply would mutate 3 of 7 modules and silently skip Roles/Channels/
+  Onboarding/CommandAccess, splitting the lie across modules and making "what actually changed?"
+  harder to reason about.
+
+## Consequences
+
+- **Positive:** kills the misleading "Changes applied" success and the false `autoAppliedOperations`
+  audit trail in hours, not weeks; keeps PRD #1059 independent; matches current reality (bot is
+  the working apply path); fully reversible when A lands.
+- **Negative:** the web's headline "apply" capability is openly downgraded to plan/record until
+  the migration completes — users must apply via the Discord `/guildconfig` command.
+- **Neutral:** a back-fill of any pre-existing falsely-`completed` web runs is _optional_ — given
+  near-zero usage it's likely unnecessary; decide when implementing.
+
+## Revisit when
+
+- **PRD #1059 reaches the adapter-seam stage** → implement Option A: build
+  `DiscordWriteAdapter`/`DiscordRestAdapter`, wire `createApplyRun` to invoke executors, restore
+  the web Apply label/toast. (This ADR's stopgap ends here.)
+- **Web-apply usage proves non-trivial** (e.g. >5% of guilds attempt it) → escalate A's priority
+  ahead of the rest of #1059.
+- **The `DiscordWriteAdapter` design reveals insurmountable Discord-state coupling** → re-litigate
+  whether the backend should apply at all (vs web → bot-bridge), per ADR 2026-05-19's own
+  no-go condition.

packages/backend/src/middleware/validate.ts

@@ -6,6 +6,14 @@ import { z } from 'zod'
 // preserves caller-side inference exactly.
 type Schema<TOutput> = z.ZodType<TOutput, unknown>
 
+function stripUnknownFields(data: object, allowedKeys: Set<string>): void {
+    for (const key of Object.keys(data)) {
+        if (!allowedKeys.has(key)) {
+            delete (data as Record<string, unknown>)[key]
+        }
+    }
+}
+
 export function validateBody<TOutput>(schema: Schema<TOutput>) {
     return (req: Request, res: Response, next: NextFunction) => {
         const result = schema.safeParse(req.body as unknown)
@@ -33,14 +41,11 @@ export function validateQuery<TOutput>(schema: Schema<TOutput>) {
             return res.status(400).json({ error: 'Validation failed', errors })
         }
 
-        // Strip unknown fields by reconstructing query with only schema keys
-        const dataKeys = new Set(Object.keys(result.data as object))
-        for (const key of Object.keys(req.query)) {
-            if (!dataKeys.has(key)) {
-                delete req.query[key]
-            }
-        }
-        // Assign validated data back (which includes transformations like coercion)
+        // Strip unknown fields and assign validated data back (which includes transformations like coercion)
+        stripUnknownFields(
+            req.query,
+            new Set(Object.keys(result.data as object)),
+        )
         Object.assign(req.query, result.data as object)
         next()
     }
@@ -57,14 +62,11 @@ export function validateParams<TOutput>(schema: Schema<TOutput>) {
             return res.status(400).json({ error: 'Validation failed', errors })
         }
 
-        // Strip unknown fields by reconstructing params with only schema keys
-        const dataKeys = new Set(Object.keys(result.data as object))
-        for (const key of Object.keys(req.params)) {
-            if (!dataKeys.has(key)) {
-                delete req.params[key]
-            }
-        }
-        // Assign validated data back (which includes transformations like coercion)
+        // Strip unknown fields and assign validated data back (which includes transformations like coercion)
+        stripUnknownFields(
+            req.params,
+            new Set(Object.keys(result.data as object)),
+        )
         Object.assign(req.params, result.data as object)
         next()
     }