-
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 11 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 |
|---|---|---|
|
|
@@ -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_if_present&.data&.dig("markdown_source").presence || system_prompt | ||
|
Comment on lines
+246
to
+247
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's profile Creative is edited directly, this new method makes 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 Swept for the same class while here and found one more: Regression test: collaborator description follows a directly-edited profile prompt, not the stale column. 53 runs across the touched reader + profile suites, 0 failures.
Comment on lines
+246
to
+247
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.
Now that this method makes 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 already fixed in Now it reads
Comment on lines
+246
to
+247
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 already has a backfilled/synced profile prompt, any non- 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. Real gap, but deferring it rather than adding an auto-sync callback here — with reasoning. The A model-level |
||
| end | ||
|
|
||
| def claude_channel_agent? | ||
| llm_model == "claude-code" | ||
| end | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,38 @@ | ||
| # 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. | ||
| # Use only from write paths (e.g. sync_profile_system_prompt!) — never from | ||
| # a hot read path, since creating a profile creative also creates a Topic. | ||
| def profile_creative | ||
| Collavre::Creative.profile_for(self) | ||
| end | ||
|
|
||
| # Read-only lookup: the existing profile creative or nil, never creating one. | ||
| # Read paths (effective_system_prompt) must use this so a hot-path read never | ||
| # writes, and so rendering never fails when the in-memory user is invalid | ||
| # (e.g. re-rendering the AI form after a failed save leaves name blank). | ||
| def profile_creative_if_present | ||
| return nil unless persisted? | ||
|
|
||
| # order(:id) matches profile_for's write-path selection so a read never | ||
| # diverges from the profile a write targets, even if a race created a dup. | ||
| Collavre::Creative.profiles.where(user: self).order(:id).first | ||
| 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 |
|---|---|---|
|
|
@@ -9,7 +9,13 @@ | |
|
|
||
| <div class="form-group"> | ||
| <%= form.label :system_prompt, t('collavre.users.new_ai.system_prompt_label') %> | ||
| <%= form.text_area :system_prompt, required: true, class: "stacked-form-control", rows: 5 %> | ||
| <%# Pre-fill from the canonical prompt (profile creative's markdown_source), | ||
| not the legacy system_prompt column. If the profile creative was edited | ||
| directly, the column is stale; posting it back through update_ai's sync | ||
| would silently clobber the profile-authored prompt on any unrelated | ||
| setting change. Rendering the effective value makes a no-touch save a | ||
| no-op on the prompt. %> | ||
| <%= form.text_area :system_prompt, value: @user.effective_system_prompt, required: true, class: "stacked-form-control", rows: 5 %> | ||
|
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 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, fixed in Fix: render the submitted value on error, the canonical value otherwise — Regression test ( |
||
| </div> | ||
|
|
||
| <%= render "collavre/users/trigger_field", form: form, routing_expression: @user.routing_expression %> | ||
|
|
||
| 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. |
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.
Fresh evidence beyond the earlier duplicate-profile thread: deterministic
.order(:id).firstonly makes helper-based reads/writes choose the oldest profile, but the normal Creative tree still lists all owned root rows (IndexQuery#handle_root_query) andCreativesController#updateedits the row identified by the request. If a race creates a newer duplicate, an agent can still edit that visible duplicate directly whileeffective_system_promptcontinues reading the oldest one, so prompt edits can still land on a profile that execution ignores; hide/merge duplicates or prevent creation rather than relying only on ordered lookup.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 — real, and fixed at the root in
1d6e5a8e. You're right that.order(:id).firstonly makes the helper read/write paths converge:IndexQuery#handle_root_querystill lists every owned root row andCreativesController#updateedits the row in the request, so a race-created duplicate stayed directly editable whileeffective_system_promptkept reading the oldest.Rather than hide/merge duplicates, this now prevents them: a partial unique index on
creatives(user_id) WHERE data->>'kind' = 'profile', andprofile_forcreates viacreate_or_find_by!so a lost create race resolves to the surviving row.Correcting the record from the earlier duplicate-profile thread: I had declined this exact index claiming
json ->>is STABLE and would be rejected at schema load. That was wrong.json_object_field_text(->>) is IMMUTABLE on Postgres (provolatile = 'i', verified on pg14 and pg16), so the partial predicate is accepted — thepostgres_schema_loadCI job on this commit is the proof, and it needs nojsonbmigration on the currentjsoncolumn. (My earlier reasoning had conflated->>with the genuinely-nonportable SQLitejson_extract.)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_or_find_byresolves a lost race to the surviving profile.