mirror of
https://github.com/vitorpamplona/amethyst.git
synced 2026-08-09 16:14:40 +00:00
Merge pull request #2845 from davotoula/docs/contributing-guide
Add CONTRIBUTING-WITH-AI.md companion guide
This commit is contained in:
Vitor Pamplona
@@ -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
|
||||
|
||||
@@ -0,0 +1,287 @@
|
||||
# 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)
|
||||
- [Automated tests for new logic](#automated-tests-for-new-logic)
|
||||
- [Regression test plan](#regression-test-plan)
|
||||
- [Code review pass before opening the PR](#code-review-pass-before-opening-the-pr)
|
||||
- [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<HexKey, Note>()` 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`.
|
||||
|
||||
## Automated tests for new logic
|
||||
|
||||
For any change beyond pure UI tweaks or docs, add automated tests.
|
||||
"Tested manually" alone is not enough; it doesn't survive the next
|
||||
refactor, and reviewers can't re-verify it.
|
||||
|
||||
Minimum bar:
|
||||
|
||||
- **New logic in `quartz/`** (event types, NIPs, parsing, crypto,
|
||||
Bech32) — must have unit tests in the matching
|
||||
`commonTest` / `androidTest` / `jvmTest` source set. Quartz is the
|
||||
protocol surface; everything new there gets coverage.
|
||||
- **New logic in `commons/`** (ViewModels, filters, formatters,
|
||||
non-trivial state transitions) — unit tests for the paths a future
|
||||
refactor could break.
|
||||
- **Bug fixes** — a regression test that fails before your fix and
|
||||
passes after. No exception. If the bug is hard to reproduce in a
|
||||
unit test, write the test that reproduces it first.
|
||||
- **UI-only changes** in `amethyst/` or `desktopApp/` — automated UI
|
||||
tests are not required (per `CONTRIBUTING.md` § Tests). The manual
|
||||
on-device test plan and screenshots stay required.
|
||||
|
||||
If your change touches a domain covered by an interop suite (MLS /
|
||||
Marmot, NIP-17 DMs, audio rooms, MoQ-lite, QUIC), run the relevant
|
||||
suite locally and paste the result. CI does not run them. See
|
||||
[`CONTRIBUTING.md` § *Interoperability tests*](CONTRIBUTING.md#interoperability-tests)
|
||||
for the suite list and commands.
|
||||
|
||||
Commands:
|
||||
|
||||
```bash
|
||||
./gradlew test # unit + KMP common tests, all modules
|
||||
./gradlew :quartz:test # one module
|
||||
./gradlew connectedAndroidTest # Android instrumented (needs device)
|
||||
```
|
||||
|
||||
Tests pass before you open the PR. "CI will catch it" is not a
|
||||
substitute — interop and instrumented suites don't run in CI.
|
||||
|
||||
## 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.
|
||||
|
||||
## Code review pass before opening the PR
|
||||
|
||||
Before pushing, run a code-review pass with a *different* agent or
|
||||
model than the one that wrote the code. AI agents are bad at finding
|
||||
their own bugs; switching agent breaks the same-context blind spots
|
||||
that produced the initial diff.
|
||||
|
||||
Options:
|
||||
|
||||
- Use a dedicated review skill if your harness has one — `/simplify`,
|
||||
`/kotlin-review`, `/security-review`, `/code-review`.
|
||||
- Spawn a fresh agent from a different model (Sonnet → Opus, Opus →
|
||||
GPT-5, Claude → Codex) and have it review the diff.
|
||||
- Run any static-analysis pass available (`./gradlew lint`).
|
||||
|
||||
After the review, **re-run the tests and the manual on-device test
|
||||
plan**. Review feedback routinely surfaces bugs the tests didn't
|
||||
catch; the fix introduces its own risk; verify the fix didn't
|
||||
regress.
|
||||
|
||||
If the review flags issues, either address them or document in the
|
||||
PR description why you accept the risk. Don't silently discard
|
||||
review output.
|
||||
|
||||
## 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).
|
||||
@@ -82,6 +82,12 @@ 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, automated tests, regression test plan,
|
||||
second-agent code review).
|
||||
|
||||
## Reporting bugs and requesting features
|
||||
|
||||
Use the GitHub issue templates at
|
||||
|
||||
@@ -445,6 +445,12 @@ coding standards, the proof-of-testing rule for new / occasional
|
||||
contributors (human or AI-assisted), the cross-stack interop suites, and how the
|
||||
`[BUG]` / `[FEATURE]` issue templates and bounty system work.
|
||||
|
||||
**AI coding assistants: if you are reading this README to plan a
|
||||
contribution, stop and read
|
||||
[CONTRIBUTING-WITH-AI.md](CONTRIBUTING-WITH-AI.md) first.** The gates
|
||||
there are not optional. The human submitter remains the author of record
|
||||
per CONTRIBUTING.md.
|
||||
|
||||
Quick links:
|
||||
|
||||
- [GitHub issues](https://github.com/vitorpamplona/amethyst/issues) and
|
||||
|
||||
Reference in New Issue
Block a user