feat: agent/user profile Creatives + system prompt sourced from profile (Subsystem A)#1412
feat: agent/user profile Creatives + system prompt sourced from profile (Subsystem A)#1412sh1nj1 wants to merge 23 commits into
Conversation
…nitized description The system prompt was written into and read from Creative#description, which runs Describable#sanitize_description_html unconditionally on every save — stripping <thinking>/<tool_call> tags and entity-escaping < > &, corrupting every prompt (and the A5 backfill would corrupt all existing agents at once). Store the prompt as Markdown instead: content_type_input="markdown" + markdown_source=<prompt> persists the raw prompt losslessly in data["markdown_source"] while deriving the sanitized HTML into description (the display view). render_system_prompt and sync_profile_system_prompt! now read/write data["markdown_source"], falling back to the legacy column. Adds corruption-guard tests (tag-bearing prompt round-trips verbatim) and an idempotency test. Full model+services suite green (1134 runs, 0 failures).
…wn_source The present?-only sync guard could set markdown_source but never unset it, so blanking a previously-set prompt left the old prompt authoritative (render prefers markdown_source over the now-blank column) — a regression vs the pre-profile column read. When the prompt is cleared, demote the profile out of markdown mode so the stale source is dropped and render falls back to the empty column. Adds a clear-path regression test. Full model+services suite green (1135 runs, 0 failures).
|
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
![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: 11a20c4883
ℹ️ 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".
| # 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. Without this, the old prompt stays authoritative. | ||
| creative.update!(content_type_input: "html") |
There was a problem hiding this comment.
When an agent clears system_prompt after a prompt was synced, this update only deletes data["markdown_source"]; Describable's content_type_input == "html" path leaves the existing description unchanged. Since profile_for creates the profile as an owned root creative and root queries return owned roots, the old rendered prompt remains visible/searchable in the profile Creative even though AiAgentService falls back to the now-blank column. Clear or reseed description in the same update so blanking the prompt removes the stale profile content too.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cb70bc1e: blanking the prompt now reseeds description to the user's name (demoting to html), so the old rendered prompt no longer stays visible/searchable on the profile root. Regression test added.
…opics Non-transactional system tests can create a user whose profile creative (HasProfileCreative) is committed together with a main topic. Rails fixture loading raw-DELETEs creatives (bypassing dependent: :destroy) but never touched the topics table (no fixture), leaving an orphaned topic that tripped check_all_foreign_keys_valid! for every subsequent fixture load -- failing all 42 engine system tests on CI while rake test (transactional) stayed green. Registering an empty topics.yml makes Rails DELETE FROM topics during fixture loading, before the FK check, clearing any leaked orphan. The host test/fixtures dir is a symlink to the engine fixtures, so one file covers both. Reproduced and verified browser-free via check_all_foreign_keys_valid! and FixtureSet.create_fixtures.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 49f7f2452d
ℹ️ 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".
| # `update_metadata` preserves them without core naming vendor-specific keys. | ||
| # --------------------------------------------------------------------------- | ||
| BUILTIN_RESERVED_METADATA_KEYS = %w[markdown_source content_type editor].freeze | ||
| BUILTIN_RESERVED_METADATA_KEYS = %w[markdown_source content_type editor profile skill].freeze |
There was a problem hiding this comment.
When a profile Creative goes through update_metadata with a payload that omits kind, the preservation loop in CreativesController (lines 417-421) will keep top-level keys named profile/skill, not the actual discriminator stored as data["kind"]. Because profile_for finds profiles via data->>'kind' = 'profile', that metadata save can make the existing profile undiscoverable and the next lookup/sync creates a second profile Creative, leaving any prompt/profile content on the orphaned row. Reserve kind (or otherwise protect this discriminator) rather than the discriminator values.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cb70bc1e: the reserved-metadata loop was preserving the dead profile/skill values instead of the actual kind discriminator, so update_metadata dropped data["kind"] and the profile marker was lost (which in turn let profile_for create duplicates). Now reserves kind. Discoverability regression test added.
| # (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.profile_creative&.data&.dig("markdown_source").presence || @agent.system_prompt |
There was a problem hiding this comment.
When the new profile Creative is edited directly (for example by an agent updating its own root Creative), normal task execution now uses the profile markdown_source here, but other agent entry points still read the legacy column, e.g. orchestration matching in Orchestration::Arbiter#extract_expertise_text (lines 208-217) and the completion API prompt builder (lines 58-62). In that scenario the same agent is routed/described or invoked with stale instructions outside AiAgentService, so profile prompt edits only partially take effect; route those consumers through a shared effective-prompt helper or keep the column in sync.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Addressed in cb70bc1e (and completed in e57ba580): every prompt reader now routes through User#effective_system_prompt (canonical data["markdown_source"] first, column fallback) — arbiter, type classifier, typo agent, admin view, and now the orchestration collaborator description. So editing the profile Creative directly takes effect for all consumers, not just AiAgentService.
… through effective_system_prompt, reseed description on clear - reserve data["kind"] (not dead profile/skill values) so profiles scope finds existing profile creatives and profile_for stops duplicating them - add User#effective_system_prompt (markdown_source first, column fallback) and route arbiter, agent_type_classifier, typo_corrector, admin view through it so editing the profile creative isn't shadowed by the stale column - reseed description to name on prompt clear so the old prompt isn't left visible/searchable in the rendered description
|
Addressed all three review findings in
Verified: 54 runs across all touched model+service tests, 0 failures; rubocop clean (9 files). |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: cb70bc1e22
ℹ️ 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".
| 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.
When an agent's profile Creative is edited directly, the legacy system_prompt column remains stale, and the edit form still posts that stale column value for every save. This new sync call then copies the stale system_prompt back into data["markdown_source"], so changing an unrelated setting such as model/API key through update_ai silently erases the profile-authored prompt. Fresh evidence beyond the earlier stale-reader comments: the edit_ai form is still bound to system_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.
Verified in code — this is real. edit_ai bound form.text_area :system_prompt to the legacy column, so directly editing the profile Creative (which the read path, effective_system_prompt, treats as canonical) left the column stale, and any update_ai save re-synced that stale value into data["markdown_source"], clobbering the profile-authored prompt.
Fixed in eeeed34a:
- The form now pre-fills from
effective_system_prompt(canonicalmarkdown_source), so an unrelated setting change re-posts the current prompt as a no-op instead of the stale column. - While verifying I found a related write-on-read:
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!). - Regression tests: form renders the canonical prompt not the stale column;
update_aipreserves a directly-edited prompt when re-posting the effective value.
…de-effect-free Codex P2: editing an agent's profile creative directly left the legacy system_prompt column stale, and the edit_ai form still pre-filled and posted that stale column. update_ai's sync then copied the stale value back into the canonical data["markdown_source"], silently erasing the profile-authored prompt on any unrelated setting change (e.g. model/API key). - edit_ai form now pre-fills from effective_system_prompt (canonical markdown_source), so a no-touch save re-posts the current prompt as a no-op. - effective_system_prompt now uses a find-only profile_creative_if_present, removing a write-on-read: reading the prompt in hot paths (orchestration matching) no longer lazily creates a profile creative (+Topic), and re-rendering the AI form after a failed save no longer crashes when the in-memory name is blank. - Regression tests: form renders the canonical prompt not the stale column; update_ai preserves a directly-edited prompt when re-posting the effective value.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: eeeed34a00
ℹ️ 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".
| 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.
Fresh evidence beyond the prior stale-form case: this controller already has routing_expression-only PATCH coverage, so update_ai supports partial settings updates that omit system_prompt. When a profile Creative was edited directly and the legacy column is stale, any such partial save still calls sync_profile_system_prompt!, which copies the stale column back over data["markdown_source"] and erases the canonical profile prompt; guard this sync on system_prompt being present in the submitted params or sync from the effective profile value.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — real, and fixed as suggested in b5d59bc3. The controller already has routing_expression-only PATCH coverage, and such a partial save left the legacy system_prompt column at its stored (possibly stale) value while sync_profile_system_prompt! still ran, copying it back over the canonical data["markdown_source"].
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 system_prompt (even when cleared → ""), so the clear path is preserved; a partial PATCH that omits it no longer touches the profile prompt. Regression test added: update_ai does not clobber the canonical prompt on a PATCH that omits system_prompt.
| # 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.
When two write paths call profile_for for the same user concurrently, such as the backfill migration running while an agent settings save first syncs that user's profile, both can pass this SELECT before either INSERT commits because there is no database uniqueness constraint for (user_id, data->>'kind' = 'profile'). That leaves duplicate root profile Creatives, and later profile_creative/effective_system_prompt reads an arbitrary .first, so prompt edits can land on one profile while agent execution reads another; add a unique constraint and use create-or-find/retry semantics.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
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 creatives.data column is json (not jsonb), and on Postgres json ->> text is STABLE, not IMMUTABLE. A partial-unique index ON creatives (user_id) WHERE data->>'kind' = 'profile' is rejected by Postgres at schema load (functions in index predicate must be marked IMMUTABLE), so it would red the postgres_schema_load job during the active sqlite→pg cutover. (It passes on SQLite, which is why a local-only green would be a false green.) The same unconstrained SELECT-then-INSERT pattern also predates this PR in inbox_for, so a real constraint should cover inbox+profile uniformly.
Shipped a portable mitigation instead (b5d59bc3): every profile lookup now orders by id, so the write path (profile_for) and the read path (profile_creative_if_present) converge on the oldest profile. A transient race-created duplicate becomes harmless (both edit and execution resolve to the same row) rather than a split brain. Regression test: reads and writes converge on the oldest profile when a duplicate exists.
A hard DB guarantee remains a good follow-up, but it requires migrating data to jsonb (or a stored generated column) first and applying to inbox too — out of scope for this PR, and tracked separately.
…on oldest profile Codex P2 #1 (real): a partial PATCH omitting system_prompt (e.g. a routing_expression-only update) still called sync_profile_system_prompt!, copying the stale legacy column back over a directly-edited canonical data[markdown_source] and erasing it. Gate the sync on the param being submitted (ai_params.key?(:system_prompt)). Codex P2 #2 (real hazard, portable fix): duplicate profile creatives are a split-brain risk. A hard partial-unique index on data->>'kind' is not portable — data is a json column and on Postgres json->> is STABLE, so the index predicate is rejected at schema load (would break postgres_schema_load mid sqlite->pg cutover). Instead order every profile lookup by id so the read path (profile_creative_if_present) and write path (profile_for) converge on the oldest profile, making a transient race-created duplicate harmless rather than a split brain. Regression tests: partial PATCH leaves the canonical prompt untouched; reads and writes converge on the oldest profile when a duplicate exists.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b5d59bc38b
ℹ️ 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".
| def effective_system_prompt | ||
| profile_creative_if_present&.data&.dig("markdown_source").presence || system_prompt |
There was a problem hiding this comment.
When an agent's profile Creative is edited directly, this new method makes data["markdown_source"] the canonical prompt, but the collaboration prompt path still bypasses it: AgentContextBuilder#extract_agent_description (engines/collavre/app/services/collavre/orchestration/agent_context_builder.rb:181) reads user.system_prompt, and AiAgentService appends those descriptions into the collaborator section. In that scenario agents receive stale collaborator expertise even though routing/classification use the edited profile prompt, so the collaboration prompt can describe the wrong role; use effective_system_prompt there as well.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified in code — real, fixed in e57ba580. AgentContextBuilder#extract_agent_description read user.system_prompt directly, so once an agent's profile Creative was edited directly, the collaborator section of other agents' prompts described the stale role while routing/classification already used the canonical prompt. Routed it through effective_system_prompt (canonical data["markdown_source"] first, column fallback).
Swept for the same class while here and found one more: new_ai.html.erb seeded the copy-source pre-fill from the stale column when duplicating an agent — also routed through effective_system_prompt. (show.html.erb and edit_ai were already canonical.)
Regression test: collaborator description follows a directly-edited profile prompt, not the stale column. 53 runs across the touched reader + profile suites, 0 failures.
Codex P2 (real): AgentContextBuilder#extract_agent_description read the legacy system_prompt column directly, so when an agent's profile Creative was edited directly the collaborator section of other agents' prompts described the stale role even though routing/classification used the edited canonical prompt. Route it through effective_system_prompt (canonical data[markdown_source] first, column fallback). Also fixed the same class in the new_ai copy-source pre-fill, which seeded a duplicated agent's form from the stale column. Regression test: collaborator description follows a directly-edited profile prompt (canonical markdown_source), not the stale column.
Deterministic order(:id) lookups only made the helper read/write paths converge on the oldest profile; the generic Creative tree (IndexQuery#handle_root_query) still lists a race-created duplicate and CreativesController#update lets an agent edit it directly, while effective_system_prompt keeps reading the oldest — so prompt edits could still land on a profile that execution ignores. Prevent the duplicate from ever existing instead of only converging on it. - Add a partial unique index on creatives(user_id) WHERE data->>'kind' = 'profile'. This is portable on the current `data` json column: json_object_field_text (`->>`) is IMMUTABLE on Postgres (verified pg14 and pg16), so the predicate is accepted at schema load; SQLite enforces it too. This corrects an earlier assumption that `->>` was STABLE and the index therefore impossible. - profile_for now creates via create_or_find_by!, resolving a lost create race to the surviving row instead of inserting a duplicate. The read-first fast path keeps the common already-exists case cheap. - The migration collapses any pre-index duplicate onto the oldest profile before adding the index. Regression tests: the DB rejects a second profile creative for the same user; create_or_find_by resolves a lost race to the surviving profile.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 1d6e5a8edd
ℹ️ 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".
| .where(user_id: uid) | ||
| .order(:id) | ||
| .pluck(:id) | ||
| Collavre::Creative.where(id: ids.drop(1)).delete_all |
There was a problem hiding this comment.
When duplicate profile rows already exist — the case this method is meant to collapse — each duplicate Creative has a Main topic from Creative's after_create :create_main_topic, and topics.creative_id is protected by a non-cascading foreign key. Calling delete_all here bypasses has_many :topics, dependent: :destroy and closure-tree cleanup, so the DELETE is rejected before the unique index is added (or leaves orphaned hierarchy rows where FKs are off); destroy the duplicate creatives through the model or remove dependent rows first.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — real, fixed in 591e5b55. collapse_duplicate_profiles! used delete_all, which bypasses has_many :topics, dependent: :destroy. Every Creative gets a Main topic (after_create :create_main_topic) behind the non-cascading topics.creative_id FK, so on Postgres the DELETE is rejected and on SQLite (FKs often off) it orphans the topic rows — exactly the case this method exists to clean up.
Now it destroys through the model (duplicates.drop(1).each(&:destroy!)), so the Main topic (and its own dependents) go with each duplicate. Race duplicates are freshly-created-and-unused, so the cascade is cheap. Regression test (collapse_duplicate_profiles removes duplicates and their Main topics) drops the unique index, creates a real duplicate with its Main topic, runs the collapse, and asserts the duplicate's topic does not orphan — it errors under delete_all, passes under destroy!.
…s are removed The State A migration's collapse_duplicate_profiles! used delete_all, which bypasses has_many :topics, dependent: :destroy. Every Creative gets a Main topic (after_create :create_main_topic) behind a non-cascading topics.creative_id FK, so delete_all is rejected on Postgres and orphans topic rows on SQLite. Destroy through the model instead; race duplicates are fresh so the cascade is cheap. Regression test asserts a collapsed duplicate's Main topic does not orphan.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 591e5b550b
ℹ️ 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".
| # to the LLM, so an inline data-URI image must NOT be rewritten into a | ||
| # blob path (which would mutate the prompt the agent receives). | ||
| creative.skip_data_uri_rewrite = true | ||
| creative.update!(content_type_input: "markdown", markdown_source: system_prompt) |
There was a problem hiding this comment.
When an agent prompt contains a fenced example or instruction block with extend ToolMeta, this profile sync saves it as a normal Creative description, so Creative's existing saved_change_to_description? callback enqueues UpdateMcpToolsJob; McpService#update_from_creative then registers any code block containing that marker as an MCP tool and creates an approval comment. During the prompt backfill, AI-settings sync, or direct profile edit, prompt examples can therefore create/replace real tools even though the profile is just the agent's system prompt; exclude data["kind"] == "profile" creatives from MCP extraction/enqueueing.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified in code — real, fixed in b2359a5a. A profile creative's description is the agent's system prompt, but Creative's after_save :mark_mcp_tools_sync_pending, if: :saved_change_to_description? fed every description change into UpdateMcpToolsJob → McpService#update_from_creative, which registers any <pre>/<code> block containing extend ToolMeta as a real, approval-gated MCP tool. So a fenced tool-definition example in a prompt would create/replace tools on backfill, AI-settings sync, or a direct profile edit.
Gated at the model (the right altitude, matching the profile data-URI fix): added Creative#profile? (data->>'kind' == 'profile') and mark_mcp_tools_sync_pending now returns early for profiles, so no job is ever enqueued. Defense-in-depth: McpService#update_from_creative also returns early when creative.effective_origin.profile?, so a direct service/job call can't register a tool either. Non-profile creatives still extract tools unchanged.
Regression tests: (1) update_from_creative skips a profile creative whose description contains extend ToolMeta (no McpTool created); (2) saving a profile description does not enqueue UpdateMcpToolsJob; (3) end-to-end through sync_profile_system_prompt! and a direct profile_creative.update! — both leave no tool registered. Verified RED without the guards (the prompt example was registered as prompt_example_tool), GREEN with them.
There was a problem hiding this comment.
Verified — real, fixed in b2359a5a. A profile Creative's description is the agent's rendered system prompt, and Creative's saved_change_to_description? callback fed every description change into McpService, so backfill, AI-settings sync, or a direct profile edit could register/replace an approval-gated MCP tool from a fenced extend ToolMeta example in the prompt.
Fixed at two layers:
- Model gate: added
Creative#profile?(data["kind"] == "profile") and skip theUpdateMcpToolsJobenqueue for profile creatives on description change. - Defense-in-depth: early return in
McpService#update_from_creativefor profile creatives, so even a manually-enqueued job is a no-op.
Regression tests cover the service guard, the enqueue gate, and end-to-end sync/direct-edit paths.
A profile creative's description is the agent's system prompt, so a fenced `extend ToolMeta` example in the prompt must never register a real, approval-gated MCP tool. Creative's saved_change_to_description callback fed every description change into McpService, so backfill, AI-settings sync, or a direct profile edit could create/replace tools from prompt text. Gate at the model (add Creative#profile?, skip the enqueue) with a defense-in-depth early return in McpService#update_from_creative. Regression tests cover the service guard, the enqueue gate, and end-to-end sync/direct-edit.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: b2359a5a46
ℹ️ 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".
|
|
||
| add_index :creatives, :user_id, | ||
| unique: true, | ||
| where: "data->>'kind' = 'profile'", |
There was a problem hiding this comment.
In deployments that rely on db/schema.rb for PostgreSQL schema loads, this partial unique index still embeds the JSON expression data->>'kind' in the index predicate. The source-of-truth migration guidance in docs/engine_development.md explicitly says “Do NOT add a JSON-expression index” and instead to promote the JSON key to a real column because adapter-specific serialization can make db:schema:load crash; since this predicate is dumped into db/schema.rb, it reintroduces that unsupported cross-adapter path. Promote the profile discriminator to an indexed column, following the existing IndexedJsonColumns pattern, before enforcing uniqueness.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — real, fixed in 6a1bb73a. You're right: the partial unique index used a JSON-expression predicate (data->>'kind' = 'profile'), and since db/schema.rb is dumped from the SQLite dev DB, a dev running db:migrate locally would regenerate it as json_extract(data, '$.kind'), which is not a PostgreSQL function → db:schema:load crashes on prod. The postgres_schema_load CI job only passed because the committed schema.rb happened to carry the PG (->>) form; the SQLite-dump path was the real, documented hazard (docs/engine_development.md), and my earlier "IMMUTABLE on Postgres" reasoning validated only one adapter.
Fixed by following the prescribed Collavre::IndexedJsonColumns pattern (same as 20260702000005_promote_channel_config_index_columns): promote the discriminator into a real creatives.kind column, kept synced from data on every save, and index that plain column (where: "kind = 'profile'"). data stays the source of truth.
Verified both adapters:
- schema.rb dumped from SQLite now carries
where: "kind = 'profile'"(notjson_extract) — the crash vector is gone. db:schema:loadinto PostgreSQL succeeds; the partial unique index is created (WHERE kind::text = 'profile'::text) and enforces (a secondkind='profile'row for the same user is rejected with a duplicate-key error).
Regression test asserts the promoted kind column stays synced with data["kind"] on save (a stale column would defeat the uniqueness guarantee).
…xpression The partial unique index enforcing one profile creative per user used a JSON-expression predicate (data->>'kind' = 'profile'). schema.rb is dumped from the SQLite dev DB, where that predicate serializes to json_extract(data, '$.kind') — not a PostgreSQL function — so db:schema:load crashes on production. docs/engine_development.md forbids JSON-expression indexes and prescribes promoting the key to a real column. Promote the kind discriminator into a real creatives.kind column, kept in sync from data by Collavre::IndexedJsonColumns, and index that plain column (where: kind = 'profile'), matching the reference 20260702000005_promote_channel_config_index_columns pattern. The index now dumps identically on both adapters. Verified: schema.rb dumped from SQLite carries 'where: kind = ...' (not json_extract); db:schema:load into PostgreSQL succeeds and the partial unique index enforces (second profile row rejected). Regression test asserts the kind column stays synced with data on save.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6a1bb73a7b
ℹ️ 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".
|
|
||
| def up | ||
| Collavre::User.find_each(batch_size: 200) do |user| | ||
| Collavre::Creative.profile_for(user) |
There was a problem hiding this comment.
On an upgrade from main, this migration runs at 20260721000000, but the creatives.kind column is not added until 20260721000010; the current Collavre::Creative model already has the IndexedJsonColumns before_save callback that writes self["kind"] from data. So each profile_for insert here tries to write an unknown column, gets rescued/logged, and the migration succeeds without creating profiles; the following prompt backfill has the same ordering problem. Move the promoted-column migration before these backfills or use a migration-local model that doesn't touch kind until the column exists.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — real P1, fixed in 95c573b0. Confirmed the mechanism: IndexedJsonColumns installs a before_save that writes self["kind"] = data["kind"] on every Creative save, and the profile backfills (20260721000000/000001) save through the full model — but kind was only added by 20260721000010. On an upgrade from main the backfills run first, each profile_for save raises ActiveModel::MissingAttributeError, the migration's rescue swallows it, and the migration "succeeds" creating zero profiles.
Fixed by moving the column promotion into a new migration 20260720235959_add_kind_column_to_creatives that runs before the backfills; 20260721000010 now only collapses duplicates and adds the unique index. The new migration guards add_column (unless column_exists?, idempotent for machines that applied an earlier revision of this PR) and calls reset_column_information so an already-loaded Creative model picks up the column.
schema.rb is unchanged — the column and index are already present and the max version is still 20260721000010, so postgres_schema_load is unaffected. That's also exactly why CI never caught this: it loads schema.rb and never replays migration order.
Validation (replayed the sequence on a scratch SQLite DB): from-zero migrate runs clean (no MissingAttributeError); with the kind column present — the post-fix backfill state — profile_for creates the profile (0 → 1) with kind populated ("profile", matching data["kind"]) and markdown_source preserved verbatim (incl. <thinking>). Regression test the kind column is added before any migration that backfills profiles locks the ordering invariant so a future re-merge/renumber fails loudly.
…iles IndexedJsonColumns installs a before_save that re-derives every promoted column from `data` on every Creative save (`self["kind"] = data["kind"]`). The profile backfills (20260721000000/000001) save Creatives through the full model, but the `kind` column was only added by 20260721000010 — so on an upgrade from main the backfills ran before the column existed. Each save raised ActiveModel::MissingAttributeError, the migration's rescue swallowed it, and the migration "succeeded" while creating no profiles at all. Move the column promotion into a new migration (20260720235959) that runs before the backfills, and drop the add_column/populate from 20260721000010 (now only collapses duplicates and adds the unique index). The new migration guards add_column (idempotent for machines that ran an earlier revision) and resets column information so any already-loaded Creative model sees the column. schema.rb is unchanged (column + index already present; max version still 20260721000010). Verified by replaying the migration sequence on a scratch DB: profile_for creates the profile with kind populated and the prompt preserved verbatim. Regression test locks the ordering invariant (CI can't — it loads schema.rb and never replays migration order).
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 95c573b0c4
ℹ️ 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".
| # docs/engine_development.md and 20260721000010_add_unique_index_to_profile_creatives). | ||
| # `data` stays the source of truth; `kind` is re-derived from it on every save. | ||
| include Collavre::IndexedJsonColumns | ||
| indexed_json_columns json: :data, columns: { kind: "kind" } |
There was a problem hiding this comment.
When this code is deployed to a database that still has pre-20260720235959 migrations pending, this callback is installed on the current Creative model before the creatives.kind column has been added. Older migrations that run before that point still save Creative records — for example engines/collavre/db/migrate/20251230113607_refactor_labels.rb calls Creative.create! — so sync_indexed_json_columns will try to write self["kind"] against a schema where the column does not exist and abort the migration. Guard the sync when the promoted column is absent, or keep those older migrations on a migration-local model that does not include the new callback.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — real, fixed in 947f073d. Confirmed the mechanism: IndexedJsonColumns#sync_indexed_json_columns (installed as a before_save on Creative via indexed_json_columns json: :data, columns: { kind: "kind" }) wrote self["kind"] unconditionally. On a full migration replay, 20251230113607_refactor_labels (Dec 2025) runs long before 20260720235959_add_kind_column_to_creatives and calls Creative.create!, so the sync hit MissingAttributeError against a table without the kind column and aborted the migration.
Rather than pin old migrations to a migration-local model (fragile — every future record-saving migration would have to remember), the guard moves into the concern itself: next unless has_attribute?(column). The JSON attribute stays the source of truth, so a promoted column that has not been added yet is simply skipped and re-derived on the next save once it exists. This makes every IndexedJsonColumns user (Creative kind, Channel repo_full_name/pr_number/worktree_id) robust to mid-replay saves, not just this one migration.
Regression test (skips promoted columns absent from the table instead of raising) points the column map at a not-yet-added column and asserts the save does not raise while still syncing the existing promoted columns — it errors without the guard, passes with it. Full IndexedJsonColumnsTest 5 runs / creative_profile 10 / user_profile_system_prompt 11, 0 failures; rubocop clean.
A full migration replay reaches older record-saving migrations (e.g. 20251230113607_refactor_labels calls Creative.create!) before the 20260720235959 migration that promotes the kind column. The IndexedJsonColumns before_save wrote self[column] unconditionally, so those saves aborted with MissingAttributeError. Guard the sync with has_attribute? so the JSON attribute stays the source of truth and the promoted column is re-derived once it exists.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 947f073d0d
ℹ️ 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".
| 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.
When update_ai fails validation (for example, a blank name), the controller re-renders this form with @user.system_prompt still holding the user's submitted prompt, but the profile Creative is not synced because the save failed. This explicit value: @user.effective_system_prompt reads the previous profile markdown_source instead, so the error page silently discards the unsaved prompt edit and shows the old canonical prompt; use the submitted value when @user.errors.any? or limit the effective-prompt override to the initial GET.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified — real, fixed in 4a5f1017. On an update_ai validation failure (e.g. blank name), @user.update has already assigned the submitted system_prompt in memory, but the success-only sync_profile_system_prompt! never ran, so the profile creative still holds the old markdown_source. The field hardcoded value: @user.effective_system_prompt, which reads that stale canonical prompt on the error re-render and silently discarded the user's unsaved edit.
Fix: render the submitted value on error, the canonical value otherwise — value: (@user.errors.any? ? @user.system_prompt : @user.effective_system_prompt). The initial GET still pre-fills from the canonical profile prompt (unchanged); only the error re-render now shows what the user typed.
Regression test (update_ai preserves the submitted prompt edit on a validation error): sets the profile markdown_source to OLD CANONICAL PROMPT, PATCHes with a blank name + system_prompt: 'MY EDITED PROMPT', asserts the re-rendered textarea shows MY EDITED PROMPT and not OLD CANONICAL PROMPT — RED under the old hardcoded value, GREEN with the guard. Full users_controller_ai_test 13 runs, 0 failures; rubocop clean.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4a5f101742
ℹ️ 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".
| profiles.create_or_find_by!(user: user) do |creative| | ||
| creative.description = user.name.to_s | ||
| creative.data = { "kind" => PROFILE_KIND } | ||
| creative.progress = 0.0 |
There was a problem hiding this comment.
When this runs for an AI agent created by another user, the profile Creative is owned by the agent account itself, but the Creative permission path only grants access to the owner or to users with CreativeShare cache entries, and create_ai only shares the agent to an optional target Creative rather than sharing this new profile back to the creator. In that common managed-agent case, the human who created the agent cannot read or write the profile Creative through the normal Creative UI/API, so the directly edited profile prompt flow added by this commit is effectively unreachable unless a separate share is created.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Verified the mechanism — accurate: profile_for owns the profile Creative as the agent (user_id = agent.id), create_ai only shares the optional target creative to the agent (share_ai_agent_to_creative), never the profile back to the creator, and the Creative permission path is owner-or-CreativeShare. So through the Creative UI/API the creator indeed has no access to the agent's profile Creative.
But "the directly edited profile prompt flow ... is effectively unreachable" overstates it for this PR:
- The prompt is fully creator-editable today via the AI form (
edit_ai/update_ai), authorized bycreated_by_id == Current.user.id. The form pre-fills from the canonical prompt (effective_system_prompt→ profiledata["markdown_source"]) and writes back throughsync_profile_system_prompt!. That's a complete read+write surface for the prompt — it is not unreachable. - There is no direct-Creative-UI entry point to an agent's profile Creative in this PR (no route/view opens it). The direct-edit capability is only a model-layer read preference (
effective_system_promptprefersmarkdown_source), plumbed so a future UI's direct edits take effect everywhere — it is not a user-facing flow this commit exposes. The profile is a root Creative owned by the agent, so it never appears half-visible-but-uneditable in the creator's own tree either.
The genuine forward-looking need you've identified — the creator needs access to the agent's profile Creative — is foundational to the two card requirements this PR intentionally does not yet implement: skills created/linked under the profile Creative (B), and the settings UI that surfaces the profile for direct editing (C). Granting the share, and deciding its permission level (admin vs write) and whether the skills subtree inherits it, is a coherent piece of that work rather than a data/functional defect in this A-only PR — and doing it here would prematurely commit those semantics while touching the share-cache invalidation path.
Concrete fix location for when B/C lands: in create_ai, after sync_profile_system_prompt!, mirror share_ai_agent_to_creative to share @user.profile_creative back to Current.user (the creator), so the creator lands as an owner/admin of the agent's profile subtree.
Summary
Foundation for Agent / User settings as Creatives (Subsystem A of the "Agent / User 설정 Creative 연동및 단순화" spec). Every user — human and AI agent — now gets an auto-created profile Creative, and an agent's system prompt is sourced from that profile Creative instead of a bare column.
This generalizes the existing
HasInboxCreativepattern (which skipped agents viaunless: ai_user?) into a symmetric persona model, and moves the system prompt onto the Creative tree so it can participate in permissions, sharing, and (later) skills.What changed
Creative.profile_for(user)+profile/skillreserved metadata kinds — a per-user root Creative (data.kind = "profile"), owned by the user themselves.HasProfileCreativeconcern — auto-creates the profile Creative for every user including agents (noai_user?guard), with idempotent self-heal mirroringHasInboxCreative.20260721000000— profile Creatives for existing users.20260721000001— mirror existingsystem_promptinto the profile.data["markdown_source"]:User#sync_profile_system_prompt!): setscontent_type_input: "markdown", markdown_source: system_promptso Describable stores the raw prompt indata["markdown_source"];descriptionbecomes the derived rendered view. Called on create, update, and backfill.AiAgentService#render_system_prompt):profile_creative.data["markdown_source"].presence || system_prompt(legacy column fallback during transition).content_type_input: "html"so the stalemarkdown_sourceis dropped and the empty column governs (restores origin/main behavior).Why
data["markdown_source"], notdescriptionCreative#descriptionrunssanitize_description_htmlunconditionally — it's a rendered display field. Storing a prompt there corrupts it:<thinking>…</thinking>→…,</>/&→ entities,<tool_call>stripped. Verified empirically thatdata["markdown_source"]round-trips prompt-shaped text (tags,<,&, multibyte) losslessly (203==203 chars).descriptionremains the derived view; the prompt is canonical inmarkdown_source.Tests
<thinking>,<tool_call>,<,&survives unchanged).Scope / follow-ups
data["profile"]) deferred — no consumer yet; will land with the settings UI to avoid dead accessors.system_promptcolumn kept as fallback; column drop is a separate PR after backfill soaks.