181 lines
18 KiB
Markdown
181 lines
18 KiB
Markdown
# PR + CI/checks status chip via gh
|
||
|
||
A per-project chip in the project-detail view that shows, for the repo's current branch: **PR state** (open / draft / merged / closed / none), **N checks passing** (from `statusCheckRollup`), and **mergeable** (clean / conflicting). It is a read-only, out-of-band side-channel — exactly like `getDiff` (`src/http/diff.ts`): `execFile('gh', …)` (no shell), timeout + `maxBuffer` bound, parses `gh`'s `--json` output, and **capability-degrades** (chip explains itself) when `gh` is missing, unauthenticated, or the branch has no PR. Cached at module scope with a short TTL (reuses `cfg.projectScanTtlMs`) so opening/refreshing a project detail doesn't hammer the GitHub API.
|
||
|
||
Grounding: `buildProjectDetail` (`src/http/projects.ts:420`) surfaces branch/dirty/worktrees but nothing PR. `getDiff`/`runGit` (`src/http/diff.ts:289`) is the runner+degrade pattern to mirror. `isValidGitDir` (`src/server.ts:123`) + the `/projects/diff` route (`src/server.ts:661`) are the exact route mirror. The FE detail header is `renderProjectDetail` (`public/projects.ts:672`, header at lines 696-706); `buildDiffSection` (`public/projects.ts:612`) and `public/diff.ts` `fetchDiff`/`normalizeDiffResult` (lines 110/38) are the FE fetch+degrade+textContent pattern.
|
||
|
||
---
|
||
|
||
## Contract
|
||
|
||
### New route
|
||
|
||
`GET /projects/pr?path=<abs-repo-dir>` — read-only, **no** Origin guard (same threat model as `/projects` and `/projects/diff`; see `src/server.ts:660` comment).
|
||
|
||
- `400 {error}` — `path` missing/empty (mirror `src/server.ts:662-666`).
|
||
- `404 {error:'project not found'}` — `!isValidGitDir(target)` (mirror `src/server.ts:667-670`, SEC-H7 three-prong).
|
||
- `200 PrStatus` — **always** on a valid git dir, including all degrade cases (the availability lives in the body, not the HTTP status — so the FE renders one chip regardless). `500 {error}` only on an unexpected throw (mirror `src/server.ts:674-677`).
|
||
|
||
### New message type — `src/types.ts` (coordination edit, next to `DiffResult` at line 475)
|
||
|
||
```ts
|
||
/* ── W3 PR + CI status chip (gh) ── */
|
||
|
||
/** Why a PrStatus has (or lacks) PR data. Drives the FE chip's degraded text. */
|
||
export type PrAvailability =
|
||
| 'ok' // a PR exists for the current branch; fields below are populated
|
||
| 'no-pr' // gh works but the branch has no PR (or no remote/default repo)
|
||
| 'not-installed' // `gh` binary not found on PATH (ENOENT)
|
||
| 'unauthenticated' // gh present but not logged in (needs `gh auth login`)
|
||
| 'disabled' // GH_ENABLED=0 — feature off, never spawns gh
|
||
| 'error'; // gh spawned but failed for another reason (timeout, etc.)
|
||
|
||
/** Rolled-up CI check counts from gh's statusCheckRollup (CheckRun + StatusContext). */
|
||
export interface PrCheckSummary {
|
||
total: number;
|
||
passing: number; // CheckRun conclusion SUCCESS/NEUTRAL/SKIPPED | StatusContext SUCCESS
|
||
failing: number; // FAILURE/TIMED_OUT/CANCELLED/ACTION_REQUIRED | ERROR/FAILURE
|
||
pending: number; // QUEUED/IN_PROGRESS/WAITING | PENDING/EXPECTED
|
||
}
|
||
|
||
/** GET /projects/pr result. Only present-when-'ok' fields are optional. */
|
||
export interface PrStatus {
|
||
availability: PrAvailability;
|
||
number?: number;
|
||
title?: string;
|
||
url?: string;
|
||
state?: 'open' | 'closed' | 'merged'; // lower-cased from gh OPEN/CLOSED/MERGED
|
||
isDraft?: boolean;
|
||
mergeable?: 'mergeable' | 'conflicting' | 'unknown'; // lower-cased from gh
|
||
headRefName?: string;
|
||
baseRefName?: string;
|
||
checks?: PrCheckSummary;
|
||
}
|
||
```
|
||
|
||
### `gh` invocation (single spawn — KISS)
|
||
|
||
One command; `statusCheckRollup` already carries per-check state, so no second `gh pr checks` spawn:
|
||
|
||
```
|
||
gh pr view --json number,state,title,url,isDraft,mergeable,headRefName,baseRefName,statusCheckRollup
|
||
```
|
||
|
||
Run with `cwd = repoPath`. gh resolves the PR from the current branch. `statusCheckRollup` items are a mix of `{__typename:'CheckRun', status, conclusion}` and `{__typename:'StatusContext', state}` — the pure parser handles both.
|
||
|
||
### New env vars — `src/config.ts` + `Config` in `src/types.ts` (coordination edit)
|
||
|
||
| Env var | Default | Purpose |
|
||
|---|---|---|
|
||
| `GH_ENABLED` | `true` (`parseBool`) | Feature flag. `false` → route returns `{availability:'disabled'}`, never spawns gh. |
|
||
| `GH_TIMEOUT_MS` | `8000` (`parseNonNegativeInt`) | Hard-kill timeout for the gh spawn. Larger than `diffTimeoutMs` (2 s) because gh hits the network. |
|
||
|
||
Cache TTL **reuses** `cfg.projectScanTtlMs` (`src/config.ts:311`, default 10 000 ms) — no new TTL var. Add the two fields to the assembled object in `loadConfig` (`src/config.ts:390-438`) and to the `Config` interface.
|
||
|
||
---
|
||
|
||
## Files to change
|
||
|
||
| Path | Concrete change |
|
||
|---|---|
|
||
| `src/types.ts` | **Coordination edit.** Add `PrAvailability`, `PrCheckSummary`, `PrStatus` near `DiffResult` (line 475). Add `ghEnabled: boolean` + `ghTimeoutMs: number` to the `Config` interface (find `Config` — used by `loadConfig`). |
|
||
| `src/config.ts` | Add `const DEFAULT_GH_TIMEOUT_MS = 8000` near line 65; parse `GH_ENABLED` via `parseBool(env['GH_ENABLED'], true)` and `GH_TIMEOUT_MS` via `parseNonNegativeInt(...)`; add both to the frozen object (lines 413-438). |
|
||
| `src/http/gh.ts` | **New file** (~180 lines), mirrors `diff.ts` structure: a `runGh` runner (`execFileAsync('gh', args, {cwd, timeout, maxBuffer})` that captures `stdout`/`stderr`/`code`/spawn-ENOENT), a **pure** `parsePrView(json): PrStatus`-core + `summarizeChecks(rollup): PrCheckSummary`, a `classifyGhFailure(exec): PrAvailability`, module-scope short-TTL cache mirroring `discoverCache` (`projects.ts:239-317`) with in-flight dedupe, `getPrStatus(repoPath, cfg): Promise<PrStatus>`, and a `_clearPrCache()` test hook (mirror `_clearProjectCache`, `projects.ts:314`). |
|
||
| `src/server.ts` | Add `import { getPrStatus } from './http/gh.js'` (next to line 41). Add `GET /projects/pr` route immediately after `/projects/diff` (after line 678), copying the `path`-missing → 400 and `!isValidGitDir` → 404 guards, then `res.json(await getPrStatus(target, cfg))` in a try/catch → 500 (mirror lines 671-677). |
|
||
| `public/gh-chip.ts` | **New file** (~120 lines), render-only, mirrors `public/diff.ts`: `normalizePrStatus(raw): PrStatus \| null`, `fetchPrStatus(repoPath): Promise<PrStatus \| null>` (mirror `fetchDiff`, `diff.ts:110`), `chipText(status): {label, cls, title}` (pure, unit-tested), `renderPrChip(status): HTMLElement` (all text via **`textContent`**, zero `innerHTML` — SEC-H4), `mountPrChip(container, repoPath): {destroy()}` that shows a loading placeholder then swaps in the resolved chip. |
|
||
| `public/projects.ts` | Add `import { mountPrChip } from './gh-chip.js'` (near line 27). In `renderProjectDetail`, after the dirty indicator (line 704) inside the `if (detail.isGit)` guard, append the chip host and mount it; track the handle in a `prRef` and `destroy()` it in the `back` click handler (lines 683-688) alongside `diffRef.h?.destroy()`. |
|
||
| `public/styles.css` (or the existing project-detail CSS file) | Add `.proj-pr-chip` + state modifier classes (`.proj-pr-open/.draft/.merged/.closed/.none/.unavailable`, `.proj-pr-checks-ok/.fail/.pending`, `.proj-pr-conflict`). Reuse the existing `.proj-branch` chip look (line 699) as the base. |
|
||
| `test/http/gh.test.ts` | **New** (node) — pure parsers + `getPrStatus` classification. |
|
||
| `test/integration/pr-status.test.ts` | **New** (node) — real `startServer`, `gh` stubbed via a PATH shim. |
|
||
| `test/gh-chip.test.ts` | **New** (jsdom) — `normalizePrStatus`/`chipText`/`renderPrChip`/`mountPrChip`. |
|
||
|
||
---
|
||
|
||
## TDD steps
|
||
|
||
Ordered; each "write test → run RED → implement → GREEN". Backend-pure first (cheap, deterministic), then route, then FE. Keeps the 80 % gate because the pure parser + classifier are the bulk of the logic and are fully covered without spawning gh.
|
||
|
||
**Backend — `test/http/gh.test.ts`** (node; mirror `test/http/diff.test.ts:1-33`). Import from `../../src/http/gh.js`.
|
||
|
||
1. `summarizeChecks` — canned `statusCheckRollup` arrays:
|
||
- CheckRun `{status:'COMPLETED',conclusion:'SUCCESS'}` → passing++. Implement `summarizeChecks`.
|
||
- CheckRun `conclusion:'FAILURE'` → failing++; `TIMED_OUT`/`CANCELLED`/`ACTION_REQUIRED` → failing.
|
||
- CheckRun `status:'IN_PROGRESS'`/`'QUEUED'` (null conclusion) → pending.
|
||
- StatusContext `{state:'SUCCESS'}` → passing; `'PENDING'` → pending; `'FAILURE'/'ERROR'` → failing.
|
||
- `NEUTRAL`/`SKIPPED` → passing (don't block). Empty/`undefined` rollup → all-zero. Unknown shape → counted in `total` only, treated as pending. Assert `total === passing+failing+pending`.
|
||
2. `parsePrView` — feed a full canned JSON string:
|
||
- Valid PR JSON → `availability:'ok'`, lower-cased `state`/`mergeable`, `number`/`title`/`url`/`isDraft`/`headRefName`/`baseRefName` mapped, `checks` from `summarizeChecks`. Implement `parsePrView` (never throws — `try/JSON.parse`; malformed → `{availability:'error'}`, mirroring `diff.ts` "never throws" house style).
|
||
- `isDraft:true` still `availability:'ok'` (FE decides the "draft" label); `mergeable:'UNKNOWN'` → `'unknown'`.
|
||
- Malformed / non-object JSON → `{availability:'error'}`.
|
||
- **Security assert**: a PR `title` containing `<script>alert(1)</script>` survives verbatim in `PrStatus.title` (proves no parsing-side mangling; FE renders it inert).
|
||
3. `classifyGhFailure` — given synthetic exec results:
|
||
- spawn ENOENT (`code:'ENOENT'`) → `'not-installed'`.
|
||
- stderr containing `gh auth login` / `not logged` / `authentication` / `HTTP 401` → `'unauthenticated'`.
|
||
- stderr containing `no pull requests found` / `no default remote` / `no git remote` → `'no-pr'`.
|
||
- other non-zero exit → `'error'`. Implement `classifyGhFailure` (regex on lower-cased stderr).
|
||
4. `getPrStatus` cache/dedupe — inject a fake runner (or spy) so no real gh spawns:
|
||
- `ghEnabled:false` in cfg → resolves `{availability:'disabled'}` **without** invoking the runner.
|
||
- Two rapid calls for the same path share one in-flight run (assert runner called once); after `_clearPrCache()`, it runs again. Mirror `projects.ts:289-311`. Implement the module cache + `_clearPrCache`.
|
||
- Cache key includes the current branch (cheap read of `.git/HEAD` like `readBranch`, `projects.ts:88`) so a branch switch busts the cache before TTL. Test: same path, different HEAD branch → runner re-invoked.
|
||
|
||
> To keep `getPrStatus` unit-testable without gh, factor the spawn into an injectable `runGh` (default real, overridable in tests) — same seam idea as `getDiff`'s `runGit`. `parsePrView`/`summarizeChecks`/`classifyGhFailure` stay pure and exported.
|
||
|
||
**Route — `test/integration/pr-status.test.ts`** (node; mirror `test/integration/projects-endpoint.test.ts:1-55`). Use `getFreePort` + a temp dir with a fake `.git` repo (reuse `makeFakeGitRepo` shape). Stub gh with a **PATH shim**: write an executable script named `gh` into a temp `bin/` that echoes canned JSON (or exits 1 with a canned stderr), then set `process.env.PATH = binDir + ':' + process.env.PATH` before `startServer` (execFile resolves `gh` via PATH). Restore PATH + `_clearPrCache()` in `afterEach`.
|
||
|
||
5. Missing `path` → **400**. Implement route guard 1.
|
||
6. Non-git dir path → **404** (`isValidGitDir` fails). Implement guard 2.
|
||
7. gh shim emits valid PR JSON → **200** with `availability:'ok'`, correct `checks`. Wire `res.json(await getPrStatus(...))`.
|
||
8. gh shim exits 1 with `no pull requests found` on stderr → **200 `{availability:'no-pr'}`**.
|
||
9. `GH_ENABLED='0'` env → **200 `{availability:'disabled'}`**, and (assert via a shim that writes a marker file) gh is never spawned.
|
||
|
||
**Frontend — `test/gh-chip.test.ts`** (jsdom; mirror `test/diff.test.ts:1-14`, `// @vitest-environment jsdom`, dynamic `await import('../public/gh-chip.js')`, `vi.stubGlobal('fetch', …)`).
|
||
|
||
10. `normalizePrStatus` — valid object round-trips; non-object / bad `availability` → `null` (mirror `normalizeDiffResult`, `diff.ts:38`). Implement.
|
||
11. `chipText(status)` — pure map: `ok`+open → `"PR #12 ✓ 5/5"`; failing checks → `"PR #12 ✕ 3/5"`; `mergeable:'conflicting'` adds a `⚠ conflicts` marker/class; `no-pr` → `"No PR"`; `not-installed` → `"gh not installed"` + `title` link to cli.github.com; `unauthenticated` → `"gh auth login"`; `disabled` → chip hidden (returns null/`display:none`). Implement.
|
||
12. `renderPrChip` — **SEC-H4 assert**: a `title` of `<img src=x onerror=...>` appears as literal text (`el.textContent` contains it, `el.querySelector('img')` is null). Zero `innerHTML`.
|
||
13. `mountPrChip` — `fetch` stubbed to resolve `ok` JSON → placeholder replaced by the chip; `fetch` rejects → degrades to an `error` chip (no throw); `destroy()` removes the node. Mirror `mountDiffViewer` (`diff.ts:253-`).
|
||
|
||
**Frontend wiring — extend `test/projects.test.ts`** (jsdom; `renderProjectDetail` is already exported/tested there).
|
||
|
||
14. `renderProjectDetail` with `detail.isGit:true` → a `.proj-pr-chip` host is present in the header; with `isGit:false` → absent. (Mock `gh-chip`'s `mountPrChip` via `vi.mock` so the DOM assertion doesn't depend on fetch.) Implement the `renderProjectDetail` edit + `prRef.destroy()` in the back handler.
|
||
|
||
Run `npm test` after each GREEN. The pure backend parser tests (steps 1-3) carry most of the coverage weight for `gh.ts`; FE steps 10-13 cover `gh-chip.ts`.
|
||
|
||
---
|
||
|
||
## Edge cases & failure modes
|
||
|
||
- **`gh` not installed** → spawn ENOENT → `'not-installed'`. Chip shows "gh not installed" (never a 500, never a stack trace).
|
||
- **`gh` present, not authenticated** → stderr auth pattern → `'unauthenticated'` → "gh auth login".
|
||
- **No PR for branch / no remote / detached HEAD** → `'no-pr'` → "No PR". (Detached HEAD: `.git/HEAD` isn't `ref:` → cache-key branch is `null`; gh itself errors → `no-pr`.)
|
||
- **PR exists but checks not started / all pending** → `checks.total>0, passing=0, pending=total` → "⧗ 0/N".
|
||
- **`mergeable:'UNKNOWN'`** (GitHub computes mergeability async right after a push) → `'unknown'` → neutral marker, not a red "conflict". Only `'conflicting'` shows the ⚠.
|
||
- **Draft PR** → `isDraft:true` → "Draft" styling; still `availability:'ok'`.
|
||
- **Merged/closed PR still on the branch** → `state:'merged'/'closed'` badge (gh returns the most recent PR).
|
||
- **gh timeout** (network hang) → `execFileAsync` kills at `ghTimeoutMs` → `'error'` → generic "PR status unavailable". Bounded, never hangs the request.
|
||
- **Huge `statusCheckRollup`** (100s of checks / monorepo) → bounded by `maxBuffer` (reuse `diffMaxBytes`); overflow → `'error'` (don't try to parse a truncated JSON). Counts are aggregate so the chip stays tiny.
|
||
- **Malformed `--json` output** (gh version drift) → `parsePrView` `JSON.parse` throws → caught → `'error'`.
|
||
- **Branch switch within TTL** → cache key includes HEAD branch, so it busts immediately rather than showing the previous branch's PR for up to 10 s.
|
||
- **FE fetch/network error** → `fetchPrStatus` returns `null` → chip renders an `'error'` state, never throws (mirror `fetchDiff`, `diff.ts:117`).
|
||
|
||
---
|
||
|
||
## Security
|
||
|
||
- **No shell**: `execFile('gh', [fixed argv])` — identical guarantee to `runGit` (`diff.ts:296`, SEC-M9). The **only** user-influenced input reaching gh is `cwd`, which is the already-validated `repoPath`. No untrusted string is ever placed in argv (gh derives the PR from the branch; we never pass a branch/base/rev). This sidesteps the `?base=` deferral rationale in `diff.ts:18-20`.
|
||
- **Path containment**: route calls `isValidGitDir(target)` (`server.ts:123`) before spawning — absolute + is-dir + has `.git` (SEC-H7). Path traversal / arbitrary-cwd is blocked exactly as `/projects/diff`.
|
||
- **DoS bounds**: `timeout: ghTimeoutMs` (hard kill) + `maxBuffer: cfg.diffMaxBytes` bound a slow/huge gh. Module-scope TTL cache + in-flight dedupe cap outbound GitHub-API calls to ≈1 per repo per `projectScanTtlMs` even under rapid detail-view refreshes (the panel auto-refreshes every 5 s, `projects.ts:33`).
|
||
- **Network egress note**: unlike every other side-channel (all local), gh talks to GitHub's API using the host's existing `gh`/`GH_TOKEN` credential. The endpoint **never accepts or forwards a token** — it only triggers gh's own auth. Document this in the route comment and in TECH_DOC §7 (this is the first feature to make an outbound call on behalf of a LAN client; the `GH_ENABLED=0` kill-switch lets a cautious operator disable it entirely).
|
||
- **No secret leakage**: never log gh **stdout** (may contain private PR titles) or the token. If logging a failure, log only `availability` + `sanitizeForLog(stderr.slice(0,200))` (reuse `server.ts:162`) — never raw stderr, mirroring the worktree audit line (`server.ts:714`) and SEC-M10 "never raw git stderr".
|
||
- **Origin/CSRF**: GET is read-only and non-mutating → no Origin guard, consistent with `/projects` and `/projects/diff` (`server.ts:660`). It spawns a subprocess but performs no state change, so CSWH/CSRF risk is limited to triggering a cached, rate-bounded read.
|
||
- **XSS**: PR `title` is attacker-controllable (anyone who can open a PR on a repo the host has access to). It is carried verbatim server-side and rendered **only via `textContent`** in `gh-chip.ts` (SEC-H4, same discipline as `diff.ts` line-rendering). Explicit jsdom test (step 12) asserts no element injection.
|
||
- **Rate-limit**: no per-IP limiter needed (read-only, same as `/projects/diff`); the TTL cache is the effective throttle. If desired later, the `createRateLimiter` helper (`server.ts:109`) is available.
|
||
|
||
---
|
||
|
||
## Effort & dependencies
|
||
|
||
- **Effort**: ~1.5–2 days. Backend `gh.ts` + route ≈ 0.75 day (the runner/cache pattern is a near-copy of `diff.ts`/`projects.ts`; the pure `parsePrView`/`summarizeChecks` classifier is the real work). FE `gh-chip.ts` + wiring + CSS ≈ 0.5 day. Tests (3 files) ≈ 0.5 day, with the PATH-shim integration harness the only novel piece.
|
||
- **Depends on**: nothing hard-blocking — `Config`/`src/types.ts`/`config.ts` coordination edits and `isValidGitDir` all exist today. Requires `gh` on the host for the live path, but the feature is designed to degrade cleanly without it (so it ships regardless).
|
||
- **Unlocks**: the **W3 "Quick wins" chip** (task #11 — sync/ahead-behind chip, recent commits) can reuse the same `gh.ts`/`gh-chip.ts` side-channel + short-TTL-cache scaffold (e.g. `gh pr status`, `git rev-list --count @{u}...HEAD`). The PATH-shim gh-integration harness is reusable by any future gh-backed feature.
|
||
- **Interacts with (no conflict)**: **W3 `?base=` diff** (task #9) touches `diff.ts`/`/projects/diff` only; this feature is an independent file (`gh.ts`) and route (`/projects/pr`). Both add a chip/section to the same `renderProjectDetail` header — coordinate the header layout (place the PR chip after the branch chip, before or beside the diff toggle) but they do not edit the same functions. |