From af84116213f541d4f0e729a77cea372082f5d4ea Mon Sep 17 00:00:00 2001 From: Armando Anaya Date: Tue, 4 Aug 2026 17:16:47 -0700 Subject: [PATCH] docs(skills): promote the 2026-08 run's lessons into the skills that own them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The run ledger was a gitignored local file — the wrong home for anything that should outlive one session. Its eight lessons move to the five skills that already own the rules they belong to; the ledger and the audit report are deleted, and the open work they carried is now issues (#281 reopened and commented, #314–#317 filed). Each lesson lands in exactly one skill, in that skill's own voice, tagged with the task it came from. Nothing is pasted verbatim — a policy document is not a changelog, so each is distilled to the rule and its one load-bearing reason. - **ui-capabilities** — a declaration is a cached answer. Every mutation that could change what a resource may be asked to do invalidates that resource's query, not only its counts. A stale `allowed_actions` is the cache-side twin of the hand-mirror this skill already bans. - **refactor-protocol** (testing) — three suites, all of them, before every push: `check.sh` runs **no browser suite**, and the real-server cycle run was three separate times the only detector. Plus `CI=1` always (`reuseExistingServer`), `git add` before trusting a local run (gates read `git ls-files`), and the generalized stub rule: no invisible-order or frozen-state semantics in a test double. - **refactor-protocol** (PR & CI) — one clause beyond the listed eight, because it is a live defect rather than a lesson: GitHub reads a closing keyword anywhere in a squashed commit message, *including inside a sentence denying it*. "Nothing here closes #281" closed #281 on #312's merge. It is reopened. - **nodejs-setup** — a green build is not a green typecheck: `tsconfig.build.json` excludes tests, and tests are where fixtures live. - **annotator-core** — a new wire field is not additive for this client. The three mirror locations are named, because `parseAnnotation` refuses an undeclared key rather than ignoring it. - **python-setup** — a method named after a builtin shadows it for every annotation declared after it; mypy's "not valid as a type" is the tell. No application code, no skill frontmatter, no auto-invoke wiring touched. --- .agents/skills/backend/python-setup/SKILL.md | 5 +++++ .../skills/frontend/annotator-core/SKILL.md | 21 +++++++++++++++++++ .agents/skills/frontend/nodejs-setup/SKILL.md | 6 ++++++ .../skills/frontend/ui-capabilities/SKILL.md | 1 + .../skills/process/refactor-protocol/SKILL.md | 13 ++++++++++++ 5 files changed, 46 insertions(+) diff --git a/.agents/skills/backend/python-setup/SKILL.md b/.agents/skills/backend/python-setup/SKILL.md index a11f05a0..77136d2c 100644 --- a/.agents/skills/backend/python-setup/SKILL.md +++ b/.agents/skills/backend/python-setup/SKILL.md @@ -72,6 +72,11 @@ uv run ruff check --fix `warn_return_any`, …). Every kernel function is fully annotated; no bare `Any`. - Outside the kernel, follow the local style — annotate new public functions. - Domain invariants belong in the pydantic v2 models, not in ad-hoc `assert`s at call sites. +- **A method named after a builtin shadows it for every annotation declared after it.** A class + with `def list(...)` makes a later `-> list[Batch]` resolve to *the method*, and mypy reports + `"..." is not valid as a type` — which reads as a mystery until you notice the name. Declare + annotated members above such a method, put helpers that need the builtin at module level, or + rename. The same applies to `dict`, `set` and `type`. — 2026-08 run, T9 ## Versioning diff --git a/.agents/skills/frontend/annotator-core/SKILL.md b/.agents/skills/frontend/annotator-core/SKILL.md index 5968851c..58967bf0 100644 --- a/.agents/skills/frontend/annotator-core/SKILL.md +++ b/.agents/skills/frontend/annotator-core/SKILL.md @@ -47,6 +47,27 @@ The document refuses only its own invariants — duplicate id, unknown id, forei rules (class↔geometry agreement, required attributes, bounds) stay the kernel's; the tools refuse at draw time, where a user can be told. +## A new wire field is not additive here + +`parseAnnotation` checks the key set **exactly** — a payload carrying a field the mirror does not +declare is *refused*, not ignored. That is deliberate (it is what makes the mirror a contract +rather than a suggestion), and the consequence is that adding a field to a kernel wire model is a +breaking change to this client until every mirror moves with it, **in the same commit**: + +| Where | What moves | +| --- | --- | +| `src/visionset/server/models.py` | the field on `AnnotationOut` (and `visionset/wire/`, MCP, CLI projections) | +| `frontend/annotator/src/core/types.ts` | the field on `Annotation` | +| `frontend/annotator/src/core/wire.ts` | `ANNOTATION_KEY_SET` **and** the `parseAnnotation` body | +| `frontend/ui-core/src/annotator/jobQueries.ts` | `WireAnnotation`, which is a second mirror on purpose | + +Then regenerate the fixture (`uv run python scripts/export_wire_fixtures.py`) and give any +in-core factory a value — `draftAnnotation` mints `null`, because the engine takes a document, +not a workflow, and must not claim provenance the service overwrites. + +A field the *service* stamps stays off `AnnotationCreate`/`AnnotationUpdate`: a field a client +could set and never observe is a lie in the schema. — 2026-08 run, T8 + ## The rules the machine enforces `src/core/**` must not import React and must not reach the DOM. Three gates, the frontend mirror diff --git a/.agents/skills/frontend/nodejs-setup/SKILL.md b/.agents/skills/frontend/nodejs-setup/SKILL.md index 6d675cce..88629ed8 100644 --- a/.agents/skills/frontend/nodejs-setup/SKILL.md +++ b/.agents/skills/frontend/nodejs-setup/SKILL.md @@ -64,3 +64,9 @@ Never hand-edit a `version` field. The repo-root `VERSION` file is the source of ## Before you say it works `pnpm -r build && pnpm -r test && pnpm -r lint` from the root. Report failures verbatim. + +**A green build is not a green typecheck.** Each package builds through +`tsconfig.build.json`, which *excludes test files*, while `lint` runs the full +`tsconfig.json` over everything. So `pnpm -r build` can pass while a type error sits in a test +or a test helper — which is where fixtures live, and fixtures are what a new required wire field +breaks. Run `lint` before calling TypeScript green. — 2026-08 run, T8 diff --git a/.agents/skills/frontend/ui-capabilities/SKILL.md b/.agents/skills/frontend/ui-capabilities/SKILL.md index f08b84b7..45de2018 100644 --- a/.agents/skills/frontend/ui-capabilities/SKILL.md +++ b/.agents/skills/frontend/ui-capabilities/SKILL.md @@ -21,6 +21,7 @@ description: Rules for how the VisionSet frontend decides which actions to offer - **Disabled-with-reason over hidden** for actions absent from `allowed_actions` but meaningful in context: render disabled with a tooltip stating why ("Batch is completed — create a correction batch to edit"). Fully hide only actions that are never meaningful on that screen. - **Read-only is a mode, not an accident.** Any surface that can open in a state where writes are not permitted (annotator on a non-`in_annotation` batch) must render an explicit read-only mode: visible banner, editing tools disabled, no dirty state possible. "Open and let saves fail" is forbidden. - **Every mutation call site** answers three questions in code review: where does a refusal render? where does success render? what happens to the rejected promise? If any answer is "nowhere", the change is incomplete. +- **A declaration is a cached answer, so invalidate it.** Every mutation that could change what a resource may be asked to do must invalidate that resource's own query — not only its counts or its data. `allowed_actions` goes stale exactly like a number does, and a stale declaration is the cache-side twin of the hand-mirror: the client is again showing something the kernel no longer agrees with. It shipped as a Finish-job button disabled over a job that was finished, because the job's declaration still described a moment when every asset was `unannotated`. — 2026-08 run, T3 - The app-level error boundary and `unhandledrejection` handler are load-bearing; never remove or bypass them. ## Scope limits (do not overreach) diff --git a/.agents/skills/process/refactor-protocol/SKILL.md b/.agents/skills/process/refactor-protocol/SKILL.md index 77e26750..70989bef 100644 --- a/.agents/skills/process/refactor-protocol/SKILL.md +++ b/.agents/skills/process/refactor-protocol/SKILL.md @@ -29,10 +29,23 @@ All work in the worktree; never the primary checkout. Conventional commits in lo - **Every mutation touched must have a refusal-rendering test**: force the refusal, assert the user sees prose (not a raw code, not nothing). - E2e fixtures seed all five asset-progress states and at least one batch per batch state when the task touches state-dependent UI. - Run the full existing suites (Python + TS) and linters; fix what your change broke, and only that. +- **Three suites, all of them, before every push** — `scripts/check.sh` runs **no browser suite at all**, so "check.sh green" is not "CI will be green": + + ```bash + bash scripts/check.sh + cd frontend/app && CI=1 npx playwright test + cd frontend/app && CI=1 npx playwright test -c playwright.cycle.config.ts + ``` + + The third is the **real-server cycle run**, and it is mandatory for anything touching state, gating, or progress: it was three separate times the *only* suite to catch a regression — a stale job declaration, a label flip standing in for feedback, and a progress counter running backwards. — 2026-08 run, T3/T5/T6 +- **Always `CI=1` for a local Playwright run.** `playwright.config.ts` sets `reuseExistingServer: !CI`, so a stale vite server on :5273 answers instead of your build and produces failures that read as code bugs. — 2026-08 run, T3 +- **`git add` new files before trusting any local check run.** Several gates read `git ls-files` — the index, not the working tree — so an untracked new file is invisible to them and passes locally while failing in CI. — 2026-08 run, T4 +- **A test double must not encode invisible-order or frozen-state semantics.** Put defaults in the *unmatched-request fallback* so an explicit stub always wins whichever order it was registered in, and derive stub responses from the state the test walks rather than from frozen literals. Both failure modes make a test assert against the fixture instead of the code, and both are silent. — 2026-08 run, T6/T7/T10 ## PR & CI 1. `gh pr create` — body includes: what changed, "Found, not fixed" list, test plan, `Closes #NNN` only for issues actually and fully closed. + **GitHub reads a closing keyword anywhere in the PR body or a squashed commit message, including inside a sentence that denies it.** "Nothing here closes #281" closed #281. To say an issue is *not* closed, name it without the keyword — `#281 is untouched`, `cf. #281`. — 2026-08 run, T9 2. `gh pr merge --auto --squash`. 3. Monitor `gh pr checks --watch`; on failure read logs, fix, push. **After 3 consecutive failures of the same check with no clear fix, stop and report** — never loop indefinitely, never disable or skip a failing check to get green.