Skip to content

Commit 0d5c74a

Browse files
committed
docs(agents): fix contradictions and stale facts in platform skills
tool-registry-boundary told tests to re-mock a global (check:test-patterns fails that); db-migrate listed a nonexistent annotate rule and omitted the pending-drop-tables contract; permission-group skills cited a removed descriptions record, a nonexistent key-order test, and a single principal-wide capability where there are two; v2-api-conventions cited a removed helper; memory-load-check called a pattern the connectors use cargo-culting. Gates now name check:api-validation:strict / check:audits, ship runs CI's schema/migration sync step, test-audit points at CLAUDE.md instead of copying it, and incident narration became current-state rules.
1 parent 6fe9f17 commit 0d5c74a

13 files changed

Lines changed: 56 additions & 83 deletions

File tree

‎.agents/skills/add-permission-group-item/SKILL.md‎

Lines changed: 12 additions & 12 deletions
Large diffs are not rendered by default.

‎.agents/skills/babysit/SKILL.md‎

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -88,8 +88,8 @@ conditions freshly after every push.
8888
across all pages has `isResolved: true`, and every check has finished and passed, stop —
8989
report the outcome (see "Reporting" below) and skip the rest of this list.
9090

91-
2. **If the PR has a merge conflict**, merge `origin/staging`, resolve the conflicts, run the
92-
usual pre-push checks, push, and go to step 8 to re-trigger review.
91+
2. **If the PR has a merge conflict**, merge `origin/staging`, resolve the conflicts, run `/ship`
92+
steps 4–6 on the merge result, push, and go to step 8 to re-trigger review.
9393

9494
3. **If no review has run yet** (fresh PR, no bot comments): both run automatically on PR open —
9595
confirm via `gh pr checks <n>` (look for `Greptile Review` and `cubic · AI code reviewer`) and

‎.agents/skills/cleanup/SKILL.md‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -7,7 +7,7 @@ argument-hint: "[scope] [fix=true|false]"
77
# Cleanup
88

99
Arguments:
10-
- scope: what to review (default: your current changes). Examples: "diff to main", "PR #123", "src/components/", "whole codebase"
10+
- scope: what to review (default: your current changes). Examples: "diff to staging", "PR #123", "src/components/", "whole codebase"
1111
- fix: whether to apply fixes (default: true). Set to false to only propose changes.
1212

1313
User arguments: $ARGUMENTS

‎.agents/skills/db-migrate/SKILL.md‎

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,7 +34,7 @@ Never put expand and contract in the same PR. If this PR both removes the code t
3434
| Change a column type | add a new column of the new type; dual-write | backfill, swap reads, drop old |
3535
| Add FK / CHECK | `ADD CONSTRAINT ... NOT VALID` | `VALIDATE CONSTRAINT` separately |
3636
| Index an existing table | `COMMIT;` breakpoint → `SET lock_timeout = 0` → `CREATE INDEX CONCURRENTLY IF NOT EXISTS` (see `packages/db/scripts/migrate.ts`) | — |
37-
| Drop an index | `COMMIT;` breakpoint → `DROP INDEX CONCURRENTLY` — plain `DROP INDEX` takes ACCESS EXCLUSIVE on the table | — |
37+
| Drop an index | `COMMIT;` breakpoint → `DROP INDEX CONCURRENTLY IF EXISTS` — plain `DROP INDEX` takes ACCESS EXCLUSIVE on the table | — |
3838
| Backfill data | batched + idempotent `UPDATE` (keyset/`WHERE`, bounded) | — |
3939

4040
A `CREATE INDEX`, `ADD COLUMN`, or `ADD CONSTRAINT` against a table **created in the same migration** is always safe (no rows, no live traffic) — the lint already suppresses those.
@@ -53,6 +53,7 @@ workspaceId: text('workspace_id'),
5353
Format: `contract-pending(<precondition>): <what to drop> — <why it's safe once the precondition holds>`. The precondition names the PR/release that removes the last reader and **must be fully deployed** before the contract ships.
5454

5555
- **The TODO list is a grep** — always accurate, never drifts: `grep -rn "contract-pending" packages/db apps/sim`. Run it when starting migration work to see what is owed.
56+
- While a drop is pending, tag each doomed column `@deprecated` and make every read of that table name its columns: no argless `select().from(t)`, `findFirst()` without `columns`, argless `.returning()`, or bare `getTableColumns(t)` — use `omit(getTableColumns(t), [...])`. `check:pending-drop-tables` enforces this.
5657
- For anything with a real owner or schedule, also open a tracking issue and put its number in the marker.
5758
- **Close the loop in the contract PR:** the contract migration's `-- migration-safe:` annotation references the expand, and you **delete the `contract-pending` marker** in the same PR:
5859
```sql
@@ -74,8 +75,8 @@ The lint flags risky *shapes*; it cannot know whether a given drop is *safe righ
7475
1. Edit `packages/db/schema.ts`, then `cd packages/db && bunx drizzle-kit generate` to produce the SQL. If this is an expand that defers a drop, leave a `contract-pending` marker on the legacy column (see "Tracking the contract"). If this is the contract, delete the marker it resolves.
7576
2. Hand-edit the generated SQL where the playbook requires it: `CONCURRENTLY` + `COMMIT;` breakpoint for indexes on existing tables, `NOT VALID` for constraints, batching for backfills.
7677
3. Run `bun run check:migrations` (base defaults to `origin/staging`).
77-
- **Hard errors** (`add-not-null-no-default`, `rename`, `index-not-concurrent`, `constraint-not-valid`, …): rewrite into expand/contract. Do **not** try to annotate them away — the lint won't accept it.
78-
- **Annotate tier** (`drop-table`, `drop-column`, `drop-default`, `set-not-null`, `alter-type`, `drop-index`): only after you've confirmed steps 1–3 above, add a comment on the line directly above the statement:
78+
- **Hard errors** (`add-not-null-no-default`, `rename`, `index-not-concurrent`, `concurrent-index-not-idempotent`, `concurrent-index-no-commit`, `drop-index-not-concurrent`, `concurrent-drop-index-not-idempotent`, `concurrent-drop-index-no-commit`, `constraint-not-valid`, …): rewrite into expand/contract. Do **not** try to annotate them away — the lint won't accept it.
79+
- **Annotate tier** (`drop-table`, `drop-column`, `drop-constraint`, `drop-default`, `set-not-null`, `alter-type`): only after you've confirmed steps 1–3 above, add a comment on the line directly above the statement:
7980
```sql
8081
-- migration-safe: `secret` read removed in v0.6.1 (#1234), shipped two deploys ago
8182
ALTER TABLE "webhook" DROP COLUMN "secret";

‎.agents/skills/memory-load-check/SKILL.md‎

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -72,9 +72,7 @@ For those, require all three:
7272
- skip oversize at listing (`stubOrSkipBySize` with the reported size) and again at fetch time (overflow -> `markSkipped`), since the listing size can be missing or under-reported
7373
- never drop/truncate silently — oversized files become content-less failed rows carrying `skippedReason`, so they stay visible in the KB UI instead of vanishing from the index
7474

75-
Skip the pattern when the source already bounds the payload:
76-
- pure API/structured-data connectors (Jira, Linear, Notion, Confluence, Sentry, Slack, Zendesk, Gmail, ...) — paginated JSON/text; apply normal pagination + concurrency bounds instead of a per-file byte cap
77-
- native-document connectors capped by the platform (Google Docs ~50 MB, Google Sheets via `MAX_ROWS`, Evernote ~25 MB/note) — a 100 MB cap can never fire, and wrapping a `response.json()`/Thrift parse in `readBodyWithLimit` is cargo-culting
75+
Skip the *per-file `CONNECTOR_MAX_FILE_BYTES` + skipped-row* pattern when the source bounds the item count (paginated JSON: Jira, Linear, Sentry, Slack, Zendesk, Gmail, ...). Every response body is still read through `readBodyWithLimit` with a connector-specific budget (`MAX_DOCS_RESPONSE_BYTES` in google-docs, `MAX_CONTENT_BYTES` in google-sheets, a remaining-bytes budget in notion). Confluence attachments use the full file pattern.
7876

7977
Litmus test: "Can a user make this one fetch arbitrarily large, with nothing upstream stopping it?" Yes -> use the pattern. No (platform hard-cap, or already paginated) -> a per-file byte cap adds noise, not safety. Borderline: a user-configured/self-hosted endpoint with no platform cap (e.g. Obsidian) — bound it only if the content is genuinely unbounded.
8078

‎.agents/skills/migrate-application-operation/SKILL.md‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -166,7 +166,7 @@ Do not create internal-, public-, or Copilot-specific versions of the same seman
166166

167167
Choose principal kinds from actual behavior. Do not accept every principal merely because the use case is shared. Workspace API keys have a write ceiling and cannot satisfy admin operations. The operation definition must fail fast when its role, workspace-key policy, and principal kinds disagree.
168168

169-
`capability` is required — name the permission-group capability that governs the operation, or `'none'` with a `// permission-group-exempt: <reason>` comment directly above it. `defineWorkspaceOperation` throws at definition time when it is absent. See `add-permission-group-item`.
169+
`capability` is required: a static, operation-declarable capability (not parameterized, not principal-wide), or `'none'` with a `// permission-group-exempt: <reason>` comment directly above it. If `principalKinds` includes `oauth_access_token`, `oauthScope` (`api:read` | `api:write` | `search:read`) is required. `defineWorkspaceOperation` throws at definition time on any violation. See `add-permission-group-item`.
170170

171171
Route declarations, tool adapters, and use cases must use the same literal operation. Runtime operation selection is permitted only from a trusted, code-defined registry. Never accept an operation ID or permission tag from an HTTP body, model argument, or other untrusted input.
172172

@@ -317,7 +317,7 @@ Stop and report a missing design rather than weakening identity, authorization,
317317

318318
## Test each risk at one boundary
319319

320-
Run the `test-audit` authoring gate before writing any test. Own each risk at exactly one boundary:
320+
Run the `test-audit` authoring gate before writing any test. Prefer one E2E/integration run over the real boundary; where a use-case unit test is justified, list its failure modes before writing code (CLAUDE.md → Testing). Own each risk at exactly one boundary:
321321

322322
- Application use-case tests own authorization, principal-kind rejection before canonical loading, workspace assertion mismatch, delegated scope, not found, conflict, no-op, audit derived from authoritative results, and infrastructure failures (storage, rate-limit, provider, or database errors raised by delegated services) propagating as 5xx-mapped errors — never converted to not-found or forbidden.
323323
- One `*.integration.ts` owns repository semantics: canonical active lookup, workspace-predicated writes, archived resources, authoritative affected rows, and database error propagation.
@@ -335,7 +335,7 @@ Run at minimum:
335335
bun run --cwd apps/sim test <focused test files>
336336
bunx biome check <changed source and test files>
337337
bunx turbo run type-check --filter=@sim/app --filter=@sim/auth
338-
bun run check:api-validation:strict
338+
bun run check:audits
339339
git diff --check
340340
```
341341

‎.agents/skills/ship/SKILL.md‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@ When the user runs `/ship`:
4545
- Then run root `bun run test` from the repo root. It chains `test:scripts` (the `scripts/*.test.ts` suite CI runs) before every workspace suite; workspace-scoped runs skip it, which is how a `scripts/check-*.test.ts` failure has reached CI. A failing test aborts ship.
4646
5. **Run migration safety** — only if the diff touches `packages/db/migrations/**` or `packages/db/schema.ts`:
4747
- Run `/db-migrate` to review the migration for zero-downtime safety (expand/contract phasing, backward-compatibility with the deployed app version).
48+
- `cd packages/db && bunx drizzle-kit generate` must leave `git status --porcelain packages/db/migrations` empty (CI's schema/migration sync step).
4849
- `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.
4950
6. **Run pre-ship checks** from the repo root before staging. This has two phases: first **regenerate** every committed artifact so generated files never drift into a CI failure (this is what catches things like `agent-stream-docs` going stale after a `models.ts` edit), then run the **full audit suite** CI's `Lint and Test` job enforces. Both phases parallelize — but only across commands that write **disjoint** outputs — and a bare `wait` swallows child exit codes, so both phases below explicitly collect each job's status and abort ship if any failed.
5051

‎.agents/skills/test-audit/SKILL.md‎

Lines changed: 3 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -17,19 +17,9 @@ mechanics (global mocks, `@sim/testing`, performance rules).
1717

1818
## The three rules
1919

20-
1. **Never write unit tests after you write code.** A test written to describe code that already
21-
exists restates the implementation, passes on the first run, and proves nothing. If the change
22-
needs proof, prove it end to end.
23-
2. **Prefer E2E tests; use them to verify complex features.** Exercise the real boundary — real
24-
Postgres/Redis (`*.integration.ts`), a running app over real HTTP, or the packaged desktop app
25-
(Playwright). Every E2E run ends with a **verifiable, repeatable artifact**: a JSON report of
26-
checks with pass/fail and durations, an HTTP status log, a trace, or a screenshot, written to a
27-
path the caller controls (`<SUITE>_REPORT_PATH`) and uploaded by CI on failure.
28-
`apps/sim/scripts/test-scim-e2e.ts` is the reference shape.
29-
3. **If you must test a system in isolation, write the failure modes down first, then write the
30-
code.** List every way the unit can fail (bad input, boundary, concurrency, partial failure,
31-
permission denial, resource cap). Each listed mode becomes one test that fails before the code
32-
exists. A mode you cannot list is not a test you should write.
20+
CLAUDE.md → Testing sets them: no unit tests after the code; prefer E2E at the real boundary,
21+
ending in a verifiable artifact; list failure modes before testing in isolation. A mode you cannot
22+
list is not a test you should write. This skill enforces them.
3323

3424
## Authoring gate
3525

0 commit comments

Comments
 (0)