fred
efb3c3a10c
ci/woodpecker/pr/ci Pipeline failed
Three guides that existed only as one host's working copy, promoted to framework templates so every estate gets them. A working copy under ~/.mosaic binds one host; only a template here binds all of them. SEAT-IDENTITY.md (new) documents how a seat's git credential is actually resolved after #1311: identity from MOSAIC_GIT_IDENTITY, then mosaic.gitIdentity, then the stdin username; host mapped to a store prefix; then ONE of two stores chosen by whether the seat directory exists, with no precedence and no fallback between them. A seat with a directory and an empty slot fails closed rather than reaching the service store, and that is the point. It also corrects how to find the helper. credential.helper commonly names an absolute path, so `command -v git-credential-mosaic` answers a different question than the one git asks, and the two stop agreeing the moment the PATH copy is removed. Git also tries EVERY configured helper in order, so a fail-closed helper in front silently hands the request to whatever is configured behind it. The guide says to read the whole list. FLEET-COMMS.md (new) documents agent-send.sh: the class table, the addressing preamble, and the exit codes — including that rc=2 means the text reached the pane as an unsubmitted draft, so retrying double-sends it. Confirm with capture-pane instead. It also says to measure the fleet rather than trust roster.yaml, which on a live host was simultaneously naming a socket that did not exist, listing seats that were not running, and omitting seats that were. CODE-REVIEW.md gains an Evidence Discipline section: a green is not a result until you have shown it could go red, measurement and explanation are separate sentences, verify by content on the ref that ships rather than by ancestry of a local sha, and confidence is part of a finding. Plus four shell-measurement rules earned on #1311, each of which produced a wrong conclusion first — `cmd | tail; echo rc=$?` reports tail's status, a missed glob under pipefail exits 2 and kills the run under set -e, nonzero-with-no-output is an environment question before it is a code question, and `git -C` in a non-repo directory answers from the enclosing repo. The estate-specific repository exception that lived in the working copy is not carried here. The template says an estate may document one, scoped to a named repository and never precedent for a second. Both new guides are added to the two routing tables that agents read.
222 lines
9.6 KiB
Markdown
Executable File
222 lines
9.6 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). Both
|
|
ignore the resolved identity and attribute the record to `mos-dt-0`, and the read-back verifies
|
|
green, so the failure is invisible from inside the tool. 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, and reporting it as a pass is the most common way a review misses the defect it
|
|
was called for.
|
|
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
|
|
|
|
Earned on mosaicstack#1311, 2026-08-18. Each of these produced a wrong conclusion first.
|
|
|
|
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.
|
|
|
|
### 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
|