Skip to content

feat: add append param to update_drawer#1762

Open
fkberthold wants to merge 3 commits into
MemPalace:developfrom
fkberthold:loom-n2b9
Open

feat: add append param to update_drawer#1762
fkberthold wants to merge 3 commits into
MemPalace:developfrom
fkberthold:loom-n2b9

Conversation

@fkberthold

Copy link
Copy Markdown

Summary

Adds an append: bool = False parameter to tool_update_drawer (and its MCP
schema entry). When append=True and content is provided, the content is
concatenated onto the existing drawer body instead of replacing it, so a
caller updating a long drawer can send only the delta rather than
re-transmitting the whole body on every edit.

append defaults to False — existing callers and the replace-in-full
behavior are unchanged. The handler already fetches the existing document, so
this is a minimal read-modify-write with no extra round-trip:

new_doc = old_doc + sanitized if append else sanitized

Motivation and the alternatives considered (a separate append_to_drawer
tool, or a richer section-patch) are in the linked issue — this PR implements
the smallest form and is happy to follow a different surface if preferred.

Closes #1761

Changes

  • mempalace/mcp_server.py
    • tool_update_drawer(...) gains append: bool = False; content path now
      appends-or-replaces based on the flag.
    • WAL log records the append flag.
    • mempalace_update_drawer MCP schema gains an append boolean property;
      tool description notes the delta-append behavior.
  • tests/test_mcp_server.py
    • test_update_drawer_append — appends a delta and asserts the original body
      survives and the delta is concatenated (old_doc + sanitize_content(delta)).
    • test_update_drawer_replace_is_default — pins that the default
      (append=False) still replaces, guarding the back-compat path.

Verification

Test-first (RED → GREEN; visible in the commit order on this branch):

  • RED commit adds test_update_drawer_append, which fails against the current
    code with TypeError: tool_update_drawer() got an unexpected keyword argument 'append'.
  • GREEN commit adds the parameter; the test passes.
$ pytest tests/ -q
889 passed, 106 deselected, 1 warning in ~56s     # 1 warning is a pre-existing,
                                                   # unrelated fact_checker runpy warning
$ ruff check mempalace/mcp_server.py
All checks passed!

CONTRIBUTING.md adherence

  • Conventional commits (test: for the failing test, feat: for the param).
  • ruff clean on the changed file (100-char limit).
  • Branched from and targeted at develop; pushed to a personal fork.
  • Tests run without API keys or network access.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review

This pull request introduces an optional append parameter to the tool_update_drawer function, allowing users to concatenate new content to an existing drawer's body instead of replacing it. It also updates the tool's schema description and adds corresponding unit tests. A review comment points out a potential TypeError if the existing drawer content (old_doc) is None when attempting to append, and suggests safely defaulting it to an empty string.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

Comment thread mempalace/mcp_server.py Outdated
sanitized = sanitize_content(content)
except ValueError as e:
return {"success": False, "error": str(e)}
new_doc = old_doc + sanitized if append else sanitized

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

medium

If old_doc is None (which can happen if a drawer was created without a document or if the database is in an unexpected state), attempting to concatenate old_doc + sanitized will raise a TypeError.

To prevent this, we should safely default old_doc to an empty string if it is None.

Suggested change
new_doc = old_doc + sanitized if append else sanitized
new_doc = (old_doc or "") + sanitized if append else sanitized

When append=True, content is concatenated onto the existing drawer body
instead of replacing it, so a caller updating a long drawer need only send
the delta rather than re-transmitting the whole body on every edit. Defaults
to False (replace), preserving prior behavior. The server already fetches the
existing document, so this is a minimal read-modify-write.
…2b9)

Chroma can return None for a stored drawer's document body. The append
path concatenated old_doc + content directly, which raises TypeError
when old_doc is None. Coerce a missing body to "" before concatenating,
and add a regression test driving the None-body case directly.

Addresses the PR MemPalace#1762 review feedback.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add append option to update_drawer (avoid re-sending the full drawer body on incremental updates)

1 participant