diff --git a/.agents/skills/acp-debug/SKILL.md b/.agents/skills/acp-debug/SKILL.md index 858ddfb248..f08c6faea2 100644 --- a/.agents/skills/acp-debug/SKILL.md +++ b/.agents/skills/acp-debug/SKILL.md @@ -72,6 +72,13 @@ apps/backend/bin/acpdbg probe --timeout 45s auggie For `matrix`, prefer `--timeout 60s` so `npx`-spawned agents have time to cold-start. +For Claude ACP, the generated default workdir is an absolute temporary +directory and is sent as the ACP `cwd`; no `--workdir` flag is required. If +you override it, supply an existing absolute directory (for example, run +`mkdir -p /tmp/kandev-acpdbg-claude` before passing that path to `--workdir`). +An empty `cwd` can cause Claude to reject `session/new`; confirm the `cwd` in +the recorded request frame before diagnosing a provider failure. + ### 3. Read the JSONL file The JSONL schema is: diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index 2ab8a1ea0d..e21ffcad6c 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -70,6 +70,16 @@ Before reviewing implementation details: - Identify missing coverage for happy path, key error paths, edge cases, auth/workspace boundaries, and concurrency/order-sensitive behavior. - For concurrent or event-driven changes, require a deterministic schedule that checks ownership or generation identity, stale-event handling, cancellation, and lock scope. Channel/barrier coordination is preferable to timing sleeps. - For stale-event races, cover both event-before-successor and delayed-old-event-after-successor orderings. Prefer integration coverage for cross-package event or callback paths when practical. +- When an HTTP mutation returns a full entity while WebSocket/event updates can + update the same entity, ensure a delayed HTTP response cannot overwrite the + newer event. Prefer a narrow mutation response or guard a full merge with an + immutable revision/`updated_at`; cover it with a deferred-response test that + applies the newer event first. +- For ordering guarantees across an event bus, trace producer, remote + transport, and gateway/client delivery. Sequential publishes on separate + subscriptions do not establish client order; require a unified stream or + sequence-aware buffering, with a transport-boundary test and local-emulator + coverage. - For terminal event streams, block an earlier publication, enqueue a terminal event (for example delete or cancellation), then enqueue a stale update. Assert no later mutation reaches an upserting consumer; queues must tombstone the entity or discard pending work at the terminal boundary. - When completion events lack a stable workload identity, test N outstanding registrations with N completion signals and duplicate delivery. A single-registration test cannot prove that uncorrelated completions retire work correctly. Compare this behavior with the accepted spec or ADR; a passing test that contradicts the contract is still a blocker. - Treat missing tests for new or changed non-UI logic as a blocker unless the change is explicitly untestable and says why. @@ -80,6 +90,9 @@ Check every changed file for the following layers. Skip layers that don't apply **Security** (blockers if found): - No secrets, tokens, or credentials in code +- When persisted configuration is copied into UI or session metadata, trace it + through the applicable sanitizer/redaction boundary; storage-safe values are + not automatically presentation-safe. - Input validation at system boundaries (user input, API handlers, external data) - No SQL injection, XSS, command injection, or path traversal risks - Authentication and authorization checks in place for new endpoints @@ -127,6 +140,11 @@ Check every changed file for the following layers. Skip layers that don't apply - For changed Makefiles, shell scripts, or CI path filters, trace each changed target through the shell and platform branches. Distinguish executable naming from recipe-shell syntax; inspect `OS`, `MSYSTEM`, and `SHELL` assumptions. +- When simulating a Windows Make branch from POSIX, prefer + `scripts/check-make-shells`. If a manual `make -n` is necessary, neutralize + its parse-time probes as that checker does (`NULL_REDIR= BUILD_TIME=simulated`) + so POSIX does not create `NUL` artifacts. Compare `git status --short` with + the initial snapshot afterward. - Use `make -n ` for every affected platform branch that is available, and confirm CI invokes the changed target. Include docs or configuration paths when a validator or test reads them. @@ -153,6 +171,10 @@ branch, do not edit the checkout or push code; report findings through the channel the user requested. Do not submit or resolve reviews unless explicitly asked. +Before a read-only review ends, compare `git status --short` with the initial +snapshot. Remove only diagnostic artifacts demonstrably created during the +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. diff --git a/.agents/skills/commit/SKILL.md b/.agents/skills/commit/SKILL.md index d529dba5da..0445069a1a 100644 --- a/.agents/skills/commit/SKILL.md +++ b/.agents/skills/commit/SKILL.md @@ -86,6 +86,14 @@ explicitly requests task tracking. Mention that you wired it up. Subsequent commits will run hooks automatically. - If both checks pass, no output needed. + - Before the first commit, ensure the worktree can run the commit-msg + dependency. If `apps/node_modules/.bin/commitlint` is absent, install it + from `apps/` before committing: + ```bash + cd apps && pnpm install --frozen-lockfile + ``` + Retry the normal commit after the install; never bypass the hook. + 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,9 +117,24 @@ qualifies as a successful hook receipt. ```bash COMMIT_LOG="$(mktemp "${TMPDIR:-/tmp}/kandev-commit.XXXXXX.log")" set -o pipefail - git commit -m "type(scope): description" 2>&1 | tee "$COMMIT_LOG" + 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 ``` - Remove the temporary log after copying the receipt into the handoff. + 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 + the handoff: + ```bash + unlink "$COMMIT_LOG" + ``` + + If a hook fails only because another worktree is already running + golangci-lint (for example, `parallel golangci-lint is running`), wait for + that run to finish and retry the same commit. Do not bypass hooks or change + code for this transient lock; verify the retry has a normal hook receipt and + a clean worktree. 6. **Return a hook receipt:** After a successful commit, report: ```text diff --git a/.agents/skills/e2e/SKILL.md b/.agents/skills/e2e/SKILL.md index 9834d052eb..126018c590 100644 --- a/.agents/skills/e2e/SKILL.md +++ b/.agents/skills/e2e/SKILL.md @@ -52,6 +52,16 @@ Each worker gets an isolated backend, frontend, database, and mock agent — no **Always run headless** (`make test-e2e`). Never use `--headed`, `e2e:headed`, or `test-e2e-headed` — headed mode requires a display and will fail in agent environments. +**Fresh worktree bootstrap:** Before the first pnpm or E2E command in a new +worktree, install the workspace dependencies: + +```bash +cd apps && pnpm install --frozen-lockfile +``` + +Do this once before changing into `apps/web` or running a filtered package +command. Shared `.git` metadata does not include `apps/node_modules`. + ### Preferred: `pnpm e2e:run` (managed runner — builds, runs, tears down) `e2e/scripts/run-e2e.sh` handles the build, the run, and cleanup in one command. Use it instead of stitching the steps together. It auto-selects docker vs host, runs N shards concurrently, enforces strict WS accounting by default (matching CI), and never leaves root-owned artifacts behind. @@ -63,10 +73,25 @@ pnpm e2e:run tests/task/my-test.spec.ts # single file (extra args pass th pnpm e2e:run tests/path/spec.ts -- --grep "exact test name" # exact CI failure with a fresh build pnpm e2e:run --shards 3 # 3 shards concurrently on this machine (isolated) pnpm e2e:run --no-build -- --grep "task creation" # skip rebuild; forward flags after -- +pnpm e2e:run --no-build --project mobile-chrome tests/layout/mobile-spa-resilience.spec.ts pnpm e2e:docker # force the docker CI image (full isolation from a host dev instance) pnpm e2e:clean # remove build/test artifacts, incl. root-owned ones from prior docker runs ``` +**Select the owning Playwright project.** The default `chromium` project +intentionally excludes routing, auth, mobile, and container suites; a matching +path with the wrong project exits with `No tests found`. Pass the project before +the spec path, for example: + +```bash +pnpm e2e:run --project auth tests/auth/auth-lifecycle.spec.ts +pnpm e2e:run --project routing tests/office-routing-.spec.ts +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. + 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 @@ -77,6 +102,12 @@ without `--no-build` or first run `make -C apps/backend e2e-plugin-package`. Prefer a normal managed build after source or base-branch changes and reserve `--no-build` for repeated runs against unchanged artifacts. +For a raw Docker/SSH/container run, `make build-backend` alone does not build +the Linux mock-agent fixture. Prefer the managed runner; otherwise run +`make build-backend build-backend-remote-helpers build-web`. If the fixture +reports a missing `KANDEV_MOCK_AGENT_LINUX_BINARY`, run +`make -C apps/backend build-mock-agent-linux` before diagnosing product code. + ### Raw commands (when you need fine control) ```bash @@ -114,6 +145,13 @@ Start by matching CI as closely as possible, then add pressure deliberately: full spec file or full shard with the same resource limits before declaring a flake non-reproducible. +A test that passes only after a Playwright retry is still a failure signal. +During triage disable per-spec retry overrides and use `--retries=0`. When +isolated repeats pass but the same shard position fails, binary-search the +preceding spec files under one worker until the smallest ordered sequence +reproduces the shared-state leak; the fix is complete only when that sequence +passes with retries disabled. + 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 @@ -141,7 +179,9 @@ Without this, tests run against stale code and failures are misleading. `make bu For a touch-specific interaction, use Playwright `.tap()` rather than `.click()` so the app receives a touch `pointerType`. Run focused mobile specs with -`-- --project=mobile-chrome`; otherwise Playwright can report no matching tests. +`pnpm e2e:run --project mobile-chrome e2e/tests//mobile-.spec.ts`. +`--project` is a runner option and must precede `--`; the mobile project only +matches `mobile-*.spec.ts` files, so another filename can produce no tests. After the interaction settles, assert the resulting state and exercise a later mouse or pen entry when the UI maintains hybrid-device pointer state. @@ -169,6 +209,13 @@ Run the assertion in the relevant desktop and mobile projects when responsive layout can change the result. Do not rely on fixed pixels when the product contract is equality or alignment. +**Animation-aware geometry:** Before reading dialog or panel geometry, wait only for currently running Web Animations with finite `effect.getComputedTiming().iterations`; await `animation.finished.catch(() => undefined)` because Radix overlays can cancel animations during close or replacement. Never blanket-await infinite animations or use a fixed sleep; then read bounding boxes and assert the relationship. + +For narrow-width clipping or overlap regressions, visibility and containment +are insufficient: assert a real hit target. Check `document.elementFromPoint()` +at the control center resolves to the control (or its descendant), then prove +the action remains clickable at the legal minimum width. + ### IDs and response shapes — common pitfalls - **`apiClient.createTaskWithAgent(...)` returns `CreateTaskResponse`**, which is `Task & { session_id?: string; agent_execution_id?: string }`. Read `created.session_id` directly — don't call `listTaskSessions(taskId)` just to fetch the session that was auto-started by the same call. @@ -219,7 +266,7 @@ KANDEV_LOG_LEVEL=warn apps/backend/bin/kandev & Start the dev frontend: ```bash -KANDEV_API_BASE_URL=http://localhost:$BACKEND_PORT NEXT_PUBLIC_KANDEV_API_PORT=$BACKEND_PORT \ +KANDEV_API_BASE_URL=http://localhost:$BACKEND_PORT VITE_KANDEV_API_PORT=$BACKEND_PORT \ pnpm --filter @kandev/web dev --port $FRONTEND_PORT & ``` @@ -303,7 +350,29 @@ Tests are grouped by feature area in subdirectories under `tests/`. When creatin - **Test through the UI, not the API.** E2E tests verify user-facing behavior. Don't write tests that only call the API and assert the response -- those are integration tests. Instead, navigate to the page, interact with UI elements, and assert what the user sees. - **Verify persistence with page reload.** After changing a setting or creating data, reload the page (`testPage.reload()`) and assert the state is still correct. This catches hydration bugs and 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. +- **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 + `e2eReset` do not reset every seeded record. A test that PATCHes a canonical + `seedData` profile, repository, executor, or setting must capture its + baseline and restore it in `test.afterEach` (so cleanup also runs after + failure); prefer a disposable record when the UI can select it. Verify by + running the mutating spec followed by its affected neighbour with + `--workers=1 --retries=0`. +- **Pass browser-evaluation values explicitly.** `locator.evaluate` and + `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)`. +- **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 + assert the absolute deviation after the asynchronous content appears. This + attributes movement to the result rather than the initiating action and + catches movement in either direction. +- **Scroll-positioned markers.** For unread dividers, restore points, or search + anchors, seed content taller than the viewport and assert the marker's bounds + are inside the viewport after navigation. A short transcript's DOM-visible + marker does not prove initial scroll behavior; cover every renderer/viewport + 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. @@ -341,8 +410,15 @@ When a test fails: ### Common issues - **"Backend did not become healthy"** — run `make build-backend build-web`, check with `E2E_DEBUG=1` -- **"Cannot find module"** — run `cd apps && pnpm install` +- **"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 +- **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 + resizes the DOM/container and fires `ResizeObserver`, while an + 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. - **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) @@ -367,7 +443,21 @@ E2E tests run against the **production Vite build served by the Go backend**, no unzip report-*.zip -d report-shard && cat report-shard/*.jsonl ``` -When a CI shard fails, download its report-*.zip artifact and unzip it; the report is a *.jsonl event stream. Build a testId map by walking the events: test titles and locations come from the testBegin events, and final status plus duration come from the testEnd events. Match them by test id. This surfaces the slow but passing specs (the timing markers in Playwright output) that never show up as outright failures but are latent flake risks. Specs whose duration approaches the 60s per-test timeout (defined in playwright.config.ts) are the flake candidates to harden. Typically by converting raw chat-flow assertions to the waitForChatIdle() / expectChatResponseVisible() recovery helpers documented earlier in this file. +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: + +```bash +gh run download --name test-results- --dir +``` + +The blob report surfaces slow-but-passing specs that are latent flake risks; +the test-results artifact identifies the concrete failure to reproduce. Specs +whose duration approaches the 60s per-test timeout (defined in +`playwright.config.ts`) are candidates to harden, typically by converting raw +chat-flow assertions to the `waitForChatIdle()` / `expectChatResponseVisible()` +recovery helpers documented earlier in this file. ### Flake triage: intrinsic race vs. contention diff --git a/.agents/skills/plan/SKILL.md b/.agents/skills/plan/SKILL.md index 8bb4eaae5e..070249aa42 100644 --- a/.agents/skills/plan/SKILL.md +++ b/.agents/skills/plan/SKILL.md @@ -134,6 +134,13 @@ For each user-facing scenario in the spec: --- +## Verification Results +Pending. On completion, synchronize this section with each task's `## Results`: +record exact commands and outcomes/counts, generated artifact paths, and +cleanup/teardown evidence. + +--- + ## Implementation Waves And Parallel Candidates Group task files by dependency order. Use waves to expose possible parallelism, @@ -189,7 +196,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`. +- **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. - **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 @@ -198,11 +205,20 @@ Each task should be small enough for one focused implementation pass: - **Output contract:** summary, files changed, tests run, blockers, risks, and task/plan status update in the same conversation. +## Results +Pending. Before marking the task done, replace this with every exact command +actually run and its outcome/count, generated artifact paths, and cleanup or +teardown evidence (including temporary capture-spec removal and +`git diff --check` when used). Record security/trust and external side-effect +boundaries when applicable, or explicitly state `None`. + Break a task down further if it touches unrelated subsystems, needs more than one focused session, or the title contains "and". When an implementation agent starts the task, it must change `status` to -`in_progress`. When it finishes, it must change `status` to `done` and update -the corresponding checkbox/status in `plan.md`. +`in_progress`. Before it finishes, reconcile **Files likely touched** with the +actual diff, including modified existing tests used as E2E evidence. It may then +change `status` to `done`, update its `## Results`, and synchronize the +corresponding checkbox/status and `## Verification Results` in `plan.md`. ``` ### Style rules diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index 3787ba373f..e98ddbe4e1 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -36,25 +36,44 @@ 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`, -`unresolved_review_thread_count`, `hidden_unresolved_threads`, and +`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. -For pending CI, do not run a rapid parent 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." +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. +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. -When the user explicitly asks to wait or monitor, the main conversation may -delegate one platform `pr-poller`. Give it the PR number and any selected -reviewer, then use its report only as status evidence. The poller is read-only, -time-bounded, and must not edit, push, comment, resolve threads, inspect source, -or spawn children. The primary conversation owns all triage and remediation; -never launch a poller automatically or relaunch one after a denied approval. +For a cross-repository PR whose current-head snapshot is unexpectedly sparse, +inspect `approval_required_runs`. A current-head workflow with +`conclusion=action_required` is blocked verification, not green or skipped CI. +Only after the user authorizes PR fixup, approve the exact run with +`gh api --method POST repos///actions/runs//approve`, +then re-run the summary and require jobs to materialize before polling. `gh run +approve` is not a valid command. Treat the state as clean only when the current head has no failed or pending checks, no merge conflict, no actionable review thread or issue comment, and @@ -62,40 +81,104 @@ qualifying exact-head semantic evidence where PR delivery requires it. ## 2. Fix CI Failures -Before changing code, confirm every reported failed check and its `run_id`. Use +Before changing code, confirm every reported failed check, its `run_id`, and +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. Reproduce the exact failed command where possible; +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`. +For unfamiliar, infrastructure, or E2E failures, load +`references/ci-troubleshooting.md` before changing code. +Also load it for unexpected zero-duration or no-op manual-review runs: event +and workflow provenance can explain them without a product-code change. + 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. ## 3. Triage And Address Reviews -Use `scripts/pr-resolve list ` to obtain unresolved threads. For each -comment, decide 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. +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. -Make only valid changes. For an invalid comment, reply with concrete reasoning -only when the user asks to respond. Resolve a thread only when the change or -response genuinely addresses it. +Make only valid changes. GitHub replies and thread resolution are external +writes. A direction to "address" valid review comments explicitly authorizes a +concise reply and resolution after the fix is pushed and targeted verification +passes; a review-only request does not. For an invalid comment, reply with +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. + +For an ordering or concurrency finding, trace the complete producer → event-bus +transport → gateway/client path. Sequential publishes do not prove delivery +order when a remote bus uses separate subscriptions; consolidate one stream or +add sequence-aware buffering when order is contractual, and cover both the +transport boundary and local emulator. + +When feedback says an action must remain reachable, add and run a regression at +the legal minimum width. Verify the actual hit target (for example, +`elementFromPoint()` at the control center) and clickability, not only +`toBeVisible`, before pushing. ## 4. Commit, Verify, Push Commit through `/commit`, then rerun only the unit, integration, or E2E command affected by the remediation. Push when that targeted check passes for the exact -current `HEAD`. Run broad `/verify` only if the user explicitly requests it or -the PR/CI finding requires it. +current `HEAD`. For a cross-repository PR, push the exact current `HEAD` to the +summary's authoritative head repository and ref only when +`pr.maintainer_can_modify` is true. Re-fetch the PR afterward and require its +`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. + +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 +match the local branch. Before a push, compare the remote head OID with the +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. ## 5. Re-check After every push, run `scripts/pr-state --summary ` again for the new head. -Treat prior review evidence as stale. Repeat this workflow only for a new CI -failure or actionable current-head review finding; otherwise report the PR as -ready or CI as still in progress. +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 +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 +`checks_snapshot_complete=true`, `failed_checks=[]`, `pending_checks=[]`, +`approval_required_runs=[]`, `actionable_issue_comment_count=0`, 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. + +## 6. User-Requested Merge + +Merge only after the user explicitly asks and the current-head state is clean. +From a linked worktree, run `gh pr merge --squash` without +`--delete-branch`: that flag can attempt a local checkout of the base branch +and fail when another worktree owns it, even after the remote merge succeeds. +Report the remote merge separately. Delete a remote or local branch only when +requested and through a worktree-safe cleanup flow. ## Guardrails diff --git a/.agents/skills/pr-fixup/references/ci-troubleshooting.md b/.agents/skills/pr-fixup/references/ci-troubleshooting.md index 92ae00db2d..0ba6fa6236 100644 --- a/.agents/skills/pr-fixup/references/ci-troubleshooting.md +++ b/.agents/skills/pr-fixup/references/ci-troubleshooting.md @@ -19,6 +19,81 @@ SHA (`git diff ..HEAD`) instead of against `main`. ## Infrastructure Failures +**Failed job in an in-progress workflow:** GitHub can expose a failed job +before the parent workflow is terminal, and `gh run view --log-failed` can +temporarily report its logs as unavailable. Confirm the job's conclusion and +steps before reproducing or changing code, then retrieve the job log directly +when needed: + +```bash +gh api repos///actions/jobs/ \ + --jq '{status, conclusion, steps: [.steps[] | {name, status, conclusion}]}' +scripts/pr-state --job-log +``` + +Treat an unavailable log stream as unknown evidence, not a product failure. +If the direct job-log request returns 404 while the job is queued or in progress, +wait for that job to complete before retrying; it is not missing evidence. +When the aggregate command exposes only a merge/report job, obtain every failed +shard `job_id` from `scripts/pr-state --summary ` and inspect at least one +failure from each shard before changing code: + +```bash +scripts/pr-state --job-log +``` + +If that summary still names only the aggregate gate, enumerate the failed jobs +from the workflow and inspect the concrete shard (then load its test-results +artifact as described in `/e2e`): + +```bash +gh run view --json jobs \ + --jq '.jobs[] | select(.conclusion == "failure") | {name, databaseId}' +``` + +Verify the output names the actual failing spec or assertion, rather than only +the merge-report exit code. + +**CodeQL code-scanning upload failure:** If CodeQL completes extraction and +query evaluation, then fails immediately after `Uploading code scanning +results` without a source finding or actionable log error, treat it as GitHub +code-scanning upload infrastructure. On the current head, rerun the failed job +once and re-check the PR state: + +```bash +gh run rerun --failed +scripts/pr-state --summary +``` + +If a newer head has already been pushed, rely on its newly triggered CodeQL +run. Report an unchanged upload failure rather than changing product code. + +**Go module-proxy transport failure:** If a Go test job fails during package +setup while downloading from `proxy.golang.org` (for example, `stream error: +stream ID ... INTERNAL_ERROR`), and has no test assertion or compile failure, +treat it as proxy transport infrastructure. Rerun the failed job once, then +re-check the current head: + +```bash +gh run rerun --failed +scripts/pr-state --summary +``` + +Only investigate product code when the rerun exposes a repeatable test or build +failure. + +**Third-party deployment fetch failure:** For a deployment action that cannot +fetch its third-party resource, compare adjacent successful runs for the same +branch/head and rerun the failed job once before changing workflow or product +code. Treat a one-off fetch failure as infrastructure unless it repeats with +an actionable configuration error. + +**Merge-ref validation drift:** GitHub can run a PR check against the synthetic +merge ref, where a current-base deletion makes a cited path or coverage entry +appear missing even though it exists on the PR head. Compare the failing path +with `origin/` before changing the PR. Update a genuinely stale +manifest or coverage entry, but do not rebase solely to satisfy this check. + **Cancelled concurrency duplicates:** A required check with `conclusion=cancelled`, 0s job durations, unexpanded `${{ matrix.* }}` job names, or a "Canceling since a higher priority waiting request ..." annotation @@ -26,6 +101,14 @@ 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 `). +**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 +`issue_comment` and `pull_request_target` runs use the trusted default-branch +workflow definition, not a newly synced PR workflow. Compare the run's +workflow path, event, and head metadata with that definition; land a workflow +fix on the default branch, then reproduce with a fresh comment. + **Semantic PR title transport failures:** If `pr-title` / `amannn/action-semantic-pull-request` fails with transport or response parsing errors such as `invalid json response body ... Unexpected end of JSON input`, diff --git a/.agents/skills/pr-fixup/references/merge-conflicts.md b/.agents/skills/pr-fixup/references/merge-conflicts.md index 3578e4a459..7c7f7b76b6 100644 --- a/.agents/skills/pr-fixup/references/merge-conflicts.md +++ b/.agents/skills/pr-fixup/references/merge-conflicts.md @@ -37,6 +37,10 @@ If `git ls-files -u` prints entries, or conflict markers are present in tracked git merge --no-edit origin/ ``` Use `git rebase origin/` only when the branch already uses a rebase-style history or the user asks for it. If a rebase is used and succeeds, the push later may need `git push --force-with-lease`. + Never add `--no-verify` to this merge or to the commit that completes it unless + 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. 4. Confirm the conflict is gone before continuing: ```bash diff --git a/.agents/skills/pr/SKILL.md b/.agents/skills/pr/SKILL.md index 6c4df2f7e3..4b8592e316 100644 --- a/.agents/skills/pr/SKILL.md +++ b/.agents/skills/pr/SKILL.md @@ -46,8 +46,16 @@ explicitly requests task tracking. 3. **Remote state:** Confirm the branch has an upstream and the remote contains the local `HEAD`. If not, run `/push` before creating the PR. + **CI artifact bootstrap:** If the diff introduces a CI-consumed registry tag + or artifact that a workflow on this branch must publish first, follow that + publisher's documented bootstrap path before rerunning consumer checks. + Request explicit user approval before a `workflow_dispatch` or other action + writes a shared registry, and verify the target tag/digest exists. + 4. **Screenshots — capture and validate before publication.** For a UI-visible - change, capture fresh desktop and mobile screenshots before creating the PR. + change, capture fresh screenshots for every affected viewport before creating + the PR. When the changed surface is structurally absent on another viewport, + record that rationale instead of capturing an unrelated screen. Use synthetic or redacted data, validate the assets, and compress PNGs using the recipe in step 7. If capture is impossible, report the concrete blocker and stop before PR publication. For non-UI changes, record that screenshots @@ -79,12 +87,30 @@ explicitly requests task tracking. 6. **If ready (not draft):** For GitHub, do not begin `/pr-fixup` until any 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 desktop and mobile assets captured and validated in step 4 through the host-specific flow before treating the PR as complete — do not wait to be asked. +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:** - - Reuse only fresh entries from `apps/web/.pr-assets/manifest.json`. + - If `npx --no-install 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. + - 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. - 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 + before the Playwright separator and use a `mobile-*.spec.ts` filename: + `pnpm e2e:run --project mobile-chrome e2e/tests//mobile-.spec.ts`. + `--project` after `--` is only a Playwright argument and leaves the + runner on its default Chromium project. + - In managed Docker E2E, write capture files directly to the mounted + workspace path `/work/apps/web/.pr-assets/.png`, not + `testInfo.outputPath(...)`, which is container-local. After the run, + confirm the host has the files before inspecting or compressing them. - Reject any asset that exposes secrets, authentication tokens, or personally identifiable information and stop for recapture. - Compress PNGs before embedding. Prefer a system `pngquant`; when it is @@ -120,14 +146,16 @@ required screenshot embedding in step 7 is complete. ``` `gh pr edit` fails on this repo (GraphQL touches the deprecated Projects-classic API). Fall back to REST — build the payload with `jq --rawfile`, never by hand-escaping shell strings: ```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}' > /tmp/pr-body-payload.json - rtk proxy jq empty /tmp/pr-body-payload.json + rtk proxy jq -n --rawfile body "" '{body: $body}' > "$PAYLOAD" + rtk proxy jq empty "$PAYLOAD" else - jq -n --rawfile body "" '{body: $body}' > /tmp/pr-body-payload.json - jq empty /tmp/pr-body-payload.json + jq -n --rawfile body "" '{body: $body}' > "$PAYLOAD" + jq empty "$PAYLOAD" fi - gh api --method PATCH repos/:owner/:repo/pulls/ --input /tmp/pr-body-payload.json + 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 @@ -141,8 +169,8 @@ required screenshot embedding in step 7 is complete. is created, so never PATCH a body reconstructed from the creation-time template or a stale `/tmp/pr-body.md`. Before every post-creation update: - 1. Fetch the current body from GitHub and use that exact response as the - merge base: + 1. Fetch the current body from GitHub and keep that pristine live response + as the merge base: `gh pr view --json body --jq .body > /tmp/pr-body-latest.md`. 2. Change only the section owned by this operation. For screenshots, wrap the section in `` and @@ -150,9 +178,13 @@ required screenshot embedding in step 7 is complete. `## Screenshots` block if those markers are not present). Preserve every byte outside that range, including ` ... `. - 3. Re-fetch the body immediately before PATCH and compare it with the - snapshot used in step 1. If it changed, discard the pending payload, - re-fetch, and merge again; do not overwrite the newer body. + 3. Build a separate merged candidate, then re-fetch the live body immediately + 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. 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 6809dc47d1..c66ba72111 100644 --- a/.agents/skills/push/SKILL.md +++ b/.agents/skills/push/SKILL.md @@ -20,7 +20,9 @@ delivery. - `--fixup` — after pushing, begin `/pr-fixup` in the same conversation. -> **Note:** This skill only uses `git push`. GitHub CLI dependency is indirect via `/pr-fixup`. +> **Note:** This skill normally uses `git push`. It uses `gh pr view` before a +> no-upstream fallback so a checked-out fork PR is not accidentally pushed to +> its base repository. ## Your task @@ -43,8 +45,30 @@ Push the already committed branch to its remote. ```bash git push ``` - If the branch has no upstream, use `git push -u origin HEAD` rather than - transcribing the branch name. Then verify + If the branch has no upstream, first look up the current branch's PR. Treat + an unavailable or ambiguous lookup as a stop condition; only a confirmed + absence of a PR permits the ordinary `origin` fallback. For a confirmed PR, + require its `headRefName` to match the checked-out local branch, then inspect + its delivery target: + ```bash + gh pr view --json isCrossRepository,headRepositoryOwner,headRepository,headRefName,headRefOid,maintainerCanModify + ``` + For a cross-repository PR, the PR head owner can push its own fork directly. + When acting as a base-repository maintainer on somebody else's fork, + `maintainerCanModify` must be `true`; do not treat that field as a universal + fork-owner gate. Push the exact current commit to the reported head owner, + repository, and ref. Before using the HTTPS URL, configure Git to use the + authenticated `gh` credential helper: + ```bash + gh auth setup-git + git push "https://github.com//.git" "HEAD:refs/heads/" + ``` + 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`. + + Only for a non-PR or same-repository PR, use `git push -u origin HEAD` + rather than transcribing the branch name. Then verify `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 diff --git a/.agents/skills/tdd/SKILL.md b/.agents/skills/tdd/SKILL.md index 2548f2b216..54afba0f10 100644 --- a/.agents/skills/tdd/SKILL.md +++ b/.agents/skills/tdd/SKILL.md @@ -50,6 +50,15 @@ 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. +### Cross-layer contracts + +When adding or renaming a field that crosses backend, WebSocket, and frontend +boundaries, use `rg` to trace every producer, DTO, store upsert or partial +merge, reconnect/readiness handler, and consumer before editing. Add focused +coverage at the affected boundaries, including refresh/reconnect and an update +that omits the field, so a partial payload cannot silently discard an existing +value. + ### Concurrent and event-driven behavior Test ordering-sensitive behavior with channels, barriers, or controllable fakes; @@ -63,6 +72,11 @@ signals, or queue ownership. Cover stale events acting after a replacement operation begins, cancellation/retry ownership, and at-most-once delivery when they apply. Run affected Go packages with `-race`. +When delayed state has a lifecycle owner, pair the boundary tests: disposal +before the threshold must cancel timers and emit nothing later; disposal or +replacement after publication must immediately clear externally observable +state; and stale callbacks must not mutate the replacement. + ## Steps ### 1. RED — Write a failing test @@ -70,7 +84,10 @@ they apply. Run affected Go packages with `-race`. 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) -4. If it passes immediately, the test is not testing new behavior — revise it +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, + make no production change, and run the focused suite. For bug fixes, use the Prove-It Pattern: reproduce the bug with a failing test before changing production code. A fix without a regression test is not complete unless the change is explicitly untestable and you say why. @@ -99,6 +116,10 @@ Run the targeted tests named in the task file and report their results. Commit and open the PR after all affected task checks pass; do not add broad local verification by default. +After the final production-code edit, rerun every new or changed regression +test and report the exact command and result. A prior green run does not cover +a later patch. + ## Testing anti-patterns **Don't test implementation details:** @@ -117,6 +138,9 @@ verification by default. **Don't use incomplete mocks:** - Mock the complete data structure as it exists in reality, not just fields your test uses. Partial mocks hide bugs when downstream code accesses omitted fields. +- When adding a named export to a shared module, search for full-module + `vi.mock()` factories and add the export to each factory. Focused tests can + pass while a full suite fails on an out-of-date module shape. **Never swallow errors in tests:** - `try/catch` that silently ignores failures in test helpers or setup — these hide real failures. @@ -127,7 +151,8 @@ verification by default. ## Red flags - Writing production code before a failing test exists — delete and start over -- Test passes on first run — it tests nothing new, revise the test +- Test passes on first run — revise it, except for clearly labelled + reviewer-requested test-only contract coverage - Fixing a test to make it pass instead of fixing the production code - Large jumps — multiple behaviors implemented between test runs - Skipping the refactor step diff --git a/.agents/skills/verify/SKILL.md b/.agents/skills/verify/SKILL.md index 0c7c113d24..d3ed3b5f12 100644 --- a/.agents/skills/verify/SKILL.md +++ b/.agents/skills/verify/SKILL.md @@ -18,6 +18,13 @@ suites in the same checkout; wait for a running command to finish before starting another. Capture targeted failure evidence instead of repeatedly rerunning a broad suite. +If the execution relay terminates an otherwise healthy long-running check, +rerun that check **once** in one named, monitored `tmux` session with an exit +sentinel. Do not run a parallel retry. Record the log path and result, then +close the session after collecting the sentinel. If that retry fails or its +result cannot be recovered, return the evidence as a blocked or failed +verification report. + - If verify passes cleanly: report success. - If verify fails: fix the reported cause in the same conversation, rerun the relevant targeted checks, commit if needed, and restart verification. @@ -149,6 +156,10 @@ evidence. Report the command, quiet-log path and relevant lines, likely files, and a concise remediation recommendation. Fix only after the failure is understood, then rerun the selected checks. +When the evidence points to a test-owned resource release/reacquisition race +(for example, loopback-port rebinding), report a deterministic-test remediation +packet. Do not retry indefinitely and do not edit the test in the verify role. + If formatting changes files after commit, review and report the formatter diff, invalidate the hook receipt and verified-commit state, then continue only to collect useful evidence. Commit the formatter result and run fresh verification diff --git a/.github/AGENTS.md b/.github/AGENTS.md new file mode 100644 index 0000000000..ef5e3f007d --- /dev/null +++ b/.github/AGENTS.md @@ -0,0 +1,19 @@ +# GitHub Actions security + +Apply this guidance whenever editing `.github/**`. + +- `issue_comment` and `pull_request_target` jobs execute the workflow from the + trusted default/base branch. Syncing a PR does not change an existing + comment-triggered run. +- Never check out or execute an untrusted PR head in a secret-bearing or + comment-triggered job before a privileged agent or action. Keep the default + branch checkout, use `persist-credentials: false`, and treat PR metadata, + diffs, and files as untrusted data. +- Constrain capabilities in the tool policy, not prompt text alone. For PR-file + reads, prefer a small GET-only helper bound to the event PR/head; validate + 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. +- 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/.github/CLAUDE.md b/.github/CLAUDE.md new file mode 120000 index 0000000000..47dc3e3d86 --- /dev/null +++ b/.github/CLAUDE.md @@ -0,0 +1 @@ +AGENTS.md \ No newline at end of file diff --git a/AGENTS.md b/AGENTS.md index 73ed0335cd..cbabbd9535 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -89,7 +89,7 @@ For multiline Markdown issue or PR bodies, write the body to a file and pass it with the relevant `gh ... --body-file ` option. Do not send escaped newlines through `--body`; GitHub will render them literally. -For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary ` for checks and unresolved-thread state, `scripts/pr-state --comment ` for a full review-comment body, `scripts/pr-resolve list ` for actionable unresolved review threads, and `scripts/pr-resolve reply ""` to reply, resolve, and react in one call. +For PR review/fixup workflows, prefer the repo helpers before manually querying GitHub/GraphQL: `scripts/pr-state --summary ` for checks and unresolved-thread state, `scripts/pr-state --comment ` for a full review-comment body, `scripts/pr-resolve list ` for actionable unresolved review threads, and `scripts/pr-resolve reply --body-file ` to reply, resolve, and react in one call. Use `--body-file` whenever a reply has Markdown or shell-sensitive characters (especially backticks, `$`, or quotes); it prevents shell substitution from corrupting the GitHub reply. When a Kandev system message references an MCP tool that is not visible in the active tool list, use the runtime's tool discovery mechanism, such as `tool_search` when available, before falling back to a less specific workflow. Some task messaging and platform helpers are exposed on demand. diff --git a/apps/backend/AGENTS.md b/apps/backend/AGENTS.md index 4d9fbd5fb1..233a29ef61 100644 --- a/apps/backend/AGENTS.md +++ b/apps/backend/AGENTS.md @@ -146,6 +146,8 @@ replace state verification, installation association, or HMAC verification. **Agent Runtime** (`internal/agent/runtime/`) is the single seam for launching, resuming, stopping, and observing agent executions. ADR 0004 introduced this in Phase 1 of task-model-unification. The public surface is `runtime.Runtime` (`runtime.go`); a thin facade (`facade.go`) delegates to a `Backend` (satisfied by `*lifecycle.Manager`). +**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. **Lifecycle Manager** (`internal/agent/runtime/lifecycle/`) manages agent instances under the runtime: @@ -159,6 +161,11 @@ replace state verification, installation association, or HMAC verification. **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. + **agentctl** is an HTTP server that: - Runs inside Docker containers or as standalone process - Manages agent subprocess via stdin/stdout (ACP protocol) @@ -215,6 +222,7 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - Provider pattern for DI; stderr for logs, stdout for ACP only. - Pass context through chains; event bus for cross-component comm. +- **Event-bus wildcard parity:** New NATS wildcard subscriptions must verify equivalent `MemoryEventBus` semantics in `go test ./internal/events/bus`. - **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. - **Execution access:** Workspace-oriented handlers (files, shell, inference, ports, vscode, LSP) MUST use `GetOrEnsureExecution(ctx, sessionID)` — it recovers from backend restarts by creating executions on-demand. Only use `GetExecutionBySessionID` for operations that require a running agent process (prompt, cancel, mode). - **Task lifecycle events:** Any code path that mutates a task row must publish via the event bus (`task.created` / `task.updated` / `task.deleted`) — either by going through `Service.CreateTask` / `UpdateTask` / `DeleteTask` / `ArchiveTask`, or by calling `publishTaskEvent` (or one of the `Publish*` helpers in `service_events.go`) directly. Walking `repository.TaskRepository` straight bypasses event publishing and breaks WS-driven UI like the All-Workflows kanban view. `HandoffService`'s cascade methods learned this the hard way — they now require a `TaskEventPublisher` wired via `SetTaskEventPublisher`. New cascade / bulk / cleanup paths must follow the same pattern. @@ -223,7 +231,19 @@ Client (WS) ← Orchestrator ← Lifecycle Manager ←──── stream update - **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. - **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`. Changing only the count can leave + higher parent indexes behind and create a malformed Git config block. - **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. ### Goroutine ownership and leak testing @@ -247,6 +267,8 @@ Every long-running goroutine must have a single owner with explicit start and st ## Schema & migrations (SQLite repository) +`internal/task/repository/sqlite` also runs against PostgreSQL: avoid unguarded SQLite-only `rowid`, JSON, or date syntax and add an environment-gated PostgreSQL behavior test for every changed dialect-sensitive method (schema replay is insufficient). + `initSchema()` in `internal/task/repository/sqlite/base_schema.go` runs the `init*Schema` (CREATE TABLE) steps **before** `runMigrations()`. The table-creation DDL uses `CREATE TABLE IF NOT EXISTS`, so on an **existing** database it is a no-op and never adds columns to a table that is already present. **Rule:** when you add a column to an existing table, add it **only** via an idempotent `ADD COLUMN` migration in `runMigrations()` (`base_migrations.go`), never by editing the table's `CREATE TABLE` alone. Anything that *references* that new column — an index, a backfill `UPDATE`, a partial-index predicate — must live in `runMigrations()` **after** the `ADD COLUMN`, not in the `init*Schema` DDL. Putting a `CREATE INDEX ... (new_col)` in the schema-init block crashes existing DBs with `no such column: new_col`, because schema init runs before the migration that adds the column. diff --git a/apps/backend/internal/agent/acpdbg/ops_test.go b/apps/backend/internal/agent/acpdbg/ops_test.go index 9959dbaa1e..1bacf4bc84 100644 --- a/apps/backend/internal/agent/acpdbg/ops_test.go +++ b/apps/backend/internal/agent/acpdbg/ops_test.go @@ -1,10 +1,14 @@ package acpdbg import ( + "bufio" "context" + "encoding/json" "os" + "path/filepath" "strings" "testing" + "time" ) func TestNewRunnerUsesResolvedTemporaryWorkdirForProtocolRequests(t *testing.T) { @@ -24,6 +28,101 @@ func TestNewRunnerUsesResolvedTemporaryWorkdirForProtocolRequests(t *testing.T) } } +func TestProbe_DefaultWorkdirReachesSessionNew(t *testing.T) { + t.Setenv("ACPDBG_HELPER_PROCESS", "1") + + ctx, cancel := context.WithTimeout(context.Background(), 5*time.Second) + defer cancel() + runner, err := NewRunner(ctx, filepath.Join(t.TempDir(), "frames.jsonl"), RunConfig{ + AgentID: "helper", + Command: []string{os.Args[0], "-test.run=^TestACPDBGHelperProcess$"}, + }) + if err != nil { + t.Fatalf("NewRunner() error = %v", err) + } + defer runner.Close("completed") + + if _, err := Probe(ctx, runner); err != nil { + t.Fatalf("Probe() error = %v", err) + } +} + +func TestACPDBGHelperProcess(t *testing.T) { + if os.Getenv("ACPDBG_HELPER_PROCESS") != "1" { + return + } + + scanner := bufio.NewScanner(os.Stdin) + encoder := json.NewEncoder(os.Stdout) + for scanner.Scan() { + var request map[string]any + if err := json.Unmarshal(scanner.Bytes(), &request); err != nil { + os.Exit(1) + } + switch request["method"] { + case "initialize": + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "result": map[string]any{"protocolVersion": 1}, + }) + case "session/new": + params, _ := request["params"].(map[string]any) + cwd, _ := params["cwd"].(string) + workdir, err := os.Getwd() + if err != nil { + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "error": map[string]any{"message": "could not get test workdir"}, + }) + continue + } + resolvedCWD, err := filepath.EvalSymlinks(cwd) + if err != nil { + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "error": map[string]any{"message": "could not resolve requested cwd"}, + }) + continue + } + resolvedWorkdir, err := filepath.EvalSymlinks(workdir) + if err != nil { + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "error": map[string]any{"message": "could not resolve test workdir"}, + }) + continue + } + if resolvedCWD != resolvedWorkdir { + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "error": map[string]any{"message": "cwd does not match test workdir"}, + }) + continue + } + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "result": map[string]any{"sessionId": "session-1"}, + }) + default: + _ = encoder.Encode(map[string]any{ + "jsonrpc": "2.0", + "id": request["id"], + "error": map[string]any{ + "code": -32601, + "message": "method not found", + }, + }) + } + } + os.Exit(0) +} + func TestSessionLoadParamsIncludeChangedWorkdir(t *testing.T) { t.Parallel() diff --git a/apps/backend/internal/agentctl/AGENTS.md b/apps/backend/internal/agentctl/AGENTS.md index 1136271454..c4bb9d6ae3 100644 --- a/apps/backend/internal/agentctl/AGENTS.md +++ b/apps/backend/internal/agentctl/AGENTS.md @@ -145,6 +145,13 @@ Both accept any value `time.ParseDuration` accepts (`30m`, `2h`, `500ms`, …). `instance.Manager.buildMcpServerConfigs` calls `exec.LookPath` on every stdio MCP `Command` before passing the list to the agent. Entries whose command can't be resolved are dropped with a warn log (URL-transport MCPs always pass through). This prevents the `/snap/bin/brave` repro in GH issue #1247 — an MCP whose binary was uninstalled after config save no longer causes a permanently broken child process to be spawned every session. +## Testing + +PTY-backed shell tests run on Windows through the supported ConPTY path. Skip +only assertions that require Unix-specific PTY semantics; keep environment and +configuration-merge tests platform-neutral so they remain covered on every +supported OS. + ## Further scoped notes - `server/api/AGENTS.md` — reverse-proxy body rewriting (`Accept-Encoding`) and iframe-blocking header stripping. diff --git a/apps/web/AGENTS.md b/apps/web/AGENTS.md index 1a2f91fefc..cdd29e00aa 100644 --- a/apps/web/AGENTS.md +++ b/apps/web/AGENTS.md @@ -26,6 +26,7 @@ import { Dialog } from "@kandev/ui/dialog"; - Use `useTouchDrawer` when a hover/popover disclosure needs a coarse-pointer `Drawer` alternative. Width-based phone composition and pointer-based disclosure behavior are related but not interchangeable. - Existing Radix DropdownMenu and ContextMenu surfaces receive inset, safe-area-aware bottom-sheet treatment below 640px in `app/globals.css`. Reuse those primitives for contextual actions and add focused coverage for long or nested menus instead of creating a parallel mobile menu. - Mobile capability parity does not require desktop layout parity. Load `/mobile-parity` for the Kandev surface decision guide, mobile design contract, and verification requirements. +- For container-query layouts, test just below and at the breakpoint plus the component's minimum supported width; preserve action order and prove clipped controls remain hit-target reachable. ## Data Flow Pattern (Critical) @@ -68,6 +69,19 @@ lib/api/domains/ # API clients **Hydration:** Go injects `window.__KANDEV_BOOT_PAYLOAD__` into the SPA shell before React mounts. `lib/state/hydration/merge-strategies.ts` has `deepMerge()`, `mergeSessionMap()`, `mergeLoadingState()` to avoid overwriting live client state. Pass `activeSessionId` to protect active sessions. +**Same-resource refreshes:** Preserve an already-loaded snapshot while +refreshing the same workspace or resource. Surface the refresh separately for +the local spinner, busy control, or disabled action; clear data only for the +initial load or an identity change. A deferred hook test should show stale +content with `loading=true`, and desktop/mobile E2E should keep the content +visible during refresh. + +**Live update provenance:** A WebSocket snapshot may omit optional fields that +the boot payload or hydrated store already established. Merge those fields in +provenance order—live payload, persisted session metadata, then existing store +value—rather than replacing known immutable metadata with `undefined`. Add a +handler regression test for an omitted-field update. + For rebasing or finishing PRs written against the old Next.js runtime, follow [`docs/nextjs-spa-migration.md`](../../docs/nextjs-spa-migration.md). @@ -205,3 +219,6 @@ When you hit a limit, extract a helper function, custom hook, or sub-component. solely because `mouseenter` or `pointerMove` failed in jsdom. - In Playwright tests, avoid strict locators that assume only one `terminal-panel` or `.xterm` exists. Mobile and dockview layouts can mount multiple terminal instances; scope to the active panel or use `.first()` / `.last()` deliberately with a comment or helper. - Shared E2E helpers that inspect mounted React/DOM internals must be scoped to the active panel/container, not global selectors, because hidden or stale mounted panels can coexist in dock/mobile layouts. +- When seeded data can create multiple tasks or repositories, scope actions to + the seeded/API ID or an exact labelled container. Do not use `.first()` on a + generic action locator unless list order is the tested contract. diff --git a/apps/web/e2e/README.md b/apps/web/e2e/README.md index f524877f16..4a9d7e1638 100644 --- a/apps/web/e2e/README.md +++ b/apps/web/e2e/README.md @@ -15,7 +15,7 @@ Playwright-based end-to-end tests. Each Playwright worker spawns its own real Go ## Playwright projects -The suite is split into four projects. Pick one with `--project=`. +The suite is split into five projects. Pick one with `--project=`. ### `routing` @@ -30,6 +30,16 @@ it directly with: pnpm e2e --project=routing ``` +### `auth` + +Runs `tests/auth/**` in an isolated desktop worker. These specs restart their +backend with authentication enabled, so `chromium` intentionally excludes them. +Select the project explicitly; otherwise Playwright reports `No tests found`: + +```sh +pnpm e2e --project=auth tests/auth/auth-lifecycle.spec.ts +``` + ### `chromium` (default) The everyday surface — runs in every CI shard. Excludes the heavyweight `containers` specs and the mobile specs. @@ -85,6 +95,7 @@ This project used to be named `docker`. It was renamed to `containers` once SSH | `pnpm e2e --project=containers` | Run container-backed tests (needs Docker). | | `pnpm e2e --project=mobile-chrome` | Run mobile responsive tests. | | `pnpm e2e --project=routing` | Run provider-mutating Office routing tests. | +| `pnpm e2e --project=auth` | Run auth-isolated tests. | | `E2E_DEBUG=1 pnpm e2e` | Surface Docker build output + extra logging. | Common flags: `--shard=1/4`, `-g "fragment of test name"`, `--repeat-each=3` (flake hunting). @@ -97,6 +108,7 @@ Common flags: `--shard=1/4`, `-g "fragment of test name"`, `--repeat-each=3` (fl pnpm e2e:run # auto: docker if the daemon + CI image are available, else host pnpm e2e:run --shards 3 # 3 shards concurrently (isolated containers, or host procs with distinct ports) pnpm e2e:run tests/chat/foo.spec.ts # extra args pass straight through to Playwright +pnpm e2e:run --project auth tests/auth/auth-lifecycle.spec.ts pnpm e2e:run --host --no-build -- --grep "x" # force host, skip rebuild, forward flags after -- pnpm e2e:docker # force the docker CI image (full isolation from a host dev instance) pnpm e2e:clean # remove build/test artifacts, incl. root-owned ones from prior docker runs @@ -134,7 +146,7 @@ The SSH executor specifically has no mock controller. Tests use a real Docker-ho ## Adding a new spec 1. Pick a directory under `tests/` (or create one for a new feature). -2. Decide which project it belongs to. Anything that needs Docker → `tests/docker/` or `tests/ssh/` (lands in `containers`). Anything mobile-specific → name it `mobile-*.spec.ts`. Otherwise it joins `chromium` automatically. +2. Decide which project it belongs to. Anything that needs Docker → `tests/docker/` or `tests/ssh/` (lands in `containers`). Auth-isolated specs belong in `tests/auth/` and need `--project=auth`; provider-mutating Office routing specs use `routing`. Anything mobile-specific → name it `mobile-*.spec.ts`. Otherwise it joins `chromium` automatically. 3. Import the right test base: - `import { test, expect } from "../../fixtures/test-base";` for normal tests. - `import { test, expect } from "../../fixtures/docker-test-base";` for Docker executor tests. diff --git a/apps/web/e2e/helpers/pr-asset-capture.ts b/apps/web/e2e/helpers/pr-asset-capture.ts index 7fb4365971..68f5c3544a 100644 --- a/apps/web/e2e/helpers/pr-asset-capture.ts +++ b/apps/web/e2e/helpers/pr-asset-capture.ts @@ -18,21 +18,13 @@ export type AssetManifest = { const DEFAULT_OUTPUT_DIR = path.resolve(__dirname, "../../.pr-assets"); const FRAMES_DIR = ".frames"; +const MANIFEST_LOCK_DIR = ".manifest.lock"; +const MANIFEST_LOCK_RETRY_MS = 10; +const MANIFEST_LOCK_TIMEOUT_MS = 5_000; +const MANIFEST_LOCK_STALE_MS = 30_000; const RECORDING_FPS = 5; const RECORDING_INTERVAL = 1000 / RECORDING_FPS; -// Ensures the output directory is wiped exactly once per process (per e2e run). -let _cleanedForRun = false; - -function cleanOutputDir(outputDir: string): void { - if (_cleanedForRun) return; - _cleanedForRun = true; - if (fs.existsSync(outputDir)) { - fs.rmSync(outputDir, { recursive: true }); - } - fs.mkdirSync(outputDir, { recursive: true }); -} - function sanitizeName(name: string): string { return name .toLowerCase() @@ -65,6 +57,47 @@ function writeManifest(outputDir: string, manifest: AssetManifest): void { fs.writeFileSync(path.join(outputDir, "manifest.json"), JSON.stringify(manifest, null, 2)); } +function waitForManifestLock(): void { + const signal = new Int32Array(new SharedArrayBuffer(4)); + Atomics.wait(signal, 0, 0, MANIFEST_LOCK_RETRY_MS); +} + +function reclaimStaleManifestLock(lockDir: string): boolean { + try { + const ageMs = Date.now() - fs.statSync(lockDir).mtimeMs; + if (ageMs < MANIFEST_LOCK_STALE_MS) return false; + fs.rmdirSync(lockDir); + return true; + } catch (error) { + if ((error as NodeJS.ErrnoException).code === "ENOENT") return true; + throw error; + } +} + +function withManifestLock(outputDir: string, write: () => T): T { + const lockDir = path.join(outputDir, MANIFEST_LOCK_DIR); + const deadline = Date.now() + MANIFEST_LOCK_TIMEOUT_MS; + while (true) { + try { + fs.mkdirSync(lockDir); + break; + } catch (error) { + if ((error as NodeJS.ErrnoException).code !== "EEXIST") throw error; + if (reclaimStaleManifestLock(lockDir)) continue; + if (Date.now() >= deadline) { + throw new Error(`timed out waiting for PR asset manifest lock: ${lockDir}`); + } + waitForManifestLock(); + } + } + + try { + return write(); + } finally { + fs.rmdirSync(lockDir); + } +} + export class PrAssetCapture { private readonly page: Page; private readonly outputDir: string; @@ -82,7 +115,7 @@ export class PrAssetCapture { this.testSlug = getTestSlug(testFile); if (this.enabled) { - cleanOutputDir(this.outputDir); + ensureDir(this.outputDir); } } @@ -184,10 +217,12 @@ export class PrAssetCapture { this.recordingInterval = null; } - const manifest = readManifest(this.outputDir); - // Remove stale entries from this test - manifest.assets = manifest.assets.filter((a) => a.test !== `${this.testSlug}.spec.ts`); - manifest.assets.push(...this.assets); - writeManifest(this.outputDir, manifest); + withManifestLock(this.outputDir, () => { + const manifest = readManifest(this.outputDir); + // Remove stale entries from this test. + manifest.assets = manifest.assets.filter((a) => a.test !== `${this.testSlug}.spec.ts`); + manifest.assets.push(...this.assets); + writeManifest(this.outputDir, manifest); + }); } } diff --git a/apps/web/e2e/scripts/run-e2e.sh b/apps/web/e2e/scripts/run-e2e.sh index 53a4367c4e..f6a70b5d9d 100755 --- a/apps/web/e2e/scripts/run-e2e.sh +++ b/apps/web/e2e/scripts/run-e2e.sh @@ -14,9 +14,11 @@ # on the host and are served by the Go backend. # • Runs N shards concurrently (--shards N): N isolated containers in docker # mode, or N host processes with distinct E2E_PORT_OFFSET + output dirs. -# • Never leaves root-owned junk in the repo: points Playwright output at a -# container-local dir. `clean` -# removes any pre-existing root-owned artifacts via a throwaway container. +# • Never leaves root-owned junk in the repo: points normal Playwright output +# at a container-local dir. With CAPTURE_PR_ASSETS, captures are written to +# the mounted workspace and ownership is restored after the run. `clean` +# removes any pre-existing root-owned normal artifacts via a throwaway +# container. # # Usage: # run-e2e.sh [run] [options] [-- ] # default subcommand: run @@ -59,16 +61,37 @@ resolve_runtime_image() { # --- clean: remove build/test artifacts, including root-owned ones from prior # docker runs (host user can't rm root-owned files, so shell out to a container). clean_artifacts() { - log "cleaning e2e/test-results, blob-report, and /tmp shard logs" - rm -rf "$WEB_DIR"/e2e/test-results* "$WEB_DIR"/e2e/blob-report 2>/dev/null + log "cleaning e2e/test-results, blob-report, PR assets, and /tmp shard logs" + rm -rf "$WEB_DIR"/e2e/test-results* "$WEB_DIR"/e2e/blob-report "$WEB_DIR"/.pr-assets 2>/dev/null rm -f /tmp/e2e-host-shard-*.log /tmp/e2e-docker-shard-*.log 2>/dev/null if docker_up; then docker run --rm -v "$WEB_DIR":/web alpine sh -c \ - 'rm -rf /web/e2e/test-results* /web/e2e/blob-report' \ + 'rm -rf /web/e2e/test-results* /web/e2e/blob-report /web/.pr-assets' \ 2>/dev/null || true fi } +prepare_pr_assets() { + [[ -n "${CAPTURE_PR_ASSETS:-}" ]] || return + log "preparing PR asset directory" + rm -rf "$WEB_DIR/.pr-assets" 2>/dev/null || true + if [[ -e "$WEB_DIR/.pr-assets" ]]; then + docker_up || die "cannot remove root-owned PR assets without Docker; run e2e:clean where Docker is available" + docker run --rm -v "$WEB_DIR":/web alpine sh -c 'rm -rf /web/.pr-assets' \ + >/dev/null || die "failed to remove root-owned PR assets" + fi + mkdir -p "$WEB_DIR/.pr-assets" +} + +restore_pr_asset_ownership() { + [[ -n "${CAPTURE_PR_ASSETS:-}" ]] || return 0 + docker run --rm -v "$WEB_DIR":/web alpine sh -c "chown -R $(id -u):$(id -g) /web/.pr-assets" \ + >/dev/null || { + log "failed to restore PR asset ownership" + return 1 + } +} + build_fe() { log "building Vite web assets" ( cd "$REPO_ROOT/apps" && pnpm --filter @kandev/web build ) \ @@ -168,6 +191,7 @@ STRICT_ENV=() # HOST mode # --------------------------------------------------------------------------- run_host() { + prepare_pr_assets [[ "$DO_BUILD" == 1 ]] && { build_backend_host; build_fe; build_plugin_package; } local base_args=(playwright test --config e2e/playwright.config.ts --project="$PROJECT") if [[ "$SHARDS" -le 1 ]]; then @@ -198,10 +222,13 @@ run_docker() { local img; img="$(resolve_runtime_image)" log "runtime image: $img" clean_artifacts + prepare_pr_assets [[ "$DO_BUILD" == 1 ]] && { build_backend_for_docker "$img"; build_fe; build_plugin_package; } local strict_flag=() [[ "$STRICT" == 1 ]] && strict_flag=(-e KANDEV_E2E_WS_ASSERT=1) + local capture_flag=() + [[ -n "${CAPTURE_PR_ASSETS:-}" ]] && capture_flag=(-e CAPTURE_PR_ASSETS) local pw="git config --global --add safe.directory /work 2>/dev/null; cd /work/apps/web && pnpm exec playwright test --config e2e/playwright.config.ts --project=\"$PROJECT\"" run_one() { # $1=shard index (or 0 for unsharded) @@ -210,13 +237,19 @@ run_docker() { docker run --rm --ipc=host \ -v "$REPO_ROOT":/work -w /work/apps/web \ ${strict_flag[@]+"${strict_flag[@]}"} \ + ${capture_flag[@]+"${capture_flag[@]}"} \ -e NODE_OPTIONS=--dns-result-order=ipv4first \ -e PLAYWRIGHT_BROWSERS_PATH=/ms-playwright \ "$img" \ bash -lc "$pw $shardflag --output=$out --reporter=list \"\$@\"" e2e-runner ${PW_ARGS[@]+"${PW_ARGS[@]}"} } - if [[ "$SHARDS" -le 1 ]]; then run_one 0; return $?; fi + if [[ "$SHARDS" -le 1 ]]; then + run_one 0 + local rc=$? + restore_pr_asset_ownership || return 1 + return $rc + fi log "running $SHARDS isolated containers" local pids=() rc=0 i for ((i=1; i<=SHARDS; i++)); do @@ -227,6 +260,7 @@ run_docker() { for ((i=1; i<=SHARDS; i++)); do printf ' shard %s: %s\n' "$i" "$(grep -Eo '[0-9]+ (passed|failed|flaky)' "/tmp/e2e-docker-shard-$i.log" | tr '\n' ' ')" >&2 done + restore_pr_asset_ownership || return 1 log "docker shard logs: /tmp/e2e-docker-shard-*.log" return $rc } diff --git a/apps/web/lib/e2e/pr-asset-capture.test.ts b/apps/web/lib/e2e/pr-asset-capture.test.ts new file mode 100644 index 0000000000..9cdf9df229 --- /dev/null +++ b/apps/web/lib/e2e/pr-asset-capture.test.ts @@ -0,0 +1,126 @@ +import type { Page } from "@playwright/test"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { afterEach, describe, expect, it } from "vitest"; + +import { PrAssetCapture } from "../../e2e/helpers/pr-asset-capture"; + +const originalCaptureSetting = process.env.CAPTURE_PR_ASSETS; +const MANIFEST_FILE = "manifest.json"; + +afterEach(() => { + if (originalCaptureSetting === undefined) { + delete process.env.CAPTURE_PR_ASSETS; + return; + } + process.env.CAPTURE_PR_ASSETS = originalCaptureSetting; +}); + +describe("PrAssetCapture", () => { + it("replaces stale entries without removing assets from another worker", async () => { + process.env.CAPTURE_PR_ASSETS = "1"; + const testFile = "mobile-capture.spec.ts"; + const outputDir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-pr-assets-")); + const page = { + screenshot: async ({ path: screenshotPath }: { path: string }) => { + fs.writeFileSync(screenshotPath, "screenshot"); + }, + } as unknown as Page; + try { + const existingAsset = path.join(outputDir, "desktop.png"); + fs.writeFileSync(existingAsset, "existing asset"); + fs.writeFileSync( + path.join(outputDir, MANIFEST_FILE), + JSON.stringify({ + generated_at: "2026-01-01T00:00:00.000Z", + assets: [ + { + name: "stale", + type: "screenshot", + format: "png", + file: "mobile-capture--stale.png", + test: testFile, + }, + { + name: "desktop", + type: "screenshot", + format: "png", + file: "desktop.png", + test: "desktop-capture.spec.ts", + }, + ], + }), + ); + + const capture = new PrAssetCapture(page, `/tmp/${testFile}`, { outputDir }); + await capture.screenshot("fresh"); + + capture.flush(); + + expect(fs.existsSync(existingAsset)).toBe(true); + const manifest = JSON.parse(fs.readFileSync(path.join(outputDir, MANIFEST_FILE), "utf-8")); + expect(manifest.assets.map((asset: { file: string }) => asset.file).sort()).toEqual([ + "desktop.png", + "mobile-capture--fresh.png", + ]); + } finally { + fs.rmSync(outputDir, { recursive: true, force: true }); + } + }); + + it("reclaims a stale manifest lock before flushing", async () => { + process.env.CAPTURE_PR_ASSETS = "1"; + const outputDir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-pr-assets-")); + const page = { + screenshot: async ({ path: screenshotPath }: { path: string }) => { + fs.writeFileSync(screenshotPath, "screenshot"); + }, + } as unknown as Page; + try { + const lockDir = path.join(outputDir, ".manifest.lock"); + fs.mkdirSync(lockDir); + const staleAt = new Date(Date.now() - 31_000); + fs.utimesSync(lockDir, staleAt, staleAt); + + const capture = new PrAssetCapture(page, "/tmp/stale-lock.spec.ts", { outputDir }); + await capture.screenshot("recovered"); + capture.flush(); + + expect(fs.existsSync(lockDir)).toBe(false); + const manifest = JSON.parse(fs.readFileSync(path.join(outputDir, MANIFEST_FILE), "utf-8")); + expect(manifest.assets.map((asset: { file: string }) => asset.file)).toEqual([ + "stale-lock--recovered.png", + ]); + } finally { + fs.rmSync(outputDir, { recursive: true, force: true }); + } + }); + + it("retains assets flushed by separate captures", async () => { + process.env.CAPTURE_PR_ASSETS = "1"; + const outputDir = fs.mkdtempSync(path.join(os.tmpdir(), "kandev-pr-assets-")); + const page = { + screenshot: async ({ path: screenshotPath }: { path: string }) => { + fs.writeFileSync(screenshotPath, "screenshot"); + }, + } as unknown as Page; + try { + const desktop = new PrAssetCapture(page, "/tmp/desktop-capture.spec.ts", { outputDir }); + const mobile = new PrAssetCapture(page, "/tmp/mobile-capture.spec.ts", { outputDir }); + await desktop.screenshot("desktop"); + await mobile.screenshot("mobile"); + + desktop.flush(); + mobile.flush(); + + const manifest = JSON.parse(fs.readFileSync(path.join(outputDir, MANIFEST_FILE), "utf-8")); + expect(manifest.assets.map((asset: { file: string }) => asset.file).sort()).toEqual([ + "desktop-capture--desktop.png", + "mobile-capture--mobile.png", + ]); + } finally { + fs.rmSync(outputDir, { recursive: true, force: true }); + } + }); +}); diff --git a/scripts/dev-isolated b/scripts/dev-isolated index 63ffe7ed75..525af3fe42 100755 --- a/scripts/dev-isolated +++ b/scripts/dev-isolated @@ -286,11 +286,11 @@ WEB_STARTED=0 if [[ "$WANT_WEB" -eq 1 ]]; then echo "dev-isolated: starting web (vite dev) on :$WEB_PORT ..." env \ - NEXT_PUBLIC_KANDEV_API_PORT="$BACKEND_PORT" \ + VITE_KANDEV_API_PORT="$BACKEND_PORT" \ KANDEV_API_BASE_URL="$BACKEND_URL" \ PORT="$WEB_PORT" \ HOSTNAME="127.0.0.1" \ - NEXT_PUBLIC_KANDEV_DEBUG="true" \ + VITE_KANDEV_DEBUG="true" \ setsid pnpm -C "$REPO_ROOT/apps" --filter @kandev/web dev >"$WEB_LOG" 2>&1 & WEB_PID=$! echo "$WEB_PID" >"$WEB_PIDFILE" diff --git a/scripts/pr-state b/scripts/pr-state index 18ccddb7ba..71547ff660 100755 --- a/scripts/pr-state +++ b/scripts/pr-state @@ -4,14 +4,17 @@ # Usage: # scripts/pr-state [--summary] [--all] [--trusted-reviewer ] # scripts/pr-state --comment +# scripts/pr-state --job-log # # Raw output: # JSON object with: -# - pr: { number, branch } +# - pr: { number, branch, is_cross_repository, 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 }] # - checks_head_sha: # - checks_snapshot_complete: +# - approval_required_runs: [{ run_id, name, workflow, status, conclusion, head_sha, url }] # - review_threads: [{ thread_id, is_resolved, path, comment_id, comment_created_at, author, body_preview }] # - hidden_unresolved_threads: [{ thread_id, comment_id, author, path, body_preview }] # - unresolved_review_thread_count: @@ -27,7 +30,8 @@ # # Summary output (--summary): # JSON object with: -# - pr: { number, branch } +# - pr: { number, branch, is_cross_repository, 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 }] # - pending_checks: [{ name, workflow, status, run_id, job_id, details_url, target_url }] @@ -35,6 +39,7 @@ # - passed_check_count: # - checks_head_sha: # - checks_snapshot_complete: +# - approval_required_runs: [{ run_id, name, workflow, status, conclusion, head_sha, url }] # - unresolved_review_thread_count: # - filtered_review_thread_count: # - unresolved_threads: [{ thread_id, comment_id, author, path, body_preview }] @@ -52,6 +57,10 @@ # JSON object for one PR review comment: # - comment_id, author, body, path, line, original_line, commit_id, html_url, created_at, updated_at # +# Job-log output (--job-log): +# Bounded context around the most likely failing test or step. The GitHub +# endpoint may return either plain text or a ZIP archive. +# # By default, comments/reviews/review threads are limited to items created after # the PR head commit. Checks are already scoped by GitHub to the current head. # Pass --all to include historical comments and reviews. @@ -69,6 +78,7 @@ INCLUDE_ALL=0 SUMMARY=0 TRUSTED_REVIEWER='' COMMENT_ID='' +JOB_LOG_ID='' PR='' while [[ $# -gt 0 ]]; do case "$1" in @@ -94,6 +104,13 @@ while [[ $# -gt 0 ]]; do COMMENT_ID=$1 shift ;; + --job-log) + shift + [[ $# -gt 0 && -z "${JOB_LOG_ID:-}" && -z "${PR:-}" ]] || usage 1 + [[ "$1" =~ ^[0-9]+$ ]] || usage 1 + JOB_LOG_ID=$1 + shift + ;; -h | --help) usage 0 ;; @@ -119,7 +136,11 @@ done [ $# -eq 0 ] || usage -if [[ -n "${COMMENT_ID:-}" ]]; then +if [[ -n "${COMMENT_ID:-}" && -n "${JOB_LOG_ID:-}" ]]; then + usage 1 +elif [[ -n "${COMMENT_ID:-}" ]]; then + [[ -z "${PR:-}" && "$SUMMARY" == "0" && "$INCLUDE_ALL" == "0" ]] || usage +elif [[ -n "${JOB_LOG_ID:-}" ]]; then [[ -z "${PR:-}" && "$SUMMARY" == "0" && "$INCLUDE_ALL" == "0" ]] || usage else [ -n "${PR:-}" ] || usage @@ -130,6 +151,7 @@ pr_json='{}' checks_json='[]' checks_head_sha='' checks_snapshot_complete=false +approval_required_runs_json='[]' review_threads_json='[]' hidden_unresolved_threads_json='[]' reviews_json='[]' @@ -187,11 +209,58 @@ emit_comment_json() { }' <<<"$comment_json" } +emit_job_log() { + local job_id=$1 + local repo_json owner repo archive_file log_file magic + archive_file="$(mktemp)" + log_file="$(mktemp)" + + repo_json="$(gh_json repo view --json owner,name)" || { + rm -f "$archive_file" "$log_file" + printf 'failed to resolve repository identity\n' >&2 + return 1 + } + owner="$(jq -r '.owner.login // ""' <<<"$repo_json")" + repo="$(jq -r '.name // ""' <<<"$repo_json")" + if [[ -z "$owner" || -z "$repo" ]]; then + rm -f "$archive_file" "$log_file" + printf 'failed to resolve repository identity\n' >&2 + return 1 + fi + if ! gh_json api "repos/$owner/$repo/actions/jobs/$job_id/logs" >"$archive_file"; then + rm -f "$archive_file" "$log_file" + printf 'failed to fetch job log %s\n' "$job_id" >&2 + return 1 + fi + + magic="$(head -c 4 "$archive_file" || true)" + if [[ "$magic" == $'PK\003\004' ]]; then + if ! unzip -p "$archive_file" >"$log_file"; then + rm -f "$archive_file" "$log_file" + printf 'failed to unpack job log %s\n' "$job_id" >&2 + return 1 + fi + else + mv "$archive_file" "$log_file" + archive_file='' + fi + + if ! rg -n -i -C 4 '(Test timeout|Error:|FAIL|failed|Process completed)' "$log_file" | sed -n '1,240p'; then + tail -n 80 "$log_file" + fi + rm -f "$archive_file" "$log_file" +} + if [[ -n "${COMMENT_ID:-}" ]]; then emit_comment_json "$COMMENT_ID" exit $? fi +if [[ -n "${JOB_LOG_ID:-}" ]]; then + emit_job_log "$JOB_LOG_ID" + exit $? +fi + extract_checks_json() { local status_json=$1 printf '%s\n' "$status_json" | jq -c ' @@ -491,6 +560,7 @@ summarize_state_json() { since, checks_head_sha, checks_snapshot_complete, + approval_required_runs: (.approval_required_runs // []), failed_checks: [ effective_checks[] | select(terminal_failure_status and bad_conclusion) @@ -538,6 +608,39 @@ summarize_state_json() { ' } +fetch_approval_required_runs_json() { + local owner=$1 repo=$2 pr_number=$3 head_sha=$4 + if [[ "$is_cross_repository" != "true" || -z "$owner" || -z "$repo" || -z "$pr_number" || -z "$head_sha" ]]; then + printf '%s\n' '[]' + return + fi + + local runs_pages + if ! runs_pages="$(gh_json api --paginate "repos/$owner/$repo/actions/runs?event=pull_request&head_sha=$head_sha&per_page=100")"; then + return 1 + fi + + jq -cs --argjson pr "$pr_number" --arg head "$head_sha" ' + [.[].workflow_runs[]? + | select( + .status == "completed" + and .conclusion == "action_required" + and .head_sha == $head + and any(.pull_requests[]?; .number == $pr and .head.sha == $head) + ) + | { + run_id: (.id | tostring), + name: (.name // "unknown"), + workflow: (.path // null), + status: (.status // null), + conclusion: (.conclusion // null), + head_sha: (.head_sha // null), + url: (.html_url // null) + } + ] | sort_by(.run_id | tonumber) + ' <<<"$runs_pages" +} + fetch_head_commit_json() { local owner=$1 local repo=$2 @@ -658,13 +761,22 @@ comments_json='[]' status_json='[]' resolved_pr_number='' is_cross_repository=null -if pr_view_json="$(gh_json pr view "$PR" --json number,headRefName,headRefOid,isCrossRepository,statusCheckRollup,comments,url)"; then +if pr_view_json="$(gh_json pr view "$PR" --json number,headRefName,headRefOid,headRepositoryOwner,headRepository,maintainerCanModify,isCrossRepository,statusCheckRollup,comments,url)"; then 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" repo_name="$(jq -r '.url | capture("github.com/(?[^/]+)/(?[^/]+)/pull/[0-9]+").repo // ""' <<<"$pr_view_json")" fi - pr_json="$(jq -c '{number: .number, branch: (.headRefName // "unknown"), is_cross_repository: (if has("isCrossRepository") then .isCrossRepository else null end)}' <<<"$pr_view_json")" + pr_json="$(jq -c '{ + number: .number, + branch: (.headRefName // "unknown"), + is_cross_repository: (if has("isCrossRepository") then .isCrossRepository else null end), + head_repository_owner: (.headRepositoryOwner.login // null), + head_repository_name: (.headRepository.name // null), + head_ref_name: (.headRefName // null), + head_ref_oid: (.headRefOid // null), + maintainer_can_modify: (.maintainerCanModify // null) + }' <<<"$pr_view_json")" is_cross_repository="$(jq -c 'if has("isCrossRepository") then .isCrossRepository else null end' <<<"$pr_view_json")" comments_json="$(jq -c '.comments // []' <<<"$pr_view_json")" status_json="$(jq -c '.statusCheckRollup // []' <<<"$pr_view_json")" @@ -679,6 +791,11 @@ else add_error "pr_view" "gh pr view failed" 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" +fi + since='' if [[ "$INCLUDE_ALL" != "1" && -n "$repo_owner" && -n "$repo_name" && -n "$resolved_pr_number" ]]; then if since_json="$(fetch_head_commit_json "$repo_owner" "$repo_name" "$resolved_pr_number")"; then @@ -765,6 +882,7 @@ trap cleanup_state_tmp EXIT printf '%s\n' "$pr_json" >"$state_tmp_dir/pr.json" printf '%s\n' "$since_json" >"$state_tmp_dir/since.json" printf '%s\n' "$checks_json" >"$state_tmp_dir/checks.json" +printf '%s\n' "$approval_required_runs_json" >"$state_tmp_dir/approval_required_runs.json" printf '%s\n' "$review_threads_json" >"$state_tmp_dir/review_threads.json" printf '%s\n' "$hidden_unresolved_threads_json" >"$state_tmp_dir/hidden_unresolved_threads.json" printf '%s\n' "$reviews_json" >"$state_tmp_dir/reviews.json" @@ -776,6 +894,7 @@ state_json="$(jq -cn \ --slurpfile pr "$state_tmp_dir/pr.json" \ --slurpfile since "$state_tmp_dir/since.json" \ --slurpfile checks "$state_tmp_dir/checks.json" \ + --slurpfile approval_required_runs "$state_tmp_dir/approval_required_runs.json" \ --slurpfile review_threads "$state_tmp_dir/review_threads.json" \ --slurpfile hidden_unresolved_threads "$state_tmp_dir/hidden_unresolved_threads.json" \ --slurpfile reviews "$state_tmp_dir/reviews.json" \ @@ -792,6 +911,7 @@ state_json="$(jq -cn \ checks: $checks[0], checks_head_sha: (if $checks_head_sha == "" then null else $checks_head_sha end), checks_snapshot_complete: $checks_snapshot_complete, + approval_required_runs: $approval_required_runs[0], review_threads: $review_threads[0], hidden_unresolved_threads: $hidden_unresolved_threads[0], unresolved_review_thread_count: $unresolved_review_thread_count, diff --git a/scripts/pr-state.test.sh b/scripts/pr-state.test.sh index f634044a20..28d5d87ab0 100644 --- a/scripts/pr-state.test.sh +++ b/scripts/pr-state.test.sh @@ -69,6 +69,11 @@ if [[ "${GH_FAIL_COMMENT:-0}" == "1" && "$1" == "api" && "$2" == repos/kdlbs/kan exit 1 fi +if [[ "${GH_FAIL_APPROVAL_RUNS:-0}" == "1" && "$*" == *"repos/kdlbs/kandev/actions/runs?event=pull_request&head_sha=abc123&per_page=100"* ]]; then + echo "workflow runs api failed" >&2 + exit 1 +fi + if [[ "$1" == "repo" && "$2" == "view" ]]; then printf '{"owner":{"login":"kdlbs"},"name":"kandev"}\n' exit 0 @@ -92,6 +97,41 @@ JSON exit 0 fi +if [[ "$*" == *"repos/kdlbs/kandev/actions/runs?event=pull_request&head_sha=abc123&per_page=100"* ]]; then + if [[ "${GH_APPROVAL_REQUIRED:-0}" == "1" ]]; then + cat <<'JSON' +{"workflow_runs":[{"id":444,"name":"Backend tests","path":".github/workflows/ci.yml","event":"pull_request","status":"completed","conclusion":"action_required","head_sha":"abc123","html_url":"https://github.com/kdlbs/kandev/actions/runs/444","pull_requests":[{"number":123,"head":{"sha":"abc123"}}]},{"id":443,"name":"Stale run","path":".github/workflows/ci.yml","event":"pull_request","status":"completed","conclusion":"action_required","head_sha":"old-head","html_url":"https://github.com/kdlbs/kandev/actions/runs/443","pull_requests":[{"number":123,"head":{"sha":"old-head"}}]}]} +JSON + else + printf '%s\n' '{"workflow_runs":[]}' + fi + exit 0 +fi + +if [[ "$1" == "api" && "$2" == "repos/kdlbs/kandev/actions/jobs/55150000002/logs" ]]; then + log_file="$(mktemp)" + { + printf '%s\n' 'setup complete' + printf '%s\n' 'Test timeout: expected ready state' + printf '%s\n' 'stack frame one' + printf '%s\n' 'stack frame two' + printf '%s\n' 'stack frame three' + printf '%s\n' 'stack frame four' + printf '%s\n' 'stack frame five' + printf '%s\n' 'stack frame six' + printf '%s\n' 'unrelated tail line' + } >"$log_file" + if [[ "${GH_JOB_LOG_FORMAT:-plain}" == "zip" ]]; then + base64 -d <<'ZIP' +UEsDBBQAAAAAALip/lxqPXj2qgAAAKoAAAAHAAAAam9iLmxvZ3NldHVwIGNvbXBsZXRlClRlc3QgdGltZW91dDogZXhwZWN0ZWQgcmVhZHkgc3RhdGUKc3RhY2sgZnJhbWUgb25lCnN0YWNrIGZyYW1lIHR3bwpzdGFjayBmcmFtZSB0aHJlZQpzdGFjayBmcmFtZSBmb3VyCnN0YWNrIGZyYW1lIGZpdmUKc3RhY2sgZnJhbWUgc2l4CnVucmVsYXRlZCB0YWlsIGxpbmUKUEsBAhQDFAAAAAAAuKn+XGo9ePaqAAAAqgAAAAcAAAAAAAAAAAAAAIABAAAAAGpvYi5sb2dQSwUGAAAAAAEAAQA1AAAAzwAAAAAA +ZIP + else + cat "$log_file" + fi + rm -f "$log_file" + exit 0 +fi + if [[ "$*" == *"actions/workflows/opencode-code-review.yml/runs"* ]]; then if [[ "${GH_FAIL_TRUSTED_WORKFLOW:-0}" == "1" ]]; then echo "workflow api failed" >&2 @@ -132,6 +172,9 @@ if [[ "$1" == "pr" && "$2" == "view" && "$4" == "--json" ]]; then "number": 123, "headRefName": "feat/pr-state", "headRefOid": "${GH_CHECKS_HEAD:-abc123}", + "headRepositoryOwner": { "login": "${GH_HEAD_REPOSITORY_OWNER:-kdlbs}" }, + "headRepository": { "name": "${GH_HEAD_REPOSITORY_NAME:-kandev}" }, + "maintainerCanModify": ${GH_MAINTAINER_CAN_MODIFY:-true}, "isCrossRepository": ${GH_CROSS_REPOSITORY:-false}, "url": $pr_url, "comments": [ @@ -657,6 +700,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 "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" @@ -1151,7 +1195,9 @@ test_summary_mode_returns_compact_fixup_state() { json="$(<"$tmp/out.json")" 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 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" assert_jq "summary failed check" '.failed_checks[] | select(.name == "e2e") | .conclusion == "failure" and .run_id == "27340000002"' "$json" assert_jq "summary reports failed duplicate over skipped" '.failed_checks[] | select(.name == "opencode-review-fork") | .conclusion == "failure" and .run_id == "27340000007"' "$json" @@ -1173,6 +1219,66 @@ test_summary_mode_returns_compact_fixup_state() { pass "--summary returns compact fixup state" } +test_summary_reports_current_head_fork_approval_runs() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_CROSS_REPOSITORY=true GH_APPROVAL_REQUIRED=1 PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "summary reports only current-head approval run" \ + '.approval_required_runs | length == 1 and .[0].run_id == "444" and .[0].head_sha == "abc123" and .[0].conclusion == "action_required"' \ + "$json" + pass "--summary reports current-head fork approval runs" +} + +test_summary_reports_approval_run_fetch_failure() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local json + GH_CROSS_REPOSITORY=true GH_FAIL_APPROVAL_RUNS=1 PATH="$tmp/bin:$PATH" "$SCRIPT" --summary 123 >"$tmp/out.json" + json="$(<"$tmp/out.json")" + + assert_jq "summary reports approval-run fetch failure" \ + '.approval_required_runs == [] and any(.errors[]; .source == "approval_required_runs" and .message == "workflow runs API failed")' \ + "$json" + pass "--summary reports approval-run fetch failure" +} + +test_job_log_mode_emits_bounded_failure_context() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local output + output="$(PATH="$tmp/bin:$PATH" "$SCRIPT" --job-log 55150000002)" + + if ! grep -q 'Test timeout: expected ready state' <<<"$output"; then + fail "--job-log emits matching failure context" + fi + if grep -q 'unrelated tail line' <<<"$output"; then + fail "--job-log keeps output bounded" + fi + pass "--job-log emits bounded failure context" +} + +test_job_log_mode_unpacks_zip_responses() { + local tmp + make_tmp_dir tmp + make_mock_gh "$tmp/bin" + + local output + output="$(GH_JOB_LOG_FORMAT=zip PATH="$tmp/bin:$PATH" "$SCRIPT" --job-log 55150000002)" + if ! grep -q 'Test timeout: expected ready state' <<<"$output"; then + fail "--job-log unpacks zip responses" + fi + pass "--job-log unpacks zip responses" +} + test_summary_all_flag_includes_historical_unresolved_threads() { local tmp make_tmp_dir tmp @@ -1242,6 +1348,13 @@ test_comment_mode_rejects_incompatible_flags() { if PATH="$tmp/bin:$PATH" "$SCRIPT" --all --comment 111 >"$tmp/out.log" 2>&1; then fail "--comment rejects --all" fi + + if PATH="$tmp/bin:$PATH" "$SCRIPT" --comment 111 --job-log 55150000002 >"$tmp/out.log" 2>&1; then + fail "--comment rejects --job-log" + fi + if ! grep -q "scripts/pr-state --comment " "$tmp/out.log"; then + fail "--comment --job-log prints usage" + fi if ! grep -q "scripts/pr-state --comment " "$tmp/out.log"; then fail "--comment --all prints usage" fi @@ -1334,7 +1447,11 @@ test_graphql_failure_records_error_but_keeps_other_data 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_approval_run_fetch_failure test_summary_all_flag_includes_historical_unresolved_threads +test_job_log_mode_emits_bounded_failure_context +test_job_log_mode_unpacks_zip_responses test_comment_mode_returns_full_review_comment test_comment_mode_reports_fetch_failure test_comment_mode_rejects_incompatible_flags