diff --git a/AGENTS.md b/AGENTS.md index 64d59dfa7..c79a646be 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -22,6 +22,7 @@ This file is the single source of truth for agents entering this repository. Rea - `packages/sidecar-proto` owns the Open Design sidecar business protocol; `packages/sidecar` owns the generic sidecar runtime; `packages/platform` owns generic OS process primitives. - `tools/dev` is the local development lifecycle control plane. - `tools/pack` is the local packaged build/start/stop/logs control plane and mac beta release artifact preparation surface. +- `tools/pr` is the maintainer PR-duty control plane: a thin `gh` wrapper that encodes this repo's review-lane derivation, forbidden-surface flags, lane checklists, and validation-command suggestions. - `e2e` owns user-level end-to-end smoke tests and Playwright UI automation; read `e2e/AGENTS.md` before editing its tests or commands. ## Inactive or placeholder directories @@ -46,8 +47,8 @@ This file is the single source of truth for agents entering this repository. Rea ## Root command boundary -- Keep root scripts reserved for true repo-level checks and tools control-plane entrypoints: `pnpm guard`, `pnpm typecheck`, `pnpm tools-dev`, and `pnpm tools-pack`. -- Do not add root aggregate `pnpm build` or `pnpm test` aliases. Build/test commands must stay package-scoped (`pnpm --filter ...`) or tool-scoped (`pnpm tools-pack ...`). +- Keep root scripts reserved for true repo-level checks and tools control-plane entrypoints: `pnpm guard`, `pnpm typecheck`, `pnpm tools-dev`, `pnpm tools-pack`, and `pnpm tools-pr`. +- Do not add root aggregate `pnpm build` or `pnpm test` aliases. Build/test commands must stay package-scoped (`pnpm --filter ...`) or tool-scoped (`pnpm tools-pack ...` / `pnpm tools-pr ...`). - Do not add root e2e aliases; e2e package commands and ownership rules live in `e2e/AGENTS.md`. ## Boundary constraints @@ -79,6 +80,19 @@ This file is the single source of truth for agents entering this repository. Rea - Blocking review feedback should focus on correctness, security/secrets, data integrity, repository boundary violations, contract/migration breakage, missing required validation, or high-risk maintainability issues. - Only maintainers may close a PR instead of requesting changes, and only when the change is not salvageable on the existing branch (wrong target product, foreign test harness, DOM/API assumptions absent from this repo, or scripts that conflict with lifecycle rules). +## PR-duty tooling + +`pnpm tools-pr` is the maintainer-only control plane for PR-duty work on this repo. It is a thin `gh` wrapper that encodes repo-specific knowledge — review-lane derivation, forbidden-surface flags, per-lane checklists, validation-command suggestions, and a fixed dictionary of factual classify tags (`bot-only-approval`, `needs-rebase`, `stale-approval`, `unresolved-changes-requested`, `awaiting-*` timing, `org-member`, etc.). The tool is read-only on the PR surface: it never approves, merges, comments, or closes; those side effects stay in explicit `gh` invocations the maintainer runs. + +Common subcommands: + +- `pnpm tools-pr list` — triage the open queue by lane and review-state bucket. +- `pnpm tools-pr view ` — factual review brief for a single PR. +- `pnpm tools-pr classify --all` — script-level tag JSON for the whole open queue (entry point for cron / digest consumers); per-PR `classify ` for spot checks. +- `pnpm tools-pr assignment` — assigner-perspective ownership + idle-time / blocker view across the queue. + +For the full tag dictionary, operational playbook (direct merge / duplicate-title / awaiting-author / org-member / agent-review flows), comment templates, language-detection rules, and tool-design constraints (precision boundaries, factual-output rule, retry + pagination strategy), see [`tools/pr/AGENTS.md`](tools/pr/AGENTS.md). + ## Validation strategy - After package, workspace, or command-entry changes, run `pnpm install` so workspace links and generated dist entries stay fresh. @@ -122,6 +136,14 @@ pnpm guard pnpm typecheck ``` +```bash +pnpm tools-pr list +pnpm tools-pr list --bucket=merge-ready,approved-blocked +pnpm tools-pr list --lane=skill,contract --json +pnpm tools-pr view 1180 +pnpm tools-pr view 1180 --json +``` + ```bash pnpm --filter @open-design/web typecheck pnpm --filter @open-design/web test @@ -131,6 +153,7 @@ pnpm --filter @open-design/daemon build pnpm --filter @open-design/desktop build pnpm --filter @open-design/tools-dev build pnpm --filter @open-design/tools-pack build +pnpm --filter @open-design/tools-pr build ``` ```bash diff --git a/apps/web/tests/components/ProjectView.api-empty-response.test.tsx b/apps/web/tests/components/ProjectView.api-empty-response.test.tsx index 1d888dbcf..bc012c09c 100644 --- a/apps/web/tests/components/ProjectView.api-empty-response.test.tsx +++ b/apps/web/tests/components/ProjectView.api-empty-response.test.tsx @@ -171,6 +171,7 @@ function renderProjectView() { config={config} agents={[] as AgentInfo[]} skills={[] as SkillSummary[]} + designTemplates={[] as SkillSummary[]} designSystems={[] as DesignSystemSummary[]} daemonLive onModeChange={vi.fn()} diff --git a/package.json b/package.json index 5c08b298a..dbdbdc7e7 100644 --- a/package.json +++ b/package.json @@ -13,6 +13,7 @@ "postinstall": "node ./scripts/postinstall.mjs", "tools-dev": "pnpm exec tools-dev", "tools-pack": "pnpm exec tools-pack", + "tools-pr": "pnpm exec tools-pr", "guard": "tsx ./scripts/guard.ts", "i18n:check": "tsx ./scripts/i18n-check.ts", "sync:community-pets": "node --experimental-strip-types scripts/sync-community-pets.ts", @@ -24,6 +25,7 @@ "devDependencies": { "@open-design/tools-dev": "workspace:*", "@open-design/tools-pack": "workspace:*", + "@open-design/tools-pr": "workspace:*", "@types/node": "^20.17.10", "tsx": "4.21.0", "typescript": "^5.6.3" diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index c65804286..7f49054ec 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -14,6 +14,9 @@ importers: '@open-design/tools-pack': specifier: workspace:* version: link:tools/pack + '@open-design/tools-pr': + specifier: workspace:* + version: link:tools/pr '@types/node': specifier: ^20.17.10 version: 20.19.39 @@ -377,6 +380,25 @@ importers: specifier: ^2.1.8 version: 2.1.9(@types/node@24.12.2)(jsdom@29.1.1) + tools/pr: + dependencies: + cac: + specifier: 6.7.14 + version: 6.7.14 + devDependencies: + '@types/node': + specifier: 24.12.2 + version: 24.12.2 + esbuild: + specifier: 0.27.7 + version: 0.27.7 + tsx: + specifier: 4.21.0 + version: 4.21.0 + typescript: + specifier: 6.0.3 + version: 6.0.3 + packages: 7zip-bin@5.2.0: diff --git a/scripts/guard.ts b/scripts/guard.ts index e20935783..7323d681d 100644 --- a/scripts/guard.ts +++ b/scripts/guard.ts @@ -59,6 +59,8 @@ const residualAllowedExactPaths = new Set([ "tools/dev/esbuild.config.mjs", "tools/pack/bin/tools-pack.mjs", "tools/pack/esbuild.config.mjs", + "tools/pr/bin/tools-pr.mjs", + "tools/pr/esbuild.config.mjs", "tools/pack/resources/mac/notarize.cjs", // electron-builder hook path; CJS compatibility entry used by tools-pack desktop builds. "tools/pack/resources/web-standalone-after-pack.cjs", @@ -360,6 +362,7 @@ const toolsRootAllowlist = new Map([ ["AGENTS.md", "file"], ["dev", "directory"], ["pack", "directory"], + ["pr", "directory"], ]); async function checkToolsLayout(): Promise { @@ -373,7 +376,7 @@ async function checkToolsLayout(): Promise { const repositoryPath = `tools/${entry.name}${entry.isDirectory() ? "/" : ""}`; if (expected == null) { - violations.push(`${repositoryPath} -> tools/ top-level entries are allowlisted; expected only AGENTS.md, dev/, and pack/`); + violations.push(`${repositoryPath} -> tools/ top-level entries are allowlisted; expected only AGENTS.md, dev/, pack/, and pr/`); continue; } diff --git a/scripts/postinstall.mjs b/scripts/postinstall.mjs index bea87ce9b..ecc1d91f9 100644 --- a/scripts/postinstall.mjs +++ b/scripts/postinstall.mjs @@ -13,6 +13,7 @@ const buildTargets = [ "packages/platform", "tools/dev", "tools/pack", + "tools/pr", ]; const jsExtensions = new Set([".js", ".cjs", ".mjs"]); diff --git a/tools/AGENTS.md b/tools/AGENTS.md index 0b053eefe..605e11962 100644 --- a/tools/AGENTS.md +++ b/tools/AGENTS.md @@ -9,6 +9,7 @@ Follow the root `AGENTS.md` first. This file only records module-level boundarie - `pnpm tools-dev run web` runs foreground daemon + web for the Playwright webServer flow. - `pnpm tools-dev inspect desktop ...` inspects the desktop runtime through sidecar IPC. - `tools/pack` provides `@open-design/tools-pack` and the `tools-pack` bin. The active slice is packaged artifact build/install/start/stop/logs/uninstall/cleanup/list/reset plus beta release artifact preparation for mac and Windows lanes, plus a Linux AppImage lane with optional containerized builds. +- `tools/pr` provides `@open-design/tools-pr` and the `tools-pr` bin. It is the maintainer PR-duty control plane: a thin `gh` wrapper that encodes this repo's review-lane derivation, forbidden-surface flags, per-lane checklists, and validation-command suggestions. It must not perform side effects (approve / request changes / merge / close / push); those stay in explicit `gh` calls the maintainer runs. ## Packaging scope @@ -31,6 +32,8 @@ pnpm --filter @open-design/tools-dev typecheck pnpm --filter @open-design/tools-dev build pnpm --filter @open-design/tools-pack typecheck pnpm --filter @open-design/tools-pack build +pnpm --filter @open-design/tools-pr typecheck +pnpm --filter @open-design/tools-pr build pnpm tools-dev status --json pnpm tools-dev logs --json pnpm tools-dev check @@ -47,4 +50,8 @@ pnpm tools-pack linux install --headless pnpm tools-pack linux start --headless pnpm tools-pack linux stop --headless pnpm tools-pack linux build --containerized +pnpm tools-pr list +pnpm tools-pr list --bucket=merge-ready,approved-blocked +pnpm tools-pr view +pnpm tools-pr view --json ``` diff --git a/tools/pr/AGENTS.md b/tools/pr/AGENTS.md new file mode 100644 index 000000000..a1df746ba --- /dev/null +++ b/tools/pr/AGENTS.md @@ -0,0 +1,214 @@ +# tools/pr + +Follow the root `AGENTS.md` and `tools/AGENTS.md` first. This tool owns the maintainer PR-duty command surface for nexu-io/open-design. + +## Owns + +- Repository-specific triage and review preparation on top of `gh`. +- Lane derivation from touched paths (default / contract / skill / design-system / craft / docs / multi), per `docs/code-review-guidelines.md` §4. +- Forbidden-surface detection on diff paths (e.g. `apps/nextjs/`, `packages/shared/`), per `docs/code-review-guidelines.md` §2. +- Per-lane rule citations matching the hard lines in `docs/code-review-guidelines.md` §4.x and `CONTRIBUTING.zh-CN.md`, with source noted inline. +- Validation command derivation from touched packages, citing `AGENTS.md §Validation strategy`. +- Factual brief assembly: de-noised top files, bot-stripped reviews/comments, CI rollup, body preview. +- Script-level tag emission via `classify`, with each tag carrying a sharp, mechanical rule and a source token (see §Tag dictionary). + +## Does not own + +- `gh` configuration, GitHub credentials, or authentication. +- PR side effects (approve / request changes / merge / close). Side effects stay in `gh` invocations the maintainer runs explicitly. +- Branch checkout, local rebase, or push operations. +- Sidecar protocol, runtime topology, packaged release, or app business logic. + +## Rules + +- **Output is strictly factual.** Every line of human or JSON output must be either (a) data observed from `gh` / the diff / repository paths, or (b) a direct citation of a rule from this repo's own docs (`AGENTS.md`, `docs/code-review-guidelines.md §X`, `CONTRIBUTING.zh-CN.md`, etc.) with the source noted inline. Tools-pr **does not** emit risk verdicts (LOW/MEDIUM/HIGH), merge recommendations, or directive language (`should`, `must`, `do not`, `recommended`, `encouraged`, `suggested`). Judgment belongs to the reviewer who consumes the brief, not to the tool. +- Stay a *thin* wrapper. Each subcommand corresponds to a real PR-duty action; do not introduce abstractions that have no caller. +- Keep dependencies minimal: `cac` for subcommand parsing, no GitHub SDKs. Use `gh` via `node:child_process`. +- `gh pr list` with 12+ JSON fields returns HTTP 502 across this repo's open queue. Chunk `--json` selections into multiple smaller calls and join by PR number (`src/gh.ts:fetchOpenPrs`). +- Heavy chunks (`reviews`, `comments`) use cursor-paginated `gh api graphql` via `fetchPaginatedPrList` with `PR_LIST_PAGE_SIZE = 30`. Light chunks (`meta`, `stats`, `files`) stay on `gh pr list --json` — they're already cheap. The split keeps per-page node count low and lets `gh` retry pages individually when the upstream gateway flakes. +- Transient gateway errors (HTTP 5xx, `EAI_AGAIN`, etc.) trigger a minimum-touch retry inside `gh()` (two attempts at 1s + 2s backoff). Anything else (4xx, auth failure, schema rejection, JSON parse) surfaces immediately — retry must not mask real problems. +- Lane and forbidden-surface rules track `docs/code-review-guidelines.md`. When that document changes, update `src/lane.ts` in the same PR. +- Per-lane rules must point at the hard lines that already exist in the review/contribution docs, with the source cited inline — do not invent new requirements here. +- Output formats are stable contracts: human report is for terminal eyes, `--json` is for downstream agents and future subcommands. Adding/removing JSON fields counts as a breaking change for the JSON consumer surface. + +## Tag dictionary (v1) + +Each tag has a single mechanical rule. Adding new tags requires the rule to be expressible as one factual sentence, derivable purely from `gh` data + file paths, and unlikely to false-positive in legitimate use. Patterns that fail this test (e.g. `missing-test-changes`, `contract-no-consumer-update`, `bulk-author`) are intentionally not implemented — see `feedback_tools_pr_precise_boundaries` in maintainer memory for the exclusion list. + +| Tag | Rule | Data source | +|---|---|---| +| `bot-only-approval` | `reviewDecision === "APPROVED"` and every review with `state === "APPROVED"` matches `isBotAuthored()` | gh.reviewDecision + latestReviews | +| `needs-rebase` | `mergeStateStatus ∈ {DIRTY, BEHIND}` | gh.mergeStateStatus | +| `forbidden-surface` | A touched path matches the regex set in `lane.ts:deriveForbidden` (AGENTS.md §Forbidden surfaces) | files + lane.deriveForbidden | +| `unlabeled` | The PR is missing at least one of the `size/`, `risk/`, `type/` label prefixes | gh.labels | +| `duplicate-title` | Another open PR by the same author has a byte-for-byte identical `title` | cross-PR titleIndexByAuthor | +| `non-ascii-slug` | A design-system root touched by the PR has a slug that fails `/^[a-z0-9-]+$/` | files + lane.DESIGN_DIR | +| `maintainer-edits-disabled` | `maintainerCanModify === false` | gh.maintainerCanModify | +| `org-member` | PR author's GitHub login appears in `gh api orgs//members` | gh REST orgs members list | +| `unresolved-changes-requested` | Any reviewer's latest review has `state === "CHANGES_REQUESTED"` | gh.latestReviews[].state | +| `stale-approval` | Any `APPROVED` review's `commit.oid` differs from current `headRefOid` | gh.latestReviews[].commit.oid + gh.headRefOid | +| `awaiting-author-response-24h` | Latest human-reviewer signal time is newer than the latest author signal time and is ≥ 24h ago | latestReviews + comments + commits | +| `awaiting-reviewer-response-24h` | Latest author signal time is newer than the latest human-reviewer signal time, ≥ 24h ago, and at least one human-reviewer signal exists | latestReviews + comments + commits | +| `awaiting-first-review-24h` | No human review or non-author non-bot comment exists, and `createdAt` is ≥ 24h ago | latestReviews + comments + createdAt | + +**Signal-time definitions** (used by the three `awaiting-*` tags): + +- *author signal* = `max(commits[].committedDate) ∪ max(comments[?author.login==prAuthor].createdAt)` +- *human-reviewer signal* = `max(latestReviews[?author!=prAuthor && !isBotAuthored].submittedAt) ∪ max(comments[?author!=prAuthor && !isBotAuthored].createdAt)` + +The three `awaiting-*` tags are mutually exclusive by construction. Each of them also sets `tag.awaitingHours` — the integer hour count between the awaiting-window start (latest reviewer signal / latest author signal / `createdAt` respectively) and the classify-run moment. Downstream consumers use it to sort PRs within an awaiting bucket by actual stuck duration, or floor-divide by 24 for days. + +### Rate-limit telemetry + +`classify --all` records a `rate` object in the report and in the summary line so detector additions that quietly inflate API cost are visible immediately: + +- `rate.before` / `rate.after`: GraphQL `rateLimit { remaining, limit, resetAt }` snapshots taken before and after the bulk fetch. +- `rate.cost`: `before.remaining − after.remaining` when both snapshots fall in the same reset window; `null` when the hourly window rolls over between snapshots. + +Each snapshot itself costs 1 point; the two extra snapshot calls per `--all` run are negligible against the 5000-point hourly budget. + +## Templates + +`tools/pr/templates/*.md` holds **aesthetic references** for the recurring comment kinds surfaced by classify tags. Each file describes the beats the comment should hit and shows one exemplar phrasing in the maintainer's voice. + +**Templates are not fill-in forms.** Do not `sed`-substitute placeholders and post the rendered text — repeated identical comments break the human-to-human tone we want to keep with contributors. Instead, for each post: read the template to absorb the tone, weave the PR-specific facts (author, awaiting duration, branch names, diff size) into a fresh comment that hits the same beats with locally adapted wording. + +**Author-addressed comments adapt to the author's language.** For nudge / duplicate-ask / close-with-reason comments — i.e. anything @-mentioning a specific contributor in a private-feeling exchange — detect the author's preferred language before writing the comment: + +```bash +gh pr view --json body,comments,author --jq ' + .author.login as $a | + ([.body, (.comments[] | select(.author.login == $a) | .body)] | join("\n")) +' +``` + +If the resulting text contains CJK characters (`grep -P "[\\p{Han}]"`), write the comment in Chinese; otherwise English. Broadcasting comments (PR descriptions, commit messages, review summaries visible to all reviewers) stay in English regardless. See maintainer memory `feedback_public_artifacts_english` for the full scope rule. + +The frontmatter of each template lists the beats and the placeholder slots. Templates are intentionally text files maintained alongside the tool source, not generated by `tools-pr` — the tool itself stays side-effect-free. + +Current templates: + +| Template | Triggered by | Posted on | +|---|---|---| +| `duplicate-title-ask.md` | `duplicate-title` tag (same author, byte-for-byte identical title) | The older / more-iterated PR of the pair | +| `awaiting-author-nudge.md` | `awaiting-author-response-24h` tag, when `tag.awaitingHours ≥ 96` (≥ 4 days) | The PR; addressed to the author | +| `agent-review.md` | Bucket-3 high-value / high-risk technical PR review prep | Not posted; internal analysis artifact at `.tmp/tools-pr/reviews/.md` | + +## Operational playbook + +Per classify tag bucket, the maintainer workflow. Each row is the minimum action — escalation (close, force-merge, etc.) is the maintainer's call. + +### Direct merge (APPROVED + CLEAN, surgical) + +1. Sanity-check the merge state: + + ```bash + gh pr view --json state,reviewDecision,mergeStateStatus,statusCheckRollup \ + --jq '{state, reviewDecision, mergeStateStatus, + checks: [.statusCheckRollup[] | {conclusion, name: (.workflowName // .name)}]}' + ``` + + Expect `state=OPEN`, `reviewDecision=APPROVED`, `mergeStateStatus=CLEAN`, every check `SUCCESS`. + +2. If `tools-pr classify ` includes `bot-only-approval`, verify the change is surgical (size/XS, single file, < ~30 lines, no boundary or contract surface) before proceeding. Judgment lives in maintainer memory (`feedback_bot_only_approval`), not in the tool. + +3. Squash-merge per repo convention: `gh pr merge --squash`. + +4. Confirm: `gh pr view --json state,mergedAt,mergeCommit --jq '{state, mergedAt, sha: .mergeCommit.oid[0:10]}'`. + +### `duplicate-title` + +1. Inspect both PRs to pick the older / more-iterated one (the author may want to preserve its history). Useful comparison: + + ```bash + gh pr view --json number,headRefName,commits,additions,deletions,createdAt,updatedAt + ``` + +2. Read `templates/duplicate-title-ask.md` for tone and beat structure, then write a fresh comment that hits the same beats with the actual PR facts woven in (author login, both branch names, both commit counts, both diff sizes). Post to the older PR: + + ```bash + # Write the composed comment to a scratch file, then: + gh pr comment -F /tmp/dup-ask-.md + ``` + +3. Wait for author response. If no response after 7d, close the older PR with `gh pr close --comment "Superseded by #."`. + +### `awaiting-author-response-24h` (long tail, ≥ 4 days) + +1. Filter the classify report for the PRs that crossed the 96h threshold, **exclude `org-member` PRs**, and rank by `awaitingHours`: + + ```bash + jq '[.byTag["awaiting-author-response-24h"][] as $n + | .byNumber[($n|tostring)] as $tags + | select($tags | map(.name) | contains(["org-member"]) | not) + | $tags[] + | select(.name=="awaiting-author-response-24h" and .awaitingHours >= 96) + | {n: $n, h: .awaitingHours}] + | sort_by(-.h)' .tmp/tools-pr/classify/.json + ``` + +2. For each remaining PR, read `templates/awaiting-author-nudge.md` for tone, then compose a fresh comment that hits the same beats with the actual author login and human-formatted awaiting duration. Vary the wording slightly between PRs nudged in the same session (a contributor seeing identical pings across their notifications breaks the friendly-human feel). + +3. Post: `gh pr comment -F /tmp/nudge-.md`. + +4. Re-check the classify report in a follow-up run; the awaiting tag should clear once the author responds. If no response by 14d, escalate (a more direct stale-warning or close-after-warning). + +### `org-member` + +`org-member` is informational and pairs with the other operational tags rather than triggering its own GitHub action. When a PR carries `org-member` alongside `awaiting-*` / `duplicate-title` / `maintainer-edits-disabled` / etc., the run-of-the-mill GitHub-comment workflow does **not** apply — those communications are routed through the team's internal IM instead. See maintainer memory `feedback_org_members_im_channel` for the channel split (operational nudges → IM; substantive review feedback and decisions → GitHub). + +Every operational playbook step that posts a public comment must filter `org-member` out first; the `awaiting-author-response-24h` flow above shows the canonical filter. + +### `tools-pr assignment` — assigner-perspective ownership view + +Read-only aggregation that pivots the open PR queue by assignee. For each currently-assigned PR per assignee, surfaces: + +- **assigned-since**: `now − assignedAt`, where `assignedAt` is the latest `ASSIGNED_EVENT` for that assignee that has not been superseded by an `UNASSIGNED_EVENT` (fetched via `gh api graphql` timeline — only path that exposes it). +- **assigned-by**: the actor on that event. Marked `(self-assigned)` when actor == assignee. +- **idle-for**: `now − max(assignedAt, last assignee activity)`, where activity = commit / comment / review by that assignee. +- **state badges**: `reviewDecision`, `mergeStateStatus`, `draft` (only when non-trivial). +- **status / blockers**: composed from existing classify tags (`needs-rebase`, `unresolved-changes-requested`, `stale-approval`, `awaiting-*`, `bot-only-approval`). No new judgments. + +Flow: + +1. `gh issue/pr edit --add-assignee ` is still the assignment action (tools-pr is read-only on this surface). +2. `pnpm tools-pr assignment` shows the resulting state grouped by assignee, sorted by idle-hours desc within each bucket. +3. Use `--user me` (or `--user `) for one bucket, `--unassigned` to expand the un-owned tail. +4. `--json` for cron / digest consumption. + +Timeline fetch uses the cursor-paginated graphql path (`fetchPaginatedPrList`) — same retry + page-size primitives as reviews/comments. + +### Agent review (bucket-3 high-value / high-risk PRs) + +For PRs that warrant a deep technical pre-review (contract lane PRs, large refactors, security-sensitive fixes, scope-mixed PRs flagged via classify, etc.), an agent produces an analysis brief under `.tmp/tools-pr/reviews/.md`. The brief is an internal artifact for the maintainer's consumption — not a GitHub comment. If maintainer decides to post review feedback, a separate downstream step adapts findings to public-facing review text (typically rephrased to address the author directly, with the channel respecting `org-member` routing). + +Flow: + +1. Pull `tools-pr view ` for the structural brief and `gh pr diff ` for the patch. +2. Read `templates/agent-review.md` for tone + section pool, then compose for the specific PR (sections appear only when they carry signal — see `feedback_agent_review_shape`). +3. Write to `.tmp/tools-pr/reviews/.md` (transient runtime artifact; this directory is not version-controlled and can be cleaned at any time). +4. Surface the brief to the maintainer; let them decide split / block / merge / IM / public-review. + +Frozen-in-time exemplars covering three PR shapes live in `tools/pr/templates/examples/` (scope-expanded, clean contract feature, CHANGES_REQUESTED with prior human reviews) and are the canonical references for the style. + +## Common commands + +```bash +pnpm --filter @open-design/tools-pr typecheck +pnpm --filter @open-design/tools-pr build +pnpm tools-pr list +pnpm tools-pr list --bucket=merge-ready,approved-blocked +pnpm tools-pr list --lane=skill,contract +pnpm tools-pr list --author=xxiaoxiong --json +pnpm tools-pr view 1180 +pnpm tools-pr view 1180 --json +pnpm tools-pr classify 1167 # single PR, stdout +pnpm tools-pr classify 1167 --json # single PR, JSON stdout +pnpm tools-pr classify --all # full queue, JSON file → .tmp/tools-pr/classify/.json +pnpm tools-pr classify --all --name daily # override filename stem +pnpm tools-pr classify --all --print # also dump JSON to stdout +pnpm tools-pr assignment # assigner-perspective queue view +pnpm tools-pr assignment --user me # only my bucket +pnpm tools-pr assignment --unassigned # expand the un-owned tail +pnpm tools-pr assignment --json # JSON for cron / digest +``` diff --git a/tools/pr/bin/tools-pr.mjs b/tools/pr/bin/tools-pr.mjs new file mode 100755 index 000000000..2061c4cf7 --- /dev/null +++ b/tools/pr/bin/tools-pr.mjs @@ -0,0 +1,18 @@ +#!/usr/bin/env node + +import { existsSync } from 'node:fs'; +import { dirname, resolve } from 'node:path'; +import { fileURLToPath, pathToFileURL } from 'node:url'; + +const entryDir = dirname(fileURLToPath(import.meta.url)); +const distEntry = resolve(entryDir, '../dist/index.mjs'); +const requiredDistEntries = [distEntry]; +const missingDistEntries = requiredDistEntries.filter((entry) => !existsSync(entry)); + +if (missingDistEntries.length > 0) { + throw new Error( + `tools-pr dist entries not found: ${missingDistEntries.join(', ')}. Run "pnpm --filter @open-design/tools-pr build" first.`, + ); +} + +await import(pathToFileURL(distEntry).href); diff --git a/tools/pr/esbuild.config.mjs b/tools/pr/esbuild.config.mjs new file mode 100644 index 000000000..8992ac9a5 --- /dev/null +++ b/tools/pr/esbuild.config.mjs @@ -0,0 +1,18 @@ +import { build } from "esbuild"; + +await build({ + banner: { + js: "#!/usr/bin/env node", + }, + bundle: true, + entryNames: "[name]", + entryPoints: ["./src/index.ts"], + format: "esm", + outdir: "./dist", + outExtension: { + ".js": ".mjs", + }, + packages: "external", + platform: "node", + target: "node24", +}); diff --git a/tools/pr/package.json b/tools/pr/package.json new file mode 100644 index 000000000..426a827fb --- /dev/null +++ b/tools/pr/package.json @@ -0,0 +1,27 @@ +{ + "name": "@open-design/tools-pr", + "version": "0.6.0", + "private": true, + "type": "module", + "bin": { + "tools-pr": "./bin/tools-pr.mjs" + }, + "scripts": { + "build": "node ./esbuild.config.mjs", + "dev": "tsx ./src/index.ts", + "test": "node --import tsx --test tests/*.test.ts", + "typecheck": "tsc -p tsconfig.json --noEmit" + }, + "dependencies": { + "cac": "6.7.14" + }, + "devDependencies": { + "@types/node": "24.12.2", + "esbuild": "0.27.7", + "tsx": "4.21.0", + "typescript": "6.0.3" + }, + "engines": { + "node": "~24" + } +} diff --git a/tools/pr/src/assignment.ts b/tools/pr/src/assignment.ts new file mode 100644 index 000000000..9c03bf5e7 --- /dev/null +++ b/tools/pr/src/assignment.ts @@ -0,0 +1,461 @@ +/** + * `tools-pr assignment` — assigner-perspective view of PR ownership. + * + * For each currently-assigned PR, surfaces who owns it, when they were + * assigned (and by whom), how long it has been idle, and the assignment's + * progress / blocker status as derived from the existing classify tag set. + * + * Read-only aggregation: no new tags, no new judgments. Status strings + * compose existing tag facts (`needs-rebase`, `awaiting-*`, `bot-only- + * approval`, etc.) — see `tools/pr/AGENTS.md` §Tag dictionary. + */ + +import { reduceLatestReviewsByAuthor } from "./bot.js"; +import { + detectRepoSlug, + fetchCurrentUser, + fetchOpenPrAssignmentTimelines, + fetchOpenPrs, + fetchOrgMembers, +} from "./gh.js"; +import { buildContext, classifyPr } from "./tags.js"; +import type { + ClassifyContext, + GhAssignmentEvent, + GhAssignmentTimeline, + PrFacts, + Tag, +} from "./types.js"; + +// ---- per-assignment derived shape --------------------------------------- + +type AssignmentEntry = { + number: number; + title: string; + assignee: string; + assignedAt: string | null; + assignedBy: string | null; + selfAssigned: boolean; + /** Hours since the assignment record (null if no event in fetched window). */ + assignedHoursAgo: number | null; + /** Most recent moment the assignee took action on the PR (commit/comment/review), + * bounded below by assignedAt. Null when assignedAt is also null and there's + * no recorded activity. */ + idleSinceAt: string | null; + idleHours: number | null; + /** Compact state badges (REVIEW_REQUIRED, CHANGES_REQUESTED, DIRTY, etc.). */ + stateBadges: string[]; + /** Status line — one of "ready to merge", "blocked: ...", "in review". */ + status: string; + /** Blocker bullet lines drawn from the tag set. Empty for non-blocked PRs. */ + blockers: string[]; + /** Raw tags present on the PR for downstream JSON consumption. */ + tags: Tag[]; + isDraft: boolean; +}; + +type AssignmentReport = { + generatedAt: string; + openPrTotal: number; + assignedCount: number; + unassignedCount: number; + byAssignee: Record; + unassigned: Array<{ + number: number; + title: string; + stateBadges: string[]; + status: string; + blockers: string[]; + isDraft: boolean; + }>; +}; + +// ---- derivation helpers ------------------------------------------------- + +const HOUR_MS = 60 * 60 * 1000; + +function hoursBetween(fromIso: string, now: number): number { + return Math.floor((now - Date.parse(fromIso)) / HOUR_MS); +} + +function formatDuration(hours: number | null): string { + if (hours === null) return "(unknown)"; + if (hours < 24) return `${hours}h`; + const days = Math.floor(hours / 24); + const rem = hours - days * 24; + return rem === 0 ? `${days}d` : `${days}d ${rem}h`; +} + +function truncate(s: string, n: number): string { + return s.length <= n ? s : `${s.slice(0, n - 1)}…`; +} + +/** + * Walks the timeline events chronologically and tracks the latest + * ASSIGNED_EVENT per login that has not been superseded by an + * UNASSIGNED_EVENT for that login. Returns a map keyed by assignee login. + */ +function indexAssignmentEvents( + events: ReadonlyArray, +): Map { + const sorted = [...events].sort((a, b) => a.createdAt.localeCompare(b.createdAt)); + const map = new Map(); + for (const event of sorted) { + const login = event.assignee?.login; + if (!login) continue; + if (event.kind === "ASSIGNED") { + map.set(login, event); + } else { + map.delete(login); + } + } + return map; +} + +function lastAssigneeActivityAt(facts: PrFacts, assignee: string): string | null { + let latest: number | null = null; + const consider = (iso: string) => { + const t = Date.parse(iso); + if (Number.isFinite(t) && (latest === null || t > latest)) latest = t; + }; + for (const c of facts.commits) { + if (c.authorLogin === assignee) consider(c.committedDate); + } + for (const cmt of facts.comments) { + if (cmt.author?.login === assignee) consider(cmt.createdAt); + } + for (const r of facts.reviews) { + if (r.author?.login === assignee) consider(r.submittedAt); + } + return latest === null ? null : new Date(latest).toISOString(); +} + +function deriveStateBadges(facts: PrFacts): string[] { + const badges: string[] = []; + badges.push(facts.reviewDecision || "REVIEW_REQUIRED"); + if (facts.mergeStateStatus !== "CLEAN" && facts.mergeStateStatus !== "UNKNOWN") { + badges.push(facts.mergeStateStatus); + } + if (facts.isDraft) badges.push("draft"); + return badges; +} + +/** + * Translates the existing tag set + PR metadata into a status line + blocker + * bullets. Status precedence (when multiple apply): blockers → bot-only- + * approval → ready-to-merge → in review. + * + * `ready to merge` requires `reviewDecision === "APPROVED"` and + * `mergeStateStatus ∈ {CLEAN, UNSTABLE}` (UNSTABLE = mergeable but a non- + * required check is failing — still actionable for the maintainer). Without + * this branch, an APPROVED + CLEAN PR with a human reviewer renders the + * same as a REVIEW_REQUIRED one, which drops the main triage signal. + * + * Each blocker bullet is a one-liner that carries the tag's `reason` + * verbatim — no judgment language, no priority labels. The assigner can + * scan the bullets and decide. + */ +function deriveStatus( + tags: ReadonlyArray, + facts: PrFacts, +): { status: string; blockers: string[] } { + const byName = new Map(tags.map((t) => [t.name, t] as const)); + const get = (name: string) => byName.get(name); + + const blockers: string[] = []; + if (get("needs-rebase")) blockers.push("needs-rebase (main has moved)"); + const unresolved = get("unresolved-changes-requested"); + if (unresolved) blockers.push(unresolved.reason); + const stale = get("stale-approval"); + if (stale) blockers.push(stale.reason); + const awaitAuthor = get("awaiting-author-response-24h"); + if (awaitAuthor) blockers.push(`awaiting author for ${formatDuration(awaitAuthor.awaitingHours ?? null)}`); + const awaitReviewer = get("awaiting-reviewer-response-24h"); + if (awaitReviewer) blockers.push(`awaiting reviewer for ${formatDuration(awaitReviewer.awaitingHours ?? null)}`); + const awaitFirst = get("awaiting-first-review-24h"); + if (awaitFirst) blockers.push(`no human review yet (${formatDuration(awaitFirst.awaitingHours ?? null)} since createdAt)`); + + const mergeReadyState = + facts.reviewDecision === "APPROVED" && + (facts.mergeStateStatus === "CLEAN" || facts.mergeStateStatus === "UNSTABLE"); + + let status: string; + if (blockers.length > 0) { + status = "blocked"; + } else if (get("bot-only-approval")) { + status = "approved (bot-only — no human formal sign-off)"; + } else if (mergeReadyState) { + status = "ready to merge"; + } else { + status = "in review"; + } + return { status, blockers }; +} + +function buildAssignmentEntries( + facts: PrFacts, + events: ReadonlyArray, + tags: ReadonlyArray, + now: number, +): AssignmentEntry[] { + const eventIndex = indexAssignmentEvents(events); + const stateBadges = deriveStateBadges(facts); + const { status, blockers } = deriveStatus(tags, facts); + + return facts.assignees.map((assignee) => { + const event = eventIndex.get(assignee) ?? null; + const assignedAt = event?.createdAt ?? null; + const assignedBy = event?.actor?.login ?? null; + const selfAssigned = assignedBy !== null && assignedBy === assignee; + const assignedHoursAgo = assignedAt === null ? null : hoursBetween(assignedAt, now); + + const lastActivity = lastAssigneeActivityAt(facts, assignee); + let idleSinceAt: string | null = null; + if (assignedAt !== null && lastActivity !== null) { + idleSinceAt = + Date.parse(assignedAt) > Date.parse(lastActivity) ? assignedAt : lastActivity; + } else if (assignedAt !== null) { + idleSinceAt = assignedAt; + } else if (lastActivity !== null) { + idleSinceAt = lastActivity; + } + const idleHours = idleSinceAt === null ? null : hoursBetween(idleSinceAt, now); + + return { + number: facts.number, + title: facts.title, + assignee, + assignedAt, + assignedBy, + selfAssigned, + assignedHoursAgo, + idleSinceAt, + idleHours, + stateBadges, + status, + blockers, + tags: [...tags], + isDraft: facts.isDraft, + }; + }); +} + +// ---- report assembly ---------------------------------------------------- + +function buildReport( + allFacts: PrFacts[], + timelines: Map>, + ctx: ClassifyContext, +): AssignmentReport { + const now = Date.now(); + const byAssignee: Record = {}; + const unassigned: AssignmentReport["unassigned"] = []; + let assignedCount = 0; + + for (const facts of allFacts) { + const events = timelines.get(facts.number) ?? []; + const tags = classifyPr(facts, ctx); + if (facts.assignees.length === 0) { + const { status, blockers } = deriveStatus(tags, facts); + unassigned.push({ + number: facts.number, + title: facts.title, + stateBadges: deriveStateBadges(facts), + status, + blockers, + isDraft: facts.isDraft, + }); + continue; + } + assignedCount += 1; + const entries = buildAssignmentEntries(facts, events, tags, now); + for (const entry of entries) { + const bucket = byAssignee[entry.assignee] ?? []; + bucket.push(entry); + byAssignee[entry.assignee] = bucket; + } + } + + // Sort each assignee bucket by idle desc (most stale at top) + for (const login of Object.keys(byAssignee)) { + byAssignee[login]?.sort((a, b) => (b.idleHours ?? -1) - (a.idleHours ?? -1)); + } + + return { + generatedAt: new Date().toISOString(), + openPrTotal: allFacts.length, + assignedCount, + unassignedCount: unassigned.length, + byAssignee, + unassigned, + }; +} + +// ---- formatting --------------------------------------------------------- + +function formatHumanReport( + report: AssignmentReport, + options: { meLogin: string | null; showUnassignedDetail: boolean; userFilter: string | null }, +): string { + const lines: string[] = []; + const header = `PR assignment overview — ${report.openPrTotal} open PRs · ${report.assignedCount} assigned · ${report.unassignedCount} unassigned`; + lines.push(header); + lines.push(""); + + const buckets = Object.entries(report.byAssignee).sort((a, b) => { + // me first, then by PR count desc, then alphabetic + if (options.meLogin && a[0] === options.meLogin) return -1; + if (options.meLogin && b[0] === options.meLogin) return 1; + if (b[1].length !== a[1].length) return b[1].length - a[1].length; + return a[0].localeCompare(b[0]); + }); + + for (const [login, entries] of buckets) { + if (options.userFilter !== null && login !== options.userFilter) continue; + const youTag = options.meLogin && login === options.meLogin ? " (you)" : ""; + lines.push(`▌ ${login}${youTag} · ${entries.length} PR${entries.length === 1 ? "" : "s"}`); + lines.push(""); + for (const entry of entries) { + const num = String(entry.number).padStart(5, " "); + const title = truncate(entry.title, 64); + const draftSuffix = entry.isDraft ? " [draft]" : ""; + lines.push(` #${num} ${title}${draftSuffix}`); + const assignedAgo = entry.assignedHoursAgo === null + ? "assignment > 20 timeline events ago (unknown exact time)" + : `assigned ${formatDuration(entry.assignedHoursAgo)} ago`; + const by = entry.assignedBy + ? entry.selfAssigned + ? " (self-assigned)" + : ` by ${entry.assignedBy}` + : ""; + const idle = entry.idleHours === null ? "" : ` · idle ${formatDuration(entry.idleHours)}`; + lines.push(` ${assignedAgo}${by}${idle}`); + lines.push(` state: ${entry.stateBadges.join(" · ")}`); + lines.push(` status: ${entry.status}`); + for (const blocker of entry.blockers) lines.push(` - ${blocker}`); + lines.push(""); + } + } + + if (options.userFilter === null) { + if (options.showUnassignedDetail) { + lines.push(`▌ (unassigned) · ${report.unassigned.length} PRs`); + lines.push(""); + for (const entry of report.unassigned) { + const num = String(entry.number).padStart(5, " "); + const title = truncate(entry.title, 64); + const draftSuffix = entry.isDraft ? " [draft]" : ""; + lines.push(` #${num} ${title}${draftSuffix}`); + lines.push(` state: ${entry.stateBadges.join(" · ")} status: ${entry.status}`); + for (const blocker of entry.blockers) lines.push(` - ${blocker}`); + } + lines.push(""); + } else { + lines.push(`▌ (unassigned) · ${report.unassignedCount} PRs`); + lines.push(" → see \`tools-pr assignment --unassigned\` for the full list"); + lines.push(""); + } + } + + return lines.join("\n"); +} + +// ---- command entry ------------------------------------------------------ + +export type AssignmentOptions = { + json?: boolean; + user?: string; + unassigned?: boolean; + includeDrafts?: boolean; + limit?: number | string; +}; + +export async function runAssignment(options: AssignmentOptions): Promise { + // Default covers the whole open queue with growth headroom; `gh pr list` + // paginates internally so a high cap is cheap. + const limit = Math.max(1, Number(options.limit ?? 1000) || 1000); + const { owner } = await detectRepoSlug(); + const userFilterRaw = options.user; + const meLoginPromise = userFilterRaw === "me" ? fetchCurrentUser() : Promise.resolve(null); + const [fetched, orgMembers, timelines, meLogin] = await Promise.all([ + fetchOpenPrs(limit, { includeCommits: true, includeComments: true }), + fetchOrgMembers(owner), + fetchOpenPrAssignmentTimelines(), + meLoginPromise, + ]); + + // Reuse classify's fact-building. Inline here to avoid a circular import. + const statsBy = new Map(fetched.stats.map((row) => [row.number, row] as const)); + const filesBy = new Map(fetched.files.map((row) => [row.number, row] as const)); + const reviewsBy = new Map(fetched.reviews.map((row) => [row.number, row] as const)); + const commitsBy = new Map((fetched.commits ?? []).map((row) => [row.number, row] as const)); + const commentsBy = new Map((fetched.comments ?? []).map((row) => [row.number, row] as const)); + + const allFacts: PrFacts[] = fetched.meta + .filter((meta) => options.includeDrafts || !meta.isDraft) + .map((meta) => { + const stats = statsBy.get(meta.number); + const filesRow = filesBy.get(meta.number); + const reviewsRow = reviewsBy.get(meta.number); + const commitsRow = commitsBy.get(meta.number); + const commentsRow = commentsBy.get(meta.number); + return { + number: meta.number, + author: meta.author.login, + title: meta.title, + createdAt: meta.createdAt, + updatedAt: meta.updatedAt, + isDraft: meta.isDraft, + reviewDecision: meta.reviewDecision, + mergeStateStatus: stats?.mergeStateStatus ?? "UNKNOWN", + maintainerCanModify: meta.maintainerCanModify, + isOrgMember: orgMembers.has(meta.author.login), + headRefOid: stats?.headRefOid ?? "", + assignees: meta.assignees.map((a) => a.login), + labels: meta.labels, + filePaths: filesRow ? filesRow.files.map((f) => f.path) : [], + reviews: reviewsRow + ? reduceLatestReviewsByAuthor(reviewsRow.reviews).map((r) => ({ + author: r.author, + body: r.body, + state: r.state, + submittedAt: r.submittedAt, + commit: r.commit ?? null, + })) + : [], + comments: commentsRow ? commentsRow.comments : [], + commits: commitsRow + ? commitsRow.commits.map((c) => ({ + committedDate: c.committedDate, + authorLogin: c.authors[0]?.login ?? null, + })) + : [], + }; + }); + const ctx = buildContext(allFacts); + + const timelineMap = new Map>(); + for (const t of timelines) timelineMap.set(t.number, t.events); + + const report = buildReport(allFacts, timelineMap, ctx); + + if (options.json) { + process.stdout.write(`${JSON.stringify(report, null, 2)}\n`); + return; + } + + // Resolve user filter for display: "me" expands to current login. + let userFilter: string | null = null; + if (userFilterRaw !== undefined) { + userFilter = userFilterRaw === "me" ? meLogin : userFilterRaw; + } + + process.stdout.write( + `${formatHumanReport(report, { + meLogin: meLogin, + showUnassignedDetail: Boolean(options.unassigned), + userFilter, + })}\n`, + ); +} + diff --git a/tools/pr/src/bot.ts b/tools/pr/src/bot.ts new file mode 100644 index 000000000..4decac8d4 --- /dev/null +++ b/tools/pr/src/bot.ts @@ -0,0 +1,78 @@ +/** + * Bot detection for review / comment bodies. + * + * Some maintainer accounts in this repo (e.g. mrcfps) operate partly as a + * Looper auto-review bot — the user account is a real MEMBER but the review + * body is generated by Looper and carries an HTML-comment stamp. We need to + * surface that distinction because GitHub's `reviewDecision === "APPROVED"` + * can otherwise land a PR in the `merge-ready` bucket with zero formal human + * sign-off. + */ + +const BOT_MARKERS = [ + //gi, ""); + stripped = stripped.replace(/[\s\S]*?Looper[\s\S]*?<\/sub>/gi, ""); + return stripped.trim(); +} + +export function condense(body: string, max: number): string { + const cleaned = stripBotMarkers(body).replace(/\s+/g, " ").trim(); + return cleaned.length <= max ? cleaned : `${cleaned.slice(0, max - 1)}…`; +} + +/** + * True when the PR's review-decision is APPROVED but every APPROVED-state + * review is bot-authored — i.e. no human reviewer formally signed off. We + * only flag when reviewDecision is APPROVED; for any other decision the + * notion doesn't apply. + */ +export function isBotOnlyApproval( + reviewDecision: string, + reviews: ReadonlyArray<{ author: { login: string } | null; body: string; state: string }>, +): boolean { + if (reviewDecision !== "APPROVED") return false; + const approvedReviews = reviews.filter((r) => r.state === "APPROVED"); + if (approvedReviews.length === 0) return false; + return approvedReviews.every((r) => isBotAuthored(r.author, r.body)); +} + +/** + * Reduces a PR's full review history to the most recent review per reviewer + * (by `submittedAt`). Anonymous / null-author entries are dropped. The + * result mirrors what `gh pr view --json latestReviews` returns, except it + * preserves the `commit.oid` field on each entry — which `latestReviews` + * does not expose. Detectors that ask "what is each reviewer's current + * stance?" should consume this output. + */ +export function reduceLatestReviewsByAuthor< + T extends { + author: { login: string } | null; + submittedAt: string; + }, +>(history: ReadonlyArray): T[] { + const byAuthor = new Map(); + for (const review of history) { + const login = review.author?.login; + if (!login) continue; + const existing = byAuthor.get(login); + if (!existing || existing.submittedAt < review.submittedAt) { + byAuthor.set(login, review); + } + } + return [...byAuthor.values()]; +} diff --git a/tools/pr/src/classify.ts b/tools/pr/src/classify.ts new file mode 100644 index 000000000..a01e24430 --- /dev/null +++ b/tools/pr/src/classify.ts @@ -0,0 +1,278 @@ +/** + * `tools-pr classify` — emit script-level tags for one PR or the full open + * queue. Output is strictly factual: each tag carries a name + factual + * reason + source token; no severity, no judgment, no merge guidance. + * + * Tag dictionary lives in `tools/pr/src/tags.ts` and is documented in + * `tools/pr/AGENTS.md` §Tag dictionary. + */ + +import { mkdir, writeFile } from "node:fs/promises"; +import path from "node:path"; + +import { reduceLatestReviewsByAuthor } from "./bot.js"; +import { + detectRepoSlug, + fetchOpenPrs, + fetchOrgMembers, + fetchRateLimit, + fetchView, + type FetchOpenPrsResult, + type RateLimitSnapshot, +} from "./gh.js"; +import { buildContext, classifyPr, KNOWN_TAGS } from "./tags.js"; +import type { + ClassifyContext, + ClassifyReport, + GhView, + PrFacts, + Tag, +} from "./types.js"; + +// ---- fact bridging ------------------------------------------------------- + +function factsFromList(input: FetchOpenPrsResult, orgMembers: ReadonlySet): PrFacts[] { + const statsBy = new Map(input.stats.map((row) => [row.number, row] as const)); + const filesBy = new Map(input.files.map((row) => [row.number, row] as const)); + const reviewsBy = new Map(input.reviews.map((row) => [row.number, row] as const)); + const commitsBy = new Map((input.commits ?? []).map((row) => [row.number, row] as const)); + const commentsBy = new Map((input.comments ?? []).map((row) => [row.number, row] as const)); + + return input.meta.map((meta) => { + const stats = statsBy.get(meta.number); + const filesRow = filesBy.get(meta.number); + const reviewsRow = reviewsBy.get(meta.number); + const commitsRow = commitsBy.get(meta.number); + const commentsRow = commentsBy.get(meta.number); + return { + number: meta.number, + author: meta.author.login, + title: meta.title, + createdAt: meta.createdAt, + updatedAt: meta.updatedAt, + isDraft: meta.isDraft, + reviewDecision: meta.reviewDecision, + mergeStateStatus: stats?.mergeStateStatus ?? "UNKNOWN", + maintainerCanModify: meta.maintainerCanModify, + isOrgMember: orgMembers.has(meta.author.login), + headRefOid: stats?.headRefOid ?? "", + assignees: meta.assignees.map((a) => a.login), + labels: meta.labels, + filePaths: filesRow ? filesRow.files.map((f) => f.path) : [], + reviews: reviewsRow + ? reduceLatestReviewsByAuthor(reviewsRow.reviews).map((r) => ({ + author: r.author, + body: r.body, + state: r.state, + submittedAt: r.submittedAt, + commit: r.commit ?? null, + })) + : [], + comments: commentsRow ? commentsRow.comments : [], + commits: commitsRow + ? commitsRow.commits.map((c) => ({ + committedDate: c.committedDate, + authorLogin: c.authors[0]?.login ?? null, + })) + : [], + }; + }); +} + +function factsFromView(num: number, view: GhView, orgMembers: ReadonlySet): PrFacts { + return { + number: num, + author: view.author.login, + title: view.title, + createdAt: view.createdAt, + updatedAt: view.updatedAt, + isDraft: view.isDraft, + reviewDecision: view.reviewDecision, + mergeStateStatus: view.mergeStateStatus, + maintainerCanModify: view.maintainerCanModify, + isOrgMember: orgMembers.has(view.author.login), + headRefOid: view.headRefOid, + assignees: view.assignees.map((a) => a.login), + labels: view.labels, + filePaths: view.files.map((f) => f.path), + reviews: reduceLatestReviewsByAuthor(view.reviews).map((r) => ({ + author: r.author, + body: r.body, + state: r.state, + submittedAt: r.submittedAt, + commit: r.commit ?? null, + })), + comments: view.comments.map((c) => ({ + author: c.author, + body: c.body, + createdAt: c.createdAt, + })), + commits: view.commits.map((c) => ({ + committedDate: c.committedDate, + authorLogin: c.authors[0]?.login ?? null, + })), + }; +} + +// ---- output paths -------------------------------------------------------- + +function timestampStem(): string { + // YYYY-MM-DDTHHmmssZ — colon-free for filesystem portability + const d = new Date(); + const iso = d.toISOString().replace(/[-:]/g, "").replace(/\..+/, ""); + // iso is "20260511T034500Z"; reinsert dashes for readability + return `${iso.slice(0, 4)}-${iso.slice(4, 6)}-${iso.slice(6, 8)}T${iso.slice(9, 15)}Z`; +} + +function classifyOutputDir(): string { + // Per AGENTS.md §Default runtime files convention: + // /.tmp//...; we own `tools-pr` as our source slot. + // import.meta.dirname is the dist directory when bundled; walk up to repo + // root via dist/.. = tools/pr -> tools -> root. + const here = import.meta.dirname; + // when bundled: tools/pr/dist/index.mjs -> dirname = tools/pr/dist + // when tsx-run: tools/pr/src/classify.ts -> dirname = tools/pr/src + const repoRoot = path.resolve(here, "..", "..", ".."); + return path.join(repoRoot, ".tmp", "tools-pr", "classify"); +} + +// ---- report assembly ----------------------------------------------------- + +function buildReport( + allFacts: PrFacts[], + ctx: ClassifyContext, + rateBefore: RateLimitSnapshot, + rateAfter: RateLimitSnapshot, +): ClassifyReport { + const byTag: Record = {}; + const byNumber: Record = {}; + for (const tagName of KNOWN_TAGS) byTag[tagName] = []; + + for (const facts of allFacts) { + const tags = classifyPr(facts, ctx); + byNumber[String(facts.number)] = tags; + for (const tag of tags) { + const bucket = byTag[tag.name] ?? []; + bucket.push(facts.number); + byTag[tag.name] = bucket; + } + } + + // Drop tag buckets that ended up empty so the JSON stays tight. + for (const tagName of Object.keys(byTag)) { + if ((byTag[tagName] ?? []).length === 0) delete byTag[tagName]; + } + + const cost = + rateBefore.resetAt === rateAfter.resetAt + ? rateBefore.remaining - rateAfter.remaining + : null; + + return { + generatedAt: new Date().toISOString(), + openPrTotal: allFacts.length, + classifiedCount: allFacts.length, + byTag, + byNumber, + rate: { + before: rateBefore, + after: rateAfter, + cost, + }, + }; +} + +// ---- human single-PR output --------------------------------------------- + +function formatAwaitingDuration(hours: number): string { + const days = Math.floor(hours / 24); + const rem = hours - days * 24; + if (days === 0) return `${rem}h`; + if (rem === 0) return `${days}d`; + return `${days}d ${rem}h`; +} + +function formatSinglePr(num: number, tags: Tag[]): string { + const lines: string[] = []; + lines.push(`PR #${num} — ${tags.length} tag${tags.length === 1 ? "" : "s"}`); + if (tags.length === 0) { + lines.push(" (no tags matched)"); + } else { + for (const tag of tags) { + const suffix = + tag.awaitingHours !== undefined + ? ` (awaiting ${formatAwaitingDuration(tag.awaitingHours)})` + : ""; + lines.push(` • ${tag.name}${suffix}`); + lines.push(` reason: ${tag.reason}`); + lines.push(` source: ${tag.source}`); + } + } + return lines.join("\n"); +} + +// ---- command entries ----------------------------------------------------- + +export type ClassifyOptions = { + json?: boolean; + all?: boolean; + name?: string; + print?: boolean; + limit?: number | string; +}; + +export async function runClassifyOne(num: number, options: ClassifyOptions): Promise { + if (!Number.isFinite(num) || num <= 0) { + throw new Error("classify requires a positive PR number"); + } + const { owner } = await detectRepoSlug(); + const [view, orgMembers] = await Promise.all([fetchView(num), fetchOrgMembers(owner)]); + const facts = factsFromView(num, view, orgMembers); + // Single-PR mode has no cross-PR title corpus; duplicate-title cannot fire. + const ctx: ClassifyContext = { titleIndexByAuthor: new Map() }; + const tags = classifyPr(facts, ctx); + + if (options.json) { + process.stdout.write(`${JSON.stringify({ number: num, tags }, null, 2)}\n`); + return; + } + process.stdout.write(`${formatSinglePr(num, tags)}\n`); +} + +export async function runClassifyAll(options: ClassifyOptions): Promise { + // Default covers the whole open queue with growth headroom; `gh pr list` + // paginates internally so a high cap is cheap. + const limit = Math.max(1, Number(options.limit ?? 1000) || 1000); + const { owner } = await detectRepoSlug(); + const rateBefore = await fetchRateLimit(); + const [fetched, orgMembers] = await Promise.all([ + fetchOpenPrs(limit, { includeCommits: true, includeComments: true }), + fetchOrgMembers(owner), + ]); + const rateAfter = await fetchRateLimit(); + const allFacts = factsFromList(fetched, orgMembers); + const ctx = buildContext(allFacts); + const report = buildReport(allFacts, ctx, rateBefore, rateAfter); + + const dir = classifyOutputDir(); + await mkdir(dir, { recursive: true }); + const stem = options.name && options.name.length > 0 ? options.name : timestampStem(); + const outPath = path.join(dir, `${stem}.json`); + await writeFile(outPath, `${JSON.stringify(report, null, 2)}\n`, "utf8"); + + const tagSummary = Object.entries(report.byTag) + .map(([name, nums]) => `${name}=${nums.length}`) + .join(" "); + const rateSummary = + report.rate.cost !== null + ? `rate cost=${report.rate.cost} remaining=${report.rate.after.remaining}/${report.rate.after.limit} reset=${report.rate.after.resetAt}` + : `rate remaining=${report.rate.after.remaining}/${report.rate.after.limit} (window rolled over; cost N/A)`; + process.stdout.write( + `wrote ${report.openPrTotal} entries to ${outPath}` + + (tagSummary ? ` [${tagSummary}]` : " [no tags matched]") + + ` ${rateSummary}\n`, + ); + if (options.print) { + process.stdout.write(`${JSON.stringify(report, null, 2)}\n`); + } +} diff --git a/tools/pr/src/gh.ts b/tools/pr/src/gh.ts new file mode 100644 index 000000000..7bd18b199 --- /dev/null +++ b/tools/pr/src/gh.ts @@ -0,0 +1,535 @@ +/** + * Thin gh CLI wrappers. Combining many fields in a single `gh pr list --json` + * call hits GitHub GraphQL with HTTP 502 across this repo's open queue, so + * fetchOpenPrs splits the query into three chunks and joins them by number. + */ + +import { execFile as execFileCallback } from "node:child_process"; +import { promisify } from "node:util"; + +import type { + GhAssignmentEvent, + GhAssignmentTimeline, + GhCommentsLite, + GhCommitsLite, + GhFiles, + GhMeta, + GhReviewsLite, + GhStats, + GhView, +} from "./types.js"; + +const execFile = promisify(execFileCallback); + +async function ghOnce(args: string[]): Promise { + const { stdout } = await execFile("gh", args, { maxBuffer: 64 * 1024 * 1024 }); + return JSON.parse(stdout) as T; +} + +// Treat upstream-side flakes as transient: 502/503/504 from the GraphQL gateway, +// localized GraphQL "Something went wrong" responses (which gh surfaces as 502), +// and connection-level errors. Everything else (auth failure, 4xx, json parse +// errors, schema rejection) must surface to the caller — retry would just hide +// real problems. +function looksTransient(err: unknown): boolean { + if (!(err instanceof Error)) return false; + const stderr = (err as { stderr?: string }).stderr ?? ""; + const haystack = `${stderr} ${err.message}`; + return /HTTP 5\d\d|GraphQL.*(?:502|503|504|timeout)|ECONN(?:REFUSED|RESET)|ETIMEDOUT|EAI_AGAIN/i.test( + haystack, + ); +} + +// Minimum-touch debounce around gh CLI calls. Two retries (1s, 2s) cover the +// short-window GitHub gateway hiccups we've observed during PR-duty runs. Long +// outages still bubble up — this isn't an exponential-backoff resilience layer. +const RETRY_BACKOFF_MS: readonly number[] = [1000, 2000] as const; + +export async function gh(args: string[]): Promise { + let lastError: unknown; + for (let attempt = 0; attempt <= RETRY_BACKOFF_MS.length; attempt++) { + try { + return await ghOnce(args); + } catch (err) { + lastError = err; + if (!looksTransient(err) || attempt === RETRY_BACKOFF_MS.length) throw err; + const delay = RETRY_BACKOFF_MS[attempt] ?? 1000; + await new Promise((resolve) => setTimeout(resolve, delay)); + } + } + throw lastError; +} + +export type RateLimitSnapshot = { + remaining: number; + limit: number; + resetAt: string; +}; + +/** + * Loads the login set of a GitHub org's members in one paginated REST call + * (`gh api orgs//members --paginate`). Cached for the process lifetime. + * + * Used by the `org-member` classify tag to route operational nudges away + * from GitHub comments (those go to internal IM instead). Outside repo + * collaborators who are not org members are NOT included — they don't have + * internal IM access either. + */ +let orgMembersCache: { org: string; members: Set } | null = null; +export async function fetchOrgMembers(org: string): Promise> { + if (orgMembersCache && orgMembersCache.org === org) return orgMembersCache.members; + const { stdout } = await execFile( + "gh", + ["api", "--paginate", `orgs/${org}/members`, "--jq", ".[].login"], + { maxBuffer: 16 * 1024 * 1024 }, + ); + const members = new Set(stdout.split("\n").filter((line) => line.length > 0)); + orgMembersCache = { org, members }; + return members; +} + +/** + * Snapshot the current GraphQL rate-limit state. The query itself costs 1 + * point — cheap enough to call twice around an expensive fetch to compute + * an exact delta cost. + */ +export async function fetchRateLimit(): Promise { + const { stdout } = await execFile( + "gh", + ["api", "graphql", "-f", "query={ rateLimit { remaining limit resetAt } }"], + { maxBuffer: 64 * 1024 }, + ); + const parsed = JSON.parse(stdout) as { data: { rateLimit: RateLimitSnapshot } }; + return parsed.data.rateLimit; +} + +/** + * Resolves the current repo's `owner/name`. Cached for the process lifetime. + * `gh pr list` auto-detects the repo from the cwd; for `gh api graphql` we + * need to pass it explicitly. + */ +let repoSlugCache: { owner: string; name: string } | null = null; +export async function detectRepoSlug(): Promise<{ owner: string; name: string }> { + if (repoSlugCache) return repoSlugCache; + const { stdout } = await execFile("gh", ["repo", "view", "--json", "owner,name"], { + maxBuffer: 64 * 1024, + }); + const parsed = JSON.parse(stdout) as { owner: { login: string }; name: string }; + repoSlugCache = { owner: parsed.owner.login, name: parsed.name }; + return repoSlugCache; +} + +/** + * Generic cursor-paginated `pullRequests(states: OPEN, ...)` fetch via + * `gh api graphql`. Used for the heavy-payload chunks (reviews, comments) + * where a single 100-PR query was both data-heavy (≈ 500 KB) and more + * likely to hit transient gateway flakes. Pagination spreads the load and + * lets per-page retries recover narrowly. Page size is 30 — the GitHub + * default — and keeps each node-traversal cost well under any limit. + * + * `nodeFields` is the per-PR graphql selection (must start with `number` + * so callers can join by PR number downstream). The query template wraps + * it with the `pullRequests(...)` connection + `pageInfo` so callers don't + * need to know about cursors. + */ +const PR_LIST_PAGE_SIZE = 30; + +type PaginatedPrPage = { + data: { + repository: { + pullRequests: { + nodes: N[]; + pageInfo: { hasNextPage: boolean; endCursor: string | null }; + }; + }; + }; +}; + +async function fetchPaginatedPrList(nodeFields: string): Promise { + const { owner, name } = await detectRepoSlug(); + const query = `query($owner: String!, $name: String!, $first: Int!, $cursor: String) { + repository(owner: $owner, name: $name) { + pullRequests(states: OPEN, first: $first, after: $cursor, orderBy: { field: UPDATED_AT, direction: DESC }) { + nodes { ${nodeFields} } + pageInfo { hasNextPage endCursor } + } + } +}`; + const all: N[] = []; + let cursor: string | null = null; + for (;;) { + const args = [ + "api", + "graphql", + "-F", + `owner=${owner}`, + "-F", + `name=${name}`, + "-F", + `first=${PR_LIST_PAGE_SIZE}`, + ]; + if (cursor !== null) args.push("-F", `cursor=${cursor}`); + args.push("-f", `query=${query}`); + const page = await gh>(args); + all.push(...page.data.repository.pullRequests.nodes); + const info = page.data.repository.pullRequests.pageInfo; + if (!info.hasNextPage) break; + cursor = info.endCursor; + } + return all; +} + +/** + * Fetches per-PR commit timestamps via cursor-paginated `gh api graphql`. + * Capped at `commits(last: 5)` per PR to keep node count well under + * GitHub's 500,000 traversal limit; `gh pr list --json commits` blows the + * budget even at limit=50 because it traverses the full commits + authors + * connection. + * + * Pagination removes the prior `first: ` one-shot — GraphQL's + * connection page size is capped at 100 by the server, so the previous + * implementation would error at runtime when callers passed `--limit + * 200`. Reusing `fetchPaginatedPrList` keeps this fetch consistent with + * the other heavy chunks (reviews, comments, assignment timelines). + */ + +async function fetchOpenPrReviews(): Promise { + type Node = { + number: number; + reviews: { + nodes: { + author: { login: string } | null; + body: string; + state: string; + submittedAt: string; + commit: { oid: string } | null; + }[]; + }; + }; + // last: 30 captures the latest review per reviewer for any realistically + // iterated PR (the heaviest in this repo's queue is ~15 reviews across + // 4 rounds). The reducer in bot.ts picks the latest per author from + // whatever subset we fetched. + const rows = await fetchPaginatedPrList( + `number + reviews(last: 30) { + nodes { + author { login } + body + state + submittedAt + commit { oid } + } + }`, + ); + return rows.map((row) => ({ number: row.number, reviews: row.reviews.nodes })); +} + +async function fetchOpenPrComments(): Promise { + type CommentNode = { + author: { login: string } | null; + body: string; + createdAt: string; + }; + type Node = { + number: number; + comments: { nodes: CommentNode[] }; + reviewThreads: { nodes: { comments: { nodes: CommentNode[] } }[] }; + }; + // last: 30 covers the issue-thread signal detection (we only need recent + // comments to decide whether human-reviewer signal predates author + // signal). reviewThreads sweeps inline review-comment replies; PR + // authors and reviewers regularly answer inline and never touch the + // conversation tab, so missing this stream would let `awaiting-*` + // tag the wrong side. last: 20 / first: 20 caps the per-PR fanout. + const rows = await fetchPaginatedPrList( + `number + comments(last: 30) { + nodes { + author { login } + body + createdAt + } + } + reviewThreads(last: 20) { + nodes { + comments(first: 20) { + nodes { + author { login } + body + createdAt + } + } + } + }`, + ); + return rows.map((row) => { + const inline = row.reviewThreads.nodes.flatMap((t) => t.comments.nodes); + return { number: row.number, comments: [...row.comments.nodes, ...inline] }; + }); +} + +/** + * Per-PR assignment lifecycle (ASSIGNED + UNASSIGNED timeline events). Only + * available through `gh api graphql` — `gh pr list --json` doesn't expose + * timeline data. The shape narrows the GraphQL union (TimelineItem) to the + * two relevant event types via inline fragments. + * + * Used by the `assignment` subcommand to derive: per-current-assignee, when + * were they assigned (latest ASSIGNED_EVENT not followed by UNASSIGNED_EVENT) + * and by whom. Other assignment-related signals (idle time, status) compose + * this with existing commits/comments data inside `assignment.ts`. + */ +async function fetchOpenPrAssignmentTimelines(): Promise { + type Node = { + number: number; + timelineItems: { + nodes: Array< + | { + __typename: "AssignedEvent"; + createdAt: string; + actor: { login: string } | null; + assignee: + | { __typename: "User"; login: string } + | { __typename: string } + | null; + } + | { + __typename: "UnassignedEvent"; + createdAt: string; + actor: { login: string } | null; + assignee: + | { __typename: "User"; login: string } + | { __typename: string } + | null; + } + >; + }; + }; + const rows = await fetchPaginatedPrList( + `number + timelineItems(itemTypes: [ASSIGNED_EVENT, UNASSIGNED_EVENT], last: 20) { + nodes { + __typename + ... on AssignedEvent { + createdAt + actor { login } + assignee { + __typename + ... on User { login } + } + } + ... on UnassignedEvent { + createdAt + actor { login } + assignee { + __typename + ... on User { login } + } + } + } + }`, + ); + return rows.map((row) => ({ + number: row.number, + events: row.timelineItems.nodes.map((event) => { + const kind: GhAssignmentEvent["kind"] = + event.__typename === "AssignedEvent" ? "ASSIGNED" : "UNASSIGNED"; + const assignee = + event.assignee && "login" in event.assignee + ? { login: event.assignee.login } + : null; + return { + kind, + createdAt: event.createdAt, + actor: event.actor, + assignee, + }; + }), + })); +} + +export { fetchOpenPrAssignmentTimelines }; + +/** + * Resolves the authenticated gh CLI user's login. Cached for the process + * lifetime — used by `tools-pr assignment --user me` to expand the alias. + */ +let currentUserCache: string | null = null; +export async function fetchCurrentUser(): Promise { + if (currentUserCache !== null) return currentUserCache; + const { stdout } = await execFile("gh", ["api", "user", "--jq", ".login"], { + maxBuffer: 64 * 1024, + }); + const login = stdout.trim(); + if (login.length === 0) throw new Error("gh api user returned no login"); + currentUserCache = login; + return login; +} + +async function fetchOpenPrCommits(): Promise { + type Node = { + number: number; + commits: { + nodes: { + commit: { + committedDate: string; + author: { user: { login: string } | null } | null; + }; + }[]; + }; + }; + const rows = await fetchPaginatedPrList( + `number + commits(last: 5) { + nodes { + commit { + committedDate + author { user { login } } + } + } + }`, + ); + return rows.map((row) => ({ + number: row.number, + commits: row.commits.nodes.map((entry) => ({ + oid: "", + committedDate: entry.commit.committedDate, + authors: [{ login: entry.commit.author?.user?.login ?? null }], + })), + })); +} + +export type FetchOpenPrsOptions = { + includeCommits?: boolean; + includeComments?: boolean; +}; + +export type FetchOpenPrsResult = { + meta: GhMeta[]; + stats: GhStats[]; + files: GhFiles[]; + reviews: GhReviewsLite[]; + commits?: GhCommitsLite[]; + comments?: GhCommentsLite[]; +}; + +export async function fetchOpenPrs( + limit: number, + options: FetchOpenPrsOptions = {}, +): Promise { + const baseArgs = ["pr", "list", "--state", "open", "--limit", String(limit)]; + const metaPromise = gh([ + ...baseArgs, + "--json", + "number,title,author,createdAt,updatedAt,isDraft,reviewDecision,labels,maintainerCanModify,assignees", + ]); + const statsPromise = gh([ + ...baseArgs, + "--json", + "number,additions,deletions,changedFiles,headRefName,headRefOid,baseRefName,mergeable,mergeStateStatus", + ]); + const filesPromise = gh([...baseArgs, "--json", "number,files"]); + const reviewsPromise = fetchOpenPrReviews(); + const commitsPromise = options.includeCommits + ? fetchOpenPrCommits() + : Promise.resolve(undefined); + const commentsPromise = options.includeComments + ? fetchOpenPrComments() + : Promise.resolve(undefined); + + const [meta, stats, files, reviews, commits, comments] = await Promise.all([ + metaPromise, + statsPromise, + filesPromise, + reviewsPromise, + commitsPromise, + commentsPromise, + ]); + + const result: FetchOpenPrsResult = { meta, stats, files, reviews }; + if (commits !== undefined) result.commits = commits; + if (comments !== undefined) result.comments = comments; + return result; +} + +const VIEW_FIELDS = [ + "url", + "title", + "body", + "isDraft", + "reviewDecision", + "mergeStateStatus", + "state", + "author", + "createdAt", + "updatedAt", + "labels", + "additions", + "deletions", + "changedFiles", + "baseRefName", + "headRefName", + "headRefOid", + "maintainerCanModify", + "assignees", + "files", + "statusCheckRollup", + "reviews", + "comments", + "commits", +].join(","); + +/** + * Inline review-thread comments — `pullRequest.comments` GraphQL (and + * `gh pr view --json comments`) only return the issue-conversation thread. + * REST `pulls/{n}/comments` returns the inline review-comment thread, + * which is where authors and reviewers actually exchange most fix-up + * replies. Used by `fetchView` to merge into `GhView.comments` so + * downstream `awaiting-*` and brief output see both streams. + */ +async function fetchInlineReviewComments(num: number): Promise< + Array<{ + author: { login: string } | null; + authorAssociation: string; + body: string; + createdAt: string; + }> +> { + type Raw = { + user: { login: string } | null; + author_association: string; + body: string; + created_at: string; + }; + const { owner, name } = await detectRepoSlug(); + const { stdout } = await execFile( + "gh", + ["api", "--paginate", `repos/${owner}/${name}/pulls/${num}/comments`], + { maxBuffer: 64 * 1024 * 1024 }, + ); + const raw = JSON.parse(stdout) as Raw[]; + return raw.map((c) => ({ + author: c.user ? { login: c.user.login } : null, + authorAssociation: c.author_association ?? "", + body: c.body ?? "", + createdAt: c.created_at, + })); +} + +export async function fetchView(num: number): Promise { + const [view, inline] = await Promise.all([ + gh(["pr", "view", String(num), "--json", VIEW_FIELDS]), + fetchInlineReviewComments(num), + ]); + return { ...view, comments: [...view.comments, ...inline] }; +} + +export function labelByPrefix(labels: { name: string }[], prefix: string): string | null { + const hit = labels.find((label) => label.name.startsWith(prefix)); + return hit ? hit.name.slice(prefix.length) : null; +} + +export function daysSince(iso: string, now: number): number { + return Math.max(0, Math.floor((now - new Date(iso).getTime()) / (24 * 60 * 60 * 1000))); +} diff --git a/tools/pr/src/index.ts b/tools/pr/src/index.ts new file mode 100644 index 000000000..2f78b9488 --- /dev/null +++ b/tools/pr/src/index.ts @@ -0,0 +1,74 @@ +/** + * tools-pr — maintainer PR-duty control plane for nexu-io/open-design. + * + * Thin gh wrapper that encodes review knowledge specific to this repo: lane + * derivation, forbidden-surface flags, per-lane checklists, and validation + * command derivation from touched paths. + */ + +import { cac } from "cac"; + +import { runAssignment, type AssignmentOptions } from "./assignment.js"; +import { + runClassifyAll, + runClassifyOne, + type ClassifyOptions, +} from "./classify.js"; +import { runList, type ListOptions } from "./list.js"; +import { runView, type ViewOptions } from "./view.js"; + +const cli = cac("tools-pr"); + +cli + .command("list", "Triage open PRs by lane and review state") + .option("--json", "print JSON") + .option("--include-drafts", "include draft PRs") + .option("--limit ", "limit (default 1000; pass a smaller value to truncate)") + .option("--lane ", "lane filter: skill,design-system,craft,contract,docs,default,multi") + .option("--bucket ", "bucket filter: merge-ready,approved-blocked,needs-rebase,changes-requested,new,stale,draft") + .option("--author ", "author login filter (comma-separated)") + .action(async (options: ListOptions) => { + await runList(options); + }); + +cli + .command("view ", "Agent-friendly review brief for a single PR") + .option("--json", "print JSON") + .action(async (numStr: string, options: ViewOptions) => { + await runView(Number(numStr), options); + }); + +cli + .command( + "assignment", + "Assigner-perspective view of PR ownership — who has what, how long, what blockers", + ) + .option("--json", "structured output to stdout") + .option("--user ", "filter to one assignee (login or 'me')") + .option("--unassigned", "list every unassigned PR (default collapses to count)") + .option("--include-drafts", "include draft PRs (default skips)") + .option("--limit ", "max open PRs to consider (default 1000)") + .action(async (options: AssignmentOptions) => { + await runAssignment(options); + }); + +cli + .command("classify [num]", "Emit script-level tags for one PR or the full open queue") + .option("--all", "classify the full open queue and write JSON to .tmp/tools-pr/classify/") + .option("--json", "with : print JSON to stdout (default is human)") + .option("--name ", "with --all: filename stem (default: ISO timestamp)") + .option("--print", "with --all: also print the report JSON to stdout") + .option("--limit ", "with --all: limit (default 1000; pass a smaller value to truncate)") + .action(async (numArg: string | undefined, options: ClassifyOptions) => { + if (options.all) { + await runClassifyAll(options); + return; + } + if (numArg === undefined) { + throw new Error("classify needs a PR number or --all"); + } + await runClassifyOne(Number(numArg), options); + }); + +cli.help(); +cli.parse(); diff --git a/tools/pr/src/lane.ts b/tools/pr/src/lane.ts new file mode 100644 index 000000000..cc9333498 --- /dev/null +++ b/tools/pr/src/lane.ts @@ -0,0 +1,86 @@ +/** + * Lane derivation from touched paths, plus forbidden-surface and public-seam + * detection. Rules track docs/code-review-guidelines.md §2 (Forbidden + * surfaces) and §4 (review lanes). + */ + +import type { ForbiddenHit, Lane } from "./types.js"; + +export const SKILL_DIR = /^skills\/[^/]+\//; +export const DESIGN_DIR = /^design-systems\/[^/]+\//; +export const CRAFT_DIR = /^craft\/[^/]+\.md$/; + +const CONTRACT_PATHS = [ + /^packages\/contracts\//, + /^packages\/sidecar-proto\//, + /^apps\/daemon\/src\/.*\/(routes|api|sse)\b/, +]; + +const DOCS_ONLY = [ + /^README(\..+)?\.md$/, + /^CONTRIBUTING(\..+)?\.md$/, + /^QUICKSTART(\..+)?\.md$/, + /^CHANGELOG\.md$/, + /^TRANSLATIONS\.md$/, + /^docs\//, +]; + +// Files we count toward changedFiles but suppress from the top-N preview. +const NOISY_FILE_PATTERNS = [ + /^pnpm-lock\.yaml$/, + /^CHANGELOG\.md$/, + /^README\.[a-zA-Z-]+\.md$/, + /^CONTRIBUTING\.[a-zA-Z-]+\.md$/, + /^QUICKSTART\.[a-zA-Z-]+\.md$/, + /^generated\//, + /^.*\.lock$/, +]; + +export function isNoisyFile(filePath: string): boolean { + return NOISY_FILE_PATTERNS.some((rx) => rx.test(filePath)); +} + +export function deriveLane(paths: string[]): { lane: Lane; hits: Set } { + const hits = new Set(); + let allDocs = paths.length > 0; + for (const filePath of paths) { + if (SKILL_DIR.test(filePath)) hits.add("skill"); + else if (DESIGN_DIR.test(filePath)) hits.add("design-system"); + else if (CRAFT_DIR.test(filePath)) hits.add("craft"); + else if (CONTRACT_PATHS.some((rx) => rx.test(filePath))) hits.add("contract"); + if (!DOCS_ONLY.some((rx) => rx.test(filePath))) allDocs = false; + } + if (hits.size === 0 && allDocs) return { lane: "docs", hits: new Set(["docs"]) }; + if (hits.size === 0) return { lane: "default", hits: new Set(["default"]) }; + if (hits.size === 1) { + const [only] = [...hits]; + return { lane: (only ?? "default") as Lane, hits }; + } + return { lane: "multi", hits }; +} + +// Path-only forbidden surfaces. We do NOT check root `package.json` here — +// AGENTS.md §Root command boundary forbids specific *lifecycle* aliases +// (pnpm dev / pnpm test / pnpm build / …), not the file itself, and +// tools-control-plane entrypoints like `pnpm tools-pr` are explicitly +// allowed. Distinguishing "forbidden alias added" from "allowed entry +// added" requires reading the diff content, which is the role of +// `pnpm guard` rather than a path-derived classify tag. +export function deriveForbidden(paths: string[]): ForbiddenHit[] { + const hits: ForbiddenHit[] = []; + if (paths.some((p) => p.startsWith("apps/nextjs/"))) hits.push("restores-apps/nextjs"); + if (paths.some((p) => p.startsWith("packages/shared/"))) hits.push("restores-packages/shared"); + return hits; +} + +export function deriveSeams(paths: string[]): string[] { + const seams: string[] = []; + if (paths.some((p) => p.startsWith("packages/contracts/"))) seams.push("packages/contracts"); + if (paths.some((p) => p.startsWith("packages/sidecar-proto/"))) seams.push("packages/sidecar-proto"); + if (paths.some((p) => p.startsWith("apps/daemon/src/") && /(routes|api|sse|http)/i.test(p))) + seams.push("daemon HTTP/SSE routes"); + if (paths.some((p) => /migration|schema|sql/i.test(p))) seams.push("persisted schema"); + if (paths.some((p) => p === "pnpm-workspace.yaml")) seams.push("workspace layout"); + if (paths.some((p) => p === "package.json")) seams.push("root package.json"); + return seams; +} diff --git a/tools/pr/src/list.ts b/tools/pr/src/list.ts new file mode 100644 index 000000000..71c90dcd6 --- /dev/null +++ b/tools/pr/src/list.ts @@ -0,0 +1,259 @@ +/** + * `tools-pr list` — triage queue scan. + * + * Classifies open PRs into review-state buckets (merge-ready, needs-rebase, + * changes-requested, new, stale, draft, approved-blocked) and lanes derived + * from touched paths. Outputs a grouped human report or JSON. + */ + +import { isBotOnlyApproval, reduceLatestReviewsByAuthor } from "./bot.js"; +import { daysSince, fetchOpenPrs, labelByPrefix } from "./gh.js"; +import { deriveForbidden, deriveLane } from "./lane.js"; +import type { + Bucket, + GhFiles, + GhMeta, + GhReviewsLite, + GhStats, + Lane, + Pr, +} from "./types.js"; + +const BUCKET_ORDER: Bucket[] = [ + "merge-ready", + "approved-blocked", + "needs-rebase", + "changes-requested", + "new", + "stale", + "draft", +]; + +const LANE_ORDER: Lane[] = [ + "contract", + "default", + "skill", + "design-system", + "craft", + "docs", + "multi", + "unknown", +]; + +function deriveBucket(args: { + isDraft: boolean; + reviewDecision: GhMeta["reviewDecision"]; + mergeStateStatus: GhStats["mergeStateStatus"]; + staleDays: number; +}): Bucket { + if (args.isDraft) return "draft"; + if (args.reviewDecision === "APPROVED") { + if (args.mergeStateStatus === "CLEAN" || args.mergeStateStatus === "UNSTABLE") return "merge-ready"; + return "approved-blocked"; + } + if (args.mergeStateStatus === "DIRTY" || args.mergeStateStatus === "BEHIND") return "needs-rebase"; + if (args.reviewDecision === "CHANGES_REQUESTED") return "changes-requested"; + if (args.staleDays > 14) return "stale"; + return "new"; +} + +function classify(input: { + meta: GhMeta[]; + stats: GhStats[]; + files: GhFiles[]; + reviews: GhReviewsLite[]; +}): Pr[] { + const now = Date.now(); + const statsByNum = new Map(input.stats.map((row) => [row.number, row] as const)); + const filesByNum = new Map(input.files.map((row) => [row.number, row] as const)); + const reviewsByNum = new Map(input.reviews.map((row) => [row.number, row] as const)); + + return input.meta.map((meta) => { + const stats = statsByNum.get(meta.number); + const filesRow = filesByNum.get(meta.number); + const reviewsRow = reviewsByNum.get(meta.number); + const paths = filesRow ? filesRow.files.map((file) => file.path) : []; + const { lane, hits } = deriveLane(paths); + const ageDays = daysSince(meta.createdAt, now); + const staleDays = daysSince(meta.updatedAt, now); + const bucket = deriveBucket({ + isDraft: meta.isDraft, + reviewDecision: meta.reviewDecision, + mergeStateStatus: stats?.mergeStateStatus ?? "UNKNOWN", + staleDays, + }); + const latestPerReviewer = reviewsRow + ? reduceLatestReviewsByAuthor(reviewsRow.reviews) + : []; + const botOnlyApproval = isBotOnlyApproval(meta.reviewDecision, latestPerReviewer); + return { + number: meta.number, + title: meta.title, + author: meta.author.login, + ageDays, + staleDays, + isDraft: meta.isDraft, + reviewDecision: meta.reviewDecision, + mergeStateStatus: stats?.mergeStateStatus ?? "UNKNOWN", + size: labelByPrefix(meta.labels, "size/"), + risk: labelByPrefix(meta.labels, "risk/"), + type: labelByPrefix(meta.labels, "type/"), + changedFiles: stats?.changedFiles ?? 0, + additions: stats?.additions ?? 0, + deletions: stats?.deletions ?? 0, + headRefName: stats?.headRefName ?? "", + baseRefName: stats?.baseRefName ?? "main", + lane, + laneHits: hits, + forbidden: deriveForbidden(paths), + bucket, + botOnlyApproval, + }; + }); +} + +function laneTag(lane: Lane): string { + switch (lane) { + case "contract": return "CONTRACT"; + case "skill": return "SKILL"; + case "design-system": return "DSGN-SYS"; + case "craft": return "CRAFT"; + case "docs": return "DOCS"; + case "multi": return "MULTI"; + case "default": return "DEFAULT"; + case "unknown": return "UNKNOWN"; + } +} + +function truncate(s: string, n: number): string { + return s.length <= n ? s : `${s.slice(0, n - 1)}…`; +} + +function formatHumanRow(pr: Pr): string { + const flags = [ + pr.risk ? `risk:${pr.risk[0]}` : null, + pr.size ? `sz:${pr.size}` : null, + pr.type ? `t:${pr.type}` : null, + pr.forbidden.length > 0 ? `forbid:${pr.forbidden.length}` : null, + pr.botOnlyApproval ? "bot-only" : null, + ] + .filter((v): v is string => v !== null) + .join(" "); + const num = String(pr.number).padStart(4, " "); + const lane = laneTag(pr.lane).padEnd(8, " "); + const age = `${String(pr.ageDays).padStart(2, " ")}d/${String(pr.staleDays).padStart(2, " ")}d`; + const author = truncate(pr.author, 16).padEnd(16, " "); + const title = truncate(pr.title, 64); + return ` #${num} ${lane} ${age} ${author} ${flags.padEnd(34, " ")} ${title}`; +} + +function formatHumanReport(prs: Pr[], total: number): string { + const byBucket = new Map(); + for (const pr of prs) { + const list = byBucket.get(pr.bucket) ?? []; + list.push(pr); + byBucket.set(pr.bucket, list); + } + + const lines: string[] = []; + const countLine = + prs.length === total + ? `open-design PR triage — ${total} open PRs` + : `open-design PR triage — showing ${prs.length} of ${total} open PRs`; + lines.push(countLine); + lines.push(""); + + for (const bucket of BUCKET_ORDER) { + const inBucket = byBucket.get(bucket); + if (!inBucket || inBucket.length === 0) continue; + lines.push(`▌ ${bucket} (${inBucket.length})`); + + inBucket.sort((a, b) => { + const laneDelta = LANE_ORDER.indexOf(a.lane) - LANE_ORDER.indexOf(b.lane); + if (laneDelta !== 0) return laneDelta; + return a.staleDays - b.staleDays; + }); + + for (const pr of inBucket) lines.push(formatHumanRow(pr)); + lines.push(""); + } + + const forbiddenPrs = prs.filter((pr) => pr.forbidden.length > 0); + if (forbiddenPrs.length > 0) { + lines.push(`▌ forbidden-surface hits (${forbiddenPrs.length})`); + for (const pr of forbiddenPrs) { + lines.push(` #${pr.number} ${pr.forbidden.join(", ")} ${truncate(pr.title, 60)}`); + } + lines.push(""); + } + + const botOnly = prs.filter((pr) => pr.botOnlyApproval); + if (botOnly.length > 0) { + lines.push(`▌ bot-only approval (${botOnly.length})`); + lines.push(" reviewDecision=APPROVED, but every APPROVED review is bot-authored"); + for (const pr of botOnly) { + lines.push(` #${pr.number} ${truncate(pr.title, 70)}`); + } + lines.push(""); + } + + lines.push("legend: age = created/updated days ago lane = derived from touched paths"); + lines.push(" risk / sz / t = gh label values (size/, risk/, type/ prefixes)"); + lines.push(" forbid:N = N path matches against AGENTS.md §Forbidden surfaces"); + lines.push(" bot-only = reviewDecision=APPROVED and every APPROVED review is bot-authored"); + return lines.join("\n"); +} + +export type ListOptions = { + json?: boolean; + includeDrafts?: boolean; + limit?: number | string; + lane?: string; + bucket?: string; + author?: string; +}; + +type Filters = { + lanes?: Set; + buckets?: Set; + authors?: Set; + includeDrafts: boolean; +}; + +function normalizeFilters(options: ListOptions): Filters { + const filters: Filters = { includeDrafts: Boolean(options.includeDrafts) }; + if (options.lane) filters.lanes = new Set(options.lane.split(",") as Lane[]); + if (options.bucket) filters.buckets = new Set(options.bucket.split(",") as Bucket[]); + if (options.author) filters.authors = new Set(options.author.split(",")); + return filters; +} + +function applyFilters(prs: Pr[], filters: Filters): Pr[] { + return prs.filter((pr) => { + if (!filters.includeDrafts && pr.isDraft) return false; + if (filters.lanes && !filters.lanes.has(pr.lane)) return false; + if (filters.buckets && !filters.buckets.has(pr.bucket)) return false; + if (filters.authors && !filters.authors.has(pr.author)) return false; + return true; + }); +} + +export async function runList(options: ListOptions): Promise { + const limitRaw = options.limit; + // Default is large enough to cover this repo's open queue plus growth + // headroom; `gh pr list --limit N` paginates internally so high values + // are cheap. Users can pass `--limit ` for a truncated preview. + const limit = Math.max(1, Number(limitRaw ?? 1000) || 1000); + const filters = normalizeFilters(options); + + const raw = await fetchOpenPrs(limit); + const classified = classify(raw); + const filtered = applyFilters(classified, filters); + + if (options.json) { + const payload = filtered.map((pr) => ({ ...pr, laneHits: [...pr.laneHits] })); + process.stdout.write(`${JSON.stringify(payload, null, 2)}\n`); + return; + } + process.stdout.write(`${formatHumanReport(filtered, classified.length)}\n`); +} diff --git a/tools/pr/src/tags.ts b/tools/pr/src/tags.ts new file mode 100644 index 000000000..0a211ec31 --- /dev/null +++ b/tools/pr/src/tags.ts @@ -0,0 +1,287 @@ +/** + * `tools-pr classify` tag detectors. + * + * Every detector returns either a Tag (fact + reason + source token) or + * null. Rules track this repo's AGENTS.md and code-review-guidelines.md; + * no judgment language enters the output. See + * `tools/pr/AGENTS.md` for the v1 tag dictionary. + */ + +import { isBotAuthored, isBotOnlyApproval } from "./bot.js"; +import { deriveForbidden, deriveLane, DESIGN_DIR } from "./lane.js"; +import type { ClassifyContext, PrFacts, Tag } from "./types.js"; + +const AWAITING_THRESHOLD_HOURS = 24; +const HOUR_MS = 60 * 60 * 1000; + +// ---- detectors ---------------------------------------------------------- + +function tagBotOnlyApproval(facts: PrFacts): Tag | null { + if (!isBotOnlyApproval(facts.reviewDecision, facts.reviews)) return null; + return { + name: "bot-only-approval", + reason: "reviewDecision=APPROVED; every APPROVED review is bot-authored", + source: "gh.reviewDecision+latestReviews", + }; +} + +function tagNeedsRebase(facts: PrFacts): Tag | null { + if (facts.mergeStateStatus === "DIRTY" || facts.mergeStateStatus === "BEHIND") { + return { + name: "needs-rebase", + reason: `mergeStateStatus=${facts.mergeStateStatus}`, + source: "gh.mergeStateStatus", + }; + } + return null; +} + +function tagForbiddenSurface(facts: PrFacts): Tag | null { + const hits = deriveForbidden(facts.filePaths); + if (hits.length === 0) return null; + return { + name: "forbidden-surface", + reason: `path matches AGENTS.md §Forbidden surfaces: ${hits.join(", ")}`, + source: "files+lane.deriveForbidden", + }; +} + +function tagUnlabeled(facts: PrFacts): Tag | null { + const names = facts.labels.map((label) => label.name); + const missing: string[] = []; + if (!names.some((n) => n.startsWith("size/"))) missing.push("size/"); + if (!names.some((n) => n.startsWith("risk/"))) missing.push("risk/"); + if (!names.some((n) => n.startsWith("type/"))) missing.push("type/"); + if (missing.length === 0) return null; + return { + name: "unlabeled", + reason: `missing label prefixes: ${missing.join(", ")}`, + source: "gh.labels", + }; +} + +function tagDuplicateTitle(facts: PrFacts, ctx: ClassifyContext): Tag | null { + const key = `${facts.author}\u0000${facts.title}`; + const siblings = ctx.titleIndexByAuthor.get(key); + if (!siblings || siblings.length < 2) return null; + const others = siblings.filter((n) => n !== facts.number); + if (others.length === 0) return null; + return { + name: "duplicate-title", + reason: `same author has another open PR(s) with byte-for-byte title: #${others.join(", #")}`, + source: "cross-pr.titleIndexByAuthor", + }; +} + +const ASCII_SLUG = /^[a-z0-9-]+$/; + +function tagNonAsciiSlug(facts: PrFacts): Tag | null { + const { lane, hits } = deriveLane(facts.filePaths); + if (lane !== "design-system" && !hits.has("design-system")) return null; + const slugs = new Set( + facts.filePaths + .filter((p) => DESIGN_DIR.test(p)) + .map((p) => p.split("/")[1]) + .filter((s): s is string => typeof s === "string"), + ); + const offenders = [...slugs].filter((s) => !ASCII_SLUG.test(s)); + if (offenders.length === 0) return null; + return { + name: "non-ascii-slug", + reason: `design-system slug fails /^[a-z0-9-]+$/: ${offenders.join(", ")}`, + source: "files+lane.DESIGN_DIR", + }; +} + +function tagMaintainerEditsDisabled(facts: PrFacts): Tag | null { + if (facts.maintainerCanModify) return null; + return { + name: "maintainer-edits-disabled", + reason: "maintainerCanModify=false on the fork — maintainers cannot push to the PR branch", + source: "gh.maintainerCanModify", + }; +} + +function tagOrgMember(facts: PrFacts): Tag | null { + if (!facts.isOrgMember) return null; + return { + name: "org-member", + reason: `author ${facts.author} is a member of the repo's org`, + source: "gh api orgs//members", + }; +} + +function tagUnresolvedChangesRequested(facts: PrFacts): Tag | null { + const reviewers = facts.reviews + .filter((r) => r.state === "CHANGES_REQUESTED" && r.author?.login) + .map((r) => r.author?.login) + .filter((login): login is string => typeof login === "string"); + if (reviewers.length === 0) return null; + return { + name: "unresolved-changes-requested", + reason: `latestReviews carries CHANGES_REQUESTED from: ${[...new Set(reviewers)].join(", ")}`, + source: "gh.latestReviews[].state", + }; +} + +function tagStaleApproval(facts: PrFacts): Tag | null { + if (!facts.headRefOid) return null; + const stale = facts.reviews + .filter((r) => r.state === "APPROVED") + .map((r) => { + const oid = r.commit?.oid; + if (!oid || oid.length === 0) return null; + if (oid === facts.headRefOid) return null; + return { login: r.author?.login ?? "(unknown)", oid }; + }) + .filter((entry): entry is { login: string; oid: string } => entry !== null); + if (stale.length === 0) return null; + const summary = stale + .map((entry) => `${entry.login}@${entry.oid.slice(0, 7)}`) + .join(", "); + return { + name: "stale-approval", + reason: `APPROVED review(s) at ${summary} predate current head ${facts.headRefOid.slice(0, 7)}`, + source: "gh.latestReviews[].commit.oid+gh.headRefOid", + }; +} + +// ---- timing detectors --------------------------------------------------- + +function authorSignalAt(facts: PrFacts): number | null { + const author = facts.author; + let max: number | null = null; + for (const c of facts.commits) { + // On maintainerCanModify=true PRs a maintainer can push a follow-up + // commit. Counting that as an author signal would flip + // `awaiting-author-response-*` off even though the author never replied. + if (c.authorLogin !== author) continue; + const t = Date.parse(c.committedDate); + if (Number.isFinite(t) && (max === null || t > max)) max = t; + } + for (const cmt of facts.comments) { + if (cmt.author?.login !== author) continue; + const t = Date.parse(cmt.createdAt); + if (Number.isFinite(t) && (max === null || t > max)) max = t; + } + return max; +} + +function humanReviewerSignalAt(facts: PrFacts): number | null { + const author = facts.author; + let max: number | null = null; + for (const r of facts.reviews) { + const login = r.author?.login; + if (!login || login === author) continue; + if (isBotAuthored(r.author, r.body)) continue; + const t = Date.parse(r.submittedAt); + if (Number.isFinite(t) && (max === null || t > max)) max = t; + } + for (const c of facts.comments) { + const login = c.author?.login; + if (!login || login === author) continue; + if (isBotAuthored(c.author, c.body)) continue; + const t = Date.parse(c.createdAt); + if (Number.isFinite(t) && (max === null || t > max)) max = t; + } + return max; +} + +function tagAwaitingAuthorResponse(facts: PrFacts): Tag | null { + const reviewer = humanReviewerSignalAt(facts); + const author = authorSignalAt(facts); + if (reviewer === null || author === null) return null; + if (reviewer <= author) return null; + const gapHours = Math.floor((Date.now() - reviewer) / HOUR_MS); + if (gapHours < AWAITING_THRESHOLD_HOURS) return null; + return { + name: `awaiting-author-response-${AWAITING_THRESHOLD_HOURS}h`, + reason: `latest human-reviewer signal (${new Date(reviewer).toISOString()}) is ${gapHours}h ago and newer than latest author signal (${new Date(author).toISOString()})`, + source: "latestReviews+comments+commits", + awaitingHours: gapHours, + }; +} + +function tagAwaitingReviewerResponse(facts: PrFacts): Tag | null { + const reviewer = humanReviewerSignalAt(facts); + const author = authorSignalAt(facts); + if (reviewer === null || author === null) return null; + if (author <= reviewer) return null; + const gapHours = Math.floor((Date.now() - author) / HOUR_MS); + if (gapHours < AWAITING_THRESHOLD_HOURS) return null; + return { + name: `awaiting-reviewer-response-${AWAITING_THRESHOLD_HOURS}h`, + reason: `latest author signal (${new Date(author).toISOString()}) is ${gapHours}h ago and newer than latest human-reviewer signal (${new Date(reviewer).toISOString()})`, + source: "latestReviews+comments+commits", + awaitingHours: gapHours, + }; +} + +function tagAwaitingFirstReview(facts: PrFacts): Tag | null { + const reviewer = humanReviewerSignalAt(facts); + if (reviewer !== null) return null; + const createdAt = Date.parse(facts.createdAt); + if (!Number.isFinite(createdAt)) return null; + const ageHours = Math.floor((Date.now() - createdAt) / HOUR_MS); + if (ageHours < AWAITING_THRESHOLD_HOURS) return null; + return { + name: `awaiting-first-review-${AWAITING_THRESHOLD_HOURS}h`, + reason: `no human review or non-author non-bot comment exists; createdAt is ${ageHours}h ago`, + source: "latestReviews+comments+createdAt", + awaitingHours: ageHours, + }; +} + +// ---- orchestrator ------------------------------------------------------- + +const DETECTORS: ReadonlyArray<(facts: PrFacts, ctx: ClassifyContext) => Tag | null> = [ + tagBotOnlyApproval, + tagNeedsRebase, + tagForbiddenSurface, + tagUnlabeled, + tagDuplicateTitle, + tagNonAsciiSlug, + tagMaintainerEditsDisabled, + tagOrgMember, + tagUnresolvedChangesRequested, + tagStaleApproval, + tagAwaitingAuthorResponse, + tagAwaitingReviewerResponse, + tagAwaitingFirstReview, +]; + +export function classifyPr(facts: PrFacts, ctx: ClassifyContext): Tag[] { + const out: Tag[] = []; + for (const detector of DETECTORS) { + const tag = detector(facts, ctx); + if (tag !== null) out.push(tag); + } + return out; +} + +export function buildContext(allFacts: PrFacts[]): ClassifyContext { + const titleIndexByAuthor = new Map(); + for (const facts of allFacts) { + const key = `${facts.author}\u0000${facts.title}`; + const existing = titleIndexByAuthor.get(key); + if (existing) existing.push(facts.number); + else titleIndexByAuthor.set(key, [facts.number]); + } + return { titleIndexByAuthor }; +} + +export const KNOWN_TAGS: readonly string[] = [ + "bot-only-approval", + "needs-rebase", + "forbidden-surface", + "unlabeled", + "duplicate-title", + "non-ascii-slug", + "maintainer-edits-disabled", + "org-member", + "unresolved-changes-requested", + "stale-approval", + `awaiting-author-response-${AWAITING_THRESHOLD_HOURS}h`, + `awaiting-reviewer-response-${AWAITING_THRESHOLD_HOURS}h`, + `awaiting-first-review-${AWAITING_THRESHOLD_HOURS}h`, +] as const; diff --git a/tools/pr/src/types.ts b/tools/pr/src/types.ts new file mode 100644 index 000000000..b9a271c41 --- /dev/null +++ b/tools/pr/src/types.ts @@ -0,0 +1,333 @@ +/** + * Shared types for tools-pr. + * + * Lanes mirror docs/code-review-guidelines.md §4 (default / contract / + * design-system / skill / craft). Buckets are triage-state derived from + * review decision + merge state + staleness. + */ + +export type Lane = + | "skill" + | "design-system" + | "craft" + | "contract" + | "docs" + | "default" + | "multi" + | "unknown"; + +export type Bucket = + | "merge-ready" + | "approved-blocked" + | "changes-requested" + | "needs-rebase" + | "new" + | "draft" + | "stale"; + +export type ForbiddenHit = + | "restores-apps/nextjs" + | "restores-packages/shared"; + +// --- gh `pr list --json` chunked shapes ---------------------------------- + +export type GhMeta = { + number: number; + title: string; + author: { login: string }; + createdAt: string; + updatedAt: string; + isDraft: boolean; + reviewDecision: "" | "APPROVED" | "CHANGES_REQUESTED" | "REVIEW_REQUIRED" | "COMMENTED"; + labels: { name: string }[]; + maintainerCanModify: boolean; + assignees: { login: string }[]; +}; + +export type GhStats = { + number: number; + additions: number; + deletions: number; + changedFiles: number; + headRefName: string; + headRefOid: string; + baseRefName: string; + mergeable: "MERGEABLE" | "CONFLICTING" | "UNKNOWN"; + mergeStateStatus: + | "CLEAN" + | "BLOCKED" + | "BEHIND" + | "DIRTY" + | "UNKNOWN" + | "UNSTABLE" + | "HAS_HOOKS" + | "DRAFT"; +}; + +export type GhFiles = { + number: number; + files: { path: string; additions: number; deletions: number }[]; +}; + +export type GhReviewsLite = { + number: number; + /** + * Full review history (every state transition by every reviewer). Use + * `reduceLatestReviewsByAuthor` to collapse to the per-reviewer current + * state. The full history is needed because `gh pr list --json + * latestReviews` strips `commit.oid`, while `--json reviews` preserves + * it — that field is load-bearing for the `stale-approval` tag. + */ + reviews: { + author: { login: string } | null; + body: string; + state: string; + submittedAt: string; + commit?: { oid: string } | null; + }[]; +}; + +export type GhCommitsLite = { + number: number; + commits: { + oid: string; + committedDate: string; + authors: { login: string | null }[]; + }[]; +}; + +export type GhCommentsLite = { + number: number; + comments: { + author: { login: string } | null; + body: string; + createdAt: string; + }[]; +}; + +/** + * Per-PR ASSIGNED_EVENT / UNASSIGNED_EVENT timeline entries fetched via + * `gh api graphql` (the only path that exposes `assignedAt` / `actor` for + * assignment lifecycle on a PR). `kind` distinguishes the two event types + * after the GraphQL union has been narrowed; `assignee` is the user the + * event targets (other actor types like Bot / Mannequin are filtered to + * null at fetch time). + */ +export type GhAssignmentEvent = { + kind: "ASSIGNED" | "UNASSIGNED"; + createdAt: string; + actor: { login: string } | null; + assignee: { login: string } | null; +}; + +export type GhAssignmentTimeline = { + number: number; + events: GhAssignmentEvent[]; +}; + +// --- gh `pr view --json` shape ------------------------------------------- + +export type GhFile = { path: string; additions: number; deletions: number; changeType: string }; + +export type GhReview = { + author: { login: string } | null; + authorAssociation: string; + body: string; + state: string; + submittedAt: string; + commit?: { oid: string } | null; +}; + +export type GhComment = { + author: { login: string } | null; + authorAssociation: string; + body: string; + createdAt: string; +}; + +export type GhCheck = { + __typename: string; + name?: string; + workflowName?: string; + conclusion?: string | null; + status?: string; + state?: string; + context?: string; +}; + +export type GhView = { + url: string; + title: string; + body: string; + isDraft: boolean; + reviewDecision: GhMeta["reviewDecision"]; + mergeStateStatus: GhStats["mergeStateStatus"]; + state: string; + author: { login: string }; + createdAt: string; + updatedAt: string; + labels: { name: string }[]; + additions: number; + deletions: number; + changedFiles: number; + baseRefName: string; + headRefName: string; + headRefOid: string; + maintainerCanModify: boolean; + assignees: { login: string }[]; + files: GhFile[]; + statusCheckRollup: GhCheck[]; + reviews: GhReview[]; + comments: GhComment[]; + commits: { + oid: string; + committedDate: string; + authors: { login: string | null }[]; + }[]; +}; + +// --- triage / brief composites ------------------------------------------- + +export type Pr = { + number: number; + title: string; + author: string; + ageDays: number; + staleDays: number; + isDraft: boolean; + reviewDecision: GhMeta["reviewDecision"]; + mergeStateStatus: GhStats["mergeStateStatus"]; + size: string | null; + risk: string | null; + type: string | null; + changedFiles: number; + additions: number; + deletions: number; + headRefName: string; + baseRefName: string; + lane: Lane; + laneHits: Set; + forbidden: ForbiddenHit[]; + bucket: Bucket; + botOnlyApproval: boolean; +}; + +export type ValidationCommand = { command: string; reason: string }; + +// --- classify ------------------------------------------------------------ + +/** + * A single tag emitted by classify. Each tag carries a script-stable name, + * a factual reason string (no judgment language), and a source token + * identifying which data field or rule produced it. + * + * `awaitingHours` is set only by the three `awaiting-*` tags; it reports + * the integer hour count between the awaiting-window start and the + * classify-run moment. Downstream consumers can floor-divide by 24 to get + * days, or use it as a sort key to prioritise the longest-waiting PRs + * inside an awaiting bucket. + */ +export type Tag = { + name: string; + reason: string; + source: string; + awaitingHours?: number; +}; + +/** + * Distilled per-PR facts consumed by tag detectors. Built from either the + * list-mode chunked fetch or the single-PR `gh pr view` fetch — both + * representations converge here so detectors stay agnostic. + */ +export type PrFacts = { + number: number; + author: string; + title: string; + createdAt: string; + updatedAt: string; + isDraft: boolean; + reviewDecision: GhMeta["reviewDecision"]; + mergeStateStatus: GhStats["mergeStateStatus"]; + maintainerCanModify: boolean; + isOrgMember: boolean; + headRefOid: string; + assignees: string[]; + labels: { name: string }[]; + filePaths: string[]; + reviews: { + author: { login: string } | null; + body: string; + state: string; + submittedAt: string; + commit?: { oid: string } | null; + }[]; + comments: { + author: { login: string } | null; + body: string; + createdAt: string; + }[]; + commits: { + committedDate: string; + authorLogin: string | null; + }[]; +}; + +/** + * Cross-PR context passed to detectors that need to look beyond a single + * PR (e.g. duplicate-title needs the full open-queue title index). + */ +export type ClassifyContext = { + /** Map of `${author}\0${title}` to PR numbers that share it. */ + titleIndexByAuthor: Map; +}; + +export type ClassifyReport = { + generatedAt: string; + openPrTotal: number; + classifiedCount: number; + byTag: Record; + byNumber: Record; + rate: { + /** GraphQL points remaining before the fetch started. */ + before: { remaining: number; limit: number; resetAt: string }; + /** GraphQL points remaining after the fetch finished. */ + after: { remaining: number; limit: number; resetAt: string }; + /** + * Computed delta = before.remaining - after.remaining, *only* when both + * snapshots fell in the same reset window. `null` when the window rolled + * over between snapshots and the delta is no longer meaningful. + */ + cost: number | null; + }; +}; + +export type Brief = { + number: number; + url: string; + title: string; + state: string; + reviewDecision: GhMeta["reviewDecision"]; + mergeStateStatus: GhStats["mergeStateStatus"]; + isDraft: boolean; + author: string; + branch: { head: string; base: string }; + age: { createdAt: string; updatedAt: string; ageDays: number; staleDays: number }; + labels: { size: string | null; risk: string | null; type: string | null; all: string[] }; + diff: { additions: number; deletions: number; changedFiles: number }; + lane: Lane; + laneHits: Lane[]; + forbidden: ForbiddenHit[]; + seamsTouched: string[]; + topFiles: { path: string; additions: number; deletions: number; changeType: string }[]; + filterSuppressedFileCount: number; + laneRules: string[]; + validation: ValidationCommand[]; + reviews: { author: string; state: string; submittedAt: string; body: string }[]; + reviewCountTotal: number; + botOnlyApproval: boolean; + comments: { author: string; createdAt: string; body: string }[]; + commentCountTotal: number; + checks: { workflow: string; passing: number; failing: number; pending: number; total: number }[]; + bodyPreview: string; + bodyChars: number; +}; diff --git a/tools/pr/src/view.ts b/tools/pr/src/view.ts new file mode 100644 index 000000000..c96148ae8 --- /dev/null +++ b/tools/pr/src/view.ts @@ -0,0 +1,408 @@ +/** + * `tools-pr view ` — factual brief for a single PR. + * + * Output is strictly facts + repo-doc citations; no judgments, no directives. + * Sections: + * - lane + forbidden-surface + public-seam observations + * - top files (lockfile, translations, generated dirs filtered as noise) + * - per-lane rules cited from code-review-guidelines.md / AGENTS.md / + * CONTRIBUTING.zh-CN.md with sources noted inline + * - validation commands derived from touched packages, per AGENTS.md + * §Validation strategy + * - bot-stripped review + comment summary (Looper-stamped reviews moved to + * the bot-only-approval fact line) + * - CI rollup grouped by workflow + */ + +import { + condense, + isBotAuthored, + isBotOnlyApproval, + reduceLatestReviewsByAuthor, +} from "./bot.js"; +import { daysSince, fetchView, labelByPrefix } from "./gh.js"; +import { deriveForbidden, deriveLane, deriveSeams, isNoisyFile, SKILL_DIR, DESIGN_DIR } from "./lane.js"; +import type { + Brief, + GhCheck, + GhView, + Lane, + ValidationCommand, +} from "./types.js"; + +// --- validation derivation ----------------------------------------------- + +function deriveValidation(paths: string[]): ValidationCommand[] { + const cmds: ValidationCommand[] = []; + const seen = new Set(); + const add = (command: string, reason: string): void => { + if (seen.has(command)) return; + seen.add(command); + cmds.push({ command, reason }); + }; + + add("pnpm guard", "TS-first + .js allowlist gate"); + add("pnpm typecheck", "workspace-wide typecheck (root)"); + + const touched = (prefix: string): boolean => paths.some((p) => p.startsWith(prefix)); + const touchedAny = (prefixes: string[]): boolean => prefixes.some(touched); + + if (touched("apps/web/")) { + add("pnpm --filter @open-design/web typecheck", "apps/web changed"); + add("pnpm --filter @open-design/web test", "apps/web changed"); + add("pnpm --filter @open-design/web build", "apps/web changed"); + } + if (touched("apps/daemon/")) { + add("pnpm --filter @open-design/daemon typecheck", "apps/daemon changed"); + add("pnpm --filter @open-design/daemon test", "apps/daemon changed"); + add("pnpm --filter @open-design/daemon build", "apps/daemon changed"); + } + if (touched("apps/desktop/")) { + add("pnpm --filter @open-design/desktop typecheck", "apps/desktop changed"); + add("pnpm --filter @open-design/desktop build", "apps/desktop changed"); + } + if (touched("apps/packaged/")) { + add("pnpm --filter @open-design/packaged typecheck", "apps/packaged changed"); + add("pnpm --filter @open-design/packaged build", "apps/packaged changed"); + } + if (touched("packages/contracts/")) { + add("pnpm --filter @open-design/contracts typecheck", "packages/contracts changed"); + } + if (touched("packages/sidecar-proto/")) { + add("pnpm --filter @open-design/sidecar-proto typecheck", "sidecar-proto changed"); + add("pnpm --filter @open-design/sidecar-proto test", "sidecar-proto changed"); + } + if (touched("packages/sidecar/")) { + add("pnpm --filter @open-design/sidecar typecheck", "packages/sidecar changed"); + add("pnpm --filter @open-design/sidecar test", "packages/sidecar changed"); + } + if (touched("packages/platform/")) { + add("pnpm --filter @open-design/platform typecheck", "packages/platform changed"); + add("pnpm --filter @open-design/platform test", "packages/platform changed"); + } + if (touched("tools/dev/")) { + add("pnpm --filter @open-design/tools-dev typecheck", "tools/dev changed"); + add("pnpm --filter @open-design/tools-dev build", "tools/dev changed"); + } + if (touched("tools/pack/")) { + add("pnpm --filter @open-design/tools-pack typecheck", "tools/pack changed"); + add("pnpm --filter @open-design/tools-pack build", "tools/pack changed"); + } + if (touched("tools/pr/")) { + add("pnpm --filter @open-design/tools-pr typecheck", "tools/pr changed"); + add("pnpm --filter @open-design/tools-pr build", "tools/pr changed"); + } + if (touchedAny(["e2e/specs/", "e2e/tests/", "e2e/lib/"])) { + add("pnpm --filter @open-design/e2e typecheck", "e2e/ changed"); + add("(cd e2e && pnpm test specs)", "e2e specs are the PR smoke gate"); + } + if (touched("e2e/ui/")) { + add("(cd e2e && pnpm exec playwright test -c playwright.config.ts)", "Playwright UI changed"); + } + + const stampRelated = paths.some((p) => /(sidecar|stamp|namespace|packaged|tools-pack)/i.test(p)); + if (stampRelated) { + add( + "# run inspect eval + screenshot for two concurrent namespaces (AGENTS.md)", + "stamp/namespace surface touched", + ); + } + const pathLogRelated = paths.some((p) => /(tools-dev|tools-pack|log|logger|\.tmp)/i.test(p)); + if (pathLogRelated) { + add( + "pnpm tools-dev logs --namespace --json", + "path/log surface touched — confirm paths under .tmp/tools-dev//", + ); + } + return cmds; +} + +// --- lane checklist ------------------------------------------------------ + +/** + * Per-lane rule citations. Each line is either an observed fact about the + * touched paths or a quotation/summary of an existing repo rule with its + * source noted. No judgments, no directives. + */ +function laneRules(lane: Lane, paths: string[]): string[] { + const items: string[] = []; + const hasFile = (suffix: string): boolean => paths.some((p) => p.endsWith(suffix)); + const skillRoots = new Set( + paths.filter((p) => SKILL_DIR.test(p)).map((p) => p.split("/").slice(0, 2).join("/")), + ); + const designRoots = new Set( + paths.filter((p) => DESIGN_DIR.test(p)).map((p) => p.split("/").slice(0, 2).join("/")), + ); + + switch (lane) { + case "skill": + items.push(`fact: skill roots touched — ${[...skillRoots].join(", ") || "(none)"}`); + items.push( + `fact: SKILL.md present at every touched root — ${[...skillRoots].every((root) => paths.includes(`${root}/SKILL.md`)) ? "yes" : "no"}`, + ); + items.push(`fact: example.html present at a touched root — ${hasFile("/example.html") ? "yes" : "no"}`); + items.push(`fact: references/checklist.md present at a touched root — ${hasFile("/references/checklist.md") ? "yes" : "no"}`); + items.push("rule [CONTRIBUTING.zh-CN.md skill 硬线 1]: real hand-crafted example.html present"); + items.push("rule [CONTRIBUTING.zh-CN.md skill 硬线 2]: anti-AI-slop list — purple gradients, generic emoji icons, Inter-as-display, invented numbers"); + items.push("rule [CONTRIBUTING.zh-CN.md skill 硬线 4]: references/checklist.md with at least the P0 gate"); + items.push("rule [CONTRIBUTING.zh-CN.md skill 硬线 5]: featured skill ⇒ docs/screenshots/skills/.png"); + items.push("rule [CONTRIBUTING.zh-CN.md skill 硬线 6]: self-contained — CDN scope ≤ existing skills, no unlicensed fonts, assets ≲ 250 KB"); + break; + case "design-system": + items.push(`fact: design-system roots touched — ${[...designRoots].join(", ") || "(none)"}`); + items.push(`fact: DESIGN.md present at a touched root — ${hasFile("/DESIGN.md") ? "yes" : "no"}`); + items.push("rule [code-review-guidelines.md §4.3]: first H1 = picker title; '> Category:' line uses an existing dropdown group"); + items.push("rule [CONTRIBUTING.zh-CN.md design-system 硬线 1]: 9 sections present (visual / color / typography / spacing / layout / components / motion / voice / anti-patterns)"); + items.push("rule [CONTRIBUTING.zh-CN.md design-system 硬线 5]: ASCII slug only (linear.app → linear-app)"); + items.push("rule [CONTRIBUTING.zh-CN.md design-system 硬线 2-3]: hex sampled from real source; OKLch acceptable for accent ramps"); + break; + case "craft": + items.push("rule [code-review-guidelines.md §4.5]: universal brand-agnostic craft (not brand-specific, not artifact-shape)"); + items.push("rule [code-review-guidelines.md §4.5]: at least one shipping skill opts in via od.craft.requires, or follow-up named in PR description"); + items.push("reference: existing craft entry shapes — craft/typography.md, craft/color.md, craft/animation-discipline.md"); + break; + case "contract": + items.push("rule [AGENTS.md §Boundary constraints]: packages/contracts free of Next/Express/Node fs|process/browser APIs/SQLite/daemon internals/sidecar control-plane deps"); + items.push("rule [AGENTS.md §Boundary constraints]: stamp fields exactly five — app, mode, namespace, ipc, source"); + items.push("rule [code-review-guidelines.md §4.2]: contract change lands before consumers, or in same PR with both sides wired"); + items.push("rule [code-review-guidelines.md §4.2]: breaking persisted-format change requires explicit migration + one-release compat window"); + items.push("rule [code-review-guidelines.md §4.2]: producer and consumer both have type/test coverage of the new shape"); + break; + case "docs": + items.push("rule [code-review-guidelines.md §7 Documentation-only review]: internal link integrity (relative paths, anchors)"); + items.push("rule [code-review-guidelines.md §7]: no conflict with AGENTS.md chain (root > directory-level)"); + items.push("rule [AGENTS.md §Validation strategy]: pnpm guard + pnpm typecheck required"); + break; + case "multi": + items.push("rule [code-review-guidelines.md §3 In scope, Multi-area]: public seam motivates the cross-cut (HTTP API / contract / sidecar / command / persisted format)"); + items.push("rule [code-review-guidelines.md §3]: owning contract/protocol/primitive change lands first or in same PR"); + items.push("rule [code-review-guidelines.md §3]: one clear primary owner"); + break; + case "default": + case "unknown": + items.push("rule [AGENTS.md §Boundary constraints]: tests live in sibling tests/ directories; no *.test.* under src/"); + items.push("rule [AGENTS.md §Boundary constraints]: shared logic in owning package; no cross-app private imports"); + items.push("rule [AGENTS.md §Boundary constraints]: shared web/daemon API DTOs in packages/contracts, not in app internals"); + items.push("reference: forbidden-surface scan in the Boundaries section above is authoritative"); + break; + } + return items; +} + +// --- check rollup -------------------------------------------------------- + +function summarizeChecks(rollup: GhCheck[]): Brief["checks"] { + const groups = new Map< + string, + { passing: number; failing: number; pending: number; total: number } + >(); + for (const check of rollup) { + const key = check.workflowName ?? check.name ?? check.context ?? "(unknown)"; + const bucket = groups.get(key) ?? { passing: 0, failing: 0, pending: 0, total: 0 }; + bucket.total += 1; + const conclusion = (check.conclusion ?? check.state ?? "").toUpperCase(); + if (conclusion === "SUCCESS" || conclusion === "NEUTRAL" || conclusion === "SKIPPED") bucket.passing += 1; + else if ( + conclusion === "FAILURE" || + conclusion === "CANCELLED" || + conclusion === "TIMED_OUT" || + conclusion === "ACTION_REQUIRED" + ) + bucket.failing += 1; + else bucket.pending += 1; + groups.set(key, bucket); + } + return [...groups.entries()] + .map(([workflow, stats]) => ({ workflow, ...stats })) + .sort((a, b) => b.failing - a.failing || a.workflow.localeCompare(b.workflow)); +} + +// --- brief assembly + formatting ----------------------------------------- + +function buildBrief(num: number, view: GhView): Brief { + const now = Date.now(); + const paths = view.files.map((file) => file.path); + const { lane, hits } = deriveLane(paths); + + const topFiles = [...view.files] + .filter((file) => !isNoisyFile(file.path)) + .sort((a, b) => b.additions + b.deletions - (a.additions + a.deletions)) + .slice(0, 8) + .map((file) => ({ + path: file.path, + additions: file.additions, + deletions: file.deletions, + changeType: file.changeType, + })); + const filterSuppressedFileCount = view.files.length - topFiles.length; + + const latestPerReviewer = reduceLatestReviewsByAuthor(view.reviews); + const reviews = latestPerReviewer + .filter((review) => !isBotAuthored(review.author, review.body)) + .map((review) => ({ + author: review.author?.login ?? "(unknown)", + state: review.state, + submittedAt: review.submittedAt, + body: condense(review.body, 200), + })) + .sort((a, b) => b.submittedAt.localeCompare(a.submittedAt)); + + const comments = view.comments + .filter((comment) => !isBotAuthored(comment.author, comment.body)) + .map((comment) => ({ + author: comment.author?.login ?? "(unknown)", + createdAt: comment.createdAt, + body: condense(comment.body, 200), + })) + .sort((a, b) => b.createdAt.localeCompare(a.createdAt)) + .slice(0, 3); + + return { + number: num, + url: view.url, + title: view.title, + state: view.state, + reviewDecision: view.reviewDecision, + mergeStateStatus: view.mergeStateStatus, + isDraft: view.isDraft, + author: view.author.login, + branch: { head: view.headRefName, base: view.baseRefName }, + age: { + createdAt: view.createdAt, + updatedAt: view.updatedAt, + ageDays: daysSince(view.createdAt, now), + staleDays: daysSince(view.updatedAt, now), + }, + labels: { + size: labelByPrefix(view.labels, "size/"), + risk: labelByPrefix(view.labels, "risk/"), + type: labelByPrefix(view.labels, "type/"), + all: view.labels.map((label) => label.name), + }, + diff: { + additions: view.additions, + deletions: view.deletions, + changedFiles: view.changedFiles, + }, + lane, + laneHits: [...hits], + forbidden: deriveForbidden(paths), + seamsTouched: deriveSeams(paths), + topFiles, + filterSuppressedFileCount, + laneRules: laneRules(lane, paths), + validation: deriveValidation(paths), + reviews, + reviewCountTotal: latestPerReviewer.length, + botOnlyApproval: isBotOnlyApproval(view.reviewDecision, latestPerReviewer), + comments, + commentCountTotal: view.comments.length, + checks: summarizeChecks(view.statusCheckRollup), + bodyPreview: condense(view.body, 400), + bodyChars: view.body.length, + }; +} + +function formatBrief(brief: Brief): string { + const lines: string[] = []; + const labelTags = [ + brief.labels.size ? `size/${brief.labels.size}` : null, + brief.labels.risk ? `risk/${brief.labels.risk}` : null, + brief.labels.type ? `type/${brief.labels.type}` : null, + ].filter((v): v is string => v !== null); + + lines.push(`PR #${brief.number} · ${brief.title}`); + lines.push(` url ${brief.url}`); + lines.push(` author ${brief.author}`); + lines.push(` branch ${brief.branch.head} → ${brief.branch.base}`); + lines.push( + ` state ${brief.state} · ${brief.reviewDecision || "REVIEW_REQUIRED"} · ${brief.mergeStateStatus}${brief.isDraft ? " · draft" : ""}`, + ); + lines.push(` age created ${brief.age.ageDays}d ago · updated ${brief.age.staleDays}d ago`); + lines.push(` labels ${labelTags.join(", ") || "(none)"}`); + lines.push( + ` diff +${brief.diff.additions} −${brief.diff.deletions} across ${brief.diff.changedFiles} files`, + ); + if (brief.botOnlyApproval) { + lines.push(""); + lines.push(" fact: bot-only approval — reviewDecision=APPROVED, every APPROVED review is bot-authored."); + lines.push(" fact: zero APPROVED reviews authored by a non-bot account."); + } + lines.push(""); + + lines.push("── Boundaries (lane / forbidden / seams) ──"); + lines.push( + ` lane ${brief.lane}${brief.laneHits.length > 1 ? ` (hits: ${brief.laneHits.join(", ")})` : ""}`, + ); + lines.push(` forbidden ${brief.forbidden.length === 0 ? "[none]" : brief.forbidden.join(", ")}`); + lines.push(` seams ${brief.seamsTouched.length === 0 ? "[none]" : brief.seamsTouched.join(", ")}`); + lines.push(""); + + lines.push(`── Top files (${brief.topFiles.length} shown, ${brief.filterSuppressedFileCount} filtered by NOISY_FILE_PATTERNS in lane.ts) ──`); + for (const file of brief.topFiles) { + const delta = `+${file.additions} −${file.deletions}`.padEnd(10, " "); + lines.push(` ${delta} ${file.path} (${file.changeType})`); + } + lines.push(""); + + lines.push(`── Lane rules (${brief.lane}) ──`); + for (const item of brief.laneRules) lines.push(` • ${item}`); + lines.push(""); + + lines.push("── Validation (AGENTS.md §Validation strategy, derived from touched packages) ──"); + for (const cmd of brief.validation) { + lines.push(` $ ${cmd.command}`); + lines.push(` ↳ ${cmd.reason}`); + } + lines.push(""); + + lines.push(`── Recent reviews (${brief.reviews.length} human-shown of ${brief.reviewCountTotal} total) ──`); + if (brief.reviews.length === 0) lines.push(" (no human reviews yet)"); + for (const review of brief.reviews) { + lines.push(` @${review.author} ${review.state} ${review.submittedAt}`); + lines.push(` "${review.body}"`); + } + lines.push(""); + + lines.push(`── Recent comments (${brief.comments.length} of ${brief.commentCountTotal}) ──`); + if (brief.comments.length === 0) lines.push(" (no human comments)"); + for (const comment of brief.comments) { + lines.push(` @${comment.author} ${comment.createdAt}`); + lines.push(` "${comment.body}"`); + } + lines.push(""); + + lines.push("── CI ──"); + if (brief.checks.length === 0) lines.push(" (no checks reported)"); + for (const group of brief.checks) { + const symbol = group.failing > 0 ? "✗" : group.pending > 0 ? "·" : "✓"; + lines.push( + ` ${symbol} ${group.workflow.padEnd(28, " ")} ${group.passing}/${group.total} pass` + + (group.failing ? `, ${group.failing} fail` : "") + + (group.pending ? `, ${group.pending} pending` : ""), + ); + } + lines.push(""); + + lines.push(`── PR body (preview, ${brief.bodyChars} chars total) ──`); + lines.push(brief.bodyPreview ? ` ${brief.bodyPreview}` : " (empty body)"); + + return lines.join("\n"); +} + +export type ViewOptions = { + json?: boolean; +}; + +export async function runView(num: number, options: ViewOptions): Promise { + if (!Number.isFinite(num) || num <= 0) { + throw new Error("view requires a positive PR number, e.g. tools-pr view 1180"); + } + const view = await fetchView(num); + const brief = buildBrief(num, view); + if (options.json) { + process.stdout.write(`${JSON.stringify(brief, null, 2)}\n`); + return; + } + process.stdout.write(`${formatBrief(brief)}\n`); +} diff --git a/tools/pr/templates/agent-review.md b/tools/pr/templates/agent-review.md new file mode 100644 index 000000000..572463d22 --- /dev/null +++ b/tools/pr/templates/agent-review.md @@ -0,0 +1,108 @@ + +@{AUTHOR} — quick check-in: this has been waiting on your follow-up for about {AWAITING_HUMAN}. Still planning to land it? Happy to help if you're stuck on anything specific; if priorities have shifted, just say so and we'll close it cleanly. + +(Surfaced via the `awaiting-author-response-24h` maintainer tooling tag.) diff --git a/tools/pr/templates/duplicate-title-ask.md b/tools/pr/templates/duplicate-title-ask.md new file mode 100644 index 000000000..7fdf10eb7 --- /dev/null +++ b/tools/pr/templates/duplicate-title-ask.md @@ -0,0 +1,36 @@ + +@{AUTHOR} — flagging this one because the open queue currently has two PRs from you with byte-for-byte identical titles (`{TITLE}`): + +- this PR (#{THIS_NUM}, branch `{THIS_BRANCH}`, {THIS_COMMITS} commits, +{THIS_PLUS} −{THIS_MINUS}) +- #{OTHER_NUM} (branch `{OTHER_BRANCH}`, {OTHER_COMMITS} commits, +{OTHER_PLUS} −{OTHER_MINUS}) + +Could you confirm which one you'd like reviewers to land? If #{OTHER_NUM} is meant to supersede this one, feel free to close this in favor of it (or let me know and I'll close it). If they're intentionally covering different cases, a one-line note here clarifying the split would help reviewers track them. + +(Surfaced by maintainer tooling: `duplicate-title` tag on same-author byte-for-byte titles.) diff --git a/tools/pr/templates/examples/agent-review-1009-changes-requested.md b/tools/pr/templates/examples/agent-review-1009-changes-requested.md new file mode 100644 index 000000000..a58afda5d --- /dev/null +++ b/tools/pr/templates/examples/agent-review-1009-changes-requested.md @@ -0,0 +1,53 @@ + + +# Agent review · PR #1009 + +## Identification + +- Title: `feat(daemon): load user design systems from ~/.open-design/design-systems` +- Author: mathd (external) +- Status: OPEN · **CHANGES_REQUESTED** · DIRTY (no push since reviews) +- Labels: `size/M`, `risk/medium`, `type/feature` +- Lane: `default` (no forbidden surfaces, no public seams touched) +- Diff: +213 / −12 across 5 files + +## Prior reviewer findings (still applicable) + +Two human MEMBER reviews on file, both still unaddressed in the current head: + +- **@lefarcen** (`CHANGES_REQUESTED`): "one critical path traversal vulnerability plus several edge-case gaps". `readDesignSystem(root, id)` and the new `readDesignSystemFromAny(bundledRoot, userRoot, id)` (`apps/daemon/src/design-systems.ts:54-71`) both do `path.join(root, id, 'DESIGN.md')` with no validation that `id` stays inside `root`. An `id` like `../../../etc/passwd` escapes the design-systems directory if reachable from user input. +- **@mrcfps** (`COMMENTED`): `OD_USER_STATE_DIR` override is non-deterministic for tests/programmatic callers, and `userDesignSystemsDir()` falling back to `~/.open-design/design-systems` causes isolated finalize runs to silently read host-local design systems. + +## Findings + +### Test isolation risk + +`apps/daemon/tests/design-systems.test.ts` adds 104 lines. If those cases don't explicitly set `OD_USER_STATE_DIR` to a tmpdir, they hit the developer's real `~/.open-design/design-systems` and become non-reproducible across machines — same shape as @mrcfps's host-leak concern but in test scope. + +### Severity vs label + +`risk/medium` likely undersells the path-traversal severity from finding above. Worth re-labeling `risk/high` once author addresses the traversal, since the fix path itself is security-sensitive. + +## Suggestions + +- Path-traversal fix should validate `id` against a strict slug regex (e.g. `/^[a-z0-9-]+$/`, matching the existing `non-ascii-slug` lane convention) **at the call site closest to user input**, not deep inside `readDesignSystem` — preserves the low-level "trust the caller" contract while moving validation to the boundary. +- Pick one isolation strategy and apply it consistently: either every test sets `OD_USER_STATE_DIR=`, or the daemon takes a `userStateDir` config knob whose default is the home-dir fallback. Mixing the two leaves footguns. +- Consider splitting the path-traversal fix into a security-only PR ahead of the feature — the traversal is exploitable as soon as `id` reaches the function from a user-facing route, regardless of whether the user-overlay feature ships. + +## Validation expected (after author addresses CHANGES_REQUESTED items) + +```bash +pnpm guard +pnpm typecheck +pnpm --filter @open-design/daemon typecheck +pnpm --filter @open-design/daemon test +``` + +Test re-runs should include a path-traversal regression (`readDesignSystem(root, '../etc/passwd')` returns `null` or throws) and an `OD_USER_STATE_DIR=` isolation case. diff --git a/tools/pr/templates/examples/agent-review-1037-scope-expansion.md b/tools/pr/templates/examples/agent-review-1037-scope-expansion.md new file mode 100644 index 000000000..a235a3f1a --- /dev/null +++ b/tools/pr/templates/examples/agent-review-1037-scope-expansion.md @@ -0,0 +1,56 @@ + + +# Agent review · PR #1037 + +## Identification + +- Title: `fix(daemon): follow symlinked skill and design-system dirs` +- Author: desmond-rai (external) +- Status: OPEN · REVIEW_REQUIRED · DIRTY · **draft** +- Labels: `size/S`, `risk/high`, `type/bugfix` +- Lane: `default` (no forbidden surfaces, no public seams touched) +- Diff: +899 / −3 across 4 files + +## Findings + +### Scope expansion vs declared intent + +The PR title declares a symlink-follow fix. The diff actually contains three distinct changes: + +| Change | Files | Lines | Matches PR title? | +|---|---|---|---| +| Symlink-follow helper + call sites | `apps/daemon/src/design-systems.ts`, `apps/daemon/src/skills.ts` | ~50 | ✓ | +| Chat-run inactivity timeout `2m → 20m` | `apps/daemon/src/server.ts:1819-1824` | 3 | ✗ unrelated | +| Hardcoded `renderReturningAiShowcase` route + 837-line implementation | `apps/daemon/src/design-system-showcase.ts:107-128, 549-1402` | 853 | ✗ unrelated, vendor-specific | + +`code-review-guidelines.md §3 Out of scope` lists "Piggybacks unrelated cleanup, formatting, dependency churn, migrations, or feature work onto a focused fix" as a block-or-split condition. + +### Product-relevance test + +`renderReturningAiShowcase` is triggered by `id === 'returning-ai' || /ReturningAI|CFD-brokerage/i.test(raw)` and renders a vendor-specific design-system showcase. The PR body doesn't name an Open Design capability this branch validates. `code-review-guidelines.md §1` lists "customer vertical / marketing experiment / unrelated rendering demo / arbitrary product page" as out-of-scope unless first-party and motivated, and this matches. + +### Symlink fix itself is clean + +`entryIsDirectoryFollowingLinks` (duplicated 14 lines in `design-systems.ts:33-46` and `skills.ts:94-107`, well-commented) is the correct fix for `Dirent.isDirectory()` returning false on symlinks. Broken links fall through `try/catch` and are skipped — identical end-state to the previous filter. No regression test for the symlink-as-directory path. lefarcen's prior `COMMENTED` review at SHA `b86885c0` predates the scope-expanding commits. + +## Suggestions + +- Ask the author to split: land the symlink fix as a standalone PR (`design-systems.ts` + `skills.ts` + one regression test), and bring the timeout and vendor-specific showcase back as separately motivated PRs or drop them. +- The 2m → 20m chat timeout has real resource implications (10× idle window); if the author wants to keep it, it needs its own motivation. The vendor-specific showcase fails `§1` as written and is the more disposable of the two. + +## Validation expected (for the trimmed symlink-fix-only scope) + +```bash +pnpm guard +pnpm typecheck +pnpm --filter @open-design/daemon typecheck +pnpm --filter @open-design/daemon test +``` + +Plus a regression test under `apps/daemon/tests/` that creates a temp dir with a symlinked subdir and confirms `listSkills` / `listDesignSystems` discover it. diff --git a/tools/pr/templates/examples/agent-review-1149-clean-contract.md b/tools/pr/templates/examples/agent-review-1149-clean-contract.md new file mode 100644 index 000000000..18d25f7b4 --- /dev/null +++ b/tools/pr/templates/examples/agent-review-1149-clean-contract.md @@ -0,0 +1,55 @@ + + +# Agent review · PR #1149 + +## Identification + +- Title: `feat(daemon): deploy linked HTML pages together as a multi-page site on Vercel with clean URLs (#1077)` +- Author: ButterHost69 (external) +- Status: OPEN · REVIEW_REQUIRED · BLOCKED (branch-protection, not a code blocker) +- Labels: `size/M`, `risk/high`, `type/feature` +- Lane: `contract` (public seam: `packages/contracts`) +- Diff: +326 / −11 across 5 files + +## Findings + +### Contract surface is clean + +Two optional fields added to existing request DTOs (`DeployProjectFileRequest.linkedPages?: string[]` and `DeployPreflightRequest.linkedPages?: string[]`). `packages/contracts/src/api/projects.ts` stays pure TS (no new imports). Daemon consumer + 130-line `deploy.test.ts` land in the same PR. Meets `code-review-guidelines.md §4.2` (contract change lands with consumer + tests, additive only, no migration window needed). + +### BFS implementation + +`collectLinkedHtmlPages` discovers `` links to local HTML; the iterative `while (linkedPageIdx < linkedPages.length)` loop in `apps/daemon/src/deploy.ts:44-102` handles transitive chains correctly and is cycle-safe via `visited.has(page.path)`. The `page.deployedName === 'index.html'` guard at lines 81-85 explicitly throws `DeployError(400)` to prevent overwriting the entry page — defensive and well-placed. `vercel.json` injection at lines 127-135 is scoped to Vercel deploys only and respects user-supplied `vercel.json`. + +### Loose ends + +- `apps/web/src/providers/registry.ts` 1-line change is unmotivated by the PR body — worth confirming it's required by the feature, not a stale touch from a rebase. +- `collectLinkedHtmlPages` (lines 1466-1487) resolves paths via `resolveReferencedPath` (out of diff). Whether `../` escapes are rejected depends on that helper — given the `risk/high` label and the new caller paths exercising it, a path-traversal regression test would be appropriate. + +## Suggestions + +- Add a hard cap on transitively discovered HTML pages (e.g. > 50 → `DeployError`). The BFS is currently unbounded; a malformed project with a wide link graph could blow up a deploy. +- Document the `vercel.json` `cleanUrls: true` injection in release notes — it changes routing semantics on the deployed site, and users with existing `.html`-URL expectations need to know. +- Add a path-traversal test alongside the new 130 lines (an `` case asserting `collectLinkedHtmlPages` rejects or normalizes). + +## Validation expected + +```bash +pnpm guard +pnpm typecheck +pnpm --filter @open-design/contracts typecheck +pnpm --filter @open-design/daemon typecheck +pnpm --filter @open-design/daemon test +pnpm --filter @open-design/daemon build +pnpm --filter @open-design/web typecheck +pnpm --filter @open-design/web test +pnpm --filter @open-design/web build +``` + +Plus a manual sandbox smoke: deploy a 3-page linked-HTML project to a test Vercel target, confirm clean-URL routing and that the entry-page index resolution still works. diff --git a/tools/pr/tsconfig.json b/tools/pr/tsconfig.json new file mode 100644 index 000000000..c070f469d --- /dev/null +++ b/tools/pr/tsconfig.json @@ -0,0 +1,20 @@ +{ + "compilerOptions": { + "allowSyntheticDefaultImports": true, + "esModuleInterop": true, + "exactOptionalPropertyTypes": true, + "forceConsistentCasingInFileNames": true, + "isolatedModules": true, + "lib": ["ES2024"], + "module": "NodeNext", + "moduleResolution": "NodeNext", + "noEmit": true, + "noUncheckedIndexedAccess": true, + "resolveJsonModule": true, + "skipLibCheck": true, + "strict": true, + "target": "ES2024", + "types": ["node"] + }, + "include": ["src/**/*.ts", "tests/**/*.ts", "esbuild.config.mjs"] +}