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