From 561c9ce984dd3f0b6f8bcfe7b9845fde4923c69b Mon Sep 17 00:00:00 2001 From: Mark Shust Date: Fri, 1 May 2026 11:03:14 -0400 Subject: [PATCH] docs: expand PR review process with package-PR checklist and merge policy Updates `.claude/pr-review-process.md` based on lessons from PR #42 (marko/vite): - Adds package-PR-specific checks (5a): module.php is optional and minimal, composer.json conventions, full new-package file structure, cross-cutting updates (issue-template dropdowns, test-count bumps, required docs page at docs/src/content/docs/packages/{name}.md) - Adds explicit "loud errors / no silent fallbacks" guidance - Adds "no defensive instanceof checks on container resolutions" - Adds "no pseudo-functionality" review item - Notes that no PR CI workflow exists; the merge-readiness signal is local lint/tests + mergeStateStatus CLEAN - Adds step 7 for drafting PR comments before posting - Reaffirms --merge (not --squash) as the merge strategy and explains Co-Authored-By trailers go on maintainer fix commits, not the merge - Expands the pre-merge checklist with package-specific items Adds a "Reviewing PRs" section to CLAUDE.md that references the doc as lookup-on-demand (not @ imported) so it loads only when relevant. Expands the .claude/ configuration list to include all existing files. Co-Authored-By: Claude Opus 4.7 (1M context) --- .claude/pr-review-process.md | 162 ++++++++++++++++++++++++++++++++--- CLAUDE.md | 9 ++ 2 files changed, 159 insertions(+), 12 deletions(-) diff --git a/.claude/pr-review-process.md b/.claude/pr-review-process.md index a32e411d..09d5a9f6 100644 --- a/.claude/pr-review-process.md +++ b/.claude/pr-review-process.md @@ -2,11 +2,13 @@ Step-by-step process for reviewing, updating, and merging external pull requests. +When reviewing a PR, walk this entire document. The first review pass should be exhaustive — items missed cost a round-trip with the contributor. + ## 1. Fetch and understand the PR ```bash # Get PR details, related issues, and diff -gh pr view --json title,body,state,headRefName,baseRefName,author,commits +gh pr view --json title,body,state,headRefName,baseRefName,author,commits,maintainerCanModify gh pr diff # Check the related issue if referenced @@ -35,16 +37,23 @@ git rebase develop If there are conflicts, resolve them. If the rebase is clean, proceed. -## 4. Run the test suite +## 4. Run the test suite and lint ```bash -./vendor/bin/pest --parallel +composer test # full suite (excludes integration-destructive) +./vendor/bin/pest packages//tests # package-specific tests +./vendor/bin/phpcs --standard=phpcs.xml packages// # phpcs +./vendor/bin/php-cs-fixer fix packages// --dry-run # cs-fixer ``` -All tests must pass. If tests fail, determine whether failures are from the PR changes or pre-existing issues. +All tests must pass and lint must be clean. Fix ALL lint errors in touched files before merging, including pre-existing ones. + +**No PR CI workflow exists in this repo.** `gh pr checks ` will report no checks. The merge-readiness signal is local lint + tests passing plus `gh pr view --json mergeStateStatus,mergeable` returning `CLEAN`/`MERGEABLE`. There is no async CI to wait for — do not write "will merge once CI is green" in PR comments. ## 5. Review the code +### General code quality + Check each changed file against project standards: - **`declare(strict_types=1)`** in every PHP file @@ -53,10 +62,99 @@ Check each changed file against project standards: - **No hardcoded paths or environment-specific values** — use `PHP_BINARY`, env vars, etc. - **Tests exist** for new classes, bindings, and behavior - **Module bindings** have corresponding tests in `ModuleBindingsTest.php` -- **No `final` classes** (blocks Preferences extensibility) +- **No `final` classes**, including exceptions (blocks Preferences extensibility — see CLAUDE.md) - **Type declarations** on all parameters, returns, properties - **Compare with sibling packages** — if adding something to `database-mysql`, check how `database-pgsql` does it and ensure consistency +### Loud errors, no silent fallbacks + +Marko's "loud errors" principle (CLAUDE.md) means missing config, missing files, or invalid state should throw with a helpful message — not return defaults, empty strings, HTML comments, or silently degrade. Specifically reject: + +- `try { ... } catch (Throwable) { return $default; }` patterns that swallow exceptions +- Returning `` or empty strings for missing build artifacts (e.g., a Vite manifest) +- "Best-effort" fallbacks that hide configuration mistakes + +Throw a `MarkoException`-derived exception with `message`, `context`, and `suggestion` fields so the `errors` package can render a useful screen. + +### No defensive checks on container resolutions + +The container is type-guaranteed. After `$x = $container->get(FooInterface::class);`, do not write `if (! $x instanceof FooInterface) { throw ... }`. That's validation for scenarios that can't happen. Reference `packages/admin-auth/module.php` for the canonical binding pattern. + +### No pseudo-functionality + +Per CLAUDE.md core principles: do not build fake features to demonstrate concepts. Examples to flag: + +- An in-memory-only `flash()` that pretends to be session-backed +- Stub config that won't actually be read +- Methods that exist only to round out an interface but throw `not implemented` + +Either integrate with a real abstraction or remove the surface. + +## 5a. Package-PR-specific checks + +When the PR adds or modifies a package under `packages/`, also verify: + +### `module.php` is optional, and minimal when used + +`ManifestParser::parseModulePhp()` returns `[]` when the file doesn't exist. **Skip `module.php` entirely** if the package doesn't need: + +- Interface → implementation bindings (e.g., `CacheInterface::class => FileCacheDriver::class`) +- Shared singletons +- Custom factory closures +- A `boot` callback +- Explicit load ordering (`sequence: { after / before }`) + +For packages whose only public surface is a concrete class with a typed constructor, autowiring already works without `module.php`. + +When you do include it, keep it minimal. Defaults the framework already provides — do not re-state: + +- `enabled: true` is the default +- `sequence: { after: [...] }` — DI resolves lazily, so module load order rarely matters; only add when a `boot` callback or eager construction depends on another module's bindings +- **Redundant `bindings` entries.** List-style singletons (`X::class` as a value, not a key) trigger autowiring directly via `BindingRegistry::registerModule()` (`if (is_int($interface)) { $this->container->singleton($implementation); continue; }`), so this is redundant: + + ```php + // Don't write this: + 'bindings' => [X::class => X::class], + 'singletons' => [X::class], + + // List-style singleton alone is enough: + 'singletons' => [X::class], + ``` + + Only add a `bindings` closure when construction needs custom logic (factory call, conditional config, multi-step wiring). See `packages/view-latte/module.php` and `packages/authentication/module.php` for examples of when closures are warranted. + +### `composer.json` + +- `"type": "marko-module"` (or `library` only if genuinely a non-module library) +- **No hardcoded `"version"`** — Composer infers from the branch +- Internal Marko deps use `"marko/x": "self.version"` +- `marko/env` must be in `require` if `config/{name}.php` uses `env()` +- `marko/testing` in `require-dev` if tests use fakes +- **4-space indent** matching every other package's composer.json +- No package-local `scripts` block, no redundant dev deps for tooling that lives at the monorepo root (pest, phpstan, php-cs-fixer) +- `extra.marko.module: true` + +### File structure for a new package + +Required files: + +- `packages/{name}/composer.json` +- `packages/{name}/module.php` (only if needed — see above) +- `packages/{name}/LICENSE` (MIT, copyright Devtomic LLC) +- `packages/{name}/.gitattributes` (export-ignore tests/, .github/, etc. — copy from a sibling package and verify column alignment) +- `packages/{name}/README.md` — slim-pointer format per `docs/DOCS-STANDARDS.md`: title + one-liner, Installation, Quick Example, Documentation link +- `packages/{name}/src/...` (PSR-4 namespace `Marko\Name\`) +- `packages/{name}/tests/...` +- `packages/{name}/config/{name}.php` if configurable + +Cross-cutting updates a new package PR must include: + +- Root `composer.json`: add to `repositories` (path), `replace`, `autoload-dev` for `Marko\Name\Tests\` +- `.github/ISSUE_TEMPLATE/bug_report.yml` — add `{name}` to the package dropdown +- `.github/ISSUE_TEMPLATE/feature_request.yml` — add `{name}` to the package dropdown +- `tests/IntegrationVerificationTest.php` and `tests/PackagingTest.php` — bump `toHaveCount(N)` and the test descriptions +- **`docs/src/content/docs/packages/{name}.md`** — REQUIRED per `docs/DOCS-STANDARDS.md`. Every package needs a corresponding docs page (frontmatter title/description, intro, Installation, Configuration, Usage, Errors, API Reference, Related Packages). The `doc-updater` agent normally handles this in the post-implementation pipeline (`.claude/pipeline.md`), but for a contributor PR review, verify the page is present — do not assume it will be added later. + ## 6. Make fixes directly on the PR If the contributor enabled "Allow edits from maintainers" (`maintainerCanModify: true`), push fixes directly to their branch: @@ -68,7 +166,7 @@ gh pr view --json maintainerCanModify,headRepositoryOwner # Add their fork as a remote (one-time) git remote add git@github.com:/marko.git -# After making changes and committing +# After making changes and committing (use Co-Authored-By trailers for the contributor) git push # If rebased (history changed), force push is needed @@ -78,15 +176,26 @@ git push --force-with-lease=: -X PATCH -f body="$(cat <<'EOF' @@ -102,21 +211,50 @@ Add a section like: - Rebased onto current `develop` - Added missing tests for ... - Fixed lint issues in ... +- Added docs/src/content/docs/packages/{name}.md ``` ## 8. Merge -Once everything passes and the review is complete, merge via GitHub. Prefer the merge commit strategy to preserve the contributor's commit history. +**Use the merge commit strategy** (not squash) to preserve the contributor's commit history in the graph. Squash collapses the contributor's authorship into a single maintainer-authored commit with a `Co-Authored-By:` trailer — the `git log --graph` view loses their work. + +```bash +gh pr merge --merge --subject "Merge pull request # from /" --body "" +``` + +`Co-Authored-By:` trailers belong on maintainer fix commits (step 6), not the merge commit itself — the merge preserves contributor authorship through the graph topology. + +After merging: + +```bash +git checkout develop && git pull origin develop +git branch -D feature/ # clean up local branch (squash/merge usually shows as not-fully-merged) +git remote remove # optional: clean up the contributor remote +``` ## Checklist -Before merging any PR: +Before merging any package PR: - [ ] Issue is valid and the fix is correct - [ ] Branch is rebased onto current `develop` -- [ ] All tests pass (`./vendor/bin/pest --parallel`) +- [ ] `composer test` passes +- [ ] `./vendor/bin/phpcs` clean on touched files +- [ ] `./vendor/bin/php-cs-fixer fix --dry-run` clean on touched files - [ ] New code has corresponding tests - [ ] No hardcoded paths or environment-specific values -- [ ] Code follows project standards (strict types, type declarations, no final classes) +- [ ] No `final` classes (including exceptions) +- [ ] No defensive `instanceof` checks on container resolutions +- [ ] No silent fallbacks (HTML comments, swallowed exceptions, default returns on missing config) +- [ ] No pseudo-functionality (in-memory-only stubs pretending to be backed) +- [ ] Strict types, type declarations everywhere +- [ ] `module.php` is omitted entirely or kept minimal (no redundant `enabled`, `sequence`, or list-style binding entries) +- [ ] `composer.json`: `type: marko-module`, no `version`, `self.version` for internal deps, 4-space indent +- [ ] LICENSE, .gitattributes (aligned), slim README per DOCS-STANDARDS +- [ ] Issue template dropdowns updated (bug + feature) +- [ ] `IntegrationVerificationTest` and `PackagingTest` counts bumped +- [ ] `docs/src/content/docs/packages/{name}.md` exists and follows the package-page structure - [ ] PR description accurately reflects all changes +- [ ] PR comment drafted, reviewed, then posted +- [ ] Merge via `--merge` (preserves contributor commits), not `--squash` - [ ] Clean up any accidental branches pushed to origin diff --git a/CLAUDE.md b/CLAUDE.md index 628e3d2b..9b9da4c6 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -60,6 +60,10 @@ Use this workflow for new features, multi-file changes, or anything requiring mu @.claude/architecture.md +## Reviewing PRs + +When reviewing or merging a pull request, read `.claude/pr-review-process.md` first. It has the full review workflow, code-quality checklist, package-PR-specific checks (module.php minimalism, required docs page, composer.json conventions, cross-cutting updates), and the merge policy. Not auto-loaded — pull it into context only when actively reviewing. + ## Detailed Configuration Project configuration files are in `.claude/`: @@ -67,3 +71,8 @@ Project configuration files are in `.claude/`: - `architecture.md` — Technical patterns and directory structure - `testing.md` — Test configuration, TDD workflow, and patterns - `code-standards.md` — Coding conventions and style rules +- `pr-review-process.md` — PR review workflow, checklist, and merge policy +- `module-development.md` — Building new packages/modules +- `sibling-modules.md` — Naming and conventions for driver packages +- `pipeline.md` — Post-plan and post-implementation agent pipeline +- `release-process.md` — Release workflow