diff --git a/.agents/commands/pr.md b/.agents/commands/pr.md index 601fd8e101..12cb746a25 100644 --- a/.agents/commands/pr.md +++ b/.agents/commands/pr.md @@ -19,7 +19,7 @@ Create a PR on GitHub using the `gh` CLI for the currently checked-out branch. ### 1. Check for Existing PR Run `gh pr view --json number,url 2>/dev/null` to check if a PR already exists for this branch. -- If PR exists: Output `PR already exists: [URL]` and stop +- If PR exists and the user explicitly requested a description update (models, relationships, or the full body): follow "Updating an existing PR description" below, then stop. Otherwise output `PR already exists: [URL]` and stop. - If no PR: Continue ### 2. Parse Arguments @@ -63,9 +63,14 @@ If no base branch argument provided, detect the repo's default branch: ### 4. Extract Linked Issues Scan commits for issue references: -- Pattern to match: `#123` (just the issue number reference) -- Extract unique issue numbers: `git log $base..HEAD --oneline | grep -oE "#[0-9]+" | sort -u` -- Fetch each issue title: `gh api "repos/$REPO/issues/NUMBER" --jq '.title'` (using repo from Step 3) +- Read commit messages with `git log $base..HEAD --format=%B`; collect unique issue references, + preserving repository-qualified references and full GitHub issue/PR URLs. Resolve bare `#123` + references in the current repository. +- Fetch each candidate with `gh api "repos/OWNER/REPO/issues/NUMBER"`; read its title and check + whether the response has a `pull_request` field (which identifies a PR, not an issue). +- Resolve each reference against its own repository and verify it is an issue before using a + closing keyword. Related PR references belong under Related PRs; do not turn Twin, Companion, + or Dependency PR numbers into issue-closing lines. - These will be used to start the PR description with linking keywords (see Step 6) ### 5. Identify Suggested Reviewers @@ -87,6 +92,7 @@ Starting from the template in `.github/pull_request_template.md`: If linked issues were found in commit messages, begin the PR description with linking keywords: - Use `Fixes #123` for bug fixes - Use `Closes #123` for features/enhancements +- Preserve `owner/repo#123` for cross-repository issues; never strip their repository qualification - One per line, before the "This PR..." opening separated by one empty line - Reference: https://docs.github.com/en/get-started/writing-on-github/working-with-advanced-formatting/using-keywords-in-issues-and-pull-requests @@ -98,6 +104,30 @@ Closes #418 This PR adds support for... ``` +**Related PRs:** +- After any issue-closing lines and before the opening summary, add applicable `Twin:`, `Companion:`, + and `Dependency:` lines, in that order. Omit labels with no confirmed relationship; do not add + empty placeholders or `N/A` lines. Use `owner/repo#number`, or a Markdown link with that label, + so the PR number identifies the correct repository. Use one line per related PR when needed. +- `Twin:` is the matching implementation of the same change in the other native Bitkit app: + Android points to `synonymdev/bitkit-ios#number`, and iOS to `synonymdev/bitkit-android#number`. + E2E tests and libraries are not native twins. +- `Companion:` is related coordinated work that is not a prerequisite, such as coverage in + `synonymdev/bitkit-e2e-tests`. +- `Dependency:` is a PR whose changes are required to build, validate, or ship this change, such as + a `synonymdev/bitkit-core` or other library PR. State any required merge/release order or published + version in the description; linking the PR does not imply its artifact is already available. + If a related E2E or app PR is itself a prerequisite, label it Dependency rather than Companion + (repository name alone does not determine the role). A twin can also be a dependency when both + relationships genuinely apply; state the reason. +- Confirm references from the user's supplied links, existing PR/issue links, and the candidate + PR's body and diff. Read the candidate PR to verify repository, number, scope and status. Branch + names or similar titles alone do not prove a relationship. If pairing is ambiguous, report it + and ask for the missing reference rather than guessing; do not block an unrelated standalone PR. +- For an existing `Counterpart:` line, classify the actual relationship and replace it with the + appropriate label when updating relationships. Do not duplicate the same link under equivalent + labels or edit the other PR automatically without authorization. + **Opening Format:** - Single change: Start with "This PR [verb]s..." as a complete sentence - Example: `This PR adds a Claude Code /pr command for generating PRs.` @@ -136,16 +166,60 @@ When the user provides custom instructions after `--`: - Preserve exact manual testing steps provided by the user (don't summarize or omit details) - If custom instructions include automated checks or coverage notes, place them under `#### Automated Checks` +**Models used:** +- Include `### Models used` using the policy in `AGENTS.md` under Agent workflow. +- Always include `Planning/scoping`, `Implementation`, and one `Review` entry. Aggregate all review + passes into that entry: list each distinct model/effort pair once, separated by commas. Do not + create numbered review-model rows. Repeated use of the same pair needs no duplicate entry. +- Optionally add `- Review rounds: N` when the number of completed review passes is known. Omit it + when unknown; count passes, not individual parallel reviewer agents. +- Use reported model names, `Not used` for a phase without AI involvement, or `Unknown` if the model + was not recorded. Use `Review: Not performed` if there has been no review. The same model is valid + in every phase; never substitute the PR-writing model for an unknown earlier model. +- Include each model's actual reasoning effort as `` `model-name` (reasoning: `medium`) ``. Consult + available session/run metadata first; use `reasoning: Unknown` if unrecorded or + `reasoning: Not exposed` if the tool does not expose the setting. Never infer it from the model + or current configuration; omit effort for `Not used` and `Not performed`. +- Wrap the model name and reasoning value in separate inline-code spans; keep the phase label + and `(reasoning: ...)` punctuation outside them. +- When updating an older PR, collapse per-round rows into Review without losing known model/effort + pairs. Preserve any known round count if included. Update the aggregate after subsequent reviews. + This is informational; it does not require different models or certify review quality. + **QA Notes / Validation:** - QA Notes separate actionable human QA instructions from automated verification coverage. +- Follow `AGENTS.md` under Stable QA case IDs: label manual cases `1.`, `2.` (sub-cases + `2a`, `2b`) and journeys `J1`, `J2`. Keep existing IDs on description updates, assign + new IDs after the highest previously assigned ID, and never reuse retired IDs. + Note removed IDs and reasons outside runnable checkboxes; reference IDs in Evidence, + Gaps and review/test reports, preserving the revision each result actually tested. - Always use this structure: ```md ### QA Notes + #### Setup #### Journeys #### Manual Tests #### Automated Checks + #### Evidence + #### Gaps ``` -- 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. +- Strongly recommend that authors run the tests they request from reviewers before requesting + review, following `AGENTS.md`'s Author verification guidance. Existing mandatory checks still apply. +- Under `#### Evidence`, briefly identify the cases the author actually verified, app commit/build, + environment, observed results, and supporting report/artifact links where useful. Distinguish tests + added or requested from tests executed; a checkbox alone does not establish a complete pass. +- Under `#### Gaps`, identify failed, blocked or untested cases, the failure or reason for not running + them, and missing prerequisites. Do not present partial verification as a complete pass. Use + `None` only when no applicable gaps remain. Documentation-only changes can report documentation + checks and a justified runtime `N/A`. Preserve authored notes and the tested revisions on updates. +- Under `#### Setup`, follow `AGENTS.md`'s Device-test setup guidance: give executable steps or a + reproducible shared recipe plus PR-specific deviations for the journeys/manual tests requested. + Include necessary service/library versions or pins, staging vs local backend/network, flags, + prerequisites, fixture accounts/identities and preparation/reset steps. Resolve recipe branches + to commits. Do not assume staging includes new Shop/Marketplace, Pubky Ring, homeserver or library + versions; explain compatible setup or missing prerequisites. Keep details proportional; use a + justified `N/A` when no device tests apply (for example, documentation-only changes). +- Under `#### Journeys`, list every journey the branch adds or updates (Step 3), one per line as an unchecked checkbox (`- [ ] `), then its stable journey ID (`J1: `), then `new` or `updated`, then the bare journey file name in backticks, then a dash and what the journey proves. - List a journey as `temporary` only when the user asks for one in custom instructions or a `.ai/pr/` file: a reproduction that needs a code change or data that will not exist on master. Put its XML, and any code change as a `.diff`, in a collapsed `
` block under its line, and do not commit it. Never add one unasked; at most, suggest it to the author and ask whether they agree. - 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. @@ -164,7 +238,7 @@ When the user provides custom instructions after `--`: - If no automated checks were run and no automated coverage changed, write `N/A` under `#### Automated Checks`. - Write manual tests using this template: ```md - - [ ] {optional_condition + →} {action} → {expectation} — {missing capability} not in Capabilities + - [ ] {manual_case_id}. {optional_condition + →} {action} → {expectation} — {missing capability} not in Capabilities ``` - 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. @@ -187,8 +261,8 @@ Example: ``` ### QA Notes #### Manual Tests -- [ ] 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 +- [ ] 1. Consumer app → exercise the updated binding flow → behavior matches the previous release +- [ ] 2. `regression:` Android integration screen → trigger the changed API path → no crash or stale data #### Automated Checks - added `FooBindingTest.kt` — covers the updated Android API path - ran `cargo test --all-features` — binding round-trip CI does not build @@ -198,10 +272,10 @@ 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 +- [ ] J1: new `send-amount-over-balance.xml` — error shows before the 15 s timeout +- [ ] J2: updated `lightning-transfer-detail.xml` — Connection opens Channel Detail #### Manual Tests -- [ ] Pair a Trezor over BLE → Home shows the hardware wallet card — BLE pairing not in Capabilities +- [ ] 1. Pair a Trezor over BLE → Home shows the hardware wallet card — BLE pairing not in Capabilities #### Automated Checks - added `TransferViewModelTest.kt` — rejects amounts over the spending balance - updated `SendFlowTest.kt` — fixed-amount invoice skips the Amount screen @@ -227,6 +301,20 @@ Only include if the PR template (`.github/pull_request_template.md`) contains a - Add code comment under each placeholder describing what it should show - Example: `` +### 6b. Verification and review + +Keep development/fix PRs in draft; publish a non-draft PR only when ready for review after +applicable validation. Prefer returning an existing PR to draft while addressing review feedback +before intermediate pushes. Follow `AGENTS.md`'s Draft status and review readiness guidance; +draft status cannot guarantee that all automation skips the PR. Keep explicitly requested drafts +as drafts and do not undraft as a side effect of description updates. + +Before publishing, apply the verification requirements in `AGENTS.md` under Agent workflow. +Prefer an independent subagent review with fresh context as described there before marking the PR +ready for human review. Summarize the models used for completed reviews under Review; include a +round count only when known. +A dry run only prepares the description; it does not certify verification or review completion. + ### 7. Save PR Description Before creating the PR: - Get next PR number: `gh api "repos/$REPO/issues?per_page=1&state=all&sort=created&direction=desc" --jq '.[0].number'` then add 1 (using repo from Step 3) @@ -293,3 +381,18 @@ If no media files were found (generated placeholders): - [ ] VIDEO_2: [description] ``` List all media placeholders as TODOs with their descriptions. + +### Updating an existing PR description + +When the user explicitly requests a model-section, relationship, or full-description update: +- Read the current body and actual base with `gh pr view --json number,url,body,baseRefName`. +- For a model-only update, apply Models used above. For a relationship-only update, apply Related + PRs above. Preserve all other PR content, including authored QA notes and known model metadata. +- For a requested full-description update, gather current branch/issue context using Steps 3–6, + comparing against the PR's actual base. Reconcile with the existing description; retain still-valid + authored QA instructions, stable case IDs and known model/effort pairs rather than replacing them with defaults. +- Save the complete updated body to `.ai/pr_NN.md` using the actual PR number. With `--dry`, stop + after saving and report the path; otherwise apply it with `gh pr edit NN --body-file .ai/pr_NN.md`. +- Description-only updates do not require application checks or an additional code review. When + source also changed, apply the repository verification requirements before publishing the update. +- Keep the existing PR's draft/ready state; do not create a duplicate PR or mark it ready implicitly. diff --git a/.github/copilot-instructions.md b/.github/copilot-instructions.md index d394676840..9aef374f66 100644 --- a/.github/copilot-instructions.md +++ b/.github/copilot-instructions.md @@ -2,6 +2,28 @@ When performing a code review, respond in English. +## Draft PRs + +Check the PR's current draft status before starting and before publishing a review. Skip draft PRs +and wait until they are ready for review, following `AGENTS.md` under Draft status and review +readiness. This guidance does not itself configure or disable external review automation. + +## QA case references + +Follow `AGENTS.md` under Stable QA case IDs. Reference the PR’s existing manual/journey +IDs in review and test reports, with the assessed revision; do not renumber the cases +or present an old result as validation of changed expectations. + +## Review summary metadata + +Follow `AGENTS.md` under "Review summary metadata": end the review summary with +`Reviewer model: ` and `Reasoning effort: ` on separate lines, once per review +(including no-findings reviews), not on each inline comment. Check available session/run metadata +for the actual model and effort; use `Unknown` if unrecorded or `Not exposed` if a reasoning setting +is not exposed. Do not infer values from the tool name or copy the implementation model. If multiple +model/effort combinations contributed, list each pair. This is informational and does not replace +the PR's Models used section. Unassisted human reviews do not need this footer. + ## Architecture & Patterns When performing a code review, ensure ViewModels are never injected as dependencies into services or repositories. Only Android activities and composable functions should use ViewModels. diff --git a/.github/pull_request_template.md b/.github/pull_request_template.md index 92961069d4..d24642809a 100644 --- a/.github/pull_request_template.md +++ b/.github/pull_request_template.md @@ -1,7 +1,14 @@ + + + ### Description @@ -20,14 +27,36 @@ ### QA Notes + + +#### Setup + + + #### Journeys - + #### Manual Tests - + #### Automated Checks + +#### Evidence + + + +#### Gaps + + + +### Models used + + + +- Planning/scoping: `` (reasoning: ``) +- Implementation: `` (reasoning: ``) +- Review: Not performed diff --git a/AGENTS.md b/AGENTS.md index 7a908daeef..c43087a911 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -6,6 +6,137 @@ This file provides guidance to Codex, Claude Code, and Cursor when working with Durable shared agent command specs live in `.agents/commands/`. For PR creation, follow `.agents/commands/pr.md`; `.claude/commands` is a compatibility symlink to the same files. +## Agent workflow + +### Verification + +- For code changes, keep the full gate: run `just compile`, `just test`, and `just lint` before + reporting completion or the final PR creation/update/push. Follow the batching and focused rerun + guidance under Rules; targeted checks during development do not replace this gate. +- Fix relevant local check failures caused by the change and rerun the affected checks before + reporting work as complete, asking the user to run those checks, pushing, or creating/updating a PR. +- If a check is blocked by missing prerequisites or fails for an unrelated reason, investigate and + report the command, cause, and remaining verification. Do not mark blocked, skipped, or unrun + checks as passed or describe validation as complete. +- For user-visible changes, run the relevant journeys when their prerequisites are available; report + any missing prerequisites and untested flows explicitly. +- In the final handoff, report the commands run and their results, including failures and blockers. + Documentation-only changes need documentation checks such as `git diff --check` and validation of + changed references, not an application build or device run. + +### Device-test setup + +When requesting journeys or manual device tests, give the reviewer enough setup instructions to +reproduce their required environment. Under `#### Setup` in PR QA Notes, provide executable steps +or a reproducible shared recipe and describe PR-specific deviations. Include only relevant details: +service/library versions or pinned revisions, staging versus local backend/network, configuration +and flags, prerequisites, fixture accounts/identities, preparation and reset steps. For a recipe on +a branch, record its resolved commit. Do not assume a required Shop/Marketplace, Pubky Ring, +homeserver or library version is available in standard staging; state how to obtain/configure the +compatible version, or identify the missing prerequisite. Keep setup proportional to the requested +cases; documentation-only/no-runtime changes can use `N/A — no device testing required.` with the +reason. Preserve existing authored QA notes when updating a description. + +### Stable QA case IDs + +Assign explicit IDs to PR QA cases: manual tests `1.`, `2.`, etc., sub-cases `2a`, +`2b`, and journeys `J1`, `J2`, etc. IDs are scoped to the PR and remain fixed once +it is open, including while in draft. Use literal labels inside task-list items +(for example `- [ ] 1. ...` and `- [ ] J1: new ...`), not automatically renumbered +ordered lists. + +When updating the description, preserve IDs for the same cases even if their order +changes. Append new IDs after the highest previously assigned ID in that category; +do not fill gaps or reuse removed IDs. Record retired IDs and why the cases were +removed, without leaving them as runnable unchecked tasks. A replacement testing a +different behavior gets a new ID; clarify edits to an existing case so earlier +results are not treated as evidence for changed expectations. Adding or renaming +an ID is not execution evidence. `N/A` sections do not need IDs. + +Reviews, author Evidence/Gaps notes and test reports should refer to these IDs, +together with the tested revision and relevant environment. Preserve historical +results at their original revisions; an unchanged ID does not make a result fresh. + +### Author verification + +Authors are strongly encouraged to run the tests they ask reviewers to perform before requesting +review. In PR QA Notes, briefly record what was verified, on which app commit/build and environment, +the observed results, and supporting evidence where useful. For anything not verified, identify the +affected cases, why they were not run, and any missing prerequisites. Report failures honestly; +partial verification must not be presented as a complete pass. This recommendation complements +mandatory checks above and does not make them optional. For documentation-only changes, report +documentation checks and a justified runtime `N/A`. + +### Draft status and review readiness + +Keep PRs in draft while development or fixes are ongoing. Mark ready for review only when the +change is ready after applicable verification; draft status does not waive the checks above. +Prefer returning a PR to draft while addressing review feedback, before pushing intermediate +commits, to reduce unnecessary automated reviews. Preserve an explicit request to keep a PR draft; +a description update alone must not undraft it. + +AI PR reviewers should check the current draft status before starting and before publishing a +review, skip drafts, and wait until ready for review. This does not prevent author-requested local +validation or an independent pre-publication review of a frozen local candidate. + +Draft is a readiness signal, not a guarantee that every automation stops. The manually dispatched +`.github/workflows/claude-code-review.yml` has no workflow-level draft guard, and its comment +minimization step runs before the review plugin. A hard skip before those steps needs a separate +workflow change; external reviewers also need to honor draft status in their own configuration. + +### Independent review (recommended) + +Before marking a PR ready for human review, preferably run an independent review using a subagent +with fresh context. Give the reviewer the requirements, scope, acceptance criteria, applicable +repository guidelines, and base/head revisions (or the working-tree diff for uncommitted changes). +Include intentional platform differences, but do not pass the implementation conversation or its +justification of the solution. The reviewer should inspect the diff, callers, and surrounding code +independently. Address actionable findings and recheck the affected changes. + +The reviewer may use the same model as the implementation agent. A separate context is the +recommendation; using a different model is not required. If subagents are unavailable, use a separate +review session when practical. Do not describe a self-review as an independent review. + +### Review summary metadata + +AI reviewers should end each review summary with `Reviewer model: ` and +`Reasoning effort: ` on separate lines, including reviews with no findings. Include this +footer once per review, not on each inline comment. It describes that review round and complements +the PR's Models used section; it is informational, not an approval or quality claim. + +Use the model and effort actually used for the review, checking available session/run metadata +first. Use `Unknown` for an unrecorded model or effort, and `Not exposed` for a reasoning setting +the tool does not expose. Never copy the implementation model or infer settings from tool names. +If multiple model/effort combinations contributed, list each pair in the footer. Unassisted human +reviews do not need it; AI-assisted human reviews should identify the AI model and effort used. + +### PR relationships + +Use optional `Twin:`, `Companion:`, and `Dependency:` lines for confirmed related PRs, following +`.agents/commands/pr.md`. A twin is the matching change in the other native Bitkit app; a companion +is related coordinated work such as E2E coverage; a dependency is a prerequisite. Omit absent +relationships and use repository-qualified PR references so cross-repository numbers are unambiguous. + +### Models used + +Include `### Models used` in the PR body with `Planning/scoping`, `Implementation`, and a single +`Review` entry. Record the model names reported by the tools as work proceeds, including subagent +reviews. For each phase, list each distinct model/effort pair once, separated by commas; repeated +reviews with the same pair do not add entries. The same model may appear in all three phases. +Do not add per-round model rows. Optionally add `Review rounds: N` when the number of completed +review passes is known; omit the count when unknown and do not count parallel reviewers as rounds. + +Record reasoning effort alongside each model as `` `model-name` (reasoning: `medium`) ``. Check +available session/run metadata before using `Unknown`. Use `reasoning: Unknown` when unrecorded, +or `reasoning: Not exposed` when the tool does not expose a setting. Do not infer effort from the +model name or apply the current setting to earlier work. Preserve all known model/effort pairs when +updating the section; collapse older per-round rows into the single Review entry. + +This section is informational, not a quality score, verification result, or approval requirement. +Use `Not used` when a phase had no AI involvement and `Unknown` when the model was not recorded; +never infer a model from a tool name. If no review occurred, write `Review: Not performed`. +`Not used` and `Not performed` need no reasoning value. See `.agents/commands/pr.md` for formatting. + ## Build Commands ```sh