Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
19 changes: 19 additions & 0 deletions .ai/checklists/README.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,19 @@
# Checklists

Cross-cutting gates. Skills carry their own domain checklists (a widget plugin's `Validation Checklist` belongs in the widget skill); these cover what no single skill owns.

| Checklist | Run it |
|---|---|
| `definition-of-done.md` | Before calling any change complete |
| `code-review.md` | When reviewing someone else's diff, or your own before opening a PR |
| `security.md` | When the change touches auth, input, scoped data, secrets, or file handling |
| `performance.md` | When the change adds a query, a loop over entities, or a page render path |
| `data-change.md` | When the change touches entities, migrations, settings, or persisted identities |
| `plugin-release.md` | Before shipping a plugin or theme |

## How to use

1. Run `definition-of-done.md` on every change. Add the situational ones the change triggers.
2. A checklist item is a question to answer, not a box to tick by reflex. "Yes, because …" is the passing answer.
3. Report items that do not apply as N/A with a reason. Silence reads as an unchecked box.
4. An item you cannot verify is a risk to state in the PR, not an item to skip.
107 changes: 107 additions & 0 deletions .ai/checklists/code-review.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,107 @@
# Checklist: Code Review

For reviewing a diff. Report only high-confidence findings, each with file, line, the rule violated, the failure it causes, and a minimal fix.

Use with `.ai/prompts/review-change.md`, which handles skill selection.

---

## Read the diff twice

- [ ] First pass: does the change do what its description claims?
- [ ] Second pass: what did it change that the description does not mention?

## Layering

- [ ] No business logic in a controller.
- [ ] No view model in a business service.
- [ ] No MongoDB driver type outside `Grand.Data`.
- [ ] No HTTP or `IWorkContext` dependency in `Grand.Domain`.
- [ ] No core → plugin reference.
- [ ] Services registered in `IStartupApplication`, not `Program.cs`.

## Scoping — the highest-yield section

- [ ] Every new query filters by store.
- [ ] Vendor-reachable code filters by `VendorId` and re-checks ownership on write.
- [ ] `LimitedToStores` / `LimitedToGroups` honoured where the entity supports them.
- [ ] Every new cache key contains store id — and language id if the data is localized.
- [ ] Settings loaded and saved with the same store scope.
- [ ] No `IWorkContext` in a scheduled task, migration, or plugin install.

## Duplication and reuse

- [ ] The logic does not already exist in a service, extension, or helper.
- [ ] A near-copy of an existing method was not introduced instead of a parameter.
- [ ] A third repetition of the same block was extracted rather than added.
- [ ] Copy-pasted code had **every** identifier updated — stale names from the source are a classic defect.

## Validation

- [ ] Inputs crossing a trust boundary are validated.
- [ ] Validators cover the new fields, not just the old ones.
- [ ] Guard clauses use `ArgumentNullException.ThrowIfNull`.
- [ ] Invalid model state does not partially save.

## Exception handling

- [ ] No empty `catch`, no `catch (Exception)` that continues silently.
- [ ] Expected business failures return results, not exceptions.
- [ ] Notification handlers cannot throw into the publisher.
- [ ] Migrations return `false` instead of throwing.
- [ ] Disposables are disposed.

## Logging

- [ ] Diagnosable failures are logged with store/entity/operation context.
- [ ] No secrets or personal data in log output.
- [ ] Log levels are proportionate.
- [ ] No logging in a hot loop.

## Async

- [ ] No `.Result`, `.Wait()`, `GetAwaiter().GetResult()`.
- [ ] No `async void`.
- [ ] No `Task.Run` wrapping synchronous work.
- [ ] `CancellationToken` forwarded where the surrounding signatures carry one.

## Data lifecycle

- [ ] Every write invalidates its cache prefix.
- [ ] Every write publishes its entity event.
- [ ] Cross-family caches that embed the entity are invalidated too.
- [ ] Migration `Identity` GUID is new.
- [ ] Migration is idempotent and destroys nothing operator-owned.
- [ ] No persisted identity renamed (plugin system name, permission name, template name, task name).

## Localization

- [ ] No hardcoded user-facing string.
- [ ] Resource keys follow the naming convention and are added **and** removed symmetrically in plugin install/uninstall.
- [ ] Localized entity properties read through the translation extension, not the raw property.

## Frontend

- [ ] Storefront data attributes preserved on touched views.
- [ ] Widget zones preserved.
- [ ] No `Html.Raw` on user content.
- [ ] Bundles rebuilt if source changed.

## Tests

- [ ] New behavior has tests, in the mirror project.
- [ ] Bug fixes have a regression test.
- [ ] No assertion weakened to make the suite pass.
- [ ] Tests mock at interface boundaries and hit no real database or network.

## Backward compatibility

- [ ] An existing installation upgrading in place still works.
- [ ] New settings default to previous behavior.
- [ ] No public interface, view model, or route changed without the PR saying so under "Breaking changes".

## Before submitting the review

- [ ] Every finding verified by opening the file — no speculative claims.
- [ ] Findings ranked by severity, not by file order.
- [ ] Style and preference comments omitted unless a mandatory rule is broken.
80 changes: 80 additions & 0 deletions .ai/checklists/data-change.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,80 @@
# Checklist: Data Change

Run when the change touches a domain entity, a migration, settings, localization resources, permissions, or any persisted identity.

The question behind every item: **what happens to a store that already has data and is running the previous version?**

---

## Entity changes

- [ ] New entity derives from `BaseEntity` (top-level) or `SubBaseEntity` (embedded).
- [ ] The marker interfaces the feature needs are implemented: `IStoreLinkEntity`, `IGroupLinkEntity`, `ISlugEntity`, `ITranslationEntity`.
- [ ] A new field has a default that makes existing documents behave as they did before.
- [ ] A removed or renamed field has a migration, or the old data is knowingly abandoned and the PR says so.
- [ ] No UI concern, persistence detail, or infrastructure dependency added to `Grand.Domain`.
- [ ] Naming matches `.ai/glossary/` — the domain word, not the nopCommerce word.

## Migration

- [ ] `Identity` is a freshly generated GUID, unique across the repository.
- [ ] `Version` matches the folder and the shipping release.
- [ ] `Priority` orders it correctly against the other migrations in that version.
- [ ] `UpgradeProcess` cannot throw — it catches and returns `false`.
- [ ] Running it twice is a no-op.
- [ ] It does not overwrite or delete anything an operator may have customized.
- [ ] A new version folder has its `MigrationUpgradeDbVersion_{version}` class.
- [ ] It does not read `IWorkContext` — there is no ambient context.

## Settings

- [ ] New settings class implements `ISettings`.
- [ ] Defaults preserve pre-upgrade behavior.
- [ ] A migration seeds the setting for existing installations.
- [ ] Store scope handled: loaded and saved with the same scope.
- [ ] System-wide fields are preserved when saving a store-scoped copy.
- [ ] `ClearCache()` called after saving.

## Localization

- [ ] Every new user-facing string has a resource key.
- [ ] Key naming follows `.ai/standards/naming.md`.
- [ ] Core resources ship through `App_Data/Resources/Upgrade/en_{version}.xml` plus an import migration.
- [ ] Plugin resources are added in `Install()` and **every one of them** removed in `Uninstall()`.
- [ ] Admin fields have both a label and a `.Hint` key.

## Permissions and navigation

- [ ] New permission registered in the `PermissionProvider`.
- [ ] Migration adds it for existing installations.
- [ ] Controllers enforce it via `[PermissionAuthorize]`.
- [ ] Admin sitemap entry added, with a migration, if the feature needs navigation.

## Persisted identities — never rename

- [ ] Plugin `SystemName` unchanged.
- [ ] Provider `SystemName` unchanged.
- [ ] `PermissionSystemName` unchanged.
- [ ] Message template name unchanged.
- [ ] `ScheduleTaskName` unchanged (and still equal to its DI key).
- [ ] Setting key unchanged.

Renaming any of these orphans data in every installation. If a rename is genuinely required, it needs a migration that moves the old records, and a "Breaking changes" entry.

## Caching and events

- [ ] Reads of the new data are cached with a key containing store id (and language id where localized).
- [ ] Every write invalidates the prefix.
- [ ] Every write publishes the entity event.
- [ ] Other cached families that embed this entity are invalidated.

## Indexes and query shape

- [ ] New query patterns are supported by an index, or the omission is deliberate and stated.
- [ ] No unbounded query over a collection that grows with orders or customers.

## Verification

- [ ] Tested against a database that already has data, not only a fresh install.
- [ ] The upgrade path was reasoned about explicitly, and the reasoning is in the PR.
- [ ] Rollback consequences stated: what an operator does if this change is wrong.
77 changes: 77 additions & 0 deletions .ai/checklists/definition-of-done.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
# Checklist: Definition of Done

Run on every change before reporting it complete.

---

## Scope

- [ ] The change does what was asked — no less.
- [ ] The change does **only** what was asked; unrelated refactors and formatting churn are out.
- [ ] Anything deliberately left out is stated explicitly, with the reason.

## Correctness

- [ ] The happy path was exercised, not just compiled.
- [ ] Boundary cases considered: empty collection, null, zero quantity, first page, last page.
- [ ] Store scope applied to every query and every cache key the change touches.
- [ ] Vendor scope applied where a vendor could reach the code.
- [ ] Behavior verified for the product types / order statuses / payment flows the change affects, or the untested ones named.

## Code quality

- [ ] No duplicated logic — the closest existing helper, service, or extension was reused. If similar code exists in three places now, that is a finding.
- [ ] Follows the closest existing file's structure, naming, and idiom.
- [ ] Naming matches `.ai/standards/naming.md` and the domain vocabulary in `.ai/glossary/`.
- [ ] No constraint from `.ai/constraints.md` violated.
- [ ] Dead code, commented-out code, and debug output removed.

## Validation

- [ ] Every input that crosses a trust boundary is validated — FluentValidation for models, guard clauses for service arguments.
- [ ] Invalid model state re-renders rather than partially saving.
- [ ] Server-side ownership re-checked for any id that arrived in a request.

## Exception handling

- [ ] Expected failures return result objects, not exceptions.
- [ ] No empty `catch`.
- [ ] Nothing catches broadly and continues as if it succeeded.
- [ ] Notification handlers and migrations cannot throw into their caller.
- [ ] Resources that need disposing are in `using` blocks.

## Logging

- [ ] Failures that an operator would need to diagnose are logged, with enough context to identify the store, entity, and operation.
- [ ] No secrets, tokens, passwords, card data, or full personal records in log messages.
- [ ] Log levels are honest: `Error` for something broken, `Warning` for something suspicious, not everything at `Information`.
- [ ] No logging inside a hot loop.

## Data safety

- [ ] Every write invalidates the affected cache prefixes.
- [ ] Every write publishes its entity event.
- [ ] New settings default to the pre-change behavior.
- [ ] New user-facing strings exist as localization resources.
- [ ] New permissions have a provider entry and a migration.
- [ ] An existing installation upgrading in place still works.

## Tests

- [ ] Tests added for new behavior, in the mirror test project.
- [ ] For a bug fix: a test that failed before the fix and passes after.
- [ ] No existing assertion weakened or deleted to make the suite pass.
- [ ] The affected test project runs green.

## Build and delivery

- [ ] The affected projects build.
- [ ] Frontend bundles rebuilt and committed if frontend source changed.
- [ ] Plugin/theme output path verified for Debug **and** Release, if applicable.
- [ ] PR follows `.ai/standards/git-and-pr.md`, with a truthful "Breaking changes" section.

## Reporting

- [ ] Commands actually run are listed, with results.
- [ ] Commands that could not be run are named.
- [ ] Remaining risk is stated plainly — not omitted because it is small.
65 changes: 65 additions & 0 deletions .ai/checklists/performance.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,65 @@
# Checklist: Performance

Run when the change adds a query, iterates over entities, touches a page-render path, or changes caching.

Complementary to `.ai/knowledge/performance.md` and `.ai/knowledge/caching.md`.

---

## Queries

- [ ] No query inside a loop. Fetch the set once and join in memory, or push the filter into the query.
- [ ] Filtering, sorting, and paging happen in the query — not after `ToList()`.
- [ ] The query projects the fields it needs when the entity is large and only a few are used.
- [ ] List endpoints are paged. An unbounded list over a growing collection is a future outage.
- [ ] Existence checks use a count or an any-style query, not "load everything and check `Count`".
- [ ] New query shapes are supported by an index, or the absence of one is stated deliberately.

## Writes

- [ ] Updating a few fields uses a partial update rather than rewriting the whole document.
- [ ] Bulk operations are batched rather than issued one document at a time.
- [ ] No read-modify-write loop that could be a single update.

## Caching

- [ ] Data that is read far more than written, and is expensive to produce, is cached.
- [ ] The cache key contains every variable that changes the result — store, language, currency, customer group, vendor, page.
- [ ] Cached values are invalidated on **every** write path, including delete.
- [ ] Cross-family caches that embed this data are invalidated too.
- [ ] Nothing customer-specific is cached under a key that omits the customer.
- [ ] `Clear()` is not used to fix a stale entry — it evicts every store's cache.
- [ ] Caching sits in the business service, not in a controller or handler.

## Render path

- [ ] No repository or service call from a Razor view.
- [ ] A view component that loads data is not invoked inside a loop over products.
- [ ] View-model preparation happens once in the handler, not per item.
- [ ] Images carry `loading` and dimension attributes as the surrounding views do.
- [ ] No new blocking external HTTP call on a page-render path. If one is unavoidable, it has a timeout and a fallback.

## Async and concurrency

- [ ] Nothing blocks on a `Task`.
- [ ] Independent awaits that could run concurrently are not serialized in a loop when the underlying calls are safe to parallelize.
- [ ] Long work is moved to a scheduled task rather than run inside a request.
- [ ] Notification handlers are fast — they run inline in the write path.

## Allocation

- [ ] No repeated string concatenation in a loop where a builder is available.
- [ ] Large collections are not copied repeatedly between list types.
- [ ] Nothing large is held in a singleton or static field.

## Scale assumptions

- [ ] The change was reasoned about with a realistic catalog: tens of thousands of products, not ten.
- [ ] Behavior under multiple stores considered — per-store caching multiplies memory.
- [ ] Behavior under multiple application instances considered — cache invalidation must propagate.

## Evidence

- [ ] Any optimization that costs readability is justified by a measurement, not intuition.
- [ ] The measurement, or its absence, is stated in the PR.
- [ ] Nothing was optimized speculatively at the expense of clarity.
Loading
Loading