Skip to content

CONTRIBUTING-WITH-AI.md

Latest commit

 

History

History
400 lines (324 loc) · 17.3 KB

CONTRIBUTING-WITH-AI.md

File metadata and controls

400 lines (324 loc) · 17.3 KB
Raw

Contributing to Amethyst with AI Assistance

This document is a companion to 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

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:

./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.

Test the release-minified build, not just *-debug

Reflection-touched code paths — ViewModelProvider.Factory, expect/actual boundaries, serialization, custom Compose runtime machinery — silently break under R8 / ProGuard while compiling fine in *-debug builds. The cheapest way to catch this on this codebase is the benchmark build type:

./gradlew :amethyst:installPlayBenchmark

That installs com.vitorpamplona.amethyst.benchmark ("Amy Benchmark" on the home screen) — R8-minified, release-flavoured, profileable = true, side-by-side with your normal Amethyst install via the .benchmark applicationId suffix. Sign in to a test npub and exercise the new flow there before opening the PR. A feature that crashes or silently no-ops in the benchmark build but works in *-debug is a missing keep rule — file it before reviewers find it.

Protocol-introducing changes

If your PR publishes a new event kind, a new tag form, a new marker, or a new interpretation of an existing NIP clause, the diff must also include:

  • Cross-client compatibility section in the PR description. State explicitly: which earlier Amethyst versions honour the new payload (forward/backward compat), whether other major clients (Damus, Primal, Iris, Coracle, etc.) implement the same NIP clause today, and what user-visible behaviour differs for users still on older builds or other clients. Don't paper over this with "syncs across devices" — say which devices, which clients, and which versions.
  • Wire-format verification. Before opening the PR, fetch the published event from a relay and confirm its tags, content, and encryption envelope match what you intended. nak req -i <event-id> wss://<relay> is the standard tool. Paste the relevant tag line (redact private content) into the PR description. This catches bugs that don't show locally — wrong markers, missing relay hints, malformed private-tag JSON.
  • NIP citation. If you claim "NIP-X allows this", quote the exact spec line in the PR. A reviewer should not have to re-derive the authority for your tag form.

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.
  • Strip diagnostic Log.d calls before commit. Logs added during on-device debugging — even lambda-form ones — must be removed from the production diff. They survive R8 stripping only in debug builds, so committed Log.d doesn't directly hurt release performance, but it bloats the diff, scatters noise across logcat for the next developer, and silently grows over time. If a log line is genuinely load-bearing for future incident-response, promote it to Log.i/w with explicit justification in the commit message.

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 for the suite list and commands.

Commands:

./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.

Worked example of the required structure (real shape from a recent feature PR; substitute your own touch points and verifications):

## Test plan

### Feature test plan

- Long-press a reply note → quick-action sheet → "Mute thread" →
  thread disappears from Home immediately.
- Force-stop the app, relaunch → muted thread still hidden (relay
  round-trip verified).
- Settings → Security & Filters → Muted threads → tap "Unmute" →
  thread reappears in Home.

### Regression test plan

Touch points and verification:

- Existing user-mute — could regress through the shared
  `Account.isAcceptable` chokepoint — verified: muted a different
  user, posts hidden as before.
- Hidden words — same chokepoint — verified: added a word, posts
  filtered; removed it, posts back.
- Multi-account switch — could leak mute list across accounts —
  verified: switched between two npubs, each saw only its own mutes.
- R8-minified build — could fail on new ViewModel factory — verified
  on `installPlayBenchmark`.
- Both flavours — built and installed `installPlayDebug`; F-Droid
  build is flavour-irrelevant for this change (no Google-proprietary
  touch points), built only.

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.
  • Multi-device sync when the feature publishes per-account state to relays. Sign in to the same npub on a second device and verify the new state propagates and is honoured there. State explicitly which Amethyst versions both sides need to be running for the sync to work (current build only, or older builds too).
  • 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.
  • Release-minified build (installPlayBenchmark) for any code path that touches reflection — ViewModel factories, expect/actual, custom serialization.

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.

When on-device QA finds bugs in your own diff

On-device QA frequently surfaces defects after the initial implementation reads as "done". Don't fold those fixes silently into the feature commit and force-push — that erases the signal that the defect existed and what it was. Instead:

  • Land each fix as its own commit, or a final squashed "Manual testing fixes" commit at the tip of the branch.
  • The commit message body names the root cause, not just the symptom. Answer the question "why was this missing from the initial diff?" — undocumented event-shape variant, collision with a recently-merged upstream change, Compose recomposition assumption, R8 keep-rule gap, lifecycle race.
  • A reviewer reading your branch sees: feature → code-review cleanup → on-device manual-QA fixes. That's a healthy development arc, not a liability.

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 and .github/PULL_REQUEST_TEMPLATE.md.