diff --git a/docs/plans/archive/agent-code-review.md b/docs/plans/archive/agent-code-review.md new file mode 100644 index 000000000..eee6d7ac8 --- /dev/null +++ b/docs/plans/archive/agent-code-review.md @@ -0,0 +1,463 @@ +--- +$schema: https://poe-platform.github.io/poe-code/schemas/plans/pipeline.schema.json +kind: pipeline +version: 1 + +setup: + prompt: | + Prepare the `poe-code` working tree before implementing this pipeline. + + Check for existing local changes and do not overwrite or revert them. If the working tree is clean, pull the latest changes for the currently checked-out branch with `git pull --ff-only` before starting task work. If local changes or a non-fast-forward update prevent pulling safely, report the condition clearly and preserve the existing worktree unchanged. + +teardown: + prompt: | + Finalize the completed `agent-code-review` pipeline work in `poe-code`. + + Run the relevant tests and validations, commit only files changed for this plan using a Conventional Commit message, and push the implementation on a branch suitable for review. Open a GitHub pull request targeting `main` for these changes, including a concise implementation summary and the validation commands/results in the pull request body. Do not include unrelated local changes in the commit or pull request. + +tasks: + - id: port-github-review-package + title: Port reusable GitHub review mechanics + prompt: | + Create a new workspace package `packages/github-review` that ports the GitHub PR/review mechanics from `/home/kjopek/automations/packages/agent-github-review-tools`. + + The package must own only GitHub mechanics, with no agent/profile/orchestrator concepts. Implement and export: + - `parseGitHubPullRequestRef(prUrl)` and `canonicalPullRequestUrl(prUrl)`. + - PR metadata fetch via `gh pr view`. + - PR diff fetch via `gh pr diff`. + - PR comments/reviews fetches needed by review agents. + - diff parsing that exposes right-side reviewable lines. + - inline comment validation against right-side diff lines. + - pull request review submission, pull request review comment edit, and delete via `gh api`. + + All GitHub authentication must be delegated to the GitHub CLI. Do not read `GITHUB_TOKEN` or `GH_TOKEN` directly; tests should use an injected fake command runner instead of calling real `gh`. + + Add `package.json`, `tsconfig.json`, `README.md`, source files, and unit tests. Use Node 22-compatible TypeScript. Keep the package independent from `agent-code-review`. + status: + implement: done + refactor: done + test: done + + - id: add-review-history-fetch + title: Add GitHub review-history fetching for profile ingest + prompt: | + Extend `packages/github-review` with paginated review-history fetching for profile ingest. + + Add an async API that accepts `{ username, repos, cwd?, since?, maxComments?, onRateLimit? }` and yields normalized comments authored by that GitHub username. Repositories are strings in `owner/name` form. + + Fetch these artifacts from GitHub using `gh api`, not HTML scraping: + - pull request review comments authored by the user; + - pull request review bodies authored by the user; + - pull request issue comments authored by the user only when the issue is a pull request. + + Normalize each item to include repo, PR number/title/url, author login, creation timestamp, kind (`review_comment`, `review_body`, or `pr_comment`), body, and path/line/diff hunk when available. + + Be conservative with GitHub rate limits. Parse pagination links and rate-limit headers such as `x-ratelimit-remaining` and `x-ratelimit-reset`. Stop with a clear resumable error or back off when remaining requests are low. Add tests with fake `gh api` responses covering pagination, rate headers, filtering, and partial results. + status: + implement: done + refactor: done + test: done + + - id: scaffold-agent-code-review-package + title: Scaffold agent-code-review package and config + prompt: | + Create a new workspace package `packages/agent-code-review` published internally as `agent-code-review`. + + The package must depend on `github-review`, `toolcraft`, `toolcraft-schema`, `@poe-code/poe-code-config`, and the existing poe-code spawn APIs as needed. It owns profile loading, prompt loading, review orchestration, YAML review state, CLI group, SDK exports, and `code-review agent-mcp`. + + Add a `codeReview` config scope using the existing `.poe-code/config.json` config system. Defaults: + - `agent`: omitted means use normal poe-code default agent resolution. + - `draftStore`: `.poe-code/code-review/reviews`. + - `humanGate.provider`: `none`. + + Do not support `AUTOMATIONS_*` environment variables. Do not require `OPENAI_API_KEY` for runtime review. Add package README and tests for config defaults and SDK input overriding config. + status: + implement: done + refactor: done + test: done + + - id: implement-install-profiles-prompts + title: Install repo-local profiles and role prompts + prompt: | + In `packages/agent-code-review`, implement repo-local profile and prompt assets. + + Profiles live under `.poe-code/code-review/profiles`. Runtime discovers every `*.md` profile there. If no repo profile files exist, expose a built-in short `generic` fallback profile. If repo profiles exist, do not automatically include `generic`. Unknown profile filters are hard errors. + + Role prompts live under `.poe-code/code-review/prompts`: + - `orchestrator.md` + - `subagent.md` + - `agent.md` + - `profile-synthesis.md` + + Runtime loads repo prompt files when present and uses built-in fallback prompts when files are missing. + + Add `poe-code code-review install [--cwd ] [--force]` through the package CLI group. It must create: + - `.poe-code/code-review/profiles/generic.md` + - `.poe-code/code-review/prompts/orchestrator.md` + - `.poe-code/code-review/prompts/subagent.md` + - `.poe-code/code-review/prompts/agent.md` + - `.poe-code/code-review/prompts/profile-synthesis.md` + + The install command must not overwrite existing files unless `--force` is passed. Add tests for profile discovery, prompt fallback, install idempotence, and forced overwrite. + status: + implement: done + refactor: done + test: done + + - id: implement-yaml-review-store + title: Store reviews as PR-scoped YAML files + prompt: | + In `packages/agent-code-review`, replace the old SQLite draft model with one YAML file per reviewed PR. + + Review files live under `.poe-code/code-review/reviews` by default and are named `__PR.yaml`, for example `acme_web_PR123.yaml`. Archive files move to `.poe-code/code-review/reviews/archive`. + + The YAML file must include: + - version, session id, PR URL, canonical PR ref, selected agent, selected profiles, state, timestamps; + - immutable `raw_reviews` per profile/subagent; + - `subagents` status records; + - one `merged_review` created by the orchestrator; + - append-only `orchestrator_actions` explaining actions such as dropped duplicate comments, merged same-line comments, or selected decision; + - `published` receipt after commit. + + Raw reviews are immutable after creation. The orchestrator may create/replace only `merged_review` and append `orchestrator_actions`. + + Implement atomic YAML writes by writing a temp file in the same directory and renaming. Commit must write the published receipt and move the full PR YAML file to `archive/`; if the archive target exists, append a timestamp suffix. Add tests for filename generation, round trips, immutable raw reviews, action appends, commit archive behavior, and archive collision handling. + status: + implement: done + refactor: done + test: done + + - id: implement-agent-mcp-role-tools + title: Implement role-filtered code-review agent MCP + prompt: | + In `packages/agent-code-review`, implement `poe-code code-review agent-mcp` as a standalone stdio MCP server for review agents. + + It must receive explicit process arguments, not environment variables: + - `--role agent|orchestrator|subagent` + - `--session ` + - `--actor ` + - `--cwd ` + - `--agent ` + - `--profiles ` when the run filters profiles + + Expose role-filtered tools: + - `agent` and `subagent`: `code_review_pr_view`, `code_review_pr_diff`, `code_review_pr_comments`, `code_review_create_draft`. + - `orchestrator`: all agent/subagent tools plus `code_review_profile_list`, `code_review_agent_spawn`, `code_review_agent_status`, `code_review_list_drafts`, `code_review_edit_inline_comment`, `code_review_delete_inline_comment`, `code_review_discard_draft`, and dry-run-only `code_review_commit_drafts`. + + Real GitHub publishing must not be available inside spawned agents. `code_review_commit_drafts` through MCP is dry-run only. + + Add tests that tool lists differ by role, context comes from args, subagents cannot access orchestrator-only tools, and MCP initialize/tools-list works. + status: + implement: done + refactor: done + test: done + + - id: implement-review-orchestration + title: Port review orchestration and subagent spawning + prompt: | + In `packages/agent-code-review`, port the multi-reviewer orchestration flow from `/home/kjopek/automations/packages/agent-github-review-tools`. + + `runCodeReview` must: + - resolve config, cwd, PR URL, session id, selected agent, and profile filters; + - fetch PR details and diff through `github-review`; + - read prior PR comments/reviews before drafting so agents avoid duplicate findings; + - load role prompts from `.poe-code/code-review/prompts` or built-in fallbacks; + - spawn an orchestrator with review MCP server config; + - let the orchestrator spawn subagents through `code_review_agent_spawn`; + - record immutable raw reviews per profile in the PR YAML file; + - have the orchestrator create exactly one `merged_review`; + - append `orchestrator_actions`; + - support `additionalFeedback` in the orchestrator prompt for external Slack/human-gate reruns. + + Use the existing poe-code `spawn` SDK. Do not shell out to `codex` directly. Subagent spawn accepts optional `agent`; when omitted, it inherits the run agent. Store the resolved agent per subagent in YAML. + + Add integration tests with fake `spawn` and fake `github-review` proving sessions, subagents, raw reviews, merged review, actions, and additional feedback behavior. + status: + implement: done + refactor: done + test: done + + - id: implement-commit-drafts + title: Commit merged review and archive YAML + prompt: | + In `packages/agent-code-review`, implement `commitCodeReviewDrafts` and CLI `poe-code code-review commit `. + + The command must read the PR YAML file from `.poe-code/code-review/reviews/__PR.yaml`, validate `merged_review.comments` against the current PR diff through `github-review`, and submit exactly one GitHub pull request review through `github-review`. + + `--dry-run` must print/return exactly what would be published without calling GitHub and without moving the YAML file. Non-dry-run commit must write a `published` receipt with GitHub review id/url, actor, session, and timestamp, then move the full PR YAML file to `.poe-code/code-review/reviews/archive`. If the archive path already exists, append a timestamp suffix. + + If there is no `merged_review`, fail clearly and do not call GitHub. If inline comments are invalid for the current diff, do not submit an invalid payload; surface the validation result clearly. + + Add unit and integration tests for dry-run, successful publish, missing merged review, invalid inline comments, published receipt, archive move, and archive collision. + status: + implement: done + refactor: done + test: done + + - id: implement-profile-ingest + title: Ingest GitHub review history into profiles + prompt: | + In `packages/agent-code-review`, implement `poe-code code-review ingest --repo [--repo ...] [--profile ] [--agent ] [--cwd ]`. + + Only support repeated `--repo owner/name` flags for multiple repositories. Do not add `--repos-file` or comma-separated repo parsing. + + The command must: + - call `github-review` review-history fetching for the GitHub username and repo list; + - write ingest artifacts under `.poe-code/code-review/ingest//`; + - write `source.yaml` with username, repos, fetch timestamp, pagination/rate-limit observations, and output profile path; + - write `comments.jsonl` with normalized comments; + - write `synthesis-prompt.md`; + - spawn the selected agent with a task to read `comments.jsonl` and directly write `.poe-code/code-review/profiles/.md`. + + Profile synthesis must use an agent spawn, not a direct LLM/OpenAI SDK call. The generated profile must be first-person, usable by runtime review prompts, and must not mention GitHub usernames, source URLs, or that it was generated. + + Add tests for CLI parsing, repeated repos, artifact writing, fake rate-limit partial progress, agent prompt content, and final profile path behavior. + status: + implement: done + refactor: done + test: done + + - id: add-code-review-schemas + title: Add schemas for config, profiles, prompts, and review YAML + prompt: | + Add explicit validation schemas for every persisted or user-provided code-review document. + + In `packages/agent-code-review`, define schemas using the repo's existing schema conventions for: + - `codeReview` config scope from `.poe-code/config.json`; + - profile frontmatter accepted in `.poe-code/code-review/profiles/*.md`; + - role prompt metadata if prompt files use frontmatter; + - PR review YAML files at `.poe-code/code-review/reviews/__PR.yaml`; + - ingest `source.yaml` files under `.poe-code/code-review/ingest//`. + + On read, validate and return actionable errors that include the file path and failing field. On write, emit only schema-valid YAML. Add tests with malformed YAML, unknown versions, missing required fields, invalid decisions, invalid comment line numbers, and invalid profile names. + + Do not silently coerce unsafe values such as path separators in profile names, actor names, session ids, repo names, or archive filenames. + status: + implement: done + refactor: done + test: done + + - id: harden-paths-and-filesystem-safety + title: Harden path handling and filesystem safety + prompt: | + Add filesystem safety protections to `packages/agent-code-review` and `packages/github-review`. + + Requirements: + - All paths derived from profile names, actors, sessions, repos, PR URLs, and archive names must be sanitized and verified to stay under the intended `.poe-code/code-review` subdirectory. + - Reject path traversal, absolute paths where a name is expected, hidden path segment tricks, empty names, and names that normalize to the same filename. + - Atomic writes must create parent directories, write temp files in the destination directory, fsync if the local helper pattern supports it, and rename. + - Archive moves must never overwrite an existing archive file; add timestamp suffixes on collisions. + - Test with spaces, Unicode-like punctuation if supported by existing repo conventions, path separators, `..`, duplicate normalized names, and concurrent-looking temp files. + + Keep tests in memory where practical using existing repo test patterns; do not create uncontrolled files in the real workspace. + status: + implement: done + refactor: done + test: done + + - id: build-fixture-harness + title: Build fixture harness for deterministic review tests + prompt: | + Build a deterministic test harness for code-review flows. + + Add fixtures under the relevant package test fixture directories, not under real user `.poe-code` state: + - a fake repo with `.poe-code/code-review/profiles`; + - a fake repo with no profiles to exercise `generic`; + - default prompt fixtures; + - GitHub PR metadata/diff/comment fixtures; + - review-history pagination fixtures including rate-limit headers; + - YAML review file fixtures for draft, merged, published, and archived states. + + Add fake implementations for: + - `github-review` command runner; + - agent spawn runner; + - clock/time; + - filesystem where the package pattern allows it. + + Use this harness in integration tests so `runCodeReview`, `commitCodeReviewDrafts`, and `ingestCodeReviewProfile` can run without network, real `gh`, real agents, or real repo state. + status: + implement: done + refactor: done + test: done + + - id: harden-github-api-rate-limits + title: Harden GitHub API pagination and rate-limit handling + prompt: | + Strengthen `packages/github-review` GitHub API behavior beyond basic fetching. + + Implement a reusable GitHub API response parser that extracts: + - JSON body; + - status code; + - pagination links; + - `x-ratelimit-limit`; + - `x-ratelimit-remaining`; + - `x-ratelimit-reset`; + - `retry-after` when present. + + Apply it to review-history ingest and any other paginated GitHub API reads. Handle secondary rate limits and abuse-limit style responses from `gh api` by returning a typed/resumable error with reset time and partial progress. + + Add tests for: + - multi-page success; + - missing link header; + - malformed header values; + - low remaining requests; + - retry-after responses; + - non-JSON API output; + - partial output already written to ingest artifacts. + status: + implement: done + refactor: done + test: done + + - id: add-prompt-contract-tests + title: Add prompt rendering and role contract tests + prompt: | + Add prompt rendering tests in `packages/agent-code-review`. + + Validate that built-in and installed prompt files receive the right variables for each role: + - orchestrator prompt includes PR URL/title, profile cards, primary profile body, prior comments/reviews summary, subagent instructions, diff summary, and optional `additionalFeedback`; + - subagent prompt includes exactly one profile body, PR URL/title, allowed MCP tools, and rules for avoiding duplicate findings; + - agent prompt covers direct non-orchestrated review mode; + - profile-synthesis prompt tells the spawned agent to read `comments.jsonl` and directly write `.poe-code/code-review/profiles/.md`. + + Add contract assertions that final review prompts forbid mentioning dry-run, fake-submit, orchestration, subagents, internal tool flow, source GitHub username, source URLs, or generation details in user-facing review/profile output. + + Include tests that repo prompt overrides are loaded and rendered, while missing prompt files fall back to built-ins. + status: + implement: done + refactor: done + test: done + + - id: add-failure-recovery-and-resume + title: Add failure recovery and resume semantics + prompt: | + Make `agent-code-review` resilient to interrupted runs and partial YAML state. + + Define and implement behavior for: + - stale pending/running subagents whose process is gone or whose log never appears; + - a run interrupted after some raw reviews were written but before `merged_review`; + - a commit interrupted after GitHub submit succeeds but before archive move; + - a commit interrupted after receipt write but before archive move; + - an archive file already existing; + - an ingest interrupted after partial `comments.jsonl` write. + + The implementation should prefer idempotent reruns. Re-running `code-review run ` should reuse valid immutable raw reviews where possible, refresh stale status, and create/replace only `merged_review` and append action logs. Re-running `commit ` after a successful published receipt should not submit a second GitHub review. + + Add tests for every recovery scenario using fake clocks, fake filesystem, fake GitHub submit, and fake agents. + status: + implement: done + refactor: done + test: done + + - id: add-security-and-env-audit-tests + title: Add security and environment isolation tests + prompt: | + Add explicit tests that the port removed automations-specific and secret-specific coupling. + + Tests must verify: + - no runtime code in `packages/agent-code-review` reads `AUTOMATIONS_*`; + - no runtime code requires `OPENAI_API_KEY`; + - GitHub submission code delegates auth to `gh` and does not read `GITHUB_TOKEN` or `GH_TOKEN` directly; + - Slack env vars such as `SLACK_BOT_TOKEN` are not referenced by `poe-code` code-review packages; + - spawned agent MCP context is passed through explicit args, not hidden env vars; + - subagent MCP role cannot access commit/publish tools; + - user-controlled text in profiles, prompts, comments, and ingest artifacts cannot change filesystem destinations. + + Implement these as unit tests or static grep-style tests using repo test conventions. Avoid brittle tests over generated dist files. + status: + implement: done + test: done + + - id: add-ci-style-e2e-test + title: Add CI-style end-to-end test path + prompt: | + Add a deterministic CI-style end-to-end test for the code-review workflow. + + The test should simulate a GitHub Actions usage without requiring actual GitHub or real agents: + - fixture repo checked out at a cwd; + - `.poe-code/config.json`; + - optional `.poe-code/code-review/profiles`; + - fake `gh` runner; + - fake agent spawn implementation; + - command invocation equivalent to `poe-code code-review run "$PR_URL"` followed by `poe-code code-review commit "$PR_URL"`. + + Verify: + - generic profile works when no profiles exist; + - installed repo prompts are used when present; + - review YAML is created at `.poe-code/code-review/reviews/__PR.yaml`; + - raw reviews are immutable; + - merged review is submitted exactly once; + - published receipt is written; + - YAML is moved to archive after commit; + - no network, real `gh`, or real model call happens. + status: + implement: done + refactor: done + test: done + + - id: wire-root-cli-and-exports + title: Wire code-review into poe-code CLI and exports + prompt: | + Wire `agent-code-review` into the root `poe-code` CLI and package exports. + + Add a `code-review` command group to the root CLI with: + - `install` + - `profiles` + - `ingest` + - `run` + - `drafts` + - `commit` + - `agent-mcp` + + The root CLI should be lightweight and only wire the package command group, consistent with repo guidance that package logic belongs in packages. Add root help entries as needed. + + Export SDK functions and types from `agent-code-review`, including profile loading, install, ingest, session creation, review run, draft read, commit, MCP server config creation, CLI group, and MCP runner. + + Add CLI help/smoke tests for the new root commands. Use Node 22. + status: + implement: done + refactor: done + test: done + + - id: document-and-qa-code-review + title: Document and QA code-review workflow + prompt: | + Add package READMEs and QA documentation for the new code review workflow. + + `packages/github-review/README.md` must explain SDK purpose, GitHub CLI authentication through `gh`, exported functions, and that tests use fake command runners. + + `packages/agent-code-review/README.md` must document: + - `.poe-code/code-review/profiles` + - `.poe-code/code-review/prompts` + - `.poe-code/code-review/reviews` + - `.poe-code/code-review/ingest` + - `poe-code code-review install` + - `profiles`, `ingest`, `run`, `drafts`, `commit`, and `agent-mcp` + - CI usage with explicit PR URL and `GH_TOKEN` for `gh` + - Slack/external human-gate integration through SDK `additionalFeedback`, with no Slack code in poe-code + + Add a markdown QA plan, not a script, covering: + - install in a fixture repo; + - profiles fallback; + - ingest with fake GitHub responses; + - run with fake agents; + - commit dry-run; + - commit against a disposable PR if credentials are available. + status: + implement: done + test: done +--- + +# Context + +This pipeline implements the feature planned in the previous generic `Agent Code Review` plan. It ports the existing review system from `/home/kjopek/automations/packages/agent-github-review-tools` into `poe-code` while changing the public shape: + +- Profiles: `.poe-code/code-review/profiles/*.md`. +- Prompts: `.poe-code/code-review/prompts/{orchestrator,subagent,agent,profile-synthesis}.md`. +- Review state: one YAML file per PR at `.poe-code/code-review/reviews/__PR.yaml`, archived after publish. +- Ingest artifacts: `.poe-code/code-review/ingest//`. +- Install command: `poe-code code-review install` creates `profiles/generic.md` and default prompts. +- GitHub auth: delegated to `gh`; code does not read `GITHUB_TOKEN` directly. +- Runtime review does not require `OPENAI_API_KEY`. +- Slack is external to `poe-code`; integrations use SDK `additionalFeedback` and `commitCodeReviewDrafts`. +- Ingest uses GitHub APIs through `gh api`, is rate-limit aware, and uses an agent to write the profile file directly. diff --git a/package-lock.json b/package-lock.json index c3534c8bc..0624f9d32 100644 --- a/package-lock.json +++ b/package-lock.json @@ -92,10 +92,12 @@ "@types/node": "^25.2.2", "@types/semver": "^7.7.1", "@vitest/coverage-v8": "^4.1.4", + "agent-code-review": "*", "auth-store": "*", "bats": "^1.13.0", "eslint": "^9.0.0", "eslint-config-prettier": "^10.1.8", + "github-review": "*", "globals": "^17.3.0", "husky": "^9.1.7", "mcp-oauth": "*", @@ -3154,6 +3156,10 @@ "acorn": "^6.0.0 || ^7.0.0 || ^8.0.0" } }, + "node_modules/agent-code-review": { + "resolved": "packages/agent-code-review", + "link": true + }, "node_modules/ajv": { "version": "8.20.0", "resolved": "https://registry.npmjs.org/ajv/-/ajv-8.20.0.tgz", @@ -4615,6 +4621,10 @@ "node": ">= 0.4" } }, + "node_modules/github-review": { + "resolved": "packages/github-review", + "link": true + }, "node_modules/glob": { "version": "11.1.0", "resolved": "https://registry.npmjs.org/glob/-/glob-11.1.0.tgz", @@ -7565,6 +7575,17 @@ "@poe-code/agent-spawn": "*" } }, + "packages/agent-code-review": { + "version": "0.1.0", + "dependencies": { + "@poe-code/agent-spawn": "*", + "@poe-code/poe-code-config": "*", + "github-review": "*", + "toolcraft": "*", + "toolcraft-schema": "*", + "yaml": "^2.8.2" + } + }, "packages/agent-defs": { "name": "@poe-code/agent-defs", "version": "0.0.1" @@ -7780,6 +7801,9 @@ "name": "@poe-code/file-lock", "version": "0.0.1" }, + "packages/github-review": { + "version": "0.1.0" + }, "packages/github-workflows": { "name": "@poe-code/github-workflows", "version": "0.0.1", diff --git a/package.json b/package.json index 8e82d8ca2..10f0bc4b4 100644 --- a/package.json +++ b/package.json @@ -147,6 +147,8 @@ "@poe-code/agent-script": "*", "@poe-code/agent-skill-config": "*", "@poe-code/agent-spawn": "*", + "agent-code-review": "*", + "github-review": "*", "@poe-code/braintrust": "*", "@poe-code/cached-resource": "*", "@poe-code/config-extends": "*", diff --git a/packages/agent-code-review/README.md b/packages/agent-code-review/README.md new file mode 100644 index 000000000..c6aa4043f --- /dev/null +++ b/packages/agent-code-review/README.md @@ -0,0 +1,105 @@ +# `agent-code-review` + +Internal package for agent-assisted GitHub code review. It owns code-review configuration, SDK run-option resolution, Markdown profile and prompt loading, YAML review state, the Toolcraft CLI group, and the spawned-agent MCP process. + +## Repo-local assets + +- `.poe-code/code-review/profiles` holds reviewer profile Markdown files. Profiles are read from `*.md`; when none exist, runtime exposes the built-in `generic` profile only. +- `.poe-code/code-review/prompts` holds `{orchestrator,subagent,agent,profile-synthesis}.md`. Each missing role prompt falls back to its built-in prompt. +- `.poe-code/code-review/reviews` is the default YAML draft-state store. Published review state moves into its `archive` subdirectory. +- `.poe-code/code-review/ingest` records ingest evidence under `/{source.yaml,comments.jsonl,synthesis-prompt.md}` before a synthesized profile is written. +- Profile Markdown may start with YAML frontmatter `version: 1` and `name: `; prompt Markdown may start with `version: 1` and `role: `. +- Install starter assets with `poe-code code-review install --cwd `; pass `--force` to overwrite existing files. + +## Configuration + +Configuration uses the normal `.poe-code/config.json` hierarchy under the `codeReview` scope: + +```json +{ + "codeReview": { + "agent": "codex", + "draftStore": ".poe-code/code-review/reviews", + "humanGate": { "provider": "none" } + } +} +``` + +- `agent` is optional. When omitted, agent selection remains with normal `poe-code` default-agent resolution. +- `draftStore` defaults to `.poe-code/code-review/reviews` and contains one YAML state file per PR, named `__PR.yaml`. +- Published review files are moved to `.poe-code/code-review/reviews/archive`; archive filename collisions receive a timestamp suffix. +- Review YAML and ingest `source.yaml` documents use `version: 1`; malformed documents report the path and failing field on read. +- `humanGate.provider` defaults to `none`. + +## Environment + +The package defines no environment variables of its own. Runtime review does not require `OPENAI_API_KEY`; only `gh` CLI authentication and a configured `poe-code` agent are needed. + +## SDK + +- `loadCodeReviewConfig` resolves project and user configuration. +- `resolveCodeReviewRunOptions` merges explicit SDK input over configured defaults and leaves `agent` absent when normal `poe-code` default-agent resolution should apply. +- `loadCodeReviewProfile` and `loadCodeReviewPrompt` load non-empty Markdown assets. +- `parseCodeReviewState` and `serializeCodeReviewState` own YAML state interchange. +- `CodeReviewYamlStore` starts PR runs, archives prior rerun state, adds immutable raw reviews, permits exactly one merged review, appends orchestrator actions, and archives files after publishing. +- `readCodeReviewDraft` returns the merged YAML draft for a pull request when one exists. +- `commitCodeReviewDrafts` validates the final merged inline comments against the current PR diff, publishes one GitHub review, records the publishing receipt, and archives its YAML state. +- `createCodeReviewAgentMcpConfig` creates an argument-bound MCP configuration passed to spawned review agents; it passes `--role`, `--session`, `--actor`, `--cwd`, `--draft-store`, `--agent`, and optional `--profiles` explicitly. +- `runCodeReview` resolves runtime config, session and agent selection; fetches PR metadata, diff, and prior review activity through `github-review`; includes optional external feedback in the orchestrator prompt; initializes YAML state; and spawns the orchestrator through the `poe-code` SDK. +- `ingestCodeReviewProfile` fetches normalized review-history records through `github-review`, records `.poe-code/code-review/ingest//{source.yaml,comments.jsonl,synthesis-prompt.md}`, and spawns the selected agent to write `.poe-code/code-review/profiles/.md` directly. +- The orchestrator can call `code_review_agent_spawn` for profile reviewers. Each subagent may override `agent`; omitted overrides inherit the run agent, and each resolved agent is persisted in YAML. + +## Recovery and resume + +- Re-running `code-review run ` resumes an active YAML file: `merged_review` is cleared for regeneration, immutable `raw_reviews` from the prior run are discarded so the new PR diff is reanalyzed, and a `resumed_run` orchestrator action is appended. +- On resume, prior raw reviews and subagent statuses are cleared so each selected profile reruns against the latest pull request diff. +- Publication records `publication_started` before submitting to GitHub. If a process dies before a receipt can be persisted, rerunning `commit ` refuses to risk a duplicate review; a known failed submit records `publication_failed` and permits a retry. +- If the published receipt was persisted before interruption, rerunning `commit ` archives that receipt without making another GitHub submission. Existing archive names are preserved and new archives use timestamped suffixes on collision. +- Ingest writes `comments.jsonl`, `source.yaml`, and the synthesis prompt atomically. A rerun replaces an interrupted partial `comments.jsonl` artifact rather than appending to corrupt evidence. + +## CLI + +The workflow uses these commands: + +- `install`: run `poe-code code-review install [--cwd ] [--force]` to install repo-local starter profiles and prompts. +- `profiles`: run `poe-code code-review profiles [--cwd ]` to list available reviewer profiles. Add or edit `.poe-code/code-review/profiles/*.md`, or rely on the built-in `generic` fallback when the directory contains no profiles. +- `ingest`: run `poe-code code-review ingest --repo [--repo ...] [--profile ] [--agent ] [--cwd ]` to synthesize a profile from authored GitHub review history; repeat `--repo` for each source repository. +- `run`: run `poe-code code-review run [--cwd ] [--agent ] [--profiles ...] [--additional-feedback ]` to fetch the PR, launch review agents, and create a merged YAML draft without publishing it. +- `drafts`: run `poe-code code-review drafts [--cwd ] [--draft-store ]` to read the active YAML draft. +- `commit`: run `poe-code code-review commit [--cwd ] [--draft-store ] [--actor ] [--dry-run]` to validate and publish the final merged review. `--dry-run` returns only the payload that would be submitted and leaves YAML state active. +- `agent-mcp`: run `poe-code code-review agent-mcp --role --session --actor --cwd [--draft-store ] --agent [--profiles ]` to start the stdio MCP process intended for spawned review agents. + +- Agents and subagents receive PR read tools plus `code_review_create_draft` only. +- Orchestrators additionally receive profile/spawn/status and draft-management tools. +- `code_review_commit_drafts` is MCP dry-run preview only; spawned agents never receive a GitHub publishing tool. + +## CI usage + +Pass the full GitHub pull request URL explicitly in CI; do not rely on local branch inference. `github-review` invokes `gh`, so expose `GH_TOKEN` to the job with permissions required to read the PR and, only for a publishing `commit`, submit a review: + +```sh +GH_TOKEN="$GH_TOKEN" poe-code code-review run \ + "https://github.com/acme/widgets/pull/123" \ + --cwd "$GITHUB_WORKSPACE" + +GH_TOKEN="$GH_TOKEN" poe-code code-review commit \ + "https://github.com/acme/widgets/pull/123" \ + --cwd "$GITHUB_WORKSPACE" \ + --actor github-actions +``` + +Use `commit --dry-run` before enabling publication in a pipeline that is still being configured. + +## External human gates + +Slack approval, ticketing, or any other human-gate integration belongs outside `poe-code`. An embedding service can collect feedback and rerun the SDK with `additionalFeedback`; `runCodeReview` includes that text in the orchestrator context for the next draft: + +```ts +await runCodeReview({ + prUrl: "https://github.com/acme/widgets/pull/123", + cwd: process.cwd(), + additionalFeedback: "Please verify rollback coverage before approval.", +}); +``` + +There is intentionally no Slack client, webhook handler, reaction polling, or Slack-specific publication logic in `poe-code` or this package. diff --git a/packages/agent-code-review/package.json b/packages/agent-code-review/package.json new file mode 100644 index 000000000..968dfcccf --- /dev/null +++ b/packages/agent-code-review/package.json @@ -0,0 +1,38 @@ +{ + "name": "agent-code-review", + "version": "0.1.0", + "private": true, + "type": "module", + "main": "dist/index.js", + "types": "dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + }, + "./cli": { + "types": "./dist/cli.d.ts", + "import": "./dist/cli.js" + }, + "./mcp": { + "types": "./dist/mcp.d.ts", + "import": "./dist/mcp.js" + } + }, + "scripts": { + "build": "rm -rf dist && tsc", + "test": "cd ../.. && vitest run packages/agent-code-review/src", + "test:unit": "cd ../.. && vitest run packages/agent-code-review/src" + }, + "files": [ + "dist" + ], + "dependencies": { + "@poe-code/agent-spawn": "*", + "@poe-code/poe-code-config": "*", + "github-review": "*", + "toolcraft": "*", + "toolcraft-schema": "*", + "yaml": "^2.8.2" + } +} diff --git a/packages/agent-code-review/src/assets.ts b/packages/agent-code-review/src/assets.ts new file mode 100644 index 000000000..eae0b6182 --- /dev/null +++ b/packages/agent-code-review/src/assets.ts @@ -0,0 +1,420 @@ +import { randomUUID } from "node:crypto"; +import { constants } from "node:fs"; +import { + link, + lstat, + mkdir, + open, + readdir, + rename, + stat, + unlink +} from "node:fs/promises"; +import { basename, dirname, join, relative, resolve, sep } from "node:path"; +import { + parseCodeReviewProfileMarkdown, + parseCodeReviewPromptMarkdown, + requireSafeDocumentSegment +} from "./document-schemas.js"; + +export type CodeReviewAssetReader = (filePath: string, encoding: BufferEncoding) => Promise; + +export const CODE_REVIEW_PROMPT_ROLES = [ + "orchestrator", + "subagent", + "agent", + "profile-synthesis" +] as const; + +export type CodeReviewPromptRole = (typeof CODE_REVIEW_PROMPT_ROLES)[number]; + +export interface CodeReviewProfile { + name: string; + content: string; + filePath?: string; + source: "repo" | "built-in"; +} + +export interface CodeReviewInstallResult { + created: string[]; + overwritten: string[]; + skipped: string[]; +} + +export const CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT = + "In user-facing review/profile output, do not mention dry-run, fake-submit, orchestration, subagents, internal tool flow, source GitHub usernames, source URLs, or generation details."; + +export const BUILT_IN_GENERIC_PROFILE = `# Generic review profile + +Focus on correctness, regressions, security, and missing tests. Report only concrete, actionable findings. +`; + +export const BUILT_IN_CODE_REVIEW_PROMPTS: Record = { + orchestrator: `Review this pull request and return actionable findings only. Use the available reviewer profiles where they help identify concrete issues. +`, + subagent: `Review the assigned pull request changes using the supplied reviewer profile. Return only concrete, actionable findings. +`, + agent: `Review the requested pull request directly without orchestrating other agents. Read the pull request context with the available tools, then create exactly one review draft with concrete, actionable findings only. + +${CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT} +`, + "profile-synthesis": `Synthesize a concise code-review profile from the supplied review evidence, emphasizing actionable review priorities and style. +` +}; + +function requireUserFacingOutputContract(prompt: string): string { + if (prompt.includes(CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT)) { + return prompt; + } + return `${prompt.trimEnd()}\n\n${CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT}\n`; +} + +const INSTALL_ASSETS: ReadonlyArray = [ + ["profiles/generic.md", BUILT_IN_GENERIC_PROFILE], + ["prompts/orchestrator.md", BUILT_IN_CODE_REVIEW_PROMPTS.orchestrator], + ["prompts/subagent.md", BUILT_IN_CODE_REVIEW_PROMPTS.subagent], + ["prompts/agent.md", BUILT_IN_CODE_REVIEW_PROMPTS.agent], + ["prompts/profile-synthesis.md", BUILT_IN_CODE_REVIEW_PROMPTS["profile-synthesis"]] +]; + +function requireMarkdownBody(content: string, filePath: string): string { + if (content.trim().length === 0) { + throw new Error(`Code review asset is empty: ${filePath}`); + } + return content; +} + +export function codeReviewAssetsDirectory(cwd: string): string { + return join(cwd, ".poe-code", "code-review"); +} + +export async function loadCodeReviewProfile( + filePath: string, + reader?: CodeReviewAssetReader +): Promise { + const parsed = parseCodeReviewProfileMarkdown( + await (reader ? reader(filePath, "utf8") : readRegularAssetFile(filePath)), + filePath + ); + return requireMarkdownBody(parsed.content, filePath); +} + +export async function loadCodeReviewPrompt( + filePath: string, + reader?: CodeReviewAssetReader +): Promise { + const parsed = parseCodeReviewPromptMarkdown( + await (reader ? reader(filePath, "utf8") : readRegularAssetFile(filePath)), + filePath + ); + return requireMarkdownBody(parsed.content, filePath); +} + +export async function discoverCodeReviewProfiles(input: { + cwd: string; + filters?: readonly string[]; +}): Promise { + const profilesDirectory = join(codeReviewAssetsDirectory(input.cwd), "profiles"); + await assertContainedAssetDirectoryOrMissing(resolve(input.cwd), profilesDirectory); + let profileFileNames: string[]; + try { + const profileEntries = await readdir(profilesDirectory, { + withFileTypes: true + }); + for (const entry of profileEntries) { + if (entry.name.endsWith(".md") && !entry.isFile()) { + throw invalidInstallTargetError(join(profilesDirectory, entry.name)); + } + } + profileFileNames = profileEntries + .filter((entry) => entry.isFile() && entry.name.endsWith(".md")) + .map((entry) => entry.name) + .sort(); + } catch (error) { + if (!isMissingFileError(error)) { + throw error; + } + profileFileNames = []; + } + + const availableNames = profileFileNames.map((fileName) => + requireSafeDocumentSegment( + profileNameFromFile(fileName), + `${join(profilesDirectory, fileName)}: filename` + ) + ); + const normalizedNames = new Set(); + for (const name of availableNames) { + const normalized = name.normalize("NFKC").toLowerCase(); + if (normalizedNames.has(normalized)) { + throw new Error(`Code review profile filenames normalize to the same name: ${name}`); + } + normalizedNames.add(normalized); + } + if (availableNames.length === 0) { + validateProfileFilters(input.filters, ["generic"]); + return [ + { + name: "generic", + content: BUILT_IN_GENERIC_PROFILE, + source: "built-in" + } + ]; + } + + validateProfileFilters(input.filters, availableNames); + const filterSet = input.filters?.length ? new Set(input.filters) : undefined; + return Promise.all( + profileFileNames + .filter((fileName) => !filterSet || filterSet.has(profileNameFromFile(fileName))) + .map(async (fileName) => { + const filePath = join(profilesDirectory, fileName); + return { + name: profileNameFromFile(fileName), + content: await loadCodeReviewProfile(filePath), + filePath, + source: "repo" as const + }; + }) + ); +} + +export async function loadCodeReviewRolePrompt(input: { + cwd: string; + role: CodeReviewPromptRole; +}): Promise { + const cwd = resolve(input.cwd); + const filePath = join(codeReviewAssetsDirectory(cwd), "prompts", `${input.role}.md`); + try { + await assertContainedAssetDirectoryOrMissing(cwd, dirname(filePath)); + const parsed = parseCodeReviewPromptMarkdown( + await readRegularAssetFile(filePath), + filePath, + input.role + ); + const prompt = requireMarkdownBody(parsed.content, filePath); + return input.role === "agent" ? requireUserFacingOutputContract(prompt) : prompt; + } catch (error) { + if (isMissingFileError(error)) { + const prompt = BUILT_IN_CODE_REVIEW_PROMPTS[input.role]; + return input.role === "agent" ? requireUserFacingOutputContract(prompt) : prompt; + } + throw error; + } +} + +export async function installCodeReviewAssets(input: { + cwd: string; + force?: boolean; +}): Promise { + const cwd = resolve(input.cwd); + const assetsDirectory = codeReviewAssetsDirectory(cwd); + const result: CodeReviewInstallResult = { + created: [], + overwritten: [], + skipped: [] + }; + for (const [relativePath, content] of INSTALL_ASSETS) { + const filePath = join(assetsDirectory, relativePath); + await ensureContainedDirectory(cwd, dirname(filePath)); + if (!input.force) { + const installed = await createAssetUnlessPresent(filePath, content); + (installed ? result.created : result.skipped).push(filePath); + continue; + } + const exists = await assertInstallTargetIsFileOrMissing(filePath); + await overwriteAssetAtomically(filePath, content); + (exists ? result.overwritten : result.created).push(filePath); + } + return result; +} + +async function ensureContainedDirectory(cwd: string, targetDirectory: string) { + const pathFromCwd = relative(cwd, targetDirectory); + if (pathFromCwd.startsWith("..") || pathFromCwd.startsWith(sep)) { + throw new Error(`Code review asset directory escapes repository: ${targetDirectory}`); + } + await mkdir(cwd, { recursive: true }); + if (!(await stat(cwd)).isDirectory()) { + throw new Error(`Code review repository path is not a directory: ${cwd}`); + } + let currentDirectory = cwd; + for (const segment of pathFromCwd.split(sep).filter(Boolean)) { + currentDirectory = join(currentDirectory, segment); + try { + await mkdir(currentDirectory); + } catch (error) { + if (!isAlreadyExistsError(error)) { + throw error; + } + } + const status = await lstat(currentDirectory); + if (!status.isDirectory()) { + throw new Error( + `Code review asset directory is not a regular directory: ${currentDirectory}` + ); + } + } +} + +async function assertContainedAssetDirectoryOrMissing( + cwd: string, + targetDirectory: string +): Promise { + const pathFromCwd = relative(cwd, targetDirectory); + if (pathFromCwd.startsWith("..") || pathFromCwd.startsWith(sep)) { + throw new Error(`Code review asset directory escapes repository: ${targetDirectory}`); + } + let currentDirectory = cwd; + for (const segment of pathFromCwd.split(sep).filter(Boolean)) { + currentDirectory = join(currentDirectory, segment); + let status: Awaited>; + try { + status = await lstat(currentDirectory); + } catch (error) { + if (isMissingFileError(error)) { + return; + } + throw error; + } + if (!status.isDirectory()) { + throw new Error( + `Code review asset directory is not a regular directory: ${currentDirectory}` + ); + } + } +} + +function profileNameFromFile(fileName: string): string { + return basename(fileName, ".md"); +} + +function validateProfileFilters( + filters: readonly string[] | undefined, + availableNames: readonly string[] +): void { + if (!filters?.length) { + return; + } + const validatedFilters = filters.map((profile) => + requireSafeDocumentSegment(profile, "Code review profile filter") + ); + const available = new Set(availableNames); + const unknown = validatedFilters.filter((profile) => !available.has(profile)); + if (unknown.length > 0) { + throw new Error( + `Unknown code review profile filter(s): ${unknown.join(", ")}. Available profiles: ${availableNames.join(", ")}.` + ); + } +} + +async function createAssetUnlessPresent(filePath: string, content: string): Promise { + for (;;) { + const temporaryPath = join(dirname(filePath), `.${basename(filePath)}.${randomUUID()}.tmp`); + let temporary: Awaited> | undefined; + try { + temporary = await open( + temporaryPath, + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY + ); + try { + await temporary.writeFile(content, "utf8"); + await temporary.sync(); + } finally { + await temporary.close(); + temporary = undefined; + } + await link(temporaryPath, filePath); + await unlink(temporaryPath); + await syncDirectory(dirname(filePath)); + return true; + } catch (error) { + await temporary?.close().catch(() => undefined); + await unlink(temporaryPath).catch(() => undefined); + if (!isAlreadyExistsError(error)) { + throw error; + } + try { + const status = await lstat(filePath); + if (!status.isFile()) { + throw invalidInstallTargetError(filePath); + } + return false; + } catch (statusError) { + if (isMissingFileError(statusError)) { + continue; + } + throw statusError; + } + } + } +} + +async function assertInstallTargetIsFileOrMissing(filePath: string): Promise { + try { + const status = await lstat(filePath); + if (!status.isFile()) { + throw invalidInstallTargetError(filePath); + } + return true; + } catch (error) { + if (isMissingFileError(error)) { + return false; + } + throw error; + } +} + +async function overwriteAssetAtomically(filePath: string, content: string): Promise { + const temporaryPath = join(dirname(filePath), `.${basename(filePath)}.${randomUUID()}.tmp`); + let temporary: Awaited> | undefined; + try { + temporary = await open( + temporaryPath, + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY + ); + await temporary.writeFile(content, "utf8"); + await temporary.sync(); + await temporary.close(); + temporary = undefined; + await rename(temporaryPath, filePath); + await syncDirectory(dirname(filePath)); + } catch (error) { + await temporary?.close().catch(() => undefined); + await unlink(temporaryPath).catch(() => undefined); + throw error; + } +} + +async function syncDirectory(directory: string): Promise { + const parent = await open(directory, constants.O_RDONLY); + try { + await parent.sync(); + } finally { + await parent.close(); + } +} + +async function readRegularAssetFile(filePath: string): Promise { + const handle = await open(filePath, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + try { + if (!(await handle.stat()).isFile()) { + throw invalidInstallTargetError(filePath); + } + return await handle.readFile("utf8"); + } finally { + await handle.close(); + } +} + +function invalidInstallTargetError(filePath: string): Error { + return new Error(`Code review asset path is not a regular file: ${filePath}`); +} + +function isAlreadyExistsError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "EEXIST"; +} + +function isMissingFileError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "ENOENT"; +} diff --git a/packages/agent-code-review/src/cli.test.ts b/packages/agent-code-review/src/cli.test.ts new file mode 100644 index 000000000..e2d1ad066 --- /dev/null +++ b/packages/agent-code-review/src/cli.test.ts @@ -0,0 +1,28 @@ +import { describe, expect, it } from "vitest"; +import { codeReviewGroup, readCodeReviewDraftCommand } from "./cli.js"; + +describe("code-review command group", () => { + it("exposes the root command surface", () => { + expect(codeReviewGroup.children.map(({ name }) => name).sort()).toEqual([ + "agent-mcp", + "commit", + "drafts", + "ingest", + "install", + "profiles", + "run" + ]); + }); + + it("reports a missing requested draft instead of a successful empty result", async () => { + await expect( + readCodeReviewDraftCommand.handler({ + params: { + prUrl: "https://github.com/acme/repo/pull/404", + cwd: "/repo" + } + } as never) + ).rejects.toThrow("No active code review draft found"); + }); + +}); diff --git a/packages/agent-code-review/src/cli.ts b/packages/agent-code-review/src/cli.ts new file mode 100644 index 000000000..c5a152629 --- /dev/null +++ b/packages/agent-code-review/src/cli.ts @@ -0,0 +1,233 @@ +import { S, defineCommand, defineGroup } from "toolcraft"; +import { discoverCodeReviewProfiles, installCodeReviewAssets } from "./assets.js"; +import { + type CodeReviewCommitResult, + type CodeReviewPublicationPayload, + type CommitCodeReviewDraftsInput, + commitCodeReviewDrafts +} from "./commit.js"; +import { ingestCodeReviewProfile } from "./ingest.js"; +import { parseCodeReviewAgentMcpArgs, runCodeReviewAgentMcp } from "./mcp.js"; +import { + type CodeReviewOrchestrationInput, + type CodeReviewResult, + runCodeReview +} from "./review.js"; +import { readCodeReviewDraft } from "./review-store.js"; + +type RunCodeReviewHandler = (input: CodeReviewOrchestrationInput) => Promise; +type CommitCodeReviewDraftsHandler = ( + input: CommitCodeReviewDraftsInput +) => Promise; + +const agentMcpCommand = defineCommand({ + name: "agent-mcp", + description: "Run the stdio MCP server used by code review agents.", + params: S.Object({ + role: S.Enum(["agent", "orchestrator", "subagent"] as const, { + description: "Review-agent role controlling exposed MCP tools." + }), + session: S.String({ description: "Code-review session id." }), + actor: S.String({ description: "Actor writing drafts in this process." }), + cwd: S.String({ description: "Repository working directory." }), + draftStore: S.Optional(S.String({ description: "Absolute YAML review state directory." })), + agent: S.String({ description: "Poe Code agent used for subagents." }), + profiles: S.Optional(S.String({ description: "Comma-separated allowed profile names." })) + }), + scope: ["cli"], + handler: async ({ params }) => + runCodeReviewAgentMcp( + parseCodeReviewAgentMcpArgs([ + "--role", + params.role, + "--session", + params.session, + "--actor", + params.actor, + "--cwd", + params.cwd, + ...(params.draftStore ? ["--draft-store", params.draftStore] : []), + "--agent", + params.agent, + ...(params.profiles ? ["--profiles", params.profiles] : []) + ]) + ) +}); + +export const installCodeReviewAssetsCommand = defineCommand({ + name: "install", + description: "Install repo-local code review profiles and prompts.", + params: S.Object({ + cwd: S.Optional(S.String({ description: "Repository root directory." })), + force: S.Optional( + S.Boolean({ + description: "Overwrite existing profile and prompt files." + }) + ) + }), + scope: ["cli"], + handler: async ({ params }) => + installCodeReviewAssets({ + cwd: params.cwd?.trim() || process.cwd(), + force: Boolean(params.force) + }) +}); + +export const listCodeReviewProfilesCommand = defineCommand({ + name: "profiles", + description: "List repo-local code review profiles.", + params: S.Object({ + cwd: S.Optional(S.String({ description: "Repository root directory." })) + }), + scope: ["cli"], + handler: async ({ params }) => + (await discoverCodeReviewProfiles({ cwd: params.cwd?.trim() || process.cwd() })).map( + ({ name, source, filePath }) => ({ + name, + source, + ...(filePath ? { filePath } : {}) + }) + ) +}); + +export const readCodeReviewDraftCommand = defineCommand({ + name: "drafts", + description: "Read the current YAML draft for a pull request.", + positional: ["prUrl"], + params: S.Object({ + prUrl: S.String({ description: "GitHub pull request URL." }), + cwd: S.Optional(S.String({ description: "Repository root directory." })), + draftStore: S.Optional(S.String({ description: "YAML review state directory." })) + }), + scope: ["cli"], + handler: async ({ params }) => { + const draft = await readCodeReviewDraft({ + prUrl: params.prUrl, + cwd: params.cwd?.trim() || process.cwd(), + ...(params.draftStore ? { draftStore: params.draftStore } : {}) + }); + if (draft === undefined) { + throw new Error(`No active code review draft found for ${params.prUrl}.`); + } + return draft; + } +}); + +function createRunCodeReviewCommand(run: RunCodeReviewHandler = (input) => runCodeReview(input)) { + return defineCommand({ + name: "run", + description: "Run an agent-assisted GitHub pull request review.", + positional: ["prUrl"], + params: S.Object({ + prUrl: S.String({ description: "GitHub pull request URL." }), + cwd: S.Optional(S.String({ description: "Repository root directory." })), + agent: S.Optional(S.String({ description: "Poe Code review agent." })), + draftStore: S.Optional(S.String({ description: "YAML review state directory." })), + profilePath: S.Optional( + S.String({ description: "Explicit reviewer profile Markdown path." }) + ), + promptPath: S.Optional( + S.String({ + description: "Explicit orchestrator prompt Markdown path." + }) + ), + profiles: S.Optional(S.Array(S.String(), { description: "Reviewer profile filter." })), + additionalFeedback: S.Optional(S.String({ description: "Additional rerun feedback." })) + }), + scope: ["cli"], + handler: async ({ params }) => + run({ + prUrl: params.prUrl, + cwd: params.cwd?.trim() || process.cwd(), + ...(params.agent ? { agent: params.agent } : {}), + ...(params.draftStore ? { draftStore: params.draftStore } : {}), + ...(params.profilePath ? { profilePath: params.profilePath } : {}), + ...(params.promptPath ? { promptPath: params.promptPath } : {}), + ...(params.profiles ? { profiles: params.profiles } : {}), + ...(params.additionalFeedback ? { additionalFeedback: params.additionalFeedback } : {}) + }) + }); +} + +export const runCodeReviewCommand = createRunCodeReviewCommand(); + +function createCommitCodeReviewDraftsCommand( + commit: CommitCodeReviewDraftsHandler = (input) => commitCodeReviewDrafts(input) +) { + return defineCommand({ + name: "commit", + description: "Validate and publish a merged code review draft to GitHub.", + positional: ["prUrl"], + params: S.Object({ + prUrl: S.String({ description: "GitHub pull request URL." }), + cwd: S.Optional(S.String({ description: "Repository root directory." })), + draftStore: S.Optional(S.String({ description: "YAML review state directory." })), + dryRun: S.Optional( + S.Boolean({ + description: "Preview the validated GitHub review payload only." + }) + ), + actor: S.Optional(S.String({ description: "Publishing actor receipt name." })) + }), + scope: ["cli"], + handler: async ({ params }) => + commit({ + prUrl: params.prUrl, + cwd: params.cwd?.trim() || process.cwd(), + ...(params.draftStore ? { draftStore: params.draftStore } : {}), + ...(params.actor ? { actor: params.actor } : {}), + dryRun: Boolean(params.dryRun) + }) + }); +} + +export const commitCodeReviewDraftsCommand = createCommitCodeReviewDraftsCommand(); + +export const ingestCodeReviewProfileCommand = defineCommand({ + name: "ingest", + description: "Build a runtime reviewer profile from GitHub review history.", + positional: ["githubUsername"], + params: S.Object({ + githubUsername: S.String({ description: "GitHub username to ingest." }), + repo: S.Array(S.String(), { + description: "GitHub owner/name repository; repeat --repo for more." + }), + profile: S.Optional(S.String({ description: "Output reviewer profile name." })), + agent: S.Optional(S.String({ description: "Poe Code agent used for synthesis." })), + cwd: S.Optional(S.String({ description: "Repository root directory." })) + }), + scope: ["cli"], + handler: async ({ params }) => + ingestCodeReviewProfile({ + username: params.githubUsername, + repos: params.repo, + ...(params.profile ? { profile: params.profile } : {}), + ...(params.agent ? { agent: params.agent } : {}), + cwd: params.cwd?.trim() || process.cwd() + }) +}); + +export interface CodeReviewCliDependencies { + run?: RunCodeReviewHandler; + commit?: CommitCodeReviewDraftsHandler; +} + +export function createCodeReviewGroup(dependencies: CodeReviewCliDependencies = {}) { + return defineGroup({ + name: "code-review", + description: "Run agent-assisted GitHub pull request reviews.", + children: [ + agentMcpCommand, + installCodeReviewAssetsCommand, + listCodeReviewProfilesCommand, + dependencies.run ? createRunCodeReviewCommand(dependencies.run) : runCodeReviewCommand, + dependencies.commit + ? createCommitCodeReviewDraftsCommand(dependencies.commit) + : commitCodeReviewDraftsCommand, + ingestCodeReviewProfileCommand, + readCodeReviewDraftCommand + ] + }); +} + +export const codeReviewGroup = createCodeReviewGroup(); diff --git a/packages/agent-code-review/src/commit.ts b/packages/agent-code-review/src/commit.ts new file mode 100644 index 000000000..e6ead03fa --- /dev/null +++ b/packages/agent-code-review/src/commit.ts @@ -0,0 +1,151 @@ +import { resolve } from "node:path"; +import { + type PullRequestReviewCommentInput, + type PullRequestReviewDecision, + fetchPullRequestDiff, + parseReviewDiff, + submitPullRequestReview, + validateInlineComments +} from "github-review"; +import { requireSafeDocumentSegment } from "./document-schemas.js"; +import type { CodeReviewState } from "./review-state.js"; +import { CodeReviewYamlStore, resolveCodeReviewStoreDirectory } from "./review-store.js"; + +export interface CodeReviewPublicationPayload { + body: string; + event: PullRequestReviewDecision; + comments: Array; +} + +export interface CommitCodeReviewDraftsInput { + prUrl: string; + cwd?: string; + draftStore?: string; + dryRun?: boolean; + actor?: string; +} + +export interface CodeReviewCommitResult { + payload: CodeReviewPublicationPayload; + published: NonNullable; + archivePath: string; +} + +export interface CommitCodeReviewDraftsDependencies { + store?: CodeReviewYamlStore; + fetchDiff?: typeof fetchPullRequestDiff; + submitReview?: typeof submitPullRequestReview; +} + +export async function commitCodeReviewDrafts( + input: CommitCodeReviewDraftsInput & { dryRun: true }, + dependencies?: CommitCodeReviewDraftsDependencies +): Promise; +export async function commitCodeReviewDrafts( + input: CommitCodeReviewDraftsInput, + dependencies?: CommitCodeReviewDraftsDependencies +): Promise; +export async function commitCodeReviewDrafts( + input: CommitCodeReviewDraftsInput, + dependencies: CommitCodeReviewDraftsDependencies = {} +): Promise { + const cwd = resolve(input.cwd ?? process.cwd()); + const store = + dependencies.store ?? new CodeReviewYamlStore({ directory: resolveDraftStore(input, cwd) }); + if (!input.dryRun) { + const recovered = await store.resumePublished(input.prUrl); + if (recovered?.state.mergedReview && recovered.state.published) { + return { + payload: payloadFromPublishedState(recovered.state), + published: recovered.state.published, + archivePath: recovered.archivePath + }; + } + } + const state = await store.read(input.prUrl); + if (!state) { + throw new Error(`Code review not found for pull request: ${input.prUrl}`); + } + if (!state.mergedReview) { + throw new Error("Cannot publish code review: YAML state has no merged_review."); + } + const decision = requireDecision(state.mergedReview.decision); + const diff = await (dependencies.fetchDiff ?? fetchPullRequestDiff)(state.prUrl, { cwd }); + let comments: PullRequestReviewCommentInput[]; + try { + comments = validateInlineComments(state.mergedReview.comments, parseReviewDiff(diff)); + } catch (error) { + throw new Error( + `Cannot publish code review: merged_review comments are invalid for the current PR diff: ${error instanceof Error ? error.message : String(error)}` + ); + } + const payload: CodeReviewPublicationPayload = { + body: state.mergedReview.body, + event: decision, + comments: comments.map((comment) => ({ ...comment, side: "RIGHT" })) + }; + if (input.dryRun) { + return payload; + } + const committed = await store.publish(state.prUrl, async (activeState) => { + if (activeState.timestamps.updatedAt !== state.timestamps.updatedAt) { + throw new Error( + "Cannot publish code review: YAML state changed while preparing publication." + ); + } + const submission = (dependencies.submitReview ?? submitPullRequestReview)({ + prUrl: activeState.prUrl, + decision, + summary: payload.body, + comments, + cwd + }); + if (submission.id === null || submission.url === null) { + throw new Error( + "Cannot archive published code review: GitHub review response is missing its id or url." + ); + } + return { + receipt: { + actor: requireActor(input.actor), + sessionId: activeState.sessionId, + decision, + reviewId: submission.id, + reviewUrl: submission.url + }, + result: undefined + }; + }); + return { + payload, + published: committed.state.published as NonNullable, + archivePath: committed.archivePath + }; +} + +function payloadFromPublishedState(state: CodeReviewState): CodeReviewPublicationPayload { + const review = state.mergedReview as NonNullable; + return { + body: review.body, + event: requireDecision(review.decision), + comments: review.comments.map((comment) => ({ ...comment, side: "RIGHT" })) + }; +} + +function resolveDraftStore(input: CommitCodeReviewDraftsInput, cwd: string): string { + return resolveCodeReviewStoreDirectory(cwd, input.draftStore); +} + +function requireActor(actor: string | undefined): string { + return requireSafeDocumentSegment(actor ?? "cli", "Code review actor"); +} + +function requireDecision(decision: string | undefined): PullRequestReviewDecision { + if (decision === undefined) { + return "COMMENT"; + } + if (["APPROVE", "REQUEST_CHANGES", "COMMENT"].includes(decision)) { + return decision as PullRequestReviewDecision; + } + throw new Error(`Cannot publish code review: invalid merged_review decision: ${decision}`); +} diff --git a/packages/agent-code-review/src/config-scope.ts b/packages/agent-code-review/src/config-scope.ts new file mode 100644 index 000000000..cf33c4530 --- /dev/null +++ b/packages/agent-code-review/src/config-scope.ts @@ -0,0 +1,70 @@ +import { defineScope } from "@poe-code/poe-code-config"; + +export interface CodeReviewHumanGateConfig extends Record { + provider: "none"; +} + +function parseHumanGate(value: unknown): CodeReviewHumanGateConfig { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new Error("humanGate must be an object"); + } + + const config = value as Record; + for (const key of Object.keys(config)) { + if (key !== "provider") { + throw new Error(`humanGate.${key} is not supported`); + } + } + const provider = config.provider ?? "none"; + if (provider !== "none") { + throw new Error('humanGate.provider must be "none"'); + } + + return { provider }; +} + +export function parseCodeReviewConfigDocument(value: unknown): { + agent?: string; + draftStore?: string; + humanGate?: CodeReviewHumanGateConfig; +} { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new Error("codeReview must be an object"); + } + const config = value as Record; + for (const key of Object.keys(config)) { + if (!["agent", "draftStore", "humanGate"].includes(key)) { + throw new Error(`codeReview.${key} is not supported`); + } + } + if (config.agent !== undefined && typeof config.agent !== "string") { + throw new Error("codeReview.agent must be a string"); + } + if (config.draftStore !== undefined && typeof config.draftStore !== "string") { + throw new Error("codeReview.draftStore must be a string"); + } + return { + ...(config.agent === undefined ? {} : { agent: config.agent }), + ...(config.draftStore === undefined ? {} : { draftStore: config.draftStore }), + ...(config.humanGate === undefined ? {} : { humanGate: parseHumanGate(config.humanGate) }) + }; +} + +export const codeReviewConfigScope = defineScope("codeReview", { + agent: { + type: "string", + default: "", + doc: "Agent used for code review; empty uses normal poe-code default agent resolution." + }, + draftStore: { + type: "string", + default: ".poe-code/code-review/reviews", + doc: "Directory under .poe-code/code-review containing YAML code review state." + }, + humanGate: { + type: "json", + default: { provider: "none" } satisfies CodeReviewHumanGateConfig, + parse: parseHumanGate, + doc: "External human-gate configuration for code review runs." + } +}); diff --git a/packages/agent-code-review/src/config.test.ts b/packages/agent-code-review/src/config.test.ts new file mode 100644 index 000000000..668c0fe3f --- /dev/null +++ b/packages/agent-code-review/src/config.test.ts @@ -0,0 +1,74 @@ +import { createMockFs } from "@poe-code/config-mutations/testing"; +import { createConfigStore } from "@poe-code/poe-code-config"; +import { describe, expect, it } from "vitest"; +import { codeReviewConfigScope } from "./config-scope.js"; +import { loadCodeReviewConfig, resolveCodeReviewRunOptions } from "./config.js"; + +const homeConfigPath = "/home/test/.poe-code/config.json"; +const projectConfigPath = "/repo/.poe-code/config.json"; + +describe("codeReview config", () => { + it("uses code review defaults without introducing an agent override", async () => { + const fs = createMockFs(); + + await expect(loadCodeReviewConfig({ fs, filePath: homeConfigPath })).resolves.toEqual({ + draftStore: ".poe-code/code-review/reviews", + humanGate: { provider: "none" } + }); + }); + + it("resolves configured values through the shared config store", async () => { + const fs = createMockFs({ + [projectConfigPath]: JSON.stringify({ + codeReview: { + agent: "codex", + draftStore: ".reviews", + humanGate: { provider: "none" } + } + }) + }); + const store = createConfigStore({ + fs, + filePath: homeConfigPath, + projectFilePath: projectConfigPath + }); + + await expect(store.scope(codeReviewConfigScope).getAll()).resolves.toEqual({ + agent: "codex", + draftStore: ".reviews", + humanGate: { provider: "none" } + }); + }); + + it("lets SDK run input override project config", async () => { + const fs = createMockFs({ + [projectConfigPath]: JSON.stringify({ + codeReview: { + agent: "claude-code", + draftStore: ".poe-code/code-review/from-config", + humanGate: { provider: "none" } + } + }) + }); + + await expect( + resolveCodeReviewRunOptions( + { + prUrl: "https://github.com/poe-platform/poe-code/pull/42", + cwd: "/repo", + agent: "codex", + draftStore: ".poe-code/code-review/from-sdk", + additionalFeedback: "Please revisit the API boundary." + }, + { fs, filePath: homeConfigPath, projectFilePath: projectConfigPath } + ) + ).resolves.toEqual({ + prUrl: "https://github.com/poe-platform/poe-code/pull/42", + cwd: "/repo", + agent: "codex", + draftStore: ".poe-code/code-review/from-sdk", + humanGate: { provider: "none" }, + additionalFeedback: "Please revisit the API boundary." + }); + }); +}); diff --git a/packages/agent-code-review/src/config.ts b/packages/agent-code-review/src/config.ts new file mode 100644 index 000000000..b6d6e2022 --- /dev/null +++ b/packages/agent-code-review/src/config.ts @@ -0,0 +1,175 @@ +import { mkdir, readFile, readdir, stat, unlink, writeFile } from "node:fs/promises"; +import { homedir } from "node:os"; +import { + type ConfigStoreOptions, + createConfigStore, + defineScope, + resolveConfigPath, + resolveProjectConfigPath +} from "@poe-code/poe-code-config"; +import { + type CodeReviewHumanGateConfig, + codeReviewConfigScope, + parseCodeReviewConfigDocument +} from "./config-scope.js"; +import { resolveCodeReviewStoreDirectory } from "./review-store.js"; + +export interface CodeReviewConfig { + agent?: string; + draftStore: string; + humanGate: CodeReviewHumanGateConfig; +} + +export interface CodeReviewRunInput { + prUrl: string; + cwd: string; + sessionId?: string; + agent?: string; + draftStore?: string; + humanGate?: CodeReviewHumanGateConfig; + profilePath?: string; + promptPath?: string; + profiles?: string[]; + additionalFeedback?: string; +} + +export interface CodeReviewRunOptions extends CodeReviewRunInput { + draftStore: string; + humanGate: CodeReviewHumanGateConfig; +} + +const poeCoreAgentScope = defineScope("core", { + defaultAgent: { + type: "string", + default: "", + env: "POE_DEFAULT_AGENT", + doc: "Normal poe-code default agent resolution for code review runs." + } +}); + +const nativeConfigFs: ConfigStoreOptions["fs"] = { + readFile: (filePath, encoding) => readFile(filePath, encoding), + writeFile: (filePath, content, options) => writeFile(filePath, content, options), + mkdir: (filePath, options) => mkdir(filePath, options).then(() => undefined), + unlink, + stat: (filePath) => stat(filePath), + readdir +}; + +export async function loadCodeReviewConfig(options: ConfigStoreOptions): Promise { + await validatePersistedCodeReviewConfig(options); + const config = await createConfigStore(options).scope(codeReviewConfigScope).getAll(); + const agent = optionalNonEmptyString(config.agent); + const draftStore = requireNonEmptyString(config.draftStore, "codeReview.draftStore"); + return { + ...(agent ? { agent } : {}), + draftStore, + humanGate: config.humanGate + }; +} + +async function validatePersistedCodeReviewConfig(options: ConfigStoreOptions): Promise { + for (const filePath of [options.filePath, options.projectFilePath]) { + if (!filePath) continue; + let content: string; + try { + content = await options.fs.readFile(filePath, "utf8"); + } catch (error) { + if (isMissingFileError(error)) continue; + throw error; + } + let document: unknown; + try { + document = JSON.parse(content) as unknown; + } catch (error) { + throw new Error( + `${filePath}: invalid JSON: ${error instanceof Error ? error.message : String(error)}` + ); + } + if (typeof document !== "object" || document === null || Array.isArray(document)) { + throw new Error(`${filePath}: config must be an object.`); + } + const codeReview = (document as Record).codeReview; + if (codeReview === undefined) continue; + try { + parseCodeReviewConfigDocument(codeReview); + } catch (error) { + throw new Error(`${filePath}: ${error instanceof Error ? error.message : String(error)}`); + } + } +} + +export async function resolveCodeReviewRunOptions( + input: CodeReviewRunInput, + configOptions: ConfigStoreOptions +): Promise { + const config = await loadCodeReviewConfig(configOptions); + const agent = + input.agent === undefined ? config.agent : requireNonEmptyString(input.agent, "agent"); + const draftStore = + input.draftStore === undefined + ? config.draftStore + : requireNonEmptyString(input.draftStore, "draftStore"); + resolveCodeReviewStoreDirectory(input.cwd, draftStore); + return { + prUrl: requireNonEmptyString(input.prUrl, "prUrl"), + cwd: requireNonEmptyString(input.cwd, "cwd"), + ...(input.sessionId === undefined + ? {} + : { sessionId: requireNonEmptyString(input.sessionId, "sessionId") }), + ...(agent ? { agent } : {}), + draftStore, + humanGate: input.humanGate ?? config.humanGate, + ...(input.profilePath === undefined ? {} : { profilePath: input.profilePath }), + ...(input.promptPath === undefined ? {} : { promptPath: input.promptPath }), + ...(input.profiles === undefined ? {} : { profiles: input.profiles }), + ...(input.additionalFeedback === undefined + ? {} + : { additionalFeedback: input.additionalFeedback }) + }; +} + +export async function resolveCodeReviewRuntimeOptions( + input: CodeReviewRunInput +): Promise { + return resolveCodeReviewRunOptions(input, { + fs: nativeConfigFs, + filePath: resolveConfigPath(homedir()), + projectFilePath: resolveProjectConfigPath(input.cwd), + env: process.env + }); +} + +export async function loadDefaultPoeCodeAgent(input: { + cwd: string; + homeDir?: string; + env?: Record; + fs?: ConfigStoreOptions["fs"]; +}): Promise { + const agent = await createConfigStore({ + fs: input.fs ?? nativeConfigFs, + filePath: resolveConfigPath(input.homeDir ?? homedir()), + projectFilePath: resolveProjectConfigPath(input.cwd), + env: input.env ?? process.env + }) + .scope(poeCoreAgentScope) + .get("defaultAgent"); + return agent.trim() || undefined; +} + +function optionalNonEmptyString(value: string): string | undefined { + const normalized = value.trim(); + return normalized.length > 0 ? normalized : undefined; +} + +function requireNonEmptyString(value: string, field: string): string { + const normalized = value.trim(); + if (!normalized) { + throw new Error(`${field} must be a non-empty string.`); + } + return normalized; +} + +function isMissingFileError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "ENOENT"; +} diff --git a/packages/agent-code-review/src/document-schemas.ts b/packages/agent-code-review/src/document-schemas.ts new file mode 100644 index 000000000..00ce9a078 --- /dev/null +++ b/packages/agent-code-review/src/document-schemas.ts @@ -0,0 +1,338 @@ +import { basename } from "node:path"; +import { parse as load, stringify } from "yaml"; + +const CODE_REVIEW_PROMPT_ROLES = [ + "orchestrator", + "subagent", + "agent", + "profile-synthesis" +] as const; +type CodeReviewPromptRole = (typeof CODE_REVIEW_PROMPT_ROLES)[number]; + +const FRONTMATTER_RE = /^---\r?\n([\s\S]*?)\r?\n---(?:\r?\n|$)([\s\S]*)$/; +const SAFE_SEGMENT_RE = /^[A-Za-z0-9._-]+$/; +const SAFE_GITHUB_ACTOR_RE = /^[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})$/; + +export interface CodeReviewProfileMetadata { + version: 1; + name: string; +} + +export interface CodeReviewPromptMetadata { + version: 1; + role: CodeReviewPromptRole; +} + +export interface CodeReviewIngestSource { + version: 1; + username: string; + repos: string[]; + fetchedAt: string; + outputProfilePath: string; + pagination: { + partial: boolean; + commentsWritten: number; + resumeEndpoint?: string; + }; + rateLimit: null | { + repo: string; + limit: number | null; + remaining: number | null; + resetAt: string | null; + retryAfter: string | null; + partialResults: number; + reason: "low_remaining" | "primary" | "secondary"; + }; +} + +export function requireSafeDocumentSegment(value: unknown, field: string): string { + if ( + typeof value !== "string" || + value.trim() !== value || + value.normalize("NFKC") !== value || + !SAFE_SEGMENT_RE.test(value) || + value.startsWith(".") || + value === "." || + value === ".." + ) { + throw new Error(`${field} must be a safe path segment.`); + } + return value; +} + +export function requireGitHubActorName(value: unknown, field: string): string { + if (typeof value !== "string" || !SAFE_GITHUB_ACTOR_RE.test(value)) { + throw new Error(`${field} must be a safe GitHub actor name.`); + } + return value; +} + +export function parseCodeReviewProfileMarkdown( + content: string, + filePath: string +): { content: string; metadata?: CodeReviewProfileMetadata } { + const parsed = parseOptionalFrontmatter(content, filePath); + if (!parsed.frontmatter) { + return { content: parsed.body }; + } + const metadata = requireMapping(parsed.frontmatter, filePath, "frontmatter"); + requireOnlyFields(metadata, filePath, "frontmatter", ["version", "name"]); + requireVersion(metadata.version, filePath, "frontmatter.version"); + const name = requireSafeDocumentSegment(metadata.name, `${filePath}: frontmatter.name`); + if (basename(filePath, ".md") !== name) { + throw invalidField(filePath, "frontmatter.name", "must match the filename"); + } + return { content: parsed.body, metadata: { version: 1, name } }; +} + +export function parseCodeReviewPromptMarkdown( + content: string, + filePath: string, + role?: CodeReviewPromptRole +): { content: string; metadata?: CodeReviewPromptMetadata } { + const parsed = parseOptionalFrontmatter(content, filePath); + if (!parsed.frontmatter) { + return { content: parsed.body }; + } + const metadata = requireMapping(parsed.frontmatter, filePath, "frontmatter"); + requireOnlyFields(metadata, filePath, "frontmatter", ["version", "role"]); + requireVersion(metadata.version, filePath, "frontmatter.version"); + if (!CODE_REVIEW_PROMPT_ROLES.includes(metadata.role as CodeReviewPromptRole)) { + throw invalidField(filePath, "frontmatter.role", "is not a supported role"); + } + const promptRole = metadata.role as CodeReviewPromptRole; + if (role !== undefined && promptRole !== role) { + throw invalidField(filePath, "frontmatter.role", `must equal ${role}`); + } + return { content: parsed.body, metadata: { version: 1, role: promptRole } }; +} + +export function parseCodeReviewIngestSource( + content: string, + filePath = "code-review ingest source.yaml" +): CodeReviewIngestSource { + try { + const source = requireMapping(load(content), filePath, "document"); + requireOnlyFields(source, filePath, "document", [ + "version", + "username", + "repos", + "fetched_at", + "output_profile_path", + "pagination", + "rate_limit" + ]); + requireVersion(source.version, filePath, "version"); + const username = requireGitHubActorName(source.username, `${filePath}: username`); + if (!Array.isArray(source.repos) || source.repos.length === 0) { + throw invalidField(filePath, "repos", "must be a non-empty array"); + } + const repos = source.repos.map((repo, index) => requireRepo(repo, filePath, `repos[${index}]`)); + const fetchedAt = requireDate(source.fetched_at, filePath, "fetched_at"); + if (typeof source.output_profile_path !== "string" || !source.output_profile_path) { + throw invalidField(filePath, "output_profile_path", "must be a non-empty string"); + } + const pagination = requireMapping(source.pagination, filePath, "pagination"); + requireOnlyFields(pagination, filePath, "pagination", [ + "partial", + "comments_written", + "resume_endpoint" + ]); + if (typeof pagination.partial !== "boolean") { + throw invalidField(filePath, "pagination.partial", "must be a boolean"); + } + const commentsWritten = requireNonNegativeInteger( + pagination.comments_written, + filePath, + "pagination.comments_written" + ); + const resumeEndpoint = optionalString( + pagination.resume_endpoint, + filePath, + "pagination.resume_endpoint" + ); + let rateLimit: CodeReviewIngestSource["rateLimit"] = null; + if (source.rate_limit !== null) { + const rate = requireMapping(source.rate_limit, filePath, "rate_limit"); + requireOnlyFields(rate, filePath, "rate_limit", [ + "repo", + "limit", + "remaining", + "reset_at", + "retry_after", + "partial_results", + "reason" + ]); + const reason = optionalString(rate.reason, filePath, "rate_limit.reason"); + if ( + reason !== undefined && + !(["low_remaining", "primary", "secondary"] as const).includes( + reason as "low_remaining" | "primary" | "secondary" + ) + ) { + throw invalidField(filePath, "rate_limit.reason", "is not supported"); + } + rateLimit = { + repo: requireRepo(rate.repo, filePath, "rate_limit.repo"), + limit: + rate.limit === null || rate.limit === undefined + ? null + : requireNonNegativeInteger(rate.limit, filePath, "rate_limit.limit"), + remaining: + rate.remaining === null + ? null + : requireNonNegativeInteger(rate.remaining, filePath, "rate_limit.remaining"), + resetAt: + rate.reset_at === null + ? null + : requireDate(rate.reset_at, filePath, "rate_limit.reset_at"), + retryAfter: + rate.retry_after === null || rate.retry_after === undefined + ? null + : (optionalString(rate.retry_after, filePath, "rate_limit.retry_after") ?? null), + partialResults: requireNonNegativeInteger( + rate.partial_results, + filePath, + "rate_limit.partial_results" + ), + reason: (reason ?? "low_remaining") as "low_remaining" | "primary" | "secondary" + }; + } + return { + version: 1, + username, + repos, + fetchedAt, + outputProfilePath: source.output_profile_path, + pagination: { + partial: pagination.partial, + commentsWritten, + ...(resumeEndpoint === undefined ? {} : { resumeEndpoint }) + }, + rateLimit + }; + } catch (error) { + if (error instanceof Error && error.message.startsWith(`${filePath}:`)) { + throw error; + } + throw new Error(`${filePath}: invalid YAML: ${errorMessage(error)}`); + } +} + +export function serializeCodeReviewIngestSource( + source: CodeReviewIngestSource, + filePath = "code-review ingest source.yaml" +): string { + const content = stringify( + { + version: source.version, + username: source.username, + repos: source.repos, + fetched_at: source.fetchedAt, + pagination: { + partial: source.pagination.partial, + comments_written: source.pagination.commentsWritten, + ...(source.pagination.resumeEndpoint === undefined + ? {} + : { resume_endpoint: source.pagination.resumeEndpoint }) + }, + rate_limit: + source.rateLimit === null + ? null + : { + repo: source.rateLimit.repo, + limit: source.rateLimit.limit, + remaining: source.rateLimit.remaining, + reset_at: source.rateLimit.resetAt, + retry_after: source.rateLimit.retryAfter, + partial_results: source.rateLimit.partialResults, + reason: source.rateLimit.reason + }, + output_profile_path: source.outputProfilePath + }, + { aliasDuplicateObjects: false, lineWidth: 0 } + ); + parseCodeReviewIngestSource(content, filePath); + return content; +} + +function parseOptionalFrontmatter(content: string, filePath: string) { + const match = content.match(FRONTMATTER_RE); + if (!match) { + if (/^---\r?\n/.test(content)) { + throw new Error(`${filePath}: frontmatter is missing a closing --- delimiter.`); + } + return { body: content }; + } + try { + return { frontmatter: load(match[1]), body: match[2] }; + } catch (error) { + throw new Error(`${filePath}: invalid frontmatter YAML: ${errorMessage(error)}`); + } +} + +function requireMapping(value: unknown, filePath: string, field: string) { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw invalidField(filePath, field, "must be a YAML mapping"); + } + return value as Record; +} + +function requireVersion(value: unknown, filePath: string, field: string): void { + if (value !== 1) { + throw invalidField(filePath, field, "must equal 1"); + } +} + +function requireOnlyFields( + value: Record, + filePath: string, + field: string, + allowedFields: readonly string[] +): void { + const allowed = new Set(allowedFields); + for (const key of Object.keys(value)) { + if (!allowed.has(key)) { + throw invalidField(filePath, `${field}.${key}`, "is not supported"); + } + } +} + +function requireRepo(value: unknown, filePath: string, field: string): string { + if (typeof value !== "string" || !/^[A-Za-z0-9._-]+\/[A-Za-z0-9._-]+$/.test(value)) { + throw invalidField(filePath, field, "must be a safe owner/repository name"); + } + return value; +} + +function requireDate(value: unknown, filePath: string, field: string): string { + if (typeof value !== "string" || Number.isNaN(Date.parse(value))) { + throw invalidField(filePath, field, "must be a valid timestamp"); + } + return value; +} + +function requireNonNegativeInteger(value: unknown, filePath: string, field: string): number { + if (!Number.isSafeInteger(value) || (value as number) < 0) { + throw invalidField(filePath, field, "must be a non-negative integer"); + } + return value as number; +} + +function optionalString(value: unknown, filePath: string, field: string): string | undefined { + if (value === undefined) { + return undefined; + } + if (typeof value !== "string" || value.length === 0) { + throw invalidField(filePath, field, "must be a non-empty string"); + } + return value; +} + +function invalidField(filePath: string, field: string, reason: string): Error { + return new Error(`${filePath}: ${field} ${reason}.`); +} + +function errorMessage(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} diff --git a/packages/agent-code-review/src/index.ts b/packages/agent-code-review/src/index.ts new file mode 100644 index 000000000..29f51f7f1 --- /dev/null +++ b/packages/agent-code-review/src/index.ts @@ -0,0 +1,109 @@ +export { + codeReviewConfigScope, + parseCodeReviewConfigDocument, + type CodeReviewHumanGateConfig +} from "./config-scope.js"; +export { + loadCodeReviewConfig, + loadDefaultPoeCodeAgent, + resolveCodeReviewRuntimeOptions, + resolveCodeReviewRunOptions, + type CodeReviewConfig, + type CodeReviewRunInput, + type CodeReviewRunOptions +} from "./config.js"; +export { + BUILT_IN_CODE_REVIEW_PROMPTS, + BUILT_IN_GENERIC_PROFILE, + CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT, + CODE_REVIEW_PROMPT_ROLES, + codeReviewAssetsDirectory, + discoverCodeReviewProfiles, + installCodeReviewAssets, + loadCodeReviewRolePrompt, + loadCodeReviewProfile, + loadCodeReviewPrompt, + type CodeReviewAssetReader, + type CodeReviewInstallResult, + type CodeReviewProfile, + type CodeReviewPromptRole +} from "./assets.js"; +export { + parseCodeReviewState, + serializeCodeReviewState, + type CodeReviewDraft, + type CodeReviewInlineComment, + type CodeReviewOrchestratorAction, + type CodeReviewPrRef, + type CodeReviewPublishedReceipt, + type CodeReviewReviewState, + type CodeReviewState, + type CodeReviewSubagentStatus, + type CodeReviewTimestamps, + type CodeReviewDecision +} from "./review-state.js"; +export { + CODE_REVIEW_DIRECTORY, + CODE_REVIEW_ARCHIVE_DIRECTORY, + DEFAULT_CODE_REVIEW_REVIEWS_DIRECTORY, + CodeReviewYamlStore, + codeReviewFileName, + createCodeReviewState, + createCodeReviewState as createCodeReviewSession, + readCodeReviewDraft, + resolveCodeReviewStoreDirectory, + type ReadCodeReviewDraftInput, + type CodeReviewStoreOptions, + type CreateCodeReviewInput +} from "./review-store.js"; +export { + codeReviewGroup, + createCodeReviewGroup, + installCodeReviewAssetsCommand, + listCodeReviewProfilesCommand, + readCodeReviewDraftCommand, + type CodeReviewCliDependencies +} from "./cli.js"; +export { + DEFAULT_CODE_REVIEW_INGEST_DIRECTORY, + DEFAULT_CODE_REVIEW_PROFILES_DIRECTORY, + ingestCodeReviewProfile, + parseCodeReviewIngestArgs, + type CodeReviewIngestDependencies, + type CodeReviewIngestInput, + type CodeReviewIngestResult, + type NormalizedIngestComment +} from "./ingest.js"; +export { + parseCodeReviewIngestSource, + parseCodeReviewProfileMarkdown, + parseCodeReviewPromptMarkdown, + serializeCodeReviewIngestSource, + type CodeReviewIngestSource, + type CodeReviewProfileMetadata, + type CodeReviewPromptMetadata +} from "./document-schemas.js"; +export { + commitCodeReviewDrafts, + type CodeReviewCommitResult, + type CodeReviewPublicationPayload, + type CommitCodeReviewDraftsDependencies, + type CommitCodeReviewDraftsInput +} from "./commit.js"; +export { + CODE_REVIEW_AGENT_MCP_ROLES, + createCodeReviewAgentMcpGroup, + createCodeReviewAgentMcpConfig, + parseCodeReviewAgentMcpArgs, + runCodeReviewAgentMcp, + type CodeReviewAgentMcpContext, + type CodeReviewAgentMcpConfig, + type CodeReviewAgentMcpDependencies, + type CodeReviewAgentMcpRole +} from "./mcp.js"; +export { + runCodeReview, + type CodeReviewOrchestrationDependencies, + type CodeReviewOrchestrationInput, + type CodeReviewResult +} from "./review.js"; diff --git a/packages/agent-code-review/src/ingest.ts b/packages/agent-code-review/src/ingest.ts new file mode 100644 index 000000000..f6cb7f2ba --- /dev/null +++ b/packages/agent-code-review/src/ingest.ts @@ -0,0 +1,544 @@ +import { randomUUID } from "node:crypto"; +import { constants } from "node:fs"; +import type { Dirent } from "node:fs"; +import { lstat, mkdir, open, readdir, rename, rm, stat } from "node:fs/promises"; +import { basename, dirname, join, relative, resolve, sep } from "node:path"; +import { + GitHubRateLimitError, + type GitHubRateLimitStatus, + type ReviewHistoryComment, + fetchReviewHistory +} from "github-review"; +import { type SpawnOptions, type SpawnResult, spawn } from "@poe-code/agent-spawn"; +import { CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT, loadCodeReviewRolePrompt } from "./assets.js"; +import { loadDefaultPoeCodeAgent } from "./config.js"; +import { + parseCodeReviewProfileMarkdown, + requireGitHubActorName, + requireSafeDocumentSegment, + serializeCodeReviewIngestSource +} from "./document-schemas.js"; + +export const DEFAULT_CODE_REVIEW_INGEST_DIRECTORY = ".poe-code/code-review/ingest"; +export const DEFAULT_CODE_REVIEW_PROFILES_DIRECTORY = ".poe-code/code-review/profiles"; + +export interface CodeReviewIngestInput { + username: string; + repos: readonly string[]; + profile?: string; + agent?: string; + cwd: string; +} + +export function parseCodeReviewIngestArgs(args: string[]): CodeReviewIngestInput { + const [username, ...flags] = args; + const input: { + username: string; + repos: string[]; + profile?: string; + agent?: string; + cwd: string; + } = { + username: username ?? "", + repos: [], + cwd: process.cwd() + }; + for (let index = 0; index < flags.length; index += 1) { + const flag = flags[index]; + if (!flag?.startsWith("--")) { + throw new Error(`Unknown code-review ingest arg: ${flag}`); + } + const value = flags[index + 1]; + if (!value || value.startsWith("--")) { + throw new Error(`${flag} requires a value.`); + } + switch (flag) { + case "--repo": + if (value.includes(",")) { + throw new Error( + "Use repeated --repo owner/name flags; comma-separated repositories are not supported." + ); + } + input.repos.push(value); + break; + case "--profile": + input.profile = value; + break; + case "--agent": + input.agent = value; + break; + case "--cwd": + input.cwd = value; + break; + default: + throw new Error(`Unknown code-review ingest arg: ${flag}`); + } + index += 1; + } + return input; +} + +export interface NormalizedIngestComment { + repo: string; + pullRequestNumber: number; + pullRequestTitle: string; + createdAt: string; + kind: ReviewHistoryComment["kind"]; + body: string; + path?: string; + line?: number; + diffHunk?: string; +} + +export interface CodeReviewIngestResult { + profile: string; + profilePath: string; + artifactsDirectory: string; + sourcePath: string; + commentsPath: string; + promptPath: string; + commentCount: number; + partial: boolean; + spawnResult: SpawnResult; +} + +export interface CodeReviewIngestDependencies { + fetchHistory?: typeof fetchReviewHistory; + resolveAgent?: () => string | undefined | Promise; + loadPrompt?: (input: { cwd: string; role: "profile-synthesis" }) => Promise; + spawnAgent?: ( + agent: string, + prompt: string, + options: Omit + ) => Promise; + now?: () => Date; +} + +interface IngestObservation { + partial: boolean; + rateLimit?: GitHubRateLimitStatus; +} + +export async function ingestCodeReviewProfile( + input: CodeReviewIngestInput, + dependencies: CodeReviewIngestDependencies = {} +): Promise { + const cwd = resolve(requireNonEmpty(input.cwd, "cwd")); + const username = requireGitHubActorName(input.username, "Code-review github username"); + const repos = validateRepos(input.repos); + const profile = validateProfileName(input.profile ?? username); + const agent = + input.agent?.trim() || + (await (dependencies.resolveAgent + ? dependencies.resolveAgent() + : loadDefaultPoeCodeAgent({ cwd }))); + if (!agent?.trim()) { + throw new Error( + "No code-review agent resolved; pass --agent or configure the normal poe-code default agent." + ); + } + + const artifactsDirectory = join(cwd, DEFAULT_CODE_REVIEW_INGEST_DIRECTORY, profile); + const profilePath = join(cwd, DEFAULT_CODE_REVIEW_PROFILES_DIRECTORY, `${profile}.md`); + const sourcePath = join(artifactsDirectory, "source.yaml"); + const commentsPath = join(artifactsDirectory, "comments.jsonl"); + const promptPath = join(artifactsDirectory, "synthesis-prompt.md"); + const legacyGeneratedProfilePath = join(artifactsDirectory, "generated-profile.md"); + await ensureContainedDirectory(cwd, dirname(profilePath)); + await assertNoNormalizedProfileCollision(dirname(profilePath), profile); + await ensureContainedDirectory(cwd, artifactsDirectory); + await assertRegularFileOrMissing(profilePath); + const previousProfile = await readRegularFileOrMissing(profilePath); + + const comments: NormalizedIngestComment[] = []; + let observation: IngestObservation = { partial: false }; + try { + for await (const comment of (dependencies.fetchHistory ?? fetchReviewHistory)({ + username, + repos, + cwd + })) { + comments.push(normalizeComment(comment)); + } + } catch (error) { + if (!(error instanceof GitHubRateLimitError)) { + throw error; + } + observation = { partial: true, rateLimit: error.status }; + } + + await writeTextAtomically( + cwd, + commentsPath, + comments.map((comment) => JSON.stringify(comment)).join("\n") + (comments.length ? "\n" : "") + ); + await writeTextAtomically( + cwd, + sourcePath, + serializeSource({ + username, + repos, + fetchedAt: (dependencies.now ?? (() => new Date()))().toISOString(), + profilePath, + commentCount: comments.length, + observation + }) + ); + const synthesisPrompt = renderSynthesisPrompt({ + template: await (dependencies.loadPrompt ?? loadCodeReviewRolePrompt)({ + cwd, + role: "profile-synthesis" + }), + commentsPath, + profilePath, + partial: observation.partial + }); + await writeTextAtomically(cwd, promptPath, synthesisPrompt); + await removeStaleLegacyOutput(legacyGeneratedProfilePath); + await rm(profilePath, { force: true }); + let spawnResult: SpawnResult; + try { + spawnResult = await (dependencies.spawnAgent ?? spawnWithPoeCode)( + agent.trim(), + synthesisPrompt, + { cwd } + ); + if (spawnResult.exitCode !== 0) { + throw new Error( + `Code-review profile synthesis failed: ${spawnResult.stderr || `exit ${spawnResult.exitCode}`}` + ); + } + await validateGeneratedProfile(profilePath, profilePath, username); + } catch (error) { + await restoreProfileAfterFailedSynthesis(cwd, profilePath, previousProfile); + throw error; + } + return { + profile, + profilePath, + artifactsDirectory, + sourcePath, + commentsPath, + promptPath, + commentCount: comments.length, + partial: observation.partial, + spawnResult + }; +} + +function normalizeComment(comment: ReviewHistoryComment): NormalizedIngestComment { + return { + repo: comment.repo, + pullRequestNumber: comment.pullRequestNumber, + pullRequestTitle: comment.pullRequestTitle, + createdAt: comment.createdAt, + kind: comment.kind, + body: comment.body, + ...(comment.path ? { path: comment.path } : {}), + ...(comment.line === undefined ? {} : { line: comment.line }), + ...(comment.diffHunk ? { diffHunk: comment.diffHunk } : {}) + }; +} + +function renderSynthesisPrompt(input: { + template: string; + commentsPath: string; + profilePath: string; + partial: boolean; +}): string { + return `${input.template.trim()}\n\n# Profile synthesis task\n\nRead the normalized review evidence from \`${input.commentsPath}\` and directly write the completed Markdown profile to \`${input.profilePath}\`.\n\nRequirements:\n- Write in first person so the profile can be inserted directly into runtime code-review prompts.\n- Describe concrete review priorities, likely concerns, tone, and useful heuristics grounded in the evidence.\n- ${CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT}\n- Do not write analysis or summaries anywhere other than the requested profile file.\n${input.partial ? "- The fetched evidence is partial because API rate limits interrupted collection; be conservative about claims.\n" : ""}`; +} + +function serializeSource(input: { + username: string; + repos: readonly string[]; + fetchedAt: string; + profilePath: string; + commentCount: number; + observation: IngestObservation; +}): string { + return serializeCodeReviewIngestSource({ + version: 1, + username: input.username, + repos: [...input.repos], + fetchedAt: input.fetchedAt, + pagination: { + partial: input.observation.partial, + commentsWritten: input.commentCount, + ...(input.observation.rateLimit + ? { resumeEndpoint: input.observation.rateLimit.resumeEndpoint } + : {}) + }, + rateLimit: input.observation.rateLimit + ? { + repo: input.observation.rateLimit.repo, + limit: input.observation.rateLimit.limit, + remaining: input.observation.rateLimit.remaining, + resetAt: input.observation.rateLimit.resetAt?.toISOString() ?? null, + retryAfter: input.observation.rateLimit.retryAfter, + partialResults: input.observation.rateLimit.partialResults, + reason: input.observation.rateLimit.reason + } + : null, + outputProfilePath: input.profilePath + }); +} + +async function validateGeneratedProfile( + generatedProfilePath: string, + profilePath: string, + username: string +): Promise { + let profile: string; + try { + profile = await readRegularFile(generatedProfilePath); + } catch (error) { + throw new Error(`Profile synthesis did not write ${generatedProfilePath}`, { + cause: error + }); + } + if (!profile.trim()) { + throw new Error(`Profile synthesis wrote an empty profile: ${generatedProfilePath}`); + } + const body = parseCodeReviewProfileMarkdown(profile, profilePath).content; + if (!/\b(?:I|my|mine)\b/i.test(body)) { + throw new Error("Profile synthesis output must be written in first person."); + } + const prohibited = [ + new RegExp(`@${escapeRegExp(username)}\\b`, "i"), + /@[A-Za-z0-9](?:[A-Za-z0-9-]{0,38})\b/, + /https?:\/\//i, + /\bgithub\.com\b/i, + /\bgithub username\b/i, + /\b(?:generated profile|profile (?:was|is) generated)\b/i + ]; + for (const pattern of prohibited) { + if (pattern.test(body)) { + throw new Error("Profile synthesis output contains prohibited source attribution."); + } + } + return profile; +} + +function escapeRegExp(value: string): string { + return value.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); +} + +function validateRepos(values: readonly string[]): string[] { + if (values.length === 0) { + throw new Error("At least one --repo owner/name flag is required."); + } + return values.map((value) => { + const repo = value; + if ( + !repo + .split("/") + .every( + (part) => + /^[A-Za-z0-9_.-]+$/.test(part) && + part.normalize("NFKC") === part && + !part.startsWith(".") + ) || + repo.split("/").length !== 2 + ) { + throw new Error(`Invalid GitHub repository: ${repo}`); + } + return repo; + }); +} + +function validateProfileName(value: string): string { + const profile = value; + try { + requireSafeDocumentSegment(profile, "profile"); + } catch { + throw new Error(`Invalid code-review profile name: ${profile}`); + } + return profile; +} + +function requireNonEmpty(value: string, label: string): string { + const normalized = value.trim(); + if (!normalized) { + throw new Error(`Code-review ${label} is required.`); + } + return normalized; +} + +async function spawnWithPoeCode( + agent: string, + prompt: string, + options: Omit +): Promise { + return spawn(agent, { prompt, ...options }); +} + +async function ensureContainedDirectory(cwd: string, targetDirectory: string) { + const pathFromCwd = relative(cwd, targetDirectory); + if (pathFromCwd.startsWith("..") || pathFromCwd.startsWith(sep)) { + throw new Error(`Code-review ingest directory escapes repository: ${targetDirectory}`); + } + await mkdir(cwd, { recursive: true }); + const cwdStatus = await stat(cwd); + if (!cwdStatus.isDirectory()) { + throw new Error(`Code-review repository path is not a directory: ${cwd}`); + } + let currentDirectory = cwd; + for (const segment of pathFromCwd.split(sep).filter(Boolean)) { + currentDirectory = join(currentDirectory, segment); + try { + await mkdir(currentDirectory); + } catch (error) { + if (!isAlreadyExistsError(error)) { + throw error; + } + } + const status = await lstat(currentDirectory); + if (!status.isDirectory()) { + throw new Error( + `Code-review ingest directory is not a regular directory: ${currentDirectory}` + ); + } + } +} + +async function assertRegularFileOrMissing(filePath: string): Promise { + try { + const status = await lstat(filePath); + if (!status.isFile()) { + throw new Error(`Code-review ingest path is not a regular file: ${filePath}`); + } + } catch (error) { + if (!isMissingFileError(error)) { + throw error; + } + } +} + +async function writeTextAtomically(cwd: string, filePath: string, content: string): Promise { + await ensureContainedDirectory(cwd, dirname(filePath)); + const temporaryPath = join(dirname(filePath), `.${basename(filePath)}.${randomUUID()}.tmp`); + let temporary: Awaited> | undefined; + try { + temporary = await open( + temporaryPath, + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY + ); + await temporary.writeFile(content, "utf8"); + await temporary.sync(); + await temporary.close(); + temporary = undefined; + await rename(temporaryPath, filePath); + await syncDirectory(dirname(filePath)); + } catch (error) { + await temporary?.close().catch(() => undefined); + await rm(temporaryPath, { force: true }).catch(() => undefined); + throw error; + } +} + +async function assertNoNormalizedProfileCollision( + profilesDirectory: string, + profile: string +): Promise { + const normalizedProfile = profile.normalize("NFKC").toLowerCase(); + let entries: Dirent[]; + try { + entries = await readdir(profilesDirectory, { withFileTypes: true }); + } catch (error) { + if (isMissingFileError(error)) { + return; + } + throw error; + } + for (const entry of entries) { + if (!entry.name.endsWith(".md")) { + continue; + } + if (!entry.isFile()) { + throw new Error( + `Code-review ingest path is not a regular file: ${join(profilesDirectory, entry.name)}` + ); + } + const existing = entry.name.slice(0, -".md".length); + if (existing !== profile && existing.normalize("NFKC").toLowerCase() === normalizedProfile) { + throw new Error(`Code-review profile name collides with existing filename: ${profile}`); + } + } +} + +async function syncDirectory(directory: string): Promise { + const parent = await open(directory, constants.O_RDONLY); + try { + await parent.sync(); + } finally { + await parent.close(); + } +} + +async function readRegularFile(filePath: string): Promise { + const handle = await open(filePath, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + try { + if (!(await handle.stat()).isFile()) { + throw new Error(`Code-review ingest path is not a regular file: ${filePath}`); + } + return await handle.readFile("utf8"); + } finally { + await handle.close(); + } +} + +async function readRegularFileOrMissing(filePath: string): Promise { + try { + return await readRegularFile(filePath); + } catch (error) { + if (isMissingFileError(error)) { + return undefined; + } + throw error; + } +} + +async function restoreProfileAfterFailedSynthesis( + cwd: string, + profilePath: string, + previousProfile: string | undefined +): Promise { + if (previousProfile !== undefined) { + await writeTextAtomically(cwd, profilePath, previousProfile); + return; + } + try { + const status = await lstat(profilePath); + if (status.isDirectory()) { + throw new Error(`Code-review ingest path is not a regular file: ${profilePath}`); + } + await rm(profilePath, { force: true }); + } catch (error) { + if (!isMissingFileError(error)) { + throw error; + } + } +} + +async function removeStaleLegacyOutput(filePath: string): Promise { + try { + const status = await lstat(filePath); + if (status.isDirectory()) { + throw new Error(`Code-review ingest path is not a regular file: ${filePath}`); + } + await rm(filePath, { force: true }); + } catch (error) { + if (!isMissingFileError(error)) { + throw error; + } + } +} + +function isAlreadyExistsError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "EEXIST"; +} + +function isMissingFileError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "ENOENT"; +} diff --git a/packages/agent-code-review/src/mcp.test.ts b/packages/agent-code-review/src/mcp.test.ts new file mode 100644 index 000000000..2c614aaa8 --- /dev/null +++ b/packages/agent-code-review/src/mcp.test.ts @@ -0,0 +1,138 @@ +import { describe, expect, it, vi } from "vitest"; +import { createCodeReviewState } from "./review-store.js"; +import { createCodeReviewAgentMcpConfig, createCodeReviewAgentMcpGroup } from "./mcp.js"; + +describe("createCodeReviewAgentMcpConfig", () => { + it("launches the MCP server through the shipped root executable", () => { + expect( + createCodeReviewAgentMcpConfig({ + role: "agent", + session: "session-1", + actor: "reviewer", + cwd: "/repo", + agent: "codex" + }) + ).toEqual({ + transport: "stdio", + command: "poe-code", + args: [ + "code-review", + "agent-mcp", + "--role", + "agent", + "--session", + "session-1", + "--actor", + "reviewer", + "--cwd", + "/repo", + "--agent", + "codex" + ] + }); + }); + + it.each([ + ["audit logging", { appendFailure: new Error("Store unavailable") }, "Store unavailable"], + ["PR loading", { fetchFailure: new Error("GitHub unavailable") }, "GitHub unavailable"] + ])("marks %s failures as failed instead of leaving pending state", async (_label, failure, message) => { + const states: Array<{ status: string; error?: string }> = []; + const state = createCodeReviewState({ + sessionId: "session-1", + prUrl: "https://github.com/acme/repo/pull/1", + selectedAgent: "codex", + selectedProfiles: ["generic"] + }); + const group = createCodeReviewAgentMcpGroup( + { + role: "orchestrator", + session: "session-1", + actor: "orchestrator", + cwd: "/repo", + agent: "codex" + }, + { + store: { + read: vi.fn(async () => state), + addSubagent: vi.fn(async (_pr, _actor, status) => { + states.push(status); + return state; + }), + updateSubagent: vi.fn(async (_pr, _actor, status) => { + states.push(status); + return state; + }), + appendOrchestratorAction: vi.fn(async () => { + if ("appendFailure" in failure) throw failure.appendFailure; + return state; + }) + } as never, + fetchPr: vi.fn(async () => { + if ("fetchFailure" in failure) throw failure.fetchFailure; + return {}; + }) + } + ); + const spawnCommand = group.children.find(({ name }) => name === "code_review_agent_spawn"); + + expect(spawnCommand).toBeDefined(); + await expect( + spawnCommand?.handler({ + params: { pr: "https://github.com/acme/repo/pull/1", profile: "generic" } + } as never) + ).resolves.toEqual({ actor: "generic", agent: "codex", status: "pending" }); + await vi.waitFor(() => { + expect(states.at(-1)).toMatchObject({ status: "failed", error: message }); + }); + }); +}); + +describe("createCodeReviewAgentMcpGroup orchestrator tools", () => { + it("exposes local merged-draft edit, delete, and discard commands", async () => { + const state = createCodeReviewState({ + sessionId: "session-1", + prUrl: "https://github.com/acme/repo/pull/1", + selectedAgent: "codex", + selectedProfiles: ["generic"] + }); + const editedState = { + ...state, + state: "merged" as const, + mergedReview: { body: "Summary", comments: [{ path: "src/a.ts", line: 3, body: "edit" }] } + }; + const store = { + read: vi.fn(async () => editedState), + editMergedInlineComment: vi.fn(async () => editedState), + deleteMergedInlineComment: vi.fn(async () => editedState), + discardMergedReview: vi.fn(async () => state), + appendOrchestratorAction: vi.fn(async () => editedState) + }; + const group = createCodeReviewAgentMcpGroup( + { + role: "orchestrator", + session: "session-1", + actor: "orchestrator", + cwd: "/repo", + agent: "codex" + }, + { store: store as never } + ); + const command = (name: string) => group.children.find((child) => child.name === name); + + await command("code_review_edit_inline_comment")?.handler({ + params: { pr: state.prUrl, index: 0, path: "src/a.ts", line: 3, body: "edit" } + } as never); + await command("code_review_delete_inline_comment")?.handler({ + params: { pr: state.prUrl, index: 0 } + } as never); + await command("code_review_discard_draft")?.handler({ params: { pr: state.prUrl } } as never); + + expect(store.editMergedInlineComment).toHaveBeenCalledWith(state.prUrl, 0, { + path: "src/a.ts", + line: 3, + body: "edit" + }); + expect(store.deleteMergedInlineComment).toHaveBeenCalledWith(state.prUrl, 0); + expect(store.discardMergedReview).toHaveBeenCalledWith(state.prUrl); + }); +}); diff --git a/packages/agent-code-review/src/mcp.ts b/packages/agent-code-review/src/mcp.ts new file mode 100644 index 000000000..3aaec8ea8 --- /dev/null +++ b/packages/agent-code-review/src/mcp.ts @@ -0,0 +1,594 @@ +import { + canonicalPullRequestUrl, + fetchPullRequestDetails, + fetchPullRequestDiff, + fetchPullRequestReviewActivity +} from "github-review"; +import { type SpawnResult, spawn } from "@poe-code/agent-spawn"; +import { S, defineCommand, defineGroup } from "toolcraft"; +import { runMCP } from "toolcraft/mcp"; +import { + CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT, + discoverCodeReviewProfiles, + loadCodeReviewRolePrompt +} from "./assets.js"; +import { requireSafeDocumentSegment } from "./document-schemas.js"; +import type { + CodeReviewDecision, + CodeReviewDraft, + CodeReviewInlineComment +} from "./review-state.js"; +import { CodeReviewYamlStore, resolveCodeReviewStoreDirectory } from "./review-store.js"; + +export const CODE_REVIEW_AGENT_MCP_ROLES = ["agent", "orchestrator", "subagent"] as const; + +export type CodeReviewAgentMcpRole = (typeof CODE_REVIEW_AGENT_MCP_ROLES)[number]; + +export interface CodeReviewAgentMcpContext { + role: CodeReviewAgentMcpRole; + session: string; + actor: string; + cwd: string; + draftStore?: string; + agent: string; + profiles?: string[]; +} + +export interface CodeReviewAgentMcpConfig { + transport: "stdio"; + command: string; + args: string[]; +} + +export interface CodeReviewAgentMcpDependencies { + store?: CodeReviewYamlStore; + now?: () => Date; + fetchPr?: typeof fetchPullRequestDetails; + fetchDiff?: typeof fetchPullRequestDiff; + fetchComments?: typeof fetchPullRequestReviewActivity; + spawnAgent?: ( + agent: string, + prompt: string, + options: { + cwd: string; + mcpServers: Record; + } + ) => Promise; +} + +const inlineCommentSchema = S.Object({ + path: S.String({ description: "Repository-relative path in the PR diff." }), + line: S.Number({ description: "Right-side line number in the PR diff." }), + body: S.String({ description: "Inline review comment body." }) +}); +const inlineCommentIndexSchema = S.Number({ + description: "Zero-based merged review inline comment index." +}); + +const prParam = S.String({ description: "GitHub pull request URL." }); +export function parseCodeReviewAgentMcpArgs(argv: string[]): CodeReviewAgentMcpContext { + const values = new Map(); + const supportedFlags = new Set([ + "--role", + "--session", + "--actor", + "--cwd", + "--draft-store", + "--agent", + "--profiles" + ]); + for (let index = 0; index < argv.length; index += 2) { + const flag = argv[index]; + const value = argv[index + 1]; + if (!flag || !supportedFlags.has(flag)) { + throw new Error(`Unknown code-review agent MCP arg: ${flag}`); + } + if (!value || value.startsWith("--")) { + throw new Error(`${flag} requires a value`); + } + if (values.has(flag)) { + throw new Error(`${flag} may only be specified once`); + } + values.set(flag, value); + } + const role = requiredValue(values, "--role"); + if (!CODE_REVIEW_AGENT_MCP_ROLES.includes(role as CodeReviewAgentMcpRole)) { + throw new Error(`Invalid code-review MCP role: ${role}`); + } + const profiles = values.get("--profiles")?.split(","); + if (values.has("--profiles") && profiles?.length === 0) { + throw new Error("--profiles requires at least one profile"); + } + return { + role: role as CodeReviewAgentMcpRole, + session: requireSafeDocumentSegment(requiredValue(values, "--session"), "--session"), + actor: requireSafeDocumentSegment(requiredValue(values, "--actor"), "--actor"), + cwd: requiredValue(values, "--cwd"), + ...(values.has("--draft-store") ? { draftStore: requiredValue(values, "--draft-store") } : {}), + agent: requiredValue(values, "--agent"), + ...(profiles && profiles.length > 0 + ? { + profiles: [ + ...new Set(profiles.map((profile) => requireSafeDocumentSegment(profile, "--profiles"))) + ] + } + : {}) + }; +} + +export function createCodeReviewAgentMcpConfig( + context: CodeReviewAgentMcpContext +): CodeReviewAgentMcpConfig { + const args = [ + "agent-mcp", + "--role", + context.role, + "--session", + context.session, + "--actor", + context.actor, + "--cwd", + context.cwd, + ...(context.draftStore ? ["--draft-store", context.draftStore] : []), + "--agent", + context.agent + ]; + if (context.profiles?.length) { + args.push("--profiles", context.profiles.join(",")); + } + return { transport: "stdio", command: "poe-code", args: ["code-review", ...args] }; +} + +export function createCodeReviewAgentMcpGroup( + context: CodeReviewAgentMcpContext, + dependencies: CodeReviewAgentMcpDependencies = {} +) { + const store = + dependencies.store ?? + new CodeReviewYamlStore({ + directory: resolveCodeReviewStoreDirectory(context.cwd, context.draftStore) + }); + const fetchPr = dependencies.fetchPr ?? fetchPullRequestDetails; + const fetchDiff = dependencies.fetchDiff ?? fetchPullRequestDiff; + const fetchComments = dependencies.fetchComments ?? fetchPullRequestReviewActivity; + const now = dependencies.now ?? (() => new Date()); + + const prViewCommand = defineCommand({ + name: "code_review_pr_view", + description: "Fetch GitHub pull request metadata and review activity.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => fetchPr(params.pr, undefined, { cwd: context.cwd }) + }); + const prDiffCommand = defineCommand({ + name: "code_review_pr_diff", + description: "Fetch the pull request unified diff.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => fetchDiff(params.pr, { cwd: context.cwd }) + }); + const prCommentsCommand = defineCommand({ + name: "code_review_pr_comments", + description: "Fetch pull request discussion and review comments.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => fetchComments(params.pr, { cwd: context.cwd }) + }); + const createDraftCommand = defineCommand({ + name: "code_review_create_draft", + description: "Save a draft review without publishing anything to GitHub.", + scope: ["mcp"], + params: S.Object({ + pr: prParam, + body: S.String({ description: "Draft review summary body." }), + decision: S.Optional( + S.Enum(["APPROVE", "REQUEST_CHANGES", "COMMENT"] as const, { + description: "Optional proposed review decision." + }) + ), + comments: S.Optional(S.Array(inlineCommentSchema)) + }), + handler: async ({ params }) => { + const pr = canonicalPullRequestUrl(params.pr); + const draft = validateDraft(params); + const currentState = await ensureState(store, context, pr); + if (context.role === "orchestrator") { + const unfinishedProfiles = Object.values(currentState.subagents) + .filter(({ status }) => status === "pending" || status === "running") + .map(({ profile }) => profile); + if (unfinishedProfiles.length > 0) { + throw new Error( + `Cannot create merged review while subagents are unfinished: ${unfinishedProfiles.join(", ")}.` + ); + } + const state = await store.setMergedReview(pr, draft); + await store.appendOrchestratorAction(pr, { + action: "created_merged_review", + details: draft.decision + }); + return { + ok: true, + dry_run: true, + actor: context.actor, + draft: state.mergedReview + }; + } + const state = await store.addRawReview(pr, context.actor, draft); + return { + ok: true, + dry_run: true, + actor: context.actor, + draft: state.rawReviews[context.actor] + }; + } + }); + + const sharedTools = [prViewCommand, prDiffCommand, prCommentsCommand, createDraftCommand]; + if (context.role !== "orchestrator") { + return defineGroup({ + name: "code_review_agent", + description: "Role-filtered tools for code review agents.", + children: sharedTools + }); + } + + const profileListCommand = defineCommand({ + name: "code_review_profile_list", + description: "List reviewer profiles permitted for this run.", + scope: ["mcp"], + params: S.Object({}), + handler: async () => + discoverCodeReviewProfiles({ + cwd: context.cwd, + filters: context.profiles + }) + }); + const agentSpawnCommand = defineCommand({ + name: "code_review_agent_spawn", + description: "Spawn one review subagent with draft-only MCP access.", + scope: ["mcp"], + params: S.Object({ + pr: prParam, + profile: S.String({ description: "Reviewer profile name." }), + agent: S.Optional(S.String({ description: "Optional agent override for this reviewer." })) + }), + handler: async ({ params }) => { + const pr = canonicalPullRequestUrl(params.pr); + const profiles = await discoverCodeReviewProfiles({ + cwd: context.cwd, + filters: context.profiles + }); + const profile = profiles.find((candidate) => candidate.name === params.profile); + if (!profile) { + throw new Error(`Code review profile is unavailable: ${params.profile}`); + } + const existingState = await ensureState(store, context, pr); + const overrideAgent = params.agent?.trim(); + if (params.agent !== undefined && !overrideAgent) { + throw new Error("agent must be a non-empty string when specified."); + } + const agent = overrideAgent ?? context.agent; + if (existingState.rawReviews[profile.name]) { + await store.updateSubagent(pr, profile.name, { + profile: profile.name, + agent, + status: "completed", + completedAt: now().toISOString() + }); + await store.appendOrchestratorAction(pr, { + action: "reused_raw_review", + profile: profile.name + }); + return { + actor: profile.name, + agent, + status: "completed", + reused: true + }; + } + if ( + existingState.subagents[profile.name]?.status === "pending" || + existingState.subagents[profile.name]?.status === "running" + ) { + throw new Error(`Code review profile was already spawned in this session: ${profile.name}`); + } + const pendingStatus = { + profile: profile.name, + agent, + status: "pending" as const, + startedAt: now().toISOString() + }; + if (existingState.subagents[profile.name]) { + await store.updateSubagent(pr, profile.name, pendingStatus); + } else { + await store.addSubagent(pr, profile.name, pendingStatus); + } + const childConfig = createCodeReviewAgentMcpConfig({ + ...context, + role: "subagent", + actor: profile.name, + agent, + profiles: [profile.name] + }); + void (async () => { + try { + await store.appendOrchestratorAction(pr, { + action: "spawned_subagent", + profile: profile.name, + details: agent + }); + const prDetails = await fetchPr(pr, undefined, { cwd: context.cwd }); + const prompt = renderSubagentPrompt({ + template: await loadCodeReviewRolePrompt({ + cwd: context.cwd, + role: "subagent" + }), + profile: profile.content, + prUrl: pr, + prDetails + }); + await store.updateSubagent(pr, profile.name, { + profile: profile.name, + agent, + status: "running", + startedAt: now().toISOString() + }); + const result = await (dependencies.spawnAgent ?? spawnWithPoeCode)(agent, prompt, { + cwd: context.cwd, + mcpServers: { + "code-review": { + command: childConfig.command, + args: childConfig.args + } + } + }); + const completedState = await store.read(pr); + const missingRawReview = + result.exitCode === 0 && !completedState?.rawReviews[profile.name]; + await store.updateSubagent(pr, profile.name, { + profile: profile.name, + agent, + status: result.exitCode === 0 && !missingRawReview ? "completed" : "failed", + completedAt: now().toISOString(), + ...(missingRawReview + ? { error: "Reviewer completed without writing a raw review." } + : result.exitCode === 0 + ? {} + : { error: result.stderr || `exit ${result.exitCode}` }) + }); + } catch (error) { + await store.updateSubagent(pr, profile.name, { + profile: profile.name, + agent, + status: "failed", + completedAt: now().toISOString(), + error: error instanceof Error ? error.message : String(error) + }); + } + })(); + return { actor: profile.name, agent, status: "pending" }; + } + }); + const agentStatusCommand = defineCommand({ + name: "code_review_agent_status", + description: "Read current subagent status for a pull request.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => (await requireState(store, context, params.pr)).subagents + }); + const listDraftsCommand = defineCommand({ + name: "code_review_list_drafts", + description: "Read draft reviews stored for a pull request.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => { + const state = await requireState(store, context, params.pr); + return { + raw_reviews: state.rawReviews, + merged_review: state.mergedReview ?? null + }; + } + }); + const editInlineCommentCommand = defineCommand({ + name: "code_review_edit_inline_comment", + description: "Replace one inline comment in the merged review draft.", + scope: ["mcp"], + params: S.Object({ + pr: prParam, + index: inlineCommentIndexSchema, + path: S.String({ description: "Repository-relative path in the PR diff." }), + line: S.Number({ description: "Right-side line number in the PR diff." }), + body: S.String({ description: "Inline review comment body." }) + }), + handler: async ({ params }) => { + const pr = canonicalPullRequestUrl(params.pr); + await requireState(store, context, pr); + const state = await store.editMergedInlineComment(pr, params.index, { + path: params.path, + line: params.line, + body: params.body + }); + await store.appendOrchestratorAction(pr, { + action: "edited_inline_comment", + details: String(params.index) + }); + return { ok: true, dry_run: true, merged_review: state.mergedReview }; + } + }); + const deleteInlineCommentCommand = defineCommand({ + name: "code_review_delete_inline_comment", + description: "Delete one inline comment from the merged review draft.", + scope: ["mcp"], + params: S.Object({ pr: prParam, index: inlineCommentIndexSchema }), + handler: async ({ params }) => { + const pr = canonicalPullRequestUrl(params.pr); + await requireState(store, context, pr); + const state = await store.deleteMergedInlineComment(pr, params.index); + await store.appendOrchestratorAction(pr, { + action: "deleted_inline_comment", + details: String(params.index) + }); + return { ok: true, dry_run: true, merged_review: state.mergedReview }; + } + }); + const discardDraftCommand = defineCommand({ + name: "code_review_discard_draft", + description: "Discard the merged review draft without changing raw reviews.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => { + const pr = canonicalPullRequestUrl(params.pr); + await requireState(store, context, pr); + await store.discardMergedReview(pr); + await store.appendOrchestratorAction(pr, { action: "discarded_merged_review" }); + return { ok: true, dry_run: true, merged_review: null }; + } + }); + const commitDraftsCommand = defineCommand({ + name: "code_review_commit_drafts", + description: "Preview publishing stored drafts; MCP never publishes to GitHub.", + scope: ["mcp"], + params: S.Object({ pr: prParam }), + handler: async ({ params }) => { + const state = await requireState(store, context, params.pr); + return { + ok: true, + dry_run: true, + would_publish: state.mergedReview ?? Object.values(state.rawReviews) + }; + } + }); + return defineGroup({ + name: "code_review_agent", + description: "Role-filtered tools for code review agents.", + children: [ + ...sharedTools, + profileListCommand, + agentSpawnCommand, + agentStatusCommand, + listDraftsCommand, + editInlineCommentCommand, + deleteInlineCommentCommand, + discardDraftCommand, + commitDraftsCommand + ] + }); +} + +export async function runCodeReviewAgentMcp(context: CodeReviewAgentMcpContext): Promise { + await runMCP(createCodeReviewAgentMcpGroup(context), { + name: "code-review-agent-mcp", + version: "0.1.0", + omitRootToolNamePrefix: true + }); +} + +async function spawnWithPoeCode( + agent: string, + prompt: string, + options: { + cwd: string; + mcpServers: Record; + } +): Promise { + return spawn(agent, { prompt, ...options }); +} + +function renderSubagentPrompt(input: { + template: string; + profile: string; + prUrl: string; + prDetails: unknown; +}): string { + return `${input.template} + +REQUIRED REVIEW FLOW +1. Read the pull request details, diff, and prior review activity with the available code_review_pr_* tools before drafting. +2. Do not raise a concern already covered by an existing comment or prior review unless changed code introduces a distinct new issue. +3. Apply the assigned profile and create exactly one raw review draft with code_review_create_draft; do not publish anything. +4. The only allowed MCP tools are code_review_pr_view, code_review_pr_diff, code_review_pr_comments, and code_review_create_draft. +5. ${CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT} + +PROFILE +${input.profile} + +PULL REQUEST +${input.prUrl} +${JSON.stringify(input.prDetails)}`; +} + +async function ensureState( + store: CodeReviewYamlStore, + context: CodeReviewAgentMcpContext, + pr: string +) { + const state = await store.read(pr); + if (state) { + if (state.sessionId !== context.session) { + throw new Error("Draft belongs to a different code-review session."); + } + return state; + } + return store.create({ + prUrl: pr, + sessionId: context.session, + selectedAgent: context.agent, + selectedProfiles: context.profiles ?? [] + }); +} + +async function requireState( + store: CodeReviewYamlStore, + context: CodeReviewAgentMcpContext, + pr: string +) { + const state = await store.read(pr); + if (!state) { + throw new Error(`No draft review exists for pull request: ${pr}`); + } + if (state.sessionId !== context.session) { + throw new Error("Draft belongs to a different code-review session."); + } + return state; +} + +function validateDraft(input: { + body: string; + decision?: CodeReviewDecision; + comments?: CodeReviewInlineComment[]; +}): CodeReviewDraft { + const body = requireText(input.body, "body"); + const comments = (input.comments ?? []).map((comment) => ({ + path: requireText(comment.path, "comments.path"), + line: requirePositiveInteger(comment.line, "comments.line"), + body: requireText(comment.body, "comments.body") + })); + return { + body, + comments, + ...(input.decision ? { decision: input.decision } : {}) + }; +} + +function requiredValue(values: Map, flag: string): string { + const value = values.get(flag); + if (value === undefined || value.length === 0) { + throw new Error(`${flag} is required`); + } + return value; +} + +function requireText(value: string, field: string): string { + const normalized = value.trim(); + if (!normalized) { + throw new Error(`${field} must be non-empty.`); + } + return normalized; +} + +function requirePositiveInteger(value: number, field: string): number { + if (!Number.isSafeInteger(value) || value <= 0) { + throw new Error(`${field} must be a positive integer.`); + } + return value; +} diff --git a/packages/agent-code-review/src/review-state.ts b/packages/agent-code-review/src/review-state.ts new file mode 100644 index 000000000..3bf4cf57c --- /dev/null +++ b/packages/agent-code-review/src/review-state.ts @@ -0,0 +1,522 @@ +import { canonicalPullRequestUrl, parseGitHubPullRequestRef } from "github-review"; +import { parse as load, stringify } from "yaml"; + +export interface CodeReviewInlineComment { + path: string; + line: number; + body: string; +} + +export interface CodeReviewDraft { + body: string; + comments: CodeReviewInlineComment[]; + decision?: CodeReviewDecision; +} + +export type CodeReviewDecision = "APPROVE" | "REQUEST_CHANGES" | "COMMENT"; + +export interface CodeReviewPrRef { + host: string; + owner: string; + repo: string; + number: number; +} + +export type CodeReviewReviewState = "in_progress" | "merged" | "published" | "failed"; + +export interface CodeReviewTimestamps { + createdAt: string; + updatedAt: string; + publishedAt?: string; +} + +export interface CodeReviewSubagentStatus { + profile: string; + status: "pending" | "running" | "completed" | "failed"; + agent?: string; + startedAt?: string; + completedAt?: string; + error?: string; +} + +export interface CodeReviewOrchestratorAction { + at: string; + action: string; + details?: string; + profile?: string; +} + +export interface CodeReviewPublishedReceipt { + publishedAt: string; + actor?: string; + sessionId?: string; + decision?: CodeReviewDecision; + reviewId?: string | number; + reviewUrl?: string; +} + +export interface CodeReviewState { + version: 1; + sessionId: string; + prUrl: string; + prRef: CodeReviewPrRef; + selectedAgent: string; + selectedProfiles: string[]; + state: CodeReviewReviewState; + timestamps: CodeReviewTimestamps; + rawReviews: Record; + subagents: Record; + mergedReview?: CodeReviewDraft; + orchestratorActions: CodeReviewOrchestratorAction[]; + published?: CodeReviewPublishedReceipt; +} + +export function parseCodeReviewState( + content: string, + filePath = "code-review review YAML" +): CodeReviewState { + let value: unknown; + try { + value = load(content) as unknown; + } catch (error) { + throw new Error( + `${filePath}: invalid YAML: ${error instanceof Error ? error.message : String(error)}` + ); + } + try { + return parseCodeReviewStateValue(value); + } catch (error) { + throw new Error(`${filePath}: ${error instanceof Error ? error.message : String(error)}`); + } +} + +function parseCodeReviewStateValue(value: unknown): CodeReviewState { + const state = requireMapping(value, "Code review state"); + requireOnlyFields(state, "Code review state", [ + "version", + "session_id", + "pr_url", + "pr_ref", + "selected_agent", + "selected_profiles", + "state", + "timestamps", + "raw_reviews", + "subagents", + "merged_review", + "orchestrator_actions", + "published" + ]); + if (state.version !== 1) throw new Error("version must equal 1."); + if (!isSafeIdentifier(state.session_id)) throw new Error("session_id must be a safe identifier."); + if (!isNonEmptyString(state.pr_url)) throw new Error("pr_url must be a non-empty string."); + if (!isNonEmptyString(state.selected_agent)) + throw new Error("selected_agent must be a non-empty string."); + if (!isReviewState(state.state)) throw new Error("state is invalid."); + const prRef = validatePrRef(state.pr_ref); + validatePrIdentity(state.pr_url, prRef); + const selectedProfiles = validateStringArray( + state.selected_profiles, + "Code review state selected_profiles" + ); + for (const [index, profile] of selectedProfiles.entries()) { + if (!isSafeIdentifier(profile)) + throw new Error(`selected_profiles[${index}] must be a safe identifier.`); + } + const timestamps = validateTimestamps(state.timestamps); + const rawReviews = validateDraftRecord(state.raw_reviews, "raw_reviews"); + const subagents = validateSubagents(state.subagents); + const orchestratorActions = validateActions(state.orchestrator_actions); + const mergedReview = + state.merged_review === undefined + ? undefined + : validateDraft(state.merged_review, "Code review state merged_review"); + const published = + state.published === undefined ? undefined : validatePublishedReceipt(state.published); + if ((state.state === "merged" || state.state === "published") && mergedReview === undefined) { + throw new Error("Merged code review state must include a merged review."); + } + if (state.state === "published" && published === undefined) { + throw new Error("Published code review state must include a receipt."); + } + if ( + state.state === "published" && + (timestamps.publishedAt === undefined || timestamps.publishedAt !== published?.publishedAt) + ) { + throw new Error("Published code review state timestamps must match its receipt."); + } + if ( + state.state !== "published" && + (published !== undefined || timestamps.publishedAt !== undefined) + ) { + throw new Error("Unpublished code review state cannot include published metadata."); + } + return { + version: 1, + sessionId: state.session_id, + prUrl: state.pr_url, + prRef, + selectedAgent: state.selected_agent, + selectedProfiles, + state: state.state, + timestamps, + rawReviews, + subagents, + ...(mergedReview === undefined ? {} : { mergedReview }), + orchestratorActions, + ...(published === undefined ? {} : { published }) + }; +} + +export function serializeCodeReviewState(state: CodeReviewState): string { + const content = stringify( + { + version: state.version, + session_id: state.sessionId, + pr_url: state.prUrl, + pr_ref: state.prRef, + selected_agent: state.selectedAgent, + selected_profiles: state.selectedProfiles, + state: state.state, + timestamps: { + created_at: state.timestamps.createdAt, + updated_at: state.timestamps.updatedAt, + ...(state.timestamps.publishedAt === undefined + ? {} + : { published_at: state.timestamps.publishedAt }) + }, + raw_reviews: state.rawReviews, + subagents: Object.fromEntries( + Object.entries(state.subagents).map(([actor, subagent]) => [ + actor, + { + profile: subagent.profile, + status: subagent.status, + ...(subagent.agent === undefined ? {} : { agent: subagent.agent }), + ...(subagent.startedAt === undefined ? {} : { started_at: subagent.startedAt }), + ...(subagent.completedAt === undefined ? {} : { completed_at: subagent.completedAt }), + ...(subagent.error === undefined ? {} : { error: subagent.error }) + } + ]) + ), + ...(state.mergedReview === undefined ? {} : { merged_review: state.mergedReview }), + orchestrator_actions: state.orchestratorActions.map((action) => ({ + at: action.at, + action: action.action, + ...(action.details === undefined ? {} : { details: action.details }), + ...(action.profile === undefined ? {} : { profile: action.profile }) + })), + ...(state.published === undefined + ? {} + : { + published: { + published_at: state.published.publishedAt, + ...(state.published.actor === undefined ? {} : { actor: state.published.actor }), + ...(state.published.sessionId === undefined + ? {} + : { session_id: state.published.sessionId }), + ...(state.published.decision === undefined + ? {} + : { decision: state.published.decision }), + ...(state.published.reviewId === undefined + ? {} + : { review_id: state.published.reviewId }), + ...(state.published.reviewUrl === undefined + ? {} + : { review_url: state.published.reviewUrl }) + } + }) + }, + { aliasDuplicateObjects: false, lineWidth: 0 } + ); + parseCodeReviewState(content); + return content; +} + +function validateDraftRecord(value: unknown, field: string): Record { + const reviews = requireMapping(value, `Code review state ${field}`); + const validated: Record = {}; + for (const [actor, draft] of Object.entries(reviews)) { + if (!isSafeIdentifier(actor)) + throw new Error(`Code review state ${field}.${actor} key must be a safe identifier.`); + validated[actor] = validateDraft(draft, `Code review state ${field}.${actor}`); + } + return validated; +} + +function validateSubagents(value: unknown): Record { + const statuses = requireMapping(value, "Code review state subagents"); + const validated: Record = {}; + for (const [actor, value] of Object.entries(statuses)) { + if (!isSafeIdentifier(actor)) + throw new Error(`Code review state subagents.${actor} key must be a safe identifier.`); + const status = requireMapping(value, `Code review state subagents.${actor}`); + requireOnlyFields(status, `Code review state subagents.${actor}`, [ + "profile", + "status", + "agent", + "started_at", + "completed_at", + "error" + ]); + if (!isSafeIdentifier(status.profile)) { + throw new Error(`Code review state subagents.${actor}.profile must be a safe identifier.`); + } + if (!isSubagentState(status.status)) { + throw new Error(`Code review state subagents.${actor}.status is invalid.`); + } + if (!optionalString(status.agent)) { + throw new Error(`Code review state subagents.${actor}.agent must be a non-empty string.`); + } + if (!optionalDateString(status.started_at)) { + throw new Error(`Code review state subagents.${actor}.started_at must be a valid timestamp.`); + } + if (!optionalDateString(status.completed_at)) { + throw new Error( + `Code review state subagents.${actor}.completed_at must be a valid timestamp.` + ); + } + if (!optionalString(status.error)) { + throw new Error(`Code review state subagents.${actor}.error must be a non-empty string.`); + } + validated[actor] = { + profile: status.profile, + status: status.status, + ...(status.agent === undefined ? {} : { agent: status.agent }), + ...(status.started_at === undefined ? {} : { startedAt: status.started_at }), + ...(status.completed_at === undefined ? {} : { completedAt: status.completed_at }), + ...(status.error === undefined ? {} : { error: status.error }) + }; + } + return validated; +} + +function validateActions(value: unknown): CodeReviewOrchestratorAction[] { + if (!Array.isArray(value)) { + throw new Error("Code review state orchestrator_actions must be an array."); + } + return value.map((value, index) => { + const field = `Code review state orchestrator_actions[${index}]`; + const action = requireMapping(value, field); + requireOnlyFields(action, field, ["at", "action", "details", "profile"]); + if (!isDateString(action.at)) { + throw new Error(`${field}.at must be a valid timestamp.`); + } + if (!isNonEmptyString(action.action)) { + throw new Error(`${field}.action must be a non-empty string.`); + } + if (!optionalString(action.details)) { + throw new Error(`${field}.details must be a non-empty string.`); + } + if (action.profile !== undefined && !isSafeIdentifier(action.profile)) { + throw new Error(`${field}.profile must be a safe identifier.`); + } + return action as unknown as CodeReviewOrchestratorAction; + }); +} + +function validatePrRef(value: unknown): CodeReviewPrRef { + const ref = requireMapping(value, "Code review state pr_ref"); + requireOnlyFields(ref, "Code review state pr_ref", ["host", "owner", "repo", "number"]); + if (!isNonEmptyString(ref.host)) { + throw new Error("Code review state pr_ref.host must be a non-empty string."); + } + if (!isSafeIdentifier(ref.owner)) { + throw new Error("Code review state pr_ref.owner must be a safe identifier."); + } + if (!isSafeIdentifier(ref.repo)) { + throw new Error("Code review state pr_ref.repo must be a safe identifier."); + } + if (!Number.isSafeInteger(ref.number) || (ref.number as number) <= 0) { + throw new Error("Code review state pr_ref.number must be a positive integer."); + } + return ref as unknown as CodeReviewPrRef; +} + +function validatePrIdentity(prUrl: string, prRef: CodeReviewPrRef): void { + const parsedRef = parseGitHubPullRequestRef(prUrl); + if ( + parsedRef === undefined || + parsedRef === null || + canonicalPullRequestUrl(prUrl) !== prUrl || + parsedRef.host !== prRef.host || + parsedRef.owner !== prRef.owner || + parsedRef.repo !== prRef.repo || + parsedRef.number !== prRef.number + ) { + throw new Error("Code review state contains inconsistent PR metadata."); + } +} + +function validateTimestamps(value: unknown): CodeReviewTimestamps { + const timestamps = requireMapping(value, "Code review state timestamps"); + requireOnlyFields(timestamps, "Code review state timestamps", [ + "created_at", + "updated_at", + "published_at" + ]); + if (!isDateString(timestamps.created_at)) { + throw new Error("Code review state timestamps.created_at must be a valid timestamp."); + } + if (!isDateString(timestamps.updated_at)) { + throw new Error("Code review state timestamps.updated_at must be a valid timestamp."); + } + if (!optionalDateString(timestamps.published_at)) { + throw new Error("Code review state timestamps.published_at must be a valid timestamp."); + } + if (Date.parse(timestamps.updated_at as string) < Date.parse(timestamps.created_at as string)) { + throw new Error("Code review state timestamps are out of order."); + } + return { + createdAt: timestamps.created_at, + updatedAt: timestamps.updated_at, + ...(timestamps.published_at === undefined ? {} : { publishedAt: timestamps.published_at }) + }; +} + +function validatePublishedReceipt(value: unknown): CodeReviewPublishedReceipt { + const receipt = requireMapping(value, "Code review state published"); + requireOnlyFields(receipt, "Code review state published", [ + "published_at", + "actor", + "session_id", + "decision", + "review_id", + "review_url" + ]); + const reviewId = receipt.review_id; + if (!isDateString(receipt.published_at)) { + throw new Error("Code review state published.published_at must be a valid timestamp."); + } + if (receipt.actor !== undefined && !isSafeIdentifier(receipt.actor)) { + throw new Error("Code review state published.actor must be a safe identifier."); + } + if (receipt.session_id !== undefined && !isSafeIdentifier(receipt.session_id)) { + throw new Error("Code review state published.session_id must be a safe identifier."); + } + if (!optionalDecision(receipt.decision)) { + throw new Error("Code review state published.decision is invalid."); + } + if ( + reviewId !== undefined && + !isNonEmptyString(reviewId) && + !(typeof reviewId === "number" && Number.isSafeInteger(reviewId) && reviewId > 0) + ) { + throw new Error("Code review state published.review_id is invalid."); + } + if (!optionalString(receipt.review_url)) { + throw new Error("Code review state published.review_url must be a non-empty string."); + } + return { + publishedAt: receipt.published_at, + ...(receipt.actor === undefined ? {} : { actor: receipt.actor }), + ...(receipt.session_id === undefined ? {} : { sessionId: receipt.session_id }), + ...(receipt.decision === undefined ? {} : { decision: receipt.decision }), + ...(reviewId === undefined ? {} : { reviewId }), + ...(receipt.review_url === undefined ? {} : { reviewUrl: receipt.review_url }) + }; +} + +function validateDraft(value: unknown, field: string): CodeReviewDraft { + const draft = requireMapping(value, field); + requireOnlyFields(draft, field, ["body", "comments", "decision"]); + if (typeof draft.body !== "string") { + throw new Error(`${field}.body must be a string.`); + } + if (!Array.isArray(draft.comments)) { + throw new Error(`${field}.comments must be an array.`); + } + if (!optionalDecision(draft.decision)) { + throw new Error(`${field}.decision is invalid.`); + } + for (const [index, comment] of draft.comments.entries()) { + const inlineComment = requireMapping(comment, `${field}.comments[${index}]`); + requireOnlyFields(inlineComment, `${field}.comments[${index}]`, ["path", "line", "body"]); + if (!isRepositoryRelativePath(inlineComment.path)) + throw new Error(`${field}.comments[${index}].path must be a repository-relative path.`); + if (!Number.isSafeInteger(inlineComment.line) || (inlineComment.line as number) <= 0) + throw new Error( + `${field}.comments[${index}].line must be a positive integer (invalid inline comment).` + ); + if (!isNonEmptyString(inlineComment.body)) + throw new Error(`${field}.comments[${index}].body must be a non-empty string.`); + } + return draft as unknown as CodeReviewDraft; +} + +function requireMapping(value: unknown, field: string): Record { + if (typeof value !== "object" || value === null || Array.isArray(value)) { + throw new Error(`${field} must be a YAML mapping.`); + } + return value as Record; +} + +function requireOnlyFields( + value: Record, + field: string, + allowedFields: readonly string[] +): void { + const allowed = new Set(allowedFields); + for (const key of Object.keys(value)) { + if (!allowed.has(key)) { + throw new Error(`${field}.${key} is not supported.`); + } + } +} + +function validateStringArray(value: unknown, field: string): string[] { + if (!Array.isArray(value) || !value.every(isNonEmptyString)) { + throw new Error(`${field} must be an array of non-empty strings.`); + } + return value; +} + +function isReviewState(value: unknown): value is CodeReviewReviewState { + return ["in_progress", "merged", "published", "failed"].includes(value as string); +} + +function isSubagentState(value: unknown): value is CodeReviewSubagentStatus["status"] { + return ["pending", "running", "completed", "failed"].includes(value as string); +} + +function optionalString(value: unknown): value is string | undefined { + return value === undefined || isNonEmptyString(value); +} + +function optionalDecision(value: unknown): value is CodeReviewDecision | undefined { + return value === undefined || ["APPROVE", "REQUEST_CHANGES", "COMMENT"].includes(value as string); +} + +function optionalDateString(value: unknown): value is string | undefined { + return value === undefined || isDateString(value); +} + +function isDateString(value: unknown): value is string { + return isNonEmptyString(value) && !Number.isNaN(Date.parse(value)); +} + +function isNonEmptyString(value: unknown): value is string { + return typeof value === "string" && value.trim().length > 0; +} + +function isRepositoryRelativePath(value: unknown): value is string { + return ( + isNonEmptyString(value) && + value.trim() === value && + !value.startsWith("/") && + !value.includes("\0") && + !value.split("/").some((segment) => segment === "." || segment === "..") + ); +} + +function isSafeIdentifier(value: unknown): value is string { + return ( + isNonEmptyString(value) && + value.trim() === value && + value.normalize("NFKC") === value && + /^[A-Za-z0-9._-]+$/.test(value) && + !value.startsWith(".") && + value !== "." && + value !== ".." + ); +} diff --git a/packages/agent-code-review/src/review-store.test.ts b/packages/agent-code-review/src/review-store.test.ts new file mode 100644 index 000000000..88ce568fc --- /dev/null +++ b/packages/agent-code-review/src/review-store.test.ts @@ -0,0 +1,105 @@ +import { beforeEach, describe, expect, it, vi } from "vitest"; + +const memoryFileSystem = vi.hoisted(() => { + const { Volume, createFsFromVolume } = require("memfs") as typeof import("memfs"); + const volume = new Volume(); + return { volume, fs: createFsFromVolume(volume) }; +}); + +vi.mock("node:fs/promises", () => memoryFileSystem.fs.promises); + +import { CodeReviewYamlStore, codeReviewFileName } from "./review-store.js"; + +const PR_URL = "https://github.com/acme/widgets/pull/123"; +const PROFILE = "security"; + +describe("CodeReviewYamlStore.startRun resume semantics", () => { + const directory = "/repo/.poe-code/code-review/reviews"; + + beforeEach(() => { + memoryFileSystem.volume.reset(); + }); + + it("clears raw_reviews and subagent state on rerun so the new PR diff is reanalyzed", async () => { + const store = new CodeReviewYamlStore({ directory }); + await store.startRun({ + sessionId: "session-1", + prUrl: PR_URL, + selectedAgent: "codex", + selectedProfiles: [PROFILE] + }); + await store.addSubagent(PR_URL, PROFILE, { + profile: PROFILE, + agent: "codex", + status: "completed", + completedAt: "2026-05-26T00:00:00.000Z" + }); + await store.addRawReview(PR_URL, PROFILE, { + body: "Prior findings against the old diff.", + comments: [] + }); + + const resumed = await store.startRun({ + sessionId: "session-2", + prUrl: PR_URL, + selectedAgent: "codex", + selectedProfiles: [PROFILE] + }); + + expect(resumed.rawReviews).toEqual({}); + expect(resumed.subagents).toEqual({}); + expect(resumed.sessionId).toBe("session-2"); + expect(resumed.orchestratorActions.at(-1)?.action).toBe("resumed_run"); + }); + + it("uses one draft filename for equivalent PR URLs with different casing", () => { + expect(codeReviewFileName("https://github.com/Acme/Widgets/pull/123")).toBe( + codeReviewFileName(PR_URL) + ); + }); +}); + +describe("CodeReviewYamlStore merged draft management", () => { + const directory = "/repo/.poe-code/code-review/reviews"; + + beforeEach(() => { + memoryFileSystem.volume.reset(); + }); + + it("edits, deletes, and discards only the merged review draft", async () => { + const store = new CodeReviewYamlStore({ directory }); + await store.startRun({ + sessionId: "session-1", + prUrl: PR_URL, + selectedAgent: "codex", + selectedProfiles: [PROFILE] + }); + await store.setMergedReview(PR_URL, { + body: "Merged summary", + comments: [ + { path: "src/a.ts", line: 4, body: "old" }, + { path: "src/b.ts", line: 7, body: "keep" } + ] + }); + + const edited = await store.editMergedInlineComment(PR_URL, 0, { + path: "src/a.ts", + line: 5, + body: "updated" + }); + expect(edited.mergedReview?.comments[0]).toEqual({ + path: "src/a.ts", + line: 5, + body: "updated" + }); + + const deleted = await store.deleteMergedInlineComment(PR_URL, 1); + expect(deleted.mergedReview?.comments).toEqual([ + { path: "src/a.ts", line: 5, body: "updated" } + ]); + + const discarded = await store.discardMergedReview(PR_URL); + expect(discarded.state).toBe("in_progress"); + expect(discarded.mergedReview).toBeUndefined(); + }); +}); diff --git a/packages/agent-code-review/src/review-store.ts b/packages/agent-code-review/src/review-store.ts new file mode 100644 index 000000000..175162511 --- /dev/null +++ b/packages/agent-code-review/src/review-store.ts @@ -0,0 +1,750 @@ +import { randomUUID } from "node:crypto"; +import { constants } from "node:fs"; +import { + access, + link, + lstat, + mkdir, + open, + readFile, + readdir, + rename, + stat, + unlink +} from "node:fs/promises"; +import { basename, dirname, isAbsolute, parse, relative, resolve, sep } from "node:path"; +import { + canonicalPullRequestUrl, + filesystemSafeNamePart, + parseGitHubPullRequestRef +} from "github-review"; +import { requireSafeDocumentSegment } from "./document-schemas.js"; +import { + type CodeReviewDraft, + type CodeReviewInlineComment, + type CodeReviewOrchestratorAction, + type CodeReviewPublishedReceipt, + type CodeReviewState, + type CodeReviewSubagentStatus, + parseCodeReviewState, + serializeCodeReviewState +} from "./review-state.js"; + +export const DEFAULT_CODE_REVIEW_REVIEWS_DIRECTORY = ".poe-code/code-review/reviews"; +export const CODE_REVIEW_DIRECTORY = ".poe-code/code-review"; +export const CODE_REVIEW_ARCHIVE_DIRECTORY = "archive"; + +export interface CreateCodeReviewInput { + sessionId: string; + prUrl: string; + selectedAgent: string; + selectedProfiles: string[]; + timestamp?: string; + subagents?: Record; +} + +export interface CodeReviewStoreOptions { + directory?: string; + now?: () => Date; + lockTimeoutMs?: number; +} + +export interface ReadCodeReviewDraftInput extends CodeReviewStoreOptions { + cwd: string; + prUrl: string; + draftStore?: string; +} + +export function codeReviewFileName(prUrl: string): string { + const ref = requirePullRequestRef(prUrl); + return `${safeFilePart(ref.owner)}_${safeFilePart(ref.repo)}_PR${ref.number}.yaml`; +} + +export function resolveCodeReviewStoreDirectory( + cwd: string, + directory = DEFAULT_CODE_REVIEW_REVIEWS_DIRECTORY +): string { + const root = resolve(cwd, CODE_REVIEW_DIRECTORY); + const target = isAbsolute(directory) ? resolve(directory) : resolve(cwd, directory); + const fromRoot = relative(root, target); + if (fromRoot.startsWith("..") || fromRoot.startsWith(sep)) { + throw new Error(`Code review draft store must stay under ${root}: ${target}`); + } + return target; +} + +export function createCodeReviewState(input: CreateCodeReviewInput): CodeReviewState { + const ref = requirePullRequestRef(input.prUrl); + const sessionId = requireSafeDocumentSegment(input.sessionId, "Code review session"); + const selectedProfiles = input.selectedProfiles.map((profile) => + requireSafeDocumentSegment(profile, "Code review profile") + ); + const timestamp = input.timestamp ?? new Date().toISOString(); + return { + version: 1, + sessionId, + prUrl: canonicalPullRequestUrl(input.prUrl), + prRef: { + host: ref.host, + owner: ref.owner, + repo: ref.repo, + number: ref.number + }, + selectedAgent: input.selectedAgent, + selectedProfiles, + state: "in_progress", + timestamps: { createdAt: timestamp, updatedAt: timestamp }, + rawReviews: {}, + subagents: input.subagents ?? {}, + orchestratorActions: [] + }; +} + +export async function readCodeReviewDraft( + input: ReadCodeReviewDraftInput +): Promise { + const directory = resolveCodeReviewStoreDirectory(input.cwd, input.draftStore ?? input.directory); + return new CodeReviewYamlStore({ + directory, + ...(input.now ? { now: input.now } : {}), + ...(input.lockTimeoutMs ? { lockTimeoutMs: input.lockTimeoutMs } : {}) + }).read(input.prUrl); +} + +export class CodeReviewYamlStore { + readonly directory: string; + readonly archiveDirectory: string; + readonly #now: () => Date; + readonly #lockTimeoutMs: number; + + constructor(options: CodeReviewStoreOptions = {}) { + this.directory = resolve(options.directory ?? DEFAULT_CODE_REVIEW_REVIEWS_DIRECTORY); + this.archiveDirectory = containedPath(this.directory, CODE_REVIEW_ARCHIVE_DIRECTORY); + this.#now = options.now ?? (() => new Date()); + this.#lockTimeoutMs = options.lockTimeoutMs ?? 10_000; + } + + pathForPullRequest(prUrl: string): string { + return containedPath(this.directory, codeReviewFileName(prUrl)); + } + + async create(input: CreateCodeReviewInput): Promise { + const state = createCodeReviewState({ + ...input, + timestamp: input.timestamp ?? this.#timestamp() + }); + const filePath = this.pathForPullRequest(state.prUrl); + return this.#withLock(state.prUrl, async () => { + if (await pathExists(filePath)) { + throw new Error(`Code review already exists for pull request: ${state.prUrl}`); + } + await writeAtomically(filePath, serializeCodeReviewState(state)); + return state; + }); + } + + async startRun(input: CreateCodeReviewInput): Promise { + const freshState = createCodeReviewState({ + ...input, + timestamp: input.timestamp ?? this.#timestamp() + }); + return this.#withLock(freshState.prUrl, async () => { + const existing = await this.read(freshState.prUrl); + if (existing && existing.state !== "published") { + const resumedAt = this.#timestamp(); + const resumed = { + ...existing, + sessionId: freshState.sessionId, + selectedAgent: freshState.selectedAgent, + selectedProfiles: freshState.selectedProfiles, + state: "in_progress" as const, + mergedReview: undefined, + rawReviews: {}, + subagents: {}, + orchestratorActions: [ + ...existing.orchestratorActions, + { at: resumedAt, action: "resumed_run" } + ] + }; + return this.#save(resumed); + } + if (existing) { + await ensureDirectory(this.archiveDirectory); + await this.#archive( + freshState.prUrl, + existing.published?.publishedAt ?? freshState.timestamps.createdAt + ); + } + await writeAtomically( + this.pathForPullRequest(freshState.prUrl), + serializeCodeReviewState(freshState) + ); + return freshState; + }); + } + + async read(prUrl: string): Promise { + const filePath = this.pathForPullRequest(prUrl); + try { + await assertDirectoryPath(this.directory); + const state = parseCodeReviewState(await readRegularFile(filePath), filePath); + if (this.pathForPullRequest(state.prUrl) !== filePath) { + throw new Error(`${filePath}: pr_ref does not match its filename.`); + } + return state; + } catch (error) { + if (isMissingFileError(error)) { + return undefined; + } + throw error; + } + } + + async addRawReview( + prUrl: string, + actor: string, + review: CodeReviewDraft + ): Promise { + return this.#updateActive(prUrl, (state) => { + const key = requireActor(actor); + if (state.rawReviews[key] !== undefined) { + throw new Error(`Raw review is immutable after creation: ${key}`); + } + return { + ...state, + rawReviews: { ...state.rawReviews, [key]: review } + }; + }); + } + + async updateSubagent( + prUrl: string, + actor: string, + status: CodeReviewSubagentStatus + ): Promise { + return this.#updateActive(prUrl, (state) => ({ + ...state, + subagents: { ...state.subagents, [requireActor(actor)]: status } + })); + } + + async addSubagent( + prUrl: string, + actor: string, + status: CodeReviewSubagentStatus + ): Promise { + return this.#updateActive(prUrl, (state) => { + const key = requireActor(actor); + if (state.subagents[key] !== undefined) { + throw new Error(`Code review profile was already spawned in this session: ${key}`); + } + return { + ...state, + subagents: { ...state.subagents, [key]: status } + }; + }); + } + + async setMergedReview(prUrl: string, review: CodeReviewDraft): Promise { + return this.#updateActive(prUrl, (state) => { + return { + ...state, + state: "merged", + mergedReview: review + }; + }); + } + + async editMergedInlineComment( + prUrl: string, + index: number, + comment: CodeReviewInlineComment + ): Promise { + return this.#updateActive(prUrl, (state) => { + const mergedReview = requireMergedReview(state); + const commentIndex = requireInlineCommentIndex(index, mergedReview.comments.length); + const comments = [...mergedReview.comments]; + comments[commentIndex] = comment; + return { + ...state, + mergedReview: { ...mergedReview, comments } + }; + }); + } + + async deleteMergedInlineComment(prUrl: string, index: number): Promise { + return this.#updateActive(prUrl, (state) => { + const mergedReview = requireMergedReview(state); + const commentIndex = requireInlineCommentIndex(index, mergedReview.comments.length); + return { + ...state, + mergedReview: { + ...mergedReview, + comments: mergedReview.comments.filter((_, currentIndex) => currentIndex !== commentIndex) + } + }; + }); + } + + async discardMergedReview(prUrl: string): Promise { + return this.#updateActive(prUrl, (state) => { + requireMergedReview(state); + return { ...state, state: "in_progress", mergedReview: undefined }; + }); + } + + async appendOrchestratorAction( + prUrl: string, + action: Omit & { at?: string } + ): Promise { + return this.#updateActive(prUrl, (state) => ({ + ...state, + orchestratorActions: [ + ...state.orchestratorActions, + { ...action, at: action.at ?? this.#timestamp() } + ] + })); + } + + async commit( + prUrl: string, + receipt: Omit & { + publishedAt?: string; + } + ): Promise<{ state: CodeReviewState; archivePath: string }> { + return this.#withLock(prUrl, async () => { + const state = await this.#requireActive(prUrl); + if (state.mergedReview === undefined) { + throw new Error("Code review must be merged before publishing."); + } + return this.#commitState(state, receipt); + }); + } + + async publish( + prUrl: string, + publish: (state: CodeReviewState) => Promise<{ + receipt: Omit & { + publishedAt?: string; + }; + result: T; + }> + ): Promise<{ state: CodeReviewState; archivePath: string; result: T }> { + return this.#withLock(prUrl, async () => { + const state = await this.#requireActive(prUrl); + if (state.mergedReview === undefined) { + throw new Error("Code review must be merged before publishing."); + } + if (hasUnresolvedPublicationClaim(state)) { + throw new Error( + "Code review publication may already have reached GitHub; refusing to submit a duplicate review without a persisted receipt." + ); + } + const claimed = await this.#save({ + ...state, + orchestratorActions: [ + ...state.orchestratorActions, + { at: this.#timestamp(), action: "publication_started" } + ] + }); + let publication: Awaited>; + try { + publication = await publish(state); + } catch (error) { + await this.#save({ + ...claimed, + orchestratorActions: [ + ...claimed.orchestratorActions, + { at: this.#timestamp(), action: "publication_failed" } + ] + }); + throw error; + } + return { + ...(await this.#commitState(claimed, publication.receipt)), + result: publication.result + }; + }); + } + + async resumePublished( + prUrl: string + ): Promise<{ state: CodeReviewState; archivePath: string } | undefined> { + return this.#withLock(prUrl, async () => { + const active = await this.read(prUrl); + if (active?.state === "published" && active.published) { + await ensureDirectory(this.archiveDirectory); + return { + state: active, + archivePath: await this.#archive(prUrl, active.published.publishedAt) + }; + } + if (active) { + return undefined; + } + return this.#findArchivedPublication(prUrl); + }); + } + + async #require(prUrl: string): Promise { + const state = await this.read(prUrl); + if (!state) { + throw new Error(`Code review not found for pull request: ${prUrl}`); + } + return state; + } + + async #requireActive(prUrl: string): Promise { + const state = await this.#require(prUrl); + if (state.state === "published") { + throw new Error("Published code reviews cannot be modified."); + } + return state; + } + + async #save(state: CodeReviewState): Promise { + const updated = { + ...state, + timestamps: { ...state.timestamps, updatedAt: this.#timestamp() } + }; + await writeAtomically( + this.pathForPullRequest(updated.prUrl), + serializeCodeReviewState(updated) + ); + return updated; + } + + async #commitState( + state: CodeReviewState, + receipt: Omit & { + publishedAt?: string; + } + ): Promise<{ state: CodeReviewState; archivePath: string }> { + const publishedAt = receipt.publishedAt ?? this.#timestamp(); + const publishedState = { + ...state, + state: "published", + timestamps: { + ...state.timestamps, + updatedAt: publishedAt, + publishedAt + }, + published: { ...receipt, publishedAt } + } satisfies CodeReviewState; + await writeAtomically( + this.pathForPullRequest(publishedState.prUrl), + serializeCodeReviewState(publishedState) + ); + await ensureDirectory(this.archiveDirectory); + const archivePath = await this.#archive(state.prUrl, publishedAt); + return { state: publishedState, archivePath }; + } + + async #updateActive( + prUrl: string, + update: (state: CodeReviewState) => CodeReviewState + ): Promise { + return this.#withLock(prUrl, async () => this.#save(update(await this.#requireActive(prUrl)))); + } + + async #withLock(prUrl: string, operation: () => Promise): Promise { + await ensureDirectory(this.directory); + return withFileLock(`${this.pathForPullRequest(prUrl)}.lock`, this.#lockTimeoutMs, operation); + } + + async #archive(prUrl: string, publishedAt: string): Promise { + const sourcePath = this.pathForPullRequest(prUrl); + await assertRegularFile(sourcePath); + const filename = codeReviewFileName(prUrl); + const suffix = publishedAt.replace(/[^0-9]/g, ""); + const stem = filename.slice(0, -".yaml".length); + let sequence = 0; + while (true) { + const candidate = + sequence === 0 + ? containedPath(this.archiveDirectory, filename) + : containedPath( + this.archiveDirectory, + `${stem}_${sequence === 1 ? suffix : `${suffix}_${sequence - 1}`}.yaml` + ); + try { + await link(sourcePath, candidate); + await unlink(sourcePath); + return candidate; + } catch (error) { + if (!isExistingFileError(error)) { + throw error; + } + if (await samePublishedState(sourcePath, candidate)) { + await unlink(sourcePath); + return candidate; + } + } + sequence += 1; + } + } + + async #findArchivedPublication( + prUrl: string + ): Promise<{ state: CodeReviewState; archivePath: string } | undefined> { + let names: string[]; + try { + names = await readdir(this.archiveDirectory); + } catch (error) { + if (isMissingFileError(error)) return undefined; + throw error; + } + const stem = codeReviewFileName(prUrl).slice(0, -".yaml".length); + const candidates: Array<{ state: CodeReviewState; archivePath: string }> = []; + for (const name of names) { + if (name !== `${stem}.yaml` && !name.startsWith(`${stem}_`)) continue; + const archivePath = containedPath(this.archiveDirectory, name); + try { + const state = parseCodeReviewState(await readRegularFile(archivePath), archivePath); + if (state.prUrl === canonicalPullRequestUrl(prUrl) && state.state === "published") { + candidates.push({ state, archivePath }); + } + } catch { + // Ignore unrelated archive collisions while recovering a receipt. + } + } + return candidates.sort((left, right) => + right.state.timestamps.updatedAt.localeCompare(left.state.timestamps.updatedAt) + )[0]; + } + + #timestamp(): string { + return this.#now().toISOString(); + } +} + +async function samePublishedState(sourcePath: string, archivePath: string): Promise { + try { + const source = parseCodeReviewState(await readRegularFile(sourcePath), sourcePath); + const archived = parseCodeReviewState(await readRegularFile(archivePath), archivePath); + return ( + source.state === "published" && + archived.state === "published" && + serializeCodeReviewState(source) === serializeCodeReviewState(archived) + ); + } catch { + return false; + } +} + + +function hasUnresolvedPublicationClaim(state: CodeReviewState): boolean { + for (let index = state.orchestratorActions.length - 1; index >= 0; index -= 1) { + const action = state.orchestratorActions[index]?.action; + if (action === "publication_started") return true; + if (action === "publication_failed") return false; + } + return false; +} + +async function withFileLock( + lockPath: string, + lockTimeoutMs: number, + operation: () => Promise +): Promise { + const startedAt = Date.now(); + let lock: Awaited>; + while (true) { + try { + lock = await open(lockPath, "wx"); + await lock.writeFile(`${process.pid}\n`, "utf8"); + break; + } catch (error) { + if (!isExistingFileError(error)) { + throw error; + } + if (await isStaleLock(lockPath, lockTimeoutMs)) { + await unlink(lockPath).catch(() => undefined); + continue; + } + if (Date.now() - startedAt >= lockTimeoutMs) { + throw new Error(`Timed out waiting for code review lock: ${lockPath}`); + } + await new Promise((resolve) => setTimeout(resolve, 5)); + } + } + try { + return await operation(); + } finally { + await lock.close(); + await unlink(lockPath).catch(() => undefined); + } +} + +async function isStaleLock(lockPath: string, lockTimeoutMs: number): Promise { + try { + const ownerPid = Number.parseInt((await readFile(lockPath, "utf8")).trim(), 10); + if (Number.isSafeInteger(ownerPid) && ownerPid > 0) { + return !isRunningProcess(ownerPid); + } + return Date.now() - (await stat(lockPath)).mtimeMs >= lockTimeoutMs; + } catch (error) { + if (isMissingFileError(error)) { + return false; + } + throw error; + } +} + +function isRunningProcess(processId: number): boolean { + try { + process.kill(processId, 0); + return true; + } catch (error) { + return !( + typeof error === "object" && + error !== null && + "code" in error && + error.code === "ESRCH" + ); + } +} + +async function writeAtomically(filePath: string, content: string): Promise { + await ensureDirectory(dirname(filePath)); + const temporaryPath = containedPath( + dirname(filePath), + `.${basename(filePath)}.${randomUUID()}.tmp` + ); + let temporary: Awaited> | undefined; + try { + temporary = await open( + temporaryPath, + constants.O_CREAT | constants.O_EXCL | constants.O_WRONLY + ); + await temporary.writeFile(content, "utf8"); + await temporary.sync(); + await temporary.close(); + temporary = undefined; + await rename(temporaryPath, filePath); + const parent = await open(dirname(filePath), constants.O_RDONLY); + try { + await parent.sync(); + } finally { + await parent.close(); + } + } catch (error) { + await temporary?.close().catch(() => undefined); + await unlink(temporaryPath).catch(() => undefined); + throw error; + } +} + +async function ensureDirectory(directory: string): Promise { + const absoluteDirectory = resolve(directory); + let currentDirectory = parse(absoluteDirectory).root; + for (const segment of relative(currentDirectory, absoluteDirectory).split(sep).filter(Boolean)) { + currentDirectory = resolve(currentDirectory, segment); + try { + await mkdir(currentDirectory); + } catch (error) { + if (!isExistingFileError(error)) { + throw error; + } + } + const status = await lstat(currentDirectory); + if (!status.isDirectory()) { + throw new Error(`Code review store path is not a regular directory: ${currentDirectory}`); + } + } +} + +async function assertDirectoryPath(directory: string): Promise { + const absoluteDirectory = resolve(directory); + let currentDirectory = parse(absoluteDirectory).root; + for (const segment of relative(currentDirectory, absoluteDirectory).split(sep).filter(Boolean)) { + currentDirectory = resolve(currentDirectory, segment); + const status = await lstat(currentDirectory); + if (!status.isDirectory()) { + throw new Error(`Code review store path is not a regular directory: ${currentDirectory}`); + } + } +} + +function containedPath(directory: string, name: string): string { + if (!name || isAbsolute(name)) { + throw new Error(`Code review path name is unsafe: ${name}`); + } + const root = resolve(directory); + const target = resolve(root, name); + const fromRoot = relative(root, target); + if (!fromRoot || fromRoot.startsWith("..") || fromRoot.startsWith(sep)) { + throw new Error(`Code review path escapes store directory: ${target}`); + } + return target; +} + +async function pathExists(path: string): Promise { + try { + await access(path); + return true; + } catch (error) { + if (isMissingFileError(error)) { + return false; + } + throw error; + } +} + +async function readRegularFile(filePath: string): Promise { + const handle = await open(filePath, constants.O_RDONLY | (constants.O_NOFOLLOW ?? 0)); + try { + if (!(await handle.stat()).isFile()) { + throw new Error(`Code review state path is not a regular file: ${filePath}`); + } + return await handle.readFile("utf8"); + } finally { + await handle.close(); + } +} + +async function assertRegularFile(filePath: string): Promise { + if (!(await lstat(filePath)).isFile()) { + throw new Error(`Code review state path is not a regular file: ${filePath}`); + } +} + +function requirePullRequestRef(prUrl: string) { + const ref = parseGitHubPullRequestRef(prUrl); + if (!ref) { + throw new Error(`Invalid GitHub pull request URL: ${prUrl}`); + } + requireSafeDocumentSegment(ref.owner, "Pull request owner"); + requireSafeDocumentSegment(ref.repo, "Pull request repository"); + return ref; +} + +function requireActor(actor: string): string { + return requireSafeDocumentSegment(actor, "Code review actor"); +} + +function requireMergedReview(state: CodeReviewState): CodeReviewDraft { + if (state.mergedReview === undefined) { + throw new Error("Code review has no merged draft to modify."); + } + return state.mergedReview; +} + +function requireInlineCommentIndex(index: number, commentCount: number): number { + if (!Number.isSafeInteger(index) || index < 0 || index >= commentCount) { + throw new Error(`Merged review inline comment index is out of range: ${index}`); + } + return index; +} + +function safeFilePart(part: string): string { + return filesystemSafeNamePart(part, "Code review filename segment"); +} + +function isMissingFileError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "ENOENT"; +} + +function isExistingFileError(error: unknown): boolean { + return typeof error === "object" && error !== null && "code" in error && error.code === "EEXIST"; +} diff --git a/packages/agent-code-review/src/review.test.ts b/packages/agent-code-review/src/review.test.ts new file mode 100644 index 000000000..a7b166c23 --- /dev/null +++ b/packages/agent-code-review/src/review.test.ts @@ -0,0 +1,69 @@ +import { resolve } from "node:path"; +import { describe, expect, it, vi } from "vitest"; +import type { CodeReviewYamlStore } from "./review-store.js"; +import { createCodeReviewState } from "./review-store.js"; +import { runCodeReview } from "./review.js"; + +const assetMocks = vi.hoisted(() => ({ + loadProfile: vi.fn(async () => "custom profile"), + loadPrompt: vi.fn(async () => "custom prompt") +})); + +vi.mock("./assets.js", async (importOriginal) => { + const actual = await importOriginal(); + return { + ...actual, + discoverCodeReviewProfiles: vi.fn(async () => [{ + name: "generic", + content: "generic profile", + source: "built-in" as const + }]), + loadCodeReviewProfile: assetMocks.loadProfile, + loadCodeReviewPrompt: assetMocks.loadPrompt + }; +}); + +const prUrl = "https://github.com/acme/widgets/pull/123"; + +function createStore(): CodeReviewYamlStore { + const state = { + ...createCodeReviewState({ + sessionId: "session-1", + prUrl, + selectedAgent: "codex", + selectedProfiles: ["generic"] + }), + mergedReview: { body: "Looks good.", comments: [] } + }; + return { + startRun: vi.fn(async () => state), + appendOrchestratorAction: vi.fn(async () => state), + read: vi.fn(async () => state) + } as unknown as CodeReviewYamlStore; +} + +describe("runCodeReview asset paths", () => { + it("resolves explicit profile and prompt paths relative to cwd", async () => { + const cwd = "/repo/worktree"; + + await runCodeReview( + { prUrl, cwd, profilePath: "profiles/security.md", promptPath: "prompts/review.md" }, + { + resolveOptions: async (input) => ({ + ...input, + agent: "codex", + draftStore: ".poe-code/code-review/reviews", + humanGate: { provider: "none" } + }), + fetchPr: async () => ({}), + fetchDiff: async () => "", + fetchComments: async () => ({}), + store: createStore(), + spawnAgent: async () => ({ stdout: "", stderr: "", exitCode: 0 }) + } + ); + + expect(assetMocks.loadProfile).toHaveBeenCalledWith(resolve(cwd, "profiles/security.md")); + expect(assetMocks.loadPrompt).toHaveBeenCalledWith(resolve(cwd, "prompts/review.md")); + }); +}); diff --git a/packages/agent-code-review/src/review.ts b/packages/agent-code-review/src/review.ts new file mode 100644 index 000000000..6cf14f6af --- /dev/null +++ b/packages/agent-code-review/src/review.ts @@ -0,0 +1,240 @@ +import { randomUUID } from "node:crypto"; +import { resolve } from "node:path"; +import { + canonicalPullRequestUrl, + fetchPullRequestDetails, + fetchPullRequestDiff, + fetchPullRequestReviewActivity +} from "github-review"; +import { type SpawnOptions, type SpawnResult, spawn } from "@poe-code/agent-spawn"; +import { + CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT, + discoverCodeReviewProfiles, + loadCodeReviewProfile, + loadCodeReviewPrompt, + loadCodeReviewRolePrompt +} from "./assets.js"; +import { + type CodeReviewRunInput, + type CodeReviewRunOptions, + loadDefaultPoeCodeAgent, + resolveCodeReviewRuntimeOptions +} from "./config.js"; +import { createCodeReviewAgentMcpConfig } from "./mcp.js"; +import type { CodeReviewState } from "./review-state.js"; +import { CodeReviewYamlStore, resolveCodeReviewStoreDirectory } from "./review-store.js"; + +export interface CodeReviewOrchestrationInput extends CodeReviewRunInput { + prompt?: string; + profile?: string; +} + +export interface CodeReviewOrchestrationDependencies { + resolveOptions?: ( + input: CodeReviewRunInput + ) => CodeReviewRunOptions | Promise; + fetchPr?: ( + prUrl: string, + cwd?: string + ) => Record | Promise>; + fetchDiff?: (prUrl: string, cwd?: string) => string | Promise; + fetchComments?: ( + prUrl: string, + cwd?: string + ) => Record | Promise>; + resolveAgent?: () => string | undefined | Promise; + createSessionId?: () => string; + store?: CodeReviewYamlStore; + spawnAgent?: ( + agent: string, + prompt: string, + options: Omit + ) => Promise; +} + +export interface CodeReviewResult { + sessionId: string; + state: CodeReviewState; + prompt: string; + spawnResult: SpawnResult; +} + +export async function runCodeReview( + input: CodeReviewOrchestrationInput, + dependencies: CodeReviewOrchestrationDependencies = {} +): Promise { + const unresolvedOptions = await (dependencies.resolveOptions ?? resolveCodeReviewRuntimeOptions)( + input + ); + const options = { + ...unresolvedOptions, + cwd: resolve(unresolvedOptions.cwd), + prUrl: canonicalPullRequestUrl(unresolvedOptions.prUrl) + }; + const agent = + options.agent ?? + (await (dependencies.resolveAgent + ? dependencies.resolveAgent() + : loadDefaultPoeCodeAgent({ cwd: options.cwd }))); + if (!agent?.trim()) { + throw new Error( + "No code-review agent resolved; configure codeReview.agent or the normal poe-code core.defaultAgent / POE_DEFAULT_AGENT setting." + ); + } + const profiles = await discoverCodeReviewProfiles({ + cwd: options.cwd, + filters: options.profiles + }); + const [profile, promptTemplate, prDetails, diff, priorActivity] = await Promise.all([ + resolveProfile(input, options, profiles), + resolvePrompt(input, options), + (dependencies.fetchPr ?? fetchPrWithCwd)(options.prUrl, options.cwd), + (dependencies.fetchDiff ?? fetchDiffWithCwd)(options.prUrl, options.cwd), + (dependencies.fetchComments ?? fetchCommentsWithCwd)(options.prUrl, options.cwd) + ]); + const requestedSessionId = options.sessionId ?? dependencies.createSessionId?.() ?? randomUUID(); + const draftStore = absoluteDraftStore(options); + const store = dependencies.store ?? new CodeReviewYamlStore({ directory: draftStore }); + const activeRun = await store.startRun({ + sessionId: requestedSessionId, + prUrl: options.prUrl, + selectedAgent: agent, + selectedProfiles: profiles.map(({ name }) => name) + }); + const sessionId = activeRun.sessionId; + await store.appendOrchestratorAction(options.prUrl, { + action: "spawned_orchestrator", + details: agent + }); + const prompt = renderReviewPrompt({ + input: options, + profiles, + profile, + promptTemplate, + prDetails, + diff, + priorActivity + }); + const mcpConfig = createCodeReviewAgentMcpConfig({ + role: "orchestrator", + session: sessionId, + actor: "orchestrator", + cwd: options.cwd, + draftStore, + agent, + ...(options.profiles ? { profiles: options.profiles } : {}) + }); + const spawnResult = await (dependencies.spawnAgent ?? spawnWithPoeCode)(agent, prompt, { + cwd: options.cwd, + mcpServers: { + "code-review": { + command: mcpConfig.command, + args: mcpConfig.args + } + } + }); + if (spawnResult.exitCode !== 0) { + throw new Error( + `Code-review orchestrator failed: ${spawnResult.stderr || `exit ${spawnResult.exitCode}`}` + ); + } + const state = await store.read(options.prUrl); + if (!state?.mergedReview) { + throw new Error("Code-review orchestrator must create exactly one merged review."); + } + await store.appendOrchestratorAction(options.prUrl, { + action: "completed_orchestration" + }); + return { + sessionId, + state: (await store.read(options.prUrl)) ?? state, + prompt, + spawnResult + }; +} + +async function resolveProfile( + input: CodeReviewOrchestrationInput, + options: CodeReviewRunOptions, + profiles: Awaited> +): Promise { + if (input.profile !== undefined) { + return input.profile; + } + if (options.profilePath) { + return loadCodeReviewProfile(resolve(options.cwd, options.profilePath)); + } + return profiles.map((profile) => `## ${profile.name}\n\n${profile.content.trim()}`).join("\n\n"); +} + +async function resolvePrompt( + input: CodeReviewOrchestrationInput, + options: CodeReviewRunOptions +): Promise { + if (input.prompt !== undefined) { + return input.prompt; + } + return options.promptPath + ? loadCodeReviewPrompt(resolve(options.cwd, options.promptPath)) + : loadCodeReviewRolePrompt({ cwd: options.cwd, role: "orchestrator" }); +} + +function renderReviewPrompt(input: { + input: CodeReviewRunOptions; + profiles: Awaited>; + profile: string; + promptTemplate: string; + prDetails: Record; + diff: string; + priorActivity: Record; +}): string { + const additionalFeedback = input.input.additionalFeedback?.trim(); + return [ + input.promptTemplate, + ORCHESTRATOR_WORKFLOW_PROMPT, + `\nPROFILE CARDS\n${input.profiles + .map((profile) => `## ${profile.name}\n\n${profile.content.trim()}`) + .join("\n\n")}`, + `\nPRIMARY REVIEW PROFILE\n${input.profile}`, + additionalFeedback ? `\nADDITIONAL FEEDBACK\n${additionalFeedback}` : "", + `\nPULL REQUEST\n${input.input.prUrl}\n${JSON.stringify(input.prDetails)}`, + `\nPRIOR COMMENTS AND REVIEWS\n${JSON.stringify(input.priorActivity)}`, + `\nDIFF SUMMARY\n${input.diff}` + ] + .filter(Boolean) + .join("\n"); +} + +const ORCHESTRATOR_WORKFLOW_PROMPT = ` +REQUIRED ORCHESTRATION FLOW +1. Treat PRIOR COMMENTS AND REVIEWS as already-raised concerns and do not repeat the same underlying finding. +2. Use code_review_profile_list and spawn useful reviewer profiles through code_review_agent_spawn; spawn at least one available profile unless none can evaluate the change. +3. code_review_agent_spawn is asynchronous. Poll code_review_agent_status until all spawned reviewers are completed or failed before reading drafts. +4. Read completed raw reviews with code_review_list_drafts, merge useful new findings, and deduplicate overlapping or previously reported concerns. +5. Create exactly one final merged review with code_review_create_draft, even when there are no new findings. +6. Do not publish reviews or expose orchestration details in the final review text. +7. ${CODE_REVIEW_USER_FACING_OUTPUT_CONTRACT}`; + +function absoluteDraftStore(input: CodeReviewRunOptions): string { + return resolveCodeReviewStoreDirectory(input.cwd, input.draftStore); +} + +function fetchPrWithCwd(prUrl: string, cwd: string): Record { + return fetchPullRequestDetails(prUrl, undefined, { cwd }); +} + +function fetchDiffWithCwd(prUrl: string, cwd: string): string { + return fetchPullRequestDiff(prUrl, { cwd }); +} + +function fetchCommentsWithCwd(prUrl: string, cwd: string): Record { + return fetchPullRequestReviewActivity(prUrl, { cwd }); +} + +async function spawnWithPoeCode( + agent: string, + prompt: string, + options: Omit +): Promise { + return spawn(agent, { prompt, ...options }); +} diff --git a/packages/agent-code-review/tsconfig.json b/packages/agent-code-review/tsconfig.json new file mode 100644 index 000000000..44b01ee75 --- /dev/null +++ b/packages/agent-code-review/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../tsconfig.json", + "compilerOptions": { + "outDir": "dist", + "rootDir": "src", + "noEmit": false, + "declaration": true + }, + "include": ["src"], + "exclude": ["**/*.test.ts"] +} diff --git a/packages/github-review/README.md b/packages/github-review/README.md new file mode 100644 index 000000000..382a87936 --- /dev/null +++ b/packages/github-review/README.md @@ -0,0 +1,51 @@ +# `github-review` + +Reusable SDK for GitHub pull request and review mechanics backed exclusively by the GitHub CLI (`gh`). It provides the GitHub transport and validation layer for code-review workflows, and intentionally contains no agent, profile, session, draft-store, or orchestration behavior. + +## GitHub CLI authentication + +Every live operation shells out to `gh`; authenticate the GitHub CLI before calling the SDK: + +```sh +gh auth login +gh auth status +``` + +For non-interactive CI, provide a token for `gh`, for example `GH_TOKEN`, with access to the repository and the review operations being performed. The SDK does not parse token environment variables itself: authentication remains the responsibility of `gh` and its configured environment. + +## Exports + +- `parseGitHubPullRequestRef(prUrl)` and `canonicalPullRequestUrl(prUrl)` parse pull request URLs. +- `filesystemSafeNamePart(value)` normalizes GitHub-derived names for local artifact paths. +- `defaultCommandRunner`, `ghPrView`, `ghPrDiff`, `ghApiJson`, and `ghApiPaginatedJsonArray` are the injectable `gh` execution primitives. +- `fetchPullRequestMetadata`, `fetchPullRequestDetails`, `fetchPullRequestDiff`, `fetchPullRequestComments`, and `fetchPullRequestReviews` read pull request data through `gh pr view` and `gh pr diff`. +- `fetchPullRequestReviewComments` reads inline review comments through paginated `gh api`; `fetchPullRequestReviewActivity` combines these with conversation comments, submitted reviews, and review-thread data when supported by the installed `gh` version. +- `fetchReviewHistory({ username, repos, since, maxComments, onRateLimit })` asynchronously yields normalized authored inline comments, submitted review bodies, and pull request conversation comments for profile ingest. +- `parseReviewDiff(diff)` exposes right-side `reviewableLines` and `validateInlineComments(comments, context)` rejects invalid inline targets. +- `submitPullRequestReview`, `editPullRequestReviewComment`, and `deletePullRequestReviewComment` write live GitHub review data through `gh api`. +- `GitHubRateLimitError` and `parseGitHubApiResponse` support rate-aware callers handling GitHub API responses. + +## Environment and configuration + +The SDK reads no environment variables and no configuration files of its own. Repository access, authentication, and rate-limit policy are determined by the `gh` CLI environment provided by the host process. + +## Testing and embedding + +All functions that invoke `gh` accept a `runner` option, enabling tests and hosts to inject command execution without invoking a real CLI: + +```ts +import { fetchPullRequestMetadata, type CommandRunner } from "github-review"; + +const runner: CommandRunner = (_command, _args) => ({ + code: 0, + stdout: JSON.stringify({ number: 123, title: "Review me" }), + stderr: "", +}); + +const pullRequest = fetchPullRequestMetadata( + "https://github.com/acme/widgets/pull/123", + { runner }, +); +``` + +Review-history ingest uses `gh api --include` to parse GitHub pagination and rate-limit headers. When remaining API requests fall to ten or fewer, it calls `onRateLimit` when provided and throws `GitHubRateLimitError` with a resumable endpoint and partial-result count so callers can stop and resume cleanly. diff --git a/packages/github-review/package.json b/packages/github-review/package.json new file mode 100644 index 000000000..fa3cd4b86 --- /dev/null +++ b/packages/github-review/package.json @@ -0,0 +1,22 @@ +{ + "name": "github-review", + "version": "0.1.0", + "private": true, + "type": "module", + "main": "dist/index.js", + "types": "dist/index.d.ts", + "exports": { + ".": { + "types": "./dist/index.d.ts", + "import": "./dist/index.js" + } + }, + "scripts": { + "build": "tsc", + "test": "cd ../.. && vitest run packages/github-review/src", + "test:unit": "cd ../.. && vitest run packages/github-review/src" + }, + "files": [ + "dist" + ] +} diff --git a/packages/github-review/src/command.test.ts b/packages/github-review/src/command.test.ts new file mode 100644 index 000000000..598d30f8a --- /dev/null +++ b/packages/github-review/src/command.test.ts @@ -0,0 +1,19 @@ +import { spawnSync } from "node:child_process"; +import { describe, expect, it, vi } from "vitest"; +import { defaultCommandRunner } from "./command.js"; + +vi.mock("node:child_process", () => ({ + spawnSync: vi.fn(() => ({ status: 0, stdout: "diff", stderr: "" })) +})); + +describe("defaultCommandRunner", () => { + it("allows large GitHub CLI output buffers for PR diffs", () => { + defaultCommandRunner("gh", ["pr", "diff", "123"]); + + expect(vi.mocked(spawnSync)).toHaveBeenCalledWith( + "gh", + ["pr", "diff", "123"], + expect.objectContaining({ maxBuffer: 50 * 1024 * 1024 }) + ); + }); +}); diff --git a/packages/github-review/src/command.ts b/packages/github-review/src/command.ts new file mode 100644 index 000000000..34eb5926b --- /dev/null +++ b/packages/github-review/src/command.ts @@ -0,0 +1,35 @@ +import { type SpawnSyncReturns, spawnSync } from "node:child_process"; + +export interface CommandResult { + code: number; + stdout: string; + stderr: string; +} + +export interface CommandRunnerOptions { + cwd?: string; + input?: string; +} + +const COMMAND_OUTPUT_MAX_BUFFER = 50 * 1024 * 1024; + +export type CommandRunner = ( + command: string, + args: readonly string[], + options?: CommandRunnerOptions +) => CommandResult; + +export const defaultCommandRunner: CommandRunner = (command, args, options = {}) => { + const result: SpawnSyncReturns = spawnSync(command, args, { + cwd: options.cwd, + input: options.input, + encoding: "utf-8", + maxBuffer: COMMAND_OUTPUT_MAX_BUFFER + }); + + return { + code: result.status ?? 1, + stdout: result.stdout ?? "", + stderr: result.stderr ?? "" + }; +}; diff --git a/packages/github-review/src/diff.ts b/packages/github-review/src/diff.ts new file mode 100644 index 000000000..6512d8ee0 --- /dev/null +++ b/packages/github-review/src/diff.ts @@ -0,0 +1,276 @@ +export interface ReviewInlineComment { + path: string; + line: number; + body: string; +} + +export interface ReviewDiffLine { + side: "LEFT" | "RIGHT"; + line: number; + text: string; +} + +export interface ReviewDiffHunk { + header: string; + lines: ReviewDiffLine[]; +} + +export interface ReviewDiffFile { + path: string; + previousPath: string | null; + status: "modified" | "added" | "deleted" | "renamed"; + hunks: ReviewDiffHunk[]; + reviewableLines: number[]; +} + +export interface ReviewDiffContext { + files: ReviewDiffFile[]; + fileOrder: Map; + reviewableLinesByPath: Map>; + rendered: string; +} + +function parseHunkHeader(header: string): { oldLine: number; newLine: number } | null { + const match = /^@@ -(\d+)(?:,\d+)? \+(\d+)(?:,\d+)? @@/.exec(header); + if (!match) { + return null; + } + return { + oldLine: Number.parseInt(match[1] ?? "0", 10), + newLine: Number.parseInt(match[2] ?? "0", 10) + }; +} + +function decodeGitQuotedPath(value: string): string { + if (!value.startsWith('"') || !value.endsWith('"')) { + return value; + } + + const bytes: number[] = []; + const contents = value.slice(1, -1); + for (let index = 0; index < contents.length; index += 1) { + const character = contents[index] ?? ""; + if (character !== "\\") { + bytes.push(...new TextEncoder().encode(character)); + continue; + } + + const escaped = contents[index + 1]; + if (escaped === undefined) { + bytes.push(92); + continue; + } + if (/[0-7]/.test(escaped)) { + const octal = contents.slice(index + 1).match(/^[0-7]{1,3}/)?.[0] ?? ""; + bytes.push(Number.parseInt(octal, 8)); + index += octal.length; + continue; + } + const escapedBytes: Record = { + b: 8, + t: 9, + n: 10, + v: 11, + f: 12, + r: 13, + '"': 34, + "\\": 92 + }; + bytes.push(escapedBytes[escaped] ?? escaped.charCodeAt(0)); + index += 1; + } + return new TextDecoder().decode(new Uint8Array(bytes)); +} + +function normalizeDiffPath(value: string): string | null { + const unquoted = decodeGitQuotedPath(value); + if (unquoted === "/dev/null") { + return null; + } + return unquoted.startsWith("a/") || unquoted.startsWith("b/") ? unquoted.slice(2) : unquoted; +} + +function parseDiffGitPaths(line: string): { previousPath: string; path: string } | null { + const plain = /^diff --git a\/(.+) b\/(.+)$/.exec(line); + if (plain) { + return { previousPath: plain[1] ?? "", path: plain[2] ?? "" }; + } + const quoted = /^diff --git "a\/(.+)" "b\/(.+)"$/.exec(line); + return quoted + ? { + previousPath: decodeGitQuotedPath(`"${quoted[1] ?? ""}"`), + path: decodeGitQuotedPath(`"${quoted[2] ?? ""}"`) + } + : null; +} + +function inferStatus(line: string): ReviewDiffFile["status"] | null { + if (line.startsWith("new file mode ")) { + return "added"; + } + if (line.startsWith("deleted file mode ")) { + return "deleted"; + } + if (line.startsWith("rename from ") || line.startsWith("rename to ")) { + return "renamed"; + } + return null; +} + +function renderReviewFile(file: ReviewDiffFile): string { + const header = [ + `FILE: ${file.path}`, + `STATUS: ${file.status}${file.previousPath && file.previousPath !== file.path ? ` (from ${file.previousPath})` : ""}` + ]; + if (file.hunks.length === 0) { + return `${header.join("\n")}\nPATCH: (no text patch available)`; + } + const lines = [...header, "PATCH:"]; + for (const hunk of file.hunks) { + lines.push(hunk.header); + for (const line of hunk.lines) { + const label = `${line.side === "RIGHT" ? "R" : "L"}${line.line}`.padStart(6, " "); + lines.push(`${label} | ${line.text}`); + } + } + return lines.join("\n"); +} + +export function parseReviewDiff(diffText: string): ReviewDiffContext { + type PendingFile = ReviewDiffFile & { reviewableLineSet: Set }; + const files: PendingFile[] = []; + let currentFile: PendingFile | null = null; + let currentHunk: ReviewDiffHunk | null = null; + let oldLine = 0; + let newLine = 0; + + const finalizeFile = (): void => { + if (currentFile) { + files.push(currentFile); + } + currentFile = null; + currentHunk = null; + }; + + for (const line of diffText.split(/\r?\n/)) { + if (line.startsWith("diff --git ")) { + finalizeFile(); + const paths = parseDiffGitPaths(line); + const previousPath = paths?.previousPath ?? ""; + const path = paths?.path ?? previousPath; + currentFile = { + path, + previousPath: previousPath && previousPath !== path ? previousPath : null, + status: "modified", + hunks: [], + reviewableLines: [], + reviewableLineSet: new Set() + }; + continue; + } + if (!currentFile) { + continue; + } + const status = inferStatus(line); + if (status) { + currentFile.status = status; + } + if (line.startsWith("rename from ")) { + currentFile.previousPath = decodeGitQuotedPath(line.slice("rename from ".length).trim()); + continue; + } + if (line.startsWith("rename to ")) { + currentFile.path = decodeGitQuotedPath(line.slice("rename to ".length).trim()); + continue; + } + if (line.startsWith("--- ")) { + currentFile.previousPath = normalizeDiffPath(line.slice(4).trim()); + continue; + } + if (line.startsWith("+++ ")) { + const path = normalizeDiffPath(line.slice(4).trim()); + if (path) { + currentFile.path = path; + } + continue; + } + if (line.startsWith("@@ ")) { + const parsed = parseHunkHeader(line); + if (!parsed) { + continue; + } + oldLine = parsed.oldLine; + newLine = parsed.newLine; + currentHunk = { header: line, lines: [] }; + currentFile.hunks.push(currentHunk); + continue; + } + if (!currentHunk) { + continue; + } + if (line.startsWith("+") && !line.startsWith("+++")) { + currentHunk.lines.push({ side: "RIGHT", line: newLine, text: line }); + currentFile.reviewableLineSet.add(newLine); + newLine += 1; + continue; + } + if (line.startsWith("-") && !line.startsWith("---")) { + currentHunk.lines.push({ side: "LEFT", line: oldLine, text: line }); + oldLine += 1; + continue; + } + if (line.startsWith(" ")) { + currentHunk.lines.push({ side: "RIGHT", line: newLine, text: line }); + currentFile.reviewableLineSet.add(newLine); + oldLine += 1; + newLine += 1; + } + } + finalizeFile(); + + const publicFiles = files.map((file) => ({ + path: file.path, + previousPath: file.previousPath, + status: file.status, + hunks: file.hunks, + reviewableLines: [...file.reviewableLineSet].sort((left, right) => left - right) + })); + return { + files: publicFiles, + fileOrder: new Map(publicFiles.map((file, index) => [file.path, index])), + reviewableLinesByPath: new Map(files.map((file) => [file.path, file.reviewableLineSet])), + rendered: publicFiles.map(renderReviewFile).join("\n\n") + }; +} + +export function validateInlineComments( + comments: readonly ReviewInlineComment[], + context: ReviewDiffContext +): ReviewInlineComment[] { + const accepted: ReviewInlineComment[] = []; + const seen = new Set(); + for (const comment of comments) { + const path = comment.path.trim(); + const body = comment.body.trim(); + if (!path || !body || !Number.isInteger(comment.line) || comment.line < 1) { + throw new Error("Inline review comments require path, positive line, and body."); + } + if (!context.reviewableLinesByPath.get(path)?.has(comment.line)) { + throw new Error( + `Inline comment target ${path}:${comment.line} is not a valid right-side diff target.` + ); + } + const key = `${path}\0${comment.line}\0${body}`; + if (!seen.has(key)) { + seen.add(key); + accepted.push({ path, line: comment.line, body }); + } + } + accepted.sort((left, right) => { + const fileDifference = + (context.fileOrder.get(left.path) ?? Number.MAX_SAFE_INTEGER) - + (context.fileOrder.get(right.path) ?? Number.MAX_SAFE_INTEGER); + return fileDifference || left.line - right.line; + }); + return accepted; +} diff --git a/packages/github-review/src/filesystem-name.ts b/packages/github-review/src/filesystem-name.ts new file mode 100644 index 000000000..92112a87a --- /dev/null +++ b/packages/github-review/src/filesystem-name.ts @@ -0,0 +1,16 @@ +const SAFE_FILESYSTEM_NAME_RE = /^[A-Za-z0-9._-]+$/; + +export function filesystemSafeNamePart(value: string, field: string): string { + if ( + !value || + value.trim() !== value || + value.normalize("NFKC") !== value || + value.startsWith(".") || + value === "." || + value === ".." || + !SAFE_FILESYSTEM_NAME_RE.test(value) + ) { + throw new Error(`${field} must be a safe filesystem name.`); + } + return value.toLowerCase().replaceAll("_", "%5f"); +} diff --git a/packages/github-review/src/gh.ts b/packages/github-review/src/gh.ts new file mode 100644 index 000000000..9c115239c --- /dev/null +++ b/packages/github-review/src/gh.ts @@ -0,0 +1,196 @@ +import { type CommandRunner, defaultCommandRunner } from "./command.js"; +import { + type GitHubApiResponse, + GitHubRateLimitError, + nextGitHubEndpoint, + parseGitHubApiResponse, + rateLimitStatus +} from "./github-api.js"; +import { parseGitHubPullRequestRef } from "./pr-url.js"; + +export interface GitHubCliOptions { + cwd?: string; + runner?: CommandRunner; +} + +function requirePullRequestRef(prUrl: string) { + const ref = parseGitHubPullRequestRef(prUrl); + if (!ref) { + throw new Error(`GitHub PR URL is required: ${prUrl}`); + } + return ref; +} + +function runGh(args: string[], options: GitHubCliOptions & { input?: string } = {}) { + const runner = options.runner ?? defaultCommandRunner; + return runner("gh", args, { cwd: options.cwd, input: options.input }); +} + +function runGhOrThrow(args: string[], options: GitHubCliOptions & { input?: string } = {}): string { + const result = runGh(args, options); + if (result.code !== 0) { + throw new Error(result.stderr.trim() || result.stdout.trim() || "gh failed"); + } + return result.stdout; +} + +function dedupe(values: string[]): string[] { + return [...new Set(values)]; +} + +function parseGhAvailableFields(stderr: string): string[] { + const fields: string[] = []; + let capture = false; + for (const line of stderr.split(/\r?\n/)) { + if (line.toLowerCase().includes("available fields")) { + capture = true; + continue; + } + if (!capture) { + continue; + } + const cleaned = line.trim().replace(/^-/, "").trim(); + fields.push(...cleaned.split(/[\s,]+/).filter(Boolean)); + } + return dedupe(fields); +} + +function parseJsonRecord(stdout: string, action: string): Record { + const parsed = parseJson(stdout, action); + if (!parsed || typeof parsed !== "object" || Array.isArray(parsed)) { + throw new Error(`Invalid ${action} output: expected a JSON object`); + } + return parsed as Record; +} + +function parseJson(stdout: string, action: string): unknown { + try { + return JSON.parse(stdout) as unknown; + } catch (error) { + throw new Error( + `Invalid ${action} output: ${error instanceof Error ? error.message : "unknown parse error"}` + ); + } +} + +export function ghPrView( + prUrl: string, + fields: readonly string[], + options: GitHubCliOptions = {} +): Record { + requirePullRequestRef(prUrl); + const requestedFields = dedupe([...fields]); + const result = runGh(["pr", "view", prUrl, "--json", requestedFields.join(",")], options); + if (result.code === 0) { + const parsed = parseJsonRecord(result.stdout, "gh pr view"); + parsed.url ??= prUrl; + return parsed; + } + + const availableFields = parseGhAvailableFields(result.stderr); + const fallbackFields = requestedFields.filter((field) => availableFields.includes(field)); + if (fallbackFields.length === 0 || fallbackFields.length === requestedFields.length) { + throw new Error(result.stderr.trim() || result.stdout.trim() || "gh pr view failed"); + } + + const fallback = runGh(["pr", "view", prUrl, "--json", fallbackFields.join(",")], options); + if (fallback.code !== 0) { + throw new Error(fallback.stderr.trim() || fallback.stdout.trim() || "gh pr view failed"); + } + const parsed = parseJsonRecord(fallback.stdout, "gh pr view"); + parsed.url ??= prUrl; + return parsed; +} + +export function ghPrDiff(prUrl: string, options: GitHubCliOptions = {}): string { + requirePullRequestRef(prUrl); + return runGhOrThrow(["pr", "diff", prUrl], options); +} + +export function ghApiJson( + prUrl: string, + args: string[], + payload?: unknown, + options: GitHubCliOptions = {} +): Record { + const ref = requirePullRequestRef(prUrl); + const commandArgs = ["api"]; + if (ref.host !== "github.com") { + commandArgs.push("--hostname", ref.host); + } + commandArgs.push(...args); + if (payload !== undefined) { + commandArgs.push("--input", "-"); + } + + const stdout = runGhOrThrow(commandArgs, { + ...options, + input: payload === undefined ? undefined : JSON.stringify(payload) + }); + if (!stdout.trim()) { + return {}; + } + return parseJsonRecord(stdout, "gh api"); +} + +export function ghApiPaginatedJsonArray( + prUrl: string, + endpoint: string, + options: GitHubCliOptions = {} +): Record[] { + const ref = requirePullRequestRef(prUrl); + const values: Record[] = []; + let nextEndpoint: string | null = endpoint; + while (nextEndpoint) { + const args = ["api"]; + if (ref.host !== "github.com") { + args.push("--hostname", ref.host); + } + args.push("--include", nextEndpoint); + const result = runGh(args, options); + let response: GitHubApiResponse; + try { + response = parseGitHubApiResponse(result.stdout); + } catch (error) { + if (result.code === 0) { + throw error; + } + throw new Error(result.stderr.trim() || result.stdout.trim() || "gh api failed"); + } + const resumeEndpoint = nextEndpoint; + const rateLimit = rateLimitStatus(response, { + repo: `${ref.owner}/${ref.repo}`, + resumeEndpoint, + partialResults: values.length + }); + if (result.code !== 0 || response.statusCode >= 400) { + if (rateLimit) { + throw new GitHubRateLimitError(rateLimit); + } + throw new Error(result.stderr.trim() || result.stdout.trim() || "gh api failed"); + } + const items = parseJsonArrayRecords(response.body); + values.push(...items); + nextEndpoint = nextGitHubEndpoint(response); + if (nextEndpoint && rateLimit) { + throw new GitHubRateLimitError({ + ...rateLimit, + resumeEndpoint: nextEndpoint, + partialResults: values.length + }); + } + } + return values; +} + +function parseJsonArrayRecords(body: unknown): Record[] { + if (!Array.isArray(body)) { + throw new Error("Invalid gh api output: expected a JSON array"); + } + return body.map((value) => { + if (!value || typeof value !== "object" || Array.isArray(value)) { + throw new Error("Invalid gh api output: expected JSON object entries"); + } + return value as Record; + }); +} diff --git a/packages/github-review/src/github-api.ts b/packages/github-review/src/github-api.ts new file mode 100644 index 000000000..00c3a2116 --- /dev/null +++ b/packages/github-review/src/github-api.ts @@ -0,0 +1,221 @@ +export const LOW_RATE_LIMIT_REMAINING = 10; + +export type GitHubRateLimitReason = "low_remaining" | "primary" | "secondary"; + +export interface GitHubRateLimitStatus { + repo: string; + limit: number | null; + remaining: number | null; + resetAt: Date | null; + retryAfter: string | null; + resumeEndpoint: string; + partialResults: number; + reason: GitHubRateLimitReason; +} + +export class GitHubRateLimitError extends Error { + readonly status: GitHubRateLimitStatus; + + constructor(status: GitHubRateLimitStatus) { + const remaining = + status.remaining === null + ? "unknown remaining requests" + : `${status.remaining} requests remaining`; + const resetMessage = status.resetAt + ? ` after ${status.resetAt.toISOString()}` + : " once GitHub allows requests again"; + super( + `GitHub API ${status.reason.replace("_", " ")} limit (${remaining}) while reading ${status.repo}. Resume ${status.resumeEndpoint}${resetMessage}; ${status.partialResults} results were already yielded.` + ); + this.name = "GitHubRateLimitError"; + this.status = status; + } +} + +export interface GitHubApiResponse { + body: unknown; + statusCode: number; + links: ReadonlyMap; + rateLimitLimit: number | null; + rateLimitRemaining: number | null; + rateLimitReset: number | null; + retryAfter: string | null; +} + +function parseNonNegativeInteger(value: string | undefined): number | null { + if (!value || !/^\d+$/.test(value.trim())) { + return null; + } + const parsed = Number(value.trim()); + return Number.isSafeInteger(parsed) ? parsed : null; +} + +function parseLinks(value: string | undefined): ReadonlyMap { + const links = new Map(); + if (!value) { + return links; + } + for (const entry of value.split(/,\s*(?=<)/)) { + const endpoint = entry.match(/^<([^>]+)>/)?.[1]; + const relations = entry + .match(/(?:^|;)\s*rel=(?:"([^"]+)"|([^;\s]+))/i) + ?.slice(1) + .find(Boolean) + ?.split(/\s+/); + if (!endpoint || !relations) { + continue; + } + for (const relation of relations) { + links.set(relation, endpoint); + } + } + return links; +} + +export function parseGitHubApiResponse(stdout: string): GitHubApiResponse { + const headers = new Map(); + let statusCode: number | null = null; + let bodyStart = 0; + while (stdout.slice(bodyStart).startsWith("HTTP/")) { + const windowsBoundary = stdout.indexOf("\r\n\r\n", bodyStart); + const unixBoundary = stdout.indexOf("\n\n", bodyStart); + const isWindows = windowsBoundary >= 0 && (unixBoundary < 0 || windowsBoundary <= unixBoundary); + const boundary = isWindows ? windowsBoundary : unixBoundary; + if (boundary < 0) { + throw new Error("Invalid gh api output: response headers have no body"); + } + const block = stdout.slice(bodyStart, boundary); + const lines = block.split(/\r?\n/); + const parsedStatus = lines[0]?.match(/^HTTP\/\S+\s+(\d{3})(?:\s|$)/)?.[1]; + if (!parsedStatus) { + throw new Error("Invalid gh api output: malformed HTTP status"); + } + statusCode = Number.parseInt(parsedStatus, 10); + headers.clear(); + for (const line of lines.slice(1)) { + const separator = line.indexOf(":"); + if (separator > 0) { + headers.set( + line.slice(0, separator).trim().toLowerCase(), + line.slice(separator + 1).trim() + ); + } + } + bodyStart = boundary + (isWindows ? 4 : 2); + } + if (statusCode === null) { + throw new Error("Invalid gh api output: response headers are missing"); + } + let body: unknown; + try { + body = JSON.parse(stdout.slice(bodyStart)) as unknown; + } catch (error) { + throw new Error( + `Invalid gh api output: ${error instanceof Error ? error.message : "unknown parse error"}` + ); + } + return { + body, + statusCode, + links: parseLinks(headers.get("link")), + rateLimitLimit: parseNonNegativeInteger(headers.get("x-ratelimit-limit")), + rateLimitRemaining: parseNonNegativeInteger(headers.get("x-ratelimit-remaining")), + rateLimitReset: parseNonNegativeInteger(headers.get("x-ratelimit-reset")), + retryAfter: headers.get("retry-after") ?? null + }; +} + +export function nextGitHubEndpoint(response: GitHubApiResponse): string | null { + const endpoint = response.links.get("next"); + if (!endpoint) { + return null; + } + try { + const url = new URL(endpoint); + return `${url.pathname.replace(/^\//, "")}${url.search}`; + } catch { + return endpoint; + } +} + +function retryAfterResetAt(retryAfter: string | null, now: () => Date): Date | null { + if (!retryAfter) { + return null; + } + const seconds = parseNonNegativeInteger(retryAfter); + if (seconds !== null) { + return validDate(now().getTime() + seconds * 1_000); + } + const date = new Date(retryAfter); + return Number.isNaN(date.getTime()) ? null : date; +} + +function validDate(milliseconds: number): Date | null { + const date = new Date(milliseconds); + return Number.isNaN(date.getTime()) ? null : date; +} + +export function githubResetAt( + response: GitHubApiResponse, + now: () => Date = () => new Date() +): Date | null { + const retryAfter = retryAfterResetAt(response.retryAfter, now); + return ( + retryAfter ?? + (response.rateLimitReset === null ? null : validDate(response.rateLimitReset * 1_000)) + ); +} + +function responseMessage(response: GitHubApiResponse): string { + if (!response.body || typeof response.body !== "object" || Array.isArray(response.body)) { + return ""; + } + const message = (response.body as Record).message; + return typeof message === "string" ? message.toLowerCase() : ""; +} + +export function rateLimitReason(response: GitHubApiResponse): GitHubRateLimitReason | null { + const message = responseMessage(response); + if ( + response.retryAfter !== null || + message.includes("secondary rate limit") || + message.includes("abuse detection") || + message.includes("abuse-rate") + ) { + return "secondary"; + } + if (response.rateLimitRemaining === 0 || message.includes("api rate limit exceeded")) { + return "primary"; + } + if ( + response.rateLimitRemaining !== null && + response.rateLimitRemaining <= LOW_RATE_LIMIT_REMAINING + ) { + return "low_remaining"; + } + return null; +} + +export function rateLimitStatus( + response: GitHubApiResponse, + input: { + repo: string; + resumeEndpoint: string; + partialResults: number; + now?: () => Date; + } +): GitHubRateLimitStatus | null { + const reason = rateLimitReason(response); + return reason === null + ? null + : { + repo: input.repo, + limit: response.rateLimitLimit, + remaining: response.rateLimitRemaining, + resetAt: githubResetAt(response, input.now), + retryAfter: response.retryAfter, + resumeEndpoint: input.resumeEndpoint, + partialResults: input.partialResults, + reason + }; +} diff --git a/packages/github-review/src/index.ts b/packages/github-review/src/index.ts new file mode 100644 index 000000000..f1f8d7385 --- /dev/null +++ b/packages/github-review/src/index.ts @@ -0,0 +1,63 @@ +export { + type CommandResult, + type CommandRunner, + type CommandRunnerOptions, + defaultCommandRunner +} from "./command.js"; +export { + parseReviewDiff, + validateInlineComments, + type ReviewDiffContext, + type ReviewDiffFile, + type ReviewDiffHunk, + type ReviewDiffLine, + type ReviewInlineComment +} from "./diff.js"; +export { + type GitHubCliOptions, + ghApiJson, + ghApiPaginatedJsonArray, + ghPrDiff, + ghPrView +} from "./gh.js"; +export { + canonicalPullRequestUrl, + parseGitHubPullRequestRef, + type GitHubPullRequestRef +} from "./pr-url.js"; +export { filesystemSafeNamePart } from "./filesystem-name.js"; +export { + DEFAULT_PULL_REQUEST_COMMENT_FIELDS, + DEFAULT_PR_FIELDS, + DEFAULT_PULL_REQUEST_METADATA_FIELDS, + DEFAULT_PULL_REQUEST_REVIEW_ACTIVITY_FIELDS, + DEFAULT_PULL_REQUEST_REVIEW_FIELDS, + fetchPullRequestComments, + fetchPullRequestDetails, + fetchPullRequestDiff, + fetchPullRequestMetadata, + fetchPullRequestReviewActivity, + fetchPullRequestReviewComments, + fetchPullRequestReviews +} from "./pull-request.js"; +export { + deletePullRequestReviewComment, + editPullRequestReviewComment, + submitPullRequestReview, + type PullRequestReviewCommentInput, + type PullRequestReviewDecision, + type PullRequestReviewSubmission +} from "./review.js"; +export { + fetchReviewHistory, + type FetchReviewHistoryOptions, + type ReviewHistoryComment, + type ReviewHistoryKind +} from "./review-history.js"; +export { + GitHubRateLimitError, + type GitHubApiResponse, + type GitHubRateLimitReason, + type GitHubRateLimitStatus, + parseGitHubApiResponse +} from "./github-api.js"; diff --git a/packages/github-review/src/pr-url.ts b/packages/github-review/src/pr-url.ts new file mode 100644 index 000000000..14ff07ddb --- /dev/null +++ b/packages/github-review/src/pr-url.ts @@ -0,0 +1,71 @@ +function parseUrl(value: string): URL | null { + try { + return new URL(value); + } catch { + return null; + } +} + +function parsePathPart(value: string): string | null { + try { + const decoded = decodeURIComponent(value); + const hasDisallowedCharacter = [...decoded].some((character) => { + const codePoint = character.codePointAt(0) ?? 0; + return character === "/" || character === "\\" || codePoint < 32 || codePoint === 127; + }); + if (!decoded || hasDisallowedCharacter) { + return null; + } + return decoded; + } catch { + return null; + } +} + +export interface GitHubPullRequestRef { + host: string; + owner: string; + repo: string; + number: number; + url: string; +} + +export function parseGitHubPullRequestRef(prUrl: string): GitHubPullRequestRef | null { + const parsed = parseUrl(prUrl); + if ( + !parsed || + parsed.protocol !== "https:" || + !parsed.hostname || + parsed.username || + parsed.password + ) { + return null; + } + + const match = /^\/([^/]+)\/([^/]+)\/pull\/(\d+)(?:\/|$)/.exec(parsed.pathname); + if (!match) { + return null; + } + const owner = parsePathPart(match[1] ?? ""); + const repo = parsePathPart(match[2] ?? ""); + const number = Number.parseInt(match[3] ?? "0", 10); + if (!owner || !repo || !Number.isSafeInteger(number) || number <= 0) { + return null; + } + + return { + host: parsed.host.toLowerCase(), + owner, + repo, + number, + url: prUrl + }; +} + +export function canonicalPullRequestUrl(prUrl: string): string { + const ref = parseGitHubPullRequestRef(prUrl); + if (!ref) { + return prUrl; + } + return `https://${ref.host}/${encodeURIComponent(ref.owner)}/${encodeURIComponent(ref.repo)}/pull/${ref.number}`; +} diff --git a/packages/github-review/src/pull-request.ts b/packages/github-review/src/pull-request.ts new file mode 100644 index 000000000..28ebccb50 --- /dev/null +++ b/packages/github-review/src/pull-request.ts @@ -0,0 +1,97 @@ +import { type GitHubCliOptions, ghApiPaginatedJsonArray, ghPrDiff, ghPrView } from "./gh.js"; +import { parseGitHubPullRequestRef } from "./pr-url.js"; + +export const DEFAULT_PULL_REQUEST_METADATA_FIELDS = [ + "number", + "url", + "title", + "body", + "headRefName", + "baseRefName", + "state", + "author", + "labels" +] as const; + +export const DEFAULT_PULL_REQUEST_COMMENT_FIELDS = ["url", "comments"] as const; + +export const DEFAULT_PULL_REQUEST_REVIEW_FIELDS = [ + "url", + "reviews", + "latestReviews", + "reviewThreads" +] as const; + +export const DEFAULT_PULL_REQUEST_REVIEW_ACTIVITY_FIELDS = [ + "url", + "comments", + "reviews", + "latestReviews", + "reviewThreads" +] as const; + +export const DEFAULT_PR_FIELDS = [ + ...DEFAULT_PULL_REQUEST_METADATA_FIELDS, + "comments", + "reviews", + "latestReviews", + "reviewThreads" +] as const; + +function reviewCommentsEndpoint(prUrl: string): string { + const ref = parseGitHubPullRequestRef(prUrl); + if (!ref) { + throw new Error(`GitHub PR URL is required: ${prUrl}`); + } + return `repos/${ref.owner}/${ref.repo}/pulls/${ref.number}/comments?per_page=100`; +} + +export function fetchPullRequestMetadata( + prUrl: string, + options: GitHubCliOptions & { fields?: readonly string[] } = {} +): Record { + return ghPrView(prUrl, options.fields ?? DEFAULT_PULL_REQUEST_METADATA_FIELDS, options); +} + +export function fetchPullRequestDetails( + prUrl: string, + fields: readonly string[] = DEFAULT_PR_FIELDS, + options: GitHubCliOptions = {} +): Record { + return ghPrView(prUrl, fields, options); +} + +export function fetchPullRequestDiff(prUrl: string, options: GitHubCliOptions = {}): string { + return ghPrDiff(prUrl, options); +} + +export function fetchPullRequestComments( + prUrl: string, + options: GitHubCliOptions = {} +): Record { + return ghPrView(prUrl, DEFAULT_PULL_REQUEST_COMMENT_FIELDS, options); +} + +export function fetchPullRequestReviews( + prUrl: string, + options: GitHubCliOptions = {} +): Record { + return ghPrView(prUrl, DEFAULT_PULL_REQUEST_REVIEW_FIELDS, options); +} + +export function fetchPullRequestReviewComments( + prUrl: string, + options: GitHubCliOptions = {} +): Record[] { + return ghApiPaginatedJsonArray(prUrl, reviewCommentsEndpoint(prUrl), options); +} + +export function fetchPullRequestReviewActivity( + prUrl: string, + options: GitHubCliOptions = {} +): Record { + return { + ...ghPrView(prUrl, DEFAULT_PULL_REQUEST_REVIEW_ACTIVITY_FIELDS, options), + reviewComments: fetchPullRequestReviewComments(prUrl, options) + }; +} diff --git a/packages/github-review/src/review-history.ts b/packages/github-review/src/review-history.ts new file mode 100644 index 000000000..d1089ebb7 --- /dev/null +++ b/packages/github-review/src/review-history.ts @@ -0,0 +1,424 @@ +import { type CommandRunner, defaultCommandRunner } from "./command.js"; +import type { GitHubCliOptions } from "./gh.js"; +import { + type GitHubApiResponse, + GitHubRateLimitError, + type GitHubRateLimitStatus, + githubResetAt, + nextGitHubEndpoint, + parseGitHubApiResponse, + rateLimitReason +} from "./github-api.js"; +export { GitHubRateLimitError, type GitHubRateLimitStatus } from "./github-api.js"; + +export type ReviewHistoryKind = "review_comment" | "review_body" | "pr_comment"; + +export interface ReviewHistoryComment { + repo: string; + pullRequestNumber: number; + pullRequestTitle: string; + pullRequestUrl: string; + authorLogin: string; + createdAt: string; + kind: ReviewHistoryKind; + body: string; + path?: string; + line?: number; + diffHunk?: string; +} + +export interface FetchReviewHistoryOptions extends GitHubCliOptions { + username: string; + repos: readonly string[]; + since?: string | Date; + maxComments?: number; + onRateLimit?: (status: GitHubRateLimitStatus) => void | Promise; +} + +interface ApiPage { + items: Record[]; + nextEndpoint: string | null; +} + +interface PullRequestMetadata { + number: number; + title: string; + url: string; +} + +function parseObject(value: unknown, action: string): Record { + if (!value || typeof value !== "object" || Array.isArray(value)) { + throw new Error(`Invalid ${action} output: expected a JSON object`); + } + return value as Record; +} + +function parseArray(value: unknown, action: string): Record[] { + if (!Array.isArray(value)) { + throw new Error(`Invalid ${action} output: expected a JSON array`); + } + return value.map((item) => parseObject(item, action)); +} + +function text(record: Record, key: string): string | null { + return typeof record[key] === "string" ? record[key] : null; +} + +function integer(record: Record, key: string): number | null { + return typeof record[key] === "number" && Number.isInteger(record[key]) ? record[key] : null; +} + +function authorLogin(record: Record): string | null { + const user = record.user; + return user && typeof user === "object" && !Array.isArray(user) + ? text(user as Record, "login") + : null; +} + +function pullRequestNumber(record: Record): number | null { + const direct = integer(record, "number"); + if (direct) { + return direct; + } + for (const key of ["pull_request_url", "issue_url", "html_url"]) { + const value = text(record, key); + const match = value?.match(/\/(?:pulls|pull|issues)\/(\d+)(?:\/|$)/); + if (match) { + return Number.parseInt(match[1] ?? "", 10); + } + } + return null; +} + +function pullRequestUrl(repo: string, record: Record, number: number): string { + const pullRequest = record.pull_request; + if (pullRequest && typeof pullRequest === "object" && !Array.isArray(pullRequest)) { + const htmlUrl = text(pullRequest as Record, "html_url"); + if (htmlUrl) return htmlUrl; + } + const htmlUrl = text(record, "html_url"); + return htmlUrl?.includes("/pull/") ? htmlUrl : `https://github.com/${repo}/pull/${number}`; +} + +function validRepo(repo: string): boolean { + return ( + repo.split("/").length === 2 && + repo + .split("/") + .every( + (part) => + /^[A-Za-z0-9_.-]+$/.test(part) && part.normalize("NFKC") === part && !part.startsWith(".") + ) + ); +} + +function appendQuery(endpoint: string, values: Record): string { + const query = new URLSearchParams(values); + return `${endpoint}?${query.toString()}`; +} + +function asIsoDate(value: string | Date | undefined): string | undefined { + if (value === undefined) { + return undefined; + } + const date = value instanceof Date ? value : new Date(value); + if (Number.isNaN(date.getTime())) { + throw new Error(`Invalid review-history since timestamp: ${String(value)}`); + } + return date.toISOString(); +} + +function isAtOrAfter(value: string | null, since: string | undefined): boolean { + if (!since) return true; + if (!value) return false; + return new Date(value).getTime() >= new Date(since).getTime(); +} + +function createdAt(record: Record, kind: ReviewHistoryKind): string | null { + return kind === "review_body" + ? (text(record, "submitted_at") ?? text(record, "created_at")) + : (text(record, "created_at") ?? text(record, "submitted_at")); +} + +class ReviewHistoryFetcher { + private readonly runner: CommandRunner; + private readonly username: string; + private readonly since?: string; + private readonly pullRequests = new Map(); + private readonly nonPullRequests = new Set(); + private rateLimit: Omit< + GitHubRateLimitStatus, + "repo" | "resumeEndpoint" | "partialResults" + > | null = null; + emitted = 0; + + constructor(private readonly options: FetchReviewHistoryOptions) { + this.runner = options.runner ?? defaultCommandRunner; + this.username = options.username.trim().toLowerCase(); + this.since = asIsoDate(options.since); + } + + private async ensureRateLimit(repo: string, resumeEndpoint: string) { + if (!this.rateLimit) { + return; + } + const status: GitHubRateLimitStatus = { + repo, + ...this.rateLimit, + resumeEndpoint, + partialResults: this.emitted + }; + await this.options.onRateLimit?.(status); + throw new GitHubRateLimitError(status); + } + + private observeRateLimit(response: GitHubApiResponse): void { + const reason = rateLimitReason(response); + this.rateLimit = reason + ? { + limit: response.rateLimitLimit, + remaining: response.rateLimitRemaining, + resetAt: githubResetAt(response), + retryAfter: response.retryAfter, + reason + } + : null; + } + + private async request(repo: string, endpoint: string): Promise { + await this.ensureRateLimit(repo, endpoint); + const result = this.runner("gh", ["api", "--include", endpoint], { + cwd: this.options.cwd + }); + let parsed: GitHubApiResponse | null = null; + try { + parsed = parseGitHubApiResponse(result.stdout); + this.observeRateLimit(parsed); + } catch (error) { + if (result.code === 0) { + throw error; + } + } + if (result.code !== 0 || (parsed?.statusCode ?? 500) >= 400) { + await this.ensureRateLimit(repo, endpoint); + throw new Error(result.stderr.trim() || result.stdout.trim() || "gh api failed"); + } + if (!parsed) { + throw new Error("Invalid gh api output: expected included response"); + } + return parsed; + } + + private async page(repo: string, endpoint: string): Promise { + const response = await this.request(repo, endpoint); + return { + items: parseArray(response.body, "gh api"), + nextEndpoint: nextGitHubEndpoint(response) + }; + } + + private async object(repo: string, endpoint: string): Promise> { + return parseObject((await this.request(repo, endpoint)).body, "gh api"); + } + + private matchesAuthor(record: Record): boolean { + return authorLogin(record)?.toLowerCase() === this.username; + } + + private matchesSince(record: Record, kind: ReviewHistoryKind): boolean { + return isAtOrAfter(createdAt(record, kind), this.since); + } + + private rememberPullRequest( + repo: string, + record: Record + ): PullRequestMetadata | null { + const number = pullRequestNumber(record); + if (!number) { + return null; + } + const metadata = { + number, + title: text(record, "title") ?? "", + url: pullRequestUrl(repo, record, number) + }; + this.pullRequests.set(`${repo}#${number}`, metadata); + return metadata; + } + + private async getPullRequest(repo: string, number: number): Promise { + const key = `${repo}#${number}`; + const cached = this.pullRequests.get(key); + if (cached) { + return cached; + } + const metadata = this.rememberPullRequest( + repo, + await this.object(repo, `repos/${repo}/pulls/${number}`) + ); + if (!metadata) { + throw new Error(`Invalid pull request metadata for ${repo}#${number}`); + } + return metadata; + } + + private normalize( + repo: string, + pullRequest: PullRequestMetadata, + record: Record, + kind: ReviewHistoryKind + ): ReviewHistoryComment | null { + const login = authorLogin(record); + const timestamp = createdAt(record, kind); + const body = text(record, "body"); + if (!login || !timestamp || body === null || (kind === "review_body" && !body.trim())) { + return null; + } + const normalized: ReviewHistoryComment = { + repo, + pullRequestNumber: pullRequest.number, + pullRequestTitle: pullRequest.title, + pullRequestUrl: pullRequest.url, + authorLogin: login, + createdAt: timestamp, + kind, + body + }; + const path = text(record, "path"); + const line = integer(record, "line") ?? integer(record, "original_line"); + const diffHunk = text(record, "diff_hunk"); + if (path) normalized.path = path; + if (line !== null) normalized.line = line; + if (diffHunk) normalized.diffHunk = diffHunk; + return normalized; + } + + async *reviewComments(repo: string): AsyncGenerator { + let endpoint: string | null = appendQuery(`repos/${repo}/pulls/comments`, { + per_page: "100", + ...(this.since ? { since: this.since } : {}) + }); + while (endpoint) { + const page = await this.page(repo, endpoint); + for (const record of page.items) { + if (!this.matchesAuthor(record) || !this.matchesSince(record, "review_comment")) { + continue; + } + const number = pullRequestNumber(record); + if (!number) continue; + const normalized = this.normalize( + repo, + await this.getPullRequest(repo, number), + record, + "review_comment" + ); + if (normalized) yield normalized; + } + endpoint = page.nextEndpoint; + } + } + + async *reviewBodies(repo: string): AsyncGenerator { + let endpoint: string | null = appendQuery(`repos/${repo}/pulls`, { + state: "all", + sort: "updated", + direction: "desc", + per_page: "100" + }); + let pastSince = false; + while (endpoint && !pastSince) { + const page = await this.page(repo, endpoint); + for (const pullRecord of page.items) { + const updatedAt = text(pullRecord, "updated_at"); + if (updatedAt && !isAtOrAfter(updatedAt, this.since)) { + pastSince = true; + break; + } + const pullRequest = this.rememberPullRequest(repo, pullRecord); + if (!pullRequest) continue; + let reviewsEndpoint: string | null = appendQuery( + `repos/${repo}/pulls/${pullRequest.number}/reviews`, + { per_page: "100" } + ); + while (reviewsEndpoint) { + const reviews = await this.page(repo, reviewsEndpoint); + for (const record of reviews.items) { + if (!this.matchesAuthor(record) || !this.matchesSince(record, "review_body")) continue; + const normalized = this.normalize(repo, pullRequest, record, "review_body"); + if (normalized) yield normalized; + } + reviewsEndpoint = reviews.nextEndpoint; + } + } + endpoint = page.nextEndpoint; + } + } + + async *pullRequestComments(repo: string): AsyncGenerator { + let endpoint: string | null = appendQuery(`repos/${repo}/issues/comments`, { + per_page: "100", + ...(this.since ? { since: this.since } : {}) + }); + while (endpoint) { + const page = await this.page(repo, endpoint); + for (const record of page.items) { + if (!this.matchesAuthor(record) || !this.matchesSince(record, "pr_comment")) { + continue; + } + const number = pullRequestNumber(record); + if (!number) continue; + const key = `${repo}#${number}`; + if (this.nonPullRequests.has(key)) continue; + let pullRequest = this.pullRequests.get(key) ?? null; + if (!pullRequest) { + const issue = await this.object(repo, `repos/${repo}/issues/${number}`); + if (!issue.pull_request) { + this.nonPullRequests.add(key); + continue; + } + pullRequest = this.rememberPullRequest(repo, issue); + } + if (!pullRequest) continue; + const normalized = this.normalize(repo, pullRequest, record, "pr_comment"); + if (normalized) yield normalized; + } + endpoint = page.nextEndpoint; + } + } +} + +export async function* fetchReviewHistory( + options: FetchReviewHistoryOptions +): AsyncGenerator { + if (!options.username.trim()) { + throw new Error("GitHub review-history username is required."); + } + if ( + options.maxComments !== undefined && + (!Number.isInteger(options.maxComments) || options.maxComments < 1) + ) { + throw new Error("Review-history maxComments must be a positive integer."); + } + const repos = [...new Set(options.repos)]; + for (const repo of repos) { + if (!validRepo(repo)) { + throw new Error(`Invalid GitHub repository name: ${repo}`); + } + } + const fetcher = new ReviewHistoryFetcher(options); + for (const repo of repos) { + for (const source of [ + fetcher.reviewComments(repo), + fetcher.reviewBodies(repo), + fetcher.pullRequestComments(repo) + ]) { + for await (const comment of source) { + fetcher.emitted += 1; + yield comment; + if (options.maxComments && fetcher.emitted >= options.maxComments) { + return; + } + } + } + } +} diff --git a/packages/github-review/src/review.ts b/packages/github-review/src/review.ts new file mode 100644 index 000000000..d0519c5ed --- /dev/null +++ b/packages/github-review/src/review.ts @@ -0,0 +1,145 @@ +import { type GitHubCliOptions, ghApiJson } from "./gh.js"; +import { parseGitHubPullRequestRef } from "./pr-url.js"; + +export type PullRequestReviewDecision = "APPROVE" | "REQUEST_CHANGES" | "COMMENT"; + +export interface PullRequestReviewCommentInput { + path: string; + line: number; + body: string; +} + +export interface PullRequestReviewSubmission { + id: number | null; + url: string | null; +} + +type PullRequestTarget = { pr?: string; prUrl?: string }; + +const REVIEW_DECISIONS: ReadonlySet = new Set(["APPROVE", "REQUEST_CHANGES", "COMMENT"]); + +function inputPullRequestUrl(input: PullRequestTarget): string { + const prUrl = input.prUrl ?? input.pr; + if (!prUrl) { + throw new Error("GitHub PR URL is required."); + } + return prUrl; +} + +function reviewEndpoint(prUrl: string): string { + const ref = parseGitHubPullRequestRef(prUrl); + if (!ref) { + throw new Error(`GitHub PR URL is required: ${prUrl}`); + } + return `repos/${ref.owner}/${ref.repo}/pulls/${ref.number}/reviews`; +} + +function commentEndpoint(prUrl: string, commentId: string | number): string { + const ref = parseGitHubPullRequestRef(prUrl); + if (!ref) { + throw new Error(`GitHub PR URL is required: ${prUrl}`); + } + return `repos/${ref.owner}/${ref.repo}/pulls/comments/${requirePositiveId(commentId)}`; +} + +function requirePositiveId(commentId: string | number): string { + if (typeof commentId === "number") { + if (!Number.isSafeInteger(commentId) || commentId <= 0) { + throw new Error("Review comment ID must be a positive integer."); + } + return String(commentId); + } + const normalized = commentId.trim(); + if (!/^[1-9]\d*$/.test(normalized)) { + throw new Error("Review comment ID must be a positive integer."); + } + return normalized; +} + +function validateReviewComments(comments: readonly PullRequestReviewCommentInput[]): void { + for (const comment of comments) { + if (!comment.path.trim()) { + throw new Error("Review comment path must be non-empty."); + } + if (!Number.isSafeInteger(comment.line) || comment.line <= 0) { + throw new Error("Review comment line must be a positive integer."); + } + if (!comment.body.trim()) { + throw new Error("Review comment body must be non-empty."); + } + } +} + +function submissionFromResponse(response: Record): PullRequestReviewSubmission { + return { + id: typeof response.id === "number" ? response.id : null, + url: typeof response.html_url === "string" ? response.html_url : null + }; +} + +export function submitPullRequestReview( + input: { + decision: PullRequestReviewDecision; + summary: string; + comments?: readonly PullRequestReviewCommentInput[]; + } & PullRequestTarget & + GitHubCliOptions +): PullRequestReviewSubmission { + const prUrl = inputPullRequestUrl(input); + if (!REVIEW_DECISIONS.has(input.decision)) { + throw new Error(`Invalid pull request review decision: ${input.decision}`); + } + const comments = input.comments ?? []; + validateReviewComments(comments); + const response = ghApiJson( + prUrl, + ["--method", "POST", reviewEndpoint(prUrl)], + { + body: input.summary, + event: input.decision, + comments: comments.map((comment) => ({ + path: comment.path, + line: comment.line, + side: "RIGHT", + body: comment.body + })) + }, + input + ); + return submissionFromResponse(response); +} + +export function editPullRequestReviewComment( + input: { + commentId: string | number; + body: string; + } & PullRequestTarget & + GitHubCliOptions +): PullRequestReviewSubmission { + const prUrl = inputPullRequestUrl(input); + if (!input.body.trim()) { + throw new Error("Review comment body must be non-empty."); + } + const response = ghApiJson( + prUrl, + ["--method", "PATCH", commentEndpoint(prUrl, input.commentId)], + { body: input.body }, + input + ); + return submissionFromResponse(response); +} + +export function deletePullRequestReviewComment( + input: { + commentId: string | number; + } & PullRequestTarget & + GitHubCliOptions +): void { + const prUrl = inputPullRequestUrl(input); + ghApiJson( + prUrl, + ["--method", "DELETE", commentEndpoint(prUrl, input.commentId)], + undefined, + input + ); +} diff --git a/packages/github-review/tsconfig.json b/packages/github-review/tsconfig.json new file mode 100644 index 000000000..44b01ee75 --- /dev/null +++ b/packages/github-review/tsconfig.json @@ -0,0 +1,11 @@ +{ + "extends": "../../tsconfig.json", + "compilerOptions": { + "outDir": "dist", + "rootDir": "src", + "noEmit": false, + "declaration": true + }, + "include": ["src"], + "exclude": ["**/*.test.ts"] +} diff --git a/src/cli/program.test.ts b/src/cli/program.test.ts index 4c7e06dea..dbc11aaba 100644 --- a/src/cli/program.test.ts +++ b/src/cli/program.test.ts @@ -98,4 +98,70 @@ describe("createProgram", () => { process.exitCode = originalExitCode; stdoutSpy.mockRestore(); }); + + it("registers the code-review command group in root help", () => { + const fs = createMemFs(homeDir); + const program = createProgram({ + fs, + prompts: async () => ({}), + env: { cwd: "/repo", homeDir }, + logger: () => {}, + exitOverride: true, + suppressCommanderOutput: true + }); + + expect(program.commands.find((command) => command.name() === "code-review")).toBeDefined(); + expect(program.helpInformation()).toContain("code-review"); + }); + + it("forwards code-review help through the root command", async () => { + const fs = createMemFs(homeDir); + const writes: string[] = []; + vi.spyOn(process.stdout, "write").mockImplementation(((chunk: unknown) => { + writes.push(String(chunk)); + return true; + }) as typeof process.stdout.write); + process.argv = ["node", "poe-code", "code-review", "--help"]; + const program = createProgram({ + fs, + prompts: async () => ({}), + env: { cwd: "/repo", homeDir }, + logger: () => {}, + exitOverride: true, + suppressCommanderOutput: true + }); + + await program.parseAsync(process.argv); + + const output = writes.join(""); + expect(output).toContain("code-review"); + for (const command of ["install", "profiles", "ingest", "run", "drafts", "commit", "agent-mcp"]) { + expect(output).toContain(command); + } + }); + + it.each(["install", "profiles", "ingest", "run", "drafts", "commit", "agent-mcp"])( + "forwards code-review %s help through the root command", + async (command) => { + const fs = createMemFs(homeDir); + const writes: string[] = []; + vi.spyOn(process.stdout, "write").mockImplementation(((chunk: unknown) => { + writes.push(String(chunk)); + return true; + }) as typeof process.stdout.write); + process.argv = ["node", "poe-code", "code-review", command, "--help"]; + const program = createProgram({ + fs, + prompts: async () => ({}), + env: { cwd: "/repo", homeDir }, + logger: () => {}, + exitOverride: true, + suppressCommanderOutput: true + }); + + await program.parseAsync(process.argv); + + expect(writes.join("")).toContain(command); + } + ); }); diff --git a/src/cli/program.ts b/src/cli/program.ts index 00e17599f..b32e7d43f 100644 --- a/src/cli/program.ts +++ b/src/cli/program.ts @@ -5,6 +5,7 @@ import { S, type Static } from "toolcraft-schema"; import { runCLI } from "toolcraft/cli"; import { evalGroup } from "@poe-code/agent-eval"; import { ghGroup } from "@poe-code/github-workflows"; +import { codeReviewGroup } from "agent-code-review"; import { superintendentGroup } from "@poe-code/superintendent"; import { runMaestro, @@ -133,6 +134,7 @@ const ROOT_HELP_COMMAND_SPECS: readonly RootHelpCommandSpec[] = [ { path: ["launch"] }, { path: ["approvals"], args: "[command]" }, { path: ["github-workflows"], args: "[automation]" }, + { path: ["code-review"], args: "[command]" }, { path: ["usage"] }, { path: ["usage", "list"] }, { path: ["utils", "config"] } @@ -724,7 +726,8 @@ function bootstrapProgram(container: CliContainer): Command { const toolcraftRoots = [ evalGroup as Group, ghGroup as Group, - superintendentGroup as Group + superintendentGroup as Group, + codeReviewGroup as Group ]; program @@ -799,6 +802,18 @@ function bootstrapProgram(container: CliContainer): Command { heading, usageCommand ); + registerForwardedToolcraftCommand( + program, + container, + { + name: codeReviewGroup.name, + description: codeReviewGroup.description ?? "", + aliases: codeReviewGroup.aliases + }, + toolcraftRoots, + heading, + usageCommand + ); registerForwardedToolcraftCommand( program, container, diff --git a/src/index.test.ts b/src/index.test.ts index 1eb73cb15..5c21ed33c 100644 --- a/src/index.test.ts +++ b/src/index.test.ts @@ -4,13 +4,25 @@ import { createLogWriter, createStateStore, createSupervisor, + codeReviewGroup, + createCodeReviewAgentMcpConfig, + createCodeReviewSession, + createCodeReviewState, + commitCodeReviewDrafts, + discoverCodeReviewProfiles, ghGroup, getPoeApiKey, + ingestCodeReviewProfile, + installCodeReviewAssets, isCliInvocation, + loadCodeReviewProfile, planDocumentSchema, planDocumentSchemaId, openaiResponsesPlugin, + readCodeReviewDraft, runExperiment, + runCodeReview, + runCodeReviewAgentMcp, runRalph, systemPromptPlugin, waitForReady, @@ -63,6 +75,21 @@ describe("entrypoint module", () => { expect(typeof runRalph).toBe("function"); }); + it("re-exports code-review SDK and CLI wiring", () => { + expect(codeReviewGroup.name).toBe("code-review"); + expect(typeof createCodeReviewAgentMcpConfig).toBe("function"); + expect(typeof createCodeReviewSession).toBe("function"); + expect(typeof createCodeReviewState).toBe("function"); + expect(typeof discoverCodeReviewProfiles).toBe("function"); + expect(typeof installCodeReviewAssets).toBe("function"); + expect(typeof loadCodeReviewProfile).toBe("function"); + expect(typeof ingestCodeReviewProfile).toBe("function"); + expect(typeof runCodeReview).toBe("function"); + expect(typeof readCodeReviewDraft).toBe("function"); + expect(typeof commitCodeReviewDrafts).toBe("function"); + expect(typeof runCodeReviewAgentMcp).toBe("function"); + }); + it("re-exports runExperiment", () => { expect(typeof runExperiment).toBe("function"); }); diff --git a/src/index.ts b/src/index.ts index 31c69e334..6e447f8fa 100644 --- a/src/index.ts +++ b/src/index.ts @@ -62,6 +62,21 @@ export type { LintIssue, LintResult } from "@poe-code/agent-eval"; +export { + codeReviewGroup, + createCodeReviewAgentMcpConfig, + createCodeReviewGroup, + createCodeReviewSession, + createCodeReviewState, + discoverCodeReviewProfiles, + ingestCodeReviewProfile, + installCodeReviewAssets, + loadCodeReviewProfile, + readCodeReviewDraft, + runCodeReview, + runCodeReviewAgentMcp, + commitCodeReviewDrafts +} from "agent-code-review"; export { followLaunchLogs, listLaunches, @@ -133,6 +148,23 @@ export type { } from "@poe-code/agent-maestro"; export type { RalphRunOptions, RalphRunResult } from "./sdk/ralph.js"; export type { AutomationDefinition } from "@poe-code/github-workflows"; +export type { + CodeReviewAgentMcpConfig, + CodeReviewAgentMcpContext, + CodeReviewCliDependencies, + CodeReviewCommitResult, + CodeReviewIngestInput, + CodeReviewIngestResult, + CodeReviewInstallResult, + CodeReviewOrchestrationInput, + CodeReviewProfile, + CodeReviewResult, + CodeReviewRunInput, + CodeReviewRunOptions, + CodeReviewState, + CommitCodeReviewDraftsInput, + ReadCodeReviewDraftInput +} from "agent-code-review"; export type { ExperimentRunOptions, ExperimentRunResult, diff --git a/src/services/config.ts b/src/services/config.ts index a1bce82f2..fb9da7922 100644 --- a/src/services/config.ts +++ b/src/services/config.ts @@ -17,6 +17,7 @@ import type { } from "@poe-code/poe-code-config"; import { parseNullablePluginConfigEntries, type PluginConfigEntry } from "@poe-code/poe-agent"; import { superintendentConfigScope } from "@poe-code/superintendent"; +import { codeReviewConfigScope } from "agent-code-review"; import type { FileSystem } from "../utils/file-system.js"; export interface ConfigStoreOptions { @@ -114,7 +115,8 @@ export const knownConfigScopes = [ experimentConfigScope, planConfigScope, agentConfigScope, - superintendentConfigScope + superintendentConfigScope, + codeReviewConfigScope ] as const; const CORE_SCOPE = coreConfigScope.scope; diff --git a/src/services/services.test.ts b/src/services/services.test.ts index 6e6443cc1..4200891d1 100644 --- a/src/services/services.test.ts +++ b/src/services/services.test.ts @@ -5,6 +5,7 @@ import { resolveConfigPath } from "@poe-code/poe-code-config"; import type { FileSystem } from "../utils/file-system.js"; import { coreConfigScope, + knownConfigScopes, loadConfig, saveConfig, loadConfiguredServices, @@ -63,6 +64,10 @@ describe("config store", () => { }); }); + it("registers the codeReview config scope", () => { + expect(knownConfigScopes.map((scope) => scope.scope)).toContain("codeReview"); + }); + it("returns stored api key when file is valid json", async () => { await saveConfig({ fs,