Jason WoltjeandClaude Fable 5
18f960d3e1
ci/woodpecker/pr/ci Pipeline was successful
Per Jason's Option-A ruling on #1216: the constitution supplies the general framework; the project declares its own flow. Defines the integration trunk once in CONSTITUTION.md Hard Gates — declared by exactly one 'Integration trunk: <branch>' line in the project's root AGENTS.md, default 'main', value bound by git check-ref-format --branch semantics, malformed or multiple declarations are a hard stop, one trunk only, no gate relaxation — and binds gates 5/15, E2E-DELIVERY, CODE-REVIEW, ORCHESTRATOR(-PROTOCOL), BOOTSTRAP provisioning, and the defaults AGENTS session closure to that term. CI-CD-PIPELINES YAML examples stay on the default trunk with a substitution note. Known mechanized consumer left as tracked follow-up on #1216: pr-merge.sh:133 hardcodes a {main,next} base allowlist — fail-closed (refuses exotic trunks, cannot misroute), to be parameterized with red-first tests separately. Refs #1216 Co-Authored-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01WYgWocp36goy8hj2ui6ps1
156 lines
5.8 KiB
Markdown
Executable File
156 lines
5.8 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):
|
|
|
|
- The integration trunk is the branch the project's root `AGENTS.md` declares (default: `main`) — see `CONSTITUTION.md` Hard Gates.
|
|
- PR target for delivery is the integration trunk.
|
|
- Direct pushes to the integration trunk are prohibited.
|
|
- Merge to the integration trunk 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).
|
|
|
|
## Review Checklist
|
|
|
|
### 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 `~/.config/mosaic/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 `~/.config/mosaic/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 (diff against the integration trunk; default: main)
|
|
git diff {integration_trunk}...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
|
|
|
|
### Feedback Categories
|
|
|
|
- **Blocker**: Must fix before merge (security, bugs, test failures)
|
|
- **Should Fix**: Important but not blocking (code quality, minor issues)
|
|
- **Suggestion**: Optional improvements (style preferences, nice-to-haves)
|
|
- **Question**: Seeking clarification
|
|
|
|
### 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 the integration trunk with squash strategy only
|