toki/iop
1
0
Fork 0
Code Issues Pull requests Projects Releases Packages Wiki Activity Actions
iop/agent-task/unbounded_cli_sessions/code_review_0.log
toki 96f79fcd08 feat: unbounded CLI sessions support and related improvements 
- Add unbounded CLI sessions agent-task with plan and code review logs
- Add edge console with tests
- Add node run manager for session lifecycle management
- Add router and store tests
- Update transport session handling
- Add runtime proto definitions
- Update configurations and packages
2026-05-03 20:07:09 +09:00

227 lines
14 KiB
Text
Raw Blame History

<!-- task=unbounded_cli_sessions plan=0 tag=REFACTOR -->
# 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 <id>`, `/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`를 작성해 후속 구현 루프를 계속한다.
Reference in a new issue View git blame Copy permalink