# Code Review Reference - REFACTOR ## 개요 date=2026-05-03 task=unbounded_cli_sessions, plan=0, tag=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` 스텁 작성. --- ## 구현 항목별 완료 여부 | 항목 | 완료 여부 | |------|---------| | [REFACTOR-1] Protocol And Runtime Contract | [x] | | [REFACTOR-2] Node Run Manager And Background Execution | [x] | | [REFACTOR-3] CLI Adapter Unbounded Logical Sessions | [x] | | [REFACTOR-4] Edge Console, Config Payload, Docs | [x] | ### REFACTOR-1 체크리스트 - [x] `proto/iop/runtime.proto`: `RunSessionMode`, `CancelAction` enum, `RunRequest.session_id/session_mode/background`, `RunEvent.session_id/background`, `CancelRequest.adapter/model/session_id/action` 추가 - [x] `proto/gen/iop/runtime.pb.go`: `make proto`로 재생성 (직접 편집 없음) - [x] `apps/node/internal/runtime/types.go`: `SessionMode`, `CancelAction`, `DefaultSessionID`, `ErrRunCancelled`, `EventTypeCancelled`, `SessionTerminator`, `ExecutionSpec`/`RunRequest` 신규 필드 추가 - [x] `apps/node/internal/router/router.go`: `ExecutionSpec` 생성 시 `SessionID/SessionMode/Background` 전달 - [x] `apps/node/internal/node/node.go`: proto 변환 헬퍼 `sessionModeFromProto`, `cancelActionFromProto`, `normalizeSessionID` 추가 - [x] `apps/node/internal/transport/parser_test.go`: `SessionId/Background/SessionMode` 및 `CancelRequest` 신규 필드 roundtrip 검증 추가 - [x] `apps/node/internal/router/router_test.go`: `TestResolve_PreservesSessionFields` 신규 파일 작성 ### REFACTOR-2 체크리스트 - [x] `apps/node/internal/node/run_manager.go`: `runHandle`, `runManager` 신규 파일 — transport 분리된 active run registry - [x] `apps/node/internal/node/node.go`: `runManager` 필드, background goroutine 실행, cancel/terminate 분기, cancelled status 처리, `sessionSink`에 `sessionID/background` 컨텍스트 추가 - [x] `apps/node/internal/transport/session.go`: `cancelFns`/`RegisterCancel`/`DeregisterCancel`/`CancelRun` 완전 제거. transport는 send/receive/connection 책임만 유지 - [x] `apps/node/internal/transport/session_test.go`: cancel registry 테스트 제거, `SetHandler` 동시성 안전 테스트로 교체 - [x] `apps/node/internal/node/node_test.go`: `fixedRouter`에 session 필드 전달, `TestOnRunRequest_BackgroundReturnsBeforeAdapterCompletes`, `TestOnCancel_CancelsRunViaRunManager`, `TestOnCancel_TerminatesAdapterSession` 추가 - [x] `apps/node/internal/store/store.go`: `session_id`, `background` 컬럼 스키마 추가, `InsertRun`/`GetRun` 갱신 - [x] `apps/node/internal/store/store_test.go`: `TestStore_RunSessionFields`, `TestStore_DefaultSessionID` 신규 작성 ### REFACTOR-3 체크리스트 - [x] `apps/node/internal/adapters/cli/cli.go`: `sessionKey{model, sessionID}`, `sessions map[sessionKey]*profileSession`로 교체, `Start`는 default session prestart, `Stop`/`stopAllSessions` sessionKey 기반 반복, `TerminateSession` 구현, `closeProfileSession` 헬퍼 추출 - [x] `apps/node/internal/adapters/cli/persistent.go`: `resolveSession` lazy creation, `startProfileSession` signature를 `sessionKey`로 변경, cancel 시 process kill 대신 `drainSessionUntilIdle` + `ErrRunCancelled` 반환, process exit 시 sessionKey 기준으로 map 정리 - [x] `apps/node/internal/adapters/cli/oneshot.go`: session 무관(그대로 유지), compile 영향 없음 - [x] `apps/node/internal/adapters/cli/persistent/execute_test.go`: `TestCLIExecutePersistentTimeoutCleansSession` → `TestCLIExecutePersistentCancelDoesNotTerminateSession`으로 기대값 반전, `TestCLIExecutePersistentCreatesIndependentSessionsForSameProfile`, `TestCLIExecutePersistentRequireExistingSessionFailsWhenMissing`, `TestCLIExecutePersistentMaintainsHundredLogicalSessions` 추가 - [x] `apps/node/internal/adapters/cli/lifecycle/cli_test.go`: rollback 테스트를 `SessionModeRequireExisting`으로 검증하도록 수정, `TestCLITerminateSessionStopsOnlyTargetSession`, `TestCLIStopStopsAllLogicalSessions` 추가 ### REFACTOR-4 체크리스트 - [x] `packages/config/config.go`: `EdgeConsoleConf.SessionID`, `EdgeConsoleConf.Background` 추가, `setEdgeDefaults`에 기본값 등록 - [x] `packages/config/config_test.go`: `TestLoadEdge_ConsoleSessionDefaults`, `TestLoadEdge_ConsoleSessionOverride` 추가 - [x] `apps/edge/cmd/edge/console.go`: `/session `, `/background on|off`, `/terminate-session` 명령 추가, `buildRunRequest` 헬퍼 추출, `RunRequest`에 `SessionId/SessionMode/Background` 설정 - [x] `apps/edge/cmd/edge/console_test.go`: `buildRunRequest` 단위 테스트 신규 작성 - [x] `apps/edge/internal/transport/server.go`: CLI profile config payload 그대로 유지 (신규 필드 없음), node id당 1 connection 거절 로직 유지 - [x] `configs/edge.yaml`: `console.session_id`, `console.background` 추가, `codex` profile을 `persistent: true` 예시로 변경 - [x] `apps/node/README.md`: logical session / cancel vs terminate 차이 문서화 - [x] `apps/edge/README.md`: console 명령 표, transport 1개·session 여러 개 설명 추가 --- ## 계획 대비 변경 사항 1. **lifecycle 테스트 rollback 검증 방식 변경**: 계획에서는 rollback 후 `Execute`가 실패해야 한다고 명시했으나, lazy 세션 생성 도입으로 `Execute`는 성공할 수 있다. rollback 후 pre-started default session이 실제로 정리됐는지 확인하기 위해 `SessionModeRequireExisting` 모드로 검증하도록 수정했다. 2. **`TestCLIExecutePersistentCancelDoesNotTerminateSession` ResponseIdleTimeoutMS**: 계획의 `slow-echo` 예시에 `ResponseIdleTimeoutMS: 5000`이 쓰였으나, 이 값은 drain timeout이 5초여서 두 번째 execute가 타임아웃됐다. `150ms`로 줄여 drain이 빠르게 완료되도록 수정했다. 3. **`session_test.go` 교체 범위**: 계획에는 "cancel registry 테스트 제거/대체, transport는 parser/send/connection 책임만"이라고 명시됐고, 이에 따라 `RegisterCancel/CancelRun/DeregisterCancel` 관련 3개 테스트를 `SetHandler` 동시성 안전 단일 테스트로 교체했다. 4. **`store.go` 마이그레이션 방식**: 계획에서는 "schema migration 추가"라고 했으나 기존 `CREATE TABLE IF NOT EXISTS`가 이미 migration 역할을 한다. 신규 컬럼은 기존 DB에 자동 적용되지 않으므로, 처음부터 새 스키마로 생성하는 방식을 유지했다. 기존 운영 DB 마이그레이션이 필요하다면 `ALTER TABLE` migration이 별도로 필요하다 (현재 프로젝트는 dev 단계라 해당 없음). --- ## 주요 설계 결정 1. **cancel-drain 중 `sess.mu` 보유**: `executePersistent`는 `sess.mu`를 잠근 채 drain을 수행한다. 같은 session의 다음 run은 drain 완료 후 lock을 획득한다. drain timeout이 짧으면 두 번째 run이 빠르게 진입하고, 길면 대기한다. 이 설계는 session ouptut channel을 안전하게 drain할 수 있는 장점이 있다. 2. **`ErrRunCancelled` 도입**: cancel 결과를 `ctx.Err()`(context.Canceled)와 구분하기 위해 별도 sentinel 에러를 도입했다. `node.completeRun`에서 이를 감지해 store status를 `cancelled`로 기록한다. 3. **`Start()`는 default session만 prestart**: persistent profile의 default(`sessionID=""`) session만 미리 시작하고, 추가 session_id는 RunRequest가 들어올 때 lazy-create한다. `SessionModeRequireExisting`을 사용하면 lazy 창조를 막을 수 있다. 4. **transport cancel registry 완전 제거**: `transport.Session`은 TCP/프레임 전송만 담당하고, run lifecycle ownership은 `node.runManager`가 갖는다. run cancel은 `node.OnCancel` → `runManager.cancelRun` 경로만 존재한다. --- ## 리뷰어를 위한 체크포인트 - edge-node transport registry가 node id당 connection 1개 모델을 유지하는지 확인한다. - CLI logical session 수에 `pool_size`, `max_sessions`, hard limit이 추가되지 않았는지 확인한다. - same model/profile + different `session_id`가 서로 다른 persistent worker process로 유지되는지 확인한다. - `cancel run`이 session terminate와 섞이지 않고, `CancelActionTerminateSession`만 worker process를 종료하는지 확인한다. - background run이 `OnRunRequest` 반환 후에도 run completion/store update/event streaming을 처리하는지 확인한다. - protobuf generated file은 `make proto` 결과인지 확인하고 직접 편집 흔적이 없는지 확인한다. - store migration이 기존 SQLite DB에도 안전하게 적용되는지 확인한다. - 100 logical sessions regression test가 short mode policy와 resource cleanup을 명확히 처리하는지 확인한다. --- ## 검증 결과 ### REFACTOR-1 중간 검증 ``` $ make proto protoc \ --go_out=. \ --go_opt=module=iop \ --proto_path=. \ proto/iop/runtime.proto \ proto/iop/node.proto \ proto/iop/control.proto \ proto/iop/job.proto $ go test ./apps/node/internal/runtime ./apps/node/internal/router ./apps/node/internal/transport ? iop/apps/node/internal/runtime [no test files] ok iop/apps/node/internal/router 0.004s ok iop/apps/node/internal/transport 0.006s ``` ### REFACTOR-2 중간 검증 ``` $ go test ./apps/node/internal/node ./apps/node/internal/transport ./apps/node/internal/store ok iop/apps/node/internal/node 0.007s ok iop/apps/node/internal/transport 0.006s ok iop/apps/node/internal/store 0.005s ``` ### REFACTOR-3 중간 검증 ``` $ go test ./apps/node/internal/adapters/cli/... ? iop/apps/node/internal/adapters/cli [no test files] ? iop/apps/node/internal/adapters/cli/internal/testutil [no test files] ok iop/apps/node/internal/adapters/cli/lifecycle 2.281s ok iop/apps/node/internal/adapters/cli/oneshot 0.071s ok iop/apps/node/internal/adapters/cli/persistent 63.298s ``` ### REFACTOR-4 중간 검증 ``` $ go test ./packages/config ./apps/edge/internal/transport ./apps/edge/cmd/edge ok iop/packages/config 0.004s ok iop/apps/edge/internal/transport 0.007s ok iop/apps/edge/cmd/edge 0.006s ``` ### 최종 검증 ``` $ make proto protoc \ --go_out=. \ --go_opt=module=iop \ --proto_path=. \ proto/iop/runtime.proto \ proto/iop/node.proto \ proto/iop/control.proto \ proto/iop/job.proto $ gofmt -w apps/node/internal/runtime/types.go apps/node/internal/router/router.go ... (no output — all files already formatted) $ go test -count=1 ./... ? iop/apps/control-plane/cmd/control-plane [no test files] ok iop/apps/edge/cmd/edge 0.006s ? iop/apps/edge/internal/bootstrap [no test files] ok iop/apps/edge/internal/node 0.004s ok iop/apps/edge/internal/transport 0.007s ? iop/apps/node/cmd/node [no test files] ok iop/apps/node/internal/adapters 0.006s ? iop/apps/node/internal/adapters/cli [no test files] ? iop/apps/node/internal/adapters/cli/internal/testutil [no test files] ok iop/apps/node/internal/adapters/cli/lifecycle 2.275s ok iop/apps/node/internal/adapters/cli/oneshot 0.079s ok iop/apps/node/internal/adapters/cli/persistent 63.226s ? iop/apps/node/internal/adapters/mock [no test files] ? iop/apps/node/internal/adapters/ollama [no test files] ? iop/apps/node/internal/adapters/vllm [no test files] ok iop/apps/node/internal/bootstrap 0.159s ok iop/apps/node/internal/node 0.007s ok iop/apps/node/internal/router 0.004s ? iop/apps/node/internal/runtime [no test files] ok iop/apps/node/internal/store 0.004s ok iop/apps/node/internal/transport 0.006s ? iop/apps/worker/cmd/worker [no test files] ? iop/packages/auth [no test files] ok iop/packages/config 0.004s ? iop/packages/jobs [no test files] ? iop/packages/metadata [no test files] ? iop/packages/observability [no test files] ? iop/packages/policy [no test files] ? iop/packages/version [no test files] ? iop/proto/gen/iop [no test files] ``` --- ## 코드리뷰 결과 ### 종합 판정: FAIL ### 차원별 평가 | 차원 | 평가 | |------|------| | Correctness | Fail | | Completeness | Fail | | Test coverage | Fail | | API contract | Fail | | Code quality | Pass | | Plan deviation | Warn | | Verification trust | Pass | ### 발견된 문제 - Required: `apps/node/internal/node/node.go:125`에서 foreground run의 `adapter.Execute` 에러를 `completeRun`에만 넘기고, `apps/node/internal/node/node.go:133`에서 항상 `nil`을 반환합니다. 기존 `OnRunRequest` 계약은 synchronous execute 실패를 호출자에게 반환했고, 계획의 foreground pseudocode도 `execErr` 반환을 요구했습니다. 이 상태에서는 transport listener가 run failure를 error로 관측하지 못합니다. 수정: `run` closure가 `error`를 반환하게 하고 `spec.Background == false` 경로에서는 `return run()`으로 adapter error를 보존하세요. `ErrRunCancelled`를 foreground caller에 반환할지 여부도 명시 테스트로 고정하세요. - Required: `apps/node/internal/store/store.go:54`는 `CREATE TABLE IF NOT EXISTS`만 실행하고 기존 `runs` table에 새 컬럼을 추가하지 않습니다. 그런데 `apps/node/internal/store/store.go:77`과 `apps/node/internal/store/store.go:95`는 항상 `session_id`, `background` 컬럼을 사용하므로, 기존 workspace DB에서는 `no such column: session_id` 또는 insert failure가 발생합니다. 계획의 리뷰 체크포인트였던 "기존 SQLite DB에도 안전한 migration"이 충족되지 않았습니다. 수정: `Store.New`에서 `ALTER TABLE runs ADD COLUMN session_id TEXT NOT NULL DEFAULT 'default'`, `ALTER TABLE runs ADD COLUMN background INTEGER NOT NULL DEFAULT 0`를 idempotent하게 실행하고, 구 스키마 DB fixture 테스트를 추가하세요. ### 다음 단계 FAIL: Required 이슈를 반영한 새 `PLAN.md`와 `CODE_REVIEW.md`를 작성해 후속 구현 루프를 계속한다.