docs(rules): guardrail against raw-key i18n leaks (follow-up to #2501)#2502
docs(rules): guardrail against raw-key i18n leaks (follow-up to #2501)#2502merchako wants to merge 3 commits into
Conversation
Adversarial review feedback on #2502: - Rule file: cite BookChapterControl/CommentEditor (verified direct English-fallback reads) instead of CommentList (which only forwards the prop; the fallback lives in its child CommentThread); describe the `strings[key] ?? key` resolver as a copy-pasted local helper, not a single shared one; collapse the triplicated lint caveat to one line + link; trim to house length. - react-patterns.md: restructure "What's Enforced by Linting" into explicit CI-enforced vs. advisory-only groups so the heading no longer implies the advisory rules are enforced. - Localization-Guide.md: fix the adjacent overstatement that `paranext/no-hardcoded-jsx-strings` "is enforced" — it runs only under lint:ai-strict.
merchako
changed the title
…ment claim Adds `.claude/rules/code-quality/localized-string-fallbacks.md` codifying two guardrails for `lib/platform-bible-react/` components, so agents stop reintroducing the raw-key leak just fixed in the ScopeSelector/Inventory stories (PR #2501): - Resolve missing localized strings to an English default (`?? 'Range'`), never to the key (`?? key`), which renders the raw `%…%` token on screen. - Type story fixtures as `Required<XxxLocalizedStrings>` so a dropped key fails to compile. Also corrects `.claude/rules/architecture/react-patterns.md` "What's Enforced by Linting": the localization/ARIA/color `paranext/*` rules run only under `npm run lint:ai-strict` (`.eslintrc.ai.js`) — not the CI `npm run lint` — and are disabled for stories/tests, so they are not a safety net. Adds a cross-reference from the Localization-Guide, flagging that several listed components currently deviate with a raw-key fallback (cleanup tracked separately).
Adversarial review feedback on #2502: - Rule file: cite BookChapterControl/CommentEditor (verified direct English-fallback reads) instead of CommentList (which only forwards the prop; the fallback lives in its child CommentThread); describe the `strings[key] ?? key` resolver as a copy-pasted local helper, not a single shared one; collapse the triplicated lint caveat to one line + link; trim to house length. - react-patterns.md: restructure "What's Enforced by Linting" into explicit CI-enforced vs. advisory-only groups so the heading no longer implies the advisory rules are enforced. - Localization-Guide.md: fix the adjacent overstatement that `paranext/no-hardcoded-jsx-strings` "is enforced" — it runs only under lint:ai-strict.
f314077 to
e78be3f
Compare
Antagonistic AI-Assisted Review — PR #2502Provenance: Adversarial review generated with Claude Code. Every finding is backed by grep/read evidence against the actual source. Human reviewers should verify independently. Two-PR context: #2501 is the Storybook fix (merge-ready). This PR (#2502) is the guardrail rule. PT-4103 is the source fix (converting 6 remaining raw-key-fallback components to English defaults). #2502 and PT-4103 are designed to travel together — the rule tells agents/contributors what the contract is; PT-4103 makes the codebase conform to it. ESLint Enforcement ClaimsThe PR's main contribution is separating "CI-enforced" from "advisory-only" lint rules. I verified each claim against The localization/ARIA/colors split is correct. The
I searched This claim was in the original Governance:
|
Adversarial review caught that the rewritten "What's Enforced by Linting" section listed the Tailwind `tw:` prefix under "Enforced by CI" — but no lint rule enforces it. The prefix comes from the Tailwind v4 config (`prefix(tw)` in index.css + twMerge); a missing prefix is a silent no-op, not a build/lint error. Reframed the section as "what tooling catches (and doesn't)" and stated the prefix mechanism accurately, so this rule stops making the same kind of over-claim it exists to correct.
1 participant
Why
#2501 fixed a case where raw localization keys (e.g.
%webView_scope_selector_range%) leaked into the UI: severallib/platform-bible-react/components resolvelocalizedStrings[key] ?? key, so a missing key falls back to the raw key. That class of bug will keep recurring unless the guardrail is written down somewhere agents and contributors actually read.This PR is my stab at that guardrail — a starting point, not a finished spec. I'd like a dev to take it from here and shape it into whatever the team actually wants to enforce.
What's here (a first cut)
.claude/rules/code-quality/localized-string-fallbacks.md: resolve a missing string to an English default (?? 'Range'), never?? key; and type story fixturesRequired<XxxLocalizedStrings>so a dropped key fails to compile..claude/rules/architecture/react-patterns.md— it listed the localization/ARIAparanext/*rules under "What's Enforced by Linting," but CI'snpm run lintdoesn't run them (they live in.eslintrc.ai.js/lint:ai-strictand are off for stories). Restructured as CI-enforced vs. advisory-only..context/standards/Localization-Guide.md.Docs/rules only — no code paths change.
Open questions for whoever picks this up
Right home / right mechanism? Is a
.claude/rulesagent-rule the right vehicle, or should this be a real lint rule (e.g.paranext/no-raw-key-fallbackflaggingstrings[key] ?? key)? Honest caveat: theparanext/*plugin rules aren't wired into CI lint today, so a new rule buys nothing until.eslintrc.jsadopts the plugin's recommended config — a bigger, separate decision I didn't want to make unilaterally.How far to push the
Required<>story-fixture convention — just these components, or library-wide?The actual source fix (converting the 6 remaining raw-key components to English fallbacks, and correcting the Localization-Guide precedent list) is tracked in PT-4103 — this PR is only the guardrail/guidance, not the fix.
Governance / review path:
.context/standards/Localization-Guide.mdis a broader-review file per.claude/rules/where-to-add-guidance.md— standards changes need broader dev sign-off, not just a quick rubber-stamp. Route that file's change to the right reviewer.Pairs with PT-4103: the guide's "established precedent" list still names raw-key components as English-fallback exemplars; the⚠️ note this PR adds is a band-aid until PT-4103 converts those components and corrects the list. Treat docs(rules): guardrail against raw-key i18n leaks (follow-up to #2501) #2502 (the rule) and PT-4103 (the conformance) as a pair.
Verification
Docs only. The ESLint-enforcement claims were checked against
.eslintrc.js,.eslintrc.ai.js,package.json,.github/workflows/test.yml, andlib/eslint-plugin-paranext/.AI Involvement
AI-assisted (Claude Code): ran the retro that surfaced the guidance gap, drafted the rule, verified the config claims, and ran an adversarial review that tightened the wording. Reviewed before commit — but this is explicitly a draft for a human to own and finish.
This change is
Resume this session:
claude --resume 63aed4b7-897a-4465-80ad-b0aa34d4a0c2(run from thefix-localisation-in-storybook-scope-selectorworktree).