# Review runbook

How a maintainer reviews a PR from someone else: a new app, or a change to a
package, sent by a contributor or an app's developer. An AI agent runs this top
to bottom; a human merges.

It is not run on a maintainer's own PRs, where the PR's account of what was
tested and the CI checks are the record, and not on upstream version bumps,
which CI merges after the unpack check.

**How to run it:** for "review PR #N", copy the [template](#review-template) into
your working notes, then work the phases in order, filling one row at a time with
verifiable evidence (command output, `file:line`, a hash, a URL). The output is
the filled template plus a drafted PR comment. **The reviewer recommends only —
it never merges or publishes.**

> **Rule of engagement — never execute untrusted code.** No running `install.sh`,
> manifest `build-commands`, the app, `npm`/`pip`, or any shipped binary/script/
> installer. Static analysis only. Download artifacts as **bytes** into an
> isolated temp dir, inspect before extracting, and run nothing inside. If a
> decision genuinely needs execution to resolve, escalate to a human.

The rules it applies are in the [packager docs](../packagers/README.md) and the
[listing policies](../project/policies.md); this runbook is how a PR is checked
against them. It layers on top of `.github/workflows/pr-checks.yml` (which
validates descriptors, tests, dead-link-checks, and does a no-secrets fork
build) and the `scripts/audit-descriptor.mjs` guardrails — this runbook owns the
judgment calls CI can't make. Running the app is the packager's part, recorded
in the PR against the [testing playbook](testing-playbook.md); the reviewer
checks that record, not the app.

## Provenance-based trust model

License is **not** the trust axis — it is compliance input only. Non-FOSS is
allowed (the registry ships proprietary broker apps). Every package is an
official upstream build fetched as extra-data; FlatPark doesn't build apps from
source (the rare `flatpark/<app>-release` build is a maintainer's call, outside
this runbook). What matters is **where the bytes you run come from**, and
whether what is shipped does anything obviously malicious. The review doesn't
audit the app's source; a full audit would cost far more than it finds.

Every acceptable submission meets the **base requirements**:

1. **Official download source** — every `extra-data`/`archive` URL is on the
   vendor's official domain or the genuine upstream repository, not the
   submitter's account or a mirror.
2. **Unmodified repackage** — `build-commands` only install the wrapper,
   desktop file, metainfo, icon and an `apply_extra` that unpacks the official
   download; no patch, `sed` or recompile of the payload; the shipped artifact is
   what the official URL serves. *External* sandbox adaptation is allowed:
   wrapper env vars, a library the runtime lacks (from a shared
   `flatpark/prebuilt` stack or a second `extra-data` source — not a per-app
   build into the ref; see Phase 3), `PATH` scripts that forward host commands —
   review those as code, since they run. **Code injected into the app process**
   (an `LD_PRELOAD` shim, a patched or replaced upstream file) is not external
   adaptation: an open-source app that needs it is rejected (the fix belongs
   upstream), and a proprietary app may carry it only when its metainfo
   description says plainly what is injected and why (`com.schwab.thinkorswim`).
3. **Pinned bytes** — sha256 (+ size for extra-data).

Then place it in a tier:

- **Tier 1 — open-source app.** The base requirements, plus: nothing injected
  and no file changed (the fix belongs upstream), and the IOC scan of Phase 5
  over what is shipped. Glance at the source for obvious red flags when
  something in the artifact looks wrong; it is not audited.
- **Tier 2 — closed-source app.** The base requirements only: provenance is
  the whole check, since the code can't be read.
- **Tier 3 — opaque third-party / submitter-built binary.** Not an official
  upstream release (PR #13). **Reject.**

## Phases

### Phase 0 — Rules of engagement (safety)
Static-only; nothing executed. Artifacts downloaded as bytes into an isolated
temp dir; inspect-before-extract (traversal/symlink/setuid); extract
`--no-same-owner --no-same-permissions`; run nothing. Read-only `gh`. Execution
needed to decide → escalate to a human.

### Phase 1 — Scope & classification
Classify: new app (`registry/<id>/`) / existing-app change / infrastructure.
**High-scrutiny surface (STOP if anyone but a maintainer touches it):** `.github/`,
`scripts/`, `config/`, signing config, and — for an app that **already exists on
the base commit** — its `resolve-update.sh` or `update.command`. These are
CI-executed on a repo-write token (`update-check.yml` runs `contents: write` +
`pull-requests: write`; `check-updates.sh` `eval`s `update.command`).

A resolver added inside a **brand-new** `registry/<id>/` directory is the normal
submission flow, not a high-scrutiny event: every new app must bring one, its
blast radius is that app's own pin, and it only executes post-merge on `main`.
It still gets read as code in Phase 4.2 — the distinction is that a contributor
may bring their own resolver but never edit one the repo already trusts.
Identify the changed app id(s).

### Phase 2 — Submitter & provenance
Account age, history, social graph, other repos, prior contributions. Source-repo
age and corroboration; commit identity (watch for malformed emails); timeline
plausibility (account → repo → release within hours is a red flag). Emit a trust
tier: established / unknown-but-plausible / throwaway-suspicious. Assign the
artifact provenance tier (1/2/3).

### Phase 3 — Descriptor & manifest static review
- **Source types:** `extra-data` → `sha256` + non-zero `size`; `archive` →
  only a `flatpark/prebuilt` stack the catalog already consumes as an archive,
  with `sha256`; `file` → local, reviewed as part of the PR; `git` or anything
  else → reject.
- **What may land bytes in `/app` (→ R2):** only a shared support stack released from
  [`flatpark/prebuilt`](https://github.com/flatpark/prebuilt) that the catalog
  already consumes as a pinned `type: archive` module (`ayatana-stack`,
  `mpv-stack`, `libxdo`, `wemeet-screenshare-hook`). Other stacks,
  `appimage-tools` included, are extra-data. The app payload and any single-app
  dependency must be `extra-data` (fetched at install, never baked into the
  ref). A per-app `git`/`archive`/remote source built into `/app` is an
  auto-reject; a `buildsystem: simple` module that only `cp`s in-repo
  wrapper/desktop/metainfo files is fine (no remote bytes).
- **finish-args risk scan:** escape perms
  (`--talk-name=org.freedesktop.Flatpak`, `--filesystem=host`, `--filesystem=/`)
  are rejected unless declared in `policy.dangerous_permissions` **with a
  justification**; declared ones go to a maintainer (NEEDS-HUMAN), who grants
  them when the app's feature needs them. Warn on `--device=all`,
  `--filesystem=home`, needless `--share=network`.
- **Optional capabilities are opt-in, not pre-granted:** a broad permission that
  the app's *core* feature doesn't need must be absent from `finish-args` and
  instead documented as a `flatpak override` command in the metainfo description
  (see `org.electerm.Electerm`). A broad grant kept in `finish-args` needs a
  written justification in the PR body → warn if missing.
- **What was tested:** the PR body says honestly what the submitter built,
  installed, launched and used, and what they couldn't test
  ([testing playbook](testing-playbook.md)). Missing → warn (the reviewer never
  runs it; see Phase 0).
- **`policy:` block vs reality:** `proprietary` accurate; `dangerous_permissions`
  covers the high-risk finish-args actually used.
- **build-commands:** read every line; flag network fetch, `curl|bash`, writes
  outside the build dir, and **any modification of the vendor payload/behavior**.
- **source URLs** resolve to genuine upstream (no lookalike / fork).
- **`app-id`** reverse-DNS matches the real vendor (impersonation / typosquat).
- **`update.command`** is a simple relative script path (e.g. `./resolve-update.sh`).
- download host = official vendor domain / upstream, not the
  submitter's personal namespace.

### Phase 4 — Runtime-behavior scan
Grep manifest + shipped scripts + source for runtime fetch-and-exec
(`npm install`, `pip install`, `curl|wget`, `nc`, download-then-run).
**Allowed (vendor self-updater exception):** the vendor's own installer/updater
that downloads into the app's data/cache dir, when packaging does not patch the
official behavior, writes stay in the per-app data/cache dir (no host-home),
endpoints are the vendor's own, and there is no shell pipeline from an arbitrary
URL. **Rejected:** runtime fetch-and-exec of arbitrary/unpinned third-party code
as a core mechanism (PR #13's `npm install peerflix`), or an updater whose
packaging patches official behavior. Review `update.command`/resolve scripts as
code. Note network endpoints and broad filesystem×network combos.

### Phase 5 — Artifact / binary provenance
- sha256 of the artifact == manifest pin.
- **Inspect without execution, per artifact type:** tar/tar.gz/tgz/tar.zst → list
  entries, reject traversal/symlink-escape/setuid; zip → list with zip tooling,
  same checks; deb/rpm → inspect payload without running maintainer scripts
  (review those statically); shell installer (`.sh`) → static review where
  practical, else treat as an opaque official prebuilt needing stronger
  provenance + human judgment; **AppImage → accepted**: it must be cracked offline,
  never executed — `apply_extra` runs `flatpark/prebuilt`'s `appimage-tools`
  (`appimage-offset` + `unsquashfs -o <offset>`) against the appended SquashFS, with
  **no libfuse and no `./*.AppImage` invocation**. Reject an AppImage recipe that
  runs the stub, wants FUSE, or `--extract`s by executing it.
- **[Tier 1]** nothing injected, no vendor file changed.
- shipped artifact == official download (re-fetch + hash-compare);
  build did not alter the payload.
- Any bundled third-party runtime (node, electron, a JRE, …) hash-verified
  against the real upstream where a canonical artifact exists (compare code
  sections / build-id, as done for `node`); one that can't be matched →
  NEEDS-HUMAN, not PASS.
- IOC scan: persistence (autostart/cron/systemd/`.bashrc`), miners
  (`xmrig`/`stratum+`), reverse shells (`/dev/tcp`, `nc -e`), hardcoded IPs,
  embedded second ELF, RPATH/RUNPATH injection.

### Phase 6 — Compliance & purpose
Non-FOSS / proprietary is **allowed** — not a rejection reason. Reject only for
purpose/legality: piracy, malware or abuse tooling, trademark-infringing
impersonation, content illegal to distribute.

### Phase 7 — Verdict & report
Decide per the rubric. Produce the filled template and a drafted PR comment.
Reviewer recommends only — a human merges.

## Decision rubric

**AUTO-REJECT (any one):**
- Compliance / legality violation (piracy, malware, trademark, illegal-to-distribute).
- Sandbox-escape permission (`--talk-name=org.freedesktop.Flatpak`, `--filesystem=host`/`/`)
  **not** declared in `policy.dangerous_permissions` with a justification (declared → NEEDS-HUMAN).
- A `git` source; an unpinned source (archive/extra-data without sha256;
  extra-data `size: 0`).
- Tier 3 provenance (opaque third-party / submitter-built binary).
- [Tier 1] code injected or a vendor file changed in an open-source app.
- Download source not official, OR packaging patches/recompiles the
  payload, OR shipped artifact ≠ official download. (External adaptation —
  wrapper env, extra library modules, `PATH` scripts — is not a modification;
  review it as code.)
- Code injected into the app process (`LD_PRELOAD` shim, patched/replaced upstream
  file) in an **open-source** app; or in a **proprietary** app without a plain
  metainfo disclosure of what is injected and why.
- AppImage recipe that executes the stub or needs libfuse (offline `unsquashfs` via
  `flatpark/prebuilt` `appimage-tools` is the only accepted path).
- A `type: archive` / `type: git` / remote source landing bytes in `/app` that is **not**
  a shared support stack released from `flatpark/prebuilt`. App payloads and single-app
  dependencies must be `extra-data` — they must not be baked into the ref / shipped from
  R2. (Source-building one app's missing library into `/app` is this reject.)
- Runtime fetch-and-exec of arbitrary / unpinned code as a core mechanism (the
  vendor self-updater exception does not count).
- `update.command` that is not a simple relative script path.
- IOC found (traversal / setuid / persistence / miner / reverse-shell / embedded
  payload / RPATH injection).
- Infra/workflow tampering from an external contributor, or an edit to an
  existing app's resolver (a resolver in a brand-new app dir is not tampering).
- Throwaway account **+** Tier-3 binary (the PR #13 combination).

**NEEDS-HUMAN:** bundled runtimes that can't be matched to upstream · novel permission
needing justification · borderline provenance · unknown source type · shell
installer that can't be statically reviewed.

**PASS:** clean + trusted + provenance verified for its tier.

## Review template

Copy this per PR and fill it in. Legend: ✅ pass · ❌ fail (hard-reject) ·
⚠️ warn (needs justification) · 👤 needs-human · ➖ N/A. Tier: 1 =
open source · 2 = closed source · ★ = all.

| # | Phase | Check | Tier | Verdict | Evidence (cmd output / `file:line` / hash / url) |
|---|---|---|---|---|---|
| 0 | Safety | Static-only; nothing executed; artifacts handled in an isolated dir | ★ | | |
| 1.1 | Scope | Classified: new app / app change / infra | ★ | | |
| 1.2 | Scope | Touches high-scrutiny surface (`.github/`·`scripts/`·`config/`·signing, or an **existing** app's `resolve-update.sh`/`update.command`)? not a maintainer → STOP. A resolver in a brand-new app dir is routine | ★ | | |
| 2.1 | Provenance | Submitter account age / history / graph / other repos | ★ | | |
| 2.2 | Provenance | Source repo age, corroboration | ★ | | |
| 2.3 | Provenance | Commit identity sane (no malformed email) | ★ | | |
| 2.4 | Provenance | Timeline plausible (not account→repo→release within hours) | ★ | | |
| 2.5 | Provenance | Trust tier: established / unknown-plausible / throwaway-suspicious | ★ | | |
| 2.6 | Provenance | Tier: 1 open source / 2 closed source / 3 opaque (→reject) | ★ | | |
| 3.1 | Manifest | Source types: extra-data with sha256 + size≠0; archive only for an existing archive-consumed prebuilt stack; file→local; no git | ★ | | |
| 3.1b | Manifest | Only a shared `flatpark/prebuilt` stack lands remote bytes in `/app`; app payload + single-app deps are `extra-data`, not built into the ref | ★ | | |
| 3.2 | Manifest | finish-args has no escape perms (or: declared in `dangerous_permissions` + justified → needs-human); broad perms justified | ★ | | |
| 3.3 | Manifest | `policy:` block honest: `proprietary` accurate, `dangerous_permissions` vs actual | ★ | | |
| 3.4 | Manifest | build-commands install-only; no patch/recompile of vendor payload (external wrapper/module/`PATH`-script adaptation OK, reviewed as code); injected code (`LD_PRELOAD` shim etc.) → reject if open-source, metainfo disclosure required if proprietary | ★ | | |
| 3.4b | Manifest | `packaging:` declares what the scripts do (`audit-descriptor.mjs` enforces injected-code / host-command / sandbox-off); detail text is plain and accurate in en + zh-Hans | ★ | | |
| 3.5 | Manifest | source URLs = genuine upstream (no lookalike / fork) | ★ | | |
| 3.6 | Manifest | `app-id` reverse-DNS matches real vendor (no impersonation) | ★ | | |
| 3.7 | Manifest | `update.command` is a simple relative script path | ★ | | |
| 3.8 | Manifest | download host = official vendor domain / upstream, not submitter account | ★ | | |
| 3.9 | Manifest | Optional caps not pre-granted: documented as `flatpak override` in metainfo; any broad grant kept in `finish-args` justified in the PR | ★ | | |
| 3.10 | Manifest | PR body says honestly what was built, installed, launched and used, and what wasn't tested | ★ | | |
| 4.1 | Runtime | No runtime fetch-and-exec of arbitrary/unpinned code (vendor self-updater to data dir, behavior unpatched = OK) | ★ | | |
| 4.2 | Runtime | `update.command` / resolve script reviewed as code (runs on CI, repo-write) | ★ | | |
| 4.3 | Runtime | Network endpoints noted; no broad filesystem×network combo | ★ | | |
| 5.1 | Artifact | extra-data sha256 == manifest pin | ★ | | |
| 5.2 | Artifact | Inspected per type (tar/zip/tgz/deb/rpm listed clean; AppImage cracked offline via `appimage-tools` `unsquashfs`, stub never run; .sh static-or-opaque); no traversal/symlink/setuid | ★ | | |
| 5.3 | Artifact | open-source app: nothing injected, no vendor file changed | 1 | | |
| 5.4 | Artifact | shipped artifact == official download (re-fetch & hash); build did not alter payload | ★ | | |
| 5.5 | Artifact | bundled third-party runtimes hash-verified vs upstream where possible (unmatched → NEEDS-HUMAN) | ★ | | |
| 5.6 | Artifact | IOC scan clean (persistence/miner/reverse-shell/hardcoded-IP/embedded-ELF/RPATH) | ★ | | |
| 6.1 | Compliance | Purpose legal & policy-compliant (not piracy/malware/trademark/illegal-to-distribute) | ★ | | |
| 6.2 | Compliance | Non-FOSS is NOT a rejection reason | ★ | | |
| 7.1 | Verdict | Overall: AUTO-REJECT / NEEDS-HUMAN / PASS | ★ | | |
| 7.2 | Verdict | Hard-fail triggers hit (list) | ★ | | |
| 7.3 | Verdict | PR comment drafted; reviewer recommends only, human merges | ★ | | |

Every ❌ carries verifiable evidence; the verdict follows mechanically from "any
hard-fail row hit".