Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
The table of contents is too big for display.
Diff view
Diff view
  •  
  •  
  •  
The diff you're trying to view is too large. We only load the first 3000 changed files.
15 changes: 8 additions & 7 deletions .agents/skills/cleanup/SKILL.md
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
---
name: cleanup
description: Run all code quality skills — effects, memo, callbacks, state, React Query, emcn design review, url-state, and comments — analyzing in parallel, then applying fixes sequentially
description: Run all code quality skills — effects, memo, callbacks, state, React Query, emcn design review, url-state, comments, and test-audit — analyzing in parallel, then applying fixes sequentially
argument-hint: "[scope] [fix=true|false]"
---

Expand All @@ -16,9 +16,9 @@ User arguments: $ARGUMENTS

Parse `$ARGUMENTS` into `scope` and `fix`: extract the `fix=true|false` token wherever it appears in the string and strip it from `scope`; defaults are the current changes and `fix=true`. `fix` is consumed by Step 3 only — the passes below always run `fix=false`.

Spawn all eight passes concurrently as subagents in a **single message** (multiple Agent tool calls). Each runs its skill on the parsed `scope` with `fix=false` — analysis and proposals ONLY, no edits. Instruct each agent to return its findings as a structured list: for every proposed change, the file path, line range, a one-line description of the change, and the exact before/after so the orchestrator can apply it without re-deriving.
Spawn all nine passes concurrently as subagents in a **single message** (multiple Agent tool calls). Each runs its skill on the parsed `scope` with `fix=false` — analysis and proposals ONLY, no edits. Instruct each agent to return its findings as a structured list: for every proposed change, the file path, line range, a one-line description of the change, and the exact before/after so the orchestrator can apply it without re-deriving.

Run these eight in parallel on the parsed `scope`:
Run these nine in parallel on the parsed `scope`:

1. `/you-might-not-need-an-effect <scope> fix=false`
2. `/you-might-not-need-a-memo <scope> fix=false`
Expand All @@ -28,22 +28,23 @@ Run these eight in parallel on the parsed `scope`:
6. `/emcn-design-review <scope> fix=false`
7. `/you-might-not-need-url-state <scope> fix=false`
8. `/you-might-not-need-a-comment <scope> fix=false`
9. `/test-audit audit <scope>` — read-only; only when the scope adds or changes test files (`*.test.ts(x)`, `*.integration.ts`, `e2e/**`). It applies the authoring gate to every new or changed test and proposes deleting the ones that fail it.

## Step 2 — Converge

Collect all findings into one list, **keeping each proposal tagged with the pass that produced it** — do NOT collapse a file's proposals into a single unlabeled patch, because Step 3 applies in pass order and needs those labels. Detect overlaps where two passes touch the same region (common: a state pass and an effect pass on the same block, or a memo and callback pass on the same component). Reconcile only genuine same-region conflicts, and drop proposals a sibling pass has made moot; a reconciled change inherits the pass label of whichever of its passes comes first in the Step 3 dependency order (effects → state → memo → callback → React Query → url-state → emcn → comments), so it is applied at the earliest safe point. Non-overlapping proposals stay as-is with their own labels. The output is a per-pass list of surviving changes, not a per-file patch.
Collect all findings into one list, **keeping each proposal tagged with the pass that produced it** — do NOT collapse a file's proposals into a single unlabeled patch, because Step 3 applies in pass order and needs those labels. Detect overlaps where two passes touch the same region (common: a state pass and an effect pass on the same block, or a memo and callback pass on the same component). Reconcile only genuine same-region conflicts, and drop proposals a sibling pass has made moot; a reconciled change inherits the pass label of whichever of its passes comes first in the Step 3 dependency order (effects → state → memo → callback → React Query → url-state → emcn → comments → tests), so it is applied at the earliest safe point. Non-overlapping proposals stay as-is with their own labels. The output is a per-pass list of surviving changes, not a per-file patch.

## Step 3 — Sequential apply

If `fix=false`, skip this step — just report the proposals from Step 2.

Otherwise apply the surviving changes yourself (in the main context, not delegated), iterating **pass by pass** in this dependency order so earlier structural changes settle before later passes build on them:

1. effects → 2. state → 3. memo → 4. callback → 5. React Query → 6. url-state → 7. emcn design → 8. comments
1. effects → 2. state → 3. memo → 4. callback → 5. React Query → 6. url-state → 7. emcn design → 8. comments → 9. tests
Comment thread
waleedlatif1 marked this conversation as resolved.

For each pass in turn, apply all of that pass's changes, then move to the next pass. A file touched by several passes is therefore edited once per pass, in this order — not once as a merged patch. This is what makes the ordering real: a single merged-per-file patch would collapse all passes into one edit and lose it.

Comments apply last, on purpose: that pass operates on whatever the earlier structural passes settled the code into, so it never edits lines a sibling pass is about to delete or rewrite.
Comments apply after every structural pass, on purpose: that pass operates on whatever the earlier passes settled the code into, so it never edits lines a sibling pass is about to delete or rewrite. Tests apply last because they only touch test files; in Step 2, drop any other pass's proposal on a test file the tests pass deletes.

**Treat every Step 1 proposal as snapshot-relative, not authoritative.** All passes analyzed the *original* files in parallel, so a proposal's line ranges and before/after text describe the code as it was *before* any edits — once an earlier pass has run, a later pass's snippet may no longer match. So for each change, before applying:

Expand All @@ -55,7 +56,7 @@ After all edits, run `bun run lint:check` (it runs `turbo run lint:check` across

## Step 4 — Summary

Output a summary across all eight passes: what each found, what was applied vs. skipped-as-redundant, and any proposals that need a human decision.
Output a summary across all passes that ran: what each found, what was applied vs. skipped-as-redundant, and any proposals that need a human decision.

## Boundary findings

Expand Down
7 changes: 4 additions & 3 deletions .agents/skills/ship/SKILL.md
Original file line number Diff line number Diff line change
Expand Up @@ -30,8 +30,9 @@ When the user runs `/ship`:
- Types: `fix`, `feat`, `improvement`, `chore`
- Scope: short identifier (e.g., `undo-redo`, `api`, `ui`)
- Keep it concise
4. **Run the cleanup pass** — only if the diff modifies UI code (any `.tsx` file, or anything under `apps/sim/components/`, `apps/sim/hooks/`, or `apps/sim/stores/`): `/cleanup`
- `/cleanup` fans out the React/UI passes (effects, memo, callbacks, state, React Query, emcn, url-state) plus the comment pass; skip it when no UI was touched. When it runs, it applies fixes so they land in this commit.
4. **Run the cleanup and test gates**
- If the diff modifies UI code (any non-test `.tsx` file, or anything under `apps/sim/components/`, `apps/sim/hooks/`, or `apps/sim/stores/`), run `/cleanup`. It fans out the React/UI passes (effects, memo, callbacks, state, React Query, emcn, url-state), the comment pass, and the test-audit pass, and applies fixes so they land in this commit.
- Otherwise, if the diff adds or changes tests (`*.test.ts(x)`, `*.integration.ts`, `e2e/**`), run `/test-audit audit <changed test files>` on its own. Every new or changed test must pass the authoring gate; delete the ones that don't rather than shipping them.
5. **Run migration safety** — only if the diff touches `packages/db/migrations/**` or `packages/db/schema.ts`:
- Run `/db-migrate` to review the migration for zero-downtime safety (expand/contract phasing, backward-compatibility with the deployed app version).
- `bun run check:migrations origin/staging` must pass (staging is the PR base). Do not silence a flagged statement with a `-- migration-safe:` annotation unless `/db-migrate` confirmed the old code no longer depends on it; otherwise split the destructive change into a later deploy.
Expand Down Expand Up @@ -143,7 +144,7 @@ Tested manually (or describe testing)
## Checklist
- [x] Code follows project style guidelines
- [x] Self-reviewed my changes
- [ ] Tests added/updated and passing
- [ ] Tests added/updated and passing (new tests pass the `test-audit` authoring gate)
- [x] No new warnings introduced
- [x] I confirm that I have read and agree to the terms outlined in the [Contributor License Agreement (CLA)](./CONTRIBUTING.md#contributor-license-agreement-cla)
```
Expand Down
148 changes: 148 additions & 0 deletions .agents/skills/test-audit/SKILL.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,148 @@
---
name: test-audit
description: Invoke whenever writing, changing, reviewing, or sweeping tests. Authoring gate for new tests, plus an audit workflow for low-value, implementation-coupled, or duplicative tests and the test-only production seams they demand.
argument-hint: "[author | audit <path> | campaign <subsystem>]"
---

# Test Audit

Three modes, one value bar. **Authoring** gates every new or changed test at write time.
**Audit** runs a focused sweep of existing tests that re-assert source, duplicate stronger proof,
couple to implementation, or keep test-only production seams alive. **Campaign** prunes one
subsystem's whole test surface in parallel lanes. Optimize for confidence, not deletion count —
but a test that cannot name the bug it catches is cost, not coverage.

Read `.claude/rules/sim-testing.md` first: it defines the test layers, file naming, and the
mechanics (global mocks, `@sim/testing`, performance rules).

## The three rules

1. **Never write unit tests after you write code.** A test written to describe code that already
exists restates the implementation, passes on the first run, and proves nothing. If the change
needs proof, prove it end to end.
Comment thread
waleedlatif1 marked this conversation as resolved.
2. **Prefer E2E tests; use them to verify complex features.** Exercise the real boundary — real
Postgres/Redis (`*.integration.ts`), a running app over real HTTP, or the packaged desktop app
(Playwright). Every E2E run ends with a **verifiable, repeatable artifact**: a JSON report of
checks with pass/fail and durations, an HTTP status log, a trace, or a screenshot, written to a
path the caller controls (`<SUITE>_REPORT_PATH`) and uploaded by CI on failure.
`apps/sim/scripts/test-scim-e2e.ts` is the reference shape.
3. **If you must test a system in isolation, write the failure modes down first, then write the
code.** List every way the unit can fail (bad input, boundary, concurrency, partial failure,
permission denial, resource cap). Each listed mode becomes one test that fails before the code
exists. A mode you cannot list is not a test you should write.

## Authoring gate

Before adding any test, answer all four. A missing answer means do not add it.

1. What observable behavior, invariant, or independent contract does it protect?
2. What credible regression makes it fail?
3. Why does existing coverage not already catch that failure? Type-check, `next build`,
`bun run check:audits`, and the integration/E2E suites are coverage too. Each contract has one
primary owner at the strongest boundary; another layer needs its own distinct risk. Prefer
extending an existing table-driven case over a near-duplicate test.
4. Does it need a production seam (export, flag, wrapper, injection hook) that no production
caller needs? If yes, test at the real boundary instead.

Then check it against every junk pattern below. A match fails the gate unless the retention bar
names the contract it independently guards. A test that would break under behavior-preserving
refactoring asserts implementation, not behavior.

**Regression tests** must fail on the pre-fix code for the intended reason. Revert each guard of
the fix separately and watch the test named for that guard go red, then restore. A regression
test that never demonstrably failed proves the mock, not the fix. One regression at the owner
boundary covers the bug; do not replay it at every layer it crosses.

## Junk patterns

- assertion-free or `toBeDefined()`-only tests; "renders without crashing";
- restating declarations: block/tool/trigger/provider config (subBlock ids, params, outputs, URL
templates, header maps), constant tables, registries, enums, export lists — type-check and
`check:audits` own these;
- route/handler tests that mock every collaborator and assert `toHaveBeenCalledWith` on the mocks,
or re-assert a mock's canned return;
- mocks that implement the asserted behavior, or one mock standing in for different APIs;
- Zod contract tests proving a schema accepts a valid object or rejects an obviously invalid one;
- React tests of text, class names, aria presence, snapshots, "calls onClick";
- hook tests asserting query keys or fetch URLs; store tests of trivial setters;
- tests of test infrastructure (mocks, factories, builders testing themselves);
- source-text or import greps (`readFileSync(src)` + `toContain`);
- expected values produced by the helper under test;
- duplicate invocations of the same contract, or provider-local replays of a shared helper;
- fixtures that supply the ordering, receipt, or callback the owner should produce;
- negative controls that pass for an unrelated reason (a different guard short-circuits first);
- names or fixtures that promise more than the input exercises;
- dead production code or exports whose only callers are tests.

## Retention bar

Keep a test when it independently enforces one of:

- **security** — authn/authz denial, tenant/workspace isolation, SSRF/URL validation, secret
redaction, encryption, signature verification, path traversal, injection, rate limits;
- **money and data integrity** — billing/usage math, metering, quotas, idempotency, migrations,
persistence semantics, concurrency/locking/leases, outbox, retries;
- **executor semantics** — DAG traversal, loops/parallels, conditions/routers, reference
resolution, streaming, pause/resume, run-from-block, cancellation;
- **real algorithms with edge cases** — chunkers, parsers, query builders, cron, diff/merge,
pagination, encoding, date math, ranking;
- **cross-process wire contracts** — realtime protocol, desktop bridge/IPC, CLI/SDK wire, provider
webhooks, MCP — that type-check cannot see;
- **a regression with a credible repeat**, shown red on the pre-fix code.

Also keep call ordering when order is observable, and a source inspection when it is the cheapest
independent guard of a user-facing byte, key, or path. A retained test that fails on the baseline
is a possible product bug: reproduce it and fix the owner rather than deleting it. Static or slow
is not a deletion reason.

## Audit mode

Keep discovery read-only and report evidence before editing. Before judging a candidate, read the
complete test and its production owner, callers, sibling implementations, overlapping tests, CI
routing (CI discovers `*.integration.ts` by glob; `.github/workflows/*.yml` names a few scripts and
files by path), and relevant history
(`git log --format='%h %s' -5 -- <file>`).

Record for every deletion candidate: the test and location; the failure it can actually detect;
non-test callers of the seam it covers; the stronger remaining proof (or why none is needed); the
production or test-support code its deletion unlocks; and the focused validation command.

**Edit shape.** One coherent owner-boundary batch per PR. When pruning inside a file, also delete
now-unused imports, mocks, fixtures, and helpers. Delete test-only exports and dead production
paths instead of preserving aliases (`rg -n '<name>' --glob '!**/*.test.*'` must show no other
reference, including string and dynamic-import references; never delete route files, registry
entries, or generated files). Prefer net-negative production LOC. Do not add replacement tests
that restate the same implementation.

## Campaign mode

For a whole subsystem or the whole repo:

1. Partition test files into lanes of ~150–350 files by owning directory, and list the protected
set (every `*.integration.ts`, `*.live.test.ts`, `__integration__/**`, `apps/desktop/e2e/**`,
and every path named in `.github/workflows/*.yml`).
2. Give each lane its own git worktree and branch (`git worktree add -b <branch> <path> <base>`,
then `bun install --frozen-lockfile` inside it). Lanes never share a checkout, never symlink
`node_modules`, and never use `git stash` — the stash is shared across worktrees.
3. Each lane commits once and writes a report: counts, categories removed with examples, notable
keeps and why, production seams removed with grep evidence, and the commands it ran.
4. Merge lane branches, then sweep orphaned shared test support (`packages/testing/**`, fixtures,
helpers) that no remaining test imports.

## Validation

Never edit source or tests while Vitest is running in the same checkout.

1. Run the touched and sibling test files. From `apps/sim`:
`../../node_modules/.bin/vitest run <paths>` (never `bunx vitest`, which fetches a different
Vitest). Other workspaces: run from the workspace directory. Never pipe the runner through
`grep`/`tail` where the pipe hides its exit code.
2. If production code changed: `bun run type-check` in that workspace.
3. `bun run check:audits` from the repo root (some audits list test files by path).
4. `bun run lint`, then `git diff --check`.
5. Report `git diff --shortstat` with production and test changes counted separately.

## Handoff

Report the categories removed, production simplifications, retained false positives and why they
stay, the validation actually run, production vs test LOC, and named follow-ups.
Loading
Loading