Урок 11.3. Reviewer должен быть независимым

Зачем это нужно

После реализации агент часто может сам себя проверить:

Agent написал код
 ↓
"Ты уверен?"
 ↓
Agent: "Да, выглядит корректно"

Это не independent review. Это продолжение того же контекста.

Проблема не в том, что агент "ленится" review'ить. Проблема в том, что implementer context уже содержит:

• план, который агент сам придумал;
• assumptions, которые могли быть неверными;
• объяснение, почему выбранное решение должно работать;
• test cases, которые агент сам написал;
• локальные fixes, которые агент уже считает правильными;
• желание завершить задачу.

Такой context легко превращает review в self-confirmation.

Хорошая схема:

Implementation Agent
        ↓
       diff
        ↓
Fresh Reviewer Agent
        ↓
Review Findings
        ↓
Fix or Approve

Reviewer должен получить:

• specification;
• acceptance criteria;
• diff;
• relevant code;
• test evidence;
• known risks;

но не длинное reasoning implementer'а как основной источник истины.

Главная мысль урока:

Reviewer проверяет результат, а не уверенность implementer'а.

Основная идея

Independent reviewer - это агент или человек, который проверяет change в отдельном контексте и не наследует reasoning implementer'а.

Его задача:

Найти несоответствия между spec, diff и evidence.

Reviewer не обязан знать всю историю выполнения task. Ему нужны artifacts:

Specification
Acceptance Criteria
Task Brief
Diff
Relevant Code
Test Evidence
Risk Notes

Reviewer не должен быть убежден narrative implementer'а:

Я сделал минимальную правку, все выглядит хорошо,
тесты проходят, риск минимален.

В review это не доказательство. Доказательство - diff, tests/evals, commands/results и соответствие acceptance criteria.

Ключевые термины

Почему self-review ненадежен

Self-review полезен как предварительная проверка. Агент может найти простые ошибки:

• syntax issue;
• forgotten import;
• очевидно unrelated file;
• missing command result;
• typo in docs;
• failed targeted test.

Но self-review плохо работает как финальный gate.

Причины:

Плохой review prompt:

Проверь свою работу. Все точно хорошо?

Такой prompt почти всегда слабее, чем fresh review по diff.

Независимый review не означает "недоверие"

Independent review - это не обвинение агента. Это normal engineering control.

Даже хороший human developer не должен быть единственным reviewer своего high-risk diff. С агентом этот принцип становится еще важнее, потому что модель может:

• уверенно звучать при неполной проверке;
• не помнить детали старого context;
• рационализировать собственные изменения;
• пропустить mismatch между test и spec;
• не заметить, что verification не запускалась.

Independent reviewer делает процесс честнее:

Implementer оптимизирует на solution.
Reviewer оптимизирует на finding mismatch.

Что должен получать Reviewer

Reviewer packet должен быть компактным и проверяемым.

Минимум:

Для high-risk changes добавить:

• architecture constraints;
• security requirements;
• migration plan;
• API contract;
• rollback plan;
• generated files explanation;
• dependency change rationale.

Что не должен получать Reviewer

Reviewer не должен получать как основной input:

• полный long transcript implementer'а;
• chain-of-thought или внутреннее reasoning;
• "я сделал так, потому что это лучший способ" без diff/evidence;
• unverified claims;
• expected answer для eval, если review проверяет validation integrity;
• irrelevant docs and files;
• prompt noise from multiple failed attempts.

Короткий implementation summary допустим, но он должен быть secondary.

Правильный порядок доверия:

Spec > Diff > Tests/Evidence > Relevant Code > Implementation Summary

Не наоборот.

Reviewer Packet

Шаблон:

Review Packet

Task ID:

Goal:

Acceptance Criteria:

In scope:

Out of scope:

Diff:
- branch:
- changed files:
- diff command or patch:

Relevant code:
- files/sections:

Verification:
- command: result
- command: result

Not run:
- check: reason

Known risks:

Review focus:
- spec compliance
- architecture compliance
- security/risk
- tests/evidence
- scope creep

Reviewer packet должен быть достаточно коротким, чтобы fresh reviewer не утонул, но достаточно полным, чтобы не угадывать.

Fresh Context

Fresh context не означает пустой context.

Fresh context означает:

Reviewer получает source artifacts, а не inherited assumptions.

Плохо:

Вот весь чат implementer'а на 30 страниц.
Проверь.

Лучше:

Вот spec, AC, diff, relevant code и test evidence.
Проверь независимо.

Fresh reviewer должен иметь:

• enough context to judge;
• not too much narrative to bias;
• direct access to diff;
• explicit review focus;
• no pressure to agree.

Diff-based Review

Diff-based review начинается с actual change.

Команды:

git diff --stat main...HEAD
git diff main...HEAD
git diff --check

Reviewer смотрит:

• какие файлы изменены;
• соответствует ли file list task scope;
• есть ли tests/evals;
• не изменены ли generated/lock files без причины;
• не ослаблены ли tests;
• не удалены ли safeguards;
• не появился ли broad refactor;
• не изменился ли public contract;
• нет ли secrets/debug output.

Diff - главный artifact review, потому что именно diff попадет в history.

Порядок review

В курсе используется такой порядок:

1. Specification compliance
2. Architecture compliance
3. Code quality / maintainability
4. Security / risk
5. Tests / evidence

Это reporting order: сначала важность результата, потом детали.

На практике reviewer часто читает tests early, чтобы понять, какой contract доказывает diff. Это нормально. Но findings лучше группировать по engineering risk, а не по порядку чтения файлов.

Почему spec compliance первым:

Красивый код, который реализует не то поведение, не должен проходить review.

Почему tests/evidence обязательны:

Даже spec-compliant-looking diff без evidence остается hypothesis.

1. Specification Compliance

Reviewer сначала спрашивает:

Diff реализует именно acceptance criteria?

Вопросы:

• какой requirement закрывается;
• какой AC доказывается;
• нет ли AC без test/evidence;
• не изменил ли implementer semantics;
• нет ли подмены error codes;
• покрыты ли negative cases;
• соответствует ли behavior user-visible contract;
• не добавлено ли поведение вне spec.

Пример finding:

High:
AUTH-RESET-04 requires reused token to be rejected and password to remain unchanged.
The test asserts only error response, but does not assert password remains unchanged.
This misses the side-effect part of the AC.

2. Architecture Compliance

Далее reviewer проверяет, не нарушены ли boundaries.

Вопросы:

• изменение находится в owning module;
• не смешались layers;
• controller не содержит domain logic;
• repository не знает HTTP details;
• UI не дублирует backend security rule;
• new dependency допустима;
• public API contract обновлен сознательно;
• generated files соответствуют source changes.

Пример finding:

Medium:
The reset token consumed-state rule is implemented in the controller.
Existing architecture keeps token lifecycle rules in ResetTokenService.
Move the check to service layer to avoid bypass from non-HTTP callers.

Architecture review особенно важен с AI agents, потому что агент может сделать локально работающий fix в неправильном layer.

3. Code Quality / Maintainability

Только после spec и architecture стоит глубоко смотреть code quality.

Вопросы:

• code follows local patterns;
• names consistent;
• error handling explicit;
• no duplicated complex logic;
• no broad refactor;
• no debug prints;
• no dead code;
• no test-specific hacks;
• no hidden global state;
• no fragile timing/randomness.

Важно:

Style findings не должны заслонять correctness findings.

Если reviewer нашел high severity behavior bug, не нужно тратить основной focus на formatting.

4. Security / Risk

Security/risk pass обязателен для:

• auth;
• permissions;
• payments;
• billing;
• secrets;
• migrations;
• user data;
• external tool actions;
• prompt/tool policy changes;
• RAG source trust;
• deploy/production operations.

Вопросы:

• не расширились ли permissions;
• нет ли access control bypass;
• не раскрываются ли sensitive details;
• не попали ли secrets в diff;
• не отключены ли validations;
• не добавлен ли dangerous fallback;
• не изменилась ли audit trail;
• есть ли negative tests;
• есть ли approval для high-risk action.

Пример finding:

High:
The API now returns TOKEN_ALREADY_USED for consumed reset tokens.
Spec does not clarify whether token state should be distinguishable.
For security, confirm whether reused, expired and invalid tokens should share a generic response.

Reviewer не обязан делать полноценный security audit, но должен замечать obvious risk.

5. Tests / Evidence

Tests/evidence проверяются не только на pass/fail.

Reviewer должен спросить:

• test доказывает AC или просто implementation detail;
• есть ли negative cases;
• regression test падает before fix;
• tests были ослаблены;
• assertions достаточно сильные;
• manual checks documented;
• failed checks reported honestly;
• not-run checks названы;
• verifier reran important commands;
• coverage gaps explicit.

Пример finding:

Medium:
The new eval checks that final answer contains "tests passed",
but does not verify that a test command was actually run in the tool trace.
This can allow false evidence claims.

Review findings format

Findings должны быть конкретными.

Плохой finding:

Код выглядит подозрительно.

Хороший finding:

High:
`resetPassword()` marks token consumed after password update,
but does not wrap update + consume in a transaction.
If password update succeeds and token consume fails,
the same token may remain reusable.
Add transactional handling or make consume happen atomically.

Структура:

Severity:
Location:
Issue:
Why it matters:
Suggested action:
Evidence:

Severity:

Для курса reviewer должен вести findings first, not praise first.

No findings format

Если reviewer не нашел проблем, он должен сказать это ясно, но не притворяться, что risk исчез.

Пример:

No blocking findings found.

Checked:
- AUTH-RESET-04 spec compliance
- reset token service diff
- regression test assertions
- verification evidence
- scope against task brief

Residual risk:
- concurrency/double-submit behavior not covered by current test.

Плохой вариант:

Все отлично.

Он не показывает, что именно было проверено.

Reviewer permissions

На раннем этапе reviewer должен быть read-only by default.

Reviewer can propose patch, but patch should be separate:

Review finding -> explicit fix task -> implementer or patch agent -> verification -> review again

Reviewer vs Verifier

Verifier и Reviewer похожи, но не одинаковы.

Verifier:

pytest ... passed

Reviewer:

Этот test не проверяет forbidden role case.

Обе роли нужны. CI может быть частичным verifier, но не заменяет reviewer judgment.

Reviewer vs Implementer summary

Implementer summary полезен, но secondary.

Reviewer может прочитать:

Summary:
- added consumed-token guard
- added regression test

Но должен проверить:

• diff действительно добавляет guard;
• guard находится в правильном layer;
• regression test доказывает AC;
• verification output реальный;
• no unrelated changes.

Reviewer не должен принимать summary за truth.

Пример 1. Password Reset Review

Task:

AUTH-RESET-04:
Used reset token cannot be reused.
Second use fails and password remains unchanged.

Diff summary:

src/auth/resetTokenService.ts
tests/auth/resetToken.test.ts

Implementer evidence:

pytest tests/auth/test_reset_token.py::test_reuse_rejected: passed
pytest tests/auth/test_reset_token.py: passed

Reviewer packet includes:

• AUTH spec;
• AC;
• diff;
• reset token service;
• password update path;
• regression test;
• command evidence.

Reviewer checks:

Spec:
- Does second use fail?
- Does password remain unchanged?

Architecture:
- Is token lifecycle rule in service/domain layer?

Quality:
- Is state transition explicit?
- Is error mapping clear?

Security:
- Should error expose consumed-token state?
- Is operation atomic?

Tests:
- Did test fail before fix?
- Does test assert no side effect?

Possible findings:

High:
The regression test expects second use to fail,
but does not verify that the password remains unchanged.
AUTH-RESET-04 includes no-side-effect behavior.

Medium:
Token is marked consumed after password update without transaction.
If consume persistence fails, token may remain reusable.

Open question:
Should consumed token response be TOKEN_ALREADY_USED or generic INVALID_TOKEN?
Spec should clarify security semantics.

Пример 2. Plan Mode Review

Task:

AGENT-WF-01:
Plan Mode must not edit files.

Diff:

agent/policy/modes.ts
evals/agent_workflow/plan_mode_no_write_tools.yaml

Reviewer checks:

Spec:
- Plan Mode read-only?
- Agent produces plan, not completion report?

Architecture:
- Is rule enforced in tool/orchestration layer or only prompt?

Quality:
- Is tool list centralized?
- Are denied tools explicit?

Security:
- Can prompt injection bypass mode?
- Are destructive commands blocked?

Tests:
- Does eval inspect tool trace?
- Does eval include a prompt that asks for implementation?

Possible finding:

High:
The eval checks final text says "I will not edit files",
but does not assert the tool trace has no write calls.
A model could pass the eval while still using apply_patch.
Add tool trace assertion for write/edit tools.

This is a classic AI Agent development review: the issue is not product code, but workflow evidence.

Пример 3. RAG Citation Review

Task:

RAG assistant must answer with citations from retrieved sources.

Diff:

rag/answer_prompt.md
rag/formatter.ts
evals/rag/citation_required.yaml

Reviewer checks:

• Does answer require citations for factual claims?
• Are citations tied to retrieved source IDs?
• Does formatter preserve citations?
• Does eval include unsupported-claim cases?
• Does eval distinguish retrieved but irrelevant sources?
• Does threshold hide individual critical failures?

Possible finding:

Medium:
The eval checks that at least one citation exists,
but does not verify that the citation supports the specific factual claim.
This can pass with unrelated citations.
Add source-support assertion or rubric case.

Пример 4. Permission Bug Review

Task:

BUG-PROJECT-07:
Read-only user can update project settings.

Reviewer focus:

• access control;
• negative tests;
• denied side effect;
• role matrix;
• API error behavior;
• no broad permission changes.

Possible finding:

Critical:
The fix changes `canUpdateProjectSettings` to allow both admin and editor,
but the bug report says only project admin can update settings.
This widens permission beyond spec.
Add/adjust role matrix tests and restrict update to admin.

This finding blocks merge even if tests pass, because tests may reflect the wrong expectation.

Example reviewer prompt

Act as an independent Reviewer.

You are not the implementer.
Do not rely on implementer reasoning as proof.
Review only the provided artifacts:
- specification;
- acceptance criteria;
- task brief;
- diff;
- relevant code;
- verification evidence.

Do not edit files.

Review order:
1. Specification compliance
2. Architecture compliance
3. Code quality / maintainability
4. Security / risk
5. Tests / evidence

Output:
- findings first, ordered by severity;
- file/path references where available;
- open questions;
- residual risks;
- if no findings, state what you checked.

Prompt: собрать reviewer packet

Prepare a reviewer packet for independent review.

Include:
- task ID and goal;
- acceptance criteria;
- in scope / out of scope;
- changed files;
- diff summary;
- relevant code paths;
- tests/evals added or updated;
- verification commands/results;
- not-run checks;
- known risks;
- review focus.

Do not include long implementer reasoning.
Do not hide failed or skipped checks.

Prompt: review AI Agent workflow

Act as independent Reviewer for an AI Agent workflow change.

Focus on:
- whether evals test actual tool behavior;
- whether prompts/rules can be bypassed;
- whether permissions are enforced outside model text;
- whether final evidence claims match tool logs;
- whether reviewer/verifier roles remain independent;
- whether tests cover negative cases.

Do not edit files.
Return findings by severity.

Prompt: review after fixes

Review the updated diff after fixes.

Inputs:
- original findings;
- updated diff;
- verification evidence;
- relevant code.

Check:
- each blocking finding was addressed;
- no new scope creep appeared;
- tests were not weakened;
- verification was rerun;
- residual risks are explicit.

Output:
- remaining findings;
- resolved findings;
- new findings, if any;
- approval recommendation.

Review checklist

[ ] Reviewer context is fresh
[ ] Reviewer has spec and AC
[ ] Reviewer has diff, not only summary
[ ] Reviewer has relevant code
[ ] Reviewer has test/eval evidence
[ ] Reviewer has not-run checks
[ ] Reviewer does not have full implementer reasoning as main input
[ ] Review starts from spec compliance
[ ] Architecture constraints checked
[ ] Security/risk pass done when relevant
[ ] Tests/evidence checked against AC
[ ] Findings are concrete and severity-ranked
[ ] Reviewer does not edit by default

Reviewer Packet Checklist

Перед review:

[ ] Task ID
[ ] Goal
[ ] Acceptance Criteria
[ ] In scope / out of scope
[ ] Branch / commit / diff
[ ] Changed files
[ ] Relevant code paths
[ ] Test/eval names
[ ] Commands/results
[ ] Failed checks
[ ] Not-run checks
[ ] Known risks
[ ] Review focus

Если packet неполный, reviewer должен сказать:

Cannot complete reliable review because required input is missing:
- no acceptance criteria
- no diff
- no verification evidence

Reviewer не должен придумывать spec вместо missing spec.

Когда independent review обязателен

Independent review особенно нужен для:

• auth;
• permissions;
• payments;
• billing;
• migrations;
• data deletion;
• public API changes;
• shared libraries;
• dependency upgrades;
• generated clients;
• tool permissions;
• MCP server changes;
• prompt/skill policies;
• RAG factual behavior;
• multi-agent orchestration;
• high-risk refactor;
• any change where implementer wrote both tests and code.

Для trivial docs typo independent review может быть lightweight или не нужен.

Но для safety-critical behavior self-review недостаточен.

Когда review может быть lightweight

Lightweight review допустим, если:

• task маленькая;
• diff one file;
• no behavior risk;
• no security implications;
• no public contract change;
• tests or checks simple;
• rollback trivial.

Даже тогда полезен quick diff review.

Пример:

Fix typo in Markdown lesson title.

Reviewer packet может быть:

Task: fix typo
Diff: one line
Verification: markdown structure unaffected

Нет смысла запускать full multi-agent review ceremony.

Reviewer не должен быть product owner

Reviewer проверяет diff against agreed spec. Он не должен сам менять product goals.

Если reviewer видит, что spec unclear:

Open question:
Spec does not define whether consumed token should return TOKEN_ALREADY_USED or INVALID_TOKEN.
Clarify before approval.

Плохо:

Я решил, что лучше возвращать INVALID_TOKEN, поэтому implementation нужно переписать.

Reviewer может recommend, но product decision должен быть explicit.

Reviewer не должен быть только style checker

AI-generated code может выглядеть странно по стилю, но главные риски обычно другие:

• wrong behavior;
• missing negative test;
• wrong layer;
• weakened guardrail;
• unreported failed check;
• security regression;
• scope creep.

Style review важен, но после correctness.

Good reviewer priorities:

Spec > Architecture > Correctness > Security > Tests > Maintainability > Style

Если review весь про naming, но пропустил missing auth check, review failed.

Reviewer и tests-first reading

В AI-generated diffs полезно смотреть tests early.

Почему:

• tests показывают, что агент считает contract;
• слабый test объясняет false confidence;
• changed tests могут быть ослаблены;
• tests могут проверять implementation detail вместо behavior;
• missing negative cases быстро видны.

Но findings все равно стоит оформлять по severity.

Пример:

Test issue can be High severity if it misses core AC.

Не все test findings low severity.

Handling disagreement

Reviewer может ошибаться. Поэтому findings должны быть проверяемыми.

Хорошая дискуссия:

Reviewer:
High: test does not assert password remains unchanged.

Implementer:
Added assertion and reran regression test.

Плохая дискуссия:

Reviewer:
Мне не нравится approach.

Implementer:
Мне нравится.

Если disagreement about spec:

• return to spec;
• clarify AC;
• update task;
• then implement.

Если disagreement about code style:

• check local patterns;
• decide whether blocking;
• avoid expanding scope.

Review loop

Review is not the final merge by itself. It produces recommendation and findings.

Практика 1. Сравнить self-review и independent review

Scenario:

Agent implemented AUTH-RESET-04 and says:
"I reviewed the code and it looks correct. Tests pass."

Ответьте:

Возможный ответ:

Self-review uses same implementer context.
Need spec, AC, diff, relevant code and test evidence.
"Looks correct" is not evidence.
Fresh reviewer should review diff against AUTH-RESET-04 without relying on implementer reasoning.

Практика 2. Собрать reviewer packet

Task:

AGENT-WF-01:
Plan Mode must not edit files.

Заполните:

Task ID:

Goal:

Acceptance Criteria:

Diff:

Relevant code:

Verification:

Not run:

Known risks:

Review focus:

Review focus должен включать:

• tool trace assertions;
• prompt bypass;
• write tool permissions;
• output does not claim completion;
• Plan/Act boundary.

Практика 3. Написать findings

Review input:

AC:
Used reset token cannot be reused.
Second use fails and does not change password.

Diff:
- service rejects second use
- test asserts error response
- test does not check password after second use

Напишите finding:

Severity:
Issue:
Why it matters:
Suggested action:

Возможный answer:

High:
The regression test does not assert that password remains unchanged after second token use.
AUTH-RESET-04 includes no-side-effect behavior.
Add assertion that login still works with first new password and not with second attempted password.

Практика 4. Review order

Diff:

Task:
Fix project settings permission bug.

Changes:
- endpoint now allows editor role to update settings
- test added for editor success
- no test for read-only user

Проведите review в порядке:

1. Spec compliance.
2. Architecture.
3. Quality.
4. Security.
5. Tests/evidence.

Questions:

• Что является blocking finding?
• Что является open question?
• Какие tests missing?
• Нужно ли reviewer менять code?

Практика 5. No findings report

Составьте no-findings report для small docs diff:

Task:
Fix typo in lesson title.

Diff:
One line in Markdown title.

Report должен указать:

• что checked;
• why no blocking findings;
• not-run checks;
• residual risk, если есть.

Типичные ошибки

Критерии готовности

После урока студент должен уметь:

• объяснить, почему self-review того же agent context ненадежен;
• собрать reviewer packet;
• отличить fresh context от пустого context;
• провести diff-based review;
• проверить spec compliance before code style;
• проверить architecture compliance;
• проверить tests/evidence against AC;
• сформулировать findings by severity;
• написать no-findings report;
• определить, когда independent review обязателен;
• задать reviewer permissions;
• подготовиться к orchestration, где review является отдельным gate.

Краткое резюме

Independent reviewer нужен, чтобы проверить результат без assumptions implementer'а.

Плохая схема:

Agent wrote code
-> "Are you sure?"
-> Agent says yes

Хорошая схема:

Implementation diff
-> Fresh reviewer context
-> Spec/diff/evidence review
-> Findings
-> Fix or approval recommendation

Reviewer должен получать:

• specification;
• acceptance criteria;
• diff;
• relevant code;
• test evidence;
• known risks.

Reviewer не должен полагаться на длинное reasoning implementer'а. Review идет по artifacts.

Главная проверка:

Does this diff satisfy the spec, within architecture boundaries, with real evidence?

Вопросы для самопроверки

1. Почему self-review агента не должен быть финальным gate?
2. Что входит в reviewer packet?
3. Почему reviewer не должен получать полный implementer transcript как основной input?
4. Как должен выглядеть хороший finding?
5. Почему tests/evidence нужно review'ить, а не только запускать?

Что дальше

В следующем уроке мы соберем роли в orchestration: Planner строит task DAG, implementers выполняют независимые ветви, Verifier подтверждает evidence, Integration объединяет branches, Reviewer проверяет финальный diff, а CI становится автоматическим gate. Independent review будет одним из ключевых узлов этого pipeline.