diff --git a/.agents/agents/pr-poller.md b/.agents/agents/pr-poller.md index e2cc4f32e4..8324397130 100644 --- a/.agents/agents/pr-poller.md +++ b/.agents/agents/pr-poller.md @@ -53,11 +53,12 @@ The `claude_summary` line carries the **latest** Claude summary's structured fin a. **CI status:** ```bash - gh pr checks --json name,status,conclusion,detailsUrl + gh pr checks --json name,workflow,status,conclusion,detailsUrl ``` - `status` ∈ `{queued, in_progress, completed}` - `completed` + `conclusion=success` → passing - - `completed` + `conclusion∈{failure,cancelled,timed_out}` → failed + - `completed` + `conclusion∈{failure,timed_out}` → failed + - `completed` + `conclusion=cancelled` → **check for supersession first**: if a newer run of the **same workflow** exists for the same head SHA (`gh run list --workflow "" --json headSha,conclusion,databaseId,createdAt` — use the check's `workflow` field, not `name`; `name` is the job name e.g. `ci/build`, `workflow` is e.g. `CI`), the cancelled one is a concurrency-superseded duplicate — exclude it from `ci_failed` (report it as `conclusion=cancelled (superseded)` at most). Only treat a cancelled run as failed when it is the newest run for that SHA. - anything else → pending b. **Bot reviews** (terminal conditions): diff --git a/.agents/agents/verify.md b/.agents/agents/verify.md index 2b783e74c5..dabe2b133c 100644 --- a/.agents/agents/verify.md +++ b/.agents/agents/verify.md @@ -53,6 +53,7 @@ Run the full verification pipeline for the monorepo, then fix any issues found. - **Lint — unused imports**: remove them - **Lint — duplicate strings**: extract to a constant - **Test failures**: read the test, understand the assertion, fix the code (not the test) unless the test is outdated + - **Test failures — goleak (package passes, TestMain fails):** loop the failing package with race to match CI: `go test -race -count=10 ./internal//...`. If stack traces mention `sleepOrStop` / `connectWorkspaceStream`, fix test cleanup (see backend AGENTS.md leak-testing section). - After fixing, re-run **only** the failed command via `scripts/run-quiet` to confirm the fix. Do not re-run the whole pipeline on every iteration. 5. **Repeat** steps 3-4 until all commands pass. If a fix introduces new issues, address those too. diff --git a/.agents/skills/fix/SKILL.md b/.agents/skills/fix/SKILL.md index 30005ef143..71249a3693 100644 --- a/.agents/skills/fix/SKILL.md +++ b/.agents/skills/fix/SKILL.md @@ -31,13 +31,33 @@ A bug fix does **not** produce a spec. Specs describe product features; bugs are Create these tasks immediately (use your task/todo tracking tool if available): +0. **Read the issue + view attachments** — When the bug originates from an issue tracker, fetch the canonical issue and view every image attachment before hypothesizing 1. **Reproduce the bug** — Write a test or find a reliable reproduction case 2. **Find the root cause** — Trace the code path, narrow the scope, state the cause clearly 3. **Fix with TDD** — Minimal fix with regression test, no surrounding refactors 4. **Verify** — Run full verification, check for similar patterns elsewhere 5. **Record** — Save any architectural decisions or insights, AND update the related feature spec if the bug exposed a requirement gap -Then start with task 1. Mark each task in_progress when you begin it and completed when you finish it. Do not skip ahead — fixing without reproducing leads to patches that don't address the real problem. Fixing without understanding the root cause leads to whack-a-mole. +Then start with task 0 when the bug is issue-sourced, otherwise task 1. Mark each task in_progress when you begin it and completed when you finish it. Do not skip ahead — fixing without reading the source issue (when one exists) or reproducing leads to patches that don't address the real problem. Fixing without understanding the root cause leads to whack-a-mole. + +--- + +## Phase 0: Read the issue + view attachments + +Mark task 0 as in_progress (skip this phase if there is no issue tracker source — e.g. a locally discovered bug with no linked issue). + +When the bug originates from an issue tracker, fetch the **canonical issue** and view **every image attachment** before hypothesizing. Handed-down restatements (Kandev task/subtask text, Slack paste, PR description) are leads only — verify against the source. + +```bash +gh issue view --repo --json title,body,comments +curl -sL "https://github.com/user-attachments/assets/" -o /tmp/issue--.png # then Read the local file +``` + +- **Transcribe exact strings from screenshots** into the repro (paths, error messages, UI labels). Screenshots often hold details missing from the text summary. +- **Mine structured issue-template fields** (OS, install mode, clean-state, version) to constrain repro conditions. +- **Video attachments** cannot be Read as stills — note their presence and rely on the issue text/comments for motion-specific details. + +Mark task 0 as completed. --- diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index 70975a55be..8c3b362de9 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -58,6 +58,8 @@ Invoke the `pr-poller` subagent with the PR number (or let it resolve via `gh pr - `bots.` — `done` / `rate_limited` / `pending` / `timeout`. Anything in `done` or `rate_limited` has had its chance; treat the rest as missing data, not a blocker. - `unresolved_review_threads` and `issue_comments_from_bots` — drive steps 3-4. If both are 0 and `ci_failed` is empty, skip to step 5 (still run verify + push if you have fixes from earlier). +**E2E CI outlasts the poller.** The pr-poller caps at ~20 minutes. The E2E matrix (10 shards × 2 projects) often runs longer. If the report shows `ci_pending` with only E2E/lint jobs and `ci_failed` is empty, re-invoke pr-poller once those jobs finish — do not spin a manual `gh pr checks` loop in the parent. If the cap hits with E2E still pending, report "CI in progress" to the user instead of blocking. + **Do not fetch poll output yourself** — that is what burns context. The report is the only thing that enters your context. Mark task 1 as completed. @@ -85,6 +87,8 @@ gh run list --branch --workflow "" --limit 10 --json con On long-lived PRs that get rebased/squashed, prior SHAs on the same branch often passed the same workflow. A `passing → failing` boundary tells you the regression is isolated to the most recent rework — diff against the last passing SHA (`git diff ..HEAD`) instead of against `main` to narrow the search dramatically. +**Recognize a cancelled concurrency-duplicate before reading any logs.** A required check with `conclusion=cancelled` — often annotated *"Canceling since a higher priority waiting request … exists"*, with 0s job durations and **unexpanded** `${{ matrix.* }}` job names (a "Merge reports" job may exit 1 for missing blobs) — is a concurrency-group artifact from a superseded run, **not a real failure**. GitHub re-triggers PR workflows when the base branch moves (e.g. a release lands on main), cancelling the in-flight run. Confirm the *non-cancelled* run for the **same head SHA** passed (`gh run list --workflow "" --json headSha,conclusion,databaseId`), then trigger a single clean run (rebase onto main + force-push, or `gh run rerun `) instead of debugging code. + **E2E test failures require special handling:** If any failing check is an E2E test (Playwright): @@ -208,6 +212,18 @@ for TID in "${!CAT[@]}"; do done ``` +### Verify resolution before moving on + +Run `scripts/pr-resolve list ` — output must be empty. + +Or confirm via GraphQL that unresolved thread count is 0: + +```bash +gh api graphql -f query='query { repository(owner:"kdlbs", name:"kandev") { pullRequest(number:) { reviewThreads(first:100) { nodes { isResolved } } } } }' --jq '[.data.repository.pullRequest.reviewThreads.nodes[] | select(.isResolved == false)] | length' +``` + +Informational threads (acknowledged, no code change) still need `scripts/pr-resolve reply` + resolve — skipping them leaves the PR blocked. + Mark task 4 as completed. ### 5. Verify, commit, and push @@ -245,6 +261,10 @@ Cap re-check loops at **3 iterations** to prevent runaway sessions. After 3, sur Mark task 6 as completed. +### Multi-round bot reviews + +**Expect new threads after every push.** CodeRabbit, Greptile, Claude, and cubic often re-review the latest commit and open fresh inline threads even when earlier ones were resolved. On cross-cutting changes (backend event payloads + frontend WS handlers + E2E), plan for 2–3 fixup rounds. After each push, always run `scripts/pr-resolve list ` before declaring done — do not rely on the prior round's zero count. + ### 7. Summary Mark task 7 as in_progress. diff --git a/AGENTS.md b/AGENTS.md index df2e9ce236..175e63b685 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -25,7 +25,7 @@ apps/ ### Worktrees and commit hooks -The commit-msg hook runs `cd apps && pnpm exec commitlint --edit "$1"`. A fresh git worktree shares `.git/` but **not** `apps/node_modules/`, so the first commit attempted in a worktree fails with `Command "commitlint" not found` / `ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL`. Run `pnpm install --frozen-lockfile` from `apps/` in the worktree once before the first commit; subsequent commits work normally. +A fresh git worktree shares `.git/` but **not** `apps/node_modules/`. The missing install breaks not just the commit-msg hook (`pnpm exec commitlint` → `Command "commitlint" not found` / `ERR_PNPM_RECURSIVE_EXEC_FIRST_FAIL`) but any pnpm command — `vitest` fails with `Failed to resolve import "vitest"`, eslint similarly. Run `pnpm install --frozen-lockfile` from `apps/` once after creating the worktree, before running tests/lint/commits; subsequent pnpm commands work normally. --- diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 984d35dcd8..88bf85907d 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -182,6 +182,8 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - **Execution access:** Workspace-oriented handlers (files, shell, inference, ports, vscode, LSP) MUST use `GetOrEnsureExecution(ctx, sessionID)` — it recovers from backend restarts by creating executions on-demand. Only use `GetExecutionBySessionID` for operations that require a running agent process (prompt, cancel, mode). - **Task lifecycle events:** Any code path that mutates a task row must publish via the event bus (`task.created` / `task.updated` / `task.deleted`) — either by going through `Service.CreateTask` / `UpdateTask` / `DeleteTask` / `ArchiveTask`, or by calling `publishTaskEvent` (or one of the `Publish*` helpers in `service_events.go`) directly. Walking `repository.TaskRepository` straight bypasses event publishing and breaks WS-driven UI like the All-Workflows kanban view. `HandoffService`'s cascade methods learned this the hard way — they now require a `TaskEventPublisher` wired via `SetTaskEventPublisher`. New cascade / bulk / cleanup paths must follow the same pattern. - **Testing:** Prefer `testing/synctest` (Go 1.24+) over `time.Sleep` for time-dependent tests. Use `synctest.Test` to wrap tests with tickers or timeouts — it advances fake time instantly when all goroutines are idle. When `synctest` is not feasible (e.g., tests spawning external processes like `git`), use channel-based synchronization (`<-started`, non-blocking `select`) instead of sleep-based waits. Reserve `time.Sleep` only for integration tests that need real subprocess execution time. + - **Test cleanup:** Register `t.Cleanup` immediately after creating resources that need teardown (adapters, `io.Pipe` writers, background goroutines) — before any `t.Fatal`/`t.Fatalf` path. Late cleanup registration leaks pipes and goroutines on early failure. + - **Joining production goroutines in tests:** When code spawns untracked goroutines (e.g. `fireWakeup`), don't rely on arbitrary sleeps. Join via an observable side effect — e.g. block on `EventTypeComplete` from `a.updatesCh` after unblocking the fake agent. Use short timeouts (~100ms) for in-process negative assertions; reserve multi-second waits for subprocess/integration tests only. ### Goroutine ownership and leak testing diff --git a/apps/web/e2e/README.md b/apps/web/e2e/README.md index 49250fc1c0..f985194888 100644 --- a/apps/web/e2e/README.md +++ b/apps/web/e2e/README.md @@ -62,6 +62,8 @@ This project used to be named `docker`. It was renamed to `containers` once SSH ## Commands +`e2e`, `e2e:run`, and `e2e:ui` are defined only in `apps/web/package.json`. Run them from `apps/web` (or `pnpm --filter @kandev/web e2e:run` from elsewhere). From the repo root you get `ERR_PNPM_NO_IMPORTER_MANIFEST_FOUND`. + | Command | What it does | | ---------------------------------- | ------------------------------------------------ | | `pnpm e2e` | Run the default (chromium) project headless. | diff --git a/apps/web/e2e/helpers/seed-session-messages.ts b/apps/web/e2e/helpers/seed-session-messages.ts index 9534475033..ddd46c6b3c 100644 --- a/apps/web/e2e/helpers/seed-session-messages.ts +++ b/apps/web/e2e/helpers/seed-session-messages.ts @@ -1,8 +1,15 @@ /** * Build a mock-agent script that emits N messages with a small delay between - * them. Each entry becomes one agent text event, producing one distinct message - * row in the chat panel. Used as the `description` of a task created via - * `apiClient.createTaskWithAgent` so the session boots with pre-seeded history. + * them. Each entry becomes one agent text event. NOTE: consecutive agent-text + * chunks COALESCE into a single chat message — the backend only flushes a + * distinct message row on a tool-call/turn boundary (manager_streaming. + * flushMessageBuffer), and e2e:delay does not flush. So N lines render as one + * growing bubble, not N rows (search specs tolerate this with N>=1 hits). + * To seed many DISTINCT rows (e.g. for pagination tests), use the test harness + * instead: apiClient.seedToolCallMessages / seedSessionMessage (called repeatedly + * with type "text" for each row), which write separate rows directly. Used as the + * `description` of a task created + * via `apiClient.createTaskWithAgent` so the session boots with pre-seeded history. */ export function multiMessageScript(lines: string[], delayMs = 10): string { const parts: string[] = [];