Keeyan Nejad (c7c8570d) at 16 Mar 17:26
Remove duplicate service account in flow triggers
... and 369 more commits
@bastirehm I tried this a couple times for non-foundational flows and I'm able to create and add flows to a project without any issue:
Thanks @Imjaydip! I asked Duo what differences there are between the 2 and it replied with this:
Can you explain the differences in what's allowed in the configuration between https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/validators/json_schemas/ai_catalog/flow_v2.json and https://gitlab.com/gitlab-org/modelops/applied-ml/code-suggestions/ai-assist/-/blob/main/docs/flow_registry/v1.md
Configuration Differences Overview
The JSON schema (
flow_v2.json) and the markdown documentation (v1.md) describe the same Flow Registry v1 framework, but with notable differences in what they allow and document.Key Differences
1. Environment Values
Schema (flow_v2.json) Documentation (v1.md) Only "ambient"allowed"chat","chat-partial", and"ambient"supportedThe schema is more restrictive, only permitting
ambientenvironments, while the documentation describes three environment types for different interaction levels.2. Top-Level Fields
Field Schema Documentation nameNot allowed Optional descriptionNot allowed Optional product_groupNot allowed Optional promptsOptional Optional (documented in detail) The schema uses
additionalProperties: false, blocking any fields not explicitly defined.3. AgentComponent Features
Feature Schema Documentation response_schema_idNot present Fully documented response_schema_versionNot present Fully documented The documentation extensively covers Response Schema Registry integration for structured agent outputs, but the JSON schema doesn't include these fields.
4. LocalPrompt/Prompts Structure
Field Schema Documentation unit_primitivesRequired Optional (default: [])modelNot present Deprecated (documented) 5. Flow Section
Field Schema Documentation entry_pointOptional with pattern validation Required in examples inputsSupported with FlowInputCategoryNot documented The schema supports a
flow.inputsarray for additional context schemas that isn't covered in the v1.md documentation.Summary
The JSON schema is stricter but incomplete compared to the documentation. It enforces validation rules but lacks support for response schemas and multiple environment types. The documentation is more comprehensive but describes features the schema doesn't validate. This suggests either the schema needs updating to match the documented capabilities, or the documentation describes planned/experimental features not yet in the validation schema.
Looking over that, and the definition in the file, I think what you have here looks good!
Custom flows are a subset of the Flow Registry v1 specification, with certain fields intentionally restricted. Add a new page documenting these restrictions and update custom.md to link to it. Without this, users may encounter validation errors and confuse when creating custom flows, as not all Flow Registry fields and features are supported for custom flows. For example : #587049 (comment 3118337142)
| Before | After |
|---|---|
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Related to #591995
Keeyan Nejad (99cf69d8) at 16 Mar 11:48
Merge branch '586427-ai-catalog-feature-tests-add-tests-for-enablin...
... and 1 more commit
Keeyan Nejad (5fc25c73) at 16 Mar 11:47
Related to #586427
N.A.
bin/rspec ee/spec/features/explore/ai_catalog/ai_catalog_agent_enable_spec.rb
bin/rspec ee/spec/features/explore/ai_catalog/ai_catalog_flow_enable_spec.rb
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Thanks @justin_ho! This looks good to me. I just left a tiny nitpick. I'll go ahead and set it to merge though.
Feel free to apply it if you want, but if it merges as is I don't think it's worth a follow up
EDIT: Ah, the pipeline has already passed, so I can merge directly. I guess no need to start a whole new pipeline just for this. Consider it just an FYI for next time then
Related to #586427
N.A.
bin/rspec ee/spec/features/explore/ai_catalog/ai_catalog_agent_enable_spec.rb
bin/rspec ee/spec/features/explore/ai_catalog/ai_catalog_flow_enable_spec.rb
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Nitpick: Same as the other comment
let_it_be(:public_flow) do
create(:ai_catalog_flow, :with_released_version, project: project, public: true)
end
let_it_be(:private_flow) do
create(:ai_catalog_flow, :with_released_version, project: project, public: false)
endNitpick: We don't really need to specify the name or description, so IMO it's best to leave blank (it also means if we ever change these fields less specs will fail and need updating)
let_it_be(:public_agent) do
create(:ai_catalog_agent, :with_released_version, project: project, public: true)
end
let_it_be(:private_agent) do
create(:ai_catalog_agent, :with_released_version, project: project, public: false)
endThanks for taking a look at this @Imjaydip!
In reply to your first post: Yeah I was imagining anyone who wanted to create and propose a new foundational agent/flow, could create a regular item and try it out until they're happy with it, and if they get approval we can then convert it to a foundational flow. Having a custom flow which can raise an MR for us, will be very handy!
But yeah keeping the columns and default values up to date could be a bit annoying. We can add a spec that confirms that the ITEMS values have the values that exist in the ai_catalog_items table, to catch any issues early on.
have you considered separating the endpoints for foundational and custom items?
Yeah I did consider that. Though I only considered separating them on the BE but preserving the same UI. The issue I had with that is that the FE would then need to manage pagination, and sort order as well as applying all filtering we add in the future (which I assume will come soon) to both endpoints.
I also considered doing most of that work on the backend with my second alternative approach, but a lot of the same issues apply.
I'm wondering if it introduces some long-term maintenance overhead
I agree completely! And I struggled with getting out this draft because of this, since it's a big trade-off and there are things I wanted to do that I couldn't because the 2 types different in a few places.
Ultimately, I decided the trade-off was worth it as it seemed to me to be the best option available.
A step we can take (which can build on-top of this change, rather than instead of) is if we convert the query to just return IDs and foundational item references, instead of instantiating Ai::Catalog::Item records. Then create an array with the 2 different types Ai::Catalog::Item records and Ai::FoundationalChatAgent and a GraphQL endpoint which returns a union (see response from Claude on how this works below). This way we're not coercing the two types into one record class, and it can remain much cleaner.
I'd like a Ruby GraphQL definition for a field that can return an array consisting of 2 different types. Item and FoundationalFlow. These things do not share a relation. Is this possible?
Yes, via a Union type.
module Types class CatalogEntryType < Types::BaseUnion graphql_name 'CatalogEntry' description 'Either a catalog item or a foundational flow' possible_types Types::ItemType, Types::FoundationalFlowType def self.resolve_type(object, _ctx) case object when ::Ai::Catalog::Item Types::ItemType when ::Ai::FoundationalFlow # or whatever your class is Types::FoundationalFlowType else raise "Unknown type: #{object.class}" end end end endThen on the field:
field :catalog_entries, [Types::CatalogEntryType], null: falseCaveat: Union members cannot share fields directly — clients must use inline fragments:
{ catalogEntries { ... on Item { id name } ... on FoundationalFlow { id title } } }If they do share a common interface (e.g. id, name), an Interface type is cleaner — but given you said they share no relation, a Union is the right call.
I'm also curious how you're thinking about the behavioral differences between foundational and custom items
Yeah I think under this model, those are just differences we have to deal with and have conditional logic. Admittedly a bit of a down side to this approach IMO. But hopefully not too painful to deal with.
For how they get enabled, this is something we could consider unifying in the future. Maybe enabling foundational agents in the same way as regular agents is something we'd want? If not then we can at least have the enable button redirect you to the settings.
Given these differences, would it make sense to treat them as distinct resource types with separate endpoints?
I think an ideal UX for backend development would have these as 2 separate pages, but for the product I believe what we are currently working towards (showing foundational items in amongst the rest) is better for the user. Since they don't need to care about the distinction as early on, and can browse everything together. IMO having 2 pages would lead to a less intuitive interface for the users.
Remove duplicate service account in flow triggers
When a flow trigger belongs to an item consumer, do not assign the service account to the flow trigger, as it's already assigned to the consumer.
| Before | After |
|---|---|
Evaluate this MR against the MR acceptance checklist. It helps you analyze changes to reduce risks in quality, performance, reliability, security, and maintainability.
Related to #590238
Keeyan Nejad (fc90feb8) at 13 Mar 18:55
Remove duplicate service account in flow triggers
Seeding can preset a few problems for Cells (which should not have global data) as well as self-managed and dedicated, since we need to replicate the data there as well, and keep the data in sync whenever we make changes.
It would be great, if instead of seeding the data, we used the ActiveRecord::FixedItemsModel::Model like we do for built_in_tool.rb.
That way we don't need to run a job to seed the data on all instances, and it won't cause any Cells incompatibilities.
I'm also working on a spike to use fixed item models for foundational agents, since we've had similar problems there. I think something like that could work well here as well.
Keeyan Nejad (354c93c8) at 12 Mar 18:01
Fix service account duplication between ItemConsumer and FlowTrigger
Resolves #590238
Flow triggers currently store the service account in ai_flow_triggers.user_id, duplicating the same information already stored in ai_catalog_item_consumers.service_account_id. This creates a potential for inconsistency in a critical area.
This MR makes ItemConsumer the single source of truth for the service account by having FlowTrigger derive it from the associated item consumer instead of storing it separately.
Ai::FlowTrigger)
#service_account method that derives the service account from the item consumer (via service_account or parent_item_consumer.service_account) when present, falling back to user for config_path triggersby_service_accounts scope that matches triggers by both direct user_id and parent item consumer service_account_id
user validation: absent when ai_catalog_item_consumer is present, required when absentRunService: Use flow_trigger.service_account instead of flow_trigger.user
PostProcessService: Use by_service_accounts scope and service_account methodIssuableBaseService: Use by_service_accounts scopeItemConsumers::CreateService: Stop setting user in flow_trigger_attributes
FlowTriggers::BaseService: Strip user_id from params when ai_catalog_item_consumer_id is presentSyncFoundationalFlowsService: Create triggers directly via FlowTrigger.create without user_id, check existing triggers by consumer IDFlowTriggerType: user field now resolves via service_account methodDuoStatusType: Use by_service_accounts scope instead of has_many associationuser_id on existing ai_flow_triggers records that have ai_catalog_item_consumer_id setUPDATE ai_flow_triggers
SET user_id = NULL
WHERE ai_catalog_item_consumer_id IS NOT NULL
AND user_id IS NOT NULL