Skip to content

Commit 42abd4f

Browse files
Migration hard-fail + post-apply integrity verifier; RLS SQL Server guard; LQL docs (#58)
## Summary - **Migration hard-fail (closes #53, #55):** add `SchemaIntegrityVerifier` and wire it into `DataProviderMigrate` so every run (including the "no operations needed" path) verifies the live DB matches the YAML and exits non-zero with a per-mismatch summary if it doesn't. - **MigrationRunner safety net:** when `ContinueOnError=true`, individual op failures used to be downgraded to warnings while the runner still returned `Ok` — closes the matching gap so any failed op rolls back the transaction and returns `Error`. - **RLS SQL Server guard:** fail closed with `MIG-E-RLS-MSSQL-UNSUPPORTED` when a schema with RLS targets SQL Server, since `Nimblesite.DataProvider.Migration.SqlServer` does not exist yet. - **Specs/docs:** refreshed migration-spec, codegen-cli-tool, RLS-PLAN, and LQL website docs; removed the orphaned RELEASE-PLAN and migration-cli-spec. - **Tooling:** updated agent skills (`fix-bug`, `submit-pr`, `spec-check`, `upgrade-packages`, `ci-prep`, `code-dedup`, `website-audit`) and Makefile, cleaned up rule files. ## Test plan - [x] `dotnet test Migration/Nimblesite.DataProvider.Migration.Tests` — 499/499 pass - [x] New `MigrationRunnerHardFailTests` — proves runner returns `Error` when any op fails with `ContinueOnError=true` - [x] New `DataProviderMigrateIntegrityTests` — proves CLI exits 1 with `SCHEMA INTEGRITY CHECK FAILED` when live DB drifts from YAML (column nullability + missing composite unique constraint) - [x] New `SchemaIntegrityVerifierTests` + `SchemaIntegrityVerifierFunctionDriftTests` — direct verifier coverage - [x] New `DataProviderMigrateRlsUnsupportedTests` — proves SQL Server + RLS exits 1 with the documented error code - [ ] Manual repro of #53/#55 against a real Postgres DB before closing the issues 🤖 Generated with [Claude Code](https://claude.com/claude-code) --------- Co-authored-by: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
1 parent f015e3e commit 42abd4f

53 files changed

Lines changed: 5011 additions & 601 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

.agents/skills/ci-prep/SKILL.md

Lines changed: 37 additions & 18 deletions
Original file line numberDiff line numberDiff line change
@@ -3,6 +3,7 @@ name: ci-prep
33
description: Prepares the current branch for CI by running the exact same steps locally and fixing issues. If CI is already failing, fetches the GH Actions logs first to diagnose. Use before pushing, when CI is red, or when the user says "fix ci".
44
argument-hint: "[--failing] [optional job name to focus on]"
55
---
6+
<!-- agent-pmo:74cf183 -->
67

78
# CI Prep
89

@@ -15,7 +16,7 @@ Prepare the current state for CI. If CI is already failing, fetch and analyze th
1516

1617
If `--failing` is NOT passed, skip directly to **Step 2**.
1718

18-
## Step 1 �� Fetch failed CI logs (only when `--failing`)
19+
## Step 1 Fetch failed CI logs (only when `--failing`)
1920

2021
You MUST do this before any other work.
2122

@@ -41,11 +42,27 @@ Read **every line** of `--log-failed` output. For each failure note the exact fi
4142

4243
## Step 2 — Analyze the CI workflow
4344

44-
1. Read `.github/workflows/ci.yml` completely. Parse every job and every step.
45-
2. Extract the ordered list of commands the CI actually runs.
46-
3. Note environment variables, matrix strategies, conditional steps, and service containers.
45+
1. Find the CI workflow file. Look in `.github/workflows/` for `ci.yml`, `build.yml`, `test.yml`, `checks.yml`, `main.yml`, `pull_request.yml`, or any workflow triggered on `pull_request` or `push`.
46+
2. Read the workflow file completely. Parse every job and every step.
47+
3. Extract the ordered list of commands the CI actually runs. In a spec-compliant repo this is `make lint → make test → make build` (REPO-STANDARDS-SPEC [MAKE-TARGETS]), but the actual CI may use `npm`, `cargo`, `dotnet`, raw shell commands, or anything else. Extract what is *actually there*.
48+
4. Note any environment variables, matrix strategies, or conditional steps that affect execution.
4749

48-
**Do NOT assume the steps are `make lint`, `make test`, `make build`.** Extract what the CI *actually does*.
50+
**Do NOT assume the steps are `make lint`, `make test`, `make build`.** The actual CI may run different commands, in a different order. Extract what the CI *actually does*. If you find extra targets beyond the 7 in [MAKE-TARGETS] (e.g. `make fmt-check`, `make coverage-check`), flag them in your final report — they should be consolidated by the agent-pmo skill.
51+
52+
### Release workflow blocker scan
53+
54+
If `.github/workflows/release.yml` exists, scan it before broad local CI. These are critical blockers
55+
and must be fixed before release work is considered CI-ready:
56+
57+
- Tag-triggered jobs checking out `ref: main` instead of the tagged SHA.
58+
- Any `git commit`, `git push`, branch mutation, or tag mutation during release.
59+
- Version bump commits after the tag already exists.
60+
- Ad hoc `sed` version stamping of structured files instead of a first-class stamper/build input.
61+
- Missing tests that pass a test version into the same stamper used by release.
62+
- Native VSIX releases without Node `22.x`, `npx vsce package --target <vsceTarget>`, one VSIX per
63+
target, target-suffixed filenames, and package-content verification.
64+
- VS Code native-binary activation that reads or mutates PATH, uses package-manager/global installs
65+
as normal startup sources, or copies bundled VSIX binaries after install.
4966

5067
## Step 3 — Run each CI step locally, in order
5168

@@ -58,45 +75,47 @@ Work through failures in this priority order:
5875

5976
For each command extracted from the CI workflow:
6077

61-
1. Run the command exactly as CI would run it.
78+
1. Run the command exactly as CI would run it (adjusting only for local environment differences like not needing `actions/checkout`).
6279
2. If the step fails, **stop and fix the issues** before continuing to the next step.
6380
3. After fixing, re-run the same step to confirm it passes.
6481
4. Move to the next step only after the current one succeeds.
6582

6683
### Hard constraints
6784

6885
- **NEVER modify test files** — fix the source code, not the tests
69-
- **NEVER add suppressions** (`#pragma warning disable`, `#[allow(...)]`, `// eslint-disable`)
86+
- **NEVER add suppressions** (`#[allow(...)]`, `// eslint-disable`, `#pragma warning disable`)
87+
- **NEVER use `any` in TypeScript** to silence type errors
7088
- **NEVER delete or ignore failing tests**
7189
- **NEVER remove assertions**
7290

7391
If stuck on the same failure after 5 attempts, ask the user for help.
7492

75-
## Step 4 — Loop
93+
## Step 4 — Report
7694

77-
- Go back to the first step and repeat until all steps pass locally. If `--failing`, you should see the exact same errors in your terminal that CI shows in the logs. Fix those errors until they are resolved.
95+
- List every step that was run and its result (pass/fail/fixed).
96+
- If any step could not be fixed, report what failed and why.
97+
- Confirm whether the branch is ready to push.
7898

79-
## Step 5 — Commit/Push (only when `--failing`)
99+
## Step 5 — Remote CI follow-up (only when `--failing`)
80100

81101
Once all CI steps pass locally:
82102

83-
1. Commit, but DO NOT MARK THE COMMIT WITH YOU AS AN AUTHOR!!! Only the user authors the commit!
84-
2. Push
85-
3. Monitor until completion or failure
86-
4. Upon failure, go back to Step 1
103+
1. Report the local fixes and exact commands that now pass.
104+
2. Do not commit or push. The user owns source-control writes.
105+
3. If the user pushes, monitor the new run until completion or failure.
106+
4. Upon failure, go back to Step 1.
87107

88108
## Rules
89109

90-
- *You are not allowed to commi/push until all tests pass*. Do not waste GitHub action minutes! The local CI must prove that everything is working.
91110
- **Always read the CI workflow first.** Never assume what commands CI runs.
92-
- Do not push if any step fails (unless `--failing` and all steps now pass)
111+
- Do not commit or push from this skill.
93112
- Fix issues found in each step before moving to the next
94113
- Never skip steps or suppress errors
95114
- If the CI workflow has multiple jobs, run all of them (respecting dependency order)
96-
- Skip steps that are CI-infrastructure-only (checkout, setup actions, cache steps, artifact uploads) — focus on the actual build/test/lint commands
115+
- Skip steps that are CI-infrastructure-only (checkout, setup-node/python/rust actions, cache steps, artifact uploads) — focus on the actual build/test/lint commands
97116

98117
## Success criteria
99118

100119
- Every command that CI runs has been executed locally and passed
101120
- All fixes are applied to the working tree
102-
- The CI passes successfully (if you are correcting an existing failure)
121+
- The CI passes successfully (if you are correcting and existing failure)

.agents/skills/code-dedup/SKILL.md

Lines changed: 45 additions & 36 deletions
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,7 @@
22
name: code-dedup
33
description: Searches for duplicate code, duplicate tests, and dead code, then safely merges or removes them. Use when the user says "deduplicate", "find duplicates", "remove dead code", "DRY up", or "code dedup". Requires test coverage — refuses to touch untested code.
44
---
5+
<!-- agent-pmo:74cf183 -->
56

67
# Code Dedup
78

@@ -12,8 +13,11 @@ Carefully search for duplicate code, duplicate tests, and dead code across the r
1213
Before touching ANY code, verify these conditions. If any fail, stop and report why.
1314

1415
1. Run `make test` — all tests must pass. If tests fail, stop. Do not dedup a broken codebase.
15-
2. Run `make coverage-check` — coverage must meet the repo's threshold. If it doesn't, stop.
16-
3. This repo uses **C#, F#, Rust, and TypeScript** — all statically typed. Proceed.
16+
2. Run `make test` — tests are fail-fast AND enforce the coverage threshold from `coverage-thresholds.json`. If anything fails, stop and fix it before deduping.
17+
3. Verify the project uses **static typing**. Check for:
18+
- Rust, C#, F#, Dart, Go: typed by default — proceed
19+
- TypeScript: `tsconfig.json` must have `"strict": true` — proceed (Lql/LqlExtension already does)
20+
- **Untyped JavaScript: STOP. Refuse to dedup.** Print: "This codebase has no static type checking. Deduplication without types is reckless — too high a risk of silent breakage. Add type checking first."
1721

1822
## Steps
1923

@@ -33,74 +37,79 @@ Dedup Progress:
3337

3438
Before deciding what to touch, understand what is tested.
3539

36-
1. Run `make test` and `make coverage-check` to confirm green baseline
40+
1. Run `make test` to confirm green baseline. `make test` is fail-fast AND enforces the coverage threshold from `coverage-thresholds.json` (REPO-STANDARDS-SPEC [TEST-RULES], [COVERAGE-THRESHOLDS-JSON]). It exits non-zero on any test failure OR coverage shortfall.
3741
2. Note the current coverage percentage — this is the floor. It must not drop.
3842
3. Identify which files/modules have coverage and which do not. Only files WITH coverage are candidates for dedup.
3943

4044
### Step 2 — Scan for dead code
4145

4246
Search for code that is never called, never imported, never referenced.
4347

44-
1. Look for unused exports, unused functions, unused records, unused variables
45-
2. Use language-appropriate tools:
46-
- **C#/F#:** Analyzer warnings for unused members (build with `-warnaserror` catches these)
47-
- **Rust:** The compiler already warns on dead code — check `make lint` output
48-
- **TypeScript:** Check for unexported functions with zero references in `Lql/LqlExtension/`
49-
3. For each candidate: **grep the entire codebase** for references (including tests, scripts, configs). Only mark as dead if truly zero references.
48+
1. Look for unused exports, unused functions, unused classes, unused variables.
49+
2. Use language-appropriate tools where available:
50+
- C#/F#: analyzer warnings (CA1801 unused parameters, IDE0051 unused private members) via `make lint`
51+
- Rust: `cargo clippy` already warns on dead code — check `make lint` output
52+
- TypeScript: `noUnusedLocals`/`noUnusedParameters` in `tsconfig.json`; look for unexported functions with zero references
53+
3. For each candidate: **grep the entire repo** (including tests, scripts, samples). Skip generated code under `Lql/lql-lsp-rust/crates/lql-parser/src/generated/`. Only mark as dead if truly zero references.
5054
4. List all dead code found with file paths and line numbers. Do NOT delete yet.
5155

5256
### Step 3 — Scan for duplicate code
5357

5458
Search for code blocks that do the same thing in multiple places.
5559

56-
1. Look for functions/methods with identical or near-identical logic
57-
2. Look for copy-pasted blocks (same structure, maybe different variable names)
58-
3. Look for multiple implementations of the same algorithm or pattern
59-
4. Check across module boundaries — duplicates often hide in different projects (DataProvider, Lql, Sync, Gatekeeper, Samples)
60-
5. For each duplicate pair: note both locations, what they do, and how they differ (if at all)
60+
1. Look for functions/methods with identical or near-identical logic.
61+
2. Look for copy-pasted blocks (same structure, maybe different variable names).
62+
3. Look for multiple implementations of the same algorithm — particularly likely across `DataProvider/`, `Migration/`, `Sync/`, `Reporting/` C# projects, and across `Lql/Nimblesite.Lql.Core` SQL transpiler dialects (`Postgres`, `SqlServer`, `SQLite`).
63+
4. Check across module boundaries — duplicates often hide in different `.csproj` projects or Rust crates (`lql-parser`, `lql-analyzer`, `lql-lsp`).
64+
5. For each duplicate pair: note both locations, what they do, and how they differ (if at all).
6165
6. List all duplicates found. Do NOT merge yet.
6266

6367
### Step 4 — Scan for duplicate tests
6468

6569
Search for tests that verify the same behavior.
6670

67-
1. Look for test functions with identical assertions against the same code paths
68-
2. Look for test fixtures/helpers that are duplicated across test files
69-
3. Look for integration tests that fully cover what a unit test also covers (keep the integration test, mark the unit test as redundant per CLAUDE.md rules)
71+
1. Look for test functions with identical assertions against the same code paths.
72+
2. Look for test fixtures/helpers that are duplicated across test projects (`Tests.Shared/` is meant to hold these).
73+
3. Look for integration tests that fully cover what a unit test also covers (keep the integration test, mark the unit test as redundant per CLAUDE.md "Prefer E2E/integration tests").
7074
4. List all duplicate tests found. Do NOT delete yet.
7175

7276
### Step 5 — Apply changes (one at a time)
7377

74-
For each change, follow this cycle: **change -> test -> verify coverage -> continue or revert**.
78+
For each change, follow this cycle: **change test verify coverage continue or revert**.
7579

7680
#### 5a. Remove dead code
77-
- Delete dead code identified in Step 2
78-
- After each deletion: run `make test` and `make coverage-check`
79-
- If tests fail or coverage drops: **revert immediately** and investigate
81+
- Delete dead code identified in Step 2.
82+
- After each deletion: run `make test` (fail-fast + coverage + threshold all in one).
83+
- If `make test` exits non-zero (test failure OR coverage drop): **revert immediately** and investigate.
84+
- Dead code removal should never break tests or drop coverage.
8085

8186
#### 5b. Merge duplicate code
82-
- For each duplicate pair: extract the shared logic into a single function/module
83-
- Update all call sites to use the shared version
84-
- After each merge: run `make test` and `make coverage-check`
87+
- For each duplicate pair: extract the shared logic into a single function/module. Prefer placing shared code in `Tests.Shared/` (for test helpers) or the closest `.Core` project (e.g. `Sync/Nimblesite.Sync.Core`, `Lql/Nimblesite.Lql.Core`).
88+
- Update all call sites to use the shared version.
89+
- After each merge: run `make test`.
8590
- If tests fail: **revert immediately**. The duplicates may have subtle differences you missed.
91+
- If coverage drops: the shared code must have equivalent test coverage. Add tests if needed before proceeding.
8692

8793
#### 5c. Remove duplicate tests
88-
- Delete the redundant test (keep the more thorough one)
89-
- After each deletion: run `make coverage-check`
90-
- If coverage drops: **revert immediately**. The "duplicate" test was covering something the other wasn't.
94+
- Delete the redundant test (keep the more thorough one).
95+
- After each deletion: run `make test`.
96+
- If coverage drops below threshold, `make test` exits non-zero — **revert immediately**. The "duplicate" test was covering something the other wasn't.
9197

9298
### Step 6 — Final verification
9399

94-
1. Run `make test` — all tests must still pass
95-
2. Run `make coverage-check` — coverage must be >= the baseline from Step 1
96-
3. Run `make lint` and `make fmt-check` — code must be clean
97-
4. Report: what was removed, what was merged, final coverage vs baseline
100+
1. Run `make lint` — all linters and the format check must pass.
101+
2. Run `make test` — tests must pass AND coverage must remain ≥ the baseline from Step 1.
102+
3. Report: what was removed, what was merged, final coverage vs baseline.
103+
104+
(Only the 7 standard targets exist — `make lint` and `make test` cover formatting and coverage checks respectively.)
98105

99106
## Rules
100107

101-
- **No test coverage = do not touch.** If a file has no tests covering it, leave it alone entirely.
102-
- **Coverage must not drop.** The coverage floor from Step 1 is sacred.
108+
- **No test coverage = do not touch.** If a file has no tests covering it, leave it alone entirely. You cannot safely dedup what you cannot verify.
109+
- **Coverage must not drop.** If removing or merging code causes coverage to decrease, revert and investigate. The coverage floor from Step 1 is sacred.
110+
- **Untyped code = refuse to dedup.** This repo has no untyped surfaces today; if any appear, refuse.
103111
- **One change at a time.** Make one dedup change, run tests, verify coverage. Never batch multiple dedup changes before testing.
104-
- **When in doubt, leave it.** If two code blocks look similar but you're not 100% sure they're functionally identical, leave both.
105-
- **Preserve public API surface.** Do not change function signatures, record names, or module exports that external code depends on. Internal refactoring only.
106-
- **Three similar lines is fine.** Only dedup when the shared logic is substantial (>10 lines) or when there are 3+ copies.
112+
- **When in doubt, leave it.** If two code blocks look similar but you're not 100% sure they're functionally identical, leave both. False dedup is worse than duplication.
113+
- **Preserve public API surface.** Do not change the public surface of `Nimblesite.DataProvider.Core`, `Nimblesite.Lql.Core`, `Nimblesite.Sync.Core`, or any package that is published to NuGet. Internal refactoring only.
114+
- **Three similar lines is fine.** Do not create abstractions for trivial duplication. Only dedup when the shared logic is substantial (>10 lines) or when there are 3+ copies.
115+
- **Never touch ANTLR-generated code.** `Lql/lql-lsp-rust/crates/lql-parser/src/generated/` is regenerated from the `.g4` grammar; any dedup there will be erased on the next regen.

.agents/skills/fix-bug/SKILL.md

Lines changed: 67 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,67 @@
1+
---
2+
name: fix-bug
3+
description: Fix a bug using test-driven development. Use when the user reports a bug, describes unexpected behavior, wants to fix a defect, or says something is broken. Enforces a strict test-first workflow where a failing test must be written and verified before any fix is attempted.
4+
argument-hint: "[bug description]"
5+
allowed-tools: Read, Grep, Glob, Edit, Write, Bash
6+
---
7+
<!-- agent-pmo:74cf183 -->
8+
9+
# Bug Fix Skill — Test-First Workflow
10+
11+
You MUST follow this exact workflow. Do NOT skip steps. Do NOT fix the bug before writing a failing test.
12+
13+
## Step 1: Understand the Bug
14+
15+
- Read the bug description: $ARGUMENTS
16+
- Investigate the codebase to understand the relevant code
17+
- Identify the root cause (or narrow down candidates)
18+
- Summarize your understanding of the bug to the user before proceeding
19+
20+
## Step 2: Write a Failing Test
21+
22+
- Write a test that **directly exercises the buggy behavior**
23+
- The test must assert the **correct/expected** behavior — so it FAILS against the current broken code
24+
- The test name should clearly describe the bug (e.g., `test_orange_color_not_applied_to_head`)
25+
- Use the project's existing test framework and conventions
26+
27+
## Step 3: Run the Test — Confirm It FAILS
28+
29+
- Run ONLY the new test (not the full suite)
30+
- **Verify the test FAILS** and that it fails **because of the bug**, not for some other reason (typo, import error, wrong selector, etc.)
31+
- If the test passes: your test does not capture the bug. Go back to Step 2
32+
- If the test fails for the wrong reason: fix the test, not the code. Go back to Step 2
33+
- **Repeat until the test fails specifically because of the bug**
34+
35+
## Step 4: Show Failure to User
36+
37+
- Show the user the test code and the failure output
38+
- Explicitly ask: "This test fails because of the bug. Can you confirm this captures the issue before I fix it?"
39+
- **STOP and WAIT for user acknowledgment before proceeding**
40+
- Do NOT continue to Step 5 until the user confirms
41+
42+
## Step 5: Fix the Bug
43+
44+
- Make the **minimum change** needed to fix the bug
45+
- Do not refactor, clean up, or "improve" surrounding code
46+
- Do not change the test
47+
48+
## Step 6: Run the Test — Confirm It PASSES
49+
50+
- Run the new test again
51+
- **Verify it PASSES**
52+
- If it fails: go back to Step 5 and adjust the fix
53+
- **Repeat until the test passes**
54+
55+
## Step 7: Run the Full Test Suite
56+
57+
- Run ALL tests to make sure nothing else broke
58+
- If other tests fail: fix the regression without breaking the new test
59+
- Report the final result to the user
60+
61+
## Rules
62+
63+
- NEVER fix the bug before the failing test is written and confirmed
64+
- NEVER skip asking the user to acknowledge the test failure
65+
- NEVER modify the test to make it pass — modify the source code
66+
- If you cannot write a test for the bug, explain why and ask the user how to proceed
67+
- Keep the fix minimal — one bug, one fix, one test

0 commit comments

Comments
 (0)