@@ -0,0 +1,218 @@ |
| 1 | +# PR checks |
| 2 | + |
| 3 | +S24 ships the data shape, REST API, and PR Checks tab for "check |
| 4 | +runs" — without executing user code. External CI systems |
| 5 | +(Jenkins/Buildkite/etc., or future shithub Actions) post check runs |
| 6 | +via PAT-authenticated `/api/v1/repos/{owner}/{repo}/check-runs`. The |
| 7 | +required-check evaluator composes with the S23 review gate at |
| 8 | +`pulls.Mergeability` and `pulls.Merge`. |
| 9 | + |
| 10 | +## Schema (migration 0025) |
| 11 | + |
| 12 | +``` |
| 13 | +check_suites — one row per (repo_id, head_sha, app_slug). Suites |
| 14 | + group runs by integration. Status + conclusion |
| 15 | + derived from runs (suite_rollup.go). |
| 16 | +check_runs — individual checks (e.g. "lint", "unit-tests"). |
| 17 | + status: queued|in_progress|completed|pending |
| 18 | + conclusion: success|failure|neutral|cancelled| |
| 19 | + skipped|timed_out|action_required|stale |
| 20 | +``` |
| 21 | + |
| 22 | +Plus one column on `branch_protection_rules`: |
| 23 | + |
| 24 | +| Column | Default | Notes | |
| 25 | +| ------------------------------------- | ------- | -------------------------------------------------- | |
| 26 | +| `dismiss_stale_status_checks_on_push` | `false` | Opt-in: mark previous-head suites stale on push | |
| 27 | + |
| 28 | +`status_checks_required text[]` already existed from S20 as a |
| 29 | +placeholder; this sprint activates enforcement. |
| 30 | + |
| 31 | +The CHECK on `check_runs` enforces "completed implies conclusion". |
| 32 | + |
| 33 | +## Suite is derived |
| 34 | + |
| 35 | +API consumers POST runs; the orchestrator finds-or-creates the |
| 36 | +matching suite via `(repo_id, head_sha, app_slug)`. Default |
| 37 | +`app_slug = 'external'`. Future shithub Actions and external integrations |
| 38 | +each get their own slug for grouping. |
| 39 | + |
| 40 | +## Suite rollup |
| 41 | + |
| 42 | +`internal/checks/suite_rollup.go::DeriveSuiteRollup` is the pure |
| 43 | +function: |
| 44 | + |
| 45 | +``` |
| 46 | +status = 'completed' iff every run is completed |
| 47 | + = 'in_progress' if any run has moved past 'queued' |
| 48 | + = 'queued' otherwise |
| 49 | + |
| 50 | +conclusion priority (when status='completed'): |
| 51 | + failure > timed_out > cancelled > action_required > |
| 52 | + success > neutral > skipped > stale |
| 53 | +``` |
| 54 | + |
| 55 | +Re-evaluated inside the same tx as Create/Update so the API response |
| 56 | +reflects the latest state. |
| 57 | + |
| 58 | +## Merge gate composition |
| 59 | + |
| 60 | +The S22 mergeable_state machine is now (priority order): |
| 61 | + |
| 62 | +``` |
| 63 | +dirty — git merge-tree reports conflicts |
| 64 | +behind — head has no commits ahead of base |
| 65 | +blocked — review gate (S23) OR checks gate (S24) unsatisfied |
| 66 | +clean — both gates satisfied |
| 67 | +``` |
| 68 | + |
| 69 | +`pulls.Mergeability` evaluates both gates and ANDs their satisfaction. |
| 70 | +`pulls.Merge`'s belt-and-braces re-evaluation inside the row lock |
| 71 | +also runs both. Either failing produces `ErrMergeBlocked`. |
| 72 | + |
| 73 | +The required-check evaluator is in |
| 74 | +`internal/checks/required_eval.go::EvaluateRequiredChecks`. A run |
| 75 | +"satisfies" iff `status='completed'` AND |
| 76 | +`conclusion ∈ {success, neutral}` on the PR's head SHA. |
| 77 | + |
| 78 | +## API surface |
| 79 | + |
| 80 | +| Method | Path | Scope | |
| 81 | +| ------ | --------------------------------------------------------------- | ------------ | |
| 82 | +| POST | `/api/v1/repos/{owner}/{repo}/check-runs` | `repo:write` | |
| 83 | +| PATCH | `/api/v1/repos/{owner}/{repo}/check-runs/{id}` | `repo:write` | |
| 84 | +| GET | `/api/v1/repos/{owner}/{repo}/commits/{sha}/check-runs` | `repo:read` | |
| 85 | +| GET | `/api/v1/repos/{owner}/{repo}/commits/{sha}/check-suites` | `repo:read` | |
| 86 | + |
| 87 | +Visibility is also gated by `policy.Can` (existence-leak 404 for |
| 88 | +private repos the actor can't see). |
| 89 | + |
| 90 | +### POST request body |
| 91 | + |
| 92 | +```json |
| 93 | +{ |
| 94 | + "name": "lint", |
| 95 | + "head_sha": "<40-char SHA>", |
| 96 | + "app_slug": "external", |
| 97 | + "status": "in_progress", |
| 98 | + "started_at": "2026-05-08T12:00:00Z", |
| 99 | + "details_url": "https://ci.example.com/job/123", |
| 100 | + "output": { |
| 101 | + "title": "lint", |
| 102 | + "summary": "5 warnings", |
| 103 | + "text": "...full log under 256 KiB..." |
| 104 | + }, |
| 105 | + "external_id": "ci-job-42" |
| 106 | +} |
| 107 | +``` |
| 108 | + |
| 109 | +`external_id` makes retries safe: when set, repeating the same POST |
| 110 | +returns the existing run instead of inserting a duplicate. |
| 111 | + |
| 112 | +### PATCH request body |
| 113 | + |
| 114 | +Every field is optional. Omitted fields keep their current value. |
| 115 | +`status='completed'` requires a `conclusion`. Re-applying the same |
| 116 | +status is a no-op. |
| 117 | + |
| 118 | +### Response shape |
| 119 | + |
| 120 | +Mirrors GitHub's check-run shape: `id, suite_id, head_sha, name, |
| 121 | +status, conclusion, started_at, completed_at, details_url, output, |
| 122 | +external_id`. RFC3339 timestamps. Empty `conclusion` omitted. |
| 123 | + |
| 124 | +## Output sizes |
| 125 | + |
| 126 | +Per spec: `output.summary` capped at 64 KiB; `output.text` capped at |
| 127 | +256 KiB. The Create/Update orchestrator returns |
| 128 | +`ErrOutputSummaryTooLarge` / `ErrOutputTextTooLarge` (HTTP 400) on |
| 129 | +overflow. |
| 130 | + |
| 131 | +## PR Checks tab |
| 132 | + |
| 133 | +`internal/web/handlers/repo/pulls.go::pullChecks` loads suites for |
| 134 | +the PR's `head_oid`, then loads runs grouped under each suite. The |
| 135 | +`output.summary` field renders through the existing markdown |
| 136 | +pipeline (Goldmark + bluemonday UGCPolicy). When no checks have |
| 137 | +reported on the head SHA, an empty-state message points at the |
| 138 | +POST endpoint. |
| 139 | + |
| 140 | +## Stale-on-push |
| 141 | + |
| 142 | +Opt-in via `branch_protection_rules.dismiss_stale_status_checks_on_push` |
| 143 | +on the longest-pattern-matching rule. When `true`, `push:process` |
| 144 | +calls `checks.MarkStaleForPreviousHead(repo, before_sha)` after a |
| 145 | +head ref moves. Suites with `status != 'completed'` get |
| 146 | +`(status='completed', conclusion='stale')`. The runs themselves |
| 147 | +stay readable for audit. |
| 148 | + |
| 149 | +The default is `false` to match GitHub. Most CI integrations |
| 150 | +already handle "this push obsoletes my in-flight build" via their |
| 151 | +own retry semantics. |
| 152 | + |
| 153 | +## Branch-protection settings UI |
| 154 | + |
| 155 | +`/{owner}/{repo}/settings/branches` adds two fields: |
| 156 | + |
| 157 | +- **Required status checks** — comma-separated check-run names that |
| 158 | + must succeed on the head SHA before merge. Empty list means no |
| 159 | + requirement. |
| 160 | +- **Mark in-flight checks stale on new push** — checkbox; sets |
| 161 | + `dismiss_stale_status_checks_on_push`. |
| 162 | + |
| 163 | +The protection-rule table now shows the `checks: [name1, name2]` |
| 164 | +list inline so admins can verify their config at a glance. |
| 165 | + |
| 166 | +## Tests |
| 167 | + |
| 168 | +| File | Covers | |
| 169 | +| ------------------------------------------------- | ------------------------------------------------------ | |
| 170 | +| `internal/checks/suite_rollup_test.go` | Pure rollup priority order across 12 cases | |
| 171 | +| `internal/checks/checks_test.go::AutoCreatesSuite` | First run on a (repo, sha, app) creates the suite | |
| 172 | +| `…::IdempotentByExternalID` | Same external_id POST returns same run id | |
| 173 | +| `…::RequiresConclusionWhenCompleted` | `status=completed` without conclusion → 400 | |
| 174 | +| `…::RejectsShortHeadSHA` | Head SHA must be ≥ 7 chars | |
| 175 | +| `…::RejectsTooLargeOutput` | Output text > 256 KiB → 400 | |
| 176 | +| `…::RollsUpSuiteConclusion` | Two completed-success runs → suite success | |
| 177 | +| `…::EvaluateRequiredChecks_NoRequired` | Empty required-list → satisfied | |
| 178 | +| `…::BlocksThenSatisfies` | None → queued → failure → success state machine | |
| 179 | +| `…::StaleOnPush_MarksSuites` | Previous-head suite goes (completed, stale) | |
| 180 | +| `…::TimestampsRoundTrip` | started_at + completed_at round-trip via PATCH | |
| 181 | + |
| 182 | +## Pitfalls handled |
| 183 | + |
| 184 | +- **`merge-tree --merge-base=<base>` semantics** — fixed in S22; not |
| 185 | + re-introduced here. |
| 186 | +- **Required-check name typo** — surfaced inline in the protection- |
| 187 | + rule table so admins notice. Cross-checking against "names seen on |
| 188 | + default branch in last 30d" is a follow-up (see Deferred). |
| 189 | +- **External-system retries** — `external_id` dedupes Create; PATCH |
| 190 | + is idempotent (re-applying the same status is a no-op via the |
| 191 | + status comparison). |
| 192 | +- **Race between check post and head push** — checks key on |
| 193 | + `head_sha`; a check posted for an old SHA correctly does not |
| 194 | + satisfy the new SHA's required-check rule. |
| 195 | +- **Output payload bombs** — capped at 256 KiB / 64 KiB at the |
| 196 | + orchestrator before insert. |
| 197 | +- **TOCTOU on merge** — the row-lock pre-flight in `pulls.Merge` |
| 198 | + re-evaluates both gates so a check failing between Mergeability |
| 199 | + and Merge is honored. |
| 200 | + |
| 201 | +## Deferred |
| 202 | + |
| 203 | +- **Workflow definition / `.shithub/workflows/*.yaml` parsing** → |
| 204 | + Actions sprint (post-MVP). |
| 205 | +- **Runner protocol / runner pool** → Actions sprint (post-MVP). |
| 206 | +- **Check log storage / live log streaming** → post-MVP. |
| 207 | +- **GitHub Statuses API parity** → post-MVP. Spec lean: defer. |
| 208 | +- **Required-check name typo warning** ("name not seen in last 30d |
| 209 | + on default branch") → S36 (perf pass groups UI nudges there). |
| 210 | +- **Webhook events for `check_suite` / `check_run`** → S33 (the |
| 211 | + deliverer drains `webhook_events_pending`; emit points are at |
| 212 | + `internal/checks/{create,update}.go`). |
| 213 | +- **CODEOWNERS-driven required reviewers + checks** → post-MVP. |
| 214 | +- **`require_code_owner_review` enforcement** → post-MVP (column |
| 215 | + exists from S23; CODEOWNERS parser is the gap). |
| 216 | +- **Auto-dismiss stale reviews on push** → post-MVP (column exists |
| 217 | + from S23; symmetric to S24's stale-on-push, but the reviewer-side |
| 218 | + semantics are spec-deferred). |