From fe6a88368db9937661227603616281aa83b88bc0 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 12:29:19 +0500 Subject: [PATCH 01/21] docs(root): promote seven confirmed review patterns into the agent guide These are not new advice. Each has at least one dated instance in merged code, and several have three or four across packages that share no source. AGENTS.md takes the five that fit as short enforced rules -- three on testing, where the recurring failure is a green that carries no information, and two on deriving rather than duplicating. The two that need a paragraph go to .claude/rules: how a derived check diverges from what it checks, and why a squash merge makes every ancestry check unsound. Nothing here is mechanically enforced, so no open PR starts failing. What changes is what a reviewer applies. --- .claude/rules/derived-checks.md | 60 ++++++++++++++++++++++++++ .claude/rules/verifying-merged-work.md | 60 ++++++++++++++++++++++++++ AGENTS.md | 23 ++++++++++ 3 files changed, 143 insertions(+) create mode 100644 .claude/rules/derived-checks.md create mode 100644 .claude/rules/verifying-merged-work.md diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md new file mode 100644 index 0000000000..232d5486b2 --- /dev/null +++ b/.claude/rules/derived-checks.md @@ -0,0 +1,60 @@ +--- +paths: + - "packages/**/*.ts" + - "packages/**/*.tsx" +--- + +When one piece of code checks, mirrors or summarises what another produces: + +## Derive it, do not recompute it + +A narrower view must be derived FROM the richer one. Two implementations of the +same question agree on the day they are written and drift afterwards, and the +drift is silent because both look correct in isolation. + +This has caused defects in five unrelated packages that share no code: block +rendering versus derived metadata, a contrast validator versus its measurement, +an email form schema versus its descriptors, a changeset's package list versus +the release group, and a data probe versus the conversion it guards. + +Two rules follow, and the second is the one that gets missed: + +- Export the answer from one place and have both callers ask it. +- **A test is a derived view of the code like any other.** Prefer OBSERVING the + real call — spy on the arguments a function actually receives — over + reconstructing the same call in the test. A hand-copied argument list keeps + passing after someone edits the line the test exists to watch. + +## A derived check must match on three axes, not one + +Asking "does it compute the same thing" is not enough: + +1. **Computation** — the same expression. +2. **Domain** — the same rows, records or inputs. A probe using the identical + expression over a different row set is still a divergence. **An existential + search may short-circuit; a universal claim may not.** `LIMIT 1` is sound + when hunting a counterexample and unsound when hunting a witness, and the + two look identical in the source. +3. **Failure semantics** — "the answer is no" and "I could not ask" are + different outcomes. A check that reports a lock timeout as a data verdict + blocks valid work while naming the wrong cause. Pin the error you mean, and + remember the signal may be WRAPPED: a driver error's code often lives on + `.cause`, and how deep depends on the transport, not on your code. + +Underneath all three sits time-of-check-to-time-of-use, which may need a +different answer per dialect. Say which dialect a mitigation covers rather than +implying one policy fits all. + +## A bare `catch` is only a defect when its fallback makes a CLAIM + +`catch { return conservative }` that degrades to caution is sound, and this +repo has several that are deliberately so. `catch { return verdict }` that +asserts something about the user's data manufactures a confident wrong +diagnosis — and it poisons every test asserting the negative outcome, because +those tests go green on the strength of _some_ error rather than the right one. + +**The direction is a joint property of the fallback and the CALLER's use**, so +the unit to audit is the call site. The live risk for a well-documented shared +helper is gaining a second caller with the opposite polarity, where the +comment above it still reads correctly and nothing at the definition looks +wrong. diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md new file mode 100644 index 0000000000..eb875ddedf --- /dev/null +++ b/.claude/rules/verifying-merged-work.md @@ -0,0 +1,60 @@ +--- +paths: + - "**/*" +--- + +## A squash merge makes every ancestry check unsound + +Merging squashes the branch into one new commit, so the branch head is **never** +an ancestor of `main`. `git branch --merged`, `git log | grep ` and "is +this commit in main" therefore answer confidently and wrongly. Verify by +CONTENT. + +**Which marker you grep for is load-bearing, because this failure has a shape: +the lost commits are always at the TAIL.** It happens when commits land on the +branch after GitHub computed the merge, or when the merge runs from a stale +head — so you lose the end of the branch, never the middle. A marker taken from +an early or middle commit passes cleanly on a PR that dropped its last three. + +1. Take a marker string from the **final** commit on the branch. +2. Confirm what was actually merged: + `gh pr view N --json headRefOid,mergeCommit` +3. Grep `main` for that marker. + +The danger window is push-a-fix-then-merge-immediately, which is what everyone +does once CI is green and threads are cleared. A PR has already merged here +missing its last commit, reading as complete with every thread resolved. + +## Before calling a red run flake, name the mechanism + +A green re-run answers "is this deterministic?". It does NOT answer "is the +cause gone?" — a failure with a conditional trigger passes whenever the +condition happens not to hold, and reads exactly like flake. + +State the mechanism that would make it intermittent, and prefer evidence that +does not depend on a second run: + +- **Legitimate:** a wall-clock ratio assertion on a machine running several + test matrices at once measures load, not code. +- **Legitimate:** the diff never reached the failing subsystem, so it could not + have caused it. **Unreachability is what exonerates a PR, not the re-run** — + and that argument holds whether or not the second run is green. +- **Not sufficient:** "it passed the second time." + +## Environment states wear the costume of code defects + +After a rebase onto a moved `main`, a package you never touched failing to +resolve (`Cannot find module ...`) is a stale install. Run +`pnpm install --frozen-lockfile` in that worktree before diagnosing anything. + +Related, and cheap to get wrong: + +- **Run gates from the worktree ROOT.** From a package directory, + `pnpm check-types --force` becomes a bare `tsc --force` and fails on the flag. +- **`pnpm lint` fails on a WARNING** (`--max-warnings 0`), and the pre-push hook + runs it, so the failure arrives after the commit exists. Check by exit code; + the log says "0 errors, 1 warning" and still fails. +- **Never run a unit suite while an integration leg is in flight.** Unrelated + files time out and read like a broad regression. +- **Never work a PR branch in the shared checkout.** Use `git worktree add`; + another session switching branches underneath you removes files mid-command. diff --git a/AGENTS.md b/AGENTS.md index 881b9c4652..59e2b9aa93 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -66,6 +66,18 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. - Some unit suites have a known pre-existing failing baseline. NEVER add to it: run the tests for the area you touch before and after your change, and fix any new failure you introduce. +- A test is only evidence once you have seen it FAIL for the intended reason. + Break the code, confirm the intended test fails, restore. A break that stops + compilation proves nothing, and the test count must not drop. After changing + a test, re-run its break: a fix to the test is a change to the experiment. +- Ask what ELSE would make a test pass. If anything other than the property + under test produces the same green, it is not covering that property yet — + a fixture that never reaches the mechanism, an unregistered type that falls + through to a default, an assertion satisfied by absence. Add the positive + control that makes the mechanism's presence observable. +- A test that passes both with and without the fix is worse than no test: + the next reader takes the green as coverage. Delete it, and say in the file + that remains where the behaviour IS covered. ## Conventions (enforced; violations will be rejected in review) @@ -99,6 +111,17 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. - Admin styling is token-driven: use `--nx-*` custom properties (defined for light AND dark in `packages/ui/src/styles/theme.css`). Zero hardcoded colors, and every visual change must work in both modes. +- One question has ONE implementation. When a narrower view of something is + needed, DERIVE it from the richer one; never compute it alongside. Two + functions that agree today drift, and the drift is silent because both look + correct. This has produced defects in five unrelated packages. +- A guard that cannot fire is still cheap; a guard added later is not. + Unreachability is a property of the current call graph, not of the code, and + the call graph changes underneath you. "This cannot happen" is a reason the + guard costs nothing, never a reason to omit it. +- A documented rule with nothing enforcing it is not a control, and filing a + task is not installing one. If the correct path and the easy path differ, + the rule will be broken by someone who knows it. ## Changesets and releases From 9121dac5aea60e7563f680085adddc564b576d3a Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 13:00:59 +0500 Subject: [PATCH 02/21] docs(root): qualify four rules that were stated more broadly than they hold The tail-marker check assumed the final commit ADDS something. When it only removes content, grepping for added text finds nothing whether or not the commit landed, and a mode or binary change is invisible to text search at all. Each case now has its own direction. The LIMIT 1 rule was stated as a property of the clause, which made it read as self-contradicting: hunting a witness is sound when the claim is existential. It is now stated as a property of the claim -- one row settles it, or it does not. The lint warning claim named the root script; the hook runs turbo over packages/* only, and admin-css lints without --max-warnings 0, so a warning there exits clean. And an unreachable guard is only free when it does no work. One that queries or recomputes pays on every call whether or not it can reject. Also widens the derived-check paths to the release inputs the rule uses as its own example, and adds the stronger form: a boundary the system cannot cross beats a scan looking for crossings. --- .claude/rules/derived-checks.md | 22 ++++++++++++++++++---- .claude/rules/verifying-merged-work.md | 26 +++++++++++++++++++------- AGENTS.md | 16 ++++++++++++---- 3 files changed, 49 insertions(+), 15 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 232d5486b2..cd6bb6e151 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -2,6 +2,10 @@ paths: - "packages/**/*.ts" - "packages/**/*.tsx" + # The changeset package list versus the release group is one of this rule's own + # examples, so it has to load when those inputs are edited too. + - ".changeset/**" + - "scripts/**" --- When one piece of code checks, mirrors or summarises what another produces: @@ -31,10 +35,20 @@ Asking "does it compute the same thing" is not enough: 1. **Computation** — the same expression. 2. **Domain** — the same rows, records or inputs. A probe using the identical - expression over a different row set is still a divergence. **An existential - search may short-circuit; a universal claim may not.** `LIMIT 1` is sound - when hunting a counterexample and unsound when hunting a witness, and the - two look identical in the source. + expression over a different row set is still a divergence. + + **`LIMIT 1` is sound exactly when ONE row settles the claim.** Which rows + those are depends on what is being claimed, not on the clause: + - claiming _something exists_ → one match settles it. `LIMIT 1` on the match + is sound. + - claiming _everything satisfies P_ → one match of P settles nothing, but one + match of NOT-P refutes it. So search for the counterexample and `LIMIT 1` + is sound; search for a witness of P and it is not. + + The two queries look nearly identical in the source, which is why the claim + has to be written down next to them. A universal check phrased as a witness + hunt passes on the first agreeable row and never reads the rest. + 3. **Failure semantics** — "the answer is no" and "I could not ask" are different outcomes. A check that reports a lock timeout as a data verdict blocks valid work while naming the wrong cause. Pin the error you mean, and diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index eb875ddedf..37798f1688 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -16,10 +16,17 @@ branch after GitHub computed the merge, or when the merge runs from a stale head — so you lose the end of the branch, never the middle. A marker taken from an early or middle commit passes cleanly on a PR that dropped its last three. -1. Take a marker string from the **final** commit on the branch. -2. Confirm what was actually merged: - `gh pr view N --json headRefOid,mergeCommit` -3. Grep `main` for that marker. +1. Confirm what was actually merged: `gh pr view N --json headRefOid,mergeCommit`. +2. Take the check from the **final** commit, in whichever direction it changed things: + - it ADDED content → grep `main` for a string it added; expect a hit. + - it only REMOVED content → grep `main` for a string it removed; expect NO hit. + Grepping for added text here finds nothing whether or not the commit landed, + which reads as failure either way and proves nothing. + - it changed a file mode, a binary, or a rename → text search cannot see it. + Compare the tree instead: `git show --stat -- `, or + `git diff .. -- ` against the same path on `main`. +3. If the final commit is a pure revert of an earlier one in the same PR, check the + NET effect, not the last hunk. The danger window is push-a-fix-then-merge-immediately, which is what everyone does once CI is green and threads are cleared. A PR has already merged here @@ -51,9 +58,14 @@ Related, and cheap to get wrong: - **Run gates from the worktree ROOT.** From a package directory, `pnpm check-types --force` becomes a bare `tsc --force` and fails on the flag. -- **`pnpm lint` fails on a WARNING** (`--max-warnings 0`), and the pre-push hook - runs it, so the failure arrives after the commit exists. Check by exit code; - the log says "0 errors, 1 warning" and still fails. +- **Most packages fail lint on a WARNING**, because their script is + `eslint . --max-warnings 0`. Check by exit code, not by grepping output for + "error": the log reads "0 errors, 1 warning" and still exits 1. + Two qualifications worth knowing before you trust a green: + - The pre-push hook runs `pnpm turbo lint --continue --filter='./packages/*'`, + not the root `pnpm lint`. It does not cover `apps/*` or `e2e/`. + - Not every package opts in. `packages/admin-css` runs a bare `eslint .`, so a + warning there exits 0 and the hook stays green. - **Never run a unit suite while an integration leg is in flight.** Unrelated files time out and read like a broad regression. - **Never work a PR branch in the shared checkout.** Use `git worktree add`; diff --git a/AGENTS.md b/AGENTS.md index 59e2b9aa93..aa1068b3f8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -115,10 +115,18 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. needed, DERIVE it from the richer one; never compute it alongside. Two functions that agree today drift, and the drift is silent because both look correct. This has produced defects in five unrelated packages. -- A guard that cannot fire is still cheap; a guard added later is not. - Unreachability is a property of the current call graph, not of the code, and - the call graph changes underneath you. "This cannot happen" is a reason the - guard costs nothing, never a reason to omit it. +- Unreachability is a property of the current call graph, not of the code, and + the call graph changes underneath you. "This cannot happen" is not a reason to + omit a guard — it is a reason the guard is CHEAP, provided it is cheap: an + assertion over values already in hand costs nothing when its rejection branch + never runs. A guard that queries, reads or recomputes still pays that cost on + every call whether or not it can ever reject, so put those behind the work + they protect rather than in front of a hot path. +- Prefer a boundary the system cannot cross to a check that looks for crossings. + A scan over syntax has an unbounded surface and can only ever be patched; a + declared dependency graph, a type, or a manifest assertion is complete by + construction. If a "must not reach X" rule can be expressed as "X is not a + dependency", that is strictly stronger than any visitor. - A documented rule with nothing enforcing it is not a control, and filing a task is not installing one. If the correct path and the easy path differ, the rule will be broken by someone who knows it. From 9f492d5882ad46a4d6ff0b7919513c115fe2e701 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 13:37:13 +0500 Subject: [PATCH 03/21] docs(root): scope three rules to the cases where they actually hold The merge-verification marker needed a uniqueness and path scope to prove anything, tree comparison replaces a diffstat that only reports a path was touched, and the module-resolution remedy now separates a missing build from a stale install. The derived-checks rule loads for package JavaScript, where admin-css keeps its product code and tests, and no longer calls every conservative catch sound regardless of what it swallows. Compile-time contract tests are exempted from the runtime break rule, and deliberate removal of an ineffective test from the count rule. --- .claude/rules/derived-checks.md | 18 ++++++++++-- .claude/rules/verifying-merged-work.md | 38 +++++++++++++++++++------- AGENTS.md | 20 +++++++++++--- 3 files changed, 60 insertions(+), 16 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index cd6bb6e151..03ff114c69 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -2,6 +2,12 @@ paths: - "packages/**/*.ts" - "packages/**/*.tsx" + # admin-css ships its product code and its tests as .mjs, and several package + # build scripts are .js/.cjs. They implement checks like any other package, so + # a TypeScript-only filter would exempt exactly the code this rule is about. + - "packages/**/*.mjs" + - "packages/**/*.js" + - "packages/**/*.cjs" # The changeset package list versus the release group is one of this rule's own # examples, so it has to load when those inputs are edited too. - ".changeset/**" @@ -61,8 +67,16 @@ implying one policy fits all. ## A bare `catch` is only a defect when its fallback makes a CLAIM -`catch { return conservative }` that degrades to caution is sound, and this -repo has several that are deliberately so. `catch { return verdict }` that +`catch { return conservative }` that degrades to caution is sound for the +failures it was written for, and this repo has several that are deliberately +so. It stops being sound when the same `catch` also swallows a failure that is +not about the data at all — a bad credential, a missing config, a dropped +connection, a `TypeError` in the handler. Those come back as "be cautious", +which blocks valid work while naming no cause, and the wider the catch the +longer that takes to find. Catch the errors you mean, and let an unexpected one +be seen: rethrow it, or log it with its code before degrading. + +`catch { return verdict }` that asserts something about the user's data manufactures a confident wrong diagnosis — and it poisons every test asserting the negative outcome, because those tests go green on the strength of _some_ error rather than the right one. diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 37798f1688..3f7e595344 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -17,15 +17,23 @@ head — so you lose the end of the branch, never the middle. A marker taken fro an early or middle commit passes cleanly on a PR that dropped its last three. 1. Confirm what was actually merged: `gh pr view N --json headRefOid,mergeCommit`. -2. Take the check from the **final** commit, in whichever direction it changed things: - - it ADDED content → grep `main` for a string it added; expect a hit. - - it only REMOVED content → grep `main` for a string it removed; expect NO hit. - Grepping for added text here finds nothing whether or not the commit landed, - which reads as failure either way and proves nothing. +2. Take the check from the **final** commit, in whichever direction it changed + things. A marker only proves anything if it is UNIQUE to that commit and the + search is SCOPED to the path it changed — a string that also occurs elsewhere + answers the same way whether or not the commit landed: + - it ADDED content → `git grep origin/main -- `; expect a hit. + - it only REMOVED content → same command; expect NO hit. Grepping for ADDED + text here finds nothing whether or not the commit landed, which reads as + failure either way and proves nothing. - it changed a file mode, a binary, or a rename → text search cannot see it. - Compare the tree instead: `git show --stat -- `, or - `git diff .. -- ` against the same path on `main`. -3. If the final commit is a pure revert of an earlier one in the same PR, check the +3. Strongest, and the only option when the change is a mode/binary/rename or has + no marker unique to it: compare the OBJECT. `git ls-tree -- ` + against `git ls-tree -- ` matches mode, type and blob id, so + identical output IS byte-identical content. Prefer this to `--stat`, which + reports only that a path was touched: when an earlier commit in the same PR + also touched that path, it prints a line that looks like success while the + final update is exactly what went missing. +4. If the final commit is a pure revert of an earlier one in the same PR, check the NET effect, not the last hunk. The danger window is push-a-fix-then-merge-immediately, which is what everyone @@ -51,8 +59,18 @@ does not depend on a second run: ## Environment states wear the costume of code defects After a rebase onto a moved `main`, a package you never touched failing to -resolve (`Cannot find module ...`) is a stale install. Run -`pnpm install --frozen-lockfile` in that worktree before diagnosing anything. +resolve (`Cannot find module ...`) is an environment state, not a code defect. +Which state it is decides the remedy, and the two look alike: + +- **Missing build output** — the import names a workspace package (`nextly/...`, + `@nextlyhq/...`) and dozens of files fail at once. Its `dist` was never built. + Run integration tests from the ROOT so turbo builds first; `pnpm install` does + not produce `dist` and will leave this exactly as it was. +- **Stale install** — the import names an external dependency, or one package + resolves while its sibling does not, after `pnpm-lock.yaml` moved underneath + you. `pnpm install --frozen-lockfile` in that worktree. + +Check whether the package's `dist` exists before choosing. Related, and cheap to get wrong: diff --git a/AGENTS.md b/AGENTS.md index aa1068b3f8..6fb58079a4 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -67,9 +67,19 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. it: run the tests for the area you touch before and after your change, and fix any new failure you introduce. - A test is only evidence once you have seen it FAIL for the intended reason. - Break the code, confirm the intended test fails, restore. A break that stops - compilation proves nothing, and the test count must not drop. After changing - a test, re-run its break: a fix to the test is a change to the experiment. + Break the code, confirm the intended test fails, restore. After changing a + test, re-run its break: a fix to the test is a change to the experiment. + What counts as the intended failure depends on when the test runs: + - A RUNTIME test that stops COMPILING proves nothing — the assertion never + executed, so the red says only that the break was malformed. + - A COMPILE-TIME contract test is the opposite case: compilation IS the + mechanism. In `*.test-d.ts`, widening a type makes its `@ts-expect-error` + unused and `check-types` fails for exactly the intended reason. Name the + diagnostic you expect, and confirm THAT one appeared. +- The test count must not drop by ACCIDENT. A suite that silently stopped being + discovered reads as a pass, which is what this guards. Removing a test on + purpose is a different act: it is sometimes correct (below), and the PR says + which test went and why. - Ask what ELSE would make a test pass. If anything other than the property under test produces the same green, it is not covering that property yet — a fixture that never reaches the mechanism, an unregistered type that falls @@ -77,7 +87,9 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. control that makes the mechanism's presence observable. - A test that passes both with and without the fix is worse than no test: the next reader takes the green as coverage. Delete it, and say in the file - that remains where the behaviour IS covered. + that remains where the behaviour IS covered. This is the deliberate removal + the count rule above exempts, so state the drop rather than letting it look + like a suite that went missing. ## Conventions (enforced; violations will be rejected in review) From 3dfc66a56f7debc28e52efdde7d9d3ba08a10a4c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 13:52:54 +0500 Subject: [PATCH 04/21] docs(root): require the expected diagnostic, not merely a red build A compile-time contract test is verified by the diagnostic it predicted. A typo or an unrelated type error in the same file stops compilation too, and `@ts-expect-error` suppresses whatever error follows it, so such a test stays green once the code fails for a different reason and after the rejection it asserts stops happening. --- AGENTS.md | 12 ++++++++++-- 1 file changed, 10 insertions(+), 2 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 6fb58079a4..373033c1bf 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -74,8 +74,16 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. executed, so the red says only that the break was malformed. - A COMPILE-TIME contract test is the opposite case: compilation IS the mechanism. In `*.test-d.ts`, widening a type makes its `@ts-expect-error` - unused and `check-types` fails for exactly the intended reason. Name the - diagnostic you expect, and confirm THAT one appeared. + unused and `check-types` fails for exactly the intended reason. Red is not + the evidence though — the EXPECTED DIAGNOSTIC is. A typo, a bad import or + an unrelated type error in the same file all stop compilation too, and + prove nothing about the property. + - `@ts-expect-error` is the sharp edge here, because it suppresses ANY error + on the line that follows. A test asserting "this call is rejected" stays + green once the code starts erroring for a different reason, and stays green + after the original rejection stops happening. Assert the diagnostic where + the tooling allows it; otherwise put the expected error code in a comment + on the directive, so a drift is visible in review rather than silent. - The test count must not drop by ACCIDENT. A suite that silently stopped being discovered reads as a pass, which is what this guards. Removing a test on purpose is a different act: it is sometimes correct (below), and the PR says From 1cc0f53ac1d482e78229c68e090c00274f5e4587 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 14:00:07 +0500 Subject: [PATCH 05/21] docs(root): name the deciding property before measuring A measurement that confirms a true fact about an adjacent property closes the question while leaving the defect in place, and it does so carrying the authority of having been run. --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 373033c1bf..2c9356ef49 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,6 +88,14 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. discovered reads as a pass, which is what this guards. Removing a test on purpose is a different act: it is sometimes correct (below), and the PR says which test went and why. +- Before measuring, name the property that DECIDES the outcome, and check that + it is the one you are about to measure. A measurement that confirms a true + fact about an adjacent property is worse than no measurement, because it + carries the authority of having been run and it closes the question. "Can the + old object be dropped" and "can the code FIND it" both look like the same + question about a database constraint; only the second one decides whether a + repair works, and measuring the first returns green on databases the repair + would silently skip. - Ask what ELSE would make a test pass. If anything other than the property under test produces the same green, it is not covering that property yet — a fixture that never reaches the mechanism, an unregistered type that falls From 79b3ea15adda31b9bfb4bf29b6a664f591447a72 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 14:03:36 +0500 Subject: [PATCH 06/21] docs(root): propagate a cited defect through the rest of the document Invoking a known defect for one purpose while an adjacent claim assumes its absence leaves both on the page with neither wrong on its own. The check is mechanical: re-read every statement about the population the defect affects. --- AGENTS.md | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index 2c9356ef49..07206793ee 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -143,6 +143,14 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. needed, DERIVE it from the richer one; never compute it alongside. Two functions that agree today drift, and the drift is silent because both look correct. This has produced defects in five unrelated packages. +- When you cite a known defect, PROPAGATE it through the same document. A + defect invoked for one purpose — arguing severity, justifying a workaround — + invalidates every other claim that assumes its absence, and the two sit on + the page together with neither looking wrong alone. The check is mechanical + rather than a matter of care: after citing it, re-read every statement about + the population it affects. A known "schema changes may not reach existing + databases" was cited for severity in one paragraph while the next asserted a + property of all existing databases derived from their schema. - Unreachability is a property of the current call graph, not of the code, and the call graph changes underneath you. "This cannot happen" is not a reason to omit a guard — it is a reason the guard is CHEAP, provided it is cheap: an From c47a92f2452f0a7cde7452356e02ccf2e029a781 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 14:12:44 +0500 Subject: [PATCH 07/21] docs(root): prefer the structural property over the name that stands for it A name is a claim made by something outside your control, so the cases where it diverges from expectation are the cases the check exists for. Five instances in one area: SQLSTATE class over a code list, column set over index name, capability probe over version string, the shared indexability rule over a restatement, and object comparison over a grep marker. --- .claude/rules/derived-checks.md | 32 ++++++++++++++++++++++++++++++++ 1 file changed, 32 insertions(+) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 03ff114c69..0e945ad0f3 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -65,6 +65,38 @@ Underneath all three sits time-of-check-to-time-of-use, which may need a different answer per dialect. Say which dialect a mitigation covers rather than implying one policy fits all. +## When a check reaches for a NAME, ask what structurally decides it + +A name, a version string or a string pattern is nearly always a proxy, and the +cases that motivated writing the check are the ones that violate the proxy. + +The reason is worth stating, because it tells you when the rule applies: **a +name is a claim made by someone else; structure is the thing itself.** The +engine chose the collision suffix, the vendor chose what version to report, a +previous release of your own code chose the prefix. You never controlled any of +those strings, and the check exists precisely for the cases where the other +party's choice diverges from your expectation — so the divergence and the check +have the same cause. Not every string in a codebase has that property; the ones +assigned by something outside your control do. + +Five instances in one area, each found only after the name-based version had +been written: + +- classify a driver failure by SQLSTATE **class**, not a list of codes; +- find a database object by the **column set it covers**, not by its name — an + engine appends `_2` on collision and truncates at its identifier limit, so + there is no single string to match; +- decide a capability by **probing it on a scratch object**, not by reading the + server's version — the platforms worth detecting are the ones that misreport; +- decide indexability by asking the **shared rule**, not by restating which + types a dialect can key; +- confirm a merge by **comparing the object**, not by grepping for a marker + that may occur elsewhere. + +The tell is a check whose correctness depends on how some other system chose to +spell something. Ask instead what property makes the answer true, and query +that. It is usually available and it usually costs the same. + ## A bare `catch` is only a defect when its fallback makes a CLAIM `catch { return conservative }` that degrades to caution is sound for the From 166e78fb6b61934ceb4cf912da72812c980a8fd3 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 15:03:57 +0500 Subject: [PATCH 08/21] docs(root): merge two rules into the property-that-separates check Measurement and assertion were the same failure: a necessary-but-insufficient property goes green from both the correct implementation and the broken one, carrying the authority of having been checked. One rule with two worked examples rather than two that each look narrow. --- AGENTS.md | 19 +++++++++++-------- 1 file changed, 11 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 07206793ee..63219aca01 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -88,14 +88,17 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. discovered reads as a pass, which is what this guards. Removing a test on purpose is a different act: it is sometimes correct (below), and the PR says which test went and why. -- Before measuring, name the property that DECIDES the outcome, and check that - it is the one you are about to measure. A measurement that confirms a true - fact about an adjacent property is worse than no measurement, because it - carries the authority of having been run and it closes the question. "Can the - old object be dropped" and "can the code FIND it" both look like the same - question about a database constraint; only the second one decides whether a - repair works, and measuring the first returns green on databases the repair - would silently skip. +- Before you assert or measure, name the property that SEPARATES a correct + implementation from the plausible broken one you are worried about, and check + that it is the property you are about to test. A necessary-but-insufficient + property returns green from both, and it does so carrying the authority of + having been checked, which closes the question. Two worked examples, both real: + - measuring whether an old database constraint could be DROPPED, when what + decides the repair is whether the code can FIND it. Dropping succeeded, and + the repair would still have skipped every database silently. + - asserting a generated identifier is `length <= 63`, when a plain truncation + is also 63 characters. The one test guarding the naming passed on the broken + implementation; distinctness was the separating property. - Ask what ELSE would make a test pass. If anything other than the property under test produces the same green, it is not covering that property yet — a fixture that never reaches the mechanism, an unregistered type that falls From d43f30f8fe27e46602d5de65691cf4d11e08131d Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 15:11:32 +0500 Subject: [PATCH 09/21] docs(root): fold two rules together and move one to the scoped tier "Ask what else would make it pass" is the operational form of naming the separating property, not a second rule, and the count-drop caveat is a rider on test-is-evidence rather than a peer of it. Propagating a cited defect through a document is situational, so it moves to the scoped rules where its worked example has room. AGENTS.md is loaded on every task and attention there is rivalrous. --- .claude/rules/derived-checks.md | 18 ++++++++++++++++++ AGENTS.md | 29 ++++++++++++----------------- 2 files changed, 30 insertions(+), 17 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 0e945ad0f3..1b29028d88 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -65,6 +65,24 @@ Underneath all three sits time-of-check-to-time-of-use, which may need a different answer per dialect. Say which dialect a mitigation covers rather than implying one policy fits all. +## Citing a known defect invalidates the claims that assume its absence + +Two implementations drifting apart is the code version of this. The prose +version is a document that invokes a known defect for one purpose — arguing +severity, justifying a workaround — while another paragraph asserts something +that is only true if the defect does not exist. Both sit on the page together +and neither looks wrong alone, which is why re-reading does not catch it. + +The check is mechanical rather than a matter of care: after citing a defect, +re-read every statement about the POPULATION it affects. + +Worked example, from a schema task in this repo. A standing "core schema +changes may not reach existing databases" was cited for severity in one +paragraph. The next asserted that no existing database could hold duplicate +rows, derived from the constraint its schema declares — which is precisely the +guarantee the cited defect removes. The safety analysis was built on the +absence of the defect being argued from. + ## When a check reaches for a NAME, ask what structurally decides it A name, a version string or a string pattern is nearly always a proxy, and the diff --git a/AGENTS.md b/AGENTS.md index 63219aca01..a73117d6a0 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -84,10 +84,10 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. after the original rejection stops happening. Assert the diagnostic where the tooling allows it; otherwise put the expected error code in a comment on the directive, so a drift is visible in review rather than silent. -- The test count must not drop by ACCIDENT. A suite that silently stopped being - discovered reads as a pass, which is what this guards. Removing a test on - purpose is a different act: it is sometimes correct (below), and the PR says - which test went and why. + - The count must not drop by ACCIDENT: a suite that silently stopped being + discovered reads as a pass, which is what that guards. Removing a test on + purpose is a different act, sometimes correct (below), and the PR says + which test went and why. - Before you assert or measure, name the property that SEPARATES a correct implementation from the plausible broken one you are worried about, and check that it is the property you are about to test. A necessary-but-insufficient @@ -99,11 +99,14 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. - asserting a generated identifier is `length <= 63`, when a plain truncation is also 63 characters. The one test guarding the naming passed on the broken implementation; distinctness was the separating property. -- Ask what ELSE would make a test pass. If anything other than the property - under test produces the same green, it is not covering that property yet — - a fixture that never reaches the mechanism, an unregistered type that falls - through to a default, an assertion satisfied by absence. Add the positive - control that makes the mechanism's presence observable. + + The operational form is to ask what ELSE would produce the same green. If + anything other than the property under test does — a fixture that never + reaches the mechanism, an unregistered type falling through to a default, an + assertion satisfied by absence, a search whose glob missed the directory — + the property is not covered yet. Add the positive control that makes the + mechanism's presence observable, and run it. + - A test that passes both with and without the fix is worse than no test: the next reader takes the green as coverage. Delete it, and say in the file that remains where the behaviour IS covered. This is the deliberate removal @@ -146,14 +149,6 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. needed, DERIVE it from the richer one; never compute it alongside. Two functions that agree today drift, and the drift is silent because both look correct. This has produced defects in five unrelated packages. -- When you cite a known defect, PROPAGATE it through the same document. A - defect invoked for one purpose — arguing severity, justifying a workaround — - invalidates every other claim that assumes its absence, and the two sit on - the page together with neither looking wrong alone. The check is mechanical - rather than a matter of care: after citing it, re-read every statement about - the population it affects. A known "schema changes may not reach existing - databases" was cited for severity in one paragraph while the next asserted a - property of all existing databases derived from their schema. - Unreachability is a property of the current call graph, not of the code, and the call graph changes underneath you. "This cannot happen" is not a reason to omit a guard — it is a reason the guard is CHEAP, provided it is cheap: an From 530e32aff45a97a7de2b7cb430650f5feecf8294 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 15:28:51 +0500 Subject: [PATCH 10/21] docs(root): audit the instrument you are auditing with A probe, a derived check, a test, a post-apply verifier and the suite baseline diff all carried the same defect in one week, and each existed to catch the layer above it. Confirming an instrument on a case where nothing moved cannot distinguish it from one that never reports anything. --- AGENTS.md | 10 ++++++++++ 1 file changed, 10 insertions(+) diff --git a/AGENTS.md b/AGENTS.md index a73117d6a0..f528bd8d7d 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -107,6 +107,16 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. the property is not covered yet. Add the positive control that makes the mechanism's presence observable, and run it. +- Whatever you are currently judging WITH is not being judged. A probe, a + derived check, a test, a post-apply verifier and the baseline diff that reads + the suite all had the same defect in one week here, and every one of them + existed to catch the layer above it. They were hard to see not because the + defect was subtle but because each occupied the position auditing is done + from, so nothing stood further out to look at it. Periodically step out one + level and give the instrument the same treatment as its subject: a positive + control on an input where you know the answer, and where the answer is not + "nothing". Confirming an instrument against a case that did not move cannot + distinguish it from one that reports nothing under any circumstances. - A test that passes both with and without the fix is worse than no test: the next reader takes the green as coverage. Delete it, and say in the file that remains where the behaviour IS covered. This is the deliberate removal From 5daa24f3df61ba27e4b90af8b83bfd540933737f Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 15:43:12 +0500 Subject: [PATCH 11/21] docs(root): show a scoped instinct failing outside its category The compartmentalisation failure is easier to see with no document involved: a disposable copy used deliberately for one control, then an in-place mutation with a hand-rolled backup twenty minutes later, with the boundary existing only as a category rather than in the work. --- .claude/rules/derived-checks.md | 14 ++++++++++++++ 1 file changed, 14 insertions(+) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 1b29028d88..05f8d78140 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -83,6 +83,20 @@ rows, derived from the constraint its schema declares — which is precisely the guarantee the cited defect removes. The safety analysis was built on the absence of the defect being argued from. +The same shape appears without any document involved, which is worth knowing +because it is the harder one to see. A control that needed to modify a test file +was run in a DISPOSABLE worktree specifically so there would be nothing to +restore. Twenty minutes later a control on a tooling script modified it in place +with a hand-rolled backup, the backup was taken after a previous run had already +contaminated the file, and "restoring" reinstated the contamination. + +The instinct was not missing. It was SCOPED — available under "test code", not +transposed to "tooling" — and the boundary was a category in the author's head +rather than anything present in the work. Both halves were twenty minutes apart, +both were the same person's, and neither looked wrong at the time. When you +solve something structurally, ask what else you are doing right now that the +same structure would fix. + ## When a check reaches for a NAME, ask what structurally decides it A name, a version string or a string pattern is nearly always a proxy, and the From ac6216a059d3b672f33d860342842857c756ef3c Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 17:03:18 +0500 Subject: [PATCH 12/21] docs(root): narrow six rules that claimed more than they hold Merge verification compares the PR's delta rather than whole objects, since a base change to the same file makes blobs differ legitimately; probes the merge commit rather than origin/main; matches markers as fixed strings; and treats a rewritten branch as a case the tail heuristic does not cover. A cited resolution failure is no longer assumed environmental without checking what main changed, and stale dist is separated from missing dist. Structural identification is qualified: SQLSTATE class is too coarse when the caller must tell integrity failures apart, and a column set does not identify a database object when several cover the same columns. A comment naming an expected TypeScript diagnostic does not make tsc validate it. An ineffective test is repaired before it is deleted when it is the only coverage. A guard that is a precondition never moves behind the work it protects. A manifest assertion is only a boundary where the resolver agrees with it. Scoped rules now load by extension across the repo rather than by directory. --- .claude/rules/derived-checks.md | 56 +++++++++++++++++------ .claude/rules/verifying-merged-work.md | 61 +++++++++++++++++++------- AGENTS.md | 39 +++++++++++----- 3 files changed, 114 insertions(+), 42 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 05f8d78140..dc247c1cbe 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -1,17 +1,21 @@ --- +# Derived checks are not a packages/ phenomenon. Enumerating directories is how +# this rule kept missing the code it is about — admin-css ships product code and +# tests as .mjs, playground scripts assert agreement between a declaration and an +# import, e2e specs recompute coordinates the app already derives, and the prose +# section below applies squarely to docs and READMEs. Match by extension across +# the repo rather than by location. paths: - - "packages/**/*.ts" - - "packages/**/*.tsx" - # admin-css ships its product code and its tests as .mjs, and several package - # build scripts are .js/.cjs. They implement checks like any other package, so - # a TypeScript-only filter would exempt exactly the code this rule is about. - - "packages/**/*.mjs" - - "packages/**/*.js" - - "packages/**/*.cjs" + - "**/*.ts" + - "**/*.tsx" + - "**/*.mjs" + - "**/*.js" + - "**/*.cjs" + - "**/*.md" + - "**/*.mdx" # The changeset package list versus the release group is one of this rule's own - # examples, so it has to load when those inputs are edited too. + # examples, so it loads when those inputs are edited too. - ".changeset/**" - - "scripts/**" --- When one piece of code checks, mirrors or summarises what another produces: @@ -114,21 +118,45 @@ assigned by something outside your control do. Five instances in one area, each found only after the name-based version had been written: -- classify a driver failure by SQLSTATE **class**, not a list of codes; -- find a database object by the **column set it covers**, not by its name — an - engine appends `_2` on collision and truncates at its identifier limit, so +- classify a driver failure by SQLSTATE at the specificity YOUR claim needs, not + by a hand-kept list of codes; +- identify a database object by its **structural signature**, not by its name — + an engine appends `_2` on collision and truncates at its identifier limit, so there is no single string to match; - decide a capability by **probing it on a scratch object**, not by reading the server's version — the platforms worth detecting are the ones that misreport; - decide indexability by asking the **shared rule**, not by restating which types a dialect can key; -- confirm a merge by **comparing the object**, not by grepping for a marker +- confirm a merge by **comparing the PR's delta**, not by grepping for a marker that may occur elsewhere. The tell is a check whose correctness depends on how some other system chose to spell something. Ask instead what property makes the answer true, and query that. It is usually available and it usually costs the same. +**"Structural" is not automatically "coarse", and the first two above are where +that bites.** Replacing a name with a broader property is only correct when the +broader property still separates the cases you must tell apart: + +- SQLSTATE **class** `23` is right for "is this an integrity failure at all". It + is wrong the moment the caller must distinguish one from another — this repo's + `packages/nextly/src/database/errors.ts` maps `23505`, `23503` and `23502` to + unique, foreign-key and not-null respectively, and collapsing them to the class + would report a missing NOT NULL as a duplicate. Match at the specificity the + claim requires: class when the claim is about the family, code when it is about + the member. +- A **column set** is right for "is any object covering these columns present". + It is wrong for "which object implements this guarantee", because one table can + carry several objects over the same columns — and this repo already treats + `{ columns: ["code"], unique: false }` and `{ columns: ["code"], unique: true }` + as different indexes during an index-to-unique transition. Match the full + signature: columns AND uniqueness AND whether a constraint owns it. + +So the rule is not "prefer the broadest structural property". It is: identify by +structure rather than by someone else's spelling, at the granularity your claim +actually needs — which is the separating-property test applied to the identifier +itself. + ## A bare `catch` is only a defect when its fallback makes a CLAIM `catch { return conservative }` that degrades to caution is sound for the diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 3f7e595344..4dc4e53192 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -10,29 +10,46 @@ an ancestor of `main`. `git branch --merged`, `git log | grep ` and "is this commit in main" therefore answer confidently and wrongly. Verify by CONTENT. -**Which marker you grep for is load-bearing, because this failure has a shape: -the lost commits are always at the TAIL.** It happens when commits land on the -branch after GitHub computed the merge, or when the merge runs from a stale -head — so you lose the end of the branch, never the middle. A marker taken from -an early or middle commit passes cleanly on a PR that dropped its last three. +**Which marker you grep for is load-bearing.** The commonest shape is a lost +TAIL: commits land on the branch after GitHub computed the merge, or the merge +runs from a stale head, so the END of the branch goes missing and a marker taken +from an early commit passes cleanly on a PR that dropped its last three. + +That heuristic says where to look FIRST; it is not what makes a check +sufficient. A branch that was rebased, amended or force-pushed after the merge +was computed diverges differently — rewriting an EARLIER commit while leaving +the final patch text unchanged means the stale merge still contains your +final-commit marker, and the check passes over content that was rewritten +underneath it. When history was rewritten, compare the delta (step 3) rather +than trusting any single marker. 1. Confirm what was actually merged: `gh pr view N --json headRefOid,mergeCommit`. + Probe the recorded **merge commit**, never `origin/main`. Run before a fetch + and `origin/main` is still the pre-merge ref, so every check reports loss + falsely; run after `main` advances and a later commit can make omitted + content look present. 2. Take the check from the **final** commit, in whichever direction it changed things. A marker only proves anything if it is UNIQUE to that commit and the search is SCOPED to the path it changed — a string that also occurs elsewhere - answers the same way whether or not the commit landed: - - it ADDED content → `git grep origin/main -- `; expect a hit. + answers the same way whether or not the commit landed. Match it as a FIXED + string: a marker containing `.`, `[` or `*` is otherwise a pattern, and can + match text it was never taken from. + - it ADDED content → `git grep -F -- `; expect a hit. - it only REMOVED content → same command; expect NO hit. Grepping for ADDED text here finds nothing whether or not the commit landed, which reads as failure either way and proves nothing. - it changed a file mode, a binary, or a rename → text search cannot see it. -3. Strongest, and the only option when the change is a mode/binary/rename or has - no marker unique to it: compare the OBJECT. `git ls-tree -- ` - against `git ls-tree -- ` matches mode, type and blob id, so - identical output IS byte-identical content. Prefer this to `--stat`, which - reports only that a path was touched: when an earlier commit in the same PR - also touched that path, it prints a line that looks like success while the - final update is exactly what went missing. +3. When nothing is unique to the commit, or the change is a mode/binary/rename, + compare the PR's **delta** — not the whole object. Diffing the merged path + entry against the branch-head entry (`git ls-tree`, blob ids) is wrong as soon + as `main` changed ANOTHER hunk of the same file after the branch point: a + correct squash contains both changes, the blobs legitimately differ, and the + check reports a loss that did not happen. Compare what the PR itself changed: + `git diff .. -- ` against + `git diff .. -- `, expecting the PR's hunks in + the second. Whole-object equality is sound only when `main` never touched the + path. `--stat` is never sound: it reports only that a path was touched, which + any earlier commit in the same PR already guarantees. 4. If the final commit is a pure revert of an earlier one in the same PR, check the NET effect, not the last hunk. @@ -59,18 +76,28 @@ does not depend on a second run: ## Environment states wear the costume of code defects After a rebase onto a moved `main`, a package you never touched failing to -resolve (`Cannot find module ...`) is an environment state, not a code defect. -Which state it is decides the remedy, and the two look alike: +resolve (`Cannot find module ...`) is USUALLY an environment state. Three +candidates, and the remedies differ: - **Missing build output** — the import names a workspace package (`nextly/...`, `@nextlyhq/...`) and dozens of files fail at once. Its `dist` was never built. Run integration tests from the ROOT so turbo builds first; `pnpm install` does not produce `dist` and will leave this exactly as it was. +- **Stale build output** — `dist` EXISTS but predates a source or export-map + change the rebase brought in, so it lacks the subpath now being imported. An + existence check on the directory says "built" and is wrong. Rebuild from the + root rather than trusting that `dist` is there. - **Stale install** — the import names an external dependency, or one package resolves while its sibling does not, after `pnpm-lock.yaml` moved underneath you. `pnpm install --frozen-lockfile` in that worktree. -Check whether the package's `dist` exists before choosing. +**Do not label it environmental without looking at what `main` changed.** If the +moved `main` altered a workspace export map, a package manifest, a tsconfig path +mapping or a shared build config, an untouched package failing to resolve is a +real regression wearing the same costume. `git diff ..origin/main -- +'**/package.json' '**/tsconfig*.json' 'turbo.jsonc'` before reaching for a +rebuild: a rebuild that "fixes" it silently absorbs a breaking change into your +branch, and a rebuild that does not fix it has told you something. Related, and cheap to get wrong: diff --git a/AGENTS.md b/AGENTS.md index f528bd8d7d..e727a00ba9 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,9 +81,13 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. - `@ts-expect-error` is the sharp edge here, because it suppresses ANY error on the line that follows. A test asserting "this call is rejected" stays green once the code starts erroring for a different reason, and stays green - after the original rejection stops happening. Assert the diagnostic where - the tooling allows it; otherwise put the expected error code in a comment - on the directive, so a drift is visible in review rather than silent. + after the original rejection stops happening. A comment naming the expected + code does NOT help: `tsc` never reads it, so the directive is still + satisfied by an unrelated error and the test still passes. Prefer an + assertion the checker actually evaluates — `expectTypeOf(...)`, or a + positive control asserting the ACCEPTED form still compiles alongside the + rejected one, so "everything on this line errors" and "the right thing + errors" stop looking alike. - The count must not drop by ACCIDENT: a suite that silently stopped being discovered reads as a pass, which is what that guards. Removing a test on purpose is a different act, sometimes correct (below), and the PR says @@ -117,10 +121,15 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. control on an input where you know the answer, and where the answer is not "nothing". Confirming an instrument against a case that did not move cannot distinguish it from one that reports nothing under any circumstances. -- A test that passes both with and without the fix is worse than no test: - the next reader takes the green as coverage. Delete it, and say in the file - that remains where the behaviour IS covered. This is the deliberate removal - the count rule above exempts, so state the drop rather than letting it look +- A test that passes both with and without the fix is worse than no test: the + next reader takes the green as coverage. **Repair it first.** Usually the + fixture never reaches the mechanism or the assertion is satisfied by absence, + and both are fixable — deleting is right only when the behaviour is genuinely + covered elsewhere, or the test asserts something the code no longer does. + Deleting the ONLY attempted coverage for a behaviour trades a misleading green + for no signal at all, which is not an improvement. When you do delete, say in + the file that remains where the behaviour IS covered; this is the deliberate + removal the count rule exempts, so state the drop rather than letting it look like a suite that went missing. ## Conventions (enforced; violations will be rejected in review) @@ -164,13 +173,21 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. omit a guard — it is a reason the guard is CHEAP, provided it is cheap: an assertion over values already in hand costs nothing when its rejection branch never runs. A guard that queries, reads or recomputes still pays that cost on - every call whether or not it can ever reject, so put those behind the work - they protect rather than in front of a hot path. + every call whether or not it can ever reject, so a purely DEFENSIVE one can + move behind the work it protects. Never move a guard that is a PRECONDITION — + authorization, ownership, validity, quota. "Behind the work" there means the + mutation has already happened when the request is rejected, which turns a cost + saving into a security hole. Preconditions run first, whatever they cost. - Prefer a boundary the system cannot cross to a check that looks for crossings. A scan over syntax has an unbounded surface and can only ever be patched; a declared dependency graph, a type, or a manifest assertion is complete by - construction. If a "must not reach X" rule can be expressed as "X is not a - dependency", that is strictly stronger than any visitor. + construction. But a manifest assertion is only a boundary if the RESOLVER + agrees with it: under pnpm a root dependency, or one hoisted for another + workspace package, stays importable from a package whose own manifest never + declares it, so "X is absent from this package.json" does not mean "this + package cannot reach X". Make the boundary real before trusting it — a + resolution test that imports the package's entry from an isolated context, or + a build that fails on an undeclared import — and only then drop the visitor. - A documented rule with nothing enforcing it is not a control, and filing a task is not installing one. If the correct path and the easy path differ, the rule will be broken by someone who knows it. From 612bd0c8c5345d39bfe94b97282305bf630958a1 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 17:36:17 +0500 Subject: [PATCH 13/21] docs(root): fetch before probing, and diff against the pre-rebase base gh reports the merge commit but does not fetch it, so probing the SHA exits unable-to-resolve rather than verifying. After a rebase the computed merge base IS origin/main, so the manifest diff is empty regardless of what the rebase brought in, and empty reads as nothing-relevant-changed. Config extensions now load the derived-checks rule, where pnpm-workspace.yaml and its mirrored ALL_PACKAGES list are one of its own examples. --- .claude/rules/derived-checks.md | 11 ++++++-- .claude/rules/verifying-merged-work.md | 38 +++++++++++++++++++++----- 2 files changed, 40 insertions(+), 9 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index dc247c1cbe..cfb65ce303 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -13,8 +13,15 @@ paths: - "**/*.cjs" - "**/*.md" - "**/*.mdx" - # The changeset package list versus the release group is one of this rule's own - # examples, so it loads when those inputs are edited too. + # Configuration is where several of this rule's own examples live: the + # changeset package list versus the release group, and `pnpm-workspace.yaml` + # against the hand-maintained ALL_PACKAGES list that `scripts/lint-report.mjs` + # says must mirror it. Editing only the config is exactly the recomputation + # drift this rule is about. + - "**/*.json" + - "**/*.jsonc" + - "**/*.yaml" + - "**/*.yml" - ".changeset/**" --- diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 4dc4e53192..08104788ca 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -23,11 +23,23 @@ final-commit marker, and the check passes over content that was rewritten underneath it. When history was rewritten, compare the delta (step 3) rather than trusting any single marker. -1. Confirm what was actually merged: `gh pr view N --json headRefOid,mergeCommit`. - Probe the recorded **merge commit**, never `origin/main`. Run before a fetch - and `origin/main` is still the pre-merge ref, so every check reports loss +1. Confirm what was actually merged, then FETCH the object before probing it: + + ``` + gh pr view N --json headRefOid,mergeCommit + git fetch origin # gh reports metadata; it does not fetch + ``` + + `gh pr view` prints PR information and adds nothing to the local object + database, so probing the reported SHA without this exits with + `unable to resolve revision` — which reads as a failed verification rather + than as a missing object. + + Probe that **merge commit**, never `origin/main`. Run before a fetch and + `origin/main` is still the pre-merge ref, so every check reports loss falsely; run after `main` advances and a later commit can make omitted content look present. + 2. Take the check from the **final** commit, in whichever direction it changed things. A marker only proves anything if it is UNIQUE to that commit and the search is SCOPED to the path it changed — a string that also occurs elsewhere @@ -94,10 +106,22 @@ candidates, and the remedies differ: **Do not label it environmental without looking at what `main` changed.** If the moved `main` altered a workspace export map, a package manifest, a tsconfig path mapping or a shared build config, an untouched package failing to resolve is a -real regression wearing the same costume. `git diff ..origin/main -- -'**/package.json' '**/tsconfig*.json' 'turbo.jsonc'` before reaching for a -rebuild: a rebuild that "fixes" it silently absorbs a breaking change into your -branch, and a rebuild that does not fix it has told you something. +real regression wearing the same costume. + +The comparison needs the **pre-rebase** base, and this is the trap: after the +rebase, `git merge-base HEAD origin/main` IS `origin/main` — it is now an +ancestor — so the diff is empty no matter what the rebase brought in. An empty +diff then reads as "main changed nothing relevant", which is the opposite of +what it means. Capture the old base before rebasing, or recover it afterwards: + +``` +OLD=$(git rev-parse HEAD@{1}) # pre-rebase HEAD, from the reflog +git diff $(git merge-base $OLD origin/main)..origin/main -- \ + '**/package.json' '**/tsconfig*.json' 'turbo.jsonc' 'pnpm-workspace.yaml' +``` + +Then decide: a rebuild that "fixes" it silently absorbs a breaking change into +your branch, and a rebuild that does not fix it has told you something. Related, and cheap to get wrong: From 1665ceec19faef762ef05c1a749bf3b1a6973a17 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 17:57:03 +0500 Subject: [PATCH 14/21] docs(root): use ORIG_HEAD, read the delta unfiltered, drop a false mitigation HEAD@{1} after a multi-step rebase is the last pick, not the pre-rebase tip, so the merge base came out as origin/main again and the empty diff returned. The path filter omitted tsup configs and the lockfile, which are the two inputs most likely to explain a stale-output or resolution failure. And a positive control on the accepted form does not distinguish an unrelated error confined to the rejected line, so only an assertion the checker evaluates counts. --- .claude/rules/verifying-merged-work.md | 18 +++++++++++++++--- AGENTS.md | 15 ++++++++------- 2 files changed, 23 insertions(+), 10 deletions(-) diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 08104788ca..054f0b03fe 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -115,11 +115,23 @@ diff then reads as "main changed nothing relevant", which is the opposite of what it means. Capture the old base before rebasing, or recover it afterwards: ``` -OLD=$(git rev-parse HEAD@{1}) # pre-rebase HEAD, from the reflog -git diff $(git merge-base $OLD origin/main)..origin/main -- \ - '**/package.json' '**/tsconfig*.json' 'turbo.jsonc' 'pnpm-workspace.yaml' +OLD=$(git rev-parse ORIG_HEAD) # rebase records the pre-rebase tip here +git diff $(git merge-base $OLD origin/main)..origin/main ``` +`ORIG_HEAD` (equivalently the BRANCH reflog, `@{1}`) is the pre-rebase +tip. **`HEAD@{1}` is not** — after a multi-step rebase that is the last +`rebase (pick)` entry, so the merge base comes out as the new `origin/main` +again and the diff is empty exactly as before, with the fix appearing to be in +place. + +**Read that delta UNFILTERED.** A path list here is the same enumeration trap: +the first version named `package.json`, `tsconfig*.json` and `turbo.jsonc`, and +would have printed nothing for a change to `packages/*/tsup.config.ts` (which +decides what `dist` contains) or to `pnpm-lock.yaml` (which decides what +resolves) — the two inputs most likely to explain the failure being diagnosed. +Scan the whole delta, then narrow. + Then decide: a rebuild that "fixes" it silently absorbs a breaking change into your branch, and a rebuild that does not fix it has told you something. diff --git a/AGENTS.md b/AGENTS.md index e727a00ba9..176388772f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -81,13 +81,14 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. - `@ts-expect-error` is the sharp edge here, because it suppresses ANY error on the line that follows. A test asserting "this call is rejected" stays green once the code starts erroring for a different reason, and stays green - after the original rejection stops happening. A comment naming the expected - code does NOT help: `tsc` never reads it, so the directive is still - satisfied by an unrelated error and the test still passes. Prefer an - assertion the checker actually evaluates — `expectTypeOf(...)`, or a - positive control asserting the ACCEPTED form still compiles alongside the - rejected one, so "everything on this line errors" and "the right thing - errors" stop looking alike. + after the original rejection stops happening. Two things that look like + mitigations and are not: a comment naming the expected code, which `tsc` + never reads; and a positive control asserting the ACCEPTED form still + compiles, which an unrelated error confined to the rejected line leaves + untouched. Only an assertion the checker EVALUATES distinguishes the cases — + `expectTypeOf(...)`, or a diagnostic-aware type test that names the error it + expects. If the property cannot be asserted that way, say in the file that + the directive is unverified rather than letting it read as coverage. - The count must not drop by ACCIDENT: a suite that silently stopped being discovered reads as a pass, which is what that guards. Removing a test on purpose is a different act, sometimes correct (below), and the PR says From 31d3ca6b05a9a28b2b88d27cb47c82fcb355385d Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 18:12:29 +0500 Subject: [PATCH 15/21] docs(root): three-valued SQL, -e for markers, fetch both refs, shell scope NOT P in SQL is not the complement of P: for a NULL input both are UNKNOWN and WHERE keeps only TRUE, so a counterexample hunt over nullable data certifies the universal claim it was meant to refute. A marker beginning with - is parsed as an option without -e. A squash commit does not have the PR head as an ancestor, so both refs need fetching before step 3 dereferences headRefOid. Shell verifiers are derived checks in every sense but the language. --- .claude/rules/derived-checks.md | 14 ++++++++++++++ .claude/rules/verifying-merged-work.md | 14 +++++++++++--- 2 files changed, 25 insertions(+), 3 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index cfb65ce303..9c103be76f 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -22,6 +22,10 @@ paths: - "**/*.jsonc" - "**/*.yaml" - "**/*.yml" + # Shell verifiers count too: `packages/nextly/scripts/phase-gate.sh` parses + # test, lint and type-check counts and compares them against stored baselines, + # which is a derived check in every sense except the language it is written in. + - "**/*.sh" - ".changeset/**" --- @@ -66,6 +70,16 @@ Asking "does it compute the same thing" is not enough: has to be written down next to them. A universal check phrased as a witness hunt passes on the first agreeable row and never reads the rest. + **In SQL, `NOT P` is not the complement of `P`.** The logic is three-valued: + for a NULL input both `P` and `NOT P` evaluate to UNKNOWN, and `WHERE` + keeps only TRUE. So `WHERE NOT (price > 0) LIMIT 1` returns no row for a + table full of NULL prices and certifies "every price is positive" — the + counterexample hunt, done correctly, silently reporting the opposite of the + truth. Decide first whether NULL violates the claim, then write the + predicate that says so: `WHERE (price > 0) IS NOT TRUE` catches NULLs as + violations, `WHERE price IS NOT NULL AND NOT (price > 0)` excludes them + deliberately. Either is fine; the bare `NOT` is the one that is neither. + 3. **Failure semantics** — "the answer is no" and "I could not ask" are different outcomes. A check that reports a lock timeout as a data verdict blocks valid work while naming the wrong cause. Pin the error you mean, and diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 054f0b03fe..e766863e2c 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -27,14 +27,19 @@ than trusting any single marker. ``` gh pr view N --json headRefOid,mergeCommit - git fetch origin # gh reports metadata; it does not fetch + git fetch origin # gh reports; it does not fetch ``` `gh pr view` prints PR information and adds nothing to the local object - database, so probing the reported SHA without this exits with + database, so probing a reported SHA without this exits with `unable to resolve revision` — which reads as a failed verification rather than as a missing object. + Fetch BOTH. A squash commit does not have the PR head as an ancestor, so + fetching only the merge commit leaves `headRefOid` unresolvable — and step 3 + dereferences it. Outside the PR worktree, or after the branch is deleted, + that is where the procedure stops. + Probe that **merge commit**, never `origin/main`. Run before a fetch and `origin/main` is still the pre-merge ref, so every check reports loss falsely; run after `main` advances and a later commit can make omitted @@ -46,7 +51,10 @@ than trusting any single marker. answers the same way whether or not the commit landed. Match it as a FIXED string: a marker containing `.`, `[` or `*` is otherwise a pattern, and can match text it was never taken from. - - it ADDED content → `git grep -F -- `; expect a hit. + - it ADDED content → `git grep -F -e "$marker" -- `; + expect a hit. The `-e` is not optional: a marker beginning with `-`, which + a Markdown list item usually does, is otherwise parsed as an option and + exits 129 without checking anything. - it only REMOVED content → same command; expect NO hit. Grepping for ADDED text here finds nothing whether or not the commit landed, which reads as failure either way and proves nothing. From 53a7a6d9afcef34936dab5530b5c9a90405d9398 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 18:22:36 +0500 Subject: [PATCH 16/21] docs(root): positive-control the removal branch, widen scope to derived artefacts git grep exits 1 for no-match and for a pathspec matching no files alike, so a mistyped path certifies a removal without reading the file. Require a hit against the preimage first. Scope now covers css and sql, where the repo has explicit source/derived pairs. --- .claude/rules/derived-checks.md | 6 ++++++ .claude/rules/verifying-merged-work.md | 11 +++++++++++ 2 files changed, 17 insertions(+) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 9c103be76f..122d278015 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -26,6 +26,12 @@ paths: # test, lint and type-check counts and compares them against stored baselines, # which is a derived check in every sense except the language it is written in. - "**/*.sh" + # And the derived ARTEFACTS, not only the code that derives them: + # `apps/playground/src/plugins/style-fixture/admin.source.css` compiles into a + # derived `admin.css`, and `templates/blog/migrations/*.sql` say outright that + # their structure mirrors `UserExtSchemaService.generateMigrationSQL()`. + - "**/*.css" + - "**/*.sql" - ".changeset/**" --- diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index e766863e2c..2baf5da0ef 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -58,7 +58,18 @@ than trusting any single marker. - it only REMOVED content → same command; expect NO hit. Grepping for ADDED text here finds nothing whether or not the commit landed, which reads as failure either way and proves nothing. + + **An absent marker is only evidence once you have shown the search CAN + find it.** `git grep` exits 1 for "no lines selected" and for "the + pathspec matched no files" alike, so a mistyped or since-renamed `` + certifies the removal without ever reading the file. Run the same command + against the commit's PREIMAGE first — `^` or `^` + — and require a hit there. That is the positive control, and without it + this branch is the one place in the procedure that passes by finding + nothing. + - it changed a file mode, a binary, or a rename → text search cannot see it. + 3. When nothing is unique to the commit, or the change is a mode/binary/rename, compare the PR's **delta** — not the whole object. Diffing the merged path entry against the branch-head entry (`git ls-tree`, blob ids) is wrong as soon From 0d9948ac350e08f13551dcc61ae6af56e83a23a0 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 18:50:54 +0500 Subject: [PATCH 17/21] docs(root): absence cannot witness a removal, and obsolete tests have no successor If text was added and removed within one PR, the pre-merge state never had it, so its absence afterwards is guaranteed whether or not the removal landed. Neither preimage separates that from a merged removal; the delta does. And a test deleted because the behaviour is gone has no remaining file to point at, so requiring one forces a false coverage comment. --- .claude/rules/verifying-merged-work.md | 20 ++++++++++++++++---- AGENTS.md | 18 +++++++++++------- 2 files changed, 27 insertions(+), 11 deletions(-) diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 2baf5da0ef..acda6f82dd 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -63,10 +63,22 @@ than trusting any single marker. find it.** `git grep` exits 1 for "no lines selected" and for "the pathspec matched no files" alike, so a mistyped or since-renamed `` certifies the removal without ever reading the file. Run the same command - against the commit's PREIMAGE first — `^` or `^` - — and require a hit there. That is the positive control, and without it - this branch is the one place in the procedure that passes by finding - nothing. + against `^` and require a hit: that proves the path resolves + and the marker is real. + + **But that control validates the INSTRUMENT, not the outcome, and for a + removal it cannot validate the outcome at all.** If the text was added + earlier in the same PR and removed later, `^` — the state + `main` was in before the merge — never contained it, so its absence + afterwards is guaranteed whether or not the removal landed. Neither + preimage separates "the removal merged" from "the text was never there". + Absence is simply not a witness here. + + So for a removal, use the control to prove the search works, then prove + the outcome with the DELTA in step 3: the removal hunk must appear in + `git diff .. -- `. That is a positive + observation of the change landing rather than an inference from nothing + being found. - it changed a file mode, a binary, or a rename → text search cannot see it. diff --git a/AGENTS.md b/AGENTS.md index 176388772f..4eba3f9733 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -125,13 +125,17 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. - A test that passes both with and without the fix is worse than no test: the next reader takes the green as coverage. **Repair it first.** Usually the fixture never reaches the mechanism or the assertion is satisfied by absence, - and both are fixable — deleting is right only when the behaviour is genuinely - covered elsewhere, or the test asserts something the code no longer does. - Deleting the ONLY attempted coverage for a behaviour trades a misleading green - for no signal at all, which is not an improvement. When you do delete, say in - the file that remains where the behaviour IS covered; this is the deliberate - removal the count rule exempts, so state the drop rather than letting it look - like a suite that went missing. + and both are fixable. Deleting the ONLY attempted coverage for a behaviour + trades a misleading green for no signal at all, which is not an improvement. + Deletion is right in two cases, and they need different notes: + - **redundant** — the behaviour is genuinely covered elsewhere. Say in the + file that remains WHERE, so the next reader can follow it. + - **obsolete** — the code no longer does the thing. There is no remaining + file, and demanding one would force a false coverage comment. Say what + behaviour was removed and in which change instead. + + Either way this is the deliberate removal the count rule exempts, so state the + drop rather than letting it look like a suite that went missing. ## Conventions (enforced; violations will be rejected in review) From 04c2f060838044f1819582a351bef5f7aedae8b1 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 19:08:52 +0500 Subject: [PATCH 18/21] docs(root): diff the squash from its own parent, and make the rules discoverable MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit mergeBase..mergeCommit sweeps in every commit main gained after the branch point; the squash commit's parent IS main at merge time, so that range is the squash patch. Snapshots and env examples are tracked derived artefacts with no source extension of their own. And .claude/rules is a Claude-only discovery location, so the review prompt now enumerates it — a reviewer running elsewhere was never loading the rules it is asked to enforce. --- .claude/rules/derived-checks.md | 6 ++++++ .claude/rules/verifying-merged-work.md | 10 +++++++--- .github/review-prompt.md | 4 ++++ 3 files changed, 17 insertions(+), 3 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index 122d278015..d91f95d162 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -26,6 +26,12 @@ paths: # test, lint and type-check counts and compares them against stored baselines, # which is a derived check in every sense except the language it is written in. - "**/*.sh" + # Tracked derived artefacts with no source extension of their own: committed + # `*.snap` files ARE the derived view, and `apps/playground/.env.example:17` + # names `packages/nextly/src/shared/lib/env.ts` as its source of truth. + - "**/*.snap" + - "**/.env.example" + - "**/*.env.example" # And the derived ARTEFACTS, not only the code that derives them: # `apps/playground/src/plugins/style-fixture/admin.source.css` compiles into a # derived `admin.css`, and `templates/blog/migrations/*.sql` say outright that diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index acda6f82dd..8721ac09a0 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -76,7 +76,7 @@ than trusting any single marker. So for a removal, use the control to prove the search works, then prove the outcome with the DELTA in step 3: the removal hunk must appear in - `git diff .. -- `. That is a positive + `git diff ^.. -- `. That is a positive observation of the change landing rather than an inference from nothing being found. @@ -89,8 +89,12 @@ than trusting any single marker. correct squash contains both changes, the blobs legitimately differ, and the check reports a loss that did not happen. Compare what the PR itself changed: `git diff .. -- ` against - `git diff .. -- `, expecting the PR's hunks in - the second. Whole-object equality is sound only when `main` never touched the + `git diff ^.. -- `, expecting the PR's hunks + in the second. The squash side is diffed from its OWN parent: a squash + commit's parent IS `main` at the moment of the merge, so that range is + exactly the squash patch. Using `` there sweeps in every commit + `main` gained after the branch point, and the PR's hunks disappear among + them. Whole-object equality is sound only when `main` never touched the path. `--stat` is never sound: it reports only that a path was touched, which any earlier commit in the same PR already guarantees. 4. If the final commit is a pure revert of an earlier one in the same PR, check the diff --git a/.github/review-prompt.md b/.github/review-prompt.md index a2b5043175..43649c8c38 100644 --- a/.github/review-prompt.md +++ b/.github/review-prompt.md @@ -34,6 +34,10 @@ Read before forming any opinion (PR-side versions if the PR touches them): - `AGENTS.md` (root): the primary contract. Cite it with line-anchored permalinks in findings. - `ARCHITECTURE.md`: layering rules and the "Key invariants (do not break these)" section. +- `.claude/rules/derived-checks.md` and `.claude/rules/verifying-merged-work.md`: the + detailed guidance behind several AGENTS.md rules, with worked examples. These load + automatically only in Claude clients, which is why they are enumerated here — a reviewer + running anywhere else would otherwise never see them. - `.claude/skills/reviewing-a-pr/SKILL.md` and `.claude/skills/release-and-changesets/SKILL.md`. - `packages/nextly/AGENTS.md` / `packages/admin/AGENTS.md` when the PR touches those packages. - `packages/plugin-sdk/STABILITY.md` / `packages/ui/STABILITY.md` when public surface changes. From 5ef0652f3ea446d0f2eb3d2c8a164434f559f6d6 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 20:44:37 +0500 Subject: [PATCH 19/21] docs(root): docker:test probes the connection, it does not start anything The documented start command only tests the connection and exits 1 on failure, and docker:up brings up the dev stack rather than the test containers, failing on a name conflict where one already exists. Names the containers directly. --- AGENTS.md | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index 4eba3f9733..b1d4c0c4b8 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -55,9 +55,16 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. with self-import errors that look real but are not. - Integration tests self-skip when the dialect's URL is unset. Use the root scripts: `pnpm test:integration:postgres17` (localhost:5435), - `:postgres15` (:5434), `:mysql` (:3307), `:sqlite` (no URL needed). Start - the databases with `pnpm docker:test`. NEVER point a TEST\_\* URL at a - database you did not create for the test run. + `:postgres15` (:5434), `:mysql` (:3307), `:sqlite` (no URL needed). NEVER + point a TEST\_\* URL at a database you did not create for the test run. +- The test databases are their own containers, separate from the dev stack: + `docker start nextly-postgres17-test nextly-mysql-test` (add + `nextly-postgres15-test` for the 15 leg). `pnpm docker:test` only PROBES the + connection and exits 1 when it fails — it starts nothing — and `pnpm +docker:up` brings up the DEV stack, which on a machine that already has one + fails with a container-name conflict. A `DBS DOWN` failure followed by a + start command that changes nothing reads like a broken environment; it is + usually just the wrong command. - Integration files in `packages/nextly` run sequentially on purpose (`fileParallelism: false`, single fork): system-table suites share fixed table names. Do not "fix" slow integration runs by re-enabling parallelism. From 87b3179bd8e6da0f9eb94dc301e33c0f63c20fd8 Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 20:45:55 +0500 Subject: [PATCH 20/21] docs(root): name both start paths for the test databases, and why they differ MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit docker start handles the usual case of existing-but-stopped containers; compose up is needed only on a fresh clone. The services set a fixed container_name, so one compose project owns them and it is whichever directory first brought them up — rarely the worktree you are in, which is why compose up conflicts. --- AGENTS.md | 26 ++++++++++++++++++-------- 1 file changed, 18 insertions(+), 8 deletions(-) diff --git a/AGENTS.md b/AGENTS.md index b1d4c0c4b8..2ce07c507f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -57,14 +57,24 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. scripts: `pnpm test:integration:postgres17` (localhost:5435), `:postgres15` (:5434), `:mysql` (:3307), `:sqlite` (no URL needed). NEVER point a TEST\_\* URL at a database you did not create for the test run. -- The test databases are their own containers, separate from the dev stack: - `docker start nextly-postgres17-test nextly-mysql-test` (add - `nextly-postgres15-test` for the 15 leg). `pnpm docker:test` only PROBES the - connection and exits 1 when it fails — it starts nothing — and `pnpm -docker:up` brings up the DEV stack, which on a machine that already has one - fails with a container-name conflict. A `DBS DOWN` failure followed by a - start command that changes nothing reads like a broken environment; it is - usually just the wrong command. +- The test databases are their own containers in `docker-compose.test.yml`, + separate from the dev stack. Neither `pnpm docker:test` nor `pnpm docker:up` + starts them: `docker:test` only PROBES a connection and exits 1 when it + fails, and `docker:up` brings up the DEV stack, which conflicts by container + name where one already exists. A `DBS DOWN` failure followed by a start + command that changes nothing reads like a broken environment; it is usually + just the wrong command. + - Already created but stopped, which is the usual case: + `docker start nextly-postgres17-test nextly-mysql-test` (add + `nextly-postgres15-test` for the 15 leg). + - Never created, on a fresh clone: + `docker compose -f docker-compose.test.yml up -d postgres17-test mysql-test`. + - Why not always the second: the services set a fixed `container_name`, so + exactly one compose project can own them, and the owner is whichever + directory first brought them up. In a repo worked through many worktrees + that is rarely the one you are standing in, and compose then tries to + CREATE containers whose names are taken and fails. `docker start` addresses + them by name and does not care which project owns them. - Integration files in `packages/nextly` run sequentially on purpose (`fileParallelism: false`, single fork): system-table suites share fixed table names. Do not "fix" slow integration runs by re-enabling parallelism. From e8cd64100d03baf02f22c88410c94f2142a166ce Mon Sep 17 00:00:00 2001 From: Mobeen Abdullah Date: Tue, 11 Aug 2026 21:21:54 +0500 Subject: [PATCH 21/21] docs(root): branch reflog over ORIG_HEAD, and pg15 in the fresh-clone start ORIG_HEAD is rewritten by any later command that sets it, git reset included, so it points at the rebased tip and the diff comes out empty again. The branch reflog moves once per rebase. Also records that the repo's index key sorts columns, so the signature does not separate (a,b) from (b,a) today. --- .claude/rules/derived-checks.md | 12 ++++++++++-- .claude/rules/verifying-merged-work.md | 15 +++++++++++---- AGENTS.md | 4 +++- 3 files changed, 24 insertions(+), 7 deletions(-) diff --git a/.claude/rules/derived-checks.md b/.claude/rules/derived-checks.md index d91f95d162..425fcee84e 100644 --- a/.claude/rules/derived-checks.md +++ b/.claude/rules/derived-checks.md @@ -182,8 +182,16 @@ broader property still separates the cases you must tell apart: It is wrong for "which object implements this guarantee", because one table can carry several objects over the same columns — and this repo already treats `{ columns: ["code"], unique: false }` and `{ columns: ["code"], unique: true }` - as different indexes during an index-to-unique transition. Match the full - signature: columns AND uniqueness AND whether a constraint owns it. + as different indexes during an index-to-unique transition. Match the signature + the CLAIM needs: columns, uniqueness, and whether a constraint owns the object. + + Note what that signature does NOT currently separate. `indexKey` in + `schema/pipeline/diff/index-util.ts` SORTS the columns, so `(a, b)` and + `(b, a)` compare equal — even though only the first serves a left-prefix + lookup on `a`. Today the pipeline emits single-column indexes, so nothing + depends on the distinction; the moment a composite one is emitted, the key + will silently treat two different objects as one. Stated here rather than + fixed, because widening the key changes every comparison that uses it. So the rule is not "prefer the broadest structural property". It is: identify by structure rather than by someone else's spelling, at the granularity your claim diff --git a/.claude/rules/verifying-merged-work.md b/.claude/rules/verifying-merged-work.md index 8721ac09a0..e2ca6a4981 100644 --- a/.claude/rules/verifying-merged-work.md +++ b/.claude/rules/verifying-merged-work.md @@ -150,13 +150,20 @@ diff then reads as "main changed nothing relevant", which is the opposite of what it means. Capture the old base before rebasing, or recover it afterwards: ``` -OLD=$(git rev-parse ORIG_HEAD) # rebase records the pre-rebase tip here +OLD=$(git rev-parse "$(git branch --show-current)@{1}") # pre-rebase tip git diff $(git merge-base $OLD origin/main)..origin/main ``` -`ORIG_HEAD` (equivalently the BRANCH reflog, `@{1}`) is the pre-rebase -tip. **`HEAD@{1}` is not** — after a multi-step rebase that is the last -`rebase (pick)` entry, so the merge base comes out as the new `origin/main` +The BRANCH reflog is the reliable source: a rebase moves the branch ref once, so +`@{1}` is its pre-rebase tip and nothing but another update to that +branch disturbs it. + +Two tempting alternatives are both wrong. **`ORIG_HEAD` is volatile** — it is +rewritten by any later command that sets it, `git reset` included, so a single +`git reset --hard HEAD` after the rebase leaves it pointing at the REBASED tip +and the diff comes out empty again. **`HEAD@{1}` is not the pre-rebase tip +either** — after a multi-step rebase it is the last `rebase (pick)` entry, so the +merge base comes out as the new `origin/main` again and the diff is empty exactly as before, with the fix appearing to be in place. diff --git a/AGENTS.md b/AGENTS.md index 2ce07c507f..16d2ff61e3 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -68,7 +68,9 @@ Before editing a package, read its README.md and check for a nested AGENTS.md. `docker start nextly-postgres17-test nextly-mysql-test` (add `nextly-postgres15-test` for the 15 leg). - Never created, on a fresh clone: - `docker compose -f docker-compose.test.yml up -d postgres17-test mysql-test`. + `docker compose -f docker-compose.test.yml up -d postgres17-test postgres15-test mysql-test`. + `postgres15-test` is the only service on 5434, so omitting it leaves the + documented `:postgres15` leg with nothing to connect to. - Why not always the second: the services set a fixed `container_name`, so exactly one compose project can own them, and the owner is whichever directory first brought them up. In a repo worked through many worktrees