fix(web/ctx): conflict dialog + force-save on 409 mtime mismatch (#763)#800
Merged
fix(web/ctx): conflict dialog + force-save on 409 mtime mismatch (#763)#800
Conversation
The Context Gateway editor's only response to a 409 mtime_ns mismatch
was a toast (`settings.ctx.mtime_conflict`) followed by a silent
`loadCtxDetail` that replaced whatever the user had typed with on-disk
content. This was tolerable when conflicts were rare; with parallel
Web sessions and the RFC-761 Settings prod expansion landing more
concurrent writers, the default of "destroy the user's work whose
only sin was racing another editor" was wrong.
The save handler now opens `#ctx-conflict-modal` with side-by-side
previews of the user's buffer vs the freshly-fetched on-disk content,
plus three explicit buttons:
* Reload (discard my edits) — fetch fresh, drop the draft.
* Open diff editor — render `diffLines(buffer, freshContent)`
inline above the textarea so the user can hand-merge; refresh
`dataset.mtimeNs` to the freshly-read value so the next Save
no longer 409s.
* Force save (overwrite) — re-PUT with the new opt-in
`force: true` body field, then `conflict_force_done` toast.
The buffer is stashed in `sessionStorage` on every 409 entry so an
Escape-out / accidental tab close does not destroy work — the next
mount of the same `(type, name)` rehydrates the textarea and surfaces
a `conflict_draft_restored` info toast.
Backend: `SkillUpdateRequest` / `AgentUpdateRequest` /
`CommandUpdateRequest` each gain a `force: bool = False` field. When
`force=True` the mtime guard inside `_gateway_lock` is skipped and
the bypass is logged at WARNING with the path plus both client and
server `mtime_ns` values, so the audit trail is reconstructable from
logs alone. The 409 response shape itself is unchanged — RFC-761 PR-2
(#770) just pinned `{"status":"aborted","reason":...,"mtime_ns":"<str>"}`
across all three routes, and adding `current_content` to every 409
response would have bloated payloads on a hot error path. The dialog
fetches fresh content via a normal GET when it opens.
The legacy `settings.ctx.mtime_conflict` i18n key is no longer used by
code but kept for one release as a deprecated alias to avoid churn
for downstream translators.
Tests: parametrize over the 3 mutator types (`agents` / `commands` /
`skills`) for `test_PUT_force_bypasses_mtime_check`,
`test_PUT_force_logs_warning_with_both_mtimes`, and
`test_PUT_force_default_false_still_409s` — the last one is the
symmetric pin (positive marker + negative `!=`) for the opt-in
default per `feedback_pin_invert_symmetric_assertion.md`.
Co-Authored-By: Claude <[email protected]>
…PUT (#763) Two review-driven fixes on top of the conflict-dialog change: 1. ``_ctxResolveConflict`` was auto-focusing the destructive Force-save button after opening the modal. The modal exists precisely to make the override an explicit choice; auto-focusing the danger button let a reflexive Enter-press silently overwrite the other writer's work. Focus the Reload button instead — Reload preserves on-disk content, and the user can still tab to Force. 2. The force PUT was sending ``fresh.mtime_ns`` (just-fetched, equal to the server's current value) as the body's ``mtime_ns``. Result: the server-side WARNING log printed ``client_mtime_ns == server_mtime_ns`` in nearly every case, defeating the audit trail's "what was being overridden" purpose. Thread the original stale ``mtime_ns`` (the value ``detailEl.dataset.mtimeNs`` held when the user first hit Save and got 409) through ``_ctxHandleConflict`` and send that instead, so the WARNING captures distinct endpoints of the override. Backend tests already pin ``client_mtime_ns=0`` vs ``server_mtime_ns > 0`` so the contract "log whatever the client sent, both endpoints visible" is covered. Regression risk for the JS contract itself is documented in the function-level comment block on ``_ctxHandleConflict``. Cache-bust ``context-gateway.js?v=15`` → ``?v=16`` per ``feedback_static_asset_cache_bust.md`` (JS body changed). Co-Authored-By: Claude <[email protected]>
memtomem
force-pushed
the
fix/issue-763-mtime-conflict-dialog
branch
from
65b307a to
b00dbe0
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
2 participants
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.
Summary
PUT /api/context/{type}/{name}409 mtime mismatch with a 3-button conflict dialog (Reload / Open diff editor / Force save) that previews the user buffer side-by-side with the freshly-fetched on-disk content. Buffer is stashed insessionStorageso an Escape-out / tab close no longer destroys work.force: bool = FalsetoSkillUpdateRequest/AgentUpdateRequest/CommandUpdateRequest. Whenforce=Truethe mtime guard inside_gateway_lockis bypassed and the override is logged at WARNING with the path plus both client and servermtime_nsvalues, so the audit trail is reconstructable from logs alone.{"status":"aborted","reason":...,"mtime_ns":"<str>"}across all three routes, and shippingcurrent_contenton every 409 would have bloated payloads. The dialog fetches fresh content via a normal GET when it opens.Closes #763.
Why this change
With the RFC-761 Settings prod expansion landing more concurrent writers and parallel Claude / Web sessions becoming routine, the previous default ("toast +
loadCtxDetailoverwrites the user's text") destroyed work whose only sin was racing another editor. The dialog forces an explicit user choice; the new force-save path preserves that choice's audit trail at the server log layer.Test plan
uv run pytest packages/memtomem/tests/test_web_routes_context_mutators.py -k force— 9 new tests pass (3 force scenarios ×agents/commands/skills).uv run pytest -m "not ollama"— 4199 pass / 11 skipped / 0 fail.uv run ruff check packages/memtomem/src packages/memtomem/tests+ruff format --check— clean.uv run pytest packages/memtomem/tests/test_i18n.py— 19 pass; new keys parity-checked againsten.json/ko.json.curlagainstmm web --port 8181: create skill → stale PUT (no force) returns 409 withmtime_nsecho → stale PUT withforce: truereturns 200 + content overwritten →WARNING force-save bypassed mtime check on … (client_mtime_ns=… server_mtime_ns=…)emitted on the server./, 0 console errors, modal element + 3 buttons + 7 helper functions all resolved onwindow. KO i18n keys render correctly._ctxResolveConflictround-trip resolved to'force'and'diff'choices when buttons clicked. Inline_ctxRenderConflictBannerproducesdiff-add/diff-dellines with the내 수정 ↔ 디스크 현재 내용heading.echo "drift" >> .memtomem/skills/<name>/SKILL.mdfrom another shell → click Save → walk through each of Reload / Force / Open diff editor) to confirm UX feels right under real conflict timing.Notes for reviewers
origin/mainbefore fix(web/ctx): surface Settings needs_confirmation in Sync All toast (#774) #798 (issue fix(web/ctx): Sync All silently skips Settings host-write target on default allow_host_writes=False #774, also touchedcontext-gateway.js+ locales) merged. The branch was rebased ontoorigin/mainafter fix(web/ctx): surface Settings needs_confirmation in Sync All toast (#774) #798 landed; no semantic conflict, only a cache-bust version stamp onindex.html. PR fix(web/ctx): surface Settings needs_confirmation in Sync All toast (#774) #798'sSync All needs_confirmationlogic is preserved verbatim alongside this work.settings.ctx.mtime_conflicti18n key is no longer referenced by code but kept for one release as a deprecated alias to avoid churn for downstream translators. Slated for removal in the next minor.sessionStorageper(type, name).WARNINGlog only. "Force-saved Nm ago" Detail-card header is YAGNI; defer.🤖 Generated with Claude Code