<!--IssueSummary start--> <details> <summary> Everyone can contribute. [Help move this issue forward](https://handbook.gitlab.com/handbook/marketing/developer-relations/engineering/community-contributors-workflows/#contributor-links) while earning points, leveling up and collecting rewards. </summary> - [Collaborate/take over this issue](https://contributors.gitlab.com/manage-issue?action=work&projectId=278964&issueIid=616229) </details> <!--IssueSummary end--> ## Problem `WorkItems::AgentPlan#content` is stored externally via `ExternallyStoredField` / a CarrierWave uploader (`WorkItems::AgentPlanContentUploader`). The DB row (`work_item_agent_plans`) holds no content column - only `file_store`, `namespace_id`, `work_item_id`. Determining whether a plan actually has content therefore requires a round-trip to object storage (loading the field, or `uploader.file.exists?`). This was acceptable while a row only existed once content was set. But https://gitlab.com/gitlab-org/gitlab/-/merge_requests/249644 changes that: we now create an `agent_plan` row when AI planning is **enabled** for a work item, before any plan content exists. So "a row exists" no longer implies "content exists" - and the only way to tell them apart today is a costly object-storage fetch. Callers that only need to know *whether* a plan exists (e.g. the AI context block in https://gitlab.com/gitlab-org/gitlab/-/merge_requests/245983) should not pay that cost. ## Why we can't derive this from existing state Before reaching for a new column, we checked whether something already in the DB answers "does content exist?". Nothing does: ### `file_store` doesn't work `file_store` is set unconditionally on every save (`Gitlab::ExternallyStoredField#set_file_store_from_uploader_config`) and is `null: false, default: 1`. It records *where the blob would live* (`1` = LOCAL, `2` = REMOTE), not *whether one exists*. A content-less row (planning enabled, no plan yet) still gets a `file_store` value, so `file_store.present?` is always `true`. Useless as an existence signal. ### CarrierWave's filename column doesn't exist here (by design) Normally CarrierWave's `mount_uploader` persists the uploaded **filename** in a DB column (e.g. `content_file`). A `NULL` filename means "no file" - that column *is* the DB-native existence check, no fetch required. This model deliberately opts out of that. From `lib/gitlab/externally_stored_field.rb`: > Because we avoid using CarrierWave's `mount_uploader` -- it wants to store a filename in our ActiveRecord record, but our filenames are deterministic so that's wasteful -- we have to manually instantiate the uploader. The filename is derived (`AgentPlanContentUploader#filename` -> `"#{work_item_id}.json"`), so the filename column was optimized away. That is exactly the column that would have answered this question for free - by dropping it, the design removed the only DB-native existence signal CarrierWave offers. What's left is `read_external_payload`, which calls `external_storage_uploader.file&.exists?` - a network stat against the Fog/remote backend. That's the round-trip we want to avoid. ### "Row exists ⇒ content exists" no longer holds We could have avoided a new column *if* a row only ever existed when content existed - then `has_content? = persisted?`. But https://gitlab.com/gitlab-org/gitlab/-/merge_requests/249644 breaks exactly that invariant by creating rows when planning is enabled, before content. That's what makes a dedicated flag necessary. **Conclusion:** a new persisted column is the minimal correct fix. Re-introducing CarrierWave's filename column would cost the same and be less idiomatic for this hand-rolled setup, so we add a purpose-built boolean instead. ## Proposal Add a persisted boolean to `work_item_agent_plans` (`has_content`) that tracks content existence, and expose `AgentPlan#has_content?` so callers can decide without touching object storage. - Set the flag `true` when content is written, `false`/reset when content is removed. Keep it in sync with every mutation path (create, update, "remove plan", widget-excluded-in-new-type destroy). - Maintain it in the same AR save as `file_store` so it commits atomically with the row (see `Gitlab::ExternallyStoredField#set_file_store_from_uploader_config` for the existing hook and its documented after-commit crash window). - Expose it via GraphQL on the `agent_plan` widget so external/HTTP consumers can check existence cheaply. ## Implementation details ### The key insight: derive the flag from the in-memory `content` attribute during save `content` is a virtual attribute (`ExternallyStoredField`), but its value **is** available in memory during the AR save cycle - callers assign `agent_plan.content = params[:content]` before saving (see `WorkItems::Callbacks::AgentPlan#handle_agent_plan_change`). So we do not need to read object storage to know whether content is present at write time; we already hold the value. We persist that presence as a boolean column, then reads never touch storage. This is the same pattern `file_store` already uses: computed in a `before_save` hook from in-memory state, committed atomically with the row, and the blob upload happens later in `after_commit`. ### 1. Migration Add a boolean to `work_item_agent_plans`, defaulting to `false` (a freshly created row = planning enabled, no content yet): add_column :work_item_agent_plans, :has_content, :boolean, null: false, default: false Table is new/small, so a plain `add_column` with a default is fine. Update `db/docs/work_item_agent_plans.yml` if needed. ### 2. Model: maintain the column + expose `has_content?` In `ee/app/models/work_items/agent_plan.rb`, set the column from the in-memory content on every save, and let the boolean column back the predicate: before_save :sync_has_content def has_content? has_content end private def sync_has_content self.has_content = content.present? end Notes: - `content.present?` here reads the **in-memory** attribute (already assigned by the caller), not object storage. On a brand-new row where planning was enabled but no content set, `content` is `nil`/blank -> `has_content = false`. Correct. - Because `content` is externally stored, be careful the `before_save` does not trigger a lazy load that hits storage. If `content` was never assigned on a persisted record, accessing it lazily loads from storage. Guard against that: only recompute when the field was actually assigned this cycle, e.g. `return unless attribute_changed?(:content) || new_record?`. Otherwise leave the existing column value untouched. Verify the exact dirty-tracking behaviour against `ExternallyStoredField` (the concern overrides `content=` to force a lazy load so writes register as changes - confirm `attribute_changed?(:content)` behaves as expected here). ### 3. Mutation paths that must stay in sync All content changes flow through the model save, so a single `before_save` covers them - but confirm each: - **Create / update content** - `WorkItems::Callbacks::AgentPlan#handle_agent_plan_change` assigns `content` then saves. `sync_has_content` sets `true`. - **Remove plan** - the callbacks `destroy!` the whole `agent_plan` record (`before_update` when `excluded_in_new_type?`, and `DataSync::Widgets::AgentPlan#post_move_cleanup`). The row is gone, so there is no stale flag. Good - but note: with !249644, if "remove content" ever becomes "keep the row, clear content" rather than destroying the record, `sync_has_content` must flip it back to `false`. Add a spec for the "set content then clear content on an existing row" path to lock this in. - **Data sync / move** - `DataSync::Widgets::AgentPlan#after_save_commit` calls `AgentPlan.create!(content: rewritten)`, which runs the same `before_save`. Fine. ### 4. GraphQL exposure On the `agent_plan` widget type, add a boolean field resolving off the model predicate - never off `content`: field :has_content, GraphQL::Types::Boolean, null: false, description: 'Whether the agent plan has content.' def has_content object.has_content? end Make sure the widget preload (`ee/lib/ee/api/helpers/work_items/preloads.rb` maps `agent_plan: [:agent_plan]`) keeps this N+1-free; the predicate reads a column on the already-loaded association. ### 5. Backfill consideration Existing rows predate the column. Since a row today only exists **because** content was set (pre-!249644 behaviour), a post-deploy backfill setting `has_content = true` for all existing rows is the safe default. Confirm ordering against !249644's rollout - if that MR ships first and starts creating content-less rows, the backfill logic must distinguish them (in that case, backfill from actual content existence, or accept that only rows created before its deploy get `true`). Because of the new `ai_planning_enabled` field... Once could maybe even say we can set the `has_content` flag to `true` when `ai_planning_enabled = false` or rows older than the deploy date of the other MR? ## Acceptance criteria - [ ] `work_item_agent_plans` has a boolean column tracking content existence (migration + `db/docs` update as needed). - [ ] The flag is set/reset correctly across all content mutation paths, verified by specs. - [ ] A model method returns content existence with **no** object-storage round-trip. - [ ] The value is exposed through the `agent_plan` GraphQL widget. - [ ] https://gitlab.com/gitlab-org/gitlab/-/merge_requests/245983 can switch its check from `widget.content.present?` to the new method. ## Context - Model: `ee/app/models/work_items/agent_plan.rb` - Uploader: `ee/app/uploaders/work_items/agent_plan_content_uploader.rb` - External storage concern: `lib/gitlab/externally_stored_field.rb` - Table: `work_item_agent_plans` (see `db/migrate/20260421071247_create_work_item_agent_plans.rb`) - Related MRs: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/245983, https://gitlab.com/gitlab-org/gitlab/-/merge_requests/249644