feat: implement config plugin

[lusu007]

Pull Request

Description

Type of Change

• 🐛 Bug fix
• ✨ New feature
• 💥 Breaking change
• ♻️ Refactoring
• 📚 Documentation
• 🔧 Chore

Related Issues

None

Testing

• Unit tests pass
• Manual testing completed
• New tests added for new functionality

Checklist

• I have performed a self-review of my own code
• Tests have been added/updated and pass (if needed)
• Documentation has been updated (if needed)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: b36e51c443

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[Copilot]

Pull request overview

Implements a cross-platform “config plugin” that provides runtime configuration management backed by gRPC (snapshots/default sync) and NATS (change notifications), with separate entrypoints for Velocity and Paper plus an example consumer plugin.

Changes:

• Added common runtime config library (definitions/bindings, gRPC client, NATS listener, environment configuration).
• Added Paper and Velocity plugin entrypoints that boot ConfigManager and register gRPC providers.
• Added an example Paper plugin demonstrating registration, reads, and live updates; plus DevSpace/Gradle scaffolding and Gradle wrapper files.

Reviewed changes

Copilot reviewed 26 out of 29 changed files in this pull request and generated 5 comments.

Show a summary per file

File	Description
velocity/src/main/kotlin/gg/grounds/config/ConfigVelocityPlugin.kt	Velocity plugin entrypoint that starts ConfigManager on proxy init/shutdown.
velocity/devspace.yaml	DevSpace config for iterating on the Velocity deployment in-cluster.
velocity/build.gradle.kts	Velocity module build config + gRPC deps.
settings.gradle.kts	Includes modules and configures plugin management repositories.
paper/src/main/resources/plugin.yml	Paper plugin descriptor for plugin-config.
paper/src/main/kotlin/gg/grounds/config/ConfigPaperPlugin.kt	Paper plugin entrypoint that starts ConfigManager and registers it as a Bukkit service.
paper/devspace.yaml	DevSpace config for iterating on the Paper deployment in-cluster.
paper/build.gradle.kts	Paper module build config + gRPC deps.
gradlew.bat	Gradle wrapper script for Windows.
gradlew	Gradle wrapper script for POSIX.
gradle/wrapper/gradle-wrapper.properties	Wrapper distribution configuration.
gradle/wrapper/gradle-wrapper.jar	Wrapper runtime jar.
example/src/main/resources/plugin.yml	Example plugin descriptor that depends on plugin-config.
example/src/main/kotlin/gg/grounds/config/example/ExamplePaperPlugin.kt	Example consumer plugin showing registration/get/onChange usage.
example/src/main/kotlin/gg/grounds/config/example/ExampleConfigDefinitions.kt	Sample typed config definitions + payload classes used by the example plugin.
example/devspace.yaml	DevSpace config for iterating on the example plugin in-cluster.
example/build.gradle.kts	Example module build config.
common/src/main/kotlin/gg/grounds/config/nats/NatsConfigListener.kt	Shared NATS listener handling subscriptions per (app, env) with reconnect backoff.
common/src/main/kotlin/gg/grounds/config/grpc/GrpcConfigClient.kt	Shared gRPC blocking client wrapper for config service calls.
common/src/main/kotlin/gg/grounds/config/grpc/BaseGrpcClient.kt	Shared gRPC channel lifecycle utilities.
common/src/main/kotlin/gg/grounds/config/EnvironmentConfig.kt	Reads required infra endpoints from environment variables.
common/src/main/kotlin/gg/grounds/config/ConfigSnapshot.kt	Snapshot/key model types for versioned config documents.
common/src/main/kotlin/gg/grounds/config/ConfigManager.kt	Core manager for registration, default syncing, snapshot loading, and live updates.
common/src/main/kotlin/gg/grounds/config/ConfigDefinitionNotRegisteredException.kt	Fail-fast exception for unregistered definition usage.
common/src/main/kotlin/gg/grounds/config/ConfigDefinition.kt	Base type for typed config document definitions.
common/src/main/kotlin/gg/grounds/config/ConfigBinding.kt	Internal binding holder managing current value + callbacks.
common/build.gradle.kts	Common module build config + deps (NATS, Jackson, gRPC contracts).
build.gradle.kts	Root build plugin configuration.
.gitignore	Repo ignore rules for build tooling and local artifacts.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[hbrombeer]

Code Review: plugin-config

Fehler & Probleme

1. Race Condition beim Bootstrap

ConfigScopeSynchronizer.bootstrap() macht drei Schritte sequentiell: syncDefaults → loadSnapshot → subscribeNats. Zwischen loadSnapshot und subscribeNats können NATS-Events verloren gehen. Wenn in dieser Lücke ein Config-Update passiert, wird es erst beim nächsten 15s-Poll erkannt.

Fix: Zuerst subscriben, dann Snapshot laden. Doppelte Events sind harmlos (Version-Check filtert).

2. Executor-Leak in ConfigScopeSynchronizer

Der ScheduledExecutorService wird mit Executors.newSingleThreadScheduledExecutor() erstellt und mit scheduleAtFixedRate gestartet, aber nur bei close() des ConfigManagers gestoppt. Wenn close() nicht aufgerufen wird (z.B. Plugin-Crash), laufen die Threads weiter.

3. Keine Retry-Logik bei gRPC-Fehlern

GrpcConfigClient hat Timeouts (5s/15s), aber keine Retry-Logik. Wenn der Service kurzzeitig nicht erreichbar ist (z.B. Rolling Update), schlägt der Bootstrap fehl und Configs bleiben uninitialisiert. Der 15s-Poll fängt das teilweise auf, aber der initiale Bootstrap nicht.

4. ConfigManagerProvider ist ein globaler Singleton

Wenn zwei Plugins denselben ConfigManager-Provider nutzen und einer unregister() aufruft, verliert der andere den Zugriff. Das ist fragil bei Plugin-Reload-Szenarien.

5. Keine Tests

Keine Tests vorhanden. Empfehlung: Unit-Tests für ConfigBinding, ConfigScopeRegistry, und Integration-Tests für den Sync-Flow.

Sicherheit

Thema	Severity
gRPC Plaintext (kein TLS)	Niedrig — akzeptabel im Cluster
Keine Auth am Client	Niedrig — Service sollte Auth erzwingen

Ideen

• Retry mit Backoff beim Bootstrap und bei gRPC-Fehlern — gerade bei Rolling Updates kritisch
• Health-Check Endpoint — z.B. "sind alle registrierten Configs initialisiert?" für Readiness-Probes
• Metrics — Config-Fetch-Latenz, NATS-Reconnects, Cache-Hit-Rate (Version unchanged)
• Graceful Degradation bei NATS-Ausfall — aktuell 15s-Poll als Fallback, aber kein Warning an den Operator wenn NATS dauerhaft weg ist

[hbrombeer]

Code Review

Verdict: Request changes — solide Struktur und überraschend ernstzunehmende Testabdeckung, aber drei echte Korrektheits-Probleme und ein Credential-Sharp-Edge im Build.

🚨 Blockers

• common/build.gradle.kts:8-9 & settings.gradle.kts — providers.gradleProperty("github.user").get() wird zur Configuration-Time unconditional aufgelöst. Jeder Build ohne gesetzte Properties (inkl. ./gradlew help) bricht hart ab, und lokale Dev-Setups riskieren, Credentials versehentlich ins Repo-gradle.properties zu committen statt ins ~/.gradle/gradle.properties. Auf .orElse("") umstellen, gradle.properties.example ergänzen und Env-Fallback (GITHUB_ACTOR/GITHUB_TOKEN) für CI vorsehen.
• common/.../ConfigScopeSynchronizer.kt → applySnapshot — setzt scope.setVersion(response.version) unbedingt, auch wenn response.version < scope.version(). Out-of-order Responses (Retry-Race, Replica-Lag auf dem Server) können damit eine neuere Version mit einer älteren überschreiben. Der withRefreshLock schützt davor nicht, weil der gRPC-Call innerhalb des Locks läuft, aber keine Aussage über Server-seitige Ordering macht. Fix: expliziter if (version <= previous) return Guard mit Log, oder currentVersion.updateAndGet { max(it, version) }.
• ConfigScopeSynchronizer.kt → applySnapshot / buildCachedDocuments — applyDocument fängt Deserialisierungsfehler pro Dokument und loggt nur (gut: ein Bad-Doc killt den Snapshot nicht). Aber unmittelbar danach wird snapshotCache.save(...) mit dem rohen documentsByKey aufgerufen — ein Dokument, dessen Binding die Deserialisierung abgelehnt hat, landet trotzdem als "gültig" im Disk-Cache und wird beim nächsten DEGRADED-Start erneut angewendet. Schlimmer: buildCachedDocuments re-serialisiert für initialisierte Bindings den alten Wert, wodurch ein alter erfolgreicher Wert ein neues defektes Dokument maskieren kann. Entweder nur erfolgreich angewandte Dokumente cachen, oder den Cache als exakten Server-Spiegel halten (ohne Binding-Re-Serialisierung).

⚠️ Should address

• ConfigScopeSynchronizer → refreshTrackedScopes — sequentielle Iteration über alle Scopes auf Single-Thread-Executor. Ein einzelner blockierender gRPC-Call (5 s deadline × 2 Retries) verzögert alle anderen Scopes um ~6 s. Bei mehreren Apps/Envs auf derselben Instanz skaliert das nicht. Pro Scope dispatchen oder asynchronen Stub verwenden.
• ConfigScopeSynchronizer Lebenszyklus — bootstrap ist nicht @Synchronized, aber start/close sind es. Race zwischen register (während eines laufenden close/start-Zyklus) und Teardown möglich. Mindestens per Javadoc klarstellen, besser expliziter Lebenszyklus-Lock.
• NatsConfigListener → connect() — Nats.connect(...) wird ohne ConnectionListener/ErrorListener konfiguriert. Der manuelle scheduleReconnect-Pfad wird nur vom initialen Connect-Exception getriggert. Falls eine Verbindung nach erfolgreichem Connect ausfällt und von jnats dauerhaft geschlossen wird, passiert hier nichts. Ein .connectionListener { _, event -> … } wäre sauberer. (Uncertain — jnats reconnecte u. U. intern; in dem Fall nur Log fragwürdig, nicht funktional defekt.)
• GrpcConfigClient → getDocument — existiert, wird aber nirgends aufgerufen und behandelt NOT_FOUND nicht explizit. Proto-Contract aus library-grpc-contracts#36 verlangt aber gerade diese Semantik. Entweder entfernen (YAGNI) oder mit StatusRuntimeException-Catch auf Code.NOT_FOUND → null umrüsten.
• BaseGrpcClient → usePlaintext — hardcoded. Für einen Config-Service, der potenziell Secrets transportiert, mindestens als TODO/Flag markieren.
• ConfigManager → scopeSynchronizer — wird bei Construction direkt erzeugt, inklusive gRPC/NATS-Factories. Der Test ConfigManagerTest braucht deshalb Reflection (field.isAccessible = true), um den Synchronizer zu stubben — klares Zeichen, dass die API hier zu eng ist. Synchronizer via Constructor injizieren (mit Default-Factory), dann verschwinden die Hacks.
• ConfigManagerProvider → get() = lastOrNull() — Stack-of-Managers-Semantik: bei zwei Plugins, die registrieren, gewinnt der zuletzt geladene. Macht die Plugin-Load-Reihenfolge load-bearing. Entweder auf "genau einen" festnageln (throw beim zweiten register) oder Multi-Tenancy mit bewusstem Disambiguator einführen.
• ConfigBinding — getAndSet(newValue) gefolgt von initialized.set(true) ist kein atomares Paar. Ein Reader zwischen den beiden Zeilen sieht einen neuen Wert, während initialized() noch false zurückgibt. Reihenfolge umkehren oder explizit dokumentieren, dass die Invariante nur eventual gilt.

🔗 Cross-PR-Befund (Koordination mit service-config#1)

• NATS-Subject config.{app}.{env}.changed wird hier konsumiert und auf der Server-Seite in ConfigChangePublisher publiziert — ohne geteilte Konstante. Wenn eine Seite umbenennt, merkt das niemand. Entweder in library-grpc-contracts als Proto-Kommentar/Konstante festschreiben, oder in eine kleine Shared-Library ziehen.
• Der Listener liest den Payload als Log-only und triggert einen vollen getSnapshotIfNewer — gut für Robustheit gegen Schema-Drift. Bedeutet aber auch: der Reconcile-Polling-Pfad ist der tatsächliche Correctness-Anker, nicht die NATS-Events. Das sollte im Doc-Comment und in einem kurzen ADR festgehalten werden, weil der Server-PR das Event-Publishing explizit als Best-Effort verlässt (Publish liegt außerhalb der DB-Transaktion).

💡 Suggestions

• ConfigScopeSynchronizer mit ~480 Zeilen und sieben Verantwortlichkeiten (Bootstrap, Default-Sync, Subscribe, Poll, Retry, Cache, Registration-Result-Mapping) in BootstrapCoordinator + RefreshScheduler + SnapshotApplier zerteilen.
• applySnapshot loggt auf INFO bei jedem Poll, auch bei changed=false — auf DEBUG reduzieren, wenn documents.size == 0 oder version == previous.
• Jackson 3.1.2 ist Milestone-Stand (Stand Mai 2025 noch nicht final). Verifizieren, dass stabil, sonst 2.17.x.
• PR-Beschreibung ist leeres Template — vor Merge ausfüllen, idealerweise mit Verweisen auf library-grpc-contracts#36 und den service-config PR.
• Keine CI konfiguriert: mindestens ./gradlew check als GitHub Action, sonst verrotten die 590 Zeilen Tests still.

✅ Was gut ist

• Sauberes Layering: ConfigManager → ConfigScopeRegistry + ConfigScopeSynchronizer → ConfigSyncClient/ConfigChangeListener-Interfaces. Die Trennung macht den 590-Zeilen-Test überhaupt möglich.
• Differenzierte Retry-Budgets: BOOTSTRAP_GRPC_MAX_ATTEMPTS=3 (robust) vs. REFRESH_GRPC_MAX_ATTEMPTS=2 (darf nicht hängen). Guter Kompromiss.
• FAIL_CLOSED vs DEGRADED als explizite Modi mit strukturiertem ConfigRegistrationResult ist eine reife API-Entscheidung — Aufrufer müssen sich bewusst mit dem Not-Ready-Fall auseinandersetzen.
• Ein defektes Dokument killt nicht den ganzen Snapshot (applyDocument fängt Exceptions pro Doc) — wichtig für Betrieb.
• Test "bootstrap subscribes before initial snapshot and applies concurrent update" testet explizit den Race, bei dem ein NATS-Event während des ersten getSnapshot ankommt. Genau der richtige Pfad. Einziger Kritikpunkt: der Test prüft nur die Call-Reihenfolge, nicht Versions-Regression (siehe Blocker 2).

---

🤖 Review produced with Claude Code (opus-4-6, 1M-context)