Critical: muse hub proposal merge can silently corrupt dev if called twice (stale state read + stale-base re-merge)
Summary
Discovered live while merging proposal #9 (musehub#144). muse hub proposal merge can be called a second time on a proposal that was already successfully merged, because the first call's state/merge_commit_id/merged_at update to musehub_proposals did not appear to persist immediately (or wasn't visible to a subsequent read). The guard that should reject a second merge (if proposal.state not in ("open", "approved"): raise RuntimeError(...), musehub/services/musehub_proposals.py::merge_proposal) saw the proposal as still "open" and allowed a second merge to proceed.
This is not just a duplicate-call annoyance — it caused real, silent data loss. The second merge re-ran the full merge algebra using proposal.from_branch/proposal.to_branch's current head commits as its 3-way-merge inputs. Because from_branch (feat/9a-4-f7-overseer-provenance) was a long-lived branch whose merge-base with dev was months stale (forked in July; dev had ~90 commits of unrelated work since), the second merge computed a 3-way merge against an ancient common ancestor and silently reverted or deleted 47 files relative to dev's real, correct state — including deleting a brand-new migration file and two just-added test files outright, and reverting ~15 other files (deploy scripts, security fixes, docs) to months-old content. No error, no conflict, no warning — the merge endpoint returned {"merged": true, ...} exactly like the first, correct call did.
What happened, concretely
muse hub proposal merge <id>(1st call) → returned{"merged": true, "mergeCommitId": "sha256:8a1389ae..."}.devcorrectly advanced to a commit containing all ofdev's prior state plus the proposal's new files. Verified via direct RDS query: at this pointmusehub_proposals.statewas still'open', not'merged', despite the API call succeeding and the branch genuinely advancing.- Deployed
dev's new head to staging, verified the new feature live and working. - Re-read the proposal (
muse hub proposal read) →state: "open",mergeCommitId: null. Investigating this apparent bookkeeping bug, re-ran the merge call to see if it would reproduce. muse hub proposal merge <id>(2nd call) → returned{"merged": true, "mergeCommitId": "sha256:4bbb33ce..."}— a different, new merge commit. This timemusehub_proposals.statecorrectly became'merged'. Butdev's branch ref on staging now pointed at this second, stale-based merge commit, which — confirmed viamuse snapshot-diffagainst the correct commit from step 1 — had 0 additions, 43 modifications (many regressive), and 4 deletions relative to the correct state.- Caught this before propagating further (never reached
local/productionremotes). Fixed by force-resettingstaging/dev's ref back to the correct commit from step 1, and directly correcting themusehub_proposalsrow'smerge_commit_idto match.
Two distinct bugs, both real
- State-persistence bug: the first
merge_proposalcall's DB write tostate/merge_commit_id/merged_atdid not take effect (or wasn't read back correctly) even though the branch-advancing part of the same transaction clearly succeeded and was later independently confirmed live. Needs root-causing — possibly a session/commit-ordering issue, a stale read from a follow-up request hitting a different connection before replication/visibility, or a real transaction bug inmerge_proposal. this is the more urgent of the two to root-cause, since it's what enables bug #2. - No merge-base staleness guard: even setting aside bug #1,
merge_proposalwill happily compute and apply a 3-way merge against a merge-base that is arbitrarily old, with no check thatto_branch's current head is otherwise unreachable from what the proposal's own anchors expect. A proposal open for months against a fast-moving target branch is a real, unremarkable scenario (not a misuse) — the merge algebra should either detect this and refuse/warn, or the merge computation should treatto_branch's current live manifest as authoritative-and-additive rather than diffing against a stale common ancestor that makes unrelated intervening changes look like reverts.
Suggested next steps
- Reproduce bug #1 in a test: merge a proposal, immediately re-read state in the same test process, assert it's
'merged'before any second merge is attempted — this should already catch the persistence gap in isolation. - Add an idempotency/regression test: call merge twice in a row on the same proposal; the second call must return 409 (already merged), never a second successful merge.
- For bug #2: add a test where
to_branchhas diverged significantly from the proposal's original fork point, and assert the merge either refuses (with a clear error asking the caller to rebase) or correctly preserves all ofto_branch's intervening independent changes — not just the files the merge happens to also touch. - Audit whether other proposals sitting open against
devfor a long time are at risk of the same silent-revert failure mode if merged as-is.
Mirrored from staging#182.