feat: Tenant-Scoped Global Variable Store + Prefix Rename (vars/vault)#470
Merged
Conversation
…onfig
Implements a non-encrypted, config-driven Global Variable Store that
allows runtime parameter updates across all agents without redeployment.
New components:
- GlobalVariable model (key/value/description/exportable)
- IGlobalVariableStore + MongoDB adapter (non-versioned flat storage)
- GlobalVariableResolver (regex-based resolution with
Caffeine cache and invalidation listener pattern)
- REST API (CRUD with key validation and cache invalidation)
Pipeline integration (8 callsites, resolution before vault secrets):
- LlmTask: template injection ({{vars.key}}) + task.type late-binding
- ChatModelRegistry: resolve params + register for invalidation
- ApiCallExecutor: URL, body, headers, query params
- McpToolProviderManager: API key/URL resolution
- A2AToolProviderManager: API key/URL resolution
- EmbeddingModelFactory: config params before model creation
- EmbeddingStoreFactory: config params before store creation
- SlackChannelRouter: channel config values
Tests: 32 new unit tests + 9 existing test suites updated for new
constructor signatures. All tests green.
…n tests Bug fix: - Hoist resolvedType above token estimator and streaming branches in LlmTask (lines 288, 411). Previously, in task.type only worked for sync chat - token-aware windowing and streaming mode used the raw unresolved string, causing UnsupportedLlmTaskException. Documentation: - New docs/global-variables.md (comprehensive public docs with architecture, syntax, REST API, use cases, comparison table) - Updated AGENTS.md template data tables with vars and snippets namespaces (sections 4.2 and 5.1) - Added changelog entry for the full eddivar feature - Added global-variables.md to docs/SUMMARY.md index Integration tests: - GlobalVariableCrudIT (8 tests) - full CRUD lifecycle against MongoDB via Quarkus DevServices Note: PostgreSQL adapter for IGlobalVariableStore is not yet implemented. PG integration test deferred until adapter exists.
New: - PostgresGlobalVariableStore — JDBC adapter for global_variables table. Follows the established pattern (Instance<DataSource>, @DefaultBean, auto-schema creation via ensureSchema()). - GlobalVariableStoreTest — 10 unit tests for MongoDB adapter - PostgresGlobalVariableStoreTest — 15 unit tests for PG adapter including all error paths (SQLException handling) - PostgresGlobalVariableCrudIT — integration test running all 8 CRUD lifecycle tests against PostgreSQL (Testcontainers) Coverage: - 5 production classes: 99% instruction / 93% branch - 57 unit tests + 16 integration tests (MongoDB + PostgreSQL) - Remaining 3 missed branches are compiler-generated try-with- resources null checks (JaCoCo limitation)
Late-binding reference syntax unified to short, clean prefixes: - → (clean rename, never shipped) - → (backward-compat alias retained) Vault backward compatibility: - SecretReference.VAULT_PATTERN uses (?:vault|eddivault) alternation - isVaultReference() checks both prefixes - toReferenceString() outputs the new canonical form - Agent import auto-migrates → - 4 backward-compat tests verify old prefix still works Changed files: 21 source, 18 tests, 13 docs + AGENTS.md
- Add null guard for tenantId in GlobalVariableResolver to default to 'default' - Unify MongoDB adapter to use composite _id filter for get/delete - Add ascending sort by key to MongoDB getAll/listAll for Postgres parity - Quote SQL reserved words 'key' and 'value' across all Postgres adapter DDL and DML - Update docs and instructions for Manager UI and Website
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Scanned FilesNone |
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, 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 the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughAdds a tenant-scoped Global Variables system (resolver, store implementations, REST API, caching/invalidation) and integrates it across LLM, API call, embedding, MCP/A2A, and Slack flows; simultaneously normalizes vault reference syntax from ChangesGlobal Variables + Vault Migration
Sequence DiagramsequenceDiagram
participant Client
participant LlmTask
participant GlobalVariableResolver
participant SecretResolver
participant LanguageModelBuilder
Client->>LlmTask: execute task with params + templates
LlmTask->>GlobalVariableResolver: resolveValue(task.type)
GlobalVariableResolver-->>LlmTask: resolvedType
LlmTask->>GlobalVariableResolver: getTemplateData()
GlobalVariableResolver-->>LlmTask: Map(vars.key -> value)
LlmTask->>LlmTask: render templates ({{vars.*}})
LlmTask->>GlobalVariableResolver: resolveAll(parameters)
GlobalVariableResolver-->>LlmTask: parameters with vars resolved
LlmTask->>SecretResolver: resolveSecrets(parameters)
SecretResolver-->>LlmTask: parameters with ${vault:...} resolved
LlmTask->>LanguageModelBuilder: build model with resolved params
LanguageModelBuilder-->>LlmTask: model instance
LlmTask-->>Client: return result
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
🚥 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)
|
There was a problem hiding this comment.
Pull request overview
Adds a tenant-scoped Global Variable Store (MongoDB + PostgreSQL) and integrates ${vars:...} late-binding + {{vars.*}} template data across the LLM pipeline, while also renaming the vault late-binding prefix from ${eddivault:...} to ${vault:...} with backward compatibility and import-time auto-migration.
Changes:
- Introduces
GlobalVariablemodel, resolver (Caffeine cache + invalidation listeners), REST CRUD API, and Mongo/Postgres store implementations + tests/ITs. - Integrates global variable resolution into key runtime callsites (LLM model registry/factories, MCP/A2A, HTTP calls, Slack) and injects
varsinto template data. - Renames vault reference prefix to
${vault:...}across code/docs/tests, keeping legacy${eddivault:...}support and normalizing on import.
Reviewed changes
Copilot reviewed 86 out of 89 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/ai/labs/eddi/secrets/sanitize/SecretScrubberTest.java | Updates tests to ${vault:...} references |
| src/test/java/ai/labs/eddi/secrets/sanitize/SecretRedactionFilterTest.java | Updates redaction tests for new prefix |
| src/test/java/ai/labs/eddi/secrets/rest/RestSecretStoreTest.java | Expects ${vault:...} refs from REST |
| src/test/java/ai/labs/eddi/secrets/model/SecretReferenceTest.java | Updates parsing/format tests + legacy compat coverage |
| src/test/java/ai/labs/eddi/secrets/impl/SecretVaultIntegrationTest.java | Updates integration expectations to ${vault:...} |
| src/test/java/ai/labs/eddi/secrets/SecretResolverTest.java | Updates secret resolution tests to new prefix |
| src/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationTest.java | Updates API key examples to ${vault:...} |
| src/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationModelsTest.java | Updates MCP/A2A config tests to ${vault:...} |
| src/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerTest.java | Adapts constructor signature + vault prefix updates |
| src/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.java | Injects GlobalVariableResolver into test wiring |
| src/test/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactoryTest.java | Adds GlobalVariableResolver + vault wording in errors |
| src/test/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactoryTest.java | Adds GlobalVariableResolver to factory tests |
| src/test/java/ai/labs/eddi/modules/llm/impl/ChatModelRegistryTest.java | Adds GlobalVariableResolver + updates secret invalidation tests |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.java | Adapts constructor signature for global vars |
| src/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.java | Adds global var resolver pass-through + vault prefix |
| src/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.java | Adds GlobalVariableResolver dependency + vault prefix updates |
| src/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.java | Adds GlobalVariableResolver + vault prefix updates |
| src/test/java/ai/labs/eddi/integration/postgres/PostgresGlobalVariableCrudIT.java | Runs GlobalVariable CRUD ITs against Postgres |
| src/test/java/ai/labs/eddi/integration/GlobalVariableCrudIT.java | Adds REST IT coverage for global variables |
| src/test/java/ai/labs/eddi/engine/mcp/McpSetupToolsTest.java | Updates setup test to assert ${vault:...} |
| src/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.java | Unit tests for Postgres global variable JDBC adapter |
| src/test/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStoreTest.java | Unit tests for REST CRUD + validation + invalidation |
| src/test/java/ai/labs/eddi/configs/variables/mongo/GlobalVariableStoreTest.java | Unit tests for Mongo global variable adapter |
| src/test/java/ai/labs/eddi/configs/variables/model/GlobalVariableTest.java | Unit tests for record defaults + JSON behavior |
| src/test/java/ai/labs/eddi/configs/variables/GlobalVariableResolverTest.java | Unit tests for ${vars:...} resolution + caching + listeners |
| src/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsModelsTest.java | Updates MCP calls model tests to ${vault:...} |
| src/main/resources/META-INF/resources/manage.html | Updates Manager UI asset hashes |
| src/main/resources/META-INF/resources/assets/yaml-BjPKFs2h.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/xml-e4Ud4yx9.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/typescript-sC3c-Jld.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/python-C1D6UJRn.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/mdx-BNrhjuw1.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/liquid-CMO_zke7.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/jsonMode-D9dJjUHG.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/javascript-inUojZyW.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/htmlMode-DdcsDH59.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/html-BWbqU3gc.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/handlebars-DNBNwtfL.js | Rebuilt UI asset referencing new entry chunk |
| src/main/resources/META-INF/resources/assets/cssMode-BHkIakpN.js | Rebuilt UI asset referencing new entry chunk |
| src/main/java/ai/labs/eddi/secrets/sanitize/SecretScrubber.java | Updates scrubber to preserve both new + legacy vault refs |
| src/main/java/ai/labs/eddi/secrets/sanitize/SecretRedactionFilter.java | Redacts both ${vault:...} and ${eddivault:...} forms |
| src/main/java/ai/labs/eddi/secrets/model/SecretReference.java | Canonicalizes output to ${vault:...}; accepts legacy prefix |
| src/main/java/ai/labs/eddi/secrets/VaultStartupBanner.java | Updates startup banner messaging to ${vault:...} |
| src/main/java/ai/labs/eddi/secrets/SecretResolver.java | Updates docs/comments to new ${vault:...} prefix |
| src/main/java/ai/labs/eddi/secrets/ISecretProvider.java | Updates docs/comments to ${vault:...} |
| src/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.java | Updates auto-vault docs/examples to ${vault:...} |
| src/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.java | Updates API key docs/examples to ${vault:...} |
| src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java | Resolves ${vars:...} then ${vault:...} for MCP API keys |
| src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java | Injects vars into template data; resolves ${vars:...} in task type |
| src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.java | Resolves ${vars:...} then vault secrets in store parameters |
| src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactory.java | Resolves ${vars:...} then vault secrets in embedding parameters |
| src/main/java/ai/labs/eddi/modules/llm/impl/ChatModelRegistry.java | Resolves ${vars:...} then vault; clears caches on var invalidation |
| src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java | Resolves ${vars:...} then vault secrets for A2A API keys |
| src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java | Resolves ${vars:...} then vault secrets in URL/body/headers/query |
| src/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.java | Resolves ${vars:...} then vault secrets for Slack credentials |
| src/main/java/ai/labs/eddi/engine/setup/AgentSetupService.java | Updates docs/comments for ${vault:...} auto-vaulting |
| src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java | Updates tool arg docs to ${vault:...} |
| src/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.java | Adds Postgres-backed global variable store + schema creation |
| src/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.java | Adds REST CRUD with validation + cache invalidation |
| src/main/java/ai/labs/eddi/configs/variables/rest/IRestGlobalVariableStore.java | Defines JAX-RS API for global variable CRUD |
| src/main/java/ai/labs/eddi/configs/variables/mongo/GlobalVariableStore.java | Adds Mongo-backed global variable store |
| src/main/java/ai/labs/eddi/configs/variables/model/GlobalVariable.java | Adds GlobalVariable record with defaults |
| src/main/java/ai/labs/eddi/configs/variables/IGlobalVariableStore.java | Adds persistence interface for global variables |
| src/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.java | Adds ${vars:...} resolver with per-tenant cache + listeners |
| src/main/java/ai/labs/eddi/configs/rag/model/RagConfiguration.java | Updates docs/examples to ${vault:...} |
| src/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsConfiguration.java | Updates API key docs/examples to ${vault:...} |
| src/main/java/ai/labs/eddi/backup/impl/RestImportService.java | Normalizes legacy ${eddivault:...} to ${vault:...} on import |
| src/main/java/ai/labs/eddi/backup/impl/AbstractBackupService.java | Adds helper to normalize legacy vault references |
| docs/slack-integration.md | Updates Slack docs to ${vault:...} |
| docs/secrets-vault.md | Updates vault docs to ${vault:...} |
| docs/release-notes-6.0.2.md | Updates release notes to ${vault:...} |
| docs/rag.md | Updates RAG docs to ${vault:...} |
| docs/model-cascade.md | Updates model cascade docs to ${vault:...} |
| docs/mcp-server.md | Updates MCP docs to ${vault:...} |
| docs/how-to....md | Updates how-to examples to ${vault:...} |
| docs/global-variables.md | Adds full documentation for global variables feature |
| docs/developer-quickstart.md | Updates quickstart vault prefix to ${vault:...} |
| docs/audit-ledger.md | Updates audit docs to ${vault:...} |
| docs/a2a-protocol.md | Updates A2A docs to ${vault:...} |
| docs/SUMMARY.md | Adds global variables doc to summary |
| AGENTS.md | Documents template data additions (snippets, vars) + vault prefix |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
src/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.java (1)
382-403:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDocument the degraded-mode fallback.
The new Javadoc reads like auto-vaulting always succeeds, but
autoVaultSecret()still returns the plaintext whensecretProvider.store(...)fails. That weakens the security guarantee and should be stated explicitly.💡 Suggested wording
- * Store a plaintext secret in the vault and return the vault reference string. - * Also scrubs the raw user input from conversation memory to prevent leakage. + * Best-effort auto-vaulting: stores the plaintext secret in the vault and returns + * a vault reference when storage succeeds. If vault storage fails, the plaintext + * is returned unchanged so callers can decide how to handle the failure.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.java` around lines 382 - 403, Update the Javadoc for PropertySetterTask.autoVaultSecret(...) to document the degraded-mode fallback: explicitly state that if secretProvider.store(...) fails the method will return the original plaintext (i.e., vaulting did not occur), that conversation memory scrubbing still runs, and callers should treat the returned string as potentially non-vaulted; reference the method name autoVaultSecret(), secretProvider.store(...), and the parameters memory, keyName, plaintext so readers can find the behavior in the code.src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java (1)
109-115:⚠️ Potential issue | 🟠 Major | ⚡ Quick winRedact query/body secrets before persisting the request map.
These lines now resolve vault/global-variable values before
createMemoryEntry(), but the only pre-persistence scrubber still targets headers. Any secret-bearing query param or body field will now land in conversation memory in plaintext.🛡️ Suggested direction
- scrubSensitiveHeaders(requestMap); + scrubSensitiveRequestData(requestMap);Also applies to: 345-352
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java` around lines 109 - 115, The requestMap is being sanitized only for headers via scrubSensitiveHeaders before calling prePostUtils.createMemoryEntry(currentStep, requestMap, objectName, KEY_HTTP_CALLS), leaving query params and body fields with resolved secret values in conversation memory; update the code so that requestMap's query parameters and body payload are scrubbed as well (either extend scrubSensitiveHeaders to also scrub requestMap.query and requestMap.body, or add a new scrubSensitiveBodyAndQuery(requestMap) helper) and invoke it before createMemoryEntry; apply the same change to the other symmetric location where createMemoryEntry is called for API requests so no query/body secrets are persisted.src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java (1)
151-167:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftInvalidate cached MCP clients when the resolved API key changes.
getOrCreateClient()caches by URL only, butcreateTransport()now bakes the resolvedAuthorizationheader into that cached client. If a${vars:...}or${vault:...}value rotates, the manager will keep using stale credentials until shutdown, which breaks the new late-binding behavior. Consider keying the cache by resolved auth material or evicting on resolver invalidation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java` around lines 151 - 167, getOrCreateClient currently keys cached MCP clients only by config.getUrl(), but createTransport bakes the resolved Authorization header from createTransport() into the client; update cache behavior in McpToolProviderManager so cached clients are invalidated or keyed by the resolved auth material. Specifically, in getOrCreateClient (and any client cache map) include the resolved API key or resolved auth header (the value produced in createTransport as resolvedKey) in the cache key, or detect when globalVariableResolver/secretResolver returns a different value and evict the existing client before creating a new one; ensure you still resolve apiKey using globalVariableResolver.resolveValue and secretResolver.resolveValue (as in createTransport) when computing the cache key or eviction check so rotated credentials produce a new transport/client.src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactory.java (1)
59-67:⚠️ Potential issue | 🟠 MajorCache key is computed before variable/secret resolution, allowing stale models to persist on config changes.
Lines 59–60 build the cache key from unresolved embedding parameters (containing
${vars:...}and${vault:...}tokens). Resolution occurs later inbuild()at lines 66–67. UnlikeChatModelRegistry, this factory has no@PostConstructinvalidation listener to react to variable or secret updates; stale entries are evicted only by explicitclearCache()calls or after 30 minutes of idle time. In dynamic environments where API keys or endpoints are rotated, old embedding models remain cached and in use until manual intervention.Proposed fix
public EmbeddingModel getOrCreate(RagConfiguration config) { - String paramKey = config.getEmbeddingParameters() != null ? new TreeMap<>(config.getEmbeddingParameters()).toString() : ""; - String cacheKey = config.getEmbeddingProvider() + ":" + paramKey; - return cache.get(cacheKey, k -> build(config)); + Map<String, String> rawParams = config.getEmbeddingParameters() != null ? config.getEmbeddingParameters() : Map.of(); + Map<String, String> resolvedParams = secretResolver.resolveSecrets(globalVariableResolver.resolveAll(rawParams)); + String paramKey = new TreeMap<>(resolvedParams).toString(); + String cacheKey = config.getEmbeddingProvider() + ":" + paramKey; + return cache.get(cacheKey, k -> build(config, resolvedParams)); } -private EmbeddingModel build(RagConfiguration config) { - Map<String, String> rawParams = config.getEmbeddingParameters() != null ? config.getEmbeddingParameters() : Map.of(); - Map<String, String> params = globalVariableResolver.resolveAll(rawParams); - params = secretResolver.resolveSecrets(params); +private EmbeddingModel build(RagConfiguration config, Map<String, String> params) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactory.java` around lines 59 - 67, The cache key is built from unresolved embedding parameters (paramKey/cacheKey) so updates to variables/secrets won't change the key; fix by resolving variables and secrets before computing the cache key: call globalVariableResolver.resolveAll(...) and then secretResolver.resolveSecrets(...) on config.getEmbeddingParameters(), normalize the resulting map (e.g., new TreeMap<>(resolvedParams)) and use that for paramKey/cacheKey when calling cache.get(...), and optionally mirror ChatModelRegistry's `@PostConstruct` invalidation listener to evict entries when resolvers update.src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.java (1)
65-70:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftCache on resolved parameters, not raw placeholders.
resolveParams()now expands tenant vars and vault references, but the cache key is still derived fromconfig.getStoreParameters(). A later variable or secret change can therefore keep returning an embedding store built with stale connection details until the 30-minute TTL expires orclearCache()is called.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.java` around lines 65 - 70, The cache key currently uses raw config.getStoreParameters(), so changes to resolved tenant/vault values aren't reflected; modify getOrCreate to call resolveParams(config) (or otherwise obtain the resolved/storeParameters map) and build the paramsKey from that resolved map instead of config.getStoreParameters(), then use that same resolved parameters when invoking build(config, kbId) (or adapt build to accept the resolved map) so the cached entry matches the actual connection details; refer to getOrCreate, resolveParams, build, cache.get and clearCache when making the change.src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java (1)
446-447:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUse
resolvedTypeconsistently in persisted identifiers.
typeis now late-bound, but the trace, audit, and output keys still store the raw template expression. That makes the recorded metadata disagree with the model actually executed whenevertask.typecomes from${vars:...}.Suggested fix
- var langchainData = dataFactory.createData(KEY_LANGCHAIN + ":" + task.getType() + ":" + task.getId(), responseContent); + var langchainData = dataFactory.createData(KEY_LANGCHAIN + ":" + resolvedType + ":" + task.getId(), responseContent); ... - String modelName = processedParams.getOrDefault("model", task.getType()); + String modelName = processedParams.getOrDefault("model", resolvedType); ... - var traceData = dataFactory.createData(KEY_LANGCHAIN + ":trace:" + task.getType() + ":" + task.getId(), toolTrace); + var traceData = dataFactory.createData(KEY_LANGCHAIN + ":trace:" + resolvedType + ":" + task.getId(), toolTrace); ... - var outputData = dataFactory.createData(LANGCHAIN_OUTPUT_IDENTIFIER + ":" + task.getType(), responseContent); + var outputData = dataFactory.createData(LANGCHAIN_OUTPUT_IDENTIFIER + ":" + resolvedType, responseContent);Also applies to: 474-476, 481-482, 493-494
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java` around lines 446 - 447, The persisted identifiers use task.getType() (the late-bound template) instead of the resolved runtime type; update all key constructions (e.g., where KEY_LANGCHAIN + ":" + task.getType() + ":" + task.getId()) to use the resolvedType variable instead so trace, audit and output keys match the actual executed model. Locate usages in LlmTask where keys are built for langchain, trace/audit and output (e.g., currentStep.storeData, any trace/audit store calls, and response content keys) and replace task.getType() with resolvedType for those concatenations; ensure resolvedType is computed before these calls and used consistently across the other occurrences mentioned.
🧹 Nitpick comments (2)
src/test/java/ai/labs/eddi/integration/GlobalVariableCrudIT.java (1)
134-145: ⚡ Quick winDon’t treat HTTP 500 as an acceptable validation outcome.
This assertion currently permits server errors for bad client input, which can hide regressions in the REST contract.
Suggested fix
- given().contentType(ContentType.JSON) - .body(body) - .put(BASE_PATH + "/" + TENANT + "/bad key!") - .then().assertThat() - .statusCode(anyOf(equalTo(400), equalTo(500))); + given().contentType(ContentType.JSON) + .body(body) + .put(BASE_PATH + "/" + TENANT + "/bad key!") + .then().assertThat() + .statusCode(400);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/test/java/ai/labs/eddi/integration/GlobalVariableCrudIT.java` around lines 134 - 145, The test method invalidKey() currently allows a 500 response for invalid client input; update the assertion in that test to require a 400 only (remove the anyOf(equalTo(400), equalTo(500)) and replace with statusCode(equalTo(400))) so PUT to BASE_PATH + "/" + TENANT + "/bad key!" fails the request as a bad request; keep the same request body and contentType but tighten the assertion to explicitly expect HTTP 400 to prevent masking server errors.src/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.java (1)
87-92: Consider usingBadRequestExceptiondirectly for consistency with the codebase pattern.
validateIdthrowsIllegalArgumentException, which is already mapped to HTTP 400 by the framework'sIllegalArgumentExceptionMapper. However, other validation-heavy components in the codebase (RestExportService, RestGdprAdmin, DocumentDescriptorFilter) useBadRequestExceptiondirectly for immediate, explicit 400 responses. UsingBadRequestExceptionhere would improve clarity and align with the established pattern.Suggested fix
+import jakarta.ws.rs.BadRequestException; @@ private static void validateId(String value, String fieldName) { if (value == null || !ID_PATTERN.matcher(value).matches()) { - throw new IllegalArgumentException( + throw new BadRequestException( fieldName + " must match [a-zA-Z0-9_.\\-]+ (letters, digits, dots, underscores, hyphens). Got: " + sanitize(value)); } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.java` around lines 87 - 92, The validateId method currently throws IllegalArgumentException; change it to throw javax.ws.rs.BadRequestException with the same sanitized message to align with other REST handlers. Update the throw in validateId(String value, String fieldName) to new BadRequestException(fieldName + " must match [a-zA-Z0-9_.\\-]+ (letters, digits, dots, underscores, hyphens). Got: " + sanitize(value)), ensure javax.ws.rs.BadRequestException is imported, and leave ID_PATTERN and sanitize usages unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/changelog.md`:
- Around line 98-109: The "Tests (48 total)" heading in docs/changelog.md is
inconsistent with the listed table rows; recalculate the correct total by
summing the tests in the rows (7 + 14 + 11 + 8 + 8 + 10 + 15 = 73) and update
the heading "Tests (48 total)" to "Tests (73 total)"; ensure the table rows
(`GlobalVariableTest`, `GlobalVariableResolverTest`,
`RestGlobalVariableStoreTest`, `GlobalVariableCrudIT`,
`PostgresGlobalVariableCrudIT`, `GlobalVariableStoreTest`,
`PostgresGlobalVariableStoreTest`, and the "9 modified test suites" note) remain
unchanged except for adjusting the total count.
- Around line 47-67: The changelog entry still references the old
eddivar/eddivault names and single-key storage semantics; update the text to
reflect the final tenant-scoped vars/vault model and composite-key design by
replacing mentions of "eddivar" and "eddivault" with the final `vars`/`vault`
terminology, change descriptions of
`GlobalVariable`/`IGlobalVariableStore`/`GlobalVariableStore`/`PostgresGlobalVariableStore`
to note tenant-scoped and composite key behavior (e.g., tenant + key as PK), and
adjust `GlobalVariableResolver` and REST entries
(`IRestGlobalVariableStore`/`RestGlobalVariableStore`) to document the
`${vars:<tenant>:<key>}` resolution pattern and cache invalidation semantics;
ensure any lines describing "single-key storage" are rewritten to indicate
composite key/tenant scope and update the pipeline integration section to show
resolution order using `${vars:...}` with tenant context.
- Line 3420: The security note contains a typo where the legacy secret prefix is
duplicated as `${vault:...}` for both forms; update the legacy form to use the
correct prefix `${eddivault:...}` (i.e., replace the left/old occurrence of
`${vault:...}` in the A2A security note with `${eddivault:...}` so the doc shows
`${eddivault:...}` for the legacy form and `${vault:...}` for the new form).
In `@docs/global-variables.md`:
- Around line 7-20: Multiple fenced code blocks in the document lack language
identifiers (MD040); update each triple-backtick fence shown (the ASCII diagram
block and the subsequent short example blocks that display assistant config and
template lists) to include a language tag (e.g., use ```text) so markdownlint
stops flagging them; ensure you replace each opening ``` with ```text for the
diagram block and the repeated small blocks (the ones containing "You are an AI
assistant...", "1. Jinja2/Qute templates...", and "You are an AI assistant for
{{vars.company-name}}...") and any other fenced blocks referenced in the
comment.
In `@docs/secrets-vault.md`:
- Around line 39-47: Add a language tag to the fenced code blocks for the vault
examples so markdownlint stops flagging them: update the two code fences that
show the vault references (${vault:keyName} and ${vault:tenantId/keyName}) to
use ```text instead of plain ```, preserving the contents exactly.
In `@src/main/java/ai/labs/eddi/backup/impl/RestImportService.java`:
- Around line 410-413: The previewImport path is missing the new vault rewrite,
so update previewImport() to call normalizeVaultReferences(agentFileString) in
the same place the real import flow does (i.e., after
normalizeLegacyUris(agentFileString)) so previews match actual imports; apply
the same change to the other preview code paths mentioned (the blocks around the
other occurrences at the same logical points) to ensure all preview branches
call normalizeVaultReferences() just like the import path.
In `@src/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.java`:
- Around line 204-206: getTemplateData(String tenantId) currently forwards the
raw tenantId to loadVariables(...) causing inconsistent behavior with
resolveValue(...); normalize tenantId the same way resolveValue does by
replacing a null tenantId with GlobalVariable.DEFAULT_TENANT before calling
loadVariables(tenantId), so getTemplateData and loadVariables use the same
fallback logic.
In
`@src/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.java`:
- Around line 63-69: The upsertVariable method dereferences the incoming
GlobalVariable parameter without null-checking; add a guard at the start of
upsertVariable to validate that the variable argument is not null (and return a
proper client error/Response.badRequest when it is) before calling
variable.value()/variable.description()/variable.exportable(), then proceed to
construct the new GlobalVariable and call store.upsert(toStore); keep existing
validateId(tenantId, "tenantId") and validateId(key, "key") behavior and
reference the method name upsertVariable, the GlobalVariable constructor usage,
and store.upsert when applying the fix.
In
`@src/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.java`:
- Around line 64-94: The code currently swallows SQLExceptions in ensureSchema()
and getAll(String tenantId), returning empty results on DB errors; change the
catches to rethrow an unchecked exception (e.g., new RuntimeException or a
project DataAccessException) with a clear message and the original SQLException
as the cause instead of just logging and returning empty values, and apply the
same change to the other read methods in this class (the methods around lines
153-170) so DB outages surface as failures rather than silent empty results.
In `@src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java`:
- Around line 59-61: Update the ToolArg description for the apiKey parameter in
McpSetupTools so it accurately reflects which cloud providers require an API key
(e.g., include "anthropic, openai, gemini" as requiring apiKey) and explicitly
exclude providers that do not require apiKey like "bedrock" and "oracle-genai";
modify the ToolArg text on the apiKey parameter declaration and the second
duplicate description occurrence (both referencing the apiKey ToolArg in
McpSetupTools) to state that apiKey is required only for certain cloud providers
and optional/unused for local LLMs and cloud providers that do not need it, and
keep the note about vault references (e.g., '${vault:openai-key}').
In `@src/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.java`:
- Around line 318-322: warnIfRawKey currently flags any apiKey that doesn't
start with "${vault:" or "${eddivault:" as raw; update the condition in
warnIfRawKey to also accept the new global-variable resolver prefix "${vars:"
(i.e., do not warn when apiKey.startsWith("${vars:")). Modify the boolean check
used before LOGGER.warnf in warnIfRawKey so it returns early for "${vars:" as
well, keeping the same warning message and behavior otherwise.
---
Outside diff comments:
In `@src/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.java`:
- Around line 109-115: The requestMap is being sanitized only for headers via
scrubSensitiveHeaders before calling prePostUtils.createMemoryEntry(currentStep,
requestMap, objectName, KEY_HTTP_CALLS), leaving query params and body fields
with resolved secret values in conversation memory; update the code so that
requestMap's query parameters and body payload are scrubbed as well (either
extend scrubSensitiveHeaders to also scrub requestMap.query and requestMap.body,
or add a new scrubSensitiveBodyAndQuery(requestMap) helper) and invoke it before
createMemoryEntry; apply the same change to the other symmetric location where
createMemoryEntry is called for API requests so no query/body secrets are
persisted.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactory.java`:
- Around line 59-67: The cache key is built from unresolved embedding parameters
(paramKey/cacheKey) so updates to variables/secrets won't change the key; fix by
resolving variables and secrets before computing the cache key: call
globalVariableResolver.resolveAll(...) and then
secretResolver.resolveSecrets(...) on config.getEmbeddingParameters(), normalize
the resulting map (e.g., new TreeMap<>(resolvedParams)) and use that for
paramKey/cacheKey when calling cache.get(...), and optionally mirror
ChatModelRegistry's `@PostConstruct` invalidation listener to evict entries when
resolvers update.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.java`:
- Around line 65-70: The cache key currently uses raw
config.getStoreParameters(), so changes to resolved tenant/vault values aren't
reflected; modify getOrCreate to call resolveParams(config) (or otherwise obtain
the resolved/storeParameters map) and build the paramsKey from that resolved map
instead of config.getStoreParameters(), then use that same resolved parameters
when invoking build(config, kbId) (or adapt build to accept the resolved map) so
the cached entry matches the actual connection details; refer to getOrCreate,
resolveParams, build, cache.get and clearCache when making the change.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.java`:
- Around line 446-447: The persisted identifiers use task.getType() (the
late-bound template) instead of the resolved runtime type; update all key
constructions (e.g., where KEY_LANGCHAIN + ":" + task.getType() + ":" +
task.getId()) to use the resolvedType variable instead so trace, audit and
output keys match the actual executed model. Locate usages in LlmTask where keys
are built for langchain, trace/audit and output (e.g., currentStep.storeData,
any trace/audit store calls, and response content keys) and replace
task.getType() with resolvedType for those concatenations; ensure resolvedType
is computed before these calls and used consistently across the other
occurrences mentioned.
In `@src/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.java`:
- Around line 151-167: getOrCreateClient currently keys cached MCP clients only
by config.getUrl(), but createTransport bakes the resolved Authorization header
from createTransport() into the client; update cache behavior in
McpToolProviderManager so cached clients are invalidated or keyed by the
resolved auth material. Specifically, in getOrCreateClient (and any client cache
map) include the resolved API key or resolved auth header (the value produced in
createTransport as resolvedKey) in the cache key, or detect when
globalVariableResolver/secretResolver returns a different value and evict the
existing client before creating a new one; ensure you still resolve apiKey using
globalVariableResolver.resolveValue and secretResolver.resolveValue (as in
createTransport) when computing the cache key or eviction check so rotated
credentials produce a new transport/client.
In `@src/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.java`:
- Around line 382-403: Update the Javadoc for
PropertySetterTask.autoVaultSecret(...) to document the degraded-mode fallback:
explicitly state that if secretProvider.store(...) fails the method will return
the original plaintext (i.e., vaulting did not occur), that conversation memory
scrubbing still runs, and callers should treat the returned string as
potentially non-vaulted; reference the method name autoVaultSecret(),
secretProvider.store(...), and the parameters memory, keyName, plaintext so
readers can find the behavior in the code.
---
Nitpick comments:
In
`@src/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.java`:
- Around line 87-92: The validateId method currently throws
IllegalArgumentException; change it to throw javax.ws.rs.BadRequestException
with the same sanitized message to align with other REST handlers. Update the
throw in validateId(String value, String fieldName) to new
BadRequestException(fieldName + " must match [a-zA-Z0-9_.\\-]+ (letters, digits,
dots, underscores, hyphens). Got: " + sanitize(value)), ensure
javax.ws.rs.BadRequestException is imported, and leave ID_PATTERN and sanitize
usages unchanged.
In `@src/test/java/ai/labs/eddi/integration/GlobalVariableCrudIT.java`:
- Around line 134-145: The test method invalidKey() currently allows a 500
response for invalid client input; update the assertion in that test to require
a 400 only (remove the anyOf(equalTo(400), equalTo(500)) and replace with
statusCode(equalTo(400))) so PUT to BASE_PATH + "/" + TENANT + "/bad key!" fails
the request as a bad request; keep the same request body and contentType but
tighten the assertion to explicitly expect HTTP 400 to prevent masking server
errors.
🪄 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: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e8727f4-dfc8-4dd9-b4cd-178a58823ccb
📒 Files selected for processing (89)
AGENTS.mddocs/SUMMARY.mddocs/a2a-protocol.mddocs/audit-ledger.mddocs/changelog.mddocs/developer-quickstart.mddocs/global-variables.mddocs/how-to....mddocs/mcp-server.mddocs/model-cascade.mddocs/rag.mddocs/release-notes-6.0.2.mddocs/secrets-vault.mddocs/slack-integration.mdsrc/main/java/ai/labs/eddi/backup/impl/AbstractBackupService.javasrc/main/java/ai/labs/eddi/backup/impl/RestImportService.javasrc/main/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsConfiguration.javasrc/main/java/ai/labs/eddi/configs/rag/model/RagConfiguration.javasrc/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.javasrc/main/java/ai/labs/eddi/configs/variables/IGlobalVariableStore.javasrc/main/java/ai/labs/eddi/configs/variables/model/GlobalVariable.javasrc/main/java/ai/labs/eddi/configs/variables/mongo/GlobalVariableStore.javasrc/main/java/ai/labs/eddi/configs/variables/rest/IRestGlobalVariableStore.javasrc/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.javasrc/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.javasrc/main/java/ai/labs/eddi/engine/setup/AgentSetupService.javasrc/main/java/ai/labs/eddi/integrations/slack/SlackChannelRouter.javasrc/main/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutor.javasrc/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/impl/ChatModelRegistry.javasrc/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactory.javasrc/main/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactory.javasrc/main/java/ai/labs/eddi/modules/llm/impl/LlmTask.javasrc/main/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManager.javasrc/main/java/ai/labs/eddi/modules/llm/model/LlmConfiguration.javasrc/main/java/ai/labs/eddi/modules/properties/impl/PropertySetterTask.javasrc/main/java/ai/labs/eddi/secrets/ISecretProvider.javasrc/main/java/ai/labs/eddi/secrets/SecretResolver.javasrc/main/java/ai/labs/eddi/secrets/VaultStartupBanner.javasrc/main/java/ai/labs/eddi/secrets/model/SecretReference.javasrc/main/java/ai/labs/eddi/secrets/sanitize/SecretRedactionFilter.javasrc/main/java/ai/labs/eddi/secrets/sanitize/SecretScrubber.javasrc/main/resources/META-INF/resources/assets/cssMode-BHkIakpN.jssrc/main/resources/META-INF/resources/assets/freemarker2-DMS7G5iu.jssrc/main/resources/META-INF/resources/assets/handlebars-DNBNwtfL.jssrc/main/resources/META-INF/resources/assets/html-BWbqU3gc.jssrc/main/resources/META-INF/resources/assets/htmlMode-DdcsDH59.jssrc/main/resources/META-INF/resources/assets/index-9Ly7lA6T.csssrc/main/resources/META-INF/resources/assets/index-Db_KEYcO.jssrc/main/resources/META-INF/resources/assets/index-_fhTfcdI.csssrc/main/resources/META-INF/resources/assets/javascript-inUojZyW.jssrc/main/resources/META-INF/resources/assets/jsonMode-D9dJjUHG.jssrc/main/resources/META-INF/resources/assets/liquid-CMO_zke7.jssrc/main/resources/META-INF/resources/assets/lspLanguageFeatures-D9UefQDj.jssrc/main/resources/META-INF/resources/assets/mdx-BNrhjuw1.jssrc/main/resources/META-INF/resources/assets/python-C1D6UJRn.jssrc/main/resources/META-INF/resources/assets/razor-1ODnFjXu.jssrc/main/resources/META-INF/resources/assets/tsMode-CfRCQJyg.jssrc/main/resources/META-INF/resources/assets/typescript-sC3c-Jld.jssrc/main/resources/META-INF/resources/assets/xml-e4Ud4yx9.jssrc/main/resources/META-INF/resources/assets/yaml-BjPKFs2h.jssrc/main/resources/META-INF/resources/manage.htmlsrc/test/java/ai/labs/eddi/configs/mcpcalls/model/McpCallsModelsTest.javasrc/test/java/ai/labs/eddi/configs/variables/GlobalVariableResolverTest.javasrc/test/java/ai/labs/eddi/configs/variables/model/GlobalVariableTest.javasrc/test/java/ai/labs/eddi/configs/variables/mongo/GlobalVariableStoreTest.javasrc/test/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStoreTest.javasrc/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.javasrc/test/java/ai/labs/eddi/engine/mcp/McpSetupToolsTest.javasrc/test/java/ai/labs/eddi/integration/GlobalVariableCrudIT.javasrc/test/java/ai/labs/eddi/integration/postgres/PostgresGlobalVariableCrudIT.javasrc/test/java/ai/labs/eddi/integrations/slack/SlackChannelRouterTest.javasrc/test/java/ai/labs/eddi/modules/apicalls/impl/ApiCallExecutorTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerExtendedTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManagerTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/ChatModelRegistryTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/EmbeddingModelFactoryTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/EmbeddingStoreFactoryTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/LlmTaskTest.javasrc/test/java/ai/labs/eddi/modules/llm/impl/McpToolProviderManagerTest.javasrc/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationModelsTest.javasrc/test/java/ai/labs/eddi/modules/llm/model/LlmConfigurationTest.javasrc/test/java/ai/labs/eddi/secrets/SecretResolverTest.javasrc/test/java/ai/labs/eddi/secrets/impl/SecretVaultIntegrationTest.javasrc/test/java/ai/labs/eddi/secrets/model/SecretReferenceTest.javasrc/test/java/ai/labs/eddi/secrets/rest/RestSecretStoreTest.javasrc/test/java/ai/labs/eddi/secrets/sanitize/SecretRedactionFilterTest.javasrc/test/java/ai/labs/eddi/secrets/sanitize/SecretScrubberTest.java
…, null guard, exception propagation, docs - Add IGlobalVariableStore CDI producer to DataStoreProducers (fixes ambiguous bean) - Add null body guard in RestGlobalVariableStore.upsertVariable() (NPE to 400) - Normalize null tenantId in GlobalVariableResolver.getTemplateData() - Propagate SQLExceptions in PostgresGlobalVariableStore (not silent empty) - Recognize vars prefix in A2AToolProviderManager.warnIfRawKey() - Refine MCP API key @ToolArg descriptions (bedrock/oracle-genai) - Assert statusCode(400) only in GlobalVariableCrudIT invalid key test - Fix changelog: eddivar to vars, A2A typo, test count, composite key desc - Add MD040 language tags to global-variables.md and secrets-vault.md
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (5)
src/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.java (2)
205-252: ⚡ Quick win
listAllSQL-exception path has no test coverage.
getAllSqlException,getSqlException,upsertSqlException, anddeleteSqlExceptionare all present, but there is nolistAllSqlExceptiontest, leaving thelistAllcatchblock unverified.🧪 Suggested addition
`@Test` `@DisplayName`("listAll — propagates SQLException as RuntimeException") void listAllSqlException() throws Exception { PreparedStatement ps = mock(PreparedStatement.class); when(connection.prepareStatement(anyString())).thenReturn(ps); when(ps.executeQuery()).thenThrow(new SQLException("test error")); RuntimeException ex = assertThrows(RuntimeException.class, () -> store.listAll(DEFAULT)); assertTrue(ex.getMessage().contains("Failed to list all global variables")); assertInstanceOf(SQLException.class, ex.getCause()); }🤖 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 `@src/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.java` around lines 205 - 252, Add a new unit test method listAllSqlException mirroring the existing getAllSqlException/upsertSqlException patterns: mock a PreparedStatement (ps), stub when(connection.prepareStatement(anyString())).thenReturn(ps) and when(ps.executeQuery()).thenThrow(new SQLException("test error")), then assertThrows(RuntimeException.class, () -> store.listAll(DEFAULT)) and verify the exception message contains "Failed to list all global variables" and that ex.getCause() is an instance of SQLException; place the test alongside the other error-handling tests.
256-267: ⚡ Quick win
atMost(1)doesn't validate that schema initialization actually ran.If
schemaInitializedlogic breaks andcreateStatement()is never called, this assertion still passes. Usetimes(1)to verify exactly-once initialization across twogetAll()calls.🔧 Proposed fix
- verify(connection, atMost(1)).createStatement(); + verify(connection, times(1)).createStatement();🤖 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 `@src/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.java` around lines 256 - 267, The test ensureSchemaIdempotent currently uses verify(connection, atMost(1)).createStatement(), which doesn't assert that schema initialization ran; change this to verify(connection, times(1)).createStatement() so that after calling store.getAll(DEFAULT) twice the createStatement() call must occur exactly once; locate the ensureSchemaIdempotent test and replace the atMost(1) verification with times(1) referencing connection.createStatement() and the two store.getAll(DEFAULT) invocations.src/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.java (1)
43-43: ⚡ Quick winAdd Micrometer metrics to this new store.
No timers or counters are wired for any of the CRUD operations. Per coding guidelines, all new features must instrument with
MeterRegistry.A minimal starting point for
getAll:📊 Suggested instrumentation sketch
+import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.Timer; ... private static final Logger LOGGER = Logger.getLogger(PostgresGlobalVariableStore.class); + private final MeterRegistry meterRegistry; + private Timer getAllTimer; + private Timer getTimer; + private Timer upsertTimer; + private Timer deleteTimer; `@Inject` - public PostgresGlobalVariableStore(Instance<DataSource> dataSourceInstance) { + public PostgresGlobalVariableStore(Instance<DataSource> dataSourceInstance, MeterRegistry meterRegistry) { this.dataSourceInstance = dataSourceInstance; + this.meterRegistry = meterRegistry; } + + `@PostConstruct` + void initMetrics() { + getAllTimer = Timer.builder("globalvariable.store.getAll").register(meterRegistry); + getTimer = Timer.builder("globalvariable.store.get").register(meterRegistry); + upsertTimer = Timer.builder("globalvariable.store.upsert").register(meterRegistry); + deleteTimer = Timer.builder("globalvariable.store.delete").register(meterRegistry); + }Wrap each method body with
Timer.record(...). Apply the same pattern tolistAll.As per coding guidelines: "Always add metrics using Micrometer MeterRegistry to new features for observability. Use appropriate metric types: counters, timers, gauges."
🤖 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 `@src/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.java` at line 43, The new PostgresGlobalVariableStore class lacks Micrometer metrics; wrap the CRUD method bodies (at least getAll and listAll) with Timer.record using an injected MeterRegistry, and add counters for errors/successes where appropriate; specifically, add a MeterRegistry field to PostgresGlobalVariableStore, initialize it via the constructor, then in getAll() and listAll() call Timer.record(() -> { /* existing method body */ }, meterRegistry.timer("postgres.globalVariableStore.getAll")) (and a separate timer name for listAll) and increment a Counter on exceptions to track failures.src/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.java (2)
79-83: ⚡ Quick win
maximumSize(32)is hardcoded while the TTL is already configurable.A deployment with more than 32 active tenants will experience constant LRU eviction, effectively negating the cache and doubling DB reads per request. Making the size configurable is a one-liner matching the existing
cacheTtlMinutespattern.♻️ Proposed fix
`@Inject` public GlobalVariableResolver(IGlobalVariableStore store, - `@ConfigProperty`(name = "eddi.variables.cache-ttl-minutes", defaultValue = "2") int cacheTtlMinutes) { + `@ConfigProperty`(name = "eddi.variables.cache-ttl-minutes", defaultValue = "2") int cacheTtlMinutes, + `@ConfigProperty`(name = "eddi.variables.cache-max-tenants", defaultValue = "32") int cacheMaxTenants) { this.store = store; this.cacheTtlMinutes = cacheTtlMinutes; + this.cacheMaxTenants = cacheMaxTenants; } ... void init() { this.cache = Caffeine.newBuilder() .expireAfterWrite(Duration.ofMinutes(cacheTtlMinutes)) - .maximumSize(32) + .maximumSize(cacheMaxTenants) .build();🤖 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 `@src/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.java` around lines 79 - 83, The cache maximum size is hardcoded (maximumSize(32)) in GlobalVariableResolver which causes evictions for >32 tenants; make the maximum size configurable similar to cacheTtlMinutes by adding/using a configuration parameter (e.g., cacheMaxSize) and replace maximumSize(32) with maximumSize(cacheMaxSize) when building this.cache via Caffeine.newBuilder(), and update the LOGGER.infof to include the configured cacheMaxSize for clarity.
70-84: ⚡ Quick winAdd Micrometer metrics to the resolver.
Cache hit/miss rates and resolution latency are the primary observability signals for diagnosing fleet-wide config stalls. Caffeine supports
recordStats()which integrates directly with Micrometer.📊 Suggested instrumentation sketch
+import io.micrometer.core.instrument.MeterRegistry; +import io.micrometer.core.instrument.binder.cache.CaffeineCacheMetrics; ... private final IGlobalVariableStore store; private final int cacheTtlMinutes; + private final MeterRegistry meterRegistry; private Cache<String, Map<String, String>> cache; `@Inject` public GlobalVariableResolver(IGlobalVariableStore store, - `@ConfigProperty`(name = "eddi.variables.cache-ttl-minutes", defaultValue = "2") int cacheTtlMinutes) { + `@ConfigProperty`(name = "eddi.variables.cache-ttl-minutes", defaultValue = "2") int cacheTtlMinutes, + MeterRegistry meterRegistry) { this.store = store; this.cacheTtlMinutes = cacheTtlMinutes; + this.meterRegistry = meterRegistry; } `@PostConstruct` void init() { this.cache = Caffeine.newBuilder() .expireAfterWrite(Duration.ofMinutes(cacheTtlMinutes)) .maximumSize(32) + .recordStats() .build(); + CaffeineCacheMetrics.monitor(meterRegistry, cache, "globalvariable.resolver.cache");As per coding guidelines: "Always add metrics using Micrometer MeterRegistry to new features for observability."
🤖 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 `@src/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.java` around lines 70 - 84, Inject a Micrometer MeterRegistry into GlobalVariableResolver (add a constructor parameter or `@Inject` field) and enable Caffeine stats in init(): call recordStats() on the Caffeine builder, keep building the cache into the existing cache field, then register the cache with Micrometer using CaffeineCacheMetrics.monitor/bind (e.g., CaffeineCacheMetrics.monitor(meterRegistry, cache, "globalVariableResolver.cache")) so hit/miss rates and latency are exposed; reference the GlobalVariableResolver class, init() method, cache field, cacheTtlMinutes and the existing constructor/store injection when adding the MeterRegistry and the recordStats() call.
🤖 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.
Nitpick comments:
In `@src/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.java`:
- Around line 79-83: The cache maximum size is hardcoded (maximumSize(32)) in
GlobalVariableResolver which causes evictions for >32 tenants; make the maximum
size configurable similar to cacheTtlMinutes by adding/using a configuration
parameter (e.g., cacheMaxSize) and replace maximumSize(32) with
maximumSize(cacheMaxSize) when building this.cache via Caffeine.newBuilder(),
and update the LOGGER.infof to include the configured cacheMaxSize for clarity.
- Around line 70-84: Inject a Micrometer MeterRegistry into
GlobalVariableResolver (add a constructor parameter or `@Inject` field) and enable
Caffeine stats in init(): call recordStats() on the Caffeine builder, keep
building the cache into the existing cache field, then register the cache with
Micrometer using CaffeineCacheMetrics.monitor/bind (e.g.,
CaffeineCacheMetrics.monitor(meterRegistry, cache,
"globalVariableResolver.cache")) so hit/miss rates and latency are exposed;
reference the GlobalVariableResolver class, init() method, cache field,
cacheTtlMinutes and the existing constructor/store injection when adding the
MeterRegistry and the recordStats() call.
In
`@src/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.java`:
- Line 43: The new PostgresGlobalVariableStore class lacks Micrometer metrics;
wrap the CRUD method bodies (at least getAll and listAll) with Timer.record
using an injected MeterRegistry, and add counters for errors/successes where
appropriate; specifically, add a MeterRegistry field to
PostgresGlobalVariableStore, initialize it via the constructor, then in getAll()
and listAll() call Timer.record(() -> { /* existing method body */ },
meterRegistry.timer("postgres.globalVariableStore.getAll")) (and a separate
timer name for listAll) and increment a Counter on exceptions to track failures.
In
`@src/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.java`:
- Around line 205-252: Add a new unit test method listAllSqlException mirroring
the existing getAllSqlException/upsertSqlException patterns: mock a
PreparedStatement (ps), stub
when(connection.prepareStatement(anyString())).thenReturn(ps) and
when(ps.executeQuery()).thenThrow(new SQLException("test error")), then
assertThrows(RuntimeException.class, () -> store.listAll(DEFAULT)) and verify
the exception message contains "Failed to list all global variables" and that
ex.getCause() is an instance of SQLException; place the test alongside the other
error-handling tests.
- Around line 256-267: The test ensureSchemaIdempotent currently uses
verify(connection, atMost(1)).createStatement(), which doesn't assert that
schema initialization ran; change this to verify(connection,
times(1)).createStatement() so that after calling store.getAll(DEFAULT) twice
the createStatement() call must occur exactly once; locate the
ensureSchemaIdempotent test and replace the atMost(1) verification with times(1)
referencing connection.createStatement() and the two store.getAll(DEFAULT)
invocations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 1f445f48-b5ab-40ab-b0bf-3318fc153b8a
📒 Files selected for processing (12)
docs/changelog.mddocs/global-variables.mddocs/secrets-vault.mdsrc/main/java/ai/labs/eddi/configs/variables/GlobalVariableResolver.javasrc/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.javasrc/main/java/ai/labs/eddi/datastore/DataStoreProducers.javasrc/main/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStore.javasrc/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.javasrc/main/java/ai/labs/eddi/modules/llm/impl/A2AToolProviderManager.javasrc/test/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStoreTest.javasrc/test/java/ai/labs/eddi/datastore/postgres/PostgresGlobalVariableStoreTest.javasrc/test/java/ai/labs/eddi/integration/GlobalVariableCrudIT.java
✅ Files skipped from review due to trivial changes (3)
- src/main/java/ai/labs/eddi/engine/mcp/McpSetupTools.java
- docs/secrets-vault.md
- docs/changelog.md
🚧 Files skipped from review as they are similar to previous changes (2)
- src/test/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStoreTest.java
- src/main/java/ai/labs/eddi/configs/variables/rest/RestGlobalVariableStore.java
Resolve conflicts: - changelog.md: keep both sets of entries (PR remediation + CVE/Slack fixes) - RagConfiguration.java: adopt Gemini/Vertex entries with canonical vault syntax - UI assets: accept main's build artifacts (manage.html + hashed JS bundles)
…separator - Remove 17 stale asset files from old branch build (manage.html references main's hashes) - Add missing --- separator between Slack Notification and Late-Binding Rename entries
rolandpickl
approved these changes
May 5, 2026
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
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.
Summary
Adds a Global Variable Store — a non-encrypted, tenant-scoped key-value store for operational configuration (default LLM model, API endpoints, feature flags). Enables fleet-wide config changes across all agents without redeployment. Also renames late-binding prefixes for consistency:
eddivault→vault.What's New
Global Variable Store
GlobalVariablerecord withtenantId,key,value,description,exportable_id: tenantId/key) and PostgreSQL (PK(tenant_id, "key")) adapters with full parity${vars:key}(default tenant) and${vars:tenantId/key}(explicit tenant) late-binding resolution with per-tenant Caffeine cache (2-min TTL) and downstream invalidation listenersGET/PUT/DELETE /variablestore/variables/{tenantId}/{key}with input validation and cache invalidation{{vars.<key>}}in system prompts viagetTemplateData()Prefix Rename
${eddivault:...}→${vault:...}(backward-compat alias retained via dual-pattern regex)${eddivault:...}→${vault:...}Bug Fixes
task.getType()used unresolved in token counter and streaming paths — hoistedresolvedTypenulltenantId now defaults to"default""key"and"value"in all DDL/DMLTests
GlobalVariableTestGlobalVariableResolverTestRestGlobalVariableStoreTestGlobalVariableStoreTest(Mongo)PostgresGlobalVariableStoreTestGlobalVariableCrudITPostgresGlobalVariableCrudITDocumentation
docs/global-variables.md— architecture, syntax, REST API, use cases, comparison tableAGENTS.md,docs/secrets-vault.md, 10+ doc files with new prefix syntaxdocs/changelog.mdwith full entryFiles Changed
89 files changed, +3,153 / −624
Manager UI
Includes updated Manager UI assets with the new Global Variables admin page (
/manage/variables).Summary by CodeRabbit
New Features
Documentation
Improvements