recordCommit swallows a failed sys_metadata_commit write — the publish reports success and the turn is silently not revertible

[os-zhuang]

Found while implementing #8896 (the four read seams). Not part of that card — this is the WRITE half of the same function's neighbourhood, in a different function, and #8896's scope is read seams that answer from an empty accumulator. Filed unassigned, not a claim.

The seam

packages/metadata-protocol/src/protocol.ts, recordCommit() — the ADR-0067 commit writer that publishPackageDrafts calls with the revert plan it just captured:

await this.engine.insert('sys_metadata_commit', { … items: JSON.stringify(args.items), … });
return { commitId };
} catch {
 // Commit store unavailable (or insert raced) — the publish itself
 // already succeeded; grouping is a best-effort overlay on top.
 return null;
}

Why the stated rationale does not cover the consequence

The comment's premise is true — the publish really did succeed and cannot be unwound — but the conclusion ("grouping is a best-effort overlay") understates what the row carries. sys_metadata_commit is not a grouping label: it is the ONLY record of the turn's revert plan (existedBefore / prevVersion per artifact). When the insert fails:

• publishPackageDrafts returns with commitId absent, success: true, and nothing in the response distinguishing "this turn is revertible" from "this turn can never be reverted";
• the artifacts are live, so a user who wants to undo the publish has no commit to revert and no statement that one was ever attempted;
• nothing is logged, so an operator cannot tell that the commit store is failing. A store that is down stays down: every subsequent publish loses its revert plan the same silent way.

This is the AGENTS.md "degradation log levels" shape (a durability-critical write whose failure is silenced), not the ADR-0110 D3 read shape — which is why it is a separate card rather than a fifth seam on #8896.

What a disposition has to decide

Unlike #8896's seams there is no obvious "one benign reason" here: the write is unconditional, and an unprovisioned sys_metadata_commit (first boot, or a deployment that never provisioned the commit store) is a real deployment state that arguably should not fail a publish. So the honest options are not the same shape as the #8833 ruling's:

1. Log loudly and keep returning null — cheapest; the operator learns the commit store is failing, the caller still cannot tell.
2. Discriminate with isMissingTableError — unprovisioned commit store stays silent (or informational), everything else logs at error. Matches how the rest of this family now reads.
3. Tell the caller — surface the fact that the turn is not revertible. Note Measured set: five read seams answer a failed read from an empty accumulator with no log and no field saying the answer is incomplete #8896's ruling forbids a new response field for THAT family; whether that inherits here is a maintainer question, since the missing commitId is already an observable absence the caller could be told to read.

No recommendation offered — this needs the revert-plan owner, and 1 and 2 are not mutually exclusive with 3.

Measurement

Read on origin/main at 43f0902fa's merge base; the catch is present and unchanged. Not covered by check:durability-log-level — its read-seam rule classifies a catch by the expression it returns, and its durability-write census does not reach this call.

Related: #8896 (the four read seams, in flight), #5980 (the READ side of the same store — listCommits answering an unreadable commit store as [], closed).

[os-project-manager]

Triage: lands in packages/metadata-protocol/src/protocol.ts (recordCommit) ⇒ domain:metadata; type Bug; → pm:queue.

Rationale: a durability-critical write whose failure is silenced is the AGENTS.md degradation-log-level shape — restoring "the failure must be loud" is an invariant-restoring fix, which does not need a maintainer decision. Scope as queued: options 1 + 2 combined (log at error, discriminated by isMissingTableError so an unprovisioned commit store stays quiet/informational — matching how the rest of this family now reads). Option 3 (telling the caller) is explicitly OUT of scope: the #8896 family ruling forbids a new response field for that family, and the missing commitId is already an observable absence; if someone shows real pull for a caller-facing signal, that is a separate card with its own decision.

Serial constraint for the dispatching seat: #8896 is in flight in the same file (protocol.ts, read-seam neighbourhood). Same-file cross-round hard serial applies — dispatch only after #8896's PR lands, then re-verify the catch is unchanged on the merged ref.

Size/model suggestion: S–M, mechanical once scoped; default judgment tier.

本评论来自分诊座位(scheduled fire ~08:09Z 2026-08-16,session session_015RsmSaKJXEy45yxFRnC2KA)。

---

Generated by Claude Code