131 lines
6.7 KiB
Text
131 lines
6.7 KiB
Text
<!-- task=cli_terminal_cycle plan=1 tag=REVIEW_API -->
|
|
|
|
# Code Review Reference - REVIEW_API
|
|
|
|
## 개요
|
|
|
|
date=2026-05-03
|
|
task=cli_terminal_cycle, plan=1, tag=REVIEW_API
|
|
|
|
## 이 파일을 읽는 리뷰 에이전트에게
|
|
|
|
각 항목의 구현을 실제 소스 파일과 대조하고, `검증 결과` 섹션의 출력이 코드와 일치하는지 확인하세요.
|
|
리뷰 완료 후 반드시 아래 순서로 아카이브하세요.
|
|
|
|
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_API-1] Persistent CLI idle complete는 첫 출력 이후에만 시작 | [x] |
|
|
| [REVIEW_API-2] Persistent process 종료/오류를 run 실패로 전파 | [x] |
|
|
| [REVIEW_API-3] Bootstrap에서 adapter start 이후 실패 cleanup 보장 | [x] |
|
|
|
|
## 계획 대비 변경 사항
|
|
|
|
없음. 계획의 before/after 패치를 그대로 적용했다.
|
|
|
|
## 주요 설계 결정
|
|
|
|
- `emitRuntimeError` helper를 추가해 error event emit + non-nil error 반환 중복을 제거했다.
|
|
- startup drain 이후 `select { case err := <-doneCh: ...; default: }` 패턴으로 non-blocking process exit 감지를 추가했다.
|
|
- bootstrap `reg.Start` 이동은 store 초기화 직후로 단순 순서 변경으로 완료됐고, `reg.Start` 실패 시 `st.Close()`도 추가해 store 누수를 막았다.
|
|
|
|
## 리뷰어를 위한 체크포인트
|
|
|
|
- persistent CLI idle timer가 첫 output 전에 complete를 발생시키지 않는지 확인한다.
|
|
- slow first output이 `response_idle_timeout_ms`보다 늦어도 `RunRequest.TimeoutSec` 안에서는 정상 delta/complete가 되는지 확인한다.
|
|
- persistent process exit, output channel close, prompt write failure가 error event와 non-nil error로 모두 전파되는지 확인한다.
|
|
- node run store가 persistent process failure를 completed가 아니라 failed로 저장할 수 있는 반환 경로인지 확인한다.
|
|
- `Start` 중 이미 종료된 persistent process를 성공 session으로 반환하지 않는지 확인한다.
|
|
- bootstrap에서 store 준비 전 adapter process가 시작되지 않고, adapter start 실패 시 session/store가 정리되는지 확인한다.
|
|
|
|
## 검증 결과
|
|
|
|
### REVIEW_API-1 중간 검증
|
|
```
|
|
$ go test ./apps/node/internal/adapters/cli
|
|
=== RUN TestCLIExecuteOneShotPassesPromptAsArg
|
|
--- PASS: TestCLIExecuteOneShotPassesPromptAsArg (0.00s)
|
|
=== RUN TestCLIExecutePersistentWaitsForSlowFirstOutput
|
|
--- PASS: TestCLIExecutePersistentWaitsForSlowFirstOutput (0.31s)
|
|
=== RUN TestCLIExecutePersistentProcessExitReturnsError
|
|
--- PASS: TestCLIExecutePersistentProcessExitReturnsError (0.06s)
|
|
=== RUN TestCLIStartPersistentReturnsErrorWhenProcessExitsDuringStartup
|
|
--- PASS: TestCLIStartPersistentReturnsErrorWhenProcessExitsDuringStartup (0.00s)
|
|
=== RUN TestCLIStartPersistentTerminalAndExecuteWritesPrompt
|
|
--- PASS: TestCLIStartPersistentTerminalAndExecuteWritesPrompt (0.36s)
|
|
PASS
|
|
ok iop/apps/node/internal/adapters/cli 0.746s
|
|
```
|
|
|
|
### REVIEW_API-2 중간 검증
|
|
```
|
|
$ go test ./apps/node/internal/adapters/cli
|
|
(위와 동일 — PASS)
|
|
```
|
|
|
|
### REVIEW_API-3 중간 검증
|
|
```
|
|
$ go test ./apps/node/internal/bootstrap ./apps/node/internal/adapters/cli ./apps/node/...
|
|
=== RUN TestModuleDoesNotStartAdaptersBeforeStoreReady
|
|
--- PASS: TestModuleDoesNotStartAdaptersBeforeStoreReady (0.16s)
|
|
PASS
|
|
ok iop/apps/node/internal/bootstrap 0.162s
|
|
ok iop/apps/node/internal/adapters/cli 0.721s
|
|
ok iop/apps/node/internal/adapters 0.003s
|
|
ok iop/apps/node/internal/node 0.006s
|
|
ok iop/apps/node/internal/transport 0.005s
|
|
```
|
|
|
|
### 최종 검증
|
|
```
|
|
$ go test ./apps/node/internal/adapters/cli
|
|
PASS ok iop/apps/node/internal/adapters/cli 0.746s
|
|
|
|
$ go test ./apps/node/internal/bootstrap ./apps/node/...
|
|
PASS ok iop/apps/node/internal/bootstrap 0.162s
|
|
|
|
$ go test ./...
|
|
ok iop/apps/edge/internal/node 0.003s
|
|
ok iop/apps/edge/internal/transport 0.005s
|
|
ok iop/apps/node/internal/adapters 0.003s
|
|
ok iop/apps/node/internal/adapters/cli 0.721s
|
|
ok iop/apps/node/internal/bootstrap 0.163s
|
|
ok iop/apps/node/internal/node 0.006s
|
|
ok iop/apps/node/internal/transport 0.005s
|
|
ok iop/packages/config 0.003s
|
|
```
|
|
|
|
---
|
|
|
|
## 코드리뷰 결과
|
|
|
|
### 종합 판정
|
|
|
|
FAIL
|
|
|
|
### 차원별 평가
|
|
|
|
| 차원 | 평가 | 근거 |
|
|
|------|------|------|
|
|
| correctness | Fail | 후속 세 항목 자체는 반영됐지만 lifecycle start가 부분 성공 후 실패할 때 이미 시작한 adapter/process를 정리하지 않는다. |
|
|
| completeness | Fail | "adapter start 실패 시 정리" 경로가 session/store에는 반영됐지만, partial-start adapter lifecycle에는 누락됐다. |
|
|
| test coverage | Fail | slow first output/process exit/bootstrap store failure 테스트는 추가됐으나 partial start rollback 테스트가 없다. |
|
|
| API contract | Pass | CLI profile 계약과 기존 adapter interface 호환성은 유지된다. |
|
|
| code quality | Warn | persistent session 상태 전파는 좋아졌지만 Start/Stop rollback 책임이 아직 분산되어 있다. |
|
|
| plan deviation | Fail | 후속 plan의 cleanup 의도에 비해 `reg.Start` 실패 시 이미 시작된 lifecycle 정리가 보장되지 않는다. |
|
|
| verification trust | Warn | 리뷰어가 `go test ./...`를 재실행했고 통과했지만, 남은 실패 경로가 테스트되지 않는다. |
|
|
|
|
### 발견된 문제
|
|
|
|
- Required: `apps/node/internal/adapters/registry.go:66`의 `Registry.Start`는 등록 순서대로 lifecycle adapter를 시작하다가 `apps/node/internal/adapters/registry.go:72`에서 실패하면 즉시 반환하지만, 앞서 성공한 lifecycle adapter의 `Stop`을 호출하지 않는다. 같은 문제가 `apps/node/internal/adapters/cli/cli.go:77`의 `CLI.Start` 내부에도 있다. 여러 persistent profile 중 하나가 먼저 시작된 뒤 다음 profile start가 실패하면 `apps/node/internal/adapters/cli/cli.go:83`에서 반환하면서 이미 시작한 process가 남는다. `apps/node/internal/bootstrap/module.go:61`도 `reg.Start` 실패 시 session/store만 닫고 `reg.Stop`을 호출하지 않아 rollback을 보강하지 못한다. 수정: `Registry.Start`에서 started lifecycle 목록을 추적해 실패 시 역순 `Stop`을 호출하고, `CLI.Start`도 profile start 실패 시 이미 시작한 sessions를 `Stop`으로 정리하며, bootstrap의 `reg.Start` 실패 경로에서도 `_ = reg.Stop(context.Background())`를 호출한다. registry와 CLI partial-start rollback 회귀 테스트를 추가한다.
|
|
|
|
### 다음 단계
|
|
|
|
FAIL: Required 항목을 반영하는 새 `PLAN.md`와 `CODE_REVIEW.md`를 작성해 후속 구현 루프를 계속한다.
|