Skip to content
Merged
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
9 changes: 5 additions & 4 deletions docs/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -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

Expand Down
44 changes: 44 additions & 0 deletions docs/review/README.md
Original file line number Diff line number Diff line change
@@ -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.
27 changes: 27 additions & 0 deletions docs/review/database.md
Original file line number Diff line number Diff line change
@@ -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.
25 changes: 25 additions & 0 deletions docs/review/docs-and-adr.md
Original file line number Diff line number Diff line change
@@ -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.
27 changes: 27 additions & 0 deletions docs/review/field-trial-report.md
Original file line number Diff line number Diff line change
@@ -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.
27 changes: 27 additions & 0 deletions docs/review/html-controller.md
Original file line number Diff line number Diff line change
@@ -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 `<input type="hidden" name="csrf_token" value="{$t_csrf_token}">`, 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.
25 changes: 25 additions & 0 deletions docs/review/openapi-contract.md
Original file line number Diff line number Diff line change
@@ -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.
23 changes: 23 additions & 0 deletions docs/review/release-ci.md
Original file line number Diff line number Diff line change
@@ -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.
Loading