Skip to content

docs: add i18n / error-localization conventions to CLAUDE.md#231

Open
an9xyz wants to merge 1 commit into
Mininglamp-OSS:mainfrom
dmwork-org:docs/i18n-conventions
Open

docs: add i18n / error-localization conventions to CLAUDE.md#231
an9xyz wants to merge 1 commit into
Mininglamp-OSS:mainfrom
dmwork-org:docs/i18n-conventions

Conversation

@an9xyz
Copy link
Copy Markdown
Contributor

@an9xyz an9xyz commented Jun 2, 2026

Part of #170 (backend i18n).

Summary

Docs-only. Captures the i18n / error-localization conventions in CLAUDE.md so future changes follow the playbook established by the migrated modules (#176/#188/#198/#203/#213/#214) and the recent OIDC (#223) and email (#224) work β€” instead of re-deriving it each time.

What's documented

  • Two facades (pkg/httperr): ResponseErrorL (fixed-400 / D14) vs ResponseErrorLWithStatus (real status, new endpoints only), with a comparison table.
  • Error codes (pkg/errcode/<module>.go): registration shape, the 5xx ⟺ Internal=true invariant, anti-enumeration (one generic code), and the Params/Details split.
  • Per-module helpers (modules/<module>/api_i18n.go) and mustLookupSharedCode.
  • The required gate: make i18n-extract / i18n-extract-check / i18n-lint, plus zh-CN translations in active.zh-CN.toml.
  • Source guard: Test<Module>NoLegacyResponseError and the baseline.txt exemption for protocol endpoints.
  • Emails: emailtmpl localized templates + i18n.OutboundLanguage(ctx).

Also adds pkg/httperr / pkg/i18n to the Key Packages table and an i18n line to Coding Conventions.

Test plan

Documents the i18n error-envelope rules so future changes follow the
established playbook:
- ResponseErrorL (fixed-400, D14) vs ResponseErrorLWithStatus (real status,
  new endpoints only) facades
- pkg/errcode registration, 5xx<=>Internal invariant, anti-enumeration,
  Params/Details split
- per-module api_i18n.go helpers, the make i18n-extract-check / i18n-lint
  gate, zh-CN translations, and the NoLegacyResponseError source guard
- emailtmpl localized templates + i18n.OutboundLanguage

Adds pkg/httperr and pkg/i18n to Key Packages and an i18n line to Coding
Conventions.
@an9xyz an9xyz requested a review from a team as a code owner June 2, 2026 14:46
@github-actions github-actions Bot added the size/S PR size: S label Jun 2, 2026
Copy link
Copy Markdown
Contributor

@Jerry-Xin Jerry-Xin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

βœ… APPROVE

Comprehensive i18n / error-localization documentation for CLAUDE.md.

  • ResponseErrorL vs ResponseErrorLWithStatus distinction (D14 compat vs real status) is clearly explained with the maintainer sign-off caveat
  • Error code registration recipe is complete: ID naming, HTTPStatus, DefaultMessage, SafeDetailKeys, Internal flag
  • 5xx ⟺ Internal=true invariant and anti-enumeration guidance are important security conventions, well documented
  • Params vs Details (D15) distinction is precise
  • CI enforcement chain (i18n-extract β†’ i18n-extract-check β†’ i18n-lint) gives clear action steps
  • Guard test and baseline exemption pattern documented
  • Email template localization section ties into the broader i18n story
  • Architecture checklist updated with the i18n cross-reference

No issues found. Clean documentation.

Copy link
Copy Markdown
Contributor

@lml2468 lml2468 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Summary

Docs-only: adds i18n / error-localization conventions to CLAUDE.md. Comprehensive, accurate, well-structured. No blocking issues.

Covers:

  • Two-facade distinction (ResponseErrorL vs ResponseErrorLWithStatus) with correct usage guidance
  • Error code registration pattern with all key fields (ID, HTTPStatus, DefaultMessage, SafeDetailKeys, Internal)
  • 5xx ⟺ Internal=true invariant
  • Anti-enumeration principle (single generic 401 for auth failures)
  • Params vs Details (D15) distinction
  • Per-module helper pattern (api_i18n.go)
  • CI enforcement commands (i18n-extract, i18n-extract-check, i18n-lint)
  • Guard test convention
  • Email template localization
  • Checklist addition reinforcing the rules

APPROVED.

Copy link
Copy Markdown
Contributor

@yujiawei yujiawei left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code Review β€” PR #231 (octo-server)

Scope: Documentation-only change to CLAUDE.md (+44/-4, single file) adding i18n / error-localization conventions. No production code is touched.

Verdict: APPROVED. I verified every concrete claim in the new documentation against the codebase at the PR head SHA. All structural claims (package paths, function signatures, struct fields, make targets, guard tests, email templates) are accurate. A few minor imprecisions are noted below as non-blocking nits.

What was verified βœ…

  • Package map β€” pkg/errcode/oidc.go, pkg/httperr/ (ResponseErrorL / ResponseErrorLWithStatus in respond.go:22,41), and pkg/i18n/ (codes registry, localizer, renderer, language negotiation, locales/) all exist as described.
  • Two-facade semantics β€” ResponseErrorL pins the wire status to 400 with the real status in error.http_status (respond.go:60-69, tested in respond_test.go:15-56); ResponseErrorLWithStatus emits the code's real HTTPStatus (respond.go:61-62, tested respond_test.go:93-120). The "currently just modules/oidc bind" claim holds β€” grep confirms the only caller is modules/oidc/api_i18n.go:40 (respondBindError).
  • Code registration shape β€” codes.Code has ID, HTTPStatus, DefaultMessage, SafeDetailKeys, Internal (registry.go:60-67); ID convention enforced by regex ^err\.(shared|server)\....$ (registry.go:43); err.shared.* codes (auth/rate/param/internal/not_found) exist in codes/shared.go.
  • Invariants β€” 5xx ⟺ Internal=true is enforced by shared_test.go:59-75 and applied by the renderer (renderer.go:69-70,85-86 hide message + details). SafeDetailKeys whitelisting is implemented in details.go:21-44. Params-vs-details (D15) split is enforced via distinct types (params.go, details.go).
  • Tooling β€” make i18n-extract / i18n-extract-check / i18n-lint targets exist in the Makefile and back the described behavior (pkg/i18n/cmd/octo-i18n-extract, tools/lint-direct-error-response, tools/lint-unregistered-code). Per-module api_i18n.go helpers + mustLookupSharedCode confirmed; Test<Module>NoLegacyResponseError guards and tools/lint-direct-error-response/baseline.txt exemptions confirmed.
  • Emails β€” localized templates under modules/base/common/emailtmpl/templates/{en-US,zh-CN}/ with subject/html/text via go:embed; send functions take a lang arg resolved via i18n.OutboundLanguage(ctx) (service_email.go:28-30, used in modules/user/api_emaillogin.go).

Non-blocking nits (P2)

  1. Code struct has an undocumented field. The actual struct (pkg/i18n/codes/registry.go:60-67) also has DefaultMessages map[string]string (extreme-fallback translations), not shown in the doc's example. It's optional and usually omitted, so the example is fine as-is β€” optionally add a one-line note that the field exists.
  2. Anti-enumeration wording is slightly broad. The doc says auth/verify failures "map to ONE generic code (e.g. a single 401)". In practice codes/shared.go intentionally has several auth codes for different scenarios (auth.required, token_missing, token_invalid, token_expired, forbidden). The single-code rule correctly applies to credential-verification endpoints (e.g. ErrUserInvalidCredentials, ErrOIDCBindInvalidCredentials), where multiple failure reasons collapse to one code. Consider clarifying "for a single login/verify endpoint, never per-reason" to avoid a reader concluding only one auth code may exist globally.

Neither nit affects correctness, build, or security, and neither blocks merge. Good, accurate documentation that matches the implemented i18n system.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size/S PR size: S

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants