Rebuild agent skills around what the repo can't say about itself; measure every cut (closes #525) - #740
Conversation
Claude Code only scans .claude/skills/<name>/SKILL.md one level deep, so the 12 project skills synced to .claude/skills/project/<name>/ have been invisible to native skill routing since they were created — only the three un-nested skills (next-best-practices, next-upgrade, web-design-guidelines) ever loaded. sync_claude() now strips source-tree grouping and writes flat, with a duplicate-name guard. Verified live: all 12 learn-* skills appear in the session skill listing immediately after the flat sync. Also fixes sync_cursor()'s reference-inlining dir check to references/ (plural, the convention bundled reference files actually use) — the singular form matched nothing.
Synthesizes Anthropic's official skill docs (Claude Code reference, Agent Skills engineering post, platform best-practices guide, all read 2026-08-27) with this repo's own principles: skills individuated by concern rather than file surface, declarative bodies over DO/DON'T lists for knowledge skills, official procedural patterns (checklists, feedback loops, solve-don't-defer scripts) for runbooks. Divergences from official examples are stated with grounds. Governs the skill rewrites that follow.
Wraps pre-commit/ts/build/pytest into one script that scopes the expensive gates to whichever side of the repo the diff touches, and runs build+pytest concurrently since they live in separate containers with no contention to serialize for.
Merges the two skills into one concern (server endpoints + data model) since AGENTS.md's own selection rule always fires them together. Preserves the shattered-parent contract and UDF policy as grounded, dated content; moves DB/migration/UDF depth into references/db-patterns.md, which now also carries the ParentChildEdges departition/drop/revert history (PR #625, PR #721, and the reverted 2026-07-17 drop attempt) mined from git and PR history.
Documents the actual seven-stage CLI sequence (backend + pipelines) to take a GeoPackage to a working, shattered, contiguity-checked map module, with a verification step after each stage. No script: the sequence requires judgment calls (shatterable vs not, batch vs one-off) that a mechanical wrapper would hide rather than solve.
…orkers into learn-map-frontend These four skills always co-fired on the same concern (how the interactive map renders and responds) per AGENTS.md's guide-selection rules — evidence the split was per-surface, not per-concern. Rewritten per AUTHORING.md: grounded invariants with their why, a territory map, causal history mined from PRs #492, #634/07af68f6, and #550, and sub-topic depth pushed into references/ (layers-and-styling, runtime-events, workers).
…migrations Procedure is generate -> review for dangerous ops (drops, large-table type changes, anything touching the partitioned parentchildedges table, lock- taking rewrites) -> fix downgrade -> upgrade/downgrade/upgrade loop. Uses commit 291cc45 as a worked example of the review habits stated in prose.
…erence Declarative rewrite grounded in fetchDocument.ts, useDocumentWithSync, idb.ts, and the four resolveConflict resolvers in assignmentsStore.ts — works through the updated_at/clientLastUpdated comparison that decides conflict vs. no-conflict and mirrors the server-side 409 check in update_assignments (learn-backend). Adds references/debugging.md (absorbing issue #525's Sync Conflict Debugger): inspecting IDB state, reading the three relevant timestamps, reproducing a conflict on purpose, and replaying each resolution path.
Mines PR #721 (graph-representation refactor, measured benchmarks), PR #540 (graph LRU cache), PR #625 (departitioning under lock contention), and PR #550 (moving unassigned-geometry computation off PostGIS onto a cached graph) into a dated, PR-anchored causal history — the concern spans backend, workers, and infra with no dedicated surface, matching AUTHORING.md's cross-cutting-concern case. Includes EXPLAIN ANALYZE guidance for query-cost triage, kept as method only since this worker cannot run it against a live DB.
…gis-validation and export-testing references One concern — how a map module comes to exist — split across a backend lifecycle skill and a pipelines skill that never fired together despite covering adjacent halves of the same pipeline-to-DB handoff. Rewritten per AUTHORING.md: the onboarding order is grounded in load_data.py's own enforcing docstring, and the pipeline/backend graph-generation split is grounded in transforms/graph.py's module docstring. Adds gis-validation.md and export-testing.md (issue #525), each written against real code paths (EPSG:4326 handling, the `path` column contract, ST_CoverageUnion topology requirements, the four DocumentExportType formats) rather than generic guidance.
Small and hard-rule-dense by design (AUTHORING.md: security is the legitimate home for invariants). Each invariant carries its ground in the actual code: VerifyToken's scope check, the shared-secret/different-aud distinction between session and share JWTs, the two separate Turnstile secrets, and the password-protected edit-access grant in save_share/main.py.
…and FE types audit_contracts.py does name-matched field-set diffing between backend Pydantic/SQLModel classes (via ast, chasing local base-class inheritance) and app/src/app/utils/api/apiHandlers/types.ts (via a brace-depth-aware regex parse). Static analysis only -- no import or execution of repo code. v1 heuristic; limitations are printed with every run.
Thin by design: delegates correctness/simplification/security review to /code-review and security-review, and adds only the repo-specific checks (concern-skill compliance, migration safety, contract drift, gate status, auth surface) that those built-ins have no way to know about.
Grounds the draft/published-column publish model (not a status enum), the TipTap node round-trip pattern via BoilerplateNode as a worked example (attrs + parseHTML/renderHTML + SSR-guarded NodeView), and the moderation-score-vs-review-status relationship in the actual code.
Bun's built-in `bun outdated`/`bun audit` for the frontend; `pip list --outdated` (container) and `uvx pip-audit -r requirements.txt` (host, no env needed) for the backend's uv-compiled lockfile. Survey-and-triage only -- findings get filed, not auto-upgraded.
… reference Quality-gate commands move out (a separate runbook owns them now); memory/lock history moves to learn-performance, cross-referenced rather than duplicated. Adds references/troubleshooting.md (absorbing issue #525's Docker Environment Doctor) as causal symptom -> root cause -> fix entries, each checked against the current docker-compose.yml rather than carried over unverified from the old skill's failure-mode list. Notes the CI/local PostGIS version mismatch (16-3.5 in test-backend.yml vs 15-3.3 in compose) found while verifying.
AGENTS.md's guide list and selection rules described the retired 12-skill surface-sliced layout; replaced with the 14-skill inventory (8 concern-individuated knowledge skills, 6 runbooks) and a pointer to AUTHORING.md, noting that Claude Code routes by description while the listing serves humans and non-routing agents. CLAUDE.md's count line updated to match. audit_contracts.py's first live run paired every suffix-stripped name collision (Assignments x AssignmentsCreate x AssignmentsCreateResponse), reporting 55 pairs of mostly cross-product noise. Exact-name matches now pair first; suffix-stripped matching only resolves leftovers with a single candidate per side. Re-run yields 18 pairs, all same-name, with findings that read as genuine drift.
The original framed get_document vs get_protected_document as the choice governing response safety — wrong axis. get_document gates the write path (requires the true UUID) and never enters a response-shape decision at all. The actual choice a route serving public_id callers faces is get_protected_document (guard-only, never return) vs get_document_public (the function that safely builds a response including the document). Caught in review; rewritten to name the real contrast.
The prior wording ("this, not get_document, is the choice...",
"a different axis entirely... never enters this choice at all")
was written as a rebuttal to a wrong prior draft, presupposing context
a cold reader never has. Restated as a flat, positive description of
what get_protected_document and get_document_public each do — no
reference to a rejected alternative framing.
The prior text framed the split as a hot-path/write-frequency
optimization ("assignments are the hot path... while document
metadata changes rarely") — doesn't hold up: metadata is small enough
that including it in assignment writes would add negligible overhead.
The real reason is cardinality: a document has one metadata row but
many assignment rows (one per geo_id, potentially tens of thousands),
which has no single-row representation on Document regardless of
write frequency. Restated as a flat structural fact.
…able paragraph Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… coupling Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… bullet Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…iew pass Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… rationale The #634/#a02f866 cross-reference pointed at a commit hash that doesn't exist in this repo (the real fix is 07af68f, already named correctly in the causal history section below); replaced with a plain pointer to that section. Also cut an unverified performance rationale ("because a full layer re-render... would be visibly slow") for the setFeatureState invariant — no code comment traces it, so the bullet now states the bare fact.
…rclaim The 'GitHub Actions re-validates every push regardless' line was true only for backend (test-backend.yml runs on any branch push touching backend/**); no equivalent frontend build/test job exists outside a PR preview or a main/dev deploy. Also trimmed a paragraph duplicating run-gates.sh's own printed pass/fail/skip/log-path output.
…ld edges load_sample_data (backend/management/load_data.py) also creates parent-child edges when a view has a child_layer, not just steps 1/2/5 as previously stated — verified against the function body.
… own triggers (per Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(per Ge) Suite and transcripts now live with the work order (orchestration records); AUTHORING's testing section points there via PR #740. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…@AGENTS.md) Imitates PR #719's root agent file, with import instead of duplication so the two can't drift. Content unchanged apart from the one pointer in backend-endpoints. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… against the bd prime hook injection The SessionStart hook already injects the full bd reference and close protocol every session — the eager file now states only the rules the hook can't: bd-not-TodoWrite, and push-completes-work. Markers kept; bd tooling may want to restore the long form (hash now stale). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s imitation/structure-carried (per Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… troubleshooting and duplicate close protocol cut Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…cating the commands Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ew as newcomer-oriented The counts drift; AUTHORING.md sits in the directory a skill-editing session already opens (its own choke point). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ap is newcomer orientation, not agent instruction Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ssion start; the lists were drifting paraphrases Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…in the script's --help, mechanics in AUTHORING Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…md (base on dev) and AUTHORING (canonical source rule); everything else duplicated the eager file or the skills Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…s now purely the skills source; fix stale Fly.io section, decisions section defers to decisions.md Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…isible, self-describing source dir (per Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…metrics removal + this branch's WAF and access-logs rows); graph.py references updated to graph_loader.py
…dpool sites inside def handlers The rebase resolved textual conflicts only; #740's base (post-#721 dev) carried graph pre-warm/load awaits inside handlers this branch had converted to def — a SyntaxError caught by pytest. All three become direct get_graph calls; the handlers already run in the threadpool. Full suite: 531 passed.
…pushed, CI-tested SHA Guards: clean backend/ worktree, local tip == remote tip (ls-remote), and a completed test-backend.yml run for that exact SHA (one gh call inside the script — no extra agent round-trips). CI success skips the ~4-min local run; CI failure still runs locally for a fresh log and prints a pointer to the CI record; any doubt falls through to a normal local run.
nofurtherinformation
left a comment
There was a problem hiding this comment.
Overall looks good and a really solid refactor + update. Just a few minor things that should be cleaned up, but marking as approved to avoid blocking unnecessarily.
It looks like these docs were made before the graph changes landed fully, so there are a few stale references to shatter_parent.sql. A quick pass to remove extra markdown line breaks would be preferred, too.
| children has a row in `document.assignments` — `zone = NULL` for a child that hasn't | ||
| been painted. The frontend uses the presence of a child row (not its zone) to decide | ||
| which block IDs are interactive; a missing row means the frontend can't tell the child | ||
| exists. Two independent write paths uphold this and never reference each other: |
There was a problem hiding this comment.
H: shatter_parent.sql is no longer active, replaced by api/gerrydb/edges/{districtr map slug}?parent_geoid={vtd:123}
There was a problem hiding this comment.
Confirmed and fixed — you're right, and the same merge that brought this in also dropped the UDFs (migration 2ecf1bdc582b), which I'd missed. Rewrote the contract clause to name the live paths: interactive shatter is client-side from graph children served by GET /api/gerrydb/edges/{slug}, CSV import heals/fills via _heal_or_fill. Also corrected the same stale claim in docs/decisions.md and run-map-onboarding (parentchildedges is now write-only pending its drop).
|
|
||
| - **Backend logic lives in Python, where tests, types, and review reach it.** New query | ||
| logic composes as SQLAlchemy/SQLModel expressions and set-based SQL. The UDFs in | ||
| `backend/app/sql/` are retained history, not precedent — some (like `shatter_parent`) |
There was a problem hiding this comment.
H: Noted elsewhere, too -- I think shatter_parent is not used in live paths now?
|
|
||
| ## Constraints | ||
|
|
||
| - **The map must feel synchronous under continuous mouse movement.** This is the value |
There was a problem hiding this comment.
PP: A quick pass to remove hard line breaks from the MD could be good, but non-critical.
There was a problem hiding this comment.
Done — unwrapped hard line breaks across the branch's markdown (skills, docs, AGENTS.md); paragraphs are single lines now.
There was a problem hiding this comment.
I: This seems correct. But, at what point is it worth considering something like HeyAPI?
There was a problem hiding this comment.
Agreed — generating types from the OpenAPI schema is the real fix, and this audit script is explicitly a stopgap for the hand-maintained types.ts. I filed #756 for HeyAPI; the skill now notes it retires if that lands.
| # --- pre-commit: always runs, always sequential (fast; sets up the rest) --- | ||
| pre_commit_status=0 | ||
| echo "Running pre-commit..." | ||
| if docker-compose up pre-commit >"${pre_commit_log}" 2>&1; then |
There was a problem hiding this comment.
H: I think this might mask the error exit code, we might want:
| if docker-compose up pre-commit >"${pre_commit_log}" 2>&1; then | |
| if docker-compose run --rm pre-commit >"${pre_commit_log}" 2>&1; then |
There was a problem hiding this comment.
Applied — run-gates.sh has since been deleted entirely (the skill became judgment-first knowledge with standalone commands), but the docker-compose up pre-commit line had migrated into the skill's Commands block; it's now docker-compose run --rm pre-commit.
… property Caught live while explaining PublicDistrictLayers/PUBLIC_SOURCE_ID during unrelated work on feat/ftv-proportionality-test: "Public" reads as an access-control term (implying a "private district" counterpart that doesn't exist) when it actually means "the read-only share-link/Evaluate viewer," as opposed to MainMap/CoiMap's interactive editor. Added to map-rendering/SKILL.md's Vocabulary section (the terminology-seam trigger) and docs/overview.md's page-composition paragraph, which was also missing PublicMap from the MapPage.tsx composition entirely.
…uidance; skill states the judgment rule Grounded in a session's gate-call history (districtr-v2-3b): mid-work ad hoc scoped checks were correct behavior; the real waste was the script re-gating the whole branch diff on every push. The skill now says: cheapest sufficient check mid-work, this script at push checkpoints, scoped via base-ref or --only. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…backend-test CI (per Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…r Ge) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ledge (per Ge) The script's features (concurrency, diff-derived selection, CI reuse) all served the run-everything model; under the two-case pytest rule and per-gate judgment they optimize cases that no longer occur, while a runnable 'run the gates' artifact invites indiscriminate runs. The skill now states which gates a situation calls for, the raw commands, and two scoping one-liners. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
- backend-endpoints, docs/decisions.md, run-map-onboarding: shatter_parent/ unshatter_parent UDFs were dropped by migration 2ecf1bdc582b; interactive shatter is client-side from graph children (GET /api/gerrydb/edges/); parentchildedges is write-only pending its drop. - run-quality-gate: docker-compose run --rm pre-commit (up does not propagate the container's exit code). - run-api-contract-audit: note stopgap status, pointer to issue #756 (HeyAPI). - Unwrap hard line breaks in the branch's markdown (reviewer preference). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Why most of the old skill text is gone
The agents these files serve can recover most facts about the codebase by reading it — faster than we can keep prose about it true. Text that restates the code is a drift-prone paraphrase that pays context rent on every load. So this PR reorganizes the agent-facing corpus around one question: what can the repository not say about itself? Three kinds of content survive it:
map_type: "default"means a district map) or one concept has several names (shatter/"Break"/"Super Draw").Everything else moved to a better home or was cut. A norm-by-norm review of the previous eight knowledge skills found most written "invariants" were observations promoted to norms: descriptions of the file layout ("all subscriptions wire through one file" — three standing counterexamples, one of them correct design), architecture facts whose violation would require tooling absent from the backend. Of ~20 candidate norms, six survived as genuinely normative and non-derivable. The rest were cut outright, moved to orientation docs, or pushed to the one place a future editor must look anyway (a comment at the choke point — the
ParentChildEdgesdeclaration, the sync-merge override).The resulting shape:
backend-endpoints,map-rendering,map-edit-sync,performance-memory,deploy-authority), each 20–50 lines: norms plus the vocabulary bound to their situations.deploy-authorityandrun-dependency-auditarepaths-gated (auto-load oninfra/**/workflow and manifest edits).run-prefix so procedure triggers read differently from norm triggers. Two of the original seven were cut:run-next-upgrade(pure official-docs paraphrase for a rare task an agent can rederive from the live guide) andrun-pr-review(a compiled list of other skills' triggers — the router already serves each checkpoint).run-quality-gatelater dropped its runner script entirely: gate-call traces from live sessions showed agents running every gate indiscriminately, so the skill is now judgment-first — decide per gate from the situation (a two-case rule for pytest, since CI reruns it on every backend push), then run standalone commands.docs/overview.md— newcomer orientation: what the system is, the full name-concordance table (12 rows), the repository layout, how modules/plans/rendering/sync/auth/CMS fit — anddocs/decisions.md— the dated history of architectural decisions (departitioning, the graph rework, undo-per-gesture, computation placement) with their grounds..agents/ARCHITECTURE.mdalso moved todocs/architecture.md(its stale Fly.io section fixed), and the skills source tree was renamed.agents/skills/→agent-skills/— no harness reads.agents/, so the dir is now visible and self-describing. Orientation lacks a moment of need to route on, so it lives where humans and agents read eagerly, outside the skill layer. These docs are seeds: harvested from the cut skill material and verified, but building them into full documentation is deliberately out of scope here — a future PR's work.map_type: "default"trap — the only two items whose need is undetectable (an agent confidently wrong gets no retrieval signal).agent-skills/AUTHORING.mdnow states the doctrine the above applies: the content test, placement by detectability of need, one skill per trigger, pointer/reference principles (a pointer's condition must be checkable at the moment it's read), and a revision procedure that grows the floor only on trace evidence.Evidence the cuts don't hurt: adversarial evals on the strongest cut content
Cuts were then challenged, not assumed. Four eval cases (official
claude plugin evalformat; run by hand pending early access) each target cut content with the strongest prior claim to being load-bearing, phrased adversarially — a task that invites exactly the mistake the cut line used to guard against. Each ran in fresh sessions against two isolated repo copies: the rebuilt skills vs. the pre-rebuild skills (2 trials per arm, plus a no-skills smoke run; LLM-rubric graded). Suite, rubrics, and all 17 transcripts are preserved at https://github.com/districtr/districtr-scratchpad/tree/main/skill-evals/2026-09-agent-skills-rebuild.docs/decisions.md, the migration's own docstring, or the old skill) and recommended against.statefpscolumn (open #633) in their top two causes from code alone.hoverfeature-state key from a sidebar? (per-key ownership rule cut as generic MapLibre lore)overlays/statefpsto the server (server-owned-fields rule → docs)map-edit-syncloaded, agents refused and cited the docs; with no skill loaded (including both old-skill trials — the old inventory never routed on this framing either), agents confidently deleted the guard.Three of four cuts confirmed safe. The fourth showed the guarded knowledge survives only when routing succeeds — so the durable fix went where routing can't miss: a why-comment at the merge site in
fetchDocument.ts, which both passing runs independently proposed. The suite also caught real repo issues along the way: thestatefpsonboarding gap is now a step-level runbook warning, and a genuineevent_id/eventIdcontract mismatch surfaced (left for a follow-up issue; the audit-driven fix was drafted and deliberately withdrawn from this PR).The eager/lazy split of the always-loaded files
The same doctrine was then applied to the memory files themselves. Root
AGENTS.md(the cross-tool standard name, as in PR #719) now carries the agent instructions, andCLAUDE.mdis a three-line@AGENTS.mdimport — one source, so the two can't drift. Because that file is loaded eagerly into every session, it was pruned to what the official CLAUDE.md guidance admits: three cross-cutting rules (document UUID,map_type: "default", Bun-not-npm), the quality-gate commands, and the pointers that make the lazy layers discoverable. Everything thebd primeSessionStart hook already injects (command reference, session-close protocol) was deduplicated away, along with stack descriptions readable from the lockfiles and the gate commands (now a pointer torun-quality-gate, their one home). The lazily-read hub (.agents/AGENTS.md) went through the same pass — three inconsistent bd sections collapsed, the generated troubleshooting catalog cut, "Frontend Conventions" dissolved (one line promoted to the eager file: Bun-not-npm), the directory tree moved todocs/overview.md, the skills listing cut (agents receive every description at session start; the list was a drifting paraphrase) — at which point its only unique facts were "base work ondev" (now a cross-cutting rule) and the skills sync workflow (now in AUTHORING's mechanics), so the hub file was deleted. The always-loaded surface is a single ~30-line rootAGENTS.md: four pointers, three cross-cutting rules (document UUID, Bun, base-on-dev), a gates pointer, and a minimal beads note.The delivery bug that started all this
scripts/sync-skills.shmirrored the source tree into.claude/skills/project/<name>/, but Claude Code discovers skills exactly one level deep — everylearn-*skill had been outside native routing since the April scaffold. Fix: the Claude sync target flattens to.claude/skills/<name>/(with a duplicate-name guard), and the Cursor target's reference-inlining matchesreferences/as the files actually name it. The source tree is flat too. Verified live: all current skills appear in a fresh session's listing after sync.Appendix: the earlier measurements
Before the norm review, two experiment sets (2026-09-01, isolated
claude -pharness, ~220 runs total) grounded AUTHORING.md's routing and content doctrine:Routing (~170 runs, Haiku + Sonnet): the model matches skills against its own one-sentence restatement of the task. Grammatical phrases in plain engineering nouns triggered 4/4 under every name/description split; the same words scrambled, 0/12; house terms never. A wrong description vetoes invocation (0/11). Two-concern tasks loaded both intended skills 12/12 across three pairs. Haiku consulted the listing essentially never (3/72 direct, 0/20 indirect — even with the skill's territory files open), and subagents invoked a skill once across the whole experiment: worker spawns carry their hard invariants in the prompt.
Point ablations (two rounds, 20 randomly sampled points, skill force-loaded with and without the single point; round-two verdicts predicted in advance, 8/10): 3 load-bearing, 12 derivable, 4 inert, 1 marginal. Every load-bearing point was arbitrary team policy; every generic-lore point tested derivable. One caution became doctrine: the study's strongest point (3/3 vs 0/3 behavior flip) steered toward a rule later judged wrong and rewritten — ablation measures leverage; review measures truth.
Tested
.claude/skills/, hot-loaded into a live session's listingaudit_contracts.pyexecuted against the real repo (86 backend models, 26 frontend types — and it found a real contract mismatch)bun outdated/ audit commands timed (~1.4s);uvx pip-audithost crash found and container fallback documented in the runbookdocker-compose up pre-commitgreen on every commit (hook-enforced)references/*.md; Codex index lists all skills