iop/agent-task/unbounded_cli_sessions/code_review_1.log

<!-- task=unbounded_cli_sessions plan=1 tag=REVIEW_REFACTOR -->

# Code Review Reference - REVIEW_REFACTOR

## 개요

date=2026-05-03
task=unbounded_cli_sessions, plan=1, tag=REVIEW_REFACTOR

## 이 파일을 읽는 리뷰 에이전트에게

각 항목의 구현을 실제 소스 파일과 대조하고, `검증 결과` 섹션의 출력이 코드와 일치하는지 확인하세요.
리뷰 완료 후 반드시 아래 순서로 아카이브하세요.

1. `CODE_REVIEW.md` → `code_review_N.log` (N = 기존 code_review_*.log 수)
2. `PLAN.md` → `plan_M.log` (M = 기존 plan_*.log 수)
3. PASS인 경우 `complete.log` 작성 후 종료. WARN/FAIL인 경우 새 `PLAN.md` + `CODE_REVIEW.md` 스텁 작성.

---

## 구현 항목별 완료 여부

| 항목 | 완료 여부 |
|------|---------|
| [REVIEW_REFACTOR-1] Restore Foreground OnRunRequest Error Return | [x] |
| [REVIEW_REFACTOR-2] Add Idempotent Store Migration For Existing Runs Table | [x] |

### REVIEW_REFACTOR-1 구현 메모

- `apps/node/internal/node/node.go:120-135`: `run` closure가 `error`를 반환하도록 변경. foreground는 `return run()`, background는 `go func() { _ = run() }()`로 dispatch 후 즉시 nil 반환. `defer cancel/deregister/close(h.done)`는 그대로 유지되어 background에서도 store update가 실행됨.
- `apps/node/internal/node/node_test.go`: `failingAdapter` test double 추가. 두 regression test 추가:
  - `TestOnRunRequest_ForegroundAdapterErrorReturned` — adapter가 `errors.New("adapter boom")`을 반환할 때 `OnRunRequest`가 non-nil error를 반환하고 store status가 `failed`이며 stored error에 `"adapter boom"`이 포함되는지 검증.
  - `TestOnRunRequest_ForegroundCancelReturnedAndStored` — adapter가 `runtime.ErrRunCancelled`를 반환할 때 `errors.Is(err, runtime.ErrRunCancelled)`가 true이고 store status가 `cancelled`인지 검증.

### REVIEW_REFACTOR-2 구현 메모

- `apps/node/internal/store/store.go`: `migrateRunsTable` 추가. `PRAGMA table_info(runs)`로 컬럼 존재 여부를 확인한 뒤 없는 컬럼만 `ALTER TABLE ... ADD COLUMN ...`으로 추가. `tableColumns` helper도 추가.
- `apps/node/internal/store/store.go`: `New`에서 schema 실행 실패와 migration 실패 시 모두 `db.Close()` 후 error 반환.
- `apps/node/internal/store/store_test.go`: `seedOldSchemaDB` helper 추가 (`session_id`/`background` 컬럼 없는 구 schema fixture). 두 regression test 추가:
  - `TestStore_MigratesExistingRunsTable` — 구 schema DB를 `store.New`로 열어서 새 컬럼과 함께 `InsertRun`/`GetRun`이 동작하는지 검증.
  - `TestStore_MigrationIsIdempotent` — 같은 DB에 `store.New`를 두 번 호출해 두 번째도 실패하지 않는지 검증.

## 계획 대비 변경 사항

없음. PLAN.md 가이드 그대로 구현했다.

## 주요 설계 결정

- migration 실패 시 호출자에게 dangling DB handle을 넘기지 않도록 `New`에서 schema 단계 실패와 migration 단계 실패 모두 `db.Close()` 후 error 반환하도록 수정 (PLAN의 migration 단계만 명시되어 있었지만 일관성 위해 schema 단계도 동일하게 처리).
- background 경로는 `go func() { _ = run() }()`로 error를 명시적으로 무시. error는 `completeRun`이 store와 logger에 이미 기록하기 때문에 caller는 추가 처리할 필요가 없음.

## 리뷰어를 위한 체크포인트

- Foreground `OnRunRequest`가 adapter error를 호출자에게 반환하는지 확인한다. → `node.go:134` `return run()`.
- Background `OnRunRequest`는 dispatch 후 즉시 `nil`을 반환하면서 store update goroutine은 유지하는지 확인한다. → `node.go:130-132`. `defer n.completeRun` 호출은 closure 안에 그대로.
- `runtime.ErrRunCancelled`가 store status `cancelled`와 foreground return error 양쪽에서 일관되게 처리되는지 확인한다. → `completeRun`은 `errors.Is(execErr, runtime.ErrRunCancelled)`로 status 분기, `run` closure는 execErr를 그대로 반환. `TestOnRunRequest_ForegroundCancelReturnedAndStored`가 검증.
- 기존 `runs` table이 있는 SQLite DB에서 `session_id`, `background` columns가 idempotent하게 추가되는지 확인한다. → `migrateRunsTable` + `TestStore_MigratesExistingRunsTable` / `TestStore_MigrationIsIdempotent`.
- migration helper가 새 DB와 기존 DB 양쪽에서 실패하지 않고, failure 시 DB handle을 정리하는지 확인한다. → `New`의 두 error 경로 모두 `_ = db.Close()`.
- 새 regression tests가 기존에 놓친 실패 경로를 실제로 검증하는지 확인한다. → 위 4 test가 fix 직전 상태에서는 실패하도록 작성됨.

## 검증 결과

### REVIEW_REFACTOR-1 중간 검증
```bash
$ go test ./apps/node/internal/node
ok  	iop/apps/node/internal/node	0.008s
```

### REVIEW_REFACTOR-2 중간 검증
```bash
$ go test ./apps/node/internal/store
ok  	iop/apps/node/internal/store	0.026s
```

### 최종 검증
```bash
$ gofmt -w apps/node/internal/node/node.go apps/node/internal/node/node_test.go apps/node/internal/store/store.go apps/node/internal/store/store_test.go
$ go test ./apps/node/internal/node
ok  	iop/apps/node/internal/node	0.008s
$ go test ./apps/node/internal/store
ok  	iop/apps/node/internal/store	0.026s
$ go test ./...
ok  	iop/apps/edge/cmd/edge	(cached)
ok  	iop/apps/edge/internal/node	(cached)
ok  	iop/apps/edge/internal/transport	(cached)
ok  	iop/apps/node/internal/adapters	(cached)
ok  	iop/apps/node/internal/adapters/cli/lifecycle	(cached)
ok  	iop/apps/node/internal/adapters/cli/oneshot	(cached)
ok  	iop/apps/node/internal/adapters/cli/persistent	(cached)
ok  	iop/apps/node/internal/bootstrap	0.160s
ok  	iop/apps/node/internal/node	(cached)
ok  	iop/apps/node/internal/router	(cached)
ok  	iop/apps/node/internal/store	(cached)
ok  	iop/apps/node/internal/transport	(cached)
ok  	iop/packages/config	(cached)
```

## 리뷰 결과

PASS

### Findings

없음.

### 확인 내용

- `apps/node/internal/node/node.go`: foreground `OnRunRequest`가 adapter error를 호출자에게 반환하고, background 실행은 goroutine에서 store completion을 유지하는지 확인했다.
- `apps/node/internal/node/node_test.go`: foreground adapter failure와 foreground cancellation regression test가 새 계약을 고정하는지 확인했다.
- `apps/node/internal/store/store.go`: 기존 `runs` table에 대해 `session_id`, `background` column을 idempotent하게 추가하고 migration 실패 시 DB handle을 닫는지 확인했다.
- `apps/node/internal/store/store_test.go`: old schema migration과 idempotent reopen case가 regression test로 추가된 것을 확인했다.

### 리뷰 검증

```bash
$ go test ./apps/node/internal/node
ok  	iop/apps/node/internal/node	(cached)

$ go test ./apps/node/internal/store
ok  	iop/apps/node/internal/store	(cached)

$ go test ./...
ok  	iop/apps/edge/cmd/edge	(cached)
ok  	iop/apps/edge/internal/node	(cached)
ok  	iop/apps/edge/internal/transport	(cached)
ok  	iop/apps/node/internal/adapters	(cached)
ok  	iop/apps/node/internal/adapters/cli/lifecycle	(cached)
ok  	iop/apps/node/internal/adapters/cli/oneshot	(cached)
ok  	iop/apps/node/internal/adapters/cli/persistent	(cached)
ok  	iop/apps/node/internal/bootstrap	(cached)
ok  	iop/apps/node/internal/node	(cached)
ok  	iop/apps/node/internal/router	(cached)
ok  	iop/apps/node/internal/store	(cached)
ok  	iop/apps/node/internal/transport	(cached)
ok  	iop/packages/config	(cached)
```