[AI-3] Add the LLM connection record and discovery transport by tangopium · Pull Request #24879 · opf/openproject

tangopium · 2026-08-22T14:54:58Z

Ticket

What are you trying to accomplish?

This is PR 1 of 11: the stacked split of the original implementation PR #24851, which was too large to review in one piece. Each PR in the stack covers one domain; nothing is user-visible until the llm_connection feature flag is flipped in a later PR.

This part lays the foundation: the llm_connections table and LlmConnection record (singleton by validation, schema ready for more), the ruby_llm gem with the provider-headers patch, Llm::Session as the one wrapper around it, Llm::Client and the error hierarchy, and the adapter layer that either queries an OpenAI-compatible server's /v1/models live or reads a registry-backed provider's catalogue. The API key is ciphered at rest and the feature flag plus its environment documentation entry are added here because the flag defines a setting.

Merge checklist

Added/updated tests
Added/updated documentation (env var list regenerated)
Feature flag llm_connection keeps everything off in production

github-actions · 2026-08-22T14:55:26Z

Caution

The provided work package version does not match the core version

Details:

Work package URL: https://community.openproject.org/work_packages/66020
Work package version: not set
Core version: 17.9.0

Please make sure that:

The work package version OR your pull request target branch is correct

github-actions · 2026-08-22T14:55:27Z

Caution

The Enterprise plan field is not set on the work package

Details:

Work package URL: https://community.openproject.org/work_packages/66020

Please make sure that:

The work package Enterprise plan field is set

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 5ee9d96d32

ℹ️ 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".

github-actions · 2026-08-22T15:30:59Z

Warning

Flaky specs

rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24879, linked for reference only):

- `rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]`

Treat this as a standalone task, unrelated to PR #24879. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24879 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @tangopium to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @tangopium, and request a review from @tangopium.
On every commit, set @tangopium as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

The persistence and transport layer for connecting OpenProject to an LLM provider, with no user interface yet. Everything sits behind the new llm_connection feature flag, which is off in production, so this changes nothing for existing instances until the flag is flipped. LlmConnection stores the endpoint: API format, base URL, API key and the designated default models. The key is ciphered through Redmine::Ciphering, matching LdapAuthSource; note this is a no-op unless database_cipher_key is configured. Only one connection is supported, enforced by a validation rather than the schema, so lifting the restriction later needs no migration. Discovery and inference speak through two seams. Llm::Adapters resolves the api_format to a discovery strategy: OpenAI-compatible endpoints are queried live, so the list is what that endpoint actually serves, while the formats RubyLLM knows discover from its registry, which is a published catalogue rather than a live query. Llm::Session wraps RubyLLM for inference with per-connection isolated configuration, and Llm::Errors is the one error taxonomy every caller maps from -- response bodies never surface in messages, because gateways echo credentials and internal hostnames in error payloads. Llm::Client remains the plain-HTTP path for OpenAI-compatible servers. Two of its lessons are worth keeping: transport failures are HTTPX::ErrorResponse instances while a 4xx also populates #error, so the response class is the discriminator; and bodies are parsed directly rather than through #json, which insists on a content type self-hosted proxies do not reliably set. Part 1 of the AI-3 stack. https://community.openproject.org/work_packages/66020

github-actions · 2026-08-22T16:39:48Z

Warning

Flaky specs

rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]
rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]

🤖 Ask Copilot to investigate

Copy 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.

@copilot The following spec(s) are flaky in CI (first seen on PR #24879, linked for reference only):

- `rspec ./modules/gantt/spec/features/timeline/timeline_dates_spec.rb[1:2:1]`
- `rspec ./spec/features/work_packages/table/switch_types_spec.rb[1:1:1]`

Treat this as a standalone task, unrelated to PR #24879. Create a new branch from origin/dev and open a new pull request targeting dev — do not stack it on PR #24879 or reuse that branch.

Follow the playbook in docs/development/testing/handling-flaky-tests/README.md to find the root cause and fix the underlying race — do not skip, delete, or weaken the spec to make it pass; disabling is a last resort per the playbook, and only with a bug ticket. Verify the fix by running the spec(s) repeatedly (e.g. `script/bulk_run_rspec --run-count 10`).

If you cannot reproduce the flake or are not confident in a fix after reasonable investigation, do not fabricate a change or skip the spec to force CI green. Instead, leave the pull request in draft and document what you tried, the suspected cause, and any leads in its description, then assign @tangopium to take over.

Once the fix is verified, title the PR after the spec(s) it fixes, and use the PR description to explain the root cause, how the change resolves it, and the before/after results. Label the PR `flaky-spec`, assign @tangopium, and request a review from @tangopium.
On every commit, set @tangopium as the sole co-author with a `Co-authored-by:` trailer (use their GitHub no-reply email so it links to their account), so it is traceable who dispatched the fix.

thykel

@tangopium This is coming up pretty well! Here's my first pass.

NB: Splitting up that massive PR definitely helped readability, but the PR stack is a bit of a Jenga tower. There are lots of base assumptions laid out in this first PR, and I want to challenge some of them, which will inevitably affect some other parts of the stack.

On the other hand, I get that this is also down to a personal preference, and LLM tools nowadays make it easier to "refresh" the rest of the stack after a structural change. And sometimes, seeing the code prototype right away can give a better idea compared to a vague diagram.

Still, for next time, I'd rather discuss the high-level technical design upfront (even with a simple ASCII diagram). This way, we can quickly identify any structural clashes that would have otherwise required burning more tokens just to rewrite a bunch of PRs.

thykel · 2026-08-25T20:31:57Z

+        end
+      end
+
+      def embeddings(model_id:, input:)


Looks like a dead method.

thykel · 2026-08-25T20:54:08Z

+# every association is already scoped by +llm_connection_id+ and the STI +type+
+# column is in place.
+class LlmConnection < ApplicationRecord
+  include Redmine::Ciphering


🤔 We don't usually do encryption at rest and the only other code paths that use this method are leftovers from Redmine. So, this doesn't look like something we want to lean into.

I'm gonna do a bit more skulking and then get back to you.

@tangopium Let's drop the attribute encryption altogether please -- the current consensus is that we do not support it, for various good reasons.

thykel · 2026-08-25T20:54:56Z

+# transport and as a source of published model capabilities; what a given
+# connection actually offers is tracked per connection, never in RubyLLM's
+# application-wide registry.
+gem "ruby_llm", "~> 1.16"


This seems to drag in a deprecation warning.

!!! RubyLLM's legacy acts_as API is deprecated and will be removed in RubyLLM 2.0.0. Please consult the migration guide at https://rubyllm.com/upgrading-to-1-7/

Please make sure it's cleaned up -- looks like it just needs a little configuration tweak.

thykel · 2026-08-25T21:14:10Z

+
+  SINGLETON_NAME = "default"
+
+  has_many :health_reports, as: :subject, dependent: :delete_all


@NobodysNightmare How do we feel about this one? Is that HealthReport framework reusable for arbitrary needs (such as LLM connection tracking)?

Uh totally. I am happy if we start using it in more places, though it's good to be aware that more consumers exist, because it's a "naturally grown abstraction", not one that was designed before it was implemented.

I didn't look into this usage here, but if there is:

a health check/self test that's triggered by an admin

and the health check tests whether the connection to an external service is working the way we expect it to work

and maybe some of the tests can only be performed if others succeeded

then it sounds like a good fit to me.

thykel · 2026-08-25T21:30:54Z

+class CreateLlmConnections < ActiveRecord::Migration[8.1]
+  def change
+    create_table :llm_connections do |t|
+      t.string :name, null: false, index: { unique: true }


"name" doesn't come off very restrictive, and I wouldn't have expected this field to serve as the de-facto ID column.

We already have an arguably better convention around here:

Suggested change

t.string :name, null: false, index: { unique: true }

t.string :identifier, null: false, index: { unique: true }

thykel · 2026-08-25T21:41:02Z

+# Only a single connection is supported today. That is enforced by a validation
+# rather than by the schema, so lifting the restriction later is a one-line change:
+# every association is already scoped by +llm_connection_id+ and the STI +type+
+# column is in place.


Do we already have plans to support multiple connections? If they are definitely coming, we might want to consider dropping that singleton approach, and just relying on a simple active flag in the database, capped by a single occurrence by a validation + index (for now).

thykel · 2026-08-25T21:49:20Z

+    def available?
+      OpenProject::FeatureDecisions.llm_connection_active? &&
+        enabled? &&
+        instance.configured?
+    end


The enabled field currently seems misplaced, especially if we consider extending this table with more connections later.

The OpenProject::FeatureDecisions toggles (reachable at admin/settings/experimental) are the pre-release "feature flags" -- we use such flag for a feature that's currently in development. Once it's finalized and ready to roll out to everyone, we generally just drop it (or force it to active).

The intent behind that enabled flag ("toggle all AI features") should probably live in the Setting framework instead -- you already introduce it in a later PR, but it's actually worth doing that at the beginning.

Then, I would suggest enabled to be reused as an "availability" flag for the LLM connection, once/if we remove that singleton restraint. Hence, for now, this would simply be true by default at all times.

thykel · 2026-08-25T21:57:57Z

+  def change
+    create_table :llm_connections do |t|
+      t.string :name, null: false, index: { unique: true }
+      t.string :type, null: false, index: true


I'm not sure why we keep this in and needlessly (?) activate STI. It seems good to drop from my POV.

thykel · 2026-08-25T22:01:21Z

+      # Model references are strings, never foreign keys: a selection must survive
+      # the model disappearing from the remote catalogue.


Why do we care about the remote catalogue, if we actually mirror models locally starting from #24882? It seems these might as well be proper references. (Created either by a later separate migration, or by shuffling the stack so that the table already gets introduced now.)

thykel · 2026-09-06T00:23:35Z

+    # RubyLLM providers whose model lists come from its registry.
+    OPENAI_COMPATIBLE = "openai"
+
+    FORMATS = %w[


So this is currently more or less a mirror of a similar listing in RubyLLM::Provider.providers.keys.

I guess it makes sense to have it here to be more explicit. But in that case, I think we need a way to detect any drift after gem updates. Something like:

it "offers only providers the gem still ships" do expect(Llm::Adapters::FORMATS).to all(be_in(RubyLLM::Provider.providers.keys.map(&:to_s))) end

tangopium mentioned this pull request Aug 22, 2026

[AI-3] Add the LLM connection administration page #24880

Open

2 tasks

tangopium mentioned this pull request Aug 22, 2026

[#66020] Add LLM connection settings to admin AI configuration #24851

Closed

3 tasks

chatgpt-codex-connector Bot reviewed Aug 22, 2026

View reviewed changes

Comment thread app/services/llm/errors.rb Outdated

tangopium force-pushed the feature/66020-pr01-connection-record branch from 5ee9d96 to 4d9b1c8 Compare August 22, 2026 15:05

tangopium changed the title ~~[#66020] Add the LLM connection record and discovery transport~~ [AI-3] Add the LLM connection record and discovery transport Aug 22, 2026

tangopium force-pushed the feature/66020-pr01-connection-record branch from 4d9b1c8 to da7c284 Compare August 22, 2026 16:16

tangopium self-assigned this Aug 22, 2026

tangopium requested a review from thykel August 22, 2026 21:39

thykel reviewed Aug 25, 2026

View reviewed changes

thykel reviewed Sep 6, 2026

View reviewed changes


		SINGLETON_NAME = "default"

		has_many :health_reports, as: :subject, dependent: :delete_all

	t.string :name, null: false, index: { unique: true }
	t.string :identifier, null: false, index: { unique: true }

		# Model references are strings, never foreign keys: a selection must survive
		# the model disappearing from the remote catalogue.

Uh oh!

Conversation

tangopium commented Aug 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Ticket

What are you trying to accomplish?

Merge checklist

Uh oh!

github-actions Bot commented Aug 22, 2026

Uh oh!

github-actions Bot commented Aug 22, 2026

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

Uh oh!

github-actions Bot commented Aug 22, 2026

Uh oh!

github-actions Bot commented Aug 22, 2026

Uh oh!

thykel left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thykel Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thykel Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thykel Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

thykel Aug 25, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Milestone

Development

Uh oh!

3 participants

tangopium commented Aug 22, 2026 •

edited

Loading

thykel Aug 25, 2026 •

edited

Loading

thykel Aug 25, 2026 •

edited

Loading

thykel Aug 25, 2026 •

edited

Loading

thykel Aug 25, 2026 •

edited

Loading