From 27ca1fd9f1870af4fdf67928b068840a958e7895 Mon Sep 17 00:00:00 2001 From: goetchstone Date: Thu, 27 Aug 2026 14:05:58 -0400 Subject: [PATCH] chore(agent): rules become principles, and the learning loop closes Applies the two-skill pattern from Warp's self-improving agents to this repo's agent layer, and restructures CLAUDE.md around principles. PRINCIPLES AS A SPINE, NOT A REPLACEMENT. Three independent restructures were proposed and then attacked for lost enforcement. All three lost it, in the same way: generalising a rule into prose reads better and enforces less. Rule 33's greppable literal and its "No exceptions"; rule 47's polarity, which is OPPOSITE to 33 (cancelled lines out, returned orders in) so a polarity-neutral merger permits both errors; rule 31's stated default; rule 39's "both numerator and denominator"; rule 42's "every mutation path". In a repo where hooks cite these numbers and 203 files quote them, that is a regression wearing a cleanup's clothes. So the rules keep their exact text and their numbers, and ten principles sit above them. Each principle names a failure mode generally enough to recognise a NEW instance, and carries a "how it hides" line -- the recurring theme here is damage that passes every check. Verified mechanically: every rule number is present exactly once, and every surviving rule's text is byte-identical to before, except five that gained clarifying additions and none that lost a clause. Rule 1 (KISS) retired. Zero citations, no incident, no enforcement home, and RULE-PROVENANCE.md already said to delete it -- it was the one rule that contradicted the doctrine it sat under. Number retired, not reused. THE LOOP. The gap that produced 65 numbers with gaps and five conflicting citations was never a missing rule. It was that "does this deserve a rule?" got decided in-session by the agent that had just been burned, and every session got to append. That is accretion, not learning. - docs/RULE-FEEDBACK.md is now the sink. Costs one line, commits to nothing, so the signal keeps flowing. Seeded with three entries from this session. - .claude/skills/improve-rules/SKILL.md is the observer. Runs later, over the accumulated pile, and proposes ONE focused edit as a PR. "No change warranted" is an explicit success case. Retirement runs the same bar in reverse, because a loop that only adds repeats the original failure. - post-failure and end-of-session no longer edit CLAUDE.md. They append. ONE DELIBERATE DEVIATION FROM WARP. Their loop feeds on human PR review comments. Measured here: 20 merged PRs, 0 reviews, 1 comment -- FRAMEWORK.md describes a solo project with no second pair of eyes, so that well is dry. The substitute is the commit that had to clean up after the last one: a fix/revert commit is literally what the agent proposed versus what reality required, with a diff attached, and roughly one commit in four is one. rules-improver-check.sh counts them and nudges past a threshold -- event-driven, so the pass fires when there is something to read rather than on a calendar. It never blocks. 3,479 unit tests pass; markdownlint clean. --- .claude/hooks/rules-improver-check.sh | 64 +++++ .claude/settings.json | 6 + .claude/skills/end-of-session/SKILL.md | 13 +- .claude/skills/improve-rules/SKILL.md | 132 ++++++++++ .claude/skills/post-failure/SKILL.md | 9 +- CLAUDE.md | 349 ++++++++++++++++++------- docs/FRAMEWORK.md | 19 ++ docs/RULE-FEEDBACK.md | 75 ++++++ docs/RULE-PROVENANCE.md | 17 +- 9 files changed, 582 insertions(+), 102 deletions(-) create mode 100755 .claude/hooks/rules-improver-check.sh create mode 100644 .claude/skills/improve-rules/SKILL.md create mode 100644 docs/RULE-FEEDBACK.md diff --git a/.claude/hooks/rules-improver-check.sh b/.claude/hooks/rules-improver-check.sh new file mode 100755 index 00000000..016bbbb3 --- /dev/null +++ b/.claude/hooks/rules-improver-check.sh @@ -0,0 +1,64 @@ +#!/usr/bin/env bash +# .claude/hooks/rules-improver-check.sh +# +# Nudges toward `.claude/skills/improve-rules/SKILL.md` once enough evidence has +# accumulated to be worth a pass. +# +# Event-driven, not time-driven, on purpose. A weekly cron runs whether or not +# anything happened, and a pass with nothing to read either returns "no change" +# (wasted) or invents one (worse). A fix commit is the signal that the rules met +# reality and something gave, so counting those fires the improver exactly when +# there is something to learn from. +# +# NEVER blocks. This is a prompt for a human decision, not a gate — the whole +# point of the improver is that it runs with distance, so forcing it mid-session +# would defeat it. + +set -euo pipefail +cd "$(dirname "$0")/../.." + +THRESHOLD="${RULES_IMPROVER_THRESHOLD:-8}" +STAMP=".claude/.rules-last-run" +SINCE=$(cat "$STAMP" 2>/dev/null || echo "") + +if [ -n "$SINCE" ]; then + RANGE=(--since="$SINCE") +else + # No stamp yet: look back a sensible window rather than all of history. + RANGE=(--since="6 weeks ago") +fi + +FIXES=$(git log "${RANGE[@]}" --format='%s' 2>/dev/null \ + | grep -icE '^(fix|revert)' || true) +FIXES=${FIXES:-0} + +LEDGER=0 +if [ -f docs/RULE-FEEDBACK.md ]; then + if [ -n "$SINCE" ]; then + # Ledger entries are "### YYYY-MM-DD — ..."; count those dated after the stamp. + LEDGER=$(grep -oE '^### [0-9]{4}-[0-9]{2}-[0-9]{2}' docs/RULE-FEEDBACK.md \ + | awk -v s="$SINCE" '{ if (substr($2,1,10) > s) n++ } END { print n+0 }') + else + LEDGER=$(grep -cE '^### [0-9]{4}-[0-9]{2}-[0-9]{2}' docs/RULE-FEEDBACK.md || true) + fi +fi +LEDGER=${LEDGER:-0} + +SIGNAL=$(( FIXES + LEDGER )) + +if [ "$SIGNAL" -ge "$THRESHOLD" ]; then + cat < .claude/.rules-last-run + ``` + +Then stop. A human merges it, and the next session inherits the improvement. diff --git a/.claude/skills/post-failure/SKILL.md b/.claude/skills/post-failure/SKILL.md index 998b9571..726429e0 100644 --- a/.claude/skills/post-failure/SKILL.md +++ b/.claude/skills/post-failure/SKILL.md @@ -35,8 +35,13 @@ Fix the actual issue. Verify with `cd app && npm run validate && npm test`. section, or a tripwire test naming the bug class (prefer a real-DB integration test over a source-text tripwire where the behavior is testable — rule 57). -- **Business rule / cross-cutting invariant** → propose a CLAUDE.md rule - update with an origin pointing at this entry. +- **Business rule / cross-cutting invariant** → append an entry to + `docs/RULE-FEEDBACK.md`, not a rule to CLAUDE.md. Say which principle this + is an instance of, or `NEW` if none fits, and answer "would a rule have + caught it?" — the answers "no" and "only if enforced differently" are the + valuable ones. `.claude/skills/improve-rules/SKILL.md` reads the ledger + later and proposes the constitutional change as a PR, with the distance that + makes the judgment worth trusting. - **Data assumption** → update the relevant `docs/domains/*.md` runbook. ## Step 5: Update the Failure Log diff --git a/CLAUDE.md b/CLAUDE.md index 2b5f7a8c..41379049 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -10,9 +10,8 @@ truth. ## The agent layer -Per `docs/FRAMEWORK.md`, this file is the **constitution**: short numbered rules, -read every session. Detail lives elsewhere, and this file points at it rather -than repeating it. +Per `docs/FRAMEWORK.md`, this file is the **constitution**, read every session. +Detail lives elsewhere, and this file points at it rather than repeating it. | Layer | Where | When to read | |---|---|---| @@ -22,71 +21,112 @@ than repeating it. | Procedures | `.claude/skills/*/SKILL.md` | at the moment they apply | | Hard gates | `.claude/hooks/*.sh` | enforced automatically | | Rule provenance + conflicts | `docs/RULE-PROVENANCE.md` | when a rule's origin matters | +| How rules change | `.claude/skills/improve-rules/SKILL.md` | when the ledger has accumulated | -**Numbering has gaps.** Rules were reconstructed from citations across the repo; -where no citation resolved to a number, nothing was invented. Five numbers have -conflicting citations — see `docs/RULE-PROVENANCE.md`. +## How to read this file -## Rules +**Principles are the spine; rules are the instances.** Each principle names a +way this system fails, general enough that you can recognise a NEW instance +before it ships. Beneath it sit the numbered rules that earned it — each one a +specific incident, stated in the exact operational terms that make it +checkable. -### Engineering standards +Read the principles to know what to look for. Read the rule under it for what +to actually write. **The rule text is the enforceable one** — where a principle +and a rule seem to differ, the rule wins, because the rule is what the hooks, +tripwires and code comments cite. -1. **KISS.** Prefer the straightforward solution over the clever one. -5. **LTS only** for every runtime and dependency. Pre-release only to close an - active CVE with confirmed exposure — document it, revert on the next GA. - Majors get their own planned session. -6. **One source of truth per concept.** Shared logic lives in `src/lib/`. -7. **Shared client/server contracts live in one file** and are imported by both - sides, so contract drift is a compile error rather than a runtime 400. +**Numbers are permanent.** 203 files cite rules by number; rule 33 alone is +cited 65 times. A number is never reused, never renumbered, and never removed +while anything cites it — a rule that stops earning a constitutional slot is +demoted to its runbook with the number preserved so the citation still +resolves. Numbering has gaps because rules were reconstructed from citations +and nothing was invented to fill them. Five numbers have conflicting citations +— see `docs/RULE-PROVENANCE.md`. -### Code quality and testing +**Rules earn their place by surviving incidents** (`docs/FRAMEWORK.md`). A rule +with no incident behind it is a preference, and preferences do not belong here. +That is also the test for adding one — see "Changing this file" at the end. -11. **Surface backend error messages** via `getErrorMessage(err, fallback)`, - never a generic "Failed to X." -12. **Tests exercise actual behaviour.** Real-DB integration tests are the - default behind Prisma. Source-text tripwires are for "someone removed the - guard" classes only. See also rule 57. -13. **Restoration migrations derive values from a column the corruption didn't - touch** — never from a memo. → `docs/domains/import-pipeline.md` -14. **Test the logic, not the wrapper.** Branching logic belongs in pure - helpers in `lib/`; handlers shrink to auth, Prisma, and error handling. +## Principles + +### 1. NULL is an escape hatch, not a value + +A nullable column is neither equal nor unequal to anything, so a row holding +NULL escapes a negative filter and never matches a unique key. Any predicate or +key touching a nullable column must name NULL explicitly, or it silently means +something other than what it reads as. + +**How it hides:** the code says what you meant. It compiles, it reads correctly, +and only the database disagrees. On the read side the total is merely low; on +the write side an operation meant to converge instead inserts a fresh row every +run. + +51. **Never use a naked `not:`/`notIn:` on a nullable column** — three-valued + logic drops NULL rows silently. Use `OR: [{ col: null }, { col: { not: X } }]`. +64. **Never upsert through a compound unique key containing a nullable column.** + Postgres treats NULLs as DISTINCT in a unique index unless it is declared + `NULLS NOT DISTINCT` — none of ours are. So the constraint does not prevent + duplicate rows, and the upsert never matches: it creates another row every + time. Use `findFirst` + increment-or-create with an explicit `col: null`. + `InventoryPosition`'s key has two nullable columns and shipped with exactly + this bug in both `allocate` and `release`; free stock would have multiplied + on every cancel. **Tell:** if you need `null as unknown as number` to make + an upsert compile, the operation is invalid, not the types. + +### 2. An aggregate must declare its population + +A row's presence in a table is not a claim that it counts. Every sum, count and +status recalculation states which rows it includes — in numerator and +denominator alike — or it silently answers a different question than the one +asked. Note the two directions are opposite and both are errors: cancelled +lines must come **out**, returned orders must stay **in**. -### Money and reporting invariants +**How it hides:** the number stays plausible. Nothing in the system knows the +right answer to compare against, so a total that is confidently wrong by a +believable margin survives indefinitely. The highest-consequence cluster in the codebase. Detail and worked examples: → `docs/domains/reporting.md`, `docs/domains/accounting.md` +31. **Zero-quantity source rows are cancelled lines.** Every aggregation decides + explicitly whether to include them. Default: exclude. 33. **Exclude cancelled lines from every sum and count** — `lineItemStatus: { not: "CANCELLED" }`. No exceptions. -37. **Business-rule definition catalogs live in exactly one file**; every - consumer imports from it. +39. **PO receiving-status recalculation excludes zero-quantity lines from both + numerator and denominator** — otherwise a lingering zero-qty line traps a PO + at `RECEIVED_PARTIAL` forever. Corollary of 31. + → `docs/domains/purchasing.md` 40. **Status is a broad hammer.** Fix a bad import at the import boundary; never patch a reporting symptom by mutating status on good rows. 41. **Ground defaults and thresholds in production data**, and hand-classify the near-boundary cases before shipping — an aggregate-only check hides cases that split both ways. -42. **A safety guard is one shared function on every mutation path that needs - it.** Present on one path and missing on another is no guard at all. - → `docs/domains/commission.md` 47. **Revenue queries include RETURNED orders** — `status: { in: SALES_REVENUE_STATUSES }`. Sister rule to 33: that one governs `lineItemStatus` (cancelled out), this governs `SalesOrder.status` (returned in). -51. **Never use a naked `not:`/`notIn:` on a nullable column** — three-valued - logic drops NULL rows silently. Use `OR: [{ col: null }, { col: { not: X } }]`. -64. **Never upsert through a compound unique key containing a nullable column.** - Postgres treats NULLs as DISTINCT in a unique index unless it is declared - `NULLS NOT DISTINCT` — none of ours are. So the constraint does not prevent - duplicate rows, and the upsert never matches: it creates another row every - time. Use `findFirst` + increment-or-create with an explicit `col: null`. - `InventoryPosition`'s key has two nullable columns and shipped with exactly - this bug in both `allocate` and `release`; free stock would have multiplied - on every cancel. **Tell:** if you need `null as unknown as number` to make - an upsert compile, the operation is invalid, not the types. -60. **Route by recorded fact, not current config.** Refunds and webhooks resolve - the processor from the payment's stored `processorType`, never from whichever - provider is active now. +### 3. A guard on one path is no guard + +An invariant that must hold at more than one site is only as strong as the site +you forgot. Enumerate the set mechanically — callers, mutation paths, models, +routes — put the invariant in one shared function, and add a check that fails +when the enumeration and reality disagree. + +**How it hides:** the reported path gets fixed and the duplicate-shaped path +does not, so the symptom returns days later through a different route and reads +as a new bug. No test catches it, because the uncovered site is the one nobody +listed. + +42. **A safety guard is one shared function on every mutation path that needs + it.** Present on one path and missing on another is no guard at all. + → `docs/domains/commission.md` +45. **Trace before refactoring shared infrastructure.** A "mechanical" change to + auth, payments, import runners, or uploads carries a `grep -rn` for callers + in the PR. +49. **Self-heal as you go.** Fixed a bug shape → grep for it elsewhere. + Mechanical sweep → fix every site in the same PR with one regression test. 65. **Anything countable gets a manifest and a tripwire.** When the repo has a set of things that must stay covered — every model seeded, every migration guard applied, every route gated — do not rely on noticing. Write down the @@ -113,7 +153,9 @@ The highest-consequence cluster in the codebase. Detail and worked examples: `prisma/testing/db-guards.sql` + `__tests__/dbGuardsCoverage.test.ts`; `__tests__/schemaNormalization.test.ts` (text-beside-its-own-FK columns, each accepted pair carrying the measurement that justified it); - `__tests__/fixtures/ungated-read-api-routes.txt`. + `__tests__/fixtures/ungated-read-api-routes.txt`; + `__tests__/clientDataTripwire.test.ts` (patterns base64-encoded — a guard + listing a real company's identifiers in plaintext is itself the leak). A ratchet beats a cleanup. Where the debt is real but removing it is not worth it today, freeze the set and require a written argument to grow it — @@ -122,7 +164,79 @@ The highest-consequence cluster in the codebase. Detail and worked examples: units of work, name them — a named unit is delegable; "the rest of the gap" is not. -### Imports and legacy data +### 4. Verify with an instrument that can disagree + +A check counts as evidence only if it was structurally capable of failing for +the real reason. A mock returns what you stubbed; an incremental install is not +the clean tree CI builds; a source-text scan agrees as long as the text is +there. Each is a proxy — fine where the proxy is the point, wrong where it +stands in for the thing that ships. + +**How it hides:** green. The signal you look at cannot contradict you, so +confidence rises exactly as the evidence stops meaning anything. + +Note this does not demote tripwires generally — rule 65 requires them for +coverage sets. The failure is using one where a real assertion was possible, +and never proving it can fail. + +12. **Tests exercise actual behaviour.** Real-DB integration tests are the + default behind Prisma. Source-text tripwires are for "someone removed the + guard" classes only. See also rule 57. +14. **Test the logic, not the wrapper.** Branching logic belongs in pure + helpers in `lib/`; handlers shrink to auth, Prisma, and error handling. +52. **Verify dependency and lockfile changes with `npm ci`, never `npm + install`.** CI installs clean from the lockfile; an incremental install + hides breakage that only appears on a clean one. +57. **Prefer behavioural tests over source-text tripwires** where the behaviour + is testable. Tripwires stay correct for "this guard must exist everywhere" + invariants — the failure mode is using one where a real assertion was + possible. + +### 5. A claim is exactly as strong as its evidence + +Never stronger than what was verified, never weaker than what is already known. +This governs messages on screen, lines in runbooks, and sentences in a PR body +alike. + +**How it hides:** the claim is the only artifact, and it does not drift when +reality does. Nobody gets a signal that it went false — they just act on it. + +11. **Surface backend error messages** via `getErrorMessage(err, fallback)`, + never a generic "Failed to X." (The under-claiming direction: a catch that + renders "Failed to save" discards a backend message that said precisely + what was wrong.) +19. **Runbooks are pinned against source**, not plausibility — cite source + opened this session or write `[NEEDS VERIFICATION]`. +56. **Verify claims against code, not docs.** Docs drift; source cannot lie + about its own current state. +58. **Report unverifiable work as unverified.** If a claim can't be exercised in + this environment, say so rather than implying it was tested. + +### 6. One home per fact + +A fact with two homes has one home and one copy, and the copy is wrong in +whichever direction nobody looked. + +**How it hides:** both copies were right when written. Drift is invisible until +the two are compared, and nothing compares them. + +6. **One source of truth per concept.** Shared logic lives in `src/lib/`. +7. **Shared client/server contracts live in one file** and are imported by both + sides, so contract drift is a compile error rather than a runtime 400. +37. **Business-rule definition catalogs live in exactly one file**; every + consumer imports from it. (Specialisation of 6 and 7 for enum-like + catalogs — `permissionCatalog`, the runner registry, + `SALES_REVENUE_STATUSES`.) + +### 7. Deployment facts are configuration + +Anything true of one deployment and not of the product is data, not code. This +is the rule the codebase violates most often and most quietly, because a +hardcoded literal works perfectly for the deployment it was written for. + +**How it hides:** it is correct — for one tenant. The second deployment does not +error; it silently routes to the wrong runner, classifies an ordinary order as a +return, or imports nothing and logs "skipped". → `docs/domains/import-pipeline.md`, `docs/domains/imports-overview.md`, `docs/domains/config-presets.md` @@ -141,40 +255,83 @@ The highest-consequence cluster in the codebase. Detail and worked examples: before writing; a second apply must write nothing. A preset is desired state, so a mapping deleted from the file is deleted from the database. -31. **Zero-quantity source rows are cancelled lines.** Every aggregation decides - explicitly whether to include them. Default: exclude. -39. **PO receiving-status recalculation excludes zero-quantity lines from both - numerator and denominator** — otherwise a lingering zero-qty line traps a PO - at `RECEIVED_PARTIAL` forever. Corollary of 31. - → `docs/domains/purchasing.md` + **Unconfigured must fail closed, not guess.** A permissive default is a + deployment fact you invented. `[A-Z]{2,}` as a store code classified `SOFA1` + and `MEGA1234` as returns; `.+_` as a report prefix routed + `Deleted_Customers.csv` into the customer master. Where there is no safe + universal value, match nothing and say so. + +### 8. Irreversible paths fail closed + +Destructive and unrecoverable operations refuse by default and require an +explicit, separately-named opt-in. Allowlist, never blocklist: a blocklist of +known-dangerous cases fails open for the one nobody thought of, which is always +the one that costs someone their data. + +**How it hides:** it works every time you run it correctly. The failure needs +one unfamiliar input — a database name nobody listed, a processor that has since +been switched — and by then it has already happened. + +13. **Restoration migrations derive values from a column the corruption didn't + touch** — never from a memo. → `docs/domains/import-pipeline.md` +59. **`fbc_test_db` is the only database tests may write**, and the demo seed + writes only a database whose NAME says it exists to be seeded (`holt_demo`, + `holt_seed_demo`, `ci`). The token seed/demo/scratch/sandbox/sample/ci must + be delimited by `_` or the ends of the name, so `holt-demo`, `demo2` and + `holt_samples` are all refused — near-misses are refused on purpose, since + a name that only nearly says "scratch" is exactly the one that turns out to + hold something. Every other database is assumed to hold restored, curated or + live local data and needs an explicit `--force-unsafe-db`; the integration + test database is refused even with it. Allowlist, not blocklist: a blocklist + of known-dangerous names fails open for the one nobody thought of, which is + always the one that costs someone their data. The `DATABASE_URL must contain + 'test'` guard in `src/lib/testing/withTestDb.ts` is a floor, not a substitute + for pointing at the right database. Enforced by + `prisma/seed/demo/guard.ts`, tested in `__tests__/seedTargetGuard.test.ts`. +60. **Route by recorded fact, not current config.** Refunds and webhooks resolve + the processor from the payment's stored `processorType`, never from whichever + provider is active now. + +### 9. Nothing leaves the session unaddressed -### Session and workflow discipline +Every finding, every deferral, every learning ends somewhere durable. The +alternative is not "we'll get to it" — it is silently dropped, and nobody knows +the difference. -→ `docs/WORKFLOW.md`, and the `pre-pr` / `pre-commit` skills +**How it hides:** intent feels like completion. A verbal "I'll sweep the rest" +and a tracked plan are indistinguishable at the end of a session and completely +different a week later. + +**18 vs 49 — which wins.** 18 says ship the simplest fix to the symptom; 49 says +fix every site of a bug shape in one PR. They pull opposite ways on the same PR +and 50 is the resolution: fix the reported symptom now, sweep only what the same +PR can prove, and the unswept sites go into a tracked plan — never a verbal +promise. Scope of the fix is 18's call; scope of the *record* is 50's, and 50 is +not optional. 18. **Ship the simplest fix to the reported symptom.** Bundle prevention layers only when asked; spawn the rest. -19. **Runbooks are pinned against source**, not plausibility — cite source - opened this session or write `[NEEDS VERIFICATION]`. 36. **Read before working, update after learning.** Read the domain runbook before touching a domain; update it before closing the session. -45. **Trace before refactoring shared infrastructure.** A "mechanical" change to - auth, payments, import runners, or uploads carries a `grep -rn` for callers - in the PR. 48. **Every scan finding ends in one of three states:** fixed, tripwire-tested, or explicitly won't-fix with rationale. Never silent-ignore. -49. **Self-heal as you go.** Fixed a bug shape → grep for it elsewhere. - Mechanical sweep → fix every site in the same PR with one regression test. 50. **Deferred work goes into a tracked plan**, never a verbal promise. The PR body says where. -### Dependencies and CI hygiene +### 10. Own the version state you depend on + +You are responsible for the tree that ships, not the one that resolved on your +machine. A suppression without an expiry is a permanent decision made by +someone who thought it was temporary. + +**How it hides:** it resolves locally. The gate is green until an unrelated PR +turns it red for reasons that have nothing to do with that PR. Full playbook: → `.claude/skills/dependency-sweep/SKILL.md` -52. **Verify dependency and lockfile changes with `npm ci`, never `npm - install`.** CI installs clean from the lockfile; an incremental install - hides breakage that only appears on a clean one. +5. **LTS only** for every runtime and dependency. Pre-release only to close an + active CVE with confirmed exposure — document it, revert on the next GA. + Majors get their own planned session. 53. **Never blanket-override a package with incompatible major lines.** Use version-scoped (`pkg@1`) or path-scoped (`minimatch@10>brace-expansion`) overrides. @@ -183,33 +340,39 @@ Full playbook: → `.claude/skills/dependency-sweep/SKILL.md` 55. **Sweep CVEs before starting a merge train.** New advisories turn the gate red for reasons unrelated to any pending PR. -### Verification and honesty - -56. **Verify claims against code, not docs.** Docs drift; source cannot lie - about its own current state. -57. **Prefer behavioural tests over source-text tripwires** where the behaviour - is testable. Tripwires stay correct for "this guard must exist everywhere" - invariants — the failure mode is using one where a real assertion was - possible. -58. **Report unverifiable work as unverified.** If a claim can't be exercised in - this environment, say so rather than implying it was tested. - -### Data safety - -59. **`fbc_test_db` is the only database tests may write**, and the demo seed - writes only a database whose NAME says it exists to be seeded (`holt_demo`, - `holt_seed_demo`, `ci`). The token seed/demo/scratch/sandbox/sample/ci must - be delimited by `_` or the ends of the name, so `holt-demo`, `demo2` and - `holt_samples` are all refused -- near-misses are refused on purpose, since - a name that only nearly says "scratch" is exactly the one that turns out to - hold something. Every other database is assumed to hold restored, curated or - live local data and needs an explicit `--force-unsafe-db`; the integration - test database is refused even with it. Allowlist, not blocklist: a blocklist - of known-dangerous names fails open for the one nobody thought of, which is - always the one that costs someone their data. The `DATABASE_URL must contain - 'test'` guard in `src/lib/testing/withTestDb.ts` is a floor, not a substitute - for pointing at the right database. Enforced by - `prisma/seed/demo/guard.ts`, tested in `__tests__/seedTargetGuard.test.ts`. +## Retired + +**1. KISS.** Retired 2026-08-26. Zero citations across the 203 rule-citing +files; no incident, no origin link, and no enforcement home in a skill, hook or +tripwire. `docs/RULE-PROVENANCE.md` already flagged it as deletable. It was also +the one rule that contradicted the doctrine it sat under — a preference, not +something an incident taught. The number is retired, not reused. + +## Changing this file + +A rule enters only by surviving an incident, and it enters through +`.claude/skills/improve-rules/SKILL.md` — an observer pass that runs over +accumulated evidence, not a decision made mid-task while the incident is still +warm. That distance is the point: deciding in the moment is how this file +reached 65 numbers with gaps and five conflicting citations. + +The bar for a proposed change: + +- **Cite the incident.** A commit SHA, a PR number, or a ledger entry. No + citation, no rule. +- **Say where it is enforced.** Skill (persuasive), hook (hard gate) or tripwire + (backstop) — per `docs/FRAMEWORK.md` §3. A rule with no enforcement home is a + wish. +- **Prefer strengthening a principle to adding a number.** Most new incidents + are new instances of a failure mode already named here. A new number is for a + failure mode that is genuinely new. +- **One change per PR.** The improver proposes a single focused edit so it can + be judged on its own. + +Retirement runs the same way and needs the same evidence: a rule whose code path +is gone, whose guard has never fired, or which no longer has citations. Demote +it to its runbook with the number preserved so existing citations still resolve; +delete a number only when nothing cites it. ## Stack and gates diff --git a/docs/FRAMEWORK.md b/docs/FRAMEWORK.md index 63e5a5d5..c0f2eb8e 100644 --- a/docs/FRAMEWORK.md +++ b/docs/FRAMEWORK.md @@ -85,6 +85,25 @@ Concrete artifact: `.claude/skills/post-failure/SKILL.md`. Every incident in the **Rules earn their place by surviving incidents.** Don't add a rule because it sounds good; add it when an incident proves it's needed. The rule then has gravitas because it has a story behind it. +### 4a. Closing the loop: the improver pass + +The loop above has a gap that took 65 numbered rules to become obvious. The decision *"is this recurring shape worth a rule?"* was made in-session, by the agent that had just been burned by it. That agent is the worst-placed judge in the system: the incident is warm, the lesson feels universal, and every session gets to append. The result is CLAUDE.md with numbering gaps and five conflicting citations — accretion, not learning. + +The fix borrows the two-skill split from [how Warp builds self-improving agents](https://claude.com/blog/how-warp-builds-self-improving-agents-on-claude): an **inner** skill that does the work, and an **outer** skill that observes how it went and proposes a change to the inner one. + +| | Inner | Outer | +|---|---|---| +| What | `CLAUDE.md` + runbooks | `.claude/skills/improve-rules/SKILL.md` | +| Runs | every session | when evidence has accumulated | +| Reads | the rules | how the rules performed | +| Writes | code | a PR against `CLAUDE.md` | + +Sessions no longer edit CLAUDE.md. They append to **`docs/RULE-FEEDBACK.md`**, a ledger that costs one line and commits to nothing — low friction is what keeps signal flowing. The improver reads the accumulated pile later, with distance, and proposes **one focused edit** as a pull request. A human merges it; the next session inherits it. + +**One deliberate deviation from Warp's design.** Their loop is fed by human PR review comments — "what the agent suggested versus how humans responded". That signal does not exist here: 20 merged PRs carry 0 reviews and 1 comment between them, because §1 of this document describes a solo project with no second pair of eyes. The substitute is **the commit that had to clean up after the last one**. A `fix:` or `revert:` commit is literally what the agent proposed versus what reality required, with a diff attached and zero friction to capture, and roughly one commit in four is one. `.claude/hooks/rules-improver-check.sh` counts them and nudges once enough accumulate — event-driven rather than scheduled, so the pass fires when there is something to read rather than on a calendar. + +**Retirement is part of the loop, not an afterthought.** Accretion was the failure mode; a loop that only adds repeats it. The same evidence bar runs in reverse — a rule whose code path is gone, whose guard has never fired, or which nothing cites. Numbers are never reused: a rule that stops earning a constitutional slot is demoted to its runbook with the number intact, so the 203 files citing rules by number keep resolving. + --- ## 5. The four discipline anti-patterns diff --git a/docs/RULE-FEEDBACK.md b/docs/RULE-FEEDBACK.md new file mode 100644 index 00000000..566ad4f4 --- /dev/null +++ b/docs/RULE-FEEDBACK.md @@ -0,0 +1,75 @@ +# Rule feedback ledger + +Append-only. **Observations go here, not into CLAUDE.md.** + +That split is the point. Deciding what deserves a constitutional rule *while the +incident is still warm* is how CLAUDE.md reached 65 numbers with gaps and five +conflicting citations — every session, in the moment, judged its own bruise +worth a rule. This ledger costs nothing to write and commits to nothing. The +judgment happens later, with distance, in +`.claude/skills/improve-rules/SKILL.md`. + +So: write the entry even when you are not sure it matters. An entry that turns +out to be noise costs one line. The entry you skipped because it felt too small +is the one the next incident needed. + +## Format + +One entry per observation, newest at the bottom. + +``` +### YYYY-MM-DD — one-line summary + +- **Signal:** fix-commit | CI failure | user correction | tripwire fired | near miss +- **What happened:** what was proposed or shipped, and what reality required instead. +- **Principle:** the CLAUDE.md principle this is an instance of, or `NEW` if none fits. +- **Would a rule have caught it?** yes (which) / no / only if enforced differently (how). +``` + +The last field is the one that matters. "No" is a useful answer — it means the +rules were fine and something else failed. "Only if enforced differently" is the +most useful of all: it says the rule exists but lives in the wrong layer, which +is a change to *where* it is enforced, not to *what* it says +(`docs/FRAMEWORK.md` §3: skill / hook / tripwire). + +## Entries + +### 2026-08-26 — a drift test compared words instead of decisions, and passed while broken + +- **Signal:** user correction + fix-commit `412a314` +- **What happened:** `setup.sh` reimplements `guard.ts`'s database allowlist in + shell. The test asserting the two stayed in step checked that the shell file + *contained the same tokens*. It did — while using unanchored substring globs + against the guard's word-bounded regex, so `setup.sh` accepted `holt_samples` + and `demolition_prod`, ran migrate and seed:roles into them, and only then did + the seed refuse. The test passed the entire time this was broken. +- **Principle:** 4 (verify with an instrument that can disagree). +- **Would a rule have caught it?** Only if enforced differently. Rule 12 already + says tests exercise actual behaviour; nothing says that when one rule has two + implementations, the test must compare their *decisions* over a shared input + set rather than their text. Candidate strengthening of principle 4. + +### 2026-08-26 — permissive defaults invented a deployment fact + +- **Signal:** fix-commit `412a314` +- **What happened:** de-tenanting turned hardcoded literals into config with + permissive defaults so nothing broke unconfigured. `[A-Z]{2,}` as a store code + classified `SOFA1` and `MEGA1234` as returns and subtracted them from revenue; + `.+_` as a report-name prefix routed `Deleted_Customers.csv` into the customer + master. Both were invisible: no fixture used a non-store word. +- **Principle:** 7 (deployment facts are configuration). +- **Would a rule have caught it?** No — rules 61-63 covered moving the literal + out, and said nothing about what the default should be. Added to 63 as + "unconfigured must fail closed, not guess", cited to this incident. + +### 2026-08-26 — the guard against publishing identifiers published them + +- **Signal:** tripwire fired (own audit) +- **What happened:** `clientDataTripwire.test.ts` listed, in plaintext in a + public repo, the surnames, town names, ZIP and phone numbers it existed to + keep out — concentrating in one indexed file exactly what every other file had + been scrubbed of. +- **Principle:** 3 (a guard on one path is no guard) — obliquely; the real shape + is "the guard is inside the set it guards". +- **Would a rule have caught it?** No. Possibly too narrow to generalise; left + here rather than promoted, as a data point in case a second instance appears. diff --git a/docs/RULE-PROVENANCE.md b/docs/RULE-PROVENANCE.md index f8fc4f8e..7f1b5678 100644 --- a/docs/RULE-PROVENANCE.md +++ b/docs/RULE-PROVENANCE.md @@ -33,12 +33,21 @@ remember the original intent, this is the list to settle. | 47 | Revenue queries include RETURNED | The sibling repo's `pre-pr` skill numbers a different rule 47 ("don't relitigate user-empirical claims"); Holt's code-level citation won | | 49 | Self-heal as you go | `detailedSalesVendorPivot.test.ts` cites 49 for "vendor join required on pivot select" — kept as a `reporting.md` note | -**Rule 1 (KISS)** has no surviving Holt citation at all — it is carried by -convention from the sibling constitution. Delete it if you disagree; nothing -depends on it. +**Rule 1 (KISS)** — **retired 2026-08-26**, on the invitation this entry +already extended. It had no surviving Holt citation, no incident, and no +enforcement home in a skill, hook or tripwire; it was carried by convention +from the sibling constitution. It was also the one rule that contradicted the +doctrine it sat under — a preference, not something an incident taught. The +number is retired, not reused; see `## Retired` in `CLAUDE.md`. (The single +`git grep` hit for "rule 1" is `sameDayRewriteCleanup.ts:59`, which refers to a +rule numbered locally inside that file's own comment, not to this one.) **Rule 37** is evidenced only in Holt, with no sibling counterpart. It reads as a -specialization of rules 6 and 7. +specialization of rules 6 and 7 — and now sits beneath them, under principle 6 +("one home per fact") in `CLAUDE.md`, with its number and its ten citations +intact. Kept rather than merged away because those citations point at concrete +instances (`permissionCatalog`, the runner registry, `SALES_REVENUE_STATUSES`) +that a reader arriving from one of them needs to land on. ## Origins