diff --git a/.github/workflows/frontend-tests.yml b/.github/workflows/frontend-tests.yml index d70fb90fe3..f43f169a0f 100644 --- a/.github/workflows/frontend-tests.yml +++ b/.github/workflows/frontend-tests.yml @@ -155,6 +155,34 @@ jobs: - name: Run i18n checks run: pnpm --filter @kandev/web i18n:check + # New-code ratchet. The eslint literal guard is an error only on migrated + # paths, which leaves new files under un-migrated directories unguarded. + # These two judge the CHANGE instead of the FILE: an added file must be + # clean outright, a modified file only on the lines it touched, and the + # guard allowlist may not shrink. Same model as + # `golangci-lint --new-from-rev` for Go. Base SHA is resolved the same way + # as the Prettier step above. + - name: Run i18n new-code ratchet + shell: bash + # The job defaults to `apps`; these scripts live one level deeper. + working-directory: apps/web + run: | + set -euo pipefail + + if [[ "${{ github.event_name }}" == "pull_request" ]]; then + BASE_SHA="${{ github.event.pull_request.base.sha }}" + else + BASE_SHA="${{ github.event.before }}" + fi + + if [[ -z "${BASE_SHA}" || "${BASE_SHA}" == "0000000000000000000000000000000000000000" ]]; then + echo "No usable base SHA; skipping the new-code ratchet." + exit 0 + fi + + node scripts/check-new-code-i18n.mjs --base "${BASE_SHA}" + node scripts/check-guard-allowlist.mjs --base "${BASE_SHA}" + - name: Run tests run: pnpm --filter @kandev/web test diff --git a/.pre-commit-config.yaml b/.pre-commit-config.yaml index f93114d564..d1a73d3526 100644 --- a/.pre-commit-config.yaml +++ b/.pre-commit-config.yaml @@ -51,3 +51,14 @@ repos: language: system files: ^apps/web/.*\.(ts|tsx|js|jsx)$ pass_filenames: true + + # New user-facing copy must go through t()/, even in a directory the + # i18n migration has not reached. Judges the change, not the file: added + # files whole, modified files only on touched lines — so it never asks you + # to migrate code you did not write. Mirrors the go-lint hook above. + - id: i18n-new-code + name: i18n guard (new code only) + entry: bash -c 'unset GIT_DIR GIT_WORK_TREE; cd apps/web && node scripts/check-new-code-i18n.mjs && node scripts/check-guard-allowlist.mjs' + language: system + files: ^apps/web/(components|app|hooks|lib|src)/.*\.tsx?$|^apps/web/eslint\.i18n\.options\.mjs$ + pass_filenames: false diff --git a/AGENTS.md b/AGENTS.md index 1ac7a452a3..195b8cb2a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,12 +74,17 @@ The web UI is localized with i18next (`namespace:key`). **All new user-facing co must go through `t()` / ``** — never a hardcoded literal — regardless of which directory you are in. -The migration of existing strings is in progress, **one directory per PR**. The -lint guard (`i18next/no-literal-string`) is an error, but only for the paths in -`i18nGuardFiles` (`apps/web/eslint.i18n.options.mjs`), so unrelated PRs are never -blocked by strings in code nobody has migrated. **When you migrate a directory, -append it to that list in the same PR**; never remove an entry to make a build -pass. +This is enforced, not just requested. Two ratchets, both of which only tighten: + +- **New code, everywhere** — `pnpm run i18n:ratchet` (pre-commit + CI) fails on a + hardcoded string in a file you added, or on a line you changed, regardless of + directory. Untouched lines are never judged, so you are never asked to migrate + code you did not write. +- **Migrated paths, whole file** — `i18next/no-literal-string` is a lint error for + the paths in `i18nGuardFiles` (`apps/web/eslint.i18n.options.mjs`). The + migration of existing strings proceeds **one directory per PR**: when you + migrate one, append it to that list in the same PR. Never remove an entry to + make a build pass — a check rejects that. Three rules that cause silent, hard-to-find bugs when broken: never translate a string compared with `===` (type-to-confirm tokens become impossible to type); @@ -87,9 +92,11 @@ never call `t()` at module scope (it freezes at the boot locale and the pseudo-locale cannot detect it); never pass an English plural ending as a value — use `count` with `_one`/`_other`. `pnpm run i18n:check` enforces the last two. -A clean lint is not proof a file is done — the rule only sees plain literals in -JSX. The pseudo-locale (Settings → General → Appearance, dev/e2e builds) is the -completeness check. Full guide: [`docs/i18n.md`](docs/i18n.md). +A clean lint is not proof a file is done. The rule only sees literals in JSX, and +it **skips anything assigned to a SCREAMING_CASE identifier** — so a +`const ROWS = [{ label: "Disk usage" }]` config table passes silently. Review +those by eye. The pseudo-locale (Settings → General → Appearance, dev/e2e builds) +is the completeness check. Full guide: [`docs/i18n.md`](docs/i18n.md). ### Knowledge - **Public docs:** Website-ready user documentation lives in `docs/public/**`. Use `/docs-maintainer` when a change affects CLI commands, config keys, install/deploy flows, workflows, executors, public APIs, screenshots, or user-facing terminology. @@ -112,7 +119,7 @@ For multiline Markdown issue or PR bodies, write the body to a file and pass it with the relevant `gh ... --body-file ` option. Do not send escaped newlines through `--body`; GitHub will render them literally. -For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary ` for checks and unresolved-thread state, `scripts/pr-state --comment ` for a full review-comment body, `scripts/pr-resolve list ` for actionable unresolved review threads, and `scripts/pr-resolve reply --body-file ` to reply, resolve, and react in one call. Use `--body-file` whenever a reply has Markdown or shell-sensitive characters (especially backticks, `$`, or quotes); it prevents shell substitution from corrupting the GitHub reply. +For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary ` for checks and unresolved-thread state, `scripts/pr-state --comment ` for a full review-comment body, `scripts/pr-resolve list ` for actionable unresolved review threads, and `scripts/pr-resolve reply ""` to reply, resolve, and react in one call. When a Kandev system message references an MCP tool that is not visible in the active tool list, use the runtime's tool discovery mechanism, such as `tool_search` when available, before falling back to a less specific workflow. Some task messaging and platform helpers are exposed on demand. diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 13b1f09f5a..8d60bab518 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -275,27 +275,6 @@ Every long-running goroutine must have a single owner with explicit start and st You may still list the column in the `CREATE TABLE` so fresh DBs get it inline, but the migration is the source of truth for evolution and must stand alone. New columns also need: the struct field in `models/`, the DTO field + `ToAPI` in `pkg/api/v1/`, and every `CreateX`/`UpdateX`/bulk write in the repo that should set it. -## Internationalization - -`internal/i18n` covers only what Go renders **directly to a browser**, which today -is just the SPA-unavailable error pages. Everything else stays English by design — -the ~1,100 `http.Error` / `gin.H{"error": …}` strings are diagnostics the SPA maps -onto its own translated copy, and log lines, agent/ACP output, and CLI output are -not display copy. The shared-task artifacts (`share.html`, gist README and -description) are server-rendered and in scope for localization, but not done yet. - -- `i18n.T(locale, key)` for lookup, `i18n.FromRequest(r)` at the HTTP boundary. -- The package has no placeholder interpolation and no context-carried locale. - Both are needed for the share artifacts; add them with that work rather than - ahead of it. -- Catalogs are embedded JSON (`internal/i18n/locales/.json`). `pseudo` is - **generated** — run `pnpm run i18n:pseudo` from `apps/web`, which writes both - the frontend and backend catalogs. A per-package test fails if an `en` key is - missing from `pseudo`. -- **For new user-facing output, prefer returning a stable error code** the - frontend translates. Reach for `i18n.T` only when Go itself writes the bytes a - user reads. - ## Code-quality limits Enforced by `apps/backend/.golangci.yml` (errors on new code only): @@ -317,4 +296,5 @@ golangci-lint run ./... --new-from-rev="" --timeout=5m - `internal/agentctl/AGENTS.md` — agentctl server route groups, adapter model, ACP protocol - `internal/agentctl/server/api/AGENTS.md` — reverse-proxy body rewriting (`Accept-Encoding`), iframe-blocking header stripping - `internal/integrations/AGENTS.md` — playbook for adding a new third-party integration (Jira/Linear pattern) +- `docs/i18n.md` ("Backend") — `internal/i18n` covers only what Go renders straight to a browser (today: the SPA-unavailable error pages). Everything else stays English by design; for new user-facing output prefer a stable error code the frontend translates. - `cmd/mock-agent/AGENTS.md` — predefined `/e2e:` scenarios vs inline `e2e:...` scripts, recipe for adding a scenario, and the rebuild-before-e2e requirement diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index a59ff6e45a..8cd47324c4 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -26,7 +26,6 @@ import { Dialog } from "@kandev/ui/dialog"; - Use `useTouchDrawer` when a hover/popover disclosure needs a coarse-pointer `Drawer` alternative. Width-based phone composition and pointer-based disclosure behavior are related but not interchangeable. - Existing Radix DropdownMenu and ContextMenu surfaces receive inset, safe-area-aware bottom-sheet treatment below 640px in `app/globals.css`. Reuse those primitives for contextual actions and add focused coverage for long or nested menus instead of creating a parallel mobile menu. - Mobile capability parity does not require desktop layout parity. Load `/mobile-parity` for the Kandev surface decision guide, mobile design contract, and verification requirements. -- For container-query layouts, test just below and at the breakpoint plus the component's minimum supported width; preserve action order and prove clipped controls remain hit-target reachable. ## Data Flow Pattern (Critical) @@ -69,19 +68,6 @@ lib/api/domains/ # API clients **Hydration:** Go injects `window.__KANDEV_BOOT_PAYLOAD__` into the SPA shell before React mounts. `lib/state/hydration/merge-strategies.ts` has `deepMerge()`, `mergeSessionMap()`, `mergeLoadingState()` to avoid overwriting live client state. Pass `activeSessionId` to protect active sessions. -**Same-resource refreshes:** Preserve an already-loaded snapshot while -refreshing the same workspace or resource. Surface the refresh separately for -the local spinner, busy control, or disabled action; clear data only for the -initial load or an identity change. A deferred hook test should show stale -content with `loading=true`, and desktop/mobile E2E should keep the content -visible during refresh. - -**Live update provenance:** A WebSocket snapshot may omit optional fields that -the boot payload or hydrated store already established. Merge those fields in -provenance order—live payload, persisted session metadata, then existing store -value—rather than replacing known immutable metadata with `undefined`. Add a -handler regression test for an omitted-field update. - For rebasing or finishing PRs written against the old Next.js runtime, follow [`docs/nextjs-spa-migration.md`](../../docs/nextjs-spa-migration.md). @@ -233,9 +219,16 @@ drifting back. Never delete an entry to make a build pass. Use `pnpm run lint:i18n ` to preview the guard on a path that is not on the list yet. -The rule also **only sees plain literals in JSX** — template literals, -`confirm()` arguments and copy in plain `.ts` helpers are invisible to it, so a -clean lint is not proof a file is done. `pnpm run i18n:check` gates key/catalog +Separately, `pnpm run i18n:ratchet` (pre-commit + CI) guards **new code +everywhere**, regardless of the allowlist: a file you added must be clean outright, +and a file you modified is judged on the lines you touched. Untouched literals are +never reported, so it cannot ask you to migrate code you did not write — the same +contract as `golangci-lint --new-from-rev` for Go. + +The rule **only sees literals in JSX** — `confirm()` arguments and copy in plain +`.ts` helpers are invisible to it — and it **skips anything assigned to a +SCREAMING_CASE identifier**, so `const ROWS = [{ label: "Disk usage" }]` passes +silently. A clean lint is not proof a file is done. `pnpm run i18n:check` gates key/catalog drift, `` tag indices, inline plurals and module-scope `t()`, and the **pseudo-locale** (Settings → General → Appearance, dev/e2e) is the completeness check — any plain-English text under it was never externalized. The tooling needs @@ -273,6 +266,3 @@ When you hit a limit, extract a helper function, custom hook, or sub-component. solely because `mouseenter` or `pointerMove` failed in jsdom. - In Playwright tests, avoid strict locators that assume only one `terminal-panel` or `.xterm` exists. Mobile and dockview layouts can mount multiple terminal instances; scope to the active panel or use `.first()` / `.last()` deliberately with a comment or helper. - Shared E2E helpers that inspect mounted React/DOM internals must be scoped to the active panel/container, not global selectors, because hidden or stale mounted panels can coexist in dock/mobile layouts. -- When seeded data can create multiple tasks or repositories, scope actions to - the seeded/API ID or an exact labelled container. Do not use `.first()` on a - generic action locator unless list order is the tested contract. diff --git a/apps/web/eslint.i18n.options.mjs b/apps/web/eslint.i18n.options.mjs index 0f2f64af1f..ac22347bbb 100644 --- a/apps/web/eslint.i18n.options.mjs +++ b/apps/web/eslint.i18n.options.mjs @@ -14,7 +14,12 @@ export const noLiteralStringOptions = { // now checked, so the `words.exclude` list below has to carry the // weight of separating copy from prop enum values. mode: "jsx-only", - "should-validate-template": false, + // Template literals in JSX ARE copy: `Select task ${title}`, `${label} tasks, + // over WIP limit`. Leaving them unchecked was the largest hole in the guard, and + // turning it on measured cheap — +2 to +5 findings per un-migrated directory, + // every one of them a real string, and zero new findings across the allowlist + // (className templates are already covered by the Tailwind patterns below). + "should-validate-template": true, // Brand/proper nouns and symbol-only strings are not translatable copy. // // NOTE: the plugin wraps each pattern as `^$` @@ -62,6 +67,14 @@ export const noLiteralStringOptions = { ".*(calc|env|url|var)\\(.*", "\\{.*\\}", "[A-Z][A-Z0-9_]*\\.[a-z]{2,4}", + // Static chunks of an interpolated DOM id. With + // `should-validate-template` on, the plugin checks each literal chunk of a + // template separately, so `startup-page-${v}-label` arrives as + // "startup-page-" and "-label" — kebab/snake fragments with a dangling + // separator, which the token patterns above do not cover. Copy chunks + // ("Select task ", " tasks, over WIP limit") carry a capital, a space or + // punctuation and so still get flagged. + "[a-z0-9]+(?:[-_][a-z0-9]*)*|[-_][a-z0-9]+(?:[-_][a-z0-9]*)*", // Single lowercase/camel/kebab tokens are prop enum values, // classnames, and identifiers (variant="ghost", side="top", // value="work-items") — never display copy, which is capitalized diff --git a/apps/web/package.json b/apps/web/package.json index 9a8b10713d..c3fb86e2e5 100644 --- a/apps/web/package.json +++ b/apps/web/package.json @@ -24,7 +24,8 @@ "e2e:capture": "cross-env CAPTURE_PR_ASSETS=true playwright test --config e2e/playwright.config.ts && tsx e2e/scripts/convert-assets.ts", "licenses:gen": "tsx scripts/generate-licenses.ts", "i18n:pseudo": "node scripts/generate-pseudo-locale.mjs", - "i18n:check": "node scripts/check-i18n-keys.mjs && node scripts/check-trans-indices.mjs && node scripts/check-inline-plurals.mjs && node scripts/check-module-scope-t.mjs" + "i18n:check": "node scripts/check-i18n-keys.mjs && node scripts/check-trans-indices.mjs && node scripts/check-inline-plurals.mjs && node scripts/check-module-scope-t.mjs", + "i18n:ratchet": "node scripts/run-i18n-ratchet.mjs" }, "dependencies": { "@base-ui/react": "^1.0.0", diff --git a/apps/web/scripts/check-guard-allowlist.mjs b/apps/web/scripts/check-guard-allowlist.mjs new file mode 100644 index 0000000000..947139f2d6 --- /dev/null +++ b/apps/web/scripts/check-guard-allowlist.mjs @@ -0,0 +1,99 @@ +#!/usr/bin/env node +/** + * The i18n guard allowlist may only grow. + * + * `i18nGuardFiles` in eslint.i18n.options.mjs is what makes migrated code stay + * migrated: once a path is listed, `i18next/no-literal-string` is an error there + * forever. That protection has one obvious failure mode — someone hits the error, + * deletes the entry, and the build goes green with the migration silently undone. + * docs/i18n.md says never to do that; this makes it mechanical. + * + * Removing an entry is legitimate exactly once: when a file is deleted or renamed. + * Both are detectable, so the check only complains when the path still exists. + * + * Usage: node scripts/check-guard-allowlist.mjs [--base ] + */ +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; + +import { git, REPO_ROOT, resolveBase, WEB_DIR } from "./lib/git-base.mjs"; + +const OPTIONS_PATH = "apps/web/eslint.i18n.options.mjs"; + +const resolved = resolveBase(); +if (resolved.skip) { + console.log(`⚠ guard allowlist check skipped — ${resolved.skip}`); + process.exit(0); +} +const { base } = resolved; + +let baseSource; +try { + // stderr silenced: "exists on disk, but not in " is the expected result + // for any branch forked before the options module landed, and printing a + // `fatal:` line there reads as a failure in CI logs when it is not one. + baseSource = git(["show", `${base}:${OPTIONS_PATH}`], { quiet: true }); +} catch { + // The options module does not exist at the base commit, so nothing can have + // been removed from it. True for any branch forked before i18n landed. + console.log("✓ guard allowlist — no baseline to compare (options module is new)"); + process.exit(0); +} + +/** + * The options module is a dependency-free set of exports, so it imports cleanly + * from a temp path. Parsing the array out with a regex would break the moment + * someone reformats it. + */ +async function guardFilesFrom(source) { + const file = path.join( + fs.mkdtempSync(path.join(os.tmpdir(), "kandev-i18n-guard-")), + "options.mjs", + ); + fs.writeFileSync(file, source); + try { + const module = await import(`file://${file}`); + return module.i18nGuardFiles ?? []; + } finally { + fs.rmSync(path.dirname(file), { recursive: true, force: true }); + } +} + +const before = await guardFilesFrom(baseSource); +const after = await guardFilesFrom(fs.readFileSync(path.join(REPO_ROOT, OPTIONS_PATH), "utf8")); + +const afterSet = new Set(after); +/** + * An entry may be removed once its path is genuinely gone — a deleted or renamed + * file, or a migrated directory that was later deleted wholesale. Only a removal + * whose target still EXISTS un-protects live code, so that is all we flag. + * + * Globs are resolved rather than assumed live: flagging every removed glob would + * block the legitimate flow of migrating `components/foo/**`, listing it, then + * deleting `foo/` entirely. + */ +const removed = before.filter((entry) => { + if (afterSet.has(entry)) return false; + if (/[*?[\]{}]/.test(entry)) { + return fs.globSync(entry, { cwd: WEB_DIR }).length > 0; + } + return fs.existsSync(path.join(WEB_DIR, entry)); +}); + +if (removed.length > 0) { + console.error( + `\n✖ ${removed.length} entr(ies) removed from i18nGuardFiles:\n\n` + + removed.map((entry) => ` ${entry}`).join("\n") + + `\n\n Those paths are still present, so removing them un-protects copy that` + + `\n was already migrated. If a literal there is failing lint, externalize it` + + `\n — do not drop the entry. See docs/i18n.md ("The guard is scoped").\n`, + ); + process.exit(1); +} + +const added = after.filter((entry) => !new Set(before).has(entry)); +console.log( + `✓ guard allowlist intact — ${after.length} entr(ies)` + + (added.length ? `, ${added.length} added` : ""), +); diff --git a/apps/web/scripts/check-new-code-i18n.mjs b/apps/web/scripts/check-new-code-i18n.mjs new file mode 100644 index 0000000000..f9d66c4e85 --- /dev/null +++ b/apps/web/scripts/check-new-code-i18n.mjs @@ -0,0 +1,129 @@ +#!/usr/bin/env node +/** + * Ratchet: NEW user-facing copy must go through `t()` / ``, everywhere. + * + * The eslint guard in eslint.config.mjs is an error only on `i18nGuardFiles` — + * paths already migrated — because a repo-wide error on a half-migrated codebase + * breaks every unrelated PR that adds a label. That leaves a hole this closes: + * a brand-new component under an un-migrated directory is currently unguarded. + * + * The fix is to judge the CHANGE rather than the FILE: + * + * - a file the change ADDED must be clean outright. It carries no legacy debt, + * so requiring zero literals costs nothing. + * - a file the change MODIFIED is judged only on the lines it touched. Existing + * literals elsewhere in the file are somebody else's migration, not this PR's + * problem — which is exactly why this can be repo-wide without a treadmill. + * + * This mirrors `golangci-lint run --new-from-rev` in .pre-commit-config.yaml, so + * it is the same contract the Go side already enforces. + * + * NOTE: this raises the floor, it does not seal the box. The rule only sees plain + * literals and template literals in JSX — `confirm()` arguments and copy returned + * from plain `.ts` helpers stay invisible. The pseudo-locale remains the + * completeness check (docs/i18n.md). + * + * Usage: + * node scripts/check-new-code-i18n.mjs [--base ] + * + * With no --base it uses `git merge-base HEAD origin/main`. CI passes the PR's + * base SHA explicitly, matching the Prettier step in frontend-tests.yml. + */ +import path from "node:path"; + +import { ESLint } from "eslint"; + +import { lineInRanges, parseAddedLineRanges } from "./lib/diff-ranges.mjs"; +import { git, REPO_ROOT, resolveBase, toPosixPath, WEB_DIR } from "./lib/git-base.mjs"; + +const WEB_PREFIX = "apps/web/"; + +/** Only UI source. Tests build fixtures out of literals on purpose. */ +function isCandidate(repoPath) { + if (!repoPath.startsWith(WEB_PREFIX)) return false; + const rel = repoPath.slice(WEB_PREFIX.length); + if (!/\.tsx?$/.test(rel)) return false; + if (/\.(test|spec)\.tsx?$/.test(rel)) return false; + if (rel.startsWith("e2e/") || rel.startsWith("scripts/")) return false; + return /^(components|app|hooks|lib|src)\//.test(rel); +} + +function changedFiles(base, filter) { + // --find-renames explicitly: rename detection defaults on since Git 2.9, but a + // repo with diff.renames=false would report a moved file as ADDED, and an + // added file is judged whole — demanding a full migration for a plain git mv. + return git(["diff", "--name-only", "--find-renames", `--diff-filter=${filter}`, base]) + .split("\n") + .map((line) => line.trim()) + .filter(isCandidate); +} + +const resolved = resolveBase(); +if (resolved.skip) { + console.log(`⚠ i18n new-code ratchet skipped — ${resolved.skip}`); + process.exit(0); +} +const { base } = resolved; + +const added = changedFiles(base, "A"); +// Renames and copies are judged like modifications, NOT like additions: moving a +// legacy file must not suddenly demand that its whole contents be migrated. A +// pure rename adds no lines and so reports nothing; a rename-plus-edit is judged +// on the edited lines, same as any other modification. +const modified = changedFiles(base, "MRC"); +const targets = [...added, ...modified]; + +if (targets.length === 0) { + console.log("✓ i18n new-code ratchet — no UI source added or modified"); + process.exit(0); +} + +// Line attribution only matters for modified files; added files are judged whole. +// +// Scoped to apps/web rather than to the individual files, because narrowing the +// pathspec to only the NEW path of a rename hides the matching deletion and git +// then reports the whole file as added — which would demand a full migration for +// a pure `git mv`. Keeping both sides in scope lets rename detection pair them, +// so a pure rename yields no hunks and reports nothing. +const addedRanges = + modified.length === 0 + ? new Map() + : parseAddedLineRanges(git(["diff", "--unified=0", "--find-renames", base, "--", WEB_PREFIX])); + +const eslint = new ESLint({ + cwd: WEB_DIR, + overrideConfigFile: path.join(WEB_DIR, "eslint.i18n.config.mjs"), +}); +const results = await eslint.lintFiles(targets.map((f) => path.join(REPO_ROOT, f))); + +const addedSet = new Set(added); +const violations = []; +for (const result of results) { + const repoPath = toPosixPath(path.relative(REPO_ROOT, result.filePath)); + const whole = addedSet.has(repoPath); + const ranges = addedRanges.get(repoPath); + for (const message of result.messages) { + if (message.ruleId !== "i18next/no-literal-string") continue; + if (!whole && !lineInRanges(ranges, message.line)) continue; + violations.push({ repoPath, whole, line: message.line, text: message.message }); + } +} + +if (violations.length === 0) { + console.log( + `✓ i18n new-code ratchet — ${added.length} added + ${modified.length} modified file(s) clean`, + ); + process.exit(0); +} + +console.error( + `\n✖ ${violations.length} hardcoded user-facing string(s) in new code.\n` + + ` New copy must go through t() / even in a directory that has not\n` + + ` been migrated yet. See docs/i18n.md ("TL;DR for adding a string").\n`, +); +for (const violation of violations) { + const scope = violation.whole ? "new file" : "changed line"; + console.error(` ${violation.repoPath}:${violation.line} [${scope}] ${violation.text}`); +} +console.error(""); +process.exit(1); diff --git a/apps/web/scripts/lib/diff-ranges.mjs b/apps/web/scripts/lib/diff-ranges.mjs new file mode 100644 index 0000000000..844750c2d6 --- /dev/null +++ b/apps/web/scripts/lib/diff-ranges.mjs @@ -0,0 +1,67 @@ +/** + * Parse `git diff --unified=0` into the set of line numbers a change ADDED, so a + * lint finding can be attributed to the change that introduced it rather than to + * the file it happens to live in. + * + * This is what lets the i18n literal guard apply repo-wide to new code without + * demanding that the surrounding legacy file be migrated first — the same model + * as `golangci-lint --new-from-rev`, which this repo already uses for Go. + */ + +/** Hunk header: `@@ -oldStart[,oldCount] +newStart[,newCount] @@ optional context` */ +const HUNK = /^@@ -\d+(?:,\d+)? \+(\d+)(?:,(\d+))? @@/; + +/** + * Map of repo-relative path -> array of inclusive `[start, end]` line ranges that + * the diff added, in the NEW file's numbering. + * + * Deleted files and pure-deletion hunks contribute nothing: there is no new line + * to attribute a finding to. + * + * @param {string} diffText output of `git diff --unified=0 ...` + * @returns {Map>} + */ +export function parseAddedLineRanges(diffText) { + const ranges = new Map(); + let current = null; + + for (const line of diffText.split("\n")) { + // Every file entry starts here, so reset first. Entries that emit no `+++` + // header at all — a 100%-similarity rename, a binary file, a mode-only + // change — would otherwise leave the PREVIOUS file selected, and any hunk + // that followed would be attributed to it. Not reachable with today's git + // output, but it is a one-line invariant instead of a standing assumption. + if (line.startsWith("diff --git ")) { + current = null; + continue; + } + if (line.startsWith("+++ ")) { + const target = line.slice(4).trim(); + // `+++ /dev/null` marks a deletion; `+++ b/path` is the new file. + current = target === "/dev/null" ? null : target.replace(/^b\//, ""); + if (current && !ranges.has(current)) ranges.set(current, []); + continue; + } + if (!current || !line.startsWith("@@")) continue; + + const match = HUNK.exec(line); + if (!match) continue; + const start = Number(match[1]); + // A missing count means exactly one line; an explicit 0 means deletion-only. + const count = match[2] === undefined ? 1 : Number(match[2]); + if (count > 0) ranges.get(current).push([start, start + count - 1]); + } + + return ranges; +} + +/** + * Whether `line` falls inside any of `ranges`. + * + * @param {Array<[number, number]>} ranges + * @param {number} line + */ +export function lineInRanges(ranges, line) { + if (!ranges) return false; + return ranges.some(([start, end]) => line >= start && line <= end); +} diff --git a/apps/web/scripts/lib/diff-ranges.test.ts b/apps/web/scripts/lib/diff-ranges.test.ts new file mode 100644 index 0000000000..06dfa95847 --- /dev/null +++ b/apps/web/scripts/lib/diff-ranges.test.ts @@ -0,0 +1,136 @@ +import { describe, expect, it } from "vitest"; + +import { lineInRanges, parseAddedLineRanges } from "./diff-ranges.mjs"; + +const OLD_A = "--- a/a.tsx"; +const NEW_A = "+++ b/a.tsx"; + +describe("parseAddedLineRanges", () => { + it("reads a single-line hunk with no explicit count", () => { + const diff = [OLD_A, NEW_A, "@@ -4 +4 @@", "-old", "+new"].join("\n"); + expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([[4, 4]]); + }); + + it("reads a multi-line hunk", () => { + const diff = [OLD_A, NEW_A, "@@ -10,0 +11,3 @@", "+x", "+y", "+z"].join("\n"); + expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([[11, 13]]); + }); + + it("ignores deletion-only hunks, which add no attributable line", () => { + const diff = [OLD_A, NEW_A, "@@ -5,2 +4,0 @@", "-gone", "-also"].join("\n"); + expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([]); + }); + + it("ignores deleted files", () => { + const diff = ["--- a/gone.tsx", "+++ /dev/null", "@@ -1,2 +0,0 @@", "-a", "-b"].join("\n"); + expect(parseAddedLineRanges(diff).has("gone.tsx")).toBe(false); + }); + + it("keeps hunks separate across several files", () => { + const diff = [ + "diff --git a/a.tsx b/a.tsx", + OLD_A, + NEW_A, + "@@ -1 +1 @@", + "+a", + "diff --git a/b.tsx b/b.tsx", + "--- a/b.tsx", + "+++ b/b.tsx", + "@@ -20,0 +21,2 @@", + "+b", + "+c", + ].join("\n"); + const ranges = parseAddedLineRanges(diff); + expect(ranges.get("a.tsx")).toEqual([[1, 1]]); + expect(ranges.get("b.tsx")).toEqual([[21, 22]]); + }); + + it("collects multiple hunks in one file", () => { + const diff = [OLD_A, NEW_A, "@@ -1 +1 @@", "+a", "@@ -50,0 +51,2 @@", "+b", "+c"].join("\n"); + expect(parseAddedLineRanges(diff).get("a.tsx")).toEqual([ + [1, 1], + [51, 52], + ]); + }); + + it("does not mistake a '+++' inside added content for a file header", () => { + // A line of added source that begins with "+++" arrives as "++++...". + const diff = [OLD_A, NEW_A, "@@ -1,0 +1,1 @@", "++++ not a header"].join("\n"); + const ranges = parseAddedLineRanges(diff); + expect([...ranges.keys()]).toEqual(["a.tsx"]); + expect(ranges.get("a.tsx")).toEqual([[1, 1]]); + }); + + it("returns nothing for an empty diff", () => { + expect(parseAddedLineRanges("").size).toBe(0); + }); + + it("ignores a pure rename, which emits no headers or hunks", () => { + // A plain `git mv` must cost nothing: attributing the moved file's existing + // lines would demand a full migration for a file nobody edited. + const diff = [ + "diff --git a/old.tsx b/new.tsx", + "similarity index 100%", + "rename from old.tsx", + "rename to new.tsx", + ].join("\n"); + expect(parseAddedLineRanges(diff).size).toBe(0); + }); + + it("does not attribute a later hunk to the file before a headerless entry", () => { + // A binary entry emits no `+++`, so without resetting on `diff --git` the + // previous file would stay selected and absorb the next hunk. + const diff = [ + "diff --git a/a.tsx b/a.tsx", + OLD_A, + NEW_A, + "@@ -1 +1 @@", + "+a", + "diff --git a/img.png b/img.png", + "Binary files a/img.png and b/img.png differ", + "@@ -99,0 +99,3 @@", + "+not really a hunk", + ].join("\n"); + const ranges = parseAddedLineRanges(diff); + expect(ranges.get("a.tsx")).toEqual([[1, 1]]); + expect(ranges.has("img.png")).toBe(false); + }); + + it("keeps a rename-plus-edit scoped to the edited lines only", () => { + const diff = [ + "diff --git a/old.tsx b/new.tsx", + "similarity index 92%", + "rename from old.tsx", + "rename to new.tsx", + "--- a/old.tsx", + "+++ b/new.tsx", + "@@ -2,0 +3 @@", + "+const added = 1;", + ].join("\n"); + expect(parseAddedLineRanges(diff).get("new.tsx")).toEqual([[3, 3]]); + }); +}); + +describe("lineInRanges", () => { + const ranges: Array<[number, number]> = [ + [3, 5], + [10, 10], + ]; + + it("matches inclusive bounds", () => { + expect(lineInRanges(ranges, 3)).toBe(true); + expect(lineInRanges(ranges, 5)).toBe(true); + expect(lineInRanges(ranges, 10)).toBe(true); + }); + + it("rejects lines outside every range", () => { + expect(lineInRanges(ranges, 2)).toBe(false); + expect(lineInRanges(ranges, 6)).toBe(false); + expect(lineInRanges(ranges, 11)).toBe(false); + }); + + it("treats a missing range list as no match", () => { + // Reached when a modified file produced no attributable added lines. + expect(lineInRanges(undefined as unknown as Array<[number, number]>, 1)).toBe(false); + }); +}); diff --git a/apps/web/scripts/lib/git-base.mjs b/apps/web/scripts/lib/git-base.mjs new file mode 100644 index 0000000000..d059077f22 --- /dev/null +++ b/apps/web/scripts/lib/git-base.mjs @@ -0,0 +1,65 @@ +/** + * Shared git plumbing for the i18n ratchet checks. + * + * Both `check-new-code-i18n.mjs` and `check-guard-allowlist.mjs` need the same + * fork point and the same fail-closed policy about it, so it lives here rather + * than being duplicated and drifting. + */ +import { execFileSync } from "node:child_process"; +import path from "node:path"; + +export const WEB_DIR = path.resolve(import.meta.dirname, "..", ".."); +export const REPO_ROOT = path.resolve(WEB_DIR, "..", ".."); + +export function git(args, { quiet = false } = {}) { + return execFileSync("git", args, { + cwd: REPO_ROOT, + encoding: "utf8", + maxBuffer: 64 * 1024 * 1024, + stdio: ["ignore", "pipe", quiet ? "ignore" : "inherit"], + }); +} + +/** + * Git reports paths with `/` on every platform; `path.relative` returns `\` on + * Windows. Comparing the two silently matches nothing, which would make the + * ratchet discard every finding and report "clean" — the worst failure mode for + * a guard, because it looks like success. + */ +export function toPosixPath(value) { + return value.split(path.sep).join("/"); +} + +/** + * The fork point to judge against, or `null` when the caller explicitly allows + * running without one. + * + * Fail-closed: if the caller passed `--base`, or we are in CI, an unresolvable + * base means a broken checkout (usually a shallow clone that never fetched the + * base), and skipping would turn that into a silent pass. Only a bare local run + * — a developer with no `origin/main`, e.g. offline or in a fresh init repo — + * is allowed to skip, because failing there would block unrelated commits for + * an environment reason. + * + * @returns {{ base: string } | { skip: string }} + */ +export function resolveBase(argv = process.argv) { + const flag = argv.indexOf("--base"); + const explicit = flag !== -1 && argv[flag + 1] ? argv[flag + 1] : null; + const requested = explicit ?? "origin/main"; + + try { + return { base: git(["merge-base", requested, "HEAD"], { quiet: true }).trim() }; + } catch { + if (explicit || process.env.CI) { + console.error( + `\n✖ cannot resolve a base to compare against (tried "${requested}").\n` + + ` In CI this means the checkout is incomplete — a shallow clone that\n` + + ` never fetched the base branch. Refusing to pass silently.\n` + + ` Fetch the base ref, or pass --base .\n`, + ); + process.exit(1); + } + return { skip: `no base ref (tried "${requested}"); pass --base to check explicitly` }; + } +} diff --git a/apps/web/scripts/run-i18n-ratchet.mjs b/apps/web/scripts/run-i18n-ratchet.mjs new file mode 100644 index 0000000000..4372f04291 --- /dev/null +++ b/apps/web/scripts/run-i18n-ratchet.mjs @@ -0,0 +1,18 @@ +#!/usr/bin/env node +/** + * Runs both ratchet checks, forwarding CLI args to each. + * + * `pnpm run i18n:ratchet --base ` used to append the flag to the last token + * of a `&&` chain, so only the second script saw it and the two disagreed about + * which base they were judging. A tiny runner keeps them in step. + */ +import { spawnSync } from "node:child_process"; +import path from "node:path"; + +const args = process.argv.slice(2); +for (const script of ["check-new-code-i18n.mjs", "check-guard-allowlist.mjs"]) { + const result = spawnSync(process.execPath, [path.join(import.meta.dirname, script), ...args], { + stdio: "inherit", + }); + if (result.status !== 0) process.exit(result.status ?? 1); +} diff --git a/docs/i18n.md b/docs/i18n.md index fb37bba044..38be0dccdc 100644 --- a/docs/i18n.md +++ b/docs/i18n.md @@ -7,9 +7,18 @@ generated **pseudo-locale** is used for QA. Requirements live in The migration is **in progress, one directory per PR**. The runtime, the gates, and Settings → General → Appearance are done; everything else still holds -English literals. See [Migrating a directory](#migrating-a-directory) for how to -convert one, and [The guard is scoped](#the-guard-is-scoped-on-purpose) for why -new code elsewhere is not blocked. +English literals. + +Two enforcement layers, and it matters which is which: + +- **New code is guarded everywhere.** Copy you add — in a new file, or on a line + you changed — must go through `t()` / `` whatever directory it is in. + See [the new-code ratchet](#but-new-code-is-guarded-everywhere). +- **Whole files are guarded only once migrated.** The eslint literal rule errors + across an entire file only for the paths in `i18nGuardFiles`, so you are never + asked to migrate lines you did not touch. See + [The guard is scoped](#the-guard-is-scoped-on-purpose) and + [Migrating a directory](#migrating-a-directory). ## TL;DR for adding a string @@ -149,6 +158,7 @@ cd apps/web pnpm run i18n:check # the four repo-wide gates (runs in CI: Frontend Tests) pnpm run i18n:pseudo # regenerate the pseudo catalog from en pnpm run lint:i18n # preview the guard on a path that is NOT yet migrated +pnpm run i18n:ratchet # new-code guard + allowlist-only-grows (pre-commit + CI) # Codemods — always inspect the diff; each declines what it cannot prove safe. node scripts/externalize-strings.mjs --write components app hooks lib @@ -205,7 +215,29 @@ Scoping inverts it. The guard can only ever tighten: Entries are globs, so use a file glob while a directory is partly migrated and collapse to `dir/**` once it is done. **Never remove an entry** to make a build -pass — that silently un-protects migrated copy. +pass — that silently un-protects migrated copy. `check-guard-allowlist.mjs` +enforces this: it fails if an entry disappears while its path still exists. + +### …but new code is guarded everywhere + +Scoping the whole-file guard leaves a hole: a brand-new component under an +un-migrated directory would be unchecked. `check-new-code-i18n.mjs` closes it by +judging the **change** rather than the file: + +| | Judged on | +|---|---| +| A file the change **added** | the whole file — it carries no legacy debt | +| A file the change **modified** | only the lines it touched | + +Existing literals elsewhere in a modified file are somebody else's migration, so +this can be repo-wide without recreating the treadmill. It is the same contract +`golangci-lint --new-from-rev` already enforces for Go in +`.pre-commit-config.yaml`, and it runs in both pre-commit and CI. + +So there are two independent ratchets, and both only tighten: + +- **`i18nGuardFiles`** — whole files, migrated paths. Grows one directory per PR. +- **the new-code ratchet** — new lines, everywhere. Always on. ## Migrating a directory @@ -232,9 +264,27 @@ pnpm run lint:i18n components/ # 5. clean? ### The guard has blind spots -`i18next/no-literal-string` only inspects **plain string literals in JSX**. It -cannot see a template literal (``aria-label={`Remove ${x}`}``), a `confirm()` -argument, or copy returned from a plain `.ts` helper. `externalize-strings.mjs` +`i18next/no-literal-string` only inspects **literals in JSX**. Template literals +*are* now checked (`should-validate-template: true` — measured at +2 to +5 +findings per un-migrated directory, all of them real copy), but it still cannot +see a `confirm()` argument or copy returned from a plain `.ts` helper. + +It has one more blind spot worth knowing, because it is the shape config tables +take: **a literal assigned to a SCREAMING_CASE identifier is skipped entirely.** + +```tsx +const HEADER = Archive board; // NOT flagged +const header = Archive board; // flagged +const ROWS = [{ label: "Disk usage" }]; // NOT flagged +``` + +That is why module-scope config objects survived the sweep, and why the +new-code ratchet inherits the same hole. `check-module-scope-t.mjs` catches the +adjacent case (a module-scope `t()` call, frozen at the boot locale) but not +module-scope literal copy. Reviewing a new `const TABLE = [...]` of labels by eye +is still required. + +`externalize-strings.mjs` covers those shapes — it walks `.ts` as well as `.tsx` and handles template literals and dialog/toast arguments — but **a zero from `pnpm lint` does not by itself mean a file is fully externalized**. The pseudo-locale is the real