fred
09a1d9f7cd
ci/woodpecker/pr/ci Pipeline was canceled
Earned on #1313, where I reported three guides as failing the format gate and was wrong. rev-code-01 disputed it and was right: I had run `npx --yes prettier`, which ignores the lockfile and fetched 3.9.6. Measuring it out was more useful than conceding it. Same three files: 3.0.0 floor of the declared ^3.0.0 CI-CD-PIPELINES fails, other two pass 3.8.1 pinned by the lockfile all three pass, and this is CI's answer 3.9.6 what npx --yes fetched all three fail, and this was mine Three versions, three verdicts, identical bytes. Rule 13 says run the pinned tool. Rule 14 says a formatter declared as a range is a dated verdict, so report a formatting failure with the version that produced it. Worth recording separately: those three guides DO fail under 3.9.6, so they become a real format-gate failure the day the pin moves past 3.8.1. Not fixed here, and not in scope for a guides change. Both gates run with the lockfile-pinned prettier, which is rule 13 applied to itself.
228 lines
10 KiB
Markdown
Executable File
228 lines
10 KiB
Markdown
Executable File
# Code Review Guide
|
|
|
|
## Hard Requirement
|
|
|
|
If an agent modifies source code, code review is REQUIRED before completion.
|
|
Do not mark code-change tasks done until review is completed and blockers are resolved or explicitly tracked.
|
|
If code/config/API contract/auth behavior changed and required docs are missing, this is a BLOCKER.
|
|
If tests pass but acceptance criteria are not verified by situational evidence, this is a BLOCKER.
|
|
If implementation diverges from `docs/PRD.md` or `docs/PRD.json` without PRD updates, this is a BLOCKER.
|
|
|
|
Merge strategy enforcement (HARD RULE):
|
|
|
|
- PR target for delivery is `main`.
|
|
- Direct pushes to `main` are prohibited.
|
|
- Merge to `main` MUST be squash-only.
|
|
- Use `~/.config/mosaic/tools/git/pr-merge.sh -n {PR_NUMBER} -m squash --expect-head {approved_full_sha}` (or PowerShell equivalent).
|
|
|
|
An estate MAY carry a documented exception for a repository whose gates are commit hooks rather
|
|
than review. Such an exception belongs in that estate's own working copy of this guide, is
|
|
scoped to the named repository, and is never precedent for a second one.
|
|
|
|
**Do not use `pr-review.sh` or `issue-comment.sh` to post a verdict** (mosaicstack#1280). Post
|
|
through a direct authenticated API call as your own seat, or hand the verdict to the requesting
|
|
seat. Handing it over is a legitimate delivery path, not a fallback.
|
|
|
|
## Evidence Discipline (applies to every finding)
|
|
|
|
The checklist below says what to look at. This section says when you are allowed to believe what
|
|
you saw. Every rule here was earned by a wrong conclusion that reached a report.
|
|
|
|
1. **A finding is a claim about behavior.** State the failing input, the path taken, and the
|
|
wrong result. "This looks fragile" is not a finding.
|
|
2. **A green check is not a result until you have shown it could go red.** Run the control. A
|
|
`0`, an empty result, or a column of identical values with no failing counterpart is a
|
|
non-result.
|
|
3. **Measurement and explanation are separate sentences.** Report the command and its output,
|
|
then, as its own sentence, what you think it means.
|
|
4. **Never widen the case you measured.** If you checked one path, the finding covers one path.
|
|
5. **Reproduce a reported failure before recording it, and say which tree you measured.** Two
|
|
correct measurements of two different trees disagree without either being wrong.
|
|
6. **Verify by content on the ref that ships**, never by ancestry of a local sha. A rebase mints
|
|
new shas; a commit being an ancestor of something local proves nothing about the remote.
|
|
Compare by digest against `origin/<branch>`.
|
|
7. **Confidence is part of the finding.** "I could not reproduce this" is a usable review
|
|
comment. A confident guess is not.
|
|
8. **Author is not reviewer** (Gate-16). Do not review your own work, or work you shaped closely
|
|
enough to be a co-author of. Say so and hand it back.
|
|
|
|
### Measuring a shell suite
|
|
|
|
Each of these produced a wrong conclusion before it was written down.
|
|
|
|
9. **`cmd | tail; echo rc=$?` reports `tail`'s exit code, not `cmd`'s.** It reads as a pass when
|
|
the command failed. Redirect to a file and check `rc` directly, or use `${PIPESTATUS[0]}`.
|
|
10. **Under `set -o pipefail`, a missed glob makes `ls` exit 2**, the pipeline inherits it, and
|
|
`set -e` kills the run. Iterate a glob with a `for` loop and an `-e` test instead of piping
|
|
`ls`.
|
|
11. **A suite that exits nonzero with ZERO output is an environment question, not a defect in
|
|
the code under review.** The usual cause is a sourced dependency that is absent, so `set -e`
|
|
kills the first case before anything prints. Extract whole tool trees — `tools/git` alone is
|
|
missing `tools/_lib/credentials.sh`. Isolate the variable and prove it by adding only that
|
|
back.
|
|
12. **`git -C <dir>` in a directory that is not itself a repo answers from the enclosing repo.**
|
|
A scratch tree under `~/.mosaic` reports `~/.mosaic`'s HEAD, not the PR's, and every
|
|
conclusion drawn from it describes the wrong tree. Confirm `git rev-parse --show-toplevel`
|
|
is the tree you think it is before trusting any git output.
|
|
|
|
13. **Run the repository's PINNED tool version, not `npx --yes <tool>`.** `npx --yes` ignores the
|
|
lockfile and fetches the latest release, so it answers about a version the project does not
|
|
use. Measured on mosaicstack#1313: the lockfile pins prettier 3.8.1, under which three guides
|
|
pass; `npx --yes prettier` fetched 3.9.6, under which the same three fail; and 3.0.0, the floor
|
|
of the declared `^3.0.0` range, fails a different one. Three versions, three verdicts, same
|
|
bytes. Use `node_modules/.bin/<tool>`, or name the version the lockfile pins.
|
|
14. **A formatter or linter declared as a range is a dated verdict, not a fact.** If a lockfile
|
|
pins it, the gate is reproducible today and will disagree with itself the day the pin moves.
|
|
Report a formatting failure with the version that produced it, always.
|
|
|
|
### Feedback Categories
|
|
|
|
- **Blocker**: must fix before merge (security, bugs, test failures)
|
|
- **Should Fix**: important but not blocking (code quality, minor issues)
|
|
- **Suggestion**: optional improvement (style preference, nice-to-have)
|
|
- **Question**: seeking clarification
|
|
|
|
## Review Checklist
|
|
|
|
Reviewer seats split this checklist by class rather than duplicating it. A seat reviews its own
|
|
sections in full and may raise anything it notices outside them as a Suggestion, never as a
|
|
Blocker on someone else's ground.
|
|
|
|
| Reviewer class | Owns |
|
|
| ---------------- | ------------------------------------------------------------------------------------------------------- |
|
|
| `rev-code-*` | 1 Correctness, 3 Testing, 4 Code Quality, 4a TypeScript, 5 Documentation, 6 Performance, 7 Dependencies |
|
|
| `rev-security-*` | 2 Security, 2a OWASP |
|
|
|
|
Where two seats of the same class review the same change, they review independently and compare
|
|
after. A second seat that reads the first seat's findings before measuring is a proofreader, not
|
|
a second opinion.
|
|
|
|
### 1. Correctness
|
|
|
|
- [ ] Code does what the issue/PR description says
|
|
- [ ] Code aligns with active PRD requirements
|
|
- [ ] Acceptance criteria are mapped to concrete verification evidence
|
|
- [ ] Edge cases are handled
|
|
- [ ] Error conditions are managed properly
|
|
- [ ] No obvious bugs or logic errors
|
|
|
|
### 2. Security
|
|
|
|
- [ ] No hardcoded secrets or credentials
|
|
- [ ] Input validation at boundaries
|
|
- [ ] SQL injection prevention (parameterized queries)
|
|
- [ ] XSS prevention (output encoding)
|
|
- [ ] Authentication/authorization checks present
|
|
- [ ] Sensitive data not logged
|
|
- [ ] Secrets follow Vault structure (see `docs/vault-secrets-structure.md`)
|
|
|
|
### 2a. OWASP Coverage (Required)
|
|
|
|
- [ ] OWASP Top 10 categories were reviewed for change impact
|
|
- [ ] Access control checks verified on protected actions
|
|
- [ ] Cryptographic handling validated (keys, hashing, TLS assumptions)
|
|
- [ ] Injection risks reviewed for all untrusted inputs
|
|
- [ ] Security misconfiguration risks reviewed (headers, CORS, defaults)
|
|
- [ ] Dependency/component risk reviewed (known vulnerable components)
|
|
- [ ] Authentication/session flows reviewed for failure paths
|
|
- [ ] Logging/monitoring preserves detection without leaking sensitive data
|
|
|
|
### 3. Testing
|
|
|
|
- [ ] Tests exist for new functionality
|
|
- [ ] Tests cover happy path AND error cases
|
|
- [ ] Situational tests cover all impacted change surfaces (primary gate)
|
|
- [ ] Tests validate required behavior/outcomes, not only internal implementation details
|
|
- [ ] TDD was applied when required by `guides/QA-TESTING.md`
|
|
- [ ] Coverage meets 85% minimum
|
|
- [ ] Tests are readable and maintainable
|
|
- [ ] No flaky tests introduced
|
|
|
|
### 4. Code Quality
|
|
|
|
- [ ] Follows Google Style Guide for the language
|
|
- [ ] Functions are focused and reasonably sized
|
|
- [ ] No unnecessary complexity
|
|
- [ ] DRY - no significant duplication
|
|
- [ ] Clear naming for variables and functions
|
|
- [ ] No dead code or commented-out code
|
|
|
|
### 4a. TypeScript Strict Typing (see `TYPESCRIPT.md`)
|
|
|
|
- [ ] **NO `any` types** — explicit types required everywhere
|
|
- [ ] **NO lazy `unknown`** — only for error catches with immediate narrowing
|
|
- [ ] **Explicit return types** on all exported/public functions
|
|
- [ ] **Explicit parameter types** — never implicit any
|
|
- [ ] **No type assertions** (`as Type`) — use type guards instead
|
|
- [ ] **No non-null assertions** (`!`) — use proper null handling
|
|
- [ ] **Interfaces for objects** — not inline types
|
|
- [ ] **Discriminated unions** for variant types
|
|
- [ ] **DTO files used at boundaries** — module/API contracts are in `*.dto.ts`, not inline payload types
|
|
|
|
### 5. Documentation
|
|
|
|
- [ ] Complex logic has explanatory comments
|
|
- [ ] Required docs updated per `guides/DOCUMENTATION.md`
|
|
- [ ] Public APIs are documented
|
|
- [ ] Private/internal APIs are documented
|
|
- [ ] API input/output schemas are documented
|
|
- [ ] API permissions/auth requirements are documented
|
|
- [ ] Site map updates are present when navigation changed
|
|
- [ ] README updated if needed
|
|
- [ ] Breaking changes noted
|
|
|
|
### 6. Performance
|
|
|
|
- [ ] No obvious N+1 queries
|
|
- [ ] No blocking operations in hot paths
|
|
- [ ] Resource cleanup (connections, file handles)
|
|
- [ ] Reasonable memory usage
|
|
|
|
### 7. Dependencies
|
|
|
|
- [ ] No deprecated packages
|
|
- [ ] No unnecessary new dependencies
|
|
- [ ] Dependency versions pinned appropriately
|
|
|
|
## Review Process
|
|
|
|
Use `~/.config/mosaic/templates/docs/DOCUMENTATION-CHECKLIST.md` whenever code/API/auth/infra changes are present.
|
|
|
|
### Getting Context
|
|
|
|
```bash
|
|
# List the issue being addressed
|
|
~/.config/mosaic/tools/git/issue-list.sh -i {issue-number}
|
|
|
|
# View the changes
|
|
git diff main...HEAD
|
|
```
|
|
|
|
### Providing Feedback
|
|
|
|
- Be specific: point to exact lines/files
|
|
- Explain WHY something is problematic
|
|
- Suggest alternatives when possible
|
|
- Distinguish between blocking issues and suggestions
|
|
- Be constructive, not critical of the person
|
|
|
|
### Review Comment Format
|
|
|
|
```
|
|
[BLOCKER] Line 42: SQL injection vulnerability
|
|
The user input is directly interpolated into the query.
|
|
Use parameterized queries instead:
|
|
`db.query("SELECT * FROM users WHERE id = ?", [userId])`
|
|
|
|
[SUGGESTION] Line 78: Consider extracting to helper
|
|
This pattern appears in 3 places. A shared helper would reduce duplication.
|
|
```
|
|
|
|
## After Review
|
|
|
|
1. Update issue with review status
|
|
2. If changes requested, assign back to author
|
|
3. If approved, note approval in issue comments
|
|
4. For merges, ensure CI passes first
|
|
5. Merge PR to `main` with squash strategy only
|