From 345e360ca8a56d4e22c50e7f758e531e2f73ba7e Mon Sep 17 00:00:00 2001 From: Hal Eisen Date: Tue, 8 Sep 2026 18:16:22 -0700 Subject: [PATCH] ADFA-5607: Nudge the documentation team on user-facing text changes Engineers ship strings through code review and QA without ever asking the documentation team. Two gaps caused it, and they need two mechanisms because one is path-matchable and the other is not. CODEOWNERS was inert. Every pattern carried a leading "./", which gitignore syntax does not match, so the documentation team was never auto-requested on a strings.xml change from 2025-04 until now. GitHub's codeowners/errors API reports no errors for this, so nothing ever surfaced it. Verified against history: PRs #1456, #1780 and #1781 all changed an owned strings.xml and the documentation team appears in none of their review_requested events, while other teams do. Also removed from CODEOWNERS: 8 rules for LayoutEditor paths deleted from the repo, and 15 translated values-XX rules. Translations are a separate review; wording is settled before it is translated. Left unowned on purpose: the vendored appintro subtree, the gradle-plugin and testing resource fixtures, logsender-sample, apk-viewer-plugin and markdown-preview-plugin. Inline literals cannot be covered by CODEOWNERS at all, since you cannot own "Kotlin files that happen to contain UI text". A new pre-push hook scans added lines in the push range instead. It follows 0002-architecture-review-nudge: non-blocking, exits 0 always, silent unless it matches. Its strings.xml list is kept identical to CODEOWNERS so the early nudge and the PR-time reviewer request agree. Measured over the last 150 commits on stage: 32 (21%) touch an owned strings.xml, and 4 contain inline literals. All 32 are resources/, which holds 1387 of the 1399 English strings; app/ (7 strings) and logsender/ (5) saw no change in that window but stay owned as cheap insurance. Of the 4 inline commits, two were real user-facing text and two were false positives - an empty setText("") and JGit's CommitCommand.setMessage under git-core/. Both filters are in the hook and both are pinned: removing either makes its commit produce a spurious hit. --- .githooks/pre-push/0003-strings-review-nudge | 76 ++++++++++++++++++++ CODEOWNERS | 32 +++------ 2 files changed, 85 insertions(+), 23 deletions(-) create mode 100755 .githooks/pre-push/0003-strings-review-nudge diff --git a/.githooks/pre-push/0003-strings-review-nudge b/.githooks/pre-push/0003-strings-review-nudge new file mode 100755 index 0000000000..d06a8fb2c9 --- /dev/null +++ b/.githooks/pre-push/0003-strings-review-nudge @@ -0,0 +1,76 @@ +#!/bin/bash + +# Non-blocking nudge: when a push changes user-facing text, remind the author to +# ask the documentation team (#documentation-request) before the PR goes out. +# +# Two triggers, because they need two mechanisms. English strings.xml files are +# path-matchable, so CODEOWNERS already auto-requests @appdevforall/documentation +# at PR time -- this hook just moves that signal earlier. Inline literals are NOT +# path-matchable (you cannot own "Kotlin files that happen to contain UI text"), +# so a content scan of the diff is the only way to catch them. +# +# This is a REMINDER, not a gate -- it always exits 0 and never blocks a push. + +set -u + +cyan=$(tput setaf 6 2>/dev/null || true) +yellow=$(tput setaf 3 2>/dev/null || true) +reset=$(tput sgr0 2>/dev/null || true) + +# Determine the commits being pushed. Prefer the tracked upstream; fall back to +# the integration branch (feature branches are based on stage). If neither is +# resolvable, stay quiet rather than nag. +if git rev-parse --abbrev-ref --symbolic-full-name '@{upstream}' >/dev/null 2>&1; then + range="@{upstream}..HEAD" +elif git rev-parse --verify -q origin/stage >/dev/null 2>&1; then + range="origin/stage..HEAD" +else + exit 0 +fi + +# The English source strings owned by the documentation team. Kept in step with +# CODEOWNERS on purpose: the hook and the PR-time reviewer request must agree. +strings=$(git diff --name-only "$range" -- \ + 'app/src/main/res/values/strings.xml' \ + 'resources/src/main/res/values/strings.xml' \ + 'logsender/src/main/res/values/strings.xml' 2>/dev/null) || exit 0 + +# Calls that put a literal in front of the user. The trailing [^"] rejects the +# empty literal, so clearing a field (editor?.setText("")) does not nag. +ui_text='android:(text|hint|contentDescription|title|summary|label)="[^@?"]' +ui_text+='|(setText|setContentText|setContentTitle|setTitle|setMessage|setSummary)\("[^"]' +ui_text+='|makeText\(.*, *"[^"]' +ui_text+='|(^|[^A-Za-z])Text\( *(text *= *)?"[^"]' + +# Added lines only ("^+", minus the "+++" file header), so pre-existing hardcoded +# text does not re-nag on every push. git-core is excluded because JGit's +# CommitCommand.setMessage writes a commit message, not UI text. +inline=$(git diff --unified=0 "$range" -- \ + '*.kt' '*.java' '*/res/layout/*.xml' \ + ':!git-core/' ':!*/src/test/*' ':!*/src/androidTest/*' 2>/dev/null \ + | grep -E '^\+' \ + | grep -vE '^\+\+\+' \ + | grep -E "$ui_text" \ + || true) + +[ -n "$strings" ] || [ -n "$inline" ] || exit 0 + +echo "" +echo "${cyan}[documentation nudge]${reset} this push changes user-facing text." + +if [ -n "$strings" ]; then + echo "${yellow} English strings.xml:${reset}" + printf '%s\n' "$strings" | sed 's/^/ /' +fi + +if [ -n "$inline" ]; then + echo "${yellow} Text written inline instead of in strings.xml:${reset}" + printf '%s\n' "$inline" | sed 's/^+[[:space:]]*/ /' +fi + +echo "${yellow} Post in ${cyan}#documentation-request${yellow} before opening the PR${reset}${yellow} -- the" +echo " documentation team would rather see wording now than after QA.${reset}" +echo "${yellow} (Reminder only -- your push continues.)${reset}" +echo "" + +exit 0 diff --git a/CODEOWNERS b/CODEOWNERS index 019cf9796a..7c3e8b07b7 100644 --- a/CODEOWNERS +++ b/CODEOWNERS @@ -1,23 +1,9 @@ -./logsender/src/main/res/values/strings.xml @appdevforall/documentation -./app/src/main/res/values/strings.xml @appdevforall/documentation -./resources/src/main/res/values-es-rES/strings.xml @appdevforall/documentation -./resources/src/main/res/values-zh-rCN/strings.xml @appdevforall/documentation -./resources/src/main/res/values-bn-rIN/strings.xml @appdevforall/documentation -./resources/src/main/res/values-de-rDE/strings.xml @appdevforall/documentation -./resources/src/main/res/values-ru-rRU/strings.xml @appdevforall/documentation -./resources/src/main/res/values-hi-rIN/strings.xml @appdevforall/documentation -./resources/src/main/res/values-pt-rBR/strings.xml @appdevforall/documentation -./resources/src/main/res/values/strings.xml @appdevforall/documentation -./resources/src/main/res/values-tr-rTR/strings.xml @appdevforall/documentation -./resources/src/main/res/values-ro-rRO/strings.xml @appdevforall/documentation -./resources/src/main/res/values-in-rID/strings.xml @appdevforall/documentation -./resources/src/main/res/values-ar-rSA/strings.xml @appdevforall/documentation -./resources/src/main/res/values-fr-rFR/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values-ru/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values-zh-rTW/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values-zh-rCN/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values-in/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values-pt-rBR/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/res/values-tr/strings.xml @appdevforall/documentation -./LayoutEditor/app/src/main/assets/strings.xml @appdevforall/documentation +# GitHub reads these as gitignore-style patterns. A leading "./" matches +# nothing, which is why every rule in this file was inert from 2025-04 until +# ADFA-5607: no strings.xml change ever auto-requested the documentation team. +# +# English source strings only. Translated values-XX/strings.xml files are a +# different review (wording is already settled by the time it is translated). +app/src/main/res/values/strings.xml @appdevforall/documentation +resources/src/main/res/values/strings.xml @appdevforall/documentation +logsender/src/main/res/values/strings.xml @appdevforall/documentation