chore: improve PR fixup and E2E harness guidance (#1351)

2026-06-13 18:59:44 +01:00
parent 42556971ef
commit 224383c557
7 changed files with 52 additions and 9 deletions
@@ -76,7 +76,7 @@ ci: add PR title linting workflow

   Why this matters: a missing hook lets lint regressions slip past local commits and only surface in CI (e.g. funlen / cognitive complexity on backend Go code). The hook catches them in <1s at commit time. See `Makefile`'s `doctor` target for the idempotent install command.

-3. **Run verify (MANDATORY — do NOT skip):** Delegate to the `verify` subagent to run the full verification pipeline (rebase, format, typecheck, test, lint). It will fix any issues it finds. **Wait for it to complete before proceeding.** Do NOT proceed to step 4 until verify passes. If verify cannot fix the failures, stop and surface the errors to the user — do not commit. Do NOT substitute this with a partial check (e.g. running only the changed package's tests).
+3. **Run verify (MANDATORY — do NOT skip):** Run `/verify` to complete the full verification pipeline (rebase, format, typecheck, test, lint). Depending on runtime policy, `/verify` may delegate to the `verify` subagent or use its documented direct-command fallback. **Wait for it to complete before proceeding.** Do NOT proceed to step 4 until verify passes. If verify cannot fix the failures, stop and surface the errors to the user — do not commit. Do NOT substitute this with a partial check (e.g. running only the changed package's tests).

 4. **Stage files:** Stage relevant files (prefer specific files over `git add -A`).
   - **Splitting commits with new files:** When introducing a brand-new file alongside the file that uses it, stage them together. The Go lint pre-commit hook stashes *unstaged* changes before linting but keeps *untracked* files in the working tree — so a new helper committed alone, while its (still-unstaged) caller sits in the working tree, lints as `unused` and rejects the commit.
@@ -220,6 +220,7 @@ Tests are grouped by feature area in subdirectories under `tests/`. When creatin
 - **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.
 - **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.
+- **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.

 ## Debugging failures

@@ -72,9 +72,9 @@ If delegated polling is unavailable, gather the same information directly withou
 scripts/pr-state <PR>
 ```

-Poll at a 30s cadence with a **20 min cap**. Stop early if any required check fails. If the cap hits and only E2E shards are still pending with no failures, report "CI in progress" instead of continuing to watch indefinitely. Do not run `gh pr checks --watch` in the main session unless the runtime can keep the watcher isolated and automatically clean it up.
+Poll at a 30s cadence with a **20 min cap**. Stop early if any required check fails. If the cap hits and only E2E shards are still pending with no failures, report "CI in progress" instead of continuing to watch indefinitely. Do not run `gh pr checks --watch` in the main session unless the runtime can keep the watcher isolated and automatically clean it up. If you do use `gh pr checks --watch`, keep watching until the command exits; GitHub can expand matrix jobs after an initial aggregate "Build" check passes, so the first green build/lint/test rows are not necessarily terminal.

-Summarize the direct-command result from `scripts/pr-state` using its raw snapshot fields: `.checks`, `.review_threads`, `.unresolved_review_thread_count`, `.reviews`, `.issue_comments`, and `.errors`. Derive `ci_failed`, `ci_pending`, and which bot comments are actionable in the parent from those raw arrays. If `.errors` is non-empty, treat the affected fields as unknown and report the data-gathering failure instead of reconstructing them ad hoc.
+Summarize the direct-command result from `scripts/pr-state` using its raw snapshot fields: `.checks`, `.review_threads`, `.unresolved_review_thread_count`, `.reviews`, `.issue_comments`, and `.errors`. Derive `ci_failed`, `ci_pending`, and which bot comments are actionable in the parent from those raw arrays. If `.errors` is non-empty, treat the affected fields as unknown and report the data-gathering failure instead of reconstructing them ad hoc. When deriving non-green checks, filter to actionable failures/pending checks; check/status records with both `status=success` and `conclusion=success` are green even if they appear in a mixed raw array.

 Mark task 1 as completed.

@@ -94,6 +94,11 @@ For each entry in the report's `ci_failed:` list:
   scripts/run-quiet gh-run -- gh run view <run-id> --log-failed
   ```
   If the printed summary is enough, stop. Only `Read` specific line ranges from the printed log path if you need surrounding context.
+   If `gh run view --log-failed` returns only metadata or says logs are unavailable while the workflow is still running, wait for the workflow/report job to finish. If it still omits details, fetch the job log directly and search the downloaded file:
+   ```bash
+   job_id="$(gh api repos/:owner/:repo/actions/runs/<run-id>/jobs --jq '[.jobs[] | select(.conclusion == "failure")] | first | .id')"
+   gh api repos/:owner/:repo/actions/jobs/"$job_id"/logs > job.log
+   ```
 3. Read the relevant source files at the failing lines (use `Read` with `offset`/`limit`, not `cat`)
 4. Fix the issues (lint errors, test failures, type errors, etc.)

@@ -113,13 +118,14 @@ If any failing check is an E2E test (Playwright):

 1. Read the `/e2e` skill (`SKILL.md`) for debugging guidance, test patterns, and run commands
 2. Follow the "Debugging failures" section — read error output, check failure screenshots in `e2e/test-results/`, classify the failure (test logic, frontend, backend)
-3. Fix the root cause. **Never increase timeouts to fix flaky tests** — find the real issue
-4. Confirm fixes pass locally before pushing. Wrap with `scripts/run-quiet`:
+3. For failed shards, identify the exact failing spec/test from logs before making changes.
+4. Fix the root cause. **Never increase timeouts to fix flaky tests** — find the real issue
+5. Confirm fixes pass locally before pushing. Build required artifacts first if global setup needs them, then run the targeted Playwright command. Wrap with `scripts/run-quiet`:
   ```bash
   scripts/run-quiet build -- make build-backend build-web
   scripts/run-quiet e2e -- bash -c 'cd apps && pnpm --filter @kandev/web e2e -- tests/path/to/failing.spec.ts'
   ```
-   Run the specific failing test file(s), not the full suite. Only proceed to step 5 after the test passes locally.
+   Run the specific failing test file(s), not the full suite. Only proceed to the verify/commit/push step after the test passes locally.

 **Don't dismiss a repeated failure as "flaky".** If the same shard or test fails 2+ poll iterations in a row, stop polling and do two cheap checks instead:

@@ -294,6 +300,12 @@ Mark task 5 as in_progress.
   fix: address review feedback and fix CI failures
   ```

+   For conflict-fix PRs, after merging or rebasing `origin/main`, check unresolved review threads again before pushing because conflict-resolution diffs can trigger new comments:
+   ```bash
+   scripts/pr-resolve list <PR>
+   scripts/pr-resolve reply <PR> <CID> <TID> "Fixed in the conflict resolution."
+   ```
+
 3. Push:
   ```bash
   git push
@@ -307,7 +319,9 @@ Mark task 6 as in_progress.

 After the push, CI restarts and bots may re-review. Use `pr-poller` again when available — same helper, same contract, same 20-min cap. If delegated polling is unavailable, use the direct-command fallback from step 1. Parse the new report:

+- Before waiting on CI, run `scripts/pr-resolve list <PR>` and address any newly opened review threads. Review bots may add new unresolved threads after earlier threads were resolved.
 - 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 `scripts/pr-state <PR>` (or the poller report) shows no failed checks and no unresolved review threads, but CI is still queued or in progress after the bounded re-check, it is acceptable to report "CI still in progress" and stop instead of polling indefinitely.
 - 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.
@@ -66,6 +66,8 @@ description: Commit, push, and create a PR. Default is ready-for-review with aut

 5. **If ready (not draft):** Run `/pr-fixup` to wait for CI checks and CodeRabbit, Greptile, Claude, and cubic review feedback, fix any failures or valid comments, and push.

+   CodeRabbit issue comments that only report rate limits or exhausted usage credits are informational. They should not block PR completion when other review threads are resolved and checks are otherwise passing or pending.
+
 6. **PR screenshots:** After creating the PR, check if `apps/web/.pr-assets/manifest.json` exists. If it does:
   - Read the manifest to list available screenshots/GIFs
   - Run `npx tsx apps/web/e2e/scripts/upload-pr-assets.ts <PR_NUMBER>` to generate embed markdown
@@ -5,13 +5,31 @@ description: Run format, typecheck, test, and lint across the monorepo. Use afte

 # Verify

-Delegate to the **`verify` subagent** to run the full verification pipeline (rebase, format, typecheck, test, lint) and fix any issues it finds. The subagent runs on Sonnet, which is cheaper than the main session and well-suited to the mechanical run-parse-fix loop.
+Delegate to the **`verify` subagent** to run the full verification pipeline (rebase, format, typecheck, test, lint) and fix any issues it finds when the runtime supports delegated helpers. The subagent runs on Sonnet, which is cheaper than the main session and well-suited to the mechanical run-parse-fix loop.
+
+If runtime policy forbids delegated helpers/subagents unless the user explicitly requested them, treat delegation as unavailable and use the direct-command fallback below. Do not stop at a partial check just because delegation is unavailable.

 ## What to do

-Invoke the `verify` subagent in a single call. Wait for it to complete and surface the result.
+Invoke the `verify` subagent in a single call when available. Wait for it to complete and surface the result.

 - If verify passes cleanly: report success.
 - If verify cannot fix all failures: surface the remaining errors to the user — do not proceed with downstream actions (commit, push, PR) that depended on a green verify.

-Do NOT run the verification commands yourself in the main session — that defeats the cost saving. The subagent's prompt already contains the full procedure (see `.claude/agents/verify.md`).
+Do NOT run the verification commands yourself in the main session when the helper is available — that defeats the cost saving. The subagent's prompt already contains the full procedure (see `.claude/agents/verify.md`).
+
+## Direct-command fallback
+
+Use this only when the runtime does not permit delegated helpers/subagents. Run the full pipeline directly from the repository root and report the exact commands and results:
+
+```bash
+# If the branch is behind main, rebase first:
+git fetch origin main
+git rebase origin/main
+make fmt
+make typecheck
+make test
+make lint
+```
+
+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.
@@ -214,6 +214,12 @@ Enforced by `apps/backend/.golangci.yml` (errors on new code only):

 When you hit a limit, extract a helper function. Prefer composition over growing a single function.

+When a PR fixup touches backend code, run the CI-style changed-file linter locally from `apps/backend` with the PR base SHA before pushing, because CI enforces changed-file complexity thresholds:
+
+```bash
+golangci-lint run ./... --new-from-rev="<base-sha>" --timeout=5m
+```
+
 ## Further scoped notes

 - `internal/agentctl/AGENTS.md` — agentctl server route groups, adapter model, ACP protocol
@@ -96,3 +96,5 @@ When you hit a limit, extract a helper function, custom hook, or sub-component.
 ## Testing notes

 - jsdom drops `secure` cookies over `http`, so `document.cookie` reads back empty. To assert a cookie write in a Vitest unit test, intercept the setter with `Object.defineProperty(document, "cookie", { set: ... })` and restore it after.
+- In Playwright tests, avoid strict locators that assume only one `terminal-panel` or `.xterm` exists. Mobile and dockview layouts can mount multiple terminal instances; scope to the active panel or use `.first()` / `.last()` deliberately with a comment or helper.
+- Shared E2E helpers that inspect mounted React/DOM internals must be scoped to the active panel/container, not global selectors, because hidden or stale mounted panels can coexist in dock/mobile layouts.