Imported from rios0rios0/dev-toolkit (.github/skills/code-review/SKILL.md). Install upstream with npx skills add rios0rios0/dev-toolkit --skill code-review. Copyright stays with the author.

Code review — dev-toolkit

dev-toolkit clones, syncs, prunes, and bootstraps repositories across GitHub, Azure DevOps, GitLab, and Codeberg, and runs language-aware build/test/lint/SAST commands. It operates on the user's whole workspace in parallel, so anything that can discard uncommitted work is the first thing to look for.

When to use this skill

Use it whenever you are asked to review a pull request, a diff, a branch, or staged changes in this repository — and before opening a pull request of your own, as a self-check. It is a review skill: it produces findings, not commits.

Source of truth

The canonical engineering standards live in the rios0rios0/guide wiki. This file is a repo-tailored index into that guide plus the rules that only apply here. Precedence, highest first:

1. This repository's .github/copilot-instructions.md, CLAUDE.md, and CONTRIBUTING.md — they describe this codebase and its load-bearing invariants.
2. The rios0rios0/guide wiki — the shared standard.
3. General language idiom.

When the guide and a general convention disagree, the guide wins. When this file and the guide disagree, the guide wins and this file should be corrected in the same pull request.

Guide pages that apply here

How to run the review

1. Establish the range. Resolve the default branch with git symbolic-ref refs/remotes/origin/HEAD (strip refs/remotes/origin/; fall back to main), then read the diff with git diff <default>...HEAD and the file list with git diff <default>...HEAD --name-only.
2. Read whole files, not just hunks. A hunk cannot show a layering violation, a missing test, or a duplicated helper. Open every changed file in full, plus the files it imports from the layer below.
3. Check the change set as a unit — not only the code. A change that alters behaviour, configuration, or architecture is incomplete without its changelog entry and its documentation update, and that omission is a finding in its own right.
4. Map every finding to a rule. Each finding must name the rule it breaks and link the guide page (or the repository file) that states it. A comment that cannot be traced to a rule is a suggestion, not a defect — label it as such.
5. Report, do not rewrite. Produce the review in the output format below. Only edit files when the request explicitly asks for fixes.

What matters most in dev-toolkit

These are the checks that catch real defects in this repository. Work through them before the generic ones.

• Provider detection is map-based, never switch. That is an explicit design decision in this repository (Codeberg included). A new switch on provider name is a finding — see the Mapper Design Pattern.
• Dirty state must survive a sync. The WIP-branch mechanism preserves uncommitted work through temporary commits. Any path that checks out, resets, or prunes without going through it is a Critical finding — it destroys the user's work.
• Worktree guards always win over removal rules. The ordered guard/removal rule tables classify linked worktrees; locked, detached, dirty, and unpushed worktrees are guarded. Reordering the tables so a removal rule can fire first is a Critical finding.
• Parallelism is bounded. Goroutines run behind a semaphore channel; unbounded fan-out over a workspace of a hundred repositories will exhaust file descriptors and trip SSH rate limits. A new parallel path needs the same bound and a context that cancels.
• SSH preflight runs before batch cloning. Removing it turns one auth failure into a hundred hanging processes; any git subprocess needs GIT_SSH_COMMAND batch mode and a timeout.
• System commands are platform-gated through runtime.GOOS at registration time — a command that only makes sense on Linux must not appear in the Windows help.
• SAST orchestration isolates per-tool failure. One scanner failing must not abort the rest; the embedded default configs are the fallback when a repository has none.
• Tokens are read from GH_TOKEN, AZURE_DEVOPS_EXT_PAT, GITLAB_TOKEN, and CODEBERG_TOKEN; a token must never be interpolated into a URL that gets logged.
• All business logic accepts interfaces, and the doubles live in internal/testutil/.

Commands a reviewer should be able to quote

make build && make lint && make test && make sast
./bin/dev repo clone mine --dry-run ~/Development/github.com/rios0rios0
go fmt ./... && go vet ./...

Local quality gates

make lint, make test, and the SAST targets (make sast, or the per-tool targets such as make semgrep / make gitleaks where this repository's Makefile defines those instead) are the gate. They import the shared targets from rios0rios0/pipelines, which load the correct configuration before invoking each tool — so never invoke the tool binaries directly (golangci-lint, pytest, eslint, semgrep, trivy, hadolint, gitleaks). A change that adds a direct binary invocation to a script or workflow, or that edits a tool configuration to make a finding disappear, is itself a finding.

Architecture and layering

See Architecture and Backend Design.

Dependencies point inward, always. Infrastructure depends on the domain; the domain never imports infrastructure, a framework, a driver, or an SDK. The layer boundary is the first thing to check on any new file:

• An import of a concrete client, ORM, HTTP library, or cloud SDK from inside the domain layer is a Critical finding, no matter how convenient.
• Entities hold business rules and stay framework-agnostic — no serialisation tags, no ORM annotations, no transport concerns. Tags belong on DTOs in the infrastructure layer.
• New external dependencies enter through a port (an interface owned by the consumer) and an adapter that implements it. Code that reaches past the port to the concrete type breaks the pattern even when it compiles.
• Mappers isolate the layers: a repository mapper converts between models and entities, a controller mapper between requests/responses and entities. Leaking a transport DTO into the domain is a finding.
• Operation names come from the standard vocabulary: List, Get, Insert, Update, Delete, BatchInsert, BatchUpdate, BatchDelete, DeleteAll.

Go conventions

See Go Conventions, Go Type System, Go Logging, and Formatting and Linting.

• File names are snake_case — list_users_command.go, never listUsersCommand.go.
• Receivers are a one- or two-letter abbreviation of the type (c for Command, r for Repository, m for Mapper), consistent across every method of that type. Never self, this, or me.
• Naming patterns: <Operation><Entity>Command in <operation>_<entity>_command.go with a single Execute; <Entity>Repository in <entity>_repository.go for the port; <Library><Entity>Repository in <library>_<entity>_repository.go for the adapter. Repository methods read FindByX, FindAllByX, HasX, Save, SaveAll, DeleteByX.
• Accept interfaces, return structs, and keep interfaces small — a consumer that needs one method should not depend on a ten-method interface.
• Logging uses Logrus imported as logger, with structured fields (logger.WithFields(...)/WithError(...)). fmt.Println, println, and the standard log package are not application logging.
• Errors are wrapped with context: fmt.Errorf("…: %w", err). Never discard an error with _ without a comment explaining why it cannot matter.
• No any in exported signatures — use a constraint or a real interface. Every //nolint directive carries a justification comment.
• Entities carry no struct tags.

Dispatch tables over switch

See Mapper Design Pattern. Two or three stable cases may stay a switch. Four or more, or a set that grows with features, becomes a map from key to handler so that adding a case is a new entry rather than an edit to the dispatcher. Flag new switch/if-else chains that dispatch on a string or enum key.

YAML

See YAML Conventions. The extension is .yaml, never .yml. String values are single-quoted; double quotes appear only where interpolation or an escape needs them; booleans and numbers are never quoted. This applies to workflows, compose files, manifests, and YAML blocks inside Markdown.

Tests

See Tests.

• Stubs and builders for every interface live in internal/testutil/.
• A change to clone, sync, prune, or worktree classification needs a test for the guarded case, not only the happy path.

Rules that hold for every test in this repository

• BDD blocks are mandatory. Every test body carries // given / // when / // then (# given / # when / # then in Python) delimiting preconditions, the action, and the assertions. A test without them is a finding.
• Descriptions follow the layer. Commands: "should call <listener> when …". Controllers: "should respond <HTTP_STATUS> when …". Services and repositories: "should … when …", with at least one success and one failure case per public method.
• Mock libraries are banned — the category, not just the names: testify/mock, golang/mock, mockery, go-sqlmock, httpmock, gock, Mockito, EasyMock, PowerMock, unittest.mock, pytest-mock, responses, requests-mock, jest.mock() module mocking, sinon, nock, testdouble. Doubles are hand-rolled: stubs return canned answers, in-memory implementations hold state, and a spy that records calls is used only when nothing observable exists. Reaching for a mock library almost always means a port is missing — extract the interface the consumer needs instead.
• Driver-level and transport-level mocks are the worst case. SQL and HTTP behaviour is proven against the real thing: a real database with the real migrations for stores, a real local test server for outbound clients. Those are not mocks; they are real implementations under test control.
• Builders construct test data. Complex entities and DTOs come from builders, not from long literal structs repeated across files.
• A test deleted, skipped, or weakened to make a change pass is a Critical finding.

Documentation and change control

See Documentation & Change Control and CHANGELOG Formatting.

This repository uses chlog fragments. CHANGELOG.md is generated and is never edited by hand.

• Every change ships a fragment created with chlog new --kind <Kind> --body "…", staged in the same commit as the code. Kinds: Added, Changed, Deprecated, Removed, Fixed, Security.
• A backward-incompatible change to the public interface additionally carries --breaking. The kind alone never triggers a major bump.
• A hand-edited CHANGELOG.md, or a code change with no fragment under .changes/unreleased/, is a Critical finding — chlog check fails the build for it.
• Fragment bodies start with a lowercase verb in simple past tense, capitalise proper nouns (GitHub, Go, Docker), and wrap code identifiers and versions in backticks.
• README.md is updated whenever usage, setup, configuration, or architecture changes; .github/copilot-instructions.md and CLAUDE.md whenever the workflow, commands, or structure changes. Documentation and code ship in one commit.

Git Flow and pull-request hygiene

See Git Flow and Merge Guide.

• Branch names are feat/, fix/, refactor/, chore/, test/, or docs/ followed by a ticket ID or a short slug — feat/TICKET-000, fix/input-mask.
• Commit subjects are type(SCOPE): message: simple past tense (added, fixed, changed, removed), lowercase first word, no trailing period, code identifiers in backticks.
• Branches are synchronised with git rebase, never git merge. A merge commit from the default branch inside a feature branch is a finding.
• Breaking changes are flagged in three places: the commit footer (**BREAKING CHANGE:** …), the changelog, and the pull-request description. One or two of the three is not enough.
• Versions follow SemVer: MAJOR for incompatible changes, MINOR for features, PATCH for fixes.

Security

See Security.

• No hard-coded secrets. API keys, tokens, passwords, and private keys belong in environment variables or a secret manager — never in source, tests, fixtures, or the changelog. A secret that reaches a commit must be rotated, not merely deleted.
• Never write a PEM header sentinel or a realistic key shape into a fixture (GitHub, OpenAI, AWS, or Slack token prefixes, JWT-shaped strings, or the dashed BEGIN … banners). Gitleaks matches the shape, not the value, so a placeholder that merely looks like a credential fails the pipeline — spelling those prefixes out here would trip it too. Use inert placeholders such as fixture-token-placeholder.
• Suppressions must be justified. Entries in .gitleaksignore, .trivyignore, .semgrepignore, or .codeql-false-positives need a fingerprint, a dated comment, and a reason. A suppression added to silence a real finding is a Critical.
• Validate and sanitise every external input; use parameterised queries; apply least privilege; keep secrets out of logs.
• Dependency manifest changes are reviewed for new transitive vulnerabilities. When a fix exists, bump the version rather than suppressing the finding.

What not to flag

A review that raises noise gets ignored. Do not report these:

• The size of cmd/dev-toolkit/main.go — all CLI wiring lives there by design.
• Generated coverage and report artefacts in the working tree.
• Anything the guide does not require and this file does not list, unless it is a genuine correctness or security defect — say so plainly and label it a Suggestion.

Review output format

## Code review: <branch or PR>

### Critical (must fix before merge)
- `path/to/file.ext:LINE` — <what is wrong> — violates <rule> (<guide page or repo file>)

### Warning (should fix)
- `path/to/file.ext:LINE` — <what is wrong> — violates <rule>

### Suggestion (optional)
- `path/to/file.ext:LINE` — <improvement>

### Change-control checklist
- [ ] Changelog entry present for every behavioural change
- [ ] `README.md` updated if usage, setup, or architecture changed
- [ ] `.github/copilot-instructions.md` and `CLAUDE.md` updated if the workflow, commands, or structure changed
- [ ] Commit messages follow `type(SCOPE): message` in simple past tense
- [ ] Breaking changes flagged in the commit footer, the changelog, and the PR description

### Verdict: APPROVE / REQUEST CHANGES
<one paragraph: the blocking findings, or why the change is ready>

Severity

Rank findings most severe first, and state plainly when nothing blocks the merge — an empty Critical section is a valid, useful review.