chore: improve harness reliability (#1716)
This commit is contained in:
Carlos Florêncio
@@ -123,6 +123,30 @@ Without this, tests run against stale code and failures are misleading. `make bu
|
||||
4. Use page objects for common interactions; create new ones for new pages
|
||||
5. For GitHub features, use `apiClient.mockGitHub*()` methods to seed mock data
|
||||
|
||||
### Visual alignment regressions
|
||||
|
||||
For a UI change whose contract is a rendered size or alignment relationship,
|
||||
assert that relationship from the intended elements' bounding boxes rather than
|
||||
only asserting visibility. Scope locators to the affected toolbar, dialog, or
|
||||
panel so unrelated controls cannot make the assertion pass.
|
||||
|
||||
```typescript
|
||||
const metrics = page.getByTestId("task-metrics");
|
||||
const actions = page.getByTestId("task-actions");
|
||||
const [metricsBox, actionsBox] = await Promise.all([
|
||||
metrics.boundingBox(),
|
||||
actions.boundingBox(),
|
||||
]);
|
||||
|
||||
expect(metricsBox).not.toBeNull();
|
||||
expect(actionsBox).not.toBeNull();
|
||||
expect(metricsBox!.height).toBeCloseTo(actionsBox!.height, 1);
|
||||
```
|
||||
|
||||
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.
|
||||
|
||||
### 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.
|
||||
@@ -257,6 +281,8 @@ 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.
|
||||
- **Nested Escape controls.** If an inner panel inside a Radix Dialog handles Escape, intercept the key in capture phase and call both `preventDefault()` and `stopPropagation()` before dismissing the inner panel. A bubble-phase window handler runs after Radix can dismiss the outer dialog. Add a regression that asserts the inner panel collapses while the outer dialog remains open.
|
||||
- **Seed via API, assert via UI.** Use `apiClient` to set up preconditions quickly, but always verify the result by opening the page and checking the DOM.
|
||||
- **Workflow/session invariants.** For session-primary/profile behavior, prefer polling backend state with `apiClient.listTaskSessions(taskId)` for invariants such as `agent_profile_id`, `is_primary`, `state`, and session count, then add UI assertions as secondary evidence. UI tab markers can lag or be absent when the backend invariant is the behavior under test.
|
||||
- **Scope terminal helpers to the active panel.** Terminal/mobile helpers must avoid document-wide `.xterm` or `terminal-xterm-host` selectors because multiple terminal panels can be mounted at once. Scope locators through `data-testid="terminal-panel"` and prefer the visible or latest panel for `page.evaluate` helpers.
|
||||
|
||||
@@ -17,7 +17,7 @@ Wait for CI and code review to complete on a pull request, fix any failures or v
|
||||
- **`/e2e`** — Read for debugging guidance when E2E tests fail in CI. Covers test patterns, run commands, failure triage, and local reproduction.
|
||||
- **`/commit`** — Use for staging and committing fixes with Conventional Commits format.
|
||||
|
||||
Prefer the `pr-poller` and `verify` helpers when the runtime supports delegated helpers. If runtime policy forbids delegated helpers/subagents unless explicitly requested by the user, or either helper fails to start or initialize, treat helper delegation as unavailable and use the direct-command fallback. Do not substitute a generic/general subagent for polling; it returns too slowly for the 30s cadence and can look hung. If helper delegation is unavailable, follow the direct-command fallback sections below and keep the same output contract: compact CI state, compact review state, bounded polling, and full local verification before pushing.
|
||||
Prefer the registered `pr-poller` and `verify` helpers when the runtime supports delegated helpers. If runtime policy forbids delegated helpers/subagents unless explicitly requested by the user, either named helper is not registered, or a helper fails to start, initialize, or access the required GitHub network/shared-worktree resources, treat helper delegation as unavailable and use the direct-command fallback. Do not substitute a generic/general subagent for polling; it returns too slowly for the 30s cadence and can look hung, and may lack DNS or shared `.git` write access. If helper delegation is unavailable, follow the direct-command fallback sections below and keep the same output contract: compact CI state, compact review state, bounded polling, and full local verification before pushing.
|
||||
|
||||
## Context
|
||||
|
||||
@@ -81,6 +81,8 @@ If delegated polling is unavailable, gather the same information directly withou
|
||||
scripts/pr-state --summary <PR>
|
||||
```
|
||||
|
||||
If this command, `scripts/pr-resolve`, or another GitHub helper fails with a transient DNS/network error, retry the parent command with the runtime's required network escalation; treat state as unknown until it succeeds.
|
||||
|
||||
`scripts/pr-state` accepts flags before or after the PR (`scripts/pr-state --summary <PR>` and `scripts/pr-state <PR> --summary` both work). When parsing output with `jq`, prefer writing the JSON to a temp file first, then running `jq` against that file; this avoids `set -e` surprises and prevents stderr from corrupting a JSON pipe.
|
||||
|
||||
Prefer `scripts/pr-state --summary <PR>` for direct polling and `scripts/pr-resolve list <PR>` for review-thread state over raw `gh pr checks`. `gh pr checks` only reports CI/status checks; review bots can open unresolved threads after CI is green, and a checks-only poll will miss that blocker.
|
||||
@@ -122,13 +124,9 @@ scripts/pr-resolve list <PR>
|
||||
|
||||
If `unresolved_review_thread_count` is nonzero but `unresolved_threads` is empty, or if `hidden_unresolved_threads` is non-empty, run `scripts/pr-resolve list <PR>` before acting. Fetch each full body with `scripts/pr-state --comment <comment_id>` or `scripts/pr-resolve show <PR> <THREAD_ID_OR_COMMENT_ID>`; hidden threads can contain valid current blockers even when the current-head filtered view is empty. `pr-state` can briefly report total unresolved count from historical state while current-head unresolved threads are empty; `pr-resolve list` is the authoritative actionable thread set for fixup work. If `scripts/pr-state --summary` reports a nonzero unresolved count but `scripts/pr-resolve show <PR> <THREAD_ID>` says the listed thread is `resolved: true`, treat the summary as stale state. Wait briefly, rerun `scripts/pr-state --summary <PR>`, and do not reply again to an already-resolved thread.
|
||||
|
||||
Poll at a 30s cadence with a **20 min cap**. Prefer one-shot `scripts/pr-state --summary <PR>` checks, or a bounded command that can finish naturally, over long inline shell loops. In this environment, a running non-TTY loop may not receive Ctrl-C through `write_stdin`, so avoid `while sleep ...; do ...; done` polling in the main session. To wait one cadence without accidentally immediate-polling, use:
|
||||
Poll at a 30s cadence with a **20 min cap**. Prefer one-shot `scripts/pr-state --summary <PR>` checks, or a bounded command that can finish naturally, over long inline shell loops. In this environment, a running non-TTY loop may not receive Ctrl-C through `write_stdin`, so avoid `while sleep ...; do ...; done` polling in the main session. Run `sleep 30` and then a fresh `scripts/pr-state --summary <PR>` as separate commands; a combined command can capture blank output. Rerun a blank summary before interpreting it.
|
||||
|
||||
```bash
|
||||
bash -lc 'sleep 30; scripts/pr-state --summary <PR>'
|
||||
```
|
||||
|
||||
Stop early if any required check fails. If the cap hits and only E2E shards are still pending with no failures or unresolved comments, report "CI in progress" instead of continuing to watch indefinitely, and include the exact pending shard names from `pending_checks`. If the user explicitly asks to poll until CI is green, continue past the normal pending-E2E stopping point with bounded one-shot `sleep N; scripts/pr-state --summary <PR>` checks; stop immediately on any `failed_checks`, and continue until `failed_checks: []`, `pending_checks: []`, and `unresolved_review_thread_count: 0`. Do not run `gh pr checks --watch` in the main session unless the runtime can keep the watcher isolated and automatically clean it up. If you do use `gh pr checks --watch`, keep watching until the command exits; GitHub can expand matrix jobs after an initial aggregate "Build" check passes, so the first green build/lint/test rows are not necessarily terminal.
|
||||
Stop early if any required check fails. A `queued` or `in_progress` job caused by GitHub runner capacity is pending CI, not a failure: when the requested watch window or cap ends with no failures or unresolved comments, do not make speculative code changes. Report each pending job's exact name and status, plus its `details_url` or Actions run URL. If the user explicitly asks to poll until CI is green, continue past the normal pending-E2E stopping point with bounded one-shot `sleep N; scripts/pr-state --summary <PR>` checks; stop immediately on any `failed_checks`, and continue until `failed_checks: []`, `pending_checks: []`, and `unresolved_review_thread_count: 0`. Do not run `gh pr checks --watch` in the main session unless the runtime can keep the watcher isolated and automatically clean it up. If you do use `gh pr checks --watch`, keep watching until the command exits; GitHub can expand matrix jobs after an initial aggregate "Build" check passes, so the first green build/lint/test rows are not necessarily terminal.
|
||||
|
||||
For E2E-only pending phases, summarize counts before dumping long shard lists into context:
|
||||
|
||||
@@ -255,6 +253,11 @@ Then classify:
|
||||
- **Nitpick or preference** — subjective style not covered by linters. Skip unless the reviewer insists.
|
||||
- **Wrong or outdated** — misunderstands the code, refers to old state, or is technically incorrect. Push back with reasoning.
|
||||
|
||||
For sanitizer-schema feedback, inspect the installed package's `defaultSchema`
|
||||
and the production renderer before changing code. If the claimed element or
|
||||
attribute is already allowed, add or retain a regression that proves the
|
||||
behavior and reply with that evidence rather than duplicating a schema change.
|
||||
|
||||
For review comments about CI/workflow changes, account for `pull_request_target` trust boundaries. Source agent instructions from the base commit or trusted workflow content, not PR-head harness/config files; keep `GH_TOKEN` out of direct model execution when a trusted wrapper can post comments afterward; and validate model-produced comment targets against the computed diff scope before posting.
|
||||
|
||||
**Push back when:**
|
||||
@@ -316,7 +319,7 @@ for simple bodies, `--body-file` for multi-sentence replies, or plain text
|
||||
without Markdown code formatting. If quoting is awkward, shorten the reply and
|
||||
reference the commit SHA.
|
||||
|
||||
If `scripts/pr-resolve reply` fails mid-flight, run `scripts/pr-resolve show <PR> <THREAD_ID>` and `scripts/pr-resolve list <PR>` before retrying. Confirm whether the reply landed and only resolve/reaction failed, so you do not post a duplicate reply. When replying to several threads, check every command result; retry individual failures caused by transient GitHub, DNS, or API errors before treating that thread as still actionable.
|
||||
If `scripts/pr-resolve reply` fails mid-flight, first rerun `scripts/pr-resolve list <PR>` (with network escalation after a transient DNS/network error), then run `scripts/pr-resolve show <PR> <THREAD_ID>` before retrying. Confirm whether the reply landed and only resolve/reaction failed, so you do not post a duplicate reply. When replying to several threads, check every command result; retry individual failures caused by transient GitHub, DNS, or API errors before treating that thread as still actionable.
|
||||
|
||||
`scripts/pr-resolve list` only prints previews. Before deciding whether a thread is valid, stale, duplicate, or already fixed, fetch the full comment/thread body:
|
||||
|
||||
|
||||
@@ -58,6 +58,27 @@ headers` are infrastructure/package-registry issues, not app or test failures.
|
||||
If GitHub rejects `gh run rerun <run-id> --failed` while the workflow is still
|
||||
active, wait for the workflow/report job to finish and retry.
|
||||
|
||||
## Go Race-Suite Flakes
|
||||
|
||||
For a backend race-suite failure, extract the named failure from the saved log:
|
||||
|
||||
```bash
|
||||
rg -n '"Action":"fail"|--- FAIL:|goleak:' /tmp/kandev-job.log
|
||||
```
|
||||
|
||||
Reproduce that exact failure first, then exercise the affected package for
|
||||
suite interaction or leak-cleanup timing:
|
||||
|
||||
```bash
|
||||
go test -race ./path/to/package -run '^TestName$' -count=20
|
||||
go test -race ./path/to/package -count=3
|
||||
```
|
||||
|
||||
If a failed-job rerun reports a different, unrelated package or test, validate
|
||||
that second failure the same way and allow one additional failed-job rerun
|
||||
instead of changing unrelated PR code. Stop and fix code when the same failure
|
||||
reproduces locally or repeats in CI.
|
||||
|
||||
## E2E Failures
|
||||
|
||||
If any failing check is an E2E test:
|
||||
|
||||
@@ -5,9 +5,9 @@ description: Run format, typecheck, test, and lint across the monorepo. Use afte
|
||||
|
||||
# Verify
|
||||
|
||||
Delegate to the **`verify` subagent** to run the full verification pipeline (rebase, format, typecheck, test, lint) and fix any issues it finds when the runtime supports delegated helpers. The subagent runs on Sonnet, which is cheaper than the main session and well-suited to the mechanical run-parse-fix loop.
|
||||
Delegate to the **registered `verify` subagent** to run the full verification pipeline (rebase, format, typecheck, test, lint) and fix any issues it finds when the runtime supports delegated helpers. The subagent runs on Sonnet, which is cheaper than the main session and well-suited to the mechanical run-parse-fix loop. Do not substitute a generic agent: it may lack the required GitHub network access or shared-worktree write permissions.
|
||||
|
||||
If runtime policy forbids delegated helpers/subagents unless the user explicitly requested them, or the helper fails to start or initialize, treat delegation as unavailable and use the direct-command fallback below. Do not stop at a partial check just because delegation is unavailable.
|
||||
If runtime policy forbids delegated helpers/subagents unless the user explicitly requested them, the named helper is not registered, or the helper fails to start, initialize, or access the required Git/network/worktree resources, treat delegation as unavailable and use the direct-command fallback below. Do not stop at a partial check just because delegation is unavailable.
|
||||
|
||||
## What to do
|
||||
|
||||
|
||||
@@ -150,6 +150,14 @@ surface.
|
||||
hover/title/tooltip metadata. Tasks with no repository, or only a non-repo
|
||||
local folder, should not render a repo chip.
|
||||
|
||||
## Markdown safety
|
||||
|
||||
Any renderer that enables embedded raw HTML must pair `rehype-raw` immediately
|
||||
with `rehype-sanitize`, and enable that combination only on the intended
|
||||
surface. Do not broaden raw-HTML support to chat, comments, or other renderers
|
||||
without a separate security decision. Add regression coverage for permitted
|
||||
README markup and for stripping executable HTML and unsafe URLs.
|
||||
|
||||
## Code-quality limits
|
||||
|
||||
Enforced by `apps/web/eslint.config.mjs` (warnings, will become errors):
|
||||
|
||||
+16
-2
@@ -70,7 +70,14 @@ LOG="${LOG}.log"
|
||||
"$@" > "$LOG" 2>&1
|
||||
RC=$?
|
||||
|
||||
if [[ $RC -eq 0 ]]; then
|
||||
EXTRACT_LOG_ON_SUCCESS=false
|
||||
case "$TAG" in
|
||||
gh-run|ci|gh-run-view)
|
||||
EXTRACT_LOG_ON_SUCCESS=true
|
||||
;;
|
||||
esac
|
||||
|
||||
if [[ $RC -eq 0 && "$EXTRACT_LOG_ON_SUCCESS" != true ]]; then
|
||||
echo "exit=0 log=$LOG"
|
||||
exit 0
|
||||
fi
|
||||
@@ -110,7 +117,10 @@ grep_for_tag() {
|
||||
gh-run|ci|gh-run-view)
|
||||
# GitHub Actions log lines: "JOB\tSTEP\t...". Pull anything that
|
||||
# mentions error/failed/FAIL, plus the surrounding job context line.
|
||||
grep -nE '^\S+\s+\S+\s+.*(error|failed|FAIL|Error:|exit code|golangci-lint run|[A-Za-z0-9_./-]+\.go:[0-9]+:[0-9]+:|[0-9]+ issues?:|apps/backend/test-results\.json|Process completed with exit code 1)' "$PLAIN"
|
||||
# `gh run view --log-failed` exits zero when it retrieved a log, even
|
||||
# when that log contains failed tests. Include Go's JSON, test, and
|
||||
# goleak markers so those failures are surfaced in that success case.
|
||||
grep -nE '^\S+\s+\S+\s+.*(error|failed|FAIL|Error:|exit code|golangci-lint run|[A-Za-z0-9_./-]+\.go:[0-9]+:[0-9]+:|[0-9]+ issues?:|apps/backend/test-results\.json|Process completed with exit code 1)|"Action":"fail"|--- FAIL:|goleak:' "$PLAIN"
|
||||
;;
|
||||
*)
|
||||
grep -nE '(error|Error|FAIL|panic:|Failed|✘|✗)' "$PLAIN"
|
||||
@@ -123,6 +133,10 @@ MATCHES="$(grep_for_tag | head -n "$TAIL_LINES")"
|
||||
if [[ -n "$MATCHES" ]]; then
|
||||
echo "--- failures ($(printf '%s\n' "$MATCHES" | wc -l | tr -d ' ') lines, of tag=$TAG) ---"
|
||||
printf '%s\n' "$MATCHES"
|
||||
elif [[ $RC -eq 0 ]]; then
|
||||
# Successful GitHub commands with no failure markers remain compact.
|
||||
rm -f "$PLAIN"
|
||||
exit 0
|
||||
else
|
||||
# Fallback: grep found nothing useful; surface the last N lines verbatim.
|
||||
echo "--- no tag matches; tail $TAIL_LINES lines of $LOG ---"
|
||||
|
||||
@@ -0,0 +1,59 @@
|
||||
#!/usr/bin/env bash
|
||||
set -euo pipefail
|
||||
|
||||
ROOT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)"
|
||||
SCRIPT="$ROOT_DIR/scripts/run-quiet"
|
||||
TMP_DIR="$(mktemp -d)"
|
||||
|
||||
cleanup() {
|
||||
rm -rf "$TMP_DIR"
|
||||
}
|
||||
trap cleanup EXIT
|
||||
|
||||
pass() {
|
||||
printf 'ok - %s\n' "$1"
|
||||
}
|
||||
|
||||
fail() {
|
||||
printf 'not ok - %s\n' "$1" >&2
|
||||
exit 1
|
||||
}
|
||||
|
||||
cat >"$TMP_DIR/success-with-go-failure" <<'EOF'
|
||||
#!/usr/bin/env bash
|
||||
printf '%s\n' '{"Time":"2026-07-15T19:00:00Z","Action":"fail","Package":"example.com/project/pkg","Test":"TestLeaky"}'
|
||||
printf '%s\n' '--- FAIL: TestLeaky (0.00s)'
|
||||
printf '%s\n' 'goleak: Errors on successful test run: found unexpected goroutines'
|
||||
EOF
|
||||
chmod +x "$TMP_DIR/success-with-go-failure"
|
||||
|
||||
for tag in gh-run ci gh-run-view; do
|
||||
gh_output="$("$SCRIPT" "$tag" -- "$TMP_DIR/success-with-go-failure")"
|
||||
if grep -q '"Action":"fail"' <<<"$gh_output" && grep -q -- '--- FAIL: TestLeaky' <<<"$gh_output" && grep -q 'goleak:' <<<"$gh_output"; then
|
||||
pass "$tag extracts Go failures from a successful command"
|
||||
else
|
||||
fail "$tag extracts Go failures from a successful command"
|
||||
fi
|
||||
done
|
||||
|
||||
clean_output="$("$SCRIPT" gh-run -- bash -c 'printf "everything is fine\\n"')"
|
||||
if [[ "$(wc -l <<<"$clean_output" | tr -d ' ')" == "1" ]] && grep -q '^exit=0 log=' <<<"$clean_output"; then
|
||||
pass "gh-run keeps clean successful output compact"
|
||||
else
|
||||
fail "gh-run keeps clean successful output compact"
|
||||
fi
|
||||
|
||||
if failing_output="$("$SCRIPT" gh-run -- bash -c 'printf "%s\\n" "--- FAIL: TestBroken"; exit 1')"; then
|
||||
fail "gh-run preserves a nonzero command exit"
|
||||
elif grep -q -- '--- FAIL: TestBroken' <<<"$failing_output"; then
|
||||
pass "gh-run extracts failures from a nonzero command"
|
||||
else
|
||||
fail "gh-run extracts failures from a nonzero command"
|
||||
fi
|
||||
|
||||
normal_output="$("$SCRIPT" ordinary -- bash -c 'printf "ordinary success\\n"')"
|
||||
if [[ "$(wc -l <<<"$normal_output" | tr -d ' ')" == "1" ]] && grep -q '^exit=0 log=' <<<"$normal_output"; then
|
||||
pass "ordinary successful tags remain compact"
|
||||
else
|
||||
fail "ordinary successful tags remain compact"
|
||||
fi
|
||||
Reference in New Issue
Block a user