Workflow, PR, code review

Что отвечать

An atomic commit is one commit that equals one logically complete change. You can describe it in a single sentence, and it **compiles and passes tests on its own**. Not a "WIP", not "fix typo + add feature + reformat". Why it matters: `git bisect` works only when every commit builds. Reverting one logical change becomes a single command. Review can understand "what exactly this commit did".

Что хотят услышать

A senior should: - name three criteria: one change, describable in a single phrase, builds and passes tests at its own point in history - explain the link to bisect: if commits are "bundled", bisect finds the commit but cannot tell what inside it is to blame - say that atomic does not mean "small": a 500-line commit can be atomic (a whole DB migration), and a 5-line one may not be (fix typo + add unused import) - name the discipline tools: `git add -p` to stage in chunks, `git commit --fixup=<sha>` for a targeted correction, interactive rebase to split a large commit into parts - mention that on teams with squash-merge atomicity is lost on main, but still matters for review before the merge

Подводные камни

• ✗ Saying "atomic means a small commit". Size is not the criterion, the criterion is "one logical change"
• ✗ Making a "refactor + new feature" commit. A revert later then needs to be rewritten by hand
• ✗ Skipping `git add -p` for speed and cramming edits to several unrelated files into one commit

Follow-up

• ? How do you split a large commit into a series of atomic ones with interactive rebase?
• ? How does `git add -p` help you make atomic commits?
• ? What happens to `git bisect` in a repository with broken commits?

Что отвечать

The format is `<type>(<scope>): <subject>` where type comes from a fixed list: feat, fix, docs, refactor, test, chore, perf, ci, build. It gives you three things: an automatic changelog (`changelogen`, `semantic-release`, `release-please` parse commits and build release notes), an automatic semver bump (feat = minor, fix = patch, `BREAKING CHANGE:` in the body = major), and a uniform language in the history. A reviewer sees the type of change right away.

Что хотят услышать

A senior should: - name the link with semver: feat → minor, fix → patch, BREAKING CHANGE → major, and the tools that automate it - say that conventional commits work without automation too, simply as discipline for reading the history - mention commitlint + husky as a way to enforce the format on a local pre-commit hook, or a GitHub Action on the PR title - say that with squash-merge the format must be in the PR title (that is what lands on main as the commit message), not in the individual PR commits - warn: on teams without release automation, conventional commits often turn into a cargo cult with no payoff

Подводные камни

• ✗ Using conventional commits as a "rule for the rule's sake" without changelog or semver automation. Then it is just a formality
• ✗ Writing `chore: stuff` with no scope and no explanation. The format is met, the meaning is zero
• ✗ Not enforcing the format in CI. The team quickly drifts between "feat:" and "Feature:"

Follow-up

• ? How do you set up commitlint plus husky to block an invalid format?
• ? What goes into `semantic-release` release notes when a PR had neither feat nor fix?
• ? Where do you write `BREAKING CHANGE:`, in the subject or in the commit body?

Что отвечать

A good PR: one task, a clear title and description (what, why, how you tested, breaking changes), a reasonable size (≤ 400 lines usually), atomic commits, passing CI, and the "let's discuss" spots explicitly marked in your own review comments. A bad PR: "refactor + feature + dependency bump", 3000 lines with no description, the reviewer is invited to "figure it out from the diff", red CI with a comment "will fix later".

Что хотят услышать

A senior should: - name the checklist for a good PR description: context, what was done, how you tested, what was **not** done or deferred, screenshots for UI - talk about reasonable size: studies (Google, SmartBear) show that defect detection drops after 400 lines, and the author's discipline drops after 800 - mention "mark the contentious spots yourself". A self-review with comments saves the reviewer an hour - name the practice of a draft PR / WIP PR for early feedback without asking for a full review - say that for a large task a series of PRs is better (one logical piece = one PR) than one mega-PR

Подводные камни

• ✗ Dumping a huge refactor + feature into one PR. The reviewer either gets stuck for a week, or merges without looking
• ✗ Writing a one-line description `fix bug`. The reviewer does not understand what exactly you fixed
• ✗ Hiding a "drive-by refactor" inside a feature PR. It blurs the review focus

Follow-up

• ? When is a draft PR appropriate versus ready-for-review?
• ? What belongs in a PR template so the author does not forget anything important?
• ? How do you split a finished huge PR into a series without losing history?

Что отвечать

First read the PR description and the context, then look at the code. Distinguish three kinds of comments: **must-fix** (a bug, security, a contract violation), **should-fix** (poor readability, a potential problem), **nit/style** (style, taste). Mark the kind explicitly, otherwise the author spends effort on all of them equally. Do not block a PR over nits. With 5+ remarks, a synchronous conversation is faster than an asynchronous exchange.

Что хотят услышать

A senior should: - name the convention of comment prefixes: `nit:`, `suggestion:`, `question:`, `blocker:`, so the author sees the priority at once - say that the reviewer must name **what they approve of**, not only what they criticize. Otherwise the author thinks the whole PR is bad - mention Conventional Comments (`praise:`, `nitpick:`, `suggestion:`, `issue:`, `question:`, `thought:`, `chore:`) as a ready vocabulary - name the "smell metrics": a PR sitting > 2 days needs a ping or a reassign. > 10 comment round-trips need a call. If one reviewer stays silent for a week, reassign - say that review should focus on **meaning**. Syntax and formatting are handled by a linter, not by the reviewer

Подводные камни

• ✗ Blocking approve over a `nit:`. The author reworks it for a day, with zero value
• ✗ Staying silent about the good parts. The author does not understand what was done well
• ✗ Arguing about tabs vs spaces in review. That is the job of prettier/black, not the reviewer

Follow-up

• ? How do Conventional Comments differ from plain review replies?
• ? When is synchronous code review better than async?
• ? How do you set up required reviewers in GitHub so review does not block on nits?

Что отвечать

`.github/CODEOWNERS` assigns automatically: "these files are touched by these people or teams". When a PR opens with an edit to `infra/**`, GitHub itself requests review from `@org/sre`. Branch protection on main requires N approvals, status checks (CI green), and often required-review-from-codeowners. Force-push and delete are forbidden. Together this gives you: "you cannot merge into main without green CI and approval from the right people".

Что хотят услышать

A senior should: - name the CODEOWNERS format (glob + handles), say that the order of lines matters (the last match wins) and that teams need write access to the repo - explain required status checks: the list of workflows that must pass (build, test, lint, security-scan) - mention "require linear history" as an option that forbids merge commits, allowing only rebase/squash - name `restrict who can push to matching branches` to forbid direct pushes to main, everything only through a PR - say that admin bypass of branch protection is usually open for emergency fixes, but it must be logged and discussed

Подводные камни

• ✗ Thinking CODEOWNERS blocks a merge. It does not block by itself, it blocks only if branch protection has `require review from code owners` turned on
• ✗ Putting CODEOWNERS in the wrong place. It is valid in `.github/`, `docs/`, or the root, but not just anywhere
• ✗ Protecting only main and forgetting about release/* and develop. Then they become the bypass

Follow-up

• ? What happens when a person in CODEOWNERS leaves but their GitHub account stays?
• ? How does `require linear history` differ from forbidding merge strategies?
• ? How do you configure branch protection through `gh api` for several repos at once?

Что отвечать

Semver (MAJOR.MINOR.PATCH): a breaking change → MAJOR++, a backward-compatible feature → MINOR++, a patch fix → PATCH++. Tools like `semantic-release`, `release-please`, `changesets` read the commits since the previous tag (usually via Conventional Commits), determine the bump type, generate `CHANGELOG.md`, create a git tag and a GitHub release. Discipline in the commit format gives you "one button press = a new release with release notes".

Что хотят услышать

A senior should: - connect the three parts: Conventional Commits → parser → semver bump + changelog + tag, and say that without the first the automation is impossible - name tools by ecosystem: `semantic-release` (Node), `release-please` (multi-language, Google), `changesets` (Node, monorepo-first), `python-semantic-release` (Python) - mention that `0.x.y` has special rules: a minor can be breaking, the project is still in pre-release - say that for libraries breaking changes are especially costly. A major jump forces consumers to migrate, so late majors often accumulate breaking changes - name `git tag -s v1.2.3` for GPG-signed release tags as part of supply-chain security

Подводные камни

• ✗ Bumping MAJOR on every small API edit. Consumers get tired of migrations
• ✗ Thinking `0.x.y` is "the same semver". In 0.x a minor can be breaking, semver explicitly allows it
• ✗ Running `git tag` without a push. The tag stays local, CI does not know about the release

Follow-up

• ? How does `release-please` differ from `semantic-release`?
• ? What do you do about a changelog when commits have been free-form until now?
• ? How do you sign a tag with a GPG key, and why does it matter for releases?