From 3ba2049566ac365350629639f1161e7229194fee Mon Sep 17 00:00:00 2001 From: davotoula Date: Mon, 11 May 2026 19:31:05 +0200 Subject: [PATCH] docs(contributing): add CONTRIBUTING-WITH-AI.md companion guide Companion to CONTRIBUTING.md targeting external PRs whose diff was substantially authored by an AI coding assistant. Adds five gates the existing doc does not cover: - Research before code (issue still valid, decentralisation fit) - Build and install both flavours (Play + F-Droid) - Performance and resource hygiene (main-thread, coroutines, LocalCache reuse, relay subscriptions, KMP source sets, lambda Log) - Regression test plan alongside feature test plan - Don't-touch list (signer/KeyStore, release pipeline, NIP direction) Filename deliberately avoids AGENTS.md to keep internal dev-team agents unaffected by auto-load conventions. CONTRIBUTING.md and the PR template each gain a one-line pointer. Co-Authored-By: Claude Opus 4.7 (1M context) --- .github/PULL_REQUEST_TEMPLATE.md | 4 + CONTRIBUTING-WITH-AI.md | 222 +++++++++++++++++++++++++++++++ CONTRIBUTING.md | 5 + 3 files changed, 231 insertions(+) create mode 100644 CONTRIBUTING-WITH-AI.md diff --git a/.github/PULL_REQUEST_TEMPLATE.md b/.github/PULL_REQUEST_TEMPLATE.md index 5aa9fbfcb..d7a136344 100644 --- a/.github/PULL_REQUEST_TEMPLATE.md +++ b/.github/PULL_REQUEST_TEMPLATE.md @@ -57,6 +57,10 @@ appreciated when an assistant did the bulk of the diff. --> - [ ] Drafted with AI assistance, manually reviewed and tested - [ ] Written by hand +If "Drafted with AI assistance" is ticked, also read +[`CONTRIBUTING-WITH-AI.md`](CONTRIBUTING-WITH-AI.md) for the additional +gates that apply to AI-authored PRs. + ## License - [ ] By submitting this PR, I agree to license my contribution under the diff --git a/CONTRIBUTING-WITH-AI.md b/CONTRIBUTING-WITH-AI.md new file mode 100644 index 000000000..c98dc3b3e --- /dev/null +++ b/CONTRIBUTING-WITH-AI.md @@ -0,0 +1,222 @@ +# Contributing to Amethyst with AI Assistance + +This document is a companion to [`CONTRIBUTING.md`](CONTRIBUTING.md). +Everything in `CONTRIBUTING.md` applies to every contribution. This doc +adds gates specific to pull requests whose diff was substantially +authored by an AI coding assistant (Claude Code, Copilot, Cursor, +Codex, etc.). Where this doc and `CONTRIBUTING.md` differ, the +stricter rule wins. + +If you are not using an AI assistant, you can stop reading here. + +- [Research before code](#research-before-code) +- [Build and install both flavours](#build-and-install-both-flavours) +- [Performance and resource hygiene](#performance-and-resource-hygiene) +- [Regression test plan](#regression-test-plan) +- [Don't touch without an issue first](#dont-touch-without-an-issue-first) +- [Everything else](#everything-else) + +--- + +## Research before code + +AI agents are good at writing plausible-looking code for issues that +no longer make sense. Before you (or your assistant) write a line of +code, confirm the issue still wants to be implemented. + +- **Issue still valid.** The issue is open, not superseded by a merged + PR, not blocked by a NIP change, and not declared out of scope. Old + bountied issues fail these checks routinely. +- **Post a research summary on the issue first.** A one-paragraph + comment stating your read of the problem, the approach you intend + to take, and the modules you expect to touch. Give maintainers a + chance to flag it as stale before you invest in a diff. +- **No duplicate PR.** Search open and recently-closed PRs for the + same feature. If a prior attempt exists, link to it and explain + what you do differently. +- **Fits Amethyst's nature.** The feature must work in a decentralised + client: no central server, no maintainer-controlled state, no + required third-party account. If the proposal assumes any of these, + the feature doesn't belong in Amethyst, regardless of the bounty. +- **NIPs still current.** If the issue references a specific NIP, + check it hasn't been deprecated or superseded. + +## Build and install both flavours + +Any change that could differ between flavours — UI, services, +dependencies, `AndroidManifest.xml`, ProGuard rules — must build and +install on both Play and F-Droid: + +```bash +./gradlew installPlayDebug +./gradlew installFdroidDebug +``` + +Paste the `BUILD SUCCESSFUL` tail of both into the PR description. + +**Why both.** F-Droid drops Google-proprietary dependencies (Play +Services, Firebase, Cast SDK, etc.). Code that compiles only on Play +is rejected. The canonical recent example is the Chromecast feature: +the Google Cast SDK is a Play-only dependency, so F-Droid required a +separate stub implementation under `amethyst/src/fdroid/`. Agents +routinely add Play-only imports without realising the F-Droid build +breaks. + +If a change is conclusively flavour-irrelevant (a pure `quartz/` +protocol fix, a docs change, a translation), one flavour is enough — +say which and why in the PR description. + +## Performance and resource hygiene + +Code that looks fine in review can wreck the app at runtime. The +following are common AI-agent footguns in this codebase. None of them +trip CI — they only surface in careful manual review or on a real +device. PRs that introduce any of them will be sent back. + +### UI thread and recomposition + +- **No main-thread blocking.** Network, JSON parsing, regex, crypto, + file I/O, and DB queries belong on `Dispatchers.IO` or + `Dispatchers.Default`. Never `runBlocking { ... }` from a Composable, + click handler, or `LaunchedEffect`. +- **Hoist work out of `@Composable` bodies and `LazyColumn` item + content.** Parsing, list filtering, building maps, allocating data + classes all belong in `remember`, `derivedStateOf`, or the + ViewModel, not in the render path. New lambdas allocated per render + also defeat `@Stable` and cause unnecessary recomposition of + children. +- **Use `collectAsStateWithLifecycle()`** for Flow → Compose, not + `collectAsState()`, so collection pauses when the screen is + off-screen. + +### Coroutines and scoping + +- **No `GlobalScope.launch` and no ad-hoc `CoroutineScope(Job())`.** + Use `viewModelScope`, a lifecycle scope, or a passed-in + `CoroutineScope`, so cancellation propagates on logout, navigation, + or process death. +- **Don't put suspend work in `init {}`** of ViewModels. It runs on + whatever thread constructed the VM and can't be cancelled. Use a + `MutableStateFlow` + `viewModelScope.launch` pattern. + +### Memory and caching + +- **Don't build a parallel cache of Notes, Users, or Events.** + Amethyst stores them once in `LocalCache` (backed by `LargeCache`), + keyed by id or pubkey, mutable in place. A new + `mutableMapOf()` in your feature doubles the working + set and gets stale. +- **Don't roll your own image cache.** Coil is wired up with + size-aware loaders. Decoding a full-resolution image yourself will + OOM mid-scroll. +- **Bound your collections.** Unbounded `mutableMapOf` or + `mutableListOf` that accrue per-event entries are memory leaks. If + you mean "the last N", use a size-bounded structure. + +### Relay traffic and mobile network + +- **Use the existing subscription layer.** + `ComposeSubscriptionManager`, `Subscribable`, and the filter + assemblers under `commons/.../relayClient/` are lifecycle-aware, + deduped, and EOSE-closed. Don't open ad-hoc WebSockets and don't + issue raw `REQ` filters from a Composable. +- **Don't re-fetch what's already in `LocalCache`.** Check the cache + first; only subscribe for what's missing. +- **Respect data-saver and connectivity context.** Auto-fetching + full-resolution video on cellular is a regression even if the code + technically works. + +### KMP source-set discipline + +- **Android-only imports don't belong in `commons/commonMain` or + `quartz/commonMain`.** Use `expect`/`actual` for platform-specific + bits, or move the Android-specific code to `androidMain`. + +### Logging + +- **Use the Quartz lambda Log.** + `com.vitorpamplona.quartz.utils.Log.d { "msg $x" }` — the lambda + body only runs when the log level is enabled. Plain + `Log.d("msg $x")` allocates the formatted string on every call, + including in feed and scroll hot paths. + +Relevant skills under `.claude/skills/`: `account-state`, +`relay-client`, `kotlin-coroutines`, `kotlin-multiplatform`, +`find-non-lambda-logs`. + +## Regression test plan + +The PR template has a **Test plan** section. For AI-authored PRs that +aren't pure docs or translations, that section must contain *two* +parts under these exact subheadings: + +- `### Feature test plan` — what you did to confirm the new thing works. +- `### Regression test plan` — what you did to confirm the old things + still work, and that you actually thought about which ones could + break. + +Don't add a new top-level section to the PR description — put both +subheadings inside the existing **Test plan** section. + +For the regression test plan, list: + +1. **Touch points** — screens, flows, ViewModels, shared state, or + modules your change reads from or modifies. List the ones a + careful reader would expect to be affected, not the entire app. +2. **Failure mode** — for each touch point, what would actually go + wrong if your change is buggy. "Feed wouldn't load." "Metadata + stale across account switch." "OOM on scroll." +3. **Verification** — what you did to confirm it still works. Same + `action → observed result` format as the feature test plan. + +Worked example, for "add a new field to `Account`": + +- Account creation — could crash on first launch — verified: created + a fresh npub, app opened home feed. +- Account switching — could leak state across users — verified: + switched twice between two npubs, feeds refreshed. +- Settings export/import — could corrupt restore — verified: + exported, wiped data, re-imported, no errors. + +Common touch-point categories worth scanning every PR for: + +- Account / login / logout / multi-account switch. +- Both flavours (Play and F-Droid). +- Feed types: home, profile, hashtag, bookmarks, notifications, + DMs, communities. +- Orientation changes. +- Cold start vs warm start. +- Background → foreground transitions. + +If a touch point can't reasonably be verified (it would require a +relay matrix you don't have, or a device combination you can't +access), state so and explain why you accept the risk. A reviewer +can tell you to do it anyway, but silent omission is not an option. + +## Don't touch without an issue first + +Open an issue and get explicit maintainer alignment before opening a +PR for: + +- **Signer and KeyStore surface** — `NostrSigner` and its + implementations (`NostrSignerInternal`, `NostrSignerRemote`, + `NostrSignerExternal`), anything touching key storage or the + signing flow. +- **Release pipeline** — workflow files under `.github/workflows/`, + signing config, Gradle plugins, packaging. +- **NIP direction calls** — anything that changes how Amethyst + interprets a NIP, or invents a new tag or kind, needs upstream NIP + discussion (and likely a NIP PR) first. + +These areas have a high cost when an unaligned PR lands — security +risk, release breakage, protocol fragmentation. Open the issue first +regardless of whether you call your change a bug fix, refactor, or +feature; describe what you observed and what you propose. A maintainer +will tell you to skip the issue gate if the change is genuinely +trivial. + +## Everything else + +For commit format, dev setup, interop tests, PR structure, translation +flow, and coding standards — see [`CONTRIBUTING.md`](CONTRIBUTING.md) +and [`.github/PULL_REQUEST_TEMPLATE.md`](.github/PULL_REQUEST_TEMPLATE.md). diff --git a/CONTRIBUTING.md b/CONTRIBUTING.md index d955010bc..985fc8535 100644 --- a/CONTRIBUTING.md +++ b/CONTRIBUTING.md @@ -82,6 +82,11 @@ If you can't run a particular target locally (e.g. no macOS, but your change affects the DMG build), say so explicitly. Honest "I couldn't test this on Windows" beats a silent guess. +If you used an AI coding assistant for a substantial portion of the diff, +also read [`CONTRIBUTING-WITH-AI.md`](CONTRIBUTING-WITH-AI.md) — it adds +gates specific to AI-authored PRs (research before code, both-flavour +build, performance footguns, regression test plan). + ## Reporting bugs and requesting features Use the GitHub issue templates at