diff --git a/.agents/agents/code-review.md b/.agents/agents/code-review.md index b18d31668f..1bd6d1311b 100644 --- a/.agents/agents/code-review.md +++ b/.agents/agents/code-review.md @@ -8,7 +8,7 @@ permissionMode: acceptEdits # Code Review -Review the current changes in the codebase (Go + Next.js monorepo). Every finding needs a `file_path:line_number` reference, an explanation of *why* it matters, and a concrete fix. +Review the current changes in the codebase (Go backend + Vite/React SPA monorepo). Every finding needs a `file_path:line_number` reference, an explanation of *why* it matters, and a concrete fix. Start from intent and evidence: read the spec/task first when available, then read tests before production code. Tests reveal the expected behavior and often expose whether the implementation is actually verified. diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 026d21ce17..d24c5bf560 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -5,7 +5,7 @@ description: Review changed code for quality, security, and architecture complia # Code Review -Review the current changes in the codebase (Go + Next.js monorepo). Every finding needs a `file_path:line_number` reference, an explanation of *why* it matters, and a concrete fix. +Review the current changes in the codebase (Go backend + Vite/React SPA monorepo). Every finding needs a `file_path:line_number` reference, an explanation of *why* it matters, and a concrete fix. Start from intent and evidence: read the spec/task first when available, then changed tests before production code. Tests reveal the expected behavior and whether the change is actually verified. diff --git a/.agents/skills/debug/references/instance.md b/.agents/skills/debug/references/instance.md index cd583d5e8e..f30c8c9e0e 100644 --- a/.agents/skills/debug/references/instance.md +++ b/.agents/skills/debug/references/instance.md @@ -20,7 +20,7 @@ Use an isolated instance for browser/UI repros and API probing that mutates data # Backend only: scripts/dev-isolated -# Backend + Next.js frontend: +# Backend + web frontend: scripts/dev-isolated --web ``` diff --git a/.agents/skills/e2e/SKILL.md b/.agents/skills/e2e/SKILL.md index ccd063d540..f7359e8ad8 100644 --- a/.agents/skills/e2e/SKILL.md +++ b/.agents/skills/e2e/SKILL.md @@ -61,7 +61,7 @@ pnpm e2e:docker # force the docker CI image (full pnpm e2e:clean # remove build/test artifacts, incl. root-owned ones from prior docker runs ``` -The runner solves the sharp edges hand-rolling would hit: in docker it builds the CGO backend on the **host** and runs it in the runtime image (forward-compatible when the host glibc ≤ the image's — the usual case; it smoke-tests this and only falls back to the build image if the host is newer), builds the FE standalone on the host, pre-creates the standalone symlinks as relative links so in-container `global-setup` doesn't recreate them as root, and keeps Playwright output container-local. See `apps/web/e2e/README.md` → "the managed runner". +The runner solves the sharp edges hand-rolling would hit: in docker it builds the CGO backend on the **host** and runs it in the runtime image (forward-compatible when the host glibc ≤ the image's — the usual case; it smoke-tests this and only falls back to the build image if the host is newer), builds the Vite web assets on the host, runs them through the Go-served SPA, and keeps Playwright output container-local. See `apps/web/e2e/README.md` → "the managed runner". ### Raw commands (when you need fine control) @@ -107,7 +107,7 @@ terminal stuck on "Starting terminal...". When a PR-specific E2E shard fails, first identify the failed spec(s). If failures are in unrelated existing specs and no changed code plausibly affects that surface, record the failure as unrelated in the PR fixup summary instead of changing unrelated tests. -**CRITICAL: E2E tests run against the production build** (`.next/standalone/`), not dev mode. After any frontend code change, you **must** rebuild before running tests (`pnpm e2e:run` does this for you): +**CRITICAL: E2E tests run against the production Vite build served by the Go backend**, not dev mode. After any frontend code change, you **must** rebuild before running tests (`pnpm e2e:run` does this for you): ```bash make build-web # ~30s, required after every frontend change @@ -118,7 +118,7 @@ Without this, tests run against stale code and failures are misleading. `make bu ## Writing a test 1. Read `helpers/api-client.ts` and `pages/` to discover available seed methods and page objects -2. Import fixtures from `../../fixtures/test-base` — provides `testPage`, `apiClient`, and `seedData` (pre-created workspace with default workflow). Pull `backend` from the fixture too when you need the backend URL — it's worker-scoped, dynamic, and `process.env.KANDEV_API_BASE_URL` is **not** set in the Playwright runner (only in the frontend SSR child process). Use `backend.baseUrl`. +2. Import fixtures from `../../fixtures/test-base` — provides `testPage`, `apiClient`, and `seedData` (pre-created workspace with default workflow). Pull `backend` from the fixture too when you need the backend URL — it's worker-scoped, dynamic, and `process.env.KANDEV_API_BASE_URL` is **not** set in the Playwright runner. Use `backend.baseUrl`. 3. Use `data-testid` attributes for selectors — add them to components as needed 4. Use page objects for common interactions; create new ones for new pages 5. For GitHub features, use `apiClient.mockGitHub*()` methods to seed mock data @@ -128,7 +128,7 @@ Without this, tests run against stale code and failures are misleading. `make bu - **`apiClient.createTaskWithAgent(...)` returns `CreateTaskResponse`**, which is `Task & { session_id?: string; agent_execution_id?: string }`. Read `created.session_id` directly — don't call `listTaskSessions(taskId)` just to fetch the session that was auto-started by the same call. - **The URL `/t/:id` contains the TASK ID**, not the session ID. Backend routes like `/port-proxy/:sessionId/:port/*path` expect the session ID. Don't extract IDs from `window.location.pathname` when you need a session ID — pull from the API response. - **`page.request` shares cookies/storage with the page context**. Fine for the current no-auth local backend; if auth ever lands, this is where you'd plug it in. -- **Flows that go through a Next.js server action fetch the backend server-side** (from the SSR process, not the browser). `page.waitForResponse("**/api/v1/...")` on the backend URL will never fire — the browser only sees the POST to the server-action endpoint. Assert the user-visible outcome (redirect, toast, store change) and/or re-query state via `apiClient` instead. Client-side fetches (most `lib/api/domains/*` calls) *are* visible to `waitForResponse`; server actions in `app/actions/*` are not. +- **Go boot-payload data is available before React mounts.** Routes that hydrate from `window.__KANDEV_BOOT_PAYLOAD__` may not issue a browser-visible API request on first paint. Use `apiClient` to seed or re-query backend state, assert the user-visible outcome, and reserve `page.waitForResponse("**/api/v1/...")` for client-side fetches that the browser actually performs. - **Preview iframe tests:** the seed repo has no `dev_script` configured, so the preview panel renders a placeholder ("Configure a dev script…") and the URL input never appears — tests that try to drive it hang on the locator timeout. To use the preview iframe in a test, set one first: `await apiClient.updateRepository(seedData.repositoryId, { dev_script: "echo dev" })`. Then click the Preview dockview tab (`await session.clickTab("Preview")`) — the toolbar will mount and the URL input becomes targetable. Example: @@ -234,7 +234,7 @@ Create `apps/web/.pr-assets/manifest.json` so the `/pr` skill picks them up: ### Final verification -Always verify against the production build before finishing — dev mode can hide SSR/hydration issues: +Always verify against the production build before finishing — dev mode can hide boot-payload, asset-serving, or hydration issues: ```bash playwright-cli close @@ -256,7 +256,7 @@ Tests are grouped by feature area in subdirectories under `tests/`. When creatin ## Test quality guidelines - **Test through the UI, not the API.** E2E tests verify user-facing behavior. Don't write tests that only call the API and assert the response -- those are integration tests. Instead, navigate to the page, interact with UI elements, and assert what the user sees. -- **Verify persistence with page reload.** After changing a setting or creating data, reload the page (`testPage.reload()`) and assert the state is still correct. This catches hydration bugs and SSR/client mismatches. +- **Verify persistence with page reload.** After changing a setting or creating data, reload the page (`testPage.reload()`) and assert the state is still correct. This catches hydration bugs and Go boot-payload/client-store mismatches. - **Seed via API, assert via UI.** Use `apiClient` to set up preconditions quickly, but always verify the result by opening the page and checking the DOM. - **Workflow/session invariants.** For session-primary/profile behavior, prefer polling backend state with `apiClient.listTaskSessions(taskId)` for invariants such as `agent_profile_id`, `is_primary`, `state`, and session count, then add UI assertions as secondary evidence. UI tab markers can lag or be absent when the backend invariant is the behavior under test. - **Scope terminal helpers to the active panel.** Terminal/mobile helpers must avoid document-wide `.xterm` or `terminal-xterm-host` selectors because multiple terminal panels can be mounted at once. Scope locators through `data-testid="terminal-panel"` and prefer the visible or latest panel for `page.evaluate` helpers. @@ -295,7 +295,7 @@ When a test fails: - **"Backend did not become healthy"** — run `make build-backend build-web`, check with `E2E_DEBUG=1` - **"Cannot find module"** — run `cd apps && pnpm install` - **Port conflicts** — backends use 18080+ and frontends use 13000+ (per worker), auto-offset by `E2E_PORT_OFFSET` (derived from PID). Set `E2E_PORT_OFFSET=0` for deterministic ports -- **Auto-started session never goes idle** — for sessions started by the same call that creates them, the mock agent can finish before the client WS subscription registers, so a raw `idleInput()` visibility wait hangs. Use `SessionPage.waitForChatIdle()` instead; it reloads once and re-derives state from SSR. +- **Auto-started session never goes idle** — for sessions started by the same call that creates them, the mock agent can finish before the client WS subscription registers, so a raw `idleInput()` visibility wait hangs. Use `SessionPage.waitForChatIdle()` instead; it reloads once and re-derives state from the Go boot payload. - **Flaky timeouts** — **never increase locator timeouts to fix flaky tests.** If a locator times out, the root cause is almost always something else: a setup failure, missing navigation, race condition, or the element genuinely not rendering. Investigate why the element never appears instead of giving it more time. Note: infrastructure health timeouts (30s in `fixtures/backend.ts`) and overall test timeouts (60s in `playwright.config.ts`) are separate and should not be modified either. - Screenshots on failure, video on first retry (CI) @@ -312,7 +312,7 @@ make build-backend build-web cd apps/web && npx playwright test --config e2e/playwright.config.ts --shard=2/10 ``` -E2E tests run against the **production build** (`next build`), not dev mode. Always rebuild with `make build-web` (or `pnpm --filter @kandev/web build`) after code changes before running E2E tests locally. +E2E tests run against the **production Vite build served by the Go backend**, not dev mode. Always rebuild with `make build-web` (or `pnpm --filter @kandev/web build`) after code changes before running E2E tests locally. ```bash # Unzip a shard's blob report from CI artifacts diff --git a/.agents/skills/mobile-parity/SKILL.md b/.agents/skills/mobile-parity/SKILL.md index 7c9ccf1089..90531fd9cc 100644 --- a/.agents/skills/mobile-parity/SKILL.md +++ b/.agents/skills/mobile-parity/SKILL.md @@ -50,7 +50,7 @@ If task has no UI surface, say why this skill does not apply and continue. - Even small user-facing UI tweaks need at least focused rendered verification when feasible: dev-server/browser check, Playwright screenshot, or targeted E2E. If not run, report the exact reason. - Check that text does not overlap, controls remain clickable, focus/keyboard flows still work, and no unintended horizontal scroll appears. - Run the focused Playwright tests. If full E2E cannot run, report the command and blocker. - - E2E runs against the standalone build, not a dev server, so rebuild after frontend changes: `make build-web` (and `make build-backend` for Go), or use `make test-e2e` which rebuilds both. Skipping this silently tests stale code. See `/e2e`. + - E2E runs against the production Vite build served by the Go backend, not a dev server, so rebuild after frontend changes: `make build-web` (and `make build-backend` for Go), or use `make test-e2e` which rebuilds both. Skipping this silently tests stale code. See `/e2e`. ## Mobile E2E Expectations diff --git a/.agents/skills/plan/SKILL.md b/.agents/skills/plan/SKILL.md index 99bfe7cd11..9ee3509dbd 100644 --- a/.agents/skills/plan/SKILL.md +++ b/.agents/skills/plan/SKILL.md @@ -129,7 +129,7 @@ For each user-facing scenario in the spec: Group all task files into waves. Parallelism rules: - Backend packages: can run in parallel -- Frontend (Next.js): single build — only one task at a time +- Frontend (Vite/React SPA): usually sequential because shared build, type, and state surfaces overlap - E2E: after all backend + frontend changes are done ``` diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index a0d50e2b9d..b237282bc0 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -35,7 +35,7 @@ Create these tasks immediately (use your task/todo tracking tool if available): 3. **Triage review comments** — Classify each comment as valid, already addressed, nitpick, or wrong 4. **Address each comment** — Fix or reply with reasoning, resolve threads 5. **Verify, commit, push** — Use `verify` when available; otherwise run the full verification commands directly; commit fixes; push -6. **Re-check** — Use `pr-poller` again when available; otherwise re-check directly. If new failures, loop back to task 2 +6. **Re-check** — Use `pr-poller` again when available; otherwise re-check directly until failures, pending checks, and unresolved review threads are clear. If new failures, loop back to task 2 7. **Summary** — Report what was done Then start with task 1. Mark each task in_progress when you begin it and completed when you finish it. @@ -238,10 +238,27 @@ gh api repos/:owner/:repo/pulls//comments gh pr view --json comments ``` +When `scripts/pr-resolve list ` returns a `comment_id`, prefer fetching the exact review comment over bulk-fetching all PR comments/reviews if only one or two unresolved threads are actionable: + +```bash +gh api repos/:owner/:repo/pulls/comments/ --jq .body +``` + +This keeps context small and avoids mixing current unresolved review comments with old bot summaries. + When piping `gh api` or `gh api graphql` to `jq`, never use `2>&1`. `gh` writes diagnostics to stderr, and merging them corrupts the JSON stream (`jq: parse error: Invalid numeric literal`). Use `2>/dev/null` or `gh`'s built-in `--jq`. Bot issue comments such as CodeRabbit walkthroughs, Claude summaries, Greptile summaries, and historical "actionable comments posted" notices are informational once `scripts/pr-resolve list ` is empty and the latest bot check is passing. Do not reply to old top-level summaries unless they contain a current unresolved request. +To verify whether OpenCode posted a no-findings or diagnostic PR-visible comment, search issue comments for its stable markers, not only review threads: + +```bash +gh api repos/:owner/:repo/issues//comments \ + --jq '.[] | select(.body | contains("opencode-review")) | {user:.user.login, created_at, updated_at, body}' +``` + +OpenCode valid-empty output is represented by ``; absence of inline review comments does not mean OpenCode skipped review. + **Verify before implementing.** Do not blindly accept review feedback — evaluate each comment technically: For each comment: @@ -317,6 +334,9 @@ scripts/pr-resolve show # Raw fallback for a single review comment: gh api repos/:owner/:repo/pulls/comments/ --jq '{body,path,line,commit_id,html_url}' +# Repo helper fallback for a single review comment: +scripts/pr-state --comment + # Raw fallback for all PR review comments when the helper only supports list/reply: gh api repos/:owner/:repo/pulls//comments --paginate @@ -328,6 +348,8 @@ Bots may post duplicate or stale review threads against the previous head immedi For valid comments: read the file, implement the fix, then call `pr-resolve reply` with a body that names the commit or the file:line of the fix. +If the fix moves, splits, or deletes tests or source files, check both the original and destination paths before committing. Use `rg` for symbols or imports that were removed from the original file, and run targeted lint/typecheck commands that cover every touched file. File splits can leave missing imports or stale references in the file that stayed behind even when the moved test passes. + For skipped comments (already addressed, nitpick, wrong, outdated): call `pr-resolve reply` with a body that explains why. Examples: - "This is already handled by X on line Y." - "This is a style preference not enforced by our linters — keeping as-is." @@ -443,8 +465,9 @@ After the push, CI restarts and bots may re-review. Use `pr-poller` again when a - Before waiting on CI, run `scripts/pr-resolve list ` and address any newly opened review threads. Review bots may add new unresolved threads after earlier threads were resolved. - The final re-check must include `scripts/pr-state --summary ` (or a fresh pr-poller report), not only CI status. New review threads can arrive after a fresh push even when the previous unresolved count was zero. -- If `ci_failed:` is empty AND `unresolved_review_threads: 0` AND `issue_comments_from_bots: 0` (no new bot comments to address) → mark task 6 completed and proceed to summary. -- If a pushed fix restarts CI while previous checks were already in progress, continue using `scripts/pr-state --summary `. Treat a stable state of `failed_checks: []` plus `unresolved_review_thread_count: 0` as the actionable fixup completion point once all current review threads have been resolved. Current-head CI can legitimately reset to queued after a push, especially for long E2E, CodeQL, or preview jobs. If checks remain pending after several clean polls, report the exact pending checks instead of waiting indefinitely unless the user explicitly asks to wait for all CI. +- If `ci_failed:` is empty AND `unresolved_review_threads: 0` AND `issue_comments_from_bots: 0` (no new bot comments to address) → keep polling until the equivalent `scripts/pr-state --summary ` fields are also clean: `failed_checks: []`, `pending_checks: []`, and `unresolved_review_thread_count: 0`. +- Treat a clean intermediate state as provisional while bot checks are still pending. Reviewer services can add new unresolved threads after CI is mostly green, so after resolving bot feedback and pushing a fixup, wait for both `pending_checks: []` and `unresolved_review_thread_count: 0` before declaring done. +- If a pushed fix restarts CI while previous checks were already in progress, continue using `scripts/pr-state --summary `. Treat `filtered_review_thread_count` as informational when `unresolved_review_thread_count` is `0`; do not re-open or re-handle already-resolved historical/current-head filtered threads. Current-head CI can legitimately reset to queued after a push, especially for long E2E, CodeQL, or preview jobs. If checks remain pending when the 20-min cap or 3-iteration loop limit is reached, report the exact pending checks instead of waiting indefinitely unless the user explicitly asks to keep waiting. - If new CI failures appeared from the latest commit → loop back to task 2 and reset task 2-5 to `in_progress` as needed. - If new review comments appeared after the push → loop back to task 3. - If the poller hit its cap (`recommendation:` mentions "timed out") → surface the remaining pending items to the user and stop. diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index 6017cfa122..0a54c8485b 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -50,6 +50,7 @@ description: Commit, push, and create a PR. Default is ready-for-review with aut - Fill the template's required sections from the actual diff, commits, and verification performed. - Remove optional sections that add no value for this change. - Preserve static required sections such as checklists exactly as the template provides them; do not pre-fill unchecked boxes. + - For docs-only PRs, keep code-centric checklist items unchanged when they do not apply, and list the docs-safe validation commands actually run. - Include related issue closing text only when an actual issue number is known. - Remove all HTML comments/placeholders from the final body. - Do NOT add tool attribution footers. diff --git a/.agents/skills/push/SKILL.md b/.agents/skills/push/SKILL.md index 3db9c2e634..bdf63de5ed 100644 --- a/.agents/skills/push/SKILL.md +++ b/.agents/skills/push/SKILL.md @@ -38,6 +38,8 @@ Commit any pending changes and push to the remote branch. git push ``` If the branch has no upstream, use `git push -u origin `. + If the branch was rebased or history was rewritten, first confirm the current + branch is not `main` or `master`, then use `git push --force-with-lease`. 4. **Report** the pushed commit hash and branch. diff --git a/.agents/skills/spec-driven-development/SKILL.md b/.agents/skills/spec-driven-development/SKILL.md index 8aab67e846..f0fa1136d2 100644 --- a/.agents/skills/spec-driven-development/SKILL.md +++ b/.agents/skills/spec-driven-development/SKILL.md @@ -148,7 +148,7 @@ Wave 4: E2E, integration, QA, docs Parallelize only when safe: - Backend packages can often run in parallel if they do not edit the same files or migrations. -- Frontend tasks are usually sequential because Next.js build/types/state surfaces overlap. +- Frontend tasks are usually sequential because Vite/React SPA build, type, and state surfaces overlap. - Database migrations, generated API types, shared DTOs, and package-wide config are sequential. - E2E runs happen after backend/frontend integration is coherent. diff --git a/.agents/skills/verify/SKILL.md b/.agents/skills/verify/SKILL.md index 124fb6511d..7bee9bbd64 100644 --- a/.agents/skills/verify/SKILL.md +++ b/.agents/skills/verify/SKILL.md @@ -44,11 +44,12 @@ make lint ``` Before rebasing, check whether `origin/main` is already an ancestor of `HEAD`. -If `git rebase origin/main` fails because there are unstaged tracked changes, -stash only the relevant tracked changes for the intended commit, rebase, then -pop the stash before running `make fmt/typecheck/test/lint`. Do not stash -unrelated user changes blindly. Resolve conflicts before continuing -verification. +If tracked files for the intended change are dirty, stash only those pathspecs +before `git rebase origin/main`, then pop the stash before running +`make fmt/typecheck/test/lint`. Do not use a broad `git stash` that could hide +unrelated user changes. If you miss this and `git rebase origin/main` fails +because of unstaged tracked changes, apply the same pathspec-only stash flow, +then rerun the rebase. Resolve conflicts before continuing verification. If `make fmt` changes files, review the diff and continue with the remaining commands. If any command fails, fix the issue and re-run the failed command; for formatter-caused changes, re-run any affected checks before reporting success. diff --git a/AGENTS.md b/AGENTS.md index dedfe38753..39c9b9e87b 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -80,7 +80,9 @@ Every code change must include tests for new or changed logic. Backend: `*_test. ### GitHub Operations Skills use `gh` CLI by default. If a `gh` command fails (not installed, not authenticated, etc.), use whatever GitHub tools are available in the environment (MCP GitHub tools, API tools, etc.) to accomplish the same operation. The goal is the same — the tool may differ. -For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary ` for checks and unresolved-thread state, `scripts/pr-resolve list ` for actionable unresolved review threads, and `scripts/pr-resolve reply ""` to reply, resolve, and react in one call. +For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary ` for checks and unresolved-thread state, `scripts/pr-state --comment ` for a full review-comment body, `scripts/pr-resolve list ` for actionable unresolved review threads, and `scripts/pr-resolve reply ""` to reply, resolve, and react in one call. + +When a Kandev system message references an MCP tool that is not visible in the active tool list, use the runtime's tool discovery mechanism, such as `tool_search` when available, before falling back to a less specific workflow. Some task messaging and platform helpers are exposed on demand. ### Third-party integrations diff --git a/docs/nextjs-spa-migration.md b/docs/nextjs-spa-migration.md index 9ce9d6cee5..ce867e01d4 100644 --- a/docs/nextjs-spa-migration.md +++ b/docs/nextjs-spa-migration.md @@ -29,6 +29,20 @@ payload needed for first paint. 4. Add or update focused tests before broad verification. 5. Re-run the Next audit command before handing off. +### Rebase loop checklist + +Use this loop when bringing an older branch across the SPA migration boundary: + +1. Capture the old base: `old_main="$(git rev-parse origin/main)"`. +2. Fetch and rebase onto the new `origin/main`. +3. Inspect only newly merged main commits: `git log --oneline "$old_main"..origin/main`. +4. For web files touched by those commits, scan for `next/*`, server-data, + route-loading, and boot-payload regressions before changing code. +5. Migrate only when the rebased branch reintroduces old Next runtime patterns + or conflicts with the Go-served SPA contract. +6. Run focused E2E for the touched area; broaden only when the touched area is + shared routing, boot payload, or app shell behavior. + Useful audit commands: ```bash diff --git a/scripts/pr-state b/scripts/pr-state index 9205e4d195..4f3e017b83 100755 --- a/scripts/pr-state +++ b/scripts/pr-state @@ -3,6 +3,7 @@ # # Usage: # scripts/pr-state [--summary] [--all] +# scripts/pr-state --comment # # Raw output: # JSON object with: @@ -30,6 +31,10 @@ # - summary_notes: [] # - errors: [{ source, message }] # +# Comment output (--comment): +# JSON object for one PR review comment: +# - comment_id, author, body, path, line, original_line, commit_id, html_url, created_at, updated_at +# # By default, comments/reviews/review threads are limited to items created after # the PR head commit. Checks are already scoped by GitHub to the current head. # Pass --all to include historical comments and reviews. @@ -45,6 +50,7 @@ usage() { INCLUDE_ALL=0 SUMMARY=0 +COMMENT_ID='' PR='' while [[ $# -gt 0 ]]; do case "$1" in @@ -56,6 +62,13 @@ while [[ $# -gt 0 ]]; do SUMMARY=1 shift ;; + --comment) + shift + [[ $# -gt 0 && -z "${COMMENT_ID:-}" && -z "${PR:-}" ]] || usage 1 + [[ "$1" =~ ^[0-9]+$ ]] || usage 1 + COMMENT_ID=$1 + shift + ;; -h | --help) usage 0 ;; @@ -79,9 +92,14 @@ while [[ $# -gt 0 ]]; do esac done -[ -n "${PR:-}" ] || usage [ $# -eq 0 ] || usage +if [[ -n "${COMMENT_ID:-}" ]]; then + [[ -z "${PR:-}" && "$SUMMARY" == "0" && "$INCLUDE_ALL" == "0" ]] || usage +else + [ -n "${PR:-}" ] || usage +fi + errors='[]' pr_json='{}' checks_json='[]' @@ -106,6 +124,45 @@ gh_json() { gh "$@" 2>/dev/null } +emit_comment_json() { + local comment_id=$1 + local repo_json + local owner + local repo + local comment_json + + repo_json="$(gh_json repo view --json owner,name)" + owner="$(jq -r '.owner.login // ""' <<<"$repo_json")" + repo="$(jq -r '.name // ""' <<<"$repo_json")" + if [[ -z "$owner" || -z "$repo" ]]; then + printf 'failed to resolve repository identity\n' >&2 + return 1 + fi + + if ! comment_json="$(gh_json api "repos/$owner/$repo/pulls/comments/$comment_id")"; then + printf 'failed to fetch PR comment %s\n' "$comment_id" >&2 + return 1 + fi + + jq -c '{ + comment_id: (.id // null), + author: (.user.login // "unknown"), + body: (.body // ""), + path: (.path // null), + line: (.line // null), + original_line: (.original_line // null), + commit_id: (.commit_id // null), + html_url: (.html_url // null), + created_at: (.created_at // null), + updated_at: (.updated_at // null) + }' <<<"$comment_json" +} + +if [[ -n "${COMMENT_ID:-}" ]]; then + emit_comment_json "$COMMENT_ID" + exit $? +fi + extract_checks_json() { local status_json=$1 jq -cn --argjson checks_in "$status_json" ' diff --git a/scripts/pr-state.test.sh b/scripts/pr-state.test.sh index b8d15d6aff..e0a60dfe97 100644 --- a/scripts/pr-state.test.sh +++ b/scripts/pr-state.test.sh @@ -64,11 +64,34 @@ if [[ "${GH_FAIL_REPO:-0}" == "1" && "$1" == "repo" && "$2" == "view" ]]; then exit 1 fi +if [[ "${GH_FAIL_COMMENT:-0}" == "1" && "$1" == "api" && "$2" == repos/kdlbs/kandev/pulls/comments/* ]]; then + echo "comment api failed" >&2 + exit 1 +fi + if [[ "$1" == "repo" && "$2" == "view" ]]; then printf '{"owner":{"login":"kdlbs"},"name":"kandev"}\n' exit 0 fi +if [[ "$1" == "api" && "$2" == "repos/kdlbs/kandev/pulls/comments/111" ]]; then + cat <<'JSON' +{ + "id": 111, + "user": { "login": "greptile-apps[bot]" }, + "body": "Please rename this helper\n\nFull rationale here.", + "path": "apps/web/file.ts", + "line": 42, + "original_line": 40, + "commit_id": "abc123", + "html_url": "https://github.com/kdlbs/kandev/pull/123#discussion_r111", + "created_at": "2026-06-01T10:00:00Z", + "updated_at": "2026-06-01T10:01:00Z" +} +JSON + exit 0 +fi + if [[ "$1" == "pr" && "$2" == "view" && "$4" == "--json" ]]; then if [[ "${GH_NO_PR_URL:-0}" == "1" ]]; then pr_url='null' @@ -530,6 +553,78 @@ test_summary_all_flag_includes_historical_unresolved_threads() { pass "--summary --all includes historical unresolved thread comments" } +test_comment_mode_returns_full_review_comment() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + PATH="$tmp/bin:$PATH" "$SCRIPT" --comment 111 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "comment id" '.comment_id == 111' "$json" + assert_jq "comment body is full" '.body | contains("Full rationale here.")' "$json" + assert_jq "comment path" '.path == "apps/web/file.ts"' "$json" + assert_jq "comment line" '.line == 42' "$json" + assert_jq "comment author" '.author == "greptile-apps[bot]"' "$json" + pass "--comment returns full review comment" +} + +test_comment_mode_reports_fetch_failure() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + if GH_FAIL_COMMENT=1 PATH="$tmp/bin:$PATH" "$SCRIPT" --comment 999 >"$tmp/out.json" 2>"$tmp/err.log"; then + fail "--comment failure exits non-zero" + fi + + if [ -s "$tmp/out.json" ]; then + fail "--comment failure does not emit JSON" + fi + if ! grep -q "failed to fetch PR comment 999" "$tmp/err.log"; then + fail "--comment failure reports clear error" + fi + + pass "--comment reports fetch failure" +} + +test_comment_mode_rejects_incompatible_flags() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + if PATH="$tmp/bin:$PATH" "$SCRIPT" --summary --comment 111 >"$tmp/out.log" 2>&1; then + fail "--comment rejects --summary" + fi + if ! grep -q "scripts/pr-state --comment " "$tmp/out.log"; then + fail "--comment --summary prints usage" + fi + + if PATH="$tmp/bin:$PATH" "$SCRIPT" --all --comment 111 >"$tmp/out.log" 2>&1; then + fail "--comment rejects --all" + fi + if ! grep -q "scripts/pr-state --comment " "$tmp/out.log"; then + fail "--comment --all prints usage" + fi + + if PATH="$tmp/bin:$PATH" "$SCRIPT" --comment --summary >"$tmp/out.log" 2>&1; then + fail "--comment rejects flag-shaped id" + fi + if ! grep -q "scripts/pr-state --comment " "$tmp/out.log"; then + fail "--comment flag-shaped id prints usage" + fi + + if PATH="$tmp/bin:$PATH" "$SCRIPT" --comment abc >"$tmp/out.log" 2>&1; then + fail "--comment rejects non-numeric id" + fi + if ! grep -q "scripts/pr-state --comment " "$tmp/out.log"; then + fail "--comment non-numeric id prints usage" + fi + + pass "--comment rejects incompatible flags" +} + test_snapshot_happy_path test_partial_failure_records_error_but_keeps_other_data test_pr_view_failure_with_non_numeric_ref_keeps_schema @@ -539,3 +634,6 @@ test_graphql_pagination_collects_all_threads test_all_flag_includes_historical_comments_and_reviews test_summary_mode_returns_compact_fixup_state test_summary_all_flag_includes_historical_unresolved_threads +test_comment_mode_returns_full_review_comment +test_comment_mode_reports_fetch_failure +test_comment_mode_rejects_incompatible_flags