Repository navigation
refactor(shared): split GuildAutomationService into Orchestrator + Repository - #982
Merged
LucasSantana-Dev merged 4 commits intoMay 24, 2026
Merged
Conversation
…pository - Create IGuildAutomationRepository interface for DB operations - Implement GuildAutomationRepository with all Prisma calls - Extract business logic into GuildAutomationOrchestrator - Slim service.ts to create dependency graph and export singleton Public API unchanged: guildAutomationService methods work identically. All tests pass. TypeScript strict mode enforced.
…d module - Extract toManifestDocument(), toJsonValue(), and isObject() to guildAutomationHelpers.ts - Remove duplicate implementations from GuildAutomationRepository and GuildAutomationOrchestrator - Fix unsafe cast in toJsonValue(): now round-trips through JSON.stringify/parse for runtime validation - Unify guard logic across both consumers (GuildAutomationRepository, GuildAutomationOrchestrator)
Contributor
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
github-actions
Bot
added
dependencies
|
Failed to generate code suggestions for PR |
|
Size Change: 0 B Total Size: 424 kB ℹ️ View Unchanged
|
|
Warning Review limit reached
Your plan currently allows 1 review/hour. Refill in 53 minutes and 40 seconds. Your organization has run out of usage credits. Purchase more in the billing tab. ⌛ How to resolve this issue?After more review capacity refills, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than trial, open-source, and free plans. In all cases, review capacity refills continuously over time. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR refactors guild automation into a repository interface, a Prisma-backed repository implementation, runtime validation/serialization helpers, and a stateful GuildAutomationOrchestrator with per-guild locking, plan/apply/cutover workflows, and run lifecycle management. ChangesGuild Automation Orchestrator & Repository
Sequence Diagram(s)sequenceDiagram
participant Client
participant Orchestrator as GuildAutomationOrchestrator
participant Repository as GuildAutomationRepository
participant PlanGen as createAutomationPlan
Client->>Orchestrator: createApplyRun(guildId)
Orchestrator->>Orchestrator: acquireLock(guildId)
Orchestrator->>Repository: getManifestRow(guildId)
Repository-->>Orchestrator: manifest + lastCapturedState
Orchestrator->>PlanGen: createAutomationPlan(desired, actual)
PlanGen-->>Orchestrator: plan + operations + protectedOperations
alt protectedOperations && !allowProtected
Orchestrator->>Repository: createPlanRecord(..., status=blocked)
else
Orchestrator->>Repository: createPlanRecord(..., status=running)
end
Orchestrator->>Orchestrator: releaseLock(guildId)
Orchestrator-->>Client: runMetadata + plan
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (5)
packages/shared/src/services/guildAutomation/service.ts (1)
51-51: 💤 Low valueConsider static import if circular dependencies are not a concern.
The dynamic import adds a slight overhead on each call (module resolution, even if cached). If this change was intentional to avoid circular dependencies or enable code splitting, it's fine. Otherwise, a static import at the top of the file would be marginally more efficient.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/services/guildAutomation/service.ts` at line 51, The dynamic import of createAutomationPlan inside service.ts (const { createAutomationPlan } = await import('./diff.js')) adds runtime overhead; replace it with a static top-level import (import { createAutomationPlan } from './diff.js') to eliminate per-call module resolution, unless you need the dynamic import to avoid a circular dependency or for code-splitting—if circular references exist, instead refactor to break the cycle or keep the dynamic import with a brief comment explaining why.packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts (3)
265-275: 💤 Low valueConsider explicit handling of undefined parity.
Spreading
manifest.paritywhen it may be undefined (line 267) works correctly but could be more explicit. While JavaScript spreading undefined results in no properties, the intent would be clearer with explicit handling.♻️ Optional refactor for clarity
const nextManifest: GuildAutomationManifestDocument = { ...manifest, parity: { - ...manifest.parity, + ...(manifest.parity ?? {}), cutoverReady: true, checklist: checklist.map((item) => ({ ...item, done: true, })), }, }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts` around lines 265 - 275, The code constructs nextManifest by spreading manifest.parity which can be undefined; make the intent explicit by defaulting parity before spreading — e.g., compute a parity base from manifest.parity (or {} if undefined) and then assign cutoverReady: true and checklist filled via checklist.map; update the block that builds nextManifest in GuildAutomationOrchestrator (the nextManifest variable) to use this explicit defaulting so parity is always an object rather than relying on spreading undefined.
112-119: 💤 Low valueConsider extracting severity thresholds as named constants.
The hardcoded thresholds (< 3, < 8) for drift severity classification would be more maintainable and self-documenting as named constants.
♻️ Suggested refactor
+const DRIFT_SEVERITY_LOW_THRESHOLD = 3 +const DRIFT_SEVERITY_MEDIUM_THRESHOLD = 8 + async createPlan( guildId: string, options?: { actualState?: GuildAutomationManifestInput initiatedBy?: string runType?: Extract<AutomationRunType, 'plan' | 'apply' | 'reconcile'> }, ) { // ... existing code ... for (const [moduleName, count] of Object.entries( plan.summary.byModule, )) { const severity: 'none' | 'low' | 'medium' | 'high' = count === 0 ? 'none' - : count < 3 + : count < DRIFT_SEVERITY_LOW_THRESHOLD ? 'low' - : count < 8 + : count < DRIFT_SEVERITY_MEDIUM_THRESHOLD ? 'medium' : 'high'🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts` around lines 112 - 119, Extract the numeric thresholds for drift severity into named constants (e.g., SEVERITY_THRESHOLD_LOW = 3 and SEVERITY_THRESHOLD_MEDIUM = 8) and replace the hardcoded comparisons in GuildAutomationOrchestrator where severity is computed (the ternary using count === 0 ? 'none' : count < 3 ? 'low' : count < 8 ? 'medium' : 'high') to use those constants; define the constants at module or class scope with brief comments and update the ternary/operator logic to reference SEVERITY_THRESHOLD_LOW and SEVERITY_THRESHOLD_MEDIUM so the thresholds are self-documenting and easier to change.
90-94: 💤 Low valueConsider avoiding redundant schema parsing.
Line 91 parses
options.actualStateeven when it may already be a validated manifest. While Zod parsing is idempotent and safe, it adds unnecessary overhead. Consider accepting a type that distinguishes validated vs. raw input, or document that callers should pass raw input here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts` around lines 90 - 94, The code redundantly calls guildAutomationManifestSchema.parse on options.actualState even when callers may pass an already-validated manifest; update the logic so parsing only happens for raw input: add an explicit discriminator (e.g., options.actualStateIsValidated boolean) or change the options type to accept a union (ValidatedManifest | RawManifest) and only call guildAutomationManifestSchema.parse when the value is the raw variant; keep the fallback to manifestRow.lastCapturedState via toManifestDocument unchanged and reference the variables actual, options.actualState, guildAutomationManifestSchema.parse, toManifestDocument, and manifestRow.lastCapturedState when making the change.packages/shared/src/services/guildAutomation/GuildAutomationRepository.ts (1)
283-288: ⚡ Quick winClamp
limitbefore querying runs.Passing
limitthrough unbounded can become an accidental hot query path. Guarding it at repository boundary keeps this endpoint safer under upstream misuse.♻️ Proposed guard
async listRuns(guildId: string, limit = 10) { + const safeLimit = Math.min(Math.max(limit, 1), 100) return this.prisma.guildAutomationRun.findMany({ where: { guildId }, orderBy: { createdAt: 'desc' }, - take: limit, + take: safeLimit, }) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@packages/shared/src/services/guildAutomation/GuildAutomationRepository.ts` around lines 283 - 288, The listRuns method currently forwards an unbounded limit to prisma.guildAutomationRun.findMany; clamp the incoming limit in GuildAutomationRepository.listRuns to a safe max (e.g., const MAX_RUNS = 100) and ensure it's at least 1 (use Math.max(1, Math.min(limit, MAX_RUNS)) or equivalent) before passing to the take option on prisma.guildAutomationRun.findMany so the DB query cannot be abused by large limits.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts`:
- Around line 13-44: The current lock via locks Map with LOCK_TTL_MS and
cleanupLocks can expire mid-operation causing race conditions; update the
locking strategy in GuildAutomationOrchestrator by adding a refresh/heartbeat
mechanism and ownership validation: modify acquireLock to store an owner token
and expiresAt in locks (use unique token per operation), add a
refreshLock(token, guildId) method that extends expiresAt periodically from
long-running tasks, ensure critical sections check that the stored token still
matches the operation's token before proceeding, and call releaseLock(guildId,
token) to only remove locks owned by that token; alternatively, if you prefer
the simpler path, increase LOCK_TTL_MS to a safe maximum and validate expiresAt
immediately before entering critical sections in methods that perform
apply/reconcile.
In `@packages/shared/src/services/guildAutomation/GuildAutomationRepository.ts`:
- Around line 67-98: The manifest upsert and run creation must be executed
atomically: wrap guildAutomationManifest.upsert and guildAutomationRun.create in
a single Prisma transaction so the run is only persisted if the manifest write
succeeds (use this.prisma.$transaction and run the upsert and create on the
transaction client, e.g., tx.guildAutomationManifest.upsert then
tx.guildAutomationRun.create), return or use the transactional results; apply
the same change for the second occurrence (the other grouped writes in
recordCapture and runCutover).
- Around line 254-270: The cutover path only writes the manifest JSON and the
run summary drops the provided checklist, causing version/checklist drift;
update the guildAutomationManifest.update call (guildAutomationManifest.update,
nextManifest, manifestRow) to also set the manifest row's version and checklist
columns from nextManifest.version and nextManifest.checklist, and include the
checklist (and optionally version) inside the guildAutomationRun.create summary
payload so the cutover run records the checklist that triggered the cutover
(guildAutomationRun.create, summary, nextManifest.checklist).
---
Nitpick comments:
In `@packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.ts`:
- Around line 265-275: The code constructs nextManifest by spreading
manifest.parity which can be undefined; make the intent explicit by defaulting
parity before spreading — e.g., compute a parity base from manifest.parity (or
{} if undefined) and then assign cutoverReady: true and checklist filled via
checklist.map; update the block that builds nextManifest in
GuildAutomationOrchestrator (the nextManifest variable) to use this explicit
defaulting so parity is always an object rather than relying on spreading
undefined.
- Around line 112-119: Extract the numeric thresholds for drift severity into
named constants (e.g., SEVERITY_THRESHOLD_LOW = 3 and SEVERITY_THRESHOLD_MEDIUM
= 8) and replace the hardcoded comparisons in GuildAutomationOrchestrator where
severity is computed (the ternary using count === 0 ? 'none' : count < 3 ? 'low'
: count < 8 ? 'medium' : 'high') to use those constants; define the constants at
module or class scope with brief comments and update the ternary/operator logic
to reference SEVERITY_THRESHOLD_LOW and SEVERITY_THRESHOLD_MEDIUM so the
thresholds are self-documenting and easier to change.
- Around line 90-94: The code redundantly calls
guildAutomationManifestSchema.parse on options.actualState even when callers may
pass an already-validated manifest; update the logic so parsing only happens for
raw input: add an explicit discriminator (e.g., options.actualStateIsValidated
boolean) or change the options type to accept a union (ValidatedManifest |
RawManifest) and only call guildAutomationManifestSchema.parse when the value is
the raw variant; keep the fallback to manifestRow.lastCapturedState via
toManifestDocument unchanged and reference the variables actual,
options.actualState, guildAutomationManifestSchema.parse, toManifestDocument,
and manifestRow.lastCapturedState when making the change.
In `@packages/shared/src/services/guildAutomation/GuildAutomationRepository.ts`:
- Around line 283-288: The listRuns method currently forwards an unbounded limit
to prisma.guildAutomationRun.findMany; clamp the incoming limit in
GuildAutomationRepository.listRuns to a safe max (e.g., const MAX_RUNS = 100)
and ensure it's at least 1 (use Math.max(1, Math.min(limit, MAX_RUNS)) or
equivalent) before passing to the take option on
prisma.guildAutomationRun.findMany so the DB query cannot be abused by large
limits.
In `@packages/shared/src/services/guildAutomation/service.ts`:
- Line 51: The dynamic import of createAutomationPlan inside service.ts (const {
createAutomationPlan } = await import('./diff.js')) adds runtime overhead;
replace it with a static top-level import (import { createAutomationPlan } from
'./diff.js') to eliminate per-call module resolution, unless you need the
dynamic import to avoid a circular dependency or for code-splitting—if circular
references exist, instead refactor to break the cycle or keep the dynamic import
with a brief comment explaining why.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d0a2221f-82f2-4f7a-a2a3-65688a021ab2
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json,!**/package-lock.json
📒 Files selected for processing (5)
packages/shared/src/services/guildAutomation/GuildAutomationOrchestrator.tspackages/shared/src/services/guildAutomation/GuildAutomationRepository.tspackages/shared/src/services/guildAutomation/IGuildAutomationRepository.tspackages/shared/src/services/guildAutomation/guildAutomationHelpers.tspackages/shared/src/services/guildAutomation/service.ts
…actions - recordCapture: manifest upsert + run create now atomic - runCutover: manifest update + run create now atomic; also persist version and checklist in the cutover run summary to prevent audit drift
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/shared/src/services/guildAutomation/GuildAutomationRepository.ts`:
- Around line 277-279: The repository is persisting the original checklist
variable instead of the finalized/normalized snapshot; when summary:
toJsonValue({ checklistComplete: true, checklist }) is written, replace the
persisted checklist with the normalized snapshot from nextManifest (use
nextManifest.parity.checklist) so that when completeChecklist is true the stored
checklist reflects the all-done normalization; update the summary construction
in GuildAutomationRepository (the toJsonValue call) to use
nextManifest.parity.checklist rather than checklist.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 6ffcb9a9-411e-442b-9ae6-f215a9201825
📒 Files selected for processing (1)
packages/shared/src/services/guildAutomation/GuildAutomationRepository.ts
There was a problem hiding this comment.
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here.
|
LucasSantana-Dev
added a commit
that referenced
this pull request
May 24, 2026
## Summary Replaces the 15+ positional arguments passed through the autoplay collector pipeline with a single `AutoplayContext` value object, improving call-site clarity and making the pipeline easier to extend. ### Changes - Introduce `AutoplayContext` interface in `autoplayContext.ts` - Update `candidateCollector`, `lastFmSeeder`, `spotifyRecommender`, `replenisher`, and `candidateFallback` to accept `AutoplayContext` instead of positional args - Fix missing `beforeEach` mock setup in `candidateCollector` tests - Remove leftover `.bak` file from refactor ### Why Positional argument lists of 15+ items are hard to read, easy to misorder, and brittle to extend. A named value object surfaces intent at every call site and isolates future additions to one interface definition. Part of the architecture refactor series (T1–T5): - T1 #979 ✅ — extract `MessagePipeline` - T2 #980 ✅ — introduce `IGuildAutomationRepository` - T3 #981 ✅ — `AutomationPlan` result type - T4 #982 ✅ — `GuildAutomationRepository` / `Orchestrator` split - T5 (this PR) — `AutoplayContext` value object
LucasSantana-Dev
added a commit
that referenced
this pull request
May 24, 2026
ADRs for the 5 refactors merged in PRs #979–#983 and the Phase 4 test reduction strategy. ## Decision records added - `2026-05-23-artist-suggestion-service.md` — ArtistSuggestionService extraction (#980) - `2026-05-23-autoplay-context-value-object.md` — AutoplayContext value object (#983) - `2026-05-23-bot-test-reduction-phase4-replacement-strategy.md` — Phase 4 test replacement strategy - `2026-05-23-guild-automation-orchestrator-repository-split.md` — GuildAutomationService split (#982) - `2026-05-23-message-pipeline-handler-chain.md` — MessagePipeline handler chain (#981) - `2026-05-23-recommendation-engine-single-entrypoint.md` — recommendTracks() single entrypoint (#979) <!-- This is an auto-generated comment: release notes by coderabbit.ai --> ## Summary by CodeRabbit * **Documentation** * Added architecture decision records documenting planned service refactoring, test optimization strategies, and API consolidation initiatives to improve system maintainability. <!-- review_stack_entry_start --> [](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/1040?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack) <!-- review_stack_entry_end --> <!-- end of auto-generated comment: release notes by coderabbit.ai -->
LucasSantana-Dev
added a commit
that referenced
this pull request
May 25, 2026
## Summary - Bumps all package.json files from `2.14.1` → `2.15.0` - Populates `CHANGELOG.md` with everything since v2.14.1 - Adds ADR `docs/decisions/2026-05-24-ci-runtime-baseline-accepted.md` (CI runtime baseline: 3–4 min accepted, Jest sharding deferred) ### Changes included in this release **Added** - ServerLogs + ServerSettings UI pages (#965) - AutoMessages executor wiring into execution service (#950) **Changed** - 4 UI redesigns: Admin, Config, Login, ServersPage, CustomCommands, GuildAutomation, Spotify, LastFm (#967–#970) - 5 refactors: AutoplayContext VO (#983), GuildAutomationOrchestrator/Repository split (#982), MessagePipeline chain (#981), ArtistSuggestionService (#980), recommendTracks entrypoint (#979) **Fixed** — deploy CI gap sweep (A–F) - Gap A: hard-fail on OAuth 429 (#1045) - Gap B: surface async deploy via commit statuses (#1046) - Gap C: bot healthcheck polls Discord gateway, not Redis TCP (#1047) - Gap D: post error status on lock contention (#1052) - Gap E: add bot to required containers, remove dead unhealthy grep (#1054) - Gap F: wait for homelab-deploy completion on docker_rebuilt=true path (#1056) - lockfile-hash BuildKit cache key (#1016) - squash-merged release branch archive (#946) **Internal** - Phase 4 test cleanup — 93 tests removed (#956–#1035) - Pre-commit hooks: husky + lint-staged + tsc (#1007) - madge gate promoted to blocking - Dependabot routine bumps (#971–#977, #1042) > **Note:** #1054 (Gap E) may still be in CI — merge this PR after #1054 lands.
](https://app.coderabbit.ai/change-stack/LucasSantana-Dev/Lucky/pull/1040?utm_source=github_walkthrough&utm_medium=github&utm_campaign=change_stack){kind=link}
This branch was successfully deployed
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Splits the monolithic
guildAutomation/service.ts(535 LOC) into four focused modules:IGuildAutomationRepository.ts— interface for all DB operationsGuildAutomationRepository.ts— Prisma implementation (~329 LOC)GuildAutomationOrchestrator.ts— business logic with injected repository (~301 LOC)service.ts— dependency wiring + public API re-exports (65 LOC)guildAutomationHelpers.ts— shared JSON helpers (toJsonValue,toManifestDocument,isObject)Why
The monolith mixed Prisma calls, business logic, and lock management in one class. Splitting behind an interface:
as Prisma.InputJsonValuecast (now runtime-validated via JSON round-trip)Impact
Zero breaking changes. Public API (
guildAutomationService+ all exported functions/types) is identical. No caller files modified.Summary by CodeRabbit