From 6831b808786a43c75980aa0ef90c571900e8bb6c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Carlos=20Flor=C3=AAncio?= Date: Tue, 14 Jul 2026 17:58:21 +0100 Subject: [PATCH] docs: improve harness and issue templates (#1681) --- .agents/skills/code-review/SKILL.md | 30 ++++++++++++++++++++++ .agents/skills/mobile-parity/SKILL.md | 2 ++ .agents/skills/pr-fixup/SKILL.md | 6 +++-- .agents/skills/tdd/SKILL.md | 13 ++++++++++ .agents/skills/verify/SKILL.md | 2 +- .github/ISSUE_TEMPLATE/bug_report.yml | 2 +- .github/ISSUE_TEMPLATE/feature_request.yml | 2 +- AGENTS.md | 10 ++++++++ 8 files changed, 62 insertions(+), 5 deletions(-) diff --git a/.agents/skills/code-review/SKILL.md b/.agents/skills/code-review/SKILL.md index d24c5bf560..a5dc38a1e9 100644 --- a/.agents/skills/code-review/SKILL.md +++ b/.agents/skills/code-review/SKILL.md @@ -21,6 +21,21 @@ Determine the right diff scope: - **Local changes**: `git diff --name-only` (unstaged) and `git diff --cached --name-only` (staged) - **PR review**: `git diff origin/...HEAD --name-only` to diff against the base branch +For an existing PR, first confirm the exact head under review. Do not assume the +local checkout is current: inspect the PR's base branch and head SHA, fetch the +head if needed, and use that immutable SHA in the diff. If the current PR head +cannot be fetched, say so rather than reporting a stale checkout as a review of +the current PR. + +Record the base and head SHA for each review round. A new contributor push starts +a new round: reassess prior findings and the verdict against the new head, and +verify checks or workflow results for that head rather than relying on a PR +number or author summary. + +Compare the PR description, checklist, and claimed manual validation with that +exact head, especially after a major refactor. Report stale claims separately; +they are not verification evidence for the current diff. + Read each changed file in full — understand surrounding code, not just the diff. Navigate callers, interfaces, and tests to understand changes end-to-end. For each file, identify which requirement or intent it serves. Flag any changes that don't map to the task — scope creep is a blocker. @@ -32,6 +47,8 @@ Before reviewing implementation details: - Check whether tests assert behavior, not implementation details. - Check whether the selected test level is appropriate: unit for pure logic, integration for boundaries, E2E for critical browser flows. - 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. - Treat missing tests for new or changed non-UI logic as a blocker unless the change is explicitly untestable and says why. ### 3. Review for issues @@ -50,6 +67,7 @@ Check every changed file for the following layers. Skip layers that don't apply **Architecture:** - Frontend: no direct data fetching in components (must go through store), shadcn imports from `@kandev/ui` not `@/components/ui/*` - Backend: provider pattern for DI, context passed through call chains, event bus for cross-component communication +- Search `docs/specs/` and `docs/decisions/` for the affected subsystem; flag an accepted spec or ADR that the change makes inaccurate - New abstractions justified — no over-engineering - Concerns cleanly separated (single responsibility) @@ -57,6 +75,11 @@ Check every changed file for the following layers. Skip layers that don't apply - Edge cases handled (empty input, nil/null, zero, max values) - Error paths covered and not silently swallowed - Race conditions or concurrency issues in concurrent code +- Async events carry an immutable identity when they can outlive the operation that created them; stale events cannot mutate a replacement operation +- Locks protect only the atomic ownership boundary and are not held across unbounded I/O or a full asynchronous operation +- Synchronous callbacks cannot re-enter a lock they already need; moving publication asynchronous also requires an immutable value snapshot, clear shutdown ownership, and protection against a delayed event changing successor state +- When a generation, token, or lease authorizes a side effect, validate and mutate within one critical section. Check every terminal path separately: success, raw error, cancellation, timeout, and disconnect. +- Detached goroutines have immutable snapshots and a real happens-before relationship before reading state that can otherwise transition underneath them **Performance:** - No N+1 queries (loop with individual DB calls) @@ -93,6 +116,13 @@ Check every changed file for the following layers. Skip layers that don't apply ### 4. Fix or report +When the user says not to post or modify the PR, do not make any GitHub +mutation: no fixes, comments, review submissions, or thread resolution. When +the user asks for a review only, or when reviewing an external contributor's +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. + - **Fix directly** any issues you can resolve confidently (dead code, unused imports, simple duplication, missing early returns) - **Report** issues that need the author's input — always explain *why* the issue matters and provide a concrete suggested fix diff --git a/.agents/skills/mobile-parity/SKILL.md b/.agents/skills/mobile-parity/SKILL.md index d46182b440..10b4f6bf4d 100644 --- a/.agents/skills/mobile-parity/SKILL.md +++ b/.agents/skills/mobile-parity/SKILL.md @@ -44,6 +44,7 @@ If task has no UI surface, say why this skill does not apply and continue. - In this repo, name mobile test files `mobile-*.spec.ts` so the `mobile-chrome` Playwright project picks them up automatically. - If no mobile project exists, configure the test or project with a realistic mobile viewport plus touch/mobile settings. - Cover mobile-specific controls such as drawers, overflow menus, stacked actions, responsive tables, or bottom controls. + - When a touch-only control is replaced or hidden, run `rg` across mobile E2E tests for the removed control. Replace every affected interaction with the intended gesture or alternate control, then run those tests together. 5. Verify visually and behaviorally. - Run the narrowest relevant viewport locally or with screenshots when possible. @@ -98,6 +99,7 @@ test.describe("feature on mobile", () => { - Mobile path has designed layout and interaction behavior. - Required controls are reachable by touch. - No required workflow depends on hover, wide viewport, or hidden desktop-only UI. +- Mobile E2E tests no longer invoke touch controls that the change replaced or hid. - Mobile Playwright coverage exists or absence is justified. - Focused rendered/visual verification was run for UI tweaks, or exact "not run" reason is reported. - Focused tests were run, or exact blocker is reported. diff --git a/.agents/skills/pr-fixup/SKILL.md b/.agents/skills/pr-fixup/SKILL.md index 115dee6c1c..0f091194c8 100644 --- a/.agents/skills/pr-fixup/SKILL.md +++ b/.agents/skills/pr-fixup/SKILL.md @@ -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, 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 `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. ## Context @@ -28,7 +28,9 @@ Prefer the `pr-poller` and `verify` helpers when the runtime supports delegated The first thing you do — before fetching PR state, before reading logs, before any fixes — is create a task list for the full pipeline. This is non-negotiable because it keeps you accountable to the process and lets the user see where you are. -Create these tasks immediately (use your task/todo tracking tool if available): +Create these tasks immediately in the current session's todo/checklist tool. Do +not create Kandev subtasks or persistent work items unless the user explicitly +requests task tracking: 1. **Gather PR state** — Use `pr-poller` when available; otherwise gather compact CI + bot review state via `scripts/pr-state`; always check PR mergeability and local conflict state 2. **Fix failing CI checks** — Read failing run logs (via `scripts/run-quiet gh-run -- gh run view ...`), fix issues, run E2E tests locally if needed diff --git a/.agents/skills/tdd/SKILL.md b/.agents/skills/tdd/SKILL.md index 485a900852..8e789a2dcd 100644 --- a/.agents/skills/tdd/SKILL.md +++ b/.agents/skills/tdd/SKILL.md @@ -43,6 +43,19 @@ 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. +### Concurrent and event-driven behavior + +Test ordering-sensitive behavior with channels, barriers, or controllable fakes; +do not use sleeps to create a race, except for a bounded, named delay that +models a known poll-loop schedule when synchronization would alter that +relationship. Pause at the ownership boundary, start the +competing operation, then release. Exercise the real delivery path where +practical, and prove the old interleaving fails before the fix. Assert both the +winner state and the untouched replacement state, including relevant buffers, +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`. + ## Steps ### 1. RED — Write a failing test diff --git a/.agents/skills/verify/SKILL.md b/.agents/skills/verify/SKILL.md index 7bee9bbd64..467f4c2134 100644 --- a/.agents/skills/verify/SKILL.md +++ b/.agents/skills/verify/SKILL.md @@ -7,7 +7,7 @@ description: Run format, typecheck, test, and lint across the monorepo. Use afte Delegate to the **`verify` subagent** to run the full verification pipeline (rebase, format, typecheck, test, lint) and fix any issues it finds when the runtime supports delegated helpers. The subagent runs on Sonnet, which is cheaper than the main session and well-suited to the mechanical run-parse-fix loop. -If runtime policy forbids delegated helpers/subagents unless the user explicitly requested them, treat delegation as unavailable and use the direct-command fallback below. Do not stop at a partial check just because delegation is unavailable. +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. ## What to do diff --git a/.github/ISSUE_TEMPLATE/bug_report.yml b/.github/ISSUE_TEMPLATE/bug_report.yml index 7892a1e56b..6ea783e1ec 100644 --- a/.github/ISSUE_TEMPLATE/bug_report.yml +++ b/.github/ISSUE_TEMPLATE/bug_report.yml @@ -1,7 +1,7 @@ name: Bug report description: Report a reproducible problem in Kandev. title: "[Bug]: " -labels: ["bug", "needs-triage"] +labels: ["bug"] body: - type: markdown attributes: diff --git a/.github/ISSUE_TEMPLATE/feature_request.yml b/.github/ISSUE_TEMPLATE/feature_request.yml index b308ab018b..2771bef373 100644 --- a/.github/ISSUE_TEMPLATE/feature_request.yml +++ b/.github/ISSUE_TEMPLATE/feature_request.yml @@ -1,7 +1,7 @@ name: Feature request description: Propose a Kandev improvement with clear user value. title: "[Feature]: " -labels: ["enhancement", "needs-triage"] +labels: ["enhancement"] body: - type: markdown attributes: diff --git a/AGENTS.md b/AGENTS.md index 182a8e8cf3..126a2aab49 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -83,6 +83,10 @@ Every code change must include tests for new or changed logic. Backend: `*_test. ### GitHub Operations Skills use `gh` CLI by default. If a `gh` command fails (not installed, not authenticated, etc.), use whatever GitHub tools are available in the environment (MCP GitHub tools, API tools, etc.) to accomplish the same operation. The goal is the same — the tool may differ. +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. 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. @@ -91,6 +95,12 @@ When a Kandev system message references an MCP tool that is not visible in the a When creating follow-up or delegated Kandev work from an active task, use `create_task_kandev` with `parent_id: "self"` when the work is related. That preserves workspace, workflow, repository, agent profile, and executor context from the current task. For genuinely unrelated top-level tasks, do not rely on workspace defaults when the user expects continuity; explicitly preserve the current task's `agent_profile_id` / `executor_profile_id`, or ask if the intended profile is ambiguous. +For remediation discovered while reviewing a PR that must start after the PR +merges, create a related subtask with `parent_id: "self"`, +`workspace_mode: "new_workspace"`, and the reviewed PR's base branch; otherwise +a same-repository subtask inherits the reviewed branch. Set `start_agent: false` +when the follow-up is intentionally queued until merge. + ### Third-party integrations Jira and Linear are the model (per-workspace credentials, 90s auth-health poller via `internal/integrations/healthpoll`, settings page with status banner). New integrations should **reuse the shared shapes** rather than copying either. Full layout, file conventions, and Jira-vs-Linear divergence notes in `apps/backend/internal/integrations/AGENTS.md` and the `/add-integration` skill — load either when scaffolding a new integration.