📎 Add better planner skill and improve testing doc

This commit is contained in:
Andrey Antukh 2026-07-13 09:50:43 +02:00
parent 33e18c72e2
commit 85dbf14344
13 changed files with 356 additions and 109 deletions

3
.gitignore vendored
View File

@ -99,5 +99,8 @@ opencode.json
/.playwright-mcp
/.devenv/mcp/
/opencode.json
/.opencode/plans
/.opencode/reports
/.opencode/prompts
/.codex/
/tools/__pycache__

View File

@ -1,6 +1,6 @@
---
name: planner
description: Read-only planning and architecture analysis for Penpot — produce a structured implementation plan (Context, Affected modules, Approach, Risks, Testing). Always output to the user; additionally save to plans/YYYY-MM-DD-<title>.md only when the calling agent has write permission.
description: Read-only planning and architecture analysis for Penpot — produce a structured implementation plan (Context, Affected modules, Approach, Risks, Testing). Always output to the user; additionally save to .opencode/plans/YYYY-MM-DD-<title>.md.
---
# Planner
@ -17,7 +17,7 @@ or modifies code.
names, and test strategy.
- The user asks "how would I implement X?" or "what's involved in fixing Y?".
- The user is about to start non-trivial work and wants a bite-sized task
breakdown (DRY, YAGNI, TDD, frequent commits).
breakdown.
Do **not** use this skill to actually implement anything — it is read-only.
@ -29,13 +29,8 @@ modify code.
You help users understand the codebase, design solutions, and create detailed
implementation plans that other agents or developers can execute. Document
everything they need to know: which files to touch for each task, code, tests,
docs they might need to check, and how to verify it. Give them the whole plan
as bite-sized tasks. DRY. YAGNI. TDD. Frequent commits.
Assume the implementer is a skilled developer, but knows almost nothing about
our toolset or problem domain. Assume they don't know good test design very
well.
everything they need to know: which files to touch for each task, code patterns,
tests, and how to verify correctness. Apply DRY and KISS principles.
Do **not** suggest commit messages or commit names anywhere in your plans or
responses — committing is the developer's responsibility.
@ -44,92 +39,233 @@ responses — committing is the developer's responsibility.
Before drafting any plan, work through the project's own guidance:
1. Read `AGENTS.md` (root) for the project-level rules.
2. Read `.serena/memories/critical-info.md` (or the equivalent entry point) to
identify which modules are affected.
1. Read `critical-info` (`.serena/memories/critical-info.md`) — the entry point
that describes the monorepo structure and module dependency graph.
2. From `critical-info`, identify which modules your task affects.
3. Read each affected module's core memory, e.g. `mem:frontend/core`,
`mem:backend/core`, `mem:common/core`, `mem:exporter/core`,
`mem:render-wasm/core`. Follow `mem:` references deeper as needed.
4. For frontend/backend work, check the relevant section's notes on lint,
format, and test commands so the plan can include them.
4. For each affected module, note its lint, format, and test commands so the
plan can include concrete verification steps.
Skipping this step is the #1 cause of incorrect or incomplete plans.
## The Planning Process
### Phase 1: Architecture Analysis
1. Read the spec, requirements, or feature request.
2. Analyze the codebase architecture and identify affected modules.
3. Read project conventions (starting with `critical-info` and module core
memories) before drafting.
4. Map dependencies between components (see the dependency graph in
`critical-info`).
5. Identify risks, edge cases, performance implications, and breaking changes.
### Phase 2: Task Breakdown
Implementation order follows the monorepo's dependency graph:
`frontend -> common`, `backend -> common`, `exporter -> common`,
`frontend -> render-wasm`. Build shared foundations first, then layer
consumers on top.
#### Slice Vertically
Instead of building all of common, then all of backend, then all of frontend —
build one complete feature path at a time:
```
Task 1: common data types + schema ← foundation
Task 2: backend RPC handler + persistence
Task 3: frontend UI component + API integration
```
Each vertical slice delivers working, testable functionality.
#### Write Tasks
Each task follows this structure:
```markdown
## Task [N]: [Short descriptive title]
**Description:** One paragraph explaining what this task accomplishes.
**Acceptance criteria:**
- [ ] [Specific, testable condition]
- [ ] [Specific, testable condition]
**Verification:**
- [ ] Tests pass (module-specific test command)
- [ ] Lint/formatter passes (module-specific check command)
**Dependencies:** [Task numbers this depends on, or "None"]
**Files likely touched:**
- `path/to/file.clj`
- `path/to/file_test.clj`
```
Replace "module-specific test command" with the actual commands for the module
(e.g. `clojure -M:dev:test` for backend/common, `npx shadow-cljs compile test && npx karma start` for frontend,
or the commands noted in the module's core memory).
#### Estimate Scope
| Size | Files | Scope |
|------|-------|-------|
| **XS** | 1 | Single function, config change, or schema tweak |
| **S** | 1-2 | One handler or component method |
| **M** | 3-5 | One vertical feature slice |
| **L** | 5-8 | Multi-component feature |
| **XL** | 8+ | **Too large — break it down further** |
If a task is L or larger, break it into smaller tasks. Agents perform best on
S and M tasks.
**When to break a task down further:**
- It would take more than one focused session
- You cannot describe the acceptance criteria in 3 or fewer bullet points
- It touches two or more independent subsystems
- You find yourself writing "and" in the task title (a sign it is two tasks)
#### Order and Checkpoints
Arrange tasks so that:
1. Dependencies are satisfied (build foundation first)
2. Each task leaves the system in a working state
3. Verification checkpoints occur after every 2-3 tasks
4. High-risk tasks are early (fail fast)
Add explicit checkpoints with the relevant module commands:
```markdown
## Checkpoint: After Tasks 1-3
- [ ] All tests pass (module-specific command)
- [ ] Lint/format passes (module-specific command)
- [ ] Core flow works end-to-end
- [ ] Review with human before proceeding
```
## Requirements
- Analyze the codebase architecture and identify affected modules.
- Read `AGENTS.md` and the memory system conventions before drafting.
- Read project conventions before drafting (start with `critical-info` and
affected module core memories).
- Break down complex features or bugs into atomic, actionable steps.
- Propose solutions with clear rationale, trade-offs, and sequencing.
- Identify risks, edge cases, performance implications, and breaking changes.
- Apply DRY and KISS principles to the proposed implementation.
- Define a testing strategy aligned with each affected module's tooling.
- Every task must have acceptance criteria and verification steps.
- Checkpoints must exist between major phases.
## Constraints
- You are **analysis-only** — never create, edit, or delete source code.
- The only file write you may attempt is the plan itself, and only when the
calling agent has write permission (see "Plan Output"). If the write is
denied, deliver the plan in the response and move on.
- The only file write you may attempt is the plan itself, saved to
`.opencode/plans/`.
- You do **not** run builds, tests, linters, or any commands that modify state.
- You do **not** create git commits or interact with version control.
- You do **not** execute shell commands beyond read-only searches (`rg`, `ls`,
`find`, `cat`, `bat`).
- You do **not** execute shell commands beyond read-only searches.
- Your output is a structured plan or analysis, ready for handoff to an
engineer agent or developer.
## Plan Output
## Output Format
The plan is always delivered in the response so the user sees it regardless
of which agent is running the skill.
Persistence is a **separate, best-effort step** that only runs when the
calling agent has `edit` write permission:
Additionally, save the plan to:
- **Has write permission** (e.g. `build`, `general`, `engineer`): in addition
to the in-response plan, save the plan to:
```
.opencode/plans/YYYY-MM-DD-<plan-one-line-title>.md
```
```
plans/YYYY-MM-DD-<plan-one-line-title>.md
```
Use today's date in the user's local timezone. The `<plan-one-line-title>`
slug is lowercase, hyphen-separated, and a short summary of the task
(e.g. `add-batch-get-profiles-for-file-comments`). Create the
`.opencode/plans/` directory if it does not exist.
Use today's date in the user's local timezone. The `<plan-one-line-title>`
slug is lowercase, hyphen-separated, and a short summary of the task
(e.g. `add-batch-get-profiles-for-file-comments`). Create the `plans/`
directory if it does not exist.
Always attempt the write. If the user explicitly provides a target file path,
use that path instead of the default.
- **No write permission** (e.g. the built-in `plan` agent, which denies
`edit`): do not attempt to write the file — the write tool will be
rejected. Just deliver the plan in the response. The user can copy it into
`plans/...` manually if they want it persisted.
### Plan Document Template
If the user explicitly provides a target file path, use that path instead of
the default `plans/YYYY-MM-DD-<slug>.md` (still subject to write permission).
```markdown
# Plan: [Feature/Project Name]
How to detect write permission: try the write. If it is denied, treat the
plan as response-only and proceed — do not retry, do not ask the user, and do
not mention the failed write in the response.
## Context
[One paragraph: what is the problem or feature request? Why is it needed?]
## Output Format
## Affected Modules
[Which modules of the monorepo are involved? Reference module paths and any
`mem:` memories that were consulted.]
Structure the plan as:
## Architecture Decisions
- [Key decision 1 and rationale]
- [Key decision 2 and rationale]
1. **Context** — What is the problem or feature request? Why is it needed?
2. **Affected modules** — Which parts of the codebase are involved? Reference
module paths and any `mem:` memories that were consulted.
3. **Approach** — Step-by-step implementation plan with file paths, function
names, and code shape where applicable. Group steps into atomic, ordered
tasks.
4. **Risks & considerations** — Edge cases, performance implications,
breaking changes, migration concerns, security implications.
5. **Testing strategy** — How to verify the implementation works correctly:
which test commands to run per module, what cases to cover, manual
verification steps, lint/format checks.
## Risks & Considerations
[Edge cases, performance implications, breaking changes, migration concerns,
security implications.]
Each step in **Approach** should be small enough to be reviewed and committed
independently. Cite exact file paths (`path/to/file.ext:line` when useful) so
the implementer can navigate directly.
## Approach
[Step-by-step implementation plan with file paths, function names, and code
shape where applicable. Group steps into atomic, ordered tasks.]
## Task List
### Phase 1: Foundation
- [ ] Task 1: ...
- [ ] Task 2: ...
### Checkpoint: Phase 1
- [ ] Tests pass, lint/formatter clean (module-specific commands)
### Phase 2: Core Features
- [ ] Task 3: ...
- [ ] Task 4: ...
### Checkpoint: Phase 2
- [ ] End-to-end flow works
### Phase 3: Polish
- [ ] Task 5: ...
- [ ] Task 6: ...
### Checkpoint: Complete
- [ ] All acceptance criteria met
- [ ] Ready for review
## Testing Strategy
[How to verify: which test commands to run per module, what cases to cover,
manual verification steps, lint/format checks. Consult each module's core
memory for the exact commands.]
## Parallelization Opportunities
- **Safe to parallelize:** Independent feature slices across separate
modules, tests for already-implemented features
- **Must be sequential:** Shared common schema changes, database migrations
- **Needs coordination:** Features that share a contract (define the contract
first, then parallelize)
## Open Questions
- [Question needing human input]
```
When the plan is purely analytical (e.g. a code review or feasibility study
with no implementation), skip the **Approach** section and lead with
**Findings** instead, keeping the rest of the structure.
with no implementation), skip the **Approach** and **Task List** sections and
lead with **Findings** instead, keeping the rest of the structure.
## Verification Checklist
Before starting implementation, confirm:
- [ ] Every task has acceptance criteria
- [ ] Every task has a verification step
- [ ] Task dependencies are identified and ordered correctly
- [ ] No task touches more than ~5 files
- [ ] Checkpoints exist between major phases
- [ ] The human has reviewed and approved the plan

View File

@ -106,4 +106,4 @@ IMPORTANT: all CLI commands must be executed from the `backend/` subdirectory.
* **Coverage:** If code is added or modified in `src/`, corresponding tests in `test/backend_tests/` must be added or updated.
* **Isolated run:** `clojure -M:dev:test --focus backend-tests.my-ns-test` for a specific test namespace.
* **Regression run:** `clojure -M:dev:test` to ensure no regressions in related functional areas.
* **Principles:** Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:common/testing-principles`.
* **Principles:** Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:testing`.

View File

@ -49,7 +49,7 @@ Components, variants, and debugging:
Text and tests:
- Shared text data conversion, DraftJS compatibility, modern text content, and derived position data: `mem:common/text-subtleties`.
- Common test commands, helper conventions, production-path test mutations, and runtime coverage choices: `mem:common/testing`.
- Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:common/testing-principles`.
- Cross-cutting testing principles, anti-patterns, and verification checklist: `mem:testing`.
## Areas without focused memories

View File

@ -1,47 +0,0 @@
# Testing Principles (Cross-Cutting)
Shared testing principles for all modules (backend, frontend, common, render-wasm, plugins). Module-specific commands and helpers are in each module's own testing memory.
## Core Principles
- **Test State, Not Interactions** — assert on outcomes, not method calls; survives refactoring
- **DAMP over DRY** — tests are specifications; duplication OK if each test is self-contained and readable
- **Prefer Real Implementations** — hierarchy: Real > Fake > Stub > Mock; mock only at boundaries (network, filesystem, email)
- **Arrange-Act-Assert** — every test: setup / action / verify
- **One Assertion Per Concept** — each test verifies one behavior; split compound assertions
- **Descriptive Test Names** — names read like specifications
## Anti-Patterns
- Testing implementation details → breaks on refactor
- Flaky tests (timing, order-dependent) → erode trust
- Mocking everything → tests pass, production breaks
- No test isolation → pass individually, fail together
- Testing framework code → waste of time
- Snapshot abuse → nobody reviews, break on any change
## Verification Checklist
After completing any implementation:
- Every new behavior has a corresponding test
- All tests pass for touched modules
- Bug fixes include a reproduction test that failed before the fix
- Test names describe the behavior being verified
- No tests were skipped or disabled
## Red Flags
- Writing code without corresponding tests
- Tests that pass on the first run (may not be testing what you think)
- Bug fixes without reproduction tests
- Tests that test framework behavior instead of app behavior
- Skipping tests to make the suite pass
## Rationalizations
- "I'll write tests after" → you won't; they'll test implementation not behavior
- "Too simple to test" → simple gets complicated; test documents expected behavior
- "Tests slow me down" → they speed up every future change
- "I tested manually" → manual testing doesn't persist
- "Code is self-explanatory" → tests ARE the specification
- "Just a prototype" → prototypes become production; test debt accumulates

View File

@ -6,6 +6,7 @@ You are working on the GitHub project `penpot/penpot`, a monorepo.
- A section's top-level memory is `<section>/core`. When a section is relevant, read the core memory
before focused memories.
- Edits/stale refs/duplication cleanup: `mem:memory-maintenance`.
- Cross-cutting testing principles, TDD workflow, and anti-patterns: `mem:testing`.
# Development workflow

View File

@ -7,6 +7,7 @@
- Source: `exporter/src/`; config: `deps.edn`, `shadow-cljs.edn`, `package.json`; runtime helpers/assets: `vendor/`, `scripts/`.
- From `exporter/`: setup `./scripts/setup`; watch `pnpm run watch` or `pnpm run watch:app`; production build `pnpm run build`; lint `pnpm run lint`; format check/fix `pnpm run check-fmt` / `pnpm run fmt`.
- Because exporter consumes `common/`, shared file/shape/model changes may need exporter verification even when the immediate change is not under `exporter/`.
- Cross-cutting testing principles and anti-patterns: `mem:testing`.
## HTTP and browser pool

View File

@ -52,7 +52,7 @@ Diagnostics and validation:
- Source-edit compile/hot-reload diagnostics: `mem:frontend/compile-diagnostics`.
- Runtime crash recovery: `mem:frontend/handling-crashes`.
- Tests and live verification: `mem:frontend/testing`.
- Cross-cutting testing principles and anti-patterns: `mem:common/testing-principles`.
- Cross-cutting testing principles and anti-patterns: `mem:testing`.
- Real pointer/keyboard gesture reproduction: `mem:frontend/playwright-gestures`.
## Areas without focused memories

View File

@ -7,6 +7,7 @@
- Source: `library/src/`; tests: `library/test/`; experimentation/docs: `playground/`, `docs/`; config: `shadow-cljs.edn`, `deps.edn`, `package.json`.
- From `library/`: build `pnpm run build`; bundle helper `pnpm run build:bundle` or `./scripts/build`; tests `pnpm run test`; watch `pnpm run watch` / `pnpm run watch:test`; lint `pnpm run lint`; format check/fix `pnpm run check-fmt` / `pnpm run fmt`.
- When changing file-format construction or export behavior in `common/`, consider whether `@penpot/library` should be tested because it constructs Penpot files outside the app UI.
- Cross-cutting testing principles and anti-patterns: `mem:testing`.
## JS API and builder state

View File

@ -85,6 +85,7 @@ From the `mcp/` directory, run
* `pnpm run build` to test the build of all packages
* `pnpm run fmt` to apply the auto-formatter
* Cross-cutting testing principles and anti-patterns: `mem:testing`.
## Devenv plugin/server wiring

View File

@ -13,6 +13,7 @@
- From `plugins/`: install `pnpm -r install`; runtime dev server `pnpm run start` or `pnpm run start:app:runtime`; sample plugin `pnpm run start:plugin:<name>`; build runtime `pnpm run build:runtime`; build plugins `pnpm run build:plugins`; lint `pnpm run lint`; format `pnpm run format:check` / `pnpm run format`; tests `pnpm run test`; e2e `pnpm run test:e2e`.
- If a change affects public Plugin API types or runtime, update `plugins/CHANGELOG.md`. Prefix type/signature entries with `**plugin-types:**`; runtime behavior entries with `**plugin-runtime:**`.
- Cross-cutting testing principles and anti-patterns: `mem:testing`.
- JS Plugin API behavior inside Penpot app: `mem:frontend/plugin-api-to-cljs-binding`; TS declarations are not runtime code; many API objects are CLJS proxies in `frontend/src/app/plugins/*.cljs`.
## Sandbox and global cleanup

View File

@ -26,6 +26,7 @@ From `render-wasm/`:
- Build/copy frontend artifacts: `./build`.
- Watch rebuild: `./watch`.
- Rust tests: `./test` or `cargo test <name>`.
- Cross-cutting testing principles and anti-patterns: `mem:testing`.
- Lint: `./lint`.
- Format check: `cargo fmt --check`.

149
.serena/memories/testing.md Normal file
View File

@ -0,0 +1,149 @@
# Testing
## Overview
Tests are proof that code works. Every behavior change needs a test.
Testing in this monorepo varies by module. Each module has its own test
commands, helpers, runner registration requirements, and conventions. This
memory covers cross-cutting testing principles. For module-specific commands
and helpers, consult:
- `mem:common/testing` — CLJC unit tests (JVM + JS), test helpers, fixture
builders, production-path change helpers
- `mem:frontend/testing` — CLJS unit tests, Playwright E2E integration tests,
live browser verification via nREPL
- Backend — JVM `clojure.test` under `backend/test/`; see `mem:backend/core`
## When to Use
- Implementing new logic or behavior
- Fixing any bug (reproduction test required)
- Modifying existing functionality
- Adding edge case handling
**When NOT to use:** Pure configuration changes, documentation updates, or
static content changes with no behavioral impact.
## TDD: Recommended Workflow
Write a failing test before writing the code that makes it pass. For bug fixes,
reproduce the bug with a test before attempting a fix.
When TDD isn't practical (exploratory work, tight coupling to unknown APIs),
still write tests before considering the work complete.
```
RED GREEN REFACTOR
Write a test Write minimal code Clean up the
that fails ──→ to make it pass ──→ implementation ──→ (repeat)
│ │ │
▼ ▼ ▼
Test FAILS Test PASSES Tests still PASS
```
- **RED** — Write the test first. It must fail. A test that passes immediately
proves nothing.
- **GREEN** — Write the minimum code to make the test pass. Don't over-engineer.
- **REFACTOR** — With tests green, improve the code without changing behavior:
extract shared logic, improve naming, remove duplication. Run tests after
every step.
## The Prove-It Pattern (Bug Fixes)
When a bug is reported, **do not start by trying to fix it.** Start by writing
a test that reproduces it:
1. Write a test that demonstrates the bug
2. Confirm the test FAILS (proving the bug exists)
3. Implement the fix
4. Confirm the test PASSES (proving the fix works)
5. Run the full test suite for the module (no regressions)
## Core Principles
- **Test State, Not Interactions** — assert on outcomes, not method calls;
survives refactoring
- **DAMP over DRY** — tests are specifications; duplication is OK if each test
is self-contained and readable. A test should tell a complete story without
requiring the reader to trace through shared helpers.
- **Prefer Real Implementations** — hierarchy: Real > Fake > Stub > Mock;
mock only at boundaries (network, RPC, filesystem, email)
- **Arrange-Act-Assert** — every test: setup / action / verify
- **One Assertion Per Concept** — each test verifies one behavior; split
compound assertions
- **Descriptive Test Names** — names read like specifications
## Prefer Real Implementations Over Mocks
Work down this list:
1. **Real implementation** — Test the actual code with real collaborators.
Highest confidence.
2. **Fake** — A simplified but functional in-memory implementation (e.g.
atom/dict-backed store instead of a real database).
3. **Stub** — Returns canned data. Use when the collaborator's logic is
irrelevant.
4. **Mock** — Last resort, only at boundaries. Use only when verifying
interaction with an external system that cannot be faked.
**Rule of thumb:** If you can write a fake or use the real implementation, do
that. If you find yourself asserting on call counts or invocation order, ask
whether a fake would be clearer.
## Fixtures over Manual Setup
Use fixture/`beforeEach` mechanisms for shared setup and teardown. Each test
should own its state so tests don't interfere with each other. Shorter-scope
fixtures (`:each` / per-test) are preferred; longer-scope fixtures (`:once` /
suite-level) are only for expensive, immutable shared setup.
## Parametrized Tests
Use your test framework's parametrize/table-driven mechanism to test multiple
scenarios with a single test body. Keeps tests concise and surfaces all cases
at a glance.
## Test Pyramid
```
╱╲
╲ E2E (few)
╲ Full flows, real browser/server
╱──────╲
╲ Integration (some)
╲ Cross-module, test DB
╱────────────╲
╲ Unit (most)
╲ Pure logic, fast
╱──────────────────╲
```
Prefer unit tests for pure logic. Reach for integration/E2E tests when covering
RPC handlers, database queries, or full user flows. In the frontend, Playwright
E2E tests should not be added unless explicitly requested.
## Anti-Patterns
| Anti-Pattern | Problem | Fix |
|---|---|---|
| Testing implementation details | Breaks on refactor | Test inputs/outputs |
| Flaky tests (timing, order-dependent) | Erodes trust | Deterministic assertions, isolate state |
| Mocking everything | Tests pass, production breaks | Prefer real implementations or fakes |
| No test isolation | Pass individually, fail together | Per-test state fixtures |
| Testing framework/platform code | Wastes time | Only test YOUR code |
| Snapshot abuse | Nobody reviews, break on any change | Focused assertions |
| Skipping tests to make suite pass | Hides real failures | Fix the test or fix the code |
## Verification Checklist
After completing any implementation:
- [ ] Every new behavior has a corresponding test
- [ ] All tests pass for touched modules
- [ ] Bug fixes include a reproduction test that failed before the fix
- [ ] Test names describe the behavior being verified
- [ ] No tests were skipped or disabled
- [ ] Lint/formatter passes for touched modules
- [ ] New test files registered in the module's runner/entrypoint (see module
testing memory)