fix(mental-models): let PATCH update content and stop misreporting empty updates as 404 - #3373
Open
JoshFunnell wants to merge 1 commit into
Open
fix(mental-models): let PATCH update content and stop misreporting empty updates as 404#3373JoshFunnell wants to merge 1 commit into
JoshFunnell wants to merge 1 commit into
Conversation
…pty updates as 404
UpdateMentalModelRequest had no `content` field, so Pydantic silently
dropped it from PATCH /mental-models/{id} even though
MemoryEngine.update_mental_model fully supports content= (embedding
recompute, mental_model_history row, last_refreshed_at) -- any
restore-from-history flow over the REST API was a silent no-op. The
same code path returns None both for "model not found" and "no fields
to update", so an empty PATCH against a model that genuinely exists
was misreported as a 404.
- Add `content` to UpdateMentalModelRequest and pass it through to the
engine call.
- Validate at least one updatable field is present before calling the
engine, returning 422 instead of the ambiguous 404 (mirrors the
existing update_document precedent at the same file).
- Regenerate the committed openapi.json (source + skill mirror) for
the new field via `generate-openapi`; diff is limited to the two
expected schema/description changes. The generated language clients
(hindsight-clients/*) still need a Docker-based
`scripts/generate-clients.sh` regen on Linux -- not run here.
- Tests: content-only PATCH applies + records history + no 404; empty
PATCH returns the clear 422 not 404; name-only PATCH unchanged.
The MCP update_mental_model tool (hindsight_api/mcp_tools.py) is
hand-declared, not derived from UpdateMentalModelRequest, and its own
docstring says content changes should go through refresh_mental_model
-- left as-is; see report for detail.
JoshFunnell
force-pushed
the
upstream/mm-content-filed
branch
from
August 11, 2026 06:38
b1dcd0d to
a48474f
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two small fixes to
PATCH /v1/default/banks/{bank_id}/mental-models/{mental_model_id}.1.
contentis settable on the engine but unreachable through the APIMemoryEngine.update_mental_modelalready acceptscontent=and does the right things with it — recomputes the embedding, records the previous content inmental_model_history, advanceslast_refreshed_at. The HTTP layer just never passes it:UpdateMentalModelRequesthas nocontentfield, so the capability exists and cannot be reached.The concrete case is restoring a previous version from mental-model history. Today the only way to change a model's content over HTTP is to trigger a refresh and accept whatever the LLM produces, which is not a restore.
/clearempties content for a clean rebuild; it cannot put a known-good version back.This adds the field and forwards it. No engine change.
Worth being explicit about, since it is a product decision rather than a mechanical one: setting
contentgoes through the existing engine path, which advanceslast_refreshed_at. A restore therefore counts as a refresh for staleness and scheduling purposes. That is inherited behaviour, not something this PR introduces, but it becomes reachable here for the first time — so if a restore should not reset that clock, that is an engine change and a separate discussion.2. An empty update reports 404 for a model that exists
The handler passes every field through and then maps a
Nonereturn to:update_mental_modelreturnsNonewhen there is nothing to update, soPATCHwith an empty or all-null body answers 404 "not found" for a model that is present and fine. That sends the caller looking for a missing model instead of at their own request.Now rejected up front with 422 when none of the fields this handler accepts are set (
name,content,source_query,max_tokens,tags,trigger), with a message naming them. Empty and all-null bodies are rejected before the engine is called; a missing model still returns 404 whenever the body has at least one accepted field.On field semantics, since it is the obvious question: omitting a field, or sending
null, means "leave it alone". Any non-null value means "set it" — including"", which is therefore a second way to empty a model alongside/clear, differing in that it writes a history row and advances the clock. There are no length bounds on the field; validation, embedding and history are all the existing engine path.Note the engine's return value stays overloaded —
Nonestill means both "not found" and "nothing to do" for any other caller. This PR peels the empty-body case off at the HTTP layer rather than changing that contract.Tests
Three, in
test_reflections.py, run locally against the embedded Postgres fixture:test_update_mental_model_content_via_api— content set through the API round-trips (PATCH echo and a fresh GET), a history row records the previous content, andlast_refreshed_atadvances. Embedding recomputation rides the same engine path but has no API-visible surface, so it is not asserted.test_update_mental_model_empty_body_returns_422_not_404— pins the fix, and fails before it.test_update_mental_model_name_only_unchanged_behavior— a name-only PATCH still succeeds unchanged, so the new branch does not swallow ordinary single-field updates.Generated artifacts
openapi.json, the bank-template schema, the docs skill, and the Go/Python clients are regenerated. The TypeScript entry ingenerated/types.gen.tsis the additivecontentfield only, matching what #3332 and #3223 landed for their request-model fields — a full local TS regeneration rewrites ~7k lines across all 16 generated files even on a pristinemainwith the pinned generator and CI's Node 20, becausehindsight-clients/typescripthas nopackage-lock.jsonand the generator's transitive dependencies float. That drift is unrelated to this change and is not carried here.Surfaces deliberately not updated
The repo is wider than the engine, so rather than leave these silent:
hindsight-api-slimhandler + request modelopenapi.json(+ docs skill copy)hindsight-cli—mental-model update--contentflag. Setting content is a restore/repair operation and a whole document body is awkward as a CLI flag;mental-model refreshremains the CLI path for regenerating content. Note the coverage manifest needs no entry:cli-coverage-checkalready countscontentas covered because it matches the identifier globally inmain.rs, where it belongs to other commands. Happy to add the flag if you'd prefer it exposed.hindsight-control-plane—mental-models-view.tsxScope
Nothing else changes. No new engine behaviour, no change to the refresh implementation, and the 422 fires only on a body with none of the accepted fields set.