diff --git a/docs/CONTRIBUTING.md b/docs/CONTRIBUTING.md index 2330244..335bd23 100644 --- a/docs/CONTRIBUTING.md +++ b/docs/CONTRIBUTING.md @@ -8,10 +8,11 @@ NeNe development is Issue driven. Every meaningful code, documentation, dependen 2. Create a branch from `main`. 3. Make a focused change. 4. Run the relevant checks. -5. Commit with the repository commit convention. -6. Push and open a pull request. -7. Merge after review or verification. -8. Keep the Issue, milestone, TODO, roadmap, and ADRs aligned. +5. Walk the self-review checklists in `docs/review/` that match the scope of the PR (see [`docs/review/README.md`](review/README.md) for the mapping). +6. Commit with the repository commit convention. +7. Push and open a pull request. +8. Merge after review or verification. +9. Keep the Issue, milestone, TODO, roadmap, and ADRs aligned. ## Branch Names diff --git a/docs/review/README.md b/docs/review/README.md new file mode 100644 index 0000000..4e02cc8 --- /dev/null +++ b/docs/review/README.md @@ -0,0 +1,44 @@ +# Self-Review Checklists + +Use these checklists as the last step before opening a PR. Each list points at the policy docs that own the rules; the checklists do not duplicate policy text, they remind you to check it. + +The format is borrowed from the sibling project NENE2's `docs/review/` and adapted to NeNe's own framework shape (session + CSRF auth, Smarty HTML, MySQL/SQLite parity, OpenAPI generic envelope, field-trial methodology). + +## When to use which + +| Checklist | Use for | +| --- | --- | +| [`rest-controller.md`](rest-controller.md) | New or modified REST handlers (`indexGetRest`, `indexPostRest`, etc.), API error catalog updates, REST auth/CSRF changes. | +| [`html-controller.md`](html-controller.md) | New or modified HTML pages (`actionAction`), Smarty templates, asset auto-discovery files, form POST handling. | +| [`database.md`](database.md) | Schema additions, mapper changes, MySQL/SQLite parity edits, transaction boundaries. | +| [`openapi-contract.md`](openapi-contract.md) | Anything that touches `docs/api/openapi.yaml` or the contract test. | +| [`docs-and-adr.md`](docs-and-adr.md) | Policy doc edits, ADR additions, roadmap / milestones / TODO changes. | +| [`release-ci.md`](release-ci.md) | `.github/workflows/`, branch protection, auto-merge config, dependency upgrades. | +| [`field-trial-report.md`](field-trial-report.md) | A new `docs/field-trials/YYYY-MM-field-trial-N.md` or its follow-up Issues. | + +A single PR may invoke more than one checklist (a REST endpoint usually triggers `rest-controller.md` + `openapi-contract.md` + `database.md`). Reference the checklist(s) you used in the PR body. + +## How to use them + +1. Open the relevant checklist before pushing the PR. +2. Walk every item. If an item does not apply, say so in the PR body — silence reads as "I didn't check". +3. If a checklist item conflicts with the source policy, fix the checklist (or escalate via ADR) — checklists are reminders, never a second source of truth. + +## Policy references + +The checklists link back to the canonical policy docs: + +- `docs/workflow.md` — Issue / branch / PR lifecycle +- `docs/CONTRIBUTING.md` — scope control, branch naming, safety +- `docs/development/coding-standards.md` — code-level conventions +- `docs/development/commit-conventions.md` — Conventional Commits style +- `docs/development/testing.md` — unit + HTTP runtime smoke tests +- `docs/development/docker.md` — local environment + env vars +- `docs/development/error-codes.md` — error catalog (post ADR-0003) +- `docs/api/README.md` — OpenAPI policy +- `docs/api/reference-client.md` — external consumer mechanics +- `docs/tutorials/building-a-service.md` — full tutorial flow +- `docs/field-trials/README.md` — FT methodology (ADR-0002) +- `docs/adr/` — major decisions (0001–0004 currently) + +If a checklist item refers to "the policy" without a link, that is a bug; file an Issue. diff --git a/docs/review/database.md b/docs/review/database.md new file mode 100644 index 0000000..46531a5 --- /dev/null +++ b/docs/review/database.md @@ -0,0 +1,27 @@ +# Database Self-Review + +Use this checklist for schema additions, mapper changes, MySQL ↔ SQLite parity edits, transactions, seed data, and per-entity model updates. + +Source policies: + +- `docs/development/coding-standards.md` (data mapper conventions, junction-table guidance from PR #239) +- `docs/development/docker.md` (Schema Parity Between SQLite and MySQL section, PR #240) +- `docs/tutorials/building-a-service.md` (sections "Add Database Code", "Add Database Transactions") + +## Checklist + +- [ ] Schema changes are mirrored in **both** `docker/mysql/init/001_schema.sql` **and** `cli/initSQLite.php`. The two files do not share a source of truth; CI does not enforce parity. +- [ ] SQLite path includes an `..._updated_at_trigger` to mirror MySQL's `ON UPDATE CURRENT_TIMESTAMP`. +- [ ] Foreign keys cascade-delete or restrict deliberately; the choice matches the entity lifecycle. +- [ ] Per-user tables expose `user_id` as a `BIGINT UNSIGNED` FK to `users.id`, indexed (`KEY ..._user_id_index`). +- [ ] Soft delete (`is_deleted TINYINT(1)`) is the default; physical delete is justified per entity. +- [ ] Unique constraints on soft-deleted columns have an explicit policy (FT2 follow-ups entry in `docs/field-trials/follow-ups.md` documents the open trade-off — choose namespacing-in-tests, or partial-unique-index, deliberately). +- [ ] Model class under `class/db/` declares `$schema` and `$validation` matching the table; field types use `DataModelBase` constants (`INTEGER`, `STRING`, `BOOLEAN`, `DATETIME`). +- [ ] Mapper extends `DataMapperBase` with `MODEL_CLASS`, `TARGET_TABLE`, `KEY_SID` constants set. +- [ ] Per-user finders take `int $userId` as the first parameter (`findRowsByUserId`, `findRowByUserIdAndId`, `createForUser`, `updateForUser`, `deleteForUser`) and bind it as `PDO::PARAM_INT`. +- [ ] Junction tables (M:N) use raw prepared statements (`DataMapperBase` composite-PK assumption breaks for them; see PR #239 / FT2 F-6 guidance in `docs/development/coding-standards.md`). +- [ ] Multi-statement writes wrap in `TransactionManager::run()`; domain validation runs before opening the transaction. Throwing `DomainException` from inside is allowed (PR #244 / ADR if introduced) and is mapped to JSON 4xx by `htdocs/index.php`. +- [ ] Seed data is idempotent (`INSERT ... WHERE NOT EXISTS` or equivalent) so re-running init does not duplicate rows. +- [ ] Test setup uses direct SQL cleanup (`UPDATE table SET is_deleted = 1` via `PdoConnection::getInstance()->exec(...)`) when the entity has no destructive HTTP endpoint; call `Initialize::init()` once per test class before touching framework PDO directly. +- [ ] `composer test` and `composer test:http` both pass after schema recreate (`docker compose down -v && docker compose up -d app`). +- [ ] PR body lists this checklist when schema or mappers change. diff --git a/docs/review/docs-and-adr.md b/docs/review/docs-and-adr.md new file mode 100644 index 0000000..2a8210b --- /dev/null +++ b/docs/review/docs-and-adr.md @@ -0,0 +1,25 @@ +# Docs and ADR Self-Review + +Use this checklist for policy docs, ADR additions, roadmap / milestones / TODO updates, and the `docs/field-trials/follow-ups.md` file. + +Source policies: + +- `docs/workflow.md` +- `docs/CONTRIBUTING.md` +- `docs/adr/README.md` +- `docs/adr/0001-record-architecture-decisions.md` +- `docs/development/commit-conventions.md` + +## Checklist + +- [ ] The source-of-truth policy doc was updated instead of adding a summary in a tutorial or README that drifts. +- [ ] When project state changes (next FT theme, an Issue closes a milestone, etc.), `docs/roadmap.md`, `docs/milestones/`, and/or `docs/todo/current.md` are updated in the same PR. +- [ ] Major architecture / public contract / dependency / release-policy decisions are recorded as ADRs under `docs/adr/` and numbered sequentially. +- [ ] ADRs use the `Status / Context / Decision / Consequences / Related` shape from ADR 0001. +- [ ] Cross-links between docs use relative markdown links (`[..](../api/openapi.yaml)`), not absolute URLs. +- [ ] `docs/field-trials/follow-ups.md` is updated when a deferred FT finding is escalated to an Issue and removed when its spawning Issue is merged (per the file's own rules). +- [ ] `docs/field-trials/README.md` is the methodology source of truth; per-trial details belong in the trial report, not in README. +- [ ] Tutorial examples use the same convention names the framework actually uses (e.g. `privatenote` not `private-note` per PR #286). +- [ ] If a checklist in `docs/review/` is touched, the linked source policy is checked for the corresponding change. +- [ ] Markdown renders without broken links or table glitches (`grep -nE 'broken\\|\\\\|\\(\\)' docs/...md` for hand-typed tables). +- [ ] PR body lists this checklist when docs / ADRs change. diff --git a/docs/review/field-trial-report.md b/docs/review/field-trial-report.md new file mode 100644 index 0000000..544101b --- /dev/null +++ b/docs/review/field-trial-report.md @@ -0,0 +1,27 @@ +# Field Trial Report Self-Review + +Use this checklist when adding a new `docs/field-trials/YYYY-MM-field-trial-N.md` report or its follow-up Issues. Also use it when editing `docs/field-trials/follow-ups.md`. + +Source policies: + +- `docs/field-trials/README.md` (methodology, friction-kind taxonomy, decision taxonomy) +- `docs/templates/field-trial-report.md` (report skeleton) +- `docs/adr/0002-adopt-field-trial-methodology.md` +- `docs/field-trials/follow-ups.md` (deferred-finding index rules) + +## Checklist + +- [ ] Report filename matches `YYYY-MM-field-trial-N.md` with `N` monotonically increasing from the previous trial. +- [ ] The companion trial clone lives under `../NeNe-FT/ft{N}-{topic}/` and was created via `tools/nene-ft-new.sh {topic}` (which now sanity-checks FRAMEWORK_ROOT — PR #283). +- [ ] Report includes the standard sections: Date, Baseline, Goal, Service Built, Steps Taken, Results, Friction Summary, Recommendations, Overall Impression, Follow-up Issues, Reminder. Skeleton at `docs/templates/field-trial-report.md`. +- [ ] Baseline section pins the NeNe ref (commit SHA) the clone was based on and lists the previous trials' improvements that were verified. +- [ ] Every finding has a stable `F-N` identifier referenced both inline and in the Friction Summary table. +- [ ] Friction Summary rows include severity (`high` / `medium` / `low`), kind (`docs-gap` / `feature-gap` / `design-trade-off` / `legacy-preserved` / `process-gap`), and decision (`fix-in-framework` / `document` / `defer` / `keep-legacy`). +- [ ] Hypotheses (H-A, H-B, ...) recorded at trial start get an outcome line in the report — explicit "no, unfired" entries are useful signal too. +- [ ] Positive findings (things that worked first-try) are recorded with the same `F-N` shape (kind `n/a (positive)`, decision `no action`). +- [ ] Each non-deferred finding has a focused GitHub Issue filed and referenced from the "Follow-up Issues" section. +- [ ] Deferred findings (decision = `defer`) are appended to `docs/field-trials/follow-ups.md` with a "Re-evaluation trigger" line. +- [ ] When a deferred entry's trigger fires in a later trial, it is escalated to an Issue and removed from `follow-ups.md` in the same PR that introduces the new trial report (or its first follow-up PR). +- [ ] Trial Issue (e.g. `#274`) is closed by the report PR (`Closes #N` in commit / PR body). +- [ ] Trial clone settings (`.claude/settings.local.json` + `.claude/CLAUDE.md`) and `FT{N}-PLAN.md` are **not** committed back to the framework repo. Only the final report ships. +- [ ] PR body lists this checklist. diff --git a/docs/review/html-controller.md b/docs/review/html-controller.md new file mode 100644 index 0000000..24f19d3 --- /dev/null +++ b/docs/review/html-controller.md @@ -0,0 +1,27 @@ +# HTML Controller Self-Review + +Use this checklist for server-rendered pages — new `actionAction()` handlers, Smarty templates, asset auto-discovery files, form POST handling, HTML login / logout flows. + +Source policies: + +- `docs/tutorials/building-a-service.md` (sections "Add a Fixed Page", "Handle a Form POST", "Protect an Authenticated Form", "Add an HTML Login Form") +- `docs/frontend/assets.md` (template, CSS, JS auto-discovery; Smarty escape behavior) +- `docs/development/coding-standards.md` + +## Checklist + +- [ ] Controller URL segment is a **single lowercase word** (`note`, `privatenote`), not kebab-case. The dispatcher's `ucfirst(strtolower(...))` cannot form a class name with hyphens. +- [ ] HTML actions use the `actionAction()` shape (e.g. `indexAction`, `itemAction`). Method-specific handlers (`indexPostRest`) are **not** added for HTML pages — let the dispatcher fall through to `actionAction` and branch on `$this->method` internally. +- [ ] Side-effect actions (`createAction`, `deleteAction`, `logoutAction`, ...) guard with `$this->method !== 'POST'` before performing any write; GET on a side-effect URL must not trigger the side effect. +- [ ] Form POST handlers read input via `$this->request->getPost($key)`, not `$_POST` directly. +- [ ] Validation failure re-renders the same form using `$this->VIEW->setTemplate('xxx/yyy.tpl')` (the auto-template would be the list, not the form). +- [ ] Authenticated state-changing forms use the CSRF helpers: controller sets `t_csrf_token` via `$this->csrfToken()`, template emits ``, handler calls `$this->verifyCsrfFromPost()` before the write. +- [ ] Post-write redirects use `$this->location('/path')` (the post/redirect/get pattern); URI normalization is handled by `location()` (no leading-slash double-up — see PR #269). +- [ ] `SESSION_CHECK = false` is set in `preAction()` only for public pages (`/auth/login` etc.). Protected pages keep the default. +- [ ] Per-controller redirect targets (e.g. admin pages → `/admin/login`) use the `unauthorizedRedirect()` hook (ADR-0004), not by editing `LOGOUT_URI` or re-implementing `sessionCheck()`. +- [ ] Templates `{extends file='layout/app.tpl'}` and define their content inside `{block name='content'}`. +- [ ] Smarty auto-escapes `{$variable}` (`setEscapeHtml(true)` is on framework-wide). Markup-emitting modifiers like `|nl2br` need `nofilter`, or use CSS `white-space: pre-line` for line breaks. See `docs/frontend/assets.md` "Smarty HTML Escaping". +- [ ] Per-action / per-controller CSS / JS files live under `htdocs/css/{controller}/{action}.css` etc. and are auto-discovered. Manual `addCSS()` / `addJS()` only for CDN URLs or files outside the convention. +- [ ] A focused HTTP smoke test under `tests/Http/` exercises the happy path + at least one failure mode (e.g. unauth redirect, missing CSRF). +- [ ] `composer test` and `composer test:http` both pass. +- [ ] PR body lists this checklist. diff --git a/docs/review/openapi-contract.md b/docs/review/openapi-contract.md new file mode 100644 index 0000000..d47802e --- /dev/null +++ b/docs/review/openapi-contract.md @@ -0,0 +1,25 @@ +# OpenAPI Contract Self-Review + +Use this checklist for any change to `docs/api/openapi.yaml`, the contract test (`tests/Http/OpenApiRuntimeContractTest.php`), or related schema / response components. + +Source policies: + +- `docs/api/README.md` +- `docs/development/error-codes.md` +- `docs/adr/0003-openapi-failure-envelope-shape.md` +- `docs/tutorials/building-a-service.md` (section "Update OpenAPI") + +## Checklist + +- [ ] Every new public REST operation appears in `docs/api/openapi.yaml` with `tags`, `summary`, `operationId`, request body schema (if any), and a response per documented HTTP status. +- [ ] Failure responses reference the canonical **generic** `ApiFailureEnvelope` schema (ADR-0003). No per-error-code envelope schemas are added; preserved-per-endpoint specificity goes into `examples:` showing the actual `errorCode` value. +- [ ] Path parameters use NeNe's `/path/item/id_{id}` form and document a `parameter` referenced from `#/components/parameters/...` (e.g. `TodoId`, `MemoId`). +- [ ] Security requirements (`sessionCookie` / `csrfToken`) match the actual gate behavior. `GET` operations only declare `sessionCookie`; state-changing operations declare both. +- [ ] `requestBody.content['application/json'].examples` includes at least one example. The runtime contract test (`OpenApiRuntimeContractTest`) auto-discovers operations and uses the first example as the probe body. +- [ ] Reusable responses (`#/components/responses/MethodNotAllowed`, `#/components/responses/TodoNotFound`, ...) are used where multiple operations share the same response shape. +- [ ] New error codes are added to `config/error_codes.php` **and** `docs/development/error-codes.md` table; OpenAPI references them via `examples` only. +- [ ] Operations with destructive side effects that should not be auto-probed by the runtime contract test set `x-nene-runtime-probe: skip` (currently unused — first use will validate the opt-out path). +- [ ] `symfony/yaml` parses the modified file (`docker compose exec -T app php -r 'Symfony\Component\Yaml\Yaml::parseFile("docs/api/openapi.yaml");'` returns without exception). +- [ ] `composer test:http` passes; the contract test exercises all documented operations. +- [ ] Swagger UI at `http://localhost:8080/api-docs/` renders without errors after the change. +- [ ] PR body lists this checklist when OpenAPI changes. diff --git a/docs/review/release-ci.md b/docs/review/release-ci.md new file mode 100644 index 0000000..9407f67 --- /dev/null +++ b/docs/review/release-ci.md @@ -0,0 +1,23 @@ +# Release and CI Self-Review + +Use this checklist for changes to `.github/workflows/`, branch protection, auto-merge config, dependency upgrades, or version tagging. + +Source policies: + +- `.github/workflows/tests.yml` +- `docs/development/docker.md` (CI-relevant env vars) +- `docs/development/commit-conventions.md` + +## Checklist + +- [ ] CI changes start from a documented local command before becoming required (e.g. `composer test`, `composer test:http`). +- [ ] The `unit` and `HTTP runtime smoke (Docker)` jobs both pass on this PR before merge. +- [ ] Required status checks on `main` branch protection match the actual job names; renaming a job in `.github/workflows/tests.yml` requires updating the branch protection rule via `gh api repos/.../branches/main/protection`. +- [ ] The "Wait for /health" step uses `healthStatus=ok` (not just HTTP 200) as the readiness signal (PR #259). +- [ ] Health-wait timeout is at least 90s for the cold-start path (PR #288). Reducing the budget requires verifying tail-end runner behavior. +- [ ] When CI fails on "Wait for /health", container logs were inspected for PHP fatals or missing dependencies **before** assuming a timing issue (see `feedback-ci-debugging-discipline` memory: an old `Constant expression contains invalid operations` masqueraded as a timeout in PR #284). +- [ ] Dependency upgrades touch `composer.json` and `composer.lock` in the same commit; the lockfile is committed. +- [ ] Branch protection on `main` continues to require status checks to pass (`gh api repos/.../branches/main/protection` confirms required contexts). +- [ ] Auto-merge (`gh pr merge --auto --merge --delete-branch`) is used for routine improvement-loop PRs; never set `--no-verify` or `--force` on shared branches. +- [ ] `.github/workflows/` YAML parses (e.g. via `actionlint` if locally available); secrets and tokens are not hardcoded. +- [ ] PR body lists this checklist when workflow or CI infra changes. diff --git a/docs/review/rest-controller.md b/docs/review/rest-controller.md new file mode 100644 index 0000000..98bb7e9 --- /dev/null +++ b/docs/review/rest-controller.md @@ -0,0 +1,27 @@ +# REST Controller Self-Review + +Use this checklist for new or modified REST handlers (`indexGetRest`, `indexPostRest`, `itemPatchRest`, `itemDeleteRest`, ...), API error catalog additions, and changes to the REST auth / CSRF path. + +Source policies: + +- `docs/tutorials/building-a-service.md` (sections "Add a REST Endpoint" and onward) +- `docs/development/coding-standards.md` +- `docs/development/error-codes.md` +- `docs/api/README.md` +- `docs/api/reference-client.md` + +## Checklist + +- [ ] Method-specific handler names (`indexGetRest`, `indexPostRest`, etc.) are used — no new `indexRest` legacy-fallback handlers. +- [ ] State-changing methods (`POST` / `PUT` / `PATCH` / `DELETE`) rely on the framework's automatic CSRF gate when the user is logged in; no per-handler CSRF re-implementation. +- [ ] `SESSION_CHECK` is left at the default `true` for protected endpoints; only set to `false` in `preAction()` for public endpoints with a documented reason. +- [ ] Success responses use `$this->API_RESPONSE->success([...])` with a stable `Data` shape. +- [ ] Failure responses use `$this->API_RESPONSE->failure('ERROR-CODE')` with a code that exists in `config/error_codes.php`. +- [ ] New error codes are added to `config/error_codes.php` **and** documented in `docs/development/error-codes.md` (catalog table). +- [ ] Per-error envelope schemas are **not** added to `docs/api/openapi.yaml`; the contract uses the canonical generic `ApiFailureEnvelope` (ADR-0003) with per-endpoint `examples` showing specific codes. +- [ ] Path parameters use the NeNe `id_X` URL form (e.g. `/todo/item/id_1`) and are extracted via `$this->request->getParam('id')`; reject non-numeric ids with `XXX-ID-REQUIRED`. +- [ ] Mappers are scoped per user where the data is per-user (e.g. `findRowsByUserId($this->AUTH_SESSION->userId())`); never expose another user's rows. +- [ ] `TransactionManager` wraps multi-statement or multi-mapper writes; domain validation runs **outside** the transaction (escape via `DomainException` if it must throw from within — see PR #244). +- [ ] OpenAPI documents the new operation: path, method, request body schema, success envelope schema, failure status codes referencing `ApiFailureEnvelope` with example `errorCode`. +- [ ] `composer test` and `composer test:http` both pass. The runtime contract test (`OpenApiRuntimeContractTest`) auto-discovers the new operation; no manual probe tuple to add. +- [ ] PR body lists this checklist and the per-entity HTTP smoke test added under `tests/Http/`. diff --git a/docs/workflow.md b/docs/workflow.md index 1460f9d..48f7406 100644 --- a/docs/workflow.md +++ b/docs/workflow.md @@ -9,11 +9,18 @@ NeNe uses an Issue driven workflow. 3. Create a topic branch from `main`. 4. Implement only the Issue scope. 5. Run checks and record the results in the PR. -6. Commit using the commit convention. -7. Push and open a PR. -8. Reference the Issue from the PR body. -9. Merge after review or verification. -10. Close or update the Issue. +6. Walk the relevant self-review checklists under `docs/review/` (see below). +7. Commit using the commit convention. +8. Push and open a PR. +9. Reference the Issue from the PR body. +10. Merge after review or verification. +11. Close or update the Issue. + +## Self-Review Checklists + +Before pushing, walk the checklists in `docs/review/` that match the scope of the change. The list at `docs/review/README.md` maps PR types to checklists (REST controller, HTML controller, database, OpenAPI, docs/ADR, release/CI, field trial report). A single PR may invoke more than one. Reference the checklist(s) you used in the PR body. + +Checklists are reminders to consult the source policy docs — they do not duplicate policy text. If a checklist item conflicts with the canonical policy, fix the checklist (or escalate via ADR). ## Pull Request Requirements