From 58e7be4b78d7396566c5b76c6835535f986add62 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 11 Aug 2026 08:28:25 +0100 Subject: [PATCH] chore: improve agent harness guidance (#2185) --- .agents/agents/pr-poller.md | 17 +- .agents/skills/code-review/SKILL.md | 33 ++- .agents/skills/commit/SKILL.md | 36 +++- .agents/skills/context-engineering/SKILL.md | 5 + .agents/skills/debug/SKILL.md | 21 +- .../skills/debug/references/backend-repro.md | 11 + .agents/skills/debug/references/browser.md | 49 +++-- .agents/skills/e2e/SKILL.md | 109 +++++----- .../skills/e2e/references/fixture-state.md | 19 ++ .../e2e/references/ui-state-and-cleanup.md | 41 ++++ .agents/skills/fix/SKILL.md | 5 + .agents/skills/harness-improvement/SKILL.md | 2 + .../references/validation.md | 21 ++ .agents/skills/plan/SKILL.md | 2 +- .agents/skills/playwright-cli/SKILL.md | 19 +- .../references/request-mocking.md | 27 +++ .agents/skills/pr-fixup/SKILL.md | 192 +++++++++++++----- .../pr-fixup/references/ci-troubleshooting.md | 38 ++++ .../pr-fixup/references/merge-conflicts.md | 62 +++++- .../pr-fixup/references/review-evidence.md | 62 +++++- .agents/skills/pr/SKILL.md | 50 +++-- .agents/skills/push/SKILL.md | 18 +- .agents/skills/release/SKILL.md | 9 + .agents/skills/runtime-feature-flags/SKILL.md | 36 ++-- .agents/skills/tdd/SKILL.md | 4 + .agents/skills/using-agent-skills/SKILL.md | 8 + .codex/agents/pr-poller.toml | 15 +- .github/AGENTS.md | 8 + .opencode/agents/pr-poller.md | 17 +- AGENTS.md | 5 +- Makefile | 1 + apps/backend/AGENTS.md | 46 ++--- apps/backend/cmd/mock-agent/AGENTS.md | 8 +- apps/backend/internal/agentctl/AGENTS.md | 7 + apps/package.json | 1 + apps/pnpm-lock.yaml | 31 +++ apps/web/AGENTS.md | 54 ++--- apps/web/components/review/AGENTS.md | 7 + apps/web/components/task/chat/AGENTS.md | 7 + apps/web/e2e/README.md | 6 + apps/web/e2e/helpers/session.ts | 21 ++ .../chat/agent-message-attribution.spec.ts | 64 +++--- apps/web/e2e/tests/chat/clarification.spec.ts | 32 ++- .../web/e2e/tests/task/autopilot-mode.spec.ts | 28 +-- scripts/playwright-blob-audit | 179 ++++++++++++++++ scripts/playwright-blob-audit.test.py | 85 ++++++++ scripts/pr-state | 136 ++++++++++++- scripts/pr-state.test.sh | 73 ++++++- 48 files changed, 1410 insertions(+), 317 deletions(-) create mode 100644 .agents/skills/e2e/references/fixture-state.md create mode 100644 .agents/skills/e2e/references/ui-state-and-cleanup.md create mode 100644 .agents/skills/harness-improvement/references/validation.md create mode 100644 apps/web/components/review/AGENTS.md create mode 100644 apps/web/components/task/chat/AGENTS.md create mode 100755 scripts/playwright-blob-audit create mode 100755 scripts/playwright-blob-audit.test.py diff --git a/.agents/agents/pr-poller.md b/.agents/agents/pr-poller.md index 3bccd0ac83..0ca5984720 100644 --- a/.agents/agents/pr-poller.md +++ b/.agents/agents/pr-poller.md @@ -4,7 +4,7 @@ description: Read-only, low-cost PR monitor. Use only after the user explicitly tools: Bash model: haiku effort: low -maxTurns: 12 +maxTurns: 44 --- # PR Poller @@ -16,9 +16,15 @@ Do not read source code, edit files, push, post or resolve GitHub comments, trigger workflows, fetch full CI logs, or spawn subagents. Use `scripts/pr-state --summary ` and `scripts/pr-resolve list ` as the -primary sources. Poll at a 30-second cadence for at most 20 minutes. Return -early for a failed check, merge conflict, actionable review feedback, or a -terminal clean state. At the time limit, return the pending checks and reviews. +primary sources. Poll at a 30-second cadence for at most 20 minutes. In the +default mode, return early for a failed check, merge conflict, actionable +review feedback, or a terminal clean state. If the caller says "wait N +minutes" or "then fix up", use strict-deadline mode: calculate and include the +absolute deadline in the polling prompt, accumulate findings, and do not return +early for findings, pending checks, or a clean snapshot. Stop early only if the +PR is merged/closed or access is revoked. At the deadline, return the latest +named pending checks and named actionable review findings, not only aggregate +CI and review states. Before the first GitHub request, obtain any runtime network approval required by the platform. A denied, cancelled, or interrupted approval is terminal: do not @@ -29,6 +35,7 @@ Return only: ```text PR at CI: -Reviews: +Reviews: ; findings: +Pending checks: Next action: ``` diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index e4780e59bd..0e9862b520 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -36,14 +36,25 @@ the current PR. Record the base and head SHA for each review round. A new contributor push starts a new round: reassess prior findings and the verdict against the new head, and verify checks or workflow results for that head rather than relying on a PR -number or author summary. +number or author summary. From `scripts/pr-state --summary`, also record +`pr.base_ref_name`, `pr.base_head_oid`, `pr.merge_base_oid`, and +`pr.base_advanced_since_head` when available. -When the base branch advanced after the reviewed head's CI run, validate the -actual merge result before declaring the PR ready. Record the latest base and -immutable head SHAs, create a temporary worktree from that base, merge the head -with `git merge --no-commit --no-ff `, and run focused verification in -the merged tree before removing the worktree. GitHub's `mergeable: MERGEABLE` -status proves conflict compatibility, not that the merged result was tested. +For a follow-up round, inspect `..` first +to isolate the author's response, then re-evaluate `...` for +complete PR coverage. Record both immutable heads. + +When `pr.base_advanced_since_head` is `true`, validate the actual merge result +before declaring the PR ready. Record the latest base and immutable head SHAs, +create a temporary worktree from that base, merge the head with `git merge +--no-commit --no-ff `, and run focused verification in the merged tree +before removing the worktree. GitHub's `mergeable: MERGEABLE` status proves +conflict compatibility, not that the merged result was tested. If an older +`pr-state` helper lacks the base fields, resolve the current base +head only as a fallback with `gh api +repos/{owner}/{repo}/git/ref/heads/{base}` and derive/record the merge base +before making the same decision; do not try the unsupported +`gh pr view --json baseRefOid` field. For an existing GitHub PR, inspect both `scripts/pr-state --summary ` and `scripts/pr-resolve list ` before treating review feedback as clean. Read @@ -176,6 +187,7 @@ Check every changed file for the following layers. Skip layers that don't apply **Testing (blocker if missing):** - Backend (Go): new or changed functions/methods must have corresponding `*_test.go` tests - Frontend: new utilities, hooks, API clients, and store slices must have focused tests. Pure React markup may skip a unit test, but behavior-bearing components (conditional status, accessibility, store-derived state, or responsive/mobile variants) need focused `*.test.tsx` coverage and/or E2E. Route responsive user-facing changes through `/mobile-parity`. +- Async UI lifecycle: loading/busy state must clear through `finally` or equivalent terminal cleanup for success, error, cancellation, and early/no-op returns; require focused tests for those terminal paths. - Exceptions: config files, generated code, and pure React component markup - Missing tests for new or changed logic is a **blocker** — suggest what tests to add and recommend `/tdd` @@ -195,6 +207,13 @@ review; preserve all pre-existing user changes. Report findings with a concrete suggested fix. Do not edit the checkout during a review-only request; otherwise remediate in the same primary conversation. +Before drafting or posting a copyable PR comment, map each proposed point against +exact-current-head review bodies, top-level discussion comments, and all +unresolved or hidden threads. If a point is already raised, omit it from the +author-facing comment but retain it in the private review summary; repeat it +only when the user explicitly asks for reinforcement. Re-fetch immediately +before posting and start a new review round if `headRefOid` changed. + ### 5. Output Use this format: diff --git a/.agents/skills/commit/SKILL.md b/.agents/skills/commit/SKILL.md index 0445069a1a..9128e897af 100644 --- a/.agents/skills/commit/SKILL.md +++ b/.agents/skills/commit/SKILL.md @@ -94,6 +94,15 @@ explicitly requests task tracking. ``` Retry the normal commit after the install; never bypass the hook. + - After merging or rebasing the base branch, if `apps/package.json` or + `apps/pnpm-lock.yaml` changed and a hook reports + `ERR_MODULE_NOT_FOUND` even though `apps/node_modules/.bin/commitlint` + exists, refresh the workspace dependencies from `apps/`: + ```bash + cd apps && pnpm install --frozen-lockfile + ``` + Retry the normal commit without bypassing hooks. + 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. **Capture the parent SHA and preserve hook evidence:** @@ -109,19 +118,28 @@ qualifies as a successful hook receipt. - **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. 5. **Commit:** Write a commit message following the format above. If changes span multiple concerns, consider separate commits. + When `MERGE_HEAD` exists or a merge commit is being completed, use + `git commit --no-edit` so a non-interactive runner does not open an editor; + normal hooks still run. Confirm the merge commit exists and `MERGE_HEAD` is + gone before reporting success. If a formatter changes files and prevents the commit, review and re-stage those files, then create a new commit attempt; do not use `--amend`. + When editing harness files such as `AGENTS.md`, `CLAUDE.md`, or skills, run + the shared validation in + `.agents/skills/harness-improvement/references/validation.md` before + committing. + If a JSX layout-only edit touches an element containing an existing + hardcoded user-facing literal, `i18n-new-code` may classify that literal as + changed copy and fail. Localize it and add matching `en`/`pseudo` catalog + entries before retrying; verify with `cd apps/web && pnpm run i18n:check` and + the normal hook receipt. Capture the normal hook stream in a temporary log while committing and use that log to record each hook ID and result. Do not infer hook results from a condensed launcher summary. For example: ```bash COMMIT_LOG="$(mktemp "${TMPDIR:-/tmp}/kandev-commit.XXXXXX.log")" set -o pipefail - if command -v rtk >/dev/null 2>&1; then - rtk proxy git commit -m "type(scope): description" - else - git commit -m "type(scope): description" - fi 2>&1 | tee "$COMMIT_LOG" >/dev/null + git commit -m "type(scope): description" 2>&1 | tee "$COMMIT_LOG" >/dev/null ``` Read the log to extract the hook receipt, rather than printing the full stream again. Remove the exact temporary file after copying the receipt into @@ -129,6 +147,9 @@ qualifies as a successful hook receipt. ```bash unlink "$COMMIT_LOG" ``` + If the commit exits nonzero, preserve and print the full or bounded log + before cleanup so hook diagnostics are not lost. Remove the temporary log + only after copying a successful hook receipt or recording the failed output. If a hook fails only because another worktree is already running golangci-lint (for example, `parallel golangci-lint is running`), wait for @@ -136,6 +157,11 @@ qualifies as a successful hook receipt. code for this transient lock; verify the retry has a normal hook receipt and a clean worktree. + If a hook fails with `ENOSPC`, load `/verify`'s Disk-constrained runners + guidance: inspect `df`, preserve managed caches, relocate only the affected + cache to an explicit persistent agent-owned path, and never bypass hooks or + blindly delete shared caches. + 6. **Return a hook receipt:** After a successful commit, report: ```text parent_sha: diff --git a/.agents/skills/context-engineering/SKILL.md b/.agents/skills/context-engineering/SKILL.md index f119a26694..d3c400e124 100644 --- a/.agents/skills/context-engineering/SKILL.md +++ b/.agents/skills/context-engineering/SKILL.md @@ -17,6 +17,11 @@ Feed the agent the right information at the right time. Too little context cause ## Kandev Loading Checklist +Before running shell commands, resolve every `@path` import in the root or +scoped `AGENTS.md`/`CLAUDE.md` files and read the referenced instructions. +If an imported file is unavailable, note the missing guidance and continue with +the best available local instructions. + Before changing code: - Read the scoped `AGENTS.md` for the subtree you will touch, e.g. `apps/backend/AGENTS.md`, `apps/web/AGENTS.md`, or integration-specific guidance. - Use `rg` to find existing patterns before inventing one. diff --git a/.agents/skills/debug/SKILL.md b/.agents/skills/debug/SKILL.md index 8ef3e3c83c..d3dc037881 100644 --- a/.agents/skills/debug/SKILL.md +++ b/.agents/skills/debug/SKILL.md @@ -1,7 +1,7 @@ --- name: debug description: Diagnose Kandev bugs, running-instance issues, UI/browser failures, and runtime behavior. Use when the user reports unexpected behavior, asks to investigate, asks to add logs/instrumentation, or when a fix needs root-cause evidence before implementing. Triage first, gather evidence safely, then hand off to /fix for code changes. -allowed-tools: Bash(curl:*) Bash(jq:*) Bash(mktemp:*) Bash(unzip:*) Bash(npx:*) Bash(scripts/kandev-instances:*) Bash(scripts/kandev-logs:*) Bash(scripts/dev-isolated:*) Bash(scripts/kandev-kill:*) Bash(go:*) Bash(rg:*) Bash(grep:*) +allowed-tools: Bash(curl:*) Bash(jq:*) Bash(mktemp:*) Bash(unzip:*) Bash(pnpm:*) Bash(scripts/kandev-instances:*) Bash(scripts/kandev-logs:*) Bash(scripts/dev-isolated:*) Bash(scripts/kandev-kill:*) Bash(go:*) Bash(rg:*) Bash(grep:*) --- # Debug @@ -48,7 +48,9 @@ Start with the cheapest faithful reproduction: 1. Backend logic: write a throwaway focused Go repro test against the real service path. If it reproduces, convert it via `/fix`. 2. Live instance in a task session: call `get_diagnostic_bundle_kandev` with `backend`, `frontend`, or `all`; inspect `manifest.json` before assuming a source is complete. 3. Host-side instance: use `scripts/kandev-logs --source backend|frontend|all`; do not relaunch. Set `KANDEV_API_TOKEN` only when authentication is enabled. -4. UI/browser: launch `scripts/dev-isolated --web`, drive `npx playwright-cli`, and correlate console/network state with a fresh all-source bundle. +4. UI/browser: launch `scripts/dev-isolated --web`, drive + `pnpm --dir apps exec playwright-cli`, and correlate console/network state + with a fresh all-source bundle. 5. Unknown: trace from the symptom backward through code and add temporary instrumentation only where it will split the search space. ### File-first log triage @@ -81,13 +83,26 @@ values, and secrets. Always inspect `manifest.json` and its warnings before assuming a source is complete, and grep task/session IDs inside the extracted ZIP before broadening to route text or timestamps. +**Cancellation intent is separate from event serialization:** A generic +per-session event-serialization mutex only orders work; it is not evidence +that cancellation was requested. Model cancellation intent with separate state +or a refcount, and mark it only around real cancellation operations. During +concurrency debugging, inspect that state independently before attributing a +queued or dropped event to cancellation. + +**Provider diagnostics:** Raw agent stderr may contain URLs, IDs, subscription +details, or other sensitive runtime data. Inspect it only in memory, sanitize it +before writing to generic logs, ring buffers, process-exit errors, persistence, +or the UI, and ensure bounded diagnostic consumers cannot block subprocess +stderr draining. + ## Reference Files Load only the reference needed for the selected path: - `references/backend-repro.md` - targeted Go repro tests and backend-first debugging. - `references/instance.md` - instance discovery, isolated launch, logs/export, and teardown. -- `references/browser.md` - `npx playwright-cli` browser debugging against isolated instances. +- `references/browser.md` - workspace-pinned `playwright-cli` browser debugging against isolated instances. - `references/instrumentation.md` - temporary vs persistent frontend/backend logging rules. ## When To Use Instrumentation diff --git a/.agents/skills/debug/references/backend-repro.md b/.agents/skills/debug/references/backend-repro.md index b29a585416..4c91d9a014 100644 --- a/.agents/skills/debug/references/backend-repro.md +++ b/.agents/skills/debug/references/backend-repro.md @@ -49,3 +49,14 @@ Before fixing, state: - exact condition that triggers it; - smallest input or state needed to reproduce; - code path from input to failure. + +## Workflow Transition Triage + +For a partial-success report where a workflow step mutates state but sends no +prompt or message, inspect the serialized step and distinguish payload fields +from trigger actions: `prompt` supplies content, while `auto_start_agent` +dispatches it. Trace session identity separately; fixed-profile switches may +retain legacy implicit-launch behavior, while in-place mutations preserve the +session and do not satisfy a `sessionSwitched` gate. Verify the distinction with +paired focused service tests—with and without the dispatch action—that assert +both the state mutation and the resulting message/prompt output. diff --git a/.agents/skills/debug/references/browser.md b/.agents/skills/debug/references/browser.md index d5ac861f28..8d2d60c9e0 100644 --- a/.agents/skills/debug/references/browser.md +++ b/.agents/skills/debug/references/browser.md @@ -6,37 +6,56 @@ Use this only for UI, layout, focus, WS-driven, console, or click-flow bugs that - Use an isolated instance from `references/instance.md`. - Never drive a browser against the user's live instance. -- Use `npx playwright-cli`; there is no guaranteed bare `playwright-cli` binary. +- Use the exact `@playwright/cli` dependency from the workspace via + `pnpm --dir apps exec playwright-cli`; do not fetch a CLI dynamically. - Reuse an existing browser session when possible. ## Commands ```bash -npx --no-install playwright-cli --version -npx playwright-cli --help -npx playwright-cli list +pnpm --dir apps exec playwright-cli --version +pnpm --dir apps exec playwright-cli --help +pnpm --dir apps exec playwright-cli list ``` Open only if no suitable session exists: ```bash -npx playwright-cli open http://localhost: +pnpm --dir apps exec playwright-cli open http://localhost: ``` Common debugging: ```bash -npx playwright-cli snapshot -npx playwright-cli snapshot "#main" -npx playwright-cli snapshot --depth=4 -npx playwright-cli console -npx playwright-cli console error -npx playwright-cli network -npx playwright-cli goto http://localhost:/some/path +pnpm --dir apps exec playwright-cli snapshot +pnpm --dir apps exec playwright-cli snapshot "#main" +pnpm --dir apps exec playwright-cli snapshot --depth=4 +pnpm --dir apps exec playwright-cli console +pnpm --dir apps exec playwright-cli console error +pnpm --dir apps exec playwright-cli network +pnpm --dir apps exec playwright-cli goto http://localhost:/some/path ``` -Correlate browser console and network activity with a fresh standard bundle or -the smallest custom source set that answers the question: +For browser/API failures that may be caused by origin capabilities, capture +these values in the isolated browser session before attributing the symptom to +WebSocket or network behavior: + +```bash +pnpm --dir apps exec playwright-cli eval 'JSON.stringify({ + origin: location.origin, + secure_context: window.isSecureContext, + random_uuid: typeof crypto === "undefined" ? "undefined" : typeof crypto.randomUUID, + subtle: typeof crypto === "undefined" ? "undefined" : typeof crypto.subtle, + clipboard_write: typeof navigator === "undefined" || !navigator.clipboard + ? "undefined" + : typeof navigator.clipboard.writeText, +})' +``` + +Verify the failing code's capability probe and fallback in that same session; +do not infer support from a different origin or from a test-only stub. + +Correlate browser console, frontend log buffer, network activity, and backend logs from: ```bash scripts/kandev-logs --source all @@ -61,5 +80,5 @@ error text. Close the browser when done unless the user asks to keep it open: ```bash -npx playwright-cli close +pnpm --dir apps exec playwright-cli close ``` diff --git a/.agents/skills/e2e/SKILL.md b/.agents/skills/e2e/SKILL.md index 932cad31c0..2e61afd586 100644 --- a/.agents/skills/e2e/SKILL.md +++ b/.agents/skills/e2e/SKILL.md @@ -19,6 +19,13 @@ Write E2E tests using TDD (Red-Green-Refactor). Always run the tests you create - **`/pr-fixup`** — Use after the PR opens only for CI or reviewer findings. - **`/playwright-cli`** — Interactive browser automation. Use to validate features against the dev server before writing tests, and to debug failing tests with `--debug=cli`. +## References + +Load `references/fixture-state.md` for capability-readiness and remembered +workflow fixture rules. +Load `references/ui-state-and-cleanup.md` for lifecycle, WebSocket, terminal, +Dockview, and sidebar/context-menu rules. + ## Location `apps/web/e2e/` @@ -92,6 +99,8 @@ pnpm e2e:run --project containers tests/docker/.spec.ts Use `mobile-chrome` only for `mobile-*.spec.ts` files. Confirm Playwright discovers the intended test count before treating a focused command as evidence. +`e2e:run` accepts one `--project`; repeating it selects only the last value, so run desktop and mobile separately when both are required and confirm discovery for each. + 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". `--no-build` reuses every production E2E artifact, not only Vite assets and the @@ -127,7 +136,7 @@ Start by matching CI as closely as possible, then add pressure deliberately: -e NODE_OPTIONS=--dns-result-order=ipv4first \ -e PLAYWRIGHT_BROWSERS_PATH=/ms-playwright \ ghcr.io/kdlbs/kandev-ci:runtime-latest \ - bash -lc 'git config --global --add safe.directory /work 2>/dev/null; npx playwright test --config e2e/playwright.config.ts --project=chromium --project=mobile-chrome --shard=10/10 --reporter=list' + bash -lc 'git config --global --add safe.directory /work 2>/dev/null; npx playwright test --config e2e/playwright.config.ts --project=chromium --project=mobile-chrome --shard=/14 --reporter=list' ``` 2. If the exact shard passes, constrain container resources and repeat the failing spec/test. GitHub-hosted runners can expose timing bugs that a roomy @@ -141,21 +150,19 @@ Start by matching CI as closely as possible, then add pressure deliberately: ghcr.io/kdlbs/kandev-ci:runtime-latest \ bash -lc 'git config --global --add safe.directory /work 2>/dev/null; npx playwright test --config e2e/playwright.config.ts --project=mobile-chrome e2e/tests/terminal/mobile-terminal-keybar.spec.ts --grep "user presses an OS-keyboard letter while no modifier is active" --repeat-each=30 --reporter=list' ``` -3. Preserve nearby test ordering when a single-test repeat stays green. Run the - full spec file or full shard with the same resource limits before declaring - a flake non-reproducible. +3. Preserve nearby test ordering when a single-test repeat stays green; run the full spec or shard with the same resource limits before declaring a flake non-reproducible. The config uses `failOnFlakyTests: !CI`: local runs fail on a flaky retry, while CI temporarily tolerates one. Either result is a failure signal for agents: -reproduce it locally with per-spec overrides disabled and `--retries=0`; never +reproduce it with `--retries=0` only after disabling any `test.describe.configure({ retries: 1 })` override; never rerun until it happens to pass. If isolated repeats stay green but the shard fails, binary-search preceding specs in one worker. The fix is complete only when the smallest reproducing sequence passes without retries. -Record the exact command, resource limits, repeat number, and failure artifact -path. Always inspect `error-context.md`; mobile/terminal flakes often show -state that the stack trace alone hides, such as duplicate active terminals or a -terminal stuck on "Starting terminal...". +Record the exact command, resource limits, repeat number, and failure artifact path. For a failed shard, inspect every `error-context.md` in its downloaded +`test-results-` artifact and compare shared page-object waits with `main` +before changing product code; the context can expose duplicate active terminals +or a terminal stuck on "Starting terminal...". When a PR E2E shard fails, investigate every spec, including those outside the diff; never dismiss, rerun, or merely record a failure as unrelated. Reproduce @@ -168,15 +175,13 @@ defect with retries disabled, or evidence a concrete external blocker. make build-web # ~30s, required after every frontend change ``` -Without this, tests run against stale code and failures are misleading. `make build-backend` is also required after Go changes. `make test-e2e` and `pnpm e2e:run` handle both automatically. +Without this, tests can exercise stale code: after backend changes run `make -C apps/backend build` before reproducing Playwright failures, and compare the binary timestamp/hash if a fixed test still fails. `make test-e2e` and `pnpm e2e:run` handle both builds. ## Writing a test -1. Read `helpers/api-client.ts` and `pages/` to discover available seed methods and page objects +1. Read `helpers/api-client.ts` and `pages/` to discover available seed methods and page objects; use `data-testid` attributes for selectors — add them to components as needed 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 +3. Use page objects for common interactions; create new ones for new pages. For GitHub features, use `apiClient.mockGitHub*()` methods to seed mock data ### Input-modality behavior @@ -245,11 +250,15 @@ test.describe("my feature", () => { ## Dev-first workflow -Before writing an E2E test, validate the feature works interactively using `playwright-cli` against a dev server. This gives a fast feedback loop — code changes are picked up by hot reload in ~1-2 seconds, no production rebuild needed. Once confirmed working, translate the interactions into a proper E2E test. +Before writing an E2E test, validate the feature works interactively using +`pnpm --dir apps exec playwright-cli` against a dev server. This gives a fast +feedback loop — code changes are picked up by hot reload in ~1-2 seconds, no +production rebuild needed. Once confirmed working, translate the interactions +into a proper E2E test. ### Start the dev environment -Multiple agents may run in parallel, so use random ports to avoid collisions. Fixture ports auto-offset from 18080 (backend) and 13000 (frontend) using `E2E_PORT_OFFSET` (derived from `PID % 30` by default) — stay outside those ranges. Parallel E2E test runs are safe by default. +Multiple agents may run in parallel, so use random ports for dev servers. Managed runner shards are isolated, but separate raw Playwright processes are not automatically safe: fixture ports are backend `18080 + E2E_PORT_OFFSET + workerIndex` and agentctl `30001 + E2E_PORT_OFFSET*1000 + workerIndex*200`; `--repeat-each` advances `workerIndex`, so nearby fixed offsets can overlap later. ```bash OFFSET=$((RANDOM % 100)) @@ -276,18 +285,18 @@ pnpm --filter @kandev/web dev --port $FRONTEND_PORT & ### Validate with playwright-cli ```bash -playwright-cli open http://localhost:$FRONTEND_PORT -playwright-cli snapshot # see page structure and element refs -playwright-cli click e5 # interact using refs from snapshot -playwright-cli fill e3 "test input" -playwright-cli snapshot # verify result +pnpm --dir apps exec playwright-cli open http://localhost:$FRONTEND_PORT +pnpm --dir apps exec playwright-cli snapshot # see page structure and element refs +pnpm --dir apps exec playwright-cli click e5 # interact using refs from snapshot +pnpm --dir apps exec playwright-cli fill e3 "test input" +pnpm --dir apps exec playwright-cli snapshot # verify result ``` ### Fast iteration cycle 1. Make a code change in `apps/web/` 2. HMR picks it up in ~1-2 seconds -3. `playwright-cli snapshot` or `playwright-cli screenshot` to verify +3. `pnpm --dir apps exec playwright-cli snapshot` or `pnpm --dir apps exec playwright-cli screenshot` to verify 4. Repeat until the flow works correctly ### Translate to E2E test @@ -308,14 +317,14 @@ After confirming the feature works, capture screenshots or a video as proof for ```bash # Screenshots of key states -playwright-cli screenshot --filename=apps/web/.pr-assets/feature-before.png +pnpm --dir apps exec playwright-cli screenshot --filename=apps/web/.pr-assets/feature-before.png # ... interact to show the feature ... -playwright-cli screenshot --filename=apps/web/.pr-assets/feature-after.png +pnpm --dir apps exec playwright-cli screenshot --filename=apps/web/.pr-assets/feature-after.png # Or record a video walkthrough -playwright-cli video-start apps/web/.pr-assets/feature-demo.webm +pnpm --dir apps exec playwright-cli video-start apps/web/.pr-assets/feature-demo.webm # ... perform the user flow ... -playwright-cli video-stop +pnpm --dir apps exec playwright-cli video-stop ``` Create `apps/web/.pr-assets/manifest.json` so the `/pr` skill picks them up: @@ -333,7 +342,7 @@ Create `apps/web/.pr-assets/manifest.json` so the `/pr` skill picks them up: Always verify against the production build before finishing — dev mode can hide boot-payload, asset-serving, or hydration issues: ```bash -playwright-cli close +pnpm --dir apps exec playwright-cli close # Kill dev server and backend make build-web cd apps && pnpm --filter @kandev/web e2e:raw -- tests/path/to/test.spec.ts @@ -347,11 +356,11 @@ Tests are grouped by feature area in subdirectories under `tests/`. When creatin - **Merge related tests into the same file.** Tests covering the same feature (e.g., git commit body and pre-hooks) belong in one file with separate `test.describe` blocks. Don't create a new file for each narrow scenario. - **Import paths from subdirectories** use `../../` (e.g., `from "../../fixtures/test-base"`). - **Standalone root files** are allowed for truly cross-cutting tests that don't fit any group. -- **Extract large shared helpers.** For large specs with shared setup or polling helpers, extract helpers into a sibling `*-helpers.ts` file once the spec approaches the repo file-size limit. Keep spec files focused on test scenarios; put reusable page polling, seeding, and Dockview cleanup helpers in the helper module. +- **Extract shared helpers.** Extract helpers into a sibling `*-helpers.ts` file whenever they are used by multiple spec files, even when small; keep only scenario-specific setup in specs. Reusable page polling, seeding, and Dockview cleanup belong in the helper module. ## 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. +- **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; use `toContainText` or a dedicated locator when labels include metadata such as file sizes. - **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. - **Restore patched persisted settings.** When a test PATCHes user settings, capture the baseline and restore it in `test.afterEach`. The backend is worker-scoped, and `e2eReset` does not reset every persisted setting, including `system_metrics_display`; leaking one can affect later tests in the same worker. Fixtures are lazy: acquire `testPage` before setting a non-default persisted value in `beforeEach`, otherwise page initialization can reapply the default and silently undo setup. Verify with the focused test that depends on that setting. - **Restore patched shared persisted state.** The worker-scoped backend and @@ -365,6 +374,8 @@ Tests are grouped by feature area in subdirectories under `tests/`. When creatin `page.evaluate` callbacks execute in the browser, so they cannot close over Node/test variables. Pass expected values as the argument instead, for example `locator.evaluate((el, expected) => Math.abs(el.scrollTop - expected), baseline)`. +- **Reset pointer state before hover assertions.** A prior click can leave the + hover target active; move to a neutral page location before asserting hover UI. - **Measure asynchronous layout from a settled baseline.** When the initiating UI action changes layout before its delayed result arrives, delay the mock response and capture the baseline after that synchronous layout settles. Then @@ -378,10 +389,6 @@ Tests are grouped by feature area in subdirectories under `tests/`. When creatin strategy selected at runtime. - **Nested Escape controls.** If an inner panel inside a Radix Dialog handles Escape, intercept the key in capture phase and call both `preventDefault()` and `stopPropagation()` before dismissing the inner panel. A bubble-phase window handler runs after Radix can dismiss the outer dialog. Add a regression that asserts the inner panel collapses while the outer dialog remains open. - **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. -- **Scope Dockview preview polling to visible panels.** Hidden or stale Dockview panels can remain mounted and produce false positives if helpers scan all matching custom elements globally. For `diffs-container`, filter candidate elements by visible layout box and computed visibility before reading shadow DOM text. -- **Poll before Dockview cleanup.** If an E2E helper uses `window.__dockviewApi__`, wait or poll for the API to be attached before acting. A one-shot `if (!api) return` cleanup can silently skip cleanup during page initialization and leak prior preview panels into later assertions. ## Debugging failures @@ -396,10 +403,10 @@ When a test fails: ```bash cd apps && PLAYWRIGHT_HTML_OPEN=never pnpm --filter @kandev/web e2e:raw -- tests/path.spec.ts --debug=cli & # Wait for "Debugging Instructions" with session name - playwright-cli attach tw- - playwright-cli snapshot # inspect page state at failure point - playwright-cli console # check for JS errors - playwright-cli network # check API responses + pnpm --dir apps exec playwright-cli attach tw- + pnpm --dir apps exec playwright-cli snapshot # inspect page state at failure point + pnpm --dir apps exec playwright-cli console # check for JS errors + pnpm --dir apps exec playwright-cli network # check API responses ``` ### Classify and fix @@ -414,7 +421,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 --frozen-lockfile` -- **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 +- **Port conflicts** — run raw repeat/stress invocations sequentially, or prove both computed port ranges are disjoint and check listeners. A totally white screenshot plus `ERR_CONNECTION_REFUSED` is a port/lifecycle signature to rule out before changing waits; never fix it with longer locator timeouts. - **Responsive layout stays stale after `page.setViewportSize()`** — record `window.innerWidth`, the affected element and parent `clientWidth`, and any layout-library width before changing waits. Headless Chromium reliably @@ -422,34 +429,31 @@ When a test fails: application-only `window.resize` listener may not be observed in the test. Prefer synchronizing with the layout container/observer and assert the intended result after both viewport and container-only changes. -- **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. +- **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()` before opening transient dialogs/drawers/popovers; it may reload and re-derive state from the Go boot payload. If it must run later, reopen the transient UI first. For WS/session hydration races, retain a bounded reload-and-retry fallback in the page object: keep the fast path immediate and the final check failing when genuinely stuck; remove it only with an equivalent deterministic readiness guarantee and focused regression. - **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) ### Debugging CI shard failures -CI splits tests across 10 shards. To reproduce a specific shard locally: +CI splits host tests across 14 shards (plus 6 container shards); reproduce a specific shard locally: ```bash # List which tests are in a shard -npx playwright test --config e2e/playwright.config.ts --shard=2/10 --list +npx playwright test --config e2e/playwright.config.ts --shard=2/14 --list # Run that shard locally (requires production build) make build-backend build-web -cd apps/web && npx playwright test --config e2e/playwright.config.ts --shard=2/10 +cd apps/web && npx playwright test --config e2e/playwright.config.ts --shard=2/14 ``` -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 unzip report-*.zip -d report-shard && cat report-shard/*.jsonl ``` -When a CI shard fails, distinguish its two useful artifacts. Download and unzip -the blob `report-*.zip` to map test IDs, titles, locations, final status, and -timings from its JSONL events. Download `test-results-` for the exact -failed assertion, `error-context.md`, screenshot, and trace: +When a CI shard fails, distinguish its useful artifacts: download/unzip +`report-*.zip` to map test IDs and timings; `test-results-` may be ready +before the workflow completes even when logs refuse, so try it for the exact assertion, `error-context.md`, screenshot, and trace: ```bash gh run download --name test-results- --dir @@ -479,13 +483,8 @@ A test that flakes under parallel/sharded load is one of two things — decide w ## Selector guidelines -- **Prefer `data-testid` selectors** over text-based locators. Text content can change when UI is updated (e.g., hiding a badge), breaking tests that match by text. Use `getByTestId()` or `locator("[data-testid='...']")` for stable targeting. -- **Scope Radix tooltip locators to the visible portal.** Radix can render an - accessibility copy as well as the visible tooltip, so global test-id, text, - or role locators may match multiple elements in strict mode. Start from a - open portal `[data-slot="tooltip-content"][data-state="open"]`, then locate its visible - descendant. Do not assume the trigger's `aria-describedby` target is the - visual portal. Use bounding-box assertions when relative placement matters. +- **Prefer `data-testid` selectors** over text-based locators. Text content can change when UI is updated (e.g., hiding a badge), breaking tests that match by text. Use `getByTestId()` or `locator("[data-testid='...']")` for stable targeting. When translated labels intentionally identify multiple routes, scope by stable `href` or a dedicated test ID rather than role/name alone. +- **Scope Radix and responsive locators to the active instance.** Tooltips may use `instant-open`, `delayed-open`, or `open`; use `[data-slot="tooltip-content"]:not([data-state="closed"])`, then scope to the visible portal/popover/container and active ancestor. Hidden mounts can make global locators match the wrong instance; do not use `.first()` to hide duplicates. - **Use page object methods** like `clickSessionChatTab()` (stable `data-testid`) instead of `sessionTabByText("1")` (fragile text match) for session tabs. - **Dropdown menus can detach** from the DOM when React re-renders the parent (e.g., WS events updating the sidebar). The `openSidebarMenuAndClick()` helper in `session-page.ts` retries the full open-click sequence on detachment — use this pattern for similar interactions. diff --git a/.agents/skills/e2e/references/fixture-state.md b/.agents/skills/e2e/references/fixture-state.md new file mode 100644 index 0000000000..565fb90146 --- /dev/null +++ b/.agents/skills/e2e/references/fixture-state.md @@ -0,0 +1,19 @@ +# E2E Fixture State + +Load this reference for tests that depend on asynchronous capability discovery or +remembered workflow selection. + +## Asynchronous capability readiness + +For tests using agent models, modes, commands, or options, treat +`not_configured` and `probing` snapshots as provisional. Select by stable name +or test ID and bounded-poll for the semantic capability before interacting. +Avoid arbitrary sleeps and `force: true` clicks. + +## Remembered workflow selection + +After seeding tasks in multiple workflows, set +`task_create_last_used.workflow_ids_by_workspace[workspaceId]` to the dialog's +workflow, or clear it to test filter fallback. Assert the selector before +downstream checks: the remembered workflow outranks `workflow_filter_id`. +Verify the focused test with `--retries=0`. diff --git a/.agents/skills/e2e/references/ui-state-and-cleanup.md b/.agents/skills/e2e/references/ui-state-and-cleanup.md new file mode 100644 index 0000000000..27e248b3f2 --- /dev/null +++ b/.agents/skills/e2e/references/ui-state-and-cleanup.md @@ -0,0 +1,41 @@ +# E2E UI State and Cleanup + +Load this reference for session/WS lifecycle assertions, terminal and Dockview +helpers, or sidebar/context-menu interactions. + +## Workflow and 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. Agent output is not lifecycle readiness: if a +follow-up must use the synchronous prompt path rather than the queued path, or +a transcript is mutated after an auto-started boot turn, poll the exact session +state such as `WAITING_FOR_INPUT` before acting. The mock agent can persist text +before the lifecycle transition completes. Verify the affected full spec and +original CI shard with `--retries=0`. + +## WebSocket capture + +Reset capture arrays immediately before the stimulus, but finish all +subscription and lifecycle assertions before clearing them. Reselecting an +already-active session may emit no new subscribe frame; consume the original +frame or trigger a distinct state transition. Verify ordering in the owning +Playwright project with `--retries=0`. + +## Terminal and Dockview helpers + +- Scope terminal/mobile helpers to the active `data-testid="terminal-panel"`; + multiple terminal panels can be mounted at once. +- Scope Dockview preview polling to visible panels. Hidden or stale panels can + remain mounted and produce false positives when helpers scan custom elements. +- If a helper uses `window.__dockviewApi__`, poll until the API is attached; + silently skipping cleanup during page initialization leaks panels into later + assertions. + +## Sidebar and context-menu editing + +For actions targeting a non-active item, create or select a separate navigation +task, assert its initial URL/task ID, act on the target row, then assert that the +URL/task ID is unchanged and persistence survives reload. Cover shared +desktop/mobile/tablet menus. diff --git a/.agents/skills/fix/SKILL.md b/.agents/skills/fix/SKILL.md index 2eee70cbea..dd9438e5b6 100644 --- a/.agents/skills/fix/SKILL.md +++ b/.agents/skills/fix/SKILL.md @@ -91,6 +91,11 @@ default: 4. Run the task's exact targeted unit, integration, or E2E command. 5. Mark the task `done` and update `plan.md` status in the primary session. +For persisted defaults or coupled settings, audit every write/reset path for +explicit empty or null values. Add the smallest invalid-pair regression, +including fixture coverage, and verify normalized read-back plus existing +validation. + When the user explicitly authorizes subagents, follow `/planner-orchestration`: native delegation only, current user-selected model, `fork_turns: "none"`, compact task-file handoffs, no recursive spawning, and diff --git a/.agents/skills/harness-improvement/SKILL.md b/.agents/skills/harness-improvement/SKILL.md index da3cb256df..fdabe9d631 100644 --- a/.agents/skills/harness-improvement/SKILL.md +++ b/.agents/skills/harness-improvement/SKILL.md @@ -61,6 +61,8 @@ mirror unless the user explicitly requests a policy reversal. git diff --check -- rg -n "old-skill|old-agent|stale-command" .agents AGENTS.md CLAUDE.md ``` + - Load `references/validation.md` for the shared harness test, lint, + whitespace, line-budget, and pre-commit commands. - For executable script changes, run syntax checks and a focused dry run or mocked command when possible. 6. **Report** diff --git a/.agents/skills/harness-improvement/references/validation.md b/.agents/skills/harness-improvement/references/validation.md new file mode 100644 index 0000000000..b4599418b0 --- /dev/null +++ b/.agents/skills/harness-improvement/references/validation.md @@ -0,0 +1,21 @@ +# Harness Validation + +Use this reference for the shared checks after changing skills, agents, +references, or always-on instruction files. + +```bash +python3 scripts/lint-harness-files.test.py +python3 .github/scripts/lint-harness-files.py --all +git diff --check -- +wc -l +``` + +Before committing, also run the targeted pre-commit hook: + +```bash +pre-commit run harness-lint --files +``` + +Keep every `AGENTS.md` and `CLAUDE.md` file at 300 lines or fewer. Move detail +to the nearest scoped file or a referenced document when a parent reaches the +limit. diff --git a/.agents/skills/plan/SKILL.md b/.agents/skills/plan/SKILL.md index 59cf724d71..37ea5735b4 100644 --- a/.agents/skills/plan/SKILL.md +++ b/.agents/skills/plan/SKILL.md @@ -197,7 +197,7 @@ spec: "../../specs//spec.md" Each task should be small enough for one focused implementation pass: - **Acceptance:** 1-3 concrete conditions. -- **Verification:** exact command(s), e.g. `cd apps/backend && go test -run TestName ./internal/path/...` or `cd apps && pnpm --filter @kandev/web test -- path/to/file.test.ts`. Frontend/E2E tasks must include the fresh-worktree bootstrap (`cd apps && pnpm install --frozen-lockfile`) when dependencies may be absent; direct web typechecking uses `cd apps/web && pnpm run typecheck`, while other workspace package commands use the documented `pnpm --filter` form. Backend commands should use the applicable repository `make` target when one exists. +- **Verification:** exact command(s), e.g. `cd apps/backend && go test -run TestName ./internal/path/...` or `cd apps && pnpm --filter @kandev/web test -- path/to/file.test.ts`. Frontend/E2E tasks must include the fresh-worktree bootstrap (`cd apps && pnpm install --frozen-lockfile`) when dependencies may be absent; direct web typechecking uses `cd apps/web && pnpm run typecheck`, while other workspace package commands use the documented `pnpm --filter` form. Backend commands should use the applicable repository `make` target when one exists. If several commands are required, write them as one sequentially runnable block rooted at a single directory (for example, `cd apps && ... && ...`) or make each `cd` explicit; do not rely on the caller's working directory surviving between commands. - **Files likely touched:** specific paths, not broad directories. - **Dependencies:** task numbers that must land first, or `None`. - **Parallelism:** `sequential` by default; set `parallel-safe` only with named diff --git a/.agents/skills/playwright-cli/SKILL.md b/.agents/skills/playwright-cli/SKILL.md index 19a817069a..a8e793af12 100644 --- a/.agents/skills/playwright-cli/SKILL.md +++ b/.agents/skills/playwright-cli/SKILL.md @@ -1,11 +1,16 @@ --- name: playwright-cli description: Automate browser interactions, test web pages and work with Playwright tests. -allowed-tools: Bash(playwright-cli:*) Bash(npx:*) Bash(npm:*) +allowed-tools: Bash(pnpm:*) --- # Browser Automation with playwright-cli +The workspace pins `@playwright/cli`. In the examples below, +`playwright-cli` means `pnpm --dir apps exec playwright-cli` from the repository +root. This keeps the examples short while using the lockfile version rather +than an unrelated global installation. + ## Quick start ```bash @@ -259,16 +264,12 @@ playwright-cli kill-all ## Installation -If global `playwright-cli` command is not available, try a local version via `npx playwright-cli`: +The workspace pins `@playwright/cli` in `apps/package.json`. If the workspace +dependencies are not installed, bootstrap them once: ```bash -npx --no-install playwright-cli --version -``` - -When local version is available, use `npx playwright-cli` in all commands. Otherwise, install `playwright-cli` as a global command: - -```bash -npm install -g @playwright/cli@latest +pnpm --dir apps install --frozen-lockfile +pnpm --dir apps exec playwright-cli --version ``` ## Example: Form submission diff --git a/.agents/skills/playwright-cli/references/request-mocking.md b/.agents/skills/playwright-cli/references/request-mocking.md index 141b505c4b..66e43e182d 100644 --- a/.agents/skills/playwright-cli/references/request-mocking.md +++ b/.agents/skills/playwright-cli/references/request-mocking.md @@ -66,6 +66,33 @@ playwright-cli run-code "async page => { }" ``` +### Hold an upload request while asserting busy state + +For multipart or large-upload pending-state tests, match one exact endpoint and +use a deterministic route handler. Resolve a `requestStarted` promise when the +handler runs, assert the busy contract, then release a promise that lets the +handler `route.fulfill` (or use `route.abort` for the failure path) and assert +recovery. Do not hold `route.fetch()` across competing upload handlers; that can +double-handle a route or corrupt gzip/body streams. + +```typescript +let markStarted!: () => void; +let release!: () => void; +const requestStarted = new Promise((resolve) => { markStarted = resolve; }); +const settle = new Promise((resolve) => { release = resolve; }); +await page.route("**/api/uploads/exact-endpoint", async (route) => { + markStarted(); + await settle; + await route.fulfill({ status: 200, json: { ok: true } }); +}); +const upload = triggerUpload(); +await requestStarted; +await expect(uploadButton).toHaveAttribute("aria-busy", "true"); +release(); +await upload; +await expect(uploadButton).not.toHaveAttribute("aria-busy", "true"); +``` + ### Simulate Network Failures ```bash diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index e98ddbe4e1..4125f6f04e 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -12,9 +12,10 @@ to wait for or monitor PR updates. For a cost-controlled workflow, the user may switch the same conversation to the lower-cost implementation/test model before starting CI remediation. -Use `gh` by default; when it is unavailable after access is approved, use an -available GitHub integration. `scripts/pr-state`, `scripts/pr-resolve`, and -`scripts/run-quiet` are at the worktree root. +Use `gh` by default; auth or transport errors leave state unknown, never clean. +If connector tools are available, use structured PR/check/thread data; avoid +dumping full HTML/diffs. Map GraphQL thread IDs to REST comment IDs before +replies, and refresh current-head state after pushes and review aggregation. ## Pipeline @@ -35,37 +36,42 @@ the environment requires. If access is denied, cancelled, or interrupted, stop the workflow permanently; retry only transient fetch failures after access is approved. -Run `scripts/pr-state --summary ` once. Record `checks_head_sha`, -`checks_snapshot_complete`, `failed_checks`, `pending_checks`, and the PR -head delivery fields (`pr.head_repository_owner`, `pr.head_repository_name`, -`pr.head_ref_name`, `pr.head_ref_oid`, and `pr.maintainer_can_modify`). For a -cross-repository PR, those fields are the only authoritative push target; do -not infer it from a local remote named `fork`, `contributor`, or similar. -Linked worktrees share Git configuration and such remotes may belong to an -unrelated task. -Record `unresolved_review_thread_count`, `hidden_unresolved_threads`, and -`actionable_issue_comment_count`. Inspect mergeability separately through -`references/merge-conflicts.md`; it is not a `pr-state --summary` field. If a -named reviewer is the semantic evidence source, use `--trusted-reviewer` only -when `review_evidence.trusted_producer` is `"true"`; never use that shortcut -for forks, security, or architecture. -Treat `trusted_producer=true` as qualifying provenance only for the dedicated OpenCode App, never merely because a reviewer name matches. -If `hidden_unresolved_threads` is non-empty, immediately run -`scripts/pr-resolve list ` and triage its output. A zero current-head -unresolved count does not make hidden threads clean. +Run `scripts/pr-state --summary ` once. Record the current-head check, +review, and PR-delivery fields described in +`references/review-evidence.md`. For a cross-repository PR, use only the +reported delivery fields as the push target; do not infer it from a local +`fork` or `contributor` remote. Load that reference for exact-head review +classification, authentication fallbacks, and hidden-thread handling. Inspect +mergeability separately through `references/merge-conflicts.md`. + +If the fresh mergeability query reports `mergeable=CONFLICTING` or +`mergeStateStatus=DIRTY`, stop CI and review triage. Load +`references/merge-conflicts.md`, resolve or rebase the conflict, verify the +result, push it, and restart this section with a new PR-state snapshot. Do not +triage comments or checks against a conflicted head. For pending CI, do not run a rapid polling loop. Wait at a reasonable interval, then run the same summary again. Stop after about 20 minutes and report the -exact pending checks as "CI in progress." If the user specifies a fixed -monitoring duration, remain in this direct loop until that duration elapses or -the PR reaches a terminal clean/failed state; do not return an early status. -Do not use interactive `gh pr checks --watch` in the primary conversation: its -TTY redraws make captured output unusable. Use saved `scripts/pr-state --summary` -snapshots about 60–90 seconds apart, or the read-only `pr-poller` only when the -user explicitly asked to wait or monitor. +exact pending checks as "CI in progress." Use two monitoring modes: default +monitoring may return early for a failed check, merge conflict, actionable +finding, or clean terminal state; strict-deadline monitoring applies when the +user says "wait N minutes" or "then fix up." In strict mode, calculate and +pass an absolute deadline, accumulate failures/comments, and do not return +early for findings, pending checks, or a clean snapshot; stop early only when +the PR is merged/closed or access is revoked. Do not use interactive `gh pr +checks --watch` in the primary conversation: its TTY redraws make captured +output unusable. Use saved `scripts/pr-state --summary` snapshots about 60–90 +seconds apart, or the read-only `pr-poller` only when the user explicitly asked +to wait or monitor. Treat a poller's unresolved/pending snapshot as provisional: it can predate a primary-session push or thread resolution. Re-run `scripts/pr-state --summary ` at the current head before acting on it or declaring completion. +If a job remains pending beyond the workflow's configured timeout, or its status +conflicts with the GitHub UI/API, query the exact job with +`gh api repos///actions/jobs/` (or inspect the run with +`gh run view `) before calling CI hung or changing code. Treat the direct +result as current-head evidence only after its `head_sha` matches +`checks_head_sha`; otherwise report the result as stale or unknown. For a cross-repository PR whose current-head snapshot is unexpectedly sparse, inspect `approval_required_runs`. A current-head workflow with @@ -86,13 +92,23 @@ the parent workflow/job status. A failed job can be visible while its workflow is still in progress; confirm its conclusion and failing step before treating it as reproducible code evidence. Use `scripts/run-quiet gh-run -- gh run view --log-failed` so large logs do -not flood the conversation. If logs are temporarily unavailable or only expose -an aggregate report job, use `scripts/pr-state --job-log ` with the -`job_id` in the summary; it handles GitHub's plain-text and ZIP job-log -responses and emits bounded matching context. Follow the rest of the fallback -in `references/ci-troubleshooting.md`. Reproduce the exact failed command where possible; -CI-specific Go lint often needs `golangci-lint run ./... --new-from-rev= ---timeout=5m`. +not flood the conversation. If it returns only GitHub request/transport lines +or no failure text, treat logs as unavailable and, after terminal state, use +`scripts/pr-state --job-log `. For temporary gaps or aggregate-only +logs, use the same fallback; it handles plain-text and ZIP responses and emits +bounded context. Follow `references/ci-troubleshooting.md`. Reproduce the exact +failed command where possible; CI-specific Go lint often needs +`golangci-lint run ./... --new-from-rev= --timeout=5m`. + +If CI reports files or commits outside the PR diff, or a stale base SHA, resolve +the authoritative base repository, ref name, and current base SHA from PR +metadata. Fetch that ref from an explicit base remote, verify its tip matches +the reported SHA, and compare `git merge-base HEAD /` +with `git diff /...HEAD`; do not assume `origin/` +when `origin` points to a fork. Inspect the parent workflow/run to determine +whether a newer base commit caused the failure before changing product or docs +code. If the fix is already upstream, update or rebase the branch only when +authorized, rerun affected checks, and invalidate all prior exact-head evidence. For unfamiliar, infrastructure, or E2E failures, load `references/ci-troubleshooting.md` before changing code. @@ -103,14 +119,27 @@ Fix with `/tdd` or `/e2e` as applicable, run focused checks, and keep each remediation scoped to the reported failure. Do not suppress a failure or mark a check clean without fresh evidence. +If a reproducible failure is outside the PR diff, compare the failing +assertion with the current implementation and concurrent or sibling PRs before +editing. If it is a stale test expectation, the smallest valid remediation may +be a test-only assertion update: keep it limited to the reported failure, run +the focused test, and call out that scope. Do not change unrelated production +behavior or duplicate a larger sibling change. +When a remediation changes a documented behavior or contract, update the +authoritative spec/guidance, plan, and task file when present; commit docs and +code together, keep regression tests and verification commands aligned, and +re-check the documentation before completion. Record why no update is needed +when the behavior remains internal. + ## 3. Triage And Address Reviews Use `scripts/pr-resolve list ` to obtain unresolved threads. Its previews -can be truncated, so expand each listed thread with -`scripts/pr-state --comment ` before deciding whether it is valid, -already addressed, a preference, or wrong for this codebase. Validate against -the current head, the spec, and existing architecture before editing or -replying. +can be truncated, so expand each listed review thread with +`scripts/pr-resolve show ` before deciding whether it is valid, +already addressed, a preference, or wrong for this codebase. Use +`scripts/pr-state --comment ` only for a flat comment view when a +thread context is not available. Validate against the current head, the spec, +and existing architecture before editing or replying. Make only valid changes. GitHub replies and thread resolution are external writes. A direction to "address" valid review comments explicitly authorizes a @@ -120,6 +149,21 @@ concrete reasoning only when that authorization includes a response. When writes are not authorized, report valid comments as addressed in code but still unresolved; do not declare the PR clean solely from the code change. Resolve an authorized thread only when the change or response genuinely addresses it. +An explicit request to "address them" authorizes replies and resolution only +for the selected current actionable threads. If new comments appear during +remediation, report them or obtain separate confirmation before replying or +resolving them. + +After an authorized fix is pushed, use the atomic helper path +`scripts/pr-resolve reply --body-file ` to +reply, resolve, and react in one operation when the body contains Markdown or +shell metacharacters. For short plain-text bodies, a safely quoted argument is +acceptable. Never interpolate review text into an unquoted shell command or +use backticks in the command itself. After the helper returns, re-fetch the +thread and verify the posted reply body before treating the write as complete. +Then rerun +`scripts/pr-resolve list ` and the exact-head `scripts/pr-state --summary +` check before reporting. For an ordering or concurrency finding, trace the complete producer → event-bus transport → gateway/client path. Sequential publishes do not prove delivery @@ -142,6 +186,11 @@ summary's authoritative head repository and ref only when `pr.head_ref_oid` to equal local `HEAD`; an upstream remote comparison is not sufficient. Run broad `/verify` only if the user explicitly requests it or the PR/CI finding requires it. +After every push, also run a fresh +`gh pr view --json baseRefName,headRefOid,mergeable,mergeStateStatus` (or +equivalent) at that pushed head. Require `headRefOid` to equal local `HEAD` and +confirm `mergeable` is not `CONFLICTING` and `mergeStateStatus` is not `DIRTY`; +`scripts/pr-state --summary` does not include mergeability. Immediately before a remediation commit or push—and again after long-running remediation—refresh PR state. Require the PR to remain open and its head ref to @@ -150,26 +199,73 @@ local upstream tip; after the push, require the PR head OID to equal local `HEAD`. If the PR merged or closed, do not recreate its deleted branch with a stale push: preserve the local fix and ask before creating a clean follow-up. +After any rebase or force-push, fetch the PR base and compare local `HEAD`, the upstream tip, and `pr.head_ref_oid`; rerun affected checks. +A rebase onto a newer base invalidates prior evidence: rerun the exact failed +command and any package-level gate required by whole-file rules (for example +max-lines). Then rerun `scripts/pr-resolve list ` and `scripts/pr-state --summary `; distinguish stale/current +failures and report pending checks separately. Use `--force-with-lease`, never an unconditional force-push. +If the rebase or conflict resolution touched `AGENTS.md`, `CLAUDE.md`, or a +skill/reference file, run the shared harness validation in +`.agents/skills/harness-improvement/references/validation.md` before pushing. + ## 5. Re-check -After every push, run `scripts/pr-state --summary ` again for the new head. +After every push, re-fetch current-head state and run +`scripts/pr-resolve list ` (and `show` for any thread you may answer), then +`scripts/pr-state --summary ` again for the new head. For explicit +consolidation, verify the target head before commenting/closing the superseded +PR; preserve its branch unless deletion is requested. Automated reviewers may +resolve or replace threads; do not reply to or resolve a thread that fresh state +reports as resolved. Before replying to a pre-push thread, run +`scripts/pr-resolve show `; if it reports `resolved: true` (often +with an `Addressed in commit ...` marker), record the thread as auto-resolved and +do not post a duplicate reply. Continue replying/resolving only for `resolved: false` threads, including hidden unresolved threads. +Treat each fresh summary as a new review-evidence snapshot: inspect every +non-empty body in `review_evidence.exact_current_head_reviews[]`, even when +`unresolved_review_thread_count=0` and `scripts/pr-resolve list` is empty. +Classify current-head review bodies and top-level bot/issue comments before +declaring the PR clean; empty thread and issue-comment counts are insufficient. +Treat a non-empty `hidden_unresolved_threads` value in that fresh snapshot as a +mandatory hidden-thread gate: run `scripts/pr-resolve list ` again after +the refresh and immediately before reporting. Require `checks_head_sha` to match that head, report pending checks separately from failures, and rerun `scripts/pr-resolve list ` before declaring the -PR clean. Treat prior review evidence as stale. When the user authorized thread +PR clean. The final predicate must also require +`hidden_unresolved_threads=[]`; do not require filtered and unresolved counts to +be equal because the filtered count includes resolved threads. Treat prior review +evidence as stale. When the user authorized thread writes, a duplicate or stale bot thread still needs an explicit reply and resolution once current source proves the finding is already fixed, including a thread surfaced only in `hidden_unresolved_threads`; only current-head -actionable threads drive code changes. Declare the PR clean only when +actionable threads drive code changes. Declare the PR clean only when the +exact-current-head review classification reports no unaddressed findings, `checks_snapshot_complete=true`, `failed_checks=[]`, `pending_checks=[]`, -`approval_required_runs=[]`, `actionable_issue_comment_count=0`, there is no merge conflict, and +`approval_required_runs=[]`, `actionable_issue_comment_count=0`, +`hidden_unresolved_threads=[]`, there is no merge conflict, and `scripts/pr-resolve list ` is empty. Within the user's monitoring limit, continue checking after resolutions until automated review jobs are terminal; otherwise report the exact pending check names. -If the task has a persisted Kandev plan, update it after fixup with the -remediation commit, final exact-head check counts, resolved-thread state, and -mergeability. Do not leave planned verification marked unstarted after it has -run. +If the user explicitly requested a persistent Kandev plan update and the task +has an external Kandev plan, call `get_task_plan_kandev` before fixup and +`update_task_plan_kandev` after fixup with the remediation commit, final +exact-head check counts, resolved-thread state, and mergeability. Without that +authorization, report the plan update as pending and do not invoke Kandev task +or session APIs. Batch plan/task synchronization into the final documentation +commit. Record the prior head's fixup evidence before a plan commit/push: that +push restarts CI and invalidates the snapshot. For tracked `docs/plans/**` +artifacts, keep prose head-agnostic and record remediation scope/local +verification before that commit; then rerun `scripts/pr-state --summary` and +`scripts/pr-resolve list` for the new head and report its pending checks +separately. Mark prior current-head claims historical/superseded when a new +head replaces them; report only the latest head's SHA, CI/review counts, and +mergeability. Do not leave planned verification marked unstarted after it has run. + +Before declaring fixup complete, verify `git status --short` is clean, +`git rev-parse HEAD` equals `git rev-parse @{upstream}`, the PR head equals +local `HEAD`, and the fresh mergeability state is not conflicting. Do not call +the PR clean from CI/review counts alone when the worktree or remote tip still +differs. ## 6. User-Requested Merge diff --git a/.agents/skills/pr-fixup/references/ci-troubleshooting.md b/.agents/skills/pr-fixup/references/ci-troubleshooting.md index 23d5bb9fae..13acc057e4 100644 --- a/.agents/skills/pr-fixup/references/ci-troubleshooting.md +++ b/.agents/skills/pr-fixup/references/ci-troubleshooting.md @@ -101,6 +101,12 @@ is usually a superseded GitHub run, not a code failure. Confirm the non-cancelled run for the same head SHA passed, then trigger one clean run (rebase onto main + force-push, or `gh run rerun `). +**Required job timeout cancellation:** If a required job is cancelled at its +configured `timeout-minutes`, inspect its concrete steps and logs. When the +race/test step has no assertion failure and one rerun repeats the same timeout, +classify it as CI infrastructure, stop rerunning, and report the exact check, +timeout, and pending-state evidence. + **Manual-review event provenance:** For an unexpected zero-duration or no-op manual bot-mention run, inspect the workflow at the repository default branch and the event payload before assuming the PR is behind `main`. Privileged @@ -173,6 +179,18 @@ past a valid race, leak, or product defect because it appears unrelated; fix it and exercise the affected package without retry masking. Only an evidenced external dependency or infrastructure fault is exempt from code remediation. +If a Backend Tests shard log names only `fail: github.com/.../package` without +the test name, download its matching `backend-test-results-` artifact: + +```bash +gh run download --name backend-test-results- --dir +rg -n '"Action":"fail"' +``` + +The artifact JSONL may start with `go: downloading ...` or other non-JSON +preamble lines; skip those lines when parsing so the recorded test name can be +reproduced exactly. + ## E2E Failures If any failing check is an E2E test: @@ -184,6 +202,12 @@ If any failing check is an E2E test: in-DOM React render errors that often show up in `e2e/test-results//error-context.md`. +Before attributing an E2E failure to product code, compare the failed run SHA +and `checks_head_sha` with local `HEAD`. PR workflows may test GitHub's +synthetic merge commit, so reproduce the exact base-plus-head merge (or checked +merge ref) with CI environment, retries disabled, and the same shard/order; +verify the checkout SHA before interpreting the result. + Useful focused commands: ```bash @@ -215,3 +239,17 @@ instead of repeatedly dumping the full checks table: gh run view --json status,conclusion,jobs \ --jq '{status, conclusion, remaining: [.jobs[] | select(.status != "completed" or .conclusion != "success") | {name, status, conclusion}]}' ``` + +For an explicit user requirement that an E2E run contain no flakes or retries, +run the deterministic blob audit after the exact-head E2E merge report +completes: + +```bash +python3 scripts/playwright-blob-audit +``` + +It recursively reads `report-*.zip` and `report.jsonl` artifacts, reports +attempts, retry attempts, `onTestEnd` status counts, and results with errors, +and exits nonzero for retries, errors, parse failures, or unexpected statuses. +Do not make this extra artifact audit part of ordinary PR fixup; a normal green +aggregate check is sufficient unless the no-flakes requirement is explicit. diff --git a/.agents/skills/pr-fixup/references/merge-conflicts.md b/.agents/skills/pr-fixup/references/merge-conflicts.md index 7c7f7b76b6..2bb46de97a 100644 --- a/.agents/skills/pr-fixup/references/merge-conflicts.md +++ b/.agents/skills/pr-fixup/references/merge-conflicts.md @@ -41,7 +41,7 @@ If `git ls-files -u` prints entries, or conflict markers are present in tracked the user explicitly authorizes bypassing hooks. If the merge commit fails or exits ambiguously, inspect and report the hook result; do not bypass it just to finish the conflict resolution. -3. If conflicts appear, inspect each conflicted file, preserve the intended behavior from both sides, remove all conflict markers, and stage only the resolved files. +3. If conflicts appear, inspect each conflicted file, preserve the intended behavior from both sides, remove all conflict markers, and stage only the resolved files. When the index has conflict stages, inspect the competing versions with `git show :2:`, `git show :3:`, and `git log -- ` (or compare the merge-base, current branch, and base branch versions when stages are unavailable). Do not choose ours/theirs merely to remove markers; preserve each side's behavioral invariant, compare analogous provider implementations and their tests, then run the focused test for the conflicted feature. 4. Confirm the conflict is gone before continuing: ```bash git ls-files -u @@ -52,6 +52,15 @@ If `git ls-files -u` prints entries, or conflict markers are present in tracked ``` Confirm that the final diff against the current GitHub base contains only the PR's intended delta. A clean index is insufficient if a retargeted stacked PR accidentally retains its merged parent's changes. + After the operation completes, inspect each resolved path with + `git diff -- origin/...HEAD -- ` and reread the + complete surrounding comments and logic; this catches a duplicate-hunk + resolution that silently drops an explanatory line. + + For structured files (especially JSON catalogs), validate syntax and duplicate + keys before continuing. JSON's default parser may silently keep the last + duplicate key; use an `object_pairs_hook` that raises on duplicates, then run + the affected formatter/test and `git diff --check`. ### When the local index is already unmerged @@ -75,16 +84,49 @@ If `git ls-files -u` shows entries, a previous merge or rebase was left incomple ``` 5. Complete the interrupted operation: ```bash - git commit + git commit --no-edit # or, if mid-rebase: git rebase --continue ``` + Use the normal hooks and capture the full receipt. If `git commit --no-edit` + fails, fix the reported semantic or hook error and retry the commit; never + bypass it with `--no-verify`. A current-base merge may stage many incoming + files, making status look like hundreds of changes; do not reset or broadly + unstage them. Resolve only `UU` paths, then inspect `git diff --stat + origin/` and `git diff --name-only origin/` before + committing, and fix any incoming harness-file violation minimally rather + than bypassing hooks. + +If Git reports an `index.lock`, inspect the exact path and active Git processes +first (`git rev-parse --git-path index.lock` and a process listing). Never delete +a lock owned by a live Git process. Remove only that exact lock path after +confirming no Git process owns it, then retry the operation. + Do not discard unrelated user changes to make a merge/rebase easier. If unrelated dirty files block the conflict-resolution attempt, stop and ask before stashing, committing, or reverting them. -## Push after rebasing +Before a force-push after a rebase or conflict merge, run the task-defined checks +for every affected package. For web changes, run the affected test suite plus +`cd apps/web && pnpm run typecheck` and the web lint gate; use the equivalent +package checks for backend changes. Cleared markers do not prove a semantic +resolution: duplicate imports/hooks and stale localized mocks require these +checks to pass. -Capture the remote branch SHA before rebasing when possible. After a successful rebase, prefer an exact force lease: +## Push after rebasing or merging + +Capture the remote branch SHA before fetching the base or starting a merge/rebase. +Recheck it immediately before pushing. This guards both merge- +based conflict fixes and rebases against another writer advancing the PR branch: + +```bash +git ls-remote origin refs/heads/ +# fetch/merge or rebase, resolve, test, and commit through hooks +git ls-remote origin refs/heads/ +``` + +If the second SHA differs from the first, stop and review the intervening remote +changes before pushing; do not overwrite them. When the captured SHA is +unchanged, prefer the explicit lease for both conflict merges and rebases: ```bash git push \ @@ -92,4 +134,14 @@ git push \ origin HEAD:refs/heads/ ``` -An intervening fetch can update the remote-tracking ref consulted by generic `--force-with-lease`. Use the generic form only when the prior remote SHA was not captured, and never use an unconditional force push. +An unchanged merge-based fixup is also a fast-forward, so a normal push is +safe when an explicit lease is unavailable: + +```bash +git push origin HEAD:refs/heads/ +``` + +An intervening fetch can update the remote-tracking ref consulted by generic +`--force-with-lease`. If the prior remote SHA was not captured, stop and capture +it before rewriting when possible; use the generic form only as an explicit last +resort, and never use an unconditional force push. diff --git a/.agents/skills/pr-fixup/references/review-evidence.md b/.agents/skills/pr-fixup/references/review-evidence.md index 03f5a1aac1..fb01036eb3 100644 --- a/.agents/skills/pr-fixup/references/review-evidence.md +++ b/.agents/skills/pr-fixup/references/review-evidence.md @@ -21,13 +21,49 @@ The summary fields are: from timestamps. - `errors`: affected data is unknown; do not reconstruct it from memory. +Record `checks_head_sha`, `checks_snapshot_complete`, `failed_checks`, +`pending_checks`, review counts, and the PR delivery fields +(`pr.head_repository_owner`, `pr.head_repository_name`, `pr.head_ref_name`, +`pr.head_ref_oid`, and `pr.maintainer_can_modify`). For a cross-repository PR, those +delivery fields are the only authoritative push target. A named-reviewer +shortcut requires `review_evidence.trusted_producer="true"` and is not valid +for forks, security, or architecture. + +Inspect every non-empty body in +`review_evidence.exact_current_head_reviews[]`, including `COMMENTED` +aggregate-bot reviews, and classify it as actionable, already addressed, +optional, or invalid before declaring reviews clear. `trusted_producer=true` +qualifies only for the dedicated OpenCode App, never merely because a reviewer +name matches. + +If `gh`, `scripts/pr-state`, or `scripts/pr-resolve` fails with an +authentication or transport error (including a broker 401), do not treat empty +or unknown output as clean. If GitHub connector tools are available, use their +equivalents of `github_fetch_pr`, +`github_list_pull_request_review_threads` (including `is_resolved` and +`is_outdated`), `github_fetch_pr_comments`, +`github_fetch_commit_workflow_runs`, and +`github_get_commit_combined_status`. Gather PR metadata, review threads and +discussion comments, and commit workflow/status evidence. Keep SSH Git +operations available for fetch, rebase, and push; after a push, require the +connector-reported PR head OID to equal local `HEAD`. If the fallback cannot +provide CI or review evidence, report it as unknown or pending, never clean. + +For connector-backed review writes, prefer structured workflow results and +`github_list_pull_request_review_threads`; do not request full PR HTML or diffs. +Map a GraphQL thread ID to its REST numeric top-level comment ID with +`github_fetch_pr_comments` before replying, but resolve using the GraphQL thread +ID. After every push and after automated review aggregation, refresh current-head +checks and threads. + If head metadata fails but check/thread data is usable, use that data only for the current poll and retry once at the next cadence. If review-thread state is unknown, do not call review clean/blocked; retry once, then use `scripts/pr-resolve list ` before a final status. If total unresolved count is nonzero while visible threads are empty, fetch the authoritative thread list -and full bodies with `scripts/pr-state --comment ` or -`scripts/pr-resolve show `. +and full bodies with `scripts/pr-resolve show `; use +`scripts/pr-state --comment ` only when a flat comment view is all +that is available. If `branch:"unknown"` or PR-view resolution is transient, retry the explicit PR-number command once before using direct targeted GitHub fallback. Do not @@ -58,16 +94,22 @@ scripts/pr-resolve list Transport/collection failure leaves checks unknown. Parseable pending/failing rows remain usable when `gh pr checks` exits 8; never hide diagnostics in a pipe. -When `hidden_unresolved_threads` exists or total unresolved count exceeds the -visible list, fetch each body with `scripts/pr-state --comment ` or -`scripts/pr-resolve show `. A listed thread that -is already resolved is stale summary state: re-poll and do not reply again. +When `hidden_unresolved_threads` is non-empty, fetch each thread body with +`scripts/pr-resolve show `; use the flat comment command only +for a comment without thread context. A listed thread that is already resolved +is stale summary state: re-poll and do not reply again. Poll at 30-second cadence with a 20-minute cap using bounded one-shot commands; -avoid long inline loops and `gh pr checks --watch`. Stop early on a required -failure. Queued/in-progress jobs are pending, not speculative-fix triggers. On -an explicit wait-through-CI request, repeat bounded checks until failures, -pending checks, and unresolved-thread count are all empty/zero. +avoid long inline loops and `gh pr checks --watch`. In default monitoring, +stop early on a required failure. For an explicit fixed-duration request, use +strict-deadline mode: accumulate failures and comments until the absolute +deadline, stopping early only if the PR is merged/closed or access is revoked. +Queued/in-progress jobs are pending, not speculative-fix triggers. On an +explicit wait-through-CI request without a fixed deadline, use the same +20-minute absolute cap: repeat bounded checks until failures, pending checks, +and unresolved-thread count are all empty/zero, or stop at the deadline and +report remaining pending checks or unresolved threads. Preserve early stopping +for failures and merged/closed or access-revoked conditions. For E2E-only pending work, summarize a saved snapshot before printing shards: diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index 4b8592e316..9f4598d13b 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -31,6 +31,11 @@ checks, and push. Ready PR monitoring and remediation continue through - `--draft` — create the PR as draft and skip the fixup step. Use when the work is not ready for review. - Default (no flag) — create as ready-for-review and continue with `/pr-fixup` in the same conversation. +**Publishing-state precedence:** explicit user state (`--draft` or an explicit +ready-for-review request) wins. If the `github:yeet` plugin is explicitly +selected, follow its draft default only when the user did not request either +state; otherwise `/pr` defaults to ready-for-review. Always report the result. + ## Steps Track these steps with an internal todo/checklist and mark them complete as you go. @@ -61,7 +66,10 @@ explicitly requests task tracking. and stop before PR publication. For non-UI changes, record that screenshots are not required and continue. -5. **Create the PR.** Use `--draft` flag if the user requested draft mode, otherwise create as ready-for-review. +5. **Create the PR.** Before creating, check open PRs for the current branch and + inspect task-linked PR metadata. Reuse an existing PR; create a duplicate only + when the user explicitly requests separate PRs. Use `--draft` if requested, + otherwise create as ready-for-review. **PR title** must follow Conventional Commits format (see `/commit` for full rules). CI validates via `pr-title.yml` — the PR title becomes the squash-merge commit used for release notes. @@ -90,16 +98,27 @@ required screenshot embedding in step 7 is complete. 7. **Screenshots — publish already captured assets.** If the diff touches user-visible UI (typically under `apps/web/`, excluding e2e-only or backend-only edits), publish the affected-viewport assets captured and validated in step 4 through the host-specific flow before treating the PR as complete — do not wait to be asked. Preserve any structural-absence rationale recorded in step 4. **Capture prerequisite:** - - If `npx --no-install playwright-cli list` has no local browser, use the + - If `pnpm --dir apps exec playwright-cli list` has no local browser, use the managed `apps/web` E2E runner with a disposable capture spec instead of treating capture as blocked. Name mobile specs `mobile-*.spec.ts`, write assets to ignored `apps/web/.pr-assets`, inspect/compress them, then remove the temporary spec and confirm `git status` is clean. + - For disposable capture specs, prefer the existing `prCapture` fixture from + `apps/web/e2e/fixtures/test-base.ts`; it writes the expected filenames and + manifest for PR assets. + - `apps/web/e2e/scripts/run-e2e.sh` clears `.pr-assets` at the start of each + managed-runner invocation. Capture desktop and mobile assets in one + invocation when possible; if separate runs are necessary, preserve/merge + the prior assets and revalidate the complete manifest afterward. - Reuse only fresh entries from `apps/web/.pr-assets/manifest.json`. After every capture, require a non-empty manifest with the intended fresh asset entries: `test -s apps/web/.pr-assets/manifest.json`. If it is absent or lacks the capture, do not treat the run as successful; rerun with `--host` and report the managed-runner gap. + - Before inspecting, compressing, or publishing the assets, verify that every + manifest entry maps to an existing file under `apps/web/.pr-assets`. If any + entry is missing, treat the capture as incomplete and restore or recapture + the asset; revalidate the complete mapping immediately before publication. - If required assets are missing, run the Playwright capture before publication; do not create the PR first. - For a mobile capture through `pnpm e2e:run`, select the runner project @@ -148,21 +167,14 @@ required screenshot embedding in step 7 is complete. ```bash set -euo pipefail PAYLOAD="/tmp/pr-body--payload.json" - if command -v rtk >/dev/null 2>&1; then - rtk proxy jq -n --rawfile body "" '{body: $body}' > "$PAYLOAD" - rtk proxy jq empty "$PAYLOAD" - else - jq -n --rawfile body "" '{body: $body}' > "$PAYLOAD" - jq empty "$PAYLOAD" - fi + jq -n --rawfile body "" '{body: $body}' > "$PAYLOAD" + jq empty "$PAYLOAD" gh api --method PATCH repos/:owner/:repo/pulls/ --input "$PAYLOAD" ``` - **RTK and JSON payloads:** RTK is optional. If it is installed, it - summarizes normal stdout, so it must not sit between a JSON producer and a - redirected file or another parser. The conditional recipe above keeps the - REST fallback byte-preserving whether or not RTK is installed. - The same rule applies to command substitutions, `xargs`, and any other - consumer that expects unmodified Git or JSON output. + **JSON payloads:** Use `jq` (or an equivalent JSON tool) to build and + validate payloads without interpolating untrusted Markdown into shell + syntax. Keep the REST fallback byte-preserving for command substitutions, + `xargs`, and any other consumer that expects unmodified Git or JSON output. **Preserve the existing PR description:** The PR body is a shared, mutable document. Preview automation and other bots may add sections after the PR @@ -182,9 +194,11 @@ required screenshot embedding in step 7 is complete. before PATCH and compare the two live snapshots (not the candidate with a snapshot). Use PR-scoped temporary filenames, fail immediately on a differing `cmp`, and discard any payload on failure; never reuse a prior - `/tmp` payload. Verify the payload contains this PR's current body and - required sentinels before PATCH. If it changed, re-fetch and merge again; - do not overwrite the newer body. + `/tmp` payload. Extract JSON bodies byte-for-byte with `jq -j .body` before + `cmp`; `jq -r .body` appends a newline and can report a false mismatch. + Verify the payload contains this PR's current body and required sentinels + before PATCH. If it changed, re-fetch and merge again; do not overwrite + the newer body. 4. After PATCH, read the body back and verify both the intended change and all previously present sentinel sections are still present. diff --git a/.agents/skills/push/SKILL.md b/.agents/skills/push/SKILL.md index c66ba72111..a9daf0fd6d 100644 --- a/.agents/skills/push/SKILL.md +++ b/.agents/skills/push/SKILL.md @@ -63,6 +63,19 @@ Push the already committed branch to its remote. gh auth setup-git git push "https://github.com//.git" "HEAD:refs/heads/" ``` + If that push fails with `git repository does not match any credential lease + scope`, do not put a token in command arguments or logs. Retry after + `gh auth setup-git`; if HTTPS still rejects the contributor-fork push, use + the SSH fallback below or report the authentication/scope blocker. + Never embed the token in a remote URL, command argument, log, or persisted + credential. Re-fetch the PR and require its `headRefOid` to equal local + `HEAD` afterward. + If HTTPS still rejects a contributor-fork push and SSH authentication is + available, verify the exact target with + `git ls-remote git@github.com:/.git`, then retry + the same exact ref over SSH (using the exact remote-OID lease if history was + rewritten). Re-fetch and verify the PR head OID again. Do not substitute a + local `fork` remote or an inferred repository. Do not use a conveniently named local `fork`/`contributor` remote: linked worktrees share remote configuration, so it can refer to another task. Re-fetch the PR and require `headRefOid` to equal local `HEAD`. @@ -72,7 +85,10 @@ Push the already committed branch to its remote. `git rev-parse HEAD` equals `git rev-parse '@{upstream}'`, and report the branch from `git branch --show-current`. 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`. + branch is not `main` or `master`, then use the exact remote-OID lease + `git push --force-with-lease=refs/heads/:`. + Use generic `--force-with-lease` only when no remote OID was captured; never + use an unconditional force push. If the branch modifies `.github/workflows/*` and GitHub rejects the push with a message like `refusing to allow an OAuth App to create or update workflow ... without workflow scope`, treat it as push authentication/scope, not a code diff --git a/.agents/skills/release/SKILL.md b/.agents/skills/release/SKILL.md index d093aaf26a..5a97a0caaa 100644 --- a/.agents/skills/release/SKILL.md +++ b/.agents/skills/release/SKILL.md @@ -36,6 +36,15 @@ Stable runs entirely in CI via `.github/workflows/release.yml`, triggered by a m 5. `publish-npm` publishes 5 `@kdlbs/runtime-*` packages + main `kandev` package to npmjs. 6. `update-homebrew-tap` pushes updated `Formula/kandev.rb` to `kdlbs/homebrew-kandev` via SSH deploy key. +**Workflow-control invariant:** When a channel intentionally skips a job, every +downstream job reachable through that dependency chain must use a status function +such as `!cancelled()` plus explicit `needs..result == 'success'` checks. +For a partial Stable release, preserve the existing signed tag and rerun with +`backfill_tag`; never run a normal bump against an existing tag. Declare Stable +complete only after `publish-release`, `publish-npm`, and `update-homebrew-tap` +each succeed and their artifacts are verified—an aggregate green run can hide +skipped publication jobs. + Stable has no local release driver; the entire Stable flow runs in GHA. The Nightly metadata and publication revalidation state machine lives in `scripts/release/nightly-release.sh`, which GHA invokes for scheduled and manual Nightly runs. diff --git a/.agents/skills/runtime-feature-flags/SKILL.md b/.agents/skills/runtime-feature-flags/SKILL.md index 6138c352cd..a260b96ad3 100644 --- a/.agents/skills/runtime-feature-flags/SKILL.md +++ b/.agents/skills/runtime-feature-flags/SKILL.md @@ -22,7 +22,10 @@ those subtrees. - Use one identity across layers: `features.`, `KANDEV_FEATURES_`, a Go `FeaturesConfig` field, its JSON tag, and the matching frontend key. Never add a parallel flag map or switch. -- Never reuse an identity listed in `retiredRuntimeFlagIdentities`. +- Never reuse an identity recorded in the repository's append-only retired + registry. Before relying on retirement, verify that `retiredRuntimeFlagIdentities` + and its collision/completeness test exist; if they do not, add that + runtimeflags infrastructure before removing a live flag. ## Add a flag @@ -34,13 +37,15 @@ Update these layers in the same change: exception. 2. **Backend config:** add a `bool` field with explicit `mapstructure` and `json` tags to `apps/backend/internal/common/config/config.go`. -3. **Registry binding:** add exactly one complete registration to +3. **Registry/config binding:** add exactly one metadata registration to `apps/backend/internal/runtimeflags/registry.go`: key, env var, kind, label, - description, stability, risk metadata, restart/mutability metadata, typed - `read`, and typed `apply` functions. -4. **Backend gates:** gate construction and every enabled-only entry path at - the narrow composition boundary. Do not only hide the frontend; direct - callers must receive the legacy behavior or a safe rejection. + description, stability, risk, and restart/mutability metadata. The registry + definition is metadata-only; add the key/env constants and update + `OptionsFromConfig`, `ValuesFromConfig`, and `ApplyStatesToConfig` in + `runtimeflags/config.go` for typed config wiring. +4. **Backend gates:** gate construction and every enabled-only entry path at the + narrow composition boundary. Do not only hide the frontend; direct callers + must receive the legacy behavior or a safe rejection. 5. **Frontend contract:** add the all-off key to `apps/web/lib/state/slices/features/types.ts`. Use `useFeature()` for client surfaces and `notFound()` from the relevant server layout/page when a route @@ -77,8 +82,9 @@ behavior unconditional and remove the live flag end-to-end: - remove flag-specific tests and documentation while keeping permanent behavior coverage. -Before removing the registration, append its exact key and environment variable -to `retiredRuntimeFlagIdentities` in `registry.go`: +Before removing the registration, verify the append-only +`retiredRuntimeFlagIdentities` registry and its collision/completeness test +exist, then append the exact key and environment variable in `registry.go`: ```go {key: "features.example", envVar: "KANDEV_FEATURES_EXAMPLE"}, @@ -92,12 +98,14 @@ state from reactivating a future feature. Never reuse either the key or env var. Run the focused checks appropriate to the change: -- from `apps/backend`: `rtk go test ./internal/runtimeflags ./internal/common/config ./internal/profiles`; -- from the repository root: `rtk make -C apps/backend lint`; -- from `apps`: `rtk pnpm --filter @kandev/web test -- lib/state/slices/features/features-contract.test.ts`; -- from `apps/web`: `rtk pnpm run typecheck` and `rtk pnpm run lint`; +- In a fresh worktree, run `pnpm install --frozen-lockfile` from `apps/` before + any pnpm-based checks, tests, lint, or commits. +- from `apps/backend`: `go test ./internal/runtimeflags ./internal/common/config ./internal/profiles`; +- from the repository root: `make -C apps/backend lint`; +- from `apps`: `pnpm --filter @kandev/web test -- lib/state/slices/features/features-contract.test.ts`; +- from `apps/web`: `pnpm run typecheck` and `pnpm run lint`; - run affected E2E coverage when the gated surface is user-visible; -- run `rtk git diff --check` before handoff. +- run `git diff --check` before handoff. Report the flag key/env identity, profile defaults, every gated entry path, disabled/enabled test evidence, restart requirements, and whether the change is diff --git a/.agents/skills/tdd/SKILL.md b/.agents/skills/tdd/SKILL.md index 54afba0f10..d58adf0b90 100644 --- a/.agents/skills/tdd/SKILL.md +++ b/.agents/skills/tdd/SKILL.md @@ -50,6 +50,8 @@ Choose the right level: Prefer state/output assertions over interaction assertions. Mock only slow, nondeterministic, or external boundaries; use real implementations or fakes when they keep the test deterministic. +For failure-path tests, inject the error at the boundary the production code claims to handle and exercise the real downstream call chain; do not short-circuit by mocking the handler under test. + ### Cross-layer contracts When adding or renaming a field that crosses backend, WebSocket, and frontend @@ -84,6 +86,7 @@ state; and stale callbacks must not mutate the replacement. 1. Identify the single behavior to implement or bug to reproduce 2. Write the **smallest test** that asserts the expected behavior — one assertion, clear name 3. Run the test and confirm it **fails with the expected assertion error** (not a compile/import error) + For a brand-new Go package, create the package directory and minimal test package first, then run the focused package test so RED fails on behavior rather than package-selection or import errors. 4. If it passes immediately, the test is not testing new behavior — revise it. Exception: a reviewer-requested test that documents behavior already present on the current head is valid test-only contract coverage. Label it as such, @@ -124,6 +127,7 @@ a later patch. **Don't test implementation details:** - Assert behavior, state, API response, DB row, emitted event, or UI outcome. Avoid assertions that only prove a helper was called or an internal query string happened to be built a certain way. +- Custom hooks and async controller helpers are behavior-bearing logic, not pure React markup: add focused tests for success, failure, cancellation/no-op, and busy/loading-state cleanup. **Don't test mock behavior:** - If your assertion checks a mock element (`*-mock` test ID, mock return value), you're testing the mock, not the code. Test real behavior or don't mock it. diff --git a/.agents/skills/using-agent-skills/SKILL.md b/.agents/skills/using-agent-skills/SKILL.md index 6b2ba926cb..ef3fd694ed 100644 --- a/.agents/skills/using-agent-skills/SKILL.md +++ b/.agents/skills/using-agent-skills/SKILL.md @@ -37,6 +37,10 @@ Task arrives `-- Release/versioning? --------------------> /release ``` +When runtime-flag work also matches new behavior or validation, compose +`/runtime-feature-flags` with `/spec-driven-development` and `/tdd` as needed. +Use the smallest covering set and state the order. + ## Operating Rules 1. Work in the user-started primary conversation; do not create a worker session. @@ -51,6 +55,10 @@ Task arrives 8. Keep all repository work in the primary conversation. Use durable spec, plan, and task files as the user-controlled handoff when switching models. 9. Product media always invokes `/product-demo-seeding` before `/product-video-capture`, even when a prior seed or capture exists. Re-prove current `origin/main`, disposable runtime/data, and teardown; never capture a developer instance or database. +10. In Kandev repos, local `/commit`, `/push`, and `/pr` workflows, the repository + PR template, and `.github/AGENTS.md` are authoritative for publication. + External `github:yeet` is transport fallback only and must still use the + local template and checklist; never replace them with a hand-composed body. ## Upstream Name Mapping diff --git a/.codex/agents/pr-poller.toml b/.codex/agents/pr-poller.toml index 69f05c6f1d..41b9d1e730 100644 --- a/.codex/agents/pr-poller.toml +++ b/.codex/agents/pr-poller.toml @@ -12,9 +12,15 @@ Do not read source code, edit files, push, post or resolve GitHub comments, trigger workflows, fetch full CI logs, or spawn subagents. Use scripts/pr-state --summary and scripts/pr-resolve list as the -primary sources. Poll at a 30-second cadence for at most 20 minutes. Return -early for a failed check, merge conflict, actionable review feedback, or a -terminal clean state. At the time limit, return the pending checks and reviews. +primary sources. Poll at a 30-second cadence for at most 20 minutes. In the +default mode, return early for a failed check, merge conflict, actionable +review feedback, or a terminal clean state. If the caller says "wait N +minutes" or "then fix up", use strict-deadline mode: calculate and include the +absolute deadline in the polling prompt, accumulate findings, and do not return +early for findings, pending checks, or a clean snapshot. Stop early only if the +PR is merged/closed or access is revoked. At the deadline, return the latest +named pending checks and named actionable review findings, not only aggregate +CI and review states. Before the first GitHub request, obtain any runtime network approval required by the platform. A denied, cancelled, or interrupted approval is terminal: do not @@ -23,6 +29,7 @@ retry or relaunch the poller. Return only: PR at CI: -Reviews: +Reviews: ; findings: +Pending checks: Next action: """ diff --git a/.github/AGENTS.md b/.github/AGENTS.md index ef5e3f007d..3fbb472537 100644 --- a/.github/AGENTS.md +++ b/.github/AGENTS.md @@ -14,6 +14,14 @@ Apply this guidance whenever editing `.github/**`. normalized repository-relative paths, regular-file type, response path, size, encoding, and content. Do not grant generic `gh api`, arbitrary interpreters, or broad Bash merely to read PR files. +- PR label/metadata cleanup jobs that operate on pull requests must declare + `pull-requests: write`, not `issues: write`; mirror the permission shape used + by `preview-env.yml`. +- Release workflow changes must trace prepare/summary outputs and conditions + through every build and publication job: skip decisions must propagate + without accidental publishing, while backfill must reuse and validate the + existing tag and still run its required build/publication path. Verify each + actual publication job and artifact, not only the aggregate workflow result. - For workflow security changes, run the relevant raw workflow-contract tests, `python3 .github/scripts/lint-action-pinning_test.py`, `zizmor .github/workflows`, and `git diff --check`. diff --git a/.opencode/agents/pr-poller.md b/.opencode/agents/pr-poller.md index 6c61c92cd7..a629e427a7 100644 --- a/.opencode/agents/pr-poller.md +++ b/.opencode/agents/pr-poller.md @@ -1,7 +1,7 @@ --- description: Read-only DeepSeek Flash monitor for a user-authorized wait on PR CI or review updates. mode: subagent -model: deepseek/flash +model: deepseek-v4-flash temperature: 0.1 permission: task: deny @@ -21,9 +21,15 @@ Do not read source code, edit files, push, post or resolve GitHub comments, trigger workflows, fetch full CI logs, or spawn subagents. Use `scripts/pr-state --summary ` and `scripts/pr-resolve list ` as the -primary sources. Poll at a 30-second cadence for at most 20 minutes. Return -early for a failed check, merge conflict, actionable review feedback, or a -terminal clean state. At the time limit, return the pending checks and reviews. +primary sources. Poll at a 30-second cadence for at most 20 minutes. In the +default mode, return early for a failed check, merge conflict, actionable +review feedback, or a terminal clean state. If the caller says "wait N +minutes" or "then fix up", use strict-deadline mode: calculate and include the +absolute deadline in the polling prompt, accumulate findings, and do not return +early for findings, pending checks, or a clean snapshot. Stop early only if the +PR is merged/closed or access is revoked. At the deadline, return the latest +named pending checks and named actionable review findings, not only aggregate +CI and review states. Before the first GitHub request, obtain any runtime network approval required by the platform. A denied, cancelled, or interrupted approval is terminal: do not @@ -34,6 +40,7 @@ Return only: ```text PR at CI: -Reviews: +Reviews: ; findings: +Pending checks: Next action: ``` diff --git a/AGENTS.md b/AGENTS.md index ef7bd8ed2f..9fc383100c 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -20,6 +20,7 @@ apps/ - **Launcher**: all four launch modes (`dev`, `start`, `run`, `service`) go through the native Go binary at `apps/backend/bin/kandev`; the launcher package is `apps/backend/internal/launcher`. `make dev` execs a copy of that binary (`bin/kandev-launcher`). - **CLI**: `apps/cli` is the npm-published shim only (`bin/cli.js` + `bin/native-shim.js`); its tests run on the Node built-in test runner (`node --test`). - **Frontend**: Vite/React SPA (`cd apps && pnpm --filter @kandev/web dev|build:vite|lint`; for direct web typecheck use `cd apps/web && pnpm run typecheck`) +- **Web-only scripts** live in `apps/web/package.json`; run them from `apps/web` (for example `pnpm run i18n:check` or `pnpm run i18n:ratchet`) or use `pnpm --filter @kandev/web ...`, not from `apps/`. - **Desktop**: Tauri shell (`cd apps && pnpm --filter @kandev/desktop build|e2e`; Rust tests from `apps/desktop/src-tauri`) - **UI**: Shadcn components via `@kandev/ui` - **E2E**: Playwright (`cd apps/web && pnpm e2e:raw`). The `containers` project (gated on `KANDEV_E2E_CONTAINERS=1`, formerly `docker`) covers both the Docker executor and the SSH executor — anything that needs a real Docker daemon on the host lives there. See `apps/web/e2e/README.md`. @@ -60,7 +61,7 @@ Architecture notes and per-area conventions live alongside the code they describ Commits to `main` **must** follow [Conventional Commits](https://www.conventionalcommits.org/) (`type: description`). PRs are squash-merged - the PR title becomes the commit, validated by CI. Changelog is auto-generated from these via git-cliff (`cliff.toml`). See `.agents/skills/commit/SKILL.md` for allowed types and examples. -The commitlint hook caps the header at **100 characters** (`type(scope): description`). The pre-commit prettier hook also reformats staged TS/TSX files - if it does, the commit fails. Re-stage the reformatted files and create a new commit (don't `--amend`). +The commitlint hook caps the header at **100 characters** (`type(scope): description`). The pre-commit prettier hook reformats staged TS/TSX files, and the `gofmt (backend)` hook can reformat staged Go files; if either hook changes files, the commit fails. Re-stage the reformatted files and create a new commit (don't `--amend`). Before committing, use `git diff --cached --name-only | grep '\.go$' | xargs gofmt -l`; run `gofmt -w` and re-add any files it reports. ### Release & Versioning @@ -213,7 +214,7 @@ Runtime feature toggles add a SQLite-backed override tier managed through `Setti Profile selection: `KANDEV_E2E_MOCK=true` → `e2e`, `KANDEV_DEBUG_DEV_MODE=true` or `KANDEV_DEBUG_PPROF_ENABLED=true` → `dev`, otherwise `prod`. The Go dev launcher (`apps/backend/internal/launcher/dev.go`) and `apps/web/e2e/fixtures/backend.ts` set only the selector — they no longer hardcode the underlying values. -For any task that adds, rolls out, promotes, graduates, or removes a release toggle, use `/runtime-feature-flags`. That skill contains the file-by-file checklist, disabled-path requirements, test commands, promotion procedure, and retired-identity removal steps; do not rely on an agent discovering an ADR or public docs. In brief: merge risky features off in every shipped profile, enable a selected install with an admin override or explicit environment, restart and test, then change `prod:` to `"true"` for the all-user release while retaining the registry entry as a kill-switch. Remove the live flag after the feature is permanent, move its key and environment variable to the append-only retired identities in `runtimeflags/registry.go`, and never reuse either identity. Completeness tests cover the registry/profile/frontend contracts. Runtime overrides and restart support are documented in `docs/decisions/0018-runtime-settings-overrides.md` and `docs/decisions/0019-restart-supervisor.md`. +For any task that adds, rolls out, promotes, graduates, or removes a release toggle, use `/runtime-feature-flags`. That skill contains the file-by-file checklist, disabled-path requirements, test commands, promotion procedure, and retired-identity removal steps; do not rely on an agent discovering an ADR or public docs. In brief: merge risky features off in every shipped profile, enable a selected install with an admin override or explicit environment, restart when required by registry metadata, then test. Change `prod:` to `"true"` for the all-user release while retaining the registry entry as a kill-switch. Remove the live flag after the feature is permanent, move its key and environment variable to the append-only retired identities in `runtimeflags/registry.go`, and never reuse either identity. Completeness tests cover the registry/profile/frontend contracts. Runtime overrides and restart support are documented in `docs/decisions/0018-runtime-settings-overrides.md` and `docs/decisions/0019-restart-supervisor.md`. --- diff --git a/Makefile b/Makefile index f8227641e1..1164df71e5 100644 --- a/Makefile +++ b/Makefile @@ -544,6 +544,7 @@ test-scripts: @python3 scripts/opencode-code-review.test.py @python3 scripts/lint-harness-files.test.py @python3 scripts/lint-architecture.test.py + @python3 scripts/playwright-blob-audit.test.py @bash scripts/release-desktop.test.sh @bash scripts/release/runtime-bundle.test.sh @node --test apps/desktop/e2e/desktop-launch-smoke.test.mjs diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index a8195677aa..8546cd8d00 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -137,7 +137,7 @@ replace state verification, installation association, or HMAC verification. **Runtime environment invariant:** `Agent.Runtime().Env` applies to every ACP subprocess entry point. Route new overrides through host-utility probes and sessionless prompts into agentctl child processes before sanitization; cover probe DTO, prompt DTO, and child-process boundaries. -**Convention:** only `internal/agent/runtime/` (and code that pre-dates Phase 1 migration) may import `runtime/lifecycle` or `runtime/agentctl` directly. New consumers — workflow engine actions, cron-driven trigger handlers, future task-tier callers — should depend on `runtime.Runtime`. Existing call sites are migrated through later phases of task-model-unification. +**Convention:** only `internal/agent/runtime/` (and code that pre-dates Phase 1 migration) may import `runtime/lifecycle` or `runtime/agentctl` directly. New consumers — workflow engine actions, cron-driven trigger handlers, future task-tier callers — should depend on `runtime.Runtime` or narrow local interfaces for the lifecycle-owned capability they consume. Existing call sites are migrated through later phases of task-model-unification. **Lifecycle Manager** (`internal/agent/runtime/lifecycle/`) manages agent instances under the runtime: - `Manager` (`manager.go`, `manager_*.go`) - central coordinator for agent lifecycle @@ -147,13 +147,11 @@ replace state verification, installation association, or HMAC verification. - `streams.go` - WebSocket stream connections to agentctl - `process_runner.go` - agent process launch and management - `profile_resolver.go` - resolves agent profiles/settings +**Lifecycle callback identity:** Process callbacks must carry the launched PID and generation; ignore delayed callbacks from replaced processes and duplicate callbacks, with tests for both old-generation and duplicate delivery. **agentctl client** (`internal/agent/runtime/agentctl/`) is the HTTP/WS client used by the lifecycle manager to talk to a running agentctl instance. It is a runtime-tier package and should not be imported outside `internal/agent/runtime/`. -**Agent discovery vs. ACP probing:** discovery answers only whether an agent -executable is available. Authentication, protocol compatibility, and supported -models or modes belong to the ACP probe path; do not reintroduce those checks -as installation gates. +**Agent discovery vs. ACP probing:** discovery answers only whether an agent executable is available; authentication, protocol compatibility, and supported models or modes belong to the ACP probe path, not installation gates. **agentctl** is an HTTP server that: - Runs inside Docker containers or as standalone process @@ -161,10 +159,7 @@ as installation gates. - Exposes workspace operations (shell, git, files) - Supports multiple concurrent instances on different ports -Standalone agentctl is launched in its own process group so terminal Ctrl+C is -handled by the backend lifecycle manager first. Do not make standalone agentctl -share the backend's foreground process group; that bypasses supervised shutdown -and can leak ACP subprocesses. +Standalone agentctl is launched in its own process group so terminal Ctrl+C is handled by the backend lifecycle manager first; do not share the backend's foreground group, which bypasses supervised shutdown and can leak ACP subprocesses. **Executor Types** (database model): - `local_pc` - Standalone process on host @@ -174,6 +169,8 @@ and can leak ACP subprocesses. **Remote SSH executor platforms:** Treat supported remote OS/arch values as an end-to-end contract. Platform probe/normalization, lifecycle support checks, agentctl helper resolution, platform default shell, SSH readiness endpoints, frontend response types, and tests must stay aligned. Preserve raw unsupported platform details in user-facing errors, but use normalized values for supported-platform matching. Keep shell defaults platform-aware: Darwin defaults to `zsh`, Linux defaults to `bash`, unless an explicit shell is saved. +**Remote SSH lifecycle:** Resolve and run remote preparation, then verify the canonical checkout, origin, and `HEAD` before starting agentctl. Retain the remote task directory in session state for stop/resume; terminal archive/delete/cascade cleanup runs over the live SSH connection before teardown, while ordinary stop and backend shutdown preserve the workspace. + **Opt-in authentication & per-user scoping** (`internal/auth/`): auth is OFF by default; the global middleware (`auth/httpmw`, installed after CORS in `backendapp.buildHTTPServer`) then injects a synthetic admin identity for the pre-auth single user, so behavior is unchanged. Enablement is the `features.auth` runtime flag (`KANDEV_FEATURES_AUTH`, Settings > System > Feature Toggles) — the auth service derives its mode from `cfg.Features.Auth` (`disabled` / `setup` = flag-on-no-admin / `enabled` = flag-on-admin-exists); there is no separate `auth.mode` setting. When enabled, requests authenticate via a `kandev_session` cookie (opaque token, SHA-256 at rest, DB-backed) or a `kandev_pat_*` bearer token. Scoping rules: - **Identity travels in the request context** (`authn.IdentityFromContext`). No identity = internal caller (pollers, event bus, office schedulers) = unscoped. Synthetic identity = auth disabled = unscoped. - **In-session agent MCP is scoped to the task owner.** The MCP tools an agent gets inside its own session are relayed over the agent's WebSocket stream, which carries no credential of its own. `internal/mcp/scope` resolves the stream's task → workspace → owner and attaches that user's real identity before dispatch (`lifecycle.Manager.SetMCPIdentityScoper`), so the same `authorize*` checks apply as for the PAT-authenticated `/mcp` endpoint. The owning task comes from the `AgentExecution`, never from the agent-supplied payload — do not "improve" this by reading `session_id`/`task_id` out of the request. Tool handlers stay identity-agnostic. Under enforced auth every dispatch is scoped to *somebody*: a resolvable active owner gets their identity; an unowned workspace gets a sentinel user ID that reaches unowned rows only; anything unresolvable (missing task/workspace row, or an owner whose account was deleted or disabled) is **denied**. Never return an identity-free context from this path — the task service reads that as an internal caller and grants everything. @@ -182,6 +179,7 @@ and can leak ACP subprocesses. - **Two session paths bypass the task service and must guard themselves.** (1) Handlers that resolve an execution by a bare in-memory lookup (`GetExecutionBySessionID`, `*BySessionID`) skip the `GetOrEnsure*` chokepoint where the lifecycle check runs — call `service.AuthorizeSessionAccess` (see `ProcessHandlers.denySessionAccess`) or `lifecycleMgr.CheckSessionAccess` first. (2) The orchestrator reads sessions through its own repo handle, so it inherits nothing from the task service; session-keyed entry points there call `authorizeSession` / `authorizeTask`, wired by `SetSessionAccessChecker` / `SetTaskAccessChecker`. An entry point that accepts **both** a task and a session ID must use `authorizeTaskSessionPair`, never `authorizeSession` alone: the caller can satisfy a session-only check with one of their own sessions while pointing `taskID` at someone else's task, which the method then uses for its task-scoped work. Put the guard **first** in the method, before any repo/executor/agent-manager use — `TestSessionKeyedEntryPointsGuardBeforeDependencies` runs each entry point with nil dependencies and fails on a panic if a guard is placed too late. - **Dispatched WS actions have a gateway backstop.** `Client.authorizeAction` (`internal/gateway/websocket/dispatch_scope.go`) checks any payload carrying `task_id` or `session_id` against the client's identity *before* dispatch, so a newly added action is scoped by default rather than only when its author remembers. Handler-level scoping is still required (defense in depth, and it covers non-WS callers) — do not delete a service-layer check because the backstop exists. The backstop reads `task_id`, `session_id` and `task_environment_id`, plus `id` on **top-level `task.` actions only** (`task.state`, `task.move`, `task.get`, … — `task.` and exactly one more segment). Keep using those names for task-scoped actions, because an action that invents a new name for the same kind of resource silently opts out. (`user_shell.stop` was exactly that: it keys off `task_environment_id` with `task_id` optional, so a `task_id`-only backstop missed it. `task.state` and `task.move` were the second instance: they name the task `id`, so the backstop parsed no refs and let one user mutate any other user's task — hence the `id` rule.) Deeper `task.*` namespaces are deliberately excluded: in `task.plan.revision.get` and `task.review.finding.update` the `id` names a revision or a finding, so those actions must carry `task_id` when they are task-scoped. Also avoid permissive types (`any`, `json.RawMessage`) for those fields: a payload that fails to unmarshal into the ref struct is treated as naming nothing. - **WS**: clients carry their identity; dispatched actions and subscriptions are scoped; workspace-carrying events route via `Hub.BroadcastToWorkspace`. A new `hub.Broadcast` (global) call site needs a `//ws:global` justification comment. +- **Task overview vs. session detail:** Keep cross-task state in the bounded, persisted `TaskStatusSummary` projection and `task.status_summary.updated`; keep unbounded transcript and execution data session-scoped. The [bounded task-status spec](../../docs/specs/platform/bounded-task-status-delivery.md) and [accepted ADR](../../docs/decisions/2026-08-01-separate-task-summary-session-stream-traffic.md) define the shared contract. - **Self-authenticating webhooks** (automation, office channels, plugin webhooks) and `/health`, `/api/v1/features`, `/api/v1/app-state` stay public — the allowlist lives in `auth/httpmw/middleware.go` with a pinning test. ## Execution Flow @@ -207,10 +205,15 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update **Executor default scripts:** Default prepare scripts are in `internal/agent/runtime/lifecycle/default_scripts.go`; `internal/scriptengine/` handles placeholder resolution. +- **Launcher quoting:** Quote interpolated environment values for POSIX/Git Bash + versus native Windows; when launch recipes change, extend + `scripts/check-make-shells` with default, spaced, and quote-containing values. + ## Conventions - Provider pattern for DI; stderr for logs, stdout for ACP only. - Pass context through chains; event bus for cross-component comm. +- **Dependency direction:** Shared admission limits used by task models and orchestrator/messagequeue belong in a neutral internal package; task/models must not import the higher-level orchestrator/messagequeue package. - Production Git commands must use `subproc.NewGitCommand` with a classified `subproc.RunGit*` helper, or hold a classified admission slot across streaming `Start`/`Wait`. Do not construct raw Git commands outside `internal/common/subproc`; choose `interactive`, `lifecycle`, or `background`. - **Cross-tier shared code belongs in `internal/common/`.** agentctl ships as a standalone binary uploaded into containers, so neither it nor the backend may own code the other imports — put shared logic in a neutral `internal/common/*` package instead of importing across the boundary or forking a copy (`ptyexec` is the worked example; `subproc`, `gitref`, `securityutil` are older ones). Duplicating to "avoid the dependency" is how the two PTY copies drifted. - **Pure computation that only matters on Windows should not carry `//go:build windows`.** One CI job runs on Windows and it tests a package allowlist, so tagged code is easily unverified — loginpty's copy of the Win32 quoting was compiled by no job at all. Keep platform API calls behind the tag, leave string/path helpers untagged, and add packages needing native coverage to the `test-windows` job in `.github/workflows/backend-tests.yml`. @@ -218,31 +221,26 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - **Repository provider identity:** Provider-backed repositories are keyed by workspace, provider, normalized `provider_host` origin, full owner/namespace, and name. Persist `provider_host` when importing or resolving a remote; do not infer self-managed GitLab rows from owner/name alone. Legacy rows with an empty host have unknown identity and must fail closed for provider write/link operations. - **PR status sync coverage:** A `github_pr_watches` row is unique per (session, repository, branch). A watch that has **found a PR** is never re-pointed: it is that PR's only sync handle, and both the poller and the on-demand sync iterate watches, so re-pointing it froze the PR at its last-observed checks/review state — which the UI renders as an open, mergeable PR long after it merged, and which silently disabled CI auto-fix and auto-merge for every branch but the one currently checked out. Branch switches (`resetPRWatchForBranchSwitch`) and the git-status branch sync (`syncPRWatchBranch`) therefore only re-point a **searching** watch (`pr_number=0`) of the same repository, and otherwise add a watch for the new branch — one searching watch per (session, repository) plus one per PR actually opened. Watches are still not a complete index of a task's PRs (sessions get deleted, PRs get linked by URL), so the orphan sweep stays. `service_pr_unwatched.go` reconciles the `github_task_prs` rows no watch points at, and deliberately writes **lifecycle fields only** (`state` / `merged_at` / `closed_at`) — check and review aggregates belong to the watch-driven sync that actually fetches reviews and check runs, and the per-PR REST status marks its counts populated even with nothing to count, so widening this path to aggregates persists 0/0 over good data (it broke six PR-popover e2e specs exactly that way). Terminal and detached rows are excluded so the set stays bounded. **The backend is the only writer of `github_task_prs`:** the PR-feedback read makes the same upstream calls the status sync does (`GetPR` + `ListPRReviews` + `ListCheckRuns`, plus comments), so it persists through `SyncTaskPR` (`service_pr_feedback_sync.go`) and lets `github.task_pr.updated` converge every surface. The frontend must never patch PR lifecycle fields into its own store copy — that write cannot reach the DB, so it desyncs the panel from the kanban card and evaporates on reload (it also masked the sync hole above for months). Both derivations share `newPRStatus` so they cannot drift; the persist rides *inside* the TTL cache's fetch closure, so a cache hit costs no write; failures are logged and swallowed because this is a read endpoint; and the row lookup is workspace-scoped (`ListTaskPRsByPRNumber`) to match the credential scope the read was authorized under. Finally, `merged` is the one state `SyncTaskPR` refuses to leave (`resolveTerminalMergeState`) since GitHub cannot un-merge a PR — do **not** generalize that into a `merged > closed > open` rank, because `closed -> open` is a real transition (a PR can be reopened) and ranking it as a regression pins reopened PRs closed forever. - **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). +- **Generated-title ownership:** Claim generated-title ownership only after task, config, and Office eligibility is known. Config tasks, Office/External modes, and workspace-only preparation (`StartAgent: false`) must never claim it; only an eligible agent-start task may claim the generated title. - **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. **Workflow steps** follow the same rule with their own publisher: step create/update/delete publish `workflow_step.created` / `.updated` / `.deleted` through `internal/workflow/stepevents.Publisher` (the WS gateway fans them out; the orchestrator re-evaluates queue admission off `.updated`). Both mutation surfaces — REST/WS in `internal/workflow/handlers`, MCP in `internal/mcp/handlers` — share that publisher and differ only in the source label they pass, so a new step-mutation path uses it rather than hand-rolling the payload; promoting a start step demotes the previous one, so also publish `.updated` for every entry in `DemotedStartSteps`. +- **Workspace deletion side tables:** Any workspace-scoped integration side table without a database foreign key/cascade must be deleted explicitly from its `WorkspaceDeleted` handler. Cover the create → delete lifecycle in a test, and include the same cleanup in E2E reset fixtures; do not assume deleting the workspace row removes orphaned integration metadata. - **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. - - **Path/security tests:** Avoid using the real filesystem root as a fixture root. Build fake absolute roots under `t.TempDir()` with `filepath.Join`; this keeps tests portable across Windows, POSIX, and privileged cloud executors. + - **Path/security tests:** Avoid using the real filesystem root as a fixture root. Build fake absolute roots under `t.TempDir()` with `filepath.Join`; this keeps tests portable across Windows, POSIX, and privileged cloud executors. For error assertions, do not compare raw `%q`/escaped strings with native `filepath` values; assert typed fields or normalize both sides with `filepath.Clean`/`filepath.FromSlash`. - **Filesystem permission tests:** Assert permission-denied behavior only after probing that the current executor enforces the permission bit change. Root-like Sprite executors may bypass `chmod` restrictions. - **Filesystem safety checks:** Carry the original `os.Lstat` `FileInfo` (or opened handle) through every decision; do not re-stat a validated path, which reopens a TOCTOU window before side effects. Test mismatches before writes or manager commands. - - **Git indexed-environment tests:** When a test supplies `GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_n`, - and `GIT_CONFIG_VALUE_n`, clear every inherited indexed key first and restore it with - `t.Cleanup`. `internal/gitconfigenv` ignores indexes past the count the way Git does, so an - uncleared parent silently changes what the block contains instead of failing loudly. + - **Git indexed-environment tests:** When a test supplies `GIT_CONFIG_COUNT`, `GIT_CONFIG_KEY_n`, and `GIT_CONFIG_VALUE_n`, clear every inherited indexed key first and restore it with `t.Cleanup`. `internal/gitconfigenv` ignores indexes past the count; uncleared parent indexes silently alter the block instead of failing loudly. + - **Derived environment contracts:** Drive derived values through the production producer/wiring seam instead of manually seeding keys; pair consumer/process coverage with a producer-boundary assertion so missing publication fails. - **Full test output:** For local full-suite pass/fail validation, prefer plain `go test -race ./...`. `go test -json ./...` can emit very large JSONL streams; if a wrapper or tracing tool truncates the stream mid-record, downstream JSON parsing may fail even when Go tests passed. Use JSON output mainly for CI artifacts or test-report tooling that explicitly requires it. - - **Private executable helpers:** When library code starts `os.Executable()` in - a private helper mode, do not rely only on a command package's dispatch or - package-specific `TestMain`: an importing package's test binary can be the - executable and recursively run tests. Use either an explicitly marked - private env-and-argument trampoline that runs before normal test entry, or - inject a dedicated helper executable. Cover it from a different importing - package, not only the helper package. + - **Private executable helpers:** When library code starts `os.Executable()` in a private helper mode, do not rely only on a command package's dispatch or package-specific `TestMain`: an importing package's test binary can be the executable and recursively run tests. + Use either an explicitly marked private env-and-argument trampoline before normal test entry, or inject a dedicated helper executable. Cover it from a different importing package, not only the helper package. ### Goroutine ownership and leak testing Every long-running goroutine must have a single owner with explicit start and stop semantics: -- **Lifecycle:** the type that spawns the goroutine also exposes `Start(ctx)` / `Stop()` (or equivalent). `Start` registers on a `sync.WaitGroup`; `Stop` cancels the goroutine's context (or closes a `stopCh`) and `wg.Wait()`s for drain. Idempotent on both ends. `internal/integrations/healthpoll`, `internal/jira`, `internal/linear`, and `internal/github` pollers are the canonical shape. +- **Lifecycle:** the type that spawns the goroutine also exposes `Start(ctx)` / `Stop()` (or equivalent). `Start` registers on a `sync.WaitGroup`; `Stop` cancels the goroutine's context (or closes a `stopCh`) and `wg.Wait()`s for drain. Idempotent on both ends. Restartable services must reset stopped state and create a fresh cancellable context/cancel under the same mutex used by `Stop`; cover `Stop` → `Start`. `internal/integrations/healthpoll`, `internal/jira`, `internal/linear`, and `internal/github` pollers are the canonical shape. - **E2E reset invariant:** `seedData`/backend are worker-scoped, so any workspace-scoped state a global poller reads (for example `github_review_watches`) must be deleted in `cmd/kandev/e2e_reset.go` before task deletion — otherwise the poller recreates rows mid-reset and later tests see duplicates. Add a `Delete...ByWorkspace` cascade when introducing a new poller-backed entity. - **Cancellation:** the goroutine selects on `ctx.Done()` (or `stopCh`) in every long wait. Never use `time.Sleep` in a retry/backoff loop — use `time.NewTimer` inside a `select` that also watches the shutdown signal (see `lifecycle.StreamManager.sleepOrStop`). - **Detached helpers:** event handlers and short-lived `go func()` calls in `internal/orchestrator/` and `internal/agent/runtime/lifecycle/` must accept a cancellable context (or check the owning type's shutdown signal) and return promptly when it fires. @@ -276,11 +274,13 @@ Built-in prompt content refreshes are seed-data migrations, not schema migration Use `i18n.T`/`i18n.Tf` with explicit locale threading (including interpolation/plurals); resolve artifact locale at creation. Catalogs are embedded in `internal/i18n/locales/`; regenerate `pseudo` with `pnpm run i18n:pseudo`. Prefer stable error codes for new output so the frontend translates it. See `docs/i18n.md` and ADR `2026-08-01-share-artifact-locale.md`. +**Table-rebuild migrations:** When a legacy or constraint migration recreates a table, mirror every new column in the replacement `CREATE TABLE` and `INSERT ... SELECT` copy list; add a replay regression test proving values, including timestamps, survive. **Destructive cutover migrations:** Build a legacy schema with `NewWithDB` on a fresh database, replace final-shaped tables with legacy-shaped ones, inject a test-only failpoint after each cutover step, and assert byte-equivalent rollback; run the same matrix with `KANDEV_TEST_POSTGRES_DSN`, looking up PostgreSQL constraint names dynamically because they truncate at 63 bytes. + ## Code-quality limits Enforced by `apps/backend/.golangci.yml` (errors on new code only): - Functions: ≤80 lines, ≤50 statements · Cyclomatic complexity: ≤15 · Cognitive complexity: ≤30 -- Nesting depth: ≤5 · Naked returns only in functions ≤30 lines · No duplicated blocks (≥150 tokens) · Repeated strings → constants (≥3 occurrences) +- Nesting depth: ≤5 · Naked returns only in functions ≤30 lines · No duplicated blocks (≥150 tokens) · Repeated strings → constants (≥3 occurrences) · Revive's 800-effective-line file limit also applies to test files; put new tests in a new file instead of appending to an already-large test file. When you hit a limit, extract a helper function. Prefer composition over growing a single function. diff --git a/apps/backend/cmd/mock-agent/AGENTS.md b/apps/backend/cmd/mock-agent/AGENTS.md index fc29c2b0d6..bf4bd4f9c2 100644 --- a/apps/backend/cmd/mock-agent/AGENTS.md +++ b/apps/backend/cmd/mock-agent/AGENTS.md @@ -8,7 +8,13 @@ Scoped guidance for `apps/backend/cmd/mock-agent/`. The mock agent satisfies the `handler.go` routes any prompt starting with `/e2e:` to `emitPredefinedScenario(e, name)`, which looks `name` up in `scenarioRegistry` (`scenarios.go`). The registry is the single source of truth for scenario names — to add a scenario, add one map entry and one `scenario(e *emitter)` function. -Friendly aliases live in `handler.go` next to the dispatcher: `/ask-single`, `/ask-multiple`, `/crash`, `/todo`, `/mermaid`, `/markdown`, `/sleep [n]`, `/tool:`, `/subagent`, `/subtask`, `/bulk[:N]`, `/background [n]`, `/detached-background [n]`, `/async-subagent-lifecycle [n]`, `/async-subagent-teardown`. +Friendly aliases live in `handler.go` next to the dispatcher: `/ask-single`, `/ask-multiple`, `/crash`, `/todo`, `/mermaid`, `/markdown`, `/sleep [n]`, `/slow [duration]`, `/tool:`, `/subagent`, `/subtask`, `/bulk[:N]`, `/background [n]`, `/detached-background [n]`, `/async-subagent-lifecycle [n]`, `/async-subagent-teardown`. + +`/slow [duration]` uses seconds for positive bare numbers and accepts explicit +Go duration units. Omitted, invalid, or non-positive values use the 5-second +fallback. Handler-path tests should use a short explicit duration such as +`50ms`; parser tests should cover long bare values without making the handler +wait for seconds. The four `background`/`async-subagent` aliases emit the foreground-yield and detached-workload frame shapes behind the fine-grained busy signal (ADR-0049): `/background` spawns a subagent while the foreground goes idle, `/detached-background` launches work that outlives the turn, and the `async-subagent-*` pair replays Claude's async Agent lifecycle with and without its completion frame. diff --git a/apps/backend/internal/agentctl/AGENTS.md b/apps/backend/internal/agentctl/AGENTS.md index ced5a88ea8..d7719e4cd3 100644 --- a/apps/backend/internal/agentctl/AGENTS.md +++ b/apps/backend/internal/agentctl/AGENTS.md @@ -15,6 +15,13 @@ agentctl exposes these route groups (see `server/api/`): - `/workspace/*` - File operations, search, tree - `/vscode/*` - VS Code integration proxy +## Multi-repository Git and review payloads + +Preserve the repository-scoped fields `repository_name`, `base_ref`, and +`is_submodule` in aggregate results; nested submodule comparisons stay anchored +to the parent gitlink. The focused contract tests are in +`server/api/git_multi_repo_review_test.go`. + ## Pull request creation (`server/process/git_pr_providers.go`) `GitOperator.CreatePR` picks a host CLI from `origin`: diff --git a/apps/package.json b/apps/package.json index 44a404c0b1..c308ce3ec3 100644 --- a/apps/package.json +++ b/apps/package.json @@ -20,6 +20,7 @@ "devDependencies": { "@commitlint/cli": "^20.4.2", "@commitlint/config-conventional": "^20.4.2", + "@playwright/cli": "0.1.17", "@types/node": "^20", "knip": "6.31.0", "prettier": "^3.6.2", diff --git a/apps/pnpm-lock.yaml b/apps/pnpm-lock.yaml index 8f9ed59504..28e21d53b5 100644 --- a/apps/pnpm-lock.yaml +++ b/apps/pnpm-lock.yaml @@ -50,6 +50,9 @@ importers: '@commitlint/config-conventional': specifier: ^20.4.2 version: 20.4.2 + '@playwright/cli': + specifier: 0.1.17 + version: 0.1.17 '@types/node': specifier: ^20 version: 20.19.28 @@ -1777,6 +1780,11 @@ packages: resolution: {integrity: sha512-1j/H/fECBuc9dEvntdWI+l435HZapw+RCJTlqCA6BboQ5TjlnE005j/ROWutXIs8aq5OAc82JI2Kwk4A1WWBgw==} engines: {vscode: ^1.0.0} + '@playwright/cli@0.1.17': + resolution: {integrity: sha512-VBw6y3p8eqOqmjKg07IkWSPGKJkpIhMRNDFI6DOYsDD6fAfcI1XYEWMLWyhSZQ0B/Oc2KN49eq4XqE64PUPHBg==} + engines: {node: '>=18'} + hasBin: true + '@playwright/test@1.61.1': resolution: {integrity: sha512-8nKv6+0RJSL9FE4jYOEGXnPeM/Hg12qZpmqzZjRh3qM0Y7c3z1mrOTfFLids72RDQYVh9WpLEfR5WdpNX4fkig==} engines: {node: '>=18'} @@ -5544,11 +5552,21 @@ packages: engines: {node: '>=18'} hasBin: true + playwright-core@1.62.0-alpha-1783623505000: + resolution: {integrity: sha512-CPJZdsA/KGT2QQlekiV6Wt+QlQrZHVSZ6oiNtOI/bYYOIVLM8jfKGWTM4zQiyd4UN+40Cq4cA6lxmZHZbtPvJQ==} + engines: {node: '>=20'} + hasBin: true + playwright@1.61.1: resolution: {integrity: sha512-DWnY5o3YbLWK4GovuAVwpqL+1VwGNdUGrRr++8j8PtQQzvAVZUIMjKQ90fY689sEJZJBbZVw1rXaOKSTitkzPQ==} engines: {node: '>=18'} hasBin: true + playwright@1.62.0-alpha-1783623505000: + resolution: {integrity: sha512-6KV9h4PP3hqu4NaGdxxcijWfYh9LJcFI/R2sP4TTC4I5cFo3oRawN0ETlW5MkE3cQEgKhhoj0KUNz4sfpCT0Tg==} + engines: {node: '>=20'} + hasBin: true + points-on-curve@0.2.0: resolution: {integrity: sha512-0mYKnYYe9ZcqMCWhUjItv/oHjvgEsfKvnUTg8sAtnHr3GVy7rGkXCb6d5cSyqrWqL4k81b9CPg3urd+T7aop3A==} @@ -7829,6 +7847,11 @@ snapshots: '@pierre/theme@0.0.28': {} + '@playwright/cli@0.1.17': + dependencies: + playwright: 1.62.0-alpha-1783623505000 + playwright-core: 1.62.0-alpha-1783623505000 + '@playwright/test@1.61.1': dependencies: playwright: 1.61.1 @@ -11871,12 +11894,20 @@ snapshots: playwright-core@1.61.1: {} + playwright-core@1.62.0-alpha-1783623505000: {} + playwright@1.61.1: dependencies: playwright-core: 1.61.1 optionalDependencies: fsevents: 2.3.2 + playwright@1.62.0-alpha-1783623505000: + dependencies: + playwright-core: 1.62.0-alpha-1783623505000 + optionalDependencies: + fsevents: 2.3.2 + points-on-curve@0.2.0: {} points-on-path@0.2.1: diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index 26693f6022..caa12b68b3 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -40,6 +40,10 @@ Go Boot Payload -> Hydrate Store -> Components Read Store -> Hooks Subscribe **Never fetch data directly in components.** +### Browser capability boundaries + +- Use `generateUUID()` for client-only non-security IDs, not `crypto.randomUUID()`; use `copyToClipboard()` for copy actions, not `navigator.clipboard.writeText()`. Keep fallbacks non-security; test missing or rejected capabilities with an `rg` audit. + ## Store Structure (Domain Slices) ```text @@ -88,6 +92,11 @@ For rebasing or finishing PRs written against the old Next.js runtime, follow [` Use subscription hooks only; the WS client auto-deduplicates. +**Task overview vs. session detail:** Shared task rows read `Task.statusSummary` and `task.status_summary.updated`; rich streams stay session-detail-only. Extend the bounded projection per the [spec](../../docs/specs/platform/bounded-task-status-delivery.md) and [ADR](../../docs/decisions/2026-08-01-separate-task-summary-session-stream-traffic.md). + +**Branch-scoped task state:** For live worktree/session state plus `task_prs`, key by `(repository, checked-out branch)`, not task/repository alone. `branch_switched` invalidates prior status/commits; reject late results with a generation/identity guard and preserve siblings. Historical PRs affect Changes only when `repository_id` and normalized `head_branch` match; Review/PR history may still show them. Test single/multi-repo cases and desktop/mobile Changes behavior. +**HTTP/WS cache races:** When HTTP hydrates a cache also updated by WebSockets, guard responses with per-scope revision and request/workspace generation; discard or refresh stale responses and cover deferred responses. `useEnsureTaskSession` re-fires `session.ensure` when an open task page sees zero sessions after a prior ensure, so deleting the last session from that page spawns a replacement; test zero-session states through the backend/API instead. + When changing task lifecycle WS handlers (`task.updated`, `task.deleted`, `task.state_changed`), check both kanban and Office surfaces. Archive/delete events may need to update kanban caches, `tasks.activeTaskId` / session pin @@ -153,33 +162,24 @@ surface. with independent open state. Touch-pinned help must close on a second trigger tap, outside interaction, and Escape; verify desktop pointer and mobile-sized touch flows. -- **Renaming a `data-testid`:** set the new id as `data-testid=""` and keep the old id as - `data-legacy-testid=""`, then migrate e2e specs to the new id in the same PR. JSX rejects two - `data-testid` attributes on one element, and Playwright's `getByTestId` only matches one attribute - name — the `data-legacy-testid` alias lets existing specs keep selecting the element mid-migration. -- **Dockview session panel activation:** session chat panels can become active through tab - pointer/keyboard events, global tab-cycling shortcuts, reopen/menu actions, and Dockview close - controls. When changing `tasks.activeSessionId` or active-session sync, audit all of those paths. - Use store state in addition to Dockview `api.isActive`; the current session's chat tab may be - Dockview-inactive while Files/Changes is active. Same-session clicks must not leave stale - activation intent, and Dockview `.dv-default-tab-action` close controls should be treated as - close/delete actions rather than session-switch intent. -- **Conditional review-panel ownership:** the reusable `pr-detail` panel may be - visible only while the active task has a linked PR or MR. A custom Default - layout's canonical panel supplies the preferred group and tab index; it does - not make an empty tab persistent. After review data hydrates, review loss - removes any canonical panel regardless of how it entered the runtime layout. - Restoration, maximized layouts, and a session's offered/dismissed marker - defer or suppress automatic insertion; linked existing panels synchronize - provider and review identity without moving them. -- **GitHub PR status UI:** visual PR/CI status surfaces should use the shared helpers in - `apps/web/components/github/pr-task-icon.tsx` (`hasPRChecksPassedForDisplay`, - `hasPRChecksInProgressForDisplay`, `hasPRChecksPassedWithoutReviewWaitForDisplay`) instead of - re-deriving status from `checks_state`, `checks_total`, or `checks_passing` locally. Aggregate - check counts are a display-only fallback when `checks_state` is empty; they may make chips or task - icons render passed/in-progress, but must not enable merge actions. Merge readiness must use - `isPRReadyToMerge`, which requires GitHub's explicit `checks_state === "success"` rollup. When - changing PR status behavior, update both `pr-task-icon.test.ts` and `pr-status-chip.test.tsx`. +- **Renaming a `data-testid`:** use `data-legacy-testid` for the old id while + migrating specs; JSX and Playwright only support one `data-testid` attribute. +- **Dockview session activation:** audit pointer/keyboard tabs, shortcuts, + reopen/menu actions, and close controls; combine store state with + `api.isActive`, clear same-session intent, and treat default-tab close as + delete rather than session switching. +- **Conditional review panels:** show `pr-detail` only for active tasks with a + linked PR/MR; default layouts only provide preferred placement. Hydrated review + loss removes canonical panels, while restoration/maximized and offered/dismissed + markers suppress insertion; existing panels sync identity without moving. +- **Dockview environment switching:** reconcile ephemeral panels before restoring views; + correlate ID-less groups by stable ID or position. Treat `chat`/`session:*` as semantic only with a non-null `activeSessionId`. +- **GitHub PR status UI:** use the shared `pr-task-icon.tsx` display helpers and + `isPRReadyToMerge`; aggregate counts are display-only and cannot enable merges. + Update `pr-task-icon.test.ts` and `pr-status-chip.test.tsx` with behavior changes. +- **GitHub PR associations:** retain terminal/merged siblings for tabs/unlink; + derive `openPRs` only for aggregate status/automation and test desktop/mobile + terminal unlink plus two-to-one collapse/focus. - **Task repository labels:** user-facing task/card repo chips should display a stable repo slug or name (`owner/repo` when known, otherwise the repo name), not a local filesystem path. Local clone paths or folder paths belong in diff --git a/apps/web/components/review/AGENTS.md b/apps/web/components/review/AGENTS.md new file mode 100644 index 0000000000..093f898b2d --- /dev/null +++ b/apps/web/components/review/AGENTS.md @@ -0,0 +1,7 @@ +# Review components + +Review file identity is repository-scoped, not path-scoped: preserve +`repository_name` and use `reviewFileKey` or an equivalent composite identity. +Preserve explicit `base_ref` and `is_submodule` metadata; nested submodule +changes remain anchored to the parent gitlink. The backend contract tests are +in `apps/backend/internal/agentctl/server/api/git_multi_repo_review_test.go`. diff --git a/apps/web/components/task/chat/AGENTS.md b/apps/web/components/task/chat/AGENTS.md new file mode 100644 index 0000000000..0db72db9ab --- /dev/null +++ b/apps/web/components/task/chat/AGENTS.md @@ -0,0 +1,7 @@ +# Chat components + +Before adding a semantic predicate for chat or tool output, search `types.ts` +and existing shared helpers. Keep reusable domain predicates, including +projected shell-output presence, in `types.ts`; reuse them in rows and groups +instead of copying equivalent logic. Add focused tests for the shared predicate +and its consumers. diff --git a/apps/web/e2e/README.md b/apps/web/e2e/README.md index 084528bf93..6fe614132a 100644 --- a/apps/web/e2e/README.md +++ b/apps/web/e2e/README.md @@ -77,6 +77,10 @@ Or a single spec: KANDEV_E2E_CONTAINERS=1 pnpm e2e:raw --project=containers tests/ssh/launch-task.spec.ts ``` +### Remote-executor fixture contracts + +Host-only `file://` fixtures are not reachable from an SSH or Docker target. Use a disposable provider-shaped HTTP Git fixture with a target-side URL rewrite; if the spec also uses host-local `GitHelper` or LSP paths, materialize a local clone at the expected temporary path. Verify both the remote checkout and every host-local fixture consumer. + ### Why "containers" instead of "docker"? This project used to be named `docker`. It was renamed to `containers` once SSH e2e tests joined it — calling it `docker` was misleading because SSH tests have nothing to do with the Docker _executor_; they just happen to use Docker as the runtime that hosts the sshd target. @@ -122,6 +126,8 @@ Why a script instead of raw `docker run`: in docker mode it builds the CGO/`fts5 > > This used to break when e2e was launched from a shell that had inherited `KANDEV_FEATURES_OFFICE=false` (e.g. from a host kandev backend running the prod profile): `profiles.ApplyProfile` only sets vars that are **unset** (so launchers/shells win — see `docs/decisions/0007-runtime-feature-flags.md`), and the fixture spreads `process.env` into the spawned backend, so the stale prod value won and 404'd every office spec. Fixed at the source: `sanitizeInheritedEnv` in `e2e/fixtures/backend.ts` strips all inherited `KANDEV_FEATURES_*` before spawn, so the e2e profile — not whatever the host exported — decides feature flags. No `unset` needed. +> **Profile-managed environment variables:** when adding an environment variable with an `e2e:` or `dev:` profile default, add it to `sanitizeInheritedEnv` in `e2e/fixtures/backend.ts` so inherited shell/task values cannot override the selected profile. Keep explicit `backend.restart({ ... })` overrides applied after baseline sanitization, so a spec can still opt into a deliberate per-test value. + > **Host oversubscription:** running >=5 heavy shards concurrently on one machine (each = Go backend + Vite-served SPA assets + Chromium + mock agent) starves CPU/IO and induces timing flakes that CI's isolated runners never see. Use 2-3 concurrent shards locally for a clean signal; see "flake triage" in the `/e2e` skill. ## Backend isolation per worker diff --git a/apps/web/e2e/helpers/session.ts b/apps/web/e2e/helpers/session.ts index e17b5276f0..a868ff37ea 100644 --- a/apps/web/e2e/helpers/session.ts +++ b/apps/web/e2e/helpers/session.ts @@ -56,6 +56,27 @@ export async function waitForSessionDone( .toBe(true); } +export async function waitForSessionState( + apiClient: ApiClient, + options: { + taskId: string; + sessionId: string; + expectedState: string; + message: string; + timeout?: number; + }, +): Promise { + await expect + .poll( + async () => { + const { sessions } = await apiClient.listTaskSessions(options.taskId); + return sessions.find((session) => session.id === options.sessionId)?.state ?? null; + }, + { timeout: options.timeout ?? 120_000, message: options.message }, + ) + .toBe(options.expectedState); +} + export async function waitForSessionEnvironment( apiClient: ApiClient, options: { diff --git a/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts b/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts index 8d1aec2d48..25ba8ecf5d 100644 --- a/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts +++ b/apps/web/e2e/tests/chat/agent-message-attribution.spec.ts @@ -2,6 +2,7 @@ import { type Page, type Locator } from "@playwright/test"; import { test, expect } from "../../fixtures/test-base"; import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; +import { waitForSessionState } from "../../helpers/session"; import { SessionPage } from "../../pages/session-page"; /** @@ -33,27 +34,6 @@ function senderBadge(session: SessionPage): Locator { return session.chat.locator("[data-testid='sender-task-badge']"); } -/** Wait for the target session to enter the state where a cross-task prompt - * takes the synchronous path instead of being queued. The mock agent emits - * its text reply before this state transition, so message content alone is - * not a reliable readiness signal. */ -async function waitForTargetIdle( - apiClient: ApiClient, - taskId: string, - sessionId: string, - timeoutMs = 90_000, -): Promise { - await expect - .poll( - async () => { - const { sessions } = await apiClient.listTaskSessions(taskId); - return sessions.find((session) => session.id === sessionId)?.state; - }, - { timeout: timeoutMs, message: `Target session ${sessionId} did not reach idle` }, - ) - .toBe("WAITING_FOR_INPUT"); -} - /** Wait for at least one user message with sender metadata to appear in the * receiving session. Returns the matching message; throws on timeout. */ async function waitForCrossTaskMessage( @@ -275,7 +255,15 @@ test.describe("Cross-task agent message attribution", () => { // its message — this exercises the default (record + prompt) branch // rather than the queue path. const session = await openTask(testPage, target.id); - await waitForTargetIdle(apiClient, target.id, target.sessionId); + await waitForSessionState(apiClient, { + taskId: target.id, + sessionId: target.sessionId, + expectedState: "WAITING_FOR_INPUT", + message: "idle target must be ready before the prompt-path follow-up", + timeout: 90_000, + }); + // Keep the visible response assertion as secondary evidence: the mock + // agent can persist it before the lifecycle state transition completes. await expect(session.chat).toContainText("ready for instructions", { timeout: 30_000 }); await createSenderTaskingTarget( @@ -380,7 +368,13 @@ test.describe("Cross-task agent message attribution", () => { test("badge link points to the sender task", async ({ testPage, apiClient, seedData }) => { const target = await createIdleTarget(apiClient, seedData, "Target — link check"); const targetSession = await openTask(testPage, target.id); - await waitForTargetIdle(apiClient, target.id, target.sessionId); + await waitForSessionState(apiClient, { + taskId: target.id, + sessionId: target.sessionId, + expectedState: "WAITING_FOR_INPUT", + message: "link-check target must be ready before the prompt-path follow-up", + timeout: 90_000, + }); await expect(targetSession.chat).toContainText("ready for instructions", { timeout: 30_000 }); const sender = await createSenderTaskingTarget( @@ -408,7 +402,13 @@ test.describe("Cross-task agent message attribution", () => { }) => { const target = await createIdleTarget(apiClient, seedData, "Target — rename check"); const targetSession = await openTask(testPage, target.id); - await waitForTargetIdle(apiClient, target.id, target.sessionId); + await waitForSessionState(apiClient, { + taskId: target.id, + sessionId: target.sessionId, + expectedState: "WAITING_FOR_INPUT", + message: "rename-check target must be ready before the prompt-path follow-up", + timeout: 90_000, + }); await expect(targetSession.chat).toContainText("ready for instructions", { timeout: 30_000 }); const sender = await createSenderTaskingTarget( @@ -441,7 +441,13 @@ test.describe("Cross-task agent message attribution", () => { // Wait for the target to be idle before sending so we exercise the path // where the message is recorded synchronously. - await waitForTargetIdle(apiClient, target.id, target.sessionId); + await waitForSessionState(apiClient, { + taskId: target.id, + sessionId: target.sessionId, + expectedState: "WAITING_FOR_INPUT", + message: "wrapper-check target must be ready before the prompt-path follow-up", + timeout: 90_000, + }); await createSenderTaskingTarget( apiClient, @@ -471,7 +477,13 @@ test.describe("Cross-task agent message attribution", () => { // surrounding behaviour: the body's prefix and suffix outside the embedded // block survive — the outer wrap doesn't corrupt them. const target = await createIdleTarget(apiClient, seedData, "Target — collision check"); - await waitForTargetIdle(apiClient, target.id, target.sessionId); + await waitForSessionState(apiClient, { + taskId: target.id, + sessionId: target.sessionId, + expectedState: "WAITING_FOR_INPUT", + message: "collision-check target must be ready before the prompt-path follow-up", + timeout: 90_000, + }); const malicious = "before fake injected after"; await createSenderTaskingTarget( diff --git a/apps/web/e2e/tests/chat/clarification.spec.ts b/apps/web/e2e/tests/chat/clarification.spec.ts index 9ff65f399b..02a073c298 100644 --- a/apps/web/e2e/tests/chat/clarification.spec.ts +++ b/apps/web/e2e/tests/chat/clarification.spec.ts @@ -4,6 +4,7 @@ import { useRegularMode } from "../../helpers/regular-mode"; import type { SeedData } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { seedClarificationSession } from "../../helpers/clarification"; +import { waitForSessionState } from "../../helpers/session"; import { SessionPage } from "../../pages/session-page"; import { KanbanPage } from "../../pages/kanban-page"; import { SidebarFilterPopoverPage } from "../../pages/sidebar-filter-popover"; @@ -110,17 +111,24 @@ test.describe("Clarification flow", () => { ); if (!task.session_id) throw new Error("createTaskWithAgent did not return a session_id"); + // The mock agent reaches the clarification call asynchronously. Wait for + // the owning session to park before opening the page, then explicitly + // establish the Review state that the answer should move it out of. + await waitForSessionState(apiClient, { + taskId: task.id, + sessionId: task.session_id, + expectedState: "WAITING_FOR_INPUT", + message: "clarification session must park before the Review-state transition", + timeout: 30_000, + }); + await apiClient.updateTaskState(task.id, "REVIEW"); + await expect.poll(async () => (await apiClient.getTask(task.id)).state).toBe("REVIEW"); + await testPage.goto(`/t/${task.id}`); const session = new SessionPage(testPage); await session.waitForLoad(); await expect(session.clarificationOverlay()).toBeVisible({ timeout: 30_000 }); - // The overlay is the durable pending-question precondition. Depending on - // whether the primary MCP response path is still connected, the session - // may correctly remain RUNNING or park in WAITING_FOR_INPUT. - await apiClient.updateTaskState(task.id, "REVIEW"); - await expect.poll(async () => (await apiClient.getTask(task.id)).state).toBe("REVIEW"); - const filters = new SidebarFilterPopoverPage(testPage); await filters.open(); await filters.setGroup("State"); @@ -262,11 +270,13 @@ test.describe("Clarification flow", () => { }) .toBe(timeoutStep.id); - const sessionsAfterTimeout = await apiClient.listTaskSessions(task.id); - const primarySession = sessionsAfterTimeout.sessions.find( - (candidate) => candidate.id === task.session_id, - ); - expect(primarySession?.state).toBe("WAITING_FOR_INPUT"); + await waitForSessionState(apiClient, { + taskId: task.id, + sessionId: task.session_id, + expectedState: "WAITING_FOR_INPUT", + message: "timed-out clarification session must remain ready for a deferred answer", + timeout: 30_000, + }); // Agent moved on; a late custom answer remains editable and goes through // the event fallback as a new prompt. diff --git a/apps/web/e2e/tests/task/autopilot-mode.spec.ts b/apps/web/e2e/tests/task/autopilot-mode.spec.ts index 0fa33881ea..815755dc00 100644 --- a/apps/web/e2e/tests/task/autopilot-mode.spec.ts +++ b/apps/web/e2e/tests/task/autopilot-mode.spec.ts @@ -1,6 +1,7 @@ import { expect, test } from "../../fixtures/test-base"; import type { ApiClient } from "../../helpers/api-client"; import { useRegularMode } from "../../helpers/regular-mode"; +import { waitForSessionState } from "../../helpers/session"; import { KanbanPage } from "../../pages/kanban-page"; import { SessionPage } from "../../pages/session-page"; @@ -32,22 +33,6 @@ function parentQuestionScript(): string { return `e2e:mcp:kandev:ask_parent_question_kandev(${args})`; } -async function waitForSessionState( - apiClient: ApiClient, - taskId: string, - state: string, -): Promise { - await expect - .poll( - async () => { - const { sessions } = await apiClient.listTaskSessions(taskId); - return sessions[0]?.state ?? ""; - }, - { timeout: 60_000, message: `task ${taskId} should reach ${state}` }, - ) - .toBe(state); -} - async function waitForParentQuestion(apiClient: ApiClient, parentTaskID: string): Promise { let questionID = ""; await expect @@ -111,13 +96,22 @@ test.describe("Task autopilot", () => { }, ); + const childSessionId = child.session_id; + if (!childSessionId) throw new Error("autopilot child did not return a session ID"); + const childTask = await apiClient.getTask(child.id); expect(childTask.autopilot).toBe(true); await testPage.goto(`/t/${parent.id}`); const parentSession = new SessionPage(testPage); await parentSession.waitForLoad(); - await waitForSessionState(apiClient, child.id, "WAITING_FOR_INPUT"); + await waitForSessionState(apiClient, { + taskId: child.id, + sessionId: childSessionId, + expectedState: "WAITING_FOR_INPUT", + message: `task ${child.id} should reach WAITING_FOR_INPUT`, + timeout: 60_000, + }); const childRow = parentSession.sidebarTaskItem("Autopilot Child"); await expect(childRow).toBeVisible({ timeout: 30_000 }); await expect(childRow.getByTestId("task-autopilot-icon")).toBeVisible(); diff --git a/scripts/playwright-blob-audit b/scripts/playwright-blob-audit new file mode 100755 index 0000000000..9047580619 --- /dev/null +++ b/scripts/playwright-blob-audit @@ -0,0 +1,179 @@ +#!/usr/bin/env python3 +"""Audit Playwright blob reports for retries, errors, and unexpected results.""" + +from __future__ import annotations + +import argparse +import io +import json +import sys +import zipfile +from collections import Counter +from pathlib import Path, PurePosixPath +from typing import Iterable + + +EXPECTED_STATUSES = {"passed", "skipped"} + + +def is_report_jsonl(path: str | Path) -> bool: + name = Path(path).name + return name == "report.jsonl" or (name.startswith("report-") and name.endswith(".jsonl")) + + +def is_report_zip(path: str | Path) -> bool: + name = Path(path).name + return name.startswith("report-") and name.endswith(".zip") + + +def discover_reports(inputs: Iterable[Path]) -> list[tuple[str, Path, str | None]]: + """Return sorted (kind, path, member) tuples without duplicate inputs.""" + + reports: dict[tuple[str, str, str | None], tuple[str, Path, str | None]] = {} + + for input_path in inputs: + if not input_path.exists(): + raise FileNotFoundError(input_path) + + candidates = [input_path] if input_path.is_file() else sorted(input_path.rglob("*")) + for candidate in candidates: + if not candidate.is_file(): + continue + if candidate.suffix == ".zip" and (is_report_zip(candidate) or candidate == input_path): + try: + with zipfile.ZipFile(candidate) as archive: + for member in archive.namelist(): + if is_report_jsonl(PurePosixPath(member).name): + key = ("zip", str(candidate), member) + reports[key] = ("zip", candidate, member) + except zipfile.BadZipFile as exc: + raise ValueError(f"invalid Playwright report archive: {candidate}") from exc + elif is_report_jsonl(candidate): + key = ("file", str(candidate), None) + reports[key] = ("file", candidate, None) + + return sorted(reports.values(), key=lambda report: (str(report[1]), report[2] or "")) + + +def iter_report_lines(kind: str, path: Path, member: str | None) -> Iterable[tuple[int, str]]: + if kind == "file": + with path.open("r", encoding="utf-8") as stream: + for line_number, line in enumerate(stream, start=1): + yield line_number, line + return + + assert member is not None + with zipfile.ZipFile(path) as archive: + with archive.open(member) as binary_stream: + stream = io.TextIOWrapper(binary_stream, encoding="utf-8") + for line_number, line in enumerate(stream, start=1): + yield line_number, line + + +def result_from(event: dict) -> dict: + params = event.get("params") + if not isinstance(params, dict): + params = {} + result = params.get("result") + if not isinstance(result, dict): + result = event.get("result") + return result if isinstance(result, dict) else {} + + +def audit_reports(reports: list[tuple[str, Path, str | None]]) -> dict: + attempt_count = 0 + retry_attempt_count = 0 + status_counts: Counter[str] = Counter() + unexpected_statuses: Counter[str] = Counter() + error_result_count = 0 + parse_error_count = 0 + event_count = 0 + report_names: list[str] = [] + + for kind, path, member in reports: + report_name = f"{path}!{member}" if member is not None else str(path) + report_names.append(report_name) + for line_number, raw_line in iter_report_lines(kind, path, member): + line = raw_line.strip() + if not line: + continue + try: + event = json.loads(line) + except json.JSONDecodeError: + parse_error_count += 1 + continue + if not isinstance(event, dict): + continue + + method = event.get("method") or event.get("event") or event.get("type") + if method not in {"onTestBegin", "onTestEnd"}: + continue + event_count += 1 + result = result_from(event) + + if method == "onTestBegin": + attempt_count += 1 + retry = result.get("retry", 0) + if isinstance(retry, (int, float)) and not isinstance(retry, bool) and retry > 0: + retry_attempt_count += 1 + continue + + status = result.get("status") + if status is None: + params = event.get("params") + status = params.get("status") if isinstance(params, dict) else event.get("status") + status_name = str(status) if status is not None else "unknown" + status_counts[status_name] += 1 + if status_name not in EXPECTED_STATUSES: + unexpected_statuses[status_name] += 1 + errors = result.get("errors") + if errors: + error_result_count += 1 + + violations: list[str] = [] + if retry_attempt_count: + violations.append(f"retry_attempts={retry_attempt_count}") + if error_result_count: + violations.append(f"error_results={error_result_count}") + if unexpected_statuses: + violations.append(f"unexpected_statuses={dict(sorted(unexpected_statuses.items()))}") + if parse_error_count: + violations.append(f"parse_errors={parse_error_count}") + if not event_count: + violations.append("no_test_events") + + return { + "reports": report_names, + "report_count": len(report_names), + "attempt_count": attempt_count, + "retry_attempt_count": retry_attempt_count, + "status_counts": dict(sorted(status_counts.items())), + "error_result_count": error_result_count, + "unexpected_statuses": dict(sorted(unexpected_statuses.items())), + "parse_error_count": parse_error_count, + "violations": violations, + } + + +def main(argv: list[str] | None = None) -> int: + parser = argparse.ArgumentParser( + description="Audit Playwright report-*.zip/report.jsonl artifacts for retries and errors." + ) + parser.add_argument("paths", nargs="+", type=Path, help="report files, archives, or directories") + args = parser.parse_args(argv) + + try: + reports = discover_reports(args.paths) + if not reports: + raise ValueError("no report-*.zip or report.jsonl artifacts found") + summary = audit_reports(reports) + except (FileNotFoundError, OSError, ValueError) as exc: + print(f"playwright-blob-audit: {exc}", file=sys.stderr) + return 2 + + print(json.dumps(summary, indent=2, sort_keys=True)) + return 1 if summary["violations"] else 0 + + +if __name__ == "__main__": + raise SystemExit(main()) diff --git a/scripts/playwright-blob-audit.test.py b/scripts/playwright-blob-audit.test.py new file mode 100755 index 0000000000..5d83eb5199 --- /dev/null +++ b/scripts/playwright-blob-audit.test.py @@ -0,0 +1,85 @@ +#!/usr/bin/env python3 +import json +import subprocess +import sys +import tempfile +import unittest +import zipfile +from pathlib import Path + + +ROOT = Path(__file__).resolve().parents[1] +SCRIPT = ROOT / "scripts" / "playwright-blob-audit" + + +def event(method: str, result: dict) -> str: + return json.dumps({"method": method, "params": {"result": result}}) + + +class PlaywrightBlobAuditTest(unittest.TestCase): + def run_audit(self, path: Path) -> subprocess.CompletedProcess[str]: + return subprocess.run( + [sys.executable, str(SCRIPT), str(path)], + cwd=ROOT, + text=True, + capture_output=True, + check=False, + ) + + def test_passes_for_recursive_zip_report(self) -> None: + with tempfile.TemporaryDirectory() as directory: + root = Path(directory) + archive_path = root / "nested" / "report-001.zip" + archive_path.parent.mkdir() + with zipfile.ZipFile(archive_path, "w") as archive: + archive.writestr( + "report.jsonl", + "\n".join( + [ + event("onTestBegin", {"retry": 0}), + event("onTestEnd", {"status": "passed", "errors": []}), + event("onTestBegin", {"retry": 0}), + event("onTestEnd", {"status": "skipped", "errors": []}), + ] + ) + + "\n", + ) + + result = self.run_audit(root) + + self.assertEqual(result.returncode, 0, result.stderr) + summary = json.loads(result.stdout) + self.assertEqual(summary["report_count"], 1) + self.assertEqual(summary["attempt_count"], 2) + self.assertEqual(summary["retry_attempt_count"], 0) + self.assertEqual(summary["status_counts"], {"passed": 1, "skipped": 1}) + + def test_fails_for_retry_error_and_unexpected_status(self) -> None: + with tempfile.TemporaryDirectory() as directory: + report_path = Path(directory) / "report.jsonl" + report_path.write_text( + "\n".join( + [ + event("onTestBegin", {"retry": 1}), + event("onTestEnd", {"status": "failed", "errors": [{"message": "boom"}]}), + ] + ) + + "\n", + encoding="utf-8", + ) + + result = self.run_audit(report_path) + + self.assertEqual(result.returncode, 1, result.stderr) + summary = json.loads(result.stdout) + self.assertEqual(summary["retry_attempt_count"], 1) + self.assertEqual(summary["error_result_count"], 1) + self.assertEqual(summary["unexpected_statuses"], {"failed": 1}) + self.assertEqual( + summary["violations"], + ["retry_attempts=1", "error_results=1", "unexpected_statuses={'failed': 1}"], + ) + + +if __name__ == "__main__": + unittest.main() diff --git a/scripts/pr-state b/scripts/pr-state index 71547ff660..426811d36d 100755 --- a/scripts/pr-state +++ b/scripts/pr-state @@ -8,7 +8,8 @@ # # Raw output: # JSON object with: -# - pr: { number, branch, is_cross_repository, head_repository_owner, +# - pr: { number, branch, is_cross_repository, base_ref_name, base_head_oid, +# merge_base_oid, base_advanced_since_head, head_repository_owner, # head_repository_name, head_ref_name, head_ref_oid, maintainer_can_modify } # - since: { head_oid, committed_at } or null # - checks: [{ type, name, workflow, status, conclusion, details_url, target_url, run_id, job_id }] @@ -30,7 +31,8 @@ # # Summary output (--summary): # JSON object with: -# - pr: { number, branch, is_cross_repository, head_repository_owner, +# - pr: { number, branch, is_cross_repository, base_ref_name, base_head_oid, +# merge_base_oid, base_advanced_since_head, head_repository_owner, # head_repository_name, head_ref_name, head_ref_oid, maintainer_can_modify } # - since: { head_oid, committed_at } or null # - failed_checks: [{ name, workflow, status, conclusion, run_id, job_id, details_url, target_url }] @@ -148,9 +150,14 @@ fi errors='[]' pr_json='{}' +pr_view_succeeded=false checks_json='[]' checks_head_sha='' checks_snapshot_complete=false +base_ref_name='' +base_head_oid='' +merge_base_oid='' +base_advanced_since_head=null approval_required_runs_json='[]' review_threads_json='[]' hidden_unresolved_threads_json='[]' @@ -670,6 +677,38 @@ fetch_head_commit_json() { }' } +fetch_base_ref_json() { + local owner=$1 + local repo=$2 + local pr_number=$3 + + gh_json api graphql \ + -f query=' + query($owner: String!, $repo: String!, $pr: Int!) { + repository(owner: $owner, name: $repo) { + pullRequest(number: $pr) { + baseRefName + baseRefOid + } + } + }' \ + -f owner="$owner" -f repo="$repo" -F pr="$pr_number" | + jq -c '{ + base_ref_name: (.data.repository.pullRequest.baseRefName // null), + base_head_oid: (.data.repository.pullRequest.baseRefOid // null) + }' +} + +fetch_merge_base_json() { + local owner=$1 + local repo=$2 + local base_head_oid=$3 + local head_oid=$4 + + gh_json api "repos/$owner/$repo/compare/$base_head_oid...$head_oid" | + jq -c '{merge_base_oid: (.merge_base_commit.sha // null)}' +} + fetch_review_threads_nodes_json() { local owner=$1 local repo=$2 @@ -761,7 +800,8 @@ comments_json='[]' status_json='[]' resolved_pr_number='' is_cross_repository=null -if pr_view_json="$(gh_json pr view "$PR" --json number,headRefName,headRefOid,headRepositoryOwner,headRepository,maintainerCanModify,isCrossRepository,statusCheckRollup,comments,url)"; then +if pr_view_json="$(gh_json pr view "$PR" --json number,baseRefName,headRefName,headRefOid,headRepositoryOwner,headRepository,maintainerCanModify,isCrossRepository,statusCheckRollup,comments,url)"; then + pr_view_succeeded=true resolved_pr_number="$(jq -r '.number // ""' <<<"$pr_view_json")" if pr_url_owner="$(jq -r '.url | capture("github.com/(?[^/]+)/(?[^/]+)/pull/[0-9]+").owner // ""' <<<"$pr_view_json" 2>/dev/null)" && [[ -n "$pr_url_owner" ]]; then repo_owner="$pr_url_owner" @@ -771,6 +811,10 @@ if pr_view_json="$(gh_json pr view "$PR" --json number,headRefName,headRefOid,he number: .number, branch: (.headRefName // "unknown"), is_cross_repository: (if has("isCrossRepository") then .isCrossRepository else null end), + base_ref_name: (.baseRefName // null), + base_head_oid: null, + merge_base_oid: null, + base_advanced_since_head: null, head_repository_owner: (.headRepositoryOwner.login // null), head_repository_name: (.headRepository.name // null), head_ref_name: (.headRefName // null), @@ -791,6 +835,43 @@ else add_error "pr_view" "gh pr view failed" fi +if [[ "$pr_view_succeeded" == "true" && -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then + if base_context_json="$(fetch_base_ref_json "$repo_owner" "$repo_name" "$resolved_pr_number")"; then + base_ref_name="$(jq -r '.base_ref_name // ""' <<<"$base_context_json")" + base_head_oid="$(jq -r '.base_head_oid // ""' <<<"$base_context_json")" + else + add_error "base" "gh api graphql base ref lookup failed" + fi + + if [[ -n "$base_head_oid" && -n "$checks_head_sha" ]]; then + if merge_base_json="$(fetch_merge_base_json "$repo_owner" "$repo_name" "$base_head_oid" "$checks_head_sha")"; then + merge_base_oid="$(jq -r '.merge_base_oid // ""' <<<"$merge_base_json")" + else + add_error "merge_base" "gh api compare base and head failed" + fi + fi + + if [[ -n "$base_head_oid" && -n "$merge_base_oid" ]]; then + if [[ "$base_head_oid" == "$merge_base_oid" ]]; then + base_advanced_since_head=false + else + base_advanced_since_head=true + fi + fi + + pr_json="$(jq -c \ + --arg base_ref_name "$base_ref_name" \ + --arg base_head_oid "$base_head_oid" \ + --arg merge_base_oid "$merge_base_oid" \ + --argjson base_advanced_since_head "$base_advanced_since_head" \ + '. + { + base_ref_name: (if $base_ref_name == "" then null else $base_ref_name end), + base_head_oid: (if $base_head_oid == "" then null else $base_head_oid end), + merge_base_oid: (if $merge_base_oid == "" then null else $merge_base_oid end), + base_advanced_since_head: $base_advanced_since_head + }' <<<"$pr_json")" +fi + if ! approval_required_runs_json="$(fetch_approval_required_runs_json "$repo_owner" "$repo_name" "$resolved_pr_number" "$checks_head_sha")"; then approval_required_runs_json='[]' add_error "approval_required_runs" "workflow runs API failed" @@ -865,6 +946,55 @@ if [[ -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then add_error "closing_head" "gh api graphql closing head commit failed" fi fi + +# Re-read the base alongside the closing head. The base can advance while +# reviews and checks are collected, so the initial divergence fields are not a +# safe final snapshot. +if [[ -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then + if closing_base_context_json="$(fetch_base_ref_json "$repo_owner" "$repo_name" "$resolved_pr_number")"; then + closing_base_ref_name="$(jq -r '.base_ref_name // ""' <<<"$closing_base_context_json")" + closing_base_head_oid="$(jq -r '.base_head_oid // ""' <<<"$closing_base_context_json")" + closing_merge_base_oid='' + comparison_head_oid="$closing_head_oid" + [[ -n "$comparison_head_oid" ]] || comparison_head_oid="$checks_head_sha" + if [[ -n "$closing_base_head_oid" && -n "$comparison_head_oid" ]]; then + if closing_merge_base_json="$(fetch_merge_base_json "$repo_owner" "$repo_name" "$closing_base_head_oid" "$comparison_head_oid")"; then + closing_merge_base_oid="$(jq -r '.merge_base_oid // ""' <<<"$closing_merge_base_json")" + else + add_error "closing_merge_base" "gh api compare base and head failed after evidence collection" + fi + fi + base_ref_name="$closing_base_ref_name" + base_head_oid="$closing_base_head_oid" + merge_base_oid="$closing_merge_base_oid" + base_advanced_since_head=null + if [[ -n "$base_head_oid" && -n "$merge_base_oid" ]]; then + if [[ "$base_head_oid" == "$merge_base_oid" ]]; then + base_advanced_since_head=false + else + base_advanced_since_head=true + fi + fi + else + add_error "closing_base" "gh api graphql base ref lookup failed after evidence collection" + base_ref_name='' + base_head_oid='' + merge_base_oid='' + base_advanced_since_head=null + fi + pr_json="$(jq -c \ + --arg base_ref_name "$base_ref_name" \ + --arg base_head_oid "$base_head_oid" \ + --arg merge_base_oid "$merge_base_oid" \ + --argjson base_advanced_since_head "$base_advanced_since_head" \ + '. + { + base_ref_name: (if $base_ref_name == "" then null else $base_ref_name end), + base_head_oid: (if $base_head_oid == "" then null else $base_head_oid end), + merge_base_oid: (if $merge_base_oid == "" then null else $merge_base_oid end), + base_advanced_since_head: $base_advanced_since_head + }' <<<"$pr_json")" +fi + if [[ -n "$opening_head_oid" && -n "$closing_head_oid" && "$opening_head_oid" != "$closing_head_oid" ]]; then add_error "review_evidence" "head changed during evidence collection" fi diff --git a/scripts/pr-state.test.sh b/scripts/pr-state.test.sh index 28d5d87ab0..163a29aa5d 100644 --- a/scripts/pr-state.test.sh +++ b/scripts/pr-state.test.sh @@ -79,6 +79,20 @@ if [[ "$1" == "repo" && "$2" == "view" ]]; then exit 0 fi +if [[ "$1" == "api" && "$2" == repos/kdlbs/kandev/compare/* ]]; then + merge_base="${GH_MERGE_BASE:-base-branch-sha}" + if [[ "${GH_BASE_SEQUENCE:-stable}" == "race" && "$2" == *"late-base-head-sha..."* ]]; then + merge_base="late-merge-base-sha" + fi + cat <"$GH_BASE_COUNTER_FILE" + fi + fi + printf '%s\n' '{ + "data": { + "repository": { + "pullRequest": { + "baseRefName": "main", + "baseRefOid": "'"$base_head"'" + } + } + } + }' + exit 0 + fi + if [[ "$*" == *"headRefOid"* ]]; then head_oid="abc123" if [[ "${GH_HEAD_SEQUENCE:-stable}" == "race" ]]; then @@ -701,6 +738,7 @@ test_snapshot_happy_path() { assert_jq "pr number" '.pr.number == 123' "$json" assert_jq "branch" '.pr.branch == "feat/pr-state"' "$json" assert_jq "head delivery target" '.pr.head_repository_owner == "kdlbs" and .pr.head_repository_name == "kandev" and .pr.head_ref_name == "feat/pr-state" and .pr.head_ref_oid == "abc123" and .pr.maintainer_can_modify == true' "$json" + assert_jq "base divergence fields" '.pr.base_ref_name == "main" and .pr.base_head_oid == "base-head-sha" and .pr.merge_base_oid == "base-branch-sha" and .pr.base_advanced_since_head == true' "$json" assert_jq "since timestamp" '.since.committed_at == "2026-06-01T12:00:00Z"' "$json" assert_jq "checks collapse duplicate workflow attempts" '.checks | length < 10' "$json" assert_jq "latest duplicate check uses newest attempt" '[.checks[] | select(.name == "web lint")][0] | .conclusion == "success" and .run_id == "27340000001"' "$json" @@ -1142,10 +1180,12 @@ test_graphql_failure_records_error_but_keeps_other_data() { assert_jq "reviews still present on graphql failure" '.reviews | length == 2' "$json" assert_jq "review threads empty on graphql failure" '.review_threads == []' "$json" assert_jq "unresolved count unknown on graphql failure" '.unresolved_review_thread_count == null' "$json" - assert_jq "graphql failure records opening and closing head errors" '.errors | length == 3' "$json" + assert_jq "graphql failure records base, opening, and closing errors" '.errors | length == 5' "$json" + assert_jq "graphql failure records base lookup error" '.errors[] | select(.source == "base") | .message == "gh api graphql base ref lookup failed"' "$json" assert_jq "graphql failure records since error" '.errors[] | select(.source == "since") | .message == "gh api graphql head commit failed; including historical comments"' "$json" assert_jq "graphql failure records review_threads error" '.errors[] | select(.source == "review_threads") | .message == "gh api graphql reviewThreads failed"' "$json" assert_jq "graphql failure records closing head error" '.errors[] | select(.source == "closing_head") | .message == "gh api graphql closing head commit failed"' "$json" + assert_jq "graphql failure records closing base error" '.errors[] | select(.source == "closing_base") | .message == "gh api graphql base ref lookup failed after evidence collection"' "$json" assert_jq "since fallback includes all historical comments" '.issue_comments | length == 8' "$json" pass "graphql failure records error but keeps other data" } @@ -1196,6 +1236,7 @@ test_summary_mode_returns_compact_fixup_state() { assert_jq "summary keeps pr" '.pr.number == 123' "$json" assert_jq "summary keeps authoritative head delivery target" '.pr.head_repository_owner == "kdlbs" and .pr.head_repository_name == "kandev" and .pr.head_ref_name == "feat/pr-state" and .pr.head_ref_oid == "abc123" and .pr.maintainer_can_modify == true' "$json" + assert_jq "summary keeps base divergence fields" '.pr.base_ref_name == "main" and .pr.base_head_oid == "base-head-sha" and .pr.merge_base_oid == "base-branch-sha" and .pr.base_advanced_since_head == true' "$json" assert_jq "summary keeps since" '.since.committed_at == "2026-06-01T12:00:00Z"' "$json" assert_jq "summary has no approval-required runs by default" '.approval_required_runs == []' "$json" assert_jq "summary failed check count" '.failed_checks | length == 3' "$json" @@ -1234,6 +1275,32 @@ test_summary_reports_current_head_fork_approval_runs() { pass "--summary reports current-head fork approval runs" } +test_summary_reports_base_not_advanced_when_head_matches_merge_base() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_BASE_HEAD=base-branch-sha GH_MERGE_BASE=base-branch-sha PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "summary reports base not advanced" '.pr.base_advanced_since_head == false' "$json" + pass "summary reports base not advanced when head matches merge base" +} + +test_summary_revalidates_base_at_closing_head() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_BASE_SEQUENCE=race GH_BASE_COUNTER_FILE="$tmp/base-calls" GH_MERGE_BASE=base-head-sha PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "summary uses closing base ref" '.pr.base_head_oid == "late-base-head-sha" and .pr.merge_base_oid == "late-merge-base-sha" and .pr.base_advanced_since_head == true' "$json" + pass "summary revalidates base at closing head" +} + test_summary_reports_approval_run_fetch_failure() { local tmp make_tmp_dir tmp @@ -1448,6 +1515,8 @@ test_graphql_pagination_collects_all_threads test_all_flag_includes_historical_comments_and_reviews test_summary_mode_returns_compact_fixup_state test_summary_reports_current_head_fork_approval_runs +test_summary_reports_base_not_advanced_when_head_matches_merge_base +test_summary_revalidates_base_at_closing_head test_summary_reports_approval_run_fetch_failure test_summary_all_flag_includes_historical_unresolved_threads test_job_log_mode_emits_bounded_failure_context