diff --git a/.agents/commands/pr.md b/.agents/commands/pr.md index 76b47e5f55..6a980e7133 100644 --- a/.agents/commands/pr.md +++ b/.agents/commands/pr.md @@ -45,6 +45,7 @@ If no base branch argument provided, detect the repo's default branch: - Fetch 10 most recent PRs (open or closed) from the extracted repo for writing style reference - Run `git log $base..HEAD --oneline` for commit messages - Run `git diff $base...HEAD --stat` for understanding scope of changes +- List the journeys the branch adds or updates: `git diff --name-only --diff-filter=d $base...HEAD -- journeys | grep '\.xml$'` - Read `docs/screens-map.md` as the starting point for locating relevant Figma frames when the diff has user-visible UI changes - **If custom instructions provided:** - If instructions reference a specific commit SHA (pattern like `commit [a-f0-9]{7,40}`): @@ -140,28 +141,32 @@ When the user provides custom instructions after `--`: - Always use this structure: ```md ### QA Notes + #### Journeys #### Manual Tests #### Automated Checks ``` +- Under `#### Journeys`, list every journey the branch adds or updates (Step 3), one per line as an unchecked checkbox (`- [ ] `), then `new` or `updated`, then the bare journey file name in backticks, then a dash and what the journey proves. +- Reference journeys by bare file name only, never the full path. Only when two listed journeys share the same name, prefix the shortest leading path segment(s) that disambiguate them, the same rule as test files. +- A PR with a user-visible change adds or updates the journey that proves it, and any journey whose route the diff changes; list them all. Reviewers drive the listed journeys on a device. +- `N/A — no user-visible behaviour change.` is the only empty value for `#### Journeys`. +- Leave every checkbox under `#### Journeys` and `#### Manual Tests` unchecked; the reviewer ticks a line after driving it on the PR head. +- When the diff changes user-visible behaviour and no journey covers it, stop and report the flows that need a journey. +- Under `#### Manual Tests`, write a step only when it needs a capability the Capabilities table in `journeys/README.md` does not list, and name that capability in the step. Everything a journey can drive belongs under `#### Journeys`. - Keep local verification commands, Gradle tasks, detekt, lint, unit tests, build passes, cargo test, cargo clippy, npm test, typecheck, CI coverage, or similar automated checks out of `#### Manual Tests`; summarize them under `#### Automated Checks` when they add useful context. -- Use `#### Automated Checks` to summarize automated verification evidence, prioritizing coverage added, modified, or removed, each with the test file name and a short explanation. +- Use `#### Automated Checks` for a flat list of automated verification evidence, in the keyword order `added`, `updated`, `removed`, `ran`: the keyword, the bare test file name in backticks, then a dash and the behaviour it proves. - Reference test files by bare file name only (e.g. `HwWalletRepoTest.kt`), never the full path. Only when two referenced test files share the same name, prefix the shortest leading path segment(s) that disambiguate them (e.g. `repositories/FooTest.kt` vs `viewmodels/FooTest.kt`). -- For removed automated coverage, state why it was removed. -- Do not list standard CI or PR bot commands as checkbox items just because they run for every PR. If standard CI coverage is worth mentioning, summarize it in one sentence. -- List raw commands only when they were run locally, are non-standard, use special flags or environment values, validate workflow behavior, or explain a meaningful verification gap. +- For a `removed` item, state why the coverage was removed. +- Use `ran` only for checks CI does not run. Do not list standard CI or PR bot commands just because they run for every PR. +- List raw commands under `ran` only when they were run locally, are non-standard, use special flags or environment values, validate workflow behavior, or explain a meaningful verification gap. - For workflow behavior validation, include `(after merge)` in the automated check item because workflow changes only take effect for PRs opened after the workflow update merges. -- If no actionable manual validation exists, write `N/A` under `#### Manual Tests`. +- If no step needs a missing capability, write `N/A` under `#### Manual Tests`. - If no automated checks were run and no automated coverage changed, write `N/A` under `#### Automated Checks`. - Write manual tests using this template: ```md - - [ ] **{numbering}.** {optional_condition + →} {screen_action} → {next_screen_action}: expectation + - [ ] {optional_condition + →} {action} → {expectation} — {missing capability} not in Capabilities ``` -- Use a list of unchecked checkboxes for each individual test. -- Use a numbered prefix for each test, in bold, for example `**1.**`, `**2.**`. -- Use `regression:` for regression checks, positioned after the numbering. -- Use sub-lists for variations of the same test. -- Use letter suffixes in numbering for each variation when a test has a sub-list, for example `**3a.**`, `**3b.**`. -- Always use `→` to denote navigation, for example `Send → Amount`. +- Use `regression:` for regression checks, positioned at the start of the action. +- Always use `→` to denote navigation, for example `Send → Amount`; the last `→` introduces the expectation. - Use screen names from code, formatted as separate words without the `Screen` suffix, for example `SendAmountScreen` becomes `Send Amount`. - Use short-form wording like `in-sheet` for sheet screens, `nav` for navigation, `back` for back nav, and `LN` for Lightning Network. @@ -175,37 +180,31 @@ Include `### Design` when the PR template contains a `### Design` heading, and a Reviewers may make at most one advisory request per PR when an existing-design UI link is omitted or an out-of-map `N/A — no design available.` claim is unverified. Valid mapped `todo`/`n/a` cases and `N/A — no UI changes.` require no request. Missing links never block approval, CI, PR creation, or review readiness. **For library repos (has `bindings/` directory or `Cargo.toml`):** -Structure manual QA around integration validation only. Automated checks belong under `#### Automated Checks`. +These repos have no `journeys/`, so the walkthrough stays under `#### Manual Tests` and needs no capability reason. Structure it around integration validation only. Automated checks belong under `#### Automated Checks`. Example: ``` ### QA Notes #### Manual Tests -- [ ] **1.** Consumer app → exercise updated binding flow: behavior matches previous release. -- [ ] **2.** `regression:` Android integration screen → trigger changed API path: no crash or stale data. +- [ ] Consumer app → exercise the updated binding flow → behavior matches the previous release +- [ ] `regression:` Android integration screen → trigger the changed API path → no crash or stale data #### Automated Checks -- Binding tests added: cover updated Android API path in `bindings/android/...`. -- CI: standard cargo and binding checks run by the PR bot. +- added `FooBindingTest.kt` — covers the updated Android API path +- ran `cargo test --all-features` — binding round-trip CI does not build ``` Concrete style target: ```md ### QA Notes +#### Journeys +- [ ] new `send-amount-over-balance.xml` — error shows before the 15 s timeout +- [ ] updated `lightning-transfer-detail.xml` — Connection opens Channel Detail #### Manual Tests -- [ ] **1.** No usable channels/spending balance → scan LN invoice: error shows immediately, not after 15s. -- [ ] **2.** Scanner → scan fixed amount LN invoice: Send Confirm or QuickPay opens directly. -- [ ] **3a.** `regression:` Send → scanner/paste fixed amount LN invoice: in-sheet nav to Confirm or QuickPay. - - [ ] **3b.** `regression:` Variable amount LN invoice/LNURL-pay: lands on Amount view. -- [ ] **4a.** Activity Detail of LN transfer → tap Connection: lands on Channel Detail. - - [ ] **4b.** back: returns to Activity Detail. -- [ ] **5a.** Settings → Lightning Connections → tap channel: still opens Channel Detail. - - [ ] **5b.** back: returns to Connections List. -- [ ] **6.** `regression:` Channel Detail → tap Close Connection: works. +- [ ] Pair a Trezor over BLE → Home shows the hardware wallet card — BLE pairing not in Capabilities #### Automated Checks -- Unit tests added: cover invoice timeout handling in `SendInvoiceTest.kt`. -- Unit tests modified: update channel navigation assertions in `ChannelDetailTest.kt`. -- Test coverage removed: delete stale mock-only assertions from `OldFlowTest.kt` because the flow no longer exists. -- CI: standard compile, unit test, and detekt checks run by the PR bot. +- added `TransferViewModelTest.kt` — rejects amounts over the spending balance +- updated `SendFlowTest.kt` — fixed-amount invoice skips the Amount screen +- removed `OldFlowTest.kt` — flow no longer exists ``` **Preview Section (conditional):** diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index c4634527c5..5c42fe6ecf 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -20,10 +20,14 @@ ### QA Notes +#### Journeys + + + #### Manual Tests - + #### Automated Checks - + diff --git a/AGENTS.md b/AGENTS.md index 8d88e4f045..7a908daeef 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -52,9 +52,15 @@ just install just clean ``` +## Prerequisites + +- The **`android` CLI and the `android-cli` skill** are required, not recommended: they are how an + agent drives a connected emulator or device, so without them the journeys under `journeys/` cannot + run and a PR's QA contract cannot be checked. Install the CLI as below. + ### Agent CLI (android) -Agents can drive a connected emulator or device with the `android` CLI, which wraps the SDK tooling +Agents drive a connected emulator or device with the `android` CLI, which wraps the SDK tooling and adds a semantic UI dump. It is not provisioned by this repo — install it if it is missing, from `https://dl.google.com/android/cli/latest//install.sh` where `` is one of `darwin_arm64`, `darwin_x86_64` or `linux_x86_64`: @@ -100,10 +106,11 @@ emulator or device — number pad caps, notification permission, widget flows, d wallet pairing and transfers. Read [`journeys/README.md`](journeys/README.md) before running or writing one; it has the format, the runner commands and the per-suite preconditions. -- Journeys are **developer-assistance specs, not a QA gate**. Nothing in `.github/workflows` reads - `journeys/`; `ui-tests.yml` runs the instrumented tests and never touches them. They are - agent-evaluated and non-deterministic, so they belong on a manual, developer-triggered run rather - than a blocking gate. An agent runs one on request. +- Journeys are **the QA contract for a PR**. A PR with a user-visible change adds or updates the + journeys that prove it and any journey whose route it changes, and lists them under `#### Journeys` + in the PR body. Reviewers drive the listed journeys on a device; nothing in `.github/workflows` + reads `journeys/`. Write a manual test only for a step that needs a capability the Capabilities + table in [`journeys/README.md`](journeys/README.md) does not list. - A journey is **not the source of truth** for app behaviour, despite what the `android-cli` skill's own `references/journeys.md` says. A journey that disagrees with the app is most likely stale. Say what you found and update the journey; escalate only once you have separately confirmed the app is diff --git a/journeys/README.md b/journeys/README.md index a5db0fa12c..ea9af8c62c 100644 --- a/journeys/README.md +++ b/journeys/README.md @@ -4,10 +4,9 @@ A journey is an XML-specified walkthrough of app behaviour, evaluated by an agen emulator or device. They are developer-assistance specs: they give an agent a reliable route through a flow so it can reproduce a bug, check a change by hand, or show you what a screen does today. -**Journeys are not a QA gate.** Nothing in `.github/workflows` reads `journeys/` — `ui-tests.yml` -runs the instrumented tests and never touches this directory. They are agent-evaluated and -non-deterministic, which is why they belong on a manual, developer-triggered run rather than a -blocking CI gate. An agent runs one on request. +**Journeys are the QA contract for a PR.** A PR with a user-visible change adds or updates the +journeys that prove it and lists them in its body, and reviewers drive the listed journeys on a +device instead of reading a prose walkthrough. **A journey is not the source of truth.** The `android` CLI ships its own journey documentation (`references/journeys.md` in the `android-cli` skill) which says the opposite — "the journey XML is @@ -110,27 +109,23 @@ Fund a wallet before any amount journey — with a zero balance the caps fall ba maximum and the journeys pass for the wrong reason. Per-suite preconditions (Trezor emulator, Pubky fixtures, push notifications) live in each suite's README. -## Suites +## Capabilities -| Suite | Journeys | Notes | -| --- | --- | --- | -| [activity](activity) | 1 | Date range sheet under rapid month taps; needs no backend, no README | -| [amount-limits](amount-limits) | 4 | Number pad caps on all four amount screens | -| [backup-restore](backup-restore) | 1 | VSS restore keeps tags and closed channels; wipes the wallet | -| [cjit-notifications](cjit-notifications) | 3 | CJIT channel-ready notifications; needs FCM push | -| [coin-selection](coin-selection) | 1 | Manual coin selection screen; needs 3+ on-chain UTXOs; no README | -| [deeplinks](deeplinks) | 2 | `bitkit://screen/…` and sheet routing behind the dev-mode gate; no README | -| [hardware-wallet](hardware-wallet) | 17 | Trezor over USB; needs the Trezor emulator | -| [home](home) | 1 | Pull to refresh on Home; checks the app log, no README | -| [node-lifecycle](node-lifecycle) | 1 | Detached LDK restart completes; a cancelled RGS server change reconciles and recovers to Running; reads the app log; no README | -| [notification-permission](notification-permission) | 4 | Background-setup toggles | -| [payment-requests](payment-requests) | 2 | Requires a linked fixture issuer; rejected shapes are unit fixtures | -| [pubky-marketplace](pubky-marketplace) | 1 | Two-wallet Paykit marketplace payment; integration fixture required | -| [security](security) | 1 | PIN result sheet layout at a long locale and font scale; no README | -| [subscriptions](subscriptions) | 4 | Paykit subscription lifecycle across two wallets, plus the Payments tab | -| [tags](tags) | 1 | Tag input length cap on an activity; no backend, no README | -| [transfers](transfers) | 1 | Transfer to Spending settling after the LSP closes the channel; no README | -| [widgets](widgets) | 2 | Needs no backend — the quickest way to see the loop work; no README | +This table is the authority for what the journey environment provides: a step it covers belongs in a +journey, and a step it does not is a manual test in the PR body naming the missing capability. + +| Capability | Provided by | +| --- | --- | +| On-chain funds and blocks on regtest | `./lsp` deposit and mine against the staging LSP — [Backend preconditions](#backend-preconditions) | +| Several separate on-chain UTXOs to choose between | three or more `./lsp` deposits, each mined, so manual coin selection has inputs to list — [Backend preconditions](#backend-preconditions) | +| Lightning channels, CJIT orders and quoted maxima | the same staging LSP the dev flavor targets, plus its node as an external LN peer — [Backend preconditions](#backend-preconditions), [amount-limits](amount-limits/README.md) | +| A hardware wallet to pair, watch and sign with | the deterministic Trezor emulator from `bitkit-docker` over the Bridge transport, with the USB attach intent injected by `adb`; USB enumeration, permission grants, the OS picker and BLE are not simulated — [hardware-wallet](hardware-wallet/README.md) | +| Push notifications to a backgrounded or killed app | an FCM push from a CJIT order paid through `./lsp`, read back with `adb shell dumpsys notification` — [cjit-notifications](cjit-notifications/README.md) | +| The OS notification-permission dialog | an API 33+ target, reset with `adb shell pm revoke to.bitkit.dev android.permission.POST_NOTIFICATIONS` — [notification-permission](notification-permission/README.md) | +| An incoming Payment Request from a linked issuer | the fixture issuer, saved as a contact and linked on receiver path `bitkit/server` — [payment-requests](payment-requests/README.md) | +| Two linked Bitkit wallets for a subscription lifecycle | a second Bitkit instance linked to the first, so a proposal can be reviewed and accepted — [subscriptions](subscriptions) | +| A Pubky identity and a two-wallet marketplace purchase | the integration fixture runtime: Pubky testnet, Paykit Server, regtest bitcoind and Fulcrum — [pubky-marketplace](pubky-marketplace/README.md) | +| Deep links, addresses and invoices handed to the app | `adb shell am start -a android.intent.action.VIEW -d ""`; `bitkit://` screen and sheet routes sit behind the dev-mode gate — [Running a journey](#running-a-journey), [deeplinks](deeplinks) | ## Cross-platform