feat(i18n): keep translation source hashes in .review.json (spec 019 T078)

docs/module-authoring.md

@@ -236,8 +236,8 @@ events fire only when the list of problems changes, not on every re-pull.
 Message ids may contain upper case letters (for example
 `config.MAX_PLAYERS.displayName`) and `~` as the reversible escapes `~0` and
 `~1`. Run `gp-module i18n stamp` after translating to record the English text
-each entry was made from (`sourceHash`); if the English text changes later,
-`gp-module validate` warns with `locale-needs-review`.
+each entry was made from in `locales/.review.json`; if the English text
+changes later, `gp-module validate` warns with `locale-needs-review`.
 
 Manifest:
 

docs/translating.md

@@ -44,8 +44,11 @@ The folder name must equal the `tag` in `meta.json`.
 - Use 2-space indentation, UTF-8, and end the file with a newline.
 - Do not invent ids. An id that English does not have is an error. You may
   leave out any id you have not translated yet.
-- `sourceHash` is written by the checker. It records which English text
-  your translation was made from. Leave it alone when you edit by hand.
+- The checker records which English text each translation was made from in
+  `<tag>/.review.json`, a separate file next to `app.json`. Do not edit it by
+  hand. An older `sourceHash` key inside `app.json` is moved there by the
+  next `--stamp`. Translation platforms rewrite `app.json`, so the hashes
+  are kept apart from it.
 
 ### meta.json
 
@@ -141,7 +144,8 @@ The checker has more options:
 
 - `--base <dir>` compares completeness with an older copy of the packs.
 - `--summary <file>` writes a markdown summary.
-- `--stamp` adds `sourceHash` to translated entries that lack one.
+- `--stamp` records the source hash of translated entries that lack one in
+  `<tag>/.review.json`, and moves any inline `sourceHash` out of `app.json`.
 - `--manifest <file>` writes the language list as JSON.
 
 You can run the checker on your machine to see findings early. It is not a
@@ -185,9 +189,13 @@ shows English.
    gets `type: docs`. Add a `docs:` prefix to be sure.
 
 You can also translate in the browser on Hosted Weblate:
-<https://hosted.weblate.org/projects/gameplane/>. Only translations that a
-human reviewer approves are merged. They arrive as a pull request from the
-`weblate/translations` branch and pass the same checks.
+<https://hosted.weblate.org/projects/gameplane/>. Reviews are on: members of a
+language's reviewers team in Weblate (for example "French reviewers", which
+has the "Review strings" role for French) approve strings there. Weblate
+sends its changes as a pull request from its `weblate-gameplane-app` branch.
+That pull request passes the same checks and needs one maintainer approval
+before it merges. Weblate rewrites `app.json` and drops keys it does not know,
+which is why the review hashes live in `.review.json`.
 
 ## Adding text to the dashboard (developers)
 

gp-module/cmd/gp-module/i18n.go

@@ -43,9 +43,10 @@ Subcommands:
       and write the English reference pack, with translator descriptions
       (default: <MODULE_PATH>/locales/en.json).
   stamp [MODULE_PATH]
-      Record the current English text hash (sourceHash) on every translated
-      entry of <MODULE_PATH>/locales/<tag>.json (except en.json) that lacks
-      one, so a later English change shows up as needs-review.
+      Record the current English text hash of every translated entry of
+      <MODULE_PATH>/locales/<tag>.json (except en.json) that lacks one in
+      <MODULE_PATH>/locales/.review.json, so a later English change shows up
+      as needs-review. Hashes still inside a <tag>.json are moved there.
 `
 
 func i18nExtract(args []string, stdout, stderr io.Writer) int {

gp-module/cmd/gp-module/i18n_stamp_test.go

@@ -22,8 +22,8 @@ func TestI18nStampCommand(t *testing.T) {
 	if got := i18nCommand([]string{"stamp", dir}, &out, &errb); got != 0 || !strings.Contains(out.String(), "stamped") {
 		t.Fatalf("exit = %d out=%q err=%q", got, out.String(), errb.String())
 	}
-	if data, _ := os.ReadFile(fr); !strings.Contains(string(data), "sourceHash") {
-		t.Errorf("fr.json = %s", data)
+	if data, _ := os.ReadFile(filepath.Join(dir, "locales", ".review.json")); !strings.Contains(string(data), "template.displayName") {
+		t.Errorf(".review.json = %s", data)
 	}
 	out.Reset()
 	if got := i18nCommand([]string{"stamp", dir}, &out, &errb); got != 0 || !strings.Contains(out.String(), "nothing to stamp") {

gp-module/internal/i18n/review_stamp_test.go

@@ -0,0 +1,135 @@
+package i18n
+
+import (
+	"errors"
+	"io/fs"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"github.com/GameplanePanel/gameplane/i18n/pack"
+)
+
+// TestStampMovesInlineHashToReview checks that Stamp moves an inline source hash into the review file.
+func TestStampMovesInlineHashToReview(t *testing.T) {
+	t.Parallel()
+	dir := writeModule(t, testModule, testTemplate)
+	locales := filepath.Join(dir, "locales")
+	if err := os.MkdirAll(locales, 0o750); err != nil {
+		t.Fatal(err)
+	}
+	hash := pack.SourceHash("Demo Game")
+	fr := filepath.Join(locales, "fr.json")
+	body := `{"template.displayName":{"defaultMessage":"Jeu","sourceHash":"` + hash + `"}}`
+	if err := os.WriteFile(fr, []byte(body), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	stamped, err := Stamp(dir)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if len(stamped) != 2 {
+		t.Fatalf("Stamp = %v; want fr.json and .review.json", stamped)
+	}
+	frData, err := os.ReadFile(fr)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if strings.Contains(string(frData), "sourceHash") {
+		t.Errorf("fr.json still carries sourceHash: %s", frData)
+	}
+	reviewData, err := os.ReadFile(filepath.Join(locales, pack.ReviewFile))
+	if err != nil {
+		t.Fatal(err)
+	}
+	review, err := pack.ParseModuleReview(reviewData)
+	if err != nil {
+		t.Fatal(err)
+	}
+	if got := review["fr"]["template.displayName"]; got != hash {
+		t.Errorf("review hash = %q; want %q", got, hash)
+	}
+}
+
+// TestStampKeepsExistingReview checks that Stamp keeps an existing review entry instead of overwriting it.
+func TestStampKeepsExistingReview(t *testing.T) {
+	t.Parallel()
+	dir := writeModule(t, testModule, testTemplate)
+	locales := filepath.Join(dir, "locales")
+	if err := os.MkdirAll(locales, 0o750); err != nil {
+		t.Fatal(err)
+	}
+	hash := pack.SourceHash("Demo Game")
+	review := `{"fr":{"template.displayName":"` + hash + `"}}`
+	if err := os.WriteFile(filepath.Join(locales, pack.ReviewFile), []byte(review), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(locales, "fr.json"), []byte(`{"template.displayName":{"defaultMessage":"Jeu"}}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+
+	stamped, err := Stamp(dir)
+	if err != nil || len(stamped) != 0 {
+		t.Fatalf("Stamp = %v, %v; want a no-op", stamped, err)
+	}
+	after, err := os.ReadFile(filepath.Join(locales, pack.ReviewFile))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(after) != review {
+		t.Errorf("review file was rewritten: %q", after)
+	}
+}
+
+// TestStampMalformedReviewIsError checks that Stamp reports an error when the review file is malformed.
+func TestStampMalformedReviewIsError(t *testing.T) {
+	t.Parallel()
+	dir := writeModule(t, testModule, testTemplate)
+	locales := filepath.Join(dir, "locales")
+	if err := os.MkdirAll(locales, 0o750); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(locales, pack.ReviewFile), []byte(`[`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	_, err := Stamp(dir)
+	if err == nil || !strings.Contains(err.Error(), pack.ReviewFile) {
+		t.Fatalf("Stamp error = %v; want one naming %s", err, pack.ReviewFile)
+	}
+}
+
+// TestStampWritesNothingWhenALocaleIsMalformed checks that a malformed locale
+// stops Stamp before any locale file loses its inline hashes.
+func TestStampWritesNothingWhenALocaleIsMalformed(t *testing.T) {
+	t.Parallel()
+	dir := writeModule(t, testModule, testTemplate)
+	locales := filepath.Join(dir, "locales")
+	if err := os.MkdirAll(locales, 0o750); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(locales, "en.json"), []byte(`{}`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	de := `{"template.displayName":{"defaultMessage":"Spiel","sourceHash":"000000000000"}}`
+	if err := os.WriteFile(filepath.Join(locales, "de.json"), []byte(de), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if err := os.WriteFile(filepath.Join(locales, "fr.json"), []byte(`{`), 0o600); err != nil {
+		t.Fatal(err)
+	}
+	if _, err := Stamp(dir); err == nil {
+		t.Fatal("Stamp succeeded with a malformed fr.json")
+	}
+	got, err := os.ReadFile(filepath.Join(locales, "de.json"))
+	if err != nil {
+		t.Fatal(err)
+	}
+	if string(got) != de {
+		t.Errorf("de.json = %q; want it unchanged", got)
+	}
+	if _, err := os.Stat(filepath.Join(locales, pack.ReviewFile)); !errors.Is(err, fs.ErrNotExist) {
+		t.Errorf("review file exists or stat failed: %v", err)
+	}
+}

gp-module/internal/i18n/stamp.go

@@ -1,7 +1,9 @@
 package i18n
 
 import (
+	"errors"
 	"fmt"
+	"io/fs"
 	"os"
 	"path/filepath"
 	"sort"
@@ -10,11 +12,14 @@ import (
 	"github.com/GameplanePanel/gameplane/i18n/pack"
 )
 
-// Stamp records the current English source hash (sourceHash, as in app packs)
-// on every translated entry of the module's locales/<tag>.json files that does
-// not have one yet. The English reference en.json is left alone. It returns the
-// files it rewrote, sorted. A file that does not parse is an error and nothing
-// after it is touched.
+// Stamp records the current English source hash of every translated entry of
+// the module's locales/<tag>.json files that does not have one yet. Hashes live
+// in locales/.review.json, keyed by tag, because translation platforms rewrite
+// the translation files and drop unknown keys; a sourceHash still inside a
+// <tag>.json is read and moved there. The English reference en.json is left
+// alone. It returns the files it rewrote, sorted. Nothing is written until
+// every locale parses, and the review file is written before any locale file
+// loses its inline hashes, so a failure never drops a hash.
 func Stamp(dir string) ([]string, error) {
 	ref, err := reference(dir)
 	if err != nil {
@@ -28,45 +33,95 @@ func Stamp(dir string) ([]string, error) {
 		}
 		return nil, fmt.Errorf("read locales directory: %w", err)
 	}
-	var stamped []string
+	review, err := readModuleReview(localesDir)
+	if err != nil {
+		return nil, err
+	}
+	type pending struct {
+		name string
+		data []byte
+	}
+	var writes []pending
+	reviewChanged := false
 	for _, e := range entries {
 		name := e.Name()
-		if e.IsDir() || !e.Type().IsRegular() || !strings.HasSuffix(name, ".json") {
+		if e.IsDir() || !e.Type().IsRegular() || !strings.HasSuffix(name, ".json") || name == pack.ReviewFile {
 			continue
 		}
 		tag := strings.TrimSuffix(name, ".json")
 		if tag == "en" {
 			continue
 		}
 		if !pack.ValidTag(tag) {
-			return stamped, fmt.Errorf("locales/%s: %q is not a valid language tag", name, tag)
+			return nil, fmt.Errorf("locales/%s: %q is not a valid language tag", name, tag)
 		}
 		path := filepath.Join(localesDir, name)
 		data, err := os.ReadFile(filepath.Clean(path))
 		if err != nil {
-			return stamped, fmt.Errorf("read locales/%s: %w", name, err)
+			return nil, fmt.Errorf("read locales/%s: %w", name, err)
 		}
 		tr, err := pack.ParseEntries(data)
 		if err != nil {
-			return stamped, fmt.Errorf("locales/%s: %w", name, err)
+			return nil, fmt.Errorf("locales/%s: %w", name, err)
 		}
+		inline := pack.HasInlineHashes(tr)
+		tr = pack.ApplyReview(tr, review[tag])
 		out, changed := pack.Stamp(ref, tr)
-		if !changed {
+		if !changed && !inline {
 			continue
 		}
-		encoded, err := pack.Marshal(out)
+		stripped, hashes := pack.SplitReview(out)
+		review[tag] = hashes
+		reviewChanged = true
+		if !inline {
+			continue
+		}
+		// Move the hashes out of the translation file, which translation
+		// platforms rewrite.
+		encoded, err := pack.Marshal(stripped)
+		if err != nil {
+			return nil, fmt.Errorf("encode locales/%s: %w", name, err)
+		}
+		writes = append(writes, pending{name: name, data: encoded})
+	}
+	var stamped []string
+	if reviewChanged {
+		encoded, err := pack.MarshalModuleReview(review)
 		if err != nil {
-			return stamped, fmt.Errorf("encode locales/%s: %w", name, err)
+			return stamped, fmt.Errorf("encode locales/%s: %w", pack.ReviewFile, err)
+		}
+		if err := writeInDir(localesDir, pack.ReviewFile, encoded); err != nil {
+			return stamped, fmt.Errorf("write locales/%s: %w", pack.ReviewFile, err)
 		}
-		if err := writeInDir(localesDir, name, encoded); err != nil {
-			return stamped, fmt.Errorf("write locales/%s: %w", name, err)
+		stamped = append(stamped, filepath.Join(localesDir, pack.ReviewFile))
+	}
+	for _, w := range writes {
+		if err := writeInDir(localesDir, w.name, w.data); err != nil {
+			return stamped, fmt.Errorf("write locales/%s: %w", w.name, err)
 		}
-		stamped = append(stamped, path)
+		stamped = append(stamped, filepath.Join(localesDir, w.name))
 	}
 	sort.Strings(stamped)
 	return stamped, nil
 }
 
+// readModuleReview reads locales/.review.json. A missing file yields an empty
+// review; a file that does not parse is an error.
+func readModuleReview(localesDir string) (map[string]map[string]string, error) {
+	data, err := os.ReadFile(filepath.Clean(filepath.Join(localesDir, pack.ReviewFile)))
+	if err != nil {
+		if errors.Is(err, fs.ErrNotExist) {
+			return map[string]map[string]string{}, nil
+		}
+		return nil, fmt.Errorf("read locales/%s: %w", pack.ReviewFile, err)
+	}
+	review, err := pack.ParseModuleReview(data)
+	if err != nil {
+		return nil, fmt.Errorf("locales/%s: %w", pack.ReviewFile, err)
+	}
+	return review, nil
+}
+
 // writeInDir writes name inside dir through an os.Root, so the write cannot
 // escape dir whatever the directory listing contained.
 func writeInDir(dir, name string, data []byte) error {

gp-module/internal/i18n/stamp_test.go

@@ -27,16 +27,16 @@ func TestStampRecordsSourceHash(t *testing.T) {
 	if err != nil || len(stamped) != 1 {
 		t.Fatalf("Stamp = %v, %v", stamped, err)
 	}
-	data, err := os.ReadFile(fr)
+	data, err := os.ReadFile(filepath.Join(dir, "locales", pack.ReviewFile))
 	if err != nil {
 		t.Fatal(err)
 	}
-	got, err := pack.ParseEntries(data)
+	review, err := pack.ParseModuleReview(data)
 	if err != nil {
 		t.Fatal(err)
 	}
-	if got["template.displayName"].SourceHash != pack.SourceHash("Demo Game") {
-		t.Errorf("entry = %+v", got["template.displayName"])
+	if got := review["fr"]["template.displayName"]; got != pack.SourceHash("Demo Game") {
+		t.Errorf("review hash = %q", got)
 	}
 	if b, _ := os.ReadFile(en); string(b) != `{}` {
 		t.Errorf("en.json was rewritten: %q", b)

gp-module/internal/packager/packager.go

@@ -242,9 +242,10 @@ func ReadModuleFiles(dirPath string) (map[string][]byte, error) {
 }
 
 // ReadLocaleFiles reads the translation files of a module directory. Keys are
-// "locales/<tag>.json"; the English reference en.json is not shipped, and
-// non-JSON files are ignored. A missing locales directory yields an empty map.
-// Symbolic links and files whose name is not a valid language tag are errors.
+// "locales/<tag>.json"; the English reference en.json and the review file
+// (pack.ReviewFile) are not shipped, and non-JSON files are ignored. A missing
+// locales directory yields an empty map. Symbolic links and files whose name is
+// not a valid language tag are errors.
 func ReadLocaleFiles(dirPath string) (map[string][]byte, error) {
 	dir := filepath.Join(dirPath, "locales")
 	entries, err := os.ReadDir(dir)
@@ -257,7 +258,7 @@ func ReadLocaleFiles(dirPath string) (map[string][]byte, error) {
 	out := make(map[string][]byte)
 	for _, entry := range entries {
 		name := entry.Name()
-		if entry.IsDir() || !strings.HasSuffix(name, ".json") {
+		if entry.IsDir() || !strings.HasSuffix(name, ".json") || name == pack.ReviewFile {
 			continue
 		}
 		if entry.Type()&os.ModeSymlink != 0 {