feat(i18n): migrate modules/message to ResponseErrorL (Phase 2.1)

[an9xyz]

Summary

Phase 2.1 i18n migration of modules/message — the next hot module after thread (#176), user (#188 / #197) and group (#198). All 336 legacy error-response sites across 8 files migrate from the legacy c.ResponseError(...) / c.ResponseErrorf(...) / c.ResponseErrorWithStatus(...) envelope to the localized httperr.ResponseErrorL facade, reusing the established playbook.

Organized as 8 reviewable commits: code registry → respond helpers → per-file migration → tests.

file	sites
api.go	138
api_manager.go	60
api_pinned.go	56
api_conversation.go	44
api_message_get.go / api_reminders.go / api_channel_files.go / api_sidebar.go	38

What changed

• 23 new err.server.message.* codes (pkg/errcode/message.go) + zh-CN runtime translations + regenerated AST marker. Codes group by class: validation (400), permission (403), not-found (404), recall window / pinned limit (400), and Internal=true query / store / notify / search buckets (500).
• modules/message/api_i18n.go respond helpers (respondMessageRequestInvalid with field detail, respondMessagePinnedLimitExceeded with max detail, and the shared auth.required / auth.token_invalid / auth.forbidden guards for the proxy-send and management-console paths).
• Internal failures collapse to the Internal=true buckets; every such site keeps (or gains) an m.Error(..., zap.Error(err)) log so the underlying error is never put on the wire. ResponseErrorf("msg", err) and errors.Wrap(err, "msg") sites preserve the original message as the log.
• Sentinels reused where the service layer already provides them: authorizeMutualDelete → delete_forbidden (403), verifyRevokeMessageID → id_seq_mismatch (400).

Behavior change (frontend, please note)

Following the i18n design (D14), every migrated error response returns HTTP wire status 400 during the compatibility window; the semantic status lives in error.http_status and clients should branch on error.code. Three api_conversation.go sync-failure sites that previously returned a real 500 now return a 400 envelope with error.http_status: 500 (Internal). This is consistent with the thread/user/group migrations already merged.

Test plan

• go build ./...
• go vet ./modules/message/
• make i18n-lint (D23 direct-error-response ratchet + unregistered-code)
• make i18n-extract-check (AST marker recall)
• go test ./modules/message ./pkg/errcode ./pkg/i18n/... (clean DB) — the full module's existing 642 test cases pass unchanged, since they assert the wire-400 contract that D14 preserves.
• New tests: TestMessageNoLegacyResponseError (source guard over all 8 files) and TestRespondMessageHelpers (helper / sentinel / Internal-no-leak / dual-envelope / detail keys).

Follow-ups (non-blocking)

• Service-layer sentinel errors (errors.Is) to replace the err-string matching used for business-vs-internal classification — the same shared follow-up tracked for thread/user/group.

[Jerry-Xin]

The PR is relevant to octo-server, but the migration is incomplete for a migrated message API file.

🔴 Blocking

• 🔴 Critical: modules/message/api_message_get.go still returns direct raw 404 JSON for not-found/invisible message cases. The same pattern appears at lines 186, 197, 224, 234, 245, 249, 264, and 291. These responses bypass httperr.ResponseErrorL, do not include error.code, are not localized, and return wire status 404 instead of the D14 compatibility wire status 400. Use ErrMessageNotFound for all indistinguishable not-found/access-denied cases; that preserves the no-enumeration behavior while keeping the i18n envelope.

💬 Non-blocking

• 🟡 Warning: modules/message/api_pinned.go returns ErrMessagePinnedLimitExceeded directly, so the new respondMessagePinnedLimitExceeded helper and its safe max detail are never used by the real handler.
• 🔵 Suggestion: modules/message/api_conversation.go and line 1084 log internal query failures without zap.Error(err), which weakens the “internal details in logs, not on the wire” rule.
• 🔵 Suggestion: modules/message/api_pinned.go logs the same store failure twice.

✅ Highlights

• The new ErrMessage* registry entries are structured consistently and mark 5xx buckets as Internal=true.
• The helper tests cover localized envelopes, shared auth codes, safe details, and internal-message suppression.
• Legacy ResponseError usage is removed from the targeted files.

Validation note: go test ./modules/message ./pkg/errcode ./pkg/i18n/... could not complete locally because modules/message test setup requires Redis at 127.0.0.1:6379; the i18n and errcode packages passed before that failure.

[lml2468]

[REQUEST_CHANGES]

Solid Phase 2.1 migration — 336 sites across 8 files, error classification is thorough, and the test guard + helper coverage matches the group module playbook. However, there is one blocking gap that Jerry-Xin already identified, plus a few additional items.

🔴 Critical (blocking)

1. api_message_get.go: 9 raw c.JSON(http.StatusNotFound, ...) calls bypass the i18n envelope entirely — agreeing with Jerry-Xin's finding. These return wire status 404 with {"msg": "message not found"} which violates the D14 contract (wire 400, semantic status in error.http_status), is not localized, and has no error.code. These are visibility-masking responses (treating not-found and access-denied identically) — ErrMessageNotFound already has exactly the right semantics for this. The TestMessageNoLegacyResponseError source guard does not catch these because it looks for c.ResponseError( not c.JSON(.

🟡 Warning (blocking)

2. api_message_get.go: 7 duplicate m.Error log calls — the migration introduced a second m.Error call at lines 132, 177, 190, 219, 240, 259, 269 while preserving the original. Each error now logs twice with the same message. Example at line 131-133:

m.Error("查询子区失败", zap.Error(err))  // original
m.Error("查询子区失败", zap.Error(err))  // added by migration
httperr.ResponseErrorL(c, errcode.ErrMessageQueryFailed, nil, nil)

Remove the duplicates — one log per error is sufficient.

3. api.go line 2246-2247: duplicate m.Error in revoke — same pattern:

m.Error("查询IM消息错误", zap.String("fakeChannelID", fakeChannelID), ...)
m.Error("查询IM消息错误", zap.Error(err))  // should merge into the line above

These should be a single m.Error call with all fields.

🔵 Suggestions (non-blocking)

4. api_pinned.go:143: calls httperr.ResponseErrorL(c, errcode.ErrMessagePinnedLimitExceeded, nil, nil) directly instead of using respondMessagePinnedLimitExceeded(c, maxCount), so the max safe detail is never surfaced to the client. The helper exists but is dead code.

5. api_conversation.go: Jerry-Xin noted two co.Error calls missing zap.Error(err) — agree this weakens the "log internally, not on the wire" contract.

✅ Highlights

• 23 new err.server.message.* codes well-organized by class with proper Internal=true on 500 buckets.
• ErrMessageSearchFailed as a dedicated internal bucket is good — separates external search-service failures from DB failures.
• TestMessageNoLegacyResponseError source guard covers all 8 files.
• Helper tests cover localized envelopes, shared auth codes, detail keys, and internal-message suppression.

CI: Build ✅ | Lint ✅ | Vet ✅ | i18n ✅ | Test pending.

[yujiawei]

Code Review — PR #203 (octo-server)

Verdict: APPROVED

This is a clean, well-executed Phase 2.1 i18n migration of modules/message (336 legacy error-response sites across 8 handler files → the localized httperr.ResponseErrorL facade). I reviewed all 13 changed files against the failure modes that bit the earlier thread/user migrations (transaction lifecycle, semantic-status regressions, internal-error leakage, wrong code mapping, dropped returns, auth-guard mapping, detail-key safety). The migration introduces no behavioral regression and is safe to merge.

Below are the verification results, then three latent (pre-existing) transaction-leak observations and a few minor cleanups. None of the latter block merge.

---

1. Verification results

Check	Result	Evidence
Wire status preserved	✅	Legacy c.ResponseError* always returned HTTP 400; ResponseErrorL hard-codes transport 400 during the compat window and carries the corrected semantic status in error.http_status. No client-visible status change.
Code → status mapping	✅	not_friend / peer_not_in_space / channel_access_denied / delete_forbidden / recall_forbidden / not_group_member → 403; *_not_found → 404; id_seq_mismatch / recall_time_exceeded / request_invalid → 400; query/store/notify/search failures → 500 Internal=true. All semantically faithful to the original Chinese strings.
ResponseErrorWithStatus(…, 500) sites	✅	The 3 explicit-500 sites (api_conversation.go, querying external group / default space / joined groups) all map to ErrMessageQueryFailed (500, Internal=true) with the underlying err still logged. Intent preserved.
Internal-error leak	✅	Every Internal=true site keeps (or newly adds) m.Error(..., zap.Error(err)); the wire response carries no raw error. The renderer collapses Internal=true codes to err.shared.internal and empties details (verified in pkg/i18n/renderer.go + TestRespondMessageHelpers wantNotContains). Two previously-unlogged conversation sites gained logging — a net improvement. api_reminders.go:53 also fixes a prior leak (nextReminderSeq err used to be surfaced raw via c.ResponseError(err); now mapped + logged).
Auth guards	✅	Proxy-send path: 请先登录 → err.shared.auth.required (401), token errors → err.shared.auth.token_invalid (401); management-console role guards → err.shared.auth.forbidden (403). Shared statuses confirmed in pkg/i18n/codes/shared_test.go.
return retained	✅	Every migrated site keeps its trailing return; no fallthrough / double-response. No live legacy c.ResponseError calls remain (only commented-out breadcrumbs).
Detail-key safety	✅	Only SafeDetailKeys (field, max) are ever populated; Details.FilterBy(registered) strips anything else. No PII.
Locale completeness	✅	All 23 err.server.message.* codes have exact 1:1 entries in both active.zh-CN.toml and the active.en-US.toml markers; en-US markers match DefaultMessage byte-for-byte; no orphan/missing keys; valid TOML.
Build / tests	✅	go build ./modules/message/... ./pkg/errcode/... passes. New TestMessageNoLegacyResponseError (source guard) + TestRespondMessageHelpers (status/detail/no-leak) pass.

---

2. Findings

P2 — Latent transaction-connection leaks (pre-existing, NOT introduced by this PR)

Three error paths inside a tx := Begin() span return without calling tx.Rollback(), and the surrounding defer only rolls back on recover() (panic), not on a normal early return. Each leaks the open transaction + its DB connection on that path.

1. modules/message/api_manager.go:166-170 — delete() loop, genMessageExtraSeq error path.
2. modules/message/api_reminders.go:53-57 — reminderDone() loop, nextReminderSeq error path.
3. modules/message/api.go:2324-2328 and 2382-2386 — revoke(), genMessageExtraSeq and deletePinnedMessage error paths.

In every one of these functions, the sibling error paths do call tx.Rollback() (e.g. api_manager.go:181/191/207), so the omission is clearly unintentional. Example (api_manager.go):

version, err := m.genMessageExtraSeq(fakeChannelID)
if err != nil {
    m.Error("生成消息扩展序列号失败", zap.Error(err))
    httperr.ResponseErrorL(c, errcode.ErrMessageStoreFailed, nil, nil)
    return // ← no tx.Rollback(); defer is panic-only → tx + conn leak
}

Why this is P2 and not a blocker:

• Pre-existing. git show <base>:… confirms these blocks are byte-identical in the base — the migration only swapped c.ResponseError(err) for ResponseErrorL(...) + added a log, preserving the existing control flow. No regression vs. the current main.
• Not on-demand exploitable. The leak only fires when GenSeq / nextReminderSeq (a Redis/DB sequence call) itself fails — an infrastructure failure, not something a caller can force with crafted input. So this is connection-pool amplification during a DB/Redis incident, not a standalone DoS primitive.

Recommendation (strongly suggested, since these exact lines are already being edited): add tx.Rollback() before the response on each of the four paths, or replace the panic-only defer with defer tx.RollbackUnlessCommitted() (the pattern already used correctly by messageEdit() at api.go:741 and the offset/readed handlers). It's a 1-line-per-site change that closes a real latent leak while the code is open. If you'd rather keep this PR scoped strictly to i18n, please file a tracking issue rather than letting it ride — these are easy to lose once the lines stop being touched.

⚠️ For the human reviewer (security-sensitive PR): the three tx-leak paths above are the items worth a manual eye. They are not regressions and not on-demand exploitable, but they are genuine latent resource leaks on DB-mutation paths that two regular-user endpoints (/v1/message/revoke, /v1/message/reminder/done) and one admin endpoint (DELETE /v1/manager/message) pass through.

P2 — Read-path failures mapped to StoreFailed instead of QueryFailed (cosmetic)

modules/message/api_conversation.go:252 (ExistMember, a pure SELECT count(*)) and :268 (GetUser, a read) map to ErrMessageStoreFailed, but the errcode docstrings reserve StoreFailed for mutations and QueryFailed for reads — and a sibling read at :1132 already uses QueryFailed. No user-facing impact (both are Internal=true, semantic 500, wire 400, no message leaked); this only affects log-grep/category consistency. Safe to fix or defer.

Nits (non-blocking)

• pkg/errcode/message.go:55 — ErrMessageCannotDeleteSelfConversation (correctly 400) is filed under the // permission (403) comment block; move it to the // validation (400) block for readability. Status is correct as-is.
• modules/message/api_i18n_test.go:54 — wireI18nRendererForMessageTest is defined but never called (dead code, plus an otherwise-unused server import). Either delete it or use it to add one end-to-end case that routes through real server boot wiring (the current suite validates helpers/renderer in isolation, not the 336 migrated call sites or the main.go renderer wiring).
• modules/message/api_i18n_test.go:30 — the legacy-guard's hard-coded 8-file list is complete today but won't auto-cover a future api_*.go handler; consider a filepath.Glob walk.
• Several read-path error sites in api_message_get.go now emit the underlying error twice (a pre-existing m.Error(...) plus a newly-added duplicate m.Error(..., zap.Error(err))), and at :218-219 the duplicate drops the richer channel_id/message_id context. Trim the duplicate log lines. Cosmetic log-volume issue only.
• A few respondMessageRequestInvalid(c, "") sites (api_channel_files.go check(), api_sidebar.go:222 validateSidebarRequest) collapse distinct validation messages into one generic 400 with no field detail. Status is unchanged; only client diagnosability is reduced. Consistent with the compat-window design — optional to enrich later.

---

3. Summary

The migration faithfully preserves wire behavior, maps every code to the correct semantic status, never leaks internal errors, and is well covered by new tests. The only items of substance are three pre-existing transaction-rollback omissions that this PR happens to touch — I recommend fixing them in place (cheap, real) but they do not regress current behavior and are not a merge blocker. Approving.

[Jerry-Xin]

The PR is in scope for octo-server and the migration is broadly consistent with the existing i18n error-response pattern.

💬 Non-blocking

🟡 Warning — modules/message/api_conversation.go:252 and modules/message/api_conversation.go:268 map read failures during membership/user lookup to ErrMessageStoreFailed. Both are still internal 500 envelopes, so this is not a blocker, but ErrMessageQueryFailed would better match the new query/store bucket contract and keep error.code semantics cleaner for clients and observability.

✅ Highlights

• Legacy message-module error responses are covered by a source guard in modules/message/api_i18n_test.go.
• New message error codes are registered and have runtime translations plus extracted source markers.
• Internal errors consistently use Internal=true codes, avoiding raw error leakage.

Verification:

• make i18n-lint passed.
• make i18n-extract-check passed.
• go test ./pkg/errcode ./pkg/i18n/... passed.
• go test ./modules/message ./pkg/errcode ./pkg/i18n/... could not complete locally because Redis was not running at 127.0.0.1:6379; modules/message panicked in newTestServer().

[yujiawei]

Code Review — PR #203 (octo-server)

Verdict: APPROVED

Phase 2.1 i18n migration of modules/message (336 legacy error-response sites across 8 handler files → the localized httperr.ResponseErrorL facade, plus 23 new err.server.message.* codes with full zh-CN + en-US marker parity). I reviewed all 13 changed files at head SHA 1afea165, with focus on the failure modes that bit the earlier thread/user migrations: transaction lifecycle, semantic-status regressions, Internal-error leakage, and auth-guard correctness.

The blocking finding raised earlier in this PR — raw c.JSON(http.StatusNotFound, …) calls in api_message_get.go bypassing the i18n envelope — has been resolved in the latest commit (fix(i18n): address PR #203 review — migrate raw c.JSON 404s + dedup logs). All 9 sites now return errcode.ErrMessageNotFound, the visibility-masking (no-enumeration) behavior is preserved, and a new source-guard assertion (TestMessageNoLegacyResponseError now also forbids non-OK c.JSON(http.Status…)) prevents regression.

Verification

Check	Result
go build ./modules/message/... ./pkg/errcode/... ./pkg/i18n/...	✅ pass
go vet ./modules/message/	✅ pass
TestMessageNoLegacyResponseError (8-file source guard)	✅ pass
TestRespondMessageHelpers (helper / sentinel / Internal-no-leak / detail keys)	✅ pass
Error-code ↔ locale parity	✅ 23 registered = 23 zh-CN = 23 en-US, all IDs match
Transaction rollback in migration-touched code (api_pinned.go clearPinnedMessage, api.go:735)	✅ every early-return branch rolls back / uses RollbackUnlessCommitted
Auth guards (respondMessageNotLoggedIn/TokenInvalid/Forbidden) → 401/401/403	✅ correct semantics, all guards terminate the handler
Internal codes mask the wire message	✅ ResponseErrorL honors Internal=true; no raw err reaches the client
Raw non-OK c.JSON remaining in the 8 files	✅ none

Non-blocking findings (P2 — recommend in-place fix, not required for merge)

These are pre-existing or observability-only issues surfaced while reviewing the migrated control flow. None changes functional or security behavior, so none blocks merge — but since the migration is already editing these exact lines, fixing them here is cheaper than a follow-up.

P2-1 — Transaction not rolled back on genMessageExtraSeq failure (modules/message/api_manager.go:166-170, function delete())
The early return when genMessageExtraSeq fails does not call tx.Rollback(), unlike the sibling branches in the same loop (updateMsgExtraVersionAndDeletedTx, line ~181) and after the loop (line ~190). The defer only rolls back inside recover() (panic path), so a normal early return here leaks the open transaction / connection. This predates the PR (the old c.ResponseError(err) branch had the same gap) and the endpoint is CheckLoginRoleIsSuperAdmin-gated, so exploitability is low — but it's a one-line fix worth folding into this safety-focused pass. (Same class as the friendSure/addUser leaks fixed in #197.)

P2-2 / P2-3 — Internal=true response sites missing the zap.Error log (modules/message/api_conversation.go:378 and :918)
insertDeviceOffsets / insertUserLastOffsets failures respond with ErrMessageStoreFailed (Internal=true, message masked on the wire — correct) but without a preceding co.Error("…", zap.Error(err)). The helper functions only log on tx-begin / commit failure, not on the per-row insertOrUpdateTx error, so a row-level DB error in this path becomes invisible to ops. This is exactly the invariant the file's own header comment states ("every such site keeps … an m.Error(…, zap.Error(err)) log so ops can debug from logs while the wire response carries no message"). Add the log at both call sites (matching e.g. api_conversation.go:1078).

Note on the wire-status semantics (not a finding)

pinnedMessage maps the "group missing/inactive" branch to ErrMessageGroupNotFound (semantic 404). Per the D14 design, ResponseErrorL always emits HTTP transport status 400; the 404 lives only in error.http_status, and the old code already returned the explicit "群不存在或已删除" message — so this is not a new enumeration signal. Consistent with the thread/user/group migrations. No action needed.

Security-sensitive sign-off

This PR was classified security_sensitive. The migration does not alter any authorization decision, transaction boundary, or visibility-masking logic — it only swaps the response envelope. The api_message_get.go 404 anti-enumeration behavior (the highest-risk area) is preserved and now carries the i18n envelope. For a human reviewer: the only things worth a manual glance are the two P2 observability gaps above; neither affects the trust boundary.

Approving. The P2 items are recommended in-place fixes but do not block merge.