-
-
Notifications
You must be signed in to change notification settings - Fork 3
fix(i18n): remove hardcoded strings; enforce mandatory pre-PR review #232
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
a8e3f02
351705e
9d4bf5e
8787e27
09aae4e
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -0,0 +1,34 @@ | ||||||||||||||||||||||||||||||
| name: PR Review Gate | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| on: | ||||||||||||||||||||||||||||||
| pull_request: | ||||||||||||||||||||||||||||||
| # "edited" re-runs the gate when the PR description changes, so the review report can be | ||||||||||||||||||||||||||||||
| # added after the PR exists and the check turns green without a new push. | ||||||||||||||||||||||||||||||
| types: [opened, edited, synchronize, reopened] | ||||||||||||||||||||||||||||||
|
Comment on lines
+3
to
+7
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [performance · medium] Suggestion:
Suggested change
|
||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| permissions: | ||||||||||||||||||||||||||||||
| contents: read | ||||||||||||||||||||||||||||||
|
|
||||||||||||||||||||||||||||||
| jobs: | ||||||||||||||||||||||||||||||
| review-gate: | ||||||||||||||||||||||||||||||
| runs-on: ubuntu-latest | ||||||||||||||||||||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [other · low] Suggestion:
Suggested change
Comment on lines
+12
to
+14
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [other · low] Suggestion:
Suggested change
|
||||||||||||||||||||||||||||||
| # Weblate opens automated translation PRs; they cannot run the agent-side review skill. | ||||||||||||||||||||||||||||||
| if: github.event.pull_request.user.login != 'weblate' | ||||||||||||||||||||||||||||||
|
Comment on lines
+12
to
+16
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [performance · low] Suggestion:
Suggested change
|
||||||||||||||||||||||||||||||
| steps: | ||||||||||||||||||||||||||||||
| - name: Require an approved pre-PR review | ||||||||||||||||||||||||||||||
| env: | ||||||||||||||||||||||||||||||
| PR_BODY: ${{ github.event.pull_request.body }} | ||||||||||||||||||||||||||||||
| run: | | ||||||||||||||||||||||||||||||
| if printf '%s' "$PR_BODY" | grep -qi 'pre-pr-review: approved'; then | ||||||||||||||||||||||||||||||
| echo "Approved pre-PR review is recorded in the PR description." | ||||||||||||||||||||||||||||||
| else | ||||||||||||||||||||||||||||||
| echo "::error::This PR has no approved pre-PR review." | ||||||||||||||||||||||||||||||
| echo "" | ||||||||||||||||||||||||||||||
| echo "Every PR must be reviewed by the repo-reviewer subagent before it is opened." | ||||||||||||||||||||||||||||||
| echo "Run the 'pre-pr-review' skill (see .opencode/skills/pre-pr-review/SKILL.md):" | ||||||||||||||||||||||||||||||
| echo " 1. Let the repo-reviewer subagent review the branch diff against origin/main." | ||||||||||||||||||||||||||||||
| echo " 2. Fix any blocking findings and re-review until the verdict is APPROVE." | ||||||||||||||||||||||||||||||
| echo " 3. Paste the reviewer's report into this PR's description inside the marker:" | ||||||||||||||||||||||||||||||
| echo " <!-- pre-pr-review: approved -->" | ||||||||||||||||||||||||||||||
| exit 1 | ||||||||||||||||||||||||||||||
| fi | ||||||||||||||||||||||||||||||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,96 @@ | ||
| --- | ||
| description: Read-only code reviewer with deep knowledge of the AndCode Android repository. Use via the pre-pr-review skill before opening a pull request. | ||
| mode: subagent | ||
| temperature: 0.1 | ||
| color: accent | ||
| permission: | ||
| edit: deny | ||
| webfetch: deny | ||
| task: deny | ||
| bash: | ||
| "*": deny | ||
| "git status*": allow | ||
| "git log*": allow | ||
| "git diff*": allow | ||
| "git show*": allow | ||
| "git branch*": allow | ||
| "git fetch*": allow | ||
| "ls*": allow | ||
| "rg *": allow | ||
| "grep *": allow | ||
| "cat *": allow | ||
| "wc *": allow | ||
| --- | ||
|
|
||
| You are the dedicated code reviewer for this repository (AndCode). You review changes like a | ||
| maintainer who knows the codebase inside out: you verify claims against the actual code instead of | ||
| trusting the diff alone. You never modify files; you only read and report. | ||
|
|
||
| ## Repository map (verify details against the code when reviewing) | ||
|
|
||
| - Android app, single Gradle module `app/`, Kotlin + Jetpack Compose, package root | ||
| `com.yugahashimoto.andcode`. JDK 17, AGP 8.x, compileSdk 35. | ||
| - `feature/` packages own screens and ViewModels: `chat`, `settings`, `workspace`, `onboarding`, | ||
| `schedule`, `assistant` (voice), `wakeword` (Vosk), `widget`, `activity`. | ||
| - `runtime/` owns agent backends: `OpenCodeBackend` interface; `runtime/local/` runs a PRoot-based | ||
| Linux environment on-device (`LocalRuntimeManager`, `LocalRuntimeInstaller`, | ||
| `ClaudeCodeController`, `AntigravityController`). Remote targets exist too. | ||
| - `core/` holds cross-cutting code: `core/api` (OpenCode HTTP/SSE client), `core/locale` | ||
| (app-language switching), `core/notification`, `core/diagnostics`, `core/security`. | ||
| - DI is Koin (`di/` modules) plus hand-rolled `ViewModelFactory` inside composables; both | ||
| construction paths must stay in sync when a ViewModel gains a dependency. | ||
| - `runtime_tools/` and `scripts/` generate the Android runtime assets; `pages/` is the website. | ||
|
|
||
| ## Hard rules of this repo (flag violations as blocking) | ||
|
|
||
| 1. **i18n**: English source is `app/src/main/res/values/strings.xml`; every key must also exist in | ||
| every `values-*/strings.xml` (ar, es, fr, ja, pt-rBR, ru, zh-rCN) unless marked | ||
| `translatable="false"` — `.github/workflows/i18n-check.yml` fails otherwise. No hardcoded | ||
| user-visible text in Kotlin or XML: use `stringResource`/`getString`. LLM prompts and log | ||
| messages stay English. ViewModels receive user-visible messages through constructor injection | ||
| with an English default (see `WorkspaceViewModel.incompleteConnectionMessage`, | ||
| `McpViewModel.authNotRemovedMessage`) or a `*Messages` interface with an | ||
| `Android*Messages(context)` implementation (see `LocalRuntimeMessages`). | ||
| 2. **Formatting/static analysis**: spotless (ktlint 1.2.1) and detekt (`config/detekt/`) run in CI; | ||
| code must pass `./gradlew detekt spotlessCheck`. | ||
| 3. **Tests**: JUnit4 unit tests under `app/src/test`; behavior changes need test updates. | ||
| 4. **No secrets** in code or config; GitHub OAuth client id comes from build config/env. | ||
| 5. **Comments**: the repo favors explanatory comments for non-obvious decisions; do not demand | ||
| their removal, and do not demand adding boilerplate comments. | ||
| 6. **Worktree rule**: changes must live on a branch created by `scripts/new-worktree.sh` | ||
| (based on `origin/main`), never directly on the main working tree. | ||
|
|
||
| ## Review procedure | ||
|
|
||
| 1. Establish scope: `git fetch origin main` then `git diff --stat origin/main...HEAD` and the full | ||
| `git diff origin/main...HEAD`. Read every hunk. | ||
| 2. For each hunk, open the surrounding code (`rg`, reads) to check: callers, tests, DI wiring, | ||
| resource keys in all 8 locale files, and any interface/implementation pairs. | ||
| 3. Evaluate against, in order: correctness, regressions in adjacent behavior, i18n/localization, | ||
| Compose recomposition and state pitfalls, concurrency (Flow/coroutine scope leaks), resource | ||
| leaks (recognizers, receivers, streams), security (secrets, path traversal, intent handling), | ||
| test coverage, repo conventions above. | ||
| 4. Do not nitpick style that spotless/detekt already enforces. Do not relitigate established | ||
| patterns listed above. Prefer fewer, high-signal findings. | ||
|
|
||
| ## Output format (mandatory) | ||
|
|
||
| Respond in Japanese (the maintainer's language) with exactly this structure: | ||
|
|
||
| ``` | ||
| 判定: APPROVE | REQUEST_CHANGES | ||
|
|
||
| ## ブロッカー | ||
| - <file>:<line> — <problem> — <suggested fix> | ||
| (なければ「なし」) | ||
|
|
||
| ## 提案(非ブロッキング) | ||
| - ... | ||
| (なければ「なし」) | ||
|
|
||
| ## チェック済み項目 | ||
| - <what you verified and how, e.g. 全8ロケールのキー一致を確認> | ||
| ``` | ||
|
|
||
| Use `REQUEST_CHANGES` if and only if there is at least one ブロッカー. A finding is a ブロッカー | ||
| when it would fail CI, break behavior, regress localization, or violate the hard rules above. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,70 @@ | ||
| --- | ||
| name: pre-pr-review | ||
| description: Mandatory review gate before opening a pull request in this repository. Runs local CI gates and the repo-reviewer subagent on the full branch diff, iterates until the reviewer approves, and records the verdict in the PR description. Use whenever you are about to create a PR. | ||
| license: MIT | ||
| --- | ||
|
|
||
| # Pre-PR review (mandatory) | ||
|
|
||
| This repository requires an approved review BEFORE any pull request is created. The reviewer is | ||
| the `repo-reviewer` subagent, which carries the repository's architecture map and hard rules. The | ||
| gate is also enforced in CI (`.github/workflows/pr-review-gate.yml`): a PR whose description does | ||
| not contain an approved review report fails the check. | ||
|
|
||
| ## Workflow | ||
|
|
||
| 1. **Sync and scope the diff** | ||
|
|
||
| ```bash | ||
| git fetch origin main | ||
| git diff --stat origin/main...HEAD | ||
| ``` | ||
|
|
||
| If the branch has no commits ahead of `origin/main`, stop: there is nothing to review. | ||
|
|
||
| 2. **Run the local gates first** (cheap failures should never reach the reviewer): | ||
|
|
||
| ```bash | ||
| ./gradlew detekt spotlessCheck | ||
| ``` | ||
|
|
||
| If `app/src/main/res/values*/strings.xml` changed, also run the key-parity check from | ||
| `.github/workflows/i18n-check.yml` (every source key must exist in every locale file). | ||
| A full Gradle build may be impossible on-device (x86_64 aapt2 on an arm64 device); that is | ||
| expected — CI compiles. Fix everything that can run locally. | ||
|
|
||
| 3. **Invoke the reviewer** with the task tool, subagent type `repo-reviewer`. Prompt it with: | ||
|
|
||
| ``` | ||
| このブランチ(<branch name>、base: origin/main)のPR前レビューをお願いします。 | ||
| 変更概要: <one or two sentences describing the change and why> | ||
| ``` | ||
|
|
||
| The subagent reviews `git diff origin/main...HEAD` itself; do not paste the whole diff into the | ||
| prompt. | ||
|
|
||
| 4. **Handle the verdict** | ||
|
|
||
| - `REQUEST_CHANGES`: fix every ブロッカー, then re-run from step 3. Repeat until `APPROVE`. | ||
| Never open the PR while a ブロッカー is outstanding. | ||
| - `APPROVE`: proceed. Consider 提案(非ブロッキング) items; apply the cheap, safe ones. | ||
|
|
||
| 5. **Record the verdict in the PR description.** The PR body must contain the reviewer's report | ||
| verbatim inside the marker block below — the CI gate looks for the first line: | ||
|
|
||
| ```markdown | ||
| <!-- pre-pr-review: approved --> | ||
| ## Pre-PR review | ||
|
|
||
| <paste the reviewer's final report here> | ||
| ``` | ||
|
|
||
| If a later push changes the branch materially, re-run this skill and update the block | ||
| (`pre-pr-review: approved` must stay truthful for the HEAD commit). | ||
|
|
||
| ## Rules | ||
|
|
||
| - Do not skip, summarize away, or forge the reviewer report; the block must be the subagent's | ||
| actual output for the current HEAD. | ||
| - If the `repo-reviewer` subagent is unavailable, say so and stop — do not open the PR silently. | ||
| - Trivial bot PRs (e.g. Weblate translation sync) are exempt and are skipped by the CI gate. |
| Original file line number | Diff line number | Diff line change | ||||||||||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,28 +1,42 @@ | ||||||||||||||||||||||
| package com.yugahashimoto.andcode.feature.chat | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| import androidx.annotation.StringRes | ||||||||||||||||||||||
| import com.yugahashimoto.andcode.R | ||||||||||||||||||||||
| import com.yugahashimoto.andcode.core.api.OpenCodeCommand | ||||||||||||||||||||||
| import com.yugahashimoto.andcode.core.api.OpenCodeSkill | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| enum class SlashAction { NEW_CHAT, CLEAR, MODEL, AGENT, ATTACH, HELP } | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| data class SlashCommand( | ||||||||||||||||||||||
| val name: String, | ||||||||||||||||||||||
| val description: String, | ||||||||||||||||||||||
| @StringRes val descriptionRes: Int, | ||||||||||||||||||||||
| val action: SlashAction, | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * One entry in the composer's slash-command popup: either an app-level command or a command/skill | ||||||||||||||||||||||
| * the connected backend advertises. All of them end up inserting `/<name> ` into the input; the | ||||||||||||||||||||||
| * send path then routes backend commands and skills through the runtime's command handling. | ||||||||||||||||||||||
| * | ||||||||||||||||||||||
| * App commands carry a string resource so their description follows the app language; backend | ||||||||||||||||||||||
| * entries carry the text the backend advertised. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| sealed interface SlashSuggestion { | ||||||||||||||||||||||
| val name: String | ||||||||||||||||||||||
| val description: String | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| @get:StringRes | ||||||||||||||||||||||
| val descriptionRes: Int? | ||||||||||||||||||||||
| get() = null | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
| * [description] is empty by design: UI must resolve [descriptionRes] so the text follows the | ||||||||||||||||||||||
| * app language. Only backend entries carry literal text. | ||||||||||||||||||||||
| */ | ||||||||||||||||||||||
| data class App(val command: SlashCommand) : SlashSuggestion { | ||||||||||||||||||||||
| override val name: String = command.name | ||||||||||||||||||||||
| override val description: String = command.description | ||||||||||||||||||||||
| override val description: String = "" | ||||||||||||||||||||||
| override val descriptionRes: Int = command.descriptionRes | ||||||||||||||||||||||
| } | ||||||||||||||||||||||
|
Comment on lines
36
to
40
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [maintainability · low] Suggestion:
Suggested change
|
||||||||||||||||||||||
|
|
||||||||||||||||||||||
| data class Backend( | ||||||||||||||||||||||
|
|
@@ -35,12 +49,12 @@ sealed interface SlashSuggestion { | |||||||||||||||||||||
| object SlashCommandRegistry { | ||||||||||||||||||||||
| val commands: List<SlashCommand> = | ||||||||||||||||||||||
| listOf( | ||||||||||||||||||||||
| SlashCommand("/new", "Start a new session", SlashAction.NEW_CHAT), | ||||||||||||||||||||||
| SlashCommand("/clear", "Clear current conversation", SlashAction.CLEAR), | ||||||||||||||||||||||
| SlashCommand("/model", "Switch model", SlashAction.MODEL), | ||||||||||||||||||||||
| SlashCommand("/agent", "Switch agent", SlashAction.AGENT), | ||||||||||||||||||||||
| SlashCommand("/attach", "Attach a file", SlashAction.ATTACH), | ||||||||||||||||||||||
| SlashCommand("/help", "Show help", SlashAction.HELP), | ||||||||||||||||||||||
| SlashCommand("/new", R.string.slash_desc_new, SlashAction.NEW_CHAT), | ||||||||||||||||||||||
| SlashCommand("/clear", R.string.slash_desc_clear, SlashAction.CLEAR), | ||||||||||||||||||||||
| SlashCommand("/model", R.string.slash_desc_model, SlashAction.MODEL), | ||||||||||||||||||||||
| SlashCommand("/agent", R.string.slash_desc_agent, SlashAction.AGENT), | ||||||||||||||||||||||
| SlashCommand("/attach", R.string.slash_desc_attach, SlashAction.ATTACH), | ||||||||||||||||||||||
| SlashCommand("/help", R.string.slash_desc_help, SlashAction.HELP), | ||||||||||||||||||||||
| ) | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
| /** | ||||||||||||||||||||||
|
|
||||||||||||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[other · low]
synchronizeやeditedトリガーにより連続 push で同じジョブが重複実行されます。このジョブは PR ごとに結果が決まるゲートなので、concurrencyによるグループ化とcancel-in-progress: trueを設定し、直前の実行をキャンセルして冗長な実行を防ぐことを推奨します。Suggestion: