diff --git a/AGENTS.md b/AGENTS.md index 4346492cc..fac33f608 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -10,7 +10,7 @@ All architectural decisions and constraints (including cross-language parity rul ## Reference Docs -Deep operational references live in `agents/reference/` and are NOT auto-loaded (this file must stay under Claude Code's 40k-char instruction limit). Each summarized section below names its reference file -- **read it before working in that area**: CI gate debugging (`testing-conventions-ci.md`), colocation/mutation/coverage gates (`testing-gates.md`), e2e attestation (`e2e-attestation.md`), changelog/migration fragments (`changelog-migrations.md`). +Deep operational references live in `agents/reference/` and are NOT auto-loaded (this file must stay under Claude Code's 40k-char instruction limit). Each summarized section below names its reference file -- **read it before working in that area**: reusable-workflow gate debugging (`testing-conventions-ci.md`), colocation/mutation/coverage gates (`testing-gates.md`), e2e attestation (`e2e-attestation.md`), changelog/migration fragments (`changelog-migrations.md`), merge gating (`pr-monitor.md`). ## Scratch Files @@ -47,11 +47,11 @@ Default to no comments. Only add one when the WHY is non-obvious -- a hidden con **Every CI check emits actionable fix instructions on failure.** A failing check must tell the contributor exactly what to change -- the file, command, or trailer to add or edit -- not merely which rule was violated. When a check can detect a *near-miss* (a fix was attempted but malformed), it names the specific defect and how to correct it rather than falling through to a generic "not satisfied" message (e.g. the `changelog-gate` names a fragment file whose name breaks the `YYYY-MM-DD-.md` convention -- pointing at the exact file -- and its "no fragment" error prints the exact path to add; dirsql#566). -**CI logic lives in scripts, not workflow YAML.** `run:` / `github-script` steps stay trivial glue -- check out, set up a toolchain, invoke one command. Anything with iteration, `case` dispatch, conditionals, or text-munging moves to a check in the `internals/checks` uv package (a click group, one subcommand per check -- see `internals/checks/src/checks/`), invoked as a one-liner (`uv run --project internals/checks dirsql-checks `), and carries **colocated unit tests** (the same testing-conventions standard as the rest of the tree -- `foo.py` ↔ `foo_test.py`). Those tests run under `conventions.yml`'s `internals-checks` job (`unit-coverage` enforces a 100% floor; full gate list in `agents/reference/testing-gates.md`). Inline workflow logic is untestable, un-runnable locally, and silently duplicated across runners; a script is none of those. +**CI logic lives in scripts, not workflow YAML.** `run:` / `github-script` steps stay trivial glue -- check out, set up a toolchain, invoke one command. Anything with iteration, `case` dispatch, conditionals, or text-munging moves to a check in the `internals/checks` uv package (a click group, one subcommand per check -- see `internals/checks/src/checks/`), invoked as a one-liner (`uv run --project internals/checks dirsql-checks `), and carries **colocated unit tests** (the same testing-conventions standard as the rest of the tree -- `foo.py` ↔ `foo_test.py`). Those tests run under `internals-checks-ci.yml`'s `internals-checks` job (`unit-coverage` enforces a 100% floor; full gate list in `agents/reference/testing-gates.md`). Inline workflow logic is untestable, un-runnable locally, and silently duplicated across runners; a script is none of those. ### Reusable-workflow gates (testing-conventions) -`conventions.yml` calls the `testing-conventions` reusable workflow at the **moving tag `@v0`**. The essentials: a removed/renamed input startup-fails the whole workflow on `main` and every PR (0 jobs, no job logs -- WebFetch the run's `html_url` and read the annotation first, never hypothesize); an unknown `testing-conventions.toml` key fails EVERY gate; read upstream behavior at a pinned sha and trust its `MIGRATIONS.md` over probing; a stale e2e attestation after a squash merge is ours to re-attest, never a tool bug; never retire a bespoke gate before a green proof. Full operational lore, the per-language gate adoption map, and the two permanent exceptions (Rust unit-coverage stays bespoke; Rust has no e2e-verify): `agents/reference/testing-conventions-ci.md`. +Six per-domain workflows call the `testing-conventions` reusable workflow at the **moving tag `@v0`** -- `dirsql-python-ci.yml`, `dirsql-typescript-ci.yml`, `dirsql-rust-ci.yml`, `internals-checks-ci.yml`, `internals-distcheck-ci.yml`, `plugin-dirsql-embedding-ci.yml` (#861 split the single `conventions.yml` into these, one per domain, so path filters could triage per lane). The essentials: a removed/renamed input startup-fails the whole *calling* workflow on `main` and every PR (0 jobs, no job logs -- WebFetch the run's `html_url` and read the annotation first, never hypothesize), and since every caller passes the same inputs, one bad input reds all six; an unknown `testing-conventions.toml` key fails EVERY gate; read upstream behavior at a pinned sha and trust its `MIGRATIONS.md` over probing; a stale e2e attestation after a squash merge is ours to re-attest, never a tool bug; never retire a bespoke gate before a green proof. Full operational lore, the per-language gate adoption map, and the two permanent exceptions (Rust unit-coverage stays bespoke; Rust has no e2e-verify): `agents/reference/testing-conventions-ci.md`. ## Imports @@ -115,17 +115,17 @@ A feature is not done until integration tests pass and cover the new functionali - `tests/integration/binding/` (#289) -- the SDK public API against the **real core** (PyO3 / napi binding, real SQLite, real temp-dir filesystem). Proves the SDK↔core marshaling and real query/watch/persist behavior the hermetic subdir mocks out -- coverage the CLI e2e suites cannot provide at the granularity the binding tier needs -- though since #721 the CLI *does* cross a binding (the launchers call `run_cli` in-process), so the e2e suites now exercise that path too. Its CI job builds the native artifact (maturin / napi + cargo). This is the only real-core coverage, so unlike e2e it **runs on every PR** -- upstream's integration definition (first-party code runs for real; mocking the outside world is *permitted*, not required) fits it as-is. Both **run in CI**. Rust has no binding subdir: it *is* the core, so `packages/rust/tests/` remains its integration tier. - **E2E tests**: `tests/e2e/` -- exercise the **CLI** only (the launcher, which since #721 runs `run_cli` in-process through the binding rather than spawning a bundled binary) with **nothing mocked**. That change makes the "nothing mocked" claim *more* faithful, not less: the suites now run the exact path a user runs. **No mocks, no fakes, no monkeypatching. NOT run in CI** -- CI verifies only the per-package *attestation* that they ran (see *E2E Attestation*). -- **Distcheck tests**: **not** an SDK-package tier -- the *functional* publishability flows (build, pack, install, and run the published artifact) live in the `internals/distcheck` package (#520), which itself follows the three-tier layout. **Run in CI** via that package's `dirsql-distcheck python` / `dirsql-distcheck node` entry points (python-test.yml / ts-test.yml `distcheck` jobs). Distinct from the **`packaging` gate** (testing-conventions, run via `conventions.yml` for all three languages), which only asserts no test files *ship* in the built artifact and never installs or runs it. +- **Distcheck tests**: **not** an SDK-package tier -- the *functional* publishability flows (build, pack, install, and run the published artifact) live in the `internals/distcheck` package (#520), which itself follows the three-tier layout. **Run in CI** via that package's `dirsql-distcheck python` / `dirsql-distcheck node` entry points (the `distcheck` jobs in `dirsql-python-ci.yml` / `dirsql-typescript-ci.yml`). Distinct from the **`packaging` gate** (testing-conventions, run from each language's CI workflow), which only asserts no test files *ship* in the built artifact and never installs or runs it. ### Enforcing Colocation (testing-conventions) -The colocation rule is a blocking CI gate ([`testing-conventions`](https://github.com/thekevinscott/testing-conventions), wired in `conventions.yml` + `testing-conventions.toml`): every source file needs a colocated unit test (Rust: inline `#[cfg(test)]`), a PR-only co-change check flags modified sources whose test didn't change, and `unit lint` enforces **isolation** -- a unit test must mock every collaborator (Python `patch(...)`, TS `vi.mock`, Rust trait double or relocate the effectful test), never weaken the test. The gates also cover the two binding crates and the repo-tooling `internals/checks` / `internals/distcheck` uv packages. **The exemption count is zero and stays there**: barrels get a colocated surface test, logic gets extracted, dead shells get deleted -- an exemption is never the escape hatch. Full wiring, per-package gate map, and history: `agents/reference/testing-gates.md`. +The colocation rule is a blocking CI gate ([`testing-conventions`](https://github.com/thekevinscott/testing-conventions), wired in the per-domain CI workflows + `testing-conventions.toml`): every source file needs a colocated unit test (Rust: inline `#[cfg(test)]`), a PR-only co-change check flags modified sources whose test didn't change, and `unit lint` enforces **isolation** -- a unit test must mock every collaborator (Python `patch(...)`, TS `vi.mock`, Rust trait double or relocate the effectful test), never weaken the test. The gates also cover the two binding crates and the repo-tooling `internals/checks` / `internals/distcheck` uv packages. **The exemption count is zero and stays there**: barrels get a colocated surface test, logic gets extracted, dead shells get deleted -- an exemption is never the escape hatch. Full wiring, per-package gate map, and history: `agents/reference/testing-gates.md`. -**Run every gate CI declares before pushing** with `just preflight` (#781), which *derives* the (source, gate) pairs from `conventions.yml` instead of restating them -- 39 pairs across 8 scan roots, versus the 6 across 3 the four hand-written recipes it replaced covered. It also encodes the invocations that differ from the naive one (python suites via the package venv, typescript via `npx`, per-gate option support), each of which used to pass locally while failing in CI. `--dry-run` prints the matrix; `--gate ` narrows it; `packaging` reports SKIP since it needs a built artifact (`just test-packaging`). +**Run every gate CI declares before pushing** with `just preflight` (#781), which *derives* the (source, gate) pairs from the CI workflows instead of restating them -- 40 pairs across 8 scan roots, versus the 6 across 3 the four hand-written recipes it replaced covered. It reads every `.github/workflows/*.yml` and folds together each job that calls the reusable workflow, so a lane added in a new per-domain file is a pair it runs. It also encodes the invocations that differ from the naive one (python suites via the package venv, typescript via `npx`, per-gate option support), each of which used to pass locally while failing in CI. `--dry-run` prints the matrix; `--gate ` and `--conventions ` narrow it (both repeatable); `packaging` reports SKIP since it needs a built artifact (`just test-packaging`). ### Mutation (testing-conventions) -The `unit mutation` gate mutates PR-changed source lines and fails on any mutant no unit test kills (engines: cosmic-ray / Stryker / cargo-mutants, all self-provisioned; PR-only, diff-scoped via `--base`). All three SDKs run it inside `conventions.yml`. Fix a survivor with a **new assertion**, never by weakening a test; only a genuinely equivalent mutant gets a reason-required `mutation` exemption (none today). Local run commands and per-language details: `agents/reference/testing-gates.md`. +The `unit mutation` gate mutates PR-changed source lines and fails on any mutant no unit test kills (engines: cosmic-ray / Stryker / cargo-mutants, all self-provisioned; PR-only, diff-scoped via `--base`). All three SDKs run it inside their own CI workflow. Fix a survivor with a **new assertion**, never by weakening a test; only a genuinely equivalent mutant gets a reason-required `mutation` exemption (none today). Local run commands and per-language details: `agents/reference/testing-gates.md`. ### Test Boundaries -- What to Mock, What Not To @@ -193,7 +193,7 @@ For docs/lint/typo-only PRs, include the section with a single line: `N/A - docs ### E2E Attestation -CI never runs the e2e suites; instead `conventions.yml`'s `e2e-verify` gate checks, per package, a committed attestation (`packages/python/e2e-attestation.json`, `packages/ts/e2e-attestation.json`) recording that the suite ran against the package's current code. **Regenerate the attestation for each package you changed, as the last commit touching that package before you push**: `just e2e-attest-python` / `just e2e-attest-ts` (needs `pip install testing-conventions`). Multi-package PRs: attest each package right after finishing *its* changes -- attesting both at the very end leaves the second one stale. **Any `packages/rust/src` change (including `cli/`) stales BOTH bindings' attestations** (CI-enforced via `[e2e].extra_scope`), so re-attest both after core changes. **A binding-crate change stales only its own package** -- `packages/python/src` (PyO3) the python attestation, `packages/ts/napi` the ts one (#933); that per-package scope is why each SDK package carries its own `testing-conventions.toml`. Full mechanics and debugging: `agents/reference/e2e-attestation.md`. +CI never runs the e2e suites; instead the `e2e-verify` gate in each package's CI workflow checks, per package, a committed attestation (`packages/python/e2e-attestation.json`, `packages/ts/e2e-attestation.json`) recording that the suite ran against the package's current code. **Regenerate the attestation for each package you changed, as the last commit touching that package before you push**: `just e2e-attest-python` / `just e2e-attest-ts` (needs `pip install testing-conventions`). Multi-package PRs: attest each package right after finishing *its* changes -- attesting both at the very end leaves the second one stale. **Any `packages/rust/src` change (including `cli/`) stales BOTH bindings' attestations** (CI-enforced via `[e2e].extra_scope`), so re-attest both after core changes. **A binding-crate change stales only its own package** -- `packages/python/src` (PyO3) the python attestation, `packages/ts/napi` the ts one (#933); that per-package scope is why each SDK package carries its own `testing-conventions.toml`. Full mechanics and debugging: `agents/reference/e2e-attestation.md`. ### Docs as Reference @@ -203,7 +203,7 @@ Product *behavior* stays covered by the normal unit / integration / binding / e2 ### Changelog and Migrations -**Every PR touching non-test source under `packages//` or `plugins//` must add a changelog fragment** at `//changelog.d/YYYY-MM-DD-.md` (UTC merge date; body leads with a bold Keep-a-Changelog category, e.g. `**Fixed**`), one per changed package. Enforced by the `changelog-gate` check in CI. Escape hatch for truly no-observable-change PRs: a `skip-changelog: ` line in any commit message. Fragments are permanent and append-only; the root `CHANGELOG.md` / `MIGRATIONS.md` are **frozen** pointer stubs -- never write into them; stray entries get relocated into the owning package's fragment dir, never folded into the frozen files. +**Every PR touching non-test source under `packages//` or `plugins//` must add a changelog fragment** at `//changelog.d/YYYY-MM-DD-.md` (UTC merge date; body leads with a bold Keep-a-Changelog category, e.g. `**Fixed**`), one per changed package. **A package `README.md` counts as source here** -- it ships in all three published artifacts, so a README edit runs `release-ci` + `changelog-check` and needs a fragment, unlike the root prose files. Enforced by the `changelog-gate` check in CI. Escape hatch for truly no-observable-change PRs: a `skip-changelog: ` line in any commit message. Fragments are permanent and append-only; the root `CHANGELOG.md` / `MIGRATIONS.md` are **frozen** pointer stubs -- never write into them; stray entries get relocated into the owning package's fragment dir, never folded into the frozen files. **A migration fragment** (`//migrations.d/YYYY-MM-DD-.md`) is additionally required when a PR breaks a public API, removes a deprecated symbol, or changes runtime behavior without an API change. It needs all five subsections (Summary / Required changes / Deprecations removed / Behavior changes without code changes / Verification); write `_None._` under any that don't apply. Template at the bottom of the frozen root `MIGRATIONS.md`. Purely additive changes and behavior-preserving fixes need no migration entry. @@ -245,7 +245,7 @@ Run `cargo bench -p dirsql` after significant changes to the Rust codebase. Not ### PR Sizing and Issues - **Every PR is M or smaller.** A change larger than M is broken into a sequence of smaller PRs, each independently reviewable and mergeable. Size by review surface, not raw line count -- a mechanical rename spanning many files can be M, while a subtle core change of far fewer lines may not be. -- **Stack that sequence; do not serialize it.** When an epic's slices depend on each other, open each PR with its `base` set to the branch below it rather than waiting for that branch to merge. Reviewers see each layer's own diff, and the slices land in order without anyone idling. [Stacked pull requests](https://github.blog/changelog/2026-07-30-stacked-pull-requests-are-now-in-public-preview/) are GitHub-native, but the mechanism is just the base branch, so it works from `mcp__github__create_pull_request` (set `base`) without any preview-only tooling -- relevant because `gh` (and therefore the `gh-stack` skill) is unavailable in the hosted sandbox. Three things are already true and need no special handling: **GitHub retargets a child to `main` automatically** when its parent merges; **branch protection and required checks apply per layer**, unchanged; and **diff-scoped gates measure the layer, not its ancestors** -- `conventions.yml` passes `base: ${{ github.event.pull_request.base.sha }}`, so `unit mutation`, changed-lines coverage and `changelog-gate` all compute against the stack parent (verified on #703 while stacked: `BASE: 0241d80…`, the tip of `claude/699-read-pdf`). Only slices with a real dependency belong in one stack; independent slices are ordinary PRs off `main`. +- **Stack that sequence; do not serialize it.** When an epic's slices depend on each other, open each PR with its `base` set to the branch below it rather than waiting for that branch to merge. Reviewers see each layer's own diff, and the slices land in order without anyone idling. [Stacked pull requests](https://github.blog/changelog/2026-07-30-stacked-pull-requests-are-now-in-public-preview/) are GitHub-native, but the mechanism is just the base branch, so it works from `mcp__github__create_pull_request` (set `base`) without any preview-only tooling -- relevant because `gh` (and therefore the `gh-stack` skill) is unavailable in the hosted sandbox. Three things are already true and need no special handling: **GitHub retargets a child to `main` automatically** when its parent merges; **branch protection and required checks apply per layer**, unchanged; and **diff-scoped gates measure the layer, not its ancestors** -- every CI workflow passes `base: ${{ github.event.pull_request.base.sha }}`, so `unit mutation`, changed-lines coverage and `changelog-gate` all compute against the stack parent (verified on #703 while stacked: `BASE: 0241d80…`, the tip of `claude/699-read-pdf`). Only slices with a real dependency belong in one stack; independent slices are ordinary PRs off `main`. - **Never request the maintainer's review on a PR.** Do not add reviewers by any mechanism -- `gh pr create --reviewer`, `gh pr edit --add-reviewer`, the GitHub API, or MCP tools. Each request fires a notification; the workflow is already shepherd-to-green and report status, with the maintainer deciding when to look and merge. Opening the PR is the signal; a review request adds nothing but noise. - **Every PR is accompanied by a GitHub issue it auto-closes.** Put a closing keyword (`Fixes #` / `Closes #`) in the PR body so the issue closes on merge; file the issue first if one does not exist yet. - **Every PR gets a unique branch name -- NEVER reuse a branch name across PRs, even for follow-up work on the same issue/epic.** After a PR merges (or when starting a new PR), branch a fresh, distinctly-named branch; do **not** re-create the just-merged branch name and stack the next change on it. Reuse hides real code divergence: an unrelated PR that merged to `main` in the meantime can textually auto-merge with your branch into a **compile error** (e.g. `main` adds a call to a helper your branch deleted). A unique branch per PR forces an honest three-way merge against current `main`. It also keeps each e2e attestation receipt on its own path (`packages//e2e-attestations/.json`), so a re-used name does not have two PRs writing the same file. (Learned the hard way in epic #601: #602 and #603 both used `claude/tackle-601-kxeykd`.) @@ -262,17 +262,13 @@ Run `cargo bench -p dirsql` after significant changes to the Rust codebase. Not ### PR Monitoring -Merge gating is via **pr-monitor** (`.github/workflows/pr-monitor.yml`, the **`CI Gate`** check — [thekevinscott/pr-monitor](https://github.com/thekevinscott/pr-monitor), `@v1` is a rolling tag tracking its `main`). It is an *aggregator*, not a test: it polls the PR head's **workflow runs** and goes green only when every other run has completed with a passing conclusion (`success` / `skipped` / `neutral` / `stale`); `failure`, `cancelled`, `timed_out`, `action_required`, and `startup_failure` all turn it red. Because it watches workflow *runs* rather than check-runs, path-filtered workflows need no enumeration (one that does not run never appears), a still-running workflow holds the gate pending, and a workflow that startup-fails before emitting any check-run still fails the gate. Branch protection requires only `CI Gate`, so adding/renaming/removing jobs or workflows needs no branch-protection or gate-config coordination — don't flag that concern. +Merge gating is via **pr-monitor** (`.github/workflows/pr-monitor.yml`, the **`CI Gate`** check — [thekevinscott/pr-monitor](https://github.com/thekevinscott/pr-monitor) at the rolling `@v1`). It is an *aggregator*, not a test, and it does not guess at the check set: [willfire](https://github.com/thekevinscott/willfire) evaluates the repo's workflow files against the PR's base branch, changed files and head commit to get the exact set of workflow runs GitHub will create, and the gate is a set comparison against the runs on the head — yellow while a predicted run is missing or unfinished, red when one concludes anything but `success` / `skipped` / `neutral` / `stale`, and red when a run appears that nothing predicted. Branch protection requires only `CI Gate`, so adding/renaming/removing jobs or workflows needs no branch-protection or gate-config coordination — don't flag that concern. -**Why not check-run counting (the Mergify era, #830–#947):** a gate built on check-run counts cannot express "everything expected has finished". `#check-failure = 0` alone is vacuously true before CI dispatches and while it runs — a gate that does not gate (#943 measured it green 2m21s before the last check finished). Adding `#check-pending = 0` deadlocks: Mergify counts its own in-progress check, so the count never reaches zero (#832; confirmed deliberately by canary #946 — its check summary showed an empty waiting-checks list with the pending condition unsatisfied). Named per-workflow anchors are the remaining option and path filtering (#834) killed it: an anchor on a workflow that does not run pends forever. Workflow-run polling sidesteps all three, at the cost of one runner for the CI duration. +**Zero predicted runs is a pass, and that is a derived verdict rather than a blind spot.** Post-#834 a diff can legitimately trigger nothing (docs, `agents/`, `notes/`); the gate reads the same workflow files GitHub does, so an empty required set means the diff really does trigger no checks. Two corollaries: a path-filtered workflow still in flight **does** hold the gate (it was predicted, so it is required — the opposite of the check-counting era), and a workflow that should have dispatched but didn't hangs the gate instead of passing silently. #862 would have closed the residual CI-config-only gap with a `ci-paths` check; it is closed `not_planned` because prediction subsumes most of it. -**The two race guards are `pre-sleep` and `minimum-checks`.** At time T a poller cannot distinguish "all runs finished" from "no runs dispatched yet". `pre-sleep: 30` (wall-clock anchored; setup time is absorbed) covers GitHub's dispatch fan-out, observed at ~2s across nine workflows. `minimum-checks: 0` makes zero runs after the pre-sleep a **pass**: post-#834 a PR can legitimately trigger zero checks, and a check that should have existed but doesn't is the workflow's defect, not the gate's (#943's scope ruling; workflow-config validation belongs to #862). Do not raise `minimum-checks` above 0 — with path filtering there is no floor that is always satisfied, and a floor the diff doesn't meet burns the full timeout as a false red. +**There is nothing to tune** — `github-token` is the whole input surface, and `pre-sleep` / `minimum-checks` / `timeout` / `job-name` / `excluded-jobs` were removed upstream when prediction landed (GitHub ignores them with a warning, so a config still passing them is doing nothing). `timeout-minutes: 20` on the job is the backstop, not a gate setting. When the gate is red or stuck, **read its log first — it prints `Required: [...]`, the prediction**: `Non-passing runs` is an ordinary CI failure, an *unexpected* run is a willfire modelling gap, and a timeout is either over-prediction or a hung job (diff `Required:` against the runs that exist to tell which). Full semantics, the configuration constraints, per-shape debugging, and why check-run counting could not work: `agents/reference/pr-monitor.md`. -**When the gate is red, its log names the culprits** (`Non-passing runs: [...]`). When it **times out** instead, it is a hung job, not load: `timeout: 45` is double the longest run in recorded history (Release Precheck, 22m21s, under multi-PR contention — which is also what killed the original gate's 20-minute timeout), and post-#834 full fan-out completes in ~3 minutes. Open the named in-progress run and look at the stuck job's current *step*: a slow job advances, a hung one sits in one step while its siblings finish in seconds. Cancel it rather than letting it burn its timeout. Learned on #727, where a test awaited an event that a behavior change meant would never arrive: three jobs sat in one step for 35+ minutes and it was read as flake twice before anyone looked at the steps. Note the gate treats `cancelled` as a failure, so a cancelled hung job needs a re-run of that workflow (and the gate) once fixed. - -**Gate changes are testable on the PR that makes them.** The gate is an ordinary workflow, so a PR editing `pr-monitor.yml` is gated by its own edited copy — unlike Mergify, which read config from `main` only and forced land-and-canary cycles to test anything. - -**Flaky jobs are back to manual handling.** Mergify's CI Insights auto-retry left with Mergify: if a job is flaky, re-run it by hand to unblock, then fix or quarantine it — the gate polls live state, so a re-run that goes green flips the gate without re-triggering anything else. (The gate's own run can be re-run too if it timed out against a since-fixed hang.) +**Gate changes are testable on the PR that makes them.** The gate is an ordinary workflow, so a PR editing `pr-monitor.yml` is gated by its own edited copy — unlike Mergify, which read config from `main` only and forced land-and-canary cycles to test anything. **Flaky jobs are manual**: re-run by hand to unblock, then fix or quarantine. **Auto-merge is GitHub-native:** enable auto-merge on the PR (UI or `mcp__github__enable_pr_auto_merge`) and it merges once branch protection is satisfied — `CI Gate` green plus an approving review. The old Mergify `auto-merge` label does nothing now. @@ -285,4 +281,4 @@ When monitoring PRs to get them across the finish line (shepherding to green): ### Coverage Floor -All three SDKs enforce unit-coverage floors via testing-conventions `unit coverage`; floors live in `testing-conventions.toml`. Python/TS run inside `conventions.yml` at 100% (plus a PR-only changed-lines check); the Rust core is bespoke in `rust-test.yml` (nightly `cargo llvm-cov --lib --features cli --branch`; floors: lines 94 / regions 93 / functions 91 / branch 75, pinned near actuals -- if adding tests lowers `branch`, nudge the floor down rather than chasing it). **Floors apply to unit tests only** -- integration/binding/e2e never pad the number; every covered source file needs in-process unit tests (inject and mock the binding layer rather than excluding a file; only line-scoped, reason-required coverage exemptions exist, and there are none today). Full rationale, Rust branch-floor caveats, and exemption rules: `agents/reference/testing-gates.md`. +All three SDKs enforce unit-coverage floors via testing-conventions `unit coverage`; floors live in `testing-conventions.toml`. Python/TS run inside `dirsql-python-ci.yml` / `dirsql-typescript-ci.yml` at 100% (plus a PR-only changed-lines check); the Rust core is bespoke in `dirsql-rust-ci.yml` (nightly `cargo llvm-cov --lib --features cli --branch`; floors: lines 94 / regions 93 / functions 91 / branch 75, pinned near actuals -- if adding tests lowers `branch`, nudge the floor down rather than chasing it). **Floors apply to unit tests only** -- integration/binding/e2e never pad the number; every covered source file needs in-process unit tests (inject and mock the binding layer rather than excluding a file; only line-scoped, reason-required coverage exemptions exist, and there are none today). Full rationale, Rust branch-floor caveats, and exemption rules: `agents/reference/testing-gates.md`. diff --git a/agents/reference/changelog-migrations.md b/agents/reference/changelog-migrations.md index a5aae32c5..bc30191f8 100644 --- a/agents/reference/changelog-migrations.md +++ b/agents/reference/changelog-migrations.md @@ -5,7 +5,9 @@ Extracted from AGENTS.md (see "Changelog and Migrations" there for the summary). **Every PR that touches public-facing SDK code must add a changelog fragment.** This is enforced in CI by the `changelog-gate` check (`internals/checks`), whose implementation mirrors [template-lib](https://github.com/thekevinbot/template-lib)'s reference gate (#566); an unmet gate blocks merge. -The scope: any change to non-test source under a package root -- `packages//` (the three SDKs) or `plugins//` (independently published plugins, #896) -- requires a fragment naming that package. A package is identified by its root-qualified directory, so `plugins/ts` and `packages/ts` would be two packages. Exempt are test files (`*_test.py`, `*.test.ts` / `*.spec.ts`, anything under `//tests/`), the package `CHANGELOG.md` / `MIGRATIONS.md` pointer stubs, the `e2e-attestations/` receipts, and the fragment folders themselves. We err toward requiring entries because the project does not yet strictly follow semver, so the changelog must carry the signal that semver would otherwise provide. +The scope: any change to non-test source under a package root -- `packages//` (the three SDKs) or `plugins//` (independently published plugins, #896) -- requires a fragment naming that package. A package is identified by its root-qualified directory, so `plugins/ts` and `packages/ts` would be two packages. Exempt are test files (`*_test.py`, `*.test.ts` / `*.spec.ts`, anything under `//tests/`), the package `CHANGELOG.md` / `MIGRATIONS.md` pointer stubs, the `e2e-attestations/` receipts, and the fragment folders themselves. + +**A package `README.md` is source, not docs, and does require a fragment** -- `_is_exempt` in `changelog_gate/decide.py` deliberately omits it. READMEs ship inside all three published artifacts (the PyPI long-description, the npm tarball, the crate), so an edit reaches users the way code does; `release-ci.yml` excludes the fragment dirs and attestation receipts from its triggers but pointedly keeps READMEs, for the same reason. So a README-only PR runs both `release-ci` and `changelog-check`, and needs a fragment (or a `skip-changelog:` reason). This is the one place where the "docs are free" rule of #834's path-filter spec does not apply -- that epic's table listed `README.md` alongside the root prose files, and the maintainer decision during its final review was to keep the gate as written and correct the spec instead. We err toward requiring entries because the project does not yet strictly follow semver, so the changelog must carry the signal that semver would otherwise provide. **Fragments are per-package and colocated (#565), so they ship with the package.** Each SDK package (`python`, `ts`, `rust`) owns its own changelog under `packages//changelog.d/`, as does each plugin under `plugins//changelog.d/`, and a PR adds one fragment per **changed package** -- the fragment lives under the same package whose source changed: diff --git a/agents/reference/e2e-attestation.md b/agents/reference/e2e-attestation.md index 73bf74e6d..54351d960 100644 --- a/agents/reference/e2e-attestation.md +++ b/agents/reference/e2e-attestation.md @@ -3,7 +3,7 @@ Extracted from AGENTS.md (see "E2E Attestation" there for the summary). -CI does not run the e2e suites -- they need real binaries, and some need live LLM calls -- but it enforces, **per package**, that they *were* run against that package's current code. Each SDK package carries its own attestation at its root -- `packages/python/e2e-attestation.json` and `packages/ts/e2e-attestation.json` -- recording (via [`testing-conventions`](https://github.com/thekevinscott/testing-conventions)) the e2e command, its exit code, and the commit it ran against. The freshness check runs **inside the reusable workflow** (`conventions.yml`, the `python-sdk` / `typescript-sdk` `e2e-verify` gate): `e2e verify "$PACKAGE_ROOT" --scope "$SCAN_PATH" --base "$BASE"` measures freshness over the **`base..HEAD` scoped-source diff**, so a PR that does not touch the SDK source has nothing to verify. It is a **freshness gate, not a test runner** -- no suite, no build, and no LLM run in CI, so it does not violate the E2E Test Policy above. The bespoke `e2e-attestation.yml` this replaced is deleted (whole-hog adoption -- the reusable `e2e-verify` gate owns per-package freshness). +CI does not run the e2e suites -- they need real binaries, and some need live LLM calls -- but it enforces, **per package**, that they *were* run against that package's current code. Each SDK package carries its own attestation at its root -- `packages/python/e2e-attestation.json` and `packages/ts/e2e-attestation.json` -- recording (via [`testing-conventions`](https://github.com/thekevinscott/testing-conventions)) the e2e command, its exit code, and the commit it ran against. The freshness check runs **inside the reusable workflow** (`dirsql-python-ci.yml` / `dirsql-typescript-ci.yml`, the `python-sdk` / `typescript-sdk` `e2e-verify` gate): `e2e verify "$PACKAGE_ROOT" --scope "$SCAN_PATH" --base "$BASE"` measures freshness over the **`base..HEAD` scoped-source diff**, so a PR that does not touch the SDK source has nothing to verify. It is a **freshness gate, not a test runner** -- no suite, no build, and no LLM run in CI, so it does not violate the E2E Test Policy above. The bespoke `e2e-attestation.yml` this replaced is deleted (whole-hog adoption -- the reusable `e2e-verify` gate owns per-package freshness). The `--scope` + `--base` diff-scoping makes the gate per-SDK by construction: a change under `packages/python/dirsql` stales only the python attestation, a change under `packages/ts/src` only the ts one, and a PR that does not touch a package's scoped source never verifies it. Each package's `[e2e].extra_scope` widens *its own* scope by the code compiled into it (below) without breaking that property. diff --git a/agents/reference/pr-monitor.md b/agents/reference/pr-monitor.md new file mode 100644 index 000000000..28b6ac5b2 --- /dev/null +++ b/agents/reference/pr-monitor.md @@ -0,0 +1,63 @@ +# Merge gating (pr-monitor): semantics & debugging + +Extracted from AGENTS.md (see "PR Monitoring" there for the summary). This is the full operational reference. + +The gate is `.github/workflows/pr-monitor.yml`, whose single job is named **`CI Gate`** — the only context branch protection requires. It runs [thekevinscott/pr-monitor](https://github.com/thekevinscott/pr-monitor) at `@v1`, a rolling tag tracking that repo's `main`. + +## How the verdict is computed + +The action does not guess at the check set. [willfire](https://github.com/thekevinscott/willfire) evaluates the repo's workflow files against the PR's base branch, changed files and head commit, returning the entries GitHub will actually create; everything willfire does not mark `no-dispatch` becomes a **required workflow file**. The gate then polls `listWorkflowRunsForRepo` for the head commit every 5s, keeps the `pull_request` runs, and compares sets: + +| Observation | Verdict | +| --- | --- | +| a required run is missing, or exists but unfinished | keep waiting (yellow) | +| every required run finished `success` / `skipped` / `neutral` / `stale` | pass | +| a required run finished anything else (`failure`, `cancelled`, `timed_out`, `action_required`, `startup_failure`) | fail | +| a run exists that nothing predicted | fail immediately | + +Both sides exclude the gate's own workflow *file*, read from `GITHUB_WORKFLOW_REF`. + +**Why workflow runs, not check runs.** willfire predicts at job granularity, but the comparison happens at run granularity: a run stays non-terminal until *all* of its jobs finish — `needs:`-gated jobs and reusable-workflow (`workflow_call`) children included — so "the run finished" already means "every job finished", with no transient gap to race against. It also makes willfire's job-level `unknown` verdicts harmless: a matrix computed at runtime from another job's output cannot be expanded statically, but the run exists either way and still has to go green. + +## Configuration + +`github-token` is the entire input surface. `pre-sleep`, `minimum-checks`, `timeout`, `check-interval`, `job-name` and `excluded-jobs` were all **removed upstream** when prediction landed — each existed only to compensate for not knowing the expected run set. GitHub warns and ignores inputs a composite action does not declare, so ours sat inert in `pr-monitor.yml` until they were dropped; a config still passing them is silently doing nothing. + +Three structural requirements: + +- **`permissions:` needs all three** of `actions: read`, `contents: read`, `pull-requests: read`. The last one is willfire reading the PR and its changed files; it postdates the check-count era, so an older block is missing it. +- **The gate stays alone in its workflow.** The action excludes its own workflow file from both sides of the comparison, so a sibling job added to `pr-monitor.yml` would go entirely unmonitored. +- **`timeout-minutes` is the backstop.** The action has no timeout of its own (see its `monitor.ts`) and would poll forever; the job-level 20 kills it instead. Post-#834 fan-out completes in ~3 minutes, so 20 is generous. The `concurrency` group with `cancel-in-progress` covers the other half: a gate run resolves its target SHA at startup, so once a new commit lands the old run is polling a SHA nobody cares about — cancelling it frees the runner rather than letting it finish. + +## Debugging a red or stuck gate + +Every run logs `Required: [...]` — the prediction. Read it before hypothesizing. + +- **`Non-passing runs: [...]`** — an ordinary CI failure, in the workflow named. Fix that. +- **An unexpected run** — a run exists that willfire did not predict, so the gate fails rather than vouch for a set it cannot explain. This is a *modelling* gap: willfire does not model `workflow_run` chains or `pull_request_target`, and it infers `opened` vs `synchronize` from the PR's commit count rather than the real event ([willfire#2](https://github.com/thekevinscott/willfire/issues/2)), so a workflow narrowing `types:` can have its verdict flipped by a wrong guess. Every dirsql workflow is on a bare `on: pull_request` (with `paths:` / `branches:`, never `types:`), so this firing means a new workflow shape crept in. +- **A timeout** — over-prediction or a hang. Diff the logged `Required:` list against the runs that exist. A required run that never dispatched is over-prediction: the workflow's filters and willfire disagree, and the workflow is the thing to look at. A run that dispatched and sat is a hung job — open it and look at the stuck job's current *step*: a slow job advances, a hung one sits in one step while its siblings finish in seconds. Cancel it rather than letting it burn the timeout. Learned on #727, where a test awaited an event that a behavior change meant would never arrive: three jobs sat in one step for 35+ minutes and it was read as flake twice before anyone looked at the steps. Because the gate treats `cancelled` as a failure, a cancelled hung job needs a re-run of that workflow (and of the gate) once fixed. + +**The required-check context is the job's `name:`**, not the workflow's. Renaming `CI Gate` leaves every open PR reporting the old name until it gets a fresh run — merge the base branch in, or push, to retrigger. GitHub does not re-evaluate an existing run against the new requirement. + +**Flaky jobs are manual.** Mergify's CI Insights auto-retry left with Mergify. Re-run a flaky job by hand to unblock, then fix or quarantine it; the gate polls live state, so a re-run that goes green flips the gate without re-triggering anything else. The gate's own run can be re-run too, if it timed out against a since-fixed hang. + +## Why not check-run counting (the Mergify era, #830–#947) + +A gate built on check-run counts cannot express "everything expected has finished". + +- `#check-failure = 0` alone is vacuously true before CI dispatches and while it runs — a gate that does not gate (#943 measured it green 2m21s before the last check finished). +- Adding `#check-pending = 0` deadlocks: Mergify counts its own in-progress check, so the count never reaches zero (#832; confirmed deliberately by canary #946, whose check summary showed an empty waiting-checks list with the pending condition unsatisfied). +- Named per-workflow anchors were the remaining option, and path filtering (#834) killed them: an anchor on a workflow that does not run pends forever. + +Predicting the run set sidesteps all three — the expected set is computed per PR instead of enumerated in config — at the cost of one runner for the CI duration. It also restores a property Mergify never had: **gate changes are testable on the PR that makes them**, since the gate is an ordinary workflow and a PR editing `pr-monitor.yml` is gated by its own edited copy. Mergify read its config from `main` only, forcing land-and-canary cycles to test anything. + +## The zero-check policy + +**A PR whose prediction is empty passes.** Post-#834 path filtering a diff can legitimately trigger nothing (docs, `agents/`, `notes/`, root `*.md`); a `[skip ci]` commit predicts nothing either. + +Under check-counting this was an accepted blind spot — the gate could not tell "nothing should run" from "nothing has started yet". Under prediction it is a derived verdict: the gate reads the same workflow files GitHub does, so an empty required set means the diff really does trigger no checks. Two consequences follow. + +1. **A path-filtered workflow still in flight now holds the gate.** It was predicted, so it is required, and the gate stays yellow until its run finishes. The old corollary — that an in-flight filtered workflow did not hold the gate — is obsolete. +2. **A workflow that should have dispatched but didn't now hangs the gate** rather than passing silently, surfacing as an over-prediction timeout. + +#862 was to close the residual CI-config-only gap with a `dirsql-checks ci-paths` check asserting that workflow filters and the gate's config agree. It is closed `not_planned`: prediction subsumes most of it, since a config-only PR gets its expected set computed from the very files it edits. diff --git a/agents/reference/testing-conventions-ci.md b/agents/reference/testing-conventions-ci.md index 368ae6924..bc9bd9977 100644 --- a/agents/reference/testing-conventions-ci.md +++ b/agents/reference/testing-conventions-ci.md @@ -3,10 +3,21 @@ Extracted from AGENTS.md (see "CI Workflows" there for the summary). This is the full operational reference. -`conventions.yml` calls the `testing-conventions` reusable workflow at the **moving tag `@v0`** (see #240 and its sub-issues). Hard-won operational rules: +Six per-domain workflows call the `testing-conventions` reusable workflow at the **moving tag `@v0`** (see #240 and its sub-issues). #861 split the single `conventions.yml` into them so GitHub's workflow-scoped `paths:` filters could triage per lane: -- **`@v0` rolls; a removed/renamed input startup-fails the WHOLE workflow.** When upstream moves an input (e.g. #289 removed `build_command`, migrating it to config), any caller still passing it fails with `startup_failure` and **0 jobs** -- and it takes down `conventions.yml` on `main` and every open PR, not just the PR that added it. So an input change is a live breakage; fix it fast. -- **On `startup_failure`, read the run annotation FIRST -- do not hypothesize.** There are no job logs (no jobs ran), so `get_job_logs` is empty; instead WebFetch the run's `html_url`, which surfaces `Invalid workflow file: conventions.yml#L -- Invalid input, is not defined in the referenced workflow`. That names the file, line, and cause exactly. Guessing "tag-roll / transient / the gate itself" wastes rerun cycles; the annotation is authoritative. +| Workflow | Caller jobs | +| --- | --- | +| `dirsql-python-ci.yml` | `python-sdk`, `rust-python-binding` | +| `dirsql-typescript-ci.yml` | `typescript-sdk`, `rust-napi-binding` | +| `dirsql-rust-ci.yml` | `rust` | +| `internals-checks-ci.yml` | `internals-checks` | +| `internals-distcheck-ci.yml` | `internals-distcheck` | +| `plugin-dirsql-embedding-ci.yml` | `plugins-embeddings` | + +The eight caller jobs are the pre-split eight lanes, job ids unchanged -- so check-run names (` / `) survived the move. Hard-won operational rules: + +- **`@v0` rolls; a removed/renamed input startup-fails the WHOLE calling workflow.** When upstream moves an input (e.g. #289 removed `build_command`, migrating it to config), any caller still passing it fails with `startup_failure` and **0 jobs** -- and it takes down that workflow on `main` and every open PR, not just the PR that added it. Every caller passes the same input set, so one removed input reds all six at once; the split changed the blast radius' shape, not its size. An input change is a live breakage; fix it fast. +- **On `startup_failure`, read the run annotation FIRST -- do not hypothesize.** There are no job logs (no jobs ran), so `get_job_logs` is empty; instead WebFetch the run's `html_url`, which surfaces `Invalid workflow file: .yml#L -- Invalid input, is not defined in the referenced workflow`. That names the file, line, and cause exactly. Guessing "tag-roll / transient / the gate itself" wastes rerun cycles; the annotation is authoritative. - **`testing-conventions.toml` is validated strictly -- an unknown key fails EVERY gate.** A bad `[].` (e.g. `[typescript].build_command`, which does not exist -- `[typescript]`/`[rust]` accept only `coverage`/`exempt`; `build_command` is `[python]`-only) makes the CLI reject the whole config, so every gate (even colocated-test) goes red. Native builds **auto-provision from the manifest** (maturin / napi / `Cargo.toml`): a napi/TS package needs **no** `build_command` at all -- keep `rust_toolchain: true` to supply cargo, nothing else. - **Read reusable-workflow behavior at a PINNED commit, and trust `MIGRATIONS.md` over probing.** `git ls-remote https://github.com/thekevinscott/testing-conventions v0` for the sha, then WebFetch the raw workflow at that sha (`@v0` can roll mid-run and spuriously `startup_failure` a probe). Before probing a gate, cross-check upstream `packages//MIGRATIONS.md` (authoritative): "landed" often means the CLI *capability* shipped while the reusable *job* isn't wired to it yet (e.g. `e2e verify --scope` shipped one release before the job passed it). - **e2e attestation goes stale after a squash merge.** The merged attestation names the pre-squash commit, which is dangling on `main`; the reusable `e2e-verify` job runs unconditionally, so it reds. `e2e verify --scope ` narrows the freshness walk to the source dir so test/docs-only commits don't stale it. A stale-attestation red is **our** re-attest to fix (as the last commit touching that package -- mind the ordering trap: `attest` records `HEAD`, so attest right after the package's own last change), never a tool bug. @@ -14,19 +25,19 @@ Extracted from AGENTS.md (see "CI Workflows" there for the summary). This is the #### Adoption state (post-#240, 2026-07-08) -Every adoptable gate runs inside `conventions.yml`, per package; `testing-conventions.toml` carries **zero** exemptions. The map: +Every adoptable gate runs inside the per-domain CI workflows, per package; `testing-conventions.toml` carries **zero** exemptions. The map: | Gate | python | typescript | rust (core) | | --- | --- | --- | --- | | colocated-test (+ co-change) | ✅ | ✅ | ✅ (presence-only) | | unit-lint | ✅ | ✅ | ✅ | | integration-lint | ✅ | ✅ | ✅ | -| unit-coverage | ✅ | ✅ | ❌ bespoke in `rust-test.yml` -- permanent, see below | +| unit-coverage | ✅ | ✅ | ❌ bespoke in `dirsql-rust-ci.yml` -- permanent, see below | | mutation | ✅ | ✅ | ✅ | | e2e-verify | ✅ | ✅ | N/A by design, see below | | packaging | ✅ | ✅ | ✅ (workspace-member support, upstream #360/#362) | The binding crates (`packages/ts/napi`, `packages/python`) additionally run colocated-test + unit-lint at their own crate roots (#405). -- **Rust unit-coverage is bespoke PERMANENTLY, not pending upstream.** The `branch` floor needs nightly (`cargo llvm-cov --branch`); the reusable coverage job provisions stable, and upstream declined a toolchain input. The sanctioned alternative -- a crate-root `packages/rust/rust-toolchain.toml` -- was probed in #437 and **breaks the release build**: the release precheck cross-compiles from `packages/rust` on stable with added musl/darwin targets, and the pin switched that build to nightly (`E0463: can't find crate for core` on every target). One crate, one directory, two callers needing different channels -- a crate-root pin cannot serve both, so `rust-test.yml`'s coverage job scopes nightly to its own step. Revisit only if upstream gains a toolchain selector that does not leak to other builds of the same crate. +- **Rust unit-coverage is bespoke PERMANENTLY, not pending upstream.** The `branch` floor needs nightly (`cargo llvm-cov --branch`); the reusable coverage job provisions stable, and upstream declined a toolchain input. The sanctioned alternative -- a crate-root `packages/rust/rust-toolchain.toml` -- was probed in #437 and **breaks the release build**: the release precheck cross-compiles from `packages/rust` on stable with added musl/darwin targets, and the pin switched that build to nightly (`E0463: can't find crate for core` on every target). One crate, one directory, two callers needing different channels -- a crate-root pin cannot serve both, so `dirsql-rust-ci.yml`'s coverage job scopes nightly to its own step. Revisit only if upstream gains a toolchain selector that does not leak to other builds of the same crate. - **Rust has no e2e-verify because it has nothing to attest.** Attestations are receipts for suites CI cannot run (they spawn the shipped binary through the real launchers); rust's outermost tier (`packages/rust/tests/`) runs directly in CI on every PR, so a receipt would be strictly weaker evidence than the run itself. Core changes are still e2e-freshness-enforced through BOTH bindings via `[e2e].extra_scope` (see *E2E Attestation*). diff --git a/agents/reference/testing-gates.md b/agents/reference/testing-gates.md index 567d284d6..d39e7674e 100644 --- a/agents/reference/testing-gates.md +++ b/agents/reference/testing-gates.md @@ -4,29 +4,30 @@ Extracted from AGENTS.md (see "Testing" there for the summary). Full reference f ## Enforcing Colocation -The Python/TypeScript/Rust colocation rule above is enforced as a **blocking CI gate** by [`testing-conventions`](https://github.com/thekevinscott/testing-conventions), a config-driven CLI that scans each SDK's source tree and fails on any source file lacking a colocated unit test (for Rust, an inline `#[cfg(test)]` module). The wiring lives in `.github/workflows/testing-conventions.yml` (it pins the CLI version and runs the per-language `unit colocated-test` presence scans, plus a PR-only `--base` co-change check for Python/TypeScript that fails when a modified source's colocated test did not change alongside it) and `testing-conventions.toml` (the exempt list). +The Python/TypeScript/Rust colocation rule above is enforced as a **blocking CI gate** by [`testing-conventions`](https://github.com/thekevinscott/testing-conventions), a config-driven CLI that scans each SDK's source tree and fails on any source file lacking a colocated unit test (for Rust, an inline `#[cfg(test)]` module). The wiring lives in the per-domain CI workflows (`dirsql-python-ci.yml`, `dirsql-typescript-ci.yml`, `dirsql-rust-ci.yml`, `internals-checks-ci.yml`, `internals-distcheck-ci.yml`, `plugin-dirsql-embedding-ci.yml` -- each calls the reusable `testing-conventions.yml`, running the per-language `unit colocated-test` presence scans plus a PR-only `--base` co-change check for Python/TypeScript that fails when a modified source's colocated test did not change alongside it) and `testing-conventions.toml` (the exempt list). The same workflow also runs `unit lint` -- the **isolation** rule: a unit test must mock every collaborator (it must not import an un-mocked one), so the test exercises only the unit under test. It is wired for **all three SDKs** (#233 / epic #231): Python and TypeScript first, then Rust once its effectful-std unit tests were either moved to the integration tier (real filesystem/subprocess/`notify` behavior belongs there) or routed through a trait-injected `FileSystem` double in the core. For Rust the rule is `no-out-of-module-call`/`no-out-of-module-import`: a unit test may reach only `super::` (the unit) and pure `std` -- no effectful `std::fs`/`std::thread`/`std::env`/`std::time` and no out-of-module first-party import. The fix for a violation is to mock the collaborator (Python: patch it by its dotted path, e.g. `patch("pkg.mod.subprocess.run", ...)`, rather than importing it; TypeScript: `vi.mock("")`; Rust: inject a trait double or relocate the effectful test to `tests/`), or, when a dependency is naturally a callable the unit receives, to inject it (DI) -- never to weaken the test. -The scan covers the two native **binding crates** too (#405): `conventions.yml`'s `rust-napi-binding` (`packages/ts/napi`) and `rust-python-binding` (`packages/python`) calls run `colocated-test` + `unit-lint` at each crate root, alongside the core `rust` call. Their **pure** conversion logic is unit-tested inline -- napi's `value_to_js` / `row_event_to_js`, and the pyo3 binding's `row_event_to_plain` (a GIL-free intermediate extracted so the variant->action / field-selection mapping is unit-testable, mirroring napi; `PyRowEvent` is built from it unchanged). The **runtime-coupled** parts (napi `napi::sys` getters / `FnRef`; pyo3 GIL conversions `py_to_value` / `value_to_py` / `value_row_to_py_dict`) stay covered by the binding tier (`tests/integration/binding`), the same #233 split the core uses for effectful code. No exemption: a rust unit test constructing first-party `Value` / `RowEvent` via `super::*` passes `unit-lint`. No `mutation`/`coverage` gate on the binding crates -- those execute the suite and would need the napi/pyo3 build; #405 is the static presence+isolation gates. +The scan covers the two native **binding crates** too (#405): `dirsql-typescript-ci.yml`'s `rust-napi-binding` (`packages/ts/napi`) and `dirsql-python-ci.yml`'s `rust-python-binding` (`packages/python`) calls run `colocated-test` + `unit-lint` at each crate root, alongside `dirsql-rust-ci.yml`'s core `rust` call. Their **pure** conversion logic is unit-tested inline -- napi's `value_to_js` / `row_event_to_js`, and the pyo3 binding's `row_event_to_plain` (a GIL-free intermediate extracted so the variant->action / field-selection mapping is unit-testable, mirroring napi; `PyRowEvent` is built from it unchanged). The **runtime-coupled** parts (napi `napi::sys` getters / `FnRef`; pyo3 GIL conversions `py_to_value` / `value_to_py` / `value_row_to_py_dict`) stay covered by the binding tier (`tests/integration/binding`), the same #233 split the core uses for effectful code. No exemption: a rust unit test constructing first-party `Value` / `RowEvent` via `super::*` passes `unit-lint`. No `mutation`/`coverage` gate on the binding crates -- those execute the suite and would need the napi/pyo3 build; #405 is the static presence+isolation gates. -`conventions.yml`'s `internals-checks` call (#494/#503) gates the repo-tooling `internals/checks` package the same way: `colocated-test`, `unit-lint`, `integration-lint`, `unit-coverage`, `mutation`, and `e2e-verify` at `internals/checks/src` -- one call per package, `integration-lint` deriving its subjects (`tests/integration/`) from the package root (#515/#417). `tests/integration/` exercises each check's `gate.run()` against real collaborators (real git, real pytest subprocess); `tests/e2e/` spawns the packaged `dirsql-checks` CLI with nothing mocked, gated by `internals/checks/e2e-attestation.json` per the E2E Attestation convention below. +`internals-checks-ci.yml`'s `internals-checks` call (#494/#503) gates the repo-tooling `internals/checks` package the same way: `colocated-test`, `unit-lint`, `integration-lint`, `unit-coverage`, `mutation`, and `e2e-verify` at `internals/checks/src` -- one call per package, `integration-lint` deriving its subjects (`tests/integration/`) from the package root (#515/#417). `tests/integration/` exercises each check's `gate.run()` against real collaborators (real git, real pytest subprocess); `tests/e2e/` spawns the packaged `dirsql-checks` CLI with nothing mocked, gated by `internals/checks/e2e-attestation.json` per the E2E Attestation convention below. -`internals/distcheck` (#520) is the same species of repo-only uv package: the packaging distcheck flows (build → pack → install → run the published artifact), extracted from the former per-package packaging suites. It is a click group (`dirsql-distcheck`) with one subcommand per flow (`dirsql-distcheck python`, `dirsql-distcheck node` -- the node flow drives `npm`/`pnpm` via subprocess from Python; one tested home matters more than harness-language purity), each backed by a `gate.run()` whose effects funnel through an injected `runner` (subprocess) + `FileSystem` seam so the orchestration is unit-testable without a real build. The real flows run in CI via the `distcheck` jobs in `python-test.yml` / `ts-test.yml` (which build the prerequisites first, then invoke `dirsql-distcheck `); `conventions.yml`'s `internals-distcheck` call gates the package with `colocated-test`, `unit-lint`, `integration-lint`, `unit-coverage`, and `mutation` at `internals/distcheck/src` (a single call -- with #417 live on `@v0`, `integration-lint` derives its subjects from the package root, so no separate integration call is needed). No `e2e-verify`/attestation: the package has no e2e tier, since its `tests/integration/` (each flow's `gate.run()` against real subprocesses) is the outermost tier and the CI distcheck jobs run the real flows directly. +`internals/distcheck` (#520) is the same species of repo-only uv package: the packaging distcheck flows (build → pack → install → run the published artifact), extracted from the former per-package packaging suites. It is a click group (`dirsql-distcheck`) with one subcommand per flow (`dirsql-distcheck python`, `dirsql-distcheck node` -- the node flow drives `npm`/`pnpm` via subprocess from Python; one tested home matters more than harness-language purity), each backed by a `gate.run()` whose effects funnel through an injected `runner` (subprocess) + `FileSystem` seam so the orchestration is unit-testable without a real build. The real flows run in CI via the `distcheck` jobs in `dirsql-python-ci.yml` / `dirsql-typescript-ci.yml` (which build the prerequisites first, then invoke `dirsql-distcheck `); `internals-distcheck-ci.yml`'s `internals-distcheck` call gates the package with `colocated-test`, `unit-lint`, `integration-lint`, `unit-coverage`, and `mutation` at `internals/distcheck/src` (a single call -- with #417 live on `@v0`, `integration-lint` derives its subjects from the package root, so no separate integration call is needed). No `e2e-verify`/attestation: the package has no e2e tier, since its `tests/integration/` (each flow's `gate.run()` against real subprocesses) is the outermost tier and the CI distcheck jobs run the real flows directly. Run it locally before pushing: ```bash -just preflight # every (root, gate) pair conventions.yml declares +just preflight # every (root, gate) pair the CI workflows declare just preflight --dry-run # print the derived matrix without running it just preflight --gate unit-lint # narrow to one gate (repeatable) ``` `just preflight` is `dirsql-checks preflight` (#781): it **derives** the pairs from -`conventions.yml` rather than restating them, so a caller added to the workflow is a -pair it runs. It replaced four hand-written recipes that between them named 6 pairs -across 3 of the 8 scan roots -- so a locally-green run said nothing about the other -33 pairs, which is how the #777 failures reached CI. It also encodes the invocations +the CI workflows rather than restating them, reading every `.github/workflows/*.yml` +and folding together each job that calls the reusable workflow -- so a caller added +in a new per-domain file is a pair it runs. It replaced four hand-written recipes +that between them named 6 pairs across 3 of the 8 scan roots -- so a locally-green +run said nothing about the other 34 pairs, which is how the #777 failures reached CI. It also encodes the invocations that differ from the naive one, each of which was a silent false pass before: python `mutation` / `unit-coverage` run through the package's own venv (#706), typescript through `npx` (only the npm CLI appends `--ts-mutation-adapter`), rust @@ -42,9 +43,9 @@ The exemption count is again **zero**. The three-tier conformance work (#517) re The rung above coverage is the **`unit mutation`** gate (#235 / epic #231): testing-conventions mutates the source and fails on any **surviving** mutant -- one no unit test caught. Engines: **cosmic-ray** (Python), **Stryker** (TypeScript), **cargo-mutants** (Rust). It is **PR-only and diff-scoped** (`--base ...HEAD`): only the lines a PR added/modified are mutated, so each PR's surface stays bounded. A PR that changes no SDK source has nothing to mutate and passes trivially. -The gate reruns the real unit suite per mutant, so it needs the native bindings built. **TypeScript and Rust mutation now run inside the reusable workflow** (`.github/workflows/conventions.yml`): the upstream monorepo primitive -- #277 derives the package root from `source` (the caller input formerly named `path`, renamed upstream; dirsql#577), #279 makes the mutation job install/build from that derived root -- lets the reusable job build the native artifact itself (ts via `build_command: pnpm build` + `rust_toolchain: true`; rust via cargo-mutants, which builds the crate itself). This retired the bespoke `ts-mutation.yml` / `rust-mutation.yml` (#417). The CLI **self-provisions Stryker and cargo-mutants**, so there are no mutation engine deps or config in this repo. +The gate reruns the real unit suite per mutant, so it needs the native bindings built. **TypeScript and Rust mutation now run inside the reusable workflow** (called from `dirsql-typescript-ci.yml` / `dirsql-rust-ci.yml`): the upstream monorepo primitive -- #277 derives the package root from `source` (the caller input formerly named `path`, renamed upstream; dirsql#577), #279 makes the mutation job install/build from that derived root -- lets the reusable job build the native artifact itself (ts via `build_command: pnpm build` + `rust_toolchain: true`; rust via cargo-mutants, which builds the crate itself). This retired the bespoke `ts-mutation.yml` / `rust-mutation.yml` (#417). The CLI **self-provisions Stryker and cargo-mutants**, so there are no mutation engine deps or config in this repo. -**Python mutation now runs in the reusable workflow too** (`conventions.yml`, `python-sdk` gates: `mutation`): the testing-conventions wheel bundles the cosmic-ray adapter as a runtime dependency, so the reusable mutation job resolves the engine from the same `python_env=uv` (`uv sync`) environment it provisions for coverage — no separate install and no bespoke workflow. This retired `python-mutation.yml` (#426). All three SDKs' `mutation` gates now run inside `conventions.yml`. +**Python mutation now runs in the reusable workflow too** (`dirsql-python-ci.yml`, `python-sdk` gates: `mutation`): the testing-conventions wheel bundles the cosmic-ray adapter as a runtime dependency, so the reusable mutation job resolves the engine from the same `python_env=uv` (`uv sync`) environment it provisions for coverage — no separate install and no bespoke workflow. This retired `python-mutation.yml` (#426). All three SDKs' `mutation` gates now run inside their own CI workflow. Run a language locally (after building its native artifact), against your PR's base -- `just preflight --gate mutation` runs all three, or one at a time: @@ -73,8 +74,8 @@ The Rust arm handles `packages/rust` being a member of the repo-root cargo works Coverage enforcement must stay explicit in CI for each SDK package. All three SDKs are now enforced by [`testing-conventions`](https://github.com/thekevinscott/testing-conventions) `unit coverage`; the per-package floors live in the config each lane reads -- `packages/python/testing-conventions.toml` and `packages/ts/testing-conventions.toml` since #933, the root `testing-conventions.toml` for the rust core: -- **Python / TypeScript** run full tree + a PR-only `--base` changed-lines check, now **inside the reusable workflow** (`conventions.yml`, the `python-sdk` / `typescript-sdk` `unit-coverage` gate) after upstream #284 taught the coverage suite job to install/build from the derived package root (python via `python_env=uv` → `uv sync` builds maturin; ts via `build_command: pnpm build`). This retired the bespoke coverage jobs in `python-test.yml` / `ts-test.yml` (#412). Floors: `[python.coverage]` = `fail_under` / `branch`; `[typescript.coverage]` = `lines` / `branches` / `functions` / `statements` -- both held at the stricter **100%** (every line unit-reachable). -- **Rust core** (#295) is still measured bespoke -- unit-only via `cargo llvm-cov --lib --features cli --branch`, wired into `rust-test.yml`'s coverage job (nightly toolchain for `--branch`, scoped to that job's step alone -- a **permanent** bespoke exception, not pending upstream: the crate-root `rust-toolchain.toml` the reusable job would need breaks release cross-compile, see *Adoption state* above / #437). The CLI now scopes to `--lib` and passes `[rust].features` through (testing-conventions #269/#270/#271). Floors in `[rust.coverage]`: `lines` **94**, `regions` **93**, `functions` **91**, `branch` **75**. #354 first added pure unit tests for the reachable unit-tier gaps (`cli/router.rs`'s `parse_sql_body`, `cli/mod.rs`'s `with_query_timeout`/`From`, `cli/init.rs`'s error `Display` arms), lifting measured unit-only coverage to lines 94.1% / regions 93.2% / functions 91.6% / branch 75.6%; the floors are then set as high as they robustly go -- the highest integer under each actual. `lines`/`regions`/`functions` are above the ≥90% ideal. **`branch` keeps a documented sub-90 floor as an accepted exception** to the ≥90% rule, for two structural reasons. **These floors are pinned near their actuals (<1pt slack) by maintainer request**, so the whole-tree gate goes red on any PR that lowers a dimension -- most fragile for `branch`, where merely adding well-formed unit tests can lower the number (reason (2)); the fix when that happens is to nudge the affected floor down to the new actual, not to chase the dip as a regression. (1) The effectful production branches (filesystem / subprocess / HTTP / `notify`) live in the integration/e2e tier by design (#233): `cli/router.rs`'s async HTTP handlers (0% branch), `cli/init.rs`'s effectful `std::fs::write` success/`--force` arms, and `lib.rs`'s racy-window / watcher / persist error arms are unreachable from a unit test, and covering them re-litigates #233. (2) `--branch` instruments the crate *with its `#[cfg(test)]` modules*, so every `matches!(…, if a && b)` / `assert!(x && y)` guard in a unit test contributes short-circuit sub-condition branches whose False arm is unreachable by construction (the assertion is written to pass) -- `differ.rs` is the clearest case: 100% line/region/function coverage but 58% branch, with every uncovered branch inside a test-module guard, not production code. Because well-formed new unit tests therefore tend to *lower* branch %, the branch floor stays conservative on purpose -- a tight floor would fail legitimate test-adding PRs. The Rust job is **whole-tree only** (no `--base` changed-lines check) for the same reason -- a per-PR changed-lines floor would fail legitimate integration-tier edits. +- **Python / TypeScript** run full tree + a PR-only `--base` changed-lines check, now **inside the reusable workflow** (`dirsql-python-ci.yml` / `dirsql-typescript-ci.yml`, the `python-sdk` / `typescript-sdk` `unit-coverage` gate) after upstream #284 taught the coverage suite job to install/build from the derived package root (python via `python_env=uv` → `uv sync` builds maturin; ts via `build_command: pnpm build`). This retired the bespoke coverage jobs in the then-current `python-test.yml` / `ts-test.yml` (#412). Floors: `[python.coverage]` = `fail_under` / `branch`; `[typescript.coverage]` = `lines` / `branches` / `functions` / `statements` -- both held at the stricter **100%** (every line unit-reachable). +- **Rust core** (#295) is still measured bespoke -- unit-only via `cargo llvm-cov --lib --features cli --branch`, wired into `dirsql-rust-ci.yml`'s coverage job (nightly toolchain for `--branch`, scoped to that job's step alone -- a **permanent** bespoke exception, not pending upstream: the crate-root `rust-toolchain.toml` the reusable job would need breaks release cross-compile, see *Adoption state* above / #437). The CLI now scopes to `--lib` and passes `[rust].features` through (testing-conventions #269/#270/#271). Floors in `[rust.coverage]`: `lines` **94**, `regions` **93**, `functions` **91**, `branch` **75**. #354 first added pure unit tests for the reachable unit-tier gaps (`cli/router.rs`'s `parse_sql_body`, `cli/mod.rs`'s `with_query_timeout`/`From`, `cli/init.rs`'s error `Display` arms), lifting measured unit-only coverage to lines 94.1% / regions 93.2% / functions 91.6% / branch 75.6%; the floors are then set as high as they robustly go -- the highest integer under each actual. `lines`/`regions`/`functions` are above the ≥90% ideal. **`branch` keeps a documented sub-90 floor as an accepted exception** to the ≥90% rule, for two structural reasons. **These floors are pinned near their actuals (<1pt slack) by maintainer request**, so the whole-tree gate goes red on any PR that lowers a dimension -- most fragile for `branch`, where merely adding well-formed unit tests can lower the number (reason (2)); the fix when that happens is to nudge the affected floor down to the new actual, not to chase the dip as a regression. (1) The effectful production branches (filesystem / subprocess / HTTP / `notify`) live in the integration/e2e tier by design (#233): `cli/router.rs`'s async HTTP handlers (0% branch), `cli/init.rs`'s effectful `std::fs::write` success/`--force` arms, and `lib.rs`'s racy-window / watcher / persist error arms are unreachable from a unit test, and covering them re-litigates #233. (2) `--branch` instruments the crate *with its `#[cfg(test)]` modules*, so every `matches!(…, if a && b)` / `assert!(x && y)` guard in a unit test contributes short-circuit sub-condition branches whose False arm is unreachable by construction (the assertion is written to pass) -- `differ.rs` is the clearest case: 100% line/region/function coverage but 58% branch, with every uncovered branch inside a test-module guard, not production code. Because well-formed new unit tests therefore tend to *lower* branch %, the branch floor stays conservative on purpose -- a tight floor would fail legitimate test-adding PRs. The Rust job is **whole-tree only** (no `--base` changed-lines check) for the same reason -- a per-PR changed-lines floor would fail legitimate integration-tier edits. When work affects more than one SDK package, split the coverage and test work across subagents so each package can be validated independently. diff --git a/internals/distcheck/README.md b/internals/distcheck/README.md index d165f6bda..b1205aa87 100644 --- a/internals/distcheck/README.md +++ b/internals/distcheck/README.md @@ -51,7 +51,7 @@ Each flow follows the same shape (mirroring `internals/checks`): and calls `gate.run`, turning a `DistcheckError` into a non-zero exit. Every module (except empty `__init__.py`s) carries a colocated `*_test.py`, -gated by `conventions.yml`'s `internals-distcheck` job: `colocated-test`, +gated by `internals-distcheck-ci.yml`'s `internals-distcheck` job: `colocated-test`, `unit-lint`, `integration-lint`, `unit-coverage`, and `mutation`. ## Test tiers @@ -64,7 +64,7 @@ gated by `conventions.yml`'s `internals-distcheck` job: `colocated-test`, execution is the `dirsql-distcheck ` job, which builds them first. No e2e tier / attestation: `tests/integration/` is the outermost tier, and the -CI `distcheck` jobs (`python-test.yml` / `ts-test.yml`) run the real flows directly. +CI `distcheck` jobs (`dirsql-python-ci.yml` / `dirsql-typescript-ci.yml`) run the real flows directly. ```bash uv run --project internals/distcheck python -m pytest internals/distcheck/src internals/distcheck/tests -q diff --git a/justfile b/justfile index f7731ad05..cbf0a2944 100644 --- a/justfile +++ b/justfile @@ -36,13 +36,13 @@ test-e2e: # Run the Python packaging distcheck flow (build the wheel, install into a fresh # venv, run the installed CLI) from the internals/distcheck package (#520). Runs in -# CI (python-test.yml `distcheck` job). Needs `cargo build -p dirsql --features cli` +# CI (dirsql-python-ci.yml `distcheck` job). Needs `cargo build -p dirsql --features cli` # and `maturin` on PATH (uv sync in packages/python) first. test-distcheck-python: uv run --project internals/distcheck dirsql-distcheck python # Run the node packaging distcheck flow (pack -> npm install -> run) from the -# internals/distcheck package. Runs in CI (ts-test.yml `distcheck` job). Needs +# internals/distcheck package. Runs in CI (dirsql-typescript-ci.yml `distcheck` job). Needs # `pnpm build` in packages/ts first. test-distcheck-node: uv run --project internals/distcheck dirsql-distcheck node @@ -61,7 +61,7 @@ e2e-attest-internals-checks: # Refresh packages/python/e2e-attestation.json: runs the python e2e suite and # commits the attestation. The CI gate runs inside the reusable workflow -# (conventions.yml, python-sdk `e2e-verify`) on PRs that touch the python SDK +# (dirsql-python-ci.yml, python-sdk `e2e-verify`) on PRs that touch the python SDK # source. Install testing-conventions first (CI always uses the latest release): # pip install testing-conventions e2e-attest-python: @@ -92,7 +92,7 @@ preflight *ARGS: uv run --project internals/checks dirsql-checks preflight {{ARGS}} # Packaging gate: assert no test files ship in the built .whl / .tgz / .crate. -# Mirrors the testing-conventions `packaging` gate run in conventions.yml; +# Mirrors the testing-conventions `packaging` gate the per-domain CI workflows run; # requires uv, pnpm, cargo, and `pip install testing-conventions`. test-packaging: #!/usr/bin/env bash diff --git a/testing-conventions.toml b/testing-conventions.toml index b97f39d5a..cb477a0be 100644 --- a/testing-conventions.toml +++ b/testing-conventions.toml @@ -114,7 +114,7 @@ features = ["cli"] # Unit-only coverage floor: measured by `cargo llvm-cov --lib --features cli` # (the `--lib` scoping keeps the integration tier under packages/rust/tests/ out # of the number -- testing-conventions #269). `branch` runs under `--branch`, -# which needs the nightly toolchain pinned in rust-test.yml's coverage job. +# which needs the nightly toolchain pinned in dirsql-rust-ci.yml's coverage job. # # The floors are BELOW the bindings' 100% because the effectful paths in # lib.rs / cli/*.rs / watcher.rs are exercised at the integration/e2e tier by