[#66020] Add LLM connection settings to admin AI configuration - #24851
Closed
tangopium wants to merge 46 commits into
Closed
[#66020] Add LLM connection settings to admin AI configuration#24851tangopium wants to merge 46 commits into
tangopium wants to merge 46 commits into
Conversation
Introduces the persistence layer for the LLM server connection: base URL, API key, enabled flag, the designated default chat/embedding models and a verbatim copy of the remote model catalogue. The API key is ciphered through Redmine::Ciphering, matching LdapAuthSource. Note this is a no-op unless database_cipher_key is configured, which no packaged install does today. Only one connection is supported for now, enforced by a validation rather than by the schema so that lifting the restriction later needs no migration. Adds the llm_connection setting (writable: false, format: :hash) as the ENV carrier for headless provisioning, and the llm_connection feature flag. https://community.openproject.org/work_packages/66020
A thin client exposing #models against a base URL that already carries the API version segment, as every provider documents and every OpenAI client library expects. The catalogue is returned verbatim rather than normalised: vLLM adds max_model_len and root to each model card, and that is the only trustworthy source for a deployment's real context window. Two details worth knowing: * transport failures are HTTPX::ErrorResponse instances, while a response carrying a 4xx also populates #error (it delegates to #raise_for_status). The response class, not #error, distinguishes the two -- checking #error first swallows every status and makes the error taxonomy unreachable. * the body is parsed directly rather than through HTTPX's #json, which insists on a JSON content type that self-hosted servers behind a proxy do not reliably set. Timeouts are overridden explicitly; the global httpx defaults (connect 3s, read 3s, request 10s) are tuned for storage calls and cannot serve inference. https://community.openproject.org/work_packages/66020
Saving the connection now proves the server is reachable and the credentials are accepted before anything is written, so a failed connect persists nothing. This follows the storages precedent, where the Nextcloud credentials validator adds a contract error on 401 and stops the write. Two constraints shape where the probe lives: * It is declared on UpdateContract, never on BaseContract. Provisioning from the environment reuses the base contract, and a `validates` line cannot be un-declared by a subclass -- an outbound request during seeding would fail the boot of a container whose LLM server has not started yet. EnvironmentUpdateContract therefore inherits from BaseContract. * The probe is guarded on the credential attributes. Without that guard every unrelated save, and every form render that builds a model through SetAttributesService, would fire an outbound request. The base URL is normalised only by stripping whitespace and a trailing slash. The /v1 segment is deliberately neither added nor removed: silently rewriting an administrator's URL makes the eventual failure harder to diagnose. https://community.openproject.org/work_packages/66020
On-premise and containerised deployments configure the connection through OPENPROJECT_LLM__CONNECTION_* variables, so an instance comes up connected without anyone opening the administration UI. Seeding never contacts the LLM server. The catalogue refresh is enqueued as Llm::SyncModelsJob instead, which is what lets a container whose LLM sidecar has not started yet finish seeding: with a dead server the seed completes in about a tenth of a second rather than blocking on a probe. Unknown keys raise the same actionable error the LDAP seeder gives, including the single-vs-double underscore explanation -- writing BASE_URL instead of BASE__URL parses as a nested base.url hash and would otherwise be silently ignored. Once provisioned this way the connection is read-only in the UI, guarded server-side by the existing configured_via_env contract error. https://community.openproject.org/work_packages/66020
Adds Administration -> AI -> LLM settings, a sibling of the MCP page, behind the llm_connection feature flag. Saving is connecting: the contract proves the server reachable, so there is no separate "test" button that could report green without persisting anything. The form shows a spinner while that round trip happens. The API key is write-only. The stored value is never sent to the browser, a blank submission keeps the current key, and a separate action removes it -- without which a key could never be cleared once set. The Stimulus controller also wipes the field on turbo:before-cache, so the back button cannot restore a typed secret, and marks the submit button aria-disabled rather than disabled, which would move focus to <body> and drop keyboard users to the top of the page. Departing from the mockup, the host and key fields are always visible instead of being revealed by the Enable checkbox. Hiding them behind the checkbox deadlocks: the fields only appear once enabled is saved, but enabling without a configured connection is rejected. Enable is a pure kill switch here. The model list renders from the cached catalogue, so opening the page never issues an HTTP request; refreshing is explicit. Context windows come from vLLM's max_model_len, which reflects the operator's actual --max-model-len. https://community.openproject.org/work_packages/66020
Contract and request specs covering the connect flow, the error taxonomy and the API key lifecycle, plus a shared LlmServerHelpers module for stubbing an OpenAI-compatible server. Three of these pin behaviour that is easy to regress silently: * the probe fires only when the credentials change, asserted by resetting WebMock's executed requests and re-validating. Without the guard every unrelated save would reach out to the LLM server. * a failed connect leaves the database untouched. * a blank API key submission keeps the stored key rather than clearing it. Contract specs are tagged :check_errors_i18n so a missing locale key for any new symbolic error code fails the build. https://community.openproject.org/work_packages/66020
Features declare themselves, the kind of model they need and the capabilities they require, so the administration UI can show which models are usable for which job instead of offering an undifferentiated list. The registry lives in lib_static because it is populated from initializers, which run before eager loading. Constants under app/ would be unloaded on a development reload and lose their registrations -- the same reason OpenProject::FeatureDecisions lives there. The capability vocabulary is deliberately two entries. Only :embeddings is a hard gate: it cannot be emulated, being a different endpoint answering with vectors. :structured_output is advisory, because responses have to be validated and repaired whatever the server claims, which makes constrained decoding an optimisation rather than a requirement. Tool calling is absent on purpose -- no vLLM endpoint reports whether its tool parser is configured, so a verdict there could only ever be a guess on our own primary stack. https://community.openproject.org/work_packages/66020
Two tables, both keyed on the model id as a plain string rather than a foreign key. The catalogue is a cache of a remote list, so a model can disappear from it; a binding has to survive that and say so, which a foreign key would make impossible. Verdicts carry three states, not a boolean, because the honest answer is very often "we cannot tell": the OpenAI model list carries no capability data at all and only some servers offer a non-standard endpoint that does. Only :unsupported blocks. Refusing on :unknown would make most self-hosted servers unusable. The source (metadata, probe, admin, observed) is orthogonal, so an administrator's assertion can be displayed as such without adding a fourth state every caller would have to handle. Bindings are one row per registered feature and are never destroyed when a feature deregisters, so flipping a feature flag does not lose the choice. Whether a binding is dangling is derived rather than stored: a status column would be a cache with no invalidation trigger, stale exactly when it matters. https://community.openproject.org/work_packages/66020
Embeddings is the one capability worth probing: it cannot be emulated, and a server either returns a vector or it does not. The verdict also yields the dimension count, which semantic search needs to size its index. The 200 case is judged by the shape of the body, not by the status. vLLM, llama.cpp and Ollama all silently drop parameters they do not understand, so a 200 on its own proves nothing -- a response without a numeric vector is recorded as unknown rather than supported. There is deliberately no tool-calling probe. Several current vLLM releases return 200 with the tool call as plain text on a fully configured server, and they do it deterministically, so retrying would launder a wrong answer into a confident one instead of correcting it. Probing is rationed. A gateway can list hundreds of models and some providers bill per request, so only the model an administrator is binding gets probed synchronously; after a connect, at most ten models whose names suggest they are embedders are probed in the background. Everything else stays unknown, which never blocks. Verdicts are discarded when the base URL or key changes, because that is a different deployment -- including administrator assertions, which were about the old one. When a model merely disappears from the same server, only probed verdicts are dropped: an operator restarting a server must not silently lose an assertion they made. https://community.openproject.org/work_packages/66020
Every consuming feature asks the same question -- which model do I use, and can I run right now -- so it is answered in one place and every feature agrees on what an unset value means: per-item override -> feature binding -> connection default -> unbound A blank value at any level inherits from the level below. Resolution fails closed and never substitutes. When the chosen model is no longer in the server's catalogue the status is model_missing, not a quiet fallback to the default: a text transform run through a different model is a different feature, and silently swapping it produces exactly the "why did the output change" report that nobody can diagnose. Only a definite unsupported verdict blocks. Unknown -- the normal state for a server that reports nothing about its models -- is surfaced as a warning and still runs, because refusing on "we could not tell" would make most self-hosted servers unusable. https://community.openproject.org/work_packages/66020
One page where every registered feature picks its model, with the instance default as the inherited choice. Models are never hidden. Hiding one produces the single support question nobody can answer -- "why can I not pick the model I know works" -- and it is exactly wrong when most verdicts are unknown, which is the normal state for a server that reports nothing about its models. Instead each option says where it stands: plain when usable, "not verified" when a required capability is unknown, and disabled with the missing capability named when it is known absent. Choosing a model for a feature that requires one probes it immediately, because that is the verdict that actually matters. Features that require nothing never trigger a request. A binding whose model has vanished from the catalogue is flagged rather than quietly repointed, and a binding locked by indexed data cannot be changed here at all -- switching an embedding model is a re-index, not a swap. Two shape notes for anyone extending this: the block form of Rails' select writes to the template's output buffer rather than into the select element, so choices are built as pairs; and ApplicationComponent already owns the name `options`, which silently swallows a memoised method of the same name. https://community.openproject.org/work_packages/66020
The probe spec pins the decision rule that matters most: a 200 whose body is not an embedding response is recorded as unknown, not supported. vLLM, llama.cpp and Ollama all silently drop parameters they do not understand, so judging by status alone would mark every model on such a server as capable. The resolver spec pins the two behaviours features depend on: a model that has disappeared from the catalogue fails closed rather than falling back to the default, and an unknown capability verdict warns without blocking -- refusing on "we could not tell" would make most self-hosted servers unusable. https://community.openproject.org/work_packages/66020
A server can speak the OpenAI API for chat and still not expose a model list. OpenProject's own hosted stack does exactly that today: its gateway answers 401 for /v1/chat/completions, so the route is known and needs a key, while /v1/models answers 404 Route Not Found -- the endpoint from #77512, which has not shipped yet. Reporting that as "not an OpenAI-API-compatible endpoint" sends the administrator looking for the wrong problem. A 404 now says the server was reached but has no model list at the path we tried, and names that path, which covers both a wrong version segment and a server that genuinely lacks the endpoint. https://community.openproject.org/work_packages/66020
The model list was stored as a verbatim jsonb blob on the connection, wholly replaced on every refresh. That shape has no room for anything an administrator owns, which is a problem the moment models can be entered by hand: a manual entry would be erased by the next sync. Models are now rows keyed [llm_connection_id, external_id]. external_id is whatever this deployment calls the model -- Scaleway serves qwen3.6-35b-a3b for weights another catalogue lists as Qwen/Qwen3.6-35B-A3B -- so it stays an opaque string and is never used to look anything up in a public registry. A model the server stops offering is deactivated rather than deleted, so a binding or verdict pointing at it still has something to name. Also introduces an adapter seam. There is no universal model-discovery standard: OpenAI-compatible servers answer GET /models with data[].id, Gemini uses /v1beta/models, Bedrock needs AWS signing, and Azure indirects through deployment names. Only the OpenAI adapter exists; api_format selects it. https://community.openproject.org/work_packages/66020
Not every OpenAI-compatible server exposes a model list. OpenProject's own hosted gateway is the case in point: it routes /v1/chat/completions and answers 404 Route Not Found for /v1/models, so the connection can serve requests that OpenProject cannot discover. A manually added model is flagged and survives a refresh that cannot see it -- the server was never the thing that confirmed it, so a refresh cannot withdraw it either. Discovered models remain the server's to add and remove, and cannot be deleted from the UI. This unblocks using a working server whose model list is missing or not yet routed, without weakening the discovery path for servers that do expose one. https://community.openproject.org/work_packages/66020
api_format records which dialect the server speaks. Only "openai" is implemented and it is the default; the column exists so that adding Azure -- the one non-OpenAI format with a real constituency under #62215's bring-your-own-infrastructure goal -- is a new adapter rather than a migration. custom_headers is sent with every request, which is what Azure's api-version and gateway-specific headers will need. Note the class is spelled AddAPIFormatToLlmConnections: inflections.rb registers "API" as an acronym, so Rails resolves this filename to that constant and silently skips a class named any other way. The migration reported itself as pending while db:migrate did nothing. https://community.openproject.org/work_packages/66020
Adds ruby_llm and uses it for two things: model discovery on providers that do not speak the OpenAI model-list API, and published capability metadata. api_format now selects between two discovery strategies. OpenAI-compatible endpoints are queried live, so the list is what that endpoint actually serves. Anthropic, Gemini, Bedrock and the rest each list models differently -- and Bedrock needs request signing rather than a bearer token -- so their lists come from RubyLLM's registry. That is a published catalogue rather than a live query, which is worth knowing: it describes what the provider offers in general, not what these particular credentials can reach. Capabilities are filled in automatically where the registry knows the model: gpt-4o arrives with tool calling, structured output and vision set, and a context window. A self-hosted id it has never heard of yields nothing, which is the case an administrator resolves by hand. Registry-derived verdicts are recorded with source "metadata" and never overwrite one from a probe or an administrator. Both of those looked at this deployment; the registry did not, and it disagrees with itself across providers -- the same weights are catalogued with contradictory capability flags and context windows an order of magnitude apart. The capability vocabulary grows to what these sources actually report: tool calling, structured output, vision and reasoning, alongside embeddings. https://community.openproject.org/work_packages/66020
The connection page now offers the API format alongside the URL and key. OpenAI-compatible remains the default and covers most gateways and self-hosted servers; the others are the providers RubyLLM speaks. Validated against the adapter list rather than a free string, so an unknown format is rejected at save time instead of failing at first use. https://community.openproject.org/work_packages/66020
Where nothing publishes a model's capabilities -- a self-hosted server naming its model "default", or a gateway exposing no model list at all -- the operator knows what it can do and OpenProject does not. Each model now has an edit screen where capabilities can be set, alongside a display name. Assertions are stored as admin-sourced verdicts and survive re-detection: an administrator looked at this deployment, and neither a published registry nor a probe outranks that. Setting a capability back to "not specified" clears the assertion rather than recording ignorance as fact, so later detection can still fill it in. A capability established by a probe or the registry is shown with its source rather than silently presented as the administrator's own. https://community.openproject.org/work_packages/66020
A connection could not be saved unless the server returned a model list, which made every server without one unusable -- including OpenProject's own hosted gateway, which routes /v1/chat/completions and answers 404 for /v1/models. Worse, it made the manual model entry unreachable on exactly the connections that need it: you could not save the connection, so you never reached the page that lets you name the models yourself. Reachability and credentials still gate the save. A connection that times out, is refused, resolves to a blocked address, or has its key rejected is still an error, because those say the connection does not work. A 404, 405 or 501 on the model list says only that the server does not publish one, so the save proceeds and the administrator is told to add models by hand. Formats whose model list comes from the registry rather than the server no longer probe the base URL at all; there was never anything there to find. One limitation worth knowing: on a server that exposes no model list there is nothing cheap left to authenticate against, so a wrong API key is not detected at save time. Validating it would mean spending a chat completion. https://community.openproject.org/work_packages/66020
Three gaps in the model list, all visible on a manually added model. The edit action was unreachable: rows render their button_links only when the table declares has_actions?, which defaults to false, so the link existed and was never drawn. Context window could not be set. A hand-entered model has nothing to report one, so the column showed a dash with no way to fill it. It is now editable, with precedence: an administrator's figure, then the server's (vLLM and SGLang publish the operator's actual --max-model-len), then whatever a registry says about the model in general. Clearing the field falls back rather than blanking. Model type was tracked but invisible. It stays derived from the embeddings verdict rather than stored twice -- a model that produces vectors is an embedding model, and that is the same fact -- but the list now has a Type column reading Chat, Embedding or Unknown, and the edit screen says that marking Embeddings as supported is what makes a model an embedding one. The type lookup is built once per table rather than per row. https://community.openproject.org/work_packages/66020
A review against the rest of app/views/admin found four deviations. 28 admin views build forms with primer_form_with; the only two using raw select_tag and text_field_tag were these. Forms are now ApplicationForm subclasses like everything else. That is not only consistency: Primer inputs read builder.object.errors themselves, so validation failures render inline against the field that caused them. The hand-written versions reported them as a flash, or not at all. Capability assertions became virtual attributes on the model so they can be ordinary form fields, which lets the whole edit screen be one Primer form. A verdict from a probe or a registry is reported in the field's caption rather than loaded into it, so saving does not silently adopt someone else's finding as the administrator's. Adding a model followed no convention at all -- it was a form parked under the table. It is now a + action button leading to a new page, matching scim_clients and the Jira importer, and that page offers every field the edit screen does rather than just the name. Deleting a model asked for confirmation through the browser. It now opens a DangerDialog, which can say which features are bound to the model and will stop working. Test selectors added throughout, since OpenProject feature specs are built on within_test_selector and there were almost none. Still missing, and known: a feature spec, which is where be_axe_clean would check the accessibility of all this. https://community.openproject.org/work_packages/66020
Llm::Client could list models and ask for an embedding. Nothing could run a completion, so no registered feature had anything to call and a server that publishes no model list could not be verified at all. Llm::Session turns a connection into something that can issue requests, mapping api_format onto RubyLLM's per-provider configuration and applying the stored endpoint, credential and custom headers in one place. Three details are not obvious: It builds a per-call RubyLLM.context rather than calling RubyLLM.configure. The global configuration is process-wide, so writing an administrator's endpoint and credential into it would leak them across requests; a spec pins that the global config stays empty. It sets max_retries explicitly. RubyLLM retries POSTs three times by default, which would bill four completions for one call and multiply every timeout by four. It supplies a placeholder key when a provider demands one and the connection has none, because ensure_configured! otherwise raises -- breaking precisely the keyless self-hosted server this feature targets. The error taxonomy moves to Llm::Errors so both this path and model discovery report failures the same way, with Llm::Client keeping the old names as aliases. Llm::Errors.translate discards the incoming message deliberately: RubyLLM falls back to response.body, which routinely contains the submitted Authorization header and internal hostnames a gateway echoed back. Custom headers reach the wire through a prepended Provider#headers. RubyLLM's public API cannot express it -- Chat#with_headers reaches chat only, embeddings take no headers at all, and it merges additional headers under the provider's own so an override silently loses. Bedrock and Vertex AI are dropped from the format select and rejected by the contract: both need credentials beyond a single api_key. https://community.openproject.org/work_packages/66020
Llm::Runtime could say which model a feature should use, but a feature then had no way to use it. Resolution gains #chat, #embed and #session, so model resolution and request construction stay in one place. They refuse unless the resolution is ready, raising NotReady with the status so a caller can tell "no server configured" from "this model cannot do that", and refuse a kind mismatch outright -- asking a chat feature to embed is a bug, and should not reach the server to find that out. A comment records that RubyLLM enforces none of a feature's declared requirements: with_schema performs no capability check, and an assumed model is described by Model::Info.default, which claims structured output, vision and function calling for everything. The capability verdicts are the only real gate. StructuredOutput.parse! exists for the same reason -- RubyLLM rescues a JSON parse failure and leaves the content a String, so a feature asking for a schema can silently receive prose. That is likeliest on a self-hosted server, exactly where the structured_output verdict is unknown rather than supported. The embeddings probe moves onto Llm::Session too, so every request that reaches an LLM server goes through one place and the probe inherits the connection's custom headers -- which it never had, so a probe against a gateway that authenticates on its own terms would have been recorded as "unsupported" when it was merely unauthenticated. Llm::Client keeps model discovery and loses everything else. RubyLLM's model parsing discards max_model_len and root, the only trustworthy statement of a self-hosted deployment's context window, and discovery stays on OpenProject.httpx so it remains covered by the SSRF filter RubyLLM's Faraday stack bypasses. https://community.openproject.org/work_packages/66020
Two small changes to shared code, kept separate so they can be reviewed on their own. HealthReports::ResultComponent hard-coded every "More information" link to the file storages troubleshooting page. That was accurate while storages was the only consumer, became wrong when wikis adopted the framework, and would be wrong again for the LLM connection. Both components now take an optional docs_href, defaulting to the storages link so the existing consumers are untouched. Every consumer reads the newest report for a subject, and health_reports carries only a [subject_type, subject_id] index, so that read sorts. It has not mattered because the table only grew when somebody clicked "Run checks". The LLM connection adds a scheduled check and a pruning delete that filters on age, so the index is added now -- concurrently, since the table exists in production. Storages and wikis get the faster read for free. https://community.openproject.org/work_packages/66020
The branch could configure a connection but never verify one. Saving proved the model list was readable, which says nothing about a server that publishes no model list -- a state that became supported when manual model entry landed. Llm::Validators::ConnectionValidator answers the question on the same HealthReports framework storages and wikis use. Five groups: configuration, what is knowable without asking the server; server, the free model-list call where the endpoint offers one; inference, a real completion; models, what the stored catalogue says; and features, whether each registered feature can actually run. The features group is driven by Llm::Runtime, so the health report and the runtime can never disagree about what is usable. It answers "will the AI features work", which is what an administrator actually wants to know. The inference group is separated because it is the only one that costs money. It runs when an administrator asks, gated on a non-persisted deep_health_check accessor -- a property of the run, not of the connection. Two things are deliberately not probed. Tool calling and structured output: several current vLLM releases answer a tool call with plain text on a fully capable server, so a probe would launder a wrong answer into a confident one. And a missing model list is never a failure, only a warning, because a gateway exposing chat alone is a supported deployment. The UI adds the side panel, the report page and the downloadable report, so "Test connection" -- which the ticket asked for and the branch never had -- is now the "Run checks" action, and the answer persists instead of vanishing with a flash. The download is built from non_confidential_configuration: it excludes the API key and the custom headers, since a gateway header routinely carries a second credential, and a request spec asserts both are absent. A spec walks the validator across four server states and asserts every check key and error code resolves; without it a missing key renders as "translation missing" and nothing else would notice. https://community.openproject.org/work_packages/66020
A connection that dies after setup stayed green until somebody happened to look at the page. Llm::HealthCheckJob re-runs the checks every six hours. It deliberately does not set deep_health_check. The inference group spends a real completion, which is billed on a hosted provider, and an unattended job must not run up a bill four times a day. The schedule therefore covers everything free -- an expired key, a withdrawn model, a binding that stopped resolving -- and the billed round trip stays behind "Run checks". A spec asserts no completion is requested. The cron key is enabled and disabled from UpdateService, so it idles rather than waking every six hours to find no connection. Llm::PruneHealthReportsJob is the first pruner health_reports has ever had, which is only now necessary: until this commit the table grew a row per button click. It keeps the 50 newest reports and anything under 90 days, and both conditions matter -- age alone would erase the only check a rarely-touched connection ever had. It is scoped to LLM connections, since storages and wikis share the table. https://community.openproject.org/work_packages/66020
A gateway can report hundreds of models -- OpenRouter returns 341 -- and an administrator had no way to say which of them the organisation actually wants used. The choice cannot live in `active`. The catalogue sync owns that column: it sets it on every model on every refresh and clears it for models the server stopped reporting. A choice stored there would be undone by the next "Refresh models", and the model would be labelled "no longer reported" when it was merely hidden. So deactivated_at is its own column, and a spec runs a sync that still reports a deactivated model to prove the deactivation survives. available_model_ids deliberately does not change. It is what Llm::Runtime resolves against, and hiding a model must never silently break a feature already bound to it -- deactivation curates the pickers, it does not enforce anything. A bound feature keeps working and says so with a banner. Both pickers keep offering the model already selected, since dropping it would blank the field on the next save. A withdrawn model renders a disabled toggle rather than none, so the column stays aligned and the row explains itself. The toggle carries an explicit aria-label: Primer's ToggleSwitch has no accessible name of its own. https://community.openproject.org/work_packages/66020
Two actions the ticket asked for that the branch never had. delete_api_key had existed since the first commit here -- a route, an action and a passing spec -- with nothing anywhere linking to it. The mechanism was built and never rendered, so a key could be replaced but not cleared. Both actions now sit in a kebab menu on the page header. Neither dialog carries a confirmation checkbox, because nothing either does is irreversible. The API key dialog exists for what is not recoverable: the catalogue sync fingerprints base_url and api_key together, so the next refresh after the key changes discards every capability verdict, including the ones an administrator asserted by hand that every other code path preserves. The dialog says so, but only when such verdicts exist. Disconnecting clears the credential and switches the connection off, keeping the endpoint, the catalogue and every binding. A destroying variant was rejected: dependent: :delete_all skips callbacks, so the cascade would take the locked embedding bindings with it -- the only record that a vector index exists and which model and dimension it was written under. Removing the key keeps updating the record directly rather than going through UpdateService, and now says why. The contract probes the server whenever credentials change, and a server that requires authentication rejects the now-keyless probe -- so routing it through the service would let the server refuse an administrator permission to remove a credential. The environment guard the contract did usefully provide is checked explicitly instead. https://community.openproject.org/work_packages/66020
llm_feature_bindings has carried dimensions, input_prefix, query_prefix and locked_at since the table was created, with no way to set any of them. Semantic search needs all four. The prefixes are stored exactly as typed and deliberately not stripped: the trailing space in "passage: " is load-bearing for the E5 and BGE families, and the captions quote the examples so it is visible. Dimensions defaults to blank, because the server decides the vector size and baking in a number it may contradict helps nobody. Where the embeddings probe has already seen a vector, its size is reported in the caption as information rather than filled into the field. The lock now freezes everything the stored index depends on, not just the model. A prefix mismatch is worse than a model mismatch: an index built with one prefix and queried under another does not error, it quietly returns worse results. It also only constrains later edits. Enforcing it on create refused to record a lock at all, since every attribute reads as changed from nil -- a latent bug in the model-only version of this guard that nothing exercised until now. A locked binding renders its values as text rather than disabled inputs. A disabled input submits nothing, so the values would arrive blank and wipe the columns the lock exists to protect. https://community.openproject.org/work_packages/66020
Saving a connection whose model list is absent reported "Connected, but the server did not return a model list". We had not connected. A 404 on /models is indistinguishable from a 404 caused by an endpoint that is simply wrong -- most often a base URL missing its API version segment, since this client appends only the endpoint path. That is not hypothetical: it is exactly what a base URL of https://llm-stack.openproject-edge.eu (no /v1) does. Every request 404s, while the save reports success, because the tolerance that lets a gateway without a model list be configured also swallows the evidence that the URL is wrong. Both messages now name the two possibilities and point at the health check, which sends a real completion and is the only thing that can tell them apart. https://community.openproject.org/work_packages/66020
The identifier field was only rendered when creating a model, on the reasoning that verdicts and bindings reference a model by that string and renaming would orphan them. The reasoning was right and the conclusion was wrong. A typo in a hand-typed identifier could only be fixed by deleting the model and entering it again, which threw away its capability assertions, its feature bindings and its place as a connection default -- a worse outcome than the orphaning it avoided. And hand-typing is exactly where typos come from: models are entered by hand only because the server publishes no list to pick from. Renaming now cascades. Capability verdicts, feature bindings and the connection defaults all follow the new identifier, so a feature bound to the model keeps resolving across the rename. It writes directly rather than through validation, deliberately: a locked binding must not refuse to follow the very model it is locked to, since this changes the name of that model rather than the model in use. Discovered models stay read-only. The server names those, and the next refresh would only put the old name back. https://community.openproject.org/work_packages/66020
A capability field showed "Not specified" in the select with "Currently Supported, from the model registry." underneath it. Both described the same field and they contradicted each other. The blank option now says what actually applies while nothing is asserted here -- "Supported (from the model registry)" -- and the caption is reduced to what choosing something else would mean. The behaviour behind it is unchanged and deliberate: a verdict from a probe or a registry is never loaded into the field, so saving the form cannot turn someone else's finding into the administrator's own assertion. Only the wording was wrong. An administrator's own assertion is loaded into the field, so it is never presented as inherited. https://community.openproject.org/work_packages/66020
The default embedding model picker listed the whole catalogue and annotated what was wrong with each entry. Against a registry-backed provider that meant 132 entries, 3 of which can embed: 122 marked as unable, 7 merely unverified. An unconfirmed capability is not a capability. Offering a model because nothing has ruled it out invites a choice that fails much later, at index time, when the vectors are already being written. Only models known to embed are offered now. That leaves nothing to choose when nothing is known to embed, which is honest rather than a dead end: an administrator who knows better than the registry says so on the model itself, by setting its embeddings capability. The caption says that, rather than leaving an empty select unexplained. The model already chosen is kept listed regardless, so a save cannot silently blank a working configuration. https://community.openproject.org/work_packages/66020
|
All contributors have signed the CLA ✍️ ✅ |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3018c7c423
ℹ️ 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".
…ection-settings # Conflicts: # Gemfile.lock # config/initializers/feature_decisions.rb
|
Caution The provided work package version does not match the core version Details:
Please make sure that:
|
|
Caution The Enterprise plan field is not set on the work package Details:
Please make sure that:
|
The merge of dev brought a refactoring that replaced OpenProject::HttpxSsrfFilter with OpenProject::ServerSideRequestForgeryError. Git merged both sides cleanly because no line conflicted, but the client still rescued the old constant, so every probe raised NameError instead of reporting a blocked host. Caught by the spec suite, not by the merge. https://community.openproject.org/work_packages/66020
Changing the format means talking to a different server, or to the same server in a different dialect, but only base URL and API key counted as connection changes. Switching the format alone therefore neither re-proved the connection nor refreshed the model list, leaving the previous provider's models and defaults active while requests went to the new one. The format is now part of the connection identity: it triggers the probe when switching to an OpenAI-compatible endpoint, triggers a model resync in both directions, and is part of the verdict fingerprint, so capability verdicts from the previous provider are discarded rather than carried over. Found by Codex review on the PR. https://community.openproject.org/work_packages/66020
Two defects in the environment path, both found by Codex review on the PR. A default model configured through the environment failed provisioning on a fresh installation. The seed runs before any model synchronisation, so the catalogue the default was validated against was necessarily empty and the seeder raised. The environment contract now skips that validation; a wrong id surfaces afterwards the same way as a model that vanished from the server. Values removed from the environment stayed in the database. Absent keys were compacted away instead of written as nil, and since the form is read-only while the connection is environment-provisioned, an operator moving to an unauthenticated endpoint had no supported way to clear the stale API key. Absent keys now clear their values: the environment is the source of truth while it is in charge. https://community.openproject.org/work_packages/66020
Renaming a manual model to an identifier already used by this connection raised through save! and produced a 500. The update now follows the create path: on a failed save the edit form is re-rendered with the uniqueness error against the field, and neither the rename cascade nor the capability changes are applied. Found by Codex review on the PR. https://community.openproject.org/work_packages/66020
Three CI follow-ups, none of them behavioural. The llm_* locale keys were inserted out of alphabetical order in two mappings, which yamllint's key-ordering rule rejects. Pure moves, no string changed. The Stimulus controller carried a block-comment copyright header instead of the canonical line-comment form the headers/header-format ESLint rule expects. Note for the next person: rake copyright:update_typescript corrupted the file when converting from the block form, so the header was applied by hand from COPYRIGHT_short. The generated environment variable list now includes the settings this branch introduces, regenerated with the incantation the docs:env_vars task itself prints for an instance with hocuspocus configured. https://community.openproject.org/work_packages/66020
The first Yamllint run only reported the first two misordered mappings, so the previous fix stopped there. This one ran yamllint with the repository configuration over the whole file and sorted every flagged mapping until it came back clean, which also swept the sections later commits had inserted unsorted. Verified as pure reordering: the parsed YAML is identical to the previous commit, and every changed line reappears unchanged elsewhere in the diff. The line-count shrink is de-duplicated blank lines between moved blocks. https://community.openproject.org/work_packages/66020
Collaborator
Author
|
I have read the CLA Document and I hereby sign the CLA |
The side panel section renders its title as h4 by default, and depending on which parts of the page are present that h4 can follow the page header directly, skipping a level. axe flags this as a heading-order violation, which is why the accessibility feature spec failed intermittently: the violation only appears in some page states. The section title is h3 now, which is correct in every state the page can be in. https://community.openproject.org/work_packages/66020
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
…ection-settings # Conflicts: # config/locales/en.yml
|
Warning Flaky specs
🤖 Ask Copilot to investigateCopy the prompt below into a new comment on this PR to delegate the investigation to GitHub Copilot. It will look into the flakiness and open a separate pull request with you as reviewer. |
3 tasks
Collaborator
Author
|
Closing in favour of the stacked split requested in review. The same tree, sliced into eleven single-domain PRs that merge bottom-up:
The branch feature/66020-llm-connection-settings stays as the CI-green reference tree; the stacked result is identical to it apart from folding the two follow-up migrations into the initial ones (the table never shipped) and regenerating the environment variable list against current dev. All review fixes from this PR are contained in the slices. |
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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.
Ticket
WP#66020
What are you trying to accomplish?
As an OpenProject admin, I want to connect OpenProject to an LLM, so that the AI features currently in specification (#69620, #69622, #77781, #77783) have a configured server and model to run against.
This PR adds three pages under Administration, Artificial Intelligence (AI):
Everything is behind the
llm_connectionfeature flag (off in production), so merging changes nothing for existing instances.How the acceptance criteria are covered
https://example.com/v1), because that is what every provider documents. The URL is never rewritten/v1/modelsgets its own message (a server can serve chat but lack a model list), and an SSRF block names the allowlist environment variableBeyond the acceptance criteria
These grew out of the consuming tickets and are the larger part of the diff:
/v1/chat/completionsbut no model list still works: models can be added, edited and deleted by hand and survive refreshes.OPENPROJECT_LLM__CONNECTION_*variables for containerised installs. Seeding never contacts the server, so a container whose LLM sidecar starts late still boots.What approach did you choose and why?
Verification happens inside the contract, following the storages precedent (the Nextcloud credentials validator adds a contract error on 401 and stops the write). The probe only fires when base URL or API key changed; without that guard, every unrelated save would hit the server.
Capabilities are verdicts with a source, not booleans. There is no standard way to ask a server what a model can do (
GET /v1/modelsreturns only id, object, created and owned_by). So each capability issupported,unsupportedorunknown, recorded with where the answer came from: admin assertion beats probe beats registry metadata. Onlyunsupportedblocks a selection. Treatingunknownas a blocker would make most self-hosted servers unusable.Only embeddings is probed behaviourally, because a server either returns a vector or it does not. Tool calling is deliberately not probed: current vLLM releases can return HTTP 200 with the tool call as plain text on a correctly configured server, so a probe would produce confident wrong answers.
Model ids are opaque strings, never foreign keys. The same weights are served under different ids by different deployments, and a binding must survive its model disappearing from the list and say so, rather than break or silently fall back.
Model resolution lives in one place (
Llm::Runtime): per-item override, then feature binding, then connection default. It fails closed; a feature running against a silently substituted model would be a different feature.Decisions reviewers should weigh in on
oauth_clients.client_secretand the Jira token (Redmine::Cipheringis a no-op withoutdatabase_cipher_key). Worth an explicit decision, since LLM keys are often billable.openproject-tokenyet, and the upsell banner raises for unknown keys./v1/modelsverbatim; re-serialising to the strict OpenAI schema stripsmax_model_lenandowned_by, which this PR reads.Merge checklist