From 8723a6828b0f03605a7153410ce619f7a5c79dec Mon Sep 17 00:00:00 2001 From: Ritika Pahwa Date: Sun, 26 Jul 2026 18:56:09 +0530 Subject: [PATCH 1/6] docs: improve agent and testing guidance --- .github/agents/architecture-reviewer.md | 18 ++++ .github/agents/pr-agent.md | 108 +++++------------------- .github/copilot-instructions.md | 35 ++++++++ AGENTS.md | 51 +++++++++++ docs/testing-strategy.md | 36 ++++++++ 5 files changed, 162 insertions(+), 86 deletions(-) create mode 100644 .github/agents/architecture-reviewer.md create mode 100644 .github/copilot-instructions.md create mode 100644 AGENTS.md create mode 100644 docs/testing-strategy.md diff --git a/.github/agents/architecture-reviewer.md b/.github/agents/architecture-reviewer.md new file mode 100644 index 00000000000..48c8af47dc6 --- /dev/null +++ b/.github/agents/architecture-reviewer.md @@ -0,0 +1,18 @@ +--- +name: architecture-reviewer +description: Reviews Android pull requests for architectural regressions, behavior bugs, and missing integration coverage. +--- + +# Architecture Reviewer + +Review the complete pull-request diff in the context of the repository, not just changed lines. Read the affected callers, lifecycle owners, persistence/API interfaces, worker paths, flavor configuration, and existing tests. + +Evaluate Android and Kotlin choices against current official documentation, especially [Android architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), [Android security](https://developer.android.com/privacy-and-security), and [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html). Treat the Commons documentation repository as historical/project context when it is older or marked as draft. Flag obsolete APIs, unsupported workarounds, and compatibility decisions that are not explained for `minSdk = 21`. + +Prioritize findings that can cause user-visible regressions: incorrect state or lifecycle handling, races and cancellation bugs, data loss or duplicate uploads, broken offline/error behavior, API/min-SDK incompatibility, security/privacy leaks, performance regressions, and architectural boundary violations. Check both `prod` and `beta` when configuration or API behavior differs. + +For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage for the scenario, using `androidTest` or a Maestro flow for critical user journeys when JVM tests cannot exercise it. Treat missing tests as a finding only when the change has a meaningful untested behavior; avoid style-only comments. End with a short risk summary and list the validation commands actually run. + +This repository does not currently use Maestro; evaluate coverage using its existing JVM, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation layers. Check that tests are deterministic: no live backend is required for PR tests, no remote state is mutated, fixed sleeps are not used as synchronization, and setup helpers do not swallow assertion or view-matching failures. Flag `@Ignore` additions, weakened assertions, and unexplained test suppression as high-risk review findings. + +Use [`docs/testing-strategy.md`](../../docs/testing-strategy.md) to decide whether a test belongs in the PR gate or a scheduled/manual Beta workflow. A live-account or live-API test is not an acceptable substitute for a deterministic regression test. diff --git a/.github/agents/pr-agent.md b/.github/agents/pr-agent.md index d289386c4bb..68615fa6e1e 100644 --- a/.github/agents/pr-agent.md +++ b/.github/agents/pr-agent.md @@ -1,101 +1,37 @@ - --- name: pr-agent -description: An autonomous agent that creates review-ready PRs for Android/Kotlin projects. - -## Role -You are an autonomous **Android/Kotlin engineering agent**. -Your primary job is to create focused, review-ready pull requests that build, run, and pass tests. - -You write Kotlin code, add tests, generate documentation, and produce PR descriptions with clarity and reasoning. - ---- - -## Persona -- You specialize in Android app development using **Kotlin + Gradle** -- You write clear documentation, comprehensive tests, and maintainable code -- You understand Android architecture, testing patterns, UI behaviors and debugging -- Your output should result in **review-ready PRs** that are fully verifiable - ---- - -## Project Knowledge -- **Platform:** Android -- **Language:** Kotlin -- **Build System:** Gradle (Android Gradle Plugin) -- **Tests:** JUnit (unit) + AndroidX Test (instrumentation) - +description: Implements focused Android/Kotlin issues and produces review-ready pull requests. --- -## Responsibilities -You must: +# Pull Request Agent -1. Propose **small, well-scoped PRs** -2. Reference related issues when implementing (`Fixes #ID`) -3. Update or add tests for any functional behavior change -4. Run build + test commands before producing a final PR output -5. Generate human-readable commit messages and a complete PR summary +Follow the root `AGENTS.md` and `.github/copilot-instructions.md`. Work only on the requested issue; do not refactor unrelated legacy code. ---- - -## Knowledge of the Project -- **Platform:** Android -- **Language:** Kotlin -- **Build System:** Gradle -- **Tests:** Unit (JUnit), Instrumentation (AndroidX) -- **Target Output:** Working features + reproducible verification steps - ---- - -## Build + Validation Commands (MUST be executed mentally or simulated) -| Action | Command | -|---|---| -| Build project | `./gradlew assembleDebug` | -| Run unit tests | `./gradlew testDebugUnitTest` | -| Run instrumentation tests | `./gradlew connectedDebugAndroidTest` | -| Lint check | `./gradlew lint` | -| Full validation before PR | `./gradlew clean build connectedAndroidTest` | +Read [`docs/testing-strategy.md`](../../docs/testing-strategy.md) before modifying tests. This repository currently uses JUnit/Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation; do not introduce Maestro as an assumption. -You must NOT produce a PR that would fail these commands. +Use current official [Android Developers](https://developer.android.com/), [Android architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), and [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html) documentation as the standard. Consult the Commons documentation repository for app-specific context, but do not treat older or draft pages as current Android guidance. ---- - -## PR Format (Strict Requirement) - -Every PR you generate must include: +## Required workflow -### 🔹 Summary -1–2 sentences describing what changed +1. Translate the issue into acceptance criteria and identify affected feature packages, flavors, callers, and failure modes. +2. Inspect existing tests and recent history before choosing an implementation. +3. Implement the smallest compatible change. Preserve `prod` and `beta` behavior unless the issue says otherwise. +4. Add or update regression tests. Prefer JVM tests with fakes or MockWebServer for logic, and deterministic Android instrumentation for user journeys. Keep live Beta tests separate. +5. Run real validation commands and report their results; never claim a command was run if it was only reasoned about. -### 🔹 Motivation -Why the change was needed -Include issue reference: `Fixes #XYZ` +## Validation commands -### 🔹 Implementation Details -How the solution was implemented -Mention files touched and reasoning +```bash +./gradlew testProdDebugUnitTest +./gradlew lintProdDebug +./gradlew assembleProdDebug assembleBetaDebug +./gradlew connectedBetaDebugAndroidTest # when an emulator/device is available +``` -### 🔹 Tests -- Add/update unit tests for logic -- Add/update instrumentation tests for UI behavior -- If no tests are required, you must justify it +Use narrower tasks first when iterating. If an instrumentation test cannot run, explain why and provide manual verification steps. -### 🔹 Verification Steps -Write clear manual test instructions maintainers can follow - ---- +Never make a test pass by adding `@Ignore`, weakening assertions, swallowing exceptions, deleting coverage, or adding unexplained sleeps. If a test fails, classify the failure and fix the underlying product or test seam. Do not use real Wikimedia credentials or mutate remote Beta state in PR-gating tests. -## Code Standards for This Agent -| Type | Format | Example | -|---|---|---| -| Functions | `camelCase` | `loadReports()` | -| Classes | `PascalCase` | `ReportViewModel` | -| Constants | `UPPER_SNAKE_CASE` | `API_TIMEOUT_SEC` | -This is the official style guide you must follow: https://developer.android.com/kotlin/style-guide +## Pull request output -**Good Kotlin example** -```kotlin -fun loadUser(id: String): User { - require(id.isNotBlank()) { "id required" } - return repository.getUser(id) -} +Include: summary, motivation with `Fixes #ID` when applicable, implementation and architectural impact, tests run, known limitations, and manual verification steps. For UI changes include screenshots or recordings. Keep commits logically separate and use an area-prefixed subject where useful, such as `upload: prevent duplicate images`. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000000..d594a0d1927 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,35 @@ +# Commons Android Copilot Instructions + +Use the repository root `AGENTS.md` as the source of truth for structure, commands, coding conventions, security, and handoff requirements. This file adds Copilot-specific review and cloud-agent guidance. + +Read [`docs/testing-strategy.md`](../docs/testing-strategy.md) before changing tests. This repository does not currently use Maestro; use the existing JUnit, Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX test infrastructure unless the issue explicitly proposes a tool change. + +## Documentation authority + +For Android, Kotlin, Gradle, Compose, lifecycle, permissions, accessibility, security, and testing questions, consult current official documentation first: [Android Developers](https://developer.android.com/), [Android app architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), and [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html). Use the Commons app documentation repository for project history and domain context, not as a substitute for current platform documentation. Avoid obsolete APIs and undocumented workarounds; if compatibility with `minSdk = 21` requires one, explain it in the PR. + +## Before changing code + +- Identify the affected feature package, data/API boundaries, persistence, background work, and UI entry points. +- Read existing tests and the relevant flavor configuration (`prod` or `beta`); do not assume `assembleDebug` or `testDebugUnitTest` exists. +- Preserve compatibility with the app's minimum SDK and both product flavors unless the issue explicitly narrows scope. +- Determine whether the change belongs in JVM tests, deterministic emulator smoke tests, or live Beta integration tests. + +## Implementation expectations + +- Prefer the smallest change that fits the existing architecture. Do not introduce a new framework, module, or abstraction without explaining its ownership and migration path. +- Keep network, storage, and Android framework calls behind testable boundaries where practical. Avoid real network calls in unit tests. +- For functional changes, add a regression test. For UI or navigation changes, add an Android instrumentation test when feasible and include manual verification if automation is not yet possible. +- Do not weaken tests, suppress lint, alter credentials, or delete existing behavior to make CI pass. +- Do not use live Wikimedia accounts or remote mutable state for PR tests. Use fake sessions, test repositories, fixtures, or MockWebServer. +- Replace fixed sleeps and swallowed `NoMatchingViewException` failures with synchronization and explicit diagnostics. + +## Review expectations + +Flag only actionable issues, prioritizing: incorrect behavior, lifecycle/state bugs, concurrency, data loss, security/privacy, offline/error handling, API compatibility, performance, and missing regression coverage. Trace changes across callers and variants; a locally correct implementation that violates an architectural boundary is a finding. Do not report formatting preferences already enforced by the repository. + +## Verification + +Run the narrowest relevant Gradle task, then `./gradlew lintProdDebug` and the affected build task. If an emulator is available, run `./gradlew connectedBetaDebugAndroidTest`; otherwise state that instrumentation was not run and provide exact manual steps. + +Never claim a command was run if it was only reasoned about. Report the exact variant, device/API level, credentials or backend used, failed test class, and failure category. diff --git a/AGENTS.md b/AGENTS.md new file mode 100644 index 00000000000..61ac135d61a --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,51 @@ +# Repository Guidelines + +## Project Structure & Module Organization + +This is a single-module Android application. Gradle configuration lives in `settings.gradle.kts`, `build.gradle.kts`, and `app/build.gradle.kts`; dependency versions are centralized in `gradle/libs.versions.toml`. Production Kotlin code is under `app/src/main/java/fr/free/nrw/commons`, organized by feature (for example, `media`, `upload`, `category`, and `settings`). Android resources and localized strings are in `app/src/main/res`. JVM tests are in `app/src/test/kotlin`; instrumented tests, when present, belong in `app/src/androidTest`. Build variants include `prod` and `beta` flavors. + +## Build, Test, and Development Commands + +Use the checked-in Gradle wrapper and Java 17. + +- `./gradlew assembleProdDebug` — build the production debug APK. +- `./gradlew assembleBetaDebug` — build the beta debug APK. +- `./gradlew testProdDebugUnitTest` — run production-flavor JVM unit tests. +- `./gradlew connectedProdDebugAndroidTest` — run instrumented tests on a connected device or emulator. +- `./gradlew lintProdDebug` — run Android Lint for the production debug variant. +- `./gradlew jacocoTestReport` — generate the configured JaCoCo test coverage report, when the task is available. + +## Coding Style & Naming Conventions + +Write Kotlin with four-space indentation, clear nullability, and small, readable functions. Follow existing Android/Kotlin conventions: `PascalCase` for classes and composables, `camelCase` for methods and properties, and descriptive `*Test` or `*UnitTest` test classes. Keep feature code in its relevant package and place UI strings in resources rather than hard-coding them. Add reasonably descriptive Javadocs/KDocs for every new class and method; do not add `@author` tags. + +## Testing Guidelines + +Use the existing JUnit/Robolectric-style unit-test setup for JVM tests and AndroidX instrumentation for device tests. Name tests after the subject and behavior, such as `MediaClientTest` or `TransformGestureDetectorUnitTest`. Add or update tests for behavior changes, and run the narrowest relevant Gradle test task before broader validation. + +## Commit & Pull Request Guidelines + +Make separate commits for logically separate changes. Use a short imperative first line, optionally prefixed with the affected area (for example, `upload: prevent duplicate images`), followed by a meaningful body when context is needed. Pull requests should explain the problem and solution, include testing performed, link related issues, and provide screenshots or recordings for UI changes. Update relevant documentation or wiki content when behavior changes. + +## Security & Configuration + +Never commit passwords, API credentials, signing keys, decrypted keystores, or generated release artifacts. Review `SECURITY.md` before reporting or handling vulnerabilities, and use environment variables/local configuration for signing and test credentials. + +## Agent Workflow + +Treat this file as the shared baseline for Codex, Claude, Cursor, and Copilot. Read the relevant feature code, tests, Gradle configuration, and recent history before editing. Keep changes narrowly scoped, preserve existing public behavior unless the task says otherwise, and do not “fix” unrelated legacy code. For behavior changes, add a regression test or document why the behavior cannot yet be automated. Before handoff, run the narrowest relevant test, then `./gradlew lintProdDebug` and the affected build variant; report skipped checks and their reason. + +GitHub Copilot cloud agent can be assigned issues from GitHub and will open a pull request. Issues should state the user-visible behavior, affected flavor, acceptance criteria, and required tests. Reviewers should inspect architecture and failure modes—not only compilation or style—and require manual verification steps for flows not covered by automation. + +Agents must never make a test pass by adding `@Ignore`, weakening assertions, swallowing exceptions, deleting coverage, or increasing a fixed sleep without identifying the race. Classify failures as product, test, environment/toolchain, external-service/credential, or flaky-test failures. Keep live Wikimedia/Beta calls and tests that mutate remote state out of the normal PR gate; prefer fakes, test repositories, and MockWebServer for deterministic tests. See [`docs/testing-strategy.md`](docs/testing-strategy.md) for the test layers and escalation path. + +## Android Documentation Policy + +Use the current official documentation as the normative source for Android platform, Gradle, Kotlin, Compose, lifecycle, permissions, accessibility, security, and testing guidance: + +- [Android Developers](https://developer.android.com/) +- [Android app architecture](https://developer.android.com/topic/architecture) +- [Android testing](https://developer.android.com/training/testing) +- [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html) + +Check the relevant official page before introducing or recommending an API, pattern, or workaround. Prefer current stable APIs and document any deliberate compatibility decision for this app's `minSdk = 21`. The Commons documentation repository is useful project context, but it may be older or contain drafts; it must not override current official Android guidance without an explicit project decision. diff --git a/docs/testing-strategy.md b/docs/testing-strategy.md new file mode 100644 index 00000000000..df98f32455c --- /dev/null +++ b/docs/testing-strategy.md @@ -0,0 +1,36 @@ +# Android Testing Strategy + +This repository uses existing JUnit/Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation infrastructure. Maestro is not part of the current testing baseline. + +## Test layers + +| Layer | Scope | Dependencies | Expected trigger | +| --- | --- | --- | --- | +| JVM tests | Models, presenters, clients, persistence, upload logic | Fakes, fixtures, MockWebServer | Every pull request | +| Emulator smoke tests | Login screen, navigation, permissions, orientation, critical screen behavior | Deterministic test session and local fixtures | Every pull request once stable | +| Beta integration tests | Real authentication, Wikimedia API compatibility, upload behavior | Dedicated Beta credentials and remote state | Manual or scheduled workflow | +| Release validation | Production-like build and critical user journeys | Release candidate environment | Release candidate | + +## Rules for reliable tests + +- Do not require live Wikimedia services or mutable remote state in PR-gating tests. +- Prefer realistic fake repositories and test implementations over verifying mock call sequences. +- Use MockWebServer for HTTP behavior and reset preferences/database state between tests. +- Replace fixed sleeps with Espresso synchronization, explicit polling with timeouts, or testable worker/network state. +- Do not catch and ignore `NoMatchingViewException` or other setup/assertion failures. +- Do not add `@Ignore`, weaken assertions, or suppress a test to make CI pass. + +## Commands + +```bash +./gradlew testProdDebugUnitTest +./gradlew lintProdDebug +./gradlew assembleProdDebug assembleBetaDebug +./gradlew connectedBetaDebugAndroidTest +``` + +Use a focused test class or method while iterating. Record the exact variant, emulator/API level, backend, credentials mode, and command for every result. Classify failures as product, test, environment/toolchain, external-service/credential, or flaky-test failures before changing code. + +## Stabilization order + +When an instrumentation test is failing, first make the failure observable, then remove timing and external-service dependencies, then add deterministic fixtures or a fake session. Promote one stable test group to the PR gate at a time. Keep upload and other remote-mutating tests outside the PR gate until isolation and cleanup are proven. From 1ec64a875dc7705bae455942e32ac2ba56884739 Mon Sep 17 00:00:00 2001 From: Ritika Pahwa Date: Sun, 26 Jul 2026 19:04:17 +0530 Subject: [PATCH 2/6] chore: make Copilot agents discoverable --- ...{architecture-reviewer.md => architecture-reviewer.agent.md} | 2 +- .github/agents/{pr-agent.md => pr-agent.agent.md} | 0 .github/copilot-instructions.md | 2 +- AGENTS.md | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename .github/agents/{architecture-reviewer.md => architecture-reviewer.agent.md} (88%) rename .github/agents/{pr-agent.md => pr-agent.agent.md} (100%) diff --git a/.github/agents/architecture-reviewer.md b/.github/agents/architecture-reviewer.agent.md similarity index 88% rename from .github/agents/architecture-reviewer.md rename to .github/agents/architecture-reviewer.agent.md index 48c8af47dc6..5daf1367e84 100644 --- a/.github/agents/architecture-reviewer.md +++ b/.github/agents/architecture-reviewer.agent.md @@ -11,7 +11,7 @@ Evaluate Android and Kotlin choices against current official documentation, espe Prioritize findings that can cause user-visible regressions: incorrect state or lifecycle handling, races and cancellation bugs, data loss or duplicate uploads, broken offline/error behavior, API/min-SDK incompatibility, security/privacy leaks, performance regressions, and architectural boundary violations. Check both `prod` and `beta` when configuration or API behavior differs. -For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage for the scenario, using `androidTest` or a Maestro flow for critical user journeys when JVM tests cannot exercise it. Treat missing tests as a finding only when the change has a meaningful untested behavior; avoid style-only comments. End with a short risk summary and list the validation commands actually run. +For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage for the scenario, using Android instrumentation for critical user journeys when JVM tests cannot exercise it. Treat missing tests as a finding only when the change has a meaningful untested behavior; avoid style-only comments. End with a short risk summary and list the validation commands actually run. This repository does not currently use Maestro; evaluate coverage using its existing JVM, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation layers. Check that tests are deterministic: no live backend is required for PR tests, no remote state is mutated, fixed sleeps are not used as synchronization, and setup helpers do not swallow assertion or view-matching failures. Flag `@Ignore` additions, weakened assertions, and unexplained test suppression as high-risk review findings. diff --git a/.github/agents/pr-agent.md b/.github/agents/pr-agent.agent.md similarity index 100% rename from .github/agents/pr-agent.md rename to .github/agents/pr-agent.agent.md diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d594a0d1927..dd4ed60d611 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,6 +1,6 @@ # Commons Android Copilot Instructions -Use the repository root `AGENTS.md` as the source of truth for structure, commands, coding conventions, security, and handoff requirements. This file adds Copilot-specific review and cloud-agent guidance. +Use the repository root `AGENTS.md` as the source of truth for structure, commands, coding conventions, security, and handoff requirements. Custom Copilot agents are defined in `.github/agents/*.agent.md`; use `pr-agent.agent.md` for implementation and `architecture-reviewer.agent.md` for focused review tasks. This file adds Copilot-specific review and cloud-agent guidance. Read [`docs/testing-strategy.md`](../docs/testing-strategy.md) before changing tests. This repository does not currently use Maestro; use the existing JUnit, Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX test infrastructure unless the issue explicitly proposes a tool change. diff --git a/AGENTS.md b/AGENTS.md index 61ac135d61a..959719605bd 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -35,7 +35,7 @@ Never commit passwords, API credentials, signing keys, decrypted keystores, or g Treat this file as the shared baseline for Codex, Claude, Cursor, and Copilot. Read the relevant feature code, tests, Gradle configuration, and recent history before editing. Keep changes narrowly scoped, preserve existing public behavior unless the task says otherwise, and do not “fix” unrelated legacy code. For behavior changes, add a regression test or document why the behavior cannot yet be automated. Before handoff, run the narrowest relevant test, then `./gradlew lintProdDebug` and the affected build variant; report skipped checks and their reason. -GitHub Copilot cloud agent can be assigned issues from GitHub and will open a pull request. Issues should state the user-visible behavior, affected flavor, acceptance criteria, and required tests. Reviewers should inspect architecture and failure modes—not only compilation or style—and require manual verification steps for flows not covered by automation. +GitHub Copilot cloud agent can be assigned issues from GitHub and will open a pull request. Issues should state the user-visible behavior, affected flavor, acceptance criteria, and required tests. Select `.github/agents/pr-agent.agent.md` for implementation or `.github/agents/architecture-reviewer.agent.md` for a focused review task. Reviewers should inspect architecture and failure modes—not only compilation or style—and require manual verification steps for flows not covered by automation. Agents must never make a test pass by adding `@Ignore`, weakening assertions, swallowing exceptions, deleting coverage, or increasing a fixed sleep without identifying the race. Classify failures as product, test, environment/toolchain, external-service/credential, or flaky-test failures. Keep live Wikimedia/Beta calls and tests that mutate remote state out of the normal PR gate; prefer fakes, test repositories, and MockWebServer for deterministic tests. See [`docs/testing-strategy.md`](docs/testing-strategy.md) for the test layers and escalation path. From bbdff86fa2591347e5b583045fecfa2e26757e25 Mon Sep 17 00:00:00 2001 From: Ritika Pahwa Date: Sun, 26 Jul 2026 19:16:52 +0530 Subject: [PATCH 3/6] Improve architectural expectations --- .github/agents/architecture-reviewer.agent.md | 14 +++++++++- .github/agents/pr-agent.agent.md | 11 ++++++++ .github/copilot-instructions.md | 22 +++++++++++++-- .github/instructions/android.instructions.md | 27 +++++++++++++++++++ AGENTS.md | 14 ++++++++++ docs/testing-strategy.md | 2 ++ 6 files changed, 87 insertions(+), 3 deletions(-) create mode 100644 .github/instructions/android.instructions.md diff --git a/.github/agents/architecture-reviewer.agent.md b/.github/agents/architecture-reviewer.agent.md index 5daf1367e84..6392e715b73 100644 --- a/.github/agents/architecture-reviewer.agent.md +++ b/.github/agents/architecture-reviewer.agent.md @@ -11,7 +11,19 @@ Evaluate Android and Kotlin choices against current official documentation, espe Prioritize findings that can cause user-visible regressions: incorrect state or lifecycle handling, races and cancellation bugs, data loss or duplicate uploads, broken offline/error behavior, API/min-SDK incompatibility, security/privacy leaks, performance regressions, and architectural boundary violations. Check both `prod` and `beta` when configuration or API behavior differs. -For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage for the scenario, using Android instrumentation for critical user journeys when JVM tests cannot exercise it. Treat missing tests as a finding only when the change has a meaningful untested behavior; avoid style-only comments. End with a short risk summary and list the validation commands actually run. +## Architecture review checklist + +- Identify the UI, domain, data, and external-system boundaries affected by the change. +- Verify that Activities, Fragments, adapters, and composables do not own business rules or external I/O. +- Verify that domain logic does not depend directly on Android, Retrofit, Room, or API DTO types. +- Check for focused interfaces and realistic test seams at new external boundaries. +- Check for multiple sources of truth or bidirectional state flow. +- Check behavior after retry, cancellation, rotation, process death, offline mode, and authentication expiry. +- Check whether uploads or workers can execute twice and whether operations are idempotent. +- Treat unnecessary interfaces, wrappers, use cases, domain events, or modules as findings only when they add ceremony without solving a named problem. +- Treat missing tests as a finding when the change introduces meaningful behavior without a deterministic regression path. + +For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage for the scenario, using Android instrumentation for critical user journeys when JVM tests cannot exercise it. Avoid style-only comments. End with a short risk summary and list the validation commands actually run. This repository does not currently use Maestro; evaluate coverage using its existing JVM, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation layers. Check that tests are deterministic: no live backend is required for PR tests, no remote state is mutated, fixed sleeps are not used as synchronization, and setup helpers do not swallow assertion or view-matching failures. Flag `@Ignore` additions, weakened assertions, and unexplained test suppression as high-risk review findings. diff --git a/.github/agents/pr-agent.agent.md b/.github/agents/pr-agent.agent.md index 68615fa6e1e..c5af9b3642c 100644 --- a/.github/agents/pr-agent.agent.md +++ b/.github/agents/pr-agent.agent.md @@ -19,6 +19,17 @@ Use current official [Android Developers](https://developer.android.com/), [Andr 4. Add or update regression tests. Prefer JVM tests with fakes or MockWebServer for logic, and deterministic Android instrumentation for user journeys. Keep live Beta tests separate. 5. Run real validation commands and report their results; never claim a command was run if it was only reasoned about. +## Architecture rules + +- Keep UI classes focused on rendering state and forwarding events. +- Keep business rules out of Activities, Fragments, adapters, Retrofit interfaces, Room DAOs, and Workers. +- Keep domain logic independent of Android framework types, Retrofit DTOs, and Room entities. +- Add focused interfaces at external boundaries when they improve testability or isolate a dependency. +- Preserve one source of truth and one-way state flow per feature. +- Define retry, cancellation, offline, authentication-expiry, and duplicate-operation behavior. +- Use SOLID and DDD concepts only when they solve a concrete coupling or domain-model problem. +- Do not introduce a new architecture, module, use-case layer, or event system solely for stylistic consistency. + ## Validation commands ```bash diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index dd4ed60d611..cf3971faaf6 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,6 +1,8 @@ # Commons Android Copilot Instructions -Use the repository root `AGENTS.md` as the source of truth for structure, commands, coding conventions, security, and handoff requirements. Custom Copilot agents are defined in `.github/agents/*.agent.md`; use `pr-agent.agent.md` for implementation and `architecture-reviewer.agent.md` for focused review tasks. This file adds Copilot-specific review and cloud-agent guidance. +Use the repository root `AGENTS.md` as the source of truth for structure, commands, coding conventions, architecture, security, and handoff requirements. Custom Copilot agents are defined in `.github/agents/*.agent.md`; use `pr-agent.agent.md` for implementation and `architecture-reviewer.agent.md` for focused review tasks. This file adds Copilot-specific review and cloud-agent guidance. + +Keep these instructions concise and actionable. Prefer short imperative bullets, concrete examples, and distinct headings. Do not add vague goals such as “improve quality” or instructions about Copilot’s response formatting. Copilot code review is non-deterministic and context-limited; prioritize the highest-value rules and iterate using real pull requests. Read [`docs/testing-strategy.md`](../docs/testing-strategy.md) before changing tests. This repository does not currently use Maestro; use the existing JUnit, Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX test infrastructure unless the issue explicitly proposes a tool change. @@ -24,9 +26,25 @@ For Android, Kotlin, Gradle, Compose, lifecycle, permissions, accessibility, sec - Do not use live Wikimedia accounts or remote mutable state for PR tests. Use fake sessions, test repositories, fixtures, or MockWebServer. - Replace fixed sleeps and swallowed `NoMatchingViewException` failures with synchronization and explicit diagnostics. +## Architecture expectations + +- Keep Activities, Fragments, adapters, and composables focused on UI state and events. +- Keep business rules out of UI classes, Retrofit interfaces, Room DAOs, and Workers. +- Do not pass Retrofit DTOs, Room entities, Android framework types, or `Context` into domain logic. +- Prefer focused interfaces at external boundaries and realistic test implementations over mock-only designs. +- Preserve one source of truth and one-way state flow within each feature. +- Define retry, cancellation, offline, authentication-expiry, and duplicate-operation behavior for network and background work. +- Apply SOLID and DDD concepts when they reduce coupling or clarify a real domain concept; do not add ceremony without a named problem. + ## Review expectations -Flag only actionable issues, prioritizing: incorrect behavior, lifecycle/state bugs, concurrency, data loss, security/privacy, offline/error handling, API compatibility, performance, and missing regression coverage. Trace changes across callers and variants; a locally correct implementation that violates an architectural boundary is a finding. Do not report formatting preferences already enforced by the repository. +- Flag only actionable issues, prioritizing: incorrect behavior, lifecycle/state bugs, concurrency, data loss, security/privacy, offline/error handling, API compatibility, performance, architectural boundary violations, and missing regression coverage. +- Trace changes across callers, persistence, workers, APIs, and both product flavors. +- Check whether domain logic depends directly on Android, Retrofit, Room, or API DTOs. +- Check whether retries, cancellation, rotation, process death, or duplicate submissions change behavior. +- Require a deterministic test seam for new external dependencies. +- Do not report formatting preferences already enforced by the repository. +- Explain the failure scenario, affected users or code path, and concrete fix for every finding. ## Verification diff --git a/.github/instructions/android.instructions.md b/.github/instructions/android.instructions.md new file mode 100644 index 00000000000..d3b558d7d2c --- /dev/null +++ b/.github/instructions/android.instructions.md @@ -0,0 +1,27 @@ +--- +applyTo: "**/*.{kt,kts,java,xml}" +--- + +# Android and Kotlin Development Rules + +Use current official Android and Kotlin documentation as the normative source. Follow the repository root `AGENTS.md` for project-specific architecture and testing rules. + +## Boundaries + +- Keep Activities, Fragments, adapters, and composables focused on UI state and events. +- Keep business rules out of UI classes, Retrofit interfaces, Room DAOs, and Workers. +- Keep domain logic independent of Android framework types, Retrofit DTOs, and Room entities. +- Prefer focused interfaces at external boundaries when they improve testability or isolate a dependency. + +## State and reliability + +- Preserve one source of truth and one-way state flow per feature. +- Define loading, empty, success, and error states where applicable. +- Consider lifecycle cancellation, rotation, process death, offline mode, retries, authentication expiry, and duplicate operations. +- Use test doubles or MockWebServer instead of real services in JVM and PR-gating tests. + +## Maintainability + +- Apply SOLID and DDD concepts only when they solve a concrete coupling or domain-model problem. +- Do not add wrappers, interfaces, use cases, modules, or domain events solely for ceremony. +- Add a deterministic regression test for meaningful behavior changes. diff --git a/AGENTS.md b/AGENTS.md index 959719605bd..bb3f3967907 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -39,6 +39,20 @@ GitHub Copilot cloud agent can be assigned issues from GitHub and will open a pu Agents must never make a test pass by adding `@Ignore`, weakening assertions, swallowing exceptions, deleting coverage, or increasing a fixed sleep without identifying the race. Classify failures as product, test, environment/toolchain, external-service/credential, or flaky-test failures. Keep live Wikimedia/Beta calls and tests that mutate remote state out of the normal PR gate; prefer fakes, test repositories, and MockWebServer for deterministic tests. See [`docs/testing-strategy.md`](docs/testing-strategy.md) for the test layers and escalation path. +## Architecture Contract + +- Use a feature-oriented modular-monolith approach. Do not introduce a new architecture pattern or module unless it solves a named problem and has a migration boundary. +- Keep UI entry points focused on rendering state and forwarding events. Do not place business rules, API calls, database queries, upload policy, or retry policy in Activities, Fragments, adapters, or composables. +- Keep domain logic independent of Retrofit, Room, Android `Context`, `View`, `Activity`, `Fragment`, and API DTO types. +- Prefer dependency inversion at external boundaries: define focused interfaces for authentication, Wikimedia APIs, persistence, uploads, filesystem access, location, and background work. +- Keep one source of truth per feature and prefer one-way state flow: user event → presenter/ViewModel → domain operation → repository → state/result → UI. +- Map external DTOs and database entities into feature/domain models before exposing them to UI code. +- Represent meaningful concepts with validated models or value objects rather than unstructured `String`, `Boolean`, or map values. +- Define retryable, permanent, offline, authentication-expired, and duplicate-operation behavior explicitly. +- For uploads and workers, reason about cancellation, retries, process death, idempotency, and duplicate submissions. +- Prefer realistic fakes, test repositories, fixtures, and MockWebServer over tests that only verify mock call sequences. +- Apply SOLID where it reduces coupling; do not add interfaces, wrappers, use cases, or domain events solely for ceremony. + ## Android Documentation Policy Use the current official documentation as the normative source for Android platform, Gradle, Kotlin, Compose, lifecycle, permissions, accessibility, security, and testing guidance: diff --git a/docs/testing-strategy.md b/docs/testing-strategy.md index df98f32455c..3c025c86809 100644 --- a/docs/testing-strategy.md +++ b/docs/testing-strategy.md @@ -2,6 +2,8 @@ This repository uses existing JUnit/Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation infrastructure. Maestro is not part of the current testing baseline. +Tests are also an architecture check: domain rules should be runnable on the JVM, external systems should be replaceable by fakes or fixtures, and UI tests should verify user-visible behavior rather than implementation-specific mock interactions. + ## Test layers | Layer | Scope | Dependencies | Expected trigger | From a98c4cff2d32eab349a2b7438608d2f369dad337 Mon Sep 17 00:00:00 2001 From: Ritika Pahwa Date: Sun, 26 Jul 2026 19:20:16 +0530 Subject: [PATCH 4/6] docs: trim agent instruction duplication --- .github/agents/architecture-reviewer.agent.md | 8 +++----- .github/agents/pr-agent.agent.md | 13 ------------- .github/copilot-instructions.md | 16 +++------------- 3 files changed, 6 insertions(+), 31 deletions(-) diff --git a/.github/agents/architecture-reviewer.agent.md b/.github/agents/architecture-reviewer.agent.md index 6392e715b73..d1cd3933f72 100644 --- a/.github/agents/architecture-reviewer.agent.md +++ b/.github/agents/architecture-reviewer.agent.md @@ -5,9 +5,7 @@ description: Reviews Android pull requests for architectural regressions, behavi # Architecture Reviewer -Review the complete pull-request diff in the context of the repository, not just changed lines. Read the affected callers, lifecycle owners, persistence/API interfaces, worker paths, flavor configuration, and existing tests. - -Evaluate Android and Kotlin choices against current official documentation, especially [Android architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), [Android security](https://developer.android.com/privacy-and-security), and [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html). Treat the Commons documentation repository as historical/project context when it is older or marked as draft. Flag obsolete APIs, unsupported workarounds, and compatibility decisions that are not explained for `minSdk = 21`. +Review the complete pull-request diff in the context of the repository, not just changed lines. Follow `AGENTS.md` and `.github/copilot-instructions.md` for the architecture contract and official documentation policy. Prioritize findings that can cause user-visible regressions: incorrect state or lifecycle handling, races and cancellation bugs, data loss or duplicate uploads, broken offline/error behavior, API/min-SDK incompatibility, security/privacy leaks, performance regressions, and architectural boundary violations. Check both `prod` and `beta` when configuration or API behavior differs. @@ -23,8 +21,8 @@ Prioritize findings that can cause user-visible regressions: incorrect state or - Treat unnecessary interfaces, wrappers, use cases, domain events, or modules as findings only when they add ceremony without solving a named problem. - Treat missing tests as a finding when the change introduces meaningful behavior without a deterministic regression path. -For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage for the scenario, using Android instrumentation for critical user journeys when JVM tests cannot exercise it. Avoid style-only comments. End with a short risk summary and list the validation commands actually run. +For every finding, explain the failure scenario, affected users or code path, and a concrete fix. Require regression coverage when behavior is meaningfully untested. Avoid style-only comments. End with a short risk summary and list the validation commands actually run. -This repository does not currently use Maestro; evaluate coverage using its existing JVM, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation layers. Check that tests are deterministic: no live backend is required for PR tests, no remote state is mutated, fixed sleeps are not used as synchronization, and setup helpers do not swallow assertion or view-matching failures. Flag `@Ignore` additions, weakened assertions, and unexplained test suppression as high-risk review findings. +Check that tests are deterministic: no live backend is required for PR tests, no remote state is mutated, fixed sleeps are not used as synchronization, and setup helpers do not swallow failures. Flag `@Ignore` additions, weakened assertions, and unexplained suppression as high-risk findings. Use [`docs/testing-strategy.md`](../../docs/testing-strategy.md) to decide whether a test belongs in the PR gate or a scheduled/manual Beta workflow. A live-account or live-API test is not an acceptable substitute for a deterministic regression test. diff --git a/.github/agents/pr-agent.agent.md b/.github/agents/pr-agent.agent.md index c5af9b3642c..d87e32dc43e 100644 --- a/.github/agents/pr-agent.agent.md +++ b/.github/agents/pr-agent.agent.md @@ -9,8 +9,6 @@ Follow the root `AGENTS.md` and `.github/copilot-instructions.md`. Work only on Read [`docs/testing-strategy.md`](../../docs/testing-strategy.md) before modifying tests. This repository currently uses JUnit/Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation; do not introduce Maestro as an assumption. -Use current official [Android Developers](https://developer.android.com/), [Android architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), and [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html) documentation as the standard. Consult the Commons documentation repository for app-specific context, but do not treat older or draft pages as current Android guidance. - ## Required workflow 1. Translate the issue into acceptance criteria and identify affected feature packages, flavors, callers, and failure modes. @@ -19,17 +17,6 @@ Use current official [Android Developers](https://developer.android.com/), [Andr 4. Add or update regression tests. Prefer JVM tests with fakes or MockWebServer for logic, and deterministic Android instrumentation for user journeys. Keep live Beta tests separate. 5. Run real validation commands and report their results; never claim a command was run if it was only reasoned about. -## Architecture rules - -- Keep UI classes focused on rendering state and forwarding events. -- Keep business rules out of Activities, Fragments, adapters, Retrofit interfaces, Room DAOs, and Workers. -- Keep domain logic independent of Android framework types, Retrofit DTOs, and Room entities. -- Add focused interfaces at external boundaries when they improve testability or isolate a dependency. -- Preserve one source of truth and one-way state flow per feature. -- Define retry, cancellation, offline, authentication-expiry, and duplicate-operation behavior. -- Use SOLID and DDD concepts only when they solve a concrete coupling or domain-model problem. -- Do not introduce a new architecture, module, use-case layer, or event system solely for stylistic consistency. - ## Validation commands ```bash diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index cf3971faaf6..5e2c4ed3366 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -1,14 +1,14 @@ # Commons Android Copilot Instructions -Use the repository root `AGENTS.md` as the source of truth for structure, commands, coding conventions, architecture, security, and handoff requirements. Custom Copilot agents are defined in `.github/agents/*.agent.md`; use `pr-agent.agent.md` for implementation and `architecture-reviewer.agent.md` for focused review tasks. This file adds Copilot-specific review and cloud-agent guidance. +Use the repository root `AGENTS.md` as the source of truth. Custom Copilot agents are defined in `.github/agents/*.agent.md`. Keep these instructions concise and actionable. Prefer short imperative bullets, concrete examples, and distinct headings. Do not add vague goals such as “improve quality” or instructions about Copilot’s response formatting. Copilot code review is non-deterministic and context-limited; prioritize the highest-value rules and iterate using real pull requests. -Read [`docs/testing-strategy.md`](../docs/testing-strategy.md) before changing tests. This repository does not currently use Maestro; use the existing JUnit, Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX test infrastructure unless the issue explicitly proposes a tool change. +Read [`docs/testing-strategy.md`](../docs/testing-strategy.md) before changing tests. This repository uses JUnit, Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation; Maestro is not part of the current baseline. ## Documentation authority -For Android, Kotlin, Gradle, Compose, lifecycle, permissions, accessibility, security, and testing questions, consult current official documentation first: [Android Developers](https://developer.android.com/), [Android app architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), and [Kotlin coding conventions](https://kotlinlang.org/docs/coding-conventions.html). Use the Commons app documentation repository for project history and domain context, not as a substitute for current platform documentation. Avoid obsolete APIs and undocumented workarounds; if compatibility with `minSdk = 21` requires one, explain it in the PR. +For Android and Kotlin questions, consult current official documentation first: [Android Developers](https://developer.android.com/), [Android architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), and [Kotlin conventions](https://kotlinlang.org/docs/coding-conventions.html). Use Commons documentation for project context, not as a substitute for current platform guidance. Explain any `minSdk = 21` compatibility workaround in the PR. ## Before changing code @@ -26,16 +26,6 @@ For Android, Kotlin, Gradle, Compose, lifecycle, permissions, accessibility, sec - Do not use live Wikimedia accounts or remote mutable state for PR tests. Use fake sessions, test repositories, fixtures, or MockWebServer. - Replace fixed sleeps and swallowed `NoMatchingViewException` failures with synchronization and explicit diagnostics. -## Architecture expectations - -- Keep Activities, Fragments, adapters, and composables focused on UI state and events. -- Keep business rules out of UI classes, Retrofit interfaces, Room DAOs, and Workers. -- Do not pass Retrofit DTOs, Room entities, Android framework types, or `Context` into domain logic. -- Prefer focused interfaces at external boundaries and realistic test implementations over mock-only designs. -- Preserve one source of truth and one-way state flow within each feature. -- Define retry, cancellation, offline, authentication-expiry, and duplicate-operation behavior for network and background work. -- Apply SOLID and DDD concepts when they reduce coupling or clarify a real domain concept; do not add ceremony without a named problem. - ## Review expectations - Flag only actionable issues, prioritizing: incorrect behavior, lifecycle/state bugs, concurrency, data loss, security/privacy, offline/error handling, API compatibility, performance, architectural boundary violations, and missing regression coverage. From 9043cc75c3a57c3405581561a222c04d492197c7 Mon Sep 17 00:00:00 2001 From: Ritika Pahwa Date: Sun, 26 Jul 2026 19:25:54 +0530 Subject: [PATCH 5/6] docs: add agent workflow examples --- .github/agents/pr-agent.agent.md | 18 +++++++++++++++++- .github/copilot-instructions.md | 16 +++++++++++++++- AGENTS.md | 18 ++++++++++++++++-- docs/testing-strategy.md | 4 ++++ 4 files changed, 52 insertions(+), 4 deletions(-) diff --git a/.github/agents/pr-agent.agent.md b/.github/agents/pr-agent.agent.md index d87e32dc43e..d115606787d 100644 --- a/.github/agents/pr-agent.agent.md +++ b/.github/agents/pr-agent.agent.md @@ -32,4 +32,20 @@ Never make a test pass by adding `@Ignore`, weakening assertions, swallowing exc ## Pull request output -Include: summary, motivation with `Fixes #ID` when applicable, implementation and architectural impact, tests run, known limitations, and manual verification steps. For UI changes include screenshots or recordings. Keep commits logically separate and use an area-prefixed subject where useful, such as `upload: prevent duplicate images`. +Include: summary, motivation with `Fixes #ID` when applicable, implementation and architectural impact, tests run, known limitations, and manual verification steps. For UI changes include screenshots or recordings. Keep commits logically separate, describe them clearly, and use an area-prefixed subject where useful, such as `upload: prevent duplicate images`. Add descriptive KDocs for new classes and methods, do not add `@author` tags, and update relevant Wiki or contributor documentation when needed. + +When an issue is underspecified, structure the proposed work like this before coding: + +```text +Problem: +Retries after a network timeout can submit the same image twice. + +Acceptance criteria: +- Retries do not create duplicate uploads. +- Successful upload behavior remains unchanged in prod and beta. +- Add a deterministic regression test. + +Out of scope: +- Redesigning the upload flow. +- Refactoring unrelated UI code. +``` diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index 5e2c4ed3366..c9e0ca15c38 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -8,7 +8,7 @@ Read [`docs/testing-strategy.md`](../docs/testing-strategy.md) before changing t ## Documentation authority -For Android and Kotlin questions, consult current official documentation first: [Android Developers](https://developer.android.com/), [Android architecture](https://developer.android.com/topic/architecture), [Android testing](https://developer.android.com/training/testing), and [Kotlin conventions](https://kotlinlang.org/docs/coding-conventions.html). Use Commons documentation for project context, not as a substitute for current platform guidance. Explain any `minSdk = 21` compatibility workaround in the PR. +Use current Android and Kotlin platform guidance when making implementation decisions. Use Commons documentation for project context, not as a substitute for current platform guidance. Explain any `minSdk = 21` compatibility workaround in the PR. The review rules in this file are self-contained; do not depend on following external links. ## Before changing code @@ -25,6 +25,8 @@ For Android and Kotlin questions, consult current official documentation first: - Do not weaken tests, suppress lint, alter credentials, or delete existing behavior to make CI pass. - Do not use live Wikimedia accounts or remote mutable state for PR tests. Use fake sessions, test repositories, fixtures, or MockWebServer. - Replace fixed sleeps and swallowed `NoMatchingViewException` failures with synchronization and explicit diagnostics. +- Add descriptive KDocs for every new class and method; do not add `@author` tags. +- Keep documentation and relevant Wiki pages current when behavior or contributor workflow changes. ## Review expectations @@ -35,6 +37,18 @@ For Android and Kotlin questions, consult current official documentation first: - Require a deterministic test seam for new external dependencies. - Do not report formatting preferences already enforced by the repository. - Explain the failure scenario, affected users or code path, and concrete fix for every finding. +- Check that new classes and methods have descriptive KDocs and no `@author` tags. +- Check that the PR description links the issue, records tests, and includes UI screenshots or recordings when applicable. + +Example finding: + +```text +[P1] Retry can upload the same file twice + +UploadWorker retries after a timeout, but the server may have accepted the +previous request. Add an idempotency key or verify the existing upload before +retrying, then cover the timeout/retry path with a deterministic test. +``` ## Verification diff --git a/AGENTS.md b/AGENTS.md index bb3f3967907..4eb2e3df02e 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -17,7 +17,7 @@ Use the checked-in Gradle wrapper and Java 17. ## Coding Style & Naming Conventions -Write Kotlin with four-space indentation, clear nullability, and small, readable functions. Follow existing Android/Kotlin conventions: `PascalCase` for classes and composables, `camelCase` for methods and properties, and descriptive `*Test` or `*UnitTest` test classes. Keep feature code in its relevant package and place UI strings in resources rather than hard-coding them. Add reasonably descriptive Javadocs/KDocs for every new class and method; do not add `@author` tags. +Write Kotlin with four-space indentation, clear nullability, and small, readable functions. Follow existing Android/Kotlin conventions: `PascalCase` for classes and composables, `camelCase` for methods and properties, and descriptive `*Test` or `*UnitTest` test classes. Keep feature code in its relevant package and place UI strings in resources rather than hard-coding them. Add reasonably descriptive KDocs for every new class and method; do not add `@author` tags. Readable code should make comments unnecessary except where they explain non-obvious behavior. ## Testing Guidelines @@ -25,7 +25,7 @@ Use the existing JUnit/Robolectric-style unit-test setup for JVM tests and Andro ## Commit & Pull Request Guidelines -Make separate commits for logically separate changes. Use a short imperative first line, optionally prefixed with the affected area (for example, `upload: prevent duplicate images`), followed by a meaningful body when context is needed. Pull requests should explain the problem and solution, include testing performed, link related issues, and provide screenshots or recordings for UI changes. Update relevant documentation or wiki content when behavior changes. +Make separate commits for logically separate changes. Use a short first line, optionally prefixed with the affected area (for example, `upload: prevent duplicate images`), followed by a meaningful body when context is needed. Pull requests should explain the problem and solution, include testing performed, link related issues, and provide screenshots or recordings for UI changes. Update relevant documentation or wiki content when behavior changes, and keep the Wiki current when a contribution changes contributor-facing instructions. ## Security & Configuration @@ -53,6 +53,20 @@ Agents must never make a test pass by adding `@Ignore`, weakening assertions, sw - Prefer realistic fakes, test repositories, fixtures, and MockWebServer over tests that only verify mock call sequences. - Apply SOLID where it reduces coupling; do not add interfaces, wrappers, use cases, or domain events solely for ceremony. +Example boundary: + +```kotlin +// Prefer: domain-facing interface +interface UploadGateway { + suspend fun upload(request: UploadRequest): UploadResult +} + +// Avoid: domain code depending directly on a Retrofit API +class UploadUseCase(private val uploadApi: UploadApi) +``` + +Example test seam: `UploadUseCaseTest → FakeUploadGateway`. Avoid `UploadActivityTest → real Wikimedia API → real account → remote upload`. + ## Android Documentation Policy Use the current official documentation as the normative source for Android platform, Gradle, Kotlin, Compose, lifecycle, permissions, accessibility, security, and testing guidance: diff --git a/docs/testing-strategy.md b/docs/testing-strategy.md index 3c025c86809..468a4de91b6 100644 --- a/docs/testing-strategy.md +++ b/docs/testing-strategy.md @@ -22,6 +22,8 @@ Tests are also an architecture check: domain rules should be runnable on the JVM - Do not catch and ignore `NoMatchingViewException` or other setup/assertion failures. - Do not add `@Ignore`, weaken assertions, or suppress a test to make CI pass. +Prefer `UploadUseCaseTest → FakeUploadGateway`. Avoid `UploadActivityTest → real Wikimedia API → real account → remote upload` for pull-request validation. + ## Commands ```bash @@ -36,3 +38,5 @@ Use a focused test class or method while iterating. Record the exact variant, em ## Stabilization order When an instrumentation test is failing, first make the failure observable, then remove timing and external-service dependencies, then add deterministic fixtures or a fake session. Promote one stable test group to the PR gate at a time. Keep upload and other remote-mutating tests outside the PR gate until isolation and cleanup are proven. + +For contributor-facing test changes, follow `CONTRIBUTING.md`: keep commits logically separate, describe the change clearly, add tests where possible, and update the Wiki or related documentation when the testing workflow changes. From 94883f71b899fa6305db5aec274bd2cb7c25a777 Mon Sep 17 00:00:00 2001 From: Ritika Pahwa <83745993+RitikaPahwa4444@users.noreply.github.com> Date: Sun, 26 Jul 2026 19:29:26 +0530 Subject: [PATCH 6/6] Potential fix for pull request finding Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com> --- docs/testing-strategy.md | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/docs/testing-strategy.md b/docs/testing-strategy.md index 468a4de91b6..283ea18f2f7 100644 --- a/docs/testing-strategy.md +++ b/docs/testing-strategy.md @@ -30,7 +30,7 @@ Prefer `UploadUseCaseTest → FakeUploadGateway`. Avoid `UploadActivityTest → ./gradlew testProdDebugUnitTest ./gradlew lintProdDebug ./gradlew assembleProdDebug assembleBetaDebug -./gradlew connectedBetaDebugAndroidTest +./gradlew connectedBetaDebugAndroidTest # when an emulator/device is available ``` Use a focused test class or method while iterating. Record the exact variant, emulator/API level, backend, credentials mode, and command for every result. Classify failures as product, test, environment/toolchain, external-service/credential, or flaky-test failures before changing code.