-
Notifications
You must be signed in to change notification settings - Fork 1
feat: agent/user profile Creatives + system prompt sourced from profile (Subsystem A) #1412
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from 9 commits
dec0da3
742ece3
afb6a7a
f63559c
ac8bff8
9051f15
11a20c4
49f7f24
cb70bc1
eeeed34
b5d59bc
e57ba58
ea5f94b
85619a0
0b20884
f1ecee5
1d6e5a8
591e5b5
b2359a5
6a1bb73
95c573b
947f073
4a5f101
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -43,6 +43,7 @@ def create_ai | |
| @user.agent_conf = params[:agent_conf] if @user.respond_to?(:agent_conf=) && params[:agent_conf].present? | ||
|
|
||
| if @user.save | ||
| @user.sync_profile_system_prompt! | ||
| Collavre::Contact.ensure(user: Current.user, contact_user: @user) | ||
| share_ai_agent_to_creative(@user, params[:creative_id]) | ||
| redirect_to user_path(Current.user, tab: "contacts"), notice: I18n.t("collavre.users.create_ai.success") | ||
|
|
@@ -61,6 +62,7 @@ def update_ai | |
| ai_params = params.require(:user).permit(:name, :system_prompt, :llm_vendor, :llm_model, :llm_api_key, :gateway_url, :searchable, :routing_expression, :agent_conf, tools: []) | ||
|
|
||
| if @user.update(ai_params) | ||
| @user.sync_profile_system_prompt! | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence beyond the prior stale-form case: this controller already has routing_expression-only PATCH coverage, so Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified — real, and fixed as suggested in Now the sync is gated on the param actually being submitted: @user.sync_profile_system_prompt! if ai_params.key?(:system_prompt)A full form submit still includes |
||
| redirect_to edit_ai_user_path(@user), notice: I18n.t("collavre.users.update_ai.success") | ||
| else | ||
| @available_tools = load_available_tools | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,11 @@ class Creative < ApplicationRecord | |
| # --------------------------------------------------------------------------- | ||
| # Reserved metadata key registry — engines register their own namespaces so | ||
| # `update_metadata` preserves them without core naming vendor-specific keys. | ||
| # "kind" is the discriminator (`inbox`/`profile`/`skill`) that scopes like | ||
| # `profiles` query on (`data->>'kind'`); it must be preserved or a metadata | ||
| # save that omits it makes the row undiscoverable and a duplicate is created. | ||
| # --------------------------------------------------------------------------- | ||
| BUILTIN_RESERVED_METADATA_KEYS = %w[markdown_source content_type editor].freeze | ||
| BUILTIN_RESERVED_METADATA_KEYS = %w[markdown_source content_type editor kind].freeze | ||
|
|
||
| class << self | ||
| def registered_reserved_metadata_keys | ||
|
|
@@ -69,6 +72,12 @@ def to_partial_path | |
| # --- Inbox --- | ||
| scope :inboxes, -> { where("data->>'kind' = 'inbox'") } | ||
|
|
||
| # --- Profile --- | ||
| PROFILE_KIND = "profile" | ||
| SKILL_KIND = "skill" | ||
|
|
||
| scope :profiles, -> { where("data->>'kind' = ?", PROFILE_KIND) } | ||
|
|
||
| SYSTEM_TOPIC_NAME = "System" | ||
| MAIN_TOPIC_NAME = "Main" | ||
| CONTENT_TOPIC_NAME = "Content" | ||
|
|
@@ -135,6 +144,20 @@ def self.inbox_for(user) | |
| ) | ||
| end | ||
|
|
||
| # Find or create the profile creative for a given user. | ||
| # Places it as a root creative (no parent) owned by the user. | ||
| def self.profile_for(user) | ||
| existing = profiles.where(user: user).first | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When two write paths call Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The split-brain hazard is real, but I did not add the suggested DB unique constraint — it isn't portable on this schema and would break CI mid-cutover. The Shipped a portable mitigation instead ( A hard DB guarantee remains a good follow-up, but it requires migrating |
||
| return existing if existing | ||
|
Comment on lines
+169
to
+171
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Fresh evidence beyond the earlier duplicate-profile thread: deterministic Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified — real, and fixed at the root in Rather than hide/merge duplicates, this now prevents them: a partial unique index on Correcting the record from the earlier duplicate-profile thread: I had declined this exact index claiming Migration collapses any pre-index duplicate onto the oldest profile first. Regression tests: the DB rejects a second profile creative for the same user; |
||
|
|
||
| create!( | ||
| description: user.name.to_s, | ||
| data: { "kind" => PROFILE_KIND }, | ||
| user: user, | ||
| progress: 0.0 | ||
| ) | ||
| end | ||
|
|
||
| attr_accessor :filtered_progress | ||
|
|
||
| belongs_to :user, class_name: Collavre.configuration.user_class_name, optional: true | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,6 +3,7 @@ class User < ApplicationRecord | |
| self.table_name = "users" | ||
|
|
||
| include HasInboxCreative | ||
| include HasProfileCreative | ||
|
|
||
| has_many :user_themes, class_name: "Collavre::UserTheme", dependent: :destroy | ||
|
|
||
|
|
@@ -207,6 +208,41 @@ def ai_user? | |
| llm_vendor.present? | ||
| end | ||
|
|
||
| # Mirror the agent's system prompt into its profile creative as Markdown, | ||
| # so the raw prompt lands losslessly in data["markdown_source"] — the | ||
| # canonical home for the prompt. Writing the prompt straight into | ||
| # `description` would run it through the HTML sanitizer (Describable), which | ||
| # strips <thinking>/<tool_call> tags and entity-escapes < > &, corrupting | ||
| # every prompt; the derived `description` is only a rendered display view. | ||
| # The system_prompt column is legacy, pending removal. | ||
| # No-op for humans and for prompt-less agents (profile keeps its name seed). | ||
| def sync_profile_system_prompt! | ||
| return unless ai_user? | ||
| creative = profile_creative | ||
| if system_prompt.present? | ||
| return if creative.data&.dig("markdown_source") == system_prompt | ||
| creative.update!(content_type_input: "markdown", markdown_source: system_prompt) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an agent prompt contains Markdown data-URI image syntax, such as an instruction/example Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified in code — real, fixed in Fix keeps Regression test: a prompt with an inline data-URI image round-trips through There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When an agent prompt contains a fenced example or instruction block with Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified in code — real, fixed in Gated at the model (the right altitude, matching the profile data-URI fix): added Regression tests: (1)
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified — real, fixed in Fixed at two layers:
Regression tests cover the service guard, the enqueue gate, and end-to-end sync/direct-edit paths. |
||
| elsif creative.data&.dig("markdown_source").present? | ||
| # The prompt was cleared. Demote out of markdown mode so the stale | ||
| # markdown_source is dropped and render falls back to the (now-blank) | ||
| # column — matching the pre-profile behavior where blanking the prompt | ||
| # took effect immediately. Also reseed `description` back to the name | ||
| # seed so the old rendered prompt doesn't stay visible/searchable on the | ||
| # owned profile root. Without this, the old prompt stays authoritative. | ||
| creative.update!(content_type_input: "html", description: name.to_s) | ||
| end | ||
| end | ||
|
|
||
| # The effective system prompt for all read consumers. The canonical prompt | ||
| # lives in the profile creative's data["markdown_source"]; the legacy | ||
| # `system_prompt` column is the fallback for rows not yet backfilled. Every | ||
| # prompt reader (orchestration matching, type classification, typo agent, | ||
| # admin view) must route here so that editing the profile creative directly | ||
| # takes effect everywhere, not just in AiAgentService. | ||
| def effective_system_prompt | ||
| profile_creative&.data&.dig("markdown_source").presence || system_prompt | ||
| end | ||
|
|
||
| def claude_channel_agent? | ||
| llm_model == "claude-code" | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,24 @@ | ||
| # frozen_string_literal: true | ||
|
|
||
| module Collavre | ||
| module HasProfileCreative | ||
| extend ActiveSupport::Concern | ||
|
|
||
| included do | ||
| after_create_commit :create_profile_creative | ||
| end | ||
|
|
||
| # Returns the user's profile creative, creating one if it doesn't exist. | ||
| def profile_creative | ||
| Collavre::Creative.profile_for(self) | ||
| end | ||
|
|
||
| private | ||
|
|
||
| def create_profile_creative | ||
| Collavre::Creative.profile_for(self) | ||
| rescue StandardError => e | ||
| Rails.logger.error("[HasProfileCreative] Failed to create profile for user #{id}: #{e.message}") | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -136,8 +136,13 @@ def prepare_rendering_context | |
| end | ||
|
|
||
| def render_system_prompt(rendering_context) | ||
| # The prompt lives losslessly in the profile creative's markdown_source | ||
| # (data["markdown_source"]); `description` is the sanitized rendered view | ||
| # and would corrupt tags/angle-brackets, so never read it here. Fall back | ||
| # to the legacy system_prompt column for rows not yet backfilled. | ||
| template = @agent.effective_system_prompt | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
When a stateful agent has already replied in a topic, Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified in code — real, fixed in Now the check also includes the profile creative's profile_updated_at = @agent.profile_creative_if_present&.updated_at
prompt_changed = profile_updated_at.present? && profile_updated_at > last_reply_at
creative_changed || agent_changed || prompt_changed
|
||
| rendered = AiSystemPromptRenderer.new( | ||
| template: @agent.system_prompt, | ||
| template: template, | ||
| context: rendering_context | ||
| ).render | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| class BackfillProfileCreatives < ActiveRecord::Migration[8.1] | ||
| disable_ddl_transaction! | ||
|
|
||
| def up | ||
| Collavre::User.find_each(batch_size: 200) do |user| | ||
| Collavre::Creative.profile_for(user) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
On an upgrade from Useful? React with 👍 / 👎.
Owner
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Verified — real P1, fixed in Fixed by moving the column promotion into a new migration
Validation (replayed the sequence on a scratch SQLite DB): from-zero migrate runs clean (no |
||
| rescue => e | ||
| Rails.logger.error("[BackfillProfileCreatives] user #{user.id}: #{e.message}") | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| # No-op: profiles are idempotent and safe to keep. | ||
| end | ||
| end | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,15 @@ | ||
| class BackfillProfileDescriptions < ActiveRecord::Migration[8.1] | ||
| disable_ddl_transaction! | ||
|
|
||
| def up | ||
| Collavre::User.find_each(batch_size: 200) do |user| | ||
| user.sync_profile_system_prompt! | ||
| rescue => e | ||
| Rails.logger.error("[BackfillProfileDescriptions] user #{user.id}: #{e.message}") | ||
| end | ||
| end | ||
|
|
||
| def down | ||
| # No-op: descriptions are safe to keep. | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,10 @@ | ||
| _fixture: | ||
| model_class: Collavre::Topic | ||
|
|
||
| # Intentionally empty. Registering the topics table as fixture-managed makes | ||
| # Rails DELETE FROM topics during fixture loading, before the whole-DB | ||
| # foreign-key check runs. Non-transactional (system) tests can create a user | ||
| # whose profile creative (Collavre::HasProfileCreative) is committed together | ||
| # with a main topic; the raw fixture DELETE of creatives bypasses | ||
| # dependent: :destroy and would otherwise leave that topic orphaned, tripping | ||
| # check_all_foreign_keys_valid! for every subsequent fixture load. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| # engines/collavre/test/models/collavre/creative_profile_test.rb | ||
| require "test_helper" | ||
|
|
||
| module Collavre | ||
| class CreativeProfileTest < ActiveSupport::TestCase | ||
| setup { @user = Collavre::User.create!(name: "Ann", email: "ann@example.com", password: "password123") } | ||
|
|
||
| test "profile_for creates a profile creative owned by the user" do | ||
| profile = Collavre::Creative.profile_for(@user) | ||
| assert_equal "profile", profile.data["kind"] | ||
| assert_equal @user.id, profile.user_id | ||
| end | ||
|
|
||
| test "profile_for is idempotent" do | ||
| first = Collavre::Creative.profile_for(@user) | ||
| assert_no_difference -> { Collavre::Creative.profiles.where(user: @user).count } do | ||
| assert_equal first.id, Collavre::Creative.profile_for(@user).id | ||
| end | ||
| end | ||
|
|
||
| test "kind discriminator is a reserved metadata key" do | ||
| # `kind` scopes profile/inbox/skill discovery; it must survive a metadata | ||
| # save that omits it, or the profile becomes undiscoverable and duplicates. | ||
| assert_includes Collavre::Creative.reserved_metadata_keys, "kind" | ||
| end | ||
|
|
||
| test "preserving reserved keys keeps a profile discoverable after a metadata save" do | ||
| profile = Collavre::Creative.profile_for(@user) | ||
| # Simulate update_metadata's preservation loop with a payload omitting kind. | ||
| incoming = { "theme" => "dark" } | ||
| Collavre::Creative.reserved_metadata_keys.each do |key| | ||
| if profile.data.key?(key) | ||
| incoming[key] = profile.data[key] | ||
| else | ||
| incoming.delete(key) | ||
| end | ||
| end | ||
| profile.update!(data: incoming) | ||
| assert_equal profile.id, Collavre::Creative.profile_for(@user).id | ||
| assert_equal "profile", profile.reload.data["kind"] | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,16 @@ | ||
| require "test_helper" | ||
|
|
||
| module Collavre | ||
| class UserProfileCreativeTest < ActiveSupport::TestCase | ||
| test "human user gets a profile creative on create" do | ||
| u = Collavre::User.create!(name: "Human", email: "h@example.com", password: "password123") | ||
| assert_equal "profile", u.profile_creative.data["kind"] | ||
| end | ||
|
|
||
| test "AI agent also gets a profile creative on create" do | ||
| a = Collavre::User.create!(name: "Bot", email: "bot@example.com", password: "password123", llm_vendor: "google") | ||
| assert a.ai_user? | ||
| assert_equal "profile", a.profile_creative.data["kind"] | ||
| end | ||
| end | ||
| end |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,87 @@ | ||
| require "test_helper" | ||
|
|
||
| module Collavre | ||
| class UserProfileSystemPromptTest < ActiveSupport::TestCase | ||
| test "sync mirrors the agent system prompt into the profile markdown_source" do | ||
| agent = Collavre::User.create!(name: "NamedBot", email: "syncbot1@ai.local", | ||
| password: "password123", llm_vendor: "google", | ||
| system_prompt: "REAL PROMPT") | ||
| # after_create seeds the profile description with the name — the bug we fix. | ||
| assert_equal "NamedBot", agent.profile_creative.description | ||
| agent.sync_profile_system_prompt! | ||
| assert_equal "REAL PROMPT", agent.profile_creative.reload.data["markdown_source"] | ||
| end | ||
|
|
||
| test "sync stores prompts with tags and angle brackets losslessly" do | ||
| # Prompts routinely contain <thinking>, tool tags, and < > &. These MUST | ||
| # survive verbatim in markdown_source; the sanitized `description` would | ||
| # strip/entity-escape them, which is exactly why the prompt is NOT stored | ||
| # in description. | ||
| prompt = "Wrap reasoning in <thinking>...</thinking>. " \ | ||
| "When count < 5 and result > 3 emit <tool_call>x</tool_call>. Use JSON & XML." | ||
| agent = Collavre::User.create!(name: "TagBot", email: "tagbot@ai.local", | ||
| password: "password123", llm_vendor: "google", | ||
| system_prompt: prompt) | ||
| agent.sync_profile_system_prompt! | ||
| assert_equal prompt, agent.profile_creative.reload.data["markdown_source"] | ||
| end | ||
|
|
||
| test "sync is idempotent and does not re-write an unchanged prompt" do | ||
| agent = Collavre::User.create!(name: "IdemBot", email: "idembot@ai.local", | ||
| password: "password123", llm_vendor: "google", | ||
| system_prompt: "STABLE PROMPT") | ||
| agent.sync_profile_system_prompt! | ||
| before = agent.profile_creative.reload.updated_at | ||
| agent.sync_profile_system_prompt! | ||
| assert_equal before, agent.profile_creative.reload.updated_at | ||
| end | ||
|
|
||
| test "clearing the system prompt drops the stale markdown_source" do | ||
| agent = Collavre::User.create!(name: "ClearBot", email: "clearbot@ai.local", | ||
| password: "password123", llm_vendor: "google", | ||
| system_prompt: "FIRST PROMPT") | ||
| agent.sync_profile_system_prompt! | ||
| assert_equal "FIRST PROMPT", agent.profile_creative.reload.data["markdown_source"] | ||
|
|
||
| # Admin blanks the prompt textarea — the old prompt must NOT stay authoritative. | ||
| agent.update!(system_prompt: "") | ||
| agent.sync_profile_system_prompt! | ||
| assert_nil agent.profile_creative.reload.data&.dig("markdown_source") | ||
|
|
||
| # The old rendered prompt must not linger as the visible profile description; | ||
| # it is reseeded back to the name so it isn't searchable/shown on the root. | ||
| assert_equal "ClearBot", agent.profile_creative.reload.description | ||
|
|
||
| svc = Collavre::AiAgentService.allocate | ||
| svc.instance_variable_set(:@agent, agent) | ||
| svc.instance_variable_set(:@creative, nil) | ||
| refute_includes svc.send(:render_system_prompt, {}), "FIRST PROMPT" | ||
| end | ||
|
|
||
| test "effective_system_prompt prefers profile markdown_source over the legacy column" do | ||
| agent = Collavre::User.create!(name: "EffBot", email: "effbot@ai.local", | ||
| password: "password123", llm_vendor: "google", | ||
| system_prompt: "LEGACY COLUMN") | ||
| # Before sync the column is the only source. | ||
| assert_equal "LEGACY COLUMN", agent.effective_system_prompt | ||
| # A direct profile edit (no column write) must take effect for all readers. | ||
| agent.profile_creative.update!(content_type_input: "markdown", markdown_source: "EDITED IN PROFILE") | ||
| assert_equal "EDITED IN PROFILE", agent.reload.effective_system_prompt | ||
| end | ||
|
|
||
| test "sync is a no-op for human users" do | ||
| human = Collavre::User.create!(name: "Human", email: "human1@example.com", password: "password123") | ||
| human.sync_profile_system_prompt! | ||
| assert_nil human.profile_creative.data&.dig("markdown_source") | ||
| assert_equal "Human", human.profile_creative.description | ||
| end | ||
|
|
||
| test "sync is a no-op when the agent has no system prompt" do | ||
| agent = Collavre::User.create!(name: "Blanky", email: "syncbot2@ai.local", | ||
| password: "password123", llm_vendor: "google") | ||
| agent.sync_profile_system_prompt! | ||
| assert_nil agent.profile_creative.data&.dig("markdown_source") | ||
| assert_equal "Blanky", agent.profile_creative.description | ||
| end | ||
| end | ||
| end |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When an agent's profile Creative is edited directly, the legacy
system_promptcolumn remains stale, and the edit form still posts that stale column value for every save. This new sync call then copies the stalesystem_promptback intodata["markdown_source"], so changing an unrelated setting such as model/API key throughupdate_aisilently erases the profile-authored prompt. Fresh evidence beyond the earlier stale-reader comments: theedit_aiform is still bound tosystem_prompt, and this added sync writes that posted value back to the canonical profile prompt.Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified in code — this is real.
edit_aiboundform.text_area :system_promptto the legacy column, so directly editing the profile Creative (which the read path,effective_system_prompt, treats as canonical) left the column stale, and anyupdate_aisave re-synced that stale value intodata["markdown_source"], clobbering the profile-authored prompt.Fixed in
eeeed34a:effective_system_prompt(canonicalmarkdown_source), so an unrelated setting change re-posts the current prompt as a no-op instead of the stale column.effective_system_promptcalledprofile_creative(find-or-create), so a hot-path prompt read lazily created a profile Creative (+Topic), and re-rendering the form after a failed save crashed oncreate!with a blank in-memory name. Switched the read path to a find-onlyprofile_creative_if_present; the create variant stays only on the write path (sync_profile_system_prompt!).update_aipreserves a directly-edited prompt when re-posting the effective value.