From e785318814c9157a3523eb70ba128dc72266df75 Mon Sep 17 00:00:00 2001 From: Menglin Li Date: Thu, 28 May 2026 10:08:52 +0800 Subject: [PATCH 1/6] chore(ci): add edited trigger to check-sprint and auto-add workflows (#177) This PR adds the `edited` event to the `pull_request_target.types` trigger in both `check-sprint.yml` and `auto-add-to-project.yml`, enabling the Issue-first Sprint check flow to work correctly when a developer adds a `Closes #` reference to a PR description after opening. Co-authored-by: Octo Bot --- .github/workflows/auto-add-to-project.yml | 2 +- .github/workflows/check-sprint.yml | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/.github/workflows/auto-add-to-project.yml b/.github/workflows/auto-add-to-project.yml index 73687b3a..d3f6bc9a 100644 --- a/.github/workflows/auto-add-to-project.yml +++ b/.github/workflows/auto-add-to-project.yml @@ -5,7 +5,7 @@ on: issues: types: [opened] pull_request_target: - types: [opened] + types: [opened, edited] permissions: {} diff --git a/.github/workflows/check-sprint.yml b/.github/workflows/check-sprint.yml index c1e9e383..6a36a036 100644 --- a/.github/workflows/check-sprint.yml +++ b/.github/workflows/check-sprint.yml @@ -33,7 +33,7 @@ name: Check Sprint on: pull_request_target: - types: [opened, synchronize, reopened] + types: [opened, synchronize, reopened, edited] branches: [main] permissions: {} From 63baf02df7d61e56c05afffa4a30282518f5391d Mon Sep 17 00:00:00 2001 From: ploy-elison Date: Thu, 28 May 2026 14:02:41 +0800 Subject: [PATCH 2/6] feat(voice): add POST /local-config/reset proxy + fix 404 handling (#184) Two changes for local ASR config reset flow: 1. **Add POST /local-config/reset proxy endpoint** - proxies reset request to octo-speech service, preserving enabled state while resetting other fields to defaults. 2. **Fix 404 handling** - when octo-speech returns 404 (config not found), voice adapter now returns 200 with empty/default response instead of propagating the error, since 'not found' means 'using defaults'. Closes #185 --- modules/voice_adapter/adapter.go | 71 +++++- modules/voice_adapter/adapter_test.go | 308 ++++++++++++++++++++++++++ modules/voice_adapter/client.go | 33 +++ 3 files changed, 410 insertions(+), 2 deletions(-) diff --git a/modules/voice_adapter/adapter.go b/modules/voice_adapter/adapter.go index 0a35f9f5..405de471 100644 --- a/modules/voice_adapter/adapter.go +++ b/modules/voice_adapter/adapter.go @@ -1,6 +1,7 @@ package voice_adapter import ( + "encoding/json" "errors" "net/http" "os" @@ -39,6 +40,7 @@ func (a *VoiceAdapter) Route(r *wkhttp.WKHttp) { auth.PUT("/local-config", a.putLocalConfig) auth.GET("/local-config", a.getLocalConfig) auth.DELETE("/local-config", a.deleteLocalConfig) + auth.POST("/local-config/reset", a.resetLocalConfig) } } @@ -265,15 +267,35 @@ func (a *VoiceAdapter) getLocalConfig(c *wkhttp.Context) { c.JSON(http.StatusOK, resp) } -func (a *VoiceAdapter) deleteLocalConfig(c *wkhttp.Context) { +func (a *VoiceAdapter) resetLocalConfig(c *wkhttp.Context) { loginUID := c.GetLoginUID() + c.Request.Body = http.MaxBytesReader(c.Writer, c.Request.Body, 64*1024) + + var req struct { + Enabled *bool `json:"enabled"` + } + if err := c.ShouldBindJSON(&req); err != nil { + var maxErr *http.MaxBytesError + if errors.As(err, &maxErr) { + c.JSON(http.StatusRequestEntityTooLarge, gin.H{"status": http.StatusRequestEntityTooLarge, "msg": "request body too large"}) + return + } + c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "msg": "invalid request body"}) + return + } + spaceID := getSpaceID(c) if spaceID == "" { c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "msg": "X-Space-ID header is required"}) return } + if req.Enabled == nil { + c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "msg": "enabled is required"}) + return + } + isMember, err := space.CheckMembership(a.ctx.DB(), spaceID, loginUID) if err != nil { a.Error("check space membership failed", zap.Error(err)) @@ -285,13 +307,58 @@ func (a *VoiceAdapter) deleteLocalConfig(c *wkhttp.Context) { return } - err = a.client.DeleteLocalConfig(c.Request.Context(), loginUID, "space", spaceID) + err = a.client.ResetLocalConfig(c.Request.Context(), loginUID, "space", spaceID, *req.Enabled) if err != nil { var svcErr *SpeechServiceError if errors.As(err, &svcErr) && svcErr.StatusCode >= 400 && svcErr.StatusCode < 500 { c.JSON(svcErr.StatusCode, gin.H{"status": svcErr.StatusCode, "msg": svcErr.Body}) return } + a.Error("reset local config failed", zap.Error(err)) + c.JSON(http.StatusBadGateway, gin.H{"status": http.StatusBadGateway, "msg": "speech service unavailable"}) + return + } + c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "msg": "ok"}) +} + +func (a *VoiceAdapter) deleteLocalConfig(c *wkhttp.Context) { + loginUID := c.GetLoginUID() + + spaceID := getSpaceID(c) + if spaceID == "" { + c.JSON(http.StatusBadRequest, gin.H{"status": http.StatusBadRequest, "msg": "X-Space-ID header is required"}) + return + } + + isMember, err := space.CheckMembership(a.ctx.DB(), spaceID, loginUID) + if err != nil { + a.Error("check space membership failed", zap.Error(err)) + c.JSON(http.StatusInternalServerError, gin.H{"status": http.StatusInternalServerError, "msg": "check space membership failed"}) + return + } + if !isMember { + c.JSON(http.StatusForbidden, gin.H{"status": http.StatusForbidden, "msg": "not a member of this space"}) + return + } + + err = a.client.DeleteLocalConfig(c.Request.Context(), loginUID, "space", spaceID) + if err != nil { + var svcErr *SpeechServiceError + if errors.As(err, &svcErr) { + if svcErr.StatusCode == 404 { + var parsed struct { + Status int `json:"status"` + } + if json.Unmarshal([]byte(svcErr.Body), &parsed) == nil && parsed.Status == 404 { + c.JSON(http.StatusOK, gin.H{"status": http.StatusOK, "msg": "ok"}) + return + } + } + if svcErr.StatusCode >= 400 && svcErr.StatusCode < 500 { + c.JSON(svcErr.StatusCode, gin.H{"status": svcErr.StatusCode, "msg": svcErr.Body}) + return + } + } a.Error("delete local config failed", zap.Error(err)) c.JSON(http.StatusBadGateway, gin.H{"status": http.StatusBadGateway, "msg": "speech service unavailable"}) return diff --git a/modules/voice_adapter/adapter_test.go b/modules/voice_adapter/adapter_test.go index 3171be04..6d4dd8eb 100644 --- a/modules/voice_adapter/adapter_test.go +++ b/modules/voice_adapter/adapter_test.go @@ -939,6 +939,91 @@ func TestDeleteLocalConfigHandler_5xxReturnsBadGateway(t *testing.T) { } } +func TestDeleteLocalConfigHandler_Speech404ConfigNotFound(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"msg":"config not found","status":404}`)) + })) + defer srv.Close() + + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + a := newTestAdapterWithDB(srv.URL, ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + gc.Request = httptest.NewRequest(http.MethodDelete, "/v1/voice/local-config", nil) + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.deleteLocalConfig(wkCtx) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "ok" { + t.Errorf("expected msg='ok', got %v", body["msg"]) + } +} + +func TestDeleteLocalConfigHandler_Speech404InvalidJSON(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`not json at all`)) + })) + defer srv.Close() + + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + a := newTestAdapterWithDB(srv.URL, ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + gc.Request = httptest.NewRequest(http.MethodDelete, "/v1/voice/local-config", nil) + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.deleteLocalConfig(wkCtx) + + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d", rec.Code) + } +} + +func TestDeleteLocalConfigHandler_Speech404WrongStatus(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusNotFound) + w.Write([]byte(`{"msg":"error","status":500}`)) + })) + defer srv.Close() + + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + a := newTestAdapterWithDB(srv.URL, ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + gc.Request = httptest.NewRequest(http.MethodDelete, "/v1/voice/local-config", nil) + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.deleteLocalConfig(wkCtx) + + if rec.Code != http.StatusNotFound { + t.Fatalf("expected 404, got %d", rec.Code) + } +} + func TestGetConfig_NonMember(t *testing.T) { ctx, mock, cleanup := newTestContextWithMockDB(t) defer cleanup() @@ -1021,6 +1106,229 @@ func TestPutLocalConfig_BodyTooLarge(t *testing.T) { } } +func TestResetLocalConfig_Success(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + if r.Method != http.MethodPost { + t.Errorf("expected POST, got %s", r.Method) + } + if r.URL.Path != "/v1/speech/local-config/reset" { + t.Errorf("unexpected path: %s", r.URL.Path) + } + var body map[string]interface{} + if err := json.NewDecoder(r.Body).Decode(&body); err != nil { + t.Errorf("decode body: %v", err) + } + if body["subject_id"] != "user1" { + t.Errorf("expected subject_id=user1, got %v", body["subject_id"]) + } + if body["scope_type"] != "space" { + t.Errorf("expected scope_type=space, got %v", body["scope_type"]) + } + if body["scope_id"] != "space1" { + t.Errorf("expected scope_id=space1, got %v", body["scope_id"]) + } + if body["enabled"] != true { + t.Errorf("expected enabled=true, got %v", body["enabled"]) + } + w.WriteHeader(http.StatusOK) + w.Write([]byte(`{"status":200,"msg":"ok"}`)) + })) + defer srv.Close() + + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + a := newTestAdapterWithDB(srv.URL, ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + reqBody := `{"enabled":true}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(reqBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(wkCtx) + + if rec.Code != http.StatusOK { + t.Fatalf("expected 200, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "ok" { + t.Errorf("expected msg='ok', got %v", body["msg"]) + } +} + +func TestResetLocalConfig_MissingSpaceID(t *testing.T) { + a := newTestAdapter("http://unused") + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + reqBody := `{"enabled":true}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(reqBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Set("uid", "user1") + ctx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(ctx) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "X-Space-ID header is required" { + t.Errorf("expected msg='X-Space-ID header is required', got %v", body["msg"]) + } +} + +func TestResetLocalConfig_MissingEnabled(t *testing.T) { + ctx, _, cleanup := newTestContextWithMockDB(t) + defer cleanup() + + a := newTestAdapterWithDB("http://unused", ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + reqBody := `{}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(reqBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(wkCtx) + + if rec.Code != http.StatusBadRequest { + t.Fatalf("expected 400, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "enabled is required" { + t.Errorf("expected msg='enabled is required', got %v", body["msg"]) + } +} + +func TestResetLocalConfig_NotMember(t *testing.T) { + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(0)) + + a := newTestAdapterWithDB("http://unused", ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + reqBody := `{"enabled":true}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(reqBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(wkCtx) + + if rec.Code != http.StatusForbidden { + t.Fatalf("expected 403, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "not a member of this space" { + t.Errorf("expected msg='not a member of this space', got %v", body["msg"]) + } +} + +func TestResetLocalConfig_SpeechError(t *testing.T) { + srv := httptest.NewServer(http.HandlerFunc(func(w http.ResponseWriter, r *http.Request) { + w.WriteHeader(http.StatusInternalServerError) + w.Write([]byte("internal error")) + })) + defer srv.Close() + + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnRows(sqlmock.NewRows([]string{"count"}).AddRow(1)) + + a := newTestAdapterWithDB(srv.URL, ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + reqBody := `{"enabled":false}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(reqBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(wkCtx) + + if rec.Code != http.StatusBadGateway { + t.Fatalf("expected 502, got %d", rec.Code) + } +} + +func TestResetLocalConfig_BodyTooLarge(t *testing.T) { + ctx, _, cleanup := newTestContextWithMockDB(t) + defer cleanup() + + a := newTestAdapterWithDB("http://unused", ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + largeBody := `{"enabled":true,"extra":"` + strings.Repeat("a", 65*1024) + `"}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(largeBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(wkCtx) + + if rec.Code != http.StatusRequestEntityTooLarge { + t.Fatalf("expected 413, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "request body too large" { + t.Errorf("expected msg='request body too large', got %v", body["msg"]) + } +} + +func TestResetLocalConfig_MembershipDBError(t *testing.T) { + ctx, mock, cleanup := newTestContextWithMockDB(t) + defer cleanup() + mock.ExpectQuery("SELECT COUNT").WillReturnError(sqlmock.ErrCancelled) + + a := newTestAdapterWithDB("http://unused", ctx) + gin.SetMode(gin.TestMode) + rec := httptest.NewRecorder() + gc, _ := gin.CreateTestContext(rec) + reqBody := `{"enabled":true}` + gc.Request = httptest.NewRequest(http.MethodPost, "/v1/voice/local-config/reset", bytes.NewBufferString(reqBody)) + gc.Request.Header.Set("Content-Type", "application/json") + gc.Request.Header.Set("X-Space-ID", "space1") + gc.Set("uid", "user1") + wkCtx := &wkhttp.Context{Context: gc} + a.resetLocalConfig(wkCtx) + + if rec.Code != http.StatusInternalServerError { + t.Fatalf("expected 500, got %d", rec.Code) + } + var body map[string]interface{} + if err := json.Unmarshal(rec.Body.Bytes(), &body); err != nil { + t.Fatalf("decode response: %v", err) + } + if body["msg"] != "check space membership failed" { + t.Errorf("expected msg='check space membership failed', got %v", body["msg"]) + } +} + func TestGetContext_QuerySpaceIDIgnored(t *testing.T) { ctx, _, cleanup := newTestContextWithMockDB(t) defer cleanup() diff --git a/modules/voice_adapter/client.go b/modules/voice_adapter/client.go index 7f8005a8..56057a6e 100644 --- a/modules/voice_adapter/client.go +++ b/modules/voice_adapter/client.go @@ -263,6 +263,39 @@ func (c *SpeechClient) GetLocalConfig(ctx context.Context, subjectID, scopeType, return result, nil } +func (c *SpeechClient) ResetLocalConfig(ctx context.Context, subjectID, scopeType, scopeID string, enabled bool) error { + payload := map[string]interface{}{ + "subject_id": subjectID, + "scope_type": scopeType, + "scope_id": scopeID, + "enabled": enabled, + } + + body, err := json.Marshal(payload) + if err != nil { + return fmt.Errorf("marshal request: %w", err) + } + + req, err := http.NewRequestWithContext(ctx, http.MethodPost, c.baseURL+"/v1/speech/local-config/reset", bytes.NewReader(body)) + if err != nil { + return fmt.Errorf("create request: %w", err) + } + req.Header.Set("Authorization", "Bearer "+c.apiKey) + req.Header.Set("Content-Type", "application/json") + + resp, err := c.client.Do(req) + if err != nil { + return fmt.Errorf("do request: %w", err) + } + defer resp.Body.Close() + + if resp.StatusCode != http.StatusOK { + respBody, _ := io.ReadAll(resp.Body) + return &SpeechServiceError{StatusCode: resp.StatusCode, Body: string(respBody)} + } + return nil +} + func (c *SpeechClient) DeleteLocalConfig(ctx context.Context, subjectID, scopeType, scopeID string) error { params := url.Values{} params.Set("subject_id", subjectID) From e85cbd86904372669c9cd3af33304c4294acc1ad Mon Sep 17 00:00:00 2001 From: an9xyz Date: Thu, 28 May 2026 15:58:40 +0800 Subject: [PATCH 3/6] feat(i18n): AST extractor + marker pipeline (#186) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Summary Adds the source-of-truth marker generator that Phase 2 module rollouts depend on. Without 100% recall here, every Phase 2 PR has to either hand-author TOML keys (drift risk) or accept silent runtime `DefaultMessage` fallback (translation gap). This PR is pure tooling — no runtime code changes. ## What ships - **`pkg/i18n/cmd/octo-i18n-extract`** — Go AST extractor that walks `pkg/i18n/codes` and `pkg/errcode`, recognizing both `Register(Code{...})` and `register(codes.Code{...})` call shapes. Strict by design: - `ID` and `DefaultMessage` MUST be basic string literals; computed values are rejected at extraction time, not papered over. - Duplicate IDs are a hard error (across files and across roots). - Test files and dot/underscore-prefixed dirs are skipped. - **100% recall guarantee** — `main.go` side-effect imports `codes` + `errcode` and asserts the AST-discovered ID set equals `codes.All()` exactly. Missing-from-AST or extra-only-in-AST surfaces a sorted diff and a non-zero exit code, so a `Register` call that `init()` somehow skipped (build tag, dead branch) cannot pass silently. - **Marker routing by ID prefix** (architecture decision D18): - `err.shared.*` → `tools/i18nmarkers/shared/active.en-US.toml` - `err.server.*` / `msg.*` → `tools/i18nmarkers/server/active.en-US.toml` - Unknown prefixes are rejected so adding a third namespace forces an explicit routing decision plus a `codes/registry.go` `idPattern` update. - **Deterministic output** — sorted keys, `strconv`-quoted strings, fixed header. Re-running the extractor on an unchanged repo produces byte-identical files. `WriteMarkerFile` only writes when content differs; `-check` mode reports diffs via exit code 3 for CI use. - **Makefile entries**: - `make i18n-extract` — regenerate marker files - `make i18n-extract-check` — CI guard (no writes; exit 3 on diff) - `make i18n-merge` — optional convenience that shells out to the upstream `goi18n` CLI ## Tests Table-driven, all green under `-race`: - `Register` / `register` call detection across both shapes - Non-`Code` composite literals ignored - Non-string-literal fields rejected - Duplicate ID rejected - Test files skipped - Prefix routing including unknown-prefix error - `RenderTOML` stability + escape correctness - `WriteMarkerFile` idempotency (second write reports unchanged) ## Out of scope CI integration (the lint job that fails PRs which forget to re-run `extract`) lands in the §0.10 lint batch alongside the other 5 lint rules. This PR ships the extractor + Makefile entrypoints only. ## Test plan - [x] `go test -race ./pkg/i18n/cmd/octo-i18n-extract/` - [x] `go vet ./pkg/i18n/cmd/octo-i18n-extract/` - [x] `make i18n-extract` → writes 9 + 18 markers - [x] `make i18n-extract` (second run) → reports unchanged - [x] `make i18n-extract-check` → exit 0 on clean tree - [x] `go test ./pkg/i18n/... ./pkg/errcode/...` (no regressions) --------- Co-authored-by: an9xyz --- Makefile | 46 +++- pkg/i18n/cmd/octo-i18n-extract/extract.go | 220 +++++++++++++++++ .../cmd/octo-i18n-extract/extract_test.go | 223 ++++++++++++++++++ pkg/i18n/cmd/octo-i18n-extract/main.go | 219 +++++++++++++++++ pkg/i18n/cmd/octo-i18n-extract/main_test.go | 167 +++++++++++++ pkg/i18n/cmd/octo-i18n-extract/write.go | 112 +++++++++ pkg/i18n/cmd/octo-i18n-extract/write_test.go | 97 ++++++++ tools/i18nmarkers/server/active.en-US.toml | 61 +++++ tools/i18nmarkers/shared/active.en-US.toml | 34 +++ 9 files changed, 1178 insertions(+), 1 deletion(-) create mode 100644 pkg/i18n/cmd/octo-i18n-extract/extract.go create mode 100644 pkg/i18n/cmd/octo-i18n-extract/extract_test.go create mode 100644 pkg/i18n/cmd/octo-i18n-extract/main.go create mode 100644 pkg/i18n/cmd/octo-i18n-extract/main_test.go create mode 100644 pkg/i18n/cmd/octo-i18n-extract/write.go create mode 100644 pkg/i18n/cmd/octo-i18n-extract/write_test.go create mode 100644 tools/i18nmarkers/server/active.en-US.toml create mode 100644 tools/i18nmarkers/shared/active.en-US.toml diff --git a/Makefile b/Makefile index 4e4734ff..b2eefeaa 100644 --- a/Makefile +++ b/Makefile @@ -23,4 +23,48 @@ stop-dev: echo " https://github.com/Mininglamp-OSS/octo-deployment"; \ exit 1 env-test: - docker-compose -f ./testenv/docker-compose.yaml up -d \ No newline at end of file + docker-compose -f ./testenv/docker-compose.yaml up -d + +# ---- i18n message marker pipeline (TODOS §0.8 / D18) ---------------------- +# +# i18n-extract : regenerate tools/i18nmarkers/{shared,server}/active.en-US.toml +# from codes.Register / errcode.register AST call sites. +# i18n-extract-check : CI guard — fails (exit 3) when on-disk markers diverge +# from what extraction would emit. Wired up alongside +# the rest of the 0.10 lint suite. +# i18n-merge : optional convenience target that feeds BOTH generated marker +# files (shared + server) into the upstream `goi18n` CLI to +# produce translate..toml stubs for new keys. Requires: +# go install github.com/nicksnyder/go-i18n/v2/goi18n@v2.6.1 +# First-run side effect: goi18n rewrites active..toml +# into its canonical format (entries sorted by ID, content +# hashes added, top-of-file comments stripped). This is +# expected once translators adopt the goi18n workflow; hashes +# are how goi18n detects source drift requiring re-translation. + +.PHONY: i18n-extract i18n-extract-check i18n-merge + +i18n-extract: + go run ./pkg/i18n/cmd/octo-i18n-extract + +i18n-extract-check: + go run ./pkg/i18n/cmd/octo-i18n-extract -check + +i18n-merge: i18n-extract + @command -v goi18n >/dev/null 2>&1 || { \ + echo "goi18n not on PATH — install with:"; \ + echo " go install github.com/nicksnyder/go-i18n/v2/goi18n@v2.6.1"; \ + exit 1; \ + } + # CRITICAL: feed BOTH shared and server marker files to goi18n as source + # inputs. With `-sourceLanguage en-US`, goi18n treats the union of source + # files as the canonical message set and rewrites any existing translation + # file (active.zh-CN.toml) to that set — entries not present in the source + # inputs are removed. Omitting the server marker file destructively wipes + # every err.server.* zh-CN translation from active.zh-CN.toml (verified + # against goi18n@v2.6.1 by PR #186 reviewers; preserving server + # translations is the contract this target must hold). + goi18n merge -sourceLanguage en-US -outdir pkg/i18n/locales \ + tools/i18nmarkers/shared/active.en-US.toml \ + tools/i18nmarkers/server/active.en-US.toml \ + pkg/i18n/locales/active.zh-CN.toml diff --git a/pkg/i18n/cmd/octo-i18n-extract/extract.go b/pkg/i18n/cmd/octo-i18n-extract/extract.go new file mode 100644 index 00000000..65a67683 --- /dev/null +++ b/pkg/i18n/cmd/octo-i18n-extract/extract.go @@ -0,0 +1,220 @@ +package main + +import ( + "fmt" + "go/ast" + "go/parser" + "go/token" + "os" + "path/filepath" + "sort" + "strconv" + "strings" +) + +// Marker is the minimal go-i18n message marker emitted per registered Code. +// We only carry ID + DefaultMessage because the AST extractor is strictly a +// source-of-truth dump for translators (D18); HTTPStatus / SafeDetailKeys / +// Internal don't belong in the runtime TOML. +type Marker struct { + // ID is the stable i18n key (e.g. "err.shared.auth.required"). + ID string + // DefaultMessage is the en-US source text the marker exposes as `other`. + DefaultMessage string + // Pos is the file:line where the Register call was found; used purely + // for human-readable error messages on duplicate / non-literal failures. + Pos string +} + +// recognizedFuncNames is the closed set of call expressions the extractor +// treats as a Code registration. Both shapes appear in-repo today: +// +// - pkg/i18n/codes/shared.go uses package-local `Register(Code{...})` +// - pkg/errcode/server.go uses a wrapper `register(codes.Code{...})` +// +// Adding a new wrapper requires updating this set AND adding a test fixture; +// silently accepting any *register* name would let typos pass through. +var recognizedFuncNames = map[string]struct{}{ + "Register": {}, + "register": {}, +} + +// ExtractFromDir walks `root` and extracts a Marker for every Code{...} +// composite literal passed to one of recognizedFuncNames. +// +// Strictness rationale (these conditions fail the run, not warn): +// - ID / DefaultMessage MUST be basic string literals; computed values +// defeat the static guarantee that markers match codes.Register input. +// - Duplicate IDs MUST fail; downstream goi18n merge produces undefined +// output on collisions. +// +// Test files (`*_test.go`) and directories whose name begins with `.` or `_` +// are skipped (the latter mirrors `go build`'s convention so test fixtures +// under `_fixtures/` don't pollute the marker set). +func ExtractFromDir(root string) ([]Marker, error) { + fset := token.NewFileSet() + var markers []Marker + + walkErr := filepath.Walk(root, func(path string, info os.FileInfo, err error) error { + if err != nil { + return err + } + if info.IsDir() { + name := info.Name() + if path != root && (strings.HasPrefix(name, ".") || strings.HasPrefix(name, "_")) { + return filepath.SkipDir + } + return nil + } + if !strings.HasSuffix(path, ".go") || strings.HasSuffix(path, "_test.go") { + return nil + } + file, err := parser.ParseFile(fset, path, nil, parser.SkipObjectResolution) + if err != nil { + return fmt.Errorf("parse %s: %w", path, err) + } + fileMarkers, err := extractFromFile(fset, file) + if err != nil { + return err + } + markers = append(markers, fileMarkers...) + return nil + }) + if walkErr != nil { + return nil, walkErr + } + + if err := checkDuplicates(markers); err != nil { + return nil, err + } + + sort.SliceStable(markers, func(i, j int) bool { return markers[i].ID < markers[j].ID }) + return markers, nil +} + +func extractFromFile(fset *token.FileSet, file *ast.File) ([]Marker, error) { + var ( + out []Marker + ferr error + ) + ast.Inspect(file, func(n ast.Node) bool { + // Once ferr is set we stop creating new state, but ast.Inspect's + // `return false` only skips the current subtree — siblings still + // trigger the visitor, which re-checks ferr and short-circuits. Net + // effect: first error wins, no further work happens, traversal walks + // the rest of the file harmlessly. Acceptable for tool-binary use. + if ferr != nil { + return false + } + call, ok := n.(*ast.CallExpr) + if !ok { + return true + } + if !isRecognizedRegisterCall(call) || len(call.Args) != 1 { + return true + } + lit, ok := call.Args[0].(*ast.CompositeLit) + if !ok || !isCodeType(lit.Type) { + return true + } + id, msg, err := readCodeLiteral(lit) + if err != nil { + ferr = fmt.Errorf("%s: %w", fset.Position(call.Pos()), err) + return false + } + // Missing ID or DefaultMessage is a hard error, not a silent skip. + // Register() also panics at runtime, but relying on that means a + // typo'd Code literal disappears from the marker set and surfaces + // only as a recall-check mismatch in main.go with no source + // position. Failing here matches the rest of this file's strictness + // rationale and gives reviewers an actionable file:line. + if id == "" || msg == "" { + ferr = fmt.Errorf("%s: Code literal is missing ID or DefaultMessage; both are required for a marker", fset.Position(call.Pos())) + return false + } + out = append(out, Marker{ + ID: id, + DefaultMessage: msg, + Pos: fset.Position(call.Pos()).String(), + }) + return true + }) + return out, ferr +} + +func isRecognizedRegisterCall(call *ast.CallExpr) bool { + switch fn := call.Fun.(type) { + case *ast.Ident: + _, ok := recognizedFuncNames[fn.Name] + return ok + case *ast.SelectorExpr: + _, ok := recognizedFuncNames[fn.Sel.Name] + return ok + } + return false +} + +// isCodeType returns true when the composite literal's type is `Code` or a +// selector ending in `.Code`. Anything else (e.g. `Other{}` inside a fake +// Register call) is ignored — false positives would leak unrelated structs +// into the marker output. +func isCodeType(t ast.Expr) bool { + switch tt := t.(type) { + case *ast.Ident: + return tt.Name == "Code" + case *ast.SelectorExpr: + return tt.Sel.Name == "Code" + } + return false +} + +func readCodeLiteral(lit *ast.CompositeLit) (id, msg string, err error) { + for _, elt := range lit.Elts { + kv, ok := elt.(*ast.KeyValueExpr) + if !ok { + continue + } + key, ok := kv.Key.(*ast.Ident) + if !ok { + continue + } + switch key.Name { + case "ID": + s, ok := basicStringLit(kv.Value) + if !ok { + return "", "", fmt.Errorf("Code.ID must be a string literal") + } + id = s + case "DefaultMessage": + s, ok := basicStringLit(kv.Value) + if !ok { + return "", "", fmt.Errorf("Code.DefaultMessage must be a string literal (id=%q)", id) + } + msg = s + } + } + return id, msg, nil +} + +func basicStringLit(e ast.Expr) (string, bool) { + lit, ok := e.(*ast.BasicLit) + if !ok || lit.Kind != token.STRING { + return "", false + } + s, err := strconv.Unquote(lit.Value) + if err != nil { + return "", false + } + return s, true +} + +func checkDuplicates(ms []Marker) error { + seen := make(map[string]string, len(ms)) + for _, m := range ms { + if prev, ok := seen[m.ID]; ok { + return fmt.Errorf("duplicate Code.ID %q registered at %s and %s", m.ID, prev, m.Pos) + } + seen[m.ID] = m.Pos + } + return nil +} diff --git a/pkg/i18n/cmd/octo-i18n-extract/extract_test.go b/pkg/i18n/cmd/octo-i18n-extract/extract_test.go new file mode 100644 index 00000000..525d376b --- /dev/null +++ b/pkg/i18n/cmd/octo-i18n-extract/extract_test.go @@ -0,0 +1,223 @@ +package main + +import ( + "os" + "path/filepath" + "sort" + "testing" +) + +// writeFile is a tiny helper that creates a fake Go source under a temp dir +// and returns its path. Tests use this to build representative fixtures of +// the codes.Register / errcode.register call shapes that ExtractFromDir must +// recognize without compiling the fixtures. +func writeFile(t *testing.T, dir, name, body string) string { + t.Helper() + p := filepath.Join(dir, name) + if err := os.WriteFile(p, []byte(body), 0o644); err != nil { + t.Fatalf("write %s: %v", p, err) + } + return p +} + +func TestExtractFromDir_RegisterPatterns(t *testing.T) { + cases := []struct { + name string + src string + want []Marker + }{ + { + name: "codes_Register direct", + src: `package fake +import "net/http" +func _() { + Register(Code{ + ID: "err.shared.auth.required", + HTTPStatus: http.StatusUnauthorized, + DefaultMessage: "Please log in to continue.", + }) +}`, + want: []Marker{{ID: "err.shared.auth.required", DefaultMessage: "Please log in to continue."}}, + }, + { + name: "errcode_register wrapper + codes.Code", + src: `package fake +import ( + "net/http" + "github.com/Mininglamp-OSS/octo-server/pkg/i18n/codes" +) +var ErrX = register(codes.Code{ + ID: "err.server.thread.not_found", + HTTPStatus: http.StatusNotFound, + DefaultMessage: "Thread not found.", +})`, + want: []Marker{{ID: "err.server.thread.not_found", DefaultMessage: "Thread not found."}}, + }, + { + name: "multiple in one file", + src: `package fake +func _() { + Register(Code{ID: "err.shared.a", DefaultMessage: "A"}) + Register(Code{ID: "err.shared.b", DefaultMessage: "B"}) +}`, + want: []Marker{ + {ID: "err.shared.a", DefaultMessage: "A"}, + {ID: "err.shared.b", DefaultMessage: "B"}, + }, + }, + { + name: "ignores non-Code composite literals", + src: `package fake +type Other struct{ ID, DefaultMessage string } +func _() { + Register(Other{ID: "ignored", DefaultMessage: "x"}) +}`, + want: nil, + }, + { + name: "ignores Register call without composite literal", + src: `package fake +func _() { + var c Code + Register(c) +}`, + want: nil, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, "fake.go", tc.src) + got, err := ExtractFromDir(dir) + if err != nil { + t.Fatalf("ExtractFromDir: %v", err) + } + sort.Slice(got, func(i, j int) bool { return got[i].ID < got[j].ID }) + sort.Slice(tc.want, func(i, j int) bool { return tc.want[i].ID < tc.want[j].ID }) + if len(got) != len(tc.want) { + t.Fatalf("len got=%d want=%d (got=%v)", len(got), len(tc.want), got) + } + for i := range got { + if got[i].ID != tc.want[i].ID || got[i].DefaultMessage != tc.want[i].DefaultMessage { + t.Errorf("[%d] got=%+v want=%+v", i, got[i], tc.want[i]) + } + } + }) + } +} + +func TestExtractFromDir_RejectsNonStringLiterals(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, "bad.go", `package fake +const msg = "hi" +func _() { + Register(Code{ID: "err.shared.x", DefaultMessage: msg}) +}`) + _, err := ExtractFromDir(dir) + if err == nil { + t.Fatalf("expected error for non-literal DefaultMessage, got nil") + } +} + +func TestExtractFromDir_RejectsDuplicateID(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, "dup.go", `package fake +func _() { + Register(Code{ID: "err.shared.dup", DefaultMessage: "a"}) + Register(Code{ID: "err.shared.dup", DefaultMessage: "b"}) +}`) + _, err := ExtractFromDir(dir) + if err == nil { + t.Fatalf("expected error for duplicate ID, got nil") + } +} + +func TestExtractFromDir_SkipsTestFiles(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, "x_test.go", `package fake +func _() { + Register(Code{ID: "err.shared.testonly", DefaultMessage: "T"}) +}`) + got, err := ExtractFromDir(dir) + if err != nil { + t.Fatalf("ExtractFromDir: %v", err) + } + if len(got) != 0 { + t.Fatalf("expected test file to be skipped, got %v", got) + } +} + +// TestExtractFromDir_RejectsEmptyFields locks the strict-fail upgrade: a +// Code literal missing ID or DefaultMessage is a typo, not a deliberate +// skip signal, and must surface with a file:line position rather than +// silently disappearing into a recall-check mismatch later. +func TestExtractFromDir_RejectsEmptyFields(t *testing.T) { + cases := []struct { + name string + src string + }{ + { + name: "empty_id", + src: `package fake +func _() { + Register(Code{ID: "", DefaultMessage: "x"}) +}`, + }, + { + name: "empty_default_message", + src: `package fake +func _() { + Register(Code{ID: "err.shared.x", DefaultMessage: ""}) +}`, + }, + { + name: "id_only", + src: `package fake +func _() { + Register(Code{ID: "err.shared.x"}) +}`, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + dir := t.TempDir() + writeFile(t, dir, "bad.go", tc.src) + _, err := ExtractFromDir(dir) + if err == nil { + t.Fatal("expected strict-fail on empty field, got nil") + } + }) + } +} + +// TestExtractFromDir_SkipsHiddenAndUnderscoreDirs codifies the convention +// that test fixtures dropped under `.hidden/` or `_fixtures/` are off-limits +// to extraction (matching `go build`'s dir-skip behavior). Without this +// fixture the convention is only described in a comment and could regress. +func TestExtractFromDir_SkipsHiddenAndUnderscoreDirs(t *testing.T) { + dir := t.TempDir() + // Real source under root — should be extracted. + writeFile(t, dir, "real.go", `package fake +func _() { + Register(Code{ID: "err.shared.real", DefaultMessage: "R"}) +}`) + for _, sub := range []string{"_fixtures", ".hidden"} { + subDir := filepath.Join(dir, sub) + if err := os.MkdirAll(subDir, 0o755); err != nil { + t.Fatalf("mkdir %s: %v", subDir, err) + } + writeFile(t, subDir, "leak.go", `package fake +func _() { + Register(Code{ID: "err.shared.LEAK_`+sub+`", DefaultMessage: "leak"}) +}`) + } + got, err := ExtractFromDir(dir) + if err != nil { + t.Fatalf("ExtractFromDir: %v", err) + } + if len(got) != 1 || got[0].ID != "err.shared.real" { + t.Fatalf("expected only real.go to extract, got %v", got) + } +} diff --git a/pkg/i18n/cmd/octo-i18n-extract/main.go b/pkg/i18n/cmd/octo-i18n-extract/main.go new file mode 100644 index 00000000..c466b393 --- /dev/null +++ b/pkg/i18n/cmd/octo-i18n-extract/main.go @@ -0,0 +1,219 @@ +// Command octo-i18n-extract is the source-of-truth AST extractor for the +// i18n message marker pipeline (D18 in `i18n 最终方案.md` v7.1). +// +// What it does: +// +// 1. Walk one or more Go source roots (default: pkg/i18n/codes pkg/errcode) +// and parse every `Register(Code{...})` / `register(codes.Code{...})` +// composite literal to recover (ID, DefaultMessage) pairs. +// 2. Route markers by ID prefix: +// - err.shared.* → /active.en-US.toml +// - err.server.* / msg.* → /active.en-US.toml +// 3. Cross-check against the registered set obtained by importing the codes +// and errcode packages. If `len(AST markers) != len(codes.All())` or the +// ID sets differ, exit non-zero — this is the 100% recall guarantee that +// Phase 2 module PRs depend on (TODOS §0.8). +// 4. With `-check`, write nothing; instead report whether any marker file +// on disk diverges from what would be written (CI use). +// +// Exit codes: 0 (clean), 1 (extraction error), 2 (recall mismatch), +// 3 (-check found diff). +// +// Typical invocations: +// +// go run ./pkg/i18n/cmd/octo-i18n-extract # rewrite markers +// go run ./pkg/i18n/cmd/octo-i18n-extract -check # CI verification +package main + +import ( + "flag" + "fmt" + "os" + "sort" + "strings" + + "github.com/Mininglamp-OSS/octo-server/pkg/i18n/codes" + // Side-effect imports: these packages register their Codes in init(). + // Listing them explicitly is what makes codes.All() complete; do NOT + // rely on transitive imports — extractor is a tool binary and must + // pin its own truth set. + _ "github.com/Mininglamp-OSS/octo-server/pkg/errcode" +) + +const ( + exitOK = 0 + exitExtractError = 1 + exitRecallMismatch = 2 + exitCheckDiff = 3 +) + +func main() { + var ( + sharedDir string + serverDir string + roots stringsFlag + check bool + ) + flag.StringVar(&sharedDir, "shared-dir", "tools/i18nmarkers/shared", "output directory for err.shared.* markers") + flag.StringVar(&serverDir, "server-dir", "tools/i18nmarkers/server", "output directory for err.server.* / msg.* markers") + flag.Var(&roots, "root", "Go source root to scan (repeatable; default: pkg/i18n/codes, pkg/errcode)") + flag.BoolVar(&check, "check", false, "verify on-disk markers match expected; exit 3 on diff, write nothing") + flag.Parse() + + if len(roots) == 0 { + roots = stringsFlag{"pkg/i18n/codes", "pkg/errcode"} + } + + markers, err := collectMarkers(roots) + if err != nil { + fmt.Fprintf(os.Stderr, "extract: %v\n", err) + os.Exit(exitExtractError) + } + registered := make([]string, 0, len(codes.All())) + for _, c := range codes.All() { + registered = append(registered, c.ID) + } + if err := verifyRecall(markers, registered); err != nil { + fmt.Fprintf(os.Stderr, "recall: %v\n", err) + os.Exit(exitRecallMismatch) + } + + groups, err := GroupByPrefix(markers) + if err != nil { + fmt.Fprintf(os.Stderr, "group: %v\n", err) + os.Exit(exitExtractError) + } + + // Iterate in a fixed order so log output (and `-check` diff hints) is + // deterministic across runs — Go map iteration would otherwise swap the + // "wrote/unchanged ... → ..." lines, hurting log diffing. + targetOrder := []string{"shared", "server"} + targets := map[string]string{ + "shared": joinPath(sharedDir, "active.en-US.toml"), + "server": joinPath(serverDir, "active.en-US.toml"), + } + + exit := exitOK + for _, group := range targetOrder { + path := targets[group] + ms := groups[group] + if check { + diff, err := checkOnDisk(path, ms) + if err != nil { + fmt.Fprintf(os.Stderr, "check %s: %v\n", path, err) + os.Exit(exitExtractError) + } + if diff { + fmt.Fprintf(os.Stderr, "diff: %s is stale (re-run `make i18n-extract`)\n", path) + exit = exitCheckDiff + } + continue + } + changed, err := WriteMarkerFile(path, ms) + if err != nil { + fmt.Fprintf(os.Stderr, "write %s: %v\n", path, err) + os.Exit(exitExtractError) + } + if changed { + fmt.Printf("wrote %d markers → %s\n", len(ms), path) + } else { + fmt.Printf("unchanged %d markers → %s\n", len(ms), path) + } + } + os.Exit(exit) +} + +// collectMarkers walks each root in turn and merges markers, checking for +// duplicate IDs across roots (a Code registered in two packages is always a +// bug — `codes.Register` would panic at runtime, but during static extraction +// the panic never fires). +func collectMarkers(roots []string) ([]Marker, error) { + var all []Marker + seen := make(map[string]string) + for _, r := range roots { + ms, err := ExtractFromDir(r) + if err != nil { + return nil, fmt.Errorf("root %s: %w", r, err) + } + for _, m := range ms { + if prev, ok := seen[m.ID]; ok { + return nil, fmt.Errorf("duplicate Code.ID %q across roots: %s and %s", m.ID, prev, m.Pos) + } + seen[m.ID] = m.Pos + all = append(all, m) + } + } + sort.SliceStable(all, func(i, j int) bool { return all[i].ID < all[j].ID }) + return all, nil +} + +// verifyRecall is the 100% guarantee: the AST-extracted set must match the +// runtime-registered set exactly. Diffs surface as a sorted list of missing +// and extra IDs to make CI failures actionable. +// +// `registered` is passed in as a slice (rather than calling codes.All() +// directly) so this function is unit-testable with synthetic inputs — the +// recall guarantee is the load-bearing safety property of this binary and +// deserves direct coverage independent of the live registry's state. +func verifyRecall(markers []Marker, registered []string) error { + astIDs := make(map[string]struct{}, len(markers)) + for _, m := range markers { + astIDs[m.ID] = struct{}{} + } + regIDs := make(map[string]struct{}, len(registered)) + for _, id := range registered { + regIDs[id] = struct{}{} + } + + var missing, extra []string + for id := range regIDs { + if _, ok := astIDs[id]; !ok { + missing = append(missing, id) + } + } + for id := range astIDs { + if _, ok := regIDs[id]; !ok { + extra = append(extra, id) + } + } + if len(missing) == 0 && len(extra) == 0 { + return nil + } + sort.Strings(missing) + sort.Strings(extra) + return fmt.Errorf( + "AST vs codes.All() mismatch: ast=%d registered=%d\n missing-from-AST: %v\n extra-only-in-AST: %v", + len(astIDs), len(regIDs), missing, extra, + ) +} + +// checkOnDisk returns true when the file at path differs from the rendering +// of ms (or does not exist). It never writes. +func checkOnDisk(path string, ms []Marker) (bool, error) { + want := RenderTOML(ms) + got, err := os.ReadFile(path) + if err != nil { + if os.IsNotExist(err) { + return true, nil + } + return false, err + } + return string(got) != want, nil +} + +func joinPath(dir, name string) string { + if dir == "" { + return name + } + if strings.HasSuffix(dir, "/") { + return dir + name + } + return dir + "/" + name +} + +// stringsFlag is a tiny -repeatable flag implementation; standard library has +// no built-in for `flag.Var` slice-of-string accumulation. +type stringsFlag []string + +func (s *stringsFlag) String() string { return strings.Join(*s, ",") } +func (s *stringsFlag) Set(v string) error { *s = append(*s, v); return nil } diff --git a/pkg/i18n/cmd/octo-i18n-extract/main_test.go b/pkg/i18n/cmd/octo-i18n-extract/main_test.go new file mode 100644 index 00000000..b3ae0d50 --- /dev/null +++ b/pkg/i18n/cmd/octo-i18n-extract/main_test.go @@ -0,0 +1,167 @@ +package main + +// Unit coverage for the load-bearing safety properties in main.go that the +// initial PR only exercised indirectly via `make i18n-extract`. Reviewer +// feedback on PR #186 (yujiawei P2) called out that verifyRecall, +// checkOnDisk, and collectMarkers' cross-root duplicate path deserve +// direct tests because they are what makes the 100% recall guarantee +// enforceable rather than aspirational. + +import ( + "os" + "path/filepath" + "strings" + "testing" +) + +func TestVerifyRecall(t *testing.T) { + cases := []struct { + name string + markers []Marker + registered []string + wantErr bool + wantSubstr []string // each substring must appear in the error message + }{ + { + name: "both_empty", + markers: nil, + registered: nil, + wantErr: false, + }, + { + name: "exact_match", + markers: []Marker{{ID: "err.shared.a"}, {ID: "err.shared.b"}}, + registered: []string{"err.shared.b", "err.shared.a"}, + wantErr: false, + }, + { + name: "missing_from_ast", + markers: []Marker{{ID: "err.shared.a"}}, + registered: []string{"err.shared.a", "err.shared.b"}, + wantErr: true, + wantSubstr: []string{"missing-from-AST", "err.shared.b"}, + }, + { + name: "extra_in_ast", + markers: []Marker{{ID: "err.shared.a"}, {ID: "err.shared.z"}}, + registered: []string{"err.shared.a"}, + wantErr: true, + wantSubstr: []string{"extra-only-in-AST", "err.shared.z"}, + }, + { + name: "both_missing_and_extra", + markers: []Marker{{ID: "err.shared.a"}, {ID: "err.shared.z"}}, + registered: []string{"err.shared.a", "err.shared.b"}, + wantErr: true, + wantSubstr: []string{"missing-from-AST", "err.shared.b", "extra-only-in-AST", "err.shared.z"}, + }, + } + for _, tc := range cases { + tc := tc + t.Run(tc.name, func(t *testing.T) { + err := verifyRecall(tc.markers, tc.registered) + if (err != nil) != tc.wantErr { + t.Fatalf("verifyRecall err=%v wantErr=%v", err, tc.wantErr) + } + if err == nil { + return + } + for _, s := range tc.wantSubstr { + if !strings.Contains(err.Error(), s) { + t.Errorf("error message missing %q\n full: %s", s, err.Error()) + } + } + }) + } +} + +func TestCheckOnDisk(t *testing.T) { + dir := t.TempDir() + ms := []Marker{{ID: "err.shared.x", DefaultMessage: "X"}} + + // State 1: file absent → diff + absent := filepath.Join(dir, "absent.toml") + diff, err := checkOnDisk(absent, ms) + if err != nil { + t.Fatalf("checkOnDisk(absent): %v", err) + } + if !diff { + t.Error("absent file must report diff=true") + } + + // State 2: file matches → no diff + matching := filepath.Join(dir, "match.toml") + if err := os.WriteFile(matching, []byte(RenderTOML(ms)), 0o644); err != nil { + t.Fatalf("seed match file: %v", err) + } + diff, err = checkOnDisk(matching, ms) + if err != nil { + t.Fatalf("checkOnDisk(match): %v", err) + } + if diff { + t.Error("matching file must report diff=false") + } + + // State 3: file differs → diff + stale := filepath.Join(dir, "stale.toml") + if err := os.WriteFile(stale, []byte("# stale unrelated content\n"), 0o644); err != nil { + t.Fatalf("seed stale file: %v", err) + } + diff, err = checkOnDisk(stale, ms) + if err != nil { + t.Fatalf("checkOnDisk(stale): %v", err) + } + if !diff { + t.Error("stale file must report diff=true") + } +} + +func TestCollectMarkers_CrossRootDuplicateID(t *testing.T) { + root1 := t.TempDir() + root2 := t.TempDir() + writeFile(t, root1, "a.go", `package fake +func _() { + Register(Code{ID: "err.shared.dup", DefaultMessage: "from root1"}) +}`) + writeFile(t, root2, "b.go", `package fake +func _() { + Register(Code{ID: "err.shared.dup", DefaultMessage: "from root2"}) +}`) + + _, err := collectMarkers([]string{root1, root2}) + if err == nil { + t.Fatal("expected cross-root duplicate to fail, got nil") + } + // Error message must point at both occurrences so reviewers can locate them. + for _, want := range []string{"err.shared.dup", root1, root2} { + if !strings.Contains(err.Error(), want) { + t.Errorf("cross-root error missing %q\n full: %s", want, err.Error()) + } + } +} + +func TestCollectMarkers_MergesDistinctRoots(t *testing.T) { + root1 := t.TempDir() + root2 := t.TempDir() + writeFile(t, root1, "a.go", `package fake +func _() { + Register(Code{ID: "err.shared.a", DefaultMessage: "A"}) +}`) + writeFile(t, root2, "b.go", `package fake +func _() { + Register(Code{ID: "err.server.b", DefaultMessage: "B"}) +}`) + + ms, err := collectMarkers([]string{root1, root2}) + if err != nil { + t.Fatalf("collectMarkers: %v", err) + } + if len(ms) != 2 { + t.Fatalf("len=%d want 2", len(ms)) + } + // Sorted output is part of the contract — main.go relies on it for + // stable downstream rendering and recall diffs. + if ms[0].ID >= ms[1].ID { + t.Errorf("collectMarkers output not sorted: %v", ms) + } +} diff --git a/pkg/i18n/cmd/octo-i18n-extract/write.go b/pkg/i18n/cmd/octo-i18n-extract/write.go new file mode 100644 index 00000000..59e35e2f --- /dev/null +++ b/pkg/i18n/cmd/octo-i18n-extract/write.go @@ -0,0 +1,112 @@ +package main + +import ( + "bytes" + "errors" + "fmt" + "os" + "path/filepath" + "sort" + "strconv" + "strings" +) + +// Prefix-to-group routing (D18). err.shared.* lives in the SDK and follows +// pkg/i18n/locales/; everything else (err.server.* and the reserved msg.* +// namespace) is server-private and ships under the repo-root locales/. +// +// The mapping is closed: unknown prefixes are a hard error. That guarantees +// that introducing a third namespace forces an explicit routing decision +// (and a Code.ID regex update in pkg/i18n/codes/registry.go). +// +// NOTE: `msg.` is a Phase 2 placeholder. pkg/i18n/codes/registry.go's +// idPattern currently restricts IDs to `err.(shared|server).*`, so any +// attempt to Register a `msg.*` Code panics at startup and never reaches +// extraction. The routing entry is intentionally pre-wired so that when +// Phase 2 widens the registry regex, this table doesn't need a parallel +// edit. Any future maintainer touching one side must update both. +var prefixGroup = map[string]string{ + "err.shared.": "shared", + "err.server.": "server", + "msg.": "server", +} + +// GroupByPrefix routes markers into their target group ("shared" or "server"). +// Returns an error if any marker's ID does not match a known prefix. +func GroupByPrefix(ms []Marker) (map[string][]Marker, error) { + out := make(map[string][]Marker) + for _, m := range ms { + group, ok := groupFor(m.ID) + if !ok { + return nil, fmt.Errorf("marker %q: unrecognized ID prefix (expected one of err.shared./err.server./msg.)", m.ID) + } + out[group] = append(out[group], m) + } + for g := range out { + ms := out[g] + sort.SliceStable(ms, func(i, j int) bool { return ms[i].ID < ms[j].ID }) + out[g] = ms + } + return out, nil +} + +func groupFor(id string) (string, bool) { + for prefix, g := range prefixGroup { + if strings.HasPrefix(id, prefix) { + return g, true + } + } + return "", false +} + +// RenderTOML produces a deterministic go-i18n marker file. Format matches +// what `goi18n merge` expects as a source: one `["id"]` table per Code with +// a single `other = "..."` key, alphabetized by ID, LF line endings, trailing +// newline. Stable output is a precondition for the CI extract-diff check. +func RenderTOML(ms []Marker) string { + sorted := append([]Marker(nil), ms...) + sort.SliceStable(sorted, func(i, j int) bool { return sorted[i].ID < sorted[j].ID }) + + var buf bytes.Buffer + buf.WriteString(markerHeader) + for i, m := range sorted { + if i > 0 { + buf.WriteByte('\n') + } + // strconv.Quote handles \n, \t, embedded quotes and unicode safely; + // go-i18n's TOML parser accepts standard basic strings. + fmt.Fprintf(&buf, "[%s]\nother = %s\n", strconv.Quote(m.ID), strconv.Quote(m.DefaultMessage)) + } + return buf.String() +} + +const markerHeader = `# AUTO-GENERATED by pkg/i18n/cmd/octo-i18n-extract. DO NOT EDIT. +# +# Source-of-truth marker file: one entry per codes.Register / errcode.register +# call in the repo. Regenerate with ` + "`make i18n-extract`" + `; the merge into +# active..toml is performed by goi18n merge (see Makefile). +# +# CI rejects PRs that change codes.Register input without re-running extract. + +` + +// WriteMarkerFile writes content (RenderTOML output) to path, creating the +// parent directory if needed. Returns changed=true only when on-disk bytes +// differ from the new payload, enabling cheap idempotency checks in CI. +func WriteMarkerFile(path string, ms []Marker) (changed bool, err error) { + if err := os.MkdirAll(filepath.Dir(path), 0o755); err != nil { + return false, fmt.Errorf("mkdir %s: %w", filepath.Dir(path), err) + } + want := []byte(RenderTOML(ms)) + got, err := os.ReadFile(path) + if err != nil && !errors.Is(err, os.ErrNotExist) { + return false, fmt.Errorf("read %s: %w", path, err) + } + if bytes.Equal(got, want) { + return false, nil + } + if err := os.WriteFile(path, want, 0o644); err != nil { + return false, fmt.Errorf("write %s: %w", path, err) + } + return true, nil +} diff --git a/pkg/i18n/cmd/octo-i18n-extract/write_test.go b/pkg/i18n/cmd/octo-i18n-extract/write_test.go new file mode 100644 index 00000000..d926912b --- /dev/null +++ b/pkg/i18n/cmd/octo-i18n-extract/write_test.go @@ -0,0 +1,97 @@ +package main + +import ( + "os" + "path/filepath" + "testing" +) + +func TestGroupByPrefix(t *testing.T) { + in := []Marker{ + {ID: "err.shared.auth.required", DefaultMessage: "a"}, + {ID: "err.server.thread.not_found", DefaultMessage: "b"}, + {ID: "msg.notify.invite", DefaultMessage: "c"}, + {ID: "err.shared.internal", DefaultMessage: "d"}, + } + got, err := GroupByPrefix(in) + if err != nil { + t.Fatalf("GroupByPrefix: %v", err) + } + if n := len(got["shared"]); n != 2 { + t.Errorf("shared group n=%d want 2", n) + } + if n := len(got["server"]); n != 2 { + t.Errorf("server group n=%d want 2 (server + msg)", n) + } +} + +func TestGroupByPrefix_RejectsUnknown(t *testing.T) { + _, err := GroupByPrefix([]Marker{{ID: "weird.foo", DefaultMessage: "x"}}) + if err == nil { + t.Fatal("expected error for unknown prefix") + } +} + +func TestRenderTOML_StableAndIdempotent(t *testing.T) { + ms := []Marker{ + {ID: "err.shared.b", DefaultMessage: "B with \"quotes\""}, + {ID: "err.shared.a", DefaultMessage: "A\nnewline"}, + } + out1 := RenderTOML(ms) + out2 := RenderTOML(ms) + if out1 != out2 { + t.Fatalf("RenderTOML not deterministic:\n--1--\n%s\n--2--\n%s", out1, out2) + } + // Sorted: "a" must appear before "b". + if got := out1; !before(got, "err.shared.a", "err.shared.b") { + t.Fatalf("expected sorted output, got:\n%s", got) + } + // Escape sanity: literal newline / quote must not survive raw. + if contains(out1, "B with \"quotes\"") { + t.Errorf("quotes not escaped:\n%s", out1) + } +} + +func TestWriteMarkerFile_IdempotentNoChange(t *testing.T) { + dir := t.TempDir() + ms := []Marker{{ID: "err.shared.x", DefaultMessage: "X"}} + path := filepath.Join(dir, "active.en-US.toml") + changed, err := WriteMarkerFile(path, ms) + if err != nil { + t.Fatalf("first write: %v", err) + } + if !changed { + t.Errorf("first write should report changed=true") + } + changed, err = WriteMarkerFile(path, ms) + if err != nil { + t.Fatalf("second write: %v", err) + } + if changed { + t.Errorf("second write should report changed=false (idempotent)") + } + b, err := os.ReadFile(path) + if err != nil { + t.Fatalf("read: %v", err) + } + if len(b) == 0 { + t.Error("output file is empty") + } +} + +func before(s, a, b string) bool { + i := indexOf(s, a) + j := indexOf(s, b) + return i >= 0 && j >= 0 && i < j +} + +func indexOf(s, sub string) int { + for i := 0; i+len(sub) <= len(s); i++ { + if s[i:i+len(sub)] == sub { + return i + } + } + return -1 +} + +func contains(s, sub string) bool { return indexOf(s, sub) >= 0 } diff --git a/tools/i18nmarkers/server/active.en-US.toml b/tools/i18nmarkers/server/active.en-US.toml new file mode 100644 index 00000000..e0220bc8 --- /dev/null +++ b/tools/i18nmarkers/server/active.en-US.toml @@ -0,0 +1,61 @@ +# AUTO-GENERATED by pkg/i18n/cmd/octo-i18n-extract. DO NOT EDIT. +# +# Source-of-truth marker file: one entry per codes.Register / errcode.register +# call in the repo. Regenerate with `make i18n-extract`; the merge into +# active..toml is performed by goi18n merge (see Makefile). +# +# CI rejects PRs that change codes.Register input without re-running extract. + +["err.server.thread.creator_cannot_leave"] +other = "Thread creator cannot leave the thread." + +["err.server.thread.deleted"] +other = "Thread has been deleted." + +["err.server.thread.group_md_content_empty"] +other = "GROUP.md content must not be empty." + +["err.server.thread.group_md_content_too_large"] +other = "GROUP.md content exceeds the maximum size." + +["err.server.thread.group_md_not_found"] +other = "Thread GROUP.md not found." + +["err.server.thread.group_no_invalid"] +other = "Invalid group number." + +["err.server.thread.name_invalid"] +other = "Thread name is required and must not exceed 100 characters." + +["err.server.thread.not_active"] +other = "Thread is not active." + +["err.server.thread.not_found"] +other = "Thread not found." + +["err.server.thread.not_group_member"] +other = "You are not a group member." + +["err.server.thread.permission_denied"] +other = "You do not have permission to perform this action." + +["err.server.thread.request_invalid"] +other = "Invalid request." + +["err.server.thread.setting_invalid"] +other = "Invalid thread setting." + +["err.server.thread.short_id_invalid"] +other = "Invalid thread short ID." + +["err.server.thread.source_message_invalid"] +other = "Invalid source message payload." + +["err.server.thread.status_changed"] +other = "Thread status changed concurrently." + +["err.server.thread.status_invalid"] +other = "Invalid thread status." + +["err.server.thread.store_failed"] +other = "Thread storage operation failed." diff --git a/tools/i18nmarkers/shared/active.en-US.toml b/tools/i18nmarkers/shared/active.en-US.toml new file mode 100644 index 00000000..381b7f0c --- /dev/null +++ b/tools/i18nmarkers/shared/active.en-US.toml @@ -0,0 +1,34 @@ +# AUTO-GENERATED by pkg/i18n/cmd/octo-i18n-extract. DO NOT EDIT. +# +# Source-of-truth marker file: one entry per codes.Register / errcode.register +# call in the repo. Regenerate with `make i18n-extract`; the merge into +# active..toml is performed by goi18n merge (see Makefile). +# +# CI rejects PRs that change codes.Register input without re-running extract. + +["err.shared.auth.forbidden"] +other = "You do not have permission to perform this action." + +["err.shared.auth.required"] +other = "Please log in to continue." + +["err.shared.auth.token_expired"] +other = "Authentication token has expired." + +["err.shared.auth.token_invalid"] +other = "Authentication token is invalid." + +["err.shared.auth.token_missing"] +other = "Authentication token is required." + +["err.shared.internal"] +other = "Internal server error." + +["err.shared.not_found"] +other = "Resource not found." + +["err.shared.param.invalid"] +other = "Invalid request parameter." + +["err.shared.rate.limited"] +other = "Too many requests, please try again later." From 0e25af6dda648896eef5295b706f811c619b73cb Mon Sep 17 00:00:00 2001 From: Menglin Li Date: Thu, 28 May 2026 18:46:23 +0800 Subject: [PATCH 4/6] feat: add pr-review-feed and pr-result-notify caller workflows (#187) Add two new caller workflows per PR notification split design. Reusable workflows in .github PR #49. --------- Co-authored-by: Octo Bot --- .github/workflows/octo-pr-feed.yml | 27 ------------- .github/workflows/octo-pr-result-notify.yml | 43 +++++++++++++++++++++ .github/workflows/octo-pr-review-feed.yml | 22 +++++++++++ 3 files changed, 65 insertions(+), 27 deletions(-) delete mode 100644 .github/workflows/octo-pr-feed.yml create mode 100644 .github/workflows/octo-pr-result-notify.yml create mode 100644 .github/workflows/octo-pr-review-feed.yml diff --git a/.github/workflows/octo-pr-feed.yml b/.github/workflows/octo-pr-feed.yml deleted file mode 100644 index 29a31ef0..00000000 --- a/.github/workflows/octo-pr-feed.yml +++ /dev/null @@ -1,27 +0,0 @@ -name: Octo PR Feed - -on: - pull_request_target: - types: [opened, closed, reopened, ready_for_review, review_requested] - -permissions: {} - -jobs: - notify: - if: ${{ !github.event.pull_request.draft }} - uses: Mininglamp-OSS/.github/.github/workflows/octo-pr-feed.yml@main - with: - repo_name: ${{ github.event.repository.name }} - pr_number: ${{ github.event.pull_request.number }} - pr_title: ${{ github.event.pull_request.title }} - pr_url: ${{ github.event.pull_request.html_url }} - pr_author: ${{ github.event.pull_request.user.login }} - pr_merged: ${{ github.event.pull_request.merged }} - pr_additions: ${{ github.event.pull_request.additions }} - pr_deletions: ${{ github.event.pull_request.deletions }} - pr_changed_files: ${{ github.event.pull_request.changed_files }} - event_action: ${{ github.event.action }} - feed_group_id: "1c303c142e9840f2a9b46c10b0972e8d" - project_group_id: "9ea115c7462b4b45b8c85d07d07e0dde" - secrets: - OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }} diff --git a/.github/workflows/octo-pr-result-notify.yml b/.github/workflows/octo-pr-result-notify.yml new file mode 100644 index 00000000..9c2ee1ef --- /dev/null +++ b/.github/workflows/octo-pr-result-notify.yml @@ -0,0 +1,43 @@ +name: Octo PR Result Notify + +on: + pull_request_target: + types: [closed, reopened] + pull_request_review: + types: [submitted] + +permissions: {} + +jobs: + pr-result: + if: github.event_name == 'pull_request_target' + uses: Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main + with: + repo_name: ${{ github.event.repository.name }} + pr_number: ${{ github.event.pull_request.number }} + pr_title: ${{ github.event.pull_request.title }} + pr_url: ${{ github.event.pull_request.html_url }} + pr_author: ${{ github.event.pull_request.user.login }} + event_kind: ${{ github.event.action == 'closed' && github.event.pull_request.merged == true && 'pr_merged' || github.event.action == 'closed' && 'pr_closed' || 'pr_reopened' }} + pr_additions: ${{ github.event.pull_request.additions }} + pr_deletions: ${{ github.event.pull_request.deletions }} + pr_changed_files: ${{ github.event.pull_request.changed_files }} + secrets: + OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }} + + review-result: + if: >- + github.event_name == 'pull_request_review' && + (github.event.review.state == 'approved' || github.event.review.state == 'changes_requested') && + github.event.pull_request.head.repo.full_name == github.repository + uses: Mininglamp-OSS/.github/.github/workflows/octo-pr-result-notify.yml@main + with: + repo_name: ${{ github.event.repository.name }} + pr_number: ${{ github.event.pull_request.number }} + pr_title: ${{ github.event.pull_request.title }} + pr_url: ${{ github.event.pull_request.html_url }} + pr_author: ${{ github.event.pull_request.user.login }} + event_kind: ${{ github.event.review.state == 'approved' && 'review_approved' || 'review_changes_requested' }} + reviewer: ${{ github.event.review.user.login }} + secrets: + OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }} diff --git a/.github/workflows/octo-pr-review-feed.yml b/.github/workflows/octo-pr-review-feed.yml new file mode 100644 index 00000000..d3384d40 --- /dev/null +++ b/.github/workflows/octo-pr-review-feed.yml @@ -0,0 +1,22 @@ +name: Octo PR Review Feed + +on: + pull_request_target: + types: [opened, ready_for_review, review_requested, synchronize] + +permissions: {} + +jobs: + notify: + if: ${{ !github.event.pull_request.draft }} + uses: Mininglamp-OSS/.github/.github/workflows/octo-pr-review-feed.yml@main + with: + repo_name: ${{ github.event.repository.name }} + pr_number: ${{ github.event.pull_request.number }} + pr_title: ${{ github.event.pull_request.title }} + pr_url: ${{ github.event.pull_request.html_url }} + pr_author: ${{ github.event.pull_request.user.login }} + event_action: ${{ github.event.action }} + project_group_id: '9ea115c7462b4b45b8c85d07d07e0dde' + secrets: + OCTO_BOT_TOKEN: ${{ secrets.OCTO_BOT_TOKEN }} From 18cb24f3f1257f945d39294f2118ef6c7e8db729 Mon Sep 17 00:00:00 2001 From: an9xyz Date: Thu, 28 May 2026 19:36:44 +0800 Subject: [PATCH 5/6] feat(appconfig): admin-tunable disable_user_create_space via system_setting MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Promote the "禁用普通用户创建空间" toggle from env-only (DM_SPACE_DISABLE_USER_CREATE) to a system_setting KV-backed value so SuperAdmin can switch it at runtime without redeploying. - common: SpaceDisableUserCreate getter (DB → env → false), schema entry (space, disable_user_create, bool); appConfigResp.disable_user_create_space surfaced on both /v1/common/appconfig branches (incl. version short-circuit). - space: createSpace now consults SystemSettings; legacy env path preserved as fallback / low-level parser. - tests: unit (getter fallback chain incl. DB empty), HTTP (appconfig default / DB override / version short-circuit), createSpace 403 on DB-set, plus end-to-end (manager write → appconfig → createSpace gate). --- modules/common/api.go | 22 +++- modules/common/api_test.go | 65 +++++++++++ modules/common/system_setting_schema.go | 6 + modules/common/system_settings.go | 49 ++++++++ modules/common/system_settings_test.go | 68 +++++++++++ modules/space/api.go | 27 ++++- modules/space/api_test.go | 148 ++++++++++++++++++++++++ 7 files changed, 376 insertions(+), 9 deletions(-) diff --git a/modules/common/api.go b/modules/common/api.go index 4ffc1f79..ff36c76d 100644 --- a/modules/common/api.go +++ b/modules/common/api.go @@ -395,9 +395,10 @@ func (cn *Common) appConfig(c *wkhttp.Context) { } if versionI64 != 0 && int(versionI64) >= appConfigM.Version { c.JSON(http.StatusOK, &appConfigResp{ - Version: appConfigM.Version, - SystemBotUIDs: spacepkg.SystemBotList(), - LocalLoginOff: boolToFlag(cn.systemSettings.LocalLoginOff()), + Version: appConfigM.Version, + SystemBotUIDs: spacepkg.SystemBotList(), + LocalLoginOff: boolToFlag(cn.systemSettings.LocalLoginOff()), + DisableUserCreateSpace: boolToFlag(cn.systemSettings.SpaceDisableUserCreate()), }) return } @@ -433,8 +434,9 @@ func (cn *Common) appConfig(c *wkhttp.Context) { OIDCResetPasswordURL: oidcResetPasswordURL(), OIDCProviders: oidcProviders(), // YUJ-219-A / GH#1283：单一真源下发系统 Bot UID 列表，替代三端硬编码。 - SystemBotUIDs: spacepkg.SystemBotList(), - LocalLoginOff: boolToFlag(cn.systemSettings.LocalLoginOff()), + SystemBotUIDs: spacepkg.SystemBotList(), + LocalLoginOff: boolToFlag(cn.systemSettings.LocalLoginOff()), + DisableUserCreateSpace: boolToFlag(cn.systemSettings.SpaceDisableUserCreate()), }) } @@ -735,6 +737,16 @@ type appConfigResp struct { // 与 app_config.version 解耦：即使客户端命中 version 短路分支，也必须能拿到 // 最新值，否则 admin 切换开关后老客户端会被本地缓存住。和 SystemBotUIDs 同理。 LocalLoginOff int `json:"local_login_off"` + + // DisableUserCreateSpace 控制客户端是否隐藏「创建空间」入口。 + // 来源 system_setting space.disable_user_create,回退到 env + // DM_SPACE_DISABLE_USER_CREATE,默认 0(允许创建)。 + // + // 与 app_config.version 解耦的原因同 LocalLoginOff：admin 在管理台 toggle + // 后老客户端命中 version 短路分支仍必须看到最新值，否则被本地缓存住失去 + // 实时性。后端 POST /v1/space/create 也走同一个 getter 校验,客户端隐藏 + // 与服务端拒绝由单一真源驱动,不存在前后端漂移。 + DisableUserCreateSpace int `json:"disable_user_create_space"` } type oidcProviderResp struct { diff --git a/modules/common/api_test.go b/modules/common/api_test.go index d0b39cfa..6ed86ea1 100644 --- a/modules/common/api_test.go +++ b/modules/common/api_test.go @@ -119,6 +119,71 @@ func TestGetAppConfig_SystemBotUIDsOnVersionShortCircuit(t *testing.T) { assert.Contains(t, body, `"fileHelper"`) } +// appconfig 必须下发 disable_user_create_space：默认 0（缺 system_setting 行且 +// env 未设置）。客户端据此显示/隐藏「创建空间」入口；缺省必须保持开放。 +func TestGetAppConfig_DisableUserCreateSpace_DefaultsZero(t *testing.T) { + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "") + s, ctx := testutil.NewTestServer() + f := New(ctx) + err := testutil.CleanAllTables(ctx) + assert.NoError(t, err) + err = f.appConfigDB.insert(&appConfigModel{}) + assert.NoError(t, err) + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/v1/common/appconfig", nil) + req.Header.Set("token", testutil.Token) + s.GetRoute().ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Contains(t, w.Body.String(), `"disable_user_create_space":0`) +} + +// DB 写入 disable_user_create=1 时 appconfig 必须下发 1，admin 在管理台切换 +// 后客户端下次拉配置即可看到入口隐藏 —— 系统级 KV + Reload 路径的实时性保证。 +func TestGetAppConfig_DisableUserCreateSpace_DBOverride(t *testing.T) { + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "") + s, ctx := testutil.NewTestServer() + f := New(ctx) + err := testutil.CleanAllTables(ctx) + assert.NoError(t, err) + err = f.appConfigDB.insert(&appConfigModel{}) + assert.NoError(t, err) + + settings := EnsureSystemSettings(ctx) + assert.NoError(t, settings.db.upsert("space", "disable_user_create", "1", settingTypeBool, "")) + assert.NoError(t, settings.Reload()) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/v1/common/appconfig", nil) + req.Header.Set("token", testutil.Token) + s.GetRoute().ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Contains(t, w.Body.String(), `"disable_user_create_space":1`) +} + +// version 短路分支同样要下发 disable_user_create_space：老客户端命中版本 +// 短路也必须看到当前开关，否则被缓存住失去"实时调整"的能力（与 LocalLoginOff +// 同样的"system_setting 与 app_config.version 解耦"约束）。 +func TestGetAppConfig_DisableUserCreateSpace_OnVersionShortCircuit(t *testing.T) { + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "") + s, ctx := testutil.NewTestServer() + f := New(ctx) + err := testutil.CleanAllTables(ctx) + assert.NoError(t, err) + err = f.appConfigDB.insert(&appConfigModel{}) + assert.NoError(t, err) + + settings := EnsureSystemSettings(ctx) + assert.NoError(t, settings.db.upsert("space", "disable_user_create", "1", settingTypeBool, "")) + assert.NoError(t, settings.Reload()) + + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/v1/common/appconfig?version=99999999", nil) + req.Header.Set("token", testutil.Token) + s.GetRoute().ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code) + assert.Contains(t, w.Body.String(), `"disable_user_create_space":1`) +} + // appconfig 必须下发 local_login_off：默认 0（缺 system_setting 行）。 func TestGetAppConfig_LocalLoginOff_DefaultsZero(t *testing.T) { s, ctx := testutil.NewTestServer() diff --git a/modules/common/system_setting_schema.go b/modules/common/system_setting_schema.go index dce5d298..d257ea26 100644 --- a/modules/common/system_setting_schema.go +++ b/modules/common/system_setting_schema.go @@ -50,6 +50,12 @@ var systemSettingSchema = []settingDef{ {Category: "login", Key: "local_off", Type: settingTypeBool, Description: "是否关闭本地账号登录入口", Effective: func(s *SystemSettings) string { return boolToCanonical(s.LocalLoginOff()) }}, + // Space user-facing creation toggle — admin 关闭后客户端隐藏创建入口, + // 后端 POST /v1/space/create 直接 403。env DM_SPACE_DISABLE_USER_CREATE + // 仍作 fallback,DB 行为单一真源。 + {Category: "space", Key: "disable_user_create", Type: settingTypeBool, Description: "是否关闭普通用户创建空间入口", + Effective: func(s *SystemSettings) string { return boolToCanonical(s.SpaceDisableUserCreate()) }}, + // Email server config — formerly yaml-only (Support.* in config.go). {Category: "support", Key: "email", Type: settingTypeString, Description: "技术支持邮箱（发件人）", Effective: func(s *SystemSettings) string { return s.SupportEmail() }}, diff --git a/modules/common/system_settings.go b/modules/common/system_settings.go index 8ea00ad9..522c7cd4 100644 --- a/modules/common/system_settings.go +++ b/modules/common/system_settings.go @@ -6,6 +6,7 @@ import ( "os" "regexp" "strconv" + "strings" "sync" "sync/atomic" "time" @@ -427,6 +428,54 @@ func (s *SystemSettings) RawLocalLoginOffFromSnapshot() bool { return s.getBool("login", "local_off", false) } +// envSpaceDisableUserCreate 与 modules/space/api.go:envDisableUserCreateSpace +// 保持同名,镜像在 common 包以避免反向依赖 (space 已 import common)。新增/修改 +// env 解析规则时两处同步,语义就是: 1/true/yes/on (任意大小写,允许前后空格) +// 视为 ON，其余皆 OFF。 +const envSpaceDisableUserCreate = "DM_SPACE_DISABLE_USER_CREATE" + +// SpaceDisableUserCreate reports whether the user-facing「创建空间」入口应被 +// 关闭。完整 fallback 链(按优先级): +// +// 1. system_setting space.disable_user_create 行的 DB 值(走 getBool 同款解析) +// 2. 空 DB 行 / DB row 不存在 / DB 写入未知字面量 → env DM_SPACE_DISABLE_USER_CREATE +// 3. 都缺失 → false (保持开放) +// +// DB 是单一真源：admin 在管理台显式 toggle 立刻生效（Reload 内存快照）， +// 多实例 60s 内收敛。env 仅作历史部署兼容入口；新部署应直接走 system_setting。 +// +// 与 modules/space/api.go:IsUserCreateDisabled 保持等价语义 —— 后者仍是 +// env-only 的低层解析器,留给没有 ctx 的调用方与 yaml 模式;实际请求路径走本 +// 方法（modules/space/api.go:createSpace）。 +// +// 实现细节：DB 路径委托给 getBool 以与其他 bool 设置共享解析规则,避免双写 +// 字面量集合(reviewer H1)。"DB 行是否存在"由独立 lookup 决定,从而区分 +// "DB 缺行 → env" 与 "DB 值=0 → 强制 false 压制 env" 两个语义。 +func (s *SystemSettings) SpaceDisableUserCreate() bool { + if _, ok := s.lookup("space", "disable_user_create"); ok { + // 走与所有其他 bool 设置一致的字面量解析;未知字面量会落到 fallback=false, + // 与 "DB 显式写了 0" 语义一致 —— 都视为 admin 不希望关闭。 + return s.getBool("space", "disable_user_create", false) + } + return parseSpaceDisableUserCreateEnv(os.Getenv(envSpaceDisableUserCreate)) +} + +// parseSpaceDisableUserCreateEnv 与 modules/space/api.go:IsUserCreateDisabled +// 的解析逻辑保持一致(1/true/yes/on,大小写不敏感,允许前后空格)。两处镜像而 +// 非提到 leaf package,理由同 LocalLoginOff/OIDC: 一个 helper 不值得为它引 +// 入一层新包。修改任何一处时两边同步,否则同一开关在两个出口语义会漂移。 +func parseSpaceDisableUserCreateEnv(v string) bool { + v = strings.TrimSpace(v) + if v == "" { + return false + } + switch strings.ToLower(v) { + case "1", "true", "yes", "on": + return true + } + return false +} + // SupportEmail returns the From address used by the SMTP sender. func (s *SystemSettings) SupportEmail() string { return s.getString("support", "email", s.ctx.GetConfig().Support.Email) diff --git a/modules/common/system_settings_test.go b/modules/common/system_settings_test.go index 5cb807b5..acaee347 100644 --- a/modules/common/system_settings_test.go +++ b/modules/common/system_settings_test.go @@ -223,6 +223,74 @@ func TestSystemSettings_LocalLoginOff_TrueWhenGiteeConfigured(t *testing.T) { assert.True(t, s.LocalLoginOff(), "Gitee OAuth 配置齐备 → 守卫生效") } +// SpaceDisableUserCreate 的 fallback 链：DB → env DM_SPACE_DISABLE_USER_CREATE → false。 +// 默认无 DB 行且无 env 时,返回 false（保持历史行为：用户侧可创建空间）。 +func TestSystemSettings_SpaceDisableUserCreate_DefaultsFalse(t *testing.T) { + s := newTestSystemSettings(t, nil) + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "") + assert.False(t, s.SpaceDisableUserCreate(), + "DB 未配置且 env 未设置时必须保持开放（false）") +} + +// DB 值优先于 env：admin 在管理台显式关闭时,即便 env 未设置也立即生效。 +func TestSystemSettings_SpaceDisableUserCreate_DBTrueWins(t *testing.T) { + s := newTestSystemSettings(t, nil) + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "") + require.NoError(t, s.db.upsert("space", "disable_user_create", "1", settingTypeBool, "")) + require.NoError(t, s.Reload()) + assert.True(t, s.SpaceDisableUserCreate(), + "DB=1 必须关闭用户侧创建入口") +} + +// DB 行存在但值为 0 时必须明确返回 false,即使 env 设置为 true 也不再"漏出去"。 +// 这一条用例固化"DB 是单一真源"的语义：admin 在管理台 toggle 回 0 必须能压住 +// 历史 env 配置；否则运维改了配置却看不到效果。 +func TestSystemSettings_SpaceDisableUserCreate_DBFalseOverridesEnv(t *testing.T) { + s := newTestSystemSettings(t, nil) + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "true") + require.NoError(t, s.db.upsert("space", "disable_user_create", "0", settingTypeBool, "")) + require.NoError(t, s.Reload()) + assert.False(t, s.SpaceDisableUserCreate(), + "DB=0 必须覆盖 env=true（DB 是单一真源）") +} + +// DB 行存在但 value 为空字符串时, lookup 视为"未配置"(与其它所有设置一致), +// 落回 env fallback —— 不是把 disable_user_create 强制 false。这条用例锁定 +// "DB row 空值 == 未配置" 的语义,与其他 bool 设置(register/login/support) +// 共享同一回退规则,避免后续维护者误以为空值压制 env。 +func TestSystemSettings_SpaceDisableUserCreate_DBEmptyValueFallsBackToEnv(t *testing.T) { + s := newTestSystemSettings(t, nil) + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", "true") + require.NoError(t, s.db.upsert("space", "disable_user_create", "", settingTypeBool, "")) + require.NoError(t, s.Reload()) + assert.True(t, s.SpaceDisableUserCreate(), + "DB 值=\"\" 视为未配置, env=true 必须生效") +} + +// 仅设置了 env 而无 DB 行时,getter 必须回退到 env 解析结果,保持对历史部署的 +// 兼容（envDisableUserCreateSpace 是这个开关的原始入口）。 +func TestSystemSettings_SpaceDisableUserCreate_EnvFallback(t *testing.T) { + s := newTestSystemSettings(t, nil) + for _, spelling := range []string{"1", "true", "TRUE", "yes", "on", " true "} { + t.Run(spelling, func(t *testing.T) { + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", spelling) + assert.True(t, s.SpaceDisableUserCreate(), + "env=%q 必须被识别为开启", spelling) + }) + } +} + +func TestSystemSettings_SpaceDisableUserCreate_EnvNegativeSpellings(t *testing.T) { + s := newTestSystemSettings(t, nil) + for _, spelling := range []string{"0", "false", "FALSE", "no", "random"} { + t.Run(spelling, func(t *testing.T) { + t.Setenv("DM_SPACE_DISABLE_USER_CREATE", spelling) + assert.False(t, s.SpaceDisableUserCreate(), + "env=%q 不应被识别为开启", spelling) + }) + } +} + func TestSystemSettings_StringFallsBackOnEmpty(t *testing.T) { s := newTestSystemSettings(t, nil) s.ctx.GetConfig().Support.EmailSmtp = "smtp.yaml.example:465" diff --git a/modules/space/api.go b/modules/space/api.go index 5ce91d2e..0ab03071 100644 --- a/modules/space/api.go +++ b/modules/space/api.go @@ -19,6 +19,7 @@ import ( "github.com/Mininglamp-OSS/octo-lib/pkg/wkevent" "github.com/Mininglamp-OSS/octo-lib/pkg/wkhttp" "github.com/Mininglamp-OSS/octo-server/modules/base/event" + commonmod "github.com/Mininglamp-OSS/octo-server/modules/common" octoredis "github.com/Mininglamp-OSS/octo-server/pkg/redis" spacepkg "github.com/Mininglamp-OSS/octo-server/pkg/space" rd "github.com/go-redis/redis" @@ -121,11 +122,22 @@ func (s *Space) Route(r *wkhttp.WKHttp) { } } -// envDisableUserCreateSpace 全局开关：运维通过环境变量 DM_SPACE_DISABLE_USER_CREATE=true -// 关闭用户侧创建空间入口（POST /v1/space/create）。管理端代建接口不受此开关约束。 +// envDisableUserCreateSpace 全局开关的历史 env 入口：运维通过环境变量 +// DM_SPACE_DISABLE_USER_CREATE=true 关闭用户侧创建空间入口 +// （POST /v1/space/create）。管理端代建接口不受此开关约束。 +// +// 单一真源已迁移到 system_setting 表的 (space, disable_user_create) 行； +// 本 env 仍作 fallback 供没有 DB override 的老部署使用,但 admin 在管理台写入 +// DB 后,DB 值优先。详见 modules/common/system_settings.go:SpaceDisableUserCreate。 const envDisableUserCreateSpace = "DM_SPACE_DISABLE_USER_CREATE" -// IsUserCreateDisabled 是否已通过环境变量关闭用户侧创建空间。 +// IsUserCreateDisabled 是否已通过环境变量关闭用户侧创建空间。env-only 的低层 +// 解析器,语义与 modules/common/system_settings.go:parseSpaceDisableUserCreateEnv +// 保持一致 —— 后者是 system_setting 查询的 env fallback。修改此函数的解析 +// 规则时必须同步那一处,否则同一开关会在两个出口产生漂移。 +// +// 调用方应优先走 (*Space).isUserCreateDisabled 以获得 DB → env 完整链路；本 +// 函数保留是为了向前兼容已有 callers / 测试,以及作为最简 env-only 探测点。 func IsUserCreateDisabled() bool { v := strings.TrimSpace(os.Getenv(envDisableUserCreateSpace)) if v == "" { @@ -138,6 +150,13 @@ func IsUserCreateDisabled() bool { return false } +// isUserCreateDisabled 走 system_setting DB → env 完整 fallback 链, 是 +// createSpace handler 的判断入口。SystemSettings snapshot 由 ticker 自动刷新, +// admin 写 DB → Reload 后 60s 内多实例收敛, 本实例立即生效。 +func (s *Space) isUserCreateDisabled() bool { + return commonmod.EnsureSystemSettings(s.ctx).SpaceDisableUserCreate() +} + // createSpaceParams 创建空间的核心参数。Creator 为目标空间 owner， // 管理端代建时设为被代建用户，业务端为登录用户。 type createSpaceParams struct { @@ -158,7 +177,7 @@ type createSpaceResult struct { // createSpace 创建空间（用户侧入口） func (s *Space) createSpace(c *wkhttp.Context) { - if IsUserCreateDisabled() { + if s.isUserCreateDisabled() { c.ResponseErrorWithStatus(errors.New("管理员已关闭空间创建功能"), http.StatusForbidden) return } diff --git a/modules/space/api_test.go b/modules/space/api_test.go index 20aca515..4fcd2af6 100644 --- a/modules/space/api_test.go +++ b/modules/space/api_test.go @@ -16,8 +16,10 @@ import ( "github.com/Mininglamp-OSS/octo-lib/common" "github.com/Mininglamp-OSS/octo-lib/config" "github.com/Mininglamp-OSS/octo-lib/pkg/util" + "github.com/Mininglamp-OSS/octo-lib/pkg/wkhttp" "github.com/Mininglamp-OSS/octo-lib/server" "github.com/Mininglamp-OSS/octo-lib/testutil" + modulescommon "github.com/Mininglamp-OSS/octo-server/modules/common" "github.com/stretchr/testify/assert" ) @@ -1535,6 +1537,54 @@ func TestCreateSpace_AllowedByDefault(t *testing.T) { assert.Equal(t, 2, mem.Role) } +// system_setting 写入 space.disable_user_create=1 后, createSpace 必须返回 403, +// 不依赖任何环境变量。这是「admin 在管理台实时关闭用户侧创建」核心路径的 +// 守卫用例 —— 离开 env 之后,DB 行 + Reload 立刻让本实例生效,多实例由 60s +// 自动 reload 收敛(参见 SystemSettings.StartAutoReload)。 +func TestCreateSpace_DisabledBySystemSetting(t *testing.T) { + s, _, err := setup(t) + assert.NoError(t, err) + // 显式清空 env, 证明开关纯由 DB 驱动 + t.Setenv(envDisableUserCreateSpace, "") + + // 直接 DB 写入 + Reload, 模拟 manager API 的写路径但避开 admin token 与 + // 路由准备 — 这条用例的关注点是 "DB → SystemSettings → createSpace 拒绝" + // 这条链路, 不是 manager API 本身(后者在 common 包已有单测覆盖)。 + _, err = testCtx.DB().InsertInto("system_setting"). + Pair("category", "space"). + Pair("key_name", "disable_user_create"). + Pair("value", "1"). + Pair("value_type", "bool"). + Pair("description", ""). + Exec() + assert.NoError(t, err) + settings := modulescommon.EnsureSystemSettings(testCtx) + assert.NoError(t, settings.Reload()) + defer func() { + _, _ = testCtx.DB().DeleteFrom("system_setting"). + Where("category=? AND key_name=?", "space", "disable_user_create"). + Exec() + _ = settings.Reload() + }() + + body := util.ToJson(map[string]interface{}{ + "name": "p2-blocked-by-db", + "join_mode": 0, + }) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/v1/space/create", bytes.NewReader([]byte(body))) + req.Header.Set("token", testutil.Token) + s.GetRoute().ServeHTTP(w, req) + + assert.Equal(t, http.StatusForbidden, w.Code, "DB 开关 ON 时应返回 403, body=%s", w.Body.String()) + assert.Contains(t, w.Body.String(), "已关闭") + + var count int + _, err = testCtx.DB().SelectBySql("SELECT COUNT(*) FROM space WHERE name=?", "p2-blocked-by-db").Load(&count) + assert.NoError(t, err) + assert.Equal(t, 0, count, "DB 开关 ON 时不应写入任何 space 记录") +} + func TestCreateSpace_DisabledByEnv(t *testing.T) { s, _, err := setup(t) assert.NoError(t, err) @@ -1807,3 +1857,101 @@ func TestRejectJoinApply_DoesNotConsumeInvite(t *testing.T) { assert.Equal(t, 2, updated.Status) assert.Equal(t, testutil.UID, updated.ReviewerUID) } + +// TestE2E_DisableUserCreateSpace_FullChain 串起完整的 admin 实时调控链路: +// +// manager POST /v1/manager/common/system_setting (写 disable_user_create=1) +// → 客户端 GET /v1/common/appconfig (看到 disable_user_create_space=1) +// → 用户 POST /v1/space/create (403) +// → manager POST 写回 0 +// → 用户 POST /v1/space/create (200) +// +// 这条 e2e 守住 "DB 单一真源 + Reload 即时生效 + 前后端用同一 getter" 的整条链路, +// 任一节点漂移(写路径未触发 Reload、appconfig 漏字段、createSpace 走老 env-only +// 路径)都会让本用例失败。 +func TestE2E_DisableUserCreateSpace_FullChain(t *testing.T) { + srv, _, err := setup(t) + assert.NoError(t, err) + t.Setenv(envDisableUserCreateSpace, "") + t.Setenv("OCTO_MASTER_KEY", "0123456789abcdef0123456789abcdef") + + // CleanAllTables 清空了 app_config,/v1/common/appconfig 没拿到行会 400。 + // 这里灌一行默认 app_config(其余 NOT NULL 列在 schema 里都有 DEFAULT), + // 让 appconfig handler 走到我们要验证的字段下发路径。 + _, err = testCtx.DB().InsertInto("app_config").Pair("version", 1).Exec() + assert.NoError(t, err) + + // 给 testutil.Token 升 super admin 角色,以便调用 manager 写接口。 + // CleanAllTables 不会清缓存里的 token 行,但 setup 内已重置一次,这里覆盖 + // 上层角色到 SuperAdmin。还原也走 cache.Set,无副作用。 + cfg := testCtx.GetConfig() + tokenKey := cfg.Cache.TokenCachePrefix + testutil.Token + origTokenVal, _ := testCtx.Cache().Get(tokenKey) + assert.NoError(t, testCtx.Cache().Set(tokenKey, + testutil.UID+"@test@"+string(wkhttp.SuperAdmin))) + defer func() { _ = testCtx.Cache().Set(tokenKey, origTokenVal) }() + + defer func() { + // 不论用例分支如何退出,把 system_setting 行清掉避免污染后续测试。 + _, _ = testCtx.DB().DeleteFrom("system_setting"). + Where("category=? AND key_name=?", "space", "disable_user_create"). + Exec() + _ = modulescommon.EnsureSystemSettings(testCtx).Reload() + }() + + writeSetting := func(value string) { + t.Helper() + body := util.ToJson(map[string]interface{}{ + "items": []map[string]string{{ + "category": "space", + "key": "disable_user_create", + "value": value, + }}, + }) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", + "/v1/manager/common/system_setting", + bytes.NewReader([]byte(body))) + req.Header.Set("token", testutil.Token) + srv.GetRoute().ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code, + "manager 写 disable_user_create=%s 应 200, body=%s", value, w.Body.String()) + } + + getAppconfig := func() string { + t.Helper() + w := httptest.NewRecorder() + req, _ := http.NewRequest("GET", "/v1/common/appconfig", nil) + srv.GetRoute().ServeHTTP(w, req) + assert.Equal(t, http.StatusOK, w.Code, "appconfig 应 200, body=%s", w.Body.String()) + return w.Body.String() + } + + createSpace := func(name string) int { + t.Helper() + body := util.ToJson(map[string]interface{}{ + "name": name, + "join_mode": 0, + }) + w := httptest.NewRecorder() + req, _ := http.NewRequest("POST", "/v1/space/create", + bytes.NewReader([]byte(body))) + req.Header.Set("token", testutil.Token) + srv.GetRoute().ServeHTTP(w, req) + return w.Code + } + + // --- Step 1: 关闭 --- + writeSetting("1") + assert.Contains(t, getAppconfig(), `"disable_user_create_space":1`, + "manager 写入后 appconfig 必须立刻下发 1") + assert.Equal(t, http.StatusForbidden, createSpace("e2e-off"), + "开关 ON 时 createSpace 必须 403") + + // --- Step 2: 重新打开 --- + writeSetting("0") + assert.Contains(t, getAppconfig(), `"disable_user_create_space":0`, + "manager 写回 0 后 appconfig 必须立刻下发 0") + assert.Equal(t, http.StatusOK, createSpace("e2e-on"), + "开关 OFF 时 createSpace 必须 200") +} From 3b0547cc106bd68ecbdff5a151ef590058b62c8a Mon Sep 17 00:00:00 2001 From: an9xyz Date: Thu, 28 May 2026 19:59:52 +0800 Subject: [PATCH 6/6] docs(system_settings): fix SpaceDisableUserCreate fallback-chain wording Jerry-Xin pointed out (PR #189 review) that the docstring claimed an unknown DB literal falls back to env, but the implementation delegates to getBool which falls back to false. Align the doc with the actual behavior; behavior unchanged. --- modules/common/system_settings.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/modules/common/system_settings.go b/modules/common/system_settings.go index 522c7cd4..670d1826 100644 --- a/modules/common/system_settings.go +++ b/modules/common/system_settings.go @@ -437,10 +437,16 @@ const envSpaceDisableUserCreate = "DM_SPACE_DISABLE_USER_CREATE" // SpaceDisableUserCreate reports whether the user-facing「创建空间」入口应被 // 关闭。完整 fallback 链(按优先级): // -// 1. system_setting space.disable_user_create 行的 DB 值(走 getBool 同款解析) -// 2. 空 DB 行 / DB row 不存在 / DB 写入未知字面量 → env DM_SPACE_DISABLE_USER_CREATE +// 1. DB 行存在且 value 非空 → 走 getBool 解析(1/true/TRUE → true; +// 0/false/FALSE → false; 未知字面量 → false)。**不再回退到 env** —— 与 +// 其他 bool 设置一致,未知字面量等同 "admin 不希望关闭"。 +// 2. DB 行不存在,或 value="" → env DM_SPACE_DISABLE_USER_CREATE // 3. 都缺失 → false (保持开放) // +// 注：manager 写接口对 bool 值已做规范化(只接受 0/1/true/false 及大小写 +// 变体),正常路径不会出现未知字面量;此规则覆盖的是有人绕过 API 直接改 DB +// 的边缘场景。 +// // DB 是单一真源：admin 在管理台显式 toggle 立刻生效（Reload 内存快照）， // 多实例 60s 内收敛。env 仅作历史部署兼容入口；新部署应直接走 system_setting。 //