diff --git a/.github/agents/architecture-reviewer.agent.md b/.github/agents/architecture-reviewer.agent.md new file mode 100644 index 00000000000..d1cd3933f72 --- /dev/null +++ b/.github/agents/architecture-reviewer.agent.md @@ -0,0 +1,28 @@ +--- +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. 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. + +## 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 when behavior is meaningfully untested. Avoid style-only comments. End with a short risk summary and list the validation commands actually run. + +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 new file mode 100644 index 00000000000..d115606787d --- /dev/null +++ b/.github/agents/pr-agent.agent.md @@ -0,0 +1,51 @@ +--- +name: pr-agent +description: Implements focused Android/Kotlin issues and produces review-ready pull requests. +--- + +# Pull Request Agent + +Follow the root `AGENTS.md` and `.github/copilot-instructions.md`. Work only on the requested issue; do not refactor unrelated legacy code. + +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. + +## Required workflow + +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. + +## Validation commands + +```bash +./gradlew testProdDebugUnitTest +./gradlew lintProdDebug +./gradlew assembleProdDebug assembleBetaDebug +./gradlew connectedBetaDebugAndroidTest # when an emulator/device is available +``` + +Use narrower tasks first when iterating. If an instrumentation test cannot run, explain why and provide manual verification steps. + +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. + +## 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, 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/agents/pr-agent.md b/.github/agents/pr-agent.md deleted file mode 100644 index d289386c4bb..00000000000 --- a/.github/agents/pr-agent.md +++ /dev/null @@ -1,101 +0,0 @@ - ---- -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) - ---- - -## Responsibilities -You must: - -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 - ---- - -## 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` | - -You must NOT produce a PR that would fail these commands. - ---- - -## PR Format (Strict Requirement) - -Every PR you generate must include: - -### 🔹 Summary -1–2 sentences describing what changed - -### 🔹 Motivation -Why the change was needed -Include issue reference: `Fixes #XYZ` - -### 🔹 Implementation Details -How the solution was implemented -Mention files touched and reasoning - -### 🔹 Tests -- Add/update unit tests for logic -- Add/update instrumentation tests for UI behavior -- If no tests are required, you must justify it - -### 🔹 Verification Steps -Write clear manual test instructions maintainers can follow - ---- - -## 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 - -**Good Kotlin example** -```kotlin -fun loadUser(id: String): User { - require(id.isNotBlank()) { "id required" } - return repository.getUser(id) -} diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md new file mode 100644 index 00000000000..c9e0ca15c38 --- /dev/null +++ b/.github/copilot-instructions.md @@ -0,0 +1,57 @@ +# Commons Android Copilot Instructions + +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 uses JUnit, Robolectric, MockWebServer, Espresso, UiAutomator, and AndroidX instrumentation; Maestro is not part of the current baseline. + +## Documentation authority + +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 + +- 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. +- 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 + +- 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. +- 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 + +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/.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 new file mode 100644 index 00000000000..4eb2e3df02e --- /dev/null +++ b/AGENTS.md @@ -0,0 +1,79 @@ +# 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 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 + +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 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 + +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. 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. + +## 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. + +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: + +- [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..283ea18f2f7 --- /dev/null +++ b/docs/testing-strategy.md @@ -0,0 +1,42 @@ +# 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. + +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 | +| --- | --- | --- | --- | +| 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. + +Prefer `UploadUseCaseTest → FakeUploadGateway`. Avoid `UploadActivityTest → real Wikimedia API → real account → remote upload` for pull-request validation. + +## Commands + +```bash +./gradlew testProdDebugUnitTest +./gradlew lintProdDebug +./gradlew assembleProdDebug assembleBetaDebug +./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. + +## 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.