Skip to content

feat(app): a GitHub App that reviews pull requests, forks included - #209

Open
anirudhkumar-nanonets wants to merge 5 commits into
mainfrom
feat/github-app
Open

feat(app): a GitHub App that reviews pull requests, forks included#209
anirudhkumar-nanonets wants to merge 5 commits into
mainfrom
feat/github-app

Conversation

@anirudhkumar-nanonets

Copy link
Copy Markdown
Collaborator

21 of our 30 open PRs come from forks, and not one of them can get a blast-radius comment: a pull_request job on a fork gets a read-only token, and pull_request_target cannot check out fork code without opting into running it. No amount of workflow YAML fixes that. Every competitor in this space — CodeRabbit, Qodo, Greptile, Graphite — ships as a GitHub App for exactly this reason.

An App's installation token belongs to the base repository, so a fork PR is ordinary work. Two things come free with it: the App serves the viewer page itself, so a private repo never needs a public gh-pages (Pages on a private repo is Enterprise-only), and installing is one click instead of a workflow file per repo.

What's here

src/app/ — around 600 lines, no new dependencies. node:http and node:crypto only: this process holds a private key, a webhook secret and other people's source code, so every dependency is one more thing to trust.

  • identity — App JWT (backdated a minute, because GitHub rejects one issued in its future and a fast clock then looks like a bad key) → installation token, cached and renewed before expiry.
  • events — narrows deliveries to the four actions that change a diff. Drafts are skipped until ready_for_review; a bot commenting on every push to a draft is the fastest way to be uninstalled.
  • queue — supersedes queued work per PR (five pushes in a minute = one review, since the first four comments get overwritten anyway) and caps concurrency so one repo cannot starve the rest.
  • checkout — fetches refs/pull/<n>/merge and the base, shallow.
  • reviewbuildGraphblastRadiusInmarkdownReport → upsert the comment by marker, all in-process against the same functions the CLI calls, so the App and graft blast on a laptop cannot drift.
  • pages — signed, expiring /p/<id>?t=<hmac>.

The security rule that shapes it

We clone attacker-authored code on every fork PR while holding a token for the base repo. So: nothing from the repo is executed — no install, no build, no hooks; the graph is tree-sitter reading source text. Git is told the same (core.hooksPath=/dev/null, GIT_CONFIG_NOSYSTEM=1, no submodule recursion). The token is passed as a per-invocation auth header, never baked into a remote URL that .git/config and the reflog would keep — asserted in a test — and it is redacted out of error text before anything is logged. An unknown page and a bad token both return 404, so the page endpoint cannot be used to enumerate pull requests.

Tests

779 pass. The interesting ones need no network and no credentials: the JWT is verified against a generated public key, the checkout test stands up a local --mirror with a real refs/pull/7/merge and asserts we land on the merge commit with the base available to diff, and the server tests drive real sockets with signed and forged deliveries.

Not built yet (documented in docs/github-app.md)

Naming falls back to hub symbols — sending a private repo's source to a model should be an explicit per-installation opt-in, not a default. Pages are in-memory, so a deploy drops them and the next push rebuilds. The comment's evidence quotes need #180.

Installing still requires admin on the target repo — that is the one thing an App does not get around, so this does not help assign.

A workflow on a fork's PR gets a read-only token and cannot comment; an App's
installation token belongs to the base repo, so a fork is ordinary work. It also
serves the viewer page itself behind a signed expiring link, which is what a
private repo needs and GitHub Pages cannot give.

node:http and node:crypto only — this process holds a private key, a webhook
secret and other people's source, so every dependency is one more thing to
trust. The PR's code is never executed: no install, no build, no hooks, just
tree-sitter reading source text.
@github-actions

github-actions Bot commented Aug 24, 2026

Copy link
Copy Markdown
Contributor

🌱 graft blast radius

1 area changed → 2 areas can be affected. 5 dependent symbols, depth 2.
Tests: 1 area updated its tests.

flowchart TB
  A0(("Code Graph Loading<br/>3 symbols"))
  A1(("Telemetry Flushing<br/>2 symbols"))
  classDef reached fill:#D9EDF3,stroke:#3AA7C9,stroke-width:1.5px,color:#0E313C;
  class A0,A1 reached;
Loading
Can be affected Symbols Nearest hop Reached from
Code Graph Loading 3 viewer/data.ts:L139-L162 loadCodeGraph — calls, depth 1 test/app-identity.test.ts
Telemetry Flushing 2 src/telemetry/send.ts:L72-L91 sendBatch — calls, depth 1 test/app-identity.test.ts
All 5 dependent symbols, grouped by area

Code Graph Loading — 3 symbols in 2 files

  • viewer/data.ts:L139-L162 — loadCodeGraph (calls, depth 1)
    145: const res = await fetch("/api/code-graph");
  • viewer/data.ts:L123-L127 — loadContextGraph (calls, depth 1)
    125: const res = await fetch("/api/context-graph");
  • viewer/main.ts:L273-L295 — loadAll (calls, depth 2)

Telemetry Flushing — 2 symbols in 2 files

  • src/telemetry/send.ts:L72-L91 — sendBatch (calls, depth 1)
    77: const res = await fetch(`${posthogHost()}${INGEST_PATH}`, {
  • src/telemetry/flush.ts:L68-L75 — runFlush (calls, depth 2)
    71: const events = drain(home);
Test signal per changed area — 1 ✓

Reached = a node under a test path has a resolved edge into the changed symbol. It undercounts anything called indirectly — through a CLI, a spawned process or a dynamic import — so read a low ratio as “look here”, never as a coverage gate.

  • jobKey — 10 of 34 reached · 4 test files changed here: test/app-checkout.test.ts, test/app-events.test.ts, test/app-identity.test.ts, test/app-server.test.ts
    • not reached: cleanup, git, b64url, constructor, invalidate, privateKey, required, constructor, …16 more
2 test suites also reference this code

2 symbols, kept out of the diagram and the table so they cannot crowd out the areas a reviewer has to look at.

  • test/telemetry-send.test.ts
  • test/viz-serve.test.ts

⚠️ 3 changed files not in the graph (Dockerfile, deploy/apprunner.sh, docs/github-app.md) — no parser claims the extension, or the index predates the file.

graft blast · origin/main...HEAD · depth 2 · 15 changed files

Open the interactive graph → — click an area to see its dependent symbols at file:line.

github-actions Bot added a commit that referenced this pull request Aug 24, 2026
github-actions Bot added a commit that referenced this pull request Aug 25, 2026
package.json runs scripts/postinstall.mjs on install, and copying only the
manifests left node unable to find it — it exits 1 before the script's own
"never fail an install" guard runs. --ignore-scripts is not the way out:
tree-sitter builds its native bindings in those same hooks.
npm ci runs this package's prepare script, which is the build, so the sources
have to be there before the install — not after. And the runtime stage cannot
reinstall: --omit=dev reruns prepare without tsc, --ignore-scripts skips the
native builds tree-sitter needs. It carries the compiled node_modules over and
prunes in place instead.
github-actions Bot added a commit that referenced this pull request Aug 25, 2026
github-actions Bot added a commit that referenced this pull request Aug 25, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant