Part 6: Sequential Patch Edits (description_edits) & Whole-Document Quick-Action Defense in MCP save_work_item

2026-11-11来源 dev.toEnglish

Series Context: Part 6 of 10 in GitLab Core Engineering: Architecture, MCP & CI/CD Deep Dives.

Featured MR: gitlab-org/gitlab!264862 (Merged into master) · Work Item: #635910

Why Sending Full Markdown Descriptions Over MCP Does Not Scale

When an AI agent fixes a typo or updates a single status checkbox in a GitLab issue or epic containing tables and implementation checklists, forcing it to resend the entire description string via save_work_item creates two real problems:

  1. Token waste & truncation risk: Large GitLab issues run thousands of tokens. Re-generating the whole body risks subtly rewriting unrelated paragraphs.
  2. Concurrent edit clobbering: Full-body overwrites silently erase edits another engineer made to a different section moments earlier.

In !264862, we added an update-only description_edits array to save_work_item:

{ "project_id": "gitlab-org/gitlab", "work_item_iid": 635910, "description_edits": [ { "old_str": "- [ ] Verify CI", "new_str": "- [x] Verify CI" }, { "old_str": "Status: In review", "new_str": "Status: Merged" } ] }

Implementing surgical text patching inside a Markdown engine that executes /quick_actions turns out to have fascinating Ruby and security subtleties.

Strict Uniqueness, Ordering & Ruby's sub Trap

Look at apply_single_description_edit inside app/services/mcp/tools/work_items/update_work_item_tool.rb:

def apply_single_description_edit(content, edit) edit_hash = edit.is_a?(Hash) ? edit.with_indifferent_access : {} unless edit_hash.key?(:old_str) && edit_hash.key?(:new_str) raise ArgumentError, 'Provide both old_str and new_str for each edit in description_edits' end old_str = edit_hash[:old_str].to_s new_str = edit_hash[:new_str].to_s raise ArgumentError, 'old_str cannot be empty in description_edits' if old_str.empty? occurrences = content.scan(old_str).length if occurrences == 0 raise ArgumentError, "old_str was not found in description: #{old_str.truncate(60).inspect}" elsif occurrences > 1 msg = [ "old_str is ambiguous in description (#{occurrences} matches found);", "provide more surrounding context: #{old_str.truncate(60).inspect}" ].join(' ') raise ArgumentError, msg end content.sub(old_str) { new_str } end

Three design choices here protect callers and maintainers alike:

  1. Sequential reduce pipeline: 
 new_description = edits.reduce(current_description) do |content, edit| apply_single_description_edit(content, edit) end

Edits apply left-to-right against the output of the previous edit. If edit #2 touches text updated by edit #1, it works deterministically—and if edit #1 introduces ambiguity, edit #2 fails loudly before mutating anything.

  1. Exact-once match enforcement (occurrences == 1): If old_str appears twice (occurrences > 1), guessing first vs all silently corrupts documents. Failing with "provide more surrounding context" gives an AI agent the exact hint needed to expand old_str on the next call. Also notice content.scan(old_str) treats a String old_str literally (no regex interpretation!).
  2. Ruby's sub(old_str) { new_str } block form: If you write content.sub(old_str, new_str), Ruby interprets \&, \1, \+, and \\ inside new_str as regex backreferences! Writing content.sub(old_str) { new_str } passes new_str verbatim, even when new_str contains shell backslashes or code snippets.

We also guarded the boundaries cleanly:

  • Rejected description_edits inside CreateWorkItemTool::UPDATE_ONLY_PARAMS (you cannot patch a record being created).
  • Enforced mutual exclusivity with description: "Provide either description or description_edits, not both".
  • Re-checked total character length against Concerns::Constants::MARKDOWN_TEXT_MAX_LENGTH (1_048_576) after all sequential expansions complete.

Security Deep Dive: Why Quick-Action Validation Must Diff the Whole Document

GitLab MCP tools forbid injecting slash quick actions (/close, /spend, /award, /label) inside descriptions so prompt-injected issues cannot silently execute commands.

If we had naively checked validate_no_quick_actions!(edit[:new_str]) on each replacement snippet alone, two injection attacks would have bypassed it completely:

  1. Deletion smuggling: Existing description: 
 Please don't /close this issue

Attacker sends { "old_str": "Please don't ", "new_str": "" }.

Neither old_str nor new_str starts with /! Yet after deletion, the line begins with /close this issue—executing a quick action!

  1. Newline smuggling: Same existing line: "Please don't /close this issue". Attacker sends { "old_str": "don't ", "new_str": "don't\n" }. Still no / anywhere in new_str! Yet injecting a newline pushes /close to column 0 of the next line.

Meanwhile, legitimate issues often already contain lines starting with / (like /etc/gitlab.rb), so blindly rejecting any new_description containing a leading / would freeze future unrelated edits to the issue!

Look at how validate_description_edits_quick_actions! threads that needle cleanly:

def validate_description_edits_quick_actions!(current_description, new_description) original_actions = current_description.lines.grep(Concerns::ContentValidation::QUICK_ACTION_PATTERN).map(&:strip) new_actions = new_description.lines.grep(Concerns::ContentValidation::QUICK_ACTION_PATTERN).map(&:strip) return if (new_actions - original_actions).empty? raise ArgumentError, 'Quick actions (commands starting with /) are not allowed in description' end

By comparing the multiset of leading-slash lines after all edits against the original document, both deletion smuggling and newline smuggling fail immediately, while untouched lines like /etc/gitlab.rb remain editable.

Bonus Cleanup: Eliminating Duplicate Work Item Queries Across UrlParser & BaseTool

Notice apply_description_edits needed the WorkItem record (to read item.description), whereas UpdateWorkItemTool previously only called resolve_work_item_id (returning a Global ID string gid://gitlab/WorkItem/123).

Instead of running a second query (::WorkItem.find_by_id), we refactored UrlParser and BaseTool:

  • Extracted UrlParser#find_work_item_from_url(url) returning the WorkItem model directly, and had resolve_work_item_from_url(url) delegate .to_global_id.to_s.
  • Renamed BaseTool#resolve_work_item_from_params -> find_work_item_from_params, memoized @resolved_work_item in BaseTool#resolve_work_item, and had resolve_work_item_id return resolve_work_item.to_global_id.to_s.
  • Verified with expect(::WorkItem).not_to receive(:find_by_id) in base_tool_spec.rb that resolving both the description and Global ID performs zero extra database lookups.

Takeaways for GitLab Engineers

  1. Always use block form String#sub(target) { replacement } when applying user- or LLM-supplied patches—non-block sub(a, b) corrupts \& and \1 sequences.
  2. Validate line-anchored invariants (like /quick_actions) against the post-patch document diff, never isolated edit chunks. Deletions and \n insertions can shift existing substrings into column 0.
  3. Share model resolution cleanly: Return the ActiveRecord instance once (resolve_work_item) and derive .to_global_id.to_s from it rather than issuing duplicate DB queries.

Series Navigation

原文：https://www.a8q9.cn/en/article/part-6-sequential-patch-edits-description-edits-whole-document-quick-action-defe-164556.html

相关文章