toridori 社で行われた Agentic Coding のレビュー議論の資料
(2025-08-06)
Sora さんから Serena の紹介
Sora さんありがとうございます
ナレッジ共有嬉しい
突然の指名の快諾感謝👏
「レビュー待ち」が
より重大という前置きをしたい
たぶんペアプロ的セッション
コード生成の横で
リアルタイムレビュー
みたいになりそう
レビューで何を見るか
この先のコードは
スライドのため省略してる
ところもあるので注意
この後の部分は
多くの議論があったので
一つの意見のたたき台
として見てください
バグか、エラーか、取りうる値か
事前条件の考え方
それらをどう書くか
function divide(a: number, b: number) {
if (b === 0) return null;
return a / b;
}
function divide(a: number, b: number) {
if (b === 0) throw new Error('Division by zero');
return a / b;
}
function divide(a: number, b: number) {
assert(b !== 0, 'Divisor must be non-zero');
return a / b;
}
const ACCESS_TOKEN = process.env.ACCESS_TOKEN;
if (ACCESS_TOKEN !== undefined) {
throw new Error(...);
}
const ACCESS_TOKEN = process.env.ACCESS_TOKEN;
assert(ACCESS_TOKEN !== undefined);
async function loadConfig() {
// ↓ この条件をどう扱う?
// await fs.exists(CONFIG_PATH)
const configSrc = await fs.readFile(path, 'utf-8');
const configObj = JSON.parse(configSrc);
const config = ConfigSchema.safeParse(configObj);
// ↓ この条件をどう扱う?
// config.success
return config.data;
}
// この as ってなんだろう?
const user = data as User;
// バグ?
assert(isUser(data), 'Bug: data must be User');
// エラー? (主にデータ発生源とかで)
if (!isUser(data)) throw new ValidationError(...);
// とりうる値?
if (isUser(data)) {
processUser(data);
} else {
processGuest();
}
最小編集バイアス
async function getUsers() {
return await api.get('/users');
}
// LLM は関数名やシグネチャを変えることを過度に恐れ以下のような修正をする
// 責任範囲や視野を与えていない人間の責任
async function getUsers(includeInactive: boolean = true) {
if (includeInactive) {
return await api.get('/users');
} else {
return await api.get('/users?filter=status:active)');
}
}
async function getAllUsers() {
return await api.get('/users');
}
async function getActiveUsers() {
return await api.get('/users?filter=status:active');
}
# 呼び出し元も grep して修正
rg 'getUsers' src/
# または build すれば影響範囲が分かる?
pnpm build
(Esc キーを押して無駄なことは直ちにやめさせる)
> その方法は受け入れられないので revert
> /compact 既存の修正を見直すために compact します。
(こういうことにこそ context が必要)
> OK. まず今やっていることの目的に立ち返って @DESIGN.md を見て
既存のコードと追加すべきコードを「もし一から全て書き直したら」
どのようなコードになるかまず詳細に考えて。
その上でそのコードと現状との差に着目して。
消すべきものは消して。名前を変えるべきものは変えて。
影響するすべてのコードを rg などのコマンドで調べて修正して。
大きな変更になっても OK. You own everything. では修正して。
防御的プログラミング
async function updateUserAge(userId: string | undefined, age: number | undefined) {
if (typeof userId !== 'string') return;
if (!UserIdSchema.safeParse(userId).success) return;
if (typeof age !== 'number') return;
if (!Number.isInteger(age)) return;
if (age < 0 || age > 150) return;
try {
const user = await getUserById(userId);
if (!user) return;
await db.updateUser(userId, { age });
} catch (error) {
console.error('Failed to update user age:', error);
}
}
async function updateUserAge(userId: string, age: number) {
assert(0 <= age && age < 150, 'Age must be [0, 150)');
const user = await getUserById(userId);
if (user === null) {
throw new UserNotFoundError(userId);
}
await updateUser(userId, { age });
}
> 防御的にならないでください Fail Fast してください。
> もし age や userId がそれらの値を含む箇所があるならば
そちらの方を修正してください。
データの検証は、データや誤差の発生源で行うべきであるので、
ロジックやデータの保存箇所で行うべきではありません。
バグに依存したシステムを許容しないため、クラッシュさせるべきです。
YAGNI
const BATCH_SIZE = process.env.BATCH_SIZE ?? 100;
function main(opts: Options) {
const verbose = opts.verbose ?? false;
const batchSize = opts.batchSize ?? BATCH_SIZE;
if (verbose) {
console.log(`Running with batch size: ${batchSize}`);
}
while (...) {
processBatch(batchSize);
}
}
// 自分は原理主義なのでこの quality も YAGNI に見える
// てか default param 嫌いは YAGNI と最小編集バイアス臭がする
// ハードコードしたい
// ラディカルすぎる?みんなはどう?
async function saveToJpeg(
path: string, image: Image, quality: number = 75) {
await fs.writeFile(path,
await JpegEncoder.encode(image, { quality }));
}
validate, encode, decode の場所
async function predictUserDemographicsByLlm(userId: string) {
const user = await getUserById(userId);
const userReprPrompt = generatePromptFromUser(user);
return await retry(3, async () => {
// ここがデータの発生源
const demographicsSrc = await callLlm(
MODEL, SYSTEM_PROMPT, userReprPrompt);
// JSON, Schema, 値の整合性や正規化、標準化
// - 後続のロジックにおける検証を全て終える
// - 検証はここでのみ行う
// - その他の箇所ではすべて assert する
return parseDemographics(demographicsSrc);
});
}
async function processMessageStream() {
// decode はデータの発生源で行う
const data = await readStream();
const messages = decodeStreamData(data);
for (const message of messages) {
// ロジックは型安全であり、型以外の整合性も取れた状態
await processMessage(message);
// 出口があれば出口の形式に encode
await outputMessage(encodeMessage(message));
// こういうのも encode の一種かも
console.log(
`Processed message: ${safeStringify(message)}`);
}
}
[入口] → [decode/validate] → [ロジック] → [encode/escape] → [出口]
> ロジックにその if はおかしくない?
バグ?検証漏れ?防御的プログラミング?
データの発生源どこ?ちゃんと検証してるか確認して
とにかくそこのコードは assert して Let it crash!
LLM の出力を
リアルタイムレビューする
絵としてコードを見る
function processSomething(user: User) {
// assert は無料! assert はあって困ることはない!
// 間違ってても呼ぶだけでクラッシュしてくれる
assert(...); ...
// データ準備 (直線で if 無し!)
const fooBar = getFooBar(user);
const items = user.items.map(mapItem);
// 末尾で判断(関数の存在意義っぽい)
// シンプルな式! 三単元の s !
if (isActive(status)) {
return doSomething(user);
} else {
return handleInactiveUser(user);
}
}
✅ だいちゅき
[入力] → [処理] → [末尾判断]
├─→ [処理]→ [結果A]
└─→ [処理]→ [結果B]
❌ ふぎいい
[入力] → [チェック1] ──return──→ [結果X]
↓
[チェック2] ──return──→ [結果Y]
↓
[処理] → [末尾] → [結果Z]
(個人の見解です)
if (process.env.ACCESS_TOKEN === undefined) return
// それはバグでは? assert しよ
if (isUser(data)) return
// 発生源で validate しない?
if (items.length === 0) totalPrice = 0;
else totalPrice = items.reduce(
(acc, item) => acc + item.price, 0);
// 下の式は length 0 を扱える
function processSomething(user: User) {
...
// この if を単一責任として切り出せそう!!
if (...) {
...
} else {
...
}
...
if (isActive(status)) {
return doSomething(user);
} else {
return handleInactiveUser(user);
}
}
キーワードをまとめます
レビューっていつかやめるよね?